Re: [PATCH v9 4/5] iommu/arm-smmu: add ACTLR data and support for SM8550

[Bibek Kumar Patro <quic_bibekkum@quicinc.com>]

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