RE: [PATCH v3 2/2] pwm: Add support for RZ/G2L GPT

From: Biju Das <biju.das.jz@bp.renesas.com>
To: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: "Thierry Reding" <thierry.reding@gmail.com>,
	"Lee Jones" <lee.jones@linaro.org>,
	"Philipp Zabel" <p.zabel@pengutronix.de>,
	"Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>,
	"Linux PWM List" <linux-pwm@vger.kernel.org>,
	"Geert Uytterhoeven" <geert+renesas@glider.be>,
	"Chris Paterson" <Chris.Paterson2@renesas.com>,
	"Biju Das" <biju.das@bp.renesas.com>,
	"Prabhakar Mahadev Lad" <prabhakar.mahadev-lad.rj@bp.renesas.com>,
	Linux-Renesas <linux-renesas-soc@vger.kernel.org>
Subject: RE: [PATCH v3 2/2] pwm: Add support for RZ/G2L GPT
Date: Thu, 21 Jul 2022 15:24:18 +0000	[thread overview]
Message-ID: <OS0PR01MB5922B77B0C92EDAC1DA5986C86919@OS0PR01MB5922.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <CAMuHMdWs_ah_UZdxnpA1hkPaSarokMhcj1r3_jt_oWFG6SWP+w@mail.gmail.com>

Hi Geert,

Thanks for the feedback.

> Subject: Re: [PATCH v3 2/2] pwm: Add support for RZ/G2L GPT
> 
> Hi Biju,
> 
> On Tue, Jul 19, 2022 at 5:59 PM Biju Das <biju.das.jz@bp.renesas.com>
> 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@bp.renesas.com>
> > ---
> > v2->v3:
> 
> Thanks for the update!
> 
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-rzg2l-gpt.c
> 
> > +static u8 rzg2l_calculate_prescale(struct rzg2l_gpt_chip *pc, u64
> > +period_cycles) {
> > +       u64 prescaled_period_cycles;
> > +       u8 prescale;
> > +       u16 i;
> 
> Why do prescale and i differ in type?
> Why not use unsigned int?

It should be u8 for both.

prescale max value is 5, that corresponds to div by 1024.
So u8 is sufficient. Yes, I should be changed to u8

> 
> > +
> > +       prescaled_period_cycles = period_cycles >> 32;
> 
> So prescaled_period_cycles can be u32?
Yes, will change it to u32.

> 
> > +       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;
> > +               }
> > +       }
> > +
> > +       return prescale;
> > +}
> 
> > +static int rzg2l_gpt_config(struct pwm_chip *chip, struct pwm_device
> *pwm,
> > +                           u64 duty_ns, u64 period_ns) {
> > +       struct rzg2l_gpt_chip *pc = to_rzg2l_gpt_chip(chip);
> > +       struct gpt_pwm_device *gpt = &pc->gpt[pwm->hwpwm];
> > +       unsigned long rate, pv, dc;
> > +       u64 period_cycles;
> > +       u8 prescale;
> > +
> > +       rate = clk_get_rate(pc->clk);
> > +       /*
> > +        * Refuse clk rates > 1 GHz to prevent overflowing the
> following
> > +        * calculation.
> > +        */
> > +       if (rate > NSEC_PER_SEC)
> > +               return -EINVAL;
> > +
> > +       period_cycles = mul_u64_u64_div_u64(rate, period_ns,
> > + NSEC_PER_SEC);
> 
> As NSEC_PER_SEC fits in 32-bit, and we know rate does, too, you can use
> the cheaper mul_u64_u32_div(period_ns, rate, NSEC_PER_SEC) instead.

OK, agreed.

> 
> > +       prescale = rzg2l_calculate_prescale(pc, period_cycles);
> > +
> > +       pv = round_down(period_cycles >> (2 * prescale), 1 << (2 *
> prescale));
> > +       period_cycles = mul_u64_u64_div_u64(rate, duty_ns,
> > + NSEC_PER_SEC);
> 
> Likewise.
OK.
> 
> > +       dc = round_down(period_cycles >> (2 * prescale), 1 << (2 *
> > + prescale));
> 
> Why are pv and dc unsigned long, which is either 32-bit or 64-bit?

It should be 64 bit(aarch64). 

Our timer is 32 bit and we have 100 MHz clock
With this max period achievable is 42 sec and min is 0.1 nsec.

Max -> (2^32 /100M) = 42 sec = 42 sec which is 32 bit.
Min -> 1 / 100M = 0.1 nsec.

Since we have dividers up to 1024,

We can have max period of 42 * 1024 sec = 43000 * 10^9 nano secs which is more than 32-bit.

> 
> > +static const struct of_device_id rzg2l_gpt_of_table[] = {
> > +       { .compatible = "renesas,rzg2l-gpt", },
> > +       { /* Sentinel */ },
> 
> Please drop the comma after the sentinel.

OK

> 
> > +};
> > +MODULE_DEVICE_TABLE(of, rzg2l_gpt_of_table);
> 
> > +static int rzg2l_gpt_probe(struct platform_device *pdev) {
> > +       struct rzg2l_gpt_chip *rzg2l_gpt;
> > +       struct clk *clk;
> > +       int ret;
> > +
> > +       rzg2l_gpt = devm_kzalloc(&pdev->dev, sizeof(*rzg2l_gpt),
> GFP_KERNEL);
> > +       if (!rzg2l_gpt)
> > +               return -ENOMEM;
> > +
> > +       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_shared(&pdev->dev,
> NULL);
> > +       if (IS_ERR(rzg2l_gpt->rstc))
> > +               return dev_err_probe(&pdev->dev, PTR_ERR(rzg2l_gpt-
> >rstc),
> > +                                    "get reset failed\n");
> > +
> > +       rzg2l_gpt->clk = devm_clk_get(&pdev->dev, NULL);
> > +       if (IS_ERR(rzg2l_gpt->clk))
> > +               return dev_err_probe(&pdev->dev, PTR_ERR(rzg2l_gpt-
> >clk),
> > +                                    "cannot get clock\n");
> > +
> > +       platform_set_drvdata(pdev, rzg2l_gpt);
> > +
> > +       ret = reset_control_deassert(rzg2l_gpt->rstc);
> > +       if (ret) {
> > +               dev_err(&pdev->dev, "cannot deassert reset control:
> %pe\n",
> > +                       ERR_PTR(ret));
> > +               return ret;
> > +       }
> > +
> > +       pm_runtime_enable(&pdev->dev);
> > +
> > +       ret = devm_add_action_or_reset(&pdev->dev,
> > +
> rzg2l_gpt_reset_assert_pm_disable,
> > +                                      rzg2l_gpt);
> > +       if (ret < 0)
> > +               return ret;
> > +
> > +       clk = devm_clk_get_optional_enabled(&pdev->dev, NULL);
> 
> Why optional?
> You already have a pointer to this clock?

My bad, will use non_optional one.

> 
> > +       if (IS_ERR(clk)) {
> > +               rzg2l_gpt_reset_assert_pm_disable(rzg2l_gpt);
> 
> Isn't this called automatically, due to devm_add_action_or_reset()?

Yes, will remove it.
> 
> > +               return dev_err_probe(&pdev->dev, PTR_ERR(clk),
> > +                                    "clk operation failed");
> > +       }
> > +
> > +       if (!(rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTCR) & RZG2L_GTCR_CST))
> > +               devm_clk_put(&pdev->dev, clk);
> 
> This is done to keep the clock enabled in case it was enabled before?
> I think this deserves a comment.

OK will add comment.

Cheers,
Biju

> 
> > +
> > +       rzg2l_gpt->chip.dev = &pdev->dev;
> > +       rzg2l_gpt->chip.ops = &rzg2l_gpt_ops;
> > +       rzg2l_gpt->chip.npwm = RZG2L_GPT_IO_PER_CHANNEL;
> > +
> > +       return devm_pwmchip_add(&pdev->dev, &rzg2l_gpt->chip); }
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-
> m68k.org
> 
> In personal conversations with technical people, I call myself a hacker.
> But when I'm talking to journalists I just say "programmer" or something
> like that.
>                                 -- Linus Torvalds