[PATCH 0/2] pwm: brcmstb: Some cleanups

[PATCH 0/2] pwm: brcmstb: Some cleanups

[Uwe Kleine-König]

Hello,

here are a few cleanups for the brcmstb PWM driver. There are a few
issues left with it, that I'm not addressing for now. Just mention it in
case someone wants to work on this driver:

 - There is no .get_state() callback
   (That needs to be implemented by some with hardware and
   documentation)

 - There are a few places where an overflow can happen in
   brcmstb_pwm_config() that are not handled

 - The loop in brcmstb_pwm_config() to calculate cword is ineffective,
   cword could be calculated ad hoc.

 - I don't understand

                /*
                 * We can be called with separate duty and period updates,
                 * so do not reject dc == 0 right away
                 */
                if (pc == PWM_PERIOD_MIN || (dc < PWM_ON_MIN && duty_ns))
                        return -EINVAL;

   The usual policy is "With the selected period, pick the biggest
   possible duty_cycle that isn't bigger thatn the requested duty_cycle.
   So should this case be handled using dc = 0 instead?
   But as I don't understand the real issue here (is this about changing
   period and duty at the same time?), I don't want to touch that.

 - The driver uses SIMPLE_DEV_PM_OPS which is deprecated.

 - The driver defines pr_fmt(fmt) but doesn't use it.

Uwe Kleine-König (2):
  pwm: brcmstb: Implement .apply() callback
  pwm: brcmstb: Remove useless locking

 drivers/pwm/pwm-brcmstb.c | 52 ++++++++++++++++++---------------------
 1 file changed, 24 insertions(+), 28 deletions(-)


base-commit: 657e54e54ba5b3859848e0ea78d6384ebb7479d6
-- 
2.34.1

[PATCH 0/2] pwm: brcmstb: Some cleanups

[Uwe Kleine-König]

Hello,

here are a few cleanups for the brcmstb PWM driver. There are a few
issues left with it, that I'm not addressing for now. Just mention it in
case someone wants to work on this driver:

 - There is no .get_state() callback
   (That needs to be implemented by some with hardware and
   documentation)

 - There are a few places where an overflow can happen in
   brcmstb_pwm_config() that are not handled

 - The loop in brcmstb_pwm_config() to calculate cword is ineffective,
   cword could be calculated ad hoc.

 - I don't understand

                /*
                 * We can be called with separate duty and period updates,
                 * so do not reject dc == 0 right away
                 */
                if (pc == PWM_PERIOD_MIN || (dc < PWM_ON_MIN && duty_ns))
                        return -EINVAL;

   The usual policy is "With the selected period, pick the biggest
   possible duty_cycle that isn't bigger thatn the requested duty_cycle.
   So should this case be handled using dc = 0 instead?
   But as I don't understand the real issue here (is this about changing
   period and duty at the same time?), I don't want to touch that.

 - The driver uses SIMPLE_DEV_PM_OPS which is deprecated.

 - The driver defines pr_fmt(fmt) but doesn't use it.

Uwe Kleine-König (2):
  pwm: brcmstb: Implement .apply() callback
  pwm: brcmstb: Remove useless locking

 drivers/pwm/pwm-brcmstb.c | 52 ++++++++++++++++++---------------------
 1 file changed, 24 insertions(+), 28 deletions(-)


base-commit: 657e54e54ba5b3859848e0ea78d6384ebb7479d6
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/2] pwm: brcmstb: Implement .apply() callback

[Uwe Kleine-König]

To eventually get rid of all legacy drivers convert this driver to the
modern world implementing .apply().

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/pwm-brcmstb.c | 45 +++++++++++++++++++++------------------
 1 file changed, 24 insertions(+), 21 deletions(-)

diff --git a/drivers/pwm/pwm-brcmstb.c b/drivers/pwm/pwm-brcmstb.c
index 3b529f82b97c..99974390aa38 100644
--- a/drivers/pwm/pwm-brcmstb.c
+++ b/drivers/pwm/pwm-brcmstb.c
@@ -95,7 +95,7 @@ static inline struct brcmstb_pwm *to_brcmstb_pwm(struct pwm_chip *chip)
  * "on" time, so this translates directly into our HW programming here.
  */
 static int brcmstb_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
-			      int duty_ns, int period_ns)
+			      u64 duty_ns, u64 period_ns)
 {
 	struct brcmstb_pwm *p = to_brcmstb_pwm(chip);
 	unsigned long pc, dc, cword = CONST_VAR_F_MAX;
@@ -114,22 +114,17 @@ static int brcmstb_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	}
 
 	while (1) {
-		u64 rate, tmp;
+		u64 rate;
 
 		/*
 		 * Calculate the base rate from base frequency and current
 		 * cword
 		 */
 		rate = (u64)clk_get_rate(p->clk) * (u64)cword;
-		do_div(rate, 1 << CWORD_BIT_SIZE);
+		rate >>= CWORD_BIT_SIZE;
 
-		tmp = period_ns * rate;
-		do_div(tmp, NSEC_PER_SEC);
-		pc = tmp;
-
-		tmp = (duty_ns + 1) * rate;
-		do_div(tmp, NSEC_PER_SEC);
-		dc = tmp;
+		pc = mul_u64_u64_div_u64(period_ns, rate, NSEC_PER_SEC);
+		dc = mul_u64_u64_div_u64(duty_ns + 1, rate, NSEC_PER_SEC);
 
 		/*
 		 * We can be called with separate duty and period updates,
@@ -202,26 +197,34 @@ static inline void brcmstb_pwm_enable_set(struct brcmstb_pwm *p,
 	spin_unlock(&p->lock);
 }
 
-static int brcmstb_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+static int brcmstb_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+			     const struct pwm_state *state)
 {
 	struct brcmstb_pwm *p = to_brcmstb_pwm(chip);
+	int err;
 
-	brcmstb_pwm_enable_set(p, pwm->hwpwm, true);
+	if (state->polarity != PWM_POLARITY_NORMAL)
+		return -EINVAL;
 
-	return 0;
-}
+	if (!state->enabled) {
+		if (pwm->state.enabled)
+			brcmstb_pwm_enable_set(p, pwm->hwpwm, false);
 
-static void brcmstb_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
-{
-	struct brcmstb_pwm *p = to_brcmstb_pwm(chip);
+		return 0;
+	}
+
+	err = brcmstb_pwm_config(chip, pwm, state->duty_cycle, state->period);
+	if (err)
+		return err;
 
-	brcmstb_pwm_enable_set(p, pwm->hwpwm, false);
+	if (!pwm->state.enabled)
+		brcmstb_pwm_enable_set(p, pwm->hwpwm, true);
+
+	return 0;
 }
 
 static const struct pwm_ops brcmstb_pwm_ops = {
-	.config = brcmstb_pwm_config,
-	.enable = brcmstb_pwm_enable,
-	.disable = brcmstb_pwm_disable,
+	.apply = brcmstb_pwm_apply,
 	.owner = THIS_MODULE,
 };
 
-- 
2.34.1

[PATCH 1/2] pwm: brcmstb: Implement .apply() callback

[Uwe Kleine-König]

To eventually get rid of all legacy drivers convert this driver to the
modern world implementing .apply().

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/pwm-brcmstb.c | 45 +++++++++++++++++++++------------------
 1 file changed, 24 insertions(+), 21 deletions(-)

diff --git a/drivers/pwm/pwm-brcmstb.c b/drivers/pwm/pwm-brcmstb.c
index 3b529f82b97c..99974390aa38 100644
--- a/drivers/pwm/pwm-brcmstb.c
+++ b/drivers/pwm/pwm-brcmstb.c
@@ -95,7 +95,7 @@ static inline struct brcmstb_pwm *to_brcmstb_pwm(struct pwm_chip *chip)
  * "on" time, so this translates directly into our HW programming here.
  */
 static int brcmstb_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
-			      int duty_ns, int period_ns)
+			      u64 duty_ns, u64 period_ns)
 {
 	struct brcmstb_pwm *p = to_brcmstb_pwm(chip);
 	unsigned long pc, dc, cword = CONST_VAR_F_MAX;
@@ -114,22 +114,17 @@ static int brcmstb_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	}
 
 	while (1) {
-		u64 rate, tmp;
+		u64 rate;
 
 		/*
 		 * Calculate the base rate from base frequency and current
 		 * cword
 		 */
 		rate = (u64)clk_get_rate(p->clk) * (u64)cword;
-		do_div(rate, 1 << CWORD_BIT_SIZE);
+		rate >>= CWORD_BIT_SIZE;
 
-		tmp = period_ns * rate;
-		do_div(tmp, NSEC_PER_SEC);
-		pc = tmp;
-
-		tmp = (duty_ns + 1) * rate;
-		do_div(tmp, NSEC_PER_SEC);
-		dc = tmp;
+		pc = mul_u64_u64_div_u64(period_ns, rate, NSEC_PER_SEC);
+		dc = mul_u64_u64_div_u64(duty_ns + 1, rate, NSEC_PER_SEC);
 
 		/*
 		 * We can be called with separate duty and period updates,
@@ -202,26 +197,34 @@ static inline void brcmstb_pwm_enable_set(struct brcmstb_pwm *p,
 	spin_unlock(&p->lock);
 }
 
-static int brcmstb_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+static int brcmstb_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+			     const struct pwm_state *state)
 {
 	struct brcmstb_pwm *p = to_brcmstb_pwm(chip);
+	int err;
 
-	brcmstb_pwm_enable_set(p, pwm->hwpwm, true);
+	if (state->polarity != PWM_POLARITY_NORMAL)
+		return -EINVAL;
 
-	return 0;
-}
+	if (!state->enabled) {
+		if (pwm->state.enabled)
+			brcmstb_pwm_enable_set(p, pwm->hwpwm, false);
 
-static void brcmstb_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
-{
-	struct brcmstb_pwm *p = to_brcmstb_pwm(chip);
+		return 0;
+	}
+
+	err = brcmstb_pwm_config(chip, pwm, state->duty_cycle, state->period);
+	if (err)
+		return err;
 
-	brcmstb_pwm_enable_set(p, pwm->hwpwm, false);
+	if (!pwm->state.enabled)
+		brcmstb_pwm_enable_set(p, pwm->hwpwm, true);
+
+	return 0;
 }
 
 static const struct pwm_ops brcmstb_pwm_ops = {
-	.config = brcmstb_pwm_config,
-	.enable = brcmstb_pwm_enable,
-	.disable = brcmstb_pwm_disable,
+	.apply = brcmstb_pwm_apply,
 	.owner = THIS_MODULE,
 };
 
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/2] pwm: brcmstb: Remove useless locking

[Uwe Kleine-König]

The lock only protects against concurrent users of the PWM API. This is not
expected to be necessary. And if there was such an issue, this is better
handled in the pwm core instead as it affects all drivers in the same way.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/pwm-brcmstb.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/pwm/pwm-brcmstb.c b/drivers/pwm/pwm-brcmstb.c
index 99974390aa38..3db3f96edf78 100644
--- a/drivers/pwm/pwm-brcmstb.c
+++ b/drivers/pwm/pwm-brcmstb.c
@@ -53,7 +53,6 @@
 
 struct brcmstb_pwm {
 	void __iomem *base;
-	spinlock_t lock;
 	struct clk *clk;
 	struct pwm_chip chip;
 };
@@ -159,7 +158,6 @@ static int brcmstb_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	 * generator output a base frequency for the constant frequency
 	 * generator to derive from.
 	 */
-	spin_lock(&p->lock);
 	brcmstb_pwm_writel(p, cword >> 8, PWM_CWORD_MSB(channel));
 	brcmstb_pwm_writel(p, cword & 0xff, PWM_CWORD_LSB(channel));
 
@@ -171,7 +169,6 @@ static int brcmstb_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	/* Configure on and period value */
 	brcmstb_pwm_writel(p, pc, PWM_PERIOD(channel));
 	brcmstb_pwm_writel(p, dc, PWM_ON(channel));
-	spin_unlock(&p->lock);
 
 	return 0;
 }
@@ -182,7 +179,6 @@ static inline void brcmstb_pwm_enable_set(struct brcmstb_pwm *p,
 	unsigned int shift = channel * CTRL_CHAN_OFFS;
 	u32 value;
 
-	spin_lock(&p->lock);
 	value = brcmstb_pwm_readl(p, PWM_CTRL);
 
 	if (enable) {
@@ -194,7 +190,6 @@ static inline void brcmstb_pwm_enable_set(struct brcmstb_pwm *p,
 	}
 
 	brcmstb_pwm_writel(p, value, PWM_CTRL);
-	spin_unlock(&p->lock);
 }
 
 static int brcmstb_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
@@ -243,8 +238,6 @@ static int brcmstb_pwm_probe(struct platform_device *pdev)
 	if (!p)
 		return -ENOMEM;
 
-	spin_lock_init(&p->lock);
-
 	p->clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(p->clk)) {
 		dev_err(&pdev->dev, "failed to obtain clock\n");
-- 
2.34.1

[PATCH 2/2] pwm: brcmstb: Remove useless locking

[Uwe Kleine-König]

The lock only protects against concurrent users of the PWM API. This is not
expected to be necessary. And if there was such an issue, this is better
handled in the pwm core instead as it affects all drivers in the same way.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/pwm-brcmstb.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/pwm/pwm-brcmstb.c b/drivers/pwm/pwm-brcmstb.c
index 99974390aa38..3db3f96edf78 100644
--- a/drivers/pwm/pwm-brcmstb.c
+++ b/drivers/pwm/pwm-brcmstb.c
@@ -53,7 +53,6 @@
 
 struct brcmstb_pwm {
 	void __iomem *base;
-	spinlock_t lock;
 	struct clk *clk;
 	struct pwm_chip chip;
 };
@@ -159,7 +158,6 @@ static int brcmstb_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	 * generator output a base frequency for the constant frequency
 	 * generator to derive from.
 	 */
-	spin_lock(&p->lock);
 	brcmstb_pwm_writel(p, cword >> 8, PWM_CWORD_MSB(channel));
 	brcmstb_pwm_writel(p, cword & 0xff, PWM_CWORD_LSB(channel));
 
@@ -171,7 +169,6 @@ static int brcmstb_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	/* Configure on and period value */
 	brcmstb_pwm_writel(p, pc, PWM_PERIOD(channel));
 	brcmstb_pwm_writel(p, dc, PWM_ON(channel));
-	spin_unlock(&p->lock);
 
 	return 0;
 }
@@ -182,7 +179,6 @@ static inline void brcmstb_pwm_enable_set(struct brcmstb_pwm *p,
 	unsigned int shift = channel * CTRL_CHAN_OFFS;
 	u32 value;
 
-	spin_lock(&p->lock);
 	value = brcmstb_pwm_readl(p, PWM_CTRL);
 
 	if (enable) {
@@ -194,7 +190,6 @@ static inline void brcmstb_pwm_enable_set(struct brcmstb_pwm *p,
 	}
 
 	brcmstb_pwm_writel(p, value, PWM_CTRL);
-	spin_unlock(&p->lock);
 }
 
 static int brcmstb_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
@@ -243,8 +238,6 @@ static int brcmstb_pwm_probe(struct platform_device *pdev)
 	if (!p)
 		return -ENOMEM;
 
-	spin_lock_init(&p->lock);
-
 	p->clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(p->clk)) {
 		dev_err(&pdev->dev, "failed to obtain clock\n");
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/2] pwm: brcmstb: Some cleanups

[Florian Fainelli]

On 2/14/22 12:23 AM, Uwe Kleine-König wrote:
> Hello,
> 
> here are a few cleanups for the brcmstb PWM driver. There are a few
> issues left with it, that I'm not addressing for now. Just mention it in
> case someone wants to work on this driver:
> 
>  - There is no .get_state() callback
>    (That needs to be implemented by some with hardware and
>    documentation)
> 
>  - There are a few places where an overflow can happen in
>    brcmstb_pwm_config() that are not handled
> 
>  - The loop in brcmstb_pwm_config() to calculate cword is ineffective,
>    cword could be calculated ad hoc.
> 
>  - I don't understand
> 
>                 /*
>                  * We can be called with separate duty and period updates,
>                  * so do not reject dc == 0 right away
>                  */
>                 if (pc == PWM_PERIOD_MIN || (dc < PWM_ON_MIN && duty_ns))
>                         return -EINVAL;
> 
>    The usual policy is "With the selected period, pick the biggest
>    possible duty_cycle that isn't bigger thatn the requested duty_cycle.
>    So should this case be handled using dc = 0 instead?
>    But as I don't understand the real issue here (is this about changing
>    period and duty at the same time?), I don't want to touch that.

IIRC, I was testing using a shell script that would exercise corner
cases by modifying the /sys/class/pwm/*/{period,duty_cycle} separately
was able to run into that. Let me see if I can dig up that script.

Can you give me a day or two to make sure your changes work properly? I
need to locate a board with an exposed PWM header so I can put a scope
on it. Thanks!
-- 
Florian

Re: [PATCH 0/2] pwm: brcmstb: Some cleanups

[Florian Fainelli]

On 2/14/22 12:23 AM, Uwe Kleine-König wrote:
> Hello,
> 
> here are a few cleanups for the brcmstb PWM driver. There are a few
> issues left with it, that I'm not addressing for now. Just mention it in
> case someone wants to work on this driver:
> 
>  - There is no .get_state() callback
>    (That needs to be implemented by some with hardware and
>    documentation)
> 
>  - There are a few places where an overflow can happen in
>    brcmstb_pwm_config() that are not handled
> 
>  - The loop in brcmstb_pwm_config() to calculate cword is ineffective,
>    cword could be calculated ad hoc.
> 
>  - I don't understand
> 
>                 /*
>                  * We can be called with separate duty and period updates,
>                  * so do not reject dc == 0 right away
>                  */
>                 if (pc == PWM_PERIOD_MIN || (dc < PWM_ON_MIN && duty_ns))
>                         return -EINVAL;
> 
>    The usual policy is "With the selected period, pick the biggest
>    possible duty_cycle that isn't bigger thatn the requested duty_cycle.
>    So should this case be handled using dc = 0 instead?
>    But as I don't understand the real issue here (is this about changing
>    period and duty at the same time?), I don't want to touch that.

IIRC, I was testing using a shell script that would exercise corner
cases by modifying the /sys/class/pwm/*/{period,duty_cycle} separately
was able to run into that. Let me see if I can dig up that script.

Can you give me a day or two to make sure your changes work properly? I
need to locate a board with an exposed PWM header so I can put a scope
on it. Thanks!
-- 
Florian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/2] pwm: brcmstb: Some cleanups

[Uwe Kleine-König]

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

On Mon, Feb 14, 2022 at 09:18:49AM -0800, Florian Fainelli wrote:
> On 2/14/22 12:23 AM, Uwe Kleine-König wrote:
> > Hello,
> > 
> > here are a few cleanups for the brcmstb PWM driver. There are a few
> > issues left with it, that I'm not addressing for now. Just mention it in
> > case someone wants to work on this driver:
> > 
> >  - There is no .get_state() callback
> >    (That needs to be implemented by some with hardware and
> >    documentation)

Assuming you have access to documentation, can you confirm, that the
registers that define the PWM's behaviour are readable? If I knew that I
could come up with an implementation for .get_state().

> >  - There are a few places where an overflow can happen in
> >    brcmstb_pwm_config() that are not handled
> > 
> >  - The loop in brcmstb_pwm_config() to calculate cword is ineffective,
> >    cword could be calculated ad hoc.
> > 
> >  - I don't understand
> > 
> >                 /*
> >                  * We can be called with separate duty and period updates,
> >                  * so do not reject dc == 0 right away
> >                  */
> >                 if (pc == PWM_PERIOD_MIN || (dc < PWM_ON_MIN && duty_ns))
> >                         return -EINVAL;
> > 
> >    The usual policy is "With the selected period, pick the biggest
> >    possible duty_cycle that isn't bigger thatn the requested duty_cycle.
> >    So should this case be handled using dc = 0 instead?
> >    But as I don't understand the real issue here (is this about changing
> >    period and duty at the same time?), I don't want to touch that.
> 
> IIRC, I was testing using a shell script that would exercise corner
> cases by modifying the /sys/class/pwm/*/{period,duty_cycle} separately
> was able to run into that. Let me see if I can dig up that script.

When you find it, it would be great to document the problem in a way
that it's still understandable some time later.

> Can you give me a day or two to make sure your changes work properly? I
> need to locate a board with an exposed PWM header so I can put a scope
> on it. Thanks!

Sure, in my experience it takes longer than two days on average until
Thierry picks up PWM patches. Thanks for your willingness to test!

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 0/2] pwm: brcmstb: Some cleanups

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 2457 bytes --]

On Mon, Feb 14, 2022 at 09:18:49AM -0800, Florian Fainelli wrote:
> On 2/14/22 12:23 AM, Uwe Kleine-König wrote:
> > Hello,
> > 
> > here are a few cleanups for the brcmstb PWM driver. There are a few
> > issues left with it, that I'm not addressing for now. Just mention it in
> > case someone wants to work on this driver:
> > 
> >  - There is no .get_state() callback
> >    (That needs to be implemented by some with hardware and
> >    documentation)

Assuming you have access to documentation, can you confirm, that the
registers that define the PWM's behaviour are readable? If I knew that I
could come up with an implementation for .get_state().

> >  - There are a few places where an overflow can happen in
> >    brcmstb_pwm_config() that are not handled
> > 
> >  - The loop in brcmstb_pwm_config() to calculate cword is ineffective,
> >    cword could be calculated ad hoc.
> > 
> >  - I don't understand
> > 
> >                 /*
> >                  * We can be called with separate duty and period updates,
> >                  * so do not reject dc == 0 right away
> >                  */
> >                 if (pc == PWM_PERIOD_MIN || (dc < PWM_ON_MIN && duty_ns))
> >                         return -EINVAL;
> > 
> >    The usual policy is "With the selected period, pick the biggest
> >    possible duty_cycle that isn't bigger thatn the requested duty_cycle.
> >    So should this case be handled using dc = 0 instead?
> >    But as I don't understand the real issue here (is this about changing
> >    period and duty at the same time?), I don't want to touch that.
> 
> IIRC, I was testing using a shell script that would exercise corner
> cases by modifying the /sys/class/pwm/*/{period,duty_cycle} separately
> was able to run into that. Let me see if I can dig up that script.

When you find it, it would be great to document the problem in a way
that it's still understandable some time later.

> Can you give me a day or two to make sure your changes work properly? I
> need to locate a board with an exposed PWM header so I can put a scope
> on it. Thanks!

Sure, in my experience it takes longer than two days on average until
Thierry picks up PWM patches. Thanks for your willingness to test!

Best regards
Uwe

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

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/2] pwm: brcmstb: Some cleanups

[Florian Fainelli]

On 2/14/22 10:34 AM, Uwe Kleine-König wrote:
> On Mon, Feb 14, 2022 at 09:18:49AM -0800, Florian Fainelli wrote:
>> On 2/14/22 12:23 AM, Uwe Kleine-König wrote:
>>> Hello,
>>>
>>> here are a few cleanups for the brcmstb PWM driver. There are a few
>>> issues left with it, that I'm not addressing for now. Just mention it in
>>> case someone wants to work on this driver:
>>>
>>>  - There is no .get_state() callback
>>>    (That needs to be implemented by some with hardware and
>>>    documentation)
> 
> Assuming you have access to documentation, can you confirm, that the
> registers that define the PWM's behaviour are readable? If I knew that I
> could come up with an implementation for .get_state().

Yes, the registers are read/write with no side effects, so this is going
to be working.

> 
>>>  - There are a few places where an overflow can happen in
>>>    brcmstb_pwm_config() that are not handled
>>>
>>>  - The loop in brcmstb_pwm_config() to calculate cword is ineffective,
>>>    cword could be calculated ad hoc.
>>>
>>>  - I don't understand
>>>
>>>                 /*
>>>                  * We can be called with separate duty and period updates,
>>>                  * so do not reject dc == 0 right away
>>>                  */
>>>                 if (pc == PWM_PERIOD_MIN || (dc < PWM_ON_MIN && duty_ns))
>>>                         return -EINVAL;
>>>
>>>    The usual policy is "With the selected period, pick the biggest
>>>    possible duty_cycle that isn't bigger thatn the requested duty_cycle.
>>>    So should this case be handled using dc = 0 instead?
>>>    But as I don't understand the real issue here (is this about changing
>>>    period and duty at the same time?), I don't want to touch that.
>>
>> IIRC, I was testing using a shell script that would exercise corner
>> cases by modifying the /sys/class/pwm/*/{period,duty_cycle} separately
>> was able to run into that. Let me see if I can dig up that script.
> 
> When you find it, it would be great to document the problem in a way
> that it's still understandable some time later.
> 
>> Can you give me a day or two to make sure your changes work properly? I
>> need to locate a board with an exposed PWM header so I can put a scope
>> on it. Thanks!
> 
> Sure, in my experience it takes longer than two days on average until
> Thierry picks up PWM patches. Thanks for your willingness to test!

The least I can do, short of doing the conversion myself ;) With that
said, I just tested your two patches and things still worked just fine:

Tested-by: Florian Fainelli <f.fainelli@gmail.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

Thanks a lot Uwe!
-- 
Florian

Re: [PATCH 0/2] pwm: brcmstb: Some cleanups

[Florian Fainelli]

On 2/14/22 10:34 AM, Uwe Kleine-König wrote:
> On Mon, Feb 14, 2022 at 09:18:49AM -0800, Florian Fainelli wrote:
>> On 2/14/22 12:23 AM, Uwe Kleine-König wrote:
>>> Hello,
>>>
>>> here are a few cleanups for the brcmstb PWM driver. There are a few
>>> issues left with it, that I'm not addressing for now. Just mention it in
>>> case someone wants to work on this driver:
>>>
>>>  - There is no .get_state() callback
>>>    (That needs to be implemented by some with hardware and
>>>    documentation)
> 
> Assuming you have access to documentation, can you confirm, that the
> registers that define the PWM's behaviour are readable? If I knew that I
> could come up with an implementation for .get_state().

Yes, the registers are read/write with no side effects, so this is going
to be working.

> 
>>>  - There are a few places where an overflow can happen in
>>>    brcmstb_pwm_config() that are not handled
>>>
>>>  - The loop in brcmstb_pwm_config() to calculate cword is ineffective,
>>>    cword could be calculated ad hoc.
>>>
>>>  - I don't understand
>>>
>>>                 /*
>>>                  * We can be called with separate duty and period updates,
>>>                  * so do not reject dc == 0 right away
>>>                  */
>>>                 if (pc == PWM_PERIOD_MIN || (dc < PWM_ON_MIN && duty_ns))
>>>                         return -EINVAL;
>>>
>>>    The usual policy is "With the selected period, pick the biggest
>>>    possible duty_cycle that isn't bigger thatn the requested duty_cycle.
>>>    So should this case be handled using dc = 0 instead?
>>>    But as I don't understand the real issue here (is this about changing
>>>    period and duty at the same time?), I don't want to touch that.
>>
>> IIRC, I was testing using a shell script that would exercise corner
>> cases by modifying the /sys/class/pwm/*/{period,duty_cycle} separately
>> was able to run into that. Let me see if I can dig up that script.
> 
> When you find it, it would be great to document the problem in a way
> that it's still understandable some time later.
> 
>> Can you give me a day or two to make sure your changes work properly? I
>> need to locate a board with an exposed PWM header so I can put a scope
>> on it. Thanks!
> 
> Sure, in my experience it takes longer than two days on average until
> Thierry picks up PWM patches. Thanks for your willingness to test!

The least I can do, short of doing the conversion myself ;) With that
said, I just tested your two patches and things still worked just fine:

Tested-by: Florian Fainelli <f.fainelli@gmail.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

Thanks a lot Uwe!
-- 
Florian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/2] pwm: brcmstb: Some cleanups

[Thierry Reding]

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

On Mon, Feb 14, 2022 at 09:23:52AM +0100, Uwe Kleine-König wrote:
> Hello,
> 
> here are a few cleanups for the brcmstb PWM driver. There are a few
> issues left with it, that I'm not addressing for now. Just mention it in
> case someone wants to work on this driver:
> 
>  - There is no .get_state() callback
>    (That needs to be implemented by some with hardware and
>    documentation)
> 
>  - There are a few places where an overflow can happen in
>    brcmstb_pwm_config() that are not handled
> 
>  - The loop in brcmstb_pwm_config() to calculate cword is ineffective,
>    cword could be calculated ad hoc.
> 
>  - I don't understand
> 
>                 /*
>                  * We can be called with separate duty and period updates,
>                  * so do not reject dc == 0 right away
>                  */
>                 if (pc == PWM_PERIOD_MIN || (dc < PWM_ON_MIN && duty_ns))
>                         return -EINVAL;
> 
>    The usual policy is "With the selected period, pick the biggest
>    possible duty_cycle that isn't bigger thatn the requested duty_cycle.
>    So should this case be handled using dc = 0 instead?
>    But as I don't understand the real issue here (is this about changing
>    period and duty at the same time?), I don't want to touch that.
> 
>  - The driver uses SIMPLE_DEV_PM_OPS which is deprecated.
> 
>  - The driver defines pr_fmt(fmt) but doesn't use it.
> 
> Uwe Kleine-König (2):
>   pwm: brcmstb: Implement .apply() callback
>   pwm: brcmstb: Remove useless locking
> 
>  drivers/pwm/pwm-brcmstb.c | 52 ++++++++++++++++++---------------------
>  1 file changed, 24 insertions(+), 28 deletions(-)

Both patches applied, thanks.

Thierry

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

Re: [PATCH 0/2] pwm: brcmstb: Some cleanups

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1744 bytes --]

On Mon, Feb 14, 2022 at 09:23:52AM +0100, Uwe Kleine-König wrote:
> Hello,
> 
> here are a few cleanups for the brcmstb PWM driver. There are a few
> issues left with it, that I'm not addressing for now. Just mention it in
> case someone wants to work on this driver:
> 
>  - There is no .get_state() callback
>    (That needs to be implemented by some with hardware and
>    documentation)
> 
>  - There are a few places where an overflow can happen in
>    brcmstb_pwm_config() that are not handled
> 
>  - The loop in brcmstb_pwm_config() to calculate cword is ineffective,
>    cword could be calculated ad hoc.
> 
>  - I don't understand
> 
>                 /*
>                  * We can be called with separate duty and period updates,
>                  * so do not reject dc == 0 right away
>                  */
>                 if (pc == PWM_PERIOD_MIN || (dc < PWM_ON_MIN && duty_ns))
>                         return -EINVAL;
> 
>    The usual policy is "With the selected period, pick the biggest
>    possible duty_cycle that isn't bigger thatn the requested duty_cycle.
>    So should this case be handled using dc = 0 instead?
>    But as I don't understand the real issue here (is this about changing
>    period and duty at the same time?), I don't want to touch that.
> 
>  - The driver uses SIMPLE_DEV_PM_OPS which is deprecated.
> 
>  - The driver defines pr_fmt(fmt) but doesn't use it.
> 
> Uwe Kleine-König (2):
>   pwm: brcmstb: Implement .apply() callback
>   pwm: brcmstb: Remove useless locking
> 
>  drivers/pwm/pwm-brcmstb.c | 52 ++++++++++++++++++---------------------
>  1 file changed, 24 insertions(+), 28 deletions(-)

Both patches applied, thanks.

Thierry

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/2] pwm: brcmstb: Some cleanups

[Uwe Kleine-König]

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

Hello Florian,

I have a few questions here looking in more detail into the brcmstb
driver:

 - What happens on PWM_ON(channel) = 0?
   I guess it just emits a flat inactive line, and refusing a small
   duty_cycle that results in PWM_ON(channel) = 0 is just artificial?

 - There is a line describing:

   	W = cword, if cword < 2 ^ 15 else 16-bit 2's complement of cword

   The driver only considers powers of two <= 2^15 for cword. Is the
   implementation just lazy, or is the comment misleading?
   At least s/</<=/ ?
   There is no sense in using a value > 2^15 as for each such value
   there is a smaller value with the same result, right?

 - clk_get_rate(p->clk) is expected to return 27 MHz, right?
   (Just for my understanding, not about to hardcode this in the code)

 - The explanation about period in the comment is:

   	The period is: (period + 1) / Fv

   so I would have expected:

   	pc = (period_ns * clkrate * cword / (NSEC_PER_SEC << 16)) - 1

   (assuming no overflows). However the -1 isn't in the code.

 - Duty-cycle calculation is unclear, the docs say:

 	"on" time is on / (period + 1)

   I suspect on time is $on / Fv though?
   But even with that I don't understand the +1 in

 	dc = mul_u64_u64_div_u64(duty_ns + 1, rate, NSEC_PER_SEC);

Can you enlighten me?

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 0/2] pwm: brcmstb: Some cleanups

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 1519 bytes --]

Hello Florian,

I have a few questions here looking in more detail into the brcmstb
driver:

 - What happens on PWM_ON(channel) = 0?
   I guess it just emits a flat inactive line, and refusing a small
   duty_cycle that results in PWM_ON(channel) = 0 is just artificial?

 - There is a line describing:

   	W = cword, if cword < 2 ^ 15 else 16-bit 2's complement of cword

   The driver only considers powers of two <= 2^15 for cword. Is the
   implementation just lazy, or is the comment misleading?
   At least s/</<=/ ?
   There is no sense in using a value > 2^15 as for each such value
   there is a smaller value with the same result, right?

 - clk_get_rate(p->clk) is expected to return 27 MHz, right?
   (Just for my understanding, not about to hardcode this in the code)

 - The explanation about period in the comment is:

   	The period is: (period + 1) / Fv

   so I would have expected:

   	pc = (period_ns * clkrate * cword / (NSEC_PER_SEC << 16)) - 1

   (assuming no overflows). However the -1 isn't in the code.

 - Duty-cycle calculation is unclear, the docs say:

 	"on" time is on / (period + 1)

   I suspect on time is $on / Fv though?
   But even with that I don't understand the +1 in

 	dc = mul_u64_u64_div_u64(duty_ns + 1, rate, NSEC_PER_SEC);

Can you enlighten me?

Best regards
Uwe

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

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/2] pwm: brcmstb: Some cleanups

[Florian Fainelli]

On 3/7/22 10:47 AM, Uwe Kleine-König wrote:
> Hello Florian,
> 
> I have a few questions here looking in more detail into the brcmstb
> driver:
> 
>  - What happens on PWM_ON(channel) = 0?
>    I guess it just emits a flat inactive line, and refusing a small
>    duty_cycle that results in PWM_ON(channel) = 0 is just artificial?
> 
>  - There is a line describing:
> 
>    	W = cword, if cword < 2 ^ 15 else 16-bit 2's complement of cword
> 
>    The driver only considers powers of two <= 2^15 for cword. Is the
>    implementation just lazy, or is the comment misleading?
>    At least s/</<=/ ?
>    There is no sense in using a value > 2^15 as for each such value
>    there is a smaller value with the same result, right?

This was copied from the specification which now that you mention it,
seems off by one in its formula, it should be <=. This is further
confirmed with:

pwm1_cword[15:0] must be less than or equal to 32768 when the
variable-frequency PWM is used as a clock for the constant-frequency PWM.
Reset value is 0x0.

so I believe that the comment is wrong and so is the specification of
the block that was used to write the driver.

> 
>  - clk_get_rate(p->clk) is expected to return 27 MHz, right?
>    (Just for my understanding, not about to hardcode this in the code)

That is right.

> 
>  - The explanation about period in the comment is:
> 
>    	The period is: (period + 1) / Fv
> 
>    so I would have expected:
> 
>    	pc = (period_ns * clkrate * cword / (NSEC_PER_SEC << 16)) - 1
> 
>    (assuming no overflows). However the -1 isn't in the code.
> 
>  - Duty-cycle calculation is unclear, the docs say:
> 
>  	"on" time is on / (period + 1)
> 
>    I suspect on time is $on / Fv though?

Yes, that is also what the specification says, not sure why I wrote that
down TBH.

>    But even with that I don't understand the +1 in
> 
>  	dc = mul_u64_u64_div_u64(duty_ns + 1, rate, NSEC_PER_SEC);
> 
> Can you enlighten me?

I wish, this was 7 years ago, and I do not remember why there was a +1
being added here, it might have been that this should have been a
DIV_ROUND_UP().

I am slowly rebuilding some context here so if you want me to test
something in the meantime, I will do that.
-- 
Florian

Re: [PATCH 0/2] pwm: brcmstb: Some cleanups

[Florian Fainelli]

On 3/7/22 10:47 AM, Uwe Kleine-König wrote:
> Hello Florian,
> 
> I have a few questions here looking in more detail into the brcmstb
> driver:
> 
>  - What happens on PWM_ON(channel) = 0?
>    I guess it just emits a flat inactive line, and refusing a small
>    duty_cycle that results in PWM_ON(channel) = 0 is just artificial?
> 
>  - There is a line describing:
> 
>    	W = cword, if cword < 2 ^ 15 else 16-bit 2's complement of cword
> 
>    The driver only considers powers of two <= 2^15 for cword. Is the
>    implementation just lazy, or is the comment misleading?
>    At least s/</<=/ ?
>    There is no sense in using a value > 2^15 as for each such value
>    there is a smaller value with the same result, right?

This was copied from the specification which now that you mention it,
seems off by one in its formula, it should be <=. This is further
confirmed with:

pwm1_cword[15:0] must be less than or equal to 32768 when the
variable-frequency PWM is used as a clock for the constant-frequency PWM.
Reset value is 0x0.

so I believe that the comment is wrong and so is the specification of
the block that was used to write the driver.

> 
>  - clk_get_rate(p->clk) is expected to return 27 MHz, right?
>    (Just for my understanding, not about to hardcode this in the code)

That is right.

> 
>  - The explanation about period in the comment is:
> 
>    	The period is: (period + 1) / Fv
> 
>    so I would have expected:
> 
>    	pc = (period_ns * clkrate * cword / (NSEC_PER_SEC << 16)) - 1
> 
>    (assuming no overflows). However the -1 isn't in the code.
> 
>  - Duty-cycle calculation is unclear, the docs say:
> 
>  	"on" time is on / (period + 1)
> 
>    I suspect on time is $on / Fv though?

Yes, that is also what the specification says, not sure why I wrote that
down TBH.

>    But even with that I don't understand the +1 in
> 
>  	dc = mul_u64_u64_div_u64(duty_ns + 1, rate, NSEC_PER_SEC);
> 
> Can you enlighten me?

I wish, this was 7 years ago, and I do not remember why there was a +1
being added here, it might have been that this should have been a
DIV_ROUND_UP().

I am slowly rebuilding some context here so if you want me to test
something in the meantime, I will do that.
-- 
Florian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/2] pwm: brcmstb: Some cleanups

[Uwe Kleine-König]

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

Hello Florian,

great to get answers from you in such a timely fashion. That helps a
lot!

On Mon, Mar 07, 2022 at 11:11:05AM -0800, Florian Fainelli wrote:
> On 3/7/22 10:47 AM, Uwe Kleine-König wrote:
> > I have a few questions here looking in more detail into the brcmstb
> > driver:
> > 
> >  - What happens on PWM_ON(channel) = 0?
> >    I guess it just emits a flat inactive line, and refusing a small
> >    duty_cycle that results in PWM_ON(channel) = 0 is just artificial?
> > 
> >  - There is a line describing:
> > 
> >    	W = cword, if cword < 2 ^ 15 else 16-bit 2's complement of cword
> > 
> >    The driver only considers powers of two <= 2^15 for cword. Is the
> >    implementation just lazy, or is the comment misleading?
> >    At least s/</<=/ ?
> >    There is no sense in using a value > 2^15 as for each such value
> >    there is a smaller value with the same result, right?
> 
> This was copied from the specification which now that you mention it,
> seems off by one in its formula, it should be <=. This is further
> confirmed with:
> 
> pwm1_cword[15:0] must be less than or equal to 32768 when the
> variable-frequency PWM is used as a clock for the constant-frequency PWM.
> Reset value is 0x0.
> 
> so I believe that the comment is wrong and so is the specification of
> the block that was used to write the driver.

OK, so the right thing would be:

	W = cword with cword <= 32768

and there is no limitation to powers of two. (However it's unclear to me
how this works in hardware for arbitrary values.)

> >  - clk_get_rate(p->clk) is expected to return 27 MHz, right?
> >    (Just for my understanding, not about to hardcode this in the code)
> 
> That is right.

ok.

> >  - The explanation about period in the comment is:
> > 
> >    	The period is: (period + 1) / Fv
> > 
> >    so I would have expected:
> > 
> >    	pc = (period_ns * clkrate * cword / (NSEC_PER_SEC << 16)) - 1
> > 
> >    (assuming no overflows). However the -1 isn't in the code.

You didn't comment on that one, I still assume this to be correct, i.e.
the -1 must be coped for in the code.

> >  - Duty-cycle calculation is unclear, the docs say:
> > 
> >  	"on" time is on / (period + 1)
> > 
> >    I suspect on time is $on / Fv though?
> 
> Yes, that is also what the specification says, not sure why I wrote that
> down TBH.

OK. on / (period + 1) would be the relative duty cycle then.

> >    But even with that I don't understand the +1 in
> > 
> >  	dc = mul_u64_u64_div_u64(duty_ns + 1, rate, NSEC_PER_SEC);
> > 
> > Can you enlighten me?
> 
> I wish, this was 7 years ago, and I do not remember why there was a +1
> being added here, it might have been that this should have been a
> DIV_ROUND_UP().

The most usual thing to do is to round down in .apply().

To sum up: The hardware works in quantums Q = 2^16 / (W * 27 MHz).
(This is nice for W = 2^n: Q = 2^(16 - n) / (27 MHz))

The period length is 

	(PWM_PERIOD(channel) + 1) * Q

and duty_cycle is defined by
 
	PWM_ON(channel) * Q

. (No +1 there?)

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 0/2] pwm: brcmstb: Some cleanups

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 3304 bytes --]

Hello Florian,

great to get answers from you in such a timely fashion. That helps a
lot!

On Mon, Mar 07, 2022 at 11:11:05AM -0800, Florian Fainelli wrote:
> On 3/7/22 10:47 AM, Uwe Kleine-König wrote:
> > I have a few questions here looking in more detail into the brcmstb
> > driver:
> > 
> >  - What happens on PWM_ON(channel) = 0?
> >    I guess it just emits a flat inactive line, and refusing a small
> >    duty_cycle that results in PWM_ON(channel) = 0 is just artificial?
> > 
> >  - There is a line describing:
> > 
> >    	W = cword, if cword < 2 ^ 15 else 16-bit 2's complement of cword
> > 
> >    The driver only considers powers of two <= 2^15 for cword. Is the
> >    implementation just lazy, or is the comment misleading?
> >    At least s/</<=/ ?
> >    There is no sense in using a value > 2^15 as for each such value
> >    there is a smaller value with the same result, right?
> 
> This was copied from the specification which now that you mention it,
> seems off by one in its formula, it should be <=. This is further
> confirmed with:
> 
> pwm1_cword[15:0] must be less than or equal to 32768 when the
> variable-frequency PWM is used as a clock for the constant-frequency PWM.
> Reset value is 0x0.
> 
> so I believe that the comment is wrong and so is the specification of
> the block that was used to write the driver.

OK, so the right thing would be:

	W = cword with cword <= 32768

and there is no limitation to powers of two. (However it's unclear to me
how this works in hardware for arbitrary values.)

> >  - clk_get_rate(p->clk) is expected to return 27 MHz, right?
> >    (Just for my understanding, not about to hardcode this in the code)
> 
> That is right.

ok.

> >  - The explanation about period in the comment is:
> > 
> >    	The period is: (period + 1) / Fv
> > 
> >    so I would have expected:
> > 
> >    	pc = (period_ns * clkrate * cword / (NSEC_PER_SEC << 16)) - 1
> > 
> >    (assuming no overflows). However the -1 isn't in the code.

You didn't comment on that one, I still assume this to be correct, i.e.
the -1 must be coped for in the code.

> >  - Duty-cycle calculation is unclear, the docs say:
> > 
> >  	"on" time is on / (period + 1)
> > 
> >    I suspect on time is $on / Fv though?
> 
> Yes, that is also what the specification says, not sure why I wrote that
> down TBH.

OK. on / (period + 1) would be the relative duty cycle then.

> >    But even with that I don't understand the +1 in
> > 
> >  	dc = mul_u64_u64_div_u64(duty_ns + 1, rate, NSEC_PER_SEC);
> > 
> > Can you enlighten me?
> 
> I wish, this was 7 years ago, and I do not remember why there was a +1
> being added here, it might have been that this should have been a
> DIV_ROUND_UP().

The most usual thing to do is to round down in .apply().

To sum up: The hardware works in quantums Q = 2^16 / (W * 27 MHz).
(This is nice for W = 2^n: Q = 2^(16 - n) / (27 MHz))

The period length is 

	(PWM_PERIOD(channel) + 1) * Q

and duty_cycle is defined by
 
	PWM_ON(channel) * Q

. (No +1 there?)

Best regards
Uwe

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

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/2] pwm: brcmstb: Some cleanups

[Florian Fainelli]

On 3/7/22 12:44 PM, Uwe Kleine-König wrote:
> Hello Florian,
> 
> great to get answers from you in such a timely fashion. That helps a
> lot!
> 
> On Mon, Mar 07, 2022 at 11:11:05AM -0800, Florian Fainelli wrote:
>> On 3/7/22 10:47 AM, Uwe Kleine-König wrote:
>>> I have a few questions here looking in more detail into the brcmstb
>>> driver:
>>>
>>>  - What happens on PWM_ON(channel) = 0?
>>>    I guess it just emits a flat inactive line, and refusing a small
>>>    duty_cycle that results in PWM_ON(channel) = 0 is just artificial?
>>>
>>>  - There is a line describing:
>>>
>>>    	W = cword, if cword < 2 ^ 15 else 16-bit 2's complement of cword
>>>
>>>    The driver only considers powers of two <= 2^15 for cword. Is the
>>>    implementation just lazy, or is the comment misleading?
>>>    At least s/</<=/ ?
>>>    There is no sense in using a value > 2^15 as for each such value
>>>    there is a smaller value with the same result, right?
>>
>> This was copied from the specification which now that you mention it,
>> seems off by one in its formula, it should be <=. This is further
>> confirmed with:
>>
>> pwm1_cword[15:0] must be less than or equal to 32768 when the
>> variable-frequency PWM is used as a clock for the constant-frequency PWM.
>> Reset value is 0x0.
>>
>> so I believe that the comment is wrong and so is the specification of
>> the block that was used to write the driver.
> 
> OK, so the right thing would be:
> 
> 	W = cword with cword <= 32768
> 
> and there is no limitation to powers of two. (However it's unclear to me
> how this works in hardware for arbitrary values.)
> 
>>>  - clk_get_rate(p->clk) is expected to return 27 MHz, right?
>>>    (Just for my understanding, not about to hardcode this in the code)
>>
>> That is right.
> 
> ok.
> 
>>>  - The explanation about period in the comment is:
>>>
>>>    	The period is: (period + 1) / Fv
>>>
>>>    so I would have expected:
>>>
>>>    	pc = (period_ns * clkrate * cword / (NSEC_PER_SEC << 16)) - 1
>>>
>>>    (assuming no overflows). However the -1 isn't in the code.
> 
> You didn't comment on that one, I still assume this to be correct, i.e.
> the -1 must be coped for in the code.
> 
>>>  - Duty-cycle calculation is unclear, the docs say:
>>>
>>>  	"on" time is on / (period + 1)
>>>
>>>    I suspect on time is $on / Fv though?
>>
>> Yes, that is also what the specification says, not sure why I wrote that
>> down TBH.
> 
> OK. on / (period + 1) would be the relative duty cycle then.
> 
>>>    But even with that I don't understand the +1 in
>>>
>>>  	dc = mul_u64_u64_div_u64(duty_ns + 1, rate, NSEC_PER_SEC);
>>>
>>> Can you enlighten me?
>>
>> I wish, this was 7 years ago, and I do not remember why there was a +1
>> being added here, it might have been that this should have been a
>> DIV_ROUND_UP().
> 
> The most usual thing to do is to round down in .apply().
> 
> To sum up: The hardware works in quantums Q = 2^16 / (W * 27 MHz).
> (This is nice for W = 2^n: Q = 2^(16 - n) / (27 MHz))
> 
> The period length is 
> 
> 	(PWM_PERIOD(channel) + 1) * Q
> 
> and duty_cycle is defined by
>  
> 	PWM_ON(channel) * Q
> 
> . (No +1 there?)

Yes, I think what you are saying here is correct and matches what is
being described in the specification:

In the case where cword < 2^15, a period in the output waveform consists
of a single 1/27 microsecond HIGH pulse followed by LOW pulse for the
rest of the period. In the case where cword ≥ 2^15, a period in the
output waveform consists of a single 1/27 microsecond LOW pulse followed
by HIGH pulse for the rest of the period. In a sequence of pulse cycles,
one cycle can have a duty cycle and period that is different from those
of the other cycles. In order to have every cycle have exactly
the same duty cycle and period, cword must be chosen such that
cword=2^i,, i=0..15.
-- 
Florian

Re: [PATCH 0/2] pwm: brcmstb: Some cleanups

[Florian Fainelli]

On 3/7/22 12:44 PM, Uwe Kleine-König wrote:
> Hello Florian,
> 
> great to get answers from you in such a timely fashion. That helps a
> lot!
> 
> On Mon, Mar 07, 2022 at 11:11:05AM -0800, Florian Fainelli wrote:
>> On 3/7/22 10:47 AM, Uwe Kleine-König wrote:
>>> I have a few questions here looking in more detail into the brcmstb
>>> driver:
>>>
>>>  - What happens on PWM_ON(channel) = 0?
>>>    I guess it just emits a flat inactive line, and refusing a small
>>>    duty_cycle that results in PWM_ON(channel) = 0 is just artificial?
>>>
>>>  - There is a line describing:
>>>
>>>    	W = cword, if cword < 2 ^ 15 else 16-bit 2's complement of cword
>>>
>>>    The driver only considers powers of two <= 2^15 for cword. Is the
>>>    implementation just lazy, or is the comment misleading?
>>>    At least s/</<=/ ?
>>>    There is no sense in using a value > 2^15 as for each such value
>>>    there is a smaller value with the same result, right?
>>
>> This was copied from the specification which now that you mention it,
>> seems off by one in its formula, it should be <=. This is further
>> confirmed with:
>>
>> pwm1_cword[15:0] must be less than or equal to 32768 when the
>> variable-frequency PWM is used as a clock for the constant-frequency PWM.
>> Reset value is 0x0.
>>
>> so I believe that the comment is wrong and so is the specification of
>> the block that was used to write the driver.
> 
> OK, so the right thing would be:
> 
> 	W = cword with cword <= 32768
> 
> and there is no limitation to powers of two. (However it's unclear to me
> how this works in hardware for arbitrary values.)
> 
>>>  - clk_get_rate(p->clk) is expected to return 27 MHz, right?
>>>    (Just for my understanding, not about to hardcode this in the code)
>>
>> That is right.
> 
> ok.
> 
>>>  - The explanation about period in the comment is:
>>>
>>>    	The period is: (period + 1) / Fv
>>>
>>>    so I would have expected:
>>>
>>>    	pc = (period_ns * clkrate * cword / (NSEC_PER_SEC << 16)) - 1
>>>
>>>    (assuming no overflows). However the -1 isn't in the code.
> 
> You didn't comment on that one, I still assume this to be correct, i.e.
> the -1 must be coped for in the code.
> 
>>>  - Duty-cycle calculation is unclear, the docs say:
>>>
>>>  	"on" time is on / (period + 1)
>>>
>>>    I suspect on time is $on / Fv though?
>>
>> Yes, that is also what the specification says, not sure why I wrote that
>> down TBH.
> 
> OK. on / (period + 1) would be the relative duty cycle then.
> 
>>>    But even with that I don't understand the +1 in
>>>
>>>  	dc = mul_u64_u64_div_u64(duty_ns + 1, rate, NSEC_PER_SEC);
>>>
>>> Can you enlighten me?
>>
>> I wish, this was 7 years ago, and I do not remember why there was a +1
>> being added here, it might have been that this should have been a
>> DIV_ROUND_UP().
> 
> The most usual thing to do is to round down in .apply().
> 
> To sum up: The hardware works in quantums Q = 2^16 / (W * 27 MHz).
> (This is nice for W = 2^n: Q = 2^(16 - n) / (27 MHz))
> 
> The period length is 
> 
> 	(PWM_PERIOD(channel) + 1) * Q
> 
> and duty_cycle is defined by
>  
> 	PWM_ON(channel) * Q
> 
> . (No +1 there?)

Yes, I think what you are saying here is correct and matches what is
being described in the specification:

In the case where cword < 2^15, a period in the output waveform consists
of a single 1/27 microsecond HIGH pulse followed by LOW pulse for the
rest of the period. In the case where cword ≥ 2^15, a period in the
output waveform consists of a single 1/27 microsecond LOW pulse followed
by HIGH pulse for the rest of the period. In a sequence of pulse cycles,
one cycle can have a duty cycle and period that is different from those
of the other cycles. In order to have every cycle have exactly
the same duty cycle and period, cword must be chosen such that
cword=2^i,, i=0..15.
-- 
Florian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/2] pwm: brcmstb: Some cleanups

[Uwe Kleine-König]

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

On Mon, Mar 07, 2022 at 02:27:22PM -0800, Florian Fainelli wrote:
> On 3/7/22 12:44 PM, Uwe Kleine-König wrote:
> > Hello Florian,
> > 
> > great to get answers from you in such a timely fashion. That helps a
> > lot!
> > 
> > On Mon, Mar 07, 2022 at 11:11:05AM -0800, Florian Fainelli wrote:
> >> On 3/7/22 10:47 AM, Uwe Kleine-König wrote:
> >>> I have a few questions here looking in more detail into the brcmstb
> >>> driver:
> >>>
> >>>  - What happens on PWM_ON(channel) = 0?
> >>>    I guess it just emits a flat inactive line, and refusing a small
> >>>    duty_cycle that results in PWM_ON(channel) = 0 is just artificial?
> >>>
> >>>  - There is a line describing:
> >>>
> >>>    	W = cword, if cword < 2 ^ 15 else 16-bit 2's complement of cword
> >>>
> >>>    The driver only considers powers of two <= 2^15 for cword. Is the
> >>>    implementation just lazy, or is the comment misleading?
> >>>    At least s/</<=/ ?
> >>>    There is no sense in using a value > 2^15 as for each such value
> >>>    there is a smaller value with the same result, right?
> >>
> >> This was copied from the specification which now that you mention it,
> >> seems off by one in its formula, it should be <=. This is further
> >> confirmed with:
> >>
> >> pwm1_cword[15:0] must be less than or equal to 32768 when the
> >> variable-frequency PWM is used as a clock for the constant-frequency PWM.
> >> Reset value is 0x0.
> >>
> >> so I believe that the comment is wrong and so is the specification of
> >> the block that was used to write the driver.
> > 
> > OK, so the right thing would be:
> > 
> > 	W = cword with cword <= 32768
> > 
> > and there is no limitation to powers of two. (However it's unclear to me
> > how this works in hardware for arbitrary values.)
> > 
> >>>  - clk_get_rate(p->clk) is expected to return 27 MHz, right?
> >>>    (Just for my understanding, not about to hardcode this in the code)
> >>
> >> That is right.
> > 
> > ok.
> > 
> >>>  - The explanation about period in the comment is:
> >>>
> >>>    	The period is: (period + 1) / Fv
> >>>
> >>>    so I would have expected:
> >>>
> >>>    	pc = (period_ns * clkrate * cword / (NSEC_PER_SEC << 16)) - 1
> >>>
> >>>    (assuming no overflows). However the -1 isn't in the code.
> > 
> > You didn't comment on that one, I still assume this to be correct, i.e.
> > the -1 must be coped for in the code.
> > 
> >>>  - Duty-cycle calculation is unclear, the docs say:
> >>>
> >>>  	"on" time is on / (period + 1)
> >>>
> >>>    I suspect on time is $on / Fv though?
> >>
> >> Yes, that is also what the specification says, not sure why I wrote that
> >> down TBH.
> > 
> > OK. on / (period + 1) would be the relative duty cycle then.
> > 
> >>>    But even with that I don't understand the +1 in
> >>>
> >>>  	dc = mul_u64_u64_div_u64(duty_ns + 1, rate, NSEC_PER_SEC);
> >>>
> >>> Can you enlighten me?
> >>
> >> I wish, this was 7 years ago, and I do not remember why there was a +1
> >> being added here, it might have been that this should have been a
> >> DIV_ROUND_UP().
> > 
> > The most usual thing to do is to round down in .apply().
> > 
> > To sum up: The hardware works in quantums Q = 2^16 / (W * 27 MHz).
> > (This is nice for W = 2^n: Q = 2^(16 - n) / (27 MHz))
> > 
> > The period length is 
> > 
> > 	(PWM_PERIOD(channel) + 1) * Q
> > 
> > and duty_cycle is defined by
> >  
> > 	PWM_ON(channel) * Q
> > 
> > . (No +1 there?)
> 
> Yes, I think what you are saying here is correct and matches what is
> being described in the specification:
> 
> In the case where cword < 2^15, a period in the output waveform consists
> of a single 1/27 microsecond HIGH pulse followed by LOW pulse for the
> rest of the period. In the case where cword ≥ 2^15, a period in the
> output waveform consists of a single 1/27 microsecond LOW pulse followed
> by HIGH pulse for the rest of the period. In a sequence of pulse cycles,
> one cycle can have a duty cycle and period that is different from those
> of the other cycles. In order to have every cycle have exactly
> the same duty cycle and period, cword must be chosen such that
> cword=2^i,, i=0..15.

I don't understand the part "a single 1/27 microsecond", does that
assume PWM_ON(channel) == 0 (or 1?). Otherwise bigger cword values seem
to enable inversed polarity, which might be a nice future expansion.

So non-power of two result in variing parameters such that in average
the period is (PWM_PERIOD(channel) + 1) * Q. The relative duty cycle is
right however. So for example if W = 0xc then every second period has
length 2^16 / (8 * 27 MHz) and the others have 2^16 / (16 * 27 MHz),
right? Well, probably they don't alternate, but we see a bunch of W=16
period by (the same ammount of) periods with W=8.

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 0/2] pwm: brcmstb: Some cleanups

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 5045 bytes --]

On Mon, Mar 07, 2022 at 02:27:22PM -0800, Florian Fainelli wrote:
> On 3/7/22 12:44 PM, Uwe Kleine-König wrote:
> > Hello Florian,
> > 
> > great to get answers from you in such a timely fashion. That helps a
> > lot!
> > 
> > On Mon, Mar 07, 2022 at 11:11:05AM -0800, Florian Fainelli wrote:
> >> On 3/7/22 10:47 AM, Uwe Kleine-König wrote:
> >>> I have a few questions here looking in more detail into the brcmstb
> >>> driver:
> >>>
> >>>  - What happens on PWM_ON(channel) = 0?
> >>>    I guess it just emits a flat inactive line, and refusing a small
> >>>    duty_cycle that results in PWM_ON(channel) = 0 is just artificial?
> >>>
> >>>  - There is a line describing:
> >>>
> >>>    	W = cword, if cword < 2 ^ 15 else 16-bit 2's complement of cword
> >>>
> >>>    The driver only considers powers of two <= 2^15 for cword. Is the
> >>>    implementation just lazy, or is the comment misleading?
> >>>    At least s/</<=/ ?
> >>>    There is no sense in using a value > 2^15 as for each such value
> >>>    there is a smaller value with the same result, right?
> >>
> >> This was copied from the specification which now that you mention it,
> >> seems off by one in its formula, it should be <=. This is further
> >> confirmed with:
> >>
> >> pwm1_cword[15:0] must be less than or equal to 32768 when the
> >> variable-frequency PWM is used as a clock for the constant-frequency PWM.
> >> Reset value is 0x0.
> >>
> >> so I believe that the comment is wrong and so is the specification of
> >> the block that was used to write the driver.
> > 
> > OK, so the right thing would be:
> > 
> > 	W = cword with cword <= 32768
> > 
> > and there is no limitation to powers of two. (However it's unclear to me
> > how this works in hardware for arbitrary values.)
> > 
> >>>  - clk_get_rate(p->clk) is expected to return 27 MHz, right?
> >>>    (Just for my understanding, not about to hardcode this in the code)
> >>
> >> That is right.
> > 
> > ok.
> > 
> >>>  - The explanation about period in the comment is:
> >>>
> >>>    	The period is: (period + 1) / Fv
> >>>
> >>>    so I would have expected:
> >>>
> >>>    	pc = (period_ns * clkrate * cword / (NSEC_PER_SEC << 16)) - 1
> >>>
> >>>    (assuming no overflows). However the -1 isn't in the code.
> > 
> > You didn't comment on that one, I still assume this to be correct, i.e.
> > the -1 must be coped for in the code.
> > 
> >>>  - Duty-cycle calculation is unclear, the docs say:
> >>>
> >>>  	"on" time is on / (period + 1)
> >>>
> >>>    I suspect on time is $on / Fv though?
> >>
> >> Yes, that is also what the specification says, not sure why I wrote that
> >> down TBH.
> > 
> > OK. on / (period + 1) would be the relative duty cycle then.
> > 
> >>>    But even with that I don't understand the +1 in
> >>>
> >>>  	dc = mul_u64_u64_div_u64(duty_ns + 1, rate, NSEC_PER_SEC);
> >>>
> >>> Can you enlighten me?
> >>
> >> I wish, this was 7 years ago, and I do not remember why there was a +1
> >> being added here, it might have been that this should have been a
> >> DIV_ROUND_UP().
> > 
> > The most usual thing to do is to round down in .apply().
> > 
> > To sum up: The hardware works in quantums Q = 2^16 / (W * 27 MHz).
> > (This is nice for W = 2^n: Q = 2^(16 - n) / (27 MHz))
> > 
> > The period length is 
> > 
> > 	(PWM_PERIOD(channel) + 1) * Q
> > 
> > and duty_cycle is defined by
> >  
> > 	PWM_ON(channel) * Q
> > 
> > . (No +1 there?)
> 
> Yes, I think what you are saying here is correct and matches what is
> being described in the specification:
> 
> In the case where cword < 2^15, a period in the output waveform consists
> of a single 1/27 microsecond HIGH pulse followed by LOW pulse for the
> rest of the period. In the case where cword ≥ 2^15, a period in the
> output waveform consists of a single 1/27 microsecond LOW pulse followed
> by HIGH pulse for the rest of the period. In a sequence of pulse cycles,
> one cycle can have a duty cycle and period that is different from those
> of the other cycles. In order to have every cycle have exactly
> the same duty cycle and period, cword must be chosen such that
> cword=2^i,, i=0..15.

I don't understand the part "a single 1/27 microsecond", does that
assume PWM_ON(channel) == 0 (or 1?). Otherwise bigger cword values seem
to enable inversed polarity, which might be a nice future expansion.

So non-power of two result in variing parameters such that in average
the period is (PWM_PERIOD(channel) + 1) * Q. The relative duty cycle is
right however. So for example if W = 0xc then every second period has
length 2^16 / (8 * 27 MHz) and the others have 2^16 / (16 * 27 MHz),
right? Well, probably they don't alternate, but we see a bunch of W=16
period by (the same ammount of) periods with W=8.

Best regards
Uwe

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

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel