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

From: Quentin Schulz
Date: Thu Aug 11 2022 - 04:45:54 EST

Hi Michael,

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

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

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

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

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

So, I think it mostly work by luck today.

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


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

A few comments inline:

On 8/2/22 11:52, Quentin Schulz wrote:
From: Quentin Schulz <quentin.schulz@xxxxxxxxxxxxxxxxxxxxx>

On some Rockchip SoCs, some SoC pins are split in what are called IO
domains.

An IO domain is supplied power externally, by regulators from a PMIC for
example. This external power supply is then used by the IO domain as
"supply" for the IO pins if they are outputs.

Each IO domain can configure which voltage the IO pins will be operating
on (1.8V or 3.3V).

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


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

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

...allows to model explicitly...?

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

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

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

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


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

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

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

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

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


In your board dts(i) :/

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

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

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

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


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

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

diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c
index 32e41395fc76..c3c2801237b5 100644
--- a/drivers/pinctrl/pinctrl-rockchip.c
+++ b/drivers/pinctrl/pinctrl-rockchip.c
@@ -24,6 +24,8 @@
#include <linux/of_address.h>
#include <linux/of_device.h>
#include <linux/of_irq.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
#include <linux/pinctrl/machine.h>
#include <linux/pinctrl/pinconf.h>
#include <linux/pinctrl/pinctrl.h>
@@ -2370,6 +2372,12 @@ static int rockchip_pmx_set(struct pinctrl_dev *pctldev, unsigned selector,
dev_dbg(dev, "enable function %s group %s\n",
info->functions[selector].name, info->groups[group].name);
+ if (info->groups[group].io_domain &&
+ !platform_get_drvdata(info->groups[group].io_domain)) {
+ dev_err(info->dev, "IO domain device is required but not probed yet, deferring...");

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


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

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

+ return -EPROBE_DEFER;
+ }
+
/*
* for each pin in the pin group selected, program the corresponding
* pin function number in the config register.
@@ -2663,6 +2671,7 @@ static int rockchip_pinctrl_parse_groups(struct device_node *np,
{
struct device *dev = info->dev;
struct rockchip_pin_bank *bank;
+ struct device_node *node;
int size;
const __be32 *list;
int num;
@@ -2684,6 +2693,16 @@ static int rockchip_pinctrl_parse_groups(struct device_node *np,
if (!size || size % 4)
return dev_err_probe(dev, -EINVAL, "wrong pins number or pins and configs should be by 4\n");
+ node = of_parse_phandle(np, "rockchip,io-domains", 0);
+ if (node) {
+ grp->io_domain = of_find_device_by_node(node);
+ of_node_put(node);
+ if (!grp->io_domain) {
+ dev_err(info->dev, "couldn't find IO domain device\n");
+ return -ENODEV;

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


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

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

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


Nothing prevents you from trying it out right now :)

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

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

Cheers,
Quentin