Re: [PATCH RFC 2/6] dmaengine: Make of_dma_request_slave_channel pass a cookie to of_xlate

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Wed, Jul 30, 2025 at 12:39:43PM -0400, Frank Li wrote:
> On Wed, Jul 30, 2025 at 11:33:29AM +0200, Konrad Dybcio wrote:
> > From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> >
> > The DMA subsystem attempts to make it theoretically possible to pair
> > any DMA block with any user. While that's convenient from a
> > codebase sanity perspective, some blocks are more intertwined.
> >
> > One such case is the Qualcomm GENI, where each wrapper contains a
> > number of Serial Engine instances, each one of which can be programmed
> > to support a different protocol (such as I2C, I3C, SPI, UART, etc.).
> >
> > The GPI DMA it's designed together with, needs to receive the ID of the
> > protocol that's in use, to adjust its behavior accordingly. Currently,
> > that's done through passing that ID through device tree, with each
> > Serial Engine expressed NUM_PROTOCOL times, resulting in terrible
> > dt-bindings that are full of useless copypasta.
> >
> > In a step to cut down on that, let the DMA user give the engine driver
> > a hint at request time.
> >
> > Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> > ---
> >  drivers/dma/amba-pl08x.c                       |  3 ++-
> >  drivers/dma/apple-admac.c                      |  3 ++-
> >  drivers/dma/at_hdmac.c                         |  6 ++++--
> >  drivers/dma/at_xdmac.c                         |  3 ++-
> >  drivers/dma/bcm2835-dma.c                      |  3 ++-
> >  drivers/dma/dma-jz4780.c                       |  3 ++-
> >  drivers/dma/dmaengine.c                        | 20 +++++++++++++++++---
> >  drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c |  3 ++-
> >  drivers/dma/dw/of.c                            |  3 ++-
> >  drivers/dma/ep93xx_dma.c                       |  6 ++++--
> >  drivers/dma/fsl-edma-main.c                    |  6 ++++--
> >  drivers/dma/img-mdc-dma.c                      |  3 ++-
> >  drivers/dma/imx-dma.c                          |  3 ++-
> >  drivers/dma/imx-sdma.c                         |  3 ++-
> >  drivers/dma/lgm/lgm-dma.c                      |  3 ++-
> >  drivers/dma/milbeaut-hdmac.c                   |  4 +++-
> >  drivers/dma/mmp_pdma.c                         |  3 ++-
> >  drivers/dma/mmp_tdma.c                         |  3 ++-
> >  drivers/dma/moxart-dma.c                       |  3 ++-
> >  drivers/dma/mxs-dma.c                          |  3 ++-
> >  drivers/dma/nbpfaxi.c                          |  3 ++-
> >  drivers/dma/of-dma.c                           | 18 +++++++++++-------
> >  drivers/dma/owl-dma.c                          |  3 ++-
> >  drivers/dma/pl330.c                            |  3 ++-
> >  drivers/dma/pxa_dma.c                          |  3 ++-
> >  drivers/dma/qcom/bam_dma.c                     |  3 ++-
> >  drivers/dma/qcom/gpi.c                         |  3 ++-
> >  drivers/dma/qcom/qcom_adm.c                    |  3 ++-
> >  drivers/dma/sh/rcar-dmac.c                     |  3 ++-
> >  drivers/dma/sh/rz-dmac.c                       |  3 ++-
> >  drivers/dma/sh/usb-dmac.c                      |  3 ++-
> >  drivers/dma/st_fdma.c                          |  3 ++-
> >  drivers/dma/ste_dma40.c                        |  3 ++-
> >  drivers/dma/stm32/stm32-dma.c                  |  3 ++-
> >  drivers/dma/stm32/stm32-dma3.c                 |  4 +++-
> >  drivers/dma/stm32/stm32-mdma.c                 |  3 ++-
> >  drivers/dma/sun4i-dma.c                        |  3 ++-
> >  drivers/dma/sun6i-dma.c                        |  3 ++-
> >  drivers/dma/tegra186-gpc-dma.c                 |  3 ++-
> >  drivers/dma/tegra20-apb-dma.c                  |  3 ++-
> >  drivers/dma/tegra210-adma.c                    |  3 ++-
> >  drivers/dma/ti/cppi41.c                        |  3 ++-
> >  drivers/dma/ti/edma.c                          |  3 ++-
> >  drivers/dma/ti/k3-udma.c                       |  3 ++-
> >  drivers/dma/uniphier-xdmac.c                   |  3 ++-
> >  drivers/dma/xilinx/xilinx_dma.c                |  3 ++-
> >  drivers/dma/xilinx/xilinx_dpdma.c              |  3 ++-
> >  drivers/dma/xilinx/zynqmp_dma.c                |  3 ++-
> >  include/linux/dmaengine.h                      |  7 +++++++
> >  include/linux/of_dma.h                         | 16 +++++++++-------
> >  sound/soc/apple/mca.c                          |  2 +-
> >  sound/soc/renesas/rcar/dma.c                   |  2 +-
> >  52 files changed, 146 insertions(+), 68 deletions(-)
> >
> > diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
> 
> ...
> 
> >  						const char *name)
> >  {
> > diff --git a/include/linux/of_dma.h b/include/linux/of_dma.h
> > index fd706cdf255c61c82ce30ef9a2c44930bef34bc8..9f9bc4207b85d48d73c25aad4b362e7c84c01756 100644
> > --- a/include/linux/of_dma.h
> > +++ b/include/linux/of_dma.h
> > @@ -19,7 +19,7 @@ struct of_dma {
> >  	struct list_head	of_dma_controllers;
> >  	struct device_node	*of_node;
> >  	struct dma_chan		*(*of_dma_xlate)
> > -				(struct of_phandle_args *, struct of_dma *);
> > +				(struct of_phandle_args *, struct of_dma *, void *);
> 
> I suggest pass down more informaiton, like client's dev point. So we can
> auto create device link between client's dev and dma chan's device.

Is .of_dma_xlate() really the right place to do that ? If you want to
create a device link for PM reasons, isn't it better created when the
channel is requested ? It should also be removed when the channel is
freed.

> 
> DMA Engineer device
>    DMA chan device
>        consumer clients' device.
> 
> If consumer device runtime pm suspend can auto trigger DMA chan's device's
> runtime pm function.
> 
> It will simplifly DMA engine's run time pm manage. Currently many DMA run
> time pm implement as, runtime_pm_get() when alloc and runtime_pm_put() at
> free channel.  But many devices request dma channel at probe, which make
> dma engine work at always 'on' state.
> 
> But ideally, dma chan should be resume only when it is used to transfer.

This is exactly what I was going to mention after reading the last
paragraph. Is there anything that prevents a DMA engine driver to
perform a rutime PM get() when a transfer is submitted and a put() when
it completes ? (Logically speaking, the actual implementation would
likely be a bit different in drivers, but the result would be similar.)

> >  	void			*(*of_dma_route_allocate)
> >  				(struct of_phandle_args *, struct of_dma *);
> >  	struct dma_router	*dma_router;
> > @@ -34,7 +34,7 @@ struct of_dma_filter_info {
> >  #ifdef CONFIG_DMA_OF
> >  extern int of_dma_controller_register(struct device_node *np,
> >  		struct dma_chan *(*of_dma_xlate)
> > -		(struct of_phandle_args *, struct of_dma *),
> > +		(struct of_phandle_args *, struct of_dma *, void *),
> >  		void *data);
> >  extern void of_dma_controller_free(struct device_node *np);
> >
> > @@ -45,16 +45,17 @@ extern int of_dma_router_register(struct device_node *np,
> >  #define of_dma_router_free of_dma_controller_free
> >
> >  extern struct dma_chan *of_dma_request_slave_channel(struct device_node *np,
> > -						     const char *name);
> > +						     const char *name,
> > +						     void *data);
> >  extern struct dma_chan *of_dma_simple_xlate(struct of_phandle_args *dma_spec,
> > -		struct of_dma *ofdma);
> > +		struct of_dma *ofdma, void *data);
> >  extern struct dma_chan *of_dma_xlate_by_chan_id(struct of_phandle_args *dma_spec,
> > -		struct of_dma *ofdma);
> > +		struct of_dma *ofdma, void *data);
> >
> >  #else
> >  static inline int of_dma_controller_register(struct device_node *np,
> >  		struct dma_chan *(*of_dma_xlate)
> > -		(struct of_phandle_args *, struct of_dma *),
> > +		(struct of_phandle_args *, struct of_dma *, void *),
> >  		void *data)
> >  {
> >  	return -ENODEV;
> > @@ -75,7 +76,8 @@ static inline int of_dma_router_register(struct device_node *np,
> >  #define of_dma_router_free of_dma_controller_free
> >
> >  static inline struct dma_chan *of_dma_request_slave_channel(struct device_node *np,
> > -						     const char *name)
> > +							    const char *name,
> > +							    void *data)
> >  {
> >  	return ERR_PTR(-ENODEV);
> >  }
> > diff --git a/sound/soc/apple/mca.c b/sound/soc/apple/mca.c
> > index 5dd24ab90d0f052bb48f451cf009dc2e9128014d..43d48e4ac8161ee9955120fe64f7b911bfdfe1ca 100644
> > --- a/sound/soc/apple/mca.c
> > +++ b/sound/soc/apple/mca.c
> > @@ -926,7 +926,7 @@ static struct dma_chan *mca_request_dma_channel(struct mca_cluster *cl, unsigned
> >  	char *name = devm_kasprintf(cl->host->dev, GFP_KERNEL,
> >  				    is_tx ? "tx%da" : "rx%db", cl->no);
> >  #endif
> > -	return of_dma_request_slave_channel(cl->host->dev->of_node, name);
> > +	return of_dma_request_slave_channel(cl->host->dev->of_node, name, NULL);
> >
> >  }
> >
> > diff --git a/sound/soc/renesas/rcar/dma.c b/sound/soc/renesas/rcar/dma.c
> > index 2035ce06fe4c4aeaa8620d817910a5319732fa58..dcbff2fc61a0472adea226371016a128563b3cd0 100644
> > --- a/sound/soc/renesas/rcar/dma.c
> > +++ b/sound/soc/renesas/rcar/dma.c
> > @@ -204,7 +204,7 @@ struct dma_chan *rsnd_dma_request_channel(struct device_node *of_node, char *nam
> >  		}
> >
> >  		if (i == rsnd_mod_id_raw(mod) && (!chan))
> > -			chan = of_dma_request_slave_channel(np, x);
> > +			chan = of_dma_request_slave_channel(np, x, NULL);
> >  		i++;
> >  	}
> >

-- 
Regards,

Laurent Pinchart