| Message ID | 20210112125834.21545-3-digetx@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Clock and reset improvements for Tegra ALSA drivers | expand |
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
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/clk.c#L133 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.
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/clk.c#L133 > > 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
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)) { + 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";