[PATCH v2 0/3] Fix Tegra194 HDA regression

[PATCH v2 0/3] Fix Tegra194 HDA regression

[Sameer Pujar]

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
=========
 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                          | 96 +++++++++++++++++++---
 3 files changed, 97 insertions(+), 17 deletions(-)

-- 
2.7.4

[PATCH v2 0/3] Fix Tegra194 HDA regression

[Sameer Pujar]

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
=========
 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                          | 96 +++++++++++++++++++---
 3 files changed, 97 insertions(+), 17 deletions(-)

-- 
2.7.4

[PATCH v2 1/3] ALSA: hda/tegra: Fix Tegra194 HDA reset failure

[Sameer Pujar]

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.

Signed-off-by: Sameer Pujar <spujar@nvidia.com>
Cc: stable@vger.kernel.org
Depends-on: 87f0e46e7559 ("ALSA: hda/tegra: Reset hardware")
---
 sound/pci/hda/hda_tegra.c | 96 ++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 86 insertions(+), 10 deletions(-)

diff --git a/sound/pci/hda/hda_tegra.c b/sound/pci/hda/hda_tegra.c
index ea700395..be010cd 100644
--- a/sound/pci/hda/hda_tegra.c
+++ b/sound/pci/hda/hda_tegra.c
@@ -68,14 +68,21 @@
  */
 #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 *reset_hda;
+	struct reset_control *reset_hda2hdmi;
+	struct reset_control *reset_hda2codec_2x;
 	struct clk_bulk_data clocks[3];
 	unsigned int nclocks;
 	void __iomem *regs;
 	struct work_struct probe_work;
+	const struct hda_tegra_soc *data;
 };
 
 #ifdef CONFIG_PM
@@ -170,9 +177,26 @@ static int __maybe_unused hda_tegra_runtime_resume(struct device *dev)
 	int rc;
 
 	if (!chip->running) {
-		rc = reset_control_assert(hda->reset);
-		if (rc)
+		rc = reset_control_assert(hda->reset_hda);
+		if (rc) {
+			dev_err(dev, "hda reset assert failed, err: %d\n", rc);
+			return rc;
+		}
+
+		rc = reset_control_assert(hda->reset_hda2hdmi);
+		if (rc) {
+			dev_err(dev, "hda2hdmi reset assert failed, err: %d\n",
+				rc);
+			return rc;
+		}
+
+		rc = reset_control_assert(hda->reset_hda2codec_2x);
+		if (rc) {
+			dev_err(dev,
+				"hda2codec_2x reset assert failed, err: %d\n",
+				rc);
 			return rc;
+		}
 	}
 
 	rc = clk_bulk_prepare_enable(hda->nclocks, hda->clocks);
@@ -187,9 +211,27 @@ static int __maybe_unused hda_tegra_runtime_resume(struct device *dev)
 	} else {
 		usleep_range(10, 100);
 
-		rc = reset_control_deassert(hda->reset);
-		if (rc)
+		rc = reset_control_deassert(hda->reset_hda);
+		if (rc) {
+			dev_err(dev, "hda reset deassert failed, err: %d\n",
+				rc);
 			return rc;
+		}
+
+		rc = reset_control_deassert(hda->reset_hda2hdmi);
+		if (rc) {
+			dev_err(dev, "hda2hdmi reset deassert failed, err: %d\n",
+				rc);
+			return rc;
+		}
+
+		rc = reset_control_deassert(hda->reset_hda2codec_2x);
+		if (rc) {
+			dev_err(dev,
+				"hda2codec_2x reset deassert failed, err: %d\n",
+				rc);
+			return rc;
+		}
 	}
 
 	return 0;
@@ -427,9 +469,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 +499,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,12 +510,34 @@ 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->reset_hda = devm_reset_control_get_exclusive(&pdev->dev, "hda");
+	if (IS_ERR(hda->reset_hda)) {
+		err = PTR_ERR(hda->reset_hda);
 		goto out_free;
 	}
 
+	hda->reset_hda2hdmi = devm_reset_control_get_exclusive(&pdev->dev,
+							       "hda2hdmi");
+	if (IS_ERR(hda->reset_hda2hdmi)) {
+		err = PTR_ERR(hda->reset_hda2hdmi);
+		goto out_free;
+	}
+
+	/*
+	 * "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->reset_hda2codec_2x =
+			devm_reset_control_get_exclusive(&pdev->dev,
+							 "hda2codec_2x");
+		if (IS_ERR(hda->reset_hda2codec_2x)) {
+			err = PTR_ERR(hda->reset_hda2codec_2x);
+			goto out_free;
+		}
+	}
+
 	hda->clocks[hda->nclocks++].id = "hda";
 	hda->clocks[hda->nclocks++].id = "hda2hdmi";
 	hda->clocks[hda->nclocks++].id = "hda2codec_2x";
-- 
2.7.4

[PATCH v2 1/3] ALSA: hda/tegra: Fix Tegra194 HDA reset failure

[Sameer Pujar]

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.

Signed-off-by: Sameer Pujar <spujar@nvidia.com>
Cc: stable@vger.kernel.org
Depends-on: 87f0e46e7559 ("ALSA: hda/tegra: Reset hardware")
---
 sound/pci/hda/hda_tegra.c | 96 ++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 86 insertions(+), 10 deletions(-)

diff --git a/sound/pci/hda/hda_tegra.c b/sound/pci/hda/hda_tegra.c
index ea700395..be010cd 100644
--- a/sound/pci/hda/hda_tegra.c
+++ b/sound/pci/hda/hda_tegra.c
@@ -68,14 +68,21 @@
  */
 #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 *reset_hda;
+	struct reset_control *reset_hda2hdmi;
+	struct reset_control *reset_hda2codec_2x;
 	struct clk_bulk_data clocks[3];
 	unsigned int nclocks;
 	void __iomem *regs;
 	struct work_struct probe_work;
+	const struct hda_tegra_soc *data;
 };
 
 #ifdef CONFIG_PM
@@ -170,9 +177,26 @@ static int __maybe_unused hda_tegra_runtime_resume(struct device *dev)
 	int rc;
 
 	if (!chip->running) {
-		rc = reset_control_assert(hda->reset);
-		if (rc)
+		rc = reset_control_assert(hda->reset_hda);
+		if (rc) {
+			dev_err(dev, "hda reset assert failed, err: %d\n", rc);
+			return rc;
+		}
+
+		rc = reset_control_assert(hda->reset_hda2hdmi);
+		if (rc) {
+			dev_err(dev, "hda2hdmi reset assert failed, err: %d\n",
+				rc);
+			return rc;
+		}
+
+		rc = reset_control_assert(hda->reset_hda2codec_2x);
+		if (rc) {
+			dev_err(dev,
+				"hda2codec_2x reset assert failed, err: %d\n",
+				rc);
 			return rc;
+		}
 	}
 
 	rc = clk_bulk_prepare_enable(hda->nclocks, hda->clocks);
@@ -187,9 +211,27 @@ static int __maybe_unused hda_tegra_runtime_resume(struct device *dev)
 	} else {
 		usleep_range(10, 100);
 
-		rc = reset_control_deassert(hda->reset);
-		if (rc)
+		rc = reset_control_deassert(hda->reset_hda);
+		if (rc) {
+			dev_err(dev, "hda reset deassert failed, err: %d\n",
+				rc);
 			return rc;
+		}
+
+		rc = reset_control_deassert(hda->reset_hda2hdmi);
+		if (rc) {
+			dev_err(dev, "hda2hdmi reset deassert failed, err: %d\n",
+				rc);
+			return rc;
+		}
+
+		rc = reset_control_deassert(hda->reset_hda2codec_2x);
+		if (rc) {
+			dev_err(dev,
+				"hda2codec_2x reset deassert failed, err: %d\n",
+				rc);
+			return rc;
+		}
 	}
 
 	return 0;
@@ -427,9 +469,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 +499,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,12 +510,34 @@ 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->reset_hda = devm_reset_control_get_exclusive(&pdev->dev, "hda");
+	if (IS_ERR(hda->reset_hda)) {
+		err = PTR_ERR(hda->reset_hda);
 		goto out_free;
 	}
 
+	hda->reset_hda2hdmi = devm_reset_control_get_exclusive(&pdev->dev,
+							       "hda2hdmi");
+	if (IS_ERR(hda->reset_hda2hdmi)) {
+		err = PTR_ERR(hda->reset_hda2hdmi);
+		goto out_free;
+	}
+
+	/*
+	 * "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->reset_hda2codec_2x =
+			devm_reset_control_get_exclusive(&pdev->dev,
+							 "hda2codec_2x");
+		if (IS_ERR(hda->reset_hda2codec_2x)) {
+			err = PTR_ERR(hda->reset_hda2codec_2x);
+			goto out_free;
+		}
+	}
+
 	hda->clocks[hda->nclocks++].id = "hda";
 	hda->clocks[hda->nclocks++].id = "hda2hdmi";
 	hda->clocks[hda->nclocks++].id = "hda2codec_2x";
-- 
2.7.4

[PATCH v2 2/3] dt-bindings: sound: tegra: Update HDA resets

[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 <spujar@nvidia.com>
Acked-by: Thierry Reding <treding@nvidia.com>
---
 .../devicetree/bindings/sound/nvidia,tegra30-hda.yaml       | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/sound/nvidia,tegra30-hda.yaml b/Documentation/devicetree/bindings/sound/nvidia,tegra30-hda.yaml
index b55775e..70dbdff5 100644
--- a/Documentation/devicetree/bindings/sound/nvidia,tegra30-hda.yaml
+++ b/Documentation/devicetree/bindings/sound/nvidia,tegra30-hda.yaml
@@ -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
-- 
2.7.4

[PATCH v2 2/3] dt-bindings: sound: tegra: Update HDA resets

[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 <spujar@nvidia.com>
Acked-by: Thierry Reding <treding@nvidia.com>
---
 .../devicetree/bindings/sound/nvidia,tegra30-hda.yaml       | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/sound/nvidia,tegra30-hda.yaml b/Documentation/devicetree/bindings/sound/nvidia,tegra30-hda.yaml
index b55775e..70dbdff5 100644
--- a/Documentation/devicetree/bindings/sound/nvidia,tegra30-hda.yaml
+++ b/Documentation/devicetree/bindings/sound/nvidia,tegra30-hda.yaml
@@ -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
-- 
2.7.4

[PATCH v2 3/3] arm64: tegra: Remove non existent Tegra194 reset

[Sameer Pujar]

Tegra194 does not really have "hda2codec_2x" related reset. Hence drop
this entry to reflect actual HW.

Fixes: 4878cc0c9fab ("arm64: tegra: Add HDA controller on Tegra194")
Signed-off-by: Sameer Pujar <spujar@nvidia.com>
---
 arch/arm64/boot/dts/nvidia/tegra194.dtsi | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/boot/dts/nvidia/tegra194.dtsi b/arch/arm64/boot/dts/nvidia/tegra194.dtsi
index 8d29b7f..6a1d896 100644
--- a/arch/arm64/boot/dts/nvidia/tegra194.dtsi
+++ b/arch/arm64/boot/dts/nvidia/tegra194.dtsi
@@ -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>;
-- 
2.7.4

[PATCH v2 3/3] arm64: tegra: Remove non existent Tegra194 reset

[Sameer Pujar]

Tegra194 does not really have "hda2codec_2x" related reset. Hence drop
this entry to reflect actual HW.

Fixes: 4878cc0c9fab ("arm64: tegra: Add HDA controller on Tegra194")
Signed-off-by: Sameer Pujar <spujar@nvidia.com>
---
 arch/arm64/boot/dts/nvidia/tegra194.dtsi | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/boot/dts/nvidia/tegra194.dtsi b/arch/arm64/boot/dts/nvidia/tegra194.dtsi
index 8d29b7f..6a1d896 100644
--- a/arch/arm64/boot/dts/nvidia/tegra194.dtsi
+++ b/arch/arm64/boot/dts/nvidia/tegra194.dtsi
@@ -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>;
-- 
2.7.4

Re: [PATCH v2 1/3] ALSA: hda/tegra: Fix Tegra194 HDA reset failure

[Dmitry Osipenko]

20.12.2021 20:30, Sameer Pujar пишет:
> 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.
> 
> Signed-off-by: Sameer Pujar <spujar@nvidia.com>
> Cc: stable@vger.kernel.org
> Depends-on: 87f0e46e7559 ("ALSA: hda/tegra: Reset hardware")
> ---
>  sound/pci/hda/hda_tegra.c | 96 ++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 86 insertions(+), 10 deletions(-)
> 
> diff --git a/sound/pci/hda/hda_tegra.c b/sound/pci/hda/hda_tegra.c
> index ea700395..be010cd 100644
> --- a/sound/pci/hda/hda_tegra.c
> +++ b/sound/pci/hda/hda_tegra.c
> @@ -68,14 +68,21 @@
>   */
>  #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 *reset_hda;
> +	struct reset_control *reset_hda2hdmi;
> +	struct reset_control *reset_hda2codec_2x;
>  	struct clk_bulk_data clocks[3];
>  	unsigned int nclocks;
>  	void __iomem *regs;
>  	struct work_struct probe_work;
> +	const struct hda_tegra_soc *data;
>  };
>  
>  #ifdef CONFIG_PM
> @@ -170,9 +177,26 @@ static int __maybe_unused hda_tegra_runtime_resume(struct device *dev)
>  	int rc;
>  
>  	if (!chip->running) {
> -		rc = reset_control_assert(hda->reset);
> -		if (rc)
> +		rc = reset_control_assert(hda->reset_hda);
> +		if (rc) {
> +			dev_err(dev, "hda reset assert failed, err: %d\n", rc);
> +			return rc;
> +		}
> +
> +		rc = reset_control_assert(hda->reset_hda2hdmi);
> +		if (rc) {
> +			dev_err(dev, "hda2hdmi reset assert failed, err: %d\n",
> +				rc);
> +			return rc;
> +		}
> +
> +		rc = reset_control_assert(hda->reset_hda2codec_2x);
> +		if (rc) {
> +			dev_err(dev,
> +				"hda2codec_2x reset assert failed, err: %d\n",
> +				rc);
>  			return rc;
> +		}
>  	}
>  
>  	rc = clk_bulk_prepare_enable(hda->nclocks, hda->clocks);
> @@ -187,9 +211,27 @@ static int __maybe_unused hda_tegra_runtime_resume(struct device *dev)
>  	} else {
>  		usleep_range(10, 100);
>  
> -		rc = reset_control_deassert(hda->reset);
> -		if (rc)
> +		rc = reset_control_deassert(hda->reset_hda);
> +		if (rc) {
> +			dev_err(dev, "hda reset deassert failed, err: %d\n",
> +				rc);
>  			return rc;
> +		}
> +
> +		rc = reset_control_deassert(hda->reset_hda2hdmi);
> +		if (rc) {
> +			dev_err(dev, "hda2hdmi reset deassert failed, err: %d\n",
> +				rc);
> +			return rc;
> +		}
> +
> +		rc = reset_control_deassert(hda->reset_hda2codec_2x);
> +		if (rc) {
> +			dev_err(dev,
> +				"hda2codec_2x reset deassert failed, err: %d\n",
> +				rc);
> +			return rc;
> +		}
>  	}
>  
>  	return 0;
> @@ -427,9 +469,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 +499,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,12 +510,34 @@ 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->reset_hda = devm_reset_control_get_exclusive(&pdev->dev, "hda");
> +	if (IS_ERR(hda->reset_hda)) {
> +		err = PTR_ERR(hda->reset_hda);
>  		goto out_free;
>  	}
>  
> +	hda->reset_hda2hdmi = devm_reset_control_get_exclusive(&pdev->dev,
> +							       "hda2hdmi");
> +	if (IS_ERR(hda->reset_hda2hdmi)) {
> +		err = PTR_ERR(hda->reset_hda2hdmi);
> +		goto out_free;
> +	}
> +
> +	/*
> +	 * "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->reset_hda2codec_2x =
> +			devm_reset_control_get_exclusive(&pdev->dev,
> +							 "hda2codec_2x");
> +		if (IS_ERR(hda->reset_hda2codec_2x)) {
> +			err = PTR_ERR(hda->reset_hda2codec_2x);
> +			goto out_free;
> +		}
> +	}
> +
>  	hda->clocks[hda->nclocks++].id = "hda";
>  	hda->clocks[hda->nclocks++].id = "hda2hdmi";
>  	hda->clocks[hda->nclocks++].id = "hda2codec_2x";
> 

All stable kernels affected by this problem that don't support the bulk
reset API are EOL now. Please use bulk reset API like I suggested in the
comment to v1, it will allow us to have a cleaner and nicer code.

The bulk reset code will look similar to the bulk clk API already used
by the HDA driver, you'll only need to skip adding the hda2codec_2x to
resets[3] and switch to use reset_control_bulk_reset_*() variants of the
functions.

Re: [PATCH v2 1/3] ALSA: hda/tegra: Fix Tegra194 HDA reset failure

[Dmitry Osipenko]

20.12.2021 20:30, Sameer Pujar пишет:
> 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.
> 
> Signed-off-by: Sameer Pujar <spujar@nvidia.com>
> Cc: stable@vger.kernel.org
> Depends-on: 87f0e46e7559 ("ALSA: hda/tegra: Reset hardware")
> ---
>  sound/pci/hda/hda_tegra.c | 96 ++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 86 insertions(+), 10 deletions(-)
> 
> diff --git a/sound/pci/hda/hda_tegra.c b/sound/pci/hda/hda_tegra.c
> index ea700395..be010cd 100644
> --- a/sound/pci/hda/hda_tegra.c
> +++ b/sound/pci/hda/hda_tegra.c
> @@ -68,14 +68,21 @@
>   */
>  #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 *reset_hda;
> +	struct reset_control *reset_hda2hdmi;
> +	struct reset_control *reset_hda2codec_2x;
>  	struct clk_bulk_data clocks[3];
>  	unsigned int nclocks;
>  	void __iomem *regs;
>  	struct work_struct probe_work;
> +	const struct hda_tegra_soc *data;
>  };
>  
>  #ifdef CONFIG_PM
> @@ -170,9 +177,26 @@ static int __maybe_unused hda_tegra_runtime_resume(struct device *dev)
>  	int rc;
>  
>  	if (!chip->running) {
> -		rc = reset_control_assert(hda->reset);
> -		if (rc)
> +		rc = reset_control_assert(hda->reset_hda);
> +		if (rc) {
> +			dev_err(dev, "hda reset assert failed, err: %d\n", rc);
> +			return rc;
> +		}
> +
> +		rc = reset_control_assert(hda->reset_hda2hdmi);
> +		if (rc) {
> +			dev_err(dev, "hda2hdmi reset assert failed, err: %d\n",
> +				rc);
> +			return rc;
> +		}
> +
> +		rc = reset_control_assert(hda->reset_hda2codec_2x);
> +		if (rc) {
> +			dev_err(dev,
> +				"hda2codec_2x reset assert failed, err: %d\n",
> +				rc);
>  			return rc;
> +		}
>  	}
>  
>  	rc = clk_bulk_prepare_enable(hda->nclocks, hda->clocks);
> @@ -187,9 +211,27 @@ static int __maybe_unused hda_tegra_runtime_resume(struct device *dev)
>  	} else {
>  		usleep_range(10, 100);
>  
> -		rc = reset_control_deassert(hda->reset);
> -		if (rc)
> +		rc = reset_control_deassert(hda->reset_hda);
> +		if (rc) {
> +			dev_err(dev, "hda reset deassert failed, err: %d\n",
> +				rc);
>  			return rc;
> +		}
> +
> +		rc = reset_control_deassert(hda->reset_hda2hdmi);
> +		if (rc) {
> +			dev_err(dev, "hda2hdmi reset deassert failed, err: %d\n",
> +				rc);
> +			return rc;
> +		}
> +
> +		rc = reset_control_deassert(hda->reset_hda2codec_2x);
> +		if (rc) {
> +			dev_err(dev,
> +				"hda2codec_2x reset deassert failed, err: %d\n",
> +				rc);
> +			return rc;
> +		}
>  	}
>  
>  	return 0;
> @@ -427,9 +469,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 +499,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,12 +510,34 @@ 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->reset_hda = devm_reset_control_get_exclusive(&pdev->dev, "hda");
> +	if (IS_ERR(hda->reset_hda)) {
> +		err = PTR_ERR(hda->reset_hda);
>  		goto out_free;
>  	}
>  
> +	hda->reset_hda2hdmi = devm_reset_control_get_exclusive(&pdev->dev,
> +							       "hda2hdmi");
> +	if (IS_ERR(hda->reset_hda2hdmi)) {
> +		err = PTR_ERR(hda->reset_hda2hdmi);
> +		goto out_free;
> +	}
> +
> +	/*
> +	 * "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->reset_hda2codec_2x =
> +			devm_reset_control_get_exclusive(&pdev->dev,
> +							 "hda2codec_2x");
> +		if (IS_ERR(hda->reset_hda2codec_2x)) {
> +			err = PTR_ERR(hda->reset_hda2codec_2x);
> +			goto out_free;
> +		}
> +	}
> +
>  	hda->clocks[hda->nclocks++].id = "hda";
>  	hda->clocks[hda->nclocks++].id = "hda2hdmi";
>  	hda->clocks[hda->nclocks++].id = "hda2codec_2x";
> 

All stable kernels affected by this problem that don't support the bulk
reset API are EOL now. Please use bulk reset API like I suggested in the
comment to v1, it will allow us to have a cleaner and nicer code.

The bulk reset code will look similar to the bulk clk API already used
by the HDA driver, you'll only need to skip adding the hda2codec_2x to
resets[3] and switch to use reset_control_bulk_reset_*() variants of the
functions.

Re: [PATCH v2 1/3] ALSA: hda/tegra: Fix Tegra194 HDA reset failure

[Sameer Pujar]

On 12/21/2021 6:51 AM, Dmitry Osipenko wrote:
>
> All stable kernels affected by this problem that don't support the bulk
> reset API are EOL now. Please use bulk reset API like I suggested in the
> comment to v1, it will allow us to have a cleaner and nicer code.

Agree that it would be compact and cleaner, but any specific reset 
failure in the group won't be obvious in the logs. In this case it 
failed silently. If compactness is preferred, then may be I can keep an 
error print at group level so that we see some failure context whenever 
it happens.

Re: [PATCH v2 1/3] ALSA: hda/tegra: Fix Tegra194 HDA reset failure

[Sameer Pujar]

On 12/21/2021 6:51 AM, Dmitry Osipenko wrote:
>
> All stable kernels affected by this problem that don't support the bulk
> reset API are EOL now. Please use bulk reset API like I suggested in the
> comment to v1, it will allow us to have a cleaner and nicer code.

Agree that it would be compact and cleaner, but any specific reset 
failure in the group won't be obvious in the logs. In this case it 
failed silently. If compactness is preferred, then may be I can keep an 
error print at group level so that we see some failure context whenever 
it happens.

Re: [PATCH v2 1/3] ALSA: hda/tegra: Fix Tegra194 HDA reset failure

[Dmitry Osipenko]

21.12.2021 09:18, Sameer Pujar пишет:
> 
> 
> On 12/21/2021 6:51 AM, Dmitry Osipenko wrote:
>>
>> All stable kernels affected by this problem that don't support the bulk
>> reset API are EOL now. Please use bulk reset API like I suggested in the
>> comment to v1, it will allow us to have a cleaner and nicer code.
> 
> Agree that it would be compact and cleaner, but any specific reset
> failure in the group won't be obvious in the logs. In this case it
> failed silently. If compactness is preferred, then may be I can keep an
> error print at group level so that we see some failure context whenever
> it happens.

The group shouldn't fail ever unless device-tree is wrong. Why do you
think we should care about the case which realistically won't ever
happen? This is a bit unpractical approach.

If we really care about those error messages, then will be much more
reasonable to add them to the reset core, like clk core does it [1],
IMO. This will be a trivial change. Will you be happy with this variant?

[1]
https://elixir.bootlin.com/linux/v5.16-rc6/source/drivers/clk/clk-bulk.c#L100

diff --git a/drivers/reset/core.c b/drivers/reset/core.c
index 61e688882643..85ce0d6eeb34 100644
--- a/drivers/reset/core.c
+++ b/drivers/reset/core.c
@@ -962,6 +962,11 @@ int __reset_control_bulk_get(struct device *dev,
int num_rstcs,
 						    shared, optional, acquired);
 		if (IS_ERR(rstcs[i].rstc)) {
 			ret = PTR_ERR(rstcs[i].rstc);
+
+			if (ret != -EPROBE_DEFER)
+				dev_err(dev, "Failed to get reset '%s': %d\n",
+					rstcs[i].id, ret);
+
 			goto err;
 		}
 	}

Re: [PATCH v2 1/3] ALSA: hda/tegra: Fix Tegra194 HDA reset failure

[Dmitry Osipenko]

21.12.2021 09:18, Sameer Pujar пишет:
> 
> 
> On 12/21/2021 6:51 AM, Dmitry Osipenko wrote:
>>
>> All stable kernels affected by this problem that don't support the bulk
>> reset API are EOL now. Please use bulk reset API like I suggested in the
>> comment to v1, it will allow us to have a cleaner and nicer code.
> 
> Agree that it would be compact and cleaner, but any specific reset
> failure in the group won't be obvious in the logs. In this case it
> failed silently. If compactness is preferred, then may be I can keep an
> error print at group level so that we see some failure context whenever
> it happens.

The group shouldn't fail ever unless device-tree is wrong. Why do you
think we should care about the case which realistically won't ever
happen? This is a bit unpractical approach.

If we really care about those error messages, then will be much more
reasonable to add them to the reset core, like clk core does it [1],
IMO. This will be a trivial change. Will you be happy with this variant?

[1]
https://elixir.bootlin.com/linux/v5.16-rc6/source/drivers/clk/clk-bulk.c#L100

diff --git a/drivers/reset/core.c b/drivers/reset/core.c
index 61e688882643..85ce0d6eeb34 100644
--- a/drivers/reset/core.c
+++ b/drivers/reset/core.c
@@ -962,6 +962,11 @@ int __reset_control_bulk_get(struct device *dev,
int num_rstcs,
 						    shared, optional, acquired);
 		if (IS_ERR(rstcs[i].rstc)) {
 			ret = PTR_ERR(rstcs[i].rstc);
+
+			if (ret != -EPROBE_DEFER)
+				dev_err(dev, "Failed to get reset '%s': %d\n",
+					rstcs[i].id, ret);
+
 			goto err;
 		}
 	}

Re: [PATCH v2 1/3] ALSA: hda/tegra: Fix Tegra194 HDA reset failure

[Sameer Pujar]

On 12/21/2021 8:50 PM, Dmitry Osipenko wrote:
> 21.12.2021 09:18, Sameer Pujar пишет:
>>
>> On 12/21/2021 6:51 AM, Dmitry Osipenko wrote:
>>> All stable kernels affected by this problem that don't support the bulk
>>> reset API are EOL now. Please use bulk reset API like I suggested in the
>>> comment to v1, it will allow us to have a cleaner and nicer code.
>> Agree that it would be compact and cleaner, but any specific reset
>> failure in the group won't be obvious in the logs. In this case it
>> failed silently. If compactness is preferred, then may be I can keep an
>> error print at group level so that we see some failure context whenever
>> it happens.
> The group shouldn't fail ever unless device-tree is wrong. Why do you
> think we should care about the case which realistically won't ever
> happen? This is a bit unpractical approach.

Though it is very rare that something like this would happen, but can't 
be ruled out completely.

> If we really care about those error messages, then will be much more
> reasonable to add them to the reset core, like clk core does it [1],
> IMO. This will be a trivial change. Will you be happy with this variant?

It would be nicer to know why exactly it failed. Yes, it makes sense to 
have this in the core. I will send v3 with bulk APIs for HDA driver. 
Thank you.

>
> [1]
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Felixir.bootlin.com%2Flinux%2Fv5.16-rc6%2Fsource%2Fdrivers%2Fclk%2Fclk-bulk.c%23L100&data=04%7C01%7Cspujar%40nvidia.com%7C53e278c9a4804612f74b08d9c49564a0%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637756968218491760%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=QEe8SlSdAN1N8nOu3XqtAdbXP1JbtMBPlswqnBIhq5w%3D&reserved=0
>
> diff --git a/drivers/reset/core.c b/drivers/reset/core.c
> index 61e688882643..85ce0d6eeb34 100644
> --- a/drivers/reset/core.c
> +++ b/drivers/reset/core.c
> @@ -962,6 +962,11 @@ int __reset_control_bulk_get(struct device *dev,
> int num_rstcs,
>                                                      shared, optional, acquired);
>                  if (IS_ERR(rstcs[i].rstc)) {
>                          ret = PTR_ERR(rstcs[i].rstc);
> +
> +                       if (ret != -EPROBE_DEFER)
> +                               dev_err(dev, "Failed to get reset '%s': %d\n",
> +                                       rstcs[i].id, ret);
> +
>                          goto err;
>                  }
>          }

Re: [PATCH v2 1/3] ALSA: hda/tegra: Fix Tegra194 HDA reset failure

[Sameer Pujar]

On 12/21/2021 8:50 PM, Dmitry Osipenko wrote:
> 21.12.2021 09:18, Sameer Pujar пишет:
>>
>> On 12/21/2021 6:51 AM, Dmitry Osipenko wrote:
>>> All stable kernels affected by this problem that don't support the bulk
>>> reset API are EOL now. Please use bulk reset API like I suggested in the
>>> comment to v1, it will allow us to have a cleaner and nicer code.
>> Agree that it would be compact and cleaner, but any specific reset
>> failure in the group won't be obvious in the logs. In this case it
>> failed silently. If compactness is preferred, then may be I can keep an
>> error print at group level so that we see some failure context whenever
>> it happens.
> The group shouldn't fail ever unless device-tree is wrong. Why do you
> think we should care about the case which realistically won't ever
> happen? This is a bit unpractical approach.

Though it is very rare that something like this would happen, but can't 
be ruled out completely.

> If we really care about those error messages, then will be much more
> reasonable to add them to the reset core, like clk core does it [1],
> IMO. This will be a trivial change. Will you be happy with this variant?

It would be nicer to know why exactly it failed. Yes, it makes sense to 
have this in the core. I will send v3 with bulk APIs for HDA driver. 
Thank you.

>
> [1]
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Felixir.bootlin.com%2Flinux%2Fv5.16-rc6%2Fsource%2Fdrivers%2Fclk%2Fclk-bulk.c%23L100&data=04%7C01%7Cspujar%40nvidia.com%7C53e278c9a4804612f74b08d9c49564a0%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637756968218491760%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=QEe8SlSdAN1N8nOu3XqtAdbXP1JbtMBPlswqnBIhq5w%3D&reserved=0
>
> diff --git a/drivers/reset/core.c b/drivers/reset/core.c
> index 61e688882643..85ce0d6eeb34 100644
> --- a/drivers/reset/core.c
> +++ b/drivers/reset/core.c
> @@ -962,6 +962,11 @@ int __reset_control_bulk_get(struct device *dev,
> int num_rstcs,
>                                                      shared, optional, acquired);
>                  if (IS_ERR(rstcs[i].rstc)) {
>                          ret = PTR_ERR(rstcs[i].rstc);
> +
> +                       if (ret != -EPROBE_DEFER)
> +                               dev_err(dev, "Failed to get reset '%s': %d\n",
> +                                       rstcs[i].id, ret);
> +
>                          goto err;
>                  }
>          }