pinctrl: msm: fix gpio-hog related boot issues

Sven Eckelmann reported an issue with the current IPQ4019 pinctrl. Setting up any gpio-hog in the device-tree for his device would "kill the bootup completely":

| [    0.477838] msm_serial 78af000.serial: could not find pctldev for node /soc/pinctrl at 1000000/serial_pinmux, deferring probe
| [    0.499828] spi_qup 78b5000.spi: could not find pctldev for node /soc/pinctrl at 1000000/spi_0_pinmux, deferring probe
| [    1.298883] requesting hog GPIO enable USB2 power (chip 1000000.pinctrl, offset 58) failed, -517
| [    1.299609] gpiochip_add_data: GPIOs 0..99 (1000000.pinctrl) failed to register
| [    1.308589] ipq4019-pinctrl 1000000.pinctrl: Failed register gpiochip
| [    1.316586] msm_serial 78af000.serial: could not find pctldev for node /soc/pinctrl at 1000000/serial_pinmux, deferring probe
| [    1.322415] spi_qup 78b5000.spi: could not find pctldev for node /soc/pinctrl at 1000000/spi_0_pinmux, deferri

This was also verified on a RT-AC58U (IPQ4018) which would no longer boot, if a gpio-hog was specified. (Tried forcing the USB LED PIN (GPIO0) to high.).

Sven Eckelmann reported an issue with the current IPQ4019 pinctrl. Setting up any gpio-hog in the device-tree for his device would "kill the bootup completely":

| [    0.477838] msm_serial 78af000.serial: could not find pctldev for node /soc/pinctrl at 1000000/serial_pinmux, deferring probe
| [    0.499828] spi_qup 78b5000.spi: could not find pctldev for node /soc/pinctrl at 1000000/spi_0_pinmux, deferring probe
| [    1.298883] requesting hog GPIO enable USB2 power (chip 1000000.pinctrl, offset 58) failed, -517
| [    1.299609] gpiochip_add_data: GPIOs 0..99 (1000000.pinctrl) failed to register
| [    1.308589] ipq4019-pinctrl 1000000.pinctrl: Failed register gpiochip
| [    1.316586] msm_serial 78af000.serial: could not find pctldev for node /soc/pinctrl at 1000000/serial_pinmux, deferring probe
| [    1.322415] spi_qup 78b5000.spi: could not find pctldev for node /soc/pinctrl at 1000000/spi_0_pinmux, deferri

This was also verified on a RT-AC58U (IPQ4018) which would no longer boot, if a gpio-hog was specified. (Tried forcing the USB LED PIN (GPIO0) to high.).

The problem is that Pinctrl+GPIO registration is currently peformed in the following order in pinctrl-msm.c:
1. pinctrl_register()
2. gpiochip_add()
3. gpiochip_add_pin_range()

The actual error code -517 == -EPROBE_DEFER is coming from pinctrl_get_device_gpio_range(), which is called through:

        gpiochip_add
            of_gpiochip_add
                of_gpiochip_scan_gpios
                    gpiod_hog
                        gpiochip_request_own_desc
                            __gpiod_request
                                chip->request
                                    gpiochip_generic_request
                                       pinctrl_gpio_request
                                          pinctrl_get_device_gpio_range

pinctrl_get_device_gpio_range() is unable to find any valid pin ranges, since nothing has been added to the pinctrldev_list yet. so the range can't be found, and the operation fails with -EPROBE_DEFER.

This patch fixes the issue by adding the "gpio-ranges" property to the pinctrl device node of all upstream Qcom SoC. The pin ranges are then added by the gpio core.

In order to remain compatible with older, existing DTs (and ACPI) a check for the "gpio-ranges" property has been added to msm_gpio_init(). This prevents the driver of adding the same entry to the pinctrldev_list twice.

Reported-by Sven Eckelmann <[email protected]>

Signed-off-by Christian Lamparter <[email protected]>

Quoted 3 lines · Christian Lamparter

Sven Eckelmann reported an issue with the current IPQ4019 pinctrl. Setting up any gpio-hog in the device-tree for his device would "kill the bootup completely":

Quoted 2 lines

Reported-by Sven Eckelmann <[email protected]>

Signed-off-by Christian Lamparter <[email protected]>

This patch looks VERY good to me.

I am fine in merging it into pin control including the ranges fixes.

I need Andy's and Bjorn's ACKs on it though!

Can you guys provide that?

Quoted 49 lines

Sven Eckelmann reported an issue with the current IPQ4019 pinctrl. Setting up any gpio-hog in the device-tree for his device would "kill the bootup completely":

| [    0.477838] msm_serial 78af000.serial: could not find pctldev for node /soc/pinctrl at 1000000/serial_pinmux, deferring probe
| [    0.499828] spi_qup 78b5000.spi: could not find pctldev for node /soc/pinctrl at 1000000/spi_0_pinmux, deferring probe
| [    1.298883] requesting hog GPIO enable USB2 power (chip 1000000.pinctrl, offset 58) failed, -517
| [    1.299609] gpiochip_add_data: GPIOs 0..99 (1000000.pinctrl) failed to register
| [    1.308589] ipq4019-pinctrl 1000000.pinctrl: Failed register gpiochip
| [    1.316586] msm_serial 78af000.serial: could not find pctldev for node /soc/pinctrl at 1000000/serial_pinmux, deferring probe
| [    1.322415] spi_qup 78b5000.spi: could not find pctldev for node /soc/pinctrl at 1000000/spi_0_pinmux, deferri

This was also verified on a RT-AC58U (IPQ4018) which would no longer boot, if a gpio-hog was specified. (Tried forcing the USB LED PIN (GPIO0) to high.).

The problem is that Pinctrl+GPIO registration is currently peformed in the following order in pinctrl-msm.c:
1. pinctrl_register()
2. gpiochip_add()
3. gpiochip_add_pin_range()

The actual error code -517 == -EPROBE_DEFER is coming from pinctrl_get_device_gpio_range(), which is called through:

        gpiochip_add
            of_gpiochip_add
                of_gpiochip_scan_gpios
                    gpiod_hog
                        gpiochip_request_own_desc
                            __gpiod_request
                                chip->request
                                    gpiochip_generic_request
                                       pinctrl_gpio_request
                                          pinctrl_get_device_gpio_range

pinctrl_get_device_gpio_range() is unable to find any valid pin ranges, since nothing has been added to the pinctrldev_list yet. so the range can't be found, and the operation fails with -EPROBE_DEFER.

This patch fixes the issue by adding the "gpio-ranges" property to the pinctrl device node of all upstream Qcom SoC. The pin ranges are then added by the gpio core.

In order to remain compatible with older, existing DTs (and ACPI) a check for the "gpio-ranges" property has been added to msm_gpio_init(). This prevents the driver of adding the same entry to the pinctrldev_list twice.

Quoted 2 lines

Reported-by Sven Eckelmann <[email protected]>

Signed-off-by Christian Lamparter <[email protected]>

This patch looks VERY good to me.

Quoting Linus Walleij (2018-04-26 05:03:45)

Quoted 53 lines · Christian Lamparter

Quoted 51 lines

Sven Eckelmann reported an issue with the current IPQ4019 pinctrl. Setting up any gpio-hog in the device-tree for his device would "kill the bootup completely":

| [    0.477838] msm_serial 78af000.serial: could not find pctldev for node /soc/pinctrl at 1000000/serial_pinmux, deferring probe
| [    0.499828] spi_qup 78b5000.spi: could not find pctldev for node /soc/pinctrl at 1000000/spi_0_pinmux, deferring probe
| [    1.298883] requesting hog GPIO enable USB2 power (chip 1000000.pinctrl, offset 58) failed, -517
| [    1.299609] gpiochip_add_data: GPIOs 0..99 (1000000.pinctrl) failed to register
| [    1.308589] ipq4019-pinctrl 1000000.pinctrl: Failed register gpiochip
| [    1.316586] msm_serial 78af000.serial: could not find pctldev for node /soc/pinctrl at 1000000/serial_pinmux, deferring probe
| [    1.322415] spi_qup 78b5000.spi: could not find pctldev for node /soc/pinctrl at 1000000/spi_0_pinmux, deferri

This was also verified on a RT-AC58U (IPQ4018) which would no longer boot, if a gpio-hog was specified. (Tried forcing the USB LED PIN (GPIO0) to high.).

The problem is that Pinctrl+GPIO registration is currently peformed in the following order in pinctrl-msm.c:
1. pinctrl_register()
2. gpiochip_add()
3. gpiochip_add_pin_range()

The actual error code -517 == -EPROBE_DEFER is coming from pinctrl_get_device_gpio_range(), which is called through:

        gpiochip_add
            of_gpiochip_add
                of_gpiochip_scan_gpios
                    gpiod_hog
                        gpiochip_request_own_desc
                            __gpiod_request
                                chip->request
                                    gpiochip_generic_request
                                       pinctrl_gpio_request
                                          pinctrl_get_device_gpio_range

pinctrl_get_device_gpio_range() is unable to find any valid pin ranges, since nothing has been added to the pinctrldev_list yet. so the range can't be found, and the operation fails with -EPROBE_DEFER.

This patch fixes the issue by adding the "gpio-ranges" property to the pinctrl device node of all upstream Qcom SoC. The pin ranges are then added by the gpio core.

In order to remain compatible with older, existing DTs (and ACPI) a check for the "gpio-ranges" property has been added to msm_gpio_init(). This prevents the driver of adding the same entry to the pinctrldev_list twice.

Reported-by Sven Eckelmann <[email protected]>

Signed-off-by Christian Lamparter <[email protected]>

Quoted 2 lines

This patch looks VERY good to me.

Why can't we register the gpiochip and tell it about the pin ranges in one API call instead of adding the chip and then adding the ranges? It doesn't look right to have to go and update all the DT nodes to list this information that is already known in the driver because the kernel implementation can't handle the order of operations correctly.

Furthermore, it looks like this becomes a silent requirement to add the gpio-ranges property into the DT so that hogs work, but none of the bindings have been updated in this patch to indicate that.

Quoted 5 lines

Why can't we register the gpiochip and tell it about the pin ranges in one API call instead of adding the chip and then adding the ranges? It doesn't look right to have to go and update all the DT nodes to list this information that is already known in the driver because the kernel implementation can't handle the order of operations correctly.

The problem is that gpiochip_add_pin_range() was not intended for DT-based pinctrls... but it got used anyway.

This topic came up in an earlier post:
"Re: pinctrl: qcom: ipq4019: Use of gpio-hog's" [0] (you must have gotten this mail too, since you are on the Cc.) which links to a ML thread titled "Re: [GIT PULL] SPEAr pinctrl updates for v-3.5"

For your convenience: (this post is from 2012-09-03 - so it's 5-6 years old by now and it looks like it predates even the DT pinctrl-msm driver. (Not entirely sure?))
<http://thread.gmane.org/gmane.linux.ports.arm.kernel/184943>

|[...]
|But I want two similar function named:
|
|gpiochip_add_pin_range();
|gpiochip_remove_pin_range();
|
|*that can be used for platforms that doesn't support DT.*
|
|For example I'd like to convert over some of my existing
|drivers that do not have DT support to do this thing instead
|of registering ranges from the pin controller...

I think you must have come across similar issues with the "gpio-reserved-ranges" property you recently added. Because
from what I can glimpse from the
"[2/3] dt-bindings: pinctrl: Add a ngpios-ranges property"
<https://patchwork.kernel.org/patch/10153785/> series.
The gpio-reserved-ranges property would also need to be added
to existing products (msm8996) as well, right?
("I stuck this inside msm8996, but maybe it can go somewhere more generic?")

Quoted 4 lines

Furthermore, it looks like this becomes a silent requirement to add the gpio-ranges property into the DT so that hogs work, but none of the bindings have been updated in this patch to indicate that.

The pinctrl-msm.c driver will fallback to using gpiochip_add_pin_range(), if the gpio-ranges property is not present. So all existing and compiled devicetree binaries files will remain compatible.

As for adding the gpio-ranges to the dt binding text files under Documentation/devicetree/bindings/pinctrl/: Sure. No problem. I can add them too :).

But I do have a question: Should I also include the missing declaration of the gpio-reserved-ranges properties too? (I can make the patches over the long weekend. If I hear nothing from anyone, I'll post them on Monday).

Quoting Christian Lamparter (2018-05-16 13:29:48)

Quoted 6 lines · CEST Stephen Boyd

Quoted 5 lines

Why can't we register the gpiochip and tell it about the pin ranges in one API call instead of adding the chip and then adding the ranges? It doesn't look right to have to go and update all the DT nodes to list this information that is already known in the driver because the kernel implementation can't handle the order of operations correctly.

Quoted 2 lines

The problem is that gpiochip_add_pin_range() was not intended for DT-based pinctrls... but it got used anyway.

Are there more users of this on DT based systems? A quick grep shows a couple more potential failures, like the qcom based SPMI gpio controllers and a mediatek one.

It's almost like we should print a huge WARN_ON() if gpio_chip::of_node is non-NULL and gpochip_add_pin_range() is called. But that probably would be noisy and can't be fixed on older DT blobs. It may also be good to bail out of that function if the node pointer exists and the property is there in the node so that we don't have to go update each driver for the backwards compat mode like was done in this patch. Plus the function should get some sort of comment that calling it is not useful on DT based platforms so this is all documented.

Quoted 4 lines

This topic came up in an earlier post:
"Re: pinctrl: qcom: ipq4019: Use of gpio-hog's" [0] (you must have gotten this mail too, since you are on the Cc.) which links to a ML thread titled "Re: [GIT PULL] SPEAr pinctrl updates for v-3.5"

I get quite a bit of email as you can tell.

Quoted 17 lines

For your convenience: (this post is from 2012-09-03 - so it's 5-6 years old by now and it looks like it predates even the DT pinctrl-msm driver. (Not entirely sure?))
<http://thread.gmane.org/gmane.linux.ports.arm.kernel/184943>

|[...]
|But I want two similar function named:
|
|gpiochip_add_pin_range();
|gpiochip_remove_pin_range();
|
|*that can be used for platforms that doesn't support DT.*
|
|For example I'd like to convert over some of my existing
|drivers that do not have DT support to do this thing instead
|of registering ranges from the pin controller...

Quoted 8 lines

I think you must have come across similar issues with the "gpio-reserved-ranges" property you recently added. Because
from what I can glimpse from the
"[2/3] dt-bindings: pinctrl: Add a ngpios-ranges property"
<https://patchwork.kernel.org/patch/10153785/> series.
The gpio-reserved-ranges property would also need to be added
to existing products (msm8996) as well, right?
("I stuck this inside msm8996, but maybe it can go somewhere more generic?")

The gpio-reserved-ranges only affects some SoCs. It should be added to the bindings on whatever chips are affected by those firmware quirks as optional properties. It would be great if you could add it to the ones that may need it. My guess is that it only matters for the pin controllers that spread out each pin into a big range of I/O memory because otherwise pins aren't locked away from non-secure systems.

Quoted 7 lines

Quoted 4 lines

Furthermore, it looks like this becomes a silent requirement to add the gpio-ranges property into the DT so that hogs work, but none of the bindings have been updated in this patch to indicate that.

The pinctrl-msm.c driver will fallback to using gpiochip_add_pin_range(), if the gpio-ranges property is not present. So all existing and compiled devicetree binaries files will remain compatible.

That's good.

Quoted 4 lines

As for adding the gpio-ranges to the dt binding text files under Documentation/devicetree/bindings/pinctrl/: Sure. No problem. I can add them too :).

Great!

Quoted 1 line

Quoted 3 lines

But I do have a question: Should I also include the missing declaration of the gpio-reserved-ranges properties too? (I can make the patches over the long weekend. If I hear nothing from anyone, I'll post them on Monday).

Sure. Do you have the list of pinctrl devices that may need the gpio-reserved-ranges property?

arch/arm64/boot/dts/qcom/msm8996.dtsi +0 -0

 			#gpio-cells = <2>;
 			interrupt-controller;
 			#interrupt-cells = <2>;

drivers/pinctrl/qcom/pinctrl-msm.c +9 -5

index e7abc8ba222b..ed889553f01c 100644
@@ -890,11 +890,24 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
 		return ret;
 	}
 
-	ret = gpiochip_add_pin_range(&pctrl->chip, dev_name(pctrl->dev), 0, 0, chip->ngpio);
-	if (ret) {
-		dev_err(pctrl->dev, "Failed to add pin range\n");
-		gpiochip_remove(&pctrl->chip);
-		return ret;
+	/*
+	 * For DeviceTree-supported systems, the gpio core checks the
+	 * pinctrl's device node for the "gpio-ranges" property.
+	 * If it is present, it takes care of adding the pin ranges
+	 * for the driver. In this case the driver can skip ahead.
+	 *
+	 * In order to remain compatible with older, existing DeviceTree
+	 * files which don't set the "gpio-ranges" property or systems that
+	 * utilize ACPI the driver has to call gpiochip_add_pin_range().

arch/arm64/boot/dts/qcom/msm8996.dtsi line 533

Quoted 9 lines · Christian Lamparter

arch/arm64/boot/dts/qcom/msm8996.dtsi +1 -0

index 0a6f7952bbb1..18511e782cbd 100644
@@ -530,6 +530,7 @@
 			reg = <0x01010000 0x300000>;
 			interrupts = <GIC_SPI 208 IRQ_TYPE_LEVEL_HIGH>;
 			gpio-controller;
+			gpio-ranges = <&msmgpio 0 0 150>;

I'm still confused to why this information is in DT at all, it feels like an implementation detail, not a system configuration thing.

Quoted 26 lines

arch/arm64/boot/dts/qcom/msm8996.dtsi +0 -0

 			#gpio-cells = <2>;
 			interrupt-controller;
 			#interrupt-cells = <2>;

drivers/pinctrl/qcom/pinctrl-msm.c +9 -5

index e7abc8ba222b..ed889553f01c 100644
@@ -890,11 +890,24 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
 		return ret;
 	}
 
-	ret = gpiochip_add_pin_range(&pctrl->chip, dev_name(pctrl->dev), 0, 0, chip->ngpio);
-	if (ret) {
-		dev_err(pctrl->dev, "Failed to add pin range\n");
-		gpiochip_remove(&pctrl->chip);
-		return ret;
+	/*
+	 * For DeviceTree-supported systems, the gpio core checks the
+	 * pinctrl's device node for the "gpio-ranges" property.
+	 * If it is present, it takes care of adding the pin ranges
+	 * for the driver. In this case the driver can skip ahead.
+	 *
+	 * In order to remain compatible with older, existing DeviceTree
+	 * files which don't set the "gpio-ranges" property or systems that
+	 * utilize ACPI the driver has to call gpiochip_add_pin_range().

drivers/pinctrl/qcom/pinctrl-msm.c static int msm_gpio_init(struct msm_pinctrl *pctrl) · line 911

Quoted 10 lines

drivers/pinctrl/qcom/pinctrl-msm.c +9 -0

+	 */
+	if (!of_property_read_bool(pctrl->dev->of_node, "gpio-ranges")) {
+		ret = gpiochip_add_pin_range(&pctrl->chip,
+			dev_name(pctrl->dev), 0, 0, chip->ngpio);
+		if (ret) {
+			dev_err(pctrl->dev, "Failed to add pin range\n");
+			gpiochip_remove(&pctrl->chip);
+			return ret;
+		}
 	}

The patch looks good, but I would like you to split it in DT and pinctrl parts, to make it less likely to collide and to allow Andy to inject the missing change of sdm845.dtsi (which is now in linux-next)

Please split it and add my

Reviewed-by Bjorn Andersson <[email protected]>

to both patches.

Quoted 4 lines · CEST Bjorn Andersson

Quoted 1 line

+ gpio-ranges = <&msmgpio 0 0 150>;

I'm still confused to why this information is in DT at all, it feels like an implementation detail, not a system configuration thing.

I did look at the commits and code from back in 2013. From what I can gather "this implementation detail" was realized the way it is now, because "devicetree was the new thing" and it seemed like a good idea to make it as extendable/generic as possible.