Fix Tegra194 HDA regression
Subsystems (5)
v3 cover Fix Tegra194 HDA regression
HDA probe failure is observed on Tegra194 based platforms and this happens due to reset failure. This series fixes the problem by skipping the failing reset and DT bindings are updated accordingly.
Changelog
=========
v2 -> v3:
---------
* Use reset bulk APIs in HDA driver as suggested by Dmitry.
v1 -> v2:
---------
* Updated HDA driver patch to skip the failing reset instead of skipping resets in general for BPMP devices as per comment from Dmitry.
* Used a better strucure name for SoC data as per comment from Thierry.
* Dropped 'Fixes' tag in binding doc patch as per comment from Dmitry.
Sameer Pujar (3):
ALSA: hda/tegra: Fix Tegra194 HDA reset failure
dt-bindings: sound: tegra: Update HDA resets
arm64: tegra: Remove non existent Tegra194 reset
.../bindings/sound/nvidia,tegra30-hda.yaml | 13 +++++-- arch/arm64/boot/dts/nvidia/tegra194.dtsi | 5 +-- sound/pci/hda/hda_tegra.c | 45 +++++++++++++++++----- 3 files changed, 47 insertions(+), 16 deletions(-)
Signature · 1 line
2.7.4
PATCH v3 1/3 ALSA: hda/tegra: Fix Tegra194 HDA reset failure
HDA regression is recently reported on Tegra194 based platforms. This happens because "hda2codec_2x" reset does not really exist in Tegra194 and it causes probe failure. All the HDA based audio tests fail at the moment. This underlying issue is exposed by commit c045ceb5a145 ("reset: tegra-bpmp: Handle errors in BPMP response") which now checks return code of BPMP command response. Fix this issue by skipping unavailable reset on Tegra194.
Depends-on: 87f0e46e7559 ("ALSA: hda/tegra: Reset hardware")
--- sound/pci/hda/hda_tegra.c | 45 ++++++++++++++++++++++++++++++++++++--------- 1 file changed, 36 insertions(+), 9 deletions(-)
sound/pci/hda/hda_tegra.c +36 -9
@@ -68,14 +68,20 @@*/ #define TEGRA194_NUM_SDO_LINES 4 +struct hda_tegra_soc { +bool has_hda2codec_2x_reset; +}; +struct hda_tegra {struct azx chip;struct device *dev; -struct reset_control *reset; +struct reset_control_bulk_data resets[3 ];struct clk_bulk_data clocks[3 ]; +unsigned int nresets;unsigned int nclocks;void __iomem *regs;struct work_struct probe_work; +const struct hda_tegra_soc *data; };#ifdef CONFIG_PM
@@ -170,7 +176,7 @@ static int __maybe_unused hda_tegra_runtime_resume(struct device *dev)int rc;if (!chip->running) { - rc =reset_control_assert (hda->reset); + rc =reset_control_bulk_assert (hda->nresets, hda->resets);if (rc)return rc; }
@@ -187,7 +193,7 @@ static int __maybe_unused hda_tegra_runtime_resume(struct device *dev) }else {usleep_range (10 ,100 ); - rc =reset_control_deassert (hda->reset); + rc =reset_control_bulk_deassert (hda->nresets, hda->resets);if (rc)return rc; }
@@ -427,9 +433,17 @@ static int hda_tegra_create(struct snd_card *card,return 0 ; } +static const struct hda_tegra_soc tegra30_data = { + .has_hda2codec_2x_reset = true, +}; + +static const struct hda_tegra_soc tegra194_data = { + .has_hda2codec_2x_reset = false, +}; +static const struct of_device_id hda_tegra_match[] = { - { .compatible ="nvidia,tegra30-hda" }, - { .compatible ="nvidia,tegra194-hda" }, + { .compatible ="nvidia,tegra30-hda" , .data = &tegra30_data }, + { .compatible ="nvidia,tegra194-hda" , .data = &tegra194_data }, {}, };MODULE_DEVICE_TABLE (of, hda_tegra_match);
@@ -449,6 +463,10 @@ static int hda_tegra_probe(struct platform_device *pdev) hda->dev = &pdev->dev; chip = &hda->chip; + hda->data =of_device_get_match_data (&pdev->dev); +if (!hda->data) +return -EINVAL ; + err =snd_card_new (&pdev->dev,SNDRV_DEFAULT_IDX1 ,SNDRV_DEFAULT_STR1 ,THIS_MODULE ,0 , &card);if (err <0 ) {
@@ -456,11 +474,20 @@ static int hda_tegra_probe(struct platform_device *pdev)return err; } - hda->reset =devm_reset_control_array_get_exclusive (&pdev->dev); -if (IS_ERR (hda->reset)) { - err =PTR_ERR (hda->reset); + hda->resets[hda->nresets++].id ="hda" ; + hda->resets[hda->nresets++].id ="hda2hdmi" ; +/* +* "hda2codec_2x" reset is not present on Tegra194. Though DT would +* be updated to reflect this, but to have backward compatibility +* below is necessary. +*/ +if (hda->data->has_hda2codec_2x_reset) + hda->resets[hda->nresets++].id ="hda2codec_2x" ; + + err =devm_reset_control_bulk_get_exclusive (&pdev->dev, hda->nresets, + hda->resets); +if (err) goto out_free; - } hda->clocks[hda->nclocks++].id ="hda" ; hda->clocks[hda->nclocks++].id ="hda2hdmi" ;
Signature · 1 line
2.7.4
Re: [PATCH v3 1/3] ALSA: hda/tegra: Fix Tegra194 HDA reset failure
22.12.2021 07:35, Sameer Pujar пишет:
Quoted 11 lines
HDA regression is recently reported on Tegra194 based platforms. This happens because "
hda2codec_2x" reset does not really exist in Tegra194 and it causes probe failure. All the HDA based audio tests fail at the moment. This underlying issue is exposed by commitc045ceb5a145("reset: tegra-bpmp: Handle errors in BPMP response") which now checks return code of BPMP command response. Fix this issue by skipping unavailable reset on Tegra194.Signed-off-by Sameer Pujar <[email protected]>Depends-on:
87f0e46e7559("ALSA: hda/tegra: Reset hardware")
Is "Depends-on" a valid tag? I can't find it in Documentation/.
Quoted 69 lines
--- sound/pci/hda/hda_tegra.c | 45 ++++++++++++++++++++++++++++++++++++--------- 1 file changed, 36 insertions(+), 9 deletions(-)sound/pci/hda/hda_tegra.c +19
-5@@ -68,14 +68,20 @@*/ #define TEGRA194_NUM_SDO_LINES 4 +struct hda_tegra_soc { +bool has_hda2codec_2x_reset; +}; +struct hda_tegra {struct azx chip;struct device *dev; -struct reset_control *reset; +struct reset_control_bulk_data resets[3 ];struct clk_bulk_data clocks[3 ]; +unsigned int nresets;unsigned int nclocks;void __iomem *regs;struct work_struct probe_work; +const struct hda_tegra_soc *data; };#ifdef CONFIG_PM @@ -170,7 +176,7 @@ static int __maybe_unused hda_tegra_runtime_resume(struct device *dev)int rc;if (!chip->running) { - rc =reset_control_assert (hda->reset); + rc =reset_control_bulk_assert (hda->nresets, hda->resets);if (rc)return rc; }@@ -187,7 +193,7 @@ static int __maybe_unused hda_tegra_runtime_resume(struct device *dev) }else {usleep_range (10 ,100 ); - rc =reset_control_deassert (hda->reset); + rc =reset_control_bulk_deassert (hda->nresets, hda->resets);if (rc)return rc; }@@ -427,9 +433,17 @@ static int hda_tegra_create(struct snd_card *card,return 0 ; } +static const struct hda_tegra_soc tegra30_data = { + .has_hda2codec_2x_reset = true, +}; + +static const struct hda_tegra_soc tegra194_data = { + .has_hda2codec_2x_reset = false, +}; +static const struct of_device_id hda_tegra_match[] = { - { .compatible ="nvidia,tegra30-hda" }, - { .compatible ="nvidia,tegra194-hda" }, + { .compatible ="nvidia,tegra30-hda" , .data = &tegra30_data }, + { .compatible ="nvidia,tegra194-hda" , .data = &tegra194_data }, {}, };MODULE_DEVICE_TABLE (of, hda_tegra_match);
sound/pci/hda/hda_tegra.c static int hda_tegra_probe(struct platform_device *pdev) · line 468
Quoted 6 lines
sound/pci/hda/hda_tegra.c +3
-0@@ -449,6 +463,10 @@ static int hda_tegra_probe(struct platform_device *pdev) hda->dev = &pdev->dev; chip = &hda->chip; + hda->data =of_device_get_match_data (&pdev->dev); +if (!hda->data) +return -EINVAL ;
hda->data can't ever be NULL because all hda_tegra_match[] compatibles above have .data assigned. Technically this check is redundant.
Thierry suggested previously to name it "hda->soc", like we usually do it in other drivers.
Quoted 19 lines
sound/pci/hda/hda_tegra.c +8
-3err =snd_card_new (&pdev->dev,SNDRV_DEFAULT_IDX1 ,SNDRV_DEFAULT_STR1 ,THIS_MODULE ,0 , &card);if (err <0 ) {@@ -456,11 +474,20 @@ static int hda_tegra_probe(struct platform_device *pdev)return err; } - hda->reset =devm_reset_control_array_get_exclusive (&pdev->dev); -if (IS_ERR (hda->reset)) { - err =PTR_ERR (hda->reset); + hda->resets[hda->nresets++].id ="hda" ; + hda->resets[hda->nresets++].id ="hda2hdmi" ; +/* +* "hda2codec_2x" reset is not present on Tegra194. Though DT would +* be updated to reflect this, but to have backward compatibility +* below is necessary. +*/ +if (hda->data->has_hda2codec_2x_reset)
sound/pci/hda/hda_tegra.c static int hda_tegra_probe(struct platform_device *pdev) · line 493
Quoted 10 lines
sound/pci/hda/hda_tegra.c +5
-1+ hda->resets[hda->nresets++].id ="hda2codec_2x" ; + + err =devm_reset_control_bulk_get_exclusive (&pdev->dev, hda->nresets, + hda->resets); +if (err) goto out_free; - } hda->clocks[hda->nclocks++].id ="hda" ; hda->clocks[hda->nclocks++].id ="hda2hdmi" ;
Not sure whether the above nits worth making v4. I'll leave it up to you and other reviewers to decide.
Overall this patch looks good to me, thank you.
Re: [PATCH v3 1/3] ALSA: hda/tegra: Fix Tegra194 HDA reset failure
Quoted 9 lines · Dmitry Osipenko
22.12.2021 07:35, Sameer Pujar пишет:
Quoted 8 lines
HDA regression is recently reported on Tegra194 based platforms. This happens because "
hda2codec_2x" reset does not really exist in Tegra194 and it causes probe failure. All the HDA based audio tests fail at the moment. This underlying issue is exposed by commitc045ceb5a145("reset: tegra-bpmp: Handle errors in BPMP response") which now checks return code of BPMP command response. Fix this issue by skipping unavailable reset on Tegra194.
Quoted 4 lines
Quoted 3 lines
Signed-off-by Sameer Pujar <[email protected]>Depends-on:
87f0e46e7559("ALSA: hda/tegra: Reset hardware")Is "Depends-on" a valid tag? I can't find it in
Documentation/.
I do find the usage of the tag in many commits though there is no reference of this in doc. I always thought it would act as a reference when commits get pulled to other branches. If this is not true and it does not mean anything, I will drop this.
Quoted 73 lines
Quoted 73 lines
--- sound/pci/hda/hda_tegra.c | 45 ++++++++++++++++++++++++++++++++++++--------- 1 file changed, 36 insertions(+), 9 deletions(-)sound/pci/hda/hda_tegra.c +19
-5@@ -68,14 +68,20 @@*/ #define TEGRA194_NUM_SDO_LINES 4 +struct hda_tegra_soc { +bool has_hda2codec_2x_reset; +}; +struct hda_tegra {struct azx chip;struct device *dev; -struct reset_control *reset; +struct reset_control_bulk_data resets[3 ];struct clk_bulk_data clocks[3 ]; +unsigned int nresets;unsigned int nclocks;void __iomem *regs;struct work_struct probe_work; +const struct hda_tegra_soc *data; };#ifdef CONFIG_PM @@ -170,7 +176,7 @@ static int __maybe_unused hda_tegra_runtime_resume(struct device *dev)int rc;if (!chip->running) { - rc =reset_control_assert (hda->reset); + rc =reset_control_bulk_assert (hda->nresets, hda->resets);if (rc)return rc; }@@ -187,7 +193,7 @@ static int __maybe_unused hda_tegra_runtime_resume(struct device *dev) }else {usleep_range (10 ,100 ); - rc =reset_control_deassert (hda->reset); + rc =reset_control_bulk_deassert (hda->nresets, hda->resets);if (rc)return rc; }@@ -427,9 +433,17 @@ static int hda_tegra_create(struct snd_card *card,return 0 ; } +static const struct hda_tegra_soc tegra30_data = { + .has_hda2codec_2x_reset = true, +}; + +static const struct hda_tegra_soc tegra194_data = { + .has_hda2codec_2x_reset = false, +}; +static const struct of_device_id hda_tegra_match[] = { - { .compatible ="nvidia,tegra30-hda" }, - { .compatible ="nvidia,tegra194-hda" }, + { .compatible ="nvidia,tegra30-hda" , .data = &tegra30_data }, + { .compatible ="nvidia,tegra194-hda" , .data = &tegra194_data }, {}, };MODULE_DEVICE_TABLE (of, hda_tegra_match);@@ -449,6 +463,10 @@ static int hda_tegra_probe(struct platform_device *pdev) hda->dev = &pdev->dev; chip = &hda->chip;
Quoted 5 lines
sound/pci/hda/hda_tegra.c static int hda_tegra_probe(struct platform_device *pdev) · line 468
Quoted 3 lines
sound/pci/hda/hda_tegra.c +3
-0+ hda->data =of_device_get_match_data (&pdev->dev); +if (!hda->data) +return -EINVAL ;
hda->datacan't ever be NULL because all hda_tegra_match[] compatibles above have .data assigned. Technically this check is redundant.
Will remove.
Quoted 3 lines
Thierry suggested previously to name it "
hda->soc", like we usually do it in other drivers.
Previously I renamed strcture, but didn't update the member name. Will update.
Quoted 35 lines
Quoted 19 lines
sound/pci/hda/hda_tegra.c +8
-3err =snd_card_new (&pdev->dev,SNDRV_DEFAULT_IDX1 ,SNDRV_DEFAULT_STR1 ,THIS_MODULE ,0 , &card);if (err <0 ) {@@ -456,11 +474,20 @@ static int hda_tegra_probe(struct platform_device *pdev)return err; } - hda->reset =devm_reset_control_array_get_exclusive (&pdev->dev); -if (IS_ERR (hda->reset)) { - err =PTR_ERR (hda->reset); + hda->resets[hda->nresets++].id ="hda" ; + hda->resets[hda->nresets++].id ="hda2hdmi" ; +/* +* "hda2codec_2x" reset is not present on Tegra194. Though DT would +* be updated to reflect this, but to have backward compatibility +* below is necessary. +*/ +if (hda->data->has_hda2codec_2x_reset)sound/pci/hda/hda_tegra.c static int hda_tegra_probe(struct platform_device *pdev) · line 493
Quoted 10 lines
sound/pci/hda/hda_tegra.c +5
-1+ hda->resets[hda->nresets++].id ="hda2codec_2x" ; + + err =devm_reset_control_bulk_get_exclusive (&pdev->dev, hda->nresets, + hda->resets); +if (err) goto out_free; - } hda->clocks[hda->nclocks++].id ="hda" ; hda->clocks[hda->nclocks++].id ="hda2hdmi" ;Not sure whether the above nits worth making v4. I'll leave it up to you and other reviewers to decide.
Overall this patch looks good to me, thank you.
Reviewed-by Dmitry Osipenko <[email protected]>
Greg KH
maintainerRe: [PATCH v3 1/3] ALSA: hda/tegra: Fix Tegra194 HDA reset failure
Quoted 15 lines · Sameer Pujar
Quoted 12 lines · Dmitry Osipenko
22.12.2021 07:35, Sameer Pujar пишет:
Quoted 11 lines
HDA regression is recently reported on Tegra194 based platforms. This happens because "
hda2codec_2x" reset does not really exist in Tegra194 and it causes probe failure. All the HDA based audio tests fail at the moment. This underlying issue is exposed by commitc045ceb5a145("reset: tegra-bpmp: Handle errors in BPMP response") which now checks return code of BPMP command response. Fix this issue by skipping unavailable reset on Tegra194.Signed-off-by Sameer Pujar <[email protected]>Depends-on:
87f0e46e7559("ALSA: hda/tegra: Reset hardware")
Quoted 6 lines
Quoted 1 line
Is "Depends-on" a valid tag? I can't find it in
Documentation/.I do find the usage of the tag in many commits though there is no reference of this in doc. I always thought it would act as a reference when commits get pulled to other branches. If this is not true and it does not mean anything, I will drop this.
It is not true at all, please read:
https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.
Re: [PATCH v3 1/3] ALSA: hda/tegra: Fix Tegra194 HDA reset failure
Quoted 15 lines · Greg KH
Quoted 14 lines · Sameer Pujar
Quoted 12 lines · Dmitry Osipenko
22.12.2021 07:35, Sameer Pujar пишет:
Quoted 11 lines
HDA regression is recently reported on Tegra194 based platforms. This happens because "
hda2codec_2x" reset does not really exist in Tegra194 and it causes probe failure. All the HDA based audio tests fail at the moment. This underlying issue is exposed by commitc045ceb5a145("reset: tegra-bpmp: Handle errors in BPMP response") which now checks return code of BPMP command response. Fix this issue by skipping unavailable reset on Tegra194.Signed-off-by Sameer Pujar <[email protected]>Depends-on:
87f0e46e7559("ALSA: hda/tegra: Reset hardware")
Quoted 8 lines
Quoted 5 lines
Quoted 1 line
Is "Depends-on" a valid tag? I can't find it in
Documentation/.I do find the usage of the tag in many commits though there is no reference of this in doc. I always thought it would act as a reference when commits get pulled to other branches. If this is not true and it does not mean anything, I will drop this.
It is not true at all, please read:
https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.
Thanks Greg for the pointer. I will drop above tag in v4.
PATCH v3 2/3 dt-bindings: sound: tegra: Update HDA resets
In reply to Sameer PujarTegra194 HDA has only two resets unlike the previous generations of Tegra SoCs. Hence update the reset list accordingly.
--- .../devicetree/bindings/sound/nvidia,tegra30-hda.yaml | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)
Documentation/devicetree/bindings/sound/nvidia,tegra30-hda.yaml +9 -4
@@ -50,13 +50,18 @@ properties: - const: hda2codec_2x resets: + minItems: 2 maxItems: 3 reset-names: - items: - - const: hda - - const: hda2hdmi - - const: hda2codec_2x + oneOf: + - items: + - const: hda + - const: hda2hdmi + - const: hda2codec_2x + - items: + - const: hda + - const: hda2hdmi power-domains: maxItems: 1
Signature · 1 line
2.7.4
Rob Herring
maintainerRe: [PATCH v3 2/3] dt-bindings: sound: tegra: Update HDA resets
Quoted 14 lines · Sameer Pujar
Tegra194 HDA has only two resets unlike the previous generations of Tegra SoCs. Hence update the reset list accordingly.
Signed-off-by Sameer Pujar <[email protected]>Acked-by Thierry Reding <[email protected]>--- .../devicetree/bindings/sound/nvidia,tegra30-hda.yaml | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)Documentation/devicetree/bindings/sound/nvidia,tegra30-hda.yaml +0
-0
Documentation/devicetree/bindings/sound/nvidia,tegra30-hda.yaml properties: · line 56
Quoted 7 lines
Documentation/devicetree/bindings/sound/nvidia,tegra30-hda.yaml +1
-0@@ -50,13 +50,18 @@ properties: - const: hda2codec_2x resets: + minItems: 2 maxItems: 3 reset-names:
Just add 'minItems: 2' here instead.
Quoted 19 lines
Documentation/devicetree/bindings/sound/nvidia,tegra30-hda.yaml +8
-4- items: - - const: hda - - const: hda2hdmi - - const: hda2codec_2x + oneOf: + - items: + - const: hda + - const: hda2hdmi + - const: hda2codec_2x + - items: + - const: hda + - const: hda2hdmi power-domains: maxItems: 1Signature · 1 line
2.7.4
PATCH v3 3/3 arm64: tegra: Remove non existent Tegra194 reset
In reply to Sameer PujarTegra194 does not really have "hda2codec_2x" related reset. Hence drop this entry to reflect actual HW.
--- arch/arm64/boot/dts/nvidia/tegra194.dtsi | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
arch/arm64/boot/dts/nvidia/tegra194.dtsi +2 -3
@@ -976,9 +976,8 @@ <&bpmp TEGRA194_CLK_HDA2CODEC_2X>; clock-names = "hda", "hda2hdmi", "hda2codec_2x"; resets = <&bpmp TEGRA194_RESET_HDA>, - <&bpmp TEGRA194_RESET_HDA2HDMICODEC>, - <&bpmp TEGRA194_RESET_HDA2CODEC_2X>; - reset-names = "hda", "hda2hdmi", "hda2codec_2x"; + <&bpmp TEGRA194_RESET_HDA2HDMICODEC>; + reset-names = "hda", "hda2hdmi"; power-domains = <&bpmp TEGRA194_POWER_DOMAIN_DISP>; interconnects = <&mc TEGRA194_MEMORY_CLIENT_HDAR &emc>, <&mc TEGRA194_MEMORY_CLIENT_HDAW &emc>;
Signature · 1 line
2.7.4