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

[Andre Przywara]

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 at 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 at 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),
> +};
>