Re: [Qemu-devel] [Qemu-ppc] [PATCH 07/19] uninorth: move PCI mmio memory region initialisation into init function

2018-03-06 Thread Mark Cave-Ayland 

On 07/03/18 07:02, Mark Cave-Ayland wrote:


On 06/03/18 23:44, BALATON Zoltan wrote:


On Tue, 6 Mar 2018, Mark Cave-Ayland wrote:
Whilst we are here, rename the memory regions to better reflect 
whether they

belong to either a PCI or an AGP bus.

Signed-off-by: Mark Cave-Ayland 
---
hw/pci-host/uninorth.c | 28 ++--
1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/hw/pci-host/uninorth.c b/hw/pci-host/uninorth.c
index b081e3c153..5b8fc3aa16 100644
--- a/hw/pci-host/uninorth.c
+++ b/hw/pci-host/uninorth.c
@@ -111,29 +111,39 @@ static const MemoryRegionOps unin_data_ops = {

static void pci_unin_main_init(Object *obj)
{
+    UNINState *s = UNI_NORTH_PCI_HOST_BRIDGE(obj);
    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
    PCIHostState *h = PCI_HOST_BRIDGE(obj);

    /* Use values found on a real PowerMac */
    /* Uninorth main bus */
    memory_region_init_io(>conf_mem, OBJECT(h), 
_host_conf_le_ops,

-  obj, "pci-conf-idx", 0x1000);
+  obj, "unin-pci-conf-idx", 0x1000);
    memory_region_init_io(>data_mem, OBJECT(h), _data_ops, obj,
-  "pci-conf-data", 0x1000);
+  "unin-pci-conf-data", 0x1000);
+
+    memory_region_init(>pci_mmio, OBJECT(s), "unin-pci-mmio",
+   0x1ULL);
+
    sysbus_init_mmio(sbd, >conf_mem);
    sysbus_init_mmio(sbd, >data_mem);
}

static void pci_u3_agp_init(Object *obj)
{
+    UNINState *s = U3_AGP_HOST_BRIDGE(obj);
    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
    PCIHostState *h = PCI_HOST_BRIDGE(obj);

    /* Uninorth U3 AGP bus */
    memory_region_init_io(>conf_mem, OBJECT(h), 
_host_conf_le_ops,

-  obj, "pci-conf-idx", 0x1000);
+  obj, "unin-pci-conf-idx", 0x1000);
    memory_region_init_io(>data_mem, OBJECT(h), _data_ops, obj,
-  "pci-conf-data", 0x1000);
+  "unin-pci-conf-data", 0x1000);
+
+    memory_region_init(>pci_mmio, OBJECT(s), "unin-pci-mmio",


The name of this function and the above comment both suggest this is 
an AGP bus so did you mean to rename these to unin-agp-* instead of 
unin-pci-*?


Well this patchset purposely avoids doing anything with the U3 model 
other than the required refactoring to move the wiring to board level as 
really the entire U3 model needs some love - I can't even boot Linux 
without this patchset (I suspect it's probably DT related).


Having said that the wiring changes are such an improvement that I would 
argue for applying this patchset if possible since any future fixes will 
be considerably easier based upon it.


Looking in detail I think this naming is still correct: the U3 PCI bus 
address is currently 0xf000 which is actually the AGP bus rather 
than the PCI bus at 0xf200...



ATB,

Mark.

Re: [Qemu-devel] [Qemu-ppc] [PATCH 07/19] uninorth: move PCI mmio memory region initialisation into init function

2018-03-06 Thread Mark Cave-Ayland 

On 06/03/18 23:44, BALATON Zoltan wrote:


On Tue, 6 Mar 2018, Mark Cave-Ayland wrote:
Whilst we are here, rename the memory regions to better reflect 
whether they

belong to either a PCI or an AGP bus.

Signed-off-by: Mark Cave-Ayland 
---
hw/pci-host/uninorth.c | 28 ++--
1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/hw/pci-host/uninorth.c b/hw/pci-host/uninorth.c
index b081e3c153..5b8fc3aa16 100644
--- a/hw/pci-host/uninorth.c
+++ b/hw/pci-host/uninorth.c
@@ -111,29 +111,39 @@ static const MemoryRegionOps unin_data_ops = {

static void pci_unin_main_init(Object *obj)
{
+    UNINState *s = UNI_NORTH_PCI_HOST_BRIDGE(obj);
    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
    PCIHostState *h = PCI_HOST_BRIDGE(obj);

    /* Use values found on a real PowerMac */
    /* Uninorth main bus */
    memory_region_init_io(>conf_mem, OBJECT(h), _host_conf_le_ops,
-  obj, "pci-conf-idx", 0x1000);
+  obj, "unin-pci-conf-idx", 0x1000);
    memory_region_init_io(>data_mem, OBJECT(h), _data_ops, obj,
-  "pci-conf-data", 0x1000);
+  "unin-pci-conf-data", 0x1000);
+
+    memory_region_init(>pci_mmio, OBJECT(s), "unin-pci-mmio",
+   0x1ULL);
+
    sysbus_init_mmio(sbd, >conf_mem);
    sysbus_init_mmio(sbd, >data_mem);
}

static void pci_u3_agp_init(Object *obj)
{
+    UNINState *s = U3_AGP_HOST_BRIDGE(obj);
    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
    PCIHostState *h = PCI_HOST_BRIDGE(obj);

    /* Uninorth U3 AGP bus */
    memory_region_init_io(>conf_mem, OBJECT(h), _host_conf_le_ops,
-  obj, "pci-conf-idx", 0x1000);
+  obj, "unin-pci-conf-idx", 0x1000);
    memory_region_init_io(>data_mem, OBJECT(h), _data_ops, obj,
-  "pci-conf-data", 0x1000);
+  "unin-pci-conf-data", 0x1000);
+
+    memory_region_init(>pci_mmio, OBJECT(s), "unin-pci-mmio",


The name of this function and the above comment both suggest this is an 
AGP bus so did you mean to rename these to unin-agp-* instead of 
unin-pci-*?


Well this patchset purposely avoids doing anything with the U3 model 
other than the required refactoring to move the wiring to board level as 
really the entire U3 model needs some love - I can't even boot Linux 
without this patchset (I suspect it's probably DT related).


Having said that the wiring changes are such an improvement that I would 
argue for applying this patchset if possible since any future fixes will 
be considerably easier based upon it.



ATB,

Mark.

Re: [Qemu-devel] [Qemu-ppc] [PATCH 07/19] uninorth: move PCI mmio memory region initialisation into init function

2018-03-06 Thread BALATON Zoltan 

On Tue, 6 Mar 2018, Mark Cave-Ayland wrote:

Whilst we are here, rename the memory regions to better reflect whether they
belong to either a PCI or an AGP bus.

Signed-off-by: Mark Cave-Ayland 
---
hw/pci-host/uninorth.c | 28 ++--
1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/hw/pci-host/uninorth.c b/hw/pci-host/uninorth.c
index b081e3c153..5b8fc3aa16 100644
--- a/hw/pci-host/uninorth.c
+++ b/hw/pci-host/uninorth.c
@@ -111,29 +111,39 @@ static const MemoryRegionOps unin_data_ops = {

static void pci_unin_main_init(Object *obj)
{
+UNINState *s = UNI_NORTH_PCI_HOST_BRIDGE(obj);
SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
PCIHostState *h = PCI_HOST_BRIDGE(obj);

/* Use values found on a real PowerMac */
/* Uninorth main bus */
memory_region_init_io(>conf_mem, OBJECT(h), _host_conf_le_ops,
-  obj, "pci-conf-idx", 0x1000);
+  obj, "unin-pci-conf-idx", 0x1000);
memory_region_init_io(>data_mem, OBJECT(h), _data_ops, obj,
-  "pci-conf-data", 0x1000);
+  "unin-pci-conf-data", 0x1000);
+
+memory_region_init(>pci_mmio, OBJECT(s), "unin-pci-mmio",
+   0x1ULL);
+
sysbus_init_mmio(sbd, >conf_mem);
sysbus_init_mmio(sbd, >data_mem);
}

static void pci_u3_agp_init(Object *obj)
{
+UNINState *s = U3_AGP_HOST_BRIDGE(obj);
SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
PCIHostState *h = PCI_HOST_BRIDGE(obj);

/* Uninorth U3 AGP bus */
memory_region_init_io(>conf_mem, OBJECT(h), _host_conf_le_ops,
-  obj, "pci-conf-idx", 0x1000);
+  obj, "unin-pci-conf-idx", 0x1000);
memory_region_init_io(>data_mem, OBJECT(h), _data_ops, obj,
-  "pci-conf-data", 0x1000);
+  "unin-pci-conf-data", 0x1000);
+
+memory_region_init(>pci_mmio, OBJECT(s), "unin-pci-mmio",


The name of this function and the above comment both suggest this is an 
AGP bus so did you mean to rename these to unin-agp-* instead of 
unin-pci-*?


Regards,
BALATON Zoltan


+   0x1ULL);
+
sysbus_init_mmio(sbd, >conf_mem);
sysbus_init_mmio(sbd, >data_mem);
}
@@ -145,9 +155,9 @@ static void pci_unin_agp_init(Object *obj)

/* Uninorth AGP bus */
memory_region_init_io(>conf_mem, OBJECT(h), _host_conf_le_ops,
-  obj, "pci-conf-idx", 0x1000);
+  obj, "unin-agp-conf-idx", 0x1000);
memory_region_init_io(>data_mem, OBJECT(h), _host_data_le_ops,
-  obj, "pci-conf-data", 0x1000);
+  obj, "unin-agp-conf-data", 0x1000);
sysbus_init_mmio(sbd, >conf_mem);
sysbus_init_mmio(sbd, >data_mem);
}
@@ -159,9 +169,9 @@ static void pci_unin_internal_init(Object *obj)

/* Uninorth internal bus */
memory_region_init_io(>conf_mem, OBJECT(h), _host_conf_le_ops,
-  obj, "pci-conf-idx", 0x1000);
+  obj, "unin-pci-conf-idx", 0x1000);
memory_region_init_io(>data_mem, OBJECT(h), _host_data_le_ops,
-  obj, "pci-conf-data", 0x1000);
+  obj, "unin-pci-conf-data", 0x1000);
sysbus_init_mmio(sbd, >conf_mem);
sysbus_init_mmio(sbd, >data_mem);
}
@@ -182,7 +192,6 @@ UNINState *pci_pmac_init(qemu_irq *pic,
s = SYS_BUS_DEVICE(dev);
h = PCI_HOST_BRIDGE(s);
d = UNI_NORTH_PCI_HOST_BRIDGE(dev);
-memory_region_init(>pci_mmio, OBJECT(d), "pci-mmio", 0x1ULL);
memory_region_init_alias(>pci_hole, OBJECT(d), "pci-hole", >pci_mmio,
 0x8000ULL, 0x1000ULL);
memory_region_add_subregion(address_space_mem, 0x8000ULL,
@@ -247,7 +256,6 @@ UNINState *pci_pmac_u3_init(qemu_irq *pic,
h = PCI_HOST_BRIDGE(dev);
d = U3_AGP_HOST_BRIDGE(dev);

-memory_region_init(>pci_mmio, OBJECT(d), "pci-mmio", 0x1ULL);
memory_region_init_alias(>pci_hole, OBJECT(d), "pci-hole", >pci_mmio,
 0x8000ULL, 0x7000ULL);
memory_region_add_subregion(address_space_mem, 0x8000ULL,