Hi Geert,
Thanks for the feedback.
> Subject: Re: [PATCH] clocksource/drivers/sh_cmt: wait for CMCNT on init
>
> Hi Biju,
>
> Thanks for your patch!
>
> On Fri, Jun 8, 2018 at 6:24 PM Biju Das <biju.das@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
> > Reviewed-by: Chris Paterson <chris.paterson2@xxxxxxxxxxx>
> > ---
> > 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.
Regards,
Biju
Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.
