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

[Dragan Simic <dsimic@manjaro.org>]

On 2024-12-23 11:11, Dragan Simic wrote:
> 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.

For future reference, here's the link to the above-mentioned
small patch series on the linux-rockchip mailing list:

https://lore.kernel.org/linux-rockchip/cover.1735313870.git.dsimic@manjaro.org/T/#u