[PATCH-for-8.0 0/7] hw/mips: Make gt64xxx_pci.c endian-agnostic

[PATCH-for-8.0 0/7] hw/mips: Make gt64xxx_pci.c endian-agnostic

[Philippe Mathieu-Daudé]

Respining an old/unfinished series... Add the 'cpu-little-endian'
qdev property to the GT64120 north bridge so [target-specific]
machines can set its endianness, allowing it to be endian agnostic.

Philippe Mathieu-Daudé (7):
  hw/mips/Kconfig: Introduce CONFIG_GT64120 to select gt64xxx_pci.c
  hw/mips/gt64xxx_pci: Let the GT64120 manage the lower 512MiB hole
  hw/mips/gt64xxx_pci: Manage endian bits with the RegisterField API
  hw/mips/gt64xxx_pci: Add a 'cpu-little-endian' qdev property
  hw/mips/malta: Explicit GT64120 endianness upon device creation
  hw/mips/meson: Make gt64xxx_pci.c endian-agnostic
  hw/mips/gt64xxx_pci: Move it to hw/pci-host/

 MAINTAINERS                                   |  2 +-
 configs/devices/mips-softmmu/common.mak       |  1 -
 hw/mips/Kconfig                               |  1 +
 hw/mips/malta.c                               | 11 ++--
 hw/mips/meson.build                           |  2 +-
 hw/mips/trace-events                          |  6 ---
 hw/pci-host/Kconfig                           |  6 +++
 hw/{mips/gt64xxx_pci.c => pci-host/gt64120.c} | 54 ++++++++++++++-----
 hw/pci-host/meson.build                       |  1 +
 hw/pci-host/trace-events                      |  7 +++
 meson.build                                   |  1 -
 11 files changed, 62 insertions(+), 30 deletions(-)
 delete mode 100644 hw/mips/trace-events
 rename hw/{mips/gt64xxx_pci.c => pci-host/gt64120.c} (95%)

-- 
2.38.1

[PATCH-for-8.0 1/7] hw/mips/Kconfig: Introduce CONFIG_GT64120 to select gt64xxx_pci.c

[Philippe Mathieu-Daudé]

From: Philippe Mathieu-Daudé <philmd@redhat.com>

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/mips/Kconfig     | 6 ++++++
 hw/mips/meson.build | 3 ++-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/hw/mips/Kconfig b/hw/mips/Kconfig
index 725525358d..d6bbbe7069 100644
--- a/hw/mips/Kconfig
+++ b/hw/mips/Kconfig
@@ -1,5 +1,6 @@
 config MALTA
     bool
+    select GT64120
     select ISA_SUPERIO
 
 config MIPSSIM
@@ -59,3 +60,8 @@ config MIPS_BOSTON
 
 config FW_CFG_MIPS
     bool
+
+config GT64120
+    bool
+    select PCI
+    select I8259
diff --git a/hw/mips/meson.build b/hw/mips/meson.build
index dd0101ad4d..6ccd385df0 100644
--- a/hw/mips/meson.build
+++ b/hw/mips/meson.build
@@ -2,7 +2,8 @@ mips_ss = ss.source_set()
 mips_ss.add(files('bootloader.c', 'mips_int.c'))
 mips_ss.add(when: 'CONFIG_FW_CFG_MIPS', if_true: files('fw_cfg.c'))
 mips_ss.add(when: 'CONFIG_LOONGSON3V', if_true: files('loongson3_bootp.c', 'loongson3_virt.c'))
-mips_ss.add(when: 'CONFIG_MALTA', if_true: files('gt64xxx_pci.c', 'malta.c'))
+mips_ss.add(when: 'CONFIG_MALTA', if_true: files('malta.c'))
+mips_ss.add(when: 'CONFIG_GT64120', if_true: files('gt64xxx_pci.c'))
 mips_ss.add(when: 'CONFIG_MIPS_CPS', if_true: files('cps.c'))
 
 if 'CONFIG_TCG' in config_all
-- 
2.38.1

Re: [PATCH-for-8.0 1/7] hw/mips/Kconfig: Introduce CONFIG_GT64120 to select gt64xxx_pci.c

[Bernhard Beschow]

Am 9. Dezember 2022 15:15:27 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>From: Philippe Mathieu-Daudé <philmd@redhat.com>
>
>Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>---
> hw/mips/Kconfig     | 6 ++++++
> hw/mips/meson.build | 3 ++-
> 2 files changed, 8 insertions(+), 1 deletion(-)
>
>diff --git a/hw/mips/Kconfig b/hw/mips/Kconfig
>index 725525358d..d6bbbe7069 100644
>--- a/hw/mips/Kconfig
>+++ b/hw/mips/Kconfig
>@@ -1,5 +1,6 @@
> config MALTA
>     bool
>+    select GT64120
>     select ISA_SUPERIO
> 
> config MIPSSIM
>@@ -59,3 +60,8 @@ config MIPS_BOSTON
> 
> config FW_CFG_MIPS
>     bool
>+
>+config GT64120
>+    bool
>+    select PCI
>+    select I8259

s/select I8259/depends on I8259/ since the model needs but doesn't provide I8259? Then just take my mail regarding the last patch as a reminder.

Otherwise:
Reviewed-by: Bernhard Beschow <shentey@gmail.com>

>diff --git a/hw/mips/meson.build b/hw/mips/meson.build
>index dd0101ad4d..6ccd385df0 100644
>--- a/hw/mips/meson.build
>+++ b/hw/mips/meson.build
>@@ -2,7 +2,8 @@ mips_ss = ss.source_set()
> mips_ss.add(files('bootloader.c', 'mips_int.c'))
> mips_ss.add(when: 'CONFIG_FW_CFG_MIPS', if_true: files('fw_cfg.c'))
> mips_ss.add(when: 'CONFIG_LOONGSON3V', if_true: files('loongson3_bootp.c', 'loongson3_virt.c'))
>-mips_ss.add(when: 'CONFIG_MALTA', if_true: files('gt64xxx_pci.c', 'malta.c'))
>+mips_ss.add(when: 'CONFIG_MALTA', if_true: files('malta.c'))
>+mips_ss.add(when: 'CONFIG_GT64120', if_true: files('gt64xxx_pci.c'))
> mips_ss.add(when: 'CONFIG_MIPS_CPS', if_true: files('cps.c'))
> 
> if 'CONFIG_TCG' in config_all

Re: [PATCH-for-8.0 1/7] hw/mips/Kconfig: Introduce CONFIG_GT64120 to select gt64xxx_pci.c

[Philippe Mathieu-Daudé]

On 12/12/22 01:13, Bernhard Beschow wrote:
> 
> 
> Am 9. Dezember 2022 15:15:27 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>> From: Philippe Mathieu-Daudé <philmd@redhat.com>
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>> hw/mips/Kconfig     | 6 ++++++
>> hw/mips/meson.build | 3 ++-
>> 2 files changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/mips/Kconfig b/hw/mips/Kconfig
>> index 725525358d..d6bbbe7069 100644
>> --- a/hw/mips/Kconfig
>> +++ b/hw/mips/Kconfig
>> @@ -1,5 +1,6 @@
>> config MALTA
>>      bool
>> +    select GT64120
>>      select ISA_SUPERIO
>>
>> config MIPSSIM
>> @@ -59,3 +60,8 @@ config MIPS_BOSTON
>>
>> config FW_CFG_MIPS
>>      bool
>> +
>> +config GT64120
>> +    bool
>> +    select PCI
>> +    select I8259
> 
> s/select I8259/depends on I8259/ since the model needs but doesn't provide I8259? Then just take my mail regarding the last patch as a reminder.

I try to remember the 'depends on' directive as "depends on BUS".
If there is no BUS, then the option is pointless. Here "select PCI"
means 'provide/expose a PCI bus on the machine'.

I8259 must be available for GT64120 to be working. If you need a
GT64120, its 'select' directive will select the minimum options
required, so it will implicitly select a I8259.

See docs/devel/kconfig.rst:

**dependencies**: ``depends on <expr>``

   This defines a dependency for this configurable element. Dependencies
   evaluate an expression and force the value of the variable to false
   if the expression is false.

**reverse dependencies**: ``select <symbol> [if <expr>]``

   While ``depends on`` can force a symbol to false, reverse dependencies
   can be used to force another symbol to true.

**devices**

   Example::

     config MEGASAS_SCSI_PCI
       bool
       default y if PCI_DEVICES
       depends on PCI
       select SCSI

   Devices are the most complex of the five.  They can have a variety
   of directives that cooperate so that a default configuration includes
   all the devices that can be accessed from QEMU.

   Devices *depend on* the bus that they lie on, for example a PCI
   device would specify ``depends on PCI``.  An MMIO device will likely
   have no ``depends on`` directive.  Devices also *select* the buses
   that the device provides, for example a SCSI adapter would specify
   ``select SCSI``.  Finally, devices are usually ``default y`` if and
   only if they have at least one ``depends on``; the default could be
   conditional on a device group.

> Otherwise:
> Reviewed-by: Bernhard Beschow <shentey@gmail.com>

Thanks!

Re: [PATCH-for-8.0 1/7] hw/mips/Kconfig: Introduce CONFIG_GT64120 to select gt64xxx_pci.c

[Bernhard Beschow]

Am 12. Dezember 2022 08:02:26 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>On 12/12/22 01:13, Bernhard Beschow wrote:
>> 
>> 
>> Am 9. Dezember 2022 15:15:27 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>>> From: Philippe Mathieu-Daudé <philmd@redhat.com>
>>> 
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>> ---
>>> hw/mips/Kconfig     | 6 ++++++
>>> hw/mips/meson.build | 3 ++-
>>> 2 files changed, 8 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/hw/mips/Kconfig b/hw/mips/Kconfig
>>> index 725525358d..d6bbbe7069 100644
>>> --- a/hw/mips/Kconfig
>>> +++ b/hw/mips/Kconfig
>>> @@ -1,5 +1,6 @@
>>> config MALTA
>>>      bool
>>> +    select GT64120
>>>      select ISA_SUPERIO
>>> 
>>> config MIPSSIM
>>> @@ -59,3 +60,8 @@ config MIPS_BOSTON
>>> 
>>> config FW_CFG_MIPS
>>>      bool
>>> +
>>> +config GT64120
>>> +    bool
>>> +    select PCI
>>> +    select I8259
>> 
>> s/select I8259/depends on I8259/ since the model needs but doesn't provide I8259? Then just take my mail regarding the last patch as a reminder.
>
>I try to remember the 'depends on' directive as "depends on BUS".
>If there is no BUS, then the option is pointless. Here "select PCI"
>means 'provide/expose a PCI bus on the machine'.
>
>I8259 must be available for GT64120 to be working. If you need a
>GT64120, its 'select' directive will select the minimum options
>required, so it will implicitly select a I8259.
>
>See docs/devel/kconfig.rst:
>
>**dependencies**: ``depends on <expr>``
>
>  This defines a dependency for this configurable element. Dependencies
>  evaluate an expression and force the value of the variable to false
>  if the expression is false.
>
>**reverse dependencies**: ``select <symbol> [if <expr>]``
>
>  While ``depends on`` can force a symbol to false, reverse dependencies
>  can be used to force another symbol to true.
>
>**devices**
>
>  Example::
>
>    config MEGASAS_SCSI_PCI
>      bool
>      default y if PCI_DEVICES
>      depends on PCI
>      select SCSI
>
>  Devices are the most complex of the five.  They can have a variety
>  of directives that cooperate so that a default configuration includes
>  all the devices that can be accessed from QEMU.
>
>  Devices *depend on* the bus that they lie on, for example a PCI
>  device would specify ``depends on PCI``.  An MMIO device will likely
>  have no ``depends on`` directive.

Yes, I agree that the patch follows this wording, so I should have given my R-b regardless (fixed below).

I was more contemplating aloud whether above wording could be interpretet as: I8259 is a device which provides an "interrupt bus" and GT64120 consumes that bus but doesn't provide I8259, hence "GT64120 depends on I8259".

Of course, a classical PIC doesn't provide a bus in the usual sense while an APIC actually does, though this isn't modelled in QEMU that way. In a more abstract view one could see devices as providing and consuming communication channels (via PCI, MMIO, ...), or even implementing and consuming interfaces, in which case above conclusion might make more sense. After all, it seems to me that today implementation details decide whether something should be "select" or "depends on".

Anyway, this is getting off-topic - just some food for thought. Sorry for the noise...

>  Devices also *select* the buses
>  that the device provides, for example a SCSI adapter would specify
>  ``select SCSI``.  Finally, devices are usually ``default y`` if and
>  only if they have at least one ``depends on``; the default could be
>  conditional on a device group.
>
>> Otherwise:
>> Reviewed-by: Bernhard Beschow <shentey@gmail.com>

That should actually be:
Anyway,
Reviewed-by: Bernhard Beschow <shentey@gmail.com>

>
>Thanks!

Re: [PATCH-for-8.0 1/7] hw/mips/Kconfig: Introduce CONFIG_GT64120 to select gt64xxx_pci.c

[Richard Henderson]

On 12/9/22 07:15, Philippe Mathieu-Daudé wrote:
> From: Philippe Mathieu-Daudé<philmd@redhat.com>
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@redhat.com>
> ---
>   hw/mips/Kconfig     | 6 ++++++
>   hw/mips/meson.build | 3 ++-
>   2 files changed, 8 insertions(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH-for-8.0 2/7] hw/mips/gt64xxx_pci: Let the GT64120 manage the lower 512MiB hole

[Philippe Mathieu-Daudé]

From: Philippe Mathieu-Daudé <f4bug@amsat.org>

Per the comment in the Malta board, the [0x0000.0000-0x2000.0000]
range is decoded by the GT64120, so move the "empty_slot" there.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 configs/devices/mips-softmmu/common.mak | 1 -
 hw/mips/Kconfig                         | 1 +
 hw/mips/gt64xxx_pci.c                   | 8 ++++++++
 hw/mips/malta.c                         | 7 -------
 4 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/configs/devices/mips-softmmu/common.mak b/configs/devices/mips-softmmu/common.mak
index 416161f833..c2b5f322fc 100644
--- a/configs/devices/mips-softmmu/common.mak
+++ b/configs/devices/mips-softmmu/common.mak
@@ -26,7 +26,6 @@ CONFIG_IDE_ISA=y
 CONFIG_PFLASH_CFI01=y
 CONFIG_I8259=y
 CONFIG_MC146818RTC=y
-CONFIG_EMPTY_SLOT=y
 CONFIG_MIPS_CPS=y
 CONFIG_MIPS_ITU=y
 CONFIG_MALTA=y
diff --git a/hw/mips/Kconfig b/hw/mips/Kconfig
index d6bbbe7069..8f7bce38fb 100644
--- a/hw/mips/Kconfig
+++ b/hw/mips/Kconfig
@@ -64,4 +64,5 @@ config FW_CFG_MIPS
 config GT64120
     bool
     select PCI
+    select EMPTY_SLOT
     select I8259
diff --git a/hw/mips/gt64xxx_pci.c b/hw/mips/gt64xxx_pci.c
index 19d0d9889f..1b9ac7f792 100644
--- a/hw/mips/gt64xxx_pci.c
+++ b/hw/mips/gt64xxx_pci.c
@@ -28,6 +28,7 @@
 #include "qemu/log.h"
 #include "hw/pci/pci.h"
 #include "hw/pci/pci_host.h"
+#include "hw/misc/empty_slot.h"
 #include "migration/vmstate.h"
 #include "hw/intc/i8259.h"
 #include "hw/irq.h"
@@ -1162,6 +1163,13 @@ static void gt64120_realize(DeviceState *dev, Error **errp)
                                 PCI_DEVFN(18, 0), TYPE_PCI_BUS);
 
     pci_create_simple(phb->bus, PCI_DEVFN(0, 0), "gt64120_pci");
+
+    /*
+     * The whole address space decoded by the GT-64120A doesn't generate
+     * exception when accessing invalid memory. Create an empty slot to
+     * emulate this feature.
+     */
+    empty_slot_init("GT64120", 0, 0x20000000);
 }
 
 static void gt64120_pci_realize(PCIDevice *d, Error **errp)
diff --git a/hw/mips/malta.c b/hw/mips/malta.c
index c0a2e0ab04..ba92022f87 100644
--- a/hw/mips/malta.c
+++ b/hw/mips/malta.c
@@ -53,7 +53,6 @@
 #include "sysemu/runstate.h"
 #include "qapi/error.h"
 #include "qemu/error-report.h"
-#include "hw/misc/empty_slot.h"
 #include "sysemu/kvm.h"
 #include "semihosting/semihost.h"
 #include "hw/mips/cps.h"
@@ -1393,12 +1392,6 @@ void mips_malta_init(MachineState *machine)
     /* Northbridge */
     dev = sysbus_create_simple("gt64120", -1, NULL);
     pci_bus = PCI_BUS(qdev_get_child_bus(dev, "pci"));
-    /*
-     * The whole address space decoded by the GT-64120A doesn't generate
-     * exception when accessing invalid memory. Create an empty slot to
-     * emulate this feature.
-     */
-    empty_slot_init("GT64120", 0, 0x20000000);
 
     /* Southbridge */
     piix4 = pci_create_simple_multifunction(pci_bus, PCI_DEVFN(10, 0), true,
-- 
2.38.1

Re: [PATCH-for-8.0 2/7] hw/mips/gt64xxx_pci: Let the GT64120 manage the lower 512MiB hole

[Richard Henderson]

On 12/9/22 07:15, Philippe Mathieu-Daudé wrote:
> From: Philippe Mathieu-Daudé<f4bug@amsat.org>
> 
> Per the comment in the Malta board, the [0x0000.0000-0x2000.0000]
> range is decoded by the GT64120, so move the "empty_slot" there.
> 
> Signed-off-by: Philippe Mathieu-Daudé<f4bug@amsat.org>
> ---
>   configs/devices/mips-softmmu/common.mak | 1 -
>   hw/mips/Kconfig                         | 1 +
>   hw/mips/gt64xxx_pci.c                   | 8 ++++++++
>   hw/mips/malta.c                         | 7 -------
>   4 files changed, 9 insertions(+), 8 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH-for-8.0 3/7] hw/mips/gt64xxx_pci: Manage endian bits with the RegisterField API

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/mips/gt64xxx_pci.c | 37 +++++++++++++++++++++++++------------
 1 file changed, 25 insertions(+), 12 deletions(-)

diff --git a/hw/mips/gt64xxx_pci.c b/hw/mips/gt64xxx_pci.c
index 1b9ac7f792..8c9ec80f7c 100644
--- a/hw/mips/gt64xxx_pci.c
+++ b/hw/mips/gt64xxx_pci.c
@@ -26,6 +26,7 @@
 #include "qapi/error.h"
 #include "qemu/units.h"
 #include "qemu/log.h"
+#include "hw/registerfields.h"
 #include "hw/pci/pci.h"
 #include "hw/pci/pci_host.h"
 #include "hw/misc/empty_slot.h"
@@ -41,6 +42,9 @@
 #define GT_CPU                  (0x000 >> 2)
 #define GT_MULTI                (0x120 >> 2)
 
+REG32(GT_CPU,                   0x000)
+FIELD(GT_CPU,                   Endianess,      12, 1)
+
 /* CPU Address Decode */
 #define GT_SCS10LD              (0x008 >> 2)
 #define GT_SCS10HD              (0x010 >> 2)
@@ -210,6 +214,13 @@
 #define GT_PCI0_CFGADDR         (0xcf8 >> 2)
 #define GT_PCI0_CFGDATA         (0xcfc >> 2)
 
+REG32(GT_PCI0_CMD,              0xc00)
+FIELD(GT_PCI0_CMD,              MByteSwap,      0,  1)
+FIELD(GT_PCI0_CMD,              SByteSwap,      16, 1)
+REG32(GT_PCI1_CMD,              0xc80)
+FIELD(GT_PCI1_CMD,              MByteSwap,      0,  1)
+FIELD(GT_PCI1_CMD,              SByteSwap,      16, 1)
+
 /* Interrupts */
 #define GT_INTRCAUSE            (0xc18 >> 2)
 #define GT_INTRMASK             (0xc1c >> 2)
@@ -983,15 +994,17 @@ static const MemoryRegionOps isd_mem_ops = {
 static void gt64120_reset(DeviceState *dev)
 {
     GT64120State *s = GT64120_PCI_HOST_BRIDGE(dev);
+#if TARGET_BIG_ENDIAN
+    unsigned cpu_le = 0;
+#else
+    unsigned cpu_le = 1;
+#endif
 
     /* FIXME: Malta specific hw assumptions ahead */
 
     /* CPU Configuration */
-#if TARGET_BIG_ENDIAN
     s->regs[GT_CPU]           = 0x00000000;
-#else
-    s->regs[GT_CPU]           = 0x00001000;
-#endif
+    s->regs[GT_CPU] = FIELD_DP32(s->regs[GT_CPU], GT_CPU, Endianess, cpu_le);
     s->regs[GT_MULTI]         = 0x00000003;
 
     /* CPU Address decode */
@@ -1098,11 +1111,11 @@ static void gt64120_reset(DeviceState *dev)
     s->regs[GT_TC_CONTROL]    = 0x00000000;
 
     /* PCI Internal */
-#if TARGET_BIG_ENDIAN
     s->regs[GT_PCI0_CMD]      = 0x00000000;
-#else
-    s->regs[GT_PCI0_CMD]      = 0x00010001;
-#endif
+    s->regs[GT_PCI0_CMD] = FIELD_DP32(s->regs[GT_PCI0_CMD],
+                                              GT_PCI0_CMD, MByteSwap, cpu_le);
+    s->regs[GT_PCI0_CMD] = FIELD_DP32(s->regs[GT_PCI0_CMD],
+                                              GT_PCI0_CMD, SByteSwap, cpu_le);
     s->regs[GT_PCI0_TOR]      = 0x0000070f;
     s->regs[GT_PCI0_BS_SCS10] = 0x00fff000;
     s->regs[GT_PCI0_BS_SCS32] = 0x00fff000;
@@ -1119,11 +1132,11 @@ static void gt64120_reset(DeviceState *dev)
     s->regs[GT_PCI0_SSCS10_BAR] = 0x00000000;
     s->regs[GT_PCI0_SSCS32_BAR] = 0x01000000;
     s->regs[GT_PCI0_SCS3BT_BAR] = 0x1f000000;
-#if TARGET_BIG_ENDIAN
     s->regs[GT_PCI1_CMD]      = 0x00000000;
-#else
-    s->regs[GT_PCI1_CMD]      = 0x00010001;
-#endif
+    s->regs[GT_PCI1_CMD] = FIELD_DP32(s->regs[GT_PCI1_CMD],
+                                              GT_PCI1_CMD, MByteSwap, cpu_le);
+    s->regs[GT_PCI1_CMD] = FIELD_DP32(s->regs[GT_PCI1_CMD],
+                                              GT_PCI1_CMD, SByteSwap, cpu_le);
     s->regs[GT_PCI1_TOR]      = 0x0000070f;
     s->regs[GT_PCI1_BS_SCS10] = 0x00fff000;
     s->regs[GT_PCI1_BS_SCS32] = 0x00fff000;
-- 
2.38.1

Re: [PATCH-for-8.0 3/7] hw/mips/gt64xxx_pci: Manage endian bits with the RegisterField API

[Richard Henderson]

On 12/9/22 07:15, Philippe Mathieu-Daudé wrote:
>       /* CPU Configuration */
> -#if TARGET_BIG_ENDIAN
>       s->regs[GT_CPU]           = 0x00000000;
> -#else
> -    s->regs[GT_CPU]           = 0x00001000;
> -#endif
> +    s->regs[GT_CPU] = FIELD_DP32(s->regs[GT_CPU], GT_CPU, Endianess, cpu_le);

Missing set to zero before deposit.
Though I wonder if

     = cpu_le ? R_GT_CPU_Endianness_MASK : 0

might be easier?

> -#if TARGET_BIG_ENDIAN
>       s->regs[GT_PCI0_CMD]      = 0x00000000;
> -#else
> -    s->regs[GT_PCI0_CMD]      = 0x00010001;
> -#endif
> +    s->regs[GT_PCI0_CMD] = FIELD_DP32(s->regs[GT_PCI0_CMD],
> +                                              GT_PCI0_CMD, MByteSwap, cpu_le);
> +    s->regs[GT_PCI0_CMD] = FIELD_DP32(s->regs[GT_PCI0_CMD],
> +                                              GT_PCI0_CMD, SByteSwap, cpu_le);

This one at least has the zero, but it might as well use the masks like above.

> -#if TARGET_BIG_ENDIAN
>       s->regs[GT_PCI1_CMD]      = 0x00000000;
> -#else
> -    s->regs[GT_PCI1_CMD]      = 0x00010001;
> -#endif
> +    s->regs[GT_PCI1_CMD] = FIELD_DP32(s->regs[GT_PCI1_CMD],
> +                                              GT_PCI1_CMD, MByteSwap, cpu_le);
> +    s->regs[GT_PCI1_CMD] = FIELD_DP32(s->regs[GT_PCI1_CMD],
> +                                              GT_PCI1_CMD, SByteSwap, cpu_le);

Likewise.

r~

[PATCH-for-8.0 4/7] hw/mips/gt64xxx_pci: Add a 'cpu-little-endian' qdev property

[Philippe Mathieu-Daudé]

From: Philippe Mathieu-Daudé <f4bug@amsat.org>

This device does not have to be TARGET-dependent.
Add a 'cpu_big_endian' property which sets the byte-swapping
options if required.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/mips/gt64xxx_pci.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/hw/mips/gt64xxx_pci.c b/hw/mips/gt64xxx_pci.c
index 8c9ec80f7c..9ae4953d1e 100644
--- a/hw/mips/gt64xxx_pci.c
+++ b/hw/mips/gt64xxx_pci.c
@@ -26,6 +26,7 @@
 #include "qapi/error.h"
 #include "qemu/units.h"
 #include "qemu/log.h"
+#include "hw/qdev-properties.h"
 #include "hw/registerfields.h"
 #include "hw/pci/pci.h"
 #include "hw/pci/pci_host.h"
@@ -242,6 +243,8 @@ FIELD(GT_PCI1_CMD,              SByteSwap,      16, 1)
 
 OBJECT_DECLARE_SIMPLE_TYPE(GT64120State, GT64120_PCI_HOST_BRIDGE)
 
+#define FEAT_CPU_LE 0
+
 struct GT64120State {
     PCIHostState parent_obj;
 
@@ -252,6 +255,9 @@ struct GT64120State {
     PCI_MAPPING_ENTRY(ISD);
     MemoryRegion pci0_mem;
     AddressSpace pci0_mem_as;
+
+    /* properties */
+    uint32_t features;
 };
 
 /* Adjust range to avoid touching space which isn't mappable via PCI */
@@ -994,11 +1000,7 @@ static const MemoryRegionOps isd_mem_ops = {
 static void gt64120_reset(DeviceState *dev)
 {
     GT64120State *s = GT64120_PCI_HOST_BRIDGE(dev);
-#if TARGET_BIG_ENDIAN
-    unsigned cpu_le = 0;
-#else
-    unsigned cpu_le = 1;
-#endif
+    unsigned cpu_le = extract32(s->features, FEAT_CPU_LE, 1);
 
     /* FIXME: Malta specific hw assumptions ahead */
 
@@ -1229,11 +1231,18 @@ static const TypeInfo gt64120_pci_info = {
     },
 };
 
+static Property gt64120_properties[] = {
+    DEFINE_PROP_BIT("cpu-little-endian", GT64120State,
+                    features, FEAT_CPU_LE, !TARGET_BIG_ENDIAN),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void gt64120_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
+    device_class_set_props(dc, gt64120_properties);
     dc->realize = gt64120_realize;
     dc->reset = gt64120_reset;
     dc->vmsd = &vmstate_gt64120;
-- 
2.38.1

Re: [PATCH-for-8.0 4/7] hw/mips/gt64xxx_pci: Add a 'cpu-little-endian' qdev property

[Richard Henderson]

On 12/9/22 07:15, Philippe Mathieu-Daudé wrote:
> +static Property gt64120_properties[] = {
> +    DEFINE_PROP_BIT("cpu-little-endian", GT64120State,
> +                    features, FEAT_CPU_LE, !TARGET_BIG_ENDIAN),

Unless you're really planning on more feature bits, DEFINE_PROP_BOOL would be better.


r~

Re: [PATCH-for-8.0 4/7] hw/mips/gt64xxx_pci: Add a 'cpu-little-endian' qdev property

[Philippe Mathieu-Daudé]

On 20/12/22 01:52, Richard Henderson wrote:
> On 12/9/22 07:15, Philippe Mathieu-Daudé wrote:
>> +static Property gt64120_properties[] = {
>> +    DEFINE_PROP_BIT("cpu-little-endian", GT64120State,
>> +                    features, FEAT_CPU_LE, !TARGET_BIG_ENDIAN),
> 
> Unless you're really planning on more feature bits, DEFINE_PROP_BOOL 
> would be better.

I can easily convert to DEFINE_PROP_BIT() if I ever get there...

[PATCH-for-8.0 5/7] hw/mips/malta: Explicit GT64120 endianness upon device creation

[Philippe Mathieu-Daudé]

Propagate the controller endianess from the machine, setting
the "cpu-little-endian" property.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/mips/malta.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/mips/malta.c b/hw/mips/malta.c
index ba92022f87..1f4e0c7acc 100644
--- a/hw/mips/malta.c
+++ b/hw/mips/malta.c
@@ -1390,7 +1390,9 @@ void mips_malta_init(MachineState *machine)
     stl_p(memory_region_get_ram_ptr(bios_copy) + 0x10, 0x00000420);
 
     /* Northbridge */
-    dev = sysbus_create_simple("gt64120", -1, NULL);
+    dev = qdev_new("gt64120");
+    qdev_prop_set_bit(dev, "cpu-little-endian", !be);
+    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
     pci_bus = PCI_BUS(qdev_get_child_bus(dev, "pci"));
 
     /* Southbridge */
-- 
2.38.1

Re: [PATCH-for-8.0 5/7] hw/mips/malta: Explicit GT64120 endianness upon device creation

[Richard Henderson]

On 12/9/22 07:15, Philippe Mathieu-Daudé wrote:
> Propagate the controller endianess from the machine, setting
> the "cpu-little-endian" property.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/mips/malta.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)

Modulo using qdev_prop_set_bool,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

> 
> diff --git a/hw/mips/malta.c b/hw/mips/malta.c
> index ba92022f87..1f4e0c7acc 100644
> --- a/hw/mips/malta.c
> +++ b/hw/mips/malta.c
> @@ -1390,7 +1390,9 @@ void mips_malta_init(MachineState *machine)
>       stl_p(memory_region_get_ram_ptr(bios_copy) + 0x10, 0x00000420);
>   
>       /* Northbridge */
> -    dev = sysbus_create_simple("gt64120", -1, NULL);
> +    dev = qdev_new("gt64120");
> +    qdev_prop_set_bit(dev, "cpu-little-endian", !be);
> +    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>       pci_bus = PCI_BUS(qdev_get_child_bus(dev, "pci"));
>   
>       /* Southbridge */

Re: [PATCH-for-8.0 5/7] hw/mips/malta: Explicit GT64120 endianness upon device creation

[Philippe Mathieu-Daudé]

+Eduardo/Markus for QOM/QDEV clarification.

On 20/12/22 01:52, Richard Henderson wrote:
> On 12/9/22 07:15, Philippe Mathieu-Daudé wrote:
>> Propagate the controller endianess from the machine, setting
>> the "cpu-little-endian" property.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   hw/mips/malta.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> Modulo using qdev_prop_set_bool,

Surprisingly there is no qdev_prop_set_bool()... I can use the QOM layer
with object_property_add_bool(), i.e.:

$ git grep memory-hotplug-support
hw/acpi/ich9.c:451:    object_property_add_bool(obj, 
"memory-hotplug-support",
hw/acpi/piix4.c:608:    DEFINE_PROP_BOOL("memory-hotplug-support", 
PIIX4PMState,

But I notice some qdev_prop_set_bit() uses, i.e. in hw/arm/:

$ git grep enable-bitband
hw/arm/armv7m.c:528:    DEFINE_PROP_BOOL("enable-bitband", ARMv7MState, 
enable_bitband, false),
hw/arm/mps2.c:242:    qdev_prop_set_bit(armv7m, "enable-bitband", true);
hw/arm/msf2-soc.c:138:    qdev_prop_set_bit(armv7m, "enable-bitband", true);
hw/arm/stellaris.c:1070:    qdev_prop_set_bit(nvic, "enable-bitband", true);
hw/arm/stm32f100_soc.c:119:    qdev_prop_set_bit(armv7m, 
"enable-bitband", true);

In that case this patch doesn't require any change.

> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

Thanks!

>>
>> diff --git a/hw/mips/malta.c b/hw/mips/malta.c
>> index ba92022f87..1f4e0c7acc 100644
>> --- a/hw/mips/malta.c
>> +++ b/hw/mips/malta.c
>> @@ -1390,7 +1390,9 @@ void mips_malta_init(MachineState *machine)
>>       stl_p(memory_region_get_ram_ptr(bios_copy) + 0x10, 0x00000420);
>>       /* Northbridge */
>> -    dev = sysbus_create_simple("gt64120", -1, NULL);
>> +    dev = qdev_new("gt64120");
>> +    qdev_prop_set_bit(dev, "cpu-little-endian", !be);
>> +    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>>       pci_bus = PCI_BUS(qdev_get_child_bus(dev, "pci"));
>>       /* Southbridge */
> 

Re: [PATCH-for-8.0 5/7] hw/mips/malta: Explicit GT64120 endianness upon device creation

[Philippe Mathieu-Daudé]

On 20/12/22 09:30, Philippe Mathieu-Daudé wrote:
> +Eduardo/Markus for QOM/QDEV clarification.
> 
> On 20/12/22 01:52, Richard Henderson wrote:
>> On 12/9/22 07:15, Philippe Mathieu-Daudé wrote:
>>> Propagate the controller endianess from the machine, setting
>>> the "cpu-little-endian" property.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>>   hw/mips/malta.c | 4 +++-
>>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> Modulo using qdev_prop_set_bool,
> 
> Surprisingly there is no qdev_prop_set_bool()... I can use the QOM layer
> with object_property_add_bool(), i.e.:
> 
> $ git grep memory-hotplug-support
> hw/acpi/ich9.c:451:    object_property_add_bool(obj, 
> "memory-hotplug-support",
> hw/acpi/piix4.c:608:    DEFINE_PROP_BOOL("memory-hotplug-support", 
> PIIX4PMState,

Oops I meant:

$ git grep reset-hivecs
hw/arm/digic.c:55:    if (!object_property_set_bool(OBJECT(&s->cpu), 
"reset-hivecs", true,
hw/arm/npcm7xx.c:469: 
object_property_set_bool(OBJECT(&s->cpu[i]), "reset-hivecs", true,
hw/arm/xlnx-zynqmp.c:246: 
object_property_set_bool(OBJECT(&s->rpu_cpu[i]), "reset-hivecs", true,
target/arm/cpu.c:1245:            DEFINE_PROP_BOOL("reset-hivecs", 
ARMCPU, reset_hivecs, false);

> But I notice some qdev_prop_set_bit() uses, i.e. in hw/arm/:
> 
> $ git grep enable-bitband
> hw/arm/armv7m.c:528:    DEFINE_PROP_BOOL("enable-bitband", ARMv7MState, 
> enable_bitband, false),
> hw/arm/mps2.c:242:    qdev_prop_set_bit(armv7m, "enable-bitband", true);
> hw/arm/msf2-soc.c:138:    qdev_prop_set_bit(armv7m, "enable-bitband", 
> true);
> hw/arm/stellaris.c:1070:    qdev_prop_set_bit(nvic, "enable-bitband", 
> true);
> hw/arm/stm32f100_soc.c:119:    qdev_prop_set_bit(armv7m, 
> "enable-bitband", true);
> 
> In that case this patch doesn't require any change.

I'll keep qdev_prop_set_bit() which is simpler.

>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> 
> Thanks!
> 
>>>
>>> diff --git a/hw/mips/malta.c b/hw/mips/malta.c
>>> index ba92022f87..1f4e0c7acc 100644
>>> --- a/hw/mips/malta.c
>>> +++ b/hw/mips/malta.c
>>> @@ -1390,7 +1390,9 @@ void mips_malta_init(MachineState *machine)
>>>       stl_p(memory_region_get_ram_ptr(bios_copy) + 0x10, 0x00000420);
>>>       /* Northbridge */
>>> -    dev = sysbus_create_simple("gt64120", -1, NULL);
>>> +    dev = qdev_new("gt64120");
>>> +    qdev_prop_set_bit(dev, "cpu-little-endian", !be);
>>> +    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>>>       pci_bus = PCI_BUS(qdev_get_child_bus(dev, "pci"));
>>>       /* Southbridge */
>>
> 

[PATCH-for-8.0 6/7] hw/mips/meson: Make gt64xxx_pci.c endian-agnostic

[Philippe Mathieu-Daudé]

From: Philippe Mathieu-Daudé <philmd@redhat.com>

The single machine using this device explicitly sets its
endianness. We don't need to set a default. This allow us
to remove the target specificity from the build system.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/mips/gt64xxx_pci.c | 2 +-
 hw/mips/meson.build   | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/mips/gt64xxx_pci.c b/hw/mips/gt64xxx_pci.c
index 9ae4953d1e..b05b2b3acd 100644
--- a/hw/mips/gt64xxx_pci.c
+++ b/hw/mips/gt64xxx_pci.c
@@ -1233,7 +1233,7 @@ static const TypeInfo gt64120_pci_info = {
 
 static Property gt64120_properties[] = {
     DEFINE_PROP_BIT("cpu-little-endian", GT64120State,
-                    features, FEAT_CPU_LE, !TARGET_BIG_ENDIAN),
+                    features, FEAT_CPU_LE, 0),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/mips/meson.build b/hw/mips/meson.build
index 6ccd385df0..152103f15f 100644
--- a/hw/mips/meson.build
+++ b/hw/mips/meson.build
@@ -3,7 +3,7 @@ mips_ss.add(files('bootloader.c', 'mips_int.c'))
 mips_ss.add(when: 'CONFIG_FW_CFG_MIPS', if_true: files('fw_cfg.c'))
 mips_ss.add(when: 'CONFIG_LOONGSON3V', if_true: files('loongson3_bootp.c', 'loongson3_virt.c'))
 mips_ss.add(when: 'CONFIG_MALTA', if_true: files('malta.c'))
-mips_ss.add(when: 'CONFIG_GT64120', if_true: files('gt64xxx_pci.c'))
+softmmu_ss.add(when: 'CONFIG_GT64120', if_true: files('gt64xxx_pci.c'))
 mips_ss.add(when: 'CONFIG_MIPS_CPS', if_true: files('cps.c'))
 
 if 'CONFIG_TCG' in config_all
-- 
2.38.1

Re: [PATCH-for-8.0 6/7] hw/mips/meson: Make gt64xxx_pci.c endian-agnostic

[Richard Henderson]

On 12/9/22 07:15, Philippe Mathieu-Daudé wrote:
> From: Philippe Mathieu-Daudé<philmd@redhat.com>
> 
> The single machine using this device explicitly sets its
> endianness. We don't need to set a default. This allow us
> to remove the target specificity from the build system.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@redhat.com>
> ---
>   hw/mips/gt64xxx_pci.c | 2 +-
>   hw/mips/meson.build   | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH-for-8.0 7/7] hw/mips/gt64xxx_pci: Move it to hw/pci-host/

[Philippe Mathieu-Daudé]

From: Philippe Mathieu-Daudé <f4bug@amsat.org>

The GT-64120 is a north-bridge, and it is not MIPS specific.
Move it with the other north-bridge devices.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 MAINTAINERS                                   | 2 +-
 hw/mips/Kconfig                               | 6 ------
 hw/mips/meson.build                           | 1 -
 hw/mips/trace-events                          | 6 ------
 hw/pci-host/Kconfig                           | 6 ++++++
 hw/{mips/gt64xxx_pci.c => pci-host/gt64120.c} | 0
 hw/pci-host/meson.build                       | 1 +
 hw/pci-host/trace-events                      | 7 +++++++
 meson.build                                   | 1 -
 9 files changed, 15 insertions(+), 15 deletions(-)
 delete mode 100644 hw/mips/trace-events
 rename hw/{mips/gt64xxx_pci.c => pci-host/gt64120.c} (100%)

diff --git a/MAINTAINERS b/MAINTAINERS
index 6966490c94..e558b53e85 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1226,7 +1226,7 @@ S: Odd Fixes
 F: hw/isa/piix4.c
 F: hw/acpi/piix4.c
 F: hw/mips/malta.c
-F: hw/mips/gt64xxx_pci.c
+F: hw/pci-host/gt64120.c
 F: include/hw/southbridge/piix.h
 F: tests/avocado/linux_ssh_mips_malta.py
 F: tests/avocado/machine_mips_malta.py
diff --git a/hw/mips/Kconfig b/hw/mips/Kconfig
index 8f7bce38fb..7a55143f8a 100644
--- a/hw/mips/Kconfig
+++ b/hw/mips/Kconfig
@@ -60,9 +60,3 @@ config MIPS_BOSTON
 
 config FW_CFG_MIPS
     bool
-
-config GT64120
-    bool
-    select PCI
-    select EMPTY_SLOT
-    select I8259
diff --git a/hw/mips/meson.build b/hw/mips/meson.build
index 152103f15f..900613fc08 100644
--- a/hw/mips/meson.build
+++ b/hw/mips/meson.build
@@ -3,7 +3,6 @@ mips_ss.add(files('bootloader.c', 'mips_int.c'))
 mips_ss.add(when: 'CONFIG_FW_CFG_MIPS', if_true: files('fw_cfg.c'))
 mips_ss.add(when: 'CONFIG_LOONGSON3V', if_true: files('loongson3_bootp.c', 'loongson3_virt.c'))
 mips_ss.add(when: 'CONFIG_MALTA', if_true: files('malta.c'))
-softmmu_ss.add(when: 'CONFIG_GT64120', if_true: files('gt64xxx_pci.c'))
 mips_ss.add(when: 'CONFIG_MIPS_CPS', if_true: files('cps.c'))
 
 if 'CONFIG_TCG' in config_all
diff --git a/hw/mips/trace-events b/hw/mips/trace-events
deleted file mode 100644
index 13ee731a48..0000000000
--- a/hw/mips/trace-events
+++ /dev/null
@@ -1,6 +0,0 @@
-# gt64xxx_pci.c
-gt64120_read(uint64_t addr, uint64_t value) "gt64120 read 0x%03"PRIx64" value:0x%08" PRIx64
-gt64120_write(uint64_t addr, uint64_t value) "gt64120 write 0x%03"PRIx64" value:0x%08" PRIx64
-gt64120_read_intreg(const char *regname, unsigned size, uint64_t value) "gt64120 read %s size:%u value:0x%08" PRIx64
-gt64120_write_intreg(const char *regname, unsigned size, uint64_t value) "gt64120 write %s size:%u value:0x%08" PRIx64
-gt64120_isd_remap(uint64_t from_length, uint64_t from_addr, uint64_t to_length, uint64_t to_addr) "ISD: 0x%08" PRIx64 "@0x%08" PRIx64 " -> 0x%08" PRIx64 "@0x%08" PRIx64
diff --git a/hw/pci-host/Kconfig b/hw/pci-host/Kconfig
index 38fd2ee8f3..a07070eddf 100644
--- a/hw/pci-host/Kconfig
+++ b/hw/pci-host/Kconfig
@@ -81,3 +81,9 @@ config MV64361
 config DINO
     bool
     select PCI
+
+config GT64120
+    bool
+    select PCI
+    select EMPTY_SLOT
+    select I8259
diff --git a/hw/mips/gt64xxx_pci.c b/hw/pci-host/gt64120.c
similarity index 100%
rename from hw/mips/gt64xxx_pci.c
rename to hw/pci-host/gt64120.c
diff --git a/hw/pci-host/meson.build b/hw/pci-host/meson.build
index e832babc9d..9a813d552e 100644
--- a/hw/pci-host/meson.build
+++ b/hw/pci-host/meson.build
@@ -1,6 +1,7 @@
 pci_ss = ss.source_set()
 pci_ss.add(when: 'CONFIG_PAM', if_true: files('pam.c'))
 pci_ss.add(when: 'CONFIG_PCI_BONITO', if_true: files('bonito.c'))
+pci_ss.add(when: 'CONFIG_GT64120', if_true: files('gt64120.c'))
 pci_ss.add(when: 'CONFIG_PCI_EXPRESS_DESIGNWARE', if_true: files('designware.c'))
 pci_ss.add(when: 'CONFIG_PCI_EXPRESS_GENERIC_BRIDGE', if_true: files('gpex.c'))
 pci_ss.add(when: ['CONFIG_PCI_EXPRESS_GENERIC_BRIDGE', 'CONFIG_ACPI'], if_true: files('gpex-acpi.c'))
diff --git a/hw/pci-host/trace-events b/hw/pci-host/trace-events
index 437e66ff50..9d216bb89f 100644
--- a/hw/pci-host/trace-events
+++ b/hw/pci-host/trace-events
@@ -6,6 +6,13 @@ bonito_spciconf_small_access(uint64_t addr, unsigned size) "PCI config address i
 # grackle.c
 grackle_set_irq(int irq_num, int level) "set_irq num %d level %d"
 
+# gt64120.c
+gt64120_read(uint64_t addr, uint64_t value) "gt64120 read 0x%03"PRIx64" value:0x%08" PRIx64
+gt64120_write(uint64_t addr, uint64_t value) "gt64120 write 0x%03"PRIx64" value:0x%08" PRIx64
+gt64120_read_intreg(const char *regname, unsigned size, uint64_t value) "gt64120 read %s size:%u value:0x%08" PRIx64
+gt64120_write_intreg(const char *regname, unsigned size, uint64_t value) "gt64120 write %s size:%u value:0x%08" PRIx64
+gt64120_isd_remap(uint64_t from_length, uint64_t from_addr, uint64_t to_length, uint64_t to_addr) "ISD: 0x%08" PRIx64 "@0x%08" PRIx64 " -> 0x%08" PRIx64 "@0x%08" PRIx64
+
 # mv64361.c
 mv64361_region_map(const char *name, uint64_t poffs, uint64_t size, uint64_t moffs) "Mapping %s 0x%"PRIx64"+0x%"PRIx64" @ 0x%"PRIx64
 mv64361_region_enable(const char *op, int num) "Should %s region %d"
diff --git a/meson.build b/meson.build
index 5c6b5a1c75..bd5774f32f 100644
--- a/meson.build
+++ b/meson.build
@@ -2944,7 +2944,6 @@ if have_system
     'hw/intc',
     'hw/isa',
     'hw/mem',
-    'hw/mips',
     'hw/misc',
     'hw/misc/macio',
     'hw/net',
-- 
2.38.1

Re: [PATCH-for-8.0 7/7] hw/mips/gt64xxx_pci: Move it to hw/pci-host/

[Bernhard Beschow]

Am 9. Dezember 2022 15:15:33 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>From: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
>The GT-64120 is a north-bridge, and it is not MIPS specific.
>Move it with the other north-bridge devices.
>
>Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>---
> MAINTAINERS                                   | 2 +-
> hw/mips/Kconfig                               | 6 ------
> hw/mips/meson.build                           | 1 -
> hw/mips/trace-events                          | 6 ------
> hw/pci-host/Kconfig                           | 6 ++++++
> hw/{mips/gt64xxx_pci.c => pci-host/gt64120.c} | 0
> hw/pci-host/meson.build                       | 1 +
> hw/pci-host/trace-events                      | 7 +++++++
> meson.build                                   | 1 -
> 9 files changed, 15 insertions(+), 15 deletions(-)
> delete mode 100644 hw/mips/trace-events
> rename hw/{mips/gt64xxx_pci.c => pci-host/gt64120.c} (100%)
>
>diff --git a/MAINTAINERS b/MAINTAINERS
>index 6966490c94..e558b53e85 100644
>--- a/MAINTAINERS
>+++ b/MAINTAINERS
>@@ -1226,7 +1226,7 @@ S: Odd Fixes
> F: hw/isa/piix4.c
> F: hw/acpi/piix4.c
> F: hw/mips/malta.c
>-F: hw/mips/gt64xxx_pci.c
>+F: hw/pci-host/gt64120.c
> F: include/hw/southbridge/piix.h
> F: tests/avocado/linux_ssh_mips_malta.py
> F: tests/avocado/machine_mips_malta.py
>diff --git a/hw/mips/Kconfig b/hw/mips/Kconfig
>index 8f7bce38fb..7a55143f8a 100644
>--- a/hw/mips/Kconfig
>+++ b/hw/mips/Kconfig
>@@ -60,9 +60,3 @@ config MIPS_BOSTON
> 
> config FW_CFG_MIPS
>     bool
>-
>-config GT64120
>-    bool
>-    select PCI
>-    select EMPTY_SLOT
>-    select I8259
>diff --git a/hw/mips/meson.build b/hw/mips/meson.build
>index 152103f15f..900613fc08 100644
>--- a/hw/mips/meson.build
>+++ b/hw/mips/meson.build
>@@ -3,7 +3,6 @@ mips_ss.add(files('bootloader.c', 'mips_int.c'))
> mips_ss.add(when: 'CONFIG_FW_CFG_MIPS', if_true: files('fw_cfg.c'))
> mips_ss.add(when: 'CONFIG_LOONGSON3V', if_true: files('loongson3_bootp.c', 'loongson3_virt.c'))
> mips_ss.add(when: 'CONFIG_MALTA', if_true: files('malta.c'))
>-softmmu_ss.add(when: 'CONFIG_GT64120', if_true: files('gt64xxx_pci.c'))
> mips_ss.add(when: 'CONFIG_MIPS_CPS', if_true: files('cps.c'))
> 
> if 'CONFIG_TCG' in config_all
>diff --git a/hw/mips/trace-events b/hw/mips/trace-events
>deleted file mode 100644
>index 13ee731a48..0000000000
>--- a/hw/mips/trace-events
>+++ /dev/null
>@@ -1,6 +0,0 @@
>-# gt64xxx_pci.c
>-gt64120_read(uint64_t addr, uint64_t value) "gt64120 read 0x%03"PRIx64" value:0x%08" PRIx64
>-gt64120_write(uint64_t addr, uint64_t value) "gt64120 write 0x%03"PRIx64" value:0x%08" PRIx64
>-gt64120_read_intreg(const char *regname, unsigned size, uint64_t value) "gt64120 read %s size:%u value:0x%08" PRIx64
>-gt64120_write_intreg(const char *regname, unsigned size, uint64_t value) "gt64120 write %s size:%u value:0x%08" PRIx64
>-gt64120_isd_remap(uint64_t from_length, uint64_t from_addr, uint64_t to_length, uint64_t to_addr) "ISD: 0x%08" PRIx64 "@0x%08" PRIx64 " -> 0x%08" PRIx64 "@0x%08" PRIx64
>diff --git a/hw/pci-host/Kconfig b/hw/pci-host/Kconfig
>index 38fd2ee8f3..a07070eddf 100644
>--- a/hw/pci-host/Kconfig
>+++ b/hw/pci-host/Kconfig
>@@ -81,3 +81,9 @@ config MV64361
> config DINO
>     bool
>     select PCI
>+
>+config GT64120
>+    bool
>+    select PCI
>+    select EMPTY_SLOT
>+    select I8259

While at it: s/select I8259/depends on I8259/ since the model needs but doesn't provide I8259?

Best regards,
Bernhard

>diff --git a/hw/mips/gt64xxx_pci.c b/hw/pci-host/gt64120.c
>similarity index 100%
>rename from hw/mips/gt64xxx_pci.c
>rename to hw/pci-host/gt64120.c
>diff --git a/hw/pci-host/meson.build b/hw/pci-host/meson.build
>index e832babc9d..9a813d552e 100644
>--- a/hw/pci-host/meson.build
>+++ b/hw/pci-host/meson.build
>@@ -1,6 +1,7 @@
> pci_ss = ss.source_set()
> pci_ss.add(when: 'CONFIG_PAM', if_true: files('pam.c'))
> pci_ss.add(when: 'CONFIG_PCI_BONITO', if_true: files('bonito.c'))
>+pci_ss.add(when: 'CONFIG_GT64120', if_true: files('gt64120.c'))
> pci_ss.add(when: 'CONFIG_PCI_EXPRESS_DESIGNWARE', if_true: files('designware.c'))
> pci_ss.add(when: 'CONFIG_PCI_EXPRESS_GENERIC_BRIDGE', if_true: files('gpex.c'))
> pci_ss.add(when: ['CONFIG_PCI_EXPRESS_GENERIC_BRIDGE', 'CONFIG_ACPI'], if_true: files('gpex-acpi.c'))
>diff --git a/hw/pci-host/trace-events b/hw/pci-host/trace-events
>index 437e66ff50..9d216bb89f 100644
>--- a/hw/pci-host/trace-events
>+++ b/hw/pci-host/trace-events
>@@ -6,6 +6,13 @@ bonito_spciconf_small_access(uint64_t addr, unsigned size) "PCI config address i
> # grackle.c
> grackle_set_irq(int irq_num, int level) "set_irq num %d level %d"
> 
>+# gt64120.c
>+gt64120_read(uint64_t addr, uint64_t value) "gt64120 read 0x%03"PRIx64" value:0x%08" PRIx64
>+gt64120_write(uint64_t addr, uint64_t value) "gt64120 write 0x%03"PRIx64" value:0x%08" PRIx64
>+gt64120_read_intreg(const char *regname, unsigned size, uint64_t value) "gt64120 read %s size:%u value:0x%08" PRIx64
>+gt64120_write_intreg(const char *regname, unsigned size, uint64_t value) "gt64120 write %s size:%u value:0x%08" PRIx64
>+gt64120_isd_remap(uint64_t from_length, uint64_t from_addr, uint64_t to_length, uint64_t to_addr) "ISD: 0x%08" PRIx64 "@0x%08" PRIx64 " -> 0x%08" PRIx64 "@0x%08" PRIx64
>+
> # mv64361.c
> mv64361_region_map(const char *name, uint64_t poffs, uint64_t size, uint64_t moffs) "Mapping %s 0x%"PRIx64"+0x%"PRIx64" @ 0x%"PRIx64
> mv64361_region_enable(const char *op, int num) "Should %s region %d"
>diff --git a/meson.build b/meson.build
>index 5c6b5a1c75..bd5774f32f 100644
>--- a/meson.build
>+++ b/meson.build
>@@ -2944,7 +2944,6 @@ if have_system
>     'hw/intc',
>     'hw/isa',
>     'hw/mem',
>-    'hw/mips',
>     'hw/misc',
>     'hw/misc/macio',
>     'hw/net',

Re: [PATCH-for-8.0 7/7] hw/mips/gt64xxx_pci: Move it to hw/pci-host/

[Richard Henderson]

On 12/9/22 07:15, Philippe Mathieu-Daudé wrote:
> From: Philippe Mathieu-Daudé<f4bug@amsat.org>
> 
> The GT-64120 is a north-bridge, and it is not MIPS specific.
> Move it with the other north-bridge devices.
> 
> Signed-off-by: Philippe Mathieu-Daudé<f4bug@amsat.org>
> ---
>   MAINTAINERS                                   | 2 +-
>   hw/mips/Kconfig                               | 6 ------
>   hw/mips/meson.build                           | 1 -
>   hw/mips/trace-events                          | 6 ------
>   hw/pci-host/Kconfig                           | 6 ++++++
>   hw/{mips/gt64xxx_pci.c => pci-host/gt64120.c} | 0
>   hw/pci-host/meson.build                       | 1 +
>   hw/pci-host/trace-events                      | 7 +++++++
>   meson.build                                   | 1 -
>   9 files changed, 15 insertions(+), 15 deletions(-)
>   delete mode 100644 hw/mips/trace-events
>   rename hw/{mips/gt64xxx_pci.c => pci-host/gt64120.c} (100%)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH-for-8.0 0/7] hw/mips: Make gt64xxx_pci.c endian-agnostic

[Bernhard Beschow]

Am 9. Dezember 2022 15:15:26 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>Respining an old/unfinished series... Add the 'cpu-little-endian'
>qdev property to the GT64120 north bridge so [target-specific]
>machines can set its endianness, allowing it to be endian agnostic.

Hi Phil,

Did you intend to use three different E-mail addresses in your series? At least your former @redhat.com address bounces...

Best regards,
Bernhard

>
>Philippe Mathieu-Daudé (7):
>  hw/mips/Kconfig: Introduce CONFIG_GT64120 to select gt64xxx_pci.c
>  hw/mips/gt64xxx_pci: Let the GT64120 manage the lower 512MiB hole
>  hw/mips/gt64xxx_pci: Manage endian bits with the RegisterField API
>  hw/mips/gt64xxx_pci: Add a 'cpu-little-endian' qdev property
>  hw/mips/malta: Explicit GT64120 endianness upon device creation
>  hw/mips/meson: Make gt64xxx_pci.c endian-agnostic
>  hw/mips/gt64xxx_pci: Move it to hw/pci-host/
>
> MAINTAINERS                                   |  2 +-
> configs/devices/mips-softmmu/common.mak       |  1 -
> hw/mips/Kconfig                               |  1 +
> hw/mips/malta.c                               | 11 ++--
> hw/mips/meson.build                           |  2 +-
> hw/mips/trace-events                          |  6 ---
> hw/pci-host/Kconfig                           |  6 +++
> hw/{mips/gt64xxx_pci.c => pci-host/gt64120.c} | 54 ++++++++++++++-----
> hw/pci-host/meson.build                       |  1 +
> hw/pci-host/trace-events                      |  7 +++
> meson.build                                   |  1 -
> 11 files changed, 62 insertions(+), 30 deletions(-)
> delete mode 100644 hw/mips/trace-events
> rename hw/{mips/gt64xxx_pci.c => pci-host/gt64120.c} (95%)
>

Re: [PATCH-for-8.0 0/7] hw/mips: Make gt64xxx_pci.c endian-agnostic

[Philippe Mathieu-Daudé]

On 12/12/22 01:55, Bernhard Beschow wrote:
> 
> 
> Am 9. Dezember 2022 15:15:26 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>> Respining an old/unfinished series... Add the 'cpu-little-endian'
>> qdev property to the GT64120 north bridge so [target-specific]
>> machines can set its endianness, allowing it to be endian agnostic.
> 
> Hi Phil,
> 
> Did you intend to use three different E-mail addresses in your series? At least your former @redhat.com address bounces...

I cherry-picked some old patches which came with my previous address, 
sorry for that confusion.