From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33280) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aNUM5-0006QA-PB for qemu-devel@nongnu.org; Sun, 24 Jan 2016 18:46:04 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aNUM2-0000sl-Qd for qemu-devel@nongnu.org; Sun, 24 Jan 2016 18:46:01 -0500 Date: Mon, 25 Jan 2016 10:40:45 +1100 From: David Gibson Message-ID: <20160124234045.GZ27454@voom.redhat.com> References: <96510826-2FD7-4967-9BEC-746DB44A81F8@gmail.com> <56A25CD0.4060709@ilande.co.uk> <0E0356B0-F2CB-4EBE-B433-ECE86466F173@gmail.com> <56A27F58.5080105@ilande.co.uk> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="kUmJ0kIGcKOYePjT" Content-Disposition: inline In-Reply-To: <56A27F58.5080105@ilande.co.uk> 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: Mark Cave-Ayland Cc: Programmingkid , "qemu-ppc@nongnu.org list:PowerPC" , qemu-devel qemu-devel , Alexander Graf --kUmJ0kIGcKOYePjT Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Jan 22, 2016 at 07:13:28PM +0000, Mark Cave-Ayland wrote: > On 22/01/16 18:26, Programmingkid wrote: >=20 > > On Jan 22, 2016, at 11:46 AM, Mark Cave-Ayland wrote: > >=20 > >> On 22/01/16 16:09, Programmingkid wrote: > >> > >>> Apple has custom PCI bridge registers that are not a part of any know= n standard. This patch implements register 0x48. With this patch the AppleM= acRiscPCI kernel extension no longer prints these error messages for the ma= c99 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] =3D 0x08; // cache_line_size > >>> d->config[0x0D] =3D 0x10; // latency_timer > >>> // d->config[0x34] =3D 0x80; // capabilities_pointer > >>> + d->config[0x48] =3D 0x0; > >>> + d->config[0x49] =3D 0x0; > >>> + d->config[0x4a] =3D 0x0; > >>> + d->config[0x4b] =3D 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 =3D=3D kMacRISCPCIAddressSelect). > >=20 > > Is this what you want: > >=20 > > Apple has custom PCI bridge registers that are not a part of any known = standard. This patch implements register 0x48. With this patch the AppleMac= RiscPCI kernel extension no longer prints these error messages for the mac9= 9 target: > > AppleMacRiscPCI: bad range 2(80000000:01000000) > > AppleMacRiscPCI: bad range 2(81000000:00001000) > > AppleMacRiscPCI: bad range 2(81080000:00080000) > >=20 > > In Apple's AppleMacRiscPCI.h source code, the register is defined as kM= acRISCPCIAddressSelect. It is accessed in the AppleMacRiscPCI.cpp file. Wha= t this register is used for is determining the address a pci bridge range t= hat is kept track of by the operating system.=20 > >=20 > >> 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. > >=20 > > 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 kern= el extension, the other would have to be changed as well. >=20 > It's mostly down to Alex/David (so please wait for some initial > feedback) but I'd prefer to see something along these lines: >=20 >=20 > Subject: uninorth.c: add support for UniNorth kMacRISCPCIAddressSelect > (0x48) register >=20 > 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 boo= t: >=20 > AppleMacRiscPCI: bad range 2(80000000:01000000) > AppleMacRiscPCI: bad range 2(81000000:00001000) > AppleMacRiscPCI: bad range 2(81080000:00080000) >=20 > 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. >=20 > /* 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? > >=20 > >=20 > > My guess is no. Only the AppleMacRiscPCI kernel extension needs to know= those details. >=20 > 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. >=20 > > Since this is only a change to the patch's comment, do I still need to = use v2 in the "[PATCH]" text? >=20 > 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. --=20 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 --kUmJ0kIGcKOYePjT Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJWpWD8AAoJEGw4ysog2bOSb+AP/AnhHTrpxOCm2xwu9kbTBcBW NDvWMa3cR55bp7uWAtbvmbJMg//XYA8Ey89gj1F95mgh5SURSNXux/rS+rI1ufwY LdtfgXao3TIZxOvim7QlIUZs67GUik4guT9HlJBAkd3PX2vEK9JZwEUPCMeacYjC pq8Z/BWqwqmGY6y3WQz4NYbKQLs0ig54zKDb7mcnqTiDUCjgmhojxyVw0KAU9BmA HQltZotOeNOiNqamp+VIovczm8+7+4CwrZ/0C2j+viuthFPhWdJ9PagfAsP1T4N5 d6clb4lWKwApvZeqlVJNPHFABlXH7iLGBdXEZ6DzbZNiotP2rRulvawUkTcAAbzF e8eAeFmAXw7N0pUDHCGrcW+yQKL/0gBaJKrdi7+uLP8etfzlh9StSzh8adXW8UM8 BITjz9oQdOCZgIH4srFhvNWExu9DJkFIZ2o6rt2+CkFb0WeclkyU3blvNzCTWF3A S3C8eB6irLwjCtlQJHFDmKAMMRuDOd+dp/A4oVZc07ZwDYjbEiJqZAQTkqAviQFT zqoGY/i57uTc/ECFIcMxvujj5CjXwda7ZZ23DXURRas2xN9dXo8xg+IT1q/b4tCg +O6i5j1P/X3DJupgsGN8RVlFs2gYVU97M5L0DY04zkq+tPB00apIy3M/BmOYzaso ijnW/duYVQU0D00XhL2x =AQtN -----END PGP SIGNATURE----- --kUmJ0kIGcKOYePjT--