Re: [PATCH v4 2/9] pc-bios: s390x: Consolidate timing functions into time.h

[Thomas Huth <thuth@redhat.com>]

On 22/06/2020 12.02, Thomas Huth wrote:
> On 22/06/2020 09.42, Janosch Frank wrote:
>> Let's consolidate timing related functions into one header.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>  pc-bios/s390-ccw/menu.c        |  1 +
>>  pc-bios/s390-ccw/netmain.c     | 15 +++------------
>>  pc-bios/s390-ccw/s390-ccw.h    |  8 ++++----
>>  pc-bios/s390-ccw/s390-time.h   | 23 +++++++++++++++++++++++
>>  pc-bios/s390-ccw/virtio-net.c  |  1 +
>>  pc-bios/s390-ccw/virtio-scsi.c |  1 +
>>  pc-bios/s390-ccw/virtio.c      | 18 +++---------------
>>  7 files changed, 36 insertions(+), 31 deletions(-)
>>  create mode 100644 pc-bios/s390-ccw/s390-time.h
>>
>> diff --git a/pc-bios/s390-ccw/menu.c b/pc-bios/s390-ccw/menu.c
>> index ce3815b201..de8260a5d6 100644
>> --- a/pc-bios/s390-ccw/menu.c
>> +++ b/pc-bios/s390-ccw/menu.c
>> @@ -12,6 +12,7 @@
>>  #include "libc.h"
>>  #include "s390-ccw.h"
>>  #include "sclp.h"
>> +#include "s390-time.h"
> 
> You could maybe drop these additional includes...
> 
>> diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
>> index 21f27e7990..fae1de363f 100644
>> --- a/pc-bios/s390-ccw/s390-ccw.h
>> +++ b/pc-bios/s390-ccw/s390-ccw.h
>> @@ -74,8 +74,6 @@ unsigned long virtio_load_direct(ulong rec_list1, ulong rec_list2,
>>  bool virtio_is_supported(SubChannelId schid);
>>  void virtio_blk_setup_device(SubChannelId schid);
>>  int virtio_read(ulong sector, void *load_addr);
>> -u64 get_clock(void);
>> -ulong get_second(void);
>>  
>>  /* bootmap.c */
>>  void zipl_load(void);
>> @@ -153,11 +151,13 @@ static inline void yield(void)
>>  
>>  #define MAX_SECTOR_SIZE 4096
>>  
>> +#include "s390-time.h"
> 
> ... since you already include s390-time.h here in this central header
> file...
> 
>>  static inline void sleep(unsigned int seconds)
>>  {
>> -    ulong target = get_second() + seconds;
>> +    ulong target = get_time_seconds() + seconds;
>>  
>> -    while (get_second() < target) {
>> +    while (get_time_seconds() < target) {
>>          yield();
>>      }
>>  }
> 
> ... or you could move the sleep() function into s390-time.h, too?

Never mind, I just saw that you move it in the next patch to a different
header. That's fine.

 Thomas