Skip to contentSkip to conversation

pwm: bcm2835: improve clock handling

PATCH New Stefan Wahren 4 participants 12 messages Dec 1, 2015 – Dec 16, 2015

Subsystems (3)
Broadcom BCM2711/BCM2835 Arm architecture PWM Broadcom BCM281XX/BCM11XXX/BCM216XX Arm architecture
lore
Share & tools
lore

Stefan Wahren

author
#0 lore raw

cover pwm: bcm2835: improve clock handling

With the upcoming PWM clock support the pwm-bcm2835 should take more care about the clock handling. But this patch series doesn't depend on these clock driver changes.

Stefan Wahren (3):
pwm: bcm2835: calculate scaler in pwm_config
pwm: bcm2835: prevent division by zero
pwm: bcm2835: fix email address specifiction

 drivers/pwm/pwm-bcm2835.c |   18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)
Signature · 1 line
1.7.9.5
author
#1 lore raw

PATCH 1/3 pwm: bcm2835: calculate scaler in pwm_config

Currently pwm-bcm2835 assumes a fixed clock rate and stores the resulting scaler in the driver structure. But with the upcoming PWM clock support for clk-bcm2835 the rate could change. So calculate the scaler in pwm_config.

Signed-off-by Stefan Wahren <[email protected]>
---
 drivers/pwm/pwm-bcm2835.c |    8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)
drivers/pwm/pwm-bcm2835.c +3 -5
index b4c7f95..174cca9 100644
@@ -29,7 +29,6 @@
 struct bcm2835_pwm {
 	struct pwm_chip chip;
 	struct device *dev;
-	unsigned long scaler;
 	void __iomem *base;
 	struct clk *clk;
 };

@@ -66,6 +65,7 @@ static int bcm2835_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 			      int duty_ns, int period_ns)
 {
 	struct bcm2835_pwm *pc = to_bcm2835_pwm(chip);
+	unsigned long scaler = NSEC_PER_SEC / clk_get_rate(pc->clk);
 
 	if (period_ns <= MIN_PERIOD) {
 		dev_err(pc->dev, "period %d not supported, minimum %d\n",

@@ -73,8 +73,8 @@ static int bcm2835_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 		return -EINVAL;
 	}
 
-	writel(duty_ns / pc->scaler, pc->base + DUTY(pwm->hwpwm));
-	writel(period_ns / pc->scaler, pc->base + PERIOD(pwm->hwpwm));
+	writel(duty_ns / scaler, pc->base + DUTY(pwm->hwpwm));
+	writel(period_ns / scaler, pc->base + PERIOD(pwm->hwpwm));
 
 	return 0;
 }

@@ -156,8 +156,6 @@ static int bcm2835_pwm_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	pc->scaler = NSEC_PER_SEC / clk_get_rate(pc->clk);
-
 	pc->chip.dev = &pdev->dev;
 	pc->chip.ops = &bcm2835_pwm_ops;
 	pc->chip.npwm = 2;
Signature · 1 line
1.7.9.5
author
#2 lore raw

PATCH 2/3 pwm: bcm2835: prevent division by zero

In reply to Stefan Wahren

It's possible that the pwm clock become an orphan. So better check the result of clk_get_rate in order to prevent a division by zero.

Signed-off-by Stefan Wahren <[email protected]>
---
 drivers/pwm/pwm-bcm2835.c |   10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)
drivers/pwm/pwm-bcm2835.c +9 -1
index 174cca9..31a6992 100644
@@ -65,7 +65,15 @@ static int bcm2835_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 			      int duty_ns, int period_ns)
 {
 	struct bcm2835_pwm *pc = to_bcm2835_pwm(chip);
-	unsigned long scaler = NSEC_PER_SEC / clk_get_rate(pc->clk);
+	unsigned long rate = clk_get_rate(pc->clk);
+	unsigned long scaler;
+
+	if (!rate) {
+		dev_err(pc->dev, "failed to get clock rate\n");
+		return -EINVAL;
+	}
+
+	scaler = NSEC_PER_SEC / rate;
 
 	if (period_ns <= MIN_PERIOD) {
 		dev_err(pc->dev, "period %d not supported, minimum %d\n",
Signature · 1 line
1.7.9.5
#3 lore raw

PATCH 2/3 pwm: bcm2835: prevent division by zero

Quoted 3 lines · Stefan Wahren

It's possible that the pwm clock become an orphan. So better check the result of clk_get_rate in order to prevent a division by zero.

How would we lose our clock when we're keeping the clock enabled from driver probe until remove?

Patches 1 and 3 are:

Reviewed-by Eric Anholt <[email protected]>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20151201/646cc66d/attachment.sig>

author
#4 lore raw

PATCH 2/3 pwm: bcm2835: prevent division by zero

Am 02.12.2015 um 00:16 schrieb Eric Anholt:

Quoted 7 lines
Quoted 3 lines · Stefan Wahren

It's possible that the pwm clock become an orphan. So better check the result of clk_get_rate in order to prevent a division by zero.

How would we lose our clock when we're keeping the clock enabled from driver probe until remove?

It's not the problem that we lose the clock inside this driver. The pwm clock could be initial assigned to a unregistered parent clock like "GND".

I'm refering to this discussion:

http://lists.infradead.org/pipermail/linux-rpi-kernel/2015-November/002603.html

Quoted 4 lines

Patches 1 and 3 are:

Reviewed-by Eric Anholt <[email protected]>
#5 lore raw

PATCH 2/3 pwm: bcm2835: prevent division by zero

Quoted 9 lines · Stefan Wahren

Am 02.12.2015 um 00:16 schrieb Eric Anholt:

Quoted 7 lines
Quoted 3 lines · Stefan Wahren

It's possible that the pwm clock become an orphan. So better check the result of clk_get_rate in order to prevent a division by zero.

How would we lose our clock when we're keeping the clock enabled from driver probe until remove?

Quoted 6 lines

It's not the problem that we lose the clock inside this driver. The pwm clock could be initial assigned to a unregistered parent clock like "GND".

I'm refering to this discussion:

http://lists.infradead.org/pipermail/linux-rpi-kernel/2015-November/002603.html

I actually would have expected that the prepare would error out on a clock with no rate set. Interesting. This seems like a good safety measure, so this one is also:

Reviewed-by Eric Anholt <[email protected]>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20151202/dd15c55a/attachment.sig>

#6 lore raw

PATCH 2/3 pwm: bcm2835: prevent division by zero

In reply to Stefan Wahren
Quoted 17 lines · Stefan Wahren

It's possible that the pwm clock become an orphan. So better check the result of clk_get_rate in order to prevent a division by zero.

Signed-off-by Stefan Wahren <[email protected]>
---
 drivers/pwm/pwm-bcm2835.c |   10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)
drivers/pwm/pwm-bcm2835.c +0 -0
index 174cca9..31a6992 100644
@@ -65,7 +65,15 @@ static int bcm2835_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
                              int duty_ns, int period_ns)
 {
        struct bcm2835_pwm *pc = to_bcm2835_pwm(chip);

drivers/pwm/pwm-bcm2835.c static int bcm2835_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, · line 76

Quoted 10 lines
drivers/pwm/pwm-bcm2835.c +9 -1
-       unsigned long scaler = NSEC_PER_SEC / clk_get_rate(pc->clk);
+       unsigned long rate = clk_get_rate(pc->clk);
+       unsigned long scaler;
+
+       if (!rate) {
+               dev_err(pc->dev, "failed to get clock rate\n");
+               return -EINVAL;
+       }
+
+       scaler = NSEC_PER_SEC / rate;

Stefan,

Please merge this code into patch 1/3. That way it is done the right way the first time around.

Thanks,
Mathieu

Quoted 11 lines
drivers/pwm/pwm-bcm2835.c +0 -1
        if (period_ns <= MIN_PERIOD) {
                dev_err(pc->dev, "period %d not supported, minimum %d\n",
--

1.7.9.5

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

#7 lore raw

PATCH 2/3 pwm: bcm2835: prevent division by zero

Quoted 31 lines · Mathieu Poirier
Quoted 17 lines · Stefan Wahren

It's possible that the pwm clock become an orphan. So better check the result of clk_get_rate in order to prevent a division by zero.

Signed-off-by Stefan Wahren <[email protected]>
---
 drivers/pwm/pwm-bcm2835.c |   10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)
drivers/pwm/pwm-bcm2835.c +0 -0
index 174cca9..31a6992 100644
@@ -65,7 +65,15 @@ static int bcm2835_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
                              int duty_ns, int period_ns)
 {
        struct bcm2835_pwm *pc = to_bcm2835_pwm(chip);

drivers/pwm/pwm-bcm2835.c static int bcm2835_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, · line 76

Quoted 10 lines
drivers/pwm/pwm-bcm2835.c +9 -1
-       unsigned long scaler = NSEC_PER_SEC / clk_get_rate(pc->clk);
+       unsigned long rate = clk_get_rate(pc->clk);
+       unsigned long scaler;
+
+       if (!rate) {
+               dev_err(pc->dev, "failed to get clock rate\n");
+               return -EINVAL;
+       }
+
+       scaler = NSEC_PER_SEC / rate;

Stefan,

Quoted 2 lines

Please merge this code into patch 1/3. That way it is done the right way the first time around.

They're separate changes and are good as separate patches.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20151202/f419b922/attachment-0001.sig>

author
#8 lore raw

PATCH 3/3 pwm: bcm2835: fix email address specifiction

In reply to Stefan Wahren
Signed-off-by Stefan Wahren <[email protected]>
---
 drivers/pwm/pwm-bcm2835.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/pwm/pwm-bcm2835.c +1 -1
index 31a6992..c5dbf16 100644
@@ -206,6 +206,6 @@ static struct platform_driver bcm2835_pwm_driver = {
 };
 module_platform_driver(bcm2835_pwm_driver);
 
-MODULE_AUTHOR("Bart Tanghe <bart.tanghe at thomasmore.be");
+MODULE_AUTHOR("Bart Tanghe <[email protected]>");
 MODULE_DESCRIPTION("Broadcom BCM2835 PWM driver");
 MODULE_LICENSE("GPL v2");
Signature · 1 line
1.7.9.5
author
#9 lore raw

cover pwm: bcm2835: improve clock handling

In reply to Stefan Wahren
Quoted 9 lines

Stefan Wahren <[email protected]> hat am 1. Dezember 2015 um 23:55 geschrieben:

With the upcoming PWM clock support the pwm-bcm2835 should take more care about the clock handling. But this patch series doesn't depend on these clock driver changes.

Quoted 8 lines

Stefan Wahren (3):
pwm: bcm2835: calculate scaler in pwm_config
pwm: bcm2835: prevent division by zero
pwm: bcm2835: fix email address specifiction

drivers/pwm/pwm-bcm2835.c | 18 ++++++++++++------

1 file changed, 12 insertions(+), 6 deletions(-)

gently ping

#10 lore raw

cover pwm: bcm2835: improve clock handling

Quoted 13 lines · Stefan Wahren
Quoted 13 lines

Stefan Wahren <[email protected]> hat am 1. Dezember 2015 um 23:55 geschrieben:

With the upcoming PWM clock support the pwm-bcm2835 should take more care about the clock handling. But this patch series doesn't depend on these clock driver changes.

Stefan Wahren (3):
pwm: bcm2835: calculate scaler in pwm_config
pwm: bcm2835: prevent division by zero
pwm: bcm2835: fix email address specifiction

Quoted 5 lines
Quoted 3 lines

drivers/pwm/pwm-bcm2835.c | 18 ++++++++++++------

1 file changed, 12 insertions(+), 6 deletions(-)

gently ping

Thierry, do you want to fold in my r-bs and apply, or should Stefan
resend with them?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20151215/b74ba940/attachment-0001.sig>

#11 lore raw

cover pwm: bcm2835: improve clock handling

In reply to Stefan Wahren
Quoted 11 lines · Stefan Wahren

With the upcoming PWM clock support the pwm-bcm2835 should take more care about the clock handling. But this patch series doesn't depend on these clock driver changes.

Stefan Wahren (3):
pwm: bcm2835: calculate scaler in pwm_config
pwm: bcm2835: prevent division by zero
pwm: bcm2835: fix email address specifiction

 drivers/pwm/pwm-bcm2835.c |   18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

All three patches applied with Eric's Reviewed-by, thanks.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20151216/973a08fa/attachment-0001.sig>