| Message ID | 1528474785-11778-1-git-send-email-biju.das@bp.renesas.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
Hello! On 6/8/2018 7:19 PM, Biju Das wrote: > As per section 57A.3.5/69A.3.5/79.A.3.5 of rz/g/r-car gen2/3 hardware 79A.3.5? > manual,it is mentioned that we need to provide 2 cycles in counter input ^ need space > clock (RCLK) for reflecting written data to counter behaviour. Adding > sufficient wait to let the CMCNT register value settle before starting the > timer channel. > > 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? > > regards, > Biju > > drivers/clocksource/sh_cmt.c | 16 +++++++++++----- > 1 file changed, 11 insertions(+), 5 deletions(-) > > diff --git a/drivers/clocksource/sh_cmt.c b/drivers/clocksource/sh_cmt.c > index 70b3cf8..48910df 100644 > --- 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); > + for (j = 0; j < 2; j++) { > + for (k = 0; k < 100; k++) { Why not simply count to 200 ISO 100? > + if (!sh_cmt_read_cmcnt(ch)) > + break; > + udelay(1); > + } > } > > if (sh_cmt_read_cmcnt(ch)) { MBR, Seregi
Hi Biju, Thanks for your patch! 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? Or should it wait 42µs unconditionally, i.e. without checking CMCNT? 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)? > + 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? Thanks! Gr{oetje,eeting}s, Geert
Hi Sergie, Thanks for the feedback. > Subject: Re: [PATCH] clocksource/drivers/sh_cmt: wait for CMCNT on init > > Hello! > > On 6/8/2018 7:19 PM, Biju Das wrote: > > > As per section 57A.3.5/69A.3.5/79.A.3.5 of rz/g/r-car gen2/3 hardware > > 79A.3.5? Will fix this. > > manual,it is mentioned that we need to provide 2 cycles in counter > > input > ^ need space Will fix this. > > clock (RCLK) for reflecting written data to counter behaviour. Adding > > sufficient wait to let the CMCNT register value settle before starting > > the timer channel. > > > > 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? > > > > regards, > > Biju > > > > drivers/clocksource/sh_cmt.c | 16 +++++++++++----- > > 1 file changed, 11 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/clocksource/sh_cmt.c > > b/drivers/clocksource/sh_cmt.c index 70b3cf8..48910df 100644 > > --- 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); > > +for (j = 0; j < 2; j++) { > > +for (k = 0; k < 100; k++) { > > Why not simply count to 200 ISO 100? This issue is related to settling of CMCNT register during init. During my testing, I found that the first loop exit immediately before actually settling this register value to "0". ie, the instantaneous value of register becomes "0" in the first check and it breaks the first loop and in the next check report the error. The new logic is to double check this register value and make sure that this register value is settled to "0", before starting the timer channel. Regards, Biju Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.
> > Subject: Re: [PATCH] clocksource/drivers/sh_cmt: wait for CMCNT on > > > diff --git a/drivers/clocksource/sh_cmt.c > > > b/drivers/clocksource/sh_cmt.c index 70b3cf8..48910df 100644 > > > --- 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); > > > +for (j = 0; j < 2; j++) { > > > +for (k = 0; k < 100; k++) { > > > > Why not simply count to 200 ISO 100?> That also works. It needs delay more than 100. The worst case wait is around 130-150 microseconds. Previously, I have put a print statement just before break statement during debugging. > This issue is related to settling of CMCNT register during init. > > During my testing, I found that the first loop exit immediately before actually > settling this register value to "0". ie, the instantaneous value of register > becomes "0" in the first check and it breaks the first loop and in the next > check report the error. > > The new logic is to double check this register value and make sure that this > register value is settled to "0", before starting the timer channel. > > Regards, > Biju Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.
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@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. Regards, Biju Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.
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? ;-) 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()? Thanks! Gr{oetje,eeting}s, Geert
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.
Hi Geert, What should we do next for fixing this error? Adding unconditional delays also fixes the issue. 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. 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. Regards, Biju > Subject: RE: [PATCH] clocksource/drivers/sh_cmt: wait for CMCNT on init > > 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.
Hi Biju, On Tue, Oct 9, 2018 at 3:02 PM Biju Das <biju.das@bp.renesas.com> 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@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? Gr{oetje,eeting}s, Geert
Hi Geert, Thanks for the feedback. > Subject: Re: [PATCH] clocksource/drivers/sh_cmt: wait for CMCNT on init > > > 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. It make sense to me as well, since the hardware manual is mentioning about inserting delays for the reliable counter operation. > > 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. I will send a patch V2, after testing on all these variants. > 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. We may start a discussion on this, when we start upstreaming CMT for R-Car M3-W devices. Basically clocksource_switch test in "selftests "uses 2 threads, In 1 thread it executes inconsistency-check test followed by nano sleep test and on the second thread it keep changing clock source one after the another. This test always fails on R-Car M3-W which is based on arm64. Regards, Biju Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.
Hi Biju, On Wed, Oct 10, 2018 at 11:50 AM Biju Das <biju.das@bp.renesas.com> wrote: > > > 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. > > We may start a discussion on this, when we start upstreaming CMT for R-Car M3-W devices. > > Basically clocksource_switch test in "selftests "uses 2 threads, > In 1 thread it executes inconsistency-check test followed by nano sleep test > and on the second thread it keep changing clock source one after the another. > > This test always fails on R-Car M3-W which is based on arm64. Sergei already enabled CMT on R-Car V3M and V3H, perhaps he has seen similar issues? Gr{oetje,eeting}s, Geert
diff --git a/drivers/clocksource/sh_cmt.c b/drivers/clocksource/sh_cmt.c index 70b3cf8..48910df 100644 --- 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); + for (j = 0; j < 2; j++) { + for (k = 0; k < 100; k++) { + if (!sh_cmt_read_cmcnt(ch)) + break; + udelay(1); + } } if (sh_cmt_read_cmcnt(ch)) {