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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 15/09/2023 5:38 pm, Quentin Schulz wrote:
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@xxxxxxxxxxxxxx> 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@xxxxxxxxxx> 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?

This is why I think it makes the most sense to describe the initial I/O domain voltage as a property of the I/O domain, since that is most truthful to what is actually needed to initialise the hardware. Ideally we could then just use that initial configuration to probe successfully, and use a notifier to pick up the regulator if and when it does appear later (on the basis that the voltage should not be able to change *without* one). And otherwise if an initial voltage isn't specified then we can assume it's OK to wait for the regulator and query it as normal.

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

Hopefully that would just be a sensible non-cyclic design that avoids this problem altogether. However there's nothing special about PMICs, this could equally apply if the I/O domain was supplied by a GPIO regulator or PWM regulator which used one of its own pins; the fundamental problem to solve is being able to determine the correct initial voltage setting for an I/O domain without having to perform any I/O via that domain itself. That is the physical dependency cycle which can exist here, regardless of how we address any other software dependencies between drivers within Linux.

Thanks,
Robin.


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

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/linux-rockchip




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux