Re: [PATCH v8 5/9] dmaengine: dw: dmamux: Introduce RZN1 DMA router support

[Vinod Koul <vkoul@kernel.org>]

On 06-04-22, 18:18, Miquel Raynal wrote:
> The Renesas RZN1 DMA IP is 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@bootlin.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/dma/dw/Kconfig       |   9 ++
>  drivers/dma/dw/Makefile      |   2 +
>  drivers/dma/dw/rzn1-dmamux.c | 157 +++++++++++++++++++++++++++++++++++
>  3 files changed, 168 insertions(+)
>  create mode 100644 drivers/dma/dw/rzn1-dmamux.c
> 
> diff --git a/drivers/dma/dw/Kconfig b/drivers/dma/dw/Kconfig
> index db25f9b7778c..a9828ddd6d06 100644
> --- a/drivers/dma/dw/Kconfig
> +++ b/drivers/dma/dw/Kconfig
> @@ -16,6 +16,15 @@ config DW_DMAC
>  	  Support the Synopsys DesignWare AHB DMA controller. This
>  	  can be integrated in chips such as the Intel Cherrytrail.
>  
> +config RZN1_DMAMUX
> +	tristate "Renesas RZ/N1 DMAMUX driver"
> +	depends on DW_DMAC
> +	depends on ARCH_RZN1 || COMPILE_TEST
> +	help
> +	  Support the Renesas RZ/N1 DMAMUX which is located in front of
> +	  the Synopsys DesignWare AHB DMA controller located on Renesas
> +	  SoCs.
> +
>  config DW_DMAC_PCI
>  	tristate "Synopsys DesignWare AHB DMA PCI driver"
>  	depends on PCI
> diff --git a/drivers/dma/dw/Makefile b/drivers/dma/dw/Makefile
> index a6f358ad8591..e1796015f213 100644
> --- a/drivers/dma/dw/Makefile
> +++ b/drivers/dma/dw/Makefile
> @@ -9,3 +9,5 @@ dw_dmac-$(CONFIG_OF)		+= of.o
>  
>  obj-$(CONFIG_DW_DMAC_PCI)	+= dw_dmac_pci.o
>  dw_dmac_pci-y			:= pci.o
> +
> +obj-$(CONFIG_RZN1_DMAMUX)	+= rzn1-dmamux.o
> diff --git a/drivers/dma/dw/rzn1-dmamux.c b/drivers/dma/dw/rzn1-dmamux.c
> new file mode 100644
> index 000000000000..5f878a55158f
> --- /dev/null
> +++ b/drivers/dma/dw/rzn1-dmamux.c
> @@ -0,0 +1,157 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2022 Schneider-Electric
> + * Author: Miquel Raynal <miquel.raynal@bootlin.com
> + * Based on TI crossbar driver written by Peter Ujfalusi <peter.ujfalusi@ti.com>
> + */
> +#include <linux/of_device.h>
> +#include <linux/of_dma.h>
> +#include <linux/slab.h>
> +#include <linux/soc/renesas/r9a06g032-sysctrl.h>
> +
> +#define RZN1_DMAMUX_LINES 64
> +#define RZN1_DMAMUX_MAX_LINES 16
> +
> +struct rzn1_dmamux_data {
> +	struct dma_router dmarouter;
> +	u32 used_chans;
> +	struct mutex lock;
> +};
> +
> +struct rzn1_dmamux_map {
> +	unsigned int req_idx;
> +};
> +
> +static void rzn1_dmamux_free(struct device *dev, void *route_data)
> +{
> +	struct rzn1_dmamux_data *dmamux = dev_get_drvdata(dev);
> +	struct rzn1_dmamux_map *map = route_data;
> +
> +	dev_dbg(dev, "Unmapping DMAMUX request %u\n", map->req_idx);
> +
> +	mutex_lock(&dmamux->lock);
> +	dmamux->used_chans &= ~BIT(map->req_idx);
> +	mutex_unlock(&dmamux->lock);

Why not use idr or bitmap for this. Hint: former does locking as well

> +
> +	kfree(map);
> +}
> +
> +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 dmac_idx, chan, val;
> +	u32 mask;
> +	int ret;
> +
> +	if (dma_spec->args_count != 6)

magic

> +		return ERR_PTR(-EINVAL);
> +
> +	map = kzalloc(sizeof(*map), GFP_KERNEL);
> +	if (!map)
> +		return ERR_PTR(-ENOMEM);
> +
> +	chan = dma_spec->args[0];
> +	map->req_idx = dma_spec->args[4];
> +	val = dma_spec->args[5];
> +	dma_spec->args_count -= 2;
> +
> +	if (chan >= RZN1_DMAMUX_MAX_LINES) {
> +		dev_err(&pdev->dev, "Invalid DMA request line: %u\n", chan);
> +		ret = -EINVAL;
> +		goto free_map;
> +	}
> +
> +	if (map->req_idx >= RZN1_DMAMUX_LINES ||
> +	    (map->req_idx % RZN1_DMAMUX_MAX_LINES) != chan) {
> +		dev_err(&pdev->dev, "Invalid MUX request line: %u\n", map->req_idx);
> +		ret = -EINVAL;
> +		goto free_map;
> +	}
> +
> +	dmac_idx = map->req_idx >= RZN1_DMAMUX_MAX_LINES ? 1 : 0;
> +	dma_spec->np = of_parse_phandle(ofdma->of_node, "dma-masters", dmac_idx);
> +	if (!dma_spec->np) {
> +		dev_err(&pdev->dev, "Can't get DMA master\n");
> +		ret = -EINVAL;
> +		goto free_map;
> +	}
> +
> +	dev_dbg(&pdev->dev, "Mapping DMAMUX request %u to DMAC%u request %u\n",
> +		map->req_idx, dmac_idx, chan);
> +
> +	mask = BIT(map->req_idx);
> +	mutex_lock(&dmamux->lock);
> +	dmamux->used_chans |= mask;
> +	ret = r9a06g032_sysctrl_set_dmamux(mask, val ? mask : 0);

I guess due to this it would be merged by whosoever merges this api.
Please mention this in cover letter and how you propose this should be
merged

-- 
~Vinod