Re: [PATCH v2 4/8] dma: dmamux: Introduce RZN1 DMA router support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Miquel,

On Tue, Feb 22, 2022 at 11:35 AM Miquel Raynal
<miquel.raynal@xxxxxxxxxxx> wrote:
> The Renesas RZN1 DMA IP is a based on a DW core, with eg. an additional
> dmamux register located in the system control area which can take up to
> 32 requests (16 per DMA controller). Each DMA channel can be wired to
> two different peripherals.
>
> We need two additional information from the 'dmas' property: the channel
> (bit in the dmamux register) that must be accessed and the value of the
> mux for this channel.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@xxxxxxxxxxx>

Thanks for your patch!

> --- /dev/null
> +++ b/drivers/dma/dw/dmamux.c

rzn1-dmamux.c?

> @@ -0,0 +1,167 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2022 Schneider-Electric
> + * Author: Miquel Raynal <miquel.raynal@xxxxxxxxxxx
> + * Based on TI crossbar driver written by Peter Ujfalusi <peter.ujfalusi@xxxxxx>
> + */
> +#include <linux/slab.h>
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/list.h>
> +#include <linux/io.h>
> +#include <linux/of_address.h>
> +#include <linux/of_device.h>
> +#include <linux/of_dma.h>
> +#include <linux/soc/renesas/r9a06g032-sysctrl.h>
> +
> +#define RZN1_DMAMUX_LINES      64

Unused. But using it wouldn't hurt, I guess?

> +static void *rzn1_dmamux_route_allocate(struct of_phandle_args *dma_spec,
> +                                       struct of_dma *ofdma)
> +{
> +       struct platform_device *pdev = of_find_device_by_node(ofdma->of_node);
> +       struct rzn1_dmamux_data *dmamux = platform_get_drvdata(pdev);
> +       struct rzn1_dmamux_map *map;
> +       unsigned int master, chan, val;
> +       u32 mask;
> +       int ret;
> +
> +       map = kzalloc(sizeof(*map), GFP_KERNEL);
> +       if (!map)
> +               return ERR_PTR(-ENOMEM);
> +
> +       if (dma_spec->args_count != 6)
> +               return ERR_PTR(-EINVAL);
> +
> +       chan = dma_spec->args[0];
> +       map->req_idx = dma_spec->args[4];
> +       val = dma_spec->args[5];
> +       dma_spec->args_count -= 2;
> +
> +       if (chan >= dmamux->dmac_requests) {
> +               dev_err(&pdev->dev, "Invalid DMA request line: %d\n", chan);

%u

> +               return ERR_PTR(-EINVAL);
> +       }
> +
> +       if (map->req_idx >= dmamux->dmamux_requests ||
> +           map->req_idx % dmamux->dmac_requests != chan) {

The reliance on .dmac_requests (i.e. "dma-requests" in the parent
DMA controller DT node) looks fragile to me.  Currently there are two
masters, each providing 16 channels, hence using all 2 x 16 =
32 bits in the DMAMUX register.
What if a variant used the same mux, and the same 16/16 split, but
the parent DMACs don't have all channels available?
I think it would be safer to hardcode this as 16 (using a #define, ofc).

> +               dev_err(&pdev->dev, "Invalid MUX request line: %d\n", map->req_idx);

%u

> +               return ERR_PTR(-EINVAL);
> +       }
> +
> +       /* The of_node_put() will be done in the core for the node */
> +       master = map->req_idx >= dmamux->dmac_requests ? 1 : 0;

The name "master" confused me: initially I thought it was used as a
boolean flag, but it really is the index of the parent DMAC.

> +       dma_spec->np = of_parse_phandle(ofdma->of_node, "dma-masters", master);
> +       if (!dma_spec->np) {
> +               dev_err(&pdev->dev, "Can't get DMA master\n");
> +               return ERR_PTR(-EINVAL);
> +       }
> +
> +       dev_dbg(&pdev->dev, "Mapping DMAMUX request %u to DMAC%u request %u\n",
> +               map->req_idx, master, chan);
> +
> +       mask = BIT(map->req_idx);
> +       mutex_lock(&dmamux->lock);
> +       dmamux->used_chans |= mask;
> +       ret = r9a06g032_sysctrl_set_dmamux(mask, val ? mask : 0);
> +       mutex_unlock(&dmamux->lock);
> +       if (ret) {
> +               rzn1_dmamux_free(&pdev->dev, map);
> +               return ERR_PTR(ret);
> +       }
> +
> +       return map;
> +}
> +
> +static const struct of_device_id rzn1_dmac_match[] __maybe_unused = {
> +       { .compatible = "renesas,rzn1-dma" },
> +       {},
> +};
> +
> +static int rzn1_dmamux_probe(struct platform_device *pdev)
> +{
> +       struct device_node *mux_node = pdev->dev.of_node;
> +       const struct of_device_id *match;
> +       struct device_node *dmac_node;
> +       struct rzn1_dmamux_data *dmamux;
> +
> +       dmamux = devm_kzalloc(&pdev->dev, sizeof(*dmamux), GFP_KERNEL);
> +       if (!dmamux)
> +               return -ENOMEM;
> +
> +       mutex_init(&dmamux->lock);
> +
> +       dmac_node = of_parse_phandle(mux_node, "dma-masters", 0);
> +       if (!dmac_node)
> +               return dev_err_probe(&pdev->dev, -ENODEV, "Can't get DMA master node\n");
> +
> +       match = of_match_node(rzn1_dmac_match, dmac_node);
> +       if (!match) {
> +               of_node_put(dmac_node);
> +               return dev_err_probe(&pdev->dev, -EINVAL, "DMA master is not supported\n");
> +       }
> +
> +       if (of_property_read_u32(dmac_node, "dma-requests", &dmamux->dmac_requests)) {
> +               of_node_put(dmac_node);
> +               return dev_err_probe(&pdev->dev, -EINVAL, "Missing DMAC requests information\n");
> +       }
> +
> +       of_node_put(dmac_node);

When hardcoding dmac_requests to 16, I guess the whole dmac_node
handling can be removed?

> +
> +       if (of_property_read_u32(mux_node, "dma-requests", &dmamux->dmamux_requests)) {

Don't obtain from DT, but fix to 32?

> +               return dev_err_probe(&pdev->dev, -EINVAL, "Missing mux requests information\n");
> +       }
> +
> +       dmamux->dmarouter.dev = &pdev->dev;
> +       dmamux->dmarouter.route_free = rzn1_dmamux_free;
> +
> +       platform_set_drvdata(pdev, dmamux);
> +
> +       return of_dma_router_register(mux_node, rzn1_dmamux_route_allocate,
> +                                     &dmamux->dmarouter);
> +}

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



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux