Ping
On 2015-11-23 14:45, Stefan Agner wrote: > A FTM PWM instance enables/disables three clocks: The bus clock, the > counter clock and the PWM clock. The bus clock gets enabled on > pwm_request, whereas the counter and PWM clocks will be enabled upon > pwm_enable. > > The driver has three closesly related issues when enabling/disabling > clocks during suspend/resume: > - The three clocks are not treated differently in regards to the > individual PWM state enabled/requested. This can lead to clocks > getting disabled which have not been enabled in the first place > (a PWM channel which only has been requested going through > suspend/resume). > > - When entering suspend, the current behavior relies on the > FTM_OUTMASK register: If a PWM output is unmasked, the driver > assumes the clocks are enabled. However, some PWM instances > have only 2 channels connected (e.g. Vybrid's FTM1). In that case, > the FTM_OUTMASK reads 0x3 if all channels are disabled, even if > the code wrote 0xff to it before. For those PWM instances, the > current approach to detect enabled PWM signals does not work. > > - A third issue applies to the bus clock only, which can get enabled > multiple times (once for each PWM channel of a PWM chip). This is > fine, however when entering suspend mode, the clock only gets > disabled once. > > This change introduces a different approach by relying on the enable > and prepared counters of the clock framework and using the frameworks > PWM signal states to address all three issues. > > Clocks get disabled during suspend and back enabled on resume > regarding to the PWM channels individual state (requested/enabled). > > Since we do not count the clock enables in the driver, this change no > longer clears the Status and Control registers Clock Source Selection > (FTM_SC[CLKS]). However, since we disable the selected clock anyway, > and we explicitly select the clock source on reenabling a PWM channel > this approach should not make a difference in practice. > > Signed-off-by: Stefan Agner <ste...@agner.ch> > --- > Hi Lee, > > I just found your new email address. Thierry and I already had some > discussion about the patch here: > http://thread.gmane.org/gmane.linux.pwm/2878 > > Could you have a look at that patch? > > Changes since v1: > - Use the pwm_is_enabled helper > > drivers/pwm/pwm-fsl-ftm.c | 58 > ++++++++++++++++++++--------------------------- > 1 file changed, 25 insertions(+), 33 deletions(-) > > diff --git a/drivers/pwm/pwm-fsl-ftm.c b/drivers/pwm/pwm-fsl-ftm.c > index f9dfc8b..7225ac6 100644 > --- a/drivers/pwm/pwm-fsl-ftm.c > +++ b/drivers/pwm/pwm-fsl-ftm.c > @@ -80,7 +80,6 @@ struct fsl_pwm_chip { > > struct mutex lock; > > - unsigned int use_count; > unsigned int cnt_select; > unsigned int clk_ps; > > @@ -300,9 +299,6 @@ static int fsl_counter_clock_enable(struct > fsl_pwm_chip *fpc) > { > int ret; > > - if (fpc->use_count++ != 0) > - return 0; > - > /* select counter clock source */ > regmap_update_bits(fpc->regmap, FTM_SC, FTM_SC_CLK_MASK, > FTM_SC_CLK(fpc->cnt_select)); > @@ -334,25 +330,6 @@ static int fsl_pwm_enable(struct pwm_chip *chip, > struct pwm_device *pwm) > return ret; > } > > -static void fsl_counter_clock_disable(struct fsl_pwm_chip *fpc) > -{ > - /* > - * already disabled, do nothing > - */ > - if (fpc->use_count == 0) > - return; > - > - /* there are still users, so can't disable yet */ > - if (--fpc->use_count > 0) > - return; > - > - /* no users left, disable PWM counter clock */ > - regmap_update_bits(fpc->regmap, FTM_SC, FTM_SC_CLK_MASK, 0); > - > - clk_disable_unprepare(fpc->clk[FSL_PWM_CLK_CNTEN]); > - clk_disable_unprepare(fpc->clk[fpc->cnt_select]); > -} > - > static void fsl_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm) > { > struct fsl_pwm_chip *fpc = to_fsl_chip(chip); > @@ -362,7 +339,8 @@ static void fsl_pwm_disable(struct pwm_chip *chip, > struct pwm_device *pwm) > regmap_update_bits(fpc->regmap, FTM_OUTMASK, BIT(pwm->hwpwm), > BIT(pwm->hwpwm)); > > - fsl_counter_clock_disable(fpc); > + clk_disable_unprepare(fpc->clk[FSL_PWM_CLK_CNTEN]); > + clk_disable_unprepare(fpc->clk[fpc->cnt_select]); > > regmap_read(fpc->regmap, FTM_OUTMASK, &val); > if ((val & 0xFF) == 0xFF) > @@ -492,17 +470,24 @@ static int fsl_pwm_remove(struct platform_device *pdev) > static int fsl_pwm_suspend(struct device *dev) > { > struct fsl_pwm_chip *fpc = dev_get_drvdata(dev); > - u32 val; > + int i; > > regcache_cache_only(fpc->regmap, true); > regcache_mark_dirty(fpc->regmap); > > - /* read from cache */ > - regmap_read(fpc->regmap, FTM_OUTMASK, &val); > - if ((val & 0xFF) != 0xFF) { > + for (i = 0; i < fpc->chip.npwm; i++) { > + struct pwm_device *pwm = &fpc->chip.pwms[i]; > + > + if (!test_bit(PWMF_REQUESTED, &pwm->flags)) > + continue; > + > + clk_disable_unprepare(fpc->clk[FSL_PWM_CLK_SYS]); > + > + if (!pwm_is_enabled(pwm)) > + continue; > + > clk_disable_unprepare(fpc->clk[FSL_PWM_CLK_CNTEN]); > clk_disable_unprepare(fpc->clk[fpc->cnt_select]); > - clk_disable_unprepare(fpc->clk[FSL_PWM_CLK_SYS]); > } > > return 0; > @@ -511,12 +496,19 @@ static int fsl_pwm_suspend(struct device *dev) > static int fsl_pwm_resume(struct device *dev) > { > struct fsl_pwm_chip *fpc = dev_get_drvdata(dev); > - u32 val; > + int i; > + > + for (i = 0; i < fpc->chip.npwm; i++) { > + struct pwm_device *pwm = &fpc->chip.pwms[i]; > + > + if (!test_bit(PWMF_REQUESTED, &pwm->flags)) > + continue; > > - /* read from cache */ > - regmap_read(fpc->regmap, FTM_OUTMASK, &val); > - if ((val & 0xFF) != 0xFF) { > clk_prepare_enable(fpc->clk[FSL_PWM_CLK_SYS]); > + > + if (!pwm_is_enabled(pwm)) > + continue; > + > clk_prepare_enable(fpc->clk[fpc->cnt_select]); > clk_prepare_enable(fpc->clk[FSL_PWM_CLK_CNTEN]); > } -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
