diff mbox series

[1/4] arm64: dts: qcom: sdm845: fix number of pins in 'gpio-ranges'

Message ID 20210303033106.549-2-shawn.guo@linaro.org
State Accepted
Commit 02058fc3839df65ff64de2a6b1c5de8c9fd705c1
Headers show
Series [1/4] arm64: dts: qcom: sdm845: fix number of pins in 'gpio-ranges' | expand

Commit Message

Shawn Guo March 3, 2021, 3:31 a.m. UTC 
The last cell of 'gpio-ranges' should be number of GPIO pins, and in
case of qcom platform it should match msm_pinctrl_soc_data.ngpio rather
than msm_pinctrl_soc_data.ngpio - 1.

This fixes the problem that when the last GPIO pin in the range is
configured with the following call sequence, it always fails with
-EPROBE_DEFER.

    pinctrl_gpio_set_config()
        pinctrl_get_device_gpio_range()
            pinctrl_match_gpio_range()

Fixes: bc2c806293c6 ("arm64: dts: qcom: sdm845: Add gpio-ranges to TLMM node")
Cc: Evan Green <evgreen@chromium.org>
Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 arch/arm64/boot/dts/qcom/sdm845.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Bjorn Andersson March 5, 2021, 9:43 p.m. UTC | #1 
On Tue 02 Mar 21:31 CST 2021, Shawn Guo wrote:

> The last cell of 'gpio-ranges' should be number of GPIO pins, and in
> case of qcom platform it should match msm_pinctrl_soc_data.ngpio rather
> than msm_pinctrl_soc_data.ngpio - 1.
> 

This is a historical artifact, SDM845 has 150 GPIO pins. In addition to
this there's an output-only pin for UFS, which I exposed as an GPIO as
well - but it's only supposed to be used as a reset-gpio for the UFS
device.

Perhaps that still mandates that gpio-ranges should cover it?

> This fixes the problem that when the last GPIO pin in the range is
> configured with the following call sequence, it always fails with
> -EPROBE_DEFER.
> 
>     pinctrl_gpio_set_config()
>         pinctrl_get_device_gpio_range()
>             pinctrl_match_gpio_range()

When do we hit this sequence? I didn't think operations on the UFS
GP(I)O would ever take this code path?

Regards,
Bjorn

> 
> Fixes: bc2c806293c6 ("arm64: dts: qcom: sdm845: Add gpio-ranges to TLMM node")
> Cc: Evan Green <evgreen@chromium.org>
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> ---
>  arch/arm64/boot/dts/qcom/sdm845.dtsi | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> index 454f794af547..6a2ed02d383d 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> @@ -2382,7 +2382,7 @@
>  			#gpio-cells = <2>;
>  			interrupt-controller;
>  			#interrupt-cells = <2>;
> -			gpio-ranges = <&tlmm 0 0 150>;
> +			gpio-ranges = <&tlmm 0 0 151>;
>  			wakeup-parent = <&pdc_intc>;
>  
>  			cci0_default: cci0-default {
> -- 
> 2.17.1
>
Bjorn Andersson March 6, 2021, 1:56 a.m. UTC | #2 
On Fri 05 Mar 19:28 CST 2021, Shawn Guo wrote:

> On Fri, Mar 05, 2021 at 03:43:08PM -0600, Bjorn Andersson wrote:

> > On Tue 02 Mar 21:31 CST 2021, Shawn Guo wrote:

> > 

> > > The last cell of 'gpio-ranges' should be number of GPIO pins, and in

> > > case of qcom platform it should match msm_pinctrl_soc_data.ngpio rather

> > > than msm_pinctrl_soc_data.ngpio - 1.

> > > 

> > 

> > This is a historical artifact, SDM845 has 150 GPIO pins. In addition to

> > this there's an output-only pin for UFS, which I exposed as an GPIO as

> > well - but it's only supposed to be used as a reset-gpio for the UFS

> > device.

> > 

> > Perhaps that still mandates that gpio-ranges should cover it?

> 

> I think the number in DT gpio-ranges should match msm_pinctrl_soc_data.ngpio.

> Otherwise, kernel will be confused and be running into the issue like

> below in some case.

> 

> > 

> > > This fixes the problem that when the last GPIO pin in the range is

> > > configured with the following call sequence, it always fails with

> > > -EPROBE_DEFER.

> > > 

> > >     pinctrl_gpio_set_config()

> > >         pinctrl_get_device_gpio_range()

> > >             pinctrl_match_gpio_range()

> > 

> > When do we hit this sequence? I didn't think operations on the UFS

> > GP(I)O would ever take this code path?

> 

> It will, if we have UFS driver booting from ACPI and requesting reset

> GPIO.


But does the UFS driver somehow request GPIO 190 on SC8180x?

I made up the idea that this is a GPIO, there really only is 190 (0-189)
GPIOs on thie SoC.

Downstream they use a pinconf node with "output-high"/"output-low" to
toggle the reset pin and I don't find any references in the Flex 5G
DSDT.

> And we are hit this sequence with my patch that adds .set_config

> for gpio_chip [1].

> 


What's calling pinctrl_gpio_set_config() in this case?

Regards,
Bjorn

> Shawn

> 

> [1] https://lore.kernel.org/linux-gpio/YEDVMpHyCGbZOrmF@smile.fi.intel.com/
Shawn Guo March 6, 2021, 8 a.m. UTC | #3 
On Fri, Mar 05, 2021 at 07:56:02PM -0600, Bjorn Andersson wrote:
> On Fri 05 Mar 19:28 CST 2021, Shawn Guo wrote:
> 
> > On Fri, Mar 05, 2021 at 03:43:08PM -0600, Bjorn Andersson wrote:
> > > On Tue 02 Mar 21:31 CST 2021, Shawn Guo wrote:
> > > 
> > > > The last cell of 'gpio-ranges' should be number of GPIO pins, and in
> > > > case of qcom platform it should match msm_pinctrl_soc_data.ngpio rather
> > > > than msm_pinctrl_soc_data.ngpio - 1.
> > > > 
> > > 
> > > This is a historical artifact, SDM845 has 150 GPIO pins. In addition to
> > > this there's an output-only pin for UFS, which I exposed as an GPIO as
> > > well - but it's only supposed to be used as a reset-gpio for the UFS
> > > device.
> > > 
> > > Perhaps that still mandates that gpio-ranges should cover it?
> > 
> > I think the number in DT gpio-ranges should match msm_pinctrl_soc_data.ngpio.
> > Otherwise, kernel will be confused and be running into the issue like
> > below in some case.
> > 
> > > 
> > > > This fixes the problem that when the last GPIO pin in the range is
> > > > configured with the following call sequence, it always fails with
> > > > -EPROBE_DEFER.
> > > > 
> > > >     pinctrl_gpio_set_config()
> > > >         pinctrl_get_device_gpio_range()
> > > >             pinctrl_match_gpio_range()
> > > 
> > > When do we hit this sequence? I didn't think operations on the UFS
> > > GP(I)O would ever take this code path?
> > 
> > It will, if we have UFS driver booting from ACPI and requesting reset
> > GPIO.
> 
> But does the UFS driver somehow request GPIO 190 on SC8180x?
> 
> I made up the idea that this is a GPIO, there really only is 190 (0-189)
> GPIOs on thie SoC.
> 
> Downstream they use a pinconf node with "output-high"/"output-low" to
> toggle the reset pin and I don't find any references in the Flex 5G
> DSDT.

Right now, I do not have to request and configure this UFS GPIO for
getting UFS work with ACPI kernel.  And the immediate problem we have is
that with gpio_chip .set_config patch, devm_gpiod_get_optional() call
from UFS driver always gets -EPROBE_DEFER.

> 
> > And we are hit this sequence with my patch that adds .set_config
> > for gpio_chip [1].
> > 
> 
> What's calling pinctrl_gpio_set_config() in this case?

  ufs_qcom_probe
    ufshcd_pltfrm_init
      ufshcd_init
        ufs_qcom_init
          devm_gpiod_get_optional
            devm_gpiod_get_index
              gpiod_get_index
                gpiod_configure_flags
                  gpiod_direction_output
                    gpiochip_generic_config

Shawn
Bjorn Andersson March 10, 2021, 6:22 p.m. UTC | #4 
On Sat 06 Mar 02:00 CST 2021, Shawn Guo wrote:

> On Fri, Mar 05, 2021 at 07:56:02PM -0600, Bjorn Andersson wrote:

> > On Fri 05 Mar 19:28 CST 2021, Shawn Guo wrote:

> > 

> > > On Fri, Mar 05, 2021 at 03:43:08PM -0600, Bjorn Andersson wrote:

> > > > On Tue 02 Mar 21:31 CST 2021, Shawn Guo wrote:

> > > > 

> > > > > The last cell of 'gpio-ranges' should be number of GPIO pins, and in

> > > > > case of qcom platform it should match msm_pinctrl_soc_data.ngpio rather

> > > > > than msm_pinctrl_soc_data.ngpio - 1.

> > > > > 

> > > > 

> > > > This is a historical artifact, SDM845 has 150 GPIO pins. In addition to

> > > > this there's an output-only pin for UFS, which I exposed as an GPIO as

> > > > well - but it's only supposed to be used as a reset-gpio for the UFS

> > > > device.

> > > > 

> > > > Perhaps that still mandates that gpio-ranges should cover it?

> > > 

> > > I think the number in DT gpio-ranges should match msm_pinctrl_soc_data.ngpio.

> > > Otherwise, kernel will be confused and be running into the issue like

> > > below in some case.

> > > 

> > > > 

> > > > > This fixes the problem that when the last GPIO pin in the range is

> > > > > configured with the following call sequence, it always fails with

> > > > > -EPROBE_DEFER.

> > > > > 

> > > > >     pinctrl_gpio_set_config()

> > > > >         pinctrl_get_device_gpio_range()

> > > > >             pinctrl_match_gpio_range()

> > > > 

> > > > When do we hit this sequence? I didn't think operations on the UFS

> > > > GP(I)O would ever take this code path?

> > > 

> > > It will, if we have UFS driver booting from ACPI and requesting reset

> > > GPIO.

> > 

> > But does the UFS driver somehow request GPIO 190 on SC8180x?

> > 

> > I made up the idea that this is a GPIO, there really only is 190 (0-189)

> > GPIOs on thie SoC.

> > 

> > Downstream they use a pinconf node with "output-high"/"output-low" to

> > toggle the reset pin and I don't find any references in the Flex 5G

> > DSDT.

> 

> Right now, I do not have to request and configure this UFS GPIO for

> getting UFS work with ACPI kernel.  And the immediate problem we have is

> that with gpio_chip .set_config patch, devm_gpiod_get_optional() call

> from UFS driver always gets -EPROBE_DEFER.

> 


But we don't have a "reset" GPIO specified in the ACPI node, or you mean
with the introduction of .set_config DT no longer works?

Regards,
Bjorn

> > 

> > > And we are hit this sequence with my patch that adds .set_config

> > > for gpio_chip [1].

> > > 

> > 

> > What's calling pinctrl_gpio_set_config() in this case?

> 

>   ufs_qcom_probe

>     ufshcd_pltfrm_init

>       ufshcd_init

>         ufs_qcom_init

>           devm_gpiod_get_optional

>             devm_gpiod_get_index

>               gpiod_get_index

>                 gpiod_configure_flags

>                   gpiod_direction_output

>                     gpiochip_generic_config

> 

> Shawn
Shawn Guo March 11, 2021, 1:19 a.m. UTC | #5 
On Wed, Mar 10, 2021 at 12:22:32PM -0600, Bjorn Andersson wrote:
> On Sat 06 Mar 02:00 CST 2021, Shawn Guo wrote:

> 

> > On Fri, Mar 05, 2021 at 07:56:02PM -0600, Bjorn Andersson wrote:

> > > On Fri 05 Mar 19:28 CST 2021, Shawn Guo wrote:

> > > 

> > > > On Fri, Mar 05, 2021 at 03:43:08PM -0600, Bjorn Andersson wrote:

> > > > > On Tue 02 Mar 21:31 CST 2021, Shawn Guo wrote:

> > > > > 

> > > > > > The last cell of 'gpio-ranges' should be number of GPIO pins, and in

> > > > > > case of qcom platform it should match msm_pinctrl_soc_data.ngpio rather

> > > > > > than msm_pinctrl_soc_data.ngpio - 1.

> > > > > > 

> > > > > 

> > > > > This is a historical artifact, SDM845 has 150 GPIO pins. In addition to

> > > > > this there's an output-only pin for UFS, which I exposed as an GPIO as

> > > > > well - but it's only supposed to be used as a reset-gpio for the UFS

> > > > > device.

> > > > > 

> > > > > Perhaps that still mandates that gpio-ranges should cover it?

> > > > 

> > > > I think the number in DT gpio-ranges should match msm_pinctrl_soc_data.ngpio.

> > > > Otherwise, kernel will be confused and be running into the issue like

> > > > below in some case.

> > > > 

> > > > > 

> > > > > > This fixes the problem that when the last GPIO pin in the range is

> > > > > > configured with the following call sequence, it always fails with

> > > > > > -EPROBE_DEFER.

> > > > > > 

> > > > > >     pinctrl_gpio_set_config()

> > > > > >         pinctrl_get_device_gpio_range()

> > > > > >             pinctrl_match_gpio_range()

> > > > > 

> > > > > When do we hit this sequence? I didn't think operations on the UFS

> > > > > GP(I)O would ever take this code path?

> > > > 

> > > > It will, if we have UFS driver booting from ACPI and requesting reset

> > > > GPIO.

> > > 

> > > But does the UFS driver somehow request GPIO 190 on SC8180x?

> > > 

> > > I made up the idea that this is a GPIO, there really only is 190 (0-189)

> > > GPIOs on thie SoC.

> > > 

> > > Downstream they use a pinconf node with "output-high"/"output-low" to

> > > toggle the reset pin and I don't find any references in the Flex 5G

> > > DSDT.

> > 

> > Right now, I do not have to request and configure this UFS GPIO for

> > getting UFS work with ACPI kernel.  And the immediate problem we have is

> > that with gpio_chip .set_config patch, devm_gpiod_get_optional() call

> > from UFS driver always gets -EPROBE_DEFER.

> > 

> 

> But we don't have a "reset" GPIO specified in the ACPI node, or you mean

> with the introduction of .set_config DT no longer works?


Yes, DT stops working because of the mismatch between
msm_pinctrl_soc_data.ngpio and gpio-ranges.

Shawn
Bjorn Andersson March 11, 2021, 4:53 p.m. UTC | #6 
On Wed 10 Mar 19:19 CST 2021, Shawn Guo wrote:

> On Wed, Mar 10, 2021 at 12:22:32PM -0600, Bjorn Andersson wrote:
> > On Sat 06 Mar 02:00 CST 2021, Shawn Guo wrote:
> > 
> > > On Fri, Mar 05, 2021 at 07:56:02PM -0600, Bjorn Andersson wrote:
> > > > On Fri 05 Mar 19:28 CST 2021, Shawn Guo wrote:
> > > > 
> > > > > On Fri, Mar 05, 2021 at 03:43:08PM -0600, Bjorn Andersson wrote:
> > > > > > On Tue 02 Mar 21:31 CST 2021, Shawn Guo wrote:
> > > > > > 
> > > > > > > The last cell of 'gpio-ranges' should be number of GPIO pins, and in
> > > > > > > case of qcom platform it should match msm_pinctrl_soc_data.ngpio rather
> > > > > > > than msm_pinctrl_soc_data.ngpio - 1.
> > > > > > > 
> > > > > > 
> > > > > > This is a historical artifact, SDM845 has 150 GPIO pins. In addition to
> > > > > > this there's an output-only pin for UFS, which I exposed as an GPIO as
> > > > > > well - but it's only supposed to be used as a reset-gpio for the UFS
> > > > > > device.
> > > > > > 
> > > > > > Perhaps that still mandates that gpio-ranges should cover it?
> > > > > 
> > > > > I think the number in DT gpio-ranges should match msm_pinctrl_soc_data.ngpio.
> > > > > Otherwise, kernel will be confused and be running into the issue like
> > > > > below in some case.
> > > > > 
> > > > > > 
> > > > > > > This fixes the problem that when the last GPIO pin in the range is
> > > > > > > configured with the following call sequence, it always fails with
> > > > > > > -EPROBE_DEFER.
> > > > > > > 
> > > > > > >     pinctrl_gpio_set_config()
> > > > > > >         pinctrl_get_device_gpio_range()
> > > > > > >             pinctrl_match_gpio_range()
> > > > > > 
> > > > > > When do we hit this sequence? I didn't think operations on the UFS
> > > > > > GP(I)O would ever take this code path?
> > > > > 
> > > > > It will, if we have UFS driver booting from ACPI and requesting reset
> > > > > GPIO.
> > > > 
> > > > But does the UFS driver somehow request GPIO 190 on SC8180x?
> > > > 
> > > > I made up the idea that this is a GPIO, there really only is 190 (0-189)
> > > > GPIOs on thie SoC.
> > > > 
> > > > Downstream they use a pinconf node with "output-high"/"output-low" to
> > > > toggle the reset pin and I don't find any references in the Flex 5G
> > > > DSDT.
> > > 
> > > Right now, I do not have to request and configure this UFS GPIO for
> > > getting UFS work with ACPI kernel.  And the immediate problem we have is
> > > that with gpio_chip .set_config patch, devm_gpiod_get_optional() call
> > > from UFS driver always gets -EPROBE_DEFER.
> > > 
> > 
> > But we don't have a "reset" GPIO specified in the ACPI node, or you mean
> > with the introduction of .set_config DT no longer works?
> 
> Yes, DT stops working because of the mismatch between
> msm_pinctrl_soc_data.ngpio and gpio-ranges.
> 

So what you're saying is that when Linus merged the .set_config patch
yesterday he broke storage on every single Qualcomm device?

Regards,
Bjorn
Shawn Guo March 11, 2021, 11:09 p.m. UTC | #7 
On Thu, Mar 11, 2021 at 10:53:49AM -0600, Bjorn Andersson wrote:
> On Wed 10 Mar 19:19 CST 2021, Shawn Guo wrote:
> > Yes, DT stops working because of the mismatch between
> > msm_pinctrl_soc_data.ngpio and gpio-ranges.
> > 
> 
> So what you're saying is that when Linus merged the .set_config patch
> yesterday he broke storage on every single Qualcomm device?

Better than that.  Only the ones that have mismatching between
msm_pinctrl_soc_data.ngpio and gpio-ranges.  More specifically, the ones
that the series are fixing.

I didn't realize this break when I was working on the .set_config change
for ACPI.  It was a surprise when I tested DT later.  You can ask Linus
to drop .set_config patch, if you do not like this break.  But I think
the mismatch issue still needs to be resolved in some way.

Shawn
Bjorn Andersson March 11, 2021, 11:17 p.m. UTC | #8 
On Thu 11 Mar 17:09 CST 2021, Shawn Guo wrote:

> On Thu, Mar 11, 2021 at 10:53:49AM -0600, Bjorn Andersson wrote:
> > On Wed 10 Mar 19:19 CST 2021, Shawn Guo wrote:
> > > Yes, DT stops working because of the mismatch between
> > > msm_pinctrl_soc_data.ngpio and gpio-ranges.
> > > 
> > 
> > So what you're saying is that when Linus merged the .set_config patch
> > yesterday he broke storage on every single Qualcomm device?
> 
> Better than that.  Only the ones that have mismatching between
> msm_pinctrl_soc_data.ngpio and gpio-ranges.  More specifically, the ones
> that the series are fixing.
> 
> I didn't realize this break when I was working on the .set_config change
> for ACPI.  It was a surprise when I tested DT later.  You can ask Linus
> to drop .set_config patch, if you do not like this break.  But I think
> the mismatch issue still needs to be resolved in some way.
> 

We're exposing the UFS as a gpio and I think that these patches
therefore are correct, so I've picked them up.

But I don't think we should break backwards compatibility will all
existing DTBs...

Regards,
Bjorn
Linus Walleij March 15, 2021, 4:11 p.m. UTC | #9 
On Thu, Mar 11, 2021 at 5:53 PM Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
> On Wed 10 Mar 19:19 CST 2021, Shawn Guo wrote:

> > > But we don't have a "reset" GPIO specified in the ACPI node, or you mean
> > > with the introduction of .set_config DT no longer works?
> >
> > Yes, DT stops working because of the mismatch between
> > msm_pinctrl_soc_data.ngpio and gpio-ranges.
> >
>
> So what you're saying is that when Linus merged the .set_config patch
> yesterday he broke storage on every single Qualcomm device?

I took out that patch for now.

Maybe we can keep all the stuff in one series if it has strict
dependencies?

Yours,
Linus Walleij
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index 454f794af547..6a2ed02d383d 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -2382,7 +2382,7 @@ 
 			#gpio-cells = <2>;
 			interrupt-controller;
 			#interrupt-cells = <2>;
-			gpio-ranges = <&tlmm 0 0 150>;
+			gpio-ranges = <&tlmm 0 0 151>;
 			wakeup-parent = <&pdc_intc>;
 
 			cci0_default: cci0-default {