[PATCH] arm64: dts: sdm845: Add iommus property to qup1

[PATCH] arm64: dts: sdm845: Add iommus property to qup1

[Stephen Boyd]

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

Re: [PATCH] arm64: dts: sdm845: Add iommus property to qup1

[Bjorn Andersson]

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
> 

Re: [PATCH] arm64: dts: sdm845: Add iommus property to qup1

[Stephen Boyd]

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.

Re: [PATCH] arm64: dts: sdm845: Add iommus property to qup1

[Doug Anderson]

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

Re: [PATCH] arm64: dts: sdm845: Add iommus property to qup1

[Bjorn Andersson]

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

Re: [PATCH] arm64: dts: sdm845: Add iommus property to qup1

[Bjorn Andersson]

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

Re: [PATCH] arm64: dts: sdm845: Add iommus property to qup1

[Vivek Gautam]

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

Re: [PATCH] arm64: dts: sdm845: Add iommus property to qup1

[Stephen Boyd]

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?

Re: [PATCH] arm64: dts: sdm845: Add iommus property to qup1

[Vivek Gautam]

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

Re: [PATCH] arm64: dts: sdm845: Add iommus property to qup1

[Marc Gonzalez]

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.

Re: [PATCH] arm64: dts: sdm845: Add iommus property to qup1

[Stephen Boyd]

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.

Re: [PATCH] arm64: dts: sdm845: Add iommus property to qup1

[Vivek Gautam]

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.
>

Re: [PATCH] arm64: dts: sdm845: Add iommus property to qup1

[Stephen Boyd]

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?

Re: [PATCH] arm64: dts: sdm845: Add iommus property to qup1

[Stephen Boyd]

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?