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