Received-SPF: pass (google.com: domain of linux-kernel+bounces-98547-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <2b0d8c5b-7e79-41ff-bc57-003d1b947c16@quicinc.com>
Date: Mon, 11 Mar 2024 14:12:06 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
From: Bibek Kumar Patro <quic_bibekkum@quicinc.com>
Subject: Re: [PATCH v9 4/5] iommu/arm-smmu: add ACTLR data and support for
 SM8550
To: Will Deacon <will@kernel.org>
CC: <robin.murphy@arm.com>, <joro@8bytes.org>, <dmitry.baryshkov@linaro.org>,
        <konrad.dybcio@linaro.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>, <krzysztof.kozlowski+dt@linaro.org>,
        <saravanak@google.com>
References: <20240123144543.9405-1-quic_bibekkum@quicinc.com>
 <20240123144543.9405-5-quic_bibekkum@quicinc.com>
 <20240213134714.GC28926@willie-the-truck>
 <201fef09-50ab-436b-af63-4535c7510d15@quicinc.com>
 <20240221132101.GB7273@willie-the-truck>
Content-Language: en-US
In-Reply-To: <20240221132101.GB7273@willie-the-truck>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit


On 2/21/2024 6:51 PM, Will Deacon wrote:
> On Wed, Feb 21, 2024 at 02:25:26PM +0530, Bibek Kumar Patro wrote:
>> On 2/13/2024 7:17 PM, Will Deacon wrote:
>>> On Tue, Jan 23, 2024 at 08:15:42PM +0530, Bibek Kumar Patro 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 | 90 ++++++++++++++++++++++
>>>>    1 file changed, 90 insertions(+)
>>>>
>>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>> index 6004c6d9a7b2..db15b1eade97 100644
>>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>> @@ -23,6 +23,86 @@
>>>>
>>>>    #define CPRE			(1 << 1)
>>>>    #define CMTLB			(1 << 0)
>>>> +#define PREFETCH_SHIFT		8
>>>> +#define PREFETCH_DEFAULT	0
>>>> +#define PREFETCH_SHALLOW	(1 << PREFETCH_SHIFT)
>>>> +#define PREFETCH_MODERATE	(2 << PREFETCH_SHIFT)
>>>> +#define PREFETCH_DEEP		(3 << PREFETCH_SHIFT)
>>>> +#define PREFETCH_SWITCH_GFX	(5 << 3)
>>>> +
>>>> +static const struct actlr_config sm8550_apps_actlr_cfg[] = {
>>>> +	{ 0x18a0, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB },
>>>> +	{ 0x18e0, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB },
>>>> +	{ 0x0800, 0x0020, PREFETCH_DEFAULT | CMTLB },
>>>> +	{ 0x1800, 0x00c0, PREFETCH_DEFAULT | CMTLB },
>>>> +	{ 0x1820, 0x0000, PREFETCH_DEFAULT | CMTLB },
>>>> +	{ 0x1860, 0x0000, PREFETCH_DEFAULT | CMTLB },
>>>> +	{ 0x0c01, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
>>>> +	{ 0x0c02, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
>>>> +	{ 0x0c03, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
>>>> +	{ 0x0c04, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
>>>> +	{ 0x0c05, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
>>>> +	{ 0x0c06, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
>>>
>>> [...]
>>>
>>> Isn't this effectively hard-coding the topology of the SoC in the driver?
>>> Wouldn't it better describing higher-level prefetch properties in the DT
>>> nodes corresponding to the upstream devices?
>>
>> Since prefetch data stored in this table represent settings for the
>> ACTLR register, and doesn't exactly define the hardware (So in this
>> manner prefetch data won't exactly be a part of soc topology ?).
> 
> The first two columns of the table are StreamID/Mask pairs, no? How is that
> _not_ the SoC topology? I really think it would be better to define some
> high-level prefetch properties in the DT binding which can be put on the
> master nodes.
> 
>> So it seemed apt not to use the device tree for storing the prefetch
>> property. Hence we reverted from the DT approach (initial proposal in
>> RFC to piggyback on iommus property to store prefetch settings) back to use
>> driver for storing this data.
>>
>> Some drivers use the same approach for storing their platform specific
>> data. Examples being
>> drivers/phy/qualcomm/phy-qcom-qmp-combo.c
>> drivers/soc/qcom/llcc-qcom.c
>> These drivers were taken as reference for storing platform specific ACTLR
>> data.
> 
> I don't know anything about those drivers, but on the SMMU side we already
> have ways to describe the topology in the DT and the driver is using them,
> so I'm struggling to see the need to add these tables as well.
> 
> But as I said before, if Robin and the DT folks prefer this approach,
> then I won't get in the way.
> 

With the driver approach at the current state of patches, it has been 
ACKed by DT folks and it seems there has been no concern/objection from 
Robin till now.
So can this patch go ahead Will?
Let us know Robin of your opinion as well please.

Thanks & regards,
Bibek

> Will