Hi Mark, On 2024-05-12 22:41, Mark Kettenis wrote: >> Date: Sat, 11 May 2024 22:55:18 +0200 >> From: Jonas Karlman <jo...@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 <jo...@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(-) >>>>>> >>>>> >>>> >>>> >> >>
