| Message ID | 20230824210135.19303-3-posteuca@mutex.one (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ASoC: amd: acp: Add sound support for a line of HUAWEI laptops | expand |
On Fri, Aug 25, 2023 at 12:01:33AM +0300, Marian Postevca wrote: > +/* In at least one AMD laptop the internal timing of the codec goes off > + * if the MCLK (48Mhz) is not divided by 2. So we will divide all MCLK > + * frequencies above and equal to 48MHz by 2. > + */ > +#define MAX_SUPPORTED_MCLK_FREQ 48000000 Given that the datasheet quotes a maximum MCLK of 51.2MHz I suspect that this is far too high and that performance is degrading well before this point, it sounds like it just so happens that you noticed issues on a machine with this MCLK rather than that's based on the spec. I would instead suggest applying the MCLK divider in any case where we can do so and still generate suitable clocking for the rest of the system, or at least hit 256fs (the datasheet quotes 256/384fs on the front page which suggests it's targetting 256fs, that'd be a fairly normal number, and there's mention of 12/24MHz USB clocks being directly usable). Doing this should either make no odds or result in better performance. It's probably also more power efficient to use a lower MCLK, though most likely the difference is marginal. The earlier in the clock tree the divider is applied the lower more of the chip is clocked and all other things being equal a lower clock usually means lower power.
Mark Brown <broonie@kernel.org> writes: > Given that the datasheet quotes a maximum MCLK of 51.2MHz I suspect that > this is far too high and that performance is degrading well before this > point, it sounds like it just so happens that you noticed issues on a > machine with this MCLK rather than that's based on the spec. I would > instead suggest applying the MCLK divider in any case where we can do so > and still generate suitable clocking for the rest of the system, or at > least hit 256fs (the datasheet quotes 256/384fs on the front page which > suggests it's targetting 256fs, that'd be a fairly normal number, and > there's mention of 12/24MHz USB clocks being directly usable). Doing > this should either make no odds or result in better performance. Not 100% sure what checks should be done for a MCLK to determine if it generates suitable clocking. Would something along this patch make sense? diff --git a/sound/soc/codecs/es8316.c b/sound/soc/codecs/es8316.c index a8f347f1affb..667648de8105 100644 --- a/sound/soc/codecs/es8316.c +++ b/sound/soc/codecs/es8316.c @@ -470,19 +470,36 @@ static int es8316_pcm_hw_params(struct snd_pcm_substream *substream, u8 bclk_divider; u16 lrck_divider; int i; + unsigned int clk = es8316->sysclk / 2; + bool clk_valid = false; + + do { + /* Validate supported sample rates that are autodetected from MCLK */ + for (i = 0; i < NR_SUPPORTED_MCLK_LRCK_RATIOS; i++) { + const unsigned int ratio = supported_mclk_lrck_ratios[i]; + + if (clk % ratio != 0) + continue; + if (clk / ratio == params_rate(params)) + break; + } + if (i == NR_SUPPORTED_MCLK_LRCK_RATIOS) { + if (clk == es8316->sysclk) + return -EINVAL; + else + clk = es8316->sysclk; + } else { + clk_valid = true; + } + } while(!clk_valid); - /* Validate supported sample rates that are autodetected from MCLK */ - for (i = 0; i < NR_SUPPORTED_MCLK_LRCK_RATIOS; i++) { - const unsigned int ratio = supported_mclk_lrck_ratios[i]; - - if (es8316->sysclk % ratio != 0) - continue; - if (es8316->sysclk / ratio == params_rate(params)) - break; + if (clk != es8316->sysclk) { + snd_soc_component_update_bits(component, ES8316_CLKMGR_CLKSW, + ES8316_CLKMGR_CLKSW_MCLK_DIV, + ES8316_CLKMGR_CLKSW_MCLK_DIV); } - if (i == NR_SUPPORTED_MCLK_LRCK_RATIOS) - return -EINVAL; - lrck_divider = es8316->sysclk / params_rate(params); + + lrck_divider = clk / params_rate(params); bclk_divider = lrck_divider / 4; switch (params_format(params)) { case SNDRV_PCM_FORMAT_S16_LE:
On Mon, Aug 28, 2023 at 12:50:45AM +0300, Marian Postevca wrote: > Mark Brown <broonie@kernel.org> writes: > > machine with this MCLK rather than that's based on the spec. I would > > instead suggest applying the MCLK divider in any case where we can do so > > and still generate suitable clocking for the rest of the system, or at > > least hit 256fs (the datasheet quotes 256/384fs on the front page which > > suggests it's targetting 256fs, that'd be a fairly normal number, and > > there's mention of 12/24MHz USB clocks being directly usable). Doing > > this should either make no odds or result in better performance. > Not 100% sure what checks should be done for a MCLK to determine if it > generates suitable clocking. Would something along this patch make > sense? In general a MCLK that allows you to configure the dividers in the CODEC appropriately for use. So long as it works your change looks fine I think modulo. > + do { > + /* Validate supported sample rates that are autodetected from MCLK */ > + for (i = 0; i < NR_SUPPORTED_MCLK_LRCK_RATIOS; i++) { > + const unsigned int ratio = supported_mclk_lrck_ratios[i]; > + > + if (clk % ratio != 0) > + continue; > + if (clk / ratio == params_rate(params)) > + break; > + } Use ARRAY_SIZE()
Mark Brown <broonie@kernel.org> writes: > > In general a MCLK that allows you to configure the dividers in the CODEC > appropriately for use. So long as it works your change looks fine I > think modulo. Sorry, maybe this question is dumb, but I am not familiar with this expression. What do you mean by "your change looks fine I think modulo"? >> + do { >> + /* Validate supported sample rates that are autodetected from MCLK */ >> + for (i = 0; i < NR_SUPPORTED_MCLK_LRCK_RATIOS; i++) { >> + const unsigned int ratio = supported_mclk_lrck_ratios[i]; >> + >> + if (clk % ratio != 0) >> + continue; >> + if (clk / ratio == params_rate(params)) >> + break; >> + } > > Use ARRAY_SIZE() > Do you mean instead of all instances of NR_SUPPORTED_MCLK_LRCK_RATIOS? If so, it's already defined as: #define NR_SUPPORTED_MCLK_LRCK_RATIOS ARRAY_SIZE(supported_mclk_lrck_ratios) If not, then I don't see where else I could use it.
On Mon, Aug 28, 2023 at 11:22:15PM +0300, Marian Postevca wrote: > Mark Brown <broonie@kernel.org> writes: > > In general a MCLK that allows you to configure the dividers in the CODEC > > appropriately for use. So long as it works your change looks fine I > > think modulo. > Sorry, maybe this question is dumb, but I am not familiar with this > expression. What do you mean by "your change looks fine I think modulo"? "modulo" means "apart from". > > Use ARRAY_SIZE() > Do you mean instead of all instances of NR_SUPPORTED_MCLK_LRCK_RATIOS? > If so, it's already defined as: > #define NR_SUPPORTED_MCLK_LRCK_RATIOS ARRAY_SIZE(supported_mclk_lrck_ratios) Yes, that's what I meant - it might be as well to just drop the define then since it's prompting this question at use sites? One of the goals of ARRAY_SIZE() is to avoid having such defines and keep them in sync.
diff --git a/sound/soc/codecs/es8316.c b/sound/soc/codecs/es8316.c index 09fc0b25f600..b506cfb9bd5d 100644 --- a/sound/soc/codecs/es8316.c +++ b/sound/soc/codecs/es8316.c @@ -26,12 +26,19 @@ /* In slave mode at single speed, the codec is documented as accepting 5 * MCLK/LRCK ratios, but we also add ratio 400, which is commonly used on * Intel Cherry Trail platforms (19.2MHz MCLK, 48kHz LRCK). + * Ratio 1000 is needed for at least one AMD SKU where MCLK is 48Mhz. */ #define NR_SUPPORTED_MCLK_LRCK_RATIOS ARRAY_SIZE(supported_mclk_lrck_ratios) static const unsigned int supported_mclk_lrck_ratios[] = { - 256, 384, 400, 500, 512, 768, 1024 + 256, 384, 400, 500, 512, 768, 1000, 1024 }; +/* In at least one AMD laptop the internal timing of the codec goes off + * if the MCLK (48Mhz) is not divided by 2. So we will divide all MCLK + * frequencies above and equal to 48MHz by 2. + */ +#define MAX_SUPPORTED_MCLK_FREQ 48000000 + struct es8316_priv { struct mutex lock; struct clk *mclk; @@ -470,6 +477,7 @@ static int es8316_pcm_hw_params(struct snd_pcm_substream *substream, u8 bclk_divider; u16 lrck_divider; int i; + unsigned int mclk_div = 1; /* Validate supported sample rates that are autodetected from MCLK */ for (i = 0; i < NR_SUPPORTED_MCLK_LRCK_RATIOS; i++) { @@ -482,7 +490,15 @@ static int es8316_pcm_hw_params(struct snd_pcm_substream *substream, } if (i == NR_SUPPORTED_MCLK_LRCK_RATIOS) return -EINVAL; - lrck_divider = es8316->sysclk / params_rate(params); + + if (es8316->sysclk >= MAX_SUPPORTED_MCLK_FREQ) { + snd_soc_component_update_bits(component, ES8316_CLKMGR_CLKSW, + ES8316_CLKMGR_CLKSW_MCLK_DIV, + ES8316_CLKMGR_CLKSW_MCLK_DIV); + mclk_div = 2; + } + + lrck_divider = es8316->sysclk / params_rate(params) / mclk_div; bclk_divider = lrck_divider / 4; switch (params_format(params)) { case SNDRV_PCM_FORMAT_S16_LE: diff --git a/sound/soc/codecs/es8316.h b/sound/soc/codecs/es8316.h index c335138e2837..0ff16f948690 100644 --- a/sound/soc/codecs/es8316.h +++ b/sound/soc/codecs/es8316.h @@ -129,4 +129,7 @@ #define ES8316_GPIO_FLAG_GM_NOT_SHORTED 0x02 #define ES8316_GPIO_FLAG_HP_NOT_INSERTED 0x04 +/* ES8316_CLKMGR_CLKSW */ +#define ES8316_CLKMGR_CLKSW_MCLK_DIV 0x80 + #endif
To properly support a line of Huawei laptops with AMD CPU and a ES8336 codec connected to the ACP3X module we need to enable the codec option to divide the MCLK by 2. The option to divide the MCLK will be enabled for one SKU with a 48Mhz MCLK. This frequency seems to be too high for the codec and leads to distorted sounds when the option is not enabled. Signed-off-by: Marian Postevca <posteuca@mutex.one> --- sound/soc/codecs/es8316.c | 20 ++++++++++++++++++-- sound/soc/codecs/es8316.h | 3 +++ 2 files changed, 21 insertions(+), 2 deletions(-)