LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] arm64: dts: sdm845: Add iommus property to qup1
@ 2019-06-04 22:29 Stephen Boyd
  2019-06-04 22:37 ` Bjorn Andersson
  0 siblings, 1 reply; 14+ messages in thread
From: Stephen Boyd @ 2019-06-04 22:29 UTC (permalink / raw)
  To: Andy Gross; +Cc: linux-kernel, linux-arm-msm, Sibi Sankar

The SMMU that sits in front of the QUP needs to be programmed properly
so that the i2c geni driver can allocate DMA descriptors. Failure to do
this leads to faults when using devices such as an i2c touchscreen where
the transaction is larger than 32 bytes and we use a DMA buffer.

 arm-smmu 15000000.iommu: Unexpected global fault, this could be serious
 arm-smmu 15000000.iommu:         GFSR 0x00000002, GFSYNR0 0x00000002, GFSYNR1 0x000006c0, GFSYNR2 0x00000000

Add the right SID and mask so this works.

Cc: Sibi Sankar <sibis@codeaurora.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 arch/arm64/boot/dts/qcom/sdm845.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index fcb93300ca62..2e57e861e17c 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -900,6 +900,7 @@
 			#address-cells = <2>;
 			#size-cells = <2>;
 			ranges;
+			iommus = <&apps_smmu 0x6c0 0x3>;
 			status = "disabled";
 
 			i2c8: i2c@a80000 {
-- 
Sent by a computer through tubes


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] arm64: dts: sdm845: Add iommus property to qup1
  2019-06-04 22:29 [PATCH] arm64: dts: sdm845: Add iommus property to qup1 Stephen Boyd
@ 2019-06-04 22:37 ` Bjorn Andersson
  2019-06-04 22:46   ` Stephen Boyd
  2019-06-04 22:48   ` Doug Anderson
  0 siblings, 2 replies; 14+ messages in thread
From: Bjorn Andersson @ 2019-06-04 22:37 UTC (permalink / raw)
  To: Stephen Boyd; +Cc: Andy Gross, linux-kernel, linux-arm-msm, Sibi Sankar

On Tue 04 Jun 15:29 PDT 2019, Stephen Boyd wrote:

> The SMMU that sits in front of the QUP needs to be programmed properly
> so that the i2c geni driver can allocate DMA descriptors. Failure to do
> this leads to faults when using devices such as an i2c touchscreen where
> the transaction is larger than 32 bytes and we use a DMA buffer.
> 

I'm pretty sure I've run into this problem, but before we marked the
smmu bypass_disable and as such didn't get the fault, thanks.

>  arm-smmu 15000000.iommu: Unexpected global fault, this could be serious
>  arm-smmu 15000000.iommu:         GFSR 0x00000002, GFSYNR0 0x00000002, GFSYNR1 0x000006c0, GFSYNR2 0x00000000
> 
> Add the right SID and mask so this works.
> 
> Cc: Sibi Sankar <sibis@codeaurora.org>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
>  arch/arm64/boot/dts/qcom/sdm845.dtsi | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> index fcb93300ca62..2e57e861e17c 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> @@ -900,6 +900,7 @@
>  			#address-cells = <2>;
>  			#size-cells = <2>;
>  			ranges;
> +			iommus = <&apps_smmu 0x6c0 0x3>;

According to the docs this stream belongs to TZ, the HLOS stream should
be 0x6c3.

Regards,
Bjorn

>  			status = "disabled";
>  
>  			i2c8: i2c@a80000 {
> -- 
> Sent by a computer through tubes
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] arm64: dts: sdm845: Add iommus property to qup1
  2019-06-04 22:37 ` Bjorn Andersson
@ 2019-06-04 22:46   ` Stephen Boyd
  2019-06-04 22:59     ` Bjorn Andersson
  2019-06-05  4:55     ` Vivek Gautam
  2019-06-04 22:48   ` Doug Anderson
  1 sibling, 2 replies; 14+ messages in thread
From: Stephen Boyd @ 2019-06-04 22:46 UTC (permalink / raw)
  To: Bjorn Andersson; +Cc: Andy Gross, linux-kernel, linux-arm-msm, Sibi Sankar

Quoting Bjorn Andersson (2019-06-04 15:37:00)
> On Tue 04 Jun 15:29 PDT 2019, Stephen Boyd wrote:
> 
> > The SMMU that sits in front of the QUP needs to be programmed properly
> > so that the i2c geni driver can allocate DMA descriptors. Failure to do
> > this leads to faults when using devices such as an i2c touchscreen where
> > the transaction is larger than 32 bytes and we use a DMA buffer.
> > 
> 
> I'm pretty sure I've run into this problem, but before we marked the
> smmu bypass_disable and as such didn't get the fault, thanks.
> 
> >  arm-smmu 15000000.iommu: Unexpected global fault, this could be serious
> >  arm-smmu 15000000.iommu:         GFSR 0x00000002, GFSYNR0 0x00000002, GFSYNR1 0x000006c0, GFSYNR2 0x00000000
> > 
> > Add the right SID and mask so this works.
> > 
> > Cc: Sibi Sankar <sibis@codeaurora.org>
> > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> > ---
> >  arch/arm64/boot/dts/qcom/sdm845.dtsi | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > index fcb93300ca62..2e57e861e17c 100644
> > --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > @@ -900,6 +900,7 @@
> >                       #address-cells = <2>;
> >                       #size-cells = <2>;
> >                       ranges;
> > +                     iommus = <&apps_smmu 0x6c0 0x3>;
> 
> According to the docs this stream belongs to TZ, the HLOS stream should
> be 0x6c3.

Aye, I saw this line in the downstream kernel but it doesn't work for
me. If I specify <&apps_smmu 0x6c3 0x0> it still blows up. I wonder if
my firmware perhaps is missing some initialization here to make the QUP
operate in HLOS mode? Otherwise, I thought that the 0x3 at the end was
the mask and so it should be split off to the second cell in the DT
specifier but that seemed a little weird.


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] arm64: dts: sdm845: Add iommus property to qup1
  2019-06-04 22:37 ` Bjorn Andersson
  2019-06-04 22:46   ` Stephen Boyd
@ 2019-06-04 22:48   ` Doug Anderson
  2019-06-04 22:56     ` Bjorn Andersson
  1 sibling, 1 reply; 14+ messages in thread
From: Doug Anderson @ 2019-06-04 22:48 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: Stephen Boyd, Andy Gross, LKML, linux-arm-msm, Sibi Sankar

Hi,

On Tue, Jun 4, 2019 at 3:37 PM Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
>
> On Tue 04 Jun 15:29 PDT 2019, Stephen Boyd wrote:
>
> > The SMMU that sits in front of the QUP needs to be programmed properly
> > so that the i2c geni driver can allocate DMA descriptors. Failure to do
> > this leads to faults when using devices such as an i2c touchscreen where
> > the transaction is larger than 32 bytes and we use a DMA buffer.
> >
>
> I'm pretty sure I've run into this problem, but before we marked the
> smmu bypass_disable and as such didn't get the fault, thanks.
>
> >  arm-smmu 15000000.iommu: Unexpected global fault, this could be serious
> >  arm-smmu 15000000.iommu:         GFSR 0x00000002, GFSYNR0 0x00000002, GFSYNR1 0x000006c0, GFSYNR2 0x00000000
> >
> > Add the right SID and mask so this works.
> >
> > Cc: Sibi Sankar <sibis@codeaurora.org>
> > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> > ---
> >  arch/arm64/boot/dts/qcom/sdm845.dtsi | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > index fcb93300ca62..2e57e861e17c 100644
> > --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > @@ -900,6 +900,7 @@
> >                       #address-cells = <2>;
> >                       #size-cells = <2>;
> >                       ranges;
> > +                     iommus = <&apps_smmu 0x6c0 0x3>;
>
> According to the docs this stream belongs to TZ, the HLOS stream should
> be 0x6c3.

Since you've already got the docs in front of you, how about looking
up the value for qup0 so we can fix both of 'em?

-Doug

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] arm64: dts: sdm845: Add iommus property to qup1
  2019-06-04 22:48   ` Doug Anderson
@ 2019-06-04 22:56     ` Bjorn Andersson
  0 siblings, 0 replies; 14+ messages in thread
From: Bjorn Andersson @ 2019-06-04 22:56 UTC (permalink / raw)
  To: Doug Anderson; +Cc: Stephen Boyd, Andy Gross, LKML, linux-arm-msm, Sibi Sankar

On Tue 04 Jun 15:48 PDT 2019, Doug Anderson wrote:

> Hi,
> 
> On Tue, Jun 4, 2019 at 3:37 PM Bjorn Andersson
> <bjorn.andersson@linaro.org> wrote:
> >
> > On Tue 04 Jun 15:29 PDT 2019, Stephen Boyd wrote:
> >
> > > The SMMU that sits in front of the QUP needs to be programmed properly
> > > so that the i2c geni driver can allocate DMA descriptors. Failure to do
> > > this leads to faults when using devices such as an i2c touchscreen where
> > > the transaction is larger than 32 bytes and we use a DMA buffer.
> > >
> >
> > I'm pretty sure I've run into this problem, but before we marked the
> > smmu bypass_disable and as such didn't get the fault, thanks.
> >
> > >  arm-smmu 15000000.iommu: Unexpected global fault, this could be serious
> > >  arm-smmu 15000000.iommu:         GFSR 0x00000002, GFSYNR0 0x00000002, GFSYNR1 0x000006c0, GFSYNR2 0x00000000
> > >
> > > Add the right SID and mask so this works.
> > >
> > > Cc: Sibi Sankar <sibis@codeaurora.org>
> > > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> > > ---
> > >  arch/arm64/boot/dts/qcom/sdm845.dtsi | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > > index fcb93300ca62..2e57e861e17c 100644
> > > --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > > +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > > @@ -900,6 +900,7 @@
> > >                       #address-cells = <2>;
> > >                       #size-cells = <2>;
> > >                       ranges;
> > > +                     iommus = <&apps_smmu 0x6c0 0x3>;
> >
> > According to the docs this stream belongs to TZ, the HLOS stream should
> > be 0x6c3.
> 
> Since you've already got the docs in front of you, how about looking
> up the value for qup0 so we can fix both of 'em?
> 

Good thinking. QUP_1 is at stream 0x3.

Regards,
Bjorn

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] arm64: dts: sdm845: Add iommus property to qup1
  2019-06-04 22:46   ` Stephen Boyd
@ 2019-06-04 22:59     ` Bjorn Andersson
  2019-06-05  4:55     ` Vivek Gautam
  1 sibling, 0 replies; 14+ messages in thread
From: Bjorn Andersson @ 2019-06-04 22:59 UTC (permalink / raw)
  To: Stephen Boyd, vivek.gautam
  Cc: Andy Gross, linux-kernel, linux-arm-msm, Sibi Sankar

On Tue 04 Jun 15:46 PDT 2019, Stephen Boyd wrote:

> Quoting Bjorn Andersson (2019-06-04 15:37:00)
> > On Tue 04 Jun 15:29 PDT 2019, Stephen Boyd wrote:
> > 
> > > The SMMU that sits in front of the QUP needs to be programmed properly
> > > so that the i2c geni driver can allocate DMA descriptors. Failure to do
> > > this leads to faults when using devices such as an i2c touchscreen where
> > > the transaction is larger than 32 bytes and we use a DMA buffer.
> > > 
> > 
> > I'm pretty sure I've run into this problem, but before we marked the
> > smmu bypass_disable and as such didn't get the fault, thanks.
> > 
> > >  arm-smmu 15000000.iommu: Unexpected global fault, this could be serious
> > >  arm-smmu 15000000.iommu:         GFSR 0x00000002, GFSYNR0 0x00000002, GFSYNR1 0x000006c0, GFSYNR2 0x00000000
> > > 
> > > Add the right SID and mask so this works.
> > > 
> > > Cc: Sibi Sankar <sibis@codeaurora.org>
> > > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> > > ---
> > >  arch/arm64/boot/dts/qcom/sdm845.dtsi | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > > index fcb93300ca62..2e57e861e17c 100644
> > > --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > > +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > > @@ -900,6 +900,7 @@
> > >                       #address-cells = <2>;
> > >                       #size-cells = <2>;
> > >                       ranges;
> > > +                     iommus = <&apps_smmu 0x6c0 0x3>;
> > 
> > According to the docs this stream belongs to TZ, the HLOS stream should
> > be 0x6c3.
> 
> Aye, I saw this line in the downstream kernel but it doesn't work for
> me. If I specify <&apps_smmu 0x6c3 0x0> it still blows up. I wonder if
> my firmware perhaps is missing some initialization here to make the QUP
> operate in HLOS mode? Otherwise, I thought that the 0x3 at the end was
> the mask and so it should be split off to the second cell in the DT
> specifier but that seemed a little weird.
> 

Both 0x3 and 0x6c3 are documented as the actual HLOS stream id, with
mask of 0x0. 

Looping in Vivek as well.

Regards,
Bjorn

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] arm64: dts: sdm845: Add iommus property to qup1
  2019-06-04 22:46   ` Stephen Boyd
  2019-06-04 22:59     ` Bjorn Andersson
@ 2019-06-05  4:55     ` Vivek Gautam
  2019-06-05 20:56       ` Stephen Boyd
  1 sibling, 1 reply; 14+ messages in thread
From: Vivek Gautam @ 2019-06-05  4:55 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Bjorn Andersson, Andy Gross, open list, linux-arm-msm, Sibi Sankar

On Wed, Jun 5, 2019 at 4:16 AM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Bjorn Andersson (2019-06-04 15:37:00)
> > On Tue 04 Jun 15:29 PDT 2019, Stephen Boyd wrote:
> >
> > > The SMMU that sits in front of the QUP needs to be programmed properly
> > > so that the i2c geni driver can allocate DMA descriptors. Failure to do
> > > this leads to faults when using devices such as an i2c touchscreen where
> > > the transaction is larger than 32 bytes and we use a DMA buffer.
> > >
> >
> > I'm pretty sure I've run into this problem, but before we marked the
> > smmu bypass_disable and as such didn't get the fault, thanks.
> >
> > >  arm-smmu 15000000.iommu: Unexpected global fault, this could be serious
> > >  arm-smmu 15000000.iommu:         GFSR 0x00000002, GFSYNR0 0x00000002, GFSYNR1 0x000006c0, GFSYNR2 0x00000000
> > >
> > > Add the right SID and mask so this works.
> > >
> > > Cc: Sibi Sankar <sibis@codeaurora.org>
> > > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> > > ---
> > >  arch/arm64/boot/dts/qcom/sdm845.dtsi | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > > index fcb93300ca62..2e57e861e17c 100644
> > > --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > > +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > > @@ -900,6 +900,7 @@
> > >                       #address-cells = <2>;
> > >                       #size-cells = <2>;
> > >                       ranges;
> > > +                     iommus = <&apps_smmu 0x6c0 0x3>;
> >
> > According to the docs this stream belongs to TZ, the HLOS stream should
> > be 0x6c3.
>
> Aye, I saw this line in the downstream kernel but it doesn't work for
> me. If I specify <&apps_smmu 0x6c3 0x0> it still blows up. I wonder if
> my firmware perhaps is missing some initialization here to make the QUP
> operate in HLOS mode? Otherwise, I thought that the 0x3 at the end was
> the mask and so it should be split off to the second cell in the DT
> specifier but that seemed a little weird.

Two things here -
0x6c0 - TZ SID. Do you see above fault on MTP sdm845 devices?
0x6c3/0x6c6 - HLOS SIDs.

Cheza will throw faults for anything that is programmed with TZ on mtp
as all of that should be handled in HLOS. The firmwares of all these
peripherals assume that the SID reservation is done (whether in TZ or HLOS).

I am inclined to moving the iommus property for all 'TZ' to board dts files.
MTP wouldn't need those SIDs. So, the SOC level dtsi will have just the
HLOS SIDs.

P.S.
As you rightly said, the second cell in iommus property is the mask so that
the iommu is able to reserve all that SIDs that are covered with the
starting SID
and the mask.


Best regards
Vivek
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] arm64: dts: sdm845: Add iommus property to qup1
  2019-06-05  4:55     ` Vivek Gautam
@ 2019-06-05 20:56       ` Stephen Boyd
  2019-06-06 11:17         ` Vivek Gautam
  0 siblings, 1 reply; 14+ messages in thread
From: Stephen Boyd @ 2019-06-05 20:56 UTC (permalink / raw)
  To: Vivek Gautam
  Cc: Bjorn Andersson, Andy Gross, open list, linux-arm-msm, Sibi Sankar

Quoting Vivek Gautam (2019-06-04 21:55:26)
> On Wed, Jun 5, 2019 at 4:16 AM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Quoting Bjorn Andersson (2019-06-04 15:37:00)
> > > On Tue 04 Jun 15:29 PDT 2019, Stephen Boyd wrote:
> > >
> > > > The SMMU that sits in front of the QUP needs to be programmed properly
> > > > so that the i2c geni driver can allocate DMA descriptors. Failure to do
> > > > this leads to faults when using devices such as an i2c touchscreen where
> > > > the transaction is larger than 32 bytes and we use a DMA buffer.
> > > >
> > >
> > > I'm pretty sure I've run into this problem, but before we marked the
> > > smmu bypass_disable and as such didn't get the fault, thanks.
> > >
> > > >  arm-smmu 15000000.iommu: Unexpected global fault, this could be serious
> > > >  arm-smmu 15000000.iommu:         GFSR 0x00000002, GFSYNR0 0x00000002, GFSYNR1 0x000006c0, GFSYNR2 0x00000000
> > > >
> > > > Add the right SID and mask so this works.
> > > >
> > > > Cc: Sibi Sankar <sibis@codeaurora.org>
> > > > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> > > > ---
> > > >  arch/arm64/boot/dts/qcom/sdm845.dtsi | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > > > index fcb93300ca62..2e57e861e17c 100644
> > > > --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > > > +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > > > @@ -900,6 +900,7 @@
> > > >                       #address-cells = <2>;
> > > >                       #size-cells = <2>;
> > > >                       ranges;
> > > > +                     iommus = <&apps_smmu 0x6c0 0x3>;
> > >
> > > According to the docs this stream belongs to TZ, the HLOS stream should
> > > be 0x6c3.
> >
> > Aye, I saw this line in the downstream kernel but it doesn't work for
> > me. If I specify <&apps_smmu 0x6c3 0x0> it still blows up. I wonder if
> > my firmware perhaps is missing some initialization here to make the QUP
> > operate in HLOS mode? Otherwise, I thought that the 0x3 at the end was
> > the mask and so it should be split off to the second cell in the DT
> > specifier but that seemed a little weird.
> 
> Two things here -
> 0x6c0 - TZ SID. Do you see above fault on MTP sdm845 devices?

No. I see the above fault on Cheza.

> 0x6c3/0x6c6 - HLOS SIDs.

Why are there two? I see some mentions of GSI mode near these SIDs so
maybe GSI has to be used for DMA here to get the above two SIDs at the
IOMMU? Otherwise if we do the non-GSI mode of DMA we're going to use the
"TZ" SID?

> 
> Cheza will throw faults for anything that is programmed with TZ on mtp
> as all of that should be handled in HLOS. The firmwares of all these
> peripherals assume that the SID reservation is done (whether in TZ or HLOS).
> 
> I am inclined to moving the iommus property for all 'TZ' to board dts files.
> MTP wouldn't need those SIDs. So, the SOC level dtsi will have just the
> HLOS SIDs.

So you're saying you'd like to have the SID be <&apps_smmu 0x6c3 0x0> in
the sdm845.dtsi file and then override this on Cheza because our SID is
different (possibly because we don't use GSI)? Why can't we program the
SID in Cheza firmware to match the "HLOS" SID of 0x6c3?

> 
> P.S.
> As you rightly said, the second cell in iommus property is the mask so that
> the iommu is able to reserve all that SIDs that are covered with the
> starting SID
> and the mask.
> 

Well if 0x6c6 is another possibility maybe it should be <&apps_smmu
0x6c0 0x7> to cover the 0x6c3 and 0x6c6 SIDs?


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] arm64: dts: sdm845: Add iommus property to qup1
  2019-06-05 20:56       ` Stephen Boyd
@ 2019-06-06 11:17         ` Vivek Gautam
  2019-06-06 14:12           ` Marc Gonzalez
  2019-06-10 23:21           ` Stephen Boyd
  0 siblings, 2 replies; 14+ messages in thread
From: Vivek Gautam @ 2019-06-06 11:17 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Bjorn Andersson, Andy Gross, open list, linux-arm-msm, Sibi Sankar

Hi Stephen,

On Thu, Jun 6, 2019 at 2:27 AM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Vivek Gautam (2019-06-04 21:55:26)
> > On Wed, Jun 5, 2019 at 4:16 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > >
> > > Quoting Bjorn Andersson (2019-06-04 15:37:00)
> > > > On Tue 04 Jun 15:29 PDT 2019, Stephen Boyd wrote:
> > > >
> > > > > The SMMU that sits in front of the QUP needs to be programmed properly
> > > > > so that the i2c geni driver can allocate DMA descriptors. Failure to do
> > > > > this leads to faults when using devices such as an i2c touchscreen where
> > > > > the transaction is larger than 32 bytes and we use a DMA buffer.
> > > > >
> > > >
> > > > I'm pretty sure I've run into this problem, but before we marked the
> > > > smmu bypass_disable and as such didn't get the fault, thanks.
> > > >
> > > > >  arm-smmu 15000000.iommu: Unexpected global fault, this could be serious
> > > > >  arm-smmu 15000000.iommu:         GFSR 0x00000002, GFSYNR0 0x00000002, GFSYNR1 0x000006c0, GFSYNR2 0x00000000
> > > > >
> > > > > Add the right SID and mask so this works.
> > > > >
> > > > > Cc: Sibi Sankar <sibis@codeaurora.org>
> > > > > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> > > > > ---
> > > > >  arch/arm64/boot/dts/qcom/sdm845.dtsi | 1 +
> > > > >  1 file changed, 1 insertion(+)
> > > > >
> > > > > diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > > > > index fcb93300ca62..2e57e861e17c 100644
> > > > > --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > > > > +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > > > > @@ -900,6 +900,7 @@
> > > > >                       #address-cells = <2>;
> > > > >                       #size-cells = <2>;
> > > > >                       ranges;
> > > > > +                     iommus = <&apps_smmu 0x6c0 0x3>;
> > > >
> > > > According to the docs this stream belongs to TZ, the HLOS stream should
> > > > be 0x6c3.
> > >
> > > Aye, I saw this line in the downstream kernel but it doesn't work for
> > > me. If I specify <&apps_smmu 0x6c3 0x0> it still blows up. I wonder if
> > > my firmware perhaps is missing some initialization here to make the QUP
> > > operate in HLOS mode? Otherwise, I thought that the 0x3 at the end was
> > > the mask and so it should be split off to the second cell in the DT
> > > specifier but that seemed a little weird.
> >
> > Two things here -
> > 0x6c0 - TZ SID. Do you see above fault on MTP sdm845 devices?
>
> No. I see the above fault on Cheza.

Right, expected.

>
> > 0x6c3/0x6c6 - HLOS SIDs.

My bad, the other SID is 0x6D6.

>
> Why are there two? I see some mentions of GSI mode near these SIDs so
> maybe GSI has to be used for DMA here to get the above two SIDs at the
> IOMMU? Otherwise if we do the non-GSI mode of DMA we're going to use the
> "TZ" SID?

Yea, one for GSI, and the other one for non-GSI DMA. I am unsure at this point
about the use of TZ SID, but i would assume this is the SID that's used by the
qup firmware, and therefore on MTP TZ programs this SID.

>
> >
> > Cheza will throw faults for anything that is programmed with TZ on mtp
> > as all of that should be handled in HLOS. The firmwares of all these
> > peripherals assume that the SID reservation is done (whether in TZ or HLOS).
> >
> > I am inclined to moving the iommus property for all 'TZ' to board dts files.
> > MTP wouldn't need those SIDs. So, the SOC level dtsi will have just the
> > HLOS SIDs.
>
> So you're saying you'd like to have the SID be <&apps_smmu 0x6c3 0x0> in
> the sdm845.dtsi file and then override this on Cheza because our SID is
> different (possibly because we don't use GSI)? Why can't we program the
> SID in Cheza firmware to match the "HLOS" SID of 0x6c3?

Sorry my bad, I missed the overriding part.
May be we add the lists of SIDs in board dts only. So, cheza dts will
have all these SIDs -
<&apps_smmu 0x6c0 0x3>   // for both 0x6c0 (TZ) and 0x6c3 (HLOS)
<&apps_smmu 0x6d6 0x0>   // if we want to use the GSI dma.
and
MTP will have
<&apps_smmu 0x6c3 0x0>
<&apps_smmu 0x6d6 0x0>
WDUT?

>
> >
> > P.S.
> > As you rightly said, the second cell in iommus property is the mask so that
> > the iommu is able to reserve all that SIDs that are covered with the
> > starting SID
> > and the mask.
> >
>
> Well if 0x6c6 is another possibility maybe it should be <&apps_smmu
> 0x6c0 0x7> to cover the 0x6c3 and 0x6c6 SIDs?

The other SID was 0x6D6.

Best regards
Vivek

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] arm64: dts: sdm845: Add iommus property to qup1
  2019-06-06 11:17         ` Vivek Gautam
@ 2019-06-06 14:12           ` Marc Gonzalez
  2019-06-10 23:21           ` Stephen Boyd
  1 sibling, 0 replies; 14+ messages in thread
From: Marc Gonzalez @ 2019-06-06 14:12 UTC (permalink / raw)
  To: Vivek Gautam, Stephen Boyd, Bjorn Andersson
  Cc: Andy Gross, LKML, MSM, Sibi Sankar, Douglas Anderson

On 06/06/2019 13:17, Vivek Gautam wrote:

> <&apps_smmu 0x6c0 0x3>   // for both 0x6c0 (TZ) and 0x6c3 (HLOS)

Another possibility is to list both:

	<&apps_smmu 0x6c0 0x0>
	<&apps_smmu 0x6c3 0x0>

which leaves 0x6c1 and 0x6c2 out of the picture, and makes 0x6c3
appear explicitly (for anyone checking the documentation).

Regards.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] arm64: dts: sdm845: Add iommus property to qup1
  2019-06-06 11:17         ` Vivek Gautam
  2019-06-06 14:12           ` Marc Gonzalez
@ 2019-06-10 23:21           ` Stephen Boyd
  2019-06-12  9:26             ` Vivek Gautam
  1 sibling, 1 reply; 14+ messages in thread
From: Stephen Boyd @ 2019-06-10 23:21 UTC (permalink / raw)
  To: Vivek Gautam
  Cc: Bjorn Andersson, Andy Gross, open list, linux-arm-msm, Sibi Sankar

Quoting Vivek Gautam (2019-06-06 04:17:16)
> Hi Stephen,
> 
> On Thu, Jun 6, 2019 at 2:27 AM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Quoting Vivek Gautam (2019-06-04 21:55:26)
> >
> > >
> > > Cheza will throw faults for anything that is programmed with TZ on mtp
> > > as all of that should be handled in HLOS. The firmwares of all these
> > > peripherals assume that the SID reservation is done (whether in TZ or HLOS).
> > >
> > > I am inclined to moving the iommus property for all 'TZ' to board dts files.
> > > MTP wouldn't need those SIDs. So, the SOC level dtsi will have just the
> > > HLOS SIDs.
> >
> > So you're saying you'd like to have the SID be <&apps_smmu 0x6c3 0x0> in
> > the sdm845.dtsi file and then override this on Cheza because our SID is
> > different (possibly because we don't use GSI)? Why can't we program the
> > SID in Cheza firmware to match the "HLOS" SID of 0x6c3?
> 
> Sorry my bad, I missed the overriding part.
> May be we add the lists of SIDs in board dts only. So, cheza dts will
> have all these SIDs -
> <&apps_smmu 0x6c0 0x3>   // for both 0x6c0 (TZ) and 0x6c3 (HLOS)
> <&apps_smmu 0x6d6 0x0>   // if we want to use the GSI dma.
> and
> MTP will have
> <&apps_smmu 0x6c3 0x0>
> <&apps_smmu 0x6d6 0x0>
> WDUT?

I'd prefer to fix the firmware so that the HLOS SID is used even on this
board. Making Cheza use something different from MTP doesn't sound so
good. Do you know how that works? Is there some configuration register
or something that I should be looking for to see why the SID is not the
HLOS one? It's definitely generating SIDs for the TZ SID (0x6c0), but
I'd like to make sure that we can't change it because it's tied to some
hardware signal like the NS bit and/or the Execution Level. Hopefully
it's a config and then our difference from MTP can be minimized.

As far as I can tell, HLOS on SDM845 has always used GPI (yet another
DMA engine) to do the DMA transfers. And the GPI is the hardware block
that uses the SID of 0x6d6 above, so putting that into iommus for the
geniqup doesn't make any sense given that GPI is another node. Can you
confirm this is the case? Furthermore, the SID of 0x6c3 sounds untested?
Has it ever been generated on SDM845 MTP?

If we ever support GPI, I'd expect to see something like this in DT:

	gpi_dma: gpi@a00000 {
		reg = <0x00a00000 0x60000>;
		iommus = <&apps_smmu 0x6d6 0x0>;
		...
	};

	geniqup@ac0000 {
		reg = <0x00ac0000 0x6000>;
		iommus = <&apps_smmu 0x6c3 0x0>;

		i2c@....{

			dmas = <&gpi_dma ....>;
		};

But now I'm worried that the geniqup needs the proper geniqup wrapper
clks to talk to it. Most likely the GPI is embedded inside the geniqup
wrapper and sits right next to the bus to do bus DMA mastering. From the
DT side, it means we should either put it inside the geniqup node, or we
should add the wrapper clks to the GPI node and hope things work out
with regards to clks and shared resources being used at the right time.

If we're left with trying to figure out how to express the different
SIDs depending on the CPU execution state then it may be easier to push
for GPI upstreaming and use that dma engine to "fold" the SID
numberspace into one SID for the GPI. This would avoid having to deal
with the HLOS vs. TZ SID problem by adding a whole other driver. Or we
could just rip out the non-GPI DMA support in this driver because the
SID is all confused.


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] arm64: dts: sdm845: Add iommus property to qup1
  2019-06-10 23:21           ` Stephen Boyd
@ 2019-06-12  9:26             ` Vivek Gautam
  2019-07-16 23:47               ` Stephen Boyd
  0 siblings, 1 reply; 14+ messages in thread
From: Vivek Gautam @ 2019-06-12  9:26 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Bjorn Andersson, Andy Gross, open list, linux-arm-msm, Sibi Sankar



On 6/11/2019 4:51 AM, Stephen Boyd wrote:
> Quoting Vivek Gautam (2019-06-06 04:17:16)
>> Hi Stephen,
>>
>> On Thu, Jun 6, 2019 at 2:27 AM Stephen Boyd <swboyd@chromium.org> wrote:
>>> Quoting Vivek Gautam (2019-06-04 21:55:26)
>>>
>>>> Cheza will throw faults for anything that is programmed with TZ on mtp
>>>> as all of that should be handled in HLOS. The firmwares of all these
>>>> peripherals assume that the SID reservation is done (whether in TZ or HLOS).
>>>>
>>>> I am inclined to moving the iommus property for all 'TZ' to board dts files.
>>>> MTP wouldn't need those SIDs. So, the SOC level dtsi will have just the
>>>> HLOS SIDs.
>>> So you're saying you'd like to have the SID be <&apps_smmu 0x6c3 0x0> in
>>> the sdm845.dtsi file and then override this on Cheza because our SID is
>>> different (possibly because we don't use GSI)? Why can't we program the
>>> SID in Cheza firmware to match the "HLOS" SID of 0x6c3?
>> Sorry my bad, I missed the overriding part.
>> May be we add the lists of SIDs in board dts only. So, cheza dts will
>> have all these SIDs -
>> <&apps_smmu 0x6c0 0x3>   // for both 0x6c0 (TZ) and 0x6c3 (HLOS)
>> <&apps_smmu 0x6d6 0x0>   // if we want to use the GSI dma.
>> and
>> MTP will have
>> <&apps_smmu 0x6c3 0x0>
>> <&apps_smmu 0x6d6 0x0>
>> WDUT?
> I'd prefer to fix the firmware so that the HLOS SID is used even on this
> board. Making Cheza use something different from MTP doesn't sound so
> good. Do you know how that works? Is there some configuration register
> or something that I should be looking for to see why the SID is not the
> HLOS one? It's definitely generating SIDs for the TZ SID (0x6c0), but
> I'd like to make sure that we can't change it because it's tied to some
> hardware signal like the NS bit and/or the Execution Level. Hopefully
> it's a config and then our difference from MTP can be minimized.

I don't think SMMU limits any such programming of SIDs. It's a design 
decision
to program few SIDs in TZ/Hyp and allocate the corresponding context banks
and create respective mappings. You should be able to see these SMR 
configurations
before kernel boots up. I use a simple T32 command -

smmu.add "<name>" <smmu_type> <base_address>
smmu.streammaptable <name>

e.g. for sdm845 apps_smmu

smmu.add "apps" MMU500 a:0x15000000
smmu.StreamMapTable apps

This dumps all the information regarding the smmu.

>
> As far as I can tell, HLOS on SDM845 has always used GPI (yet another
> DMA engine) to do the DMA transfers. And the GPI is the hardware block
> that uses the SID of 0x6d6 above, so putting that into iommus for the
> geniqup doesn't make any sense given that GPI is another node. Can you
> confirm this is the case? Furthermore, the SID of 0x6c3 sounds untested?
> Has it ever been generated on SDM845 MTP?

I will get back with this information.

BRs
Vivek

>
> If we ever support GPI, I'd expect to see something like this in DT:
>
> 	gpi_dma: gpi@a00000 {
> 		reg = <0x00a00000 0x60000>;
> 		iommus = <&apps_smmu 0x6d6 0x0>;
> 		...
> 	};
>
> 	geniqup@ac0000 {
> 		reg = <0x00ac0000 0x6000>;
> 		iommus = <&apps_smmu 0x6c3 0x0>;
>
> 		i2c@....{
>
> 			dmas = <&gpi_dma ....>;
> 		};
>
> But now I'm worried that the geniqup needs the proper geniqup wrapper
> clks to talk to it. Most likely the GPI is embedded inside the geniqup
> wrapper and sits right next to the bus to do bus DMA mastering. From the
> DT side, it means we should either put it inside the geniqup node, or we
> should add the wrapper clks to the GPI node and hope things work out
> with regards to clks and shared resources being used at the right time.
>
> If we're left with trying to figure out how to express the different
> SIDs depending on the CPU execution state then it may be easier to push
> for GPI upstreaming and use that dma engine to "fold" the SID
> numberspace into one SID for the GPI. This would avoid having to deal
> with the HLOS vs. TZ SID problem by adding a whole other driver. Or we
> could just rip out the non-GPI DMA support in this driver because the
> SID is all confused.
>


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] arm64: dts: sdm845: Add iommus property to qup1
  2019-06-12  9:26             ` Vivek Gautam
@ 2019-07-16 23:47               ` Stephen Boyd
  2019-08-30 22:03                 ` Stephen Boyd
  0 siblings, 1 reply; 14+ messages in thread
From: Stephen Boyd @ 2019-07-16 23:47 UTC (permalink / raw)
  To: Vivek Gautam
  Cc: Bjorn Andersson, Andy Gross, open list, linux-arm-msm, Sibi Sankar

Quoting Vivek Gautam (2019-06-12 02:26:20)
> 
> 
> On 6/11/2019 4:51 AM, Stephen Boyd wrote:
> > hardware signal like the NS bit and/or the Execution Level. Hopefully
> > it's a config and then our difference from MTP can be minimized.
> 
> I don't think SMMU limits any such programming of SIDs. It's a design 
> decision
> to program few SIDs in TZ/Hyp and allocate the corresponding context banks
> and create respective mappings. You should be able to see these SMR 
> configurations
> before kernel boots up. I use a simple T32 command -
> 
> smmu.add "<name>" <smmu_type> <base_address>
> smmu.streammaptable <name>
> 
> e.g. for sdm845 apps_smmu
> 
> smmu.add "apps" MMU500 a:0x15000000
> smmu.StreamMapTable apps
> 
> This dumps all the information regarding the smmu.

Preferably I can see this by using devmem instead of jtag and T32. Do
you know the address of the register? Otherwise I'll go dig into the
documentation and try to figure it out.

> 
> >
> > As far as I can tell, HLOS on SDM845 has always used GPI (yet another
> > DMA engine) to do the DMA transfers. And the GPI is the hardware block
> > that uses the SID of 0x6d6 above, so putting that into iommus for the
> > geniqup doesn't make any sense given that GPI is another node. Can you
> > confirm this is the case? Furthermore, the SID of 0x6c3 sounds untested?
> > Has it ever been generated on SDM845 MTP?
> 
> I will get back with this information.
> 

Any update?

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] arm64: dts: sdm845: Add iommus property to qup1
  2019-07-16 23:47               ` Stephen Boyd
@ 2019-08-30 22:03                 ` Stephen Boyd
  0 siblings, 0 replies; 14+ messages in thread
From: Stephen Boyd @ 2019-08-30 22:03 UTC (permalink / raw)
  To: Vivek Gautam
  Cc: Bjorn Andersson, Andy Gross, open list, linux-arm-msm, Sibi Sankar

Quoting Stephen Boyd (2019-07-16 16:47:07)
> Quoting Vivek Gautam (2019-06-12 02:26:20)
> > 
> > 
> > On 6/11/2019 4:51 AM, Stephen Boyd wrote:
> > > hardware signal like the NS bit and/or the Execution Level. Hopefully
> > > it's a config and then our difference from MTP can be minimized.
> > 
> > I don't think SMMU limits any such programming of SIDs. It's a design 
> > decision
> > to program few SIDs in TZ/Hyp and allocate the corresponding context banks
> > and create respective mappings. You should be able to see these SMR 
> > configurations
> > before kernel boots up. I use a simple T32 command -
> > 
> > smmu.add "<name>" <smmu_type> <base_address>
> > smmu.streammaptable <name>
> > 
> > e.g. for sdm845 apps_smmu
> > 
> > smmu.add "apps" MMU500 a:0x15000000
> > smmu.StreamMapTable apps
> > 
> > This dumps all the information regarding the smmu.
> 
> Preferably I can see this by using devmem instead of jtag and T32. Do
> you know the address of the register? Otherwise I'll go dig into the
> documentation and try to figure it out.

And this won't really help me will it? I thought the stream ID was "fixed" by
hardware, so it seems sort of weird that we're talking about limiting it in
TZ/hyp. Here's the S2CR table from Cheza in case that is useful.

localhost ~ # mem
Welcome to mem interactive mode (featuring python!)

Available functions:
- r(addr) # Read a single 32-bit word (little endian).
- rm(addr, nbytes) # Read memory at the given addr as a string.
- w(addr, val) # Write a single 32-bit word (little endian).
- wm(addr, val) # Write a string to memory at the given addr.
>>> for x in range(64):
...     r(0x15000c00 + (x << 2))
...
0x00000000
0x00000000
0x00000001
0x00000002
0x00000003
0x00000004
0x00000005
0x00020000
0x00020000
0x00020000
0x00020000
0x00020000
0x00020000
0x00020000
0x00020000
0x00020000
0x00020000
0x00020000
0x00020000
0x00020000
0x00020000
0x00020000
0x00020000
0x00020000
0x00020000
0x00020000
0x00020000
0x00020000
0x00020000
0x00020000
0x00020000
0x00020000
0x00020000
0x00020000
0x00020000
0x00020000
0x00020000
0x00020000
0x00020000
0x00020000
0x00020000
0x00020000
0x00020000
0x00020000
0x00020000
0x00020000
0x00020000
0x00020000
0x00020000
0x00020000
0x00020000
0x00020000
0x00020000
0x00020000
0x00020000
0x00020000
0x00020000
0x00020000
0x00020000
0x00020000
0x00020000
0x00020000
0x00020000
0x00020000

> 
> > 
> > >
> > > As far as I can tell, HLOS on SDM845 has always used GPI (yet another
> > > DMA engine) to do the DMA transfers. And the GPI is the hardware block
> > > that uses the SID of 0x6d6 above, so putting that into iommus for the
> > > geniqup doesn't make any sense given that GPI is another node. Can you
> > > confirm this is the case? Furthermore, the SID of 0x6c3 sounds untested?
> > > Has it ever been generated on SDM845 MTP?
> > 
> > I will get back with this information.
> > 
> 
> Any update?

Any news?


^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2019-08-30 22:03 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-04 22:29 [PATCH] arm64: dts: sdm845: Add iommus property to qup1 Stephen Boyd
2019-06-04 22:37 ` Bjorn Andersson
2019-06-04 22:46   ` Stephen Boyd
2019-06-04 22:59     ` Bjorn Andersson
2019-06-05  4:55     ` Vivek Gautam
2019-06-05 20:56       ` Stephen Boyd
2019-06-06 11:17         ` Vivek Gautam
2019-06-06 14:12           ` Marc Gonzalez
2019-06-10 23:21           ` Stephen Boyd
2019-06-12  9:26             ` Vivek Gautam
2019-07-16 23:47               ` Stephen Boyd
2019-08-30 22:03                 ` Stephen Boyd
2019-06-04 22:48   ` Doug Anderson
2019-06-04 22:56     ` Bjorn Andersson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).