From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:38629) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TibLc-00010k-T0 for qemu-devel@nongnu.org; Tue, 11 Dec 2012 20:43:05 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TibLY-0003o4-Pd for qemu-devel@nongnu.org; Tue, 11 Dec 2012 20:42:56 -0500 Date: Tue, 11 Dec 2012 19:42:28 -0600 From: Scott Wood In-Reply-To: (from agraf@suse.de on Tue Dec 11 18:53:44 2012) Message-ID: <1355276548.16025.6@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 16/19] openpic: add Shared MSI support 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/11/2012 06:53:44 PM, Alexander Graf wrote: >=20 > On 11.12.2012, at 18:35, Scott Wood wrote: >=20 > > On 12/11/2012 02:10:14 AM, Alexander Graf wrote: > >> On 11.12.2012, at 01:36, Scott Wood =20 > wrote: > >> > On 12/08/2012 07:44:39 AM, Alexander Graf wrote: > >> >> The OpenPIC allows MSI access through shared MSI registers. =20 > Implement > >> >> them for the MPC8544 MPIC, so we can support MSIs. > >> >> Signed-off-by: Alexander Graf > >> >> --- > >> >> hw/openpic.c | 150 =20 > ++++++++++++++++++++++++++++++++++++++++++++++++++-------- > >> >> 1 files changed, 130 insertions(+), 20 deletions(-) > >> >> diff --git a/hw/openpic.c b/hw/openpic.c > >> >> index f2f152f..f71d668 100644 > >> >> --- a/hw/openpic.c > >> >> +++ b/hw/openpic.c > >> >> @@ -38,6 +38,7 @@ > >> >> #include "pci.h" > >> >> #include "openpic.h" > >> >> #include "sysbus.h" > >> >> +#include "msi.h" > >> >> //#define DEBUG_OPENPIC > >> >> @@ -52,6 +53,7 @@ > >> >> #define MAX_TMR 4 > >> >> #define VECTOR_BITS 8 > >> >> #define MAX_IPI 4 > >> >> +#define MAX_MSI 8 > >> >> #define VID 0x03 /* MPIC version ID */ > >> >> /* OpenPIC capability flags */ > >> >> @@ -62,6 +64,8 @@ > >> >> #define OPENPIC_GLB_REG_SIZE 0x10F0 > >> >> #define OPENPIC_TMR_REG_START 0x10F0 > >> >> #define OPENPIC_TMR_REG_SIZE 0x220 > >> >> +#define OPENPIC_MSI_REG_START 0x1600 > >> >> +#define OPENPIC_MSI_REG_SIZE 0x200 > >> >> #define OPENPIC_SRC_REG_START 0x10000 > >> >> #define OPENPIC_SRC_REG_SIZE (MAX_IRQ * 0x20) > >> >> #define OPENPIC_CPU_REG_START 0x20000 > >> >> @@ -126,6 +130,12 @@ > >> >> #define IDR_P1_SHIFT 1 > >> >> #define IDR_P0_SHIFT 0 > >> >> +#define MSIIR_OFFSET 0x140 > >> >> +#define MSIIR_SRS_SHIFT 29 > >> >> +#define MSIIR_SRS_MASK (0x7 << MSIIR_SRS_SHIFT) > >> >> +#define MSIIR_IBS_SHIFT 24 > >> >> +#define MSIIR_IBS_MASK (0x1f << MSIIR_IBS_SHIFT) > >> > > >> > FWIW, if you want to model newer MPICs such as on p4080, they =20 > have multiple banks of MSIs, so you may not want to hardcode one bank. > >> The OpenPIC code was suffering a lot from attempts to generalize =20 > different implementations without implementing them. > >> If we want to add support for p4080 MPICs later, we add a new =20 > model to the emulation and make the nr of msi banks a parameter, like =20 > the patch set does for all the other raven/mpc8544 differences. That =20 > way we don't get into the current mess of a halfway accurate =20 > emulation unless we really want it. > > > > So because the old code made a mess of it, we're saying =20 > "abstraction is bad" in general? >=20 > No, because the old code messed up abstraction, I would like to move =20 > to a non-abstract level and then start abstracting at the correct =20 > layer. What that correct layer is is still up for discussion :). >=20 > > All I'm saying is this should be done with a runtime data structure =20 > (of which there could be more than one) rather than #defines. >=20 > Yeah, and my argument was that this should be introduced as part of a =20 > new model. But if it makes you happy I can of course also make it =20 > generic right now, without a user, which means we can't even test =20 > whether the generalization works, which means we might screw it up =20 > again ;). It would have a user. It just wouldn't have multiple users, or a user =20 that instantiates multiple banks. This feels like writing a kernel =20 driver that can only handle one instance of a device, just because =20 that's all your SoC happens to have at the moment. Nobody objects to a =20 driver having its state in a struct, or the driver maintaining a list =20 of said structs, just because you don't have hardware to test the case =20 where there are multiple instances. -Scott=