Hi Biju, On Tue, Oct 9, 2018 at 3:02 PM Biju Das <biju.das@xxxxxxxxxxxxxx> wrote: > What should we do next for fixing this error? Adding unconditional delays also fixes the issue. I'd add 50 µs delays after the three register writes. > But I do not have the setup to verify this on gen1/gen2/gen3 variants. > > I have enabled CMT0/1/2/3 on R-Car M3-W Salvator-XS board and this issue is not seen with original code. > > Only on R-Car Gen2/ RZ/G1, we are seeing this issue. As you can test on Koelsch, M3-W Salvator-XS, and RZ/G1, that should cover most variants we care about. CMT does not seem to be enabled on R-Car M1A and H1. I'l do boot tests on older SH/R-Mobile SoCs as part of my general testing. > Note:- > > For R-Car M3-W board, inconsistency-check and nanosleep tests are working fine. > > However there is a failure with clocksource_switch "asynchronous" test. > The inconsistency-check is failing for "arch_sys_counter" after some clocksource_switch operations > > So I skipped the "clocksource_switching" for arch_sys_counter and the asynchronous test is passing for > CMT0/1/2/3 timers. Sorry, being no timer expert, I don't understand the impact of the above paragraph. > > Subject: RE: [PATCH] clocksource/drivers/sh_cmt: wait for CMCNT on init > > > -----Original Message----- > > > From: Geert Uytterhoeven [mailto:geert@xxxxxxxxxxxxxx] > > > Sent: 11 June 2018 12:41 > > > To: Biju Das <biju.das@xxxxxxxxxxxxxx> > > > Cc: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>; Thomas Gleixner > > > <tglx@xxxxxxxxxxxxx>; Simon Horman <horms@xxxxxxxxxxxx>; Geert > > > Uytterhoeven <geert+renesas@xxxxxxxxx>; Chris Paterson > > > <Chris.Paterson2@xxxxxxxxxxx>; Fabrizio Castro > > > <fabrizio.castro@xxxxxxxxxxxxxx>; Linux-Renesas <linux-renesas- > > > soc@xxxxxxxxxxxxxxx> > > > 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@xxxxxxxxxxxxxx> > > 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@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. > > > > > > 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? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx 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
