linux-kernel - Re: [PATCH v2 6/7] dmaengine: sh: rz-dmac: Add RZ/V2H(P) support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAMuHMdVUr6Z1o6MhxOj18d8rwV8O-AJQxWFEpMT8pcvb=DHB3A@mail.gmail.com>
Date: Thu, 13 Feb 2025 15:19:31 +0100
From: Geert Uytterhoeven <geert@...ux-m68k.org>
To: Fabrizio Castro <fabrizio.castro.jz@...esas.com>
Cc: Vinod Koul <vkoul@...nel.org>, Magnus Damm <magnus.damm@...il.com>, 
	Philipp Zabel <p.zabel@...gutronix.de>, Wolfram Sang <wsa+renesas@...g-engineering.com>, 
	Biju Das <biju.das.jz@...renesas.com>, 
	Uwe Kleine-König <u.kleine-koenig@...libre.com>, 
	dmaengine@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-renesas-soc@...r.kernel.org, 
	Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
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@...esas.com> 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@...esas.com>
> ---
> 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.

>         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)...

> +};
> +
>  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.

>  };

> @@ -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?
Is the returned pdev valid?
Will rzv2h_icu_register_dma_req_ack() crash when dereferencing priv?

> +       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?

>
>         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@...ux-m68k.org

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