diff mbox series

[kvm-unit-tests,v8,06/12] s390x: clock and delays caluculations

Message ID 1591603981-16879-7-git-send-email-pmorel@linux.ibm.com (mailing list archive)
State New, archived
Headers show
Series s390x: Testing the Channel Subsystem I/O | expand

Commit Message

Pierre Morel June 8, 2020, 8:12 a.m. UTC
The hardware gives us a good definition of the microsecond,
let's keep this information and let the routine accessing
the hardware keep all the information and return microseconds.

Calculate delays in microseconds and take care about wrapping
around zero.

Define values with macros and use inlines to keep the
milliseconds interface.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 lib/s390x/asm/time.h | 29 +++++++++++++++++++++++++++--
 1 file changed, 27 insertions(+), 2 deletions(-)

Comments

Thomas Huth June 8, 2020, 3:55 p.m. UTC | #1
On 08/06/2020 10.12, Pierre Morel wrote:
> The hardware gives us a good definition of the microsecond,
> let's keep this information and let the routine accessing
> the hardware keep all the information and return microseconds.
> 
> Calculate delays in microseconds and take care about wrapping
> around zero.
> 
> Define values with macros and use inlines to keep the
> milliseconds interface.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  lib/s390x/asm/time.h | 29 +++++++++++++++++++++++++++--
>  1 file changed, 27 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/s390x/asm/time.h b/lib/s390x/asm/time.h
> index 1791380..eb15941 100644
> --- a/lib/s390x/asm/time.h
> +++ b/lib/s390x/asm/time.h
> @@ -13,14 +13,39 @@
>  #ifndef ASM_S390X_TIME_H
>  #define ASM_S390X_TIME_H
>  
> -static inline uint64_t get_clock_ms(void)
> +#define STCK_SHIFT	(63 - 51)

Could you maybe rather call this STCK_SHIFT_US instead, so that it is
clear that we refer to the microsecond bit here?

> +#define STCK_MAX	((1 << (STCK_SHIFT + 1)) - 1)

Hmm, is that really right? Shouldn't that rather be

 ((1 << (51 + 1)) - 1)

instead?

(you want to have a max. value for the 52 bits that count the
microseconds, not a max value for the remainder bits, do you?)

> +static inline uint64_t get_clock_us(void)
>  {
>  	uint64_t clk;
>  
>  	asm volatile(" stck %0 " : : "Q"(clk) : "memory");
>  
>  	/* Bit 51 is incrememented each microsecond */
> -	return (clk >> (63 - 51)) / 1000;
> +	return clk >> STCK_SHIFT;
> +}
> +
> +static inline void udelay(unsigned long us)
> +{
> +	unsigned long startclk = get_clock_us();
> +	unsigned long c;
> +
> +	do {
> +		c = get_clock_us();
> +		if (c < startclk)
> +			c += STCK_MAX;
> +	} while (c < (startclk + us));

You could omit the parentheses around "startclk + us" here.

> +}
> +
> +static inline void mdelay(unsigned long ms)
> +{
> +	udelay(ms * 1000);
> +}
> +
> +static inline uint64_t get_clock_ms(void)
> +{
> +	return get_clock_us() / 1000;
>  }
>  
>  #endif
> 

 Thomas
Pierre Morel June 8, 2020, 4:16 p.m. UTC | #2
On 2020-06-08 17:55, Thomas Huth wrote:
> On 08/06/2020 10.12, Pierre Morel wrote:
>> The hardware gives us a good definition of the microsecond,
>> let's keep this information and let the routine accessing
>> the hardware keep all the information and return microseconds.
>>
>> Calculate delays in microseconds and take care about wrapping
>> around zero.
>>
>> Define values with macros and use inlines to keep the
>> milliseconds interface.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   lib/s390x/asm/time.h | 29 +++++++++++++++++++++++++++--
>>   1 file changed, 27 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/s390x/asm/time.h b/lib/s390x/asm/time.h
>> index 1791380..eb15941 100644
>> --- a/lib/s390x/asm/time.h
>> +++ b/lib/s390x/asm/time.h
>> @@ -13,14 +13,39 @@
>>   #ifndef ASM_S390X_TIME_H
>>   #define ASM_S390X_TIME_H
>>   
>> -static inline uint64_t get_clock_ms(void)
>> +#define STCK_SHIFT	(63 - 51)
> 
> Could you maybe rather call this STCK_SHIFT_US instead, so that it is
> clear that we refer to the microsecond bit here?
> 
>> +#define STCK_MAX	((1 << (STCK_SHIFT + 1)) - 1)
> 
> Hmm, is that really right? Shouldn't that rather be
> 
>   ((1 << (51 + 1)) - 1)
> 
> instead?

:) ooops, right.

> 
> (you want to have a max. value for the 52 bits that count the
> microseconds, not a max value for the remainder bits, do you?)
> 
>> +static inline uint64_t get_clock_us(void)
>>   {
>>   	uint64_t clk;
>>   
>>   	asm volatile(" stck %0 " : : "Q"(clk) : "memory");
>>   
>>   	/* Bit 51 is incrememented each microsecond */
>> -	return (clk >> (63 - 51)) / 1000;
>> +	return clk >> STCK_SHIFT;
>> +}
>> +
>> +static inline void udelay(unsigned long us)
>> +{
>> +	unsigned long startclk = get_clock_us();
>> +	unsigned long c;
>> +
>> +	do {
>> +		c = get_clock_us();
>> +		if (c < startclk)
>> +			c += STCK_MAX;
>> +	} while (c < (startclk + us));
> 
> You could omit the parentheses around "startclk + us" here.

OK

Thanks,
Pierre
diff mbox series

Patch

diff --git a/lib/s390x/asm/time.h b/lib/s390x/asm/time.h
index 1791380..eb15941 100644
--- a/lib/s390x/asm/time.h
+++ b/lib/s390x/asm/time.h
@@ -13,14 +13,39 @@ 
 #ifndef ASM_S390X_TIME_H
 #define ASM_S390X_TIME_H
 
-static inline uint64_t get_clock_ms(void)
+#define STCK_SHIFT	(63 - 51)
+#define STCK_MAX	((1 << (STCK_SHIFT + 1)) - 1)
+
+static inline uint64_t get_clock_us(void)
 {
 	uint64_t clk;
 
 	asm volatile(" stck %0 " : : "Q"(clk) : "memory");
 
 	/* Bit 51 is incrememented each microsecond */
-	return (clk >> (63 - 51)) / 1000;
+	return clk >> STCK_SHIFT;
+}
+
+static inline void udelay(unsigned long us)
+{
+	unsigned long startclk = get_clock_us();
+	unsigned long c;
+
+	do {
+		c = get_clock_us();
+		if (c < startclk)
+			c += STCK_MAX;
+	} while (c < (startclk + us));
+}
+
+static inline void mdelay(unsigned long ms)
+{
+	udelay(ms * 1000);
+}
+
+static inline uint64_t get_clock_ms(void)
+{
+	return get_clock_us() / 1000;
 }
 
 #endif