On 2022-06-08 11:27, AngeloGioacchino Del Regno wrote:
Il 06/06/22 00:06, Marijn Suijten ha scritto:
On 2022-05-31 16:55:59, Will Deacon wrote:
On Fri, May 27, 2022 at 11:28:57PM +0200, Konrad Dybcio wrote:
From: AngeloGioacchino Del Regno
<angelogioacchino.delregno@xxxxxxxxxxxxxx>
As also stated in the arm-smmu driver, we must write the TCR before
writing the TTBRs, since the TCR determines the access behavior of
some fields.
Where is this stated in the arm-smmu driver?
Signed-off-by: AngeloGioacchino Del Regno
<angelogioacchino.delregno@xxxxxxxxxxxxxx>
Signed-off-by: Marijn Suijten <marijn.suijten@xxxxxxxxxxxxxx>
Signed-off-by: Konrad Dybcio <konrad.dybcio@xxxxxxxxxxxxxx>
---
drivers/iommu/arm/arm-smmu/qcom_iommu.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
index 1728d4d7fe25..75f353866c40 100644
--- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
+++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
@@ -273,18 +273,18 @@ static int qcom_iommu_init_domain(struct
iommu_domain *domain,
ctx->secure_init = true;
}
- /* TTBRs */
- iommu_writeq(ctx, ARM_SMMU_CB_TTBR0,
- pgtbl_cfg.arm_lpae_s1_cfg.ttbr |
- FIELD_PREP(ARM_SMMU_TTBRn_ASID, ctx->asid));
- iommu_writeq(ctx, ARM_SMMU_CB_TTBR1, 0);
-
/* TCR */
iommu_writel(ctx, ARM_SMMU_CB_TCR2,
arm_smmu_lpae_tcr2(&pgtbl_cfg));
iommu_writel(ctx, ARM_SMMU_CB_TCR,
arm_smmu_lpae_tcr(&pgtbl_cfg) | ARM_SMMU_TCR_EAE);
+ /* TTBRs */
+ iommu_writeq(ctx, ARM_SMMU_CB_TTBR0,
+ pgtbl_cfg.arm_lpae_s1_cfg.ttbr |
+ FIELD_PREP(ARM_SMMU_TTBRn_ASID, ctx->asid));
+ iommu_writeq(ctx, ARM_SMMU_CB_TTBR1, 0);
I'd have thought that SCTLR.M would be clear here, so it shouldn't
matter
what order we write these in.
Having tested the series without this particular patch on 8976 (Sony
Loire Suzu), it doesn't seem to matter indeed. I'll ask around if this
"access behaviour" was observed on a different board/platform.
- Marijn
On some platforms, the bootloader (and/or the hypervisor) is performing
some
initialization of the IOMMU which, depending on the actual firmware version
that ran before booting Linux, may or may not leave SCTLR.M cleared.
But does it actually matter even then? If we're only allowed to program
the same ASID that was in use beforehand, then logically we can't be
changing TCR2.AS in a way that makes any difference anyway.
I see no point in pretending to worry about theoretical architectural
correctness in a driver tied to specific implementations that already
violate the given architecture in many other ways. If there's a known
firmware implementation that definitely requires this, that should be
called out; otherwise, there doesn't seem much justification for the
patch at all.
Thanks,
Robin.
