| Message ID | 20231127145412.3981-1-quic_bibekkum@quicinc.com |
|---|---|
| Headers | show |
| Series | iommu/arm-smmu: introduction of ACTLR implementation for Qualcomm SoCs | expand |
On Mon, Nov 27, 2023 at 08:24:09PM +0530, Bibek Kumar Patro 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 In the previous discussion with Dmitry you stated that the "prefetch setting" doesn't directly map to any known values. This commit message give a clear indication about the meaning of these values. So, please fix the commit message to properly document the value space - to avoid confusion when people are searching for the meaning of the defines... Please also clarify why there are 4 possible values here, 4 possible values of the 2 prefetch settings bits in the register, but only 3 defines in the actual patch. Regards, Bjorn > 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 | 56 +++++++++++++++++++++- > 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, 68 insertions(+), 4 deletions(-) > > diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c > index 7f52ac67495f..4a38cae29be2 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,10 +211,40 @@ static bool qcom_adreno_can_do_ttbr1(struct arm_smmu_device *smmu) > return true; > } > > +static void arm_smmu_set_actlr(struct device *dev, struct arm_smmu_device *smmu, int cbndx, > + const struct actlr_config *actlrcfg, size_t actlrcfg_size) > +{ > + struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev); > + struct arm_smmu_master_cfg *cfg = dev_iommu_priv_get(dev); > + struct arm_smmu_smr *smr; > + int i; > + int j; > + u16 id; > + u16 mask; > + int idx; > + > + for (i = 0; i < actlrcfg_size; ++i) { > + id = (actlrcfg + i)->sid; > + mask = (actlrcfg + i)->mask; > + > + for_each_cfg_sme(cfg, fwspec, j, idx) { > + smr = &smmu->smrs[idx]; > + if (smr_is_subset(*smr, id, mask)) > + arm_smmu_cb_write(smmu, cbndx, ARM_SMMU_CB_ACTLR, > + (actlrcfg + i)->actlr); > + } > + } > +} > + > static int qcom_adreno_smmu_init_context(struct arm_smmu_domain *smmu_domain, > struct io_pgtable_cfg *pgtbl_cfg, struct device *dev) > { > struct adreno_smmu_priv *priv; > + struct arm_smmu_device *smmu = smmu_domain->smmu; > + struct qcom_smmu *qsmmu = to_qcom_smmu(smmu); > + const struct actlr_config *actlrcfg; > + size_t actlrcfg_size; > + int cbndx = smmu_domain->cfg.cbndx; > > smmu_domain->cfg.flush_walk_prefer_tlbiasid = true; > > @@ -238,6 +274,12 @@ 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) { > + actlrcfg = qsmmu->data->actlrcfg_gfx; > + actlrcfg_size = qsmmu->data->actlrcfg_gfx_size; > + arm_smmu_set_actlr(dev, smmu, cbndx, actlrcfg, actlrcfg_size); > + } > + > return 0; > } > > @@ -263,6 +305,18 @@ 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; > + size_t actlrcfg_size; > + int cbndx = smmu_domain->cfg.cbndx; > + > + if (qsmmu->data->actlrcfg) { > + actlrcfg = qsmmu->data->actlrcfg; > + actlrcfg_size = qsmmu->data->actlrcfg_size; > + arm_smmu_set_actlr(dev, smmu, cbndx, actlrcfg, actlrcfg_size); > + } > + > smmu_domain->cfg.flush_walk_prefer_tlbiasid = true; > > return 0; > @@ -464,7 +518,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..138fc57f7b0d 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; > @@ -25,6 +25,10 @@ struct qcom_smmu_config { > }; > > struct qcom_smmu_match_data { > + const struct actlr_config *actlrcfg; > + size_t actlrcfg_size; > + const struct actlr_config *actlrcfg_gfx; > + size_t actlrcfg_gfx_size; > 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..8e4faf015286 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..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/27/2023 9:05 PM, Konrad Dybcio wrote: > On 27.11.2023 15:54, Bibek Kumar Patro wrote: >> 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 247eaa194129..f0ad09f9a974 100644 >> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >> @@ -27,6 +27,20 @@ struct actlr_config { >> #define CPRE BIT(1) /* Enable context caching in the prefetch buffer */ >> #define CMTLB BIT(0) /* Enable context caching in the macro TLB */ >> >> +static const struct actlr_config sc7280_apps_actlr_cfg[] = { >> + { 0x0800, 0x24E1, PREFETCH_DISABLE | CMTLB }, > hex should be lowercase Noted,thanks for pointing this out will take care of this in next version. > >> + { 0x2000, 0x0163, PREFETCH_DISABLE | CMTLB }, >> + { 0x2080, 0x0461, PREFETCH_DISABLE | CMTLB }, >> + { 0x2100, 0x0161, PREFETCH_DISABLE | CMTLB }, >> + { 0x0900, 0x0407, PREFETCH_SHALLOW | CPRE | CMTLB }, >> + { 0x2180, 0x0027, PREFETCH_SHALLOW | CPRE | CMTLB }, >> + { 0x1000, 0x07ff, PREFETCH_DEEP | CPRE | CMTLB }, >> +}; > Any reason this list is so much smaller than 8550's? Is it complete? Yes it's complete only. This list varies targetwise actually so we just fill it referring the hardware settings reference document. So size of the list might vary as per target. > > Konrad Thanks & regards, Bibek
On 11/27/2023 9:37 PM, Dmitry Baryshkov wrote: > On Mon, 27 Nov 2023 at 16:54, 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 | 56 +++++++++++++++++++++- >> 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, 68 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >> index 7f52ac67495f..4a38cae29be2 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,10 +211,40 @@ static bool qcom_adreno_can_do_ttbr1(struct arm_smmu_device *smmu) >> return true; >> } >> >> +static void arm_smmu_set_actlr(struct device *dev, struct arm_smmu_device *smmu, int cbndx, >> + const struct actlr_config *actlrcfg, size_t actlrcfg_size) >> +{ >> + struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev); >> + struct arm_smmu_master_cfg *cfg = dev_iommu_priv_get(dev); >> + struct arm_smmu_smr *smr; >> + int i; >> + int j; >> + u16 id; >> + u16 mask; >> + int idx; >> + >> + for (i = 0; i < actlrcfg_size; ++i) { >> + id = (actlrcfg + i)->sid; >> + mask = (actlrcfg + i)->mask; >> + >> + for_each_cfg_sme(cfg, fwspec, j, idx) { >> + smr = &smmu->smrs[idx]; >> + if (smr_is_subset(*smr, id, mask)) >> + arm_smmu_cb_write(smmu, cbndx, ARM_SMMU_CB_ACTLR, >> + (actlrcfg + i)->actlr); >> + } >> + } >> +} >> + >> static int qcom_adreno_smmu_init_context(struct arm_smmu_domain *smmu_domain, >> struct io_pgtable_cfg *pgtbl_cfg, struct device *dev) >> { >> struct adreno_smmu_priv *priv; >> + struct arm_smmu_device *smmu = smmu_domain->smmu; >> + struct qcom_smmu *qsmmu = to_qcom_smmu(smmu); >> + const struct actlr_config *actlrcfg; >> + size_t actlrcfg_size; >> + int cbndx = smmu_domain->cfg.cbndx; >> >> smmu_domain->cfg.flush_walk_prefer_tlbiasid = true; >> >> @@ -238,6 +274,12 @@ 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) { >> + actlrcfg = qsmmu->data->actlrcfg_gfx; >> + actlrcfg_size = qsmmu->data->actlrcfg_gfx_size; >> + arm_smmu_set_actlr(dev, smmu, cbndx, actlrcfg, actlrcfg_size); >> + } >> + >> return 0; >> } >> >> @@ -263,6 +305,18 @@ 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; >> + size_t actlrcfg_size; >> + int cbndx = smmu_domain->cfg.cbndx; >> + >> + if (qsmmu->data->actlrcfg) { >> + actlrcfg = qsmmu->data->actlrcfg; >> + actlrcfg_size = qsmmu->data->actlrcfg_size; >> + arm_smmu_set_actlr(dev, smmu, cbndx, actlrcfg, actlrcfg_size); >> + } >> + >> smmu_domain->cfg.flush_walk_prefer_tlbiasid = true; >> >> return 0; >> @@ -464,7 +518,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; > > This should go to a separate commit. It is not related to ACTLR support > >> >> 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..138fc57f7b0d 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; >> @@ -25,6 +25,10 @@ struct qcom_smmu_config { >> }; >> >> struct qcom_smmu_match_data { >> + const struct actlr_config *actlrcfg; >> + size_t actlrcfg_size; >> + const struct actlr_config *actlrcfg_gfx; >> + size_t actlrcfg_gfx_size; >> 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..8e4faf015286 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..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) > > A pointer to the struct, please Noted, will evaluate address this in next revision. Thanks & Regards, Bibek > >> +{ >> + 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/28/2023 8:31 AM, Bjorn Andersson wrote: > On Mon, Nov 27, 2023 at 08:24:09PM +0530, Bibek Kumar Patro 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 > > In the previous discussion with Dmitry you stated that the "prefetch > setting" doesn't directly map to any known values. This commit message > give a clear indication about the meaning of these values. > > So, please fix the commit message to properly document the value space - > to avoid confusion when people are searching for the meaning of the > defines... > Noted, agree on the same. Thanks for pointing this out. Will fix the description accordingly, avoid mentioning meaning of these values. > > Please also clarify why there are 4 possible values here, 4 possible > values of the 2 prefetch settings bits in the register, but only 3 > defines in the actual patch. > One of the values haven't been yet used in the targets whose list are posted in this series, hence corresponding define is not mentioned in the actual patch yet. Thanks & Regards, Bibek > Regards, > Bjorn > >> 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 | 56 +++++++++++++++++++++- >> 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, 68 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >> index 7f52ac67495f..4a38cae29be2 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,10 +211,40 @@ static bool qcom_adreno_can_do_ttbr1(struct arm_smmu_device *smmu) >> return true; >> } >> >> +static void arm_smmu_set_actlr(struct device *dev, struct arm_smmu_device *smmu, int cbndx, >> + const struct actlr_config *actlrcfg, size_t actlrcfg_size) >> +{ >> + struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev); >> + struct arm_smmu_master_cfg *cfg = dev_iommu_priv_get(dev); >> + struct arm_smmu_smr *smr; >> + int i; >> + int j; >> + u16 id; >> + u16 mask; >> + int idx; >> + >> + for (i = 0; i < actlrcfg_size; ++i) { >> + id = (actlrcfg + i)->sid; >> + mask = (actlrcfg + i)->mask; >> + >> + for_each_cfg_sme(cfg, fwspec, j, idx) { >> + smr = &smmu->smrs[idx]; >> + if (smr_is_subset(*smr, id, mask)) >> + arm_smmu_cb_write(smmu, cbndx, ARM_SMMU_CB_ACTLR, >> + (actlrcfg + i)->actlr); >> + } >> + } >> +} >> + >> static int qcom_adreno_smmu_init_context(struct arm_smmu_domain *smmu_domain, >> struct io_pgtable_cfg *pgtbl_cfg, struct device *dev) >> { >> struct adreno_smmu_priv *priv; >> + struct arm_smmu_device *smmu = smmu_domain->smmu; >> + struct qcom_smmu *qsmmu = to_qcom_smmu(smmu); >> + const struct actlr_config *actlrcfg; >> + size_t actlrcfg_size; >> + int cbndx = smmu_domain->cfg.cbndx; >> >> smmu_domain->cfg.flush_walk_prefer_tlbiasid = true; >> >> @@ -238,6 +274,12 @@ 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) { >> + actlrcfg = qsmmu->data->actlrcfg_gfx; >> + actlrcfg_size = qsmmu->data->actlrcfg_gfx_size; >> + arm_smmu_set_actlr(dev, smmu, cbndx, actlrcfg, actlrcfg_size); >> + } >> + >> return 0; >> } >> >> @@ -263,6 +305,18 @@ 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; >> + size_t actlrcfg_size; >> + int cbndx = smmu_domain->cfg.cbndx; >> + >> + if (qsmmu->data->actlrcfg) { >> + actlrcfg = qsmmu->data->actlrcfg; >> + actlrcfg_size = qsmmu->data->actlrcfg_size; >> + arm_smmu_set_actlr(dev, smmu, cbndx, actlrcfg, actlrcfg_size); >> + } >> + >> smmu_domain->cfg.flush_walk_prefer_tlbiasid = true; >> >> return 0; >> @@ -464,7 +518,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..138fc57f7b0d 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; >> @@ -25,6 +25,10 @@ struct qcom_smmu_config { >> }; >> >> struct qcom_smmu_match_data { >> + const struct actlr_config *actlrcfg; >> + size_t actlrcfg_size; >> + const struct actlr_config *actlrcfg_gfx; >> + size_t actlrcfg_gfx_size; >> 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..8e4faf015286 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..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/27/2023 9:03 PM, Konrad Dybcio wrote: > On 27.11.2023 15:54, Bibek Kumar Patro 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 | 56 +++++++++++++++++++++- >> 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, 68 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >> index 7f52ac67495f..4a38cae29be2 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,10 +211,40 @@ static bool qcom_adreno_can_do_ttbr1(struct arm_smmu_device *smmu) >> return true; >> } >> >> +static void arm_smmu_set_actlr(struct device *dev, struct arm_smmu_device *smmu, int cbndx, >> + const struct actlr_config *actlrcfg, size_t actlrcfg_size) >> +{ >> + struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev); >> + struct arm_smmu_master_cfg *cfg = dev_iommu_priv_get(dev); >> + struct arm_smmu_smr *smr; >> + int i; >> + int j; >> + u16 id; >> + u16 mask; >> + int idx; >> + >> + for (i = 0; i < actlrcfg_size; ++i) { >> + id = (actlrcfg + i)->sid; >> + mask = (actlrcfg + i)->mask; > actrlcfg[i].id? > Noted, array indexing instead of incrementing the base address should also work. >> + >> + for_each_cfg_sme(cfg, fwspec, j, idx) { >> + smr = &smmu->smrs[idx]; >> + if (smr_is_subset(*smr, id, mask)) > Any reason for this value to be a pointer? > >> + arm_smmu_cb_write(smmu, cbndx, ARM_SMMU_CB_ACTLR, >> + (actlrcfg + i)->actlr); > ditto > Noted >> + } >> + } >> +} >> + >> static int qcom_adreno_smmu_init_context(struct arm_smmu_domain *smmu_domain, >> struct io_pgtable_cfg *pgtbl_cfg, struct device *dev) >> { >> struct adreno_smmu_priv *priv; >> + struct arm_smmu_device *smmu = smmu_domain->smmu; >> + struct qcom_smmu *qsmmu = to_qcom_smmu(smmu); >> + const struct actlr_config *actlrcfg; >> + size_t actlrcfg_size; >> + int cbndx = smmu_domain->cfg.cbndx; > Reverse-Christmas-tree sorting, please > Noted, thanks for pointing this, I will take care of this in next revision. >> >> smmu_domain->cfg.flush_walk_prefer_tlbiasid = true; >> >> @@ -238,6 +274,12 @@ 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) { >> + actlrcfg = qsmmu->data->actlrcfg_gfx; >> + actlrcfg_size = qsmmu->data->actlrcfg_gfx_size; > These can be passed directly s arm_smmu_set_actrl arguments > Noted, will address in next revision. since there won't be any issue during time of access, I can pass these values directly. >> + arm_smmu_set_actlr(dev, smmu, cbndx, actlrcfg, actlrcfg_size); >> + } >> + >> return 0; >> } >> >> @@ -263,6 +305,18 @@ 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; >> + size_t actlrcfg_size; >> + int cbndx = smmu_domain->cfg.cbndx; >> + >> + if (qsmmu->data->actlrcfg) { >> + actlrcfg = qsmmu->data->actlrcfg; >> + actlrcfg_size = qsmmu->data->actlrcfg_size; >> + arm_smmu_set_actlr(dev, smmu, cbndx, actlrcfg, actlrcfg_size); > ditto > Noted. > Konrad
On 11/27/2023 9:37 PM, Dmitry Baryshkov wrote: > On Mon, 27 Nov 2023 at 16:54, 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 | 56 +++++++++++++++++++++- >> 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, 68 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >> index 7f52ac67495f..4a38cae29be2 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,10 +211,40 @@ static bool qcom_adreno_can_do_ttbr1(struct arm_smmu_device *smmu) >> return true; >> } >> >> +static void arm_smmu_set_actlr(struct device *dev, struct arm_smmu_device *smmu, int cbndx, >> + const struct actlr_config *actlrcfg, size_t actlrcfg_size) >> +{ >> + struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev); >> + struct arm_smmu_master_cfg *cfg = dev_iommu_priv_get(dev); >> + struct arm_smmu_smr *smr; >> + int i; >> + int j; >> + u16 id; >> + u16 mask; >> + int idx; >> + >> + for (i = 0; i < actlrcfg_size; ++i) { >> + id = (actlrcfg + i)->sid; >> + mask = (actlrcfg + i)->mask; >> + >> + for_each_cfg_sme(cfg, fwspec, j, idx) { >> + smr = &smmu->smrs[idx]; >> + if (smr_is_subset(*smr, id, mask)) >> + arm_smmu_cb_write(smmu, cbndx, ARM_SMMU_CB_ACTLR, >> + (actlrcfg + i)->actlr); >> + } >> + } >> +} >> + >> static int qcom_adreno_smmu_init_context(struct arm_smmu_domain *smmu_domain, >> struct io_pgtable_cfg *pgtbl_cfg, struct device *dev) >> { >> struct adreno_smmu_priv *priv; >> + struct arm_smmu_device *smmu = smmu_domain->smmu; >> + struct qcom_smmu *qsmmu = to_qcom_smmu(smmu); >> + const struct actlr_config *actlrcfg; >> + size_t actlrcfg_size; >> + int cbndx = smmu_domain->cfg.cbndx; >> >> smmu_domain->cfg.flush_walk_prefer_tlbiasid = true; >> >> @@ -238,6 +274,12 @@ 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) { >> + actlrcfg = qsmmu->data->actlrcfg_gfx; >> + actlrcfg_size = qsmmu->data->actlrcfg_gfx_size; >> + arm_smmu_set_actlr(dev, smmu, cbndx, actlrcfg, actlrcfg_size); >> + } >> + >> return 0; >> } >> >> @@ -263,6 +305,18 @@ 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; >> + size_t actlrcfg_size; >> + int cbndx = smmu_domain->cfg.cbndx; >> + >> + if (qsmmu->data->actlrcfg) { >> + actlrcfg = qsmmu->data->actlrcfg; >> + actlrcfg_size = qsmmu->data->actlrcfg_size; >> + arm_smmu_set_actlr(dev, smmu, cbndx, actlrcfg, actlrcfg_size); >> + } >> + >> smmu_domain->cfg.flush_walk_prefer_tlbiasid = true; >> >> return 0; >> @@ -464,7 +518,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; > > This should go to a separate commit. It is not related to ACTLR support qsmmu->data has the actlrcfg/actlrcfg_gfx as well hence clubbed this change here as rightly suggested by Robin[1] on v2 revision. Initially planned[2] for separate patch, but later clubbing it with actlr patch looked like a cleaner approach. Would it be okay to keep it here? Or separate patch would be better? [1]:https://lore.kernel.org/all/c75d107a-44cb-4df3-b583-13719df1f8be@arm.com/ [2]:https://lore.kernel.org/all/9b406a7c-57b8-4b5f-8fbc-714560cce8cf@quicinc.com/ > >> >> 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..138fc57f7b0d 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; >> @@ -25,6 +25,10 @@ struct qcom_smmu_config { >> }; >> >> struct qcom_smmu_match_data { >> + const struct actlr_config *actlrcfg; >> + size_t actlrcfg_size; >> + const struct actlr_config *actlrcfg_gfx; >> + size_t actlrcfg_gfx_size; >> 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..8e4faf015286 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..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) > > A pointer to the struct, please > >> +{ >> + 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 >> > >