diff mbox

[v4] pinctrl: msm: fix gpio-hog related boot issues

Message ID 20180412190138.12372-1-chunkeey@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Christian Lamparter April 12, 2018, 7:01 p.m. UTC
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(-)

Comments

Sven Eckelmann April 16, 2018, 11:50 a.m. UTC | #1
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
Linus Walleij April 26, 2018, 12:03 p.m. UTC | #2
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
Linus Walleij May 16, 2018, 12:28 p.m. UTC | #3
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
Stephen Boyd May 16, 2018, 3:31 p.m. UTC | #4
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.
Christian Lamparter May 16, 2018, 8:29 p.m. UTC | #5
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>
Stephen Boyd May 17, 2018, 6:56 a.m. UTC | #6
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?
Bjorn Andersson May 18, 2018, 5:18 a.m. UTC | #7
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
Christian Lamparter May 19, 2018, 9:52 a.m. UTC | #8
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>
Christian Lamparter May 19, 2018, 11:38 a.m. UTC | #9
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
Linus Walleij May 24, 2018, 7:29 a.m. UTC | #10
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 mbox

Patch

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,