Re: [PATCH 2/2] irqchip/gic-v3-its: Make "dma-noncoherent" preferred for RK358x errata

From: Dragan Simic
Date: Fri Dec 27 2024 - 12:09:04 EST

Hello Marc,

On 2024-12-27 17:46, Marc Zyngier wrote:
On Fri, 27 Dec 2024 15:42:24 +0000,
Dragan Simic <dsimic@xxxxxxxxxxx> wrote:

The preferred way to denote hardware with non-coherent DMA is to use the
"dma-noncoherent" DT property, [1] instead of relying on the compatibles. [2]
Alas, older versions of the Rockchip RK3588 and RK3588S SoC dts(i) files
failed to specify this DT property, which means that checking the compatibles
remains required for backward SoC dts(i) compatibility.

Let's have the Rockchip 3588001 hardware errata handled the preferred way,
with newer versions of the Rockchip RK3588, RK3588S and RK3582 SoC dts(i)
files that properly specify the "dma-noncoherent" DT properties at both the
GIC redistributor and the GIC ITS levels, while falling back to checking the
compatibles for backward RK3588 and RK3588S SoC dts(i) compatibility.

[1] Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.yaml
[2] https://lore.kernel.org/linux-rockchip/86msgoozqa.wl-maz@xxxxxxxxxx/

Cc: Marc Zyngier <maz@xxxxxxxxxx>
Cc: FUKAUMI Naoki <naoki@xxxxxxxxx>
Signed-off-by: Dragan Simic <dsimic@xxxxxxxxxxx>
---
drivers/irqchip/irq-gic-v3-its.c | 12 ++++++++++++
1 file changed, 12 insertions(+)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index fdec478ba5e7..982dcbb30f39 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -4747,6 +4747,18 @@ static bool __maybe_unused its_enable_rk3588001(void *data)
{
struct its_node *its = data;

+ /*
+ * The preferred way to denote hardware with non-coherent DMA is to use
+ * the "dma-noncoherent" DT property, which the older RK3588 SoC dts(i)
+ * files failed to specify, relying on the compatibles instead.
+ *
+ * Thus, check for the presence of "dma-noncoherent" DT property first,
+ * to let the hardware quirk be handled the preferred way, and fall back
+ * to checking the compatibles for backward dts(i) compatibility.
+ */
+ if (!of_dma_is_coherent(to_of_node(its->fwnode_handle)))
+ return false;
+
if (!of_machine_is_compatible("rockchip,rk3588") &&
!of_machine_is_compatible("rockchip,rk3588s"))
return false;


I'm sorry, but this patch serves no purpose. We already have a
workaround in place, and we don't need to handle it *twice*.

Thanks for reviewing this series so quickly!

Regarding handling the workaround twice, I agree that this patch
is somewhat redundant, but without this patch and with the "dma-
noncoherent" DT properties in the proper places we'll actually
end up with some kind of double hanling.

More precisely, RDIST_FLAGS_FORCE_NON_SHAREABLE will be set twice,
which doesn't do any harm, but it's something that made me twitch
a bit and produce this patch.

Worse, it is actively harmful. A DT that would only advertise the
property on the ITS would result in a broken system, while it would
work correctly with the current code.

Indeed, that possible issue bugged me as well. Though, we can
probably rest assured that the RK3588 dtsi files are changed to
include the "dma-noncoherent" DT properties correctly.

Additionally, look at what of_dma_is_coherent() does, and how it will
fetch a property that is potentially completely unrelated to the GIC
node, or eventually return some random default. The binding documents
the dma-noncoherent property as part of the relevant nodes, and not
anywhere else.

That's a possibility, but I think that the same argument about the
RK3588 dtsi correctness may also apply here.

In the end, this is just dead code, and potentially worse. So while
I'm rather supportive of your first patch in this series as it allows
OSs other than Linux to *maybe* avoid this hack, the Linux driver
doesn't need any change.

I'm happy to drop this patch, but I just wanted to describe the
rationale behind it, as explained above.