[PATCH] q800: move dp8393x_prom memory region to Q800MachineState

[PATCH] q800: move dp8393x_prom memory region to Q800MachineState

[Mark Cave-Ayland]

There is no need to dynamically allocate the memory region from the heap.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/m68k/q800.c         | 7 +++----
 include/hw/m68k/q800.h | 1 +
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
index 83d1571d02..b80a3b6d5f 100644
--- a/hw/m68k/q800.c
+++ b/hw/m68k/q800.c
@@ -253,7 +253,6 @@ static void q800_machine_init(MachineState *machine)
     int bios_size;
     ram_addr_t initrd_base;
     int32_t initrd_size;
-    MemoryRegion *dp8393x_prom = g_new(MemoryRegion, 1);
     uint8_t *prom;
     int i, checksum;
     MacFbMode *macfb_mode;
@@ -406,13 +405,13 @@ static void q800_machine_init(MachineState *machine)
     sysbus_connect_irq(sysbus, 0,
                        qdev_get_gpio_in(DEVICE(&m->glue), GLUE_IRQ_IN_SONIC));
 
-    memory_region_init_rom(dp8393x_prom, NULL, "dp8393x-q800.prom",
+    memory_region_init_rom(&m->dp8393x_prom, NULL, "dp8393x-q800.prom",
                            SONIC_PROM_SIZE, &error_fatal);
     memory_region_add_subregion(get_system_memory(), SONIC_PROM_BASE,
-                                dp8393x_prom);
+                                &m->dp8393x_prom);
 
     /* Add MAC address with valid checksum to PROM */
-    prom = memory_region_get_ram_ptr(dp8393x_prom);
+    prom = memory_region_get_ram_ptr(&m->dp8393x_prom);
     checksum = 0;
     for (i = 0; i < 6; i++) {
         prom[i] = revbit8(nd_table[0].macaddr.a[i]);
diff --git a/include/hw/m68k/q800.h b/include/hw/m68k/q800.h
index a9661f65f6..34365c9860 100644
--- a/include/hw/m68k/q800.h
+++ b/include/hw/m68k/q800.h
@@ -55,6 +55,7 @@ struct Q800MachineState {
     MOS6522Q800VIA1State via1;
     MOS6522Q800VIA2State via2;
     dp8393xState dp8393x;
+    MemoryRegion dp8393x_prom;
     ESCCState escc;
     OrIRQState escc_orgate;
     SysBusESPState esp;
-- 
2.39.2

Re: [PATCH] q800: move dp8393x_prom memory region to Q800MachineState

[Thomas Huth]

Am Wed, 27 Dec 2023 21:02:12 +0000
schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:

> There is no need to dynamically allocate the memory region from the heap.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/m68k/q800.c         | 7 +++----
>  include/hw/m68k/q800.h | 1 +
>  2 files changed, 4 insertions(+), 4 deletions(-)

Reviewed-by: Thomas Huth <huth@tuxfamily.org>

Re: [PATCH] q800: move dp8393x_prom memory region to Q800MachineState

[Philippe Mathieu-Daudé]

On 27/12/23 22:02, Mark Cave-Ayland wrote:
> There is no need to dynamically allocate the memory region from the heap.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/m68k/q800.c         | 7 +++----
>   include/hw/m68k/q800.h | 1 +
>   2 files changed, 4 insertions(+), 4 deletions(-)


> @@ -406,13 +405,13 @@ static void q800_machine_init(MachineState *machine)
>       sysbus_connect_irq(sysbus, 0,
>                          qdev_get_gpio_in(DEVICE(&m->glue), GLUE_IRQ_IN_SONIC));
>   
> -    memory_region_init_rom(dp8393x_prom, NULL, "dp8393x-q800.prom",
> +    memory_region_init_rom(&m->dp8393x_prom, NULL, "dp8393x-q800.prom",
>                              SONIC_PROM_SIZE, &error_fatal);
>       memory_region_add_subregion(get_system_memory(), SONIC_PROM_BASE,
> -                                dp8393x_prom);
> +                                &m->dp8393x_prom);
>   
>       /* Add MAC address with valid checksum to PROM */
> -    prom = memory_region_get_ram_ptr(dp8393x_prom);
> +    prom = memory_region_get_ram_ptr(&m->dp8393x_prom);
>       checksum = 0;
>       for (i = 0; i < 6; i++) {
>           prom[i] = revbit8(nd_table[0].macaddr.a[i]);

Similar pattern in mips_jazz_init(). I wonder if we can extract the
PROM checksums in a common helper declared in "hw/net/dp8393x.h".

Anyhow for this patch:
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH] q800: move dp8393x_prom memory region to Q800MachineState

[Laurent Vivier]

Le 27/12/2023 à 22:02, Mark Cave-Ayland a écrit :
> There is no need to dynamically allocate the memory region from the heap.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/m68k/q800.c         | 7 +++----
>   include/hw/m68k/q800.h | 1 +
>   2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
> index 83d1571d02..b80a3b6d5f 100644
> --- a/hw/m68k/q800.c
> +++ b/hw/m68k/q800.c
> @@ -253,7 +253,6 @@ static void q800_machine_init(MachineState *machine)
>       int bios_size;
>       ram_addr_t initrd_base;
>       int32_t initrd_size;
> -    MemoryRegion *dp8393x_prom = g_new(MemoryRegion, 1);
>       uint8_t *prom;
>       int i, checksum;
>       MacFbMode *macfb_mode;
> @@ -406,13 +405,13 @@ static void q800_machine_init(MachineState *machine)
>       sysbus_connect_irq(sysbus, 0,
>                          qdev_get_gpio_in(DEVICE(&m->glue), GLUE_IRQ_IN_SONIC));
>   
> -    memory_region_init_rom(dp8393x_prom, NULL, "dp8393x-q800.prom",
> +    memory_region_init_rom(&m->dp8393x_prom, NULL, "dp8393x-q800.prom",
>                              SONIC_PROM_SIZE, &error_fatal);
>       memory_region_add_subregion(get_system_memory(), SONIC_PROM_BASE,
> -                                dp8393x_prom);
> +                                &m->dp8393x_prom);
>   
>       /* Add MAC address with valid checksum to PROM */
> -    prom = memory_region_get_ram_ptr(dp8393x_prom);
> +    prom = memory_region_get_ram_ptr(&m->dp8393x_prom);
>       checksum = 0;
>       for (i = 0; i < 6; i++) {
>           prom[i] = revbit8(nd_table[0].macaddr.a[i]);
> diff --git a/include/hw/m68k/q800.h b/include/hw/m68k/q800.h
> index a9661f65f6..34365c9860 100644
> --- a/include/hw/m68k/q800.h
> +++ b/include/hw/m68k/q800.h
> @@ -55,6 +55,7 @@ struct Q800MachineState {
>       MOS6522Q800VIA1State via1;
>       MOS6522Q800VIA2State via2;
>       dp8393xState dp8393x;
> +    MemoryRegion dp8393x_prom;
>       ESCCState escc;
>       OrIRQState escc_orgate;
>       SysBusESPState esp;

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Re: [PATCH] q800: move dp8393x_prom memory region to Q800MachineState

[Mark Cave-Ayland]

On 28/12/2023 09:46, Philippe Mathieu-Daudé wrote:

> On 27/12/23 22:02, Mark Cave-Ayland wrote:
>> There is no need to dynamically allocate the memory region from the heap.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   hw/m68k/q800.c         | 7 +++----
>>   include/hw/m68k/q800.h | 1 +
>>   2 files changed, 4 insertions(+), 4 deletions(-)
> 
> 
>> @@ -406,13 +405,13 @@ static void q800_machine_init(MachineState *machine)
>>       sysbus_connect_irq(sysbus, 0,
>>                          qdev_get_gpio_in(DEVICE(&m->glue), GLUE_IRQ_IN_SONIC));
>> -    memory_region_init_rom(dp8393x_prom, NULL, "dp8393x-q800.prom",
>> +    memory_region_init_rom(&m->dp8393x_prom, NULL, "dp8393x-q800.prom",
>>                              SONIC_PROM_SIZE, &error_fatal);
>>       memory_region_add_subregion(get_system_memory(), SONIC_PROM_BASE,
>> -                                dp8393x_prom);
>> +                                &m->dp8393x_prom);
>>       /* Add MAC address with valid checksum to PROM */
>> -    prom = memory_region_get_ram_ptr(dp8393x_prom);
>> +    prom = memory_region_get_ram_ptr(&m->dp8393x_prom);
>>       checksum = 0;
>>       for (i = 0; i < 6; i++) {
>>           prom[i] = revbit8(nd_table[0].macaddr.a[i]);
> 
> Similar pattern in mips_jazz_init(). I wonder if we can extract the
> PROM checksums in a common helper declared in "hw/net/dp8393x.h".

That used to be the case before I added the support for the q800 machine, however the 
encoding of the MAC address and checksum are completely different between the jazz 
and q800 machines. The misleading part is the memory regions have been created with a 
_prom suffix and AFAICT the SONIC doesn't have any on-board NVRAM at all, so my guess 
is that the MAC address is stored in per-machine non-volatile memory.

> Anyhow for this patch:
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Thanks!


ATB,

Mark.