Hi Uwe,
Thanks for the feedback.
> -----Original Message-----
> From: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxx>
> Sent: Tuesday, July 30, 2024 8:54 PM
> Subject: Re: [PATCH v20 3/4] pwm: Add support for RZ/G2L GPT
>
> Hello,
>
> I'm a bit unlucky about this driver. I have the impression it is complicated and wonder if that is
> necessary because the hardware is unusual or if we just have to spot some simplifications.
I agree it is little bit complex driver. Once this driver is accepted, going forward, I need to support
other drivers like (Counter , ADC triggering and POEG(Output disable) support).
> I guess another problem is that the time between two consecutive reviews is long and I forget most
> things I learned about the hardware from one to the other. While this is mostly my problem, the same
> problem arises if the driver is touched later again. So I wonder if some more documentation is needed
> about the relation between channels and outputs and subchannels. If the driver only supported one
> output per channel, it could be considerably simpler (I think). But I guess that would be a
> practically relevant restriction??
Yes, one output per channel means, we cannot use POEG IP which is for short circuit protection in
switching circuits. So, we need to use both IOs in the channel.
I will add the below documentation to make it clear.
* - 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.
Please let me know is it ok with respect to the initial driver?
>
> Some simplifications spotted below.
>
> On Fri, Jun 14, 2024 at 04:42:41PM +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 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@xxxxxxxxxxxxxx>
> > ---
> > 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()
> > [...]
> > ---
> > drivers/pwm/Kconfig | 11 +
> > drivers/pwm/Makefile | 1 +
> > drivers/pwm/pwm-rzg2l-gpt.c | 555
> > ++++++++++++++++++++++++++++++++++++
> > 3 files changed, 567 insertions(+)
> > create mode 100644 drivers/pwm/pwm-rzg2l-gpt.c
> >
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index
> > 00a543de8f82..3d398b308e3f 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -522,6 +522,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
> > 6964ba45c795..fb9a2d9b9adb 100644
> > --- a/drivers/pwm/Makefile
> > +++ b/drivers/pwm/Makefile
> > @@ -47,6 +47,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..6005a689173e
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-rzg2l-gpt.c
> > @@ -0,0 +1,555 @@
> > +// 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.
> > + * - 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/pm_runtime.h>
> > +#include <linux/pwm.h>
> > +#include <linux/reset.h>
> > +#include <linux/time.h>
> > +#include <linux/units.h>
> > +
> > +#define RZG2L_GTCR 0x2c
> > +#define RZG2L_GTUDDTYC 0x30
> > +#define RZG2L_GTIOR 0x34
> > +#define RZG2L_GTBER 0x40
> > +#define RZG2L_GTCNT 0x48
> > +#define RZG2L_GTCCR(i) (0x4c + 4 * (i))
> > +#define RZG2L_GTPR 0x64
These will be replaced as
+#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_GTINTAD(ch) (0x38 + 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_GET_CH(a) ((a) / 2)
> > +
> > +#define RZG2L_GET_CH_OFFS(i) (0x100 * (i))
> > +
> > +struct rzg2l_gpt_chip {
> > + void __iomem *mmio;
> > + struct reset_control *rstc;
> > + struct mutex lock; /* lock to protect shared channel resources */
> > + unsigned long rate_khz;
> > + u64 max_val;
> > + u32 period_cycles[RZG2L_MAX_HW_CHANNELS];
> > + u32 user_count[RZG2L_MAX_HW_CHANNELS];
> > + u32 enable_count[RZG2L_MAX_HW_CHANNELS];
> > + u8 bootloader_enabled_channels[RZG2L_MAX_PWM_CHANNELS];
>
> Semantically bootloader_enabled_channels[x] is a bool, right?
OK will change it to bool.
>
> > +};
> > +
> > +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_cycles)
> > +{
> > + u32 prescaled_period_cycles;
> > + u8 prescale;
> > +
> > + prescaled_period_cycles = period_cycles >> 32;
> > + if (prescaled_period_cycles >= 256)
> > + prescale = 5;
> > + else
> > + prescale = (fls(prescaled_period_cycles) + 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);
> > +
> > + mutex_lock(&rzg2l_gpt->lock);
> > + rzg2l_gpt->user_count[ch]++;
> > + mutex_unlock(&rzg2l_gpt->lock);
> > +
> > + 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);
> > +
> > + mutex_lock(&rzg2l_gpt->lock);
> > + rzg2l_gpt->user_count[ch]--;
> > + mutex_unlock(&rzg2l_gpt->lock);
> > +}
> > +
> > +static bool rzg2l_gpt_is_ch_enabled(struct rzg2l_gpt_chip *rzg2l_gpt,
> > +u8 hwpwm) {
> > + u8 ch = RZG2L_GET_CH(hwpwm);
> > + u32 offs = RZG2L_GET_CH_OFFS(ch);
> > + u32 val;
> > +
> > + val = rzg2l_gpt_read(rzg2l_gpt, offs + RZG2L_GTCR);
> > + if (!(val & RZG2L_GTCR_CST))
> > + return false;
> > +
> > + val = rzg2l_gpt_read(rzg2l_gpt, offs + RZG2L_GTIOR);
> > +
> > + return val & RZG2L_GTIOR_OxE(rzg2l_gpt_subchannel(hwpwm));
> > +}
> > +
> > +static int 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);
> > + u32 offs = RZG2L_GET_CH_OFFS(ch);
> > +
> > + mutex_lock(&rzg2l_gpt->lock);
> > + /* Enable pin output */
> > + rzg2l_gpt_modify(rzg2l_gpt, offs + RZG2L_GTIOR, val,
> > + RZG2L_GTIOR_GTIOx_OUT_HI_END_TOGGLE_CMP_MATCH(sub_ch));
> > +
> > + if (!rzg2l_gpt->enable_count[ch])
> > + rzg2l_gpt_modify(rzg2l_gpt, offs + RZG2L_GTCR, 0, RZG2L_GTCR_CST);
> > +
> > + rzg2l_gpt->enable_count[ch]++;
> > + mutex_unlock(&rzg2l_gpt->lock);
> > +
> > + return 0;
> > +}
> > +
> > +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);
> > + u32 offs = RZG2L_GET_CH_OFFS(ch);
> > +
> > + /* Stop count, Output low on GTIOCx pin when counting stops */
> > + mutex_lock(&rzg2l_gpt->lock);
> > + rzg2l_gpt->enable_count[ch]--;
> > +
> > + if (!rzg2l_gpt->enable_count[ch])
> > + rzg2l_gpt_modify(rzg2l_gpt, offs + RZG2L_GTCR, RZG2L_GTCR_CST, 0);
> > +
> > + /* Disable pin output */
> > + rzg2l_gpt_modify(rzg2l_gpt, offs + RZG2L_GTIOR, RZG2L_GTIOR_OxE(sub_ch), 0);
> > + mutex_unlock(&rzg2l_gpt->lock);
> > +}
> > +
> > +static u64 calculate_period_or_duty(struct rzg2l_gpt_chip *rzg2l_gpt,
> > +u32 val, u8 prescale) {
> > + u64 tmp;
> > +
> > + /*
> > + * This cannot overflow because,
> > + * 2^32 * 2^10 (prescalar) * 10^6 (rate_khz) < 2^64
>
> I find the notation here hard to parse. I personally would prefer:
>
> /*
> * 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
> */
>
> Is it only me?
Agreed, will change like this as it is clear to every one.
>
> > + */
> > + 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);
> > + int rc;
> > +
> > + rc = pm_runtime_resume_and_get(pwmchip_parent(chip));
> > + if (rc)
> > + return rc;
> > +
> > + state->enabled = rzg2l_gpt_is_ch_enabled(rzg2l_gpt, pwm->hwpwm);
> > + if (state->enabled) {
> > + u32 ch = RZG2L_GET_CH(pwm->hwpwm);
> > + u32 offs = RZG2L_GET_CH_OFFS(ch);
> > + u8 prescale;
> > + u32 val;
> > +
> > + val = rzg2l_gpt_read(rzg2l_gpt, offs + RZG2L_GTCR);
> > + prescale = FIELD_GET(RZG2L_GTCR_TPCS, val);
>
> Can it happen that prescale is > 5 here?
Yes, if bootloader wrongly set it to 6 or 7. I will add a check in
Probe and forcefully set to 5, if that the case. Is it ok?
+ if (prescale > 5) {
+ dev_warn(dev, "Invalid prescale %d > 5, force setting to 5", prescale);
+ /* Set prescale value of 5. */
+ rzg2l_gpt_modify(rzg2l_gpt, RZG2L_GTCR(ch), RZG2L_GTCR_TPCS,
+ FIELD_PREP(RZG2L_GTCR_TPCS, 5));
+ }
>
> > + val = rzg2l_gpt_read(rzg2l_gpt, offs + RZG2L_GTPR);
> > + state->period = calculate_period_or_duty(rzg2l_gpt, val, prescale);
> > +
> > + val = rzg2l_gpt_read(rzg2l_gpt,
> > + offs + RZG2L_GTCCR(rzg2l_gpt_subchannel(pwm->hwpwm)));
> > + 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;
> > + pm_runtime_put(pwmchip_parent(chip));
> > +
> > + return 0;
> > +}
> > +
> > +static u32 rzg2l_gpt_calculate_pv_or_dc(u64 period_or_duty_cycle, u8
> > +prescale) {
> > + return min_t(u64, (period_or_duty_cycle + (1 << (2 * prescale)) - 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);
> > + u64 period, duty_cycle, period_cycles, duty_cycles;
> > + u8 ch = RZG2L_GET_CH(pwm->hwpwm);
> > + u32 offs = RZG2L_GET_CH_OFFS(ch);
> > + unsigned long pv, dc;
> > + u8 prescale;
> > +
> > + /* Limit period/duty cycle to max value supported by the HW */
> > + period = min(state->period, rzg2l_gpt->max_val);
> > + period_cycles = mul_u64_u64_div_u64(period, rzg2l_gpt->rate_khz,
> > +USEC_PER_SEC);
> > +
> > + /*
> > + * 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_cycles < rzg2l_gpt->period_cycles[ch])
> > + return -EBUSY;
> > +
> > + prescale = rzg2l_gpt_calculate_prescale(rzg2l_gpt, period_cycles);
> > + pv = rzg2l_gpt_calculate_pv_or_dc(period_cycles, prescale);
> > +
> > + duty_cycle = min(state->duty_cycle, rzg2l_gpt->max_val);
> > + duty_cycles = mul_u64_u64_div_u64(duty_cycle, rzg2l_gpt->rate_khz,
> > +USEC_PER_SEC);
>
> These two variable names are too similar. One is in nano seconds, the other in hw ticks. Maybe rename
> the latter to duty_ticks, or duty_count.
OK. Will change period_cycles->period_ticks as well.
>
> > + dc = rzg2l_gpt_calculate_pv_or_dc(duty_cycles, prescale);
>
> If you use mul_u64_u64_div_u64() anyhow you can implement the check if the value is too big later,
> that is:
>
> #define RZG2L_MAX_TICKS ((u64)U32_MAX * RZG2L_MAX_SCALE_FACTOR) /* is this right? */
Yes.
> ...
> 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;
>
> Then you could get rid of .max_val I think.
Agreed, it can be dropped.
>
> > + /*
> > + * GPT counter is shared by multiple channels, we cache the period cycles
> > + * from the first enabled channel and use the same value for both
> > + * channels.
> > + */
> > + rzg2l_gpt->period_cycles[ch] = period_cycles;
> > +
> > + /*
> > + * 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, offs + RZG2L_GTCR, RZG2L_GTCR_CST, 0);
>
> Maybe instead of using the local variable offs introduce a helper à la:
>
> static void rzg2l_gpt_ch_modify(struct rzg2l_gpt_chip *rzg2l_gpt, u8 ch, u32 reg, u32 clr, u32
> set)
> {
> u32 offs = RZG2L_GET_CH_OFFS(ch);
>
> rrzg2l_gpt_modify(rzg2l_gpt, offs + reg, clr, set);
> }
>
> or define RZG2L_GTCR as:
>
> #define RZG2L_GTCR(ch) (RZG2L_GET_CH_OFFS(ch) + 0x2c)
>
> . I think I like the latter better.
OK.
>
> For RZG2L_GTCCR (which depends on channel and subchannel) maybe pass hwpwm as macro parameter. (It's a
> bit ugly to have different parameters for different register offsets, but passing hwpwm if only the
> channel matters is also strange. Hmm, unsure; your chance to have a strong opinion :-)
#define RZG2L_GTCCR(ch, sub_ch) (0x4c + RZG2L_GET_CH_OFFS(ch) + 4 * (sub_ch))
I will use this macro here.
>
> > + /* GPT set operating mode (saw-wave up-counting) */
> > + rzg2l_gpt_modify(rzg2l_gpt, offs + RZG2L_GTCR, RZG2L_GTCR_MD,
> > + RZG2L_GTCR_MD_SAW_WAVE_PWM_MODE);
> > +
> > + /* Set count direction */
> > + rzg2l_gpt_write(rzg2l_gpt, offs + RZG2L_GTUDDTYC, RZG2L_GTUDDTYC);
> > +
> > + /* Select count clock */
> > + rzg2l_gpt_modify(rzg2l_gpt, offs + RZG2L_GTCR, RZG2L_GTCR_TPCS,
> > + FIELD_PREP(RZG2L_GTCR_TPCS, prescale));
> > +
> > + /* Set period */
> > + rzg2l_gpt_write(rzg2l_gpt, offs + RZG2L_GTPR, pv);
> > + }
> > +
> > + /* Set duty cycle */
> > + rzg2l_gpt_write(rzg2l_gpt, offs + RZG2L_GTCCR(rzg2l_gpt_subchannel(pwm->hwpwm)),
> > + dc);
> > +
> > + if (rzg2l_gpt->enable_count[ch] <= 1) {
> > + /* Set initial value for counter */
> > + rzg2l_gpt_write(rzg2l_gpt, offs + RZG2L_GTCNT, 0);
> > +
> > + /* Set no buffer operation */
> > + rzg2l_gpt_write(rzg2l_gpt, offs + RZG2L_GTBER, 0);
> > +
> > + /* Restart the counter after updating the registers */
> > + rzg2l_gpt_modify(rzg2l_gpt, offs + RZG2L_GTCR,
> > + 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;
> > +
> > + if (!state->enabled) {
> > + if (enabled) {
> > + /*
> > + * Probe() sets bootloader_enabled_channels. In such case,
> > + * clearing the flag will avoid errors during unbind.
> > + */
> > + if (rzg2l_gpt->bootloader_enabled_channels[pwm->hwpwm])
> > + rzg2l_gpt->bootloader_enabled_channels[pwm->hwpwm] = 0;
> > +
> > + rzg2l_gpt_disable(rzg2l_gpt, pwm);
> > + pm_runtime_put_sync(pwmchip_parent(chip));
> > + }
> > +
> > + return 0;
> > + }
> > +
> > + if (rzg2l_gpt->bootloader_enabled_channels[pwm->hwpwm]) {
> > + /*
> > + * it's already be on. Instead of reenabling in hardware
> > + * just take over from the bootloader
> > + */
> > + rzg2l_gpt->bootloader_enabled_channels[pwm->hwpwm] = 0;
>
> If .apply() is called, bootloader_enabled_channels[pwm->hwpwm] is certainly 0 afterwards, right? I
> think this means it could be simplified by only handling bootloader_enabled_channels[pwm->hwpwm] ==
> true in a single place?!
Yes will add at the top to handle it in single place.
+ /*
+ * Probe() sets bootloader_enabled_channels. In such case,
+ * clearing the flag will avoid errors during unbind.
+ */
+ if (enabled && rzg2l_gpt->bootloader_enabled_channels[pwm->hwpwm])
+ rzg2l_gpt->bootloader_enabled_channels[pwm->hwpwm] = false;
+
>
> > + } else {
> > + if (!enabled) {
> > + ret = pm_runtime_resume_and_get(pwmchip_parent(chip));
> > + if (ret)
> > + return ret;
> > + }
> > + }
> > +
> > + mutex_lock(&rzg2l_gpt->lock);
> > + ret = rzg2l_gpt_config(chip, pwm, state);
> > + mutex_unlock(&rzg2l_gpt->lock);
> > + if (ret)
> > + goto err_pm_runtime_put;
>
> So if rzg2l_gpt_config() fails you call pm_runtime_put_sync(). But the next time .apply() is called we
> might still have pwm->state.enabled == true and so assume we're still holding a pm_runtime reference.
> That's a bug, right?
You are right, I should not drop pm_run time reference during error.
With enable()->acquire pm_runtime reference
disable()-->release the pm_runtime reference
For config error case, don't call pm_runtime_put_sync().
>
> > +
> > + if (!enabled) {
> > + ret = rzg2l_gpt_enable(rzg2l_gpt, pwm);
> > + if (ret)
> > + goto err_pm_runtime_put;
> > + }
> > +
> > + return 0;
> > +
> > +err_pm_runtime_put:
> > + pm_runtime_put_sync(pwmchip_parent(chip));
> > + 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) {
> > + struct pwm_chip *chip = data;
> > + struct rzg2l_gpt_chip *rzg2l_gpt = to_rzg2l_gpt_chip(chip);
> > + u32 i;
> > +
> > + /*
> > + * The below check is for making balanced PM usage count
> > + * eg: boot loader is turning on PWM and probe increments the PM usage
> > + * count. Before apply, if there is unbind/remove callback we need to
> > + * decrement the PM usage count.
> > + */
> > + for (i = 0; i < chip->npwm; i++) {
> > + if (rzg2l_gpt->bootloader_enabled_channels[i])
> > + pm_runtime_put(pwmchip_parent(chip));
> > + }
> > +
> > + reset_control_assert(rzg2l_gpt->rstc);
> > +}
>
> If you split this in two cleanups, and register the one for
> reset_control_assert(rzg2l_gpt->rstc) earlier in .probe() the error handling there gets easier. This
> would also simplify conversion to
> devm_reset_control_get_exclusive_deasserted() once that lands in mainline.
Agreed.
>
> > +static int rzg2l_gpt_probe(struct platform_device *pdev) {
> > + struct rzg2l_gpt_chip *rzg2l_gpt;
> > + struct device *dev = &pdev->dev;
> > + 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);
> > +
> > + rzg2l_gpt->rstc = devm_reset_control_get_exclusive(dev, NULL);
> > + if (IS_ERR(rzg2l_gpt->rstc))
> > + return dev_err_probe(dev, PTR_ERR(rzg2l_gpt->rstc),
> > + "get reset failed\n");
> > +
> > + clk = devm_clk_get(dev, NULL);
> > + if (IS_ERR(clk))
> > + return dev_err_probe(dev, PTR_ERR(clk), "cannot get clock\n");
> > +
> > + ret = reset_control_deassert(rzg2l_gpt->rstc);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "cannot deassert reset control\n");
> > +
> > + ret = devm_pm_runtime_enable(dev);
> > + if (ret)
> > + goto err_reset;
> > +
> > + ret = pm_runtime_resume_and_get(dev);
> > + if (ret)
> > + goto err_reset;
> > +
> > + ret = devm_clk_rate_exclusive_get(dev, clk);
> > + if (ret)
> > + goto err_pm_put;
> > +
> > + rate = clk_get_rate(clk);
> > + if (!rate) {
> > + ret = dev_err_probe(dev, -EINVAL, "gpt clk rate is 0");
> > + goto err_pm_put;
> > + }
> > +
> > + /*
> > + * Refuse clk rates > 1 GHz to prevent overflow later for computing
> > + * period and duty cycle.
> > + */
> > + if (rate > NSEC_PER_SEC) {
> > + ret = dev_err_probe(dev, -EINVAL, "gpt clk rate is > 1GHz");
> > + goto err_pm_put;
> > + }
> > +
> > + /*
> > + * 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) {
> > + ret = dev_err_probe(dev, -EINVAL, "rate is not multiple of 1000");
> > + goto err_pm_put;
> > + }
> > +
> > + rzg2l_gpt->max_val = div64_u64((u64)U32_MAX * USEC_PER_SEC,
> > + rzg2l_gpt->rate_khz) * RZG2L_MAX_SCALE_FACTOR;
>
> This might loose precision (not entirely sure this is relevant, still more if you drop .max_val as
> suggested above).
Since the rate is 100MHz, in this case we don't lose any precision.
>
> > +
> > + /*
> > + * 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);
> > +
> > + rzg2l_gpt->bootloader_enabled_channels[i] = 1;
> > + rzg2l_gpt->enable_count[ch]++;
> > + pm_runtime_get_sync(dev);
> > + }
> > + }
> > +
> > + pm_runtime_put(dev);
> > +
> > + mutex_init(&rzg2l_gpt->lock);
> > + ret = devm_add_action_or_reset(dev, rzg2l_gpt_reset_assert, chip);
> > + if (ret < 0)
> > + return ret;
> > +
> > + 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;
> > +
> > +err_pm_put:
> > + pm_runtime_put(dev);
>
> A guard (à la guard(pm_runtime_resume)(dev), similar to the guards for mutexes and spinlocks) would be
> elegant here and simplify error handling. (Only if you're motivated, I wouldn't make this a
> precondition for accepting your driver.)
We normally backport these drivers to CIP kernel(SLTSI) 5.10 and 6.1 once it hits mainline and then linux-rc series
