[RFC PATCH 0/1] Making Rockchip IO domains dependency from other devices explicit

[RFC PATCH 0/1] Making Rockchip IO domains dependency from other devices explicit

[Quentin Schulz]

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(+)

-- 
2.37.1

[RFC PATCH 1/1] pinctrl: rockchip: add support for per-pinmux io-domain dependency

[Quentin Schulz]

From: Quentin Schulz <quentin.schulz@theobroma-systems.com>

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.

[1] drivers/soc/rockchip/io-domain.c
[2] Documentation/devicetree/bindings/power/rockchip-io-domain.yaml

Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
---
 drivers/pinctrl/pinctrl-rockchip.c | 19 +++++++++++++++++++
 drivers/pinctrl/pinctrl-rockchip.h |  1 +
 2 files changed, 20 insertions(+)

diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c
index 32e41395fc76..c3c2801237b5 100644
--- a/drivers/pinctrl/pinctrl-rockchip.c
+++ b/drivers/pinctrl/pinctrl-rockchip.c
@@ -24,6 +24,8 @@
 #include <linux/of_address.h>
 #include <linux/of_device.h>
 #include <linux/of_irq.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
 #include <linux/pinctrl/machine.h>
 #include <linux/pinctrl/pinconf.h>
 #include <linux/pinctrl/pinctrl.h>
@@ -2370,6 +2372,12 @@ static int rockchip_pmx_set(struct pinctrl_dev *pctldev, unsigned selector,
 	dev_dbg(dev, "enable function %s group %s\n",
 		info->functions[selector].name, info->groups[group].name);
 
+	if (info->groups[group].io_domain &&
+	    !platform_get_drvdata(info->groups[group].io_domain)) {
+		dev_err(info->dev, "IO domain device is required but not probed yet, deferring...");
+		return -EPROBE_DEFER;
+	}
+
 	/*
 	 * for each pin in the pin group selected, program the corresponding
 	 * pin function number in the config register.
@@ -2663,6 +2671,7 @@ static int rockchip_pinctrl_parse_groups(struct device_node *np,
 {
 	struct device *dev = info->dev;
 	struct rockchip_pin_bank *bank;
+	struct device_node *node;
 	int size;
 	const __be32 *list;
 	int num;
@@ -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;
+		}
+	}
+
 	grp->npins = size / 4;
 
 	grp->pins = devm_kcalloc(dev, grp->npins, sizeof(*grp->pins), GFP_KERNEL);
diff --git a/drivers/pinctrl/pinctrl-rockchip.h b/drivers/pinctrl/pinctrl-rockchip.h
index ec46f8815ac9..56bc008eb7df 100644
--- a/drivers/pinctrl/pinctrl-rockchip.h
+++ b/drivers/pinctrl/pinctrl-rockchip.h
@@ -434,6 +434,7 @@ struct rockchip_pin_group {
 	unsigned int			npins;
 	unsigned int			*pins;
 	struct rockchip_pin_config	*data;
+	struct platform_device		*io_domain;
 };
 
 /**
-- 
2.37.1

Re: [RFC PATCH 1/1] pinctrl: rockchip: add support for per-pinmux io-domain dependency

[Michael Riesch]

Hi Quentin,

Thank you for your efforts! This will solve several issues that are
bound to pop up if a board deviates from the Rockchip reference design.
It seems this does not happen very often, though, which would explain
the lack of responses to your initial query... :-/

A few comments inline:

On 8/2/22 11:52, Quentin Schulz wrote:
> From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> 
> 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).

Just for the sake of completeness: 2.5V is possibly as well (at least on
RK356x), right?

> There already exists an IO domain driver for Rockchip SoCs[1]. This
> driver allows to explicit the relationship between the external power

...allows to model explicitly...?

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

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>;?

If not, at which place are the rockchip,iodomains properties inserted?

> [1] drivers/soc/rockchip/io-domain.c
> [2] Documentation/devicetree/bindings/power/rockchip-io-domain.yaml
> 
> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> ---
>  drivers/pinctrl/pinctrl-rockchip.c | 19 +++++++++++++++++++
>  drivers/pinctrl/pinctrl-rockchip.h |  1 +
>  2 files changed, 20 insertions(+)
> 
> diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c
> index 32e41395fc76..c3c2801237b5 100644
> --- a/drivers/pinctrl/pinctrl-rockchip.c
> +++ b/drivers/pinctrl/pinctrl-rockchip.c
> @@ -24,6 +24,8 @@
>  #include <linux/of_address.h>
>  #include <linux/of_device.h>
>  #include <linux/of_irq.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
>  #include <linux/pinctrl/machine.h>
>  #include <linux/pinctrl/pinconf.h>
>  #include <linux/pinctrl/pinctrl.h>
> @@ -2370,6 +2372,12 @@ static int rockchip_pmx_set(struct pinctrl_dev *pctldev, unsigned selector,
>  	dev_dbg(dev, "enable function %s group %s\n",
>  		info->functions[selector].name, info->groups[group].name);
>  
> +	if (info->groups[group].io_domain &&
> +	    !platform_get_drvdata(info->groups[group].io_domain)) {
> +		dev_err(info->dev, "IO domain device is required but not probed yet, deferring...");

Probably this has been left in there for debugging, but should be
removed to avoid spamming dmesg. IIUC this condition could occur several
times.

> +		return -EPROBE_DEFER;
> +	}
> +
>  	/*
>  	 * for each pin in the pin group selected, program the corresponding
>  	 * pin function number in the config register.
> @@ -2663,6 +2671,7 @@ static int rockchip_pinctrl_parse_groups(struct device_node *np,
>  {
>  	struct device *dev = info->dev;
>  	struct rockchip_pin_bank *bank;
> +	struct device_node *node;
>  	int size;
>  	const __be32 *list;
>  	int num;
> @@ -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?

> +		}
> +	}
> +
>  	grp->npins = size / 4;
>  
>  	grp->pins = devm_kcalloc(dev, grp->npins, sizeof(*grp->pins), GFP_KERNEL);
> diff --git a/drivers/pinctrl/pinctrl-rockchip.h b/drivers/pinctrl/pinctrl-rockchip.h
> index ec46f8815ac9..56bc008eb7df 100644
> --- a/drivers/pinctrl/pinctrl-rockchip.h
> +++ b/drivers/pinctrl/pinctrl-rockchip.h
> @@ -434,6 +434,7 @@ struct rockchip_pin_group {
>  	unsigned int			npins;
>  	unsigned int			*pins;
>  	struct rockchip_pin_config	*data;
> +	struct platform_device		*io_domain;
>  };
>  
>  /**

Looking forward to the v1 of this series, which I'd be happy to test.

Best regards,
Michael

Re: [RFC PATCH 1/1] pinctrl: rockchip: add support for per-pinmux io-domain dependency

[Quentin Schulz]

Hi Michael,

On 8/11/22 09:52, Michael Riesch wrote:
> Hi Quentin,
> 
> Thank you for your efforts! This will solve several issues that are
> bound to pop up if a board deviates from the Rockchip reference design.

Actually, most of the devices could work by chance. If you have any IO 
domain running at anything but the default value (3V3 on PX30 and 
RK3399), it won't work until configured by the IO domain driver.

If the IO domain driver is probed before the device requiring it is 
probed, it'll work flawlessly.

Also, we actually hit this issue because the camera driver (ov5675.c) is 
doing an i2c transfer in its probe function. I assume devices where no 
interaction with the device is required during probe (e.g. no 
I2C/SPI/whatever transfer or clock enabling) of the driver, will also 
work just fine if their subsystem don't call callbacks before the IO 
domain device is probed.

So, I think it mostly work by luck today.

> It seems this does not happen very often, though, which would explain
> the lack of responses to your initial query... :-/
> 

Might also be thanks to "custom" bootloaders (our upstream U-Boot does 
it already for another IO domain) setting this up before Linux boots.

> A few comments inline:
> 
> On 8/2/22 11:52, Quentin Schulz wrote:
>> From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>>
>> 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).
> 
> Just for the sake of completeness: 2.5V is possibly as well (at least on
> RK356x), right?
> 

If you say so :) This patch has no knowledge of it and does not require 
it, it just makes sure it probes the IO domain devices before any device 
use the pins.

>> There already exists an IO domain driver for Rockchip SoCs[1]. This
>> driver allows to explicit the relationship between the external power
> 
> ...allows to model explicitly...?
> 
>> 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.
> 
> 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.

The RFC mail contains a bit more info about the pitfalls and what's 
needed to use it.

> If not, at which place are the rockchip,iodomains properties inserted?
> 

In your board dts(i) :/

This would probably be what's required for PCIe to work on our Puma 
Haikou (I tested this patchset for a camera extension board, which isn't 
upstreamed yet, nor mass-produced):

diff --git a/arch/arm64/boot/dts/rockchip/rk3399-puma-haikou.dts 
b/arch/arm64/boot/dts/rockchip/rk3399-puma-haikou.dts
index 04c752f49be98..21d2aa46d9553 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399-puma-haikou.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3399-puma-haikou.dts
@@ -160,7 +160,7 @@ &pcie0 {
  	ep-gpios = <&gpio4 RK_PC6 GPIO_ACTIVE_HIGH>;
  	num-lanes = <4>;
  	pinctrl-names = "default";
-	pinctrl-0 = <&pcie_clkreqn_cpm>;
+	pinctrl-0 = <&pcie_clkreqn_cpm>, <&pcie_ep_gpio>;
  	status = "okay";
  };

@@ -189,6 +189,14 @@ sd_card_led_pin: sd-card-led-pin {
  		};
  	};

+	pcie {
+		pcie_ep_gpio: pci-ep-gpio {
+			rockchip,pins =
+				<4 RK_PC6 RK_FUNC_GPIO &pcfg_pull_none>;
+			rockchip,io-domains = <&io_domains>;
+		};
+	};
+
  	usb2 {
  		otg_vbus_drv: otg-vbus-drv {
  			rockchip,pins =


>> [1] drivers/soc/rockchip/io-domain.c
>> [2] Documentation/devicetree/bindings/power/rockchip-io-domain.yaml
>>
>> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>> ---
>>   drivers/pinctrl/pinctrl-rockchip.c | 19 +++++++++++++++++++
>>   drivers/pinctrl/pinctrl-rockchip.h |  1 +
>>   2 files changed, 20 insertions(+)
>>
>> diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c
>> index 32e41395fc76..c3c2801237b5 100644
>> --- a/drivers/pinctrl/pinctrl-rockchip.c
>> +++ b/drivers/pinctrl/pinctrl-rockchip.c
>> @@ -24,6 +24,8 @@
>>   #include <linux/of_address.h>
>>   #include <linux/of_device.h>
>>   #include <linux/of_irq.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/platform_device.h>
>>   #include <linux/pinctrl/machine.h>
>>   #include <linux/pinctrl/pinconf.h>
>>   #include <linux/pinctrl/pinctrl.h>
>> @@ -2370,6 +2372,12 @@ static int rockchip_pmx_set(struct pinctrl_dev *pctldev, unsigned selector,
>>   	dev_dbg(dev, "enable function %s group %s\n",
>>   		info->functions[selector].name, info->groups[group].name);
>>   
>> +	if (info->groups[group].io_domain &&
>> +	    !platform_get_drvdata(info->groups[group].io_domain)) {
>> +		dev_err(info->dev, "IO domain device is required but not probed yet, deferring...");
> 
> Probably this has been left in there for debugging, but should be
> removed to avoid spamming dmesg. IIUC this condition could occur several
> times.
> 

Considering that the deferred probing mechanism is to retry the 
to-be-deferred device after all other devices have been tried, it is 
very likely to not spam dmesg.

We could remove it though, no strong opinion on this.

>> +		return -EPROBE_DEFER;
>> +	}
>> +
>>   	/*
>>   	 * for each pin in the pin group selected, program the corresponding
>>   	 * pin function number in the config register.
>> @@ -2663,6 +2671,7 @@ static int rockchip_pinctrl_parse_groups(struct device_node *np,
>>   {
>>   	struct device *dev = info->dev;
>>   	struct rockchip_pin_bank *bank;
>> +	struct device_node *node;
>>   	int size;
>>   	const __be32 *list;
>>   	int num;
>> @@ -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.

>> +		}
>> +	}
>> +
>>   	grp->npins = size / 4;
>>   
>>   	grp->pins = devm_kcalloc(dev, grp->npins, sizeof(*grp->pins), GFP_KERNEL);
>> diff --git a/drivers/pinctrl/pinctrl-rockchip.h b/drivers/pinctrl/pinctrl-rockchip.h
>> index ec46f8815ac9..56bc008eb7df 100644
>> --- a/drivers/pinctrl/pinctrl-rockchip.h
>> +++ b/drivers/pinctrl/pinctrl-rockchip.h
>> @@ -434,6 +434,7 @@ struct rockchip_pin_group {
>>   	unsigned int			npins;
>>   	unsigned int			*pins;
>>   	struct rockchip_pin_config	*data;
>> +	struct platform_device		*io_domain;
>>   };
>>   
>>   /**
> 
> Looking forward to the v1 of this series, which I'd be happy to test.
> 

Nothing prevents you from trying it out right now :)

I might be tempted to resend it in a few weeks as v1 to force a 
discussion since this isn't really getting the attention I'd like it to 
receive (and we really need it, though we can work around all this and 
do it in the bootloader). Well.. it's also summer and people usually 
take long holidays or are busy with life :)

Thanks for the review, looking forward to having people use this.

Cheers,
Quentin

Re: [RFC PATCH 1/1] pinctrl: rockchip: add support for per-pinmux io-domain dependency

[Heiko Stübner]

Hi,

Am Donnerstag, 11. August 2022, 10:45:07 CEST schrieb Quentin Schulz:
> Hi Michael,
> 
> On 8/11/22 09:52, Michael Riesch wrote:
> > Hi Quentin,
> > 
> > Thank you for your efforts! This will solve several issues that are
> > bound to pop up if a board deviates from the Rockchip reference design.

I find this approach quite nice. io-domains in their core specify pin
voltages, so having the tie in the pinctrl space makes a lot of sense.


> >> There already exists an IO domain driver for Rockchip SoCs[1]. This
> >> driver allows to explicit the relationship between the external power
> > 
> > ...allows to model explicitly...?
> > 
> >> 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.
> > 
> > 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.


> >> diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c
> >> index 32e41395fc76..c3c2801237b5 100644
> >> --- a/drivers/pinctrl/pinctrl-rockchip.c
> >> +++ b/drivers/pinctrl/pinctrl-rockchip.c
> >> @@ -24,6 +24,8 @@
> >>   #include <linux/of_address.h>
> >>   #include <linux/of_device.h>
> >>   #include <linux/of_irq.h>
> >> +#include <linux/of_platform.h>
> >> +#include <linux/platform_device.h>
> >>   #include <linux/pinctrl/machine.h>
> >>   #include <linux/pinctrl/pinconf.h>
> >>   #include <linux/pinctrl/pinctrl.h>
> >> @@ -2370,6 +2372,12 @@ static int rockchip_pmx_set(struct pinctrl_dev *pctldev, unsigned selector,
> >>   	dev_dbg(dev, "enable function %s group %s\n",
> >>   		info->functions[selector].name, info->groups[group].name);
> >>   
> >> +	if (info->groups[group].io_domain &&
> >> +	    !platform_get_drvdata(info->groups[group].io_domain)) {
> >> +		dev_err(info->dev, "IO domain device is required but not probed yet, deferring...");
> > 
> > Probably this has been left in there for debugging, but should be
> > removed to avoid spamming dmesg. IIUC this condition could occur several
> > times.
> > 
> 
> Considering that the deferred probing mechanism is to retry the 
> to-be-deferred device after all other devices have been tried, it is 
> very likely to not spam dmesg.
> 
> We could remove it though, no strong opinion on this.

just move it to use dev_dbg and everybody is happy :-) .


> >> @@ -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 :-) .



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.

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.


Heiko

Re: [RFC PATCH 1/1] pinctrl: rockchip: add support for per-pinmux io-domain dependency

[Quentin Schulz]

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

Re: [RFC PATCH 0/1] Making Rockchip IO domains dependency from other devices explicit

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

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

Re: [RFC PATCH 0/1] Making Rockchip IO domains dependency from other devices explicit

[Heiko Stübner]

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
> 

Re: [RFC PATCH 1/1] pinctrl: rockchip: add support for per-pinmux io-domain dependency

[Sascha Hauer]

Hi Quentin,

On Tue, Aug 02, 2022 at 11:52:52AM +0200, Quentin Schulz wrote:
> From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> 
> 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.

I don't know the status of this patch, but I haven't found anything
newer, so please point me to newer patches if the discussion has
continued somewhere else. Anyway, here are some thoughts about this
patch

I think the general approach is fine but could be improved. Right now we
have one io-domain device with several supplies. That means once one
consumer needs an io-domain, the supplies for all domains need to be
probed beforehand.  We could relax this requirement by adding a subnode
for each domain, so instead of doing this:

pmu_io_domains: io-domains {
	compatible = "rockchip,rk3568-pmu-io-voltage-domain";
	pmuio1-supply = <&vcc3v3_pmu>;
	pmuio2-supply = <&vcc3v3_pmu>;
	vccio1-supply = <&vccio_acodec>;
	vccio2-supply = <&vcc_1v8>;
	vccio3-supply = <&vccio_sd>;
	vccio4-supply = <&vcc_1v8>;
	vccio5-supply = <&vcc_3v3>;
	vccio6-supply = <&vcc_1v8>;
	vccio7-supply = <&vcc_3v3>;
};

We could do this:

pmu_io_domains: io-domains {
	compatible = "rockchip,rk3568-pmu-io-voltage-domain";

	io_domain_pmuio1: io-domain@ {
		reg = <0>;
		supply = <&vcc3v3_pmu>;
	};

	io_domain_pmuio2: io-domain@1 {
		reg = <1>;
		supply = <&vcc3v3_pmu>;
	};

	...
};

This way we could put a driver on each io-domain. When another device
needs an io-domain we no longer have to wait for all regulators to
appear, but only for the regulator that actually supplies that domain.

With that we could specify the io-domain dependencies at dtsi or core
level. A board would only have to make sure that the io-domain that is
needed to access the PMIC does not itself need a supply from the very
same PMIC to not get into circular dependencies. The supplies for the
io-domains are specified at board level anyway, so all that a board
would have to do is to skip (or replace with a fixed-regulator) the
supply for the io-domain that provides access to the I2C port the PMIC
is on. That is not too bad I guess as the regulator that supplies the
io-domain to access the PMIC needs to be always-on anyway. In the end if
we would turn that regulator off, we would no longer be able to turn it
on again.

One thing about putting the "rockchip,io-domains" property into the
pingroups. We would have to put this property into each and every
existing pingroup in all dts[i] files and new files would have to be
reviewed in this regard as well. The pinctrl driver already has
knowledge about all pins, so I think that would be the natural place to
also add the knowledge about which io-domain a pin is in. With that in
place we would get the knowledge if a io-domain is in use and could
disable unused io-domains. I am afraid that the "rockchip,io-domains"
property would only be added in places where it actually hurts someone.

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 |

Re: [RFC PATCH 1/1] pinctrl: rockchip: add support for per-pinmux io-domain dependency

[Quentin Schulz]

Hi Sascha,

On 2/15/23 11:23, Sascha Hauer wrote:
> 
> Hi Quentin,
> 
> On Tue, Aug 02, 2022 at 11:52:52AM +0200, Quentin Schulz wrote:
>> From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>>
>> 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.
> 
> I don't know the status of this patch, but I haven't found anything
> newer, so please point me to newer patches if the discussion has
> continued somewhere else. Anyway, here are some thoughts about this
> patch

No new version, a bit drowning in work but we are dependent on this 
patchset for an adapter board we want to upstream so I'll have to get 
back to it in the next few weeks/months.

> 
> I think the general approach is fine but could be improved. Right now we
> have one io-domain device with several supplies. That means once one
> consumer needs an io-domain, the supplies for all domains need to be
> probed beforehand.  We could relax this requirement by adding a subnode
> for each domain, so instead of doing this:
> 
> pmu_io_domains: io-domains {
> 	compatible = "rockchip,rk3568-pmu-io-voltage-domain";
> 	pmuio1-supply = <&vcc3v3_pmu>;
> 	pmuio2-supply = <&vcc3v3_pmu>;
> 	vccio1-supply = <&vccio_acodec>;
> 	vccio2-supply = <&vcc_1v8>;
> 	vccio3-supply = <&vccio_sd>;
> 	vccio4-supply = <&vcc_1v8>;
> 	vccio5-supply = <&vcc_3v3>;
> 	vccio6-supply = <&vcc_1v8>;
> 	vccio7-supply = <&vcc_3v3>;
> };
> 
> We could do this:
> 
> pmu_io_domains: io-domains {
> 	compatible = "rockchip,rk3568-pmu-io-voltage-domain";
> 
> 	io_domain_pmuio1: io-domain@ {
> 		reg = <0>;
> 		supply = <&vcc3v3_pmu>;
> 	};
> 
> 	io_domain_pmuio2: io-domain@1 {
> 		reg = <1>;
> 		supply = <&vcc3v3_pmu>;
> 	};
> 
> 	...
> };
> 
> This way we could put a driver on each io-domain. When another device
> needs an io-domain we no longer have to wait for all regulators to
> appear, but only for the regulator that actually supplies that domain.
> 

Mmm, that's something I indeed hadn't thought about. We'd need to handle 
pmu_io_domains probing (and making available) **some** io-domains 
devices and not unregister them if other io-domains devices aren't able 
to probe (e.g. EPROBE_DEFER or invalid configuration for some reason; 
missing supply in board DTSI). Nothing impossible, haven't developed 
such a thing yet (I guess it's just kind of a bus mechanism then).

The other issue I'm thinking about ATM is whether we should support 
upward compatibility (i.e. old io-domain driver with newer dts) and 
backward compatibility (i.e. new io-domain driver with older dts). This 
may make things a lot more complex. This is a maintainer choice though.

> With that we could specify the io-domain dependencies at dtsi or core
> level. A board would only have to make sure that the io-domain that is
> needed to access the PMIC does not itself need a supply from the very
> same PMIC to not get into circular dependencies. The supplies for the
> io-domains are specified at board level anyway, so all that a board
> would have to do is to skip (or replace with a fixed-regulator) the
> supply for the io-domain that provides access to the I2C port the PMIC
> is on. That is not too bad I guess as the regulator that supplies the
> io-domain to access the PMIC needs to be always-on anyway. In the end if
> we would turn that regulator off, we would no longer be able to turn it
> on again.
> 

Correct.

> One thing about putting the "rockchip,io-domains" property into the
> pingroups. We would have to put this property into each and every
> existing pingroup in all dts[i] files and new files would have to be
> reviewed in this regard as well. The pinctrl driver already has
> knowledge about all pins, so I think that would be the natural place to
> also add the knowledge about which io-domain a pin is in. With that in
> place we would get the knowledge if a io-domain is in use and could
> disable unused io-domains. I am afraid that the "rockchip,io-domains"
> property would only be added in places where it actually hurts someone.
> 

The Device Tree is here to explicit the dependencies between HW blocks, 
which is what io-domains and pinctrl devices are and rockchip,io-domains 
the relations between the both of them.

While we know which pin is assigned to which io-domain because it's 
fixed in the silicon, this information is linking two different HW 
blocks and linux drivers (and linux devices actually). I'm wondering how 
exactly you think we should get this link in code without reading the 
Device Tree? Because we'll have to traverse the list of io-domains 
devices and find a way to identify them. This very much seems like 
something DT wanted to avoid? Can you tell me what I'm missing from the 
big picture?

Thanks for the feedback,
Cheers,
Quentin

Re: [RFC PATCH 1/1] pinctrl: rockchip: add support for per-pinmux io-domain dependency

[Sascha Hauer]

On Tue, Feb 21, 2023 at 01:14:28PM +0100, Quentin Schulz wrote:
> Hi Sascha,
> 
> On 2/15/23 11:23, Sascha Hauer wrote:
> > 
> > Hi Quentin,
> > 
> > On Tue, Aug 02, 2022 at 11:52:52AM +0200, Quentin Schulz wrote:
> > > From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> > > 
> > > 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.
> > 
> > I don't know the status of this patch, but I haven't found anything
> > newer, so please point me to newer patches if the discussion has
> > continued somewhere else. Anyway, here are some thoughts about this
> > patch
> 
> No new version, a bit drowning in work but we are dependent on this patchset
> for an adapter board we want to upstream so I'll have to get back to it in
> the next few weeks/months.
> 
> > 
> > I think the general approach is fine but could be improved. Right now we
> > have one io-domain device with several supplies. That means once one
> > consumer needs an io-domain, the supplies for all domains need to be
> > probed beforehand.  We could relax this requirement by adding a subnode
> > for each domain, so instead of doing this:
> > 
> > pmu_io_domains: io-domains {
> > 	compatible = "rockchip,rk3568-pmu-io-voltage-domain";
> > 	pmuio1-supply = <&vcc3v3_pmu>;
> > 	pmuio2-supply = <&vcc3v3_pmu>;
> > 	vccio1-supply = <&vccio_acodec>;
> > 	vccio2-supply = <&vcc_1v8>;
> > 	vccio3-supply = <&vccio_sd>;
> > 	vccio4-supply = <&vcc_1v8>;
> > 	vccio5-supply = <&vcc_3v3>;
> > 	vccio6-supply = <&vcc_1v8>;
> > 	vccio7-supply = <&vcc_3v3>;
> > };
> > 
> > We could do this:
> > 
> > pmu_io_domains: io-domains {
> > 	compatible = "rockchip,rk3568-pmu-io-voltage-domain";
> > 
> > 	io_domain_pmuio1: io-domain@ {
> > 		reg = <0>;
> > 		supply = <&vcc3v3_pmu>;
> > 	};
> > 
> > 	io_domain_pmuio2: io-domain@1 {
> > 		reg = <1>;
> > 		supply = <&vcc3v3_pmu>;
> > 	};
> > 
> > 	...
> > };
> > 
> > This way we could put a driver on each io-domain. When another device
> > needs an io-domain we no longer have to wait for all regulators to
> > appear, but only for the regulator that actually supplies that domain.
> > 
> 
> Mmm, that's something I indeed hadn't thought about. We'd need to handle
> pmu_io_domains probing (and making available) **some** io-domains devices
> and not unregister them if other io-domains devices aren't able to probe
> (e.g. EPROBE_DEFER or invalid configuration for some reason; missing supply
> in board DTSI). Nothing impossible, haven't developed such a thing yet (I
> guess it's just kind of a bus mechanism then).
> 
> The other issue I'm thinking about ATM is whether we should support upward
> compatibility (i.e. old io-domain driver with newer dts) and backward
> compatibility (i.e. new io-domain driver with older dts). This may make
> things a lot more complex. This is a maintainer choice though.

New io-domain driver with older dts is easy enough to handle as the new
io-domain driver could fall back to old behaviour when the io-domains
node doesn't have subnodes.
Old io-domain driver with new dts doesn't work though, but I think is
also less important.

> 
> > With that we could specify the io-domain dependencies at dtsi or core
> > level. A board would only have to make sure that the io-domain that is
> > needed to access the PMIC does not itself need a supply from the very
> > same PMIC to not get into circular dependencies. The supplies for the
> > io-domains are specified at board level anyway, so all that a board
> > would have to do is to skip (or replace with a fixed-regulator) the
> > supply for the io-domain that provides access to the I2C port the PMIC
> > is on. That is not too bad I guess as the regulator that supplies the
> > io-domain to access the PMIC needs to be always-on anyway. In the end if
> > we would turn that regulator off, we would no longer be able to turn it
> > on again.
> > 
> 
> Correct.
> 
> > One thing about putting the "rockchip,io-domains" property into the
> > pingroups. We would have to put this property into each and every
> > existing pingroup in all dts[i] files and new files would have to be
> > reviewed in this regard as well. The pinctrl driver already has
> > knowledge about all pins, so I think that would be the natural place to
> > also add the knowledge about which io-domain a pin is in. With that in
> > place we would get the knowledge if a io-domain is in use and could
> > disable unused io-domains. I am afraid that the "rockchip,io-domains"
> > property would only be added in places where it actually hurts someone.
> > 
> 
> The Device Tree is here to explicit the dependencies between HW blocks,
> which is what io-domains and pinctrl devices are and rockchip,io-domains the
> relations between the both of them.
> 
> While we know which pin is assigned to which io-domain because it's fixed in
> the silicon, this information is linking two different HW blocks and linux
> drivers (and linux devices actually). I'm wondering how exactly you think we
> should get this link in code without reading the Device Tree? Because we'll
> have to traverse the list of io-domains devices and find a way to identify
> them. This very much seems like something DT wanted to avoid? Can you tell
> me what I'm missing from the big picture?

We could add a rockchip,io-domains property to the rockchip,rk3xxx-pinctrl
node pointing to the io-domain driver (the toplevel driver, not the
subnodes), like:

	pinctrl: pinctrl {
		compatible = "rockchip,rk3568-pinctrl";
		rockchip,io-domains = <&pmu_io_domains>;
		...
	};

With that the pinctrl driver could call into the io-domain driver with
something like:

	rockchip_iodomain_get(struct device_node *io_domain_toplevel_node, int num);

'num' would map to the subnode providing the domain. Alternatively this
could be a const char *name instead. rockchip_iodomain_get() would then
either return some cookie or -EPROBE_DEFER when the regulator is not yet
available.

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 |