Am Fri, 8 Mar 2024 10:08:12 +0100
schrieb Angelo Dureghello <ang...@kernel-space.org>:
> Add reset support for mcf5208.
>
> Signed-off-by: Angelo Dureghello <ang...@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
