| 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
| ||
|
Message-ID: <CAGETcx-stUfkVmkwGhj7iBWfCRsY5uZ=CxJdX9pPY6OO6oGUhg@mail.gmail.com>
Date: Wed, 20 Sep 2023 15:00:28 -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 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.
pictrl doesn't depend on anything.
vcc3v3_pmu will depend on pinctrl (not shown in DT above).
pinctrl-io depends on pinctrl and vcc3v3_pmu.
consumerA depends on pinctrl-io.
So pinctrl probes first.
vcc3v3 will probe next.
pinctrl-io will probe now that the supply is ready.
consumerA will probe now that pinctrl-io is ready.
fw_devlink will enforce all these dependencies because it understands
pinctrl and -supply bindings.
-Saravana
>
> 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 = <®_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?
>
> 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.
>
> 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.
>
> 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 |
Powered by blists - more mailing lists