Re: [PATCH v2] hw/m68k/mcf5208: add support for reset

[Thomas Huth]

Am Fri,  8 Mar 2024 10:08:12 +0100
schrieb Angelo Dureghello <angelo@kernel-space.org>:

> Add reset support for mcf5208.
> 
> Signed-off-by: Angelo Dureghello <angelo@kernel-space.org>
> ---
>  hw/m68k/mcf5208.c | 49 ++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 46 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/m68k/mcf5208.c b/hw/m68k/mcf5208.c
> index 0cfb806c20..d8a38274d0 100644
> --- a/hw/m68k/mcf5208.c
> +++ b/hw/m68k/mcf5208.c
> @@ -40,6 +40,8 @@
>  #define PCSR_PRE_SHIFT  8
>  #define PCSR_PRE_MASK   0x0f00
>  
> +#define RCR_SOFTRST     0x80
> +
>  typedef struct {
>      MemoryRegion iomem;
>      qemu_irq irq;
> @@ -185,13 +187,54 @@ static const MemoryRegionOps m5208_sys_ops = {
>      .endianness = DEVICE_NATIVE_ENDIAN,
>  };
>  
> -static void mcf5208_sys_init(MemoryRegion *address_space, qemu_irq *pic)
> +static uint64_t m5208_rcm_read(void *opaque, hwaddr addr,
> +                               unsigned size)
> +{
> +    qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset 0x%" HWADDR_PRIX "\n",
> +                  __func__, addr);

I'd maybe rather not print this message if the guest tries to read from RCR
or RSR - reading back a 0 should be fine here without logging a "bad
offset".

> +    return 0;
> +}
> +
> +static void m5208_rcm_write(void *opaque, hwaddr addr,
> +                            uint64_t value, unsigned size)
> +{
> +    M68kCPU *cpu = opaque;
> +    CPUState *cs = CPU(cpu);
> +    switch (addr) {
> +    case 0x0: /* RCR */
> +        if (value & RCR_SOFTRST) {
> +            cpu_reset(cs);
> +            cpu->env.aregs[7] = ldl_phys(cs->as, 0);
> +            cpu->env.pc = ldl_phys(cs->as, 4);
> +        }
> +        break;
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset 0x%" HWADDR_PRIX "\n",
> +                      __func__, addr);
> +        break;
> +    }
> +}
> +
> +static const MemoryRegionOps m5208_rcm_ops = {
> +    .read = m5208_rcm_read,
> +    .write = m5208_rcm_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
> +static void mcf5208_sys_init(MemoryRegion *address_space, qemu_irq *pic,
> +                             M68kCPU *cpu)
>  {
> -    MemoryRegion *iomem = g_new(MemoryRegion, 1);
> +    MemoryRegion *iomem;

I think it's more common coding style in the QEMU sources to have separate
variables for separate MemoryRegions instead of re-using one variable for
separate MemoryRegions like you do it below. See e.g.
microchip_pfsoc_soc_realize() in hw/riscv/microchip_pfsoc.c.

>      m5208_timer_state *s;
>      int i;
>  
> +    /* RCM */
> +    iomem = g_new(MemoryRegion, 1);
> +    memory_region_init_io(iomem, NULL, &m5208_rcm_ops, cpu,
> +                          "m5208-rcm", 0x00000080);
> +    memory_region_add_subregion(address_space, 0xfc0a0000, iomem);
>      /* SDRAMC.  */
> +    iomem = g_new(MemoryRegion, 1);
>      memory_region_init_io(iomem, NULL, &m5208_sys_ops, NULL, "m5208-sys", 
> 0x00004000);
>      memory_region_add_subregion(address_space, 0xfc0a8000, iomem);
>      /* Timers.  */
> @@ -265,7 +308,7 @@ static void mcf5208evb_init(MachineState *machine)
>      mcf_uart_create_mmap(0xfc064000, pic[27], serial_hd(1));
>      mcf_uart_create_mmap(0xfc068000, pic[28], serial_hd(2));
>  
> -    mcf5208_sys_init(address_space_mem, pic);
> +    mcf5208_sys_init(address_space_mem, pic, cpu);
>  
>      mcf_fec_init(address_space_mem, 0xfc030000, pic + 36);

All in all, patch looks fine to me ... if you could update the above two
nits by Monday, I can try to get this merged before the next soft freeze of
QEMU (on Tuesday).

 Thanks,
  Thomas