Message ID | 96510826-2FD7-4967-9BEC-746DB44A81F8@gmail.com |
---|---|
State | New |
Headers | show |
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.
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?
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.
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.
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)
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(-)