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:
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
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
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
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)
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)
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
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)
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@lists.freedesktop.org
-
Hans Verkuil -
Tomi Valkeinen