Hi,

Am Donnerstag, 11. August 2022, 10:45:07 CEST schrieb Quentin Schulz:

Hi Michael,

On 8/11/22 09:52, Michael Riesch wrote:

quoted

Hi Quentin,

Thank you for your efforts! This will solve several issues that are
bound to pop up if a board deviates from the Rockchip reference design.

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

quoted

quoted

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

...allows to model explicitly...?

quoted

supplies and IO domains[2]. This makes sure the regulators are enabled
by the Linux kernel so the IO domains are supplied with power and
correctly configured as per the supplied voltage.
This driver is a regulator consumer and does not offer any other
interface for device dependency.

However, IO pins belonging to an IO domain need to have this IO domain
correctly configured before they are being used otherwise they do not
operate correctly (in our case, a pin configured as output clock was
oscillating between 0 and 150mV instead of the expected 1V8).

In order to make this dependency transparent to the consumer of those
pins and not add Rockchip-specific code to third party drivers (a camera
driver in our case), it is hooked into the pinctrl driver which is
Rockchip-specific obviously.

This approach seems reasonable. But just for my understanding: Does this
mean we need to edit e.g. rk3568-pinctrl.dtsi, iterate over all entries,
and add rockchip,iodomains = <&corresponding_io_domain>;?

That would have been my hope yes, but it is not possible for one of the 
boards we have based on PX30.

All pinmux listed in the px30.dtsi today belong to an IO domain. This 
includes the I2C pins for the bus on which the PMIC is.
Adding the rockchip,io-domains on each pinctrl will create the following 
circular dependency:
pinctrl depends on the io-domain device which depends on
regulators from a PMIC on i2c which requires the i2c bus pins to be
muxed from the pinctrl

Since the PMIC powering the IO domains can virtually be on any I2C bus, 
we cannot add it to the main SoC.dtsi, it'll need to be added per board 
sadly.

though you could also add the main props to the dtsi and use a per-board
/delete-property/ to free up the pmic-i2c, same result but less duplicate
dt additions and less clutter.

quoted

quoted

diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c
index 32e41395fc76..c3c2801237b5 100644
--- a/drivers/pinctrl/pinctrl-rockchip.c
+++ b/drivers/pinctrl/pinctrl-rockchip.c

@@ -24,6 +24,8 @@
  #include <linux/of_address.h>
  #include <linux/of_device.h>
  #include <linux/of_irq.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
  #include <linux/pinctrl/machine.h>
  #include <linux/pinctrl/pinconf.h>
  #include <linux/pinctrl/pinctrl.h>

@@ -2370,6 +2372,12 @@ static int rockchip_pmx_set(struct pinctrl_dev *pctldev, unsigned selector,
  	dev_dbg(dev, "enable function %s group %s\n",
  		info->functions[selector].name, info->groups[group].name);
  
+	if (info->groups[group].io_domain &&
+	    !platform_get_drvdata(info->groups[group].io_domain)) {
+		dev_err(info->dev, "IO domain device is required but not probed yet, deferring...");

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

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

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

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

quoted

quoted

@@ -2684,6 +2693,16 @@ static int rockchip_pinctrl_parse_groups(struct device_node *np,
  	if (!size || size % 4)
  		return dev_err_probe(dev, -EINVAL, "wrong pins number or pins and configs should be by 4\n");
  
+	node = of_parse_phandle(np, "rockchip,io-domains", 0);
+	if (node) {
+		grp->io_domain = of_find_device_by_node(node);
+		of_node_put(node);
+		if (!grp->io_domain) {
+			dev_err(info->dev, "couldn't find IO domain device\n");
+			return -ENODEV;

Again just for my understanding: The property is optional in order to
provide compatibility with older device trees, right?

Of course (at least that's the intent). If it is omitted, 
of_parse_phandle will return NULL and we'll not be executing this part 
of the code. However, if one phandle is provided and the device does not 
actually exist (IIUC, the phandle points to a DT-valid node but the 
device pointed at by the phandle is either disabled or its driver is not 
built). That being said, I don't know how this would work with an IO 
domain driver built as a module. That would be a pretty dumb thing to do 
though.

I think this should work even with io-domain "disabled" or as module
when slightly modified.

I.e. for disabled nodes, no kernel-device should be created
(grp->io_domain will be NULL) and for a module the device itself is created
when the dt is parsed (of_populate...) and will just not have probed yet.

Together with the comment farther above of having the io-domain link always
present we should get rid of the error condition though :-) .



Hmm, while going through this one thought was, do we want more verbosity
in the dt for this?

I.e. with the current approach we'll have

&io_domains {
	status = "okay";

	audio-supply = <&pp1800_audio>;
	bt656-supply = <&pp1800_ap_io>;
	gpio1830-supply = <&pp3000_ap>;
	sdmmc-supply = <&ppvar_sd_card_io>;
};

and pinctrl entries linking to the core <&io_domains> node. This might bite
us down the road again in some form.

Something like doing an optional updated binding like:

&io_domains {
	status = "okay";

	audio-domain {
		domain-supply = <&pp1800_audio>;
	};
	bt656-domain {
		domain-supply = <&pp1800_ap_io>;
	};
	gpio1830-domain {
		domain-supply = <&pp3000_ap>;
	};
	sdmmc-domain {
		domain-supply = <&ppvar_sd_card_io>;
	};
};

       pcie {
               pcie_ep_gpio: pci-ep-gpio {
                       rockchip,pins =
                               <4 RK_PC6 RK_FUNC_GPIO &pcfg_pull_none>;
                       rockchip,io-domains = <&gpio1830_domain>;
               };
       };


I.e. linking the pin-set to a definition of its actual io-domain, instead
of only the general io-domain node. Somewhat similar to power-domains.

The code itself could be the same as now (except needing to get the parent
of the linked node for the io-domains), but would leave us the option of
modifying code behaviour without touching the binding if needed down the
road.


Heiko



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel