[PATCH 0/3] Make Rockchip IO domains dependency from other devices explicit

[PATCH 0/3] Make Rockchip IO domains dependency from other devices explicit

[Sascha Hauer]

This is a continuation of the patch posted by Quentin Schulz here [1]

This series aims to solve a problem with Rockchip IO domains. On many
Rockchip SoCs the pins are driven by external supplies normally
controlled by regulators of a PMIC. There are multiple voltages allowed
for the regulators; additionally the chosen voltage has to be programmed
into SoC registers. There already is a driver [2] handling setting these
registers. The driver works by registering a notifier on the regulators.
Whenever a regulator is about to change its voltage then the notifier will
program the IO domain registers suitably for the new voltage.

The problem is that there is no dependency between pins and the IO
domain driver which means that it can happen that a pin is used before
the IO domain driver has been probed. In that case the pin can end up
being non functional as neither the regulator has been configured
correctly nor the SoC registers have been adjusted to the regulators
voltage.

One way to ensure correct probing order is to defer probing of devices
in the pinctrl driver until the IO domain driver has been probed. We
can't do this for all devices though, as that would introduce a cyclic
dependency when for example the I2C port needed to access the PMIC for
the regulators is part of a IO domain itself.

This series solves these problems similarly to Quentins patch. With
Quentins patch we would have to add rockchip,io-domain properties for
all pin group nodes we wish to honor the IO domain dependency for. We
could put these properties into the board dts files which would mean
that we either only add the properties to nodes which actually byte us,
or that we would have to add the properties to all possible pin groups
except the ones needed to access the PMIC. We could also put these
properties into the dtsi files, but that would mean a board has to add a
/delete-property/ rockchip,io-domain to the pin groups needed to access
the PMIC to avoid circular dependencies.

The approach chosen here is slightly different. First of all this series
doesn't change the current behaviour without board specific dts changes.
To activate the IO domain dependency handling, a board has to add a
rockchip,io-domains property to the pinctrl node. When this property is
present all pins are assumed to need the IO domain driver. Pin groups
needed to access the PMIC can then be given a rockchip,io-domain-boot-on
property. When this property is given then the IO domain is assumed to
be correctly configured by the boot loader. It should be added to all
pin groups needed to access the PMIC to avoid cyclic dependencies. Patch
3/3 contains a usage example for the Radxa Rock-3a.

Sascha

[1] https://lore.kernel.org/lkml/20220802095252.2486591-1-foss+kernel@0leil.net/
[2] drivers/soc/rockchip/io-domain.c

Sascha Hauer (3):
  pinctrl: rockchip: add support for io-domain dependency
  dt-bindings: pinctrl: rockchip: Add io domain properties
  arm64: dts: rockchip: rock-3a: add io domain properties

 .../bindings/pinctrl/rockchip,pinctrl.yaml    | 13 +++-
 .../boot/dts/rockchip/rk3568-rock-3a.dts      | 11 ++++
 drivers/pinctrl/pinctrl-rockchip.c            | 64 +++++++++++++++++++
 drivers/pinctrl/pinctrl-rockchip.h            |  3 +
 4 files changed, 90 insertions(+), 1 deletion(-)

-- 
2.39.2

[PATCH 1/3] pinctrl: rockchip: add support for io-domain dependency

[Sascha Hauer]

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
configured correctly before they are being used otherwise they do not
operate correctly.

We currently do not have any knowledge about which pin is on which IO
domain, so we assume that all pins are on some IO domain and defer
probing of the pin consumers until the IO domain driver has been probed.
Some pins however are needed to access the regulators driving an IO
domain. Deferring probe for them as well would introduce a cyclic
dependency. To break out of this dependency a pin group can be supplied
a rockchip,io-domain-boot-on property. Probe won't be deferred for pin
groups with this property. rockchip,io-domain-boot-on should be added
to all pin groups needed to access the PMIC driving the IO domains.

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

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/pinctrl/pinctrl-rockchip.c | 64 ++++++++++++++++++++++++++++++
 drivers/pinctrl/pinctrl-rockchip.h |  3 ++
 2 files changed, 67 insertions(+)

diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c
index 0276b52f37168..663bd9d6840a5 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>
@@ -2678,6 +2680,43 @@ static int rockchip_pmx_get_groups(struct pinctrl_dev *pctldev,
 	return 0;
 }
 
+static int rockchip_pmx_check_io_domain(struct rockchip_pinctrl *info, unsigned group)
+{
+	struct platform_device *pdev;
+	int i;
+
+	if (!info->io_domains)
+		return 0;
+
+	if (info->groups[group].io_domain_skip)
+		return 0;
+
+	for (i = 0; i < info->num_io_domains; i++) {
+		if (!info->io_domains[i])
+			continue;
+
+		pdev = of_find_device_by_node(info->io_domains[i]);
+		if (!pdev) {
+			dev_err(info->dev, "couldn't find IO domain device\n");
+			return -ENODEV;
+		}
+
+		if (!platform_get_drvdata(pdev)) {
+			dev_err(info->dev, "IO domain device is not probed yet, deferring...(%s)",
+				info->groups[group].name);
+			return -EPROBE_DEFER;
+		}
+
+		of_node_put(info->io_domains[i]);
+		info->io_domains[i] = NULL;
+	}
+
+	devm_kfree(info->dev, info->io_domains);
+	info->io_domains = NULL;
+
+	return 0;
+}
+
 static int rockchip_pmx_set(struct pinctrl_dev *pctldev, unsigned selector,
 			    unsigned group)
 {
@@ -2691,6 +2730,10 @@ 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);
 
+	ret = rockchip_pmx_check_io_domain(info, group);
+	if (ret)
+		return ret;
+
 	/*
 	 * for each pin in the pin group selected, program the corresponding
 	 * pin function number in the config register.
@@ -3019,6 +3062,8 @@ 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");
 
+	grp->io_domain_skip = of_property_read_bool(np, "rockchip,io-domain-boot-on");
+
 	grp->npins = size / 4;
 
 	grp->pins = devm_kcalloc(dev, grp->npins, sizeof(*grp->pins), GFP_KERNEL);
@@ -3417,6 +3462,22 @@ static int rockchip_pinctrl_probe(struct platform_device *pdev)
 			return PTR_ERR(info->regmap_pmu);
 	}
 
+	info->num_io_domains = of_property_count_u32_elems(np, "rockchip,io-domains");
+	if (info->num_io_domains) {
+		int i;
+
+		info->io_domains = devm_kmalloc_array(dev, info->num_io_domains,
+						      sizeof(*info->io_domains), GFP_KERNEL);
+		if (!info->io_domains)
+			return -ENOMEM;
+
+		for (i = 0; i < info->num_io_domains; i++) {
+			info->io_domains[i] = of_parse_phandle(np, "rockchip,io-domains", 0);
+			if (!info->io_domains[i])
+				return -EINVAL;
+		}
+	}
+
 	ret = rockchip_pinctrl_register(pdev, info);
 	if (ret)
 		return ret;
@@ -3439,6 +3500,9 @@ static int rockchip_pinctrl_remove(struct platform_device *pdev)
 
 	of_platform_depopulate(&pdev->dev);
 
+	for (i = 0; i < info->num_io_domains; i++)
+		of_node_put(info->io_domains[i]);
+
 	for (i = 0; i < info->ctrl->nr_banks; i++) {
 		bank = &info->ctrl->pin_banks[i];
 
diff --git a/drivers/pinctrl/pinctrl-rockchip.h b/drivers/pinctrl/pinctrl-rockchip.h
index 4759f336941ef..d2ac79b0a7bc4 100644
--- a/drivers/pinctrl/pinctrl-rockchip.h
+++ b/drivers/pinctrl/pinctrl-rockchip.h
@@ -435,6 +435,7 @@ struct rockchip_pin_group {
 	unsigned int			npins;
 	unsigned int			*pins;
 	struct rockchip_pin_config	*data;
+	bool				io_domain_skip;
 };
 
 /**
@@ -462,6 +463,8 @@ struct rockchip_pinctrl {
 	unsigned int			ngroups;
 	struct rockchip_pmx_func	*functions;
 	unsigned int			nfunctions;
+	struct device_node		**io_domains;
+	int				num_io_domains;
 };
 
 #endif
-- 
2.39.2

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

[Linus Walleij]

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.

Saravana, can you advice how we should handle this properly?

Yours,
Linus Walleij

On Mon, Sep 4, 2023 at 1:58 PM Sascha Hauer <s.hauer@pengutronix.de> wrote:
>
> 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
> configured correctly before they are being used otherwise they do not
> operate correctly.
>
> We currently do not have any knowledge about which pin is on which IO
> domain, so we assume that all pins are on some IO domain and defer
> probing of the pin consumers until the IO domain driver has been probed.
> Some pins however are needed to access the regulators driving an IO
> domain. Deferring probe for them as well would introduce a cyclic
> dependency. To break out of this dependency a pin group can be supplied
> a rockchip,io-domain-boot-on property. Probe won't be deferred for pin
> groups with this property. rockchip,io-domain-boot-on should be added
> to all pin groups needed to access the PMIC driving the IO domains.
>
> [1] drivers/soc/rockchip/io-domain.c
> [2] Documentation/devicetree/bindings/power/rockchip-io-domain.yaml
>
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  drivers/pinctrl/pinctrl-rockchip.c | 64 ++++++++++++++++++++++++++++++
>  drivers/pinctrl/pinctrl-rockchip.h |  3 ++
>  2 files changed, 67 insertions(+)
>
> diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c
> index 0276b52f37168..663bd9d6840a5 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>
> @@ -2678,6 +2680,43 @@ static int rockchip_pmx_get_groups(struct pinctrl_dev *pctldev,
>         return 0;
>  }
>
> +static int rockchip_pmx_check_io_domain(struct rockchip_pinctrl *info, unsigned group)
> +{
> +       struct platform_device *pdev;
> +       int i;
> +
> +       if (!info->io_domains)
> +               return 0;
> +
> +       if (info->groups[group].io_domain_skip)
> +               return 0;
> +
> +       for (i = 0; i < info->num_io_domains; i++) {
> +               if (!info->io_domains[i])
> +                       continue;
> +
> +               pdev = of_find_device_by_node(info->io_domains[i]);
> +               if (!pdev) {
> +                       dev_err(info->dev, "couldn't find IO domain device\n");
> +                       return -ENODEV;
> +               }
> +
> +               if (!platform_get_drvdata(pdev)) {
> +                       dev_err(info->dev, "IO domain device is not probed yet, deferring...(%s)",
> +                               info->groups[group].name);
> +                       return -EPROBE_DEFER;
> +               }
> +
> +               of_node_put(info->io_domains[i]);
> +               info->io_domains[i] = NULL;
> +       }
> +
> +       devm_kfree(info->dev, info->io_domains);
> +       info->io_domains = NULL;
> +
> +       return 0;
> +}
> +
>  static int rockchip_pmx_set(struct pinctrl_dev *pctldev, unsigned selector,
>                             unsigned group)
>  {
> @@ -2691,6 +2730,10 @@ 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);
>
> +       ret = rockchip_pmx_check_io_domain(info, group);
> +       if (ret)
> +               return ret;
> +
>         /*
>          * for each pin in the pin group selected, program the corresponding
>          * pin function number in the config register.
> @@ -3019,6 +3062,8 @@ 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");
>
> +       grp->io_domain_skip = of_property_read_bool(np, "rockchip,io-domain-boot-on");
> +
>         grp->npins = size / 4;
>
>         grp->pins = devm_kcalloc(dev, grp->npins, sizeof(*grp->pins), GFP_KERNEL);
> @@ -3417,6 +3462,22 @@ static int rockchip_pinctrl_probe(struct platform_device *pdev)
>                         return PTR_ERR(info->regmap_pmu);
>         }
>
> +       info->num_io_domains = of_property_count_u32_elems(np, "rockchip,io-domains");
> +       if (info->num_io_domains) {
> +               int i;
> +
> +               info->io_domains = devm_kmalloc_array(dev, info->num_io_domains,
> +                                                     sizeof(*info->io_domains), GFP_KERNEL);
> +               if (!info->io_domains)
> +                       return -ENOMEM;
> +
> +               for (i = 0; i < info->num_io_domains; i++) {
> +                       info->io_domains[i] = of_parse_phandle(np, "rockchip,io-domains", 0);
> +                       if (!info->io_domains[i])
> +                               return -EINVAL;
> +               }
> +       }
> +
>         ret = rockchip_pinctrl_register(pdev, info);
>         if (ret)
>                 return ret;
> @@ -3439,6 +3500,9 @@ static int rockchip_pinctrl_remove(struct platform_device *pdev)
>
>         of_platform_depopulate(&pdev->dev);
>
> +       for (i = 0; i < info->num_io_domains; i++)
> +               of_node_put(info->io_domains[i]);
> +
>         for (i = 0; i < info->ctrl->nr_banks; i++) {
>                 bank = &info->ctrl->pin_banks[i];
>
> diff --git a/drivers/pinctrl/pinctrl-rockchip.h b/drivers/pinctrl/pinctrl-rockchip.h
> index 4759f336941ef..d2ac79b0a7bc4 100644
> --- a/drivers/pinctrl/pinctrl-rockchip.h
> +++ b/drivers/pinctrl/pinctrl-rockchip.h
> @@ -435,6 +435,7 @@ struct rockchip_pin_group {
>         unsigned int                    npins;
>         unsigned int                    *pins;
>         struct rockchip_pin_config      *data;
> +       bool                            io_domain_skip;
>  };
>
>  /**
> @@ -462,6 +463,8 @@ struct rockchip_pinctrl {
>         unsigned int                    ngroups;
>         struct rockchip_pmx_func        *functions;
>         unsigned int                    nfunctions;
> +       struct device_node              **io_domains;
> +       int                             num_io_domains;
>  };
>
>  #endif
> --
> 2.39.2
>

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

[Saravana Kannan]

On Tue, Sep 12, 2023 at 1:06 AM 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.
>
> Saravana, can you advice how we should handle this properly?

Thanks for looping me in. More comments inline.


> Yours,
> Linus Walleij
>
> On Mon, Sep 4, 2023 at 1:58 PM Sascha Hauer <s.hauer@pengutronix.de> wrote:
> >
> > 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
> > configured correctly before they are being used otherwise they do not
> > operate correctly.
> >
> > We currently do not have any knowledge about which pin is on which IO
> > domain, so we assume that all pins are on some IO domain and defer
> > probing of the pin consumers until the IO domain driver has been probed.
> > Some pins however are needed to access the regulators driving an IO
> > domain. Deferring probe for them as well would introduce a cyclic
> > dependency.

It took me a while to understand what's going on with the pinctrl
framework (I'm not too familiar with it) and the rockchip io-domain
driver.

The cyclic dependency seems to be something like this:
pinctrl -> pmu_io_domains -> rk809: pmic@20 -> pinctrl (maybe through
the i2c parent device?).

However, the problem here seems to be that the probe order needs to be
something like:

1. pinctrl registers with pinctrl framework and probes successfully.
2. Defer all pinctrl consumers except rk809.
3. rk809 probes
4. IO domain device probes.
5. Allow the rest of the consumers of pinctrl to probe because the IO
domain device has probed now.

At least in the current state, fw_devlink can't solve this because we
are asking to defer consumers of pinctrl AFTER pinctrl claims it has
successfully probed.

The only way I can think of fixing this at a framework level is to:
1. Add io-domain dependency to all the pins that depend on the pmu_io_domains.
2. Convert each of the struct pin_desc to be a device (feels like an overkill).
3. Add the pin_desc device to a pin_desc bus and have a probe function
that makes the pin "available" for consumers.
3. fw_devlink can make sure pin_desc doesn't probe until the io-domain is ready.

I'll think more about this tomorrow, but this is the best I can come
up with after staring at it for a couple of hours.

-Saravana

> > To break out of this dependency a pin group can be supplied
> > a rockchip,io-domain-boot-on property. Probe won't be deferred for pin
> > groups with this property. rockchip,io-domain-boot-on should be added
> > to all pin groups needed to access the PMIC driving the IO domains.
> >
> > [1] drivers/soc/rockchip/io-domain.c
> > [2] Documentation/devicetree/bindings/power/rockchip-io-domain.yaml
> >
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > ---
> >  drivers/pinctrl/pinctrl-rockchip.c | 64 ++++++++++++++++++++++++++++++
> >  drivers/pinctrl/pinctrl-rockchip.h |  3 ++
> >  2 files changed, 67 insertions(+)
> >
> > diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c
> > index 0276b52f37168..663bd9d6840a5 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>
> > @@ -2678,6 +2680,43 @@ static int rockchip_pmx_get_groups(struct pinctrl_dev *pctldev,
> >         return 0;
> >  }
> >
> > +static int rockchip_pmx_check_io_domain(struct rockchip_pinctrl *info, unsigned group)
> > +{
> > +       struct platform_device *pdev;
> > +       int i;
> > +
> > +       if (!info->io_domains)
> > +               return 0;
> > +
> > +       if (info->groups[group].io_domain_skip)
> > +               return 0;
> > +
> > +       for (i = 0; i < info->num_io_domains; i++) {
> > +               if (!info->io_domains[i])
> > +                       continue;
> > +
> > +               pdev = of_find_device_by_node(info->io_domains[i]);
> > +               if (!pdev) {
> > +                       dev_err(info->dev, "couldn't find IO domain device\n");
> > +                       return -ENODEV;
> > +               }
> > +
> > +               if (!platform_get_drvdata(pdev)) {
> > +                       dev_err(info->dev, "IO domain device is not probed yet, deferring...(%s)",
> > +                               info->groups[group].name);
> > +                       return -EPROBE_DEFER;
> > +               }
> > +
> > +               of_node_put(info->io_domains[i]);
> > +               info->io_domains[i] = NULL;
> > +       }
> > +
> > +       devm_kfree(info->dev, info->io_domains);
> > +       info->io_domains = NULL;
> > +
> > +       return 0;
> > +}
> > +
> >  static int rockchip_pmx_set(struct pinctrl_dev *pctldev, unsigned selector,
> >                             unsigned group)
> >  {
> > @@ -2691,6 +2730,10 @@ 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);
> >
> > +       ret = rockchip_pmx_check_io_domain(info, group);
> > +       if (ret)
> > +               return ret;
> > +
> >         /*
> >          * for each pin in the pin group selected, program the corresponding
> >          * pin function number in the config register.
> > @@ -3019,6 +3062,8 @@ 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");
> >
> > +       grp->io_domain_skip = of_property_read_bool(np, "rockchip,io-domain-boot-on");
> > +
> >         grp->npins = size / 4;
> >
> >         grp->pins = devm_kcalloc(dev, grp->npins, sizeof(*grp->pins), GFP_KERNEL);
> > @@ -3417,6 +3462,22 @@ static int rockchip_pinctrl_probe(struct platform_device *pdev)
> >                         return PTR_ERR(info->regmap_pmu);
> >         }
> >
> > +       info->num_io_domains = of_property_count_u32_elems(np, "rockchip,io-domains");
> > +       if (info->num_io_domains) {
> > +               int i;
> > +
> > +               info->io_domains = devm_kmalloc_array(dev, info->num_io_domains,
> > +                                                     sizeof(*info->io_domains), GFP_KERNEL);
> > +               if (!info->io_domains)
> > +                       return -ENOMEM;
> > +
> > +               for (i = 0; i < info->num_io_domains; i++) {
> > +                       info->io_domains[i] = of_parse_phandle(np, "rockchip,io-domains", 0);
> > +                       if (!info->io_domains[i])
> > +                               return -EINVAL;
> > +               }
> > +       }
> > +
> >         ret = rockchip_pinctrl_register(pdev, info);
> >         if (ret)
> >                 return ret;
> > @@ -3439,6 +3500,9 @@ static int rockchip_pinctrl_remove(struct platform_device *pdev)
> >
> >         of_platform_depopulate(&pdev->dev);
> >
> > +       for (i = 0; i < info->num_io_domains; i++)
> > +               of_node_put(info->io_domains[i]);
> > +
> >         for (i = 0; i < info->ctrl->nr_banks; i++) {
> >                 bank = &info->ctrl->pin_banks[i];
> >
> > diff --git a/drivers/pinctrl/pinctrl-rockchip.h b/drivers/pinctrl/pinctrl-rockchip.h
> > index 4759f336941ef..d2ac79b0a7bc4 100644
> > --- a/drivers/pinctrl/pinctrl-rockchip.h
> > +++ b/drivers/pinctrl/pinctrl-rockchip.h
> > @@ -435,6 +435,7 @@ struct rockchip_pin_group {
> >         unsigned int                    npins;
> >         unsigned int                    *pins;
> >         struct rockchip_pin_config      *data;
> > +       bool                            io_domain_skip;
> >  };
> >
> >  /**
> > @@ -462,6 +463,8 @@ struct rockchip_pinctrl {
> >         unsigned int                    ngroups;
> >         struct rockchip_pmx_func        *functions;
> >         unsigned int                    nfunctions;
> > +       struct device_node              **io_domains;
> > +       int                             num_io_domains;
> >  };
> >
> >  #endif
> > --
> > 2.39.2
> >

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

[Chen-Yu Tsai]

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?

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.

ChenYu



> Saravana, can you advice how we should handle this properly?
>
> Yours,
> Linus Walleij
>
> On Mon, Sep 4, 2023 at 1:58 PM Sascha Hauer <s.hauer@pengutronix.de> wrote:
> >
> > 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
> > configured correctly before they are being used otherwise they do not
> > operate correctly.
> >
> > We currently do not have any knowledge about which pin is on which IO
> > domain, so we assume that all pins are on some IO domain and defer
> > probing of the pin consumers until the IO domain driver has been probed.
> > Some pins however are needed to access the regulators driving an IO
> > domain. Deferring probe for them as well would introduce a cyclic
> > dependency. To break out of this dependency a pin group can be supplied
> > a rockchip,io-domain-boot-on property. Probe won't be deferred for pin
> > groups with this property. rockchip,io-domain-boot-on should be added
> > to all pin groups needed to access the PMIC driving the IO domains.
> >
> > [1] drivers/soc/rockchip/io-domain.c
> > [2] Documentation/devicetree/bindings/power/rockchip-io-domain.yaml
> >
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > ---
> >  drivers/pinctrl/pinctrl-rockchip.c | 64 ++++++++++++++++++++++++++++++
> >  drivers/pinctrl/pinctrl-rockchip.h |  3 ++
> >  2 files changed, 67 insertions(+)
> >
> > diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c
> > index 0276b52f37168..663bd9d6840a5 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>
> > @@ -2678,6 +2680,43 @@ static int rockchip_pmx_get_groups(struct pinctrl_dev *pctldev,
> >         return 0;
> >  }
> >
> > +static int rockchip_pmx_check_io_domain(struct rockchip_pinctrl *info, unsigned group)
> > +{
> > +       struct platform_device *pdev;
> > +       int i;
> > +
> > +       if (!info->io_domains)
> > +               return 0;
> > +
> > +       if (info->groups[group].io_domain_skip)
> > +               return 0;
> > +
> > +       for (i = 0; i < info->num_io_domains; i++) {
> > +               if (!info->io_domains[i])
> > +                       continue;
> > +
> > +               pdev = of_find_device_by_node(info->io_domains[i]);
> > +               if (!pdev) {
> > +                       dev_err(info->dev, "couldn't find IO domain device\n");
> > +                       return -ENODEV;
> > +               }
> > +
> > +               if (!platform_get_drvdata(pdev)) {
> > +                       dev_err(info->dev, "IO domain device is not probed yet, deferring...(%s)",
> > +                               info->groups[group].name);
> > +                       return -EPROBE_DEFER;
> > +               }
> > +
> > +               of_node_put(info->io_domains[i]);
> > +               info->io_domains[i] = NULL;
> > +       }
> > +
> > +       devm_kfree(info->dev, info->io_domains);
> > +       info->io_domains = NULL;
> > +
> > +       return 0;
> > +}
> > +
> >  static int rockchip_pmx_set(struct pinctrl_dev *pctldev, unsigned selector,
> >                             unsigned group)
> >  {
> > @@ -2691,6 +2730,10 @@ 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);
> >
> > +       ret = rockchip_pmx_check_io_domain(info, group);
> > +       if (ret)
> > +               return ret;
> > +
> >         /*
> >          * for each pin in the pin group selected, program the corresponding
> >          * pin function number in the config register.
> > @@ -3019,6 +3062,8 @@ 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");
> >
> > +       grp->io_domain_skip = of_property_read_bool(np, "rockchip,io-domain-boot-on");
> > +
> >         grp->npins = size / 4;
> >
> >         grp->pins = devm_kcalloc(dev, grp->npins, sizeof(*grp->pins), GFP_KERNEL);
> > @@ -3417,6 +3462,22 @@ static int rockchip_pinctrl_probe(struct platform_device *pdev)
> >                         return PTR_ERR(info->regmap_pmu);
> >         }
> >
> > +       info->num_io_domains = of_property_count_u32_elems(np, "rockchip,io-domains");
> > +       if (info->num_io_domains) {
> > +               int i;
> > +
> > +               info->io_domains = devm_kmalloc_array(dev, info->num_io_domains,
> > +                                                     sizeof(*info->io_domains), GFP_KERNEL);
> > +               if (!info->io_domains)
> > +                       return -ENOMEM;
> > +
> > +               for (i = 0; i < info->num_io_domains; i++) {
> > +                       info->io_domains[i] = of_parse_phandle(np, "rockchip,io-domains", 0);
> > +                       if (!info->io_domains[i])
> > +                               return -EINVAL;
> > +               }
> > +       }
> > +
> >         ret = rockchip_pinctrl_register(pdev, info);
> >         if (ret)
> >                 return ret;
> > @@ -3439,6 +3500,9 @@ static int rockchip_pinctrl_remove(struct platform_device *pdev)
> >
> >         of_platform_depopulate(&pdev->dev);
> >
> > +       for (i = 0; i < info->num_io_domains; i++)
> > +               of_node_put(info->io_domains[i]);
> > +
> >         for (i = 0; i < info->ctrl->nr_banks; i++) {
> >                 bank = &info->ctrl->pin_banks[i];
> >
> > diff --git a/drivers/pinctrl/pinctrl-rockchip.h b/drivers/pinctrl/pinctrl-rockchip.h
> > index 4759f336941ef..d2ac79b0a7bc4 100644
> > --- a/drivers/pinctrl/pinctrl-rockchip.h
> > +++ b/drivers/pinctrl/pinctrl-rockchip.h
> > @@ -435,6 +435,7 @@ struct rockchip_pin_group {
> >         unsigned int                    npins;
> >         unsigned int                    *pins;
> >         struct rockchip_pin_config      *data;
> > +       bool                            io_domain_skip;
> >  };
> >
> >  /**
> > @@ -462,6 +463,8 @@ struct rockchip_pinctrl {
> >         unsigned int                    ngroups;
> >         struct rockchip_pmx_func        *functions;
> >         unsigned int                    nfunctions;
> > +       struct device_node              **io_domains;
> > +       int                             num_io_domains;
> >  };
> >
> >  #endif
> > --
> > 2.39.2
> >
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Sascha Hauer]

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

> 
> 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 = <&reg_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.

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: [PATCH 1/3] pinctrl: rockchip: add support for io-domain dependency

[Saravana Kannan]

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.

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?

Is there something that prevents us from doing that?

> >
> > 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 = <&reg_aldo2>;
>          */
> };
>
> At least it show me that I am not the first one who has this problem ;)
>
> We could add the supplies to the pingroup subnodes of the pinctrl driver
> to avoid that, but as Saravana already menioned, that would feel like
> overkill.

So my comment yesterday was that it'd be an overkill to make every
struct pin_desc into a device. But if you can split that rockchip
pinctrl into two devices, that should be okay and definitely not an
overkill.

Maybe something like:

pinctrl {
    compatible = "rockchip,rk3568-pinctrl";
    i2c0 {
                /omit-if-no-ref/
                i2c0_xfer: i2c0-xfer {
                        rockchip,pins =
                                /* i2c0_scl */
                                <0 RK_PB1 1 &pcfg_pull_none_smt>,
                                /* i2c0_sda */
                                <0 RK_PB2 1 &pcfg_pull_none_smt>;
                };
    }
    ...
    ...
    pinctrl-io {
        compatible = "rockchip,rk3568-pinctrl-io";
        pmuio1-supply = <&vcc3v3_pmu>;
        cam {
            ....
        }
        ....
        ....
}

So pinctrl will probe successfully and add it's child device
pinctrl-io. i2c0 will probe once pinctrl is available. Then eventually
the regulator will probe. And after all that, pinctrl-io would probe.

This has no cycles and IMHO represents the hardware accurately. You
have a pinctrl block and there's a sub component of it (pinctrl-io)
that works differently and has additional dependencies.

Any thoughts on this?

-Saravana

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

[Sascha Hauer]

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.
> 
> Then we should try to do this and fix any issues blocking us.
> 
> > However, with that the pinctrl node would get the supplies that the IO
> > domain node now has and we would never get into the probe of the pinctrl
> > driver due to the circular dependencies.
> 
> From a fw_devlink perspective, the circular dependency shouldn't be a
> problem. It's smart enough to recognize all cycle possibilities (since
> 6.3) and not enforce ordering between nodes in a cycle.
> 
> So, this is really only a matter of pinctrl not trying to do
> regulator_get() in its probe function. You need to do the
> regulator_get() when the pins that depend on the io-domain are
> requested. And if the regulator isn't ready yet, return -EPROBE_DEFER?

That's basically what my series does already, I return -EPROBE_DEFER
from the pinctrl driver when a pin is requested and the IO domain is not
yet ready.

> 
> Is there something that prevents us from doing that?

No. We could do that, but it wouldn't buy us anthing. I am glad to hear
that fw_devlink can break the circular dependencies. With this we could
add the supplies to the pinctrl node and the pinctrl driver would still
be probed.

With the IO domain supplies added to the pinctrl node our binding would
be cleaner, but still we would have to defer probe of many requested
pins until finally the I2C driver providing access to the PMIC comes
along. We also still need a "Do not defer probe for these pins" property
in the pingrp needed for the I2C driver.

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 = <&reg_aldo2>;
> >          */
> > };
> >
> > At least it show me that I am not the first one who has this problem ;)
> >
> > We could add the supplies to the pingroup subnodes of the pinctrl driver
> > to avoid that, but as Saravana already menioned, that would feel like
> > overkill.
> 
> So my comment yesterday was that it'd be an overkill to make every
> struct pin_desc into a device. But if you can split that rockchip
> pinctrl into two devices, that should be okay and definitely not an
> overkill.
> 
> Maybe something like:
> 
> pinctrl {
>     compatible = "rockchip,rk3568-pinctrl";
>     i2c0 {
>                 /omit-if-no-ref/
>                 i2c0_xfer: i2c0-xfer {
>                         rockchip,pins =
>                                 /* i2c0_scl */
>                                 <0 RK_PB1 1 &pcfg_pull_none_smt>,
>                                 /* i2c0_sda */
>                                 <0 RK_PB2 1 &pcfg_pull_none_smt>;
>                 };
>     }
>     ...
>     ...
>     pinctrl-io {
>         compatible = "rockchip,rk3568-pinctrl-io";
>         pmuio1-supply = <&vcc3v3_pmu>;
>         cam {
>             ....
>         }
>         ....
>         ....
> }
> 
> So pinctrl will probe successfully and add it's child device
> pinctrl-io. i2c0 will probe once pinctrl is available. Then eventually
> the regulator will probe. And after all that, pinctrl-io would probe.
> 
> This has no cycles and IMHO represents the hardware accurately. You
> have a pinctrl block and there's a sub component of it (pinctrl-io)
> that works differently and has additional dependencies.
> 
> Any thoughts on this?

By making the IO domain device a child node of the pinctrl node we
wouldn't need a phandle from the pinctrl node to the IO domain node
anymore, but apart from that the approach is equivalent to what we have
already.

Given that fw_devlink allows us to add the supplies directly to the
pinctrl node, I would prefer doing that. But as said, it doesn't solve
the problem.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

[Quentin Schulz]

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 = <&reg_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?

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

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

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

[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 = <&reg_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

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

[Saravana Kannan]

On Thu, Sep 14, 2023 at 11:51 PM Sascha Hauer <s.hauer@pengutronix.de> wrote:
>
> On Wed, Sep 13, 2023 at 01:48:12PM -0700, Saravana Kannan wrote:
> > On Tue, Sep 12, 2023 at 11:58 PM Sascha Hauer <s.hauer@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.
> >
> > Then we should try to do this and fix any issues blocking us.
> >
> > > However, with that the pinctrl node would get the supplies that the IO
> > > domain node now has and we would never get into the probe of the pinctrl
> > > driver due to the circular dependencies.
> >
> > From a fw_devlink perspective, the circular dependency shouldn't be a
> > problem. It's smart enough to recognize all cycle possibilities (since
> > 6.3) and not enforce ordering between nodes in a cycle.
> >
> > So, this is really only a matter of pinctrl not trying to do
> > regulator_get() in its probe function. You need to do the
> > regulator_get() when the pins that depend on the io-domain are
> > requested. And if the regulator isn't ready yet, return -EPROBE_DEFER?
>
> That's basically what my series does already, I return -EPROBE_DEFER
> from the pinctrl driver when a pin is requested and the IO domain is not
> yet ready.
>
> >
> > Is there something that prevents us from doing that?
>
> No. We could do that, but it wouldn't buy us anthing. I am glad to hear
> that fw_devlink can break the circular dependencies. With this we could
> add the supplies to the pinctrl node and the pinctrl driver would still
> be probed.
>
> With the IO domain supplies added to the pinctrl node our binding would
> be cleaner, but still we would have to defer probe of many requested
> pins until finally the I2C driver providing access to the PMIC comes
> along. We also still need a "Do not defer probe for these pins" property
> in the pingrp needed for the I2C driver.

Sorry about the slow reply. Been a bit busy.

Oh, this is not true though. With the example binding I gave,
fw_devlink will automatically defer the probe of devices that depend
on pins that need an iodomain/regulator.

pinctrl {
    compatible = "rockchip,rk3568-pinctrl";
    i2c0 {
                /omit-if-no-ref/
                i2c0_xfer: i2c0-xfer {
                        rockchip,pins =
                                /* i2c0_scl */
                                <0 RK_PB1 1 &pcfg_pull_none_smt>,
                                /* i2c0_sda */
                                <0 RK_PB2 1 &pcfg_pull_none_smt>;
                };
    }
    ...
    ...
    pinctrl-io {
        compatible = "rockchip,rk3568-pinctrl-io";
        pmuio1-supply = <&vcc3v3_pmu>;
        cam {
            ....
        }
        ....
        ....
}

consumerA {
   pinctrl-0 = <&cam>;
}

With this model above, there are no cycles anymore.

pictrl doesn't depend on anything.
vcc3v3_pmu will depend on pinctrl (not shown in DT above).
pinctrl-io depends on pinctrl and vcc3v3_pmu.
consumerA depends on pinctrl-io.

So pinctrl probes first.
vcc3v3 will probe next.
pinctrl-io will probe now that the supply is ready.
consumerA will probe now that pinctrl-io is ready.

fw_devlink will enforce all these dependencies because it understands
pinctrl and -supply bindings.

-Saravana

>
> I would consider this being a way to cleanup the bindings, but not a
> solution at DT core level that Linus was aiming at.
>
> >
> > > >
> > > > IIRC on Allwinner SoCs the PMIC pins don't have a separate power rail,
> > > > or if they do they almost certainly use the default I/O rail that is
> > > > always on, and so we omit it to work around the dependency cycle.
> > >
> > > I looked into sun50i as an example. This one has two pinctrl nodes, pio
> > > and r_pio. Only the former has supplies whereas the latter, where the
> > > PMIC is connected to, has (found in sun50i-a64-pinephone.dtsi):
> > >
> > > &r_pio {
> > >         /*
> > >          * FIXME: We can't add that supply for now since it would
> > >          * create a circular dependency between pinctrl, the regulator
> > >          * and the RSB Bus.
> > >          *
> > >          * vcc-pl-supply = <&reg_aldo2>;
> > >          */
> > > };
> > >
> > > At least it show me that I am not the first one who has this problem ;)
> > >
> > > We could add the supplies to the pingroup subnodes of the pinctrl driver
> > > to avoid that, but as Saravana already menioned, that would feel like
> > > overkill.
> >
> > So my comment yesterday was that it'd be an overkill to make every
> > struct pin_desc into a device. But if you can split that rockchip
> > pinctrl into two devices, that should be okay and definitely not an
> > overkill.
> >
> > Maybe something like:
> >
> > pinctrl {
> >     compatible = "rockchip,rk3568-pinctrl";
> >     i2c0 {
> >                 /omit-if-no-ref/
> >                 i2c0_xfer: i2c0-xfer {
> >                         rockchip,pins =
> >                                 /* i2c0_scl */
> >                                 <0 RK_PB1 1 &pcfg_pull_none_smt>,
> >                                 /* i2c0_sda */
> >                                 <0 RK_PB2 1 &pcfg_pull_none_smt>;
> >                 };
> >     }
> >     ...
> >     ...
> >     pinctrl-io {
> >         compatible = "rockchip,rk3568-pinctrl-io";
> >         pmuio1-supply = <&vcc3v3_pmu>;
> >         cam {
> >             ....
> >         }
> >         ....
> >         ....
> > }
> >
> > So pinctrl will probe successfully and add it's child device
> > pinctrl-io. i2c0 will probe once pinctrl is available. Then eventually
> > the regulator will probe. And after all that, pinctrl-io would probe.
> >
> > This has no cycles and IMHO represents the hardware accurately. You
> > have a pinctrl block and there's a sub component of it (pinctrl-io)
> > that works differently and has additional dependencies.
> >
> > Any thoughts on this?
>
> By making the IO domain device a child node of the pinctrl node we
> wouldn't need a phandle from the pinctrl node to the IO domain node
> anymore, but apart from that the approach is equivalent to what we have
> already.
>
> Given that fw_devlink allows us to add the supplies directly to the
> pinctrl node, I would prefer doing that. But as said, it doesn't solve
> the problem.
>
> Sascha
>
> --
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

[Sascha Hauer]

On Wed, Sep 20, 2023 at 03:00:28PM -0700, Saravana Kannan wrote:
> On Thu, Sep 14, 2023 at 11:51 PM Sascha Hauer <s.hauer@pengutronix.de> wrote:
> >
> > On Wed, Sep 13, 2023 at 01:48:12PM -0700, Saravana Kannan wrote:
> > > On Tue, Sep 12, 2023 at 11:58 PM Sascha Hauer <s.hauer@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.
> > >
> > > Then we should try to do this and fix any issues blocking us.
> > >
> > > > However, with that the pinctrl node would get the supplies that the IO
> > > > domain node now has and we would never get into the probe of the pinctrl
> > > > driver due to the circular dependencies.
> > >
> > > From a fw_devlink perspective, the circular dependency shouldn't be a
> > > problem. It's smart enough to recognize all cycle possibilities (since
> > > 6.3) and not enforce ordering between nodes in a cycle.
> > >
> > > So, this is really only a matter of pinctrl not trying to do
> > > regulator_get() in its probe function. You need to do the
> > > regulator_get() when the pins that depend on the io-domain are
> > > requested. And if the regulator isn't ready yet, return -EPROBE_DEFER?
> >
> > That's basically what my series does already, I return -EPROBE_DEFER
> > from the pinctrl driver when a pin is requested and the IO domain is not
> > yet ready.
> >
> > >
> > > Is there something that prevents us from doing that?
> >
> > No. We could do that, but it wouldn't buy us anthing. I am glad to hear
> > that fw_devlink can break the circular dependencies. With this we could
> > add the supplies to the pinctrl node and the pinctrl driver would still
> > be probed.
> >
> > With the IO domain supplies added to the pinctrl node our binding would
> > be cleaner, but still we would have to defer probe of many requested
> > pins until finally the I2C driver providing access to the PMIC comes
> > along. We also still need a "Do not defer probe for these pins" property
> > in the pingrp needed for the I2C driver.
> 
> Sorry about the slow reply. Been a bit busy.
> 
> Oh, this is not true though. With the example binding I gave,
> fw_devlink will automatically defer the probe of devices that depend
> on pins that need an iodomain/regulator.
> 
> pinctrl {
>     compatible = "rockchip,rk3568-pinctrl";
>     i2c0 {
>                 /omit-if-no-ref/
>                 i2c0_xfer: i2c0-xfer {
>                         rockchip,pins =
>                                 /* i2c0_scl */
>                                 <0 RK_PB1 1 &pcfg_pull_none_smt>,
>                                 /* i2c0_sda */
>                                 <0 RK_PB2 1 &pcfg_pull_none_smt>;
>                 };
>     }
>     ...
>     ...
>     pinctrl-io {
>         compatible = "rockchip,rk3568-pinctrl-io";
>         pmuio1-supply = <&vcc3v3_pmu>;
>         cam {
>             ....
>         }
>         ....
>         ....
> }
> 
> consumerA {
>    pinctrl-0 = <&cam>;
> }
> 
> With this model above, there are no cycles anymore.

The cycles are gone because you skipped the problematic case in your
example.

Replace consumerA in your example with the I2C node providing access to
the PMIC which provides &vcc3v3_pmu and then you have the cycles back.

The I2C master device needs the IO domain which needs a regulator
provided by a client on the very same I2C master. The cycles are
actually there in hardware, you can't define them away ;)

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: [PATCH 1/3] pinctrl: rockchip: add support for io-domain dependency

[Saravana Kannan]

On Thu, Sep 21, 2023 at 6:57 AM Sascha Hauer <s.hauer@pengutronix.de> wrote:
>
> On Wed, Sep 20, 2023 at 03:00:28PM -0700, Saravana Kannan wrote:
> > On Thu, Sep 14, 2023 at 11:51 PM Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > >
> > > On Wed, Sep 13, 2023 at 01:48:12PM -0700, Saravana Kannan wrote:
> > > > On Tue, Sep 12, 2023 at 11:58 PM Sascha Hauer <s.hauer@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.
> > > >
> > > > Then we should try to do this and fix any issues blocking us.
> > > >
> > > > > However, with that the pinctrl node would get the supplies that the IO
> > > > > domain node now has and we would never get into the probe of the pinctrl
> > > > > driver due to the circular dependencies.
> > > >
> > > > From a fw_devlink perspective, the circular dependency shouldn't be a
> > > > problem. It's smart enough to recognize all cycle possibilities (since
> > > > 6.3) and not enforce ordering between nodes in a cycle.
> > > >
> > > > So, this is really only a matter of pinctrl not trying to do
> > > > regulator_get() in its probe function. You need to do the
> > > > regulator_get() when the pins that depend on the io-domain are
> > > > requested. And if the regulator isn't ready yet, return -EPROBE_DEFER?
> > >
> > > That's basically what my series does already, I return -EPROBE_DEFER
> > > from the pinctrl driver when a pin is requested and the IO domain is not
> > > yet ready.
> > >
> > > >
> > > > Is there something that prevents us from doing that?
> > >
> > > No. We could do that, but it wouldn't buy us anthing. I am glad to hear
> > > that fw_devlink can break the circular dependencies. With this we could
> > > add the supplies to the pinctrl node and the pinctrl driver would still
> > > be probed.
> > >
> > > With the IO domain supplies added to the pinctrl node our binding would
> > > be cleaner, but still we would have to defer probe of many requested
> > > pins until finally the I2C driver providing access to the PMIC comes
> > > along. We also still need a "Do not defer probe for these pins" property
> > > in the pingrp needed for the I2C driver.
> >
> > Sorry about the slow reply. Been a bit busy.
> >
> > Oh, this is not true though. With the example binding I gave,
> > fw_devlink will automatically defer the probe of devices that depend
> > on pins that need an iodomain/regulator.
> >
> > pinctrl {
> >     compatible = "rockchip,rk3568-pinctrl";
> >     i2c0 {
> >                 /omit-if-no-ref/
> >                 i2c0_xfer: i2c0-xfer {
> >                         rockchip,pins =
> >                                 /* i2c0_scl */
> >                                 <0 RK_PB1 1 &pcfg_pull_none_smt>,
> >                                 /* i2c0_sda */
> >                                 <0 RK_PB2 1 &pcfg_pull_none_smt>;
> >                 };
> >     }
> >     ...
> >     ...
> >     pinctrl-io {
> >         compatible = "rockchip,rk3568-pinctrl-io";
> >         pmuio1-supply = <&vcc3v3_pmu>;
> >         cam {
> >             ....
> >         }
> >         ....
> >         ....
> > }
> >
> > consumerA {
> >    pinctrl-0 = <&cam>;
> > }
> >
> > With this model above, there are no cycles anymore.
>
> The cycles are gone because you skipped the problematic case in your
> example.
>
> Replace consumerA in your example with the I2C node providing access to
> the PMIC which provides &vcc3v3_pmu and then you have the cycles back.

When you are talking about the I2C node that's the bus master for the
PMIC providing the supply, wouldn't it be dependent on "i2c0_xfer"?
And not "cam"?

Otherwise there's a cyclic functional dependency in hardware that can
never be met? Because in that case, your changes would end up
deferring the I2C device probe too.

I'm basically asking to split out the pins that need IO domain to work
into a new subnode "pinctrl-io" of the main "pinctrl" device node.

> The I2C master device needs the IO domain which needs a regulator
> provided by a client on the very same I2C master. The cycles are
> actually there in hardware, you can't define them away ;)

Right, there can be a cyclic connection dependency in hardware and you
can't define them away. But clearly the I2C master doesn't need the IO
domain to work for the I2C to be initialized, right? Otherwise, how
can the I2C hardware be initialized? It doesn't matter what OS we
have, that hardware can't work. So, what am I missing? We are clearly
not on the same page on some details.

Thanks,
Saravana

>
> 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: [PATCH 1/3] pinctrl: rockchip: add support for io-domain dependency

[Sascha Hauer]

On Thu, Sep 21, 2023 at 01:49:21PM -0700, Saravana Kannan wrote:
> On Thu, Sep 21, 2023 at 6:57 AM Sascha Hauer <s.hauer@pengutronix.de> wrote:
> >
> > On Wed, Sep 20, 2023 at 03:00:28PM -0700, Saravana Kannan wrote:
> > > On Thu, Sep 14, 2023 at 11:51 PM Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > > >
> > > > On Wed, Sep 13, 2023 at 01:48:12PM -0700, Saravana Kannan wrote:
> > > > > On Tue, Sep 12, 2023 at 11:58 PM Sascha Hauer <s.hauer@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.
> > > > >
> > > > > Then we should try to do this and fix any issues blocking us.
> > > > >
> > > > > > However, with that the pinctrl node would get the supplies that the IO
> > > > > > domain node now has and we would never get into the probe of the pinctrl
> > > > > > driver due to the circular dependencies.
> > > > >
> > > > > From a fw_devlink perspective, the circular dependency shouldn't be a
> > > > > problem. It's smart enough to recognize all cycle possibilities (since
> > > > > 6.3) and not enforce ordering between nodes in a cycle.
> > > > >
> > > > > So, this is really only a matter of pinctrl not trying to do
> > > > > regulator_get() in its probe function. You need to do the
> > > > > regulator_get() when the pins that depend on the io-domain are
> > > > > requested. And if the regulator isn't ready yet, return -EPROBE_DEFER?
> > > >
> > > > That's basically what my series does already, I return -EPROBE_DEFER
> > > > from the pinctrl driver when a pin is requested and the IO domain is not
> > > > yet ready.
> > > >
> > > > >
> > > > > Is there something that prevents us from doing that?
> > > >
> > > > No. We could do that, but it wouldn't buy us anthing. I am glad to hear
> > > > that fw_devlink can break the circular dependencies. With this we could
> > > > add the supplies to the pinctrl node and the pinctrl driver would still
> > > > be probed.
> > > >
> > > > With the IO domain supplies added to the pinctrl node our binding would
> > > > be cleaner, but still we would have to defer probe of many requested
> > > > pins until finally the I2C driver providing access to the PMIC comes
> > > > along. We also still need a "Do not defer probe for these pins" property
> > > > in the pingrp needed for the I2C driver.
> > >
> > > Sorry about the slow reply. Been a bit busy.
> > >
> > > Oh, this is not true though. With the example binding I gave,
> > > fw_devlink will automatically defer the probe of devices that depend
> > > on pins that need an iodomain/regulator.
> > >
> > > pinctrl {
> > >     compatible = "rockchip,rk3568-pinctrl";
> > >     i2c0 {
> > >                 /omit-if-no-ref/
> > >                 i2c0_xfer: i2c0-xfer {
> > >                         rockchip,pins =
> > >                                 /* i2c0_scl */
> > >                                 <0 RK_PB1 1 &pcfg_pull_none_smt>,
> > >                                 /* i2c0_sda */
> > >                                 <0 RK_PB2 1 &pcfg_pull_none_smt>;
> > >                 };
> > >     }
> > >     ...
> > >     ...
> > >     pinctrl-io {
> > >         compatible = "rockchip,rk3568-pinctrl-io";
> > >         pmuio1-supply = <&vcc3v3_pmu>;
> > >         cam {
> > >             ....
> > >         }
> > >         ....
> > >         ....
> > > }
> > >
> > > consumerA {
> > >    pinctrl-0 = <&cam>;
> > > }
> > >
> > > With this model above, there are no cycles anymore.
> >
> > The cycles are gone because you skipped the problematic case in your
> > example.
> >
> > Replace consumerA in your example with the I2C node providing access to
> > the PMIC which provides &vcc3v3_pmu and then you have the cycles back.
> 
> When you are talking about the I2C node that's the bus master for the
> PMIC providing the supply, wouldn't it be dependent on "i2c0_xfer"?
> And not "cam"?
> 
> Otherwise there's a cyclic functional dependency in hardware that can
> never be met? Because in that case, your changes would end up
> deferring the I2C device probe too.

Yes, that's exactly the problem. There is a functional dependency in
hardware. This can only be resolved by assuming the hardware is already
correctly configured to access the PMIC.

> 
> I'm basically asking to split out the pins that need IO domain to work
> into a new subnode "pinctrl-io" of the main "pinctrl" device node.
> 
> > The I2C master device needs the IO domain which needs a regulator
> > provided by a client on the very same I2C master. The cycles are
> > actually there in hardware, you can't define them away ;)
> 
> Right, there can be a cyclic connection dependency in hardware and you
> can't define them away. But clearly the I2C master doesn't need the IO
> domain to work for the I2C to be initialized, right?

No, not right. The I2C master indeed does need the IO domain to be
correctly configured and the IO domain can only be configured correctly
when we know the voltage the PMIC supplies to the IO domain.

> Otherwise, how
> can the I2C hardware be initialized? It doesn't matter what OS we
> have, that hardware can't work. So, what am I missing? We are clearly
> not on the same page on some details.

This works by configuring the IO domain with static values in the
bootloader which knows the reset default PMIC voltage.

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: [PATCH 1/3] pinctrl: rockchip: add support for io-domain dependency

[Samuel Holland]

On 9/13/23 15:48, 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.
> 
> 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?
> 
> Is there something that prevents us from doing that?

Calling regulator_get() from the pin request function does not solve the
problem on its own. We already do that in the Allwinner driver (in
sunxi_pmx_request()), and we still have the circular dependency:

  __driver_probe_device(I2C/RSB controller)
    pinctrl_bind_pins(I2C/RSB controller)
      pinctrl_select_state(I2C/RSB controller default pins)
        pinmux_enable_setting()
          pin_request()
            sunxi_pmx_request()
              regulator_get(vcc-pl)
                [depends on the PMIC/regulator driver]
                  [depends on the I2C/RSB controller driver]

To break the cycle, you need to defer the regulator_get() call during
this specific call to the pin request function, then come back later and
call regulator_get() once the regulator is actually registered.

If we have a DT property somewhere that provides an initial voltage for
the I/O domain, then regulator_get() returning -EPROBE_DEFER would not
be an error. Instead, we would configure the I/O domain based on the DT
property, and add the pair (IO domain, regulator OF node) to a list.
Then register a notifier for new regulator class devices. Check each new
 device's OF node against the list; if it is found, hook up the voltage
notifier and remove the list entry. When the list is empty, remove the
regulator class notifier.

I thought about (ab)using the pinctrl "init" state so pin_request() gets
called a second time inside pinctrl_init_done() after the PMIC's bus
controller gets probed, but that would rely on the regulator getting
registered synchronously by some recursive call inside the bus
controller probe function. So it would break if probing the
PMIC/regulator driver got deferred for any reason.

So the suggestion from my perspective ends up being the same as what
Robin just suggested elsewhere in the thread. :)

Regards,
Samuel

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

[Rob Herring]

On Wed, Sep 13, 2023 at 08:58:43AM +0200, Sascha Hauer 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.

DT should reflect the hardware. If this is in fact 1 block, then it 
should be 1 DT node. How you want to split the driver or not is up to 
you.

Rob

[PATCH 2/3] dt-bindings: pinctrl: rockchip: Add io domain properties

[Sascha Hauer]

Add rockchip,io-domains property to the Rockchip pinctrl driver. This
list of phandles points to the IO domain device(s) the pins of the
pinctrl driver are supplied from.

Also a rockchip,io-domain-boot-on property is added to pin groups
which can be used for pin groups which themselves are needed to access
the regulators an IO domain is driven from.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 .../bindings/pinctrl/rockchip,pinctrl.yaml          | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.yaml
index 10c335efe619e..92075419d29cf 100644
--- a/Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.yaml
+++ b/Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.yaml
@@ -62,6 +62,11 @@ properties:
       Required for at least rk3188 and rk3288. On the rk3368 this should
       point to the PMUGRF syscon.
 
+  rockchip,io-domains:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    description:
+      Phandles to io domains
+
   "#address-cells":
     enum: [1, 2]
 
@@ -137,7 +142,13 @@ additionalProperties:
             - description:
                 The phandle of a node contains the generic pinconfig options
                 to use as described in pinctrl-bindings.txt.
-
+      rockchip,io-domain-boot-on:
+        type: boolean
+        description:
+          If true assume that the io domain needed for this pin group has been
+          configured correctly by the bootloader. This is needed to break cyclic
+          dependencies introduced when a io domain needs a regulator that can be
+          accessed through pins configured here.
 examples:
   - |
     #include <dt-bindings/interrupt-controller/arm-gic.h>
-- 
2.39.2

Re: [PATCH 2/3] dt-bindings: pinctrl: rockchip: Add io domain properties

[Robin Murphy]

On 2023-09-04 12:58, Sascha Hauer wrote:
> Add rockchip,io-domains property to the Rockchip pinctrl driver. This
> list of phandles points to the IO domain device(s) the pins of the
> pinctrl driver are supplied from.
> 
> Also a rockchip,io-domain-boot-on property is added to pin groups
> which can be used for pin groups which themselves are needed to access
> the regulators an IO domain is driven from.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>   .../bindings/pinctrl/rockchip,pinctrl.yaml          | 13 ++++++++++++-
>   1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.yaml
> index 10c335efe619e..92075419d29cf 100644
> --- a/Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.yaml
> +++ b/Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.yaml
> @@ -62,6 +62,11 @@ properties:
>         Required for at least rk3188 and rk3288. On the rk3368 this should
>         point to the PMUGRF syscon.
>   
> +  rockchip,io-domains:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    description:
> +      Phandles to io domains
> +
>     "#address-cells":
>       enum: [1, 2]
>   
> @@ -137,7 +142,13 @@ additionalProperties:
>               - description:
>                   The phandle of a node contains the generic pinconfig options
>                   to use as described in pinctrl-bindings.txt.
> -
> +      rockchip,io-domain-boot-on:

I don't think "on" is a particularly descriptive or useful property name 
for something that has no "off" state. Furthermore it's no help at all 
if the DT consumer *is* the bootloader that's expected to configure this 
in the first place. IMO it would seem a lot more sensible to have an 
integer (or enum) property which describes the actual value for the 
initial I/O domain setting. Then Linux can choose to assume the presence 
of the property at all implies that the bootloader should have set it up 
already, but also has the option of actively enforcing it as well if we 
want to.

> +        type: boolean
> +        description:
> +          If true assume that the io domain needed for this pin group has been
> +          configured correctly by the bootloader. This is needed to break cyclic
> +          dependencies introduced when a io domain needs a regulator that can be
> +          accessed through pins configured here.

This is describing a Linux implementation detail, not the binding 
itself. There's no technical reason a DT consumer couldn't already 
figure this much out from the existing topology (by observing that the 
pinctrl consumer is a grandparent of the I/O domain's supply).

Thanks,
Robin.

>   examples:
>     - |
>       #include <dt-bindings/interrupt-controller/arm-gic.h>

Re: [PATCH 2/3] dt-bindings: pinctrl: rockchip: Add io domain properties

[Sascha Hauer]

On Tue, Sep 05, 2023 at 10:03:20AM +0100, Robin Murphy wrote:
> On 2023-09-04 12:58, Sascha Hauer wrote:
> > Add rockchip,io-domains property to the Rockchip pinctrl driver. This
> > list of phandles points to the IO domain device(s) the pins of the
> > pinctrl driver are supplied from.
> > 
> > Also a rockchip,io-domain-boot-on property is added to pin groups
> > which can be used for pin groups which themselves are needed to access
> > the regulators an IO domain is driven from.
> > 
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > ---
> >   .../bindings/pinctrl/rockchip,pinctrl.yaml          | 13 ++++++++++++-
> >   1 file changed, 12 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.yaml
> > index 10c335efe619e..92075419d29cf 100644
> > --- a/Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.yaml
> > +++ b/Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.yaml
> > @@ -62,6 +62,11 @@ properties:
> >         Required for at least rk3188 and rk3288. On the rk3368 this should
> >         point to the PMUGRF syscon.
> > +  rockchip,io-domains:
> > +    $ref: /schemas/types.yaml#/definitions/phandle-array
> > +    description:
> > +      Phandles to io domains
> > +
> >     "#address-cells":
> >       enum: [1, 2]
> > @@ -137,7 +142,13 @@ additionalProperties:
> >               - description:
> >                   The phandle of a node contains the generic pinconfig options
> >                   to use as described in pinctrl-bindings.txt.
> > -
> > +      rockchip,io-domain-boot-on:
> 
> I don't think "on" is a particularly descriptive or useful property name for
> something that has no "off" state.

In fact it has an "off" state. A IO Domain can be disabled in the SoC
registers and also the corresponding regulator can be disabled.

> Furthermore it's no help at all if the DT
> consumer *is* the bootloader that's expected to configure this in the first
> place. IMO it would seem a lot more sensible to have an integer (or enum)
> property which describes the actual value for the initial I/O domain
> setting.

I agree though that a particular setting instead of a boolean is better
and could help the bootloader.

> Then Linux can choose to assume the presence of the property at all
> implies that the bootloader should have set it up already, but also has the
> option of actively enforcing it as well if we want to.

Ok.

Thanks,
 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: [PATCH 2/3] dt-bindings: pinctrl: rockchip: Add io domain properties

[Robin Murphy]

On 2023-09-06 08:21, Sascha Hauer wrote:
> On Tue, Sep 05, 2023 at 10:03:20AM +0100, Robin Murphy wrote:
>> On 2023-09-04 12:58, Sascha Hauer wrote:
>>> Add rockchip,io-domains property to the Rockchip pinctrl driver. This
>>> list of phandles points to the IO domain device(s) the pins of the
>>> pinctrl driver are supplied from.
>>>
>>> Also a rockchip,io-domain-boot-on property is added to pin groups
>>> which can be used for pin groups which themselves are needed to access
>>> the regulators an IO domain is driven from.
>>>
>>> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
>>> ---
>>>    .../bindings/pinctrl/rockchip,pinctrl.yaml          | 13 ++++++++++++-
>>>    1 file changed, 12 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.yaml
>>> index 10c335efe619e..92075419d29cf 100644
>>> --- a/Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.yaml
>>> +++ b/Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.yaml
>>> @@ -62,6 +62,11 @@ properties:
>>>          Required for at least rk3188 and rk3288. On the rk3368 this should
>>>          point to the PMUGRF syscon.
>>> +  rockchip,io-domains:
>>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
>>> +    description:
>>> +      Phandles to io domains
>>> +
>>>      "#address-cells":
>>>        enum: [1, 2]
>>> @@ -137,7 +142,13 @@ additionalProperties:
>>>                - description:
>>>                    The phandle of a node contains the generic pinconfig options
>>>                    to use as described in pinctrl-bindings.txt.
>>> -
>>> +      rockchip,io-domain-boot-on:
>>
>> I don't think "on" is a particularly descriptive or useful property name for
>> something that has no "off" state.
> 
> In fact it has an "off" state. A IO Domain can be disabled in the SoC
> registers

Oh, is that a thing on newer SoCs? At least in the RK3399 TRM the only 
I/O-domain-related control I can find is the 1.8V/3.0V logic level 
threshold in GRF_IO_VSEL (plus the one outlier in PMUGRF_SOC_CON0).

> and also the corresponding regulator can be disabled.

...which is clearly a property of the regulator, not of its consumers ;)

However it's also not a meaningful state in this context anyway, since 
if the supply was actually off, and thus we were unable to communicate 
with the PMIC to turn it on... oh dear.

Cheers,
Robin.

>> Furthermore it's no help at all if the DT
>> consumer *is* the bootloader that's expected to configure this in the first
>> place. IMO it would seem a lot more sensible to have an integer (or enum)
>> property which describes the actual value for the initial I/O domain
>> setting.
> 
> I agree though that a particular setting instead of a boolean is better
> and could help the bootloader.
> 
>> Then Linux can choose to assume the presence of the property at all
>> implies that the bootloader should have set it up already, but also has the
>> option of actively enforcing it as well if we want to.
> 
> Ok.
> 
> Thanks,
>   Sascha
> 

Re: [PATCH 2/3] dt-bindings: pinctrl: rockchip: Add io domain properties

[Sascha Hauer]

On Thu, Sep 07, 2023 at 05:35:26PM +0100, Robin Murphy wrote:
> On 2023-09-06 08:21, Sascha Hauer wrote:
> > On Tue, Sep 05, 2023 at 10:03:20AM +0100, Robin Murphy wrote:
> > > On 2023-09-04 12:58, Sascha Hauer wrote:
> > > > Add rockchip,io-domains property to the Rockchip pinctrl driver. This
> > > > list of phandles points to the IO domain device(s) the pins of the
> > > > pinctrl driver are supplied from.
> > > > 
> > > > Also a rockchip,io-domain-boot-on property is added to pin groups
> > > > which can be used for pin groups which themselves are needed to access
> > > > the regulators an IO domain is driven from.
> > > > 
> > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > > > ---
> > > >    .../bindings/pinctrl/rockchip,pinctrl.yaml          | 13 ++++++++++++-
> > > >    1 file changed, 12 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.yaml
> > > > index 10c335efe619e..92075419d29cf 100644
> > > > --- a/Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.yaml
> > > > +++ b/Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.yaml
> > > > @@ -62,6 +62,11 @@ properties:
> > > >          Required for at least rk3188 and rk3288. On the rk3368 this should
> > > >          point to the PMUGRF syscon.
> > > > +  rockchip,io-domains:
> > > > +    $ref: /schemas/types.yaml#/definitions/phandle-array
> > > > +    description:
> > > > +      Phandles to io domains
> > > > +
> > > >      "#address-cells":
> > > >        enum: [1, 2]
> > > > @@ -137,7 +142,13 @@ additionalProperties:
> > > >                - description:
> > > >                    The phandle of a node contains the generic pinconfig options
> > > >                    to use as described in pinctrl-bindings.txt.
> > > > -
> > > > +      rockchip,io-domain-boot-on:
> > > 
> > > I don't think "on" is a particularly descriptive or useful property name for
> > > something that has no "off" state.
> > 
> > In fact it has an "off" state. A IO Domain can be disabled in the SoC
> > registers
> 
> Oh, is that a thing on newer SoCs? At least in the RK3399 TRM the only
> I/O-domain-related control I can find is the 1.8V/3.0V logic level threshold
> in GRF_IO_VSEL (plus the one outlier in PMUGRF_SOC_CON0).

I didn't realize that it's new, the RK3568 is the first Rockchip SoC I
work on, but yes, on RK3568 we have three bits per domain. One bit is to
enable 1.8V, one to enable 2.5V and one for 3.3V. I would assume that
clearing all bits means disable, and whatever strange things may happen
when multiple bits are set...

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: [PATCH 2/3] dt-bindings: pinctrl: rockchip: Add io domain properties

[Quentin Schulz]

Sascha, Robin,

On 9/5/23 11:03, Robin Murphy wrote:
> [You don't often get email from robin.murphy@arm.com. Learn why this is 
> important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> On 2023-09-04 12:58, Sascha Hauer wrote:
>> Add rockchip,io-domains property to the Rockchip pinctrl driver. This
>> list of phandles points to the IO domain device(s) the pins of the
>> pinctrl driver are supplied from.
>>
>> Also a rockchip,io-domain-boot-on property is added to pin groups
>> which can be used for pin groups which themselves are needed to access
>> the regulators an IO domain is driven from.
>>
>> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
>> ---
>>   .../bindings/pinctrl/rockchip,pinctrl.yaml          | 13 ++++++++++++-
>>   1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git 
>> a/Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.yaml 
>> b/Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.yaml
>> index 10c335efe619e..92075419d29cf 100644
>> --- a/Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.yaml
>> +++ b/Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.yaml
>> @@ -62,6 +62,11 @@ properties:
>>         Required for at least rk3188 and rk3288. On the rk3368 this 
>> should
>>         point to the PMUGRF syscon.
>>
>> +  rockchip,io-domains:
>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
>> +    description:
>> +      Phandles to io domains
>> +
>>     "#address-cells":
>>       enum: [1, 2]
>>
>> @@ -137,7 +142,13 @@ additionalProperties:
>>               - description:
>>                   The phandle of a node contains the generic pinconfig 
>> options
>>                   to use as described in pinctrl-bindings.txt.
>> -
>> +      rockchip,io-domain-boot-on:
> 
> I don't think "on" is a particularly descriptive or useful property name
> for something that has no "off" state. Furthermore it's no help at all
> if the DT consumer *is* the bootloader that's expected to configure this
> in the first place. IMO it would seem a lot more sensible to have an
> integer (or enum) property which describes the actual value for the
> initial I/O domain setting. Then Linux can choose to assume the presence
> of the property at all implies that the bootloader should have set it up
> already, but also has the option of actively enforcing it as well if we
> want to.
> 

This is actually highly misleading. Whether the bootloader handles IO 
domains for pinctrl or not absolutely doesn't matter to the kernel since 
the kernel is required to handle IO domain for pinctrl as well. They're 
not exclusive, they are not complementary.

The point is that the voltage of the IO domain **can** change at 
runtime, at any point in time. We could theoretically have the 
bootloader require the regulator to be running at 1.8V and then the 
kernel at 3.3V, both should work (can't think of anything that would 
work like that but why not, the kernel IO domain driver is supposed to 
handle that within the kernel runtime even).

The issue here is that we want to avoid a cyclic dependency, where the 
pinctrl is needed for the IO domain regulator that we're trying to add 
as a dependency of the same pinctrl. There needs to be either some smart 
detection or a property to specify that the IO domain dependency needs 
to be ignored. This seems unfortunately to be for working around how 
Linux handles dependencies between devices and doesn't allow cyclic 
dependencies. At the same time, I do not know if there's anyway to not 
work around it?

>> +        type: boolean
>> +        description:
>> +          If true assume that the io domain needed for this pin group 
>> has been
>> +          configured correctly by the bootloader. This is needed to 
>> break cyclic
>> +          dependencies introduced when a io domain needs a regulator 
>> that can be
>> +          accessed through pins configured here.
> 
> This is describing a Linux implementation detail, not the binding
> itself. There's no technical reason a DT consumer couldn't already
> figure this much out from the existing topology (by observing that the
> pinctrl consumer is a grandparent of the I/O domain's supply).
> 

I am guessing you're suggesting to have some complex handling in the 
driver to detect those cyclic dependencies and ignore the IO domain 
dependency for the pinctrl pins where this happens?

This can actually be quite difficult to detect reliably:
We need to go through the phandle in pinctrl to the IO domain DT node, 
then check all phandles there to other DT node (likely regulators), then 
we need to look into the pinctrl-0 (actually, the one for "default" 
maybe, but what about the other states of pinctrl?) phandles and then 
parse the pinctrt DT nodes to see if they're pointing to the same DT 
node as the one we're trying to use. Here, we also do not know if the 
regulator DT node has other dependencies that needs to be accounted for.

I haven't put too much thoughts into it so maybe it's easier/harder than 
what I'm saying here (or maybe I'm completely off too...).

One of the issues we're having here too is that we lose granularity. 
There are multiple domains inside an IO domain device and here we make 
the whole pinctrl device depend on all domains from one IO domain device 
(there can be multiple ones) while it is factually (on the HW level) 
only dependent on one domain. Considering (if I remember correctly) 
Heiko highly suggested we think about adding child nodes to the IO 
domain devices to have a DT node per domain in the IO domain device, how 
would this work with the suggested DT binding?

Cheers,
Quentin

Re: [PATCH 2/3] dt-bindings: pinctrl: rockchip: Add io domain properties

[Sascha Hauer]

On Wed, Sep 06, 2023 at 10:20:26AM +0200, Quentin Schulz wrote:
> Sascha, Robin,
> 
> On 9/5/23 11:03, Robin Murphy wrote:
> > [You don't often get email from robin.murphy@arm.com. Learn why this is
> > important at https://aka.ms/LearnAboutSenderIdentification ]
> > 
> > > +        type: boolean
> > > +        description:
> > > +          If true assume that the io domain needed for this pin
> > > group has been
> > > +          configured correctly by the bootloader. This is needed to
> > > break cyclic
> > > +          dependencies introduced when a io domain needs a
> > > regulator that can be
> > > +          accessed through pins configured here.
> > 
> > This is describing a Linux implementation detail, not the binding
> > itself. There's no technical reason a DT consumer couldn't already
> > figure this much out from the existing topology (by observing that the
> > pinctrl consumer is a grandparent of the I/O domain's supply).
> > 
> 
> I am guessing you're suggesting to have some complex handling in the driver
> to detect those cyclic dependencies and ignore the IO domain dependency for
> the pinctrl pins where this happens?

I haven't read this as a suggestion, but only as an argument to make it
clear that I should describe the binding rather than anticipating
how it should be used.

I may have misunderstood it though.

> One of the issues we're having here too is that we lose granularity. There
> are multiple domains inside an IO domain device and here we make the whole
> pinctrl device depend on all domains from one IO domain device (there can be
> multiple ones) while it is factually (on the HW level) only dependent on one
> domain. Considering (if I remember correctly) Heiko highly suggested we
> think about adding child nodes to the IO domain devices to have a DT node
> per domain in the IO domain device, how would this work with the suggested
> DT binding?

I started implementing that. I have moved the IO domains into subnodes
of the IO domain controller and started adding phandles from the pin
groups in rk3568-pinctrl.dtsi to the corresponding IO domains. After a
couple of hours I had phandles for around a quarter of the existing
groups of only one SoC, so doing this for all SoCs would really be a
cumbersome job.

In the end I realized this doesn't solve any problem. Also adding the
properties I suggested doesn't prevent us from adding the more specific
dependencies from the pins to their actual IO domains later.

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: [PATCH 2/3] dt-bindings: pinctrl: rockchip: Add io domain properties

[Robin Murphy]

On 06/09/2023 11:19 am, Sascha Hauer wrote:
> On Wed, Sep 06, 2023 at 10:20:26AM +0200, Quentin Schulz wrote:
>> Sascha, Robin,
>>
>> On 9/5/23 11:03, Robin Murphy wrote:
>>> [You don't often get email from robin.murphy@arm.com. Learn why this is
>>> important at https://aka.ms/LearnAboutSenderIdentification ]
>>>
>>>> +ￜￜￜￜￜￜￜ type: boolean
>>>> +ￜￜￜￜￜￜￜ description:
>>>> +ￜￜￜￜￜￜￜￜￜ If true assume that the io domain needed for this pin
>>>> group has been
>>>> +ￜￜￜￜￜￜￜￜￜ configured correctly by the bootloader. This is needed to
>>>> break cyclic
>>>> +ￜￜￜￜￜￜￜￜￜ dependencies introduced when a io domain needs a
>>>> regulator that can be
>>>> +ￜￜￜￜￜￜￜￜￜ accessed through pins configured here.
>>>
>>> This is describing a Linux implementation detail, not the binding
>>> itself. There's no technical reason a DT consumer couldn't already
>>> figure this much out from the existing topology (by observing that the
>>> pinctrl consumer is a grandparent of the I/O domain's supply).
>>>
>>
>> I am guessing you're suggesting to have some complex handling in the driver
>> to detect those cyclic dependencies and ignore the IO domain dependency for
>> the pinctrl pins where this happens?
> 
> I haven't read this as a suggestion, but only as an argument to make it
> clear that I should describe the binding rather than anticipating
> how it should be used.
> 
> I may have misunderstood it though.

Indeed it was more about the definition itself - an extra property isn't 
*needed* to break the cycle since the cycle is already fully described 
in DT, so anyone who can parse parents and phandles already has 
sufficient information to detect it and break it at any point they 
choose. However, as mentioned subsequently, breaking the cycle alone 
isn't enough to guarantee that things will actually work in general.

AFAICS what we fundamentally need to know is the initial voltage of the 
supply regulator, to be able to short-circuit requiring the I/O domain 
in order to query it from the regulator itself, and instead just 
initialise the I/O domain directly. However that would still represent a 
bunch of fiddly extra DT parsing, so for practical purposes it seems 
reasonable to then short-cut that into directly describing the initial 
setting of the I/O domain on the node itself, such that the consumer of 
the binding can easily handle it all in a self-contained manner.

Cheers,
Robin

>> One of the issues we're having here too is that we lose granularity. There
>> are multiple domains inside an IO domain device and here we make the whole
>> pinctrl device depend on all domains from one IO domain device (there can be
>> multiple ones) while it is factually (on the HW level) only dependent on one
>> domain. Considering (if I remember correctly) Heiko highly suggested we
>> think about adding child nodes to the IO domain devices to have a DT node
>> per domain in the IO domain device, how would this work with the suggested
>> DT binding?
> 
> I started implementing that. I have moved the IO domains into subnodes
> of the IO domain controller and started adding phandles from the pin
> groups in rk3568-pinctrl.dtsi to the corresponding IO domains. After a
> couple of hours I had phandles for around a quarter of the existing
> groups of only one SoC, so doing this for all SoCs would really be a
> cumbersome job.
> 
> In the end I realized this doesn't solve any problem. Also adding the
> properties I suggested doesn't prevent us from adding the more specific
> dependencies from the pins to their actual IO domains later.
> 
> Sascha
> 

Re: [PATCH 2/3] dt-bindings: pinctrl: rockchip: Add io domain properties

[Rob Herring]

On Mon, Sep 04, 2023 at 01:58:15PM +0200, Sascha Hauer wrote:
> Add rockchip,io-domains property to the Rockchip pinctrl driver. This
> list of phandles points to the IO domain device(s) the pins of the
> pinctrl driver are supplied from.

Is there an actual need for multiple IO devices with multiple pinctrl 
blocks? If not, you don't need a property, just lookup the IO domain 
node by compatible.

> 
> Also a rockchip,io-domain-boot-on property is added to pin groups
> which can be used for pin groups which themselves are needed to access
> the regulators an IO domain is driven from.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  .../bindings/pinctrl/rockchip,pinctrl.yaml          | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.yaml
> index 10c335efe619e..92075419d29cf 100644
> --- a/Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.yaml
> +++ b/Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.yaml
> @@ -62,6 +62,11 @@ properties:
>        Required for at least rk3188 and rk3288. On the rk3368 this should
>        point to the PMUGRF syscon.
>  
> +  rockchip,io-domains:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    description:
> +      Phandles to io domains
> +
>    "#address-cells":
>      enum: [1, 2]
>  
> @@ -137,7 +142,13 @@ additionalProperties:
>              - description:
>                  The phandle of a node contains the generic pinconfig options
>                  to use as described in pinctrl-bindings.txt.
> -
> +      rockchip,io-domain-boot-on:
> +        type: boolean
> +        description:
> +          If true assume that the io domain needed for this pin group has been
> +          configured correctly by the bootloader. This is needed to break cyclic
> +          dependencies introduced when a io domain needs a regulator that can be
> +          accessed through pins configured here.
>  examples:
>    - |
>      #include <dt-bindings/interrupt-controller/arm-gic.h>
> -- 
> 2.39.2
> 

Re: [PATCH 2/3] dt-bindings: pinctrl: rockchip: Add io domain properties

[Quentin Schulz]

Hi Rob,

On 9/5/23 20:14, Rob Herring wrote:
> On Mon, Sep 04, 2023 at 01:58:15PM +0200, Sascha Hauer wrote:
>> Add rockchip,io-domains property to the Rockchip pinctrl driver. This
>> list of phandles points to the IO domain device(s) the pins of the
>> pinctrl driver are supplied from.
> 
> Is there an actual need for multiple IO devices with multiple pinctrl
> blocks? If not, you don't need a property, just lookup the IO domain
> node by compatible.
> 

Yes. There can be multiple IO domain devices on Rockchip SoCs and we 
typically have only one pinctrl controller. Each pinctrl "pin" (for a 
lack of the appropriate term to use here) belongs to one domain of one 
of the IO domain controller/device.

However what I don't like here is that we do not explicit this link 
between the pinctrl "pin" and the IO domain controller it belongs to 
(even less so on which domain of the IO domain controller, which to be 
fair we do not represent in the DT at the moment except through a 
phandle property in the IO domain controller, c.f. 
https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/power/rockchip-io-domain.yaml#L84).

Cheers,
Quentin

[PATCH 3/3] arm64: dts: rockchip: rock-3a: add io domain properties

[Sascha Hauer]

Tell the pinctrl driver that it shall wait for the io domain driver
before configuring pins. Also add exceptions for the pins needed to
access the PMIC.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 arch/arm64/boot/dts/rockchip/rk3568-rock-3a.dts | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3568-rock-3a.dts b/arch/arm64/boot/dts/rockchip/rk3568-rock-3a.dts
index e05ab11981f55..9d709afc101af 100644
--- a/arch/arm64/boot/dts/rockchip/rk3568-rock-3a.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3568-rock-3a.dts
@@ -300,6 +300,14 @@ &hdmi_sound {
 	status = "okay";
 };
 
+&i2c0_xfer {
+	rockchip,io-domain-boot-on;
+};
+
+&i2s1m0_mclk {
+	rockchip,io-domain-boot-on;
+};
+
 &i2c0 {
 	status = "okay";
 
@@ -615,6 +623,8 @@ &pcie3x2 {
 };
 
 &pinctrl {
+	rockchip,io-domains = <&pmu_io_domains>;
+
 	cam {
 		vcc_cam_en: vcc_cam_en {
 			rockchip,pins = <1 RK_PB1 RK_FUNC_GPIO &pcfg_pull_none>;
@@ -659,6 +669,7 @@ pmic {
 		pmic_int: pmic_int {
 			rockchip,pins =
 				<0 RK_PA3 RK_FUNC_GPIO &pcfg_pull_up>;
+		rockchip,io-domain-boot-on;
 		};
 	};
 
-- 
2.39.2

Re: [PATCH 0/3] Make Rockchip IO domains dependency from other devices explicit

[Jonas Karlman]

Hi Sascha,

On 2023-09-04 13:58, Sascha Hauer wrote:
> This is a continuation of the patch posted by Quentin Schulz here [1]
> 
> This series aims to solve a problem with Rockchip IO domains. On many
> Rockchip SoCs the pins are driven by external supplies normally
> controlled by regulators of a PMIC. There are multiple voltages allowed
> for the regulators; additionally the chosen voltage has to be programmed
> into SoC registers. There already is a driver [2] handling setting these
> registers. The driver works by registering a notifier on the regulators.
> Whenever a regulator is about to change its voltage then the notifier will
> program the IO domain registers suitably for the new voltage.
> 
> The problem is that there is no dependency between pins and the IO
> domain driver which means that it can happen that a pin is used before
> the IO domain driver has been probed. In that case the pin can end up
> being non functional as neither the regulator has been configured
> correctly nor the SoC registers have been adjusted to the regulators
> voltage.
> 
> One way to ensure correct probing order is to defer probing of devices
> in the pinctrl driver until the IO domain driver has been probed. We
> can't do this for all devices though, as that would introduce a cyclic
> dependency when for example the I2C port needed to access the PMIC for
> the regulators is part of a IO domain itself.
> 
> This series solves these problems similarly to Quentins patch. With
> Quentins patch we would have to add rockchip,io-domain properties for
> all pin group nodes we wish to honor the IO domain dependency for. We
> could put these properties into the board dts files which would mean
> that we either only add the properties to nodes which actually byte us,
> or that we would have to add the properties to all possible pin groups
> except the ones needed to access the PMIC. We could also put these
> properties into the dtsi files, but that would mean a board has to add a
> /delete-property/ rockchip,io-domain to the pin groups needed to access
> the PMIC to avoid circular dependencies.
> 
> The approach chosen here is slightly different. First of all this series
> doesn't change the current behaviour without board specific dts changes.
> To activate the IO domain dependency handling, a board has to add a
> rockchip,io-domains property to the pinctrl node. When this property is
> present all pins are assumed to need the IO domain driver. Pin groups
> needed to access the PMIC can then be given a rockchip,io-domain-boot-on
> property. When this property is given then the IO domain is assumed to
> be correctly configured by the boot loader. It should be added to all
> pin groups needed to access the PMIC to avoid cyclic dependencies. Patch
> 3/3 contains a usage example for the Radxa Rock-3a.

FYI, I have sent out a series that ports the IO domain driver to U-Boot.
This was needed to have working Ethernet on RK356x devices that use a
1.8V PHY. Initially only RK356x support have been ported but support for
other SoCs should follow in the future. Vendor U-Boot also initialize
the IO domain configuration based on the voltage reported by the
supplying regulator.

So at least for the example board IO domains should be configured when
entering linux while booting using a future version of mainline U-Boot.

https://lore.kernel.org/u-boot/20230821223020.3918620-1-jonas@kwiboo.se/

Regards,
Jonas

> 
> Sascha
> 
> [1] https://lore.kernel.org/lkml/20220802095252.2486591-1-foss+kernel@0leil.net/
> [2] drivers/soc/rockchip/io-domain.c
> 
> Sascha Hauer (3):
>   pinctrl: rockchip: add support for io-domain dependency
>   dt-bindings: pinctrl: rockchip: Add io domain properties
>   arm64: dts: rockchip: rock-3a: add io domain properties
> 
>  .../bindings/pinctrl/rockchip,pinctrl.yaml    | 13 +++-
>  .../boot/dts/rockchip/rk3568-rock-3a.dts      | 11 ++++
>  drivers/pinctrl/pinctrl-rockchip.c            | 64 +++++++++++++++++++
>  drivers/pinctrl/pinctrl-rockchip.h            |  3 +
>  4 files changed, 90 insertions(+), 1 deletion(-)
>