omap4: how to get the HDMI core IRQ?

omap4: how to get the HDMI core IRQ?

[Hans Verkuil]

Hi Tomi,

I hope you (or someone else on this list) can help me find the problem in this code.

I am working on a kernel framework for HDMI CEC (see https://lwn.net/Articles/680942/).
In order to get as much experience with different devices as possible I am trying to
implement it on my omap4430 Pandaboard. The big problem I am facing is that the CEC
interrupts come in through the HDMI_IRQ_CORE interrupt, and that just refuses to
trigger.

The code below adds support for this core interrupt and it is supposed to trigger it
using the Software Induced interrupt to keep the code as simple as possible.

On boot I get this debug line from the pr_info in my code:

irqstat 02000000 wp_irq 06000001 raw 20010000 intr_state 00000001 intr1 00000080 unmask1 00000080 intr_ctrl 0000000a

As far as I can see everything looks perfectly fine, except for the fact that bit 0
of the irqstat is stubbornly 0.

This is using kernel 4.5 with only this patch applied.

What am I missing?

The reward for the right answer will be HDMI CEC support for omap4 (and any other TI device
with the same CEC IP).

Regards,

	Hans

diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi4.c b/drivers/gpu/drm/omapdrm/dss/hdmi4.c
index 7103c65..999b5ec 100644
--- a/drivers/gpu/drm/omapdrm/dss/hdmi4.c
+++ b/drivers/gpu/drm/omapdrm/dss/hdmi4.c
@@ -75,6 +75,14 @@ static irqreturn_t hdmi_irq_handler(int irq, void *data)
 	irqstatus = hdmi_wp_get_irqstatus(wp);
 	hdmi_wp_set_irqstatus(wp, irqstatus);

+	pr_info("irqstat %08x wp_irq %08x raw %08x intr_state %08x intr1 %08x unmask1 %08x intr_ctrl %08x\n",
+		irqstatus,
+		hdmi_read_reg(hdmi.wp.base, HDMI_WP_IRQENABLE_SET),
+		hdmi_read_reg(hdmi.wp.base, HDMI_WP_IRQSTATUS_RAW),
+		hdmi_read_reg(hdmi.core.base, HDMI_CORE_SYS_INTR_STATE),
+		hdmi_read_reg(hdmi.core.base, HDMI_CORE_SYS_INTR1),
+		hdmi_read_reg(hdmi.core.base, HDMI_CORE_SYS_INTR_UNMASK1),
+		hdmi_read_reg(hdmi.core.base, HDMI_CORE_SYS_INTR_CTRL));
 	if ((irqstatus & HDMI_IRQ_LINK_CONNECT) &&
 			irqstatus & HDMI_IRQ_LINK_DISCONNECT) {
 		/*
@@ -94,6 +102,13 @@ static irqreturn_t hdmi_irq_handler(int irq, void *data)
 	} else if (irqstatus & HDMI_IRQ_LINK_DISCONNECT) {
 		hdmi_wp_set_phy_pwr(wp, HDMI_PHYPWRCMD_LDOON);
 	}
+	if (irqstatus & HDMI_IRQ_CORE) {
+		u32 intr1 = hdmi_read_reg(hdmi.core.base, HDMI_CORE_SYS_INTR1);
+
+		hdmi_write_reg(hdmi.core.base, HDMI_CORE_SYS_INTR_CTRL, 2);
+		pr_info("clear sw irq\n");
+		hdmi_write_reg(hdmi.core.base, HDMI_CORE_SYS_INTR1, intr1);
+	}

 	return IRQ_HANDLED;
 }
@@ -222,9 +237,12 @@ static int hdmi_power_on_full(struct omap_dss_device *dssdev)
 	if (r)
 		goto err_mgr_enable;

+	hdmi_write_reg(hdmi.core.base, HDMI_CORE_SYS_INTR_UNMASK1, 0x80);
 	hdmi_wp_set_irqenable(wp,
-		HDMI_IRQ_LINK_CONNECT | HDMI_IRQ_LINK_DISCONNECT);
+		HDMI_IRQ_LINK_CONNECT | HDMI_IRQ_LINK_DISCONNECT | HDMI_IRQ_CORE);

+	pr_info("set sw irq\n");
+	hdmi_write_reg(hdmi.core.base, HDMI_CORE_SYS_INTR_CTRL, 0xa);
 	return 0;

 err_mgr_enable:
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: omap4: how to get the HDMI core IRQ?

[Tomi Valkeinen]

[-- Attachment #1.1.1: Type: text/plain, Size: 1670 bytes --]

Hi Hans,

On 24/03/16 23:20, Hans Verkuil wrote:
> Hi Tomi,
> 
> I hope you (or someone else on this list) can help me find the problem in this code.
> 
> I am working on a kernel framework for HDMI CEC (see https://lwn.net/Articles/680942/).
> In order to get as much experience with different devices as possible I am trying to
> implement it on my omap4430 Pandaboard. The big problem I am facing is that the CEC
> interrupts come in through the HDMI_IRQ_CORE interrupt, and that just refuses to
> trigger.
> 
> The code below adds support for this core interrupt and it is supposed to trigger it
> using the Software Induced interrupt to keep the code as simple as possible.

So this irq is just for testing?

> On boot I get this debug line from the pr_info in my code:
> 
> irqstat 02000000 wp_irq 06000001 raw 20010000 intr_state 00000001 intr1 00000080 unmask1 00000080 intr_ctrl 0000000a
> 
> As far as I can see everything looks perfectly fine, except for the fact that bit 0
> of the irqstat is stubbornly 0.
> 
> This is using kernel 4.5 with only this patch applied.
> 
> What am I missing?

Set SYS_CTRL1:PD to 1 (I presume you have the NDA HDMI TRM?).

Apparently we set it always to 0 in
hdmi4_core.c:hdmi_core_powerdown_disable(), but never enable it. I guess
it only affects core irqs, so there have been no side effects.

But it would make sense to either have a matching call in the enable
path, or then just set it to 0 when initializing the IP.

> 
> The reward for the right answer will be HDMI CEC support for omap4 (and any other TI device
> with the same CEC IP).

Ok. When is it ready? ;)

 Tomi


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: omap4: how to get the HDMI core IRQ?

[Hans Verkuil]

Hi Tomi,

On 03/30/2016 03:37 AM, Tomi Valkeinen wrote:
> Hi Hans,
>
> On 24/03/16 23:20, Hans Verkuil wrote:
>> Hi Tomi,
>>
>> I hope you (or someone else on this list) can help me find the problem in this code.
>>
>> I am working on a kernel framework for HDMI CEC (see https://lwn.net/Articles/680942/).
>> In order to get as much experience with different devices as possible I am trying to
>> implement it on my omap4430 Pandaboard. The big problem I am facing is that the CEC
>> interrupts come in through the HDMI_IRQ_CORE interrupt, and that just refuses to
>> trigger.
>>
>> The code below adds support for this core interrupt and it is supposed to trigger it
>> using the Software Induced interrupt to keep the code as simple as possible.
>
> So this irq is just for testing?

Yes, that was the easiest way to check the core irq without requiring lots of other changes.

>> On boot I get this debug line from the pr_info in my code:
>>
>> irqstat 02000000 wp_irq 06000001 raw 20010000 intr_state 00000001 intr1 00000080 unmask1 00000080 intr_ctrl 0000000a
>>
>> As far as I can see everything looks perfectly fine, except for the fact that bit 0
>> of the irqstat is stubbornly 0.
>>
>> This is using kernel 4.5 with only this patch applied.
>>
>> What am I missing?
>
> Set SYS_CTRL1:PD to 1 (I presume you have the NDA HDMI TRM?).

Yes, I have it.

> Apparently we set it always to 0 in
> hdmi4_core.c:hdmi_core_powerdown_disable(), but never enable it. I guess
> it only affects core irqs, so there have been no side effects.
>
> But it would make sense to either have a matching call in the enable
> path, or then just set it to 0 when initializing the IP.

I think it should be set in hdmi_core_video_config(). It sets other SYS_CTRL1 bits there as
well, and that is probably why I missed it. I just never realized that the PD bit wasn't set
there.

Thank you very much! I'm abroad right now, but once I'm back I'll test this first thing.

>
>>
>> The reward for the right answer will be HDMI CEC support for omap4 (and any other TI device
>> with the same CEC IP).
>
> Ok. When is it ready? ;)

Once I get the irq working the omap4 support should be ready very quickly, getting the CEC framework
merged takes a bit longer, but I am aiming for kernel 4.7 pending some final tests. I'm cross-posting
the patch series to dri-devel, so with luck when I post v15 it will have an omap4 driver as well.

Regards,

	Hans
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: omap4: how to get the HDMI core IRQ?

[Tomi Valkeinen]

[-- Attachment #1.1.1: Type: text/plain, Size: 1472 bytes --]

On 01/04/16 03:46, Hans Verkuil wrote:

>> Apparently we set it always to 0 in
>> hdmi4_core.c:hdmi_core_powerdown_disable(), but never enable it. I guess
>> it only affects core irqs, so there have been no side effects.
>>
>> But it would make sense to either have a matching call in the enable
>> path, or then just set it to 0 when initializing the IP.
> 
> I think it should be set in hdmi_core_video_config(). It sets other
> SYS_CTRL1 bits there as
> well, and that is probably why I missed it. I just never realized that
> the PD bit wasn't set
> there.

Hmm. Well, it's "power-down". It's quite unclear what it actually does,
but from the name of it, I think it makes sense to set it only when HDMI
core is enabled.

In hdmi4.c, there are hdmi_power_on_core() and hdmi_power_off_core()
functions, those might be good places to handle the bit.

Ehh... Actually, looking at the code more carefully...

hdmi_core_powerdown_disable() is called from hdmi4_configure() when
setting everything up, and there's a comment "power down off". So
apparently the intention of the code is to disable power-down mode, but
it sets the bit to a wrong state.

So probably we could just fix hdmi_core_powerdown_disable(), so that it
sets PD to 1, which is what it was meant to do. This assumes that there
are no bad side effects having PD 1 even if the HDMI is blanked, which
is something we need to verify. I can do a few tests with that.

 Tomi


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: omap4: how to get the HDMI core IRQ?

[Tomi Valkeinen]

[-- Attachment #1.1.1: Type: text/plain, Size: 1878 bytes --]


On 01/04/16 10:03, Tomi Valkeinen wrote:

> So probably we could just fix hdmi_core_powerdown_disable(), so that it
> sets PD to 1, which is what it was meant to do. This assumes that there
> are no bad side effects having PD 1 even if the HDMI is blanked, which
> is something we need to verify. I can do a few tests with that.

I can't see any bad side effects with fixing the function. So here:

From 5cddaa31e28c059ea99a21ab03c4c1864bf5e610 Mon Sep 17 00:00:00 2001
From: Tomi Valkeinen <tomi.valkeinen@ti.com>
Date: Fri, 1 Apr 2016 10:29:29 +0300
Subject: [PATCH] drm/omap: fix OMAP4 hdmi_core_powerdown_disable()

hdmi_core_powerdown_disable() is supposed to disable HDMI core's
power-down mode. Howver, the function sets the power-down bit to 0,
which means "enable power-down".

This hasn't caused any issues as the PD seems to affect only interrupts
from HDMI core, and none of those interrupts are used at the moment. CEC
functionality requires core interrupts, and the PD mode needs to be
fixed.

This patch fixes hdmi_core_powerdown_disable() to actually disable the
PD mode.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Reported-by: Hans Verkuil <hverkuil@xs4all.nl>

diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi4_core.c b/drivers/gpu/drm/omapdrm/dss/hdmi4_core.c
index fa72e735dad2..ef3afe99e487 100644
--- a/drivers/gpu/drm/omapdrm/dss/hdmi4_core.c
+++ b/drivers/gpu/drm/omapdrm/dss/hdmi4_core.c
@@ -211,7 +211,7 @@ static void hdmi_core_init(struct hdmi_core_video_config *video_cfg)
 static void hdmi_core_powerdown_disable(struct hdmi_core_data *core)
 {
 	DSSDBG("Enter hdmi_core_powerdown_disable\n");
-	REG_FLD_MOD(core->base, HDMI_CORE_SYS_SYS_CTRL1, 0x0, 0, 0);
+	REG_FLD_MOD(core->base, HDMI_CORE_SYS_SYS_CTRL1, 0x1, 0, 0);
 }
 
 static void hdmi_core_swreset_release(struct hdmi_core_data *core)


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: omap4: how to get the HDMI core IRQ?

[Hans Verkuil]

Hi Tomi,

On 04/01/2016 12:35 AM, Tomi Valkeinen wrote:
>
> On 01/04/16 10:03, Tomi Valkeinen wrote:
>
>> So probably we could just fix hdmi_core_powerdown_disable(), so that it
>> sets PD to 1, which is what it was meant to do. This assumes that there
>> are no bad side effects having PD 1 even if the HDMI is blanked, which
>> is something we need to verify. I can do a few tests with that.
>
> I can't see any bad side effects with fixing the function. So here:

Thanks for the patch. It will have to wait until I'm back from the ELC, but
I'll test it as soon as I can.

BTW, do you know if the omap3 (beagleboard) has the same CEC block?

Regards,

	Hans

>  From 5cddaa31e28c059ea99a21ab03c4c1864bf5e610 Mon Sep 17 00:00:00 2001
> From: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Date: Fri, 1 Apr 2016 10:29:29 +0300
> Subject: [PATCH] drm/omap: fix OMAP4 hdmi_core_powerdown_disable()
>
> hdmi_core_powerdown_disable() is supposed to disable HDMI core's
> power-down mode. Howver, the function sets the power-down bit to 0,
> which means "enable power-down".
>
> This hasn't caused any issues as the PD seems to affect only interrupts
> from HDMI core, and none of those interrupts are used at the moment. CEC
> functionality requires core interrupts, and the PD mode needs to be
> fixed.
>
> This patch fixes hdmi_core_powerdown_disable() to actually disable the
> PD mode.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Reported-by: Hans Verkuil <hverkuil@xs4all.nl>
>
> diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi4_core.c b/drivers/gpu/drm/omapdrm/dss/hdmi4_core.c
> index fa72e735dad2..ef3afe99e487 100644
> --- a/drivers/gpu/drm/omapdrm/dss/hdmi4_core.c
> +++ b/drivers/gpu/drm/omapdrm/dss/hdmi4_core.c
> @@ -211,7 +211,7 @@ static void hdmi_core_init(struct hdmi_core_video_config *video_cfg)
>   static void hdmi_core_powerdown_disable(struct hdmi_core_data *core)
>   {
>   	DSSDBG("Enter hdmi_core_powerdown_disable\n");
> -	REG_FLD_MOD(core->base, HDMI_CORE_SYS_SYS_CTRL1, 0x0, 0, 0);
> +	REG_FLD_MOD(core->base, HDMI_CORE_SYS_SYS_CTRL1, 0x1, 0, 0);
>   }
>
>   static void hdmi_core_swreset_release(struct hdmi_core_data *core)
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: omap4: how to get the HDMI core IRQ?

[Tomi Valkeinen]

[-- Attachment #1.1.1: Type: text/plain, Size: 887 bytes --]

On 01/04/16 19:56, Hans Verkuil wrote:
> Hi Tomi,
> 
> On 04/01/2016 12:35 AM, Tomi Valkeinen wrote:
>>
>> On 01/04/16 10:03, Tomi Valkeinen wrote:
>>
>>> So probably we could just fix hdmi_core_powerdown_disable(), so that it
>>> sets PD to 1, which is what it was meant to do. This assumes that there
>>> are no bad side effects having PD 1 even if the HDMI is blanked, which
>>> is something we need to verify. I can do a few tests with that.
>>
>> I can't see any bad side effects with fixing the function. So here:
> 
> Thanks for the patch. It will have to wait until I'm back from the ELC, but
> I'll test it as soon as I can.
> 
> BTW, do you know if the omap3 (beagleboard) has the same CEC block?

OMAP3 doesn't have HDMI, so no CEC. OMAP5 (and AM5/DRA7) have HDMI, but
the HDMI IP is different than on OMAP4, so I presume CEC is different too.

 Tomi


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: omap4: how to get the HDMI core IRQ?

[Hans Verkuil]

On 04/01/2016 09:35 AM, Tomi Valkeinen wrote:
> 
> On 01/04/16 10:03, Tomi Valkeinen wrote:
> 
>> So probably we could just fix hdmi_core_powerdown_disable(), so that it
>> sets PD to 1, which is what it was meant to do. This assumes that there
>> are no bad side effects having PD 1 even if the HDMI is blanked, which
>> is something we need to verify. I can do a few tests with that.
> 
> I can't see any bad side effects with fixing the function. So here:
> 
> From 5cddaa31e28c059ea99a21ab03c4c1864bf5e610 Mon Sep 17 00:00:00 2001
> From: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Date: Fri, 1 Apr 2016 10:29:29 +0300
> Subject: [PATCH] drm/omap: fix OMAP4 hdmi_core_powerdown_disable()
> 
> hdmi_core_powerdown_disable() is supposed to disable HDMI core's
> power-down mode. Howver, the function sets the power-down bit to 0,
> which means "enable power-down".
> 
> This hasn't caused any issues as the PD seems to affect only interrupts
> from HDMI core, and none of those interrupts are used at the moment. CEC
> functionality requires core interrupts, and the PD mode needs to be
> fixed.
> 
> This patch fixes hdmi_core_powerdown_disable() to actually disable the
> PD mode.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Reported-by: Hans Verkuil <hverkuil@xs4all.nl>

Tested-by: Hans Verkuil <hverkuil@xs4all.nl>

Works like a charm!

Thanks!

Regards,

	Hans

> 
> diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi4_core.c b/drivers/gpu/drm/omapdrm/dss/hdmi4_core.c
> index fa72e735dad2..ef3afe99e487 100644
> --- a/drivers/gpu/drm/omapdrm/dss/hdmi4_core.c
> +++ b/drivers/gpu/drm/omapdrm/dss/hdmi4_core.c
> @@ -211,7 +211,7 @@ static void hdmi_core_init(struct hdmi_core_video_config *video_cfg)
>  static void hdmi_core_powerdown_disable(struct hdmi_core_data *core)
>  {
>  	DSSDBG("Enter hdmi_core_powerdown_disable\n");
> -	REG_FLD_MOD(core->base, HDMI_CORE_SYS_SYS_CTRL1, 0x0, 0, 0);
> +	REG_FLD_MOD(core->base, HDMI_CORE_SYS_SYS_CTRL1, 0x1, 0, 0);
>  }
>  
>  static void hdmi_core_swreset_release(struct hdmi_core_data *core)
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: omap4: how to get the HDMI core IRQ?

[Hans Verkuil]

On 04/01/2016 12:03 AM, Tomi Valkeinen wrote:
> On 01/04/16 03:46, Hans Verkuil wrote:
>
>>> Apparently we set it always to 0 in
>>> hdmi4_core.c:hdmi_core_powerdown_disable(), but never enable it. I guess
>>> it only affects core irqs, so there have been no side effects.
>>>
>>> But it would make sense to either have a matching call in the enable
>>> path, or then just set it to 0 when initializing the IP.
>>
>> I think it should be set in hdmi_core_video_config(). It sets other
>> SYS_CTRL1 bits there as
>> well, and that is probably why I missed it. I just never realized that
>> the PD bit wasn't set
>> there.
>
> Hmm. Well, it's "power-down". It's quite unclear what it actually does,
> but from the name of it, I think it makes sense to set it only when HDMI
> core is enabled.
>
> In hdmi4.c, there are hdmi_power_on_core() and hdmi_power_off_core()
> functions, those might be good places to handle the bit.
>
> Ehh... Actually, looking at the code more carefully...
>
> hdmi_core_powerdown_disable() is called from hdmi4_configure() when
> setting everything up, and there's a comment "power down off". So
> apparently the intention of the code is to disable power-down mode, but
> it sets the bit to a wrong state.
>
> So probably we could just fix hdmi_core_powerdown_disable(), so that it
> sets PD to 1, which is what it was meant to do. This assumes that there
> are no bad side effects having PD 1 even if the HDMI is blanked, which
> is something we need to verify. I can do a few tests with that.

Sounds good. BTW, I got confused yesterday by the name 'powerdown_disable()'.
How about calling it 'powerup()'? :-)

Regards,

	Hans
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel