Messages in this thread | | | Date | Tue, 19 Dec 2023 17:09:13 +0530 | Subject | Re: [PATCH v4 3/5] iommu/arm-smmu: add ACTLR data and support for SM8550 | From | Bibek Kumar Patro <> |
| |
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 >>>> >>> >>> > > >
| |