[PATCH v3] imx-drm: Add mx6 hdmi transmitter support

[PATCH v3] imx-drm: Add mx6 hdmi transmitter support

[Greg KH]

On Mon, Oct 28, 2013 at 06:26:49PM -0200, Fabio Estevam wrote:
> From: Fabio Estevam <fabio.estevam@freescale.com>
> 
> This is based on the initial work done by Sascha Hauer and Tony Prisk.

What is "this"?

And why is new support being added to this driver instead of working to
get it out of staging?

I thought we went through this already...

greg k-h

[PATCH v3] imx-drm: Add mx6 hdmi transmitter support

[Russell King - ARM Linux]

On Mon, Oct 28, 2013 at 06:26:49PM -0200, Fabio Estevam wrote:
> From: Fabio Estevam <fabio.estevam@freescale.com>
> 
> This is based on the initial work done by Sascha Hauer and Tony Prisk.

It looks like you've also taken some of the suggestions I've made too -
like the voltage drive and symbol transmit control settings.

It would be nice to identify the changes a bit better so that those of
us who are testing your drivers (and have patches on top of them to make
things work) know what's changed.

[PATCH v3] imx-drm: Add mx6 hdmi transmitter support

[Fabio Estevam]

On Mon, Oct 28, 2013 at 7:19 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Mon, Oct 28, 2013 at 06:26:49PM -0200, Fabio Estevam wrote:
>> From: Fabio Estevam <fabio.estevam@freescale.com>
>>
>> This is based on the initial work done by Sascha Hauer and Tony Prisk.
>
> It looks like you've also taken some of the suggestions I've made too -
> like the voltage drive and symbol transmit control settings.
>
> It would be nice to identify the changes a bit better so that those of
> us who are testing your drivers (and have patches on top of them to make
> things work) know what's changed.

Yes, I have been out the two past weeks and tried to capture all of
your inputs concerning the HDMI driver.

Today I was able to get access to a wandboard solo and the HDMI image
does look different compared to a wandboard quad board:

- The Linux logo penguins are steady on mx6q, but on mx6solo the
contour lines look like they flicker.
- The kernel log text appears in good white colour with mx6q, but not
so white on mx6solo (tending to green).

Even the U-boot HDMI splash screen is not very steady on mx6solo.

I will try to look at these mx6 solo HDMI issues.

Regards,

Fabio Estevam

[PATCH v3] imx-drm: Add mx6 hdmi transmitter support

[Fabio Estevam]

On Tue, Oct 29, 2013 at 1:00 PM, Fabio Estevam <festevam@gmail.com> wrote:

> Today I was able to get access to a wandboard solo and the HDMI image
> does look different compared to a wandboard quad board:
>
> - The Linux logo penguins are steady on mx6q, but on mx6solo the
> contour lines look like they flicker.
> - The kernel log text appears in good white colour with mx6q, but not
> so white on mx6solo (tending to green).
>
> Even the U-boot HDMI splash screen is not very steady on mx6solo.
>
> I will try to look at these mx6 solo HDMI issues.

With the change below I am able to get a good HDMI quality image on
mx6solo (and mx6quad as well):

--- a/drivers/staging/imx-drm/ipuv3-crtc.c
+++ b/drivers/staging/imx-drm/ipuv3-crtc.c
@@ -238,7 +238,7 @@ static int ipu_crtc_mode_set(struct drm_crtc *crtc,
                sig_cfg.Vsync_pol = 1;

        sig_cfg.enable_pol = 1;
-       sig_cfg.clk_pol = 0;
+       sig_cfg.clk_pol = 1;
        sig_cfg.width = mode->hdisplay;
        sig_cfg.height = mode->vdisplay;
        sig_cfg.pixel_fmt = out_pixel_fmt;

Regards,

Fabio Estevam

[PATCH v3] imx-drm: Add mx6 hdmi transmitter support

[Russell King - ARM Linux]

On Tue, Oct 29, 2013 at 01:45:50PM -0200, Fabio Estevam wrote:
> On Tue, Oct 29, 2013 at 1:00 PM, Fabio Estevam <festevam@gmail.com> wrote:
> 
> > Today I was able to get access to a wandboard solo and the HDMI image
> > does look different compared to a wandboard quad board:
> >
> > - The Linux logo penguins are steady on mx6q, but on mx6solo the
> > contour lines look like they flicker.
> > - The kernel log text appears in good white colour with mx6q, but not
> > so white on mx6solo (tending to green).
> >
> > Even the U-boot HDMI splash screen is not very steady on mx6solo.
> >
> > I will try to look at these mx6 solo HDMI issues.
> 
> With the change below I am able to get a good HDMI quality image on
> mx6solo (and mx6quad as well):

Yes, I need that too on the Carrier-1 board, as suggested by Sascha.

[PATCH v3] imx-drm: Add mx6 hdmi transmitter support

[Fabio Estevam]

Hi Greg,

On Mon, Oct 28, 2013 at 7:03 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Mon, Oct 28, 2013 at 06:26:49PM -0200, Fabio Estevam wrote:
>> From: Fabio Estevam <fabio.estevam@freescale.com>
>>
>> This is based on the initial work done by Sascha Hauer and Tony Prisk.
>
> What is "this"?

Ok, I will improve the commit log.

> And why is new support being added to this driver instead of working to
> get it out of staging?

Philipp Zabel mentioned that he will submit the imx-drm driver out of staging:
http://www.spinics.net/lists/dri-devel/msg46784.html

> I thought we went through this already...

My understading from your reply at:
http://www.spinics.net/lists/arm-kernel/msg280395.html

,was that the imx-hdmi driver could be moved out of staging (actually
it is not even in staging yet), when it becomes in good shape.

As we had so many issues with the previous imx-hdmi patch version, I
thought it was better to still send v3 to staging so that we could go
through more reviews/testing. Please let me know if I should do it
differently for v4.

Thanks,

Fabio Estevam

[PATCH v3] imx-drm: Add mx6 hdmi transmitter support

[Russell King - ARM Linux]

On Tue, Oct 29, 2013 at 08:01:46PM -0200, Fabio Estevam wrote:
> Hi Greg,
> 
> On Mon, Oct 28, 2013 at 7:03 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> > And why is new support being added to this driver instead of working to
> > get it out of staging?
> 
> Philipp Zabel mentioned that he will submit the imx-drm driver out of staging:
> http://www.spinics.net/lists/dri-devel/msg46784.html

The issues surrounding DT and DRM were given an airing last week at
kernel summit in a session chaired by David Airlie.

The Kernel Summit session was to discuss issues surrounding DT and DRM,
or more specifically DT and componentised subsystems like DRM, ASoC and
V4L2.  Grant Likely has said that he will attempt to summarise some of
the discussion there; unfortunately I'm not aware of anyone making any
real "minutes" of what was discussed - it wasn't a formal session.

I also had discussions with David Airlie and, separately, with the
Pengutronix folk about imx-drm.

It is very clear to me that David is against imx-drm moving into
drivers/gpu/drm - the driver requires an amount of rework in various
places.  So I don't think it will move out of drivers/staging any
time soon.

I have a few other bits to sort out first, but I've volunteered to try
to resolve some of these blocking issues so that imx-drm can move
forward and hopefully out of staging.

To put it another way, moving this driver out of staging is not as
simple as moving the files; it requires real work to address the
issues the subsystem maintainer has with this code.

[PATCH v3] imx-drm: Add mx6 hdmi transmitter support

[Fabio Estevam]

On Tue, Oct 29, 2013 at 9:47 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:

> The issues surrounding DT and DRM were given an airing last week at
> kernel summit in a session chaired by David Airlie.
>
> The Kernel Summit session was to discuss issues surrounding DT and DRM,
> or more specifically DT and componentised subsystems like DRM, ASoC and
> V4L2.  Grant Likely has said that he will attempt to summarise some of
> the discussion there; unfortunately I'm not aware of anyone making any
> real "minutes" of what was discussed - it wasn't a formal session.
>
> I also had discussions with David Airlie and, separately, with the
> Pengutronix folk about imx-drm.
>
> It is very clear to me that David is against imx-drm moving into
> drivers/gpu/drm - the driver requires an amount of rework in various
> places.  So I don't think it will move out of drivers/staging any
> time soon.
>
> I have a few other bits to sort out first, but I've volunteered to try
> to resolve some of these blocking issues so that imx-drm can move
> forward and hopefully out of staging.
>
> To put it another way, moving this driver out of staging is not as
> simple as moving the files; it requires real work to address the
> issues the subsystem maintainer has with this code.

Thanks for your detailed explanation, Russell.

[PATCH v3] imx-drm: Add mx6 hdmi transmitter support

[Russell King - ARM Linux]

On Mon, Oct 28, 2013 at 06:26:49PM -0200, Fabio Estevam wrote:
> +	/*Wait for PHY PLL lock */
> +	msec = 4;
> +	val = hdmi_readb(hdmi, HDMI_PHY_STAT0) & HDMI_PHY_TX_PHY_LOCK;
> +	while (!val) {
> +		udelay(1000);
> +		if (msec-- == 0) {
> +			dev_dbg(hdmi->dev, "PHY PLL not locked\n");
> +			return -EINVAL;
> +		}
> +		val = hdmi_readb(hdmi, HDMI_PHY_STAT0) & HDMI_PHY_TX_PHY_LOCK;
> +	}

This loop is not always sufficient for the PLL to always lock.  It's also
buggy because it behaves like this:

	msec = 4
	read register
	check bit - still one?
		yes - wait 1ms
		msec == 0?
			no - msec = 3
			read register
... msec eventually msec = 0
	read register
	check bit - still one
		yes - wait 1ms
		msec == 0?
			yes - print debug and exit

So the last wait for 1ms performs no real function other than to delay
the inevitable exit.  If we really want to wait 5ms for the condition
we're testing to become true, then do it like this:

	msec = 5;
	do {
		val = hdmi_readb(hdmi, HDMI_PHY_STAT0) & HDMI_PHY_TX_PHY_LOCK;
		if (!val)
			break;
 
		if (msec == 0) {
			dev_err(hdmi->dev, "PHY PLL not locked\n");
			return -EINVAL;
		}
 
		udelay(1000);
		msec--;
	} while (1);

This way, we check for the success condition immediately before checking
for the failure condition, and only waiting after that.

I'd also suggest making it a dev_err() so that it's obvious when it
doesn't lock - the current dev_dbg() remains hidden and causes
failures which don't seem to be visible to the user via the logs.
Hence why this issue has been overlooked...

> +static void hdmi_av_composer(struct imx_hdmi *hdmi,
> +			     const struct drm_display_mode *mode)
> +{
> +	u8 inv_val;
> +	struct hdmi_vmode *vmode = &hdmi->hdmi_data.video_mode;
> +	int hblank, vblank, h_de_hs, v_de_vs, hsync_len, vsync_len;
> +
> +	vmode->mhsyncpolarity = !!(mode->flags & DRM_MODE_FLAG_PHSYNC);
> +	vmode->mvsyncpolarity = !!(mode->flags & DRM_MODE_FLAG_PVSYNC);
> +	vmode->minterlaced = !!(mode->flags & DRM_MODE_FLAG_INTERLACE);
> +	vmode->mpixelclock = mode->htotal * mode->vtotal *
> +			     drm_mode_vrefresh(mode);

We can do better here:

	vmode->mpixelclock = mode->clock * 1000;

as the above is not quite correct when considering all the
possibilities.  Since we're given the pixel clock in the mode structure,
we might as well make use it.

> +	/* Set up VSYNC active edge delay (in pixel clks) */
> +	vsync_len = mode->vsync_end - mode->vsync_start;
> +	hdmi_writeb(hdmi, vsync_len, HDMI_FC_VSYNCINWIDTH);

Commentry - vsync len is in lines not pixel clocks.

Other than that, it seems to work here on the Carrier-1.  Thanks.

[PATCH v3] imx-drm: Add mx6 hdmi transmitter support

[Russell King - ARM Linux]

On Wed, Oct 30, 2013 at 07:36:22PM +0000, Russell King - ARM Linux wrote:
> Other than that, it seems to work here on the Carrier-1.  Thanks.

Here's the remainder of my changes to the HDMI driver which are not part
of your patch.  I've dropped driverdevel and gregkh because this is
really discussion between us at the moment.

[PATCH 1/6] imx-drm: hdmi: fix PLL lock wait

[Russell King]

Enabling the debug for PLL lock shows that this times out almost every
time.  Having such an event at debug level is probably a bug in itself
because it hides this fact.

Waiting 5ms seems to allow it to lock.  Also, adjust the loop so that
we check for success before checking whether we've timed out: we don't
want to wait and then fail without first checking whether we locked.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/staging/imx-drm/imx-hdmi.c |   20 ++++++++++++--------
 1 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/imx-drm/imx-hdmi.c b/drivers/staging/imx-drm/imx-hdmi.c
index 2022635fffdf..ff00759b4d6f 100644
--- a/drivers/staging/imx-drm/imx-hdmi.c
+++ b/drivers/staging/imx-drm/imx-hdmi.c
@@ -1032,16 +1032,20 @@ static int hdmi_phy_configure(struct imx_hdmi *hdmi, unsigned char prep,
 	imx_hdmi_phy_gen2_pddq(hdmi, 0);
 
 	/*Wait for PHY PLL lock */
-	msec = 4;
-	val = hdmi_readb(hdmi, HDMI_PHY_STAT0) & HDMI_PHY_TX_PHY_LOCK;
-	while (!val) {
-		udelay(1000);
-		if (msec-- == 0) {
-			dev_dbg(hdmi->dev, "PHY PLL not locked\n");
+	msec = 5;
+	do {
+		val = hdmi_readb(hdmi, HDMI_PHY_STAT0) & HDMI_PHY_TX_PHY_LOCK;
+		if (!val)
+			break;
+
+		if (msec == 0) {
+			dev_err(hdmi->dev, "PHY PLL not locked\n");
 			return -EINVAL;
 		}
-		val = hdmi_readb(hdmi, HDMI_PHY_STAT0) & HDMI_PHY_TX_PHY_LOCK;
-	}
+
+		udelay(1000);
+		msec--;
+	} while (1);
 
 	return 0;
 }
-- 
1.7.4.4

[PATCH 2/6] imx-drm: hdmi: fix pixel clock

[Russell King]

The correct pixel clock can be found in the drm_display_mode structure.
Use this rather than trying to calculate it from the h/v total and the
refresh rate, which can be inaccurate.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/staging/imx-drm/imx-hdmi.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/imx-drm/imx-hdmi.c b/drivers/staging/imx-drm/imx-hdmi.c
index ff00759b4d6f..fe3a6779b620 100644
--- a/drivers/staging/imx-drm/imx-hdmi.c
+++ b/drivers/staging/imx-drm/imx-hdmi.c
@@ -1206,8 +1206,7 @@ static void hdmi_av_composer(struct imx_hdmi *hdmi,
 	vmode->mhsyncpolarity = !!(mode->flags & DRM_MODE_FLAG_PHSYNC);
 	vmode->mvsyncpolarity = !!(mode->flags & DRM_MODE_FLAG_PVSYNC);
 	vmode->minterlaced = !!(mode->flags & DRM_MODE_FLAG_INTERLACE);
-	vmode->mpixelclock = mode->htotal * mode->vtotal *
-			     drm_mode_vrefresh(mode);
+	vmode->mpixelclock = mode->clock * 1000;
 
 	dev_dbg(hdmi->dev, "final pixclk = %d\n", vmode->mpixelclock);
 
-- 
1.7.4.4

[PATCH 3/6] imx-drm: hdmi: fix wrong comment

[Russell King]

The vsync active edge delay is in lines, not pixel clocks.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/staging/imx-drm/imx-hdmi.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/staging/imx-drm/imx-hdmi.c b/drivers/staging/imx-drm/imx-hdmi.c
index fe3a6779b620..0cba5d4a9966 100644
--- a/drivers/staging/imx-drm/imx-hdmi.c
+++ b/drivers/staging/imx-drm/imx-hdmi.c
@@ -1275,7 +1275,7 @@ static void hdmi_av_composer(struct imx_hdmi *hdmi,
 	hdmi_writeb(hdmi, hsync_len >> 8, HDMI_FC_HSYNCINWIDTH1);
 	hdmi_writeb(hdmi, hsync_len, HDMI_FC_HSYNCINWIDTH0);
 
-	/* Set up VSYNC active edge delay (in pixel clks) */
+	/* Set up VSYNC active edge delay (in lines) */
 	vsync_len = mode->vsync_end - mode->vsync_start;
 	hdmi_writeb(hdmi, vsync_len, HDMI_FC_VSYNCINWIDTH);
 }
-- 
1.7.4.4

[PATCH 4/6] imx-drm: hdmi: get rid of pointless fb_reg

[Russell King]

fb_reg provides no real benefit to the driver: imx_hdmi_fb_registered()
will never be called multiple times.  Let's get rid of this.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/staging/imx-drm/imx-hdmi.c |    6 ------
 1 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/imx-drm/imx-hdmi.c b/drivers/staging/imx-drm/imx-hdmi.c
index 0cba5d4a9966..267d3c9114c6 100644
--- a/drivers/staging/imx-drm/imx-hdmi.c
+++ b/drivers/staging/imx-drm/imx-hdmi.c
@@ -130,7 +130,6 @@ struct imx_hdmi {
 	int vic;
 
 	u8 edid[HDMI_EDID_LEN];
-	bool fb_reg;
 	bool cable_plugin;
 
 	bool phy_enabled;
@@ -1451,9 +1450,6 @@ static int imx_hdmi_fb_registered(struct imx_hdmi *hdmi)
 {
 	int ret;
 
-	if (hdmi->fb_reg)
-		return 0;
-
 	ret = clk_prepare_enable(hdmi->iahb_clk);
 	if (ret)
 		return ret;
@@ -1474,8 +1470,6 @@ static int imx_hdmi_fb_registered(struct imx_hdmi *hdmi)
 	/* Unmute interrupts */
 	hdmi_writeb(hdmi, ~HDMI_IH_PHY_STAT0_HPD, HDMI_IH_MUTE_PHY_STAT0);
 
-	hdmi->fb_reg = true;
-
 	clk_disable_unprepare(hdmi->iahb_clk);
 
 	return 0;
-- 
1.7.4.4

[PATCH 5/6] imx-drm: hdmi: get rid of clk manipulations in imx_hdmi_fb_registered()

[Russell King]

The clock manipulations do nothing for us: the clock is already enabled
by the only caller (imx_hdmi_platform_probe()).  Get rid of these and
simplify the code.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/staging/imx-drm/imx-hdmi.c |    8 --------
 1 files changed, 0 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/imx-drm/imx-hdmi.c b/drivers/staging/imx-drm/imx-hdmi.c
index 267d3c9114c6..1eb12c57aa3e 100644
--- a/drivers/staging/imx-drm/imx-hdmi.c
+++ b/drivers/staging/imx-drm/imx-hdmi.c
@@ -1448,12 +1448,6 @@ static int imx_hdmi_setup(struct imx_hdmi *hdmi, struct drm_display_mode *mode)
 /* Wait until we are registered to enable interrupts */
 static int imx_hdmi_fb_registered(struct imx_hdmi *hdmi)
 {
-	int ret;
-
-	ret = clk_prepare_enable(hdmi->iahb_clk);
-	if (ret)
-		return ret;
-
 	hdmi_writeb(hdmi, HDMI_PHY_I2CM_INT_ADDR_DONE_POL,
 		    HDMI_PHY_I2CM_INT_ADDR);
 
@@ -1470,8 +1464,6 @@ static int imx_hdmi_fb_registered(struct imx_hdmi *hdmi)
 	/* Unmute interrupts */
 	hdmi_writeb(hdmi, ~HDMI_IH_PHY_STAT0_HPD, HDMI_IH_MUTE_PHY_STAT0);
 
-	clk_disable_unprepare(hdmi->iahb_clk);
-
 	return 0;
 }
 
-- 
1.7.4.4

[PATCH 6/6] imx-drm: hdmi: minor cleanups

[Russell King]

Some minor cleanups to the HDMI driver.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/staging/imx-drm/imx-hdmi.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/imx-drm/imx-hdmi.c b/drivers/staging/imx-drm/imx-hdmi.c
index 1eb12c57aa3e..ee0fceb7b5b2 100644
--- a/drivers/staging/imx-drm/imx-hdmi.c
+++ b/drivers/staging/imx-drm/imx-hdmi.c
@@ -19,7 +19,6 @@
 #include <linux/regmap.h>
 #include <linux/mfd/syscon.h>
 #include <linux/mfd/syscon/imx6q-iomuxc-gpr.h>
-
 #include <linux/of_device.h>
 
 #include <drm/drmP.h>
@@ -37,7 +36,7 @@
 #define YCBCR444		1
 #define YCBCR422_16BITS		2
 #define YCBCR422_8BITS		3
-#define	XVYCC444		4
+#define XVYCC444		4
 
 enum hdmi_datamap {
 	RGB444_8B = 0x01,
@@ -1762,8 +1761,8 @@ static int imx_hdmi_platform_probe(struct platform_device *pdev)
 	struct device_node *np = pdev->dev.of_node;
 	struct device_node *ddc_node;
 	struct imx_hdmi *hdmi;
-	int ret, irq;
 	struct resource *iores;
+	int ret, irq;
 
 	hdmi = devm_kzalloc(&pdev->dev, sizeof(*hdmi), GFP_KERNEL);
 	if (!hdmi)
-- 
1.7.4.4