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

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