Re: [PATCH 1/3] irqchip/gic-v3: Enable Rockchip 3588001 erratum workaround for RK3582

[Dragan Simic <dsimic@manjaro.org>]

Hello Marc,

On 2024-12-22 10:04, Marc Zyngier wrote:
> On Sun, 22 Dec 2024 03:03:53 +0000,
> FUKAUMI Naoki <naoki@radxa.com> wrote:
>> 
>> Rockchip RK3582 is a scaled down version of Rockchip RK3588(S). Apply
>> Rockchip 3588001 erratum workaround to RK3582.
>> 
>> Signed-off-by: FUKAUMI Naoki <naoki@radxa.com>
>> ---
>>  drivers/irqchip/irq-gic-v3-its.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/irqchip/irq-gic-v3-its.c 
>> b/drivers/irqchip/irq-gic-v3-its.c
>> index 92244cfa0464..c59ce9332dc0 100644
>> --- a/drivers/irqchip/irq-gic-v3-its.c
>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>> @@ -4861,7 +4861,8 @@ static bool __maybe_unused 
>> its_enable_rk3588001(void *data)
>>  {
>>  	struct its_node *its = data;
>> 
>> -	if (!of_machine_is_compatible("rockchip,rk3588") &&
>> +	if (!of_machine_is_compatible("rockchip,rk3582") &&
>> +	    !of_machine_is_compatible("rockchip,rk3588") &&
>>  	    !of_machine_is_compatible("rockchip,rk3588s"))
>>  		return false;
>> 
> 
> Please use the relevant property for that purpose ("dma-noncoherent")
> at the distributor and ITS levels. We're not adding extra compatibles
> for this anymore, and you might as well fix the core dtsi to expose
> such property.

Thanks for your response.

After a more detailed look into drivers/irqchip/irq-gic-v3-its.c,
it seems that relying on the "dma-noncoherent" DT property may not
be equivalent to adding another compatible check.  Here are a few
quotations from irq-gic-v3-its.c, to illustrate this better:

4746 static bool __maybe_unused its_enable_rk3588001(void *data)
4747 {
4748         struct its_node *its = data;
4749
4750         if (!of_machine_is_compatible("rockchip,rk3588") &&
4751             !of_machine_is_compatible("rockchip,rk3588s"))
4752                 return false;
4753
4754         its->flags |= ITS_FLAGS_FORCE_NON_SHAREABLE;
4755         gic_rdists->flags |= RDIST_FLAGS_FORCE_NON_SHAREABLE;
4756
4757         return true;
4758 }
4759
4760 static bool its_set_non_coherent(void *data)
4761 {
4762         struct its_node *its = data;
4763
4764         its->flags |= ITS_FLAGS_FORCE_NON_SHAREABLE;
4765         return true;
4766 }

4814 #ifdef CONFIG_ROCKCHIP_ERRATUM_3588001
4815         {
4816                 .desc   = "ITS: Rockchip erratum RK3588001",
4817                 .iidr   = 0x0201743b,
4818                 .mask   = 0xffffffff,
4819                 .init   = its_enable_rk3588001,
4820         },
4821 #endif
4822         {
4823                 .desc   = "ITS: non-coherent attribute",
4824                 .property = "dma-noncoherent",
4825                 .init   = its_set_non_coherent,
4826         },

As visible above, using the "dma-noncoherent" DT property results
in not setting the RDIST_FLAGS_FORCE_NON_SHAREABLE flag, which the
its_enable_rk3588001() function does.  In other words, it doesn't
seem that "dma-noncoherent" is a "drop-in" replacement for adding
yet another compatible for the RK3582.

Modifying the current behavior of the "dma-noncoherent" DT property
doesn't seem like an option, because it's already used in a couple
of board dts(i) files.  Should we introduce another DT property,
perhaps "dma-noncoherent-rdist" or something similar?

Could you, please, advise on how to move forward with this?  I'm
willing to implement the required patches, but I'd prefer to reduce
the possible back-and-forth on them, to save everyone's time.