search

jgarff / rpi_ws281x

Userspace Raspberry Pi PWM library for WS281X LEDs
BSD 2-Clause "Simplified" License
1.78k stars 622 forks source link

Use mailbox to get hwrev #495

Open cdsteinkuehler opened 2 years ago

cdsteinkuehler commented 2 years ago

I ran into some trouble using this library on my system because (see below for details if interested).

Anyway, I was having issues with identifying the Raspberry Pi hardware platform. Since the code is already talking to the firmware mailbox register I just switched to directly asking the firmware for the hardware revision. I have no idea if this is a desired change or not, but to me it seems to make the code somewhat cleaner (as well as fixing my use case). I wonder if someone had intended to do something like this previously as there is a "dangling" definition for a get_version() function in mailbox.h that has no code behind it.

Details on my issue: I'm using Buildroot + U-Boot + RAUC to provide a system that supports safe firmware updates and a fallback rescue partition (see: https://github.com/cdsteinkuehler/br2rauc). I'm using a 64-bit OS so I don't get the hardware details in /proc/cpuinfo. The /system device-tree entries the existing code in rpihw.c is looking for are actually populated by the RPi firmware. Since I load the device-tree from the selected root filesystem (allowing for kernel and device-tree updates over time) I don't have these entries in my device tree. I may see about having my U-Boot script copy over some of these RPi firmware populated device-tree nodes, but it was quicker in the short term to just update the hardware detection logic.

Thanks for the great project!

Gadgetoid commented 2 years ago

I'd take a wild guess that this would elevate the privilege level required for the library to say "your board is not supported, go away" (since IIRC mailbox requires root?) but I don't really see that as a problem.

This might take some significant regression testing, and potentially be obsolete once we figure out #183, but the code is much cleaner.

I have never been all that comfortable with the /proc/cpuinfo method of grabbing the revision, and your case is one I didn't consider in my argument against it.

I don't believe it's that you're using a 64bit OS, but rather that you're not including this patch (and whatever related patches make it show up in /proc/cpuinfo) - https://github.com/raspberrypi/linux/commit/3e787f9e5bb0b2d71df10e9f232df3d9a1046161

Looks like Revision ID in /proc/cpuinfo is- in fact- doomed to be the downstream hack I suspected it was.