Hi Frank,
On Wed, Jul 30, 2025 at 02:37:54PM -0400, Frank Li wrote:
> On Wed, Jul 30, 2025 at 09:04:17PM +0300, Laurent Pinchart wrote:
> > 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@xxxxxxxxxxxxxxxx>
> > > >
> > > > 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@xxxxxxxxxxxxxxxx>
> > > > ---
> > > > 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.
>
> I remember just need record client device pointer here.
>
> > >
> > > 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
>
> DMA description is a queue, It is hard to track each descriptor submit and
> finished. espcially cycle buffer case.
>
> And according to dma engine API defination, submit a descriptor not
> neccessary to turn on clock, maybe just pure software operation, such as
> enqueue it to a software list.
>
> Many driver call dmaengine_submit() in irq context, submit new descriptor
> when previous descriptor finished. runtime_pm_get() can NOT be called in
> atomic context.
>
> And some driver submit many descripor advance. Only issue_transfer() is
> actually trigger hardware to start transfer.
>
> Some client use cycle descripor, such audio devices. Some audio devices
> have not free descriptor at their run time suspend function, just disable
> audio devices's clocks. Audio devices run time suspend, which means no
> one use this dma channel, dma channel can auto suspend if built device link
> between audio device and dma chan devices.
>
> Some DMA client have not devices, such as memory to memory. for this kind
> case, it need keep chan always on.
>
> issue_transfer() can be call in atomic context. but trigger hardware transfer
> need clock and runtime_pm_get() can't be called in atomic context.
>
> Most case issue_transfer() is call in irq handle, which means device should
> already be in runtime resume statue. DMA engine can safely access their
> register if using device link.
You have good points there, in particular the fact the issue_transfer()
can be called in interrupt context.
For me this calls for new DMA engine operations to "start/stop" the DMA
engine (better names are likely needed) from a client perspective.
> > 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
