[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <DDDW9FN6B097.188MIFGQOOSQW@linaro.org>
Date: Thu, 09 Oct 2025 16:25:03 +0100
From: "Alexey Klimov" <alexey.klimov@...aro.org>
To: "Prasad Kumpatla" <prasad.kumpatla@....qualcomm.com>, "Srinivas
Kandagatla" <srini@...nel.org>, "Liam Girdwood" <lgirdwood@...il.com>,
"Mark Brown" <broonie@...nel.org>, "Rob Herring" <robh@...nel.org>,
"Krzysztof Kozlowski" <krzk+dt@...nel.org>, "Conor Dooley"
<conor+dt@...nel.org>, "Jaroslav Kysela" <perex@...ex.cz>, "Takashi Iwai"
<tiwai@...e.com>, "Rao Mandadapu" <quic_srivasam@...cinc.com>
Cc: <linux-arm-msm@...r.kernel.org>, <linux-sound@...r.kernel.org>,
<devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<kernel@....qualcomm.com>, <aiqun.yu@....qualcomm.com>,
<tingwei.zhang@....qualcomm.com>, <trilok.soni@....qualcomm.com>,
<yijie.yang@....qualcomm.com>, <jingyi.wang@....qualcomm.com>,
<konrad.dybcio@....qualcomm.com>
Subject: Re: [PATCH v2 1/5] ASoC: codecs: va-macro: Rework version checking
On Thu Oct 9, 2025 at 3:36 PM BST, Prasad Kumpatla wrote:
> From: Konrad Dybcio <konrad.dybcio@....qualcomm.com>
>
> Open-code some of the registers to make the checks anywhere near human-
> readable. Error out if the version is unsupported or if the VA macro
> isn't supposed to be present within this LPASS instance (since we can
> check for that now).
[...]
> +static int va_macro_set_lpass_codec_version(struct va_macro *va)
> {
> - int core_id_0 = 0, core_id_1 = 0, core_id_2 = 0;
> int version = LPASS_CODEC_VERSION_UNKNOWN;
> + u32 maj, min, step;
> + u32 val;
>
> - regmap_read(va->regmap, CDC_VA_TOP_CSR_CORE_ID_0, &core_id_0);
> - regmap_read(va->regmap, CDC_VA_TOP_CSR_CORE_ID_1, &core_id_1);
> - regmap_read(va->regmap, CDC_VA_TOP_CSR_CORE_ID_2, &core_id_2);
> + regmap_read(va->regmap, CDC_VA_TOP_CSR_CORE_ID_0, &val);
> + maj = FIELD_GET(CORE_ID_0_REV_MAJ, val);
>
> - if ((core_id_0 == 0x01) && (core_id_1 == 0x0F))
> - version = LPASS_CODEC_VERSION_2_0;
> - if ((core_id_0 == 0x02) && (core_id_1 == 0x0F) && core_id_2 == 0x01)
> + regmap_read(va->regmap, CDC_VA_TOP_CSR_CORE_ID_1, &val);
> + if (!FIELD_GET(CORE_ID_1_HAS_VAMACRO, val)) {
> + dev_err(va->dev, "This is not a VA macro instance\n");
> + return -ENODEV;
> + }
> +
> + regmap_read(va->regmap, CDC_VA_TOP_CSR_CORE_ID_2, &val);
> + min = FIELD_GET(CORE_ID_2_REV_MIN, val);
> + step = FIELD_GET(CORE_ID_2_REV_STEP, val);
> +
> + if (maj == 1) {
> version = LPASS_CODEC_VERSION_2_0;
> - if ((core_id_0 == 0x02) && (core_id_1 == 0x0E))
> - version = LPASS_CODEC_VERSION_2_1;
> - if ((core_id_0 == 0x02) && (core_id_1 == 0x0F) && (core_id_2 == 0x50 || core_id_2 == 0x51))
> - version = LPASS_CODEC_VERSION_2_5;
> - if ((core_id_0 == 0x02) && (core_id_1 == 0x0F) && (core_id_2 == 0x60 || core_id_2 == 0x61))
> - version = LPASS_CODEC_VERSION_2_6;
> - if ((core_id_0 == 0x02) && (core_id_1 == 0x0F) && (core_id_2 == 0x70 || core_id_2 == 0x71))
> - version = LPASS_CODEC_VERSION_2_7;
> - if ((core_id_0 == 0x02) && (core_id_1 == 0x0F) && (core_id_2 == 0x80 || core_id_2 == 0x81))
> - version = LPASS_CODEC_VERSION_2_8;
> - if ((core_id_0 == 0x02) && (core_id_1 == 0x0F) && (core_id_2 == 0x90 || core_id_2 == 0x91))
> - version = LPASS_CODEC_VERSION_2_9;
> -
> - if (version == LPASS_CODEC_VERSION_UNKNOWN)
> - dev_warn(va->dev, "Unknown Codec version, ID: %02x / %02x / %02x\n",
> - core_id_0, core_id_1, core_id_2);
> + } else if (maj == 2) {
> + switch (min) {
> + case 0:
> + version = LPASS_CODEC_VERSION_2_0;
> + break;
> + case 5:
> + version = LPASS_CODEC_VERSION_2_5;
> + break;
> + case 6:
> + version = LPASS_CODEC_VERSION_2_6;
> + break;
> + case 7:
> + version = LPASS_CODEC_VERSION_2_7;
> + break;
> + case 8:
> + version = LPASS_CODEC_VERSION_2_8;
> + break;
> + case 9:
> + version = LPASS_CODEC_VERSION_2_9;
> + break;
> + default:
> + break;
> + }
> + }
> +
> + if (version == LPASS_CODEC_VERSION_UNKNOWN) {
> + dev_err(va->dev, "VA Macro v%u.%u.%u is not supported\n",
> + maj, min, step);
> + return -EOPNOTSUPP;
> + }
Why?
As far as I understand the behaviour before this change is to continue
even with unsupported LPASS va macro version. IIRC when I enabled sound
on Kaanapali QRD device it worked even with unsupported version, it just
needed a fix to calm down the warning.
Why this needed to be changed to error out as unsupported now? Will there
be a permanent damage to hw/fw if we continue?
Shouldn't this print a warning that execution continues regardless but
with low expectations?
I kinda get why we should give up if va macro instance is not present
but not about change of behaviour here.
> lpass_macro_set_codec_version(version);
>
> dev_dbg(va->dev, "LPASS Codec Version %s\n", lpass_macro_get_codec_version_string(version));
> +
> + return 0;
> }
>
> static int va_macro_probe(struct platform_device *pdev)
> @@ -1594,10 +1626,14 @@ static int va_macro_probe(struct platform_device *pdev)
> * old version of codecs do not have a reliable way to determine the
> * version from registers, get them from soc specific data
> */
> - if (data->version)
> + if (data->version) {
> lpass_macro_set_codec_version(data->version);
> - else /* read version from register */
> - va_macro_set_lpass_codec_version(va);
> + } else {
> + /* read version from register */
> + ret = va_macro_set_lpass_codec_version(va);
> + if (ret)
> + return ret;
> + }
Thanks,
Alexey
Powered by blists - more mailing lists