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

[Sascha Hauer]

On Thu, Sep 21, 2023 at 01:49:21PM -0700, Saravana Kannan wrote:
> On Thu, Sep 21, 2023 at 6:57 AM Sascha Hauer <s.hauer@xxxxxxxxxxxxxx> wrote:
> >
> > On Wed, Sep 20, 2023 at 03:00:28PM -0700, Saravana Kannan wrote:
> > > On Thu, Sep 14, 2023 at 11:51 PM Sascha Hauer <s.hauer@xxxxxxxxxxxxxx> 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.
> > > > >
> > > > > 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
> > > > along. We also still need a "Do not defer probe for these pins" property
> > > > in the pingrp needed for the I2C driver.
> > >
> > > Sorry about the slow reply. Been a bit busy.
> > >
> > > Oh, this is not true though. With the example binding I gave,
> > > fw_devlink will automatically defer the probe of devices that depend
> > > on pins that need an iodomain/regulator.
> > >
> > > 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 {
> > >             ....
> > >         }
> > >         ....
> > >         ....
> > > }
> > >
> > > consumerA {
> > >    pinctrl-0 = <&cam>;
> > > }
> > >
> > > With this model above, there are no cycles anymore.
> >
> > The cycles are gone because you skipped the problematic case in your
> > example.
> >
> > Replace consumerA in your example with the I2C node providing access to
> > the PMIC which provides &vcc3v3_pmu and then you have the cycles back.
> 
> When you are talking about the I2C node that's the bus master for the
> PMIC providing the supply, wouldn't it be dependent on "i2c0_xfer"?
> And not "cam"?
> 
> Otherwise there's a cyclic functional dependency in hardware that can
> never be met? Because in that case, your changes would end up
> deferring the I2C device probe too.

Yes, that's exactly the problem. There is a functional dependency in
hardware. This can only be resolved by assuming the hardware is already
correctly configured to access the PMIC.

> 
> I'm basically asking to split out the pins that need IO domain to work
> into a new subnode "pinctrl-io" of the main "pinctrl" device node.
> 
> > The I2C master device needs the IO domain which needs a regulator
> > provided by a client on the very same I2C master. The cycles are
> > actually there in hardware, you can't define them away ;)
> 
> Right, there can be a cyclic connection dependency in hardware and you
> can't define them away. But clearly the I2C master doesn't need the IO
> domain to work for the I2C to be initialized, right?

No, not right. The I2C master indeed does need the IO domain to be
correctly configured and the IO domain can only be configured correctly
when we know the voltage the PMIC supplies to the IO domain.

> Otherwise, how
> can the I2C hardware be initialized? It doesn't matter what OS we
> have, that hardware can't work. So, what am I missing? We are clearly
> not on the same page on some details.

This works by configuring the IO domain with static values in the
bootloader which knows the reset default PMIC voltage.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |