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

[Jonas Karlman]

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.

> 
>> 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?

> 
>> 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(-)
>>>>>>
>>>>>
>>>>
>>>>
>>
>>