| Message ID | 20231215101827.30549-4-quic_bibekkum@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iommu/arm-smmu: introduction of ACTLR implementation for Qualcomm SoCs | expand |
On Fri, 15 Dec 2023 at 12:19, Bibek Kumar Patro <quic_bibekkum@quicinc.com> wrote: > > Add ACTLR data table for SM8550 along with support for > same including SM8550 specific implementation operations. > > Signed-off-by: Bibek Kumar Patro <quic_bibekkum@quicinc.com> > --- > drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 89 ++++++++++++++++++++++ > 1 file changed, 89 insertions(+) > > diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c > index cb49291f5233..d2006f610243 100644 > --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c > +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c > @@ -20,6 +20,85 @@ struct actlr_config { > u32 actlr; > }; > > +/* > + * SMMU-500 TRM defines BIT(0) as CMTLB (Enable context caching in the > + * macro TLB) and BIT(1) as CPRE (Enable context caching in the prefetch > + * buffer). The remaining bits are implementation defined and vary across > + * SoCs. > + */ > + > +#define PREFETCH_DEFAULT 0 > +#define PREFETCH_SHALLOW BIT(8) > +#define PREFETCH_MODERATE BIT(9) > +#define PREFETCH_DEEP (BIT(9) | BIT(8)) I thin the following might be more correct: #include <linux/bitfield.h> #define PREFETCH_MASK GENMASK(9, 8) #define PREFETCH_DEFAULT FIELD_PREP(PREFETCH_MASK, 0) #define PREFETCH_SHALLOW FIELD_PREP(PREFETCH_MASK, 1) #define PREFETCH_MODERATE FIELD_PREP(PREFETCH_MASK, 2) #define PREFETCH_DEEP FIELD_PREP(PREFETCH_MASK, 3) > +#define PREFETCH_SWITCH_GFX (BIT(5) | BIT(3)) > +#define CPRE BIT(1) > +#define CMTLB BIT(0) > + > +static const struct actlr_config sm8550_apps_actlr_cfg[] = { > + { 0x18a0, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB }, > + { 0x18e0, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB }, > + { 0x0800, 0x0020, PREFETCH_DEFAULT | CMTLB }, > + { 0x1800, 0x00c0, PREFETCH_DEFAULT | CMTLB }, > + { 0x1820, 0x0000, PREFETCH_DEFAULT | CMTLB }, > + { 0x1860, 0x0000, PREFETCH_DEFAULT | CMTLB }, > + { 0x0c01, 0x0020, PREFETCH_DEEP | CPRE | CMTLB }, > + { 0x0c02, 0x0020, PREFETCH_DEEP | CPRE | CMTLB }, > + { 0x0c03, 0x0020, PREFETCH_DEEP | CPRE | CMTLB }, > + { 0x0c04, 0x0020, PREFETCH_DEEP | CPRE | CMTLB }, > + { 0x0c05, 0x0020, PREFETCH_DEEP | CPRE | CMTLB }, > + { 0x0c06, 0x0020, PREFETCH_DEEP | CPRE | CMTLB }, > + { 0x0c07, 0x0020, PREFETCH_DEEP | CPRE | CMTLB }, > + { 0x0c08, 0x0020, PREFETCH_DEEP | CPRE | CMTLB }, > + { 0x0c09, 0x0020, PREFETCH_DEEP | CPRE | CMTLB }, > + { 0x0c0c, 0x0020, PREFETCH_DEEP | CPRE | CMTLB }, > + { 0x0c0d, 0x0020, PREFETCH_DEEP | CPRE | CMTLB }, > + { 0x0c0e, 0x0020, PREFETCH_DEEP | CPRE | CMTLB }, > + { 0x0c0f, 0x0020, PREFETCH_DEEP | CPRE | CMTLB }, > + { 0x1961, 0x0000, PREFETCH_DEEP | CPRE | CMTLB }, > + { 0x1962, 0x0000, PREFETCH_DEEP | CPRE | CMTLB }, > + { 0x1963, 0x0000, PREFETCH_DEEP | CPRE | CMTLB }, > + { 0x1964, 0x0000, PREFETCH_DEEP | CPRE | CMTLB }, > + { 0x1965, 0x0000, PREFETCH_DEEP | CPRE | CMTLB }, > + { 0x1966, 0x0000, PREFETCH_DEEP | CPRE | CMTLB }, > + { 0x1967, 0x0000, PREFETCH_DEEP | CPRE | CMTLB }, > + { 0x1968, 0x0000, PREFETCH_DEEP | CPRE | CMTLB }, > + { 0x1969, 0x0000, PREFETCH_DEEP | CPRE | CMTLB }, > + { 0x196c, 0x0000, PREFETCH_DEEP | CPRE | CMTLB }, > + { 0x196d, 0x0000, PREFETCH_DEEP | CPRE | CMTLB }, > + { 0x196e, 0x0000, PREFETCH_DEEP | CPRE | CMTLB }, > + { 0x196f, 0x0000, PREFETCH_DEEP | CPRE | CMTLB }, > + { 0x19c1, 0x0010, PREFETCH_DEEP | CPRE | CMTLB }, > + { 0x19c2, 0x0010, PREFETCH_DEEP | CPRE | CMTLB }, > + { 0x19c3, 0x0010, PREFETCH_DEEP | CPRE | CMTLB }, > + { 0x19c4, 0x0010, PREFETCH_DEEP | CPRE | CMTLB }, > + { 0x19c5, 0x0010, PREFETCH_DEEP | CPRE | CMTLB }, > + { 0x19c6, 0x0010, PREFETCH_DEEP | CPRE | CMTLB }, > + { 0x19c7, 0x0010, PREFETCH_DEEP | CPRE | CMTLB }, > + { 0x19c8, 0x0010, PREFETCH_DEEP | CPRE | CMTLB }, > + { 0x19c9, 0x0010, PREFETCH_DEEP | CPRE | CMTLB }, > + { 0x19cc, 0x0010, PREFETCH_DEEP | CPRE | CMTLB }, > + { 0x19cd, 0x0010, PREFETCH_DEEP | CPRE | CMTLB }, > + { 0x19ce, 0x0010, PREFETCH_DEEP | CPRE | CMTLB }, > + { 0x19cf, 0x0010, PREFETCH_DEEP | CPRE | CMTLB }, > + { 0x1c00, 0x0002, PREFETCH_SHALLOW | CPRE | CMTLB }, > + { 0x1c01, 0x0000, PREFETCH_DEFAULT | CMTLB }, > + { 0x1920, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB }, > + { 0x1923, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB }, > + { 0x1924, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB }, > + { 0x1940, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB }, > + { 0x1941, 0x0004, PREFETCH_SHALLOW | CPRE | CMTLB }, > + { 0x1943, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB }, > + { 0x1944, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB }, > + { 0x1947, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB }, > + {}, > +}; > + > +static const struct actlr_config sm8550_gfx_actlr_cfg[] = { > + { 0x0000, 0x03ff, PREFETCH_SWITCH_GFX | PREFETCH_DEEP | CPRE | CMTLB }, > + {}, > +}; > + > static struct qcom_smmu *to_qcom_smmu(struct arm_smmu_device *smmu) > { > return container_of(smmu, struct qcom_smmu, smmu); > @@ -549,6 +628,15 @@ static const struct qcom_smmu_match_data sdm845_smmu_500_data = { > /* Also no debug configuration. */ > }; > > + > +static const struct qcom_smmu_match_data sm8550_smmu_500_impl0_data = { > + .impl = &qcom_smmu_500_impl, > + .adreno_impl = &qcom_adreno_smmu_500_impl, > + .cfg = &qcom_smmu_impl0_cfg, > + .actlrcfg = sm8550_apps_actlr_cfg, > + .actlrcfg_gfx = sm8550_gfx_actlr_cfg, > +}; > + > static const struct qcom_smmu_match_data qcom_smmu_500_impl0_data = { > .impl = &qcom_smmu_500_impl, > .adreno_impl = &qcom_adreno_smmu_500_impl, > @@ -583,6 +671,7 @@ static const struct of_device_id __maybe_unused qcom_smmu_impl_of_match[] = { > { .compatible = "qcom,sm8250-smmu-500", .data = &qcom_smmu_500_impl0_data }, > { .compatible = "qcom,sm8350-smmu-500", .data = &qcom_smmu_500_impl0_data }, > { .compatible = "qcom,sm8450-smmu-500", .data = &qcom_smmu_500_impl0_data }, > + { .compatible = "qcom,sm8550-smmu-500", .data = &sm8550_smmu_500_impl0_data }, > { .compatible = "qcom,smmu-500", .data = &qcom_smmu_500_impl0_data }, > { } > }; > -- > 2.17.1 >
On 12/15/2023 4:14 PM, Dmitry Baryshkov wrote: > On Fri, 15 Dec 2023 at 12:19, Bibek Kumar Patro > <quic_bibekkum@quicinc.com> wrote: >> >> Add ACTLR data table for SM8550 along with support for >> same including SM8550 specific implementation operations. >> >> Signed-off-by: Bibek Kumar Patro <quic_bibekkum@quicinc.com> >> --- >> drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 89 ++++++++++++++++++++++ >> 1 file changed, 89 insertions(+) >> >> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >> index cb49291f5233..d2006f610243 100644 >> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >> @@ -20,6 +20,85 @@ struct actlr_config { >> u32 actlr; >> }; >> >> +/* >> + * SMMU-500 TRM defines BIT(0) as CMTLB (Enable context caching in the >> + * macro TLB) and BIT(1) as CPRE (Enable context caching in the prefetch >> + * buffer). The remaining bits are implementation defined and vary across >> + * SoCs. >> + */ >> + >> +#define PREFETCH_DEFAULT 0 >> +#define PREFETCH_SHALLOW BIT(8) >> +#define PREFETCH_MODERATE BIT(9) >> +#define PREFETCH_DEEP (BIT(9) | BIT(8)) > > I thin the following might be more correct: > > #include <linux/bitfield.h> > > #define PREFETCH_MASK GENMASK(9, 8) > #define PREFETCH_DEFAULT FIELD_PREP(PREFETCH_MASK, 0) > #define PREFETCH_SHALLOW FIELD_PREP(PREFETCH_MASK, 1) > #define PREFETCH_MODERATE FIELD_PREP(PREFETCH_MASK, 2) > #define PREFETCH_DEEP FIELD_PREP(PREFETCH_MASK, 3) > Ack, thanks for this suggestion. Let me try this out using GENMASK. Once tested, will take care of this in next version. Thanks, Bibek >> +#define PREFETCH_SWITCH_GFX (BIT(5) | BIT(3)) >> +#define CPRE BIT(1) >> +#define CMTLB BIT(0) >> + >> +static const struct actlr_config sm8550_apps_actlr_cfg[] = { >> + { 0x18a0, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB }, >> + { 0x18e0, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB }, >> + { 0x0800, 0x0020, PREFETCH_DEFAULT | CMTLB }, >> + { 0x1800, 0x00c0, PREFETCH_DEFAULT | CMTLB }, >> + { 0x1820, 0x0000, PREFETCH_DEFAULT | CMTLB }, >> + { 0x1860, 0x0000, PREFETCH_DEFAULT | CMTLB }, >> + { 0x0c01, 0x0020, PREFETCH_DEEP | CPRE | CMTLB }, >> + { 0x0c02, 0x0020, PREFETCH_DEEP | CPRE | CMTLB }, >> + { 0x0c03, 0x0020, PREFETCH_DEEP | CPRE | CMTLB }, >> + { 0x0c04, 0x0020, PREFETCH_DEEP | CPRE | CMTLB }, >> + { 0x0c05, 0x0020, PREFETCH_DEEP | CPRE | CMTLB }, >> + { 0x0c06, 0x0020, PREFETCH_DEEP | CPRE | CMTLB }, >> + { 0x0c07, 0x0020, PREFETCH_DEEP | CPRE | CMTLB }, >> + { 0x0c08, 0x0020, PREFETCH_DEEP | CPRE | CMTLB }, >> + { 0x0c09, 0x0020, PREFETCH_DEEP | CPRE | CMTLB }, >> + { 0x0c0c, 0x0020, PREFETCH_DEEP | CPRE | CMTLB }, >> + { 0x0c0d, 0x0020, PREFETCH_DEEP | CPRE | CMTLB }, >> + { 0x0c0e, 0x0020, PREFETCH_DEEP | CPRE | CMTLB }, >> + { 0x0c0f, 0x0020, PREFETCH_DEEP | CPRE | CMTLB }, >> + { 0x1961, 0x0000, PREFETCH_DEEP | CPRE | CMTLB }, >> + { 0x1962, 0x0000, PREFETCH_DEEP | CPRE | CMTLB }, >> + { 0x1963, 0x0000, PREFETCH_DEEP | CPRE | CMTLB }, >> + { 0x1964, 0x0000, PREFETCH_DEEP | CPRE | CMTLB }, >> + { 0x1965, 0x0000, PREFETCH_DEEP | CPRE | CMTLB }, >> + { 0x1966, 0x0000, PREFETCH_DEEP | CPRE | CMTLB }, >> + { 0x1967, 0x0000, PREFETCH_DEEP | CPRE | CMTLB }, >> + { 0x1968, 0x0000, PREFETCH_DEEP | CPRE | CMTLB }, >> + { 0x1969, 0x0000, PREFETCH_DEEP | CPRE | CMTLB }, >> + { 0x196c, 0x0000, PREFETCH_DEEP | CPRE | CMTLB }, >> + { 0x196d, 0x0000, PREFETCH_DEEP | CPRE | CMTLB }, >> + { 0x196e, 0x0000, PREFETCH_DEEP | CPRE | CMTLB }, >> + { 0x196f, 0x0000, PREFETCH_DEEP | CPRE | CMTLB }, >> + { 0x19c1, 0x0010, PREFETCH_DEEP | CPRE | CMTLB }, >> + { 0x19c2, 0x0010, PREFETCH_DEEP | CPRE | CMTLB }, >> + { 0x19c3, 0x0010, PREFETCH_DEEP | CPRE | CMTLB }, >> + { 0x19c4, 0x0010, PREFETCH_DEEP | CPRE | CMTLB }, >> + { 0x19c5, 0x0010, PREFETCH_DEEP | CPRE | CMTLB }, >> + { 0x19c6, 0x0010, PREFETCH_DEEP | CPRE | CMTLB }, >> + { 0x19c7, 0x0010, PREFETCH_DEEP | CPRE | CMTLB }, >> + { 0x19c8, 0x0010, PREFETCH_DEEP | CPRE | CMTLB }, >> + { 0x19c9, 0x0010, PREFETCH_DEEP | CPRE | CMTLB }, >> + { 0x19cc, 0x0010, PREFETCH_DEEP | CPRE | CMTLB }, >> + { 0x19cd, 0x0010, PREFETCH_DEEP | CPRE | CMTLB }, >> + { 0x19ce, 0x0010, PREFETCH_DEEP | CPRE | CMTLB }, >> + { 0x19cf, 0x0010, PREFETCH_DEEP | CPRE | CMTLB }, >> + { 0x1c00, 0x0002, PREFETCH_SHALLOW | CPRE | CMTLB }, >> + { 0x1c01, 0x0000, PREFETCH_DEFAULT | CMTLB }, >> + { 0x1920, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB }, >> + { 0x1923, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB }, >> + { 0x1924, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB }, >> + { 0x1940, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB }, >> + { 0x1941, 0x0004, PREFETCH_SHALLOW | CPRE | CMTLB }, >> + { 0x1943, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB }, >> + { 0x1944, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB }, >> + { 0x1947, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB }, >> + {}, >> +}; >> + >> +static const struct actlr_config sm8550_gfx_actlr_cfg[] = { >> + { 0x0000, 0x03ff, PREFETCH_SWITCH_GFX | PREFETCH_DEEP | CPRE | CMTLB }, >> + {}, >> +}; >> + >> static struct qcom_smmu *to_qcom_smmu(struct arm_smmu_device *smmu) >> { >> return container_of(smmu, struct qcom_smmu, smmu); >> @@ -549,6 +628,15 @@ static const struct qcom_smmu_match_data sdm845_smmu_500_data = { >> /* Also no debug configuration. */ >> }; >> >> + >> +static const struct qcom_smmu_match_data sm8550_smmu_500_impl0_data = { >> + .impl = &qcom_smmu_500_impl, >> + .adreno_impl = &qcom_adreno_smmu_500_impl, >> + .cfg = &qcom_smmu_impl0_cfg, >> + .actlrcfg = sm8550_apps_actlr_cfg, >> + .actlrcfg_gfx = sm8550_gfx_actlr_cfg, >> +}; >> + >> static const struct qcom_smmu_match_data qcom_smmu_500_impl0_data = { >> .impl = &qcom_smmu_500_impl, >> .adreno_impl = &qcom_adreno_smmu_500_impl, >> @@ -583,6 +671,7 @@ static const struct of_device_id __maybe_unused qcom_smmu_impl_of_match[] = { >> { .compatible = "qcom,sm8250-smmu-500", .data = &qcom_smmu_500_impl0_data }, >> { .compatible = "qcom,sm8350-smmu-500", .data = &qcom_smmu_500_impl0_data }, >> { .compatible = "qcom,sm8450-smmu-500", .data = &qcom_smmu_500_impl0_data }, >> + { .compatible = "qcom,sm8550-smmu-500", .data = &sm8550_smmu_500_impl0_data }, >> { .compatible = "qcom,smmu-500", .data = &qcom_smmu_500_impl0_data }, >> { } >> }; >> -- >> 2.17.1 >> > >
On 2023-12-15 12:20 pm, Bibek Kumar Patro wrote: > > > On 12/15/2023 4:14 PM, Dmitry Baryshkov wrote: >> On Fri, 15 Dec 2023 at 12:19, Bibek Kumar Patro >> <quic_bibekkum@quicinc.com> wrote: >>> >>> Add ACTLR data table for SM8550 along with support for >>> same including SM8550 specific implementation operations. >>> >>> Signed-off-by: Bibek Kumar Patro <quic_bibekkum@quicinc.com> >>> --- >>> drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 89 ++++++++++++++++++++++ >>> 1 file changed, 89 insertions(+) >>> >>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>> b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>> index cb49291f5233..d2006f610243 100644 >>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>> @@ -20,6 +20,85 @@ struct actlr_config { >>> u32 actlr; >>> }; >>> >>> +/* >>> + * SMMU-500 TRM defines BIT(0) as CMTLB (Enable context caching in the >>> + * macro TLB) and BIT(1) as CPRE (Enable context caching in the >>> prefetch >>> + * buffer). The remaining bits are implementation defined and vary >>> across >>> + * SoCs. >>> + */ >>> + >>> +#define PREFETCH_DEFAULT 0 >>> +#define PREFETCH_SHALLOW BIT(8) >>> +#define PREFETCH_MODERATE BIT(9) >>> +#define PREFETCH_DEEP (BIT(9) | BIT(8)) >> >> I thin the following might be more correct: >> >> #include <linux/bitfield.h> >> >> #define PREFETCH_MASK GENMASK(9, 8) >> #define PREFETCH_DEFAULT FIELD_PREP(PREFETCH_MASK, 0) >> #define PREFETCH_SHALLOW FIELD_PREP(PREFETCH_MASK, 1) >> #define PREFETCH_MODERATE FIELD_PREP(PREFETCH_MASK, 2) >> #define PREFETCH_DEEP FIELD_PREP(PREFETCH_MASK, 3) >> > > Ack, thanks for this suggestion. Let me try this out using > GENMASK. Once tested, will take care of this in next version. FWIW the more typical usage would be to just define the named macros for the raw field values, then put the FIELD_PREP() at the point of use. However in this case that's liable to get pretty verbose, so although I'm usually a fan of bitfield.h, the most readable option here might actually be to stick with simpler definitions of "(0 << 8)", "(1 << 8)", etc. However it's not really a big deal either way, and I defer to whatever Dmitry and Konrad prefer, since they're the ones looking after arm-smmu-qcom the most :) Thanks, Robin. > > Thanks, > Bibek > >>> +#define PREFETCH_SWITCH_GFX (BIT(5) | BIT(3)) >>> +#define CPRE BIT(1) >>> +#define CMTLB BIT(0) >>> + >>> +static const struct actlr_config sm8550_apps_actlr_cfg[] = { >>> + { 0x18a0, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB }, >>> + { 0x18e0, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB }, >>> + { 0x0800, 0x0020, PREFETCH_DEFAULT | CMTLB }, >>> + { 0x1800, 0x00c0, PREFETCH_DEFAULT | CMTLB }, >>> + { 0x1820, 0x0000, PREFETCH_DEFAULT | CMTLB }, >>> + { 0x1860, 0x0000, PREFETCH_DEFAULT | CMTLB }, >>> + { 0x0c01, 0x0020, PREFETCH_DEEP | CPRE | CMTLB }, >>> + { 0x0c02, 0x0020, PREFETCH_DEEP | CPRE | CMTLB }, >>> + { 0x0c03, 0x0020, PREFETCH_DEEP | CPRE | CMTLB }, >>> + { 0x0c04, 0x0020, PREFETCH_DEEP | CPRE | CMTLB }, >>> + { 0x0c05, 0x0020, PREFETCH_DEEP | CPRE | CMTLB }, >>> + { 0x0c06, 0x0020, PREFETCH_DEEP | CPRE | CMTLB }, >>> + { 0x0c07, 0x0020, PREFETCH_DEEP | CPRE | CMTLB }, >>> + { 0x0c08, 0x0020, PREFETCH_DEEP | CPRE | CMTLB }, >>> + { 0x0c09, 0x0020, PREFETCH_DEEP | CPRE | CMTLB }, >>> + { 0x0c0c, 0x0020, PREFETCH_DEEP | CPRE | CMTLB }, >>> + { 0x0c0d, 0x0020, PREFETCH_DEEP | CPRE | CMTLB }, >>> + { 0x0c0e, 0x0020, PREFETCH_DEEP | CPRE | CMTLB }, >>> + { 0x0c0f, 0x0020, PREFETCH_DEEP | CPRE | CMTLB }, >>> + { 0x1961, 0x0000, PREFETCH_DEEP | CPRE | CMTLB }, >>> + { 0x1962, 0x0000, PREFETCH_DEEP | CPRE | CMTLB }, >>> + { 0x1963, 0x0000, PREFETCH_DEEP | CPRE | CMTLB }, >>> + { 0x1964, 0x0000, PREFETCH_DEEP | CPRE | CMTLB }, >>> + { 0x1965, 0x0000, PREFETCH_DEEP | CPRE | CMTLB }, >>> + { 0x1966, 0x0000, PREFETCH_DEEP | CPRE | CMTLB }, >>> + { 0x1967, 0x0000, PREFETCH_DEEP | CPRE | CMTLB }, >>> + { 0x1968, 0x0000, PREFETCH_DEEP | CPRE | CMTLB }, >>> + { 0x1969, 0x0000, PREFETCH_DEEP | CPRE | CMTLB }, >>> + { 0x196c, 0x0000, PREFETCH_DEEP | CPRE | CMTLB }, >>> + { 0x196d, 0x0000, PREFETCH_DEEP | CPRE | CMTLB }, >>> + { 0x196e, 0x0000, PREFETCH_DEEP | CPRE | CMTLB }, >>> + { 0x196f, 0x0000, PREFETCH_DEEP | CPRE | CMTLB }, >>> + { 0x19c1, 0x0010, PREFETCH_DEEP | CPRE | CMTLB }, >>> + { 0x19c2, 0x0010, PREFETCH_DEEP | CPRE | CMTLB }, >>> + { 0x19c3, 0x0010, PREFETCH_DEEP | CPRE | CMTLB }, >>> + { 0x19c4, 0x0010, PREFETCH_DEEP | CPRE | CMTLB }, >>> + { 0x19c5, 0x0010, PREFETCH_DEEP | CPRE | CMTLB }, >>> + { 0x19c6, 0x0010, PREFETCH_DEEP | CPRE | CMTLB }, >>> + { 0x19c7, 0x0010, PREFETCH_DEEP | CPRE | CMTLB }, >>> + { 0x19c8, 0x0010, PREFETCH_DEEP | CPRE | CMTLB }, >>> + { 0x19c9, 0x0010, PREFETCH_DEEP | CPRE | CMTLB }, >>> + { 0x19cc, 0x0010, PREFETCH_DEEP | CPRE | CMTLB }, >>> + { 0x19cd, 0x0010, PREFETCH_DEEP | CPRE | CMTLB }, >>> + { 0x19ce, 0x0010, PREFETCH_DEEP | CPRE | CMTLB }, >>> + { 0x19cf, 0x0010, PREFETCH_DEEP | CPRE | CMTLB }, >>> + { 0x1c00, 0x0002, PREFETCH_SHALLOW | CPRE | CMTLB }, >>> + { 0x1c01, 0x0000, PREFETCH_DEFAULT | CMTLB }, >>> + { 0x1920, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB }, >>> + { 0x1923, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB }, >>> + { 0x1924, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB }, >>> + { 0x1940, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB }, >>> + { 0x1941, 0x0004, PREFETCH_SHALLOW | CPRE | CMTLB }, >>> + { 0x1943, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB }, >>> + { 0x1944, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB }, >>> + { 0x1947, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB }, >>> + {}, >>> +}; >>> + >>> +static const struct actlr_config sm8550_gfx_actlr_cfg[] = { >>> + { 0x0000, 0x03ff, PREFETCH_SWITCH_GFX | PREFETCH_DEEP | CPRE >>> | CMTLB }, >>> + {}, >>> +}; >>> + >>> static struct qcom_smmu *to_qcom_smmu(struct arm_smmu_device *smmu) >>> { >>> return container_of(smmu, struct qcom_smmu, smmu); >>> @@ -549,6 +628,15 @@ static const struct qcom_smmu_match_data >>> sdm845_smmu_500_data = { >>> /* Also no debug configuration. */ >>> }; >>> >>> + >>> +static const struct qcom_smmu_match_data sm8550_smmu_500_impl0_data = { >>> + .impl = &qcom_smmu_500_impl, >>> + .adreno_impl = &qcom_adreno_smmu_500_impl, >>> + .cfg = &qcom_smmu_impl0_cfg, >>> + .actlrcfg = sm8550_apps_actlr_cfg, >>> + .actlrcfg_gfx = sm8550_gfx_actlr_cfg, >>> +}; >>> + >>> static const struct qcom_smmu_match_data qcom_smmu_500_impl0_data = { >>> .impl = &qcom_smmu_500_impl, >>> .adreno_impl = &qcom_adreno_smmu_500_impl, >>> @@ -583,6 +671,7 @@ static const struct of_device_id __maybe_unused >>> qcom_smmu_impl_of_match[] = { >>> { .compatible = "qcom,sm8250-smmu-500", .data = >>> &qcom_smmu_500_impl0_data }, >>> { .compatible = "qcom,sm8350-smmu-500", .data = >>> &qcom_smmu_500_impl0_data }, >>> { .compatible = "qcom,sm8450-smmu-500", .data = >>> &qcom_smmu_500_impl0_data }, >>> + { .compatible = "qcom,sm8550-smmu-500", .data = >>> &sm8550_smmu_500_impl0_data }, >>> { .compatible = "qcom,smmu-500", .data = >>> &qcom_smmu_500_impl0_data }, >>> { } >>> }; >>> -- >>> 2.17.1 >>> >> >>
On 15.12.2023 11:18, Bibek Kumar Patro wrote: > Add ACTLR data table for SM8550 along with support for > same including SM8550 specific implementation operations. > > Signed-off-by: Bibek Kumar Patro <quic_bibekkum@quicinc.com> > --- [...] > +static const struct qcom_smmu_match_data sm8550_smmu_500_impl0_data = { > + .impl = &qcom_smmu_500_impl, > + .adreno_impl = &qcom_adreno_smmu_500_impl, > + .cfg = &qcom_smmu_impl0_cfg, > + .actlrcfg = sm8550_apps_actlr_cfg, > + .actlrcfg_gfx = sm8550_gfx_actlr_cfg, There are platforms that feature more than just APPS and Adreno SMMUs, this implementation seems to assume there's only these two :/ I suppose the only way to solve this would be to introduce new compatibles for each one of them.. Krzysztof, do you think that's reasonable? E.g. MSM8996 has at least 5 instances, 8998 has at least 4 etc. Konrad
On 15.12.2023 13:54, Robin Murphy wrote: > On 2023-12-15 12:20 pm, Bibek Kumar Patro wrote: >> >> >> On 12/15/2023 4:14 PM, Dmitry Baryshkov wrote: >>> On Fri, 15 Dec 2023 at 12:19, Bibek Kumar Patro >>> <quic_bibekkum@quicinc.com> wrote: >>>> >>>> Add ACTLR data table for SM8550 along with support for >>>> same including SM8550 specific implementation operations. >>>> >>>> Signed-off-by: Bibek Kumar Patro <quic_bibekkum@quicinc.com> >>>> --- >>>> drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 89 ++++++++++++++++++++++ >>>> 1 file changed, 89 insertions(+) >>>> >>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>> index cb49291f5233..d2006f610243 100644 >>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>> @@ -20,6 +20,85 @@ struct actlr_config { >>>> u32 actlr; >>>> }; >>>> >>>> +/* >>>> + * SMMU-500 TRM defines BIT(0) as CMTLB (Enable context caching in the >>>> + * macro TLB) and BIT(1) as CPRE (Enable context caching in the prefetch >>>> + * buffer). The remaining bits are implementation defined and vary across >>>> + * SoCs. >>>> + */ >>>> + >>>> +#define PREFETCH_DEFAULT 0 >>>> +#define PREFETCH_SHALLOW BIT(8) >>>> +#define PREFETCH_MODERATE BIT(9) >>>> +#define PREFETCH_DEEP (BIT(9) | BIT(8)) >>> >>> I thin the following might be more correct: >>> >>> #include <linux/bitfield.h> >>> >>> #define PREFETCH_MASK GENMASK(9, 8) >>> #define PREFETCH_DEFAULT FIELD_PREP(PREFETCH_MASK, 0) >>> #define PREFETCH_SHALLOW FIELD_PREP(PREFETCH_MASK, 1) >>> #define PREFETCH_MODERATE FIELD_PREP(PREFETCH_MASK, 2) >>> #define PREFETCH_DEEP FIELD_PREP(PREFETCH_MASK, 3) >>> >> >> Ack, thanks for this suggestion. Let me try this out using >> GENMASK. Once tested, will take care of this in next version. > > FWIW the more typical usage would be to just define the named macros for the raw field values, then put the FIELD_PREP() at the point of use. However in this case that's liable to get pretty verbose, so although I'm usually a fan of bitfield.h, the most readable option here might actually be to stick with simpler definitions of "(0 << 8)", "(1 << 8)", etc. However it's not really a big deal either way, and I defer to whatever Dmitry and Konrad prefer, since they're the ones looking after arm-smmu-qcom the most :) My 5 cents would be to just use the "common" style of doing this, so: #define ACTRL_PREFETCH GENMASK(9, 8) #define PREFETCH_DEFAULT 0 #define PREFETCH_SHALLOW 1 #define PREFETCH_MODERATE 2 #define PREFETCH_DEEP 3 and then use | FIELD_PREP(ACTRL_PREFETCH, PREFETCH_x) it can get verbose, but.. arguably that's good, since you really want to make sure the right bits are set here Konrad
On 16/12/2023 02:03, Konrad Dybcio wrote: > On 15.12.2023 13:54, Robin Murphy wrote: >> On 2023-12-15 12:20 pm, Bibek Kumar Patro wrote: >>> >>> >>> On 12/15/2023 4:14 PM, Dmitry Baryshkov wrote: >>>> On Fri, 15 Dec 2023 at 12:19, Bibek Kumar Patro >>>> <quic_bibekkum@quicinc.com> wrote: >>>>> >>>>> Add ACTLR data table for SM8550 along with support for >>>>> same including SM8550 specific implementation operations. >>>>> >>>>> Signed-off-by: Bibek Kumar Patro <quic_bibekkum@quicinc.com> >>>>> --- >>>>> drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 89 ++++++++++++++++++++++ >>>>> 1 file changed, 89 insertions(+) >>>>> >>>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>>> index cb49291f5233..d2006f610243 100644 >>>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>>> @@ -20,6 +20,85 @@ struct actlr_config { >>>>> u32 actlr; >>>>> }; >>>>> >>>>> +/* >>>>> + * SMMU-500 TRM defines BIT(0) as CMTLB (Enable context caching in the >>>>> + * macro TLB) and BIT(1) as CPRE (Enable context caching in the prefetch >>>>> + * buffer). The remaining bits are implementation defined and vary across >>>>> + * SoCs. >>>>> + */ >>>>> + >>>>> +#define PREFETCH_DEFAULT 0 >>>>> +#define PREFETCH_SHALLOW BIT(8) >>>>> +#define PREFETCH_MODERATE BIT(9) >>>>> +#define PREFETCH_DEEP (BIT(9) | BIT(8)) >>>> >>>> I thin the following might be more correct: >>>> >>>> #include <linux/bitfield.h> >>>> >>>> #define PREFETCH_MASK GENMASK(9, 8) >>>> #define PREFETCH_DEFAULT FIELD_PREP(PREFETCH_MASK, 0) >>>> #define PREFETCH_SHALLOW FIELD_PREP(PREFETCH_MASK, 1) >>>> #define PREFETCH_MODERATE FIELD_PREP(PREFETCH_MASK, 2) >>>> #define PREFETCH_DEEP FIELD_PREP(PREFETCH_MASK, 3) >>>> >>> >>> Ack, thanks for this suggestion. Let me try this out using >>> GENMASK. Once tested, will take care of this in next version. >> >> FWIW the more typical usage would be to just define the named macros for the raw field values, then put the FIELD_PREP() at the point of use. However in this case that's liable to get pretty verbose, so although I'm usually a fan of bitfield.h, the most readable option here might actually be to stick with simpler definitions of "(0 << 8)", "(1 << 8)", etc. However it's not really a big deal either way, and I defer to whatever Dmitry and Konrad prefer, since they're the ones looking after arm-smmu-qcom the most :) > My 5 cents would be to just use the "common" style of doing this, so: > > #define ACTRL_PREFETCH GENMASK(9, 8) > #define PREFETCH_DEFAULT 0 > #define PREFETCH_SHALLOW 1 > #define PREFETCH_MODERATE 2 > #define PREFETCH_DEEP 3 > > and then use > > | FIELD_PREP(ACTRL_PREFETCH, PREFETCH_x) > > it can get verbose, but.. arguably that's good, since you really want > to make sure the right bits are set here Sounds good to me.
On 16/12/2023 01:35, Konrad Dybcio wrote: > On 15.12.2023 11:18, Bibek Kumar Patro wrote: >> Add ACTLR data table for SM8550 along with support for >> same including SM8550 specific implementation operations. >> >> Signed-off-by: Bibek Kumar Patro <quic_bibekkum@quicinc.com> >> --- > [...] > >> +static const struct qcom_smmu_match_data sm8550_smmu_500_impl0_data = { >> + .impl = &qcom_smmu_500_impl, >> + .adreno_impl = &qcom_adreno_smmu_500_impl, >> + .cfg = &qcom_smmu_impl0_cfg, >> + .actlrcfg = sm8550_apps_actlr_cfg, >> + .actlrcfg_gfx = sm8550_gfx_actlr_cfg, > There are platforms that feature more than just APPS and Adreno SMMUs, > this implementation seems to assume there's only these two :/ > > I suppose the only way to solve this would be to introduce new compatibles > for each one of them.. Krzysztof, do you think that's reasonable? E.g. > MSM8996 has at least 5 instances, 8998 has at least 4 etc. Ugh. I don't think compatibles will make sense here. I think we have to resolve to the hated solution of putting identifying the instance via the IO address.
On 12/15/2023 6:24 PM, Robin Murphy wrote: > On 2023-12-15 12:20 pm, Bibek Kumar Patro wrote: >> >> >> On 12/15/2023 4:14 PM, Dmitry Baryshkov wrote: >>> On Fri, 15 Dec 2023 at 12:19, Bibek Kumar Patro >>> <quic_bibekkum@quicinc.com> wrote: >>>> >>>> Add ACTLR data table for SM8550 along with support for >>>> same including SM8550 specific implementation operations. >>>> >>>> Signed-off-by: Bibek Kumar Patro <quic_bibekkum@quicinc.com> >>>> --- >>>> drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 89 >>>> ++++++++++++++++++++++ >>>> 1 file changed, 89 insertions(+) >>>> >>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>> b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>> index cb49291f5233..d2006f610243 100644 >>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>> @@ -20,6 +20,85 @@ struct actlr_config { >>>> u32 actlr; >>>> }; >>>> >>>> +/* >>>> + * SMMU-500 TRM defines BIT(0) as CMTLB (Enable context caching in the >>>> + * macro TLB) and BIT(1) as CPRE (Enable context caching in the >>>> prefetch >>>> + * buffer). The remaining bits are implementation defined and vary >>>> across >>>> + * SoCs. >>>> + */ >>>> + >>>> +#define PREFETCH_DEFAULT 0 >>>> +#define PREFETCH_SHALLOW BIT(8) >>>> +#define PREFETCH_MODERATE BIT(9) >>>> +#define PREFETCH_DEEP (BIT(9) | BIT(8)) >>> >>> I thin the following might be more correct: >>> >>> #include <linux/bitfield.h> >>> >>> #define PREFETCH_MASK GENMASK(9, 8) >>> #define PREFETCH_DEFAULT FIELD_PREP(PREFETCH_MASK, 0) >>> #define PREFETCH_SHALLOW FIELD_PREP(PREFETCH_MASK, 1) >>> #define PREFETCH_MODERATE FIELD_PREP(PREFETCH_MASK, 2) >>> #define PREFETCH_DEEP FIELD_PREP(PREFETCH_MASK, 3) >>> >> >> Ack, thanks for this suggestion. Let me try this out using >> GENMASK. Once tested, will take care of this in next version. > > FWIW the more typical usage would be to just define the named macros for > the raw field values, then put the FIELD_PREP() at the point of use. > However in this case that's liable to get pretty verbose, so although > I'm usually a fan of bitfield.h, the most readable option here might > actually be to stick with simpler definitions of "(0 << 8)", "(1 << 8)", > etc. However it's not really a big deal either way, and I defer to > whatever Dmitry and Konrad prefer, since they're the ones looking after > arm-smmu-qcom the most :) > Agree, surely simple macros would be easy to understand the bits we are setting/resetting, but to get good verbosity bitfield would surely be helpful as you rightly pointed. I can see some improved suggestions form Konrad as well in the latest reply, the way it'd be better in arm-smmu- qcom. Will try to incorporate these inputs in next version. Thanks, Bibek > Thanks, > Robin. > >> >> Thanks, >> Bibek >> >>>> +#define PREFETCH_SWITCH_GFX (BIT(5) | BIT(3)) >>>> +#define CPRE BIT(1) >>>> +#define CMTLB BIT(0) >>>> + >>>> +static const struct actlr_config sm8550_apps_actlr_cfg[] = { >>>> + { 0x18a0, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB }, >>>> + { 0x18e0, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB }, >>>> + { 0x0800, 0x0020, PREFETCH_DEFAULT | CMTLB }, >>>> + { 0x1800, 0x00c0, PREFETCH_DEFAULT | CMTLB }, >>>> + { 0x1820, 0x0000, PREFETCH_DEFAULT | CMTLB }, >>>> + { 0x1860, 0x0000, PREFETCH_DEFAULT | CMTLB }, >>>> + { 0x0c01, 0x0020, PREFETCH_DEEP | CPRE | CMTLB }, >>>> + { 0x0c02, 0x0020, PREFETCH_DEEP | CPRE | CMTLB }, >>>> + { 0x0c03, 0x0020, PREFETCH_DEEP | CPRE | CMTLB }, >>>> + { 0x0c04, 0x0020, PREFETCH_DEEP | CPRE | CMTLB }, >>>> + { 0x0c05, 0x0020, PREFETCH_DEEP | CPRE | CMTLB }, >>>> + { 0x0c06, 0x0020, PREFETCH_DEEP | CPRE | CMTLB }, >>>> + { 0x0c07, 0x0020, PREFETCH_DEEP | CPRE | CMTLB }, >>>> + { 0x0c08, 0x0020, PREFETCH_DEEP | CPRE | CMTLB }, >>>> + { 0x0c09, 0x0020, PREFETCH_DEEP | CPRE | CMTLB }, >>>> + { 0x0c0c, 0x0020, PREFETCH_DEEP | CPRE | CMTLB }, >>>> + { 0x0c0d, 0x0020, PREFETCH_DEEP | CPRE | CMTLB }, >>>> + { 0x0c0e, 0x0020, PREFETCH_DEEP | CPRE | CMTLB }, >>>> + { 0x0c0f, 0x0020, PREFETCH_DEEP | CPRE | CMTLB }, >>>> + { 0x1961, 0x0000, PREFETCH_DEEP | CPRE | CMTLB }, >>>> + { 0x1962, 0x0000, PREFETCH_DEEP | CPRE | CMTLB }, >>>> + { 0x1963, 0x0000, PREFETCH_DEEP | CPRE | CMTLB }, >>>> + { 0x1964, 0x0000, PREFETCH_DEEP | CPRE | CMTLB }, >>>> + { 0x1965, 0x0000, PREFETCH_DEEP | CPRE | CMTLB }, >>>> + { 0x1966, 0x0000, PREFETCH_DEEP | CPRE | CMTLB }, >>>> + { 0x1967, 0x0000, PREFETCH_DEEP | CPRE | CMTLB }, >>>> + { 0x1968, 0x0000, PREFETCH_DEEP | CPRE | CMTLB }, >>>> + { 0x1969, 0x0000, PREFETCH_DEEP | CPRE | CMTLB }, >>>> + { 0x196c, 0x0000, PREFETCH_DEEP | CPRE | CMTLB }, >>>> + { 0x196d, 0x0000, PREFETCH_DEEP | CPRE | CMTLB }, >>>> + { 0x196e, 0x0000, PREFETCH_DEEP | CPRE | CMTLB }, >>>> + { 0x196f, 0x0000, PREFETCH_DEEP | CPRE | CMTLB }, >>>> + { 0x19c1, 0x0010, PREFETCH_DEEP | CPRE | CMTLB }, >>>> + { 0x19c2, 0x0010, PREFETCH_DEEP | CPRE | CMTLB }, >>>> + { 0x19c3, 0x0010, PREFETCH_DEEP | CPRE | CMTLB }, >>>> + { 0x19c4, 0x0010, PREFETCH_DEEP | CPRE | CMTLB }, >>>> + { 0x19c5, 0x0010, PREFETCH_DEEP | CPRE | CMTLB }, >>>> + { 0x19c6, 0x0010, PREFETCH_DEEP | CPRE | CMTLB }, >>>> + { 0x19c7, 0x0010, PREFETCH_DEEP | CPRE | CMTLB }, >>>> + { 0x19c8, 0x0010, PREFETCH_DEEP | CPRE | CMTLB }, >>>> + { 0x19c9, 0x0010, PREFETCH_DEEP | CPRE | CMTLB }, >>>> + { 0x19cc, 0x0010, PREFETCH_DEEP | CPRE | CMTLB }, >>>> + { 0x19cd, 0x0010, PREFETCH_DEEP | CPRE | CMTLB }, >>>> + { 0x19ce, 0x0010, PREFETCH_DEEP | CPRE | CMTLB }, >>>> + { 0x19cf, 0x0010, PREFETCH_DEEP | CPRE | CMTLB }, >>>> + { 0x1c00, 0x0002, PREFETCH_SHALLOW | CPRE | CMTLB }, >>>> + { 0x1c01, 0x0000, PREFETCH_DEFAULT | CMTLB }, >>>> + { 0x1920, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB }, >>>> + { 0x1923, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB }, >>>> + { 0x1924, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB }, >>>> + { 0x1940, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB }, >>>> + { 0x1941, 0x0004, PREFETCH_SHALLOW | CPRE | CMTLB }, >>>> + { 0x1943, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB }, >>>> + { 0x1944, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB }, >>>> + { 0x1947, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB }, >>>> + {}, >>>> +}; >>>> + >>>> +static const struct actlr_config sm8550_gfx_actlr_cfg[] = { >>>> + { 0x0000, 0x03ff, PREFETCH_SWITCH_GFX | PREFETCH_DEEP | CPRE >>>> | CMTLB }, >>>> + {}, >>>> +}; >>>> + >>>> static struct qcom_smmu *to_qcom_smmu(struct arm_smmu_device *smmu) >>>> { >>>> return container_of(smmu, struct qcom_smmu, smmu); >>>> @@ -549,6 +628,15 @@ static const struct qcom_smmu_match_data >>>> sdm845_smmu_500_data = { >>>> /* Also no debug configuration. */ >>>> }; >>>> >>>> + >>>> +static const struct qcom_smmu_match_data sm8550_smmu_500_impl0_data >>>> = { >>>> + .impl = &qcom_smmu_500_impl, >>>> + .adreno_impl = &qcom_adreno_smmu_500_impl, >>>> + .cfg = &qcom_smmu_impl0_cfg, >>>> + .actlrcfg = sm8550_apps_actlr_cfg, >>>> + .actlrcfg_gfx = sm8550_gfx_actlr_cfg, >>>> +}; >>>> + >>>> static const struct qcom_smmu_match_data qcom_smmu_500_impl0_data = { >>>> .impl = &qcom_smmu_500_impl, >>>> .adreno_impl = &qcom_adreno_smmu_500_impl, >>>> @@ -583,6 +671,7 @@ static const struct of_device_id __maybe_unused >>>> qcom_smmu_impl_of_match[] = { >>>> { .compatible = "qcom,sm8250-smmu-500", .data = >>>> &qcom_smmu_500_impl0_data }, >>>> { .compatible = "qcom,sm8350-smmu-500", .data = >>>> &qcom_smmu_500_impl0_data }, >>>> { .compatible = "qcom,sm8450-smmu-500", .data = >>>> &qcom_smmu_500_impl0_data }, >>>> + { .compatible = "qcom,sm8550-smmu-500", .data = >>>> &sm8550_smmu_500_impl0_data }, >>>> { .compatible = "qcom,smmu-500", .data = >>>> &qcom_smmu_500_impl0_data }, >>>> { } >>>> }; >>>> -- >>>> 2.17.1 >>>> >>> >>>
On 12/16/2023 9:45 PM, Dmitry Baryshkov wrote: > On 16/12/2023 02:03, Konrad Dybcio wrote: >> On 15.12.2023 13:54, Robin Murphy wrote: >>> On 2023-12-15 12:20 pm, Bibek Kumar Patro wrote: >>>> >>>> >>>> On 12/15/2023 4:14 PM, Dmitry Baryshkov wrote: >>>>> On Fri, 15 Dec 2023 at 12:19, Bibek Kumar Patro >>>>> <quic_bibekkum@quicinc.com> wrote: >>>>>> >>>>>> Add ACTLR data table for SM8550 along with support for >>>>>> same including SM8550 specific implementation operations. >>>>>> >>>>>> Signed-off-by: Bibek Kumar Patro <quic_bibekkum@quicinc.com> >>>>>> --- >>>>>> drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 89 >>>>>> ++++++++++++++++++++++ >>>>>> 1 file changed, 89 insertions(+) >>>>>> >>>>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>>>> b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>>>> index cb49291f5233..d2006f610243 100644 >>>>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>>>> @@ -20,6 +20,85 @@ struct actlr_config { >>>>>> u32 actlr; >>>>>> }; >>>>>> >>>>>> +/* >>>>>> + * SMMU-500 TRM defines BIT(0) as CMTLB (Enable context caching >>>>>> in the >>>>>> + * macro TLB) and BIT(1) as CPRE (Enable context caching in the >>>>>> prefetch >>>>>> + * buffer). The remaining bits are implementation defined and >>>>>> vary across >>>>>> + * SoCs. >>>>>> + */ >>>>>> + >>>>>> +#define PREFETCH_DEFAULT 0 >>>>>> +#define PREFETCH_SHALLOW BIT(8) >>>>>> +#define PREFETCH_MODERATE BIT(9) >>>>>> +#define PREFETCH_DEEP (BIT(9) | BIT(8)) >>>>> >>>>> I thin the following might be more correct: >>>>> >>>>> #include <linux/bitfield.h> >>>>> >>>>> #define PREFETCH_MASK GENMASK(9, 8) >>>>> #define PREFETCH_DEFAULT FIELD_PREP(PREFETCH_MASK, 0) >>>>> #define PREFETCH_SHALLOW FIELD_PREP(PREFETCH_MASK, 1) >>>>> #define PREFETCH_MODERATE FIELD_PREP(PREFETCH_MASK, 2) >>>>> #define PREFETCH_DEEP FIELD_PREP(PREFETCH_MASK, 3) >>>>> >>>> >>>> Ack, thanks for this suggestion. Let me try this out using >>>> GENMASK. Once tested, will take care of this in next version. >>> >>> FWIW the more typical usage would be to just define the named macros >>> for the raw field values, then put the FIELD_PREP() at the point of >>> use. However in this case that's liable to get pretty verbose, so >>> although I'm usually a fan of bitfield.h, the most readable option >>> here might actually be to stick with simpler definitions of "(0 << >>> 8)", "(1 << 8)", etc. However it's not really a big deal either way, >>> and I defer to whatever Dmitry and Konrad prefer, since they're the >>> ones looking after arm-smmu-qcom the most :) >> My 5 cents would be to just use the "common" style of doing this, so: >> >> #define ACTRL_PREFETCH GENMASK(9, 8) >> #define PREFETCH_DEFAULT 0 >> #define PREFETCH_SHALLOW 1 >> #define PREFETCH_MODERATE 2 >> #define PREFETCH_DEEP 3 >> >> and then use >> >> | FIELD_PREP(ACTRL_PREFETCH, PREFETCH_x) >> >> it can get verbose, but.. arguably that's good, since you really want >> to make sure the right bits are set here > > Sounds good to me. > Acked.
On 12/16/2023 5:33 AM, Konrad Dybcio wrote: > On 15.12.2023 13:54, Robin Murphy wrote: >> On 2023-12-15 12:20 pm, Bibek Kumar Patro wrote: >>> >>> >>> On 12/15/2023 4:14 PM, Dmitry Baryshkov wrote: >>>> On Fri, 15 Dec 2023 at 12:19, Bibek Kumar Patro >>>> <quic_bibekkum@quicinc.com> wrote: >>>>> >>>>> Add ACTLR data table for SM8550 along with support for >>>>> same including SM8550 specific implementation operations. >>>>> >>>>> Signed-off-by: Bibek Kumar Patro <quic_bibekkum@quicinc.com> >>>>> --- >>>>> drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 89 ++++++++++++++++++++++ >>>>> 1 file changed, 89 insertions(+) >>>>> >>>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>>> index cb49291f5233..d2006f610243 100644 >>>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>>> @@ -20,6 +20,85 @@ struct actlr_config { >>>>> u32 actlr; >>>>> }; >>>>> >>>>> +/* >>>>> + * SMMU-500 TRM defines BIT(0) as CMTLB (Enable context caching in the >>>>> + * macro TLB) and BIT(1) as CPRE (Enable context caching in the prefetch >>>>> + * buffer). The remaining bits are implementation defined and vary across >>>>> + * SoCs. >>>>> + */ >>>>> + >>>>> +#define PREFETCH_DEFAULT 0 >>>>> +#define PREFETCH_SHALLOW BIT(8) >>>>> +#define PREFETCH_MODERATE BIT(9) >>>>> +#define PREFETCH_DEEP (BIT(9) | BIT(8)) >>>> >>>> I thin the following might be more correct: >>>> >>>> #include <linux/bitfield.h> >>>> >>>> #define PREFETCH_MASK GENMASK(9, 8) >>>> #define PREFETCH_DEFAULT FIELD_PREP(PREFETCH_MASK, 0) >>>> #define PREFETCH_SHALLOW FIELD_PREP(PREFETCH_MASK, 1) >>>> #define PREFETCH_MODERATE FIELD_PREP(PREFETCH_MASK, 2) >>>> #define PREFETCH_DEEP FIELD_PREP(PREFETCH_MASK, 3) >>>> >>> >>> Ack, thanks for this suggestion. Let me try this out using >>> GENMASK. Once tested, will take care of this in next version. >> >> FWIW the more typical usage would be to just define the named macros for the raw field values, then put the FIELD_PREP() at the point of use. However in this case that's liable to get pretty verbose, so although I'm usually a fan of bitfield.h, the most readable option here might actually be to stick with simpler definitions of "(0 << 8)", "(1 << 8)", etc. However it's not really a big deal either way, and I defer to whatever Dmitry and Konrad prefer, since they're the ones looking after arm-smmu-qcom the most :) > My 5 cents would be to just use the "common" style of doing this, so: > > #define ACTRL_PREFETCH GENMASK(9, 8) > #define PREFETCH_DEFAULT 0 > #define PREFETCH_SHALLOW 1 > #define PREFETCH_MODERATE 2 > #define PREFETCH_DEEP 3 > > and then use > > | FIELD_PREP(ACTRL_PREFETCH, PREFETCH_x) > > it can get verbose, but.. arguably that's good, since you really want > to make sure the right bits are set here > Thanks for the suggestion with these mods. Let me try out the suggested way and once tested will post this in next version. Thanks, Bibek > Konrad
On 12/16/2023 9:45 PM, Dmitry Baryshkov wrote: > On 16/12/2023 02:03, Konrad Dybcio wrote: >> On 15.12.2023 13:54, Robin Murphy wrote: >>> On 2023-12-15 12:20 pm, Bibek Kumar Patro wrote: >>>> >>>> >>>> On 12/15/2023 4:14 PM, Dmitry Baryshkov wrote: >>>>> On Fri, 15 Dec 2023 at 12:19, Bibek Kumar Patro >>>>> <quic_bibekkum@quicinc.com> wrote: >>>>>> >>>>>> Add ACTLR data table for SM8550 along with support for >>>>>> same including SM8550 specific implementation operations. >>>>>> >>>>>> Signed-off-by: Bibek Kumar Patro <quic_bibekkum@quicinc.com> >>>>>> --- >>>>>> drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 89 >>>>>> ++++++++++++++++++++++ >>>>>> 1 file changed, 89 insertions(+) >>>>>> >>>>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>>>> b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>>>> index cb49291f5233..d2006f610243 100644 >>>>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>>>> @@ -20,6 +20,85 @@ struct actlr_config { >>>>>> u32 actlr; >>>>>> }; >>>>>> >>>>>> +/* >>>>>> + * SMMU-500 TRM defines BIT(0) as CMTLB (Enable context caching >>>>>> in the >>>>>> + * macro TLB) and BIT(1) as CPRE (Enable context caching in the >>>>>> prefetch >>>>>> + * buffer). The remaining bits are implementation defined and >>>>>> vary across >>>>>> + * SoCs. >>>>>> + */ >>>>>> + >>>>>> +#define PREFETCH_DEFAULT 0 >>>>>> +#define PREFETCH_SHALLOW BIT(8) >>>>>> +#define PREFETCH_MODERATE BIT(9) >>>>>> +#define PREFETCH_DEEP (BIT(9) | BIT(8)) >>>>> >>>>> I thin the following might be more correct: >>>>> >>>>> #include <linux/bitfield.h> >>>>> >>>>> #define PREFETCH_MASK GENMASK(9, 8) >>>>> #define PREFETCH_DEFAULT FIELD_PREP(PREFETCH_MASK, 0) >>>>> #define PREFETCH_SHALLOW FIELD_PREP(PREFETCH_MASK, 1) >>>>> #define PREFETCH_MODERATE FIELD_PREP(PREFETCH_MASK, 2) >>>>> #define PREFETCH_DEEP FIELD_PREP(PREFETCH_MASK, 3) >>>>> >>>> >>>> Ack, thanks for this suggestion. Let me try this out using >>>> GENMASK. Once tested, will take care of this in next version. >>> >>> FWIW the more typical usage would be to just define the named macros >>> for the raw field values, then put the FIELD_PREP() at the point of >>> use. However in this case that's liable to get pretty verbose, so >>> although I'm usually a fan of bitfield.h, the most readable option >>> here might actually be to stick with simpler definitions of "(0 << >>> 8)", "(1 << 8)", etc. However it's not really a big deal either way, >>> and I defer to whatever Dmitry and Konrad prefer, since they're the >>> ones looking after arm-smmu-qcom the most :) >> My 5 cents would be to just use the "common" style of doing this, so: >> >> #define ACTRL_PREFETCH GENMASK(9, 8) >> #define PREFETCH_DEFAULT 0 >> #define PREFETCH_SHALLOW 1 >> #define PREFETCH_MODERATE 2 >> #define PREFETCH_DEEP 3 >> >> and then use >> >> | FIELD_PREP(ACTRL_PREFETCH, PREFETCH_x) >> >> it can get verbose, but.. arguably that's good, since you really want >> to make sure the right bits are set here > > Sounds good to me. > Konrad, Dimitry, just checked FIELD_PREP() implementation #define FIELD_FIT(_mask, _val) ({ \ __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: "); \ ((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask); \ }) since it is defined as a block, it won't be possible to use FIELD_PREP in macro or as a structure value, and can only be used inside a block/function. Orelse would show compilation errors as following kernel/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c:94:20: note: in expansion of macro 'PREFETCH_SHALLOW' { 0x1947, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB }, ^ kernel/include/linux/bitfield.h:113:2: error: braced-group within expression allowed only inside a function ({ \ ^ So as per my understanding I think, we might need to go ahead with the generic implementation only. Let me know if I missed something. Thanks, Bibek
On 12/16/2023 5:05 AM, Konrad Dybcio wrote: > On 15.12.2023 11:18, Bibek Kumar Patro wrote: >> Add ACTLR data table for SM8550 along with support for >> same including SM8550 specific implementation operations. >> >> Signed-off-by: Bibek Kumar Patro <quic_bibekkum@quicinc.com> >> --- > [...] > >> +static const struct qcom_smmu_match_data sm8550_smmu_500_impl0_data = { >> + .impl = &qcom_smmu_500_impl, >> + .adreno_impl = &qcom_adreno_smmu_500_impl, >> + .cfg = &qcom_smmu_impl0_cfg, >> + .actlrcfg = sm8550_apps_actlr_cfg, >> + .actlrcfg_gfx = sm8550_gfx_actlr_cfg, > There are platforms that feature more than just APPS and Adreno SMMUs, > this implementation seems to assume there's only these two :/ > Yes, some platforms can feature additional SMMUs as well including APPS and Adreno. In that case there would be a corresponding compatible string and an additional field in qcom_smmu_match_data might be needed. Thanks, Bibek > I suppose the only way to solve this would be to introduce new compatibles > for each one of them.. Krzysztof, do you think that's reasonable? E.g. > MSM8996 has at least 5 instances, 8998 has at least 4 etc. > > Konrad
On 18/12/2023 13:23, Bibek Kumar Patro wrote: > > > On 12/16/2023 9:45 PM, Dmitry Baryshkov wrote: >> On 16/12/2023 02:03, Konrad Dybcio wrote: >>> On 15.12.2023 13:54, Robin Murphy wrote: >>>> On 2023-12-15 12:20 pm, Bibek Kumar Patro wrote: >>>>> >>>>> >>>>> On 12/15/2023 4:14 PM, Dmitry Baryshkov wrote: >>>>>> On Fri, 15 Dec 2023 at 12:19, Bibek Kumar Patro >>>>>> <quic_bibekkum@quicinc.com> wrote: >>>>>>> >>>>>>> Add ACTLR data table for SM8550 along with support for >>>>>>> same including SM8550 specific implementation operations. >>>>>>> >>>>>>> Signed-off-by: Bibek Kumar Patro <quic_bibekkum@quicinc.com> >>>>>>> --- >>>>>>> drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 89 >>>>>>> ++++++++++++++++++++++ >>>>>>> 1 file changed, 89 insertions(+) >>>>>>> >>>>>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>>>>> b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>>>>> index cb49291f5233..d2006f610243 100644 >>>>>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>>>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>>>>> @@ -20,6 +20,85 @@ struct actlr_config { >>>>>>> u32 actlr; >>>>>>> }; >>>>>>> >>>>>>> +/* >>>>>>> + * SMMU-500 TRM defines BIT(0) as CMTLB (Enable context caching >>>>>>> in the >>>>>>> + * macro TLB) and BIT(1) as CPRE (Enable context caching in the >>>>>>> prefetch >>>>>>> + * buffer). The remaining bits are implementation defined and >>>>>>> vary across >>>>>>> + * SoCs. >>>>>>> + */ >>>>>>> + >>>>>>> +#define PREFETCH_DEFAULT 0 >>>>>>> +#define PREFETCH_SHALLOW BIT(8) >>>>>>> +#define PREFETCH_MODERATE BIT(9) >>>>>>> +#define PREFETCH_DEEP (BIT(9) | BIT(8)) >>>>>> >>>>>> I thin the following might be more correct: >>>>>> >>>>>> #include <linux/bitfield.h> >>>>>> >>>>>> #define PREFETCH_MASK GENMASK(9, 8) >>>>>> #define PREFETCH_DEFAULT FIELD_PREP(PREFETCH_MASK, 0) >>>>>> #define PREFETCH_SHALLOW FIELD_PREP(PREFETCH_MASK, 1) >>>>>> #define PREFETCH_MODERATE FIELD_PREP(PREFETCH_MASK, 2) >>>>>> #define PREFETCH_DEEP FIELD_PREP(PREFETCH_MASK, 3) >>>>>> >>>>> >>>>> Ack, thanks for this suggestion. Let me try this out using >>>>> GENMASK. Once tested, will take care of this in next version. >>>> >>>> FWIW the more typical usage would be to just define the named macros >>>> for the raw field values, then put the FIELD_PREP() at the point of >>>> use. However in this case that's liable to get pretty verbose, so >>>> although I'm usually a fan of bitfield.h, the most readable option >>>> here might actually be to stick with simpler definitions of "(0 << >>>> 8)", "(1 << 8)", etc. However it's not really a big deal either way, >>>> and I defer to whatever Dmitry and Konrad prefer, since they're the >>>> ones looking after arm-smmu-qcom the most :) >>> My 5 cents would be to just use the "common" style of doing this, so: >>> >>> #define ACTRL_PREFETCH GENMASK(9, 8) >>> #define PREFETCH_DEFAULT 0 >>> #define PREFETCH_SHALLOW 1 >>> #define PREFETCH_MODERATE 2 >>> #define PREFETCH_DEEP 3 >>> >>> and then use >>> >>> | FIELD_PREP(ACTRL_PREFETCH, PREFETCH_x) >>> >>> it can get verbose, but.. arguably that's good, since you really want >>> to make sure the right bits are set here >> >> Sounds good to me. >> > > Konrad, Dimitry, just checked FIELD_PREP() implementation > > #define FIELD_FIT(_mask, _val) > ({ \ > __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: "); \ > ((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask); \ > }) > > since it is defined as a block, it won't be possible to use FIELD_PREP > in macro or as a structure value, and can only be used inside a > block/function. Orelse would show compilation errors as following > > kernel/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c:94:20: note: in > expansion of macro 'PREFETCH_SHALLOW' > { 0x1947, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB }, > ^ > kernel/include/linux/bitfield.h:113:2: error: braced-group within > expression allowed only inside a function > ({ \ > ^ > > So as per my understanding I think, we might need to go ahead with the > generic implementation only. Let me know if I missed something. Then anyway (foo << bar) is better compared to BIT(n) | BIT(m).
On 12/18/2023 7:51 PM, Dmitry Baryshkov wrote: > On 18/12/2023 13:23, Bibek Kumar Patro wrote: >> >> >> On 12/16/2023 9:45 PM, Dmitry Baryshkov wrote: >>> On 16/12/2023 02:03, Konrad Dybcio wrote: >>>> On 15.12.2023 13:54, Robin Murphy wrote: >>>>> On 2023-12-15 12:20 pm, Bibek Kumar Patro wrote: >>>>>> >>>>>> >>>>>> On 12/15/2023 4:14 PM, Dmitry Baryshkov wrote: >>>>>>> On Fri, 15 Dec 2023 at 12:19, Bibek Kumar Patro >>>>>>> <quic_bibekkum@quicinc.com> wrote: >>>>>>>> >>>>>>>> Add ACTLR data table for SM8550 along with support for >>>>>>>> same including SM8550 specific implementation operations. >>>>>>>> >>>>>>>> Signed-off-by: Bibek Kumar Patro <quic_bibekkum@quicinc.com> >>>>>>>> --- >>>>>>>> drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 89 >>>>>>>> ++++++++++++++++++++++ >>>>>>>> 1 file changed, 89 insertions(+) >>>>>>>> >>>>>>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>>>>>> b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>>>>>> index cb49291f5233..d2006f610243 100644 >>>>>>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>>>>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>>>>>> @@ -20,6 +20,85 @@ struct actlr_config { >>>>>>>> u32 actlr; >>>>>>>> }; >>>>>>>> >>>>>>>> +/* >>>>>>>> + * SMMU-500 TRM defines BIT(0) as CMTLB (Enable context caching >>>>>>>> in the >>>>>>>> + * macro TLB) and BIT(1) as CPRE (Enable context caching in the >>>>>>>> prefetch >>>>>>>> + * buffer). The remaining bits are implementation defined and >>>>>>>> vary across >>>>>>>> + * SoCs. >>>>>>>> + */ >>>>>>>> + >>>>>>>> +#define PREFETCH_DEFAULT 0 >>>>>>>> +#define PREFETCH_SHALLOW BIT(8) >>>>>>>> +#define PREFETCH_MODERATE BIT(9) >>>>>>>> +#define PREFETCH_DEEP (BIT(9) | BIT(8)) >>>>>>> >>>>>>> I thin the following might be more correct: >>>>>>> >>>>>>> #include <linux/bitfield.h> >>>>>>> >>>>>>> #define PREFETCH_MASK GENMASK(9, 8) >>>>>>> #define PREFETCH_DEFAULT FIELD_PREP(PREFETCH_MASK, 0) >>>>>>> #define PREFETCH_SHALLOW FIELD_PREP(PREFETCH_MASK, 1) >>>>>>> #define PREFETCH_MODERATE FIELD_PREP(PREFETCH_MASK, 2) >>>>>>> #define PREFETCH_DEEP FIELD_PREP(PREFETCH_MASK, 3) >>>>>>> >>>>>> >>>>>> Ack, thanks for this suggestion. Let me try this out using >>>>>> GENMASK. Once tested, will take care of this in next version. >>>>> >>>>> FWIW the more typical usage would be to just define the named >>>>> macros for the raw field values, then put the FIELD_PREP() at the >>>>> point of use. However in this case that's liable to get pretty >>>>> verbose, so although I'm usually a fan of bitfield.h, the most >>>>> readable option here might actually be to stick with simpler >>>>> definitions of "(0 << 8)", "(1 << 8)", etc. However it's not really >>>>> a big deal either way, and I defer to whatever Dmitry and Konrad >>>>> prefer, since they're the ones looking after arm-smmu-qcom the most :) >>>> My 5 cents would be to just use the "common" style of doing this, so: >>>> >>>> #define ACTRL_PREFETCH GENMASK(9, 8) >>>> #define PREFETCH_DEFAULT 0 >>>> #define PREFETCH_SHALLOW 1 >>>> #define PREFETCH_MODERATE 2 >>>> #define PREFETCH_DEEP 3 >>>> >>>> and then use >>>> >>>> | FIELD_PREP(ACTRL_PREFETCH, PREFETCH_x) >>>> >>>> it can get verbose, but.. arguably that's good, since you really want >>>> to make sure the right bits are set here >>> >>> Sounds good to me. >>> >> >> Konrad, Dimitry, just checked FIELD_PREP() implementation >> >> #define FIELD_FIT(_mask, _val) >> ({ \ >> __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: "); \ >> ((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask); \ >> }) >> >> since it is defined as a block, it won't be possible to use FIELD_PREP >> in macro or as a structure value, and can only be used inside a >> block/function. Orelse would show compilation errors as following >> >> kernel/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c:94:20: note: in >> expansion of macro 'PREFETCH_SHALLOW' >> { 0x1947, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB }, >> ^ >> kernel/include/linux/bitfield.h:113:2: error: braced-group within >> expression allowed only inside a function >> ({ \ >> ^ >> >> So as per my understanding I think, we might need to go ahead with the >> generic implementation only. Let me know if I missed something. > > Then anyway (foo << bar) is better compared to BIT(n) | BIT(m). > Sure Dmitry, (foo << bar) would be simpler as well as Robin mentioned earlier in his reply. I can implement the defines as: #define PREFETCH_DEFAULT 0 #define PREFETCH_SHALLOW (1 << 8) #define PREFETCH_MODERATE (1 << 9) #define PREFETCH_DEEP (3 << 8) This should be okay I think ? Thanks, Bibek
On Tue, 19 Dec 2023 at 10:25, Bibek Kumar Patro <quic_bibekkum@quicinc.com> wrote: > > > > On 12/18/2023 7:51 PM, Dmitry Baryshkov wrote: > > On 18/12/2023 13:23, Bibek Kumar Patro wrote: > >> > >> > >> On 12/16/2023 9:45 PM, Dmitry Baryshkov wrote: > >>> On 16/12/2023 02:03, Konrad Dybcio wrote: > >>>> On 15.12.2023 13:54, Robin Murphy wrote: > >>>>> On 2023-12-15 12:20 pm, Bibek Kumar Patro wrote: > >>>>>> > >>>>>> > >>>>>> On 12/15/2023 4:14 PM, Dmitry Baryshkov wrote: > >>>>>>> On Fri, 15 Dec 2023 at 12:19, Bibek Kumar Patro > >>>>>>> <quic_bibekkum@quicinc.com> wrote: > >>>>>>>> > >>>>>>>> Add ACTLR data table for SM8550 along with support for > >>>>>>>> same including SM8550 specific implementation operations. > >>>>>>>> > >>>>>>>> Signed-off-by: Bibek Kumar Patro <quic_bibekkum@quicinc.com> > >>>>>>>> --- > >>>>>>>> drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 89 > >>>>>>>> ++++++++++++++++++++++ > >>>>>>>> 1 file changed, 89 insertions(+) > >>>>>>>> > >>>>>>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c > >>>>>>>> b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c > >>>>>>>> index cb49291f5233..d2006f610243 100644 > >>>>>>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c > >>>>>>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c > >>>>>>>> @@ -20,6 +20,85 @@ struct actlr_config { > >>>>>>>> u32 actlr; > >>>>>>>> }; > >>>>>>>> > >>>>>>>> +/* > >>>>>>>> + * SMMU-500 TRM defines BIT(0) as CMTLB (Enable context caching > >>>>>>>> in the > >>>>>>>> + * macro TLB) and BIT(1) as CPRE (Enable context caching in the > >>>>>>>> prefetch > >>>>>>>> + * buffer). The remaining bits are implementation defined and > >>>>>>>> vary across > >>>>>>>> + * SoCs. > >>>>>>>> + */ > >>>>>>>> + > >>>>>>>> +#define PREFETCH_DEFAULT 0 > >>>>>>>> +#define PREFETCH_SHALLOW BIT(8) > >>>>>>>> +#define PREFETCH_MODERATE BIT(9) > >>>>>>>> +#define PREFETCH_DEEP (BIT(9) | BIT(8)) > >>>>>>> > >>>>>>> I thin the following might be more correct: > >>>>>>> > >>>>>>> #include <linux/bitfield.h> > >>>>>>> > >>>>>>> #define PREFETCH_MASK GENMASK(9, 8) > >>>>>>> #define PREFETCH_DEFAULT FIELD_PREP(PREFETCH_MASK, 0) > >>>>>>> #define PREFETCH_SHALLOW FIELD_PREP(PREFETCH_MASK, 1) > >>>>>>> #define PREFETCH_MODERATE FIELD_PREP(PREFETCH_MASK, 2) > >>>>>>> #define PREFETCH_DEEP FIELD_PREP(PREFETCH_MASK, 3) > >>>>>>> > >>>>>> > >>>>>> Ack, thanks for this suggestion. Let me try this out using > >>>>>> GENMASK. Once tested, will take care of this in next version. > >>>>> > >>>>> FWIW the more typical usage would be to just define the named > >>>>> macros for the raw field values, then put the FIELD_PREP() at the > >>>>> point of use. However in this case that's liable to get pretty > >>>>> verbose, so although I'm usually a fan of bitfield.h, the most > >>>>> readable option here might actually be to stick with simpler > >>>>> definitions of "(0 << 8)", "(1 << 8)", etc. However it's not really > >>>>> a big deal either way, and I defer to whatever Dmitry and Konrad > >>>>> prefer, since they're the ones looking after arm-smmu-qcom the most :) > >>>> My 5 cents would be to just use the "common" style of doing this, so: > >>>> > >>>> #define ACTRL_PREFETCH GENMASK(9, 8) > >>>> #define PREFETCH_DEFAULT 0 > >>>> #define PREFETCH_SHALLOW 1 > >>>> #define PREFETCH_MODERATE 2 > >>>> #define PREFETCH_DEEP 3 > >>>> > >>>> and then use > >>>> > >>>> | FIELD_PREP(ACTRL_PREFETCH, PREFETCH_x) > >>>> > >>>> it can get verbose, but.. arguably that's good, since you really want > >>>> to make sure the right bits are set here > >>> > >>> Sounds good to me. > >>> > >> > >> Konrad, Dimitry, just checked FIELD_PREP() implementation > >> > >> #define FIELD_FIT(_mask, _val) > >> ({ \ > >> __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: "); \ > >> ((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask); \ > >> }) > >> > >> since it is defined as a block, it won't be possible to use FIELD_PREP > >> in macro or as a structure value, and can only be used inside a > >> block/function. Orelse would show compilation errors as following > >> > >> kernel/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c:94:20: note: in > >> expansion of macro 'PREFETCH_SHALLOW' > >> { 0x1947, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB }, > >> ^ > >> kernel/include/linux/bitfield.h:113:2: error: braced-group within > >> expression allowed only inside a function > >> ({ \ > >> ^ > >> > >> So as per my understanding I think, we might need to go ahead with the > >> generic implementation only. Let me know if I missed something. > > > > Then anyway (foo << bar) is better compared to BIT(n) | BIT(m). > > > > Sure Dmitry, (foo << bar) would be simpler as well as Robin mentioned > earlier in his reply. > I can implement the defines as: > > #define PREFETCH_DEFAULT 0 > #define PREFETCH_SHALLOW (1 << 8) > #define PREFETCH_MODERATE (1 << 9) 2 << 8. Isn't that hard. > #define PREFETCH_DEEP (3 << 8) > > This should be okay I think ? > > Thanks, > Bibek >
On 12/19/2023 3:51 PM, Dmitry Baryshkov wrote: > On Tue, 19 Dec 2023 at 10:25, Bibek Kumar Patro > <quic_bibekkum@quicinc.com> wrote: >> >> >> >> On 12/18/2023 7:51 PM, Dmitry Baryshkov wrote: >>> On 18/12/2023 13:23, Bibek Kumar Patro wrote: >>>> >>>> >>>> On 12/16/2023 9:45 PM, Dmitry Baryshkov wrote: >>>>> On 16/12/2023 02:03, Konrad Dybcio wrote: >>>>>> On 15.12.2023 13:54, Robin Murphy wrote: >>>>>>> On 2023-12-15 12:20 pm, Bibek Kumar Patro wrote: >>>>>>>> >>>>>>>> >>>>>>>> On 12/15/2023 4:14 PM, Dmitry Baryshkov wrote: >>>>>>>>> On Fri, 15 Dec 2023 at 12:19, Bibek Kumar Patro >>>>>>>>> <quic_bibekkum@quicinc.com> wrote: >>>>>>>>>> >>>>>>>>>> Add ACTLR data table for SM8550 along with support for >>>>>>>>>> same including SM8550 specific implementation operations. >>>>>>>>>> >>>>>>>>>> Signed-off-by: Bibek Kumar Patro <quic_bibekkum@quicinc.com> >>>>>>>>>> --- >>>>>>>>>> drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 89 >>>>>>>>>> ++++++++++++++++++++++ >>>>>>>>>> 1 file changed, 89 insertions(+) >>>>>>>>>> >>>>>>>>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>>>>>>>> b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>>>>>>>> index cb49291f5233..d2006f610243 100644 >>>>>>>>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>>>>>>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>>>>>>>> @@ -20,6 +20,85 @@ struct actlr_config { >>>>>>>>>> u32 actlr; >>>>>>>>>> }; >>>>>>>>>> >>>>>>>>>> +/* >>>>>>>>>> + * SMMU-500 TRM defines BIT(0) as CMTLB (Enable context caching >>>>>>>>>> in the >>>>>>>>>> + * macro TLB) and BIT(1) as CPRE (Enable context caching in the >>>>>>>>>> prefetch >>>>>>>>>> + * buffer). The remaining bits are implementation defined and >>>>>>>>>> vary across >>>>>>>>>> + * SoCs. >>>>>>>>>> + */ >>>>>>>>>> + >>>>>>>>>> +#define PREFETCH_DEFAULT 0 >>>>>>>>>> +#define PREFETCH_SHALLOW BIT(8) >>>>>>>>>> +#define PREFETCH_MODERATE BIT(9) >>>>>>>>>> +#define PREFETCH_DEEP (BIT(9) | BIT(8)) >>>>>>>>> >>>>>>>>> I thin the following might be more correct: >>>>>>>>> >>>>>>>>> #include <linux/bitfield.h> >>>>>>>>> >>>>>>>>> #define PREFETCH_MASK GENMASK(9, 8) >>>>>>>>> #define PREFETCH_DEFAULT FIELD_PREP(PREFETCH_MASK, 0) >>>>>>>>> #define PREFETCH_SHALLOW FIELD_PREP(PREFETCH_MASK, 1) >>>>>>>>> #define PREFETCH_MODERATE FIELD_PREP(PREFETCH_MASK, 2) >>>>>>>>> #define PREFETCH_DEEP FIELD_PREP(PREFETCH_MASK, 3) >>>>>>>>> >>>>>>>> >>>>>>>> Ack, thanks for this suggestion. Let me try this out using >>>>>>>> GENMASK. Once tested, will take care of this in next version. >>>>>>> >>>>>>> FWIW the more typical usage would be to just define the named >>>>>>> macros for the raw field values, then put the FIELD_PREP() at the >>>>>>> point of use. However in this case that's liable to get pretty >>>>>>> verbose, so although I'm usually a fan of bitfield.h, the most >>>>>>> readable option here might actually be to stick with simpler >>>>>>> definitions of "(0 << 8)", "(1 << 8)", etc. However it's not really >>>>>>> a big deal either way, and I defer to whatever Dmitry and Konrad >>>>>>> prefer, since they're the ones looking after arm-smmu-qcom the most :) >>>>>> My 5 cents would be to just use the "common" style of doing this, so: >>>>>> >>>>>> #define ACTRL_PREFETCH GENMASK(9, 8) >>>>>> #define PREFETCH_DEFAULT 0 >>>>>> #define PREFETCH_SHALLOW 1 >>>>>> #define PREFETCH_MODERATE 2 >>>>>> #define PREFETCH_DEEP 3 >>>>>> >>>>>> and then use >>>>>> >>>>>> | FIELD_PREP(ACTRL_PREFETCH, PREFETCH_x) >>>>>> >>>>>> it can get verbose, but.. arguably that's good, since you really want >>>>>> to make sure the right bits are set here >>>>> >>>>> Sounds good to me. >>>>> >>>> >>>> Konrad, Dimitry, just checked FIELD_PREP() implementation >>>> >>>> #define FIELD_FIT(_mask, _val) >>>> ({ \ >>>> __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: "); \ >>>> ((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask); \ >>>> }) >>>> >>>> since it is defined as a block, it won't be possible to use FIELD_PREP >>>> in macro or as a structure value, and can only be used inside a >>>> block/function. Orelse would show compilation errors as following >>>> >>>> kernel/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c:94:20: note: in >>>> expansion of macro 'PREFETCH_SHALLOW' >>>> { 0x1947, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB }, >>>> ^ >>>> kernel/include/linux/bitfield.h:113:2: error: braced-group within >>>> expression allowed only inside a function >>>> ({ \ >>>> ^ >>>> >>>> So as per my understanding I think, we might need to go ahead with the >>>> generic implementation only. Let me know if I missed something. >>> >>> Then anyway (foo << bar) is better compared to BIT(n) | BIT(m). >>> >> >> Sure Dmitry, (foo << bar) would be simpler as well as Robin mentioned >> earlier in his reply. >> I can implement the defines as: >> >> #define PREFETCH_DEFAULT 0 >> #define PREFETCH_SHALLOW (1 << 8) >> #define PREFETCH_MODERATE (1 << 9) > > 2 << 8. Isn't that hard. > Ah, right. This is nice! . Will use 2 << 8 instead. Thanks for the suggestion. Thanks, Bibek >> #define PREFETCH_DEEP (3 << 8) >> >> This should be okay I think ? >> >> Thanks, >> Bibek >> > >
On Tue, 19 Dec 2023 at 12:37, Bibek Kumar Patro <quic_bibekkum@quicinc.com> wrote: > > > > On 12/19/2023 3:51 PM, Dmitry Baryshkov wrote: > > On Tue, 19 Dec 2023 at 10:25, Bibek Kumar Patro > > <quic_bibekkum@quicinc.com> wrote: > >> > >> > >> > >> On 12/18/2023 7:51 PM, Dmitry Baryshkov wrote: > >>> On 18/12/2023 13:23, Bibek Kumar Patro wrote: > >>>> > >>>> > >>>> On 12/16/2023 9:45 PM, Dmitry Baryshkov wrote: > >>>>> On 16/12/2023 02:03, Konrad Dybcio wrote: > >>>>>> On 15.12.2023 13:54, Robin Murphy wrote: > >>>>>>> On 2023-12-15 12:20 pm, Bibek Kumar Patro wrote: > >>>>>>>> > >>>>>>>> > >>>>>>>> On 12/15/2023 4:14 PM, Dmitry Baryshkov wrote: > >>>>>>>>> On Fri, 15 Dec 2023 at 12:19, Bibek Kumar Patro > >>>>>>>>> <quic_bibekkum@quicinc.com> wrote: > >>>>>>>>>> > >>>>>>>>>> Add ACTLR data table for SM8550 along with support for > >>>>>>>>>> same including SM8550 specific implementation operations. > >>>>>>>>>> > >>>>>>>>>> Signed-off-by: Bibek Kumar Patro <quic_bibekkum@quicinc.com> > >>>>>>>>>> --- > >>>>>>>>>> drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 89 > >>>>>>>>>> ++++++++++++++++++++++ > >>>>>>>>>> 1 file changed, 89 insertions(+) > >>>>>>>>>> > >>>>>>>>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c > >>>>>>>>>> b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c > >>>>>>>>>> index cb49291f5233..d2006f610243 100644 > >>>>>>>>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c > >>>>>>>>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c > >>>>>>>>>> @@ -20,6 +20,85 @@ struct actlr_config { > >>>>>>>>>> u32 actlr; > >>>>>>>>>> }; > >>>>>>>>>> > >>>>>>>>>> +/* > >>>>>>>>>> + * SMMU-500 TRM defines BIT(0) as CMTLB (Enable context caching > >>>>>>>>>> in the > >>>>>>>>>> + * macro TLB) and BIT(1) as CPRE (Enable context caching in the > >>>>>>>>>> prefetch > >>>>>>>>>> + * buffer). The remaining bits are implementation defined and > >>>>>>>>>> vary across > >>>>>>>>>> + * SoCs. > >>>>>>>>>> + */ > >>>>>>>>>> + > >>>>>>>>>> +#define PREFETCH_DEFAULT 0 > >>>>>>>>>> +#define PREFETCH_SHALLOW BIT(8) > >>>>>>>>>> +#define PREFETCH_MODERATE BIT(9) > >>>>>>>>>> +#define PREFETCH_DEEP (BIT(9) | BIT(8)) > >>>>>>>>> > >>>>>>>>> I thin the following might be more correct: > >>>>>>>>> > >>>>>>>>> #include <linux/bitfield.h> > >>>>>>>>> > >>>>>>>>> #define PREFETCH_MASK GENMASK(9, 8) > >>>>>>>>> #define PREFETCH_DEFAULT FIELD_PREP(PREFETCH_MASK, 0) > >>>>>>>>> #define PREFETCH_SHALLOW FIELD_PREP(PREFETCH_MASK, 1) > >>>>>>>>> #define PREFETCH_MODERATE FIELD_PREP(PREFETCH_MASK, 2) > >>>>>>>>> #define PREFETCH_DEEP FIELD_PREP(PREFETCH_MASK, 3) > >>>>>>>>> > >>>>>>>> > >>>>>>>> Ack, thanks for this suggestion. Let me try this out using > >>>>>>>> GENMASK. Once tested, will take care of this in next version. > >>>>>>> > >>>>>>> FWIW the more typical usage would be to just define the named > >>>>>>> macros for the raw field values, then put the FIELD_PREP() at the > >>>>>>> point of use. However in this case that's liable to get pretty > >>>>>>> verbose, so although I'm usually a fan of bitfield.h, the most > >>>>>>> readable option here might actually be to stick with simpler > >>>>>>> definitions of "(0 << 8)", "(1 << 8)", etc. However it's not really > >>>>>>> a big deal either way, and I defer to whatever Dmitry and Konrad > >>>>>>> prefer, since they're the ones looking after arm-smmu-qcom the most :) > >>>>>> My 5 cents would be to just use the "common" style of doing this, so: > >>>>>> > >>>>>> #define ACTRL_PREFETCH GENMASK(9, 8) > >>>>>> #define PREFETCH_DEFAULT 0 > >>>>>> #define PREFETCH_SHALLOW 1 > >>>>>> #define PREFETCH_MODERATE 2 > >>>>>> #define PREFETCH_DEEP 3 > >>>>>> > >>>>>> and then use > >>>>>> > >>>>>> | FIELD_PREP(ACTRL_PREFETCH, PREFETCH_x) > >>>>>> > >>>>>> it can get verbose, but.. arguably that's good, since you really want > >>>>>> to make sure the right bits are set here > >>>>> > >>>>> Sounds good to me. > >>>>> > >>>> > >>>> Konrad, Dimitry, just checked FIELD_PREP() implementation > >>>> > >>>> #define FIELD_FIT(_mask, _val) > >>>> ({ \ > >>>> __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: "); \ > >>>> ((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask); \ > >>>> }) > >>>> > >>>> since it is defined as a block, it won't be possible to use FIELD_PREP > >>>> in macro or as a structure value, and can only be used inside a > >>>> block/function. Orelse would show compilation errors as following > >>>> > >>>> kernel/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c:94:20: note: in > >>>> expansion of macro 'PREFETCH_SHALLOW' > >>>> { 0x1947, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB }, > >>>> ^ > >>>> kernel/include/linux/bitfield.h:113:2: error: braced-group within > >>>> expression allowed only inside a function > >>>> ({ \ > >>>> ^ > >>>> > >>>> So as per my understanding I think, we might need to go ahead with the > >>>> generic implementation only. Let me know if I missed something. > >>> > >>> Then anyway (foo << bar) is better compared to BIT(n) | BIT(m). > >>> > >> > >> Sure Dmitry, (foo << bar) would be simpler as well as Robin mentioned > >> earlier in his reply. > >> I can implement the defines as: > >> > >> #define PREFETCH_DEFAULT 0 > >> #define PREFETCH_SHALLOW (1 << 8) > >> #define PREFETCH_MODERATE (1 << 9) > > > > 2 << 8. Isn't that hard. > > > > Ah, right. This is nice! . > Will use 2 << 8 instead. Thanks for the suggestion. It might still be useful to define the PREFETCH_SHIFT equal to 8. > > Thanks, > Bibek > > >> #define PREFETCH_DEEP (3 << 8) > >> > >> This should be okay I think ? > >> > >> Thanks, > >> Bibek > >> > > > >
On 12/19/2023 4:14 PM, Dmitry Baryshkov wrote: > On Tue, 19 Dec 2023 at 12:37, Bibek Kumar Patro > <quic_bibekkum@quicinc.com> wrote: >> >> >> >> On 12/19/2023 3:51 PM, Dmitry Baryshkov wrote: >>> On Tue, 19 Dec 2023 at 10:25, Bibek Kumar Patro >>> <quic_bibekkum@quicinc.com> wrote: >>>> >>>> >>>> >>>> On 12/18/2023 7:51 PM, Dmitry Baryshkov wrote: >>>>> On 18/12/2023 13:23, Bibek Kumar Patro wrote: >>>>>> >>>>>> >>>>>> On 12/16/2023 9:45 PM, Dmitry Baryshkov wrote: >>>>>>> On 16/12/2023 02:03, Konrad Dybcio wrote: >>>>>>>> On 15.12.2023 13:54, Robin Murphy wrote: >>>>>>>>> On 2023-12-15 12:20 pm, Bibek Kumar Patro wrote: >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> On 12/15/2023 4:14 PM, Dmitry Baryshkov wrote: >>>>>>>>>>> On Fri, 15 Dec 2023 at 12:19, Bibek Kumar Patro >>>>>>>>>>> <quic_bibekkum@quicinc.com> wrote: >>>>>>>>>>>> >>>>>>>>>>>> Add ACTLR data table for SM8550 along with support for >>>>>>>>>>>> same including SM8550 specific implementation operations. >>>>>>>>>>>> >>>>>>>>>>>> Signed-off-by: Bibek Kumar Patro <quic_bibekkum@quicinc.com> >>>>>>>>>>>> --- >>>>>>>>>>>> drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 89 >>>>>>>>>>>> ++++++++++++++++++++++ >>>>>>>>>>>> 1 file changed, 89 insertions(+) >>>>>>>>>>>> >>>>>>>>>>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>>>>>>>>>> b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>>>>>>>>>> index cb49291f5233..d2006f610243 100644 >>>>>>>>>>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>>>>>>>>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>>>>>>>>>> @@ -20,6 +20,85 @@ struct actlr_config { >>>>>>>>>>>> u32 actlr; >>>>>>>>>>>> }; >>>>>>>>>>>> >>>>>>>>>>>> +/* >>>>>>>>>>>> + * SMMU-500 TRM defines BIT(0) as CMTLB (Enable context caching >>>>>>>>>>>> in the >>>>>>>>>>>> + * macro TLB) and BIT(1) as CPRE (Enable context caching in the >>>>>>>>>>>> prefetch >>>>>>>>>>>> + * buffer). The remaining bits are implementation defined and >>>>>>>>>>>> vary across >>>>>>>>>>>> + * SoCs. >>>>>>>>>>>> + */ >>>>>>>>>>>> + >>>>>>>>>>>> +#define PREFETCH_DEFAULT 0 >>>>>>>>>>>> +#define PREFETCH_SHALLOW BIT(8) >>>>>>>>>>>> +#define PREFETCH_MODERATE BIT(9) >>>>>>>>>>>> +#define PREFETCH_DEEP (BIT(9) | BIT(8)) >>>>>>>>>>> >>>>>>>>>>> I thin the following might be more correct: >>>>>>>>>>> >>>>>>>>>>> #include <linux/bitfield.h> >>>>>>>>>>> >>>>>>>>>>> #define PREFETCH_MASK GENMASK(9, 8) >>>>>>>>>>> #define PREFETCH_DEFAULT FIELD_PREP(PREFETCH_MASK, 0) >>>>>>>>>>> #define PREFETCH_SHALLOW FIELD_PREP(PREFETCH_MASK, 1) >>>>>>>>>>> #define PREFETCH_MODERATE FIELD_PREP(PREFETCH_MASK, 2) >>>>>>>>>>> #define PREFETCH_DEEP FIELD_PREP(PREFETCH_MASK, 3) >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> Ack, thanks for this suggestion. Let me try this out using >>>>>>>>>> GENMASK. Once tested, will take care of this in next version. >>>>>>>>> >>>>>>>>> FWIW the more typical usage would be to just define the named >>>>>>>>> macros for the raw field values, then put the FIELD_PREP() at the >>>>>>>>> point of use. However in this case that's liable to get pretty >>>>>>>>> verbose, so although I'm usually a fan of bitfield.h, the most >>>>>>>>> readable option here might actually be to stick with simpler >>>>>>>>> definitions of "(0 << 8)", "(1 << 8)", etc. However it's not really >>>>>>>>> a big deal either way, and I defer to whatever Dmitry and Konrad >>>>>>>>> prefer, since they're the ones looking after arm-smmu-qcom the most :) >>>>>>>> My 5 cents would be to just use the "common" style of doing this, so: >>>>>>>> >>>>>>>> #define ACTRL_PREFETCH GENMASK(9, 8) >>>>>>>> #define PREFETCH_DEFAULT 0 >>>>>>>> #define PREFETCH_SHALLOW 1 >>>>>>>> #define PREFETCH_MODERATE 2 >>>>>>>> #define PREFETCH_DEEP 3 >>>>>>>> >>>>>>>> and then use >>>>>>>> >>>>>>>> | FIELD_PREP(ACTRL_PREFETCH, PREFETCH_x) >>>>>>>> >>>>>>>> it can get verbose, but.. arguably that's good, since you really want >>>>>>>> to make sure the right bits are set here >>>>>>> >>>>>>> Sounds good to me. >>>>>>> >>>>>> >>>>>> Konrad, Dimitry, just checked FIELD_PREP() implementation >>>>>> >>>>>> #define FIELD_FIT(_mask, _val) >>>>>> ({ \ >>>>>> __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: "); \ >>>>>> ((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask); \ >>>>>> }) >>>>>> >>>>>> since it is defined as a block, it won't be possible to use FIELD_PREP >>>>>> in macro or as a structure value, and can only be used inside a >>>>>> block/function. Orelse would show compilation errors as following >>>>>> >>>>>> kernel/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c:94:20: note: in >>>>>> expansion of macro 'PREFETCH_SHALLOW' >>>>>> { 0x1947, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB }, >>>>>> ^ >>>>>> kernel/include/linux/bitfield.h:113:2: error: braced-group within >>>>>> expression allowed only inside a function >>>>>> ({ \ >>>>>> ^ >>>>>> >>>>>> So as per my understanding I think, we might need to go ahead with the >>>>>> generic implementation only. Let me know if I missed something. >>>>> >>>>> Then anyway (foo << bar) is better compared to BIT(n) | BIT(m). >>>>> >>>> >>>> Sure Dmitry, (foo << bar) would be simpler as well as Robin mentioned >>>> earlier in his reply. >>>> I can implement the defines as: >>>> >>>> #define PREFETCH_DEFAULT 0 >>>> #define PREFETCH_SHALLOW (1 << 8) >>>> #define PREFETCH_MODERATE (1 << 9) >>> >>> 2 << 8. Isn't that hard. >>> >> >> Ah, right. This is nice! . >> Will use 2 << 8 instead. Thanks for the suggestion. > > It might still be useful to define the PREFETCH_SHIFT equal to 8. > Sure, looks okay to me as well to define PREFETCH_SHIFT to 8 as it's constant. Thanks, Bibek >> >> Thanks, >> Bibek >> >>>> #define PREFETCH_DEEP (3 << 8) >>>> >>>> This should be okay I think ? >>>> >>>> Thanks, >>>> Bibek >>>> >>> >>> > > >
diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c index cb49291f5233..d2006f610243 100644 --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c @@ -20,6 +20,85 @@ struct actlr_config { u32 actlr; }; +/* + * SMMU-500 TRM defines BIT(0) as CMTLB (Enable context caching in the + * macro TLB) and BIT(1) as CPRE (Enable context caching in the prefetch + * buffer). The remaining bits are implementation defined and vary across + * SoCs. + */ + +#define PREFETCH_DEFAULT 0 +#define PREFETCH_SHALLOW BIT(8) +#define PREFETCH_MODERATE BIT(9) +#define PREFETCH_DEEP (BIT(9) | BIT(8)) +#define PREFETCH_SWITCH_GFX (BIT(5) | BIT(3)) +#define CPRE BIT(1) +#define CMTLB BIT(0) + +static const struct actlr_config sm8550_apps_actlr_cfg[] = { + { 0x18a0, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB }, + { 0x18e0, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB }, + { 0x0800, 0x0020, PREFETCH_DEFAULT | CMTLB }, + { 0x1800, 0x00c0, PREFETCH_DEFAULT | CMTLB }, + { 0x1820, 0x0000, PREFETCH_DEFAULT | CMTLB }, + { 0x1860, 0x0000, PREFETCH_DEFAULT | CMTLB }, + { 0x0c01, 0x0020, PREFETCH_DEEP | CPRE | CMTLB }, + { 0x0c02, 0x0020, PREFETCH_DEEP | CPRE | CMTLB }, + { 0x0c03, 0x0020, PREFETCH_DEEP | CPRE | CMTLB }, + { 0x0c04, 0x0020, PREFETCH_DEEP | CPRE | CMTLB }, + { 0x0c05, 0x0020, PREFETCH_DEEP | CPRE | CMTLB }, + { 0x0c06, 0x0020, PREFETCH_DEEP | CPRE | CMTLB }, + { 0x0c07, 0x0020, PREFETCH_DEEP | CPRE | CMTLB }, + { 0x0c08, 0x0020, PREFETCH_DEEP | CPRE | CMTLB }, + { 0x0c09, 0x0020, PREFETCH_DEEP | CPRE | CMTLB }, + { 0x0c0c, 0x0020, PREFETCH_DEEP | CPRE | CMTLB }, + { 0x0c0d, 0x0020, PREFETCH_DEEP | CPRE | CMTLB }, + { 0x0c0e, 0x0020, PREFETCH_DEEP | CPRE | CMTLB }, + { 0x0c0f, 0x0020, PREFETCH_DEEP | CPRE | CMTLB }, + { 0x1961, 0x0000, PREFETCH_DEEP | CPRE | CMTLB }, + { 0x1962, 0x0000, PREFETCH_DEEP | CPRE | CMTLB }, + { 0x1963, 0x0000, PREFETCH_DEEP | CPRE | CMTLB }, + { 0x1964, 0x0000, PREFETCH_DEEP | CPRE | CMTLB }, + { 0x1965, 0x0000, PREFETCH_DEEP | CPRE | CMTLB }, + { 0x1966, 0x0000, PREFETCH_DEEP | CPRE | CMTLB }, + { 0x1967, 0x0000, PREFETCH_DEEP | CPRE | CMTLB }, + { 0x1968, 0x0000, PREFETCH_DEEP | CPRE | CMTLB }, + { 0x1969, 0x0000, PREFETCH_DEEP | CPRE | CMTLB }, + { 0x196c, 0x0000, PREFETCH_DEEP | CPRE | CMTLB }, + { 0x196d, 0x0000, PREFETCH_DEEP | CPRE | CMTLB }, + { 0x196e, 0x0000, PREFETCH_DEEP | CPRE | CMTLB }, + { 0x196f, 0x0000, PREFETCH_DEEP | CPRE | CMTLB }, + { 0x19c1, 0x0010, PREFETCH_DEEP | CPRE | CMTLB }, + { 0x19c2, 0x0010, PREFETCH_DEEP | CPRE | CMTLB }, + { 0x19c3, 0x0010, PREFETCH_DEEP | CPRE | CMTLB }, + { 0x19c4, 0x0010, PREFETCH_DEEP | CPRE | CMTLB }, + { 0x19c5, 0x0010, PREFETCH_DEEP | CPRE | CMTLB }, + { 0x19c6, 0x0010, PREFETCH_DEEP | CPRE | CMTLB }, + { 0x19c7, 0x0010, PREFETCH_DEEP | CPRE | CMTLB }, + { 0x19c8, 0x0010, PREFETCH_DEEP | CPRE | CMTLB }, + { 0x19c9, 0x0010, PREFETCH_DEEP | CPRE | CMTLB }, + { 0x19cc, 0x0010, PREFETCH_DEEP | CPRE | CMTLB }, + { 0x19cd, 0x0010, PREFETCH_DEEP | CPRE | CMTLB }, + { 0x19ce, 0x0010, PREFETCH_DEEP | CPRE | CMTLB }, + { 0x19cf, 0x0010, PREFETCH_DEEP | CPRE | CMTLB }, + { 0x1c00, 0x0002, PREFETCH_SHALLOW | CPRE | CMTLB }, + { 0x1c01, 0x0000, PREFETCH_DEFAULT | CMTLB }, + { 0x1920, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB }, + { 0x1923, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB }, + { 0x1924, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB }, + { 0x1940, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB }, + { 0x1941, 0x0004, PREFETCH_SHALLOW | CPRE | CMTLB }, + { 0x1943, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB }, + { 0x1944, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB }, + { 0x1947, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB }, + {}, +}; + +static const struct actlr_config sm8550_gfx_actlr_cfg[] = { + { 0x0000, 0x03ff, PREFETCH_SWITCH_GFX | PREFETCH_DEEP | CPRE | CMTLB }, + {}, +}; + static struct qcom_smmu *to_qcom_smmu(struct arm_smmu_device *smmu) { return container_of(smmu, struct qcom_smmu, smmu); @@ -549,6 +628,15 @@ static const struct qcom_smmu_match_data sdm845_smmu_500_data = { /* Also no debug configuration. */ }; + +static const struct qcom_smmu_match_data sm8550_smmu_500_impl0_data = { + .impl = &qcom_smmu_500_impl, + .adreno_impl = &qcom_adreno_smmu_500_impl, + .cfg = &qcom_smmu_impl0_cfg, + .actlrcfg = sm8550_apps_actlr_cfg, + .actlrcfg_gfx = sm8550_gfx_actlr_cfg, +}; + static const struct qcom_smmu_match_data qcom_smmu_500_impl0_data = { .impl = &qcom_smmu_500_impl, .adreno_impl = &qcom_adreno_smmu_500_impl, @@ -583,6 +671,7 @@ static const struct of_device_id __maybe_unused qcom_smmu_impl_of_match[] = { { .compatible = "qcom,sm8250-smmu-500", .data = &qcom_smmu_500_impl0_data }, { .compatible = "qcom,sm8350-smmu-500", .data = &qcom_smmu_500_impl0_data }, { .compatible = "qcom,sm8450-smmu-500", .data = &qcom_smmu_500_impl0_data }, + { .compatible = "qcom,sm8550-smmu-500", .data = &sm8550_smmu_500_impl0_data }, { .compatible = "qcom,smmu-500", .data = &qcom_smmu_500_impl0_data }, { } };
Add ACTLR data table for SM8550 along with support for same including SM8550 specific implementation operations. Signed-off-by: Bibek Kumar Patro <quic_bibekkum@quicinc.com> --- drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 89 ++++++++++++++++++++++ 1 file changed, 89 insertions(+) -- 2.17.1