Hi Uwe,
> Subject: RE: [PATCH v2 2/2] pwm: Add support for RZ/G2L GPT
>
> Hi Uwe,
>
> Thanks for the feedback.
>
> > Subject: Re: [PATCH v2 2/2] pwm: Add support for RZ/G2L GPT
> >
> > Hello,
> >
> > On Mon, Jun 06, 2022 at 05:05:09PM +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 × 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
> > >
> > > This patch adds 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>
> > > ---
> > > 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 | 351
> > > ++++++++++++++++++++++++++++++++++++
> > > 3 files changed, 363 insertions(+)
> > > create mode 100644 drivers/pwm/pwm-rzg2l-gpt.c
> > >
> > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index
> > > 904de8d61828..a6cf24cb31e0 100644
> > > --- a/drivers/pwm/Kconfig
> > > +++ b/drivers/pwm/Kconfig
> > > @@ -471,6 +471,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_RENESAS || 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_SAMSUNG
> > > tristate "Samsung PWM support"
> > > depends on PLAT_SAMSUNG || ARCH_S5PV210 || ARCH_EXYNOS ||
> > > COMPILE_TEST diff --git a/drivers/pwm/Makefile
> > > b/drivers/pwm/Makefile index 5c08bdb817b4..12bc2a005e24 100644
> > > --- a/drivers/pwm/Makefile
> > > +++ b/drivers/pwm/Makefile
> > > @@ -43,6 +43,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_SAMSUNG) += pwm-samsung.o
> > > obj-$(CONFIG_PWM_SIFIVE) += pwm-sifive.o
> > > obj-$(CONFIG_PWM_SL28CPLD) += pwm-sl28cpld.o
> > > diff --git a/drivers/pwm/pwm-rzg2l-gpt.c
> > > b/drivers/pwm/pwm-rzg2l-gpt.c new file mode 100644 index
> > > 000000000000..f83ba2d5c219
> > > --- /dev/null
> > > +++ b/drivers/pwm/pwm-rzg2l-gpt.c
> > > @@ -0,0 +1,351 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Renesas RZ/G2L General PWM Timer (GPT) driver
> > > + *
> > > + * Copyright (C) 2022 Renesas Electronics Corporation
> > > + *
> > > + * Limitations:
> > > + * - Mode and Prescalar must be set, while the GTCNT is stopped.
> > > + * - Configured for Output low on GTIOCx pin when counting stops.
> >
> > The last item means the PWM emits the inactive level when disabled,
> > right? Then I suggest to write that as:
> >
> > * - When PWM is disabled, the output is driven to inactive.
> >
> > to simplify understanding that. Also add:
> >
> > * - While the hardware supports both polarities, the driver (for now)
> > * only handles normal polarity.
>
> OK.
>
> >
> > > + */
> > > +
> > > +#include <linux/bitfield.h>
> > > +#include <linux/clk.h>
> > > +#include <linux/io.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>
> > > +
> > > +#define GPT_IO_PER_CHANNEL 2
> > > +
> > > +#define GTPR_MAX_VALUE 0xFFFFFFFF
> > > +#define GTCR 0x2c
> > > +#define GTUDDTYC 0x30
> > > +#define GTIOR 0x34
> > > +#define GTBER 0x40
> > > +#define GTCNT 0x48
> > > +#define GTCCRA 0x4c
> > > +#define GTCCRB 0x50
> > > +#define GTPR 0x64
> > > +
> > > +#define GTCR_CST BIT(0)
> > > +#define GTCR_MD GENMASK(18, 16)
> > > +#define GTCR_TPCS GENMASK(26, 24)
> > > +
> > > +#define GTCR_MD_SAW_WAVE_PWM_MODE FIELD_PREP(GTCR_MD, 0)
> > > +
> > > +#define GTUDDTYC_UP BIT(0)
> > > +#define GTUDDTYC_UDF BIT(1)
> > > +#define UP_COUNTING (GTUDDTYC_UP | GTUDDTYC_UDF)
> > > +
> > > +#define GTIOR_GTIOA GENMASK(4, 0)
> > > +#define GTIOR_GTIOB GENMASK(20, 16)
> > > +#define GTIOR_OAE BIT(8)
> > > +#define GTIOR_OBE BIT(24)
> > > +
> > > +#define INIT_OUT_LO_OUT_LO_END_TOGGLE 0x07
> > > +#define INIT_OUT_HI_OUT_HI_END_TOGGLE 0x1b
> > > +
> > > +#define GTIOR_GTIOA_OUT_HI_END_TOGGLE_CMP_MATCH
> > (INIT_OUT_HI_OUT_HI_END_TOGGLE | GTIOR_OAE)
> > > +#define GTIOR_GTIOA_OUT_LO_END_TOGGLE_CMP_MATCH
> > (INIT_OUT_LO_OUT_LO_END_TOGGLE | GTIOR_OAE)
> > > +#define GTIOR_GTIOB_OUT_HI_END_TOGGLE_CMP_MATCH
> > ((INIT_OUT_HI_OUT_HI_END_TOGGLE << 16) | GTIOR_OBE)
> > > +#define GTIOR_GTIOB_OUT_LO_END_TOGGLE_CMP_MATCH
> > ((INIT_OUT_LO_OUT_LO_END_TOGGLE << 16) | GTIOR_OBE)
> >
> > the LHS of the last define can be better written as:
> >
> > FIELD_PREP(GTIOR_GTIOB, INIT_OUT_LO_OUT_LO_END_TOGGLE) | GTIOR_OBE
> >
> > It's a bit longer, but doesn't duplicate the 16. Similar for the other
> > defines.
>
> But this is giving compilation error, Any pointers to fix this issue?
>
> In file included from drivers/pwm/pwm-rzg2l-gpt.c:14:
> ./include/linux/bitfield.h:113:2: error: braced-group within expression
> allowed only inside a function
> 113 | ({ \
> | ^
> drivers/pwm/pwm-rzg2l-gpt.c:57:55: note: in expansion of macro
> 'FIELD_PREP'
> 57 | #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
> | ^~~~~~~~~~
> drivers/pwm/pwm-rzg2l-gpt.c:75:12: note: in expansion of macro
> 'RZG2L_GTIOR_GTIOB_OUT_HI_END_TOGGLE_CMP_MATCH'
> 75 | .value = RZG2L_GTIOR_GTIOB_OUT_HI_END_TOGGLE_CMP_MATCH,
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> make[2]: *** [scripts/Makefile.build:249: drivers/pwm/pwm-rzg2l-gpt.o]
> Error 1
> make[1]: *** [scripts/Makefile.build:466: drivers/pwm] Error 2
> make[1]: *** Waiting for unfinished jobs....
> make: *** [Makefile:1843: drivers] Error 2
> make: *** Waiting for unfinished jobs....
>
> >
> > Can you please prefix all these defines all by RZG2L_?
>
> Ok, Agreed.
>
> >
> > > +
> > > +struct phase {
> > > + u32 value;
> > > + u32 mask;
> > > + u32 duty_reg_offset;
> > > +};
> > > +
> > > +static const struct phase phase_params[] = {
> > > + /* Setting for phase A */
> > > + {
> > > + .value = GTIOR_GTIOA_OUT_HI_END_TOGGLE_CMP_MATCH,
> > > + .mask = GTIOR_GTIOA | GTIOR_OAE,
> > > + .duty_reg_offset = GTCCRA,
> > > + },
> > > + /* Setting for phase B */
> > > + {
> > > + .value = GTIOR_GTIOB_OUT_HI_END_TOGGLE_CMP_MATCH,
> > > + .mask = GTIOR_GTIOB | GTIOR_OBE,
> > > + .duty_reg_offset = GTCCRB,
> > > + },
> > > +};
> > > +
> > > +struct gpt_pwm_device {
> > > + const struct phase *ph;
> > > +};
> > > +
> > > +struct rzg2l_gpt_chip {
> > > + struct pwm_chip chip;
> > > + void __iomem *mmio;
> > > + struct reset_control *rstc;
> > > + struct clk *clk;
> > > + struct gpt_pwm_device gpt[2];
> > > +};
> > > +
> > > +static inline struct rzg2l_gpt_chip *to_rzg2l_gpt_chip(struct
> > > +pwm_chip *chip) {
> > > + return container_of(chip, struct rzg2l_gpt_chip, chip); }
> > > +
> > > +static void rzg2l_gpt_write(struct rzg2l_gpt_chip *pc, u32 reg, u32
> > > +data) {
> > > + iowrite32(data, pc->mmio + reg);
> > > +}
> > > +
> > > +static u32 rzg2l_gpt_read(struct rzg2l_gpt_chip *pc, u32 reg) {
> > > + return ioread32(pc->mmio + reg);
> > > +}
> > > +
> > > +static void rzg2l_gpt_modify(struct rzg2l_gpt_chip *pc, u32 reg,
> > > +u32 clr, u32 set) {
> > > + rzg2l_gpt_write(pc, reg, (rzg2l_gpt_read(pc, reg) & ~clr) | set);
> > > +}
> > > +
> > > +static u8 rzg2l_calculate_prescale(struct rzg2l_gpt_chip *pc, u64
> > > +period_cycles) {
> > > + u16 i, prod;
> > > + u8 prescale;
> > > +
> > > + prescale = 5;
> > > + /* prescale 1, 4, 16, 64, 256 and 1024 */
> > > + for (i = 0; i < 6; i++) {
> > > + prod = 1 << (2 * i);
> > > + if ((period_cycles / (1ULL * GTPR_MAX_VALUE * prod)) == 0) {
> > > + prescale = i;
> > > + break;
> > > + }
> > > + }
> > > +
> > > + return prescale;
> >
> > You must not do 64 bit divisions using /.
> > Also you can shorten the calculation using something like:
> >
> > prescaled_period_cycles = period_cycles;
> > do_div(prescaled_period_cycles, GTPR_MAX_VALUE);
> >
> > prescale = fls((prescaled_period_cycles + 1) >> 1);
> > return min(prescale, 5);
> >
> > (Please double check, I didn't)
>
> OK, will do.
I double checked and prescalar values seems to be wrong with the above calculation.
Rate=100MHz
32 bit counter
With this max period achievable is 42 sec and min is 0.1 nsec.
Max -> 2^32 / 100M = 42 sec
Min -> 1 / 100M = 0.1 nsec.
To increase the max period, say
from 42 sec to 168 sec we need to use prescalar = 4, So Clk rate reduces to 100 M/ 4.
from 168 sec to 640 sec we need to use prescalar = 16
etc ...
I used the below logic and it gives proper prescale values.
+ prescaled_period_cycles = period_cycles >> 32;
+ prescale = 5;
+ /* prescale 1, 4, 16, 64, 256 and 1024 */
+ for (i = 0; i < 6; i++) {
+ if ((1 << (2 * i)) > prescaled_period_cycles) {
+ prescale = i;
+ break;
+ }
+ }
Please correct me, if anything wrong with this calculation.
Cheers,
Biju
