Messages in this thread | | | Date | Fri, 15 Sep 2023 18:24:25 +0100 | Subject | Re: [PATCH 1/3] pinctrl: rockchip: add support for io-domain dependency | From | Robin Murphy <> |
| |
On 15/09/2023 5:38 pm, Quentin Schulz wrote: > 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@pengutronix.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@linaro.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. >>> > > 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?
This is why I think it makes the most sense to describe the initial I/O domain voltage as a property of the I/O domain, since that is most truthful to what is actually needed to initialise the hardware. Ideally we could then just use that initial configuration to probe successfully, and use a notifier to pick up the regulator if and when it does appear later (on the basis that the voltage should not be able to change *without* one). And otherwise if an initial voltage isn't specified then we can assume it's OK to wait for the regulator and query it as normal.
> I'm also wondering how this would play out if the PMIC isn't supplying > power to the IO domain the bus controller to which it's connected... but > I guess that's a HW design issue :)
Hopefully that would just be a sensible non-cyclic design that avoids this problem altogether. However there's nothing special about PMICs, this could equally apply if the I/O domain was supplied by a GPIO regulator or PWM regulator which used one of its own pins; the fundamental problem to solve is being able to determine the correct initial voltage setting for an I/O domain without having to perform any I/O via that domain itself. That is the physical dependency cycle which can exist here, regardless of how we address any other software dependencies between drivers within Linux.
Thanks, Robin.
> > It's Friday evening here so hopefully my brain wasn't already on weekend > mode and I could convey properly everything I had in mind. > > Cheers, > Quentin > > _______________________________________________ > Linux-rockchip mailing list > Linux-rockchip@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-rockchip
| |