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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 11.03.2025 17:19, Oleksii Kurochko wrote:
> --- /dev/null
> +++ b/xen/arch/riscv/time.c
> @@ -0,0 +1,38 @@
> +#include <xen/device_tree.h>
> +#include <xen/init.h>
> +#include <xen/lib.h>
> +#include <xen/sections.h>
> +
> +unsigned long __ro_after_init cpu_khz; /* CPU clock frequency in kHz. */
> +unsigned long __read_mostly boot_count;

Why not also __ro_after_init? And what is this variable actually needed
for? Common code doesn't use it, so a better name (describing what it
really holds) might be desirable, even if this then means not being in
sync with Arm code.

Furthermore, I can't spot a declaration of this variable. Was it meant
to be static?

> +static __initdata struct dt_device_node *timer;
> +
> +/* 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  __initconst timer_ids[] =

Nit: Stray double blank.

> +    {
> +        DT_MATCH_PATH("/cpus"),
> +        { /* sentinel */ },
> +    };
> +    u32 rate;

Nit: uint32_t please in new code.

> +    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");

Please be consistent with the full stop in panic messages. (I think there
shouldn't be any.)

Jan