[Qemu-devel] [PATCH] hw/pci-host/uninorth.c: Add support for Apple's PCI bridge register 0x48

[Qemu-devel] [PATCH] hw/pci-host/uninorth.c: Add support for Apple's PCI bridge register 0x48

[Programmingkid]

Apple has custom PCI bridge registers that are not a part of any known standard. This patch implements register 0x48. With this patch the AppleMacRiscPCI kernel extension no longer prints these error messages for the mac99 target:
AppleMacRiscPCI: bad range 2(80000000:01000000)
AppleMacRiscPCI: bad range 2(81000000:00001000)
AppleMacRiscPCI: bad range 2(81080000:00080000)

Signed-off-by: John Arbuckle <programmingkidx@gmail.com>

---
 hw/pci-host/uninorth.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/hw/pci-host/uninorth.c b/hw/pci-host/uninorth.c
index 215b64f..6541b10 100644
--- a/hw/pci-host/uninorth.c
+++ b/hw/pci-host/uninorth.c
@@ -330,6 +330,10 @@ static void unin_agp_pci_host_realize(PCIDevice *d, Error **errp)
     d->config[0x0C] = 0x08; // cache_line_size
     d->config[0x0D] = 0x10; // latency_timer
     //    d->config[0x34] = 0x80; // capabilities_pointer
+    d->config[0x48] = 0x0;
+    d->config[0x49] = 0x0;
+    d->config[0x4a] = 0x0;
+    d->config[0x4b] = 0x1;
 }
 
 static void u3_agp_pci_host_realize(PCIDevice *d, Error **errp)
-- 
1.7.5.4

Re: [Qemu-devel] [PATCH] hw/pci-host/uninorth.c: Add support for Apple's PCI bridge register 0x48

[Mark Cave-Ayland]

On 22/01/16 16:09, Programmingkid wrote:

> Apple has custom PCI bridge registers that are not a part of any known standard. This patch implements register 0x48. With this patch the AppleMacRiscPCI kernel extension no longer prints these error messages for the mac99 target:
> AppleMacRiscPCI: bad range 2(80000000:01000000)
> AppleMacRiscPCI: bad range 2(81000000:00001000)
> AppleMacRiscPCI: bad range 2(81080000:00080000)
> 
> Signed-off-by: John Arbuckle <programmingkidx@gmail.com>
> 
> ---
>  hw/pci-host/uninorth.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/pci-host/uninorth.c b/hw/pci-host/uninorth.c
> index 215b64f..6541b10 100644
> --- a/hw/pci-host/uninorth.c
> +++ b/hw/pci-host/uninorth.c
> @@ -330,6 +330,10 @@ static void unin_agp_pci_host_realize(PCIDevice *d, Error **errp)
>      d->config[0x0C] = 0x08; // cache_line_size
>      d->config[0x0D] = 0x10; // latency_timer
>      //    d->config[0x34] = 0x80; // capabilities_pointer
> +    d->config[0x48] = 0x0;
> +    d->config[0x49] = 0x0;
> +    d->config[0x4a] = 0x0;
> +    d->config[0x4b] = 0x1;
>  }
>  
>  static void u3_agp_pci_host_realize(PCIDevice *d, Error **errp)

Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

As this config space register is seemingly an Apple custom option (or at
least I can't find a mention of it in the PCI-PCI bridge spec) I think
this should have a comment explaining exactly what it does, and should
reference both AppleMacRiscPCI.cpp filename and the enum for the
register value (0x48 == kMacRISCPCIAddressSelect).

I'd also like to see a note explaining that this sets up the register to
match the PCI memory region base/size currently used in QEMU/OpenBIOS
too in order to provide a hint that if one changes, so must the other.

BTW is the register required for any of the other uni-north realize
functions? Alex?


ATB,

Mark.

Re: [Qemu-devel] [PATCH] hw/pci-host/uninorth.c: Add support for Apple's PCI bridge register 0x48

[Programmingkid]

On Jan 22, 2016, at 11:46 AM, Mark Cave-Ayland wrote:

> On 22/01/16 16:09, Programmingkid wrote:
> 
>> Apple has custom PCI bridge registers that are not a part of any known standard. This patch implements register 0x48. With this patch the AppleMacRiscPCI kernel extension no longer prints these error messages for the mac99 target:
>> AppleMacRiscPCI: bad range 2(80000000:01000000)
>> AppleMacRiscPCI: bad range 2(81000000:00001000)
>> AppleMacRiscPCI: bad range 2(81080000:00080000)
>> 
>> Signed-off-by: John Arbuckle <programmingkidx@gmail.com>
>> 
>> ---
>> hw/pci-host/uninorth.c |    4 ++++
>> 1 files changed, 4 insertions(+), 0 deletions(-)
>> 
>> diff --git a/hw/pci-host/uninorth.c b/hw/pci-host/uninorth.c
>> index 215b64f..6541b10 100644
>> --- a/hw/pci-host/uninorth.c
>> +++ b/hw/pci-host/uninorth.c
>> @@ -330,6 +330,10 @@ static void unin_agp_pci_host_realize(PCIDevice *d, Error **errp)
>>     d->config[0x0C] = 0x08; // cache_line_size
>>     d->config[0x0D] = 0x10; // latency_timer
>>     //    d->config[0x34] = 0x80; // capabilities_pointer
>> +    d->config[0x48] = 0x0;
>> +    d->config[0x49] = 0x0;
>> +    d->config[0x4a] = 0x0;
>> +    d->config[0x4b] = 0x1;
>> }
>> 
>> static void u3_agp_pci_host_realize(PCIDevice *d, Error **errp)
> 
> Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> 
> As this config space register is seemingly an Apple custom option (or at
> least I can't find a mention of it in the PCI-PCI bridge spec) I think
> this should have a comment explaining exactly what it does, and should
> reference both AppleMacRiscPCI.cpp filename and the enum for the
> register value (0x48 == kMacRISCPCIAddressSelect).

Is this what you want:

Apple has custom PCI bridge registers that are not a part of any known standard. This patch implements register 0x48. With this patch the AppleMacRiscPCI kernel extension no longer prints these error messages for the mac99 target:
AppleMacRiscPCI: bad range 2(80000000:01000000)
AppleMacRiscPCI: bad range 2(81000000:00001000)
AppleMacRiscPCI: bad range 2(81080000:00080000)

In Apple's AppleMacRiscPCI.h source code, the register is defined as kMacRISCPCIAddressSelect. It is accessed in the AppleMacRiscPCI.cpp file. What this register is used for is determining the address a pci bridge range that is kept track of by the operating system. 

> I'd also like to see a note explaining that this sets up the register to
> match the PCI memory region base/size currently used in QEMU/OpenBIOS
> too in order to provide a hint that if one changes, so must the other.

Note: OpenBIOS in the arch/ppc/qemu/init.c file has a structure with an index of [ARCH_MAC99]. It keeps track of the PCI MMIO range for the mac99 target. If a change happens to either this file or the AppleMacRiscPCI kernel extension, the other would have to be changed as well.

> 
> BTW is the register required for any of the other uni-north realize
> functions? Alex?


My guess is no. Only the AppleMacRiscPCI kernel extension needs to know those details.

Since this is only a change to the patch's comment, do I still need to use v2 in the "[PATCH]" text?

Re: [Qemu-devel] [PATCH] hw/pci-host/uninorth.c: Add support for Apple's PCI bridge register 0x48

[Mark Cave-Ayland]

On 22/01/16 18:26, Programmingkid wrote:

> On Jan 22, 2016, at 11:46 AM, Mark Cave-Ayland wrote:
> 
>> On 22/01/16 16:09, Programmingkid wrote:
>>
>>> Apple has custom PCI bridge registers that are not a part of any known standard. This patch implements register 0x48. With this patch the AppleMacRiscPCI kernel extension no longer prints these error messages for the mac99 target:
>>> AppleMacRiscPCI: bad range 2(80000000:01000000)
>>> AppleMacRiscPCI: bad range 2(81000000:00001000)
>>> AppleMacRiscPCI: bad range 2(81080000:00080000)
>>>
>>> Signed-off-by: John Arbuckle <programmingkidx@gmail.com>
>>>
>>> ---
>>> hw/pci-host/uninorth.c |    4 ++++
>>> 1 files changed, 4 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/hw/pci-host/uninorth.c b/hw/pci-host/uninorth.c
>>> index 215b64f..6541b10 100644
>>> --- a/hw/pci-host/uninorth.c
>>> +++ b/hw/pci-host/uninorth.c
>>> @@ -330,6 +330,10 @@ static void unin_agp_pci_host_realize(PCIDevice *d, Error **errp)
>>>     d->config[0x0C] = 0x08; // cache_line_size
>>>     d->config[0x0D] = 0x10; // latency_timer
>>>     //    d->config[0x34] = 0x80; // capabilities_pointer
>>> +    d->config[0x48] = 0x0;
>>> +    d->config[0x49] = 0x0;
>>> +    d->config[0x4a] = 0x0;
>>> +    d->config[0x4b] = 0x1;
>>> }
>>>
>>> static void u3_agp_pci_host_realize(PCIDevice *d, Error **errp)
>>
>> Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>
>> As this config space register is seemingly an Apple custom option (or at
>> least I can't find a mention of it in the PCI-PCI bridge spec) I think
>> this should have a comment explaining exactly what it does, and should
>> reference both AppleMacRiscPCI.cpp filename and the enum for the
>> register value (0x48 == kMacRISCPCIAddressSelect).
> 
> Is this what you want:
> 
> Apple has custom PCI bridge registers that are not a part of any known standard. This patch implements register 0x48. With this patch the AppleMacRiscPCI kernel extension no longer prints these error messages for the mac99 target:
> AppleMacRiscPCI: bad range 2(80000000:01000000)
> AppleMacRiscPCI: bad range 2(81000000:00001000)
> AppleMacRiscPCI: bad range 2(81080000:00080000)
> 
> In Apple's AppleMacRiscPCI.h source code, the register is defined as kMacRISCPCIAddressSelect. It is accessed in the AppleMacRiscPCI.cpp file. What this register is used for is determining the address a pci bridge range that is kept track of by the operating system. 
> 
>> I'd also like to see a note explaining that this sets up the register to
>> match the PCI memory region base/size currently used in QEMU/OpenBIOS
>> too in order to provide a hint that if one changes, so must the other.
> 
> Note: OpenBIOS in the arch/ppc/qemu/init.c file has a structure with an index of [ARCH_MAC99]. It keeps track of the PCI MMIO range for the mac99 target. If a change happens to either this file or the AppleMacRiscPCI kernel extension, the other would have to be changed as well.

It's mostly down to Alex/David (so please wait for some initial
feedback) but I'd prefer to see something along these lines:


Subject: uninorth.c: add support for UniNorth kMacRISCPCIAddressSelect
(0x48) register

Darwin/OS X use the undocumented kMacRISCPCIAddressSelect (0x48) to
configure PCI memory space size for mac99 machines. Without this
register, warnings similar to below are emitted to the console during boot:

AppleMacRiscPCI: bad range 2(80000000:01000000)
AppleMacRiscPCI: bad range 2(81000000:00001000)
AppleMacRiscPCI: bad range 2(81080000:00080000)

Based upon the algorithm in Darwin's AppleMacRiscPCI.cpp driver, set the
kMacRISCPCIAddressSelect register so that Darwin considers the PCI
memory space to be at 0x80000000 (size 0x10000000) which matches that
currently used by QEMU and OpenBIOS.


Similarly I think a 2-line comment should be added in the actual code
itself e.g.

/* Set kMacRISCPCIAddressSelect (0x48) register to indicate PCI memory
space with base 0x80000000, size 0x10000000 for Apple's AppleMacRiscPCI
driver */

>> BTW is the register required for any of the other uni-north realize
>> functions? Alex?
> 
> 
> My guess is no. Only the AppleMacRiscPCI kernel extension needs to know those details.

I was thinking more about AGP and non-AGP uninorth bridges, but there is
definitely some overlap as you can see that the AppleMacRiscPCI driver
is also used to configure AGP.

> Since this is only a change to the patch's comment, do I still need to use v2 in the "[PATCH]" text?

Yeah, it's best to do this regardless of the changes so that it's clear
to the maintainers which version of the patch to use.


ATB,

Mark.

Re: [Qemu-devel] [PATCH] hw/pci-host/uninorth.c: Add support for Apple's PCI bridge register 0x48

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 5598 bytes --]

On Fri, Jan 22, 2016 at 07:13:28PM +0000, Mark Cave-Ayland wrote:
> On 22/01/16 18:26, Programmingkid wrote:
> 
> > On Jan 22, 2016, at 11:46 AM, Mark Cave-Ayland wrote:
> > 
> >> On 22/01/16 16:09, Programmingkid wrote:
> >>
> >>> Apple has custom PCI bridge registers that are not a part of any known standard. This patch implements register 0x48. With this patch the AppleMacRiscPCI kernel extension no longer prints these error messages for the mac99 target:
> >>> AppleMacRiscPCI: bad range 2(80000000:01000000)
> >>> AppleMacRiscPCI: bad range 2(81000000:00001000)
> >>> AppleMacRiscPCI: bad range 2(81080000:00080000)
> >>>
> >>> Signed-off-by: John Arbuckle <programmingkidx@gmail.com>
> >>>
> >>> ---
> >>> hw/pci-host/uninorth.c |    4 ++++
> >>> 1 files changed, 4 insertions(+), 0 deletions(-)
> >>>
> >>> diff --git a/hw/pci-host/uninorth.c b/hw/pci-host/uninorth.c
> >>> index 215b64f..6541b10 100644
> >>> --- a/hw/pci-host/uninorth.c
> >>> +++ b/hw/pci-host/uninorth.c
> >>> @@ -330,6 +330,10 @@ static void unin_agp_pci_host_realize(PCIDevice *d, Error **errp)
> >>>     d->config[0x0C] = 0x08; // cache_line_size
> >>>     d->config[0x0D] = 0x10; // latency_timer
> >>>     //    d->config[0x34] = 0x80; // capabilities_pointer
> >>> +    d->config[0x48] = 0x0;
> >>> +    d->config[0x49] = 0x0;
> >>> +    d->config[0x4a] = 0x0;
> >>> +    d->config[0x4b] = 0x1;
> >>> }
> >>>
> >>> static void u3_agp_pci_host_realize(PCIDevice *d, Error **errp)
> >>
> >> Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> >>
> >> As this config space register is seemingly an Apple custom option (or at
> >> least I can't find a mention of it in the PCI-PCI bridge spec) I think
> >> this should have a comment explaining exactly what it does, and should
> >> reference both AppleMacRiscPCI.cpp filename and the enum for the
> >> register value (0x48 == kMacRISCPCIAddressSelect).
> > 
> > Is this what you want:
> > 
> > Apple has custom PCI bridge registers that are not a part of any known standard. This patch implements register 0x48. With this patch the AppleMacRiscPCI kernel extension no longer prints these error messages for the mac99 target:
> > AppleMacRiscPCI: bad range 2(80000000:01000000)
> > AppleMacRiscPCI: bad range 2(81000000:00001000)
> > AppleMacRiscPCI: bad range 2(81080000:00080000)
> > 
> > In Apple's AppleMacRiscPCI.h source code, the register is defined as kMacRISCPCIAddressSelect. It is accessed in the AppleMacRiscPCI.cpp file. What this register is used for is determining the address a pci bridge range that is kept track of by the operating system. 
> > 
> >> I'd also like to see a note explaining that this sets up the register to
> >> match the PCI memory region base/size currently used in QEMU/OpenBIOS
> >> too in order to provide a hint that if one changes, so must the other.
> > 
> > Note: OpenBIOS in the arch/ppc/qemu/init.c file has a structure with an index of [ARCH_MAC99]. It keeps track of the PCI MMIO range for the mac99 target. If a change happens to either this file or the AppleMacRiscPCI kernel extension, the other would have to be changed as well.
> 
> It's mostly down to Alex/David (so please wait for some initial
> feedback) but I'd prefer to see something along these lines:
> 
> 
> Subject: uninorth.c: add support for UniNorth kMacRISCPCIAddressSelect
> (0x48) register
> 
> Darwin/OS X use the undocumented kMacRISCPCIAddressSelect (0x48) to
> configure PCI memory space size for mac99 machines. Without this
> register, warnings similar to below are emitted to the console during boot:
> 
> AppleMacRiscPCI: bad range 2(80000000:01000000)
> AppleMacRiscPCI: bad range 2(81000000:00001000)
> AppleMacRiscPCI: bad range 2(81080000:00080000)
> 
> Based upon the algorithm in Darwin's AppleMacRiscPCI.cpp driver, set the
> kMacRISCPCIAddressSelect register so that Darwin considers the PCI
> memory space to be at 0x80000000 (size 0x10000000) which matches that
> currently used by QEMU and OpenBIOS.

That's much better - without the context mentioning Darwin / OS X, the
filename isn't much use.

> Similarly I think a 2-line comment should be added in the actual code
> itself e.g.
> 
> /* Set kMacRISCPCIAddressSelect (0x48) register to indicate PCI memory
> space with base 0x80000000, size 0x10000000 for Apple's AppleMacRiscPCI
> driver */

I've applied the patch to ppc-for-2.6, but I've revised the commit
message and comment as suggested by Mark.

John, in future do remember that the people reviewing patches probably
won't be working in the same sub-area as you: the commit message needs
to provide enough context for them to understand why the patch is
desirable.


> >> BTW is the register required for any of the other uni-north realize
> >> functions? Alex?
> > 
> > 
> > My guess is no. Only the AppleMacRiscPCI kernel extension needs to know those details.
> 
> I was thinking more about AGP and non-AGP uninorth bridges, but there is
> definitely some overlap as you can see that the AppleMacRiscPCI driver
> is also used to configure AGP.
> 
> > Since this is only a change to the patch's comment, do I still need to use v2 in the "[PATCH]" text?
> 
> Yeah, it's best to do this regardless of the changes so that it's clear
> to the maintainers which version of the patch to use.

Agreed.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]