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