Re: [PATCH v2 1/4] xen/riscv: introduce preinit_xen_time()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.03.2025 20:49, Oleksii Kurochko wrote:
> On 3/26/25 4:13 PM, Jan Beulich wrote:
>> On 25.03.2025 18:36, Oleksii Kurochko wrote:
>>> +/* Set up the timer on the boot CPU (early init function) */
>>> +static void __init preinit_dt_xen_time(void)
>>> +{
>>> +    static const struct dt_device_match __initconstrel timer_ids[] =
>>> +    {
>>> +        DT_MATCH_PATH("/cpus"),
>>> +        { /* sentinel */ },
>>> +    };
>>> +    uint32_t rate;
>>> +
>>> +    timer = dt_find_matching_node(NULL, timer_ids);
>>> +    if ( !timer )
>>> +        panic("Unable to find a compatible timer in the device tree\n");
>>> +
>>> +    dt_device_set_used_by(timer, DOMID_XEN);
>>> +
>>> +    if ( !dt_property_read_u32(timer, "timebase-frequency", &rate) )
>>> +        panic("Unable to find clock frequency\n");
>>> +
>>> +    cpu_khz = rate / 1000;
>> "rate" being only 32 bits wide, what about clock rates above 4GHz? Or is
>> this some external clock running at a much lower rate than the CPU would?
> 
> It is the frequency of the hardware timer that drives the 
> |mtime|register, it defines the frequency (in Hz) at which the timer 
> increments.

And that timer can't plausibly run at more than 4 GHz?

>>> +void __init preinit_xen_time(void)
>>> +{
>>> +    preinit_dt_xen_time();
>>> +
>>> +    xen_start_clock_cycles = get_cycles();
>>> +}
>> I take it that more stuff is going to be added to this function? Else I
>> wouldn't see why preinit_dt_xen_time() needed to be a separate function.
> 
> Actually, I checked my latest downstream branch and I haven't added nothing
> extra to this function.
> Only one thing that I want to add is ACPI case:
>      if ( acpi_disabled )
>          preinit_dt_xen_time();
>      else
>          panic("TBD\n"); /* preinit_acpi_xen_time(); */

That's probably a good enough reason then to keep the function separate.

Jan