Received-SPF: pass (google.com: domain of linux-kernel+bounces-3036-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <7caf4e99-49fd-4a81-9a97-4d67675a5c66@quicinc.com>
Date: Mon, 18 Dec 2023 11:47:30 +0530
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-US
To: Konrad Dybcio <konrad.dybcio@linaro.org>,
        Robin Murphy
	<robin.murphy@arm.com>,
        Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
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>
From: Bibek Kumar Patro <quic_bibekkum@quicinc.com>
In-Reply-To: <1eee8bae-59f0-4066-9d04-8c3a5f750d3a@linaro.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit


On 12/16/2023 5:33 AM, 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
> 

Thanks for the suggestion with these mods.
Let me try out the suggested way and once tested will post this in next
version.

Thanks,
Bibek

> Konrad