Re: [Qemu-ppc] [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 <address@hidden>
>>>
>>> ---
>>> 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 <address@hidden>
>>
>> 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.