Hi Uwe,
Thanks for the feedback.
> -----Original Message-----
> From: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxx>
> Sent: Tuesday, August 6, 2024 7:47 AM
> Subject: Re: [PATCH v20 3/4] pwm: Add support for RZ/G2L GPT
>
> Hello Biju,
>
> On Fri, Aug 02, 2024 at 07:02:19AM +0000, Biju Das wrote:
> > > -----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?
>
> looks fine.
>
> > > 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-gro
> > > > +up-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))
>
> I like this better, thanks.
>
> > > > + */
> > > > + 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));
> > + }
>
> I wouldn't write back the 5 then. Just assume that the value read back was 5. (Well unless the
> hardware behaves according to the normal formula that applies for prescale ≤ 5, then it might make
> sense to continue with the read value.)
OK, will just print the warning.
dev_warn(dev, "Invalid prescale set %d > 5, prescale);
>
> > > > +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 From that angle, I would like to use mutexes and
> spinlocks, if it is ok for you.
> >
> > Later I will send patch for using guards for pm_runtime_resume,
> > mutexes and spinlocks which I cannot backport to 5.10 and 6.1 kernel.
> >
> > Please let me know is it ok for you?
>
> sure.
Thanks,
Biju
