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
