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 (mailing list archive)
State Accepted
Commit c0590924525ac368535a103e3482666fcdc1faeb
Headers show
Series 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
>
Shawn Guo March 6, 2021, 1:28 a.m. UTC | #2
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.  And we are hit this sequence with my patch that adds .set_config
for gpio_chip [1].

Shawn

[1] https://lore.kernel.org/linux-gpio/YEDVMpHyCGbZOrmF@smile.fi.intel.com/
Bjorn Andersson March 6, 2021, 1:56 a.m. UTC | #3
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 | #4
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 | #5
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 | #6
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 | #7
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 | #8
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 | #9
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 | #10
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 {