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

[David Gibson <david@gibson.dropbear.id.au>]

[-- 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 --]