| Message ID | 20180328180705.29147-1-chunkeey@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed 28 Mar 11:07 PDT 2018, Christian Lamparter wrote: > 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@1000000/serial_pinmux, deferring probe > | [ 0.499828] spi_qup 78b5000.spi: could not find pctldev for node /soc/pinctrl@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@1000000/serial_pinmux, deferring probe > | [ 1.322415] spi_qup 78b5000.spi: could not find pctldev for node /soc/pinctrl@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.). > It's quite clear that this is a generic issue with the msm pinctrl driver. For the cases where I've been in need of static configuration of certain pins I've simply made the pinctrl node itself have a pinctrl-0 that refer to a state that I want to hold. This has worked well and I didn't even know about the gpio-hog property. [..] > diff --git a/arch/arm/boot/dts/qcom-apq8064.dtsi b/arch/arm/boot/dts/qcom-apq8064.dtsi > index 3ca96e361878..17ad9cbd9f8c 100644 > --- a/arch/arm/boot/dts/qcom-apq8064.dtsi > +++ b/arch/arm/boot/dts/qcom-apq8064.dtsi > @@ -327,6 +327,7 @@ > reg = <0x800000 0x4000>; > > gpio-controller; > + gpio-ranges = <&tlmm_pinmux 0 0 90>; This seems reasonable. > #gpio-cells = <2>; > interrupt-controller; > #interrupt-cells = <2>; [..] > diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c > index 495432f3341b..f2580bbba741 100644 > --- a/drivers/pinctrl/qcom/pinctrl-msm.c > +++ b/drivers/pinctrl/qcom/pinctrl-msm.c > @@ -831,13 +831,6 @@ 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; > - } > - But, this will break backwards compatibility with existing DTBs and I don't see how this would work with the ACPI based platforms using this driver. Iirc this driver follows the same pattern as several other pinctrl drivers providing gpios, how are they handling this? Regards, Bjorn
On Thu 29 Mar 2018 02:27:23 CEST Bjorn Andersson wrote: > On Wed 28 Mar 11:07 PDT 2018, Christian Lamparter wrote: > > > 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@1000000/serial_pinmux, deferring probe > > | [ 0.499828] spi_qup 78b5000.spi: could not find pctldev for node /soc/pinctrl@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@1000000/serial_pinmux, deferring probe > > | [ 1.322415] spi_qup 78b5000.spi: could not find pctldev for node /soc/pinctrl@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.). > > > > It's quite clear that this is a generic issue with the msm pinctrl > driver. From what I understand it's not only the msm-pinctrl. All pinctrl and gpio drivers that support a DT binding but use gpiochip_add_pin_range as the sole way to add the ranges. I made a (probably incomplete) list in the thread by Sven: <https://lkml.kernel.org/r/46130418.GjRpGRXmAF@debian64> > For the cases where I've been in need of static configuration of > certain pins I've simply made the pinctrl node itself have a pinctrl-0 > that refer to a state that I want to hold. This has worked well and I > didn't even know about the gpio-hog property. yes, that will work too. One advantage of the gpio-hog is that it will also request the gpio properly. So it cannot be exported (and changed) without getting rid of the gpio-hog first. It also allows to specify a user-friendly line-name that shows up in various places. i.e.: |root@mbl:/# cat /sys/kernel/debug/gpio |gpiochip1: GPIOs 472-479, parent: platform/4e0000000.gpio1, 4e0000000.gpio1: | gpio-472 ( |enable EMAC PHY ) out hi | gpio-475 ( |Power Drive Port 1 ) out hi | > [..] > > diff --git a/arch/arm/boot/dts/qcom-apq8064.dtsi b/arch/arm/boot/dts/qcom-apq8064.dtsi > > index 3ca96e361878..17ad9cbd9f8c 100644 > > --- a/arch/arm/boot/dts/qcom-apq8064.dtsi > > +++ b/arch/arm/boot/dts/qcom-apq8064.dtsi > > @@ -327,6 +327,7 @@ > > reg = <0x800000 0x4000>; > > > > gpio-controller; > > + gpio-ranges = <&tlmm_pinmux 0 0 90>; > > This seems reasonable. > > > #gpio-cells = <2>; > > interrupt-controller; > > #interrupt-cells = <2>; > [..] > > diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c > > index 495432f3341b..f2580bbba741 100644 > > --- a/drivers/pinctrl/qcom/pinctrl-msm.c > > +++ b/drivers/pinctrl/qcom/pinctrl-msm.c > > @@ -831,13 +831,6 @@ 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; > > - } > > - > > But, this will break backwards compatibility with existing DTBs and I > don't see how this would work with the ACPI based platforms using this > driver. Ok I see, I was aware of ACPI but not that a pinctrl-msm based driver is using it. Well, I thinks is possible to use is_acpi_device_node() or !is_of_node() to detect whenever we are dealing with a OF or not: would it be ok to do something like this? | if (!is_of_node(chip->of_node)) { | /* | * (lengthy note about gpiochip_add_pin_range and OF with | * reference to Documentation/devicetree/bindings/gpio/gpio.txt | * - TBD) | */ | ret = gpiochip_add_pin_range(&pctrl->chip, | [...] | } > Iirc this driver follows the same pattern as several other pinctrl > drivers providing gpios, how are they handling this? Well, my commit message was based on a similar patch done by Geert Uytterhoeven <geert+renesas@glider.be> for the sh73a0: <https://marc.info/?l=git-commits-head&m=144114029919118&w=2> Another one was this patch from Linus: <https://patches.linaro.org/patch/51382/> and there are many more (basically git blame on every .dts* in arch/ that already has a gpio-ranges property.) Regards, Christian
On Thu 29 Mar 2018 14:23:35 CEST Christian Lamparter wrote: > Ok I see, I was aware of ACPI but not that a pinctrl-msm based driver is > using it. Well, I thinks is possible to use is_acpi_device_node() or > !is_of_node() to detect whenever we are dealing with a OF or not: > > would it be ok to do something like this? > > | if (!is_of_node(chip->of_node)) { oops, this should be: if (!is_of_node(pctrl->dev->fwnode)) { > | /* > | * (lengthy note about gpiochip_add_pin_range and OF with > | * reference to Documentation/devicetree/bindings/gpio/gpio.txt > | * - TBD) > | */ > | ret = gpiochip_add_pin_range(&pctrl->chip, > | [...] > | }
On Wed, Mar 28, 2018 at 8:07 PM, Christian Lamparter <chunkeey@gmail.com> wrote: > This patch fixes the issue by adding the "gpio-ranges" property > to the pinctrl device node of all upstream Qcom SoC, so the > ranges are added by of_gpiochip_add_pin_range(), which is > called by of_gpiochip_add() before the call to > of_gpiochip_scan_gpios() happens.gpiochip_add_pin_range() is longer > needed and removed (to prevent adding the same entry to the > pinctrldev_list twice). That's pretty neat! > gpio-controller; > + gpio-ranges = <&tlmm_pinmux 0 0 90>; nice! > - 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; > - } If you instead of deleteing this, just wrap it inside something like: if (!of_property_read_bool(np, "gpio-ranges") { (...) } You will stay compatible with elder device trees, solving Björns issue. You will only be adding hogs to newer device trees with the ranges defined anyway. Be genereous with comments in the code if you choose this approach so everyone realize what is going on. Yours, Linus Walleij
On Donnerstag, 12. April 2018 14:48:56 CEST Linus Walleij wrote: > On Wed, Mar 28, 2018 at 8:07 PM, Christian Lamparter <chunkeey@gmail.com> wrote: > > > - 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; > > - } > > If you instead of deleteing this, just wrap it inside > something like: > > if (!of_property_read_bool(np, "gpio-ranges") { > (...) > } > > You will stay compatible with elder device trees, solving Björns > issue. You will only be adding hogs to newer device trees with > the ranges defined anyway. > > Be genereous with comments in the code if you choose this > approach so everyone realize what is going on. Thank you for your insightful advice. I just sent out v4 which goes the of_property_read_bool route. Let's wait and see what kbuilt-bot has to say. Best Regards, Christian
diff --git a/arch/arm/boot/dts/qcom-apq8064.dtsi b/arch/arm/boot/dts/qcom-apq8064.dtsi index 3ca96e361878..17ad9cbd9f8c 100644 --- a/arch/arm/boot/dts/qcom-apq8064.dtsi +++ b/arch/arm/boot/dts/qcom-apq8064.dtsi @@ -327,6 +327,7 @@ reg = <0x800000 0x4000>; gpio-controller; + gpio-ranges = <&tlmm_pinmux 0 0 90>; #gpio-cells = <2>; interrupt-controller; #interrupt-cells = <2>; diff --git a/arch/arm/boot/dts/qcom-apq8084.dtsi b/arch/arm/boot/dts/qcom-apq8084.dtsi index 0e1e98707e3f..d9481d083802 100644 --- a/arch/arm/boot/dts/qcom-apq8084.dtsi +++ b/arch/arm/boot/dts/qcom-apq8084.dtsi @@ -396,6 +396,7 @@ compatible = "qcom,apq8084-pinctrl"; reg = <0xfd510000 0x4000>; gpio-controller; + gpio-ranges = <&tlmm 0 0 147>; #gpio-cells = <2>; interrupt-controller; #interrupt-cells = <2>; diff --git a/arch/arm/boot/dts/qcom-ipq4019.dtsi b/arch/arm/boot/dts/qcom-ipq4019.dtsi index c1e65aaf3cad..d85b15774c64 100644 --- a/arch/arm/boot/dts/qcom-ipq4019.dtsi +++ b/arch/arm/boot/dts/qcom-ipq4019.dtsi @@ -147,6 +147,7 @@ compatible = "qcom,ipq4019-pinctrl"; reg = <0x01000000 0x300000>; gpio-controller; + gpio-ranges = <&tlmm 0 0 100>; #gpio-cells = <2>; interrupt-controller; #interrupt-cells = <2>; diff --git a/arch/arm/boot/dts/qcom-ipq8064.dtsi b/arch/arm/boot/dts/qcom-ipq8064.dtsi index 1e0a3b446f7a..26eab9a68d90 100644 --- a/arch/arm/boot/dts/qcom-ipq8064.dtsi +++ b/arch/arm/boot/dts/qcom-ipq8064.dtsi @@ -108,6 +108,7 @@ reg = <0x800000 0x4000>; gpio-controller; + gpio-ranges = <&qcom_pinmux 0 0 69>; #gpio-cells = <2>; interrupt-controller; #interrupt-cells = <2>; diff --git a/arch/arm/boot/dts/qcom-mdm9615.dtsi b/arch/arm/boot/dts/qcom-mdm9615.dtsi index c852b69229c9..cfdaca5f259a 100644 --- a/arch/arm/boot/dts/qcom-mdm9615.dtsi +++ b/arch/arm/boot/dts/qcom-mdm9615.dtsi @@ -128,6 +128,7 @@ msmgpio: pinctrl@800000 { compatible = "qcom,mdm9615-pinctrl"; gpio-controller; + gpio-ranges = <&msmgpio 0 0 88>; #gpio-cells = <2>; interrupts = <GIC_SPI 16 IRQ_TYPE_LEVEL_HIGH>; interrupt-controller; diff --git a/arch/arm/boot/dts/qcom-msm8660.dtsi b/arch/arm/boot/dts/qcom-msm8660.dtsi index 33030f9419fe..47cf9c4ca062 100644 --- a/arch/arm/boot/dts/qcom-msm8660.dtsi +++ b/arch/arm/boot/dts/qcom-msm8660.dtsi @@ -110,6 +110,7 @@ reg = <0x800000 0x4000>; gpio-controller; + gpio-ranges = <&tlmm 0 0 173>; #gpio-cells = <2>; interrupts = <0 16 0x4>; interrupt-controller; diff --git a/arch/arm/boot/dts/qcom-msm8960.dtsi b/arch/arm/boot/dts/qcom-msm8960.dtsi index 1733d8f40ab1..f6d8b1af5a8a 100644 --- a/arch/arm/boot/dts/qcom-msm8960.dtsi +++ b/arch/arm/boot/dts/qcom-msm8960.dtsi @@ -102,6 +102,7 @@ msmgpio: pinctrl@800000 { compatible = "qcom,msm8960-pinctrl"; gpio-controller; + gpio-ranges = <&msmgpio 0 0 152>; #gpio-cells = <2>; interrupts = <0 16 0x4>; interrupt-controller; diff --git a/arch/arm/boot/dts/qcom-msm8974.dtsi b/arch/arm/boot/dts/qcom-msm8974.dtsi index d9019a49b292..1250e071a6e2 100644 --- a/arch/arm/boot/dts/qcom-msm8974.dtsi +++ b/arch/arm/boot/dts/qcom-msm8974.dtsi @@ -696,6 +696,7 @@ compatible = "qcom,msm8974-pinctrl"; reg = <0xfd510000 0x4000>; gpio-controller; + gpio-ranges = <&msmgpio 0 0 146>; #gpio-cells = <2>; interrupt-controller; #interrupt-cells = <2>; diff --git a/arch/arm64/boot/dts/qcom/ipq8074.dtsi b/arch/arm64/boot/dts/qcom/ipq8074.dtsi index 2bc5dec5614d..d2c36b467466 100644 --- a/arch/arm64/boot/dts/qcom/ipq8074.dtsi +++ b/arch/arm64/boot/dts/qcom/ipq8074.dtsi @@ -24,11 +24,12 @@ ranges = <0 0 0 0xffffffff>; compatible = "simple-bus"; - pinctrl@1000000 { + tlmm: pinctrl@1000000 { compatible = "qcom,ipq8074-pinctrl"; reg = <0x1000000 0x300000>; interrupts = <GIC_SPI 208 IRQ_TYPE_LEVEL_HIGH>; gpio-controller; + gpio-ranges = <&tlmm 0 0 70>; #gpio-cells = <0x2>; interrupt-controller; #interrupt-cells = <0x2>; diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi index e51b04900726..e06cb90c8ec3 100644 --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi @@ -281,6 +281,7 @@ reg = <0x1000000 0x300000>; interrupts = <GIC_SPI 208 IRQ_TYPE_LEVEL_HIGH>; gpio-controller; + gpio-ranges = <&msmgpio 0 0 122>; #gpio-cells = <2>; interrupt-controller; #interrupt-cells = <2>; diff --git a/arch/arm64/boot/dts/qcom/msm8992.dtsi b/arch/arm64/boot/dts/qcom/msm8992.dtsi index 171578747ed0..173b6bc60816 100644 --- a/arch/arm64/boot/dts/qcom/msm8992.dtsi +++ b/arch/arm64/boot/dts/qcom/msm8992.dtsi @@ -179,6 +179,7 @@ reg = <0xfd510000 0x4000>; interrupts = <GIC_SPI 208 IRQ_TYPE_LEVEL_HIGH>; gpio-controller; + gpio-ranges = <&msmgpio 0 0 146>; #gpio-cells = <2>; interrupt-controller; #interrupt-cells = <2>; diff --git a/arch/arm64/boot/dts/qcom/msm8994.dtsi b/arch/arm64/boot/dts/qcom/msm8994.dtsi index f33c41d01c86..68705db4696b 100644 --- a/arch/arm64/boot/dts/qcom/msm8994.dtsi +++ b/arch/arm64/boot/dts/qcom/msm8994.dtsi @@ -141,6 +141,7 @@ reg = <0xfd510000 0x4000>; interrupts = <GIC_SPI 208 IRQ_TYPE_LEVEL_HIGH>; gpio-controller; + gpio-ranges = <&msmgpio 0 0 146>; #gpio-cells = <2>; interrupt-controller; #interrupt-cells = <2>; diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi b/arch/arm64/boot/dts/qcom/msm8996.dtsi index 0a6f7952bbb1..18511e782cbd 100644 --- a/arch/arm64/boot/dts/qcom/msm8996.dtsi +++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi @@ -530,6 +530,7 @@ reg = <0x01010000 0x300000>; interrupts = <GIC_SPI 208 IRQ_TYPE_LEVEL_HIGH>; gpio-controller; + gpio-ranges = <&msmgpio 0 0 150>; #gpio-cells = <2>; interrupt-controller; #interrupt-cells = <2>; diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c index 495432f3341b..f2580bbba741 100644 --- a/drivers/pinctrl/qcom/pinctrl-msm.c +++ b/drivers/pinctrl/qcom/pinctrl-msm.c @@ -831,13 +831,6 @@ 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; - } - ret = gpiochip_irqchip_add(chip, &msm_gpio_irq_chip, 0,
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@1000000/serial_pinmux, deferring probe | [ 0.499828] spi_qup 78b5000.spi: could not find pctldev for node /soc/pinctrl@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@1000000/serial_pinmux, deferring probe | [ 1.322415] spi_qup 78b5000.spi: could not find pctldev for node /soc/pinctrl@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, so the ranges are added by of_gpiochip_add_pin_range(), which is called by of_gpiochip_add() before the call to of_gpiochip_scan_gpios() happens.gpiochip_add_pin_range() is longer needed and removed (to prevent adding the same entry to the pinctrldev_list twice). Cc: stable@vger.kernel.org Reported-by: Sven Eckelmann <sven.eckelmann@openmesh.com> Signed-off-by: Christian Lamparter <chunkeey@gmail.com> --- The msm8998 would need something like gpio-ranges = <&tlmm 0 0 150>; --- arch/arm/boot/dts/qcom-apq8064.dtsi | 1 + arch/arm/boot/dts/qcom-apq8084.dtsi | 1 + arch/arm/boot/dts/qcom-ipq4019.dtsi | 1 + arch/arm/boot/dts/qcom-ipq8064.dtsi | 1 + arch/arm/boot/dts/qcom-mdm9615.dtsi | 1 + arch/arm/boot/dts/qcom-msm8660.dtsi | 1 + arch/arm/boot/dts/qcom-msm8960.dtsi | 1 + arch/arm/boot/dts/qcom-msm8974.dtsi | 1 + arch/arm64/boot/dts/qcom/ipq8074.dtsi | 3 ++- arch/arm64/boot/dts/qcom/msm8916.dtsi | 1 + arch/arm64/boot/dts/qcom/msm8992.dtsi | 1 + arch/arm64/boot/dts/qcom/msm8994.dtsi | 1 + arch/arm64/boot/dts/qcom/msm8996.dtsi | 1 + drivers/pinctrl/qcom/pinctrl-msm.c | 7 ------- 14 files changed, 14 insertions(+), 8 deletions(-)