This driver enables bcm2835-virtgpio GPIO controller found on RPi3B and some CM boards.
On which, the ACT (green) LED is connected to this controller.
It is essential for FreeBSD to have this driver to control this LED.
Details
Details
It works properly on RPi3B w/ 15-CURRENT. The green LED can be accessed via /dev/led/led0 .
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 65855 Build 62738: arc lint + arc unit
Event Timeline
| sys/arm/broadcom/bcm2835/raspberrypi_virtgpio.c | ||
|---|---|---|
| 43 | Not needed. | |
| 130 | ||
| 173 | ||
| 253–257 | This check belongs in rpi_virt_gpio_probe(). I am not sure it is necessary at all. | |
| 288–307 | I am not convinced that busdma is the correct interface for what you are trying to achieve, since you do not actually use the busdma API to perform any transfers, but it appears to be handled opaquely by firmware. So really, the goal is to allocate a buffer with the correct constraints. Most likely, you want contigmalloc(9) for this purpose. Something like this: /* Allocate a page of memory below 1GB, with PAGE_SIZE alignment. */
sc->vaddr = contigmalloc(
PAGE_SIZE, /* size */
M_DEVBUF, /* malloc type */
M_ZERO, /* malloc flags */
0, /* low phys addr boundary */
0x3FFFFFFFUL, /* high phys addr bounds */
PAGE_SIZE, 0); /* alignment, boundary */
/* Get the physical address of the new buffer. */
sc->paddr = vtophys(sc->vaddr);(please verify the API usage) | |
Comment Actions
I will update the diff shortly, resolving all comments by mhorne. Thanks.
| sys/arm/broadcom/bcm2835/raspberrypi_virtgpio.c | ||
|---|---|---|
| 253–257 | Moved to probe routine. BTW, I'm not sure too if this is really necessary. I found similar in raspberrypi_gpio.c though. | |
| 288–307 | Thanks! What I really wanted to do is contigmalloc(). The use of busdma was inappropriate. | |
Comment Actions
- Use contigmalloc() rather than busdma to allocate memory region.
Use of busdma to only allocate a physical memory region is inappropriate. Use contigmalloc() and vtophys() instead.
Resolve some small issues pointed by mhorne.
Comment Actions
Added pmap_change_attr(... VM_MEMATTR_UNCACHEABLE) to ensure cache coherence. I will test it on Monday and upload the new patch.
| sys/arm/broadcom/bcm2835/raspberrypi_virtgpio.c | ||
|---|---|---|
| 245 | No. raspberrypi_gpio.c was using sx_xlock() so I left it without thinking. spin mutex should be used here. | |
| 311 | Correct. The if statement is unnecessary. | |
Comment Actions
Unfortunately, changing the memory to uncacheable does not ensure cache coherence. Both parties(firmware and kernel) must have the memory mapped with the same attribute, otherwise undefined behavior will occur.
Please also note that the current implementation of the pmap_change_attr() does not allow its use on memory pages that are mapped multiple times (such DMAP and kernel).
Comment Actions
I tested the updated code and found it works as expected, so I will submit the patch shortly.
During the test, there was no particular difference in operation depending on the presence of VM_MEMATTR_UNCACHEABLE.
Since the firmware only reads physical memory from the GPU, even if the firmware reads the memory before it is written from the cache, it may be that the LED is only momentarily incorrect.
Comment Actions
- Make my memory region uncacheable by pmap_change_attr(... VM_MEMATTR_UNCACHEABLE).
- Use spin mutex rather than sx lock.
Comment Actions
Seems fine to me, with a couple tweaks needed.
I note however that I could not find the brcm,bcm2835-virtgpio compatible string in any official device tree sources. What is meant to provide it?
| sys/arm/broadcom/bcm2835/raspberrypi_virtgpio.c | ||
|---|---|---|
| 139–160 | I do not really understand why the state is split into two fields for "on" and "off". I would think this should be a single bit. But I can say with certainty this code should be simpler. We are setting the LED on or off, according to value. No need for d; no need for now; no need for the printf; no need, really, for the on/off locals. Just a dumb update of the state according to the desired value would be sufficient. Or do I misunderstand something? | |
| 278–279 | Indentation is wrong here. | |
| 283–284 | ||
| sys/arm/broadcom/bcm2835/raspberrypi_virtgpio.c | ||
|---|---|---|
| 275 | The use of this constant is fine, but I did find BCM2838_PERIPH_MAXADDR and bcm283x_dmabus_peripheral_lowaddr() in bcm2835_vcbus.h. These might be more correct/appropriate, but I am not sure. | |
Comment Actions
It looks like it's in downstream dts files, e.g. https://github.com/raspberrypi/linux/blob/45079112ab81154eaf418ff05a989e6fdf63db79/arch/arm/boot/dts/broadcom/bcm2710-rpi-cm3.dts#L143-L148
| sys/arm/broadcom/bcm2835/raspberrypi_virtgpio.c | ||
|---|---|---|
| 143 | This is the logic the Linux driver uses | |
| 245 | Not sure why I used them there, they could probably also become a spin mutex | |
Comment Actions
- Reorganized rpi_virt_gpio_pin_set() by removing unnecessary variables.
- Replaced immediate values with macros defined in bcm2835_vcbus.h.
- Fixed indentation.
Comment Actions
You can find brcm,bcm2835-virtgpio by decompiling the official dtb available from the following link, for example, bcm2710-rpi-3-b.dtb .
https://github.com/raspberrypi/firmware/tree/master/boot
Anyway, I've just uploaded the new patch to address issues pointed by mhorne and andrew. Thanks for comments.
| sys/arm/broadcom/bcm2835/raspberrypi_virtgpio.c | ||
|---|---|---|
| 139–160 | Considering alignment and endianness, I suppose it is better to avoid treating uint32_t sc->state[pin] directly as a uint16_t array. So I think on and off are necessary. OTOH, d and now are delete because they are not necessary. | |
| 143 | Under normal operation, when the GPIO pin is asserted 65536 times, ‘on’ overflows first and becomes 0. At that point, the condition d>0 no longer evaluates to true, indicating the Linux counterpart is incorrect. | |
| 275 | I think BCM2838_PERIPH_MAXADDR is appropriate here. | |
Comment Actions
- Reorganized rpi_virt_gpio_pin_set() by removed unnecessary variables.
- Replaced immediate values with macros defined in bcm2835_vcbus.h.
- Fixed indentation.
Comment Actions
D51578 removed gpiobus_attach_bus in favor of gpiobus_add_bus & bus_attach_children, so this needs to be updated.
Apologies for the inconvenience.