[RFC v1 0/1] imx-drm: match ipu_di_signal_cfg's clk_pol with its observed behaviour.

[RFC v1 0/1] imx-drm: match ipu_di_signal_cfg's clk_pol with its observed behaviour.

[thesven73]

From: Sven Van Asbroeck <svendev@arcx.com>

Request For Comments

We believe that the ipuv3 DI's pixel clock polarity is possibly being
misconfigured by the imx-drm driver.

We used an oscilloscope the observe the actual behaviour on our hardware,
and it is the opposite of what the driver is supposed to configure.

Some confusion could arise because the Freescale documentation does not
speak about data stable on rising or falling edge, but only of clock active
high / active low:

i.MX 6Dual/6Quad Applications Processor Reference Manual, Rev. 3, 07/2015,
page 3298)
  bit 17 DI0 Output Clock's polarity
  This bits define the polarity of the DI0's clock.
    1 The output clock is active high
    0 The output clock is active low

However, the supposedly wrong behaviour has been in place since April 2014,
and no-one has complained so far.
See commit 85de9d17c485c4196f74d45de2206d4802f8a3be

What are we missing ?

Sven Van Asbroeck (1):
  imx-drm: match ipu_di_signal_cfg's clk_pol with its observed
    behaviour.

 drivers/gpu/ipu-v3/ipu-di.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.17.1

[RFC v1 1/1] imx-drm: match ipu_di_signal_cfg's clk_pol with its observed behaviour.

[thesven73]

From: Sven Van Asbroeck <svendev@arcx.com>

We used an oscilloscope to observe the actual polarity of the
DI's pixel clock, and saw the following:

DI_GENERAL bit 17 is SET:
	pixel data is stable on the pixel clock's FALLING edge
DI_GENERAL bit 17 is CLEAR:
	pixel data is stable on the pixel clock's RISING  edge

However, the current driver configures the exact opposite of the
behaviour documented in video/imx-ipu-v3.h:
  unsigned clk_pol:1;     /* true = rising edge */

Fixes: 85de9d17c485c4196f74 ("imx-drm: match ipu_di_signal_cfg's clk_pol with its description.")
Signed-off-by: Sven Van Asbroeck <svendev@arcx.com>
---
 drivers/gpu/ipu-v3/ipu-di.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/ipu-v3/ipu-di.c b/drivers/gpu/ipu-v3/ipu-di.c
index d2f1bd9d3deb..f296fa461875 100644
--- a/drivers/gpu/ipu-v3/ipu-di.c
+++ b/drivers/gpu/ipu-v3/ipu-di.c
@@ -619,7 +619,7 @@ int ipu_di_init_sync_panel(struct ipu_di *di, struct ipu_di_signal_cfg *sig)
 	if (sig->mode.flags & DISPLAY_FLAGS_VSYNC_HIGH)
 		di_gen |= ipu_di_gen_polarity(sig->vsync_pin);
 
-	if (sig->clk_pol)
+	if (!sig->clk_pol)
 		di_gen |= DI_GEN_POLARITY_DISP_CLK;
 
 	ipu_di_write(di, di_gen, DI_GENERAL);
-- 
2.17.1

Re: [RFC v1 1/1] imx-drm: match ipu_di_signal_cfg's clk_pol with its observed behaviour.

[Russell King - ARM Linux]

On Thu, Oct 25, 2018 at 12:17:11PM -0400, thesven73@gmail.com wrote:
> From: Sven Van Asbroeck <svendev@arcx.com>
> 
> We used an oscilloscope to observe the actual polarity of the
> DI's pixel clock, and saw the following:
> 
> DI_GENERAL bit 17 is SET:
> 	pixel data is stable on the pixel clock's FALLING edge
> DI_GENERAL bit 17 is CLEAR:
> 	pixel data is stable on the pixel clock's RISING  edge
> 
> However, the current driver configures the exact opposite of the
> behaviour documented in video/imx-ipu-v3.h:
>   unsigned clk_pol:1;     /* true = rising edge */

The definition in the manual is:

       17           DI0 Output Clock's polarity
  di0_polarity_     This bits define the polarity of the DI0's clock.
    disp_clk        1    The output clock is active high
                    0    The output clock is active low

but what does that mean...

There's the hint in IMX6SDLCEC that when the clock is active high,
it's expected that the panel will latch data on the _falling_ edge:

  IPP_DISP_CLK latches data into the panel on its negative edge (when
  positive polarity is selected). In active mode, IPP_DISP_CLK runs
  continuously.

That seems to imply that when DI_GEN_POLARITY_DISP_CLK is set, it is
expected that the panel latches data on the _falling_ edge, not on
the positive edge.

What this actually means as far as the output signal is not defined
very well beyond what I've quoted above in any of the IMX6 manuals
that I've checked so far.

Now, the interpretation of the comment "/* true = rising edge */"
is ambiguous, because it doesn't state what "rising edge" refers to -
is that referring to the edge that the data changes, or the edge that
the panel is supposed to latch the data.

Given what's in the documentation, I'd opt for it describing the
edge that the output data changes, not the latch edge.  With that
interpretation, the existing code is correct.

In any case, I suspect if we were to change this as per your patch,
we'd end up breaking stuff that works today, thereby causing
regressions.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [RFC v1 0/1] imx-drm: match ipu_di_signal_cfg's clk_pol with its observed behaviour.

[thesven73]

Hi Russell, thanks for looking at the RFC !

> Given what's in the documentation, I'd opt for it describing the
> edge that the output data changes, not the latch edge.  With that
> interpretation, the existing code is correct.

The clock polarity is reversed in Freescale's non-mainline fbdev code,
which we were using previously, contributing to the confusion.

Yes, I think your interpretation is the correct one. Let's double-check.

Suppose we set this bus flag in the panel/mode definition:

drm/drm_connector.h:
/* drive data on pos. edge */
#define DRM_BUS_FLAG_PIXDATA_POSEDGE	(1<<2)

According to your interpretation, this means we're asking the data to change
on the positive edge, and be stable on the negative edge. Correct?

then clk_pol == 1:
drivers/gpu/drm/imx/ipuv3-crtc.c:
	sig_cfg.clk_pol = !!(imx_crtc_state->bus_flags &
			     DRM_BUS_FLAG_PIXDATA_POSEDGE);

then DI_GENERAL bit 17 is set:
drivers/gpu/ipu-v3/ipu-di.c:
	if (sig->clk_pol)
		di_gen |= DI_GEN_POLARITY_DISP_CLK;

and data is stable on the pixel clock's falling edge, as we observed on
the oscilloscope.

All good. Thank you !