leds-gpio on x86

leds-gpio on x86

[Vincent Pelletier]

Hello,

I have recently acquired a NAS[1] which is basically an x86_64 with UEFI
bios and adequate enclosure (drive trays and associated activity & error
leds).

Not being too happy with stock firmware features, I am in the process
of replacing it with a general distribution. The distribution works
great with stock kernel, but I do not get the error leds (nor, but I'm
less interested, the global power/error/usb leds).

Poking in the original firmware, I found a file declaring which SuperIO
GPIO pins are used, and confirmed they do work by loading the
appropriate gpio driver.

Now, I wonder how far I should go: I would like to use the leds
subsystem for easy triggering, but leds-acpi finds nothing.
A quick read shows this driver finds relevant devices using
openfirmware enumeration functions. If I understand correctly, this
would be devicetree on ARM/MIPS/... but ACPI tables on x86. After an
uneducated look at disassembled DSDT, I think I found the
SuperIO declaration:
  Name (IO3B, 0x0290)
  [...]
  Device (SIO1)
  {
    Name (_HID, EisaId ("PNP0C02") /* PNP Motherboard Resources */)  // _HID: Hardware ID
    [...]
    Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Settings
    {
      [...]
      If (IO3B)
      {
        CreateWordField (CRS, \_SB.PCI0.SBRG.SIO1._Y0A._MIN, GP30)  // _MIN: Minimum Base Address
        CreateWordField (CRS, \_SB.PCI0.SBRG.SIO1._Y0A._MAX, GP31)  // _MAX: Maximum Base Address
        CreateByteField (CRS, \_SB.PCI0.SBRG.SIO1._Y0A._LEN, GPL3)  // _LEN: Length
        GP30 = IO3B /* \IO3B */
        GP31 = IO3B /* \IO3B */
        GPL3 = IO3L /* \IO3L */
      }
(dmesg showing the SuperIO being based at 0x290 and this value not
appearing elsewhere) which does not define any GPIO (no GpioIo() in
the Device {} block), even less leds.

One option seems to patch this to declare leds, but it seems dangerous
(from the examples[2] I found, the change needed is less than 100 lines
for the 6 leds, but that's an awful lot of error possibilities for the
ACPI illiterate I am).

Another option seems to develop a dumb module just declaring what the
leds. This means I'll have to maintain an off-tree module (I do not
see how it would detect that it's actually on this NAS model, so
upstream inclusion seems unlikely).

Yet another option would be to extend an existing driver, to make it
configurable (ex: via sysfs). This would be my favourite (it is
clearer to see how it would get merged), but registration need several
values (name, pin number, active high/low, maybe color ?) and I have no
idea how clean the syntax would be.

Everything seems possible but I cannot tell which has better chances of
success.

Is my analysis correct so far ?
Which next step would you recommend ?

[1] QNAP TS-651
[2] https://lwn.net/Articles/612062/
-- 
Vincent Pelletier

Re: leds-gpio on x86

[Mika Westerberg]

On Wed, Aug 05, 2015 at 10:09:48PM +0200, Vincent Pelletier wrote:
> Hello,
> 
> I have recently acquired a NAS[1] which is basically an x86_64 with UEFI
> bios and adequate enclosure (drive trays and associated activity & error
> leds).
> 
> Not being too happy with stock firmware features, I am in the process
> of replacing it with a general distribution. The distribution works
> great with stock kernel, but I do not get the error leds (nor, but I'm
> less interested, the global power/error/usb leds).
> 
> Poking in the original firmware, I found a file declaring which SuperIO
> GPIO pins are used, and confirmed they do work by loading the
> appropriate gpio driver.
> 
> Now, I wonder how far I should go: I would like to use the leds
> subsystem for easy triggering, but leds-acpi finds nothing.
> A quick read shows this driver finds relevant devices using
> openfirmware enumeration functions. If I understand correctly, this
> would be devicetree on ARM/MIPS/... but ACPI tables on x86. After an
> uneducated look at disassembled DSDT, I think I found the
> SuperIO declaration:
>   Name (IO3B, 0x0290)
>   [...]
>   Device (SIO1)
>   {
>     Name (_HID, EisaId ("PNP0C02") /* PNP Motherboard Resources */)  // _HID: Hardware ID
>     [...]
>     Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Settings
>     {
>       [...]
>       If (IO3B)
>       {
>         CreateWordField (CRS, \_SB.PCI0.SBRG.SIO1._Y0A._MIN, GP30)  // _MIN: Minimum Base Address
>         CreateWordField (CRS, \_SB.PCI0.SBRG.SIO1._Y0A._MAX, GP31)  // _MAX: Maximum Base Address
>         CreateByteField (CRS, \_SB.PCI0.SBRG.SIO1._Y0A._LEN, GPL3)  // _LEN: Length
>         GP30 = IO3B /* \IO3B */
>         GP31 = IO3B /* \IO3B */
>         GPL3 = IO3L /* \IO3L */
>       }
> (dmesg showing the SuperIO being based at 0x290 and this value not
> appearing elsewhere) which does not define any GPIO (no GpioIo() in
> the Device {} block), even less leds.

Does it list the GPIO controller itself in the DSDT? If it does not, you
would need to patch that there as well.

Just in case, can you provide me the DSDT? I can take a look.

> One option seems to patch this to declare leds, but it seems dangerous
> (from the examples[2] I found, the change needed is less than 100 lines
> for the 6 leds, but that's an awful lot of error possibilities for the
> ACPI illiterate I am).

Indeed toggling random GPIOs might break something ;-)

> Another option seems to develop a dumb module just declaring what the
> leds. This means I'll have to maintain an off-tree module (I do not
> see how it would detect that it's actually on this NAS model, so
> upstream inclusion seems unlikely).
> 
> Yet another option would be to extend an existing driver, to make it
> configurable (ex: via sysfs). This would be my favourite (it is
> clearer to see how it would get merged), but registration need several
> values (name, pin number, active high/low, maybe color ?) and I have no
> idea how clean the syntax would be.
> 
> Everything seems possible but I cannot tell which has better chances of
> success.
> 
> Is my analysis correct so far ?

Sounds correct to me.

> Which next step would you recommend ?

I would just go for writing a custom board file that fills in platform
data to leds-gpio.c driver.

Re: leds-gpio on x86

[Vincent Pelletier]

On Thu, 6 Aug 2015 19:59:24 +0300, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> Does it list the GPIO controller itself in the DSDT? If it does not, you
> would need to patch that there as well.

I do see this in the dsdt:
  Name (_HID, "INT33FC" /* Intel Baytrail GPIO Controller */)  // _HID: Hardware ID
  Name (_CID, "INT33FC" /* Intel Baytrail GPIO Controller */)  // _CID: Compatible ID

I do not know the distinction between a gpio controller and the chip
which has the pins directly, though.

> Just in case, can you provide me the DSDT? I can take a look.

(sent off-list)

> I would just go for writing a custom board file that fills in platform
> data to leds-gpio.c driver.

Thanks, I'll go this way.

Regards,
-- 
Vincent Pelletier

Re: leds-gpio on x86

[Mika Westerberg]

On Thu, Aug 06, 2015 at 07:17:27PM +0200, Vincent Pelletier wrote:
> On Thu, 6 Aug 2015 19:59:24 +0300, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > Does it list the GPIO controller itself in the DSDT? If it does not, you
> > would need to patch that there as well.
> 
> I do see this in the dsdt:
>   Name (_HID, "INT33FC" /* Intel Baytrail GPIO Controller */)  // _HID: Hardware ID
>   Name (_CID, "INT33FC" /* Intel Baytrail GPIO Controller */)  // _CID: Compatible ID
> 
> I do not know the distinction between a gpio controller and the chip
> which has the pins directly, though.

Do you know which GPIOs are used for leds?

Since the GPIO host controller is listed in the DSDT, I think patching
the leds device should be pretty simple.

> > Just in case, can you provide me the DSDT? I can take a look.
> 
> (sent off-list)

Got it, thanks.

Re: leds-gpio on x86

[Vincent Pelletier]

On Thu, 6 Aug 2015 20:35:58 +0300, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> Do you know which GPIOs are used for leds?
> 
> Since the GPIO host controller is listed in the DSDT, I think patching
> the leds device should be pretty simple.

That would be fantastic.

Green status: 62, green, active low
Red status: 63, red, active low
HDD racks: 70 to 75, red, active low
USB: 17, blue, active low

I tested all via sysfs with gpio-f7188x driver loaded (numbers above
are the values I wrote to /sys/class/gpio/register).

"green status" led works minimally out of the box: static-on when the
box is on, off otherwise (I would make it blink on S3, for example).

Regards,
-- 
Vincent Pelletier

Re: leds-gpio on x86

[Mika Westerberg]

On Thu, Aug 06, 2015 at 08:18:20PM +0200, Vincent Pelletier wrote:
> On Thu, 6 Aug 2015 20:35:58 +0300, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > Do you know which GPIOs are used for leds?
> > 
> > Since the GPIO host controller is listed in the DSDT, I think patching
> > the leds device should be pretty simple.
> 
> That would be fantastic.
> 
> Green status: 62, green, active low
> Red status: 63, red, active low
> HDD racks: 70 to 75, red, active low
> USB: 17, blue, active low
> 
> I tested all via sysfs with gpio-f7188x driver loaded (numbers above
> are the values I wrote to /sys/class/gpio/register).

Ah, this is different GPIO controller that is listed in the DSDT (the
Baytrail GPIO host controller).

In order to get ACPI GPIO stuff work here you would need to first add
the GPIO device to the ACPI namespace and then modify gpio-f7188x.c to
probe it from there. You would also need to invent a _HID to the device
in order to get it matched. Unfortunately using random _HID will make
upstreaming the changes difficult.

Re: leds-gpio on x86

[Vincent Pelletier]

On Fri, 7 Aug 2015 13:57:45 +0300, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> Ah, this is different GPIO controller that is listed in the DSDT (the
> Baytrail GPIO host controller).
> 
> In order to get ACPI GPIO stuff work here you would need to first add
> the GPIO device to the ACPI namespace and then modify gpio-f7188x.c to
> probe it from there. You would also need to invent a _HID to the device
> in order to get it matched. Unfortunately using random _HID will make
> upstreaming the changes difficult.

I gave a shot at writing a module. I got to the point where it builds
and loads, but it does not appear to do anything (no error
returned modprobe, nothing in dmesg).

I have no idea if the ~4 lines of "actual" code (ie, not just data
definition) are even in the good direction actually. I took
  http://www.armadeus.com/wiki/index.php?title=GPIO_LEDS
as an example. This is for a platform definition for a devboard
(arm ?), so it may be a wrong example. I took a look at other led
drivers, and their complexity varies a lot.

Would you mind taking a look ?
  https://github.com/vpelletier/linux/commits/ts651

For example, I have no idea how to explicitly depend on gpio-f7188x
and leds-gpio.
-- 
Vincent Pelletier

Re: leds-gpio on x86

[Mika Westerberg]

On Sat, Aug 08, 2015 at 02:06:56PM +0200, Vincent Pelletier wrote:
> On Fri, 7 Aug 2015 13:57:45 +0300, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > Ah, this is different GPIO controller that is listed in the DSDT (the
> > Baytrail GPIO host controller).
> > 
> > In order to get ACPI GPIO stuff work here you would need to first add
> > the GPIO device to the ACPI namespace and then modify gpio-f7188x.c to
> > probe it from there. You would also need to invent a _HID to the device
> > in order to get it matched. Unfortunately using random _HID will make
> > upstreaming the changes difficult.
> 
> I gave a shot at writing a module. I got to the point where it builds
> and loads, but it does not appear to do anything (no error
> returned modprobe, nothing in dmesg).
> 
> I have no idea if the ~4 lines of "actual" code (ie, not just data
> definition) are even in the good direction actually. I took
>   http://www.armadeus.com/wiki/index.php?title=GPIO_LEDS
> as an example. This is for a platform definition for a devboard
> (arm ?), so it may be a wrong example. I took a look at other led
> drivers, and their complexity varies a lot.
> 
> Would you mind taking a look ?
>   https://github.com/vpelletier/linux/commits/ts651

One thing I noticed:

	qnap_tsx51_leds_platform_device = platform_device_register_resndata(NULL, "led-gpio", -1, NULL, 0,
		&qnap_tsx51_led_data, sizeof(qnap_tsx51_led_data));

The driver expects "leds-gpio" not "led-gpio".

Re: leds-gpio on x86

[Vincent Pelletier]

On Tue, 11 Aug 2015 15:00:38 +0300, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> One thing I noticed:
> 
> 	qnap_tsx51_leds_platform_device = platform_device_register_resndata(NULL, "led-gpio", -1, NULL, 0,
> 		&qnap_tsx51_led_data, sizeof(qnap_tsx51_led_data));
> 
> The driver expects "leds-gpio" not "led-gpio".

And indeed, this is what was preventing proper detection.
Very nice catch, thanks a lot.

Now, I see two more things I need to do and for which I have no idea:
- Somehow depend on gpio-f7188x and cause leds-gpio to get loaded (is it
  a dependence too ?).
  Module writing documentation mention soft dependencies, but it feels
  wrong here.
- Somehow detect that it is actually a qnap of expected model (and, by
  extension, actually implement led count substraction).
  I tried (and failed so far) to understand what the original firmware
  does. dmidecode does not bring something relevant. I have no idea
  what is typically done in this area.

Regards,
-- 
Vincent Pelletier

Re: leds-gpio on x86

[Mika Westerberg]

On Tue, Aug 11, 2015 at 07:42:56PM +0200, Vincent Pelletier wrote:
> On Tue, 11 Aug 2015 15:00:38 +0300, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > One thing I noticed:
> > 
> > 	qnap_tsx51_leds_platform_device = platform_device_register_resndata(NULL, "led-gpio", -1, NULL, 0,
> > 		&qnap_tsx51_led_data, sizeof(qnap_tsx51_led_data));
> > 
> > The driver expects "leds-gpio" not "led-gpio".
> 
> And indeed, this is what was preventing proper detection.
> Very nice catch, thanks a lot.
> 
> Now, I see two more things I need to do and for which I have no idea:
> - Somehow depend on gpio-f7188x and cause leds-gpio to get loaded (is it
>   a dependence too ?).
>   Module writing documentation mention soft dependencies, but it feels
>   wrong here.

Once your module gets loaded, it creates the "leds-gpio" platform device
which in turn makes the leds-gpio driver to load.

For gpio-f7188x you need to load it manually because it does not have
any module strings modprobe can match with a device. You can create
platform device for this in your qnap board file as well and then add
MODULE_ALIAS() to the driver to get it loaded automatically.

> - Somehow detect that it is actually a qnap of expected model (and, by
>   extension, actually implement led count substraction).
>   I tried (and failed so far) to understand what the original firmware
>   does. dmidecode does not bring something relevant. I have no idea
>   what is typically done in this area.

Typically we get necessary information from ACPI or similar device
description ;-)

You may check DMI strings in _init() of your board file and only create
the platform devices if they match qnap. /sys/class/dmi/id/* should have
something to differentiate it from others.

Re: leds-gpio on x86

[Vincent Pelletier]

On Wed, 12 Aug 2015 15:53:14 +0300, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> Once your module gets loaded, it creates the "leds-gpio" platform device
> which in turn makes the leds-gpio driver to load.

Oh, right, I somehow thought I had to modprobe it too and didn't check
without. It does work.

> For gpio-f7188x you need to load it manually because it does not have
> any module strings modprobe can match with a device. You can create
> platform device for this in your qnap board file as well and then add
> MODULE_ALIAS() to the driver to get it loaded automatically.

Looking at MODULE_ALIAS, I found "request_module()". It works great.

I've also added declaration for both buttons, and moved my code to
drivers/platform/x86/ as it is no longer leds-specific. I found that
there are things like laptop-[brand-]specific modules there, so I guess
it is reasonable.

> > - Somehow detect that it is actually a qnap of expected model (and, by
> >   extension, actually implement led count substraction).
> >   I tried (and failed so far) to understand what the original firmware
> >   does. dmidecode does not bring something relevant. I have no idea
> >   what is typically done in this area.
> 
> Typically we get necessary information from ACPI or similar device
> description ;-)

Right :) .

> You may check DMI strings in _init() of your board file and only create
> the platform devices if they match qnap. /sys/class/dmi/id/* should have
> something to differentiate it from others.

Sadly, ACPI-careless OEM is DMI-careless too:

Handle 0x0000, DMI type 0, 24 bytes
BIOS Information
        Vendor: American Megatrends Inc.
        Version: 5.6.5
        Release Date: 05/19/2014
        Address: 0xF0000
        Runtime Size: 64 kB
        ROM Size: 1024 kB
        Characteristics:
                PCI is supported
                BIOS is upgradeable
                BIOS shadowing is allowed
                Boot from CD is supported
                Selectable boot is supported
                BIOS ROM is socketed
                EDD is supported
                5.25"/1.2 MB floppy services are supported (int 13h)
                3.5"/720 kB floppy services are supported (int 13h)
                3.5"/2.88 MB floppy services are supported (int 13h)
                Print screen service is supported (int 5h)
                8042 keyboard services are supported (int 9h)
                Serial services are supported (int 14h)
                Printer services are supported (int 17h)
                ACPI is supported
                USB legacy is supported
                BIOS boot specification is supported
                Targeted content distribution is supported
                UEFI is supported
        BIOS Revision: 5.6

Handle 0x0001, DMI type 1, 27 bytes
System Information
        Manufacturer: To be filled by O.E.M.
        Product Name: To be filled by O.E.M.
        Version: To be filled by O.E.M.
        Serial Number: To be filled by O.E.M.
        UUID: 03000200-0400-0500-0006-000700080009
        Wake-up Type: Power Switch
        SKU Number: To be filled by O.E.M.
        Family: To be filled by O.E.M.

Handle 0x0002, DMI type 2, 15 bytes
Base Board Information
        Manufacturer: AMI Corporation
        Product Name: Aptio CRB
        Version: To be filled by O.E.M.
        Serial Number: To be filled by O.E.M.
        Asset Tag: To be filled by O.E.M.
        Features:
                Board is a hosting board
                Board is replaceable
        Location In Chassis: To be filled by O.E.M.
        Chassis Handle: 0x0003
        Type: Motherboard
        Contained Object Handles: 0

Handle 0x0003, DMI type 3, 25 bytes
Chassis Information
        Manufacturer: To Be Filled By O.E.M.
        Type: Desktop
        Lock: Not Present
        Version: To Be Filled By O.E.M.
        Serial Number: To Be Filled By O.E.M.
        Asset Tag: To Be Filled By O.E.M.
        Boot-up State: Safe
        Power Supply State: Safe
        Thermal State: Safe
        Security Status: None
        OEM Information: 0x00000000
        Height: Unspecified
        Number Of Power Cords: 1
        Contained Elements: 1
                <OUT OF SPEC> (0)
        SKU Number: To be filled by O.E.M.

BIOS block values look nice, but not specific enough.
"Aptio CRB" seems to be an AMI trademark for their UEFI bios.
UUID looks garbage (or rather, it looks too clean to be relevant).

So I guess /etc/modules is the way.

I think the module is now quite close to submission for broader
review, thanks to your help.

Regards,
-- 
Vincent Pelletier