Hi Geert,
Thanks for your feedback!
> From: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> Sent: 13 February 2025 14:20
> Subject: Re: [PATCH v2 6/7] dmaengine: sh: rz-dmac: Add RZ/V2H(P) support
>
> Hi Fabrizio,
>
> On Wed, 12 Feb 2025 at 23:13, Fabrizio Castro
> <fabrizio.castro.jz@xxxxxxxxxxx> wrote:
> > The DMAC IP found on the Renesas RZ/V2H(P) family of SoCs is
> > similar to the version found on the Renesas RZ/G2L family of
> > SoCs, but there are some differences:
> > * It only uses one register area
> > * It only uses one clock
> > * It only uses one reset
> > * Instead of using MID/IRD it uses REQ NO/ACK NO
> > * It is connected to the Interrupt Control Unit (ICU)
> > * On the RZ/G2L there is only 1 DMAC, on the RZ/V2H(P) there are 5
> >
> > Add specific support for the Renesas RZ/V2H(P) family of SoC by
> > tackling the aforementioned differences.
> >
> > Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@xxxxxxxxxxx>
> > ---
> > v1->v2:
> > * Switched to new macros for minimum values.
>
> Thanks for the update!
>
> > --- a/drivers/dma/sh/Kconfig
> > +++ b/drivers/dma/sh/Kconfig
> > @@ -53,6 +53,7 @@ config RZ_DMAC
> > depends on ARCH_R7S72100 || ARCH_RZG2L || COMPILE_TEST
> > select RENESAS_DMA
> > select DMA_VIRTUAL_CHANNELS
> > + select RENESAS_RZV2H_ICU
>
> This enables RENESAS_RZV2H_ICU unconditionally, while it is only
> really needed on RZ/V2H, and not on other arm64 SoCs, or on arm32
> or riscv SoCs.
Good point, I'll follow up on this on your other email.
>
> > help
> > This driver supports the general purpose DMA controller typically
> > found in the Renesas RZ SoC variants.
> > diff --git a/drivers/dma/sh/rz-dmac.c b/drivers/dma/sh/rz-dmac.c
> > index d7a4ce28040b..24a8c6a337d5 100644
> > --- a/drivers/dma/sh/rz-dmac.c
> > +++ b/drivers/dma/sh/rz-dmac.c
> > @@ -14,6 +14,7 @@
> > #include <linux/dmaengine.h>
> > #include <linux/interrupt.h>
> > #include <linux/iopoll.h>
> > +#include <linux/irqchip/irq-renesas-rzv2h.h>
> > #include <linux/list.h>
> > #include <linux/module.h>
> > #include <linux/of.h>
> > @@ -28,6 +29,11 @@
> > #include "../dmaengine.h"
> > #include "../virt-dma.h"
> >
> > +enum rz_dmac_type {
> > + RZ_DMAC_RZG2L,
> > + RZ_DMAC_RZV2H,
>
> So basically these mean !has_icu respectively has_icu (more below)...
Yes.
>
> > +};
> > +
> > enum rz_dmac_prep_type {
> > RZ_DMAC_DESC_MEMCPY,
> > RZ_DMAC_DESC_SLAVE_SG,
> > @@ -85,20 +91,32 @@ struct rz_dmac_chan {
> > struct rz_lmdesc *tail;
> > dma_addr_t base_dma;
> > } lmdesc;
> > +
> > + /* RZ/V2H ICU related signals */
> > + u16 req_no;
> > + u8 ack_no;
>
> This could be an anonymous union with mid_rid, as mid_rid is
> mutually-exclusive with req_no and ack_no.
Indeed, I'll adjust accordingly.
>
> > };
>
> > @@ -824,6 +907,40 @@ static int rz_dmac_chan_probe(struct rz_dmac *dmac,
> > return 0;
> > }
> >
> > +static int rz_dmac_parse_of_icu(struct device *dev, struct rz_dmac *dmac)
> > +{
> > + struct device_node *icu_np, *np = dev->of_node;
> > + struct of_phandle_args args;
> > + uint32_t dmac_index;
> > + int ret;
> > +
> > + ret = of_parse_phandle_with_fixed_args(np, "renesas,icu", 1, 0, &args);
> > + if (ret)
> > + return ret;
> > +
> > + icu_np = args.np;
> > + dmac_index = args.args[0];
> > +
> > + if (dmac_index > RZV2H_MAX_DMAC_INDEX) {
> > + dev_err(dev, "DMAC index %u invalid.\n", dmac_index);
> > + ret = -EINVAL;
> > + goto free_icu_np;
> > + }
> > +
> > + dmac->icu.pdev = of_find_device_by_node(icu_np);
>
> What if the DMAC is probed before the ICU?
This doesn't look like a possible scenario, as irqchips are initialized very early.
> Is the returned pdev valid?
> Will rzv2h_icu_register_dma_req_ack() crash when dereferencing priv?
Even though it doesn't seem possible for the ICU driver to get probed after the DMAC
driver, I have still looked into possible ways your comment could apply, and I have
found one, although it can't really happen in practice, as the system will hang before
getting there.
If the probing of the ICU driver _fails_, then of_find_device_by_node() returns a valid
pointer. At some point we call into rzv2h_icu_register_dma_req_ack(), and the first time
we do anything with `priv` we deal with a NULL pointer.
However, if the probing of the ICU driver fails, the system hangs very early on because
the ICU is the interrupt parent of the pintctrl node.
In order to see the failure I have described I had to take `interrupt-parent = <&icu>;`
out of the pinctrl node, on top of manually make the ICU driver fail.
If the ICU driver fails its initialization the system is gone, and not because of DMAC,
therefore I'll leave this bit unchanged for the next version of the series.
>
> > + if (!dmac->icu.pdev) {
> > + ret = -ENODEV;
> > + goto free_icu_np;
> > + }
> > +
> > + dmac->icu.dmac_index = dmac_index;
> > +
> > +free_icu_np:
> > + of_node_put(icu_np);
> > +
> > + return ret;
> > +}
> > +
> > static int rz_dmac_parse_of(struct device *dev, struct rz_dmac *dmac)
> > {
> > struct device_node *np = dev->of_node;
> > @@ -859,6 +976,7 @@ static int rz_dmac_probe(struct platform_device *pdev)
> >
> > dmac->dev = &pdev->dev;
> > platform_set_drvdata(pdev, dmac);
> > + dmac->type = (enum rz_dmac_type)of_device_get_match_data(dmac->dev);
>
> (uintptr_t)
>
> But as "renesas,icu" is a required property for RZ/V2H, perhaps you
> can devise the has_icu flag at runtime?
I'll switch to using the has_icu flag at runtime.
Thanks!
Cheers,
Fab
>
> >
> > ret = rz_dmac_parse_of(&pdev->dev, dmac);
> > if (ret < 0)
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
> -- Linus Torvalds
