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 = <®_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?
