Nuclei-Software / nuclei-sdk

Nuclei RISC-V Software Development Kit
https://doc.nucleisys.com/nuclei_sdk
Apache License 2.0
117 stars 50 forks source link

[WIP] soc: Add clock() implementation #9

Closed tuupola closed 3 years ago

tuupola commented 3 years ago

Currently all calls to clock() return -1. This is default behavior when _times() is not implemented.

https://github.com/riscv/riscv-newlib/blob/riscv-newlib-3.2.0/newlib/libc/time/clock.c

This PR implements the _times() stub by storing the MCYCLE register value as the System CPU time. Based on newlib description below I assume this is correct.

DESCRIPTION _Calculates the best available approximation of the cumulative amount of time used by your program since it started. To convert the result into seconds, divide by the macro <>._

RETURNS _The amount of processor time used so far by your program, in units defined by the machine-dependent macro <>. If no measurement is available, the result is (clockt)<<-1>>.

There still is the problem that currently CLOCKS_PER_SEC is defined as 1000000 which is wrong.

https://github.com/riscv/riscv-newlib/blob/riscv-newlib-3.2.0/newlib/libc/include/machine/time.h#L5

It should be SystemCoreClock ie __SYSTEM_CLOCK_108M_PLL_HXTAL ie 108000000 but I am not sure in what file it should be defined.

fanghuaqi commented 3 years ago

Hi, thank you for your PR, as I can see, we have implemented _gettimeofday function, which can be used by _times functions defined here https://github.com/riscv/riscv-newlib/blob/riscv-newlib-3.2.0/libgloss/riscv/sys_times.c#L21-L36.

Maybe we don't need to reimplement this _times function, could you take a try?

Thanks Huaqi

tuupola commented 3 years ago

I am sure that could be used too. I think it will result in slightly slower code because of the extra conversion step. Current implementation of _gettimeofday() calls __get_rv_cycle(); to get the ticks and divides with clock to get seconds and microseconds. The _times() implementation you posted gets those seconds multiplies the with clock to convert back to the ticks again.

https://github.com/Nuclei-Software/nuclei-sdk/blob/master/SoC/gd32vf103/Common/Source/Stubs/gettimeofday.c

I will tinker around a bit to see how the existing _times() could be used.

tuupola commented 3 years ago

Scrap previous. Reading the code it seems using using the _times() from libgloss is preferred. Creating a stub by copying the code from libgloss makes clock() work and also fixes the CLOCKS_PER_SEC problem. I updated this PR with the copied code.

However copying the code feels wrong. Shouldn't the linker pick up the _times() function already defined in libgloss? Below is the output without applying this PR.

$ /opt/nuclei/gcc/bin/../lib/gcc/riscv-nuclei-elf/9.2.0/../../../../riscv-nuclei-elf/bin/ld: /opt/nuclei/gcc/bin/../lib/gcc/riscv-nuclei-elf/9.2.0/../../../../riscv-nuclei-elf/lib/rv32imac/ilp32/libg_nano.a(lib_a-timesr.o): in function `_times_r':
timesr.c:(.text._times_r+0x2): warning: _times is not implemented and will always fail

I am bit out of my depth here. I am not sure why linker does not use the _times() function from libgloss. Is it just a linker switch or is rebuilding the toolchain required?

https://github.com/riscv/riscv-newlib/blob/riscv-newlib-3.2.0/libgloss/riscv/sys_times.c#L21-L36

fanghuaqi commented 3 years ago

Hi @tuupola , I think this is caused by the linker didn't link the libgloss library, so user can define its own _times stub function.

tuupola commented 3 years ago

Ok. If the code is currently ok is there anything else I should do. Should I squash the commits and (force) push them back here. Or do you use squash merging?

fanghuaqi commented 3 years ago

Done