sifive_gpio: Add SiFive GPIO controller driver

[jrtc27]

Summary

This is present on both the FU540 and FU740, but only needed for the
FU740 in order to assert reset and power enable signals for its PCIe
controller.

[mhorne]

Looks straightforward to me, with a couple of questions. I have never written a gpio driver myself, so maybe @manu or @mmel can give it a quick look.

sys/riscv/sifive/sifive_gpio.c

30

I would be interested in implementing this, but not sure how soon that would be...

218

Is it ever the case that gp_pin != i? Seems like these loops could be removed, if not.

396

No detach method?

[manu]

Looks ok to me but I suggest implementing the gpio_pin_access_32 and gpio_pin_config_32 too.
Interrupts can be implemented after.

sys/riscv/sifive/sifive_gpio.c

161

Is the device_get_unit really necessary ?
That would mean that a sifive_gpio1 driver expose sifive_gpio1.X and since you would need to -f /dev/gpioc1 too that's a bit redondant.
Same for the 'sifive_gpio' prefix, for Allwinner and Rockchip we name them after the pad/bank-pin name as this is what is used in the schematics of the different boards.

396

Detach for gpio controller is complicated as it would mean to track any driver that uses a gpio. Also if there is any driver (which on embedded is more than likely as you often have reset lines etc ...) you couldn't detach.

[skibo]

sys/riscv/sifive/sifive_gpio.c

381

I think you need to update sc->gpio_pins[i].gp_flags here.

[jrtc27]

sys/riscv/sifive/sifive_gpio.c

161

I was using other drivers like dwgpio and qoriq_gpio as a reference. If what they're doing also doesn't make a huge amount of sense then this I can just change this.

The schematics just name them GPIOn, with a note saying what some of them are connected to.

218

This code is indeed weird. A bunch of other drivers have this exact pattern (basically most drivers using struct gpio_pin, there are some exceptions that do things more simply) and as far as I can tell will also never have the pins not match, I just copied this idiom from them... So maybe I should drop these loops and just assert that sc->gpio_pins[pin].gp_pin == pin?

[mhorne]

sys/riscv/sifive/sifive_gpio.c

218

Yeah, seems okay to me to drop the loops here.

Looks like ar5315_gpio and ar71xx_gpio are drivers where this mapping isn't 1:1, but for most it doesn't matter.

[jrtc27]

• Use GPIOn for names to match schematic rather than verbose/redundant sifive_gpioX.Y
• Update gp_flags in setflags to keep in sync with hardware state
• Drop loops searching through gpio_pins and just index directly given the numbering is always the identity
• Make gpio_pin_get return output status for output pins to match other drivers (many get it for free by using the same register for both in hardware with its meaning dependent on the direction, others manually do something like this to give equivalent behaviour)
• Add gpio_pin_access_32 and gpio_pin_config_32 implementations

[mhorne]

There is a list of gpio drivers in the gpiobus(4) man page. It is not up to date, but you should add an entry for this driver.

sys/riscv/sifive/sifive_gpio.c

407–408

If you did the reads after the loop, you could reduce the time spent holding the lock. I don't think it's a big deal.

[mhorne]

Forgot to hit accept :)

[jrtc27]

sys/riscv/sifive/sifive_gpio.c

407–408

That's true, though I'd then need [io]{set,clear} variables, and if this driver's lock is contended then I have serious questions... :)

[jrtc27]

sys/riscv/sifive/sifive_gpio.c

407–408

Actually no, the sc->gpio_pins[i].gp_flags all need protecting by the lock too, so reordering the loop with the I/O reads doesn't let you delay taking the lock