This patch series consist of five parts and covers the following:
1. Re-enable context caching for Qualcomm SoCs to retain prefetcher
settings during reset and runtime suspend.
2. Remove cfg inside qcom_smmu structure and replace it with single
pointer to qcom_smmu_match_data avoiding replication of multiple
members from same.
3. Introduce intital set of driver changes to implement ACTLR register
for custom prefetcher settings in Qualcomm SoCs.
4. Add ACTLR data and implementation operations for SM8550.
5. Add ACTLR data and implementation operations for SC7280.
Changes in v6 from v5:
- Remove extra Suggested-by tags.
- Add return check for arm_mmu500_reset in 1/5 as discussed.
Link to v5:
https://lore.kernel.org/all/[email protected]/
Changes in v5 from v4:
New addition:
- Modify copyright year in arm-smmu-qcom.h to 2023 from 2022.
Changes to incorporate suggestions from Dmitry as follows:
- Modify the defines for prefetch in (foo << bar) format
as suggested.(FIELD_PREP could not be used in defines
is not inside any block/function)
Changes to incorporate suggestions from Konrad as follows:
- Shift context caching enablement patch as 1/5 instead of 5/5 to
be picked up as independent patch.
- Fix the codestyle to orient variables in reverse xmas tree format
for patch 1/5.
- Fix variable name in patch 1/5 as suggested.
Link to v3:
https://lore.kernel.org/all/[email protected]/
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/[email protected]/
[2]:
https://download.01.org/0day-ci/archive/20231201/[email protected]/config
Link to v3:
https://lore.kernel.org/all/[email protected]/
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/[email protected]/
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/[email protected]/
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/[email protected]/
Bibek Kumar Patro (5):
iommu/arm-smmu: re-enable context caching in smmu reset operation
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/arm-smmu/arm-smmu-qcom-debug.c | 2 +-
drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 189 +++++++++++++++++-
drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h | 8 +-
drivers/iommu/arm/arm-smmu/arm-smmu.c | 5 +-
drivers/iommu/arm/arm-smmu/arm-smmu.h | 5 +
5 files changed, 199 insertions(+), 10 deletions(-)
--
2.17.1
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 <[email protected]>
---
drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 37 ++++++++++++++++++++--
1 file changed, 34 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 549ae4dba3a6..000e207346af 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
@@ -14,6 +14,16 @@
#define QCOM_DUMMY_VAL -1
+/*
+ * 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 CPRE (1 << 1)
+#define CMTLB (1 << 0)
+
static struct qcom_smmu *to_qcom_smmu(struct arm_smmu_device *smmu)
{
return container_of(smmu, struct qcom_smmu, smmu);
@@ -376,11 +386,32 @@ 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 ret;
+ u32 val;
+ int i;
+
+ ret = arm_mmu500_reset(smmu);
+
+ if (ret)
+ return ret;
+
+ /* arm_mmu500_reset() disables CPRE which is re-enabled here */
+ for (i = 0; i < smmu->num_context_banks; ++i) {
+ val = arm_smmu_cb_read(smmu, i, ARM_SMMU_CB_ACTLR);
+ val |= CPRE;
+ arm_smmu_cb_write(smmu, i, ARM_SMMU_CB_ACTLR, val);
+ }
+
+ 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,
@@ -407,7 +438,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,
};
@@ -432,7 +463,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
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 <[email protected]>
Signed-off-by: Bibek Kumar Patro <[email protected]>
---
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 000e207346af..20c9836d859b 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
@@ -496,7 +496,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
Add ACTLR data table for SC7280 along with support for
same including SC7280 specific implementation operations.
Signed-off-by: Bibek Kumar Patro <[email protected]>
---
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 d6e87d0c8a4f..0b98e07820e1 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
@@ -36,6 +36,22 @@ struct actlr_config {
u32 actlr;
};
+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 },
@@ -650,6 +666,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,
@@ -676,7 +699,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
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 <[email protected]>
---
drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 45 ++++++++++++++++++++++
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 +++
4 files changed, 58 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 20c9836d859b..1cefdd0ca110 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
@@ -24,6 +24,12 @@
#define CPRE (1 << 1)
#define CMTLB (1 << 0)
+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);
@@ -215,9 +221,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;
@@ -248,6 +283,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;
}
@@ -274,6 +312,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..cb4cb402c202 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h
@@ -1,6 +1,6 @@
/* SPDX-License-Identifier: GPL-2.0-only */
/*
- * Copyright (c) 2022, Qualcomm Innovation Center, Inc. All rights reserved.
+ * Copyright (c) 2023, Qualcomm Innovation Center, Inc. All rights reserved.
*/
#ifndef _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
Add ACTLR data table for SM8550 along with support for
same including SM8550 specific implementation operations.
Signed-off-by: Bibek Kumar Patro <[email protected]>
---
drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 80 ++++++++++++++++++++++
1 file changed, 80 insertions(+)
diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
index 1cefdd0ca110..d6e87d0c8a4f 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
@@ -23,6 +23,12 @@
#define CPRE (1 << 1)
#define CMTLB (1 << 0)
+#define PREFETCH_SHIFT 8
+#define PREFETCH_DEFAULT 0
+#define PREFETCH_SHALLOW (1 << PREFETCH_SHIFT)
+#define PREFETCH_MODERATE (2 << PREFETCH_SHIFT)
+#define PREFETCH_DEEP (3 << PREFETCH_SHIFT)
+#define PREFETCH_SWITCH_GFX (5 << 3)
struct actlr_config {
u16 sid;
@@ -30,6 +36,70 @@ struct actlr_config {
u32 actlr;
};
+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);
@@ -580,6 +650,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,
@@ -614,6 +693,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
On Wed, 20 Dec 2023 at 15:38, Bibek Kumar Patro
<[email protected]> wrote:
>
> 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 <[email protected]>
> Signed-off-by: Bibek Kumar Patro <[email protected]>
> ---
> 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(-)
Reviewed-by: Dmitry Baryshkov <[email protected]>
--
With best wishes
Dmitry
On Wed, 20 Dec 2023 at 15:39, Bibek Kumar Patro
<[email protected]> wrote:
>
> Currently in Qualcomm SoCs the default prefetch is set to 1 which allows
> the TLB to fetch just the next page table. MMU-500 features ACTLR
> register which is implementation defined and is used for Qualcomm SoCs
> to have a 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 <[email protected]>
> ---
> drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 45 ++++++++++++++++++++++
> 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 +++
> 4 files changed, 58 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 20c9836d859b..1cefdd0ca110 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> @@ -24,6 +24,12 @@
> #define CPRE (1 << 1)
> #define CMTLB (1 << 0)
>
> +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);
> @@ -215,9 +221,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;
> @@ -248,6 +283,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);
There was a feedback point against v4 that there can be more than two
(apps + gpu) SMMU devices. No, we can not use additional compat
strings, the SMMU units are compatible with each other. Please add
matching between the smmu and particular actlr table using the IO
address of the SMMU block.
> +
> return 0;
> }
>
> @@ -274,6 +312,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);
> +
One issue occured to me, while I was reviewing the patchset. The ACTLR
settings are related to the whole SMMU setup, but we are applying them
each time there is an SMMU context init (in other words, one per each
domain). Is that correct? Or it's just that there is no better place
for initialising the global register set? Would it be better to
reprogram the ACTLR registers which are related just to this
particular domain?
> 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..cb4cb402c202 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h
> @@ -1,6 +1,6 @@
> /* SPDX-License-Identifier: GPL-2.0-only */
> /*
> - * Copyright (c) 2022, Qualcomm Innovation Center, Inc. All rights reserved.
> + * Copyright (c) 2023, Qualcomm Innovation Center, Inc. All rights reserved.
> */
>
> #ifndef _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
>
--
With best wishes
Dmitry
On Thu, 21 Dec 2023 at 02:36, Dmitry Baryshkov
<[email protected]> wrote:
>
> On Wed, 20 Dec 2023 at 15:39, Bibek Kumar Patro
> <[email protected]> wrote:
> >
> > Currently in Qualcomm SoCs the default prefetch is set to 1 which allows
> > the TLB to fetch just the next page table. MMU-500 features ACTLR
> > register which is implementation defined and is used for Qualcomm SoCs
> > to have a 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 <[email protected]>
> > ---
> > drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 45 ++++++++++++++++++++++
> > 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 +++
> > 4 files changed, 58 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 20c9836d859b..1cefdd0ca110 100644
> > --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> > +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> > @@ -24,6 +24,12 @@
> > #define CPRE (1 << 1)
> > #define CMTLB (1 << 0)
> >
> > +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);
> > @@ -215,9 +221,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;
> > @@ -248,6 +283,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);
>
> There was a feedback point against v4 that there can be more than two
> (apps + gpu) SMMU devices. No, we can not use additional compat
> strings, the SMMU units are compatible with each other. Please add
> matching between the smmu and particular actlr table using the IO
> address of the SMMU block.
>
> > +
> > return 0;
> > }
> >
> > @@ -274,6 +312,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);
> > +
>
> One issue occured to me, while I was reviewing the patchset. The ACTLR
> settings are related to the whole SMMU setup, but we are applying them
> each time there is an SMMU context init (in other words, one per each
> domain). Is that correct? Or it's just that there is no better place
> for initialising the global register set? Would it be better to
> reprogram the ACTLR registers which are related just to this
> particular domain?
Ignore this, I went back to qcom_smmu_set_actlr() and understood that
I failed to read it properly.
>
> > 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..cb4cb402c202 100644
> > --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h
> > +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h
> > @@ -1,6 +1,6 @@
> > /* SPDX-License-Identifier: GPL-2.0-only */
> > /*
> > - * Copyright (c) 2022, Qualcomm Innovation Center, Inc. All rights reserved.
> > + * Copyright (c) 2023, Qualcomm Innovation Center, Inc. All rights reserved.
> > */
> >
> > #ifndef _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
> >
>
>
> --
> With best wishes
> Dmitry
--
With best wishes
Dmitry
On 12/21/2023 6:06 AM, Dmitry Baryshkov wrote:
> On Wed, 20 Dec 2023 at 15:39, Bibek Kumar Patro
> <[email protected]> wrote:
>>
>> Currently in Qualcomm SoCs the default prefetch is set to 1 which allows
>> the TLB to fetch just the next page table. MMU-500 features ACTLR
>> register which is implementation defined and is used for Qualcomm SoCs
>> to have a 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 <[email protected]>
>> ---
>> drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 45 ++++++++++++++++++++++
>> 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 +++
>> 4 files changed, 58 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 20c9836d859b..1cefdd0ca110 100644
>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>> @@ -24,6 +24,12 @@
>> #define CPRE (1 << 1)
>> #define CMTLB (1 << 0)
>>
>> +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);
>> @@ -215,9 +221,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;
>> @@ -248,6 +283,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);
>
> There was a feedback point against v4 that there can be more than two
> (apps + gpu) SMMU devices. No, we can not use additional compat
> strings, the SMMU units are compatible with each other.
Just to understand better, did you mean if in the below check
[inside arm-smmu-qcom.c file during qcom_smmu_create()], "else" has two
things? (Currently adreno_impl for gpu smmu, else for only
apps smmu)
if (np && of_device_is_compatible(np, "qcom,adreno-smmu"))
impl = data->adreno_impl;
else
impl = data->impl;
> Please add
> matching between the smmu and particular actlr table using the IO
> address of the SMMU block.
>
The ACTLR table for each smmu will have A IO address attached, so based
on IO address we can apply ACTLR.
Is this your proposal((IMO hardcoding IO in driver won't be viable,
isn't it?), or in smmu DT we would need to set the IO?
Thanks & regards,
Bibek
>> +
>> return 0;
>> }
>>
>> @@ -274,6 +312,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);
>> +
>
> One issue occured to me, while I was reviewing the patchset. The ACTLR
> settings are related to the whole SMMU setup, but we are applying them
> each time there is an SMMU context init (in other words, one per each
> domain). Is that correct? Or it's just that there is no better place
> for initialising the global register set? Would it be better to
> reprogram the ACTLR registers which are related just to this
> particular domain?
>
>> 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..cb4cb402c202 100644
>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h
>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h
>> @@ -1,6 +1,6 @@
>> /* SPDX-License-Identifier: GPL-2.0-only */
>> /*
>> - * Copyright (c) 2022, Qualcomm Innovation Center, Inc. All rights reserved.
>> + * Copyright (c) 2023, Qualcomm Innovation Center, Inc. All rights reserved.
>> */
>>
>> #ifndef _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
>>
>
>
On Thu, 21 Dec 2023 at 12:02, Bibek Kumar Patro
<[email protected]> wrote:
>
>
>
> On 12/21/2023 6:06 AM, Dmitry Baryshkov wrote:
> > On Wed, 20 Dec 2023 at 15:39, Bibek Kumar Patro
> > <[email protected]> wrote:
> >>
> >> Currently in Qualcomm SoCs the default prefetch is set to 1 which allows
> >> the TLB to fetch just the next page table. MMU-500 features ACTLR
> >> register which is implementation defined and is used for Qualcomm SoCs
> >> to have a 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 <[email protected]>
> >> ---
> >> drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 45 ++++++++++++++++++++++
> >> 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 +++
> >> 4 files changed, 58 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 20c9836d859b..1cefdd0ca110 100644
> >> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> >> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> >> @@ -24,6 +24,12 @@
> >> #define CPRE (1 << 1)
> >> #define CMTLB (1 << 0)
> >>
> >> +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);
> >> @@ -215,9 +221,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;
> >> @@ -248,6 +283,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);
> >
> > There was a feedback point against v4 that there can be more than two
> > (apps + gpu) SMMU devices. No, we can not use additional compat
> > strings, the SMMU units are compatible with each other.
>
> Just to understand better, did you mean if in the below check
> [inside arm-smmu-qcom.c file during qcom_smmu_create()], "else" has two
> things? (Currently adreno_impl for gpu smmu, else for only
> apps smmu)
qcom,adreno-smmu is quite unique here, this is the only distinctive
substring. We do not have such compat strings for any other of SMMU
nodes.
>
> if (np && of_device_is_compatible(np, "qcom,adreno-smmu"))
> impl = data->adreno_impl;
> else
> impl = data->impl;
>
> > Please add
> > matching between the smmu and particular actlr table using the IO
> > address of the SMMU block.
> >
>
> The ACTLR table for each smmu will have A IO address attached, so based
> on IO address we can apply ACTLR.
> Is this your proposal((IMO hardcoding IO in driver won't be viable,
> isn't it?), or in smmu DT we would need to set the IO?
Unfortunately, I meant exactly that: hardcoding addresses of the SMMU
register spaces. see drivers/gpu/drm/msm/dsi_cfg.c
Then during device probe the driver can match the IO address to the
list of the per-platform ACTLR tables and select the correct one.
Then you don't even need a special actlrcfg_gfx. The GFX will fall
into the main schema.
>
>
> Thanks & regards,
> Bibek
>
> >> +
> >> return 0;
> >> }
> >>
> >> @@ -274,6 +312,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);
> >> +
> >
> > One issue occured to me, while I was reviewing the patchset. The ACTLR
> > settings are related to the whole SMMU setup, but we are applying them
> > each time there is an SMMU context init (in other words, one per each
> > domain). Is that correct? Or it's just that there is no better place
> > for initialising the global register set? Would it be better to
> > reprogram the ACTLR registers which are related just to this
> > particular domain?
> >
> >> 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..cb4cb402c202 100644
> >> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h
> >> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h
> >> @@ -1,6 +1,6 @@
> >> /* SPDX-License-Identifier: GPL-2.0-only */
> >> /*
> >> - * Copyright (c) 2022, Qualcomm Innovation Center, Inc. All rights reserved.
> >> + * Copyright (c) 2023, Qualcomm Innovation Center, Inc. All rights reserved.
> >> */
> >>
> >> #ifndef _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
> >>
> >
> >
--
With best wishes
Dmitry
On 12/21/2023 4:02 PM, Dmitry Baryshkov wrote:
> On Thu, 21 Dec 2023 at 12:02, Bibek Kumar Patro
> <[email protected]> wrote:
>>
>>
>>
>> On 12/21/2023 6:06 AM, Dmitry Baryshkov wrote:
>>> On Wed, 20 Dec 2023 at 15:39, Bibek Kumar Patro
>>> <[email protected]> wrote:
>>>>
>>>> Currently in Qualcomm SoCs the default prefetch is set to 1 which allows
>>>> the TLB to fetch just the next page table. MMU-500 features ACTLR
>>>> register which is implementation defined and is used for Qualcomm SoCs
>>>> to have a 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 <[email protected]>
>>>> ---
>>>> drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 45 ++++++++++++++++++++++
>>>> 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 +++
>>>> 4 files changed, 58 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 20c9836d859b..1cefdd0ca110 100644
>>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>> @@ -24,6 +24,12 @@
>>>> #define CPRE (1 << 1)
>>>> #define CMTLB (1 << 0)
>>>>
>>>> +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);
>>>> @@ -215,9 +221,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;
>>>> @@ -248,6 +283,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);
>>>
>>> There was a feedback point against v4 that there can be more than two
>>> (apps + gpu) SMMU devices. No, we can not use additional compat
>>> strings, the SMMU units are compatible with each other.
>>
>> Just to understand better, did you mean if in the below check
>> [inside arm-smmu-qcom.c file during qcom_smmu_create()], "else" has two
>> things? (Currently adreno_impl for gpu smmu, else for only
>> apps smmu)
>
> qcom,adreno-smmu is quite unique here, this is the only distinctive
> substring. We do not have such compat strings for any other of SMMU
> nodes.
>
Apologies for the delayed response, I was on leave and could
not monitor the replies.
for other SMMUs (except qcom-adreno-smmu) we would
need to use the IO address matching.
>>
>> if (np && of_device_is_compatible(np, "qcom,adreno-smmu"))
>> impl = data->adreno_impl;
>> else
>> impl = data->impl;
>>
>>> Please add
>>> matching between the smmu and particular actlr table using the IO
>>> address of the SMMU block.
>>>
>>
>> The ACTLR table for each smmu will have A IO address attached, so based
>> on IO address we can apply ACTLR.
>> Is this your proposal((IMO hardcoding IO in driver won't be viable,
>> isn't it?), or in smmu DT we would need to set the IO?
>
> Unfortunately, I meant exactly that: hardcoding addresses of the SMMU
> register spaces. see drivers/gpu/drm/msm/dsi_cfg.c
> Then during device probe the driver can match the IO address to the
> list of the per-platform ACTLR tables and select the correct one.
> Then you don't even need a special actlrcfg_gfx. The GFX will fall
> into the main schema.
>
Thanks for the reference, I will check once and try to evaluate a
similar implementation for ACTLR table as well.
Thanks,
Bibek
>>
>>
>> Thanks & regards,
>> Bibek
>>
>>>> +
>>>> return 0;
>>>> }
>>>>
>>>> @@ -274,6 +312,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);
>>>> +
>>>
>>> One issue occured to me, while I was reviewing the patchset. The ACTLR
>>> settings are related to the whole SMMU setup, but we are applying them
>>> each time there is an SMMU context init (in other words, one per each
>>> domain). Is that correct? Or it's just that there is no better place
>>> for initialising the global register set? Would it be better to
>>> reprogram the ACTLR registers which are related just to this
>>> particular domain?
>>>
>>>> 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..cb4cb402c202 100644
>>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h
>>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h
>>>> @@ -1,6 +1,6 @@
>>>> /* SPDX-License-Identifier: GPL-2.0-only */
>>>> /*
>>>> - * Copyright (c) 2022, Qualcomm Innovation Center, Inc. All rights reserved.
>>>> + * Copyright (c) 2023, Qualcomm Innovation Center, Inc. All rights reserved.
>>>> */
>>>>
>>>> #ifndef _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
>>>>
>>>
>>>
>
>
>