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 <anars...@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 <anars...@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(®s->ctrl);
> + v &= ~SUNXI_PWM_CTRL_CLK_GATE;
> + writel(v, ®s->ctrl);
> + v &= ~SUNXI_PWM_CTRL_PRESCALE0_MASK;
> + v |= (priv->prescaler & SUNXI_PWM_CTRL_PRESCALE0_MASK);
> + writel(v, ®s->ctrl);
> + v |= SUNXI_PWM_CTRL_CLK_GATE;
> + writel(v, ®s->ctrl);
> + priv->prescaler = prescaler;
> + }
> +
> + writel(SUNXI_PWM_CH0_PERIOD_PRD(period) |
> + SUNXI_PWM_CH0_PERIOD_DUTY(duty), ®s->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(®s->ctrl);
> + if (!enable) {
> + v &= ~SUNXI_PWM_CTRL_ENABLE0;
> + writel(v, ®s->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, ®s->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),
> +};
>
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
