[PATCH v1 0/5] Clock and reset improvements for Tegra ALSA drivers

[PATCH v1 0/5] Clock and reset improvements for Tegra ALSA drivers

[Dmitry Osipenko]

This series improves the handling of clock and reset controls of
NVIDA Tegra ALSA drivers. Tegra HDA and AHUB drivers aren't handling
resets properly, which needs to be fixed in order to unblock other patches
related to fixes on the reset controller driver since HDA/AHUB are bound
to fail once reset controller driver will be corrected. In particular ALSA
drivers are relying on implicit de-assertion of resets which is done by the
tegra-clk driver. It's not the business of the clk driver to touch resets
and we need to fix this because it breaks reset/clk programming sequences
of other Tegra drivers.

Dmitry Osipenko (5):
  ALSA: hda/tegra: Use clk_bulk helpers
  ALSA: hda/tegra: Reset hardware
  ASoC: tegra: ahub: Use of_reset_control_array_get_exclusive()
  ASoC: tegra: ahub: Use clk_bulk helpers
  ASoC: tegra: ahub: Reset hardware properly

 sound/pci/hda/hda_tegra.c      |  86 +++++++++------------------
 sound/soc/tegra/tegra30_ahub.c | 103 ++++++---------------------------
 sound/soc/tegra/tegra30_ahub.h |   6 +-
 3 files changed, 49 insertions(+), 146 deletions(-)

-- 
2.29.2

[PATCH v1 1/5] ALSA: hda/tegra: Use clk_bulk helpers

[Dmitry Osipenko]

Use clk_bulk helpers to make code cleaner.

Tested-by: Peter Geis <pgwipeout@gmail.com>
Tested-by: Nicolas Chauvet <kwizart@gmail.com>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 sound/pci/hda/hda_tegra.c | 68 ++++++---------------------------------
 1 file changed, 9 insertions(+), 59 deletions(-)

diff --git a/sound/pci/hda/hda_tegra.c b/sound/pci/hda/hda_tegra.c
index 70164d1428d4..4c799661c2f6 100644
--- a/sound/pci/hda/hda_tegra.c
+++ b/sound/pci/hda/hda_tegra.c
@@ -70,9 +70,8 @@
 struct hda_tegra {
 	struct azx chip;
 	struct device *dev;
-	struct clk *hda_clk;
-	struct clk *hda2codec_2x_clk;
-	struct clk *hda2hdmi_clk;
+	struct clk_bulk_data clocks[3];
+	unsigned int nclocks;
 	void __iomem *regs;
 	struct work_struct probe_work;
 };
@@ -113,36 +112,6 @@ static void hda_tegra_init(struct hda_tegra *hda)
 	writel(v, hda->regs + HDA_IPFS_INTR_MASK);
 }
 
-static int hda_tegra_enable_clocks(struct hda_tegra *data)
-{
-	int rc;
-
-	rc = clk_prepare_enable(data->hda_clk);
-	if (rc)
-		return rc;
-	rc = clk_prepare_enable(data->hda2codec_2x_clk);
-	if (rc)
-		goto disable_hda;
-	rc = clk_prepare_enable(data->hda2hdmi_clk);
-	if (rc)
-		goto disable_codec_2x;
-
-	return 0;
-
-disable_codec_2x:
-	clk_disable_unprepare(data->hda2codec_2x_clk);
-disable_hda:
-	clk_disable_unprepare(data->hda_clk);
-	return rc;
-}
-
-static void hda_tegra_disable_clocks(struct hda_tegra *data)
-{
-	clk_disable_unprepare(data->hda2hdmi_clk);
-	clk_disable_unprepare(data->hda2codec_2x_clk);
-	clk_disable_unprepare(data->hda_clk);
-}
-
 /*
  * power management
  */
@@ -186,7 +155,7 @@ static int __maybe_unused hda_tegra_runtime_suspend(struct device *dev)
 		azx_stop_chip(chip);
 		azx_enter_link_reset(chip);
 	}
-	hda_tegra_disable_clocks(hda);
+	clk_bulk_disable_unprepare(hda->nclocks, hda->clocks);
 
 	return 0;
 }
@@ -198,7 +167,7 @@ static int __maybe_unused hda_tegra_runtime_resume(struct device *dev)
 	struct hda_tegra *hda = container_of(chip, struct hda_tegra, chip);
 	int rc;
 
-	rc = hda_tegra_enable_clocks(hda);
+	rc = clk_bulk_prepare_enable(hda->nclocks, hda->clocks);
 	if (rc != 0)
 		return rc;
 	if (chip && chip->running) {
@@ -268,29 +237,6 @@ static int hda_tegra_init_chip(struct azx *chip, struct platform_device *pdev)
 	return 0;
 }
 
-static int hda_tegra_init_clk(struct hda_tegra *hda)
-{
-	struct device *dev = hda->dev;
-
-	hda->hda_clk = devm_clk_get(dev, "hda");
-	if (IS_ERR(hda->hda_clk)) {
-		dev_err(dev, "failed to get hda clock\n");
-		return PTR_ERR(hda->hda_clk);
-	}
-	hda->hda2codec_2x_clk = devm_clk_get(dev, "hda2codec_2x");
-	if (IS_ERR(hda->hda2codec_2x_clk)) {
-		dev_err(dev, "failed to get hda2codec_2x clock\n");
-		return PTR_ERR(hda->hda2codec_2x_clk);
-	}
-	hda->hda2hdmi_clk = devm_clk_get(dev, "hda2hdmi");
-	if (IS_ERR(hda->hda2hdmi_clk)) {
-		dev_err(dev, "failed to get hda2hdmi clock\n");
-		return PTR_ERR(hda->hda2hdmi_clk);
-	}
-
-	return 0;
-}
-
 static int hda_tegra_first_init(struct azx *chip, struct platform_device *pdev)
 {
 	struct hda_tegra *hda = container_of(chip, struct hda_tegra, chip);
@@ -495,7 +441,11 @@ static int hda_tegra_probe(struct platform_device *pdev)
 		return err;
 	}
 
-	err = hda_tegra_init_clk(hda);
+	hda->clocks[hda->nclocks++].id = "hda";
+	hda->clocks[hda->nclocks++].id = "hda2hdmi";
+	hda->clocks[hda->nclocks++].id = "hda2codec_2x";
+
+	err = devm_clk_bulk_get(&pdev->dev, hda->nclocks, hda->clocks);
 	if (err < 0)
 		goto out_free;
 
-- 
2.29.2

Re: [PATCH v1 1/5] ALSA: hda/tegra: Use clk_bulk helpers

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 4187 bytes --]

On Tue, Jan 12, 2021 at 03:58:30PM +0300, Dmitry Osipenko wrote:
> Use clk_bulk helpers to make code cleaner.
> 
> Tested-by: Peter Geis <pgwipeout@gmail.com>
> Tested-by: Nicolas Chauvet <kwizart@gmail.com>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  sound/pci/hda/hda_tegra.c | 68 ++++++---------------------------------
>  1 file changed, 9 insertions(+), 59 deletions(-)

Heh... I have a branch samewhere with this same patch. Glad I can cross
that off my list. One thing jumped out at me, see below.

> diff --git a/sound/pci/hda/hda_tegra.c b/sound/pci/hda/hda_tegra.c
> index 70164d1428d4..4c799661c2f6 100644
> --- a/sound/pci/hda/hda_tegra.c
> +++ b/sound/pci/hda/hda_tegra.c
> @@ -70,9 +70,8 @@
>  struct hda_tegra {
>  	struct azx chip;
>  	struct device *dev;
> -	struct clk *hda_clk;
> -	struct clk *hda2codec_2x_clk;
> -	struct clk *hda2hdmi_clk;
> +	struct clk_bulk_data clocks[3];
> +	unsigned int nclocks;
>  	void __iomem *regs;
>  	struct work_struct probe_work;
>  };
> @@ -113,36 +112,6 @@ static void hda_tegra_init(struct hda_tegra *hda)
>  	writel(v, hda->regs + HDA_IPFS_INTR_MASK);
>  }
>  
> -static int hda_tegra_enable_clocks(struct hda_tegra *data)
> -{
> -	int rc;
> -
> -	rc = clk_prepare_enable(data->hda_clk);
> -	if (rc)
> -		return rc;
> -	rc = clk_prepare_enable(data->hda2codec_2x_clk);
> -	if (rc)
> -		goto disable_hda;
> -	rc = clk_prepare_enable(data->hda2hdmi_clk);
> -	if (rc)
> -		goto disable_codec_2x;
> -
> -	return 0;
> -
> -disable_codec_2x:
> -	clk_disable_unprepare(data->hda2codec_2x_clk);
> -disable_hda:
> -	clk_disable_unprepare(data->hda_clk);
> -	return rc;
> -}
> -
> -static void hda_tegra_disable_clocks(struct hda_tegra *data)
> -{
> -	clk_disable_unprepare(data->hda2hdmi_clk);
> -	clk_disable_unprepare(data->hda2codec_2x_clk);
> -	clk_disable_unprepare(data->hda_clk);
> -}
> -
>  /*
>   * power management
>   */
> @@ -186,7 +155,7 @@ static int __maybe_unused hda_tegra_runtime_suspend(struct device *dev)
>  		azx_stop_chip(chip);
>  		azx_enter_link_reset(chip);
>  	}
> -	hda_tegra_disable_clocks(hda);
> +	clk_bulk_disable_unprepare(hda->nclocks, hda->clocks);
>  
>  	return 0;
>  }
> @@ -198,7 +167,7 @@ static int __maybe_unused hda_tegra_runtime_resume(struct device *dev)
>  	struct hda_tegra *hda = container_of(chip, struct hda_tegra, chip);
>  	int rc;
>  
> -	rc = hda_tegra_enable_clocks(hda);
> +	rc = clk_bulk_prepare_enable(hda->nclocks, hda->clocks);
>  	if (rc != 0)
>  		return rc;
>  	if (chip && chip->running) {
> @@ -268,29 +237,6 @@ static int hda_tegra_init_chip(struct azx *chip, struct platform_device *pdev)
>  	return 0;
>  }
>  
> -static int hda_tegra_init_clk(struct hda_tegra *hda)
> -{
> -	struct device *dev = hda->dev;
> -
> -	hda->hda_clk = devm_clk_get(dev, "hda");
> -	if (IS_ERR(hda->hda_clk)) {
> -		dev_err(dev, "failed to get hda clock\n");
> -		return PTR_ERR(hda->hda_clk);
> -	}
> -	hda->hda2codec_2x_clk = devm_clk_get(dev, "hda2codec_2x");
> -	if (IS_ERR(hda->hda2codec_2x_clk)) {
> -		dev_err(dev, "failed to get hda2codec_2x clock\n");
> -		return PTR_ERR(hda->hda2codec_2x_clk);
> -	}
> -	hda->hda2hdmi_clk = devm_clk_get(dev, "hda2hdmi");
> -	if (IS_ERR(hda->hda2hdmi_clk)) {
> -		dev_err(dev, "failed to get hda2hdmi clock\n");
> -		return PTR_ERR(hda->hda2hdmi_clk);
> -	}
> -
> -	return 0;
> -}
> -
>  static int hda_tegra_first_init(struct azx *chip, struct platform_device *pdev)
>  {
>  	struct hda_tegra *hda = container_of(chip, struct hda_tegra, chip);
> @@ -495,7 +441,11 @@ static int hda_tegra_probe(struct platform_device *pdev)
>  		return err;
>  	}
>  
> -	err = hda_tegra_init_clk(hda);
> +	hda->clocks[hda->nclocks++].id = "hda";
> +	hda->clocks[hda->nclocks++].id = "hda2hdmi";
> +	hda->clocks[hda->nclocks++].id = "hda2codec_2x";

Originally the code did this in this order: "hda", "hda2codec_2x" and
"hda2hdmi". I don't expect the exact order to be very relevant, but was
there any particular reason to change it?

In either case, this should be fine:

Acked-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 1/5] ALSA: hda/tegra: Use clk_bulk helpers

[Dmitry Osipenko]

15.01.2021 18:22, Thierry Reding пишет:
...
>>  static int hda_tegra_first_init(struct azx *chip, struct platform_device *pdev)
>>  {
>>  	struct hda_tegra *hda = container_of(chip, struct hda_tegra, chip);
>> @@ -495,7 +441,11 @@ static int hda_tegra_probe(struct platform_device *pdev)
>>  		return err;
>>  	}
>>  
>> -	err = hda_tegra_init_clk(hda);
>> +	hda->clocks[hda->nclocks++].id = "hda";
>> +	hda->clocks[hda->nclocks++].id = "hda2hdmi";
>> +	hda->clocks[hda->nclocks++].id = "hda2codec_2x";
> 
> Originally the code did this in this order: "hda", "hda2codec_2x" and
> "hda2hdmi". I don't expect the exact order to be very relevant, but was
> there any particular reason to change it?

The reason was "to make code look nicer". This was a conscious decision
since indeed the clocks order shouldn't matter for this driver.

Re: [PATCH v1 1/5] ALSA: hda/tegra: Use clk_bulk helpers

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1089 bytes --]

On Mon, Jan 18, 2021 at 02:31:35AM +0300, Dmitry Osipenko wrote:
> 15.01.2021 18:22, Thierry Reding пишет:
> ...
> >>  static int hda_tegra_first_init(struct azx *chip, struct platform_device *pdev)
> >>  {
> >>  	struct hda_tegra *hda = container_of(chip, struct hda_tegra, chip);
> >> @@ -495,7 +441,11 @@ static int hda_tegra_probe(struct platform_device *pdev)
> >>  		return err;
> >>  	}
> >>  
> >> -	err = hda_tegra_init_clk(hda);
> >> +	hda->clocks[hda->nclocks++].id = "hda";
> >> +	hda->clocks[hda->nclocks++].id = "hda2hdmi";
> >> +	hda->clocks[hda->nclocks++].id = "hda2codec_2x";
> > 
> > Originally the code did this in this order: "hda", "hda2codec_2x" and
> > "hda2hdmi". I don't expect the exact order to be very relevant, but was
> > there any particular reason to change it?
> 
> The reason was "to make code look nicer". This was a conscious decision
> since indeed the clocks order shouldn't matter for this driver.

Yeah, it's probably fine. In case this ends up causing trouble after all
we can always change the order back.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v1 2/5] ALSA: hda/tegra: Reset hardware

[Dmitry Osipenko]

Reset hardware in order to bring it into a predictable state.

Tested-by: Peter Geis <pgwipeout@gmail.com>
Tested-by: Nicolas Chauvet <kwizart@gmail.com>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 sound/pci/hda/hda_tegra.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/sound/pci/hda/hda_tegra.c b/sound/pci/hda/hda_tegra.c
index 4c799661c2f6..e406b7980f31 100644
--- a/sound/pci/hda/hda_tegra.c
+++ b/sound/pci/hda/hda_tegra.c
@@ -17,6 +17,7 @@
 #include <linux/moduleparam.h>
 #include <linux/mutex.h>
 #include <linux/of_device.h>
+#include <linux/reset.h>
 #include <linux/slab.h>
 #include <linux/time.h>
 #include <linux/string.h>
@@ -70,6 +71,7 @@
 struct hda_tegra {
 	struct azx chip;
 	struct device *dev;
+	struct reset_control *reset;
 	struct clk_bulk_data clocks[3];
 	unsigned int nclocks;
 	void __iomem *regs;
@@ -167,6 +169,12 @@ static int __maybe_unused hda_tegra_runtime_resume(struct device *dev)
 	struct hda_tegra *hda = container_of(chip, struct hda_tegra, chip);
 	int rc;
 
+	if (!(chip && chip->running)) {
+		rc = reset_control_assert(hda->reset);
+		if (rc)
+			return rc;
+	}
+
 	rc = clk_bulk_prepare_enable(hda->nclocks, hda->clocks);
 	if (rc != 0)
 		return rc;
@@ -176,6 +184,10 @@ static int __maybe_unused hda_tegra_runtime_resume(struct device *dev)
 		/* disable controller wake up event*/
 		azx_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) &
 			   ~STATESTS_INT_MASK);
+	} else {
+		rc = reset_control_reset(hda->reset);
+		if (rc)
+			return rc;
 	}
 
 	return 0;
@@ -441,6 +453,12 @@ 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);
+		goto out_free;
+	}
+
 	hda->clocks[hda->nclocks++].id = "hda";
 	hda->clocks[hda->nclocks++].id = "hda2hdmi";
 	hda->clocks[hda->nclocks++].id = "hda2codec_2x";
-- 
2.29.2

Re: [PATCH v1 2/5] ALSA: hda/tegra: Reset hardware

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2627 bytes --]

On Tue, Jan 12, 2021 at 03:58:31PM +0300, Dmitry Osipenko wrote:
> Reset hardware in order to bring it into a predictable state.
> 
> Tested-by: Peter Geis <pgwipeout@gmail.com>
> Tested-by: Nicolas Chauvet <kwizart@gmail.com>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  sound/pci/hda/hda_tegra.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/sound/pci/hda/hda_tegra.c b/sound/pci/hda/hda_tegra.c
> index 4c799661c2f6..e406b7980f31 100644
> --- a/sound/pci/hda/hda_tegra.c
> +++ b/sound/pci/hda/hda_tegra.c
> @@ -17,6 +17,7 @@
>  #include <linux/moduleparam.h>
>  #include <linux/mutex.h>
>  #include <linux/of_device.h>
> +#include <linux/reset.h>
>  #include <linux/slab.h>
>  #include <linux/time.h>
>  #include <linux/string.h>
> @@ -70,6 +71,7 @@
>  struct hda_tegra {
>  	struct azx chip;
>  	struct device *dev;
> +	struct reset_control *reset;
>  	struct clk_bulk_data clocks[3];
>  	unsigned int nclocks;
>  	void __iomem *regs;
> @@ -167,6 +169,12 @@ static int __maybe_unused hda_tegra_runtime_resume(struct device *dev)
>  	struct hda_tegra *hda = container_of(chip, struct hda_tegra, chip);
>  	int rc;
>  
> +	if (!(chip && chip->running)) {

Isn't that check for !chip a bit redundant? If that pointer isn't valid,
we're just going to go crash when dereferencing hda later on, so I think
this can simply be:

	if (!chip->running)

I guess you took this from the inverse check below, but I think we can
also drop it from there, perhaps in a separate patch.

> +		rc = reset_control_assert(hda->reset);
> +		if (rc)
> +			return rc;
> +	}
> +
>  	rc = clk_bulk_prepare_enable(hda->nclocks, hda->clocks);
>  	if (rc != 0)
>  		return rc;
> @@ -176,6 +184,10 @@ static int __maybe_unused hda_tegra_runtime_resume(struct device *dev)
>  		/* disable controller wake up event*/
>  		azx_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) &
>  			   ~STATESTS_INT_MASK);
> +	} else {
> +		rc = reset_control_reset(hda->reset);

The "if (chip)" part definitely doesn't make sense after this anymore
because now if chip == NULL, then we end up in here and dereference an
invalid "hda" pointer.

Also, why reset_control_reset() here? We'll reach this if we ran
reset_control_assert() above, so this should just be
reset_control_deassert() to undo that, right? I suppose it wouldn't hurt
to put throw that standard usleep_range() in there as well that we use
to wait between reset assert and deassert to make sure the clocks have
stabilized and the reset has indeed propagated through the whole IP.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 2/5] ALSA: hda/tegra: Reset hardware

[Dmitry Osipenko]

15.01.2021 18:35, Thierry Reding пишет:
> On Tue, Jan 12, 2021 at 03:58:31PM +0300, Dmitry Osipenko wrote:
>> Reset hardware in order to bring it into a predictable state.
>>
>> Tested-by: Peter Geis <pgwipeout@gmail.com>
>> Tested-by: Nicolas Chauvet <kwizart@gmail.com>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  sound/pci/hda/hda_tegra.c | 18 ++++++++++++++++++
>>  1 file changed, 18 insertions(+)
>>
>> diff --git a/sound/pci/hda/hda_tegra.c b/sound/pci/hda/hda_tegra.c
>> index 4c799661c2f6..e406b7980f31 100644
>> --- a/sound/pci/hda/hda_tegra.c
>> +++ b/sound/pci/hda/hda_tegra.c
>> @@ -17,6 +17,7 @@
>>  #include <linux/moduleparam.h>
>>  #include <linux/mutex.h>
>>  #include <linux/of_device.h>
>> +#include <linux/reset.h>
>>  #include <linux/slab.h>
>>  #include <linux/time.h>
>>  #include <linux/string.h>
>> @@ -70,6 +71,7 @@
>>  struct hda_tegra {
>>  	struct azx chip;
>>  	struct device *dev;
>> +	struct reset_control *reset;
>>  	struct clk_bulk_data clocks[3];
>>  	unsigned int nclocks;
>>  	void __iomem *regs;
>> @@ -167,6 +169,12 @@ static int __maybe_unused hda_tegra_runtime_resume(struct device *dev)
>>  	struct hda_tegra *hda = container_of(chip, struct hda_tegra, chip);
>>  	int rc;
>>  
>> +	if (!(chip && chip->running)) {
> 
> Isn't that check for !chip a bit redundant? If that pointer isn't valid,
> we're just going to go crash when dereferencing hda later on, so I think
> this can simply be:
> 
> 	if (!chip->running)
> 
> I guess you took this from the inverse check below, but I think we can
> also drop it from there, perhaps in a separate patch.
> 
>> +		rc = reset_control_assert(hda->reset);
>> +		if (rc)
>> +			return rc;
>> +	}
>> +
>>  	rc = clk_bulk_prepare_enable(hda->nclocks, hda->clocks);
>>  	if (rc != 0)
>>  		return rc;
>> @@ -176,6 +184,10 @@ static int __maybe_unused hda_tegra_runtime_resume(struct device *dev)
>>  		/* disable controller wake up event*/
>>  		azx_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) &
>>  			   ~STATESTS_INT_MASK);
>> +	} else {
>> +		rc = reset_control_reset(hda->reset);
> 
> The "if (chip)" part definitely doesn't make sense after this anymore
> because now if chip == NULL, then we end up in here and dereference an
> invalid "hda" pointer.

Okay, I took a note for the v3.

> Also, why reset_control_reset() here? We'll reach this if we ran
> reset_control_assert() above, so this should just be
> reset_control_deassert() to undo that, right? I suppose it wouldn't hurt
> to put throw that standard usleep_range() in there as well that we use
> to wait between reset assert and deassert to make sure the clocks have
> stabilized and the reset has indeed propagated through the whole IP.

The reset_control_reset() does the delaying before the deassert, i.e. it
does assert -> udelay(1) -> deassert.

https://elixir.free-electrons.com/linux/v5.11-rc3/source/drivers/clk/tegra/clk.c#L133

The reset_control_reset() usage appears to be a bit more code-tidy
variant in comparison to delaying directly. But I don't mind to use
delay + reset_control_deassert() directly since it may not be obvious to
everyone what reset_control_reset() does.
I'll change it in v3.

Re: [PATCH v1 2/5] ALSA: hda/tegra: Reset hardware

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3838 bytes --]

On Mon, Jan 18, 2021 at 02:39:37AM +0300, Dmitry Osipenko wrote:
> 15.01.2021 18:35, Thierry Reding пишет:
> > On Tue, Jan 12, 2021 at 03:58:31PM +0300, Dmitry Osipenko wrote:
> >> Reset hardware in order to bring it into a predictable state.
> >>
> >> Tested-by: Peter Geis <pgwipeout@gmail.com>
> >> Tested-by: Nicolas Chauvet <kwizart@gmail.com>
> >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> >> ---
> >>  sound/pci/hda/hda_tegra.c | 18 ++++++++++++++++++
> >>  1 file changed, 18 insertions(+)
> >>
> >> diff --git a/sound/pci/hda/hda_tegra.c b/sound/pci/hda/hda_tegra.c
> >> index 4c799661c2f6..e406b7980f31 100644
> >> --- a/sound/pci/hda/hda_tegra.c
> >> +++ b/sound/pci/hda/hda_tegra.c
> >> @@ -17,6 +17,7 @@
> >>  #include <linux/moduleparam.h>
> >>  #include <linux/mutex.h>
> >>  #include <linux/of_device.h>
> >> +#include <linux/reset.h>
> >>  #include <linux/slab.h>
> >>  #include <linux/time.h>
> >>  #include <linux/string.h>
> >> @@ -70,6 +71,7 @@
> >>  struct hda_tegra {
> >>  	struct azx chip;
> >>  	struct device *dev;
> >> +	struct reset_control *reset;
> >>  	struct clk_bulk_data clocks[3];
> >>  	unsigned int nclocks;
> >>  	void __iomem *regs;
> >> @@ -167,6 +169,12 @@ static int __maybe_unused hda_tegra_runtime_resume(struct device *dev)
> >>  	struct hda_tegra *hda = container_of(chip, struct hda_tegra, chip);
> >>  	int rc;
> >>  
> >> +	if (!(chip && chip->running)) {
> > 
> > Isn't that check for !chip a bit redundant? If that pointer isn't valid,
> > we're just going to go crash when dereferencing hda later on, so I think
> > this can simply be:
> > 
> > 	if (!chip->running)
> > 
> > I guess you took this from the inverse check below, but I think we can
> > also drop it from there, perhaps in a separate patch.
> > 
> >> +		rc = reset_control_assert(hda->reset);
> >> +		if (rc)
> >> +			return rc;
> >> +	}
> >> +
> >>  	rc = clk_bulk_prepare_enable(hda->nclocks, hda->clocks);
> >>  	if (rc != 0)
> >>  		return rc;
> >> @@ -176,6 +184,10 @@ static int __maybe_unused hda_tegra_runtime_resume(struct device *dev)
> >>  		/* disable controller wake up event*/
> >>  		azx_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) &
> >>  			   ~STATESTS_INT_MASK);
> >> +	} else {
> >> +		rc = reset_control_reset(hda->reset);
> > 
> > The "if (chip)" part definitely doesn't make sense after this anymore
> > because now if chip == NULL, then we end up in here and dereference an
> > invalid "hda" pointer.
> 
> Okay, I took a note for the v3.
> 
> > Also, why reset_control_reset() here? We'll reach this if we ran
> > reset_control_assert() above, so this should just be
> > reset_control_deassert() to undo that, right? I suppose it wouldn't hurt
> > to put throw that standard usleep_range() in there as well that we use
> > to wait between reset assert and deassert to make sure the clocks have
> > stabilized and the reset has indeed propagated through the whole IP.
> 
> The reset_control_reset() does the delaying before the deassert, i.e. it
> does assert -> udelay(1) -> deassert.
> 
> https://elixir.free-electrons.com/linux/v5.11-rc3/source/drivers/clk/tegra/clk.c#L133
> 
> The reset_control_reset() usage appears to be a bit more code-tidy
> variant in comparison to delaying directly. But I don't mind to use
> delay + reset_control_deassert() directly since it may not be obvious to
> everyone what reset_control_reset() does.
> I'll change it in v3.

Thanks. I know that manually having to add the delay everywhere seems a
bit tedious, but I like the way we very explicitly only ever do reset
assert and deassert, rather than the combined reset pulse, because the
latter can give the impression that the device isn't actually in reset
when we do reset_control_reset().

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v1 3/5] ASoC: tegra: ahub: Use of_reset_control_array_get_exclusive()

[Dmitry Osipenko]

Some of resets are erroneously missed in the configlink_mods[], like APBIF
for example. Use of_reset_control_array_get_exclusive() which requests all
the resets. The problem was hidden by the clk driver which implicitly
de-asserts the missing resets.

Tested-by: Peter Geis <pgwipeout@gmail.com>
Tested-by: Nicolas Chauvet <kwizart@gmail.com>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 sound/soc/tegra/tegra30_ahub.c | 66 +++++-----------------------------
 sound/soc/tegra/tegra30_ahub.h |  1 -
 2 files changed, 9 insertions(+), 58 deletions(-)

diff --git a/sound/soc/tegra/tegra30_ahub.c b/sound/soc/tegra/tegra30_ahub.c
index 156e3b9d613c..1e9767c75b11 100644
--- a/sound/soc/tegra/tegra30_ahub.c
+++ b/sound/soc/tegra/tegra30_ahub.c
@@ -323,41 +323,6 @@ int tegra30_ahub_unset_rx_cif_source(enum tegra30_ahub_rxcif rxcif)
 }
 EXPORT_SYMBOL_GPL(tegra30_ahub_unset_rx_cif_source);
 
-#define MOD_LIST_MASK_TEGRA30	BIT(0)
-#define MOD_LIST_MASK_TEGRA114	BIT(1)
-#define MOD_LIST_MASK_TEGRA124	BIT(2)
-
-#define MOD_LIST_MASK_TEGRA30_OR_LATER \
-		(MOD_LIST_MASK_TEGRA30 | MOD_LIST_MASK_TEGRA114 | \
-			MOD_LIST_MASK_TEGRA124)
-#define MOD_LIST_MASK_TEGRA114_OR_LATER \
-		(MOD_LIST_MASK_TEGRA114 | MOD_LIST_MASK_TEGRA124)
-
-static const struct {
-	const char *rst_name;
-	u32 mod_list_mask;
-} configlink_mods[] = {
-	{ "i2s0", MOD_LIST_MASK_TEGRA30_OR_LATER },
-	{ "i2s1", MOD_LIST_MASK_TEGRA30_OR_LATER },
-	{ "i2s2", MOD_LIST_MASK_TEGRA30_OR_LATER },
-	{ "i2s3", MOD_LIST_MASK_TEGRA30_OR_LATER },
-	{ "i2s4", MOD_LIST_MASK_TEGRA30_OR_LATER },
-	{ "dam0", MOD_LIST_MASK_TEGRA30_OR_LATER },
-	{ "dam1", MOD_LIST_MASK_TEGRA30_OR_LATER },
-	{ "dam2", MOD_LIST_MASK_TEGRA30_OR_LATER },
-	{ "spdif", MOD_LIST_MASK_TEGRA30_OR_LATER },
-	{ "amx", MOD_LIST_MASK_TEGRA114_OR_LATER },
-	{ "adx", MOD_LIST_MASK_TEGRA114_OR_LATER },
-	{ "amx1", MOD_LIST_MASK_TEGRA124 },
-	{ "adx1", MOD_LIST_MASK_TEGRA124 },
-	{ "afc0", MOD_LIST_MASK_TEGRA124 },
-	{ "afc1", MOD_LIST_MASK_TEGRA124 },
-	{ "afc2", MOD_LIST_MASK_TEGRA124 },
-	{ "afc3", MOD_LIST_MASK_TEGRA124 },
-	{ "afc4", MOD_LIST_MASK_TEGRA124 },
-	{ "afc5", MOD_LIST_MASK_TEGRA124 },
-};
-
 #define LAST_REG(name) \
 	(TEGRA30_AHUB_##name + \
 	 (TEGRA30_AHUB_##name##_STRIDE * TEGRA30_AHUB_##name##_COUNT) - 4)
@@ -484,17 +449,14 @@ static const struct regmap_config tegra30_ahub_ahub_regmap_config = {
 };
 
 static struct tegra30_ahub_soc_data soc_data_tegra30 = {
-	.mod_list_mask = MOD_LIST_MASK_TEGRA30,
 	.set_audio_cif = tegra30_ahub_set_cif,
 };
 
 static struct tegra30_ahub_soc_data soc_data_tegra114 = {
-	.mod_list_mask = MOD_LIST_MASK_TEGRA114,
 	.set_audio_cif = tegra30_ahub_set_cif,
 };
 
 static struct tegra30_ahub_soc_data soc_data_tegra124 = {
-	.mod_list_mask = MOD_LIST_MASK_TEGRA124,
 	.set_audio_cif = tegra124_ahub_set_cif,
 };
 
@@ -510,7 +472,6 @@ static int tegra30_ahub_probe(struct platform_device *pdev)
 	const struct of_device_id *match;
 	const struct tegra30_ahub_soc_data *soc_data;
 	struct reset_control *rst;
-	int i;
 	struct resource *res0;
 	void __iomem *regs_apbif, *regs_ahub;
 	int ret = 0;
@@ -528,26 +489,17 @@ static int tegra30_ahub_probe(struct platform_device *pdev)
 	 * operate correctly, all devices on this bus must be out of reset.
 	 * Ensure that here.
 	 */
-	for (i = 0; i < ARRAY_SIZE(configlink_mods); i++) {
-		if (!(configlink_mods[i].mod_list_mask &
-					soc_data->mod_list_mask))
-			continue;
-
-		rst = reset_control_get_exclusive(&pdev->dev,
-						  configlink_mods[i].rst_name);
-		if (IS_ERR(rst)) {
-			dev_err(&pdev->dev, "Can't get reset %s\n",
-				configlink_mods[i].rst_name);
-			ret = PTR_ERR(rst);
-			return ret;
-		}
-
-		ret = reset_control_deassert(rst);
-		reset_control_put(rst);
-		if (ret)
-			return ret;
+	rst = of_reset_control_array_get_exclusive(pdev->dev.of_node);
+	if (IS_ERR(rst)) {
+		dev_err(&pdev->dev, "Can't get reset: %p\n", rst);
+		return PTR_ERR(rst);
 	}
 
+	ret = reset_control_deassert(rst);
+	reset_control_put(rst);
+	if (ret)
+		return ret;
+
 	ahub = devm_kzalloc(&pdev->dev, sizeof(struct tegra30_ahub),
 			    GFP_KERNEL);
 	if (!ahub)
diff --git a/sound/soc/tegra/tegra30_ahub.h b/sound/soc/tegra/tegra30_ahub.h
index 6889c5f23d02..5a2500b4ea06 100644
--- a/sound/soc/tegra/tegra30_ahub.h
+++ b/sound/soc/tegra/tegra30_ahub.h
@@ -491,7 +491,6 @@ void tegra124_ahub_set_cif(struct regmap *regmap, unsigned int reg,
 			   struct tegra30_ahub_cif_conf *conf);
 
 struct tegra30_ahub_soc_data {
-	u32 mod_list_mask;
 	void (*set_audio_cif)(struct regmap *regmap,
 			      unsigned int reg,
 			      struct tegra30_ahub_cif_conf *conf);
-- 
2.29.2

Re: [PATCH v1 3/5] ASoC: tegra: ahub: Use of_reset_control_array_get_exclusive()

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 941 bytes --]

On Tue, Jan 12, 2021 at 03:58:32PM +0300, Dmitry Osipenko wrote:
> Some of resets are erroneously missed in the configlink_mods[], like APBIF
> for example. Use of_reset_control_array_get_exclusive() which requests all
> the resets. The problem was hidden by the clk driver which implicitly
> de-asserts the missing resets.
> 
> Tested-by: Peter Geis <pgwipeout@gmail.com>
> Tested-by: Nicolas Chauvet <kwizart@gmail.com>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  sound/soc/tegra/tegra30_ahub.c | 66 +++++-----------------------------
>  sound/soc/tegra/tegra30_ahub.h |  1 -
>  2 files changed, 9 insertions(+), 58 deletions(-)

Doing it this way is slightly suboptimal because now we don't actually
have a way of checking that the DT has all the necessary resets listed.

Can we not just make the list complete instead to keep the checks in
place? That should be a much smaller patch, too.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 3/5] ASoC: tegra: ahub: Use of_reset_control_array_get_exclusive()

[Dmitry Osipenko]

15.01.2021 18:37, Thierry Reding пишет:
> On Tue, Jan 12, 2021 at 03:58:32PM +0300, Dmitry Osipenko wrote:
>> Some of resets are erroneously missed in the configlink_mods[], like APBIF
>> for example. Use of_reset_control_array_get_exclusive() which requests all
>> the resets. The problem was hidden by the clk driver which implicitly
>> de-asserts the missing resets.
>>
>> Tested-by: Peter Geis <pgwipeout@gmail.com>
>> Tested-by: Nicolas Chauvet <kwizart@gmail.com>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  sound/soc/tegra/tegra30_ahub.c | 66 +++++-----------------------------
>>  sound/soc/tegra/tegra30_ahub.h |  1 -
>>  2 files changed, 9 insertions(+), 58 deletions(-)
> 
> Doing it this way is slightly suboptimal because now we don't actually
> have a way of checking that the DT has all the necessary resets listed.
> 
> Can we not just make the list complete instead to keep the checks in
> place? That should be a much smaller patch, too.

I'll change it in v3.

[PATCH v1 4/5] ASoC: tegra: ahub: Use clk_bulk helpers

[Dmitry Osipenko]

Use clk_bulk helpers to make code cleaner.

Tested-by: Peter Geis <pgwipeout@gmail.com>
Tested-by: Nicolas Chauvet <kwizart@gmail.com>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 sound/soc/tegra/tegra30_ahub.c | 30 +++++++-----------------------
 sound/soc/tegra/tegra30_ahub.h |  4 ++--
 2 files changed, 9 insertions(+), 25 deletions(-)

diff --git a/sound/soc/tegra/tegra30_ahub.c b/sound/soc/tegra/tegra30_ahub.c
index 1e9767c75b11..4dbb58f7ea36 100644
--- a/sound/soc/tegra/tegra30_ahub.c
+++ b/sound/soc/tegra/tegra30_ahub.c
@@ -45,8 +45,7 @@ static int tegra30_ahub_runtime_suspend(struct device *dev)
 	regcache_cache_only(ahub->regmap_apbif, true);
 	regcache_cache_only(ahub->regmap_ahub, true);
 
-	clk_disable_unprepare(ahub->clk_apbif);
-	clk_disable_unprepare(ahub->clk_d_audio);
+	clk_bulk_disable_unprepare(ahub->nclocks, ahub->clocks);
 
 	return 0;
 }
@@ -66,17 +65,9 @@ static int tegra30_ahub_runtime_resume(struct device *dev)
 {
 	int ret;
 
-	ret = clk_prepare_enable(ahub->clk_d_audio);
-	if (ret) {
-		dev_err(dev, "clk_enable d_audio failed: %d\n", ret);
-		return ret;
-	}
-	ret = clk_prepare_enable(ahub->clk_apbif);
-	if (ret) {
-		dev_err(dev, "clk_enable apbif failed: %d\n", ret);
-		clk_disable(ahub->clk_d_audio);
+	ret = clk_bulk_prepare_enable(ahub->nclocks, ahub->clocks);
+	if (ret)
 		return ret;
-	}
 
 	regcache_cache_only(ahub->regmap_apbif, false);
 	regcache_cache_only(ahub->regmap_ahub, false);
@@ -509,19 +500,12 @@ static int tegra30_ahub_probe(struct platform_device *pdev)
 	ahub->soc_data = soc_data;
 	ahub->dev = &pdev->dev;
 
-	ahub->clk_d_audio = devm_clk_get(&pdev->dev, "d_audio");
-	if (IS_ERR(ahub->clk_d_audio)) {
-		dev_err(&pdev->dev, "Can't retrieve ahub d_audio clock\n");
-		ret = PTR_ERR(ahub->clk_d_audio);
-		return ret;
-	}
+	ahub->clocks[ahub->nclocks++].id = "apbif";
+	ahub->clocks[ahub->nclocks++].id = "d_audio";
 
-	ahub->clk_apbif = devm_clk_get(&pdev->dev, "apbif");
-	if (IS_ERR(ahub->clk_apbif)) {
-		dev_err(&pdev->dev, "Can't retrieve ahub apbif clock\n");
-		ret = PTR_ERR(ahub->clk_apbif);
+	ret = devm_clk_bulk_get(&pdev->dev, ahub->nclocks, ahub->clocks);
+	if (ret)
 		return ret;
-	}
 
 	res0 = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	regs_apbif = devm_ioremap_resource(&pdev->dev, res0);
diff --git a/sound/soc/tegra/tegra30_ahub.h b/sound/soc/tegra/tegra30_ahub.h
index 5a2500b4ea06..063aed5037d7 100644
--- a/sound/soc/tegra/tegra30_ahub.h
+++ b/sound/soc/tegra/tegra30_ahub.h
@@ -510,8 +510,8 @@ struct tegra30_ahub_soc_data {
 struct tegra30_ahub {
 	const struct tegra30_ahub_soc_data *soc_data;
 	struct device *dev;
-	struct clk *clk_d_audio;
-	struct clk *clk_apbif;
+	struct clk_bulk_data clocks[2];
+	unsigned int nclocks;
 	resource_size_t apbif_addr;
 	struct regmap *regmap_apbif;
 	struct regmap *regmap_ahub;
-- 
2.29.2

Re: [PATCH v1 4/5] ASoC: tegra: ahub: Use clk_bulk helpers

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 492 bytes --]

On Tue, Jan 12, 2021 at 03:58:33PM +0300, Dmitry Osipenko wrote:
> Use clk_bulk helpers to make code cleaner.
> 
> Tested-by: Peter Geis <pgwipeout@gmail.com>
> Tested-by: Nicolas Chauvet <kwizart@gmail.com>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  sound/soc/tegra/tegra30_ahub.c | 30 +++++++-----------------------
>  sound/soc/tegra/tegra30_ahub.h |  4 ++--
>  2 files changed, 9 insertions(+), 25 deletions(-)

Acked-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v1 5/5] ASoC: tegra: ahub: Reset hardware properly

[Dmitry Osipenko]

Assert hardware reset before clocks are enabled and then de-assert it
after clocks are enabled. This brings hardware into a predictable state
and removes relying on implicit de-assertion of resets which is done by
the clk driver.

Tested-by: Peter Geis <pgwipeout@gmail.com>
Tested-by: Nicolas Chauvet <kwizart@gmail.com>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 sound/soc/tegra/tegra30_ahub.c | 33 ++++++++++++++++-----------------
 sound/soc/tegra/tegra30_ahub.h |  1 +
 2 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/sound/soc/tegra/tegra30_ahub.c b/sound/soc/tegra/tegra30_ahub.c
index 4dbb58f7ea36..246cf6a373a1 100644
--- a/sound/soc/tegra/tegra30_ahub.c
+++ b/sound/soc/tegra/tegra30_ahub.c
@@ -65,10 +65,20 @@ static int tegra30_ahub_runtime_resume(struct device *dev)
 {
 	int ret;
 
+	ret = reset_control_assert(ahub->reset);
+	if (ret)
+		return ret;
+
 	ret = clk_bulk_prepare_enable(ahub->nclocks, ahub->clocks);
 	if (ret)
 		return ret;
 
+	ret = reset_control_reset(ahub->reset);
+	if (ret) {
+		clk_bulk_disable_unprepare(ahub->nclocks, ahub->clocks);
+		return ret;
+	}
+
 	regcache_cache_only(ahub->regmap_apbif, false);
 	regcache_cache_only(ahub->regmap_ahub, false);
 
@@ -462,7 +472,6 @@ static int tegra30_ahub_probe(struct platform_device *pdev)
 {
 	const struct of_device_id *match;
 	const struct tegra30_ahub_soc_data *soc_data;
-	struct reset_control *rst;
 	struct resource *res0;
 	void __iomem *regs_apbif, *regs_ahub;
 	int ret = 0;
@@ -475,22 +484,6 @@ static int tegra30_ahub_probe(struct platform_device *pdev)
 		return -EINVAL;
 	soc_data = match->data;
 
-	/*
-	 * The AHUB hosts a register bus: the "configlink". For this to
-	 * operate correctly, all devices on this bus must be out of reset.
-	 * Ensure that here.
-	 */
-	rst = of_reset_control_array_get_exclusive(pdev->dev.of_node);
-	if (IS_ERR(rst)) {
-		dev_err(&pdev->dev, "Can't get reset: %p\n", rst);
-		return PTR_ERR(rst);
-	}
-
-	ret = reset_control_deassert(rst);
-	reset_control_put(rst);
-	if (ret)
-		return ret;
-
 	ahub = devm_kzalloc(&pdev->dev, sizeof(struct tegra30_ahub),
 			    GFP_KERNEL);
 	if (!ahub)
@@ -507,6 +500,12 @@ static int tegra30_ahub_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	ahub->reset = devm_reset_control_array_get_exclusive(&pdev->dev);
+	if (IS_ERR(ahub->reset)) {
+		dev_err(&pdev->dev, "Can't get reset: %p\n", ahub->reset);
+		return PTR_ERR(ahub->reset);
+	}
+
 	res0 = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	regs_apbif = devm_ioremap_resource(&pdev->dev, res0);
 	if (IS_ERR(regs_apbif))
diff --git a/sound/soc/tegra/tegra30_ahub.h b/sound/soc/tegra/tegra30_ahub.h
index 063aed5037d7..ceb056575e98 100644
--- a/sound/soc/tegra/tegra30_ahub.h
+++ b/sound/soc/tegra/tegra30_ahub.h
@@ -510,6 +510,7 @@ struct tegra30_ahub_soc_data {
 struct tegra30_ahub {
 	const struct tegra30_ahub_soc_data *soc_data;
 	struct device *dev;
+	struct reset_control *reset;
 	struct clk_bulk_data clocks[2];
 	unsigned int nclocks;
 	resource_size_t apbif_addr;
-- 
2.29.2

Re: [PATCH v1 5/5] ASoC: tegra: ahub: Reset hardware properly

[Dmitry Osipenko]

12.01.2021 15:58, Dmitry Osipenko пишет:
> Assert hardware reset before clocks are enabled and then de-assert it
> after clocks are enabled. This brings hardware into a predictable state
> and removes relying on implicit de-assertion of resets which is done by
> the clk driver.
> 
> Tested-by: Peter Geis <pgwipeout@gmail.com>
> Tested-by: Nicolas Chauvet <kwizart@gmail.com>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  sound/soc/tegra/tegra30_ahub.c | 33 ++++++++++++++++-----------------
>  sound/soc/tegra/tegra30_ahub.h |  1 +
>  2 files changed, 17 insertions(+), 17 deletions(-)
> 
> diff --git a/sound/soc/tegra/tegra30_ahub.c b/sound/soc/tegra/tegra30_ahub.c
> index 4dbb58f7ea36..246cf6a373a1 100644
> --- a/sound/soc/tegra/tegra30_ahub.c
> +++ b/sound/soc/tegra/tegra30_ahub.c
> @@ -65,10 +65,20 @@ static int tegra30_ahub_runtime_resume(struct device *dev)
>  {
>  	int ret;
>  
> +	ret = reset_control_assert(ahub->reset);
> +	if (ret)
> +		return ret;
> +
>  	ret = clk_bulk_prepare_enable(ahub->nclocks, ahub->clocks);
>  	if (ret)
>  		return ret;
>  
> +	ret = reset_control_reset(ahub->reset);
> +	if (ret) {
> +		clk_bulk_disable_unprepare(ahub->nclocks, ahub->clocks);
> +		return ret;
> +	}
> +
>  	regcache_cache_only(ahub->regmap_apbif, false);
>  	regcache_cache_only(ahub->regmap_ahub, false);

I just realized that this is incorrect version of the patch which misses
the regcache syncing after the h/w reset. I'll make a v2.

Re: [PATCH v1 5/5] ASoC: tegra: ahub: Reset hardware properly

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3173 bytes --]

On Tue, Jan 12, 2021 at 03:58:34PM +0300, Dmitry Osipenko wrote:
> Assert hardware reset before clocks are enabled and then de-assert it
> after clocks are enabled. This brings hardware into a predictable state
> and removes relying on implicit de-assertion of resets which is done by
> the clk driver.
> 
> Tested-by: Peter Geis <pgwipeout@gmail.com>
> Tested-by: Nicolas Chauvet <kwizart@gmail.com>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  sound/soc/tegra/tegra30_ahub.c | 33 ++++++++++++++++-----------------
>  sound/soc/tegra/tegra30_ahub.h |  1 +
>  2 files changed, 17 insertions(+), 17 deletions(-)
> 
> diff --git a/sound/soc/tegra/tegra30_ahub.c b/sound/soc/tegra/tegra30_ahub.c
> index 4dbb58f7ea36..246cf6a373a1 100644
> --- a/sound/soc/tegra/tegra30_ahub.c
> +++ b/sound/soc/tegra/tegra30_ahub.c
> @@ -65,10 +65,20 @@ static int tegra30_ahub_runtime_resume(struct device *dev)
>  {
>  	int ret;
>  
> +	ret = reset_control_assert(ahub->reset);
> +	if (ret)
> +		return ret;
> +
>  	ret = clk_bulk_prepare_enable(ahub->nclocks, ahub->clocks);
>  	if (ret)
>  		return ret;
>  
> +	ret = reset_control_reset(ahub->reset);
> +	if (ret) {
> +		clk_bulk_disable_unprepare(ahub->nclocks, ahub->clocks);
> +		return ret;
> +	}
> +
>  	regcache_cache_only(ahub->regmap_apbif, false);
>  	regcache_cache_only(ahub->regmap_ahub, false);
>  
> @@ -462,7 +472,6 @@ static int tegra30_ahub_probe(struct platform_device *pdev)
>  {
>  	const struct of_device_id *match;
>  	const struct tegra30_ahub_soc_data *soc_data;
> -	struct reset_control *rst;
>  	struct resource *res0;
>  	void __iomem *regs_apbif, *regs_ahub;
>  	int ret = 0;
> @@ -475,22 +484,6 @@ static int tegra30_ahub_probe(struct platform_device *pdev)
>  		return -EINVAL;
>  	soc_data = match->data;
>  
> -	/*
> -	 * The AHUB hosts a register bus: the "configlink". For this to
> -	 * operate correctly, all devices on this bus must be out of reset.
> -	 * Ensure that here.
> -	 */
> -	rst = of_reset_control_array_get_exclusive(pdev->dev.of_node);
> -	if (IS_ERR(rst)) {
> -		dev_err(&pdev->dev, "Can't get reset: %p\n", rst);
> -		return PTR_ERR(rst);
> -	}
> -
> -	ret = reset_control_deassert(rst);
> -	reset_control_put(rst);
> -	if (ret)
> -		return ret;
> -
>  	ahub = devm_kzalloc(&pdev->dev, sizeof(struct tegra30_ahub),
>  			    GFP_KERNEL);
>  	if (!ahub)
> @@ -507,6 +500,12 @@ static int tegra30_ahub_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> +	ahub->reset = devm_reset_control_array_get_exclusive(&pdev->dev);
> +	if (IS_ERR(ahub->reset)) {
> +		dev_err(&pdev->dev, "Can't get reset: %p\n", ahub->reset);

I didn't notice that the prior patch already introduced this, but I'd
prefer for this to either be %pe so that the symbolic error name is
printed, or %ld with PTR_ERR(ahub->reset) to format this in a more
standard way that can be more easily grepped for and parsed.

It also seems like the prior patch that converts this to use
of_reset_control_array_get_exclusive() is a bit pointless now. Why not
just move to this directly instead?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 5/5] ASoC: tegra: ahub: Reset hardware properly

[Dmitry Osipenko]

15.01.2021 18:44, Thierry Reding пишет:
> On Tue, Jan 12, 2021 at 03:58:34PM +0300, Dmitry Osipenko wrote:
>> Assert hardware reset before clocks are enabled and then de-assert it
>> after clocks are enabled. This brings hardware into a predictable state
>> and removes relying on implicit de-assertion of resets which is done by
>> the clk driver.
>>
>> Tested-by: Peter Geis <pgwipeout@gmail.com>
>> Tested-by: Nicolas Chauvet <kwizart@gmail.com>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  sound/soc/tegra/tegra30_ahub.c | 33 ++++++++++++++++-----------------
>>  sound/soc/tegra/tegra30_ahub.h |  1 +
>>  2 files changed, 17 insertions(+), 17 deletions(-)
>>
>> diff --git a/sound/soc/tegra/tegra30_ahub.c b/sound/soc/tegra/tegra30_ahub.c
>> index 4dbb58f7ea36..246cf6a373a1 100644
>> --- a/sound/soc/tegra/tegra30_ahub.c
>> +++ b/sound/soc/tegra/tegra30_ahub.c
>> @@ -65,10 +65,20 @@ static int tegra30_ahub_runtime_resume(struct device *dev)
>>  {
>>  	int ret;
>>  
>> +	ret = reset_control_assert(ahub->reset);
>> +	if (ret)
>> +		return ret;
>> +
>>  	ret = clk_bulk_prepare_enable(ahub->nclocks, ahub->clocks);
>>  	if (ret)
>>  		return ret;
>>  
>> +	ret = reset_control_reset(ahub->reset);
>> +	if (ret) {
>> +		clk_bulk_disable_unprepare(ahub->nclocks, ahub->clocks);
>> +		return ret;
>> +	}
>> +
>>  	regcache_cache_only(ahub->regmap_apbif, false);
>>  	regcache_cache_only(ahub->regmap_ahub, false);
>>  
>> @@ -462,7 +472,6 @@ static int tegra30_ahub_probe(struct platform_device *pdev)
>>  {
>>  	const struct of_device_id *match;
>>  	const struct tegra30_ahub_soc_data *soc_data;
>> -	struct reset_control *rst;
>>  	struct resource *res0;
>>  	void __iomem *regs_apbif, *regs_ahub;
>>  	int ret = 0;
>> @@ -475,22 +484,6 @@ static int tegra30_ahub_probe(struct platform_device *pdev)
>>  		return -EINVAL;
>>  	soc_data = match->data;
>>  
>> -	/*
>> -	 * The AHUB hosts a register bus: the "configlink". For this to
>> -	 * operate correctly, all devices on this bus must be out of reset.
>> -	 * Ensure that here.
>> -	 */
>> -	rst = of_reset_control_array_get_exclusive(pdev->dev.of_node);
>> -	if (IS_ERR(rst)) {
>> -		dev_err(&pdev->dev, "Can't get reset: %p\n", rst);
>> -		return PTR_ERR(rst);
>> -	}
>> -
>> -	ret = reset_control_deassert(rst);
>> -	reset_control_put(rst);
>> -	if (ret)
>> -		return ret;
>> -
>>  	ahub = devm_kzalloc(&pdev->dev, sizeof(struct tegra30_ahub),
>>  			    GFP_KERNEL);
>>  	if (!ahub)
>> @@ -507,6 +500,12 @@ static int tegra30_ahub_probe(struct platform_device *pdev)
>>  	if (ret)
>>  		return ret;
>>  
>> +	ahub->reset = devm_reset_control_array_get_exclusive(&pdev->dev);
>> +	if (IS_ERR(ahub->reset)) {
>> +		dev_err(&pdev->dev, "Can't get reset: %p\n", ahub->reset);
> 
> I didn't notice that the prior patch already introduced this, but I'd
> prefer for this to either be %pe so that the symbolic error name is
> printed, or %ld with PTR_ERR(ahub->reset) to format this in a more
> standard way that can be more easily grepped for and parsed.

This is already fixed in v2. Good catch anyways, thanks.

> It also seems like the prior patch that converts this to use
> of_reset_control_array_get_exclusive() is a bit pointless now. Why not
> just move to this directly instead?

These are two independent changes. The previous patch fixed the missing
resets, this patch changes the hardware initialization logic.

Re: [PATCH v1 5/5] ASoC: tegra: ahub: Reset hardware properly

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 4044 bytes --]

On Mon, Jan 18, 2021 at 03:02:38AM +0300, Dmitry Osipenko wrote:
> 15.01.2021 18:44, Thierry Reding пишет:
> > On Tue, Jan 12, 2021 at 03:58:34PM +0300, Dmitry Osipenko wrote:
> >> Assert hardware reset before clocks are enabled and then de-assert it
> >> after clocks are enabled. This brings hardware into a predictable state
> >> and removes relying on implicit de-assertion of resets which is done by
> >> the clk driver.
> >>
> >> Tested-by: Peter Geis <pgwipeout@gmail.com>
> >> Tested-by: Nicolas Chauvet <kwizart@gmail.com>
> >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> >> ---
> >>  sound/soc/tegra/tegra30_ahub.c | 33 ++++++++++++++++-----------------
> >>  sound/soc/tegra/tegra30_ahub.h |  1 +
> >>  2 files changed, 17 insertions(+), 17 deletions(-)
> >>
> >> diff --git a/sound/soc/tegra/tegra30_ahub.c b/sound/soc/tegra/tegra30_ahub.c
> >> index 4dbb58f7ea36..246cf6a373a1 100644
> >> --- a/sound/soc/tegra/tegra30_ahub.c
> >> +++ b/sound/soc/tegra/tegra30_ahub.c
> >> @@ -65,10 +65,20 @@ static int tegra30_ahub_runtime_resume(struct device *dev)
> >>  {
> >>  	int ret;
> >>  
> >> +	ret = reset_control_assert(ahub->reset);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >>  	ret = clk_bulk_prepare_enable(ahub->nclocks, ahub->clocks);
> >>  	if (ret)
> >>  		return ret;
> >>  
> >> +	ret = reset_control_reset(ahub->reset);
> >> +	if (ret) {
> >> +		clk_bulk_disable_unprepare(ahub->nclocks, ahub->clocks);
> >> +		return ret;
> >> +	}
> >> +
> >>  	regcache_cache_only(ahub->regmap_apbif, false);
> >>  	regcache_cache_only(ahub->regmap_ahub, false);
> >>  
> >> @@ -462,7 +472,6 @@ static int tegra30_ahub_probe(struct platform_device *pdev)
> >>  {
> >>  	const struct of_device_id *match;
> >>  	const struct tegra30_ahub_soc_data *soc_data;
> >> -	struct reset_control *rst;
> >>  	struct resource *res0;
> >>  	void __iomem *regs_apbif, *regs_ahub;
> >>  	int ret = 0;
> >> @@ -475,22 +484,6 @@ static int tegra30_ahub_probe(struct platform_device *pdev)
> >>  		return -EINVAL;
> >>  	soc_data = match->data;
> >>  
> >> -	/*
> >> -	 * The AHUB hosts a register bus: the "configlink". For this to
> >> -	 * operate correctly, all devices on this bus must be out of reset.
> >> -	 * Ensure that here.
> >> -	 */
> >> -	rst = of_reset_control_array_get_exclusive(pdev->dev.of_node);
> >> -	if (IS_ERR(rst)) {
> >> -		dev_err(&pdev->dev, "Can't get reset: %p\n", rst);
> >> -		return PTR_ERR(rst);
> >> -	}
> >> -
> >> -	ret = reset_control_deassert(rst);
> >> -	reset_control_put(rst);
> >> -	if (ret)
> >> -		return ret;
> >> -
> >>  	ahub = devm_kzalloc(&pdev->dev, sizeof(struct tegra30_ahub),
> >>  			    GFP_KERNEL);
> >>  	if (!ahub)
> >> @@ -507,6 +500,12 @@ static int tegra30_ahub_probe(struct platform_device *pdev)
> >>  	if (ret)
> >>  		return ret;
> >>  
> >> +	ahub->reset = devm_reset_control_array_get_exclusive(&pdev->dev);
> >> +	if (IS_ERR(ahub->reset)) {
> >> +		dev_err(&pdev->dev, "Can't get reset: %p\n", ahub->reset);
> > 
> > I didn't notice that the prior patch already introduced this, but I'd
> > prefer for this to either be %pe so that the symbolic error name is
> > printed, or %ld with PTR_ERR(ahub->reset) to format this in a more
> > standard way that can be more easily grepped for and parsed.
> 
> This is already fixed in v2. Good catch anyways, thanks.
> 
> > It also seems like the prior patch that converts this to use
> > of_reset_control_array_get_exclusive() is a bit pointless now. Why not
> > just move to this directly instead?
> 
> These are two independent changes. The previous patch fixed the missing
> resets, this patch changes the hardware initialization logic.

But moving to devm_reset_control_array_get_exclusive() isn't really part
of the hardware initialization logic change, right? So it's not strictly
related to the rest of this patch.

Anyway, I don't feel strongly about it being part of this patch, so feel
free to keep it here.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 0/5] Clock and reset improvements for Tegra ALSA drivers

[Takashi Iwai]

On Tue, 12 Jan 2021 13:58:29 +0100,
Dmitry Osipenko wrote:
> 
> This series improves the handling of clock and reset controls of
> NVIDA Tegra ALSA drivers. Tegra HDA and AHUB drivers aren't handling
> resets properly, which needs to be fixed in order to unblock other patches
> related to fixes on the reset controller driver since HDA/AHUB are bound
> to fail once reset controller driver will be corrected. In particular ALSA
> drivers are relying on implicit de-assertion of resets which is done by the
> tegra-clk driver. It's not the business of the clk driver to touch resets
> and we need to fix this because it breaks reset/clk programming sequences
> of other Tegra drivers.
> 
> Dmitry Osipenko (5):
>   ALSA: hda/tegra: Use clk_bulk helpers
>   ALSA: hda/tegra: Reset hardware
>   ASoC: tegra: ahub: Use of_reset_control_array_get_exclusive()
>   ASoC: tegra: ahub: Use clk_bulk helpers
>   ASoC: tegra: ahub: Reset hardware properly

Thierry, Jonathan, Sameer, could you guys check those please?


thanks,

Takashi

> 
>  sound/pci/hda/hda_tegra.c      |  86 +++++++++------------------
>  sound/soc/tegra/tegra30_ahub.c | 103 ++++++---------------------------
>  sound/soc/tegra/tegra30_ahub.h |   6 +-
>  3 files changed, 49 insertions(+), 146 deletions(-)
> 
> -- 
> 2.29.2
> 

Re: [PATCH v1 0/5] Clock and reset improvements for Tegra ALSA drivers

[Ben Dooks]

On 12/01/2021 12:58, Dmitry Osipenko wrote:
> This series improves the handling of clock and reset controls of
> NVIDA Tegra ALSA drivers. Tegra HDA and AHUB drivers aren't handling
> resets properly, which needs to be fixed in order to unblock other patches
> related to fixes on the reset controller driver since HDA/AHUB are bound
> to fail once reset controller driver will be corrected. In particular ALSA
> drivers are relying on implicit de-assertion of resets which is done by the
> tegra-clk driver. It's not the business of the clk driver to touch resets
> and we need to fix this because it breaks reset/clk programming sequences
> of other Tegra drivers.
> 
> Dmitry Osipenko (5):
>    ALSA: hda/tegra: Use clk_bulk helpers
>    ALSA: hda/tegra: Reset hardware
>    ASoC: tegra: ahub: Use of_reset_control_array_get_exclusive()
>    ASoC: tegra: ahub: Use clk_bulk helpers
>    ASoC: tegra: ahub: Reset hardware properly
> 
>   sound/pci/hda/hda_tegra.c      |  86 +++++++++------------------
>   sound/soc/tegra/tegra30_ahub.c | 103 ++++++---------------------------
>   sound/soc/tegra/tegra30_ahub.h |   6 +-
>   3 files changed, 49 insertions(+), 146 deletions(-)

I wonder if this will help with the issues we saw when the tegra is
the i2s clock slave.


-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

https://www.codethink.co.uk/privacy.html

Re: [PATCH v1 0/5] Clock and reset improvements for Tegra ALSA drivers

[Dmitry Osipenko]

15.01.2021 13:52, Ben Dooks пишет:
> On 12/01/2021 12:58, Dmitry Osipenko wrote:
>> This series improves the handling of clock and reset controls of
>> NVIDA Tegra ALSA drivers. Tegra HDA and AHUB drivers aren't handling
>> resets properly, which needs to be fixed in order to unblock other
>> patches
>> related to fixes on the reset controller driver since HDA/AHUB are bound
>> to fail once reset controller driver will be corrected. In particular
>> ALSA
>> drivers are relying on implicit de-assertion of resets which is done
>> by the
>> tegra-clk driver. It's not the business of the clk driver to touch resets
>> and we need to fix this because it breaks reset/clk programming sequences
>> of other Tegra drivers.
>>
>> Dmitry Osipenko (5):
>>    ALSA: hda/tegra: Use clk_bulk helpers
>>    ALSA: hda/tegra: Reset hardware
>>    ASoC: tegra: ahub: Use of_reset_control_array_get_exclusive()
>>    ASoC: tegra: ahub: Use clk_bulk helpers
>>    ASoC: tegra: ahub: Reset hardware properly
>>
>>   sound/pci/hda/hda_tegra.c      |  86 +++++++++------------------
>>   sound/soc/tegra/tegra30_ahub.c | 103 ++++++---------------------------
>>   sound/soc/tegra/tegra30_ahub.h |   6 +-
>>   3 files changed, 49 insertions(+), 146 deletions(-)
> 
> I wonder if this will help with the issues we saw when the tegra is
> the i2s clock slave.

Probably no, this series shouldn't fix any of the current problems. I
will be surprised if it does.