Skip to contentSkip to conversation

INTC issue

DISCUSSION Yoshihiro Shimoda 2 participants 7 messages Feb 5, 2009 – Feb 25, 2009 SuperH

lore
Share & tools
lore

Yoshihiro Shimoda

author
#0 lore raw

Hi,

I found INTC issue. For example, an interrupt is prohibited when I handle it as follows.

1) Driver A call request_irq of DMAC ch0
2) Driver B call request_irq of DMAC ch1.
3) Driver A call free_irq of DMAC ch0.
4) Driver B use DMAC ch1, but DMAC ch1 interrupt disabled.

The interrupt mask register of SH7785 is INT2MSKR.
The bit 11 of INT2MSKR is DMAC channels 0 to 5 interrupts mask bit.

I think that it is necessary to count the number of times admitted an interrupt to solve this problem. I think that intc have a counter every interrupt bit for a simple implementation method, but there is much waste.

Any ideas?

Thanks,
Yoshihiro Shimoda

#1 lore raw

Hi Shimoda-san,

Thanks for bringing up this issue and thanks for the patch!

Quoted 12 lines · Yoshihiro Shimoda

Hi,

I found INTC issue. For example, an interrupt is prohibited when I handle it as follows.

1) Driver A call request_irq of DMAC ch0
2) Driver B call request_irq of DMAC ch1.
3) Driver A call free_irq of DMAC ch0.
4) Driver B use DMAC ch1, but DMAC ch1 interrupt disabled.

The interrupt mask register of SH7785 is INT2MSKR.
The bit 11 of INT2MSKR is DMAC channels 0 to 5 interrupts mask bit.

Quoted 6 lines

I think that it is necessary to count the number of times admitted an interrupt to solve this problem. I think that intc have a counter every interrupt bit for a simple implementation method, but there is much waste.

Any ideas?

Hm, counter may work half-ok, but I think we end up with broken logic anyhow. Counters would result in that enabling and disabling of interrupts sometimes result in enable/disable depending on the count value. For interrupt sources belonging to a single driver we can probably live with that, but it may become very strange if we share enable bit between multiple drivers. A driver most likely expects that interrupt disable really disables interrupts. =)

I would prefer to map all vectors that share a enable/disable bit to a single interrupt number instead. This is somewhat similar to interrupt sharing. The common irq code handles interrupt count for us. Look at desc->depth inside linux/kernel/irq/*.

As an example, right now in the sh7785 dma case you have 14 vectors divided into 7 DMAC0 sources and 7 DMAC1 sources. A total of 14 dma interrupts in linux. I would prefer to map them to 2 linux interrupts instead, one for DMAC0 and one for DMAC1. So all DMAC0 vectors map to the DMAC0 linux interrupt and same for DMAC1. Each linux interrupt can be enabled and disabled as a regular interrupt source.

Would that work for you? I hope you can determine interrupt source by checking status bits in the dma hardware block. Or maybe you can't and need separate interrupt handlers?

Cheers,

/ magnus

author
#2 lore raw
In reply to Yoshihiro Shimoda

Hi Magnus-san,

Thank you very much for your comment!

Quoted 4 lines · Magnus Damm

Hi Shimoda-san,

Thanks for bringing up this issue and thanks for the patch!

- snip -

Quoted 21 lines

Hm, counter may work half-ok, but I think we end up with broken logic anyhow. Counters would result in that enabling and disabling of interrupts sometimes result in enable/disable depending on the count value. For interrupt sources belonging to a single driver we can probably live with that, but it may become very strange if we share enable bit between multiple drivers. A driver most likely expects that interrupt disable really disables interrupts. =)

I would prefer to map all vectors that share a enable/disable bit to a single interrupt number instead. This is somewhat similar to interrupt sharing. The common irq code handles interrupt count for us. Look at desc->depth inside linux/kernel/irq/*.

As an example, right now in the sh7785 dma case you have 14 vectors divided into 7 DMAC0 sources and 7 DMAC1 sources. A total of 14 dma interrupts in linux. I would prefer to map them to 2 linux interrupts instead, one for DMAC0 and one for DMAC1. So all DMAC0 vectors map to the DMAC0 linux interrupt and same for DMAC1. Each linux interrupt can be enabled and disabled as a regular interrupt source.

Quoted 3 lines

Would that work for you? I hope you can determine interrupt source by checking status bits in the dma hardware block. Or maybe you can't and need separate interrupt handlers?

I sent the patch.
http://marc.info/?l=linux-sh&m3417952130537&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,
Yoshihiro Shimoda

#3 lore raw
In reply to Yoshihiro Shimoda

Hi Shimoda-san!

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

Quoted 5 lines · Yoshihiro Shimoda

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

arch/sh/kernel/cpu/sh4a/setup-sh7785.c +32 -63
@@ -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 = {
arch/sh/kernel/irq.c +1 -1
@@ -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();
drivers/sh/intc.c +47 -0

67 lines not on this page · raw message

include/linux/sh_intc.h +1 -0
@@ -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);
author
#4 lore raw
In reply to Yoshihiro Shimoda

Hi Magnus-san!

I am very sorry for late reply.

Magnus Damm wrote:
- snip -

Quoted 2 lines

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.

Quoted 10 lines

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

#5 lore raw
In reply to Yoshihiro Shimoda

Hi Shimoda-san,

Quoted 6 lines · Yoshihiro Shimoda
Quoted 3 lines · Magnus Damm

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!

Quoted 3 lines

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.

Quoted 5 lines

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

author
#6 lore raw
In reply to Yoshihiro Shimoda

Hi Magnus-san,

Quoted 6 lines · Magnus Damm

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. :)

Quoted 3 lines

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.

Quoted 5 lines

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