[PATCH 1/1] iommu/arm-smmu: Implement qcom,skip-init

[PATCH 1/1] iommu/arm-smmu: Implement qcom,skip-init

[Konrad Dybcio]

This adds the downstream property required to support
SMMUs on SDM630 and other platforms (the need for it
most likely depends on firmware configuration).

Signed-off-by: Konrad Dybcio <konradybcio@gmail.com>
---
 .../devicetree/bindings/iommu/arm,smmu.yaml       | 10 ++++++++++
 drivers/iommu/arm-smmu.c                          | 15 +++++++++------
 2 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
index d7ceb4c34423..9abd6d41a32c 100644
--- a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
+++ b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
@@ -102,6 +102,16 @@ properties:
       access to SMMU configuration registers. In this case non-secure aliases of
       secure registers have to be used during SMMU configuration.
 
+  qcom,skip-init:
+    description: |
+      Disable resetting configuration for all context banks
+      during device reset.  This is useful for targets where
+      some context banks are dedicated to other execution
+      environments outside of Linux and those other EEs are
+      programming their own stream match tables, SCTLR, etc.
+      Without setting this option we will trample on their
+      configuration.
+
   stream-match-mask:
     $ref: /schemas/types.yaml#/definitions/uint32
     description: |
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 243bc4cb2705..a5c623d4caf9 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1655,13 +1655,16 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
 	 * Reset stream mapping groups: Initial values mark all SMRn as
 	 * invalid and all S2CRn as bypass unless overridden.
 	 */
-	for (i = 0; i < smmu->num_mapping_groups; ++i)
-		arm_smmu_write_sme(smmu, i);
 
-	/* Make sure all context banks are disabled and clear CB_FSR  */
-	for (i = 0; i < smmu->num_context_banks; ++i) {
-		arm_smmu_write_context_bank(smmu, i);
-		arm_smmu_cb_write(smmu, i, ARM_SMMU_CB_FSR, ARM_SMMU_FSR_FAULT);
+	if (!of_find_property(smmu->dev->of_node, "qcom,skip-init", NULL)) {
+		for (i = 0; i < smmu->num_mapping_groups; ++i)
+			arm_smmu_write_sme(smmu, i);
+
+		/* Make sure all context banks are disabled and clear CB_FSR  */
+		for (i = 0; i < smmu->num_context_banks; ++i) {
+			arm_smmu_write_context_bank(smmu, i);
+			arm_smmu_cb_write(smmu, i, ARM_SMMU_CB_FSR, ARM_SMMU_FSR_FAULT);
+		}
 	}
 
 	/* Invalidate the TLB, just in case */
-- 
2.27.0

Re: [PATCH 1/1] iommu/arm-smmu: Implement qcom,skip-init

[Will Deacon]

[Adding Bjorn, Jordan and John because I really don't want a bunch of
different ways to tell the driver that the firmware is screwing things up]

On Sat, Jul 04, 2020 at 02:28:09PM +0200, Konrad Dybcio wrote:
> This adds the downstream property required to support
> SMMUs on SDM630 and other platforms (the need for it
> most likely depends on firmware configuration).
> 
> Signed-off-by: Konrad Dybcio <konradybcio@gmail.com>
> ---
>  .../devicetree/bindings/iommu/arm,smmu.yaml       | 10 ++++++++++
>  drivers/iommu/arm-smmu.c                          | 15 +++++++++------
>  2 files changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
> index d7ceb4c34423..9abd6d41a32c 100644
> --- a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
> +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
> @@ -102,6 +102,16 @@ properties:
>        access to SMMU configuration registers. In this case non-secure aliases of
>        secure registers have to be used during SMMU configuration.
>  
> +  qcom,skip-init:
> +    description: |
> +      Disable resetting configuration for all context banks
> +      during device reset.  This is useful for targets where
> +      some context banks are dedicated to other execution
> +      environments outside of Linux and those other EEs are
> +      programming their own stream match tables, SCTLR, etc.
> +      Without setting this option we will trample on their
> +      configuration.

It would probably be better to know _which_ context banks we shouldn't
touch, no? Otherwise what happens to the others?

> +
>    stream-match-mask:
>      $ref: /schemas/types.yaml#/definitions/uint32
>      description: |
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 243bc4cb2705..a5c623d4caf9 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -1655,13 +1655,16 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
>  	 * Reset stream mapping groups: Initial values mark all SMRn as
>  	 * invalid and all S2CRn as bypass unless overridden.
>  	 */
> -	for (i = 0; i < smmu->num_mapping_groups; ++i)
> -		arm_smmu_write_sme(smmu, i);
>  
> -	/* Make sure all context banks are disabled and clear CB_FSR  */
> -	for (i = 0; i < smmu->num_context_banks; ++i) {
> -		arm_smmu_write_context_bank(smmu, i);
> -		arm_smmu_cb_write(smmu, i, ARM_SMMU_CB_FSR, ARM_SMMU_FSR_FAULT);
> +	if (!of_find_property(smmu->dev->of_node, "qcom,skip-init", NULL)) {
> +		for (i = 0; i < smmu->num_mapping_groups; ++i)
> +			arm_smmu_write_sme(smmu, i);
> +
> +		/* Make sure all context banks are disabled and clear CB_FSR  */
> +		for (i = 0; i < smmu->num_context_banks; ++i) {
> +			arm_smmu_write_context_bank(smmu, i);
> +			arm_smmu_cb_write(smmu, i, ARM_SMMU_CB_FSR, ARM_SMMU_FSR_FAULT);
> +		}
>  	}

Do we not need to worry about the SMRs as well?

Will

Re: [PATCH 1/1] iommu/arm-smmu: Implement qcom,skip-init

[Konrad Dybcio]

> It would probably be better to know _which_ context banks we shouldn't
> touch, no? Otherwise what happens to the others?

> Do we not need to worry about the SMRs as well?

This was mimicked from CAF (think [1]) and the SMMUs don't make the
hypervisor angry anymore, so I wouldn't be too picky on that if it works..


[1] https://github.com/sonyxperiadev/kernel/blob/aosp/LA.UM.7.1.r1/drivers/iommu/arm-smmu.c#L4104-L4109

Regards
Konrad

Re: [PATCH 1/1] iommu/arm-smmu: Implement qcom,skip-init

[Bjorn Andersson]

On Sat 04 Jul 06:09 PDT 2020, Will Deacon wrote:

> [Adding Bjorn, Jordan and John because I really don't want a bunch of
> different ways to tell the driver that the firmware is screwing things up]
> 

Thanks Will.

> On Sat, Jul 04, 2020 at 02:28:09PM +0200, Konrad Dybcio wrote:
> > This adds the downstream property required to support
> > SMMUs on SDM630 and other platforms (the need for it
> > most likely depends on firmware configuration).
> > 
> > Signed-off-by: Konrad Dybcio <konradybcio@gmail.com>
> > ---
> >  .../devicetree/bindings/iommu/arm,smmu.yaml       | 10 ++++++++++
> >  drivers/iommu/arm-smmu.c                          | 15 +++++++++------
> >  2 files changed, 19 insertions(+), 6 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
> > index d7ceb4c34423..9abd6d41a32c 100644
> > --- a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
> > +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
> > @@ -102,6 +102,16 @@ properties:
> >        access to SMMU configuration registers. In this case non-secure aliases of
> >        secure registers have to be used during SMMU configuration.
> >  
> > +  qcom,skip-init:
> > +    description: |
> > +      Disable resetting configuration for all context banks
> > +      during device reset.  This is useful for targets where
> > +      some context banks are dedicated to other execution
> > +      environments outside of Linux and those other EEs are
> > +      programming their own stream match tables, SCTLR, etc.
> > +      Without setting this option we will trample on their
> > +      configuration.
> 
> It would probably be better to know _which_ context banks we shouldn't
> touch, no? Otherwise what happens to the others?
> 

From my investigations of the issue of maintaining the boot display
through the initialization of arm-smmu I assume the reason for skipping
this step don't want to flush out the SMR, S2CR and context bank
initialization because it would disrupt the display hardware's access to
memory.

And in itself I believe that this is quite certainly going to work -
until you start attaching devices. Because in itself this does nothing
to ensure that the driver won't overwrite stream mapping or context bank
configuration as devices are attached.

So on e.g. SDM845 we need to ensure that the driver doesn't stomp over
the display mapping left by the bootloader.


Further more, on platforms such as QCS405 (which are derived from
platforms supported by qcom_iommu today), the stream mapping registers
(SMR and S2CR) are write ignore, which means that without knowledge
about the existing mappings the hardware and driver will be out of sync.

NB. Compared to the platforms that is supported by qcom_iommu, the
stream mapping registers are readable on these newer platforms, while on
e.g. MSM8916 we get an access violation by attempting to read SMR/S2CR.

> > +
> >    stream-match-mask:
> >      $ref: /schemas/types.yaml#/definitions/uint32
> >      description: |
> > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> > index 243bc4cb2705..a5c623d4caf9 100644
> > --- a/drivers/iommu/arm-smmu.c
> > +++ b/drivers/iommu/arm-smmu.c
> > @@ -1655,13 +1655,16 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
> >  	 * Reset stream mapping groups: Initial values mark all SMRn as
> >  	 * invalid and all S2CRn as bypass unless overridden.
> >  	 */
> > -	for (i = 0; i < smmu->num_mapping_groups; ++i)
> > -		arm_smmu_write_sme(smmu, i);
> >  
> > -	/* Make sure all context banks are disabled and clear CB_FSR  */
> > -	for (i = 0; i < smmu->num_context_banks; ++i) {
> > -		arm_smmu_write_context_bank(smmu, i);
> > -		arm_smmu_cb_write(smmu, i, ARM_SMMU_CB_FSR, ARM_SMMU_FSR_FAULT);
> > +	if (!of_find_property(smmu->dev->of_node, "qcom,skip-init", NULL)) {
> > +		for (i = 0; i < smmu->num_mapping_groups; ++i)
> > +			arm_smmu_write_sme(smmu, i);
> > +
> > +		/* Make sure all context banks are disabled and clear CB_FSR  */
> > +		for (i = 0; i < smmu->num_context_banks; ++i) {
> > +			arm_smmu_write_context_bank(smmu, i);
> > +			arm_smmu_cb_write(smmu, i, ARM_SMMU_CB_FSR, ARM_SMMU_FSR_FAULT);
> > +		}
> >  	}
> 
> Do we not need to worry about the SMRs as well?
> 

I don't think we should skip the actual initialization, because to avoid
strange side effects we need to ensure that the driver and hardware are
in sync (either for specific streams/banks or for all of them).

I've continued my work on supporting boot display on e.g. SDM845, based
on Thierry's patches, but still have some unresolved corner cases to
fully resolve - e.g. how to ensure that the display hardware's stream
mapping survives the probe deferral of the display driver. Hopefully I
will be able to post something in a few days.



That said, there's a generation of platforms between MSM8916 (which we
support using qcom_iommu) and SDM845 (which can run with arm-smmu).
AngeloGioacchino proposed a series last year to extend the qcom_iommu to
support these [1]. If SD630 falls in this category, or in the newer
SDM845/SM8150 category I don't know.

It would be quite interesting to hear more about the exact behaviors
seems on SDM630, to see how we can support this as well.

[1] https://lore.kernel.org/linux-arm-msm/20191001155641.37117-1-kholk11@gmail.com/

Regards,
Bjorn

Re: [PATCH 1/1] iommu/arm-smmu: Implement qcom,skip-init

[Konrad Dybcio]

So.. is this a no-no?

I of course would like to omit this entirely, but SMMUs on sdm630 and
friends are REALLY picky.. What seems to happen is that when the
driver tries to do things the "standard" way, hypervisor decides to
hang the platform or force a reboot. Not very usable.


This thing is needed for the platform to even boot properly and one
more [1] is required to make mdss work with video mode panels (the
fact that CMD-mode panels work is kinda hilarious to me).

To be honest, there are even more qcom quirks (of which at least
qcom,dynamic and qcom-use-3-lvl-tables are used on 630).. [2]

Looking forward to your answers and possibly better solutions.

[1] https://github.com/konradybcio/linux/commit/83ac38af259968f92b6a8b7eab90096c78469f87
[2] https://github.com/sonyxperiadev/kernel/blob/aosp/LA.UM.7.1.r1/drivers/iommu/arm-smmu.c#L404-L415

Regards
Konrad

Re: [PATCH 1/1] iommu/arm-smmu: Implement qcom,skip-init

[Jordan Crouse]

On Tue, Jul 21, 2020 at 05:04:11PM +0200, Konrad Dybcio wrote:
> So.. is this a no-no?
> 
> I of course would like to omit this entirely, but SMMUs on sdm630 and
> friends are REALLY picky.. What seems to happen is that when the
> driver tries to do things the "standard" way, hypervisor decides to
> hang the platform or force a reboot. Not very usable.
> 
> 
> This thing is needed for the platform to even boot properly and one
> more [1] is required to make mdss work with video mode panels (the
> fact that CMD-mode panels work is kinda hilarious to me).
> 
> To be honest, there are even more qcom quirks (of which at least
> qcom,dynamic and qcom-use-3-lvl-tables are used on 630).. [2]
> 
> Looking forward to your answers and possibly better solutions.

Nobody is disputing that the qcom SMMUs don't have their share of quirks but it
seems that the community has mostly settled on the agreement that there are
better ways to solve this than a handful of device tree properties. The current
focus has been on moving more of the SMMU specific bits into the arm-smmu-qcom
implementation [1] and I think that is the right way to go.

As for the other quirks we can probably discuss those on a case by case basis.
I doubt you will find much enthusiasm for qcom,use-3-lvl-tables and I've been
working on replacing qcom,dynamic with something much better [2].

[1] https://lists.linuxfoundation.org/pipermail/iommu/2020-July/046304.html
[2] https://lists.linuxfoundation.org/pipermail/iommu/2020-July/046756.html

Jordan

> [1] https://github.com/konradybcio/linux/commit/83ac38af259968f92b6a8b7eab90096c78469f87
> [2] https://github.com/sonyxperiadev/kernel/blob/aosp/LA.UM.7.1.r1/drivers/iommu/arm-smmu.c#L404-L415
> 
> Regards
> Konrad

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 1/1] iommu/arm-smmu: Implement qcom,skip-init

[Konrad Dybcio]

>The current
>focus has been on moving more of the SMMU specific bits into the arm-smmu-qcom
>implementation [1] and I think that is the right way to go.

Pardon if I overlooked something obvious, but I can't seem to find a
clean way for implementing qcom,skip-init in arm-smmu-qcom, as neither
the arm_smmu_test_smr_masks nor the probe function seem to be
alterable with arm_smmu_impl. I'm open to your ideas guys.

Konrad

Re: [PATCH 1/1] iommu/arm-smmu: Implement qcom,skip-init

[Bjorn Andersson]

On Tue 21 Jul 09:20 PDT 2020, Konrad Dybcio wrote:

> >The current
> >focus has been on moving more of the SMMU specific bits into the arm-smmu-qcom
> >implementation [1] and I think that is the right way to go.
> 
> Pardon if I overlooked something obvious, but I can't seem to find a
> clean way for implementing qcom,skip-init in arm-smmu-qcom, as neither
> the arm_smmu_test_smr_masks nor the probe function seem to be
> alterable with arm_smmu_impl. I'm open to your ideas guys.
> 

Is the problem on SDM630 that when you write to SMR/S2CR the device
reboots? Or that when you start writing out the context bank
configuration that trips the display and the device reboots?

Regards,
Bjorn

Re: [PATCH 1/1] iommu/arm-smmu: Implement qcom,skip-init

[Konrad Dybcio]

>Is the problem on SDM630 that when you write to SMR/S2CR the device
>reboots? Or that when you start writing out the context bank
>configuration that trips the display and the device reboots?

I added some debug prints and the phone hangs after reaching the
seventh CB (with i=6) at

arm_smmu_cb_write(smmu, i, ARM_SMMU_CB_FSR, ARM_SMMU_FSR_FAULT);

line in arm_smmu_device_reset.

Konrad

Re: [PATCH 1/1] iommu/arm-smmu: Implement qcom,skip-init

[Bjorn Andersson]

On Wed 22 Jul 13:11 PDT 2020, Konrad Dybcio wrote:

> >Is the problem on SDM630 that when you write to SMR/S2CR the device
> >reboots? Or that when you start writing out the context bank
> >configuration that trips the display and the device reboots?
> 
> I added some debug prints and the phone hangs after reaching the
> seventh CB (with i=6) at
> 
> arm_smmu_cb_write(smmu, i, ARM_SMMU_CB_FSR, ARM_SMMU_FSR_FAULT);
> 
> line in arm_smmu_device_reset.
> 

Sounds like things are progressing nicely for a while there, presumably
until the next time the display is being refreshed.

Would you be willing to try out the following work in progress:
https://lore.kernel.org/linux-arm-msm/20200717001619.325317-1-bjorn.andersson@linaro.org/

You need to adjust drivers/iommu/arm-smmu-impl.c so that
arm_smmu_impl_init() will invoke qcom_smmu_impl_init() as it spots your
apps smmu.

Regards,
Bjorn

Re: [PATCH 1/1] iommu/arm-smmu: Implement qcom,skip-init

[Konrad Dybcio]

> Sounds like things are progressing nicely for a while there, presumably
> until the next time the display is being refreshed.
>
> Would you be willing to try out the following work in progress:
> https://lore.kernel.org/linux-arm-msm/20200717001619.325317-1-bjorn.andersson@linaro.org/

I sure would like to if you could be kind enough to tell me which tree
I should apply it against. Latest -next brought some changes to
drivers/iommu/ structure which makes this not apply at all :/

Konrad