Message-ID: <wwkzprliai3vge53fcveosfkixmri4hoyfjeulbzoezmaayoci@6hor5uwwdag4>
Date: Thu, 29 Feb 2024 17:42:16 +0100
From: Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
To: Fabrizio Castro <fabrizio.castro.jz@...esas.com>
Cc: Philipp Zabel <p.zabel@...gutronix.de>, 
	Geert Uytterhoeven <geert+renesas@...der.be>, Biju Das <biju.das.jz@...renesas.com>, 
	Magnus Damm <magnus.damm@...il.com>, linux-kernel@...r.kernel.org, linux-pwm@...r.kernel.org, 
	linux-renesas-soc@...r.kernel.org
Subject: Re: [PATCH v7 2/4] pwm: Add support for RZ/V2M PWM driver

On Mon, Feb 12, 2024 at 09:06:50PM +0000, Fabrizio Castro wrote:
> From: Biju Das <biju.das.jz@...renesas.com>
> 
> 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@...renesas.com>
> Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@...esas.com>
> ---
> 
> v6->v7:
>  * Addressed the build issue reported by the kernel test robot.
>  * Added include math64.h.
>  * Reworked rzv2m_pwm_mul_u64_u64_div_u64_roundup to make use of
>    div64_u64 and to get rid of % while keeping the same formula.
>  * Added rzv2m_pwm_mul_u64_u64_div_u64_rounddown.
>  * Replaced / with div64_u64 wherever necessary.
> v5->v6:
>  * Added Fab's Signed-off-by.
>  * Updated copyright year to 2024.
>  * Added include of limits.h.
>  * Added variable max_period to rzv2m_pwm_chip.
>  * Simplified the calculations by calculating max_period during probe,
>    based on the numerical limits of the formula and the u64 data type.
>  * Added rzv2m_pwm_mul_u64_u64_div_u64_roundup.
>  * Added rzv2m_pwm_prescale_to_shift to fix the calculation of the
>    frequency divider.
>  * Improved the calculations and the variable names of
>    rzv2m_pwm_get_state.
>  * Improved the calculations of rzv2m_pwm_config.
>  * Removed .owner from rzv2m_pwm_ops.
>  * Improved rzv2m_pwm_pm_runtime_resume and renamed its err variable to
>    ret.
>  * Removed of_match_ptr.
>  * Added Fab as module author.
> 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 | 480 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 492 insertions(+)
>  create mode 100644 drivers/pwm/pwm-rzv2m.c
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 4b956d661755..55d46e6183a2 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -524,6 +524,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 c5ec9e168ee7..cf5a4a1c3b1a 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -48,6 +48,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..eb9062293590
> --- /dev/null
> +++ b/drivers/pwm/pwm-rzv2m.c
> @@ -0,0 +1,480 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Renesas RZ/V2M PWM Timer (PWM) driver
> + *
> + * Copyright (C) 2024 Renesas Electronics Corporation
> + *
> + * Hardware manual for this IP can be found here
> + * https://www.renesas.com/in/en/document/mah/rzv2m-users-manual-hardware?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.

If polarity or period is changed, the hardware has to be stopped, to
this yields glitches.

> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/limits.h>
> +#include <linux/math64.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)
> +#define RZV2M_PWMLOW_PERIOD	GENMASK(23, 0)
> +
> +struct rzv2m_pwm_chip {
> +	u64 max_period;
> +	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 u64 rzv2m_pwm_mul_u64_u64_div_u64_roundup(u64 a, u64 b, u64 c)
> +{
> +	u64 ab = a * b;

a * b might overflow?!

> +	u64 d = div64_u64(ab, c);
> +	u64 e = d * c;
> +
> +	return d + ((ab - e) ? 1 : 0);
> +}
> +
> +static inline u64 rzv2m_pwm_mul_u64_u64_div_u64_rounddown(u64 a, u64 b, u64 c)
> +{
> +	return div64_u64(a * b, c);

ditto. This is the same function as mul_u64_u64_div_u64() isn't it?

> +}
> +
> +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 inline int rzv2m_pwm_prescale_to_shift(u8 prescale)
> +{
> +	return prescale == 3 ? 11 : prescale * 4;
> +}
> +
> +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);
> +	u16 frequency_divisor;
> +	u32 ctr, cyc, low;
> +	u8 prescale;
> +
> +	pm_runtime_get_sync(chip->dev);
> +	ctr = rzv2m_pwm_read(rzv2m_pwm, RZV2M_PWMCTR);
> +	state->enabled = FIELD_GET(RZV2M_PWMCTR_PWME, ctr);
> +	state->polarity = FIELD_GET(RZV2M_PWMCTR_PWMHL, ctr) ?
> +		PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;
> +	prescale = FIELD_GET(RZV2M_PWMCTR_PWMPS, ctr);
> +	frequency_divisor = 1 << rzv2m_pwm_prescale_to_shift(prescale);

Giving a shift is cheaper than a multiplication, I suggest to do

	frequency_divisor = rzv2m_pwm_prescale_to_shift(prescale);

and instead of multiply by frequency_divisor, use a left-shift
operation.

> +	cyc = rzv2m_pwm_read(rzv2m_pwm, RZV2M_PWMCYC);
> +	state->period = rzv2m_pwm_mul_u64_u64_div_u64_roundup(cyc + 1,
> +				NSEC_PER_SEC * frequency_divisor,
> +				rzv2m_pwm->rate);
> +
> +	low = rzv2m_pwm_read(rzv2m_pwm, RZV2M_PWMLOW);
> +	state->duty_cycle = rzv2m_pwm_mul_u64_u64_div_u64_roundup(cyc + 1 - low,
> +				NSEC_PER_SEC * frequency_divisor,
> +				rzv2m_pwm->rate);

The register semantic makes me wonder if each period starts with the low
part. In that case the hardware called "normal" what is called inverted
in the pwm framework?!

> +	return pm_runtime_put(chip->dev);

If you evaluate the return value of pm_runtime_put() maybe check
pm_runtime_get_sync() for symmetry, too?

> +}
> +
> +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);
> +	u64 period = state->period, duty_cycle = state->duty_cycle;
> +	u16 frequency_divisor;
> +	u64 pwm_cyc, pwm_low;
> +	u8 prescale;
> +	u32 pwm_ctr;
> +
> +	/*
> +	 * Clamp period and duty cycle to their maximum values for our current
> +	 * configuration rather than letting our calculations overflow.
> +	 */
> +	if (period > rzv2m_pwm->max_period) {
> +		period = rzv2m_pwm->max_period;
> +		if (duty_cycle > rzv2m_pwm->max_period)
> +			duty_cycle = period;
> +	}
> +
> +	/*
> +	 * Formula for calculating PWM Cycle Setting Register:
> +	 * PWM cycle = (PWM period(ns) / (PWM_CLK period(ns) × Div ratio)) - 1
> +	 */
> +	pwm_cyc = rzv2m_pwm_mul_u64_u64_div_u64_rounddown(period,
> +							  rzv2m_pwm->rate,
> +							  NSEC_PER_SEC);
> +	pwm_cyc = pwm_cyc ? pwm_cyc : 1;

if pwm_cyc is 0 here, the period is too short to be realized, right?
That's an error condition.

> +	prescale = rzv2m_pwm_calculate_prescale(rzv2m_pwm, pwm_cyc - 1);
> +	frequency_divisor = 1 << rzv2m_pwm_prescale_to_shift(prescale);
> +	if (frequency_divisor > 1) {
> +		pwm_cyc = rzv2m_pwm_mul_u64_u64_div_u64_rounddown(period,
> +				rzv2m_pwm->rate,
> +				NSEC_PER_SEC * frequency_divisor);
> +		pwm_cyc = pwm_cyc ? pwm_cyc : 1;
> +	}
> +
> +	if (pwm_cyc && !FIELD_FIT(RZV2M_PWMCYC_PERIOD, pwm_cyc - 1))
> +		pwm_cyc = RZV2M_PWMCYC_PERIOD + 1;

I don't understand the relevance of FIELD_FIT(RZV2M_PWMCYC_PERIOD,
pwm_cyc - 1).

> +
> +	/*
> +	 * Formula for calculating PWMLOW register:
> +	 * PWMLOW register = PWM cycle * Low pulse width ratio (%)
> +	 */
> +	pwm_low = rzv2m_pwm_mul_u64_u64_div_u64_rounddown(duty_cycle,
> +			rzv2m_pwm->rate, NSEC_PER_SEC * frequency_divisor);
> +
> +	pwm_low = pwm_cyc - pwm_low;

Either the old or the new value of pwm_low doesn't match the variable's
name. Please add another variable for the wrong one. The compiler should
optimize that out and the reader can more easily understand the code.

> +	if (!FIELD_FIT(RZV2M_PWMLOW_PERIOD, pwm_low))
> +		pwm_low = RZV2M_PWMLOW_PERIOD;
> +
> +	pwm_cyc--;
> +
> +	/*
> +	 * 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);
> +
> +	/*
> +	 * 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_probe(struct platform_device *pdev)
> +{
> +	struct rzv2m_pwm_chip *rzv2m_pwm;
> +	unsigned long apb_clk_rate;
> +	int ret;
> +
> +	rzv2m_pwm = devm_kzalloc(&pdev->dev, sizeof(*rzv2m_pwm), GFP_KERNEL);
> +	if (!rzv2m_pwm)
> +		return -ENOMEM;
> +
> +	rzv2m_pwm->mmio = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(rzv2m_pwm->mmio))
> +		return PTR_ERR(rzv2m_pwm->mmio);
> +
> +	rzv2m_pwm->apb_clk = devm_clk_get(&pdev->dev, "apb");
> +	if (IS_ERR(rzv2m_pwm->apb_clk))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(rzv2m_pwm->apb_clk),
> +				     "cannot get apb clock\n");
> +
> +	rzv2m_pwm->pwm_clk = devm_clk_get(&pdev->dev, "pwm");
> +	if (IS_ERR(rzv2m_pwm->pwm_clk))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(rzv2m_pwm->pwm_clk),
> +				     "cannot get pwm clock\n");
> +
> +	rzv2m_pwm->rstc = devm_reset_control_get_shared(&pdev->dev, NULL);
> +	if (IS_ERR(rzv2m_pwm->rstc))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(rzv2m_pwm->rstc),
> +				     "get reset failed\n");
> +
> +	platform_set_drvdata(pdev, rzv2m_pwm);
> +	ret = reset_control_deassert(rzv2m_pwm->rstc);
> +	if (ret) {
> +		return dev_err_probe(&pdev->dev, ret,
> +			      "cannot deassert reset control\n");
> +	}
> +
> +	ret = clk_prepare_enable(rzv2m_pwm->apb_clk);
> +	if (ret < 0)
> +		goto err_reset;
> +
> +	ret = clk_prepare_enable(rzv2m_pwm->pwm_clk);
> +	if (ret < 0)
> +		goto disable_apb_clk;
> +
> +	clk_rate_exclusive_get(rzv2m_pwm->apb_clk);

There is a devm_clk_rate_exclusive_get() in next starting from tomorrow
I hope. Using it should simplify the driver.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists