[PATCH] pwm: stm32: Implement .get_state()

[PATCH] pwm: stm32: Implement .get_state()

[Philipp Zabel]

Stop stm32_pwm_detect_channels() from disabling all channels and count
the number of enabled PWMs to keep the clock running. Implement the
&pwm_ops->get_state callback so drivers can inherit PWM state set by
the bootloader.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
Make the necessary changes to allow inheriting PWM state set by the
bootloader, for example to avoid flickering with a pre-enabled PWM
backlight.
---
 drivers/pwm/pwm-stm32.c | 75 ++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 59 insertions(+), 16 deletions(-)

diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
index 62e397aeb9aa..e0677c954bdf 100644
--- a/drivers/pwm/pwm-stm32.c
+++ b/drivers/pwm/pwm-stm32.c
@@ -52,6 +52,21 @@ static u32 active_channels(struct stm32_pwm *dev)
 	return ccer & TIM_CCER_CCXE;
 }
 
+static int read_ccrx(struct stm32_pwm *dev, int ch, u32 *value)
+{
+	switch (ch) {
+	case 0:
+		return regmap_read(dev->regmap, TIM_CCR1, value);
+	case 1:
+		return regmap_read(dev->regmap, TIM_CCR2, value);
+	case 2:
+		return regmap_read(dev->regmap, TIM_CCR3, value);
+	case 3:
+		return regmap_read(dev->regmap, TIM_CCR4, value);
+	}
+	return -EINVAL;
+}
+
 static int write_ccrx(struct stm32_pwm *dev, int ch, u32 value)
 {
 	switch (ch) {
@@ -486,9 +501,40 @@ static int stm32_pwm_apply_locked(struct pwm_chip *chip, struct pwm_device *pwm,
 	return ret;
 }
 
+static int stm32_pwm_get_state(struct pwm_chip *chip,
+			       struct pwm_device *pwm, struct pwm_state *state)
+{
+	struct stm32_pwm *priv = to_stm32_pwm_dev(chip);
+	int ch = pwm->hwpwm;
+	unsigned long rate;
+	u32 ccer, psc, arr, ccr;
+	u64 dty, prd;
+	int ret;
+
+	ret = regmap_read(priv->regmap, TIM_CCER, &ccer);
+	if (ret)
+		return ret;
+
+	state->enabled = ccer & (TIM_CCER_CC1E << (ch * 4));
+	state->polarity = (ccer & (TIM_CCER_CC1P << (ch * 4))) ?
+			  PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;
+	regmap_read(priv->regmap, TIM_PSC, &psc);
+	regmap_read(priv->regmap, TIM_ARR, &arr);
+	read_ccrx(priv, ch, &ccr);
+	rate = clk_get_rate(priv->clk);
+
+	prd = (u64)NSEC_PER_SEC * (psc + 1) * (arr + 1);
+	state->period = DIV_ROUND_UP_ULL(prd, rate);
+	dty = (u64)NSEC_PER_SEC * (psc + 1) * ccr;
+	state->duty_cycle = DIV_ROUND_UP_ULL(dty, rate);
+
+	return ret;
+}
+
 static const struct pwm_ops stm32pwm_ops = {
 	.owner = THIS_MODULE,
 	.apply = stm32_pwm_apply_locked,
+	.get_state = stm32_pwm_get_state,
 	.capture = IS_ENABLED(CONFIG_DMA_ENGINE) ? stm32_pwm_capture : NULL,
 };
 
@@ -579,30 +625,22 @@ static void stm32_pwm_detect_complementary(struct stm32_pwm *priv)
 	priv->have_complementary_output = (ccer != 0);
 }
 
-static int stm32_pwm_detect_channels(struct stm32_pwm *priv)
+static int stm32_pwm_detect_channels(struct stm32_pwm *priv, int *n_enabled)
 {
-	u32 ccer;
-	int npwm = 0;
+	u32 ccer, ccer_backup;
+	int npwm;
 
 	/*
 	 * 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);
 
-	if (ccer & TIM_CCER_CC1E)
-		npwm++;
-
-	if (ccer & TIM_CCER_CC2E)
-		npwm++;
-
-	if (ccer & TIM_CCER_CC3E)
-		npwm++;
-
-	if (ccer & TIM_CCER_CC4E)
-		npwm++;
+	npwm = hweight32(ccer & TIM_CCER_CCXE);
+	*n_enabled = hweight32(ccer_backup & TIM_CCER_CCXE);
 
 	return npwm;
 }
@@ -613,7 +651,9 @@ 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;
+	int n_enabled;
 	int ret;
+	int i;
 
 	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
@@ -635,7 +675,10 @@ 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, &n_enabled);
+
+	for (i = 0; i < n_enabled; i++)
+		clk_enable(priv->clk);
 
 	ret = pwmchip_add(&priv->chip);
 	if (ret < 0)

---
base-commit: ac9a78681b921877518763ba0e89202254349d1b
change-id: 20230608-pwm-stm32-get-state-4107bcadd786

Best regards,
-- 
Philipp Zabel <p.zabel@pengutronix.de>

Re: [PATCH] pwm: stm32: Implement .get_state()

[Fabrice Gasnier]

Hi Philipp,

On 6/8/23 16:06, Philipp Zabel wrote:
> Stop stm32_pwm_detect_channels() from disabling all channels and count
> the number of enabled PWMs to keep the clock running. Implement the
> &pwm_ops->get_state callback so drivers can inherit PWM state set by
> the bootloader.
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
> Make the necessary changes to allow inheriting PWM state set by the
> bootloader, for example to avoid flickering with a pre-enabled PWM
> backlight.
> ---
>  drivers/pwm/pwm-stm32.c | 75 ++++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 59 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
> index 62e397aeb9aa..e0677c954bdf 100644
> --- a/drivers/pwm/pwm-stm32.c
> +++ b/drivers/pwm/pwm-stm32.c
> @@ -52,6 +52,21 @@ static u32 active_channels(struct stm32_pwm *dev)
>  	return ccer & TIM_CCER_CCXE;
>  }
>  
> +static int read_ccrx(struct stm32_pwm *dev, int ch, u32 *value)
> +{
> +	switch (ch) {
> +	case 0:
> +		return regmap_read(dev->regmap, TIM_CCR1, value);
> +	case 1:
> +		return regmap_read(dev->regmap, TIM_CCR2, value);
> +	case 2:
> +		return regmap_read(dev->regmap, TIM_CCR3, value);
> +	case 3:
> +		return regmap_read(dev->regmap, TIM_CCR4, value);
> +	}
> +	return -EINVAL;
> +}
> +
>  static int write_ccrx(struct stm32_pwm *dev, int ch, u32 value)
>  {
>  	switch (ch) {
> @@ -486,9 +501,40 @@ static int stm32_pwm_apply_locked(struct pwm_chip *chip, struct pwm_device *pwm,
>  	return ret;
>  }
>  
> +static int stm32_pwm_get_state(struct pwm_chip *chip,
> +			       struct pwm_device *pwm, struct pwm_state *state)
> +{
> +	struct stm32_pwm *priv = to_stm32_pwm_dev(chip);
> +	int ch = pwm->hwpwm;
> +	unsigned long rate;
> +	u32 ccer, psc, arr, ccr;
> +	u64 dty, prd;
> +	int ret;
> +
> +	ret = regmap_read(priv->regmap, TIM_CCER, &ccer);
> +	if (ret)
> +		return ret;
> +
> +	state->enabled = ccer & (TIM_CCER_CC1E << (ch * 4));
> +	state->polarity = (ccer & (TIM_CCER_CC1P << (ch * 4))) ?
> +			  PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;
> +	regmap_read(priv->regmap, TIM_PSC, &psc);
> +	regmap_read(priv->regmap, TIM_ARR, &arr);
> +	read_ccrx(priv, ch, &ccr);
> +	rate = clk_get_rate(priv->clk);
> +
> +	prd = (u64)NSEC_PER_SEC * (psc + 1) * (arr + 1);
> +	state->period = DIV_ROUND_UP_ULL(prd, rate);
> +	dty = (u64)NSEC_PER_SEC * (psc + 1) * ccr;
> +	state->duty_cycle = DIV_ROUND_UP_ULL(dty, rate);

Just a question/thought, could it be worth to use DIV_ROUND_CLOSEST_ULL() ?

> +
> +	return ret;
> +}
> +
>  static const struct pwm_ops stm32pwm_ops = {
>  	.owner = THIS_MODULE,
>  	.apply = stm32_pwm_apply_locked,
> +	.get_state = stm32_pwm_get_state,
>  	.capture = IS_ENABLED(CONFIG_DMA_ENGINE) ? stm32_pwm_capture : NULL,
>  };
>  
> @@ -579,30 +625,22 @@ static void stm32_pwm_detect_complementary(struct stm32_pwm *priv)
>  	priv->have_complementary_output = (ccer != 0);
>  }
>  
> -static int stm32_pwm_detect_channels(struct stm32_pwm *priv)
> +static int stm32_pwm_detect_channels(struct stm32_pwm *priv, int *n_enabled)
>  {
> -	u32 ccer;
> -	int npwm = 0;
> +	u32 ccer, ccer_backup;
> +	int npwm;
>  
>  	/*
>  	 * 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);
>  
> -	if (ccer & TIM_CCER_CC1E)
> -		npwm++;
> -
> -	if (ccer & TIM_CCER_CC2E)
> -		npwm++;
> -
> -	if (ccer & TIM_CCER_CC3E)
> -		npwm++;
> -
> -	if (ccer & TIM_CCER_CC4E)
> -		npwm++;
> +	npwm = hweight32(ccer & TIM_CCER_CCXE);
> +	*n_enabled = hweight32(ccer_backup & TIM_CCER_CCXE);
>  
>  	return npwm;
>  }
> @@ -613,7 +651,9 @@ 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;
> +	int n_enabled;
>  	int ret;
> +	int i;
>  
>  	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>  	if (!priv)
> @@ -635,7 +675,10 @@ 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, &n_enabled);
> +

I'd suggest to comment a bit here, to explain it initializes the PWM
counter clock refcount in sync with PWM initial state left by the
bootloader.

In all case, this is fine for me, you can add my:
Reviewed-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>

Best Regards,
Thanks,
Fabrice

> +	for (i = 0; i < n_enabled; i++)
> +		clk_enable(priv->clk);
>  
>  	ret = pwmchip_add(&priv->chip);
>  	if (ret < 0)
> 
> ---
> base-commit: ac9a78681b921877518763ba0e89202254349d1b
> change-id: 20230608-pwm-stm32-get-state-4107bcadd786
> 
> Best regards,

Re: [PATCH] pwm: stm32: Implement .get_state()

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1618 bytes --]

Hello Fabrice,

On Fri, Jun 09, 2023 at 03:06:47PM +0200, Fabrice Gasnier wrote:
> On 6/8/23 16:06, Philipp Zabel wrote:
> > +static int stm32_pwm_get_state(struct pwm_chip *chip,
> > +			       struct pwm_device *pwm, struct pwm_state *state)
> > +{
> > +	struct stm32_pwm *priv = to_stm32_pwm_dev(chip);
> > +	int ch = pwm->hwpwm;
> > +	unsigned long rate;
> > +	u32 ccer, psc, arr, ccr;
> > +	u64 dty, prd;
> > +	int ret;
> > +
> > +	ret = regmap_read(priv->regmap, TIM_CCER, &ccer);
> > +	if (ret)
> > +		return ret;
> > +
> > +	state->enabled = ccer & (TIM_CCER_CC1E << (ch * 4));
> > +	state->polarity = (ccer & (TIM_CCER_CC1P << (ch * 4))) ?
> > +			  PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;
> > +	regmap_read(priv->regmap, TIM_PSC, &psc);
> > +	regmap_read(priv->regmap, TIM_ARR, &arr);
> > +	read_ccrx(priv, ch, &ccr);
> > +	rate = clk_get_rate(priv->clk);
> > +
> > +	prd = (u64)NSEC_PER_SEC * (psc + 1) * (arr + 1);
> > +	state->period = DIV_ROUND_UP_ULL(prd, rate);
> > +	dty = (u64)NSEC_PER_SEC * (psc + 1) * ccr;
> > +	state->duty_cycle = DIV_ROUND_UP_ULL(dty, rate);
> 
> Just a question/thought, could it be worth to use DIV_ROUND_CLOSEST_ULL() ?

No, round up is the right choice. The reason for that is that .apply()
rounds down in its divisions. If you use ROUND_CLOSEST here, reapplying
the result from .get_state() might not be idempotent.

> > +
> > +	return ret;
> > +}

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] pwm: stm32: Implement .get_state()

[Philipp Zabel]

Hi Fabrice,

On Fr, 2023-06-09 at 15:06 +0200, Fabrice Gasnier wrote:
[...]
> > @@ -635,7 +675,10 @@ 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, &n_enabled);
> > +
> 
> I'd suggest to comment a bit here, to explain it initializes the PWM
> counter clock refcount in sync with PWM initial state left by the
> bootloader.
> 
> In all case, this is fine for me, you can add my:
> Reviewed-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>

Thank you, I'll add a comment here.

regards
Philipp

Re: [PATCH] pwm: stm32: Implement .get_state()

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 6648 bytes --]

On Thu, Jun 08, 2023 at 04:06:02PM +0200, Philipp Zabel wrote:
> Stop stm32_pwm_detect_channels() from disabling all channels and count
> the number of enabled PWMs to keep the clock running. Implement the
> &pwm_ops->get_state callback so drivers can inherit PWM state set by
> the bootloader.
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
> Make the necessary changes to allow inheriting PWM state set by the
> bootloader, for example to avoid flickering with a pre-enabled PWM
> backlight.
> ---
>  drivers/pwm/pwm-stm32.c | 75 ++++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 59 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
> index 62e397aeb9aa..e0677c954bdf 100644
> --- a/drivers/pwm/pwm-stm32.c
> +++ b/drivers/pwm/pwm-stm32.c
> @@ -52,6 +52,21 @@ static u32 active_channels(struct stm32_pwm *dev)
>  	return ccer & TIM_CCER_CCXE;
>  }
>  
> +static int read_ccrx(struct stm32_pwm *dev, int ch, u32 *value)
> +{
> +	switch (ch) {
> +	case 0:
> +		return regmap_read(dev->regmap, TIM_CCR1, value);
> +	case 1:
> +		return regmap_read(dev->regmap, TIM_CCR2, value);
> +	case 2:
> +		return regmap_read(dev->regmap, TIM_CCR3, value);
> +	case 3:
> +		return regmap_read(dev->regmap, TIM_CCR4, value);
> +	}
> +	return -EINVAL;
> +}

I'd prefer having something like:

	#define TIM_CCR(i)	(0x30 + 4 * (i))	/* Capt/Comp Register i, for i in 1 .. 4 */
	#define TIM_CCR1	TIM_CCR(1)
	#define TIM_CCR2	TIM_CCR(2)
	#define TIM_CCR3	TIM_CCR(3)
	#define TIM_CCR4	TIM_CCR(4)

and then read_ccrx could be simplified to:

	return regmap_read(dev->regmap, TIM_CCR(i + 1), value);

. (Not sure if passing an invalid channel really needs handling.)

But given that write_ccrx looks the same, I'm ok with that.

> +
>  static int write_ccrx(struct stm32_pwm *dev, int ch, u32 value)
>  {
>  	switch (ch) {
> @@ -486,9 +501,40 @@ static int stm32_pwm_apply_locked(struct pwm_chip *chip, struct pwm_device *pwm,
>  	return ret;
>  }
>  
> +static int stm32_pwm_get_state(struct pwm_chip *chip,
> +			       struct pwm_device *pwm, struct pwm_state *state)
> +{
> +	struct stm32_pwm *priv = to_stm32_pwm_dev(chip);
> +	int ch = pwm->hwpwm;
> +	unsigned long rate;
> +	u32 ccer, psc, arr, ccr;
> +	u64 dty, prd;
> +	int ret;
> +
> +	ret = regmap_read(priv->regmap, TIM_CCER, &ccer);
> +	if (ret)
> +		return ret;
> +
> +	state->enabled = ccer & (TIM_CCER_CC1E << (ch * 4));

Other parts of the driver use the macros from <linux/bitfield.h>. With a
similar approach as suggested for TIM_CCR above, this could be made to
read as:

	state->enabled = FIELD_GET(TIM_CCER_CCxE(ch + 1), ccer);

> +	state->polarity = (ccer & (TIM_CCER_CC1P << (ch * 4))) ?
> +			  PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;
> +	regmap_read(priv->regmap, TIM_PSC, &psc);
> +	regmap_read(priv->regmap, TIM_ARR, &arr);
> +	read_ccrx(priv, ch, &ccr);

You don't use the return value of read_ccrx(), so make it void (or check
it)? If you check it, also do it for regmap_read().

> +	rate = clk_get_rate(priv->clk);
> +
> +	prd = (u64)NSEC_PER_SEC * (psc + 1) * (arr + 1);

This might overflow?!

> +	state->period = DIV_ROUND_UP_ULL(prd, rate);
> +	dty = (u64)NSEC_PER_SEC * (psc + 1) * ccr;
> +	state->duty_cycle = DIV_ROUND_UP_ULL(dty, rate);
> +
> +	return ret;
> +}

While looking at stm32_pwm_config() to check if it matches your
stm32_pwm_get_state() implementation, I noticed that for small values of
period_ns, prd might become 0 which than yields surprising effects in
combination with

	regmap_write(priv->regmap, TIM_ARR, prd - 1);

Also the driver needs locking because of the PSC and ARR registers are
shared for all channels and there are rounding issues (prd is used for
the calculation of dty).

> +
>  static const struct pwm_ops stm32pwm_ops = {
>  	.owner = THIS_MODULE,
>  	.apply = stm32_pwm_apply_locked,
> +	.get_state = stm32_pwm_get_state,
>  	.capture = IS_ENABLED(CONFIG_DMA_ENGINE) ? stm32_pwm_capture : NULL,
>  };
>  
> @@ -579,30 +625,22 @@ static void stm32_pwm_detect_complementary(struct stm32_pwm *priv)
>  	priv->have_complementary_output = (ccer != 0);
>  }
>  
> -static int stm32_pwm_detect_channels(struct stm32_pwm *priv)
> +static int stm32_pwm_detect_channels(struct stm32_pwm *priv, int *n_enabled)
>  {
> -	u32 ccer;
> -	int npwm = 0;
> +	u32 ccer, ccer_backup;
> +	int npwm;
>  
>  	/*
>  	 * 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);
>  
> -	if (ccer & TIM_CCER_CC1E)
> -		npwm++;
> -
> -	if (ccer & TIM_CCER_CC2E)
> -		npwm++;
> -
> -	if (ccer & TIM_CCER_CC3E)
> -		npwm++;
> -
> -	if (ccer & TIM_CCER_CC4E)
> -		npwm++;
> +	npwm = hweight32(ccer & TIM_CCER_CCXE);
> +	*n_enabled = hweight32(ccer_backup & TIM_CCER_CCXE);

hweight32 returns an unsigned int. While there is probably no problem
with values that don't fit, using unsigned for *n_enabled (and also
npwm) looks better IMHO. Maybe split making npwm unsigned and using
hweight32 to assign it to a separate preparing patch? The inconsistency
between "npwm" (without underscore) and "n_enabled" (with underscore)
strikes me a bit. given that struct pwm_chip has "npwm", too, maybe drop
the _ from "n_enabled"?

>  	return npwm;
>  }
> @@ -613,7 +651,9 @@ 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;
> +	int n_enabled;
>  	int ret;
> +	int i;
>  
>  	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>  	if (!priv)
> @@ -635,7 +675,10 @@ 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, &n_enabled);
> +
> +	for (i = 0; i < n_enabled; i++)
> +		clk_enable(priv->clk);
>  
>  	ret = pwmchip_add(&priv->chip);
>  	if (ret < 0)
> 

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] pwm: stm32: Implement .get_state()

[Philipp Zabel]

Hi Uwe,

On Mi, 2023-06-14 at 09:33 +0200, Uwe Kleine-König wrote:
> On Thu, Jun 08, 2023 at 04:06:02PM +0200, Philipp Zabel wrote:
> > Stop stm32_pwm_detect_channels() from disabling all channels and count
> > the number of enabled PWMs to keep the clock running. Implement the
> > &pwm_ops->get_state callback so drivers can inherit PWM state set by
> > the bootloader.
> > 
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > ---
> > Make the necessary changes to allow inheriting PWM state set by the
> > bootloader, for example to avoid flickering with a pre-enabled PWM
> > backlight.
> > ---
> >  drivers/pwm/pwm-stm32.c | 75 ++++++++++++++++++++++++++++++++++++++-----------
> >  1 file changed, 59 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
> > index 62e397aeb9aa..e0677c954bdf 100644
> > --- a/drivers/pwm/pwm-stm32.c
> > +++ b/drivers/pwm/pwm-stm32.c
> > @@ -52,6 +52,21 @@ static u32 active_channels(struct stm32_pwm *dev)
> >  	return ccer & TIM_CCER_CCXE;
> >  }
> >  
> > +static int read_ccrx(struct stm32_pwm *dev, int ch, u32 *value)
> > +{
> > +	switch (ch) {
> > +	case 0:
> > +		return regmap_read(dev->regmap, TIM_CCR1, value);
> > +	case 1:
> > +		return regmap_read(dev->regmap, TIM_CCR2, value);
> > +	case 2:
> > +		return regmap_read(dev->regmap, TIM_CCR3, value);
> > +	case 3:
> > +		return regmap_read(dev->regmap, TIM_CCR4, value);
> > +	}
> > +	return -EINVAL;
> > +}
> 
> I'd prefer having something like:
> 
> 	#define TIM_CCR(i)	(0x30 + 4 * (i))	/* Capt/Comp Register i, for i in 1 .. 4 */
> 	#define TIM_CCR1	TIM_CCR(1)
> 	#define TIM_CCR2	TIM_CCR(2)
> 	#define TIM_CCR3	TIM_CCR(3)
> 	#define TIM_CCR4	TIM_CCR(4)

I find this a bit confusing due to the 1-based register numbering. For
example, 0x30 is not one of the CCR registers at all.
When TIM_CCR(i) is used in the code, it's not evident that the valid
range is 1...4.

I'd prefer to leave this as is ...

> and then read_ccrx could be simplified to:
> 
> 	return regmap_read(dev->regmap, TIM_CCR(i + 1), value);

... and just turn this into

	regmap_read(regmap, TIM_CCR1 + 4 * ch, value);

> . (Not sure if passing an invalid channel really needs handling.)

I think it is not necessary.

ch is set to pwm->hwpwm, which is < pwm->npwm, which is <= 4.

> But given that write_ccrx looks the same, I'm ok with that.

I'd like to drop read/write_ccrx altogether and replace them with a
single regmap_read/write.

> > +
> >  static int write_ccrx(struct stm32_pwm *dev, int ch, u32 value)
> >  {
> >  	switch (ch) {
> > @@ -486,9 +501,40 @@ static int stm32_pwm_apply_locked(struct pwm_chip *chip, struct pwm_device *pwm,
> >  	return ret;
> >  }
> >  
> > +static int stm32_pwm_get_state(struct pwm_chip *chip,
> > +			       struct pwm_device *pwm, struct pwm_state *state)
> > +{
> > +	struct stm32_pwm *priv = to_stm32_pwm_dev(chip);
> > +	int ch = pwm->hwpwm;
> > +	unsigned long rate;
> > +	u32 ccer, psc, arr, ccr;
> > +	u64 dty, prd;
> > +	int ret;
> > +
> > +	ret = regmap_read(priv->regmap, TIM_CCER, &ccer);
> > +	if (ret)
> > +		return ret;
> > +
> > +	state->enabled = ccer & (TIM_CCER_CC1E << (ch * 4));
> 
> Other parts of the driver use the macros from <linux/bitfield.h>. With a
> similar approach as suggested for TIM_CCR above, this could be made to
> read as:
> 
> 	state->enabled = FIELD_GET(TIM_CCER_CCxE(ch + 1), ccer);

Again this feels like an unnecessary indirection to me. I think it
doesn't work like this either, the mask has to be a compile time
constant.

There's already a few examples of the (TIM_CCER_CC1E << (ch * 4))
pattern in the driver. If they must be converted to something else, I'd
prefer this to be done separately, for all of them.

> > +	state->polarity = (ccer & (TIM_CCER_CC1P << (ch * 4))) ?
> > +			  PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;
> > +	regmap_read(priv->regmap, TIM_PSC, &psc);
> > +	regmap_read(priv->regmap, TIM_ARR, &arr);
> > +	read_ccrx(priv, ch, &ccr);
> 
> You don't use the return value of read_ccrx(), so make it void (or check
> it)? If you check it, also do it for regmap_read().

Yes, thanks.

> > +	rate = clk_get_rate(priv->clk);
> > +
> > +	prd = (u64)NSEC_PER_SEC * (psc + 1) * (arr + 1);
> 
> This might overflow?!

In practice this can't happen because PSC, ARR, and CCRx fields are
only 16-bit.
Although I'm not sure whether this will be true for future designs.

> > +	state->period = DIV_ROUND_UP_ULL(prd, rate);
> > +	dty = (u64)NSEC_PER_SEC * (psc + 1) * ccr;
> > +	state->duty_cycle = DIV_ROUND_UP_ULL(dty, rate);
> > +
> > +	return ret;
> > +}
> 
> While looking at stm32_pwm_config() to check if it matches your
> stm32_pwm_get_state() implementation, I noticed that for small values of
> period_ns, prd might become 0 which than yields surprising effects in
> combination with
> 
> 	regmap_write(priv->regmap, TIM_ARR, prd - 1);

What to do about this, "prd = max(1, div);"?
This should probably be fixed separately.

> Also the driver needs locking because of the PSC and ARR registers are
> shared for all channels

I'll lock priv->lock in get_state.

> and there are rounding issues (prd is used for
> the calculation of dty).

This should be fixed separately as well.

> > +
> >  static const struct pwm_ops stm32pwm_ops = {
> >  	.owner = THIS_MODULE,
> >  	.apply = stm32_pwm_apply_locked,
> > +	.get_state = stm32_pwm_get_state,
> >  	.capture = IS_ENABLED(CONFIG_DMA_ENGINE) ? stm32_pwm_capture : NULL,
> >  };
> >  
> > @@ -579,30 +625,22 @@ static void stm32_pwm_detect_complementary(struct stm32_pwm *priv)
> >  	priv->have_complementary_output = (ccer != 0);
> >  }
> >  
> > -static int stm32_pwm_detect_channels(struct stm32_pwm *priv)
> > +static int stm32_pwm_detect_channels(struct stm32_pwm *priv, int *n_enabled)
> >  {
> > -	u32 ccer;
> > -	int npwm = 0;
> > +	u32 ccer, ccer_backup;
> > +	int npwm;
> >  
> >  	/*
> >  	 * 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);
> >  
> > -	if (ccer & TIM_CCER_CC1E)
> > -		npwm++;
> > -
> > -	if (ccer & TIM_CCER_CC2E)
> > -		npwm++;
> > -
> > -	if (ccer & TIM_CCER_CC3E)
> > -		npwm++;
> > -
> > -	if (ccer & TIM_CCER_CC4E)
> > -		npwm++;
> > +	npwm = hweight32(ccer & TIM_CCER_CCXE);
> > +	*n_enabled = hweight32(ccer_backup & TIM_CCER_CCXE);
> 
> hweight32 returns an unsigned int. While there is probably no problem
> with values that don't fit, using unsigned for *n_enabled (and also
> npwm) looks better IMHO. Maybe split making npwm unsigned and using
> hweight32 to assign it to a separate preparing patch?

Yes, I'll do that.

> The inconsistency
> between "npwm" (without underscore) and "n_enabled" (with underscore)
> strikes me a bit. given that struct pwm_chip has "npwm", too, maybe drop
> the _ from "n_enabled"?

I can't properly read "nenabled", I'll turn it into "num_enabled"
(there's precedence with "priv->num_breakinputs") and drop "npwm"
altogether, as the value can be returned directly.

> 
regards
Philipp

Re: [PATCH] pwm: stm32: Implement .get_state()

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3780 bytes --]

On Thu, Jun 08, 2023 at 04:06:02PM +0200, Philipp Zabel wrote:
> Stop stm32_pwm_detect_channels() from disabling all channels and count
> the number of enabled PWMs to keep the clock running. Implement the
> &pwm_ops->get_state callback so drivers can inherit PWM state set by
> the bootloader.
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
> Make the necessary changes to allow inheriting PWM state set by the
> bootloader, for example to avoid flickering with a pre-enabled PWM
> backlight.
> ---
>  drivers/pwm/pwm-stm32.c | 75 ++++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 59 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
> index 62e397aeb9aa..e0677c954bdf 100644
> --- a/drivers/pwm/pwm-stm32.c
> +++ b/drivers/pwm/pwm-stm32.c
> @@ -52,6 +52,21 @@ static u32 active_channels(struct stm32_pwm *dev)
>  	return ccer & TIM_CCER_CCXE;
>  }
>  
> +static int read_ccrx(struct stm32_pwm *dev, int ch, u32 *value)
> +{
> +	switch (ch) {
> +	case 0:
> +		return regmap_read(dev->regmap, TIM_CCR1, value);
> +	case 1:
> +		return regmap_read(dev->regmap, TIM_CCR2, value);
> +	case 2:
> +		return regmap_read(dev->regmap, TIM_CCR3, value);
> +	case 3:
> +		return regmap_read(dev->regmap, TIM_CCR4, value);
> +	}
> +	return -EINVAL;
> +}

Looking at the register definitions we should be able to replace this
with a single line and parameterize based on channel.

I realize you probably just copied from write_ccrx(), but might as well
improve this while at it. Could be a separate patch, though.

Also, ch should be unsigned int since it comes from pwm->hwpwm.

> +
>  static int write_ccrx(struct stm32_pwm *dev, int ch, u32 value)
>  {
>  	switch (ch) {
> @@ -486,9 +501,40 @@ static int stm32_pwm_apply_locked(struct pwm_chip *chip, struct pwm_device *pwm,
>  	return ret;
>  }
>  
> +static int stm32_pwm_get_state(struct pwm_chip *chip,
> +			       struct pwm_device *pwm, struct pwm_state *state)
> +{
> +	struct stm32_pwm *priv = to_stm32_pwm_dev(chip);
> +	int ch = pwm->hwpwm;

This should reflect the type of pwm->hwpwm.

> +	unsigned long rate;
> +	u32 ccer, psc, arr, ccr;
> +	u64 dty, prd;
> +	int ret;
> +
> +	ret = regmap_read(priv->regmap, TIM_CCER, &ccer);
> +	if (ret)
> +		return ret;
> +
> +	state->enabled = ccer & (TIM_CCER_CC1E << (ch * 4));
> +	state->polarity = (ccer & (TIM_CCER_CC1P << (ch * 4))) ?
> +			  PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;
> +	regmap_read(priv->regmap, TIM_PSC, &psc);
> +	regmap_read(priv->regmap, TIM_ARR, &arr);

We should probably check regmap_read() consistently here.

> +	read_ccrx(priv, ch, &ccr);
> +	rate = clk_get_rate(priv->clk);
> +
> +	prd = (u64)NSEC_PER_SEC * (psc + 1) * (arr + 1);
> +	state->period = DIV_ROUND_UP_ULL(prd, rate);
> +	dty = (u64)NSEC_PER_SEC * (psc + 1) * ccr;
> +	state->duty_cycle = DIV_ROUND_UP_ULL(dty, rate);
> +
> +	return ret;
> +}
> +
>  static const struct pwm_ops stm32pwm_ops = {
>  	.owner = THIS_MODULE,
>  	.apply = stm32_pwm_apply_locked,
> +	.get_state = stm32_pwm_get_state,
>  	.capture = IS_ENABLED(CONFIG_DMA_ENGINE) ? stm32_pwm_capture : NULL,
>  };
>  
> @@ -579,30 +625,22 @@ static void stm32_pwm_detect_complementary(struct stm32_pwm *priv)
>  	priv->have_complementary_output = (ccer != 0);
>  }
>  
> -static int stm32_pwm_detect_channels(struct stm32_pwm *priv)
> +static int stm32_pwm_detect_channels(struct stm32_pwm *priv, int *n_enabled)

unsigned int * for n_enabled.

>  {
> -	u32 ccer;
> -	int npwm = 0;
> +	u32 ccer, ccer_backup;
> +	int npwm;

Also make this and the return value unsigned int while at it. These can
never be negative.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] pwm: stm32: Implement .get_state()

[Philipp Zabel]

Hi Thierry,

On Di, 2023-07-18 at 09:30 +0200, Thierry Reding wrote:
> On Thu, Jun 08, 2023 at 04:06:02PM +0200, Philipp Zabel wrote:
> > Stop stm32_pwm_detect_channels() from disabling all channels and count
> > the number of enabled PWMs to keep the clock running. Implement the
> > &pwm_ops->get_state callback so drivers can inherit PWM state set by
> > the bootloader.
> > 
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > ---
> > Make the necessary changes to allow inheriting PWM state set by the
> > bootloader, for example to avoid flickering with a pre-enabled PWM
> > backlight.
> > ---
> >  drivers/pwm/pwm-stm32.c | 75 ++++++++++++++++++++++++++++++++++++++-----------
> >  1 file changed, 59 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
> > index 62e397aeb9aa..e0677c954bdf 100644
> > --- a/drivers/pwm/pwm-stm32.c
> > +++ b/drivers/pwm/pwm-stm32.c
> > @@ -52,6 +52,21 @@ static u32 active_channels(struct stm32_pwm *dev)
> >  	return ccer & TIM_CCER_CCXE;
> >  }
> >  
> > +static int read_ccrx(struct stm32_pwm *dev, int ch, u32 *value)
> > +{
> > +	switch (ch) {
> > +	case 0:
> > +		return regmap_read(dev->regmap, TIM_CCR1, value);
> > +	case 1:
> > +		return regmap_read(dev->regmap, TIM_CCR2, value);
> > +	case 2:
> > +		return regmap_read(dev->regmap, TIM_CCR3, value);
> > +	case 3:
> > +		return regmap_read(dev->regmap, TIM_CCR4, value);
> > +	}
> > +	return -EINVAL;
> > +}
> 
> Looking at the register definitions we should be able to replace this
> with a single line and parameterize based on channel.
> 
> I realize you probably just copied from write_ccrx(), but might as well
> improve this while at it. Could be a separate patch, though.
> 
> Also, ch should be unsigned int since it comes from pwm->hwpwm.

Thank you, I'll make both changes separately.

> >  static int write_ccrx(struct stm32_pwm *dev, int ch, u32 value)
> >  {
> >  	switch (ch) {
> > @@ -486,9 +501,40 @@ static int stm32_pwm_apply_locked(struct pwm_chip *chip, struct pwm_device *pwm,
> >  	return ret;
> >  }
> >  
> > +static int stm32_pwm_get_state(struct pwm_chip *chip,
> > +			       struct pwm_device *pwm, struct pwm_state *state)
> > +{
> > +	struct stm32_pwm *priv = to_stm32_pwm_dev(chip);
> > +	int ch = pwm->hwpwm;
> 
> This should reflect the type of pwm->hwpwm.

Ok.

> > +	unsigned long rate;
> > +	u32 ccer, psc, arr, ccr;
> > +	u64 dty, prd;
> > +	int ret;
> > +
> > +	ret = regmap_read(priv->regmap, TIM_CCER, &ccer);
> > +	if (ret)
> > +		return ret;
> > +
> > +	state->enabled = ccer & (TIM_CCER_CC1E << (ch * 4));
> > +	state->polarity = (ccer & (TIM_CCER_CC1P << (ch * 4))) ?
> > +			  PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;
> > +	regmap_read(priv->regmap, TIM_PSC, &psc);
> > +	regmap_read(priv->regmap, TIM_ARR, &arr);
> 
> We should probably check regmap_read() consistently here.

Will do.

I'll also add locking so we can't PSC/ARR/CCRx in an in-between state.

> > +	read_ccrx(priv, ch, &ccr);
> > +	rate = clk_get_rate(priv->clk);
> > +
> > +	prd = (u64)NSEC_PER_SEC * (psc + 1) * (arr + 1);
> > +	state->period = DIV_ROUND_UP_ULL(prd, rate);
> > +	dty = (u64)NSEC_PER_SEC * (psc + 1) * ccr;
> > +	state->duty_cycle = DIV_ROUND_UP_ULL(dty, rate);
> > +
> > +	return ret;
> > +}
> > +
> >  static const struct pwm_ops stm32pwm_ops = {
> >  	.owner = THIS_MODULE,
> >  	.apply = stm32_pwm_apply_locked,
> > +	.get_state = stm32_pwm_get_state,
> >  	.capture = IS_ENABLED(CONFIG_DMA_ENGINE) ? stm32_pwm_capture : NULL,
> >  };
> >  
> > @@ -579,30 +625,22 @@ static void stm32_pwm_detect_complementary(struct stm32_pwm *priv)
> >  	priv->have_complementary_output = (ccer != 0);
> >  }
> >  
> > -static int stm32_pwm_detect_channels(struct stm32_pwm *priv)
> > +static int stm32_pwm_detect_channels(struct stm32_pwm *priv, int *n_enabled)
> 
> unsigned int * for n_enabled.

Ok.

> >  {
> > -	u32 ccer;
> > -	int npwm = 0;
> > +	u32 ccer, ccer_backup;
> > +	int npwm;
> 
> Also make this and the return value unsigned int while at it. These can
> never be negative.

Thanks, I'll split this out into a separate patch.

regards
Philipp