| Message ID | 20231019200658.1754190-12-u.kleine-koenig@pengutronix.de |
|---|---|
| State | Accepted |
| Delegated to: | Uwe Kleine-König |
| Headers | show |
| Series | pwm: stm32: Cleanups, get_state() and proper hw take over | expand |
On 10/19/23 22:07, Uwe Kleine-König wrote: > From: Philipp Zabel <p.zabel@pengutronix.de> > > Make the driver take over hardware state without disabling in .probe() > and enable the clock for each enabled channel. > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > [ukleinek: split off from a patch that also implemented .get_state()] > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- > drivers/pwm/pwm-stm32.c | 18 ++++++++++++++---- > 1 file changed, 14 insertions(+), 4 deletions(-) Hi Uwe, I'd only have a suggestion on the commit title: pwm: stm32: Fix enable count for clk in .probe() ^ On first sight, the "Fix" may suggest that this fixes something older in the tree. This would suggest a Fixes tag which isn't the case in this series, as this is a split of the .get_state() patch. Is it possible to rephrase ? something like: set clk enable count when probing channels ? Apart from that, you can add my: Reviewed-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com> --- I've additional questions, unrelated to this patch. I had a look at pwm-stm32-lp.c, the clock enabling is done directly in the .get_state() routine. I think this should be moved to the .probe() routine as done here. There's likely a risk, that clk enable count gets increased artificially, at least since commit cfc4c189bc70 "pwm: Read initial hardware state at request time". Shall I send a patch for pwm-stm32-lp.c, similar as this patch ? Or has it been spotted already ? Best Regards, Thanks, Fabrice > > diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c > index 68239567a564..97a2c3c09b69 100644 > --- a/drivers/pwm/pwm-stm32.c > +++ b/drivers/pwm/pwm-stm32.c > @@ -605,17 +605,21 @@ static void stm32_pwm_detect_complementary(struct stm32_pwm *priv) > priv->have_complementary_output = (ccer != 0); > } > > -static unsigned int stm32_pwm_detect_channels(struct stm32_pwm *priv) > +static unsigned int stm32_pwm_detect_channels(struct stm32_pwm *priv, > + unsigned int *num_enabled) > { > - u32 ccer; > + u32 ccer, ccer_backup; > > /* > * If channels enable bits don't exist writing 1 will have no > * effect so we can detect and count them. > */ > + regmap_read(priv->regmap, TIM_CCER, &ccer_backup); > regmap_set_bits(priv->regmap, TIM_CCER, TIM_CCER_CCXE); > regmap_read(priv->regmap, TIM_CCER, &ccer); > - regmap_clear_bits(priv->regmap, TIM_CCER, TIM_CCER_CCXE); > + regmap_write(priv->regmap, TIM_CCER, ccer_backup); > + > + *num_enabled = hweight32(ccer_backup & TIM_CCER_CCXE); > > return hweight32(ccer & TIM_CCER_CCXE); > } > @@ -626,6 +630,8 @@ static int stm32_pwm_probe(struct platform_device *pdev) > struct device_node *np = dev->of_node; > struct stm32_timers *ddata = dev_get_drvdata(pdev->dev.parent); > struct stm32_pwm *priv; > + unsigned int num_enabled; > + unsigned int i; > int ret; > > priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > @@ -648,7 +654,11 @@ static int stm32_pwm_probe(struct platform_device *pdev) > > priv->chip.dev = dev; > priv->chip.ops = &stm32pwm_ops; > - priv->chip.npwm = stm32_pwm_detect_channels(priv); > + priv->chip.npwm = stm32_pwm_detect_channels(priv, &num_enabled); > + > + /* Initialize clock refcount to number of enabled PWM channels. */ > + for (i = 0; i < num_enabled; i++) > + clk_enable(priv->clk); > > ret = devm_pwmchip_add(dev, &priv->chip); > if (ret < 0)
On Tue, Nov 14, 2023 at 02:35:19PM +0100, Fabrice Gasnier wrote: > On 10/19/23 22:07, Uwe Kleine-König wrote: > > From: Philipp Zabel <p.zabel@pengutronix.de> > > > > Make the driver take over hardware state without disabling in .probe() > > and enable the clock for each enabled channel. > > > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > > [ukleinek: split off from a patch that also implemented .get_state()] > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > --- > > drivers/pwm/pwm-stm32.c | 18 ++++++++++++++---- > > 1 file changed, 14 insertions(+), 4 deletions(-) > > > Hi Uwe, > > I'd only have a suggestion on the commit title: > pwm: stm32: Fix enable count for clk in .probe() > ^ > On first sight, the "Fix" may suggest that this fixes something older in > the tree. This would suggest a Fixes tag which isn't the case in this > series, as this is a split of the .get_state() patch. > Is it possible to rephrase ? IMHO it is a fix, the hw state wasn't properly taken over before. If you want a Fixes line, that's: Fixes: 7edf7369205b ("pwm: Add driver for STM32 plaftorm") > something like: set clk enable count when probing channels ? > > Apart from that, you can add my: > Reviewed-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com> > > --- > I've additional questions, unrelated to this patch. I had a look at > pwm-stm32-lp.c, the clock enabling is done directly in the .get_state() > routine. I think this should be moved to the .probe() routine as done > here. There's likely a risk, that clk enable count gets increased > artificially, at least since commit cfc4c189bc70 "pwm: Read initial > hardware state at request time". > Shall I send a patch for pwm-stm32-lp.c, similar as this patch ? Or has > it been spotted already ? I'm not aware of someone working on stm32-lp, so feel free to prepare a patch! Best regards and thanks for your review, Uwe
On 11/14/23 22:20, Uwe Kleine-König wrote: > On Tue, Nov 14, 2023 at 02:35:19PM +0100, Fabrice Gasnier wrote: >> On 10/19/23 22:07, Uwe Kleine-König wrote: >>> From: Philipp Zabel <p.zabel@pengutronix.de> >>> >>> Make the driver take over hardware state without disabling in .probe() >>> and enable the clock for each enabled channel. >>> >>> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> >>> [ukleinek: split off from a patch that also implemented .get_state()] >>> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> >>> --- >>> drivers/pwm/pwm-stm32.c | 18 ++++++++++++++---- >>> 1 file changed, 14 insertions(+), 4 deletions(-) >> >> >> Hi Uwe, >> >> I'd only have a suggestion on the commit title: >> pwm: stm32: Fix enable count for clk in .probe() >> ^ >> On first sight, the "Fix" may suggest that this fixes something older in >> the tree. This would suggest a Fixes tag which isn't the case in this >> series, as this is a split of the .get_state() patch. >> Is it possible to rephrase ? > > IMHO it is a fix, the hw state wasn't properly taken over before. Hi Uwe, Indeed, the HW state wasn't taken. Instead the driver used to be forcibly stop each channel: regmap_clear_bits(priv->regmap, TIM_CCER, TIM_CCER_CCXE); So the clk enable count (of 0) reflects this. That's kind of consistent state. > If you want a Fixes line, that's: > > Fixes: 7edf7369205b ("pwm: Add driver for STM32 plaftorm") Well, the original driver didn't implement the .get_state. BUT, more thinking about this, I think it lacks to read the global enable status of the timer, e.g. TIM_CR1_CEN. Original driver stops each channel (clear CCXE), but after probing, the global TIM_CR1_CEN bit may remains 1 (from bootloader), without a running clock. If we're talking about fixing the original driver (probably unrelevant without a working .get_state), then I think this part is missing. (with or without a Fixes tag) Could you add a check on global counter enable bit (TIM_CR1_CEN) both in the .get_state(), and in the stm32_pwm_detect_channels(), that counts the num_enabled channels ? In case the TIM_CR1_CEN isn't set (but some of the TIM_CCER_CCXE are), the driver will report enabled channels. But physically, the pwm output will be inactive. That's more a robustness case... depending on what's been done possibly by bootloader. What to you think ? > >> something like: set clk enable count when probing channels ? >> >> Apart from that, you can add my: >> Reviewed-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com> >> >> --- >> I've additional questions, unrelated to this patch. I had a look at >> pwm-stm32-lp.c, the clock enabling is done directly in the .get_state() >> routine. I think this should be moved to the .probe() routine as done >> here. There's likely a risk, that clk enable count gets increased >> artificially, at least since commit cfc4c189bc70 "pwm: Read initial >> hardware state at request time". >> Shall I send a patch for pwm-stm32-lp.c, similar as this patch ? Or has >> it been spotted already ? > > I'm not aware of someone working on stm32-lp, so feel free to prepare a > patch! Ok thanks! Will look into it. Best Regards, Fabrice > > Best regards and thanks for your review, > Uwe >
Hello Fabrice, On Wed, Nov 15, 2023 at 10:02:20AM +0100, Fabrice Gasnier wrote: > (with or without a Fixes tag) Could you add a check on global counter > enable bit (TIM_CR1_CEN) both in the .get_state(), and in the > stm32_pwm_detect_channels(), that counts the num_enabled channels ? I'd address that separately, mostly because the patches forwarded here are not mine. > In case the TIM_CR1_CEN isn't set (but some of the TIM_CCER_CCXE are), > the driver will report enabled channels. But physically, the pwm output > will be inactive. Huuu, that means if channel #0 is running and I start a capture on channel #1 which results in unconditionally calling regmap_clear_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN); (in stm32_pwm_raw_capture()) channel #0 stops to toggle! > That's more a robustness case... depending on what's been done possibly > by bootloader. What to you think ? I would assume that a bootloader that sets the CCXE bits also enables TIM_CR1_CEN and so in practise Philipp's patch is fine. But I agree that doing things properly would be better. Best regards Uwe
On 11/15/23 22:01, Uwe Kleine-König wrote: > Hello Fabrice, > > On Wed, Nov 15, 2023 at 10:02:20AM +0100, Fabrice Gasnier wrote: >> (with or without a Fixes tag) Could you add a check on global counter >> enable bit (TIM_CR1_CEN) both in the .get_state(), and in the >> stm32_pwm_detect_channels(), that counts the num_enabled channels ? > > I'd address that separately, mostly because the patches forwarded here > are not mine. Ok, let's go this way. > >> In case the TIM_CR1_CEN isn't set (but some of the TIM_CCER_CCXE are), >> the driver will report enabled channels. But physically, the pwm output >> will be inactive. > > Huuu, that means if channel #0 is running and I start a capture on > channel #1 which results in unconditionally calling > regmap_clear_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN); (in > stm32_pwm_raw_capture()) channel #0 stops to toggle! Basically no, please find my answers sent separately on "[PATCH] pwm: stm32: Mark capture support as broken" > >> That's more a robustness case... depending on what's been done possibly >> by bootloader. What to you think ? > > I would assume that a bootloader that sets the CCXE bits also enables > TIM_CR1_CEN and so in practise Philipp's patch is fine. But I agree that > doing things properly would be better. ok, thanks, Best Regards, Fabrice > > Best regards > Uwe >
diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c index 68239567a564..97a2c3c09b69 100644 --- a/drivers/pwm/pwm-stm32.c +++ b/drivers/pwm/pwm-stm32.c @@ -605,17 +605,21 @@ static void stm32_pwm_detect_complementary(struct stm32_pwm *priv) priv->have_complementary_output = (ccer != 0); } -static unsigned int stm32_pwm_detect_channels(struct stm32_pwm *priv) +static unsigned int stm32_pwm_detect_channels(struct stm32_pwm *priv, + unsigned int *num_enabled) { - u32 ccer; + u32 ccer, ccer_backup; /* * If channels enable bits don't exist writing 1 will have no * effect so we can detect and count them. */ + regmap_read(priv->regmap, TIM_CCER, &ccer_backup); regmap_set_bits(priv->regmap, TIM_CCER, TIM_CCER_CCXE); regmap_read(priv->regmap, TIM_CCER, &ccer); - regmap_clear_bits(priv->regmap, TIM_CCER, TIM_CCER_CCXE); + regmap_write(priv->regmap, TIM_CCER, ccer_backup); + + *num_enabled = hweight32(ccer_backup & TIM_CCER_CCXE); return hweight32(ccer & TIM_CCER_CCXE); } @@ -626,6 +630,8 @@ static int stm32_pwm_probe(struct platform_device *pdev) struct device_node *np = dev->of_node; struct stm32_timers *ddata = dev_get_drvdata(pdev->dev.parent); struct stm32_pwm *priv; + unsigned int num_enabled; + unsigned int i; int ret; priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); @@ -648,7 +654,11 @@ static int stm32_pwm_probe(struct platform_device *pdev) priv->chip.dev = dev; priv->chip.ops = &stm32pwm_ops; - priv->chip.npwm = stm32_pwm_detect_channels(priv); + priv->chip.npwm = stm32_pwm_detect_channels(priv, &num_enabled); + + /* Initialize clock refcount to number of enabled PWM channels. */ + for (i = 0; i < num_enabled; i++) + clk_enable(priv->clk); ret = devm_pwmchip_add(dev, &priv->chip); if (ret < 0)