Page MenuHomeFreeBSD

rk805: add support for controlling two output pins via gpio interface
Needs ReviewPublic

Authored by avg on Jun 16 2021, 10:53 AM.

Details

Reviewers
manu
peterj
gonzo
Summary

This function seems to be present in RK805 only (that is, not in RK808).
Rock64 uses those pins to control the on-board LEDs.

Caveat: led(9) uses the mutex to protect its state and the mutex is held
when modifying a led state. This driver uses the sx lock in gpio calls.
rk_i2c sleeps in I2C transfers. Both do not play well with the mutex.
The solution is to modify the led driver to use a sleepable lock to allow
for various LED communication mechanisms (I2C, USB, etc).
That would not be a straightforward change as the lock is used in
a callout.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 39930
Build 36819: arc lint + arc unit

Event Timeline

avg requested review of this revision.Jun 16 2021, 10:53 AM
avg added reviewers: peterj, gonzo. avg added 1 blocking reviewer(s): manu.Jun 16 2021, 10:55 AM
peterj requested changes to this revision.Jun 19 2021, 12:52 PM

I agree this is only applicable to the RK805. The patch works and provides access to the Rock64 LEDs (pin 0 is red and pin 1 is white, with the LED lit when the output is 0). On the downside, and as noted in the description, there is a LOR with led(9). I'm a bit concerned at adding known new LORs to the code. Are there any options for getting rid of the LOR other than redesigning led(9)?

lock order reversal: (sleepable after non-sleepable)
 1st 0xffff000000bad548 LED mtx (LED mtx, sleep mutex) @ /usr/src/sys/dev/led/led.c:298
 2nd 0xffffa00000d74d00 rk805 gpio (rk805 gpio, sx) @ /usr/src/sys/arm64/rockchip/rk805.c:811
lock order LED mtx -> rk805 gpio attempted at:
#0 0xffff0000004780b4 at witness_checkorder+0xbe0
#1 0xffff00000041c8c8 at _sx_xlock+0x28
#2 0xffff0000006b7fc4 at rk805_gpio_pin_set+0x4c
#3 0xffff0000002acd08 at led_create_state+0x144
#4 0xffff00000029dcbc at gpioled_attach+0x290
#5 0xffff00000044a540 at device_attach+0x3f0
#6 0xffff00000044c34c at bus_generic_new_pass+0x104
#7 0xffff00000044c2f0 at bus_generic_new_pass+0xa8
#8 0xffff00000044c2f0 at bus_generic_new_pass+0xa8
#9 0xffff00000044e42c at root_bus_configure+0x40
#10 0xffff0000003ac900 at mi_startup+0x254
#11 0xffff0000000008a8 at virtdone+0x6c
This revision now requires changes to proceed.Jun 19 2021, 12:52 PM

I cannot think of any way.
rk_i2c needs to sleep to wait for an interrupt.
led(8) uses a non-sleepable mutex.

I think that led(9) better be redesigned to allow for various transports like I2C, USB, etc.
Simple I/O (like I/O ports or memory mapped) is not the only way to control LEDs.

(Sorry for the delay. I thought I'd responded). Then I withdraw my objections. The code itself looks good.

This revision now requires review to proceed.Jul 2 2021, 10:05 AM