Support variable clock frequency in SYSINFO

[rapgenic]

Is your feature request related to a problem? Please describe.
I am implementing NEORV32 in my own project, where the core starts up with a low frequency crystal generated clock, i.e. 12 MHz, configures an external IC to generate a faster clock and then (through a wishbone register connected via XBUS) switches the clock source from the slow to the fast one.

At the moment the CLOCK_FREQUENCY parameter is a synthesis constant. This causes the timing related peripherals and HALs to work correctly only at one of the two speeds (i.e. UART, delay). I have created my own uart_setup function to account for this, but it is not optimal to change all the HAL functions

Describe the solution you'd like
In my specific case, the various clock frequencies are known at synthesis time, so it would be nice to be able to specify the frequency as a signal instead of as a generic, to be changed at clock switch, so that the code always knows the correct clock frequency.

Describe alternatives you've considered
Modifying/duplicating the HALs to have a variable clock frequency.

[stnolting]

In my specific case, the various clock frequencies are known at synthesis time, so it would be nice to be able to specify the frequency as a signal instead of as a generic, to be changed at clock switch, so that the code always knows the correct clock frequency.

That sounds like a good idea. 👍

So implementing SYSINFO's CLK as actual read/write register should solve this problem, right?

[rapgenic]

So implementing SYSINFO's CLK as actual read/write register should solve this problem, right?

For my specific use case is more than needed (it would be enough to be able to just change it in hardware directly) but actually your solution is equivalent, with the difference that it would be changed by firmware rather than in hardware... Not a problem either way 👍

[stnolting]

it would be enough to be able to just change it in hardware directly

I am not quite sure what you mean with this. 🤔

If you want to change the value in the CLK register during runtime the only option (I see) is to make it an actual register that can be overridden by the application software.

Modifying/duplicating the HALs to have a variable clock frequency.

I also like this idea. Right now, SYSINFO is a quite small module requiring only a hand full of LUTs. Adding an actual 32-bit register to it would increase the hardware utilization.

[rapgenic]

If you want to change the value in the CLK register during runtime the only option (I see) is to make it an actual register that can be overridden by the application software.

I actually have a signal that drives a the clock switch instance. That same signal could drive a mux that assigns to the CLOCK_FREQUENCY signal the corresponding clock frequency.

But that's an oddly specific and little useful use case.

Actually working at HAL level is what STM32 does with their own cores, if I'm not mistaken they have a static variable used by all HALs and set by the clock config routine.

The only downside, then, is that the actual CLOCK_FREQUENCY register would be a little misleading due to the fact that it would not reflect the system state

[stnolting]

I actually have a signal that drives a the clock switch instance. That same signal could drive a mux that assigns to the CLOCK_FREQUENCY signal the corresponding clock frequency.

That would mean that we need to convert the CLOCK_FREQUENCY generic into an actual input port, right?

Actually working at HAL level is what STM32 does with their own cores, if I'm not mistaken they have a static variable used by all HALs and set by the clock config routine.

That's a nice solution!

The only downside, then, is that the actual CLOCK_FREQUENCY register would be a little misleading due to the fact that it would not reflect the system state

That's also a good point.

So I think I would prefer to turn SYSINFO's CLK into an actual register that can be overridden by the application software. What do you think?

[stnolting]

I thought about this again. Basically, we have three options (and I like all of them 😅):

Option 1 - Clock speed as additional functions argument

Add the clock speed as another argument to all clock-related HAL functions, e.g. configuring the UART baud rate or using busy-wait functions.

Pros/Cons:

• ✔️ no hardware modification, no additional hardware overhead
• ❌ HAL is not backwards-compatible
• ❌ harder to use; might be error-prone
• ✔️ consistent and transparent
• ✔️ thread-safe

Functions affected:

• neorv32_cpu_delay_ms()
• neorv32_mtime_set_unixtime() and neorv32_mtime_get_unixtime()
• neorv32_spi_get_clock_speed()
• neorv32_xip_get_clock_speed()
• neorv32_onewire_setup()
• neorv32_uart_setup()
• neorv32_neoled_setup_ws2812()

Option 2 - Global variable for clock speed

Use a global variable to store the (current) clock frequency and consume that by all clock-related functions. This variable could be initialized by the crt0 start-up code using the NEORV32_SYSINFO->CLK registers(and thus, the CLOCK_FREQUENCY generic). Application software can override this variable at any time.

Similar to this: https://www.keil.com/pack/doc/CMSIS_Dev/Core/html/group__system__init__gr.html

Pros/Cons:

• ✔️ no hardware modification, no additional hardware overhead
• ✔️ HAL remains unchanged
• ✔️ easy to use
• ❌ might be non-transparent for the user; correct setup of the global variable requires the NEORV32 crt0 (and maybe the default linker script); hard to port to other setups that just use the HAL drivers, but not the rest of the SW framework
• ❌ not thread-safe

Option 3 - Make NEORV32_SYSINFO.CLK writable

Make NEORV32_SYSINFO->CLK an actual read/write register that can be overriden by the application software.

Pros/Cons:

• ❌ hardware modification with additional hardware overhead (that might not be used by many/most implementations that have a fixed clock speed)
• ✔️ HAL stays unchanged
• ✔️ easy to use
• ❌ maybe inconsistent as SYSINFO reflects static configurations derived from the generics
• ❌ not thread-safe

Any opinions on this? 🤔

[rapgenic]

Hi @stnolting sorry for the delay.

Option 1 - Clock speed as additional functions argument

This one I like the least, mainly because of HAL incompatibility, plus I suspect not many people would need to change the core clock frequency, so it would add a lot of noise to the function signatures.

Option 2 - Global variable for clock speed

This is IMO the best one, because it respects the readonly nature of a sys INFO register (I would say that information registers shouldn't be editable, by their definition, as you also have noticed). Plus anyone that has used ARM cores should be more or less familiar with this method.

The drawback I didn't think about is actually not being thread safe, but I also think that is is minor, since I expect the change of clock frequency to be a delicate operation performed in a setup phase, when thread safety should not be that much of a concern.

Option 3 - Make NEORV32_SYSINFO.CLK writable

I like this less, as I said, because I expect info registers not to be editable. (Also for HW usage if you target small FPGAs) But other than that it is not that different than a static variable somewhere in RAM (at least from a user perspective), so for me both option 2 and 3 are great!

[stnolting]

sorry for the delay.

No worries!

Option 1 - Clock speed as additional functions argument

This one I like the least, mainly because of HAL incompatibility, plus I suspect not many people would need to change the core clock frequency, so it would add a lot of noise to the function signatures.

I agree. Let's try not to break backwards compatibility. 😉

Now how about options 2 and 3... I'm not sure which option is the better one.

A global variable is nice because it does not add any further hardware overhead while providing advanced features. But besides being not thread-safa, this variable is hard to protect when you run a secure RTOS (e.g. you would need a single PMP entry to protect this variable).

Furthermore, where do we initialize this variable? Do we need to call a dedicated setup function right at the beginning of main? Do we add a specific constructor for that? Or do we initialize it from crt0?

If we just define another (volatile) global variable, will the compiler trim that if no function is referencing it?

Option 3 would be the easiest to implement. But yes, I agree that SYSINFO register should be read-only as the are intended to reflect static configuration (via generics) only.