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

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

[Bibek Kumar Patro]

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

1. Remove cfg inside qcom_smmu structure and replace it with single
   pointer to qcom_smmu_match_data avoiding replication of multiple
   members from same.

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

3. Add ACTLR data and implementation operations for SM8550.

4. Add ACTLR data and implementation operations for SC7280.

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

Changes in v4 from v3:
 New addition:
 - Remove actlrcfg_size and use NULL end element instead to traverse
   the actlr table, as this would be a cleaner approach by removing
   redundancy of actlrcfg_size.
 - Renaming of actlr set function to arm_smmu_qcom based proprietary
   convention.
 - break from loop once sid is found and ACTLR value is initialized
   in qcom_smmu_set_actlr.
 - Modify the GFX prefetch value separating into 2 sensible defines.
 - Modify comments for prefetch defines as per SMMU-500 TRM.
 Changes to incorporate suggestions from Konrad as follows:
 - Use Reverse-Christmas-tree sorting wherever applicable.
 - Pass arguments directly to arm_smmu_set_actlr instead of creating
   duplicate variables.
 - Use array indexing instead of direct pointer addressed by new
   addition of eliminating actlrcfg_size.
 - Switch the HEX value's case from upper to lower case in SC7280
   actlrcfg table.
 Changes to incorporate suggestions from Dmitry as follows:
 - Separate changes not related to ACTLR support to different commit
   with patch 5/5.
 - Using pointer to struct for arguments in smr_is_subset().
 Changes to incorporate suggestions from Bjorn as follows:
 - fix the commit message for patch 2/5 to properly document the
   value space to avoid confusion.
 Fixed build issues reported by kernel test robot [1] for
 arm64-allyesconfig [2].
 [1]: https://lore.kernel.org/all/202312011750.Pwca3TWE-lkp@intel.com/
 [2]: https://download.01.org/0day-ci/archive/20231201/202312011750.Pwca3TWE-lkp@intel.com/config
 Link to v3:
https://lore.kernel.org/all/20231127145412.3981-1-quic_bibekkum@quicinc.com/

Changes in v3 from v2:
 New addition:
 - Include patch 3/4 for adding ACTLR support and data for SC7280.
 - Add driver changes for actlr support in gpu smmu.
 - Add target wise actlr data and implementation ops for gpu smmu.
 Changes to incorporate suggestions from Robin as follows:
 - Match the ACTLR values with individual corresponding SID instead
   of assuming that any SMR will be programmed to match a superset of
   the data.
 - Instead of replicating each elements from qcom_smmu_match_data to
   qcom_smmu structre during smmu device creation, replace the
   replicated members with qcom_smmu_match_data structure inside
   qcom_smmu structre and handle the dereference in places that
   requires them.
 Changes to incorporate suggestions from Dmitry and Konrad as follows:
 - Maintain actlr table inside a single structure instead of
   nested structure.
 - Rename prefetch defines to more appropriately describe their behavior.
 - Remove SM8550 specific implementation ops and roll back to default
   qcom_smmu_500_impl implementation ops.
 - Add back the removed comments which are NAK.
 - Fix commit description for patch 4/4.
 Link to v2:
https://lore.kernel.org/all/20231114135654.30475-1-quic_bibekkum@quicinc.com/

Changes in v2 from v1:
 - Incorporated suggestions on v1 from Dmitry,Konrad,Pratyush.
 - Added defines for ACTLR values.
 - Linked sm8550 implementation structure to corresponding
   compatible string.
 - Repackaged actlr value set implementation to separate function.
 - Fixed indentation errors.
 - Link to v1:
https://lore.kernel.org/all/20231103215124.1095-1-quic_bibekkum@quicinc.com/

Changes in v1 from RFC:
 - Incorporated suggestion form Robin on RFC
 - Moved the actlr data table into driver, instead of maintaining
   it inside soc specific DT and piggybacking on exisiting iommus
   property (iommu = <SID, MASK, ACTLR>) to set this value during
   smmu probe.
 - Link to RFC:
https://lore.kernel.org/all/a01e7e60-6ead-4a9e-ba90-22a8a6bbd03f@quicinc.com/
Bibek Kumar Patro (5):
  iommu/arm-smmu: refactor qcom_smmu structure to include single pointer
  iommu/arm-smmu: introduction of ACTLR for custom prefetcher settings
  iommu/arm-smmu: add ACTLR data and support for SM8550
  iommu/arm-smmu: add ACTLR data and support for SC7280
  iommu/arm-smmu: re-enable context caching in smmu reset operation

 .../iommu/arm/arm-smmu/arm-smmu-qcom-debug.c  |   2 +-
 drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c    | 184 +++++++++++++++++-
 drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h    |   6 +-
 drivers/iommu/arm/arm-smmu/arm-smmu.c         |   5 +-
 drivers/iommu/arm/arm-smmu/arm-smmu.h         |   5 +
 5 files changed, 193 insertions(+), 9 deletions(-)

--
2.17.1

[PATCH v4 1/5] iommu/arm-smmu: refactor qcom_smmu structure to include single pointer

[Bibek Kumar Patro]

qcom_smmu_match_data is static and constant so refactor qcom_smmu
to store single pointer to qcom_smmu_match_data instead of
replicating multiple child members of the same and handle the further
dereferences in the places that want them.

Suggested-by: Robin Murphy <robin.murphy@arm.com>
Signed-off-by: Bibek Kumar Patro <quic_bibekkum@quicinc.com>
---
 drivers/iommu/arm/arm-smmu/arm-smmu-qcom-debug.c | 2 +-
 drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c       | 2 +-
 drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h       | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom-debug.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom-debug.c
index bb89d49adf8d..e9798b133cbb 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom-debug.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom-debug.c
@@ -22,7 +22,7 @@ void qcom_smmu_tlb_sync_debug(struct arm_smmu_device *smmu)
 	if (__ratelimit(&rs)) {
 		dev_err(smmu->dev, "TLB sync timed out -- SMMU may be deadlocked\n");

-		cfg = qsmmu->cfg;
+		cfg = qsmmu->data->cfg;
 		if (!cfg)
 			return;

diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
index 549ae4dba3a6..8a7c16a86694 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
@@ -465,7 +465,7 @@ static struct arm_smmu_device *qcom_smmu_create(struct arm_smmu_device *smmu,
 		return ERR_PTR(-ENOMEM);

 	qsmmu->smmu.impl = impl;
-	qsmmu->cfg = data->cfg;
+	qsmmu->data = data;

 	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..f3b91963e234 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h
@@ -8,7 +8,7 @@

 struct qcom_smmu {
 	struct arm_smmu_device smmu;
-	const struct qcom_smmu_config *cfg;
+	const struct qcom_smmu_match_data *data;
 	bool bypass_quirk;
 	u8 bypass_cbndx;
 	u32 stall_enabled;
--
2.17.1

[PATCH v4 2/5] 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 custom prefetch setting 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 | 45 ++++++++++++++++++++++
 drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h |  4 ++
 drivers/iommu/arm/arm-smmu/arm-smmu.c      |  5 ++-
 drivers/iommu/arm/arm-smmu/arm-smmu.h      |  5 +++
 4 files changed, 57 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 8a7c16a86694..cb49291f5233 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
@@ -14,6 +14,12 @@

 #define QCOM_DUMMY_VAL	-1

+struct actlr_config {
+	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);
@@ -205,9 +211,38 @@ static bool qcom_adreno_can_do_ttbr1(struct arm_smmu_device *smmu)
 	return true;
 }

+static void qcom_smmu_set_actlr(struct device *dev, struct arm_smmu_device *smmu, int cbndx,
+		const struct actlr_config *actlrcfg)
+{
+	struct arm_smmu_master_cfg *cfg = dev_iommu_priv_get(dev);
+	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
+	struct arm_smmu_smr *smr;
+	u16 mask;
+	int idx;
+	u16 id;
+	int i;
+
+	for (; actlrcfg->sid || actlrcfg->mask || actlrcfg->actlr; actlrcfg++) {
+		id = actlrcfg->sid;
+		mask = actlrcfg->mask;
+
+		for_each_cfg_sme(cfg, fwspec, i, idx) {
+			smr = &smmu->smrs[idx];
+			if (smr_is_subset(smr, id, mask)) {
+				arm_smmu_cb_write(smmu, cbndx, ARM_SMMU_CB_ACTLR,
+						actlrcfg->actlr);
+				break;
+			}
+		}
+	}
+}
+
 static int qcom_adreno_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);
+	int cbndx = smmu_domain->cfg.cbndx;
 	struct adreno_smmu_priv *priv;

 	smmu_domain->cfg.flush_walk_prefer_tlbiasid = true;
@@ -238,6 +273,9 @@ static int qcom_adreno_smmu_init_context(struct arm_smmu_domain *smmu_domain,
 	priv->set_stall = qcom_adreno_smmu_set_stall;
 	priv->resume_translation = qcom_adreno_smmu_resume_translation;

+	if (qsmmu->data->actlrcfg_gfx)
+		qcom_smmu_set_actlr(dev, smmu, cbndx, qsmmu->data->actlrcfg_gfx);
+
 	return 0;
 }

@@ -264,6 +302,13 @@ 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);
+	int cbndx = smmu_domain->cfg.cbndx;
+
+	if (qsmmu->data->actlrcfg)
+		qcom_smmu_set_actlr(dev, smmu, cbndx, qsmmu->data->actlrcfg);
+
 	smmu_domain->cfg.flush_walk_prefer_tlbiasid = true;

 	return 0;
diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h
index f3b91963e234..448a66b5332f 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h
@@ -24,7 +24,11 @@ struct qcom_smmu_config {
 	const u32 *reg_offset;
 };

+struct actlr_config;
+
 struct qcom_smmu_match_data {
+	const struct actlr_config *actlrcfg;
+	const struct actlr_config *actlrcfg_gfx;
 	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 d6d1a2a55cc0..0c7f700b27dd 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
@@ -990,9 +990,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..2e4f65412c6b 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 v4 3/5] 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 | 89 ++++++++++++++++++++++
 1 file changed, 89 insertions(+)

diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
index cb49291f5233..d2006f610243 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
@@ -20,6 +20,85 @@ struct actlr_config {
 	u32 actlr;
 };

+/*
+ * SMMU-500 TRM defines BIT(0) as CMTLB (Enable context caching in the
+ * macro TLB) and BIT(1) as CPRE (Enable context caching in the prefetch
+ * buffer). The remaining bits are implementation defined and vary across
+ * SoCs.
+ */
+
+#define PREFETCH_DEFAULT	0
+#define PREFETCH_SHALLOW	BIT(8)
+#define PREFETCH_MODERATE	BIT(9)
+#define PREFETCH_DEEP		(BIT(9) | BIT(8))
+#define PREFETCH_SWITCH_GFX	(BIT(5) | BIT(3))
+#define CPRE			BIT(1)
+#define CMTLB			BIT(0)
+
+static const struct actlr_config sm8550_apps_actlr_cfg[] = {
+	{ 0x18a0, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB },
+	{ 0x18e0, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB },
+	{ 0x0800, 0x0020, PREFETCH_DEFAULT | CMTLB },
+	{ 0x1800, 0x00c0, PREFETCH_DEFAULT | CMTLB },
+	{ 0x1820, 0x0000, PREFETCH_DEFAULT | CMTLB },
+	{ 0x1860, 0x0000, PREFETCH_DEFAULT | CMTLB },
+	{ 0x0c01, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
+	{ 0x0c02, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
+	{ 0x0c03, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
+	{ 0x0c04, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
+	{ 0x0c05, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
+	{ 0x0c06, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
+	{ 0x0c07, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
+	{ 0x0c08, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
+	{ 0x0c09, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
+	{ 0x0c0c, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
+	{ 0x0c0d, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
+	{ 0x0c0e, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
+	{ 0x0c0f, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
+	{ 0x1961, 0x0000, PREFETCH_DEEP | CPRE | CMTLB },
+	{ 0x1962, 0x0000, PREFETCH_DEEP | CPRE | CMTLB },
+	{ 0x1963, 0x0000, PREFETCH_DEEP | CPRE | CMTLB },
+	{ 0x1964, 0x0000, PREFETCH_DEEP | CPRE | CMTLB },
+	{ 0x1965, 0x0000, PREFETCH_DEEP | CPRE | CMTLB },
+	{ 0x1966, 0x0000, PREFETCH_DEEP | CPRE | CMTLB },
+	{ 0x1967, 0x0000, PREFETCH_DEEP | CPRE | CMTLB },
+	{ 0x1968, 0x0000, PREFETCH_DEEP | CPRE | CMTLB },
+	{ 0x1969, 0x0000, PREFETCH_DEEP | CPRE | CMTLB },
+	{ 0x196c, 0x0000, PREFETCH_DEEP | CPRE | CMTLB },
+	{ 0x196d, 0x0000, PREFETCH_DEEP | CPRE | CMTLB },
+	{ 0x196e, 0x0000, PREFETCH_DEEP | CPRE | CMTLB },
+	{ 0x196f, 0x0000, PREFETCH_DEEP | CPRE | CMTLB },
+	{ 0x19c1, 0x0010, PREFETCH_DEEP | CPRE | CMTLB },
+	{ 0x19c2, 0x0010, PREFETCH_DEEP | CPRE | CMTLB },
+	{ 0x19c3, 0x0010, PREFETCH_DEEP | CPRE | CMTLB },
+	{ 0x19c4, 0x0010, PREFETCH_DEEP | CPRE | CMTLB },
+	{ 0x19c5, 0x0010, PREFETCH_DEEP | CPRE | CMTLB },
+	{ 0x19c6, 0x0010, PREFETCH_DEEP | CPRE | CMTLB },
+	{ 0x19c7, 0x0010, PREFETCH_DEEP | CPRE | CMTLB },
+	{ 0x19c8, 0x0010, PREFETCH_DEEP | CPRE | CMTLB },
+	{ 0x19c9, 0x0010, PREFETCH_DEEP | CPRE | CMTLB },
+	{ 0x19cc, 0x0010, PREFETCH_DEEP | CPRE | CMTLB },
+	{ 0x19cd, 0x0010, PREFETCH_DEEP | CPRE | CMTLB },
+	{ 0x19ce, 0x0010, PREFETCH_DEEP | CPRE | CMTLB },
+	{ 0x19cf, 0x0010, PREFETCH_DEEP | CPRE | CMTLB },
+	{ 0x1c00, 0x0002, PREFETCH_SHALLOW | CPRE | CMTLB },
+	{ 0x1c01, 0x0000, PREFETCH_DEFAULT | CMTLB },
+	{ 0x1920, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB },
+	{ 0x1923, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB },
+	{ 0x1924, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB },
+	{ 0x1940, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB },
+	{ 0x1941, 0x0004, PREFETCH_SHALLOW | CPRE | CMTLB },
+	{ 0x1943, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB },
+	{ 0x1944, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB },
+	{ 0x1947, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB },
+	{},
+};
+
+static const struct actlr_config sm8550_gfx_actlr_cfg[] = {
+	{ 0x0000, 0x03ff, PREFETCH_SWITCH_GFX | PREFETCH_DEEP | CPRE | CMTLB },
+	{},
+};
+
 static struct qcom_smmu *to_qcom_smmu(struct arm_smmu_device *smmu)
 {
 	return container_of(smmu, struct qcom_smmu, smmu);
@@ -549,6 +628,15 @@ 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 = &qcom_smmu_500_impl,
+	.adreno_impl = &qcom_adreno_smmu_500_impl,
+	.cfg = &qcom_smmu_impl0_cfg,
+	.actlrcfg = sm8550_apps_actlr_cfg,
+	.actlrcfg_gfx = sm8550_gfx_actlr_cfg,
+};
+
 static const struct qcom_smmu_match_data qcom_smmu_500_impl0_data = {
 	.impl = &qcom_smmu_500_impl,
 	.adreno_impl = &qcom_adreno_smmu_500_impl,
@@ -583,6 +671,7 @@ static const struct of_device_id __maybe_unused qcom_smmu_impl_of_match[] = {
 	{ .compatible = "qcom,sm8250-smmu-500", .data = &qcom_smmu_500_impl0_data },
 	{ .compatible = "qcom,sm8350-smmu-500", .data = &qcom_smmu_500_impl0_data },
 	{ .compatible = "qcom,sm8450-smmu-500", .data = &qcom_smmu_500_impl0_data },
+	{ .compatible = "qcom,sm8550-smmu-500", .data = &sm8550_smmu_500_impl0_data },
 	{ .compatible = "qcom,smmu-500", .data = &qcom_smmu_500_impl0_data },
 	{ }
 };
--
2.17.1

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

[Dmitry Baryshkov]

On Fri, 15 Dec 2023 at 12:19, 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 | 89 ++++++++++++++++++++++
>  1 file changed, 89 insertions(+)
>
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> index cb49291f5233..d2006f610243 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> @@ -20,6 +20,85 @@ struct actlr_config {
>         u32 actlr;
>  };
>
> +/*
> + * SMMU-500 TRM defines BIT(0) as CMTLB (Enable context caching in the
> + * macro TLB) and BIT(1) as CPRE (Enable context caching in the prefetch
> + * buffer). The remaining bits are implementation defined and vary across
> + * SoCs.
> + */
> +
> +#define PREFETCH_DEFAULT       0
> +#define PREFETCH_SHALLOW       BIT(8)
> +#define PREFETCH_MODERATE      BIT(9)
> +#define PREFETCH_DEEP          (BIT(9) | BIT(8))

I thin the following might be more correct:

#include <linux/bitfield.h>

#define PREFETCH_MASK GENMASK(9, 8)
#define PREFETCH_DEFAULT FIELD_PREP(PREFETCH_MASK, 0)
#define PREFETCH_SHALLOW FIELD_PREP(PREFETCH_MASK, 1)
#define PREFETCH_MODERATE FIELD_PREP(PREFETCH_MASK, 2)
#define PREFETCH_DEEP FIELD_PREP(PREFETCH_MASK, 3)

> +#define PREFETCH_SWITCH_GFX    (BIT(5) | BIT(3))
> +#define CPRE                   BIT(1)
> +#define CMTLB                  BIT(0)
> +
> +static const struct actlr_config sm8550_apps_actlr_cfg[] = {
> +       { 0x18a0, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB },
> +       { 0x18e0, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB },
> +       { 0x0800, 0x0020, PREFETCH_DEFAULT | CMTLB },
> +       { 0x1800, 0x00c0, PREFETCH_DEFAULT | CMTLB },
> +       { 0x1820, 0x0000, PREFETCH_DEFAULT | CMTLB },
> +       { 0x1860, 0x0000, PREFETCH_DEFAULT | CMTLB },
> +       { 0x0c01, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
> +       { 0x0c02, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
> +       { 0x0c03, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
> +       { 0x0c04, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
> +       { 0x0c05, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
> +       { 0x0c06, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
> +       { 0x0c07, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
> +       { 0x0c08, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
> +       { 0x0c09, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
> +       { 0x0c0c, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
> +       { 0x0c0d, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
> +       { 0x0c0e, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
> +       { 0x0c0f, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
> +       { 0x1961, 0x0000, PREFETCH_DEEP | CPRE | CMTLB },
> +       { 0x1962, 0x0000, PREFETCH_DEEP | CPRE | CMTLB },
> +       { 0x1963, 0x0000, PREFETCH_DEEP | CPRE | CMTLB },
> +       { 0x1964, 0x0000, PREFETCH_DEEP | CPRE | CMTLB },
> +       { 0x1965, 0x0000, PREFETCH_DEEP | CPRE | CMTLB },
> +       { 0x1966, 0x0000, PREFETCH_DEEP | CPRE | CMTLB },
> +       { 0x1967, 0x0000, PREFETCH_DEEP | CPRE | CMTLB },
> +       { 0x1968, 0x0000, PREFETCH_DEEP | CPRE | CMTLB },
> +       { 0x1969, 0x0000, PREFETCH_DEEP | CPRE | CMTLB },
> +       { 0x196c, 0x0000, PREFETCH_DEEP | CPRE | CMTLB },
> +       { 0x196d, 0x0000, PREFETCH_DEEP | CPRE | CMTLB },
> +       { 0x196e, 0x0000, PREFETCH_DEEP | CPRE | CMTLB },
> +       { 0x196f, 0x0000, PREFETCH_DEEP | CPRE | CMTLB },
> +       { 0x19c1, 0x0010, PREFETCH_DEEP | CPRE | CMTLB },
> +       { 0x19c2, 0x0010, PREFETCH_DEEP | CPRE | CMTLB },
> +       { 0x19c3, 0x0010, PREFETCH_DEEP | CPRE | CMTLB },
> +       { 0x19c4, 0x0010, PREFETCH_DEEP | CPRE | CMTLB },
> +       { 0x19c5, 0x0010, PREFETCH_DEEP | CPRE | CMTLB },
> +       { 0x19c6, 0x0010, PREFETCH_DEEP | CPRE | CMTLB },
> +       { 0x19c7, 0x0010, PREFETCH_DEEP | CPRE | CMTLB },
> +       { 0x19c8, 0x0010, PREFETCH_DEEP | CPRE | CMTLB },
> +       { 0x19c9, 0x0010, PREFETCH_DEEP | CPRE | CMTLB },
> +       { 0x19cc, 0x0010, PREFETCH_DEEP | CPRE | CMTLB },
> +       { 0x19cd, 0x0010, PREFETCH_DEEP | CPRE | CMTLB },
> +       { 0x19ce, 0x0010, PREFETCH_DEEP | CPRE | CMTLB },
> +       { 0x19cf, 0x0010, PREFETCH_DEEP | CPRE | CMTLB },
> +       { 0x1c00, 0x0002, PREFETCH_SHALLOW | CPRE | CMTLB },
> +       { 0x1c01, 0x0000, PREFETCH_DEFAULT | CMTLB },
> +       { 0x1920, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB },
> +       { 0x1923, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB },
> +       { 0x1924, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB },
> +       { 0x1940, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB },
> +       { 0x1941, 0x0004, PREFETCH_SHALLOW | CPRE | CMTLB },
> +       { 0x1943, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB },
> +       { 0x1944, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB },
> +       { 0x1947, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB },
> +       {},
> +};
> +
> +static const struct actlr_config sm8550_gfx_actlr_cfg[] = {
> +       { 0x0000, 0x03ff, PREFETCH_SWITCH_GFX | PREFETCH_DEEP | CPRE | CMTLB },
> +       {},
> +};
> +
>  static struct qcom_smmu *to_qcom_smmu(struct arm_smmu_device *smmu)
>  {
>         return container_of(smmu, struct qcom_smmu, smmu);
> @@ -549,6 +628,15 @@ 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 = &qcom_smmu_500_impl,
> +       .adreno_impl = &qcom_adreno_smmu_500_impl,
> +       .cfg = &qcom_smmu_impl0_cfg,
> +       .actlrcfg = sm8550_apps_actlr_cfg,
> +       .actlrcfg_gfx = sm8550_gfx_actlr_cfg,
> +};
> +
>  static const struct qcom_smmu_match_data qcom_smmu_500_impl0_data = {
>         .impl = &qcom_smmu_500_impl,
>         .adreno_impl = &qcom_adreno_smmu_500_impl,
> @@ -583,6 +671,7 @@ static const struct of_device_id __maybe_unused qcom_smmu_impl_of_match[] = {
>         { .compatible = "qcom,sm8250-smmu-500", .data = &qcom_smmu_500_impl0_data },
>         { .compatible = "qcom,sm8350-smmu-500", .data = &qcom_smmu_500_impl0_data },
>         { .compatible = "qcom,sm8450-smmu-500", .data = &qcom_smmu_500_impl0_data },
> +       { .compatible = "qcom,sm8550-smmu-500", .data = &sm8550_smmu_500_impl0_data },
>         { .compatible = "qcom,smmu-500", .data = &qcom_smmu_500_impl0_data },
>         { }
>  };
> --
> 2.17.1
>


-- 
With best wishes
Dmitry

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

[Bibek Kumar Patro]

On 12/15/2023 4:14 PM, Dmitry Baryshkov wrote:
> On Fri, 15 Dec 2023 at 12:19, 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 | 89 ++++++++++++++++++++++
>>   1 file changed, 89 insertions(+)
>>
>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>> index cb49291f5233..d2006f610243 100644
>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>> @@ -20,6 +20,85 @@ struct actlr_config {
>>          u32 actlr;
>>   };
>>
>> +/*
>> + * SMMU-500 TRM defines BIT(0) as CMTLB (Enable context caching in the
>> + * macro TLB) and BIT(1) as CPRE (Enable context caching in the prefetch
>> + * buffer). The remaining bits are implementation defined and vary across
>> + * SoCs.
>> + */
>> +
>> +#define PREFETCH_DEFAULT       0
>> +#define PREFETCH_SHALLOW       BIT(8)
>> +#define PREFETCH_MODERATE      BIT(9)
>> +#define PREFETCH_DEEP          (BIT(9) | BIT(8))
> 
> I thin the following might be more correct:
> 
> #include <linux/bitfield.h>
> 
> #define PREFETCH_MASK GENMASK(9, 8)
> #define PREFETCH_DEFAULT FIELD_PREP(PREFETCH_MASK, 0)
> #define PREFETCH_SHALLOW FIELD_PREP(PREFETCH_MASK, 1)
> #define PREFETCH_MODERATE FIELD_PREP(PREFETCH_MASK, 2)
> #define PREFETCH_DEEP FIELD_PREP(PREFETCH_MASK, 3)
> 

Ack, thanks for this suggestion. Let me try this out using
GENMASK. Once tested, will take care of this in next version.

Thanks,
Bibek

>> +#define PREFETCH_SWITCH_GFX    (BIT(5) | BIT(3))
>> +#define CPRE                   BIT(1)
>> +#define CMTLB                  BIT(0)
>> +
>> +static const struct actlr_config sm8550_apps_actlr_cfg[] = {
>> +       { 0x18a0, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB },
>> +       { 0x18e0, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB },
>> +       { 0x0800, 0x0020, PREFETCH_DEFAULT | CMTLB },
>> +       { 0x1800, 0x00c0, PREFETCH_DEFAULT | CMTLB },
>> +       { 0x1820, 0x0000, PREFETCH_DEFAULT | CMTLB },
>> +       { 0x1860, 0x0000, PREFETCH_DEFAULT | CMTLB },
>> +       { 0x0c01, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
>> +       { 0x0c02, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
>> +       { 0x0c03, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
>> +       { 0x0c04, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
>> +       { 0x0c05, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
>> +       { 0x0c06, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
>> +       { 0x0c07, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
>> +       { 0x0c08, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
>> +       { 0x0c09, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
>> +       { 0x0c0c, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
>> +       { 0x0c0d, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
>> +       { 0x0c0e, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
>> +       { 0x0c0f, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
>> +       { 0x1961, 0x0000, PREFETCH_DEEP | CPRE | CMTLB },
>> +       { 0x1962, 0x0000, PREFETCH_DEEP | CPRE | CMTLB },
>> +       { 0x1963, 0x0000, PREFETCH_DEEP | CPRE | CMTLB },
>> +       { 0x1964, 0x0000, PREFETCH_DEEP | CPRE | CMTLB },
>> +       { 0x1965, 0x0000, PREFETCH_DEEP | CPRE | CMTLB },
>> +       { 0x1966, 0x0000, PREFETCH_DEEP | CPRE | CMTLB },
>> +       { 0x1967, 0x0000, PREFETCH_DEEP | CPRE | CMTLB },
>> +       { 0x1968, 0x0000, PREFETCH_DEEP | CPRE | CMTLB },
>> +       { 0x1969, 0x0000, PREFETCH_DEEP | CPRE | CMTLB },
>> +       { 0x196c, 0x0000, PREFETCH_DEEP | CPRE | CMTLB },
>> +       { 0x196d, 0x0000, PREFETCH_DEEP | CPRE | CMTLB },
>> +       { 0x196e, 0x0000, PREFETCH_DEEP | CPRE | CMTLB },
>> +       { 0x196f, 0x0000, PREFETCH_DEEP | CPRE | CMTLB },
>> +       { 0x19c1, 0x0010, PREFETCH_DEEP | CPRE | CMTLB },
>> +       { 0x19c2, 0x0010, PREFETCH_DEEP | CPRE | CMTLB },
>> +       { 0x19c3, 0x0010, PREFETCH_DEEP | CPRE | CMTLB },
>> +       { 0x19c4, 0x0010, PREFETCH_DEEP | CPRE | CMTLB },
>> +       { 0x19c5, 0x0010, PREFETCH_DEEP | CPRE | CMTLB },
>> +       { 0x19c6, 0x0010, PREFETCH_DEEP | CPRE | CMTLB },
>> +       { 0x19c7, 0x0010, PREFETCH_DEEP | CPRE | CMTLB },
>> +       { 0x19c8, 0x0010, PREFETCH_DEEP | CPRE | CMTLB },
>> +       { 0x19c9, 0x0010, PREFETCH_DEEP | CPRE | CMTLB },
>> +       { 0x19cc, 0x0010, PREFETCH_DEEP | CPRE | CMTLB },
>> +       { 0x19cd, 0x0010, PREFETCH_DEEP | CPRE | CMTLB },
>> +       { 0x19ce, 0x0010, PREFETCH_DEEP | CPRE | CMTLB },
>> +       { 0x19cf, 0x0010, PREFETCH_DEEP | CPRE | CMTLB },
>> +       { 0x1c00, 0x0002, PREFETCH_SHALLOW | CPRE | CMTLB },
>> +       { 0x1c01, 0x0000, PREFETCH_DEFAULT | CMTLB },
>> +       { 0x1920, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB },
>> +       { 0x1923, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB },
>> +       { 0x1924, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB },
>> +       { 0x1940, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB },
>> +       { 0x1941, 0x0004, PREFETCH_SHALLOW | CPRE | CMTLB },
>> +       { 0x1943, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB },
>> +       { 0x1944, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB },
>> +       { 0x1947, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB },
>> +       {},
>> +};
>> +
>> +static const struct actlr_config sm8550_gfx_actlr_cfg[] = {
>> +       { 0x0000, 0x03ff, PREFETCH_SWITCH_GFX | PREFETCH_DEEP | CPRE | CMTLB },
>> +       {},
>> +};
>> +
>>   static struct qcom_smmu *to_qcom_smmu(struct arm_smmu_device *smmu)
>>   {
>>          return container_of(smmu, struct qcom_smmu, smmu);
>> @@ -549,6 +628,15 @@ 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 = &qcom_smmu_500_impl,
>> +       .adreno_impl = &qcom_adreno_smmu_500_impl,
>> +       .cfg = &qcom_smmu_impl0_cfg,
>> +       .actlrcfg = sm8550_apps_actlr_cfg,
>> +       .actlrcfg_gfx = sm8550_gfx_actlr_cfg,
>> +};
>> +
>>   static const struct qcom_smmu_match_data qcom_smmu_500_impl0_data = {
>>          .impl = &qcom_smmu_500_impl,
>>          .adreno_impl = &qcom_adreno_smmu_500_impl,
>> @@ -583,6 +671,7 @@ static const struct of_device_id __maybe_unused qcom_smmu_impl_of_match[] = {
>>          { .compatible = "qcom,sm8250-smmu-500", .data = &qcom_smmu_500_impl0_data },
>>          { .compatible = "qcom,sm8350-smmu-500", .data = &qcom_smmu_500_impl0_data },
>>          { .compatible = "qcom,sm8450-smmu-500", .data = &qcom_smmu_500_impl0_data },
>> +       { .compatible = "qcom,sm8550-smmu-500", .data = &sm8550_smmu_500_impl0_data },
>>          { .compatible = "qcom,smmu-500", .data = &qcom_smmu_500_impl0_data },
>>          { }
>>   };
>> --
>> 2.17.1
>>
> 
> 

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

[Robin Murphy]

On 2023-12-15 12:20 pm, Bibek Kumar Patro wrote:
> 
> 
> On 12/15/2023 4:14 PM, Dmitry Baryshkov wrote:
>> On Fri, 15 Dec 2023 at 12:19, 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 | 89 ++++++++++++++++++++++
>>>   1 file changed, 89 insertions(+)
>>>
>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c 
>>> b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>> index cb49291f5233..d2006f610243 100644
>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>> @@ -20,6 +20,85 @@ struct actlr_config {
>>>          u32 actlr;
>>>   };
>>>
>>> +/*
>>> + * SMMU-500 TRM defines BIT(0) as CMTLB (Enable context caching in the
>>> + * macro TLB) and BIT(1) as CPRE (Enable context caching in the 
>>> prefetch
>>> + * buffer). The remaining bits are implementation defined and vary 
>>> across
>>> + * SoCs.
>>> + */
>>> +
>>> +#define PREFETCH_DEFAULT       0
>>> +#define PREFETCH_SHALLOW       BIT(8)
>>> +#define PREFETCH_MODERATE      BIT(9)
>>> +#define PREFETCH_DEEP          (BIT(9) | BIT(8))
>>
>> I thin the following might be more correct:
>>
>> #include <linux/bitfield.h>
>>
>> #define PREFETCH_MASK GENMASK(9, 8)
>> #define PREFETCH_DEFAULT FIELD_PREP(PREFETCH_MASK, 0)
>> #define PREFETCH_SHALLOW FIELD_PREP(PREFETCH_MASK, 1)
>> #define PREFETCH_MODERATE FIELD_PREP(PREFETCH_MASK, 2)
>> #define PREFETCH_DEEP FIELD_PREP(PREFETCH_MASK, 3)
>>
> 
> Ack, thanks for this suggestion. Let me try this out using
> GENMASK. Once tested, will take care of this in next version.

FWIW the more typical usage would be to just define the named macros for 
the raw field values, then put the FIELD_PREP() at the point of use. 
However in this case that's liable to get pretty verbose, so although 
I'm usually a fan of bitfield.h, the most readable option here might 
actually be to stick with simpler definitions of "(0 << 8)", "(1 << 8)", 
etc. However it's not really a big deal either way, and I defer to 
whatever Dmitry and Konrad prefer, since they're the ones looking after 
arm-smmu-qcom the most :)

Thanks,
Robin.

> 
> Thanks,
> Bibek
> 
>>> +#define PREFETCH_SWITCH_GFX    (BIT(5) | BIT(3))
>>> +#define CPRE                   BIT(1)
>>> +#define CMTLB                  BIT(0)
>>> +
>>> +static const struct actlr_config sm8550_apps_actlr_cfg[] = {
>>> +       { 0x18a0, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB },
>>> +       { 0x18e0, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB },
>>> +       { 0x0800, 0x0020, PREFETCH_DEFAULT | CMTLB },
>>> +       { 0x1800, 0x00c0, PREFETCH_DEFAULT | CMTLB },
>>> +       { 0x1820, 0x0000, PREFETCH_DEFAULT | CMTLB },
>>> +       { 0x1860, 0x0000, PREFETCH_DEFAULT | CMTLB },
>>> +       { 0x0c01, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
>>> +       { 0x0c02, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
>>> +       { 0x0c03, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
>>> +       { 0x0c04, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
>>> +       { 0x0c05, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
>>> +       { 0x0c06, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
>>> +       { 0x0c07, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
>>> +       { 0x0c08, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
>>> +       { 0x0c09, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
>>> +       { 0x0c0c, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
>>> +       { 0x0c0d, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
>>> +       { 0x0c0e, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
>>> +       { 0x0c0f, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
>>> +       { 0x1961, 0x0000, PREFETCH_DEEP | CPRE | CMTLB },
>>> +       { 0x1962, 0x0000, PREFETCH_DEEP | CPRE | CMTLB },
>>> +       { 0x1963, 0x0000, PREFETCH_DEEP | CPRE | CMTLB },
>>> +       { 0x1964, 0x0000, PREFETCH_DEEP | CPRE | CMTLB },
>>> +       { 0x1965, 0x0000, PREFETCH_DEEP | CPRE | CMTLB },
>>> +       { 0x1966, 0x0000, PREFETCH_DEEP | CPRE | CMTLB },
>>> +       { 0x1967, 0x0000, PREFETCH_DEEP | CPRE | CMTLB },
>>> +       { 0x1968, 0x0000, PREFETCH_DEEP | CPRE | CMTLB },
>>> +       { 0x1969, 0x0000, PREFETCH_DEEP | CPRE | CMTLB },
>>> +       { 0x196c, 0x0000, PREFETCH_DEEP | CPRE | CMTLB },
>>> +       { 0x196d, 0x0000, PREFETCH_DEEP | CPRE | CMTLB },
>>> +       { 0x196e, 0x0000, PREFETCH_DEEP | CPRE | CMTLB },
>>> +       { 0x196f, 0x0000, PREFETCH_DEEP | CPRE | CMTLB },
>>> +       { 0x19c1, 0x0010, PREFETCH_DEEP | CPRE | CMTLB },
>>> +       { 0x19c2, 0x0010, PREFETCH_DEEP | CPRE | CMTLB },
>>> +       { 0x19c3, 0x0010, PREFETCH_DEEP | CPRE | CMTLB },
>>> +       { 0x19c4, 0x0010, PREFETCH_DEEP | CPRE | CMTLB },
>>> +       { 0x19c5, 0x0010, PREFETCH_DEEP | CPRE | CMTLB },
>>> +       { 0x19c6, 0x0010, PREFETCH_DEEP | CPRE | CMTLB },
>>> +       { 0x19c7, 0x0010, PREFETCH_DEEP | CPRE | CMTLB },
>>> +       { 0x19c8, 0x0010, PREFETCH_DEEP | CPRE | CMTLB },
>>> +       { 0x19c9, 0x0010, PREFETCH_DEEP | CPRE | CMTLB },
>>> +       { 0x19cc, 0x0010, PREFETCH_DEEP | CPRE | CMTLB },
>>> +       { 0x19cd, 0x0010, PREFETCH_DEEP | CPRE | CMTLB },
>>> +       { 0x19ce, 0x0010, PREFETCH_DEEP | CPRE | CMTLB },
>>> +       { 0x19cf, 0x0010, PREFETCH_DEEP | CPRE | CMTLB },
>>> +       { 0x1c00, 0x0002, PREFETCH_SHALLOW | CPRE | CMTLB },
>>> +       { 0x1c01, 0x0000, PREFETCH_DEFAULT | CMTLB },
>>> +       { 0x1920, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB },
>>> +       { 0x1923, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB },
>>> +       { 0x1924, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB },
>>> +       { 0x1940, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB },
>>> +       { 0x1941, 0x0004, PREFETCH_SHALLOW | CPRE | CMTLB },
>>> +       { 0x1943, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB },
>>> +       { 0x1944, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB },
>>> +       { 0x1947, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB },
>>> +       {},
>>> +};
>>> +
>>> +static const struct actlr_config sm8550_gfx_actlr_cfg[] = {
>>> +       { 0x0000, 0x03ff, PREFETCH_SWITCH_GFX | PREFETCH_DEEP | CPRE 
>>> | CMTLB },
>>> +       {},
>>> +};
>>> +
>>>   static struct qcom_smmu *to_qcom_smmu(struct arm_smmu_device *smmu)
>>>   {
>>>          return container_of(smmu, struct qcom_smmu, smmu);
>>> @@ -549,6 +628,15 @@ 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 = &qcom_smmu_500_impl,
>>> +       .adreno_impl = &qcom_adreno_smmu_500_impl,
>>> +       .cfg = &qcom_smmu_impl0_cfg,
>>> +       .actlrcfg = sm8550_apps_actlr_cfg,
>>> +       .actlrcfg_gfx = sm8550_gfx_actlr_cfg,
>>> +};
>>> +
>>>   static const struct qcom_smmu_match_data qcom_smmu_500_impl0_data = {
>>>          .impl = &qcom_smmu_500_impl,
>>>          .adreno_impl = &qcom_adreno_smmu_500_impl,
>>> @@ -583,6 +671,7 @@ static const struct of_device_id __maybe_unused 
>>> qcom_smmu_impl_of_match[] = {
>>>          { .compatible = "qcom,sm8250-smmu-500", .data = 
>>> &qcom_smmu_500_impl0_data },
>>>          { .compatible = "qcom,sm8350-smmu-500", .data = 
>>> &qcom_smmu_500_impl0_data },
>>>          { .compatible = "qcom,sm8450-smmu-500", .data = 
>>> &qcom_smmu_500_impl0_data },
>>> +       { .compatible = "qcom,sm8550-smmu-500", .data = 
>>> &sm8550_smmu_500_impl0_data },
>>>          { .compatible = "qcom,smmu-500", .data = 
>>> &qcom_smmu_500_impl0_data },
>>>          { }
>>>   };
>>> -- 
>>> 2.17.1
>>>
>>
>>

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

[Konrad Dybcio]

On 15.12.2023 13:54, Robin Murphy wrote:
> On 2023-12-15 12:20 pm, Bibek Kumar Patro wrote:
>>
>>
>> On 12/15/2023 4:14 PM, Dmitry Baryshkov wrote:
>>> On Fri, 15 Dec 2023 at 12:19, 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 | 89 ++++++++++++++++++++++
>>>>   1 file changed, 89 insertions(+)
>>>>
>>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>> index cb49291f5233..d2006f610243 100644
>>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>> @@ -20,6 +20,85 @@ struct actlr_config {
>>>>          u32 actlr;
>>>>   };
>>>>
>>>> +/*
>>>> + * SMMU-500 TRM defines BIT(0) as CMTLB (Enable context caching in the
>>>> + * macro TLB) and BIT(1) as CPRE (Enable context caching in the prefetch
>>>> + * buffer). The remaining bits are implementation defined and vary across
>>>> + * SoCs.
>>>> + */
>>>> +
>>>> +#define PREFETCH_DEFAULT       0
>>>> +#define PREFETCH_SHALLOW       BIT(8)
>>>> +#define PREFETCH_MODERATE      BIT(9)
>>>> +#define PREFETCH_DEEP          (BIT(9) | BIT(8))
>>>
>>> I thin the following might be more correct:
>>>
>>> #include <linux/bitfield.h>
>>>
>>> #define PREFETCH_MASK GENMASK(9, 8)
>>> #define PREFETCH_DEFAULT FIELD_PREP(PREFETCH_MASK, 0)
>>> #define PREFETCH_SHALLOW FIELD_PREP(PREFETCH_MASK, 1)
>>> #define PREFETCH_MODERATE FIELD_PREP(PREFETCH_MASK, 2)
>>> #define PREFETCH_DEEP FIELD_PREP(PREFETCH_MASK, 3)
>>>
>>
>> Ack, thanks for this suggestion. Let me try this out using
>> GENMASK. Once tested, will take care of this in next version.
> 
> FWIW the more typical usage would be to just define the named macros for the raw field values, then put the FIELD_PREP() at the point of use. However in this case that's liable to get pretty verbose, so although I'm usually a fan of bitfield.h, the most readable option here might actually be to stick with simpler definitions of "(0 << 8)", "(1 << 8)", etc. However it's not really a big deal either way, and I defer to whatever Dmitry and Konrad prefer, since they're the ones looking after arm-smmu-qcom the most :)
My 5 cents would be to just use the "common" style of doing this, so:

#define ACTRL_PREFETCH	GENMASK(9, 8)
 #define PREFETCH_DEFAULT 0
 #define PREFETCH_SHALLOW 1
 #define PREFETCH_MODERATE 2
 #define PREFETCH_DEEP 3

and then use 

| FIELD_PREP(ACTRL_PREFETCH, PREFETCH_x)

it can get verbose, but.. arguably that's good, since you really want
to make sure the right bits are set here

Konrad

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

[Dmitry Baryshkov]

On 16/12/2023 02:03, Konrad Dybcio wrote:
> On 15.12.2023 13:54, Robin Murphy wrote:
>> On 2023-12-15 12:20 pm, Bibek Kumar Patro wrote:
>>>
>>>
>>> On 12/15/2023 4:14 PM, Dmitry Baryshkov wrote:
>>>> On Fri, 15 Dec 2023 at 12:19, 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 | 89 ++++++++++++++++++++++
>>>>>    1 file changed, 89 insertions(+)
>>>>>
>>>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>>> index cb49291f5233..d2006f610243 100644
>>>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>>> @@ -20,6 +20,85 @@ struct actlr_config {
>>>>>           u32 actlr;
>>>>>    };
>>>>>
>>>>> +/*
>>>>> + * SMMU-500 TRM defines BIT(0) as CMTLB (Enable context caching in the
>>>>> + * macro TLB) and BIT(1) as CPRE (Enable context caching in the prefetch
>>>>> + * buffer). The remaining bits are implementation defined and vary across
>>>>> + * SoCs.
>>>>> + */
>>>>> +
>>>>> +#define PREFETCH_DEFAULT       0
>>>>> +#define PREFETCH_SHALLOW       BIT(8)
>>>>> +#define PREFETCH_MODERATE      BIT(9)
>>>>> +#define PREFETCH_DEEP          (BIT(9) | BIT(8))
>>>>
>>>> I thin the following might be more correct:
>>>>
>>>> #include <linux/bitfield.h>
>>>>
>>>> #define PREFETCH_MASK GENMASK(9, 8)
>>>> #define PREFETCH_DEFAULT FIELD_PREP(PREFETCH_MASK, 0)
>>>> #define PREFETCH_SHALLOW FIELD_PREP(PREFETCH_MASK, 1)
>>>> #define PREFETCH_MODERATE FIELD_PREP(PREFETCH_MASK, 2)
>>>> #define PREFETCH_DEEP FIELD_PREP(PREFETCH_MASK, 3)
>>>>
>>>
>>> Ack, thanks for this suggestion. Let me try this out using
>>> GENMASK. Once tested, will take care of this in next version.
>>
>> FWIW the more typical usage would be to just define the named macros for the raw field values, then put the FIELD_PREP() at the point of use. However in this case that's liable to get pretty verbose, so although I'm usually a fan of bitfield.h, the most readable option here might actually be to stick with simpler definitions of "(0 << 8)", "(1 << 8)", etc. However it's not really a big deal either way, and I defer to whatever Dmitry and Konrad prefer, since they're the ones looking after arm-smmu-qcom the most :)
> My 5 cents would be to just use the "common" style of doing this, so:
> 
> #define ACTRL_PREFETCH	GENMASK(9, 8)
>   #define PREFETCH_DEFAULT 0
>   #define PREFETCH_SHALLOW 1
>   #define PREFETCH_MODERATE 2
>   #define PREFETCH_DEEP 3
> 
> and then use
> 
> | FIELD_PREP(ACTRL_PREFETCH, PREFETCH_x)
> 
> it can get verbose, but.. arguably that's good, since you really want
> to make sure the right bits are set here

Sounds good to me.


-- 
With best wishes
Dmitry

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

[Bibek Kumar Patro]

On 12/16/2023 9:45 PM, Dmitry Baryshkov wrote:
> On 16/12/2023 02:03, Konrad Dybcio wrote:
>> On 15.12.2023 13:54, Robin Murphy wrote:
>>> On 2023-12-15 12:20 pm, Bibek Kumar Patro wrote:
>>>>
>>>>
>>>> On 12/15/2023 4:14 PM, Dmitry Baryshkov wrote:
>>>>> On Fri, 15 Dec 2023 at 12:19, 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 | 89 
>>>>>> ++++++++++++++++++++++
>>>>>>    1 file changed, 89 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c 
>>>>>> b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>>>> index cb49291f5233..d2006f610243 100644
>>>>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>>>> @@ -20,6 +20,85 @@ struct actlr_config {
>>>>>>           u32 actlr;
>>>>>>    };
>>>>>>
>>>>>> +/*
>>>>>> + * SMMU-500 TRM defines BIT(0) as CMTLB (Enable context caching 
>>>>>> in the
>>>>>> + * macro TLB) and BIT(1) as CPRE (Enable context caching in the 
>>>>>> prefetch
>>>>>> + * buffer). The remaining bits are implementation defined and 
>>>>>> vary across
>>>>>> + * SoCs.
>>>>>> + */
>>>>>> +
>>>>>> +#define PREFETCH_DEFAULT       0
>>>>>> +#define PREFETCH_SHALLOW       BIT(8)
>>>>>> +#define PREFETCH_MODERATE      BIT(9)
>>>>>> +#define PREFETCH_DEEP          (BIT(9) | BIT(8))
>>>>>
>>>>> I thin the following might be more correct:
>>>>>
>>>>> #include <linux/bitfield.h>
>>>>>
>>>>> #define PREFETCH_MASK GENMASK(9, 8)
>>>>> #define PREFETCH_DEFAULT FIELD_PREP(PREFETCH_MASK, 0)
>>>>> #define PREFETCH_SHALLOW FIELD_PREP(PREFETCH_MASK, 1)
>>>>> #define PREFETCH_MODERATE FIELD_PREP(PREFETCH_MASK, 2)
>>>>> #define PREFETCH_DEEP FIELD_PREP(PREFETCH_MASK, 3)
>>>>>
>>>>
>>>> Ack, thanks for this suggestion. Let me try this out using
>>>> GENMASK. Once tested, will take care of this in next version.
>>>
>>> FWIW the more typical usage would be to just define the named macros 
>>> for the raw field values, then put the FIELD_PREP() at the point of 
>>> use. However in this case that's liable to get pretty verbose, so 
>>> although I'm usually a fan of bitfield.h, the most readable option 
>>> here might actually be to stick with simpler definitions of "(0 << 
>>> 8)", "(1 << 8)", etc. However it's not really a big deal either way, 
>>> and I defer to whatever Dmitry and Konrad prefer, since they're the 
>>> ones looking after arm-smmu-qcom the most :)
>> My 5 cents would be to just use the "common" style of doing this, so:
>>
>> #define ACTRL_PREFETCH    GENMASK(9, 8)
>>   #define PREFETCH_DEFAULT 0
>>   #define PREFETCH_SHALLOW 1
>>   #define PREFETCH_MODERATE 2
>>   #define PREFETCH_DEEP 3
>>
>> and then use
>>
>> | FIELD_PREP(ACTRL_PREFETCH, PREFETCH_x)
>>
>> it can get verbose, but.. arguably that's good, since you really want
>> to make sure the right bits are set here
> 
> Sounds good to me.
> 

Acked.

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

[Bibek Kumar Patro]

On 12/16/2023 9:45 PM, Dmitry Baryshkov wrote:
> On 16/12/2023 02:03, Konrad Dybcio wrote:
>> On 15.12.2023 13:54, Robin Murphy wrote:
>>> On 2023-12-15 12:20 pm, Bibek Kumar Patro wrote:
>>>>
>>>>
>>>> On 12/15/2023 4:14 PM, Dmitry Baryshkov wrote:
>>>>> On Fri, 15 Dec 2023 at 12:19, 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 | 89 
>>>>>> ++++++++++++++++++++++
>>>>>>    1 file changed, 89 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c 
>>>>>> b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>>>> index cb49291f5233..d2006f610243 100644
>>>>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>>>> @@ -20,6 +20,85 @@ struct actlr_config {
>>>>>>           u32 actlr;
>>>>>>    };
>>>>>>
>>>>>> +/*
>>>>>> + * SMMU-500 TRM defines BIT(0) as CMTLB (Enable context caching 
>>>>>> in the
>>>>>> + * macro TLB) and BIT(1) as CPRE (Enable context caching in the 
>>>>>> prefetch
>>>>>> + * buffer). The remaining bits are implementation defined and 
>>>>>> vary across
>>>>>> + * SoCs.
>>>>>> + */
>>>>>> +
>>>>>> +#define PREFETCH_DEFAULT       0
>>>>>> +#define PREFETCH_SHALLOW       BIT(8)
>>>>>> +#define PREFETCH_MODERATE      BIT(9)
>>>>>> +#define PREFETCH_DEEP          (BIT(9) | BIT(8))
>>>>>
>>>>> I thin the following might be more correct:
>>>>>
>>>>> #include <linux/bitfield.h>
>>>>>
>>>>> #define PREFETCH_MASK GENMASK(9, 8)
>>>>> #define PREFETCH_DEFAULT FIELD_PREP(PREFETCH_MASK, 0)
>>>>> #define PREFETCH_SHALLOW FIELD_PREP(PREFETCH_MASK, 1)
>>>>> #define PREFETCH_MODERATE FIELD_PREP(PREFETCH_MASK, 2)
>>>>> #define PREFETCH_DEEP FIELD_PREP(PREFETCH_MASK, 3)
>>>>>
>>>>
>>>> Ack, thanks for this suggestion. Let me try this out using
>>>> GENMASK. Once tested, will take care of this in next version.
>>>
>>> FWIW the more typical usage would be to just define the named macros 
>>> for the raw field values, then put the FIELD_PREP() at the point of 
>>> use. However in this case that's liable to get pretty verbose, so 
>>> although I'm usually a fan of bitfield.h, the most readable option 
>>> here might actually be to stick with simpler definitions of "(0 << 
>>> 8)", "(1 << 8)", etc. However it's not really a big deal either way, 
>>> and I defer to whatever Dmitry and Konrad prefer, since they're the 
>>> ones looking after arm-smmu-qcom the most :)
>> My 5 cents would be to just use the "common" style of doing this, so:
>>
>> #define ACTRL_PREFETCH    GENMASK(9, 8)
>>   #define PREFETCH_DEFAULT 0
>>   #define PREFETCH_SHALLOW 1
>>   #define PREFETCH_MODERATE 2
>>   #define PREFETCH_DEEP 3
>>
>> and then use
>>
>> | FIELD_PREP(ACTRL_PREFETCH, PREFETCH_x)
>>
>> it can get verbose, but.. arguably that's good, since you really want
>> to make sure the right bits are set here
> 
> Sounds good to me.
> 

Konrad, Dimitry, just checked FIELD_PREP() implementation

#define FIELD_FIT(_mask, _val)
({                                                              \
                  __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: ");  \
                  ((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask); \
})

since it is defined as a block, it won't be possible to use FIELD_PREP
in macro or as a structure value, and can only be used inside a 
block/function. Orelse would show compilation errors as following

kernel/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c:94:20: note: in 
expansion of macro 'PREFETCH_SHALLOW'
   { 0x1947, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB },
                     ^
kernel/include/linux/bitfield.h:113:2: error: braced-group within 
expression allowed only inside a function
   ({        \
   ^

So as per my understanding I think, we might need to go ahead with the
generic implementation only. Let me know if I missed something.

Thanks,
Bibek

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

[Dmitry Baryshkov]

On 18/12/2023 13:23, Bibek Kumar Patro wrote:
> 
> 
> On 12/16/2023 9:45 PM, Dmitry Baryshkov wrote:
>> On 16/12/2023 02:03, Konrad Dybcio wrote:
>>> On 15.12.2023 13:54, Robin Murphy wrote:
>>>> On 2023-12-15 12:20 pm, Bibek Kumar Patro wrote:
>>>>>
>>>>>
>>>>> On 12/15/2023 4:14 PM, Dmitry Baryshkov wrote:
>>>>>> On Fri, 15 Dec 2023 at 12:19, 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 | 89 
>>>>>>> ++++++++++++++++++++++
>>>>>>>    1 file changed, 89 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c 
>>>>>>> b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>>>>> index cb49291f5233..d2006f610243 100644
>>>>>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>>>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>>>>> @@ -20,6 +20,85 @@ struct actlr_config {
>>>>>>>           u32 actlr;
>>>>>>>    };
>>>>>>>
>>>>>>> +/*
>>>>>>> + * SMMU-500 TRM defines BIT(0) as CMTLB (Enable context caching 
>>>>>>> in the
>>>>>>> + * macro TLB) and BIT(1) as CPRE (Enable context caching in the 
>>>>>>> prefetch
>>>>>>> + * buffer). The remaining bits are implementation defined and 
>>>>>>> vary across
>>>>>>> + * SoCs.
>>>>>>> + */
>>>>>>> +
>>>>>>> +#define PREFETCH_DEFAULT       0
>>>>>>> +#define PREFETCH_SHALLOW       BIT(8)
>>>>>>> +#define PREFETCH_MODERATE      BIT(9)
>>>>>>> +#define PREFETCH_DEEP          (BIT(9) | BIT(8))
>>>>>>
>>>>>> I thin the following might be more correct:
>>>>>>
>>>>>> #include <linux/bitfield.h>
>>>>>>
>>>>>> #define PREFETCH_MASK GENMASK(9, 8)
>>>>>> #define PREFETCH_DEFAULT FIELD_PREP(PREFETCH_MASK, 0)
>>>>>> #define PREFETCH_SHALLOW FIELD_PREP(PREFETCH_MASK, 1)
>>>>>> #define PREFETCH_MODERATE FIELD_PREP(PREFETCH_MASK, 2)
>>>>>> #define PREFETCH_DEEP FIELD_PREP(PREFETCH_MASK, 3)
>>>>>>
>>>>>
>>>>> Ack, thanks for this suggestion. Let me try this out using
>>>>> GENMASK. Once tested, will take care of this in next version.
>>>>
>>>> FWIW the more typical usage would be to just define the named macros 
>>>> for the raw field values, then put the FIELD_PREP() at the point of 
>>>> use. However in this case that's liable to get pretty verbose, so 
>>>> although I'm usually a fan of bitfield.h, the most readable option 
>>>> here might actually be to stick with simpler definitions of "(0 << 
>>>> 8)", "(1 << 8)", etc. However it's not really a big deal either way, 
>>>> and I defer to whatever Dmitry and Konrad prefer, since they're the 
>>>> ones looking after arm-smmu-qcom the most :)
>>> My 5 cents would be to just use the "common" style of doing this, so:
>>>
>>> #define ACTRL_PREFETCH    GENMASK(9, 8)
>>>   #define PREFETCH_DEFAULT 0
>>>   #define PREFETCH_SHALLOW 1
>>>   #define PREFETCH_MODERATE 2
>>>   #define PREFETCH_DEEP 3
>>>
>>> and then use
>>>
>>> | FIELD_PREP(ACTRL_PREFETCH, PREFETCH_x)
>>>
>>> it can get verbose, but.. arguably that's good, since you really want
>>> to make sure the right bits are set here
>>
>> Sounds good to me.
>>
> 
> Konrad, Dimitry, just checked FIELD_PREP() implementation
> 
> #define FIELD_FIT(_mask, _val)
> ({                                                              \
>                   __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: ");  \
>                   ((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask); \
> })
> 
> since it is defined as a block, it won't be possible to use FIELD_PREP
> in macro or as a structure value, and can only be used inside a 
> block/function. Orelse would show compilation errors as following
> 
> kernel/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c:94:20: note: in 
> expansion of macro 'PREFETCH_SHALLOW'
>    { 0x1947, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB },
>                      ^
> kernel/include/linux/bitfield.h:113:2: error: braced-group within 
> expression allowed only inside a function
>    ({        \
>    ^
> 
> So as per my understanding I think, we might need to go ahead with the
> generic implementation only. Let me know if I missed something.

Then anyway (foo << bar) is better compared to BIT(n) | BIT(m).

-- 
With best wishes
Dmitry

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

[Bibek Kumar Patro]

On 12/18/2023 7:51 PM, Dmitry Baryshkov wrote:
> On 18/12/2023 13:23, Bibek Kumar Patro wrote:
>>
>>
>> On 12/16/2023 9:45 PM, Dmitry Baryshkov wrote:
>>> On 16/12/2023 02:03, Konrad Dybcio wrote:
>>>> On 15.12.2023 13:54, Robin Murphy wrote:
>>>>> On 2023-12-15 12:20 pm, Bibek Kumar Patro wrote:
>>>>>>
>>>>>>
>>>>>> On 12/15/2023 4:14 PM, Dmitry Baryshkov wrote:
>>>>>>> On Fri, 15 Dec 2023 at 12:19, 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 | 89 
>>>>>>>> ++++++++++++++++++++++
>>>>>>>>    1 file changed, 89 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c 
>>>>>>>> b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>>>>>> index cb49291f5233..d2006f610243 100644
>>>>>>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>>>>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>>>>>> @@ -20,6 +20,85 @@ struct actlr_config {
>>>>>>>>           u32 actlr;
>>>>>>>>    };
>>>>>>>>
>>>>>>>> +/*
>>>>>>>> + * SMMU-500 TRM defines BIT(0) as CMTLB (Enable context caching 
>>>>>>>> in the
>>>>>>>> + * macro TLB) and BIT(1) as CPRE (Enable context caching in the 
>>>>>>>> prefetch
>>>>>>>> + * buffer). The remaining bits are implementation defined and 
>>>>>>>> vary across
>>>>>>>> + * SoCs.
>>>>>>>> + */
>>>>>>>> +
>>>>>>>> +#define PREFETCH_DEFAULT       0
>>>>>>>> +#define PREFETCH_SHALLOW       BIT(8)
>>>>>>>> +#define PREFETCH_MODERATE      BIT(9)
>>>>>>>> +#define PREFETCH_DEEP          (BIT(9) | BIT(8))
>>>>>>>
>>>>>>> I thin the following might be more correct:
>>>>>>>
>>>>>>> #include <linux/bitfield.h>
>>>>>>>
>>>>>>> #define PREFETCH_MASK GENMASK(9, 8)
>>>>>>> #define PREFETCH_DEFAULT FIELD_PREP(PREFETCH_MASK, 0)
>>>>>>> #define PREFETCH_SHALLOW FIELD_PREP(PREFETCH_MASK, 1)
>>>>>>> #define PREFETCH_MODERATE FIELD_PREP(PREFETCH_MASK, 2)
>>>>>>> #define PREFETCH_DEEP FIELD_PREP(PREFETCH_MASK, 3)
>>>>>>>
>>>>>>
>>>>>> Ack, thanks for this suggestion. Let me try this out using
>>>>>> GENMASK. Once tested, will take care of this in next version.
>>>>>
>>>>> FWIW the more typical usage would be to just define the named 
>>>>> macros for the raw field values, then put the FIELD_PREP() at the 
>>>>> point of use. However in this case that's liable to get pretty 
>>>>> verbose, so although I'm usually a fan of bitfield.h, the most 
>>>>> readable option here might actually be to stick with simpler 
>>>>> definitions of "(0 << 8)", "(1 << 8)", etc. However it's not really 
>>>>> a big deal either way, and I defer to whatever Dmitry and Konrad 
>>>>> prefer, since they're the ones looking after arm-smmu-qcom the most :)
>>>> My 5 cents would be to just use the "common" style of doing this, so:
>>>>
>>>> #define ACTRL_PREFETCH    GENMASK(9, 8)
>>>>   #define PREFETCH_DEFAULT 0
>>>>   #define PREFETCH_SHALLOW 1
>>>>   #define PREFETCH_MODERATE 2
>>>>   #define PREFETCH_DEEP 3
>>>>
>>>> and then use
>>>>
>>>> | FIELD_PREP(ACTRL_PREFETCH, PREFETCH_x)
>>>>
>>>> it can get verbose, but.. arguably that's good, since you really want
>>>> to make sure the right bits are set here
>>>
>>> Sounds good to me.
>>>
>>
>> Konrad, Dimitry, just checked FIELD_PREP() implementation
>>
>> #define FIELD_FIT(_mask, _val)
>> ({                                                              \
>>                   __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: ");  \
>>                   ((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask); \
>> })
>>
>> since it is defined as a block, it won't be possible to use FIELD_PREP
>> in macro or as a structure value, and can only be used inside a 
>> block/function. Orelse would show compilation errors as following
>>
>> kernel/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c:94:20: note: in 
>> expansion of macro 'PREFETCH_SHALLOW'
>>    { 0x1947, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB },
>>                      ^
>> kernel/include/linux/bitfield.h:113:2: error: braced-group within 
>> expression allowed only inside a function
>>    ({        \
>>    ^
>>
>> So as per my understanding I think, we might need to go ahead with the
>> generic implementation only. Let me know if I missed something.
> 
> Then anyway (foo << bar) is better compared to BIT(n) | BIT(m).
> 

Sure Dmitry, (foo << bar) would be simpler as well as Robin mentioned
earlier in his reply.
I can implement the defines as:

#define PREFETCH_DEFAULT       0
#define PREFETCH_SHALLOW       (1 << 8)
#define PREFETCH_MODERATE      (1 << 9)
#define PREFETCH_DEEP          (3 << 8)

This should be okay I think ?

Thanks,
Bibek

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

[Dmitry Baryshkov]

On Tue, 19 Dec 2023 at 10:25, Bibek Kumar Patro
<quic_bibekkum@quicinc.com> wrote:
>
>
>
> On 12/18/2023 7:51 PM, Dmitry Baryshkov wrote:
> > On 18/12/2023 13:23, Bibek Kumar Patro wrote:
> >>
> >>
> >> On 12/16/2023 9:45 PM, Dmitry Baryshkov wrote:
> >>> On 16/12/2023 02:03, Konrad Dybcio wrote:
> >>>> On 15.12.2023 13:54, Robin Murphy wrote:
> >>>>> On 2023-12-15 12:20 pm, Bibek Kumar Patro wrote:
> >>>>>>
> >>>>>>
> >>>>>> On 12/15/2023 4:14 PM, Dmitry Baryshkov wrote:
> >>>>>>> On Fri, 15 Dec 2023 at 12:19, 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 | 89
> >>>>>>>> ++++++++++++++++++++++
> >>>>>>>>    1 file changed, 89 insertions(+)
> >>>>>>>>
> >>>>>>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> >>>>>>>> b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> >>>>>>>> index cb49291f5233..d2006f610243 100644
> >>>>>>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> >>>>>>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> >>>>>>>> @@ -20,6 +20,85 @@ struct actlr_config {
> >>>>>>>>           u32 actlr;
> >>>>>>>>    };
> >>>>>>>>
> >>>>>>>> +/*
> >>>>>>>> + * SMMU-500 TRM defines BIT(0) as CMTLB (Enable context caching
> >>>>>>>> in the
> >>>>>>>> + * macro TLB) and BIT(1) as CPRE (Enable context caching in the
> >>>>>>>> prefetch
> >>>>>>>> + * buffer). The remaining bits are implementation defined and
> >>>>>>>> vary across
> >>>>>>>> + * SoCs.
> >>>>>>>> + */
> >>>>>>>> +
> >>>>>>>> +#define PREFETCH_DEFAULT       0
> >>>>>>>> +#define PREFETCH_SHALLOW       BIT(8)
> >>>>>>>> +#define PREFETCH_MODERATE      BIT(9)
> >>>>>>>> +#define PREFETCH_DEEP          (BIT(9) | BIT(8))
> >>>>>>>
> >>>>>>> I thin the following might be more correct:
> >>>>>>>
> >>>>>>> #include <linux/bitfield.h>
> >>>>>>>
> >>>>>>> #define PREFETCH_MASK GENMASK(9, 8)
> >>>>>>> #define PREFETCH_DEFAULT FIELD_PREP(PREFETCH_MASK, 0)
> >>>>>>> #define PREFETCH_SHALLOW FIELD_PREP(PREFETCH_MASK, 1)
> >>>>>>> #define PREFETCH_MODERATE FIELD_PREP(PREFETCH_MASK, 2)
> >>>>>>> #define PREFETCH_DEEP FIELD_PREP(PREFETCH_MASK, 3)
> >>>>>>>
> >>>>>>
> >>>>>> Ack, thanks for this suggestion. Let me try this out using
> >>>>>> GENMASK. Once tested, will take care of this in next version.
> >>>>>
> >>>>> FWIW the more typical usage would be to just define the named
> >>>>> macros for the raw field values, then put the FIELD_PREP() at the
> >>>>> point of use. However in this case that's liable to get pretty
> >>>>> verbose, so although I'm usually a fan of bitfield.h, the most
> >>>>> readable option here might actually be to stick with simpler
> >>>>> definitions of "(0 << 8)", "(1 << 8)", etc. However it's not really
> >>>>> a big deal either way, and I defer to whatever Dmitry and Konrad
> >>>>> prefer, since they're the ones looking after arm-smmu-qcom the most :)
> >>>> My 5 cents would be to just use the "common" style of doing this, so:
> >>>>
> >>>> #define ACTRL_PREFETCH    GENMASK(9, 8)
> >>>>   #define PREFETCH_DEFAULT 0
> >>>>   #define PREFETCH_SHALLOW 1
> >>>>   #define PREFETCH_MODERATE 2
> >>>>   #define PREFETCH_DEEP 3
> >>>>
> >>>> and then use
> >>>>
> >>>> | FIELD_PREP(ACTRL_PREFETCH, PREFETCH_x)
> >>>>
> >>>> it can get verbose, but.. arguably that's good, since you really want
> >>>> to make sure the right bits are set here
> >>>
> >>> Sounds good to me.
> >>>
> >>
> >> Konrad, Dimitry, just checked FIELD_PREP() implementation
> >>
> >> #define FIELD_FIT(_mask, _val)
> >> ({                                                              \
> >>                   __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: ");  \
> >>                   ((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask); \
> >> })
> >>
> >> since it is defined as a block, it won't be possible to use FIELD_PREP
> >> in macro or as a structure value, and can only be used inside a
> >> block/function. Orelse would show compilation errors as following
> >>
> >> kernel/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c:94:20: note: in
> >> expansion of macro 'PREFETCH_SHALLOW'
> >>    { 0x1947, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB },
> >>                      ^
> >> kernel/include/linux/bitfield.h:113:2: error: braced-group within
> >> expression allowed only inside a function
> >>    ({        \
> >>    ^
> >>
> >> So as per my understanding I think, we might need to go ahead with the
> >> generic implementation only. Let me know if I missed something.
> >
> > Then anyway (foo << bar) is better compared to BIT(n) | BIT(m).
> >
>
> Sure Dmitry, (foo << bar) would be simpler as well as Robin mentioned
> earlier in his reply.
> I can implement the defines as:
>
> #define PREFETCH_DEFAULT       0
> #define PREFETCH_SHALLOW       (1 << 8)
> #define PREFETCH_MODERATE      (1 << 9)

2 << 8. Isn't that hard.

> #define PREFETCH_DEEP          (3 << 8)
>
> This should be okay I think ?
>
> Thanks,
> Bibek
>


-- 
With best wishes
Dmitry

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

[Bibek Kumar Patro]

On 12/19/2023 3:51 PM, Dmitry Baryshkov wrote:
> On Tue, 19 Dec 2023 at 10:25, Bibek Kumar Patro
> <quic_bibekkum@quicinc.com> wrote:
>>
>>
>>
>> On 12/18/2023 7:51 PM, Dmitry Baryshkov wrote:
>>> On 18/12/2023 13:23, Bibek Kumar Patro wrote:
>>>>
>>>>
>>>> On 12/16/2023 9:45 PM, Dmitry Baryshkov wrote:
>>>>> On 16/12/2023 02:03, Konrad Dybcio wrote:
>>>>>> On 15.12.2023 13:54, Robin Murphy wrote:
>>>>>>> On 2023-12-15 12:20 pm, Bibek Kumar Patro wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 12/15/2023 4:14 PM, Dmitry Baryshkov wrote:
>>>>>>>>> On Fri, 15 Dec 2023 at 12:19, 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 | 89
>>>>>>>>>> ++++++++++++++++++++++
>>>>>>>>>>     1 file changed, 89 insertions(+)
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>>>>>>>> b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>>>>>>>> index cb49291f5233..d2006f610243 100644
>>>>>>>>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>>>>>>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>>>>>>>> @@ -20,6 +20,85 @@ struct actlr_config {
>>>>>>>>>>            u32 actlr;
>>>>>>>>>>     };
>>>>>>>>>>
>>>>>>>>>> +/*
>>>>>>>>>> + * SMMU-500 TRM defines BIT(0) as CMTLB (Enable context caching
>>>>>>>>>> in the
>>>>>>>>>> + * macro TLB) and BIT(1) as CPRE (Enable context caching in the
>>>>>>>>>> prefetch
>>>>>>>>>> + * buffer). The remaining bits are implementation defined and
>>>>>>>>>> vary across
>>>>>>>>>> + * SoCs.
>>>>>>>>>> + */
>>>>>>>>>> +
>>>>>>>>>> +#define PREFETCH_DEFAULT       0
>>>>>>>>>> +#define PREFETCH_SHALLOW       BIT(8)
>>>>>>>>>> +#define PREFETCH_MODERATE      BIT(9)
>>>>>>>>>> +#define PREFETCH_DEEP          (BIT(9) | BIT(8))
>>>>>>>>>
>>>>>>>>> I thin the following might be more correct:
>>>>>>>>>
>>>>>>>>> #include <linux/bitfield.h>
>>>>>>>>>
>>>>>>>>> #define PREFETCH_MASK GENMASK(9, 8)
>>>>>>>>> #define PREFETCH_DEFAULT FIELD_PREP(PREFETCH_MASK, 0)
>>>>>>>>> #define PREFETCH_SHALLOW FIELD_PREP(PREFETCH_MASK, 1)
>>>>>>>>> #define PREFETCH_MODERATE FIELD_PREP(PREFETCH_MASK, 2)
>>>>>>>>> #define PREFETCH_DEEP FIELD_PREP(PREFETCH_MASK, 3)
>>>>>>>>>
>>>>>>>>
>>>>>>>> Ack, thanks for this suggestion. Let me try this out using
>>>>>>>> GENMASK. Once tested, will take care of this in next version.
>>>>>>>
>>>>>>> FWIW the more typical usage would be to just define the named
>>>>>>> macros for the raw field values, then put the FIELD_PREP() at the
>>>>>>> point of use. However in this case that's liable to get pretty
>>>>>>> verbose, so although I'm usually a fan of bitfield.h, the most
>>>>>>> readable option here might actually be to stick with simpler
>>>>>>> definitions of "(0 << 8)", "(1 << 8)", etc. However it's not really
>>>>>>> a big deal either way, and I defer to whatever Dmitry and Konrad
>>>>>>> prefer, since they're the ones looking after arm-smmu-qcom the most :)
>>>>>> My 5 cents would be to just use the "common" style of doing this, so:
>>>>>>
>>>>>> #define ACTRL_PREFETCH    GENMASK(9, 8)
>>>>>>    #define PREFETCH_DEFAULT 0
>>>>>>    #define PREFETCH_SHALLOW 1
>>>>>>    #define PREFETCH_MODERATE 2
>>>>>>    #define PREFETCH_DEEP 3
>>>>>>
>>>>>> and then use
>>>>>>
>>>>>> | FIELD_PREP(ACTRL_PREFETCH, PREFETCH_x)
>>>>>>
>>>>>> it can get verbose, but.. arguably that's good, since you really want
>>>>>> to make sure the right bits are set here
>>>>>
>>>>> Sounds good to me.
>>>>>
>>>>
>>>> Konrad, Dimitry, just checked FIELD_PREP() implementation
>>>>
>>>> #define FIELD_FIT(_mask, _val)
>>>> ({                                                              \
>>>>                    __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: ");  \
>>>>                    ((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask); \
>>>> })
>>>>
>>>> since it is defined as a block, it won't be possible to use FIELD_PREP
>>>> in macro or as a structure value, and can only be used inside a
>>>> block/function. Orelse would show compilation errors as following
>>>>
>>>> kernel/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c:94:20: note: in
>>>> expansion of macro 'PREFETCH_SHALLOW'
>>>>     { 0x1947, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB },
>>>>                       ^
>>>> kernel/include/linux/bitfield.h:113:2: error: braced-group within
>>>> expression allowed only inside a function
>>>>     ({        \
>>>>     ^
>>>>
>>>> So as per my understanding I think, we might need to go ahead with the
>>>> generic implementation only. Let me know if I missed something.
>>>
>>> Then anyway (foo << bar) is better compared to BIT(n) | BIT(m).
>>>
>>
>> Sure Dmitry, (foo << bar) would be simpler as well as Robin mentioned
>> earlier in his reply.
>> I can implement the defines as:
>>
>> #define PREFETCH_DEFAULT       0
>> #define PREFETCH_SHALLOW       (1 << 8)
>> #define PREFETCH_MODERATE      (1 << 9)
> 
> 2 << 8. Isn't that hard.
> 

Ah, right. This is nice! .
Will use 2 << 8 instead. Thanks for the suggestion.

Thanks,
Bibek

>> #define PREFETCH_DEEP          (3 << 8)
>>
>> This should be okay I think ?
>>
>> Thanks,
>> Bibek
>>
> 
> 

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

[Dmitry Baryshkov]

On Tue, 19 Dec 2023 at 12:37, Bibek Kumar Patro
<quic_bibekkum@quicinc.com> wrote:
>
>
>
> On 12/19/2023 3:51 PM, Dmitry Baryshkov wrote:
> > On Tue, 19 Dec 2023 at 10:25, Bibek Kumar Patro
> > <quic_bibekkum@quicinc.com> wrote:
> >>
> >>
> >>
> >> On 12/18/2023 7:51 PM, Dmitry Baryshkov wrote:
> >>> On 18/12/2023 13:23, Bibek Kumar Patro wrote:
> >>>>
> >>>>
> >>>> On 12/16/2023 9:45 PM, Dmitry Baryshkov wrote:
> >>>>> On 16/12/2023 02:03, Konrad Dybcio wrote:
> >>>>>> On 15.12.2023 13:54, Robin Murphy wrote:
> >>>>>>> On 2023-12-15 12:20 pm, Bibek Kumar Patro wrote:
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> On 12/15/2023 4:14 PM, Dmitry Baryshkov wrote:
> >>>>>>>>> On Fri, 15 Dec 2023 at 12:19, 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 | 89
> >>>>>>>>>> ++++++++++++++++++++++
> >>>>>>>>>>     1 file changed, 89 insertions(+)
> >>>>>>>>>>
> >>>>>>>>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> >>>>>>>>>> b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> >>>>>>>>>> index cb49291f5233..d2006f610243 100644
> >>>>>>>>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> >>>>>>>>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> >>>>>>>>>> @@ -20,6 +20,85 @@ struct actlr_config {
> >>>>>>>>>>            u32 actlr;
> >>>>>>>>>>     };
> >>>>>>>>>>
> >>>>>>>>>> +/*
> >>>>>>>>>> + * SMMU-500 TRM defines BIT(0) as CMTLB (Enable context caching
> >>>>>>>>>> in the
> >>>>>>>>>> + * macro TLB) and BIT(1) as CPRE (Enable context caching in the
> >>>>>>>>>> prefetch
> >>>>>>>>>> + * buffer). The remaining bits are implementation defined and
> >>>>>>>>>> vary across
> >>>>>>>>>> + * SoCs.
> >>>>>>>>>> + */
> >>>>>>>>>> +
> >>>>>>>>>> +#define PREFETCH_DEFAULT       0
> >>>>>>>>>> +#define PREFETCH_SHALLOW       BIT(8)
> >>>>>>>>>> +#define PREFETCH_MODERATE      BIT(9)
> >>>>>>>>>> +#define PREFETCH_DEEP          (BIT(9) | BIT(8))
> >>>>>>>>>
> >>>>>>>>> I thin the following might be more correct:
> >>>>>>>>>
> >>>>>>>>> #include <linux/bitfield.h>
> >>>>>>>>>
> >>>>>>>>> #define PREFETCH_MASK GENMASK(9, 8)
> >>>>>>>>> #define PREFETCH_DEFAULT FIELD_PREP(PREFETCH_MASK, 0)
> >>>>>>>>> #define PREFETCH_SHALLOW FIELD_PREP(PREFETCH_MASK, 1)
> >>>>>>>>> #define PREFETCH_MODERATE FIELD_PREP(PREFETCH_MASK, 2)
> >>>>>>>>> #define PREFETCH_DEEP FIELD_PREP(PREFETCH_MASK, 3)
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>> Ack, thanks for this suggestion. Let me try this out using
> >>>>>>>> GENMASK. Once tested, will take care of this in next version.
> >>>>>>>
> >>>>>>> FWIW the more typical usage would be to just define the named
> >>>>>>> macros for the raw field values, then put the FIELD_PREP() at the
> >>>>>>> point of use. However in this case that's liable to get pretty
> >>>>>>> verbose, so although I'm usually a fan of bitfield.h, the most
> >>>>>>> readable option here might actually be to stick with simpler
> >>>>>>> definitions of "(0 << 8)", "(1 << 8)", etc. However it's not really
> >>>>>>> a big deal either way, and I defer to whatever Dmitry and Konrad
> >>>>>>> prefer, since they're the ones looking after arm-smmu-qcom the most :)
> >>>>>> My 5 cents would be to just use the "common" style of doing this, so:
> >>>>>>
> >>>>>> #define ACTRL_PREFETCH    GENMASK(9, 8)
> >>>>>>    #define PREFETCH_DEFAULT 0
> >>>>>>    #define PREFETCH_SHALLOW 1
> >>>>>>    #define PREFETCH_MODERATE 2
> >>>>>>    #define PREFETCH_DEEP 3
> >>>>>>
> >>>>>> and then use
> >>>>>>
> >>>>>> | FIELD_PREP(ACTRL_PREFETCH, PREFETCH_x)
> >>>>>>
> >>>>>> it can get verbose, but.. arguably that's good, since you really want
> >>>>>> to make sure the right bits are set here
> >>>>>
> >>>>> Sounds good to me.
> >>>>>
> >>>>
> >>>> Konrad, Dimitry, just checked FIELD_PREP() implementation
> >>>>
> >>>> #define FIELD_FIT(_mask, _val)
> >>>> ({                                                              \
> >>>>                    __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: ");  \
> >>>>                    ((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask); \
> >>>> })
> >>>>
> >>>> since it is defined as a block, it won't be possible to use FIELD_PREP
> >>>> in macro or as a structure value, and can only be used inside a
> >>>> block/function. Orelse would show compilation errors as following
> >>>>
> >>>> kernel/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c:94:20: note: in
> >>>> expansion of macro 'PREFETCH_SHALLOW'
> >>>>     { 0x1947, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB },
> >>>>                       ^
> >>>> kernel/include/linux/bitfield.h:113:2: error: braced-group within
> >>>> expression allowed only inside a function
> >>>>     ({        \
> >>>>     ^
> >>>>
> >>>> So as per my understanding I think, we might need to go ahead with the
> >>>> generic implementation only. Let me know if I missed something.
> >>>
> >>> Then anyway (foo << bar) is better compared to BIT(n) | BIT(m).
> >>>
> >>
> >> Sure Dmitry, (foo << bar) would be simpler as well as Robin mentioned
> >> earlier in his reply.
> >> I can implement the defines as:
> >>
> >> #define PREFETCH_DEFAULT       0
> >> #define PREFETCH_SHALLOW       (1 << 8)
> >> #define PREFETCH_MODERATE      (1 << 9)
> >
> > 2 << 8. Isn't that hard.
> >
>
> Ah, right. This is nice! .
> Will use 2 << 8 instead. Thanks for the suggestion.

It might still be useful to define the PREFETCH_SHIFT equal to 8.

>
> Thanks,
> Bibek
>
> >> #define PREFETCH_DEEP          (3 << 8)
> >>
> >> This should be okay I think ?
> >>
> >> Thanks,
> >> Bibek
> >>
> >
> >



-- 
With best wishes
Dmitry

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

[Bibek Kumar Patro]

On 12/19/2023 4:14 PM, Dmitry Baryshkov wrote:
> On Tue, 19 Dec 2023 at 12:37, Bibek Kumar Patro
> <quic_bibekkum@quicinc.com> wrote:
>>
>>
>>
>> On 12/19/2023 3:51 PM, Dmitry Baryshkov wrote:
>>> On Tue, 19 Dec 2023 at 10:25, Bibek Kumar Patro
>>> <quic_bibekkum@quicinc.com> wrote:
>>>>
>>>>
>>>>
>>>> On 12/18/2023 7:51 PM, Dmitry Baryshkov wrote:
>>>>> On 18/12/2023 13:23, Bibek Kumar Patro wrote:
>>>>>>
>>>>>>
>>>>>> On 12/16/2023 9:45 PM, Dmitry Baryshkov wrote:
>>>>>>> On 16/12/2023 02:03, Konrad Dybcio wrote:
>>>>>>>> On 15.12.2023 13:54, Robin Murphy wrote:
>>>>>>>>> On 2023-12-15 12:20 pm, Bibek Kumar Patro wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 12/15/2023 4:14 PM, Dmitry Baryshkov wrote:
>>>>>>>>>>> On Fri, 15 Dec 2023 at 12:19, 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 | 89
>>>>>>>>>>>> ++++++++++++++++++++++
>>>>>>>>>>>>      1 file changed, 89 insertions(+)
>>>>>>>>>>>>
>>>>>>>>>>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>>>>>>>>>> b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>>>>>>>>>> index cb49291f5233..d2006f610243 100644
>>>>>>>>>>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>>>>>>>>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>>>>>>>>>> @@ -20,6 +20,85 @@ struct actlr_config {
>>>>>>>>>>>>             u32 actlr;
>>>>>>>>>>>>      };
>>>>>>>>>>>>
>>>>>>>>>>>> +/*
>>>>>>>>>>>> + * SMMU-500 TRM defines BIT(0) as CMTLB (Enable context caching
>>>>>>>>>>>> in the
>>>>>>>>>>>> + * macro TLB) and BIT(1) as CPRE (Enable context caching in the
>>>>>>>>>>>> prefetch
>>>>>>>>>>>> + * buffer). The remaining bits are implementation defined and
>>>>>>>>>>>> vary across
>>>>>>>>>>>> + * SoCs.
>>>>>>>>>>>> + */
>>>>>>>>>>>> +
>>>>>>>>>>>> +#define PREFETCH_DEFAULT       0
>>>>>>>>>>>> +#define PREFETCH_SHALLOW       BIT(8)
>>>>>>>>>>>> +#define PREFETCH_MODERATE      BIT(9)
>>>>>>>>>>>> +#define PREFETCH_DEEP          (BIT(9) | BIT(8))
>>>>>>>>>>>
>>>>>>>>>>> I thin the following might be more correct:
>>>>>>>>>>>
>>>>>>>>>>> #include <linux/bitfield.h>
>>>>>>>>>>>
>>>>>>>>>>> #define PREFETCH_MASK GENMASK(9, 8)
>>>>>>>>>>> #define PREFETCH_DEFAULT FIELD_PREP(PREFETCH_MASK, 0)
>>>>>>>>>>> #define PREFETCH_SHALLOW FIELD_PREP(PREFETCH_MASK, 1)
>>>>>>>>>>> #define PREFETCH_MODERATE FIELD_PREP(PREFETCH_MASK, 2)
>>>>>>>>>>> #define PREFETCH_DEEP FIELD_PREP(PREFETCH_MASK, 3)
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Ack, thanks for this suggestion. Let me try this out using
>>>>>>>>>> GENMASK. Once tested, will take care of this in next version.
>>>>>>>>>
>>>>>>>>> FWIW the more typical usage would be to just define the named
>>>>>>>>> macros for the raw field values, then put the FIELD_PREP() at the
>>>>>>>>> point of use. However in this case that's liable to get pretty
>>>>>>>>> verbose, so although I'm usually a fan of bitfield.h, the most
>>>>>>>>> readable option here might actually be to stick with simpler
>>>>>>>>> definitions of "(0 << 8)", "(1 << 8)", etc. However it's not really
>>>>>>>>> a big deal either way, and I defer to whatever Dmitry and Konrad
>>>>>>>>> prefer, since they're the ones looking after arm-smmu-qcom the most :)
>>>>>>>> My 5 cents would be to just use the "common" style of doing this, so:
>>>>>>>>
>>>>>>>> #define ACTRL_PREFETCH    GENMASK(9, 8)
>>>>>>>>     #define PREFETCH_DEFAULT 0
>>>>>>>>     #define PREFETCH_SHALLOW 1
>>>>>>>>     #define PREFETCH_MODERATE 2
>>>>>>>>     #define PREFETCH_DEEP 3
>>>>>>>>
>>>>>>>> and then use
>>>>>>>>
>>>>>>>> | FIELD_PREP(ACTRL_PREFETCH, PREFETCH_x)
>>>>>>>>
>>>>>>>> it can get verbose, but.. arguably that's good, since you really want
>>>>>>>> to make sure the right bits are set here
>>>>>>>
>>>>>>> Sounds good to me.
>>>>>>>
>>>>>>
>>>>>> Konrad, Dimitry, just checked FIELD_PREP() implementation
>>>>>>
>>>>>> #define FIELD_FIT(_mask, _val)
>>>>>> ({                                                              \
>>>>>>                     __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: ");  \
>>>>>>                     ((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask); \
>>>>>> })
>>>>>>
>>>>>> since it is defined as a block, it won't be possible to use FIELD_PREP
>>>>>> in macro or as a structure value, and can only be used inside a
>>>>>> block/function. Orelse would show compilation errors as following
>>>>>>
>>>>>> kernel/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c:94:20: note: in
>>>>>> expansion of macro 'PREFETCH_SHALLOW'
>>>>>>      { 0x1947, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB },
>>>>>>                        ^
>>>>>> kernel/include/linux/bitfield.h:113:2: error: braced-group within
>>>>>> expression allowed only inside a function
>>>>>>      ({        \
>>>>>>      ^
>>>>>>
>>>>>> So as per my understanding I think, we might need to go ahead with the
>>>>>> generic implementation only. Let me know if I missed something.
>>>>>
>>>>> Then anyway (foo << bar) is better compared to BIT(n) | BIT(m).
>>>>>
>>>>
>>>> Sure Dmitry, (foo << bar) would be simpler as well as Robin mentioned
>>>> earlier in his reply.
>>>> I can implement the defines as:
>>>>
>>>> #define PREFETCH_DEFAULT       0
>>>> #define PREFETCH_SHALLOW       (1 << 8)
>>>> #define PREFETCH_MODERATE      (1 << 9)
>>>
>>> 2 << 8. Isn't that hard.
>>>
>>
>> Ah, right. This is nice! .
>> Will use 2 << 8 instead. Thanks for the suggestion.
> 
> It might still be useful to define the PREFETCH_SHIFT equal to 8.
> 

Sure, looks okay to me as well to define PREFETCH_SHIFT to 8
as it's constant.

Thanks,
Bibek

>>
>> Thanks,
>> Bibek
>>
>>>> #define PREFETCH_DEEP          (3 << 8)
>>>>
>>>> This should be okay I think ?
>>>>
>>>> Thanks,
>>>> Bibek
>>>>
>>>
>>>
> 
> 
> 

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

[Bibek Kumar Patro]

On 12/16/2023 5:33 AM, Konrad Dybcio wrote:
> On 15.12.2023 13:54, Robin Murphy wrote:
>> On 2023-12-15 12:20 pm, Bibek Kumar Patro wrote:
>>>
>>>
>>> On 12/15/2023 4:14 PM, Dmitry Baryshkov wrote:
>>>> On Fri, 15 Dec 2023 at 12:19, 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 | 89 ++++++++++++++++++++++
>>>>>    1 file changed, 89 insertions(+)
>>>>>
>>>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>>> index cb49291f5233..d2006f610243 100644
>>>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>>> @@ -20,6 +20,85 @@ struct actlr_config {
>>>>>           u32 actlr;
>>>>>    };
>>>>>
>>>>> +/*
>>>>> + * SMMU-500 TRM defines BIT(0) as CMTLB (Enable context caching in the
>>>>> + * macro TLB) and BIT(1) as CPRE (Enable context caching in the prefetch
>>>>> + * buffer). The remaining bits are implementation defined and vary across
>>>>> + * SoCs.
>>>>> + */
>>>>> +
>>>>> +#define PREFETCH_DEFAULT       0
>>>>> +#define PREFETCH_SHALLOW       BIT(8)
>>>>> +#define PREFETCH_MODERATE      BIT(9)
>>>>> +#define PREFETCH_DEEP          (BIT(9) | BIT(8))
>>>>
>>>> I thin the following might be more correct:
>>>>
>>>> #include <linux/bitfield.h>
>>>>
>>>> #define PREFETCH_MASK GENMASK(9, 8)
>>>> #define PREFETCH_DEFAULT FIELD_PREP(PREFETCH_MASK, 0)
>>>> #define PREFETCH_SHALLOW FIELD_PREP(PREFETCH_MASK, 1)
>>>> #define PREFETCH_MODERATE FIELD_PREP(PREFETCH_MASK, 2)
>>>> #define PREFETCH_DEEP FIELD_PREP(PREFETCH_MASK, 3)
>>>>
>>>
>>> Ack, thanks for this suggestion. Let me try this out using
>>> GENMASK. Once tested, will take care of this in next version.
>>
>> FWIW the more typical usage would be to just define the named macros for the raw field values, then put the FIELD_PREP() at the point of use. However in this case that's liable to get pretty verbose, so although I'm usually a fan of bitfield.h, the most readable option here might actually be to stick with simpler definitions of "(0 << 8)", "(1 << 8)", etc. However it's not really a big deal either way, and I defer to whatever Dmitry and Konrad prefer, since they're the ones looking after arm-smmu-qcom the most :)
> My 5 cents would be to just use the "common" style of doing this, so:
> 
> #define ACTRL_PREFETCH	GENMASK(9, 8)
>   #define PREFETCH_DEFAULT 0
>   #define PREFETCH_SHALLOW 1
>   #define PREFETCH_MODERATE 2
>   #define PREFETCH_DEEP 3
> 
> and then use
> 
> | FIELD_PREP(ACTRL_PREFETCH, PREFETCH_x)
> 
> it can get verbose, but.. arguably that's good, since you really want
> to make sure the right bits are set here
> 

Thanks for the suggestion with these mods.
Let me try out the suggested way and once tested will post this in next
version.

Thanks,
Bibek

> Konrad

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

[Bibek Kumar Patro]

On 12/15/2023 6:24 PM, Robin Murphy wrote:
> On 2023-12-15 12:20 pm, Bibek Kumar Patro wrote:
>>
>>
>> On 12/15/2023 4:14 PM, Dmitry Baryshkov wrote:
>>> On Fri, 15 Dec 2023 at 12:19, 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 | 89 
>>>> ++++++++++++++++++++++
>>>>   1 file changed, 89 insertions(+)
>>>>
>>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c 
>>>> b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>> index cb49291f5233..d2006f610243 100644
>>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>> @@ -20,6 +20,85 @@ struct actlr_config {
>>>>          u32 actlr;
>>>>   };
>>>>
>>>> +/*
>>>> + * SMMU-500 TRM defines BIT(0) as CMTLB (Enable context caching in the
>>>> + * macro TLB) and BIT(1) as CPRE (Enable context caching in the 
>>>> prefetch
>>>> + * buffer). The remaining bits are implementation defined and vary 
>>>> across
>>>> + * SoCs.
>>>> + */
>>>> +
>>>> +#define PREFETCH_DEFAULT       0
>>>> +#define PREFETCH_SHALLOW       BIT(8)
>>>> +#define PREFETCH_MODERATE      BIT(9)
>>>> +#define PREFETCH_DEEP          (BIT(9) | BIT(8))
>>>
>>> I thin the following might be more correct:
>>>
>>> #include <linux/bitfield.h>
>>>
>>> #define PREFETCH_MASK GENMASK(9, 8)
>>> #define PREFETCH_DEFAULT FIELD_PREP(PREFETCH_MASK, 0)
>>> #define PREFETCH_SHALLOW FIELD_PREP(PREFETCH_MASK, 1)
>>> #define PREFETCH_MODERATE FIELD_PREP(PREFETCH_MASK, 2)
>>> #define PREFETCH_DEEP FIELD_PREP(PREFETCH_MASK, 3)
>>>
>>
>> Ack, thanks for this suggestion. Let me try this out using
>> GENMASK. Once tested, will take care of this in next version.
> 
> FWIW the more typical usage would be to just define the named macros for 
> the raw field values, then put the FIELD_PREP() at the point of use. 
> However in this case that's liable to get pretty verbose, so although 
> I'm usually a fan of bitfield.h, the most readable option here might 
> actually be to stick with simpler definitions of "(0 << 8)", "(1 << 8)", 
> etc. However it's not really a big deal either way, and I defer to 
> whatever Dmitry and Konrad prefer, since they're the ones looking after 
> arm-smmu-qcom the most :)
> 

Agree, surely simple macros would be easy to understand the bits we are
setting/resetting, but to get good verbosity bitfield would surely be
helpful as you rightly pointed. I can see some improved suggestions form
Konrad as well in the latest reply, the way it'd be better in arm-smmu-
qcom. Will try to incorporate these inputs in next version.

Thanks,
Bibek

> Thanks,
> Robin.
> 
>>
>> Thanks,
>> Bibek
>>
>>>> +#define PREFETCH_SWITCH_GFX    (BIT(5) | BIT(3))
>>>> +#define CPRE                   BIT(1)
>>>> +#define CMTLB                  BIT(0)
>>>> +
>>>> +static const struct actlr_config sm8550_apps_actlr_cfg[] = {
>>>> +       { 0x18a0, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB },
>>>> +       { 0x18e0, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB },
>>>> +       { 0x0800, 0x0020, PREFETCH_DEFAULT | CMTLB },
>>>> +       { 0x1800, 0x00c0, PREFETCH_DEFAULT | CMTLB },
>>>> +       { 0x1820, 0x0000, PREFETCH_DEFAULT | CMTLB },
>>>> +       { 0x1860, 0x0000, PREFETCH_DEFAULT | CMTLB },
>>>> +       { 0x0c01, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
>>>> +       { 0x0c02, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
>>>> +       { 0x0c03, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
>>>> +       { 0x0c04, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
>>>> +       { 0x0c05, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
>>>> +       { 0x0c06, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
>>>> +       { 0x0c07, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
>>>> +       { 0x0c08, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
>>>> +       { 0x0c09, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
>>>> +       { 0x0c0c, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
>>>> +       { 0x0c0d, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
>>>> +       { 0x0c0e, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
>>>> +       { 0x0c0f, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
>>>> +       { 0x1961, 0x0000, PREFETCH_DEEP | CPRE | CMTLB },
>>>> +       { 0x1962, 0x0000, PREFETCH_DEEP | CPRE | CMTLB },
>>>> +       { 0x1963, 0x0000, PREFETCH_DEEP | CPRE | CMTLB },
>>>> +       { 0x1964, 0x0000, PREFETCH_DEEP | CPRE | CMTLB },
>>>> +       { 0x1965, 0x0000, PREFETCH_DEEP | CPRE | CMTLB },
>>>> +       { 0x1966, 0x0000, PREFETCH_DEEP | CPRE | CMTLB },
>>>> +       { 0x1967, 0x0000, PREFETCH_DEEP | CPRE | CMTLB },
>>>> +       { 0x1968, 0x0000, PREFETCH_DEEP | CPRE | CMTLB },
>>>> +       { 0x1969, 0x0000, PREFETCH_DEEP | CPRE | CMTLB },
>>>> +       { 0x196c, 0x0000, PREFETCH_DEEP | CPRE | CMTLB },
>>>> +       { 0x196d, 0x0000, PREFETCH_DEEP | CPRE | CMTLB },
>>>> +       { 0x196e, 0x0000, PREFETCH_DEEP | CPRE | CMTLB },
>>>> +       { 0x196f, 0x0000, PREFETCH_DEEP | CPRE | CMTLB },
>>>> +       { 0x19c1, 0x0010, PREFETCH_DEEP | CPRE | CMTLB },
>>>> +       { 0x19c2, 0x0010, PREFETCH_DEEP | CPRE | CMTLB },
>>>> +       { 0x19c3, 0x0010, PREFETCH_DEEP | CPRE | CMTLB },
>>>> +       { 0x19c4, 0x0010, PREFETCH_DEEP | CPRE | CMTLB },
>>>> +       { 0x19c5, 0x0010, PREFETCH_DEEP | CPRE | CMTLB },
>>>> +       { 0x19c6, 0x0010, PREFETCH_DEEP | CPRE | CMTLB },
>>>> +       { 0x19c7, 0x0010, PREFETCH_DEEP | CPRE | CMTLB },
>>>> +       { 0x19c8, 0x0010, PREFETCH_DEEP | CPRE | CMTLB },
>>>> +       { 0x19c9, 0x0010, PREFETCH_DEEP | CPRE | CMTLB },
>>>> +       { 0x19cc, 0x0010, PREFETCH_DEEP | CPRE | CMTLB },
>>>> +       { 0x19cd, 0x0010, PREFETCH_DEEP | CPRE | CMTLB },
>>>> +       { 0x19ce, 0x0010, PREFETCH_DEEP | CPRE | CMTLB },
>>>> +       { 0x19cf, 0x0010, PREFETCH_DEEP | CPRE | CMTLB },
>>>> +       { 0x1c00, 0x0002, PREFETCH_SHALLOW | CPRE | CMTLB },
>>>> +       { 0x1c01, 0x0000, PREFETCH_DEFAULT | CMTLB },
>>>> +       { 0x1920, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB },
>>>> +       { 0x1923, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB },
>>>> +       { 0x1924, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB },
>>>> +       { 0x1940, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB },
>>>> +       { 0x1941, 0x0004, PREFETCH_SHALLOW | CPRE | CMTLB },
>>>> +       { 0x1943, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB },
>>>> +       { 0x1944, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB },
>>>> +       { 0x1947, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB },
>>>> +       {},
>>>> +};
>>>> +
>>>> +static const struct actlr_config sm8550_gfx_actlr_cfg[] = {
>>>> +       { 0x0000, 0x03ff, PREFETCH_SWITCH_GFX | PREFETCH_DEEP | CPRE 
>>>> | CMTLB },
>>>> +       {},
>>>> +};
>>>> +
>>>>   static struct qcom_smmu *to_qcom_smmu(struct arm_smmu_device *smmu)
>>>>   {
>>>>          return container_of(smmu, struct qcom_smmu, smmu);
>>>> @@ -549,6 +628,15 @@ 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 = &qcom_smmu_500_impl,
>>>> +       .adreno_impl = &qcom_adreno_smmu_500_impl,
>>>> +       .cfg = &qcom_smmu_impl0_cfg,
>>>> +       .actlrcfg = sm8550_apps_actlr_cfg,
>>>> +       .actlrcfg_gfx = sm8550_gfx_actlr_cfg,
>>>> +};
>>>> +
>>>>   static const struct qcom_smmu_match_data qcom_smmu_500_impl0_data = {
>>>>          .impl = &qcom_smmu_500_impl,
>>>>          .adreno_impl = &qcom_adreno_smmu_500_impl,
>>>> @@ -583,6 +671,7 @@ static const struct of_device_id __maybe_unused 
>>>> qcom_smmu_impl_of_match[] = {
>>>>          { .compatible = "qcom,sm8250-smmu-500", .data = 
>>>> &qcom_smmu_500_impl0_data },
>>>>          { .compatible = "qcom,sm8350-smmu-500", .data = 
>>>> &qcom_smmu_500_impl0_data },
>>>>          { .compatible = "qcom,sm8450-smmu-500", .data = 
>>>> &qcom_smmu_500_impl0_data },
>>>> +       { .compatible = "qcom,sm8550-smmu-500", .data = 
>>>> &sm8550_smmu_500_impl0_data },
>>>>          { .compatible = "qcom,smmu-500", .data = 
>>>> &qcom_smmu_500_impl0_data },
>>>>          { }
>>>>   };
>>>> -- 
>>>> 2.17.1
>>>>
>>>
>>>

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

[Konrad Dybcio]

On 15.12.2023 11:18, 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>
> ---
[...]

> +static const struct qcom_smmu_match_data sm8550_smmu_500_impl0_data = {
> +	.impl = &qcom_smmu_500_impl,
> +	.adreno_impl = &qcom_adreno_smmu_500_impl,
> +	.cfg = &qcom_smmu_impl0_cfg,
> +	.actlrcfg = sm8550_apps_actlr_cfg,
> +	.actlrcfg_gfx = sm8550_gfx_actlr_cfg,
There are platforms that feature more than just APPS and Adreno SMMUs,
this implementation seems to assume there's only these two :/

I suppose the only way to solve this would be to introduce new compatibles
for each one of them.. Krzysztof, do you think that's reasonable? E.g.
MSM8996 has at least 5 instances, 8998 has at least 4 etc.

Konrad

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

[Dmitry Baryshkov]

On 16/12/2023 01:35, Konrad Dybcio wrote:
> On 15.12.2023 11:18, 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>
>> ---
> [...]
> 
>> +static const struct qcom_smmu_match_data sm8550_smmu_500_impl0_data = {
>> +	.impl = &qcom_smmu_500_impl,
>> +	.adreno_impl = &qcom_adreno_smmu_500_impl,
>> +	.cfg = &qcom_smmu_impl0_cfg,
>> +	.actlrcfg = sm8550_apps_actlr_cfg,
>> +	.actlrcfg_gfx = sm8550_gfx_actlr_cfg,
> There are platforms that feature more than just APPS and Adreno SMMUs,
> this implementation seems to assume there's only these two :/
> 
> I suppose the only way to solve this would be to introduce new compatibles
> for each one of them.. Krzysztof, do you think that's reasonable? E.g.
> MSM8996 has at least 5 instances, 8998 has at least 4 etc.

Ugh. I don't think compatibles will make sense here. I think we have to 
resolve to the hated solution of putting identifying the instance via 
the IO address.

-- 
With best wishes
Dmitry

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

[Bibek Kumar Patro]

On 12/16/2023 5:05 AM, Konrad Dybcio wrote:
> On 15.12.2023 11:18, 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>
>> ---
> [...]
> 
>> +static const struct qcom_smmu_match_data sm8550_smmu_500_impl0_data = {
>> +	.impl = &qcom_smmu_500_impl,
>> +	.adreno_impl = &qcom_adreno_smmu_500_impl,
>> +	.cfg = &qcom_smmu_impl0_cfg,
>> +	.actlrcfg = sm8550_apps_actlr_cfg,
>> +	.actlrcfg_gfx = sm8550_gfx_actlr_cfg,
> There are platforms that feature more than just APPS and Adreno SMMUs,
> this implementation seems to assume there's only these two :/
> 

Yes, some platforms can feature additional SMMUs as well including APPS 
and Adreno. In that case there would be a corresponding compatible 
string and an additional field in qcom_smmu_match_data might be needed.

Thanks,
Bibek

> I suppose the only way to solve this would be to introduce new compatibles
> for each one of them.. Krzysztof, do you think that's reasonable? E.g.
> MSM8996 has at least 5 instances, 8998 has at least 4 etc.
> 
> Konrad

[PATCH v4 4/5] iommu/arm-smmu: add ACTLR data and support for SC7280

[Bibek Kumar Patro]

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

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

diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
index d2006f610243..c8f5dd4186b7 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
@@ -35,6 +35,22 @@ struct actlr_config {
 #define CPRE			BIT(1)
 #define CMTLB			BIT(0)

+static const struct actlr_config sc7280_apps_actlr_cfg[] = {
+	{ 0x0800, 0x24e1, PREFETCH_DEFAULT | CMTLB },
+	{ 0x2000, 0x0163, PREFETCH_DEFAULT | CMTLB },
+	{ 0x2080, 0x0461, PREFETCH_DEFAULT | CMTLB },
+	{ 0x2100, 0x0161, PREFETCH_DEFAULT | CMTLB },
+	{ 0x0900, 0x0407, PREFETCH_SHALLOW | CPRE | CMTLB },
+	{ 0x2180, 0x0027, PREFETCH_SHALLOW | CPRE | CMTLB },
+	{ 0x1000, 0x07ff, PREFETCH_DEEP | CPRE | CMTLB },
+	{},
+};
+
+static const struct actlr_config sc7280_gfx_actlr_cfg[] = {
+	{ 0x0000, 0x07ff, PREFETCH_SWITCH_GFX | PREFETCH_DEEP | CPRE | CMTLB },
+	{},
+};
+
 static const struct actlr_config sm8550_apps_actlr_cfg[] = {
 	{ 0x18a0, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB },
 	{ 0x18e0, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB },
@@ -628,6 +644,13 @@ static const struct qcom_smmu_match_data sdm845_smmu_500_data = {
 	/* Also no debug configuration. */
 };

+static const struct qcom_smmu_match_data sc7280_smmu_500_impl0_data = {
+	.impl = &qcom_smmu_500_impl,
+	.adreno_impl = &qcom_adreno_smmu_500_impl,
+	.cfg = &qcom_smmu_impl0_cfg,
+	.actlrcfg = sc7280_apps_actlr_cfg,
+	.actlrcfg_gfx = sc7280_gfx_actlr_cfg,
+};

 static const struct qcom_smmu_match_data sm8550_smmu_500_impl0_data = {
 	.impl = &qcom_smmu_500_impl,
@@ -654,7 +677,7 @@ static const struct of_device_id __maybe_unused qcom_smmu_impl_of_match[] = {
 	{ .compatible = "qcom,qdu1000-smmu-500", .data = &qcom_smmu_500_impl0_data  },
 	{ .compatible = "qcom,sc7180-smmu-500", .data = &qcom_smmu_500_impl0_data },
 	{ .compatible = "qcom,sc7180-smmu-v2", .data = &qcom_smmu_v2_data },
-	{ .compatible = "qcom,sc7280-smmu-500", .data = &qcom_smmu_500_impl0_data },
+	{ .compatible = "qcom,sc7280-smmu-500", .data = &sc7280_smmu_500_impl0_data },
 	{ .compatible = "qcom,sc8180x-smmu-500", .data = &qcom_smmu_500_impl0_data },
 	{ .compatible = "qcom,sc8280xp-smmu-500", .data = &qcom_smmu_500_impl0_data },
 	{ .compatible = "qcom,sdm630-smmu-v2", .data = &qcom_smmu_v2_data },
--
2.17.1

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

[Bibek Kumar Patro]

Default MMU-500 reset operation disables context caching in
prefetch buffer. It is however expected for context banks using
the ACTLR register to retain their prefetch value during reset
and runtime suspend.

Replace default MMU-500 reset operation with Qualcomm specific reset
operation which envelope the default reset operation and re-enables
context caching in prefetch buffer for Qualcomm SoCs.

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

diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
index c8f5dd4186b7..70d2a5d43993 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
@@ -516,11 +516,28 @@ static int qcom_smmu_def_domain_type(struct device *dev)
 	return match ? IOMMU_DOMAIN_IDENTITY : 0;
 }

+static int qcom_smmu500_reset(struct arm_smmu_device *smmu)
+{
+	int i;
+	u32 reg;
+
+	arm_mmu500_reset(smmu);
+
+	/* arm_mmu500_reset() disables CPRE which is re-enabled here */
+	for (i = 0; i < smmu->num_context_banks; ++i) {
+		reg = arm_smmu_cb_read(smmu, i, ARM_SMMU_CB_ACTLR);
+		reg |= 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,
@@ -547,7 +564,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,
 };
@@ -572,7 +589,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 v4 5/5] iommu/arm-smmu: re-enable context caching in smmu reset operation

[Konrad Dybcio]

On 15.12.2023 11:18, Bibek Kumar Patro wrote:
> Default MMU-500 reset operation disables context caching in
> prefetch buffer. It is however expected for context banks using
> the ACTLR register to retain their prefetch value during reset
> and runtime suspend.
> 
> Replace default MMU-500 reset operation with Qualcomm specific reset
> operation which envelope the default reset operation and re-enables
> context caching in prefetch buffer for Qualcomm SoCs.
> 
> Signed-off-by: Bibek Kumar Patro <quic_bibekkum@quicinc.com>
> ---
This probably deserves a fixes tag, but I can't find a good commit for
it, so I guess not having it is fine as well.

Also, since it seems to be independent from the rest of the patches, please
reorder it to become patch 1 in the next spin, so that it can perhaps be
easily picked up independently of the rest.

>  drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 23 +++++++++++++++++++---
>  1 file changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> index c8f5dd4186b7..70d2a5d43993 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> @@ -516,11 +516,28 @@ static int qcom_smmu_def_domain_type(struct device *dev)
>  	return match ? IOMMU_DOMAIN_IDENTITY : 0;
>  }
> 
> +static int qcom_smmu500_reset(struct arm_smmu_device *smmu)
> +{
> +	int i;
> +	u32 reg;
That's rather nitty/codestyle-y, but:

- reverse Christmas tree would be nice (it's in a week! :D)
- "reg" to me sounds like "register address", "val" is used widely for
  register values

> +
> +	arm_mmu500_reset(smmu);
We should check the return value here, in case the function is modified
some day in a way that makes it return something else than 0

LGTM otherwise!

Konrad

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

[Bibek Kumar Patro]

On 12/16/2023 5:24 AM, Konrad Dybcio wrote:
> On 15.12.2023 11:18, Bibek Kumar Patro wrote:
>> Default MMU-500 reset operation disables context caching in
>> prefetch buffer. It is however expected for context banks using
>> the ACTLR register to retain their prefetch value during reset
>> and runtime suspend.
>>
>> Replace default MMU-500 reset operation with Qualcomm specific reset
>> operation which envelope the default reset operation and re-enables
>> context caching in prefetch buffer for Qualcomm SoCs.
>>
>> Signed-off-by: Bibek Kumar Patro <quic_bibekkum@quicinc.com>
>> ---
> This probably deserves a fixes tag, but I can't find a good commit for
> it, so I guess not having it is fine as well.
> 

I was also trying to find any suitable commit, but this default reset
implementation didn't have a separate commit, so did not add any fixes
tag here.

> Also, since it seems to be independent from the rest of the patches, please
> reorder it to become patch 1 in the next spin, so that it can perhaps be
> easily picked up independently of the rest.
> 

Sure, agree on the same. Will keep this as 1st patch, and start the rest
of the patches on top of it.

>>   drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 23 +++++++++++++++++++---
>>   1 file changed, 20 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>> index c8f5dd4186b7..70d2a5d43993 100644
>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>> @@ -516,11 +516,28 @@ static int qcom_smmu_def_domain_type(struct device *dev)
>>   	return match ? IOMMU_DOMAIN_IDENTITY : 0;
>>   }
>>
>> +static int qcom_smmu500_reset(struct arm_smmu_device *smmu)
>> +{
>> +	int i;
>> +	u32 reg;
> That's rather nitty/codestyle-y, but:
> 
> - reverse Christmas tree would be nice (it's in a week! :D)
> - "reg" to me sounds like "register address", "val" is used widely for
>    register values
> 

Thanks for pointing to this, will take care of this val name and
sorting in next version.(Will try to post the reserse xmas tree sorted
patch asap for the xmas celebration! :) )

>> +
>> +	arm_mmu500_reset(smmu);
> We should check the return value here, in case the function is modified
> some day in a way that makes it return something else than 0
> 
> LGTM otherwise!
>

Thanks,
Bibek

> Konrad

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

[Bibek Kumar Patro]

On 12/16/2023 5:24 AM, Konrad Dybcio wrote:
[...]
>> +
>> +	arm_mmu500_reset(smmu);
> We should check the return value here, in case the function is modified
> some day in a way that makes it return something else than 0
> 

Thanks for pointing this actually. I crosschecked on the
arm_mmu500_reset function behavior, looks like there's no return value
other than 0 and so the functionality won't change. I think we can
keep it as it is in this case.

Thanks,
Bibek

> LGTM otherwise!
> 
> Konrad