Messages in this thread | | | Subject | Re: [PATCH] arm64: PCI: Enable SMC conduit | From | Jeremy Linton <> | Date | Thu, 7 Jan 2021 13:45:45 -0600 |
| |
Hi,
On 1/7/21 11:36 AM, Rob Herring wrote: > On Thu, Jan 7, 2021 at 9:24 AM Jeremy Linton <jeremy.linton@arm.com> wrote: >> >> Hi, >> >> >> On 1/7/21 9:28 AM, Rob Herring wrote: >>> On Mon, Jan 4, 2021 at 9:57 PM Jeremy Linton <jeremy.linton@arm.com> wrote: >>>> >>>> Given that most arm64 platform's PCI implementations needs quirks >>>> to deal with problematic config accesses, this is a good place to >>>> apply a firmware abstraction. The ARM PCI SMMCCC spec details a >>>> standard SMC conduit designed to provide a simple PCI config >>>> accessor. This specification enhances the existing ACPI/PCI >>>> abstraction and expects power, config, etc functionality is handled
(trimming)
>>>> >>>> +static int smccc_pcie_check_conduit(u16 seg) >>> >>> check what? Based on how you use this, perhaps _has_conduit() instead. >> >> Sure. >> >>> >>>> +{ >>>> + struct arm_smccc_res res; >>>> + >>>> + if (arm_smccc_1_1_get_conduit() == SMCCC_CONDUIT_NONE) >>>> + return -EOPNOTSUPP; >>>> + >>>> + arm_smccc_smc(SMCCC_PCI_VERSION, 0, 0, 0, 0, 0, 0, 0, &res); >>>> + if ((int)res.a0 < 0) >>>> + return -EOPNOTSUPP; >>>> + >>>> + arm_smccc_smc(SMCCC_PCI_SEG_INFO, seg, 0, 0, 0, 0, 0, 0, &res); >>>> + if ((int)res.a0 < 0) >>>> + return -EOPNOTSUPP; >>> >>> Don't you need to check that read and write functions are supported? >> >> In theory no, the first version of the specification makes them >> mandatory for all implementations. There isn't a partial access method, >> so nothing works if only read or write were implemented. > > Then the spec should change: > > 2.3.3 Caller responsibilities > The caller has the following responsibilities: > • The caller must ensure that this function is implemented before > issuing a call. This function is discoverable > by calling PCI_FEATURES with pci_func_id set to 0x8400_0132. > > > I guess knowing the version is ensuring, but the 2nd sentence makes it > seem that is how one should ensure.
Ok, yes i understand, I will add the check.
> > Related, are there any sort of tests for the interface? I generally > don't think the kernel's job is validating firmware (a frequent topic > for DT), but we should have something. Maybe an SMC unittest module? > If nothing else, seems like we should have at least one PCI_FEATURES > call to make sure it works. We don't want to add it later only to find > that it breaks on some firmware implementations. Though we can just > add firmware quirks. ;)
I'm not aware of any unit tests at the moment. My testing so far has been against these patches: https://review.trustedfirmware.org/q/topic:"Arm_PCI_Config_Space_Interface"
But given the next version does the PCI_FEATURES calls, that will satisfy your concern, right?
| |