[PATCH 1/3] pinctrl: rockchip: add support for io-domain dependency

[Quentin Schulz]

Hi all,

On 9/15/23 08:51, Sascha Hauer wrote:
> On Wed, Sep 13, 2023 at 01:48:12PM -0700, Saravana Kannan wrote:
>> On Tue, Sep 12, 2023 at 11:58 PM Sascha Hauer <s.hauer at pengutronix.de> wrote:
>>>
>>> On Wed, Sep 13, 2023 at 12:37:54PM +0800, Chen-Yu Tsai wrote:
>>>> On Tue, Sep 12, 2023 at 4:07 PM Linus Walleij <linus.walleij at linaro.org> wrote:
>>>>>
>>>>> Top posting to bring Saravana Kannan into this discussion.
>>>>>
>>>>> This looks like a big hack to me, Saravana has been working
>>>>> tirelessly to make the device tree probe order "sort itself out"
>>>>> and I am pretty sure this issue needs to be fixed at the DT
>>>>> core level and not in a driver.
>>>>
>>>> We could merge all the IO domain stuff into the pinctrl node/driver,
>>>> like is done for Allwinner? Maybe that would simplify things a bit?
>>>
>>> I thought about this as well. On Rockchip the pinctrl driver and the IO
>>> domain driver even work on the same register space, so putting these
>>> into a single node/driver would even feel more natural than what we have
>>> now.
>>

While technically not really wrong, I wouldn't say this is true either.

There's no real pinctrl IP address space on Rockchip SoCs (at least the 
ones I worked on, so RK3399 and PX30), at least nothing delimited 
properly. The typical pinctrl duties are scattered all over two (more 
depending on the SoC maybe?) register address spaces, for PX30 and 
RK3399 they are called GRF and PMUGRF. Many, MANY, IPs actually have 
some registers to modify in those two register address spaces as well, 
c.f. all the rockchip,grf and rockchip,pmu properties all over the place.

The pinctrl driver does refer those two register address spaces via the 
aforementioned DT properties. Those two register address spaces are 
represented as syscon... because if I remember correctly that's how you 
are supposed to handle multiple devices on the same register address 
space where registers or even register bitfields are mixed for different 
IPs?

At the same time, IO domains also aren't in their own "real" address 
space, similar as to how pinctrl is handled in HW.

>> Then we should try to do this and fix any issues blocking us.
>>
>>> However, with that the pinctrl node would get the supplies that the IO
>>> domain node now has and we would never get into the probe of the pinctrl
>>> driver due to the circular dependencies.
>>
>>  From a fw_devlink perspective, the circular dependency shouldn't be a
>> problem. It's smart enough to recognize all cycle possibilities (since
>> 6.3) and not enforce ordering between nodes in a cycle.
>>
>> So, this is really only a matter of pinctrl not trying to do
>> regulator_get() in its probe function. You need to do the
>> regulator_get() when the pins that depend on the io-domain are
>> requested. And if the regulator isn't ready yet, return -EPROBE_DEFER?
> 
> That's basically what my series does already, I return -EPROBE_DEFER
> from the pinctrl driver when a pin is requested and the IO domain is not
> yet ready.
> 
>>
>> Is there something that prevents us from doing that?
> 
> No. We could do that, but it wouldn't buy us anthing. I am glad to hear
> that fw_devlink can break the circular dependencies. With this we could
> add the supplies to the pinctrl node and the pinctrl driver would still
> be probed.
> 
> With the IO domain supplies added to the pinctrl node our binding would
> be cleaner, but still we would have to defer probe of many requested
> pins until finally the I2C driver providing access to the PMIC comes

I don't think there's any way around the deferral "of many requested 
pins until finally the I2C driver providing access to the PMIC comes 
along", this is actually necessary.

> along. We also still need a "Do not defer probe for these pins" property
> in the pingrp needed for the I2C driver.
 >

Yes, this is the difficult part right now. In the RFC, I suggested to 
have an io-domains property per pinmux DT node:

"""
&pinctrl {
     group {
         pinmux {
              io-domains = <&io_domains>;
              rockchip,pins = <0 RK_PA0 0 &pcfg_pull_none>,
                          <3 RK_PB5 0 &pcfg_pull_none>;
         };
     };
};
"""

But this is very tedious for both SoC maintainers (though they would 
probably have to do it "only" once) AND for board maintainers, for each 
new pinmux they require. Since the SoC maintainer cannot know on which 
i2c (or spi?) bus the PMIC will be, they have two choices: either let 
board maintainers not forget to add the io-domains property to the 
i2c/spi pinmux nodes of all but the one to whcih the PMIC is attached, 
or have the board maintainers add a /delete-property/ io-domains to the 
proper i2c/spi pinmux node to which the PMIC is attached.

The first one is very error-prone, the second is not very liked by DT 
people I think (and also... well is a hack on DT level to manage a 
driver/subsystem issue).

Also on a side note, the current binding for the io-domains is a bit not 
granular enough as it represents the collection of IO domains on the 
same register address space, and you can have multiple ones. e.g. for 
RK3399 you have four in "grf"/"normal" IO domain, which makes the 
inter-dependencies unnecessarily complex (but that's probably tangent to 
the current problem in discussion).

> I would consider this being a way to cleanup the bindings, but not a
> solution at DT core level that Linus was aiming at.
> 
>>
>>>>
>>>> IIRC on Allwinner SoCs the PMIC pins don't have a separate power rail,
>>>> or if they do they almost certainly use the default I/O rail that is
>>>> always on, and so we omit it to work around the dependency cycle.
>>>
>>> I looked into sun50i as an example. This one has two pinctrl nodes, pio
>>> and r_pio. Only the former has supplies whereas the latter, where the
>>> PMIC is connected to, has (found in sun50i-a64-pinephone.dtsi):
>>>
>>> &r_pio {
>>>          /*
>>>           * FIXME: We can't add that supply for now since it would
>>>           * create a circular dependency between pinctrl, the regulator
>>>           * and the RSB Bus.
>>>           *
>>>           * vcc-pl-supply = <&reg_aldo2>;
>>>           */
>>> };
>>>
>>> At least it show me that I am not the first one who has this problem ;)
>>>
>>> We could add the supplies to the pingroup subnodes of the pinctrl driver
>>> to avoid that, but as Saravana already menioned, that would feel like
>>> overkill.
>>

I think this is actually a somewhat bad idea. Let me explain.

Nothing prevents the user to create a new DT node with two pins from 
different IO domains. e.g. I could very well have the following:

"""
&pinctrl {
     group {
         two_iodomain_mux {
              rockchip,pins = <0 RK_PA0 0 &pcfg_pull_none>,
                          <3 RK_PB5 0 &pcfg_pull_none>;
         };
     };
};
"""

for example if I have a device that uses GPIO0_A0 and GPIO3_B5 as gpios 
and I need to configure their pinconf appropriately.

So from this, I guess we'd need to support multiple io-domains per node 
(don't know the proper pinctrl subsystem name for that one sorry, the 
two_iodomain_mux one in the above example).

We could also now group pinmux nodes by their io-domain, e.g.:

"""
&pinctrl {
     bt656-io-domain {
         power-supply = <&whatever>;

         only_pinmuxes_from_bt656 {
         };

         only_pinmuxes_from_bt656_2 {
         };
     };
     pmu1830-io-domain {
         power-supply = <&something>;

         only_pinmuxes_from_pmu1830 {
         };

         only_pinmuxes_from_pmu1830_2 {
         };
     };
     [...]
};
"""

This means we would need to go through all existing pinmux definition on 
rockchip devices and check if they belong to the same io domain and if 
they don't, split them in one or more pinmuxes for example.

Also, I think it'd be easier to ask board maintainers to only add a 
power-supply property to all io-domains rather than to each and every 
pinmux.

We could probably enforce that no subgroup other than the one named 
after the ones named after the io-domain can be created on the driver 
level as well. Not sure if it's wise but we could probably also check 
that within a pingroup only pinmuxes belonging to the io-domain are listed.

>> So my comment yesterday was that it'd be an overkill to make every
>> struct pin_desc into a device. But if you can split that rockchip
>> pinctrl into two devices, that should be okay and definitely not an
>> overkill.
>>
>> Maybe something like:
>>
>> pinctrl {
>>      compatible = "rockchip,rk3568-pinctrl";
>>      i2c0 {
>>                  /omit-if-no-ref/
>>                  i2c0_xfer: i2c0-xfer {
>>                          rockchip,pins =
>>                                  /* i2c0_scl */
>>                                  <0 RK_PB1 1 &pcfg_pull_none_smt>,
>>                                  /* i2c0_sda */
>>                                  <0 RK_PB2 1 &pcfg_pull_none_smt>;
>>                  };
>>      }
>>      ...
>>      ...
>>      pinctrl-io {
>>          compatible = "rockchip,rk3568-pinctrl-io";
>>          pmuio1-supply = <&vcc3v3_pmu>;
>>          cam {
>>              ....
>>          }
>>          ....
>>          ....
>> }
>>
>> So pinctrl will probe successfully and add it's child device
>> pinctrl-io. i2c0 will probe once pinctrl is available. Then eventually
>> the regulator will probe. And after all that, pinctrl-io would probe.
>>
>> This has no cycles and IMHO represents the hardware accurately. You
>> have a pinctrl block and there's a sub component of it (pinctrl-io)
>> that works differently and has additional dependencies.
>>
>> Any thoughts on this?
> 

Just to be clear that whether i2c0 is where the PMIC is is HW dependent, 
so we cannot have that in the main SoC dtsi (on Rockchip we typically 
have a bunch of those in the main SoC dtsi to avoid common nodes to be 
copy-pasted all over the place).

> By making the IO domain device a child node of the pinctrl node we
> wouldn't need a phandle from the pinctrl node to the IO domain node
> anymore, but apart from that the approach is equivalent to what we have
> already.
> 

Indeed, just one less item in the cyclic dependency but it's still there.

> Given that fw_devlink allows us to add the supplies directly to the
> pinctrl node, I would prefer doing that. But as said, it doesn't solve
> the problem.
> 

Absolutely.

The issue is that we have pinctrl that needs to probe for anything to 
work really.

Considering that pinctrl pingroups requires (on the HW level) to be 
linked to an IO domain to be working properly, the IO domain depending 
on a regulator (which can have different voltages at runtime, hence why 
this link is absolutely critical to not damage the SoC by having the IO 
domain configured for a different voltage than provided by its 
regulator), which is on a bus (i2c/spi) that needs a specific pinmux in 
order to work.

Saravana gave one example of the cyclic dependency on the DT level 
earlier. The issue isn't the DT-part of the cyclic dependency, it's that 
the drivers actually also have this cyclic dependency, the i2c/spi 
controller via its pinctrl default state and the pinctrl driver with a 
dependency on a PMIC driver that could'nt have been probed yet because 
its on the i2c bus. I don't see how we can not have a special property 
in the DT binding for ignoring this cyclic dependency for one specific 
loop. We cannot hardcode the driver to look for a specific compatible or 
something like that because this is HW dependent, there's no rule on 
which i2c/spi bus one needs to put their PMIC on. Maybe we could try to 
look for the PMIC on child nodes of consumers of pinctrl (if possible 
only when a cyclic dependency is detected) and bypass this dependency on 
the regulator? Or maybe check if the parent of the PMIC of the IO domain 
of the currently requested pingroup is the same as the consumer of the 
currently requested pinmux within this pingroup?

I'm also wondering how this would play out if the PMIC isn't supplying 
power to the IO domain the bus controller to which it's connected... but 
I guess that's a HW design issue :)

It's Friday evening here so hopefully my brain wasn't already on weekend 
mode and I could convey properly everything I had in mind.

Cheers,
Quentin