diff mbox

INTC issue

Message ID aec7e5c30902120409y1d215eacu2e529c3f6fe169bb@mail.gmail.com (mailing list archive)
State Superseded
Headers show

Commit Message

Magnus Damm Feb. 12, 2009, 12:09 p.m. UTC
Hi Shimoda-san!

Sorry about the delay. I need much time to think about intc issues. =)

On Tue, Feb 10, 2009 at 7:28 PM, Yoshihiro Shimoda
<shimoda.yoshihiro@renesas.com> wrote:
> I sent the patch.
> http://marc.info/?l=linux-sh&m=123417952130537&w=2
> But, I did not write down the explanation of this patch...
> I implemented the "maps" to this patch. The "enabled_bit" is it.
> Would you check this patch?

Thanks for the patch. I think I understand your problem, but I'm not
sure if your patch is the best and simplest solution. I'm afraid that
your patch adds quite a bit of memory usage overhead and on top of
that the intc code becomes even more complex. I'd like to keep the
code simple and with low memory foot print if possible. =)

How about the attached patch? It should fix the issue on sh7785 with
only a few lines of added code.

Note that with this patch you now should use only one linux interrupt
for the special hardware blocks that come with multiple vectors but
only a single masking method. Look at the DMAC0/DMAC1/SCIF0/SCIF1 intc
table changes for more information. The interrupt calculated from the
first INTC_VECT in vectors[] is used to for all vectors with the same
enum_id.

Please let me know if the patch solves your issue. I'll break out the
patch and fix up other processors and post a proper patch set in a
little while if this change solves your problem.

Cheers,

/ magnus

Comments

Yoshihiro Shimoda Feb. 17, 2009, 6:21 a.m. UTC | #1
Hi Magnus-san!

I am very sorry for late reply.

Magnus Damm wrote:
- snip -
> How about the attached patch? It should fix the issue on sh7785 with
> only a few lines of added code.

Thank you for your comment and patch.

> Note that with this patch you now should use only one linux interrupt
> for the special hardware blocks that come with multiple vectors but
> only a single masking method. Look at the DMAC0/DMAC1/SCIF0/SCIF1 intc
> table changes for more information. The interrupt calculated from the
> first INTC_VECT in vectors[] is used to for all vectors with the same
> enum_id.
> 
> Please let me know if the patch solves your issue. I'll break out the
> patch and fix up other processors and post a proper patch set in a
> little while if this change solves your problem.

When I used this patch and change DMAC driver, my issue was solved!
But I think that I might as well change DMAC only. For example, when SCIF
driver get same interrupt number, SCIF driver use sci_mpxed_interrupt().
Because this function check interrupt flag, overheads increase just a little.

The DMAC driver changes:
 - fix register maps for SH7785. :)
 - change DMAC irq number.
 - add IRQF_SHARED flag of request_irq().
 - add dummy args of request_irq() in sh_dmac_init().

Thanks,
Yoshihiro Shimoda

--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Magnus Damm Feb. 24, 2009, 2:12 p.m. UTC | #2
Hi Shimoda-san,

On Tue, Feb 17, 2009 at 3:21 PM, Yoshihiro Shimoda
<shimoda.yoshihiro@renesas.com> wrote:
> Magnus Damm wrote:
>> Please let me know if the patch solves your issue. I'll break out the
>> patch and fix up other processors and post a proper patch set in a
>> little while if this change solves your problem.
>
> When I used this patch and change DMAC driver, my issue was solved!

Excellent!

> But I think that I might as well change DMAC only. For example, when SCIF
> driver get same interrupt number, SCIF driver use sci_mpxed_interrupt().
> Because this function check interrupt flag, overheads increase just a little.

I agree that the overhead is increased a bit, but 100% working
interrupt masking is more important than performance. This only
affects some processor types though, so hopefully future processors
allow us to mask interrupts in a more fine-grained fashion. For
instance, SuperH Mobile devices such as sh7722 and sh7723 does not
require any fixes like this. =)

Also, regarding performance, out cache footprint becomes slightly
smaller with a single interrupt for multiple vectors. That may
compensate a bit for the added overhead.

> The DMAC driver changes:
>  - fix register maps for SH7785. :)
>  - change DMAC irq number.
>  - add IRQF_SHARED flag of request_irq().
>  - add dummy args of request_irq() in sh_dmac_init().

Cool. If you want to use a single interrupt but keep the overhead to a
minimum I recommend a change similar to my recent sh-rtc patch. It is
using a single shared interrupt handler - like the
sci_mpxed_interrupt(). I think such a strategy may be more efficient
than using multiple shared interrupt handlers.

Thanks for your help!

/ magnus
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Yoshihiro Shimoda Feb. 25, 2009, 4:11 a.m. UTC | #3
Hi Magnus-san,

Magnus Damm wrote:
> I agree that the overhead is increased a bit, but 100% working
> interrupt masking is more important than performance. This only
> affects some processor types though, so hopefully future processors
> allow us to mask interrupts in a more fine-grained fashion. For
> instance, SuperH Mobile devices such as sh7722 and sh7723 does not
> require any fixes like this. =)

I think so too. :)

> Also, regarding performance, out cache footprint becomes slightly
> smaller with a single interrupt for multiple vectors. That may
> compensate a bit for the added overhead.

I see! I did not think of cache.

> Cool. If you want to use a single interrupt but keep the overhead to a
> minimum I recommend a change similar to my recent sh-rtc patch. It is
> using a single shared interrupt handler - like the
> sci_mpxed_interrupt(). I think such a strategy may be more efficient
> than using multiple shared interrupt handlers.

I refer to your sh-rtc patch and I will modify DMAC driver.

Thank you very much for your help!

Thanks,
Yoshihiro Shimoda

--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

--- 0001/arch/sh/kernel/cpu/sh4a/setup-sh7785.c
+++ work/arch/sh/kernel/cpu/sh4a/setup-sh7785.c	2009-02-12 20:45:36.000000000 +0900
@@ -20,18 +20,13 @@  static struct plat_sci_port sci_platform
 		.mapbase	= 0xffea0000,
 		.flags		= UPF_BOOT_AUTOCONF,
 		.type		= PORT_SCIF,
-		.irqs		= { 40, 41, 43, 42 },
+		.irqs		= { 40, 40, 40, 40 },
 	}, {
 		.mapbase	= 0xffeb0000,
 		.flags		= UPF_BOOT_AUTOCONF,
 		.type		= PORT_SCIF,
-		.irqs		= { 44, 45, 47, 46 },
-	},
-
-	/*
-	 * The rest of these all have multiplexed IRQs
-	 */
-	{
+		.irqs		= { 44, 44, 44, 44 },
+	}, {
 		.mapbase	= 0xffec0000,
 		.flags		= UPF_BOOT_AUTOCONF,
 		.type		= PORT_SCIF,
@@ -91,33 +86,19 @@  enum {
 	IRL4_HHLL, IRL4_HHLH, IRL4_HHHL,
 
 	IRQ0, IRQ1, IRQ2, IRQ3, IRQ4, IRQ5, IRQ6, IRQ7,
-	WDT,
-	TMU0, TMU1, TMU2, TMU2_TICPI,
-	HUDI,
-	DMAC0_DMINT0, DMAC0_DMINT1, DMAC0_DMINT2, DMAC0_DMINT3,
-	DMAC0_DMINT4, DMAC0_DMINT5, DMAC0_DMAE,
-	SCIF0_ERI, SCIF0_RXI, SCIF0_BRI, SCIF0_TXI,
-	SCIF1_ERI, SCIF1_RXI, SCIF1_BRI, SCIF1_TXI,
-	DMAC1_DMINT6, DMAC1_DMINT7, DMAC1_DMINT8, DMAC1_DMINT9,
-	DMAC1_DMINT10, DMAC1_DMINT11, DMAC1_DMAE,
-	HSPI,
+	WDT, TMU0, TMU1, TMU2, TMU2_TICPI,
+	HUDI, DMAC0, SCIF0, SCIF1, DMAC1, HSPI,
 	SCIF2, SCIF3, SCIF4, SCIF5,
-	PCISERR, PCIINTA, PCIINTB, PCIINTC, PCIINTD,
-	PCIERR, PCIPWD3, PCIPWD2, PCIPWD1, PCIPWD0,
-	SIOF,
-	MMCIF_FSTAT, MMCIF_TRAN, MMCIF_ERR, MMCIF_FRDY,
-	DU,
-	GDTA_GACLI, GDTA_GAMCI, GDTA_GAERI,
+	PCISERR, PCIINTA, PCIINTB, PCIINTC, PCIINTD, PCIC5,
+	SIOF, MMCIF, DU, GDTA,
 	TMU3, TMU4, TMU5,
 	SSI0, SSI1,
 	HAC0, HAC1,
-	FLCTL_FLSTE, FLCTL_FLEND, FLCTL_FLTRQ0, FLCTL_FLTRQ1,
-	GPIOI0, GPIOI1, GPIOI2, GPIOI3,
+	FLCTL, GPIO,
 
 	/* interrupt groups */
 
-	TMU012, DMAC0, SCIF0, SCIF1, DMAC1,
-	PCIC5, MMCIF, GDTA, TMU345, FLCTL, GPIO
+	TMU012,	TMU345
 };
 
 static struct intc_vect vectors[] __initdata = {
@@ -125,57 +106,45 @@  static struct intc_vect vectors[] __init
 	INTC_VECT(TMU0, 0x580), INTC_VECT(TMU1, 0x5a0),
 	INTC_VECT(TMU2, 0x5c0), INTC_VECT(TMU2_TICPI, 0x5e0),
 	INTC_VECT(HUDI, 0x600),
-	INTC_VECT(DMAC0_DMINT0, 0x620), INTC_VECT(DMAC0_DMINT1, 0x640),
-	INTC_VECT(DMAC0_DMINT2, 0x660), INTC_VECT(DMAC0_DMINT3, 0x680),
-	INTC_VECT(DMAC0_DMINT4, 0x6a0), INTC_VECT(DMAC0_DMINT5, 0x6c0),
-	INTC_VECT(DMAC0_DMAE, 0x6e0),
-	INTC_VECT(SCIF0_ERI, 0x700), INTC_VECT(SCIF0_RXI, 0x720),
-	INTC_VECT(SCIF0_BRI, 0x740), INTC_VECT(SCIF0_TXI, 0x760),
-	INTC_VECT(SCIF1_ERI, 0x780), INTC_VECT(SCIF1_RXI, 0x7a0),
-	INTC_VECT(SCIF1_BRI, 0x7c0), INTC_VECT(SCIF1_TXI, 0x7e0),
-	INTC_VECT(DMAC1_DMINT6, 0x880), INTC_VECT(DMAC1_DMINT7, 0x8a0),
-	INTC_VECT(DMAC1_DMINT8, 0x8c0), INTC_VECT(DMAC1_DMINT9, 0x8e0),
-	INTC_VECT(DMAC1_DMINT10, 0x900), INTC_VECT(DMAC1_DMINT11, 0x920),
-	INTC_VECT(DMAC1_DMAE, 0x940),
+	INTC_VECT(DMAC0, 0x620), INTC_VECT(DMAC0, 0x640),
+	INTC_VECT(DMAC0, 0x660), INTC_VECT(DMAC0, 0x680),
+	INTC_VECT(DMAC0, 0x6a0), INTC_VECT(DMAC0, 0x6c0),
+	INTC_VECT(DMAC0, 0x6e0),
+	INTC_VECT(SCIF0, 0x700), INTC_VECT(SCIF0, 0x720),
+	INTC_VECT(SCIF0, 0x740), INTC_VECT(SCIF0, 0x760),
+	INTC_VECT(SCIF1, 0x780), INTC_VECT(SCIF1, 0x7a0),
+	INTC_VECT(SCIF1, 0x7c0), INTC_VECT(SCIF1, 0x7e0),
+	INTC_VECT(DMAC1, 0x880), INTC_VECT(DMAC1, 0x8a0),
+	INTC_VECT(DMAC1, 0x8c0), INTC_VECT(DMAC1, 0x8e0),
+	INTC_VECT(DMAC1, 0x900), INTC_VECT(DMAC1, 0x920),
+	INTC_VECT(DMAC1, 0x940),
 	INTC_VECT(HSPI, 0x960),
 	INTC_VECT(SCIF2, 0x980), INTC_VECT(SCIF3, 0x9a0),
 	INTC_VECT(SCIF4, 0x9c0), INTC_VECT(SCIF5, 0x9e0),
 	INTC_VECT(PCISERR, 0xa00), INTC_VECT(PCIINTA, 0xa20),
 	INTC_VECT(PCIINTB, 0xa40), INTC_VECT(PCIINTC, 0xa60),
-	INTC_VECT(PCIINTD, 0xa80), INTC_VECT(PCIERR, 0xaa0),
-	INTC_VECT(PCIPWD3, 0xac0), INTC_VECT(PCIPWD2, 0xae0),
-	INTC_VECT(PCIPWD1, 0xb00), INTC_VECT(PCIPWD0, 0xb20),
+	INTC_VECT(PCIINTD, 0xa80), INTC_VECT(PCIC5, 0xaa0),
+	INTC_VECT(PCIC5, 0xac0), INTC_VECT(PCIC5, 0xae0),
+	INTC_VECT(PCIC5, 0xb00), INTC_VECT(PCIC5, 0xb20),
 	INTC_VECT(SIOF, 0xc00),
-	INTC_VECT(MMCIF_FSTAT, 0xd00), INTC_VECT(MMCIF_TRAN, 0xd20),
-	INTC_VECT(MMCIF_ERR, 0xd40), INTC_VECT(MMCIF_FRDY, 0xd60),
+	INTC_VECT(MMCIF, 0xd00), INTC_VECT(MMCIF, 0xd20),
+	INTC_VECT(MMCIF, 0xd40), INTC_VECT(MMCIF, 0xd60),
 	INTC_VECT(DU, 0xd80),
-	INTC_VECT(GDTA_GACLI, 0xda0), INTC_VECT(GDTA_GAMCI, 0xdc0),
-	INTC_VECT(GDTA_GAERI, 0xde0),
+	INTC_VECT(GDTA, 0xda0), INTC_VECT(GDTA, 0xdc0),
+	INTC_VECT(GDTA, 0xde0),
 	INTC_VECT(TMU3, 0xe00), INTC_VECT(TMU4, 0xe20),
 	INTC_VECT(TMU5, 0xe40),
 	INTC_VECT(SSI0, 0xe80), INTC_VECT(SSI1, 0xea0),
 	INTC_VECT(HAC0, 0xec0), INTC_VECT(HAC1, 0xee0),
-	INTC_VECT(FLCTL_FLSTE, 0xf00), INTC_VECT(FLCTL_FLEND, 0xf20),
-	INTC_VECT(FLCTL_FLTRQ0, 0xf40), INTC_VECT(FLCTL_FLTRQ1, 0xf60),
-	INTC_VECT(GPIOI0, 0xf80), INTC_VECT(GPIOI1, 0xfa0),
-	INTC_VECT(GPIOI2, 0xfc0), INTC_VECT(GPIOI3, 0xfe0),
+	INTC_VECT(FLCTL, 0xf00), INTC_VECT(FLCTL, 0xf20),
+	INTC_VECT(FLCTL, 0xf40), INTC_VECT(FLCTL, 0xf60),
+	INTC_VECT(GPIO, 0xf80), INTC_VECT(GPIO, 0xfa0),
+	INTC_VECT(GPIO, 0xfc0), INTC_VECT(GPIO, 0xfe0),
 };
 
 static struct intc_group groups[] __initdata = {
 	INTC_GROUP(TMU012, TMU0, TMU1, TMU2, TMU2_TICPI),
-	INTC_GROUP(DMAC0, DMAC0_DMINT0, DMAC0_DMINT1, DMAC0_DMINT2,
-		   DMAC0_DMINT3, DMAC0_DMINT4, DMAC0_DMINT5, DMAC0_DMAE),
-	INTC_GROUP(SCIF0, SCIF0_ERI, SCIF0_RXI, SCIF0_BRI, SCIF0_TXI),
-	INTC_GROUP(SCIF1, SCIF1_ERI, SCIF1_RXI, SCIF1_BRI, SCIF1_TXI),
-	INTC_GROUP(DMAC1, DMAC1_DMINT6, DMAC1_DMINT7, DMAC1_DMINT8,
-		   DMAC1_DMINT9, DMAC1_DMINT10, DMAC1_DMINT11, DMAC1_DMAE),
-	INTC_GROUP(PCIC5, PCIERR, PCIPWD3, PCIPWD2, PCIPWD1, PCIPWD0),
-	INTC_GROUP(MMCIF, MMCIF_FSTAT, MMCIF_TRAN, MMCIF_ERR, MMCIF_FRDY),
-	INTC_GROUP(GDTA, GDTA_GACLI, GDTA_GAMCI, GDTA_GAERI),
 	INTC_GROUP(TMU345, TMU3, TMU4, TMU5),
-	INTC_GROUP(FLCTL, FLCTL_FLSTE, FLCTL_FLEND,
-		   FLCTL_FLTRQ0, FLCTL_FLTRQ1),
-	INTC_GROUP(GPIO, GPIOI0, GPIOI1, GPIOI2, GPIOI3),
 };
 
 static struct intc_mask_reg mask_registers[] __initdata = {
--- 0001/arch/sh/kernel/irq.c
+++ work/arch/sh/kernel/irq.c	2009-02-12 20:25:38.000000000 +0900
@@ -106,7 +106,7 @@  asmlinkage int do_IRQ(unsigned int irq, 
 	}
 #endif
 
-	irq = irq_demux(evt2irq(irq));
+	irq = irq_demux(intc_evt2irq(irq));
 
 #ifdef CONFIG_IRQSTACKS
 	curctx = (union irq_ctx *)current_thread_info();
--- 0001/drivers/sh/intc.c
+++ work/drivers/sh/intc.c	2009-02-12 20:50:03.000000000 +0900
@@ -568,6 +568,10 @@  static void __init intc_register_irq(str
 	if (!data[0] && data[1])
 		primary = 1;
 
+	if (!data[0] && !data[1])
+		pr_warning("intc: missing unique irq mask for 0x%04x\n",
+			   irq2evt(irq));
+
 	data[0] = data[0] ? data[0] : intc_mask_data(desc, d, enum_id, 1);
 	data[1] = data[1] ? data[1] : intc_prio_data(desc, d, enum_id, 1);
 
@@ -641,6 +645,17 @@  static unsigned int __init save_reg(stru
 	return 0;
 }
 
+static unsigned char *intc_evt2irq_table;
+
+unsigned int intc_evt2irq(unsigned int vector)
+{
+	unsigned int irq = evt2irq(vector);
+
+	if (intc_evt2irq_table && intc_evt2irq_table[irq])
+		irq = intc_evt2irq_table[irq];
+
+	return irq;
+}
 
 void __init register_intc_controller(struct intc_desc *desc)
 {
@@ -705,8 +720,40 @@  void __init register_intc_controller(str
 
 	BUG_ON(k > 256); /* _INTC_ADDR_E() and _INTC_ADDR_D() are 8 bits */
 
+	/* keep the first vector only if same enum is used multiple times */
 	for (i = 0; i < desc->nr_vectors; i++) {
 		struct intc_vect *vect = desc->vectors + i;
+		int first_irq = evt2irq(vect->vect);
+
+		if (!vect->enum_id)
+			continue;
+
+		for (k = i + 1; k < desc->nr_vectors; k++) {
+			struct intc_vect *vect2 = desc->vectors + k;
+
+			if (vect->enum_id != vect2->enum_id)
+				continue;
+
+			vect2->enum_id = 0;
+
+			if (!intc_evt2irq_table)
+				intc_evt2irq_table = alloc_bootmem(NR_IRQS);
+
+			if (!intc_evt2irq_table) {
+				pr_warning("intc: cannot allocate evt2irq!\n");
+				continue;
+			}
+
+			intc_evt2irq_table[evt2irq(vect2->vect)] = first_irq;
+		}
+	}
+
+	/* register the vectors one by one */
+	for (i = 0; i < desc->nr_vectors; i++) {
+		struct intc_vect *vect = desc->vectors + i;
+
+		if (!vect->enum_id)
+			continue;
 
 		intc_register_irq(desc, d, vect->enum_id, evt2irq(vect->vect));
 	}
--- 0001/include/linux/sh_intc.h
+++ work/include/linux/sh_intc.h	2009-02-12 20:34:43.000000000 +0900
@@ -85,6 +85,7 @@  struct intc_desc symbol __initdata = {		
 }
 #endif
 
+unsigned int intc_evt2irq(unsigned int vector);
 void __init register_intc_controller(struct intc_desc *desc);
 int intc_set_priority(unsigned int irq, unsigned int prio);