RE: [PATCh v3 06/14] ASoC: rsnd: Add RZ/G3E DMA address calculation support

[John Madieu]

Hi Kuninori,

Thanks for your review.

> -----Original Message-----
> From: Kuninori Morimoto <kuninori.morimoto.gx@xxxxxxxxxxx>
> Sent: Friday, April 3, 2026 3:09 AM
> To: John Madieu <john.madieu.xa@xxxxxxxxxxxxxx>
> Subject: Re: [PATCh v3 06/14] ASoC: rsnd: Add RZ/G3E DMA address
> calculation support
> 
> 
> Hi John
> 
> Thank you for your patch
> 
> > RZ/G3E has different DMA register base addresses and offset
> > calculations compared to R-Car platforms.
> >
> > Add dedicated rsnd_rzg3e_dma_addr() function with dispatch from
> > rsnd_dma_addr(), following the existing per-generation pattern.
> >
> > Signed-off-by: John Madieu <john.madieu.xa@xxxxxxxxxxxxxx>
> > ---
> (snip)
> > diff --git a/sound/soc/renesas/rcar/dma.c
> > b/sound/soc/renesas/rcar/dma.c index 0afe4636b005..5b63206361ef 100644
> > --- a/sound/soc/renesas/rcar/dma.c
> > +++ b/sound/soc/renesas/rcar/dma.c
> > @@ -496,7 +496,35 @@ 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
> >   */
> > +
> > +/* 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))
> 
> Please add RZ/G3E info on top of rzg3e_dma_addr()
> 
> > +/* R-Car DMA address macros */
> >  #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)
> 
> And R-Car Gen2 info to gen2_dma_addr()
> 
> > +rsnd_dma_addr_lookup(struct rsnd_dai_stream *io,
> > +		     struct rsnd_mod *mod,
> > +		     const struct rsnd_dma_addr tbl[3][2][3],
> > +		     int is_play, int is_from)
> >  {
> > -	struct rsnd_priv *priv = rsnd_io_to_priv(io);
> > -	struct device *dev = rsnd_priv_to_dev(priv);
> > -	phys_addr_t ssi_reg = rsnd_gen_get_phy_addr(priv, RSND_BASE_SSI);
> > -	phys_addr_t src_reg = rsnd_gen_get_phy_addr(priv, RSND_BASE_SCU);
> > +	struct device *dev = rsnd_priv_to_dev(rsnd_io_to_priv(io));
> 
> Creating lookup() is nice idea.
> Very nitpick, but do we need to remove priv, and remake dev ?
> 
> > @@ -574,20 +671,10 @@ rsnd_gen2_dma_addr(struct rsnd_dai_stream *io,
> >  	 * out of calculation rule
> >  	 */
> >  	if ((id == 9) && (busif >= 4))
> > -		dev_err(dev, "This driver doesn't support SSI%d-%d, so far",
> > -			id, busif);
> > -
> > -	/* it shouldn't happen */
> > -	if (use_cmd && !use_src)
> > -		dev_err(dev, "DVC is selected without SRC\n");
> > -
> > -	/* use SSIU or SSI ? */
> > -	if (is_ssi && rsnd_ssi_use_busif(io))
> > -		is_ssi++;
> > +		dev_err(rsnd_priv_to_dev(priv),
> > +			"This driver doesn't support SSI%d-%d, so far", id,
> busif);
> 
> Very nitpick, but please keep dev on top.
> 

I'll address your comments in v4, moving each macro on top of the
appropriate function and keep dev, avoiding remaking it, and also
pass priv as parameter to rsnd_dma_addr_lookup.

Regards,
John

> 
> Thank you for your help !!
> 
> Best regards
> ---
> Kuninori Morimoto