[resend rfc v3] pwm: add BCM2835 PWM driver

Discussion:

Bart Tanghe

2014-04-28 12:54:46 UTC

Add some better error handling and Device table support
Added Documentation/devicetree/bindings/pwm/pwm-bcm2835.txt

Signed-off-by: Bart Tanghe <***@thomasmore.be>
diff --git a/Documentation/devicetree/bindings/pwm/pwm-bcm2835.txt b/Documentation/devicetree/bindings/pwm/pwm-bcm2835.txt
new file mode 100644
index 0000000..44c0b95
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/pwm-bcm2835.txt
@@ -0,0 +1,13 @@
+bcm2835 PWM controller
+
+Required properties:
+- compatible: should be "bcrm,pwm-bcm2835"
+- reg: physical base address and length of the controller's registers
+
+Examples:
+
+***@0x2020C000 {
+ compatible = "bcrm,pwm-bcm2835";
+ reg = <0x2020C000 0x28>;
+ clocks = <&clk_pwm>;
+};
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 22f2f28..20341a3 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -62,6 +62,18 @@ config PWM_ATMEL_TCB
To compile this driver as a module, choose M here: the module
will be called pwm-atmel-tcb.

+config PWM_BCM2835
+ tristate "BCM2835 PWM support"
+ depends on MACH_BCM2835 || MACH_BCM2708
+ help
+ PWM framework driver for BCM2835 controller (raspberry pi)
+ Only 1 channel is implemented.
+
+ The pwm core is tested with a pwm basic frequency of 1Mhz.
+ Use period above 1000ns
+
+ To compile this driver as a module, choose M here: the module
+ will be called pwm-bcm2835.
+
config PWM_BFIN
tristate "Blackfin PWM support"
depends on BFIN_GPTIMERS
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index d8906ec..9863590 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -3,6 +3,7 @@ obj-$(CONFIG_PWM_SYSFS) += sysfs.o
obj-$(CONFIG_PWM_AB8500) += pwm-ab8500.o
obj-$(CONFIG_PWM_ATMEL) += pwm-atmel.o
obj-$(CONFIG_PWM_ATMEL_TCB) += pwm-atmel-tcb.o
+obj-$(CONFIG_PWM_BCM2835) += pwm-bcm2835.o
obj-$(CONFIG_PWM_BFIN) += pwm-bfin.o
obj-$(CONFIG_PWM_EP93XX) += pwm-ep93xx.o
obj-$(CONFIG_PWM_IMX) += pwm-imx.o
diff --git a/drivers/pwm/pwm-bcm2835.c b/drivers/pwm/pwm-bcm2835.c
new file mode 100644
index 0000000..ec9829b
--- /dev/null
+++ b/drivers/pwm/pwm-bcm2835.c
@@ -0,0 +1,198 @@
+/*
+ * pwm-bcm2835 driver
+ * Standard raspberry pi (gpio18 - pwm0)
+ *
+ * Copyright (C) 2014 Thomas more
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2.
+ */
+
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+
+/*mmio regiser mapping*/
+
+#define DUTY 0x14
+#define PERIOD 0x10
+#define CHANNEL 0x10
+
+#define PWM_ENABLE 0x00000001
+#define PWM_POLARITY 0x00000010
+
+#define MASK_CTL_PWM 0x000000FF
+#define CTL_PWM 0x00000081
+
+#define DRIVER_AUTHOR "Bart Tanghe <***@thomasmore.be>"
+#define DRIVER_DESC "A bcm2835 pwm driver - raspberry pi development platform"
+
+struct bcm2835_pwm_chip {
+ struct pwm_chip chip;
+ struct device *dev;
+ int channel;
+ int scaler;
+ void __iomem *mmio_base;
+};
+
+static inline struct bcm2835_pwm_chip *to_bcm2835_pwm_chip(
+ struct pwm_chip *chip){
+
+ return container_of(chip, struct bcm2835_pwm_chip, chip);
+}
+
+static int bcm2835_pwm_config(struct pwm_chip *chip,
+ struct pwm_device *pwm,
+ int duty_ns, int period_ns){
+
+ struct bcm2835_pwm_chip *pc;
+
+ pc = container_of(chip, struct bcm2835_pwm_chip, chip);
+
+ iowrite32(duty_ns/pc->scaler, pc->mmio_base + DUTY);
+ iowrite32(period_ns/pc->scaler, pc->mmio_base + PERIOD);
+
+ return 0;
+}
+
+static int bcm2835_pwm_enable(struct pwm_chip *chip,
+ struct pwm_device *pwm){
+
+ struct bcm2835_pwm_chip *pc;
+
+ pc = container_of(chip, struct bcm2835_pwm_chip, chip);
+
+ iowrite32(ioread32(pc->mmio_base) | PWM_ENABLE, pc->mmio_base);
+ return 0;
+}
+
+static void bcm2835_pwm_disable(struct pwm_chip *chip,
+ struct pwm_device *pwm)
+{
+ struct bcm2835_pwm_chip *pc;
+
+ pc = to_bcm2835_pwm_chip(chip);
+
+ iowrite32(ioread32(pc->mmio_base) & ~PWM_ENABLE, pc->mmio_base);
+}
+
+static int bcm2835_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
+ enum pwm_polarity polarity)
+{
+ struct bcm2835_pwm_chip *pc;
+
+ pc = to_bcm2835_pwm_chip(chip);
+
+ if (polarity == PWM_POLARITY_NORMAL)
+ iowrite32((ioread32(pc->mmio_base) & ~PWM_POLARITY),
+ pc->mmio_base);
+ else if (polarity == PWM_POLARITY_INVERSED)
+ iowrite32((ioread32(pc->mmio_base) | PWM_POLARITY),
+ pc->mmio_base);
+
+ return 0;
+}
+
+static const struct pwm_ops bcm2835_pwm_ops = {
+ .config = bcm2835_pwm_config,
+ .enable = bcm2835_pwm_enable,
+ .disable = bcm2835_pwm_disable,
+ .set_polarity = bcm2835_set_polarity,
+ .owner = THIS_MODULE,
+};
+
+static int bcm2835_pwm_probe(struct platform_device *pdev)
+{
+ struct bcm2835_pwm_chip *pwm;
+
+ int ret;
+ struct resource *r;
+ u32 start, end;
+ struct clk *clk;
+
+ pwm = devm_kzalloc(&pdev->dev, sizeof(*pwm), GFP_KERNEL);
+ if (!pwm) {
+ dev_err(&pdev->dev, "failed to allocate memory\n");
+ return -ENOMEM;
+ }
+
+ pwm->dev = &pdev->dev;
+
+ clk = clk_get(&pdev->dev, NULL);
+ if (IS_ERR(clk)) {
+ dev_err(&pdev->dev, "could not find clk: %ld\n", PTR_ERR(clk));
+ devm_kfree(&pdev->dev, pwm);
+ return PTR_ERR(clk);
+ }
+
+ pwm->scaler = (int)1000000000/clk_get_rate(clk);
+
+ r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ pwm->mmio_base = devm_ioremap_resource(&pdev->dev, r);
+ if (IS_ERR(pwm->mmio_base)) {
+ devm_kfree(&pdev->dev, pwm);
+ return PTR_ERR(pwm->mmio_base);
+ }
+
+ start = r->start;
+ end = r->end;
+
+ pwm->chip.dev = &pdev->dev;
+ pwm->chip.ops = &bcm2835_pwm_ops;
+ pwm->chip.npwm = 2;
+
+ ret = pwmchip_add(&pwm->chip);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret);
+ devm_kfree(&pdev->dev, pwm);
+ return -1;
+ }
+
+ /*set the pwm0 configuration*/
+ iowrite32((ioread32(pwm->mmio_base) & ~MASK_CTL_PWM)
+ | CTL_PWM, pwm->mmio_base);
+
+ platform_set_drvdata(pdev, pwm);
+
+ return 0;
+}
+
+static int bcm2835_pwm_remove(struct platform_device *pdev)
+{
+
+ struct bcm2835_pwm_chip *pc;
+ pc = platform_get_drvdata(pdev);
+
+ if (WARN_ON(!pc))
+ return -ENODEV;
+
+ return pwmchip_remove(&pc->chip);
+}
+
+static const struct of_device_id bcm2835_pwm_of_match[] = {
+ { .compatible = "bcrm,pwm-bcm2835", },
+ { /* sentinel */ }
+};
+
+MODULE_DEVICE_TABLE(of, bcm2835_pwm_of_match);
+
+static struct platform_driver bcm2835_pwm_driver = {
+ .driver = {
+ .name = "pwm-bcm2835",
+ .owner = THIS_MODULE,
+ .of_match_table = bcm2835_pwm_of_match,
+ },
+ .probe = bcm2835_pwm_probe,
+ .remove = bcm2835_pwm_remove,
+};
+module_platform_driver(bcm2835_pwm_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR(DRIVER_AUTHOR);
+MODULE_DESCRIPTION(DRIVER_DESC);
--
To unsubscribe from this list: send the line "unsubscribe linux-pwm" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Thierry Reding

2014-08-25 13:19:10 UTC

Permalink

Sorry for taking so long to reply to this, I had completely forgotten.

Post by Bart Tanghe
Add some better error handling and Device table support
Added Documentation/devicetree/bindings/pwm/pwm-bcm2835.txt

There should be a description about this driver in the commit message.
The above reads like a changelog since earlier versions of this patch,
in which case it should go below a --- separator line, like so:

Commit message goes here...
...

Signed-off-by: Bart Tanghe <***@thomasmore.be>
---
Changes in v3:
- add some better error handling
- add device tree support

I assume that "device table" was meant to be "device tree"? Also it
might be useful to mention Raspberry Pi in the commit message.

Post by Bart Tanghe
diff --git a/Documentation/devicetree/bindings/pwm/pwm-bcm2835.txt b/Documentation/devicetree/bindings/pwm/pwm-bcm2835.txt
new file mode 100644
index 0000000..44c0b95
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/pwm-bcm2835.txt
@@ -0,0 +1,13 @@
+bcm2835 PWM controller

I think this ought to be "BCM2835". Again, maybe this should mention the
Raspberry Pi so that when people search for it they get a match on this
document.

Post by Bart Tanghe
+- compatible: should be "bcrm,pwm-bcm2835"

According to Documentation/devicetree/bindings/vendor-prefixes.txt this
should be "brcm,...". Also I'd suggest putting the SoC first, followed
by the hardware block name:

"brcm,bcm2835-pwm"

Post by Bart Tanghe
+- reg: physical base address and length of the controller's registers
+
+
+ compatible = "bcrm,pwm-bcm2835";
+ reg = <0x2020C000 0x28>;

I think other BCM2835 device tree bindings use lower-case for addresses,
so you might want to follow that for consistency. Also unit-addresses
are always in hexadecimal and shouldn't take a 0x prefix, so the above
would become:

***@2020c000 {
...
};

Post by Bart Tanghe
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 22f2f28..20341a3 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -62,6 +62,18 @@ config PWM_ATMEL_TCB
To compile this driver as a module, choose M here: the module
will be called pwm-atmel-tcb.
+config PWM_BCM2835
+ tristate "BCM2835 PWM support"
+ depends on MACH_BCM2835 || MACH_BCM2708
+ help
+ PWM framework driver for BCM2835 controller (raspberry pi)

I think the correct capitalization would be "Raspberry Pi".

Post by Bart Tanghe
+ Only 1 channel is implemented.

How many can it take? Why haven't all been implemented?

Post by Bart Tanghe
+ The pwm core is tested with a pwm basic frequency of 1Mhz.
+ Use period above 1000ns

s/pwm/PWM/ in prose. Why this restriction? Doesn't it work with higher
frequencies? Perhaps this shouldn't be a comment in the Kconfig entry
but rather a runtime check (and error message)?

Post by Bart Tanghe
diff --git a/drivers/pwm/pwm-bcm2835.c b/drivers/pwm/pwm-bcm2835.c
new file mode 100644
index 0000000..ec9829b
--- /dev/null
+++ b/drivers/pwm/pwm-bcm2835.c
@@ -0,0 +1,198 @@
+/*
+ * pwm-bcm2835 driver
+ * Standard raspberry pi (gpio18 - pwm0)

Just drop these two lines, they don't provide very useful information.

Post by Bart Tanghe
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+
+/*mmio regiser mapping*/

s/mmio/MMIO/, s/regiser/register/. Also spaces after /* and before */.

Post by Bart Tanghe
+
+#define DUTY 0x14
+#define PERIOD 0x10
+#define CHANNEL 0x10

CHANNEL doesn't seem to be used.

Post by Bart Tanghe
+
+#define PWM_ENABLE 0x00000001
+#define PWM_POLARITY 0x00000010
+
+#define MASK_CTL_PWM 0x000000FF

I prefer lowercase for hexadecimals. And there's no need to repeat all
the leading zeroes. PWM_ENABLE and PWM_POLARITY seem to be bits, so I'd
prefer:

#define PWM_ENABLE (1 << 0)
#define PWM_POLARITY (1 << 4)

Post by Bart Tanghe
+#define CTL_PWM 0x00000081

What does this value mean? Also even if this register is at offset 0 you
should still add a symbolic name for it.

Post by Bart Tanghe
+#define DRIVER_DESC "A bcm2835 pwm driver - raspberry pi development platform"

These are only used once, so you don't have to #define them. Use them in
the MODULE_*() macros directly.

Also, perhaps a better and more generic description would be:

"Broadcom BCM2835 (Raspberry Pi) PWM driver"

And perhaps even drop "(Raspberry Pi)" since presumably the driver will
work on any BCM2835-based board.

Post by Bart Tanghe
+struct bcm2835_pwm_chip {
+ struct pwm_chip chip;
+ struct device *dev;
+ int channel;

This field seems to be unused.

Post by Bart Tanghe
+ int scaler;

Perhaps this should be unsigned long instead of int?

Post by Bart Tanghe
+ void __iomem *mmio_base;

Calling this something like "base" or "regs" will save a lot of
characters when accessing registers.

Post by Bart Tanghe
+static inline struct bcm2835_pwm_chip *to_bcm2835_pwm_chip(
+ struct pwm_chip *chip){
+
+ return container_of(chip, struct bcm2835_pwm_chip, chip);
+}

The preferred way to write the above is:

static inline struct bcm2835_pwm_chip *
to_bcm2835_pwm_chip(struct pwm_chip *chip)
{
...
}

Perhaps if you make names a little shorter you can even get away without
wrapping it:

static inline struct bcm2835_pwm *to_bcm2835_pwm(struct pwm_chip *chip)
{
...
}

Post by Bart Tanghe
+static int bcm2835_pwm_config(struct pwm_chip *chip,
+ struct pwm_device *pwm,
+ int duty_ns, int period_ns){

The pwm argument still fits on the first line. Also the opening brace
({) should go on a separate line.

Post by Bart Tanghe
+
+ struct bcm2835_pwm_chip *pc;
+
+ pc = container_of(chip, struct bcm2835_pwm_chip, chip);

This should use the to_bcm2835_pwm() function defined earlier.

Post by Bart Tanghe
+
+ iowrite32(duty_ns/pc->scaler, pc->mmio_base + DUTY);
+ iowrite32(period_ns/pc->scaler, pc->mmio_base + PERIOD);

These should use writel() instead of iowrite32() and there need to be
spaces around '/'.

Post by Bart Tanghe
+static int bcm2835_pwm_enable(struct pwm_chip *chip,
+ struct pwm_device *pwm){

Same as above.

Post by Bart Tanghe
+
+ struct bcm2835_pwm_chip *pc;
+
+ pc = container_of(chip, struct bcm2835_pwm_chip, chip);

Should use to_bcm2835_pwm() and can go on a single line:

struct bcm2835_pwm *pc = to_bcm2835_pwm(chip);

Post by Bart Tanghe
+
+ iowrite32(ioread32(pc->mmio_base) | PWM_ENABLE, pc->mmio_base);

Please break this up into:

u32 value;
...
value = readl(pc->mmio_base + ...);
value |= PWM_ENABLE;
writel(value, pc->mmio_base + ...);

Post by Bart Tanghe
+static void bcm2835_pwm_disable(struct pwm_chip *chip,
+ struct pwm_device *pwm)
+{
+ struct bcm2835_pwm_chip *pc;
+
+ pc = to_bcm2835_pwm_chip(chip);

The above can go on a single line.

Post by Bart Tanghe
+
+ iowrite32(ioread32(pc->mmio_base) & ~PWM_ENABLE, pc->mmio_base);
+}

Same as above and below.

Post by Bart Tanghe
+static int bcm2835_pwm_probe(struct platform_device *pdev)
+{
+ struct bcm2835_pwm_chip *pwm;
+

Gratuitous blank line.

Post by Bart Tanghe
+ int ret;
+ struct resource *r;
+ u32 start, end;

start and end don't seem to be used.

Post by Bart Tanghe
+ struct clk *clk;
+
+ pwm = devm_kzalloc(&pdev->dev, sizeof(*pwm), GFP_KERNEL);
+ if (!pwm) {
+ dev_err(&pdev->dev, "failed to allocate memory\n");

No need for this error message.

Post by Bart Tanghe
+ return -ENOMEM;
+ }
+
+ pwm->dev = &pdev->dev;
+
+ clk = clk_get(&pdev->dev, NULL);

devm_clk_get()? Also, don't you want to keep a reference to the clock in
struct bcm2835_pwm so that you can disable the clock on driver removal?

Post by Bart Tanghe
+ if (IS_ERR(clk)) {
+ dev_err(&pdev->dev, "could not find clk: %ld\n", PTR_ERR(clk));

In text I prefer to use "clock" rather than "clk".

Post by Bart Tanghe
+ devm_kfree(&pdev->dev, pwm);

No need for this. The point of the devm_*() functions is that you don't
have to manually clean up the resources that they manage.

Post by Bart Tanghe
+ return PTR_ERR(clk);
+ }
+
+ pwm->scaler = (int)1000000000/clk_get_rate(clk);

There's NSEC_PER_SEC and you shouldn't cast this if you change the type
of scaler to unsigned long. So this becomes:

pwm->scaler = NSEC_PER_SEC / clk_get_rate(clk);

Post by Bart Tanghe
+
+ r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ pwm->mmio_base = devm_ioremap_resource(&pdev->dev, r);
+ if (IS_ERR(pwm->mmio_base)) {
+ devm_kfree(&pdev->dev, pwm);

Again, no need to free the memory here.

Post by Bart Tanghe
+ return PTR_ERR(pwm->mmio_base);
+ }
+
+ start = r->start;
+ end = r->end;
+
+ pwm->chip.dev = &pdev->dev;
+ pwm->chip.ops = &bcm2835_pwm_ops;
+ pwm->chip.npwm = 2;

The Kconfig entry says that only a single PWM is implemented, so this
should be 1, shouldn't it?

Post by Bart Tanghe
+
+ ret = pwmchip_add(&pwm->chip);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret);
+ devm_kfree(&pdev->dev, pwm);

Drop this.

Post by Bart Tanghe
+ return -1;

This needs to propagate ret.

Post by Bart Tanghe
+ }
+
+ /*set the pwm0 configuration*/

Spaces after /* and before */.

Post by Bart Tanghe
+ iowrite32((ioread32(pwm->mmio_base) & ~MASK_CTL_PWM)
+ | CTL_PWM, pwm->mmio_base);

Should this perhaps be delayed until the PWM is requested? What are the
consequences of configuring the PWM?

Also split this up into an explicit read/modify/write sequence please.

Post by Bart Tanghe
+
+ platform_set_drvdata(pdev, pwm);
+
+ return 0;
+}

I notice that you never prepare or enable the clock here. Perhaps this
isn't required because it's always on, but I think you should still call
clk_prepare_enable() here (and clk_disable_unprepare() in .remove()) to
make sure the driver is more portable.

Post by Bart Tanghe
+
+static int bcm2835_pwm_remove(struct platform_device *pdev)
+{
+

Gratuitous blank line.

Post by Bart Tanghe
+ struct bcm2835_pwm_chip *pc;
+ pc = platform_get_drvdata(pdev);

The above can go on a single line.

Post by Bart Tanghe
+
+ if (WARN_ON(!pc))
+ return -ENODEV;

There's no need for this check.

Post by Bart Tanghe
+
+ return pwmchip_remove(&pc->chip);
+}
+
+static const struct of_device_id bcm2835_pwm_of_match[] = {
+ { .compatible = "bcrm,pwm-bcm2835", },

s/bcrm/brcm/

Post by Bart Tanghe
+ { /* sentinel */ }
+};
+

Gratuitous blank line.

Post by Bart Tanghe
+MODULE_DEVICE_TABLE(of, bcm2835_pwm_of_match);
+
+static struct platform_driver bcm2835_pwm_driver = {
+ .driver = {
+ .name = "pwm-bcm2835",
+ .owner = THIS_MODULE,

No need to initialize .owner here. module_platform_driver() will do that
for you.

Post by Bart Tanghe
+ .of_match_table = bcm2835_pwm_of_match,
+ },
+ .probe = bcm2835_pwm_probe,
+ .remove = bcm2835_pwm_remove,
+};
+module_platform_driver(bcm2835_pwm_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR(DRIVER_AUTHOR);
+MODULE_DESCRIPTION(DRIVER_DESC);

A more natural ordering would be:

MODULE_AUTHOR(...);
MODULE_DESCRIPTION(...);
MODULE_LICENSE(...);

Thierry

Stephen Warren

2014-09-04 15:06:48 UTC

Permalink

No problem. Thanks for the feedback.
I've got some question below.

Post by Thierry Reding
Sorry for taking so long to reply to this, I had completely forgotten.

Post by Bart Tanghe
Add some better error handling and Device table support
Added Documentation/devicetree/bindings/pwm/pwm-bcm2835.txt
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 22f2f28..20341a3 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -62,6 +62,18 @@ config PWM_ATMEL_TCB
To compile this driver as a module, choose M here: the module
will be called pwm-atmel-tcb.
+config PWM_BCM2835
+ tristate "BCM2835 PWM support"
+ depends on MACH_BCM2835 || MACH_BCM2708
+ help
+ PWM framework driver for BCM2835 controller (raspberry pi)

I think the correct capitalization would be "Raspberry Pi".

Post by Bart Tanghe
+ Only 1 channel is implemented.

How many can it take? Why haven't all been implemented?

BCM2835 can take 2 pwm channels.
I can implement 2 channels but can't physically test the second channel. Is that a problem?

I don't think that's a problem; I would expect the channels to be
identical, so testing 1 should be fine.

Post by Thierry Reding
I notice that you never prepare or enable the clock here. Perhaps this
isn't required because it's always on, but I think you should still call
clk_prepare_enable() here (and clk_disable_unprepare() in .remove()) to
make sure the driver is more portable.

The frequency can be minimized by a clock_divider ( the pwm clock is default disabled). this has to be done by
a clock driver, as mentioned in a previous comment by Stephen Warren.
Any clock programming should be performed by a clock driver. We don't
have one of those upstream yet, mainly because it would rely on talking
to the firmware (running on the VideoCore) to manipulate the clocks, and
we don't have a firmware protocol driver either.
Nowadays, I'm using a userspace program to change the clock_divider, but would like to implement this in a clock driver.
The clock hardware description isn't implemented in the datasheet. I can convert the userspace prog to a clock driver but this is very experimental.
If anyone has some suggestions?

Oh dear. It sounds like we need at least some form of clock driver for
the platform then. I still don't think there's complete documentation
for the HW, even though a lot of register docs were published which
presumably cover the clock HW? Equally, given that the VC firmware
assumes it owns most of the HW, it seems best to manipulate the clocks
through the firmware interface rather than directly touching the HW.
Unfortunately, I don't believe there's any ABI guarantee on the firmware
interface. Perhaps we can get one?

Post by Thierry Reding

Post by Bart Tanghe
+static const struct of_device_id bcm2835_pwm_of_match[] = {
+ { .compatible = "bcrm,pwm-bcm2835", },

s/bcrm/brcm/

Probably swap the order, so "brcm,bcm2835-pwm". That would be consistent
with all the other HW on this SoC.
--
To unsubscribe from this list: send the line "unsubscribe linux-pwm" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Thierry Reding

2014-09-26 07:11:10 UTC

Permalink

No problem. Thanks for the feedback.
I've got some question below.

Post by Thierry Reding
Sorry for taking so long to reply to this, I had completely forgotten.

I think the correct capitalization would be "Raspberry Pi".

Post by Bart Tanghe
+ Only 1 channel is implemented.

How many can it take? Why haven't all been implemented?

BCM2835 can take 2 pwm channels.
I can implement 2 channels but can't physically test the second channel. Is that a problem?

I don't think that's a problem; I would expect the channels to be identical,
so testing 1 should be fine.

Agreed. If it turns out not to work it can always be fixed.

Oh dear. It sounds like we need at least some form of clock driver for the
platform then. I still don't think there's complete documentation for the
HW, even though a lot of register docs were published which presumably cover
the clock HW? Equally, given that the VC firmware assumes it owns most of
the HW, it seems best to manipulate the clocks through the firmware
interface rather than directly touching the HW. Unfortunately, I don't
believe there's any ABI guarantee on the firmware interface. Perhaps we can
get one?

Urgs... this VC firmware seems to be more of a headache that I thought
it was. How is this handled in other drivers? Surely PWM isn't the first
one that needs clocks?

Post by Thierry Reding

Post by Bart Tanghe
+static const struct of_device_id bcm2835_pwm_of_match[] = {
+ { .compatible = "bcrm,pwm-bcm2835", },

s/bcrm/brcm/

Probably swap the order, so "brcm,bcm2835-pwm". That would be consistent
with all the other HW on this SoC.

Yes, please.

Thierry

Bart Tanghe

2014-09-26 10:06:52 UTC

Permalink

Post by Thierry Reding

No problem. Thanks for the feedback.
I've got some question below.

Post by Thierry Reding
Sorry for taking so long to reply to this, I had completely forgotten.

I think the correct capitalization would be "Raspberry Pi".

Post by Bart Tanghe
+ Only 1 channel is implemented.

How many can it take? Why haven't all been implemented?

BCM2835 can take 2 pwm channels.
I can implement 2 channels but can't physically test the second channel. Is that a problem?

I don't think that's a problem; I would expect the channels to be identical,
so testing 1 should be fine.

Agreed. If it turns out not to work it can always be fixed.

Oh dear. It sounds like we need at least some form of clock driver for the
platform then. I still don't think there's complete documentation for the
HW, even though a lot of register docs were published which presumably cover
the clock HW? Equally, given that the VC firmware assumes it owns most of
the HW, it seems best to manipulate the clocks through the firmware
interface rather than directly touching the HW. Unfortunately, I don't
believe there's any ABI guarantee on the firmware interface. Perhaps we can
get one?

Urgs... this VC firmware seems to be more of a headache that I thought
it was. How is this handled in other drivers? Surely PWM isn't the first
one that needs clocks?

I've looked at the i2c, spi and uart clocks. They seems default enabled, or enabled at boot by the firmware.
The clock dividers are accessible in the address range of the block in contrast to the pwm clock.
In the firmware documentation, I can't find any reference to the pwm clock.

Post by Thierry Reding

Post by Bart Tanghe
+static const struct of_device_id bcm2835_pwm_of_match[] = {
+ { .compatible = "bcrm,pwm-bcm2835", },

s/bcrm/brcm/

Probably swap the order, so "brcm,bcm2835-pwm". That would be consistent
with all the other HW on this SoC.

Yes, please.
Thierry

Stephen Warren

2014-09-26 14:45:57 UTC

Permalink

Post by Thierry Reding

No problem. Thanks for the feedback.
I've got some question below.

Post by Thierry Reding
Sorry for taking so long to reply to this, I had completely forgotten.

I think the correct capitalization would be "Raspberry Pi".

Post by Bart Tanghe
+ Only 1 channel is implemented.

How many can it take? Why haven't all been implemented?

BCM2835 can take 2 pwm channels.
I can implement 2 channels but can't physically test the second channel. Is that a problem?

I don't think that's a problem; I would expect the channels to be identical,
so testing 1 should be fine.

Agreed. If it turns out not to work it can always be fixed.

Oh dear. It sounds like we need at least some form of clock driver for the
platform then. I still don't think there's complete documentation for the
HW, even though a lot of register docs were published which presumably cover
the clock HW? Equally, given that the VC firmware assumes it owns most of
the HW, it seems best to manipulate the clocks through the firmware
interface rather than directly touching the HW. Unfortunately, I don't
believe there's any ABI guarantee on the firmware interface. Perhaps we can
get one?

Urgs... this VC firmware seems to be more of a headache that I thought
it was. How is this handled in other drivers? Surely PWM isn't the first
one that needs clocks?

For the other clocks, I've set up dummy fixed-rate clocks in the DT
and/or "clock driver" code to satisfy references by phandle or clock
name respectively. Since the other drivers don't actually manipulate the
clock rates etc., this is enough for the drivers.

I always hoped that enough HW information would either be reverse
engineered or released to somehow disable the VC firmware, and implement
all the drivers within Linux talking to HW directly.
--
To unsubscribe from this list: send the line "unsubscribe linux-pwm" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Thierry Reding

2014-09-29 05:33:13 UTC

Permalink

[...]

Post by Thierry Reding

Oh dear. It sounds like we need at least some form of clock driver for the
platform then. I still don't think there's complete documentation for the
HW, even though a lot of register docs were published which presumably cover
the clock HW? Equally, given that the VC firmware assumes it owns most of
the HW, it seems best to manipulate the clocks through the firmware
interface rather than directly touching the HW. Unfortunately, I don't
believe there's any ABI guarantee on the firmware interface. Perhaps we can
get one?

Urgs... this VC firmware seems to be more of a headache that I thought
it was. How is this handled in other drivers? Surely PWM isn't the first
one that needs clocks?

Given that this driver only queries the clock frequency adding a fixed-
rate clock to the device tree should work as well. Then the calls to
clk_prepare_enable() and clk_disable_unprepare() can still be added as
appropriate so that the driver doesn't need to change if a proper clock
driver ever gets added.

Or am I missing anything? Perhaps the issue is that the default clock
rate for the PWM clock isn't usable? That would still not prevent the
driver from being merged.

Thierry

Bart Tanghe

2014-09-29 13:37:50 UTC

Permalink

Post by Thierry Reding
[...]

Post by Thierry Reding

Oh dear. It sounds like we need at least some form of clock driver for the
platform then. I still don't think there's complete documentation for the
HW, even though a lot of register docs were published which presumably cover
the clock HW? Equally, given that the VC firmware assumes it owns most of
the HW, it seems best to manipulate the clocks through the firmware
interface rather than directly touching the HW. Unfortunately, I don't
believe there's any ABI guarantee on the firmware interface. Perhaps we can
get one?

Urgs... this VC firmware seems to be more of a headache that I thought
it was. How is this handled in other drivers? Surely PWM isn't the first
one that needs clocks?

Given that this driver only queries the clock frequency adding a fixed-
rate clock to the device tree should work as well. Then the calls to
clk_prepare_enable() and clk_disable_unprepare() can still be added as
appropriate so that the driver doesn't need to change if a proper clock
driver ever gets added.
Or am I missing anything? Perhaps the issue is that the default clock
rate for the PWM clock isn't usable? That would still not prevent the
driver from being merged.
Thierry

The pwm clock is default unusable. To let the pwm clock run, It's necessary to change some
clock registers. I've added the clk_prepare_enable and the clk_disable_unprepare functions. So the
driver is able to work with a proper clock driver in the future.

Bart

Thierry Reding

2014-09-29 14:18:21 UTC

Permalink

Post by Bart Tanghe

Post by Thierry Reding
[...]

Post by Thierry Reding

Oh dear. It sounds like we need at least some form of clock driver for the
platform then. I still don't think there's complete documentation for the
HW, even though a lot of register docs were published which presumably cover
the clock HW? Equally, given that the VC firmware assumes it owns most of
the HW, it seems best to manipulate the clocks through the firmware
interface rather than directly touching the HW. Unfortunately, I don't
believe there's any ABI guarantee on the firmware interface. Perhaps we can
get one?

Urgs... this VC firmware seems to be more of a headache that I thought
it was. How is this handled in other drivers? Surely PWM isn't the first
one that needs clocks?

Given that this driver only queries the clock frequency adding a fixed-
rate clock to the device tree should work as well. Then the calls to
clk_prepare_enable() and clk_disable_unprepare() can still be added as
appropriate so that the driver doesn't need to change if a proper clock
driver ever gets added.
Or am I missing anything? Perhaps the issue is that the default clock
rate for the PWM clock isn't usable? That would still not prevent the
driver from being merged.
Thierry

Okay. Sounds good to me.

Thierry