[PATCH 0/3] iommu/arm-smmu: introduction of ACTLR implementation for Qualcomm SoCs

[PATCH 0/3] iommu/arm-smmu: introduction of ACTLR implementation for Qualcomm SoCs

[Bibek Kumar Patro]

This patch series consists of three parts and covers the following:

1. Introducing intital set of driver changes to implement ACTLR register
for custom prefetcher settings in Qualcomm SoCs.

2. Adding ACTLR data and implementation operations for SM8550.

3. Re-enabling context caching for Qualcomm SoCs to retain prefetcher
settings during reset and runtime suspend.

Link to discussion on RFC posted on ACTLR implementation:
https://lore.kernel.org/all/a01e7e60-6ead-4a9e-ba90-22a8a6bbd03f@quicinc.com/

Bibek Kumar Patro (3):
  iommu/arm-smmu: introduction of ACTLR for custom prefetcher settings
  iommu/arm-smmu: add ACTLR data and support for sm8550
  iommu/arm-smmu: re-enable context caching in smmu reset operation

 drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 143 ++++++++++++++++++++-
 drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h |   2 +
 drivers/iommu/arm/arm-smmu/arm-smmu.c      |   5 +-
 drivers/iommu/arm/arm-smmu/arm-smmu.h      |   5 +
 4 files changed, 146 insertions(+), 9 deletions(-)

--
2.17.1

[PATCH 1/3] iommu/arm-smmu: introduction of ACTLR for custom prefetcher settings

[Bibek Kumar Patro]

Currently in Qualcomm  SoCs the default prefetch is set to 1 which allows
the TLB to fetch just the next page table. MMU-500 features ACTLR
register which is implementation defined and is used for Qualcomm SoCs
to have a prefetch setting of 1/3/7/15 enabling TLB to prefetch
the next set of page tables accordingly allowing for faster translations.

ACTLR value is unique for each SMR (Stream matching register) and stored
in a pre-populated table. This value is set to the register during
context bank initialisation.

Signed-off-by: Bibek Kumar Patro <quic_bibekkum@quicinc.com>
---
 drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 34 ++++++++++++++++++++++
 drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h |  2 ++
 drivers/iommu/arm/arm-smmu/arm-smmu.c      |  5 ++--
 drivers/iommu/arm/arm-smmu/arm-smmu.h      |  5 ++++
 4 files changed, 44 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
index ae7cae015193..68c1f4908473 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
@@ -14,6 +14,17 @@

 #define QCOM_DUMMY_VAL	-1

+struct actlr_config {
+	const struct actlr_data *adata;
+	u32 size;
+};
+
+struct actlr_data {
+	u16 sid;
+	u16 mask;
+	u32 actlr;
+};
+
 static struct qcom_smmu *to_qcom_smmu(struct arm_smmu_device *smmu)
 {
 	return container_of(smmu, struct qcom_smmu, smmu);
@@ -270,6 +281,26 @@ static const struct of_device_id qcom_smmu_client_of_match[] __maybe_unused = {
 static int qcom_smmu_init_context(struct arm_smmu_domain *smmu_domain,
 		struct io_pgtable_cfg *pgtbl_cfg, struct device *dev)
 {
+	struct arm_smmu_device *smmu = smmu_domain->smmu;
+	struct qcom_smmu *qsmmu = to_qcom_smmu(smmu);
+	const struct actlr_config *actlrcfg;
+	struct arm_smmu_smr *smr = smmu->smrs;
+	int idx = smmu_domain->cfg.cbndx;
+	int i;
+	u16 id;
+	u16 mask;
+
+	if (qsmmu->actlrcfg) {
+		actlrcfg = qsmmu->actlrcfg;
+		for (i = 0; i < actlrcfg->size; ++i) {
+			id = actlrcfg->adata[i].sid;
+			mask = actlrcfg->adata[i].mask;
+			if (!smr_is_subset(*smr, id, mask))
+				arm_smmu_cb_write(smmu, idx, ARM_SMMU_CB_ACTLR,
+						actlrcfg->adata[i].actlr);
+		}
+	}
+
 	smmu_domain->cfg.flush_walk_prefer_tlbiasid = true;

 	return 0;
@@ -459,6 +490,9 @@ static struct arm_smmu_device *qcom_smmu_create(struct arm_smmu_device *smmu,
 	qsmmu->smmu.impl = impl;
 	qsmmu->cfg = data->cfg;

+	if (data->actlrcfg && (data->actlrcfg->size))
+		qsmmu->actlrcfg = data->actlrcfg;
+
 	return &qsmmu->smmu;
 }

diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h
index 593910567b88..4b6862715070 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h
@@ -9,6 +9,7 @@
 struct qcom_smmu {
 	struct arm_smmu_device smmu;
 	const struct qcom_smmu_config *cfg;
+	const struct actlr_config *actlrcfg;
 	bool bypass_quirk;
 	u8 bypass_cbndx;
 	u32 stall_enabled;
@@ -25,6 +26,7 @@ struct qcom_smmu_config {
 };

 struct qcom_smmu_match_data {
+	const struct actlr_config *actlrcfg;
 	const struct qcom_smmu_config *cfg;
 	const struct arm_smmu_impl *impl;
 	const struct arm_smmu_impl *adreno_impl;
diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
index 4c79ef6f4c75..38ac1cbc799b 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
@@ -992,9 +992,10 @@ static int arm_smmu_find_sme(struct arm_smmu_device *smmu, u16 id, u16 mask)
 		 * expect simply identical entries for this case, but there's
 		 * no harm in accommodating the generalisation.
 		 */
-		if ((mask & smrs[i].mask) == mask &&
-		    !((id ^ smrs[i].id) & ~smrs[i].mask))
+
+		if (smr_is_subset(smrs[i], id, mask))
 			return i;
+
 		/*
 		 * If the new entry has any other overlap with an existing one,
 		 * though, then there always exists at least one stream ID
diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.h b/drivers/iommu/arm/arm-smmu/arm-smmu.h
index 703fd5817ec1..b1638bbc41d4 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.h
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.h
@@ -501,6 +501,11 @@ static inline void arm_smmu_writeq(struct arm_smmu_device *smmu, int page,
 		writeq_relaxed(val, arm_smmu_page(smmu, page) + offset);
 }

+static inline bool smr_is_subset(struct arm_smmu_smr smrs, u16 id, u16 mask)
+{
+	return (mask & smrs.mask) == mask && !((id ^ smrs.id) & ~smrs.mask);
+}
+
 #define ARM_SMMU_GR0		0
 #define ARM_SMMU_GR1		1
 #define ARM_SMMU_CB(s, n)	((s)->numpage + (n))
--
2.17.1

[PATCH 2/3] iommu/arm-smmu: add ACTLR data and support for SM8550

[Bibek Kumar Patro]

Add ACTLR data table for SM8550 along with support for
same including SM8550 specific implementation operations.

Signed-off-by: Bibek Kumar Patro <quic_bibekkum@quicinc.com>
---
 drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 85 +++++++++++++++++++++-
 1 file changed, 81 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
index 68c1f4908473..590b7c285299 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
@@ -25,6 +25,64 @@ struct actlr_data {
 	u32 actlr;
 };

+static const struct actlr_data sm8550_apps_actlr_data[] = {
+	{ 0x18a0, 0x0000, 0x00000103 },
+	{ 0x18e0, 0x0000, 0x00000103 },
+	{ 0x0800, 0x0020, 0x00000001 },
+	{ 0x1800, 0x00c0, 0x00000001 },
+	{ 0x1820, 0x0000, 0x00000001 },
+	{ 0x1860, 0x0000, 0x00000001 },
+	{ 0x0c01, 0x0020, 0x00000303 },
+	{ 0x0c02, 0x0020, 0x00000303 },
+	{ 0x0c03, 0x0020, 0x00000303 },
+	{ 0x0c04, 0x0020, 0x00000303 },
+	{ 0x0c05, 0x0020, 0x00000303 },
+	{ 0x0c06, 0x0020, 0x00000303 },
+	{ 0x0c07, 0x0020, 0x00000303 },
+	{ 0x0c08, 0x0020, 0x00000303 },
+	{ 0x0c09, 0x0020, 0x00000303 },
+	{ 0x0c0c, 0x0020, 0x00000303 },
+	{ 0x0c0d, 0x0020, 0x00000303 },
+	{ 0x0c0e, 0x0020, 0x00000303 },
+	{ 0x0c0f, 0x0020, 0x00000303 },
+	{ 0x1961, 0x0000, 0x00000303 },
+	{ 0x1962, 0x0000, 0x00000303 },
+	{ 0x1963, 0x0000, 0x00000303 },
+	{ 0x1964, 0x0000, 0x00000303 },
+	{ 0x1965, 0x0000, 0x00000303 },
+	{ 0x1966, 0x0000, 0x00000303 },
+	{ 0x1967, 0x0000, 0x00000303 },
+	{ 0x1968, 0x0000, 0x00000303 },
+	{ 0x1969, 0x0000, 0x00000303 },
+	{ 0x196c, 0x0000, 0x00000303 },
+	{ 0x196d, 0x0000, 0x00000303 },
+	{ 0x196e, 0x0000, 0x00000303 },
+	{ 0x196f, 0x0000, 0x00000303 },
+	{ 0x19c1, 0x0010, 0x00000303 },
+	{ 0x19c2, 0x0010, 0x00000303 },
+	{ 0x19c3, 0x0010, 0x00000303 },
+	{ 0x19c4, 0x0010, 0x00000303 },
+	{ 0x19c5, 0x0010, 0x00000303 },
+	{ 0x19c6, 0x0010, 0x00000303 },
+	{ 0x19c7, 0x0010, 0x00000303 },
+	{ 0x19c8, 0x0010, 0x00000303 },
+	{ 0x19c9, 0x0010, 0x00000303 },
+	{ 0x19cc, 0x0010, 0x00000303 },
+	{ 0x19cd, 0x0010, 0x00000303 },
+	{ 0x19ce, 0x0010, 0x00000303 },
+	{ 0x19cf, 0x0010, 0x00000303 },
+	{ 0x1c00, 0x0002, 0x00000103 },
+	{ 0x1c01, 0x0000, 0x00000001 },
+	{ 0x1920, 0x0000, 0x00000103 },
+	{ 0x1923, 0x0000, 0x00000103 },
+	{ 0x1924, 0x0000, 0x00000103 },
+	{ 0x1940, 0x0000, 0x00000103 },
+	{ 0x1941, 0x0004, 0x00000103 },
+	{ 0x1943, 0x0000, 0x00000103 },
+	{ 0x1944, 0x0000, 0x00000103 },
+	{ 0x1947, 0x0000, 0x00000103 },
+};
+
 static struct qcom_smmu *to_qcom_smmu(struct arm_smmu_device *smmu)
 {
 	return container_of(smmu, struct qcom_smmu, smmu);
@@ -444,6 +502,16 @@ static const struct arm_smmu_impl sdm845_smmu_500_impl = {
 	.tlb_sync = qcom_smmu_tlb_sync,
 };

+
+static const struct arm_smmu_impl sm8550_smmu_500_impl = {
+	.init_context = qcom_smmu_init_context,
+	.cfg_probe = qcom_smmu_cfg_probe,
+	.def_domain_type = qcom_smmu_def_domain_type,
+	.reset = arm_mmu500_reset,
+	.write_s2cr = qcom_smmu_write_s2cr,
+	.tlb_sync = qcom_smmu_tlb_sync,
+};
+
 static const struct arm_smmu_impl qcom_adreno_smmu_v2_impl = {
 	.init_context = qcom_adreno_smmu_init_context,
 	.def_domain_type = qcom_smmu_def_domain_type,
@@ -507,6 +575,11 @@ static const struct qcom_smmu_config qcom_smmu_impl0_cfg = {
 	.reg_offset = qcom_smmu_impl0_reg_offset,
 };

+static const struct actlr_config sm8550_actlrcfg = {
+	.adata = sm8550_apps_actlr_data,
+	.size = ARRAY_SIZE(sm8550_apps_actlr_data),
+};
+
 /*
  * It is not yet possible to use MDP SMMU with the bypass quirk on the msm8996,
  * there are not enough context banks.
@@ -530,16 +603,20 @@ static const struct qcom_smmu_match_data sdm845_smmu_500_data = {
 	/* Also no debug configuration. */
 };

+
+static const struct qcom_smmu_match_data sm8550_smmu_500_impl0_data = {
+	.impl = &sm8550_smmu_500_impl,
+	.adreno_impl = &qcom_adreno_smmu_500_impl,
+	.cfg = &qcom_smmu_impl0_cfg,
+	.actlrcfg = &sm8550_actlrcfg,
+};
+
 static const struct qcom_smmu_match_data qcom_smmu_500_impl0_data = {
 	.impl = &qcom_smmu_500_impl,
 	.adreno_impl = &qcom_adreno_smmu_500_impl,
 	.cfg = &qcom_smmu_impl0_cfg,
 };

-/*
- * Do not add any more qcom,SOC-smmu-500 entries to this list, unless they need
- * special handling and can not be covered by the qcom,smmu-500 entry.
- */
 static const struct of_device_id __maybe_unused qcom_smmu_impl_of_match[] = {
 	{ .compatible = "qcom,msm8996-smmu-v2", .data = &msm8996_smmu_data },
 	{ .compatible = "qcom,msm8998-smmu-v2", .data = &qcom_smmu_v2_data },
--
2.17.1

[PATCH 3/3] iommu/arm-smmu: re-enable context caching in smmu reset operation

[Bibek Kumar Patro]

Context caching is re-enabled in the prefetch buffer for Qualcomm SoCs
through SoC specific reset ops, which is disabled in the default MMU-500
reset ops, but is expected for context banks using ACTLR register to
retain the prefetch value during reset and runtime suspend.

Signed-off-by: Bibek Kumar Patro <quic_bibekkum@quicinc.com>
---
 drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 26 ++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
index 590b7c285299..f342b4778cf1 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
@@ -457,11 +457,29 @@ static int qcom_smmu_def_domain_type(struct device *dev)
 	return match ? IOMMU_DOMAIN_IDENTITY : 0;
 }

+#define ARM_MMU500_ACTLR_CPRE          BIT(1)
+
+static int qcom_smmu500_reset(struct arm_smmu_device *smmu)
+{
+	int i;
+	u32 reg;
+
+	arm_mmu500_reset(smmu);
+
+		for (i = 0; i < smmu->num_context_banks; ++i) {
+			reg = arm_smmu_cb_read(smmu, i, ARM_SMMU_CB_ACTLR);
+			reg |= ARM_MMU500_ACTLR_CPRE;
+			arm_smmu_cb_write(smmu, i, ARM_SMMU_CB_ACTLR, reg);
+		}
+
+	return 0;
+}
+
 static int qcom_sdm845_smmu500_reset(struct arm_smmu_device *smmu)
 {
 	int ret;

-	arm_mmu500_reset(smmu);
+	qcom_smmu500_reset(smmu);

 	/*
 	 * To address performance degradation in non-real time clients,
@@ -488,7 +506,7 @@ static const struct arm_smmu_impl qcom_smmu_500_impl = {
 	.init_context = qcom_smmu_init_context,
 	.cfg_probe = qcom_smmu_cfg_probe,
 	.def_domain_type = qcom_smmu_def_domain_type,
-	.reset = arm_mmu500_reset,
+	.reset = qcom_smmu500_reset,
 	.write_s2cr = qcom_smmu_write_s2cr,
 	.tlb_sync = qcom_smmu_tlb_sync,
 };
@@ -507,7 +525,7 @@ static const struct arm_smmu_impl sm8550_smmu_500_impl = {
 	.init_context = qcom_smmu_init_context,
 	.cfg_probe = qcom_smmu_cfg_probe,
 	.def_domain_type = qcom_smmu_def_domain_type,
-	.reset = arm_mmu500_reset,
+	.reset = qcom_smmu500_reset,
 	.write_s2cr = qcom_smmu_write_s2cr,
 	.tlb_sync = qcom_smmu_tlb_sync,
 };
@@ -523,7 +541,7 @@ static const struct arm_smmu_impl qcom_adreno_smmu_v2_impl = {
 static const struct arm_smmu_impl qcom_adreno_smmu_500_impl = {
 	.init_context = qcom_adreno_smmu_init_context,
 	.def_domain_type = qcom_smmu_def_domain_type,
-	.reset = arm_mmu500_reset,
+	.reset = qcom_smmu500_reset,
 	.alloc_context_bank = qcom_adreno_smmu_alloc_context_bank,
 	.write_sctlr = qcom_adreno_smmu_write_sctlr,
 	.tlb_sync = qcom_smmu_tlb_sync,
--
2.17.1

Re: [PATCH 3/3] iommu/arm-smmu: re-enable context caching in smmu reset operation

[Dmitry Baryshkov]

On Fri, 3 Nov 2023 at 23:53, Bibek Kumar Patro
<quic_bibekkum@quicinc.com> wrote:
>
> Context caching is re-enabled in the prefetch buffer for Qualcomm SoCs
> through SoC specific reset ops, which is disabled in the default MMU-500
> reset ops, but is expected for context banks using ACTLR register to
> retain the prefetch value during reset and runtime suspend.
>
> Signed-off-by: Bibek Kumar Patro <quic_bibekkum@quicinc.com>
> ---
>  drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 26 ++++++++++++++++++----
>  1 file changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> index 590b7c285299..f342b4778cf1 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> @@ -457,11 +457,29 @@ static int qcom_smmu_def_domain_type(struct device *dev)
>         return match ? IOMMU_DOMAIN_IDENTITY : 0;
>  }
>
> +#define ARM_MMU500_ACTLR_CPRE          BIT(1)
> +
> +static int qcom_smmu500_reset(struct arm_smmu_device *smmu)
> +{
> +       int i;
> +       u32 reg;
> +
> +       arm_mmu500_reset(smmu);
> +
> +               for (i = 0; i < smmu->num_context_banks; ++i) {
> +                       reg = arm_smmu_cb_read(smmu, i, ARM_SMMU_CB_ACTLR);
> +                       reg |= ARM_MMU500_ACTLR_CPRE;
> +                       arm_smmu_cb_write(smmu, i, ARM_SMMU_CB_ACTLR, reg);
> +               }

Wrong indentation. Did you run your patches through checkpatch.pl?

> +
> +       return 0;
> +}
> +
>  static int qcom_sdm845_smmu500_reset(struct arm_smmu_device *smmu)
>  {
>         int ret;
>
> -       arm_mmu500_reset(smmu);
> +       qcom_smmu500_reset(smmu);
>
>         /*
>          * To address performance degradation in non-real time clients,
> @@ -488,7 +506,7 @@ static const struct arm_smmu_impl qcom_smmu_500_impl = {
>         .init_context = qcom_smmu_init_context,
>         .cfg_probe = qcom_smmu_cfg_probe,
>         .def_domain_type = qcom_smmu_def_domain_type,
> -       .reset = arm_mmu500_reset,
> +       .reset = qcom_smmu500_reset,
>         .write_s2cr = qcom_smmu_write_s2cr,
>         .tlb_sync = qcom_smmu_tlb_sync,
>  };
> @@ -507,7 +525,7 @@ static const struct arm_smmu_impl sm8550_smmu_500_impl = {
>         .init_context = qcom_smmu_init_context,
>         .cfg_probe = qcom_smmu_cfg_probe,
>         .def_domain_type = qcom_smmu_def_domain_type,
> -       .reset = arm_mmu500_reset,
> +       .reset = qcom_smmu500_reset,
>         .write_s2cr = qcom_smmu_write_s2cr,
>         .tlb_sync = qcom_smmu_tlb_sync,
>  };
> @@ -523,7 +541,7 @@ static const struct arm_smmu_impl qcom_adreno_smmu_v2_impl = {
>  static const struct arm_smmu_impl qcom_adreno_smmu_500_impl = {
>         .init_context = qcom_adreno_smmu_init_context,
>         .def_domain_type = qcom_smmu_def_domain_type,
> -       .reset = arm_mmu500_reset,
> +       .reset = qcom_smmu500_reset,
>         .alloc_context_bank = qcom_adreno_smmu_alloc_context_bank,
>         .write_sctlr = qcom_adreno_smmu_write_sctlr,
>         .tlb_sync = qcom_smmu_tlb_sync,
> --
> 2.17.1
>


-- 
With best wishes
Dmitry

Re: [PATCH 2/3] iommu/arm-smmu: add ACTLR data and support for SM8550

[Dmitry Baryshkov]

On Fri, 3 Nov 2023 at 23:53, Bibek Kumar Patro
<quic_bibekkum@quicinc.com> wrote:
>
> Add ACTLR data table for SM8550 along with support for
> same including SM8550 specific implementation operations.
>
> Signed-off-by: Bibek Kumar Patro <quic_bibekkum@quicinc.com>
> ---
>  drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 85 +++++++++++++++++++++-
>  1 file changed, 81 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> index 68c1f4908473..590b7c285299 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> @@ -25,6 +25,64 @@ struct actlr_data {
>         u32 actlr;
>  };
>
> +static const struct actlr_data sm8550_apps_actlr_data[] = {
> +       { 0x18a0, 0x0000, 0x00000103 },
> +       { 0x18e0, 0x0000, 0x00000103 },
> +       { 0x0800, 0x0020, 0x00000001 },
> +       { 0x1800, 0x00c0, 0x00000001 },
> +       { 0x1820, 0x0000, 0x00000001 },
> +       { 0x1860, 0x0000, 0x00000001 },
> +       { 0x0c01, 0x0020, 0x00000303 },
> +       { 0x0c02, 0x0020, 0x00000303 },
> +       { 0x0c03, 0x0020, 0x00000303 },
> +       { 0x0c04, 0x0020, 0x00000303 },
> +       { 0x0c05, 0x0020, 0x00000303 },
> +       { 0x0c06, 0x0020, 0x00000303 },
> +       { 0x0c07, 0x0020, 0x00000303 },
> +       { 0x0c08, 0x0020, 0x00000303 },
> +       { 0x0c09, 0x0020, 0x00000303 },
> +       { 0x0c0c, 0x0020, 0x00000303 },
> +       { 0x0c0d, 0x0020, 0x00000303 },
> +       { 0x0c0e, 0x0020, 0x00000303 },
> +       { 0x0c0f, 0x0020, 0x00000303 },
> +       { 0x1961, 0x0000, 0x00000303 },
> +       { 0x1962, 0x0000, 0x00000303 },
> +       { 0x1963, 0x0000, 0x00000303 },
> +       { 0x1964, 0x0000, 0x00000303 },
> +       { 0x1965, 0x0000, 0x00000303 },
> +       { 0x1966, 0x0000, 0x00000303 },
> +       { 0x1967, 0x0000, 0x00000303 },
> +       { 0x1968, 0x0000, 0x00000303 },
> +       { 0x1969, 0x0000, 0x00000303 },
> +       { 0x196c, 0x0000, 0x00000303 },
> +       { 0x196d, 0x0000, 0x00000303 },
> +       { 0x196e, 0x0000, 0x00000303 },
> +       { 0x196f, 0x0000, 0x00000303 },
> +       { 0x19c1, 0x0010, 0x00000303 },
> +       { 0x19c2, 0x0010, 0x00000303 },
> +       { 0x19c3, 0x0010, 0x00000303 },
> +       { 0x19c4, 0x0010, 0x00000303 },
> +       { 0x19c5, 0x0010, 0x00000303 },
> +       { 0x19c6, 0x0010, 0x00000303 },
> +       { 0x19c7, 0x0010, 0x00000303 },
> +       { 0x19c8, 0x0010, 0x00000303 },
> +       { 0x19c9, 0x0010, 0x00000303 },
> +       { 0x19cc, 0x0010, 0x00000303 },
> +       { 0x19cd, 0x0010, 0x00000303 },
> +       { 0x19ce, 0x0010, 0x00000303 },
> +       { 0x19cf, 0x0010, 0x00000303 },
> +       { 0x1c00, 0x0002, 0x00000103 },
> +       { 0x1c01, 0x0000, 0x00000001 },
> +       { 0x1920, 0x0000, 0x00000103 },
> +       { 0x1923, 0x0000, 0x00000103 },
> +       { 0x1924, 0x0000, 0x00000103 },
> +       { 0x1940, 0x0000, 0x00000103 },
> +       { 0x1941, 0x0004, 0x00000103 },
> +       { 0x1943, 0x0000, 0x00000103 },
> +       { 0x1944, 0x0000, 0x00000103 },
> +       { 0x1947, 0x0000, 0x00000103 },
> +};

This is nearly impossible to handle.
Please add defines for 0x1, 0x103 and 0x303. Also please consider
adding comments for the devices.

> +
>  static struct qcom_smmu *to_qcom_smmu(struct arm_smmu_device *smmu)
>  {
>         return container_of(smmu, struct qcom_smmu, smmu);
> @@ -444,6 +502,16 @@ static const struct arm_smmu_impl sdm845_smmu_500_impl = {
>         .tlb_sync = qcom_smmu_tlb_sync,
>  };
>
> +
> +static const struct arm_smmu_impl sm8550_smmu_500_impl = {
> +       .init_context = qcom_smmu_init_context,
> +       .cfg_probe = qcom_smmu_cfg_probe,
> +       .def_domain_type = qcom_smmu_def_domain_type,
> +       .reset = arm_mmu500_reset,
> +       .write_s2cr = qcom_smmu_write_s2cr,
> +       .tlb_sync = qcom_smmu_tlb_sync,
> +};
> +
>  static const struct arm_smmu_impl qcom_adreno_smmu_v2_impl = {
>         .init_context = qcom_adreno_smmu_init_context,
>         .def_domain_type = qcom_smmu_def_domain_type,
> @@ -507,6 +575,11 @@ static const struct qcom_smmu_config qcom_smmu_impl0_cfg = {
>         .reg_offset = qcom_smmu_impl0_reg_offset,
>  };
>
> +static const struct actlr_config sm8550_actlrcfg = {
> +       .adata = sm8550_apps_actlr_data,
> +       .size = ARRAY_SIZE(sm8550_apps_actlr_data),
> +};
> +
>  /*
>   * It is not yet possible to use MDP SMMU with the bypass quirk on the msm8996,
>   * there are not enough context banks.
> @@ -530,16 +603,20 @@ static const struct qcom_smmu_match_data sdm845_smmu_500_data = {
>         /* Also no debug configuration. */
>  };
>
> +
> +static const struct qcom_smmu_match_data sm8550_smmu_500_impl0_data = {
> +       .impl = &sm8550_smmu_500_impl,
> +       .adreno_impl = &qcom_adreno_smmu_500_impl,
> +       .cfg = &qcom_smmu_impl0_cfg,
> +       .actlrcfg = &sm8550_actlrcfg,
> +};

This structure doesn't seem to be linked. Did you test your patches?

> +
>  static const struct qcom_smmu_match_data qcom_smmu_500_impl0_data = {
>         .impl = &qcom_smmu_500_impl,
>         .adreno_impl = &qcom_adreno_smmu_500_impl,
>         .cfg = &qcom_smmu_impl0_cfg,
>  };
>
> -/*
> - * Do not add any more qcom,SOC-smmu-500 entries to this list, unless they need
> - * special handling and can not be covered by the qcom,smmu-500 entry.
> - */

Leave the comment in place.

>  static const struct of_device_id __maybe_unused qcom_smmu_impl_of_match[] = {
>         { .compatible = "qcom,msm8996-smmu-v2", .data = &msm8996_smmu_data },
>         { .compatible = "qcom,msm8998-smmu-v2", .data = &qcom_smmu_v2_data },
> --
> 2.17.1
>


-- 
With best wishes
Dmitry

Re: [PATCH 1/3] iommu/arm-smmu: introduction of ACTLR for custom prefetcher settings

[Dmitry Baryshkov]

On Fri, 3 Nov 2023 at 23:53, Bibek Kumar Patro
<quic_bibekkum@quicinc.com> wrote:
>
> Currently in Qualcomm  SoCs the default prefetch is set to 1 which allows
> the TLB to fetch just the next page table. MMU-500 features ACTLR
> register which is implementation defined and is used for Qualcomm SoCs
> to have a prefetch setting of 1/3/7/15 enabling TLB to prefetch
> the next set of page tables accordingly allowing for faster translations.
>
> ACTLR value is unique for each SMR (Stream matching register) and stored
> in a pre-populated table. This value is set to the register during
> context bank initialisation.
>
> Signed-off-by: Bibek Kumar Patro <quic_bibekkum@quicinc.com>
> ---
>  drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 34 ++++++++++++++++++++++
>  drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h |  2 ++
>  drivers/iommu/arm/arm-smmu/arm-smmu.c      |  5 ++--
>  drivers/iommu/arm/arm-smmu/arm-smmu.h      |  5 ++++
>  4 files changed, 44 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> index ae7cae015193..68c1f4908473 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> @@ -14,6 +14,17 @@
>
>  #define QCOM_DUMMY_VAL -1
>
> +struct actlr_config {
> +       const struct actlr_data *adata;
> +       u32 size;

This should be size_t.

Also could you please drop the separate struct actlr_config and move
these two fields into struct qcom_smmu_config.

> +};
> +
> +struct actlr_data {
> +       u16 sid;
> +       u16 mask;
> +       u32 actlr;
> +};
> +
>  static struct qcom_smmu *to_qcom_smmu(struct arm_smmu_device *smmu)
>  {
>         return container_of(smmu, struct qcom_smmu, smmu);
> @@ -270,6 +281,26 @@ static const struct of_device_id qcom_smmu_client_of_match[] __maybe_unused = {
>  static int qcom_smmu_init_context(struct arm_smmu_domain *smmu_domain,
>                 struct io_pgtable_cfg *pgtbl_cfg, struct device *dev)
>  {
> +       struct arm_smmu_device *smmu = smmu_domain->smmu;
> +       struct qcom_smmu *qsmmu = to_qcom_smmu(smmu);
> +       const struct actlr_config *actlrcfg;
> +       struct arm_smmu_smr *smr = smmu->smrs;
> +       int idx = smmu_domain->cfg.cbndx;
> +       int i;
> +       u16 id;
> +       u16 mask;
> +
> +       if (qsmmu->actlrcfg) {
> +               actlrcfg = qsmmu->actlrcfg;
> +               for (i = 0; i < actlrcfg->size; ++i) {
> +                       id = actlrcfg->adata[i].sid;
> +                       mask = actlrcfg->adata[i].mask;
> +                       if (!smr_is_subset(*smr, id, mask))
> +                               arm_smmu_cb_write(smmu, idx, ARM_SMMU_CB_ACTLR,
> +                                               actlrcfg->adata[i].actlr);
> +               }
> +       }

Consider extracting this to a separate function. This way you can
reduce 4 indentation levels into a single loop.

> +
>         smmu_domain->cfg.flush_walk_prefer_tlbiasid = true;
>
>         return 0;
> @@ -459,6 +490,9 @@ static struct arm_smmu_device *qcom_smmu_create(struct arm_smmu_device *smmu,
>         qsmmu->smmu.impl = impl;
>         qsmmu->cfg = data->cfg;
>
> +       if (data->actlrcfg && (data->actlrcfg->size))
> +               qsmmu->actlrcfg = data->actlrcfg;
> +
>         return &qsmmu->smmu;
>  }
>
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h
> index 593910567b88..4b6862715070 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h
> @@ -9,6 +9,7 @@
>  struct qcom_smmu {
>         struct arm_smmu_device smmu;
>         const struct qcom_smmu_config *cfg;
> +       const struct actlr_config *actlrcfg;
>         bool bypass_quirk;
>         u8 bypass_cbndx;
>         u32 stall_enabled;
> @@ -25,6 +26,7 @@ struct qcom_smmu_config {
>  };
>
>  struct qcom_smmu_match_data {
> +       const struct actlr_config *actlrcfg;
>         const struct qcom_smmu_config *cfg;
>         const struct arm_smmu_impl *impl;
>         const struct arm_smmu_impl *adreno_impl;
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> index 4c79ef6f4c75..38ac1cbc799b 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> @@ -992,9 +992,10 @@ static int arm_smmu_find_sme(struct arm_smmu_device *smmu, u16 id, u16 mask)
>                  * expect simply identical entries for this case, but there's
>                  * no harm in accommodating the generalisation.
>                  */
> -               if ((mask & smrs[i].mask) == mask &&
> -                   !((id ^ smrs[i].id) & ~smrs[i].mask))
> +
> +               if (smr_is_subset(smrs[i], id, mask))
>                         return i;
> +
>                 /*
>                  * If the new entry has any other overlap with an existing one,
>                  * though, then there always exists at least one stream ID
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.h b/drivers/iommu/arm/arm-smmu/arm-smmu.h
> index 703fd5817ec1..b1638bbc41d4 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.h
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.h
> @@ -501,6 +501,11 @@ static inline void arm_smmu_writeq(struct arm_smmu_device *smmu, int page,
>                 writeq_relaxed(val, arm_smmu_page(smmu, page) + offset);
>  }
>
> +static inline bool smr_is_subset(struct arm_smmu_smr smrs, u16 id, u16 mask)
> +{
> +       return (mask & smrs.mask) == mask && !((id ^ smrs.id) & ~smrs.mask);
> +}
> +
>  #define ARM_SMMU_GR0           0
>  #define ARM_SMMU_GR1           1
>  #define ARM_SMMU_CB(s, n)      ((s)->numpage + (n))
> --
> 2.17.1
>


-- 
With best wishes
Dmitry

Re: [PATCH 3/3] iommu/arm-smmu: re-enable context caching in smmu reset operation

[Bibek Kumar Patro]

On 11/4/2023 3:28 AM, Dmitry Baryshkov wrote:
> On Fri, 3 Nov 2023 at 23:53, Bibek Kumar Patro
> <quic_bibekkum@quicinc.com> wrote:
>>
>> Context caching is re-enabled in the prefetch buffer for Qualcomm SoCs
>> through SoC specific reset ops, which is disabled in the default MMU-500
>> reset ops, but is expected for context banks using ACTLR register to
>> retain the prefetch value during reset and runtime suspend.
>>
>> Signed-off-by: Bibek Kumar Patro <quic_bibekkum@quicinc.com>
>> ---
>>   drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 26 ++++++++++++++++++----
>>   1 file changed, 22 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>> index 590b7c285299..f342b4778cf1 100644
>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>> @@ -457,11 +457,29 @@ static int qcom_smmu_def_domain_type(struct device *dev)
>>          return match ? IOMMU_DOMAIN_IDENTITY : 0;
>>   }
>>
>> +#define ARM_MMU500_ACTLR_CPRE          BIT(1)
>> +
>> +static int qcom_smmu500_reset(struct arm_smmu_device *smmu)
>> +{
>> +       int i;
>> +       u32 reg;
>> +
>> +       arm_mmu500_reset(smmu);
>> +
>> +               for (i = 0; i < smmu->num_context_banks; ++i) {
>> +                       reg = arm_smmu_cb_read(smmu, i, ARM_SMMU_CB_ACTLR);
>> +                       reg |= ARM_MMU500_ACTLR_CPRE;
>> +                       arm_smmu_cb_write(smmu, i, ARM_SMMU_CB_ACTLR, reg);
>> +               }
> 
> Wrong indentation. Did you run your patches through checkpatch.pl?
> 

Yes Dmitry, I did run checkpatch.pl script on this patch as well as 
others, got 0 errors and 0 warnings. With -f option as well. Did not
get any related errors and warnings.

>> +
>> +       return 0;
>> +}
>> +
>>   static int qcom_sdm845_smmu500_reset(struct arm_smmu_device *smmu)
>>   {
>>          int ret;
>>
>> -       arm_mmu500_reset(smmu);
>> +       qcom_smmu500_reset(smmu);
>>
>>          /*
>>           * To address performance degradation in non-real time clients,
>> @@ -488,7 +506,7 @@ static const struct arm_smmu_impl qcom_smmu_500_impl = {
>>          .init_context = qcom_smmu_init_context,
>>          .cfg_probe = qcom_smmu_cfg_probe,
>>          .def_domain_type = qcom_smmu_def_domain_type,
>> -       .reset = arm_mmu500_reset,
>> +       .reset = qcom_smmu500_reset,
>>          .write_s2cr = qcom_smmu_write_s2cr,
>>          .tlb_sync = qcom_smmu_tlb_sync,
>>   };
>> @@ -507,7 +525,7 @@ static const struct arm_smmu_impl sm8550_smmu_500_impl = {
>>          .init_context = qcom_smmu_init_context,
>>          .cfg_probe = qcom_smmu_cfg_probe,
>>          .def_domain_type = qcom_smmu_def_domain_type,
>> -       .reset = arm_mmu500_reset,
>> +       .reset = qcom_smmu500_reset,
>>          .write_s2cr = qcom_smmu_write_s2cr,
>>          .tlb_sync = qcom_smmu_tlb_sync,
>>   };
>> @@ -523,7 +541,7 @@ static const struct arm_smmu_impl qcom_adreno_smmu_v2_impl = {
>>   static const struct arm_smmu_impl qcom_adreno_smmu_500_impl = {
>>          .init_context = qcom_adreno_smmu_init_context,
>>          .def_domain_type = qcom_smmu_def_domain_type,
>> -       .reset = arm_mmu500_reset,
>> +       .reset = qcom_smmu500_reset,
>>          .alloc_context_bank = qcom_adreno_smmu_alloc_context_bank,
>>          .write_sctlr = qcom_adreno_smmu_write_sctlr,
>>          .tlb_sync = qcom_smmu_tlb_sync,
>> --
>> 2.17.1
>>
> 
> 

Re: [PATCH 3/3] iommu/arm-smmu: re-enable context caching in smmu reset operation

[Dmitry Baryshkov]

On Sat, 4 Nov 2023 at 00:07, Bibek Kumar Patro
<quic_bibekkum@quicinc.com> wrote:
>
>
>
> On 11/4/2023 3:28 AM, Dmitry Baryshkov wrote:
> > On Fri, 3 Nov 2023 at 23:53, Bibek Kumar Patro
> > <quic_bibekkum@quicinc.com> wrote:
> >>
> >> Context caching is re-enabled in the prefetch buffer for Qualcomm SoCs
> >> through SoC specific reset ops, which is disabled in the default MMU-500
> >> reset ops, but is expected for context banks using ACTLR register to
> >> retain the prefetch value during reset and runtime suspend.
> >>
> >> Signed-off-by: Bibek Kumar Patro <quic_bibekkum@quicinc.com>
> >> ---
> >>   drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 26 ++++++++++++++++++----
> >>   1 file changed, 22 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> >> index 590b7c285299..f342b4778cf1 100644
> >> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> >> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> >> @@ -457,11 +457,29 @@ static int qcom_smmu_def_domain_type(struct device *dev)
> >>          return match ? IOMMU_DOMAIN_IDENTITY : 0;
> >>   }
> >>
> >> +#define ARM_MMU500_ACTLR_CPRE          BIT(1)
> >> +
> >> +static int qcom_smmu500_reset(struct arm_smmu_device *smmu)
> >> +{
> >> +       int i;
> >> +       u32 reg;
> >> +
> >> +       arm_mmu500_reset(smmu);
> >> +
> >> +               for (i = 0; i < smmu->num_context_banks; ++i) {
> >> +                       reg = arm_smmu_cb_read(smmu, i, ARM_SMMU_CB_ACTLR);
> >> +                       reg |= ARM_MMU500_ACTLR_CPRE;
> >> +                       arm_smmu_cb_write(smmu, i, ARM_SMMU_CB_ACTLR, reg);
> >> +               }
> >
> > Wrong indentation. Did you run your patches through checkpatch.pl?
> >
>
> Yes Dmitry, I did run checkpatch.pl script on this patch as well as
> others, got 0 errors and 0 warnings. With -f option as well. Did not
> get any related errors and warnings.

Ack, I beg your pardon. checkpatch indeed doesn't warn about this
indentation. Though it is still incorrect.

>
> >> +
> >> +       return 0;
> >> +}
> >> +
> >>   static int qcom_sdm845_smmu500_reset(struct arm_smmu_device *smmu)
> >>   {
> >>          int ret;
> >>
> >> -       arm_mmu500_reset(smmu);
> >> +       qcom_smmu500_reset(smmu);
> >>
> >>          /*
> >>           * To address performance degradation in non-real time clients,
> >> @@ -488,7 +506,7 @@ static const struct arm_smmu_impl qcom_smmu_500_impl = {
> >>          .init_context = qcom_smmu_init_context,
> >>          .cfg_probe = qcom_smmu_cfg_probe,
> >>          .def_domain_type = qcom_smmu_def_domain_type,
> >> -       .reset = arm_mmu500_reset,
> >> +       .reset = qcom_smmu500_reset,
> >>          .write_s2cr = qcom_smmu_write_s2cr,
> >>          .tlb_sync = qcom_smmu_tlb_sync,
> >>   };
> >> @@ -507,7 +525,7 @@ static const struct arm_smmu_impl sm8550_smmu_500_impl = {
> >>          .init_context = qcom_smmu_init_context,
> >>          .cfg_probe = qcom_smmu_cfg_probe,
> >>          .def_domain_type = qcom_smmu_def_domain_type,
> >> -       .reset = arm_mmu500_reset,
> >> +       .reset = qcom_smmu500_reset,
> >>          .write_s2cr = qcom_smmu_write_s2cr,
> >>          .tlb_sync = qcom_smmu_tlb_sync,
> >>   };
> >> @@ -523,7 +541,7 @@ static const struct arm_smmu_impl qcom_adreno_smmu_v2_impl = {
> >>   static const struct arm_smmu_impl qcom_adreno_smmu_500_impl = {
> >>          .init_context = qcom_adreno_smmu_init_context,
> >>          .def_domain_type = qcom_smmu_def_domain_type,
> >> -       .reset = arm_mmu500_reset,
> >> +       .reset = qcom_smmu500_reset,
> >>          .alloc_context_bank = qcom_adreno_smmu_alloc_context_bank,
> >>          .write_sctlr = qcom_adreno_smmu_write_sctlr,
> >>          .tlb_sync = qcom_smmu_tlb_sync,
> >> --
> >> 2.17.1
> >>
> >
> >



-- 
With best wishes
Dmitry

Re: [PATCH 2/3] iommu/arm-smmu: add ACTLR data and support for SM8550

[Bibek Kumar Patro]

On 11/4/2023 3:31 AM, Dmitry Baryshkov wrote:
> On Fri, 3 Nov 2023 at 23:53, Bibek Kumar Patro
> <quic_bibekkum@quicinc.com> wrote:
>>
>> Add ACTLR data table for SM8550 along with support for
>> same including SM8550 specific implementation operations.
>>
>> Signed-off-by: Bibek Kumar Patro <quic_bibekkum@quicinc.com>
>> ---
>>   drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 85 +++++++++++++++++++++-
>>   1 file changed, 81 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>> index 68c1f4908473..590b7c285299 100644
>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>> @@ -25,6 +25,64 @@ struct actlr_data {
>>          u32 actlr;
>>   };
>>
>> +static const struct actlr_data sm8550_apps_actlr_data[] = {
>> +       { 0x18a0, 0x0000, 0x00000103 },
>> +       { 0x18e0, 0x0000, 0x00000103 },
>> +       { 0x0800, 0x0020, 0x00000001 },
>> +       { 0x1800, 0x00c0, 0x00000001 },
>> +       { 0x1820, 0x0000, 0x00000001 },
>> +       { 0x1860, 0x0000, 0x00000001 },
>> +       { 0x0c01, 0x0020, 0x00000303 },
>> +       { 0x0c02, 0x0020, 0x00000303 },
>> +       { 0x0c03, 0x0020, 0x00000303 },
>> +       { 0x0c04, 0x0020, 0x00000303 },
>> +       { 0x0c05, 0x0020, 0x00000303 },
>> +       { 0x0c06, 0x0020, 0x00000303 },
>> +       { 0x0c07, 0x0020, 0x00000303 },
>> +       { 0x0c08, 0x0020, 0x00000303 },
>> +       { 0x0c09, 0x0020, 0x00000303 },
>> +       { 0x0c0c, 0x0020, 0x00000303 },
>> +       { 0x0c0d, 0x0020, 0x00000303 },
>> +       { 0x0c0e, 0x0020, 0x00000303 },
>> +       { 0x0c0f, 0x0020, 0x00000303 },
>> +       { 0x1961, 0x0000, 0x00000303 },
>> +       { 0x1962, 0x0000, 0x00000303 },
>> +       { 0x1963, 0x0000, 0x00000303 },
>> +       { 0x1964, 0x0000, 0x00000303 },
>> +       { 0x1965, 0x0000, 0x00000303 },
>> +       { 0x1966, 0x0000, 0x00000303 },
>> +       { 0x1967, 0x0000, 0x00000303 },
>> +       { 0x1968, 0x0000, 0x00000303 },
>> +       { 0x1969, 0x0000, 0x00000303 },
>> +       { 0x196c, 0x0000, 0x00000303 },
>> +       { 0x196d, 0x0000, 0x00000303 },
>> +       { 0x196e, 0x0000, 0x00000303 },
>> +       { 0x196f, 0x0000, 0x00000303 },
>> +       { 0x19c1, 0x0010, 0x00000303 },
>> +       { 0x19c2, 0x0010, 0x00000303 },
>> +       { 0x19c3, 0x0010, 0x00000303 },
>> +       { 0x19c4, 0x0010, 0x00000303 },
>> +       { 0x19c5, 0x0010, 0x00000303 },
>> +       { 0x19c6, 0x0010, 0x00000303 },
>> +       { 0x19c7, 0x0010, 0x00000303 },
>> +       { 0x19c8, 0x0010, 0x00000303 },
>> +       { 0x19c9, 0x0010, 0x00000303 },
>> +       { 0x19cc, 0x0010, 0x00000303 },
>> +       { 0x19cd, 0x0010, 0x00000303 },
>> +       { 0x19ce, 0x0010, 0x00000303 },
>> +       { 0x19cf, 0x0010, 0x00000303 },
>> +       { 0x1c00, 0x0002, 0x00000103 },
>> +       { 0x1c01, 0x0000, 0x00000001 },
>> +       { 0x1920, 0x0000, 0x00000103 },
>> +       { 0x1923, 0x0000, 0x00000103 },
>> +       { 0x1924, 0x0000, 0x00000103 },
>> +       { 0x1940, 0x0000, 0x00000103 },
>> +       { 0x1941, 0x0004, 0x00000103 },
>> +       { 0x1943, 0x0000, 0x00000103 },
>> +       { 0x1944, 0x0000, 0x00000103 },
>> +       { 0x1947, 0x0000, 0x00000103 },
>> +};
> 
> This is nearly impossible to handle.
> Please add defines for 0x1, 0x103 and 0x303. Also please consider
> adding comments for the devices.
> 

Ack, Initially added the comments for devices, but since only driver is
handling this data, and clients won't refer this so removed it. Will
consider adding it back. This actlr field value might different (other
than 0x1,0x103,0x303) in other platforms as per my anticipation, 
depending on the bit settings, so won't the defines change with 
different platforms? Hence this register setting value might be apt?

>> +
>>   static struct qcom_smmu *to_qcom_smmu(struct arm_smmu_device *smmu)
>>   {
>>          return container_of(smmu, struct qcom_smmu, smmu);
>> @@ -444,6 +502,16 @@ static const struct arm_smmu_impl sdm845_smmu_500_impl = {
>>          .tlb_sync = qcom_smmu_tlb_sync,
>>   };
>>
>> +
>> +static const struct arm_smmu_impl sm8550_smmu_500_impl = {
>> +       .init_context = qcom_smmu_init_context,
>> +       .cfg_probe = qcom_smmu_cfg_probe,
>> +       .def_domain_type = qcom_smmu_def_domain_type,
>> +       .reset = arm_mmu500_reset,
>> +       .write_s2cr = qcom_smmu_write_s2cr,
>> +       .tlb_sync = qcom_smmu_tlb_sync,
>> +};
>> +
>>   static const struct arm_smmu_impl qcom_adreno_smmu_v2_impl = {
>>          .init_context = qcom_adreno_smmu_init_context,
>>          .def_domain_type = qcom_smmu_def_domain_type,
>> @@ -507,6 +575,11 @@ static const struct qcom_smmu_config qcom_smmu_impl0_cfg = {
>>          .reg_offset = qcom_smmu_impl0_reg_offset,
>>   };
>>
>> +static const struct actlr_config sm8550_actlrcfg = {
>> +       .adata = sm8550_apps_actlr_data,
>> +       .size = ARRAY_SIZE(sm8550_apps_actlr_data),
>> +};
>> +
>>   /*
>>    * It is not yet possible to use MDP SMMU with the bypass quirk on the msm8996,
>>    * there are not enough context banks.
>> @@ -530,16 +603,20 @@ static const struct qcom_smmu_match_data sdm845_smmu_500_data = {
>>          /* Also no debug configuration. */
>>   };
>>
>> +
>> +static const struct qcom_smmu_match_data sm8550_smmu_500_impl0_data = {
>> +       .impl = &sm8550_smmu_500_impl,
>> +       .adreno_impl = &qcom_adreno_smmu_500_impl,
>> +       .cfg = &qcom_smmu_impl0_cfg,
>> +       .actlrcfg = &sm8550_actlrcfg,
>> +};
> 
> This structure doesn't seem to be linked. Did you test your patches?
> 

Apologies Dmitry, just checked back my patches, I tested it but while 
refining the patches I somehow missed this link
{ .compatible = "qcom,sm8550-smmu-500", .data = 
&sm8550_smmu_500_impl0_data };
in below qcom_smmu_500_impl0_data structure.
I will take care of this in next version.

>> +
>>   static const struct qcom_smmu_match_data qcom_smmu_500_impl0_data = {
>>          .impl = &qcom_smmu_500_impl,
>>          .adreno_impl = &qcom_adreno_smmu_500_impl,
>>          .cfg = &qcom_smmu_impl0_cfg,
>>   };
>>
>> -/*
>> - * Do not add any more qcom,SOC-smmu-500 entries to this list, unless they need
>> - * special handling and can not be covered by the qcom,smmu-500 entry.
>> - */
> 
> Leave the comment in place.
> 

Thanks for this comment which helped me to note the above mentioned
mistake.

>>   static const struct of_device_id __maybe_unused qcom_smmu_impl_of_match[] = {
>>          { .compatible = "qcom,msm8996-smmu-v2", .data = &msm8996_smmu_data },
>>          { .compatible = "qcom,msm8998-smmu-v2", .data = &qcom_smmu_v2_data },
>> --
>> 2.17.1
>>
> 
> 

Re: [PATCH 2/3] iommu/arm-smmu: add ACTLR data and support for SM8550

[Dmitry Baryshkov]

On Sat, 4 Nov 2023 at 00:38, Bibek Kumar Patro
<quic_bibekkum@quicinc.com> wrote:
>
>
>
> On 11/4/2023 3:31 AM, Dmitry Baryshkov wrote:
> > On Fri, 3 Nov 2023 at 23:53, Bibek Kumar Patro
> > <quic_bibekkum@quicinc.com> wrote:
> >>
> >> Add ACTLR data table for SM8550 along with support for
> >> same including SM8550 specific implementation operations.
> >>
> >> Signed-off-by: Bibek Kumar Patro <quic_bibekkum@quicinc.com>
> >> ---
> >>   drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 85 +++++++++++++++++++++-
> >>   1 file changed, 81 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> >> index 68c1f4908473..590b7c285299 100644
> >> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> >> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> >> @@ -25,6 +25,64 @@ struct actlr_data {
> >>          u32 actlr;
> >>   };
> >>
> >> +static const struct actlr_data sm8550_apps_actlr_data[] = {
> >> +       { 0x18a0, 0x0000, 0x00000103 },
> >> +       { 0x18e0, 0x0000, 0x00000103 },
> >> +       { 0x0800, 0x0020, 0x00000001 },
> >> +       { 0x1800, 0x00c0, 0x00000001 },
> >> +       { 0x1820, 0x0000, 0x00000001 },
> >> +       { 0x1860, 0x0000, 0x00000001 },
> >> +       { 0x0c01, 0x0020, 0x00000303 },
> >> +       { 0x0c02, 0x0020, 0x00000303 },
> >> +       { 0x0c03, 0x0020, 0x00000303 },
> >> +       { 0x0c04, 0x0020, 0x00000303 },
> >> +       { 0x0c05, 0x0020, 0x00000303 },
> >> +       { 0x0c06, 0x0020, 0x00000303 },
> >> +       { 0x0c07, 0x0020, 0x00000303 },
> >> +       { 0x0c08, 0x0020, 0x00000303 },
> >> +       { 0x0c09, 0x0020, 0x00000303 },
> >> +       { 0x0c0c, 0x0020, 0x00000303 },
> >> +       { 0x0c0d, 0x0020, 0x00000303 },
> >> +       { 0x0c0e, 0x0020, 0x00000303 },
> >> +       { 0x0c0f, 0x0020, 0x00000303 },
> >> +       { 0x1961, 0x0000, 0x00000303 },
> >> +       { 0x1962, 0x0000, 0x00000303 },
> >> +       { 0x1963, 0x0000, 0x00000303 },
> >> +       { 0x1964, 0x0000, 0x00000303 },
> >> +       { 0x1965, 0x0000, 0x00000303 },
> >> +       { 0x1966, 0x0000, 0x00000303 },
> >> +       { 0x1967, 0x0000, 0x00000303 },
> >> +       { 0x1968, 0x0000, 0x00000303 },
> >> +       { 0x1969, 0x0000, 0x00000303 },
> >> +       { 0x196c, 0x0000, 0x00000303 },
> >> +       { 0x196d, 0x0000, 0x00000303 },
> >> +       { 0x196e, 0x0000, 0x00000303 },
> >> +       { 0x196f, 0x0000, 0x00000303 },
> >> +       { 0x19c1, 0x0010, 0x00000303 },
> >> +       { 0x19c2, 0x0010, 0x00000303 },
> >> +       { 0x19c3, 0x0010, 0x00000303 },
> >> +       { 0x19c4, 0x0010, 0x00000303 },
> >> +       { 0x19c5, 0x0010, 0x00000303 },
> >> +       { 0x19c6, 0x0010, 0x00000303 },
> >> +       { 0x19c7, 0x0010, 0x00000303 },
> >> +       { 0x19c8, 0x0010, 0x00000303 },
> >> +       { 0x19c9, 0x0010, 0x00000303 },
> >> +       { 0x19cc, 0x0010, 0x00000303 },
> >> +       { 0x19cd, 0x0010, 0x00000303 },
> >> +       { 0x19ce, 0x0010, 0x00000303 },
> >> +       { 0x19cf, 0x0010, 0x00000303 },
> >> +       { 0x1c00, 0x0002, 0x00000103 },
> >> +       { 0x1c01, 0x0000, 0x00000001 },
> >> +       { 0x1920, 0x0000, 0x00000103 },
> >> +       { 0x1923, 0x0000, 0x00000103 },
> >> +       { 0x1924, 0x0000, 0x00000103 },
> >> +       { 0x1940, 0x0000, 0x00000103 },
> >> +       { 0x1941, 0x0004, 0x00000103 },
> >> +       { 0x1943, 0x0000, 0x00000103 },
> >> +       { 0x1944, 0x0000, 0x00000103 },
> >> +       { 0x1947, 0x0000, 0x00000103 },
> >> +};
> >
> > This is nearly impossible to handle.
> > Please add defines for 0x1, 0x103 and 0x303. Also please consider
> > adding comments for the devices.
> >
>
> Ack, Initially added the comments for devices, but since only driver is
> handling this data, and clients won't refer this so removed it. Will
> consider adding it back.

It will help developers / porters who will try matching the SID and the device.

>  This actlr field value might different (other
> than 0x1,0x103,0x303) in other platforms as per my anticipation,
> depending on the bit settings, so won't the defines change with
> different platforms? Hence this register setting value might be apt?

It is simple. 0x1, 0x103 and 0x303 are pure magic values. Please
provide sensible defines so that we can understand and review them.

Other platforms might use new defines.

>
> >> +
> >>   static struct qcom_smmu *to_qcom_smmu(struct arm_smmu_device *smmu)
> >>   {
> >>          return container_of(smmu, struct qcom_smmu, smmu);
> >> @@ -444,6 +502,16 @@ static const struct arm_smmu_impl sdm845_smmu_500_impl = {
> >>          .tlb_sync = qcom_smmu_tlb_sync,
> >>   };
> >>
> >> +
> >> +static const struct arm_smmu_impl sm8550_smmu_500_impl = {
> >> +       .init_context = qcom_smmu_init_context,
> >> +       .cfg_probe = qcom_smmu_cfg_probe,
> >> +       .def_domain_type = qcom_smmu_def_domain_type,
> >> +       .reset = arm_mmu500_reset,
> >> +       .write_s2cr = qcom_smmu_write_s2cr,
> >> +       .tlb_sync = qcom_smmu_tlb_sync,
> >> +};
> >> +
> >>   static const struct arm_smmu_impl qcom_adreno_smmu_v2_impl = {
> >>          .init_context = qcom_adreno_smmu_init_context,
> >>          .def_domain_type = qcom_smmu_def_domain_type,
> >> @@ -507,6 +575,11 @@ static const struct qcom_smmu_config qcom_smmu_impl0_cfg = {
> >>          .reg_offset = qcom_smmu_impl0_reg_offset,
> >>   };
> >>
> >> +static const struct actlr_config sm8550_actlrcfg = {
> >> +       .adata = sm8550_apps_actlr_data,
> >> +       .size = ARRAY_SIZE(sm8550_apps_actlr_data),
> >> +};
> >> +
> >>   /*
> >>    * It is not yet possible to use MDP SMMU with the bypass quirk on the msm8996,
> >>    * there are not enough context banks.
> >> @@ -530,16 +603,20 @@ static const struct qcom_smmu_match_data sdm845_smmu_500_data = {
> >>          /* Also no debug configuration. */
> >>   };
> >>
> >> +
> >> +static const struct qcom_smmu_match_data sm8550_smmu_500_impl0_data = {
> >> +       .impl = &sm8550_smmu_500_impl,
> >> +       .adreno_impl = &qcom_adreno_smmu_500_impl,
> >> +       .cfg = &qcom_smmu_impl0_cfg,
> >> +       .actlrcfg = &sm8550_actlrcfg,
> >> +};
> >
> > This structure doesn't seem to be linked. Did you test your patches?
> >
>
> Apologies Dmitry, just checked back my patches, I tested it but while
> refining the patches I somehow missed this link
> { .compatible = "qcom,sm8550-smmu-500", .data =
> &sm8550_smmu_500_impl0_data };
> in below qcom_smmu_500_impl0_data structure.
> I will take care of this in next version.
>
> >> +
> >>   static const struct qcom_smmu_match_data qcom_smmu_500_impl0_data = {
> >>          .impl = &qcom_smmu_500_impl,
> >>          .adreno_impl = &qcom_adreno_smmu_500_impl,
> >>          .cfg = &qcom_smmu_impl0_cfg,
> >>   };
> >>
> >> -/*
> >> - * Do not add any more qcom,SOC-smmu-500 entries to this list, unless they need
> >> - * special handling and can not be covered by the qcom,smmu-500 entry.
> >> - */
> >
> > Leave the comment in place.
> >
>
> Thanks for this comment which helped me to note the above mentioned
> mistake.
>
> >>   static const struct of_device_id __maybe_unused qcom_smmu_impl_of_match[] = {
> >>          { .compatible = "qcom,msm8996-smmu-v2", .data = &msm8996_smmu_data },
> >>          { .compatible = "qcom,msm8998-smmu-v2", .data = &qcom_smmu_v2_data },
> >> --
> >> 2.17.1
> >>
> >
> >



-- 
With best wishes
Dmitry

Re: [PATCH 3/3] iommu/arm-smmu: re-enable context caching in smmu reset operation

[Bibek Kumar Patro]

On 11/4/2023 3:53 AM, Dmitry Baryshkov wrote:
> On Sat, 4 Nov 2023 at 00:07, Bibek Kumar Patro
> <quic_bibekkum@quicinc.com> wrote:
>>
>>
>>
>> On 11/4/2023 3:28 AM, Dmitry Baryshkov wrote:
>>> On Fri, 3 Nov 2023 at 23:53, Bibek Kumar Patro
>>> <quic_bibekkum@quicinc.com> wrote:
>>>>
>>>> Context caching is re-enabled in the prefetch buffer for Qualcomm SoCs
>>>> through SoC specific reset ops, which is disabled in the default MMU-500
>>>> reset ops, but is expected for context banks using ACTLR register to
>>>> retain the prefetch value during reset and runtime suspend.
>>>>
>>>> Signed-off-by: Bibek Kumar Patro <quic_bibekkum@quicinc.com>
>>>> ---
>>>>    drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 26 ++++++++++++++++++----
>>>>    1 file changed, 22 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>> index 590b7c285299..f342b4778cf1 100644
>>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>> @@ -457,11 +457,29 @@ static int qcom_smmu_def_domain_type(struct device *dev)
>>>>           return match ? IOMMU_DOMAIN_IDENTITY : 0;
>>>>    }
>>>>
>>>> +#define ARM_MMU500_ACTLR_CPRE          BIT(1)
>>>> +
>>>> +static int qcom_smmu500_reset(struct arm_smmu_device *smmu)
>>>> +{
>>>> +       int i;
>>>> +       u32 reg;
>>>> +
>>>> +       arm_mmu500_reset(smmu);
>>>> +
>>>> +               for (i = 0; i < smmu->num_context_banks; ++i) {
>>>> +                       reg = arm_smmu_cb_read(smmu, i, ARM_SMMU_CB_ACTLR);
>>>> +                       reg |= ARM_MMU500_ACTLR_CPRE;
>>>> +                       arm_smmu_cb_write(smmu, i, ARM_SMMU_CB_ACTLR, reg);
>>>> +               }
>>>
>>> Wrong indentation. Did you run your patches through checkpatch.pl?
>>>
>>
>> Yes Dmitry, I did run checkpatch.pl script on this patch as well as
>> others, got 0 errors and 0 warnings. With -f option as well. Did not
>> get any related errors and warnings.
> 
> Ack, I beg your pardon. checkpatch indeed doesn't warn about this
> indentation. Though it is still incorrect.
> 

Ack, thanks for pointing this out. Will fix this indentation for loop in
next iteration.

>>
>>>> +
>>>> +       return 0;
>>>> +}
>>>> +
>>>>    static int qcom_sdm845_smmu500_reset(struct arm_smmu_device *smmu)
>>>>    {
>>>>           int ret;
>>>>
>>>> -       arm_mmu500_reset(smmu);
>>>> +       qcom_smmu500_reset(smmu);
>>>>
>>>>           /*
>>>>            * To address performance degradation in non-real time clients,
>>>> @@ -488,7 +506,7 @@ static const struct arm_smmu_impl qcom_smmu_500_impl = {
>>>>           .init_context = qcom_smmu_init_context,
>>>>           .cfg_probe = qcom_smmu_cfg_probe,
>>>>           .def_domain_type = qcom_smmu_def_domain_type,
>>>> -       .reset = arm_mmu500_reset,
>>>> +       .reset = qcom_smmu500_reset,
>>>>           .write_s2cr = qcom_smmu_write_s2cr,
>>>>           .tlb_sync = qcom_smmu_tlb_sync,
>>>>    };
>>>> @@ -507,7 +525,7 @@ static const struct arm_smmu_impl sm8550_smmu_500_impl = {
>>>>           .init_context = qcom_smmu_init_context,
>>>>           .cfg_probe = qcom_smmu_cfg_probe,
>>>>           .def_domain_type = qcom_smmu_def_domain_type,
>>>> -       .reset = arm_mmu500_reset,
>>>> +       .reset = qcom_smmu500_reset,
>>>>           .write_s2cr = qcom_smmu_write_s2cr,
>>>>           .tlb_sync = qcom_smmu_tlb_sync,
>>>>    };
>>>> @@ -523,7 +541,7 @@ static const struct arm_smmu_impl qcom_adreno_smmu_v2_impl = {
>>>>    static const struct arm_smmu_impl qcom_adreno_smmu_500_impl = {
>>>>           .init_context = qcom_adreno_smmu_init_context,
>>>>           .def_domain_type = qcom_smmu_def_domain_type,
>>>> -       .reset = arm_mmu500_reset,
>>>> +       .reset = qcom_smmu500_reset,
>>>>           .alloc_context_bank = qcom_adreno_smmu_alloc_context_bank,
>>>>           .write_sctlr = qcom_adreno_smmu_write_sctlr,
>>>>           .tlb_sync = qcom_smmu_tlb_sync,
>>>> --
>>>> 2.17.1
>>>>
>>>
>>>
> 
> 
> 

Re: [PATCH 0/3] iommu/arm-smmu: introduction of ACTLR implementation for Qualcomm SoCs

[Dmitry Baryshkov]

On Fri, 3 Nov 2023 at 23:53, Bibek Kumar Patro
<quic_bibekkum@quicinc.com> wrote:
>
> This patch series consists of three parts and covers the following:
>
> 1. Introducing intital set of driver changes to implement ACTLR register
> for custom prefetcher settings in Qualcomm SoCs.
>
> 2. Adding ACTLR data and implementation operations for SM8550.
>
> 3. Re-enabling context caching for Qualcomm SoCs to retain prefetcher
> settings during reset and runtime suspend.
>
> Link to discussion on RFC posted on ACTLR implementation:
> https://lore.kernel.org/all/a01e7e60-6ead-4a9e-ba90-22a8a6bbd03f@quicinc.com/

First two patches in that thread were not archived for some reason.

However, as there was a previous iteration, it is suggested to include
the changelog between RFC and v1.

>
> Bibek Kumar Patro (3):
>   iommu/arm-smmu: introduction of ACTLR for custom prefetcher settings
>   iommu/arm-smmu: add ACTLR data and support for sm8550
>   iommu/arm-smmu: re-enable context caching in smmu reset operation
>
>  drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 143 ++++++++++++++++++++-
>  drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h |   2 +

Although it is not mentioned in the MAINTAINERS file, could you please
CC linux-arm-msm when sending v2?

>  drivers/iommu/arm/arm-smmu/arm-smmu.c      |   5 +-
>  drivers/iommu/arm/arm-smmu/arm-smmu.h      |   5 +
>  4 files changed, 146 insertions(+), 9 deletions(-)
>
> --
> 2.17.1
>


-- 
With best wishes
Dmitry

Re: [PATCH 2/3] iommu/arm-smmu: add ACTLR data and support for SM8550

[Konrad Dybcio]

On 11/3/23 22:51, Bibek Kumar Patro wrote:
> Add ACTLR data table for SM8550 along with support for
> same including SM8550 specific implementation operations.
> 
> Signed-off-by: Bibek Kumar Patro <quic_bibekkum@quicinc.com>
> ---
>   drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 85 +++++++++++++++++++++-
>   1 file changed, 81 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> index 68c1f4908473..590b7c285299 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> @@ -25,6 +25,64 @@ struct actlr_data {
>   	u32 actlr;
>   };
> 
> +static const struct actlr_data sm8550_apps_actlr_data[] = {
I assume this data will be different for each SoC.. perhaps
moving this to a separate file (not sure if dt makes sense if
it's hardcoded per platform) makes sense.

This will also assume that these can not differ between firmware
versions.

Konrad

Re: [PATCH 3/3] iommu/arm-smmu: re-enable context caching in smmu reset operation

[Konrad Dybcio]

On 11/3/23 22:51, Bibek Kumar Patro wrote:
> Context caching is re-enabled in the prefetch buffer for Qualcomm SoCs
> through SoC specific reset ops, which is disabled in the default MMU-500
> reset ops, but is expected for context banks using ACTLR register to
> retain the prefetch value during reset and runtime suspend.
> 
> Signed-off-by: Bibek Kumar Patro <quic_bibekkum@quicinc.com>
> ---
>   drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 26 ++++++++++++++++++----
>   1 file changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> index 590b7c285299..f342b4778cf1 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> @@ -457,11 +457,29 @@ static int qcom_smmu_def_domain_type(struct device *dev)
>   	return match ? IOMMU_DOMAIN_IDENTITY : 0;
>   }
> 
> +#define ARM_MMU500_ACTLR_CPRE          BIT(1)
> +
> +static int qcom_smmu500_reset(struct arm_smmu_device *smmu)
> +{
> +	int i;
> +	u32 reg;
> +
> +	arm_mmu500_reset(smmu);
> +
> +		for (i = 0; i < smmu->num_context_banks; ++i) {
This loop deserves a comment above it like

/* Re-enable context caching after reset */

Konrad

Re: [PATCH 2/3] iommu/arm-smmu: add ACTLR data and support for SM8550

[kernel test robot]

Hi Bibek,

kernel test robot noticed the following build warnings:

[auto build test WARNING on v6.6]
[also build test WARNING on linus/master next-20231103]
[cannot apply to joro-iommu/next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Bibek-Kumar-Patro/iommu-arm-smmu-introduction-of-ACTLR-for-custom-prefetcher-settings/20231104-055625
base:   v6.6
patch link:    https://lore.kernel.org/r/20231103215124.1095-3-quic_bibekkum%40quicinc.com
patch subject: [PATCH 2/3] iommu/arm-smmu: add ACTLR data and support for SM8550
config: arm64-allyesconfig (https://download.01.org/0day-ci/archive/20231104/202311042304.pVLm1ZYk-lkp@intel.com/config)
compiler: aarch64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231104/202311042304.pVLm1ZYk-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202311042304.pVLm1ZYk-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c:614:42: warning: 'sm8550_smmu_500_impl0_data' defined but not used [-Wunused-const-variable=]
     614 | static const struct qcom_smmu_match_data sm8550_smmu_500_impl0_data = {
         |                                          ^~~~~~~~~~~~~~~~~~~~~~~~~~


vim +/sm8550_smmu_500_impl0_data +614 drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c

   612	
   613	
 > 614	static const struct qcom_smmu_match_data sm8550_smmu_500_impl0_data = {
   615		.impl = &sm8550_smmu_500_impl,
   616		.adreno_impl = &qcom_adreno_smmu_500_impl,
   617		.cfg = &qcom_smmu_impl0_cfg,
   618		.actlrcfg = &sm8550_actlrcfg,
   619	};
   620	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 2/3] iommu/arm-smmu: add ACTLR data and support for SM8550

[Bibek Kumar Patro]

On 11/4/2023 4:11 AM, Dmitry Baryshkov wrote:
> On Sat, 4 Nov 2023 at 00:38, Bibek Kumar Patro
> <quic_bibekkum@quicinc.com> wrote:
>>
>>
>>
>> On 11/4/2023 3:31 AM, Dmitry Baryshkov wrote:
>>> On Fri, 3 Nov 2023 at 23:53, Bibek Kumar Patro
>>> <quic_bibekkum@quicinc.com> wrote:
>>>>
>>>> Add ACTLR data table for SM8550 along with support for
>>>> same including SM8550 specific implementation operations.
>>>>
>>>> Signed-off-by: Bibek Kumar Patro <quic_bibekkum@quicinc.com>
>>>> ---
>>>>    drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 85 +++++++++++++++++++++-
>>>>    1 file changed, 81 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>> index 68c1f4908473..590b7c285299 100644
>>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>> @@ -25,6 +25,64 @@ struct actlr_data {
>>>>           u32 actlr;
>>>>    };
>>>>
>>>> +static const struct actlr_data sm8550_apps_actlr_data[] = {
>>>> +       { 0x18a0, 0x0000, 0x00000103 },
>>>> +       { 0x18e0, 0x0000, 0x00000103 },
>>>> +       { 0x0800, 0x0020, 0x00000001 },
>>>> +       { 0x1800, 0x00c0, 0x00000001 },
>>>> +       { 0x1820, 0x0000, 0x00000001 },
>>>> +       { 0x1860, 0x0000, 0x00000001 },
>>>> +       { 0x0c01, 0x0020, 0x00000303 },
>>>> +       { 0x0c02, 0x0020, 0x00000303 },
>>>> +       { 0x0c03, 0x0020, 0x00000303 },
>>>> +       { 0x0c04, 0x0020, 0x00000303 },
>>>> +       { 0x0c05, 0x0020, 0x00000303 },
>>>> +       { 0x0c06, 0x0020, 0x00000303 },
>>>> +       { 0x0c07, 0x0020, 0x00000303 },
>>>> +       { 0x0c08, 0x0020, 0x00000303 },
>>>> +       { 0x0c09, 0x0020, 0x00000303 },
>>>> +       { 0x0c0c, 0x0020, 0x00000303 },
>>>> +       { 0x0c0d, 0x0020, 0x00000303 },
>>>> +       { 0x0c0e, 0x0020, 0x00000303 },
>>>> +       { 0x0c0f, 0x0020, 0x00000303 },
>>>> +       { 0x1961, 0x0000, 0x00000303 },
>>>> +       { 0x1962, 0x0000, 0x00000303 },
>>>> +       { 0x1963, 0x0000, 0x00000303 },
>>>> +       { 0x1964, 0x0000, 0x00000303 },
>>>> +       { 0x1965, 0x0000, 0x00000303 },
>>>> +       { 0x1966, 0x0000, 0x00000303 },
>>>> +       { 0x1967, 0x0000, 0x00000303 },
>>>> +       { 0x1968, 0x0000, 0x00000303 },
>>>> +       { 0x1969, 0x0000, 0x00000303 },
>>>> +       { 0x196c, 0x0000, 0x00000303 },
>>>> +       { 0x196d, 0x0000, 0x00000303 },
>>>> +       { 0x196e, 0x0000, 0x00000303 },
>>>> +       { 0x196f, 0x0000, 0x00000303 },
>>>> +       { 0x19c1, 0x0010, 0x00000303 },
>>>> +       { 0x19c2, 0x0010, 0x00000303 },
>>>> +       { 0x19c3, 0x0010, 0x00000303 },
>>>> +       { 0x19c4, 0x0010, 0x00000303 },
>>>> +       { 0x19c5, 0x0010, 0x00000303 },
>>>> +       { 0x19c6, 0x0010, 0x00000303 },
>>>> +       { 0x19c7, 0x0010, 0x00000303 },
>>>> +       { 0x19c8, 0x0010, 0x00000303 },
>>>> +       { 0x19c9, 0x0010, 0x00000303 },
>>>> +       { 0x19cc, 0x0010, 0x00000303 },
>>>> +       { 0x19cd, 0x0010, 0x00000303 },
>>>> +       { 0x19ce, 0x0010, 0x00000303 },
>>>> +       { 0x19cf, 0x0010, 0x00000303 },
>>>> +       { 0x1c00, 0x0002, 0x00000103 },
>>>> +       { 0x1c01, 0x0000, 0x00000001 },
>>>> +       { 0x1920, 0x0000, 0x00000103 },
>>>> +       { 0x1923, 0x0000, 0x00000103 },
>>>> +       { 0x1924, 0x0000, 0x00000103 },
>>>> +       { 0x1940, 0x0000, 0x00000103 },
>>>> +       { 0x1941, 0x0004, 0x00000103 },
>>>> +       { 0x1943, 0x0000, 0x00000103 },
>>>> +       { 0x1944, 0x0000, 0x00000103 },
>>>> +       { 0x1947, 0x0000, 0x00000103 },
>>>> +};
>>>
>>> This is nearly impossible to handle.
>>> Please add defines for 0x1, 0x103 and 0x303. Also please consider
>>> adding comments for the devices.
>>>
>>
>> Ack, Initially added the comments for devices, but since only driver is
>> handling this data, and clients won't refer this so removed it. Will
>> consider adding it back.
> 
> It will help developers / porters who will try matching the SID and the device.
> 

Agree on the same, I'll add those comments for devices back.

>>   This actlr field value might different (other
>> than 0x1,0x103,0x303) in other platforms as per my anticipation,
>> depending on the bit settings, so won't the defines change with
>> different platforms? Hence this register setting value might be apt?
> 
> It is simple. 0x1, 0x103 and 0x303 are pure magic values. Please
> provide sensible defines so that we can understand and review them.
>

Understandable, In next patch I'll populate the actlr_data in
following format { SID, MASK, PRE_FETCH_n | CPRE | CMTLB }.
where " PRE_FETCH_n | CPRE | CMTLB " will be defines for
the actlr values (0x1,0x103,0x303).
This would help in understanding these values. Hope this
proposed format will be okay?


> Other platforms might use new defines.
> 
>>
>>>> +
>>>>    static struct qcom_smmu *to_qcom_smmu(struct arm_smmu_device *smmu)
>>>>    {
>>>>           return container_of(smmu, struct qcom_smmu, smmu);
>>>> @@ -444,6 +502,16 @@ static const struct arm_smmu_impl sdm845_smmu_500_impl = {
>>>>           .tlb_sync = qcom_smmu_tlb_sync,
>>>>    };
>>>>
>>>> +
>>>> +static const struct arm_smmu_impl sm8550_smmu_500_impl = {
>>>> +       .init_context = qcom_smmu_init_context,
>>>> +       .cfg_probe = qcom_smmu_cfg_probe,
>>>> +       .def_domain_type = qcom_smmu_def_domain_type,
>>>> +       .reset = arm_mmu500_reset,
>>>> +       .write_s2cr = qcom_smmu_write_s2cr,
>>>> +       .tlb_sync = qcom_smmu_tlb_sync,
>>>> +};
>>>> +
>>>>    static const struct arm_smmu_impl qcom_adreno_smmu_v2_impl = {
>>>>           .init_context = qcom_adreno_smmu_init_context,
>>>>           .def_domain_type = qcom_smmu_def_domain_type,
>>>> @@ -507,6 +575,11 @@ static const struct qcom_smmu_config qcom_smmu_impl0_cfg = {
>>>>           .reg_offset = qcom_smmu_impl0_reg_offset,
>>>>    };
>>>>
>>>> +static const struct actlr_config sm8550_actlrcfg = {
>>>> +       .adata = sm8550_apps_actlr_data,
>>>> +       .size = ARRAY_SIZE(sm8550_apps_actlr_data),
>>>> +};
>>>> +
>>>>    /*
>>>>     * It is not yet possible to use MDP SMMU with the bypass quirk on the msm8996,
>>>>     * there are not enough context banks.
>>>> @@ -530,16 +603,20 @@ static const struct qcom_smmu_match_data sdm845_smmu_500_data = {
>>>>           /* Also no debug configuration. */
>>>>    };
>>>>
>>>> +
>>>> +static const struct qcom_smmu_match_data sm8550_smmu_500_impl0_data = {
>>>> +       .impl = &sm8550_smmu_500_impl,
>>>> +       .adreno_impl = &qcom_adreno_smmu_500_impl,
>>>> +       .cfg = &qcom_smmu_impl0_cfg,
>>>> +       .actlrcfg = &sm8550_actlrcfg,
>>>> +};
>>>
>>> This structure doesn't seem to be linked. Did you test your patches?
>>>
>>
>> Apologies Dmitry, just checked back my patches, I tested it but while
>> refining the patches I somehow missed this link
>> { .compatible = "qcom,sm8550-smmu-500", .data =
>> &sm8550_smmu_500_impl0_data };
>> in below qcom_smmu_500_impl0_data structure.
>> I will take care of this in next version.
>>
>>>> +
>>>>    static const struct qcom_smmu_match_data qcom_smmu_500_impl0_data = {
>>>>           .impl = &qcom_smmu_500_impl,
>>>>           .adreno_impl = &qcom_adreno_smmu_500_impl,
>>>>           .cfg = &qcom_smmu_impl0_cfg,
>>>>    };
>>>>
>>>> -/*
>>>> - * Do not add any more qcom,SOC-smmu-500 entries to this list, unless they need
>>>> - * special handling and can not be covered by the qcom,smmu-500 entry.
>>>> - */
>>>
>>> Leave the comment in place.
>>>
>>
>> Thanks for this comment which helped me to note the above mentioned
>> mistake.
>>
>>>>    static const struct of_device_id __maybe_unused qcom_smmu_impl_of_match[] = {
>>>>           { .compatible = "qcom,msm8996-smmu-v2", .data = &msm8996_smmu_data },
>>>>           { .compatible = "qcom,msm8998-smmu-v2", .data = &qcom_smmu_v2_data },
>>>> --
>>>> 2.17.1
>>>>
>>>
>>>
> 
> 
> 

Re: [PATCH 1/3] iommu/arm-smmu: introduction of ACTLR for custom prefetcher settings

[Bibek Kumar Patro]

On 11/4/2023 3:33 AM, Dmitry Baryshkov wrote:
> On Fri, 3 Nov 2023 at 23:53, Bibek Kumar Patro
> <quic_bibekkum@quicinc.com> wrote:
>>
>> Currently in Qualcomm  SoCs the default prefetch is set to 1 which allows
>> the TLB to fetch just the next page table. MMU-500 features ACTLR
>> register which is implementation defined and is used for Qualcomm SoCs
>> to have a prefetch setting of 1/3/7/15 enabling TLB to prefetch
>> the next set of page tables accordingly allowing for faster translations.
>>
>> ACTLR value is unique for each SMR (Stream matching register) and stored
>> in a pre-populated table. This value is set to the register during
>> context bank initialisation.
>>
>> Signed-off-by: Bibek Kumar Patro <quic_bibekkum@quicinc.com>
>> ---
>>   drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 34 ++++++++++++++++++++++
>>   drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h |  2 ++
>>   drivers/iommu/arm/arm-smmu/arm-smmu.c      |  5 ++--
>>   drivers/iommu/arm/arm-smmu/arm-smmu.h      |  5 ++++
>>   4 files changed, 44 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>> index ae7cae015193..68c1f4908473 100644
>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>> @@ -14,6 +14,17 @@
>>
>>   #define QCOM_DUMMY_VAL -1
>>
>> +struct actlr_config {
>> +       const struct actlr_data *adata;
>> +       u32 size;
> 
> This should be size_t.
> 
> Also could you please drop the separate struct actlr_config and move
> these two fields into struct qcom_smmu_config.
> 

Ack, will address both these inputs in the next patch.

>> +};
>> +
>> +struct actlr_data {
>> +       u16 sid;
>> +       u16 mask;
>> +       u32 actlr;
>> +};
>> +
>>   static struct qcom_smmu *to_qcom_smmu(struct arm_smmu_device *smmu)
>>   {
>>          return container_of(smmu, struct qcom_smmu, smmu);
>> @@ -270,6 +281,26 @@ static const struct of_device_id qcom_smmu_client_of_match[] __maybe_unused = {
>>   static int qcom_smmu_init_context(struct arm_smmu_domain *smmu_domain,
>>                  struct io_pgtable_cfg *pgtbl_cfg, struct device *dev)
>>   {
>> +       struct arm_smmu_device *smmu = smmu_domain->smmu;
>> +       struct qcom_smmu *qsmmu = to_qcom_smmu(smmu);
>> +       const struct actlr_config *actlrcfg;
>> +       struct arm_smmu_smr *smr = smmu->smrs;
>> +       int idx = smmu_domain->cfg.cbndx;
>> +       int i;
>> +       u16 id;
>> +       u16 mask;
>> +
>> +       if (qsmmu->actlrcfg) {
>> +               actlrcfg = qsmmu->actlrcfg;
>> +               for (i = 0; i < actlrcfg->size; ++i) {
>> +                       id = actlrcfg->adata[i].sid;
>> +                       mask = actlrcfg->adata[i].mask;
>> +                       if (!smr_is_subset(*smr, id, mask))
>> +                               arm_smmu_cb_write(smmu, idx, ARM_SMMU_CB_ACTLR,
>> +                                               actlrcfg->adata[i].actlr);
>> +               }
>> +       }
> 
> Consider extracting this to a separate function. This way you can
> reduce 4 indentation levels into a single loop.
> 

Ack, thanks for this sugestion. Will move this entire for loop into a 
separate function for simplicity reduced indent levels.

>> +
>>          smmu_domain->cfg.flush_walk_prefer_tlbiasid = true;
>>
>>          return 0;
>> @@ -459,6 +490,9 @@ static struct arm_smmu_device *qcom_smmu_create(struct arm_smmu_device *smmu,
>>          qsmmu->smmu.impl = impl;
>>          qsmmu->cfg = data->cfg;
>>
>> +       if (data->actlrcfg && (data->actlrcfg->size))
>> +               qsmmu->actlrcfg = data->actlrcfg;
>> +
>>          return &qsmmu->smmu;
>>   }
>>
>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h
>> index 593910567b88..4b6862715070 100644
>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h
>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h
>> @@ -9,6 +9,7 @@
>>   struct qcom_smmu {
>>          struct arm_smmu_device smmu;
>>          const struct qcom_smmu_config *cfg;
>> +       const struct actlr_config *actlrcfg;
>>          bool bypass_quirk;
>>          u8 bypass_cbndx;
>>          u32 stall_enabled;
>> @@ -25,6 +26,7 @@ struct qcom_smmu_config {
>>   };
>>
>>   struct qcom_smmu_match_data {
>> +       const struct actlr_config *actlrcfg;
>>          const struct qcom_smmu_config *cfg;
>>          const struct arm_smmu_impl *impl;
>>          const struct arm_smmu_impl *adreno_impl;
>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
>> index 4c79ef6f4c75..38ac1cbc799b 100644
>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
>> @@ -992,9 +992,10 @@ static int arm_smmu_find_sme(struct arm_smmu_device *smmu, u16 id, u16 mask)
>>                   * expect simply identical entries for this case, but there's
>>                   * no harm in accommodating the generalisation.
>>                   */
>> -               if ((mask & smrs[i].mask) == mask &&
>> -                   !((id ^ smrs[i].id) & ~smrs[i].mask))
>> +
>> +               if (smr_is_subset(smrs[i], id, mask))
>>                          return i;
>> +
>>                  /*
>>                   * If the new entry has any other overlap with an existing one,
>>                   * though, then there always exists at least one stream ID
>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.h b/drivers/iommu/arm/arm-smmu/arm-smmu.h
>> index 703fd5817ec1..b1638bbc41d4 100644
>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.h
>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.h
>> @@ -501,6 +501,11 @@ static inline void arm_smmu_writeq(struct arm_smmu_device *smmu, int page,
>>                  writeq_relaxed(val, arm_smmu_page(smmu, page) + offset);
>>   }
>>
>> +static inline bool smr_is_subset(struct arm_smmu_smr smrs, u16 id, u16 mask)
>> +{
>> +       return (mask & smrs.mask) == mask && !((id ^ smrs.id) & ~smrs.mask);
>> +}
>> +
>>   #define ARM_SMMU_GR0           0
>>   #define ARM_SMMU_GR1           1
>>   #define ARM_SMMU_CB(s, n)      ((s)->numpage + (n))
>> --
>> 2.17.1
>>
> 
> 

Re: [PATCH 2/3] iommu/arm-smmu: add ACTLR data and support for SM8550

[Bibek Kumar Patro]

On 11/4/2023 4:59 PM, Konrad Dybcio wrote:
> 
> 
> On 11/3/23 22:51, Bibek Kumar Patro wrote:
>> Add ACTLR data table for SM8550 along with support for
>> same including SM8550 specific implementation operations.
>>
>> Signed-off-by: Bibek Kumar Patro <quic_bibekkum@quicinc.com>
>> ---
>>   drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 85 +++++++++++++++++++++-
>>   1 file changed, 81 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c 
>> b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>> index 68c1f4908473..590b7c285299 100644
>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>> @@ -25,6 +25,64 @@ struct actlr_data {
>>       u32 actlr;
>>   };
>>
>> +static const struct actlr_data sm8550_apps_actlr_data[] = {
> I assume this data will be different for each SoC.. perhaps
> moving this to a separate file (not sure if dt makes sense if
> it's hardcoded per platform) makes sense.
> 

Yes, this data will be different for each SoC.
Right, adding these properties in dt won't be a right thing
since this is a register setting and not a hardware defining property.
As per my understanding passing register content via device tree is 
highly discouraged, so hosting this data inside the driver.
For reference, adding the RFC link archiving this discussion:
https://lore.kernel.org/all/a01e7e60-6ead-4a9e-ba90-22a8a6bbd03f@quicinc.com/#t
If my recollection is correct, some drivers like llcc is also
following similar implementation
drivers/phy/qualcomm/phy-qcom-qmp-combo.c
drivers/soc/qcom/llcc-qcom.c


> This will also assume that these can not differ between firmware
> versions.
>

Right, these won't differ between firmware versions.

Thanks & regards,
Bibek

> Konrad

Re: [PATCH 3/3] iommu/arm-smmu: re-enable context caching in smmu reset operation

[Bibek Kumar Patro]

On 11/4/2023 5:00 PM, Konrad Dybcio wrote:
> 
> 
> On 11/3/23 22:51, Bibek Kumar Patro wrote:
>> Context caching is re-enabled in the prefetch buffer for Qualcomm SoCs
>> through SoC specific reset ops, which is disabled in the default MMU-500
>> reset ops, but is expected for context banks using ACTLR register to
>> retain the prefetch value during reset and runtime suspend.
>>
>> Signed-off-by: Bibek Kumar Patro <quic_bibekkum@quicinc.com>
>> ---
>>   drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 26 ++++++++++++++++++----
>>   1 file changed, 22 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c 
>> b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>> index 590b7c285299..f342b4778cf1 100644
>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>> @@ -457,11 +457,29 @@ static int qcom_smmu_def_domain_type(struct 
>> device *dev)
>>       return match ? IOMMU_DOMAIN_IDENTITY : 0;
>>   }
>>
>> +#define ARM_MMU500_ACTLR_CPRE          BIT(1)
>> +
>> +static int qcom_smmu500_reset(struct arm_smmu_device *smmu)
>> +{
>> +    int i;
>> +    u32 reg;
>> +
>> +    arm_mmu500_reset(smmu);
>> +
>> +        for (i = 0; i < smmu->num_context_banks; ++i) {
> This loop deserves a comment above it like
> 
> /* Re-enable context caching after reset */
> 

Ack, thanks for this suggestion, this might help to explain
this addition.
I would incorporate this in next patch.

regards,
Bibek

> Konrad

Re: [PATCH 0/3] iommu/arm-smmu: introduction of ACTLR implementation for Qualcomm SoCs

[Bibek Kumar Patro]

On 11/4/2023 4:14 AM, Dmitry Baryshkov wrote:
> On Fri, 3 Nov 2023 at 23:53, Bibek Kumar Patro
> <quic_bibekkum@quicinc.com> wrote:
>>
>> This patch series consists of three parts and covers the following:
>>
>> 1. Introducing intital set of driver changes to implement ACTLR register
>> for custom prefetcher settings in Qualcomm SoCs.
>>
>> 2. Adding ACTLR data and implementation operations for SM8550.
>>
>> 3. Re-enabling context caching for Qualcomm SoCs to retain prefetcher
>> settings during reset and runtime suspend.
>>
>> Link to discussion on RFC posted on ACTLR implementation:
>> https://lore.kernel.org/all/a01e7e60-6ead-4a9e-ba90-22a8a6bbd03f@quicinc.com/
> 
> First two patches in that thread were not archived for some reason.
> 
> However, as there was a previous iteration, it is suggested to include
> the changelog between RFC and v1.
> 

Right, in the initial RFC sent somehow only the replies were archived.
Although the RFC patch was a WIP patch in rough state, sent to take 
upstream inputs.
Will try adding the changelog in v2.
To give a context, in the RFC patch, we we're initially proposing a dt
based approach to set the actlr property, where we would piggyback on 
the iommus property to store actlr value and set the property during
smmu device probe.

>>
>> Bibek Kumar Patro (3):
>>    iommu/arm-smmu: introduction of ACTLR for custom prefetcher settings
>>    iommu/arm-smmu: add ACTLR data and support for sm8550
>>    iommu/arm-smmu: re-enable context caching in smmu reset operation
>>
>>   drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 143 ++++++++++++++++++++-
>>   drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h |   2 +
> 
> Although it is not mentioned in the MAINTAINERS file, could you please
> CC linux-arm-msm when sending v2?
> 

Sure Dmitry, will take care of this and CC linux-arm-msm while sending v2.

>>   drivers/iommu/arm/arm-smmu/arm-smmu.c      |   5 +-
>>   drivers/iommu/arm/arm-smmu/arm-smmu.h      |   5 +
>>   4 files changed, 146 insertions(+), 9 deletions(-)
>>
>> --
>> 2.17.1
>>
> 
> 

Re: [PATCH 2/3] iommu/arm-smmu: add ACTLR data and support for SM8550

[Dmitry Baryshkov]

On Mon, 6 Nov 2023 at 08:10, Bibek Kumar Patro
<quic_bibekkum@quicinc.com> wrote:
>
>
>
> On 11/4/2023 4:11 AM, Dmitry Baryshkov wrote:
> > On Sat, 4 Nov 2023 at 00:38, Bibek Kumar Patro
> > <quic_bibekkum@quicinc.com> wrote:
> >>
> >>
> >>
> >> On 11/4/2023 3:31 AM, Dmitry Baryshkov wrote:
> >>> On Fri, 3 Nov 2023 at 23:53, Bibek Kumar Patro
> >>> <quic_bibekkum@quicinc.com> wrote:
> >>>>
> >>>> Add ACTLR data table for SM8550 along with support for
> >>>> same including SM8550 specific implementation operations.
> >>>>
> >>>> Signed-off-by: Bibek Kumar Patro <quic_bibekkum@quicinc.com>
> >>>> ---
> >>>>    drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 85 +++++++++++++++++++++-
> >>>>    1 file changed, 81 insertions(+), 4 deletions(-)
> >>>>
> >>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> >>>> index 68c1f4908473..590b7c285299 100644
> >>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> >>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> >>>> @@ -25,6 +25,64 @@ struct actlr_data {
> >>>>           u32 actlr;
> >>>>    };
> >>>>
> >>>> +static const struct actlr_data sm8550_apps_actlr_data[] = {
> >>>> +       { 0x18a0, 0x0000, 0x00000103 },
> >>>> +       { 0x18e0, 0x0000, 0x00000103 },
> >>>> +       { 0x0800, 0x0020, 0x00000001 },
> >>>> +       { 0x1800, 0x00c0, 0x00000001 },
> >>>> +       { 0x1820, 0x0000, 0x00000001 },
> >>>> +       { 0x1860, 0x0000, 0x00000001 },
> >>>> +       { 0x0c01, 0x0020, 0x00000303 },
> >>>> +       { 0x0c02, 0x0020, 0x00000303 },
> >>>> +       { 0x0c03, 0x0020, 0x00000303 },
> >>>> +       { 0x0c04, 0x0020, 0x00000303 },
> >>>> +       { 0x0c05, 0x0020, 0x00000303 },
> >>>> +       { 0x0c06, 0x0020, 0x00000303 },
> >>>> +       { 0x0c07, 0x0020, 0x00000303 },
> >>>> +       { 0x0c08, 0x0020, 0x00000303 },
> >>>> +       { 0x0c09, 0x0020, 0x00000303 },
> >>>> +       { 0x0c0c, 0x0020, 0x00000303 },
> >>>> +       { 0x0c0d, 0x0020, 0x00000303 },
> >>>> +       { 0x0c0e, 0x0020, 0x00000303 },
> >>>> +       { 0x0c0f, 0x0020, 0x00000303 },
> >>>> +       { 0x1961, 0x0000, 0x00000303 },
> >>>> +       { 0x1962, 0x0000, 0x00000303 },
> >>>> +       { 0x1963, 0x0000, 0x00000303 },
> >>>> +       { 0x1964, 0x0000, 0x00000303 },
> >>>> +       { 0x1965, 0x0000, 0x00000303 },
> >>>> +       { 0x1966, 0x0000, 0x00000303 },
> >>>> +       { 0x1967, 0x0000, 0x00000303 },
> >>>> +       { 0x1968, 0x0000, 0x00000303 },
> >>>> +       { 0x1969, 0x0000, 0x00000303 },
> >>>> +       { 0x196c, 0x0000, 0x00000303 },
> >>>> +       { 0x196d, 0x0000, 0x00000303 },
> >>>> +       { 0x196e, 0x0000, 0x00000303 },
> >>>> +       { 0x196f, 0x0000, 0x00000303 },
> >>>> +       { 0x19c1, 0x0010, 0x00000303 },
> >>>> +       { 0x19c2, 0x0010, 0x00000303 },
> >>>> +       { 0x19c3, 0x0010, 0x00000303 },
> >>>> +       { 0x19c4, 0x0010, 0x00000303 },
> >>>> +       { 0x19c5, 0x0010, 0x00000303 },
> >>>> +       { 0x19c6, 0x0010, 0x00000303 },
> >>>> +       { 0x19c7, 0x0010, 0x00000303 },
> >>>> +       { 0x19c8, 0x0010, 0x00000303 },
> >>>> +       { 0x19c9, 0x0010, 0x00000303 },
> >>>> +       { 0x19cc, 0x0010, 0x00000303 },
> >>>> +       { 0x19cd, 0x0010, 0x00000303 },
> >>>> +       { 0x19ce, 0x0010, 0x00000303 },
> >>>> +       { 0x19cf, 0x0010, 0x00000303 },
> >>>> +       { 0x1c00, 0x0002, 0x00000103 },
> >>>> +       { 0x1c01, 0x0000, 0x00000001 },
> >>>> +       { 0x1920, 0x0000, 0x00000103 },
> >>>> +       { 0x1923, 0x0000, 0x00000103 },
> >>>> +       { 0x1924, 0x0000, 0x00000103 },
> >>>> +       { 0x1940, 0x0000, 0x00000103 },
> >>>> +       { 0x1941, 0x0004, 0x00000103 },
> >>>> +       { 0x1943, 0x0000, 0x00000103 },
> >>>> +       { 0x1944, 0x0000, 0x00000103 },
> >>>> +       { 0x1947, 0x0000, 0x00000103 },
> >>>> +};
> >>>
> >>> This is nearly impossible to handle.
> >>> Please add defines for 0x1, 0x103 and 0x303. Also please consider
> >>> adding comments for the devices.
> >>>
> >>
> >> Ack, Initially added the comments for devices, but since only driver is
> >> handling this data, and clients won't refer this so removed it. Will
> >> consider adding it back.
> >
> > It will help developers / porters who will try matching the SID and the device.
> >
>
> Agree on the same, I'll add those comments for devices back.
>
> >>   This actlr field value might different (other
> >> than 0x1,0x103,0x303) in other platforms as per my anticipation,
> >> depending on the bit settings, so won't the defines change with
> >> different platforms? Hence this register setting value might be apt?
> >
> > It is simple. 0x1, 0x103 and 0x303 are pure magic values. Please
> > provide sensible defines so that we can understand and review them.
> >
>
> Understandable, In next patch I'll populate the actlr_data in
> following format { SID, MASK, PRE_FETCH_n | CPRE | CMTLB }.
> where " PRE_FETCH_n | CPRE | CMTLB " will be defines for
> the actlr values (0x1,0x103,0x303).
> This would help in understanding these values. Hope this
> proposed format will be okay?

Yes, this sounds good. Please also add description for CPRE and CMTLB bits.

>
>
> > Other platforms might use new defines.
> >
> >>
> >>>> +
> >>>>    static struct qcom_smmu *to_qcom_smmu(struct arm_smmu_device *smmu)
> >>>>    {
> >>>>           return container_of(smmu, struct qcom_smmu, smmu);
> >>>> @@ -444,6 +502,16 @@ static const struct arm_smmu_impl sdm845_smmu_500_impl = {
> >>>>           .tlb_sync = qcom_smmu_tlb_sync,
> >>>>    };
> >>>>
> >>>> +
> >>>> +static const struct arm_smmu_impl sm8550_smmu_500_impl = {
> >>>> +       .init_context = qcom_smmu_init_context,
> >>>> +       .cfg_probe = qcom_smmu_cfg_probe,
> >>>> +       .def_domain_type = qcom_smmu_def_domain_type,
> >>>> +       .reset = arm_mmu500_reset,
> >>>> +       .write_s2cr = qcom_smmu_write_s2cr,
> >>>> +       .tlb_sync = qcom_smmu_tlb_sync,
> >>>> +};
> >>>> +
> >>>>    static const struct arm_smmu_impl qcom_adreno_smmu_v2_impl = {
> >>>>           .init_context = qcom_adreno_smmu_init_context,
> >>>>           .def_domain_type = qcom_smmu_def_domain_type,
> >>>> @@ -507,6 +575,11 @@ static const struct qcom_smmu_config qcom_smmu_impl0_cfg = {
> >>>>           .reg_offset = qcom_smmu_impl0_reg_offset,
> >>>>    };
> >>>>
> >>>> +static const struct actlr_config sm8550_actlrcfg = {
> >>>> +       .adata = sm8550_apps_actlr_data,
> >>>> +       .size = ARRAY_SIZE(sm8550_apps_actlr_data),
> >>>> +};
> >>>> +
> >>>>    /*
> >>>>     * It is not yet possible to use MDP SMMU with the bypass quirk on the msm8996,
> >>>>     * there are not enough context banks.
> >>>> @@ -530,16 +603,20 @@ static const struct qcom_smmu_match_data sdm845_smmu_500_data = {
> >>>>           /* Also no debug configuration. */
> >>>>    };
> >>>>
> >>>> +
> >>>> +static const struct qcom_smmu_match_data sm8550_smmu_500_impl0_data = {
> >>>> +       .impl = &sm8550_smmu_500_impl,
> >>>> +       .adreno_impl = &qcom_adreno_smmu_500_impl,
> >>>> +       .cfg = &qcom_smmu_impl0_cfg,
> >>>> +       .actlrcfg = &sm8550_actlrcfg,
> >>>> +};
> >>>
> >>> This structure doesn't seem to be linked. Did you test your patches?
> >>>
> >>
> >> Apologies Dmitry, just checked back my patches, I tested it but while
> >> refining the patches I somehow missed this link
> >> { .compatible = "qcom,sm8550-smmu-500", .data =
> >> &sm8550_smmu_500_impl0_data };
> >> in below qcom_smmu_500_impl0_data structure.
> >> I will take care of this in next version.
> >>
> >>>> +
> >>>>    static const struct qcom_smmu_match_data qcom_smmu_500_impl0_data = {
> >>>>           .impl = &qcom_smmu_500_impl,
> >>>>           .adreno_impl = &qcom_adreno_smmu_500_impl,
> >>>>           .cfg = &qcom_smmu_impl0_cfg,
> >>>>    };
> >>>>
> >>>> -/*
> >>>> - * Do not add any more qcom,SOC-smmu-500 entries to this list, unless they need
> >>>> - * special handling and can not be covered by the qcom,smmu-500 entry.
> >>>> - */
> >>>
> >>> Leave the comment in place.
> >>>
> >>
> >> Thanks for this comment which helped me to note the above mentioned
> >> mistake.
> >>
> >>>>    static const struct of_device_id __maybe_unused qcom_smmu_impl_of_match[] = {
> >>>>           { .compatible = "qcom,msm8996-smmu-v2", .data = &msm8996_smmu_data },
> >>>>           { .compatible = "qcom,msm8998-smmu-v2", .data = &qcom_smmu_v2_data },
> >>>> --
> >>>> 2.17.1
> >>>>
> >>>
> >>>
> >
> >
> >



-- 
With best wishes
Dmitry

Re: [PATCH 2/3] iommu/arm-smmu: add ACTLR data and support for SM8550

[Bibek Kumar Patro]

On 11/6/2023 2:35 PM, Dmitry Baryshkov wrote:
> On Mon, 6 Nov 2023 at 08:10, Bibek Kumar Patro
> <quic_bibekkum@quicinc.com> wrote:
>>
>>
>>
>> On 11/4/2023 4:11 AM, Dmitry Baryshkov wrote:
>>> On Sat, 4 Nov 2023 at 00:38, Bibek Kumar Patro
>>> <quic_bibekkum@quicinc.com> wrote:
>>>>
>>>>
>>>>
>>>> On 11/4/2023 3:31 AM, Dmitry Baryshkov wrote:
>>>>> On Fri, 3 Nov 2023 at 23:53, Bibek Kumar Patro
>>>>> <quic_bibekkum@quicinc.com> wrote:
>>>>>>
>>>>>> Add ACTLR data table for SM8550 along with support for
>>>>>> same including SM8550 specific implementation operations.
>>>>>>
>>>>>> Signed-off-by: Bibek Kumar Patro <quic_bibekkum@quicinc.com>
>>>>>> ---
>>>>>>     drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 85 +++++++++++++++++++++-
>>>>>>     1 file changed, 81 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>>>> index 68c1f4908473..590b7c285299 100644
>>>>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>>>> @@ -25,6 +25,64 @@ struct actlr_data {
>>>>>>            u32 actlr;
>>>>>>     };
>>>>>>
>>>>>> +static const struct actlr_data sm8550_apps_actlr_data[] = {
>>>>>> +       { 0x18a0, 0x0000, 0x00000103 },
>>>>>> +       { 0x18e0, 0x0000, 0x00000103 },
>>>>>> +       { 0x0800, 0x0020, 0x00000001 },
>>>>>> +       { 0x1800, 0x00c0, 0x00000001 },
>>>>>> +       { 0x1820, 0x0000, 0x00000001 },
>>>>>> +       { 0x1860, 0x0000, 0x00000001 },
>>>>>> +       { 0x0c01, 0x0020, 0x00000303 },
>>>>>> +       { 0x0c02, 0x0020, 0x00000303 },
>>>>>> +       { 0x0c03, 0x0020, 0x00000303 },
>>>>>> +       { 0x0c04, 0x0020, 0x00000303 },
>>>>>> +       { 0x0c05, 0x0020, 0x00000303 },
>>>>>> +       { 0x0c06, 0x0020, 0x00000303 },
>>>>>> +       { 0x0c07, 0x0020, 0x00000303 },
>>>>>> +       { 0x0c08, 0x0020, 0x00000303 },
>>>>>> +       { 0x0c09, 0x0020, 0x00000303 },
>>>>>> +       { 0x0c0c, 0x0020, 0x00000303 },
>>>>>> +       { 0x0c0d, 0x0020, 0x00000303 },
>>>>>> +       { 0x0c0e, 0x0020, 0x00000303 },
>>>>>> +       { 0x0c0f, 0x0020, 0x00000303 },
>>>>>> +       { 0x1961, 0x0000, 0x00000303 },
>>>>>> +       { 0x1962, 0x0000, 0x00000303 },
>>>>>> +       { 0x1963, 0x0000, 0x00000303 },
>>>>>> +       { 0x1964, 0x0000, 0x00000303 },
>>>>>> +       { 0x1965, 0x0000, 0x00000303 },
>>>>>> +       { 0x1966, 0x0000, 0x00000303 },
>>>>>> +       { 0x1967, 0x0000, 0x00000303 },
>>>>>> +       { 0x1968, 0x0000, 0x00000303 },
>>>>>> +       { 0x1969, 0x0000, 0x00000303 },
>>>>>> +       { 0x196c, 0x0000, 0x00000303 },
>>>>>> +       { 0x196d, 0x0000, 0x00000303 },
>>>>>> +       { 0x196e, 0x0000, 0x00000303 },
>>>>>> +       { 0x196f, 0x0000, 0x00000303 },
>>>>>> +       { 0x19c1, 0x0010, 0x00000303 },
>>>>>> +       { 0x19c2, 0x0010, 0x00000303 },
>>>>>> +       { 0x19c3, 0x0010, 0x00000303 },
>>>>>> +       { 0x19c4, 0x0010, 0x00000303 },
>>>>>> +       { 0x19c5, 0x0010, 0x00000303 },
>>>>>> +       { 0x19c6, 0x0010, 0x00000303 },
>>>>>> +       { 0x19c7, 0x0010, 0x00000303 },
>>>>>> +       { 0x19c8, 0x0010, 0x00000303 },
>>>>>> +       { 0x19c9, 0x0010, 0x00000303 },
>>>>>> +       { 0x19cc, 0x0010, 0x00000303 },
>>>>>> +       { 0x19cd, 0x0010, 0x00000303 },
>>>>>> +       { 0x19ce, 0x0010, 0x00000303 },
>>>>>> +       { 0x19cf, 0x0010, 0x00000303 },
>>>>>> +       { 0x1c00, 0x0002, 0x00000103 },
>>>>>> +       { 0x1c01, 0x0000, 0x00000001 },
>>>>>> +       { 0x1920, 0x0000, 0x00000103 },
>>>>>> +       { 0x1923, 0x0000, 0x00000103 },
>>>>>> +       { 0x1924, 0x0000, 0x00000103 },
>>>>>> +       { 0x1940, 0x0000, 0x00000103 },
>>>>>> +       { 0x1941, 0x0004, 0x00000103 },
>>>>>> +       { 0x1943, 0x0000, 0x00000103 },
>>>>>> +       { 0x1944, 0x0000, 0x00000103 },
>>>>>> +       { 0x1947, 0x0000, 0x00000103 },
>>>>>> +};
>>>>>
>>>>> This is nearly impossible to handle.
>>>>> Please add defines for 0x1, 0x103 and 0x303. Also please consider
>>>>> adding comments for the devices.
>>>>>
>>>>
>>>> Ack, Initially added the comments for devices, but since only driver is
>>>> handling this data, and clients won't refer this so removed it. Will
>>>> consider adding it back.
>>>
>>> It will help developers / porters who will try matching the SID and the device.
>>>
>>
>> Agree on the same, I'll add those comments for devices back.
>>
>>>>    This actlr field value might different (other
>>>> than 0x1,0x103,0x303) in other platforms as per my anticipation,
>>>> depending on the bit settings, so won't the defines change with
>>>> different platforms? Hence this register setting value might be apt?
>>>
>>> It is simple. 0x1, 0x103 and 0x303 are pure magic values. Please
>>> provide sensible defines so that we can understand and review them.
>>>
>>
>> Understandable, In next patch I'll populate the actlr_data in
>> following format { SID, MASK, PRE_FETCH_n | CPRE | CMTLB }.
>> where " PRE_FETCH_n | CPRE | CMTLB " will be defines for
>> the actlr values (0x1,0x103,0x303).
>> This would help in understanding these values. Hope this
>> proposed format will be okay?
> 
> Yes, this sounds good. Please also add description for CPRE and CMTLB bits.
> 

Thanks for the confirmation. Sure will add the descriptions for
CPRE and CMTLB bits as well in v2

>>
>>
>>> Other platforms might use new defines.
>>>
>>>>
>>>>>> +
>>>>>>     static struct qcom_smmu *to_qcom_smmu(struct arm_smmu_device *smmu)
>>>>>>     {
>>>>>>            return container_of(smmu, struct qcom_smmu, smmu);
>>>>>> @@ -444,6 +502,16 @@ static const struct arm_smmu_impl sdm845_smmu_500_impl = {
>>>>>>            .tlb_sync = qcom_smmu_tlb_sync,
>>>>>>     };
>>>>>>
>>>>>> +
>>>>>> +static const struct arm_smmu_impl sm8550_smmu_500_impl = {
>>>>>> +       .init_context = qcom_smmu_init_context,
>>>>>> +       .cfg_probe = qcom_smmu_cfg_probe,
>>>>>> +       .def_domain_type = qcom_smmu_def_domain_type,
>>>>>> +       .reset = arm_mmu500_reset,
>>>>>> +       .write_s2cr = qcom_smmu_write_s2cr,
>>>>>> +       .tlb_sync = qcom_smmu_tlb_sync,
>>>>>> +};
>>>>>> +
>>>>>>     static const struct arm_smmu_impl qcom_adreno_smmu_v2_impl = {
>>>>>>            .init_context = qcom_adreno_smmu_init_context,
>>>>>>            .def_domain_type = qcom_smmu_def_domain_type,
>>>>>> @@ -507,6 +575,11 @@ static const struct qcom_smmu_config qcom_smmu_impl0_cfg = {
>>>>>>            .reg_offset = qcom_smmu_impl0_reg_offset,
>>>>>>     };
>>>>>>
>>>>>> +static const struct actlr_config sm8550_actlrcfg = {
>>>>>> +       .adata = sm8550_apps_actlr_data,
>>>>>> +       .size = ARRAY_SIZE(sm8550_apps_actlr_data),
>>>>>> +};
>>>>>> +
>>>>>>     /*
>>>>>>      * It is not yet possible to use MDP SMMU with the bypass quirk on the msm8996,
>>>>>>      * there are not enough context banks.
>>>>>> @@ -530,16 +603,20 @@ static const struct qcom_smmu_match_data sdm845_smmu_500_data = {
>>>>>>            /* Also no debug configuration. */
>>>>>>     };
>>>>>>
>>>>>> +
>>>>>> +static const struct qcom_smmu_match_data sm8550_smmu_500_impl0_data = {
>>>>>> +       .impl = &sm8550_smmu_500_impl,
>>>>>> +       .adreno_impl = &qcom_adreno_smmu_500_impl,
>>>>>> +       .cfg = &qcom_smmu_impl0_cfg,
>>>>>> +       .actlrcfg = &sm8550_actlrcfg,
>>>>>> +};
>>>>>
>>>>> This structure doesn't seem to be linked. Did you test your patches?
>>>>>
>>>>
>>>> Apologies Dmitry, just checked back my patches, I tested it but while
>>>> refining the patches I somehow missed this link
>>>> { .compatible = "qcom,sm8550-smmu-500", .data =
>>>> &sm8550_smmu_500_impl0_data };
>>>> in below qcom_smmu_500_impl0_data structure.
>>>> I will take care of this in next version.
>>>>
>>>>>> +
>>>>>>     static const struct qcom_smmu_match_data qcom_smmu_500_impl0_data = {
>>>>>>            .impl = &qcom_smmu_500_impl,
>>>>>>            .adreno_impl = &qcom_adreno_smmu_500_impl,
>>>>>>            .cfg = &qcom_smmu_impl0_cfg,
>>>>>>     };
>>>>>>
>>>>>> -/*
>>>>>> - * Do not add any more qcom,SOC-smmu-500 entries to this list, unless they need
>>>>>> - * special handling and can not be covered by the qcom,smmu-500 entry.
>>>>>> - */
>>>>>
>>>>> Leave the comment in place.
>>>>>
>>>>
>>>> Thanks for this comment which helped me to note the above mentioned
>>>> mistake.
>>>>
>>>>>>     static const struct of_device_id __maybe_unused qcom_smmu_impl_of_match[] = {
>>>>>>            { .compatible = "qcom,msm8996-smmu-v2", .data = &msm8996_smmu_data },
>>>>>>            { .compatible = "qcom,msm8998-smmu-v2", .data = &qcom_smmu_v2_data },
>>>>>> --
>>>>>> 2.17.1
>>>>>>
>>>>>
>>>>>
>>>
>>>
>>>
> 
> 
> 

Re: [PATCH 2/3] iommu/arm-smmu: add ACTLR data and support for SM8550

[Pratyush Brahma]

On 06-11-2023 17:39, Bibek Kumar Patro wrote:
>
>
> On 11/6/2023 2:35 PM, Dmitry Baryshkov wrote:
>> On Mon, 6 Nov 2023 at 08:10, Bibek Kumar Patro
>> <quic_bibekkum@quicinc.com> wrote:
>>>
>>>
>>>
>>> On 11/4/2023 4:11 AM, Dmitry Baryshkov wrote:
>>>> On Sat, 4 Nov 2023 at 00:38, Bibek Kumar Patro
>>>> <quic_bibekkum@quicinc.com> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 11/4/2023 3:31 AM, Dmitry Baryshkov wrote:
>>>>>> On Fri, 3 Nov 2023 at 23:53, Bibek Kumar Patro
>>>>>> <quic_bibekkum@quicinc.com> wrote:
>>>>>>>
>>>>>>> Add ACTLR data table for SM8550 along with support for
>>>>>>> same including SM8550 specific implementation operations.
>>>>>>>
>>>>>>> Signed-off-by: Bibek Kumar Patro <quic_bibekkum@quicinc.com>
>>>>>>> ---
>>>>>>>     drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 85 
>>>>>>> +++++++++++++++++++++-
>>>>>>>     1 file changed, 81 insertions(+), 4 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c 
>>>>>>> b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>>>>> index 68c1f4908473..590b7c285299 100644
>>>>>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>>>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>>>>> @@ -25,6 +25,64 @@ struct actlr_data {
>>>>>>>            u32 actlr;
>>>>>>>     };
>>>>>>>
>>>>>>> +static const struct actlr_data sm8550_apps_actlr_data[] = {
>>>>>>> +       { 0x18a0, 0x0000, 0x00000103 },
>>>>>>> +       { 0x18e0, 0x0000, 0x00000103 },
>>>>>>> +       { 0x0800, 0x0020, 0x00000001 },
>>>>>>> +       { 0x1800, 0x00c0, 0x00000001 },
>>>>>>> +       { 0x1820, 0x0000, 0x00000001 },
>>>>>>> +       { 0x1860, 0x0000, 0x00000001 },
>>>>>>> +       { 0x0c01, 0x0020, 0x00000303 },
>>>>>>> +       { 0x0c02, 0x0020, 0x00000303 },
>>>>>>> +       { 0x0c03, 0x0020, 0x00000303 },
>>>>>>> +       { 0x0c04, 0x0020, 0x00000303 },
>>>>>>> +       { 0x0c05, 0x0020, 0x00000303 },
>>>>>>> +       { 0x0c06, 0x0020, 0x00000303 },
>>>>>>> +       { 0x0c07, 0x0020, 0x00000303 },
>>>>>>> +       { 0x0c08, 0x0020, 0x00000303 },
>>>>>>> +       { 0x0c09, 0x0020, 0x00000303 },
>>>>>>> +       { 0x0c0c, 0x0020, 0x00000303 },
>>>>>>> +       { 0x0c0d, 0x0020, 0x00000303 },
>>>>>>> +       { 0x0c0e, 0x0020, 0x00000303 },
>>>>>>> +       { 0x0c0f, 0x0020, 0x00000303 },
>>>>>>> +       { 0x1961, 0x0000, 0x00000303 },
>>>>>>> +       { 0x1962, 0x0000, 0x00000303 },
>>>>>>> +       { 0x1963, 0x0000, 0x00000303 },
>>>>>>> +       { 0x1964, 0x0000, 0x00000303 },
>>>>>>> +       { 0x1965, 0x0000, 0x00000303 },
>>>>>>> +       { 0x1966, 0x0000, 0x00000303 },
>>>>>>> +       { 0x1967, 0x0000, 0x00000303 },
>>>>>>> +       { 0x1968, 0x0000, 0x00000303 },
>>>>>>> +       { 0x1969, 0x0000, 0x00000303 },
>>>>>>> +       { 0x196c, 0x0000, 0x00000303 },
>>>>>>> +       { 0x196d, 0x0000, 0x00000303 },
>>>>>>> +       { 0x196e, 0x0000, 0x00000303 },
>>>>>>> +       { 0x196f, 0x0000, 0x00000303 },
>>>>>>> +       { 0x19c1, 0x0010, 0x00000303 },
>>>>>>> +       { 0x19c2, 0x0010, 0x00000303 },
>>>>>>> +       { 0x19c3, 0x0010, 0x00000303 },
>>>>>>> +       { 0x19c4, 0x0010, 0x00000303 },
>>>>>>> +       { 0x19c5, 0x0010, 0x00000303 },
>>>>>>> +       { 0x19c6, 0x0010, 0x00000303 },
>>>>>>> +       { 0x19c7, 0x0010, 0x00000303 },
>>>>>>> +       { 0x19c8, 0x0010, 0x00000303 },
>>>>>>> +       { 0x19c9, 0x0010, 0x00000303 },
>>>>>>> +       { 0x19cc, 0x0010, 0x00000303 },
>>>>>>> +       { 0x19cd, 0x0010, 0x00000303 },
>>>>>>> +       { 0x19ce, 0x0010, 0x00000303 },
>>>>>>> +       { 0x19cf, 0x0010, 0x00000303 },
>>>>>>> +       { 0x1c00, 0x0002, 0x00000103 },
>>>>>>> +       { 0x1c01, 0x0000, 0x00000001 },
>>>>>>> +       { 0x1920, 0x0000, 0x00000103 },
>>>>>>> +       { 0x1923, 0x0000, 0x00000103 },
>>>>>>> +       { 0x1924, 0x0000, 0x00000103 },
>>>>>>> +       { 0x1940, 0x0000, 0x00000103 },
>>>>>>> +       { 0x1941, 0x0004, 0x00000103 },
>>>>>>> +       { 0x1943, 0x0000, 0x00000103 },
>>>>>>> +       { 0x1944, 0x0000, 0x00000103 },
>>>>>>> +       { 0x1947, 0x0000, 0x00000103 },
>>>>>>> +};
>>>>>>
>>>>>> This is nearly impossible to handle.
>>>>>> Please add defines for 0x1, 0x103 and 0x303. Also please consider
>>>>>> adding comments for the devices.
>>>>>>
>>>>>
>>>>> Ack, Initially added the comments for devices, but since only 
>>>>> driver is
>>>>> handling this data, and clients won't refer this so removed it. Will
>>>>> consider adding it back.
>>>>
>>>> It will help developers / porters who will try matching the SID and 
>>>> the device.
>>>>
>>>
>>> Agree on the same, I'll add those comments for devices back.
TBH, I don't see much value add in adding comments for device names as 
developers
are aware of the SIDs they use and can refer to them in the table if 
they wish to.
Secondly, developers need not care about this configuration unless they 
want to modify it which
  is not the case here as these are platform specific configurations. I 
think, only adding
the defines for the actlr reg values should suffice.
>>>>>    This actlr field value might different (other
>>>>> than 0x1,0x103,0x303) in other platforms as per my anticipation,
>>>>> depending on the bit settings, so won't the defines change with
>>>>> different platforms? Hence this register setting value might be apt?
>>>>
>>>> It is simple. 0x1, 0x103 and 0x303 are pure magic values. Please
>>>> provide sensible defines so that we can understand and review them.
>>>>
>>>
>>> Understandable, In next patch I'll populate the actlr_data in
>>> following format { SID, MASK, PRE_FETCH_n | CPRE | CMTLB }.
>>> where " PRE_FETCH_n | CPRE | CMTLB " will be defines for
>>> the actlr values (0x1,0x103,0x303).
>>> This would help in understanding these values. Hope this
>>> proposed format will be okay?
>>
>> Yes, this sounds good. Please also add description for CPRE and CMTLB 
>> bits.
>>
>
> Thanks for the confirmation. Sure will add the descriptions for
> CPRE and CMTLB bits as well in v2
>
>>>
>>>
>>>> Other platforms might use new defines.
>>>>
>>>>>
>>>>>>> +
>>>>>>>     static struct qcom_smmu *to_qcom_smmu(struct arm_smmu_device 
>>>>>>> *smmu)
>>>>>>>     {
>>>>>>>            return container_of(smmu, struct qcom_smmu, smmu);
>>>>>>> @@ -444,6 +502,16 @@ static const struct arm_smmu_impl 
>>>>>>> sdm845_smmu_500_impl = {
>>>>>>>            .tlb_sync = qcom_smmu_tlb_sync,
>>>>>>>     };
>>>>>>>
>>>>>>> +
>>>>>>> +static const struct arm_smmu_impl sm8550_smmu_500_impl = {
>>>>>>> +       .init_context = qcom_smmu_init_context,
>>>>>>> +       .cfg_probe = qcom_smmu_cfg_probe,
>>>>>>> +       .def_domain_type = qcom_smmu_def_domain_type,
>>>>>>> +       .reset = arm_mmu500_reset,
>>>>>>> +       .write_s2cr = qcom_smmu_write_s2cr,
>>>>>>> +       .tlb_sync = qcom_smmu_tlb_sync,
>>>>>>> +};
>>>>>>> +
>>>>>>>     static const struct arm_smmu_impl qcom_adreno_smmu_v2_impl = {
>>>>>>>            .init_context = qcom_adreno_smmu_init_context,
>>>>>>>            .def_domain_type = qcom_smmu_def_domain_type,
>>>>>>> @@ -507,6 +575,11 @@ static const struct qcom_smmu_config 
>>>>>>> qcom_smmu_impl0_cfg = {
>>>>>>>            .reg_offset = qcom_smmu_impl0_reg_offset,
>>>>>>>     };
>>>>>>>
>>>>>>> +static const struct actlr_config sm8550_actlrcfg = {
>>>>>>> +       .adata = sm8550_apps_actlr_data,
>>>>>>> +       .size = ARRAY_SIZE(sm8550_apps_actlr_data),
>>>>>>> +};
>>>>>>> +
>>>>>>>     /*
>>>>>>>      * It is not yet possible to use MDP SMMU with the bypass 
>>>>>>> quirk on the msm8996,
>>>>>>>      * there are not enough context banks.
>>>>>>> @@ -530,16 +603,20 @@ static const struct qcom_smmu_match_data 
>>>>>>> sdm845_smmu_500_data = {
>>>>>>>            /* Also no debug configuration. */
>>>>>>>     };
>>>>>>>
>>>>>>> +
>>>>>>> +static const struct qcom_smmu_match_data 
>>>>>>> sm8550_smmu_500_impl0_data = {
>>>>>>> +       .impl = &sm8550_smmu_500_impl,
>>>>>>> +       .adreno_impl = &qcom_adreno_smmu_500_impl,
>>>>>>> +       .cfg = &qcom_smmu_impl0_cfg,
>>>>>>> +       .actlrcfg = &sm8550_actlrcfg,
>>>>>>> +};
>>>>>>
>>>>>> This structure doesn't seem to be linked. Did you test your patches?
>>>>>>
>>>>>
>>>>> Apologies Dmitry, just checked back my patches, I tested it but while
>>>>> refining the patches I somehow missed this link
>>>>> { .compatible = "qcom,sm8550-smmu-500", .data =
>>>>> &sm8550_smmu_500_impl0_data };
>>>>> in below qcom_smmu_500_impl0_data structure.
>>>>> I will take care of this in next version.
>>>>>
>>>>>>> +
>>>>>>>     static const struct qcom_smmu_match_data 
>>>>>>> qcom_smmu_500_impl0_data = {
>>>>>>>            .impl = &qcom_smmu_500_impl,
>>>>>>>            .adreno_impl = &qcom_adreno_smmu_500_impl,
>>>>>>>            .cfg = &qcom_smmu_impl0_cfg,
>>>>>>>     };
>>>>>>>
>>>>>>> -/*
>>>>>>> - * Do not add any more qcom,SOC-smmu-500 entries to this list, 
>>>>>>> unless they need
>>>>>>> - * special handling and can not be covered by the qcom,smmu-500 
>>>>>>> entry.
>>>>>>> - */
>>>>>>
>>>>>> Leave the comment in place.
>>>>>>
>>>>>
>>>>> Thanks for this comment which helped me to note the above mentioned
>>>>> mistake.
>>>>>
>>>>>>>     static const struct of_device_id __maybe_unused 
>>>>>>> qcom_smmu_impl_of_match[] = {
>>>>>>>            { .compatible = "qcom,msm8996-smmu-v2", .data = 
>>>>>>> &msm8996_smmu_data },
>>>>>>>            { .compatible = "qcom,msm8998-smmu-v2", .data = 
>>>>>>> &qcom_smmu_v2_data },
>>>>>>> -- 
>>>>>>> 2.17.1
>>>>>>>
>>>>>>
>>>>>>
>>>>
>>>>
>>>>
>>
>>
>>
Thanks,

Pratyush

Re: [PATCH 1/3] iommu/arm-smmu: introduction of ACTLR for custom prefetcher settings

[Bibek Kumar Patro]

On 11/6/2023 11:42 AM, Bibek Kumar Patro wrote:
> 
> 
> On 11/4/2023 3:33 AM, Dmitry Baryshkov wrote:
>> On Fri, 3 Nov 2023 at 23:53, Bibek Kumar Patro
>> <quic_bibekkum@quicinc.com> wrote:
>>>
>>> Currently in Qualcomm  SoCs the default prefetch is set to 1 which 
>>> allows
>>> the TLB to fetch just the next page table. MMU-500 features ACTLR
>>> register which is implementation defined and is used for Qualcomm SoCs
>>> to have a prefetch setting of 1/3/7/15 enabling TLB to prefetch
>>> the next set of page tables accordingly allowing for faster 
>>> translations.
>>>
>>> ACTLR value is unique for each SMR (Stream matching register) and stored
>>> in a pre-populated table. This value is set to the register during
>>> context bank initialisation.
>>>
>>> Signed-off-by: Bibek Kumar Patro <quic_bibekkum@quicinc.com>
>>> ---
>>>   drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 34 ++++++++++++++++++++++
>>>   drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h |  2 ++
>>>   drivers/iommu/arm/arm-smmu/arm-smmu.c      |  5 ++--
>>>   drivers/iommu/arm/arm-smmu/arm-smmu.h      |  5 ++++
>>>   4 files changed, 44 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c 
>>> b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>> index ae7cae015193..68c1f4908473 100644
>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>> @@ -14,6 +14,17 @@
>>>
>>>   #define QCOM_DUMMY_VAL -1
>>>
>>> +struct actlr_config {
>>> +       const struct actlr_data *adata;
>>> +       u32 size;
>>
>> This should be size_t.
>>
>> Also could you please drop the separate struct actlr_config and move
>> these two fields into struct qcom_smmu_config.
>>
> 
> Ack, will address both these inputs in the next patch.
> 

Dimitry, Tried moving both fields to qcom_smmu_config but since
actlr_data need to be a pointer to array and not scalar, size needs
to be calculated dynamically for each SoC data in a loop which is 
doable.But readily available implementations like ARRAY_SIZE cannot be 
used, so I think this extra struct indirection of actlr_config would be 
beneficial.
Some drivers like llcc (drivers/soc/qcom/llcc-qcom.c) is also using
similar implementation, I believe for the same reason.

regards,
Bibek
>>> +};
>>> +
>>> +struct actlr_data {
>>> +       u16 sid;
>>> +       u16 mask;
>>> +       u32 actlr;
>>> +};
>>> +
>>>   static struct qcom_smmu *to_qcom_smmu(struct arm_smmu_device *smmu)
>>>   {
>>>          return container_of(smmu, struct qcom_smmu, smmu);
>>> @@ -270,6 +281,26 @@ static const struct of_device_id 
>>> qcom_smmu_client_of_match[] __maybe_unused = {
>>>   static int qcom_smmu_init_context(struct arm_smmu_domain *smmu_domain,
>>>                  struct io_pgtable_cfg *pgtbl_cfg, struct device *dev)
>>>   {
>>> +       struct arm_smmu_device *smmu = smmu_domain->smmu;
>>> +       struct qcom_smmu *qsmmu = to_qcom_smmu(smmu);
>>> +       const struct actlr_config *actlrcfg;
>>> +       struct arm_smmu_smr *smr = smmu->smrs;
>>> +       int idx = smmu_domain->cfg.cbndx;
>>> +       int i;
>>> +       u16 id;
>>> +       u16 mask;
>>> +
>>> +       if (qsmmu->actlrcfg) {
>>> +               actlrcfg = qsmmu->actlrcfg;
>>> +               for (i = 0; i < actlrcfg->size; ++i) {
>>> +                       id = actlrcfg->adata[i].sid;
>>> +                       mask = actlrcfg->adata[i].mask;
>>> +                       if (!smr_is_subset(*smr, id, mask))
>>> +                               arm_smmu_cb_write(smmu, idx, 
>>> ARM_SMMU_CB_ACTLR,
>>> +                                               
>>> actlrcfg->adata[i].actlr);
>>> +               }
>>> +       }
>>
>> Consider extracting this to a separate function. This way you can
>> reduce 4 indentation levels into a single loop.
>>
> 
> Ack, thanks for this sugestion. Will move this entire for loop into a 
> separate function for simplicity reduced indent levels.
> 
>>> +
>>>          smmu_domain->cfg.flush_walk_prefer_tlbiasid = true;
>>>
>>>          return 0;
>>> @@ -459,6 +490,9 @@ static struct arm_smmu_device 
>>> *qcom_smmu_create(struct arm_smmu_device *smmu,
>>>          qsmmu->smmu.impl = impl;
>>>          qsmmu->cfg = data->cfg;
>>>
>>> +       if (data->actlrcfg && (data->actlrcfg->size))
>>> +               qsmmu->actlrcfg = data->actlrcfg;
>>> +
>>>          return &qsmmu->smmu;
>>>   }
>>>
>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h 
>>> b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h
>>> index 593910567b88..4b6862715070 100644
>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h
>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h
>>> @@ -9,6 +9,7 @@
>>>   struct qcom_smmu {
>>>          struct arm_smmu_device smmu;
>>>          const struct qcom_smmu_config *cfg;
>>> +       const struct actlr_config *actlrcfg;
>>>          bool bypass_quirk;
>>>          u8 bypass_cbndx;
>>>          u32 stall_enabled;
>>> @@ -25,6 +26,7 @@ struct qcom_smmu_config {
>>>   };
>>>
>>>   struct qcom_smmu_match_data {
>>> +       const struct actlr_config *actlrcfg;
>>>          const struct qcom_smmu_config *cfg;
>>>          const struct arm_smmu_impl *impl;
>>>          const struct arm_smmu_impl *adreno_impl;
>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c 
>>> b/drivers/iommu/arm/arm-smmu/arm-smmu.c
>>> index 4c79ef6f4c75..38ac1cbc799b 100644
>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
>>> @@ -992,9 +992,10 @@ static int arm_smmu_find_sme(struct 
>>> arm_smmu_device *smmu, u16 id, u16 mask)
>>>                   * expect simply identical entries for this case, 
>>> but there's
>>>                   * no harm in accommodating the generalisation.
>>>                   */
>>> -               if ((mask & smrs[i].mask) == mask &&
>>> -                   !((id ^ smrs[i].id) & ~smrs[i].mask))
>>> +
>>> +               if (smr_is_subset(smrs[i], id, mask))
>>>                          return i;
>>> +
>>>                  /*
>>>                   * If the new entry has any other overlap with an 
>>> existing one,
>>>                   * though, then there always exists at least one 
>>> stream ID
>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.h 
>>> b/drivers/iommu/arm/arm-smmu/arm-smmu.h
>>> index 703fd5817ec1..b1638bbc41d4 100644
>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.h
>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.h
>>> @@ -501,6 +501,11 @@ static inline void arm_smmu_writeq(struct 
>>> arm_smmu_device *smmu, int page,
>>>                  writeq_relaxed(val, arm_smmu_page(smmu, page) + 
>>> offset);
>>>   }
>>>
>>> +static inline bool smr_is_subset(struct arm_smmu_smr smrs, u16 id, 
>>> u16 mask)
>>> +{
>>> +       return (mask & smrs.mask) == mask && !((id ^ smrs.id) & 
>>> ~smrs.mask);
>>> +}
>>> +
>>>   #define ARM_SMMU_GR0           0
>>>   #define ARM_SMMU_GR1           1
>>>   #define ARM_SMMU_CB(s, n)      ((s)->numpage + (n))
>>> -- 
>>> 2.17.1
>>>
>>
>>

Re: [PATCH 1/3] iommu/arm-smmu: introduction of ACTLR for custom prefetcher settings

[Dmitry Baryshkov]

On Tue, 14 Nov 2023 at 12:20, Bibek Kumar Patro
<quic_bibekkum@quicinc.com> wrote:
>
>
>
> On 11/6/2023 11:42 AM, Bibek Kumar Patro wrote:
> >
> >
> > On 11/4/2023 3:33 AM, Dmitry Baryshkov wrote:
> >> On Fri, 3 Nov 2023 at 23:53, Bibek Kumar Patro
> >> <quic_bibekkum@quicinc.com> wrote:
> >>>
> >>> Currently in Qualcomm  SoCs the default prefetch is set to 1 which
> >>> allows
> >>> the TLB to fetch just the next page table. MMU-500 features ACTLR
> >>> register which is implementation defined and is used for Qualcomm SoCs
> >>> to have a prefetch setting of 1/3/7/15 enabling TLB to prefetch
> >>> the next set of page tables accordingly allowing for faster
> >>> translations.
> >>>
> >>> ACTLR value is unique for each SMR (Stream matching register) and stored
> >>> in a pre-populated table. This value is set to the register during
> >>> context bank initialisation.
> >>>
> >>> Signed-off-by: Bibek Kumar Patro <quic_bibekkum@quicinc.com>
> >>> ---
> >>>   drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 34 ++++++++++++++++++++++
> >>>   drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h |  2 ++
> >>>   drivers/iommu/arm/arm-smmu/arm-smmu.c      |  5 ++--
> >>>   drivers/iommu/arm/arm-smmu/arm-smmu.h      |  5 ++++
> >>>   4 files changed, 44 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> >>> b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> >>> index ae7cae015193..68c1f4908473 100644
> >>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> >>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> >>> @@ -14,6 +14,17 @@
> >>>
> >>>   #define QCOM_DUMMY_VAL -1
> >>>
> >>> +struct actlr_config {
> >>> +       const struct actlr_data *adata;
> >>> +       u32 size;
> >>
> >> This should be size_t.
> >>
> >> Also could you please drop the separate struct actlr_config and move
> >> these two fields into struct qcom_smmu_config.
> >>
> >
> > Ack, will address both these inputs in the next patch.
> >
>
> Dimitry, Tried moving both fields to qcom_smmu_config but since
> actlr_data need to be a pointer to array and not scalar, size needs
> to be calculated dynamically for each SoC data in a loop which is
> doable.But readily available implementations like ARRAY_SIZE cannot be
> used, so I think this extra struct indirection of actlr_config would be
> beneficial.

This should work fine from my point of view:

static const struct qcom_smmu_match_data sm8550_smmu_500_impl0_data = {
 .impl = &sm8550_smmu_500_impl,
 .adreno_impl = &qcom_adreno_smmu_500_impl,
 .cfg = &qcom_smmu_impl0_cfg,
 .actlrcfg = &sm8550_apps_actlr_data,
 .actlrcfg_size = ARRAY_SIZE(sm8550_apps_actlr_data),
};



> Some drivers like llcc (drivers/soc/qcom/llcc-qcom.c) is also using
> similar implementation, I believe for the same reason.
>
> regards,
> Bibek
> >>> +};
> >>> +
> >>> +struct actlr_data {
> >>> +       u16 sid;
> >>> +       u16 mask;
> >>> +       u32 actlr;
> >>> +};
> >>> +
> >>>   static struct qcom_smmu *to_qcom_smmu(struct arm_smmu_device *smmu)
> >>>   {
> >>>          return container_of(smmu, struct qcom_smmu, smmu);
> >>> @@ -270,6 +281,26 @@ static const struct of_device_id
> >>> qcom_smmu_client_of_match[] __maybe_unused = {
> >>>   static int qcom_smmu_init_context(struct arm_smmu_domain *smmu_domain,
> >>>                  struct io_pgtable_cfg *pgtbl_cfg, struct device *dev)
> >>>   {
> >>> +       struct arm_smmu_device *smmu = smmu_domain->smmu;
> >>> +       struct qcom_smmu *qsmmu = to_qcom_smmu(smmu);
> >>> +       const struct actlr_config *actlrcfg;
> >>> +       struct arm_smmu_smr *smr = smmu->smrs;
> >>> +       int idx = smmu_domain->cfg.cbndx;
> >>> +       int i;
> >>> +       u16 id;
> >>> +       u16 mask;
> >>> +
> >>> +       if (qsmmu->actlrcfg) {
> >>> +               actlrcfg = qsmmu->actlrcfg;
> >>> +               for (i = 0; i < actlrcfg->size; ++i) {
> >>> +                       id = actlrcfg->adata[i].sid;
> >>> +                       mask = actlrcfg->adata[i].mask;
> >>> +                       if (!smr_is_subset(*smr, id, mask))
> >>> +                               arm_smmu_cb_write(smmu, idx,
> >>> ARM_SMMU_CB_ACTLR,
> >>> +
> >>> actlrcfg->adata[i].actlr);
> >>> +               }
> >>> +       }
> >>
> >> Consider extracting this to a separate function. This way you can
> >> reduce 4 indentation levels into a single loop.
> >>
> >
> > Ack, thanks for this sugestion. Will move this entire for loop into a
> > separate function for simplicity reduced indent levels.
> >
> >>> +
> >>>          smmu_domain->cfg.flush_walk_prefer_tlbiasid = true;
> >>>
> >>>          return 0;
> >>> @@ -459,6 +490,9 @@ static struct arm_smmu_device
> >>> *qcom_smmu_create(struct arm_smmu_device *smmu,
> >>>          qsmmu->smmu.impl = impl;
> >>>          qsmmu->cfg = data->cfg;
> >>>
> >>> +       if (data->actlrcfg && (data->actlrcfg->size))
> >>> +               qsmmu->actlrcfg = data->actlrcfg;
> >>> +
> >>>          return &qsmmu->smmu;
> >>>   }
> >>>
> >>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h
> >>> b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h
> >>> index 593910567b88..4b6862715070 100644
> >>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h
> >>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h
> >>> @@ -9,6 +9,7 @@
> >>>   struct qcom_smmu {
> >>>          struct arm_smmu_device smmu;
> >>>          const struct qcom_smmu_config *cfg;
> >>> +       const struct actlr_config *actlrcfg;
> >>>          bool bypass_quirk;
> >>>          u8 bypass_cbndx;
> >>>          u32 stall_enabled;
> >>> @@ -25,6 +26,7 @@ struct qcom_smmu_config {
> >>>   };
> >>>
> >>>   struct qcom_smmu_match_data {
> >>> +       const struct actlr_config *actlrcfg;
> >>>          const struct qcom_smmu_config *cfg;
> >>>          const struct arm_smmu_impl *impl;
> >>>          const struct arm_smmu_impl *adreno_impl;
> >>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> >>> b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> >>> index 4c79ef6f4c75..38ac1cbc799b 100644
> >>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> >>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> >>> @@ -992,9 +992,10 @@ static int arm_smmu_find_sme(struct
> >>> arm_smmu_device *smmu, u16 id, u16 mask)
> >>>                   * expect simply identical entries for this case,
> >>> but there's
> >>>                   * no harm in accommodating the generalisation.
> >>>                   */
> >>> -               if ((mask & smrs[i].mask) == mask &&
> >>> -                   !((id ^ smrs[i].id) & ~smrs[i].mask))
> >>> +
> >>> +               if (smr_is_subset(smrs[i], id, mask))
> >>>                          return i;
> >>> +
> >>>                  /*
> >>>                   * If the new entry has any other overlap with an
> >>> existing one,
> >>>                   * though, then there always exists at least one
> >>> stream ID
> >>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.h
> >>> b/drivers/iommu/arm/arm-smmu/arm-smmu.h
> >>> index 703fd5817ec1..b1638bbc41d4 100644
> >>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.h
> >>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.h
> >>> @@ -501,6 +501,11 @@ static inline void arm_smmu_writeq(struct
> >>> arm_smmu_device *smmu, int page,
> >>>                  writeq_relaxed(val, arm_smmu_page(smmu, page) +
> >>> offset);
> >>>   }
> >>>
> >>> +static inline bool smr_is_subset(struct arm_smmu_smr smrs, u16 id,
> >>> u16 mask)
> >>> +{
> >>> +       return (mask & smrs.mask) == mask && !((id ^ smrs.id) &
> >>> ~smrs.mask);
> >>> +}
> >>> +
> >>>   #define ARM_SMMU_GR0           0
> >>>   #define ARM_SMMU_GR1           1
> >>>   #define ARM_SMMU_CB(s, n)      ((s)->numpage + (n))
> >>> --
> >>> 2.17.1
> >>>
> >>
> >>



-- 
With best wishes
Dmitry