[PATCH v2 0/5] Clock and reset improvements for Tegra ALSA drivers
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 of 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.
Changelog:
v2: - Added regcache_sync() to the "ahub: Reset hardware properly" patch, which was missed by accident in v1.
- Corrected typo in the format of the error message in "ahub: Use of_reset_control_array_get_exclusive()" patch by s/%p/%pe/.
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 | 113 +++++++++------------------------ sound/soc/tegra/tegra30_ahub.h | 6 +- 3 files changed, 59 insertions(+), 146 deletions(-)
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 361cf2041911..a25bf7083c28 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;
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 a25bf7083c28..1467725e390d 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";
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..8bd3e83b2c3b 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: %pe\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);
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 8bd3e83b2c3b..37a847569a40 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;
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 | 43 ++++++++++++++++++++-------------- sound/soc/tegra/tegra30_ahub.h | 1 + 2 files changed, 27 insertions(+), 17 deletions(-)
diff --git a/sound/soc/tegra/tegra30_ahub.c b/sound/soc/tegra/tegra30_ahub.c index 37a847569a40..df4ca2b35566 100644 --- a/sound/soc/tegra/tegra30_ahub.c +++ b/sound/soc/tegra/tegra30_ahub.c @@ -65,12 +65,32 @@ 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); + regcache_mark_dirty(ahub->regmap_apbif); + regcache_mark_dirty(ahub->regmap_ahub); + + ret = regcache_sync(ahub->regmap_apbif); + if (ret) + return ret; + + ret = regcache_sync(ahub->regmap_ahub); + if (ret) + return ret;
return 0; } @@ -462,7 +482,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 +494,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: %pe\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 +510,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: %pe\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;
15.01.2021 17:01, Dmitry Osipenko пишет:
@@ -65,12 +65,32 @@ 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);
regcache_mark_dirty(ahub->regmap_apbif);
regcache_mark_dirty(ahub->regmap_ahub);
ret = regcache_sync(ahub->regmap_apbif);
if (ret)
return ret;ret = regcache_sync(ahub->regmap_ahub);
if (ret)
return ret;
The regcache syncing is corrected now in v2, but I missed to disable the clocks in the error path :) I'll make a v3 around next Tuesday. If you'll spot anything else that needs to be improved, please let me know.
participants (1)
-
Dmitry Osipenko