[2/2] pwm: Add support for RZ/G2L GPT

RZ/G2L General PWM Timer (GPT) composed of 8 channels with 32-bit timer
(GPT32E). It supports the following functions
 * 32 bits × 8 channels
 * Up-counting or down-counting (saw waves) or up/down-counting
   (triangle waves) for each counter.
 * Clock sources independently selectable for each channel
 * Two I/O pins per channel
 * Two output compare/input capture registers per channel
 * For the two output compare/input capture registers of each channel,
   four registers are provided as buffer registers and are capable of
   operating as comparison registers when buffering is not in use.
 * In output compare operation, buffer switching can be at crests or
   troughs, enabling the generation of laterally asymmetric PWM waveforms.
 * Registers for setting up frame cycles in each channel (with capability
   for generating interrupts at overflow or underflow)
 * Generation of dead times in PWM operation
 * Synchronous starting, stopping and clearing counters for arbitrary
   channels
 * Starting, stopping, clearing and up/down counters in response to input
   level comparison
 * Starting, clearing, stopping and up/down counters in response to a
   maximum of four external triggers
 * Output pin disable function by dead time error and detected
   short-circuits between output pins
 * A/D converter start triggers can be generated (GPT32E0 to GPT32E3)
 * Enables the noise filter for input capture and external trigger
   operation

This patch adds basic pwm support for RZ/G2L GPT driver by creating
separate logical channels for each IOs.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
RFC->V1:
 * Updated macros
 * replaced rzg2l_gpt_write_mask()->rzg2l_gpt_modify()
 * Added rzg2l_gpt_read()
---
 drivers/pwm/Kconfig         |  11 ++
 drivers/pwm/Makefile        |   1 +
 drivers/pwm/pwm-rzg2l-gpt.c | 355 ++++++++++++++++++++++++++++++++++++
 3 files changed, 367 insertions(+)
 create mode 100644 drivers/pwm/pwm-rzg2l-gpt.c

Hello,

On Tue, May 10, 2022 at 03:42:59PM +0100, Biju Das wrote:
> RZ/G2L General PWM Timer (GPT) composed of 8 channels with 32-bit timer
> (GPT32E). It supports the following functions
>  * 32 bits × 8 channels
>  * Up-counting or down-counting (saw waves) or up/down-counting
>    (triangle waves) for each counter.
>  * Clock sources independently selectable for each channel
>  * Two I/O pins per channel
>  * Two output compare/input capture registers per channel
>  * For the two output compare/input capture registers of each channel,
>    four registers are provided as buffer registers and are capable of
>    operating as comparison registers when buffering is not in use.
>  * In output compare operation, buffer switching can be at crests or
>    troughs, enabling the generation of laterally asymmetric PWM waveforms.
>  * Registers for setting up frame cycles in each channel (with capability
>    for generating interrupts at overflow or underflow)
>  * Generation of dead times in PWM operation
>  * Synchronous starting, stopping and clearing counters for arbitrary
>    channels
>  * Starting, stopping, clearing and up/down counters in response to input
>    level comparison
>  * Starting, clearing, stopping and up/down counters in response to a
>    maximum of four external triggers
>  * Output pin disable function by dead time error and detected
>    short-circuits between output pins
>  * A/D converter start triggers can be generated (GPT32E0 to GPT32E3)
>  * Enables the noise filter for input capture and external trigger
>    operation
> 
> This patch adds basic pwm support for RZ/G2L GPT driver by creating
> separate logical channels for each IOs.
> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
> RFC->V1:
>  * Updated macros
>  * replaced rzg2l_gpt_write_mask()->rzg2l_gpt_modify()
>  * Added rzg2l_gpt_read()
> ---
>  drivers/pwm/Kconfig         |  11 ++
>  drivers/pwm/Makefile        |   1 +
>  drivers/pwm/pwm-rzg2l-gpt.c | 355 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 367 insertions(+)
>  create mode 100644 drivers/pwm/pwm-rzg2l-gpt.c
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 21e3b05a5153..d93b510f9ca8 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -471,6 +471,17 @@ config PWM_ROCKCHIP
>  	  Generic PWM framework driver for the PWM controller found on
>  	  Rockchip SoCs.
>  
> +config PWM_RZG2L_GPT
> +	tristate "Renesas RZ/G2L General PWM Timer support"
> +	depends on ARCH_RENESAS || COMPILE_TEST
> +	depends on HAS_IOMEM
> +	help
> +	  This driver exposes the General PWM Timer controller found in Renesas
> +	  RZ/G2L like chips through the PWM API.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called pwm-rzg2l-gpt.
> +
>  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 708840b7fba8..bd213ae64074 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -43,6 +43,7 @@ obj-$(CONFIG_PWM_RASPBERRYPI_POE)	+= pwm-raspberrypi-poe.o
>  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_RZG2L_GPT)	+= pwm-rzg2l-gpt.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-rzg2l-gpt.c b/drivers/pwm/pwm-rzg2l-gpt.c
> new file mode 100644
> index 000000000000..d5d22b1ff792
> --- /dev/null
> +++ b/drivers/pwm/pwm-rzg2l-gpt.c
> @@ -0,0 +1,355 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Renesas RZ/G2L General PWM Timer (GPT) driver
> + *
> + * Copyright (C) 2022 Renesas Electronics Corporation
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/pwm.h>
> +#include <linux/reset.h>
> +#include <linux/units.h>
> +
> +#define GPT_IO_PER_CHANNEL	2
> +
> +#define GTPR_MAX_VALUE	0xFFFFFFFF
> +#define GTCR		0x2c
> +#define GTUDDTYC	0x30
> +#define GTIOR		0x34
> +#define GTBER		0x40
> +#define GTCNT		0x48
> +#define GTCCRA		0x4c
> +#define GTCCRB		0x50
> +#define GTPR		0x64
> +
> +#define GTCR_CST			BIT(0)
> +#define GTCR_MD_MASK			GENMASK(18, 16)
> +#define GTCR_TPCS_MASK			GENMASK(26, 24)
> +#define GTCR_MD_SAW_WAVE_PWM_MODE	(0 << 16)

I assume this should better be FIELD_PREF(GTCR_MD_MASK, 0).

Also I suggest to drop "_MASK" from the define names.

> +#define GTUDDTYC_UP	BIT(0)
> +#define GTUDDTYC_UDF	BIT(1)
> +#define UP_COUNTING	(GTUDDTYC_UP | GTUDDTYC_UDF)
> +
> +#define GTIOR_GTIOA_MASK			GENMASK(4, 0)
> +#define GTIOR_GTIOB_MASK			GENMASK(20, 16)
> +#define GTIOR_OAE				BIT(8)
> +#define GTIOR_OBE				BIT(24)
> +
> +#define INIT_OUT_LO_OUT_LO_END_TOGGLE		(0x07)
> +#define INIT_OUT_HI_OUT_HI_END_TOGGLE		(0x1B)
> +#define GTIOR_GTIOA_OUT_HI_END_TOGGLE_CMP_MATCH	(INIT_OUT_HI_OUT_HI_END_TOGGLE | GTIOR_OAE)
> +#define GTIOR_GTIOA_OUT_LO_END_TOGGLE_CMP_MATCH	(INIT_OUT_LO_OUT_LO_END_TOGGLE | GTIOR_OAE)
> +#define GTIOR_GTIOB_OUT_HI_END_TOGGLE_CMP_MATCH	((INIT_OUT_HI_OUT_HI_END_TOGGLE << 16) | GTIOR_OBE)
> +#define GTIOR_GTIOB_OUT_LO_END_TOGGLE_CMP_MATCH	((INIT_OUT_LO_OUT_LO_END_TOGGLE << 16) | GTIOR_OBE)
> +
> +struct phase {
> +	u32 value;
> +	u32 mask;
> +	u32 duty_reg_offset;
> +};
> +
> +static const struct phase phase_params[] = {
> +	/* Setting for phase A */
> +	{
> +		GTIOR_GTIOA_OUT_HI_END_TOGGLE_CMP_MATCH,
> +		GTIOR_GTIOA_MASK | GTIOR_OAE,
> +		GTCCRA,

Please use named initializer (i.e.

	.value = GTIOR_GTIOA_OUT_HI_END_TOGGLE_CMP_MATCH,
	.mask = GTIOR_GTIOA_MASK | GTIOR_OAE,
	.duty_reg_offset = GTCCRA,

)

> +	},
> +
> +	/* Setting for phase B */
> +	{
> +		GTIOR_GTIOB_OUT_HI_END_TOGGLE_CMP_MATCH,
> +		GTIOR_GTIOB_MASK | GTIOR_OBE,
> +		GTCCRB,
> +	},
> +};
> +
> +struct rzg2l_gpt_chip;
> +
> +struct gpt_pwm_device {
> +	struct rzg2l_gpt_chip *pc;
> +	const struct phase *ph;
> +	unsigned int channel;	/* IO channel number in the GPT */
> +
> +	enum pwm_polarity polarity;
> +};
> +
> +struct rzg2l_gpt_chip {
> +	struct pwm_chip chip;
> +	void __iomem *mmio;
> +	struct reset_control *rstc;
> +	struct clk *clk;
> +};

I suggest to not allocate memory in .request, instead put the two struct
gpt_pwm_device into a flexible array member in rzg2l_gpt_chip.
I also suspect that most of struct gpt_pwm_device isn't worth being
tracked. E.g. polarity is a write only variable.

> +static inline struct rzg2l_gpt_chip *to_rzg2l_gpt_chip(struct pwm_chip *chip)
> +{
> +	return container_of(chip, struct rzg2l_gpt_chip, chip);
> +}
> +
> +static void rzg2l_gpt_write(struct rzg2l_gpt_chip *pc, u32 reg, u32 data)
> +{
> +	iowrite32(data, pc->mmio + reg);
> +}
> +
> +static u32 rzg2l_gpt_read(struct rzg2l_gpt_chip *pc, u32 reg)
> +{
> +	return ioread32(pc->mmio + reg);
> +}
> +
> +static void rzg2l_gpt_modify(struct rzg2l_gpt_chip *pc, u32 reg, u32 clr, u32 set)
> +{
> +	rzg2l_gpt_write(pc, reg, (rzg2l_gpt_read(pc, reg) & ~clr) | set);
> +}
> +
> +static int rzg2l_calculate_prescale(struct rzg2l_gpt_chip *pc, int period_ns)
> +{
> +	unsigned long long c, clk_rate;
> +	unsigned long period_cycles;
> +	int prescale;
> +	int i, prod;
> +
> +	clk_rate = clk_get_rate(pc->clk);
> +	c = clk_rate * period_ns;

This might overflow (once you keep period_ns as u64).

> +	period_cycles = div_u64(c, NANO);

Please use NSEC_PER_SEC here.

> +
> +	if (period_cycles < 1)
> +		period_cycles = 1;
> +
> +	prescale = -1;
> +	/* prescale 1, 4, 16, 64, 256 and 1024 */
> +	for (i = 0, prod = 1; i < 6; i++) {
> +		if ((period_cycles / GTPR_MAX_VALUE * prod) == 0) {
> +			prescale = i;
> +			break;
> +		}
> +
> +		prod *= 4;
> +	}

This would be better understandable if you used:

	for (i = 0; i < 6; i++) {
		prod = 1 << (2 * i);
		...

	}

Have you tested this? The division by GTPR_MAX_VALUE (= 0xFFFFFFFF)
looks suspicious. Unless I'm missing something

	if ((period_cycles / GTPR_MAX_VALUE * prod) == 0)

is equivalent to:

	if (period_cycles < GTPR_MAX_VALUE)

. Is this really what you want here?

> +
> +	return prescale;
> +}
> +
> +static unsigned long
> +rzg2l_time_to_tick_number(struct rzg2l_gpt_chip *pc, int time_ns,
> +			  unsigned long prescale)
> +{
> +	unsigned long long c, clk_rate;
> +	unsigned long period_cycles;
> +	int i, prod;
> +
> +	clk_rate = clk_get_rate(pc->clk);
> +	c = clk_rate * time_ns;
> +	period_cycles = div_u64(c, NANO);
> +
> +	if (period_cycles < 1)
> +		period_cycles = 1;
> +
> +	/* Divide by 1, 4, 16, 64, 256 and 1024 */
> +	for (i = 0, prod = 1; i < prescale; i++)
> +		prod *= 4;

	prod = 1 << (2 * prescale);

> +
> +	return period_cycles / prod;

	return period_cycles >> (2 * prescale);

> +}
> +
> +static int rzg2l_gpt_request(struct pwm_chip *chip, struct pwm_device *_pwm)
> +{
> +	struct rzg2l_gpt_chip *pc = to_rzg2l_gpt_chip(chip);
> +	struct gpt_pwm_device *pwm;
> +
> +	if (_pwm->hwpwm >= GPT_IO_PER_CHANNEL)
> +		return -EINVAL;
> +
> +	pwm = kzalloc(sizeof(*pwm), GFP_KERNEL);
> +	if (!pwm)
> +		return -ENOMEM;
> +
> +	pwm->pc = pc;
> +	pwm->channel = _pwm->hwpwm;
> +	pwm->polarity = PWM_POLARITY_NORMAL;
> +	pwm->ph = &phase_params[pwm->channel & 0x1];
> +	pwm_set_chip_data(_pwm, pwm);
> +
> +	pm_runtime_get_sync(chip->dev);
> +
> +	return 0;
> +}
> +
> +static void rzg2l_gpt_free(struct pwm_chip *chip, struct pwm_device *_pwm)
> +{
> +	struct gpt_pwm_device *pwm = pwm_get_chip_data(_pwm);
> +
> +	pm_runtime_put(chip->dev);
> +	kfree(pwm);
> +}
> +
> +static int rzg2l_gpt_config(struct pwm_chip *chip, struct pwm_device *_pwm,
> +			    int duty_ns, int period_ns)
> +{
> +	struct gpt_pwm_device *pwm = pwm_get_chip_data(_pwm);

Please use the variable name pwm only for pointers to struct pwm_device.

> +	struct rzg2l_gpt_chip *pc = to_rzg2l_gpt_chip(chip);
> +	unsigned long pv, dc;
> +	int prescale;
> +
> +	if (duty_ns < 0 || period_ns < 0) {
> +		dev_err(chip->dev, "ch=%d Set time negative\n", pwm->channel);
> +		return -EINVAL;
> +	}
> +
> +	prescale = rzg2l_calculate_prescale(pc, period_ns);
> +	if (prescale < 0) {
> +		dev_err(chip->dev, "ch=%d wrong prescale val\n", pwm->channel);
> +		return -EINVAL;
> +	}
> +
> +	pv = rzg2l_time_to_tick_number(pc, period_ns, prescale);
> +	dc = rzg2l_time_to_tick_number(pc, duty_ns, prescale);

I think the algorithm could be implemented in a much more understandable
way.

> +	if (duty_ns == period_ns)
> +		dc = pv;

Isn't that already the case? If not, why? If yes, why do you calculate
dc using rzg2l_time_to_tick_number at all?

> +	/* GPT setting saw-wave up-counting */
> +	rzg2l_gpt_modify(pc, GTCR, GTCR_MD_MASK, GTCR_MD_SAW_WAVE_PWM_MODE);
> +	rzg2l_gpt_modify(pc, GTCR, GTCR_TPCS_MASK, prescale << 24);

You're doing 2 reads and 2 writes here. Wouldn't a single write be
better?

Also please use FIELD_PREP(GTCR_TPCS_MASK, prescale) instead of the abov
expression.

> +	/* Set counting mode */
> +	rzg2l_gpt_write(pc, GTUDDTYC, UP_COUNTING);
> +	/* Set period */
> +	rzg2l_gpt_write(pc, GTPR, pv);
> +
> +	/* Enable pin output */
> +	rzg2l_gpt_modify(pc, GTIOR, pwm->ph->mask, pwm->ph->value);
> +
> +	/* Set duty cycle */
> +	rzg2l_gpt_write(pc, pwm->ph->duty_reg_offset, dc);
> +
> +	/* Set initial value for counter */
> +	rzg2l_gpt_write(pc, GTCNT, 0);
> +	/* Set no buffer operation */
> +	rzg2l_gpt_write(pc, GTBER, 0);

How does the output behave on reprogramming? Does it complete the
currently programmed period? Please document this behaviour as e.g.
drivers/pwm/pwm-sl28cpld.c does.

> +	return 0;
> +}
> +
> +static int rzg2l_gpt_enable(struct rzg2l_gpt_chip *pc)
> +{
> +	/* Start count */
> +	rzg2l_gpt_modify(pc, GTCR, GTCR_CST, GTCR_CST);
> +
> +	return 0;
> +}
> +
> +static void rzg2l_gpt_disable(struct rzg2l_gpt_chip *pc)
> +{
> +	/* Stop count */
> +	rzg2l_gpt_modify(pc, GTCR, GTCR_CST, 0);

Same question here: How does the hardware behave? Does it complete the
currently running period? How does the output behave? (Typical
candidates are: freeze at the level where it is currently, constant 0,
high-z.)

> +}
> +
> +static int rzg2l_gpt_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +			   const struct pwm_state *state)
> +{
> +	struct rzg2l_gpt_chip *pc = to_rzg2l_gpt_chip(chip);
> +	int ret;

As you don't support different polarities, there is the following
missing:

	if (state->polarity != PWM_POLARITY_NORMAL)
		return -EINVAL;

> +
> +	if (!state->enabled) {
> +		rzg2l_gpt_disable(pc);
> +		return 0;
> +	}
> +
> +	ret = rzg2l_gpt_config(chip, pwm, state->duty_cycle, state->period);

Note that state->duty_cycle is an u64, but the 3rd parameter to
rzg2l_gpt_config is an int. So you're loosing bits here. (Yes, that is a
problem that the core has, too, but you should still do better here.)

> +	if (!ret)
> +		ret = rzg2l_gpt_enable(pc);
> +
> +	return ret;

It would make sense to unroll the function calls here.

> +}
> +
> +static const struct pwm_ops rzg2l_gpt_ops = {
> +	.request = rzg2l_gpt_request,
> +	.free = rzg2l_gpt_free,
> +	.apply = rzg2l_gpt_apply,

Please implement .get_state()

> +	.owner = THIS_MODULE,
> +};
> +
> +static const struct of_device_id rzg2l_gpt_of_table[] = {
> +	{ .compatible = "renesas,rzg2l-gpt", },
> +	{ /* Sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, rzg2l_gpt_of_table);
> +
> +static int rzg2l_gpt_probe(struct platform_device *pdev)
> +{
> +	struct rzg2l_gpt_chip *rzg2l_gpt;
> +	int ret;
> +
> +	rzg2l_gpt = devm_kzalloc(&pdev->dev, sizeof(*rzg2l_gpt), GFP_KERNEL);
> +	if (!rzg2l_gpt)
> +		return -ENOMEM;
> +
> +	rzg2l_gpt->mmio = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(rzg2l_gpt->mmio))
> +		return PTR_ERR(rzg2l_gpt->mmio);
> +
> +	rzg2l_gpt->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL);
> +	if (IS_ERR(rzg2l_gpt->rstc))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(rzg2l_gpt->rstc),
> +				     "get reset failed\n");
> +
> +	rzg2l_gpt->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(rzg2l_gpt->clk))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(rzg2l_gpt->clk),
> +				     "cannot get clock\n");
> +
> +	platform_set_drvdata(pdev, rzg2l_gpt);
> +
> +	ret = reset_control_deassert(rzg2l_gpt->rstc);
> +	if (ret) {
> +		dev_err(&pdev->dev, "cannot deassert reset control: %pe\n",
> +			ERR_PTR(ret));
> +		return ret;
> +	}
> +
> +	pm_runtime_enable(&pdev->dev);
> +
> +	rzg2l_gpt->chip.dev = &pdev->dev;
> +	rzg2l_gpt->chip.ops = &rzg2l_gpt_ops;
> +	rzg2l_gpt->chip.npwm = GPT_IO_PER_CHANNEL;
> +
> +	ret = pwmchip_add(&rzg2l_gpt->chip);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "failed to register GPT chip: %d\n", ret);

Please use dev_err_probe.

> +		pm_runtime_disable(&pdev->dev);
> +		reset_control_assert(rzg2l_gpt->rstc);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int rzg2l_gpt_remove(struct platform_device *pdev)
> +{
> +	struct rzg2l_gpt_chip *rzg2l_gpt = platform_get_drvdata(pdev);
> +
> +	pwmchip_remove(&rzg2l_gpt->chip);
> +	pm_runtime_disable(&pdev->dev);
> +	reset_control_assert(rzg2l_gpt->rstc);

All these have devm variants that can be used to simplify the error
paths in .probe and then you don't need a .remove function at all.

> +	return 0;
> +}
> +
> +static struct platform_driver rzg2l_gpt_driver = {
> +	.driver = {
> +		.name = "pwm-rzg2l-gpt",
> +		.of_match_table = of_match_ptr(rzg2l_gpt_of_table),
> +	},
> +	.probe = rzg2l_gpt_probe,
> +	.remove = rzg2l_gpt_remove,
> +};
> +module_platform_driver(rzg2l_gpt_driver);
> +
> +MODULE_AUTHOR("Biju Das <biju.das.jz@bp.renesas.com>");
> +MODULE_DESCRIPTION("Renesas RZ/G2L General PWM Timer (GPT) Driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:pwm-rzg2l-gpt");

Please test your driver with PWM_DEBUG enabled.

Assuming you test using sysfs, a good test is:

	echo 0 > duty_cycle

	for i in $(seq 10000 -1 1); do
		echo $i > period
	done

	for i in $(seq 1 10000); do
		echo $i > period
	done

	for i in $(seq 10000 -1 1); do
		echo $i > duty_cycle
	done

	for i in $(seq 1 10000); do
		echo $i > duty_cycle
	done

	

(Maybe pick more sensible bounds than 1 and 10000.)

Best regards
Uwe

Hi Uwe,

Thanks for the feedback.

> Subject: Re: [PATCH 2/2] pwm: Add support for RZ/G2L GPT
> 
> Hello,
> 
> On Tue, May 10, 2022 at 03:42:59PM +0100, Biju Das wrote:
> > RZ/G2L General PWM Timer (GPT) composed of 8 channels with 32-bit
> > timer (GPT32E). It supports the following functions
> >  * 32 bits × 8 channels
> >  * Up-counting or down-counting (saw waves) or up/down-counting
> >    (triangle waves) for each counter.
> >  * Clock sources independently selectable for each channel
> >  * Two I/O pins per channel
> >  * Two output compare/input capture registers per channel
> >  * For the two output compare/input capture registers of each channel,
> >    four registers are provided as buffer registers and are capable of
> >    operating as comparison registers when buffering is not in use.
> >  * In output compare operation, buffer switching can be at crests or
> >    troughs, enabling the generation of laterally asymmetric PWM
> waveforms.
> >  * Registers for setting up frame cycles in each channel (with capability
> >    for generating interrupts at overflow or underflow)
> >  * Generation of dead times in PWM operation
> >  * Synchronous starting, stopping and clearing counters for arbitrary
> >    channels
> >  * Starting, stopping, clearing and up/down counters in response to input
> >    level comparison
> >  * Starting, clearing, stopping and up/down counters in response to a
> >    maximum of four external triggers
> >  * Output pin disable function by dead time error and detected
> >    short-circuits between output pins
> >  * A/D converter start triggers can be generated (GPT32E0 to GPT32E3)
> >  * Enables the noise filter for input capture and external trigger
> >    operation
> >
> > This patch adds basic pwm support for RZ/G2L GPT driver by creating
> > separate logical channels for each IOs.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> > RFC->V1:
> >  * Updated macros
> >  * replaced rzg2l_gpt_write_mask()->rzg2l_gpt_modify()
> >  * Added rzg2l_gpt_read()
> > ---
> >  drivers/pwm/Kconfig         |  11 ++
> >  drivers/pwm/Makefile        |   1 +
> >  drivers/pwm/pwm-rzg2l-gpt.c | 355
> > ++++++++++++++++++++++++++++++++++++
> >  3 files changed, 367 insertions(+)
> >  create mode 100644 drivers/pwm/pwm-rzg2l-gpt.c
> >
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index
> > 21e3b05a5153..d93b510f9ca8 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -471,6 +471,17 @@ config PWM_ROCKCHIP
> >  	  Generic PWM framework driver for the PWM controller found on
> >  	  Rockchip SoCs.
> >
> > +config PWM_RZG2L_GPT
> > +	tristate "Renesas RZ/G2L General PWM Timer support"
> > +	depends on ARCH_RENESAS || COMPILE_TEST
> > +	depends on HAS_IOMEM
> > +	help
> > +	  This driver exposes the General PWM Timer controller found in
> Renesas
> > +	  RZ/G2L like chips through the PWM API.
> > +
> > +	  To compile this driver as a module, choose M here: the module
> > +	  will be called pwm-rzg2l-gpt.
> > +
> >  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 708840b7fba8..bd213ae64074 100644
> > --- a/drivers/pwm/Makefile
> > +++ b/drivers/pwm/Makefile
> > @@ -43,6 +43,7 @@ obj-$(CONFIG_PWM_RASPBERRYPI_POE)	+= pwm-raspberrypi-
> poe.o
> >  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_RZG2L_GPT)	+= pwm-rzg2l-gpt.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-rzg2l-gpt.c b/drivers/pwm/pwm-rzg2l-gpt.c
> > new file mode 100644 index 000000000000..d5d22b1ff792
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-rzg2l-gpt.c
> > @@ -0,0 +1,355 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Renesas RZ/G2L General PWM Timer (GPT) driver
> > + *
> > + * Copyright (C) 2022 Renesas Electronics Corporation  */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/pwm.h>
> > +#include <linux/reset.h>
> > +#include <linux/units.h>
> > +
> > +#define GPT_IO_PER_CHANNEL	2
> > +
> > +#define GTPR_MAX_VALUE	0xFFFFFFFF
> > +#define GTCR		0x2c
> > +#define GTUDDTYC	0x30
> > +#define GTIOR		0x34
> > +#define GTBER		0x40
> > +#define GTCNT		0x48
> > +#define GTCCRA		0x4c
> > +#define GTCCRB		0x50
> > +#define GTPR		0x64
> > +
> > +#define GTCR_CST			BIT(0)
> > +#define GTCR_MD_MASK			GENMASK(18, 16)
> > +#define GTCR_TPCS_MASK			GENMASK(26, 24)
> > +#define GTCR_MD_SAW_WAVE_PWM_MODE	(0 << 16)
> 
> I assume this should better be FIELD_PREF(GTCR_MD_MASK, 0).
> 
> Also I suggest to drop "_MASK" from the define names.

OK.

> 
> > +#define GTUDDTYC_UP	BIT(0)
> > +#define GTUDDTYC_UDF	BIT(1)
> > +#define UP_COUNTING	(GTUDDTYC_UP | GTUDDTYC_UDF)
> > +
> > +#define GTIOR_GTIOA_MASK			GENMASK(4, 0)
> > +#define GTIOR_GTIOB_MASK			GENMASK(20, 16)
> > +#define GTIOR_OAE				BIT(8)
> > +#define GTIOR_OBE				BIT(24)
> > +
> > +#define INIT_OUT_LO_OUT_LO_END_TOGGLE		(0x07)
> > +#define INIT_OUT_HI_OUT_HI_END_TOGGLE		(0x1B)
> > +#define GTIOR_GTIOA_OUT_HI_END_TOGGLE_CMP_MATCH
> 	(INIT_OUT_HI_OUT_HI_END_TOGGLE | GTIOR_OAE)
> > +#define GTIOR_GTIOA_OUT_LO_END_TOGGLE_CMP_MATCH
> 	(INIT_OUT_LO_OUT_LO_END_TOGGLE | GTIOR_OAE)
> > +#define GTIOR_GTIOB_OUT_HI_END_TOGGLE_CMP_MATCH
> 	((INIT_OUT_HI_OUT_HI_END_TOGGLE << 16) | GTIOR_OBE)
> > +#define GTIOR_GTIOB_OUT_LO_END_TOGGLE_CMP_MATCH
> 	((INIT_OUT_LO_OUT_LO_END_TOGGLE << 16) | GTIOR_OBE)
> > +
> > +struct phase {
> > +	u32 value;
> > +	u32 mask;
> > +	u32 duty_reg_offset;
> > +};
> > +
> > +static const struct phase phase_params[] = {
> > +	/* Setting for phase A */
> > +	{
> > +		GTIOR_GTIOA_OUT_HI_END_TOGGLE_CMP_MATCH,
> > +		GTIOR_GTIOA_MASK | GTIOR_OAE,
> > +		GTCCRA,
> 
> Please use named initializer (i.e.
> 
> 	.value = GTIOR_GTIOA_OUT_HI_END_TOGGLE_CMP_MATCH,
> 	.mask = GTIOR_GTIOA_MASK | GTIOR_OAE,
> 	.duty_reg_offset = GTCCRA,
> 
> )
OK. Agreed.

> 
> > +	},
> > +
> > +	/* Setting for phase B */
> > +	{
> > +		GTIOR_GTIOB_OUT_HI_END_TOGGLE_CMP_MATCH,
> > +		GTIOR_GTIOB_MASK | GTIOR_OBE,
> > +		GTCCRB,
> > +	},
> > +};
> > +
> > +struct rzg2l_gpt_chip;
> > +
> > +struct gpt_pwm_device {
> > +	struct rzg2l_gpt_chip *pc;
> > +	const struct phase *ph;
> > +	unsigned int channel;	/* IO channel number in the GPT */
> > +
> > +	enum pwm_polarity polarity;
> > +};
> > +
> > +struct rzg2l_gpt_chip {
> > +	struct pwm_chip chip;
> > +	void __iomem *mmio;
> > +	struct reset_control *rstc;
> > +	struct clk *clk;
> > +};
> 
> I suggest to not allocate memory in .request, instead put the two struct
> gpt_pwm_device into a flexible array member in rzg2l_gpt_chip.
> I also suspect that most of struct gpt_pwm_device isn't worth being
> tracked. E.g. polarity is a write only variable.

OK. Will add to rzg2l_gpt_chip.

> 
> > +static inline struct rzg2l_gpt_chip *to_rzg2l_gpt_chip(struct
> > +pwm_chip *chip) {
> > +	return container_of(chip, struct rzg2l_gpt_chip, chip); }
> > +
> > +static void rzg2l_gpt_write(struct rzg2l_gpt_chip *pc, u32 reg, u32
> > +data) {
> > +	iowrite32(data, pc->mmio + reg);
> > +}
> > +
> > +static u32 rzg2l_gpt_read(struct rzg2l_gpt_chip *pc, u32 reg) {
> > +	return ioread32(pc->mmio + reg);
> > +}
> > +
> > +static void rzg2l_gpt_modify(struct rzg2l_gpt_chip *pc, u32 reg, u32
> > +clr, u32 set) {
> > +	rzg2l_gpt_write(pc, reg, (rzg2l_gpt_read(pc, reg) & ~clr) | set); }
> > +
> > +static int rzg2l_calculate_prescale(struct rzg2l_gpt_chip *pc, int
> > +period_ns) {
> > +	unsigned long long c, clk_rate;
> > +	unsigned long period_cycles;
> > +	int prescale;
> > +	int i, prod;
> > +
> > +	clk_rate = clk_get_rate(pc->clk);
> > +	c = clk_rate * period_ns;
> 
> This might overflow (once you keep period_ns as u64).
OK, the logic is changed like below to avoid overflow.

	freq = div_u64(clk_get_rate(pc->clk), 1000000);
	period_cycles = div_u64(freq * period_ns, 1000);

> 
> > +	period_cycles = div_u64(c, NANO);
> 
> Please use NSEC_PER_SEC here.
> 
> > +
> > +	if (period_cycles < 1)
> > +		period_cycles = 1;
> > +
> > +	prescale = -1;
> > +	/* prescale 1, 4, 16, 64, 256 and 1024 */
> > +	for (i = 0, prod = 1; i < 6; i++) {
> > +		if ((period_cycles / GTPR_MAX_VALUE * prod) == 0) {
> > +			prescale = i;
> > +			break;
> > +		}
> > +
> > +		prod *= 4;
> > +	}
> 
> This would be better understandable if you used:
> 
> 	for (i = 0; i < 6; i++) {
> 		prod = 1 << (2 * i);
> 		...
> 
> 	}
> 
> Have you tested this? The division by GTPR_MAX_VALUE (= 0xFFFFFFFF) looks
> suspicious. Unless I'm missing something
> 
> 	if ((period_cycles / GTPR_MAX_VALUE * prod) == 0)
> 
> is equivalent to:
> 
> 	if (period_cycles < GTPR_MAX_VALUE)
> 
> . Is this really what you want here?

On the next version, I have changed the logic to check this condition in caller function.

	if (period_cycles > 1UL * GTPR_MAX_VALUE * GPT_MAX_PRESCALE_VAL) {
		dev_warn(chip->dev, "ch=%d period exceed limit\n", pwm->hwpwm);
		return -EINVAL;
	}


> 
> > +
> > +	return prescale;
> > +}
> > +
> > +static unsigned long
> > +rzg2l_time_to_tick_number(struct rzg2l_gpt_chip *pc, int time_ns,
> > +			  unsigned long prescale)
> > +{
> > +	unsigned long long c, clk_rate;
> > +	unsigned long period_cycles;
> > +	int i, prod;
> > +
> > +	clk_rate = clk_get_rate(pc->clk);
> > +	c = clk_rate * time_ns;
> > +	period_cycles = div_u64(c, NANO);
> > +
> > +	if (period_cycles < 1)
> > +		period_cycles = 1;
> > +
> > +	/* Divide by 1, 4, 16, 64, 256 and 1024 */
> > +	for (i = 0, prod = 1; i < prescale; i++)
> > +		prod *= 4;
> 
> 	prod = 1 << (2 * prescale);
> 
> > +
> > +	return period_cycles / prod;
> 
> 	return period_cycles >> (2 * prescale);

OK this function is removed in next version and directly using in the caller.

	period_cycles = div_u64(freq * duty_ns, 1000);
	dc = period_cycles >> (2 * prescale);

> 
> > +}
> > +
> > +static int rzg2l_gpt_request(struct pwm_chip *chip, struct pwm_device
> > +*_pwm) {
> > +	struct rzg2l_gpt_chip *pc = to_rzg2l_gpt_chip(chip);
> > +	struct gpt_pwm_device *pwm;
> > +
> > +	if (_pwm->hwpwm >= GPT_IO_PER_CHANNEL)
> > +		return -EINVAL;
> > +
> > +	pwm = kzalloc(sizeof(*pwm), GFP_KERNEL);
> > +	if (!pwm)
> > +		return -ENOMEM;
> > +
> > +	pwm->pc = pc;
> > +	pwm->channel = _pwm->hwpwm;
> > +	pwm->polarity = PWM_POLARITY_NORMAL;
> > +	pwm->ph = &phase_params[pwm->channel & 0x1];
> > +	pwm_set_chip_data(_pwm, pwm);
> > +
> > +	pm_runtime_get_sync(chip->dev);
> > +
> > +	return 0;
> > +}
> > +
> > +static void rzg2l_gpt_free(struct pwm_chip *chip, struct pwm_device
> > +*_pwm) {
> > +	struct gpt_pwm_device *pwm = pwm_get_chip_data(_pwm);
> > +
> > +	pm_runtime_put(chip->dev);
> > +	kfree(pwm);
> > +}
> > +
> > +static int rzg2l_gpt_config(struct pwm_chip *chip, struct pwm_device
> *_pwm,
> > +			    int duty_ns, int period_ns)
> > +{
> > +	struct gpt_pwm_device *pwm = pwm_get_chip_data(_pwm);
> 
> Please use the variable name pwm only for pointers to struct pwm_device.
Agreed.

> 
> > +	struct rzg2l_gpt_chip *pc = to_rzg2l_gpt_chip(chip);
> > +	unsigned long pv, dc;
> > +	int prescale;
> > +
> > +	if (duty_ns < 0 || period_ns < 0) {
> > +		dev_err(chip->dev, "ch=%d Set time negative\n", pwm->channel);
> > +		return -EINVAL;
> > +	}
> > +
> > +	prescale = rzg2l_calculate_prescale(pc, period_ns);
> > +	if (prescale < 0) {
> > +		dev_err(chip->dev, "ch=%d wrong prescale val\n", pwm-
> >channel);
> > +		return -EINVAL;
> > +	}
> > +
> > +	pv = rzg2l_time_to_tick_number(pc, period_ns, prescale);
> > +	dc = rzg2l_time_to_tick_number(pc, duty_ns, prescale);
> 
> I think the algorithm could be implemented in a much more understandable
> way.
> 
> > +	if (duty_ns == period_ns)
> > +		dc = pv;
> 
> Isn't that already the case? If not, why? If yes, why do you calculate dc
> using rzg2l_time_to_tick_number at all?

It is not required.

> 
> > +	/* GPT setting saw-wave up-counting */
> > +	rzg2l_gpt_modify(pc, GTCR, GTCR_MD_MASK, GTCR_MD_SAW_WAVE_PWM_MODE);
> > +	rzg2l_gpt_modify(pc, GTCR, GTCR_TPCS_MASK, prescale << 24);
> 
> You're doing 2 reads and 2 writes here. Wouldn't a single write be better?

As per the hardware flow the sequence is like this, I need to set count direction
In between.
	/* GPT set operating mode (saw-wave up-counting) */
	/* Set count direction */
	/* Select count clock(prescalar) */


> 
> Also please use FIELD_PREP(GTCR_TPCS_MASK, prescale) instead of the abov
> expression.

Agreed.

> 
> > +	/* Set counting mode */
> > +	rzg2l_gpt_write(pc, GTUDDTYC, UP_COUNTING);
> > +	/* Set period */
> > +	rzg2l_gpt_write(pc, GTPR, pv);
> > +
> > +	/* Enable pin output */
> > +	rzg2l_gpt_modify(pc, GTIOR, pwm->ph->mask, pwm->ph->value);
> > +
> > +	/* Set duty cycle */
> > +	rzg2l_gpt_write(pc, pwm->ph->duty_reg_offset, dc);
> > +
> > +	/* Set initial value for counter */
> > +	rzg2l_gpt_write(pc, GTCNT, 0);
> > +	/* Set no buffer operation */
> > +	rzg2l_gpt_write(pc, GTBER, 0);
> 
> How does the output behave on reprogramming? Does it complete the currently
> programmed period? Please document this behaviour as e.g.
> drivers/pwm/pwm-sl28cpld.c does.

Mode and Prescalar must be set, only when the GTCNT is stopped.
This condition will document.

> 
> > +	return 0;
> > +}
> > +
> > +static int rzg2l_gpt_enable(struct rzg2l_gpt_chip *pc) {
> > +	/* Start count */
> > +	rzg2l_gpt_modify(pc, GTCR, GTCR_CST, GTCR_CST);
> > +
> > +	return 0;
> > +}
> > +
> > +static void rzg2l_gpt_disable(struct rzg2l_gpt_chip *pc) {
> > +	/* Stop count */
> > +	rzg2l_gpt_modify(pc, GTCR, GTCR_CST, 0);
> 
> Same question here: How does the hardware behave? Does it complete the
> currently running period? How does the output behave? (Typical candidates
> are: freeze at the level where it is currently, constant 0,
> high-z.)

It is set to output low during stop.

> 
> > +}
> > +
> > +static int rzg2l_gpt_apply(struct pwm_chip *chip, struct pwm_device
> *pwm,
> > +			   const struct pwm_state *state)
> > +{
> > +	struct rzg2l_gpt_chip *pc = to_rzg2l_gpt_chip(chip);
> > +	int ret;
> 
> As you don't support different polarities, there is the following
> missing:

There is a plan to add polarity in later version.

> 
> 	if (state->polarity != PWM_POLARITY_NORMAL)
> 		return -EINVAL;

Agree, will add this check in initial version.

> 
> > +
> > +	if (!state->enabled) {
> > +		rzg2l_gpt_disable(pc);
> > +		return 0;
> > +	}
> > +
> > +	ret = rzg2l_gpt_config(chip, pwm, state->duty_cycle, state->period);
> 
> Note that state->duty_cycle is an u64, but the 3rd parameter to
> rzg2l_gpt_config is an int. So you're loosing bits here. (Yes, that is a
> problem that the core has, too, but you should still do better here.)
> 
> > +	if (!ret)
> > +		ret = rzg2l_gpt_enable(pc);
> > +
> > +	return ret;
> 
> It would make sense to unroll the function calls here.

OK, Agreed.

> 
> > +}
> > +
> > +static const struct pwm_ops rzg2l_gpt_ops = {
> > +	.request = rzg2l_gpt_request,
> > +	.free = rzg2l_gpt_free,
> > +	.apply = rzg2l_gpt_apply,
> 
> Please implement .get_state()

Agreed.

> 
> > +	.owner = THIS_MODULE,
> > +};
> > +
> > +static const struct of_device_id rzg2l_gpt_of_table[] = {
> > +	{ .compatible = "renesas,rzg2l-gpt", },
> > +	{ /* Sentinel */ },
> > +};
> > +MODULE_DEVICE_TABLE(of, rzg2l_gpt_of_table);
> > +
> > +static int rzg2l_gpt_probe(struct platform_device *pdev) {
> > +	struct rzg2l_gpt_chip *rzg2l_gpt;
> > +	int ret;
> > +
> > +	rzg2l_gpt = devm_kzalloc(&pdev->dev, sizeof(*rzg2l_gpt),
> GFP_KERNEL);
> > +	if (!rzg2l_gpt)
> > +		return -ENOMEM;
> > +
> > +	rzg2l_gpt->mmio = devm_platform_ioremap_resource(pdev, 0);
> > +	if (IS_ERR(rzg2l_gpt->mmio))
> > +		return PTR_ERR(rzg2l_gpt->mmio);
> > +
> > +	rzg2l_gpt->rstc = devm_reset_control_get_exclusive(&pdev->dev,
> NULL);
> > +	if (IS_ERR(rzg2l_gpt->rstc))
> > +		return dev_err_probe(&pdev->dev, PTR_ERR(rzg2l_gpt->rstc),
> > +				     "get reset failed\n");
> > +
> > +	rzg2l_gpt->clk = devm_clk_get(&pdev->dev, NULL);
> > +	if (IS_ERR(rzg2l_gpt->clk))
> > +		return dev_err_probe(&pdev->dev, PTR_ERR(rzg2l_gpt->clk),
> > +				     "cannot get clock\n");
> > +
> > +	platform_set_drvdata(pdev, rzg2l_gpt);
> > +
> > +	ret = reset_control_deassert(rzg2l_gpt->rstc);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "cannot deassert reset control: %pe\n",
> > +			ERR_PTR(ret));
> > +		return ret;
> > +	}
> > +
> > +	pm_runtime_enable(&pdev->dev);
> > +
> > +	rzg2l_gpt->chip.dev = &pdev->dev;
> > +	rzg2l_gpt->chip.ops = &rzg2l_gpt_ops;
> > +	rzg2l_gpt->chip.npwm = GPT_IO_PER_CHANNEL;
> > +
> > +	ret = pwmchip_add(&rzg2l_gpt->chip);
> > +	if (ret < 0) {
> > +		dev_err(&pdev->dev, "failed to register GPT chip: %d\n", ret);
> 
> Please use dev_err_probe.

OK.

> 
> > +		pm_runtime_disable(&pdev->dev);
> > +		reset_control_assert(rzg2l_gpt->rstc);
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int rzg2l_gpt_remove(struct platform_device *pdev) {
> > +	struct rzg2l_gpt_chip *rzg2l_gpt = platform_get_drvdata(pdev);
> > +
> > +	pwmchip_remove(&rzg2l_gpt->chip);
> > +	pm_runtime_disable(&pdev->dev);
> > +	reset_control_assert(rzg2l_gpt->rstc);
> 
> All these have devm variants that can be used to simplify the error paths
> in .probe and then you don't need a .remove function at all.

Agreed.

> 
> > +	return 0;
> > +}
> > +
> > +static struct platform_driver rzg2l_gpt_driver = {
> > +	.driver = {
> > +		.name = "pwm-rzg2l-gpt",
> > +		.of_match_table = of_match_ptr(rzg2l_gpt_of_table),
> > +	},
> > +	.probe = rzg2l_gpt_probe,
> > +	.remove = rzg2l_gpt_remove,
> > +};
> > +module_platform_driver(rzg2l_gpt_driver);
> > +
> > +MODULE_AUTHOR("Biju Das <biju.das.jz@bp.renesas.com>");
> > +MODULE_DESCRIPTION("Renesas RZ/G2L General PWM Timer (GPT) Driver");
> > +MODULE_LICENSE("GPL"); MODULE_ALIAS("platform:pwm-rzg2l-gpt");
> 
> Please test your driver with PWM_DEBUG enabled.

OK.

> 
> Assuming you test using sysfs, a good test is:
> 
> 	echo 0 > duty_cycle
> 
> 	for i in $(seq 10000 -1 1); do
> 		echo $i > period
> 	done
> 
> 	for i in $(seq 1 10000); do
> 		echo $i > period
> 	done
> 
> 	for i in $(seq 10000 -1 1); do
> 		echo $i > duty_cycle
> 	done
> 
> 	for i in $(seq 1 10000); do
> 		echo $i > duty_cycle
> 	done

Cheers,
Biju

Hello,

On Wed, Jun 01, 2022 at 07:25:28PM +0000, Biju Das wrote:
> > On Tue, May 10, 2022 at 03:42:59PM +0100, Biju Das wrote:
> > > +	clk_rate = clk_get_rate(pc->clk);
> > > +	c = clk_rate * period_ns;
> > 
> > This might overflow (once you keep period_ns as u64).
> OK, the logic is changed like below to avoid overflow.
> 
> 	freq = div_u64(clk_get_rate(pc->clk), 1000000);
> 	period_cycles = div_u64(freq * period_ns, 1000);

This might still overflow for big period_ns and freq > 1.

Better use mul_u64_u64_div_u64 and limit clkrate to prevent an overflow.
This yields a higher precision without overflow. Something like:

	rate = clk_get_rate(pc->clk);

	/*
	 * Refuse clk rates > 1 GHz to prevent overflowing the following
	 * calculation.
	 */
	if (rate > NSEC_PER_SEC)
		return -EINVAL;

	period_cycles = mul_u64_u64_div_u64(rate, period_ns, NSEC_PER_SEC);


> > > +	period_cycles = div_u64(c, NANO);
> > 
> > Please use NSEC_PER_SEC here.
> > 
> > > +
> > > +	if (period_cycles < 1)
> > > +		period_cycles = 1;
> > > +
> > > +	prescale = -1;
> > > +	/* prescale 1, 4, 16, 64, 256 and 1024 */
> > > +	for (i = 0, prod = 1; i < 6; i++) {
> > > +		if ((period_cycles / GTPR_MAX_VALUE * prod) == 0) {
> > > +			prescale = i;
> > > +			break;
> > > +		}
> > > +
> > > +		prod *= 4;
> > > +	}
> > 
> > This would be better understandable if you used:
> > 
> > 	for (i = 0; i < 6; i++) {
> > 		prod = 1 << (2 * i);
> > 		...
> > 
> > 	}
> > 
> > Have you tested this? The division by GTPR_MAX_VALUE (= 0xFFFFFFFF) looks
> > suspicious. Unless I'm missing something
> > 
> > 	if ((period_cycles / GTPR_MAX_VALUE * prod) == 0)
> > 
> > is equivalent to:
> > 
> > 	if (period_cycles < GTPR_MAX_VALUE)
> > 
> > . Is this really what you want here?
> 
> On the next version, I have changed the logic to check this condition in caller function.
> 
> 	if (period_cycles > 1UL * GTPR_MAX_VALUE * GPT_MAX_PRESCALE_VAL) {
> 		dev_warn(chip->dev, "ch=%d period exceed limit\n", pwm->hwpwm);
> 		return -EINVAL;
> 	}

Please always round down period, that is, if the request it's too big to
implement, do the biggest period that is possible.

With GTPR_MAX_VALUE = 0xffffffff and unsigned long being 32 bit on some
arch, this isn't a sensible test.

> > > +	/* Set counting mode */
> > > +	rzg2l_gpt_write(pc, GTUDDTYC, UP_COUNTING);
> > > +	/* Set period */
> > > +	rzg2l_gpt_write(pc, GTPR, pv);
> > > +
> > > +	/* Enable pin output */
> > > +	rzg2l_gpt_modify(pc, GTIOR, pwm->ph->mask, pwm->ph->value);
> > > +
> > > +	/* Set duty cycle */
> > > +	rzg2l_gpt_write(pc, pwm->ph->duty_reg_offset, dc);
> > > +
> > > +	/* Set initial value for counter */
> > > +	rzg2l_gpt_write(pc, GTCNT, 0);
> > > +	/* Set no buffer operation */
> > > +	rzg2l_gpt_write(pc, GTBER, 0);
> > 
> > How does the output behave on reprogramming? Does it complete the currently
> > programmed period? Please document this behaviour as e.g.
> > drivers/pwm/pwm-sl28cpld.c does.
> 
> Mode and Prescalar must be set, only when the GTCNT is stopped.
> This condition will document.

Please document this at the top of the driver. Take e.g. pwm-sifive.c as
a template. (And please stick to the format used there for easy
greppability, i.e. use "Limitations:" and a list of items with no empty
line between them.)

> > > +	return 0;
> > > +}
> > > +
> > > +static int rzg2l_gpt_enable(struct rzg2l_gpt_chip *pc) {
> > > +	/* Start count */
> > > +	rzg2l_gpt_modify(pc, GTCR, GTCR_CST, GTCR_CST);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static void rzg2l_gpt_disable(struct rzg2l_gpt_chip *pc) {
> > > +	/* Stop count */
> > > +	rzg2l_gpt_modify(pc, GTCR, GTCR_CST, 0);
> > 
> > Same question here: How does the hardware behave? Does it complete the
> > currently running period? How does the output behave? (Typical candidates
> > are: freeze at the level where it is currently, constant 0,
> > high-z.)
> 
> It is set to output low during stop.

OK, that should go to the Limitations section, too (even though it's not
a limitation).

> > > +}
> > > +
> > > +static int rzg2l_gpt_apply(struct pwm_chip *chip, struct pwm_device
> > *pwm,
> > > +			   const struct pwm_state *state)
> > > +{
> > > +	struct rzg2l_gpt_chip *pc = to_rzg2l_gpt_chip(chip);
> > > +	int ret;
> > 
> > As you don't support different polarities, there is the following
> > missing:
> 
> There is a plan to add polarity in later version.
> 
> > 
> > 	if (state->polarity != PWM_POLARITY_NORMAL)
> > 		return -EINVAL;
> 
> Agree, will add this check in initial version.

Does it output low or inactive level on disable if inversed polarity is
configured?

Best regards
Uwe

Hi Uwe,

Thanks for the feedback.

> Subject: Re: [PATCH 2/2] pwm: Add support for RZ/G2L GPT
> 
> Hello,
> 
> On Wed, Jun 01, 2022 at 07:25:28PM +0000, Biju Das wrote:
> > > On Tue, May 10, 2022 at 03:42:59PM +0100, Biju Das wrote:
> > > > +	clk_rate = clk_get_rate(pc->clk);
> > > > +	c = clk_rate * period_ns;
> > >
> > > This might overflow (once you keep period_ns as u64).
> > OK, the logic is changed like below to avoid overflow.
> >
> > 	freq = div_u64(clk_get_rate(pc->clk), 1000000);
> > 	period_cycles = div_u64(freq * period_ns, 1000);
> 
> This might still overflow for big period_ns and freq > 1.
> 
> Better use mul_u64_u64_div_u64 and limit clkrate to prevent an overflow.
> This yields a higher precision without overflow. Something like:
> 
> 	rate = clk_get_rate(pc->clk);
> 
> 	/*
> 	 * Refuse clk rates > 1 GHz to prevent overflowing the following
> 	 * calculation.
> 	 */
> 	if (rate > NSEC_PER_SEC)
> 		return -EINVAL;
> 
> 	period_cycles = mul_u64_u64_div_u64(rate, period_ns, NSEC_PER_SEC);

OK. Will use this logic.

> 
> > > > +	period_cycles = div_u64(c, NANO);
> > >
> > > Please use NSEC_PER_SEC here.
> > >
> > > > +
> > > > +	if (period_cycles < 1)
> > > > +		period_cycles = 1;
> > > > +
> > > > +	prescale = -1;
> > > > +	/* prescale 1, 4, 16, 64, 256 and 1024 */
> > > > +	for (i = 0, prod = 1; i < 6; i++) {
> > > > +		if ((period_cycles / GTPR_MAX_VALUE * prod) == 0) {
> > > > +			prescale = i;
> > > > +			break;
> > > > +		}
> > > > +
> > > > +		prod *= 4;
> > > > +	}
> > >
> > > This would be better understandable if you used:
> > >
> > > 	for (i = 0; i < 6; i++) {
> > > 		prod = 1 << (2 * i);
> > > 		...
> > >
> > > 	}
> > >
> > > Have you tested this? The division by GTPR_MAX_VALUE (= 0xFFFFFFFF)
> > > looks suspicious. Unless I'm missing something
> > >
> > > 	if ((period_cycles / GTPR_MAX_VALUE * prod) == 0)
> > >
> > > is equivalent to:
> > >
> > > 	if (period_cycles < GTPR_MAX_VALUE)
> > >
> > > . Is this really what you want here?
> >
> > On the next version, I have changed the logic to check this condition in
> caller function.
> >
> > 	if (period_cycles > 1UL * GTPR_MAX_VALUE * GPT_MAX_PRESCALE_VAL) {
> > 		dev_warn(chip->dev, "ch=%d period exceed limit\n", pwm-
> >hwpwm);
> > 		return -EINVAL;
> > 	}
> 
> Please always round down period, that is, if the request it's too big to
> implement, do the biggest period that is possible.
> 

OK, will use round down.

> With GTPR_MAX_VALUE = 0xffffffff and unsigned long being 32 bit on some
> arch, this isn't a sensible test.

OK.

> 
> > > > +	/* Set counting mode */
> > > > +	rzg2l_gpt_write(pc, GTUDDTYC, UP_COUNTING);
> > > > +	/* Set period */
> > > > +	rzg2l_gpt_write(pc, GTPR, pv);
> > > > +
> > > > +	/* Enable pin output */
> > > > +	rzg2l_gpt_modify(pc, GTIOR, pwm->ph->mask, pwm->ph->value);
> > > > +
> > > > +	/* Set duty cycle */
> > > > +	rzg2l_gpt_write(pc, pwm->ph->duty_reg_offset, dc);
> > > > +
> > > > +	/* Set initial value for counter */
> > > > +	rzg2l_gpt_write(pc, GTCNT, 0);
> > > > +	/* Set no buffer operation */
> > > > +	rzg2l_gpt_write(pc, GTBER, 0);
> > >
> > > How does the output behave on reprogramming? Does it complete the
> > > currently programmed period? Please document this behaviour as e.g.
> > > drivers/pwm/pwm-sl28cpld.c does.
> >
> > Mode and Prescalar must be set, only when the GTCNT is stopped.
> > This condition will document.
> 
> Please document this at the top of the driver. Take e.g. pwm-sifive.c as a
> template. (And please stick to the format used there for easy greppability,
> i.e. use "Limitations:" and a list of items with no empty line between
> them.)

OK. Agreed.

> 
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int rzg2l_gpt_enable(struct rzg2l_gpt_chip *pc) {
> > > > +	/* Start count */
> > > > +	rzg2l_gpt_modify(pc, GTCR, GTCR_CST, GTCR_CST);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static void rzg2l_gpt_disable(struct rzg2l_gpt_chip *pc) {
> > > > +	/* Stop count */
> > > > +	rzg2l_gpt_modify(pc, GTCR, GTCR_CST, 0);
> > >
> > > Same question here: How does the hardware behave? Does it complete
> > > the currently running period? How does the output behave? (Typical
> > > candidates
> > > are: freeze at the level where it is currently, constant 0,
> > > high-z.)
> >
> > It is set to output low during stop.
> 
> OK, that should go to the Limitations section, too (even though it's not a
> limitation).

OK. Agreed.

> 
> > > > +}
> > > > +
> > > > +static int rzg2l_gpt_apply(struct pwm_chip *chip, struct
> > > > +pwm_device
> > > *pwm,
> > > > +			   const struct pwm_state *state) {
> > > > +	struct rzg2l_gpt_chip *pc = to_rzg2l_gpt_chip(chip);
> > > > +	int ret;
> > >
> > > As you don't support different polarities, there is the following
> > > missing:
> >
> > There is a plan to add polarity in later version.
> >
> > >
> > > 	if (state->polarity != PWM_POLARITY_NORMAL)
> > > 		return -EINVAL;
> >
> > Agree, will add this check in initial version.
> 
> Does it output low or inactive level on disable if inversed polarity is
> configured?

As output low on counter stop is controlled by GTIOR register(currently it is configured output low for Normal polarity on disable). We could make it to either output low or output high on disable for inversed polarity. I am planning to configure it as output low for inversed polarity, when we add the support for same.

Setting GTIOR_GTIOB_OUT_HI_END_TOGGLE_CMP_MATCH--> gives normal polarity
Setting GTIOR_GTIOB_OUT_LO_END_TOGGLE_CMP_MATCH--> gives inverse polarity which is 180 degree out of phase.

Cheers,
Biju