Received-SPF: pass (google.com: domain of linux-kernel+bounces-3827-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <8338db1e-0216-4fc5-b6ab-ddf43adf3648@linaro.org>
Date: Mon, 18 Dec 2023 16:21:00 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v4 3/5] iommu/arm-smmu: add ACTLR data and support for
 SM8550
Content-Language: en-GB
To: Bibek Kumar Patro <quic_bibekkum@quicinc.com>,
 Konrad Dybcio <konrad.dybcio@linaro.org>, Robin Murphy <robin.murphy@arm.com>
Cc: will@kernel.org, joro@8bytes.org, jsnitsel@redhat.com,
 quic_bjorande@quicinc.com, mani@kernel.org, quic_eberman@quicinc.com,
 robdclark@chromium.org, u.kleine-koenig@pengutronix.de, robh@kernel.org,
 vladimir.oltean@nxp.com, quic_pkondeti@quicinc.com,
 quic_molvera@quicinc.com, linux-arm-msm@vger.kernel.org,
 linux-arm-kernel@lists.infradead.org, iommu@lists.linux.dev,
 linux-kernel@vger.kernel.org, qipl.kernel.upstream@quicinc.com
References: <20231215101827.30549-1-quic_bibekkum@quicinc.com>
 <20231215101827.30549-4-quic_bibekkum@quicinc.com>
 <CAA8EJppcsr1sbeD1fK0nZ+rASABNcetBK3yMvaP7OiA4JPwskw@mail.gmail.com>
 <c9493c5f-ccf8-4e21-b00c-5fbc2a5f2edb@quicinc.com>
 <b7f2bbf9-fb5a-430d-aa32-3a220b46c2f0@arm.com>
 <1eee8bae-59f0-4066-9d04-8c3a5f750d3a@linaro.org>
 <42d627af-164b-4b50-973e-fa71d86cb84c@linaro.org>
 <aa8b2ccd-33da-404b-9a93-3d88cf63ec77@quicinc.com>
From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
In-Reply-To: <aa8b2ccd-33da-404b-9a93-3d88cf63ec77@quicinc.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

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).

-- 
With best wishes
Dmitry