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@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.
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
