| Message ID | 20220802095252.2486591-1-foss+kernel@0leil.net |
|---|---|
| Headers | show |
| Series | Making Rockchip IO domains dependency from other devices explicit | expand |
Hi Heiko, On 8/11/22 11:26, Heiko Stübner wrote: [...] >>>> In order to make this dependency transparent to the consumer of those >>>> pins and not add Rockchip-specific code to third party drivers (a camera >>>> driver in our case), it is hooked into the pinctrl driver which is >>>> Rockchip-specific obviously. >>> >>> This approach seems reasonable. But just for my understanding: Does this >>> mean we need to edit e.g. rk3568-pinctrl.dtsi, iterate over all entries, >>> and add rockchip,iodomains = <&corresponding_io_domain>;? >>> >> >> That would have been my hope yes, but it is not possible for one of the >> boards we have based on PX30. >> >> All pinmux listed in the px30.dtsi today belong to an IO domain. This >> includes the I2C pins for the bus on which the PMIC is. >> Adding the rockchip,io-domains on each pinctrl will create the following >> circular dependency: >> pinctrl depends on the io-domain device which depends on >> regulators from a PMIC on i2c which requires the i2c bus pins to be >> muxed from the pinctrl >> >> Since the PMIC powering the IO domains can virtually be on any I2C bus, >> we cannot add it to the main SoC.dtsi, it'll need to be added per board >> sadly. > > though you could also add the main props to the dtsi and use a per-board > /delete-property/ to free up the pmic-i2c, same result but less duplicate > dt additions and less clutter. > That is also a possibility, yes. However, this means that it'll make the bring-up of a new board slightly more complex since it'll just not boot until you have this /delete-property/ in your board dts. Remember that the current implementation basically forbids *all* pinmux (well the ones with rockchip,io-domains.. but that would be all of them in most cases) to be used until io-domains is probed, which very likely involves boot media such as eMMC which require some pinmux to be done. (I had this issue on my device already, so not hypothetical). [...] >>>> @@ -2684,6 +2693,16 @@ static int rockchip_pinctrl_parse_groups(struct device_node *np, >>>> if (!size || size % 4) >>>> return dev_err_probe(dev, -EINVAL, "wrong pins number or pins and configs should be by 4\n"); >>>> >>>> + node = of_parse_phandle(np, "rockchip,io-domains", 0); >>>> + if (node) { >>>> + grp->io_domain = of_find_device_by_node(node); >>>> + of_node_put(node); >>>> + if (!grp->io_domain) { >>>> + dev_err(info->dev, "couldn't find IO domain device\n"); >>>> + return -ENODEV; >>> >>> Again just for my understanding: The property is optional in order to >>> provide compatibility with older device trees, right? >>> >> >> Of course (at least that's the intent). If it is omitted, >> of_parse_phandle will return NULL and we'll not be executing this part >> of the code. However, if one phandle is provided and the device does not >> actually exist (IIUC, the phandle points to a DT-valid node but the >> device pointed at by the phandle is either disabled or its driver is not >> built). That being said, I don't know how this would work with an IO >> domain driver built as a module. That would be a pretty dumb thing to do >> though. > > I think this should work even with io-domain "disabled" or as module > when slightly modified. > > I.e. for disabled nodes, no kernel-device should be created > (grp->io_domain will be NULL) and for a module the device itself is created > when the dt is parsed (of_populate...) and will just not have probed yet. > > Together with the comment farther above of having the io-domain link always > present we should get rid of the error condition though :-) . > It is not possible to have a rockchip,io-domains entry for all pinctrl, because at least a few needs to be removed, the ones related to the regulators used by the io-domain devices. But I guess you were talking about the check on grp->io_domain pointer? > > > Hmm, while going through this one thought was, do we want more verbosity > in the dt for this? > > I.e. with the current approach we'll have > > &io_domains { > status = "okay"; > > audio-supply = <&pp1800_audio>; > bt656-supply = <&pp1800_ap_io>; > gpio1830-supply = <&pp3000_ap>; > sdmmc-supply = <&ppvar_sd_card_io>; > }; > > and pinctrl entries linking to the core <&io_domains> node. This might bite > us down the road again in some form. > > Something like doing an optional updated binding like: > > &io_domains { > status = "okay"; > > audio-domain { > domain-supply = <&pp1800_audio>; > }; > bt656-domain { > domain-supply = <&pp1800_ap_io>; > }; > gpio1830-domain { > domain-supply = <&pp3000_ap>; > }; > sdmmc-domain { > domain-supply = <&ppvar_sd_card_io>; > }; > }; > > pcie { > pcie_ep_gpio: pci-ep-gpio { > rockchip,pins = > <4 RK_PC6 RK_FUNC_GPIO &pcfg_pull_none>; > rockchip,io-domains = <&gpio1830_domain>; > }; > }; > > > I.e. linking the pin-set to a definition of its actual io-domain, instead > of only the general io-domain node. Somewhat similar to power-domains. > I like this (well, not the "modifying existing bindings" part though). This could allow io-domain driver to be more of a bus kind and "probe" each subnode individually, allowing to break out of circular dependencies. E.g., I could have some supplies provided by fixed non-controllable always-on regulators, and some by some controllable PMIC. Though this wouldn't have helped for our design since the PMIC is providing the power to the IO domains pins of the i2c bus on which the PMIC is (so whatever we do, the HW representation will always include a theoretical circular loop). This would maybe allow us to mitigate the issue I talked about earlier with the need of removing some rockchip,io-domains to break this circular dependency. > The code itself could be the same as now (except needing to get the parent > of the linked node for the io-domains), but would leave us the option of > modifying code behaviour without touching the binding if needed down the > road. > Would I need to support forward compatibility of the DT? i.e. having rockchip,io-domains DT property work with the io_domains phandle AND sdmmc_domain phandle? Cheers, Quentin
On Tue, Aug 2, 2022 at 11:53 AM Quentin Schulz <foss+kernel@0leil.net> wrote: > Some background on IO domains on Rockchip: > > On some Rockchip SoCs, some SoC pins are split in what are called IO > domains. > > An IO domain is supplied power externally, by regulators from a PMIC for > example. This external power supply is then used by the IO domain as > "supply" for the IO pins if they are outputs. > > Each IO domain can configure which voltage the IO pins will be operating > on (1.8V or 3.3V). > > There already exists an IO domain driver for Rockchip SoCs[1]. This > driver allows to explicit the relationship between the external power > supplies and IO domains[2]. This makes sure the regulators are enabled > by the Linux kernel so the IO domains are supplied with power and > correctly configured as per the supplied voltage. > This driver is a regulator consumer and does not offer any other > interface for device dependency. What makes me confused about the patch is the relationship, if any, between this "IO domain" and generic power domains (genpd) that has been worked on for ~10 years. I am worried that we are reinventing the world. While my intuitive feeling is that genpd power domains are only on-chip and not considering off-chip pins, I am not so sure that it warrants its own abstraction and want to know whether this can be retrofit into genpd rather than inventing this? Documentation/devicetree/bindings/power/power-domain.yaml include/linux/pm_domain.h Yours, Linus Walleij
Hi Linus, Am Montag, 22. August 2022, 10:38:11 CEST schrieb Linus Walleij: > On Tue, Aug 2, 2022 at 11:53 AM Quentin Schulz <foss+kernel@0leil.net> wrote: > > > Some background on IO domains on Rockchip: > > > > On some Rockchip SoCs, some SoC pins are split in what are called IO > > domains. > > > > An IO domain is supplied power externally, by regulators from a PMIC for > > example. This external power supply is then used by the IO domain as > > "supply" for the IO pins if they are outputs. > > > > Each IO domain can configure which voltage the IO pins will be operating > > on (1.8V or 3.3V). > > > > There already exists an IO domain driver for Rockchip SoCs[1]. This > > driver allows to explicit the relationship between the external power > > supplies and IO domains[2]. This makes sure the regulators are enabled > > by the Linux kernel so the IO domains are supplied with power and > > correctly configured as per the supplied voltage. > > This driver is a regulator consumer and does not offer any other > > interface for device dependency. > > What makes me confused about the patch is the relationship, if any, > between this "IO domain" and generic power domains (genpd) that has > been worked on for ~10 years. > > I am worried that we are reinventing the world. In a nutshell, the Rockchip io-domains handle the voltages of specific pin-groups. I.e. mostly it is just switching between 1.8V and 3.3V . The voltage itself is always set in a (i2c-)regulator but there is a separate step necessary to tell the soc this information. 3.3 -> 1.8: set regulator to 1.8, tell io-domain "we're at 1.8 now" 1.8 -> 3.3: tell io-domain "3.3", set regulator to 3.3. There is supposedly a soc-health-issue if you set the regulator to 3.3 while the io-domain thinks it's at 1.8 . So the io-domain driver right now, just attaches to the regulator, catches the voltage-change events and sets the io-domain setting accordingly. What Quentin is trying to solve is a probe-dependency issue that can happen when stuff is built into the kernel, the regulator has probed the regulator using driver has probed, but the io.domain driver hasn't, as that also only attached to the regulator as described above. Heiko > While my intuitive feeling is that genpd power domains are only on-chip > and not considering off-chip pins, I am not so sure that it warrants > its own abstraction and want to know whether this can be retrofit into > genpd rather than inventing this? > > Documentation/devicetree/bindings/power/power-domain.yaml > include/linux/pm_domain.h > > Yours, > Linus Walleij >
From: Quentin Schulz <quentin.schulz@theobroma-systems.com> This is a follow-up to the mail sent almost two months ago asking for guidance: https://lore.kernel.org/lkml/778790a4-1239-e9d9-0549-6760a8792ceb@theobroma-systems.com/ This is what I could come up with but I'm not too happy about it so feel free to give some ideas or other possible implementations that would have less downsides than this one. Some background on IO domains on Rockchip: On some Rockchip SoCs, some SoC pins are split in what are called IO domains. An IO domain is supplied power externally, by regulators from a PMIC for example. This external power supply is then used by the IO domain as "supply" for the IO pins if they are outputs. Each IO domain can configure which voltage the IO pins will be operating on (1.8V or 3.3V). There already exists an IO domain driver for Rockchip SoCs[1]. This driver allows to explicit the relationship between the external power supplies and IO domains[2]. This makes sure the regulators are enabled by the Linux kernel so the IO domains are supplied with power and correctly configured as per the supplied voltage. This driver is a regulator consumer and does not offer any other interface for device dependency. However, IO pins belonging to an IO domain need to have this IO domain correctly configured before they are being used otherwise they do not operate correctly (in our case, a pin configured as output clock was oscillating between 0 and 150mV instead of the expected 1V8). In order to make this dependency transparent to the consumer of those pins and not add Rockchip-specific code to third party drivers (a camera driver in our case), it is hooked into the pinctrl driver which is Rockchip-specific obviously. Unfortunately, the dependency is not about the ultimate presence of the io-domain devices but more that prior to being able to use pins, the io-domain devices need to be probed. However of_find_device_by_node does not provide this information, hence the check whether the platform_device actually has its drvdata structure set (it defaults to NULL until it is filled by the driver during the probe of the device). Moreover, this dependency needs to be explicit on a pinmux level and postponed after the probe of the pinctrl driver because a circular dependency is observed otherwise with the following: pinctrl device depends on the io-domain device which depends on regulators from a PMIC on i2c which requires the i2c bus pins to be muxed from the pinctrl device. Instead, the pinmux dependency on IO domain is checked in set_mux callback and returns EPROBE_DEFER if the IO domain device hasn't probed yet. I wanted to add the appropriate rockchip,io-domains DT property to existing pinmux DT node. However, *all* of PX30's belong to an IO domain, including the i2c bus on which the PMIC supplying the power to the IO domain is on. Since the PMIC can be virtually on any i2c bus, a specific pinmux cannot be omitted or ignored, therefore none are added and it's up to the board maintainers to add them themselves. This is also assumed only necessary for IO domain that are configured differently by the bootloader or by register defaults (3V3 for RK3399/PX30) than their expected values in Linux and whose supplied power is fixed (e.g. not expected to be able to change from 3V3 to 1V8). The hope is to not have to handle the io domain configuration in the bootloader as it is currently done, c.f. https://elixir.bootlin.com/u-boot/latest/source/board/theobroma-systems/puma_rk3399/puma-rk3399.c#L28 (we'll need an additional IO domain configuration for a camera soon). However, this still relies on IO domains defaults or bootloader configuration to be able to omit some IO domain<->pinmux relationships to avoid circular dependencies. I don't know how well this RFC implementation would work with suspend/resume. [1] drivers/soc/rockchip/io-domain.c [2] Documentation/devicetree/bindings/power/rockchip-io-domain.yaml Cheers, Quentin Quentin Schulz (1): pinctrl: rockchip: add support for per-pinmux io-domain dependency drivers/pinctrl/pinctrl-rockchip.c | 19 +++++++++++++++++++ drivers/pinctrl/pinctrl-rockchip.h | 1 + 2 files changed, 20 insertions(+)