RE: [PATCH] clocksource/drivers/sh_cmt: wait for CMCNT on init

[Biju Das <biju.das@bp.renesas.com>]

Hi Geert,


> -----Original Message-----
> From: Geert Uytterhoeven [mailto:geert@linux-m68k.org]
> Sent: 11 June 2018 12:41
> To: Biju Das <biju.das@bp.renesas.com>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>; Thomas Gleixner
> <tglx@linutronix.de>; Simon Horman <horms@verge.net.au>; Geert
> Uytterhoeven <geert+renesas@glider.be>; Chris Paterson
> <Chris.Paterson2@renesas.com>; Fabrizio Castro
> <fabrizio.castro@bp.renesas.com>; Linux-Renesas <linux-renesas-
> soc@vger.kernel.org>
> Subject: Re: [PATCH] clocksource/drivers/sh_cmt: wait for CMCNT on init
>
> Hi Biju,
>
> On Mon, Jun 11, 2018 at 1:21 PM Biju Das <biju.das@bp.renesas.com> wrote:
> > > Subject: Re: [PATCH] clocksource/drivers/sh_cmt: wait for CMCNT on
> > > init On Fri, Jun 8, 2018 at 6:24 PM Biju Das <biju.das@bp.renesas.com>
> wrote:
> > > > As per section 57A.3.5/69A.3.5/79.A.3.5 of rz/g/r-car gen2/3
> > > > hardware manual,it is mentioned that we need to provide 2 cycles
> > > > in counter input clock (RCLK) for reflecting written data to counter
> behaviour.
> > > > Adding sufficient wait to let the CMCNT register value settle
> > > > before starting the timer channel.
> > >
> > > RCLK usually runs at ca. 32 kHz, so 32µs should be sufficient on
> > > R-Car
> > > Gen2/3 and RZ/G1.
> > > R-Mobile A1 is the exception: RCLK runs at 23 kHz, so you need 43µs
> > > to be safe.
> > >
> > > > It fixes the error "sh_cmt ffca0000.timer: ch1: cannot clear CMCNT"
> > > >
> > > > Signed-off-by: Biju Das <biju.das@bp.renesas.com>
> > > > Reviewed-by: Chris Paterson <chris.paterson2@renesas.com>
> > > > ---
> > > > Hello,
> > > >
> > > > During cmt testing, the tool
> > > > (tools/testing/selftests/timers/clocksource-switch.c)
> > > > is complaining about the error "sh_cmt ffca0000.timer: ch1: cannot
> > > > clear
> > > CMCNT".
> > > > The above patch fixes this issue is by adding sufficient wait to
> > > > let the CMCNT register value settle before starting the timer channel.
> > > >
> > > > This issue is reproduced on Koelsch/RZ/G1[ME] based iwave boards
> > > > etc..., as I assume the same issue should be present on lager etc. as
> well?
> > >
> > > > --- a/drivers/clocksource/sh_cmt.c
> > > > +++ b/drivers/clocksource/sh_cmt.c
> > > > @@ -328,7 +328,7 @@ static void sh_cmt_start_stop_ch(struct
> > > > sh_cmt_channel *ch, int start)
> > > >
> > > >  static int sh_cmt_enable(struct sh_cmt_channel *ch)  {
> > > > -       int k, ret;
> > > > +       int j, k, ret;
> > > >
> > > >         pm_runtime_get_sync(&ch->cmt->pdev->dev);
> > > >         dev_pm_syscore_device(&ch->cmt->pdev->dev, true); @@
> > > > -368,11 +368,17 @@ static int sh_cmt_enable(struct sh_cmt_channel
> > > *ch)
> > > >          * While at it, we're supposed to clear out the CMCNT as of this
> > > >          * moment, so make sure it's processed properly here.  This will
> > > >          * take RCLKx2 at maximum.
> > > > +        *
> > > > +        * Similar register access usage for CMCNT is mentioned in R-Car
> > > > +        * Gen[2/3]/RZ/G1 user's manual, RCLKx2 for cmt0 and RCLKx2 or
> > > > +        * CPϕx2 (CPEXϕx2)) for cmt1.
> > > >          */
> > > > -       for (k = 0; k < 100; k++) {
> > > > -               if (!sh_cmt_read_cmcnt(ch))
> > > > -                       break;
> > > > -               udelay(1);
> > >
> > > 100 * 1µs = 100µs, so the original code should be sufficient?
> >
> > From the test results,  the original code is not sufficient. It needs 137 µs on
> koelsch platform to settle this value.
> >
> > > Or should it wait 42µs unconditionally, i.e. without checking CMCNT?
> >
> > Yes, Adding  wait 42µs unconditionally, just before the for loop also fixes
> the issue, since worst case wait is 137 µs.
> > What is your suggestion here?
> >
> > > The datasheet mentions some other registers 2 RCLK cycles, too.
> > > Should there be a fixed 2 RCLK delay in sh_cmt_write_cmcsr() and
> > > sh_cmt_write_cmcor(), which are both called just before
> > > sh_cmt_write_cmcnt()
> > > above (all out of diff context)?
> >
> > I am not sure on this.
> >
> > > > +       for (j = 0; j < 2; j++) {
> > > > +               for (k = 0; k < 100; k++) {
> > > > +                       if (!sh_cmt_read_cmcnt(ch))
> > > > +                               break;
> > > > +                       udelay(1);
> > > > +               }
> > >
> > > This is now two loops, with two checks for CMCNT, which looks
> > > strange to me.
> > > Do you have figures for the number of loops needed, for both the
> > > first (j=0) and the 2nd (j=1) cycle?
> >
> > From the test results, first cycle  j=0, loop count(max)=100 Second
> > cycle  j=2, loop count(max)= 37.
>
> 137µs ~= 3 x 42µs, and there are 3 register writes above.
> Is that a coincidence? ;-)

I think so.

> Does it work if you insert udelay(42) after the register writes in
> sh_cmt_write_cmcsr(), sh_cmt_write_cmcor(), and sh_cmt_write_cmcnt()?

yes,  I confirm it worked by putting  udelay(42) after the register writes in
sh_cmt_write_cmcsr(), sh_cmt_write_cmcor(), and sh_cmt_write_cmcnt().

Here  is the result after  inserting udelay (42) .

Koelsch board:-  with a max wait of (100+42) µs ( with more than 1000 iterations)
RZ/G1M board:- with a max value of (100+42) µs ( with more than 1000 iterations)
RZ/G1E board:-  with a max value of (86+42) µs (with more than 1000 iterations)

Looks like udelay(50)  is the safest, even though it worked with udelay(42). What do you think?

Regards,
Biju





Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.