Re: [PATCH v2 2/4] ASoC: qcom: q6afe: provide fallback for digital codec clock

[Stephan Gerhold <stephan@xxxxxxxxxxx>]

Hi Srini,

On Fri, Nov 17, 2023 at 01:26:43PM +0000, Srinivas Kandagatla wrote:
> On 29/10/2023 16:57, Otto Pflüger wrote:
> > When q6afe is used as a clock provider through q6afe-clocks.c, it uses
> > an interface for setting clocks that is not present in older firmware
> > versions. However, using Q6AFE_LPASS_CLK_ID_INTERNAL_DIGITAL_CODEC_CORE
> > as the codec MCLK in the device tree can be useful on older platforms
> > too. Provide a fallback that sets this clock using the old method when
> > an old firmware version is detected.
> > 
> > MSM8916 did not need this because of a workaround that controls this
> > clock directly through the GCC driver, but newer SoCs do not have this
> > clock in their GCC drivers because it is meant to be controlled by the
> > DSP.
> > 
> > Signed-off-by: Otto Pflüger <otto.pflueger@xxxxxxxxx>
> > ---
> >   sound/soc/qcom/qdsp6/q6afe.c | 26 ++++++++++++++++++++++++++
> >   1 file changed, 26 insertions(+)
> > 
> > diff --git a/sound/soc/qcom/qdsp6/q6afe.c b/sound/soc/qcom/qdsp6/q6afe.c
> > index 91d39f6ad0bd..f14d3b366aa4 100644
> > --- a/sound/soc/qcom/qdsp6/q6afe.c
> > +++ b/sound/soc/qcom/qdsp6/q6afe.c
> > @@ -1111,6 +1111,32 @@ int q6afe_set_lpass_clock(struct device *dev, int clk_id, int attri,
> >   	struct q6afe *afe = dev_get_drvdata(dev->parent);
> >   	struct afe_clk_set cset = {0,};
> > +	/*
> > +	 * v2 clocks specified in the device tree may not be supported by the
> > +	 * firmware. If this is the digital codec core clock, fall back to the
> > +	 * old method for setting it.
> > +	 */
> > +	if (q6core_get_adsp_version() < Q6_ADSP_VERSION_2_7) {
> > +		struct q6afe_port *port;
> > +		struct afe_digital_clk_cfg dcfg = {0,};
> > +		int ret;
> > +
> > +		if (clk_id != Q6AFE_LPASS_CLK_ID_INTERNAL_DIGITAL_CODEC_CORE)
> > +			return -EINVAL;
> > +
> > +		port = q6afe_port_get_from_id(dev, PRIMARY_MI2S_RX);
> > +		if (IS_ERR(port))
> > +			return PTR_ERR(port);
> > +
> > +		dcfg.i2s_cfg_minor_version = AFE_API_VERSION_I2S_CONFIG;
> > +		dcfg.clk_val = freq;
> > +		dcfg.clk_root = 5;
> > +		ret = q6afe_set_digital_codec_core_clock(port, &dcfg);
> > +
> > +		q6afe_port_put(port);
> 
> This redirection of clks looks totally confusing and hacky.
> Why can not we do this from machine driver based something like this:
> 
> if (q6core_get_adsp_version() < Q6_ADSP_VERSION_2_7)
> 	ret = snd_soc_dai_set_sysclk(cpu_dai, LPAIF_DIG_CLK, freq, 0);
> 

Unfortunately this doesn't work for the digital codec clock. This clock
is consumed and dynamically controlled by the msm8916-wcd-digital driver
via the clk subsystem, not via ASoC sysclks.

Using q6afe-clocks a typical setup in the DT would look like:

	lpass_codec: audio-codec@771c000 {
		compatible = "qcom,msm8916-wcd-digital-codec";
		reg = <0x0771c000 0x400>;
		clocks = <&xo_board>,
			 <&q6afecc LPASS_CLK_ID_INTERNAL_DIGITAL_CODEC_CORE
				   LPASS_CLK_ATTRIBUTE_COUPLE_NO>;
		clock-names = "ahbix-clk", "mclk";
		#sound-dai-cells = <1>;
	};

However, this works only for newer firmware versions (>= 2.7), not for
older firmware versions controlling this clock via LPAIF_DIG_CLK.

We need a way to describe this independent from the firmware version in
the DT, since there are SoCs such as MSM8909 where both firmware
versions have been used (perhaps even on the same device depending on
which firmware version you have installed).

Any solution involving the machine driver will inevitably end up in a
"chicken-and-egg" problem: The machine driver needs the codec drivers to
probe successfully, but the msm8916-wcd-digital driver first needs the
digital codec clock (mclk) to probe successfully.

I agree that these kind of checks in the qdsp6 code are a bit hacky, but
in my opinion it's still far better than exposing this firmware
detection problem to the device tree.

Do you see any other straightforward way to solve this for the digital
codec clock consumed by msm8916-wcd-digital?

Thanks,
Stephan