From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39221) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aMh9Y-0001R4-6x for qemu-devel@nongnu.org; Fri, 22 Jan 2016 14:13:49 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aMh9W-0005HL-W4 for qemu-devel@nongnu.org; Fri, 22 Jan 2016 14:13:48 -0500 References: <96510826-2FD7-4967-9BEC-746DB44A81F8@gmail.com> <56A25CD0.4060709@ilande.co.uk> <0E0356B0-F2CB-4EBE-B433-ECE86466F173@gmail.com> From: Mark Cave-Ayland Message-ID: <56A27F58.5080105@ilande.co.uk> Date: Fri, 22 Jan 2016 19:13:28 +0000 MIME-Version: 1.0 In-Reply-To: <0E0356B0-F2CB-4EBE-B433-ECE86466F173@gmail.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] hw/pci-host/uninorth.c: Add support for Apple's PCI bridge register 0x48 List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Programmingkid Cc: david@gibson.dropbear.id.au, "qemu-ppc@nongnu.org list:PowerPC" , qemu-devel qemu-devel , Alexander Graf 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 >>> >>> --- >>> 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 >> >> 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.