| Message ID | 1576880825-15010-13-git-send-email-skomatineni@nvidia.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Move PMC clocks into Tegra PMC driver | expand |
21.12.2019 01:26, Sowjanya Komatineni пишет: > Tegra PMC clock clk_out_1 is dedicated for audio mclk from Tegra30 > through Tegra210 and currently Tegra clock driver does initial parent > configuration for audio mclk "clk_out_1" and enables them by default. > > With the move of Tera PMC clocks from clock driver to Tegra PMC > driver, initial parent configuration for audio clocks are through > the device tree using assigned-clock-parents property. > > Default clock parents can be specified in device tree using > assigned-clocks and assigned-clock-parents and there is no need > to have clock driver do parent configuration and enable audio related > clocks. > > This patch has implementation for initial parent configuration in > audio driver when default parent configuration is not specified in the > device tree using assigned-clock properties and enables audio clocks > during the clock rate change. > > This patch configures PLLA_OUT0 as parent to extern1 and extern1 > as parent to clk_out_1 and uses clk_out_1 as cdev1 clock to allow > mclk control from this driver. > > Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com> > --- > sound/soc/tegra/tegra_asoc_utils.c | 71 ++++++++++++++++++++++---------------- > 1 file changed, 41 insertions(+), 30 deletions(-) > > diff --git a/sound/soc/tegra/tegra_asoc_utils.c b/sound/soc/tegra/tegra_asoc_utils.c > index 38535962029c..fc3135c08f43 100644 > --- a/sound/soc/tegra/tegra_asoc_utils.c > +++ b/sound/soc/tegra/tegra_asoc_utils.c > @@ -7,6 +7,7 @@ > */ > > #include <linux/clk.h> > +#include <linux/clk-provider.h> This is illegal, it is not a clock provider. > #include <linux/device.h> > #include <linux/err.h> > #include <linux/kernel.h> > @@ -59,9 +60,8 @@ int tegra_asoc_utils_set_rate(struct tegra_asoc_utils_data *data, int srate, > data->set_baseclock = 0; > data->set_mclk = 0; > > - clk_disable_unprepare(data->clk_cdev1); > - clk_disable_unprepare(data->clk_pll_a_out0); > - clk_disable_unprepare(data->clk_pll_a); > + if (__clk_is_enabled(data->clk_cdev1)) > + clk_disable_unprepare(data->clk_cdev1); The root of the problem is that you removed clocks enabling from tegra_asoc_utils_init(). I'm not sure why clocks should be disabled during the rate-changing, probably this action is not really needed. diff --git a/sound/soc/tegra/tegra_asoc_utils.c b/sound/soc/tegra/tegra_asoc_utils.c index 46ff70c16b74..789fd03e51a7 100644 --- a/sound/soc/tegra/tegra_asoc_utils.c +++ b/sound/soc/tegra/tegra_asoc_utils.c @@ -7,7 +7,6 @@ */ #include <linux/clk.h> -#include <linux/clk-provider.h> #include <linux/device.h> #include <linux/err.h> #include <linux/kernel.h> @@ -60,9 +59,6 @@ int tegra_asoc_utils_set_rate(struct tegra_asoc_utils_data *data, int srate, data->set_baseclock = 0; data->set_mclk = 0; - if (__clk_is_enabled(data->clk_cdev1)) - clk_disable_unprepare(data->clk_cdev1); - err = clk_set_rate(data->clk_pll_a, new_baseclock); if (err) { dev_err(data->dev, "Can't set pll_a rate: %d\n", err); @@ -77,12 +73,6 @@ int tegra_asoc_utils_set_rate(struct tegra_asoc_utils_data *data, int srate, /* Don't set cdev1/extern1 rate; it's locked to pll_a_out0 */ - err = clk_prepare_enable(data->clk_cdev1); - if (err) { - dev_err(data->dev, "Can't enable cdev1: %d\n", err); - return err; - } - data->set_baseclock = new_baseclock; data->set_mclk = mclk; @@ -96,9 +86,6 @@ int tegra_asoc_utils_set_ac97_rate(struct tegra_asoc_utils_data *data) const int ac97_rate = 24576000; int err; - if (__clk_is_enabled(data->clk_cdev1)) - clk_disable_unprepare(data->clk_cdev1); - /* * AC97 rate is fixed at 24.576MHz and is used for both the host * controller and the external codec @@ -117,12 +104,6 @@ int tegra_asoc_utils_set_ac97_rate(struct tegra_asoc_utils_data *data) /* Don't set cdev1/extern1 rate; it's locked to pll_a_out0 */ - err = clk_prepare_enable(data->clk_cdev1); - if (err) { - dev_err(data->dev, "Can't enable cdev1: %d\n", err); - return err; - } - data->set_baseclock = pll_rate; data->set_mclk = ac97_rate; @@ -213,6 +194,12 @@ int tegra_asoc_utils_init(struct tegra_asoc_utils_data *data, data->clk_cdev1 = clk_out_1; } + ret = clk_prepare_enable(data->clk_cdev1); + if (ret) { + dev_err(data->dev, "Can't enable cdev1: %d\n", ret); + return ret; + } + ret = tegra_asoc_utils_set_rate(data, 44100, 256 * 44100); return ret; > err = clk_set_rate(data->clk_pll_a, new_baseclock); > if (err) { > @@ -77,18 +77,6 @@ int tegra_asoc_utils_set_rate(struct tegra_asoc_utils_data *data, int srate, > > /* Don't set cdev1/extern1 rate; it's locked to pll_a_out0 */ > > - err = clk_prepare_enable(data->clk_pll_a); > - if (err) { > - dev_err(data->dev, "Can't enable pll_a: %d\n", err); > - return err; > - } > - > - err = clk_prepare_enable(data->clk_pll_a_out0); > - if (err) { > - dev_err(data->dev, "Can't enable pll_a_out0: %d\n", err); > - return err; > - } > - > err = clk_prepare_enable(data->clk_cdev1); > if (err) { > dev_err(data->dev, "Can't enable cdev1: %d\n", err); > @@ -108,9 +96,8 @@ int tegra_asoc_utils_set_ac97_rate(struct tegra_asoc_utils_data *data) > const int ac97_rate = 24576000; > int err; > > - clk_disable_unprepare(data->clk_cdev1); > - clk_disable_unprepare(data->clk_pll_a_out0); > - clk_disable_unprepare(data->clk_pll_a); > + if (__clk_is_enabled(data->clk_cdev1)) > + clk_disable_unprepare(data->clk_cdev1); > > /* > * AC97 rate is fixed at 24.576MHz and is used for both the host > @@ -130,18 +117,6 @@ int tegra_asoc_utils_set_ac97_rate(struct tegra_asoc_utils_data *data) > > /* Don't set cdev1/extern1 rate; it's locked to pll_a_out0 */ > > - err = clk_prepare_enable(data->clk_pll_a); > - if (err) { > - dev_err(data->dev, "Can't enable pll_a: %d\n", err); > - return err; > - } > - > - err = clk_prepare_enable(data->clk_pll_a_out0); > - if (err) { > - dev_err(data->dev, "Can't enable pll_a_out0: %d\n", err); > - return err; > - } > - > err = clk_prepare_enable(data->clk_cdev1); > if (err) { > dev_err(data->dev, "Can't enable cdev1: %d\n", err); > @@ -158,6 +133,7 @@ EXPORT_SYMBOL_GPL(tegra_asoc_utils_set_ac97_rate); > int tegra_asoc_utils_init(struct tegra_asoc_utils_data *data, > struct device *dev) > { > + struct clk *clk_out_1, *clk_extern1; > int ret; > > data->dev = dev; > @@ -193,6 +169,41 @@ int tegra_asoc_utils_init(struct tegra_asoc_utils_data *data, > return PTR_ERR(data->clk_cdev1); > } > > + /* > + * If clock parents are not set in DT, configure here to use clk_out_1 > + * as mclk and extern1 as parent for Tegra30 and higher. > + */ > + if (!of_find_property(dev->of_node, "assigned-clock-parents", NULL) && > + data->soc > TEGRA_ASOC_UTILS_SOC_TEGRA20) { Please add a message here about falling back to configuring clocks for a legacy device-tree, telling that device-tree needs to be updated. > + clk_extern1 = devm_clk_get(dev, "extern1"); > + if (IS_ERR(clk_extern1)) { > + dev_err(data->dev, "Can't retrieve clk extern1\n"); > + return PTR_ERR(clk_extern1); > + } > + > + ret = clk_set_parent(clk_extern1, data->clk_pll_a_out0); > + if (ret < 0) { > + dev_err(data->dev, > + "Set parent failed for clk extern1\n"); > + return ret; > + } > + > + clk_out_1 = devm_clk_get(dev, "clk_out_1"); > + if (IS_ERR(clk_out_1)) { > + dev_err(data->dev, "Can't retrieve clk clk_out_1\n"); > + return PTR_ERR(clk_out_1); > + } > + > + ret = clk_set_parent(clk_out_1, clk_extern1); > + if (ret < 0) { > + dev_err(data->dev, > + "Set parent failed for clk_out_1\n"); > + return ret; > + } > + > + data->clk_cdev1 = clk_out_1; > + } > + > ret = tegra_asoc_utils_set_rate(data, 44100, 256 * 44100); > > return ret; > I'd also add tegra_asoc_utils_deinit() to disable clock on drivers removal.
23.12.2019 00:14, Dmitry Osipenko пишет: > 21.12.2019 01:26, Sowjanya Komatineni пишет: >> Tegra PMC clock clk_out_1 is dedicated for audio mclk from Tegra30 >> through Tegra210 and currently Tegra clock driver does initial parent >> configuration for audio mclk "clk_out_1" and enables them by default. >> >> With the move of Tera PMC clocks from clock driver to Tegra PMC >> driver, initial parent configuration for audio clocks are through >> the device tree using assigned-clock-parents property. >> >> Default clock parents can be specified in device tree using >> assigned-clocks and assigned-clock-parents and there is no need >> to have clock driver do parent configuration and enable audio related >> clocks. >> >> This patch has implementation for initial parent configuration in >> audio driver when default parent configuration is not specified in the >> device tree using assigned-clock properties and enables audio clocks >> during the clock rate change. >> >> This patch configures PLLA_OUT0 as parent to extern1 and extern1 >> as parent to clk_out_1 and uses clk_out_1 as cdev1 clock to allow >> mclk control from this driver. >> >> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com> >> --- >> sound/soc/tegra/tegra_asoc_utils.c | 71 ++++++++++++++++++++++---------------- >> 1 file changed, 41 insertions(+), 30 deletions(-) >> >> diff --git a/sound/soc/tegra/tegra_asoc_utils.c b/sound/soc/tegra/tegra_asoc_utils.c >> index 38535962029c..fc3135c08f43 100644 >> --- a/sound/soc/tegra/tegra_asoc_utils.c >> +++ b/sound/soc/tegra/tegra_asoc_utils.c >> @@ -7,6 +7,7 @@ >> */ >> >> #include <linux/clk.h> >> +#include <linux/clk-provider.h> > > This is illegal, it is not a clock provider. > >> #include <linux/device.h> >> #include <linux/err.h> >> #include <linux/kernel.h> >> @@ -59,9 +60,8 @@ int tegra_asoc_utils_set_rate(struct tegra_asoc_utils_data *data, int srate, >> data->set_baseclock = 0; >> data->set_mclk = 0; >> >> - clk_disable_unprepare(data->clk_cdev1); >> - clk_disable_unprepare(data->clk_pll_a_out0); >> - clk_disable_unprepare(data->clk_pll_a); >> + if (__clk_is_enabled(data->clk_cdev1)) >> + clk_disable_unprepare(data->clk_cdev1); > > The root of the problem is that you removed clocks enabling from > tegra_asoc_utils_init(). > > I'm not sure why clocks should be disabled during the rate-changing, > probably this action is not really needed. > > diff --git a/sound/soc/tegra/tegra_asoc_utils.c > b/sound/soc/tegra/tegra_asoc_utils.c > index 46ff70c16b74..789fd03e51a7 100644 > --- a/sound/soc/tegra/tegra_asoc_utils.c > +++ b/sound/soc/tegra/tegra_asoc_utils.c > @@ -7,7 +7,6 @@ > */ > > #include <linux/clk.h> > -#include <linux/clk-provider.h> > #include <linux/device.h> > #include <linux/err.h> > #include <linux/kernel.h> > @@ -60,9 +59,6 @@ int tegra_asoc_utils_set_rate(struct > tegra_asoc_utils_data *data, int srate, > data->set_baseclock = 0; > data->set_mclk = 0; > > - if (__clk_is_enabled(data->clk_cdev1)) > - clk_disable_unprepare(data->clk_cdev1); > - > err = clk_set_rate(data->clk_pll_a, new_baseclock); > if (err) { > dev_err(data->dev, "Can't set pll_a rate: %d\n", err); > @@ -77,12 +73,6 @@ int tegra_asoc_utils_set_rate(struct > tegra_asoc_utils_data *data, int srate, > > /* Don't set cdev1/extern1 rate; it's locked to pll_a_out0 */ > > - err = clk_prepare_enable(data->clk_cdev1); > - if (err) { > - dev_err(data->dev, "Can't enable cdev1: %d\n", err); > - return err; > - } > - > data->set_baseclock = new_baseclock; > data->set_mclk = mclk; > > @@ -96,9 +86,6 @@ int tegra_asoc_utils_set_ac97_rate(struct > tegra_asoc_utils_data *data) > const int ac97_rate = 24576000; > int err; > > - if (__clk_is_enabled(data->clk_cdev1)) > - clk_disable_unprepare(data->clk_cdev1); > - > /* > * AC97 rate is fixed at 24.576MHz and is used for both the host > * controller and the external codec > @@ -117,12 +104,6 @@ int tegra_asoc_utils_set_ac97_rate(struct > tegra_asoc_utils_data *data) > > /* Don't set cdev1/extern1 rate; it's locked to pll_a_out0 */ > > - err = clk_prepare_enable(data->clk_cdev1); > - if (err) { > - dev_err(data->dev, "Can't enable cdev1: %d\n", err); > - return err; > - } > - > data->set_baseclock = pll_rate; > data->set_mclk = ac97_rate; > > @@ -213,6 +194,12 @@ int tegra_asoc_utils_init(struct > tegra_asoc_utils_data *data, > data->clk_cdev1 = clk_out_1; > } > > + ret = clk_prepare_enable(data->clk_cdev1); > + if (ret) { > + dev_err(data->dev, "Can't enable cdev1: %d\n", ret); > + return ret; > + } > + > ret = tegra_asoc_utils_set_rate(data, 44100, 256 * 44100); > > return ret; > > >> err = clk_set_rate(data->clk_pll_a, new_baseclock); >> if (err) { >> @@ -77,18 +77,6 @@ int tegra_asoc_utils_set_rate(struct tegra_asoc_utils_data *data, int srate, >> >> /* Don't set cdev1/extern1 rate; it's locked to pll_a_out0 */ >> >> - err = clk_prepare_enable(data->clk_pll_a); >> - if (err) { >> - dev_err(data->dev, "Can't enable pll_a: %d\n", err); >> - return err; >> - } >> - >> - err = clk_prepare_enable(data->clk_pll_a_out0); >> - if (err) { >> - dev_err(data->dev, "Can't enable pll_a_out0: %d\n", err); >> - return err; >> - } >> - >> err = clk_prepare_enable(data->clk_cdev1); >> if (err) { >> dev_err(data->dev, "Can't enable cdev1: %d\n", err); >> @@ -108,9 +96,8 @@ int tegra_asoc_utils_set_ac97_rate(struct tegra_asoc_utils_data *data) >> const int ac97_rate = 24576000; >> int err; >> >> - clk_disable_unprepare(data->clk_cdev1); >> - clk_disable_unprepare(data->clk_pll_a_out0); >> - clk_disable_unprepare(data->clk_pll_a); >> + if (__clk_is_enabled(data->clk_cdev1)) >> + clk_disable_unprepare(data->clk_cdev1); >> >> /* >> * AC97 rate is fixed at 24.576MHz and is used for both the host >> @@ -130,18 +117,6 @@ int tegra_asoc_utils_set_ac97_rate(struct tegra_asoc_utils_data *data) >> >> /* Don't set cdev1/extern1 rate; it's locked to pll_a_out0 */ >> >> - err = clk_prepare_enable(data->clk_pll_a); >> - if (err) { >> - dev_err(data->dev, "Can't enable pll_a: %d\n", err); >> - return err; >> - } >> - >> - err = clk_prepare_enable(data->clk_pll_a_out0); >> - if (err) { >> - dev_err(data->dev, "Can't enable pll_a_out0: %d\n", err); >> - return err; >> - } >> - >> err = clk_prepare_enable(data->clk_cdev1); >> if (err) { >> dev_err(data->dev, "Can't enable cdev1: %d\n", err); >> @@ -158,6 +133,7 @@ EXPORT_SYMBOL_GPL(tegra_asoc_utils_set_ac97_rate); >> int tegra_asoc_utils_init(struct tegra_asoc_utils_data *data, >> struct device *dev) >> { >> + struct clk *clk_out_1, *clk_extern1; >> int ret; >> >> data->dev = dev; >> @@ -193,6 +169,41 @@ int tegra_asoc_utils_init(struct tegra_asoc_utils_data *data, >> return PTR_ERR(data->clk_cdev1); >> } >> >> + /* >> + * If clock parents are not set in DT, configure here to use clk_out_1 >> + * as mclk and extern1 as parent for Tegra30 and higher. >> + */ >> + if (!of_find_property(dev->of_node, "assigned-clock-parents", NULL) && >> + data->soc > TEGRA_ASOC_UTILS_SOC_TEGRA20) { > > Please add a message here about falling back to configuring clocks for a > legacy device-tree, telling that device-tree needs to be updated. > >> + clk_extern1 = devm_clk_get(dev, "extern1"); >> + if (IS_ERR(clk_extern1)) { >> + dev_err(data->dev, "Can't retrieve clk extern1\n"); >> + return PTR_ERR(clk_extern1); >> + } >> + >> + ret = clk_set_parent(clk_extern1, data->clk_pll_a_out0); >> + if (ret < 0) { >> + dev_err(data->dev, >> + "Set parent failed for clk extern1\n"); >> + return ret; >> + } >> + >> + clk_out_1 = devm_clk_get(dev, "clk_out_1"); >> + if (IS_ERR(clk_out_1)) { >> + dev_err(data->dev, "Can't retrieve clk clk_out_1\n"); >> + return PTR_ERR(clk_out_1); >> + } >> + >> + ret = clk_set_parent(clk_out_1, clk_extern1); >> + if (ret < 0) { >> + dev_err(data->dev, >> + "Set parent failed for clk_out_1\n"); >> + return ret; >> + } >> + >> + data->clk_cdev1 = clk_out_1; >> + } >> + >> ret = tegra_asoc_utils_set_rate(data, 44100, 256 * 44100); Actually, this tegra_asoc_utils_set_rate() should be removed because it doesn't do anything useful since drivers configure the clock rate when necessary. >> return ret; >> > > I'd also add tegra_asoc_utils_deinit() to disable clock on drivers removal. >
On Mon, Dec 23, 2019 at 12:14:34AM +0300, Dmitry Osipenko wrote: > 21.12.2019 01:26, Sowjanya Komatineni пишет: > > Tegra PMC clock clk_out_1 is dedicated for audio mclk from Tegra30 > > through Tegra210 and currently Tegra clock driver does initial parent > > configuration for audio mclk "clk_out_1" and enables them by default. Please delete unneeded context from mails when replying. Doing this makes it much easier to find your reply in the message, helping ensure it won't be missed by people scrolling through the irrelevant quoted material. > > - clk_disable_unprepare(data->clk_cdev1); > > - clk_disable_unprepare(data->clk_pll_a_out0); > > - clk_disable_unprepare(data->clk_pll_a); > > + if (__clk_is_enabled(data->clk_cdev1)) > > + clk_disable_unprepare(data->clk_cdev1); > The root of the problem is that you removed clocks enabling from > tegra_asoc_utils_init(). > I'm not sure why clocks should be disabled during the rate-changing, > probably this action is not really needed. I know nothing about this particular device but this is not that unusual a restriction for audio hardware, you often can't robustly reconfigure the clocking for a device while it's active due to issues in the hardware. You often see issues with FIFOs glitching or state machines getting stuck. This may not be an issue here but if it's something that's documented as a requirement it's probably good to pay attention.
25.12.2019 20:57, Mark Brown пишет: > On Mon, Dec 23, 2019 at 12:14:34AM +0300, Dmitry Osipenko wrote: >> 21.12.2019 01:26, Sowjanya Komatineni пишет: >>> Tegra PMC clock clk_out_1 is dedicated for audio mclk from Tegra30 >>> through Tegra210 and currently Tegra clock driver does initial parent >>> configuration for audio mclk "clk_out_1" and enables them by default. > > Please delete unneeded context from mails when replying. Doing this > makes it much easier to find your reply in the message, helping ensure > it won't be missed by people scrolling through the irrelevant quoted > material. Ok >>> - clk_disable_unprepare(data->clk_cdev1); >>> - clk_disable_unprepare(data->clk_pll_a_out0); >>> - clk_disable_unprepare(data->clk_pll_a); >>> + if (__clk_is_enabled(data->clk_cdev1)) >>> + clk_disable_unprepare(data->clk_cdev1); > >> The root of the problem is that you removed clocks enabling from >> tegra_asoc_utils_init(). > >> I'm not sure why clocks should be disabled during the rate-changing, >> probably this action is not really needed. > > I know nothing about this particular device but this is not that > unusual a restriction for audio hardware, you often can't > robustly reconfigure the clocking for a device while it's active > due to issues in the hardware. You often see issues with FIFOs > glitching or state machines getting stuck. This may not be an > issue here but if it's something that's documented as a > requirement it's probably good to pay attention. I don't know details about that hardware either, maybe it is simply not safe to change PLL_A rate dynamically and then CLK_SET_RATE_GATE could be used. If nobody knows for sure, then will be better to keep tegra_asoc_utils_set_rate() unchanged.
On 12/27/19 6:56 AM, Dmitry Osipenko wrote: > 25.12.2019 20:57, Mark Brown пишет: >> On Mon, Dec 23, 2019 at 12:14:34AM +0300, Dmitry Osipenko wrote: >>> 21.12.2019 01:26, Sowjanya Komatineni пишет: >>>> Tegra PMC clock clk_out_1 is dedicated for audio mclk from Tegra30 >>>> through Tegra210 and currently Tegra clock driver does initial parent >>>> configuration for audio mclk "clk_out_1" and enables them by default. >> Please delete unneeded context from mails when replying. Doing this >> makes it much easier to find your reply in the message, helping ensure >> it won't be missed by people scrolling through the irrelevant quoted >> material. > Ok > >>>> - clk_disable_unprepare(data->clk_cdev1); >>>> - clk_disable_unprepare(data->clk_pll_a_out0); >>>> - clk_disable_unprepare(data->clk_pll_a); >>>> + if (__clk_is_enabled(data->clk_cdev1)) >>>> + clk_disable_unprepare(data->clk_cdev1); >>> The root of the problem is that you removed clocks enabling from >>> tegra_asoc_utils_init(). currently, audio mclk and its parent clocks enabling are from clock driver init and not from tegra_asoc_utils_init. >>> I'm not sure why clocks should be disabled during the rate-changing, >>> probably this action is not really needed. >> I know nothing about this particular device but this is not that >> unusual a restriction for audio hardware, you often can't >> robustly reconfigure the clocking for a device while it's active >> due to issues in the hardware. You often see issues with FIFOs >> glitching or state machines getting stuck. This may not be an >> issue here but if it's something that's documented as a >> requirement it's probably good to pay attention. > I don't know details about that hardware either, maybe it is simply not > safe to change PLL_A rate dynamically and then CLK_SET_RATE_GATE could > be used. > > If nobody knows for sure, then will be better to keep > tegra_asoc_utils_set_rate() unchanged. plla rate change through tegra_asoc_utils_set_rate() happens only when there is not active playback or record corresponding to this sound device. So, I don't see reason for disabling clock during rate change and not sure why we had this from the beginning. Thierry/Sameer, Can you please comment? Yes, we can use CLK_SET_RATE_GATE for PLLA and remove clock disabling before rate change.
On 12/22/19 1:18 PM, Dmitry Osipenko wrote: > 23.12.2019 00:14, Dmitry Osipenko пишет: >> 21.12.2019 01:26, Sowjanya Komatineni пишет: >>> Tegra PMC clock clk_out_1 is dedicated for audio mclk from Tegra30 >>> through Tegra210 and currently Tegra clock driver does initial parent >>> configuration for audio mclk "clk_out_1" and enables them by default. >>> >>> With the move of Tera PMC clocks from clock driver to Tegra PMC >>> driver, initial parent configuration for audio clocks are through >>> the device tree using assigned-clock-parents property. >>> >>> Default clock parents can be specified in device tree using >>> assigned-clocks and assigned-clock-parents and there is no need >>> to have clock driver do parent configuration and enable audio related >>> clocks. >>> >>> This patch has implementation for initial parent configuration in >>> audio driver when default parent configuration is not specified in the >>> device tree using assigned-clock properties and enables audio clocks >>> during the clock rate change. >>> >>> This patch configures PLLA_OUT0 as parent to extern1 and extern1 >>> as parent to clk_out_1 and uses clk_out_1 as cdev1 clock to allow >>> mclk control from this driver. >>> >>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com> >>> --- >>> sound/soc/tegra/tegra_asoc_utils.c | 71 ++++++++++++++++++++++---------------- >>> 1 file changed, 41 insertions(+), 30 deletions(-) >>> >>> diff --git a/sound/soc/tegra/tegra_asoc_utils.c b/sound/soc/tegra/tegra_asoc_utils.c >>> index 38535962029c..fc3135c08f43 100644 >>> --- a/sound/soc/tegra/tegra_asoc_utils.c >>> +++ b/sound/soc/tegra/tegra_asoc_utils.c >>> @@ -7,6 +7,7 @@ >>> */ >>> >>> #include <linux/clk.h> >>> +#include <linux/clk-provider.h> >> This is illegal, it is not a clock provider. >> __clk_is_enabled API is used in this patch to disable clock only when its enabled. __clk_is_enabled API is from clk-provider.h >>> #include <linux/device.h> >>> #include <linux/err.h> >>> #include <linux/kernel.h> >>> @@ -59,9 +60,8 @@ int tegra_asoc_utils_set_rate(struct tegra_asoc_utils_data *data, int srate, >>> data->set_baseclock = 0; >>> data->set_mclk = 0; >>> >>> - clk_disable_unprepare(data->clk_cdev1); >>> - clk_disable_unprepare(data->clk_pll_a_out0); >>> - clk_disable_unprepare(data->clk_pll_a); >>> + if (__clk_is_enabled(data->clk_cdev1)) >>> + clk_disable_unprepare(data->clk_cdev1); >> The root of the problem is that you removed clocks enabling from >> tegra_asoc_utils_init(). >> >> I'm not sure why clocks should be disabled during the rate-changing, >> probably this action is not really needed. >> >> diff --git a/sound/soc/tegra/tegra_asoc_utils.c >> b/sound/soc/tegra/tegra_asoc_utils.c >> index 46ff70c16b74..789fd03e51a7 100644 >> --- a/sound/soc/tegra/tegra_asoc_utils.c >> +++ b/sound/soc/tegra/tegra_asoc_utils.c >> @@ -7,7 +7,6 @@ >> */ >> >> #include <linux/clk.h> >> -#include <linux/clk-provider.h> >> #include <linux/device.h> >> #include <linux/err.h> >> #include <linux/kernel.h> >> @@ -60,9 +59,6 @@ int tegra_asoc_utils_set_rate(struct >> tegra_asoc_utils_data *data, int srate, >> data->set_baseclock = 0; >> data->set_mclk = 0; >> >> - if (__clk_is_enabled(data->clk_cdev1)) >> - clk_disable_unprepare(data->clk_cdev1); >> - >> err = clk_set_rate(data->clk_pll_a, new_baseclock); >> if (err) { >> dev_err(data->dev, "Can't set pll_a rate: %d\n", err); >> @@ -77,12 +73,6 @@ int tegra_asoc_utils_set_rate(struct >> tegra_asoc_utils_data *data, int srate, >> >> /* Don't set cdev1/extern1 rate; it's locked to pll_a_out0 */ >> >> - err = clk_prepare_enable(data->clk_cdev1); >> - if (err) { >> - dev_err(data->dev, "Can't enable cdev1: %d\n", err); >> - return err; >> - } >> - >> data->set_baseclock = new_baseclock; >> data->set_mclk = mclk; >> >> @@ -96,9 +86,6 @@ int tegra_asoc_utils_set_ac97_rate(struct >> tegra_asoc_utils_data *data) >> const int ac97_rate = 24576000; >> int err; >> >> - if (__clk_is_enabled(data->clk_cdev1)) >> - clk_disable_unprepare(data->clk_cdev1); >> - >> /* >> * AC97 rate is fixed at 24.576MHz and is used for both the host >> * controller and the external codec >> @@ -117,12 +104,6 @@ int tegra_asoc_utils_set_ac97_rate(struct >> tegra_asoc_utils_data *data) >> >> /* Don't set cdev1/extern1 rate; it's locked to pll_a_out0 */ >> >> - err = clk_prepare_enable(data->clk_cdev1); >> - if (err) { >> - dev_err(data->dev, "Can't enable cdev1: %d\n", err); >> - return err; >> - } >> - >> data->set_baseclock = pll_rate; >> data->set_mclk = ac97_rate; >> >> @@ -213,6 +194,12 @@ int tegra_asoc_utils_init(struct >> tegra_asoc_utils_data *data, >> data->clk_cdev1 = clk_out_1; >> } >> >> + ret = clk_prepare_enable(data->clk_cdev1); >> + if (ret) { >> + dev_err(data->dev, "Can't enable cdev1: %d\n", ret); >> + return ret; >> + } >> + >> ret = tegra_asoc_utils_set_rate(data, 44100, 256 * 44100); >> >> return ret; >> >> >>> err = clk_set_rate(data->clk_pll_a, new_baseclock); >>> if (err) { >>> @@ -77,18 +77,6 @@ int tegra_asoc_utils_set_rate(struct tegra_asoc_utils_data *data, int srate, >>> >>> /* Don't set cdev1/extern1 rate; it's locked to pll_a_out0 */ >>> >>> - err = clk_prepare_enable(data->clk_pll_a); >>> - if (err) { >>> - dev_err(data->dev, "Can't enable pll_a: %d\n", err); >>> - return err; >>> - } >>> - >>> - err = clk_prepare_enable(data->clk_pll_a_out0); >>> - if (err) { >>> - dev_err(data->dev, "Can't enable pll_a_out0: %d\n", err); >>> - return err; >>> - } >>> - >>> err = clk_prepare_enable(data->clk_cdev1); >>> if (err) { >>> dev_err(data->dev, "Can't enable cdev1: %d\n", err); >>> @@ -108,9 +96,8 @@ int tegra_asoc_utils_set_ac97_rate(struct tegra_asoc_utils_data *data) >>> const int ac97_rate = 24576000; >>> int err; >>> >>> - clk_disable_unprepare(data->clk_cdev1); >>> - clk_disable_unprepare(data->clk_pll_a_out0); >>> - clk_disable_unprepare(data->clk_pll_a); >>> + if (__clk_is_enabled(data->clk_cdev1)) >>> + clk_disable_unprepare(data->clk_cdev1); >>> >>> /* >>> * AC97 rate is fixed at 24.576MHz and is used for both the host >>> @@ -130,18 +117,6 @@ int tegra_asoc_utils_set_ac97_rate(struct tegra_asoc_utils_data *data) >>> >>> /* Don't set cdev1/extern1 rate; it's locked to pll_a_out0 */ >>> >>> - err = clk_prepare_enable(data->clk_pll_a); >>> - if (err) { >>> - dev_err(data->dev, "Can't enable pll_a: %d\n", err); >>> - return err; >>> - } >>> - >>> - err = clk_prepare_enable(data->clk_pll_a_out0); >>> - if (err) { >>> - dev_err(data->dev, "Can't enable pll_a_out0: %d\n", err); >>> - return err; >>> - } >>> - >>> err = clk_prepare_enable(data->clk_cdev1); >>> if (err) { >>> dev_err(data->dev, "Can't enable cdev1: %d\n", err); >>> @@ -158,6 +133,7 @@ EXPORT_SYMBOL_GPL(tegra_asoc_utils_set_ac97_rate); >>> int tegra_asoc_utils_init(struct tegra_asoc_utils_data *data, >>> struct device *dev) >>> { >>> + struct clk *clk_out_1, *clk_extern1; >>> int ret; >>> >>> data->dev = dev; >>> @@ -193,6 +169,41 @@ int tegra_asoc_utils_init(struct tegra_asoc_utils_data *data, >>> return PTR_ERR(data->clk_cdev1); >>> } >>> >>> + /* >>> + * If clock parents are not set in DT, configure here to use clk_out_1 >>> + * as mclk and extern1 as parent for Tegra30 and higher. >>> + */ >>> + if (!of_find_property(dev->of_node, "assigned-clock-parents", NULL) && >>> + data->soc > TEGRA_ASOC_UTILS_SOC_TEGRA20) { >> Please add a message here about falling back to configuring clocks for a >> legacy device-tree, telling that device-tree needs to be updated. >> Will add in v6 >>> + clk_extern1 = devm_clk_get(dev, "extern1"); >>> + if (IS_ERR(clk_extern1)) { >>> + dev_err(data->dev, "Can't retrieve clk extern1\n"); >>> + return PTR_ERR(clk_extern1); >>> + } >>> + >>> + ret = clk_set_parent(clk_extern1, data->clk_pll_a_out0); >>> + if (ret < 0) { >>> + dev_err(data->dev, >>> + "Set parent failed for clk extern1\n"); >>> + return ret; >>> + } >>> + >>> + clk_out_1 = devm_clk_get(dev, "clk_out_1"); >>> + if (IS_ERR(clk_out_1)) { >>> + dev_err(data->dev, "Can't retrieve clk clk_out_1\n"); >>> + return PTR_ERR(clk_out_1); >>> + } >>> + >>> + ret = clk_set_parent(clk_out_1, clk_extern1); >>> + if (ret < 0) { >>> + dev_err(data->dev, >>> + "Set parent failed for clk_out_1\n"); >>> + return ret; >>> + } >>> + >>> + data->clk_cdev1 = clk_out_1; >>> + } >>> + >>> ret = tegra_asoc_utils_set_rate(data, 44100, 256 * 44100); > Actually, this tegra_asoc_utils_set_rate() should be removed because it > doesn't do anything useful since drivers configure the clock rate when > necessary. > >>> return ret; >>> >> I'd also add tegra_asoc_utils_deinit() to disable clock on drivers removal. Will add this in v6
On Fri, Dec 27, 2019 at 05:56:06PM +0300, Dmitry Osipenko wrote: > 25.12.2019 20:57, Mark Brown пишет: > > On Mon, Dec 23, 2019 at 12:14:34AM +0300, Dmitry Osipenko wrote: > >> I'm not sure why clocks should be disabled during the rate-changing, > >> probably this action is not really needed. > > I know nothing about this particular device but this is not that > > unusual a restriction for audio hardware, you often can't > > robustly reconfigure the clocking for a device while it's active > If nobody knows for sure, then will be better to keep > tegra_asoc_utils_set_rate() unchanged. Yeah, that's my instinct - unless we understand the disable it's safer not to do it.
28.12.2019 00:25, Sowjanya Komatineni пишет: > > On 12/22/19 1:18 PM, Dmitry Osipenko wrote: >> 23.12.2019 00:14, Dmitry Osipenko пишет: >>> 21.12.2019 01:26, Sowjanya Komatineni пишет: >>>> Tegra PMC clock clk_out_1 is dedicated for audio mclk from Tegra30 >>>> through Tegra210 and currently Tegra clock driver does initial parent >>>> configuration for audio mclk "clk_out_1" and enables them by default. >>>> >>>> With the move of Tera PMC clocks from clock driver to Tegra PMC >>>> driver, initial parent configuration for audio clocks are through >>>> the device tree using assigned-clock-parents property. >>>> >>>> Default clock parents can be specified in device tree using >>>> assigned-clocks and assigned-clock-parents and there is no need >>>> to have clock driver do parent configuration and enable audio related >>>> clocks. >>>> >>>> This patch has implementation for initial parent configuration in >>>> audio driver when default parent configuration is not specified in the >>>> device tree using assigned-clock properties and enables audio clocks >>>> during the clock rate change. >>>> >>>> This patch configures PLLA_OUT0 as parent to extern1 and extern1 >>>> as parent to clk_out_1 and uses clk_out_1 as cdev1 clock to allow >>>> mclk control from this driver. >>>> >>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com> >>>> --- >>>> sound/soc/tegra/tegra_asoc_utils.c | 71 >>>> ++++++++++++++++++++++---------------- >>>> 1 file changed, 41 insertions(+), 30 deletions(-) >>>> >>>> diff --git a/sound/soc/tegra/tegra_asoc_utils.c >>>> b/sound/soc/tegra/tegra_asoc_utils.c >>>> index 38535962029c..fc3135c08f43 100644 >>>> --- a/sound/soc/tegra/tegra_asoc_utils.c >>>> +++ b/sound/soc/tegra/tegra_asoc_utils.c >>>> @@ -7,6 +7,7 @@ >>>> */ >>>> #include <linux/clk.h> >>>> +#include <linux/clk-provider.h> >>> This is illegal, it is not a clock provider. >>> > __clk_is_enabled API is used in this patch to disable clock only when > its enabled. As I wrote in the other reply, this is a wrong solution since it works around the problem and doesn't solve the root of the problem. Please fix it properly in the next version. > __clk_is_enabled API is from clk-provider.h That's exactly the reason why it is in clk-provider.h, you absolutely cannot use __clk_is_enabled() outside of clk providers because: 1. __clk_is_enabled doesn't use any lockings, so you need to be very careful when using it 2. every function that is prefixed with __ is usually meant for internal use only 3. tegra_asoc_utils is simply not a clk provider, such cases when you need to do hacks in order to achieve something are a good indication about that you're likely doing something wrong
02.01.2020 10:03, Sowjanya Komatineni пишет: > > On 12/27/19 1:19 PM, Sowjanya Komatineni wrote: >> >> On 12/27/19 6:56 AM, Dmitry Osipenko wrote: >>> 25.12.2019 20:57, Mark Brown пишет: >>>> On Mon, Dec 23, 2019 at 12:14:34AM +0300, Dmitry Osipenko wrote: >>>>> 21.12.2019 01:26, Sowjanya Komatineni пишет: >>>>>> Tegra PMC clock clk_out_1 is dedicated for audio mclk from Tegra30 >>>>>> through Tegra210 and currently Tegra clock driver does initial parent >>>>>> configuration for audio mclk "clk_out_1" and enables them by default. >>>> Please delete unneeded context from mails when replying. Doing this >>>> makes it much easier to find your reply in the message, helping ensure >>>> it won't be missed by people scrolling through the irrelevant quoted >>>> material. >>> Ok >>> >>>>>> - clk_disable_unprepare(data->clk_cdev1); >>>>>> - clk_disable_unprepare(data->clk_pll_a_out0); >>>>>> - clk_disable_unprepare(data->clk_pll_a); >>>>>> + if (__clk_is_enabled(data->clk_cdev1)) >>>>>> + clk_disable_unprepare(data->clk_cdev1); >>>>> The root of the problem is that you removed clocks enabling from >>>>> tegra_asoc_utils_init(). >> currently, audio mclk and its parent clocks enabling are from clock >> driver init and not from tegra_asoc_utils_init. >>>>> I'm not sure why clocks should be disabled during the rate-changing, >>>>> probably this action is not really needed. >>>> I know nothing about this particular device but this is not that >>>> unusual a restriction for audio hardware, you often can't >>>> robustly reconfigure the clocking for a device while it's active >>>> due to issues in the hardware. You often see issues with FIFOs >>>> glitching or state machines getting stuck. This may not be an >>>> issue here but if it's something that's documented as a >>>> requirement it's probably good to pay attention. >>> I don't know details about that hardware either, maybe it is simply not >>> safe to change PLL_A rate dynamically and then CLK_SET_RATE_GATE could >>> be used. >>> >>> If nobody knows for sure, then will be better to keep >>> tegra_asoc_utils_set_rate() unchanged. >> plla rate change through tegra_asoc_utils_set_rate() happens only when >> there is not active playback or record corresponding to this sound >> device. >> >> So, I don't see reason for disabling clock during rate change and not >> sure why we had this from the beginning. >> >> Thierry/Sameer, >> >> Can you please comment? >> >> Yes, we can use CLK_SET_RATE_GATE for PLLA and remove clock disabling >> before rate change. >> > PLLA is used for both I2S controller clock and also for audio mclk. I2S > driver suspend resume implementations takes care of enabling and > disabling I2S clock but audio mclk will be enabled during that time and > PLLA disable might not happen. So using CLK_SET_RATE_GATE prevents rate > change to happen and as rate change happens only when there is no active > audio record/playback, we can perform rate change without disable/enable > during rate change. > > So probably below changes should be good. > > * remove asoc_utils_set_rate call from asoc_utils_init as set_rate > happens during existing hw_params callback implementations in sound > drivers and there is no need to do rate change during asoc_utils_init. > * remove disable/enable clocks during rate change in asoc_utils_set_rate. > * add startup and shutdown snd_soc_ops callbacks to do enable and > disable audio mclk. > Sounds good, thanks. I'll be happy to review and test it.
04.01.2020 08:49, Sowjanya Komatineni пишет: > > On 1/2/20 8:12 AM, Dmitry Osipenko wrote: >> 02.01.2020 10:03, Sowjanya Komatineni пишет: >>> On 12/27/19 1:19 PM, Sowjanya Komatineni wrote: >>>> On 12/27/19 6:56 AM, Dmitry Osipenko wrote: >>>>> 25.12.2019 20:57, Mark Brown пишет: >>>>>> On Mon, Dec 23, 2019 at 12:14:34AM +0300, Dmitry Osipenko wrote: >>>>>>> 21.12.2019 01:26, Sowjanya Komatineni пишет: >>>>>>>> Tegra PMC clock clk_out_1 is dedicated for audio mclk from Tegra30 >>>>>>>> through Tegra210 and currently Tegra clock driver does initial parent >>>>>>>> configuration for audio mclk "clk_out_1" and enables them by default. >>>>>> Please delete unneeded context from mails when replying. Doing this >>>>>> makes it much easier to find your reply in the message, helping ensure >>>>>> it won't be missed by people scrolling through the irrelevant quoted >>>>>> material. >>>>> Ok >>>>> >>>>>>>> - clk_disable_unprepare(data->clk_cdev1); >>>>>>>> - clk_disable_unprepare(data->clk_pll_a_out0); >>>>>>>> - clk_disable_unprepare(data->clk_pll_a); >>>>>>>> + if (__clk_is_enabled(data->clk_cdev1)) >>>>>>>> + clk_disable_unprepare(data->clk_cdev1); >>>>>>> The root of the problem is that you removed clocks enabling from >>>>>>> tegra_asoc_utils_init(). >>>> currently, audio mclk and its parent clocks enabling are from clock >>>> driver init and not from tegra_asoc_utils_init. >>>>>>> I'm not sure why clocks should be disabled during the rate-changing, >>>>>>> probably this action is not really needed. >>>>>> I know nothing about this particular device but this is not that >>>>>> unusual a restriction for audio hardware, you often can't >>>>>> robustly reconfigure the clocking for a device while it's active >>>>>> due to issues in the hardware. You often see issues with FIFOs >>>>>> glitching or state machines getting stuck. This may not be an >>>>>> issue here but if it's something that's documented as a >>>>>> requirement it's probably good to pay attention. >>>>> I don't know details about that hardware either, maybe it is simply not >>>>> safe to change PLL_A rate dynamically and then CLK_SET_RATE_GATE could >>>>> be used. >>>>> >>>>> If nobody knows for sure, then will be better to keep >>>>> tegra_asoc_utils_set_rate() unchanged. >>>> plla rate change through tegra_asoc_utils_set_rate() happens only when >>>> there is not active playback or record corresponding to this sound >>>> device. >>>> >>>> So, I don't see reason for disabling clock during rate change and not >>>> sure why we had this from the beginning. >>>> >>>> Thierry/Sameer, >>>> >>>> Can you please comment? >>>> >>>> Yes, we can use CLK_SET_RATE_GATE for PLLA and remove clock disabling >>>> before rate change. >>>> >>> PLLA is used for both I2S controller clock and also for audio mclk. I2S >>> driver suspend resume implementations takes care of enabling and >>> disabling I2S clock but audio mclk will be enabled during that time and >>> PLLA disable might not happen. So using CLK_SET_RATE_GATE prevents rate >>> change to happen and as rate change happens only when there is no active >>> audio record/playback, we can perform rate change without disable/enable >>> during rate change. >>> >>> So probably below changes should be good. >>> >>> * remove asoc_utils_set_rate call from asoc_utils_init as set_rate >>> happens during existing hw_params callback implementations in sound >>> drivers and there is no need to do rate change during asoc_utils_init. >>> * remove disable/enable clocks during rate change in asoc_utils_set_rate. >>> * add startup and shutdown snd_soc_ops callbacks to do enable and >>> disable audio mclk. >>> >> Sounds good, thanks. I'll be happy to review and test it. > > Regarding disabling audio mclk during PLLA rate change, no need to > explicitly disable PLLA on asoc utils as clock driver takes care of it > properly during pll rate change. > > But the downstream clock divider hardware can malfunction without > recovery when subject to unstable PLL output during locking, unless > clock is gated. > > So it is recommended to disable downstream clocks during PLL rate change. > > PLLA downstream clocks are I2S and audio mclk (cdev1/clk1 and extern1 > clocks) and I2S clock is disabled in I2S driver by PM runtime ops. The I2S driver uses asynchronous pm_runtime_put() and thus there is no guarantee that I2S clock is disabled at the time of changing PLLA rate. Could this be a problem? > For audio mclk, need to make sure mclk are disabled during rate change. > > So below are the changes to audio clocks that will be in next version. > > * remove tegra_asoc_utils_set_rate call from tegra_asoc_utils_init as > tegra_asoc_utils_set_rate happens during hw_params callback. > * add shutdown snd_soc_ops callbacks to disable of audio mclk. > * remove disable audio mclk (cdev1) and plla clocks prior to rate > change in tegra_asoc_utils_set_rate (as audio mclk will always be in > disabled state every time hw_param callback gets executed) and keep > audio mclk enable after the rate change in tegra_asoc_utils_set_rate. >
On 1/4/20 5:05 PM, Dmitry Osipenko wrote: > 04.01.2020 08:49, Sowjanya Komatineni пишет: >> On 1/2/20 8:12 AM, Dmitry Osipenko wrote: >>> 02.01.2020 10:03, Sowjanya Komatineni пишет: >>>> On 12/27/19 1:19 PM, Sowjanya Komatineni wrote: >>>>> On 12/27/19 6:56 AM, Dmitry Osipenko wrote: >>>>>> 25.12.2019 20:57, Mark Brown пишет: >>>>>>> On Mon, Dec 23, 2019 at 12:14:34AM +0300, Dmitry Osipenko wrote: >>>>>>>> 21.12.2019 01:26, Sowjanya Komatineni пишет: >>>>>>>>> Tegra PMC clock clk_out_1 is dedicated for audio mclk from Tegra30 >>>>>>>>> through Tegra210 and currently Tegra clock driver does initial parent >>>>>>>>> configuration for audio mclk "clk_out_1" and enables them by default. >>>>>>> Please delete unneeded context from mails when replying. Doing this >>>>>>> makes it much easier to find your reply in the message, helping ensure >>>>>>> it won't be missed by people scrolling through the irrelevant quoted >>>>>>> material. >>>>>> Ok >>>>>> >>>>>>>>> - clk_disable_unprepare(data->clk_cdev1); >>>>>>>>> - clk_disable_unprepare(data->clk_pll_a_out0); >>>>>>>>> - clk_disable_unprepare(data->clk_pll_a); >>>>>>>>> + if (__clk_is_enabled(data->clk_cdev1)) >>>>>>>>> + clk_disable_unprepare(data->clk_cdev1); >>>>>>>> The root of the problem is that you removed clocks enabling from >>>>>>>> tegra_asoc_utils_init(). >>>>> currently, audio mclk and its parent clocks enabling are from clock >>>>> driver init and not from tegra_asoc_utils_init. >>>>>>>> I'm not sure why clocks should be disabled during the rate-changing, >>>>>>>> probably this action is not really needed. >>>>>>> I know nothing about this particular device but this is not that >>>>>>> unusual a restriction for audio hardware, you often can't >>>>>>> robustly reconfigure the clocking for a device while it's active >>>>>>> due to issues in the hardware. You often see issues with FIFOs >>>>>>> glitching or state machines getting stuck. This may not be an >>>>>>> issue here but if it's something that's documented as a >>>>>>> requirement it's probably good to pay attention. >>>>>> I don't know details about that hardware either, maybe it is simply not >>>>>> safe to change PLL_A rate dynamically and then CLK_SET_RATE_GATE could >>>>>> be used. >>>>>> >>>>>> If nobody knows for sure, then will be better to keep >>>>>> tegra_asoc_utils_set_rate() unchanged. >>>>> plla rate change through tegra_asoc_utils_set_rate() happens only when >>>>> there is not active playback or record corresponding to this sound >>>>> device. >>>>> >>>>> So, I don't see reason for disabling clock during rate change and not >>>>> sure why we had this from the beginning. >>>>> >>>>> Thierry/Sameer, >>>>> >>>>> Can you please comment? >>>>> >>>>> Yes, we can use CLK_SET_RATE_GATE for PLLA and remove clock disabling >>>>> before rate change. >>>>> >>>> PLLA is used for both I2S controller clock and also for audio mclk. I2S >>>> driver suspend resume implementations takes care of enabling and >>>> disabling I2S clock but audio mclk will be enabled during that time and >>>> PLLA disable might not happen. So using CLK_SET_RATE_GATE prevents rate >>>> change to happen and as rate change happens only when there is no active >>>> audio record/playback, we can perform rate change without disable/enable >>>> during rate change. >>>> >>>> So probably below changes should be good. >>>> >>>> * remove asoc_utils_set_rate call from asoc_utils_init as set_rate >>>> happens during existing hw_params callback implementations in sound >>>> drivers and there is no need to do rate change during asoc_utils_init. >>>> * remove disable/enable clocks during rate change in asoc_utils_set_rate. >>>> * add startup and shutdown snd_soc_ops callbacks to do enable and >>>> disable audio mclk. >>>> >>> Sounds good, thanks. I'll be happy to review and test it. >> Regarding disabling audio mclk during PLLA rate change, no need to >> explicitly disable PLLA on asoc utils as clock driver takes care of it >> properly during pll rate change. >> >> But the downstream clock divider hardware can malfunction without >> recovery when subject to unstable PLL output during locking, unless >> clock is gated. >> >> So it is recommended to disable downstream clocks during PLL rate change. >> >> PLLA downstream clocks are I2S and audio mclk (cdev1/clk1 and extern1 >> clocks) and I2S clock is disabled in I2S driver by PM runtime ops. > The I2S driver uses asynchronous pm_runtime_put() and thus there is no > guarantee that I2S clock is disabled at the time of changing PLLA rate. > Could this be a problem? Looking into soc_pcm_hw_params, I see dai_link hw_params ops happens prior to platform snd_soc_dai_driver hw_params ops. So, PLL rate change thru asoc_utils_set_rate happens during sample rate config of dai_link hw_params ops and during this time I2S will always be in idle state. Sameer, Please confirm. >> For audio mclk, need to make sure mclk are disabled during rate change. >> >> So below are the changes to audio clocks that will be in next version. >> >> * remove tegra_asoc_utils_set_rate call from tegra_asoc_utils_init as >> tegra_asoc_utils_set_rate happens during hw_params callback. >> * add shutdown snd_soc_ops callbacks to disable of audio mclk. >> * remove disable audio mclk (cdev1) and plla clocks prior to rate >> change in tegra_asoc_utils_set_rate (as audio mclk will always be in >> disabled state every time hw_param callback gets executed) and keep >> audio mclk enable after the rate change in tegra_asoc_utils_set_rate. >>
On 1/5/2020 10:33 AM, Sowjanya Komatineni wrote: > > On 1/4/20 5:05 PM, Dmitry Osipenko wrote: >> 04.01.2020 08:49, Sowjanya Komatineni пишет: >>> On 1/2/20 8:12 AM, Dmitry Osipenko wrote: >>>> 02.01.2020 10:03, Sowjanya Komatineni пишет: >>>>> On 12/27/19 1:19 PM, Sowjanya Komatineni wrote: >>>>>> On 12/27/19 6:56 AM, Dmitry Osipenko wrote: >>>>>>> 25.12.2019 20:57, Mark Brown пишет: >>>>>>>> On Mon, Dec 23, 2019 at 12:14:34AM +0300, Dmitry Osipenko wrote: >>>>>>>>> 21.12.2019 01:26, Sowjanya Komatineni пишет: >>>>>>>>>> Tegra PMC clock clk_out_1 is dedicated for audio mclk from >>>>>>>>>> Tegra30 >>>>>>>>>> through Tegra210 and currently Tegra clock driver does >>>>>>>>>> initial parent >>>>>>>>>> configuration for audio mclk "clk_out_1" and enables them by >>>>>>>>>> default. >>>>>>>> Please delete unneeded context from mails when replying. Doing >>>>>>>> this >>>>>>>> makes it much easier to find your reply in the message, helping >>>>>>>> ensure >>>>>>>> it won't be missed by people scrolling through the irrelevant >>>>>>>> quoted >>>>>>>> material. >>>>>>> Ok >>>>>>> >>>>>>>>>> - clk_disable_unprepare(data->clk_cdev1); >>>>>>>>>> - clk_disable_unprepare(data->clk_pll_a_out0); >>>>>>>>>> - clk_disable_unprepare(data->clk_pll_a); >>>>>>>>>> + if (__clk_is_enabled(data->clk_cdev1)) >>>>>>>>>> + clk_disable_unprepare(data->clk_cdev1); >>>>>>>>> The root of the problem is that you removed clocks enabling from >>>>>>>>> tegra_asoc_utils_init(). >>>>>> currently, audio mclk and its parent clocks enabling are from clock >>>>>> driver init and not from tegra_asoc_utils_init. >>>>>>>>> I'm not sure why clocks should be disabled during the >>>>>>>>> rate-changing, >>>>>>>>> probably this action is not really needed. >>>>>>>> I know nothing about this particular device but this is not that >>>>>>>> unusual a restriction for audio hardware, you often can't >>>>>>>> robustly reconfigure the clocking for a device while it's active >>>>>>>> due to issues in the hardware. You often see issues with FIFOs >>>>>>>> glitching or state machines getting stuck. This may not be an >>>>>>>> issue here but if it's something that's documented as a >>>>>>>> requirement it's probably good to pay attention. >>>>>>> I don't know details about that hardware either, maybe it is >>>>>>> simply not >>>>>>> safe to change PLL_A rate dynamically and then CLK_SET_RATE_GATE >>>>>>> could >>>>>>> be used. >>>>>>> >>>>>>> If nobody knows for sure, then will be better to keep >>>>>>> tegra_asoc_utils_set_rate() unchanged. >>>>>> plla rate change through tegra_asoc_utils_set_rate() happens only >>>>>> when >>>>>> there is not active playback or record corresponding to this sound >>>>>> device. >>>>>> >>>>>> So, I don't see reason for disabling clock during rate change and >>>>>> not >>>>>> sure why we had this from the beginning. >>>>>> >>>>>> Thierry/Sameer, >>>>>> >>>>>> Can you please comment? >>>>>> >>>>>> Yes, we can use CLK_SET_RATE_GATE for PLLA and remove clock >>>>>> disabling >>>>>> before rate change. >>>>>> >>>>> PLLA is used for both I2S controller clock and also for audio >>>>> mclk. I2S >>>>> driver suspend resume implementations takes care of enabling and >>>>> disabling I2S clock but audio mclk will be enabled during that >>>>> time and >>>>> PLLA disable might not happen. So using CLK_SET_RATE_GATE prevents >>>>> rate >>>>> change to happen and as rate change happens only when there is no >>>>> active >>>>> audio record/playback, we can perform rate change without >>>>> disable/enable >>>>> during rate change. >>>>> >>>>> So probably below changes should be good. >>>>> >>>>> * remove asoc_utils_set_rate call from asoc_utils_init as set_rate >>>>> happens during existing hw_params callback implementations in >>>>> sound >>>>> drivers and there is no need to do rate change during >>>>> asoc_utils_init. >>>>> * remove disable/enable clocks during rate change in >>>>> asoc_utils_set_rate. >>>>> * add startup and shutdown snd_soc_ops callbacks to do enable and >>>>> disable audio mclk. >>>>> >>>> Sounds good, thanks. I'll be happy to review and test it. >>> Regarding disabling audio mclk during PLLA rate change, no need to >>> explicitly disable PLLA on asoc utils as clock driver takes care of it >>> properly during pll rate change. >>> >>> But the downstream clock divider hardware can malfunction without >>> recovery when subject to unstable PLL output during locking, unless >>> clock is gated. >>> >>> So it is recommended to disable downstream clocks during PLL rate >>> change. >>> >>> PLLA downstream clocks are I2S and audio mclk (cdev1/clk1 and extern1 >>> clocks) and I2S clock is disabled in I2S driver by PM runtime ops. >> The I2S driver uses asynchronous pm_runtime_put() and thus there is no >> guarantee that I2S clock is disabled at the time of changing PLLA rate. >> Could this be a problem? > Looking into soc_pcm_hw_params, I see dai_link hw_params ops happens > prior to platform snd_soc_dai_driver hw_params ops. This is true. > > So, PLL rate change thru asoc_utils_set_rate happens during sample > rate config of dai_link hw_params ops and during this time I2S will > always be in idle state. This is probably not the case, since runtime resume for I2S would have already enabled the clock for I2S and in turn PLLA. The hw_param() call would happen later. We could have used a fixed clock rate for PLLA, but the reason why we are setting the rate at runtime is, we support sample rates (and multuples) of 8kHz and 11.025kHz. Both of these require a different PLLA base rate for downstream clock dividers to work properly. That is why, I think we have two base rates for PLLA. Even if we want to enable the clocks (for i2s) in hw_param(), this still may not help. For example there could be multiple I2S instances, which can use the same PLLA source. ALSA playback/capture on different I2S can run independently. Hence we are not sure if clk_disable_unprepare() in tegra_asoc_utils.c would actually disable PLLA. Hence I think the problem exists in current code too. We really want to allow user to run any sample rate in the supported range. However the sample rate is known during hw_param() callback. Looking at current discussion, we may have to provide an aternate way of switching PLLA base rate (may be not in ALSA callbacks) > > Sameer, Please confirm. > >>> For audio mclk, need to make sure mclk are disabled during rate change. >>> >>> So below are the changes to audio clocks that will be in next version. >>> >>> * remove tegra_asoc_utils_set_rate call from >>> tegra_asoc_utils_init as >>> tegra_asoc_utils_set_rate happens during hw_params callback. >>> * add shutdown snd_soc_ops callbacks to disable of audio mclk. >>> * remove disable audio mclk (cdev1) and plla clocks prior to rate >>> change in tegra_asoc_utils_set_rate (as audio mclk will always >>> be in >>> disabled state every time hw_param callback gets executed) and >>> keep >>> audio mclk enable after the rate change in >>> tegra_asoc_utils_set_rate. >>>
On 1/5/20 8:21 PM, Sameer Pujar wrote: > > On 1/5/2020 10:33 AM, Sowjanya Komatineni wrote: >> >> On 1/4/20 5:05 PM, Dmitry Osipenko wrote: >>> 04.01.2020 08:49, Sowjanya Komatineni пишет: >>>> On 1/2/20 8:12 AM, Dmitry Osipenko wrote: >>>>> 02.01.2020 10:03, Sowjanya Komatineni пишет: >>>>>> On 12/27/19 1:19 PM, Sowjanya Komatineni wrote: >>>>>>> On 12/27/19 6:56 AM, Dmitry Osipenko wrote: >>>>>>>> 25.12.2019 20:57, Mark Brown пишет: >>>>>>>>> On Mon, Dec 23, 2019 at 12:14:34AM +0300, Dmitry Osipenko wrote: >>>>>>>>>> 21.12.2019 01:26, Sowjanya Komatineni пишет: >>>>>>>>>>> Tegra PMC clock clk_out_1 is dedicated for audio mclk from >>>>>>>>>>> Tegra30 >>>>>>>>>>> through Tegra210 and currently Tegra clock driver does >>>>>>>>>>> initial parent >>>>>>>>>>> configuration for audio mclk "clk_out_1" and enables them by >>>>>>>>>>> default. >>>>>>>>> Please delete unneeded context from mails when replying. >>>>>>>>> Doing this >>>>>>>>> makes it much easier to find your reply in the message, >>>>>>>>> helping ensure >>>>>>>>> it won't be missed by people scrolling through the irrelevant >>>>>>>>> quoted >>>>>>>>> material. >>>>>>>> Ok >>>>>>>> >>>>>>>>>>> - clk_disable_unprepare(data->clk_cdev1); >>>>>>>>>>> - clk_disable_unprepare(data->clk_pll_a_out0); >>>>>>>>>>> - clk_disable_unprepare(data->clk_pll_a); >>>>>>>>>>> + if (__clk_is_enabled(data->clk_cdev1)) >>>>>>>>>>> + clk_disable_unprepare(data->clk_cdev1); >>>>>>>>>> The root of the problem is that you removed clocks enabling from >>>>>>>>>> tegra_asoc_utils_init(). >>>>>>> currently, audio mclk and its parent clocks enabling are from clock >>>>>>> driver init and not from tegra_asoc_utils_init. >>>>>>>>>> I'm not sure why clocks should be disabled during the >>>>>>>>>> rate-changing, >>>>>>>>>> probably this action is not really needed. >>>>>>>>> I know nothing about this particular device but this is not that >>>>>>>>> unusual a restriction for audio hardware, you often can't >>>>>>>>> robustly reconfigure the clocking for a device while it's active >>>>>>>>> due to issues in the hardware. You often see issues with FIFOs >>>>>>>>> glitching or state machines getting stuck. This may not be an >>>>>>>>> issue here but if it's something that's documented as a >>>>>>>>> requirement it's probably good to pay attention. >>>>>>>> I don't know details about that hardware either, maybe it is >>>>>>>> simply not >>>>>>>> safe to change PLL_A rate dynamically and then >>>>>>>> CLK_SET_RATE_GATE could >>>>>>>> be used. >>>>>>>> >>>>>>>> If nobody knows for sure, then will be better to keep >>>>>>>> tegra_asoc_utils_set_rate() unchanged. >>>>>>> plla rate change through tegra_asoc_utils_set_rate() happens >>>>>>> only when >>>>>>> there is not active playback or record corresponding to this sound >>>>>>> device. >>>>>>> >>>>>>> So, I don't see reason for disabling clock during rate change >>>>>>> and not >>>>>>> sure why we had this from the beginning. >>>>>>> >>>>>>> Thierry/Sameer, >>>>>>> >>>>>>> Can you please comment? >>>>>>> >>>>>>> Yes, we can use CLK_SET_RATE_GATE for PLLA and remove clock >>>>>>> disabling >>>>>>> before rate change. >>>>>>> >>>>>> PLLA is used for both I2S controller clock and also for audio >>>>>> mclk. I2S >>>>>> driver suspend resume implementations takes care of enabling and >>>>>> disabling I2S clock but audio mclk will be enabled during that >>>>>> time and >>>>>> PLLA disable might not happen. So using CLK_SET_RATE_GATE >>>>>> prevents rate >>>>>> change to happen and as rate change happens only when there is no >>>>>> active >>>>>> audio record/playback, we can perform rate change without >>>>>> disable/enable >>>>>> during rate change. >>>>>> >>>>>> So probably below changes should be good. >>>>>> >>>>>> * remove asoc_utils_set_rate call from asoc_utils_init as >>>>>> set_rate >>>>>> happens during existing hw_params callback implementations >>>>>> in sound >>>>>> drivers and there is no need to do rate change during >>>>>> asoc_utils_init. >>>>>> * remove disable/enable clocks during rate change in >>>>>> asoc_utils_set_rate. >>>>>> * add startup and shutdown snd_soc_ops callbacks to do enable and >>>>>> disable audio mclk. >>>>>> >>>>> Sounds good, thanks. I'll be happy to review and test it. >>>> Regarding disabling audio mclk during PLLA rate change, no need to >>>> explicitly disable PLLA on asoc utils as clock driver takes care of it >>>> properly during pll rate change. >>>> >>>> But the downstream clock divider hardware can malfunction without >>>> recovery when subject to unstable PLL output during locking, unless >>>> clock is gated. >>>> >>>> So it is recommended to disable downstream clocks during PLL rate >>>> change. >>>> >>>> PLLA downstream clocks are I2S and audio mclk (cdev1/clk1 and extern1 >>>> clocks) and I2S clock is disabled in I2S driver by PM runtime ops. >>> The I2S driver uses asynchronous pm_runtime_put() and thus there is no >>> guarantee that I2S clock is disabled at the time of changing PLLA rate. >>> Could this be a problem? >> Looking into soc_pcm_hw_params, I see dai_link hw_params ops happens >> prior to platform snd_soc_dai_driver hw_params ops. > > This is true. > >> >> So, PLL rate change thru asoc_utils_set_rate happens during sample >> rate config of dai_link hw_params ops and during this time I2S will >> always be in idle state. > > This is probably not the case, since runtime resume for I2S would have > already enabled the clock for I2S and in turn PLLA. The hw_param() > call would happen later. > We could have used a fixed clock rate for PLLA, but the reason why we > are setting the rate at runtime is, we support sample rates (and > multuples) of 8kHz and 11.025kHz. > Both of these require a different PLLA base rate for downstream clock > dividers to work properly. That is why, I think we have two base rates > for PLLA. > > Even if we want to enable the clocks (for i2s) in hw_param(), this > still may not help. > For example there could be multiple I2S instances, which can use the > same PLLA source. ALSA playback/capture on different I2S can run > independently. > Hence we are not sure if clk_disable_unprepare() in tegra_asoc_utils.c > would actually disable PLLA. Hence I think the problem exists in > current code too. clk_disable_unprepare in aosc_utils_set_rate will not disable PLLA as it will be in use by other consumer (I2S). But clock driver it self takes care of disabling pll output and keeping it in bypass state so its not really like PLLA output is off. So regarding PLLA rate change, we dont have to explicitly disable in audio driver as pll clock driver takes care during rate update, But consumer clocks of PLLA need to be disabled during rate update so existing dividers doesn't cause any malfunction with new rate update. audio mclk can will be in disabled state during rate update thru hw_params with addition of shutdown snd_soc_ops callbacks to disable of audio mclk. But issue is for I2S clock where clock might be in enabled state and this is issue with current I2S driver already. Sameer/Dmitry, Making sure I2S clock is in disabled state during rate update is something need to work thru for proper fix and this is not related to this patch series as this issue exists with current upstream. So, can we take care of this as separate patch out of this series so I can get this series out as this PMC clock changes are needed for upcoming camera drivers. > > We really want to allow user to run any sample rate in the supported > range. However the sample rate is known during hw_param() callback. > > Looking at current discussion, we may have to provide an aternate way > of switching PLLA base rate (may be not in ALSA callbacks) > >> >> Sameer, Please confirm. >> >>>> For audio mclk, need to make sure mclk are disabled during rate >>>> change. >>>> >>>> So below are the changes to audio clocks that will be in next version. >>>> >>>> * remove tegra_asoc_utils_set_rate call from >>>> tegra_asoc_utils_init as >>>> tegra_asoc_utils_set_rate happens during hw_params callback. >>>> * add shutdown snd_soc_ops callbacks to disable of audio mclk. >>>> * remove disable audio mclk (cdev1) and plla clocks prior to rate >>>> change in tegra_asoc_utils_set_rate (as audio mclk will always >>>> be in >>>> disabled state every time hw_param callback gets executed) and >>>> keep >>>> audio mclk enable after the rate change in >>>> tegra_asoc_utils_set_rate. >>>>
06.01.2020 19:30, Sowjanya Komatineni пишет: > > On 1/6/20 8:09 AM, Sowjanya Komatineni wrote: >> >> On 1/5/20 8:21 PM, Sameer Pujar wrote: >>> >>> On 1/5/2020 10:33 AM, Sowjanya Komatineni wrote: >>>> >>>> On 1/4/20 5:05 PM, Dmitry Osipenko wrote: >>>>> 04.01.2020 08:49, Sowjanya Komatineni пишет: >>>>>> On 1/2/20 8:12 AM, Dmitry Osipenko wrote: >>>>>>> 02.01.2020 10:03, Sowjanya Komatineni пишет: >>>>>>>> On 12/27/19 1:19 PM, Sowjanya Komatineni wrote: >>>>>>>>> On 12/27/19 6:56 AM, Dmitry Osipenko wrote: >>>>>>>>>> 25.12.2019 20:57, Mark Brown пишет: >>>>>>>>>>> On Mon, Dec 23, 2019 at 12:14:34AM +0300, Dmitry Osipenko wrote: >>>>>>>>>>>> 21.12.2019 01:26, Sowjanya Komatineni пишет: >>>>>>>>>>>>> Tegra PMC clock clk_out_1 is dedicated for audio mclk from >>>>>>>>>>>>> Tegra30 >>>>>>>>>>>>> through Tegra210 and currently Tegra clock driver does >>>>>>>>>>>>> initial parent >>>>>>>>>>>>> configuration for audio mclk "clk_out_1" and enables them >>>>>>>>>>>>> by default. >>>>>>>>>>> Please delete unneeded context from mails when replying. >>>>>>>>>>> Doing this >>>>>>>>>>> makes it much easier to find your reply in the message, >>>>>>>>>>> helping ensure >>>>>>>>>>> it won't be missed by people scrolling through the irrelevant >>>>>>>>>>> quoted >>>>>>>>>>> material. >>>>>>>>>> Ok >>>>>>>>>> >>>>>>>>>>>>> - clk_disable_unprepare(data->clk_cdev1); >>>>>>>>>>>>> - clk_disable_unprepare(data->clk_pll_a_out0); >>>>>>>>>>>>> - clk_disable_unprepare(data->clk_pll_a); >>>>>>>>>>>>> + if (__clk_is_enabled(data->clk_cdev1)) >>>>>>>>>>>>> + clk_disable_unprepare(data->clk_cdev1); >>>>>>>>>>>> The root of the problem is that you removed clocks enabling >>>>>>>>>>>> from >>>>>>>>>>>> tegra_asoc_utils_init(). >>>>>>>>> currently, audio mclk and its parent clocks enabling are from >>>>>>>>> clock >>>>>>>>> driver init and not from tegra_asoc_utils_init. >>>>>>>>>>>> I'm not sure why clocks should be disabled during the >>>>>>>>>>>> rate-changing, >>>>>>>>>>>> probably this action is not really needed. >>>>>>>>>>> I know nothing about this particular device but this is not that >>>>>>>>>>> unusual a restriction for audio hardware, you often can't >>>>>>>>>>> robustly reconfigure the clocking for a device while it's active >>>>>>>>>>> due to issues in the hardware. You often see issues with FIFOs >>>>>>>>>>> glitching or state machines getting stuck. This may not be an >>>>>>>>>>> issue here but if it's something that's documented as a >>>>>>>>>>> requirement it's probably good to pay attention. >>>>>>>>>> I don't know details about that hardware either, maybe it is >>>>>>>>>> simply not >>>>>>>>>> safe to change PLL_A rate dynamically and then >>>>>>>>>> CLK_SET_RATE_GATE could >>>>>>>>>> be used. >>>>>>>>>> >>>>>>>>>> If nobody knows for sure, then will be better to keep >>>>>>>>>> tegra_asoc_utils_set_rate() unchanged. >>>>>>>>> plla rate change through tegra_asoc_utils_set_rate() happens >>>>>>>>> only when >>>>>>>>> there is not active playback or record corresponding to this sound >>>>>>>>> device. >>>>>>>>> >>>>>>>>> So, I don't see reason for disabling clock during rate change >>>>>>>>> and not >>>>>>>>> sure why we had this from the beginning. >>>>>>>>> >>>>>>>>> Thierry/Sameer, >>>>>>>>> >>>>>>>>> Can you please comment? >>>>>>>>> >>>>>>>>> Yes, we can use CLK_SET_RATE_GATE for PLLA and remove clock >>>>>>>>> disabling >>>>>>>>> before rate change. >>>>>>>>> >>>>>>>> PLLA is used for both I2S controller clock and also for audio >>>>>>>> mclk. I2S >>>>>>>> driver suspend resume implementations takes care of enabling and >>>>>>>> disabling I2S clock but audio mclk will be enabled during that >>>>>>>> time and >>>>>>>> PLLA disable might not happen. So using CLK_SET_RATE_GATE >>>>>>>> prevents rate >>>>>>>> change to happen and as rate change happens only when there is >>>>>>>> no active >>>>>>>> audio record/playback, we can perform rate change without >>>>>>>> disable/enable >>>>>>>> during rate change. >>>>>>>> >>>>>>>> So probably below changes should be good. >>>>>>>> >>>>>>>> * remove asoc_utils_set_rate call from asoc_utils_init as >>>>>>>> set_rate >>>>>>>> happens during existing hw_params callback implementations >>>>>>>> in sound >>>>>>>> drivers and there is no need to do rate change during >>>>>>>> asoc_utils_init. >>>>>>>> * remove disable/enable clocks during rate change in >>>>>>>> asoc_utils_set_rate. >>>>>>>> * add startup and shutdown snd_soc_ops callbacks to do enable >>>>>>>> and >>>>>>>> disable audio mclk. >>>>>>>> >>>>>>> Sounds good, thanks. I'll be happy to review and test it. >>>>>> Regarding disabling audio mclk during PLLA rate change, no need to >>>>>> explicitly disable PLLA on asoc utils as clock driver takes care >>>>>> of it >>>>>> properly during pll rate change. >>>>>> >>>>>> But the downstream clock divider hardware can malfunction without >>>>>> recovery when subject to unstable PLL output during locking, unless >>>>>> clock is gated. >>>>>> >>>>>> So it is recommended to disable downstream clocks during PLL rate >>>>>> change. >>>>>> >>>>>> PLLA downstream clocks are I2S and audio mclk (cdev1/clk1 and extern1 >>>>>> clocks) and I2S clock is disabled in I2S driver by PM runtime ops. >>>>> The I2S driver uses asynchronous pm_runtime_put() and thus there is no >>>>> guarantee that I2S clock is disabled at the time of changing PLLA >>>>> rate. >>>>> Could this be a problem? >>>> Looking into soc_pcm_hw_params, I see dai_link hw_params ops happens >>>> prior to platform snd_soc_dai_driver hw_params ops. >>> >>> This is true. >>> >>>> >>>> So, PLL rate change thru asoc_utils_set_rate happens during sample >>>> rate config of dai_link hw_params ops and during this time I2S will >>>> always be in idle state. >>> >>> This is probably not the case, since runtime resume for I2S would >>> have already enabled the clock for I2S and in turn PLLA. The >>> hw_param() call would happen later. >>> We could have used a fixed clock rate for PLLA, but the reason why we >>> are setting the rate at runtime is, we support sample rates (and >>> multuples) of 8kHz and 11.025kHz. >>> Both of these require a different PLLA base rate for downstream clock >>> dividers to work properly. That is why, I think we have two base >>> rates for PLLA. >>> >>> Even if we want to enable the clocks (for i2s) in hw_param(), this >>> still may not help. >>> For example there could be multiple I2S instances, which can use the >>> same PLLA source. ALSA playback/capture on different I2S can run >>> independently. >>> Hence we are not sure if clk_disable_unprepare() in >>> tegra_asoc_utils.c would actually disable PLLA. Hence I think the >>> problem exists in current code too. >> >> clk_disable_unprepare in aosc_utils_set_rate will not disable PLLA as >> it will be in use by other consumer (I2S). >> >> But clock driver it self takes care of disabling pll output and >> keeping it in bypass state so its not really like PLLA output is off. >> >> So regarding PLLA rate change, we dont have to explicitly disable in >> audio driver as pll clock driver takes care during rate update, >> >> But consumer clocks of PLLA need to be disabled during rate update so >> existing dividers doesn't cause any malfunction with new rate update. >> >> audio mclk can will be in disabled state during rate update thru >> hw_params with addition of shutdown snd_soc_ops callbacks to disable >> of audio mclk. >> >> But issue is for I2S clock where clock might be in enabled state and >> this is issue with current I2S driver already. >> >> Sameer/Dmitry, >> >> Making sure I2S clock is in disabled state during rate update is >> something need to work thru for proper fix and this is not related to >> this patch series as this issue exists with current upstream. >> >> So, can we take care of this as separate patch out of this series so I >> can get this series out as this PMC clock changes are needed for >> upcoming camera drivers. >> > Based on internal discussion with sameer, proper I2S clock fix will be > take care separately and this is not something introduced with this > patch series. > > So, will move once with audio MCLK fix in next version of patch series. > > * remove asoc_utils_set_rate call from asoc_utils_init as set_rate > happens during hw_params callback based on existing driver > * add shutdown snd_soc_ops callbacks to disable of audio mclk > * remove disable clocks prior to rate change in asoc_utils_set_rate > (as audio mclk will already be in disabled state by the time > hw_param callback gets executed) and keep audio mclk enable after > rate change in asoc_utils_set_rate > >>> We really want to allow user to run any sample rate in the supported >>> range. However the sample rate is known during hw_param() callback. >>> >>> Looking at current discussion, we may have to provide an aternate way >>> of switching PLLA base rate (may be not in ALSA callbacks) >>> >>>> >>>> Sameer, Please confirm. >>>> >>>>>> For audio mclk, need to make sure mclk are disabled during rate >>>>>> change. >>>>>> >>>>>> So below are the changes to audio clocks that will be in next >>>>>> version. >>>>>> >>>>>> * remove tegra_asoc_utils_set_rate call from >>>>>> tegra_asoc_utils_init as >>>>>> tegra_asoc_utils_set_rate happens during hw_params callback. >>>>>> * add shutdown snd_soc_ops callbacks to disable of audio mclk. >>>>>> * remove disable audio mclk (cdev1) and plla clocks prior to rate >>>>>> change in tegra_asoc_utils_set_rate (as audio mclk will >>>>>> always be in >>>>>> disabled state every time hw_param callback gets executed) >>>>>> and keep >>>>>> audio mclk enable after the rate change in >>>>>> tegra_asoc_utils_set_rate. >>>>>> Indeed, it should be better to factor out all changes that are not directly related to the PMC patches into separate patchset.
diff --git a/sound/soc/tegra/tegra_asoc_utils.c b/sound/soc/tegra/tegra_asoc_utils.c index 38535962029c..fc3135c08f43 100644 --- a/sound/soc/tegra/tegra_asoc_utils.c +++ b/sound/soc/tegra/tegra_asoc_utils.c @@ -7,6 +7,7 @@ */ #include <linux/clk.h> +#include <linux/clk-provider.h> #include <linux/device.h> #include <linux/err.h> #include <linux/kernel.h> @@ -59,9 +60,8 @@ int tegra_asoc_utils_set_rate(struct tegra_asoc_utils_data *data, int srate, data->set_baseclock = 0; data->set_mclk = 0; - clk_disable_unprepare(data->clk_cdev1); - clk_disable_unprepare(data->clk_pll_a_out0); - clk_disable_unprepare(data->clk_pll_a); + if (__clk_is_enabled(data->clk_cdev1)) + clk_disable_unprepare(data->clk_cdev1); err = clk_set_rate(data->clk_pll_a, new_baseclock); if (err) { @@ -77,18 +77,6 @@ int tegra_asoc_utils_set_rate(struct tegra_asoc_utils_data *data, int srate, /* Don't set cdev1/extern1 rate; it's locked to pll_a_out0 */ - err = clk_prepare_enable(data->clk_pll_a); - if (err) { - dev_err(data->dev, "Can't enable pll_a: %d\n", err); - return err; - } - - err = clk_prepare_enable(data->clk_pll_a_out0); - if (err) { - dev_err(data->dev, "Can't enable pll_a_out0: %d\n", err); - return err; - } - err = clk_prepare_enable(data->clk_cdev1); if (err) { dev_err(data->dev, "Can't enable cdev1: %d\n", err); @@ -108,9 +96,8 @@ int tegra_asoc_utils_set_ac97_rate(struct tegra_asoc_utils_data *data) const int ac97_rate = 24576000; int err; - clk_disable_unprepare(data->clk_cdev1); - clk_disable_unprepare(data->clk_pll_a_out0); - clk_disable_unprepare(data->clk_pll_a); + if (__clk_is_enabled(data->clk_cdev1)) + clk_disable_unprepare(data->clk_cdev1); /* * AC97 rate is fixed at 24.576MHz and is used for both the host @@ -130,18 +117,6 @@ int tegra_asoc_utils_set_ac97_rate(struct tegra_asoc_utils_data *data) /* Don't set cdev1/extern1 rate; it's locked to pll_a_out0 */ - err = clk_prepare_enable(data->clk_pll_a); - if (err) { - dev_err(data->dev, "Can't enable pll_a: %d\n", err); - return err; - } - - err = clk_prepare_enable(data->clk_pll_a_out0); - if (err) { - dev_err(data->dev, "Can't enable pll_a_out0: %d\n", err); - return err; - } - err = clk_prepare_enable(data->clk_cdev1); if (err) { dev_err(data->dev, "Can't enable cdev1: %d\n", err); @@ -158,6 +133,7 @@ EXPORT_SYMBOL_GPL(tegra_asoc_utils_set_ac97_rate); int tegra_asoc_utils_init(struct tegra_asoc_utils_data *data, struct device *dev) { + struct clk *clk_out_1, *clk_extern1; int ret; data->dev = dev; @@ -193,6 +169,41 @@ int tegra_asoc_utils_init(struct tegra_asoc_utils_data *data, return PTR_ERR(data->clk_cdev1); } + /* + * If clock parents are not set in DT, configure here to use clk_out_1 + * as mclk and extern1 as parent for Tegra30 and higher. + */ + if (!of_find_property(dev->of_node, "assigned-clock-parents", NULL) && + data->soc > TEGRA_ASOC_UTILS_SOC_TEGRA20) { + clk_extern1 = devm_clk_get(dev, "extern1"); + if (IS_ERR(clk_extern1)) { + dev_err(data->dev, "Can't retrieve clk extern1\n"); + return PTR_ERR(clk_extern1); + } + + ret = clk_set_parent(clk_extern1, data->clk_pll_a_out0); + if (ret < 0) { + dev_err(data->dev, + "Set parent failed for clk extern1\n"); + return ret; + } + + clk_out_1 = devm_clk_get(dev, "clk_out_1"); + if (IS_ERR(clk_out_1)) { + dev_err(data->dev, "Can't retrieve clk clk_out_1\n"); + return PTR_ERR(clk_out_1); + } + + ret = clk_set_parent(clk_out_1, clk_extern1); + if (ret < 0) { + dev_err(data->dev, + "Set parent failed for clk_out_1\n"); + return ret; + } + + data->clk_cdev1 = clk_out_1; + } + ret = tegra_asoc_utils_set_rate(data, 44100, 256 * 44100); return ret;
Tegra PMC clock clk_out_1 is dedicated for audio mclk from Tegra30 through Tegra210 and currently Tegra clock driver does initial parent configuration for audio mclk "clk_out_1" and enables them by default. With the move of Tera PMC clocks from clock driver to Tegra PMC driver, initial parent configuration for audio clocks are through the device tree using assigned-clock-parents property. Default clock parents can be specified in device tree using assigned-clocks and assigned-clock-parents and there is no need to have clock driver do parent configuration and enable audio related clocks. This patch has implementation for initial parent configuration in audio driver when default parent configuration is not specified in the device tree using assigned-clock properties and enables audio clocks during the clock rate change. This patch configures PLLA_OUT0 as parent to extern1 and extern1 as parent to clk_out_1 and uses clk_out_1 as cdev1 clock to allow mclk control from this driver. Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com> --- sound/soc/tegra/tegra_asoc_utils.c | 71 ++++++++++++++++++++++---------------- 1 file changed, 41 insertions(+), 30 deletions(-)