PC Engines APU2: LEDs not working in LEDE 17.01

R1D2 · March 18, 2018, 8:06pm

Hi developers! I installed LEDE 17.01.4 on an APU2C4 from PC Engines and noticed that the leds-apu2 and gpio-nct5104d drivers don't work anymore with coreboot v4.6.1 (mainline, latest, but happens also with coreboot v4.0.x legacy versions):

[    4.445697] kmodloader: 2 modules could not be probed
[    4.450811] kmodloader: - gpio-nct5104d - 0
[    4.455042] kmodloader: - leds-apu2 - 0

Reason for this is a change in coreboot's board name from APU2 to PCEngines apu2 in legacy and PC Engines apu2 in mainline versions.

Since the original maintainer for the leds-apu2 package has set his git repo as read-only (and since github doesn't support my browser anymore ) I couldn't open an issue in the issue tracker, so I thought I offer to send the (tiny) patch for leds-apu2 to this forum if you want me to do so.

Albeit LEDs now work again, the default LED config still gets not set up in /etc/config/system. Error message during boot is:

WARNING: Variable 'led' does not exist or is not an array/object

after pre-init. I'm not familiar with the new default config generation process in LEDE and therefore will just create a default LED setup using a predefined system config file for my firmware image. If someone could give me a pointer to where I can find the docs or (commented!) sources used in the board config process, I would be willing to examine this further and fix/test it. But probably those of you used to this config process can find and correct it much faster than I could.

Re gpio-nct5104d: this also uses the old APU2 board name and a similar patch as for the LED module applies here, too, but I couldn't get it running at all. Since w/o this module LEDs are working anyway, I didn't investigate any further and did compile a firmware image without this module. FWIW, this are the errors thrown on an APU2C4 during boot with the kernel module gpio-nct5104d probed:

[    3.361023] gpio-nct5104d: Unsupported device 0xc453
[    3.366030] gpio-nct5104d: Unsupported device 0xffff

Should I post the quilt-style patch for leds-apu2 here or would you prefer to fix the board names in the leds-apu2 and probably gpio-nct5104d modules for yourself?

sebastian_de · March 30, 2018, 8:46am

I just discovered your post. leds-apu2 is maintained in OpenWrt git, so you can send patches directly to to the lede-dev mailinglist.
I had the same idea as you had, but I found out that the board names will be in the form "apuX" in future firmware releases again (see this comment by one of the PCEngines employees).
They didn't publish a binary release yet, but building coreboot from source is easy, you can follow the official guide. This guide was written for coreboot 4.5.x, but it works the same with branch coreboot-4.6.x

Greetings
Sebastian

R1D2 · March 30, 2018, 1:35pm

Hello Sebastian,
thanks for your advice. I will send this and other patches to the lede-dev mailing list soon.

Yes, I saw the comment about the planned name change for APUs already. The patch accepts all name variations, so it will work with board names in existing native legacy (4.0.x), mainline (4.6.x) and even in future coreboot versions.

But the LED default setup still doesn't work and I couldn't find out why - the board detection of LEDE still is kind of magic to me. No big problem, I currently use a static UCI setup for the LEDs.

Best wishes and have a nice weekend!

ElektromAn · March 30, 2018, 4:38pm

FWIW:
for leds on APU2 or the older APU, there is a driver in upstream linux.
And for APU2 the vendor messed up the board name.

If I have time I do a backport of the driver.

sebastian_de · March 30, 2018, 7:55pm

I've sent a patch addressing the board names a few days ago, see https://patchwork.ozlabs.org/patch/890989/
As you can see in this thread, I currently tend to not applying this patch. It's my impression, that the (buggy) 4.6.x versions are mostly used by "bleeding edge" users that'll switch to a more recent version anyway (once that's released). But I might be wrong - what do you think?

R1D2 · March 31, 2018, 2:19am

I'm an old-fashion UNIX guy and as such I'm used to fix all kinds of mess, even if introduced by the vendor.
But I have no idea how many users actually do use latest coreboot version.

Only reason why I did an upgrade was a claim somewhere that the pre-installed generic coreboot from PC Engines (4.0.7 IIRC) can't boot from SD cards. I have to deploy 12x APUs in the field ASAP and I guess that my clients won't upgrade coreboot at all - they are all but no bleeding edge users ...

But never mind and feel free to decide what's best for the upstream version. It is no problem for me to apply a patch locally. OTOH, if you just check for the substring APU2 or even only APU in the board name using strcasestr(3), this would cover all name changes so far. Since the vendor name is also being tested for in the matching expression, checking just for APU seems not to unspecific to me. This is my patch, probably I will change it to use strcasestr instead of strcasecmp:

--- a/leds-apu2.c
+++ b/leds-apu2.c
@@ -328,7 +328,12 @@ static int __init gpio_apu2_init (void)
        const char *board_name = dmi_get_system_info(DMI_BOARD_NAME);
 
        /* Match the device name/model */
-       if (!board_name || !board_vendor || strcasecmp(board_vendor, "PC Engines") || strcasecmp(board_name, "apu2")) {
+       if (!board_name || !board_vendor ||
+           strcasecmp(board_vendor, "PC Engines") ||
+           (strcasecmp(board_name, "PCEngines apu2") &&
+            strcasecmp(board_name, "PC Engines apu2") &&
+            strcasecmp(board_name, "APU2"))
+          ) {
                err = -ENODEV;
                goto exit;
        }

sebastian_de · March 31, 2018, 10:59am

This would make the check simpler, of course, but there are also the APU4 and APU5 boards that may use the same led driver, but are not publicly available, so nobody can test this. Therefore I would stick to checking for APU2/3 explicitly. By the time another APU board is released, someone may have backported the upstream driver ElektromAn mentioned anyway
Just to make things clear, I'm not an OpenWrt developer, the led patch was actually the first one I ever submited...

Out of curiosity: What are you using the 12 APUs for?

Sebastian

R1D2 · March 31, 2018, 4:20pm

Yes, you're right. Didn't think of APU5. I'm also no OpenWRT developer, but do use it for 11+ years now. Latest version 17.01 is really great, I like this very much. The APUs will replace older routers in public WLAN hotspots deployments.

Thanks to ElektromAn and you for your help, much appreciated!

muhahacz · June 15, 2018, 8:00pm

Can anyone submit patch also for gpio-nct5104d.c ? Thanks

--- a/gpio-nct5104d.c
+++ b/gpio-nct5104d.c
@@ -435,8 +435,15 @@ static int __init nct5104d_gpio_init(void)
        const char *board_name = dmi_get_system_info(DMI_BOARD_NAME);
 
        /* Make sure we only run on PC Engine APU boards */
-       if (!board_name || !board_vendor || strcasecmp(board_vendor, "PC Engines") || strncasecmp(board_name, "apu", 3)) {
-               return -ENODEV;
+       if (!board_name \
+                       || !board_vendor \
+                       || strcasecmp(board_vendor, "PC Engines") \
+                       || (strcasecmp(board_name, "apu2") \
+                               && strcasecmp(board_name, "apu3") \
+                               && strcasecmp(board_name, "PC Engines apu2") \
+                               && strcasecmp(board_name, "PC Engines apu3"))) {
+               err = -ENODEV;
+               goto exit;
        }
 
        if (nct5104d_find(0x2e, &sio) &&

jeff · June 15, 2018, 9:15pm

You'll want to check into the board names. I have a vague recollection that some versions answer "PC Engines" and others answer "PCEngines" or something like that.

R1D2 · June 15, 2018, 10:26pm

Hi muhahacz, here are two patches. First is for board names:

package/kernel/gpio-nct5104d/patches/301-fix-apu2-boardname.patch

--- a/gpio-nct5104d.c
+++ b/gpio-nct5104d.c
@@ -429,7 +429,12 @@ static int __init nct5104d_gpio_init(voi
        const char *board_name = dmi_get_system_info(DMI_BOARD_NAME);
 
        /* Make sure we only run on PC Engine APU boards */
-       if (!board_name || !board_vendor || strcasecmp(board_vendor, "PC Engines") || strncasecmp(board_name, "apu", 3)) {
+       if (!board_name || !board_vendor ||
+           strcasecmp(board_vendor, "PC Engines") ||
+           (strncasecmp(board_name, "PCEngines apu", 13) &&
+            strncasecmp(board_name, "PC Engines apu", 14) &&
+            strncasecmp(board_name, "apu", 3))
+          ) {
                return -ENODEV;
        }

Second patch is for new NCT5104D chip ID used in APU2:

package/kernel/gpio-nct5104d/patches/302-fix-apu2-nct5104d-chipID.patch

--- a/gpio-nct5104d.c
+++ b/gpio-nct5104d.c
@@ -35,7 +35,8 @@
 #define SIO_LOCK_KEY           0xAA    /* Key to disable Super-I/O */
 
 #define SIO_NCT5104D_ID                                        0x1061  /* Chip ID */
-#define SIO_PCENGINES_APU_NCT5104D_ID  0xc452  /* Chip ID */
+#define SIO_PCENGINES_APU_NCT5104D_ID_B        0xc452  /* Chip ID */
+#define SIO_PCENGINES_APU_NCT5104D_ID_C        0xc453  /* Chip ID */
 
 enum chips { nct5104d };
 
@@ -346,7 +347,8 @@ static int __init nct5104d_find(int addr
        devid = superio_inw(addr, SIO_CHIPID);
        switch (devid) {
        case SIO_NCT5104D_ID:
-       case SIO_PCENGINES_APU_NCT5104D_ID:
+       case SIO_PCENGINES_APU_NCT5104D_ID_B:
+       case SIO_PCENGINES_APU_NCT5104D_ID_C:
                sio->type = nct5104d;
                /* enable GPIO0 and GPIO1 */
                superio_select(addr, SIO_LD_GPIO);

Now LEDE boots fast again on APU2 if gpio-nct5104d kernel module is enabled.

muhahacz · June 16, 2018, 8:41am

R1D2: Thanks. Will you sumbit patches? Consider to let it backport to 18.06 ?

R1D2 · June 17, 2018, 9:13am

I have send them to package maintainers and filed a feature request to the coreboot developers to correct the APU1 & APU2 names in the bootloader. For sending patches to the OpenWRT developer list, AFAIK they require a formal "patchwork" format or creating an own branch on github with pull requests. Sorry, I won't set up a mail account on our isolated development server just for sending small changes. Github doesn't support my laptop's OS anymore, so for me that's no option either. Those changes are not rocket science and honestly, since we don't use git here, I have no time to set up an environment to just send out a few lines of code. Getting those small changes into quilt patch format did cost enough time already.

As for 18.06: feel free to add it. I'm still overloaded with work to get even 17.01 running stable on five more of our supported platforms given all those "enhancements" in 17.01, which broke our application and several system functions (block mounts, procd restarting services by killing the service first, procd exclusively using hardware watchdog, not mounting boot partition if more than one rootfs due to UUID weirdness, wrong LED handler timing during boot, fstab screwed up, mount command not working anymore, crontabs file generation vanished, etc. to name just a few), so I still need to catch up with all this before considering an update to 18.06.

muhahacz · June 17, 2018, 9:21am

Are these patches working out of the box in 17.01 ? Just copying to package/kernel/gpio-nct5104d/patches/ .. without proper patching method (quilt ?). Because It does not work for me.... Hunk FAILED when compiling ..

R1D2 · June 17, 2018, 9:33am

Yes, they work on my copy of 17.01-4. The leds patch needs to be in package/kernel/leds-apu2/patches, while the two gpio patches should be in package/kernel/gpio-nct5104d/patches (don't ask my why, but couldn't get it work from the top-level patches directory and have no idea why there are no serial files compared to patches in the top level patches directory). As I said, I'm not an OpenWRT developer, just need to get the systems running and tried this time to use quilt patches according to the docs on the OpenWRT website rather than copying changes over the source files.

If it still doesn't work, I suggest to change the sources directly if you are in a hurry.

ElektromAn · June 17, 2018, 1:25pm

upstream has a led driver for APU and APU2
only APU3 support is missing here.

And a driver for the reset button is also missing in upstream.

I think the big problem for OpenWrt is here the naming scheme in DMI.

R1D2 · June 17, 2018, 8:14pm

Yes, it's the one checking for model name "APU2", but coreboot sets either "PCEngines apu2" or "PC Engines apu2" depending on version (note the blank between "PC" and "Engines") making the full vendor/model name: "PC Engines PCEngines APU2", thus the patch.

muhahacz · June 18, 2018, 8:00pm

R1D21: Can You please include all patches/issues ? I mean links, to track it. Thanks

R1D2 · June 19, 2018, 12:23am

Not sure what you mean. This are just quick local hacks to get LEDs and GPIO modules running on APU2, not official patches.