From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:33783) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TiwYC-0003Ek-BA for qemu-devel@nongnu.org; Wed, 12 Dec 2012 19:21:22 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TiwY9-00029u-O4 for qemu-devel@nongnu.org; Wed, 12 Dec 2012 19:21:20 -0500 Date: Wed, 12 Dec 2012 18:20:59 -0600 From: Scott Wood In-Reply-To: (from agraf@suse.de on Wed Dec 12 18:04:11 2012) Message-ID: <1355358059.28445.20@snotra> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; delsp=Yes; format=Flowed Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [Qemu-ppc] [PATCH 3/5] PPC: E500: Generate dt pci irq map dynamically List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Graf Cc: "qemu-ppc@nongnu.org List" , qemu-devel qemu-devel On 12/12/2012 06:04:11 PM, Alexander Graf wrote: >=20 > On 13.12.2012, at 00:43, Scott Wood wrote: >=20 > > On 12/12/2012 05:38:32 PM, Alexander Graf wrote: > >> On 12.12.2012, at 19:40, Scott Wood wrote: > >> > On 12/12/2012 08:09:56 AM, Alexander Graf wrote: > >> >> + for (slot =3D first_slot; slot < last_slot; slot++) { > >> >> + for (pci_irq =3D 0; pci_irq < 4; pci_irq++) { > >> >> + pci_map[i++] =3D cpu_to_be32(slot << 11); > >> >> + pci_map[i++] =3D cpu_to_be32(0x0); > >> >> + pci_map[i++] =3D cpu_to_be32(0x0); > >> >> + pci_map[i++] =3D cpu_to_be32(pci_irq + 1); > >> >> + pci_map[i++] =3D cpu_to_be32(mpic); > >> >> + pci_map[i++] =3D cpu_to_be32(((pci_irq + slot) % 4) =20 > + 1); > >> >> + pci_map[i++] =3D cpu_to_be32(0x1); > >> >> + } > >> >> } > >> > > >> > It would be nice if the slot-to-IRQ calculation were done in =20 > only one place rather than duplicated here. > >> Sure, what exactly would you suggest to do? :) > > > > Have a common function to calculate the IRQ given the slot number, =20 > and call that both from here and from mpc85xx_pci_map_irq(). > > > >> We can move the whole function to ppce500_pci.c. > >> We could export the function(slot, pci_irq) through the header of =20 > ppce500_pci.c. > > > > Either works, though I'd lean towards moving this function into =20 > ppce500_pci.c. >=20 > Well, I'm not sure Anthony would be happy about that. He wanted to =20 > keep device tree generation inside the machine files. Sigh. I don't understand the hostility to device tree generation, to =20 the point of enforcing unnatural code grouping and possibly even =20 duplication. > But this one might be an exception, because it's not a generic device. So what happens when we do have a generic device? Duplicate the code =20 in every machine that uses it, or have a parallel "hw/device_dt.c" (or =20 maybe some better hidden place) to factor out common code while (sort =20 of) complying with Anthony's mandate? :-P > >> We could also try and traverse the pci bus to find the function =20 > that is actually called to convert irq numbers internally, so we =20 > potentially support other pci host controllers. > > > > Not sure what you mean here. >=20 > We could call bus->map_irq(...) with an artificially created =20 > PCIDevice struct ;). But that's pretty hacky. If we do anything like that, it should probably be to iterate over the =20 devices that actually exist and add interrupt-map entries only for =20 those. > So you're indicating you'd like the below patch? I think you pasted a bit more than one patch, but yes. > Do you think it's worth the additional code for a simple + and & 3? It's not about duplicating "+ and & 3" so much as having only one place =20 where the relationship is defined, in case someone wants to alter it =20 (e.g. for adding some other board where the mapping is done =20 differently). > agraf@lychee:/home/agraf/release/qemu> git add hw/ppce500_pci.h > agraf@lychee:/home/agraf/release/qemu> git diff HEAD > agraf@lychee:/home/agraf/release/qemu> git diff HEAD | cat What does piping through cat get you? -Scott=