[PATCH] OMAPDSS: HDMI: wait for RXDET before putting phy in TX_ON

[PATCH] OMAPDSS: HDMI: wait for RXDET before putting phy in TX_ON

[mythripk]

From: Mythri P K <mythripk@ti.com>

Currently TX_PHY is put to TX_ON(transmission state) on receiving HPD.
It just ensures that the TV is connected but does not guarantee
that TMDS data lines and clock lines are up and ready for transmission.
Which although is very rare scenario has a potential to  damage the HDMI
port.(A use case where TV is connected to board but it is in different
channel would still trigger a HPD but TMDS lines will be down).

Thus this patch adds a rxdet check on getting a HPD which ensures that
TMDS lines are UP before changing the PHY state from LDO_ON to TX_ON.

Signed-off-by: Mythri P K <mythripk@ti.com>
---
 drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c |   37 +++++++++++++++++++++++++++-
 drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h |    2 +
 2 files changed, 37 insertions(+), 2 deletions(-)

diff --git a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
index bb784d2..bc55528 100644
--- a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
+++ b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
@@ -224,6 +224,30 @@ void ti_hdmi_4xxx_pll_disable(struct hdmi_ip_data *ip_data)
 	hdmi_set_pll_pwr(ip_data, HDMI_PLLPWRCMD_ALLOFF);
 }
 
+int hdmi_ti_4xxx_rxdet(struct hdmi_ip_data *ip_data)
+{
+	int loop = 0, tmds_data0, tmds_data1, tmds_data2, tmds_clk;
+
+	hdmi_write_reg(hdmi_wp_base(ip_data), HDMI_WP_WP_DEBUG_CFG, 4);
+
+	do {
+		tmds_data0 = hdmi_read_reg(hdmi_wp_base(ip_data),
+					HDMI_WP_WP_DEBUG_DATA);
+		tmds_data1 = hdmi_read_reg(hdmi_wp_base(ip_data),
+					HDMI_WP_WP_DEBUG_DATA);
+		tmds_data2 = hdmi_read_reg(hdmi_wp_base(ip_data),
+					HDMI_WP_WP_DEBUG_DATA);
+		tmds_clk = hdmi_read_reg(hdmi_wp_base(ip_data),
+					HDMI_WP_WP_DEBUG_DATA);
+		udelay(15);
+	} while ((tmds_data0 != tmds_data1 || tmds_data1 != tmds_data2
+			|| tmds_data1 != tmds_clk) && (loop < 500));
+
+	hdmi_write_reg(hdmi_wp_base(ip_data), HDMI_WP_WP_DEBUG_CFG, 0);
+
+	return ((loop == 500) ? -1 : (tmds_data0 & 1)) ;
+}
+
 static int hdmi_check_hpd_state(struct hdmi_ip_data *ip_data)
 {
 	unsigned long flags;
@@ -241,10 +265,19 @@ static int hdmi_check_hpd_state(struct hdmi_ip_data *ip_data)
 		return 0;
 	}
 
-	if (hpd)
+	if (hpd) {
+		/* before putting the phy in TX_ON,ensure that TMDS lanes
+		 * are pulled up .
+		 */
+		r = hdmi_ti_4xxx_rxdet(ip_data);
+		if (r <= 0) {
+			DSSERR("rxdet not set\n");
+			return r;
+		}
 		r = hdmi_set_phy_pwr(ip_data, HDMI_PHYPWRCMD_TXON);
-	else
+	} else {
 		r = hdmi_set_phy_pwr(ip_data, HDMI_PHYPWRCMD_LDOON);
+	}
 
 	if (r) {
 		DSSERR("Failed to %s PHY TX power\n",
diff --git a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h
index a14d1a0..efa6f29 100644
--- a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h
+++ b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h
@@ -47,6 +47,8 @@
 #define HDMI_WP_AUDIO_CFG2			0x84
 #define HDMI_WP_AUDIO_CTRL			0x88
 #define HDMI_WP_AUDIO_DATA			0x8C
+#define HDMI_WP_WP_DEBUG_CFG			0x90
+#define HDMI_WP_WP_DEBUG_DATA			0x94
 
 /* HDMI IP Core System */
 
-- 
1.7.5.4

Re: [PATCH] OMAPDSS: HDMI: wait for RXDET before putting phy in TX_ON

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 3851 bytes --]

Hi,

On Thu, 2012-03-01 at 13:44 +0530, mythripk@ti.com wrote:
> From: Mythri P K <mythripk@ti.com>
> 
> Currently TX_PHY is put to TX_ON(transmission state) on receiving HPD.
> It just ensures that the TV is connected but does not guarantee
> that TMDS data lines and clock lines are up and ready for transmission.
> Which although is very rare scenario has a potential to  damage the HDMI
> port.(A use case where TV is connected to board but it is in different
> channel would still trigger a HPD but TMDS lines will be down).

When/how does the damaging happen? When the HDMI cable is disconnected
in the case above? Or does the damage just happen by having the cable
connected, but the TV on different channel?

> Thus this patch adds a rxdet check on getting a HPD which ensures that
> TMDS lines are UP before changing the PHY state from LDO_ON to TX_ON.
> 
> Signed-off-by: Mythri P K <mythripk@ti.com>
> ---
>  drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c |   37 +++++++++++++++++++++++++++-
>  drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h |    2 +
>  2 files changed, 37 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
> index bb784d2..bc55528 100644
> --- a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
> +++ b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
> @@ -224,6 +224,30 @@ void ti_hdmi_4xxx_pll_disable(struct hdmi_ip_data *ip_data)
>  	hdmi_set_pll_pwr(ip_data, HDMI_PLLPWRCMD_ALLOFF);
>  }
>  
> +int hdmi_ti_4xxx_rxdet(struct hdmi_ip_data *ip_data)
> +{
> +	int loop = 0, tmds_data0, tmds_data1, tmds_data2, tmds_clk;
> +
> +	hdmi_write_reg(hdmi_wp_base(ip_data), HDMI_WP_WP_DEBUG_CFG, 4);
> +
> +	do {
> +		tmds_data0 = hdmi_read_reg(hdmi_wp_base(ip_data),
> +					HDMI_WP_WP_DEBUG_DATA);
> +		tmds_data1 = hdmi_read_reg(hdmi_wp_base(ip_data),
> +					HDMI_WP_WP_DEBUG_DATA);
> +		tmds_data2 = hdmi_read_reg(hdmi_wp_base(ip_data),
> +					HDMI_WP_WP_DEBUG_DATA);
> +		tmds_clk = hdmi_read_reg(hdmi_wp_base(ip_data),
> +					HDMI_WP_WP_DEBUG_DATA);
> +		udelay(15);

This code doesn't make any sense, or the HDMI TRM is wrong. You read the
same register, HDMI_WP_WP_DEBUG_DATA, four times, assigning the value to
data0-2/clk. The TRM I have doesn't talk about anything like that. What
is the code supposed to do?

The register HDMI_TXPHY_PAD_CONFIG_CONTROL also has bits for RXDET_LINE.
Is that something different?

> +	} while ((tmds_data0 != tmds_data1 || tmds_data1 != tmds_data2
> +			|| tmds_data1 != tmds_clk) && (loop < 500));

This is a rather confusing way to do the loop. Why not just use for-loop
with the timeout, and check the tmds variables inside the loop. Much
easier to read.

In the worst case the above causes a 7.5ms busy loop in an interrupt
handler. That's not very good thing. Why is there a need for the loop?

> +	hdmi_write_reg(hdmi_wp_base(ip_data), HDMI_WP_WP_DEBUG_CFG, 0);
> +
> +	return ((loop == 500) ? -1 : (tmds_data0 & 1)) ;

This is also confusing. And you should return a proper error value, not
-1. Perhaps:

if (loop == 500)
	return -ETIMEDOUT;

return tmds_data0 & 1;

> +}
> +
>  static int hdmi_check_hpd_state(struct hdmi_ip_data *ip_data)
>  {
>  	unsigned long flags;
> @@ -241,10 +265,19 @@ static int hdmi_check_hpd_state(struct hdmi_ip_data *ip_data)
>  		return 0;
>  	}
>  
> -	if (hpd)
> +	if (hpd) {
> +		/* before putting the phy in TX_ON,ensure that TMDS lanes
> +		 * are pulled up .
> +		 */
> +		r = hdmi_ti_4xxx_rxdet(ip_data);
> +		if (r <= 0) {
> +			DSSERR("rxdet not set\n");
> +			return r;
> +		}

Does this mean that if the user connects a TV which is in a different
channel than HDMI, the only way for the user to get an image is to
disconnect the cable and connect it again?

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] OMAPDSS: HDMI: wait for RXDET before putting phy in TX_ON

[K, Mythri P]

Hi Tomi,

On Thu, Mar 1, 2012 at 5:01 PM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> Hi,
>
> On Thu, 2012-03-01 at 13:44 +0530, mythripk@ti.com wrote:
>> From: Mythri P K <mythripk@ti.com>
>>
>> Currently TX_PHY is put to TX_ON(transmission state) on receiving HPD.
>> It just ensures that the TV is connected but does not guarantee
>> that TMDS data lines and clock lines are up and ready for transmission.
>> Which although is very rare scenario has a potential to  damage the HDMI
>> port.(A use case where TV is connected to board but it is in different
>> channel would still trigger a HPD but TMDS lines will be down).
>
> When/how does the damaging happen? When the HDMI cable is disconnected
> in the case above? Or does the damage just happen by having the cable
> connected, but the TV on different channel?

Damage never happens with the cable connected for a long time.. Damage
happens only
when the cable is connected when the PHY is in TX ON state. Which
requires the following sequence to be followed.
1. Wait for HPD connect
2. Wait for the PHY connect ( TMDS lines are up)
3. Enable PHY
We are currently missing step 2.
>> +             tmds_clk = hdmi_read_reg(hdmi_wp_base(ip_data),
>> +                                     HDMI_WP_WP_DEBUG_DATA);
>> +             udelay(15);
>
> This code doesn't make any sense, or the HDMI TRM is wrong. You read the
> same register, HDMI_WP_WP_DEBUG_DATA, four times, assigning the value to
> data0-2/clk. The TRM I have doesn't talk about anything like that. What
> is the code supposed to do?
>
I am not very sure which TRM you have but the HDMI_WP_WP_DEBUG_DATA
can also be used to probe the TMDS lines as well, but i might as well
try pad config and let you know.
> The register HDMI_TXPHY_PAD_CONFIG_CONTROL also has bits for RXDET_LINE.
> Is that something different?
>
>> +     } while ((tmds_data0 != tmds_data1 || tmds_data1 != tmds_data2
>> +                     || tmds_data1 != tmds_clk) && (loop < 500));
>
> This is a rather confusing way to do the loop. Why not just use for-loop
> with the timeout, and check the tmds variables inside the loop. Much
> easier to read.
>
> In the worst case the above causes a 7.5ms busy loop in an interrupt
> handler. That's not very good thing. Why is there a need for the loop?
I agree looping in interrupt context is bad.. That was the reason i
had a threaded irq handler and i case even here it is the threaded irq
handler i dont think it is happening in irq context.

>> +                     DSSERR("rxdet not set\n");
>> +                     return r;
>> +             }
>
> Does this mean that if the user connects a TV which is in a different
> channel than HDMI, the only way for the user to get an image is to
> disconnect the cable and connect it again?
Well If the handling is based on HPD then yes.. Or one another thing
is i can add the core interrupt support and we can then wait on the
PHY_CONNECT instead of all the above scenario.
I shall put a wait_on_completion check in HPD and we can wait till we
get PHY connect.

Thanks and regards,
Mythri.
>
>  Tomi
>
--
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: [PATCH] OMAPDSS: HDMI: wait for RXDET before putting phy in TX_ON

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 3305 bytes --]

On Fri, 2012-03-02 at 13:35 +0530, K, Mythri P wrote:
> Hi Tomi,
> 
> On Thu, Mar 1, 2012 at 5:01 PM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> > Hi,
> >
> > On Thu, 2012-03-01 at 13:44 +0530, mythripk@ti.com wrote:
> >> From: Mythri P K <mythripk@ti.com>
> >>
> >> Currently TX_PHY is put to TX_ON(transmission state) on receiving HPD.
> >> It just ensures that the TV is connected but does not guarantee
> >> that TMDS data lines and clock lines are up and ready for transmission.
> >> Which although is very rare scenario has a potential to  damage the HDMI
> >> port.(A use case where TV is connected to board but it is in different
> >> channel would still trigger a HPD but TMDS lines will be down).
> >
> > When/how does the damaging happen? When the HDMI cable is disconnected
> > in the case above? Or does the damage just happen by having the cable
> > connected, but the TV on different channel?
> 
> Damage never happens with the cable connected for a long time.. Damage
> happens only
> when the cable is connected when the PHY is in TX ON state. Which
> requires the following sequence to be followed.
> 1. Wait for HPD connect
> 2. Wait for the PHY connect ( TMDS lines are up)
> 3. Enable PHY
> We are currently missing step 2.

Doesn't HPD already tell us that the cable is connected?

> >> +             tmds_clk = hdmi_read_reg(hdmi_wp_base(ip_data),
> >> +                                     HDMI_WP_WP_DEBUG_DATA);
> >> +             udelay(15);
> >
> > This code doesn't make any sense, or the HDMI TRM is wrong. You read the
> > same register, HDMI_WP_WP_DEBUG_DATA, four times, assigning the value to
> > data0-2/clk. The TRM I have doesn't talk about anything like that. What
> > is the code supposed to do?
> >
> I am not very sure which TRM you have but the HDMI_WP_WP_DEBUG_DATA
> can also be used to probe the TMDS lines as well, but i might as well
> try pad config and let you know.

I have 4460 HDMI TRM vF and 4430 HDMI TRM vL. Both look the same
regarding the debug registers. Can you send me the latest ones that
explain the DEBUG registers correctly?

> > The register HDMI_TXPHY_PAD_CONFIG_CONTROL also has bits for RXDET_LINE.
> > Is that something different?
> >
> >> +     } while ((tmds_data0 != tmds_data1 || tmds_data1 != tmds_data2
> >> +                     || tmds_data1 != tmds_clk) && (loop < 500));
> >
> > This is a rather confusing way to do the loop. Why not just use for-loop
> > with the timeout, and check the tmds variables inside the loop. Much
> > easier to read.
> >
> > In the worst case the above causes a 7.5ms busy loop in an interrupt
> > handler. That's not very good thing. Why is there a need for the loop?
> I agree looping in interrupt context is bad.. That was the reason i
> had a threaded irq handler and i case even here it is the threaded irq
> handler i dont think it is happening in irq context.

We should nevertheless avoid 7.5ms busyloop if at all possible. Is there
an estimate of how long it takes for the PHY to connect? I think we
could just start a delayed work from the interrupt, and do the check
after a short delay. I don't think the user cares if the connect happens
after 1ms or 10ms from the moment the cable is plugged in =).

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] OMAPDSS: HDMI: wait for RXDET before putting phy in TX_ON

[K, Mythri P]

Hi Tomi,

>> Damage never happens with the cable connected for a long time.. Damage
>> happens only
>> when the cable is connected when the PHY is in TX ON state. Which
>> requires the following sequence to be followed.
>> 1. Wait for HPD connect
>> 2. Wait for the PHY connect ( TMDS lines are up)
>> 3. Enable PHY
>> We are currently missing step 2.
>
> Doesn't HPD already tell us that the cable is connected?
>
Yes HPD tells us that cable is connected but does not guarantee that
the line is ready for transmission of data, HPD just means the DDC
line is up.
>> I am not very sure which TRM you have but the HDMI_WP_WP_DEBUG_DATA
>> can also be used to probe the TMDS lines as well, but i might as well
>> try pad config and let you know.
>
> I have 4460 HDMI TRM vF and 4430 HDMI TRM vL. Both look the same
> regarding the debug registers. Can you send me the latest ones that
> explain the DEBUG registers correctly?
sure.
>

>> > In the worst case the above causes a 7.5ms busy loop in an interrupt
>> > handler. That's not very good thing. Why is there a need for the loop?
>> I agree looping in interrupt context is bad.. That was the reason i
>> had a threaded irq handler and i case even here it is the threaded irq
>> handler i dont think it is happening in irq context.
>
> We should nevertheless avoid 7.5ms busyloop if at all possible. Is there
> an estimate of how long it takes for the PHY to connect? I think we
> could just start a delayed work from the interrupt, and do the check
> after a short delay. I don't think the user cares if the connect happens
> after 1ms or 10ms from the moment the cable is plugged in =).
>
Actually by using the PHY_CONNECT interrupt we can avoid the wait.
If the TV channel is the same as the HDMI port connected to then there
is hardly any delay between HPD and TMDS lines. Problem is when user
connects to HDMI but is running  say AV1 /AV2  on TV.
In such case we will see that HPD is high but TMDS lines are low, So
there is no guarantee it will be done in certain period, as it is user
dependent.
So i am more inclined to have a interrupt based way, On receiving HPD
in the threaded irq handler we can wait_on_completion of PHY_CONNECT
which will put the thread in sleep until TMDS lines are high.
I shall make that patch test out and post it soon.

Thanks and regards,
Mythri.

>  Tomi
>
--
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: [PATCH] OMAPDSS: HDMI: wait for RXDET before putting phy in TX_ON

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 2996 bytes --]

On Fri, 2012-03-02 at 16:55 +0530, K, Mythri P wrote:
> Hi Tomi,
> 
> >> Damage never happens with the cable connected for a long time.. Damage
> >> happens only
> >> when the cable is connected when the PHY is in TX ON state. Which
> >> requires the following sequence to be followed.
> >> 1. Wait for HPD connect
> >> 2. Wait for the PHY connect ( TMDS lines are up)
> >> 3. Enable PHY
> >> We are currently missing step 2.
> >
> > Doesn't HPD already tell us that the cable is connected?
> >
> Yes HPD tells us that cable is connected but does not guarantee that
> the line is ready for transmission of data, HPD just means the DDC
> line is up.

Okay, but above you say "damage happens only when the cable is connected
when the PHY is in TX_ON". The HPD interrupt tells that the cable is
physically connected, right? And we set TX_ON after HPD, i.e. after the
cable is connected.

And below you say that this depends on which input the user selected
from the TV. So it sounds to me that this problem is not actually
related to the cable connect at all, but it's about enabling the PHY on
OMAP side before the TMDS lines are (somehow) in ready state?

What happens if the HDMI output is enabled properly, but then later,
while the output is still on, the user changes the input of the TV to,
say, AV1? And then, later, back to HDMI?

> >> > In the worst case the above causes a 7.5ms busy loop in an interrupt
> >> > handler. That's not very good thing. Why is there a need for the loop?
> >> I agree looping in interrupt context is bad.. That was the reason i
> >> had a threaded irq handler and i case even here it is the threaded irq
> >> handler i dont think it is happening in irq context.
> >
> > We should nevertheless avoid 7.5ms busyloop if at all possible. Is there
> > an estimate of how long it takes for the PHY to connect? I think we
> > could just start a delayed work from the interrupt, and do the check
> > after a short delay. I don't think the user cares if the connect happens
> > after 1ms or 10ms from the moment the cable is plugged in =).
> >
> Actually by using the PHY_CONNECT interrupt we can avoid the wait.
> If the TV channel is the same as the HDMI port connected to then there
> is hardly any delay between HPD and TMDS lines. Problem is when user
> connects to HDMI but is running  say AV1 /AV2  on TV.
> In such case we will see that HPD is high but TMDS lines are low, So
> there is no guarantee it will be done in certain period, as it is user
> dependent.
> So i am more inclined to have a interrupt based way, On receiving HPD
> in the threaded irq handler we can wait_on_completion of PHY_CONNECT
> which will put the thread in sleep until TMDS lines are high.
> I shall make that patch test out and post it soon.

Is there any reason to wait? Why not just enable the PHY_CONNECT
interrupt in HPD interrupt, perhaps set a flag, and exit. Then at the
PHY_CONNECT interrupt set TX_ON.

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]