Hi,
This patch series adds support for the PWM controller found on the Allwinner
SoCs.
The first patch adds the driver itself.
The second patch adds the DT binding documentation
Changes in v8:
- renamed the driver sun4i as the PWM IP is different in the next sunxi SoCs
- Took into account comments from Thierry
Changes in v9:
- moved from using a mutex to a spinlock
Alexandre Belloni (2):
pwm: Add Allwinner SoC support
pwm: sunxi: document OF bindings
.../devicetree/bindings/pwm/pwm-sun4i.txt | 20 ++
drivers/pwm/Kconfig | 9 +
drivers/pwm/Makefile | 1 +
drivers/pwm/pwm-sun4i.c | 366 +++++++++++++++++++++
4 files changed, 396 insertions(+)
create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sun4i.txt
create mode 100644 drivers/pwm/pwm-sun4i.c
--
2.1.0
This adds a generic PWM framework driver for the PWM controller
found on Allwinner SoCs.
Signed-off-by: Alexandre Belloni <[email protected]>
Acked-by: Maxime Ripard <[email protected]>
---
drivers/pwm/Kconfig | 9 ++
drivers/pwm/Makefile | 1 +
drivers/pwm/pwm-sun4i.c | 366 ++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 376 insertions(+)
create mode 100644 drivers/pwm/pwm-sun4i.c
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 3865dfb9ed08..424359d3cbb1 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -262,6 +262,15 @@ config PWM_STI
To compile this driver as a module, choose M here: the module
will be called pwm-sti.
+config PWM_SUN4I
+ tristate "Allwinner PWM support"
+ depends on ARCH_SUNXI || COMPILE_TEST
+ help
+ Generic PWM framework driver for Allwinner SoCs.
+
+ To compile this driver as a module, choose M here: the module
+ will be called pwm-sunxi.
+
config PWM_TEGRA
tristate "NVIDIA Tegra PWM support"
depends on ARCH_TEGRA
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index c458606c3755..d607804deea1 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -24,6 +24,7 @@ obj-$(CONFIG_PWM_ROCKCHIP) += pwm-rockchip.o
obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o
obj-$(CONFIG_PWM_SPEAR) += pwm-spear.o
obj-$(CONFIG_PWM_STI) += pwm-sti.o
+obj-$(CONFIG_PWM_SUN4I) += pwm-sun4i.o
obj-$(CONFIG_PWM_TEGRA) += pwm-tegra.o
obj-$(CONFIG_PWM_TIECAP) += pwm-tiecap.o
obj-$(CONFIG_PWM_TIEHRPWM) += pwm-tiehrpwm.o
diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
new file mode 100644
index 000000000000..e2c3b1379f77
--- /dev/null
+++ b/drivers/pwm/pwm-sun4i.c
@@ -0,0 +1,366 @@
+/*
+ * Driver for Allwinner sun4i Pulse Width Modulation Controller
+ *
+ * Copyright (C) 2014 Alexandre Belloni <[email protected]>
+ *
+ * Licensed under GPLv2.
+ */
+
+#include <linux/bitops.h>
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+#include <linux/time.h>
+
+#define PWM_CTRL_REG 0x0
+
+#define PWM_CH_PRD_BASE 0x4
+#define PWM_CH_PRD_OFFSET 0x4
+#define PWM_CH_PRD(ch) (PWM_CH_PRD_BASE + PWM_CH_PRD_OFFSET * (ch))
+
+#define PWMCH_OFFSET 15
+#define PWM_PRESCAL_MASK GENMASK(3, 0)
+#define PWM_PRESCAL_OFF 0
+#define PWM_EN BIT(4)
+#define PWM_ACT_STATE BIT(5)
+#define PWM_CLK_GATING BIT(6)
+#define PWM_MODE BIT(7)
+#define PWM_PULSE BIT(8)
+#define PWM_BYPASS BIT(9)
+
+#define PWM_RDY_BASE 28
+#define PWM_RDY_OFFSET 1
+#define PWM_RDY(ch) BIT(PWM_RDY_BASE + PWM_RDY_OFFSET * (ch))
+
+#define PWM_PRD(prd) (((prd) - 1) << 16)
+#define PWM_PRD_MASK GENMASK(15, 0)
+
+#define PWM_DTY_MASK GENMASK(15, 0)
+
+#define BIT_CH(bit, chan) ((bit) << ((chan) * PWMCH_OFFSET))
+
+static const u32 prescaler_table[] = {
+ 120,
+ 180,
+ 240,
+ 360,
+ 480,
+ 0,
+ 0,
+ 0,
+ 12000,
+ 24000,
+ 36000,
+ 48000,
+ 72000,
+ 0,
+ 0,
+ 0, /* Actually 1 but tested separately */
+};
+
+struct sun4i_pwm_data {
+ bool has_prescaler_bypass;
+ bool has_rdy;
+};
+
+struct sun4i_pwm_chip {
+ struct pwm_chip chip;
+ struct clk *clk;
+ void __iomem *base;
+ spinlock_t ctrl_lock;
+ const struct sun4i_pwm_data *data;
+};
+
+static inline struct sun4i_pwm_chip *to_sun4i_pwm_chip(struct pwm_chip *chip)
+{
+ return container_of(chip, struct sun4i_pwm_chip, chip);
+}
+
+static inline u32 sun4i_pwm_readl(struct sun4i_pwm_chip *chip,
+ unsigned long offset)
+{
+ return readl(chip->base + offset);
+}
+
+static inline void sun4i_pwm_writel(struct sun4i_pwm_chip *chip,
+ u32 val, unsigned long offset)
+{
+ writel(val, chip->base + offset);
+}
+
+static int sun4i_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
+ int duty_ns, int period_ns)
+{
+ struct sun4i_pwm_chip *sun4i_pwm = to_sun4i_pwm_chip(chip);
+ u32 clk_rate, prd, dty, val, clk_gate;
+ u64 div = 0;
+ unsigned int prescaler = 0;
+ int err;
+
+ clk_rate = clk_get_rate(sun4i_pwm->clk);
+
+ if (sun4i_pwm->data->has_prescaler_bypass) {
+ /* First, test without any prescaler when available */
+ prescaler = PWM_PRESCAL_MASK;
+ /*
+ * When not using any prescaler, the clock period in nanoseconds
+ * is not an integer so round it half up instead of
+ * truncating to get less surprising values.
+ */
+ div = clk_rate * (u64)period_ns + NSEC_PER_SEC/2;
+ do_div(div, NSEC_PER_SEC);
+ if (div - 1 > PWM_PRD_MASK)
+ prescaler = 0;
+ }
+
+ if (prescaler == 0) {
+ /* Go up from the first divider */
+ for (prescaler = 0; prescaler < PWM_PRESCAL_MASK; prescaler++) {
+ if (!prescaler_table[prescaler])
+ continue;
+ div = clk_rate / prescaler_table[prescaler];
+ div = div * (u64)period_ns;
+ do_div(div, NSEC_PER_SEC);
+ if (div - 1 <= PWM_PRD_MASK)
+ break;
+ }
+
+ if (div - 1 > PWM_PRD_MASK) {
+ dev_err(chip->dev, "period exceeds the maximum value\n");
+ return -EINVAL;
+ }
+ }
+
+ prd = div;
+ div *= duty_ns;
+ do_div(div, period_ns);
+ dty = div;
+
+ err = clk_prepare_enable(sun4i_pwm->clk);
+ if (err) {
+ dev_err(chip->dev, "failed to enable PWM clock\n");
+ return err;
+ }
+
+ spin_lock(&sun4i_pwm->ctrl_lock);
+ val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
+
+ if (sun4i_pwm->data->has_rdy && (val & PWM_RDY(pwm->hwpwm))) {
+ spin_unlock(&sun4i_pwm->ctrl_lock);
+ clk_disable_unprepare(sun4i_pwm->clk);
+ return -EBUSY;
+ }
+
+ clk_gate = val & BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
+ if (clk_gate) {
+ val &= ~BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
+ sun4i_pwm_writel(sun4i_pwm, val, PWM_CTRL_REG);
+ }
+
+ val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
+ val &= ~BIT_CH(PWM_PRESCAL_MASK, pwm->hwpwm);
+ val |= BIT_CH(prescaler, pwm->hwpwm);
+ sun4i_pwm_writel(sun4i_pwm, val, PWM_CTRL_REG);
+
+ val = (dty & PWM_DTY_MASK) | PWM_PRD(prd);
+ sun4i_pwm_writel(sun4i_pwm, val, PWM_CH_PRD(pwm->hwpwm));
+
+ if (clk_gate) {
+ val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
+ val |= clk_gate;
+ sun4i_pwm_writel(sun4i_pwm, val, PWM_CTRL_REG);
+ }
+
+ spin_unlock(&sun4i_pwm->ctrl_lock);
+ clk_disable_unprepare(sun4i_pwm->clk);
+
+ return 0;
+}
+
+static int sun4i_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
+ enum pwm_polarity polarity)
+{
+ struct sun4i_pwm_chip *sun4i_pwm = to_sun4i_pwm_chip(chip);
+ u32 val;
+ int ret;
+
+ ret = clk_prepare_enable(sun4i_pwm->clk);
+ if (ret) {
+ dev_err(chip->dev, "failed to enable PWM clock\n");
+ return ret;
+ }
+
+ spin_lock(&sun4i_pwm->ctrl_lock);
+ val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
+
+ if (polarity != PWM_POLARITY_NORMAL)
+ val &= ~BIT_CH(PWM_ACT_STATE, pwm->hwpwm);
+ else
+ val |= BIT_CH(PWM_ACT_STATE, pwm->hwpwm);
+
+ sun4i_pwm_writel(sun4i_pwm, val, PWM_CTRL_REG);
+
+ spin_unlock(&sun4i_pwm->ctrl_lock);
+ clk_disable_unprepare(sun4i_pwm->clk);
+
+ return 0;
+}
+
+static int sun4i_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+ struct sun4i_pwm_chip *sun4i_pwm = to_sun4i_pwm_chip(chip);
+ u32 val;
+ int ret;
+
+ ret = clk_prepare_enable(sun4i_pwm->clk);
+ if (ret) {
+ dev_err(chip->dev, "failed to enable PWM clock\n");
+ return ret;
+ }
+
+ spin_lock(&sun4i_pwm->ctrl_lock);
+ val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
+ val |= BIT_CH(PWM_EN, pwm->hwpwm);
+ val |= BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
+ sun4i_pwm_writel(sun4i_pwm, val, PWM_CTRL_REG);
+ spin_unlock(&sun4i_pwm->ctrl_lock);
+
+ return 0;
+}
+
+static void sun4i_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+ struct sun4i_pwm_chip *sun4i_pwm = to_sun4i_pwm_chip(chip);
+ u32 val;
+
+ spin_lock(&sun4i_pwm->ctrl_lock);
+ val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
+ val &= ~BIT_CH(PWM_EN, pwm->hwpwm);
+ val &= ~BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
+ sun4i_pwm_writel(sun4i_pwm, val, PWM_CTRL_REG);
+ spin_unlock(&sun4i_pwm->ctrl_lock);
+
+ clk_disable_unprepare(sun4i_pwm->clk);
+}
+
+static const struct pwm_ops sun4i_pwm_ops = {
+ .config = sun4i_pwm_config,
+ .set_polarity = sun4i_pwm_set_polarity,
+ .enable = sun4i_pwm_enable,
+ .disable = sun4i_pwm_disable,
+ .owner = THIS_MODULE,
+};
+
+static const struct sun4i_pwm_data sun4i_pwm_data_a10 = {
+ .has_prescaler_bypass = false,
+ .has_rdy = false,
+};
+
+static const struct sun4i_pwm_data sun4i_pwm_data_a20 = {
+ .has_prescaler_bypass = true,
+ .has_rdy = true,
+};
+
+static const struct of_device_id sun4i_pwm_dt_ids[] = {
+ {
+ .compatible = "allwinner,sun4i-a10-pwm",
+ .data = &sun4i_pwm_data_a10,
+ }, {
+ .compatible = "allwinner,sun7i-a20-pwm",
+ .data = &sun4i_pwm_data_a20,
+ }, {
+ /* sentinel */
+ },
+};
+MODULE_DEVICE_TABLE(of, sun4i_pwm_dt_ids);
+
+static int sun4i_pwm_probe(struct platform_device *pdev)
+{
+ struct sun4i_pwm_chip *pwm;
+ struct resource *res;
+ u32 val;
+ int i, ret;
+ const struct of_device_id *match;
+
+ match = of_match_device(sun4i_pwm_dt_ids, &pdev->dev);
+
+ pwm = devm_kzalloc(&pdev->dev, sizeof(*pwm), GFP_KERNEL);
+ if (!pwm)
+ return -ENOMEM;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ pwm->base = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(pwm->base))
+ return PTR_ERR(pwm->base);
+
+ pwm->clk = devm_clk_get(&pdev->dev, NULL);
+ if (IS_ERR(pwm->clk))
+ return PTR_ERR(pwm->clk);
+
+ pwm->chip.dev = &pdev->dev;
+ pwm->chip.ops = &sun4i_pwm_ops;
+ pwm->chip.base = -1;
+ pwm->chip.npwm = 2;
+ pwm->chip.can_sleep = true;
+ pwm->chip.of_xlate = of_pwm_xlate_with_flags;
+ pwm->chip.of_pwm_n_cells = 3;
+ pwm->data = match->data;
+
+ spin_lock_init(&pwm->ctrl_lock);
+
+ ret = pwmchip_add(&pwm->chip);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "failed to add PWM chip: %d\n", ret);
+ return ret;
+ }
+
+ platform_set_drvdata(pdev, pwm);
+
+ ret = clk_prepare_enable(pwm->clk);
+ if (ret) {
+ dev_err(&pdev->dev, "failed to enable PWM clock\n");
+ goto clk_error;
+ }
+
+ val = sun4i_pwm_readl(pwm, PWM_CTRL_REG);
+ for (i = 0; i < pwm->chip.npwm; i++) {
+ if (!(val & BIT_CH(PWM_ACT_STATE, i)))
+ pwm->chip.pwms[i].polarity = PWM_POLARITY_INVERSED;
+ }
+ clk_disable_unprepare(pwm->clk);
+
+ return 0;
+
+clk_error:
+ pwmchip_remove(&pwm->chip);
+ return ret;
+}
+
+static int sun4i_pwm_remove(struct platform_device *pdev)
+{
+ struct sun4i_pwm_chip *pwm = platform_get_drvdata(pdev);
+
+ return pwmchip_remove(&pwm->chip);
+}
+
+static struct platform_driver sun4i_pwm_driver = {
+ .driver = {
+ .name = "sun4i-pwm",
+ .of_match_table = sun4i_pwm_dt_ids,
+ },
+ .probe = sun4i_pwm_probe,
+ .remove = sun4i_pwm_remove,
+};
+module_platform_driver(sun4i_pwm_driver);
+
+MODULE_ALIAS("platform:sun4i-pwm");
+MODULE_AUTHOR("Alexandre Belloni <[email protected]>");
+MODULE_DESCRIPTION("Allwinner sun4i PWM driver");
+MODULE_LICENSE("GPL v2");
--
2.1.0
This is the documentation for the Allwinner SoCs PWM bindings.
Signed-off-by: Alexandre Belloni <[email protected]>
Acked-by: Maxime Ripard <[email protected]>
---
Documentation/devicetree/bindings/pwm/pwm-sun4i.txt | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sun4i.txt
diff --git a/Documentation/devicetree/bindings/pwm/pwm-sun4i.txt b/Documentation/devicetree/bindings/pwm/pwm-sun4i.txt
new file mode 100644
index 000000000000..ae0273e19506
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/pwm-sun4i.txt
@@ -0,0 +1,20 @@
+Allwinner sun4i and sun7i SoC PWM controller
+
+Required properties:
+ - compatible: should be one of:
+ - "allwinner,sun4i-a10-pwm"
+ - "allwinner,sun7i-a20-pwm"
+ - reg: physical base address and length of the controller's registers
+ - #pwm-cells: should be 3. See pwm.txt in this directory for a description of
+ the cells format.
+ - clocks: From common clock binding, handle to the parent clock.
+
+Example:
+
+ pwm: pwm@01c20e00 {
+ compatible = "allwinner,sun7i-a20-pwm";
+ reg = <0x01c20e00 0xc>;
+ clocks = <&osc24M>;
+ #pwm-cells = <3>;
+ status = "disabled";
+ };
--
2.1.0
On Wed, Nov 05, 2014 at 04:15:44PM +0100, Alexandre Belloni wrote:
> This adds a generic PWM framework driver for the PWM controller
> found on Allwinner SoCs.
>
> Signed-off-by: Alexandre Belloni <[email protected]>
> Acked-by: Maxime Ripard <[email protected]>
> ---
> drivers/pwm/Kconfig | 9 ++
> drivers/pwm/Makefile | 1 +
> drivers/pwm/pwm-sun4i.c | 366 ++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 376 insertions(+)
> create mode 100644 drivers/pwm/pwm-sun4i.c
>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 3865dfb9ed08..424359d3cbb1 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -262,6 +262,15 @@ config PWM_STI
> To compile this driver as a module, choose M here: the module
> will be called pwm-sti.
>
> +config PWM_SUN4I
> + tristate "Allwinner PWM support"
> + depends on ARCH_SUNXI || COMPILE_TEST
I think you're going to need a bunch of other dependencies here, too.
HAS_IOMEM and COMMON_CLK at least, I'd expect.
> + help
> + Generic PWM framework driver for Allwinner SoCs.
> +
> + To compile this driver as a module, choose M here: the module
> + will be called pwm-sunxi.
According to the Makefile extract below it'll be called pwm-sun4i.
> +
> config PWM_TEGRA
> tristate "NVIDIA Tegra PWM support"
> depends on ARCH_TEGRA
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index c458606c3755..d607804deea1 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -24,6 +24,7 @@ obj-$(CONFIG_PWM_ROCKCHIP) += pwm-rockchip.o
> obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o
> obj-$(CONFIG_PWM_SPEAR) += pwm-spear.o
> obj-$(CONFIG_PWM_STI) += pwm-sti.o
> +obj-$(CONFIG_PWM_SUN4I) += pwm-sun4i.o
> obj-$(CONFIG_PWM_TEGRA) += pwm-tegra.o
> obj-$(CONFIG_PWM_TIECAP) += pwm-tiecap.o
> obj-$(CONFIG_PWM_TIEHRPWM) += pwm-tiehrpwm.o
> diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
[...]
> +static int sun4i_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> + int duty_ns, int period_ns)
[...]
> +{
> + struct sun4i_pwm_chip *sun4i_pwm = to_sun4i_pwm_chip(chip);
> + u32 clk_rate, prd, dty, val, clk_gate;
> + u64 div = 0;
> + unsigned int prescaler = 0;
> + int err;
> +
> + clk_rate = clk_get_rate(sun4i_pwm->clk);
> +
> + if (sun4i_pwm->data->has_prescaler_bypass) {
> + /* First, test without any prescaler when available */
> + prescaler = PWM_PRESCAL_MASK;
> + /*
> + * When not using any prescaler, the clock period in nanoseconds
> + * is not an integer so round it half up instead of
> + * truncating to get less surprising values.
> + */
> + div = clk_rate * (u64)period_ns + NSEC_PER_SEC/2;
The cast here looks odd. Perhaps a better way would be to make clk_rate
a u64 so that type promotion rules will automatically cast here for you.
> + do_div(div, NSEC_PER_SEC);
> + if (div - 1 > PWM_PRD_MASK)
> + prescaler = 0;
> + }
> +
> + if (prescaler == 0) {
> + /* Go up from the first divider */
> + for (prescaler = 0; prescaler < PWM_PRESCAL_MASK; prescaler++) {
> + if (!prescaler_table[prescaler])
> + continue;
> + div = clk_rate / prescaler_table[prescaler];
> + div = div * (u64)period_ns;
Type promotion rules should make the explicit cast unnecessary.
> + val = sun4i_pwm_readl(pwm, PWM_CTRL_REG);
> + for (i = 0; i < pwm->chip.npwm; i++) {
> + if (!(val & BIT_CH(PWM_ACT_STATE, i)))
> + pwm->chip.pwms[i].polarity = PWM_POLARITY_INVERSED;
> + }
There's no need for the braces here.
Thierry
Hey Alexandre,
On 05-11-14 16:15, Alexandre Belloni wrote:
> Hi,
>
> This patch series adds support for the PWM controller found on the Allwinner
> SoCs.
>
> The first patch adds the driver itself.
> The second patch adds the DT binding documentation
>
> Changes in v8:
> - renamed the driver sun4i as the PWM IP is different in the next sunxi SoCs
Why did you decide to rename it to sun4i? From your bindings document I
understand you still support sun4i and sun7i, what happened to sun5i?
I went over the datasheets of sun4i, sun5i and sun7i and the disp_lcd.c
from the latest linux-sunxi kernels and have to agree, they are all 4
inconsistent and messy, but I'm not sure what you mean with PWM IP is
different in next sunxi soc's. is sun5i different to sun4i? is sun7i
different? or is sun6i (A31), sun8i (a80) and sun9i (A23) different?
What I get from the datasheet is, that sun4i and sun5i are exactly the
same, with the exception that sun5i only has 1 PWM (~exposed~). I belive
that is easily solved with the bindings by having allwinner-sun4i and
allwinner sun5i bindings if I'm not mistaken.
As for sun7i compared to the other ones, according to disp_lcd.c sun5i
and sun7i should behave exactly the same. This is contradicting to the
datasheet, where sun4i and sun5i are the same.
So what are the major differences that I can see between the 3? sun4i
defines the PWM prescaler register value 0b1111 as being undefined, and
sun5i and sun7i as /1? Did you verify this (I haven't I admit, i bumped
into this while looking for your patch ;-) )? I wouldn't be supprised if
it where a typo on allwinners end in the datasheet ... disp_lcd.c stops
at 72000 for the last entry. We should just check sun4i, sun5i and sun7i
hardware to see if it behaves the same with a prescaler of 0b1111, which
I would not be totally surprised if it did.
The other difference I notice is that sun7i and sun5i use 16bit period
register where sun4i uses a 8bit register. This is probably the only
reason why they put a #ifdef in disp_lcd.c, calculations turn out
differently. I don't recognize this behavior at all in your driver
however. I do think they that there is a difference here, since they did
split up the original driver here because of this difference.
The pre-scaler bypass bit and pwm ready bit you all ready take care of :)
Finally, though I'm sure I should have replied to it inline in the
actual code, but hoping i can let it slip through here is that you
define your local data as:
+ static const struct sun4i_pwm_data sun4i_pwm_data_a20 = {
which looks really strange to me, since there is no a20 using the sun4i
architecture :) I know it's just nitpicking, it just looks really odd.
Maybe the compatible naming works just as well? sun4i_pwm_data;
sun7i_pwm_data (and sun5i_pwm_data if you want to take care of only
pwm0, only pwm1 (if we ever encounter such a configuration, disabled
pwm0, enabled pwm1) or both to be used?)
Sorry for the long naggy mail, just looking for clarification as I'm
going to be using your patch ;)
Olliver
> - Took into account comments from Thierry
>
> Changes in v9:
> - moved from using a mutex to a spinlock
>
> Alexandre Belloni (2):
> pwm: Add Allwinner SoC support
> pwm: sunxi: document OF bindings
>
> .../devicetree/bindings/pwm/pwm-sun4i.txt | 20 ++
> drivers/pwm/Kconfig | 9 +
> drivers/pwm/Makefile | 1 +
> drivers/pwm/pwm-sun4i.c | 366 +++++++++++++++++++++
> 4 files changed, 396 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sun4i.txt
> create mode 100644 drivers/pwm/pwm-sun4i.c
>
On Tue, Nov 18, 2014 at 02:47:33PM +0100, Olliver Schinagl wrote:
> Hey Alexandre,
>
> On 05-11-14 16:15, Alexandre Belloni wrote:
> >Hi,
> >
> >This patch series adds support for the PWM controller found on the Allwinner
> >SoCs.
> >
> >The first patch adds the driver itself.
> >The second patch adds the DT binding documentation
> >
> >Changes in v8:
> > - renamed the driver sun4i as the PWM IP is different in the next sunxi SoCs
> Why did you decide to rename it to sun4i? From your bindings document I
> understand you still support sun4i and sun7i, what happened to sun5i?
>
> I went over the datasheets of sun4i, sun5i and sun7i and the disp_lcd.c from
> the latest linux-sunxi kernels and have to agree, they are all 4
> inconsistent and messy, but I'm not sure what you mean with PWM IP is
> different in next sunxi soc's. is sun5i different to sun4i? is sun7i
> different? or is sun6i (A31), sun8i (a80) and sun9i (A23) different?
>
> What I get from the datasheet is, that sun4i and sun5i are exactly the same,
> with the exception that sun5i only has 1 PWM (~exposed~). I belive that is
> easily solved with the bindings by having allwinner-sun4i and allwinner
> sun5i bindings if I'm not mistaken.
>
> As for sun7i compared to the other ones, according to disp_lcd.c sun5i and
> sun7i should behave exactly the same. This is contradicting to the
> datasheet, where sun4i and sun5i are the same.
>
> So what are the major differences that I can see between the 3? sun4i
> defines the PWM prescaler register value 0b1111 as being undefined, and
> sun5i and sun7i as /1? Did you verify this (I haven't I admit, i bumped into
> this while looking for your patch ;-) )? I wouldn't be supprised if it where
> a typo on allwinners end in the datasheet ... disp_lcd.c stops at 72000 for
> the last entry. We should just check sun4i, sun5i and sun7i hardware to see
> if it behaves the same with a prescaler of 0b1111, which I would not be
> totally surprised if it did.
>
> The other difference I notice is that sun7i and sun5i use 16bit period
> register where sun4i uses a 8bit register. This is probably the only reason
> why they put a #ifdef in disp_lcd.c, calculations turn out differently. I
> don't recognize this behavior at all in your driver however. I do think they
> that there is a difference here, since they did split up the original driver
> here because of this difference.
>
> The pre-scaler bypass bit and pwm ready bit you all ready take care of :)
A31 and later are using a different IP, that is not supported by this
driver.
This driver only support the controller introduced with the A10, that
only saw minor differences between SoCs, like you have shown.
> Finally, though I'm sure I should have replied to it inline in the actual
> code, but hoping i can let it slip through here is that you define your
> local data as:
>
> + static const struct sun4i_pwm_data sun4i_pwm_data_a20 = {
>
> which looks really strange to me, since there is no a20 using the sun4i
> architecture :) I know it's just nitpicking, it just looks really odd. Maybe
> the compatible naming works just as well? sun4i_pwm_data; sun7i_pwm_data
> (and sun5i_pwm_data if you want to take care of only pwm0, only pwm1 (if we
> ever encounter such a configuration, disabled pwm0, enabled pwm1) or both to
> be used?)
This driver is name sun4i_pwm, hence the prefix.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
On 18-11-14 14:54, Maxime Ripard wrote:
> On Tue, Nov 18, 2014 at 02:47:33PM +0100, Olliver Schinagl wrote:
>> Hey Alexandre,
>>
>> On 05-11-14 16:15, Alexandre Belloni wrote:
>>> Hi,
>>>
>>> This patch series adds support for the PWM controller found on the Allwinner
>>> SoCs.
>>>
>>> The first patch adds the driver itself.
>>> The second patch adds the DT binding documentation
>>>
>>> Changes in v8:
>>> - renamed the driver sun4i as the PWM IP is different in the next sunxi SoCs
>> Why did you decide to rename it to sun4i? From your bindings document I
>> understand you still support sun4i and sun7i, what happened to sun5i?
>>
>> I went over the datasheets of sun4i, sun5i and sun7i and the disp_lcd.c from
>> the latest linux-sunxi kernels and have to agree, they are all 4
>> inconsistent and messy, but I'm not sure what you mean with PWM IP is
>> different in next sunxi soc's. is sun5i different to sun4i? is sun7i
>> different? or is sun6i (A31), sun8i (a80) and sun9i (A23) different?
>>
>> What I get from the datasheet is, that sun4i and sun5i are exactly the same,
>> with the exception that sun5i only has 1 PWM (~exposed~). I belive that is
>> easily solved with the bindings by having allwinner-sun4i and allwinner
>> sun5i bindings if I'm not mistaken.
>>
>> As for sun7i compared to the other ones, according to disp_lcd.c sun5i and
>> sun7i should behave exactly the same. This is contradicting to the
>> datasheet, where sun4i and sun5i are the same.
>>
>> So what are the major differences that I can see between the 3? sun4i
>> defines the PWM prescaler register value 0b1111 as being undefined, and
>> sun5i and sun7i as /1? Did you verify this (I haven't I admit, i bumped into
>> this while looking for your patch ;-) )? I wouldn't be supprised if it where
>> a typo on allwinners end in the datasheet ... disp_lcd.c stops at 72000 for
>> the last entry. We should just check sun4i, sun5i and sun7i hardware to see
>> if it behaves the same with a prescaler of 0b1111, which I would not be
>> totally surprised if it did.
>>
>> The other difference I notice is that sun7i and sun5i use 16bit period
>> register where sun4i uses a 8bit register. This is probably the only reason
>> why they put a #ifdef in disp_lcd.c, calculations turn out differently. I
>> don't recognize this behavior at all in your driver however. I do think they
>> that there is a difference here, since they did split up the original driver
>> here because of this difference.
>>
>> The pre-scaler bypass bit and pwm ready bit you all ready take care of :)
> A31 and later are using a different IP, that is not supported by this
> driver.
Ah, see that was not clear to me ;) Also a comment in the code would be
helpful how the 8bit vs 16bit period register is being handled with
regards to sun4i and sun7i. I can't seem to spot where this is taken
into account, since the disp_lcd.c code suggests there is supposedly
some difference? I can't tell from their code what is really going on,
since they are using unsigned 32 bit integers and crap that in the lower
and upper half of the 16bit register, so maybe the register has always
been 16bit but the docs are simply wrong? But then why differentiate
between the two chip generations ...
>
> This driver only support the controller introduced with the A10, that
> only saw minor differences between SoCs, like you have shown.
>
>> Finally, though I'm sure I should have replied to it inline in the actual
>> code, but hoping i can let it slip through here is that you define your
>> local data as:
>>
>> + static const struct sun4i_pwm_data sun4i_pwm_data_a20 = {
>>
>> which looks really strange to me, since there is no a20 using the sun4i
>> architecture :) I know it's just nitpicking, it just looks really odd. Maybe
>> the compatible naming works just as well? sun4i_pwm_data; sun7i_pwm_data
>> (and sun5i_pwm_data if you want to take care of only pwm0, only pwm1 (if we
>> ever encounter such a configuration, disabled pwm0, enabled pwm1) or both to
>> be used?)
> This driver is name sun4i_pwm, hence the prefix.
Ah right, unrelated, but I guess I should change my sunxi driver name to
sun4i as well then?
Anyway, I'll go and test this now ;)
Olliver
>
> Maxime
>
On Tue, Nov 18, 2014 at 03:11:26PM +0100, Olliver Schinagl wrote:
>
> On 18-11-14 14:54, Maxime Ripard wrote:
> >On Tue, Nov 18, 2014 at 02:47:33PM +0100, Olliver Schinagl wrote:
> >>Hey Alexandre,
> >>
> >>On 05-11-14 16:15, Alexandre Belloni wrote:
> >>>Hi,
> >>>
> >>>This patch series adds support for the PWM controller found on the Allwinner
> >>>SoCs.
> >>>
> >>>The first patch adds the driver itself.
> >>>The second patch adds the DT binding documentation
> >>>
> >>>Changes in v8:
> >>> - renamed the driver sun4i as the PWM IP is different in the next sunxi SoCs
> >>Why did you decide to rename it to sun4i? From your bindings document I
> >>understand you still support sun4i and sun7i, what happened to sun5i?
> >>
> >>I went over the datasheets of sun4i, sun5i and sun7i and the disp_lcd.c from
> >>the latest linux-sunxi kernels and have to agree, they are all 4
> >>inconsistent and messy, but I'm not sure what you mean with PWM IP is
> >>different in next sunxi soc's. is sun5i different to sun4i? is sun7i
> >>different? or is sun6i (A31), sun8i (a80) and sun9i (A23) different?
> >>
> >>What I get from the datasheet is, that sun4i and sun5i are exactly the same,
> >>with the exception that sun5i only has 1 PWM (~exposed~). I belive that is
> >>easily solved with the bindings by having allwinner-sun4i and allwinner
> >>sun5i bindings if I'm not mistaken.
> >>
> >>As for sun7i compared to the other ones, according to disp_lcd.c sun5i and
> >>sun7i should behave exactly the same. This is contradicting to the
> >>datasheet, where sun4i and sun5i are the same.
> >>
> >>So what are the major differences that I can see between the 3? sun4i
> >>defines the PWM prescaler register value 0b1111 as being undefined, and
> >>sun5i and sun7i as /1? Did you verify this (I haven't I admit, i bumped into
> >>this while looking for your patch ;-) )? I wouldn't be supprised if it where
> >>a typo on allwinners end in the datasheet ... disp_lcd.c stops at 72000 for
> >>the last entry. We should just check sun4i, sun5i and sun7i hardware to see
> >>if it behaves the same with a prescaler of 0b1111, which I would not be
> >>totally surprised if it did.
> >>
> >>The other difference I notice is that sun7i and sun5i use 16bit period
> >>register where sun4i uses a 8bit register. This is probably the only reason
> >>why they put a #ifdef in disp_lcd.c, calculations turn out differently. I
> >>don't recognize this behavior at all in your driver however. I do think they
> >>that there is a difference here, since they did split up the original driver
> >>here because of this difference.
> >>
> >>The pre-scaler bypass bit and pwm ready bit you all ready take care of :)
> >A31 and later are using a different IP, that is not supported by this
> >driver.
>
> Ah, see that was not clear to me ;) Also a comment in the code would be
> helpful how the 8bit vs 16bit period register is being handled with regards
> to sun4i and sun7i. I can't seem to spot where this is taken into account,
> since the disp_lcd.c code suggests there is supposedly some difference? I
> can't tell from their code what is really going on, since they are using
> unsigned 32 bit integers and crap that in the lower and upper half of the
> 16bit register, so maybe the register has always been 16bit but the docs are
> simply wrong? But then why differentiate between the two chip generations
> ...
Is this a bug report? Or just some theorical issues that we might
never encounter?
> >This driver only support the controller introduced with the A10, that
> >only saw minor differences between SoCs, like you have shown.
> >
> >>Finally, though I'm sure I should have replied to it inline in the actual
> >>code, but hoping i can let it slip through here is that you define your
> >>local data as:
> >>
> >>+ static const struct sun4i_pwm_data sun4i_pwm_data_a20 = {
> >>
> >>which looks really strange to me, since there is no a20 using the sun4i
> >>architecture :) I know it's just nitpicking, it just looks really odd. Maybe
> >>the compatible naming works just as well? sun4i_pwm_data; sun7i_pwm_data
> >>(and sun5i_pwm_data if you want to take care of only pwm0, only pwm1 (if we
> >>ever encounter such a configuration, disabled pwm0, enabled pwm1) or both to
> >>be used?)
> >This driver is name sun4i_pwm, hence the prefix.
>
> Ah right, unrelated, but I guess I should change my sunxi driver name to
> sun4i as well then?
What? Why?
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
On 18-11-14 15:55, Maxime Ripard wrote:
> On Tue, Nov 18, 2014 at 03:11:26PM +0100, Olliver Schinagl wrote:
>> On 18-11-14 14:54, Maxime Ripard wrote:
>>> On Tue, Nov 18, 2014 at 02:47:33PM +0100, Olliver Schinagl wrote:
>>>> Hey Alexandre,
>>>>
>>>> On 05-11-14 16:15, Alexandre Belloni wrote:
>>>>> Hi,
>>>>>
>>>>> This patch series adds support for the PWM controller found on the Allwinner
>>>>> SoCs.
>>>>>
>>>>> The first patch adds the driver itself.
>>>>> The second patch adds the DT binding documentation
>>>>>
>>>>> Changes in v8:
>>>>> - renamed the driver sun4i as the PWM IP is different in the next sunxi SoCs
>>>> Why did you decide to rename it to sun4i? From your bindings document I
>>>> understand you still support sun4i and sun7i, what happened to sun5i?
>>>>
>>>> I went over the datasheets of sun4i, sun5i and sun7i and the disp_lcd.c from
>>>> the latest linux-sunxi kernels and have to agree, they are all 4
>>>> inconsistent and messy, but I'm not sure what you mean with PWM IP is
>>>> different in next sunxi soc's. is sun5i different to sun4i? is sun7i
>>>> different? or is sun6i (A31), sun8i (a80) and sun9i (A23) different?
>>>>
>>>> What I get from the datasheet is, that sun4i and sun5i are exactly the same,
>>>> with the exception that sun5i only has 1 PWM (~exposed~). I belive that is
>>>> easily solved with the bindings by having allwinner-sun4i and allwinner
>>>> sun5i bindings if I'm not mistaken.
>>>>
>>>> As for sun7i compared to the other ones, according to disp_lcd.c sun5i and
>>>> sun7i should behave exactly the same. This is contradicting to the
>>>> datasheet, where sun4i and sun5i are the same.
>>>>
>>>> So what are the major differences that I can see between the 3? sun4i
>>>> defines the PWM prescaler register value 0b1111 as being undefined, and
>>>> sun5i and sun7i as /1? Did you verify this (I haven't I admit, i bumped into
>>>> this while looking for your patch ;-) )? I wouldn't be supprised if it where
>>>> a typo on allwinners end in the datasheet ... disp_lcd.c stops at 72000 for
>>>> the last entry. We should just check sun4i, sun5i and sun7i hardware to see
>>>> if it behaves the same with a prescaler of 0b1111, which I would not be
>>>> totally surprised if it did.
>>>>
>>>> The other difference I notice is that sun7i and sun5i use 16bit period
>>>> register where sun4i uses a 8bit register. This is probably the only reason
>>>> why they put a #ifdef in disp_lcd.c, calculations turn out differently. I
>>>> don't recognize this behavior at all in your driver however. I do think they
>>>> that there is a difference here, since they did split up the original driver
>>>> here because of this difference.
>>>>
>>>> The pre-scaler bypass bit and pwm ready bit you all ready take care of :)
>>> A31 and later are using a different IP, that is not supported by this
>>> driver.
>> Ah, see that was not clear to me ;) Also a comment in the code would be
>> helpful how the 8bit vs 16bit period register is being handled with regards
>> to sun4i and sun7i. I can't seem to spot where this is taken into account,
>> since the disp_lcd.c code suggests there is supposedly some difference? I
>> can't tell from their code what is really going on, since they are using
>> unsigned 32 bit integers and crap that in the lower and upper half of the
>> 16bit register, so maybe the register has always been 16bit but the docs are
>> simply wrong? But then why differentiate between the two chip generations
>> ...
> Is this a bug report? Or just some theorical issues that we might
> never encounter?
No, I'm asking why it is nowhere documented or not directly clear from
the code, that sun4i according to the datasheet uses 8bits for the
period register and sun5i and sun7i use 16bits. Also, the only reason
why the allwinner code differentiates between sun4i and sun5/7i is
because of this register. It contradicts what we know at this point, so
a comment indicating this would be nice.
I am currently building a kernel with these patches added for sun7i and
see if it works; i'll try to find some sun4i hardware and test it there
as well and report back then.
Olliver
>
>>> This driver only support the controller introduced with the A10, that
>>> only saw minor differences between SoCs, like you have shown.
>>>
>>>> Finally, though I'm sure I should have replied to it inline in the actual
>>>> code, but hoping i can let it slip through here is that you define your
>>>> local data as:
>>>>
>>>> + static const struct sun4i_pwm_data sun4i_pwm_data_a20 = {
>>>>
>>>> which looks really strange to me, since there is no a20 using the sun4i
>>>> architecture :) I know it's just nitpicking, it just looks really odd. Maybe
>>>> the compatible naming works just as well? sun4i_pwm_data; sun7i_pwm_data
>>>> (and sun5i_pwm_data if you want to take care of only pwm0, only pwm1 (if we
>>>> ever encounter such a configuration, disabled pwm0, enabled pwm1) or both to
>>>> be used?)
>>> This driver is name sun4i_pwm, hence the prefix.
>> Ah right, unrelated, but I guess I should change my sunxi driver name to
>> sun4i as well then?
> What? Why?
>
> Maxime
>
Hi,
I finally got some time to work on that again.
On 18/11/2014 at 14:47:33 +0100, Olliver Schinagl wrote :
> What I get from the datasheet is, that sun4i and sun5i are exactly the same,
> with the exception that sun5i only has 1 PWM (~exposed~). I belive that is
> easily solved with the bindings by having allwinner-sun4i and allwinner
> sun5i bindings if I'm not mistaken.
>
> As for sun7i compared to the other ones, according to disp_lcd.c sun5i and
> sun7i should behave exactly the same. This is contradicting to the
> datasheet, where sun4i and sun5i are the same.
>
> So what are the major differences that I can see between the 3? sun4i
> defines the PWM prescaler register value 0b1111 as being undefined, and
> sun5i and sun7i as /1? Did you verify this (I haven't I admit, i bumped into
> this while looking for your patch ;-) )? I wouldn't be supprised if it where
> a typo on allwinners end in the datasheet ... disp_lcd.c stops at 72000 for
> the last entry. We should just check sun4i, sun5i and sun7i hardware to see
> if it behaves the same with a prescaler of 0b1111, which I would not be
> totally surprised if it did.
>
> The other difference I notice is that sun7i and sun5i use 16bit period
> register where sun4i uses a 8bit register. This is probably the only reason
> why they put a #ifdef in disp_lcd.c, calculations turn out differently. I
> don't recognize this behavior at all in your driver however. I do think they
> that there is a difference here, since they did split up the original driver
> here because of this difference.
>
That is something I overlooked and I can't test at all, I only have a
cubietruck. Did you have some time to test on a sun4i?
But, from the only datasheet I have access to [1], page 56:
Each channel has a dedicated internal 16-bit up counter. If the counter
reaches the value stored in the channel period register, it resets. At
the beginning of a count period cycle, the PWMOUT is set to active state
and count from 0x0000
So I would say that they all have a 16bits period.
[1] http://dl.linux-sunxi.org/A10/A10%20Datasheet%20-%20v1.21%20%282012-04-06%29.pdf
--
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
Hey Alexandre!
On 17-12-14 20:58, Alexandre Belloni wrote:
> Hi,
>
> I finally got some time to work on that again.
awesome :D
>
> On 18/11/2014 at 14:47:33 +0100, Olliver Schinagl wrote :
>> What I get from the datasheet is, that sun4i and sun5i are exactly the same,
>> with the exception that sun5i only has 1 PWM (~exposed~). I belive that is
>> easily solved with the bindings by having allwinner-sun4i and allwinner
>> sun5i bindings if I'm not mistaken.
>>
>> As for sun7i compared to the other ones, according to disp_lcd.c sun5i and
>> sun7i should behave exactly the same. This is contradicting to the
>> datasheet, where sun4i and sun5i are the same.
>>
>> So what are the major differences that I can see between the 3? sun4i
>> defines the PWM prescaler register value 0b1111 as being undefined, and
>> sun5i and sun7i as /1? Did you verify this (I haven't I admit, i bumped into
>> this while looking for your patch ;-) )? I wouldn't be supprised if it where
>> a typo on allwinners end in the datasheet ... disp_lcd.c stops at 72000 for
>> the last entry. We should just check sun4i, sun5i and sun7i hardware to see
>> if it behaves the same with a prescaler of 0b1111, which I would not be
>> totally surprised if it did.
>>
>> The other difference I notice is that sun7i and sun5i use 16bit period
>> register where sun4i uses a 8bit register. This is probably the only reason
>> why they put a #ifdef in disp_lcd.c, calculations turn out differently. I
>> don't recognize this behavior at all in your driver however. I do think they
>> that there is a difference here, since they did split up the original driver
>> here because of this difference.
>>
> That is something I overlooked and I can't test at all, I only have a
> cubietruck. Did you have some time to test on a sun4i?
I have sun4i, sun5i and sun7i to test this on; though my sun5i is a
tablet so needs some work to setup as a dev environment (solder uart
etc). On sun7i the pwm worked perfectly; I will find some time to test
in on sun4i and sun5i in the next few weeks with your v10 patches
>
> But, from the only datasheet I have access to [1], page 56:
> Each channel has a dedicated internal 16-bit up counter. If the counter
> reaches the value stored in the channel period register, it resets. At
> the beginning of a count period cycle, the PWMOUT is set to active state
> and count from 0x0000
>
> So I would say that they all have a 16bits period.
>
>
> [1] http://dl.linux-sunxi.org/A10/A10%20Datasheet%20-%20v1.21%20%282012-04-06%29.pdf
yeah we only have the datasheets available on dl.linux-sunxi.org; but
don't only rely on the datasheets, it's a horrid copy/paste mess that
may be very wrong due to lazy editing of the pastes :(
The sorucecode helps a bit in this, as pwm0 is always used as a
background light driver, so should atleast be somewhat verified to work.
Olliver
>