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

[Dragan Simic <dsimic@manjaro.org>]

On 2024-12-23 10:29, Marc Zyngier wrote:
> On Mon, 23 Dec 2024 06:10:21 +0000,
> Dragan Simic <dsimic@manjaro.org> wrote:
>> On 2024-12-23 00:16, Marc Zyngier wrote:
>> > On Sun, 22 Dec 2024 18:25:02 +0000,
>> > Dragan Simic <dsimic@manjaro.org> wrote:
>> >> 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.
>> >
>> > It is. My email makes it plain what needs doing.
>> >
>> >> 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         },
>> >
>> > Nothing tickles me more than having my own work being thrown back at
>> > me.
>> 
>> I'm sorry, that wasn't my intention.  I just wanted to make
>> referencing to what I was talking about a bit easier.  Though,
>> I now see that I was wrong, and I apologize for the noise.
> 
> No need to apologise. Just understand that the way you approached the
> discussion was suboptimal. Next time, just ask how the proposed
> solution works, rather than asserting that it doesn't.

Thanks.  Indeed, the way I approached it was waaay suboptimal.
I just wanted to clarify that it was an honest mistake resulting
from not looking at the code carefully enough, nothing else.

> Hopefully we can move on and you and Naoki can come up with a set of
> patches that does the right thing.

Of course.  I've already prepared a small patch series that,
hopefully, does the right thing when it comes to the Rockchip
3588001 errata.  I'll submit it soon, after I check the patches
a bit further.