[PATCH 0/3] OMAPDSS: HDMI: Interrupt and PHY state handling support

[PATCH 0/3] OMAPDSS: HDMI: Interrupt and PHY state handling support

[mythripk]

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

Add support for handling the DSS_HDMI interrupt in HDMI, A line that serves to
notify HDMI of status change in PHY, PLL and CORE based on the registration.
Also logic to support enabling of the PHY in TX_ON state only when a PHY_CONNECT
which would make sure that TMDS lines are high before putting it in TX_ON state.

Mythri P K (3):
  OMAPDSS: HDMI: support for interrupt enabling
  OMAPDSS: HDMI: Add support for DSS_HDMI Interrupt
  OMAPDSS: HDMI: wait for TMDS to be high before putting phy in TX_ON

 drivers/video/omap2/dss/dss_features.c    |    1 +
 drivers/video/omap2/dss/hdmi.c            |   21 +++++
 drivers/video/omap2/dss/ti_hdmi.h         |   20 +++++
 drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c |  117 +++++++++++++++++++++++++++-
 drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h |    3 +
 5 files changed, 157 insertions(+), 5 deletions(-)

-- 
1.7.5.4

[PATCH 1/3] OMAPDSS: HDMI: support for interrupt enabling

[mythripk]

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

Add function to enable or clear interrupts in the HDMI wrapper.

Signed-off-by: Mythri P K <mythripk@ti.com>
---
 drivers/video/omap2/dss/ti_hdmi.h         |   16 ++++++++++
 drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c |   45 +++++++++++++++++++++++++++++
 drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h |    1 +
 3 files changed, 62 insertions(+), 0 deletions(-)

diff --git a/drivers/video/omap2/dss/ti_hdmi.h b/drivers/video/omap2/dss/ti_hdmi.h
index 1f58b84..6d65b3b 100644
--- a/drivers/video/omap2/dss/ti_hdmi.h
+++ b/drivers/video/omap2/dss/ti_hdmi.h
@@ -80,6 +80,22 @@ struct hdmi_pll_info {
 	enum hdmi_clk_refsel refsel;
 };
 
+struct hdmi_irq_vector {
+	u8	pll_recal;
+	u8	pll_unlock;
+	u8	pll_lock;
+	u8	phy_disconnect;
+	u8	phy_connect;
+	u8	phy_short_5v;
+	u8	video_end_fr;
+	u8	video_vsync;
+	u8	fifo_sample_req;
+	u8	fifo_overflow;
+	u8	fifo_underflow;
+	u8	ocp_timeout;
+	u8	core;
+};
+
 struct ti_hdmi_ip_ops {
 
 	void (*video_configure)(struct hdmi_ip_data *ip_data);
diff --git a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
index bfe6fe6..5272f49 100644
--- a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
+++ b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
@@ -224,6 +224,51 @@ void ti_hdmi_4xxx_pll_disable(struct hdmi_ip_data *ip_data)
 	hdmi_set_pll_pwr(ip_data, HDMI_PLLPWRCMD_ALLOFF);
 }
 
+static void hdmi_wp_irq_init(struct hdmi_irq_vector *irq_enable)
+{
+	irq_enable->pll_recal = 0;
+	irq_enable->pll_unlock = 0;
+	irq_enable->pll_lock = 0;
+	irq_enable->phy_disconnect = 0;
+	irq_enable->phy_connect = 0;
+	irq_enable->phy_short_5v = 0;
+	irq_enable->video_end_fr = 0;
+	irq_enable->video_vsync = 0;
+	irq_enable->fifo_sample_req = 0;
+	irq_enable->fifo_overflow = 0;
+	irq_enable->fifo_underflow = 0;
+	irq_enable->ocp_timeout = 0;
+	irq_enable->core = 0;
+}
+
+static void hdmi_wp_clr_irq(struct hdmi_ip_data *ip_data)
+{
+	hdmi_write_reg(hdmi_wp_base(ip_data),
+			HDMI_WP_IRQENABLE_CLR, 0XFFFFFFFF);
+}
+
+static void hdmi_wp_irq_enable(struct hdmi_ip_data *ip_data,
+				struct hdmi_irq_vector *irq_enable)
+{
+	u32 r = 0;
+
+	r = ((irq_enable->pll_recal << 31) |
+		(irq_enable->pll_unlock << 30) |
+		(irq_enable->pll_lock << 29) |
+		(irq_enable->phy_disconnect << 26) |
+		(irq_enable->phy_connect << 25) |
+		(irq_enable->phy_short_5v << 24) |
+		(irq_enable->video_end_fr << 17) |
+		(irq_enable->video_vsync << 16) |
+		(irq_enable->fifo_sample_req << 10) |
+		(irq_enable->fifo_overflow << 9) |
+		(irq_enable->fifo_underflow << 8) |
+		(irq_enable->ocp_timeout << 4) |
+		(irq_enable->core << 0));
+
+	hdmi_write_reg(hdmi_wp_base(ip_data), HDMI_WP_IRQENABLE_SET, r);
+}
+
 static int hdmi_check_hpd_state(struct hdmi_ip_data *ip_data)
 {
 	unsigned long flags;
diff --git a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h
index a14d1a0..3090e81 100644
--- a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h
+++ b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h
@@ -38,6 +38,7 @@
 #define HDMI_WP_IRQSTATUS			0x28
 #define HDMI_WP_PWR_CTRL			0x40
 #define HDMI_WP_IRQENABLE_SET			0x2C
+#define HDMI_WP_IRQENABLE_CLR			0x30
 #define HDMI_WP_VIDEO_CFG			0x50
 #define HDMI_WP_VIDEO_SIZE			0x60
 #define HDMI_WP_VIDEO_TIMING_H			0x68
-- 
1.7.5.4

[PATCH 2/3] OMAPDSS: HDMI: Add support for DSS_HDMI Interrupt

[mythripk]

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

Add support for DSS_HDMI interrupt handling in HDMI driver
by registering for the same. This is the path for many necessary HDMI
interrupts such as PLL lock/unlock, PHY connect/disconnet, video frame
done etc.

Signed-off-by: Mythri P K <mythripk@ti.com>
---
 drivers/video/omap2/dss/dss_features.c    |    1 +
 drivers/video/omap2/dss/hdmi.c            |   21 +++++++++++++++++++++
 drivers/video/omap2/dss/ti_hdmi.h         |    3 +++
 drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c |   13 +++++++++++++
 4 files changed, 38 insertions(+), 0 deletions(-)

diff --git a/drivers/video/omap2/dss/dss_features.c b/drivers/video/omap2/dss/dss_features.c
index ce14aa6..29c5548 100644
--- a/drivers/video/omap2/dss/dss_features.c
+++ b/drivers/video/omap2/dss/dss_features.c
@@ -566,6 +566,7 @@ static const struct ti_hdmi_ip_ops omap4_hdmi_functions = {
 	.dump_core		=	ti_hdmi_4xxx_core_dump,
 	.dump_pll		=	ti_hdmi_4xxx_pll_dump,
 	.dump_phy		=	ti_hdmi_4xxx_phy_dump,
+	.irq_handle		=	ti_hdmi_4xxx_intr_handler,
 #if defined(CONFIG_SND_OMAP_SOC_OMAP4_HDMI) || \
 	defined(CONFIG_SND_OMAP_SOC_OMAP4_HDMI_MODULE)
 	.audio_enable		=       ti_hdmi_4xxx_wp_audio_enable,
diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c
index c4b4f69..14e90b5 100644
--- a/drivers/video/omap2/dss/hdmi.c
+++ b/drivers/video/omap2/dss/hdmi.c
@@ -67,6 +67,8 @@ static struct {
 	struct platform_device *pdev;
 	struct hdmi_ip_data ip_data;
 
+	int irq;
+	spinlock_t irq_lock;
 	struct clk *sys_clk;
 } hdmi;
 
@@ -790,6 +792,19 @@ static void hdmi_put_clocks(void)
 		clk_put(hdmi.sys_clk);
 }
 
+static irqreturn_t hdmi_irq_handler(int irq, void *arg)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&hdmi.irq_lock, flags);
+
+	hdmi.ip_data.ops->irq_handle(&hdmi.ip_data);
+
+	spin_unlock_irqrestore(&hdmi.irq_lock, flags);
+
+	return IRQ_HANDLED;
+}
+
 /* HDMI HW IP initialisation */
 static int omapdss_hdmihw_probe(struct platform_device *pdev)
 {
@@ -823,6 +838,10 @@ static int omapdss_hdmihw_probe(struct platform_device *pdev)
 
 	pm_runtime_enable(&pdev->dev);
 
+	hdmi.irq = platform_get_irq(pdev, 0);
+	r = request_threaded_irq(hdmi.irq, NULL, hdmi_irq_handler, 0,
+				"OMAP HDMI", (void *)0);
+
 	hdmi.ip_data.core_sys_offset = HDMI_CORE_SYS;
 	hdmi.ip_data.core_av_offset = HDMI_CORE_AV;
 	hdmi.ip_data.pll_offset = HDMI_PLLCTRL;
@@ -853,6 +872,8 @@ static int omapdss_hdmihw_remove(struct platform_device *pdev)
 	snd_soc_unregister_codec(&pdev->dev);
 #endif
 
+	free_irq(hdmi.irq, NULL);
+
 	pm_runtime_disable(&pdev->dev);
 
 	hdmi_put_clocks();
diff --git a/drivers/video/omap2/dss/ti_hdmi.h b/drivers/video/omap2/dss/ti_hdmi.h
index 6d65b3b..5e7e0da 100644
--- a/drivers/video/omap2/dss/ti_hdmi.h
+++ b/drivers/video/omap2/dss/ti_hdmi.h
@@ -122,6 +122,8 @@ struct ti_hdmi_ip_ops {
 
 	void (*dump_phy)(struct hdmi_ip_data *ip_data, struct seq_file *s);
 
+	void (*irq_handle)(struct hdmi_ip_data *ip_data);
+
 #if defined(CONFIG_SND_OMAP_SOC_OMAP4_HDMI) || \
 	defined(CONFIG_SND_OMAP_SOC_OMAP4_HDMI_MODULE)
 	void (*audio_enable)(struct hdmi_ip_data *ip_data, bool start);
@@ -197,6 +199,7 @@ void ti_hdmi_4xxx_wp_dump(struct hdmi_ip_data *ip_data, struct seq_file *s);
 void ti_hdmi_4xxx_pll_dump(struct hdmi_ip_data *ip_data, struct seq_file *s);
 void ti_hdmi_4xxx_core_dump(struct hdmi_ip_data *ip_data, struct seq_file *s);
 void ti_hdmi_4xxx_phy_dump(struct hdmi_ip_data *ip_data, struct seq_file *s);
+void ti_hdmi_4xxx_intr_handler(struct hdmi_ip_data *ip_data);
 #if defined(CONFIG_SND_OMAP_SOC_OMAP4_HDMI) || \
 	defined(CONFIG_SND_OMAP_SOC_OMAP4_HDMI_MODULE)
 void ti_hdmi_4xxx_wp_audio_enable(struct hdmi_ip_data *ip_data, bool enable);
diff --git a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
index 5272f49..31d9927 100644
--- a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
+++ b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
@@ -312,6 +312,19 @@ static irqreturn_t hpd_irq_handler(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
+/* Interrupt handler */
+void ti_hdmi_4xxx_intr_handler(struct hdmi_ip_data *ip_data)
+{
+	u32 val;
+
+	val = hdmi_read_reg(hdmi_wp_base(ip_data), HDMI_WP_IRQSTATUS);
+	/* Ack other interrupts if any */
+	hdmi_write_reg(hdmi_wp_base(ip_data), HDMI_WP_IRQSTATUS, val);
+	/* flush posted write */
+	hdmi_read_reg(hdmi_wp_base(ip_data), HDMI_WP_IRQSTATUS);
+
+}
+
 int ti_hdmi_4xxx_phy_enable(struct hdmi_ip_data *ip_data)
 {
 	u16 r = 0;
-- 
1.7.5.4

[PATCH 3/3] OMAPDSS: HDMI: wait for TMDS to be high before putting

[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.
Thus this patch adds the support based on PHY interrupts.
On getting a HPD it registers for PHY connect/disconnect interrupt,
On recieving those it is made sure 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.h         |    1 +
 drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c |   61 ++++++++++++++++++++++++++---
 drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h |    2 +
 3 files changed, 58 insertions(+), 6 deletions(-)

diff --git a/drivers/video/omap2/dss/ti_hdmi.h b/drivers/video/omap2/dss/ti_hdmi.h
index 5e7e0da..5051df6 100644
--- a/drivers/video/omap2/dss/ti_hdmi.h
+++ b/drivers/video/omap2/dss/ti_hdmi.h
@@ -186,6 +186,7 @@ struct hdmi_ip_data {
 	/* ti_hdmi_4xxx_ip private data. These should be in a separate struct */
 	int hpd_gpio;
 	bool phy_tx_enabled;
+	bool phy_enabled;
 };
 int ti_hdmi_4xxx_phy_enable(struct hdmi_ip_data *ip_data);
 void ti_hdmi_4xxx_phy_disable(struct hdmi_ip_data *ip_data);
diff --git a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
index 31d9927..a54c811 100644
--- a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
+++ b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
@@ -273,7 +273,8 @@ static int hdmi_check_hpd_state(struct hdmi_ip_data *ip_data)
 {
 	unsigned long flags;
 	bool hpd;
-	int r;
+	int r = 0;
+	struct hdmi_irq_vector irq_enable;
 	/* this should be in ti_hdmi_4xxx_ip private data */
 	static DEFINE_SPINLOCK(phy_tx_lock);
 
@@ -286,11 +287,21 @@ static int hdmi_check_hpd_state(struct hdmi_ip_data *ip_data)
 		return 0;
 	}
 
-	if (hpd)
-		r = hdmi_set_phy_pwr(ip_data, HDMI_PHYPWRCMD_TXON);
-	else
+	hdmi_wp_clr_irq(ip_data);
+	hdmi_wp_irq_init(&irq_enable);
+	if (hpd) {
+		if (ip_data->phy_enabled) {
+			r = hdmi_set_phy_pwr(ip_data, HDMI_PHYPWRCMD_TXON);
+		} else {
+			irq_enable.phy_connect = 1;
+			irq_enable.phy_disconnect = 0;
+		}
+	} else {
 		r = hdmi_set_phy_pwr(ip_data, HDMI_PHYPWRCMD_LDOON);
-
+		irq_enable.phy_connect = 0;
+		irq_enable.phy_disconnect = 0;
+		ip_data->phy_enabled = false;
+	}
 	if (r) {
 		DSSERR("Failed to %s PHY TX power\n",
 				hpd ? "enable" : "disable");
@@ -300,6 +311,7 @@ static int hdmi_check_hpd_state(struct hdmi_ip_data *ip_data)
 	ip_data->phy_tx_enabled = hpd;
 err:
 	spin_unlock_irqrestore(&phy_tx_lock, flags);
+	hdmi_wp_irq_enable(ip_data, &irq_enable);
 	return r;
 }
 
@@ -312,17 +324,54 @@ static irqreturn_t hpd_irq_handler(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
+int hdmi_ti_4xxx_rxdet(struct hdmi_ip_data *ip_data)
+{
+       int tmds_lines =0;
+
+	tmds_lines = hdmi_read_reg(hdmi_phy_base(ip_data),
+					HDMI_TXPHY_PAD_CFG_CTRL);
+
+	return (tmds_lines && 0x7F80);
+}
 /* Interrupt handler */
 void ti_hdmi_4xxx_intr_handler(struct hdmi_ip_data *ip_data)
 {
-	u32 val;
+	u32 val, r = 0;
+	struct hdmi_irq_vector irq_enable;
 
 	val = hdmi_read_reg(hdmi_wp_base(ip_data), HDMI_WP_IRQSTATUS);
+
+	hdmi_wp_clr_irq(ip_data);
+	hdmi_wp_irq_init(&irq_enable);
+
+	if (val & HDMI_WP_IRQSTATUS_PHYCONNECT) {
+		if (ip_data->phy_tx_enabled && hdmi_ti_4xxx_rxdet(ip_data)) {
+			r = hdmi_set_phy_pwr(ip_data, HDMI_PHYPWRCMD_TXON);
+			irq_enable.phy_connect = 0;
+			irq_enable.phy_disconnect = 1;
+			ip_data->phy_enabled = true;
+		}
+	}
+
+	/* We can get connect / disconnect simultaneously due to glitch	*/
+	if (val & HDMI_WP_IRQSTATUS_PHYDISCONNECT) {
+		if (ip_data->phy_tx_enabled && !hdmi_ti_4xxx_rxdet(ip_data)) {
+			r = hdmi_set_phy_pwr(ip_data, HDMI_PHYPWRCMD_LDOON);
+			irq_enable.phy_connect = 1;
+			irq_enable.phy_disconnect = 0;
+			ip_data->phy_enabled = false;
+		}
+	}
+
+	if (r)
+		DSSERR("Failed to set PHY TX power\n");
+
 	/* Ack other interrupts if any */
 	hdmi_write_reg(hdmi_wp_base(ip_data), HDMI_WP_IRQSTATUS, val);
 	/* flush posted write */
 	hdmi_read_reg(hdmi_wp_base(ip_data), HDMI_WP_IRQSTATUS);
 
+	hdmi_wp_irq_enable(ip_data, &irq_enable);
 }
 
 int ti_hdmi_4xxx_phy_enable(struct hdmi_ip_data *ip_data)
diff --git a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h
index 3090e81..38af675 100644
--- a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h
+++ b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h
@@ -48,6 +48,8 @@
 #define HDMI_WP_AUDIO_CFG2			0x84
 #define HDMI_WP_AUDIO_CTRL			0x88
 #define HDMI_WP_AUDIO_DATA			0x8C
+#define HDMI_WP_IRQSTATUS_PHYCONNECT		0x02000000
+#define HDMI_WP_IRQSTATUS_PHYDISCONNECT		0x04000000
 
 /* HDMI IP Core System */
 
-- 
1.7.5.4

Re: [PATCH 0/3] OMAPDSS: HDMI: Interrupt and PHY state handling support

[Tomi Valkeinen]

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

On Tue, 2012-03-20 at 18:45 +0530, mythripk@ti.com wrote:
> From: Mythri P K <mythripk@ti.com>
> 
> Add support for handling the DSS_HDMI interrupt in HDMI, A line that serves to
> notify HDMI of status change in PHY, PLL and CORE based on the registration.
> Also logic to support enabling of the PHY in TX_ON state only when a PHY_CONNECT
> which would make sure that TMDS lines are high before putting it in TX_ON state.
> 
> Mythri P K (3):
>   OMAPDSS: HDMI: support for interrupt enabling
>   OMAPDSS: HDMI: Add support for DSS_HDMI Interrupt
>   OMAPDSS: HDMI: wait for TMDS to be high before putting phy in TX_ON

Sorry, but this patch set is quite bad.

First of all, the subject on your third patch is again cut short. How do
you even do that? The subject seems to be fine above. I've never seen
other people do that, but you somehow manage to do it all the time. So,
please, read the git manual, fix your environment, your way of working,
or whatever is causing this so that we'll never see it again.

Then, the intro text above is rather confusing. Are there words missing
around the part with "PHY_CONNECT"? Please spend more time on the
descriptions, and possibly ask someone to proof-read them so that at
least there aren't words missing and they are somehow understandable.

I'll comment on other things in later mails.

 Tomi


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

Re: [PATCH 1/3] OMAPDSS: HDMI: support for interrupt enabling

[Tomi Valkeinen]

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

On Tue, 2012-03-20 at 18:45 +0530, mythripk@ti.com wrote:
> From: Mythri P K <mythripk@ti.com>
> 
> Add function to enable or clear interrupts in the HDMI wrapper.
> 
> Signed-off-by: Mythri P K <mythripk@ti.com>
> ---
>  drivers/video/omap2/dss/ti_hdmi.h         |   16 ++++++++++
>  drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c |   45 +++++++++++++++++++++++++++++
>  drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h |    1 +
>  3 files changed, 62 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/video/omap2/dss/ti_hdmi.h b/drivers/video/omap2/dss/ti_hdmi.h
> index 1f58b84..6d65b3b 100644
> --- a/drivers/video/omap2/dss/ti_hdmi.h
> +++ b/drivers/video/omap2/dss/ti_hdmi.h
> @@ -80,6 +80,22 @@ struct hdmi_pll_info {
>  	enum hdmi_clk_refsel refsel;
>  };
>  
> +struct hdmi_irq_vector {
> +	u8	pll_recal;
> +	u8	pll_unlock;
> +	u8	pll_lock;
> +	u8	phy_disconnect;
> +	u8	phy_connect;
> +	u8	phy_short_5v;
> +	u8	video_end_fr;
> +	u8	video_vsync;
> +	u8	fifo_sample_req;
> +	u8	fifo_overflow;
> +	u8	fifo_underflow;
> +	u8	ocp_timeout;
> +	u8	core;
> +};

This feels like a very complex way to do a simple thing... Why is this
in ti_hdmi.h anyway? These are omap4 HDMI interrupts, right?

Why don't you just define the interrupts the same way dispc/dsi do:

#define HDMI_IRQ_RECAL (1 << 31)

Then you could have simple functions like hdmi_enable_irq(u32 irqmask)
and call it like:

hdmi_enable_irq(HDMI_IRQ_RECAL | HDMI_IRQ_SOMETHING);

 Tomi


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

Re: [PATCH 2/3] OMAPDSS: HDMI: Add support for DSS_HDMI Interrupt

[Tomi Valkeinen]

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

On Tue, 2012-03-20 at 18:45 +0530, mythripk@ti.com wrote:
> From: Mythri P K <mythripk@ti.com>
> 
> Add support for DSS_HDMI interrupt handling in HDMI driver
> by registering for the same. This is the path for many necessary HDMI
> interrupts such as PLL lock/unlock, PHY connect/disconnet, video frame
> done etc.
> 
> Signed-off-by: Mythri P K <mythripk@ti.com>
> ---
>  drivers/video/omap2/dss/dss_features.c    |    1 +
>  drivers/video/omap2/dss/hdmi.c            |   21 +++++++++++++++++++++
>  drivers/video/omap2/dss/ti_hdmi.h         |    3 +++
>  drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c |   13 +++++++++++++
>  4 files changed, 38 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/video/omap2/dss/dss_features.c b/drivers/video/omap2/dss/dss_features.c
> index ce14aa6..29c5548 100644
> --- a/drivers/video/omap2/dss/dss_features.c
> +++ b/drivers/video/omap2/dss/dss_features.c
> @@ -566,6 +566,7 @@ static const struct ti_hdmi_ip_ops omap4_hdmi_functions = {
>  	.dump_core		=	ti_hdmi_4xxx_core_dump,
>  	.dump_pll		=	ti_hdmi_4xxx_pll_dump,
>  	.dump_phy		=	ti_hdmi_4xxx_phy_dump,
> +	.irq_handle		=	ti_hdmi_4xxx_intr_handler,
>  #if defined(CONFIG_SND_OMAP_SOC_OMAP4_HDMI) || \
>  	defined(CONFIG_SND_OMAP_SOC_OMAP4_HDMI_MODULE)
>  	.audio_enable		=       ti_hdmi_4xxx_wp_audio_enable,
> diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c
> index c4b4f69..14e90b5 100644
> --- a/drivers/video/omap2/dss/hdmi.c
> +++ b/drivers/video/omap2/dss/hdmi.c
> @@ -67,6 +67,8 @@ static struct {
>  	struct platform_device *pdev;
>  	struct hdmi_ip_data ip_data;
>  
> +	int irq;
> +	spinlock_t irq_lock;
>  	struct clk *sys_clk;
>  } hdmi;
>  
> @@ -790,6 +792,19 @@ static void hdmi_put_clocks(void)
>  		clk_put(hdmi.sys_clk);
>  }
>  
> +static irqreturn_t hdmi_irq_handler(int irq, void *arg)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&hdmi.irq_lock, flags);
> +
> +	hdmi.ip_data.ops->irq_handle(&hdmi.ip_data);
> +
> +	spin_unlock_irqrestore(&hdmi.irq_lock, flags);

So what is this spinlock supposed to protect? This is the only place you
use it.

> +
> +	return IRQ_HANDLED;
> +}

I think all this code should be in the ti_hdmi_4xxx_ip.c, as it's
dealing with on omap4 hdmi interrupt.

 Tomi


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

Re: [PATCH 3/3] OMAPDSS: HDMI: wait for TMDS to be high before putting

[Tomi Valkeinen]

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

On Tue, 2012-03-20 at 18:45 +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.
> Thus this patch adds the support based on PHY interrupts.
> On getting a HPD it registers for PHY connect/disconnect interrupt,
> On recieving those it is made sure 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.h         |    1 +
>  drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c |   61 ++++++++++++++++++++++++++---
>  drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h |    2 +
>  3 files changed, 58 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/video/omap2/dss/ti_hdmi.h b/drivers/video/omap2/dss/ti_hdmi.h
> index 5e7e0da..5051df6 100644
> --- a/drivers/video/omap2/dss/ti_hdmi.h
> +++ b/drivers/video/omap2/dss/ti_hdmi.h
> @@ -186,6 +186,7 @@ struct hdmi_ip_data {
>  	/* ti_hdmi_4xxx_ip private data. These should be in a separate struct */
>  	int hpd_gpio;
>  	bool phy_tx_enabled;
> +	bool phy_enabled;
>  };
>  int ti_hdmi_4xxx_phy_enable(struct hdmi_ip_data *ip_data);
>  void ti_hdmi_4xxx_phy_disable(struct hdmi_ip_data *ip_data);
> diff --git a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
> index 31d9927..a54c811 100644
> --- a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
> +++ b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
> @@ -273,7 +273,8 @@ static int hdmi_check_hpd_state(struct hdmi_ip_data *ip_data)
>  {
>  	unsigned long flags;
>  	bool hpd;
> -	int r;
> +	int r = 0;
> +	struct hdmi_irq_vector irq_enable;
>  	/* this should be in ti_hdmi_4xxx_ip private data */
>  	static DEFINE_SPINLOCK(phy_tx_lock);
>  
> @@ -286,11 +287,21 @@ static int hdmi_check_hpd_state(struct hdmi_ip_data *ip_data)
>  		return 0;
>  	}
>  
> -	if (hpd)
> -		r = hdmi_set_phy_pwr(ip_data, HDMI_PHYPWRCMD_TXON);
> -	else
> +	hdmi_wp_clr_irq(ip_data);
> +	hdmi_wp_irq_init(&irq_enable);
> +	if (hpd) {
> +		if (ip_data->phy_enabled) {
> +			r = hdmi_set_phy_pwr(ip_data, HDMI_PHYPWRCMD_TXON);
> +		} else {
> +			irq_enable.phy_connect = 1;
> +			irq_enable.phy_disconnect = 0;
> +		}
> +	} else {
>  		r = hdmi_set_phy_pwr(ip_data, HDMI_PHYPWRCMD_LDOON);
> -
> +		irq_enable.phy_connect = 0;
> +		irq_enable.phy_disconnect = 0;
> +		ip_data->phy_enabled = false;
> +	}

Why do you need this elaborate mechanism where you turn on/off the PHY
CONNECT/DISCONNECT interrupts? Why don't you just enable them when the
HDMI is enabled?

>  	if (r) {
>  		DSSERR("Failed to %s PHY TX power\n",
>  				hpd ? "enable" : "disable");
> @@ -300,6 +311,7 @@ static int hdmi_check_hpd_state(struct hdmi_ip_data *ip_data)
>  	ip_data->phy_tx_enabled = hpd;
>  err:
>  	spin_unlock_irqrestore(&phy_tx_lock, flags);
> +	hdmi_wp_irq_enable(ip_data, &irq_enable);

What happens if the connect irq happened before you enable the
interrupt?

>  	return r;
>  }
>  
> @@ -312,17 +324,54 @@ static irqreturn_t hpd_irq_handler(int irq, void *data)
>  	return IRQ_HANDLED;
>  }
>  
> +int hdmi_ti_4xxx_rxdet(struct hdmi_ip_data *ip_data)
> +{
> +       int tmds_lines =0;
> +
> +	tmds_lines = hdmi_read_reg(hdmi_phy_base(ip_data),
> +					HDMI_TXPHY_PAD_CFG_CTRL);
> +
> +	return (tmds_lines && 0x7F80);

I'm quite sure the line above is not correct, and thus the whole rxdet
system doesn't work properly.

>  /* Interrupt handler */
>  void ti_hdmi_4xxx_intr_handler(struct hdmi_ip_data *ip_data)
>  {
> -	u32 val;
> +	u32 val, r = 0;
> +	struct hdmi_irq_vector irq_enable;
>  
>  	val = hdmi_read_reg(hdmi_wp_base(ip_data), HDMI_WP_IRQSTATUS);
> +
> +	hdmi_wp_clr_irq(ip_data);
> +	hdmi_wp_irq_init(&irq_enable);
> +
> +	if (val & HDMI_WP_IRQSTATUS_PHYCONNECT) {
> +		if (ip_data->phy_tx_enabled && hdmi_ti_4xxx_rxdet(ip_data)) {
> +			r = hdmi_set_phy_pwr(ip_data, HDMI_PHYPWRCMD_TXON);
> +			irq_enable.phy_connect = 0;
> +			irq_enable.phy_disconnect = 1;
> +			ip_data->phy_enabled = true;
> +		}
> +	}
> +
> +	/* We can get connect / disconnect simultaneously due to glitch	*/
> +	if (val & HDMI_WP_IRQSTATUS_PHYDISCONNECT) {
> +		if (ip_data->phy_tx_enabled && !hdmi_ti_4xxx_rxdet(ip_data)) {
> +			r = hdmi_set_phy_pwr(ip_data, HDMI_PHYPWRCMD_LDOON);
> +			irq_enable.phy_connect = 1;
> +			irq_enable.phy_disconnect = 0;
> +			ip_data->phy_enabled = false;
> +		}
> +	}

Instead of spreading this code into two functions, why not handle
everything in one function. You have four states, connected/disconnected
for both HPD and PHY. Just track the states of those, and have one
function decide what to do depending on those states.

> +
> +	if (r)
> +		DSSERR("Failed to set PHY TX power\n");
> +
>  	/* Ack other interrupts if any */
>  	hdmi_write_reg(hdmi_wp_base(ip_data), HDMI_WP_IRQSTATUS, val);
>  	/* flush posted write */
>  	hdmi_read_reg(hdmi_wp_base(ip_data), HDMI_WP_IRQSTATUS);
>  
> +	hdmi_wp_irq_enable(ip_data, &irq_enable);
>  }
>  
>  int ti_hdmi_4xxx_phy_enable(struct hdmi_ip_data *ip_data)
> diff --git a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h
> index 3090e81..38af675 100644
> --- a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h
> +++ b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h
> @@ -48,6 +48,8 @@
>  #define HDMI_WP_AUDIO_CFG2			0x84
>  #define HDMI_WP_AUDIO_CTRL			0x88
>  #define HDMI_WP_AUDIO_DATA			0x8C
> +#define HDMI_WP_IRQSTATUS_PHYCONNECT		0x02000000
> +#define HDMI_WP_IRQSTATUS_PHYDISCONNECT		0x04000000

Use (1 << xx) style there. And you could name them the same way as other
components: HDMI_WP_IRQ_PHYCONNECT. And if you add few, just add them
all.

 Tomi


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