RE: [PATCH v20 3/4] pwm: Add support for RZ/G2L GPT

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hello Uwe,

> -----Original Message-----
> From: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxx>
> Sent: Tuesday, August 6, 2024 4:13 PM
> Subject: Re: [PATCH v20 3/4] pwm: Add support for RZ/G2L GPT
> 
> Hello Biju,
> 
> On Tue, Aug 06, 2024 at 08:35:39AM +0000, Biju Das wrote:
> > > -----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);
> 
> No, please don't warn (or do it at most once per pwm_device).

OK, Will change it to dev_warn_once(), so that it will print only once as it is
an unlikely case(bootloader setting wrong prescale values).

Cheers,
Biju




[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux