diff mbox

[07/19] uninorth: move PCI mmio memory region initialisation into init function

Message ID 20180306203103.25563-8-mark.cave-ayland@ilande.co.uk (mailing list archive)
State New, archived
Headers show

Commit Message

Mark Cave-Ayland March 6, 2018, 8:30 p.m. UTC 
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 <mark.cave-ayland@ilande.co.uk>
---
 hw/pci-host/uninorth.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

Comments

BALATON Zoltan March 6, 2018, 11:44 p.m. UTC | #1 
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 <mark.cave-ayland@ilande.co.uk>
> ---
> 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(&h->conf_mem, OBJECT(h), &pci_host_conf_le_ops,
> -                          obj, "pci-conf-idx", 0x1000);
> +                          obj, "unin-pci-conf-idx", 0x1000);
>     memory_region_init_io(&h->data_mem, OBJECT(h), &unin_data_ops, obj,
> -                          "pci-conf-data", 0x1000);
> +                          "unin-pci-conf-data", 0x1000);
> +
> +    memory_region_init(&s->pci_mmio, OBJECT(s), "unin-pci-mmio",
> +                       0x100000000ULL);
> +
>     sysbus_init_mmio(sbd, &h->conf_mem);
>     sysbus_init_mmio(sbd, &h->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(&h->conf_mem, OBJECT(h), &pci_host_conf_le_ops,
> -                          obj, "pci-conf-idx", 0x1000);
> +                          obj, "unin-pci-conf-idx", 0x1000);
>     memory_region_init_io(&h->data_mem, OBJECT(h), &unin_data_ops, obj,
> -                          "pci-conf-data", 0x1000);
> +                          "unin-pci-conf-data", 0x1000);
> +
> +    memory_region_init(&s->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

> +                       0x100000000ULL);
> +
>     sysbus_init_mmio(sbd, &h->conf_mem);
>     sysbus_init_mmio(sbd, &h->data_mem);
> }
> @@ -145,9 +155,9 @@ static void pci_unin_agp_init(Object *obj)
>
>     /* Uninorth AGP bus */
>     memory_region_init_io(&h->conf_mem, OBJECT(h), &pci_host_conf_le_ops,
> -                          obj, "pci-conf-idx", 0x1000);
> +                          obj, "unin-agp-conf-idx", 0x1000);
>     memory_region_init_io(&h->data_mem, OBJECT(h), &pci_host_data_le_ops,
> -                          obj, "pci-conf-data", 0x1000);
> +                          obj, "unin-agp-conf-data", 0x1000);
>     sysbus_init_mmio(sbd, &h->conf_mem);
>     sysbus_init_mmio(sbd, &h->data_mem);
> }
> @@ -159,9 +169,9 @@ static void pci_unin_internal_init(Object *obj)
>
>     /* Uninorth internal bus */
>     memory_region_init_io(&h->conf_mem, OBJECT(h), &pci_host_conf_le_ops,
> -                          obj, "pci-conf-idx", 0x1000);
> +                          obj, "unin-pci-conf-idx", 0x1000);
>     memory_region_init_io(&h->data_mem, OBJECT(h), &pci_host_data_le_ops,
> -                          obj, "pci-conf-data", 0x1000);
> +                          obj, "unin-pci-conf-data", 0x1000);
>     sysbus_init_mmio(sbd, &h->conf_mem);
>     sysbus_init_mmio(sbd, &h->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(&d->pci_mmio, OBJECT(d), "pci-mmio", 0x100000000ULL);
>     memory_region_init_alias(&d->pci_hole, OBJECT(d), "pci-hole", &d->pci_mmio,
>                              0x80000000ULL, 0x10000000ULL);
>     memory_region_add_subregion(address_space_mem, 0x80000000ULL,
> @@ -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(&d->pci_mmio, OBJECT(d), "pci-mmio", 0x100000000ULL);
>     memory_region_init_alias(&d->pci_hole, OBJECT(d), "pci-hole", &d->pci_mmio,
>                              0x80000000ULL, 0x70000000ULL);
>     memory_region_add_subregion(address_space_mem, 0x80000000ULL,
>
Mark Cave-Ayland March 7, 2018, 7:02 a.m. UTC | #2 
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 <mark.cave-ayland@ilande.co.uk>
>> ---
>> 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(&h->conf_mem, OBJECT(h), &pci_host_conf_le_ops,
>> -                          obj, "pci-conf-idx", 0x1000);
>> +                          obj, "unin-pci-conf-idx", 0x1000);
>>     memory_region_init_io(&h->data_mem, OBJECT(h), &unin_data_ops, obj,
>> -                          "pci-conf-data", 0x1000);
>> +                          "unin-pci-conf-data", 0x1000);
>> +
>> +    memory_region_init(&s->pci_mmio, OBJECT(s), "unin-pci-mmio",
>> +                       0x100000000ULL);
>> +
>>     sysbus_init_mmio(sbd, &h->conf_mem);
>>     sysbus_init_mmio(sbd, &h->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(&h->conf_mem, OBJECT(h), &pci_host_conf_le_ops,
>> -                          obj, "pci-conf-idx", 0x1000);
>> +                          obj, "unin-pci-conf-idx", 0x1000);
>>     memory_region_init_io(&h->data_mem, OBJECT(h), &unin_data_ops, obj,
>> -                          "pci-conf-data", 0x1000);
>> +                          "unin-pci-conf-data", 0x1000);
>> +
>> +    memory_region_init(&s->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.
Mark Cave-Ayland March 7, 2018, 7:23 a.m. UTC | #3 
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 <mark.cave-ayland@ilande.co.uk>
>>> ---
>>> 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(&h->conf_mem, OBJECT(h), 
>>> &pci_host_conf_le_ops,
>>> -                          obj, "pci-conf-idx", 0x1000);
>>> +                          obj, "unin-pci-conf-idx", 0x1000);
>>>     memory_region_init_io(&h->data_mem, OBJECT(h), &unin_data_ops, obj,
>>> -                          "pci-conf-data", 0x1000);
>>> +                          "unin-pci-conf-data", 0x1000);
>>> +
>>> +    memory_region_init(&s->pci_mmio, OBJECT(s), "unin-pci-mmio",
>>> +                       0x100000000ULL);
>>> +
>>>     sysbus_init_mmio(sbd, &h->conf_mem);
>>>     sysbus_init_mmio(sbd, &h->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(&h->conf_mem, OBJECT(h), 
>>> &pci_host_conf_le_ops,
>>> -                          obj, "pci-conf-idx", 0x1000);
>>> +                          obj, "unin-pci-conf-idx", 0x1000);
>>>     memory_region_init_io(&h->data_mem, OBJECT(h), &unin_data_ops, obj,
>>> -                          "pci-conf-data", 0x1000);
>>> +                          "unin-pci-conf-data", 0x1000);
>>> +
>>> +    memory_region_init(&s->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 0xf0000000 which is actually the AGP bus rather 
than the PCI bus at 0xf2000000...


ATB,

Mark.
diff mbox

Patch

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(&h->conf_mem, OBJECT(h), &pci_host_conf_le_ops,
-                          obj, "pci-conf-idx", 0x1000);
+                          obj, "unin-pci-conf-idx", 0x1000);
     memory_region_init_io(&h->data_mem, OBJECT(h), &unin_data_ops, obj,
-                          "pci-conf-data", 0x1000);
+                          "unin-pci-conf-data", 0x1000);
+
+    memory_region_init(&s->pci_mmio, OBJECT(s), "unin-pci-mmio",
+                       0x100000000ULL);
+
     sysbus_init_mmio(sbd, &h->conf_mem);
     sysbus_init_mmio(sbd, &h->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(&h->conf_mem, OBJECT(h), &pci_host_conf_le_ops,
-                          obj, "pci-conf-idx", 0x1000);
+                          obj, "unin-pci-conf-idx", 0x1000);
     memory_region_init_io(&h->data_mem, OBJECT(h), &unin_data_ops, obj,
-                          "pci-conf-data", 0x1000);
+                          "unin-pci-conf-data", 0x1000);
+
+    memory_region_init(&s->pci_mmio, OBJECT(s), "unin-pci-mmio",
+                       0x100000000ULL);
+
     sysbus_init_mmio(sbd, &h->conf_mem);
     sysbus_init_mmio(sbd, &h->data_mem);
 }
@@ -145,9 +155,9 @@  static void pci_unin_agp_init(Object *obj)
 
     /* Uninorth AGP bus */
     memory_region_init_io(&h->conf_mem, OBJECT(h), &pci_host_conf_le_ops,
-                          obj, "pci-conf-idx", 0x1000);
+                          obj, "unin-agp-conf-idx", 0x1000);
     memory_region_init_io(&h->data_mem, OBJECT(h), &pci_host_data_le_ops,
-                          obj, "pci-conf-data", 0x1000);
+                          obj, "unin-agp-conf-data", 0x1000);
     sysbus_init_mmio(sbd, &h->conf_mem);
     sysbus_init_mmio(sbd, &h->data_mem);
 }
@@ -159,9 +169,9 @@  static void pci_unin_internal_init(Object *obj)
 
     /* Uninorth internal bus */
     memory_region_init_io(&h->conf_mem, OBJECT(h), &pci_host_conf_le_ops,
-                          obj, "pci-conf-idx", 0x1000);
+                          obj, "unin-pci-conf-idx", 0x1000);
     memory_region_init_io(&h->data_mem, OBJECT(h), &pci_host_data_le_ops,
-                          obj, "pci-conf-data", 0x1000);
+                          obj, "unin-pci-conf-data", 0x1000);
     sysbus_init_mmio(sbd, &h->conf_mem);
     sysbus_init_mmio(sbd, &h->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(&d->pci_mmio, OBJECT(d), "pci-mmio", 0x100000000ULL);
     memory_region_init_alias(&d->pci_hole, OBJECT(d), "pci-hole", &d->pci_mmio,
                              0x80000000ULL, 0x10000000ULL);
     memory_region_add_subregion(address_space_mem, 0x80000000ULL,
@@ -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(&d->pci_mmio, OBJECT(d), "pci-mmio", 0x100000000ULL);
     memory_region_init_alias(&d->pci_hole, OBJECT(d), "pci-hole", &d->pci_mmio,
                              0x80000000ULL, 0x70000000ULL);
     memory_region_add_subregion(address_space_mem, 0x80000000ULL,