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

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

 



Hi Geert,

Thanks for the feedback.

> 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())
> 
> 
>     pm_runtime_disable(rzg2l_gpt->chip.dev);
>     pm_runtime_set_suspended(rzg2l_gpt->chip.dev);
> 
>         => does call rzg2l_gpt_pm_runtime_suspend()
>            (and thus does call clk_disable_unprepare())
> 
> That sounds like an issue in Runtime PM?
> 
> > > > +                                      rzg2l_gpt);
> > > > +       if (ret < 0)
> > > > +               return ret;
> > > > +
> > > > +       mutex_init(&rzg2l_gpt->lock);
> > > > +       platform_set_drvdata(pdev, rzg2l_gpt);
> > > > +
> > > > +       /*
> > > > +        *  We need to keep the clock on, in case the bootloader has
> enabled the
> > > > +        *  PWM and is running during probe().
> > > > +        */
> > > > +       for (i = 0; i < RZG2L_MAX_PWM_CHANNELS; i++) {
> > > > +               ch_en[i] = rzg2l_gpt_is_ch_enabled(rzg2l_gpt, i);
> > > > +               if (ch_en[i])
> 
> ch_en[] could be a bitmask instead of an array...


It is a local array. Maybe we need to initialize a u8
variable to 0 initially for bitmask operation??

> 
> > > > +                       pm_runtime_get_sync(&pdev->dev);
> 
> Does that work across driver remove/unbind?

Yes it works. Please find the test logs

Bootloader turning on PWM case
-------------------------

mw 0x11010840 0x10001
mw 0x11010540 0x10001
mw 0x1004842c 0x1
mw 0x10048434 0x031b031b
mw 0x10048464 0x0

root@smarc-rzg2l:~# /pwm.sh
16
pwmchip0
Read at address  0x11010540 (0xffff9d64d540): 0x00000001


#### Zero duty cycle ###
#### decrement Period ###
#### Increment Period ###
#### decrement duty cycle ###
#### Increment duty cycle ###
Read at address  0x11010540 (0xffffad6ce540): 0x00000000
root@smarc-rzg2l:~# lsmod
Module                  Size  Used by
panfrost               65536  0
drm_shmem_helper       24576  1 panfrost
gpu_sched              36864  1 panfrost
snd_soc_simple_card    20480  0
crct10dif_ce           16384  1
snd_soc_simple_card_utils    24576  1 snd_soc_simple_card
drm                   565248  4 gpu_sched,drm_shmem_helper,panfrost
renesas_usbhs          61440  0
snd_soc_wm8978         45056  1
rcar_canfd             32768  0
snd_soc_rz_ssi         20480  1
can_dev                36864  1 rcar_canfd
renesas_rpc_if         20480  0
rzg2l_adc              16384  0
spi_rspi               24576  0
pwm_rzg2l_gpt          16384  0
ipv6                  458752  18
root@smarc-rzg2l:~# rmmod pwm_rzg2l_gpt
root@smarc-rzg2l:~# modprobe pwm_rzg2l_gpt
root@smarc-rzg2l:~# /pwm.sh
16
pwmchip0
Read at address  0x11010540 (0xffff89a98540): 0x00000000


#### Zero duty cycle ###
#### decrement Period ###
#### Increment Period ###
#### decrement duty cycle ###
#### Increment duty cycle ###
Read at address  0x11010540 (0xffff8b11c540): 0x00000000
root@smarc-rzg2l:~# rmmod pwm_rzg2l_gpt
root@smarc-rzg2l:~# modprobe pwm_rzg2l_gpt
root@smarc-rzg2l:~#

PWM enabled only by Linux case:-
-------------------------------
root@smarc-rzg2l:~# /pwm.sh
16
pwmchip0
Read at address  0x11010540 (0xffffad57b540): 0x00000000


#### Zero duty cycle ###
#### decrement Period ###
#### Increment Period ###
#### decrement duty cycle ###
#### Increment duty cycle ###
Read at address  0x11010540 (0xffffa52e1540): 0x00000000
root@smarc-rzg2l:~# rmmod pwm_rzg2l_gpt
root@smarc-rzg2l:~# modprobe pwm_rzg2l_gpt
root@smarc-rzg2l:~# /pwm.sh
16
pwmchip0
Read at address  0x11010540 (0xffff9557f540): 0x00000000


#### Zero duty cycle ###
#### decrement Period ###
#### Increment Period ###
#### decrement duty cycle ###
#### Increment duty cycle ###
Read at address  0x11010540 (0xffff958da540): 0x00000000
root@smarc-rzg2l:~# rmmod pwm_rzg2l_gpt
root@smarc-rzg2l:~# modprobe pwm_rzg2l_gpt
root@smarc-rzg2l:~#

Cheers,
Biju
> 
> > > > +       }
> > > > +
> > > > +       rzg2l_gpt->chip.dev = &pdev->dev;
> > > > +       rzg2l_gpt->chip.ops = &rzg2l_gpt_ops;
> > > > +       rzg2l_gpt->chip.npwm = RZG2L_MAX_PWM_CHANNELS;
> > > > +
> > > > +       ret = devm_pwmchip_add(&pdev->dev, &rzg2l_gpt->chip);
> > > > +       if (ret) {
> > > > +               for (i = 0; i < RZG2L_MAX_PWM_CHANNELS; i++) {
> > > > +                       if (ch_en[i])
> > > > +                               pm_runtime_put(&pdev->dev);
> > > > +               }
> > > > +
> > > > +               dev_err_probe(&pdev->dev, ret, "failed to add PWM
> chip\n");
> > > > +       }
> > > > +
> > > > +       return ret;
> > > > +}
> 
> 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




[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