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 <[email protected]> >>> >>> --- >>> 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 <[email protected]> >> >> 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.
