[permalink] [raw]

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

On 12/19/2023 4:14 PM, Dmitry Baryshkov wrote:
> On Tue, 19 Dec 2023 at 12:37, Bibek Kumar Patro
> <[email protected]> wrote:
>>
>>
>>
>> On 12/19/2023 3:51 PM, Dmitry Baryshkov wrote:
>>> On Tue, 19 Dec 2023 at 10:25, Bibek Kumar Patro
>>> <[email protected]> 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
>>>>>>>>>>> <[email protected]> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> Add ACTLR data table for SM8550 along with support for
>>>>>>>>>>>> same including SM8550 specific implementation operations.
>>>>>>>>>>>>
>>>>>>>>>>>> Signed-off-by: Bibek Kumar Patro <[email protected]>
>>>>>>>>>>>> ---
>>>>>>>>>>>> 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
>>>>
>>>
>>>
>
>
>