[PATCH v2 01/10] ARC: timer: rtc: implement read loop in "C" vs. inline asm

[PATCH v2 01/10] ARC: timer: rtc: implement read loop in "C" vs. inline asm

[Vineet Gupta]

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)));
 
 	return stamp.full;
 }
-- 
2.7.4

Re: [PATCH v2 01/10] ARC: timer: rtc: implement read loop in "C" vs. inline asm

[Daniel Lezcano]

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
> 

Re: [PATCH v2 01/10] ARC: timer: rtc: implement read loop in "C" vs. inline asm

[Vineet Gupta]

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
>>

Re: [PATCH v2 01/10] ARC: timer: rtc: implement read loop in "C" vs. inline asm

[Daniel Lezcano]

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'.

Re: [PATCH v2 01/10] ARC: timer: rtc: implement read loop in "C" vs. inline asm

[Vineet Gupta]

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 !

Re: [PATCH v2 01/10] ARC: timer: rtc: implement read loop in "C" vs. inline asm

[Daniel Lezcano]

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>

RE: [PATCH v2 01/10] ARC: timer: rtc: implement read loop in "C" vs. inline asm

[Alexey Brodkin]

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