| Message ID | 20240223163502.11619-1-erosca@de.adit-jv.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ASoC: rcar: adg: correct TIMSEL setting for SSI9 | expand |
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
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
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
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
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
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
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
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,