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

[Geert Uytterhoeven <geert@linux-m68k.org>]

Hi Miquel,

On Wed, Feb 23, 2022 at 5:49 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> geert@linux-m68k.org wrote on Wed, 23 Feb 2022 13:46:11 +0100:
> > 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

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

The 32 is a property of the hardware (32 bits in DMAMUX register).
So IMHO it falls under the "differentiate by compatible value,
not by property" rule.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-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