| Message ID | 1371194159-17332-2-git-send-email-Minghuan.Lian@freescale.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On 06/14/2013 02:15:56 AM, Minghuan Lian wrote: > @@ -421,10 +440,29 @@ static int fsl_of_msi_probe(struct > platform_device *dev) > } > msi->msiir_offset = > features->msiir_offset + (res.start & 0xfffff); > + > + /* > + * First read the MSIIR/MSIIR1 offset from dts > + * If failure use the hardcode MSIIR offset "On failure" > + */ > + if (of_address_to_resource(dev->dev.of_node, 1, &msiir)) > + msi->msiir_offset = features->msiir_offset + > + (res.start & > MSIIR_OFFSET_MASK); > + else > + msi->msiir_offset = msiir.start & > MSIIR_OFFSET_MASK; > } > > msi->feature = features->fsl_pic_ip; > > + if (of_device_is_compatible(dev->dev.of_node, > "fsl,mpic-msi-v4.3")) { > + msi->srs_shift = MSIIR1_SRS_SHIFT; > + msi->ibs_shift = MSIIR1_IBS_SHIFT; > + > + } else { > + msi->srs_shift = MSIIR_SRS_SHIFT; > + msi->ibs_shift = MSIIR_IBS_SHIFT; > + } Remove the blank line just before the "} else {". > diff --git a/arch/powerpc/sysdev/fsl_msi.h > b/arch/powerpc/sysdev/fsl_msi.h > index 8225f86..43a9d99 100644 > --- a/arch/powerpc/sysdev/fsl_msi.h > +++ b/arch/powerpc/sysdev/fsl_msi.h > @@ -16,7 +16,7 @@ > #include <linux/of.h> > #include <asm/msi_bitmap.h> > > -#define NR_MSI_REG 8 > +#define NR_MSI_REG 16 > #define IRQS_PER_MSI_REG 32 > #define NR_MSI_IRQS (NR_MSI_REG * IRQS_PER_MSI_REG) I don't see where you update all_avail in fsl_of_msi_probe. We should also be bounds-checking the contents of msi-available-ranges. Currently it looks like we just silently overrun the bitmap if we get bad input from the device tree. -Scott
Hi Scott, please see my comments inline. On 06/15/2013 06:09 AM, Scott Wood wrote: > On 06/14/2013 02:15:56 AM, Minghuan Lian wrote: >> @@ -421,10 +440,29 @@ static int fsl_of_msi_probe(struct >> platform_device *dev) >> } >> msi->msiir_offset = >> features->msiir_offset + (res.start & 0xfffff); >> + >> + /* >> + * First read the MSIIR/MSIIR1 offset from dts >> + * If failure use the hardcode MSIIR offset > > "On failure" > [Minghuan] OK, thanks. >> + */ >> + if (of_address_to_resource(dev->dev.of_node, 1, &msiir)) >> + msi->msiir_offset = features->msiir_offset + >> + (res.start & MSIIR_OFFSET_MASK); >> + else >> + msi->msiir_offset = msiir.start & MSIIR_OFFSET_MASK; >> } >> >> msi->feature = features->fsl_pic_ip; >> >> + if (of_device_is_compatible(dev->dev.of_node, >> "fsl,mpic-msi-v4.3")) { >> + msi->srs_shift = MSIIR1_SRS_SHIFT; >> + msi->ibs_shift = MSIIR1_IBS_SHIFT; >> + >> + } else { >> + msi->srs_shift = MSIIR_SRS_SHIFT; >> + msi->ibs_shift = MSIIR_IBS_SHIFT; >> + } > > Remove the blank line just before the "} else {". > [Minghuan] OK, Thanks. >> diff --git a/arch/powerpc/sysdev/fsl_msi.h >> b/arch/powerpc/sysdev/fsl_msi.h >> index 8225f86..43a9d99 100644 >> --- a/arch/powerpc/sysdev/fsl_msi.h >> +++ b/arch/powerpc/sysdev/fsl_msi.h >> @@ -16,7 +16,7 @@ >> #include <linux/of.h> >> #include <asm/msi_bitmap.h> >> >> -#define NR_MSI_REG 8 >> +#define NR_MSI_REG 16 >> #define IRQS_PER_MSI_REG 32 >> #define NR_MSI_IRQS (NR_MSI_REG * IRQS_PER_MSI_REG) > > I don't see where you update all_avail in fsl_of_msi_probe. > > We should also be bounds-checking the contents of msi-available-ranges. > Currently it looks like we just silently overrun the bitmap if we get bad > input from the device tree. > [Minghuan] all_avail definition: static const u32 all_avail[] = { 0, NR_MSI_IRQS }; When changing NR_MSI_REG to 16, NR_MSI_IRQS has been changed to 16*32, and all_avail also is updated. Before calling fsl_msi_setup_hwirq(), the code has checked 'msi-available-ranges', only the interrupts lied in 'msi-available-ranges' will be initialized by call fsl_msi_setup_hwirq() , and the corresponding bitmap will be freed. I moved msi_bitmap_free_hwirqs() to fsl_msi_setup_hwirq(), because the code would generate different bitmap when using MSIIR or MSIIR1. > -Scott
On 06/16/2013 10:00:01 PM, Lian Minghuan-b31939 wrote: > Hi Scott, > > please see my comments inline. > > On 06/15/2013 06:09 AM, Scott Wood wrote: >> On 06/14/2013 02:15:56 AM, Minghuan Lian wrote: >>> diff --git a/arch/powerpc/sysdev/fsl_msi.h >>> b/arch/powerpc/sysdev/fsl_msi.h >>> index 8225f86..43a9d99 100644 >>> --- a/arch/powerpc/sysdev/fsl_msi.h >>> +++ b/arch/powerpc/sysdev/fsl_msi.h >>> @@ -16,7 +16,7 @@ >>> #include <linux/of.h> >>> #include <asm/msi_bitmap.h> >>> >>> -#define NR_MSI_REG 8 >>> +#define NR_MSI_REG 16 >>> #define IRQS_PER_MSI_REG 32 >>> #define NR_MSI_IRQS (NR_MSI_REG * IRQS_PER_MSI_REG) >> >> I don't see where you update all_avail in fsl_of_msi_probe. >> >> We should also be bounds-checking the contents of >> msi-available-ranges. >> Currently it looks like we just silently overrun the bitmap if we >> get bad >> input from the device tree. >> > [Minghuan] all_avail definition: static const u32 all_avail[] = { 0, > NR_MSI_IRQS }; > When changing NR_MSI_REG to 16, NR_MSI_IRQS has been changed to > 16*32, and all_avail also is updated. That's my point. It shouldn't change for older hardware. > Before calling fsl_msi_setup_hwirq(), the code has checked > 'msi-available-ranges', only the interrupts lied in > 'msi-available-ranges' will be initialized by call > fsl_msi_setup_hwirq() , and the corresponding bitmap will be freed. I > moved msi_bitmap_free_hwirqs() to fsl_msi_setup_hwirq(), because the > code would generate different bitmap when using MSIIR or MSIIR1. And what happens if msi-available-ranges is bad, and refers to non-existent MSIs past the end of the bitmap? -Scott
Hi Soctt, please see my comments inline. On 06/18/2013 08:15 AM, Scott Wood wrote: > On 06/16/2013 10:00:01 PM, Lian Minghuan-b31939 wrote: >> Hi Scott, >> >> please see my comments inline. >> >> On 06/15/2013 06:09 AM, Scott Wood wrote: >>> On 06/14/2013 02:15:56 AM, Minghuan Lian wrote: >>>> diff --git a/arch/powerpc/sysdev/fsl_msi.h >>>> b/arch/powerpc/sysdev/fsl_msi.h >>>> index 8225f86..43a9d99 100644 >>>> --- a/arch/powerpc/sysdev/fsl_msi.h >>>> +++ b/arch/powerpc/sysdev/fsl_msi.h >>>> @@ -16,7 +16,7 @@ >>>> #include <linux/of.h> >>>> #include <asm/msi_bitmap.h> >>>> >>>> -#define NR_MSI_REG 8 >>>> +#define NR_MSI_REG 16 >>>> #define IRQS_PER_MSI_REG 32 >>>> #define NR_MSI_IRQS (NR_MSI_REG * IRQS_PER_MSI_REG) >>> >>> I don't see where you update all_avail in fsl_of_msi_probe. >>> >>> We should also be bounds-checking the contents of msi-available-ranges. >>> Currently it looks like we just silently overrun the bitmap if we >>> get bad >>> input from the device tree. >>> >> [Minghuan] all_avail definition: static const u32 all_avail[] = { 0, >> NR_MSI_IRQS }; >> When changing NR_MSI_REG to 16, NR_MSI_IRQS has been changed to >> 16*32, and all_avail also is updated. > > That's my point. It shouldn't change for older hardware. [Minghaun] the older hardware has 8 registers, mipcv4.3 has 16 registers. If we do not use 16*32 bitmap to indicate 8*32 irqs.(this way just only wastes some memory and has no other harm) we have two choice I think. 1. Use a variable assigned value 8 or 16 based on compatible, then dynamically create bitmap 2. Add a new file for mpic v4.3. What do you think? > >> Before calling fsl_msi_setup_hwirq(), the code has checked >> 'msi-available-ranges', only the interrupts lied in >> 'msi-available-ranges' will be initialized by call >> fsl_msi_setup_hwirq() , and the corresponding bitmap will be freed. I >> moved msi_bitmap_free_hwirqs() to fsl_msi_setup_hwirq(), because the >> code would generate different bitmap when using MSIIR or MSIIR1. > > And what happens if msi-available-ranges is bad, and refers to > non-existent MSIs past the end of the bitmap? [Minghuan] If msi-available-ranges is bad, the below code will get error. virt_msir = irq_of_parse_and_map(dev->dev.of_node, irq_index); And the related error will be printed out and fsl_msi_setup_hwirq() will return error directly. There is no chance to set non-existent MSIs past the end of the bitmap. > > -Scott
diff --git a/arch/powerpc/sysdev/fsl_msi.c b/arch/powerpc/sysdev/fsl_msi.c index ab02db3..34510b7 100644 --- a/arch/powerpc/sysdev/fsl_msi.c +++ b/arch/powerpc/sysdev/fsl_msi.c @@ -28,6 +28,18 @@ #include "fsl_msi.h" #include "fsl_pci.h" +#define MSIIR_OFFSET_MASK 0xfffff +#define MSIIR_IBS_SHIFT 0 +#define MSIIR_SRS_SHIFT 5 +#define MSIIR1_IBS_SHIFT 4 +#define MSIIR1_SRS_SHIFT 0 +#define MSI_SRS_MASK 0xf +#define MSI_IBS_MASK 0x1f + +#define msi_hwirq(msi, msir_index, intr_index) \ + ((msir_index) << (msi)->srs_shift | \ + ((intr_index) << (msi)->ibs_shift)) + static LIST_HEAD(msi_head); struct fsl_msi_feature { @@ -80,18 +92,19 @@ static const struct irq_domain_ops fsl_msi_host_ops = { static int fsl_msi_init_allocator(struct fsl_msi *msi_data) { - int rc; + int rc, hwirq; rc = msi_bitmap_alloc(&msi_data->bitmap, NR_MSI_IRQS, msi_data->irqhost->of_node); if (rc) return rc; - rc = msi_bitmap_reserve_dt_hwirqs(&msi_data->bitmap); - if (rc < 0) { - msi_bitmap_free(&msi_data->bitmap); - return rc; - } + /* + * Reserve all the hwirqs + * The available hwirqs will be released in fsl_msi_setup_hwirq() + */ + for (hwirq = 0; hwirq < NR_MSI_IRQS; hwirq++) + msi_bitmap_reserve_hwirq(&msi_data->bitmap, hwirq); return 0; } @@ -144,8 +157,9 @@ static void fsl_compose_msi_msg(struct pci_dev *pdev, int hwirq, msg->data = hwirq; - pr_debug("%s: allocated srs: %d, ibs: %d\n", - __func__, hwirq / IRQS_PER_MSI_REG, hwirq % IRQS_PER_MSI_REG); + pr_debug("%s: allocated srs: %d, ibs: %d\n", __func__, + (hwirq >> msi_data->srs_shift) & MSI_SRS_MASK, + (hwirq >> msi_data->ibs_shift) & MSI_IBS_MASK); } static int fsl_setup_msi_irqs(struct pci_dev *pdev, int nvec, int type) @@ -285,8 +299,8 @@ static void fsl_msi_cascade(unsigned int irq, struct irq_desc *desc) intr_index = ffs(msir_value) - 1; cascade_irq = irq_linear_revmap(msi_data->irqhost, - msir_index * IRQS_PER_MSI_REG + - intr_index + have_shift); + msi_hwirq(msi_data, msir_index, + intr_index + have_shift)); if (cascade_irq != NO_IRQ) generic_handle_irq(cascade_irq); have_shift += intr_index + 1; @@ -339,7 +353,7 @@ static int fsl_msi_setup_hwirq(struct fsl_msi *msi, struct platform_device *dev, int offset, int irq_index) { struct fsl_msi_cascade_data *cascade_data = NULL; - int virt_msir; + int virt_msir, i; virt_msir = irq_of_parse_and_map(dev->dev.of_node, irq_index); if (virt_msir == NO_IRQ) { @@ -360,6 +374,11 @@ static int fsl_msi_setup_hwirq(struct fsl_msi *msi, struct platform_device *dev, irq_set_handler_data(virt_msir, cascade_data); irq_set_chained_handler(virt_msir, fsl_msi_cascade); + /* Release the hwirqs corresponding to this MSI register */ + for (i = 0; i < IRQS_PER_MSI_REG; i++) + msi_bitmap_free_hwirqs(&msi->bitmap, + msi_hwirq(msi, offset, i), 1); + return 0; } @@ -368,7 +387,7 @@ static int fsl_of_msi_probe(struct platform_device *dev) { const struct of_device_id *match; struct fsl_msi *msi; - struct resource res; + struct resource res, msiir; int err, i, j, irq_index, count; int rc; const u32 *p; @@ -421,10 +440,29 @@ static int fsl_of_msi_probe(struct platform_device *dev) } msi->msiir_offset = features->msiir_offset + (res.start & 0xfffff); + + /* + * First read the MSIIR/MSIIR1 offset from dts + * If failure use the hardcode MSIIR offset + */ + if (of_address_to_resource(dev->dev.of_node, 1, &msiir)) + msi->msiir_offset = features->msiir_offset + + (res.start & MSIIR_OFFSET_MASK); + else + msi->msiir_offset = msiir.start & MSIIR_OFFSET_MASK; } msi->feature = features->fsl_pic_ip; + if (of_device_is_compatible(dev->dev.of_node, "fsl,mpic-msi-v4.3")) { + msi->srs_shift = MSIIR1_SRS_SHIFT; + msi->ibs_shift = MSIIR1_IBS_SHIFT; + + } else { + msi->srs_shift = MSIIR_SRS_SHIFT; + msi->ibs_shift = MSIIR_IBS_SHIFT; + } + /* * Remember the phandle, so that we can match with any PCI nodes * that have an "fsl,msi" property. diff --git a/arch/powerpc/sysdev/fsl_msi.h b/arch/powerpc/sysdev/fsl_msi.h index 8225f86..43a9d99 100644 --- a/arch/powerpc/sysdev/fsl_msi.h +++ b/arch/powerpc/sysdev/fsl_msi.h @@ -16,7 +16,7 @@ #include <linux/of.h> #include <asm/msi_bitmap.h> -#define NR_MSI_REG 8 +#define NR_MSI_REG 16 #define IRQS_PER_MSI_REG 32 #define NR_MSI_IRQS (NR_MSI_REG * IRQS_PER_MSI_REG) @@ -31,6 +31,8 @@ struct fsl_msi { unsigned long cascade_irq; u32 msiir_offset; /* Offset of MSIIR, relative to start of CCSR */ + u32 ibs_shift; /* Shift of interrupt bit select */ + u32 srs_shift; /* Shift of the shared interrupt register select */ void __iomem *msi_regs; u32 feature; int msi_virqs[NR_MSI_REG];
MPIC controller v4.3 provides MSIIR1 to index 16 MSI registers. MSIIR can only index 8 MSI registers. MSIIR1 uses different bits definition than MSIIR. This patch adds ibs_shift and srs_shift to indicate the bits definition of the MSIIR and MSIIR1, so the same code can handle the MSIIR and MSIIR1 simultaneously. Signed-off-by: Minghuan Lian <Minghuan.Lian@freescale.com> --- arch/powerpc/sysdev/fsl_msi.c | 62 ++++++++++++++++++++++++++++++++++--------- arch/powerpc/sysdev/fsl_msi.h | 4 ++- 2 files changed, 53 insertions(+), 13 deletions(-)