Re: [PATCH v4 2/2] cache: add SMCCC-backed cache invalidate provider

[Andre Przywara <andre.przywara@arm.com>]

Hi,

On 10/15/26 09:43, Bence Csókás wrote:
> Hi,
> 
> On 2026. 10. 14. 17:38, Andre Przywara wrote:
>>> +static int smccc_cache_probe(struct arm_smccc_device *sdev)
>>> +{
>>> +    struct arm_smccc_res res = {};
>>> +    struct smccc_cache *cache;
>>> +    int ret;
>>> +
>>> +    arm_smccc_1_1_invoke(sdev->func_id, &res);
>>
>> I think you should provide the explicit function ID name here, and not 
>> rely on which function exactly is used to probe for the interface's 
>> existence.
> 
> I thought that was the point of the matcher, that you could in theory

But that probe() function here is not a "matcher" or a check function, 
that is done in the SMCCC bus generic code, using a function ID that has 
no side effects and does not return -1.

The reason for this SMC call here is to learn the attributes, they are 
used below to fill the latency and rate_limit fields in the struct.
So we should then make this explicit, I think, by using the 
ARM_SMCCC_ARCH_CLEAN_INV_MEMREGION_ATTRIBUTES name.
> assign the same probe handler to any function ID? For example if there's 
> some FW that implements this (or a compatible) function, but at a 
> different ID, you would only update the match table? At least that's how 
> most subsystems (I2C, PCI, USB) do it.

Not sure I understand this, this is a driver specifically for the 
CLEANINV_MEM_REGION SMCCC interface, I don't see how you would reuse 
that, or plug any other interfaces to it.

>>> +    if ((s32)res.a0)
>>> +        return -ENODEV;
>>> +
>>> +    cache = cache_coherency_ops_instance_alloc(&smccc_cache_ops,
>>> +                           struct smccc_cache, cci);
>>> +    if (!cache)
>>> +        return -ENOMEM;
>>> +
>>> +    mutex_init(&cache->lock);
>>> +    cache->latency_us = lower_32_bits(res.a2);
>>> +    cache->rate_limit = res.a3;
>>
>> According to the spec you would need lower_32_bits() for a3 as well - 
>> if we are going to use this value. But I don't think we need to, see 
>> above.
> Where does it say that? My copy of DEN 0028 1.7 H, released 2026/Jul/24, 
> only says the following:
> 
>    X3 (uint64_t attr_2)
>      Rate Limit: The maximum number of SMCCCC_ARCH_CLEAN_INV_MEMREGION 
> calls allowed per second. If set to 0, it indicates that this field is 
> not reported by the implementation.
> 
> See my second-to-last comment on v2 of this patch:
> 
> https://lore.kernel.org/lkml/591e7ac9-310e-40f4-ab22-161d6ea8f487@arm.com/
> 
> Maybe I have an outdated copy already? ;_;

No, I had! Mine said v1.7H, but is was an early version, which had the 
upper 32 bits of rate_limit as reserved.
Indeed in the later public version it's a 64-bit field, so ignore my 
comment. Sorry for the noise!

Cheers,
Andre