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-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))
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.)
> > > +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.
Best regards
Uwe
Attachment:
signature.asc
Description: PGP signature
