| 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 |
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 >
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/
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/
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
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
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
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
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
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
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 --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 {
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(-)