From: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
To: "Александр Шубин" <privatesub2@gmail.com>
Cc: linux-kernel@vger.kernel.org,
Thierry Reding <thierry.reding@gmail.com>,
Rob Herring <robh+dt@kernel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
Conor Dooley <conor+dt@kernel.org>, Chen-Yu Tsai <wens@csie.org>,
Jernej Skrabec <jernej.skrabec@gmail.com>,
Samuel Holland <samuel@sholland.org>,
Paul Walmsley <paul.walmsley@sifive.com>,
Palmer Dabbelt <palmer@dabbelt.com>,
Albert Ou <aou@eecs.berkeley.edu>,
Philipp Zabel <p.zabel@pengutronix.de>,
Cristian Ciocaltea <cristian.ciocaltea@collabora.com>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
linux-pwm@vger.kernel.org, devicetree@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-sunxi@lists.linux.dev, linux-riscv@lists.infradead.org
Subject: Re: [PATCH v3 2/3] pwm: Add Allwinner's D1/T113-S3/R329 SoCs PWM support
Date: Fri, 4 Aug 2023 11:39:51 +0200 [thread overview]
Message-ID: <20230804093951.rsjuz5itn62ltqy5@pengutronix.de> (raw)
In-Reply-To: <CAF4idNmUn3G0=MU+PxTMNVb7u0xmQX4i5Z3zO6azUDBb1BezfQ@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 17262 bytes --]
Hello,
On Fri, Aug 04, 2023 at 10:19:26AM +0300, Александр Шубин wrote:
> пн, 10 июл. 2023 г. в 12:14, Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> > On Tue, Jun 27, 2023 at 11:23:25AM +0300, Aleksandr Shubin wrote:
> > > Allwinner's D1, T113-S3 and R329 SoCs have a quite different PWM
> > > controllers with ones supported by pwm-sun4i driver.
> > >
> > > This patch adds a PWM controller driver for Allwinner's D1,
> > > T113-S3 and R329 SoCs. The main difference between these SoCs
> > > is the number of channels defined by the DT property.
> > >
> > > Signed-off-by: Aleksandr Shubin <privatesub2@gmail.com>
> > > ---
> > > drivers/pwm/Kconfig | 10 ++
> > > drivers/pwm/Makefile | 1 +
> > > drivers/pwm/pwm-sun20i.c | 322 +++++++++++++++++++++++++++++++++++++++
> > > 3 files changed, 333 insertions(+)
> > > create mode 100644 drivers/pwm/pwm-sun20i.c
> > >
> > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > > index 8df861b1f4a3..05c48a36969e 100644
> > > --- a/drivers/pwm/Kconfig
> > > +++ b/drivers/pwm/Kconfig
> > > @@ -594,6 +594,16 @@ config PWM_SUN4I
> > > To compile this driver as a module, choose M here: the module
> > > will be called pwm-sun4i.
> > >
> > > +config PWM_SUN20I
> > > + tristate "Allwinner D1/T113s/R329 PWM support"
> > > + depends on ARCH_SUNXI || COMPILE_TEST
> > > + depends on COMMON_CLK
> > > + help
> > > + Generic PWM framework driver for Allwinner D1/T113s/R329 SoCs.
> > > +
> > > + To compile this driver as a module, choose M here: the module
> > > + will be called pwm-sun20i.
> > > +
> > > config PWM_SUNPLUS
> > > tristate "Sunplus PWM support"
> > > depends on ARCH_SUNPLUS || COMPILE_TEST
> > > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> > > index 19899b912e00..cea872e22c78 100644
> > > --- a/drivers/pwm/Makefile
> > > +++ b/drivers/pwm/Makefile
> > > @@ -55,6 +55,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_SUN20I) += pwm-sun20i.o
> > > obj-$(CONFIG_PWM_SUNPLUS) += pwm-sunplus.o
> > > obj-$(CONFIG_PWM_TEGRA) += pwm-tegra.o
> > > obj-$(CONFIG_PWM_TIECAP) += pwm-tiecap.o
> > > diff --git a/drivers/pwm/pwm-sun20i.c b/drivers/pwm/pwm-sun20i.c
> > > new file mode 100644
> > > index 000000000000..63e9c64e0e18
> > > --- /dev/null
> > > +++ b/drivers/pwm/pwm-sun20i.c
> > > @@ -0,0 +1,322 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * PWM Controller Driver for sunxi platforms (D1, T113-S3 and R329)
> > > + *
> > > + * Limitations:
> > > + * - When the parameters change, current running period will not be completed
> > > + * and run new settings immediately.
> > > + * - It output HIGH-Z state when PWM channel disabled.
> > > + *
> > > + * Copyright (c) 2023 Aleksandr Shubin <privatesub2@gmail.com>
> > > + */
> > > +
> > > +#include <linux/bitfield.h>
> > > +#include <linux/clk.h>
> > > +#include <linux/err.h>
> > > +#include <linux/io.h>
> > > +#include <linux/module.h>
> > > +#include <linux/of_device.h>
> > > +#include <linux/pwm.h>
> > > +#include <linux/reset.h>
> > > +
> > > +#define PWM_CLK_CFG_REG(chan) (0x20 + (((chan) >> 1) * 0x4))
> > > +#define PWM_CLK_SRC GENMASK(8, 7)
> > > +#define PWM_CLK_DIV_M GENMASK(3, 0)
> > > +
> > > +#define PWM_CLK_GATE_REG 0x40
> > > +#define PWM_CLK_BYPASS(chan) BIT((chan) - 16)
> > > +#define PWM_CLK_GATING(chan) BIT(chan)
> > > +
> > > +#define PWM_ENABLE_REG 0x80
> > > +#define PWM_EN(chan) BIT(chan)
> > > +
> > > +#define PWM_CTL_REG(chan) (0x100 + (chan) * 0x20)
> > > +#define PWM_ACT_STA BIT(8)
> > > +#define PWM_PRESCAL_K GENMASK(7, 0)
> > > +
> > > +#define PWM_PERIOD_REG(chan) (0x104 + (chan) * 0x20)
> > > +#define PWM_ENTIRE_CYCLE GENMASK(31, 16)
> > > +#define PWM_ACT_CYCLE GENMASK(15, 0)
> >
> > Can you please adapt the register field names to include the register
> > name? I'd use:
> >
> > #define PWM_CTL(chan) (0x100 + (chan) * 0x20)
> > #define PWM_CTL_ACT_STA BIT(8)
> > #define PWM_CTL_PRESCAL_K GENMASK(7, 0)
> >
> > then you get a chance to spot when PWM_CLK_BYPASS(x) is written to
> > PWM_CLK_CFG.
> >
> >
> > > +struct sun20i_pwm_chip {
> > > + struct pwm_chip chip;
> > > + struct clk *clk_bus, *clk_hosc;
> > > + struct reset_control *rst;
> > > + void __iomem *base;
> > > + /* Mutex to protect pwm apply state */
> > > + struct mutex mutex;
> > > +};
> > > +
> > > +static inline struct sun20i_pwm_chip *to_sun20i_pwm_chip(struct pwm_chip *chip)
> > > +{
> > > + return container_of(chip, struct sun20i_pwm_chip, chip);
> > > +}
> > > +
> > > +static inline u32 sun20i_pwm_readl(struct sun20i_pwm_chip *chip,
> > > + unsigned long offset)
> > > +{
> > > + return readl(chip->base + offset);
> > > +}
> > > +
> > > +static inline void sun20i_pwm_writel(struct sun20i_pwm_chip *chip,
> > > + u32 val, unsigned long offset)
> > > +{
> > > + writel(val, chip->base + offset);
> > > +}
> > > +
> > > +static int sun20i_pwm_get_state(struct pwm_chip *chip,
> > > + struct pwm_device *pwm,
> > > + struct pwm_state *state)
> > > +{
> > > + struct sun20i_pwm_chip *sun20i_chip = to_sun20i_pwm_chip(chip);
> > > + u64 clk_rate, tmp;
> > > + u32 val;
> > > + u16 ent_cycle, act_cycle;
> > > + u8 prescal, div_id;
> > > +
> > > + mutex_lock(&sun20i_chip->mutex);
> > > +
> > > + val = sun20i_pwm_readl(sun20i_chip, PWM_CLK_CFG_REG(pwm->hwpwm));
> > > + div_id = FIELD_GET(PWM_CLK_DIV_M, val);
> > > + if (FIELD_GET(PWM_CLK_SRC, val) == 0)
> > > + clk_rate = clk_get_rate(sun20i_chip->clk_hosc);
> > > + else
> > > + clk_rate = clk_get_rate(sun20i_chip->clk_bus);
> > > +
> > > + val = sun20i_pwm_readl(sun20i_chip, PWM_CTL_REG(pwm->hwpwm));
> > > + state->polarity = (PWM_ACT_STA & val) ? PWM_POLARITY_NORMAL : PWM_POLARITY_INVERSED;
> > > +
> > > + prescal = FIELD_GET(PWM_PRESCAL_K, val) + 1;
> >
> > If PWM_PRESCAL_K is 0xff, prescal ends up being 0. This isn't right, is
> > it?
> >
> > > + val = sun20i_pwm_readl(sun20i_chip, PWM_ENABLE_REG);
> > > + state->enabled = (PWM_EN(pwm->hwpwm) & val) ? true : false;
> > > +
> > > + val = sun20i_pwm_readl(sun20i_chip, PWM_PERIOD_REG(pwm->hwpwm));
> > > + act_cycle = FIELD_GET(PWM_ACT_CYCLE, val);
> > > + ent_cycle = FIELD_GET(PWM_ENTIRE_CYCLE, val);
> > > + if (act_cycle > ent_cycle)
> > > + act_cycle = ent_cycle;
> > > +
> >
> > A comment that with the width of the used factors this cannot overflow
> > would be nice here.
> >
> > > + tmp = (u64)(act_cycle) * prescal * (1U << div_id) * NSEC_PER_SEC;
> >
> > Can be simplified to:
> >
> > tmp = (u64)act_cycle * prescal << div_id * NSEC_PER_SEC;
> >
> > > + state->duty_cycle = DIV_ROUND_UP_ULL(tmp, clk_rate);
> > > + tmp = (u64)(ent_cycle) * prescal * (1U << div_id) * NSEC_PER_SEC;
> > > + state->period = DIV_ROUND_UP_ULL(tmp, clk_rate);
> > > + mutex_unlock(&sun20i_chip->mutex);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int sun20i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > > + const struct pwm_state *state)
> > > +{
> > > + int ret = 0;
> > > + u32 clk_gate, clk_cfg, pwm_en, ctl, period;
> > > + u64 bus_rate, hosc_rate, clk_div, val;
> > > + u16 prescaler, div_m;
> > > + bool use_bus_clk, calc_div_m;
> > > + struct sun20i_pwm_chip *sun20i_chip = to_sun20i_pwm_chip(chip);
> > > +
> > > + mutex_lock(&sun20i_chip->mutex);
> > > +
> > > + pwm_en = sun20i_pwm_readl(sun20i_chip, PWM_ENABLE_REG);
> > > +
> > > + if (state->enabled != pwm->state.enabled)
> > > + clk_gate = sun20i_pwm_readl(sun20i_chip, PWM_CLK_GATE_REG);
> > > +
> > > + if (state->enabled != pwm->state.enabled && !state->enabled) {
> > > + clk_gate &= ~PWM_CLK_GATING(pwm->hwpwm);
> > > + pwm_en &= ~PWM_EN(pwm->hwpwm);
> > > + sun20i_pwm_writel(sun20i_chip, pwm_en, PWM_ENABLE_REG);
> > > + sun20i_pwm_writel(sun20i_chip, clk_gate, PWM_CLK_GATE_REG);
> > > + }
> > > +
> > > + if (state->polarity != pwm->state.polarity ||
> > > + state->duty_cycle != pwm->state.duty_cycle ||
> > > + state->period != pwm->state.period) {
> > > + ctl = sun20i_pwm_readl(sun20i_chip, PWM_CTL_REG(pwm->hwpwm));
> > > + clk_cfg = sun20i_pwm_readl(sun20i_chip, PWM_CLK_CFG_REG(pwm->hwpwm));
> > > + hosc_rate = clk_get_rate(sun20i_chip->clk_hosc);
> > > + bus_rate = clk_get_rate(sun20i_chip->clk_bus);
> > > + if (pwm_en & PWM_EN(pwm->hwpwm ^ 1)) {
> > > + /* if the neighbor channel is enable, check period only */
> > > + use_bus_clk = FIELD_GET(PWM_CLK_SRC, clk_cfg) != 0;
> > > + if (use_bus_clk)
> > > + val = state->period * bus_rate;
> > > + else
> > > + val = state->period * hosc_rate;
> > > + do_div(val, NSEC_PER_SEC);
> > > +
> > > + div_m = FIELD_GET(PWM_CLK_DIV_M, clk_cfg);
> > > + calc_div_m = false;
> > > + } else {
> > > + /* check period and select clock source */
> > > + use_bus_clk = false;
> > > + val = state->period * hosc_rate;
> > > + do_div(val, NSEC_PER_SEC);
> > > + if (val <= 1) {
> > > + use_bus_clk = true;
> > > + val = state->period * bus_rate;
> > > + do_div(val, NSEC_PER_SEC);
> > > + if (val <= 1) {
> > > + ret = -EINVAL;
> > > + goto unlock_mutex;
> > > + }
> > > + }
> > > + div_m = 0;
> > > + calc_div_m = true;
> > > +
> > > + /* set up the CLK_DIV_M and clock CLK_SRC */
> > > + clk_cfg = FIELD_PREP(PWM_CLK_DIV_M, div_m);
> > > + clk_cfg |= FIELD_PREP(PWM_CLK_SRC, use_bus_clk ? 1 : 0);
> > > +
> > > + sun20i_pwm_writel(sun20i_chip, clk_cfg, PWM_CLK_CFG_REG(pwm->hwpwm));
> > > + }
> > > +
> > > + /* calculate prescaler, M factor, PWM entire cycle */
> > > + clk_div = val;
> >
> > This assignment is useless as it is overwritten in the loop below, isn't
> > it?
> >
> > > + for (prescaler = 0;; prescaler++) {
> > > + if (prescaler >= 256) {
> > > + if (calc_div_m) {
> > > + prescaler = 0;
> > > + div_m++;
> > > + if (div_m >= 9) {
> > > + ret = -EINVAL;
> > > + goto unlock_mutex;
> > > + }
> > > + } else {
> > > + ret = -EINVAL;
> > > + goto unlock_mutex;
> > > + }
> > > + }
> > > +
> > > + clk_div = val >> div_m;
> > > + do_div(clk_div, prescaler + 1);
> > > + if (clk_div <= 65534)
> > > + break;
> >
> > This can be calculated without a loop.
>
> Point me please where can I see the calculation of two divisors without a loop?
for a given value of div_m you search the smallest prescaler such that
(val >> div_m) // (prescaler + 1) ≤ 65534
(Using Python syntax where // denotes the usual
round-down-to-next-integer division and / normal exact division.)
This is equivalent to:
(val >> div_m) // (prescaler + 1) ≤ 65534
⟺ (val >> div_m) ≤ 65534 * (prescaler + 1) + prescaler
⟺ (val >> div_m) ≤ 65535 * prescaler + 65534
⟺ (val >> div_m) - 65534 ≤ 65535 * prescaler
⟺ ((val >> div_m) - 65534) / 65535 ≤ prescaler
And as prescaler is integer, this is
...
⟺ ((val >> div_m) - 65534) // 65535 ≤ prescaler
So the prescaler value you're looking for is:
((val >> div_m) - 65534) // 65535
And then you have to pick the smallest div_m such that prescaler ≤ 255:
((val >> div_m) - 65534) // 65535 ≤ 255
⟺ (val >> div_m) - 65534 ≤ 255 * 65535 + 65534
⟺ val >> div_m ≤ 255 * 65535 + 2 * 65534
⟺ val >> div_m ≤ 16842493
⟺ val >> div_m < 16842494
so div_m is fls((val) / 16842494).
You might want to double check this and explain the algorithm in a
comment similar to the above calculation.
> > > + }
> > > +
> > > + period = FIELD_PREP(PWM_ENTIRE_CYCLE, clk_div);
> > > +
> > > + /* set duty cycle */
> > > + if (use_bus_clk)
> > > + val = state->duty_cycle * bus_rate;
> > > + else
> > > + val = state->duty_cycle * hosc_rate;
maybe better use:
if (use_bus_clk)
rate = bus_rate;
else
rate = hosc_rate;
val = state->duty_cycle * rate
> > > + do_div(val, NSEC_PER_SEC);
> > > + clk_div = val >> div_m;
> > > + do_div(clk_div, prescaler + 1);
> > > +
> > > + if (state->duty_cycle == state->period)
> > > + clk_div++;
> >
> > I don't understand that one. Can you explain that in a comment please?
>
> The formula of the output period and the duty-cycle for PWM are as follows.
> T period = (PWM01_CLK / PWM0_PRESCALE_K)^-1 * (PPR0.PWM_ENTIRE_CYCLE + 1)
That can be simpler written as:
Tperiod = PWM0_PRESCALE_K / PWM01_CLK * (PPR0.PWM_ENTIRE_CYCLE + 1)
right?
> T high-level = (PWM01_CLK / PWM0_PRESCALE_K)^-1 * PPR0.PWM_ACT_CYCLE
simlar:
Thigh = PWM0_PRESCALE_K / PWM01_CLK * PPR0.PWM_ACT_CYCLE
> Duty-cycle = T high-level / T period
> In accordance with this formula, in order to set the duty-cycle to 100%,
> it is necessary that PWM_ACT_CYCLE >= PWM_ENTIRE_CYCLE + 1
The +1 is also relevant for duty_cycles other than state->period, right?
> > > + period |= FIELD_PREP(PWM_ACT_CYCLE, clk_div);
> > > + sun20i_pwm_writel(sun20i_chip, period, PWM_PERIOD_REG(pwm->hwpwm));
> > > +
> > > + ctl = FIELD_PREP(PWM_PRESCAL_K, prescaler);
> > > + if (state->polarity == PWM_POLARITY_NORMAL)
> > > + ctl |= PWM_ACT_STA;
> > > +
> > > + sun20i_pwm_writel(sun20i_chip, ctl, PWM_CTL_REG(pwm->hwpwm));
> >
> > Is this racy? I.e. does the write to PWM_PERIOD_REG(pwm->hwpwm) above
> > already has an effect before PWM_CTL_REG(pwm->hwpwm) is written?
> >
> > > + }
> > > +
> > > + if (state->enabled != pwm->state.enabled && state->enabled) {
> > > + clk_gate &= ~PWM_CLK_BYPASS(pwm->hwpwm);
> > > + clk_gate |= PWM_CLK_GATING(pwm->hwpwm);
> > > + pwm_en |= PWM_EN(pwm->hwpwm);
> > > + sun20i_pwm_writel(sun20i_chip, pwm_en, PWM_ENABLE_REG);
> > > + sun20i_pwm_writel(sun20i_chip, clk_gate, PWM_CLK_GATE_REG);
> >
> > This is (I guess) racy. If your PWM is running with
> >
> > .period = 10000
> > .duty_cyle = 0
> > .enabled = true
> >
> > and you configure it to
> >
> > .period = 10000
> > .duty_cyle = 10000
> > .enabled = false
> >
> > you get a short spike. For a enabled=true -> enabled=false transition
> > you should disable first before configuring duty+period (or skip the
> > latter completely).
>
> When switching enabled=true -> enabled=false then before setting the period,
> the enable register will be written false and the pvm will become inactive.
> this is the place:
> if (state->enabled != pwm->state.enabled && !state->enabled) {
Ah, it seems I missed that (or I just don't understand any more what
meant back then :-)
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
WARNING: multiple messages have this Message-ID (diff)
From: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
To: "Александр Шубин" <privatesub2@gmail.com>
Cc: linux-arm-kernel@lists.infradead.org, linux-pwm@vger.kernel.org,
Conor Dooley <conor+dt@kernel.org>,
Albert Ou <aou@eecs.berkeley.edu>,
Philipp Zabel <p.zabel@pengutronix.de>,
Samuel Holland <samuel@sholland.org>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
linux-kernel@vger.kernel.org, Rob Herring <robh+dt@kernel.org>,
Chen-Yu Tsai <wens@csie.org>,
Thierry Reding <thierry.reding@gmail.com>,
Palmer Dabbelt <palmer@dabbelt.com>,
Jernej Skrabec <jernej.skrabec@gmail.com>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
Paul Walmsley <paul.walmsley@sifive.com>,
linux-riscv@lists.infradead.org, linux-sunxi@lists.linux.dev,
devicetree@vger.kernel.org
Subject: Re: [PATCH v3 2/3] pwm: Add Allwinner's D1/T113-S3/R329 SoCs PWM support
Date: Fri, 4 Aug 2023 11:39:51 +0200 [thread overview]
Message-ID: <20230804093951.rsjuz5itn62ltqy5@pengutronix.de> (raw)
In-Reply-To: <CAF4idNmUn3G0=MU+PxTMNVb7u0xmQX4i5Z3zO6azUDBb1BezfQ@mail.gmail.com>
[-- Attachment #1.1: Type: text/plain, Size: 17262 bytes --]
Hello,
On Fri, Aug 04, 2023 at 10:19:26AM +0300, Александр Шубин wrote:
> пн, 10 июл. 2023 г. в 12:14, Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> > On Tue, Jun 27, 2023 at 11:23:25AM +0300, Aleksandr Shubin wrote:
> > > Allwinner's D1, T113-S3 and R329 SoCs have a quite different PWM
> > > controllers with ones supported by pwm-sun4i driver.
> > >
> > > This patch adds a PWM controller driver for Allwinner's D1,
> > > T113-S3 and R329 SoCs. The main difference between these SoCs
> > > is the number of channels defined by the DT property.
> > >
> > > Signed-off-by: Aleksandr Shubin <privatesub2@gmail.com>
> > > ---
> > > drivers/pwm/Kconfig | 10 ++
> > > drivers/pwm/Makefile | 1 +
> > > drivers/pwm/pwm-sun20i.c | 322 +++++++++++++++++++++++++++++++++++++++
> > > 3 files changed, 333 insertions(+)
> > > create mode 100644 drivers/pwm/pwm-sun20i.c
> > >
> > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > > index 8df861b1f4a3..05c48a36969e 100644
> > > --- a/drivers/pwm/Kconfig
> > > +++ b/drivers/pwm/Kconfig
> > > @@ -594,6 +594,16 @@ config PWM_SUN4I
> > > To compile this driver as a module, choose M here: the module
> > > will be called pwm-sun4i.
> > >
> > > +config PWM_SUN20I
> > > + tristate "Allwinner D1/T113s/R329 PWM support"
> > > + depends on ARCH_SUNXI || COMPILE_TEST
> > > + depends on COMMON_CLK
> > > + help
> > > + Generic PWM framework driver for Allwinner D1/T113s/R329 SoCs.
> > > +
> > > + To compile this driver as a module, choose M here: the module
> > > + will be called pwm-sun20i.
> > > +
> > > config PWM_SUNPLUS
> > > tristate "Sunplus PWM support"
> > > depends on ARCH_SUNPLUS || COMPILE_TEST
> > > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> > > index 19899b912e00..cea872e22c78 100644
> > > --- a/drivers/pwm/Makefile
> > > +++ b/drivers/pwm/Makefile
> > > @@ -55,6 +55,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_SUN20I) += pwm-sun20i.o
> > > obj-$(CONFIG_PWM_SUNPLUS) += pwm-sunplus.o
> > > obj-$(CONFIG_PWM_TEGRA) += pwm-tegra.o
> > > obj-$(CONFIG_PWM_TIECAP) += pwm-tiecap.o
> > > diff --git a/drivers/pwm/pwm-sun20i.c b/drivers/pwm/pwm-sun20i.c
> > > new file mode 100644
> > > index 000000000000..63e9c64e0e18
> > > --- /dev/null
> > > +++ b/drivers/pwm/pwm-sun20i.c
> > > @@ -0,0 +1,322 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * PWM Controller Driver for sunxi platforms (D1, T113-S3 and R329)
> > > + *
> > > + * Limitations:
> > > + * - When the parameters change, current running period will not be completed
> > > + * and run new settings immediately.
> > > + * - It output HIGH-Z state when PWM channel disabled.
> > > + *
> > > + * Copyright (c) 2023 Aleksandr Shubin <privatesub2@gmail.com>
> > > + */
> > > +
> > > +#include <linux/bitfield.h>
> > > +#include <linux/clk.h>
> > > +#include <linux/err.h>
> > > +#include <linux/io.h>
> > > +#include <linux/module.h>
> > > +#include <linux/of_device.h>
> > > +#include <linux/pwm.h>
> > > +#include <linux/reset.h>
> > > +
> > > +#define PWM_CLK_CFG_REG(chan) (0x20 + (((chan) >> 1) * 0x4))
> > > +#define PWM_CLK_SRC GENMASK(8, 7)
> > > +#define PWM_CLK_DIV_M GENMASK(3, 0)
> > > +
> > > +#define PWM_CLK_GATE_REG 0x40
> > > +#define PWM_CLK_BYPASS(chan) BIT((chan) - 16)
> > > +#define PWM_CLK_GATING(chan) BIT(chan)
> > > +
> > > +#define PWM_ENABLE_REG 0x80
> > > +#define PWM_EN(chan) BIT(chan)
> > > +
> > > +#define PWM_CTL_REG(chan) (0x100 + (chan) * 0x20)
> > > +#define PWM_ACT_STA BIT(8)
> > > +#define PWM_PRESCAL_K GENMASK(7, 0)
> > > +
> > > +#define PWM_PERIOD_REG(chan) (0x104 + (chan) * 0x20)
> > > +#define PWM_ENTIRE_CYCLE GENMASK(31, 16)
> > > +#define PWM_ACT_CYCLE GENMASK(15, 0)
> >
> > Can you please adapt the register field names to include the register
> > name? I'd use:
> >
> > #define PWM_CTL(chan) (0x100 + (chan) * 0x20)
> > #define PWM_CTL_ACT_STA BIT(8)
> > #define PWM_CTL_PRESCAL_K GENMASK(7, 0)
> >
> > then you get a chance to spot when PWM_CLK_BYPASS(x) is written to
> > PWM_CLK_CFG.
> >
> >
> > > +struct sun20i_pwm_chip {
> > > + struct pwm_chip chip;
> > > + struct clk *clk_bus, *clk_hosc;
> > > + struct reset_control *rst;
> > > + void __iomem *base;
> > > + /* Mutex to protect pwm apply state */
> > > + struct mutex mutex;
> > > +};
> > > +
> > > +static inline struct sun20i_pwm_chip *to_sun20i_pwm_chip(struct pwm_chip *chip)
> > > +{
> > > + return container_of(chip, struct sun20i_pwm_chip, chip);
> > > +}
> > > +
> > > +static inline u32 sun20i_pwm_readl(struct sun20i_pwm_chip *chip,
> > > + unsigned long offset)
> > > +{
> > > + return readl(chip->base + offset);
> > > +}
> > > +
> > > +static inline void sun20i_pwm_writel(struct sun20i_pwm_chip *chip,
> > > + u32 val, unsigned long offset)
> > > +{
> > > + writel(val, chip->base + offset);
> > > +}
> > > +
> > > +static int sun20i_pwm_get_state(struct pwm_chip *chip,
> > > + struct pwm_device *pwm,
> > > + struct pwm_state *state)
> > > +{
> > > + struct sun20i_pwm_chip *sun20i_chip = to_sun20i_pwm_chip(chip);
> > > + u64 clk_rate, tmp;
> > > + u32 val;
> > > + u16 ent_cycle, act_cycle;
> > > + u8 prescal, div_id;
> > > +
> > > + mutex_lock(&sun20i_chip->mutex);
> > > +
> > > + val = sun20i_pwm_readl(sun20i_chip, PWM_CLK_CFG_REG(pwm->hwpwm));
> > > + div_id = FIELD_GET(PWM_CLK_DIV_M, val);
> > > + if (FIELD_GET(PWM_CLK_SRC, val) == 0)
> > > + clk_rate = clk_get_rate(sun20i_chip->clk_hosc);
> > > + else
> > > + clk_rate = clk_get_rate(sun20i_chip->clk_bus);
> > > +
> > > + val = sun20i_pwm_readl(sun20i_chip, PWM_CTL_REG(pwm->hwpwm));
> > > + state->polarity = (PWM_ACT_STA & val) ? PWM_POLARITY_NORMAL : PWM_POLARITY_INVERSED;
> > > +
> > > + prescal = FIELD_GET(PWM_PRESCAL_K, val) + 1;
> >
> > If PWM_PRESCAL_K is 0xff, prescal ends up being 0. This isn't right, is
> > it?
> >
> > > + val = sun20i_pwm_readl(sun20i_chip, PWM_ENABLE_REG);
> > > + state->enabled = (PWM_EN(pwm->hwpwm) & val) ? true : false;
> > > +
> > > + val = sun20i_pwm_readl(sun20i_chip, PWM_PERIOD_REG(pwm->hwpwm));
> > > + act_cycle = FIELD_GET(PWM_ACT_CYCLE, val);
> > > + ent_cycle = FIELD_GET(PWM_ENTIRE_CYCLE, val);
> > > + if (act_cycle > ent_cycle)
> > > + act_cycle = ent_cycle;
> > > +
> >
> > A comment that with the width of the used factors this cannot overflow
> > would be nice here.
> >
> > > + tmp = (u64)(act_cycle) * prescal * (1U << div_id) * NSEC_PER_SEC;
> >
> > Can be simplified to:
> >
> > tmp = (u64)act_cycle * prescal << div_id * NSEC_PER_SEC;
> >
> > > + state->duty_cycle = DIV_ROUND_UP_ULL(tmp, clk_rate);
> > > + tmp = (u64)(ent_cycle) * prescal * (1U << div_id) * NSEC_PER_SEC;
> > > + state->period = DIV_ROUND_UP_ULL(tmp, clk_rate);
> > > + mutex_unlock(&sun20i_chip->mutex);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int sun20i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > > + const struct pwm_state *state)
> > > +{
> > > + int ret = 0;
> > > + u32 clk_gate, clk_cfg, pwm_en, ctl, period;
> > > + u64 bus_rate, hosc_rate, clk_div, val;
> > > + u16 prescaler, div_m;
> > > + bool use_bus_clk, calc_div_m;
> > > + struct sun20i_pwm_chip *sun20i_chip = to_sun20i_pwm_chip(chip);
> > > +
> > > + mutex_lock(&sun20i_chip->mutex);
> > > +
> > > + pwm_en = sun20i_pwm_readl(sun20i_chip, PWM_ENABLE_REG);
> > > +
> > > + if (state->enabled != pwm->state.enabled)
> > > + clk_gate = sun20i_pwm_readl(sun20i_chip, PWM_CLK_GATE_REG);
> > > +
> > > + if (state->enabled != pwm->state.enabled && !state->enabled) {
> > > + clk_gate &= ~PWM_CLK_GATING(pwm->hwpwm);
> > > + pwm_en &= ~PWM_EN(pwm->hwpwm);
> > > + sun20i_pwm_writel(sun20i_chip, pwm_en, PWM_ENABLE_REG);
> > > + sun20i_pwm_writel(sun20i_chip, clk_gate, PWM_CLK_GATE_REG);
> > > + }
> > > +
> > > + if (state->polarity != pwm->state.polarity ||
> > > + state->duty_cycle != pwm->state.duty_cycle ||
> > > + state->period != pwm->state.period) {
> > > + ctl = sun20i_pwm_readl(sun20i_chip, PWM_CTL_REG(pwm->hwpwm));
> > > + clk_cfg = sun20i_pwm_readl(sun20i_chip, PWM_CLK_CFG_REG(pwm->hwpwm));
> > > + hosc_rate = clk_get_rate(sun20i_chip->clk_hosc);
> > > + bus_rate = clk_get_rate(sun20i_chip->clk_bus);
> > > + if (pwm_en & PWM_EN(pwm->hwpwm ^ 1)) {
> > > + /* if the neighbor channel is enable, check period only */
> > > + use_bus_clk = FIELD_GET(PWM_CLK_SRC, clk_cfg) != 0;
> > > + if (use_bus_clk)
> > > + val = state->period * bus_rate;
> > > + else
> > > + val = state->period * hosc_rate;
> > > + do_div(val, NSEC_PER_SEC);
> > > +
> > > + div_m = FIELD_GET(PWM_CLK_DIV_M, clk_cfg);
> > > + calc_div_m = false;
> > > + } else {
> > > + /* check period and select clock source */
> > > + use_bus_clk = false;
> > > + val = state->period * hosc_rate;
> > > + do_div(val, NSEC_PER_SEC);
> > > + if (val <= 1) {
> > > + use_bus_clk = true;
> > > + val = state->period * bus_rate;
> > > + do_div(val, NSEC_PER_SEC);
> > > + if (val <= 1) {
> > > + ret = -EINVAL;
> > > + goto unlock_mutex;
> > > + }
> > > + }
> > > + div_m = 0;
> > > + calc_div_m = true;
> > > +
> > > + /* set up the CLK_DIV_M and clock CLK_SRC */
> > > + clk_cfg = FIELD_PREP(PWM_CLK_DIV_M, div_m);
> > > + clk_cfg |= FIELD_PREP(PWM_CLK_SRC, use_bus_clk ? 1 : 0);
> > > +
> > > + sun20i_pwm_writel(sun20i_chip, clk_cfg, PWM_CLK_CFG_REG(pwm->hwpwm));
> > > + }
> > > +
> > > + /* calculate prescaler, M factor, PWM entire cycle */
> > > + clk_div = val;
> >
> > This assignment is useless as it is overwritten in the loop below, isn't
> > it?
> >
> > > + for (prescaler = 0;; prescaler++) {
> > > + if (prescaler >= 256) {
> > > + if (calc_div_m) {
> > > + prescaler = 0;
> > > + div_m++;
> > > + if (div_m >= 9) {
> > > + ret = -EINVAL;
> > > + goto unlock_mutex;
> > > + }
> > > + } else {
> > > + ret = -EINVAL;
> > > + goto unlock_mutex;
> > > + }
> > > + }
> > > +
> > > + clk_div = val >> div_m;
> > > + do_div(clk_div, prescaler + 1);
> > > + if (clk_div <= 65534)
> > > + break;
> >
> > This can be calculated without a loop.
>
> Point me please where can I see the calculation of two divisors without a loop?
for a given value of div_m you search the smallest prescaler such that
(val >> div_m) // (prescaler + 1) ≤ 65534
(Using Python syntax where // denotes the usual
round-down-to-next-integer division and / normal exact division.)
This is equivalent to:
(val >> div_m) // (prescaler + 1) ≤ 65534
⟺ (val >> div_m) ≤ 65534 * (prescaler + 1) + prescaler
⟺ (val >> div_m) ≤ 65535 * prescaler + 65534
⟺ (val >> div_m) - 65534 ≤ 65535 * prescaler
⟺ ((val >> div_m) - 65534) / 65535 ≤ prescaler
And as prescaler is integer, this is
...
⟺ ((val >> div_m) - 65534) // 65535 ≤ prescaler
So the prescaler value you're looking for is:
((val >> div_m) - 65534) // 65535
And then you have to pick the smallest div_m such that prescaler ≤ 255:
((val >> div_m) - 65534) // 65535 ≤ 255
⟺ (val >> div_m) - 65534 ≤ 255 * 65535 + 65534
⟺ val >> div_m ≤ 255 * 65535 + 2 * 65534
⟺ val >> div_m ≤ 16842493
⟺ val >> div_m < 16842494
so div_m is fls((val) / 16842494).
You might want to double check this and explain the algorithm in a
comment similar to the above calculation.
> > > + }
> > > +
> > > + period = FIELD_PREP(PWM_ENTIRE_CYCLE, clk_div);
> > > +
> > > + /* set duty cycle */
> > > + if (use_bus_clk)
> > > + val = state->duty_cycle * bus_rate;
> > > + else
> > > + val = state->duty_cycle * hosc_rate;
maybe better use:
if (use_bus_clk)
rate = bus_rate;
else
rate = hosc_rate;
val = state->duty_cycle * rate
> > > + do_div(val, NSEC_PER_SEC);
> > > + clk_div = val >> div_m;
> > > + do_div(clk_div, prescaler + 1);
> > > +
> > > + if (state->duty_cycle == state->period)
> > > + clk_div++;
> >
> > I don't understand that one. Can you explain that in a comment please?
>
> The formula of the output period and the duty-cycle for PWM are as follows.
> T period = (PWM01_CLK / PWM0_PRESCALE_K)^-1 * (PPR0.PWM_ENTIRE_CYCLE + 1)
That can be simpler written as:
Tperiod = PWM0_PRESCALE_K / PWM01_CLK * (PPR0.PWM_ENTIRE_CYCLE + 1)
right?
> T high-level = (PWM01_CLK / PWM0_PRESCALE_K)^-1 * PPR0.PWM_ACT_CYCLE
simlar:
Thigh = PWM0_PRESCALE_K / PWM01_CLK * PPR0.PWM_ACT_CYCLE
> Duty-cycle = T high-level / T period
> In accordance with this formula, in order to set the duty-cycle to 100%,
> it is necessary that PWM_ACT_CYCLE >= PWM_ENTIRE_CYCLE + 1
The +1 is also relevant for duty_cycles other than state->period, right?
> > > + period |= FIELD_PREP(PWM_ACT_CYCLE, clk_div);
> > > + sun20i_pwm_writel(sun20i_chip, period, PWM_PERIOD_REG(pwm->hwpwm));
> > > +
> > > + ctl = FIELD_PREP(PWM_PRESCAL_K, prescaler);
> > > + if (state->polarity == PWM_POLARITY_NORMAL)
> > > + ctl |= PWM_ACT_STA;
> > > +
> > > + sun20i_pwm_writel(sun20i_chip, ctl, PWM_CTL_REG(pwm->hwpwm));
> >
> > Is this racy? I.e. does the write to PWM_PERIOD_REG(pwm->hwpwm) above
> > already has an effect before PWM_CTL_REG(pwm->hwpwm) is written?
> >
> > > + }
> > > +
> > > + if (state->enabled != pwm->state.enabled && state->enabled) {
> > > + clk_gate &= ~PWM_CLK_BYPASS(pwm->hwpwm);
> > > + clk_gate |= PWM_CLK_GATING(pwm->hwpwm);
> > > + pwm_en |= PWM_EN(pwm->hwpwm);
> > > + sun20i_pwm_writel(sun20i_chip, pwm_en, PWM_ENABLE_REG);
> > > + sun20i_pwm_writel(sun20i_chip, clk_gate, PWM_CLK_GATE_REG);
> >
> > This is (I guess) racy. If your PWM is running with
> >
> > .period = 10000
> > .duty_cyle = 0
> > .enabled = true
> >
> > and you configure it to
> >
> > .period = 10000
> > .duty_cyle = 10000
> > .enabled = false
> >
> > you get a short spike. For a enabled=true -> enabled=false transition
> > you should disable first before configuring duty+period (or skip the
> > latter completely).
>
> When switching enabled=true -> enabled=false then before setting the period,
> the enable register will be written false and the pvm will become inactive.
> this is the place:
> if (state->enabled != pwm->state.enabled && !state->enabled) {
Ah, it seems I missed that (or I just don't understand any more what
meant back then :-)
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 161 bytes --]
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
WARNING: multiple messages have this Message-ID (diff)
From: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
To: "Александр Шубин" <privatesub2@gmail.com>
Cc: linux-kernel@vger.kernel.org,
Thierry Reding <thierry.reding@gmail.com>,
Rob Herring <robh+dt@kernel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
Conor Dooley <conor+dt@kernel.org>, Chen-Yu Tsai <wens@csie.org>,
Jernej Skrabec <jernej.skrabec@gmail.com>,
Samuel Holland <samuel@sholland.org>,
Paul Walmsley <paul.walmsley@sifive.com>,
Palmer Dabbelt <palmer@dabbelt.com>,
Albert Ou <aou@eecs.berkeley.edu>,
Philipp Zabel <p.zabel@pengutronix.de>,
Cristian Ciocaltea <cristian.ciocaltea@collabora.com>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
linux-pwm@vger.kernel.org, devicetree@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-sunxi@lists.linux.dev, linux-riscv@lists.infradead.org
Subject: Re: [PATCH v3 2/3] pwm: Add Allwinner's D1/T113-S3/R329 SoCs PWM support
Date: Fri, 4 Aug 2023 11:39:51 +0200 [thread overview]
Message-ID: <20230804093951.rsjuz5itn62ltqy5@pengutronix.de> (raw)
In-Reply-To: <CAF4idNmUn3G0=MU+PxTMNVb7u0xmQX4i5Z3zO6azUDBb1BezfQ@mail.gmail.com>
[-- Attachment #1.1: Type: text/plain, Size: 17262 bytes --]
Hello,
On Fri, Aug 04, 2023 at 10:19:26AM +0300, Александр Шубин wrote:
> пн, 10 июл. 2023 г. в 12:14, Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> > On Tue, Jun 27, 2023 at 11:23:25AM +0300, Aleksandr Shubin wrote:
> > > Allwinner's D1, T113-S3 and R329 SoCs have a quite different PWM
> > > controllers with ones supported by pwm-sun4i driver.
> > >
> > > This patch adds a PWM controller driver for Allwinner's D1,
> > > T113-S3 and R329 SoCs. The main difference between these SoCs
> > > is the number of channels defined by the DT property.
> > >
> > > Signed-off-by: Aleksandr Shubin <privatesub2@gmail.com>
> > > ---
> > > drivers/pwm/Kconfig | 10 ++
> > > drivers/pwm/Makefile | 1 +
> > > drivers/pwm/pwm-sun20i.c | 322 +++++++++++++++++++++++++++++++++++++++
> > > 3 files changed, 333 insertions(+)
> > > create mode 100644 drivers/pwm/pwm-sun20i.c
> > >
> > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > > index 8df861b1f4a3..05c48a36969e 100644
> > > --- a/drivers/pwm/Kconfig
> > > +++ b/drivers/pwm/Kconfig
> > > @@ -594,6 +594,16 @@ config PWM_SUN4I
> > > To compile this driver as a module, choose M here: the module
> > > will be called pwm-sun4i.
> > >
> > > +config PWM_SUN20I
> > > + tristate "Allwinner D1/T113s/R329 PWM support"
> > > + depends on ARCH_SUNXI || COMPILE_TEST
> > > + depends on COMMON_CLK
> > > + help
> > > + Generic PWM framework driver for Allwinner D1/T113s/R329 SoCs.
> > > +
> > > + To compile this driver as a module, choose M here: the module
> > > + will be called pwm-sun20i.
> > > +
> > > config PWM_SUNPLUS
> > > tristate "Sunplus PWM support"
> > > depends on ARCH_SUNPLUS || COMPILE_TEST
> > > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> > > index 19899b912e00..cea872e22c78 100644
> > > --- a/drivers/pwm/Makefile
> > > +++ b/drivers/pwm/Makefile
> > > @@ -55,6 +55,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_SUN20I) += pwm-sun20i.o
> > > obj-$(CONFIG_PWM_SUNPLUS) += pwm-sunplus.o
> > > obj-$(CONFIG_PWM_TEGRA) += pwm-tegra.o
> > > obj-$(CONFIG_PWM_TIECAP) += pwm-tiecap.o
> > > diff --git a/drivers/pwm/pwm-sun20i.c b/drivers/pwm/pwm-sun20i.c
> > > new file mode 100644
> > > index 000000000000..63e9c64e0e18
> > > --- /dev/null
> > > +++ b/drivers/pwm/pwm-sun20i.c
> > > @@ -0,0 +1,322 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * PWM Controller Driver for sunxi platforms (D1, T113-S3 and R329)
> > > + *
> > > + * Limitations:
> > > + * - When the parameters change, current running period will not be completed
> > > + * and run new settings immediately.
> > > + * - It output HIGH-Z state when PWM channel disabled.
> > > + *
> > > + * Copyright (c) 2023 Aleksandr Shubin <privatesub2@gmail.com>
> > > + */
> > > +
> > > +#include <linux/bitfield.h>
> > > +#include <linux/clk.h>
> > > +#include <linux/err.h>
> > > +#include <linux/io.h>
> > > +#include <linux/module.h>
> > > +#include <linux/of_device.h>
> > > +#include <linux/pwm.h>
> > > +#include <linux/reset.h>
> > > +
> > > +#define PWM_CLK_CFG_REG(chan) (0x20 + (((chan) >> 1) * 0x4))
> > > +#define PWM_CLK_SRC GENMASK(8, 7)
> > > +#define PWM_CLK_DIV_M GENMASK(3, 0)
> > > +
> > > +#define PWM_CLK_GATE_REG 0x40
> > > +#define PWM_CLK_BYPASS(chan) BIT((chan) - 16)
> > > +#define PWM_CLK_GATING(chan) BIT(chan)
> > > +
> > > +#define PWM_ENABLE_REG 0x80
> > > +#define PWM_EN(chan) BIT(chan)
> > > +
> > > +#define PWM_CTL_REG(chan) (0x100 + (chan) * 0x20)
> > > +#define PWM_ACT_STA BIT(8)
> > > +#define PWM_PRESCAL_K GENMASK(7, 0)
> > > +
> > > +#define PWM_PERIOD_REG(chan) (0x104 + (chan) * 0x20)
> > > +#define PWM_ENTIRE_CYCLE GENMASK(31, 16)
> > > +#define PWM_ACT_CYCLE GENMASK(15, 0)
> >
> > Can you please adapt the register field names to include the register
> > name? I'd use:
> >
> > #define PWM_CTL(chan) (0x100 + (chan) * 0x20)
> > #define PWM_CTL_ACT_STA BIT(8)
> > #define PWM_CTL_PRESCAL_K GENMASK(7, 0)
> >
> > then you get a chance to spot when PWM_CLK_BYPASS(x) is written to
> > PWM_CLK_CFG.
> >
> >
> > > +struct sun20i_pwm_chip {
> > > + struct pwm_chip chip;
> > > + struct clk *clk_bus, *clk_hosc;
> > > + struct reset_control *rst;
> > > + void __iomem *base;
> > > + /* Mutex to protect pwm apply state */
> > > + struct mutex mutex;
> > > +};
> > > +
> > > +static inline struct sun20i_pwm_chip *to_sun20i_pwm_chip(struct pwm_chip *chip)
> > > +{
> > > + return container_of(chip, struct sun20i_pwm_chip, chip);
> > > +}
> > > +
> > > +static inline u32 sun20i_pwm_readl(struct sun20i_pwm_chip *chip,
> > > + unsigned long offset)
> > > +{
> > > + return readl(chip->base + offset);
> > > +}
> > > +
> > > +static inline void sun20i_pwm_writel(struct sun20i_pwm_chip *chip,
> > > + u32 val, unsigned long offset)
> > > +{
> > > + writel(val, chip->base + offset);
> > > +}
> > > +
> > > +static int sun20i_pwm_get_state(struct pwm_chip *chip,
> > > + struct pwm_device *pwm,
> > > + struct pwm_state *state)
> > > +{
> > > + struct sun20i_pwm_chip *sun20i_chip = to_sun20i_pwm_chip(chip);
> > > + u64 clk_rate, tmp;
> > > + u32 val;
> > > + u16 ent_cycle, act_cycle;
> > > + u8 prescal, div_id;
> > > +
> > > + mutex_lock(&sun20i_chip->mutex);
> > > +
> > > + val = sun20i_pwm_readl(sun20i_chip, PWM_CLK_CFG_REG(pwm->hwpwm));
> > > + div_id = FIELD_GET(PWM_CLK_DIV_M, val);
> > > + if (FIELD_GET(PWM_CLK_SRC, val) == 0)
> > > + clk_rate = clk_get_rate(sun20i_chip->clk_hosc);
> > > + else
> > > + clk_rate = clk_get_rate(sun20i_chip->clk_bus);
> > > +
> > > + val = sun20i_pwm_readl(sun20i_chip, PWM_CTL_REG(pwm->hwpwm));
> > > + state->polarity = (PWM_ACT_STA & val) ? PWM_POLARITY_NORMAL : PWM_POLARITY_INVERSED;
> > > +
> > > + prescal = FIELD_GET(PWM_PRESCAL_K, val) + 1;
> >
> > If PWM_PRESCAL_K is 0xff, prescal ends up being 0. This isn't right, is
> > it?
> >
> > > + val = sun20i_pwm_readl(sun20i_chip, PWM_ENABLE_REG);
> > > + state->enabled = (PWM_EN(pwm->hwpwm) & val) ? true : false;
> > > +
> > > + val = sun20i_pwm_readl(sun20i_chip, PWM_PERIOD_REG(pwm->hwpwm));
> > > + act_cycle = FIELD_GET(PWM_ACT_CYCLE, val);
> > > + ent_cycle = FIELD_GET(PWM_ENTIRE_CYCLE, val);
> > > + if (act_cycle > ent_cycle)
> > > + act_cycle = ent_cycle;
> > > +
> >
> > A comment that with the width of the used factors this cannot overflow
> > would be nice here.
> >
> > > + tmp = (u64)(act_cycle) * prescal * (1U << div_id) * NSEC_PER_SEC;
> >
> > Can be simplified to:
> >
> > tmp = (u64)act_cycle * prescal << div_id * NSEC_PER_SEC;
> >
> > > + state->duty_cycle = DIV_ROUND_UP_ULL(tmp, clk_rate);
> > > + tmp = (u64)(ent_cycle) * prescal * (1U << div_id) * NSEC_PER_SEC;
> > > + state->period = DIV_ROUND_UP_ULL(tmp, clk_rate);
> > > + mutex_unlock(&sun20i_chip->mutex);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int sun20i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > > + const struct pwm_state *state)
> > > +{
> > > + int ret = 0;
> > > + u32 clk_gate, clk_cfg, pwm_en, ctl, period;
> > > + u64 bus_rate, hosc_rate, clk_div, val;
> > > + u16 prescaler, div_m;
> > > + bool use_bus_clk, calc_div_m;
> > > + struct sun20i_pwm_chip *sun20i_chip = to_sun20i_pwm_chip(chip);
> > > +
> > > + mutex_lock(&sun20i_chip->mutex);
> > > +
> > > + pwm_en = sun20i_pwm_readl(sun20i_chip, PWM_ENABLE_REG);
> > > +
> > > + if (state->enabled != pwm->state.enabled)
> > > + clk_gate = sun20i_pwm_readl(sun20i_chip, PWM_CLK_GATE_REG);
> > > +
> > > + if (state->enabled != pwm->state.enabled && !state->enabled) {
> > > + clk_gate &= ~PWM_CLK_GATING(pwm->hwpwm);
> > > + pwm_en &= ~PWM_EN(pwm->hwpwm);
> > > + sun20i_pwm_writel(sun20i_chip, pwm_en, PWM_ENABLE_REG);
> > > + sun20i_pwm_writel(sun20i_chip, clk_gate, PWM_CLK_GATE_REG);
> > > + }
> > > +
> > > + if (state->polarity != pwm->state.polarity ||
> > > + state->duty_cycle != pwm->state.duty_cycle ||
> > > + state->period != pwm->state.period) {
> > > + ctl = sun20i_pwm_readl(sun20i_chip, PWM_CTL_REG(pwm->hwpwm));
> > > + clk_cfg = sun20i_pwm_readl(sun20i_chip, PWM_CLK_CFG_REG(pwm->hwpwm));
> > > + hosc_rate = clk_get_rate(sun20i_chip->clk_hosc);
> > > + bus_rate = clk_get_rate(sun20i_chip->clk_bus);
> > > + if (pwm_en & PWM_EN(pwm->hwpwm ^ 1)) {
> > > + /* if the neighbor channel is enable, check period only */
> > > + use_bus_clk = FIELD_GET(PWM_CLK_SRC, clk_cfg) != 0;
> > > + if (use_bus_clk)
> > > + val = state->period * bus_rate;
> > > + else
> > > + val = state->period * hosc_rate;
> > > + do_div(val, NSEC_PER_SEC);
> > > +
> > > + div_m = FIELD_GET(PWM_CLK_DIV_M, clk_cfg);
> > > + calc_div_m = false;
> > > + } else {
> > > + /* check period and select clock source */
> > > + use_bus_clk = false;
> > > + val = state->period * hosc_rate;
> > > + do_div(val, NSEC_PER_SEC);
> > > + if (val <= 1) {
> > > + use_bus_clk = true;
> > > + val = state->period * bus_rate;
> > > + do_div(val, NSEC_PER_SEC);
> > > + if (val <= 1) {
> > > + ret = -EINVAL;
> > > + goto unlock_mutex;
> > > + }
> > > + }
> > > + div_m = 0;
> > > + calc_div_m = true;
> > > +
> > > + /* set up the CLK_DIV_M and clock CLK_SRC */
> > > + clk_cfg = FIELD_PREP(PWM_CLK_DIV_M, div_m);
> > > + clk_cfg |= FIELD_PREP(PWM_CLK_SRC, use_bus_clk ? 1 : 0);
> > > +
> > > + sun20i_pwm_writel(sun20i_chip, clk_cfg, PWM_CLK_CFG_REG(pwm->hwpwm));
> > > + }
> > > +
> > > + /* calculate prescaler, M factor, PWM entire cycle */
> > > + clk_div = val;
> >
> > This assignment is useless as it is overwritten in the loop below, isn't
> > it?
> >
> > > + for (prescaler = 0;; prescaler++) {
> > > + if (prescaler >= 256) {
> > > + if (calc_div_m) {
> > > + prescaler = 0;
> > > + div_m++;
> > > + if (div_m >= 9) {
> > > + ret = -EINVAL;
> > > + goto unlock_mutex;
> > > + }
> > > + } else {
> > > + ret = -EINVAL;
> > > + goto unlock_mutex;
> > > + }
> > > + }
> > > +
> > > + clk_div = val >> div_m;
> > > + do_div(clk_div, prescaler + 1);
> > > + if (clk_div <= 65534)
> > > + break;
> >
> > This can be calculated without a loop.
>
> Point me please where can I see the calculation of two divisors without a loop?
for a given value of div_m you search the smallest prescaler such that
(val >> div_m) // (prescaler + 1) ≤ 65534
(Using Python syntax where // denotes the usual
round-down-to-next-integer division and / normal exact division.)
This is equivalent to:
(val >> div_m) // (prescaler + 1) ≤ 65534
⟺ (val >> div_m) ≤ 65534 * (prescaler + 1) + prescaler
⟺ (val >> div_m) ≤ 65535 * prescaler + 65534
⟺ (val >> div_m) - 65534 ≤ 65535 * prescaler
⟺ ((val >> div_m) - 65534) / 65535 ≤ prescaler
And as prescaler is integer, this is
...
⟺ ((val >> div_m) - 65534) // 65535 ≤ prescaler
So the prescaler value you're looking for is:
((val >> div_m) - 65534) // 65535
And then you have to pick the smallest div_m such that prescaler ≤ 255:
((val >> div_m) - 65534) // 65535 ≤ 255
⟺ (val >> div_m) - 65534 ≤ 255 * 65535 + 65534
⟺ val >> div_m ≤ 255 * 65535 + 2 * 65534
⟺ val >> div_m ≤ 16842493
⟺ val >> div_m < 16842494
so div_m is fls((val) / 16842494).
You might want to double check this and explain the algorithm in a
comment similar to the above calculation.
> > > + }
> > > +
> > > + period = FIELD_PREP(PWM_ENTIRE_CYCLE, clk_div);
> > > +
> > > + /* set duty cycle */
> > > + if (use_bus_clk)
> > > + val = state->duty_cycle * bus_rate;
> > > + else
> > > + val = state->duty_cycle * hosc_rate;
maybe better use:
if (use_bus_clk)
rate = bus_rate;
else
rate = hosc_rate;
val = state->duty_cycle * rate
> > > + do_div(val, NSEC_PER_SEC);
> > > + clk_div = val >> div_m;
> > > + do_div(clk_div, prescaler + 1);
> > > +
> > > + if (state->duty_cycle == state->period)
> > > + clk_div++;
> >
> > I don't understand that one. Can you explain that in a comment please?
>
> The formula of the output period and the duty-cycle for PWM are as follows.
> T period = (PWM01_CLK / PWM0_PRESCALE_K)^-1 * (PPR0.PWM_ENTIRE_CYCLE + 1)
That can be simpler written as:
Tperiod = PWM0_PRESCALE_K / PWM01_CLK * (PPR0.PWM_ENTIRE_CYCLE + 1)
right?
> T high-level = (PWM01_CLK / PWM0_PRESCALE_K)^-1 * PPR0.PWM_ACT_CYCLE
simlar:
Thigh = PWM0_PRESCALE_K / PWM01_CLK * PPR0.PWM_ACT_CYCLE
> Duty-cycle = T high-level / T period
> In accordance with this formula, in order to set the duty-cycle to 100%,
> it is necessary that PWM_ACT_CYCLE >= PWM_ENTIRE_CYCLE + 1
The +1 is also relevant for duty_cycles other than state->period, right?
> > > + period |= FIELD_PREP(PWM_ACT_CYCLE, clk_div);
> > > + sun20i_pwm_writel(sun20i_chip, period, PWM_PERIOD_REG(pwm->hwpwm));
> > > +
> > > + ctl = FIELD_PREP(PWM_PRESCAL_K, prescaler);
> > > + if (state->polarity == PWM_POLARITY_NORMAL)
> > > + ctl |= PWM_ACT_STA;
> > > +
> > > + sun20i_pwm_writel(sun20i_chip, ctl, PWM_CTL_REG(pwm->hwpwm));
> >
> > Is this racy? I.e. does the write to PWM_PERIOD_REG(pwm->hwpwm) above
> > already has an effect before PWM_CTL_REG(pwm->hwpwm) is written?
> >
> > > + }
> > > +
> > > + if (state->enabled != pwm->state.enabled && state->enabled) {
> > > + clk_gate &= ~PWM_CLK_BYPASS(pwm->hwpwm);
> > > + clk_gate |= PWM_CLK_GATING(pwm->hwpwm);
> > > + pwm_en |= PWM_EN(pwm->hwpwm);
> > > + sun20i_pwm_writel(sun20i_chip, pwm_en, PWM_ENABLE_REG);
> > > + sun20i_pwm_writel(sun20i_chip, clk_gate, PWM_CLK_GATE_REG);
> >
> > This is (I guess) racy. If your PWM is running with
> >
> > .period = 10000
> > .duty_cyle = 0
> > .enabled = true
> >
> > and you configure it to
> >
> > .period = 10000
> > .duty_cyle = 10000
> > .enabled = false
> >
> > you get a short spike. For a enabled=true -> enabled=false transition
> > you should disable first before configuring duty+period (or skip the
> > latter completely).
>
> When switching enabled=true -> enabled=false then before setting the period,
> the enable register will be written false and the pvm will become inactive.
> this is the place:
> if (state->enabled != pwm->state.enabled && !state->enabled) {
Ah, it seems I missed that (or I just don't understand any more what
meant back then :-)
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2023-08-04 9:57 UTC|newest]
Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-06-27 8:23 [PATCH v3 0/3] Add support for Allwinner PWM on D1/T113s/R329 SoCs Aleksandr Shubin
2023-06-27 8:23 ` Aleksandr Shubin
2023-06-27 8:23 ` Aleksandr Shubin
2023-06-27 8:23 ` [PATCH v3 1/3] dt-bindings: pwm: Add binding for Allwinner D1/T113-S3/R329 PWM controller Aleksandr Shubin
2023-06-27 8:23 ` Aleksandr Shubin
2023-06-27 8:23 ` Aleksandr Shubin
2023-06-27 15:47 ` Conor Dooley
2023-06-27 15:47 ` Conor Dooley
2023-06-27 15:47 ` Conor Dooley
2023-06-27 8:23 ` [PATCH v3 2/3] pwm: Add Allwinner's D1/T113-S3/R329 SoCs PWM support Aleksandr Shubin
2023-06-27 8:23 ` Aleksandr Shubin
2023-06-27 8:23 ` Aleksandr Shubin
2023-07-07 8:18 ` Philipp Zabel
2023-07-07 8:18 ` Philipp Zabel
2023-07-07 8:18 ` Philipp Zabel
2023-07-10 9:14 ` Uwe Kleine-König
2023-07-10 9:14 ` Uwe Kleine-König
2023-07-10 9:14 ` Uwe Kleine-König
2023-08-04 7:19 ` Александр Шубин
2023-08-04 7:19 ` Александр Шубин
2023-08-04 7:19 ` Александр Шубин
2023-08-04 9:39 ` Uwe Kleine-König [this message]
2023-08-04 9:39 ` Uwe Kleine-König
2023-08-04 9:39 ` Uwe Kleine-König
2023-06-27 8:23 ` [PATCH v3 3/3] riscv: dts: allwinner: d1: Add pwm node Aleksandr Shubin
2023-06-27 8:23 ` Aleksandr Shubin
2023-06-27 8:23 ` Aleksandr Shubin
2023-07-06 17:08 ` [PATCH v3 0/3] Add support for Allwinner PWM on D1/T113s/R329 SoCs Palmer Dabbelt
2023-07-06 17:08 ` Palmer Dabbelt
2023-07-06 17:08 ` Palmer Dabbelt
2023-07-06 20:20 ` Uwe Kleine-König
2023-07-06 20:20 ` Uwe Kleine-König
2023-07-06 20:20 ` Uwe Kleine-König
2023-07-11 18:10 ` Jernej Škrabec
2023-07-11 18:10 ` Jernej Škrabec
2023-07-11 18:10 ` Jernej Škrabec
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=20230804093951.rsjuz5itn62ltqy5@pengutronix.de \
--to=u.kleine-koenig@pengutronix.de \
--cc=aou@eecs.berkeley.edu \
--cc=conor+dt@kernel.org \
--cc=cristian.ciocaltea@collabora.com \
--cc=devicetree@vger.kernel.org \
--cc=gregkh@linuxfoundation.org \
--cc=jernej.skrabec@gmail.com \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pwm@vger.kernel.org \
--cc=linux-riscv@lists.infradead.org \
--cc=linux-sunxi@lists.linux.dev \
--cc=p.zabel@pengutronix.de \
--cc=palmer@dabbelt.com \
--cc=paul.walmsley@sifive.com \
--cc=privatesub2@gmail.com \
--cc=robh+dt@kernel.org \
--cc=samuel@sholland.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: link
Be 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.