Re: [U-Boot] [PATCH v2 1/3] pwm: sunxi: add support for PWM found on Allwinner A64 and H3

Hi Vasily,

On 21/09/17 07:07, Vasily Khoruzhick wrote:

This commit adds basic support for PWM found on Allwinner A64 and H3 It can be used for pwm_backlight driver (e.g. for Pinebook)

Signed-off-by: Vasily Khoruzhick anarsoul@gmail.com

v2: - move pinmux config into enable function to make driver more friendly to the boards with ethernet - drop 'sun50i-a64-pwm' compatible string and use 'sun8i-h3-pwm' instead, since A64 PWM is compatible with one on H3

arch/arm/include/asm/arch-sunxi/gpio.h | 1 + arch/arm/include/asm/arch-sunxi/pwm.h | 12 +++ drivers/pwm/Kconfig | 7 ++ drivers/pwm/Makefile | 1 + drivers/pwm/sunxi_pwm.c | 184 +++++++++++++++++++++++++++++++++ 5 files changed, 205 insertions(+) create mode 100644 drivers/pwm/sunxi_pwm.c

diff --git a/arch/arm/include/asm/arch-sunxi/gpio.h b/arch/arm/include/asm/arch-sunxi/gpio.h index 24f85206c8..7265d18099 100644 --- a/arch/arm/include/asm/arch-sunxi/gpio.h +++ b/arch/arm/include/asm/arch-sunxi/gpio.h @@ -173,6 +173,7 @@ enum sunxi_gpio_number { #define SUN8I_GPD_SDC1 3 #define SUNXI_GPD_LCD0 2 #define SUNXI_GPD_LVDS0 3 +#define SUNXI_GPD_PWM 2

#define SUN5I_GPE_SDC2 3 #define SUN8I_GPE_TWI2 3 diff --git a/arch/arm/include/asm/arch-sunxi/pwm.h b/arch/arm/include/asm/arch-sunxi/pwm.h index 5884b5dbe7..673e0eb7b5 100644 --- a/arch/arm/include/asm/arch-sunxi/pwm.h +++ b/arch/arm/include/asm/arch-sunxi/pwm.h @@ -11,8 +11,15 @@ #define SUNXI_PWM_CH0_PERIOD (SUNXI_PWM_BASE + 4)

#define SUNXI_PWM_CTRL_PRESCALE0(x) ((x) & 0xf) +#define SUNXI_PWM_CTRL_PRESCALE0_MASK (0xf)

No need for the brackets around just a number.

Actually I wonder why we would need those new constants in a header file, when they are actually internals only relevant to the driver. I see we have it here already, because we also hack something in sunxi_display.c, but that should not affect the new driver. Leave it up to you ...

#define SUNXI_PWM_CTRL_ENABLE0 (0x5 << 4) #define SUNXI_PWM_CTRL_POLARITY0(x) ((x) << 5) +#define SUNXI_PWM_CTRL_POLARITY0_MASK (1 << 5)

MASK sounds a bit confusing, what about: SUNXI_PWM_CTRL_INV_POLARITY or SUNXI_PWM_CTRL_ACTIVE_LOW or SUNXI_PWM_CH0_ACT_STA (to match the spec)

+#define SUNXI_PWM_CTRL_CLK_GATE (1 << 6)

+#define SUNXI_PWM_CH0_PERIOD_MAX (0xffff) +#define SUNXI_PWM_CH0_PERIOD_PRD(x) ((x & 0xffff) << 16) +#define SUNXI_PWM_CH0_PERIOD_DUTY(x) ((x) & 0xffff)

#define SUNXI_PWM_PERIOD_80PCT 0x04af03c0

@@ -31,4 +38,9 @@ #define SUNXI_PWM_MUX SUN8I_GPH_PWM #endif

+struct sunxi_pwm {

• u32 ctrl;
• u32 ch0_period;

+};

#endif diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index e827558052..2984b79766 100644 --- a/drivers/pwm/Kconfig +++ b/drivers/pwm/Kconfig @@ -43,3 +43,10 @@ config PWM_TEGRA four channels with a programmable period and duty cycle. Only a 32KHz clock is supported by the driver but the duty cycle is configurable.

+config PWM_SUNXI

• bool "Enable support for the Allwinner Sunxi PWM"
• depends on DM_PWM
• help

•  This PWM is found on H3, A64 and other Allwinner SoCs. It supports a
  

•  programmable period and duty cycle. A 16-bit counter is used.
  

diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile index 29d59916cb..1a8f8a58bc 100644 --- a/drivers/pwm/Makefile +++ b/drivers/pwm/Makefile @@ -17,3 +17,4 @@ obj-$(CONFIG_PWM_IMX) += pwm-imx.o pwm-imx-util.o obj-$(CONFIG_PWM_ROCKCHIP) += rk_pwm.o obj-$(CONFIG_PWM_SANDBOX) += sandbox_pwm.o obj-$(CONFIG_PWM_TEGRA) += tegra_pwm.o +obj-$(CONFIG_PWM_SUNXI) += sunxi_pwm.o diff --git a/drivers/pwm/sunxi_pwm.c b/drivers/pwm/sunxi_pwm.c new file mode 100644 index 0000000000..cfea7d69f3 --- /dev/null +++ b/drivers/pwm/sunxi_pwm.c @@ -0,0 +1,184 @@ +/*

• • Copyright (c) 2017 Vasily Khoruzhick anarsoul@gmail.com
• • SPDX-License-Identifier: GPL-2.0+
• */

+#include <common.h> +#include <div64.h> +#include <dm.h> +#include <pwm.h> +#include <regmap.h> +#include <syscon.h> +#include <asm/io.h> +#include <asm/arch/pwm.h> +#include <asm/arch/gpio.h> +#include <power/regulator.h>

+DECLARE_GLOBAL_DATA_PTR;

+struct sunxi_pwm_priv {

• struct sunxi_pwm *regs;
• ulong freq;

What do you need this "freq" for? I see it's only set once (with the constant oscillator frequency), then never changed.

• bool invert;
• uint32_t prescaler;

+};

+static const uint32_t prescaler_table[] = {

• 120, /* 0000 */
• 180, /* 0001 */
• 240, /* 0010 */
• 360, /* 0011 */
• 480, /* 0100 */
• 0, /* 0101 */
• 0, /* 0110 */
• 0, /* 0111 */
• 12000, /* 1000 */
• 24000, /* 1001 */
• 36000, /* 1010 */
• 48000, /* 1011 */
• 72000, /* 1100 */
• 0, /* 1101 */
• 0, /* 1110 */
• 1, /* 1111 */

+};

+static const uint64_t nsecs_per_sec = 1000000000L;

Any reason you made this a variable here? Also the type is wrong, it's used as a uint32_t (lldiv() divisor) below and fits well into 32-bit, so also no need for the L suffix (U would be sufficient to not blow it up to 64-bit on arm64). You might want to put this into some generic header file and let fs/ubifs/ubifs.h and drivers/i2c/stm32f7_i2c.c use it as well, if you like.

+static int sunxi_pwm_config_pinmux(void) +{ +#ifdef CONFIG_MACH_SUN50I

• sunxi_gpio_set_cfgpin(SUNXI_GPD(22), SUNXI_GPD_PWM);

+#endif

• return 0;

+}

+static int sunxi_pwm_set_invert(struct udevice *dev, uint channel, bool polarity) +{

• struct sunxi_pwm_priv *priv = dev_get_priv(dev);
• debug("%s: polarity=%u\n", __func__, polarity);
• priv->invert = polarity;
• return 0;

+}

+static int sunxi_pwm_set_config(struct udevice *dev, uint channel, uint period_ns,

• 	     uint duty_ns)
  

indentation?

+{

• struct sunxi_pwm_priv *priv = dev_get_priv(dev);
• struct sunxi_pwm *regs = priv->regs;
• int prescaler;
• u32 v, period, duty;
• uint64_t div = 0, pval = 0, scaled_freq = 0;
• debug("%s: period_ns=%u, duty_ns=%u\n", __func__, period_ns, duty_ns);
• for (prescaler = 0; prescaler < SUNXI_PWM_CTRL_PRESCALE0_MASK; prescaler++) {

• if (!prescaler_table[prescaler])
  

• 	continue;
  

• div = priv->freq;
  

• pval = prescaler_table[prescaler];
  

• scaled_freq = lldiv(div, pval);
  

This is a bit hard to read. What about: scaled_freq = lldiv(OSC_24MHZ, prescaler_table[prescaler]); Let's you get rid of pval and priv->freq as well and avoids the confusing usage of "div" here.

• div = scaled_freq * period_ns;
  

• div = lldiv(div, nsecs_per_sec);
  

Same here, div seems to be abused. period = lldiv(scaled_freq * period_ns, NSECS_PER_SEC);

That should allow you to get rid of "div" at all, I believe.

• if (div - 1 <= SUNXI_PWM_CH0_PERIOD_MAX)
  

• 	break;
  

• }
• if (div - 1 > SUNXI_PWM_CH0_PERIOD_MAX) {

• debug("%s: failed to find prescaler value\n", __func__);
  

• return -EINVAL;
  

• }
• period = div;
• div = scaled_freq * duty_ns;
• div = lldiv(div, nsecs_per_sec);
• duty = div;

Can all be shortened to: duty = lldiv(scaled_freq * duty_ns, NSECS_PER_SEC);

• if (priv->prescaler != prescaler) {

• /* Mask clock to update prescaler */
  

• v = readl(&regs->ctrl);
  

• v &= ~SUNXI_PWM_CTRL_CLK_GATE;
  

• writel(v, &regs->ctrl);
  

• v &= ~SUNXI_PWM_CTRL_PRESCALE0_MASK;
  

• v |= (priv->prescaler & SUNXI_PWM_CTRL_PRESCALE0_MASK);
  

• writel(v, &regs->ctrl);
  

• v |= SUNXI_PWM_CTRL_CLK_GATE;
  

• writel(v, &regs->ctrl);
  

• priv->prescaler = prescaler;
  

• }
• writel(SUNXI_PWM_CH0_PERIOD_PRD(period) |

•       SUNXI_PWM_CH0_PERIOD_DUTY(duty), &regs->ch0_period);
  

• debug("%s: prescaler: %d, period: %d, duty: %d\n", __func__, priv->prescaler,

•      period, duty);
  

• return 0;

+}

+static int sunxi_pwm_set_enable(struct udevice *dev, uint channel, bool enable) +{

• struct sunxi_pwm_priv *priv = dev_get_priv(dev);
• struct sunxi_pwm *regs = priv->regs;
• uint32_t v;
• debug("%s: Enable '%s'\n", __func__, dev->name);
• v = readl(&regs->ctrl);
• if (!enable) {

• v &= ~SUNXI_PWM_CTRL_ENABLE0;
  

• writel(v, &regs->ctrl);
  

• return 0;
  

• }
• sunxi_pwm_config_pinmux();
• v &= ~SUNXI_PWM_CTRL_POLARITY0_MASK;
• v |= priv->invert ? SUNXI_PWM_CTRL_POLARITY0(0) :

•       SUNXI_PWM_CTRL_POLARITY0(1);
  

I think: if (priv->invert) v |= SUNXI_PWM_CTRL_INV_POLARITY; else v &= ~SUNXI_PWM_CTRL_INV_POLARITY;

would be easier to read. The SUNXI_PWM_CTRL_POLARITY0() macro looks like there is some magic behind it, where it actually is just one bit.

Cheers, Andre.

• v |= SUNXI_PWM_CTRL_ENABLE0;
• writel(v, &regs->ctrl);
• return 0;

+}

+static int sunxi_pwm_ofdata_to_platdata(struct udevice *dev) +{

• struct sunxi_pwm_priv *priv = dev_get_priv(dev);
• priv->regs = (struct sunxi_pwm *)devfdt_get_addr(dev);
• return 0;

+}

+static int sunxi_pwm_probe(struct udevice *dev) +{

• struct sunxi_pwm_priv *priv = dev_get_priv(dev);
• priv->freq = 24000000;
• return 0;

+}

+static const struct pwm_ops sunxi_pwm_ops = {

• .set_invert = sunxi_pwm_set_invert,
• .set_config = sunxi_pwm_set_config,
• .set_enable = sunxi_pwm_set_enable,

+};

+static const struct udevice_id sunxi_pwm_ids[] = {

• { .compatible = "allwinner,sun8i-h3-pwm" },
• { }

+};

+U_BOOT_DRIVER(sunxi_pwm) = {

• .name = "sunxi_pwm",
• .id = UCLASS_PWM,
• .of_match = sunxi_pwm_ids,
• .ops = &sunxi_pwm_ops,
• .ofdata_to_platdata = sunxi_pwm_ofdata_to_platdata,
• .probe = sunxi_pwm_probe,
• .priv_auto_alloc_size = sizeof(struct sunxi_pwm_priv),

+};