1. Patchew
  2. QEMU
  3. View series

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

Angelo Dureghello posted 1 patch 1 month, 2 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20240308090812.1316635-1-angelo@kernel-space.org
Maintainers: Thomas Huth <huth@tuxfamily.org>
There is a newer version of this series
hw/m68k/mcf5208.c | 49 ++++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 46 insertions(+), 3 deletions(-)
[PATCH v2] hw/m68k/mcf5208: add support for reset
Posted by Angelo Dureghello 1 month, 2 weeks ago 
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);
+    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;
     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);
 
-- 
2.44.0
Re: [PATCH v2] hw/m68k/mcf5208: add support for reset
Posted by Thomas Huth 1 month, 2 weeks ago 
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
Re: [PATCH v2] hw/m68k/mcf5208: add support for reset
Posted by Angelo Dureghello 1 month, 2 weeks ago 
Hi Thomas,

On 09/03/24 10:06 AM, Thomas Huth wrote:
> 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 a lot, sending v3.

> 
>   Thanks,
>    Thomas

Regards,
angelo

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.