qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [Qemu-ppc] [PATCH 16/19] openpic: add Shared MSI suppor


From: Scott Wood
Subject: Re: [Qemu-devel] [Qemu-ppc] [PATCH 16/19] openpic: add Shared MSI support
Date: Tue, 11 Dec 2012 19:42:28 -0600

On 12/11/2012 06:53:44 PM, Alexander Graf wrote:

On 11.12.2012, at 18:35, Scott Wood wrote:

> On 12/11/2012 02:10:14 AM, Alexander Graf wrote:
>> On 11.12.2012, at 01:36, Scott Wood <address@hidden> wrote:
>> > On 12/08/2012 07:44:39 AM, Alexander Graf wrote:
>> >> The OpenPIC allows MSI access through shared MSI registers. Implement
>> >> them for the MPC8544 MPIC, so we can support MSIs.
>> >> Signed-off-by: Alexander Graf <address@hidden>
>> >> ---
>> >> hw/openpic.c | 150 ++++++++++++++++++++++++++++++++++++++++++++++++++--------
>> >> 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 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 different implementations without implementing them. >> If we want to add support for p4080 MPICs later, we add a new model to the emulation and make the nr of msi banks a parameter, like the patch set does for all the other raven/mpc8544 differences. That way we don't get into the current mess of a halfway accurate emulation unless we really want it.
>
> So because the old code made a mess of it, we're saying "abstraction is bad" in general?

No, because the old code messed up abstraction, I would like to move to a non-abstract level and then start abstracting at the correct layer. What that correct layer is is still up for discussion :).

> All I'm saying is this should be done with a runtime data structure (of which there could be more than one) rather than #defines.

Yeah, and my argument was that this should be introduced as part of a new model. But if it makes you happy I can of course also make it generic right now, without a user, which means we can't even test whether the generalization works, which means we might screw it up again ;).

It would have a user. It just wouldn't have multiple users, or a user that instantiates multiple banks. This feels like writing a kernel driver that can only handle one instance of a device, just because that's all your SoC happens to have at the moment. Nobody objects to a driver having its state in a struct, or the driver maintaining a list of said structs, just because you don't have hardware to test the case where there are multiple instances.

-Scott



reply via email to

[Prev in Thread] Current Thread [Next in Thread]