Re: [PATCH v5 1/8] Implement STM32L4x5_RCC skeleton

[Arnaud Minier]

Thanks Peter for the review,

----- Original Message -----
> From: "Peter Maydell" 
> To: "Arnaud Minier" 
> Cc: "qemu-devel" , "Thomas Huth" , "Laurent Vivier" , "Inès
> Varhol" , "Samuel Tardieu" , "qemu-arm"
> , "Alistair Francis" , "Paolo Bonzini" , "Alistair
> Francis" 
> Sent: Friday, February 23, 2024 3:26:46 PM
> Subject: Re: [PATCH v5 1/8] Implement STM32L4x5_RCC skeleton

> On Mon, 19 Feb 2024 at 20:11, Arnaud Minier
>  wrote:
>>
>> Add the necessary files to add a simple RCC implementation with just
>> reads from and writes to registers. Also instanciate the RCC in the
> 
> "instantiate"

Fixed.

> 
>> STM32L4x5_SoC. It is needed for accurate emulation of all the SoC
>> clocks and timers.
>>
>> Signed-off-by: Arnaud Minier 
>> Signed-off-by: Inès Varhol 
>> Acked-by: Alistair Francis 
>> ---
> 
> 
> 
>> +static const MemoryRegionOps stm32l4x5_rcc_ops = {
>> +    .read = stm32l4x5_rcc_read,
>> +    .write = stm32l4x5_rcc_write,
>> +    .endianness = DEVICE_NATIVE_ENDIAN,
>> +    .valid = {
>> +        .max_access_size = 4,
>> +        .unaligned = false
>> +    },
>> +};
> 
> What's the .valid.min_access_size ?
> Do we need to set the .impl max/min access size here too ?

I honestly don't really understand the differences between .valid and .impl.
However, since all the code assumes that 4-byte accesses are made,
I think we can set all these values to 4 for now.

> 
> 
>> +
>> +static const ClockPortInitArray stm32l4x5_rcc_clocks = {
>> +    QDEV_CLOCK_IN(Stm32l4x5RccState, hsi16_rc, NULL, 0),
>> +    QDEV_CLOCK_IN(Stm32l4x5RccState, msi_rc, NULL, 0),
>> +    QDEV_CLOCK_IN(Stm32l4x5RccState, hse, NULL, 0),
>> +    QDEV_CLOCK_IN(Stm32l4x5RccState, lsi_rc, NULL, 0),
>> +    QDEV_CLOCK_IN(Stm32l4x5RccState, lse_crystal, NULL, 0),
>> +    QDEV_CLOCK_IN(Stm32l4x5RccState, sai1_extclk, NULL, 0),
>> +    QDEV_CLOCK_IN(Stm32l4x5RccState, sai2_extclk, NULL, 0),
>> +    QDEV_CLOCK_END
>> +};
> 
> These are input clocks, so they each need a VMSTATE_CLOCK()
> line in the VMStateDescription. (I think only input clocks
> need to be migrated.)

Sure, will add these in the VMStateDescription.

> 
>> +
>> +
>> +static void stm32l4x5_rcc_init(Object *obj)
>> +{
>> +    Stm32l4x5RccState *s = STM32L4X5_RCC(obj);
>> +
>> +    sysbus_init_irq(SYS_BUS_DEVICE(obj), &s->irq);
>> +
>> +    memory_region_init_io(&s->mmio, obj, &stm32l4x5_rcc_ops, s,
>> +                          TYPE_STM32L4X5_RCC, 0x400);
>> +    sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->mmio);
>> +
>> +    qdev_init_clocks(DEVICE(s), stm32l4x5_rcc_clocks);
>> +
>> +    s->gnd = clock_new(obj, "gnd");
>> +}
> 
> Otherwise
> Reviewed-by: Peter Maydell 
> 
> thanks
> -- PMM