| Message ID | 20180402121005.10080-1-chunkeey@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] pinctrl: msm: fix gpio-hog related boot issues | expand |
On Mon 02 Apr 05:10 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>; > #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..258fa357d946 100644 > --- a/drivers/pinctrl/qcom/pinctrl-msm.c > +++ b/drivers/pinctrl/qcom/pinctrl-msm.c > @@ -831,11 +831,22 @@ 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; > + if (!is_of_node(pctrl->dev->fwnode)) { Afaict this still means that if I boot this kernel with yesterday's dtb (without gpio-ranges) I will not get any gpios. This isn't okay. @Linus, I count 24 callers of gpiochip_add_pin_range(). Is this suggestion reasonable? Can we make gpiochip_add_pin_range() check if there's already a gpio-range and return ok in some way? > + /* > + * gpiochip_add_pin_range() is meant for platforms that > + * don't support DT. All DT platforms should just add > + * the gpio-ranges property to the pinctrl device node. > + * > + * See Documentation/devicetree/bindings/gpio/gpio.txt . > + */ > + 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; > + } Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Montag, 2. April 2018 17:04:47 CEST Bjorn Andersson wrote: > On Mon 02 Apr 05:10 PDT 2018, Christian Lamparter wrote: > > diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c > > index 495432f3341b..258fa357d946 100644 > > --- a/drivers/pinctrl/qcom/pinctrl-msm.c > > +++ b/drivers/pinctrl/qcom/pinctrl-msm.c > > @@ -831,11 +831,22 @@ 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; > > + if (!is_of_node(pctrl->dev->fwnode)) { > > Afaict this still means that if I boot this kernel with yesterday's dtb > (without gpio-ranges) I will not get any gpios. This isn't okay. Ok, fair point. I drop this chunk. > @Linus, I count 24 callers of gpiochip_add_pin_range(). Is this > suggestion reasonable? > > Can we make gpiochip_add_pin_range() check if there's already a > gpio-range and return ok in some way? Looks like Linus is currently really busy updating the gemini target (a lot of work went into it) for OpenWrt. <https://lists.openwrt.org/pipermail/openwrt-devel/2018-April/043752.html> (Kinda funny, because I do help to maintain the apm821xx and the new ipq40xx target over there.) In any case, I implemented your suggestion and it does look reasonable. The gpiolib already uses the gpio_offset as an ID of some sorts. For now I went with a simple ID value check, but this could be extended to a range/intersection check if necessary. But then again, let's not overengineer it. Comments are welcome, I'll wait around till sometime next week before I post v3. Regards, Christian --- diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index d66de67ef307..21c0f88e1159 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -2013,6 +2013,19 @@ int gpiochip_generic_config(struct gpio_chip *chip, unsigned offset, } EXPORT_SYMBOL_GPL(gpiochip_generic_config); +static struct gpio_pin_range *gpiochip_find_by_id(struct gpio_chip *chip, + unsigned int id) +{ + struct gpio_pin_range *pin_range; + struct gpio_device *gdev = chip->gpiodev; + + list_for_each_entry(pin_range, &gdev->pin_ranges, node) { + if (pin_range->range.id == id) + return pin_range; + } + return NULL; +} + #ifdef CONFIG_PINCTRL /** @@ -2030,6 +2043,20 @@ int gpiochip_add_pingroup_range(struct gpio_chip *chip, struct gpio_device *gdev = chip->gpiodev; int ret; + /* + * look if a GPIO range with the same ID has already been registered. + * For pinctrls that are set up through devicetree, the GPIO Range + * might be already set by the the gpio-ranges property. + * (see Documentation/devicetree/bindings/gpio/gpio.txt) + */ + pin_range = gpiochip_find_by_id(chip, gpio_offset); + if (pin_range) { + chip_dbg(chip, "found existing GPIO range %d->%d - skipping\n", + gpio_offset, + gpio_offset + pin_range->range.npins - 1); + return 0; + } + pin_range = kzalloc(sizeof(*pin_range), GFP_KERNEL); if (!pin_range) { chip_err(chip, "failed to allocate pin ranges\n"); @@ -2083,6 +2110,20 @@ int gpiochip_add_pin_range(struct gpio_chip *chip, const char *pinctl_name, struct gpio_device *gdev = chip->gpiodev; int ret; + /* + * look if a GPIO range with the same ID has already been registered. + * For pinctrls that are set up through devicetree, the GPIO Range + * might be already set by the the gpio-ranges property. + * (see Documentation/devicetree/bindings/gpio/gpio.txt) + */ + pin_range = gpiochip_find_by_id(chip, gpio_offset); + if (pin_range) { + chip_dbg(chip, "found existing GPIO range %d->%d - skipping\n", + gpio_offset, + gpio_offset + pin_range->range.npins - 1); + return 0; + } + pin_range = kzalloc(sizeof(*pin_range), GFP_KERNEL); if (!pin_range) { chip_err(chip, "failed to allocate pin ranges\n"); --- -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
I see I might have missed this mail, sorry for that. On Thu, Apr 5, 2018 at 6:35 PM, Christian Lamparter <chunkeey@gmail.com> wrote: > On Montag, 2. April 2018 17:04:47 CEST Bjorn Andersson wrote: >> On Mon 02 Apr 05:10 PDT 2018, Christian Lamparter wrote: >> > diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c >> > index 495432f3341b..258fa357d946 100644 >> > --- a/drivers/pinctrl/qcom/pinctrl-msm.c >> > +++ b/drivers/pinctrl/qcom/pinctrl-msm.c >> > @@ -831,11 +831,22 @@ 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; >> > + if (!is_of_node(pctrl->dev->fwnode)) { >> >> Afaict this still means that if I boot this kernel with yesterday's dtb >> (without gpio-ranges) I will not get any gpios. This isn't okay. > Ok, fair point. I drop this chunk. > >> @Linus, I count 24 callers of gpiochip_add_pin_range(). Is this >> suggestion reasonable? >> >> Can we make gpiochip_add_pin_range() check if there's already a >> gpio-range and return ok in some way? I think I replied in some other mail that I think we need to be backwards compatible and it's not too hard to do both. (Correct me if I'm wrong.) > Looks like Linus is currently really busy updating the gemini > target (a lot of work went into it) for OpenWrt. > <https://lists.openwrt.org/pipermail/openwrt-devel/2018-April/043752.html> > (Kinda funny, because I do help to maintain the apm821xx and the new > ipq40xx target over there.) Yeah it was more of a hobby, partly research for a lecture on maintaining old ARM systems and all dangerous aftermarket devices that is littering the world. Since I have noticed that the adoption of OpenWRT/LEDE goes far beyond routers and it is one of the most prevalent distributions that is existing in the IoT of darkness on the planet being the most deployed OS for bitcoin mining and being picked up for functional safety (such as automotive) because of its minimalist userspace. I'm still amazed by the weirdness of our business. > In any case, I implemented your suggestion and it does look reasonable. > The gpiolib already uses the gpio_offset as an ID of some sorts. For > now I went with a simple ID value check, but this could be extended to > a range/intersection check if necessary. But then again, let's not > overengineer it. Comments are welcome, I'll wait around till sometime > next week before I post v3. I'm going through my mailbox now, sorry for delays in feedback and review :( Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Apr 26, 2018 11:12:21 CEST Linus Walleij wrote: > On Thu, Apr 5, 2018 at 6:35 PM, Christian Lamparter wrote: > > On Montag, 2. April 2018 17:04:47 CEST Bjorn Andersson wrote: > >> @Linus, I count 24 callers of gpiochip_add_pin_range(). Is this > >> suggestion reasonable? > >> > >> Can we make gpiochip_add_pin_range() check if there's already a > >> gpio-range and return ok in some way? > > I think I replied in some other mail that I think we need to > be backwards compatible and it's not too hard to do > both. (Correct me if I'm wrong.) I think so too, I looked around and found that the nvidia pinctrl was doing something similar with of_find_property(): <https://elixir.bootlin.com/linux/v4.17-rc2/source/drivers/pinctrl/tegra/pinctrl-tegra.c#L652> | has_prop = of_find_property(np, "gpio-ranges", NULL); However this looks kinda funny, since "has_prob" is declared as a bool and of_find_property() returns a pointer to a "struct property".... Tell you what: If nobody beats me to it, I'll sent a patch for this after the pinctrl-msm's gpio-hog has been dealt with. :) > > Looks like Linus is currently really busy updating the gemini > > target (a lot of work went into it) for OpenWrt. > > <https://lists.openwrt.org/pipermail/openwrt-devel/2018-April/043752.html> > > (Kinda funny, because I do help to maintain the apm821xx and the new > > ipq40xx target over there.) > > Yeah it was more of a hobby, partly research for a lecture > on maintaining old ARM systems and all dangerous aftermarket > devices that is littering the world. > > Since I have noticed that the adoption of OpenWRT/LEDE goes > far beyond routers and it is one of the most prevalent > distributions that is existing in the IoT of darkness on the > planet being the most deployed OS for bitcoin mining and > being picked up for functional safety (such as automotive) > because of its minimalist userspace. > > I'm still amazed by the weirdness of our business. I think I can add something to this thought as well. Albeit, it's a more traditional, less zany story. I started with the apm821xx (a PowerPC 464 derivative SoC) back in 2015-2016 because of the wide availabilty of cheap (as low as $5 ~ $10) secondhand enterprise accesspoints like the Cisco Meraki MR24 <https://openwrt.org/toh/meraki/mr24>. What happen was that cooperations ditched all the outdated 802.11n devices and upgraded to the shiny new wave1 802.11ac gear. Now, the old 802.11n devices still worked perfectly fine but the firmware on the device requires the user to have a valid subscription to a "Meraki Cloud License". And as you can probably guess: The License is sold as a yearly subscription starting from $150 for a single device. So, these devices became practically e-waste since nobody in their right mind would buy a used device and then fork over the ~$150 fee per annum. The MR24 craze is mostly over by now. You can still find a few. However some listings are now selling them with OpenWrt/LEDE for ~$40. And obviously, this cycle will continue on, but now with the old wave1 802.11ac gear that gets replaced. In fact this business has spawned companies that are actively working on supporting "old" enterprise gear via their own OpenWrt/LEDE derivatives. > > In any case, I implemented your suggestion and it does look reasonable. > > The gpiolib already uses the gpio_offset as an ID of some sorts. For > > now I went with a simple ID value check, but this could be extended to > > a range/intersection check if necessary. But then again, let's not > > overengineer it. Comments are welcome, I'll wait around till sometime > > next week before I post v3. > > I'm going through my mailbox now, sorry for delays in feedback > and review :( I see you skipped v3 and replied to v4. Best Regards, Christian Lamparter -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Apr 26, 2018 at 11:47 PM, Christian Lamparter <chunkeey@gmail.com> wrote: > On Thu, Apr 26, 2018 11:12:21 CEST Linus Walleij wrote: >> I think I replied in some other mail that I think we need to >> be backwards compatible and it's not too hard to do >> both. (Correct me if I'm wrong.) > > I think so too, I looked around and found that the nvidia pinctrl was > doing something similar with of_find_property(): > <https://elixir.bootlin.com/linux/v4.17-rc2/source/drivers/pinctrl/tegra/pinctrl-tegra.c#L652> > | has_prop = of_find_property(np, "gpio-ranges", NULL); > > However this looks kinda funny, since "has_prob" is declared as a bool > and of_find_property() returns a pointer to a "struct property".... > Tell you what: If nobody beats me to it, I'll sent a patch for this after > the pinctrl-msm's gpio-hog has been dealt with. :) Yeah the nVidia driver is one of the oldest and also at the time DT was kind of new. I haven't heard from Stephen for a while but I bet he will pop up, else check with Laxman, he's got a good grip on nVidia pinctrl+GPIO as well. > And as you can probably guess: The License is sold as a yearly subscription > starting from $150 for a single device. So, these devices became practically > e-waste since nobody in their right mind would buy a used device and then > fork over the ~$150 fee per annum. > > The MR24 craze is mostly over by now. You can still find a few. However > some listings are now selling them with OpenWrt/LEDE for ~$40. > > And obviously, this cycle will continue on, but now with the old wave1 > 802.11ac gear that gets replaced. In fact this business has spawned > companies that are actively working on supporting "old" enterprise gear > via their own OpenWrt/LEDE derivatives. Haha that is just awesome :D I hope they salvage a lot of them. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mittwoch, 2. Mai 2018 14:14:39 CEST Linus Walleij wrote: > On Thu, Apr 26, 2018 at 11:47 PM, Christian Lamparter > <chunkeey@gmail.com> wrote: > > On Thu, Apr 26, 2018 11:12:21 CEST Linus Walleij wrote: > > >> I think I replied in some other mail that I think we need to > >> be backwards compatible and it's not too hard to do > >> both. (Correct me if I'm wrong.) > > > > I think so too, I looked around and found that the nvidia pinctrl was > > doing something similar with of_find_property(): > > <https://elixir.bootlin.com/linux/v4.17-rc2/source/drivers/pinctrl/tegra/pinctrl-tegra.c#L652> > > | has_prop = of_find_property(np, "gpio-ranges", NULL); > > > > However this looks kinda funny, since "has_prob" is declared as a bool > > and of_find_property() returns a pointer to a "struct property".... > > Tell you what: If nobody beats me to it, I'll sent a patch for this after > > the pinctrl-msm's gpio-hog has been dealt with. :) > > Yeah the nVidia driver is one of the oldest and also at the time > DT was kind of new. I haven't heard from Stephen for a while > but I bet he will pop up, else check with Laxman, he's got > a good grip on nVidia pinctrl+GPIO as well. All in good time. But first @Bjorn and @Andy or @David can you please look and review v4 "pinctrl: msm: fix gpio-hog related boot issues" <https://patchwork.kernel.org/patch/10339129/> Thanks. > > The MR24 craze is mostly over by now. You can still find a few. However > > some listings are now selling them with OpenWrt/LEDE for ~$40. > > > > And obviously, this cycle will continue on, but now with the old wave1 > > 802.11ac gear that gets replaced. In fact this business has spawned > > companies that are actively working on supporting "old" enterprise gear > > via their own OpenWrt/LEDE derivatives. > > Haha that is just awesome :D > I hope they salvage a lot of them. Yes, I'm aware of that some of them where put to good use in the Personal Telco Projec (501(c)(3) non-profit organization in Portland, Oregon): <https://personaltelco.net/wiki/PersonalTelco> And they have been somewhat vocal about it too: <https://groups.google.com/forum/#!msg/ptp-general/RZ-VjKFonVo/cQgnGn2wAgAJ> "The MR24 is a dual-band 802.11n 3x3 MIMO access point with a single ethernet port. They are "last-gen" devices, and are starting to show up on ebay at reasonable prices as Meraki's deep-pocket cloud-management enterprise users are beginning to "upgrade" to 802.11ac gear. Note that 802.11n in this case means a fully-open-source driver (ath9k). 802.11ac drivers involve firmware blobs across the board. You don't get super-wide 5GHz channels, but you get freedom. " ;) By the way, it gets even weirder. In the past (and to this day) Meraki has given away their current crop of enterprise APs via their "Free AP for IT Professionals" <https://meraki.cisco.com/tc/freeap> program. Of course, the main idea probably was to get them all "hooked/sold" on their cloud-management firmware. Because of course that "Free AP" is intended to be only good for the lifespan of included the 3-year license. However, "IT Professionals" do have their own mind and that's why there is some continued interest in making alternative firmwares for these devices. Best Regards, Christian -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday 03 May 2018 11:13 PM, Christian Lamparter wrote: > On Mittwoch, 2. Mai 2018 14:14:39 CEST Linus Walleij wrote: >> On Thu, Apr 26, 2018 at 11:47 PM, Christian Lamparter >> <chunkeey@gmail.com> wrote: >>> On Thu, Apr 26, 2018 11:12:21 CEST Linus Walleij wrote: >>>> I think I replied in some other mail that I think we need to >>>> be backwards compatible and it's not too hard to do >>>> both. (Correct me if I'm wrong.) >>> I think so too, I looked around and found that the nvidia pinctrl was >>> doing something similar with of_find_property(): >>> <https://elixir.bootlin.com/linux/v4.17-rc2/source/drivers/pinctrl/tegra/pinctrl-tegra.c#L652> >>> | has_prop = of_find_property(np, "gpio-ranges", NULL); >>> >>> However this looks kinda funny, since "has_prob" is declared as a bool >>> and of_find_property() returns a pointer to a "struct property".... >>> Tell you what: If nobody beats me to it, I'll sent a patch for this after >>> the pinctrl-msm's gpio-hog has been dealt with. :) >> Yeah the nVidia driver is one of the oldest and also at the time >> DT was kind of new. I haven't heard from Stephen for a while >> but I bet he will pop up, else check with Laxman, he's got >> a good grip on nVidia pinctrl+GPIO as well. > All in good time. > > But first @Bjorn and @Andy or @David can you please look and > review v4 "pinctrl: msm: fix gpio-hog related boot issues" > <https://patchwork.kernel.org/patch/10339129/> Agree with the patch as by adding gpio-ranges, we add the gpio range part of gpiochip_add(), called by of_gpiochip_add()->of_gpiochip_add_pin_range(). and so does not need to explicitly call of the gpiochip_add_pin_range(). Please add ack in above patch. Acked-by: Laxman Dewangan <ldewangan@nvidia.com> -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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..258fa357d946 100644 --- a/drivers/pinctrl/qcom/pinctrl-msm.c +++ b/drivers/pinctrl/qcom/pinctrl-msm.c @@ -831,11 +831,22 @@ 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; + if (!is_of_node(pctrl->dev->fwnode)) { + /* + * gpiochip_add_pin_range() is meant for platforms that + * don't support DT. All DT platforms should just add + * the gpio-ranges property to the pinctrl device node. + * + * See Documentation/devicetree/bindings/gpio/gpio.txt . + */ + 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, 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>; v1->v2: restore gpiochip_add_pin_range() for ACPI compatibility --- 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 | 21 ++++++++++++++++----- 14 files changed, 30 insertions(+), 6 deletions(-)