Page MenuHomeFreeBSD

Add Arm pl061 GPIO driver
Needs ReviewPublic

Authored by alisaidi_amazon.com on Mar 13 2020, 10:05 PM.

Details

Reviewers
andrew
cperciva
Group Reviewers
arm64
ARM
Summary

A PL061 is a simple 8 pin GPIO controller. This GPIO device is used to signal an internal request for shutdown on some virtual machines including Arm-based Amazon EC2 instances.

Test Plan

This change is dependent on a change from Andrew that doesn't greedily allocate all irqsrc for all possible gicv3 ITS MSIs so that some are available for the gpio device. In addition to the gpio driver some additional support is required in the acpi implementation to make this an ACPI event that can trigger a power button device and I haven't started to work on that.

I've tested this by loading the driver, seeing the 8 gpio pins as input, confirming that if the gpio device signals an edge on one pin the driver sees this and that the pins can be changed to output and written to. Other than hacking the driver to register an interrupt for a pin and confirming that pl061_intr() ends up being called in this case I haven't found a way to test the interrupt generation and handling in the code yet. Any pointers on how to do this would be appreciated.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

manu added a subscriber: manu.Mar 14 2020, 12:21 PM
manu added inline comments.
sys/dev/gpio/pl061.c
94

You need to return the intr caps (GPIO_INTR_LEVEL_* GPIO_INTR_EDGE_*) for the type of supported interrupts in getcaps but you only return this.

sys/dev/gpio/pl061.c
94

I had tried that originally, but when I do that gpioctl says it doesn't know the other CAPs which made be think that since the direction it set via gpio_pin_setflags and the interrupt handing is set in pic_setup_intr that it the level/edge CAPS shouldn't be returned from gpio_pin_getcaps

For example when providing all the caps gpioctl returns the following:

pin 00:	0	p0<IN>, caps:<IN,OUT,UNKNOWN,UNKNOWN,UNKNOWN,UNKNOWN,UNKNOWN>
pin 01:	0	p1<IN>, caps:<IN,OUT,UNKNOWN,UNKNOWN,UNKNOWN,UNKNOWN,UNKNOWN>
pin 02:	0	p2<IN>, caps:<IN,OUT,UNKNOWN,UNKNOWN,UNKNOWN,UNKNOWN,UNKNOWN>
pin 03:	0	p3<IN>, caps:<IN,OUT,UNKNOWN,UNKNOWN,UNKNOWN,UNKNOWN,UNKNOWN>
pin 04:	0	p4<IN>, caps:<IN,OUT,UNKNOWN,UNKNOWN,UNKNOWN,UNKNOWN,UNKNOWN>
pin 05:	0	p5<IN>, caps:<IN,OUT,UNKNOWN,UNKNOWN,UNKNOWN,UNKNOWN,UNKNOWN>
pin 06:	0	p6<IN>, caps:<IN,OUT,UNKNOWN,UNKNOWN,UNKNOWN,UNKNOWN,UNKNOWN>
pin 07:	0	p7<IN>, caps:<IN,OUT,UNKNOWN,UNKNOWN,UNKNOWN,UNKNOWN,UNKNOWN>

I'm happy to make the change if you believe this is still correct.

manu added inline comments.Mar 20 2020, 1:29 AM
sys/dev/gpio/pl061.c
94

Some drivers do call gpio_pin_getcaps before calling bus_setup_intr, that helps knowing if the underlying hardware support EDGE or LEVEL (or both).
gpioctl(1) didn't knew about interrupts caps, see D24133 for a patch that should fix it.

sys/dev/gpio/pl061.c
94

Great! Will fix the CAPS to be correct here.

Add the interrupt capabilities

emaste added inline comments.Mar 25 2020, 3:15 PM
sys/dev/gpio/pl061.c
5

Minor aside, we've dropped the "All rights reserved." from our standard templates. If it's here only because you found it in a template or copied the header block from an existing file feel free to remove it. You are of course free to include it on files you create if your policy requires it.

andrew added inline comments.Mon, May 18, 12:57 PM
sys/modules/Makefile
578

The pl061 module directory is missing.

sys/modules/Makefile
578

Sorry, I'm missing something basic here I imagine.

I don't see a hierarchy of directories with other modules when I make or install them. What am I missing?

cperciva added inline comments.Thu, May 21, 7:28 PM
sys/modules/Makefile
578

I think andrew is saying that you've added "pl061" to the list of subdirectories which sys/modules/Makefile should recurse into, but sys/modules/pl061 isn't included in this patch (which means that buildkernel will fail when it tries to build the module).