Hi Linus
I'm working on gpio for an AMD Family 16h Model 30h system[1]. The SoC is the same as the GX412-TC used in the PC Engines APU2.
There is an out-of-tree gpio driver (gpio-amd) for this SoC in the meta-amd yocto layer[2].
Another driver (gpio-sb8xx) was submitted for upstream inclusion, but was knocked back with the suggestion that pinctrl is the way forward[3].
I would much prefer to use a mainline driver for the system I'm working on, so I'm looking at the pinctrl-amd driver to see whether it applies to our SoC, or whether it could be extended, or used as starting point for a new driver.
The out-of-tree drivers apply to the GX412-TC SoC and uses PCI for probing:
gpio-amd registers a platform driver that applies to { PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_HUDSON2_SMBUS }
gpio-sb8xx applies to { PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_HUDSON2_SMBUS } and { PCI_DEVICE(PCI_VENDOR_ID_ATI, PCI_DEVICE_ID_ATI_SBX00_SMBUS }
These IDs make it easy to cross reference with the datasheet, and keeps the coupling between ACPI and the driver low.
These drivers do not provide a mechanism for firmware (ACPI or DT) to specify which gpios are safe to use or how to use them.
In contrast, the pinctrl-amd driver only mentions the newer KERNCZ platform name and uses ACPI for probing without disclosing any Family or Model numbers.
pinctrl-amd applies to "AMD0030" and "AMDI0030"
The ACPI HID matching makes it difficult to determine what family and model the driver applies to, or rather, I have not been able to find such a mapping of HIDs to family and model numbers. It's also impossible to guess an ACPI _HID that may or may not exist for the Family 16h Model 30h platform and even if I allocate a new HID for our ACPI implementation, that HID has little hope of being accepted into the mainline driver.
I would like to extend the generically named, but very specifically implemented pinctrl-amd driver to also work on Family 16h, Model 30h (specifically the FT3b package), and I propose to use the PCI_DEVICE_ID_AMD_16H_M30H_NB_F3 symbol to probe for the device.
Does this seem like a sensible way forward?
Thanks!
Andrew
1. http://support.amd.com/TechDocs/52740_16h_Models_30h-3Fh_BKDG.pdf
2. http://git.yoctoproject.org/cgit/cgit.cgi/meta-amd/tree/meta-steppeeagle/recipes-kernel/amd-gpio/files/gpio-amd.c
3. https://lkml.org/lkml/2015/6/20/202
Hi Andrew!
Thank you for your mail!
On Wed, Dec 20, 2017 at 11:25 PM, Andrew Cooks
<[email protected]> wrote:
> I'm working on gpio for an AMD Family 16h Model 30h system[1].
> The SoC is the same as the GX412-TC used in the PC Engines APU2.
OK
> There is an out-of-tree gpio driver (gpio-amd) for this SoC in the meta-amd yocto layer[2].
That is bad. It needs to be upstream.
I have to try very hard to avoid sarcasm about it "seeming like a good idea
at the time" and silly things like that.
What we are seeing is breakage of social norms, in this case,
upstream first. :(
> Another driver (gpio-sb8xx) was submitted for upstream inclusion, but was
> knocked back with the suggestion that pinctrl is the way forward[3].
Hm I cannot follow link [3] right now. And I don't remember the submission :(
It doesn't seem to be in my mail archives either.
> I would much prefer to use a mainline driver for the system I'm working
> on, so I'm looking at the pinctrl-amd driver to see whether it applies to our
> SoC, or whether it could be extended, or used as starting point for a new driver.
This is the right spirit!
> The out-of-tree drivers apply to the GX412-TC SoC and uses PCI for probing:
>
> gpio-amd registers a platform driver that applies to { PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_HUDSON2_SMBUS }
> gpio-sb8xx applies to { PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_HUDSON2_SMBUS } and { PCI_DEVICE(PCI_VENDOR_ID_ATI, PCI_DEVICE_ID_ATI_SBX00_SMBUS }
>
> These IDs make it easy to cross reference with the datasheet, and keeps the coupling between ACPI and the driver low.
> These drivers do not provide a mechanism for firmware (ACPI or DT) to specify which gpios are safe to use or how to use them.
The Intel pin control driver has gone to lengths to avoid using unusable
ACPI-controlled GPIOs. Thought they were mostly concerned with
GPIOs not available for IRQs. This is what the "valid_mask" in
struct gpio_irq_chip is for.
Timur Tabi from CodeAurora is currently
floating a patch set that makes it possible to remove entire sets of
lines as "controlled by someone else" (read: ACPI BIOS).
This will apply to the whole GPIO chip.
https://marc.info/?l=linux-gpio&m=151379704013464&w=2
This is however under heavy discussion.
So there is work being done to mask out sets of GPIOs that
are used by ACPI. (Or secure worlds or whatever.)
> In contrast, the pinctrl-amd driver only mentions the newer KERNCZ platform
> name and uses ACPI for probing without disclosing any Family or Model numbers.
>
> pinctrl-amd applies to "AMD0030" and "AMDI0030"
>
> The ACPI HID matching makes it difficult to determine what family and model the
> driver applies to, or rather, I have not been able to find such a mapping of HIDs
> to family and model numbers. It's also impossible to guess an ACPI _HID
> that may or may not exist for the Family 16h Model 30h platform and even if I
> allocate a new HID for our ACPI implementation, that HID has little hope of
> being accepted into the mainline driver.
I didn't understand anything of what you just wrote.
I am basically ignorant when it comes to ACPI details.
So let's CC the GPIO ACPI maintainer, Mika Westerberg.
> I would like to extend the generically named, but very specifically implemented
> pinctrl-amd driver to also work on Family 16h, Model 30h (specifically the FT3b
> package), and I propose to use the PCI_DEVICE_ID_AMD_16H_M30H_NB_F3
> symbol to probe for the device.
>
> Does this seem like a sensible way forward?
Sounds good to me.
Any ACPI questions, I hope Mika can help out with.
The AMD folks are sometimes silent, I would say just hack on it so
we get somewhere with this!
Your effort is appreciated.
Yours,
Linus Walleij
On Thu, Dec 21, 2017 at 11:11:18AM +0100, Linus Walleij wrote:
> > In contrast, the pinctrl-amd driver only mentions the newer KERNCZ platform
> > name and uses ACPI for probing without disclosing any Family or Model numbers.
> >
> > pinctrl-amd applies to "AMD0030" and "AMDI0030"
> >
> > The ACPI HID matching makes it difficult to determine what family and model the
> > driver applies to, or rather, I have not been able to find such a mapping of HIDs
> > to family and model numbers. It's also impossible to guess an ACPI _HID
> > that may or may not exist for the Family 16h Model 30h platform and even if I
> > allocate a new HID for our ACPI implementation, that HID has little hope of
> > being accepted into the mainline driver.
>
> I didn't understand anything of what you just wrote.
> I am basically ignorant when it comes to ACPI details.
>
> So let's CC the GPIO ACPI maintainer, Mika Westerberg.
If the hardware is not the same that is already supported by the
pinctrl-amd, then you definitely should allocate a new separate ACPI
_HID for it. That's pretty much what we do with every new SoC because
they typically don't have identical pin lists among other things.
On Thursday, December 21, 2017 8:25:03 AM CET Andrew Cooks wrote:
> I'm working on gpio for an AMD Family 16h Model 30h system[1].
> The SoC is the same as the GX412-TC used in the PC Engines APU2.
>
> There is an out-of-tree gpio driver (gpio-amd) for this SoC in the
> meta-amd yocto layer[2]. Another driver (gpio-sb8xx) was submitted
> for upstream inclusion, but was knocked back with the suggestion
> that pinctrl is the way forward[3].
>
> I would much prefer to use a mainline driver for the system I'm
> working on, so I'm looking at the pinctrl-amd driver to see
> whether it applies to our SoC, or whether it could be extended,
> or used as starting point for a new driver.
>
> The out-of-tree drivers apply to the GX412-TC SoC and uses PCI
> for probing:
Just a FYI: due to these difficulties with getting a gpio driver
upstream, Alan Mizrahi upstreamed an in-kernel led-apu.c driver [0]
that sort of bypasses the whole pinctrl vs gpio issue.
|#define APU1_FCH_ACPI_MMIO_BASE 0xFED80000
|#define APU1_FCH_GPIO_BASE (APU1_FCH_ACPI_MMIO_BASE + 0x01BD)
|#define APU1_LEDON 0x08
|#define APU1_LEDOFF 0xC8
|#define APU1_NUM_GPIO 3
|#define APU1_IOSIZE sizeof(u8)
|
|#define APU2_FCH_ACPI_MMIO_BASE 0xFED80000
|#define APU2_FCH_GPIO_BASE (APU2_FCH_ACPI_MMIO_BASE + 0x1500)
|#define APU2_GPIO_BIT_WRITE 22
|#define APU2_APU2_NUM_GPIO 4
|#define APU2_IOSIZE sizeof(u32)
If you are just after LEDs or gpio-keys you probably can go the same
route?
Thanks,
Christian
[0] <https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/leds/leds-apu.c>
On 21/12/17 22:12, Mika Westerberg wrote:
> On Thu, Dec 21, 2017 at 11:11:18AM +0100, Linus Walleij wrote:
>>> In contrast, the pinctrl-amd driver only mentions the newer KERNCZ platform
>>> name and uses ACPI for probing without disclosing any Family or Model numbers.
>>>
>>> pinctrl-amd applies to "AMD0030" and "AMDI0030"
>>>
>>> The ACPI HID matching makes it difficult to determine what family and model the
>>> driver applies to, or rather, I have not been able to find such a mapping of HIDs
>>> to family and model numbers. It's also impossible to guess an ACPI _HID
>>> that may or may not exist for the Family 16h Model 30h platform and even if I
>>> allocate a new HID for our ACPI implementation, that HID has little hope of
>>> being accepted into the mainline driver.
>>
>> I didn't understand anything of what you just wrote.
>> I am basically ignorant when it comes to ACPI details.
>>
>> So let's CC the GPIO ACPI maintainer, Mika Westerberg.
>
> If the hardware is not the same that is already supported by the
> pinctrl-amd, then you definitely should allocate a new separate ACPI
> _HID for it. That's pretty much what we do with every new SoC because
> they typically don't have identical pin lists among other things.
>
Right. I wonder if my reasons for objecting to using ACPI _HID in this way (as the existing drivers do) are being overlooked, or if there's something missing in my understanding.
Given the HID "AMDI0030", it is difficult for anyone besides AMD to determine what SoC that is intended to match. Joe Random Developer would not be able to find the datasheet for the SoC and verify that this driver works correctly, or whether it is used by any firmware at all.
However, my specific problem is the inverse. Given a SoC without vendor-supplied HID or DSDT ASL (ie. I'm writing the DSDT ASL), I don't know what HID to allocate for the driver and DSDT, and I'm too low in the food chain to allocate the one true HID for this SoC that every firmware developer and driver developer should use. This is not a problem for out-of-tree patches, but it blocks me from contributing usable support for a new SoC.
So my objection is that the coupling between the driver and ACPI firmware, caused by these special HID strings, is both unnecessary and disempowering. If an appropriate ID register exists in the MMIO space, I think that would solve this issue.
I'm hoping to confirm whether my assessment is correct.
Thanks
a.
Hi Linus
On 21/12/17 20:11, Linus Walleij wrote:
> Hi Andrew!
>
> Thank you for your mail!
>
> On Wed, Dec 20, 2017 at 11:25 PM, Andrew Cooks
> <[email protected]> wrote:
>
>> Another driver (gpio-sb8xx) was submitted for upstream inclusion, but was
>> knocked back with the suggestion that pinctrl is the way forward[3].
>
> Hm I cannot follow link [3] right now. And I don't remember the submission :(
> It doesn't seem to be in my mail archives either.
Here's the same thread in a different list archive: https://marc.info/?t=143483759600001&r=1&w=2
>
> Timur Tabi from CodeAurora is currently
> floating a patch set that makes it possible to remove entire sets of
> lines as "controlled by someone else" (read: ACPI BIOS).
> This will apply to the whole GPIO chip.
> https://marc.info/?l=linux-gpio&m=151379704013464&w=2
>
> This is however under heavy discussion.
Thanks, I'm following that discussion.
Andrew
Hi Tobias
On 21/12/17 23:06, Tobias Diedrich wrote:
>
> > Another driver (gpio-sb8xx) was submitted for upstream inclusion, but was
> > knocked back with the suggestion that pinctrl is the way forward[3].
>
> Hm I cannot follow link [3] right now. And I don't remember the submission :(
> It doesn't seem to be in my mail archives either.
>
>
> FWIW I wrote the gpio-sb8xx driver mostly to allow toggling the (board-specific) APU1 leds and didn't have time to rewrite it for the pinctrl api (also it was more of a "would be neat to have", so I didn't push it much).
> It would actually be neat to have ACPI support for arbitrary leds in some better way (toggling the pinctrl bits could easily be implemented in ACPI if there was some method called to enabled/disable the led).>
> The coreboot bits were submitted:
> https://github.com/coreboot/coreboot/blob/master/src/mainboard/pcengines/apu1/acpi/leds.asl
> https://github.com/coreboot/coreboot/blob/master/src/mainboard/pcengines/apu1/acpi/gpio.asl
> The latter is specifically asking for "gpio-sb8xx" (or compatible) (this is essentially an ACPI-provided devicetree snippet).
Our ACPI LED support is on a I2C GPIO expander, but looks similar. I'll submit that to coreboot after the holidays. We use the SoC gpios for buttons and other peripherals.
a.
Hi Christian
On 21/12/17 23:02, Christian Lamparter wrote:
> On Thursday, December 21, 2017 8:25:03 AM CET Andrew Cooks wrote:
>> I'm working on gpio for an AMD Family 16h Model 30h system[1].
>> The SoC is the same as the GX412-TC used in the PC Engines APU2.
>>
>> There is an out-of-tree gpio driver (gpio-amd) for this SoC in the
>> meta-amd yocto layer[2]. Another driver (gpio-sb8xx) was submitted
>> for upstream inclusion, but was knocked back with the suggestion
>> that pinctrl is the way forward[3].
>>
>> I would much prefer to use a mainline driver for the system I'm
>> working on, so I'm looking at the pinctrl-amd driver to see
>> whether it applies to our SoC, or whether it could be extended,
>> or used as starting point for a new driver.
>>
>> The out-of-tree drivers apply to the GX412-TC SoC and uses PCI
>> for probing:
>
> Just a FYI: due to these difficulties with getting a gpio driver
> upstream, Alan Mizrahi upstreamed an in-kernel led-apu.c driver [0]
> that sort of bypasses the whole pinctrl vs gpio issue.
Thanks, I saw that and was somewhat surprised to see it accepted.
>
> If you are just after LEDs or gpio-keys you probably can go the same
> route?
I could. The gpio-amd driver in the meta-amd yocto layer, which is already functional, is another workable alternative. However, I would prefer the more elegant mainline pinctrl solution, if it's attainable.
Thanks
Andrew
On Fri, Dec 22, 2017 at 10:37:32AM +1000, Andrew Cooks wrote:
>
>
> On 21/12/17 22:12, Mika Westerberg wrote:
> > On Thu, Dec 21, 2017 at 11:11:18AM +0100, Linus Walleij wrote:
> >>> In contrast, the pinctrl-amd driver only mentions the newer KERNCZ platform
> >>> name and uses ACPI for probing without disclosing any Family or Model numbers.
> >>>
> >>> pinctrl-amd applies to "AMD0030" and "AMDI0030"
> >>>
> >>> The ACPI HID matching makes it difficult to determine what family and model the
> >>> driver applies to, or rather, I have not been able to find such a mapping of HIDs
> >>> to family and model numbers. It's also impossible to guess an ACPI _HID
> >>> that may or may not exist for the Family 16h Model 30h platform and even if I
> >>> allocate a new HID for our ACPI implementation, that HID has little hope of
> >>> being accepted into the mainline driver.
> >>
> >> I didn't understand anything of what you just wrote.
> >> I am basically ignorant when it comes to ACPI details.
> >>
> >> So let's CC the GPIO ACPI maintainer, Mika Westerberg.
> >
> > If the hardware is not the same that is already supported by the
> > pinctrl-amd, then you definitely should allocate a new separate ACPI
> > _HID for it. That's pretty much what we do with every new SoC because
> > they typically don't have identical pin lists among other things.
> >
>
> Right. I wonder if my reasons for objecting to using ACPI _HID in this
> way (as the existing drivers do) are being overlooked, or if there's
> something missing in my understanding.
No, we don't object valid ACPI IDs. Where did you learn that?
> Given the HID "AMDI0030", it is difficult for anyone besides AMD to
> determine what SoC that is intended to match. Joe Random Developer
> would not be able to find the datasheet for the SoC and verify that
> this driver works correctly, or whether it is used by any firmware at
> all.
>
> However, my specific problem is the inverse. Given a SoC without
> vendor-supplied HID or DSDT ASL (ie. I'm writing the DSDT ASL), I
> don't know what HID to allocate for the driver and DSDT, and I'm too
> low in the food chain to allocate the one true HID for this SoC that
> every firmware developer and driver developer should use. This is not
> a problem for out-of-tree patches, but it blocks me from contributing
> usable support for a new SoC.
I think there is a process that allows you to allocate _HID for your
device buried somewhere in UEFI forums.
There is also PRP0001 _HID that can be used with Device Tree compatible
devices (see Documentation/acpi/enumeration.txt that allows you to use
DT compatible string instead.
> So my objection is that the coupling between the driver and ACPI
> firmware, caused by these special HID strings, is both unnecessary and
> disempowering. If an appropriate ID register exists in the MMIO space,
> I think that would solve this issue.
Well, in order to access the MMIO your device needs to be enumerated by
the kernel. In order to load a driver for the device you need to
have some sort of identifier for the thing. Yes, you can also create
a "board file" that has this information and then creates platform
device for your driver but that sort of things we try to avoid by using
firmware to describe devices.
On Fri, Dec 22, 2017 at 2:17 AM, Andrew Cooks <[email protected]> wrote:
> On 21/12/17 23:02, Christian Lamparter wrote:
>> Just a FYI: due to these difficulties with getting a gpio driver
>> upstream, Alan Mizrahi upstreamed an in-kernel led-apu.c driver [0]
>> that sort of bypasses the whole pinctrl vs gpio issue.
>
> Thanks, I saw that and was somewhat surprised to see it accepted.
I looked at the driver and it seems actually good and doing the
right thing. If I understand it right:
- It does a bunch of magic dmi_match() on DMI_BOARD_NAME
to figure out what board this is.
- It then proceeds to register LEDs using some bits in the MMIO
area that are used for LEDs.
So these bits/lines are actually not GPIO, since GPIO means
"general purpose input/output" - they are specific purpose and the
specific purpose can be detected.
So I don't have any objections from an architectural point of view.
Some may expose the register as GPIO and add LEDs on top but
it (using drivers/leds/leds-gpio.c) but that may be pointless extra
abstraction.
It has a deeper problem though. It is writing these GPIO registers
without any thought of someone else possibly accessing them and
without any shared locks.
This problem is usually solved by either using GPIO inbetween if
the GPIO driver takes care of protecting the register, or using
regmap, preferably with drivers/mfd/syscon.c, so that regmap will
protect the MMIO register.
So there may be technical reasons to use GPIO or syscon abstractions
that are not architectural reasons if you see what I mean.
If only these few GPIO lines are actually used and only used for
these LEDs, (the rest of the bits in the register unused) then this
driver is as good as any.
Yours,
Linus Walleij
On Friday, December 22, 2017 8:48:22 AM CET Linus Walleij wrote:
> On Fri, Dec 22, 2017 at 2:17 AM, Andrew Cooks <[email protected]> wrote:
> > On 21/12/17 23:02, Christian Lamparter wrote:
>
> >> Just a FYI: due to these difficulties with getting a gpio driver
> >> upstream, Alan Mizrahi upstreamed an in-kernel led-apu.c driver [0]
> >> that sort of bypasses the whole pinctrl vs gpio issue.
> >
> > Thanks, I saw that and was somewhat surprised to see it accepted.
>
> I looked at the driver and it seems actually good and doing the
> right thing. If I understand it right:
>
> - It does a bunch of magic dmi_match() on DMI_BOARD_NAME
> to figure out what board this is.
>
> - It then proceeds to register LEDs using some bits in the MMIO
> area that are used for LEDs.
>
> So these bits/lines are actually not GPIO, since GPIO means
> "general purpose input/output" - they are specific purpose and the
> specific purpose can be detected.
> [...]
> If only these few GPIO lines are actually used and only used for
> these LEDs, (the rest of the bits in the register unused) then this
> driver is as good as any.
Yeah :) I wanted to point this out Andrew Cooks too.
Hence:
>>If you are just after LEDs or gpio-keys you probably can go
>>the same route?"
But thank you for confirming this as well. This special led driver
will do in a pinch. That said, the APU1/APU2 also has a pushbutton
(modeswitch) wired to one of the FCH's other pins. So, this is were
having a pinctrl/gpio driver would become *convenient*. Because
leds-gpio and gpio-keys-polled make it possible to support
all these bits and bobs without having to duplicate the MMIO access.
(Furthermore, the LEDS and pushbutton/modeswitch lines are also
accessible through the J4 Header on the board).
Note: the vendor (PC Engines) happily provides PDFs and schematics
for their boards:
<https://www.pcengines.ch/pdf/apu1.pdf>
<https://www.pcengines.ch/schema/apu1c.pdf>
<https://www.pcengines.ch/pdf/apu2.pdf>
<http://pcengines.ch/schema/apu2c.pdf>
So, it's possible to repurpose several test points as additional
GPIOs and more.
Note2:
On both boards there is also a dedicated GPIO pin header J19, but
these pins are controlled by the SuperIO Nuvoton NCT5104D.
Thanks,
Christian