Re: [PATCH v7 2/2] pwm: Add support for RZ/G2L GPT

[Geert Uytterhoeven <geert@linux-m68k.org>]

Hi Thierry,

On Wed, Sep 28, 2022 at 3:50 PM Thierry Reding <thierry.reding@gmail.com> wrote:
> On Wed, Sep 21, 2022 at 03:57: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 × 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>

> > --- /dev/null
> > +++ b/drivers/pwm/pwm-rzg2l-gpt.c
> > +static int rzg2l_gpt_probe(struct platform_device *pdev)
> > +{
> > +     struct rzg2l_gpt_chip *rzg2l_gpt;
> > +     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");
> > +
> > +     ret = reset_control_deassert(rzg2l_gpt->rstc);
> > +     if (ret)
> > +             return dev_err_probe(&pdev->dev, ret,
> > +                                  "cannot deassert reset control\n");
> > +
> > +     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;
> > +
> > +     rzg2l_gpt->clk = devm_clk_get_enabled(&pdev->dev, NULL);
> > +     if (IS_ERR(rzg2l_gpt->clk))
> > +             return dev_err_probe(&pdev->dev, PTR_ERR(rzg2l_gpt->clk),
> > +                                  "cannot get clock\n");
> > +
> > +     rzg2l_gpt->rate = clk_get_rate(rzg2l_gpt->clk);
> > +     /*
> > +      *  We need to keep the clock on, in case the bootloader has enabled the
> > +      *  PWM and is running during probe(). A variable pwm_enabled_by_
> > +      *  bootloader is set to true in that case and during first
> > +      *  pwm_disable(), it is set to false and release the clock resource.
> > +      *
> > +      *  In case the pwm is not enabled by the bootloader, devm_clk_put
> > +      *  disables the clk and holding a reference on the clk isn't needed
> > +      *  because runtime-pm handles the needed enabling.
> > +      */
>
> Where does this happen? I'm not aware of any code that would
> automatically enable clocks for runtime PM. Typically you would need to
> implement driver-specific callbacks for that to happen.
>
> > +     if (rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTCR) & RZG2L_GTCR_CST)
> > +             rzg2l_gpt->pwm_enabled_by_bootloader = true;
> > +     else
> > +             devm_clk_put(&pdev->dev, rzg2l_gpt->clk);
>
> So in either case I would expect you to want to hold on to the clock
> pointer here and use that in the runtime PM callbacks.
>
> This whole business of keeping a separate variable to track this also
> seems a bit fishy to me because it only partially reflects in the
> software state what's really going on. So I think what you really want
> here is to call pm_runtime_set_active() before pm_runtime_enable() to
> make sure that your device is marked as such.
>
> I wonder if that alone wouldn't already solve this problem. IIRC, the
> runtime PM infrastructure will consider a device to be runtime suspended
> after ->probe() by default. And if the clock is indeed managed by
> runtime PM somehow, then that might just cause it to get disabled.
> Again, it'd be great to know how exactly runtime PM knows how to manage
> the clock if you don't tell it here. Is the clock perhaps shared between
> multiple IPs? Perhaps a parent device that managed it in runtime PM?

It's handled by the clock domain code in the PM Domain framework,
cfr. GENPD_FLAG_PM_CLK.  All members of the PM Domain have
their module clock(s) managed automatically through Runtime PM.

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