Re: [PATCH v14 5/6] iommu/arm-smmu: add ACTLR data and support for SC7280

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

On 8/27/2024 6:17 PM, Will Deacon wrote:
> On Mon, Aug 26, 2024 at 04:33:24PM +0530, Bibek Kumar Patro wrote:
>>
>>
>> On 8/23/2024 9:29 PM, Will Deacon wrote:
>>> On Fri, Aug 16, 2024 at 11:12:58PM +0530, Bibek Kumar Patro wrote:
>>>> Add ACTLR data table for SC7280 along with support for
>>>> same including SC7280 specific implementation operations.
>>>>
>>>> Signed-off-by: Bibek Kumar Patro <quic_bibekkum@quicinc.com>
>>>> ---
>>>>    drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 58 +++++++++++++++++++++-
>>>>    1 file changed, 57 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>> index dc143b250704..a776c7906c76 100644
>>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>> @@ -31,6 +31,55 @@
>>>>    #define PREFETCH_MODERATE	(2 << PREFETCH_SHIFT)
>>>>    #define PREFETCH_DEEP		(3 << PREFETCH_SHIFT)
>>>>
>>>> +static const struct actlr_config sc7280_apps_actlr_cfg[] = {
>>>> +	{ 0x0800, 0x04e0, PREFETCH_DEFAULT | CMTLB },
>>>> +	{ 0x0900, 0x0402, PREFETCH_SHALLOW | CPRE | CMTLB },
>>>> +	{ 0x0901, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB },
>>>> +	{ 0x0d01, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB },
>>>> +	{ 0x1181, 0x0420, PREFETCH_DEEP | CPRE | CMTLB },
>>>> +	{ 0x1182, 0x0420, PREFETCH_DEEP | CPRE | CMTLB },
>>>> +	{ 0x1183, 0x0420, PREFETCH_DEEP | CPRE | CMTLB },
>>>> +	{ 0x1184, 0x0420, PREFETCH_DEEP | CPRE | CMTLB },
>>>> +	{ 0x1185, 0x0420, PREFETCH_DEEP | CPRE | CMTLB },
>>>> +	{ 0x1186, 0x0420, PREFETCH_DEEP | CPRE | CMTLB },
>>>> +	{ 0x1187, 0x0420, PREFETCH_DEEP | CPRE | CMTLB },
>>>> +	{ 0x1188, 0x0420, PREFETCH_DEEP | CPRE | CMTLB },
>>>> +	{ 0x1189, 0x0420, PREFETCH_DEEP | CPRE | CMTLB },
>>>> +	{ 0x118b, 0x0420, PREFETCH_DEEP | CPRE | CMTLB },
>>>> +	{ 0x118c, 0x0420, PREFETCH_DEEP | CPRE | CMTLB },
>>>> +	{ 0x118d, 0x0420, PREFETCH_DEEP | CPRE | CMTLB },
>>>> +	{ 0x118e, 0x0420, PREFETCH_DEEP | CPRE | CMTLB },
>>>> +	{ 0x118f, 0x0420, PREFETCH_DEEP | CPRE | CMTLB },
>>>> +	{ 0x2000, 0x0020, PREFETCH_DEFAULT | CMTLB },
>>>> +	{ 0x2040, 0x0000, PREFETCH_DEFAULT | CMTLB },
>>>> +	{ 0x2062, 0x0000, PREFETCH_DEFAULT | CMTLB },
>>>> +	{ 0x2080, 0x0020, PREFETCH_DEFAULT | CMTLB },
>>>> +	{ 0x20c0, 0x0020, PREFETCH_DEFAULT | CMTLB },
>>>> +	{ 0x2100, 0x0020, PREFETCH_DEFAULT | CMTLB },
>>>> +	{ 0x2140, 0x0000, PREFETCH_DEFAULT | CMTLB },
>>>> +	{ 0x2180, 0x0020, PREFETCH_SHALLOW | CPRE | CMTLB },
>>>> +	{ 0x2181, 0x0004, PREFETCH_SHALLOW | CPRE | CMTLB },
>>>> +	{ 0x2183, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB },
>>>> +	{ 0x2184, 0x0020, PREFETCH_SHALLOW | CPRE | CMTLB },
>>>> +	{ 0x2187, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB },
>>>> +};
>>>> +
>>>> +static const struct actlr_config sc7280_gfx_actlr_cfg[] = {
>>>> +	{ 0x0000, 0x07ff, PREFETCH_DEEP | CPRE | CMTLB },
>>>> +};
>>>
>>> It's Will "stuck record" Deacon here again to say that I don't think
>>> this data belongs in the driver.
>>>
>>
>> Hi Will,
>>
>> It will be difficult to reach a consensus here, with Robin and the DT folks
>> okay to keep it in the driver, while you believe it doesn't belong there.
>>
>> Robin, Rob, could you please share your thoughts on concluding the placement
>> of this prefetch data?
>>
>> As discussed earlier [1], the prefetch value for each client doesn’t define
>> the hardware topology and is implementation-defined register writes used by
>> the software driver.
> 
> It does reflect the hardware topology though, doesn't it? Those magic hex
> masks above refer to stream ids, so the table is hard-coding the prefetch
> values for particular matches.

That is correct in the sense that stream id is mapped to context bank
where these configurations are applied.
However the other part of it is implementation-defined register/values
for which community opinion was register/value kind of data, should not
belong to device tree and are not generally approved of.

Would also like to point out that the prefetch values are recommended
settings and doesn’t mean these are the only configuration which would
work for the soc.
So the SID-to-prefetch isn't strictly SoC defined but is a software
configuration, IMO.

> If I run on a different SoC configuration > with the same table, then the prefetch settings will be applied to the
> wrong devices. How is that not hardware topology?
> 

The configuration table is tied to SoC compatible string however as I
mentioned above, its basically a s/w recommended setting.
(using prefetch settings other than the recommended values e.g 
PREFECH_DEFAULT instead of PREFETCH_DEEP would not render the device 
unusable unlike changing stream-ids which can make it unusable).

Since it is implementation specific we cannot have a generic DT binding,
tying stream ids to these recommended settings.
Even with qcom specific binding due to dependency on implementation, not
sure if we would be able to maintain consistency.

So from maintenance perspective carrying these in driver appear to be
simpler/flexible. And if it doesn’t violate existing precedence, we
would prefer to carry it that way.

This parallels how _"QoS settings"_ are handled within the driver 
(similar to this example [1]).

[1]. 
https://lore.kernel.org/linux-arm-msm/20231030-sc8280xp-dpu-safe-lut-v1-1-6d485d7b428f@quicinc.com/#t

Thanks & regards,
Bibek

> WIll