On 12.12.2012, at 02:42, Scott Wood wrote: > 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 <scottw...@freescale.com> 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 <ag...@suse.de> >> >> >> --- >> >> >> 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.
Looking at this a bit more, I am getting more and more reluctant to make this dynamic with the current openpic models we have. To get multiple MSI banks cleanly working, we'd need to make a number of fields in the state struct dynamically allocated (msir backing, sub_io_mem). We'd also need to find a way to identify which bank a callback happens at, which the memory api hides from us today. So we again would probably need to allocate some container struct that we can pass as opaque which tells us the region id we're in. And all of the pitfalls above would be something you could only spot using code review, not by actual execution tests, because we would have to hardcode msi_banks to 1 for fsl_mpic_20. So every time we assume cur_msi_bank == 0 the code would "just work". I would really much rather like to postpone that generalization on to the point in time we start implementing more than 1 msi bank. Then we can be sure that the code we're doing generically actually works. Alex