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

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Geert,

geert@linux-m68k.org wrote on Wed, 23 Feb 2022 13:46:11 +0100:

> Hi Miquel,
> 
> On Tue, Feb 22, 2022 at 11:35 AM Miquel Raynal
> <miquel.raynal@bootlin.com> 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@bootlin.com>  
> 
> Thanks for your patch!
> 
> > --- /dev/null
> > +++ b/drivers/dma/dw/dmamux.c  
> 
> rzn1-dmamux.c?

Ok.

> 
> > @@ -0,0 +1,167 @@
> > +// 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/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).

That's right, I assumed this was safe but indeed it does not work in
all cases. I will change the second condition to:

		map->req_idx % <16> != chan

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

I personally prefer using true/false for booleans ;) Whatever, the name
is badly chosen I agree, I'll switch to "dmac_idx" which seems more
accurate.

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

Not really, I think the following checks are still valid and fortunate,
and they need some of_ handling to work properly:
- verify that the chan requested is within the range of dmac_requests
  in the _route_allocate() callback
- ensure the dmamux is wired to a supported DMAC in the DT (this
  condition might be loosen in the future if needed or dropped entirely
  if considered useless)
- I would like to add a check against the number of requests supported
  by the dmamux and the dmac (not done yet).
For the record, I've taken inspiration to write these lines on the other
dma router driver from TI.

Unless, and I know some people think like that, we do not try to
validate the DT and if the DT is wrong that is none of our business.

> 
> > +
> > +       if (of_property_read_u32(mux_node, "dma-requests", &dmamux->dmamux_requests)) {  
> 
> Don't obtain from DT, but fix to 32?

I believe the answer to the previous question should give me a clue
about why you would prefer hardcoding than reading from the DT such
an information. Perhaps I should mention that all these properties are
already part of the bindings, and are not specific to the driver, the
information will be in the DT anyway.

Thanks,
Miquèl