| Message ID | 20231029165716.69878-3-otto.pflueger@abscue.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: qcom: check ADSP version when setting clocks | expand |
Thanks Otto for the patch, 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@abscue.de> > --- > 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); --srini > + return ret; > + } > + > cset.clk_set_minor_version = AFE_API_VERSION_CLOCK_SET; > cset.clk_id = clk_id; > cset.clk_freq_in_hz = freq;
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@abscue.de> > > --- > > 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
Hi Stephan, On 04/12/2023 10:34, Stephan Gerhold wrote: > 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@abscue.de> >>> --- >>> 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. Thankyou for explaining this, I see the issue, older firmware versions do not support setting internal digital codec core clock via AFE_PARAM_ID_CLOCK_SET. we could do a better job here to allow NULL port arguments to q6afe_port_set_param_v2 and make this look much better. Could you also add asoc mailing list in CC, I somehow missed these emails. something like this should be much cleaner: --------------------------------------->cut<---------------------------------- diff --git a/sound/soc/qcom/qdsp6/q6afe.c b/sound/soc/qcom/qdsp6/q6afe.c index 91d39f6ad0bd..8dec2c6c7bba 100644 --- a/sound/soc/qcom/qdsp6/q6afe.c +++ b/sound/soc/qcom/qdsp6/q6afe.c @@ -1031,14 +1031,13 @@ static int q6afe_port_set_param(struct q6afe_port *port, void *data, psize, port->token); } -static int q6afe_port_set_param_v2(struct q6afe_port *port, void *data, +static int q6afe_port_set_param_v2(struct q6afe *afe, struct q6afe_port *port, void *data, int param_id, int module_id, int psize) { struct afe_port_cmd_set_param_v2 *param; struct afe_port_param_data_v2 *pdata; - struct q6afe *afe = port->afe; struct apr_pkt *pkt; - u16 port_id = port->id; + u16 port_id = port ? port->id : 0; int ret, pkt_size; void *p, *pl; @@ -1059,7 +1058,7 @@ static int q6afe_port_set_param_v2(struct q6afe_port *port, void *data, pkt->hdr.pkt_size = pkt_size; pkt->hdr.src_port = 0; pkt->hdr.dest_port = 0; - pkt->hdr.token = port->token; + pkt->hdr.token = port ? port->token : 0; pkt->hdr.opcode = AFE_PORT_CMD_SET_PARAM_V2; param->port_id = port_id; @@ -1083,7 +1082,7 @@ static int q6afe_port_set_param_v2(struct q6afe_port *port, void *data, static int q6afe_port_set_lpass_clock(struct q6afe_port *port, struct afe_clk_cfg *cfg) { - return q6afe_port_set_param_v2(port, cfg, + return q6afe_port_set_param_v2(port->afe, port, cfg, AFE_PARAM_ID_LPAIF_CLK_CONFIG, AFE_MODULE_AUDIO_DEV_INTERFACE, sizeof(*cfg)); @@ -1099,7 +1098,7 @@ static int q6afe_set_lpass_clock_v2(struct q6afe_port *port, static int q6afe_set_digital_codec_core_clock(struct q6afe_port *port, struct afe_digital_clk_cfg *cfg) { - return q6afe_port_set_param_v2(port, cfg, + return q6afe_port_set_param_v2(port->afe, port, cfg, AFE_PARAM_ID_INT_DIGITAL_CDC_CLK_CONFIG, AFE_MODULE_AUDIO_DEV_INTERFACE, sizeof(*cfg)); @@ -1117,6 +1116,22 @@ int q6afe_set_lpass_clock(struct device *dev, int clk_id, int attri, cset.clk_attri = attri; cset.clk_root = clk_root; cset.enable = !!freq; + if (q6core_geti_adsp_version() < Q6_ADSP_VERSION_2_7) { + struct afe_digital_clk_cfg dcfg = {0,}; + + switch (clk_id) { + /* DSP firmware versions below 2.7 do not support configuring internal + * digital codec core clk using AFE_PARAM_ID_CLOCK_SET */ + case: Q6AFE_LPASS_CLK_ID_INTERNAL_DIGITAL_CODEC_CORE: + dcfg.i2s_cfg_minor_version = AFE_API_VERSION_I2S_CONFIG; + dcfg.clk_val = freq; + dcfg.clk_root = 5; + return q6afe_set_digital_codec_core_clock(port, &dcfg); + + default: + break; + } + } return q6afe_set_param(afe, NULL, &cset, AFE_PARAM_ID_CLOCK_SET, AFE_MODULE_CLOCK_SET, sizeof(cset), @@ -1493,7 +1508,7 @@ int q6afe_port_start(struct q6afe_port *port) int pkt_size; void *p; - ret = q6afe_port_set_param_v2(port, &port->port_cfg, param_id, + ret = q6afe_port_set_param_v2(afe, port, &port->port_cfg, param_id, AFE_MODULE_AUDIO_DEV_INTERFACE, sizeof(port->port_cfg)); if (ret) { @@ -1503,7 +1518,7 @@ int q6afe_port_start(struct q6afe_port *port) } if (port->scfg) { - ret = q6afe_port_set_param_v2(port, port->scfg, + ret = q6afe_port_set_param_v2(afe, port, port->scfg, AFE_PARAM_ID_PORT_SLOT_MAPPING_CONFIG, AFE_MODULE_TDM, sizeof(*port->scfg)); if (ret) { --------------------------------------->cut<---------------------------------- Thanks, Srini > > 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
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); + return ret; + } + cset.clk_set_minor_version = AFE_API_VERSION_CLOCK_SET; cset.clk_id = clk_id; cset.clk_freq_in_hz = freq;
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@abscue.de> --- sound/soc/qcom/qdsp6/q6afe.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+)