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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20230921135756.GT637806@pengutronix.de>
Date:   Thu, 21 Sep 2023 15:57:56 +0200
From:   Sascha Hauer <s.hauer@...gutronix.de>
To:     Saravana Kannan <saravanak@...gle.com>
Cc:     Chen-Yu Tsai <wens@...nel.org>,
        Linus Walleij <linus.walleij@...aro.org>,
        linux-rockchip@...ts.infradead.org,
        Heiko Stuebner <heiko@...ech.de>,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org, linux-gpio@...r.kernel.org,
        kernel@...gutronix.de,
        Quentin Schulz <quentin.schulz@...obroma-systems.com>,
        Michael Riesch <michael.riesch@...fvision.net>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>
Subject: Re: [PATCH 1/3] pinctrl: rockchip: add support for io-domain
 dependency

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@...gutronix.de> 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@...gutronix.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@...aro.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.
> > >
> > > 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.

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

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 |