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: <CAGETcx8rO=aykjb6=5k0wpOyscqokNwSL6w-AHnodY7pNXyzGQ@mail.gmail.com>
Date:   Wed, 13 Sep 2023 13:48:12 -0700
From:   Saravana Kannan <saravanak@...gle.com>
To:     Sascha Hauer <s.hauer@...gutronix.de>
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 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?

Is there something that prevents us from doing that?

> >
> > 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.

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?

-Saravana