| Message ID | 56F45A30.4070701@xs4all.nl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 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
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: