[PATCH] OMAP: DSS2: Common IRQ handler for all OMAPs

[PATCH] OMAP: DSS2: Common IRQ handler for all OMAPs

[Archit Taneja]

OMAP2 has an irq line dedicated for DISPC interrupts, there is no DSI
on omap2.
OMAP3 has a common irq line for DISPC and DSI interrupts.
OMAP4 has seperate irq lines for DISPC and DSI Interrupts.

Use dss_features to have a common DSS irq handler for all OMAP revisions.

Also, use a member of the global dss structure to store the irq number
as it is used in 2 functions.
 
Signed-off-by: Archit Taneja <archit@ti.com>
---
Note: Applies over a) v10 of OMAP2,3 DSS2 HWMOD b)v3 of DSS2: Generalize clock names
and c) v3 of DSS2: OMAP4 DSS HWMOD :

https://patchwork.kernel.org/patch/500191/
https://patchwork.kernel.org/patch/520191/
https://patchwork.kernel.org/patch/511211/

 drivers/video/omap2/dss/dss.c          |   46 +++++++++++++------------------
 drivers/video/omap2/dss/dss_features.c |    5 ++-
 drivers/video/omap2/dss/dss_features.h |   17 ++++++-----
 3 files changed, 31 insertions(+), 37 deletions(-)

diff --git a/drivers/video/omap2/dss/dss.c b/drivers/video/omap2/dss/dss.c
index c7cdbea..24d6f98 100644
--- a/drivers/video/omap2/dss/dss.c
+++ b/drivers/video/omap2/dss/dss.c
@@ -33,6 +33,7 @@
 #include <plat/display.h>
 #include <plat/clock.h>
 #include "dss.h"
+#include "dss_features.h"
 
 #define DSS_SZ_REGS			SZ_512
 
@@ -61,6 +62,7 @@ static struct {
 	struct platform_device *pdev;
 	void __iomem    *base;
 	int             ctx_id;
+	int		irq;
 
 	struct clk	*dpll4_m4_ck;
 	struct clk	*dss_ick;
@@ -494,28 +496,22 @@ found:
 	return 0;
 }
 
-
-
-static irqreturn_t dss_irq_handler_omap2(int irq, void *arg)
-{
-	dispc_irq_handler();
-
-	return IRQ_HANDLED;
-}
-
-static irqreturn_t dss_irq_handler_omap3(int irq, void *arg)
+static irqreturn_t dss_irq_handler(int irq, void *arg)
 {
-	u32 irqstatus;
+	if (dss_has_feature(FEAT_COMMON_IRQ_DISPC_DSI)) {
+		u32 irqstatus;
 
-	irqstatus = dss_read_reg(DSS_IRQSTATUS);
+		irqstatus = dss_read_reg(DSS_IRQSTATUS);
 
-	if (irqstatus & (1<<0))	/* DISPC_IRQ */
-		dispc_irq_handler();
+		if (irqstatus & (1<<0))	/* DISPC_IRQ */
+			dispc_irq_handler();
 #ifdef CONFIG_OMAP2_DSS_DSI
-	if (irqstatus & (1<<1))	/* DSI_IRQ */
-		dsi_irq_handler();
+		if (irqstatus & (1<<1))	/* DSI_IRQ */
+			dsi_irq_handler();
 #endif
-
+	} else {
+		dispc_irq_handler();
+	}
 	return IRQ_HANDLED;
 }
 
@@ -563,7 +559,7 @@ void dss_set_dac_pwrdn_bgz(bool enable)
 
 static int dss_init(bool skip_init)
 {
-	int r, dss_irq;
+	int r;
 	u32 rev;
 	struct resource *dss_mem;
 
@@ -609,18 +605,14 @@ static int dss_init(bool skip_init)
 	REG_FLD_MOD(DSS_CONTROL, 0, 2, 2);	/* venc clock mode = normal */
 #endif
 
-	dss_irq = platform_get_irq(dss.pdev, 0);
-	if (dss_irq < 0) {
+	dss.irq = platform_get_irq(dss.pdev, 0);
+	if (dss.irq < 0) {
 		DSSERR("omap2 dss: platform_get_irq failed\n");
 		r = -ENODEV;
 		goto fail1;
 	}
 
-	r = request_irq(dss_irq,
-		cpu_is_omap24xx()
-		? dss_irq_handler_omap2
-		: dss_irq_handler_omap3,
-		0, "OMAP DSS", NULL);
+	r = request_irq(dss.irq, dss_irq_handler, 0, "OMAP DSS", NULL);
 
 	if (r < 0) {
 		DSSERR("omap2 dss: request_irq failed\n");
@@ -648,7 +640,7 @@ static int dss_init(bool skip_init)
 	return 0;
 
 fail2:
-	free_irq(dss_irq, NULL);
+	free_irq(dss.irq, NULL);
 fail1:
 	iounmap(dss.base);
 fail0:
@@ -660,7 +652,7 @@ static void dss_exit(void)
 	if (cpu_is_omap34xx())
 		clk_put(dss.dpll4_m4_ck);
 
-	free_irq(INT_24XX_DSS_IRQ, NULL);
+	free_irq(dss.irq, NULL);
 
 	iounmap(dss.base);
 }
diff --git a/drivers/video/omap2/dss/dss_features.c b/drivers/video/omap2/dss/dss_features.c
index cf3ef69..f3ef929 100644
--- a/drivers/video/omap2/dss/dss_features.c
+++ b/drivers/video/omap2/dss/dss_features.c
@@ -157,7 +157,7 @@ static struct omap_dss_features omap3430_dss_features = {
 	.has_feature	=
 		FEAT_GLOBAL_ALPHA | FEAT_LCDENABLEPOL |
 		FEAT_LCDENABLESIGNAL | FEAT_PCKFREEENABLE |
-		FEAT_FUNCGATED,
+		FEAT_FUNCGATED | FEAT_COMMON_IRQ_DISPC_DSI,
 
 	.num_mgrs = 2,
 	.num_ovls = 3,
@@ -172,7 +172,8 @@ static struct omap_dss_features omap3630_dss_features = {
 	.has_feature    =
 		FEAT_GLOBAL_ALPHA | FEAT_LCDENABLEPOL |
 		FEAT_LCDENABLESIGNAL | FEAT_PCKFREEENABLE |
-		FEAT_PRE_MULT_ALPHA | FEAT_FUNCGATED,
+		FEAT_PRE_MULT_ALPHA | FEAT_FUNCGATED |
+		FEAT_COMMON_IRQ_DISPC_DSI,
 
 	.num_mgrs = 2,
 	.num_ovls = 3,
diff --git a/drivers/video/omap2/dss/dss_features.h b/drivers/video/omap2/dss/dss_features.h
index b9c70be..1c93a49 100644
--- a/drivers/video/omap2/dss/dss_features.h
+++ b/drivers/video/omap2/dss/dss_features.h
@@ -25,14 +25,15 @@
 
 /* DSS has feature id */
 enum dss_feat_id {
-	FEAT_GLOBAL_ALPHA	= 1 << 0,
-	FEAT_GLOBAL_ALPHA_VID1	= 1 << 1,
-	FEAT_PRE_MULT_ALPHA	= 1 << 2,
-	FEAT_LCDENABLEPOL	= 1 << 3,
-	FEAT_LCDENABLESIGNAL	= 1 << 4,
-	FEAT_PCKFREEENABLE	= 1 << 5,
-	FEAT_FUNCGATED		= 1 << 6,
-	FEAT_MGR_LCD2		= 1 << 7,
+	FEAT_GLOBAL_ALPHA		= 1 << 0,
+	FEAT_GLOBAL_ALPHA_VID1		= 1 << 1,
+	FEAT_PRE_MULT_ALPHA		= 1 << 2,
+	FEAT_LCDENABLEPOL		= 1 << 3,
+	FEAT_LCDENABLESIGNAL		= 1 << 4,
+	FEAT_PCKFREEENABLE		= 1 << 5,
+	FEAT_FUNCGATED			= 1 << 6,
+	FEAT_MGR_LCD2			= 1 << 7,
+	FEAT_COMMON_IRQ_DISPC_DSI	= 1 << 8,
 };
 
 /* DSS register field id */
-- 
1.7.1

Re: OMAP: DSS2: Common IRQ handler for all OMAPs

[Tomi Valkeinen]

Hi,

On Wed, 2011-02-02 at 08:56 +0000, archit taneja wrote:
> OMAP2 has an irq line dedicated for DISPC interrupts, there is no DSI
> on omap2.
> OMAP3 has a common irq line for DISPC and DSI interrupts.
> OMAP4 has seperate irq lines for DISPC and DSI Interrupts.
> 
> Use dss_features to have a common DSS irq handler for all OMAP revisions.
> 
> Also, use a member of the global dss structure to store the irq number
> as it is used in 2 functions.

It's good to remove the cpu_is_xxxx() calls, but I'm not quite sure
about this patch...

Could we use shared interrupt handlers here, so that dss.c would handle
only DISPC interrupts (or should it be even in dispc.c?) and dsi.c would
handle DSI interrupts?

On OMAP3 both dss.c and dsi.c would register to the same interrupt, and
they would need to check if the interrupt was really for them. On OMAP4
the code could be the same, even though the check is unnecessary.

Also, as I mentioned in the email I sent some minutes ago, this patch
fixes the free_irq call in dss_exit. Things like that should never be
fixed silently, even if it's trivial like in this case.

 Tomi

Re: OMAP: DSS2: Common IRQ handler for all OMAPs

[Felipe Balbi]

Hi,

On Mon, Feb 14, 2011 at 04:21:47PM +0200, Tomi Valkeinen wrote:
> On Wed, 2011-02-02 at 08:56 +0000, archit taneja wrote:
> > OMAP2 has an irq line dedicated for DISPC interrupts, there is no DSI
> > on omap2.
> > OMAP3 has a common irq line for DISPC and DSI interrupts.
> > OMAP4 has seperate irq lines for DISPC and DSI Interrupts.
> > 
> > Use dss_features to have a common DSS irq handler for all OMAP revisions.
> > 
> > Also, use a member of the global dss structure to store the irq number
> > as it is used in 2 functions.
> 
> It's good to remove the cpu_is_xxxx() calls, but I'm not quite sure
> about this patch...
> 
> Could we use shared interrupt handlers here, so that dss.c would handle
> only DISPC interrupts (or should it be even in dispc.c?) and dsi.c would
> handle DSI interrupts?
> 
> On OMAP3 both dss.c and dsi.c would register to the same interrupt, and
> they would need to check if the interrupt was really for them. On OMAP4
> the code could be the same, even though the check is unnecessary.
> 
> Also, as I mentioned in the email I sent some minutes ago, this patch
> fixes the free_irq call in dss_exit. Things like that should never be
> fixed silently, even if it's trivial like in this case.

does it make sense to install an irq_chip for that ? I mean, can you
mask/unmask dss and or dsi IRQs ? If you can, then it might make sense
to take a look into GENIRQ and install an irq_chip for that. Then both
dsi and dss would be able to use standard request_irq() API.

Take a look at drivers/mfd/twl*irq.c and drivers/cbus/retu.c (the last
one on linux-omap only) there are examples of how that should be
implemented. Actually twl*irq.c is wrong, as it bypasses the threaded
IRQ stuff. I have some patches for those, but I'm not sure they are
working correctly yet, needs more testing. My twl4030-irq patches are
available at [1] for reference.

[1] http://gitorious.org/usb/usb/commits/twlirq

-- 
balbi

Re: OMAP: DSS2: Common IRQ handler for all OMAPs

[archit taneja]

Hi,

On Monday 14 February 2011 08:00 PM, Balbi, Felipe wrote:
> Hi,
>
> On Mon, Feb 14, 2011 at 04:21:47PM +0200, Tomi Valkeinen wrote:
>> On Wed, 2011-02-02 at 08:56 +0000, archit taneja wrote:
>>> OMAP2 has an irq line dedicated for DISPC interrupts, there is no DSI
>>> on omap2.
>>> OMAP3 has a common irq line for DISPC and DSI interrupts.
>>> OMAP4 has seperate irq lines for DISPC and DSI Interrupts.
>>>
>>> Use dss_features to have a common DSS irq handler for all OMAP revisions.
>>>
>>> Also, use a member of the global dss structure to store the irq number
>>> as it is used in 2 functions.
>>
>> It's good to remove the cpu_is_xxxx() calls, but I'm not quite sure
>> about this patch...
>>
>> Could we use shared interrupt handlers here, so that dss.c would handle
>> only DISPC interrupts (or should it be even in dispc.c?) and dsi.c would
>> handle DSI interrupts?

Could you elaborate this a bit more?

>>
>> On OMAP3 both dss.c and dsi.c would register to the same interrupt, and
>> they would need to check if the interrupt was really for them. On OMAP4
>> the code could be the same, even though the check is unnecessary.

The code can't be exactly the same. The DSS_IRQSTATUS register used on 
OMAP3 doesn't exist on OMAP4. A read to this register on OMAP4 would 
cause a hang/crash.

>>
>> Also, as I mentioned in the email I sent some minutes ago, this patch
>> fixes the free_irq call in dss_exit. Things like that should never be
>> fixed silently, even if it's trivial like in this case.
>

Will take care of this from now on.

> does it make sense to install an irq_chip for that ? I mean, can you
> mask/unmask dss and or dsi IRQs ? If you can, then it might make sense
> to take a look into GENIRQ and install an irq_chip for that. Then both
> dsi and dss would be able to use standard request_irq() API.
>

We could disable dsi IRQs by masking all the possible interrupt events 
in DSI_IRQSTATUS. The same goes for dispc. Is this what you meant by 
masking/unmasking irqs?

<snip>

Regards,
Archit

Re: OMAP: DSS2: Common IRQ handler for all OMAPs

[Tomi Valkeinen]

On Mon, 2011-02-14 at 22:28 -0600, Taneja, Archit wrote:
> Hi,
> 
> On Monday 14 February 2011 08:00 PM, Balbi, Felipe wrote:
> > Hi,
> >
> > On Mon, Feb 14, 2011 at 04:21:47PM +0200, Tomi Valkeinen wrote:
> >> On Wed, 2011-02-02 at 08:56 +0000, archit taneja wrote:
> >>> OMAP2 has an irq line dedicated for DISPC interrupts, there is no DSI
> >>> on omap2.
> >>> OMAP3 has a common irq line for DISPC and DSI interrupts.
> >>> OMAP4 has seperate irq lines for DISPC and DSI Interrupts.
> >>>
> >>> Use dss_features to have a common DSS irq handler for all OMAP revisions.
> >>>
> >>> Also, use a member of the global dss structure to store the irq number
> >>> as it is used in 2 functions.
> >>
> >> It's good to remove the cpu_is_xxxx() calls, but I'm not quite sure
> >> about this patch...
> >>
> >> Could we use shared interrupt handlers here, so that dss.c would handle
> >> only DISPC interrupts (or should it be even in dispc.c?) and dsi.c would
> >> handle DSI interrupts?
> 
> Could you elaborate this a bit more?

I meant something like this:

dispc.c:

dispc_init()
{
	/* did we have a pdev for dispc? if not, this needs to be dss.pdev */
	request_irq(platform_get_irq(dispc.pdev, 0), irq_handler, IRQF_SHARED, "dispc irq", foo);
}

irq_handler()
{
	if (irq_can_be_shared) {
		check if the irq is for us. exit if not;
	}

	handle;
}

dsi.c:

dsi_init()
{
	request_irq(platform_get_irq(dsi.pdev, 0), irq_handler, IRQF_SHARED, "dsi irq", foo);
}

irq_handler()
{
	if (irq_can_be_shared) {
		check if the irq is for us. exit if not;
	}

	handle;
}


> 
> >>
> >> On OMAP3 both dss.c and dsi.c would register to the same interrupt, and
> >> they would need to check if the interrupt was really for them. On OMAP4
> >> the code could be the same, even though the check is unnecessary.
> 
> The code can't be exactly the same. The DSS_IRQSTATUS register used on 
> OMAP3 doesn't exist on OMAP4. A read to this register on OMAP4 would 
> cause a hang/crash.

Ok, we need a dss_feature bit for this then.

 Tomi

Re: OMAP: DSS2: Common IRQ handler for all OMAPs

[Tomi Valkeinen]

On Mon, 2011-02-14 at 08:30 -0600, Balbi, Felipe wrote:
> Hi,
> 
> On Mon, Feb 14, 2011 at 04:21:47PM +0200, Tomi Valkeinen wrote:
> > On Wed, 2011-02-02 at 08:56 +0000, archit taneja wrote:
> > > OMAP2 has an irq line dedicated for DISPC interrupts, there is no DSI
> > > on omap2.
> > > OMAP3 has a common irq line for DISPC and DSI interrupts.
> > > OMAP4 has seperate irq lines for DISPC and DSI Interrupts.
> > > 
> > > Use dss_features to have a common DSS irq handler for all OMAP revisions.
> > > 
> > > Also, use a member of the global dss structure to store the irq number
> > > as it is used in 2 functions.
> > 
> > It's good to remove the cpu_is_xxxx() calls, but I'm not quite sure
> > about this patch...
> > 
> > Could we use shared interrupt handlers here, so that dss.c would handle
> > only DISPC interrupts (or should it be even in dispc.c?) and dsi.c would
> > handle DSI interrupts?
> > 
> > On OMAP3 both dss.c and dsi.c would register to the same interrupt, and
> > they would need to check if the interrupt was really for them. On OMAP4
> > the code could be the same, even though the check is unnecessary.
> > 
> > Also, as I mentioned in the email I sent some minutes ago, this patch
> > fixes the free_irq call in dss_exit. Things like that should never be
> > fixed silently, even if it's trivial like in this case.
> 
> does it make sense to install an irq_chip for that ? I mean, can you
> mask/unmask dss and or dsi IRQs ? If you can, then it might make sense
> to take a look into GENIRQ and install an irq_chip for that. Then both
> dsi and dss would be able to use standard request_irq() API.
> 
> Take a look at drivers/mfd/twl*irq.c and drivers/cbus/retu.c (the last
> one on linux-omap only) there are examples of how that should be
> implemented. Actually twl*irq.c is wrong, as it bypasses the threaded
> IRQ stuff. I have some patches for those, but I'm not sure they are
> working correctly yet, needs more testing. My twl4030-irq patches are
> available at [1] for reference.

I'm not familiar with genirq/irq_chip. But yes, as Archit said, we can
mask/unmask DSS interrupts. I mean, there's only one interrupt line, but
DSS has irqstatus and irqenable registers that can be used.

If I understood correctly, irq_chip could be used to manage DSS and DSI
interrupts, but I don't know right away what that would mean in
practice, and would it make things easier or not. But this could be
something that needs more study, as there's a custom isr handling system
in DSS, and it would be nice if that could be replaced with genirq.

But I guess the main problem still remains with genirqs also: we have
one interrupt line on omap3, used by two separate modules. And one irq
on omap2, used by one module, and two irqs on omap4, used by two
modules.

 Tomi

Re: OMAP: DSS2: Common IRQ handler for all OMAPs

[archit taneja]

Hi,


On Tuesday 15 February 2011 01:15 PM, Valkeinen, Tomi wrote:
<snip>

> I'm not familiar with genirq/irq_chip. But yes, as Archit said, we can
> mask/unmask DSS interrupts. I mean, there's only one interrupt line, but
> DSS has irqstatus and irqenable registers that can be used.
>
> If I understood correctly, irq_chip could be used to manage DSS and DSI
> interrupts, but I don't know right away what that would mean in
> practice, and would it make things easier or not. But this could be
> something that needs more study, as there's a custom isr handling system
> in DSS, and it would be nice if that could be replaced with genirq.
>
> But I guess the main problem still remains with genirqs also: we have
> one interrupt line on omap3, used by two separate modules. And one irq
> on omap2, used by one module, and two irqs on omap4, used by two
> modules.
>
>   Tomi

We actually have 4 interrupt lines on OMAP4, 1 for dispc, 2 for dsi1 and 
dsi2 and 1 for hdmi.

Regards,
Archit

Re: OMAP: DSS2: Common IRQ handler for all OMAPs

[Felipe Balbi]

Hi,

On Tue, Feb 15, 2011 at 09:58:24AM +0530, archit taneja wrote:
> >does it make sense to install an irq_chip for that ? I mean, can you
> >mask/unmask dss and or dsi IRQs ? If you can, then it might make sense
> >to take a look into GENIRQ and install an irq_chip for that. Then both
> >dsi and dss would be able to use standard request_irq() API.
> >
> 
> We could disable dsi IRQs by masking all the possible interrupt
> events in DSI_IRQSTATUS. The same goes for dispc. Is this what you
> meant by masking/unmasking irqs?

yes it is. Then it makes sense to have an irq_chip for those two irqs, I
think.

/proc/interrupt will reflect how the hardware works (DSI and DISPC IRQs
being handled by DSS), both dsi and dispc can use normal request_irq()
without setting IRQF_SHARED, etc etc. All you need to do is:

static struct irq_chip dss_irq_chip = {
	.name			= "DSS",
	.irq_bus_lock		= dss_bus_lock,
	.irq_bus_sync_unlock	= dss_bus_sync_unlock,
	.irq_mask		= dss_irq_mask,
	.irq_unmask		= dss_irq_unmask,
	.irq_ack		= dss_irq_ack,
};

then, somewhere during probe() you have to:

for (irq = irq_base; irq < irq_end; irq++) {
#ifdef CONFIG_ARM
	set_irq_flags(irq, IRQF_VALID)
#else
	set_irq_noprobe(irq);
#endif

	set_irq_data(irq, dss_device_structure_pointer);
	set_irq_chip_and_handler(irq, &dss_irq_chip,
		handle_simple_irq);
}

and on exit() you just need to cleanup:

for (irq = irq_base; irq < irq_end; irq++) {
#ifdef CONFIG_ARM
	set_irq_flags(irq, 0)
#endif

	set_irq_chip_and_handler(irq, NULL, NULL);
	set_irq_data(irq, NULL);
}

-- 
balbi

Re: OMAP: DSS2: Common IRQ handler for all OMAPs

[archit taneja]

Hi,

On Tuesday 15 February 2011 01:35 PM, Balbi, Felipe wrote:
> Hi,
>
> On Tue, Feb 15, 2011 at 09:58:24AM +0530, archit taneja wrote:
>>> does it make sense to install an irq_chip for that ? I mean, can you
>>> mask/unmask dss and or dsi IRQs ? If you can, then it might make sense
>>> to take a look into GENIRQ and install an irq_chip for that. Then both
>>> dsi and dss would be able to use standard request_irq() API.
>>>
>>
>> We could disable dsi IRQs by masking all the possible interrupt
>> events in DSI_IRQSTATUS. The same goes for dispc. Is this what you
>> meant by masking/unmasking irqs?
>
> yes it is. Then it makes sense to have an irq_chip for those two irqs, I
> think.
>
> /proc/interrupt will reflect how the hardware works (DSI and DISPC IRQs
> being handled by DSS), both dsi and dispc can use normal request_irq()
> without setting IRQF_SHARED, etc etc. All you need to do is:
>
> static struct irq_chip dss_irq_chip = {
> 	.name			= "DSS",
> 	.irq_bus_lock		= dss_bus_lock,
> 	.irq_bus_sync_unlock	= dss_bus_sync_unlock,
> 	.irq_mask		= dss_irq_mask,
> 	.irq_unmask		= dss_irq_unmask,
> 	.irq_ack		= dss_irq_ack,
> };
>
> then, somewhere during probe() you have to:
>
> for (irq = irq_base; irq<  irq_end; irq++) {
> #ifdef CONFIG_ARM
> 	set_irq_flags(irq, IRQF_VALID)
> #else
> 	set_irq_noprobe(irq);
> #endif
>
> 	set_irq_data(irq, dss_device_structure_pointer);
> 	set_irq_chip_and_handler(irq,&dss_irq_chip,
> 		handle_simple_irq);
> }
>
> and on exit() you just need to cleanup:
>
> for (irq = irq_base; irq<  irq_end; irq++) {
> #ifdef CONFIG_ARM
> 	set_irq_flags(irq, 0)
> #endif
>
> 	set_irq_chip_and_handler(irq, NULL, NULL);
> 	set_irq_data(irq, NULL);
> }
>

Thanks for the info, but this seems to be suitable for the case
when there are multiple irq events coming from the same interrupt line. 
On OMAP4 we have 4 different IRQ lines going to ARM, i.e 4 lines defined 
in irqs-44xx.h.

We are looking for a common irq handler located somewhere centrally, and 
each module can hook their irq line to this handler.

Does irq_chip do this? what does irq_base and irq_end signify?

Regards,
Archit

Re: OMAP: DSS2: Common IRQ handler for all OMAPs

[Felipe Balbi]

Hi,

On Tue, Feb 15, 2011 at 01:50:49PM +0530, archit taneja wrote:
> >yes it is. Then it makes sense to have an irq_chip for those two irqs, I
> >think.
> >
> >/proc/interrupt will reflect how the hardware works (DSI and DISPC IRQs
> >being handled by DSS), both dsi and dispc can use normal request_irq()
> >without setting IRQF_SHARED, etc etc. All you need to do is:
> >
> >static struct irq_chip dss_irq_chip = {
> >	.name			= "DSS",
> >	.irq_bus_lock		= dss_bus_lock,
> >	.irq_bus_sync_unlock	= dss_bus_sync_unlock,
> >	.irq_mask		= dss_irq_mask,
> >	.irq_unmask		= dss_irq_unmask,
> >	.irq_ack		= dss_irq_ack,
> >};
> >
> >then, somewhere during probe() you have to:
> >
> >for (irq = irq_base; irq<  irq_end; irq++) {
> >#ifdef CONFIG_ARM
> >	set_irq_flags(irq, IRQF_VALID)
> >#else
> >	set_irq_noprobe(irq);
> >#endif
> >
> >	set_irq_data(irq, dss_device_structure_pointer);
> >	set_irq_chip_and_handler(irq,&dss_irq_chip,
> >		handle_simple_irq);
> >}
> >
> >and on exit() you just need to cleanup:
> >
> >for (irq = irq_base; irq<  irq_end; irq++) {
> >#ifdef CONFIG_ARM
> >	set_irq_flags(irq, 0)
> >#endif
> >
> >	set_irq_chip_and_handler(irq, NULL, NULL);
> >	set_irq_data(irq, NULL);
> >}
> >
> 
> Thanks for the info, but this seems to be suitable for the case
> when there are multiple irq events coming from the same interrupt
> line. On OMAP4 we have 4 different IRQ lines going to ARM, i.e 4
> lines defined in irqs-44xx.h.

do you have 4 lines for 4 different modules ? If so, what's the problem
here ? The irq_chip you're using is INTC

> We are looking for a common irq handler located somewhere centrally,
> and each module can hook their irq line to this handler.
> 
> Does irq_chip do this? what does irq_base and irq_end signify?

I guess it can't do that, no. irq_base is the starting number for this
irq_chip's IRQ and irq_end is irq_base + number_of_irqs.

-- 
balbi

Re: OMAP: DSS2: Common IRQ handler for all OMAPs

[archit taneja]

Hi,

On Tuesday 15 February 2011 12:57 PM, Valkeinen, Tomi wrote:

<snip>

> I meant something like this:
>
> dispc.c:
>
> dispc_init()
> {
> 	/* did we have a pdev for dispc? if not, this needs to be dss.pdev */
> 	request_irq(platform_get_irq(dispc.pdev, 0), irq_handler, IRQF_SHARED, "dispc irq", foo);
> }
>
> irq_handler()
> {
> 	if (irq_can_be_shared) {
> 		check if the irq is for us. exit if not;
> 	}
>
> 	handle;
> }
>
> dsi.c:
>
> dsi_init()
> {
> 	request_irq(platform_get_irq(dsi.pdev, 0), irq_handler, IRQF_SHARED, "dsi irq", foo);
> }
>
> irq_handler()
> {
> 	if (irq_can_be_shared) {
> 		check if the irq is for us. exit if not;
> 	}
>
> 	handle;
> }
>

This approach looks clean, but isn't IRQF_SHARED used the other way 
around. One irq line and multiple handlers?

Regards,
Archit

Re: OMAP: DSS2: Common IRQ handler for all OMAPs

[Tomi Valkeinen]

On Tue, 2011-02-15 at 02:30 -0600, Taneja, Archit wrote:
> Hi,
> 
> On Tuesday 15 February 2011 12:57 PM, Valkeinen, Tomi wrote:
> 
> <snip>
> 
> > I meant something like this:
> >
> > dispc.c:
> >
> > dispc_init()
> > {
> > 	/* did we have a pdev for dispc? if not, this needs to be dss.pdev */
> > 	request_irq(platform_get_irq(dispc.pdev, 0), irq_handler, IRQF_SHARED, "dispc irq", foo);
> > }
> >
> > irq_handler()
> > {
> > 	if (irq_can_be_shared) {
> > 		check if the irq is for us. exit if not;
> > 	}
> >
> > 	handle;
> > }
> >
> > dsi.c:
> >
> > dsi_init()
> > {
> > 	request_irq(platform_get_irq(dsi.pdev, 0), irq_handler, IRQF_SHARED, "dsi irq", foo);
> > }
> >
> > irq_handler()
> > {
> > 	if (irq_can_be_shared) {
> > 		check if the irq is for us. exit if not;
> > 	}
> >
> > 	handle;
> > }
> >
> 
> This approach looks clean, but isn't IRQF_SHARED used the other way 
> around. One irq line and multiple handlers?

That is the case here, isn't it (on omap3)? One interrupt line (the DSS
irq, the same returned both from dsi.pdev and dispc.pdev), and two
handlers, one in dispc and one in dsi? Or what do you mean?

On omap2 there's no dsi code ran, so dispc is the only one requesting
the irq, and thus IRQF_SHARED is extra. In omap4 there are separate irq
lines (dsi.pdev and dispc.pdev return different irqs), and so
IRQF_SHARED is again extra. But I don't see any harm in IRQF_SHARED even
in omap2/4.

 Tomi

Re: OMAP: DSS2: Common IRQ handler for all OMAPs

[archit taneja]

Hi,

On Tuesday 15 February 2011 02:07 PM, Valkeinen, Tomi wrote:
> On Tue, 2011-02-15 at 02:30 -0600, Taneja, Archit wrote:
>> Hi,
>>
>> On Tuesday 15 February 2011 12:57 PM, Valkeinen, Tomi wrote:
>>
>> <snip>
>>
>>> I meant something like this:
>>>
>>> dispc.c:
>>>
>>> dispc_init()
>>> {
>>> 	/* did we have a pdev for dispc? if not, this needs to be dss.pdev */
>>> 	request_irq(platform_get_irq(dispc.pdev, 0), irq_handler, IRQF_SHARED, "dispc irq", foo);
>>> }
>>>
>>> irq_handler()
>>> {
>>> 	if (irq_can_be_shared) {
>>> 		check if the irq is for us. exit if not;
>>> 	}
>>>
>>> 	handle;
>>> }
>>>
>>> dsi.c:
>>>
>>> dsi_init()
>>> {
>>> 	request_irq(platform_get_irq(dsi.pdev, 0), irq_handler, IRQF_SHARED, "dsi irq", foo);
>>> }
>>>
>>> irq_handler()
>>> {
>>> 	if (irq_can_be_shared) {
>>> 		check if the irq is for us. exit if not;
>>> 	}
>>>
>>> 	handle;
>>> }
>>>
>>
>> This approach looks clean, but isn't IRQF_SHARED used the other way
>> around. One irq line and multiple handlers?
>
> That is the case here, isn't it (on omap3)? One interrupt line (the DSS
> irq, the same returned both from dsi.pdev and dispc.pdev), and two
> handlers, one in dispc and one in dsi? Or what do you mean?

Okay, so for OMAP3 we should populate dsi.pdev and dispc.pdev in such a 
way that they return the single irq line number for DSS. And for OMAP4,
the separate line numbers will go for the modules anyway.

How do differentiate with the common handler now? It will be dirty if we 
have checks on the irq_line. Could we pass the pdev as the arg to 
differentiate the source?

>
> On omap2 there's no dsi code ran, so dispc is the only one requesting
> the irq, and thus IRQF_SHARED is extra. In omap4 there are separate irq
> lines (dsi.pdev and dispc.pdev return different irqs), and so
> IRQF_SHARED is again extra. But I don't see any harm in IRQF_SHARED even
> in omap2/4.
>
>   Tomi
>
>

Regards,
Archit

Re: OMAP: DSS2: Common IRQ handler for all OMAPs

[archit taneja]

Copying Benoit,

On Tuesday 15 February 2011 02:07 PM, Valkeinen, Tomi wrote:
> On Tue, 2011-02-15 at 02:30 -0600, Taneja, Archit wrote:
>> Hi,
>>
>> On Tuesday 15 February 2011 12:57 PM, Valkeinen, Tomi wrote:
>>
>> <snip>
>>
>>> I meant something like this:
>>>
>>> dispc.c:
>>>
>>> dispc_init()
>>> {
>>> 	/* did we have a pdev for dispc? if not, this needs to be dss.pdev */
>>> 	request_irq(platform_get_irq(dispc.pdev, 0), irq_handler, IRQF_SHARED, "dispc irq", foo);
>>> }
>>>
>>> irq_handler()
>>> {
>>> 	if (irq_can_be_shared) {
>>> 		check if the irq is for us. exit if not;
>>> 	}
>>>
>>> 	handle;
>>> }
>>>
>>> dsi.c:
>>>
>>> dsi_init()
>>> {
>>> 	request_irq(platform_get_irq(dsi.pdev, 0), irq_handler, IRQF_SHARED, "dsi irq", foo);
>>> }
>>>
>>> irq_handler()
>>> {
>>> 	if (irq_can_be_shared) {
>>> 		check if the irq is for us. exit if not;
>>> 	}
>>>
>>> 	handle;
>>> }
>>>
>>
>> This approach looks clean, but isn't IRQF_SHARED used the other way
>> around. One irq line and multiple handlers?
>
> That is the case here, isn't it (on omap3)? One interrupt line (the DSS
> irq, the same returned both from dsi.pdev and dispc.pdev), and two
> handlers, one in dispc and one in dsi? Or what do you mean?
>
> On omap2 there's no dsi code ran, so dispc is the only one requesting
> the irq, and thus IRQF_SHARED is extra. In omap4 there are separate irq
> lines (dsi.pdev and dispc.pdev return different irqs), and so
> IRQF_SHARED is again extra. But I don't see any harm in IRQF_SHARED even
> in omap2/4.
>
>   Tomi
>

Benoit,

Is it okay to have the same irq entry for 2 different hwmods?
This requirement comes from OMAP3 where dispc and dsi have a common irq 
line, where as on OMAP4 dispc and dsi have separate irq lines.

We basically want to get rid of a central dss irq handler (hence, remove 
irq entries for dss_core hwmod) and instead have separate irq handlers 
for each module which may or may not share an irq line.

Regards,
Archit

Re: OMAP: DSS2: Common IRQ handler for all OMAPs

[Cousson, Benoit]

Hi Archit,

On 2/15/2011 10:25 AM, Taneja, Archit wrote:
> Copying Benoit,
>
> On Tuesday 15 February 2011 02:07 PM, Valkeinen, Tomi wrote:
>> On Tue, 2011-02-15 at 02:30 -0600, Taneja, Archit wrote:
>>> Hi,
>>>
>>> On Tuesday 15 February 2011 12:57 PM, Valkeinen, Tomi wrote:
>>>
>>> <snip>
>>>
>>>> I meant something like this:
>>>>
>>>> dispc.c:
>>>>
>>>> dispc_init()
>>>> {
>>>> 	/* did we have a pdev for dispc? if not, this needs to be dss.pdev */
>>>> 	request_irq(platform_get_irq(dispc.pdev, 0), irq_handler, IRQF_SHARED, "dispc irq", foo);
>>>> }
>>>>
>>>> irq_handler()
>>>> {
>>>> 	if (irq_can_be_shared) {
>>>> 		check if the irq is for us. exit if not;
>>>> 	}
>>>>
>>>> 	handle;
>>>> }
>>>>
>>>> dsi.c:
>>>>
>>>> dsi_init()
>>>> {
>>>> 	request_irq(platform_get_irq(dsi.pdev, 0), irq_handler, IRQF_SHARED, "dsi irq", foo);
>>>> }
>>>>
>>>> irq_handler()
>>>> {
>>>> 	if (irq_can_be_shared) {
>>>> 		check if the irq is for us. exit if not;
>>>> 	}
>>>>
>>>> 	handle;
>>>> }
>>>>
>>>
>>> This approach looks clean, but isn't IRQF_SHARED used the other way
>>> around. One irq line and multiple handlers?
>>
>> That is the case here, isn't it (on omap3)? One interrupt line (the DSS
>> irq, the same returned both from dsi.pdev and dispc.pdev), and two
>> handlers, one in dispc and one in dsi? Or what do you mean?
>>
>> On omap2 there's no dsi code ran, so dispc is the only one requesting
>> the irq, and thus IRQF_SHARED is extra. In omap4 there are separate irq
>> lines (dsi.pdev and dispc.pdev return different irqs), and so
>> IRQF_SHARED is again extra. But I don't see any harm in IRQF_SHARED even
>> in omap2/4.
>>
>>    Tomi
>>
>
> Benoit,
>
> Is it okay to have the same irq entry for 2 different hwmods?
> This requirement comes from OMAP3 where dispc and dsi have a common irq
> line, where as on OMAP4 dispc and dsi have separate irq lines.

Well, no. I explained that in one of my comment about hwmod 
modification. The hwmod data are reflecting the exact HW capabilities.
So, if there is a change in the HW, the hwmod will be different.
It is up to the driver to adapt to this change.

The driver has to evolve to handle properly the new HW capabilities 
while keeping the old stuff working.

> We basically want to get rid of a central dss irq handler (hence, remove
> irq entries for dss_core hwmod) and instead have separate irq handlers
> for each module which may or may not share an irq line.

Then you need to hack your driver but not the hwmod data:-(

Regards,
Benoit

Re: OMAP: DSS2: Common IRQ handler for all OMAPs

[Semwal, Sumit]

Hi Benoit,

On Tue, Feb 15, 2011 at 3:53 PM, Cousson, Benoit <b-cousson@ti.com> wrote:
> Hi Archit,
>
> On 2/15/2011 10:25 AM, Taneja, Archit wrote:
>>
>> Copying Benoit,
>>
>> On Tuesday 15 February 2011 02:07 PM, Valkeinen, Tomi wrote:
>>>
>>> On Tue, 2011-02-15 at 02:30 -0600, Taneja, Archit wrote:
>>>>
>>>> Hi,
>>>>
>>>> On Tuesday 15 February 2011 12:57 PM, Valkeinen, Tomi wrote:
>>>>
>>>> <snip>
>>>>
>>>>> I meant something like this:
>>>>>
>>>>> dispc.c:
>>>>>
>>>>> dispc_init()
>>>>> {
>>>>>        /* did we have a pdev for dispc? if not, this needs to be
>>>>> dss.pdev */
>>>>>        request_irq(platform_get_irq(dispc.pdev, 0), irq_handler,
>>>>> IRQF_SHARED, "dispc irq", foo);
>>>>> }
>>>>>
>>>>> irq_handler()
>>>>> {
>>>>>        if (irq_can_be_shared) {
>>>>>                check if the irq is for us. exit if not;
>>>>>        }
>>>>>
>>>>>        handle;
>>>>> }
>>>>>
>>>>> dsi.c:
>>>>>
>>>>> dsi_init()
>>>>> {
>>>>>        request_irq(platform_get_irq(dsi.pdev, 0), irq_handler,
>>>>> IRQF_SHARED, "dsi irq", foo);
>>>>> }
>>>>>
>>>>> irq_handler()
>>>>> {
>>>>>        if (irq_can_be_shared) {
>>>>>                check if the irq is for us. exit if not;
>>>>>        }
>>>>>
>>>>>        handle;
>>>>> }
>>>>>
>>>>
>>>> This approach looks clean, but isn't IRQF_SHARED used the other way
>>>> around. One irq line and multiple handlers?
>>>
>>> That is the case here, isn't it (on omap3)? One interrupt line (the DSS
>>> irq, the same returned both from dsi.pdev and dispc.pdev), and two
>>> handlers, one in dispc and one in dsi? Or what do you mean?
>>>
>>> On omap2 there's no dsi code ran, so dispc is the only one requesting
>>> the irq, and thus IRQF_SHARED is extra. In omap4 there are separate irq
>>> lines (dsi.pdev and dispc.pdev return different irqs), and so
>>> IRQF_SHARED is again extra. But I don't see any harm in IRQF_SHARED even
>>> in omap2/4.
>>>
>>>   Tomi
>>>
>>
>> Benoit,
>>
>> Is it okay to have the same irq entry for 2 different hwmods?
>> This requirement comes from OMAP3 where dispc and dsi have a common irq
>> line, where as on OMAP4 dispc and dsi have separate irq lines.
>
> Well, no. I explained that in one of my comment about hwmod modification.
> The hwmod data are reflecting the exact HW capabilities.
> So, if there is a change in the HW, the hwmod will be different.
> It is up to the driver to adapt to this change.
I guess what Archit wanted to say is, for hw IPs DISPC and DSI, on
OMAP3, have a common IRQ line, so could both their hwmod databases
have the same IRQ added for them? This would us call, for a common IRQ
line shared w/ DISPC and DSI, like
mentioned in Tomi's sample code above.

How is hwmod data for these cases handled today? [shared IRQ,
different platform devices?]

Best regards,
~Sumit.
>
> The driver has to evolve to handle properly the new HW capabilities while
> keeping the old stuff working.
>
>> We basically want to get rid of a central dss irq handler (hence, remove
>> irq entries for dss_core hwmod) and instead have separate irq handlers
>> for each module which may or may not share an irq line.
>
> Then you need to hack your driver but not the hwmod data:-(
>
> Regards,
> Benoit
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: OMAP: DSS2: Common IRQ handler for all OMAPs

[Cousson, Benoit]

Hi Sumit,

On 2/15/2011 11:28 AM, Semwal, Sumit wrote:
> Hi Benoit,
>
> On Tue, Feb 15, 2011 at 3:53 PM, Cousson, Benoit<b-cousson@ti.com>  wrote:
>> Hi Archit,
>>
>> On 2/15/2011 10:25 AM, Taneja, Archit wrote:
>>>
>>> Copying Benoit,
>>>
>>> On Tuesday 15 February 2011 02:07 PM, Valkeinen, Tomi wrote:
>>>>
>>>> On Tue, 2011-02-15 at 02:30 -0600, Taneja, Archit wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> On Tuesday 15 February 2011 12:57 PM, Valkeinen, Tomi wrote:
>>>>>
>>>>> <snip>
>>>>>
>>>>>> I meant something like this:
>>>>>>
>>>>>> dispc.c:
>>>>>>
>>>>>> dispc_init()
>>>>>> {
>>>>>>         /* did we have a pdev for dispc? if not, this needs to be
>>>>>> dss.pdev */
>>>>>>         request_irq(platform_get_irq(dispc.pdev, 0), irq_handler,
>>>>>> IRQF_SHARED, "dispc irq", foo);
>>>>>> }
>>>>>>
>>>>>> irq_handler()
>>>>>> {
>>>>>>         if (irq_can_be_shared) {
>>>>>>                 check if the irq is for us. exit if not;
>>>>>>         }
>>>>>>
>>>>>>         handle;
>>>>>> }
>>>>>>
>>>>>> dsi.c:
>>>>>>
>>>>>> dsi_init()
>>>>>> {
>>>>>>         request_irq(platform_get_irq(dsi.pdev, 0), irq_handler,
>>>>>> IRQF_SHARED, "dsi irq", foo);
>>>>>> }
>>>>>>
>>>>>> irq_handler()
>>>>>> {
>>>>>>         if (irq_can_be_shared) {
>>>>>>                 check if the irq is for us. exit if not;
>>>>>>         }
>>>>>>
>>>>>>         handle;
>>>>>> }
>>>>>>
>>>>>
>>>>> This approach looks clean, but isn't IRQF_SHARED used the other way
>>>>> around. One irq line and multiple handlers?
>>>>
>>>> That is the case here, isn't it (on omap3)? One interrupt line (the DSS
>>>> irq, the same returned both from dsi.pdev and dispc.pdev), and two
>>>> handlers, one in dispc and one in dsi? Or what do you mean?
>>>>
>>>> On omap2 there's no dsi code ran, so dispc is the only one requesting
>>>> the irq, and thus IRQF_SHARED is extra. In omap4 there are separate irq
>>>> lines (dsi.pdev and dispc.pdev return different irqs), and so
>>>> IRQF_SHARED is again extra. But I don't see any harm in IRQF_SHARED even
>>>> in omap2/4.
>>>>
>>>>    Tomi
>>>>
>>>
>>> Benoit,
>>>
>>> Is it okay to have the same irq entry for 2 different hwmods?
>>> This requirement comes from OMAP3 where dispc and dsi have a common irq
>>> line, where as on OMAP4 dispc and dsi have separate irq lines.
>>
>> Well, no. I explained that in one of my comment about hwmod modification.
>> The hwmod data are reflecting the exact HW capabilities.
>> So, if there is a change in the HW, the hwmod will be different.
>> It is up to the driver to adapt to this change.
> I guess what Archit wanted to say is, for hw IPs DISPC and DSI, on
> OMAP3, have a common IRQ line, so could both their hwmod databases
> have the same IRQ added for them? This would us call, for a common IRQ
> line shared w/ DISPC and DSI, like
> mentioned in Tomi's sample code above.

OK, thanks for the clarification, actually I missed a little bit the 
point :-(

So in fact the 2 modules share that same IRQ today, and you just want to 
populate both hwmod with the same input.
If this is a real OR between the two IRQ lines, meaning the dispc cannot 
mask the dsi IRQ or the opposite, then having the same IRQ number in the 
two different hwmods is a correct representation of the HW.

Then both devices with get the same IRQ number during the 
platform_get_irq, so if the driver is aware of that it is fine.

> How is hwmod data for these cases handled today? [shared IRQ,
> different platform devices?]

I don't think we have such case today at least on OMAP4. Or maybe it is 
just not properly documented, so only one hwmod is mapped to the IRQ line.

Regards,
Benoit

>
> Best regards,
> ~Sumit.
>>
>> The driver has to evolve to handle properly the new HW capabilities while
>> keeping the old stuff working.
>>
>>> We basically want to get rid of a central dss irq handler (hence, remove
>>> irq entries for dss_core hwmod) and instead have separate irq handlers
>>> for each module which may or may not share an irq line.
>>
>> Then you need to hack your driver but not the hwmod data:-(
>>
>> Regards,
>> Benoit
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>

Re: OMAP: DSS2: Common IRQ handler for all OMAPs

[Felipe Balbi]

Hi,

On Tue, Feb 15, 2011 at 10:37:37AM +0200, Tomi Valkeinen wrote:
> > This approach looks clean, but isn't IRQF_SHARED used the other way 
> > around. One irq line and multiple handlers?
> 
> That is the case here, isn't it (on omap3)? One interrupt line (the DSS
> irq, the same returned both from dsi.pdev and dispc.pdev), and two
> handlers, one in dispc and one in dsi? Or what do you mean?

IMO, for omap3 it would be better to have irq_chip there. Then you can
keep e.g. DISPC IRQ disabled until dispc.c calls request_irq(). What
happens today if you have IRQ enabled but dispc isn't ready to act on
those ?

> On omap2 there's no dsi code ran, so dispc is the only one requesting
> the irq, and thus IRQF_SHARED is extra. In omap4 there are separate irq
> lines (dsi.pdev and dispc.pdev return different irqs), and so
> IRQF_SHARED is again extra. But I don't see any harm in IRQF_SHARED even
> in omap2/4.

What if another HW requests the wrong IRQ number and it ends up being
your dispc IRQ line ?

-- 
balbi

Re: OMAP: DSS2: Common IRQ handler for all OMAPs

[Tomi Valkeinen]

On Tue, 2011-02-15 at 04:57 -0600, Balbi, Felipe wrote:
> Hi,
> 
> On Tue, Feb 15, 2011 at 10:37:37AM +0200, Tomi Valkeinen wrote:
> > > This approach looks clean, but isn't IRQF_SHARED used the other way 
> > > around. One irq line and multiple handlers?
> > 
> > That is the case here, isn't it (on omap3)? One interrupt line (the DSS
> > irq, the same returned both from dsi.pdev and dispc.pdev), and two
> > handlers, one in dispc and one in dsi? Or what do you mean?
> 
> IMO, for omap3 it would be better to have irq_chip there. Then you can
> keep e.g. DISPC IRQ disabled until dispc.c calls request_irq(). What
> happens today if you have IRQ enabled but dispc isn't ready to act on
> those ?

Currently we have a single interrupt handler, which then calls either
dispc and/or dsi handler. Dispc and dsi are always ready to handle
those.

I don't think it would be a good solution if irq_chip would be used only
for omap3. Then we'd have totally different solutions for different omap
versions. But if irq_chip can be easily used for all omap versions, then
perhaps.

But then again, using IRQF_SHARED should (I think) solve the problem
quite easily without big changes to the code. irq_chip sounds like a
bigger change.

> > On omap2 there's no dsi code ran, so dispc is the only one requesting
> > the irq, and thus IRQF_SHARED is extra. In omap4 there are separate irq
> > lines (dsi.pdev and dispc.pdev return different irqs), and so
> > IRQF_SHARED is again extra. But I don't see any harm in IRQF_SHARED even
> > in omap2/4.
> 
> What if another HW requests the wrong IRQ number and it ends up being
> your dispc IRQ line ?

Are you asking what happens if we have a bug in kernel code? Anything
can happen =). But I don't see that as a reason not to use IRQF_SHARED.

 Tomi

Re: OMAP: DSS2: Common IRQ handler for all OMAPs

[Felipe Balbi]

Hi,

On Tue, Feb 15, 2011 at 01:25:34PM +0200, Tomi Valkeinen wrote:
> > On Tue, Feb 15, 2011 at 10:37:37AM +0200, Tomi Valkeinen wrote:
> > > > This approach looks clean, but isn't IRQF_SHARED used the other way 
> > > > around. One irq line and multiple handlers?
> > > 
> > > That is the case here, isn't it (on omap3)? One interrupt line (the DSS
> > > irq, the same returned both from dsi.pdev and dispc.pdev), and two
> > > handlers, one in dispc and one in dsi? Or what do you mean?
> > 
> > IMO, for omap3 it would be better to have irq_chip there. Then you can
> > keep e.g. DISPC IRQ disabled until dispc.c calls request_irq(). What
> > happens today if you have IRQ enabled but dispc isn't ready to act on
> > those ?
> 
> Currently we have a single interrupt handler, which then calls either
> dispc and/or dsi handler. Dispc and dsi are always ready to handle
> those.

Exactly the kind of thing irq_chip would help you :-)

> I don't think it would be a good solution if irq_chip would be used only
> for omap3. Then we'd have totally different solutions for different omap
> versions. But if irq_chip can be easily used for all omap versions, then
> perhaps.

But the HW is different anyway. On OMAP3 you're connect to a DSS irq
demuxer, on OMAP4 you have dedicated lines straight from OMAP's INTC.

> > > On omap2 there's no dsi code ran, so dispc is the only one requesting
> > > the irq, and thus IRQF_SHARED is extra. In omap4 there are separate irq
> > > lines (dsi.pdev and dispc.pdev return different irqs), and so
> > > IRQF_SHARED is again extra. But I don't see any harm in IRQF_SHARED even
> > > in omap2/4.
> > 
> > What if another HW requests the wrong IRQ number and it ends up being
> > your dispc IRQ line ?
> 
> Are you asking what happens if we have a bug in kernel code? Anything
> can happen =). But I don't see that as a reason not to use IRQF_SHARED.

ok, I have to agree. :-) But you will be allowing for that to happen. If
you don't IRQF_SHARED, request_irq() will fail on the second call with
-EBUSY I believe.

-- 
balbi

Re: OMAP: DSS2: Common IRQ handler for all OMAPs

[archit taneja]

Hi,

<snip>

>>>>
>>>> Is it okay to have the same irq entry for 2 different hwmods?
>>>> This requirement comes from OMAP3 where dispc and dsi have a common irq
>>>> line, where as on OMAP4 dispc and dsi have separate irq lines.
>>>
>>> Well, no. I explained that in one of my comment about hwmod modification.
>>> The hwmod data are reflecting the exact HW capabilities.
>>> So, if there is a change in the HW, the hwmod will be different.
>>> It is up to the driver to adapt to this change.
>> I guess what Archit wanted to say is, for hw IPs DISPC and DSI, on
>> OMAP3, have a common IRQ line, so could both their hwmod databases
>> have the same IRQ added for them? This would us call, for a common IRQ
>> line shared w/ DISPC and DSI, like
>> mentioned in Tomi's sample code above.
>
> OK, thanks for the clarification, actually I missed a little bit the
> point :-(
>
> So in fact the 2 modules share that same IRQ today, and you just want to
> populate both hwmod with the same input.
> If this is a real OR between the two IRQ lines, meaning the dispc cannot
> mask the dsi IRQ or the opposite, then having the same IRQ number in the
> two different hwmods is a correct representation of the HW.

There is a real OR between the 2 irq lines in OMAP3, as there is no 
DSS_IRQENABLE, but there is a DSS_IRQSTATUS.

You can mask one of DISPC or DSI by zeroing all the bits in 
DISPC_IRQENABLE or DSI_IRQENABLE respectively. But there is no higher 
level register to mask them.

<snip>

Regards,
Archit

Re: OMAP: DSS2: Common IRQ handler for all OMAPs

[Cousson, Benoit]

On 2/15/2011 1:43 PM, Taneja, Archit wrote:
> Hi,
>
> <snip>
>
>>>>>
>>>>> Is it okay to have the same irq entry for 2 different hwmods?
>>>>> This requirement comes from OMAP3 where dispc and dsi have a common irq
>>>>> line, where as on OMAP4 dispc and dsi have separate irq lines.
>>>>
>>>> Well, no. I explained that in one of my comment about hwmod modification.
>>>> The hwmod data are reflecting the exact HW capabilities.
>>>> So, if there is a change in the HW, the hwmod will be different.
>>>> It is up to the driver to adapt to this change.
>>> I guess what Archit wanted to say is, for hw IPs DISPC and DSI, on
>>> OMAP3, have a common IRQ line, so could both their hwmod databases
>>> have the same IRQ added for them? This would us call, for a common IRQ
>>> line shared w/ DISPC and DSI, like
>>> mentioned in Tomi's sample code above.
>>
>> OK, thanks for the clarification, actually I missed a little bit the
>> point :-(
>>
>> So in fact the 2 modules share that same IRQ today, and you just want to
>> populate both hwmod with the same input.
>> If this is a real OR between the two IRQ lines, meaning the dispc cannot
>> mask the dsi IRQ or the opposite, then having the same IRQ number in the
>> two different hwmods is a correct representation of the HW.
>
> There is a real OR between the 2 irq lines in OMAP3, as there is no
> DSS_IRQENABLE, but there is a DSS_IRQSTATUS.
>
> You can mask one of DISPC or DSI by zeroing all the bits in
> DISPC_IRQENABLE or DSI_IRQENABLE respectively. But there is no higher
> level register to mask them.

That's perfect then, and it deserves the duplication of this irq number 
for both hwmods.

Regards,
Benoit