[PATCH 0/5] PCI address and property fixes

[Mark Cave-Ayland]

This patchset primarily fixes up pci_bus_addr_to_host_addr() to support
both memory and IO spaces in order to generate a correct AAPL,address
property for IO space mappings. As well as this, it removes the 
configuration space range from the PCI host bridge for all machines except
SPARC64 which fixes an issue with Darwin/OS X calculating the wrong address
spaces for PCI accesses.

Signed-off-by: Mark Cave-Ayland

Mark Cave-Ayland (5):
  pci: switch ob_pci_map_in() to use physical addresses rather than
    region and size
  pci: support PCI spaces other than memory in
    pci_bus_addr_to_host_addr()
  pci: fix AAPL,address property for IO space mappings
  pci: enable AAPL,address property for all Apple PPC machines
  pci: remove the configuration space range from the PCI host bridge by
    default

openbios-devel/drivers/pci.c  |   56 ++++++++++++++++++++++++---------
 openbios-devel/drivers/pci.fs |   68 +++++------------------------------------
 openbios-devel/drivers/vga.fs |   11 ++++---
 3 files changed, 56 insertions(+), 79 deletions(-)

[PATCH 1/5] pci: switch ob_pci_map_in() to use physical addresses rather than region and size

[Mark Cave-Ayland]

This matches the parameters for the map-in word as defined in the IEEE-1275
PCI bindings and provides extra information about the address space in use to
be used later. Even better this allows us to considerably simplify the Forth.

Signed-off-by: Mark Cave-Ayland 
---
 openbios-devel/drivers/pci.c  |    3 +-
 openbios-devel/drivers/pci.fs |   68 +++++------------------------------------
 openbios-devel/drivers/vga.fs |   11 ++++---
 3 files changed, 16 insertions(+), 66 deletions(-)

diff --git a/openbios-devel/drivers/pci.c b/openbios-devel/drivers/pci.c
index 935ecb8..6347118 100644
--- a/openbios-devel/drivers/pci.c
+++ b/openbios-devel/drivers/pci.c
@@ -340,7 +340,7 @@ ob_pci_encode_unit(int *idx)
            ss, dev, fn, buf);
 }

-/* ( pci-addr.lo pci-addr.hi size -- virt ) */
+/* ( pci-addr.lo pci-addr.mid pci-addr.hi size -- virt ) */

static void
 ob_pci_map_in(int *idx)
@@ -353,6 +353,7 @@ ob_pci_map_in(int *idx)

size = POP();
    POP();
+	POP();
    ba = POP();

phys = pci_bus_addr_to_host_addr(ba);
diff --git a/openbios-devel/drivers/pci.fs b/openbios-devel/drivers/pci.fs
index 563b652..a7b56e1 100644
--- a/openbios-devel/drivers/pci.fs
+++ b/openbios-devel/drivers/pci.fs
@@ -12,59 +12,19 @@
   rot encode-int encode+ 
   ;

-\ Get region offset for BAR reg
-: pci-bar-offset@ ( bar-reg -- off.lo off.hi -1 | 0 )
-  " reg" active-package get-package-property 0= if
-    begin
-      decode-phys    \ ( reg prop prop-len phys.lo phys.mid phys.hi )
-      ff and 5 pick = if
-        >r >r 3drop r> r>
-        -1 exit
-      else
-        2drop
-      then
-      \ Drop the size as we don't need it
-      decode-int drop decode-int drop
-      dup 0=
-    until
-    3drop
-    0 exit
-  else
-    0
-  then
-  ;
-
-\ Get region size for BAR reg
-: pci-bar-size@ ( bar-reg -- size )
-  " reg" active-package get-package-property 0= if
-    begin
-      decode-phys    \ ( reg prop prop-len phys.lo phys.mid phys.hi )
-      ff and 5 pick = if
-        2drop decode-int drop
-        decode-int
-        >r 3drop r>
-        exit
-      else
-        2drop decode-int drop
-        decode-int drop
-      then
-      dup 0=
-    until
-    3drop
-    0    \ default size of 0 if BAR not found
-  then
-  ;
-
-\ Get base address for configured BAR reg
-: pci-bar-base@ ( bar-reg -- addr.lo addr.hi -1 | 0 )
+\ Get PCI physical address and size for configured BAR reg
+: pci-bar>pci-addr ( bar-reg -- addr.lo addr.mid addr.hi size -1 | 0 )
   " assigned-addresses" active-package get-package-property 0= if
     begin
       decode-phys    \ ( reg prop prop-len phys.lo phys.mid phys.hi )
-      ff and 5 pick = if
-        >r >r 3drop r> r>
+      dup ff and 6 pick = if
+        >r >r >r rot drop
+        decode-int drop decode-int
+        -rot 2drop
+        r> swap r> r> rot
         -1 exit
       else
-        2drop
+        3drop
       then
       \ Drop the size as we don't need it
       decode-int drop decode-int drop
@@ -77,16 +37,4 @@
   then
   ;

-\ Get PCI bus address and size for configured BAR reg
-: pci-bar>pci-region  ( bar-reg -- addr.lo addr.hi size )
-  dup
-  >r pci-bar-offset@ if
-    swap r@ pci-bar-base@ if
-      swap d+
-    then
-    swap r@ pci-bar-size@
-  then
-  r> drop
-  ;
-
 [THEN]
diff --git a/openbios-devel/drivers/vga.fs b/openbios-devel/drivers/vga.fs
index ec4c6c5..29a043a 100644
--- a/openbios-devel/drivers/vga.fs
+++ b/openbios-devel/drivers/vga.fs
@@ -109,16 +109,17 @@ h# 1 constant VBE_DISPI_ENABLED
 \ PCI
 \

-" pci-bar>pci-region" (find-xt) value pci-bar>pci-region-xt
-: pci-bar>pci-region pci-bar>pci-region-xt execute ;
+" pci-bar>pci-addr" (find-xt) value pci-bar>pci-addr-xt
+: pci-bar>pci-addr pci-bar>pci-addr-xt execute ;

h# 10 constant cfg-bar0    \ Framebuffer BAR
 -1 value fb-addr

: map-fb ( -- )
-  cfg-bar0 pci-bar>pci-region   \ ( pci-addr.lo pci-addr.hi size )
-  " pci-map-in" $call-parent
-  to fb-addr
+  cfg-bar0 pci-bar>pci-addr if   \ ( pci-addr.lo pci-addr.mid pci-addr.hi size )
+    " pci-map-in" $call-parent
+    to fb-addr
+  then
 ;

\

[PATCH 2/5] pci: support PCI spaces other than memory in pci_bus_addr_to_host_addr()

[Mark Cave-Ayland]

Currently only PCI memory space is mapped in ob_pci_map_in(). By adding
multi-space support to pci_bus_addr_to_host_addr(), it becomes possible
to translate physical addresses for both memory and IO space.

Signed-off-by: Mark Cave-Ayland 
---
 openbios-devel/drivers/pci.c |   28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

diff --git a/openbios-devel/drivers/pci.c b/openbios-devel/drivers/pci.c
index 6347118..97d3ef3 100644
--- a/openbios-devel/drivers/pci.c
+++ b/openbios-devel/drivers/pci.c
@@ -144,9 +144,13 @@ static void dump_reg_property(const char* description, int nreg, u32 *reg)
 }
 #endif

-static unsigned long pci_bus_addr_to_host_addr(uint32_t ba)
+static unsigned long pci_bus_addr_to_host_addr(int space, uint32_t ba)
 {
-    return arch->host_pci_base + (unsigned long)ba;
+    if (space == IO_SPACE) {
+        return arch->io_base + (unsigned long)ba;
+    } else {
+        return arch->host_pci_base + (unsigned long)ba;
+    }
 }

static void
@@ -347,16 +351,20 @@ ob_pci_map_in(int *idx)
 {
    phys_addr_t phys;
    uint32_t ba;
-	ucell size, virt;
+	ucell size, virt, tmp;
+	int space;

PCI_DPRINTF("ob_pci_bar_map_in idx=%p\n", idx);

size = POP();
-	POP();
+	tmp = POP();
    POP();
    ba = POP();

-	phys = pci_bus_addr_to_host_addr(ba);
+	/* Get the space from the pci-addr.hi */
+	space = ((tmp & 0x03000000) >> 24);
+
+	phys = pci_bus_addr_to_host_addr(space, ba);

#if defined(CONFIG_OFMEM)
    ofmem_claim_phys(phys, size, 0);
@@ -753,13 +761,19 @@ int macio_keylargo_config_cb (const pci_config_t *config)
 int vga_config_cb (const pci_config_t *config)
 {
         unsigned long rom;
-        uint32_t rom_size, size;
+        uint32_t rom_size, size, mask;
+        int flags, space_code;
         phandle_t ph;

if (config->assigned[0] != 0x00000000) {
             setup_video();

-            rom = pci_bus_addr_to_host_addr(config->assigned[1] & ~0x0000000F);
+            pci_decode_pci_addr(config->assigned[1],
+                &flags, &space_code, &mask);
+
+            rom = pci_bus_addr_to_host_addr(space_code,
+                                            config->assigned[1] & ~0x0000000F);
+
             rom_size = config->sizes[1];

ph = get_cur_dev();

[PATCH 2/5] pci: support PCI spaces other than memory in pci_bus_addr_to_host_addr()

[Alexander Graf]

On 02.01.16 21:44, Mark Cave-Ayland wrote:
> Currently only PCI memory space is mapped in ob_pci_map_in(). By adding
> multi-space support to pci_bus_addr_to_host_addr(), it becomes possible
> to translate physical addresses for both memory and IO space.
> Signed-off-by: Mark Cave-Ayland
>
> openbios-devel/drivers/pci.c |   28 +++++++++++++++++++++-------
>  1 file changed, 21 insertions(+), 7 deletions(-)
>
> diff --git a/openbios-devel/drivers/pci.c b/openbios-devel/drivers/pci.c
> index 6347118..97d3ef3 100644
> --- a/openbios-devel/drivers/pci.c
> +++ b/openbios-devel/drivers/pci.c
> @@ -144,9 +144,13 @@ static void dump_reg_property(const char* description, int nreg, u32 *reg)
>  }
>  #endif
>
> -static unsigned long pci_bus_addr_to_host_addr(uint32_t ba)
> +static unsigned long pci_bus_addr_to_host_addr(int space, uint32_t ba)
>  {
> return arch->host_pci_base + (unsigned long)ba;
> if (space == IO_SPACE) {
>    return arch->io_base + (unsigned long)ba;
> } else {
>    return arch->host_pci_base + (unsigned long)ba;
> }
>
> }
>
> static void
> @@ -347,16 +351,20 @@ ob_pci_map_in(int *idx)
>  {
>    phys_addr_t phys;
>    uint32_t ba;
> ucell size, virt;
>
> ucell size, virt, tmp;
>
> int space;
>
> PCI_DPRINTF("ob_pci_bar_map_in idx=%p\n", idx);
>
> size = POP();
> POP();
> tmp = POP();
> POP();
> ba = POP();
> phys = pci_bus_addr_to_host_addr(ba);
> /* Get the space from the pci-addr.hi */
> space = ((tmp & 0x03000000) >> 24);

You could try to reuse the defines we have in QEMU for the parameter bits:

#define FDT_PCI_RANGE_RELOCATABLE          0x80000000
#define FDT_PCI_RANGE_PREFETCHABLE         0x40000000
#define FDT_PCI_RANGE_ALIASED              0x20000000
#define FDT_PCI_RANGE_TYPE_MASK            0x03000000
#define FDT_PCI_RANGE_MMIO_64BIT           0x03000000
#define FDT_PCI_RANGE_MMIO                 0x02000000
#define FDT_PCI_RANGE_IOPORT               0x01000000
#define FDT_PCI_RANGE_CONFIG               0x00000000

While at it, you probably also want to double-check you're not mapping
MMIO_64BIT or CONFIG by accident ;).

Alex
> phys = pci_bus_addr_to_host_addr(space, ba);
>
> #if defined(CONFIG_OFMEM)
>    ofmem_claim_phys(phys, size, 0);
> @@ -753,13 +761,19 @@ int macio_keylargo_config_cb (const pci_config_t *config)
>  int vga_config_cb (const pci_config_t *config)
>  {
>          unsigned long rom;
>    uint32_t rom_size, size;
>    uint32_t rom_size, size, mask;
>    int flags, space_code;
>    phandle_t ph;
>
>    if (config->assigned[0] != 0x00000000) {
>        setup_video();
>
>        rom = pci_bus_addr_to_host_addr(config->assigned[1] & ~0x0000000F);
>        pci_decode_pci_addr(config->assigned[1],
>            &flags, &space_code, &mask);
>        rom = pci_bus_addr_to_host_addr(space_code,
>                                        config->assigned[1] & ~0x0000000F);
>        rom_size = config->sizes[1];
>
>        ph = get_cur_dev();

[PATCH 2/5] pci: support PCI spaces other than memory in pci_bus_addr_to_host_addr()

[Mark Cave-Ayland]

On 04/01/16 08:05, Alexander Graf wrote:
> On 02.01.16 21:44, Mark Cave-Ayland wrote:
> > Currently only PCI memory space is mapped in ob_pci_map_in(). By adding
> > multi-space support to pci_bus_addr_to_host_addr(), it becomes possible
> > to translate physical addresses for both memory and IO space.
> > Signed-off-by: Mark Cave-Ayland
> >
> > openbios-devel/drivers/pci.c |   28 +++++++++++++++++++++-------
> >  1 file changed, 21 insertions(+), 7 deletions(-)
> >
> > diff --git a/openbios-devel/drivers/pci.c b/openbios-devel/drivers/pci.c
> > index 6347118..97d3ef3 100644
> > --- a/openbios-devel/drivers/pci.c
> > +++ b/openbios-devel/drivers/pci.c
> > @@ -144,9 +144,13 @@ static void dump_reg_property(const char* description, int nreg, u32 *reg)
> >  }
> >  #endif
> >
> > -static unsigned long pci_bus_addr_to_host_addr(uint32_t ba)
> > +static unsigned long pci_bus_addr_to_host_addr(int space, uint32_t ba)
> >  {
> > return arch->host_pci_base + (unsigned long)ba;
> > if (space == IO_SPACE) {
> >    return arch->io_base + (unsigned long)ba;
> > } else {
> >    return arch->host_pci_base + (unsigned long)ba;
> > }
> >
> > }
> >
> > static void
> > @@ -347,16 +351,20 @@ ob_pci_map_in(int *idx)
> >  {
> >    phys_addr_t phys;
> >    uint32_t ba;
> > ucell size, virt;
> >
> > ucell size, virt, tmp;
> >
> > int space;
> >
> > PCI_DPRINTF("ob_pci_bar_map_in idx=%p\n", idx);
> >
> > size = POP();
> > POP();
> > tmp = POP();
> > POP();
> > ba = POP();
> > phys = pci_bus_addr_to_host_addr(ba);
> > /* Get the space from the pci-addr.hi */
> > space = ((tmp & 0x03000000) >> 24);
>
> You could try to reuse the defines we have in QEMU for the parameter bits:
>
> #define FDT_PCI_RANGE_RELOCATABLE          0x80000000
> #define FDT_PCI_RANGE_PREFETCHABLE         0x40000000
> #define FDT_PCI_RANGE_ALIASED              0x20000000
> #define FDT_PCI_RANGE_TYPE_MASK            0x03000000
> #define FDT_PCI_RANGE_MMIO_64BIT           0x03000000
> #define FDT_PCI_RANGE_MMIO                 0x02000000
> #define FDT_PCI_RANGE_IOPORT               0x01000000
> #define FDT_PCI_RANGE_CONFIG               0x00000000

Yeah that seems a good idea - I'll add them into drivers/pci.h without
the FDT_ prefix in the next respin.
> While at it, you probably also want to double-check you're not mapping
> MMIO_64BIT or CONFIG by accident ;).

The default behaviour was just to return the address unaltered, so I'll
add that in too. Not that it will help anything that tries to access an
incorrect address, but at the very least it will appear unaltered in any
properties which makes it quite easy to spot.

I also have another patchset which tidies up the OpenPIC mappings which
I'll include in the next revision too, hopefully with the aim of
committing tomorrow to help clear my patch backlog(!)

ATB,

Mark.

[PATCH 3/5] pci: fix AAPL, address property for IO space mappings

[Mark Cave-Ayland]

According to "Designing PCI Cards and Drivers for Power Macintosh Computers" the
AAPL,address property should be an array of virtual addresses representing the
bus addresses in the assigned-addresses array.

While this works for PCI memory addresses, IO addresses are incorrect because
the calculated address is still the PCI bus address. Switch over to using the
new space-aware pci_bus_addr_to_host_addr() so that addresses in both memory
and IO space are correctly calculated.

Signed-off-by: Mark Cave-Ayland 
---
 openbios-devel/drivers/pci.c |    9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/openbios-devel/drivers/pci.c b/openbios-devel/drivers/pci.c
index 97d3ef3..45e954b 100644
--- a/openbios-devel/drivers/pci.c
+++ b/openbios-devel/drivers/pci.c
@@ -594,13 +594,18 @@ static void pci_set_AAPL_address(const pci_config_t *config)
 {
    phandle_t dev = get_cur_dev();
    cell props[7];
-	int ncells, i;
+	uint32_t mask;
+	int ncells, i, flags, space_code;

ncells = 0;
    for (i = 0; i < 6; i++) {
    	if (!config->assigned[i] || !config->sizes[i])
    		continue;
-		props[ncells++] = config->assigned[i] & ~0x0000000F;
+		pci_decode_pci_addr(config->assigned[i],
+				    &flags, &space_code, &mask);
+
+		props[ncells++] = pci_bus_addr_to_host_addr(space_code,
+					config->assigned[i] & ~mask);
    }
    if (ncells)
    	set_property(dev, "AAPL,address", (char *)props,

[PATCH 4/5] pci: enable AAPL, address property for all Apple PPC machines

[Mark Cave-Ayland]

The property appears in device trees for both Old World and New World Macs
so let's do the same.

Signed-off-by: Mark Cave-Ayland 
---
 openbios-devel/drivers/pci.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/openbios-devel/drivers/pci.c b/openbios-devel/drivers/pci.c
index 45e954b..65f5fe9 100644
--- a/openbios-devel/drivers/pci.c
+++ b/openbios-devel/drivers/pci.c
@@ -1017,7 +1017,10 @@ static void ob_pci_add_properties(phandle_t phandle,
    }

pci_set_assigned_addresses(phandle, config, num_bars);
-	OLDWORLD(pci_set_AAPL_address(config));
+	
+	if (is_apple()) {
+		OLDWORLD(pci_set_AAPL_address(config));
+	}

PCI_DPRINTF("\n");
 }

[PATCH 4/5] pci: enable AAPL, address property for all Apple PPC machines

[BALATON Zoltan]

On Sat, 2 Jan 2016, Mark Cave-Ayland wrote:
> The property appears in device trees for both Old World and New World Macs
> so let's do the same.
> Signed-off-by: Mark Cave-Ayland
>
> openbios-devel/drivers/pci.c |    5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/openbios-devel/drivers/pci.c b/openbios-devel/drivers/pci.c
> index 45e954b..65f5fe9 100644
> --- a/openbios-devel/drivers/pci.c
> +++ b/openbios-devel/drivers/pci.c
> @@ -1017,7 +1017,10 @@ static void ob_pci_add_properties(phandle_t phandle,
>    }
>
> pci_set_assigned_addresses(phandle, config, num_bars);
> OLDWORLD(pci_set_AAPL_address(config));
> if (is_apple()) {
> OLDWORLD(pci_set_AAPL_address(config));

The commit message suggests you intend to add it to New World machines too 
but it is still enclosed in OLDWORLD() after this patch. Is that correct? 
I think you wanted to replace OLDWORLD() with if(is_apple()).

Regards,
BALATON Zoltan
> }
>
> PCI_DPRINTF("\n");
>
> }

[PATCH 4/5] pci: enable AAPL, address property for all Apple PPC machines

[Mark Cave-Ayland]

On 02/01/16 22:41, BALATON Zoltan wrote:
> On Sat, 2 Jan 2016, Mark Cave-Ayland wrote:
> > The property appears in device trees for both Old World and New World
> > Macs
> > so let's do the same.
> > Signed-off-by: Mark Cave-Ayland
> >
> > openbios-devel/drivers/pci.c |    5 ++++-
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/openbios-devel/drivers/pci.c b/openbios-devel/drivers/pci.c
> > index 45e954b..65f5fe9 100644
> > --- a/openbios-devel/drivers/pci.c
> > +++ b/openbios-devel/drivers/pci.c
> > @@ -1017,7 +1017,10 @@ static void ob_pci_add_properties(phandle_t
> > phandle,
> >     }
> > pci_set_assigned_addresses(phandle, config, num_bars);
> > OLDWORLD(pci_set_AAPL_address(config));
> > if (is_apple()) {
> >    OLDWORLD(pci_set_AAPL_address(config));
>
> The commit message suggests you intend to add it to New World machines
> too but it is still enclosed in OLDWORLD() after this patch. Is that
> correct? I think you wanted to replace OLDWORLD() with if(is_apple()).

Ah yes, and in fact the fix is to remove the surrounding OLDWORLD()
macro completely. Thanks for the review - I don't suppose it makes any
difference to MorphOS at all?

ATB,

Mark.

[PATCH 4/5] pci: enable AAPL, address property for all Apple PPC machines

[Programmingkid]

On Jan 2, 2016, at 5:56 PM, Mark Cave-Ayland wrote:
> On 02/01/16 22:41, BALATON Zoltan wrote:
> > On Sat, 2 Jan 2016, Mark Cave-Ayland wrote:
> > > The property appears in device trees for both Old World and New World
> > > Macs
> > > so let's do the same.
> > > Signed-off-by: Mark Cave-Ayland
> > >
> > > openbios-devel/drivers/pci.c |    5 ++++-
> > > 1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/openbios-devel/drivers/pci.c b/openbios-devel/drivers/pci.c
> > > index 45e954b..65f5fe9 100644
> > > --- a/openbios-devel/drivers/pci.c
> > > +++ b/openbios-devel/drivers/pci.c
> > > @@ -1017,7 +1017,10 @@ static void ob_pci_add_properties(phandle_t
> > > phandle,
> > >    }
> > >
> > > pci_set_assigned_addresses(phandle, config, num_bars);
> > > OLDWORLD(pci_set_AAPL_address(config));
> > > if (is_apple()) {
> > >    OLDWORLD(pci_set_AAPL_address(config));
> >
> > The commit message suggests you intend to add it to New World machines
> > too but it is still enclosed in OLDWORLD() after this patch. Is that
> > correct? I think you wanted to replace OLDWORLD() with if(is_apple()).
>
> Ah yes, and in fact the fix is to remove the surrounding OLDWORLD()
> macro completely. Thanks for the review - I don't suppose it makes any
> difference to MorphOS at all?

Excellent job Mark as usual. Your patches work. I have tested all of them in this series. I even removed the OLDWORLD function call like you suggested. Mac OS 10.4 is able to use the RTL8139 without problem. The PCI config space is accessed successfully from the driver. The fix does not hurt PPC Linux or Windows XP's access to the RTL8139 NIC.

reviewed-by: [e-mail]

[PATCH 4/5] pci: enable AAPL, address property for all Apple PPC machines

[Mark Cave-Ayland]

On 03/01/16 00:40, Programmingkid wrote:
> Excellent job Mark as usual. Your patches work. I have tested all of them in this series. I even removed the OLDWORLD function call like you suggested. Mac OS 10.4 is able to use the RTL8139 without problem. The PCI config space is accessed successfully from the driver. The fix does not hurt PPC Linux or Windows XP's access to the RTL8139 NIC.
>
> reviewed-by: [e-mail]

Great! I'll get this committed to SVN trunk as soon as I can get a
review from a PPC maintainer just to make sure I'm not doing anything
too ridiculous ;)

ATB,

Mark.

[PATCH 4/5] pci: enable AAPL, address property for all Apple PPC machines

[BALATON Zoltan]

On Sat, 2 Jan 2016, Mark Cave-Ayland wrote:
> macro completely. Thanks for the review - I don't suppose it makes any
> difference to MorphOS at all?

I gave it a try after reading the thread hoping that it may help if 
something similar is happening there than on Mac OS X. Unfortunately this 
is not enough for MorphOS and that still needs my bus master enable patch 
on top of yours for any network traffic but your patches and later changes 
in QEMU up to 2.5.0 did not break it and still apears to work as much as 
before but not any better either.

Regards,
BALATON Zoltan

[PATCH 4/5] pci: enable AAPL, address property for all Apple PPC machines

[Mark Cave-Ayland]

On 02/01/16 22:56, Mark Cave-Ayland wrote:
> On 02/01/16 22:41, BALATON Zoltan wrote:
> > On Sat, 2 Jan 2016, Mark Cave-Ayland wrote:
> > > The property appears in device trees for both Old World and New World
> > > Macs
> > > so let's do the same.
> > > Signed-off-by: Mark Cave-Ayland
> > >
> > > openbios-devel/drivers/pci.c |    5 ++++-
> > > 1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/openbios-devel/drivers/pci.c b/openbios-devel/drivers/pci.c
> > > index 45e954b..65f5fe9 100644
> > > --- a/openbios-devel/drivers/pci.c
> > > +++ b/openbios-devel/drivers/pci.c
> > > @@ -1017,7 +1017,10 @@ static void ob_pci_add_properties(phandle_t
> > > phandle,
> > >     }
> > > pci_set_assigned_addresses(phandle, config, num_bars);
> > > OLDWORLD(pci_set_AAPL_address(config));
> > > if (is_apple()) {
> > >    OLDWORLD(pci_set_AAPL_address(config));
> >
> > The commit message suggests you intend to add it to New World machines
> > too but it is still enclosed in OLDWORLD() after this patch. Is that
> > correct? I think you wanted to replace OLDWORLD() with if(is_apple()).
>
> Ah yes, and in fact the fix is to remove the surrounding OLDWORLD()
> macro completely. Thanks for the review - I don't suppose it makes any
> difference to MorphOS at all?

Actually this may be a red herring, since the AAPL,address properties
are present in the G4 AGP device trees but not the G4 PCI device trees.
So unless G4 AGP are old world then this patch might be a red herring. Alex?

ATB,

Mark.

[PATCH 4/5] pci: enable AAPL, address property for all Apple PPC machines

[BALATON Zoltan]

On Sun, 3 Jan 2016, Mark Cave-Ayland wrote:
> On 02/01/16 22:56, Mark Cave-Ayland wrote:
> > On 02/01/16 22:41, BALATON Zoltan wrote:
> > > On Sat, 2 Jan 2016, Mark Cave-Ayland wrote:
> > > > The property appears in device trees for both Old World and New World
> > > > Macs
> > > > so let's do the same.
> > > > Signed-off-by: Mark Cave-Ayland
> > > >
> > > > openbios-devel/drivers/pci.c |    5 ++++-
> > > > 1 file changed, 4 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/openbios-devel/drivers/pci.c b/openbios-devel/drivers/pci.c
> > > > index 45e954b..65f5fe9 100644
> > > > --- a/openbios-devel/drivers/pci.c
> > > > +++ b/openbios-devel/drivers/pci.c
> > > > @@ -1017,7 +1017,10 @@ static void ob_pci_add_properties(phandle_t
> > > > phandle,
> > > >     }
> > > > pci_set_assigned_addresses(phandle, config, num_bars);
> > > > OLDWORLD(pci_set_AAPL_address(config));
> > > > if (is_apple()) {
> > > >    OLDWORLD(pci_set_AAPL_address(config));
> > >
> > > The commit message suggests you intend to add it to New World machines
> > > too but it is still enclosed in OLDWORLD() after this patch. Is that
> > > correct? I think you wanted to replace OLDWORLD() with if(is_apple()).
> >
> > Ah yes, and in fact the fix is to remove the surrounding OLDWORLD()
> > macro completely. Thanks for the review - I don't suppose it makes any
> > difference to MorphOS at all?
>
> Actually this may be a red herring, since the AAPL,address properties
> are present in the G4 AGP device trees but not the G4 PCI device trees.
> So unless G4 AGP are old world then this patch might be a red herring. Alex?

I think all G4 Macs are New World as it started somewhere around G3 and 
all later models had New World ROM (that is OpenFirmware without Toolbox). 
So I'm not sure why you don't see these in G4 PCI or if you do it's not 
because of Old World ROM.

The model id we have now (PowerMac3,1) is corresponding to the G4 AGP and 
I've picked this because it is the minimum required by MorphOS. Previously 
it was probably some PCI model but the set of devices currently emulated 
by QEMU may not really correspond to any of these real machines.

Regards,
BALATON Zoltan

[PATCH 4/5] pci: enable AAPL, address property for all Apple PPC machines

[Alexander Graf]

On 04.01.16 02:57, BALATON Zoltan wrote:
> On Sun, 3 Jan 2016, Mark Cave-Ayland wrote:
> > On 02/01/16 22:56, Mark Cave-Ayland wrote:
> > > On 02/01/16 22:41, BALATON Zoltan wrote:
> > > > On Sat, 2 Jan 2016, Mark Cave-Ayland wrote:
> > > > > The property appears in device trees for both Old World and New World
> > > > > Macs
> > > > > so let's do the same.
> > > > > Signed-off-by: Mark Cave-Ayland
> > > > >
> > > > > openbios-devel/drivers/pci.c |    5 ++++-
> > > > > 1 file changed, 4 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/openbios-devel/drivers/pci.c
> > > > > b/openbios-devel/drivers/pci.c
> > > > > index 45e954b..65f5fe9 100644
> > > > > --- a/openbios-devel/drivers/pci.c
> > > > > +++ b/openbios-devel/drivers/pci.c
> > > > > @@ -1017,7 +1017,10 @@ static void ob_pci_add_properties(phandle_t
> > > > > phandle,
> > > > >     }
> > > > > pci_set_assigned_addresses(phandle, config, num_bars);
> > > > > OLDWORLD(pci_set_AAPL_address(config));
> > > > > if (is_apple()) {
> > > > >    OLDWORLD(pci_set_AAPL_address(config));
> > > >
> > > > The commit message suggests you intend to add it to New World machines
> > > > too but it is still enclosed in OLDWORLD() after this patch. Is that
> > > > correct? I think you wanted to replace OLDWORLD() with if(is_apple()).
> > >
> > > Ah yes, and in fact the fix is to remove the surrounding OLDWORLD()
> > > macro completely. Thanks for the review - I don't suppose it makes any
> > > difference to MorphOS at all?
> >
> > Actually this may be a red herring, since the AAPL,address properties
> > are present in the G4 AGP device trees but not the G4 PCI device trees.
> > So unless G4 AGP are old world then this patch might be a red herring.
> > Alex?
>
> I think all G4 Macs are New World as it started somewhere around G3 and
> all later models had New World ROM (that is OpenFirmware without
> Toolbox). So I'm not sure why you don't see these in G4 PCI or if you do
> it's not because of Old World ROM.
>
> The model id we have now (PowerMac3,1) is corresponding to the G4 AGP
> and I've picked this because it is the minimum required by MorphOS.
> Previously it was probably some PCI model but the set of devices
> currently emulated by QEMU may not really correspond to any of these
> real machines.

Yeah, my guess is that they left the legacy AAPL, bits in for Mac OS 9
compatibility in systems where they cared about it. Since it is present
in real world DTs, I think it's perfectly valid to expose it in OpenBIOS
as well.

Alex

[PATCH 4/5] pci: enable AAPL, address property for all Apple PPC machines

[Mark Cave-Ayland]

On 04/01/16 08:20, Alexander Graf wrote:
> On 04.01.16 02:57, BALATON Zoltan wrote:
> > On Sun, 3 Jan 2016, Mark Cave-Ayland wrote:
> > > On 02/01/16 22:56, Mark Cave-Ayland wrote:
> > > > On 02/01/16 22:41, BALATON Zoltan wrote:
> > > > > On Sat, 2 Jan 2016, Mark Cave-Ayland wrote:
> > > > > > The property appears in device trees for both Old World and New World
> > > > > > Macs
> > > > > > so let's do the same.
> > > > > > Signed-off-by: Mark Cave-Ayland
> > > > > >
> > > > > > openbios-devel/drivers/pci.c |    5 ++++-
> > > > > > 1 file changed, 4 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/openbios-devel/drivers/pci.c
> > > > > > b/openbios-devel/drivers/pci.c
> > > > > > index 45e954b..65f5fe9 100644
> > > > > > --- a/openbios-devel/drivers/pci.c
> > > > > > +++ b/openbios-devel/drivers/pci.c
> > > > > > @@ -1017,7 +1017,10 @@ static void ob_pci_add_properties(phandle_t
> > > > > > phandle,
> > > > > >     }
> > > > > > pci_set_assigned_addresses(phandle, config, num_bars);
> > > > > > OLDWORLD(pci_set_AAPL_address(config));
> > > > > > if (is_apple()) {
> > > > > >    OLDWORLD(pci_set_AAPL_address(config));
> > > > >
> > > > > The commit message suggests you intend to add it to New World machines
> > > > > too but it is still enclosed in OLDWORLD() after this patch. Is that
> > > > > correct? I think you wanted to replace OLDWORLD() with if(is_apple()).
> > > >
> > > > Ah yes, and in fact the fix is to remove the surrounding OLDWORLD()
> > > > macro completely. Thanks for the review - I don't suppose it makes any
> > > > difference to MorphOS at all?
> > >
> > > Actually this may be a red herring, since the AAPL,address properties
> > > are present in the G4 AGP device trees but not the G4 PCI device trees.
> > > So unless G4 AGP are old world then this patch might be a red herring.
> > > Alex?
> >
> > I think all G4 Macs are New World as it started somewhere around G3 and
> > all later models had New World ROM (that is OpenFirmware without
> > Toolbox). So I'm not sure why you don't see these in G4 PCI or if you do
> > it's not because of Old World ROM.
> >
> > The model id we have now (PowerMac3,1) is corresponding to the G4 AGP
> > and I've picked this because it is the minimum required by MorphOS.
> > Previously it was probably some PCI model but the set of devices
> > currently emulated by QEMU may not really correspond to any of these
> > real machines.
>
> Yeah, my guess is that they left the legacy AAPL, bits in for Mac OS 9
> compatibility in systems where they cared about it. Since it is present
> in real world DTs, I think it's perfectly valid to expose it in OpenBIOS
> as well.

In the examples I've looked at, AAPL,address isn't included in all
devices on the G4 PCI so I suspect it may be generated by older FCode
ROMs for cards that can work on both old world and new world Macs.

ATB,

Mark.

[PATCH 4/5] pci: enable AAPL, address property for all Apple PPC machines

[BALATON Zoltan]

On Mon, 4 Jan 2016, Mark Cave-Ayland wrote:
> On 04/01/16 08:20, Alexander Graf wrote:
> > On 04.01.16 02:57, BALATON Zoltan wrote:
> > > On Sun, 3 Jan 2016, Mark Cave-Ayland wrote:
> > > > Actually this may be a red herring, since the AAPL,address properties
> > > > are present in the G4 AGP device trees but not the G4 PCI device trees.
> > > > So unless G4 AGP are old world then this patch might be a red herring.
> > > > Alex?
> > >
> > > I think all G4 Macs are New World as it started somewhere around G3 and
> > > all later models had New World ROM (that is OpenFirmware without
> > > Toolbox). So I'm not sure why you don't see these in G4 PCI or if you do
> > > it's not because of Old World ROM.
> > >
> > > The model id we have now (PowerMac3,1) is corresponding to the G4 AGP
> > > and I've picked this because it is the minimum required by MorphOS.
> > > Previously it was probably some PCI model but the set of devices
> > > currently emulated by QEMU may not really correspond to any of these
> > > real machines.
> >
> > Yeah, my guess is that they left the legacy AAPL, bits in for Mac OS 9
> > compatibility in systems where they cared about it. Since it is present
> > in real world DTs, I think it's perfectly valid to expose it in OpenBIOS
> > as well.
>
> In the examples I've looked at, AAPL,address isn't included in all
> devices on the G4 PCI so I suspect it may be generated by older FCode
> ROMs for cards that can work on both old world and new world Macs.

The device tree I've seen here:
http://web.archive.org/web/20090107145016/http://penguinppc.org/historical/d....

only contains APPL,address properties for i2c nodes that lack the 
assigned-addresses property so I think it may have been added as an 
extension or were used before assigned-addresses was standardised and only 
retained for those devices where no standard property exists so having 
both of these on the same node may not make sense.

There's also AAPL,address-step that seems to appear together but I'm not 
sure what would be the standard property for that. Maybe #address-cells?

Regards,
BALATON Zoltan

[PATCH 4/5] pci: enable AAPL, address property for all Apple PPC machines

[Mark Cave-Ayland]

On 04/01/16 01:57, BALATON Zoltan wrote:
> > Actually this may be a red herring, since the AAPL,address properties
> > are present in the G4 AGP device trees but not the G4 PCI device trees.
> > So unless G4 AGP are old world then this patch might be a red herring.
> > Alex?
>
> I think all G4 Macs are New World as it started somewhere around G3 and
> all later models had New World ROM (that is OpenFirmware without
> Toolbox). So I'm not sure why you don't see these in G4 PCI or if you do
> it's not because of Old World ROM.
>
> The model id we have now (PowerMac3,1) is corresponding to the G4 AGP
> and I've picked this because it is the minimum required by MorphOS.
> Previously it was probably some PCI model but the set of devices
> currently emulated by QEMU may not really correspond to any of these
> real machines.

There are some reports over on emaculation that OS X 10.0 and 10.1 hang
in QEMU when trying to initialise the display, while 10.1 can run fine
on PearPC. I wonder if it's something to do with the fact that PearPC
provides a PCI rather than an AGP graphics card?

ATB,

Mark.

[PATCH 5/5] pci: remove the configuration space range from the PCI host bridge by default

[Mark Cave-Ayland]

While the configuration space range appears in real SPARC device trees, it
isn't mentioned in the IEEE-1275 PCI bindings and in fact causes Darwin/OS X
to calculate PCI address spaces incorrectly.

Disable this range in the PCI host bridge by default, except for SPARC64
where it can evidently still appear.

Signed-off-by: Mark Cave-Ayland 
---
 openbios-devel/drivers/pci.c |   13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/openbios-devel/drivers/pci.c b/openbios-devel/drivers/pci.c
index 65f5fe9..5627c90 100644
--- a/openbios-devel/drivers/pci.c
+++ b/openbios-devel/drivers/pci.c
@@ -457,13 +457,18 @@ static void pci_host_set_ranges(const pci_config_t *config)
    int ncells;

ncells = 0;
-	/* first encode PCI configuration space */
-	{
-	    ncells += pci_encode_phys_addr(props + ncells, 0, CONFIGURATION_SPACE,
+	
+#ifdef CONFIG_SPARC64
+        /* While configuration space isn't mentioned in the IEEE-1275 PCI
+           bindings, it appears in the PCI host bridge ranges property in
+           real device trees. Hence we disable this range for all host
+           bridges except for SPARC, particularly as it causes Darwin/OS X
+           to incorrectly calculated PCI memory space ranges on PPC. */
+	ncells += pci_encode_phys_addr(props + ncells, 0, CONFIGURATION_SPACE,
                      config->dev, 0, 0);
         ncells += host_encode_phys_addr(props + ncells, arch->cfg_addr);
         ncells += pci_encode_size(props + ncells, arch->cfg_len);
-	}
+#endif

if (arch->io_base) {
        ncells += pci_encode_phys_addr(props + ncells, 0, IO_SPACE,

[PATCH 5/5] pci: remove the configuration space range from the PCI host bridge by default

[Alexander Graf]

On 02.01.16 21:44, Mark Cave-Ayland wrote:
> While the configuration space range appears in real SPARC device trees, it
> isn't mentioned in the IEEE-1275 PCI bindings and in fact causes Darwin/OS X
> to calculate PCI address spaces incorrectly.
>
> Disable this range in the PCI host bridge by default, except for SPARC64
> where it can evidently still appear.
>
> Signed-off-by: Mark Cave-Ayland

This is an excerpt from a PowerMac3,1 device tree pci ranges node:

< 0x2000000 0x0 0xf5000000 0xf5000000 0x0 0x1000000
  0x1000000 0x0 0x0 0xf4000000 0x0 0x800000>;

So yes, only MMIO32 and PIO are set. Awesome ;).

Alex
> openbios-devel/drivers/pci.c |   13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/openbios-devel/drivers/pci.c b/openbios-devel/drivers/pci.c
> index 65f5fe9..5627c90 100644
> --- a/openbios-devel/drivers/pci.c
> +++ b/openbios-devel/drivers/pci.c
> @@ -457,13 +457,18 @@ static void pci_host_set_ranges(const pci_config_t *config)
>    int ncells;
>
> ncells = 0;
> /* first encode PCI configuration space */
> {
>    ncells += pci_encode_phys_addr(props + ncells, 0, CONFIGURATION_SPACE,
>
> +#ifdef CONFIG_SPARC64
>    /* While configuration space isn't mentioned in the IEEE-1275 PCI
>       bindings, it appears in the PCI host bridge ranges property in
>       real device trees. Hence we disable this range for all host
>       bridges except for SPARC, particularly as it causes Darwin/OS X
>       to incorrectly calculated PCI memory space ranges on PPC. */
> ncells += pci_encode_phys_addr(props + ncells, 0, CONFIGURATION_SPACE,
>                  config->dev, 0, 0);
>     ncells += host_encode_phys_addr(props + ncells, arch->cfg_addr);
>     ncells += pci_encode_size(props + ncells, arch->cfg_len);
> }
>
> +#endif
>
> if (arch->io_base) {
>        ncells += pci_encode_phys_addr(props + ncells, 0, IO_SPACE,

[PATCH 5/5] pci: remove the configuration space range from the PCI host bridge by default

[Mark Cave-Ayland]

On 04/01/16 08:34, Alexander Graf wrote:
> On 02.01.16 21:44, Mark Cave-Ayland wrote:
> > While the configuration space range appears in real SPARC device trees, it
> > isn't mentioned in the IEEE-1275 PCI bindings and in fact causes Darwin/OS X
> > to calculate PCI address spaces incorrectly.
> >
> > Disable this range in the PCI host bridge by default, except for SPARC64
> > where it can evidently still appear.
> >
> > Signed-off-by: Mark Cave-Ayland
>
> This is an excerpt from a PowerMac3,1 device tree pci ranges node:
>
> < 0x2000000 0x0 0xf5000000 0xf5000000 0x0 0x1000000
>   0x1000000 0x0 0x0 0xf4000000 0x0 0x800000>;
>
> So yes, only MMIO32 and PIO are set. Awesome ;).

Yup. And Darwin assumes fixed indexes for these in the ranges node
starting from 0, leading to the problem where IO reads were going to
config space, memory reads were going to IO space and MMIO reads were
just broken...

ATB,

Mark.

[Alexander Graf]

On 02.01.16 21:44, Mark Cave-Ayland wrote:
> This patchset primarily fixes up pci_bus_addr_to_host_addr() to support
> both memory and IO spaces in order to generate a correct AAPL,address
> property for IO space mappings. As well as this, it removes the 
> configuration space range from the PCI host bridge for all machines except
> SPARC64 which fixes an issue with Darwin/OS X calculating the wrong address
> spaces for PCI accesses.
>
> Signed-off-by: Mark Cave-Ayland

(all except patch 1/5, my forth is weak)
Reviewed-by: Alexander Graf
> Mark Cave-Ayland (5):
>   pci: switch ob_pci_map_in() to use physical addresses rather than
>     region and size
>   pci: support PCI spaces other than memory in
>     pci_bus_addr_to_host_addr()
>   pci: fix AAPL,address property for IO space mappings
>   pci: enable AAPL,address property for all Apple PPC machines
>   pci: remove the configuration space range from the PCI host bridge by
>     default
>
> openbios-devel/drivers/pci.c  |   56 ++++++++++++++++++++++++---------
>  openbios-devel/drivers/pci.fs |   68 +++++------------------------------------
>  openbios-devel/drivers/vga.fs |   11 ++++---
>  3 files changed, 56 insertions(+), 79 deletions(-)