From: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de> To: Hao Zhang <hao5781286@gmail.com> Cc: robh+dt@kernel.org, mark.rutland@arm.com, maxime.ripard@bootlin.com, wens@csie.org, mturquette@baylibre.com, sboyd@kernel.org, thierry.reding@gmail.com, linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-pwm@vger.kernel.org, linux-sunxi@googlegroups.com Subject: Re: [PATCH v3 6/6] ARM: PWM: add allwinner sun8i R40/T3/V40 PWM support. Date: Tue, 27 Nov 2018 11:33:54 +0100 [thread overview] Message-ID: <20181127103354.mdisx2qy42faep3s@pengutronix.de> (raw) In-Reply-To: <20181126213158.q5m5bbwnmewud2gb@pengutronix.de> Hello, On Mon, Nov 26, 2018 at 10:31:58PM +0100, Uwe Kleine-König wrote: > On Mon, Nov 26, 2018 at 12:23:19AM +0800, Hao Zhang wrote: > > The sun8i R40/T3/V40 PWM has 8 PWM channals and divides to 4 PWM pairs, > > each PWM pair built-in 1 clock module, 2 timer logic module and 1 > > programmable dead-time generator, it also support waveform capture. > > It has 2 clock sources OSC24M and APB1, it is different with the > > sun4i-pwm driver, Therefore add a new driver for it. > > > > Signed-off-by: Hao Zhang <hao5781286@gmail.com> > > --- > > drivers/pwm/Kconfig | 12 +- > > drivers/pwm/Makefile | 1 + > > drivers/pwm/pwm-sun8i.c | 418 ++++++++++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 430 insertions(+), 1 deletion(-) > > create mode 100644 drivers/pwm/pwm-sun8i.c > > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > > index 504d252..6105ac8 100644 > > --- a/drivers/pwm/Kconfig > > +++ b/drivers/pwm/Kconfig > > @@ -426,7 +426,7 @@ config PWM_STMPE > > expanders. > > > > config PWM_SUN4I > > - tristate "Allwinner PWM support" > > + tristate "Allwinner SUN4I PWM support" > > depends on ARCH_SUNXI || COMPILE_TEST > > depends on HAS_IOMEM && COMMON_CLK > > help > > @@ -435,6 +435,16 @@ config PWM_SUN4I > > To compile this driver as a module, choose M here: the module > > will be called pwm-sun4i. > > > > +config PWM_SUN8I > > + tristate "Allwinner SUN8I (R40/V40/T3) PWM support" > > + depends on ARCH_SUNXI || COMPILE_TEST > > + depends on HAS_IOMEM && COMMON_CLK > > + help > > + Generic PWM framework driver for Allwinner R40/V40/T3 SoCs. > > + > > + To compile this driver as a module, choose M here: the module > > + will be called pwm-sun8i. > > + > > config PWM_TEGRA > > tristate "NVIDIA Tegra PWM support" > > depends on ARCH_TEGRA > > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile > > index 9c676a0..32c8d2d 100644 > > --- a/drivers/pwm/Makefile > > +++ b/drivers/pwm/Makefile > > @@ -43,6 +43,7 @@ obj-$(CONFIG_PWM_STM32) += pwm-stm32.o > > obj-$(CONFIG_PWM_STM32_LP) += pwm-stm32-lp.o > > obj-$(CONFIG_PWM_STMPE) += pwm-stmpe.o > > obj-$(CONFIG_PWM_SUN4I) += pwm-sun4i.o > > +obj-$(CONFIG_PWM_SUN8I) += pwm-sun8i.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-sun8i.c b/drivers/pwm/pwm-sun8i.c > > new file mode 100644 > > index 0000000..d8597e4 > > --- /dev/null > > +++ b/drivers/pwm/pwm-sun8i.c > > @@ -0,0 +1,418 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +// Copyright (C) 2018 Hao Zhang <hao5781286@gmail.com> Given that the documentation is publically available, I suggest to add a link to it in a comment here. (http://linux-sunxi.org/File:Allwinner_R40_User_Manual_V1.0.pdf) > > + /* calculate and set prescaler, div table, PWM entire cycle */ > > + clk_div = val; > > + while (clk_div > 65535) { > > + prescaler++; > > + clk_div = val; > > + do_div(clk_div, 1U << div_id); > > + do_div(clk_div, prescaler + 1); > > + > > + if (prescaler == 255) { > > + prescaler = 0; > > + div_id++; > > + if (div_id == 9) { > > + dev_err(sun8i_pwm->chip.dev, > > + "unsupport period value\n"); > > + return -EINVAL; > > + } > > + } > > + } > > Noting the underlying formula for the calculation and the bitwidth for > the related register fields above would be good. > > > + > > + sun8i_pwm_set_value(sun8i_pwm, PWM_PERIOD_REG(ch), > > + PWM_ENTIRE_CYCLE, clk_div << 16); > > + sun8i_pwm_set_value(sun8i_pwm, PWM_CTR_REG(ch), > > + PWM_PRESCAL_K, prescaler << 0); > > + sun8i_pwm_set_value(sun8i_pwm, CLK_CFG_REG(ch), > > + CLK_DIV_M, div_id << 0); > > + > > + /* set duty cycle */ > > + val = state->period; > > + do_div(val, clk_div); > > + clk_div = state->duty_cycle; > > + do_div(clk_div, val); > > + if (clk_div > 65535) > > + clk_div = 65535; > > + > > + sun8i_pwm_set_value(sun8i_pwm, PWM_PERIOD_REG(ch), > > + PWM_ACT_CYCLE, clk_div << 0); > > Why "<< 0"? > > > + return 0; > > +} > > + > > +static int sun8i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > + struct pwm_state *state) > > +{ > > + int ret; > > + struct sun8i_pwm_chip *sun8i_pwm = to_sun8i_pwm_chip(chip); > > + struct pwm_state cstate; > > + > > + pwm_get_state(pwm, &cstate); > > + if (!cstate.enabled) { > > + ret = clk_prepare_enable(sun8i_pwm->clk); > > + if (ret) { > > + dev_err(chip->dev, "Failed to enable PWM clock\n"); > > + return ret; > > + } > > + } > > + > > + if ((cstate.period != state->period) || > > + (cstate.duty_cycle != state->duty_cycle)) { > > + ret = sun8i_pwm_config(sun8i_pwm, pwm->hwpwm, state); > > + if (ret) { > > + dev_err(chip->dev, "Failed to config PWM\n"); > > + return ret; > > + } > > + } > > sun8i_pwm_config writes the registers that are relevant for period and > duty_cycle. When do these values take effect? If it's already here, > switching the polarity below might introduce a glitch. I think this is the case after taking a look into the reference manual. There are two 16 bit fields in the PWM_PERIOD_REG. One specifies the number of clock ticks defining the period ("PWM_ENTIRE_CYCLE") and the other the duty cycle ("PWM_ACT_CYCLE"). So if you go from duty_cycle=5 + period=10 + POLARITY_NORMAL to duty_cycle=3 + period=10 + POLARITY_INVERTED this might generate: ____ __ ______ / \____/ \_________/ \__/ ^ ^ ^ ^ Also there is a PWM_PERIOD_RDY bit field that probably has to be consulted before writing to the PWM_PERIOD_REG register. It's not entirely clear to me if the PWM_ACT_STA bit that is used for inversion is shadowed until the next period, too. That's what I assumed above. If it's not the wave might look as follows: ____ __ _____ ______ / \____/ \/ \__/ \__/ ^ ^ * ^ ^ Where * marks the point where the inversion starts to take effect. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ |
WARNING: multiple messages have this Message-ID (diff)
From: u.kleine-koenig@pengutronix.de (Uwe Kleine-König) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH v3 6/6] ARM: PWM: add allwinner sun8i R40/T3/V40 PWM support. Date: Tue, 27 Nov 2018 11:33:54 +0100 [thread overview] Message-ID: <20181127103354.mdisx2qy42faep3s@pengutronix.de> (raw) In-Reply-To: <20181126213158.q5m5bbwnmewud2gb@pengutronix.de> Hello, On Mon, Nov 26, 2018 at 10:31:58PM +0100, Uwe Kleine-K?nig wrote: > On Mon, Nov 26, 2018 at 12:23:19AM +0800, Hao Zhang wrote: > > The sun8i R40/T3/V40 PWM has 8 PWM channals and divides to 4 PWM pairs, > > each PWM pair built-in 1 clock module, 2 timer logic module and 1 > > programmable dead-time generator, it also support waveform capture. > > It has 2 clock sources OSC24M and APB1, it is different with the > > sun4i-pwm driver, Therefore add a new driver for it. > > > > Signed-off-by: Hao Zhang <hao5781286@gmail.com> > > --- > > drivers/pwm/Kconfig | 12 +- > > drivers/pwm/Makefile | 1 + > > drivers/pwm/pwm-sun8i.c | 418 ++++++++++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 430 insertions(+), 1 deletion(-) > > create mode 100644 drivers/pwm/pwm-sun8i.c > > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > > index 504d252..6105ac8 100644 > > --- a/drivers/pwm/Kconfig > > +++ b/drivers/pwm/Kconfig > > @@ -426,7 +426,7 @@ config PWM_STMPE > > expanders. > > > > config PWM_SUN4I > > - tristate "Allwinner PWM support" > > + tristate "Allwinner SUN4I PWM support" > > depends on ARCH_SUNXI || COMPILE_TEST > > depends on HAS_IOMEM && COMMON_CLK > > help > > @@ -435,6 +435,16 @@ config PWM_SUN4I > > To compile this driver as a module, choose M here: the module > > will be called pwm-sun4i. > > > > +config PWM_SUN8I > > + tristate "Allwinner SUN8I (R40/V40/T3) PWM support" > > + depends on ARCH_SUNXI || COMPILE_TEST > > + depends on HAS_IOMEM && COMMON_CLK > > + help > > + Generic PWM framework driver for Allwinner R40/V40/T3 SoCs. > > + > > + To compile this driver as a module, choose M here: the module > > + will be called pwm-sun8i. > > + > > config PWM_TEGRA > > tristate "NVIDIA Tegra PWM support" > > depends on ARCH_TEGRA > > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile > > index 9c676a0..32c8d2d 100644 > > --- a/drivers/pwm/Makefile > > +++ b/drivers/pwm/Makefile > > @@ -43,6 +43,7 @@ obj-$(CONFIG_PWM_STM32) += pwm-stm32.o > > obj-$(CONFIG_PWM_STM32_LP) += pwm-stm32-lp.o > > obj-$(CONFIG_PWM_STMPE) += pwm-stmpe.o > > obj-$(CONFIG_PWM_SUN4I) += pwm-sun4i.o > > +obj-$(CONFIG_PWM_SUN8I) += pwm-sun8i.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-sun8i.c b/drivers/pwm/pwm-sun8i.c > > new file mode 100644 > > index 0000000..d8597e4 > > --- /dev/null > > +++ b/drivers/pwm/pwm-sun8i.c > > @@ -0,0 +1,418 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +// Copyright (C) 2018 Hao Zhang <hao5781286@gmail.com> Given that the documentation is publically available, I suggest to add a link to it in a comment here. (http://linux-sunxi.org/File:Allwinner_R40_User_Manual_V1.0.pdf) > > + /* calculate and set prescaler, div table, PWM entire cycle */ > > + clk_div = val; > > + while (clk_div > 65535) { > > + prescaler++; > > + clk_div = val; > > + do_div(clk_div, 1U << div_id); > > + do_div(clk_div, prescaler + 1); > > + > > + if (prescaler == 255) { > > + prescaler = 0; > > + div_id++; > > + if (div_id == 9) { > > + dev_err(sun8i_pwm->chip.dev, > > + "unsupport period value\n"); > > + return -EINVAL; > > + } > > + } > > + } > > Noting the underlying formula for the calculation and the bitwidth for > the related register fields above would be good. > > > + > > + sun8i_pwm_set_value(sun8i_pwm, PWM_PERIOD_REG(ch), > > + PWM_ENTIRE_CYCLE, clk_div << 16); > > + sun8i_pwm_set_value(sun8i_pwm, PWM_CTR_REG(ch), > > + PWM_PRESCAL_K, prescaler << 0); > > + sun8i_pwm_set_value(sun8i_pwm, CLK_CFG_REG(ch), > > + CLK_DIV_M, div_id << 0); > > + > > + /* set duty cycle */ > > + val = state->period; > > + do_div(val, clk_div); > > + clk_div = state->duty_cycle; > > + do_div(clk_div, val); > > + if (clk_div > 65535) > > + clk_div = 65535; > > + > > + sun8i_pwm_set_value(sun8i_pwm, PWM_PERIOD_REG(ch), > > + PWM_ACT_CYCLE, clk_div << 0); > > Why "<< 0"? > > > + return 0; > > +} > > + > > +static int sun8i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > + struct pwm_state *state) > > +{ > > + int ret; > > + struct sun8i_pwm_chip *sun8i_pwm = to_sun8i_pwm_chip(chip); > > + struct pwm_state cstate; > > + > > + pwm_get_state(pwm, &cstate); > > + if (!cstate.enabled) { > > + ret = clk_prepare_enable(sun8i_pwm->clk); > > + if (ret) { > > + dev_err(chip->dev, "Failed to enable PWM clock\n"); > > + return ret; > > + } > > + } > > + > > + if ((cstate.period != state->period) || > > + (cstate.duty_cycle != state->duty_cycle)) { > > + ret = sun8i_pwm_config(sun8i_pwm, pwm->hwpwm, state); > > + if (ret) { > > + dev_err(chip->dev, "Failed to config PWM\n"); > > + return ret; > > + } > > + } > > sun8i_pwm_config writes the registers that are relevant for period and > duty_cycle. When do these values take effect? If it's already here, > switching the polarity below might introduce a glitch. I think this is the case after taking a look into the reference manual. There are two 16 bit fields in the PWM_PERIOD_REG. One specifies the number of clock ticks defining the period ("PWM_ENTIRE_CYCLE") and the other the duty cycle ("PWM_ACT_CYCLE"). So if you go from duty_cycle=5 + period=10 + POLARITY_NORMAL to duty_cycle=3 + period=10 + POLARITY_INVERTED this might generate: ____ __ ______ / \____/ \_________/ \__/ ^ ^ ^ ^ Also there is a PWM_PERIOD_RDY bit field that probably has to be consulted before writing to the PWM_PERIOD_REG register. It's not entirely clear to me if the PWM_ACT_STA bit that is used for inversion is shadowed until the next period, too. That's what I assumed above. If it's not the wave might look as follows: ____ __ _____ ______ / \____/ \/ \__/ \__/ ^ ^ * ^ ^ Where * marks the point where the inversion starts to take effect. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-K?nig | Industrial Linux Solutions | http://www.pengutronix.de/ |
next prev parent reply other threads:[~2018-11-27 10:33 UTC|newest] Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-11-25 16:23 [PATCH v3 6/6] ARM: PWM: add allwinner sun8i R40/T3/V40 PWM support Hao Zhang 2018-11-25 16:23 ` Hao Zhang 2018-11-25 16:23 ` Hao Zhang 2018-11-26 21:31 ` Uwe Kleine-König 2018-11-26 21:31 ` Uwe Kleine-König 2018-11-27 10:33 ` Uwe Kleine-König [this message] 2018-11-27 10:33 ` Uwe Kleine-König 2018-12-03 9:49 ` Uwe Kleine-König 2018-12-03 9:49 ` Uwe Kleine-König 2018-11-27 8:07 ` Maxime Ripard 2018-11-27 8:07 ` Maxime Ripard 2018-11-27 8:07 ` Maxime Ripard [not found] ` <CAJeuY7-U=EF_FCe2jmBB-Otuemp_rcMPs=g7ipPi=YP4qy5mtg@mail.gmail.com> [not found] ` <CAJeuY7-U=EF_FCe2jmBB-Otuemp_rcMPs=g7ipPi=YP4qy5mtg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2018-12-03 10:38 ` Maxime Ripard 2018-12-03 10:38 ` Maxime Ripard 2018-12-03 10:38 ` Maxime Ripard 2018-12-20 17:57 ` Thierry Reding 2018-12-20 17:57 ` Thierry Reding 2018-12-20 17:57 ` Thierry Reding 2019-03-12 5:42 ` Hao Zhang 2019-03-12 5:42 ` Hao Zhang 2019-03-12 5:42 ` Hao Zhang
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20181127103354.mdisx2qy42faep3s@pengutronix.de \ --to=u.kleine-koenig@pengutronix.de \ --cc=devicetree@vger.kernel.org \ --cc=hao5781286@gmail.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-gpio@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-pwm@vger.kernel.org \ --cc=linux-sunxi@googlegroups.com \ --cc=mark.rutland@arm.com \ --cc=maxime.ripard@bootlin.com \ --cc=mturquette@baylibre.com \ --cc=robh+dt@kernel.org \ --cc=sboyd@kernel.org \ --cc=thierry.reding@gmail.com \ --cc=wens@csie.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.