| Message ID | 20230904115816.1237684-2-s.hauer@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Make Rockchip IO domains dependency from other devices explicit | expand |
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 >
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 > >
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
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 = <®_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
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 = <®_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
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 = <®_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
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
Hi all, On 9/15/23 08:51, Sascha Hauer wrote: > On Wed, Sep 13, 2023 at 01:48:12PM -0700, Saravana Kannan wrote: >> On Tue, Sep 12, 2023 at 11:58 PM Sascha Hauer <s.hauer@pengutronix.de> wrote: >>> >>> On Wed, Sep 13, 2023 at 12:37:54PM +0800, Chen-Yu Tsai wrote: >>>> On Tue, Sep 12, 2023 at 4:07 PM Linus Walleij <linus.walleij@linaro.org> wrote: >>>>> >>>>> Top posting to bring Saravana Kannan into this discussion. >>>>> >>>>> This looks like a big hack to me, Saravana has been working >>>>> tirelessly to make the device tree probe order "sort itself out" >>>>> and I am pretty sure this issue needs to be fixed at the DT >>>>> core level and not in a driver. >>>> >>>> We could merge all the IO domain stuff into the pinctrl node/driver, >>>> like is done for Allwinner? Maybe that would simplify things a bit? >>> >>> I thought about this as well. On Rockchip the pinctrl driver and the IO >>> domain driver even work on the same register space, so putting these >>> into a single node/driver would even feel more natural than what we have >>> now. >> While technically not really wrong, I wouldn't say this is true either. There's no real pinctrl IP address space on Rockchip SoCs (at least the ones I worked on, so RK3399 and PX30), at least nothing delimited properly. The typical pinctrl duties are scattered all over two (more depending on the SoC maybe?) register address spaces, for PX30 and RK3399 they are called GRF and PMUGRF. Many, MANY, IPs actually have some registers to modify in those two register address spaces as well, c.f. all the rockchip,grf and rockchip,pmu properties all over the place. The pinctrl driver does refer those two register address spaces via the aforementioned DT properties. Those two register address spaces are represented as syscon... because if I remember correctly that's how you are supposed to handle multiple devices on the same register address space where registers or even register bitfields are mixed for different IPs? At the same time, IO domains also aren't in their own "real" address space, similar as to how pinctrl is handled in HW. >> Then we should try to do this and fix any issues blocking us. >> >>> However, with that the pinctrl node would get the supplies that the IO >>> domain node now has and we would never get into the probe of the pinctrl >>> driver due to the circular dependencies. >> >> From a fw_devlink perspective, the circular dependency shouldn't be a >> problem. It's smart enough to recognize all cycle possibilities (since >> 6.3) and not enforce ordering between nodes in a cycle. >> >> So, this is really only a matter of pinctrl not trying to do >> regulator_get() in its probe function. You need to do the >> regulator_get() when the pins that depend on the io-domain are >> requested. And if the regulator isn't ready yet, return -EPROBE_DEFER? > > That's basically what my series does already, I return -EPROBE_DEFER > from the pinctrl driver when a pin is requested and the IO domain is not > yet ready. > >> >> Is there something that prevents us from doing that? > > No. We could do that, but it wouldn't buy us anthing. I am glad to hear > that fw_devlink can break the circular dependencies. With this we could > add the supplies to the pinctrl node and the pinctrl driver would still > be probed. > > With the IO domain supplies added to the pinctrl node our binding would > be cleaner, but still we would have to defer probe of many requested > pins until finally the I2C driver providing access to the PMIC comes I don't think there's any way around the deferral "of many requested pins until finally the I2C driver providing access to the PMIC comes along", this is actually necessary. > along. We also still need a "Do not defer probe for these pins" property > in the pingrp needed for the I2C driver. > Yes, this is the difficult part right now. In the RFC, I suggested to have an io-domains property per pinmux DT node: """ &pinctrl { group { pinmux { io-domains = <&io_domains>; rockchip,pins = <0 RK_PA0 0 &pcfg_pull_none>, <3 RK_PB5 0 &pcfg_pull_none>; }; }; }; """ But this is very tedious for both SoC maintainers (though they would probably have to do it "only" once) AND for board maintainers, for each new pinmux they require. Since the SoC maintainer cannot know on which i2c (or spi?) bus the PMIC will be, they have two choices: either let board maintainers not forget to add the io-domains property to the i2c/spi pinmux nodes of all but the one to whcih the PMIC is attached, or have the board maintainers add a /delete-property/ io-domains to the proper i2c/spi pinmux node to which the PMIC is attached. The first one is very error-prone, the second is not very liked by DT people I think (and also... well is a hack on DT level to manage a driver/subsystem issue). Also on a side note, the current binding for the io-domains is a bit not granular enough as it represents the collection of IO domains on the same register address space, and you can have multiple ones. e.g. for RK3399 you have four in "grf"/"normal" IO domain, which makes the inter-dependencies unnecessarily complex (but that's probably tangent to the current problem in discussion). > I would consider this being a way to cleanup the bindings, but not a > solution at DT core level that Linus was aiming at. > >> >>>> >>>> IIRC on Allwinner SoCs the PMIC pins don't have a separate power rail, >>>> or if they do they almost certainly use the default I/O rail that is >>>> always on, and so we omit it to work around the dependency cycle. >>> >>> I looked into sun50i as an example. This one has two pinctrl nodes, pio >>> and r_pio. Only the former has supplies whereas the latter, where the >>> PMIC is connected to, has (found in sun50i-a64-pinephone.dtsi): >>> >>> &r_pio { >>> /* >>> * FIXME: We can't add that supply for now since it would >>> * create a circular dependency between pinctrl, the regulator >>> * and the RSB Bus. >>> * >>> * vcc-pl-supply = <®_aldo2>; >>> */ >>> }; >>> >>> At least it show me that I am not the first one who has this problem ;) >>> >>> We could add the supplies to the pingroup subnodes of the pinctrl driver >>> to avoid that, but as Saravana already menioned, that would feel like >>> overkill. >> I think this is actually a somewhat bad idea. Let me explain. Nothing prevents the user to create a new DT node with two pins from different IO domains. e.g. I could very well have the following: """ &pinctrl { group { two_iodomain_mux { rockchip,pins = <0 RK_PA0 0 &pcfg_pull_none>, <3 RK_PB5 0 &pcfg_pull_none>; }; }; }; """ for example if I have a device that uses GPIO0_A0 and GPIO3_B5 as gpios and I need to configure their pinconf appropriately. So from this, I guess we'd need to support multiple io-domains per node (don't know the proper pinctrl subsystem name for that one sorry, the two_iodomain_mux one in the above example). We could also now group pinmux nodes by their io-domain, e.g.: """ &pinctrl { bt656-io-domain { power-supply = <&whatever>; only_pinmuxes_from_bt656 { }; only_pinmuxes_from_bt656_2 { }; }; pmu1830-io-domain { power-supply = <&something>; only_pinmuxes_from_pmu1830 { }; only_pinmuxes_from_pmu1830_2 { }; }; [...] }; """ This means we would need to go through all existing pinmux definition on rockchip devices and check if they belong to the same io domain and if they don't, split them in one or more pinmuxes for example. Also, I think it'd be easier to ask board maintainers to only add a power-supply property to all io-domains rather than to each and every pinmux. We could probably enforce that no subgroup other than the one named after the ones named after the io-domain can be created on the driver level as well. Not sure if it's wise but we could probably also check that within a pingroup only pinmuxes belonging to the io-domain are listed. >> So my comment yesterday was that it'd be an overkill to make every >> struct pin_desc into a device. But if you can split that rockchip >> pinctrl into two devices, that should be okay and definitely not an >> overkill. >> >> Maybe something like: >> >> pinctrl { >> compatible = "rockchip,rk3568-pinctrl"; >> i2c0 { >> /omit-if-no-ref/ >> i2c0_xfer: i2c0-xfer { >> rockchip,pins = >> /* i2c0_scl */ >> <0 RK_PB1 1 &pcfg_pull_none_smt>, >> /* i2c0_sda */ >> <0 RK_PB2 1 &pcfg_pull_none_smt>; >> }; >> } >> ... >> ... >> pinctrl-io { >> compatible = "rockchip,rk3568-pinctrl-io"; >> pmuio1-supply = <&vcc3v3_pmu>; >> cam { >> .... >> } >> .... >> .... >> } >> >> So pinctrl will probe successfully and add it's child device >> pinctrl-io. i2c0 will probe once pinctrl is available. Then eventually >> the regulator will probe. And after all that, pinctrl-io would probe. >> >> This has no cycles and IMHO represents the hardware accurately. You >> have a pinctrl block and there's a sub component of it (pinctrl-io) >> that works differently and has additional dependencies. >> >> Any thoughts on this? > Just to be clear that whether i2c0 is where the PMIC is is HW dependent, so we cannot have that in the main SoC dtsi (on Rockchip we typically have a bunch of those in the main SoC dtsi to avoid common nodes to be copy-pasted all over the place). > By making the IO domain device a child node of the pinctrl node we > wouldn't need a phandle from the pinctrl node to the IO domain node > anymore, but apart from that the approach is equivalent to what we have > already. > Indeed, just one less item in the cyclic dependency but it's still there. > Given that fw_devlink allows us to add the supplies directly to the > pinctrl node, I would prefer doing that. But as said, it doesn't solve > the problem. > Absolutely. The issue is that we have pinctrl that needs to probe for anything to work really. Considering that pinctrl pingroups requires (on the HW level) to be linked to an IO domain to be working properly, the IO domain depending on a regulator (which can have different voltages at runtime, hence why this link is absolutely critical to not damage the SoC by having the IO domain configured for a different voltage than provided by its regulator), which is on a bus (i2c/spi) that needs a specific pinmux in order to work. Saravana gave one example of the cyclic dependency on the DT level earlier. The issue isn't the DT-part of the cyclic dependency, it's that the drivers actually also have this cyclic dependency, the i2c/spi controller via its pinctrl default state and the pinctrl driver with a dependency on a PMIC driver that could'nt have been probed yet because its on the i2c bus. I don't see how we can not have a special property in the DT binding for ignoring this cyclic dependency for one specific loop. We cannot hardcode the driver to look for a specific compatible or something like that because this is HW dependent, there's no rule on which i2c/spi bus one needs to put their PMIC on. Maybe we could try to look for the PMIC on child nodes of consumers of pinctrl (if possible only when a cyclic dependency is detected) and bypass this dependency on the regulator? Or maybe check if the parent of the PMIC of the IO domain of the currently requested pingroup is the same as the consumer of the currently requested pinmux within this pingroup? 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
On 15/09/2023 5:38 pm, Quentin Schulz wrote: > Hi all, > > On 9/15/23 08:51, Sascha Hauer wrote: >> On Wed, Sep 13, 2023 at 01:48:12PM -0700, Saravana Kannan wrote: >>> On Tue, Sep 12, 2023 at 11:58 PM Sascha Hauer >>> <s.hauer@pengutronix.de> wrote: >>>> >>>> On Wed, Sep 13, 2023 at 12:37:54PM +0800, Chen-Yu Tsai wrote: >>>>> On Tue, Sep 12, 2023 at 4:07 PM Linus Walleij >>>>> <linus.walleij@linaro.org> wrote: >>>>>> >>>>>> Top posting to bring Saravana Kannan into this discussion. >>>>>> >>>>>> This looks like a big hack to me, Saravana has been working >>>>>> tirelessly to make the device tree probe order "sort itself out" >>>>>> and I am pretty sure this issue needs to be fixed at the DT >>>>>> core level and not in a driver. >>>>> >>>>> We could merge all the IO domain stuff into the pinctrl node/driver, >>>>> like is done for Allwinner? Maybe that would simplify things a bit? >>>> >>>> I thought about this as well. On Rockchip the pinctrl driver and the IO >>>> domain driver even work on the same register space, so putting these >>>> into a single node/driver would even feel more natural than what we >>>> have >>>> now. >>> > > While technically not really wrong, I wouldn't say this is true either. > > There's no real pinctrl IP address space on Rockchip SoCs (at least the > ones I worked on, so RK3399 and PX30), at least nothing delimited > properly. The typical pinctrl duties are scattered all over two (more > depending on the SoC maybe?) register address spaces, for PX30 and > RK3399 they are called GRF and PMUGRF. Many, MANY, IPs actually have > some registers to modify in those two register address spaces as well, > c.f. all the rockchip,grf and rockchip,pmu properties all over the place. > > The pinctrl driver does refer those two register address spaces via the > aforementioned DT properties. Those two register address spaces are > represented as syscon... because if I remember correctly that's how you > are supposed to handle multiple devices on the same register address > space where registers or even register bitfields are mixed for different > IPs? > > At the same time, IO domains also aren't in their own "real" address > space, similar as to how pinctrl is handled in HW. > >>> Then we should try to do this and fix any issues blocking us. >>> >>>> However, with that the pinctrl node would get the supplies that the IO >>>> domain node now has and we would never get into the probe of the >>>> pinctrl >>>> driver due to the circular dependencies. >>> >>> From a fw_devlink perspective, the circular dependency shouldn't be a >>> problem. It's smart enough to recognize all cycle possibilities (since >>> 6.3) and not enforce ordering between nodes in a cycle. >>> >>> So, this is really only a matter of pinctrl not trying to do >>> regulator_get() in its probe function. You need to do the >>> regulator_get() when the pins that depend on the io-domain are >>> requested. And if the regulator isn't ready yet, return -EPROBE_DEFER? >> >> That's basically what my series does already, I return -EPROBE_DEFER >> from the pinctrl driver when a pin is requested and the IO domain is not >> yet ready. >> >>> >>> Is there something that prevents us from doing that? >> >> No. We could do that, but it wouldn't buy us anthing. I am glad to hear >> that fw_devlink can break the circular dependencies. With this we could >> add the supplies to the pinctrl node and the pinctrl driver would still >> be probed. >> >> With the IO domain supplies added to the pinctrl node our binding would >> be cleaner, but still we would have to defer probe of many requested >> pins until finally the I2C driver providing access to the PMIC comes > > I don't think there's any way around the deferral "of many requested > pins until finally the I2C driver providing access to the PMIC comes > along", this is actually necessary. > >> along. We also still need a "Do not defer probe for these pins" property >> in the pingrp needed for the I2C driver. > > > > Yes, this is the difficult part right now. In the RFC, I suggested to > have an io-domains property per pinmux DT node: > > """ > &pinctrl { > group { > pinmux { > io-domains = <&io_domains>; > rockchip,pins = <0 RK_PA0 0 &pcfg_pull_none>, > <3 RK_PB5 0 &pcfg_pull_none>; > }; > }; > }; > """ > > But this is very tedious for both SoC maintainers (though they would > probably have to do it "only" once) AND for board maintainers, for each > new pinmux they require. Since the SoC maintainer cannot know on which > i2c (or spi?) bus the PMIC will be, they have two choices: either let > board maintainers not forget to add the io-domains property to the > i2c/spi pinmux nodes of all but the one to whcih the PMIC is attached, > or have the board maintainers add a /delete-property/ io-domains to the > proper i2c/spi pinmux node to which the PMIC is attached. > > The first one is very error-prone, the second is not very liked by DT > people I think (and also... well is a hack on DT level to manage a > driver/subsystem issue). > > Also on a side note, the current binding for the io-domains is a bit not > granular enough as it represents the collection of IO domains on the > same register address space, and you can have multiple ones. e.g. for > RK3399 you have four in "grf"/"normal" IO domain, which makes the > inter-dependencies unnecessarily complex (but that's probably tangent to > the current problem in discussion). > >> I would consider this being a way to cleanup the bindings, but not a >> solution at DT core level that Linus was aiming at. >> >>> >>>>> >>>>> IIRC on Allwinner SoCs the PMIC pins don't have a separate power rail, >>>>> or if they do they almost certainly use the default I/O rail that is >>>>> always on, and so we omit it to work around the dependency cycle. >>>> >>>> I looked into sun50i as an example. This one has two pinctrl nodes, pio >>>> and r_pio. Only the former has supplies whereas the latter, where the >>>> PMIC is connected to, has (found in sun50i-a64-pinephone.dtsi): >>>> >>>> &r_pio { >>>> /* >>>> * FIXME: We can't add that supply for now since it would >>>> * create a circular dependency between pinctrl, the regulator >>>> * and the RSB Bus. >>>> * >>>> * vcc-pl-supply = <®_aldo2>; >>>> */ >>>> }; >>>> >>>> At least it show me that I am not the first one who has this problem ;) >>>> >>>> We could add the supplies to the pingroup subnodes of the pinctrl >>>> driver >>>> to avoid that, but as Saravana already menioned, that would feel like >>>> overkill. >>> > > I think this is actually a somewhat bad idea. Let me explain. > > Nothing prevents the user to create a new DT node with two pins from > different IO domains. e.g. I could very well have the following: > > """ > &pinctrl { > group { > two_iodomain_mux { > rockchip,pins = <0 RK_PA0 0 &pcfg_pull_none>, > <3 RK_PB5 0 &pcfg_pull_none>; > }; > }; > }; > """ > > for example if I have a device that uses GPIO0_A0 and GPIO3_B5 as gpios > and I need to configure their pinconf appropriately. > > So from this, I guess we'd need to support multiple io-domains per node > (don't know the proper pinctrl subsystem name for that one sorry, the > two_iodomain_mux one in the above example). > > We could also now group pinmux nodes by their io-domain, e.g.: > > """ > &pinctrl { > bt656-io-domain { > power-supply = <&whatever>; > > only_pinmuxes_from_bt656 { > }; > > only_pinmuxes_from_bt656_2 { > }; > }; > pmu1830-io-domain { > power-supply = <&something>; > > only_pinmuxes_from_pmu1830 { > }; > > only_pinmuxes_from_pmu1830_2 { > }; > }; > [...] > }; > """ > > This means we would need to go through all existing pinmux definition on > rockchip devices and check if they belong to the same io domain and if > they don't, split them in one or more pinmuxes for example. > > Also, I think it'd be easier to ask board maintainers to only add a > power-supply property to all io-domains rather than to each and every > pinmux. > > We could probably enforce that no subgroup other than the one named > after the ones named after the io-domain can be created on the driver > level as well. Not sure if it's wise but we could probably also check > that within a pingroup only pinmuxes belonging to the io-domain are listed. > >>> So my comment yesterday was that it'd be an overkill to make every >>> struct pin_desc into a device. But if you can split that rockchip >>> pinctrl into two devices, that should be okay and definitely not an >>> overkill. >>> >>> Maybe something like: >>> >>> pinctrl { >>> compatible = "rockchip,rk3568-pinctrl"; >>> i2c0 { >>> /omit-if-no-ref/ >>> i2c0_xfer: i2c0-xfer { >>> rockchip,pins = >>> /* i2c0_scl */ >>> <0 RK_PB1 1 &pcfg_pull_none_smt>, >>> /* i2c0_sda */ >>> <0 RK_PB2 1 &pcfg_pull_none_smt>; >>> }; >>> } >>> ... >>> ... >>> pinctrl-io { >>> compatible = "rockchip,rk3568-pinctrl-io"; >>> pmuio1-supply = <&vcc3v3_pmu>; >>> cam { >>> .... >>> } >>> .... >>> .... >>> } >>> >>> So pinctrl will probe successfully and add it's child device >>> pinctrl-io. i2c0 will probe once pinctrl is available. Then eventually >>> the regulator will probe. And after all that, pinctrl-io would probe. >>> >>> This has no cycles and IMHO represents the hardware accurately. You >>> have a pinctrl block and there's a sub component of it (pinctrl-io) >>> that works differently and has additional dependencies. >>> >>> Any thoughts on this? >> > > Just to be clear that whether i2c0 is where the PMIC is is HW dependent, > so we cannot have that in the main SoC dtsi (on Rockchip we typically > have a bunch of those in the main SoC dtsi to avoid common nodes to be > copy-pasted all over the place). > >> By making the IO domain device a child node of the pinctrl node we >> wouldn't need a phandle from the pinctrl node to the IO domain node >> anymore, but apart from that the approach is equivalent to what we have >> already. >> > > Indeed, just one less item in the cyclic dependency but it's still there. > >> Given that fw_devlink allows us to add the supplies directly to the >> pinctrl node, I would prefer doing that. But as said, it doesn't solve >> the problem. >> > > Absolutely. > > The issue is that we have pinctrl that needs to probe for anything to > work really. > > Considering that pinctrl pingroups requires (on the HW level) to be > linked to an IO domain to be working properly, the IO domain depending > on a regulator (which can have different voltages at runtime, hence why > this link is absolutely critical to not damage the SoC by having the IO > domain configured for a different voltage than provided by its > regulator), which is on a bus (i2c/spi) that needs a specific pinmux in > order to work. > > Saravana gave one example of the cyclic dependency on the DT level > earlier. The issue isn't the DT-part of the cyclic dependency, it's that > the drivers actually also have this cyclic dependency, the i2c/spi > controller via its pinctrl default state and the pinctrl driver with a > dependency on a PMIC driver that could'nt have been probed yet because > its on the i2c bus. I don't see how we can not have a special property > in the DT binding for ignoring this cyclic dependency for one specific > loop. We cannot hardcode the driver to look for a specific compatible or > something like that because this is HW dependent, there's no rule on > which i2c/spi bus one needs to put their PMIC on. Maybe we could try to > look for the PMIC on child nodes of consumers of pinctrl (if possible > only when a cyclic dependency is detected) and bypass this dependency on > the regulator? Or maybe check if the parent of the PMIC of the IO domain > of the currently requested pingroup is the same as the consumer of the > currently requested pinmux within this pingroup? This is why I think it makes the most sense to describe the initial I/O domain voltage as a property of the I/O domain, since that is most truthful to what is actually needed to initialise the hardware. Ideally we could then just use that initial configuration to probe successfully, and use a notifier to pick up the regulator if and when it does appear later (on the basis that the voltage should not be able to change *without* one). And otherwise if an initial voltage isn't specified then we can assume it's OK to wait for the regulator and query it as normal. > I'm also wondering how this would play out if the PMIC isn't supplying > power to the IO domain the bus controller to which it's connected... but > I guess that's a HW design issue :) Hopefully that would just be a sensible non-cyclic design that avoids this problem altogether. However there's nothing special about PMICs, this could equally apply if the I/O domain was supplied by a GPIO regulator or PWM regulator which used one of its own pins; the fundamental problem to solve is being able to determine the correct initial voltage setting for an I/O domain without having to perform any I/O via that domain itself. That is the physical dependency cycle which can exist here, regardless of how we address any other software dependencies between drivers within Linux. Thanks, Robin. > > It's Friday evening here so hopefully my brain wasn't already on weekend > mode and I could convey properly everything I had in mind. > > Cheers, > Quentin > > _______________________________________________ > Linux-rockchip mailing list > Linux-rockchip@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-rockchip
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
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 = <®_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 |
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
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 |
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
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
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(+)