> -----Original Message-----
> From: Biju Das
> Sent: 07 November 2022 12:02
> To: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> Cc: Thierry Reding <thierry.reding@xxxxxxxxx>; Philipp Zabel
> <p.zabel@xxxxxxxxxxxxxx>; Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>;
> linux-pwm@xxxxxxxxxxxxxxx; Chris Paterson <Chris.Paterson2@xxxxxxxxxxx>; Biju
> Das <biju.das@xxxxxxxxxxxxxx>; Prabhakar Mahadev Lad <prabhakar.mahadev-
> lad.rj@xxxxxxxxxxxxxx>; linux-renesas-soc@xxxxxxxxxxxxxxx; Linux PM list
> <linux-pm@xxxxxxxxxxxxxxx>
> Subject: RE: [PATCH v9 2/2] pwm: Add support for RZ/G2L GPT
>
> Hi Geert,
>
> > -----Original Message-----
> > From: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> > Sent: 07 November 2022 10:50
> > To: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > Cc: Thierry Reding <thierry.reding@xxxxxxxxx>; Philipp Zabel
> > <p.zabel@xxxxxxxxxxxxxx>; Uwe Kleine-König
> > <u.kleine-koenig@xxxxxxxxxxxxxx>; linux-pwm@xxxxxxxxxxxxxxx; Chris
> > Paterson <Chris.Paterson2@xxxxxxxxxxx>; Biju Das
> > <biju.das@xxxxxxxxxxxxxx>; Prabhakar Mahadev Lad <prabhakar.mahadev-
> > lad.rj@xxxxxxxxxxxxxx>; linux-renesas-soc@xxxxxxxxxxxxxxx; Linux PM
> > list <linux-pm@xxxxxxxxxxxxxxx>
> > Subject: Re: [PATCH v9 2/2] pwm: Add support for RZ/G2L GPT
> >
> > Hi Biju,
> >
> > CC linux-pm
> >
> > On Mon, Nov 7, 2022 at 11:35 AM Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> wrote:
> > > > -----Original Message-----
> > > > From: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> > > > Sent: 07 November 2022 10:28
> > > > On Fri, Oct 28, 2022 at 12:42 PM Biju Das
> > > > <biju.das.jz@xxxxxxxxxxxxxx>
> > 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>
> > > > > ---
> > > > > v8->v9:
> > > > > * deassert after devm_clk_get() to avoid reset stays
> > > > > deasserted,in
> > case
> > > > > clk_get() fails.
> > > > > * Removed ch_offs from struct rzg2l_gpt_chip and use macro instead.
> > > > > * Removed clk_disable_unprepare() from probe as it is giving
> > > > > gpt_pclk already disabled warning in the error path.
> > > > > [ 0.915664] clk_core_disable+0x25c/0x274
> > > > > [ 0.915754] clk_disable+0x2c/0x44
> > > > > [ 0.915833] rzg2l_gpt_pm_runtime_suspend+0x1c/0x34
> > > > > [ 0.915938] pm_generic_runtime_suspend+0x28/0x40
> > > > > [ 0.916042] genpd_runtime_suspend+0xa8/0x2b0
> > > > > [ 0.916136] __rpm_callback+0x44/0x13c
> > > > > [ 0.916218] rpm_callback+0x64/0x70
> > > > > [ 0.916296] rpm_suspend+0x104/0x630
> > > > > [ 0.916374] pm_runtime_work+0xb4/0xbc
> > > > > [ 0.916456] process_one_work+0x288/0x6a
> > > >
> > > > Thanks for the update!
> > > >
> > > > > --- /dev/null
> > > > > +++ b/drivers/pwm/pwm-rzg2l-gpt.c
> > > >
> > > > > +static int __maybe_unused rzg2l_gpt_pm_runtime_suspend(struct
> > > > > +device
> > > > > +*dev) {
> > > > > + struct rzg2l_gpt_chip *rzg2l_gpt = dev_get_drvdata(dev);
> > > > > +
> > > > > + clk_disable_unprepare(rzg2l_gpt->clk);
> > > > > +
> > > > > + return 0;
> > > > > +}
> > > > > +
> > > > > +static int __maybe_unused rzg2l_gpt_pm_runtime_resume(struct
> > > > > +device
> > > > > +*dev) {
> > > > > + struct rzg2l_gpt_chip *rzg2l_gpt = dev_get_drvdata(dev);
> > > > > +
> > > > > + clk_prepare_enable(rzg2l_gpt->clk);
> > > > > +
> > > > > + return 0;
> > > > > +}
> > > > > +
> > > > > +static const struct dev_pm_ops rzg2l_gpt_pm_ops = {
> > > > > + SET_RUNTIME_PM_OPS(rzg2l_gpt_pm_runtime_suspend,
> > > > > +rzg2l_gpt_pm_runtime_resume, NULL) };
> > > > > +
> > > > > +static void rzg2l_gpt_reset_assert_pm_disable(void *data) {
> > > > > + struct rzg2l_gpt_chip *rzg2l_gpt = data;
> > > > > +
> > > > > + pm_runtime_disable(rzg2l_gpt->chip.dev);
> > > > > + pm_runtime_set_suspended(rzg2l_gpt->chip.dev);
> > > > > + reset_control_assert(rzg2l_gpt->rstc);
> > > > > +}
> > > > > +
> > > > > +static int rzg2l_gpt_probe(struct platform_device *pdev) {
> > > > > + bool ch_en[RZG2L_MAX_PWM_CHANNELS];
> > > > > + struct rzg2l_gpt_chip *rzg2l_gpt;
> > > > > + int ret;
> > > > > + u32 i;
> > > > > +
> > > > > + 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_exclusive(&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");
> > > > > +
> > > > > + ret = reset_control_deassert(rzg2l_gpt->rstc);
> > > > > + if (ret)
> > > > > + return dev_err_probe(&pdev->dev, ret,
> > > > > + "cannot deassert reset
> > > > > + control\n");
> > > > > +
> > > > > + rzg2l_gpt->rate = clk_get_rate(rzg2l_gpt->clk);
> > > > > +
> > > > > + clk_prepare_enable(rzg2l_gpt->clk);
> > > >
> > > > So you enable the clock in .probe()...
> > >
> > > Yes, But the PM handler context disable the clock once we return
> > > from
> > probe.
> > > Please see the context for clk_disable.
> > >
> > > [ 0.915754] clk_disable+0x2c/0x44
> > > [ 0.915833] rzg2l_gpt_pm_runtime_suspend+0x1c/0x34
> > > [ 0.915938] pm_generic_runtime_suspend+0x28/0x40
> > > [ 0.916042] genpd_runtime_suspend+0xa8/0x2b0
> > > [ 0.916136] __rpm_callback+0x44/0x13c
> > > [ 0.916218] rpm_callback+0x64/0x70
> > > [ 0.916296] rpm_suspend+0x104/0x630
> > > [ 0.916374] pm_runtime_work+0xb4/0xbc
> > >
> > > >
> > > > > + pm_runtime_set_active(&pdev->dev);
> > > > > + pm_runtime_enable(&pdev->dev);
> > > > > + ret = devm_add_action_or_reset(&pdev->dev,
> > > > > +
> > > > > + rzg2l_gpt_reset_assert_pm_disable,
> > > >
> > > > ... and rely on Runtime PM to disable the clock on error/remove?
> > > > Does that actually work?
> > >
> > > See above.
> >
> > According to that backtrace, there must be an asymmetry in the
> > behavior of the following two pairs of Runtime PM calls:
> >
> > pm_runtime_set_active(&pdev->dev);
> > pm_runtime_enable(&pdev->dev);
> >
> > => does not call rzg2l_gpt_pm_runtime_resume()
> > (and thus does not call clk_prepare_enable())
>
> Yes, that is correct.
>
> But when we return from the probe() failure/success case, it calls for
Just a correction, On error case there is no pm_runtime_work() context
Previously for simulating error case, I have used goto statement but return value as 0 from probe().
[ 13.618443] pwm-rzg2l-gpt: probe of 10048000.pwm failed with error -22
[ 13.670395] ############rzg2l_gpt_reset_assert_pm_disable########
> suspend in pm_runtime_work() context.
>
> [ 0.915754] clk_disable+0x2c/0x44
> [ 0.915833] rzg2l_gpt_pm_runtime_suspend+0x1c/0x34
Cheers,
Biju
