[PATCH 0/4] rockchip: Add gpio request() ops and drop PCIe reset-gpios workaround
Alex Bee
knaerzche at gmail.com
Wed May 22 21:46:39 CEST 2024
Am 22.05.24 um 18:20 schrieb Jonas Karlman:
> On 2024-05-22 16:18, Alex Bee wrote:
>> Am 13.05.24 um 01:22 schrieb Jonas Karlman:
>>> On 2024-05-13 00:34, Alex Bee wrote:
>>>> Am 12.05.24 um 23:37 schrieb Jonas Karlman:
>>>>> Hi Alex,
>>>>>
>>>>> On 2024-05-12 21:49, Alex Bee wrote:
>>>>>> Am 11.05.24 um 20:47 schrieb Jonas Karlman:
>>>>>>> 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 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?
>>>>>> No, no general ("technical") issue with adding a .request hook to the gpio
>>>>>> driver. But now you are now moving the original workaround to an even more
>>>>>> invisible place which does things implicitly. Maybe just don't remove the
>>>>>> pinctrl from the boards u-boot-dtsi's - just replace it with
>>>>>>
>>>>>> &pcie30x2m1_pins {
>>>>>> rockchip,pins =
>>>>>> <2 RK_PD4 4 &pcfg_pull_none>,
>>>>>> <2 RK_PD6 RK_FUNC_GPIO &pcfg_pull_none>,
>>>>>> <2 RK_PD5 4 &pcfg_pull_none>;
>>>>>> };
>>>>>>
>>>>>> Even if it would (now) work without. It, at least, documents that there are
>>>>>> things left to do for the upstream DT.
>>>>> This is what I was doing when testing PCIe on ROCK 3B, I would still like
>>>>> to drop the "bad" workaround for ROCK 3A and E25 and have it fixed in
>>>>> upstream DT and let it trickle back now that RK356x use OF_UPSTREAM.
>>>>>
>>>>> I do not see the point of keeping a workaround that no longer is needed,
>>>>> especially when there is plans to also adjust upstream DT.
>>>> Yeah, sure: but that certainly should be done/happen first, before it's
>>>> removed here. Everybody knows how long that takes, patches being forgotten
>>>> ....
>>> I am fully aware of patches being forgotten :-)
>>>
>>> Still, here I try to cleanup a bad workaround that I probably never should
>>> have added in the first place.
>> Why is it bad? It's incomplete and the correct pin configuration should
>> have been added to the upstream board DT in the first place. That's what
>> *-u-boot.dtsi are doing at times :) It's not as worse as adding implicit
>> pin function switching to a driver to compensate the incomplete/incorrect
>> upstream board device tree, imho.
> From my point-of-view the upstream pinctrl is more correct and I wrongly
> added a patch that completely disregarded existing pinctrl just to fix
> pci enumeration in U-Boot when no pci device is connected without
> thinking of how it could affect linux/bsd, hence why I now think that old
> workaround/hack is something bad and something I want to cleanup/remove.
Yeah, sure. That's why I suggested[0] to replace the current override like
...
-&pcie3x2 {
- pinctrl-0 = <&pcie3x2_reset_h>;
-};
-
-&pinctrl {
- pcie {
- pcie3x2_reset_h: pcie3x2-reset-h {
- rockchip,pins = <2 RK_PD6 RK_FUNC_GPIO &pcfg_pull_none>;
- };
- };
+&pcie30x2m1_pins {
+ rockchip,pins =
+ <2 RK_PD4 4 &pcfg_pull_none>,
+ <2 RK_PD6 RK_FUNC_GPIO &pcfg_pull_none>,
+ <2 RK_PD5 4 &pcfg_pull_none>;
};
....
The proper upstream solution should probably add another pinctrl-set to
rk3568-pinctrl.dtsi - someting like
...
+ /omit-if-no-ref/
+ pcie30x2m1_ep_pins: pcie30x2m1-ep-pins {
+ rockchip,pins =
+ /* pcie30x2_clkreqnm1 */
+ <2 RK_PD4 4 &pcfg_pull_none>,
+ /* pcie30x2_perstnm1 */
+ <2 RK_PD6 RK_FUNC_GPIO &pcfg_pull_none>,
+ /* pcie30x2_wakenm1 */
+ <2 RK_PD5 4 &pcfg_pull_none>;
+ };
+
...
and boards needing this just switch from pcie30x2m1_pins to
pcie30x2m1_ep_pins
> In my mind the current pinctrl use for pcie3 in U-Boot is describing the
> hw much worse than what is done in upstream Linux.
No, not really. It describes pin settings wrong. The DT suggests GPIO2_D6
is used with func4, but in reality its used with func0 (gpio). The only
reason that works correctly is probe order: if the pinctrl driver would
probe after the gpio driver (which is sort of impossible for Rockchip) the
pin would be switched to func4 and the controller wouldn't work.
>>>>>> What you were saying in reply to Mark's email is not completely true: Not
>>>>>> all pins are initialized with gpio func as default. It actually depends on
>>>>>> the pin which function the bootrom sets initially. In case of RK356x's
>>>>>> GPIO2_PD6 (the pin in question) it's BT656_D6M0 (func2), for instance. So,
>>>>>> in fact, you are changing it's function when implicitly setting it's func
>>>>>> to gpio (func0).
>>>>> Sure, bootrom will change pin func on some pins so that it e.g. can read
>>>>> from storage etc, but in general gpio mux is what most pins will use
>>>>> after POR.
>>>>>
>>>>> On my ROCK 3A I see the pcie30x2m1 pins using func0 (gpio) after POR:
>>>>>
>>>>> => pinmux status
>>>>> GPIO2_D4 : gpio
>>>>> GPIO2_D5 : gpio
>>>>> GPIO2_D6 : gpio
>>>>>
>>>>> And with this after a "pci enum":
>>>>>
>>>>> => pinmux status
>>>>> GPIO2_D4 : func-4
>>>>> GPIO2_D5 : func-4
>>>>> GPIO2_D6 : gpio
>>>>>
>>>>> Not sure why your board would use BT656_D6M0 (func2) after POR unless
>>>>> there is some other code writing to the IOMUX regs.
>>>> Oh, I haven't checked on an actual board - I trusted the TRM [0], page 253
>>>> GRF_GPIO2D_IOMUX_H[10:8] is (should be) 0x2. But (sadly) there are lot of
>>>> blobs involved. for RK35xx SoCs - so maybe something switches the func for
>>>> this pin for some reason.
>>>>> The only thing this series changes is that when a U-Boot drivers use e.g.
>>>>> gpio_request_by_name("reset-gpios") the referenced pin in DT will
>>>>> implicitly be configured for gpio func.
>>>>>
>>>>> I would still like to understand if there is any other reason, not
>>>>> related to dropping current "bad" PCIe DT workaround, to not to adopt
>>>>> this implicit configuration and match Linux kernel?
>>>> Generally speaking: device trees _must_ represent the actual hardware
>>>> completely independent from any driver or any software. They are NOT
>>>> helpers or configuration for drivers. Drivers can use them and have to
>>>> adapt to them - not vice versa. So: Being as explict as possible is a must.
>>> I am fully aware of this, and I would argue that the DT already is very
>>> explicit about the HW in regard to PCIe3x2.
>>>
>>> The pinctrl function/group explains what function needs to be activated to
>>> route PCIe3x2 related signals to the PCIe controller.
>>> And the reset-gpios explains what pin is used for PERST# signal.
>>>
>>> However, if we instead are too explicit in the pinctrl function/group and
>>> say that that the PERST# pin should use gpio iomux function, aren't we
>>> then actually just describing something that is relevant for software/driver?
>>>
>>> And actually ends up loosing information on how the pin can be configured
>>> to route the PERST# pin to the controller?
>>>
>>> And as Mark pointed out, the PERST# pin should probably be configured for
>>> gpio output when controller works in RC mode, and should use PERSTn func
>>> when controller works in EP mode.
>>>
>>> Guess that could/should possible be described as different pinctrl
>>> states or if I am not mistaken EP is described as a separate device
>>> using a different compatible.
>> Actually I wouldn't be too surprised if the "reset-gpios" property does
>> actually only exist to not have to define a extra pinctrl and the actual
>> reset-functionality (i.e. pulling the pin up / down by the driver) isn't
>> required at all. Though, untested. That would be similar to define a
>> "cd-gpios" property for a (sd) mmc-controller which is a similar workaround
>> for not switching the card detection pin to gpio func.
> From what I could test the driver need to mux the pin for gpio output
> use when we use the controller in RC mode and the pin should be muxed to
> pcie func when the controller is used in EP mode.
>
> Leaving the pin muxed for pcie func when we use the controller in RC
> mode seem to make the board freeze.
From what you're saying (maybe I should test myself at some point :)
rockchip,pins = <2 RK_PD6 RK_FUNC_GPIO &pcfg_pull_up>;
instead of
rockchip,pins = <2 RK_PD6 RK_FUNC_GPIO &pcfg_pull_none>;
could do the trick and no "reset-gpios" required at all.
>>>> At some point, it is planned, to split whole DT "subsystem" from the linux
>>>> kernel.
>>>> I also have a vague remembering (some mailing list discussion I can't find
>>>> right now), that this whole implicit function switching of pins is sort of
>>>> "not welcome" in linux anymore. So adding it here also is sort of a "step
>>>> back" from that POV.
>>> I was not expecting this much backlash on this series, so thanks for
>>> some concrete feedback on why we maybe should not add the gpio request()
>>> ops :-)
>> I did my best but couldn't find the discussion I had in mind. The context
>> was: pinctrl and gpio are different subsystems and one really shouldn't
>> configure the other. That makes sense, I guess. That it exists for Rockchip
>> in linux kernel might be related to the fact that this originally was a
>> single driver which was splited up later.
>>
> From what I can find in the Linux pinctrl documentation it seem to suggest that
> the gpiolib driver should actually hide pinmuxing from a gpio consumer driver.
>
> See "Pin control interaction with the GPIO subsystem":
>
> """
> NOTE that platforms and individual drivers shall NOT request GPIO pins
> to be controlled e.g. muxed in. Instead, implement a proper gpiolib
> driver and have that driver request proper muxing and other control for
> its pins.
> """
>
> https://www.kernel.org/doc/html/latest/driver-api/pin-control.html#pin-control-interaction-with-the-gpio-subsystem
>
> And from "Drivers needing both pin control and GPIOs":
>
> """
> But there are also situations where it makes sense for the GPIO
> subsystem to communicate directly with the pinctrl subsystem, using the
> latter as a back-end. This is when the GPIO driver may call out to the
> functions described in the section Pin control interaction with the GPIO
> subsystem above. This only involves per-pin multiplexing, and will be
> completely hidden behind the gpiod_*() function namespace. In this case,
> the driver need not interact with the pin control subsystem at all.
>
> If a pin control driver and a GPIO driver is dealing with the same pins
> and the use cases involve multiplexing, you MUST implement the pin
> controller as a back-end for the GPIO driver like this, unless your
> hardware design is such that the GPIO controller can override the pin
> controller’s multiplexing state through hardware without the need to
> interact with the pin control system.
> """
>
> https://www.kernel.org/doc/html/latest/driver-api/pin-control.html#drivers-needing-both-pin-control-and-gpios
>
>> Anyway: I hope we can agree on the point that the correct pinctrl settings
>> for the controller of this board should make it to the upstream device tree
>> in some way :D
> Fully agree that it should be correct in upstream, my point is that on
> one hand I actually think it already is correct in upstream and that
> changing the reset pin to use gpio func as default instead would then be
> done to assist software, and not correctly describe the hw.
>
> On the other hand, if I am not mistaken to use the controller in EP mode
> another compatible/device node needs to be described and if that is the
> case I can fully agree that the device node for RC mode could include
> pinctrl to mux the reset pin for gpio func.
>
> Regardless on what is to be done with pinctrl in DT, I still think it
> will be good to implement these gpio/pinctrl ops in U-Boot to closer
> match "a proper gpiolib driver" as suggested in the Linux pinctrl
> documentation where such driver request proper muxing and other control
> for its pins.
I can't exactly follow where in the documenation you are reading this, but
adding the ops should not be an issue, but DT shouldn't depend on it.
Regards
Alex
[0]
https://lore.kernel.org/all/1b3b8c24-a3d9-4c36-a72f-45baee63f385@gmail.com/
> Anyway, I will merge the two gpio/pinctrl series and rearrange patches,
> and also drop any DT changes in a v2.
>
> Regards,
> Jonas
>
>> Regards,
>> Alex
>>> Maybe I should just drop this and only keep the follow up series.
>>>
>>> Regards,
>>> Jonas
>>>
>>>> Alex
>>>>
>>>> [0]
>>>> https://opensource.rock-chips.com/images/2/26/Rockchip_RK3568_TRM_Part1_V1.3-20220930P.PDF
>>>>
>>>>> Regards,
>>>>> Jonas
>>>>>
>>>>>> Alex
>>>>>>
>>>>>>> Regards,
>>>>>>> Jonas
>>>>>>>
>>>>>>>> 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(-)
>>>>>>>>>
More information about the U-Boot
mailing list