| Message ID | 1478208701-30923-2-git-send-email-vgupta@synopsys.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Nov 03, 2016 at 02:31:32PM -0700, Vineet Gupta wrote: > The current code doesn't even compile .... Give a better description in the log, especially if this patch is supposed to go to stable@ > CC: stable@vger.kernel.org > Signed-off-by: Vineet Gupta <vgupta@synopsys.com> > --- > arch/arc/kernel/time.c | 19 +++++++++++-------- > 1 file changed, 11 insertions(+), 8 deletions(-) > > diff --git a/arch/arc/kernel/time.c b/arch/arc/kernel/time.c > index f927b8dc6edd..1a117b999c0c 100644 > --- a/arch/arc/kernel/time.c > +++ b/arch/arc/kernel/time.c > @@ -152,14 +152,17 @@ static cycle_t arc_read_rtc(struct clocksource *cs) > cycle_t full; > } stamp; > > - > - __asm__ __volatile( > - "1: \n" > - " lr %0, [AUX_RTC_LOW] \n" > - " lr %1, [AUX_RTC_HIGH] \n" > - " lr %2, [AUX_RTC_CTRL] \n" > - " bbit0.nt %2, 31, 1b \n" > - : "=r" (stamp.low), "=r" (stamp.high), "=r" (status)); > + /* > + * hardware has an internal state machine which tracks readout of > + * low/high and updates the CTRL.status if > + * - interrupt/exception taken between the two reads > + * - high increments after low has been read > + */ > + do { > + stamp.low = read_aux_reg(AUX_RTC_LOW); > + stamp.high = read_aux_reg(AUX_RTC_HIGH); > + status = read_aux_reg(AUX_RTC_CTRL); > + } while (!(status & _BITUL(31))); Is the condition correct ? If I refer to your previous answer, the bit will be set for status if the counter wrapped up. So in this case, we won't exit the loop until we wrap up, no ? > return stamp.full; > } > -- > 2.7.4 >
On 11/03/2016 02:52 PM, Daniel Lezcano wrote: > On Thu, Nov 03, 2016 at 02:31:32PM -0700, Vineet Gupta wrote: >> The current code doesn't even compile .... > > Give a better description in the log, especially if this patch is supposed to > go to stable@ OK. > >> CC: stable@vger.kernel.org >> Signed-off-by: Vineet Gupta <vgupta@synopsys.com> >> --- >> arch/arc/kernel/time.c | 19 +++++++++++-------- >> 1 file changed, 11 insertions(+), 8 deletions(-) >> >> diff --git a/arch/arc/kernel/time.c b/arch/arc/kernel/time.c >> index f927b8dc6edd..1a117b999c0c 100644 >> --- a/arch/arc/kernel/time.c >> +++ b/arch/arc/kernel/time.c >> @@ -152,14 +152,17 @@ static cycle_t arc_read_rtc(struct clocksource *cs) >> cycle_t full; >> } stamp; >> >> - >> - __asm__ __volatile( >> - "1: \n" >> - " lr %0, [AUX_RTC_LOW] \n" >> - " lr %1, [AUX_RTC_HIGH] \n" >> - " lr %2, [AUX_RTC_CTRL] \n" >> - " bbit0.nt %2, 31, 1b \n" >> - : "=r" (stamp.low), "=r" (stamp.high), "=r" (status)); >> + /* >> + * hardware has an internal state machine which tracks readout of >> + * low/high and updates the CTRL.status if >> + * - interrupt/exception taken between the two reads >> + * - high increments after low has been read >> + */ >> + do { >> + stamp.low = read_aux_reg(AUX_RTC_LOW); >> + stamp.high = read_aux_reg(AUX_RTC_HIGH); >> + status = read_aux_reg(AUX_RTC_CTRL); >> + } while (!(status & _BITUL(31))); > > Is the condition correct ? If I refer to your previous answer, the bit will be > set for status if the counter wrapped up. So in this case, we won't exit the > loop until we wrap up, no ? No thats not what I meant. Bit being set there means things are fine (no interrupt taken, no increment of high after low was readetc). All I changed here was use of 0x8000_0000 to the macro. BBIT0 in assembler means branch if bit was clear. > >> return stamp.full; >> } >> -- >> 2.7.4 >>
On Thu, Nov 03, 2016 at 03:23:09PM -0700, Vineet Gupta wrote: > On 11/03/2016 02:52 PM, Daniel Lezcano wrote: > > On Thu, Nov 03, 2016 at 02:31:32PM -0700, Vineet Gupta wrote: > >> The current code doesn't even compile .... > > > > Give a better description in the log, especially if this patch is supposed to > > go to stable@ > > OK. [ ... ] > > Is the condition correct ? If I refer to your previous answer, the bit will be > > set for status if the counter wrapped up. So in this case, we won't exit the > > loop until we wrap up, no ? > > No thats not what I meant. Bit being set there means things are fine (no interrupt > taken, no increment of high after low was readetc). All I changed here was use of > 0x8000_0000 to the macro. BBIT0 in assembler means branch if bit was clear. Fair enough. So the logic is inverted 'status' == 0 means 'not fine'.
On 11/03/2016 03:35 PM, Daniel Lezcano wrote: > On Thu, Nov 03, 2016 at 03:23:09PM -0700, Vineet Gupta wrote: >> On 11/03/2016 02:52 PM, Daniel Lezcano wrote: >>> On Thu, Nov 03, 2016 at 02:31:32PM -0700, Vineet Gupta wrote: >>>> The current code doesn't even compile .... >>> >>> Give a better description in the log, especially if this patch is supposed to >>> go to stable@ >> >> OK. > > [ ... ] Here's what I added ----> ARC: timer: rtc: implement read loop in "C" vs. inline asm The current code doesn't even compile as somehow the inline assembly can't see the register names defined as ARC_RTC_* I'm pretty sure It worked when I first got it merged, but the tools were definitely different then. So better to write this in "C" anyways. > >>> Is the condition correct ? If I refer to your previous answer, the bit will be >>> set for status if the counter wrapped up. So in this case, we won't exit the >>> loop until we wrap up, no ? >> >> No thats not what I meant. Bit being set there means things are fine (no interrupt >> taken, no increment of high after low was readetc). All I changed here was use of >> 0x8000_0000 to the macro. BBIT0 in assembler means branch if bit was clear. > > Fair enough. So the logic is inverted 'status' == 0 means 'not fine'. Indeed !
On Thu, Nov 03, 2016 at 03:44:24PM -0700, Vineet Gupta wrote: > On 11/03/2016 03:35 PM, Daniel Lezcano wrote: > > On Thu, Nov 03, 2016 at 03:23:09PM -0700, Vineet Gupta wrote: > >> On 11/03/2016 02:52 PM, Daniel Lezcano wrote: > >>> On Thu, Nov 03, 2016 at 02:31:32PM -0700, Vineet Gupta wrote: > >>>> The current code doesn't even compile .... > >>> > >>> Give a better description in the log, especially if this patch is supposed to > >>> go to stable@ > >> > >> OK. > > > > [ ... ] > > Here's what I added > > ----> > ARC: timer: rtc: implement read loop in "C" vs. inline asm > > The current code doesn't even compile as somehow the inline assembly > can't see the register names defined as ARC_RTC_* > I'm pretty sure It worked when I first got it merged, but the tools were > definitely different then. > > So better to write this in "C" anyways. Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Hi Vineet, > -----Original Message----- > From: Vineet Gupta [mailto:vgupta@synopsys.com] > Sent: Friday, November 04, 2016 12:32 AM > To: Daniel Lezcano <daniel.lezcano@linaro.org> > Cc: Noam Camus <noamca@mellanox.com>; tglx@linutronix.de; linux-snps-arc@lists.infradead.org; linux-kernel@vger.kernel.org; > Alexey.Brodkin@synopsys.com; Vineet Gupta <vgupta@synopsys.com>; stable@vger.kernel.org > Subject: [PATCH v2 01/10] ARC: timer: rtc: implement read loop in "C" vs. inline asm > > The current code doesn't even compile .... > > CC: stable@vger.kernel.org > Signed-off-by: Vineet Gupta <vgupta@synopsys.com> > --- > arch/arc/kernel/time.c | 19 +++++++++++-------- > 1 file changed, 11 insertions(+), 8 deletions(-) > > diff --git a/arch/arc/kernel/time.c b/arch/arc/kernel/time.c > index f927b8dc6edd..1a117b999c0c 100644 > --- a/arch/arc/kernel/time.c > +++ b/arch/arc/kernel/time.c > @@ -152,14 +152,17 @@ static cycle_t arc_read_rtc(struct clocksource *cs) > cycle_t full; > } stamp; > > - > - __asm__ __volatile( > - "1: \n" > - " lr %0, [AUX_RTC_LOW] \n" > - " lr %1, [AUX_RTC_HIGH] \n" > - " lr %2, [AUX_RTC_CTRL] \n" > - " bbit0.nt %2, 31, 1b \n" > - : "=r" (stamp.low), "=r" (stamp.high), "=r" (status)); > + /* > + * hardware has an internal state machine which tracks readout of > + * low/high and updates the CTRL.status if > + * - interrupt/exception taken between the two reads > + * - high increments after low has been read > + */ > + do { > + stamp.low = read_aux_reg(AUX_RTC_LOW); > + stamp.high = read_aux_reg(AUX_RTC_HIGH); > + status = read_aux_reg(AUX_RTC_CTRL); > + } while (!(status & _BITUL(31))); I think original Daniel's comment was about constant value used here. Now with "_BITUL" it already looks much better but IMHO it would be even better if 31 gets converted here to something like: ------------------------>8-------------------- #define RTC_CTRL_A1_OFFSET 31 ------------------------>8-------------------- -Alexey
diff --git a/arch/arc/kernel/time.c b/arch/arc/kernel/time.c index f927b8dc6edd..1a117b999c0c 100644 --- a/arch/arc/kernel/time.c +++ b/arch/arc/kernel/time.c @@ -152,14 +152,17 @@ static cycle_t arc_read_rtc(struct clocksource *cs) cycle_t full; } stamp; - - __asm__ __volatile( - "1: \n" - " lr %0, [AUX_RTC_LOW] \n" - " lr %1, [AUX_RTC_HIGH] \n" - " lr %2, [AUX_RTC_CTRL] \n" - " bbit0.nt %2, 31, 1b \n" - : "=r" (stamp.low), "=r" (stamp.high), "=r" (status)); + /* + * hardware has an internal state machine which tracks readout of + * low/high and updates the CTRL.status if + * - interrupt/exception taken between the two reads + * - high increments after low has been read + */ + do { + stamp.low = read_aux_reg(AUX_RTC_LOW); + stamp.high = read_aux_reg(AUX_RTC_HIGH); + status = read_aux_reg(AUX_RTC_CTRL); + } while (!(status & _BITUL(31))); return stamp.full; }
The current code doesn't even compile .... CC: stable@vger.kernel.org Signed-off-by: Vineet Gupta <vgupta@synopsys.com> --- arch/arc/kernel/time.c | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-)