[PATCH 0/4] rockchip: Add gpio request() ops and drop PCIe reset-gpios workaround

Tue May 21 23:30:48 CEST 2024

> Date: Mon, 13 May 2024 00:23:33 +0200
> From: Jonas Karlman <jonas at kwiboo.se>

Hi Jonas,

Sorry for the delayed reply.  Last week was a bit busy here...

> Hi Mark,
> 
> On 2024-05-12 22:41, Mark Kettenis wrote:
> >> Date: Sat, 11 May 2024 22:55:18 +0200
> >> From: Jonas Karlman <jonas at kwiboo.se>
> >>
> >> Hi Mark,
> >>
> >> On 2024-05-11 21:57, Mark Kettenis wrote:
> >>>> Date: Sat, 11 May 2024 20:47:40 +0200
> >>>> From: Jonas Karlman <jonas at kwiboo.se>
> >>>
> >>> Hi Jonas & Alex,
> >>>
> >>>> Hi Alex,
> >>>>
> >>>> On 2024-05-11 19:44, Alex Bee wrote:
> >>>>> Hi Jonas,
> >>>>>
> >>>>> Am 11.05.24 um 13:28 schrieb Jonas Karlman:
> >>>>>> This series add gpio request() and pinctrl gpio_request_enable() ops so
> >>>>>> that a gpio requested pin automatically use gpio pinmux and U-Boot
> >>>>>> behaves more similar to Linux kernel.
> >>>>>
> >>>>> I'm not sure that's a good idea.
> >>>>> While linux does it the same way, we really shouldn't expect every 
> >>>>> software/os/ … which uses DT (now or in future) to implicitly switch the 
> >>>>> pin function when using a pin as gpio. So the real fix would probably be 
> >>>>> to add the the correct pinctrl settings to the upstream DT of those 
> >>>>> boards and sync it later on (not sure those if those SoCs already using 
> >>>>> OF_UPSTREAM) and leave the -u-boot.dtsi-"hack" alone for now.
> >>>
> >>> I missed Alex's mail, but OpenBSD certainly is one of the OSes that
> >>> could break if the pinctrl settings get removed.  We currently have no
> >>> code to automatically mux the gpio pins on these Rockchip SoCs. I
> >>> suppose as long as U-Boot probes the PCIe bus, the pins will already
> >>> be muxed correctly and things will continue to work.  But I think
> >>> there are certain boot scenarios where this won't happen.
> >>
> >> The control FDT on the known affected boards was only patched to prevent
> >> the boards from a full device freeze when PCIe was enumerated. An OS
> >> should probably not depend on a workaround made for U-Boot use.
> > 
> > Sure, but I think that means we should try to minimise the number of
> > workarounds in U-Boot ;).
> 
> Agree, and something this series tries to accomplish, removing one "bad"
> workaround so that once upstream DT is fixed and it trickle down to 
> dts/upstream/ nothing else needs to be changed in U-Boot.

To me it looks as if you're trying to replace an "incomplete" fix with
a "questionable" workaround.

> >> Is it common for *BDS to use the control FDT or is it more common that a
> >> separate .dtb-file to be loaded during boot?
> > 
> > Not sure what the other BSDs do; at this point they're largely
> > separate codebases that just share a common ancestry.  And I don't
> > really like the term "control FDT"; in ideal world there should be
> > just be a single FDT that gets used by the various firmware layers,
> > U-Boot and the OS.  But yes, my goal is to make OpenBSD work with FDT
> > provided by U-Boot without loading a separte .dtb file.
> 
> Thanks for the insights. Typically I test linux-next kernel using the
> FDT provided by U-Boot. Any suggestions on how I can get a minimal
> OpenBSD snapshot to boot on e.g. a RK356x board?

Very simple.  Just take it a "miniroot" image, e.g. the 7.5 release
image:

  https://ftp.openbsd.org/pub/OpenBSD/7.5/arm64/miniroot75.img

and use dd to write it to a micro-SD card or a USB flash drive.  I
usually put OpenBSD on a USB flash drive and U-Boot on a micro-SD card
for testing.  If you prefer to use a micro-SD card, just use dd to
write u-boot-rockchip.bin on top of the "miniroot" image.

This will boot into the installer, but that is probably good enough
for testing U-Boot.

> >> To recap, the issue on e.g. ROCK 3A is that the upstream DT use pinctrl
> >> to signal PCIE30X2 CLKREQn_M1/WAKEn_M1/PERSTn_M1 function, DT also
> >> specify a reset-gpios prop pointing on PERSTn_M1. To me this seem like a
> >> correct description of HW.
> > 
> > Actually I'm starting to think that it isn't.  If you look at table
> > 11-11 in Part 2 of the RK3588 TRM (V1.0) you'll see that the
> > pcie_perst_n signal is designated as input.  The corrsponding chapter
> > is missing from the copy of the RK3568 TRM I have, but it is likely
> > that the same applies to the PCIe conroller on the RK3568 SoC.  Now
> > these PCIe controllers can function in either Root Complex (RC) or
> > Endpoint (EP) mode.  And given the the pcie_perst_n signal is
> > designated as input I think that means that the PERSTn_M1 function
> > only makes sense when the controller is in EP mode.  So for the
> > majority of the boards that use the PCIe controller in RC mode, the DT
> > should only configure the CLKREQn_M1 and WAKEn_M1 functions and
> > configure the pin that is used to output PERST# as a GPIO.
> 
> Thanks for this insights!
> 
> I can also clarify that the freeze only happens when a PCIe device is
> not attached, removing the reset-gpio prop and leaving the pin in
> PERSTn_M1 function my nvme drive is discovered correctly. Without
> anything attached the device freeze during "pci enum".
> 
> > 
> >> When U-Boot probe the PCIE30X2 device the pinctrl driver would
> >> automatically configure CLKREQn_M1/WAKEn_M1/PERSTn_M1 function.
> >>
> >> When U-Boot RK PCIe driver try to use the reset-gpios pin the device
> >> would freeze unless the PERSTn pin is first re-configured for gpio use.
> >>
> >> Current workaround in U-Boot throws away the existing upstream pinctrl
> >> and replace it to only configure gpio func on the PERSTn pin. This also
> >> leaves the CLKREQn and WAKEn to incorrectly use gpio func.
> >>
> >> With this series I try to cleanup the old workaround that I added without
> >> much insight beyond: device no longer freeze and nvme "works" :-)
> >>
> >>>
> >>> I've wondered in the past what the purpose of the gpio-ranges
> >>> properties was.  I never considered that they could be used to
> >>> automatically mux the pins for GPIO since the gpio-ranges mapping
> >>> provides no indication of what the correct pin settings arefor the
> >>> GPIO pins.
> >>
> >> The gpio-ranges prop is only used to provide details on how the pins are
> >> mapped between gpio and pin controller. It is not used to change any
> >> muxing. But the property is required to make the U-Boot helpers
> >> pinctrl_gpio_request()/pinctrl_gpio_free() work and figure out what
> >> pinctrl udevice to use if another driver use e.g. gpio_request_by_name()
> >> to request use of a pin for gpio use.
> > 
> > But that means you rely on the Linux (and now U-Boot) driver
> > interfaces.  What I was saying is that this is all rather implicit and
> > not explicitly documented in the device tree bindings.
> 
> I fully understand this, however, from a developer perspective and
> seeing how there is no API to configure pinmux from code but there is an
> API to request and work with gpio, I would expect that the abstraction
> layer handle anything needed to be able to use the returned gpio.
> 
> > 
> >> Keep in mind that this series should not change any behavior except for
> >> the special case when DT pinctrl may configure a pin for non-gpio
> >> function and later U-Boot request the pin to be used for gpio.
> > 
> > To me such a case would be suspicious.  What would be the purpose of
> > such a configuration?  And wouldn't this mean that you now implictly
> > rely on the gpio being requested after the pinctl config has been
> > applied?
> 
> Main purpose is to ensure the U-Boot gpio API works as expected for
> debugging and development purposes. For final DTs I am expecting that
> pinctrl is configured correctly.
> 
> Regards,
> Jonas
> 
> > 
> >>>> I fully agree that the pinctrl for the problematic boards should be
> >>>> corrected in upstream DT, but that is a separate issue and should not
> >>>> block adding support for the request()/gpio_request_enable() ops.
> >>>>
> >>>> While the pcie reset-gpios full board freeze that was my driving factor
> >>>> to fully implement the gpio request() ops it is not the only use case,
> >>>> using the gpio cmd on a pin that use a non-gpio pinmux is another.
> >>>>
> >>>> Or do you see any technical issue with having the gpio request() ops
> >>>> implemented and having it ensure gpio pinmux is used on a gpio requested
> >>>> pin? Similar to how gpio/pinctrl is behaving in Linux and on some other
> >>>> platforms in U-Boot?
> >>>
> >>> Well, it removes the incentive to fix the upstream DTs and would make
> >>> it harder to notice missing pinctrls in the DTs.
> >>
> >> I can understand that, but gpio pinmux is default so any missing pinctrl
> >> would still cause issues, this changes nothing in that regard.
> >>
> >> This only affect if pinctrl exists and pin gets configured for non-gpio
> >> function use. And later a driver/cmd tries to use such pin for gpio use.
> >> Before this patch the use of gpio_request() would not change the pin mux
> >> for gpio use. After this series when code explicily request a pin for
> >> gpio use the pinctrl driver gets notified that it should change pinmux
> >> of the pin for gpio use.
> >>
> >> Regards,
> >> Jonas
> >>
> >>>
> >>> Cheers,
> >>>
> >>> Mark
> >>>
> >>>>> Alex
> >>>>>>
> >>>>>> With the gpio and pinctrl ops implemented this series also remove a PCIe
> >>>>>> reset-gpios related device lock-up workaround from board u-boot.dtsi.
> >>>>>>
> >>>>>> PX30, RK3066, RK3188, RK356x and RK3588 are the only SoCs that currently
> >>>>>> define gpio-ranges props and is affected by this series.
> >>>>>>
> >>>>>> A follow up series adding support for the pinmux status cmd will also
> >>>>>> add gpio-ranges props for remaining RK SoCs.
> >>>>>>
> >>>>>> Jonas Karlman (4):
> >>>>>>    pinctrl: rockchip: Add gpio_request_enable() ops
> >>>>>>    gpio: rockchip: Add request() ops
> >>>>>>    rockchip: rk3568-rock-3a: Drop PCIe reset-gpios workaround
> >>>>>>    rockchip: rk3568-radxa-e25: Drop PCIe reset-gpios workaround
> >>>>>>
> >>>>>>   arch/arm/dts/rk3568-radxa-e25-u-boot.dtsi     | 12 -------
> >>>>>>   arch/arm/dts/rk3568-rock-3a-u-boot.dtsi       | 12 -------
> >>>>>>   drivers/gpio/rk_gpio.c                        | 10 ++++++
> >>>>>>   .../pinctrl/rockchip/pinctrl-rockchip-core.c  | 31 +++++++++++++++++++
> >>>>>>   4 files changed, 41 insertions(+), 24 deletions(-)
> >>>>>>
> >>>>>
> >>>>
> >>>>
> >>
> >>
> 
> 