| Message ID | 20241222030355.2246-2-naoki@radxa.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | rockchip: Add support for RK3582 | expand |
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, M.
Hi Marc, On 12/22/24 18: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. I see. I'll drop this patch in v2. Best regards, -- FUKAUMI Naoki Radxa Computer (Shenzhen) Co., Ltd. > Thanks, > > M. >
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.
Dragan, On Sun, 22 Dec 2024 18:25:02 +0000, Dragan Simic <dsimic@manjaro.org> wrote: > > 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. 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. > > 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. You clearly haven't read what I wrote. Or rather, you read what you wanted to read, and ignored half of it. > > 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? No. We have everything we need. Believe it or not, I actually know what I'm talking about. I know, this is surprising. I surprise myself sometimes. > Could you, please, advise on how to move forward with this? I'm I already have. > willing to implement the required patches, but I'd prefer to reduce > the possible back-and-forth on them, to save everyone's time. May I suggest that you read my email again? How about grepping through the upstream DT collection and (shock, horror) look at the imx95.dtsi file, which suffers from the same braindead behaviour as the RK stuff? For clarity, let me paste it here again, and add some emphasis for extra clarity: > > Please use the relevant property for that purpose ("dma-noncoherent") > > at the distributor and ITS levels. We're not adding extra compatibles ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Now, please go look at the code for real this time, appreciate how the "dma-noncoherent" property placed at the distributor *AND* ITS levels combine to give you the effects the hardware requires. To sum it up: the standard properties and the Rockchip hacks are strictly equivalent, there is no need for anything extra, and I stand by my NAK on this very patch. M.
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;
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(-)