| Message ID | 1445142552-3530-1-git-send-email-stefan@agner.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Thierry, I realized that this patch did not make it into 4.4-rc1, while others, IMHO less important patches which have been posted later (e.g. sunxi whitespace fixes) have made it! :-( Anything wrong with that? Or am I on your spam list? Note that this is already a RESEND :-) -- Stefan On 2015-10-17 21:29, Stefan Agner wrote: > The driver has multiple issues when enabling/disabling clocks. A given > 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. > > 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, e.g. Vybrid's FTM1, only > have 2 channels connected. In that case, the FTM_OUTMASK reads only > 0x3, even when it was initialized with 0xff. Hence for those PWM > instances, the current approach does not work at all. > > A second issue is that the three clocks are not treated differently > regarding PWM's enabled/requested state. This can lead to clocks > getting disabled which have not been enabled in the first place (a PWM > channel which only has been requested). > > 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. > > To fix the issues this change introduces a different approach by > relying on the enable/prepared counters of the clock framework. Clocks > get disabled during suspend and back enabled on resume regarding to > the PWM channels individual state. However, since we do not count the > clocks locally, this change no longer clears the Status and Control > registers Clock Source Selection (FTM_SC[CLKS]). There have not > observed issues with that approach so far. > > Signed-off-by: Stefan Agner <stefan@agner.ch> > --- > 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..5e1dd96 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 (!test_bit(PWMF_ENABLED, &pwm->flags)) > + 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 (!test_bit(PWMF_ENABLED, &pwm->flags)) > + continue; > + > clk_prepare_enable(fpc->clk[fpc->cnt_select]); > clk_prepare_enable(fpc->clk[FSL_PWM_CLK_CNTEN]); > }
On 19.11.2015 04:04, Stefan Agner wrote: > Thierry, > > I realized that this patch did not make it into 4.4-rc1, while others, > IMHO less important patches which have been posted later (e.g. sunxi > whitespace fixes) have made it! :-( > > Anything wrong with that? Or am I on your spam list? Note that this is > already a RESEND :-) You are not alone with this problem, I have unreviewed changes in linux-pwm mailing list sent in 2014 :) -- With best wishes, Vladimir
On Wed, Nov 18, 2015 at 06:04:12PM -0800, Stefan Agner wrote: > Thierry, > > I realized that this patch did not make it into 4.4-rc1, while others, > IMHO less important patches which have been posted later (e.g. sunxi > whitespace fixes) have made it! :-( The reason why I merged them is because they are low-risk, whereas this patch of yours changes existing behaviour, and hasn't received any feedback from anyone. So the choice that I need to make is to either trust the original author to have tested the driver and it was working, or you to have verified that it is still working after the patch on all setups that it used to work on. The first option obviously carries the least risk, and that's the reason why the patch hasn't been merged. > Anything wrong with that? Or am I on your spam list? Note that this is > already a RESEND :-) If you want to get this merged, you should try to get some feedback from at least the original author. Xiubo Li and I extensively discussed this back at the time when he submitted the driver and we came up with the current code as the best approach to making sure that clocks are on and off when they should be. So if it's not working for you, I'm fine taking the patch if Xiubo or somebody else has had the chance to look at it and review or test it. So a Reviewed-by or Tested-by tag will go a long way to convince me that it's safe to apply. Also you enumerate all the various bits that are broken, and it would seem to me that they could each be fixed individually rather than go and implement something completely different which might have undesirable side-effects. Such an approach would also make it more likely for me to merge the patch because it would hopefully be more obvious what is being fixed and that it's a correct fix. It's not that I mind the rework, but I'd at least like for someone to verify that it's all still working on existing setups. Now, I understand that people can go missing, so if nobody were to give you a Reviewed-by or Tested-by for a couple of weeks I'd even consider applying without, but as it is, you didn't even Cc Xiubo on the patch, so he's likely missed it entirely. Thierry
On Fri, Nov 20, 2015 at 02:38:05AM +0200, Vladimir Zapolskiy wrote: > On 19.11.2015 04:04, Stefan Agner wrote: > > Thierry, > > > > I realized that this patch did not make it into 4.4-rc1, while others, > > IMHO less important patches which have been posted later (e.g. sunxi > > whitespace fixes) have made it! :-( > > > > Anything wrong with that? Or am I on your spam list? Note that this is > > already a RESEND :-) > > You are not alone with this problem, I have unreviewed changes in > linux-pwm mailing list sent in 2014 :) Please either ping me or resend patches if I don't reply within a week or so. Chances are I've either not seen them or meant to reply but never got around to. Also, the Linux kernel is a community-driven project, so I fully expect contributors to review each other's patches. Review isn't something that only maintainers need to do. Thierry
Thanks Thierry for looking into that! Added Li as recipient to start discussion. Some comments below: On 2015-11-23 01:45, Thierry Reding wrote: > On Wed, Nov 18, 2015 at 06:04:12PM -0800, Stefan Agner wrote: >> Thierry, >> >> I realized that this patch did not make it into 4.4-rc1, while others, >> IMHO less important patches which have been posted later (e.g. sunxi >> whitespace fixes) have made it! :-( > > The reason why I merged them is because they are low-risk, whereas this > patch of yours changes existing behaviour, and hasn't received any > feedback from anyone. So the choice that I need to make is to either > trust the original author to have tested the driver and it was working, > or you to have verified that it is still working after the patch on all > setups that it used to work on. The first option obviously carries the > least risk, and that's the reason why the patch hasn't been merged. > >> Anything wrong with that? Or am I on your spam list? Note that this is >> already a RESEND :-) > > If you want to get this merged, you should try to get some feedback from > at least the original author. Xiubo Li and I extensively discussed this > back at the time when he submitted the driver and we came up with the > current code as the best approach to making sure that clocks are on and > off when they should be. So if it's not working for you, I'm fine taking > the patch if Xiubo or somebody else has had the chance to look at it and > review or test it. So a Reviewed-by or Tested-by tag will go a long way > to convince me that it's safe to apply. In mainline, the driver is only used in Freescale Vybrid device trees. I think most issues have been introduced with the patches adding suspend support: http://thread.gmane.org/gmane.linux.kernel/1806940 Not sure against what suspend/resume implementation Li created the patches, so far there is no suspend/resume implementation for Vybrid upstream. While working on Vybrid's low power mode LPSTOP3, I noticed the issues.... > Also you enumerate all the various bits that are broken, and it would > seem to me that they could each be fixed individually rather than go and > implement something completely different which might have undesirable > side-effects. Such an approach would also make it more likely for me to > merge the patch because it would hopefully be more obvious what is being > fixed and that it's a correct fix. There are different issues addressed by one solution: Using the clock frameworks enable/disable counts... I looked through the code again, it is really quite hard to split it up. I could create one patch which only switches the PWM enable/disable part to clock framework counts, and leave the suspend/resume part broken. Than, in a second patch fix the suspend/resume part. But that sounds like the wrong approach to me too... I took inspiration from other PWM drivers, I think the suspend/resume idea was from TI EHRPWM PWM driver. So this shouldn't be far off what others are doing. > > It's not that I mind the rework, but I'd at least like for someone to > verify that it's all still working on existing setups. Now, I understand > that people can go missing, so if nobody were to give you a Reviewed-by > or Tested-by for a couple of weeks I'd even consider applying without, > but as it is, you didn't even Cc Xiubo on the patch, so he's likely > missed it entirely. I have had Li in the initial email, don't know/remember why I removed him from the list in RESEND... Will keep him in the loop. Just realized that there is now a helper function for test_bit(PWMF_ENABLED,... Will send a v2 anyway then. -- Stefan
On 2015-11-23 13:40, Stefan Agner wrote: > Thanks Thierry for looking into that! > > Added Li as recipient to start discussion. Yeah, kind of remember now, Li is not with Freescale anymore, at least his address bounces: The original message was received at Mon, 23 Nov 2015 14:50:20 -0700 from az84f-il1-il2-vlan904.am.freescale.net [192.88.158.118] ----- The following addresses had permanent fatal errors ----- <Li.Xiubo@freescale.com> (reason: 550 5.1.1 <Li.Xiubo@freescale.com>... User unknown) I did not found a newer/updated email address. I will send a v2 with the helper. -- Stefan > > Some comments below: > > On 2015-11-23 01:45, Thierry Reding wrote: >> On Wed, Nov 18, 2015 at 06:04:12PM -0800, Stefan Agner wrote: >>> Thierry, >>> >>> I realized that this patch did not make it into 4.4-rc1, while others, >>> IMHO less important patches which have been posted later (e.g. sunxi >>> whitespace fixes) have made it! :-( >> >> The reason why I merged them is because they are low-risk, whereas this >> patch of yours changes existing behaviour, and hasn't received any >> feedback from anyone. So the choice that I need to make is to either >> trust the original author to have tested the driver and it was working, >> or you to have verified that it is still working after the patch on all >> setups that it used to work on. The first option obviously carries the >> least risk, and that's the reason why the patch hasn't been merged. >> >>> Anything wrong with that? Or am I on your spam list? Note that this is >>> already a RESEND :-) >> >> If you want to get this merged, you should try to get some feedback from >> at least the original author. Xiubo Li and I extensively discussed this >> back at the time when he submitted the driver and we came up with the >> current code as the best approach to making sure that clocks are on and >> off when they should be. So if it's not working for you, I'm fine taking >> the patch if Xiubo or somebody else has had the chance to look at it and >> review or test it. So a Reviewed-by or Tested-by tag will go a long way >> to convince me that it's safe to apply. > > In mainline, the driver is only used in Freescale Vybrid device trees. I > think most issues have been introduced with the patches adding suspend > support: > http://thread.gmane.org/gmane.linux.kernel/1806940 > > Not sure against what suspend/resume implementation Li created the > patches, so far there is no suspend/resume implementation for Vybrid > upstream. While working on Vybrid's low power mode LPSTOP3, I noticed > the issues.... > >> Also you enumerate all the various bits that are broken, and it would >> seem to me that they could each be fixed individually rather than go and >> implement something completely different which might have undesirable >> side-effects. Such an approach would also make it more likely for me to >> merge the patch because it would hopefully be more obvious what is being >> fixed and that it's a correct fix. > > There are different issues addressed by one solution: Using the clock > frameworks enable/disable counts... I looked through the code again, it > is really quite hard to split it up. I could create one patch which only > switches the PWM enable/disable part to clock framework counts, and > leave the suspend/resume part broken. Than, in a second patch fix the > suspend/resume part. But that sounds like the wrong approach to me > too... > > I took inspiration from other PWM drivers, I think the suspend/resume > idea was from TI EHRPWM PWM driver. So this shouldn't be far off what > others are doing. > >> >> It's not that I mind the rework, but I'd at least like for someone to >> verify that it's all still working on existing setups. Now, I understand >> that people can go missing, so if nobody were to give you a Reviewed-by >> or Tested-by for a couple of weeks I'd even consider applying without, >> but as it is, you didn't even Cc Xiubo on the patch, so he's likely >> missed it entirely. > > I have had Li in the initial email, don't know/remember why I removed > him from the list in RESEND... Will keep him in the loop. > > Just realized that there is now a helper function for > test_bit(PWMF_ENABLED,... Will send a v2 anyway then. > > -- > Stefan
diff --git a/drivers/pwm/pwm-fsl-ftm.c b/drivers/pwm/pwm-fsl-ftm.c index f9dfc8b..5e1dd96 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 (!test_bit(PWMF_ENABLED, &pwm->flags)) + 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 (!test_bit(PWMF_ENABLED, &pwm->flags)) + continue; + clk_prepare_enable(fpc->clk[fpc->cnt_select]); clk_prepare_enable(fpc->clk[FSL_PWM_CLK_CNTEN]); }
The driver has multiple issues when enabling/disabling clocks. A given 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. 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, e.g. Vybrid's FTM1, only have 2 channels connected. In that case, the FTM_OUTMASK reads only 0x3, even when it was initialized with 0xff. Hence for those PWM instances, the current approach does not work at all. A second issue is that the three clocks are not treated differently regarding PWM's enabled/requested state. This can lead to clocks getting disabled which have not been enabled in the first place (a PWM channel which only has been requested). 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. To fix the issues this change introduces a different approach by relying on the enable/prepared counters of the clock framework. Clocks get disabled during suspend and back enabled on resume regarding to the PWM channels individual state. However, since we do not count the clocks locally, this change no longer clears the Status and Control registers Clock Source Selection (FTM_SC[CLKS]). There have not observed issues with that approach so far. Signed-off-by: Stefan Agner <stefan@agner.ch> --- drivers/pwm/pwm-fsl-ftm.c | 58 ++++++++++++++++++++--------------------------- 1 file changed, 25 insertions(+), 33 deletions(-)