Re: [PATCH v4 3/5] iommu/arm-smmu: add ACTLR data and support for SM8550

[Bibek Kumar Patro <quic_bibekkum@quicinc.com>]

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