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

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