Re: [PATCH 5/5] hw/riscv: Configurable MPFS CLINT timebase freq

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 14/2/25 07:24, Sebastian Huber wrote:
> This property enables the setting of the CLINT timebase frequency
> through the command line, for example:
> 
>    -machine microchip-icicle-kit,clint-timebase-frequency=10000000
> 
> Signed-off-by: Sebastian Huber <sebastian.huber@embedded-brains.de>
> ---
>   hw/riscv/microchip_pfsoc.c         | 49 +++++++++++++++++++++++++++---
>   include/hw/riscv/microchip_pfsoc.h |  1 +
>   2 files changed, 46 insertions(+), 4 deletions(-)

OK, so:
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Although few comments related to pre-existing code inlined.

> 
> diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c
> index 76a2c56419..c83d588962 100644
> --- a/hw/riscv/microchip_pfsoc.c
> +++ b/hw/riscv/microchip_pfsoc.c
> @@ -39,6 +39,7 @@
>   #include "qemu/units.h"
>   #include "qemu/cutils.h"
>   #include "qapi/error.h"
> +#include "qapi/visitor.h"
>   #include "hw/boards.h"
>   #include "hw/loader.h"
>   #include "hw/sysbus.h"
> @@ -61,9 +62,6 @@
>   #define BIOS_FILENAME   "hss.bin"
>   #define RESET_VECTOR    0x20220000
>   
> -/* CLINT timebase frequency */
> -#define CLINT_TIMEBASE_FREQ 1000000
> -
>   /* GEM version */
>   #define GEM_REVISION    0x0107010c
>   
> @@ -193,6 +191,7 @@ static void microchip_pfsoc_soc_instance_init(Object *obj)
>   static void microchip_pfsoc_soc_realize(DeviceState *dev, Error **errp)
>   {
>       MachineState *ms = MACHINE(qdev_get_machine());
> +    MicrochipIcicleKitState *iks = MICROCHIP_ICICLE_KIT_MACHINE(ms);
>       MicrochipPFSoCState *s = MICROCHIP_PFSOC(dev);
>       const MemMapEntry *memmap = microchip_pfsoc_memmap;
>       MemoryRegion *system_memory = get_system_memory();
> @@ -253,7 +252,7 @@ static void microchip_pfsoc_soc_realize(DeviceState *dev, Error **errp)
>           memmap[MICROCHIP_PFSOC_CLINT].base + RISCV_ACLINT_SWI_SIZE,
>           RISCV_ACLINT_DEFAULT_MTIMER_SIZE, 0, ms->smp.cpus,
>           RISCV_ACLINT_DEFAULT_MTIMECMP, RISCV_ACLINT_DEFAULT_MTIME,
> -        CLINT_TIMEBASE_FREQ, false);
> +        iks->clint_timebase_freq, false);

1/ Ideally TYPE_RISCV_ACLINT_MTIMER should take a Clock input.

>   
>       /* L2 cache controller */
>       create_unimplemented_device("microchip.pfsoc.l2cc",
> @@ -665,6 +664,40 @@ static void microchip_icicle_kit_machine_init(MachineState *machine)
>       }
>   }
>   
> +static void microchip_icicle_kit_set_clint_timebase_freq(Object *obj,
> +                                                         Visitor *v,
> +                                                         const char *name,
> +                                                         void *opaque,
> +                                                         Error **errp)
> +{
> +    MicrochipIcicleKitState *s = MICROCHIP_ICICLE_KIT_MACHINE(obj);
> +    uint32_t value;
> +
> +    if (!visit_type_uint32(v, name, &value, errp)) {

2/ Could we use qemu_strtosz_metric() here?

Better would be to implement visit_type_frequency(), similar
to visit_type_size(), to parse a 'Hz' suffix.

> +        return;
> +    }
> +
> +    s->clint_timebase_freq = value;
> +}
> +
> +static void microchip_icicle_kit_get_clint_timebase_freq(Object *obj,
> +                                                         Visitor *v,
> +                                                         const char *name,
> +                                                         void *opaque,
> +                                                         Error **errp)
> +{
> +    MicrochipIcicleKitState *s = MICROCHIP_ICICLE_KIT_MACHINE(obj);
> +    uint32_t value = s->clint_timebase_freq;
> +
> +    visit_type_uint32(v, name, &value, errp);
> +}
> +
> +static void microchip_icicle_kit_machine_instance_init(Object *obj)
> +{
> +    MicrochipIcicleKitState *m = MICROCHIP_ICICLE_KIT_MACHINE(obj);
> +    m->clint_timebase_freq = 1000000;
> +}

3/ If MachineState were inheriting QDev, we could use qdev-properties.h
API. I'm not sure why we have to use duplicated QOM boiler plate code
here.

Peter, Paolo, Markus, do you have views on this?

Thanks,

Phil.

>   static void microchip_icicle_kit_machine_class_init(ObjectClass *oc, void *data)
>   {
>       MachineClass *mc = MACHINE_CLASS(oc);
> @@ -685,12 +718,20 @@ static void microchip_icicle_kit_machine_class_init(ObjectClass *oc, void *data)
>        * See memory_tests() in mss_ddr.c in the HSS source code.
>        */
>       mc->default_ram_size = 1537 * MiB;
> +
> +    object_class_property_add(oc, "clint-timebase-frequency", "uint32_t",
> +                              microchip_icicle_kit_get_clint_timebase_freq,
> +                              microchip_icicle_kit_set_clint_timebase_freq,
> +                              NULL, NULL);
> +    object_class_property_set_description(oc, "clint-timebase-frequency",
> +                                  "Set CLINT timebase frequency in Hz.");
>   }
>   
>   static const TypeInfo microchip_icicle_kit_machine_typeinfo = {
>       .name       = MACHINE_TYPE_NAME("microchip-icicle-kit"),
>       .parent     = TYPE_MACHINE,
>       .class_init = microchip_icicle_kit_machine_class_init,
> +    .instance_init = microchip_icicle_kit_machine_instance_init,
>       .instance_size = sizeof(MicrochipIcicleKitState),
>   };
>   
> diff --git a/include/hw/riscv/microchip_pfsoc.h b/include/hw/riscv/microchip_pfsoc.h
> index daef086da6..7ca9b976c1 100644
> --- a/include/hw/riscv/microchip_pfsoc.h
> +++ b/include/hw/riscv/microchip_pfsoc.h
> @@ -67,6 +67,7 @@ typedef struct MicrochipIcicleKitState {
>       MachineState parent_obj;
>   
>       /*< public >*/
> +    uint32_t clint_timebase_freq;
>       MicrochipPFSoCState soc;
>   } MicrochipIcicleKitState;
>