diff mbox series

[v22,3/4] pwm: Add support for RZ/G2L GPT

Message ID 20241018130049.138775-4-biju.das.jz@bp.renesas.com
State Changes Requested
Headers show
Series Add support for RZ/G2L GPT | expand

Commit Message

Biju Das Oct. 18, 2024, 1 p.m. UTC
RZ/G2L General PWM Timer (GPT) composed of 8 channels with 32-bit timer
(GPT32E). It supports the following functions
 * 32 bits x 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

Add 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>
---
v21->v22:
 * Started using guard(mutex)calls.
 * Replaced round up->down operation in rzg2l_gpt_calculate_pv_or_dc() as
   it converts a nanosecond value to a register value.
 * Replaced devm_clk_get->devm_clk_get_enabled() to make clock on during
   the lifetime of driver and get rid of bootloader_enabled_channels
   variable. Also dropped rzg2l_gpt_rpm_put(), pm_runtime.h header and
   pm runtime calls, that simplified the probe() and other functions.
 * Replaced return type of the rzg2l_gpt_enable() as void as it is not
   returning any error.
 * Dropped lock from enable() and disable() as caller is holding the lock.
   this way we won't release the lock just to allow a called function to retake
   it.
v20->v21:
 * Added documentation about the relation between channels and outputs and
   subchannels.
 * Dropped using the local variable offs for calculating channel regs and
   instead start using macros.
 * Replaced u8->bool for the  bootloader_enabled_channels variable in
   struct rzg2l_gpt_chip
 * Replaced duty_cycles->duty_ticks and period_cycles->period_ticks as it
   represent hw ticks.
 * Introduced RZG2L_MAX_TICKS macro for max hw ticks and dropped the
   variable max_val from struct rzg2l_gpt_chip.
 * Updated comment related to 64-bit overflow calculation in
   calculate_period_or_duty().
 * Simplified handling of bootloader_enabled_channels[pwm->hwpwm] == true
   in a single place in rzg2l_gpt_apply()
 * For config error case, don't call pm_runtime_put_sync() and with
   enable()->acquire pm_runtime reference and disable()-->release the
   pm_runtime reference
 * Split the cleanups into two. One for reset_control_deassert() earlier
   in .probe() and dropped rstc variable from struct rzg2l_gpt_chip.
 * Added warn_once() message in probe() for bootloader setting wrong
   prescale values.
v19->v20:
 * Added locks for rmw operations in rzg2l_gpt_{en,dis}able().
 * Dropped decremeng enable_count based ch_en_bits in rzg2l_gpt_disable().
 * Added a comment in calculate_period_or_duty() related to overflow.
 * Replaced ch_en_bits->bootloader_enabled_channels and used this variable
   in probe(), apply() and remove() for simplification
 * Replaced pm_runtime_enable()->devm_pm_runtime_enable()
v18->v19:
 * Replaced RZG2L_UP_COUNTING->RZG2L_GTUDDTYC_UP_COUNTING macro.
 * Aligned RZG2L_GET_CH and RZG2L_GET_CH_OFFS macro
 * Dropped chip and clk from struct rzg2l_gpt_chip as started using
   devm_pwmchip_alloc() and devm_clk_rate_exclusive_get() to replace it.
 * Replaced rate->rate_khz in struct rzg2l_gpt_chip and added a check in
   probe() to make sure rate is multiple of 1000.
 * Replaced container_of->pwmchip_get_drvdata() to get device data.
 * Added a check in rzg2l_gpt_disable() not to decrement enable_count if
   ch_en_bits is set by the probe.
 * Dropped rzg2l_gpt_mul_u64_u64_div_u64()
 * Simplified calculate_period_or_duty() using rate_khz
 * Simplified rzg2l_gpt_config() using min macro for calculating period
   and duty_cycle.
 * Added checks in rzg2l_gpt_config() to prevent second channel setting
   shared register.
 * Updated error handling rzg2l_gpt_apply()
 * Added local variable dev for &pdev->dev in probe()
 * Added local varibles rate, chip and clk in probe()
 * Dropped err_clk_rate_put label as started using
   devm_clk_rate_exclusive_get()
 * Replaced rzg2l_gpt->chip as data for devm_add_action_or_reset().
 * Added error message for rate > 1GHz in probe.
v17->v18:
 * Updated copyright from 2023->2024.
 * Added units.h for KILO macro.
 * Replaced RZG2L_GTCCR{A,B}->RZG2L_GTCCR(i)
 * Introduced macros RZG2L_GTIOR_{GTIOx,OxE} to handle subchannels.
 * Replaced RZG2L_IS_IOB()->rzg2l_gpt_subchannel()
 * Replaced the cache period->period_cycles.
 * Updated rzg2l_gpt_is_ch_enabled() to return early if counter is not
   running.
 * Updated calculate_period_or_duty() for avoiding overflows.
 * Updated rzg2l_gpt_calculate_pv_or_dc() with simplified calculation for
   DIV64_U64_ROUND_UP() and dropped the cast for U32_MAX in min_t.
 * Replaced mul_u64_u32_div->rzg2l_gpt_mul_u64_u64_div_u64() helper.
 * Dropped pm pointer from struct rzg2l_gpt_driver() and simplified clk
   handling in probe().
v16->v17:
* Added ret = dev_err_probe() to avoid return success in probe().
* Dropped unneeded MODULE_ALIAS().
* Dropped .owner from struct rzg2l_gpt_ops.
* Fixed build issue reported by kernel test robot <lkp@intel.com> by
  replacing DIV_ROUND_UP()->DIV64_U64_ROUND_UP() in
  rzg2l_gpt_calculate_pv_or_dc().
* Added max_val to struct rzg2l_gpt_chip to compute maximum period
  supported by the HW in probe() and limit its value in apply() to
  avoid 64-bit overflow with computation.
* Added helper function calculate_period_or_duty() to avoid losing
  precision for smaller period/duty cycle values
  ((2^32 * 10^9 << 2) < 2^64), by not processing the rounded values.
* Replaced mul_u64_u64_div_u64()->mul_u64_u32_div() as the former is
  giving warnings with CONFIG_PWM_DEBUG enabled for very high values. 
  eg:
	echo "###### Medium setting 11000 sec = 3hours ##"
	echo 11000000000000 > /sys/class/pwm/$PWMCHIP/pwm${IO_1}/period
	echo  5500000000000 > /sys/class/pwm/$PWMCHIP/pwm${IO_1}/duty_cycle
	dumpgptreg
	
	echo "###### High setting##"
	echo 43980465100800 > /sys/class/pwm/$PWMCHIP/pwm${IO_1}/period
	echo 23980465100800 > /sys/class/pwm/$PWMCHIP/pwm${IO_1}/duty_cycle
	dumpgptreg

	with mul_u64_u32_div():
	###### Medium setting 11000 sec = 3hours ##
	Read at address  0x10048464 (0xffffb9426464): 0x400746FE
	Read at address  0x1004844C (0xffff8fdfb44c): 0x2003A37F
	Read at address  0x1004842C (0xffff9855b42c): 0x05000001
	###### High setting##
	Read at address  0x10048464 (0xffff9101b464): 0xFFFFFFFF
	Read at address  0x1004844C (0xffffaee0544c): 0x8B95AD77
	Read at address  0x1004842C (0xffffbbc8a42c): 0x05000001

	with mul_u64_u64_div_u64():
	###### Medium setting 11000 sec = 3hours ##
	Read at address  0x10048464 (0xffffb3185464): 0x400746FE
	Read at address  0x1004844C (0xffff81ebb44c): 0x2003A37F
	Read at address  0x1004842C (0xffff904fd42c): 0x05000001
	######[  304.213944] pwm-rzg2l-gpt 10048000.pwm: .apply is not idempotent (ena=1 pol=0 5500000000000/43980352512000) -> (ena=1 pol=0 5500000000000/43980239923200)
	 High setting##
	[  304.230854] pwm-rzg2l-gpt 10048000.pwm: .apply is not idempotent (ena=1 pol=0 23980465100800/43980352512000) -> (ena=1 pol=0 23980465100800/43980239923200)
	Read at address  0x10048464 (0xffffb5bb3464): 0xFFFFAA19
	Read at address  0x1004844C (0xffff99b8c44c): 0x8B95AD77
	Read at address  0x1004842C (0xffffbba2342c): 0x05000001
v15->v16:
* Replaced the macro DIV_ROUND_UP_ULL->DIV64_U64_ROUND_UP
* Added DIV_ROUND_UP in rzg2l_gpt_calculate_pv_or_dc() to avoid loss of
  precision.
* Replaced min->min_t() in rzg2l_gpt_calculate_pv_or_dc().
* Added a comment for rzg2l_gpt_config()
* Replaced mul_u64_u32_div()->mul_u64_u64_div_u64() in rzg2l_gpt_config()
* Fixed the logical condition related to counter stop in
  rzg2l_gpt_config().
* Dropped pm_runtime_resume_*() from rzg2l_gpt_config() as it is managed
  by rzg2l_gpt_apply().
* Moved pm_runtime_resume_*() from rzg2l_gpt_{en,dis}able() to
  rzg2l_gpt_apply().
v14->v15:
* Added enable_count and ch_en_bits variables to struct rzg2l_gpt_chip
  based on feedback for pwm_mtu3 driver.
* Updated copyright header and commit description by replacing "This patch
  adds"-> "Add"
* Replaced macro RZG2L_GET_CH_INDEX->RZG2L_GET_CH and replaced ch_index->ch
  throughout
* rzg2l_gpt_{enable,disable}() enables/disables PWM based on the
  enable_count.
* Replaced pm_runtime_get_sync->pm_runtime_resume_and_get and propogated
  the error in rzg2l_gpt_get_state() and rzg2l_gpt_config()
* Reduced variable scope in rzg2l_gpt_get_state() by moving most of variables
  inside the if statement.
* Updated rzg2l_gpt_get_state() by moving duty > period check
  inside the top if block.
* Added helper functions rzg2l_gpt_calculate_pv_or_dc()to simplify config. 
  Also Improved the logic in rzg2l_gpt_calculate_pv_or_dc() by using
  min(period_or_duty_cycle >> (2 * prescale), (u64)U32_MAX);
* Updated rzg2l_gpt_get_state() by moving duty > period check
  inside the top if block.
* Simplified rzg2l_gpt_config() for updating registers
* Dropped pm_runtime_get_sync() and used bitmap variable "ch_en_bits"
  to make balanced PM usage count in rzg2l_gpt_reset_assert_pm_disable()
  For case were unbind is called before apply where pwm is enabled by
  bootloader.
* Added error check for clk_rate_exclusive_get() and clk_get_rate() in
  probe().
* Dropped prescale from struct rzg2l_gpt_chip.
* Replaced of_match_ptr(rzg2l_gpt_of_table)->rzg2l_gpt_of_table in struct
  rzg2l_gpt_driver
v13->v14:
 * Removed parenthesis for RZG2L_MAX_HW_CHANNELS and RZG2L_CHANNELS_PER_IO
 * Removed duty_cycle variable from struct rzg2l_gpt_chip and added comment
   for cache for prescale variable.
 * Fixed a bug in rzg2l_gpt_cntr_need_stop().
 * Reordered rzg2l_gpt_config() just above apply()
 * Replaced pwm_is_enabled()->pwm->state.enabled in config
 * Replaced pm_runtime_resume_and_get with unconditional pm_runtime_get_sync()
   in config().
 * Restored duty_cycle > period check in rzg2l_gpt_get_state().
 * Added error check for clk_prepare_enable() in probe() and propagating error
   to the caller for pm_runtime_resume()
 * clk_get_rate() is called after enabling the clock and clk_rate_exclusive_get()
 * Simplified rzg2l_gpt_probe() by removing bitmap variables.
 * Added pm_runtime_idle() to suspend the device during probe.
 * Moved overflow condition check from config->probe().
 * Simplified rzg2l_gpt_reset_assert_pm_disable().
v12->v13:
 * Replaced Kconfig dependency from ARCH_RENESAS->ARCH_RZG2L
 * Sorted #include <linux/limits.h> alphabetically
 * Added a comment for mutex_lock to fix check patch warning
 * Replaced data type of duty_cycle from unsigned int->u32 as
   the maximum value stored is U32_MAX.
 * Improved rzg2l_gpt_config() by removing unwanted duty_cycle related code.
 * Improved rzg2l_gpt_get_state() by setting "val = rzg2l_gpt->duty_cycle[pwm->hwpwm];", 
   and factor "tmp = NSEC_PER_SEC * (u64)val;" out of the if-statement.
 * Started using DEFINE_RUNTIME_DEV_PM_OPS(), and dropped __maybe_unused
   from the callbacks.
v11->v12:
 * Added return code for get_state()
 * Cache duty cycle/prescale as the driver cannot read the current duty
   cycle/prescale from the hardware if the hardware is disabled. Cache the
   last programmed duty cycle/prescale value to return in that case.
 * Updated rzg2l_gpt_enable to enable the clocks.
 * Updated rzg2l_gpt_disable to disable the clocks.
 * Updated rzg2l_gpt_config() to cache duty cucle/prescale value
 * Updated rzg2l_gpt_get_state to use cached value of duty cycle/prescale,If the PWM
   is disabled.
 * Simplified rzg2l_gpt_apply()
 * Added comments in rzg2l_gpt_reset_assert_pm_disable()
v10->v11:
 * Used bitmap_zero for initializing bitmap varable.
 * Fixed clock imbalance during remove for the case bootloader turning
   on PWM and module unload is called just after the boot.
 * Fixed over flow condition in get_state() for a prescale value of 2 & more.
 * Improved rzg2l_gpt_cntr_need_stop() based on prescale as it is the
   only runtime variable.
 * Added array for Cache variables state_period and prescale
 * Probe caches the prescale value set by the bootloader.
 * Updated rzg2l_gpt_config() to make use of array variables.
v9->v10:
 * Updated the error handling in probe(), clk_disable_unprepare called
   on the error path.
 * Removed ch_en array and started using bitmask instead.
v8->v9:
 * deassert after devm_clk_get() to avoid reset stays deasserted,in case
   clk_get() fails.
 * Removed ch_offs from struct rzg2l_gpt_chip and use macro instead.
 * Updated error handling in probe()
v7->v8:
 * Modelled as single PWM device handling multiple channels
 * Replaced shared reset->devm_reset_control_get_exclusive()
 * Replaced iowrite32->writel and ioread32->readl
 * Updated prescale calculation
 * Added PM runtime callbacks
 * Updated PM handling and removed "pwm_enabled_by_bootloader" variable
 * Introduced rzg2l_gpt_is_ch_enabled for checking enable status on both
   IO's
 * Moved enable/disable output pins from config->enable/disable.
 * Added rzg2l_gpt_cntr_need_stop() for caching prescalar/mode values.
v6->v7:
 * Added the comment for cacheing rzg2l_gpt->state_period.
 * Fixed boundary values for pv and dc.
 * Added comment for modifying mode, prescaler, timer counter and buffer enable
   registers.
 * Fixed buffer overflow in get_state()
 * Removed unnecessary assignment of state->period value in get_state().
 * Fixed state->duty_cycle value in get_state().
 * Added a limitation for disabling the channels.
v5->v6:
 * Updated macros RZG2L_GTIOR_GTIOB_OUT_HI_END_TOGGLE_CMP_MATCH and
   RZG2L_GTIOR_GTIOB_OUT_LO_END_TOGGLE_CMP_MATCH with computation
   involving FIELD_PREP macro.
 * Removed struct rzg2l_gpt_phase and started using RZG2L_GTCCR macro
   for duty_offset.
 * replaced misnomer real_period->state_period.
 * Added handling for values >= (1024 << 32) for both period
   and duty cycle.
 * Added comments for pwm {en,dis}abled by bootloader during probe.
v4->v5:
 * Added Hardware manual details
 * Replaced the comment GTCNT->Counter
 * Removed the macros RZG2L_GPT_IO_PER_CHANNEL and chip.npwm directly
   used in probe.
 * Removed the unsed macro RZG2L_GTPR_MAX_VALUE
 * Added driver prefix for the type name and the variable.
 * Initialization of per_channel data moved from request->probe.
 * Updated clr parameter for rzg2l_gpt_modify for Start count.
 * Started using mutex and usage_count for handling shared
   period and prescalar for the 2 channels.
 * Updated the comment cycle->period.
 * Removed clk_disable from rzg2l_gpt_reset_assert_pm_disable()
 * Replaced pc->rzg2l_gpt.
 * Updated prescale calculation.
 * Moved pm_runtime_{get_sync,put} from {request,free}->{enable,disable}
 * Removed platform_set_drvdata as it is unused
 * Removed the variable pwm_enabled_by_bootloader 
 * Added dev_err_probe in various error paths in probe.
 * Added an error message, if devm_pwmchip_add() fails.
v3->v4:
 * Changed the local variable type i from u16->u8 and prescaled_period_
   cycles from u64->u32 in calculate_prescale().
 * Replaced mul_u64_u64_div_u64()->mul_u64_u32_div()
 * Dropped the comma after the sentinel.
 * Add a variable to track pwm enabled by bootloader and added comments
   in probe().
 * Removed unnecessary rzg2l_gpt_reset_assert_pm_disable() from probe.
 * Replaced devm_clk_get()->devm_clk_get_prepared()
 * Removed devm_clk_get_optional_enabled()
v2->v3:
 * Updated limitation section
 * Added prefix "RZG2L_" for all macros
 * Modified prescale calculation
 * Removed pwm_set_chip_data
 * Updated comment related to modifying Mode and Prescaler
 * Updated setting of prescale value in rzg2l_gpt_config()
 * Removed else branch from rzg2l_gpt_get_state()
 * removed the err label from rzg2l_gpt_apply()
 * Added devm_clk_get_optional_enabled() to retain clk on status,
   in case bootloader turns on the clk of pwm.
 * Replaced devm_reset_control_get_exclusive->devm_reset_control_get_shared
   as single reset shared between 8 channels.
v1->v2:
 * Added Limitations section
 * dropped "_MASK" from the define names.
 * used named initializer for struct phase
 * Added gpt_pwm_device into a flexible array member in rzg2l_gpt_chip
 * Revised the logic for prescale
 * Added .get_state callback
 * Improved error handling in rzg2l_gpt_apply
 * Removed .remove callback
 * Tested driver with PWM_DEBUG enabled
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 | 473 ++++++++++++++++++++++++++++++++++++
 3 files changed, 485 insertions(+)
 create mode 100644 drivers/pwm/pwm-rzg2l-gpt.c

Comments

Uwe Kleine-König Nov. 29, 2024, 9:50 a.m. UTC | #1
Hello,

as I already wrote in earlier revisions I find this driver complicated
and wonder if this is because the hardware is complicated or because the
driver adds unneeded complexity. So here come a few suggestions that
might seem to be trivial but IMHO simplify understanding the driver.

On Fri, Oct 18, 2024 at 02:00:44PM +0100, Biju Das wrote:
> [...]
> diff --git a/drivers/pwm/pwm-rzg2l-gpt.c b/drivers/pwm/pwm-rzg2l-gpt.c
> new file mode 100644
> index 000000000000..28ed39eecb93
> --- /dev/null
> +++ b/drivers/pwm/pwm-rzg2l-gpt.c
> @@ -0,0 +1,473 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Renesas RZ/G2L General PWM Timer (GPT) driver
> + *
> + * Copyright (C) 2024 Renesas Electronics Corporation
> + *
> + * Hardware manual for this IP can be found here
> + * https://www.renesas.com/eu/en/document/mah/rzg2l-group-rzg2lc-group-users-manual-hardware-0?language=en
> + *
> + * Limitations:
> + * - Counter must be stopped before modifying Mode and Prescaler.
> + * - When PWM is disabled, the output is driven to inactive.
> + * - While the hardware supports both polarities, the driver (for now)
> + *   only handles normal polarity.
> + * - General PWM Timer (GPT) has 8 HW channels for PWM operations and
> + *   each HW channel have 2 IOs.
> + * - Each IO is modelled as an independent PWM channel.
> + * - When both channels are used, disabling the channel on one stops the
> + *   other.
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/limits.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/reset.h>
> +#include <linux/time.h>
> +#include <linux/units.h>
> +
> +#define RZG2L_GET_CH(a)		((a) / 2)

The parameter is a hwpwm value. If you use "hwpwm" instead of "a" this
is directly obvious.

> +#define RZG2L_GET_CH_OFFS(i)	(0x100 * (i))

The parameter is a channel number, rename it to ch.

> +#define RZG2L_GTCR(ch)		(0x2c + RZG2L_GET_CH_OFFS(ch))
> +#define RZG2L_GTUDDTYC(ch)	(0x30 + RZG2L_GET_CH_OFFS(ch))
> +#define RZG2L_GTIOR(ch)		(0x34 + RZG2L_GET_CH_OFFS(ch))
> +#define RZG2L_GTBER(ch)		(0x40 + RZG2L_GET_CH_OFFS(ch))
> +#define RZG2L_GTCNT(ch)		(0x48 + RZG2L_GET_CH_OFFS(ch))
> +#define RZG2L_GTCCR(ch, sub_ch)	(0x4c + RZG2L_GET_CH_OFFS(ch) + 4 * (sub_ch))
> +#define RZG2L_GTPR(ch)		(0x64 + RZG2L_GET_CH_OFFS(ch))
> +
> +#define RZG2L_GTCR_CST		BIT(0)
> +#define RZG2L_GTCR_MD		GENMASK(18, 16)
> +#define RZG2L_GTCR_TPCS		GENMASK(26, 24)
> +
> +#define RZG2L_GTCR_MD_SAW_WAVE_PWM_MODE	FIELD_PREP(RZG2L_GTCR_MD, 0)
> +
> +#define RZG2L_GTUDDTYC_UP	BIT(0)
> +#define RZG2L_GTUDDTYC_UDF	BIT(1)
> +#define RZG2L_GTUDDTYC_UP_COUNTING	(RZG2L_GTUDDTYC_UP | RZG2L_GTUDDTYC_UDF)
> +
> +#define RZG2L_GTIOR_GTIOA	GENMASK(4, 0)
> +#define RZG2L_GTIOR_GTIOB	GENMASK(20, 16)
> +#define RZG2L_GTIOR_GTIOx(a)	((a) ? RZG2L_GTIOR_GTIOB : RZG2L_GTIOR_GTIOA)

sub_ch instead of a.

> +#define RZG2L_GTIOR_OAE		BIT(8)
> +#define RZG2L_GTIOR_OBE		BIT(24)
> +#define RZG2L_GTIOR_OxE(a)	((a) ? RZG2L_GTIOR_OBE : RZG2L_GTIOR_OAE)
> +
> +#define RZG2L_INIT_OUT_HI_OUT_HI_END_TOGGLE	0x1b
> +#define RZG2L_GTIOR_GTIOA_OUT_HI_END_TOGGLE_CMP_MATCH \
> +	(RZG2L_INIT_OUT_HI_OUT_HI_END_TOGGLE | RZG2L_GTIOR_OAE)
> +#define RZG2L_GTIOR_GTIOB_OUT_HI_END_TOGGLE_CMP_MATCH \
> +	(FIELD_PREP(RZG2L_GTIOR_GTIOB, RZG2L_INIT_OUT_HI_OUT_HI_END_TOGGLE) | RZG2L_GTIOR_OBE)
> +
> +#define RZG2L_GTIOR_GTIOx_OUT_HI_END_TOGGLE_CMP_MATCH(a) \
> +	((a) ? RZG2L_GTIOR_GTIOB_OUT_HI_END_TOGGLE_CMP_MATCH : \
> +	 RZG2L_GTIOR_GTIOA_OUT_HI_END_TOGGLE_CMP_MATCH)
> +
> +#define RZG2L_MAX_HW_CHANNELS	8
> +#define RZG2L_CHANNELS_PER_IO	2
> +#define RZG2L_MAX_PWM_CHANNELS	(RZG2L_MAX_HW_CHANNELS * RZG2L_CHANNELS_PER_IO)
> +#define RZG2L_MAX_SCALE_FACTOR	1024
> +#define RZG2L_MAX_TICKS ((u64)U32_MAX * RZG2L_MAX_SCALE_FACTOR)
> +
> +struct rzg2l_gpt_chip {
> +	void __iomem *mmio;
> +	struct mutex lock; /* lock to protect shared channel resources */

Hmm, I nearly claimed you'd not need that lock since 1cc2e1faafb3 ("pwm:
Add more locking") but that doesn't cover ->request(). Probably that
should change. (i.e. no action item for you.)

> +	unsigned long rate_khz;
> +	u32 period_ticks[RZG2L_MAX_HW_CHANNELS];
> +	u32 user_count[RZG2L_MAX_HW_CHANNELS];

This tracks the count of requests per channel. So maybe call it
channel_request_count?

> +	u32 enable_count[RZG2L_MAX_HW_CHANNELS];

channel_enable_count?

> +};
> [...]
> +/* Caller holds the lock while calling rzg2l_gpt_disable() */
> +static void rzg2l_gpt_disable(struct rzg2l_gpt_chip *rzg2l_gpt,
> +			      struct pwm_device *pwm)
> +{
> +	u8 sub_ch = rzg2l_gpt_subchannel(pwm->hwpwm);
> +	u8 ch = RZG2L_GET_CH(pwm->hwpwm);
> +
> +	/* Stop count, Output low on GTIOCx pin when counting stops */
> +	rzg2l_gpt->enable_count[ch]--;
> +
> +	if (!rzg2l_gpt->enable_count[ch])
> +		rzg2l_gpt_modify(rzg2l_gpt, RZG2L_GTCR(ch), RZG2L_GTCR_CST, 0);
> +
> +	/* Disable pin output */
> +	rzg2l_gpt_modify(rzg2l_gpt, RZG2L_GTIOR(ch), RZG2L_GTIOR_OxE(sub_ch), 0);
> +}
> +
> +static u64 calculate_period_or_duty(struct rzg2l_gpt_chip *rzg2l_gpt, u32 val, u8 prescale)

Missing name prefix

> +{
> [...]
> +/* Caller holds the lock while calling rzg2l_gpt_config() */
> +static int rzg2l_gpt_config(struct pwm_chip *chip, struct pwm_device *pwm,
> +			    const struct pwm_state *state)
> +{
> +	struct rzg2l_gpt_chip *rzg2l_gpt = to_rzg2l_gpt_chip(chip);
> +	u8 sub_ch = rzg2l_gpt_subchannel(pwm->hwpwm);
> +	u8 ch = RZG2L_GET_CH(pwm->hwpwm);
> +	u64 period_ticks, duty_ticks;
> +	unsigned long pv, dc;
> +	u8 prescale;
> +
> +	/* Limit period/duty cycle to max value supported by the HW */
> +	period_ticks = mul_u64_u64_div_u64(state->period, rzg2l_gpt->rate_khz, USEC_PER_SEC);
> +	if (period_ticks > RZG2L_MAX_TICKS)
> +		period_ticks = RZG2L_MAX_TICKS;
> +	/*
> +	 * GPT counter is shared by multiple channels, so prescale and period

shared by the two IOs of a single channel?

> +	 * can NOT be modified when there are multiple channels in use with

multiple IOs?

> +	 * different settings.
> +	 */
> +	if (rzg2l_gpt->user_count[ch] > 1 && period_ticks < rzg2l_gpt->period_ticks[ch])
> +		return -EBUSY;
> +
> +	prescale = rzg2l_gpt_calculate_prescale(rzg2l_gpt, period_ticks);
> +	pv = rzg2l_gpt_calculate_pv_or_dc(period_ticks, prescale);
> +
> +	duty_ticks = mul_u64_u64_div_u64(state->duty_cycle, rzg2l_gpt->rate_khz, USEC_PER_SEC);
> +	if (duty_ticks > RZG2L_MAX_TICKS)
> +		duty_ticks = RZG2L_MAX_TICKS;
> +	dc = rzg2l_gpt_calculate_pv_or_dc(duty_ticks, prescale);
> +
> +	/*
> +	 * GPT counter is shared by multiple channels, we cache the period ticks
> +	 * from the first enabled channel and use the same value for both
> +	 * channels.
> +	 */
> +	rzg2l_gpt->period_ticks[ch] = period_ticks;

Unless I'm missing something you might overwrite the value of the other
IO in the same channel here.

> +	/*
> +	 * Counter must be stopped before modifying mode, prescaler, timer
> +	 * counter and buffer enable registers. These registers are shared
> +	 * between both channels. So allow updating these registers only for the

both IOs?

> +	 * first enabled channel.
> +	 */
> +	if (rzg2l_gpt->enable_count[ch] <= 1) {
> +		rzg2l_gpt_modify(rzg2l_gpt, RZG2L_GTCR(ch), RZG2L_GTCR_CST, 0);
> +
> +		/* GPT set operating mode (saw-wave up-counting) */
> +		rzg2l_gpt_modify(rzg2l_gpt, RZG2L_GTCR(ch), RZG2L_GTCR_MD,
> +				 RZG2L_GTCR_MD_SAW_WAVE_PWM_MODE);
> +
> +		/* Set count direction */
> +		rzg2l_gpt_write(rzg2l_gpt, RZG2L_GTUDDTYC(ch), RZG2L_GTUDDTYC_UP_COUNTING);
> +
> +		/* Select count clock */
> +		rzg2l_gpt_modify(rzg2l_gpt, RZG2L_GTCR(ch), RZG2L_GTCR_TPCS,
> +				 FIELD_PREP(RZG2L_GTCR_TPCS, prescale));
> +
> +		/* Set period */
> +		rzg2l_gpt_write(rzg2l_gpt, RZG2L_GTPR(ch), pv);
> +	}
> +
> +	/* Set duty cycle */
> +	rzg2l_gpt_write(rzg2l_gpt, RZG2L_GTCCR(ch, sub_ch), dc);
> +
> +	if (rzg2l_gpt->enable_count[ch] <= 1) {
> +		/* Set initial value for counter */
> +		rzg2l_gpt_write(rzg2l_gpt, RZG2L_GTCNT(ch), 0);
> +
> +		/* Set no buffer operation */
> +		rzg2l_gpt_write(rzg2l_gpt, RZG2L_GTBER(ch), 0);
> +
> +		/* Restart the counter after updating the registers */
> +		rzg2l_gpt_modify(rzg2l_gpt, RZG2L_GTCR(ch),
> +				 RZG2L_GTCR_CST, RZG2L_GTCR_CST);
> +	}

So you're not writing duty_cycle to hardware. Then you should check that
the actual value in use is <= the intended value as you did above with
period.

> +static int rzg2l_gpt_probe(struct platform_device *pdev)
> +{
> [...]
> +	rstc = devm_reset_control_get_exclusive(dev, NULL);
> +	if (IS_ERR(rstc))
> +		return dev_err_probe(dev, PTR_ERR(rstc), "get reset failed\n");
> +
> +	clk = devm_clk_get_enabled(dev, NULL);
> +	if (IS_ERR(clk))
> +		return dev_err_probe(dev, PTR_ERR(clk), "cannot get clock\n");
> +
> +	ret = devm_clk_rate_exclusive_get(dev, clk);
> +	if (ret)
> +		return ret;
> +
> +	rate = clk_get_rate(clk);
> +	if (!rate)
> +		return dev_err_probe(dev, -EINVAL, "gpt clk rate is 0");
> +
> +	/*
> +	 * Refuse clk rates > 1 GHz to prevent overflow later for computing
> +	 * period and duty cycle.
> +	 */
> +	if (rate > NSEC_PER_SEC)
> +		return dev_err_probe(dev, -EINVAL, "gpt clk rate is > 1GHz");
> +
> +	/*
> +	 * Rate is in MHz and is always integer for peripheral clk
> +	 * 2^32 * 2^10 (prescalar) * 10^6 (rate_khz) < 2^64
> +	 * So make sure rate is multiple of 1000.
> +	 */
> +	rzg2l_gpt->rate_khz = rate / KILO;
> +	if (rzg2l_gpt->rate_khz * KILO != rate)
> +		return dev_err_probe(dev, -EINVAL, "rate is not multiple of 1000");
> +
> +	ret = reset_control_deassert(rstc);

Please move reset deassertion directly after
devm_reset_control_get_exclusive() that it can later be trivially
converted to devm_reset_control_get_exclusive_deasserted().
If you base the next revision on top of v6.13-rc1 you can also make use
of it already.

> +	chip->ops = &rzg2l_gpt_ops;
> +	ret = devm_pwmchip_add(dev, chip);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to add PWM chip\n");

nitpick: Can you make the error messages all start with a capital letter
please?

Best regards
Uwe
Biju Das Dec. 4, 2024, 6:24 p.m. UTC | #2
Hi Uwe,

Thanks for the feedback.

> -----Original Message-----
> From: Uwe Kleine-König <ukleinek@kernel.org>
> Sent: 29 November 2024 09:50
> Subject: Re: [PATCH v22 3/4] pwm: Add support for RZ/G2L GPT
> 
> Hello,
> 
> as I already wrote in earlier revisions I find this driver complicated and wonder if this is because
> the hardware is complicated or because the driver adds unneeded complexity. So here come a few
> suggestions that might seem to be trivial but IMHO simplify understanding the driver.

I agree the hardware is complicated or the driver maybe adding unneeded complexity.

> 
> On Fri, Oct 18, 2024 at 02:00:44PM +0100, Biju Das wrote:
> > [...]
> > diff --git a/drivers/pwm/pwm-rzg2l-gpt.c b/drivers/pwm/pwm-rzg2l-gpt.c
> > new file mode 100644 index 000000000000..28ed39eecb93
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-rzg2l-gpt.c
> > @@ -0,0 +1,473 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Renesas RZ/G2L General PWM Timer (GPT) driver
> > + *
> > + * Copyright (C) 2024 Renesas Electronics Corporation
> > + *
> > + * Hardware manual for this IP can be found here
> > + *
> > +https://www.renesas.com/eu/en/document/mah/rzg2l-group-rzg2lc-group-u
> > +sers-manual-hardware-0?language=en
> > + *
> > + * Limitations:
> > + * - Counter must be stopped before modifying Mode and Prescaler.
> > + * - When PWM is disabled, the output is driven to inactive.
> > + * - While the hardware supports both polarities, the driver (for now)
> > + *   only handles normal polarity.
> > + * - General PWM Timer (GPT) has 8 HW channels for PWM operations and
> > + *   each HW channel have 2 IOs.
> > + * - Each IO is modelled as an independent PWM channel.
> > + * - When both channels are used, disabling the channel on one stops the
> > + *   other.
> > + */
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/clk.h>
> > +#include <linux/io.h>
> > +#include <linux/limits.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pwm.h>
> > +#include <linux/reset.h>
> > +#include <linux/time.h>
> > +#include <linux/units.h>
> > +
> > +#define RZG2L_GET_CH(a)		((a) / 2)
> 
> The parameter is a hwpwm value. If you use "hwpwm" instead of "a" this is directly obvious.
Ok. Will fix.

> 
> > +#define RZG2L_GET_CH_OFFS(i)	(0x100 * (i))
> 
> The parameter is a channel number, rename it to ch.
Ok. Will fix.

> 
> > +#define RZG2L_GTCR(ch)		(0x2c + RZG2L_GET_CH_OFFS(ch))
> > +#define RZG2L_GTUDDTYC(ch)	(0x30 + RZG2L_GET_CH_OFFS(ch))
> > +#define RZG2L_GTIOR(ch)		(0x34 + RZG2L_GET_CH_OFFS(ch))
> > +#define RZG2L_GTBER(ch)		(0x40 + RZG2L_GET_CH_OFFS(ch))
> > +#define RZG2L_GTCNT(ch)		(0x48 + RZG2L_GET_CH_OFFS(ch))
> > +#define RZG2L_GTCCR(ch, sub_ch)	(0x4c + RZG2L_GET_CH_OFFS(ch) + 4 * (sub_ch))
> > +#define RZG2L_GTPR(ch)		(0x64 + RZG2L_GET_CH_OFFS(ch))
> > +
> > +#define RZG2L_GTCR_CST		BIT(0)
> > +#define RZG2L_GTCR_MD		GENMASK(18, 16)
> > +#define RZG2L_GTCR_TPCS		GENMASK(26, 24)
> > +
> > +#define RZG2L_GTCR_MD_SAW_WAVE_PWM_MODE	FIELD_PREP(RZG2L_GTCR_MD, 0)
> > +
> > +#define RZG2L_GTUDDTYC_UP	BIT(0)
> > +#define RZG2L_GTUDDTYC_UDF	BIT(1)
> > +#define RZG2L_GTUDDTYC_UP_COUNTING	(RZG2L_GTUDDTYC_UP | RZG2L_GTUDDTYC_UDF)
> > +
> > +#define RZG2L_GTIOR_GTIOA	GENMASK(4, 0)
> > +#define RZG2L_GTIOR_GTIOB	GENMASK(20, 16)
> > +#define RZG2L_GTIOR_GTIOx(a)	((a) ? RZG2L_GTIOR_GTIOB : RZG2L_GTIOR_GTIOA)
> 
> sub_ch instead of a.
Ok. Will fix.

> 
> > +#define RZG2L_GTIOR_OAE		BIT(8)
> > +#define RZG2L_GTIOR_OBE		BIT(24)
> > +#define RZG2L_GTIOR_OxE(a)	((a) ? RZG2L_GTIOR_OBE : RZG2L_GTIOR_OAE)
> > +
> > +#define RZG2L_INIT_OUT_HI_OUT_HI_END_TOGGLE	0x1b
> > +#define RZG2L_GTIOR_GTIOA_OUT_HI_END_TOGGLE_CMP_MATCH \
> > +	(RZG2L_INIT_OUT_HI_OUT_HI_END_TOGGLE | RZG2L_GTIOR_OAE) #define
> > +RZG2L_GTIOR_GTIOB_OUT_HI_END_TOGGLE_CMP_MATCH \
> > +	(FIELD_PREP(RZG2L_GTIOR_GTIOB, RZG2L_INIT_OUT_HI_OUT_HI_END_TOGGLE)
> > +| RZG2L_GTIOR_OBE)
> > +
> > +#define RZG2L_GTIOR_GTIOx_OUT_HI_END_TOGGLE_CMP_MATCH(a) \
> > +	((a) ? RZG2L_GTIOR_GTIOB_OUT_HI_END_TOGGLE_CMP_MATCH : \
> > +	 RZG2L_GTIOR_GTIOA_OUT_HI_END_TOGGLE_CMP_MATCH)
> > +
> > +#define RZG2L_MAX_HW_CHANNELS	8
> > +#define RZG2L_CHANNELS_PER_IO	2
> > +#define RZG2L_MAX_PWM_CHANNELS	(RZG2L_MAX_HW_CHANNELS * RZG2L_CHANNELS_PER_IO)
> > +#define RZG2L_MAX_SCALE_FACTOR	1024
> > +#define RZG2L_MAX_TICKS ((u64)U32_MAX * RZG2L_MAX_SCALE_FACTOR)
> > +
> > +struct rzg2l_gpt_chip {
> > +	void __iomem *mmio;
> > +	struct mutex lock; /* lock to protect shared channel resources */
> 
> Hmm, I nearly claimed you'd not need that lock since 1cc2e1faafb3 ("pwm:
> Add more locking") but that doesn't cover ->request(). Probably that should change. (i.e. no action
> item for you.)
> 
> > +	unsigned long rate_khz;
> > +	u32 period_ticks[RZG2L_MAX_HW_CHANNELS];
> > +	u32 user_count[RZG2L_MAX_HW_CHANNELS];
> 
> This tracks the count of requests per channel. So maybe call it channel_request_count?
Ok. Will fix.

> 
> > +	u32 enable_count[RZG2L_MAX_HW_CHANNELS];
> 
> channel_enable_count?

Ok. Will fix.

> 
> > +};
> > [...]
> > +/* Caller holds the lock while calling rzg2l_gpt_disable() */ static
> > +void rzg2l_gpt_disable(struct rzg2l_gpt_chip *rzg2l_gpt,
> > +			      struct pwm_device *pwm)
> > +{
> > +	u8 sub_ch = rzg2l_gpt_subchannel(pwm->hwpwm);
> > +	u8 ch = RZG2L_GET_CH(pwm->hwpwm);
> > +
> > +	/* Stop count, Output low on GTIOCx pin when counting stops */
> > +	rzg2l_gpt->enable_count[ch]--;
> > +
> > +	if (!rzg2l_gpt->enable_count[ch])
> > +		rzg2l_gpt_modify(rzg2l_gpt, RZG2L_GTCR(ch), RZG2L_GTCR_CST, 0);
> > +
> > +	/* Disable pin output */
> > +	rzg2l_gpt_modify(rzg2l_gpt, RZG2L_GTIOR(ch),
> > +RZG2L_GTIOR_OxE(sub_ch), 0); }
> > +
> > +static u64 calculate_period_or_duty(struct rzg2l_gpt_chip *rzg2l_gpt,
> > +u32 val, u8 prescale)
> 
> Missing name prefix

Ok. Will fix.
> 
> > +{
> > [...]
> > +/* Caller holds the lock while calling rzg2l_gpt_config() */ static
> > +int rzg2l_gpt_config(struct pwm_chip *chip, struct pwm_device *pwm,
> > +			    const struct pwm_state *state) {
> > +	struct rzg2l_gpt_chip *rzg2l_gpt = to_rzg2l_gpt_chip(chip);
> > +	u8 sub_ch = rzg2l_gpt_subchannel(pwm->hwpwm);
> > +	u8 ch = RZG2L_GET_CH(pwm->hwpwm);
> > +	u64 period_ticks, duty_ticks;
> > +	unsigned long pv, dc;
> > +	u8 prescale;
> > +
> > +	/* Limit period/duty cycle to max value supported by the HW */
> > +	period_ticks = mul_u64_u64_div_u64(state->period, rzg2l_gpt->rate_khz, USEC_PER_SEC);
> > +	if (period_ticks > RZG2L_MAX_TICKS)
> > +		period_ticks = RZG2L_MAX_TICKS;
> > +	/*
> > +	 * GPT counter is shared by multiple channels, so prescale and
> > +period
> 
> shared by the two IOs of a single channel?

Correct
> 
> > +	 * can NOT be modified when there are multiple channels in use with
> 
> multiple IOs?

Correct.

> 
> > +	 * different settings.
> > +	 */
> > +	if (rzg2l_gpt->user_count[ch] > 1 && period_ticks < rzg2l_gpt->period_ticks[ch])
> > +		return -EBUSY;

Do we need to allow this operation (period_ticks < rzg2l_gpt->period_ticks[ch]) ?

For example,
   First IO (IO_A) period/duty is in the order nsec (PWM frequency in MHz) and second channel period/duty in the
order of microsec(PWM frequency in kHz)

Allowing period_ticks < rzg2l_gpt->period_ticks[ch] will lead to incorrect operations 
for First IO (IO_A) as PWM frequency will be in kHz compared to MHz.

According to me, we should not allow updating periods for second enabled channel.

Please correct me if I am wrong.


> > +
> > +	prescale = rzg2l_gpt_calculate_prescale(rzg2l_gpt, period_ticks);
> > +	pv = rzg2l_gpt_calculate_pv_or_dc(period_ticks, prescale);
> > +
> > +	duty_ticks = mul_u64_u64_div_u64(state->duty_cycle, rzg2l_gpt->rate_khz, USEC_PER_SEC);
> > +	if (duty_ticks > RZG2L_MAX_TICKS)
> > +		duty_ticks = RZG2L_MAX_TICKS;
> > +	dc = rzg2l_gpt_calculate_pv_or_dc(duty_ticks, prescale);
> > +
> > +	/*
> > +	 * GPT counter is shared by multiple channels, we cache the period ticks
> > +	 * from the first enabled channel and use the same value for both
> > +	 * channels.
> > +	 */
> > +	rzg2l_gpt->period_ticks[ch] = period_ticks;
> 
> Unless I'm missing something you might overwrite the value of the other IO in the same channel here.

based on above.

> 
> > +	/*
> > +	 * Counter must be stopped before modifying mode, prescaler, timer
> > +	 * counter and buffer enable registers. These registers are shared
> > +	 * between both channels. So allow updating these registers only for
> > +the
> 
> both IOs?

based on above.

> 
> > +	 * first enabled channel.
> > +	 */
> > +	if (rzg2l_gpt->enable_count[ch] <= 1) {
> > +		rzg2l_gpt_modify(rzg2l_gpt, RZG2L_GTCR(ch), RZG2L_GTCR_CST, 0);
> > +
> > +		/* GPT set operating mode (saw-wave up-counting) */
> > +		rzg2l_gpt_modify(rzg2l_gpt, RZG2L_GTCR(ch), RZG2L_GTCR_MD,
> > +				 RZG2L_GTCR_MD_SAW_WAVE_PWM_MODE);
> > +
> > +		/* Set count direction */
> > +		rzg2l_gpt_write(rzg2l_gpt, RZG2L_GTUDDTYC(ch),
> > +RZG2L_GTUDDTYC_UP_COUNTING);
> > +
> > +		/* Select count clock */
> > +		rzg2l_gpt_modify(rzg2l_gpt, RZG2L_GTCR(ch), RZG2L_GTCR_TPCS,
> > +				 FIELD_PREP(RZG2L_GTCR_TPCS, prescale));
> > +
> > +		/* Set period */
> > +		rzg2l_gpt_write(rzg2l_gpt, RZG2L_GTPR(ch), pv);
> > +	}
> > +
> > +	/* Set duty cycle */
> > +	rzg2l_gpt_write(rzg2l_gpt, RZG2L_GTCCR(ch, sub_ch), dc);
> > +
> > +	if (rzg2l_gpt->enable_count[ch] <= 1) {
> > +		/* Set initial value for counter */
> > +		rzg2l_gpt_write(rzg2l_gpt, RZG2L_GTCNT(ch), 0);
> > +
> > +		/* Set no buffer operation */
> > +		rzg2l_gpt_write(rzg2l_gpt, RZG2L_GTBER(ch), 0);
> > +
> > +		/* Restart the counter after updating the registers */
> > +		rzg2l_gpt_modify(rzg2l_gpt, RZG2L_GTCR(ch),
> > +				 RZG2L_GTCR_CST, RZG2L_GTCR_CST);
> > +	}
> 
> So you're not writing duty_cycle to hardware. Then you should check that the actual value in use is <=
> the intended value as you did above with period.

based on above.

> 
> > +static int rzg2l_gpt_probe(struct platform_device *pdev) {
> > [...]
> > +	rstc = devm_reset_control_get_exclusive(dev, NULL);
> > +	if (IS_ERR(rstc))
> > +		return dev_err_probe(dev, PTR_ERR(rstc), "get reset failed\n");
> > +
> > +	clk = devm_clk_get_enabled(dev, NULL);
> > +	if (IS_ERR(clk))
> > +		return dev_err_probe(dev, PTR_ERR(clk), "cannot get clock\n");
> > +
> > +	ret = devm_clk_rate_exclusive_get(dev, clk);
> > +	if (ret)
> > +		return ret;
> > +
> > +	rate = clk_get_rate(clk);
> > +	if (!rate)
> > +		return dev_err_probe(dev, -EINVAL, "gpt clk rate is 0");
> > +
> > +	/*
> > +	 * Refuse clk rates > 1 GHz to prevent overflow later for computing
> > +	 * period and duty cycle.
> > +	 */
> > +	if (rate > NSEC_PER_SEC)
> > +		return dev_err_probe(dev, -EINVAL, "gpt clk rate is > 1GHz");
> > +
> > +	/*
> > +	 * Rate is in MHz and is always integer for peripheral clk
> > +	 * 2^32 * 2^10 (prescalar) * 10^6 (rate_khz) < 2^64
> > +	 * So make sure rate is multiple of 1000.
> > +	 */
> > +	rzg2l_gpt->rate_khz = rate / KILO;
> > +	if (rzg2l_gpt->rate_khz * KILO != rate)
> > +		return dev_err_probe(dev, -EINVAL, "rate is not multiple of 1000");
> > +
> > +	ret = reset_control_deassert(rstc);
> 
> Please move reset deassertion directly after
> devm_reset_control_get_exclusive() that it can later be trivially converted to
> devm_reset_control_get_exclusive_deasserted().
> If you base the next revision on top of v6.13-rc1 you can also make use of it already.

Agreed.

> 
> > +	chip->ops = &rzg2l_gpt_ops;
> > +	ret = devm_pwmchip_add(dev, chip);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "failed to add PWM chip\n");
> 
> nitpick: Can you make the error messages all start with a capital letter please?


Agreed. Will fix this.


Cheers,
Biju
Uwe Kleine-König Dec. 5, 2024, 8:28 a.m. UTC | #3
Hello Biju,

On Wed, Dec 04, 2024 at 06:24:19PM +0000, Biju Das wrote:
> > > +	 * different settings.
> > > +	 */
> > > +	if (rzg2l_gpt->user_count[ch] > 1 && period_ticks < rzg2l_gpt->period_ticks[ch])
> > > +		return -EBUSY;
> 
> Do we need to allow this operation (period_ticks < rzg2l_gpt->period_ticks[ch]) ?
> 
> For example,
>    First IO (IO_A) period/duty is in the order nsec (PWM frequency in MHz) and second channel period/duty in the
> order of microsec(PWM frequency in kHz)
> 
> Allowing period_ticks < rzg2l_gpt->period_ticks[ch] will lead to incorrect operations 
> for First IO (IO_A) as PWM frequency will be in kHz compared to MHz.

Well, the policy is to pick the highest possible period not bigger than
the requested period. So if B is asked to be set to 5ms and 5ns is the
highest currently possible value, that's it.

I agree that being off by a factor of 1000 isn't nice. But if you say
this is too much, you have to draw a line somewhere. Where should that
be? Everything you pick is arbitrary and I'm sure there is a use case
for every choice where it's wrong. Additionally if you are too picky --
in the extreme case don't allow any modification >= 1ns -- the API
becomes very hard to use.

The only sensible way out is to allow consumers to query the result
before actually applying a request. This is in the making and if you
want to benefit from it, look at the waveform stuff that recently hit
mainline. (deaba9cff8092cbb2bef4dc79a6ce296017904b1 is an example driver
conversion, 6c5126c6406d1c31e91f5b925c621c1c785366be and
17e40c25158f2505cbcdeda96624afcbab4af368 are the relevant core changes.)
 
> According to me, we should not allow updating periods for second enabled channel.

Not entirely sure you mean the right thing here. If IO A operates at 5ns
and IO B is requested to set .period = 5 ms, every operation that also
changes A is out-of-bounds. So your options are only: Use the 5ns, or
return an error. The latter is hard for consumers because it's unclear
what to do then.

Best regards
Uwe
Geert Uytterhoeven Dec. 5, 2024, 8:52 a.m. UTC | #4
Hi Uwe,

On Thu, Dec 5, 2024 at 9:28 AM Uwe Kleine-König <ukleinek@kernel.org> wrote:
> Well, the policy is to pick the highest possible period not bigger than
> the requested period. So if B is asked to be set to 5ms and 5ns is the
> highest currently possible value, that's it.

Really? So overclocking is preferred over underclocking?

Gr{oetje,eeting}s,

                        Geert
Biju Das Dec. 5, 2024, 9:29 a.m. UTC | #5
Hi Uwe,

> -----Original Message-----
> From: Uwe Kleine-König <ukleinek@kernel.org>
> Sent: 05 December 2024 08:28
> Subject: Re: [PATCH v22 3/4] pwm: Add support for RZ/G2L GPT
> 
> Hello Biju,
> 
> > According to me, we should not allow updating periods for second enabled channel.
> 
> Not entirely sure you mean the right thing here. If IO A operates at 5ns and IO B is requested to
> set .period = 5 ms, every operation that also changes A is out-of-bounds. So your options are only:
> Use the 5ns, or return an error. The latter is hard for consumers because it's unclear what to do
> then.


I guess, these 2 IO's mostly used in switching circuits. So, can't we return error, if period of IO_A != IO_B?
Then the user know the mistake and he will configure with proper values??

For eg:

Case 1:
IO_A= 5msec, IO_B=5nsec

Here IO_B has faster switching(MHz) compared to IO_A (kHZ),
So, by allowing IO_A to operate at IO_B frequency, we are doing
Some incorrect operation for IO_A.

Case 2:
IO_A= 5nsec, IO_B=5msec

Here we are returning error, that is ok.


Cheers,
Biju
Uwe Kleine-König Dec. 5, 2024, 10:31 a.m. UTC | #6
Hello Geert,

On Thu, Dec 05, 2024 at 09:52:28AM +0100, Geert Uytterhoeven wrote:
> On Thu, Dec 5, 2024 at 9:28 AM Uwe Kleine-König <ukleinek@kernel.org> wrote:
> > Well, the policy is to pick the highest possible period not bigger than
> > the requested period. So if B is asked to be set to 5ms and 5ns is the
> > highest currently possible value, that's it.
> 
> Really? So overclocking is preferred over underclocking?

It really depends on your use case if 8ms or 10 ms is better in a
situation where 9 ms is the optimal value.

Note that with the new waveform abstraction there is also a way to round
a waveform W to the waveform W' that would actually be configured if W
was passed to the apply function. That allows a consumer (in the future
probably with the help of some functions provided by the pwm framework)
to determine a request to get the smallest period >= 9 ms.

Best regards
Uwe
Uwe Kleine-König Dec. 5, 2024, 4:26 p.m. UTC | #7
Hello,

On Thu, Dec 05, 2024 at 09:29:35AM +0000, Biju Das wrote:
> > -----Original Message-----
> > From: Uwe Kleine-König <ukleinek@kernel.org>
> > Sent: 05 December 2024 08:28
> > Subject: Re: [PATCH v22 3/4] pwm: Add support for RZ/G2L GPT
> > 
> > Hello Biju,
> > 
> > > According to me, we should not allow updating periods for second enabled channel.
> > 
> > Not entirely sure you mean the right thing here. If IO A operates at 5ns and IO B is requested to
> > set .period = 5 ms, every operation that also changes A is out-of-bounds. So your options are only:
> > Use the 5ns, or return an error. The latter is hard for consumers because it's unclear what to do
> > then.
> 
> 
> I guess, these 2 IO's mostly used in switching circuits. So, can't we return error, if period of IO_A != IO_B?
> Then the user know the mistake and he will configure with proper values??

You can do that. However in general the user (or consumer driver) of IO
A won't know about the connection between the two PWMs and so they only
see their request failing.

> Here IO_B has faster switching(MHz) compared to IO_A (kHZ),
> So, by allowing IO_A to operate at IO_B frequency, we are doing
> Some incorrect operation for IO_A.

"incorrect" is very subjective and depends on the use case. Many drivers
only care about the relative duty cycle. So if they initially want

	.duty_cycle = 1ms
	.period = 5ms

and then learn that only period = 5ns is possible, they are also happy
with

	.duty_cycle = 1ns
	.period = 5ns

Some other drivers are happy with a given duty cycle and hardly care
about the period, so they only care that .duty_cycle is 3ns and both
.period = 5ms and .period = 5ns is ok.

Or a driver that requests

	.duty_cycle = 0ms
	.period = 5ms

doesn't care about the period at all if only .duty_cycle is zero.

So the only way to make everybody happy, is to be able to query the
hardware possibilities.

Best regards
Uwe
Biju Das Dec. 8, 2024, 5:38 p.m. UTC | #8
Hi Uwe Kleine-König,

> -----Original Message-----
> From: Uwe Kleine-König <ukleinek@kernel.org>
> Sent: 05 December 2024 16:27
> Subject: Re: [PATCH v22 3/4] pwm: Add support for RZ/G2L GPT
> 
> Hello,
> 
> On Thu, Dec 05, 2024 at 09:29:35AM +0000, Biju Das wrote:
> > > -----Original Message-----
> > > From: Uwe Kleine-König <ukleinek@kernel.org>
> > > Sent: 05 December 2024 08:28
> > > Subject: Re: [PATCH v22 3/4] pwm: Add support for RZ/G2L GPT
> > >
> > > Hello Biju,
> > >
> > > > According to me, we should not allow updating periods for second enabled channel.
> > >
> > > Not entirely sure you mean the right thing here. If IO A operates at
> > > 5ns and IO B is requested to set .period = 5 ms, every operation that also changes A is out-of-
> bounds. So your options are only:
> > > Use the 5ns, or return an error. The latter is hard for consumers
> > > because it's unclear what to do then.
> >
> >
> > I guess, these 2 IO's mostly used in switching circuits. So, can't we return error, if period of
> IO_A != IO_B?
> > Then the user know the mistake and he will configure with proper values??
> 
> You can do that. However in general the user (or consumer driver) of IO A won't know about the
> connection between the two PWMs and so they only see their request failing.

Ok, thanks. This will make the driver simpler for the initial version.

> 
> > Here IO_B has faster switching(MHz) compared to IO_A (kHZ), So, by
> > allowing IO_A to operate at IO_B frequency, we are doing Some
> > incorrect operation for IO_A.
> 
> "incorrect" is very subjective and depends on the use case. Many drivers only care about the relative

Sorry for my bad english.

> duty cycle. So if they initially want
> 
> 	.duty_cycle = 1ms
> 	.period = 5ms
> 
> and then learn that only period = 5ns is possible, they are also happy with
> 
> 	.duty_cycle = 1ns
> 	.period = 5ns
> 
> Some other drivers are happy with a given duty cycle and hardly care about the period, so they only
> care that .duty_cycle is 3ns and both .period = 5ms and .period = 5ns is ok.
> 
> Or a driver that requests
> 
> 	.duty_cycle = 0ms
> 	.period = 5ms
> 
> doesn't care about the period at all if only .duty_cycle is zero.
> 
> So the only way to make everybody happy, is to be able to query the hardware possibilities.

I got your point a duty cycle of 50% can be achieved at period/duty configured at ms or in nsec.

The former being aimed for low frequency PWM application and later for higher frequency
application and the period in ms or nsec are purely application specific.

If you allow me, Shall I send simplified version of the driver? ie, will return
error if requested_period of IO_A != IO_B.


Later we can accommodate adapting duty cycle of first channel, if we allow changing
periods from second channel, based on any customer use case. There are some customers
using GPT for backlight in LCD panels.

Please let me know.

Going forward there are lot of features are planned for this driver
1) inverse feature
2) Detecting short circuits between IO's
3) Phase counting
4) Input capture
5) PWM using buffer
6) DMA operation
7) ADC start trigger

Cheers,
Biju
diff mbox series

Patch

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 0915c1e7df16..38fc77a9ed16 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -540,6 +540,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_RZG2L || 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_RZ_MTU3
 	tristate "Renesas RZ/G2L MTU3a PWM Timer support"
 	depends on RZ_MTU3
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 9081e0c0e9e0..39c029dd3af9 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -49,6 +49,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_RZ_MTU3)	+= pwm-rz-mtu3.o
 obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung.o
 obj-$(CONFIG_PWM_SIFIVE)	+= pwm-sifive.o
diff --git a/drivers/pwm/pwm-rzg2l-gpt.c b/drivers/pwm/pwm-rzg2l-gpt.c
new file mode 100644
index 000000000000..28ed39eecb93
--- /dev/null
+++ b/drivers/pwm/pwm-rzg2l-gpt.c
@@ -0,0 +1,473 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Renesas RZ/G2L General PWM Timer (GPT) driver
+ *
+ * Copyright (C) 2024 Renesas Electronics Corporation
+ *
+ * Hardware manual for this IP can be found here
+ * https://www.renesas.com/eu/en/document/mah/rzg2l-group-rzg2lc-group-users-manual-hardware-0?language=en
+ *
+ * Limitations:
+ * - Counter must be stopped before modifying Mode and Prescaler.
+ * - When PWM is disabled, the output is driven to inactive.
+ * - While the hardware supports both polarities, the driver (for now)
+ *   only handles normal polarity.
+ * - General PWM Timer (GPT) has 8 HW channels for PWM operations and
+ *   each HW channel have 2 IOs.
+ * - Each IO is modelled as an independent PWM channel.
+ * - When both channels are used, disabling the channel on one stops the
+ *   other.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/limits.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/reset.h>
+#include <linux/time.h>
+#include <linux/units.h>
+
+#define RZG2L_GET_CH(a)		((a) / 2)
+#define RZG2L_GET_CH_OFFS(i)	(0x100 * (i))
+
+#define RZG2L_GTCR(ch)		(0x2c + RZG2L_GET_CH_OFFS(ch))
+#define RZG2L_GTUDDTYC(ch)	(0x30 + RZG2L_GET_CH_OFFS(ch))
+#define RZG2L_GTIOR(ch)		(0x34 + RZG2L_GET_CH_OFFS(ch))
+#define RZG2L_GTBER(ch)		(0x40 + RZG2L_GET_CH_OFFS(ch))
+#define RZG2L_GTCNT(ch)		(0x48 + RZG2L_GET_CH_OFFS(ch))
+#define RZG2L_GTCCR(ch, sub_ch)	(0x4c + RZG2L_GET_CH_OFFS(ch) + 4 * (sub_ch))
+#define RZG2L_GTPR(ch)		(0x64 + RZG2L_GET_CH_OFFS(ch))
+
+#define RZG2L_GTCR_CST		BIT(0)
+#define RZG2L_GTCR_MD		GENMASK(18, 16)
+#define RZG2L_GTCR_TPCS		GENMASK(26, 24)
+
+#define RZG2L_GTCR_MD_SAW_WAVE_PWM_MODE	FIELD_PREP(RZG2L_GTCR_MD, 0)
+
+#define RZG2L_GTUDDTYC_UP	BIT(0)
+#define RZG2L_GTUDDTYC_UDF	BIT(1)
+#define RZG2L_GTUDDTYC_UP_COUNTING	(RZG2L_GTUDDTYC_UP | RZG2L_GTUDDTYC_UDF)
+
+#define RZG2L_GTIOR_GTIOA	GENMASK(4, 0)
+#define RZG2L_GTIOR_GTIOB	GENMASK(20, 16)
+#define RZG2L_GTIOR_GTIOx(a)	((a) ? RZG2L_GTIOR_GTIOB : RZG2L_GTIOR_GTIOA)
+#define RZG2L_GTIOR_OAE		BIT(8)
+#define RZG2L_GTIOR_OBE		BIT(24)
+#define RZG2L_GTIOR_OxE(a)	((a) ? RZG2L_GTIOR_OBE : RZG2L_GTIOR_OAE)
+
+#define RZG2L_INIT_OUT_HI_OUT_HI_END_TOGGLE	0x1b
+#define RZG2L_GTIOR_GTIOA_OUT_HI_END_TOGGLE_CMP_MATCH \
+	(RZG2L_INIT_OUT_HI_OUT_HI_END_TOGGLE | RZG2L_GTIOR_OAE)
+#define RZG2L_GTIOR_GTIOB_OUT_HI_END_TOGGLE_CMP_MATCH \
+	(FIELD_PREP(RZG2L_GTIOR_GTIOB, RZG2L_INIT_OUT_HI_OUT_HI_END_TOGGLE) | RZG2L_GTIOR_OBE)
+
+#define RZG2L_GTIOR_GTIOx_OUT_HI_END_TOGGLE_CMP_MATCH(a) \
+	((a) ? RZG2L_GTIOR_GTIOB_OUT_HI_END_TOGGLE_CMP_MATCH : \
+	 RZG2L_GTIOR_GTIOA_OUT_HI_END_TOGGLE_CMP_MATCH)
+
+#define RZG2L_MAX_HW_CHANNELS	8
+#define RZG2L_CHANNELS_PER_IO	2
+#define RZG2L_MAX_PWM_CHANNELS	(RZG2L_MAX_HW_CHANNELS * RZG2L_CHANNELS_PER_IO)
+#define RZG2L_MAX_SCALE_FACTOR	1024
+#define RZG2L_MAX_TICKS ((u64)U32_MAX * RZG2L_MAX_SCALE_FACTOR)
+
+struct rzg2l_gpt_chip {
+	void __iomem *mmio;
+	struct mutex lock; /* lock to protect shared channel resources */
+	unsigned long rate_khz;
+	u32 period_ticks[RZG2L_MAX_HW_CHANNELS];
+	u32 user_count[RZG2L_MAX_HW_CHANNELS];
+	u32 enable_count[RZG2L_MAX_HW_CHANNELS];
+};
+
+static inline struct rzg2l_gpt_chip *to_rzg2l_gpt_chip(struct pwm_chip *chip)
+{
+	return pwmchip_get_drvdata(chip);
+}
+
+static inline unsigned int rzg2l_gpt_subchannel(unsigned int hwpwm)
+{
+	return hwpwm & 0x1;
+}
+
+static void rzg2l_gpt_write(struct rzg2l_gpt_chip *rzg2l_gpt, u32 reg, u32 data)
+{
+	writel(data, rzg2l_gpt->mmio + reg);
+}
+
+static u32 rzg2l_gpt_read(struct rzg2l_gpt_chip *rzg2l_gpt, u32 reg)
+{
+	return readl(rzg2l_gpt->mmio + reg);
+}
+
+static void rzg2l_gpt_modify(struct rzg2l_gpt_chip *rzg2l_gpt, u32 reg, u32 clr,
+			     u32 set)
+{
+	rzg2l_gpt_write(rzg2l_gpt, reg,
+			(rzg2l_gpt_read(rzg2l_gpt, reg) & ~clr) | set);
+}
+
+static u8 rzg2l_gpt_calculate_prescale(struct rzg2l_gpt_chip *rzg2l_gpt,
+				       u64 period_ticks)
+{
+	u32 prescaled_period_ticks;
+	u8 prescale;
+
+	prescaled_period_ticks = period_ticks >> 32;
+	if (prescaled_period_ticks >= 256)
+		prescale = 5;
+	else
+		prescale = (fls(prescaled_period_ticks) + 1) / 2;
+
+	return prescale;
+}
+
+static int rzg2l_gpt_request(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct rzg2l_gpt_chip *rzg2l_gpt = to_rzg2l_gpt_chip(chip);
+	u32 ch = RZG2L_GET_CH(pwm->hwpwm);
+
+	guard(mutex)(&rzg2l_gpt->lock);
+	rzg2l_gpt->user_count[ch]++;
+
+	return 0;
+}
+
+static void rzg2l_gpt_free(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct rzg2l_gpt_chip *rzg2l_gpt = to_rzg2l_gpt_chip(chip);
+	u32 ch = RZG2L_GET_CH(pwm->hwpwm);
+
+	guard(mutex)(&rzg2l_gpt->lock);
+	rzg2l_gpt->user_count[ch]--;
+}
+
+static bool rzg2l_gpt_is_ch_enabled(struct rzg2l_gpt_chip *rzg2l_gpt, u8 hwpwm)
+{
+	u8 ch = RZG2L_GET_CH(hwpwm);
+	u32 val;
+
+	val = rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTCR(ch));
+	if (!(val & RZG2L_GTCR_CST))
+		return false;
+
+	val = rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTIOR(ch));
+
+	return val & RZG2L_GTIOR_OxE(rzg2l_gpt_subchannel(hwpwm));
+}
+
+/* Caller holds the lock while calling rzg2l_gpt_enable() */
+static void rzg2l_gpt_enable(struct rzg2l_gpt_chip *rzg2l_gpt,
+			     struct pwm_device *pwm)
+{
+	u8 sub_ch = rzg2l_gpt_subchannel(pwm->hwpwm);
+	u32 val = RZG2L_GTIOR_GTIOx(sub_ch) | RZG2L_GTIOR_OxE(sub_ch);
+	u8 ch = RZG2L_GET_CH(pwm->hwpwm);
+
+	/* Enable pin output */
+	rzg2l_gpt_modify(rzg2l_gpt, RZG2L_GTIOR(ch), val,
+			 RZG2L_GTIOR_GTIOx_OUT_HI_END_TOGGLE_CMP_MATCH(sub_ch));
+
+	if (!rzg2l_gpt->enable_count[ch])
+		rzg2l_gpt_modify(rzg2l_gpt, RZG2L_GTCR(ch), 0, RZG2L_GTCR_CST);
+
+	rzg2l_gpt->enable_count[ch]++;
+}
+
+/* Caller holds the lock while calling rzg2l_gpt_disable() */
+static void rzg2l_gpt_disable(struct rzg2l_gpt_chip *rzg2l_gpt,
+			      struct pwm_device *pwm)
+{
+	u8 sub_ch = rzg2l_gpt_subchannel(pwm->hwpwm);
+	u8 ch = RZG2L_GET_CH(pwm->hwpwm);
+
+	/* Stop count, Output low on GTIOCx pin when counting stops */
+	rzg2l_gpt->enable_count[ch]--;
+
+	if (!rzg2l_gpt->enable_count[ch])
+		rzg2l_gpt_modify(rzg2l_gpt, RZG2L_GTCR(ch), RZG2L_GTCR_CST, 0);
+
+	/* Disable pin output */
+	rzg2l_gpt_modify(rzg2l_gpt, RZG2L_GTIOR(ch), RZG2L_GTIOR_OxE(sub_ch), 0);
+}
+
+static u64 calculate_period_or_duty(struct rzg2l_gpt_chip *rzg2l_gpt, u32 val, u8 prescale)
+{
+	u64 tmp;
+
+	/*
+	 * The calculation doesn't overflow an u64 because prescale ≤ 5 and so
+	 * tmp = val << (2 * prescale) * USEC_PER_SEC
+	 *     < 2^32 * 2^10 * 10^6
+	 *     < 2^32 * 2^10 * 2^20
+	 *     = 2^62
+	 */
+	tmp = (u64)val << (2 * prescale);
+	tmp *= USEC_PER_SEC;
+
+	return DIV64_U64_ROUND_UP(tmp, rzg2l_gpt->rate_khz);
+}
+
+static int rzg2l_gpt_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
+			       struct pwm_state *state)
+{
+	struct rzg2l_gpt_chip *rzg2l_gpt = to_rzg2l_gpt_chip(chip);
+
+	state->enabled = rzg2l_gpt_is_ch_enabled(rzg2l_gpt, pwm->hwpwm);
+	if (state->enabled) {
+		u32 sub_ch = rzg2l_gpt_subchannel(pwm->hwpwm);
+		u32 ch = RZG2L_GET_CH(pwm->hwpwm);
+		u8 prescale;
+		u32 val;
+
+		val = rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTCR(ch));
+		prescale = FIELD_GET(RZG2L_GTCR_TPCS, val);
+
+		val = rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTPR(ch));
+		state->period = calculate_period_or_duty(rzg2l_gpt, val, prescale);
+
+		val = rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTCCR(ch, sub_ch));
+		state->duty_cycle = calculate_period_or_duty(rzg2l_gpt, val, prescale);
+		if (state->duty_cycle > state->period)
+			state->duty_cycle = state->period;
+	}
+
+	state->polarity = PWM_POLARITY_NORMAL;
+
+	return 0;
+}
+
+static u32 rzg2l_gpt_calculate_pv_or_dc(u64 period_or_duty_cycle, u8 prescale)
+{
+	return min_t(u64, DIV_ROUND_DOWN_ULL(period_or_duty_cycle, 1 << (2 * prescale)),
+		     U32_MAX);
+}
+
+/* Caller holds the lock while calling rzg2l_gpt_config() */
+static int rzg2l_gpt_config(struct pwm_chip *chip, struct pwm_device *pwm,
+			    const struct pwm_state *state)
+{
+	struct rzg2l_gpt_chip *rzg2l_gpt = to_rzg2l_gpt_chip(chip);
+	u8 sub_ch = rzg2l_gpt_subchannel(pwm->hwpwm);
+	u8 ch = RZG2L_GET_CH(pwm->hwpwm);
+	u64 period_ticks, duty_ticks;
+	unsigned long pv, dc;
+	u8 prescale;
+
+	/* Limit period/duty cycle to max value supported by the HW */
+	period_ticks = mul_u64_u64_div_u64(state->period, rzg2l_gpt->rate_khz, USEC_PER_SEC);
+	if (period_ticks > RZG2L_MAX_TICKS)
+		period_ticks = RZG2L_MAX_TICKS;
+	/*
+	 * GPT counter is shared by multiple channels, so prescale and period
+	 * can NOT be modified when there are multiple channels in use with
+	 * different settings.
+	 */
+	if (rzg2l_gpt->user_count[ch] > 1 && period_ticks < rzg2l_gpt->period_ticks[ch])
+		return -EBUSY;
+
+	prescale = rzg2l_gpt_calculate_prescale(rzg2l_gpt, period_ticks);
+	pv = rzg2l_gpt_calculate_pv_or_dc(period_ticks, prescale);
+
+	duty_ticks = mul_u64_u64_div_u64(state->duty_cycle, rzg2l_gpt->rate_khz, USEC_PER_SEC);
+	if (duty_ticks > RZG2L_MAX_TICKS)
+		duty_ticks = RZG2L_MAX_TICKS;
+	dc = rzg2l_gpt_calculate_pv_or_dc(duty_ticks, prescale);
+
+	/*
+	 * GPT counter is shared by multiple channels, we cache the period ticks
+	 * from the first enabled channel and use the same value for both
+	 * channels.
+	 */
+	rzg2l_gpt->period_ticks[ch] = period_ticks;
+
+	/*
+	 * Counter must be stopped before modifying mode, prescaler, timer
+	 * counter and buffer enable registers. These registers are shared
+	 * between both channels. So allow updating these registers only for the
+	 * first enabled channel.
+	 */
+	if (rzg2l_gpt->enable_count[ch] <= 1) {
+		rzg2l_gpt_modify(rzg2l_gpt, RZG2L_GTCR(ch), RZG2L_GTCR_CST, 0);
+
+		/* GPT set operating mode (saw-wave up-counting) */
+		rzg2l_gpt_modify(rzg2l_gpt, RZG2L_GTCR(ch), RZG2L_GTCR_MD,
+				 RZG2L_GTCR_MD_SAW_WAVE_PWM_MODE);
+
+		/* Set count direction */
+		rzg2l_gpt_write(rzg2l_gpt, RZG2L_GTUDDTYC(ch), RZG2L_GTUDDTYC_UP_COUNTING);
+
+		/* Select count clock */
+		rzg2l_gpt_modify(rzg2l_gpt, RZG2L_GTCR(ch), RZG2L_GTCR_TPCS,
+				 FIELD_PREP(RZG2L_GTCR_TPCS, prescale));
+
+		/* Set period */
+		rzg2l_gpt_write(rzg2l_gpt, RZG2L_GTPR(ch), pv);
+	}
+
+	/* Set duty cycle */
+	rzg2l_gpt_write(rzg2l_gpt, RZG2L_GTCCR(ch, sub_ch), dc);
+
+	if (rzg2l_gpt->enable_count[ch] <= 1) {
+		/* Set initial value for counter */
+		rzg2l_gpt_write(rzg2l_gpt, RZG2L_GTCNT(ch), 0);
+
+		/* Set no buffer operation */
+		rzg2l_gpt_write(rzg2l_gpt, RZG2L_GTBER(ch), 0);
+
+		/* Restart the counter after updating the registers */
+		rzg2l_gpt_modify(rzg2l_gpt, RZG2L_GTCR(ch),
+				 RZG2L_GTCR_CST, RZG2L_GTCR_CST);
+	}
+
+	return 0;
+}
+
+static int rzg2l_gpt_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+			   const struct pwm_state *state)
+{
+	struct rzg2l_gpt_chip *rzg2l_gpt = to_rzg2l_gpt_chip(chip);
+	bool enabled = pwm->state.enabled;
+	int ret;
+
+	if (state->polarity != PWM_POLARITY_NORMAL)
+		return -EINVAL;
+
+	guard(mutex)(&rzg2l_gpt->lock);
+	if (!state->enabled) {
+		if (enabled)
+			rzg2l_gpt_disable(rzg2l_gpt, pwm);
+
+		return 0;
+	}
+
+	ret = rzg2l_gpt_config(chip, pwm, state);
+	if (!ret && !enabled)
+		rzg2l_gpt_enable(rzg2l_gpt, pwm);
+
+	return ret;
+}
+
+static const struct pwm_ops rzg2l_gpt_ops = {
+	.request = rzg2l_gpt_request,
+	.free = rzg2l_gpt_free,
+	.get_state = rzg2l_gpt_get_state,
+	.apply = rzg2l_gpt_apply,
+};
+
+static void rzg2l_gpt_reset_assert(void *data)
+{
+	reset_control_assert(data);
+}
+
+static int rzg2l_gpt_probe(struct platform_device *pdev)
+{
+	struct rzg2l_gpt_chip *rzg2l_gpt;
+	struct device *dev = &pdev->dev;
+	struct reset_control *rstc;
+	struct pwm_chip *chip;
+	unsigned long rate;
+	struct clk *clk;
+	int ret;
+	u32 i;
+
+	chip = devm_pwmchip_alloc(dev, RZG2L_MAX_PWM_CHANNELS, sizeof(*rzg2l_gpt));
+	if (IS_ERR(chip))
+		return PTR_ERR(chip);
+	rzg2l_gpt = to_rzg2l_gpt_chip(chip);
+
+	rzg2l_gpt->mmio = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(rzg2l_gpt->mmio))
+		return PTR_ERR(rzg2l_gpt->mmio);
+
+	rstc = devm_reset_control_get_exclusive(dev, NULL);
+	if (IS_ERR(rstc))
+		return dev_err_probe(dev, PTR_ERR(rstc), "get reset failed\n");
+
+	clk = devm_clk_get_enabled(dev, NULL);
+	if (IS_ERR(clk))
+		return dev_err_probe(dev, PTR_ERR(clk), "cannot get clock\n");
+
+	ret = devm_clk_rate_exclusive_get(dev, clk);
+	if (ret)
+		return ret;
+
+	rate = clk_get_rate(clk);
+	if (!rate)
+		return dev_err_probe(dev, -EINVAL, "gpt clk rate is 0");
+
+	/*
+	 * Refuse clk rates > 1 GHz to prevent overflow later for computing
+	 * period and duty cycle.
+	 */
+	if (rate > NSEC_PER_SEC)
+		return dev_err_probe(dev, -EINVAL, "gpt clk rate is > 1GHz");
+
+	/*
+	 * Rate is in MHz and is always integer for peripheral clk
+	 * 2^32 * 2^10 (prescalar) * 10^6 (rate_khz) < 2^64
+	 * So make sure rate is multiple of 1000.
+	 */
+	rzg2l_gpt->rate_khz = rate / KILO;
+	if (rzg2l_gpt->rate_khz * KILO != rate)
+		return dev_err_probe(dev, -EINVAL, "rate is not multiple of 1000");
+
+	ret = reset_control_deassert(rstc);
+	if (ret)
+		return dev_err_probe(dev, ret, "cannot deassert reset control\n");
+
+	ret = devm_add_action_or_reset(dev, rzg2l_gpt_reset_assert, rstc);
+	if (ret)
+		return ret;
+
+	/*
+	 * We need to keep the clock on, in case the bootloader has enabled the
+	 * PWM and is running during probe().
+	 */
+	for (i = 0; i < RZG2L_MAX_PWM_CHANNELS; i++) {
+		if (rzg2l_gpt_is_ch_enabled(rzg2l_gpt, i)) {
+			u8 ch = RZG2L_GET_CH(i);
+			u8 prescale;
+			u32 val;
+
+			val = rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTCR(ch));
+			prescale = FIELD_GET(RZG2L_GTCR_TPCS, val);
+			if (prescale > 5)
+				dev_warn_once(dev, "Invalid prescale set %d > 5", prescale);
+
+			rzg2l_gpt->enable_count[ch]++;
+		}
+	}
+
+	mutex_init(&rzg2l_gpt->lock);
+
+	chip->ops = &rzg2l_gpt_ops;
+	ret = devm_pwmchip_add(dev, chip);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to add PWM chip\n");
+
+	return 0;
+}
+
+static const struct of_device_id rzg2l_gpt_of_table[] = {
+	{ .compatible = "renesas,rzg2l-gpt", },
+	{ /* Sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, rzg2l_gpt_of_table);
+
+static struct platform_driver rzg2l_gpt_driver = {
+	.driver = {
+		.name = "pwm-rzg2l-gpt",
+		.of_match_table = rzg2l_gpt_of_table,
+	},
+	.probe = rzg2l_gpt_probe,
+};
+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");