* [PATCH v5 1/4] gpio: mvebu: Add limited PWM support
@ 2017-04-09 18:09 ` Ralph Sennhauser
0 siblings, 0 replies; 40+ messages in thread
From: Ralph Sennhauser @ 2017-04-09 18:09 UTC (permalink / raw)
To: linux-arm-kernel
From: Andrew Lunn <andrew@lunn.ch>
Armada 370/XP devices can 'blink' GPIO lines with a configurable on
and off period. This can be modelled as a PWM.
However, there are only two sets of PWM configuration registers for
all the GPIO lines. This driver simply allows a single GPIO line per
GPIO chip of 32 lines to be used as a PWM. Attempts to use more return
EBUSY.
Due to the interleaving of registers it is not simple to separate the
PWM driver from the GPIO driver. Thus the GPIO driver has been
extended with a PWM driver.
Signed-off-by: Andrew Lunn <andrew@lunn.ch>
URL: https://patchwork.ozlabs.org/patch/427287/
URL: https://patchwork.ozlabs.org/patch/427295/
[Ralph Sennhauser:
* Port forward
* Merge PWM portion into gpio-mvebu.c
* Switch to atomic PWM API
* Add new compatible string marvell,armada-370-xp-gpio
* Update and merge documentation patch
* Update MAINTAINERS]
Signed-off-by: Ralph Sennhauser <ralph.sennhauser@gmail.com>
Tested-by: Andrew Lunn <andrew@lunn.ch>
---
.../devicetree/bindings/gpio/gpio-mvebu.txt | 32 ++
MAINTAINERS | 2 +
drivers/gpio/gpio-mvebu.c | 324 ++++++++++++++++++++-
3 files changed, 346 insertions(+), 12 deletions(-)
diff --git a/Documentation/devicetree/bindings/gpio/gpio-mvebu.txt b/Documentation/devicetree/bindings/gpio/gpio-mvebu.txt
index a6f3bec..fe49e9d 100644
--- a/Documentation/devicetree/bindings/gpio/gpio-mvebu.txt
+++ b/Documentation/devicetree/bindings/gpio/gpio-mvebu.txt
@@ -38,6 +38,24 @@ Required properties:
- #gpio-cells: Should be two. The first cell is the pin number. The
second cell is reserved for flags, unused at the moment.
+Optional properties:
+
+In order to use the gpio lines in PWM mode, some additional optional
+properties are required. Only Armada 370 and XP support these properties.
+
+- compatible: Must contain "marvell,armada-370-xp-gpio"
+
+- reg: an additional register set is needed, for the GPIO Blink
+ Counter on/off registers.
+
+- reg-names: Must contain an entry "pwm" corresponding to the
+ additional register range needed for pwm operation.
+
+- #pwm-cells: Should be two. The first cell is the GPIO line number. The
+ second cell is the period in nanoseconds.
+
+- clocks: Must be a phandle to the clock for the gpio controller.
+
Example:
gpio0: gpio at d0018100 {
@@ -51,3 +69,17 @@ Example:
#interrupt-cells = <2>;
interrupts = <16>, <17>, <18>, <19>;
};
+
+ gpio1: gpio at 18140 {
+ compatible = "marvell,armada-370-xp-gpio";
+ reg = <0x18140 0x40>, <0x181c8 0x08>;
+ reg-names = "gpio", "pwm";
+ ngpios = <17>;
+ gpio-controller;
+ #gpio-cells = <2>;
+ #pwm-cells = <2>;
+ interrupt-controller;
+ #interrupt-cells = <2>;
+ interrupts = <87>, <88>, <89>;
+ clocks = <&coreclk 0>;
+ };
diff --git a/MAINTAINERS b/MAINTAINERS
index 58b3a22..19382f5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10295,6 +10295,8 @@ F: include/linux/pwm.h
F: drivers/pwm/
F: drivers/video/backlight/pwm_bl.c
F: include/linux/pwm_backlight.h
+F: drivers/gpio/gpio-mvebu.c
+F: Documentation/devicetree/bindings/gpio/gpio-mvebu.txt
PXA2xx/PXA3xx SUPPORT
M: Daniel Mack <daniel@zonque.org>
diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c
index fae4db6..e310951 100644
--- a/drivers/gpio/gpio-mvebu.c
+++ b/drivers/gpio/gpio-mvebu.c
@@ -42,22 +42,34 @@
#include <linux/io.h>
#include <linux/of_irq.h>
#include <linux/of_device.h>
+#include <linux/pwm.h>
#include <linux/clk.h>
#include <linux/pinctrl/consumer.h>
#include <linux/irqchip/chained_irq.h>
+#include <linux/platform_device.h>
#include <linux/bitops.h>
+#include "gpiolib.h"
+
/*
* GPIO unit register offsets.
*/
-#define GPIO_OUT_OFF 0x0000
-#define GPIO_IO_CONF_OFF 0x0004
-#define GPIO_BLINK_EN_OFF 0x0008
-#define GPIO_IN_POL_OFF 0x000c
-#define GPIO_DATA_IN_OFF 0x0010
-#define GPIO_EDGE_CAUSE_OFF 0x0014
-#define GPIO_EDGE_MASK_OFF 0x0018
-#define GPIO_LEVEL_MASK_OFF 0x001c
+#define GPIO_OUT_OFF 0x0000
+#define GPIO_IO_CONF_OFF 0x0004
+#define GPIO_BLINK_EN_OFF 0x0008
+#define GPIO_IN_POL_OFF 0x000c
+#define GPIO_DATA_IN_OFF 0x0010
+#define GPIO_EDGE_CAUSE_OFF 0x0014
+#define GPIO_EDGE_MASK_OFF 0x0018
+#define GPIO_LEVEL_MASK_OFF 0x001c
+#define GPIO_BLINK_CNT_SELECT_OFF 0x0020
+
+/*
+ * PWM register offsets.
+ */
+#define PWM_BLINK_ON_DURATION_OFF 0x0
+#define PWM_BLINK_OFF_DURATION_OFF 0x4
+
/* The MV78200 has per-CPU registers for edge mask and level mask */
#define GPIO_EDGE_MASK_MV78200_OFF(cpu) ((cpu) ? 0x30 : 0x18)
@@ -78,6 +90,20 @@
#define MVEBU_MAX_GPIO_PER_BANK 32
+struct mvebu_pwm {
+ void __iomem *membase;
+ unsigned long clk_rate;
+ bool used;
+ struct pwm_chip chip;
+ spinlock_t lock;
+ struct mvebu_gpio_chip *mvchip;
+
+ /* Used to preserve GPIO/PWM registers across suspend/resume */
+ u32 blink_select;
+ u32 blink_on_duration;
+ u32 blink_off_duration;
+};
+
struct mvebu_gpio_chip {
struct gpio_chip chip;
spinlock_t lock;
@@ -87,6 +113,10 @@ struct mvebu_gpio_chip {
struct irq_domain *domain;
int soc_variant;
+ /* Used for PWM support */
+ struct clk *clk;
+ struct mvebu_pwm *mvpwm;
+
/* Used to preserve GPIO registers across suspend/resume */
u32 out_reg;
u32 io_conf_reg;
@@ -110,6 +140,12 @@ static void __iomem *mvebu_gpioreg_blink(struct mvebu_gpio_chip *mvchip)
return mvchip->membase + GPIO_BLINK_EN_OFF;
}
+static void __iomem *mvebu_gpioreg_blink_counter_select(struct mvebu_gpio_chip
+ *mvchip)
+{
+ return mvchip->membase + GPIO_BLINK_CNT_SELECT_OFF;
+}
+
static void __iomem *mvebu_gpioreg_io_conf(struct mvebu_gpio_chip *mvchip)
{
return mvchip->membase + GPIO_IO_CONF_OFF;
@@ -181,6 +217,20 @@ static void __iomem *mvebu_gpioreg_level_mask(struct mvebu_gpio_chip *mvchip)
}
/*
+ * Functions returning addresses of individual registers for a given
+ * PWM controller.
+ */
+static void __iomem *mvebu_pwmreg_blink_on_duration(struct mvebu_pwm *mvpwm)
+{
+ return mvpwm->membase + PWM_BLINK_ON_DURATION_OFF;
+}
+
+static void __iomem *mvebu_pwmreg_blink_off_duration(struct mvebu_pwm *mvpwm)
+{
+ return mvpwm->membase + PWM_BLINK_OFF_DURATION_OFF;
+}
+
+/*
* Functions implementing the gpio_chip methods
*/
static void mvebu_gpio_set(struct gpio_chip *chip, unsigned int pin, int value)
@@ -484,6 +534,243 @@ static void mvebu_gpio_irq_handler(struct irq_desc *desc)
chained_irq_exit(chip, desc);
}
+/*
+ * Functions implementing the pwm_chip methods
+ */
+static struct mvebu_pwm *to_mvebu_pwm(struct pwm_chip *chip)
+{
+ return container_of(chip, struct mvebu_pwm, chip);
+}
+
+static int mvebu_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+ struct mvebu_pwm *mvpwm = to_mvebu_pwm(chip);
+ struct gpio_desc *desc = gpio_to_desc(pwm->pwm);
+ unsigned long flags;
+ int ret = 0;
+
+ spin_lock_irqsave(&mvpwm->lock, flags);
+ if (mvpwm->used) {
+ ret = -EBUSY;
+ } else {
+ if (!desc) {
+ ret = -ENODEV;
+ goto out;
+ }
+ ret = gpiod_request(desc, "mvebu-pwm");
+ if (ret)
+ goto out;
+
+ ret = gpiod_direction_output(desc, 0);
+ if (ret) {
+ gpiod_free(desc);
+ goto out;
+ }
+
+ mvpwm->used = true;
+ }
+
+out:
+ spin_unlock_irqrestore(&mvpwm->lock, flags);
+ return ret;
+}
+
+static void mvebu_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+ struct mvebu_pwm *mvpwm = to_mvebu_pwm(chip);
+ struct gpio_desc *desc = gpio_to_desc(pwm->pwm);
+ unsigned long flags;
+
+ spin_lock_irqsave(&mvpwm->lock, flags);
+ gpiod_free(desc);
+ mvpwm->used = false;
+ spin_unlock_irqrestore(&mvpwm->lock, flags);
+}
+
+static void mvebu_pwm_get_state(struct pwm_chip *chip,
+ struct pwm_device *pwm,
+ struct pwm_state *state) {
+
+ struct mvebu_pwm *mvpwm = to_mvebu_pwm(chip);
+ struct mvebu_gpio_chip *mvchip = mvpwm->mvchip;
+ unsigned long long val;
+ unsigned long flags;
+ u32 u;
+
+ spin_lock_irqsave(&mvpwm->lock, flags);
+
+ val = (unsigned long long)
+ readl_relaxed(mvebu_pwmreg_blink_on_duration);
+ val *= NSEC_PER_SEC;
+ do_div(val, mvpwm->clk_rate);
+ if (val > UINT_MAX)
+ state->duty_cycle = UINT_MAX;
+ else if (val)
+ state->duty_cycle = val;
+ else
+ state->duty_cycle = 1;
+
+ val = (unsigned long long)
+ readl_relaxed(mvebu_pwmreg_blink_off_duration);
+ val *= NSEC_PER_SEC;
+ do_div(val, mvpwm->clk_rate);
+ if (val < state->duty_cycle) {
+ state->period = 1;
+ } else {
+ val -= state->duty_cycle;
+ if (val > UINT_MAX)
+ state->period = UINT_MAX;
+ else if (val)
+ state->period = val;
+ else
+ state->period = 1;
+ }
+
+ u = readl_relaxed(mvebu_gpioreg_blink(mvchip));
+ if (u)
+ state->enabled = true;
+ else
+ state->enabled = false;
+
+ spin_unlock_irqrestore(&mvpwm->lock, flags);
+}
+
+static int mvebu_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+ struct pwm_state *state)
+{
+ struct mvebu_pwm *mvpwm = to_mvebu_pwm(chip);
+ struct mvebu_gpio_chip *mvchip = mvpwm->mvchip;
+ unsigned long long val;
+ unsigned long flags;
+ unsigned int on, off;
+
+ val = (unsigned long long) mvpwm->clk_rate * state->duty_cycle;
+ do_div(val, NSEC_PER_SEC);
+ if (val > UINT_MAX)
+ return -EINVAL;
+ if (val)
+ on = val;
+ else
+ on = 1;
+
+ val = (unsigned long long) mvpwm->clk_rate *
+ (state->period - state->duty_cycle);
+ do_div(val, NSEC_PER_SEC);
+ if (val > UINT_MAX)
+ return -EINVAL;
+ if (val)
+ off = val;
+ else
+ off = 1;
+
+ spin_lock_irqsave(&mvpwm->lock, flags);
+
+ writel_relaxed(on, mvebu_pwmreg_blink_on_duration(mvpwm));
+ writel_relaxed(off, mvebu_pwmreg_blink_off_duration(mvpwm));
+ if (state->enabled)
+ mvebu_gpio_blink(&mvchip->chip, pwm->hwpwm, 1);
+ else
+ mvebu_gpio_blink(&mvchip->chip, pwm->hwpwm, 0);
+
+ spin_unlock_irqrestore(&mvpwm->lock, flags);
+
+ return 0;
+}
+
+static const struct pwm_ops mvebu_pwm_ops = {
+ .request = mvebu_pwm_request,
+ .free = mvebu_pwm_free,
+ .get_state = mvebu_pwm_get_state,
+ .apply = mvebu_pwm_apply,
+ .owner = THIS_MODULE,
+};
+
+static void __maybe_unused mvebu_pwm_suspend(struct mvebu_gpio_chip *mvchip)
+{
+ struct mvebu_pwm *mvpwm = mvchip->mvpwm;
+
+ mvpwm->blink_select =
+ readl_relaxed(mvebu_gpioreg_blink_counter_select(mvchip));
+ mvpwm->blink_on_duration =
+ readl_relaxed(mvebu_pwmreg_blink_on_duration(mvpwm));
+ mvpwm->blink_off_duration =
+ readl_relaxed(mvebu_pwmreg_blink_off_duration(mvpwm));
+}
+
+static void __maybe_unused mvebu_pwm_resume(struct mvebu_gpio_chip *mvchip)
+{
+ struct mvebu_pwm *mvpwm = mvchip->mvpwm;
+
+ writel_relaxed(mvpwm->blink_select,
+ mvebu_gpioreg_blink_counter_select(mvchip));
+ writel_relaxed(mvpwm->blink_on_duration,
+ mvebu_pwmreg_blink_on_duration(mvpwm));
+ writel_relaxed(mvpwm->blink_off_duration,
+ mvebu_pwmreg_blink_off_duration(mvpwm));
+}
+
+static int mvebu_pwm_probe(struct platform_device *pdev,
+ struct mvebu_gpio_chip *mvchip,
+ int id)
+{
+ struct device *dev = &pdev->dev;
+ struct mvebu_pwm *mvpwm;
+ struct resource *res;
+
+ if (!of_device_is_compatible(mvchip->chip.of_node,
+ "marvell,armada-370-xp-gpio"))
+ return 0;
+ /*
+ * There are only two sets of PWM configuration registers for
+ * all the GPIO lines on those SoCs which this driver reserves
+ * for the first two GPIO chips. So if the resource is missing
+ * we can't treat it as an error.
+ */
+ res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "pwm");
+ if (!res)
+ return 0;
+
+ /*
+ * Use set A for lines of GPIO chip with id 0, B for GPIO chip
+ * with id 1. Don't allow further GPIO chips to be used for PWM.
+ */
+ if (id == 0)
+ writel_relaxed(0, mvebu_gpioreg_blink_counter_select(mvchip));
+ else if (id == 1)
+ writel_relaxed(U32_MAX,
+ mvebu_gpioreg_blink_counter_select(mvchip));
+ else
+ return -EINVAL;
+
+ mvpwm = devm_kzalloc(dev, sizeof(struct mvebu_pwm), GFP_KERNEL);
+ if (!mvpwm)
+ return -ENOMEM;
+ mvchip->mvpwm = mvpwm;
+ mvpwm->mvchip = mvchip;
+
+ mvpwm->membase = devm_ioremap_resource(dev, res);
+ if (IS_ERR(mvpwm->membase))
+ return PTR_ERR(mvpwm->membase);
+
+ if (IS_ERR(mvchip->clk))
+ return PTR_ERR(mvchip->clk);
+
+ mvpwm->clk_rate = clk_get_rate(mvchip->clk);
+ if (!mvpwm->clk_rate) {
+ dev_err(dev, "failed to get clock rate\n");
+ return -EINVAL;
+ }
+
+ mvpwm->chip.dev = dev;
+ mvpwm->chip.ops = &mvebu_pwm_ops;
+ mvpwm->chip.base = mvchip->chip.base;
+ mvpwm->chip.npwm = mvchip->chip.ngpio;
+
+ spin_lock_init(&mvpwm->lock);
+
+ return pwmchip_add(&mvpwm->chip);
+}
+
#ifdef CONFIG_DEBUG_FS
#include <linux/seq_file.h>
@@ -555,6 +842,10 @@ static const struct of_device_id mvebu_gpio_of_match[] = {
.data = (void *) MVEBU_GPIO_SOC_VARIANT_ARMADAXP,
},
{
+ .compatible = "marvell,armada-370-xp-gpio",
+ .data = (void *) MVEBU_GPIO_SOC_VARIANT_ORION,
+ },
+ {
/* sentinel */
},
};
@@ -600,6 +891,9 @@ static int mvebu_gpio_suspend(struct platform_device *pdev, pm_message_t state)
BUG();
}
+ if (IS_ENABLED(CONFIG_PWM))
+ mvebu_pwm_suspend(mvchip);
+
return 0;
}
@@ -643,6 +937,9 @@ static int mvebu_gpio_resume(struct platform_device *pdev)
BUG();
}
+ if (IS_ENABLED(CONFIG_PWM))
+ mvebu_pwm_resume(mvchip);
+
return 0;
}
@@ -654,7 +951,6 @@ static int mvebu_gpio_probe(struct platform_device *pdev)
struct resource *res;
struct irq_chip_generic *gc;
struct irq_chip_type *ct;
- struct clk *clk;
unsigned int ngpios;
bool have_irqs;
int soc_variant;
@@ -688,10 +984,10 @@ static int mvebu_gpio_probe(struct platform_device *pdev)
return id;
}
- clk = devm_clk_get(&pdev->dev, NULL);
+ mvchip->clk = devm_clk_get(&pdev->dev, NULL);
/* Not all SoCs require a clock.*/
- if (!IS_ERR(clk))
- clk_prepare_enable(clk);
+ if (!IS_ERR(mvchip->clk))
+ clk_prepare_enable(mvchip->clk);
mvchip->soc_variant = soc_variant;
mvchip->chip.label = dev_name(&pdev->dev);
@@ -822,6 +1118,10 @@ static int mvebu_gpio_probe(struct platform_device *pdev)
mvchip);
}
+ /* Armada 370/XP has simple PWM support for GPIO lines */
+ if (IS_ENABLED(CONFIG_PWM))
+ return mvebu_pwm_probe(pdev, mvchip, id);
+
return 0;
err_domain:
--
2.10.2
^ permalink raw reply related [flat|nested] 40+ messages in thread* [PATCH v5 1/4] gpio: mvebu: Add limited PWM support
@ 2017-04-09 18:09 ` Ralph Sennhauser
0 siblings, 0 replies; 40+ messages in thread
From: Ralph Sennhauser @ 2017-04-09 18:09 UTC (permalink / raw)
To: Thierry Reding
Cc: Andrew Lunn, Ralph Sennhauser, Linus Walleij, Alexandre Courbot,
Rob Herring, Mark Rutland, Jason Cooper, Gregory Clement,
Sebastian Hesselbarth, Russell King, linux-pwm, linux-gpio,
devicetree, linux-kernel, linux-arm-kernel
From: Andrew Lunn <andrew@lunn.ch>
Armada 370/XP devices can 'blink' GPIO lines with a configurable on
and off period. This can be modelled as a PWM.
However, there are only two sets of PWM configuration registers for
all the GPIO lines. This driver simply allows a single GPIO line per
GPIO chip of 32 lines to be used as a PWM. Attempts to use more return
EBUSY.
Due to the interleaving of registers it is not simple to separate the
PWM driver from the GPIO driver. Thus the GPIO driver has been
extended with a PWM driver.
Signed-off-by: Andrew Lunn <andrew@lunn.ch>
URL: https://patchwork.ozlabs.org/patch/427287/
URL: https://patchwork.ozlabs.org/patch/427295/
[Ralph Sennhauser:
* Port forward
* Merge PWM portion into gpio-mvebu.c
* Switch to atomic PWM API
* Add new compatible string marvell,armada-370-xp-gpio
* Update and merge documentation patch
* Update MAINTAINERS]
Signed-off-by: Ralph Sennhauser <ralph.sennhauser@gmail.com>
Tested-by: Andrew Lunn <andrew@lunn.ch>
---
.../devicetree/bindings/gpio/gpio-mvebu.txt | 32 ++
MAINTAINERS | 2 +
drivers/gpio/gpio-mvebu.c | 324 ++++++++++++++++++++-
3 files changed, 346 insertions(+), 12 deletions(-)
diff --git a/Documentation/devicetree/bindings/gpio/gpio-mvebu.txt b/Documentation/devicetree/bindings/gpio/gpio-mvebu.txt
index a6f3bec..fe49e9d 100644
--- a/Documentation/devicetree/bindings/gpio/gpio-mvebu.txt
+++ b/Documentation/devicetree/bindings/gpio/gpio-mvebu.txt
@@ -38,6 +38,24 @@ Required properties:
- #gpio-cells: Should be two. The first cell is the pin number. The
second cell is reserved for flags, unused at the moment.
+Optional properties:
+
+In order to use the gpio lines in PWM mode, some additional optional
+properties are required. Only Armada 370 and XP support these properties.
+
+- compatible: Must contain "marvell,armada-370-xp-gpio"
+
+- reg: an additional register set is needed, for the GPIO Blink
+ Counter on/off registers.
+
+- reg-names: Must contain an entry "pwm" corresponding to the
+ additional register range needed for pwm operation.
+
+- #pwm-cells: Should be two. The first cell is the GPIO line number. The
+ second cell is the period in nanoseconds.
+
+- clocks: Must be a phandle to the clock for the gpio controller.
+
Example:
gpio0: gpio@d0018100 {
@@ -51,3 +69,17 @@ Example:
#interrupt-cells = <2>;
interrupts = <16>, <17>, <18>, <19>;
};
+
+ gpio1: gpio@18140 {
+ compatible = "marvell,armada-370-xp-gpio";
+ reg = <0x18140 0x40>, <0x181c8 0x08>;
+ reg-names = "gpio", "pwm";
+ ngpios = <17>;
+ gpio-controller;
+ #gpio-cells = <2>;
+ #pwm-cells = <2>;
+ interrupt-controller;
+ #interrupt-cells = <2>;
+ interrupts = <87>, <88>, <89>;
+ clocks = <&coreclk 0>;
+ };
diff --git a/MAINTAINERS b/MAINTAINERS
index 58b3a22..19382f5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10295,6 +10295,8 @@ F: include/linux/pwm.h
F: drivers/pwm/
F: drivers/video/backlight/pwm_bl.c
F: include/linux/pwm_backlight.h
+F: drivers/gpio/gpio-mvebu.c
+F: Documentation/devicetree/bindings/gpio/gpio-mvebu.txt
PXA2xx/PXA3xx SUPPORT
M: Daniel Mack <daniel@zonque.org>
diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c
index fae4db6..e310951 100644
--- a/drivers/gpio/gpio-mvebu.c
+++ b/drivers/gpio/gpio-mvebu.c
@@ -42,22 +42,34 @@
#include <linux/io.h>
#include <linux/of_irq.h>
#include <linux/of_device.h>
+#include <linux/pwm.h>
#include <linux/clk.h>
#include <linux/pinctrl/consumer.h>
#include <linux/irqchip/chained_irq.h>
+#include <linux/platform_device.h>
#include <linux/bitops.h>
+#include "gpiolib.h"
+
/*
* GPIO unit register offsets.
*/
-#define GPIO_OUT_OFF 0x0000
-#define GPIO_IO_CONF_OFF 0x0004
-#define GPIO_BLINK_EN_OFF 0x0008
-#define GPIO_IN_POL_OFF 0x000c
-#define GPIO_DATA_IN_OFF 0x0010
-#define GPIO_EDGE_CAUSE_OFF 0x0014
-#define GPIO_EDGE_MASK_OFF 0x0018
-#define GPIO_LEVEL_MASK_OFF 0x001c
+#define GPIO_OUT_OFF 0x0000
+#define GPIO_IO_CONF_OFF 0x0004
+#define GPIO_BLINK_EN_OFF 0x0008
+#define GPIO_IN_POL_OFF 0x000c
+#define GPIO_DATA_IN_OFF 0x0010
+#define GPIO_EDGE_CAUSE_OFF 0x0014
+#define GPIO_EDGE_MASK_OFF 0x0018
+#define GPIO_LEVEL_MASK_OFF 0x001c
+#define GPIO_BLINK_CNT_SELECT_OFF 0x0020
+
+/*
+ * PWM register offsets.
+ */
+#define PWM_BLINK_ON_DURATION_OFF 0x0
+#define PWM_BLINK_OFF_DURATION_OFF 0x4
+
/* The MV78200 has per-CPU registers for edge mask and level mask */
#define GPIO_EDGE_MASK_MV78200_OFF(cpu) ((cpu) ? 0x30 : 0x18)
@@ -78,6 +90,20 @@
#define MVEBU_MAX_GPIO_PER_BANK 32
+struct mvebu_pwm {
+ void __iomem *membase;
+ unsigned long clk_rate;
+ bool used;
+ struct pwm_chip chip;
+ spinlock_t lock;
+ struct mvebu_gpio_chip *mvchip;
+
+ /* Used to preserve GPIO/PWM registers across suspend/resume */
+ u32 blink_select;
+ u32 blink_on_duration;
+ u32 blink_off_duration;
+};
+
struct mvebu_gpio_chip {
struct gpio_chip chip;
spinlock_t lock;
@@ -87,6 +113,10 @@ struct mvebu_gpio_chip {
struct irq_domain *domain;
int soc_variant;
+ /* Used for PWM support */
+ struct clk *clk;
+ struct mvebu_pwm *mvpwm;
+
/* Used to preserve GPIO registers across suspend/resume */
u32 out_reg;
u32 io_conf_reg;
@@ -110,6 +140,12 @@ static void __iomem *mvebu_gpioreg_blink(struct mvebu_gpio_chip *mvchip)
return mvchip->membase + GPIO_BLINK_EN_OFF;
}
+static void __iomem *mvebu_gpioreg_blink_counter_select(struct mvebu_gpio_chip
+ *mvchip)
+{
+ return mvchip->membase + GPIO_BLINK_CNT_SELECT_OFF;
+}
+
static void __iomem *mvebu_gpioreg_io_conf(struct mvebu_gpio_chip *mvchip)
{
return mvchip->membase + GPIO_IO_CONF_OFF;
@@ -181,6 +217,20 @@ static void __iomem *mvebu_gpioreg_level_mask(struct mvebu_gpio_chip *mvchip)
}
/*
+ * Functions returning addresses of individual registers for a given
+ * PWM controller.
+ */
+static void __iomem *mvebu_pwmreg_blink_on_duration(struct mvebu_pwm *mvpwm)
+{
+ return mvpwm->membase + PWM_BLINK_ON_DURATION_OFF;
+}
+
+static void __iomem *mvebu_pwmreg_blink_off_duration(struct mvebu_pwm *mvpwm)
+{
+ return mvpwm->membase + PWM_BLINK_OFF_DURATION_OFF;
+}
+
+/*
* Functions implementing the gpio_chip methods
*/
static void mvebu_gpio_set(struct gpio_chip *chip, unsigned int pin, int value)
@@ -484,6 +534,243 @@ static void mvebu_gpio_irq_handler(struct irq_desc *desc)
chained_irq_exit(chip, desc);
}
+/*
+ * Functions implementing the pwm_chip methods
+ */
+static struct mvebu_pwm *to_mvebu_pwm(struct pwm_chip *chip)
+{
+ return container_of(chip, struct mvebu_pwm, chip);
+}
+
+static int mvebu_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+ struct mvebu_pwm *mvpwm = to_mvebu_pwm(chip);
+ struct gpio_desc *desc = gpio_to_desc(pwm->pwm);
+ unsigned long flags;
+ int ret = 0;
+
+ spin_lock_irqsave(&mvpwm->lock, flags);
+ if (mvpwm->used) {
+ ret = -EBUSY;
+ } else {
+ if (!desc) {
+ ret = -ENODEV;
+ goto out;
+ }
+ ret = gpiod_request(desc, "mvebu-pwm");
+ if (ret)
+ goto out;
+
+ ret = gpiod_direction_output(desc, 0);
+ if (ret) {
+ gpiod_free(desc);
+ goto out;
+ }
+
+ mvpwm->used = true;
+ }
+
+out:
+ spin_unlock_irqrestore(&mvpwm->lock, flags);
+ return ret;
+}
+
+static void mvebu_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+ struct mvebu_pwm *mvpwm = to_mvebu_pwm(chip);
+ struct gpio_desc *desc = gpio_to_desc(pwm->pwm);
+ unsigned long flags;
+
+ spin_lock_irqsave(&mvpwm->lock, flags);
+ gpiod_free(desc);
+ mvpwm->used = false;
+ spin_unlock_irqrestore(&mvpwm->lock, flags);
+}
+
+static void mvebu_pwm_get_state(struct pwm_chip *chip,
+ struct pwm_device *pwm,
+ struct pwm_state *state) {
+
+ struct mvebu_pwm *mvpwm = to_mvebu_pwm(chip);
+ struct mvebu_gpio_chip *mvchip = mvpwm->mvchip;
+ unsigned long long val;
+ unsigned long flags;
+ u32 u;
+
+ spin_lock_irqsave(&mvpwm->lock, flags);
+
+ val = (unsigned long long)
+ readl_relaxed(mvebu_pwmreg_blink_on_duration);
+ val *= NSEC_PER_SEC;
+ do_div(val, mvpwm->clk_rate);
+ if (val > UINT_MAX)
+ state->duty_cycle = UINT_MAX;
+ else if (val)
+ state->duty_cycle = val;
+ else
+ state->duty_cycle = 1;
+
+ val = (unsigned long long)
+ readl_relaxed(mvebu_pwmreg_blink_off_duration);
+ val *= NSEC_PER_SEC;
+ do_div(val, mvpwm->clk_rate);
+ if (val < state->duty_cycle) {
+ state->period = 1;
+ } else {
+ val -= state->duty_cycle;
+ if (val > UINT_MAX)
+ state->period = UINT_MAX;
+ else if (val)
+ state->period = val;
+ else
+ state->period = 1;
+ }
+
+ u = readl_relaxed(mvebu_gpioreg_blink(mvchip));
+ if (u)
+ state->enabled = true;
+ else
+ state->enabled = false;
+
+ spin_unlock_irqrestore(&mvpwm->lock, flags);
+}
+
+static int mvebu_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+ struct pwm_state *state)
+{
+ struct mvebu_pwm *mvpwm = to_mvebu_pwm(chip);
+ struct mvebu_gpio_chip *mvchip = mvpwm->mvchip;
+ unsigned long long val;
+ unsigned long flags;
+ unsigned int on, off;
+
+ val = (unsigned long long) mvpwm->clk_rate * state->duty_cycle;
+ do_div(val, NSEC_PER_SEC);
+ if (val > UINT_MAX)
+ return -EINVAL;
+ if (val)
+ on = val;
+ else
+ on = 1;
+
+ val = (unsigned long long) mvpwm->clk_rate *
+ (state->period - state->duty_cycle);
+ do_div(val, NSEC_PER_SEC);
+ if (val > UINT_MAX)
+ return -EINVAL;
+ if (val)
+ off = val;
+ else
+ off = 1;
+
+ spin_lock_irqsave(&mvpwm->lock, flags);
+
+ writel_relaxed(on, mvebu_pwmreg_blink_on_duration(mvpwm));
+ writel_relaxed(off, mvebu_pwmreg_blink_off_duration(mvpwm));
+ if (state->enabled)
+ mvebu_gpio_blink(&mvchip->chip, pwm->hwpwm, 1);
+ else
+ mvebu_gpio_blink(&mvchip->chip, pwm->hwpwm, 0);
+
+ spin_unlock_irqrestore(&mvpwm->lock, flags);
+
+ return 0;
+}
+
+static const struct pwm_ops mvebu_pwm_ops = {
+ .request = mvebu_pwm_request,
+ .free = mvebu_pwm_free,
+ .get_state = mvebu_pwm_get_state,
+ .apply = mvebu_pwm_apply,
+ .owner = THIS_MODULE,
+};
+
+static void __maybe_unused mvebu_pwm_suspend(struct mvebu_gpio_chip *mvchip)
+{
+ struct mvebu_pwm *mvpwm = mvchip->mvpwm;
+
+ mvpwm->blink_select =
+ readl_relaxed(mvebu_gpioreg_blink_counter_select(mvchip));
+ mvpwm->blink_on_duration =
+ readl_relaxed(mvebu_pwmreg_blink_on_duration(mvpwm));
+ mvpwm->blink_off_duration =
+ readl_relaxed(mvebu_pwmreg_blink_off_duration(mvpwm));
+}
+
+static void __maybe_unused mvebu_pwm_resume(struct mvebu_gpio_chip *mvchip)
+{
+ struct mvebu_pwm *mvpwm = mvchip->mvpwm;
+
+ writel_relaxed(mvpwm->blink_select,
+ mvebu_gpioreg_blink_counter_select(mvchip));
+ writel_relaxed(mvpwm->blink_on_duration,
+ mvebu_pwmreg_blink_on_duration(mvpwm));
+ writel_relaxed(mvpwm->blink_off_duration,
+ mvebu_pwmreg_blink_off_duration(mvpwm));
+}
+
+static int mvebu_pwm_probe(struct platform_device *pdev,
+ struct mvebu_gpio_chip *mvchip,
+ int id)
+{
+ struct device *dev = &pdev->dev;
+ struct mvebu_pwm *mvpwm;
+ struct resource *res;
+
+ if (!of_device_is_compatible(mvchip->chip.of_node,
+ "marvell,armada-370-xp-gpio"))
+ return 0;
+ /*
+ * There are only two sets of PWM configuration registers for
+ * all the GPIO lines on those SoCs which this driver reserves
+ * for the first two GPIO chips. So if the resource is missing
+ * we can't treat it as an error.
+ */
+ res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "pwm");
+ if (!res)
+ return 0;
+
+ /*
+ * Use set A for lines of GPIO chip with id 0, B for GPIO chip
+ * with id 1. Don't allow further GPIO chips to be used for PWM.
+ */
+ if (id == 0)
+ writel_relaxed(0, mvebu_gpioreg_blink_counter_select(mvchip));
+ else if (id == 1)
+ writel_relaxed(U32_MAX,
+ mvebu_gpioreg_blink_counter_select(mvchip));
+ else
+ return -EINVAL;
+
+ mvpwm = devm_kzalloc(dev, sizeof(struct mvebu_pwm), GFP_KERNEL);
+ if (!mvpwm)
+ return -ENOMEM;
+ mvchip->mvpwm = mvpwm;
+ mvpwm->mvchip = mvchip;
+
+ mvpwm->membase = devm_ioremap_resource(dev, res);
+ if (IS_ERR(mvpwm->membase))
+ return PTR_ERR(mvpwm->membase);
+
+ if (IS_ERR(mvchip->clk))
+ return PTR_ERR(mvchip->clk);
+
+ mvpwm->clk_rate = clk_get_rate(mvchip->clk);
+ if (!mvpwm->clk_rate) {
+ dev_err(dev, "failed to get clock rate\n");
+ return -EINVAL;
+ }
+
+ mvpwm->chip.dev = dev;
+ mvpwm->chip.ops = &mvebu_pwm_ops;
+ mvpwm->chip.base = mvchip->chip.base;
+ mvpwm->chip.npwm = mvchip->chip.ngpio;
+
+ spin_lock_init(&mvpwm->lock);
+
+ return pwmchip_add(&mvpwm->chip);
+}
+
#ifdef CONFIG_DEBUG_FS
#include <linux/seq_file.h>
@@ -555,6 +842,10 @@ static const struct of_device_id mvebu_gpio_of_match[] = {
.data = (void *) MVEBU_GPIO_SOC_VARIANT_ARMADAXP,
},
{
+ .compatible = "marvell,armada-370-xp-gpio",
+ .data = (void *) MVEBU_GPIO_SOC_VARIANT_ORION,
+ },
+ {
/* sentinel */
},
};
@@ -600,6 +891,9 @@ static int mvebu_gpio_suspend(struct platform_device *pdev, pm_message_t state)
BUG();
}
+ if (IS_ENABLED(CONFIG_PWM))
+ mvebu_pwm_suspend(mvchip);
+
return 0;
}
@@ -643,6 +937,9 @@ static int mvebu_gpio_resume(struct platform_device *pdev)
BUG();
}
+ if (IS_ENABLED(CONFIG_PWM))
+ mvebu_pwm_resume(mvchip);
+
return 0;
}
@@ -654,7 +951,6 @@ static int mvebu_gpio_probe(struct platform_device *pdev)
struct resource *res;
struct irq_chip_generic *gc;
struct irq_chip_type *ct;
- struct clk *clk;
unsigned int ngpios;
bool have_irqs;
int soc_variant;
@@ -688,10 +984,10 @@ static int mvebu_gpio_probe(struct platform_device *pdev)
return id;
}
- clk = devm_clk_get(&pdev->dev, NULL);
+ mvchip->clk = devm_clk_get(&pdev->dev, NULL);
/* Not all SoCs require a clock.*/
- if (!IS_ERR(clk))
- clk_prepare_enable(clk);
+ if (!IS_ERR(mvchip->clk))
+ clk_prepare_enable(mvchip->clk);
mvchip->soc_variant = soc_variant;
mvchip->chip.label = dev_name(&pdev->dev);
@@ -822,6 +1118,10 @@ static int mvebu_gpio_probe(struct platform_device *pdev)
mvchip);
}
+ /* Armada 370/XP has simple PWM support for GPIO lines */
+ if (IS_ENABLED(CONFIG_PWM))
+ return mvebu_pwm_probe(pdev, mvchip, id);
+
return 0;
err_domain:
--
2.10.2
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [PATCH v5 1/4] gpio: mvebu: Add limited PWM support
2017-04-09 18:09 ` Ralph Sennhauser
@ 2017-04-12 14:31 ` Thomas Petazzoni
-1 siblings, 0 replies; 40+ messages in thread
From: Thomas Petazzoni @ 2017-04-12 14:31 UTC (permalink / raw)
To: Ralph Sennhauser
Cc: Thierry Reding, Mark Rutland, Andrew Lunn, Jason Cooper,
Alexandre Courbot, Linus Walleij, Russell King, linux-pwm,
linux-kernel, Gregory Clement, devicetree, Rob Herring,
linux-gpio, linux-arm-kernel, Sebastian Hesselbarth
Hello,
Sorry for the late feedback about this.
On Sun, 9 Apr 2017 20:09:27 +0200, Ralph Sennhauser wrote:
> + gpio1: gpio@18140 {
> + compatible = "marvell,armada-370-xp-gpio";
> + reg = <0x18140 0x40>, <0x181c8 0x08>;
One issue I see is that you have only two counters A and B. You
associate counter A with the first bank of GPIOs, and counter B with
the second bank of GPIOs.
Which means that if you need to PWM a GPIO from the third bank of
GPIOs, you can't, even if the HW allows it.
While I'm fine with not supporting all the HW features, but it's a bit
sad that this gets encoded into the DT.
But I guess the only way to make this possible would be to have a
single node for all GPIOs rather than one per bank? Or do we have a way
to have those counter A/B registers bound to a separate PWM driver, and
then the GPIO driver being smart enough to select the counter to be
used? Seems not easy to do though :-/
> +struct mvebu_pwm {
> + void __iomem *membase;
> + unsigned long clk_rate;
> + bool used;
> + struct pwm_chip chip;
> + spinlock_t lock;
> + struct mvebu_gpio_chip *mvchip;
> +
> + /* Used to preserve GPIO/PWM registers across suspend/resume */
> + u32 blink_select;
> + u32 blink_on_duration;
> + u32 blink_off_duration;
> +};
> +
> struct mvebu_gpio_chip {
> struct gpio_chip chip;
> spinlock_t lock;
> @@ -87,6 +113,10 @@ struct mvebu_gpio_chip {
> struct irq_domain *domain;
> int soc_variant;
>
> + /* Used for PWM support */
> + struct clk *clk;
> + struct mvebu_pwm *mvpwm;
Why does mvpwm needs to be allocated separately? Why not directly embed
it inside the mvebu_gpio_chip structure?
Do we need a separate spinlock?
> @@ -555,6 +842,10 @@ static const struct of_device_id mvebu_gpio_of_match[] = {
> .data = (void *) MVEBU_GPIO_SOC_VARIANT_ARMADAXP,
> },
> {
> + .compatible = "marvell,armada-370-xp-gpio",
> + .data = (void *) MVEBU_GPIO_SOC_VARIANT_ORION,
Whum, what are you doing here?
Thanks,
Thomas
--
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply [flat|nested] 40+ messages in thread* [PATCH v5 1/4] gpio: mvebu: Add limited PWM support
@ 2017-04-12 14:31 ` Thomas Petazzoni
0 siblings, 0 replies; 40+ messages in thread
From: Thomas Petazzoni @ 2017-04-12 14:31 UTC (permalink / raw)
To: linux-arm-kernel
Hello,
Sorry for the late feedback about this.
On Sun, 9 Apr 2017 20:09:27 +0200, Ralph Sennhauser wrote:
> + gpio1: gpio at 18140 {
> + compatible = "marvell,armada-370-xp-gpio";
> + reg = <0x18140 0x40>, <0x181c8 0x08>;
One issue I see is that you have only two counters A and B. You
associate counter A with the first bank of GPIOs, and counter B with
the second bank of GPIOs.
Which means that if you need to PWM a GPIO from the third bank of
GPIOs, you can't, even if the HW allows it.
While I'm fine with not supporting all the HW features, but it's a bit
sad that this gets encoded into the DT.
But I guess the only way to make this possible would be to have a
single node for all GPIOs rather than one per bank? Or do we have a way
to have those counter A/B registers bound to a separate PWM driver, and
then the GPIO driver being smart enough to select the counter to be
used? Seems not easy to do though :-/
> +struct mvebu_pwm {
> + void __iomem *membase;
> + unsigned long clk_rate;
> + bool used;
> + struct pwm_chip chip;
> + spinlock_t lock;
> + struct mvebu_gpio_chip *mvchip;
> +
> + /* Used to preserve GPIO/PWM registers across suspend/resume */
> + u32 blink_select;
> + u32 blink_on_duration;
> + u32 blink_off_duration;
> +};
> +
> struct mvebu_gpio_chip {
> struct gpio_chip chip;
> spinlock_t lock;
> @@ -87,6 +113,10 @@ struct mvebu_gpio_chip {
> struct irq_domain *domain;
> int soc_variant;
>
> + /* Used for PWM support */
> + struct clk *clk;
> + struct mvebu_pwm *mvpwm;
Why does mvpwm needs to be allocated separately? Why not directly embed
it inside the mvebu_gpio_chip structure?
Do we need a separate spinlock?
> @@ -555,6 +842,10 @@ static const struct of_device_id mvebu_gpio_of_match[] = {
> .data = (void *) MVEBU_GPIO_SOC_VARIANT_ARMADAXP,
> },
> {
> + .compatible = "marvell,armada-370-xp-gpio",
> + .data = (void *) MVEBU_GPIO_SOC_VARIANT_ORION,
Whum, what are you doing here?
Thanks,
Thomas
--
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v5 1/4] gpio: mvebu: Add limited PWM support
2017-04-12 14:31 ` Thomas Petazzoni
@ 2017-04-12 15:19 ` Andrew Lunn
-1 siblings, 0 replies; 40+ messages in thread
From: Andrew Lunn @ 2017-04-12 15:19 UTC (permalink / raw)
To: Thomas Petazzoni
Cc: Ralph Sennhauser, Thierry Reding, Mark Rutland, Jason Cooper,
Alexandre Courbot, Linus Walleij, Russell King, linux-pwm,
linux-kernel, Gregory Clement, devicetree, Rob Herring,
linux-gpio, linux-arm-kernel, Sebastian Hesselbarth
On Wed, Apr 12, 2017 at 04:31:28PM +0200, Thomas Petazzoni wrote:
> Hello,
>
> Sorry for the late feedback about this.
>
> On Sun, 9 Apr 2017 20:09:27 +0200, Ralph Sennhauser wrote:
>
> > + gpio1: gpio@18140 {
> > + compatible = "marvell,armada-370-xp-gpio";
> > + reg = <0x18140 0x40>, <0x181c8 0x08>;
>
> One issue I see is that you have only two counters A and B. You
> associate counter A with the first bank of GPIOs, and counter B with
> the second bank of GPIOs.
>
> Which means that if you need to PWM a GPIO from the third bank of
> GPIOs, you can't, even if the HW allows it.
>
>
> While I'm fine with not supporting all the HW features, but it's a bit
> sad that this gets encoded into the DT.
>
> But I guess the only way to make this possible would be to have a
> single node for all GPIOs rather than one per bank? Or do we have a way
> to have those counter A/B registers bound to a separate PWM driver, and
> then the GPIO driver being smart enough to select the counter to be
> used? Seems not easy to do though :-/
Hi Thomas
Yep. It was a compromise. By adding a new binding for the GPIO driver,
this might be possible. But it did not seem worth such a major change.
The prime use of this feature is for controlling a fan. So far, i've
not seen any hardware with more than one fan, i.e. needs more than one
PWM. Nor have i seen any hardware with the GPIO for the fan being on
the third bank. A hardware manufacture could add multiple fans, but i
doubt it, they make noise and fail. And if a manufacture does place a
fan on the third bank, it can still be controlled as a plain GPIO fan,
as we have been doing for the last few years.
So i personally think it is an O.K. compromise.
> > @@ -555,6 +842,10 @@ static const struct of_device_id mvebu_gpio_of_match[] = {
> > .data = (void *) MVEBU_GPIO_SOC_VARIANT_ARMADAXP,
> > },
> > {
> > + .compatible = "marvell,armada-370-xp-gpio",
> > + .data = (void *) MVEBU_GPIO_SOC_VARIANT_ORION,
>
> Whum, what are you doing here?
Since you are late to the discussion, you probably missed this part.
Only 370 and XP have the hardware needed to do this. Kirkwood and
Orion5x does not. It was requested a compatible string was added to
indicate SoC has the needed hardware.
The driver was extended when XP was added, due it is per CPU
interrupts. However, that turned out to be broken. One CPU would
enable the interrupt, and it was delivered to another, causing it to
be missed. So XP was reverted to use the plan old ORION way of doing
interrupts.
So this patch adds in a new compatible string for 370 and XP, to
indicate the PWM hardware is available, and keeps using the ORION way
of handing interrupts.
Andrew
^ permalink raw reply [flat|nested] 40+ messages in thread* [PATCH v5 1/4] gpio: mvebu: Add limited PWM support
@ 2017-04-12 15:19 ` Andrew Lunn
0 siblings, 0 replies; 40+ messages in thread
From: Andrew Lunn @ 2017-04-12 15:19 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Apr 12, 2017 at 04:31:28PM +0200, Thomas Petazzoni wrote:
> Hello,
>
> Sorry for the late feedback about this.
>
> On Sun, 9 Apr 2017 20:09:27 +0200, Ralph Sennhauser wrote:
>
> > + gpio1: gpio at 18140 {
> > + compatible = "marvell,armada-370-xp-gpio";
> > + reg = <0x18140 0x40>, <0x181c8 0x08>;
>
> One issue I see is that you have only two counters A and B. You
> associate counter A with the first bank of GPIOs, and counter B with
> the second bank of GPIOs.
>
> Which means that if you need to PWM a GPIO from the third bank of
> GPIOs, you can't, even if the HW allows it.
>
>
> While I'm fine with not supporting all the HW features, but it's a bit
> sad that this gets encoded into the DT.
>
> But I guess the only way to make this possible would be to have a
> single node for all GPIOs rather than one per bank? Or do we have a way
> to have those counter A/B registers bound to a separate PWM driver, and
> then the GPIO driver being smart enough to select the counter to be
> used? Seems not easy to do though :-/
Hi Thomas
Yep. It was a compromise. By adding a new binding for the GPIO driver,
this might be possible. But it did not seem worth such a major change.
The prime use of this feature is for controlling a fan. So far, i've
not seen any hardware with more than one fan, i.e. needs more than one
PWM. Nor have i seen any hardware with the GPIO for the fan being on
the third bank. A hardware manufacture could add multiple fans, but i
doubt it, they make noise and fail. And if a manufacture does place a
fan on the third bank, it can still be controlled as a plain GPIO fan,
as we have been doing for the last few years.
So i personally think it is an O.K. compromise.
> > @@ -555,6 +842,10 @@ static const struct of_device_id mvebu_gpio_of_match[] = {
> > .data = (void *) MVEBU_GPIO_SOC_VARIANT_ARMADAXP,
> > },
> > {
> > + .compatible = "marvell,armada-370-xp-gpio",
> > + .data = (void *) MVEBU_GPIO_SOC_VARIANT_ORION,
>
> Whum, what are you doing here?
Since you are late to the discussion, you probably missed this part.
Only 370 and XP have the hardware needed to do this. Kirkwood and
Orion5x does not. It was requested a compatible string was added to
indicate SoC has the needed hardware.
The driver was extended when XP was added, due it is per CPU
interrupts. However, that turned out to be broken. One CPU would
enable the interrupt, and it was delivered to another, causing it to
be missed. So XP was reverted to use the plan old ORION way of doing
interrupts.
So this patch adds in a new compatible string for 370 and XP, to
indicate the PWM hardware is available, and keeps using the ORION way
of handing interrupts.
Andrew
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v5 1/4] gpio: mvebu: Add limited PWM support
2017-04-12 15:19 ` Andrew Lunn
(?)
@ 2017-04-21 9:19 ` Thomas Petazzoni
-1 siblings, 0 replies; 40+ messages in thread
From: Thomas Petazzoni @ 2017-04-21 9:19 UTC (permalink / raw)
To: Andrew Lunn
Cc: Mark Rutland, Alexandre Courbot, Jason Cooper, linux-pwm,
Linus Walleij, Russell King, Rob Herring, linux-kernel,
Gregory Clement, devicetree, Thierry Reding, linux-gpio,
Ralph Sennhauser, linux-arm-kernel, Sebastian Hesselbarth
Hello,
On Wed, 12 Apr 2017 17:19:32 +0200, Andrew Lunn wrote:
> Yep. It was a compromise. By adding a new binding for the GPIO driver,
> this might be possible. But it did not seem worth such a major change.
>
> The prime use of this feature is for controlling a fan. So far, i've
> not seen any hardware with more than one fan, i.e. needs more than one
> PWM. Nor have i seen any hardware with the GPIO for the fan being on
> the third bank. A hardware manufacture could add multiple fans, but i
> doubt it, they make noise and fail. And if a manufacture does place a
> fan on the third bank, it can still be controlled as a plain GPIO fan,
> as we have been doing for the last few years.
Right.
> So i personally think it is an O.K. compromise.
I clearly don't want to block this, but I believe this is a very good
illustration of why stable DT bindings simply don't work. We are
realizing here that having each GPIO bank represented as a separate DT
node doesn't work, because this blinking functionality is not per GPIO
bank, but global to all GPIO banks.
I am totally fine with compromise, and having things simple first, and
extend them later if needed. But this stable DT binding rule makes this
quite impossible: what is a compromise today might put you in big
troubles tomorrow.
Anyway, it's fine for me, I don't think it's worth the effort making a
much more complicated solution/change.
Best regards,
Thomas
--
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v5 1/4] gpio: mvebu: Add limited PWM support
@ 2017-04-21 9:19 ` Thomas Petazzoni
0 siblings, 0 replies; 40+ messages in thread
From: Thomas Petazzoni @ 2017-04-21 9:19 UTC (permalink / raw)
To: linux-arm-kernel
Hello,
On Wed, 12 Apr 2017 17:19:32 +0200, Andrew Lunn wrote:
> Yep. It was a compromise. By adding a new binding for the GPIO driver,
> this might be possible. But it did not seem worth such a major change.
>
> The prime use of this feature is for controlling a fan. So far, i've
> not seen any hardware with more than one fan, i.e. needs more than one
> PWM. Nor have i seen any hardware with the GPIO for the fan being on
> the third bank. A hardware manufacture could add multiple fans, but i
> doubt it, they make noise and fail. And if a manufacture does place a
> fan on the third bank, it can still be controlled as a plain GPIO fan,
> as we have been doing for the last few years.
Right.
> So i personally think it is an O.K. compromise.
I clearly don't want to block this, but I believe this is a very good
illustration of why stable DT bindings simply don't work. We are
realizing here that having each GPIO bank represented as a separate DT
node doesn't work, because this blinking functionality is not per GPIO
bank, but global to all GPIO banks.
I am totally fine with compromise, and having things simple first, and
extend them later if needed. But this stable DT binding rule makes this
quite impossible: what is a compromise today might put you in big
troubles tomorrow.
Anyway, it's fine for me, I don't think it's worth the effort making a
much more complicated solution/change.
Best regards,
Thomas
--
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v5 1/4] gpio: mvebu: Add limited PWM support
@ 2017-04-21 9:19 ` Thomas Petazzoni
0 siblings, 0 replies; 40+ messages in thread
From: Thomas Petazzoni @ 2017-04-21 9:19 UTC (permalink / raw)
To: Andrew Lunn
Cc: Ralph Sennhauser, Thierry Reding, Mark Rutland, Jason Cooper,
Alexandre Courbot, Linus Walleij, Russell King, linux-pwm,
linux-kernel, Gregory Clement, devicetree, Rob Herring,
linux-gpio, linux-arm-kernel, Sebastian Hesselbarth
Hello,
On Wed, 12 Apr 2017 17:19:32 +0200, Andrew Lunn wrote:
> Yep. It was a compromise. By adding a new binding for the GPIO driver,
> this might be possible. But it did not seem worth such a major change.
>
> The prime use of this feature is for controlling a fan. So far, i've
> not seen any hardware with more than one fan, i.e. needs more than one
> PWM. Nor have i seen any hardware with the GPIO for the fan being on
> the third bank. A hardware manufacture could add multiple fans, but i
> doubt it, they make noise and fail. And if a manufacture does place a
> fan on the third bank, it can still be controlled as a plain GPIO fan,
> as we have been doing for the last few years.
Right.
> So i personally think it is an O.K. compromise.
I clearly don't want to block this, but I believe this is a very good
illustration of why stable DT bindings simply don't work. We are
realizing here that having each GPIO bank represented as a separate DT
node doesn't work, because this blinking functionality is not per GPIO
bank, but global to all GPIO banks.
I am totally fine with compromise, and having things simple first, and
extend them later if needed. But this stable DT binding rule makes this
quite impossible: what is a compromise today might put you in big
troubles tomorrow.
Anyway, it's fine for me, I don't think it's worth the effort making a
much more complicated solution/change.
Best regards,
Thomas
--
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v5 1/4] gpio: mvebu: Add limited PWM support
2017-04-21 9:19 ` Thomas Petazzoni
(?)
@ 2017-04-24 9:15 ` Linus Walleij
-1 siblings, 0 replies; 40+ messages in thread
From: Linus Walleij @ 2017-04-24 9:15 UTC (permalink / raw)
To: Thomas Petazzoni
Cc: Andrew Lunn, Ralph Sennhauser, Thierry Reding, Mark Rutland,
Jason Cooper, Alexandre Courbot, Russell King, linux-pwm,
linux-kernel, Gregory Clement, devicetree, Rob Herring,
linux-gpio, linux-arm-kernel, Sebastian Hesselbarth
On Fri, Apr 21, 2017 at 11:19 AM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> I clearly don't want to block this, but I believe this is a very good
> illustration of why stable DT bindings simply don't work. We are
> realizing here that having each GPIO bank represented as a separate DT
> node doesn't work, because this blinking functionality is not per GPIO
> bank, but global to all GPIO banks.
>
> I am totally fine with compromise, and having things simple first, and
> extend them later if needed. But this stable DT binding rule makes this
> quite impossible: what is a compromise today might put you in big
> troubles tomorrow.
Really "stable bindings" I never believed in. It's just a pipe dream.
Well they might become stable when the system is "finished"
whenever that happens.
I think a better rationale is that of the IETF:
"rough consensus and running code", make deployed DTs work,
if they are not deployed, or only getting deployed together with the
kernel, changing the bindings are not a problem.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v5 1/4] gpio: mvebu: Add limited PWM support
@ 2017-04-24 9:15 ` Linus Walleij
0 siblings, 0 replies; 40+ messages in thread
From: Linus Walleij @ 2017-04-24 9:15 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Apr 21, 2017 at 11:19 AM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> I clearly don't want to block this, but I believe this is a very good
> illustration of why stable DT bindings simply don't work. We are
> realizing here that having each GPIO bank represented as a separate DT
> node doesn't work, because this blinking functionality is not per GPIO
> bank, but global to all GPIO banks.
>
> I am totally fine with compromise, and having things simple first, and
> extend them later if needed. But this stable DT binding rule makes this
> quite impossible: what is a compromise today might put you in big
> troubles tomorrow.
Really "stable bindings" I never believed in. It's just a pipe dream.
Well they might become stable when the system is "finished"
whenever that happens.
I think a better rationale is that of the IETF:
"rough consensus and running code", make deployed DTs work,
if they are not deployed, or only getting deployed together with the
kernel, changing the bindings are not a problem.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v5 1/4] gpio: mvebu: Add limited PWM support
@ 2017-04-24 9:15 ` Linus Walleij
0 siblings, 0 replies; 40+ messages in thread
From: Linus Walleij @ 2017-04-24 9:15 UTC (permalink / raw)
To: Thomas Petazzoni
Cc: Andrew Lunn, Ralph Sennhauser, Thierry Reding, Mark Rutland,
Jason Cooper, Alexandre Courbot, Russell King, linux-pwm,
linux-kernel, Gregory Clement, devicetree, Rob Herring,
linux-gpio, linux-arm-kernel, Sebastian Hesselbarth
On Fri, Apr 21, 2017 at 11:19 AM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> I clearly don't want to block this, but I believe this is a very good
> illustration of why stable DT bindings simply don't work. We are
> realizing here that having each GPIO bank represented as a separate DT
> node doesn't work, because this blinking functionality is not per GPIO
> bank, but global to all GPIO banks.
>
> I am totally fine with compromise, and having things simple first, and
> extend them later if needed. But this stable DT binding rule makes this
> quite impossible: what is a compromise today might put you in big
> troubles tomorrow.
Really "stable bindings" I never believed in. It's just a pipe dream.
Well they might become stable when the system is "finished"
whenever that happens.
I think a better rationale is that of the IETF:
"rough consensus and running code", make deployed DTs work,
if they are not deployed, or only getting deployed together with the
kernel, changing the bindings are not a problem.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v5 1/4] gpio: mvebu: Add limited PWM support
2017-04-09 18:09 ` Ralph Sennhauser
@ 2017-04-12 17:11 ` Thierry Reding
-1 siblings, 0 replies; 40+ messages in thread
From: Thierry Reding @ 2017-04-12 17:11 UTC (permalink / raw)
To: Ralph Sennhauser
Cc: Andrew Lunn, Linus Walleij, Alexandre Courbot, Rob Herring,
Mark Rutland, Jason Cooper, Gregory Clement,
Sebastian Hesselbarth, Russell King, linux-pwm, linux-gpio,
devicetree, linux-kernel, linux-arm-kernel
[-- Attachment #1: Type: text/plain, Size: 17676 bytes --]
On Sun, Apr 09, 2017 at 08:09:27PM +0200, Ralph Sennhauser wrote:
> From: Andrew Lunn <andrew@lunn.ch>
>
> Armada 370/XP devices can 'blink' GPIO lines with a configurable on
> and off period. This can be modelled as a PWM.
>
> However, there are only two sets of PWM configuration registers for
> all the GPIO lines. This driver simply allows a single GPIO line per
> GPIO chip of 32 lines to be used as a PWM. Attempts to use more return
> EBUSY.
>
> Due to the interleaving of registers it is not simple to separate the
> PWM driver from the GPIO driver. Thus the GPIO driver has been
> extended with a PWM driver.
>
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> URL: https://patchwork.ozlabs.org/patch/427287/
> URL: https://patchwork.ozlabs.org/patch/427295/
> [Ralph Sennhauser:
> * Port forward
> * Merge PWM portion into gpio-mvebu.c
> * Switch to atomic PWM API
> * Add new compatible string marvell,armada-370-xp-gpio
> * Update and merge documentation patch
> * Update MAINTAINERS]
> Signed-off-by: Ralph Sennhauser <ralph.sennhauser@gmail.com>
> Tested-by: Andrew Lunn <andrew@lunn.ch>
> ---
> .../devicetree/bindings/gpio/gpio-mvebu.txt | 32 ++
> MAINTAINERS | 2 +
> drivers/gpio/gpio-mvebu.c | 324 ++++++++++++++++++++-
> 3 files changed, 346 insertions(+), 12 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/gpio/gpio-mvebu.txt b/Documentation/devicetree/bindings/gpio/gpio-mvebu.txt
> index a6f3bec..fe49e9d 100644
> --- a/Documentation/devicetree/bindings/gpio/gpio-mvebu.txt
> +++ b/Documentation/devicetree/bindings/gpio/gpio-mvebu.txt
> @@ -38,6 +38,24 @@ Required properties:
> - #gpio-cells: Should be two. The first cell is the pin number. The
> second cell is reserved for flags, unused at the moment.
>
> +Optional properties:
> +
> +In order to use the gpio lines in PWM mode, some additional optional
> +properties are required. Only Armada 370 and XP support these properties.
> +
> +- compatible: Must contain "marvell,armada-370-xp-gpio"
> +
> +- reg: an additional register set is needed, for the GPIO Blink
> + Counter on/off registers.
> +
> +- reg-names: Must contain an entry "pwm" corresponding to the
> + additional register range needed for pwm operation.
> +
> +- #pwm-cells: Should be two. The first cell is the GPIO line number. The
> + second cell is the period in nanoseconds.
> +
> +- clocks: Must be a phandle to the clock for the gpio controller.
> +
> Example:
>
> gpio0: gpio@d0018100 {
> @@ -51,3 +69,17 @@ Example:
> #interrupt-cells = <2>;
> interrupts = <16>, <17>, <18>, <19>;
> };
> +
> + gpio1: gpio@18140 {
> + compatible = "marvell,armada-370-xp-gpio";
> + reg = <0x18140 0x40>, <0x181c8 0x08>;
> + reg-names = "gpio", "pwm";
> + ngpios = <17>;
> + gpio-controller;
> + #gpio-cells = <2>;
> + #pwm-cells = <2>;
> + interrupt-controller;
> + #interrupt-cells = <2>;
> + interrupts = <87>, <88>, <89>;
> + clocks = <&coreclk 0>;
> + };
This is going to need an Acked-by from one of the device tree
maintainers. Rob and devicetree@vger.kernel.org are on Cc, but I suspect
nobody might look for the binding change "hidden" in this patch.
Maybe best to split this off into a separate patch, or explicitly ping
Rob to look at this patch.
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 58b3a22..19382f5 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -10295,6 +10295,8 @@ F: include/linux/pwm.h
> F: drivers/pwm/
> F: drivers/video/backlight/pwm_bl.c
> F: include/linux/pwm_backlight.h
> +F: drivers/gpio/gpio-mvebu.c
> +F: Documentation/devicetree/bindings/gpio/gpio-mvebu.txt
>
> PXA2xx/PXA3xx SUPPORT
> M: Daniel Mack <daniel@zonque.org>
> diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c
> index fae4db6..e310951 100644
> --- a/drivers/gpio/gpio-mvebu.c
> +++ b/drivers/gpio/gpio-mvebu.c
> @@ -42,22 +42,34 @@
> #include <linux/io.h>
> #include <linux/of_irq.h>
> #include <linux/of_device.h>
> +#include <linux/pwm.h>
> #include <linux/clk.h>
> #include <linux/pinctrl/consumer.h>
> #include <linux/irqchip/chained_irq.h>
> +#include <linux/platform_device.h>
> #include <linux/bitops.h>
>
> +#include "gpiolib.h"
> +
> /*
> * GPIO unit register offsets.
> */
> -#define GPIO_OUT_OFF 0x0000
> -#define GPIO_IO_CONF_OFF 0x0004
> -#define GPIO_BLINK_EN_OFF 0x0008
> -#define GPIO_IN_POL_OFF 0x000c
> -#define GPIO_DATA_IN_OFF 0x0010
> -#define GPIO_EDGE_CAUSE_OFF 0x0014
> -#define GPIO_EDGE_MASK_OFF 0x0018
> -#define GPIO_LEVEL_MASK_OFF 0x001c
> +#define GPIO_OUT_OFF 0x0000
> +#define GPIO_IO_CONF_OFF 0x0004
> +#define GPIO_BLINK_EN_OFF 0x0008
> +#define GPIO_IN_POL_OFF 0x000c
> +#define GPIO_DATA_IN_OFF 0x0010
> +#define GPIO_EDGE_CAUSE_OFF 0x0014
> +#define GPIO_EDGE_MASK_OFF 0x0018
> +#define GPIO_LEVEL_MASK_OFF 0x001c
> +#define GPIO_BLINK_CNT_SELECT_OFF 0x0020
> +
> +/*
> + * PWM register offsets.
> + */
> +#define PWM_BLINK_ON_DURATION_OFF 0x0
> +#define PWM_BLINK_OFF_DURATION_OFF 0x4
> +
>
> /* The MV78200 has per-CPU registers for edge mask and level mask */
> #define GPIO_EDGE_MASK_MV78200_OFF(cpu) ((cpu) ? 0x30 : 0x18)
> @@ -78,6 +90,20 @@
>
> #define MVEBU_MAX_GPIO_PER_BANK 32
>
> +struct mvebu_pwm {
> + void __iomem *membase;
> + unsigned long clk_rate;
> + bool used;
I think you could've probably made this a little simpler by making bool
used into struct gpio_desc *gpio; and reference that rather than having
to convert with gpio_to_desc(). More on why I think that's important
below.
> + struct pwm_chip chip;
> + spinlock_t lock;
> + struct mvebu_gpio_chip *mvchip;
> +
> + /* Used to preserve GPIO/PWM registers across suspend/resume */
> + u32 blink_select;
> + u32 blink_on_duration;
> + u32 blink_off_duration;
> +};
> +
> struct mvebu_gpio_chip {
> struct gpio_chip chip;
> spinlock_t lock;
> @@ -87,6 +113,10 @@ struct mvebu_gpio_chip {
> struct irq_domain *domain;
> int soc_variant;
>
> + /* Used for PWM support */
> + struct clk *clk;
> + struct mvebu_pwm *mvpwm;
> +
> /* Used to preserve GPIO registers across suspend/resume */
> u32 out_reg;
> u32 io_conf_reg;
> @@ -110,6 +140,12 @@ static void __iomem *mvebu_gpioreg_blink(struct mvebu_gpio_chip *mvchip)
> return mvchip->membase + GPIO_BLINK_EN_OFF;
> }
>
> +static void __iomem *mvebu_gpioreg_blink_counter_select(struct mvebu_gpio_chip
> + *mvchip)
> +{
> + return mvchip->membase + GPIO_BLINK_CNT_SELECT_OFF;
> +}
> +
> static void __iomem *mvebu_gpioreg_io_conf(struct mvebu_gpio_chip *mvchip)
> {
> return mvchip->membase + GPIO_IO_CONF_OFF;
> @@ -181,6 +217,20 @@ static void __iomem *mvebu_gpioreg_level_mask(struct mvebu_gpio_chip *mvchip)
> }
>
> /*
> + * Functions returning addresses of individual registers for a given
> + * PWM controller.
> + */
> +static void __iomem *mvebu_pwmreg_blink_on_duration(struct mvebu_pwm *mvpwm)
> +{
> + return mvpwm->membase + PWM_BLINK_ON_DURATION_OFF;
> +}
> +
> +static void __iomem *mvebu_pwmreg_blink_off_duration(struct mvebu_pwm *mvpwm)
> +{
> + return mvpwm->membase + PWM_BLINK_OFF_DURATION_OFF;
> +}
> +
> +/*
> * Functions implementing the gpio_chip methods
> */
> static void mvebu_gpio_set(struct gpio_chip *chip, unsigned int pin, int value)
> @@ -484,6 +534,243 @@ static void mvebu_gpio_irq_handler(struct irq_desc *desc)
> chained_irq_exit(chip, desc);
> }
>
> +/*
> + * Functions implementing the pwm_chip methods
> + */
> +static struct mvebu_pwm *to_mvebu_pwm(struct pwm_chip *chip)
> +{
> + return container_of(chip, struct mvebu_pwm, chip);
> +}
> +
> +static int mvebu_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> + struct mvebu_pwm *mvpwm = to_mvebu_pwm(chip);
> + struct gpio_desc *desc = gpio_to_desc(pwm->pwm);
This assumes that the GPIO chip base and the PWM chip base are the same.
You make sure of that by setting the PWM chip base to be the same as the
GPIO chip base, which is the easy way out. It's also somewhat brittle
because some other PWM chip may have occupied the region that you want
to use. It's fairly unlikely, but I think you can very easily side-step
any issues by simply doing:
struct mvebu_gpio_chip *mvchip = mvpwm->mvchip;
desc = gpio_to_desc(mvchip->chip.base + pwm->hwpwm);
Now that's somewhat complicated, but you only have to do it once if you
store the desc in mvpwm->gpio as pointed out above.
> + unsigned long flags;
> + int ret = 0;
> +
> + spin_lock_irqsave(&mvpwm->lock, flags);
> + if (mvpwm->used) {
> + ret = -EBUSY;
Then you can also easily use mvpwm->gpio to check for -EBUSY here.
> + } else {
> + if (!desc) {
> + ret = -ENODEV;
> + goto out;
> + }
> + ret = gpiod_request(desc, "mvebu-pwm");
> + if (ret)
> + goto out;
> +
> + ret = gpiod_direction_output(desc, 0);
> + if (ret) {
> + gpiod_free(desc);
> + goto out;
> + }
> +
> + mvpwm->used = true;
> + }
> +
> +out:
> + spin_unlock_irqrestore(&mvpwm->lock, flags);
> + return ret;
> +}
> +
> +static void mvebu_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> + struct mvebu_pwm *mvpwm = to_mvebu_pwm(chip);
> + struct gpio_desc *desc = gpio_to_desc(pwm->pwm);
> + unsigned long flags;
> +
> + spin_lock_irqsave(&mvpwm->lock, flags);
> + gpiod_free(desc);
> + mvpwm->used = false;
> + spin_unlock_irqrestore(&mvpwm->lock, flags);
> +}
> +
> +static void mvebu_pwm_get_state(struct pwm_chip *chip,
> + struct pwm_device *pwm,
> + struct pwm_state *state) {
> +
> + struct mvebu_pwm *mvpwm = to_mvebu_pwm(chip);
> + struct mvebu_gpio_chip *mvchip = mvpwm->mvchip;
> + unsigned long long val;
> + unsigned long flags;
> + u32 u;
> +
> + spin_lock_irqsave(&mvpwm->lock, flags);
> +
> + val = (unsigned long long)
> + readl_relaxed(mvebu_pwmreg_blink_on_duration);
> + val *= NSEC_PER_SEC;
> + do_div(val, mvpwm->clk_rate);
> + if (val > UINT_MAX)
> + state->duty_cycle = UINT_MAX;
> + else if (val)
> + state->duty_cycle = val;
> + else
> + state->duty_cycle = 1;
> +
> + val = (unsigned long long)
> + readl_relaxed(mvebu_pwmreg_blink_off_duration);
> + val *= NSEC_PER_SEC;
> + do_div(val, mvpwm->clk_rate);
> + if (val < state->duty_cycle) {
> + state->period = 1;
> + } else {
> + val -= state->duty_cycle;
> + if (val > UINT_MAX)
> + state->period = UINT_MAX;
> + else if (val)
> + state->period = val;
> + else
> + state->period = 1;
> + }
> +
> + u = readl_relaxed(mvebu_gpioreg_blink(mvchip));
> + if (u)
> + state->enabled = true;
> + else
> + state->enabled = false;
> +
> + spin_unlock_irqrestore(&mvpwm->lock, flags);
> +}
> +
> +static int mvebu_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> + struct pwm_state *state)
> +{
> + struct mvebu_pwm *mvpwm = to_mvebu_pwm(chip);
> + struct mvebu_gpio_chip *mvchip = mvpwm->mvchip;
> + unsigned long long val;
> + unsigned long flags;
> + unsigned int on, off;
> +
> + val = (unsigned long long) mvpwm->clk_rate * state->duty_cycle;
> + do_div(val, NSEC_PER_SEC);
> + if (val > UINT_MAX)
> + return -EINVAL;
> + if (val)
> + on = val;
> + else
> + on = 1;
> +
> + val = (unsigned long long) mvpwm->clk_rate *
> + (state->period - state->duty_cycle);
> + do_div(val, NSEC_PER_SEC);
> + if (val > UINT_MAX)
> + return -EINVAL;
> + if (val)
> + off = val;
> + else
> + off = 1;
> +
> + spin_lock_irqsave(&mvpwm->lock, flags);
> +
> + writel_relaxed(on, mvebu_pwmreg_blink_on_duration(mvpwm));
> + writel_relaxed(off, mvebu_pwmreg_blink_off_duration(mvpwm));
> + if (state->enabled)
> + mvebu_gpio_blink(&mvchip->chip, pwm->hwpwm, 1);
> + else
> + mvebu_gpio_blink(&mvchip->chip, pwm->hwpwm, 0);
> +
> + spin_unlock_irqrestore(&mvpwm->lock, flags);
> +
> + return 0;
> +}
> +
> +static const struct pwm_ops mvebu_pwm_ops = {
> + .request = mvebu_pwm_request,
> + .free = mvebu_pwm_free,
> + .get_state = mvebu_pwm_get_state,
> + .apply = mvebu_pwm_apply,
> + .owner = THIS_MODULE,
> +};
> +
> +static void __maybe_unused mvebu_pwm_suspend(struct mvebu_gpio_chip *mvchip)
> +{
> + struct mvebu_pwm *mvpwm = mvchip->mvpwm;
> +
> + mvpwm->blink_select =
> + readl_relaxed(mvebu_gpioreg_blink_counter_select(mvchip));
> + mvpwm->blink_on_duration =
> + readl_relaxed(mvebu_pwmreg_blink_on_duration(mvpwm));
> + mvpwm->blink_off_duration =
> + readl_relaxed(mvebu_pwmreg_blink_off_duration(mvpwm));
> +}
> +
> +static void __maybe_unused mvebu_pwm_resume(struct mvebu_gpio_chip *mvchip)
> +{
> + struct mvebu_pwm *mvpwm = mvchip->mvpwm;
> +
> + writel_relaxed(mvpwm->blink_select,
> + mvebu_gpioreg_blink_counter_select(mvchip));
> + writel_relaxed(mvpwm->blink_on_duration,
> + mvebu_pwmreg_blink_on_duration(mvpwm));
> + writel_relaxed(mvpwm->blink_off_duration,
> + mvebu_pwmreg_blink_off_duration(mvpwm));
> +}
> +
> +static int mvebu_pwm_probe(struct platform_device *pdev,
> + struct mvebu_gpio_chip *mvchip,
> + int id)
> +{
> + struct device *dev = &pdev->dev;
> + struct mvebu_pwm *mvpwm;
> + struct resource *res;
> +
> + if (!of_device_is_compatible(mvchip->chip.of_node,
> + "marvell,armada-370-xp-gpio"))
> + return 0;
> + /*
> + * There are only two sets of PWM configuration registers for
> + * all the GPIO lines on those SoCs which this driver reserves
> + * for the first two GPIO chips. So if the resource is missing
> + * we can't treat it as an error.
> + */
> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "pwm");
> + if (!res)
> + return 0;
> +
> + /*
> + * Use set A for lines of GPIO chip with id 0, B for GPIO chip
> + * with id 1. Don't allow further GPIO chips to be used for PWM.
> + */
> + if (id == 0)
> + writel_relaxed(0, mvebu_gpioreg_blink_counter_select(mvchip));
> + else if (id == 1)
> + writel_relaxed(U32_MAX,
> + mvebu_gpioreg_blink_counter_select(mvchip));
You could've just set a variable and then call writel_relaxed() after
the return -EINVAL below.
> + else
> + return -EINVAL;
> +
> + mvpwm = devm_kzalloc(dev, sizeof(struct mvebu_pwm), GFP_KERNEL);
> + if (!mvpwm)
> + return -ENOMEM;
> + mvchip->mvpwm = mvpwm;
> + mvpwm->mvchip = mvchip;
> +
> + mvpwm->membase = devm_ioremap_resource(dev, res);
> + if (IS_ERR(mvpwm->membase))
> + return PTR_ERR(mvpwm->membase);
> +
> + if (IS_ERR(mvchip->clk))
> + return PTR_ERR(mvchip->clk);
Maybe do this much earlier to avoid all the unnecessary register
accesses, allocations and mappings?
> +
> + mvpwm->clk_rate = clk_get_rate(mvchip->clk);
> + if (!mvpwm->clk_rate) {
> + dev_err(dev, "failed to get clock rate\n");
> + return -EINVAL;
> + }
> +
> + mvpwm->chip.dev = dev;
> + mvpwm->chip.ops = &mvebu_pwm_ops;
> + mvpwm->chip.base = mvchip->chip.base;
> + mvpwm->chip.npwm = mvchip->chip.ngpio;
I still would've done this differently. If you use this with a PWM user
you have to hook it up via DT anyway, so it doesn't matter whether you
specify the PWM index or the GPIO via some other property. The _only_
use-case where this might actually be an advantage is if you request a
PWM via the sysfs interface.
> + spin_lock_init(&mvpwm->lock);
> +
> + return pwmchip_add(&mvpwm->chip);
> +}
> +
> #ifdef CONFIG_DEBUG_FS
> #include <linux/seq_file.h>
>
> @@ -555,6 +842,10 @@ static const struct of_device_id mvebu_gpio_of_match[] = {
> .data = (void *) MVEBU_GPIO_SOC_VARIANT_ARMADAXP,
> },
> {
> + .compatible = "marvell,armada-370-xp-gpio",
> + .data = (void *) MVEBU_GPIO_SOC_VARIANT_ORION,
> + },
> + {
> /* sentinel */
> },
> };
> @@ -600,6 +891,9 @@ static int mvebu_gpio_suspend(struct platform_device *pdev, pm_message_t state)
> BUG();
> }
>
> + if (IS_ENABLED(CONFIG_PWM))
> + mvebu_pwm_suspend(mvchip);
> +
> return 0;
> }
>
> @@ -643,6 +937,9 @@ static int mvebu_gpio_resume(struct platform_device *pdev)
> BUG();
> }
>
> + if (IS_ENABLED(CONFIG_PWM))
> + mvebu_pwm_resume(mvchip);
> +
> return 0;
> }
>
> @@ -654,7 +951,6 @@ static int mvebu_gpio_probe(struct platform_device *pdev)
> struct resource *res;
> struct irq_chip_generic *gc;
> struct irq_chip_type *ct;
> - struct clk *clk;
> unsigned int ngpios;
> bool have_irqs;
> int soc_variant;
> @@ -688,10 +984,10 @@ static int mvebu_gpio_probe(struct platform_device *pdev)
> return id;
> }
>
> - clk = devm_clk_get(&pdev->dev, NULL);
> + mvchip->clk = devm_clk_get(&pdev->dev, NULL);
> /* Not all SoCs require a clock.*/
> - if (!IS_ERR(clk))
> - clk_prepare_enable(clk);
> + if (!IS_ERR(mvchip->clk))
> + clk_prepare_enable(mvchip->clk);
>
> mvchip->soc_variant = soc_variant;
> mvchip->chip.label = dev_name(&pdev->dev);
> @@ -822,6 +1118,10 @@ static int mvebu_gpio_probe(struct platform_device *pdev)
> mvchip);
> }
>
> + /* Armada 370/XP has simple PWM support for GPIO lines */
> + if (IS_ENABLED(CONFIG_PWM))
> + return mvebu_pwm_probe(pdev, mvchip, id);
> +
> return 0;
>
> err_domain:
> --
> 2.10.2
All of my comments are effectively of a bikeshed nature, so from a PWM
perspective this is:
Acked-by: Thierry Reding <thierry.reding@gmail.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 40+ messages in thread* [PATCH v5 1/4] gpio: mvebu: Add limited PWM support
@ 2017-04-12 17:11 ` Thierry Reding
0 siblings, 0 replies; 40+ messages in thread
From: Thierry Reding @ 2017-04-12 17:11 UTC (permalink / raw)
To: linux-arm-kernel
On Sun, Apr 09, 2017 at 08:09:27PM +0200, Ralph Sennhauser wrote:
> From: Andrew Lunn <andrew@lunn.ch>
>
> Armada 370/XP devices can 'blink' GPIO lines with a configurable on
> and off period. This can be modelled as a PWM.
>
> However, there are only two sets of PWM configuration registers for
> all the GPIO lines. This driver simply allows a single GPIO line per
> GPIO chip of 32 lines to be used as a PWM. Attempts to use more return
> EBUSY.
>
> Due to the interleaving of registers it is not simple to separate the
> PWM driver from the GPIO driver. Thus the GPIO driver has been
> extended with a PWM driver.
>
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> URL: https://patchwork.ozlabs.org/patch/427287/
> URL: https://patchwork.ozlabs.org/patch/427295/
> [Ralph Sennhauser:
> * Port forward
> * Merge PWM portion into gpio-mvebu.c
> * Switch to atomic PWM API
> * Add new compatible string marvell,armada-370-xp-gpio
> * Update and merge documentation patch
> * Update MAINTAINERS]
> Signed-off-by: Ralph Sennhauser <ralph.sennhauser@gmail.com>
> Tested-by: Andrew Lunn <andrew@lunn.ch>
> ---
> .../devicetree/bindings/gpio/gpio-mvebu.txt | 32 ++
> MAINTAINERS | 2 +
> drivers/gpio/gpio-mvebu.c | 324 ++++++++++++++++++++-
> 3 files changed, 346 insertions(+), 12 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/gpio/gpio-mvebu.txt b/Documentation/devicetree/bindings/gpio/gpio-mvebu.txt
> index a6f3bec..fe49e9d 100644
> --- a/Documentation/devicetree/bindings/gpio/gpio-mvebu.txt
> +++ b/Documentation/devicetree/bindings/gpio/gpio-mvebu.txt
> @@ -38,6 +38,24 @@ Required properties:
> - #gpio-cells: Should be two. The first cell is the pin number. The
> second cell is reserved for flags, unused at the moment.
>
> +Optional properties:
> +
> +In order to use the gpio lines in PWM mode, some additional optional
> +properties are required. Only Armada 370 and XP support these properties.
> +
> +- compatible: Must contain "marvell,armada-370-xp-gpio"
> +
> +- reg: an additional register set is needed, for the GPIO Blink
> + Counter on/off registers.
> +
> +- reg-names: Must contain an entry "pwm" corresponding to the
> + additional register range needed for pwm operation.
> +
> +- #pwm-cells: Should be two. The first cell is the GPIO line number. The
> + second cell is the period in nanoseconds.
> +
> +- clocks: Must be a phandle to the clock for the gpio controller.
> +
> Example:
>
> gpio0: gpio at d0018100 {
> @@ -51,3 +69,17 @@ Example:
> #interrupt-cells = <2>;
> interrupts = <16>, <17>, <18>, <19>;
> };
> +
> + gpio1: gpio at 18140 {
> + compatible = "marvell,armada-370-xp-gpio";
> + reg = <0x18140 0x40>, <0x181c8 0x08>;
> + reg-names = "gpio", "pwm";
> + ngpios = <17>;
> + gpio-controller;
> + #gpio-cells = <2>;
> + #pwm-cells = <2>;
> + interrupt-controller;
> + #interrupt-cells = <2>;
> + interrupts = <87>, <88>, <89>;
> + clocks = <&coreclk 0>;
> + };
This is going to need an Acked-by from one of the device tree
maintainers. Rob and devicetree at vger.kernel.org are on Cc, but I suspect
nobody might look for the binding change "hidden" in this patch.
Maybe best to split this off into a separate patch, or explicitly ping
Rob to look at this patch.
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 58b3a22..19382f5 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -10295,6 +10295,8 @@ F: include/linux/pwm.h
> F: drivers/pwm/
> F: drivers/video/backlight/pwm_bl.c
> F: include/linux/pwm_backlight.h
> +F: drivers/gpio/gpio-mvebu.c
> +F: Documentation/devicetree/bindings/gpio/gpio-mvebu.txt
>
> PXA2xx/PXA3xx SUPPORT
> M: Daniel Mack <daniel@zonque.org>
> diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c
> index fae4db6..e310951 100644
> --- a/drivers/gpio/gpio-mvebu.c
> +++ b/drivers/gpio/gpio-mvebu.c
> @@ -42,22 +42,34 @@
> #include <linux/io.h>
> #include <linux/of_irq.h>
> #include <linux/of_device.h>
> +#include <linux/pwm.h>
> #include <linux/clk.h>
> #include <linux/pinctrl/consumer.h>
> #include <linux/irqchip/chained_irq.h>
> +#include <linux/platform_device.h>
> #include <linux/bitops.h>
>
> +#include "gpiolib.h"
> +
> /*
> * GPIO unit register offsets.
> */
> -#define GPIO_OUT_OFF 0x0000
> -#define GPIO_IO_CONF_OFF 0x0004
> -#define GPIO_BLINK_EN_OFF 0x0008
> -#define GPIO_IN_POL_OFF 0x000c
> -#define GPIO_DATA_IN_OFF 0x0010
> -#define GPIO_EDGE_CAUSE_OFF 0x0014
> -#define GPIO_EDGE_MASK_OFF 0x0018
> -#define GPIO_LEVEL_MASK_OFF 0x001c
> +#define GPIO_OUT_OFF 0x0000
> +#define GPIO_IO_CONF_OFF 0x0004
> +#define GPIO_BLINK_EN_OFF 0x0008
> +#define GPIO_IN_POL_OFF 0x000c
> +#define GPIO_DATA_IN_OFF 0x0010
> +#define GPIO_EDGE_CAUSE_OFF 0x0014
> +#define GPIO_EDGE_MASK_OFF 0x0018
> +#define GPIO_LEVEL_MASK_OFF 0x001c
> +#define GPIO_BLINK_CNT_SELECT_OFF 0x0020
> +
> +/*
> + * PWM register offsets.
> + */
> +#define PWM_BLINK_ON_DURATION_OFF 0x0
> +#define PWM_BLINK_OFF_DURATION_OFF 0x4
> +
>
> /* The MV78200 has per-CPU registers for edge mask and level mask */
> #define GPIO_EDGE_MASK_MV78200_OFF(cpu) ((cpu) ? 0x30 : 0x18)
> @@ -78,6 +90,20 @@
>
> #define MVEBU_MAX_GPIO_PER_BANK 32
>
> +struct mvebu_pwm {
> + void __iomem *membase;
> + unsigned long clk_rate;
> + bool used;
I think you could've probably made this a little simpler by making bool
used into struct gpio_desc *gpio; and reference that rather than having
to convert with gpio_to_desc(). More on why I think that's important
below.
> + struct pwm_chip chip;
> + spinlock_t lock;
> + struct mvebu_gpio_chip *mvchip;
> +
> + /* Used to preserve GPIO/PWM registers across suspend/resume */
> + u32 blink_select;
> + u32 blink_on_duration;
> + u32 blink_off_duration;
> +};
> +
> struct mvebu_gpio_chip {
> struct gpio_chip chip;
> spinlock_t lock;
> @@ -87,6 +113,10 @@ struct mvebu_gpio_chip {
> struct irq_domain *domain;
> int soc_variant;
>
> + /* Used for PWM support */
> + struct clk *clk;
> + struct mvebu_pwm *mvpwm;
> +
> /* Used to preserve GPIO registers across suspend/resume */
> u32 out_reg;
> u32 io_conf_reg;
> @@ -110,6 +140,12 @@ static void __iomem *mvebu_gpioreg_blink(struct mvebu_gpio_chip *mvchip)
> return mvchip->membase + GPIO_BLINK_EN_OFF;
> }
>
> +static void __iomem *mvebu_gpioreg_blink_counter_select(struct mvebu_gpio_chip
> + *mvchip)
> +{
> + return mvchip->membase + GPIO_BLINK_CNT_SELECT_OFF;
> +}
> +
> static void __iomem *mvebu_gpioreg_io_conf(struct mvebu_gpio_chip *mvchip)
> {
> return mvchip->membase + GPIO_IO_CONF_OFF;
> @@ -181,6 +217,20 @@ static void __iomem *mvebu_gpioreg_level_mask(struct mvebu_gpio_chip *mvchip)
> }
>
> /*
> + * Functions returning addresses of individual registers for a given
> + * PWM controller.
> + */
> +static void __iomem *mvebu_pwmreg_blink_on_duration(struct mvebu_pwm *mvpwm)
> +{
> + return mvpwm->membase + PWM_BLINK_ON_DURATION_OFF;
> +}
> +
> +static void __iomem *mvebu_pwmreg_blink_off_duration(struct mvebu_pwm *mvpwm)
> +{
> + return mvpwm->membase + PWM_BLINK_OFF_DURATION_OFF;
> +}
> +
> +/*
> * Functions implementing the gpio_chip methods
> */
> static void mvebu_gpio_set(struct gpio_chip *chip, unsigned int pin, int value)
> @@ -484,6 +534,243 @@ static void mvebu_gpio_irq_handler(struct irq_desc *desc)
> chained_irq_exit(chip, desc);
> }
>
> +/*
> + * Functions implementing the pwm_chip methods
> + */
> +static struct mvebu_pwm *to_mvebu_pwm(struct pwm_chip *chip)
> +{
> + return container_of(chip, struct mvebu_pwm, chip);
> +}
> +
> +static int mvebu_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> + struct mvebu_pwm *mvpwm = to_mvebu_pwm(chip);
> + struct gpio_desc *desc = gpio_to_desc(pwm->pwm);
This assumes that the GPIO chip base and the PWM chip base are the same.
You make sure of that by setting the PWM chip base to be the same as the
GPIO chip base, which is the easy way out. It's also somewhat brittle
because some other PWM chip may have occupied the region that you want
to use. It's fairly unlikely, but I think you can very easily side-step
any issues by simply doing:
struct mvebu_gpio_chip *mvchip = mvpwm->mvchip;
desc = gpio_to_desc(mvchip->chip.base + pwm->hwpwm);
Now that's somewhat complicated, but you only have to do it once if you
store the desc in mvpwm->gpio as pointed out above.
> + unsigned long flags;
> + int ret = 0;
> +
> + spin_lock_irqsave(&mvpwm->lock, flags);
> + if (mvpwm->used) {
> + ret = -EBUSY;
Then you can also easily use mvpwm->gpio to check for -EBUSY here.
> + } else {
> + if (!desc) {
> + ret = -ENODEV;
> + goto out;
> + }
> + ret = gpiod_request(desc, "mvebu-pwm");
> + if (ret)
> + goto out;
> +
> + ret = gpiod_direction_output(desc, 0);
> + if (ret) {
> + gpiod_free(desc);
> + goto out;
> + }
> +
> + mvpwm->used = true;
> + }
> +
> +out:
> + spin_unlock_irqrestore(&mvpwm->lock, flags);
> + return ret;
> +}
> +
> +static void mvebu_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> + struct mvebu_pwm *mvpwm = to_mvebu_pwm(chip);
> + struct gpio_desc *desc = gpio_to_desc(pwm->pwm);
> + unsigned long flags;
> +
> + spin_lock_irqsave(&mvpwm->lock, flags);
> + gpiod_free(desc);
> + mvpwm->used = false;
> + spin_unlock_irqrestore(&mvpwm->lock, flags);
> +}
> +
> +static void mvebu_pwm_get_state(struct pwm_chip *chip,
> + struct pwm_device *pwm,
> + struct pwm_state *state) {
> +
> + struct mvebu_pwm *mvpwm = to_mvebu_pwm(chip);
> + struct mvebu_gpio_chip *mvchip = mvpwm->mvchip;
> + unsigned long long val;
> + unsigned long flags;
> + u32 u;
> +
> + spin_lock_irqsave(&mvpwm->lock, flags);
> +
> + val = (unsigned long long)
> + readl_relaxed(mvebu_pwmreg_blink_on_duration);
> + val *= NSEC_PER_SEC;
> + do_div(val, mvpwm->clk_rate);
> + if (val > UINT_MAX)
> + state->duty_cycle = UINT_MAX;
> + else if (val)
> + state->duty_cycle = val;
> + else
> + state->duty_cycle = 1;
> +
> + val = (unsigned long long)
> + readl_relaxed(mvebu_pwmreg_blink_off_duration);
> + val *= NSEC_PER_SEC;
> + do_div(val, mvpwm->clk_rate);
> + if (val < state->duty_cycle) {
> + state->period = 1;
> + } else {
> + val -= state->duty_cycle;
> + if (val > UINT_MAX)
> + state->period = UINT_MAX;
> + else if (val)
> + state->period = val;
> + else
> + state->period = 1;
> + }
> +
> + u = readl_relaxed(mvebu_gpioreg_blink(mvchip));
> + if (u)
> + state->enabled = true;
> + else
> + state->enabled = false;
> +
> + spin_unlock_irqrestore(&mvpwm->lock, flags);
> +}
> +
> +static int mvebu_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> + struct pwm_state *state)
> +{
> + struct mvebu_pwm *mvpwm = to_mvebu_pwm(chip);
> + struct mvebu_gpio_chip *mvchip = mvpwm->mvchip;
> + unsigned long long val;
> + unsigned long flags;
> + unsigned int on, off;
> +
> + val = (unsigned long long) mvpwm->clk_rate * state->duty_cycle;
> + do_div(val, NSEC_PER_SEC);
> + if (val > UINT_MAX)
> + return -EINVAL;
> + if (val)
> + on = val;
> + else
> + on = 1;
> +
> + val = (unsigned long long) mvpwm->clk_rate *
> + (state->period - state->duty_cycle);
> + do_div(val, NSEC_PER_SEC);
> + if (val > UINT_MAX)
> + return -EINVAL;
> + if (val)
> + off = val;
> + else
> + off = 1;
> +
> + spin_lock_irqsave(&mvpwm->lock, flags);
> +
> + writel_relaxed(on, mvebu_pwmreg_blink_on_duration(mvpwm));
> + writel_relaxed(off, mvebu_pwmreg_blink_off_duration(mvpwm));
> + if (state->enabled)
> + mvebu_gpio_blink(&mvchip->chip, pwm->hwpwm, 1);
> + else
> + mvebu_gpio_blink(&mvchip->chip, pwm->hwpwm, 0);
> +
> + spin_unlock_irqrestore(&mvpwm->lock, flags);
> +
> + return 0;
> +}
> +
> +static const struct pwm_ops mvebu_pwm_ops = {
> + .request = mvebu_pwm_request,
> + .free = mvebu_pwm_free,
> + .get_state = mvebu_pwm_get_state,
> + .apply = mvebu_pwm_apply,
> + .owner = THIS_MODULE,
> +};
> +
> +static void __maybe_unused mvebu_pwm_suspend(struct mvebu_gpio_chip *mvchip)
> +{
> + struct mvebu_pwm *mvpwm = mvchip->mvpwm;
> +
> + mvpwm->blink_select =
> + readl_relaxed(mvebu_gpioreg_blink_counter_select(mvchip));
> + mvpwm->blink_on_duration =
> + readl_relaxed(mvebu_pwmreg_blink_on_duration(mvpwm));
> + mvpwm->blink_off_duration =
> + readl_relaxed(mvebu_pwmreg_blink_off_duration(mvpwm));
> +}
> +
> +static void __maybe_unused mvebu_pwm_resume(struct mvebu_gpio_chip *mvchip)
> +{
> + struct mvebu_pwm *mvpwm = mvchip->mvpwm;
> +
> + writel_relaxed(mvpwm->blink_select,
> + mvebu_gpioreg_blink_counter_select(mvchip));
> + writel_relaxed(mvpwm->blink_on_duration,
> + mvebu_pwmreg_blink_on_duration(mvpwm));
> + writel_relaxed(mvpwm->blink_off_duration,
> + mvebu_pwmreg_blink_off_duration(mvpwm));
> +}
> +
> +static int mvebu_pwm_probe(struct platform_device *pdev,
> + struct mvebu_gpio_chip *mvchip,
> + int id)
> +{
> + struct device *dev = &pdev->dev;
> + struct mvebu_pwm *mvpwm;
> + struct resource *res;
> +
> + if (!of_device_is_compatible(mvchip->chip.of_node,
> + "marvell,armada-370-xp-gpio"))
> + return 0;
> + /*
> + * There are only two sets of PWM configuration registers for
> + * all the GPIO lines on those SoCs which this driver reserves
> + * for the first two GPIO chips. So if the resource is missing
> + * we can't treat it as an error.
> + */
> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "pwm");
> + if (!res)
> + return 0;
> +
> + /*
> + * Use set A for lines of GPIO chip with id 0, B for GPIO chip
> + * with id 1. Don't allow further GPIO chips to be used for PWM.
> + */
> + if (id == 0)
> + writel_relaxed(0, mvebu_gpioreg_blink_counter_select(mvchip));
> + else if (id == 1)
> + writel_relaxed(U32_MAX,
> + mvebu_gpioreg_blink_counter_select(mvchip));
You could've just set a variable and then call writel_relaxed() after
the return -EINVAL below.
> + else
> + return -EINVAL;
> +
> + mvpwm = devm_kzalloc(dev, sizeof(struct mvebu_pwm), GFP_KERNEL);
> + if (!mvpwm)
> + return -ENOMEM;
> + mvchip->mvpwm = mvpwm;
> + mvpwm->mvchip = mvchip;
> +
> + mvpwm->membase = devm_ioremap_resource(dev, res);
> + if (IS_ERR(mvpwm->membase))
> + return PTR_ERR(mvpwm->membase);
> +
> + if (IS_ERR(mvchip->clk))
> + return PTR_ERR(mvchip->clk);
Maybe do this much earlier to avoid all the unnecessary register
accesses, allocations and mappings?
> +
> + mvpwm->clk_rate = clk_get_rate(mvchip->clk);
> + if (!mvpwm->clk_rate) {
> + dev_err(dev, "failed to get clock rate\n");
> + return -EINVAL;
> + }
> +
> + mvpwm->chip.dev = dev;
> + mvpwm->chip.ops = &mvebu_pwm_ops;
> + mvpwm->chip.base = mvchip->chip.base;
> + mvpwm->chip.npwm = mvchip->chip.ngpio;
I still would've done this differently. If you use this with a PWM user
you have to hook it up via DT anyway, so it doesn't matter whether you
specify the PWM index or the GPIO via some other property. The _only_
use-case where this might actually be an advantage is if you request a
PWM via the sysfs interface.
> + spin_lock_init(&mvpwm->lock);
> +
> + return pwmchip_add(&mvpwm->chip);
> +}
> +
> #ifdef CONFIG_DEBUG_FS
> #include <linux/seq_file.h>
>
> @@ -555,6 +842,10 @@ static const struct of_device_id mvebu_gpio_of_match[] = {
> .data = (void *) MVEBU_GPIO_SOC_VARIANT_ARMADAXP,
> },
> {
> + .compatible = "marvell,armada-370-xp-gpio",
> + .data = (void *) MVEBU_GPIO_SOC_VARIANT_ORION,
> + },
> + {
> /* sentinel */
> },
> };
> @@ -600,6 +891,9 @@ static int mvebu_gpio_suspend(struct platform_device *pdev, pm_message_t state)
> BUG();
> }
>
> + if (IS_ENABLED(CONFIG_PWM))
> + mvebu_pwm_suspend(mvchip);
> +
> return 0;
> }
>
> @@ -643,6 +937,9 @@ static int mvebu_gpio_resume(struct platform_device *pdev)
> BUG();
> }
>
> + if (IS_ENABLED(CONFIG_PWM))
> + mvebu_pwm_resume(mvchip);
> +
> return 0;
> }
>
> @@ -654,7 +951,6 @@ static int mvebu_gpio_probe(struct platform_device *pdev)
> struct resource *res;
> struct irq_chip_generic *gc;
> struct irq_chip_type *ct;
> - struct clk *clk;
> unsigned int ngpios;
> bool have_irqs;
> int soc_variant;
> @@ -688,10 +984,10 @@ static int mvebu_gpio_probe(struct platform_device *pdev)
> return id;
> }
>
> - clk = devm_clk_get(&pdev->dev, NULL);
> + mvchip->clk = devm_clk_get(&pdev->dev, NULL);
> /* Not all SoCs require a clock.*/
> - if (!IS_ERR(clk))
> - clk_prepare_enable(clk);
> + if (!IS_ERR(mvchip->clk))
> + clk_prepare_enable(mvchip->clk);
>
> mvchip->soc_variant = soc_variant;
> mvchip->chip.label = dev_name(&pdev->dev);
> @@ -822,6 +1118,10 @@ static int mvebu_gpio_probe(struct platform_device *pdev)
> mvchip);
> }
>
> + /* Armada 370/XP has simple PWM support for GPIO lines */
> + if (IS_ENABLED(CONFIG_PWM))
> + return mvebu_pwm_probe(pdev, mvchip, id);
> +
> return 0;
>
> err_domain:
> --
> 2.10.2
All of my comments are effectively of a bikeshed nature, so from a PWM
perspective this is:
Acked-by: Thierry Reding <thierry.reding@gmail.com>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170412/5fdbbbe3/attachment-0001.sig>
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v5 1/4] gpio: mvebu: Add limited PWM support
2017-04-12 17:11 ` Thierry Reding
@ 2017-04-13 7:45 ` Ralph Sennhauser
-1 siblings, 0 replies; 40+ messages in thread
From: Ralph Sennhauser @ 2017-04-13 7:45 UTC (permalink / raw)
To: Thierry Reding
Cc: Andrew Lunn, Linus Walleij, Alexandre Courbot, Rob Herring,
Mark Rutland, Jason Cooper, Gregory Clement,
Sebastian Hesselbarth, Russell King, linux-pwm, linux-gpio,
devicetree, linux-kernel, linux-arm-kernel
On Wed, 12 Apr 2017 19:11:21 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:
> On Sun, Apr 09, 2017 at 08:09:27PM +0200, Ralph Sennhauser wrote:
> > From: Andrew Lunn <andrew@lunn.ch>
> >
> > Armada 370/XP devices can 'blink' GPIO lines with a configurable on
> > and off period. This can be modelled as a PWM.
> >
> > However, there are only two sets of PWM configuration registers for
> > all the GPIO lines. This driver simply allows a single GPIO line per
> > GPIO chip of 32 lines to be used as a PWM. Attempts to use more
> > return EBUSY.
> >
> > Due to the interleaving of registers it is not simple to separate
> > the PWM driver from the GPIO driver. Thus the GPIO driver has been
> > extended with a PWM driver.
> >
> > Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> > URL: https://patchwork.ozlabs.org/patch/427287/
> > URL: https://patchwork.ozlabs.org/patch/427295/
> > [Ralph Sennhauser:
> > * Port forward
> > * Merge PWM portion into gpio-mvebu.c
> > * Switch to atomic PWM API
> > * Add new compatible string marvell,armada-370-xp-gpio
> > * Update and merge documentation patch
> > * Update MAINTAINERS]
> > Signed-off-by: Ralph Sennhauser <ralph.sennhauser@gmail.com>
> > Tested-by: Andrew Lunn <andrew@lunn.ch>
> > ---
> > .../devicetree/bindings/gpio/gpio-mvebu.txt | 32 ++
> > MAINTAINERS | 2 +
> > drivers/gpio/gpio-mvebu.c | 324
> > ++++++++++++++++++++- 3 files changed, 346 insertions(+), 12
> > deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/gpio/gpio-mvebu.txt
> > b/Documentation/devicetree/bindings/gpio/gpio-mvebu.txt index
> > a6f3bec..fe49e9d 100644 ---
> > a/Documentation/devicetree/bindings/gpio/gpio-mvebu.txt +++
> > b/Documentation/devicetree/bindings/gpio/gpio-mvebu.txt @@ -38,6
> > +38,24 @@ Required properties:
> > - #gpio-cells: Should be two. The first cell is the pin number. The
> > second cell is reserved for flags, unused at the moment.
> >
> > +Optional properties:
> > +
> > +In order to use the gpio lines in PWM mode, some additional
> > optional +properties are required. Only Armada 370 and XP support
> > these properties. +
> > +- compatible: Must contain "marvell,armada-370-xp-gpio"
> > +
> > +- reg: an additional register set is needed, for the GPIO Blink
> > + Counter on/off registers.
> > +
> > +- reg-names: Must contain an entry "pwm" corresponding to the
> > + additional register range needed for pwm operation.
> > +
> > +- #pwm-cells: Should be two. The first cell is the GPIO line
> > number. The
> > + second cell is the period in nanoseconds.
> > +
> > +- clocks: Must be a phandle to the clock for the gpio controller.
> > +
> > Example:
> >
> > gpio0: gpio@d0018100 {
> > @@ -51,3 +69,17 @@ Example:
> > #interrupt-cells = <2>;
> > interrupts = <16>, <17>, <18>, <19>;
> > };
> > +
> > + gpio1: gpio@18140 {
> > + compatible = "marvell,armada-370-xp-gpio";
> > + reg = <0x18140 0x40>, <0x181c8 0x08>;
> > + reg-names = "gpio", "pwm";
> > + ngpios = <17>;
> > + gpio-controller;
> > + #gpio-cells = <2>;
> > + #pwm-cells = <2>;
> > + interrupt-controller;
> > + #interrupt-cells = <2>;
> > + interrupts = <87>, <88>, <89>;
> > + clocks = <&coreclk 0>;
> > + };
>
> This is going to need an Acked-by from one of the device tree
> maintainers. Rob and devicetree@vger.kernel.org are on Cc, but I
> suspect nobody might look for the binding change "hidden" in this
> patch.
>
> Maybe best to split this off into a separate patch, or explicitly ping
> Rob to look at this patch.
Hi Thierry,
Rob asked for the new compatible string so he did see it presumably. As
you prefer to have an ACK by him I'll see to getting one for the driver
(bindings part). The patch could be split but then one might want to
split it even further. Like this the first patch in the series is a nice
self contained package.
>
<snip/>
>
> > +
> > + mvpwm->clk_rate = clk_get_rate(mvchip->clk);
> > + if (!mvpwm->clk_rate) {
> > + dev_err(dev, "failed to get clock rate\n");
> > + return -EINVAL;
> > + }
> > +
> > + mvpwm->chip.dev = dev;
> > + mvpwm->chip.ops = &mvebu_pwm_ops;
> > + mvpwm->chip.base = mvchip->chip.base;
> > + mvpwm->chip.npwm = mvchip->chip.ngpio;
>
> I still would've done this differently. If you use this with a PWM
> user you have to hook it up via DT anyway, so it doesn't matter
> whether you specify the PWM index or the GPIO via some other
> property. The _only_ use-case where this might actually be an
> advantage is if you request a PWM via the sysfs interface.
Let me answer this in the other mail where you bring this up.
>
<snip/>
> All of my comments are effectively of a bikeshed nature, so from a PWM
> perspective this is:
>
> Acked-by: Thierry Reding <thierry.reding@gmail.com>
Thanks for the detailed review and the ACK. Will work on all the
mentioned bits for v6.
Ralph
^ permalink raw reply [flat|nested] 40+ messages in thread* [PATCH v5 1/4] gpio: mvebu: Add limited PWM support
@ 2017-04-13 7:45 ` Ralph Sennhauser
0 siblings, 0 replies; 40+ messages in thread
From: Ralph Sennhauser @ 2017-04-13 7:45 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, 12 Apr 2017 19:11:21 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:
> On Sun, Apr 09, 2017 at 08:09:27PM +0200, Ralph Sennhauser wrote:
> > From: Andrew Lunn <andrew@lunn.ch>
> >
> > Armada 370/XP devices can 'blink' GPIO lines with a configurable on
> > and off period. This can be modelled as a PWM.
> >
> > However, there are only two sets of PWM configuration registers for
> > all the GPIO lines. This driver simply allows a single GPIO line per
> > GPIO chip of 32 lines to be used as a PWM. Attempts to use more
> > return EBUSY.
> >
> > Due to the interleaving of registers it is not simple to separate
> > the PWM driver from the GPIO driver. Thus the GPIO driver has been
> > extended with a PWM driver.
> >
> > Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> > URL: https://patchwork.ozlabs.org/patch/427287/
> > URL: https://patchwork.ozlabs.org/patch/427295/
> > [Ralph Sennhauser:
> > * Port forward
> > * Merge PWM portion into gpio-mvebu.c
> > * Switch to atomic PWM API
> > * Add new compatible string marvell,armada-370-xp-gpio
> > * Update and merge documentation patch
> > * Update MAINTAINERS]
> > Signed-off-by: Ralph Sennhauser <ralph.sennhauser@gmail.com>
> > Tested-by: Andrew Lunn <andrew@lunn.ch>
> > ---
> > .../devicetree/bindings/gpio/gpio-mvebu.txt | 32 ++
> > MAINTAINERS | 2 +
> > drivers/gpio/gpio-mvebu.c | 324
> > ++++++++++++++++++++- 3 files changed, 346 insertions(+), 12
> > deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/gpio/gpio-mvebu.txt
> > b/Documentation/devicetree/bindings/gpio/gpio-mvebu.txt index
> > a6f3bec..fe49e9d 100644 ---
> > a/Documentation/devicetree/bindings/gpio/gpio-mvebu.txt +++
> > b/Documentation/devicetree/bindings/gpio/gpio-mvebu.txt @@ -38,6
> > +38,24 @@ Required properties:
> > - #gpio-cells: Should be two. The first cell is the pin number. The
> > second cell is reserved for flags, unused at the moment.
> >
> > +Optional properties:
> > +
> > +In order to use the gpio lines in PWM mode, some additional
> > optional +properties are required. Only Armada 370 and XP support
> > these properties. +
> > +- compatible: Must contain "marvell,armada-370-xp-gpio"
> > +
> > +- reg: an additional register set is needed, for the GPIO Blink
> > + Counter on/off registers.
> > +
> > +- reg-names: Must contain an entry "pwm" corresponding to the
> > + additional register range needed for pwm operation.
> > +
> > +- #pwm-cells: Should be two. The first cell is the GPIO line
> > number. The
> > + second cell is the period in nanoseconds.
> > +
> > +- clocks: Must be a phandle to the clock for the gpio controller.
> > +
> > Example:
> >
> > gpio0: gpio at d0018100 {
> > @@ -51,3 +69,17 @@ Example:
> > #interrupt-cells = <2>;
> > interrupts = <16>, <17>, <18>, <19>;
> > };
> > +
> > + gpio1: gpio at 18140 {
> > + compatible = "marvell,armada-370-xp-gpio";
> > + reg = <0x18140 0x40>, <0x181c8 0x08>;
> > + reg-names = "gpio", "pwm";
> > + ngpios = <17>;
> > + gpio-controller;
> > + #gpio-cells = <2>;
> > + #pwm-cells = <2>;
> > + interrupt-controller;
> > + #interrupt-cells = <2>;
> > + interrupts = <87>, <88>, <89>;
> > + clocks = <&coreclk 0>;
> > + };
>
> This is going to need an Acked-by from one of the device tree
> maintainers. Rob and devicetree at vger.kernel.org are on Cc, but I
> suspect nobody might look for the binding change "hidden" in this
> patch.
>
> Maybe best to split this off into a separate patch, or explicitly ping
> Rob to look at this patch.
Hi Thierry,
Rob asked for the new compatible string so he did see it presumably. As
you prefer to have an ACK by him I'll see to getting one for the driver
(bindings part). The patch could be split but then one might want to
split it even further. Like this the first patch in the series is a nice
self contained package.
>
<snip/>
>
> > +
> > + mvpwm->clk_rate = clk_get_rate(mvchip->clk);
> > + if (!mvpwm->clk_rate) {
> > + dev_err(dev, "failed to get clock rate\n");
> > + return -EINVAL;
> > + }
> > +
> > + mvpwm->chip.dev = dev;
> > + mvpwm->chip.ops = &mvebu_pwm_ops;
> > + mvpwm->chip.base = mvchip->chip.base;
> > + mvpwm->chip.npwm = mvchip->chip.ngpio;
>
> I still would've done this differently. If you use this with a PWM
> user you have to hook it up via DT anyway, so it doesn't matter
> whether you specify the PWM index or the GPIO via some other
> property. The _only_ use-case where this might actually be an
> advantage is if you request a PWM via the sysfs interface.
Let me answer this in the other mail where you bring this up.
>
<snip/>
> All of my comments are effectively of a bikeshed nature, so from a PWM
> perspective this is:
>
> Acked-by: Thierry Reding <thierry.reding@gmail.com>
Thanks for the detailed review and the ACK. Will work on all the
mentioned bits for v6.
Ralph
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v5 1/4] gpio: mvebu: Add limited PWM support
2017-04-09 18:09 ` Ralph Sennhauser
@ 2017-04-12 17:21 ` Thierry Reding
-1 siblings, 0 replies; 40+ messages in thread
From: Thierry Reding @ 2017-04-12 17:21 UTC (permalink / raw)
To: Ralph Sennhauser
Cc: Andrew Lunn, Linus Walleij, Alexandre Courbot, Rob Herring,
Mark Rutland, Jason Cooper, Gregory Clement,
Sebastian Hesselbarth, Russell King, linux-pwm, linux-gpio,
devicetree, linux-kernel, linux-arm-kernel
[-- Attachment #1: Type: text/plain, Size: 2739 bytes --]
On Sun, Apr 09, 2017 at 08:09:27PM +0200, Ralph Sennhauser wrote:
> From: Andrew Lunn <andrew@lunn.ch>
>
> Armada 370/XP devices can 'blink' GPIO lines with a configurable on
> and off period. This can be modelled as a PWM.
>
> However, there are only two sets of PWM configuration registers for
> all the GPIO lines. This driver simply allows a single GPIO line per
> GPIO chip of 32 lines to be used as a PWM. Attempts to use more return
> EBUSY.
>
> Due to the interleaving of registers it is not simple to separate the
> PWM driver from the GPIO driver. Thus the GPIO driver has been
> extended with a PWM driver.
>
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> URL: https://patchwork.ozlabs.org/patch/427287/
> URL: https://patchwork.ozlabs.org/patch/427295/
> [Ralph Sennhauser:
> * Port forward
> * Merge PWM portion into gpio-mvebu.c
> * Switch to atomic PWM API
> * Add new compatible string marvell,armada-370-xp-gpio
> * Update and merge documentation patch
> * Update MAINTAINERS]
> Signed-off-by: Ralph Sennhauser <ralph.sennhauser@gmail.com>
> Tested-by: Andrew Lunn <andrew@lunn.ch>
> ---
> .../devicetree/bindings/gpio/gpio-mvebu.txt | 32 ++
> MAINTAINERS | 2 +
> drivers/gpio/gpio-mvebu.c | 324 ++++++++++++++++++++-
> 3 files changed, 346 insertions(+), 12 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/gpio/gpio-mvebu.txt b/Documentation/devicetree/bindings/gpio/gpio-mvebu.txt
> index a6f3bec..fe49e9d 100644
> --- a/Documentation/devicetree/bindings/gpio/gpio-mvebu.txt
> +++ b/Documentation/devicetree/bindings/gpio/gpio-mvebu.txt
> @@ -38,6 +38,24 @@ Required properties:
> - #gpio-cells: Should be two. The first cell is the pin number. The
> second cell is reserved for flags, unused at the moment.
>
> +Optional properties:
> +
> +In order to use the gpio lines in PWM mode, some additional optional
> +properties are required. Only Armada 370 and XP support these properties.
> +
> +- compatible: Must contain "marvell,armada-370-xp-gpio"
> +
> +- reg: an additional register set is needed, for the GPIO Blink
> + Counter on/off registers.
> +
> +- reg-names: Must contain an entry "pwm" corresponding to the
> + additional register range needed for pwm operation.
> +
> +- #pwm-cells: Should be two. The first cell is the GPIO line number. The
> + second cell is the period in nanoseconds.
> +
> +- clocks: Must be a phandle to the clock for the gpio controller.
One other thing: there's a mix of pwm/PWM and gpio/GPIO in this hunk. In
prose, always use the all-uppercase variants because they are
abbreviations.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v5 1/4] gpio: mvebu: Add limited PWM support
@ 2017-04-12 17:21 ` Thierry Reding
0 siblings, 0 replies; 40+ messages in thread
From: Thierry Reding @ 2017-04-12 17:21 UTC (permalink / raw)
To: linux-arm-kernel
On Sun, Apr 09, 2017 at 08:09:27PM +0200, Ralph Sennhauser wrote:
> From: Andrew Lunn <andrew@lunn.ch>
>
> Armada 370/XP devices can 'blink' GPIO lines with a configurable on
> and off period. This can be modelled as a PWM.
>
> However, there are only two sets of PWM configuration registers for
> all the GPIO lines. This driver simply allows a single GPIO line per
> GPIO chip of 32 lines to be used as a PWM. Attempts to use more return
> EBUSY.
>
> Due to the interleaving of registers it is not simple to separate the
> PWM driver from the GPIO driver. Thus the GPIO driver has been
> extended with a PWM driver.
>
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> URL: https://patchwork.ozlabs.org/patch/427287/
> URL: https://patchwork.ozlabs.org/patch/427295/
> [Ralph Sennhauser:
> * Port forward
> * Merge PWM portion into gpio-mvebu.c
> * Switch to atomic PWM API
> * Add new compatible string marvell,armada-370-xp-gpio
> * Update and merge documentation patch
> * Update MAINTAINERS]
> Signed-off-by: Ralph Sennhauser <ralph.sennhauser@gmail.com>
> Tested-by: Andrew Lunn <andrew@lunn.ch>
> ---
> .../devicetree/bindings/gpio/gpio-mvebu.txt | 32 ++
> MAINTAINERS | 2 +
> drivers/gpio/gpio-mvebu.c | 324 ++++++++++++++++++++-
> 3 files changed, 346 insertions(+), 12 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/gpio/gpio-mvebu.txt b/Documentation/devicetree/bindings/gpio/gpio-mvebu.txt
> index a6f3bec..fe49e9d 100644
> --- a/Documentation/devicetree/bindings/gpio/gpio-mvebu.txt
> +++ b/Documentation/devicetree/bindings/gpio/gpio-mvebu.txt
> @@ -38,6 +38,24 @@ Required properties:
> - #gpio-cells: Should be two. The first cell is the pin number. The
> second cell is reserved for flags, unused at the moment.
>
> +Optional properties:
> +
> +In order to use the gpio lines in PWM mode, some additional optional
> +properties are required. Only Armada 370 and XP support these properties.
> +
> +- compatible: Must contain "marvell,armada-370-xp-gpio"
> +
> +- reg: an additional register set is needed, for the GPIO Blink
> + Counter on/off registers.
> +
> +- reg-names: Must contain an entry "pwm" corresponding to the
> + additional register range needed for pwm operation.
> +
> +- #pwm-cells: Should be two. The first cell is the GPIO line number. The
> + second cell is the period in nanoseconds.
> +
> +- clocks: Must be a phandle to the clock for the gpio controller.
One other thing: there's a mix of pwm/PWM and gpio/GPIO in this hunk. In
prose, always use the all-uppercase variants because they are
abbreviations.
Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170412/d8af46fc/attachment.sig>
^ permalink raw reply [flat|nested] 40+ messages in thread
[parent not found: <20170409180931.4884-2-ralph.sennhauser-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* Re: [PATCH v5 1/4] gpio: mvebu: Add limited PWM support
2017-04-09 18:09 ` Ralph Sennhauser
(?)
@ 2017-04-13 20:14 ` Rob Herring
-1 siblings, 0 replies; 40+ messages in thread
From: Rob Herring @ 2017-04-13 20:14 UTC (permalink / raw)
To: Ralph Sennhauser
Cc: Thierry Reding, Andrew Lunn, Linus Walleij, Alexandre Courbot,
Mark Rutland, Jason Cooper, Gregory Clement,
Sebastian Hesselbarth, Russell King,
linux-pwm-u79uwXL29TY76Z2rM5mHXA,
linux-gpio-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
On Sun, Apr 09, 2017 at 08:09:27PM +0200, Ralph Sennhauser wrote:
> From: Andrew Lunn <andrew-g2DYL2Zd6BY@public.gmane.org>
>
> Armada 370/XP devices can 'blink' GPIO lines with a configurable on
> and off period. This can be modelled as a PWM.
>
> However, there are only two sets of PWM configuration registers for
> all the GPIO lines. This driver simply allows a single GPIO line per
> GPIO chip of 32 lines to be used as a PWM. Attempts to use more return
> EBUSY.
>
> Due to the interleaving of registers it is not simple to separate the
> PWM driver from the GPIO driver. Thus the GPIO driver has been
> extended with a PWM driver.
>
> Signed-off-by: Andrew Lunn <andrew-g2DYL2Zd6BY@public.gmane.org>
> URL: https://patchwork.ozlabs.org/patch/427287/
> URL: https://patchwork.ozlabs.org/patch/427295/
> [Ralph Sennhauser:
> * Port forward
> * Merge PWM portion into gpio-mvebu.c
> * Switch to atomic PWM API
> * Add new compatible string marvell,armada-370-xp-gpio
> * Update and merge documentation patch
> * Update MAINTAINERS]
> Signed-off-by: Ralph Sennhauser <ralph.sennhauser-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Tested-by: Andrew Lunn <andrew-g2DYL2Zd6BY@public.gmane.org>
> ---
> .../devicetree/bindings/gpio/gpio-mvebu.txt | 32 ++
> MAINTAINERS | 2 +
Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> drivers/gpio/gpio-mvebu.c | 324 ++++++++++++++++++++-
> 3 files changed, 346 insertions(+), 12 deletions(-)
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v5 1/4] gpio: mvebu: Add limited PWM support
@ 2017-04-13 20:14 ` Rob Herring
0 siblings, 0 replies; 40+ messages in thread
From: Rob Herring @ 2017-04-13 20:14 UTC (permalink / raw)
To: linux-arm-kernel
On Sun, Apr 09, 2017 at 08:09:27PM +0200, Ralph Sennhauser wrote:
> From: Andrew Lunn <andrew@lunn.ch>
>
> Armada 370/XP devices can 'blink' GPIO lines with a configurable on
> and off period. This can be modelled as a PWM.
>
> However, there are only two sets of PWM configuration registers for
> all the GPIO lines. This driver simply allows a single GPIO line per
> GPIO chip of 32 lines to be used as a PWM. Attempts to use more return
> EBUSY.
>
> Due to the interleaving of registers it is not simple to separate the
> PWM driver from the GPIO driver. Thus the GPIO driver has been
> extended with a PWM driver.
>
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> URL: https://patchwork.ozlabs.org/patch/427287/
> URL: https://patchwork.ozlabs.org/patch/427295/
> [Ralph Sennhauser:
> * Port forward
> * Merge PWM portion into gpio-mvebu.c
> * Switch to atomic PWM API
> * Add new compatible string marvell,armada-370-xp-gpio
> * Update and merge documentation patch
> * Update MAINTAINERS]
> Signed-off-by: Ralph Sennhauser <ralph.sennhauser@gmail.com>
> Tested-by: Andrew Lunn <andrew@lunn.ch>
> ---
> .../devicetree/bindings/gpio/gpio-mvebu.txt | 32 ++
> MAINTAINERS | 2 +
Acked-by: Rob Herring <robh@kernel.org>
> drivers/gpio/gpio-mvebu.c | 324 ++++++++++++++++++++-
> 3 files changed, 346 insertions(+), 12 deletions(-)
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v5 1/4] gpio: mvebu: Add limited PWM support
@ 2017-04-13 20:14 ` Rob Herring
0 siblings, 0 replies; 40+ messages in thread
From: Rob Herring @ 2017-04-13 20:14 UTC (permalink / raw)
To: Ralph Sennhauser
Cc: Thierry Reding, Andrew Lunn, Linus Walleij, Alexandre Courbot,
Mark Rutland, Jason Cooper, Gregory Clement,
Sebastian Hesselbarth, Russell King, linux-pwm, linux-gpio,
devicetree, linux-kernel, linux-arm-kernel
On Sun, Apr 09, 2017 at 08:09:27PM +0200, Ralph Sennhauser wrote:
> From: Andrew Lunn <andrew@lunn.ch>
>
> Armada 370/XP devices can 'blink' GPIO lines with a configurable on
> and off period. This can be modelled as a PWM.
>
> However, there are only two sets of PWM configuration registers for
> all the GPIO lines. This driver simply allows a single GPIO line per
> GPIO chip of 32 lines to be used as a PWM. Attempts to use more return
> EBUSY.
>
> Due to the interleaving of registers it is not simple to separate the
> PWM driver from the GPIO driver. Thus the GPIO driver has been
> extended with a PWM driver.
>
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> URL: https://patchwork.ozlabs.org/patch/427287/
> URL: https://patchwork.ozlabs.org/patch/427295/
> [Ralph Sennhauser:
> * Port forward
> * Merge PWM portion into gpio-mvebu.c
> * Switch to atomic PWM API
> * Add new compatible string marvell,armada-370-xp-gpio
> * Update and merge documentation patch
> * Update MAINTAINERS]
> Signed-off-by: Ralph Sennhauser <ralph.sennhauser@gmail.com>
> Tested-by: Andrew Lunn <andrew@lunn.ch>
> ---
> .../devicetree/bindings/gpio/gpio-mvebu.txt | 32 ++
> MAINTAINERS | 2 +
Acked-by: Rob Herring <robh@kernel.org>
> drivers/gpio/gpio-mvebu.c | 324 ++++++++++++++++++++-
> 3 files changed, 346 insertions(+), 12 deletions(-)
^ permalink raw reply [flat|nested] 40+ messages in thread