[PATCH] ASoC: rcar: adg: correct TIMSEL setting for SSI9

[PATCH] ASoC: rcar: adg: correct TIMSEL setting for SSI9

[Eugeniu Rosca]

From: Andreas Pape <Andreas.Pape4@bosch.com>

Timing select register for SRC and CMD is by default
referring to the corresponding SSI word select.
The calculation rule from HW spec skips SSI8, which has
no clock connection.

0110: ssi_ws0
0111: ssi_ws1
1000: ssi_ws2
1001: ssi_ws3
1010: ssi_ws4
1011: ssi_ws5
1100: ssi_ws6
1101: ssi_ws7
<GAP>
1110: ssi_ws9
1111: Setting prohibited

The driver does not currently reflect that GAP, leading to
prohibited timsel value 1111 (0xf) for SSI9:

[21.695055] rcar_sound ec500000.sound: b adg[0]-CMDOUT_TIMSEL (32):00000f00/00000f1f

Correct the timsel assignment.

Fixes: 629509c5bc478c ("ASoC: rsnd: add Gen2 SRC and DMAEngine support")
Signed-off-by: Andreas Pape <Andreas.Pape4@bosch.com>
Signed-off-by: Yeswanth Rayapati <yeswanth.rayapati@in.bosch.com>
[erosca: minor improvements in commit description]
Signed-off-by: Eugeniu Rosca <eugeniu.rosca@bosch.com>
---
 sound/soc/sh/rcar/adg.c | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/sound/soc/sh/rcar/adg.c b/sound/soc/sh/rcar/adg.c
index 230c48648af359..137db9feab495e 100644
--- a/sound/soc/sh/rcar/adg.c
+++ b/sound/soc/sh/rcar/adg.c
@@ -95,25 +95,40 @@ static u32 rsnd_adg_ssi_ws_timing_gen2(struct rsnd_dai_stream *io)
 {
 	struct rsnd_mod *ssi_mod = rsnd_io_to_mod_ssi(io);
 	int id = rsnd_mod_id(ssi_mod);
-	int ws = id;
+
+	u8 ssi_ws[] = {
+		0x6, /* ssi0 */
+		0x7, /* ssi1 */
+		0x8, /* ssi2 */
+		0x9, /* ssi3 */
+		0xa, /* ssi4 */
+		0xb, /* ssi5 */
+		0xc, /* ssi6 */
+		0xd, /* ssi7 */
+		0xf, /* INVALID */
+		0xe, /* ssi9 */
+	};
 
 	if (rsnd_ssi_is_pin_sharing(io)) {
 		switch (id) {
 		case 1:
 		case 2:
 		case 9:
-			ws = 0;
+			id = 0;
 			break;
 		case 4:
-			ws = 3;
+			id = 3;
 			break;
 		case 8:
-			ws = 7;
+			id = 7;
 			break;
 		}
 	}
 
-	return (0x6 + ws) << 8;
+	if (id > 9)
+		return 0xf << 8;
+	else
+		return ssi_ws[id] << 8;
 }
 
 static void __rsnd_adg_get_timesel_ratio(struct rsnd_priv *priv,
-- 
2.43.2

Re: [PATCH] ASoC: rcar: adg: correct TIMSEL setting for SSI9

[Kuninori Morimoto]

Hi Eugeniu

Thank you for the patch

> Timing select register for SRC and CMD is by default
> referring to the corresponding SSI word select.
> The calculation rule from HW spec skips SSI8, which has
> no clock connection.
> 
> 0110: ssi_ws0
> 0111: ssi_ws1
> 1000: ssi_ws2
> 1001: ssi_ws3
> 1010: ssi_ws4
> 1011: ssi_ws5
> 1100: ssi_ws6
> 1101: ssi_ws7
> <GAP>
> 1110: ssi_ws9
> 1111: Setting prohibited
> 
> The driver does not currently reflect that GAP, leading to
> prohibited timsel value 1111 (0xf) for SSI9:
> 
> [21.695055] rcar_sound ec500000.sound: b adg[0]-CMDOUT_TIMSEL (32):00000f00/00000f1f
> 
> Correct the timsel assignment.
> 
> Fixes: 629509c5bc478c ("ASoC: rsnd: add Gen2 SRC and DMAEngine support")
> Signed-off-by: Andreas Pape <Andreas.Pape4@bosch.com>
> Signed-off-by: Yeswanth Rayapati <yeswanth.rayapati@in.bosch.com>
> [erosca: minor improvements in commit description]
> Signed-off-by: Eugeniu Rosca <eugeniu.rosca@bosch.com>
> ---
>  sound/soc/sh/rcar/adg.c | 25 ++++++++++++++++++++-----
>  1 file changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/sound/soc/sh/rcar/adg.c b/sound/soc/sh/rcar/adg.c
> index 230c48648af359..137db9feab495e 100644
> --- a/sound/soc/sh/rcar/adg.c
> +++ b/sound/soc/sh/rcar/adg.c
> @@ -95,25 +95,40 @@ static u32 rsnd_adg_ssi_ws_timing_gen2(struct rsnd_dai_stream *io)
>  {
>  	struct rsnd_mod *ssi_mod = rsnd_io_to_mod_ssi(io);
>  	int id = rsnd_mod_id(ssi_mod);
> -	int ws = id;
> +
> +	u8 ssi_ws[] = {
> +		0x6, /* ssi0 */
> +		0x7, /* ssi1 */
> +		0x8, /* ssi2 */
> +		0x9, /* ssi3 */
> +		0xa, /* ssi4 */
> +		0xb, /* ssi5 */
> +		0xc, /* ssi6 */
> +		0xd, /* ssi7 */
> +		0xf, /* INVALID */
> +		0xe, /* ssi9 */
> +	};
>  
>  	if (rsnd_ssi_is_pin_sharing(io)) {
>  		switch (id) {
>  		case 1:
>  		case 2:
>  		case 9:
> -			ws = 0;
> +			id = 0;
>  			break;
>  		case 4:
> -			ws = 3;
> +			id = 3;
>  			break;
>  		case 8:
> -			ws = 7;
> +			id = 7;
>  			break;
>  		}
>  	}
>  
> -	return (0x6 + ws) << 8;
> +	if (id > 9)
> +		return 0xf << 8;
> +	else
> +		return ssi_ws[id] << 8;
>  }

I don't think we want to have table for this purpose.
SSIx special handling exist every where on this IP.

How about below ? It is more simple.

---------------------------------
diff --git a/sound/soc/sh/rcar/adg.c b/sound/soc/sh/rcar/adg.c
index 230c48648af3..bbc7845c68b3 100644
--- a/sound/soc/sh/rcar/adg.c
+++ b/sound/soc/sh/rcar/adg.c
@@ -113,6 +113,13 @@ static u32 rsnd_adg_ssi_ws_timing_gen2(struct rsnd_dai_stream *io)
 		}
 	}
 
+	/*
+	 * SSI8 is not connected to ADG.
+	 * Thus SSI9 is using 8
+	 */
+	if (id == 9)
+		ws = 8;
+
 	return (0x6 + ws) << 8;
 }
---------------------------------



Thank you for your help !!

Best regards
---
Renesas Electronics
Ph.D. Kuninori Morimoto

Re: [PATCH] ASoC: rcar: adg: correct TIMSEL setting for SSI9

[Eugeniu Rosca]

Hello Morimoto-san,

Thank you for the prompt response.

On Sun, Feb 25, 2024 at 11:21:16PM +0000, Kuninori Morimoto wrote:

[..]

> >  
> > -	return (0x6 + ws) << 8;
> > +	if (id > 9)
> > +		return 0xf << 8;
> > +	else
> > +		return ssi_ws[id] << 8;
> >  }
> 
> I don't think we want to have table for this purpose.
> SSIx special handling exist every where on this IP.
> 
> How about below ? It is more simple.
> 
> ---------------------------------
> diff --git a/sound/soc/sh/rcar/adg.c b/sound/soc/sh/rcar/adg.c
> index 230c48648af3..bbc7845c68b3 100644
> --- a/sound/soc/sh/rcar/adg.c
> +++ b/sound/soc/sh/rcar/adg.c
> @@ -113,6 +113,13 @@ static u32 rsnd_adg_ssi_ws_timing_gen2(struct rsnd_dai_stream *io)
>  		}
>  	}
>  
> +	/*
> +	 * SSI8 is not connected to ADG.
> +	 * Thus SSI9 is using 8
> +	 */
> +	if (id == 9)
> +		ws = 8;
> +
>  	return (0x6 + ws) << 8;
>  }
> ---------------------------------

Appreciate very much the simpler counter-proposal.

Quick/preliminary verification attempts showed that it might not be
fully equivalent to the original patch, but we are still trying to
get that finally confirmed or invalidated. Will keep you posted.

BR, Eugeniu

Re: [PATCH] ASoC: rcar: adg: correct TIMSEL setting for SSI9

[Kuninori Morimoto]

Hi Eugeniu

> > +	/*
> > +	 * SSI8 is not connected to ADG.
> > +	 * Thus SSI9 is using 8
> > +	 */
> > +	if (id == 9)
> > +		ws = 8;
(snip)
> Quick/preliminary verification attempts showed that it might not be
> fully equivalent to the original patch

If it is indicating that above simple code doesn't care about
SSI8 error case, I don't think it needs to care about it.
Because SSI8 with shared pin setting is *mandatory*.


Thank you for your help !!

Best regards
---
Renesas Electronics
Ph.D. Kuninori Morimoto

Re: [PATCH] ASoC: rcar: adg: correct TIMSEL setting for SSI9

[Eugeniu Rosca]

Hello Morimoto-san,

On Tue, Feb 27, 2024 at 11:22:53PM +0000, Kuninori Morimoto wrote:
> 
> > > +	/*
> > > +	 * SSI8 is not connected to ADG.
> > > +	 * Thus SSI9 is using 8
> > > +	 */
> > > +	if (id == 9)
> > > +		ws = 8;
> (snip)
> > Quick/preliminary verification attempts showed that it might not be
> > fully equivalent to the original patch
> 
> If it is indicating that above simple code doesn't care about
> SSI8 error case, I don't think it needs to care about it.

A number of concerns have been raised internally, related to the fact
that the "optimized/simplified" counter-proposal behaves differently
depending on the value returned by rsnd_ssi_is_pin_sharing().

> Because SSI8 with shared pin setting is *mandatory*.

While it may be clear for you that pin sharing is mandatory, it is not
immediately obvious to the casual reader/contributor purely based on
code review. From this particular viewpoint, I would rather vote in
favor of the original patch authored by Andreas (Cc), since it makes
things very clear and does not hide any dependencies/assumptions.

Please, kindly provide your verdict, so that we can proceed with the
right solution, due to the issue being time critical for us.

BR, Eugeniu

Re: [PATCH] ASoC: rcar: adg: correct TIMSEL setting for SSI9

[Kuninori Morimoto]

Hi Eugeniu

> > > > +	/*
> > > > +	 * SSI8 is not connected to ADG.
> > > > +	 * Thus SSI9 is using 8
> > > > +	 */
> > > > +	if (id == 9)
> > > > +		ws = 8;
> > (snip)
> > > Quick/preliminary verification attempts showed that it might not be
> > > fully equivalent to the original patch
> > 
> > If it is indicating that above simple code doesn't care about
> > SSI8 error case, I don't think it needs to care about it.
> 
> A number of concerns have been raised internally, related to the fact
> that the "optimized/simplified" counter-proposal behaves differently
> depending on the value returned by rsnd_ssi_is_pin_sharing().
>
> While it may be clear for you that pin sharing is mandatory, it is not
> immediately obvious to the casual reader/contributor purely based on
> code review. From this particular viewpoint, I would rather vote in
> favor of the original patch authored by Andreas (Cc), since it makes
> things very clear and does not hide any dependencies/assumptions.

Hmm.. ?
Could you please indicate the sample case of differently behaves ?
For example, if xxx was xxx, original code behaves xxx, but simple
code behaves xxx, etc. I'm not sure what is your concern...
For me, original code is solving simple problems in complex ways
by using much memory.


Thank you for your help !!

Best regards
---
Renesas Electronics
Ph.D. Kuninori Morimoto

Re: [PATCH] ASoC: rcar: adg: correct TIMSEL setting for SSI9

[Kuninori Morimoto]

Hi Eugeniu

> > A number of concerns have been raised internally, related to the fact
> > that the "optimized/simplified" counter-proposal behaves differently
> > depending on the value returned by rsnd_ssi_is_pin_sharing().
(snip)
> Could you please indicate the sample case of differently behaves ?
> For example, if xxx was xxx, original code behaves xxx, but simple
> code behaves xxx, etc. I'm not sure what is your concern...

Ah.., in case of SSI8.

> > While it may be clear for you that pin sharing is mandatory, it is not
> > immediately obvious to the casual reader/contributor purely based on
> > code review.

SSI8 with pin sharing is not only this function issue,
you can see same comment on rsnd_adg_set_ssi_clk().
# It is not clear for me either, I have been forgot about it :)
# and I have never use SSI8 before, so I'm not sure what happen
# if someone use it

If you have concern about it, why don't you add such error/message
when begining time, instead of each functions ?
Because of compatibility, rsnd_ssi_probe() is not good place,
so I think rsnd_ssi_connect() is good place.

Thank you for your help !!

Best regards
---
Renesas Electronics
Ph.D. Kuninori Morimoto

Re: [PATCH] ASoC: rcar: adg: correct TIMSEL setting for SSI9

[Eugeniu Rosca]

Hello Morimoto-san,

On Thu, Feb 29, 2024 at 12:16:09AM +0000, Kuninori Morimoto wrote:
> > > A number of concerns have been raised internally, related to the fact
> > > that the "optimized/simplified" counter-proposal behaves differently
> > > depending on the value returned by rsnd_ssi_is_pin_sharing().
> (snip)
> > Could you please indicate the sample case of differently behaves ?
> > For example, if xxx was xxx, original code behaves xxx, but simple
> > code behaves xxx, etc. I'm not sure what is your concern...
> 
> Ah.., in case of SSI8.
> 
> > > While it may be clear for you that pin sharing is mandatory, it is not
> > > immediately obvious to the casual reader/contributor purely based on
> > > code review.
> 
> SSI8 with pin sharing is not only this function issue,
> you can see same comment on rsnd_adg_set_ssi_clk().
> # It is not clear for me either, I have been forgot about it :)
> # and I have never use SSI8 before, so I'm not sure what happen
> # if someone use it
> 
> If you have concern about it, why don't you add such error/message
> when begining time, instead of each functions ?
> Because of compatibility, rsnd_ssi_probe() is not good place,
> so I think rsnd_ssi_connect() is good place.

Thanks for your patience and for accepting different viewpoints,
while discussing the issue. Please, kindly review v2:

https://lore.kernel.org/linux-sound/20240301085003.3057-1-erosca@de.adit-jv.com

BR, Eugeniu