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

[Kuninori Morimoto]

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.


Thank you for your help !!

Best regards
---
Kuninori Morimoto