| Message ID | 20210112125834.21545-6-digetx@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Clock and reset improvements for Tegra ALSA drivers | expand |
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.
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
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.
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
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;