[{"id":1772536,"web_url":"http://patchwork.ozlabs.org/comment/1772536/","msgid":"","list_archive_url":null,"date":"2017-09-21T08:29:05","subject":"Re: [U-Boot] [PATCH v2 1/3] pwm: sunxi: add support for PWM found\n\ton Allwinner A64 and H3","submitter":{"id":61837,"url":"http://patchwork.ozlabs.org/api/people/61837/","name":"Andre Przywara","email":"andre.przywara@arm.com"},"content":"Hi Vasily,\n\nOn 21/09/17 07:07, Vasily Khoruzhick wrote:\n> This commit adds basic support for PWM found on Allwinner A64 and H3\n> It can be used for pwm_backlight driver (e.g. for Pinebook)\n> \n> Signed-off-by: Vasily Khoruzhick \n> ---\n> v2: - move pinmux config into enable function to make driver more friendly\n> to the boards with ethernet\n> - drop 'sun50i-a64-pwm' compatible string and use 'sun8i-h3-pwm' instead,\n> since A64 PWM is compatible with one on H3\n> \n> arch/arm/include/asm/arch-sunxi/gpio.h | 1 +\n> arch/arm/include/asm/arch-sunxi/pwm.h | 12 +++\n> drivers/pwm/Kconfig | 7 ++\n> drivers/pwm/Makefile | 1 +\n> drivers/pwm/sunxi_pwm.c | 184 +++++++++++++++++++++++++++++++++\n> 5 files changed, 205 insertions(+)\n> create mode 100644 drivers/pwm/sunxi_pwm.c\n> \n> diff --git a/arch/arm/include/asm/arch-sunxi/gpio.h b/arch/arm/include/asm/arch-sunxi/gpio.h\n> index 24f85206c8..7265d18099 100644\n> --- a/arch/arm/include/asm/arch-sunxi/gpio.h\n> +++ b/arch/arm/include/asm/arch-sunxi/gpio.h\n> @@ -173,6 +173,7 @@ enum sunxi_gpio_number {\n> #define SUN8I_GPD_SDC1\t\t3\n> #define SUNXI_GPD_LCD0\t\t2\n> #define SUNXI_GPD_LVDS0\t\t3\n> +#define SUNXI_GPD_PWM\t\t2\n> \n> #define SUN5I_GPE_SDC2\t\t3\n> #define SUN8I_GPE_TWI2\t\t3\n> diff --git a/arch/arm/include/asm/arch-sunxi/pwm.h b/arch/arm/include/asm/arch-sunxi/pwm.h\n> index 5884b5dbe7..673e0eb7b5 100644\n> --- a/arch/arm/include/asm/arch-sunxi/pwm.h\n> +++ b/arch/arm/include/asm/arch-sunxi/pwm.h\n> @@ -11,8 +11,15 @@\n> #define SUNXI_PWM_CH0_PERIOD\t\t(SUNXI_PWM_BASE + 4)\n> \n> #define SUNXI_PWM_CTRL_PRESCALE0(x)\t((x) & 0xf)\n> +#define SUNXI_PWM_CTRL_PRESCALE0_MASK\t(0xf)\n\nNo need for the brackets around just a number.\n\nActually I wonder why we would need those new constants in a header\nfile, when they are actually internals only relevant to the driver.\nI see we have it here already, because we also hack something in\nsunxi_display.c, but that should not affect the new driver.\nLeave it up to you ...\n\n> #define SUNXI_PWM_CTRL_ENABLE0\t\t(0x5 << 4)\n> #define SUNXI_PWM_CTRL_POLARITY0(x)\t((x) << 5)\n> +#define SUNXI_PWM_CTRL_POLARITY0_MASK\t(1 << 5)\n\nMASK sounds a bit confusing, what about:\nSUNXI_PWM_CTRL_INV_POLARITY or\nSUNXI_PWM_CTRL_ACTIVE_LOW or\nSUNXI_PWM_CH0_ACT_STA (to match the spec)\n\n> +#define SUNXI_PWM_CTRL_CLK_GATE\t\t(1 << 6)\n> +\n> +#define SUNXI_PWM_CH0_PERIOD_MAX\t(0xffff)\n> +#define SUNXI_PWM_CH0_PERIOD_PRD(x)\t((x & 0xffff) << 16)\n> +#define SUNXI_PWM_CH0_PERIOD_DUTY(x)\t((x) & 0xffff)\n> \n> #define SUNXI_PWM_PERIOD_80PCT\t\t0x04af03c0\n> \n> @@ -31,4 +38,9 @@\n> #define SUNXI_PWM_MUX\t\t\tSUN8I_GPH_PWM\n> #endif\n> \n> +struct sunxi_pwm {\n> +\tu32 ctrl;\n> +\tu32 ch0_period;\n> +};\n> +\n> #endif\n> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig\n> index e827558052..2984b79766 100644\n> --- a/drivers/pwm/Kconfig\n> +++ b/drivers/pwm/Kconfig\n> @@ -43,3 +43,10 @@ config PWM_TEGRA\n> \t four channels with a programmable period and duty cycle. Only a\n> \t 32KHz clock is supported by the driver but the duty cycle is\n> \t configurable.\n> +\n> +config PWM_SUNXI\n> +\tbool \"Enable support for the Allwinner Sunxi PWM\"\n> +\tdepends on DM_PWM\n> +\thelp\n> +\t This PWM is found on H3, A64 and other Allwinner SoCs. It supports a\n> +\t programmable period and duty cycle. A 16-bit counter is used.\n> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile\n> index 29d59916cb..1a8f8a58bc 100644\n> --- a/drivers/pwm/Makefile\n> +++ b/drivers/pwm/Makefile\n> @@ -17,3 +17,4 @@ obj-$(CONFIG_PWM_IMX)\t\t+= pwm-imx.o pwm-imx-util.o\n> obj-$(CONFIG_PWM_ROCKCHIP)\t+= rk_pwm.o\n> obj-$(CONFIG_PWM_SANDBOX)\t+= sandbox_pwm.o\n> obj-$(CONFIG_PWM_TEGRA)\t\t+= tegra_pwm.o\n> +obj-$(CONFIG_PWM_SUNXI)\t\t+= sunxi_pwm.o\n> diff --git a/drivers/pwm/sunxi_pwm.c b/drivers/pwm/sunxi_pwm.c\n> new file mode 100644\n> index 0000000000..cfea7d69f3\n> --- /dev/null\n> +++ b/drivers/pwm/sunxi_pwm.c\n> @@ -0,0 +1,184 @@\n> +/*\n> + * Copyright (c) 2017 Vasily Khoruzhick \n> + *\n> + * SPDX-License-Identifier:\tGPL-2.0+\n> + */\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +DECLARE_GLOBAL_DATA_PTR;\n> +\n> +struct sunxi_pwm_priv {\n> +\tstruct sunxi_pwm *regs;\n> +\tulong freq;\n\nWhat do you need this \"freq\" for? I see it's only set once (with the\nconstant oscillator frequency), then never changed.\n\n> +\tbool invert;\n> +\tuint32_t prescaler;\n> +};\n> +\n> +static const uint32_t prescaler_table[] = {\n> +\t120,\t/* 0000 */\n> +\t180,\t/* 0001 */\n> +\t240,\t/* 0010 */\n> +\t360,\t/* 0011 */\n> +\t480,\t/* 0100 */\n> +\t0,\t/* 0101 */\n> +\t0,\t/* 0110 */\n> +\t0,\t/* 0111 */\n> +\t12000,\t/* 1000 */\n> +\t24000,\t/* 1001 */\n> +\t36000,\t/* 1010 */\n> +\t48000,\t/* 1011 */\n> +\t72000,\t/* 1100 */\n> +\t0,\t/* 1101 */\n> +\t0,\t/* 1110 */\n> +\t1,\t/* 1111 */\n> +};\n> +\n> +static const uint64_t nsecs_per_sec = 1000000000L;\n\nAny reason you made this a variable here?\nAlso the type is wrong, it's used as a uint32_t (lldiv() divisor) below\nand fits well into 32-bit, so also no need for the L suffix (U would be\nsufficient to not blow it up to 64-bit on arm64).\nYou might want to put this into some generic header file and let\nfs/ubifs/ubifs.h and drivers/i2c/stm32f7_i2c.c use it as well, if you like.\n\n> +\n> +static int sunxi_pwm_config_pinmux(void)\n> +{\n> +#ifdef CONFIG_MACH_SUN50I\n> +\tsunxi_gpio_set_cfgpin(SUNXI_GPD(22), SUNXI_GPD_PWM);\n> +#endif\n> +\treturn 0;\n> +}\n> +\n> +static int sunxi_pwm_set_invert(struct udevice *dev, uint channel, bool polarity)\n> +{\n> +\tstruct sunxi_pwm_priv *priv = dev_get_priv(dev);\n> +\n> +\tdebug(\"%s: polarity=%u\\n\", __func__, polarity);\n> +\tpriv->invert = polarity;\n> +\n> +\treturn 0;\n> +}\n> +\n> +static int sunxi_pwm_set_config(struct udevice *dev, uint channel, uint period_ns,\n> +\t\t\t uint duty_ns)\n\nindentation?\n\n> +{\n> +\tstruct sunxi_pwm_priv *priv = dev_get_priv(dev);\n> +\tstruct sunxi_pwm *regs = priv->regs;\n> +\tint prescaler;\n> +\tu32 v, period, duty;\n> +\tuint64_t div = 0, pval = 0, scaled_freq = 0;\n> +\n> +\tdebug(\"%s: period_ns=%u, duty_ns=%u\\n\", __func__, period_ns, duty_ns);\n> +\n> +\tfor (prescaler = 0; prescaler < SUNXI_PWM_CTRL_PRESCALE0_MASK; prescaler++) {\n> +\t\tif (!prescaler_table[prescaler])\n> +\t\t\tcontinue;\n> +\t\tdiv = priv->freq;\n> +\t\tpval = prescaler_table[prescaler];\n> +\t\tscaled_freq = lldiv(div, pval);\n\nThis is a bit hard to read. What about:\n\tscaled_freq = lldiv(OSC_24MHZ, prescaler_table[prescaler]);\nLet's you get rid of pval and priv->freq as well and avoids the\nconfusing usage of \"div\" here.\n\n> +\t\tdiv = scaled_freq * period_ns;\n> +\t\tdiv = lldiv(div, nsecs_per_sec);\n\nSame here, div seems to be abused.\n\tperiod = lldiv(scaled_freq * period_ns, NSECS_PER_SEC);\n\nThat should allow you to get rid of \"div\" at all, I believe.\n\n> +\t\tif (div - 1 <= SUNXI_PWM_CH0_PERIOD_MAX)\n> +\t\t\tbreak;\n> +\t}\n> +\n> +\tif (div - 1 > SUNXI_PWM_CH0_PERIOD_MAX) {\n> +\t\tdebug(\"%s: failed to find prescaler value\\n\", __func__);\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tperiod = div;\n> +\tdiv = scaled_freq * duty_ns;\n> +\tdiv = lldiv(div, nsecs_per_sec);\n> +\tduty = div;\n\nCan all be shortened to:\n\tduty = lldiv(scaled_freq * duty_ns, NSECS_PER_SEC);\n> +\n> +\tif (priv->prescaler != prescaler) {\n> +\t\t/* Mask clock to update prescaler */\n> +\t\tv = readl(®s->ctrl);\n> +\t\tv &= ~SUNXI_PWM_CTRL_CLK_GATE;\n> +\t\twritel(v, ®s->ctrl);\n> +\t\tv &= ~SUNXI_PWM_CTRL_PRESCALE0_MASK;\n> +\t\tv |= (priv->prescaler & SUNXI_PWM_CTRL_PRESCALE0_MASK);\n> +\t\twritel(v, ®s->ctrl);\n> +\t\tv |= SUNXI_PWM_CTRL_CLK_GATE;\n> +\t\twritel(v, ®s->ctrl);\n> +\t\tpriv->prescaler = prescaler;\n> +\t}\n> +\n> +\twritel(SUNXI_PWM_CH0_PERIOD_PRD(period) |\n> +\t SUNXI_PWM_CH0_PERIOD_DUTY(duty), ®s->ch0_period);\n> +\n> +\tdebug(\"%s: prescaler: %d, period: %d, duty: %d\\n\", __func__, priv->prescaler,\n> +\t period, duty);\n> +\n> +\treturn 0;\n> +}\n> +\n> +static int sunxi_pwm_set_enable(struct udevice *dev, uint channel, bool enable)\n> +{\n> +\tstruct sunxi_pwm_priv *priv = dev_get_priv(dev);\n> +\tstruct sunxi_pwm *regs = priv->regs;\n> +\tuint32_t v;\n> +\n> +\tdebug(\"%s: Enable '%s'\\n\", __func__, dev->name);\n> +\n> +\tv = readl(®s->ctrl);\n> +\tif (!enable) {\n> +\t\tv &= ~SUNXI_PWM_CTRL_ENABLE0;\n> +\t\twritel(v, ®s->ctrl);\n> +\t\treturn 0;\n> +\t}\n> +\n> +\tsunxi_pwm_config_pinmux();\n> +\n> +\tv &= ~SUNXI_PWM_CTRL_POLARITY0_MASK;\n> +\tv |= priv->invert ? SUNXI_PWM_CTRL_POLARITY0(0) :\n> +\t\t SUNXI_PWM_CTRL_POLARITY0(1);\n\nI think:\n\tif (priv->invert)\n\t\tv |= SUNXI_PWM_CTRL_INV_POLARITY;\n\telse\n\t\tv &= ~SUNXI_PWM_CTRL_INV_POLARITY;\n\nwould be easier to read. The SUNXI_PWM_CTRL_POLARITY0() macro looks like\nthere is some magic behind it, where it actually is just one bit.\n\nCheers,\nAndre.\n\n> +\tv |= SUNXI_PWM_CTRL_ENABLE0;\n> +\twritel(v, ®s->ctrl);\n> +\n> +\treturn 0;\n> +}\n> +\n> +static int sunxi_pwm_ofdata_to_platdata(struct udevice *dev)\n> +{\n> +\tstruct sunxi_pwm_priv *priv = dev_get_priv(dev);\n> +\n> +\tpriv->regs = (struct sunxi_pwm *)devfdt_get_addr(dev);\n> +\n> +\treturn 0;\n> +}\n> +\n> +static int sunxi_pwm_probe(struct udevice *dev)\n> +{\n> +\tstruct sunxi_pwm_priv *priv = dev_get_priv(dev);\n> +\n> +\tpriv->freq = 24000000;\n> +\n> +\treturn 0;\n> +}\n> +\n> +static const struct pwm_ops sunxi_pwm_ops = {\n> +\t.set_invert\t= sunxi_pwm_set_invert,\n> +\t.set_config\t= sunxi_pwm_set_config,\n> +\t.set_enable\t= sunxi_pwm_set_enable,\n> +};\n> +\n> +static const struct udevice_id sunxi_pwm_ids[] = {\n> +\t{ .compatible = \"allwinner,sun8i-h3-pwm\" },\n> +\t{ }\n> +};\n> +\n> +U_BOOT_DRIVER(sunxi_pwm) = {\n> +\t.name\t= \"sunxi_pwm\",\n> +\t.id\t= UCLASS_PWM,\n> +\t.of_match = sunxi_pwm_ids,\n> +\t.ops\t= &sunxi_pwm_ops,\n> +\t.ofdata_to_platdata\t= sunxi_pwm_ofdata_to_platdata,\n> +\t.probe\t\t= sunxi_pwm_probe,\n> +\t.priv_auto_alloc_size\t= sizeof(struct sunxi_pwm_priv),\n> +};\n>","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=lists.denx.de\n\t(client-ip=81.169.180.215; helo=lists.denx.de;\n\tenvelope-from=u-boot-bounces@lists.denx.de;\n\treceiver=)","Received":["from lists.denx.de (dione.denx.de [81.169.180.215])\n\tby ozlabs.org (Postfix) with ESMTP id 3xyVBr65Cjz9sBZ\n\tfor ;\n\tThu, 21 Sep 2017 18:29:08 +1000 (AEST)","by lists.denx.de (Postfix, from userid 105)\n\tid 9EECEC21FA9; Thu, 21 Sep 2017 08:29:05 +0000 (UTC)","from lists.denx.de (localhost [IPv6:::1])\n\tby lists.denx.de (Postfix) with ESMTP id 38460C21E26;\n\tThu, 21 Sep 2017 08:29:01 +0000 (UTC)","by lists.denx.de (Postfix, from userid 105)\n\tid EE5B9C21E26; Thu, 21 Sep 2017 08:28:59 +0000 (UTC)","from foss.arm.com (usa-sjc-mx-foss1.foss.arm.com [217.140.101.70])\n\tby lists.denx.de (Postfix) with ESMTP id 63681C21E23\n\tfor ; Thu, 21 Sep 2017 08:28:58 +0000 (UTC)","from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249])\n\tby usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 5581F1435;\n\tThu, 21 Sep 2017 01:28:57 -0700 (PDT)","from [192.168.67.35] (usa-sjc-imap-foss1.foss.arm.com\n\t[10.72.51.249])\n\tby usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id\n\t42F663F53D; Thu, 21 Sep 2017 01:28:56 -0700 (PDT)"],"X-Spam-Checker-Version":"SpamAssassin 3.4.0 (2014-02-07) on lists.denx.de","X-Spam-Level":"","X-Spam-Status":"No, score=-5.0 required=5.0 tests=RCVD_IN_DNSWL_HI\n\tautolearn=unavailable autolearn_force=no version=3.4.0","To":"Vasily Khoruzhick , Jagan Teki , \n\tMaxime Ripard ,\n\tAnatolij Gustschin , u-boot@lists.denx.de, icenowy@aosc.io","References":"<20170921060704.19153-1-anarsoul@gmail.com>\n\t<20170921060704.19153-2-anarsoul@gmail.com>","From":"Andre Przywara ","Message-ID":"","Date":"Thu, 21 Sep 2017 09:29:05 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.3.0","MIME-Version":"1.0","In-Reply-To":"<20170921060704.19153-2-anarsoul@gmail.com>","Content-Language":"en-GB","Subject":"Re: [U-Boot] [PATCH v2 1/3] pwm: sunxi: add support for PWM found\n\ton Allwinner A64 and H3","X-BeenThere":"u-boot@lists.denx.de","X-Mailman-Version":"2.1.18","Precedence":"list","List-Id":"U-Boot discussion ","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"u-boot-bounces@lists.denx.de","Sender":"\"U-Boot\" "}}]