RE: [PATCH 10/22] ASoC: rsnd: Add DMA support infrastructure for RZ/G3E

[John Madieu]

Hi Kuninori,

Thanks for the review.

> -----Original Message-----
> From: Kuninori Morimoto <kuninori.morimoto.gx@xxxxxxxxxxx>
> Sent: Monday, March 23, 2026 2:08 AM
> To: John Madieu <john.madieu.xa@xxxxxxxxxxxxxx>
> Subject: Re: [PATCH 10/22] ASoC: rsnd: Add DMA support infrastructure for
> RZ/G3E
> 
> 
> Hi John
> 
> Thank you for your patch
> 
> > RZ/G3E has different DMA register base addresses and offset
> > calculations compared to R-Car platforms, and requires additional
> > audmac-pp clock and reset lines for Audio DMAC operation.
> >
> > Add RZ/G3E-specific DMA address macros and audmac-pp clock/reset
> > support using optional APIs to remain transparent to other platforms.
> >
> > Signed-off-by: John Madieu <john.madieu.xa@xxxxxxxxxxxxxx>
> > ---
> 
> I think it include many features in 1 patch.
> You should separate it into each features.

Agreed, I'll split this into separate patches. One for
RZ/G3E DMA address support and one for audmac-pp clock/reset
management. What do you think of this approach ?

> 
> > diff --git a/sound/soc/renesas/rcar/dma.c
> > b/sound/soc/renesas/rcar/dma.c index 68c859897e68..d3123ae3b402 100644
> > --- a/sound/soc/renesas/rcar/dma.c
> > +++ b/sound/soc/renesas/rcar/dma.c
> > @@ -496,24 +496,71 @@ static struct rsnd_mod_ops rsnd_dmapp_ops = {
> >   *	SSIU: 0xec541000 / 0xec100000 / 0xec100000 / 0xec400000 / 0xec400000
> >   *	SCU : 0xec500000 / 0xec000000 / 0xec004000 / 0xec300000 / 0xec304000
> >   *	CMD : 0xec500000 /            / 0xec008000                0xec308000
> > + *
> > + * 	ex) G3E case
> > + *	      mod        / DMAC in    / DMAC out   / DMAC PP in / DMAC pp
> out
> > + *	SSI : 0x13C31000 / 0x13C40000 / 0x13C40000
> > + *	SSIU: 0x13C31000 / 0x13C40000 / 0x13C40000 / 0xEC400000 / 0xEC400000
> > + *	SCU : 0x13C00000 / 0x13C10000 / 0x13C14000 / 0xEC300000 / 0xEC304000
> > + *	CMD : 0x13C00000 /            / 0x13C18000                0xEC308000
> >   */
> > -#define RDMA_SSI_I_N(addr, i)	(addr ##_reg - 0x00300000 + (0x40 * i)
> + 0x8)
> > -#define RDMA_SSI_O_N(addr, i)	(addr ##_reg - 0x00300000 + (0x40 * i)
> + 0xc)
> >
> > -#define RDMA_SSIU_I_N(addr, i, j) (addr ##_reg - 0x00441000 + (0x1000
> > * (i)) + (((j) / 4) * 0xA000) + (((j) % 4) * 0x400) - (0x4000 * ((i) /
> > 9) * ((j) / 4))) -#define RDMA_SSIU_O_N(addr, i, j)
> > RDMA_SSIU_I_N(addr, i, j)
> > +/* RZ/G3E DMA address macros */
> > +#define RDMA_SSI_I_N_G3E(addr, i)	(addr ##_reg + 0x0000F000 +
> (0x1000 * i))
> > +#define RDMA_SSI_O_N_G3E(addr, i)	(addr ##_reg + 0x0000F000 +
> (0x1000 * i))
> > +
> > +#define RDMA_SSIU_I_N_G3E(addr, i, j) (addr ##_reg + 0x0000F000 +
> > +(0x1000 * (i)) + (((j) / 4) * 0xA000) + (((j) % 4) * 0x400) - (0x4000
> > +* ((i) / 9) * ((j) / 4))) #define RDMA_SSIU_O_N_G3E(addr, i, j)
> > +RDMA_SSIU_I_N_G3E(addr, i, j)
> > +
> > +#define RDMA_SSIU_I_P_G3E(addr, i, j) (addr ##_reg + 0xD87CF000 +
> > +(0x1000 * (i)) + (((j) / 4) * 0xA000) + (((j) % 4) * 0x400) - (0x4000
> > +* ((i) / 9) * ((j) / 4))) #define RDMA_SSIU_O_P_G3E(addr, i, j)
> > +RDMA_SSIU_I_P_G3E(addr, i, j)
> > +
> > +#define RDMA_SRC_I_N_G3E(addr, i)	(addr ##_reg + 0x00010000 +
> (0x400 * i))
> > +#define RDMA_SRC_O_N_G3E(addr, i)	(addr ##_reg + 0x00014000 +
> (0x400 * i))
> > +
> > +#define RDMA_SRC_I_P_G3E(addr, i)	(addr ##_reg + 0xD8700000 +
> (0x400 * i))
> > +#define RDMA_SRC_O_P_G3E(addr, i)	(addr ##_reg + 0xD8704000 +
> (0x400 * i))
> > +
> > +#define RDMA_CMD_O_N_G3E(addr, i)	(addr ##_reg + 0x00018000 +
> (0x400 * i))
> > +#define RDMA_CMD_O_P_G3E(addr, i)	(addr ##_reg + 0xD8708000 +
> (0x400 * i))
> > +
> > +/* R-Car DMA address macros */
> > +#define RDMA_SSI_I_N_RCAR(addr, i)	(addr ##_reg - 0x00300000 +
> (0x40 * i) + 0x8)
> > +#define RDMA_SSI_O_N_RCAR(addr, i)	(addr ##_reg - 0x00300000 +
> (0x40 * i) + 0xc)
> >
> > -#define RDMA_SSIU_I_P(addr, i, j) (addr ##_reg - 0x00141000 + (0x1000
> > * (i)) + (((j) / 4) * 0xA000) + (((j) % 4) * 0x400) - (0x4000 * ((i) /
> > 9) * ((j) / 4))) -#define RDMA_SSIU_O_P(addr, i, j)
> > RDMA_SSIU_I_P(addr, i, j)
> > +#define RDMA_SSIU_I_N_RCAR(addr, i, j) (addr ##_reg - 0x00441000 +
> > +(0x1000 * (i)) + (((j) / 4) * 0xA000) + (((j) % 4) * 0x400) - (0x4000
> > +* ((i) / 9) * ((j) / 4))) #define RDMA_SSIU_O_N_RCAR(addr, i, j)
> > +RDMA_SSIU_I_N_RCAR(addr, i, j)
> >
> > -#define RDMA_SRC_I_N(addr, i)	(addr ##_reg - 0x00500000 + (0x400 *
> i))
> > -#define RDMA_SRC_O_N(addr, i)	(addr ##_reg - 0x004fc000 + (0x400 *
> i))
> > +#define RDMA_SSIU_I_P_RCAR(addr, i, j) (addr ##_reg - 0x00141000 +
> > +(0x1000 * (i)) + (((j) / 4) * 0xA000) + (((j) % 4) * 0x400) - (0x4000
> > +* ((i) / 9) * ((j) / 4))) #define RDMA_SSIU_O_P_RCAR(addr, i, j)
> > +RDMA_SSIU_I_N_RCAR(addr, i, j)
> >
> > -#define RDMA_SRC_I_P(addr, i)	(addr ##_reg - 0x00200000 + (0x400 *
> i))
> > -#define RDMA_SRC_O_P(addr, i)	(addr ##_reg - 0x001fc000 + (0x400 *
> i))
> > +#define RDMA_SRC_I_N_RCAR(addr, i)	(addr ##_reg - 0x00500000 +
> (0x400 * i))
> > +#define RDMA_SRC_O_N_RCAR(addr, i)	(addr ##_reg - 0x004fc000 +
> (0x400 * i))
> >
> > -#define RDMA_CMD_O_N(addr, i)	(addr ##_reg - 0x004f8000 + (0x400 *
> i))
> > -#define RDMA_CMD_O_P(addr, i)	(addr ##_reg - 0x001f8000 + (0x400 *
> i))
> > +#define RDMA_SRC_I_P_RCAR(addr, i)	(addr ##_reg - 0x00200000 +
> (0x400 * i))
> > +#define RDMA_SRC_O_P_RCAR(addr, i)	(addr ##_reg - 0x001fc000 +
> (0x400 * i))
> > +
> > +#define RDMA_CMD_O_N_RCAR(addr, i)	(addr ##_reg - 0x004f8000 +
> (0x400 * i))
> > +#define RDMA_CMD_O_P_RCAR(addr, i)	(addr ##_reg - 0x001f8000 +
> (0x400 * i))
> > +
> > +/* Platform-agnostic address macros */
> > +#define RDMA_SSI_I_N(p, addr, i)	rsnd_is_rzg3e(p) ?
> RDMA_SSI_I_N_G3E(addr, i) : RDMA_SSI_I_N_RCAR(addr, i)
> > +#define RDMA_SSI_O_N(p, addr, i)	rsnd_is_rzg3e(p) ?
> RDMA_SSI_O_N_G3E(addr, i) : RDMA_SSI_O_N_RCAR(addr, i)
> > +
> > +#define RDMA_SSIU_I_N(p, addr, i, j) rsnd_is_rzg3e(p) ?
> > +RDMA_SSIU_I_N_G3E(addr, i, j) : RDMA_SSIU_I_N_RCAR(addr, i, j)
> > +#define RDMA_SSIU_O_N(p, addr, i, j) rsnd_is_rzg3e(p) ?
> > +RDMA_SSIU_O_N_G3E(addr, i, j) : RDMA_SSIU_O_N_RCAR(addr, i, j)
> > +
> > +#define RDMA_SSIU_I_P(p, addr, i, j) rsnd_is_rzg3e(p) ?
> > +RDMA_SSIU_I_P_G3E(addr, i, j) : RDMA_SSIU_I_P_RCAR(addr, i, j)
> > +#define RDMA_SSIU_O_P(p, addr, i, j) rsnd_is_rzg3e(p) ?
> > +RDMA_SSIU_O_P_G3E(addr, i, j) : RDMA_SSIU_O_P_RCAR(addr, i, j)
> > +
> > +#define RDMA_SRC_I_N(p, addr, i)	rsnd_is_rzg3e(p) ?
> RDMA_SRC_I_N_G3E(addr, i) : RDMA_SRC_I_N_RCAR(addr, i)
> > +#define RDMA_SRC_O_N(p, addr, i)	rsnd_is_rzg3e(p) ?
> RDMA_SRC_O_N_G3E(addr, i) : RDMA_SRC_O_N_RCAR(addr, i)
> > +
> > +#define RDMA_SRC_I_P(p, addr, i)	rsnd_is_rzg3e(p) ?
> RDMA_SRC_I_P_G3E(addr, i) : RDMA_SRC_I_P_RCAR(addr, i)
> > +#define RDMA_SRC_O_P(p, addr, i)	rsnd_is_rzg3e(p) ?
> RDMA_SRC_O_P_G3E(addr, i) : RDMA_SRC_O_P_RCAR(addr, i)
> > +
> > +#define RDMA_CMD_O_N(p, addr, i)	rsnd_is_rzg3e(p) ?
> RDMA_CMD_O_N_G3E(addr, i) : RDMA_CMD_O_N_RCAR(addr, i)
> > +#define RDMA_CMD_O_P(p, addr, i)	rsnd_is_rzg3e(p) ?
> RDMA_CMD_O_P_G3E(addr, i) : RDMA_CMD_O_P_RCAR(addr, i)
> 
> I think you want to create new rsnd_rzg3e_dma_addr() and call it, instead
> of makes existing code complex.

Makes sense. I'll drop the macro-based approach and create a
dedicated rsnd_rzg3e_dma_addr() function following the same
pattern as rsnd_gen4_dma_addr()/rsnd_gen2_dma_addr(), with
dispatch in rsnd_dma_addr().

> 
> + static dma_addr_t rsnd_rzg3e_dma_addr(...)  {
> +	...
> + }
> ...
>   static dma_addr_t rsnd_dma_addr(...)
>   {
> 	...
> 	else if (rsnd_is_gen4(priv))
> 		return rsnd_gen4_dma_addr(...);
> +	else if (rsnd_is_rzg3e(priv))
> +		return rsnd_rzg3e_dma_addr(...)
> 	else
> 		return rsnd_gen2_dma_addr(...);
> }
> 
> > @@ -860,6 +917,56 @@ int rsnd_dma_probe(struct rsnd_priv *priv)
> >  		return 0; /* it will be PIO mode */
> >  	}
> >
> > +	/*
> > +	 * audmac_pp clock/reset management strategy:
> > +	 *
> > +	 * Unlike other modules (SSI, SRC, etc.) which have their own
> dedicated
> > +	 * clocks, all DMA modules share the single audmac_pp clock/reset.
> > +	 * Managing it per-stream or per-DMA-module causes
> > +	 * reference count imbalances:
> > +	 *
> > +	 *   - rsnd_mod_init() does clk_prepare_enable() then clk_disable(),
> > +	 *     leaving prepare_count=1 per module
> > +	 *   - With N DMA modules sharing the same clock handle,
> prepare_count=N
> > +	 *   - suspend does single clk_disable_unprepare() (-1)
> > +	 *   - resume does single clk_prepare_enable() (+1)
> > +	 *   - Result: prepare_count leaks on each suspend/resume cycle
> > +	 *
> > +	 * Per-stream management (iterating DMA modules in suspend/resume)
> is
> > +	 * not worth the complexity:
> > +	 *
> > +	 *   - No power benefit: audmac_pp is needed whenever ANY stream is
> > +	 *     active, and every stream uses DMA, so it's essentially always
> on
> > +	 *   - Architecture mismatch: DMA modules live in io->dma, not in a
> > +	 *     priv array -- no clean way to iterate like SSI/SRC/DVC
> > +	 *   - Shared handle problem: all DMA modules point to the same
> clock,
> > +	 *     so iterating would call clk_unprepare() N times on one clock
> > +	 *   - Would require manual refcounting ("enable on first stream,
> > +	 *     disable on last") -- reimplementing what clk framework does
> > +	 *
> > +	 * The correct approach is to treat audmac_pp as always-on
> infrastructure
> > +	 * (same pattern as clk_adg), managed globally:
> > +	 *   - Probe: acquire + enable (via devm_clk_get_optional_enabled)
> > +	 *   - Suspend/Resume: toggle in core.c rsnd_suspend/rsnd_resume
> > +	 *   - Remove: devm cleanup
> > +	 *   - DMA modules: pass NULL clock/reset to rsnd_mod_init()
> > +	 *
> > +	 * Use devm variants that handle deassert/enable automatically.
> > +	 * Order: reset deasserted first, then clock enabled.
> > +	 */
> > +	priv->rstc_audmac_pp =
> > +		devm_reset_control_get_optional_exclusive_deasserted(dev,
> "audmac_pp");
> > +	if (IS_ERR(priv->rstc_audmac_pp)) {
> > +		return dev_err_probe(dev, PTR_ERR(priv->rstc_audmac_pp),
> > +				     "failed to get audmac_pp reset\n");
> > +	}
> > +
> > +	priv->clk_audmac_pp = devm_clk_get_optional_enabled(dev,
> "audmac_pp");
> > +	if (IS_ERR(priv->clk_audmac_pp)) {
> > +		return dev_err_probe(dev, PTR_ERR(priv->clk_audmac_pp),
> > +				     "failed to get audmac_pp clock\n");
> > +	}
> 
> rsnd_dma_probe() is common fucntion.
> Is above possible to keep compatible with other SoCs ?

Other SoCs do not need or specify these clock/reset in DTS and
the fact I use optional APIs makes it compatible with these other
SoCs. I'm wondering if you meant something else here?


> 
> And, we are already using "audmacpp".

What do you mean by the above ?

> I think it time to update rsnd_dma_probe() like below ?
> 
> 	int rsnd_dma_probe(...)
> 	{
> 		if (rsnd_is_gen1(..))
> 			return ...
> 		else if (rsnd_is_gen2(...) ||
> 			 rsnd_is_gen3(...))
> 			return ...
> 		else if (rsnd_is_gen4(...))
> 			return ...
> 		else if (rsnd_is_rzg3e(...))
> 			return ...
> 		...
> 	}

Regarding rsnd_dma_probe(), I intentionally used
devm_clk_get_optional_enabled() and
devm_reset_control_get_optional_exclusive_deasserted() - these
return NULL when the clock/reset is not present in the device tree,
so they are fully transparent to existing SoCs (R-Car Gen2/3/4).

Adding per-SoC branches in rsnd_dma_probe() would duplicate the
common DMAC setup logic. I believe keeping this in the common
path is the cleaner approach, but I'm happy to discuss if you
see a specific concern beyond compatibility.

Regards,
John