riscv jh7110 gpio driver enhancements

[bscott_bunyatech.com.au]

Summary

Fix reporting of state and capabilities by the gpioctl command
Support selection of pull-up and pull-down resistors
Support second gpio device (aon - always on power domain) to allow attaching gpioled device to visionfive2 status LED or querying boot selection switches.

Test Plan

Tested on 15 (visionfive2 sbc) and full patch applies cleanly to 16 (i.e. source file jh7110_gpio.c is unchanged).
Tested with gpioled devices displaying morse code.
Tested with gpioctl command to toggle and query outputs and adjust pull-up/down settings
Tested with gpioths device
Tested with ow_temp device on ow bus (small defensive change needed to owc_gpiobus which I'll put in another review later)
Tested with ds3231 clock on iicbb iicbus (outstanding problem with iicbb working properly at boot time but works when ds3231 driver loaded after boot)
dts source available for overlays if required

[mhorne]

Hi, thanks for this submission.

As you noted on the mailing list, I do have some WIP updates to this driver, but they are half-baked and stalled a while ago. I would be happy to take these changes first.

A few notes/questions inline, before we proceed.

sys/riscv/starfive/jh7110_gpio.c

51

Seemingly unused; is that intended?

220–221

Where does the name RGPIO come from?

273–282

I am not sure if we want to clear these settings or not. They might be preconfigured by firmware for a reason. Was it needed in your testing?

But, something is wrong here; PAD_DRIVESTRENGTH is itself a shifted bit (1 << 1), not a shift value. 3 should become a named constant.

293

Why have you added the -1? It would remove the low bit from the mask.

362–364

It's actually a pointer (not obvious).

370

[bscott_bunyatech.com.au]

Thanks for looking at this. It will be next week before I can test any changes and put up a new, tested diff. I'm away from home at the moment and don't trust doing a restart remotely. I've managed to get the restart working more often that not (it simply doesn't work by default) but haven't got anything working well enough to publish or trust

sys/riscv/starfive/jh7110_gpio.c

51

Just for completeness. The rest of the value is documented so I was just finishing it off.

220–221

JH7110_Datasheet.pdf,
JH7110_TRM_StarFive_Preliminary_V2.pdf,
various web pages.

It's how the official documentation seems to differentiate between the GPIO pins on the main controller and the ones on the AON (always on) controller.
I have no idea what the 'R' stands for.

273–282

Agree that the 3 should be a named constant.
PAD_DRIVESTRENGTH is simply the bit position of the drivestrength field (2 bits starting at bit 1) and I want to mask out both bits of the field (the 3) and the PAD_SLEW bit.
I spent a while thinking about preset values here. My conclusion was that when we start handling a pin as a simple GPIO on/off control or input, the preset values from the dtb or defaults from the startup iomux behavior go out the window. We aren't doing the original function any more. On the JH7110 all pins have predefined functions at startup that aren't simple GPIO functions.
The alternative (and theoretically better) approach would be to implement pinctrl functions in the driver here so this could be set correctly for any intended use. Using the pinctrl functions in u-boot by providing a dtb overlay in u-boot is very messy. Updating the dtb to the earlier stage u-boot/opensbi would be even more difficult.
For a little while I experimented with setting both values in the driver but removed the code from this patch because there is no way to request this from user space.

293

Yes, another case where I should have used a named constant and maybe a comment.
The intent here is to keep the output state initially as the same as had been set previously. I have been playing around with the 1-wire, dht22, and iicbb stuff and was concerned that the time between switching to output and setting the value may be producing a glitch that may cause problems to connected devices. As it turns out, all of these devices only use 0 for output and a resistor pulled up input for 1 so all would be fine with an initial push to 0 on switching to output.
I left it in because it seemed like a polite thing to do as a default action for any other uses.

362–364

Yes. Noted.

370

likewise.

[bscott_bunyatech.com.au]

As described in my previous comments. Now tested and shown to be working.

[bscott_bunyatech.com.au]

I think I may have had some brain fade about shifting with a shifted number.

New version of the patch coming real soon.

[bscott_bunyatech.com.au]

Updated for very confusing way of describing the pin drive strength field that had resulted in an off by one error that cleared the pull-up control.

[mhorne]

LGTM! Thanks for your work. I will make the necessary style tweaks before committing (I noted a couple).

sys/riscv/starfive/jh7110_gpio.c

51–52

376–393

[mhorne]

sys/riscv/starfive/jh7110_gpio.c

273–282

I agree with your thinking!

I have a branch with some other WIP changes to this driver, including the pinctrl stuff. I am not sure if I will get to updating it soon, but if so I'll tag you.