Currently in Qualcomm SoCs the default prefetch is set to 1 which allows
the TLB to fetch just the next page table. MMU-500 features ACTLR
register which is implementation defined and is used for Qualcomm SoCs
to have a prefetch setting of 1/3/7/15 enabling TLB to prefetch
the next set of page tables accordingly allowing for faster translations.
ACTLR value is unique for each SMR (Stream matching register) and stored
in a pre-populated table. This value is set to the register during
context bank initialisation.
Signed-off-by: Bibek Kumar Patro <quic_bibekkum@quicinc.com>
---
drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 34 ++++++++++++++++++++++
drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h | 2 ++
drivers/iommu/arm/arm-smmu/arm-smmu.c | 5 ++--
drivers/iommu/arm/arm-smmu/arm-smmu.h | 5 ++++
4 files changed, 44 insertions(+), 2 deletions(-)
diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
index ae7cae015193..68c1f4908473 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
@@ -14,6 +14,17 @@
#define QCOM_DUMMY_VAL -1
+struct actlr_config {
+ const struct actlr_data *adata;
+ u32 size;
+};
+
+struct actlr_data {
+ u16 sid;
+ u16 mask;
+ u32 actlr;
+};
+
static struct qcom_smmu *to_qcom_smmu(struct arm_smmu_device *smmu)
{
return container_of(smmu, struct qcom_smmu, smmu);
@@ -270,6 +281,26 @@ static const struct of_device_id qcom_smmu_client_of_match[] __maybe_unused = {
static int qcom_smmu_init_context(struct arm_smmu_domain *smmu_domain,
struct io_pgtable_cfg *pgtbl_cfg, struct device *dev)
{
+ struct arm_smmu_device *smmu = smmu_domain->smmu;
+ struct qcom_smmu *qsmmu = to_qcom_smmu(smmu);
+ const struct actlr_config *actlrcfg;
+ struct arm_smmu_smr *smr = smmu->smrs;
+ int idx = smmu_domain->cfg.cbndx;
+ int i;
+ u16 id;
+ u16 mask;
+
+ if (qsmmu->actlrcfg) {
+ actlrcfg = qsmmu->actlrcfg;
+ for (i = 0; i < actlrcfg->size; ++i) {
+ id = actlrcfg->adata[i].sid;
+ mask = actlrcfg->adata[i].mask;
+ if (!smr_is_subset(*smr, id, mask))
+ arm_smmu_cb_write(smmu, idx, ARM_SMMU_CB_ACTLR,
+ actlrcfg->adata[i].actlr);
+ }
+ }
+
smmu_domain->cfg.flush_walk_prefer_tlbiasid = true;
return 0;
@@ -459,6 +490,9 @@ static struct arm_smmu_device *qcom_smmu_create(struct arm_smmu_device *smmu,
qsmmu->smmu.impl = impl;
qsmmu->cfg = data->cfg;
+ if (data->actlrcfg && (data->actlrcfg->size))
+ qsmmu->actlrcfg = data->actlrcfg;
+
return &qsmmu->smmu;
}
diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h
index 593910567b88..4b6862715070 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h
@@ -9,6 +9,7 @@
struct qcom_smmu {
struct arm_smmu_device smmu;
const struct qcom_smmu_config *cfg;
+ const struct actlr_config *actlrcfg;
bool bypass_quirk;
u8 bypass_cbndx;
u32 stall_enabled;
@@ -25,6 +26,7 @@ struct qcom_smmu_config {
};
struct qcom_smmu_match_data {
+ const struct actlr_config *actlrcfg;
const struct qcom_smmu_config *cfg;
const struct arm_smmu_impl *impl;
const struct arm_smmu_impl *adreno_impl;
diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
index 4c79ef6f4c75..38ac1cbc799b 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
@@ -992,9 +992,10 @@ static int arm_smmu_find_sme(struct arm_smmu_device *smmu, u16 id, u16 mask)
* expect simply identical entries for this case, but there's
* no harm in accommodating the generalisation.
*/
- if ((mask & smrs[i].mask) == mask &&
- !((id ^ smrs[i].id) & ~smrs[i].mask))
+
+ if (smr_is_subset(smrs[i], id, mask))
return i;
+
/*
* If the new entry has any other overlap with an existing one,
* though, then there always exists at least one stream ID
diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.h b/drivers/iommu/arm/arm-smmu/arm-smmu.h
index 703fd5817ec1..b1638bbc41d4 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.h
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.h
@@ -501,6 +501,11 @@ static inline void arm_smmu_writeq(struct arm_smmu_device *smmu, int page,
writeq_relaxed(val, arm_smmu_page(smmu, page) + offset);
}
+static inline bool smr_is_subset(struct arm_smmu_smr smrs, u16 id, u16 mask)
+{
+ return (mask & smrs.mask) == mask && !((id ^ smrs.id) & ~smrs.mask);
+}
+
#define ARM_SMMU_GR0 0
#define ARM_SMMU_GR1 1
#define ARM_SMMU_CB(s, n) ((s)->numpage + (n))
--
2.17.1On Fri, 3 Nov 2023 at 23:53, Bibek Kumar Patro
<quic_bibekkum@quicinc.com> wrote:
>
> Currently in Qualcomm SoCs the default prefetch is set to 1 which allows
> the TLB to fetch just the next page table. MMU-500 features ACTLR
> register which is implementation defined and is used for Qualcomm SoCs
> to have a prefetch setting of 1/3/7/15 enabling TLB to prefetch
> the next set of page tables accordingly allowing for faster translations.
>
> ACTLR value is unique for each SMR (Stream matching register) and stored
> in a pre-populated table. This value is set to the register during
> context bank initialisation.
>
> Signed-off-by: Bibek Kumar Patro <quic_bibekkum@quicinc.com>
> ---
> drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 34 ++++++++++++++++++++++
> drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h | 2 ++
> drivers/iommu/arm/arm-smmu/arm-smmu.c | 5 ++--
> drivers/iommu/arm/arm-smmu/arm-smmu.h | 5 ++++
> 4 files changed, 44 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> index ae7cae015193..68c1f4908473 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> @@ -14,6 +14,17 @@
>
> #define QCOM_DUMMY_VAL -1
>
> +struct actlr_config {
> + const struct actlr_data *adata;
> + u32 size;
This should be size_t.
Also could you please drop the separate struct actlr_config and move
these two fields into struct qcom_smmu_config.
> +};
> +
> +struct actlr_data {
> + u16 sid;
> + u16 mask;
> + u32 actlr;
> +};
> +
> static struct qcom_smmu *to_qcom_smmu(struct arm_smmu_device *smmu)
> {
> return container_of(smmu, struct qcom_smmu, smmu);
> @@ -270,6 +281,26 @@ static const struct of_device_id qcom_smmu_client_of_match[] __maybe_unused = {
> static int qcom_smmu_init_context(struct arm_smmu_domain *smmu_domain,
> struct io_pgtable_cfg *pgtbl_cfg, struct device *dev)
> {
> + struct arm_smmu_device *smmu = smmu_domain->smmu;
> + struct qcom_smmu *qsmmu = to_qcom_smmu(smmu);
> + const struct actlr_config *actlrcfg;
> + struct arm_smmu_smr *smr = smmu->smrs;
> + int idx = smmu_domain->cfg.cbndx;
> + int i;
> + u16 id;
> + u16 mask;
> +
> + if (qsmmu->actlrcfg) {
> + actlrcfg = qsmmu->actlrcfg;
> + for (i = 0; i < actlrcfg->size; ++i) {
> + id = actlrcfg->adata[i].sid;
> + mask = actlrcfg->adata[i].mask;
> + if (!smr_is_subset(*smr, id, mask))
> + arm_smmu_cb_write(smmu, idx, ARM_SMMU_CB_ACTLR,
> + actlrcfg->adata[i].actlr);
> + }
> + }
Consider extracting this to a separate function. This way you can
reduce 4 indentation levels into a single loop.
> +
> smmu_domain->cfg.flush_walk_prefer_tlbiasid = true;
>
> return 0;
> @@ -459,6 +490,9 @@ static struct arm_smmu_device *qcom_smmu_create(struct arm_smmu_device *smmu,
> qsmmu->smmu.impl = impl;
> qsmmu->cfg = data->cfg;
>
> + if (data->actlrcfg && (data->actlrcfg->size))
> + qsmmu->actlrcfg = data->actlrcfg;
> +
> return &qsmmu->smmu;
> }
>
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h
> index 593910567b88..4b6862715070 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h
> @@ -9,6 +9,7 @@
> struct qcom_smmu {
> struct arm_smmu_device smmu;
> const struct qcom_smmu_config *cfg;
> + const struct actlr_config *actlrcfg;
> bool bypass_quirk;
> u8 bypass_cbndx;
> u32 stall_enabled;
> @@ -25,6 +26,7 @@ struct qcom_smmu_config {
> };
>
> struct qcom_smmu_match_data {
> + const struct actlr_config *actlrcfg;
> const struct qcom_smmu_config *cfg;
> const struct arm_smmu_impl *impl;
> const struct arm_smmu_impl *adreno_impl;
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> index 4c79ef6f4c75..38ac1cbc799b 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> @@ -992,9 +992,10 @@ static int arm_smmu_find_sme(struct arm_smmu_device *smmu, u16 id, u16 mask)
> * expect simply identical entries for this case, but there's
> * no harm in accommodating the generalisation.
> */
> - if ((mask & smrs[i].mask) == mask &&
> - !((id ^ smrs[i].id) & ~smrs[i].mask))
> +
> + if (smr_is_subset(smrs[i], id, mask))
> return i;
> +
> /*
> * If the new entry has any other overlap with an existing one,
> * though, then there always exists at least one stream ID
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.h b/drivers/iommu/arm/arm-smmu/arm-smmu.h
> index 703fd5817ec1..b1638bbc41d4 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.h
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.h
> @@ -501,6 +501,11 @@ static inline void arm_smmu_writeq(struct arm_smmu_device *smmu, int page,
> writeq_relaxed(val, arm_smmu_page(smmu, page) + offset);
> }
>
> +static inline bool smr_is_subset(struct arm_smmu_smr smrs, u16 id, u16 mask)
> +{
> + return (mask & smrs.mask) == mask && !((id ^ smrs.id) & ~smrs.mask);
> +}
> +
> #define ARM_SMMU_GR0 0
> #define ARM_SMMU_GR1 1
> #define ARM_SMMU_CB(s, n) ((s)->numpage + (n))
> --
> 2.17.1
>
--
With best wishes
Dmitry
On 11/4/2023 3:33 AM, Dmitry Baryshkov wrote:
> On Fri, 3 Nov 2023 at 23:53, Bibek Kumar Patro
> <quic_bibekkum@quicinc.com> wrote:
>>
>> Currently in Qualcomm SoCs the default prefetch is set to 1 which allows
>> the TLB to fetch just the next page table. MMU-500 features ACTLR
>> register which is implementation defined and is used for Qualcomm SoCs
>> to have a prefetch setting of 1/3/7/15 enabling TLB to prefetch
>> the next set of page tables accordingly allowing for faster translations.
>>
>> ACTLR value is unique for each SMR (Stream matching register) and stored
>> in a pre-populated table. This value is set to the register during
>> context bank initialisation.
>>
>> Signed-off-by: Bibek Kumar Patro <quic_bibekkum@quicinc.com>
>> ---
>> drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 34 ++++++++++++++++++++++
>> drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h | 2 ++
>> drivers/iommu/arm/arm-smmu/arm-smmu.c | 5 ++--
>> drivers/iommu/arm/arm-smmu/arm-smmu.h | 5 ++++
>> 4 files changed, 44 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>> index ae7cae015193..68c1f4908473 100644
>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>> @@ -14,6 +14,17 @@
>>
>> #define QCOM_DUMMY_VAL -1
>>
>> +struct actlr_config {
>> + const struct actlr_data *adata;
>> + u32 size;
>
> This should be size_t.
>
> Also could you please drop the separate struct actlr_config and move
> these two fields into struct qcom_smmu_config.
>
Ack, will address both these inputs in the next patch.
>> +};
>> +
>> +struct actlr_data {
>> + u16 sid;
>> + u16 mask;
>> + u32 actlr;
>> +};
>> +
>> static struct qcom_smmu *to_qcom_smmu(struct arm_smmu_device *smmu)
>> {
>> return container_of(smmu, struct qcom_smmu, smmu);
>> @@ -270,6 +281,26 @@ static const struct of_device_id qcom_smmu_client_of_match[] __maybe_unused = {
>> static int qcom_smmu_init_context(struct arm_smmu_domain *smmu_domain,
>> struct io_pgtable_cfg *pgtbl_cfg, struct device *dev)
>> {
>> + struct arm_smmu_device *smmu = smmu_domain->smmu;
>> + struct qcom_smmu *qsmmu = to_qcom_smmu(smmu);
>> + const struct actlr_config *actlrcfg;
>> + struct arm_smmu_smr *smr = smmu->smrs;
>> + int idx = smmu_domain->cfg.cbndx;
>> + int i;
>> + u16 id;
>> + u16 mask;
>> +
>> + if (qsmmu->actlrcfg) {
>> + actlrcfg = qsmmu->actlrcfg;
>> + for (i = 0; i < actlrcfg->size; ++i) {
>> + id = actlrcfg->adata[i].sid;
>> + mask = actlrcfg->adata[i].mask;
>> + if (!smr_is_subset(*smr, id, mask))
>> + arm_smmu_cb_write(smmu, idx, ARM_SMMU_CB_ACTLR,
>> + actlrcfg->adata[i].actlr);
>> + }
>> + }
>
> Consider extracting this to a separate function. This way you can
> reduce 4 indentation levels into a single loop.
>
Ack, thanks for this sugestion. Will move this entire for loop into a
separate function for simplicity reduced indent levels.
>> +
>> smmu_domain->cfg.flush_walk_prefer_tlbiasid = true;
>>
>> return 0;
>> @@ -459,6 +490,9 @@ static struct arm_smmu_device *qcom_smmu_create(struct arm_smmu_device *smmu,
>> qsmmu->smmu.impl = impl;
>> qsmmu->cfg = data->cfg;
>>
>> + if (data->actlrcfg && (data->actlrcfg->size))
>> + qsmmu->actlrcfg = data->actlrcfg;
>> +
>> return &qsmmu->smmu;
>> }
>>
>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h
>> index 593910567b88..4b6862715070 100644
>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h
>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h
>> @@ -9,6 +9,7 @@
>> struct qcom_smmu {
>> struct arm_smmu_device smmu;
>> const struct qcom_smmu_config *cfg;
>> + const struct actlr_config *actlrcfg;
>> bool bypass_quirk;
>> u8 bypass_cbndx;
>> u32 stall_enabled;
>> @@ -25,6 +26,7 @@ struct qcom_smmu_config {
>> };
>>
>> struct qcom_smmu_match_data {
>> + const struct actlr_config *actlrcfg;
>> const struct qcom_smmu_config *cfg;
>> const struct arm_smmu_impl *impl;
>> const struct arm_smmu_impl *adreno_impl;
>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
>> index 4c79ef6f4c75..38ac1cbc799b 100644
>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
>> @@ -992,9 +992,10 @@ static int arm_smmu_find_sme(struct arm_smmu_device *smmu, u16 id, u16 mask)
>> * expect simply identical entries for this case, but there's
>> * no harm in accommodating the generalisation.
>> */
>> - if ((mask & smrs[i].mask) == mask &&
>> - !((id ^ smrs[i].id) & ~smrs[i].mask))
>> +
>> + if (smr_is_subset(smrs[i], id, mask))
>> return i;
>> +
>> /*
>> * If the new entry has any other overlap with an existing one,
>> * though, then there always exists at least one stream ID
>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.h b/drivers/iommu/arm/arm-smmu/arm-smmu.h
>> index 703fd5817ec1..b1638bbc41d4 100644
>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.h
>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.h
>> @@ -501,6 +501,11 @@ static inline void arm_smmu_writeq(struct arm_smmu_device *smmu, int page,
>> writeq_relaxed(val, arm_smmu_page(smmu, page) + offset);
>> }
>>
>> +static inline bool smr_is_subset(struct arm_smmu_smr smrs, u16 id, u16 mask)
>> +{
>> + return (mask & smrs.mask) == mask && !((id ^ smrs.id) & ~smrs.mask);
>> +}
>> +
>> #define ARM_SMMU_GR0 0
>> #define ARM_SMMU_GR1 1
>> #define ARM_SMMU_CB(s, n) ((s)->numpage + (n))
>> --
>> 2.17.1
>>
>
>
On 11/6/2023 11:42 AM, Bibek Kumar Patro wrote:
>
>
> On 11/4/2023 3:33 AM, Dmitry Baryshkov wrote:
>> On Fri, 3 Nov 2023 at 23:53, Bibek Kumar Patro
>> <quic_bibekkum@quicinc.com> wrote:
>>>
>>> Currently in Qualcomm SoCs the default prefetch is set to 1 which
>>> allows
>>> the TLB to fetch just the next page table. MMU-500 features ACTLR
>>> register which is implementation defined and is used for Qualcomm SoCs
>>> to have a prefetch setting of 1/3/7/15 enabling TLB to prefetch
>>> the next set of page tables accordingly allowing for faster
>>> translations.
>>>
>>> ACTLR value is unique for each SMR (Stream matching register) and stored
>>> in a pre-populated table. This value is set to the register during
>>> context bank initialisation.
>>>
>>> Signed-off-by: Bibek Kumar Patro <quic_bibekkum@quicinc.com>
>>> ---
>>> drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 34 ++++++++++++++++++++++
>>> drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h | 2 ++
>>> drivers/iommu/arm/arm-smmu/arm-smmu.c | 5 ++--
>>> drivers/iommu/arm/arm-smmu/arm-smmu.h | 5 ++++
>>> 4 files changed, 44 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>> b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>> index ae7cae015193..68c1f4908473 100644
>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>> @@ -14,6 +14,17 @@
>>>
>>> #define QCOM_DUMMY_VAL -1
>>>
>>> +struct actlr_config {
>>> + const struct actlr_data *adata;
>>> + u32 size;
>>
>> This should be size_t.
>>
>> Also could you please drop the separate struct actlr_config and move
>> these two fields into struct qcom_smmu_config.
>>
>
> Ack, will address both these inputs in the next patch.
>
Dimitry, Tried moving both fields to qcom_smmu_config but since
actlr_data need to be a pointer to array and not scalar, size needs
to be calculated dynamically for each SoC data in a loop which is
doable.But readily available implementations like ARRAY_SIZE cannot be
used, so I think this extra struct indirection of actlr_config would be
beneficial.
Some drivers like llcc (drivers/soc/qcom/llcc-qcom.c) is also using
similar implementation, I believe for the same reason.
regards,
Bibek
>>> +};
>>> +
>>> +struct actlr_data {
>>> + u16 sid;
>>> + u16 mask;
>>> + u32 actlr;
>>> +};
>>> +
>>> static struct qcom_smmu *to_qcom_smmu(struct arm_smmu_device *smmu)
>>> {
>>> return container_of(smmu, struct qcom_smmu, smmu);
>>> @@ -270,6 +281,26 @@ static const struct of_device_id
>>> qcom_smmu_client_of_match[] __maybe_unused = {
>>> static int qcom_smmu_init_context(struct arm_smmu_domain *smmu_domain,
>>> struct io_pgtable_cfg *pgtbl_cfg, struct device *dev)
>>> {
>>> + struct arm_smmu_device *smmu = smmu_domain->smmu;
>>> + struct qcom_smmu *qsmmu = to_qcom_smmu(smmu);
>>> + const struct actlr_config *actlrcfg;
>>> + struct arm_smmu_smr *smr = smmu->smrs;
>>> + int idx = smmu_domain->cfg.cbndx;
>>> + int i;
>>> + u16 id;
>>> + u16 mask;
>>> +
>>> + if (qsmmu->actlrcfg) {
>>> + actlrcfg = qsmmu->actlrcfg;
>>> + for (i = 0; i < actlrcfg->size; ++i) {
>>> + id = actlrcfg->adata[i].sid;
>>> + mask = actlrcfg->adata[i].mask;
>>> + if (!smr_is_subset(*smr, id, mask))
>>> + arm_smmu_cb_write(smmu, idx,
>>> ARM_SMMU_CB_ACTLR,
>>> +
>>> actlrcfg->adata[i].actlr);
>>> + }
>>> + }
>>
>> Consider extracting this to a separate function. This way you can
>> reduce 4 indentation levels into a single loop.
>>
>
> Ack, thanks for this sugestion. Will move this entire for loop into a
> separate function for simplicity reduced indent levels.
>
>>> +
>>> smmu_domain->cfg.flush_walk_prefer_tlbiasid = true;
>>>
>>> return 0;
>>> @@ -459,6 +490,9 @@ static struct arm_smmu_device
>>> *qcom_smmu_create(struct arm_smmu_device *smmu,
>>> qsmmu->smmu.impl = impl;
>>> qsmmu->cfg = data->cfg;
>>>
>>> + if (data->actlrcfg && (data->actlrcfg->size))
>>> + qsmmu->actlrcfg = data->actlrcfg;
>>> +
>>> return &qsmmu->smmu;
>>> }
>>>
>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h
>>> b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h
>>> index 593910567b88..4b6862715070 100644
>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h
>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h
>>> @@ -9,6 +9,7 @@
>>> struct qcom_smmu {
>>> struct arm_smmu_device smmu;
>>> const struct qcom_smmu_config *cfg;
>>> + const struct actlr_config *actlrcfg;
>>> bool bypass_quirk;
>>> u8 bypass_cbndx;
>>> u32 stall_enabled;
>>> @@ -25,6 +26,7 @@ struct qcom_smmu_config {
>>> };
>>>
>>> struct qcom_smmu_match_data {
>>> + const struct actlr_config *actlrcfg;
>>> const struct qcom_smmu_config *cfg;
>>> const struct arm_smmu_impl *impl;
>>> const struct arm_smmu_impl *adreno_impl;
>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c
>>> b/drivers/iommu/arm/arm-smmu/arm-smmu.c
>>> index 4c79ef6f4c75..38ac1cbc799b 100644
>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
>>> @@ -992,9 +992,10 @@ static int arm_smmu_find_sme(struct
>>> arm_smmu_device *smmu, u16 id, u16 mask)
>>> * expect simply identical entries for this case,
>>> but there's
>>> * no harm in accommodating the generalisation.
>>> */
>>> - if ((mask & smrs[i].mask) == mask &&
>>> - !((id ^ smrs[i].id) & ~smrs[i].mask))
>>> +
>>> + if (smr_is_subset(smrs[i], id, mask))
>>> return i;
>>> +
>>> /*
>>> * If the new entry has any other overlap with an
>>> existing one,
>>> * though, then there always exists at least one
>>> stream ID
>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.h
>>> b/drivers/iommu/arm/arm-smmu/arm-smmu.h
>>> index 703fd5817ec1..b1638bbc41d4 100644
>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.h
>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.h
>>> @@ -501,6 +501,11 @@ static inline void arm_smmu_writeq(struct
>>> arm_smmu_device *smmu, int page,
>>> writeq_relaxed(val, arm_smmu_page(smmu, page) +
>>> offset);
>>> }
>>>
>>> +static inline bool smr_is_subset(struct arm_smmu_smr smrs, u16 id,
>>> u16 mask)
>>> +{
>>> + return (mask & smrs.mask) == mask && !((id ^ smrs.id) &
>>> ~smrs.mask);
>>> +}
>>> +
>>> #define ARM_SMMU_GR0 0
>>> #define ARM_SMMU_GR1 1
>>> #define ARM_SMMU_CB(s, n) ((s)->numpage + (n))
>>> --
>>> 2.17.1
>>>
>>
>>
On Tue, 14 Nov 2023 at 12:20, Bibek Kumar Patro
<quic_bibekkum@quicinc.com> wrote:
>
>
>
> On 11/6/2023 11:42 AM, Bibek Kumar Patro wrote:
> >
> >
> > On 11/4/2023 3:33 AM, Dmitry Baryshkov wrote:
> >> On Fri, 3 Nov 2023 at 23:53, Bibek Kumar Patro
> >> <quic_bibekkum@quicinc.com> wrote:
> >>>
> >>> Currently in Qualcomm SoCs the default prefetch is set to 1 which
> >>> allows
> >>> the TLB to fetch just the next page table. MMU-500 features ACTLR
> >>> register which is implementation defined and is used for Qualcomm SoCs
> >>> to have a prefetch setting of 1/3/7/15 enabling TLB to prefetch
> >>> the next set of page tables accordingly allowing for faster
> >>> translations.
> >>>
> >>> ACTLR value is unique for each SMR (Stream matching register) and stored
> >>> in a pre-populated table. This value is set to the register during
> >>> context bank initialisation.
> >>>
> >>> Signed-off-by: Bibek Kumar Patro <quic_bibekkum@quicinc.com>
> >>> ---
> >>> drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 34 ++++++++++++++++++++++
> >>> drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h | 2 ++
> >>> drivers/iommu/arm/arm-smmu/arm-smmu.c | 5 ++--
> >>> drivers/iommu/arm/arm-smmu/arm-smmu.h | 5 ++++
> >>> 4 files changed, 44 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> >>> b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> >>> index ae7cae015193..68c1f4908473 100644
> >>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> >>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> >>> @@ -14,6 +14,17 @@
> >>>
> >>> #define QCOM_DUMMY_VAL -1
> >>>
> >>> +struct actlr_config {
> >>> + const struct actlr_data *adata;
> >>> + u32 size;
> >>
> >> This should be size_t.
> >>
> >> Also could you please drop the separate struct actlr_config and move
> >> these two fields into struct qcom_smmu_config.
> >>
> >
> > Ack, will address both these inputs in the next patch.
> >
>
> Dimitry, Tried moving both fields to qcom_smmu_config but since
> actlr_data need to be a pointer to array and not scalar, size needs
> to be calculated dynamically for each SoC data in a loop which is
> doable.But readily available implementations like ARRAY_SIZE cannot be
> used, so I think this extra struct indirection of actlr_config would be
> beneficial.
This should work fine from my point of view:
static const struct qcom_smmu_match_data sm8550_smmu_500_impl0_data = {
.impl = &sm8550_smmu_500_impl,
.adreno_impl = &qcom_adreno_smmu_500_impl,
.cfg = &qcom_smmu_impl0_cfg,
.actlrcfg = &sm8550_apps_actlr_data,
.actlrcfg_size = ARRAY_SIZE(sm8550_apps_actlr_data),
};
> Some drivers like llcc (drivers/soc/qcom/llcc-qcom.c) is also using
> similar implementation, I believe for the same reason.
>
> regards,
> Bibek
> >>> +};
> >>> +
> >>> +struct actlr_data {
> >>> + u16 sid;
> >>> + u16 mask;
> >>> + u32 actlr;
> >>> +};
> >>> +
> >>> static struct qcom_smmu *to_qcom_smmu(struct arm_smmu_device *smmu)
> >>> {
> >>> return container_of(smmu, struct qcom_smmu, smmu);
> >>> @@ -270,6 +281,26 @@ static const struct of_device_id
> >>> qcom_smmu_client_of_match[] __maybe_unused = {
> >>> static int qcom_smmu_init_context(struct arm_smmu_domain *smmu_domain,
> >>> struct io_pgtable_cfg *pgtbl_cfg, struct device *dev)
> >>> {
> >>> + struct arm_smmu_device *smmu = smmu_domain->smmu;
> >>> + struct qcom_smmu *qsmmu = to_qcom_smmu(smmu);
> >>> + const struct actlr_config *actlrcfg;
> >>> + struct arm_smmu_smr *smr = smmu->smrs;
> >>> + int idx = smmu_domain->cfg.cbndx;
> >>> + int i;
> >>> + u16 id;
> >>> + u16 mask;
> >>> +
> >>> + if (qsmmu->actlrcfg) {
> >>> + actlrcfg = qsmmu->actlrcfg;
> >>> + for (i = 0; i < actlrcfg->size; ++i) {
> >>> + id = actlrcfg->adata[i].sid;
> >>> + mask = actlrcfg->adata[i].mask;
> >>> + if (!smr_is_subset(*smr, id, mask))
> >>> + arm_smmu_cb_write(smmu, idx,
> >>> ARM_SMMU_CB_ACTLR,
> >>> +
> >>> actlrcfg->adata[i].actlr);
> >>> + }
> >>> + }
> >>
> >> Consider extracting this to a separate function. This way you can
> >> reduce 4 indentation levels into a single loop.
> >>
> >
> > Ack, thanks for this sugestion. Will move this entire for loop into a
> > separate function for simplicity reduced indent levels.
> >
> >>> +
> >>> smmu_domain->cfg.flush_walk_prefer_tlbiasid = true;
> >>>
> >>> return 0;
> >>> @@ -459,6 +490,9 @@ static struct arm_smmu_device
> >>> *qcom_smmu_create(struct arm_smmu_device *smmu,
> >>> qsmmu->smmu.impl = impl;
> >>> qsmmu->cfg = data->cfg;
> >>>
> >>> + if (data->actlrcfg && (data->actlrcfg->size))
> >>> + qsmmu->actlrcfg = data->actlrcfg;
> >>> +
> >>> return &qsmmu->smmu;
> >>> }
> >>>
> >>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h
> >>> b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h
> >>> index 593910567b88..4b6862715070 100644
> >>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h
> >>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h
> >>> @@ -9,6 +9,7 @@
> >>> struct qcom_smmu {
> >>> struct arm_smmu_device smmu;
> >>> const struct qcom_smmu_config *cfg;
> >>> + const struct actlr_config *actlrcfg;
> >>> bool bypass_quirk;
> >>> u8 bypass_cbndx;
> >>> u32 stall_enabled;
> >>> @@ -25,6 +26,7 @@ struct qcom_smmu_config {
> >>> };
> >>>
> >>> struct qcom_smmu_match_data {
> >>> + const struct actlr_config *actlrcfg;
> >>> const struct qcom_smmu_config *cfg;
> >>> const struct arm_smmu_impl *impl;
> >>> const struct arm_smmu_impl *adreno_impl;
> >>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> >>> b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> >>> index 4c79ef6f4c75..38ac1cbc799b 100644
> >>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> >>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> >>> @@ -992,9 +992,10 @@ static int arm_smmu_find_sme(struct
> >>> arm_smmu_device *smmu, u16 id, u16 mask)
> >>> * expect simply identical entries for this case,
> >>> but there's
> >>> * no harm in accommodating the generalisation.
> >>> */
> >>> - if ((mask & smrs[i].mask) == mask &&
> >>> - !((id ^ smrs[i].id) & ~smrs[i].mask))
> >>> +
> >>> + if (smr_is_subset(smrs[i], id, mask))
> >>> return i;
> >>> +
> >>> /*
> >>> * If the new entry has any other overlap with an
> >>> existing one,
> >>> * though, then there always exists at least one
> >>> stream ID
> >>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.h
> >>> b/drivers/iommu/arm/arm-smmu/arm-smmu.h
> >>> index 703fd5817ec1..b1638bbc41d4 100644
> >>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.h
> >>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.h
> >>> @@ -501,6 +501,11 @@ static inline void arm_smmu_writeq(struct
> >>> arm_smmu_device *smmu, int page,
> >>> writeq_relaxed(val, arm_smmu_page(smmu, page) +
> >>> offset);
> >>> }
> >>>
> >>> +static inline bool smr_is_subset(struct arm_smmu_smr smrs, u16 id,
> >>> u16 mask)
> >>> +{
> >>> + return (mask & smrs.mask) == mask && !((id ^ smrs.id) &
> >>> ~smrs.mask);
> >>> +}
> >>> +
> >>> #define ARM_SMMU_GR0 0
> >>> #define ARM_SMMU_GR1 1
> >>> #define ARM_SMMU_CB(s, n) ((s)->numpage + (n))
> >>> --
> >>> 2.17.1
> >>>
> >>
> >>
--
With best wishes
Dmitry
© 2016 - 2025 Red Hat, Inc.