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

[Robin Murphy <robin.murphy@arm.com>]

On 30/08/2024 11:00 am, Bibek Kumar Patro wrote:
> 
> 
> 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.

What's particularly confusing is that most of the IDs encoded here don't 
actually seem to line up with what's in the respective SoC DTSIs...

However by this point I'm wary of whether we've lost sight of *why* 
we're doing this, and that we're deep into begging the question of 
whether identifying devices by StreamID is the right thing to do in the 
first place. For example, as best I can tell from a quick skim, we have 
over 2 dozen lines of data here which all serve the exact same purpose 
of applying PREFETCH_DEEP | CPRE | CMTLB to instances of 
"qcom,fastrpc-compute-cb". In general it seems unlikely that the same 
device would want wildly different prefetch settings across different 
SoCs, or even between different instances in the same SoC, so I'm really 
coming round to the conclusion that this data would probably be best 
handled as an extension of the existing qcom_smmu_client_of_match mechanism.

Thanks,
Robin.

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