| Message ID | 20211122190552.74073-3-kaperez@linux.microsoft.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | arm64: dts: sm8350: add support for Microsoft Surface Duo 2 | expand |
On 22-11-21, 11:05, Katherine Perez wrote: > TLMM controller base address is incorrect and will hang on some platforms. > Fix by giving the correct address. Thanks, recheck the spec this looks correct. We should have tlmm reg space here and not tlmm base which also contains xpu region (thus hang) Reviewed-by: Vinod Koul <vkoul@kernel.org> Fixes: b7e8f433a673 ("arm64: dts: qcom: Add basic devicetree support for SM8350 SoC") > > Signed-off-by: Katherine Perez <kaperez@linux.microsoft.com> > --- > arch/arm64/boot/dts/qcom/sm8350.dtsi | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/arm64/boot/dts/qcom/sm8350.dtsi b/arch/arm64/boot/dts/qcom/sm8350.dtsi > index d134280e2939..624d294612d8 100644 > --- a/arch/arm64/boot/dts/qcom/sm8350.dtsi > +++ b/arch/arm64/boot/dts/qcom/sm8350.dtsi > @@ -960,9 +960,9 @@ spmi_bus: spmi@c440000 { > #interrupt-cells = <4>; > }; > > - tlmm: pinctrl@f100000 { > + tlmm: pinctrl@f000000 { > compatible = "qcom,sm8350-tlmm"; > - reg = <0 0x0f100000 0 0x300000>; > + reg = <0 0x0f000000 0 0x300000>; > interrupts = <GIC_SPI 208 IRQ_TYPE_LEVEL_HIGH>; > gpio-controller; > #gpio-cells = <2>; > -- > 2.31.1
On Mon 22 Nov 22:03 CST 2021, Vinod Koul wrote: > On 22-11-21, 11:05, Katherine Perez wrote: > > TLMM controller base address is incorrect and will hang on some platforms. > > Fix by giving the correct address. > > Thanks, recheck the spec this looks correct. We should have tlmm reg > space here and not tlmm base which also contains xpu region (thus hang) > Aren't you reading the patch backwards? Afaict downstream the driver carries an offset of 0x100000, which we dropped as we upstreamed the driver. As such changing reg to 0x0f000000 should cause most gpio register accesses to fall outside the actual register window. Or perhaps I'm missing something here? Regards, Bjorn > Reviewed-by: Vinod Koul <vkoul@kernel.org> > Fixes: b7e8f433a673 ("arm64: dts: qcom: Add basic devicetree support for SM8350 SoC") > > > > > Signed-off-by: Katherine Perez <kaperez@linux.microsoft.com> > > --- > > arch/arm64/boot/dts/qcom/sm8350.dtsi | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/arch/arm64/boot/dts/qcom/sm8350.dtsi b/arch/arm64/boot/dts/qcom/sm8350.dtsi > > index d134280e2939..624d294612d8 100644 > > --- a/arch/arm64/boot/dts/qcom/sm8350.dtsi > > +++ b/arch/arm64/boot/dts/qcom/sm8350.dtsi > > @@ -960,9 +960,9 @@ spmi_bus: spmi@c440000 { > > #interrupt-cells = <4>; > > }; > > > > - tlmm: pinctrl@f100000 { > > + tlmm: pinctrl@f000000 { > > compatible = "qcom,sm8350-tlmm"; > > - reg = <0 0x0f100000 0 0x300000>; > > + reg = <0 0x0f000000 0 0x300000>; > > interrupts = <GIC_SPI 208 IRQ_TYPE_LEVEL_HIGH>; > > gpio-controller; > > #gpio-cells = <2>; > > -- > > 2.31.1 > > -- > ~Vinod
On Tue, Nov 30, 2021 at 09:15:29PM -0600, Bjorn Andersson wrote: > On Mon 22 Nov 22:03 CST 2021, Vinod Koul wrote: > > > On 22-11-21, 11:05, Katherine Perez wrote: > > > TLMM controller base address is incorrect and will hang on some platforms. > > > Fix by giving the correct address. > > > > Thanks, recheck the spec this looks correct. We should have tlmm reg > > space here and not tlmm base which also contains xpu region (thus hang) > > > > Aren't you reading the patch backwards? > > Afaict downstream the driver carries an offset of 0x100000, which we > dropped as we upstreamed the driver. As such changing reg to 0x0f000000 > should cause most gpio register accesses to fall outside the actual > register window. > > Or perhaps I'm missing something here? > > Regards, > Bjorn Hi Vinod, Gentle reminder about the response above. Without the change to the TLMM base address, my platform hangs. I have ensured that the pinctrl driver contains the patch that Bjorn has previously submitted here: https://lore.kernel.org/all/20211104170835.1993686-1-bjorn.andersson@linaro.org/ Best, Katherine > > > Reviewed-by: Vinod Koul <vkoul@kernel.org> > > Fixes: b7e8f433a673 ("arm64: dts: qcom: Add basic devicetree support for SM8350 SoC") > > > > > > > > Signed-off-by: Katherine Perez <kaperez@linux.microsoft.com> > > > --- > > > arch/arm64/boot/dts/qcom/sm8350.dtsi | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/arch/arm64/boot/dts/qcom/sm8350.dtsi b/arch/arm64/boot/dts/qcom/sm8350.dtsi > > > index d134280e2939..624d294612d8 100644 > > > --- a/arch/arm64/boot/dts/qcom/sm8350.dtsi > > > +++ b/arch/arm64/boot/dts/qcom/sm8350.dtsi > > > @@ -960,9 +960,9 @@ spmi_bus: spmi@c440000 { > > > #interrupt-cells = <4>; > > > }; > > > > > > - tlmm: pinctrl@f100000 { > > > + tlmm: pinctrl@f000000 { > > > compatible = "qcom,sm8350-tlmm"; > > > - reg = <0 0x0f100000 0 0x300000>; > > > + reg = <0 0x0f000000 0 0x300000>; > > > interrupts = <GIC_SPI 208 IRQ_TYPE_LEVEL_HIGH>; > > > gpio-controller; > > > #gpio-cells = <2>; > > > -- > > > 2.31.1 > > > > -- > > ~Vinod
On 30-11-21, 21:15, Bjorn Andersson wrote: > On Mon 22 Nov 22:03 CST 2021, Vinod Koul wrote: > > > On 22-11-21, 11:05, Katherine Perez wrote: > > > TLMM controller base address is incorrect and will hang on some platforms. > > > Fix by giving the correct address. > > > > Thanks, recheck the spec this looks correct. We should have tlmm reg > > space here and not tlmm base which also contains xpu region (thus hang) > > > > Aren't you reading the patch backwards? I guess :( > Afaict downstream the driver carries an offset of 0x100000, which we > dropped as we upstreamed the driver. As such changing reg to 0x0f000000 > should cause most gpio register accesses to fall outside the actual > register window. > > Or perhaps I'm missing something here? I relooked and XPU is at 0xF000000 and Reg at 0xF100000 So this patch should be dropped as such. The size mentioned in documentation is also correct Katherine, can you elaborate more on the hang you have observed? Any specific pins you use which causes this?
On Tue, Dec 07, 2021 at 05:16:14PM +0530, Vinod Koul wrote: > On 30-11-21, 21:15, Bjorn Andersson wrote: > > On Mon 22 Nov 22:03 CST 2021, Vinod Koul wrote: > > > > > On 22-11-21, 11:05, Katherine Perez wrote: > > > > TLMM controller base address is incorrect and will hang on some platforms. > > > > Fix by giving the correct address. > > > > > > Thanks, recheck the spec this looks correct. We should have tlmm reg > > > space here and not tlmm base which also contains xpu region (thus hang) > > > > > > > Aren't you reading the patch backwards? > > I guess :( > > > Afaict downstream the driver carries an offset of 0x100000, which we > > dropped as we upstreamed the driver. As such changing reg to 0x0f000000 > > should cause most gpio register accesses to fall outside the actual > > register window. > > > > Or perhaps I'm missing something here? > > I relooked and XPU is at 0xF000000 and Reg at 0xF100000 > So this patch should be dropped as such. The size mentioned in > documentation is also correct > > Katherine, can you elaborate more on the hang you have observed? Any > specific pins you use which causes this? Hi Vinod, Yes, it seems to hang in msm_pinctrl_probe. Specifically, line 734 in gpiolib.c: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpio/gpiolib.c#n734. On i=4, it hangs on assign_bit and the system goes into a reboot loop. When I set the TLMM address to f000000, I don't see this issue at all. > > > -- > ~Vinod
On Tue 07 Dec 18:21 PST 2021, Katherine Perez wrote: > On Tue, Dec 07, 2021 at 05:16:14PM +0530, Vinod Koul wrote: > > On 30-11-21, 21:15, Bjorn Andersson wrote: > > > On Mon 22 Nov 22:03 CST 2021, Vinod Koul wrote: > > > > > > > On 22-11-21, 11:05, Katherine Perez wrote: > > > > > TLMM controller base address is incorrect and will hang on some platforms. > > > > > Fix by giving the correct address. > > > > > > > > Thanks, recheck the spec this looks correct. We should have tlmm reg > > > > space here and not tlmm base which also contains xpu region (thus hang) > > > > > > > > > > Aren't you reading the patch backwards? > > > > I guess :( > > > > > Afaict downstream the driver carries an offset of 0x100000, which we > > > dropped as we upstreamed the driver. As such changing reg to 0x0f000000 > > > should cause most gpio register accesses to fall outside the actual > > > register window. > > > > > > Or perhaps I'm missing something here? > > > > I relooked and XPU is at 0xF000000 and Reg at 0xF100000 > > So this patch should be dropped as such. The size mentioned in > > documentation is also correct > > > > Katherine, can you elaborate more on the hang you have observed? Any > > specific pins you use which causes this? > > Hi Vinod, > > Yes, it seems to hang in msm_pinctrl_probe. Specifically, line 734 in > gpiolib.c: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpio/gpiolib.c#n734. > On i=4, it hangs on assign_bit and the system goes into a reboot loop. > When I set the TLMM address to f000000, I don't see this issue at all. > The cause for that is quite likely that gc->get_direction() will read the configuration from gpio<i>'s registers and gpio4 in your system is reserved for use by some trusted application. When you change the TLMM address you avoid this problem by just reading random registers outside the region that contains protected registers. Adjust the gpio-reserved-ranges in your device's tlmm node to mark gpio4 (probably 4 pins long) as "invalid", gpiolib will then not touch them. Regards, Bjorn > > > > > > -- > > ~Vinod
On Tue, Dec 07, 2021 at 06:35:41PM -0800, Bjorn Andersson wrote: > On Tue 07 Dec 18:21 PST 2021, Katherine Perez wrote: > > > On Tue, Dec 07, 2021 at 05:16:14PM +0530, Vinod Koul wrote: > > > On 30-11-21, 21:15, Bjorn Andersson wrote: > > > > On Mon 22 Nov 22:03 CST 2021, Vinod Koul wrote: > > > > > > > > > On 22-11-21, 11:05, Katherine Perez wrote: > > > > > > TLMM controller base address is incorrect and will hang on some platforms. > > > > > > Fix by giving the correct address. > > > > > > > > > > Thanks, recheck the spec this looks correct. We should have tlmm reg > > > > > space here and not tlmm base which also contains xpu region (thus hang) > > > > > > > > > > > > > Aren't you reading the patch backwards? > > > > > > I guess :( > > > > > > > Afaict downstream the driver carries an offset of 0x100000, which we > > > > dropped as we upstreamed the driver. As such changing reg to 0x0f000000 > > > > should cause most gpio register accesses to fall outside the actual > > > > register window. > > > > > > > > Or perhaps I'm missing something here? > > > > > > I relooked and XPU is at 0xF000000 and Reg at 0xF100000 > > > So this patch should be dropped as such. The size mentioned in > > > documentation is also correct > > > > > > Katherine, can you elaborate more on the hang you have observed? Any > > > specific pins you use which causes this? > > > > Hi Vinod, > > > > Yes, it seems to hang in msm_pinctrl_probe. Specifically, line 734 in > > gpiolib.c: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpio/gpiolib.c#n734. > > On i=4, it hangs on assign_bit and the system goes into a reboot loop. > > When I set the TLMM address to f000000, I don't see this issue at all. > > > > The cause for that is quite likely that gc->get_direction() will read > the configuration from gpio<i>'s registers and gpio4 in your system is > reserved for use by some trusted application. > > When you change the TLMM address you avoid this problem by just reading > random registers outside the region that contains protected registers. > > > Adjust the gpio-reserved-ranges in your device's tlmm node to mark gpio4 > (probably 4 pins long) as "invalid", gpiolib will then not touch them. > > Regards, > Bjorn Thanks, Bjorn. That makes sense. I'll resubmit with the changes to my device's TLMM node and will drop this patch. -Katherine > > > > > > > > > > -- > > > ~Vinod
diff --git a/arch/arm64/boot/dts/qcom/sm8350.dtsi b/arch/arm64/boot/dts/qcom/sm8350.dtsi index d134280e2939..624d294612d8 100644 --- a/arch/arm64/boot/dts/qcom/sm8350.dtsi +++ b/arch/arm64/boot/dts/qcom/sm8350.dtsi @@ -960,9 +960,9 @@ spmi_bus: spmi@c440000 { #interrupt-cells = <4>; }; - tlmm: pinctrl@f100000 { + tlmm: pinctrl@f000000 { compatible = "qcom,sm8350-tlmm"; - reg = <0 0x0f100000 0 0x300000>; + reg = <0 0x0f000000 0 0x300000>; interrupts = <GIC_SPI 208 IRQ_TYPE_LEVEL_HIGH>; gpio-controller; #gpio-cells = <2>;
TLMM controller base address is incorrect and will hang on some platforms. Fix by giving the correct address. Signed-off-by: Katherine Perez <kaperez@linux.microsoft.com> --- arch/arm64/boot/dts/qcom/sm8350.dtsi | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)