| Message ID | 20180412190138.12372-1-chunkeey@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Donnerstag, 12. April 2018 21:01:38 CEST 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.). [...] Sorry that I was so silent while you did all the work. I have applied your patch and now I see a [ 0.020619] GPIO line 58 (enable USB2 power) hogged as output/low when adding following node directly to the pinctrl enable-usb-power { gpio-hog; gpios = <58 GPIO_ACTIVE_HIGH>; output-low; line-name = "enable USB2 power"; }; (this looks at the first glance like it is deactivating USB by setting it to low but GPIO_ACTIVE_LOW would switch the meaning of output-low for gpio-hogs and the GPIO must really set to signal level low to enable USB) Tested-by: Sven Eckelmann <sven.eckelmann@openmesh.com> Thanks, Sven
On Thu, Apr 12, 2018 at 9:01 PM, Christian Lamparter <chunkeey@gmail.com> 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.). > > 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 <sven.eckelmann@openmesh.com> > Signed-off-by: Christian Lamparter <chunkeey@gmail.com> 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? It might be that Andy want the DTS changes separately to avoid any collisions, so I'm fine with splitting it as well. Yours, Linus Walleij
On Thu, Apr 26, 2018 at 2:03 PM, Linus Walleij <linus.walleij@linaro.org> wrote: > On Thu, Apr 12, 2018 at 9:01 PM, Christian Lamparter <chunkeey@gmail.com> 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": (...) >> Reported-by: Sven Eckelmann <sven.eckelmann@openmesh.com> >> Signed-off-by: Christian Lamparter <chunkeey@gmail.com> > > 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? Hm? Andy, Bjorn? Seems you guys are a bit snowed under. I guess I have to just apply it at some point and see what happens... Yours, Linus Walleij
Quoting Linus Walleij (2018-04-26 05:03:45) > On Thu, Apr 12, 2018 at 9:01 PM, Christian Lamparter <chunkeey@gmail.com> 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.). > > > > 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 <sven.eckelmann@openmesh.com> > > Signed-off-by: Christian Lamparter <chunkeey@gmail.com> > > 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.
On Wednesday, May 16, 2018 5:31:16 PM CEST Stephen Boyd wrote: > Quoting Linus Walleij (2018-04-26 05:03:45) > > On Thu, Apr 12, 2018 at 9:01 PM, Christian Lamparter <chunkeey@gmail.com> 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.). > > > > > > 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 <sven.eckelmann@openmesh.com> > > > Signed-off-by: Christian Lamparter <chunkeey@gmail.com> > > > > 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. 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?") > 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). (Also, it looks like the nvidia tegra pinctrl-driver has the gpio-ranges property in the DTS, but not in the binding documentation. I'll add that to the nvidia pile.) Thanks, Christian [0] <https://www.spinics.net/lists/linux-arm-msm/msg34833.html>
Quoting Christian Lamparter (2018-05-16 13:29:48) > On Wednesday, May 16, 2018 5:31:16 PM CEST Stephen Boyd wrote: > > 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. 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. In general, I'm just asking for this to be made much more obvious that it's wrong to do and more clearly documented. > > 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. > > 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?") 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. > > > 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. > > 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! > > 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?
On Thu 12 Apr 12:01 PDT 2018, Christian Lamparter wrote: > 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>; I'm still confused to why this information is in DT at all, it feels like an implementation detail, not a system configuration thing. > #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 e7abc8ba222b..ed889553f01c 100644 > --- a/drivers/pinctrl/qcom/pinctrl-msm.c > +++ b/drivers/pinctrl/qcom/pinctrl-msm.c > @@ -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(). > + */ > + 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 <bjorn.andersson@linaro.org> to both patches. Regards, Bjorn
On Friday, May 18, 2018 7:18:26 AM CEST Bjorn Andersson wrote: > On Thu 12 Apr 12:01 PDT 2018, Christian Lamparter wrote: > > 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>; > > 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. You should definitely check out the gpio/gpio.txt [0] file from section "2.1) gpio- and pin-controller interaction" onwards. (there are way more bindings in there) Maybe Linus has the full story. > > > #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 e7abc8ba222b..ed889553f01c 100644 > > --- a/drivers/pinctrl/qcom/pinctrl-msm.c > > +++ b/drivers/pinctrl/qcom/pinctrl-msm.c > > @@ -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(). > > + */ > > + 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 <bjorn.andersson@linaro.org> > > to both patches. Ok, thanks. Regards, Christian [0] <https://www.kernel.org/doc/Documentation/devicetree/bindings/gpio/gpio.txt>
On Thursday, May 17, 2018 8:56:05 AM CEST Stephen Boyd wrote: > Quoting Christian Lamparter (2018-05-16 13:29:48) > > On Wednesday, May 16, 2018 5:31:16 PM CEST Stephen Boyd wrote: > > > 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. > > 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. Yes, it there are a few. In the reply to the original report from Sven I found: <https://www.spinics.net/lists/linux-arm-msm/msg34726.html> pinctrl-mt7622, pinctrl-mtk-common.c, pinctrl-abx500.c, pinctrl-msm.c, pinctrl-as3722.c, pinctrl-at91-pio4.c, pinctrl-axp209.c, pinctrl-coh901.c, pinctrl-digicolor.c, pinctrl-pistachio.c, pinctrl-sx150x.c .. And now the new Actions S900 gpio/pinctrl patch as well. (<https://lkml.org/lkml/2018/5/19/44>) > 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. Agreed. Though, adding a warning now is likely a bit much, since the code has to be compatible with existing definitions. But if Linus agrees I think it would be fair to call drivers out with something like "the use of this function is deprecated for DT" debug level message. (As for the documentation update to gpiochip_add_pin_range() and gpiochip_add_pingroup_range(). yeah I'll give it a go.) > > 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. Everyone does :D. > > > > 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?") > > 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. (- see text the end - ) > > > > > 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. > > > > 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! > > > > > 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? > Oh, let me clarify. My plan is to add the binding documentation text for the (now) semi-required gpio-ranges property. And while I'm patching the files in Documentation/devicetree/bindings/pinctrl/qcom-* I can also add the new gpio-reserved-ranges as an optional property well. This way it is in place for the future. (This is nothing fancy. As both properties are part of the gpio.txt already). As for the dtsi updates, I don't think I can add any sensible gpio-reserved-ranges to the individual SoC's dts in arch/arm(64)/boot/dts/qcom-*dtsi without the HW/SoC or the documentation which ranges need to be reserved. Because unlike the gpio-ranges values (which are easy to extract from the drivers in drivers/pinctrl/qcom/) the gpio-reserved-ranges for each SoCs is not yet documented (or I can't find it?). Regards, Christian
On Sat, May 19, 2018 at 11:52 AM, Christian Lamparter <chunkeey@gmail.com> wrote: > On Friday, May 18, 2018 7:18:26 AM CEST Bjorn Andersson wrote: >> > + 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. This reflects especially the layout of systems such as HiSilicon that have a standard fit-all pin controller (pinctrl-single.c, shared with OMAP) and a standard PL061 GPIO primecell from ARM (gpio-pl061.c) shared with many other SoCs. These are non-HiSilicon-specific drivers used in other SoCs. So the ranges in e.g. arch/arm64/boot/dts/hisilicon/hi6220.dtsi define how the pin controller and the GPIO blocks inside the SoC are connected using these ranges. So "system configuration" is something different for you guys than for HiSilicon. They need to configure how the stuff inside the SoC are wired up, because they are using standard silicon used in other SoCs. As I understand it, your SoCs are so Qualcomm-specific and idiomatic that this situation never happened to you :) > 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. Many mistakes were made when shoehorning device tree into the ARM architecture with one finger constantly on the fast-forward button. There were reasons for this. Mea culpa. I hope to fix them all before I retire. One use case (which still applied) are pin controllers and GPIO chips defined through the use of platform data. If we have for example a PCI card with a pin controller and GPIO block (Alessandro Rubini ran into such a device at CERN IIRC) we need to be able to define both components and connect them using ranges, even without either device tree or ACPI hardware descriptions. Yours, Linus Walleij
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 10d112a4078e..9a81d2da87a0 100644 --- a/arch/arm/boot/dts/qcom-ipq4019.dtsi +++ b/arch/arm/boot/dts/qcom-ipq4019.dtsi @@ -146,6 +146,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 e7abc8ba222b..ed889553f01c 100644 --- a/drivers/pinctrl/qcom/pinctrl-msm.c +++ b/drivers/pinctrl/qcom/pinctrl-msm.c @@ -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(). + */ + 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; + } } ret = gpiochip_irqchip_add(chip,
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. 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 <sven.eckelmann@openmesh.com> Signed-off-by: Christian Lamparter <chunkeey@gmail.com> --- 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 | 23 ++++++++++++++++++----- 14 files changed, 32 insertions(+), 6 deletions(-)