| Message ID | 20180220221511.22861-2-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Feb 20, 2018 at 11:15:02PM +0100, Hans de Goede wrote: > TL;DR: The PLL1-supply must always be powered up when PLL1 is the sysclk > source and we can simply set the RT5651_PWR_PLL bit when selecting PLL1. Only if jack detection is enabled, if jack detection is not in use for some reason then the PLL isn't required and should be powered off - this is normally handled by having the jack detection code force enable things.
Hi, On 21-02-18 12:18, Mark Brown wrote: > On Tue, Feb 20, 2018 at 11:15:02PM +0100, Hans de Goede wrote: > >> TL;DR: The PLL1-supply must always be powered up when PLL1 is the sysclk >> source and we can simply set the RT5651_PWR_PLL bit when selecting PLL1. > > Only if jack detection is enabled, if jack detection is not in use for > some reason then the PLL isn't required and should be powered off - this > is normally handled by having the jack detection code force enable > things. As the commit message tries to explain, the code this removes is fundamentally broken and this is not jack-detection related: "dapm_power_widgets() first builds a list of which widgets to power-up before actually powering any of them up. For dapm-supply widgets their connected method, in our case is_sys_clk_from_pll() get called at this point. Before this commit is_sys_clk_from_pll() was looking at the actually selected clock in the RT5651_GBL_CLK register. But the sysclk itself is selected by another dapm-supply (the "Platform Clock" supply in the machine driver) and any changes to that supply part of the same power- transition get executed after building the list, causing is_sys_clk_from_pll() to return the wrong value." I actually wrote this patch for the rt5640 driver first (but my rt5640 work / patch-series is not finished yet). I saw the following problem on rt5640 boards before even introducing jack-detect: 1) Have a generic UCM file 2) Start recording 3) Select an input which is not part of the input-map quirk in the machine driver 4) Switch to an input which is part of the input-map 5) Notice how only silence is recorded 6) Use i2c-get to get the PWR_ANLG2 register, observe that the PLL-bit is OFF at this point despite a stream being recorded due to the ordering issues 7) Use i2c-get to get the GLB_CLK register notice that it does point to PLL1, so is_sys_clk_from_pll() will return 1 if called *NOW*, but clearly it returned 0 when called as proven by 6) which shows the ordering issue is real 8) Use i2c-set to enable the PLL-bit in PWR_ANLG2 and the recording starts working As I tried to explain in the commit message the is_sys_clk_from_pll() thingie simply can NOT work because it should check for what the GLB_CLK register will be *after* the dapm transaction / transition but in reality it checks for what it was *before* the transition, which means that it will only return the right thing if 2 transitions are made in a row where the second transition preserves the GLB_CLK value of the first. As for that the PLL should be powered-off when not needed, I agree but that is controlled by the machine-driver through the "Platform Clock" dapm supply and if that leaves things on for some reason then that is the problem which we actually need to fix, e.g. this will also leave the MCLK input clk enabled needlessly. I really don't see how configuring a broken sysclk (GBL_CLK points to PLL1, but PLL-bit in PWR_ANLG2 is OFF) is ever a good thing, instead we should make sure we always switch to the RCCLK when we don't need the SYSCLK. Last force-enabling the PLL bit on all machines where we can do jack-detection (not only over-current, but the JD irq in general needs a valid sysclk) would in practice mean always force-enabling the PLL bit since almost all boards have jack-detection once we add the necessary quirks, so this really is not a solution. Regards, Hans
On Wed, Feb 21, 2018 at 08:38:01PM +0100, Hans de Goede wrote: > On 21-02-18 12:18, Mark Brown wrote: > > On Tue, Feb 20, 2018 at 11:15:02PM +0100, Hans de Goede wrote: > > Only if jack detection is enabled, if jack detection is not in use for > > some reason then the PLL isn't required and should be powered off - this > > is normally handled by having the jack detection code force enable > > things. > As the commit message tries to explain, the code this removes is fundamentally > broken and this is not jack-detection related: > "dapm_power_widgets() first builds a list of which widgets to power-up > before actually powering any of them up. For dapm-supply widgets their > connected method, in our case is_sys_clk_from_pll() get called at this > point. > Before this commit is_sys_clk_from_pll() was looking at the actually > selected clock in the RT5651_GBL_CLK register. But the sysclk itself is > selected by another dapm-supply (the "Platform Clock" supply in the > machine driver) and any changes to that supply part of the same power- > transition get executed after building the list, causing > is_sys_clk_from_pll() to return the wrong value." Right, but there's a couple of jumps in your reasoning with the actual solution. > As for that the PLL should be powered-off when not needed, I agree but that > is controlled by the machine-driver through the "Platform Clock" dapm supply > and if that leaves things on for some reason then that is the problem which > we actually need to fix, e.g. this will also leave the MCLK input clk enabled > needlessly. I don't follow this bit, sorry. If there's a DAPM supply that's being enabled when it's not needed then we should just disable it. > I really don't see how configuring a broken sysclk (GBL_CLK points to PLL1, > but PLL-bit in PWR_ANLG2 is OFF) is ever a good thing, instead we should make > sure we always switch to the RCCLK when we don't need the SYSCLK. > Last force-enabling the PLL bit on all machines where we can do jack-detection > (not only over-current, but the JD irq in general needs a valid sysclk) would > in practice mean always force-enabling the PLL bit since almost all boards have > jack-detection once we add the necessary quirks, so this really is not a solution. A bit confused here as well, sorry - I think you're assuming I'm suggesting some particular solution but I'm not sure what that is.
Hi, On 22-02-18 11:48, Mark Brown wrote: > On Wed, Feb 21, 2018 at 08:38:01PM +0100, Hans de Goede wrote: >> On 21-02-18 12:18, Mark Brown wrote: >>> On Tue, Feb 20, 2018 at 11:15:02PM +0100, Hans de Goede wrote: > >>> Only if jack detection is enabled, if jack detection is not in use for >>> some reason then the PLL isn't required and should be powered off - this >>> is normally handled by having the jack detection code force enable >>> things. > >> As the commit message tries to explain, the code this removes is fundamentally >> broken and this is not jack-detection related: > >> "dapm_power_widgets() first builds a list of which widgets to power-up >> before actually powering any of them up. For dapm-supply widgets their >> connected method, in our case is_sys_clk_from_pll() get called at this >> point. > >> Before this commit is_sys_clk_from_pll() was looking at the actually >> selected clock in the RT5651_GBL_CLK register. But the sysclk itself is >> selected by another dapm-supply (the "Platform Clock" supply in the >> machine driver) and any changes to that supply part of the same power- >> transition get executed after building the list, causing >> is_sys_clk_from_pll() to return the wrong value." > > Right, but there's a couple of jumps in your reasoning with the actual > solution. > >> As for that the PLL should be powered-off when not needed, I agree but that >> is controlled by the machine-driver through the "Platform Clock" dapm supply >> and if that leaves things on for some reason then that is the problem which >> we actually need to fix, e.g. this will also leave the MCLK input clk enabled >> needlessly. > > I don't follow this bit, sorry. If there's a DAPM supply that's being > enabled when it's not needed then we should just disable it. Right, that is exactly what I'm saying too. Whether we use PLL1 as sys-clk or the RCCLK is controlled by the "Platform Clock" dapm supply. If we proper disable that supply when we don't need PLL1, then it will switch to RCCLK and we can simply have rt5651_set_dai_sysclk() enable / disable the PLL pwr bit in PWR_ANLG2 when we switch clock rather then using the is_sys_clk_from_pll() check. I hope this helps clear up the confusion surrounding this patch. And I guess I need to improve the commit message... Regards, Hans
On Thu, Feb 22, 2018 at 12:00:35PM +0100, Hans de Goede wrote: > I hope this helps clear up the confusion surrounding this patch. And I guess > I need to improve the commit message... Yeah, that's *probably* the main issue here.
diff --git a/sound/soc/codecs/rt5651.c b/sound/soc/codecs/rt5651.c index c5d19a80506b..a48278f6205d 100644 --- a/sound/soc/codecs/rt5651.c +++ b/sound/soc/codecs/rt5651.c @@ -393,20 +393,6 @@ static int set_dmic_clk(struct snd_soc_dapm_widget *w, return idx; } -static int is_sysclk_from_pll(struct snd_soc_dapm_widget *source, - struct snd_soc_dapm_widget *sink) -{ - struct snd_soc_codec *codec = snd_soc_dapm_to_codec(source->dapm); - unsigned int val; - - val = snd_soc_read(codec, RT5651_GLB_CLK); - val &= RT5651_SCLK_SRC_MASK; - if (val == RT5651_SCLK_SRC_PLL1) - return 1; - else - return 0; -} - /* Digital Mixer */ static const struct snd_kcontrol_new rt5651_sto1_adc_l_mix[] = { SOC_DAPM_SINGLE("ADC1 Switch", RT5651_STO1_ADC_MIXER, @@ -878,8 +864,6 @@ static const struct snd_soc_dapm_widget rt5651_dapm_widgets[] = { SND_SOC_DAPM_SUPPLY_S("ADC ASRC", 1, RT5651_PLL_MODE_2, 11, 0, NULL, 0), - SND_SOC_DAPM_SUPPLY("PLL1", RT5651_PWR_ANLG2, - RT5651_PWR_PLL_BIT, 0, NULL, 0), /* Input Side */ SND_SOC_DAPM_SUPPLY("JD Power", RT5651_PWR_ANLG2, RT5651_PWM_JD_M_BIT, 0, NULL, 0), @@ -1162,7 +1146,6 @@ static const struct snd_soc_dapm_route rt5651_dapm_routes[] = { {"Stereo1 ADC MIXL", "ADC1 Switch", "Stereo1 ADC L1 Mux"}, {"Stereo1 ADC MIXL", "ADC2 Switch", "Stereo1 ADC L2 Mux"}, {"Stereo1 ADC MIXL", NULL, "Stereo1 Filter"}, - {"Stereo1 Filter", NULL, "PLL1", is_sysclk_from_pll}, {"Stereo1 Filter", NULL, "ADC ASRC"}, {"Stereo1 ADC MIXR", "ADC1 Switch", "Stereo1 ADC R1 Mux"}, @@ -1172,7 +1155,6 @@ static const struct snd_soc_dapm_route rt5651_dapm_routes[] = { {"Stereo2 ADC MIXL", "ADC1 Switch", "Stereo2 ADC L1 Mux"}, {"Stereo2 ADC MIXL", "ADC2 Switch", "Stereo2 ADC L2 Mux"}, {"Stereo2 ADC MIXL", NULL, "Stereo2 Filter"}, - {"Stereo2 Filter", NULL, "PLL1", is_sysclk_from_pll}, {"Stereo2 Filter", NULL, "ADC ASRC"}, {"Stereo2 ADC MIXR", "ADC1 Switch", "Stereo2 ADC R1 Mux"}, @@ -1239,10 +1221,8 @@ static const struct snd_soc_dapm_route rt5651_dapm_routes[] = { {"PDM R Mux", "DD MIX", "DAC MIXR"}, {"DAC L1", NULL, "Stereo DAC MIXL"}, - {"DAC L1", NULL, "PLL1", is_sysclk_from_pll}, {"DAC L1", NULL, "DAC L1 Power"}, {"DAC R1", NULL, "Stereo DAC MIXR"}, - {"DAC R1", NULL, "PLL1", is_sysclk_from_pll}, {"DAC R1", NULL, "DAC R1 Power"}, {"DD MIXL", "DAC L1 Switch", "DAC MIXL"}, @@ -1438,6 +1418,7 @@ static int rt5651_set_dai_sysclk(struct snd_soc_dai *dai, struct snd_soc_codec *codec = dai->codec; struct rt5651_priv *rt5651 = snd_soc_codec_get_drvdata(codec); unsigned int reg_val = 0; + unsigned int pll_bit = 0; if (freq == rt5651->sysclk && clk_id == rt5651->sysclk_src) return 0; @@ -1448,6 +1429,7 @@ static int rt5651_set_dai_sysclk(struct snd_soc_dai *dai, break; case RT5651_SCLK_S_PLL1: reg_val |= RT5651_SCLK_SRC_PLL1; + pll_bit |= RT5651_PWR_PLL; break; case RT5651_SCLK_S_RCCLK: reg_val |= RT5651_SCLK_SRC_RCCLK; @@ -1456,6 +1438,9 @@ static int rt5651_set_dai_sysclk(struct snd_soc_dai *dai, dev_err(codec->dev, "Invalid clock id (%d)\n", clk_id); return -EINVAL; } + + snd_soc_update_bits(codec, RT5651_PWR_ANLG2, + RT5651_PWR_PLL, pll_bit); snd_soc_update_bits(codec, RT5651_GLB_CLK, RT5651_SCLK_SRC_MASK, reg_val); rt5651->sysclk = freq;
dapm_power_widgets() first builds a list of which widgets to power-up before actually powering any of them up. For dapm-supply widgets their connected method, in our case is_sys_clk_from_pll() get called at this point. Before this commit is_sys_clk_from_pll() was looking at the actually selected clock in the RT5651_GBL_CLK register. But the sysclk itself is selected by another dapm-supply (the "Platform Clock" supply in the machine driver) and any changes to that supply part of the same power- transition get executed after building the list, causing is_sys_clk_from_pll() to return the wrong value. This sometimes leads to the PWR_PLL bit not getting set even though we have configured an active audio chain and RT5651_GBL_CLK does point to PLL1 as the sysclk-source. Note that even if we do not have an active audio chain we should still keep the PWR_PLL bit set if PLL1 is our sysclk-source, because we must always have a working sysclk-source, otherwise e.g. jack-detection will not work. If we don't have an active audio-chain then the machine-driver should switch the sysclk-source to the RCCLK and if does not then that is a machine-driver (or UCM config) problem and not something we should try to work around by disabling our sysclk-source and breaking jack-detect. TL;DR: The PLL1-supply must always be powered up when PLL1 is the sysclk source and we can simply set the RT5651_PWR_PLL bit when selecting PLL1. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- Note that many of the other rt56?? drivers have the same is_sys_clk_from_pll() stuff and may need similar fixes. --- sound/soc/codecs/rt5651.c | 25 +++++-------------------- 1 file changed, 5 insertions(+), 20 deletions(-)