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

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

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

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/c...

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

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

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