Hi Uwe,
Thanks for the feedback.
As I am busy with other activities, Fabrizio will respond to your comments as he has RZ/V2M board.
Cheers,
Biju
> -----Original Message-----
> From: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>
> Sent: Saturday, September 16, 2023 4:33 PM
> To: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> Cc: Thierry Reding <thierry.reding@xxxxxxxxx>; Philipp Zabel
> <p.zabel@xxxxxxxxxxxxxx>; Geert Uytterhoeven <geert+renesas@xxxxxxxxx>;
> Magnus Damm <magnus.damm@xxxxxxxxx>; linux-pwm@xxxxxxxxxxxxxxx; linux-
> renesas-soc@xxxxxxxxxxxxxxx; Fabrizio Castro
> <fabrizio.castro.jz@xxxxxxxxxxx>
> Subject: Re: [PATCH v5 2/4] pwm: Add support for RZ/V2M PWM driver
>
> Hello,
>
> sorry this took so long.
>
> On Fri, Jun 30, 2023 at 12:40:01PM +0100, Biju Das wrote:
> > The RZ/V2{M, MA} PWM Timer supports the following functions:
> >
> > * The PWM has 24-bit counters which operate at PWM_CLK (48 MHz).
> > * The frequency division ratio for internal counter operation is
> > selectable as PWM_CLK divided by 1, 16, 256, or 2048.
> > * The period as well as the duty cycle is adjustable.
> > * The low-level and high-level order of the PWM signals can be
> > inverted.
> > * The duty cycle of the PWM signal is selectable in the range from
> > 0 to 100%.
> > * The minimum resolution is 20.83 ns.
> > * Three interrupt sources: Rising and falling edges of the PWM signal
> > and clearing of the counter
> > * Counter operation and the bus interface are asynchronous and both
> > can operate independently of the magnitude relationship of the
> > respective clock periods.
> >
> > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > ---
> > v4->v5:
> > * Sorted KConfig file
> > * Sorted Make file
> > * Updated copyright header 2022->2023.
> > * Updated limitation section.
> > * Replaced the variable chip->rzv2m_pwm in rzv2m_pwm_wait_delay()
> > * Replaced polarity logic as per HW manual dutycycle = Ton/Ton+Toff, so
> > eventhough native polarity is inverted from period point of view it is
> > correct.
> > * Added logic for supporting 0% , 100% and remaining duty cycle.
> > * On config() replaced
> > pm_runtime_resume_and_get()->pm_runtime_get_sync()
> > * Counter is stopped while updating period/polarity to avoid glitches.
> > * Added error check for clk_prepare_enable()
> > * Introduced is_ch_enabled variable to cache channel enable status.
> > * clk_get_rate is called after enabling the clock and
> > clk_rate_exclusive_get()
> > * Added comment for delay
> > * Replaced 1000000000UL->NSEC_PER_SEC.
> > * Improved error handling in probe().
> > v3->v4:
> > * Documented the hardware properties in "Limitations" section
> > * Dropped the macros F2CYCLE_NSEC, U24_MASK and U24_MAX.
> > * Added RZV2M_PWMCYC_PERIOD macro for U24_MAX
> > * Dropped rzv2m_pwm_freq_div variable and started using 1 << (4 * i) for
> > calculating divider as it is power of 16.
> > * Reordered the functions to have rzv2m_pwm_config() directly before
> > rzv2m_pwm_apply().
> > * Improved the logic for calculating period and duty cycle in
> > config()
> > * Merged multiple RZV2M_PWMCTR register writes to a single write in
> > config()
> > * replaced pwm_is_enabled()->pwm->state.enabled
> > * Avoided assigning bit value as enum pwm_polarity instead used enum
> constant.
> > * Fixed various issues in probe error path.
> > * Updated the logic for PWM cycle setting register
> > * A 100% duty cycle is only possible with PWMLOW > PWMCYC. So
> restricting
> > PWMCYC values < 0xffffff
> > * The native polarity of the hardware is inverted (i.e. it starts
> > with the
> > * low part). So switched the inversion bit handling.
> > v2->v3:
> > * Added return code for rzv2m_pwm_get_state()
> > * Added comment in rzv2m_pwm_reset_assert_pm_disable()
> > v1->v2:
> > * Replaced
> > devm_reset_control_get_optional_shared->devm_reset_control_get_shared
> > ---
> > drivers/pwm/Kconfig | 11 +
> > drivers/pwm/Makefile | 1 +
> > drivers/pwm/pwm-rzv2m.c | 451
> > ++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 463 insertions(+)
> > create mode 100644 drivers/pwm/pwm-rzv2m.c
> >
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index
> > 6210babb0741..1c8dbb064ee5 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -514,6 +514,17 @@ config PWM_RZ_MTU3
> > To compile this driver as a module, choose M here: the module
> > will be called pwm-rz-mtu3.
> >
> > +config PWM_RZV2M
> > + tristate "Renesas RZ/V2M PWM support"
> > + depends on ARCH_R9A09G011 || COMPILE_TEST
> > + depends on HAS_IOMEM
> > + help
> > + This driver exposes the PWM controller found in Renesas
> > + RZ/V2M like chips through the PWM API.
> > +
> > + To compile this driver as a module, choose M here: the module
> > + will be called pwm-rzv2m.
> > +
> > config PWM_SAMSUNG
> > tristate "Samsung PWM support"
> > depends on PLAT_SAMSUNG || ARCH_S5PV210 || ARCH_EXYNOS ||
> > COMPILE_TEST diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> > index c822389c2a24..ee190df16279 100644
> > --- a/drivers/pwm/Makefile
> > +++ b/drivers/pwm/Makefile
> > @@ -47,6 +47,7 @@ obj-$(CONFIG_PWM_RCAR) += pwm-rcar.o
> > obj-$(CONFIG_PWM_RENESAS_TPU) += pwm-renesas-tpu.o
> > obj-$(CONFIG_PWM_ROCKCHIP) += pwm-rockchip.o
> > obj-$(CONFIG_PWM_RZ_MTU3) += pwm-rz-mtu3.o
> > +obj-$(CONFIG_PWM_RZV2M) += pwm-rzv2m.o
> > obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o
> > obj-$(CONFIG_PWM_SIFIVE) += pwm-sifive.o
> > obj-$(CONFIG_PWM_SL28CPLD) += pwm-sl28cpld.o
> > diff --git a/drivers/pwm/pwm-rzv2m.c b/drivers/pwm/pwm-rzv2m.c new
> > file mode 100644 index 000000000000..0c5c0b9f0e70
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-rzv2m.c
> > @@ -0,0 +1,451 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Renesas RZ/V2M PWM Timer (PWM) driver
> > + *
> > + * Copyright (C) 2023 Renesas Electronics Corporation
> > + *
> > + * Hardware manual for this IP can be found here
> > + *
> > +https://www.renesas.com/in/en/document/mah/rzv2m-users-manual-hardwar
> > +e?language=en
> > + *
> > + * Limitations:
> > + * - Changes to the duty cycle configuration get effective only after
> the next
> > + * period end.
> > + * - The duty cycle can be changed only by modifying the PWMLOW
> > +register
> > + * value and changing the pulse width at low level. The duty cycle
> > +becomes
> > + * 0% for the low width when the value of the PWMLOW register is
> > +0x0h
> > + * and 100% for the low width when the value of the PWMLOW > PWMCYC.
> > + */
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/clk.h>
> > +#include <linux/delay.h>
> > +#include <linux/io.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/pwm.h>
> > +#include <linux/reset.h>
> > +#include <linux/time.h>
> > +
> > +#define RZV2M_PWMCTR 0x0
> > +#define RZV2M_PWMCYC 0x4
> > +#define RZV2M_PWMLOW 0x8
> > +#define RZV2M_PWMCNT 0xc
> > +
> > +#define RZV2M_PWMCTR_PWMPS GENMASK(17, 16)
> > +#define RZV2M_PWMCTR_PWMHL BIT(3)
> > +#define RZV2M_PWMCTR_PWMTM BIT(2)
> > +#define RZV2M_PWMCTR_PWME BIT(1)
> > +
> > +#define RZV2M_PWMCYC_PERIOD GENMASK(23, 0)
> > +
> > +struct rzv2m_pwm_chip {
> > + struct pwm_chip chip;
> > + void __iomem *mmio;
> > + struct reset_control *rstc;
> > + struct clk *apb_clk;
> > + struct clk *pwm_clk;
> > + unsigned long rate;
> > + unsigned long delay;
> > + unsigned long pwm_cyc;
> > + enum pwm_polarity polarity;
> > + bool is_ch_enabled;
> > +};
> > +
> > +static inline struct rzv2m_pwm_chip *to_rzv2m_pwm_chip(struct
> > +pwm_chip *chip) {
> > + return container_of(chip, struct rzv2m_pwm_chip, chip); }
> > +
> > +static void rzv2m_pwm_wait_delay(struct rzv2m_pwm_chip *rzv2m_pwm) {
> > + /* delay timer when change the setting register */
> > + ndelay(rzv2m_pwm->delay);
> > +}
> > +
> > +static void rzv2m_pwm_write(struct rzv2m_pwm_chip *rzv2m_pwm, u32
> > +reg, u32 data) {
> > + writel(data, rzv2m_pwm->mmio + reg); }
> > +
> > +static u32 rzv2m_pwm_read(struct rzv2m_pwm_chip *rzv2m_pwm, u32 reg)
> > +{
> > + return readl(rzv2m_pwm->mmio + reg); }
> > +
> > +static void rzv2m_pwm_modify(struct rzv2m_pwm_chip *rzv2m_pwm, u32 reg,
> u32 clr,
> > + u32 set)
> > +{
> > + rzv2m_pwm_write(rzv2m_pwm, reg,
> > + (rzv2m_pwm_read(rzv2m_pwm, reg) & ~clr) | set); }
> > +
> > +static u8 rzv2m_pwm_calculate_prescale(struct rzv2m_pwm_chip *rzv2m_pwm,
> > + u64 period_cycles)
> > +{
> > + u32 prescaled_period_cycles;
> > + u8 prescale;
> > +
> > + prescaled_period_cycles = period_cycles >> 24;
> > + if (prescaled_period_cycles >= 256)
> > + prescale = 3;
> > + else
> > + prescale = (fls(prescaled_period_cycles) + 3) / 4;
> > +
> > + return prescale;
> > +}
> > +
> > +static int rzv2m_pwm_enable(struct rzv2m_pwm_chip *rzv2m_pwm) {
> > + int rc;
> > +
> > + rc = pm_runtime_resume_and_get(rzv2m_pwm->chip.dev);
> > + if (rc)
> > + return rc;
> > +
> > + rzv2m_pwm_modify(rzv2m_pwm, RZV2M_PWMCTR, RZV2M_PWMCTR_PWME,
> > + RZV2M_PWMCTR_PWME);
> > + rzv2m_pwm_wait_delay(rzv2m_pwm);
> > + rzv2m_pwm->is_ch_enabled = true;
> > +
> > + return 0;
> > +}
> > +
> > +static void rzv2m_pwm_disable(struct rzv2m_pwm_chip *rzv2m_pwm) {
> > + rzv2m_pwm_modify(rzv2m_pwm, RZV2M_PWMCTR, RZV2M_PWMCTR_PWME, 0);
> > + rzv2m_pwm_wait_delay(rzv2m_pwm);
> > + pm_runtime_put_sync(rzv2m_pwm->chip.dev);
> > + rzv2m_pwm->is_ch_enabled = false;
> > +}
> > +
> > +static int rzv2m_pwm_get_state(struct pwm_chip *chip, struct pwm_device
> *pwm,
> > + struct pwm_state *state)
> > +{
> > + struct rzv2m_pwm_chip *rzv2m_pwm = to_rzv2m_pwm_chip(chip);
> > + u32 period, low, val;
> > + u8 prescale;
> > +
> > + pm_runtime_get_sync(chip->dev);
> > + val = rzv2m_pwm_read(rzv2m_pwm, RZV2M_PWMCTR);
> > + state->enabled = FIELD_GET(RZV2M_PWMCTR_PWME, val);
> > + state->polarity = FIELD_GET(RZV2M_PWMCTR_PWMHL, val) ?
> > + PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;
> > + prescale = FIELD_GET(RZV2M_PWMCTR_PWMPS, val);
> > + period = rzv2m_pwm_read(rzv2m_pwm, RZV2M_PWMCYC);
> > + low = rzv2m_pwm_read(rzv2m_pwm, RZV2M_PWMLOW);
> > + if (low > period) /* 0% duty cycle */
> > + val = 0;
> > + else if (!low) /* 100% duty cycle */
> > + val = period;
> > + else
> > + val = period - low;
>
> This could be simplified to:
>
> if (low > period) /* 0% duty cycle */
> val = 0;
> else
> val = period - low;
>
> Maybe it makes sense add another variable "duty_cycle" here and use that
> for improved clarity? And then maybe rename "val" to "ctr"?
>
> > + if (period)
> > + period += 1;
>
> This looks bogus, why don't you add 1 if RZV2M_PWMCYC is 0?
>
> Also it suggests that the hardware cannot do a 100% relative duty cycle?
> If I didn't miss anything here, please add that to the list of Limitations
> above.
>
> > + state->period = DIV_ROUND_UP_ULL(NSEC_PER_SEC * (u64)period << (4 *
> prescale),
> > + rzv2m_pwm->rate);
>
> The multiplication can overflow. I see no easy way to prevent this without
> introducing a mul_u64_u64_div_roundup helper. Maybe I miss something?
>
> > + state->duty_cycle = DIV_ROUND_UP_ULL(NSEC_PER_SEC * (u64)val << (4 *
> prescale),
> > + rzv2m_pwm->rate);
> > + pm_runtime_put(chip->dev);
> > +
> > + return 0;
> > +}
> > +
> > +static int rzv2m_pwm_config(struct pwm_chip *chip, struct pwm_device
> *pwm,
> > + const struct pwm_state *state) {
> > + struct rzv2m_pwm_chip *rzv2m_pwm = to_rzv2m_pwm_chip(chip);
> > + unsigned long pwm_cyc, pwm_low;
> > + u8 prescale;
> > + u32 pwm_ctr;
> > + u64 pc, dc;
> > +
> > + /*
> > + * Refuse clk rates > 1 GHz to prevent overflowing the following
> > + * calculation.
> > + */
> > + if (rzv2m_pwm->rate > NSEC_PER_SEC)
> > + return -EINVAL;
> > +
> > + /*
> > + * Formula for calculating PWM Cycle Setting Register
> > + * PWM cycle = (PWM period(ns) / (PWM_CLK period(ns) × Div ratio)) -
> 1
> > + */
> > + pc = mul_u64_u32_div(state->period, rzv2m_pwm->rate, NSEC_PER_SEC);
>
> rzv2m_pwm->rate is an unsigned long. So mul_u64_u32_div is the wrong
> function (at least on archs where sizeof(long) > 4).
>
> > + dc = mul_u64_u32_div(state->duty_cycle, rzv2m_pwm->rate,
> NSEC_PER_SEC);
> > + prescale = rzv2m_pwm_calculate_prescale(rzv2m_pwm, pc);
> > +
> > + pwm_cyc = pc >> (4 * prescale);
>
> The division above might have round down the result, so you're losing
> precision here?
>
> > + if (pwm_cyc)
> > + pwm_cyc -= 1;
>
> If the requested period is too small, please refuse the request.
> PWM_DEBUG should catch that and emit a warning.
>
> > + if (!FIELD_FIT(RZV2M_PWMCYC_PERIOD, pwm_cyc))
> > + pwm_cyc = FIELD_MAX(RZV2M_PWMCYC_PERIOD);
> > +
> > + /*
> > + * Formula for calculating PWMLOW register
> > + * PWMLOW register = PWM cycle * Low pulse width ratio (%)
> > + */
> > + pwm_low = dc >> (4 * prescale);
> > + if (!dc) /* 0% duty cycle */
> > + pwm_low = pwm_cyc + 1;
>
> if pwm_cyc == FIELD_MAX(RZV2M_PWMCYC_PERIOD) that + 1 isn't a good idea, is
> it?
>
> > + else if (pc == dc) /* 100% duty cycle */
> > + pwm_low = 0;
> > + else
> > + pwm_low = pwm_cyc - pwm_low;
> > +
> > + /*
> > + * If the PWM channel is disabled, make sure to turn on the clock
> > + * before writing the register.
> > + */
> > + if (!pwm->state.enabled)
> > + pm_runtime_get_sync(rzv2m_pwm->chip.dev);
>
> Can it happen that this already makes the hardware emit a (probably
> wrong) signal?
>
> > + /*
> > + * To change the setting value of the PWM cycle setting register
> > + * (PWMm_PWMCYC) or polarity, set the PWME bit of the PWM control
> > + * register (PWMm_PWMCTR) to 0b and stop the counter operation.
> > + */
> > + if (rzv2m_pwm->polarity != state->polarity || rzv2m_pwm->pwm_cyc !=
> pwm_cyc) {
> > + rzv2m_pwm_modify(rzv2m_pwm, RZV2M_PWMCTR, RZV2M_PWMCTR_PWME,
> 0);
> > + rzv2m_pwm_wait_delay(rzv2m_pwm);
> > + }
> > +
> > + rzv2m_pwm_write(rzv2m_pwm, RZV2M_PWMCYC, pwm_cyc);
> > + rzv2m_pwm_write(rzv2m_pwm, RZV2M_PWMLOW, pwm_low);
> > +
> > + pwm_ctr = FIELD_PREP(RZV2M_PWMCTR_PWMPS, prescale);
> > + if (state->polarity == PWM_POLARITY_INVERSED)
> > + pwm_ctr |= RZV2M_PWMCTR_PWMHL;
> > +
> > + rzv2m_pwm_modify(rzv2m_pwm, RZV2M_PWMCTR, RZV2M_PWMCTR_PWMTM |
> > + RZV2M_PWMCTR_PWMPS | RZV2M_PWMCTR_PWMHL, pwm_ctr);
> > +
> > + if (rzv2m_pwm->polarity != state->polarity || rzv2m_pwm->pwm_cyc !=
> pwm_cyc) {
> > + rzv2m_pwm->polarity = state->polarity;
> > + rzv2m_pwm->pwm_cyc = pwm_cyc;
> > + rzv2m_pwm_modify(rzv2m_pwm, RZV2M_PWMCTR, RZV2M_PWMCTR_PWME,
> > + RZV2M_PWMCTR_PWME);
> > + }
> > +
> > + rzv2m_pwm_wait_delay(rzv2m_pwm);
> > +
> > + /* If the PWM is not enabled, turn the clock off again to save power.
> */
> > + if (!pwm->state.enabled)
> > + pm_runtime_put(rzv2m_pwm->chip.dev);
> > +
> > + return 0;
> > +}
> > +
> > +static int rzv2m_pwm_apply(struct pwm_chip *chip, struct pwm_device
> *pwm,
> > + const struct pwm_state *state)
> > +{
> > + struct rzv2m_pwm_chip *rzv2m_pwm = to_rzv2m_pwm_chip(chip);
> > + bool enabled = pwm->state.enabled;
> > + int ret;
> > +
> > + if (!state->enabled) {
> > + if (enabled)
> > + rzv2m_pwm_disable(rzv2m_pwm);
> > +
> > + return 0;
> > + }
> > +
> > + ret = rzv2m_pwm_config(chip, pwm, state);
> > + if (ret)
> > + return ret;
> > +
> > + if (!enabled)
> > + ret = rzv2m_pwm_enable(rzv2m_pwm);
> > +
> > + return ret;
> > +}
> > +
> > +static const struct pwm_ops rzv2m_pwm_ops = {
> > + .get_state = rzv2m_pwm_get_state,
> > + .apply = rzv2m_pwm_apply,
> > + .owner = THIS_MODULE,
> > +};
> > +
> > +static int rzv2m_pwm_pm_runtime_suspend(struct device *dev) {
> > + struct rzv2m_pwm_chip *rzv2m_pwm = dev_get_drvdata(dev);
> > +
> > + clk_disable_unprepare(rzv2m_pwm->pwm_clk);
> > + clk_disable_unprepare(rzv2m_pwm->apb_clk);
> > +
> > + return 0;
> > +}
> > +
> > +static int rzv2m_pwm_pm_runtime_resume(struct device *dev) {
> > + struct rzv2m_pwm_chip *rzv2m_pwm = dev_get_drvdata(dev);
> > + int err;
> > +
> > + err = clk_prepare_enable(rzv2m_pwm->apb_clk);
> > + if (err)
> > + return err;
> > +
> > + err = clk_prepare_enable(rzv2m_pwm->pwm_clk);
> > + if (err)
> > + return err;
>
> It would be consequent here to disable apb_clk in the error case to prevent
> a clk enable imbalance.
>
> > +
> > + return 0;
> > +}
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-König |
> Industrial Linux Solutions | https://www.pengutronix.de/ |
