Message ID | 20140610151306.GZ23430@n2100.arm.linux.org.uk (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Tue, Jun 10, 2014 at 04:13:06PM +0100, Russell King - ARM Linux wrote: > where 'M' is the IPU DI clock muxer. However, we're currently setting > this up as: > > LM --+---------------- LDB serial > `- /7 -+-------- LDB DI clock > IPM --- /N ---- IM --- IPU DI clock > > and hoping that the LDB and IPU DI clocks are appropriately synchronised. I've just found that we do indeed do this - but there's nothing to switch the configuration back when the LDB is no longer using a particular DI. Also, I'm having a hard time working out why we have the LDB being given all sorts of clocks... LM --+----------------- LDB serial (clks 33, aka di0_pll) `- /7 -+--------- LDB DI clock (clks 135, aka di0) `- IM --- IPU DI clock (clks 39, aka di0_sel) The LDB is given all of these to play with, including reprogramming the IM, and there's nothing which ever programs IM to anything but the LDB DI clock once it's set there. Not only does this feel horribly unclean from the DT perspective, but it's also a horrid violation of reasonable layering. What if we wanted to fix this by adding control of di0_sel to the HDMI interface too? We then need to list yet again the IPU DI clock and the desired input clock there, and make the imx-hdmi code aware of that. Wouldn't it be better to have the ipuv3-crtc, or even the IPU DI code be in control of its external clock mux, and request the IPU DI code to select a particular input clock? In other words, have one central place where the IPU DI clock is controlled, rather than ending up with it spread through lots of different sub-drivers?
On Tue, Jun 10, 2014 at 05:14:21PM +0100, Russell King - ARM Linux wrote: > On Tue, Jun 10, 2014 at 04:13:06PM +0100, Russell King - ARM Linux wrote: > > where 'M' is the IPU DI clock muxer. However, we're currently setting > > this up as: > > > > LM --+---------------- LDB serial > > `- /7 -+-------- LDB DI clock > > IPM --- /N ---- IM --- IPU DI clock > > > > and hoping that the LDB and IPU DI clocks are appropriately synchronised. > > I've just found that we do indeed do this - but there's nothing to > switch the configuration back when the LDB is no longer using a > particular DI. > > Also, I'm having a hard time working out why we have the LDB being > given all sorts of clocks... > > LM --+----------------- LDB serial (clks 33, aka di0_pll) > `- /7 -+--------- LDB DI clock (clks 135, aka di0) > `- IM --- IPU DI clock (clks 39, aka di0_sel) > > The LDB is given all of these to play with, including reprogramming > the IM, and there's nothing which ever programs IM to anything but > the LDB DI clock once it's set there. *Sigh*... is the clock tree represented in Linux even correct? --|\ --| | --| |------------------+----------------------------------------- --| | ^ ldb_di0_sel | --|/ (clks 33) | `-- /3.5 ---- /2 ------------------ G -+-- ^ ^ ldb_di0_podf | ^ ldb_di0 ldb_di0_div_3_5 | .----------------------' | '------|\ (ldb_di1)------------| | (ipp_di0)------------| |--------- G ---- (ipp_di1)------------| | ^ ^ ipu1_di0 (ipu1_di0_pre)------------|/ ipu1_di0_sel This diagram is drawn from the code in clk-imx6.c, and it does not agree with what is in the SoC manuals - this is the representation redrawn from the manuals: --|\ --| | --| |------------------+---------------------------------- G ---- --| | ^ ldb_di0_sel | ^ ldb serial --|/ (clks 33) | `-- /3.5 ---- /2 -----------------+------- ^ ^ ldb_di0_podf | ^ ldb di ldb_di0_div_3_5 | .-----------------' | '------|\ (ldb_di1)------------| | (ipp_di0)------------| |--------- G ---- (ipp_di1)------------| | ^ ^ ipu1_di0 (ipu1_di0_pre)------------|/ ipu1_di0_sel The difference is, there is no clock gate between the LDB DI clock and the /7 divider, but there is a clock gate on the LDB serial clock. In another location, the iMX6QDL manual suggests that it may be more like this: --|\ --| | --| |----------- cg ---+----------------------------------------- --| | ^ ldb_di0_sel | ^ ldb serial --|/ (clks 33) | `-- /3.5 ---- /2 -----------------+------- ^ ^ ldb_di0_podf | ^ ldb di ldb_di0_div_3_5 | .-----------------' | '------|\ (ldb_di1)------------| | (ipp_di0)------------| |---------------- (ipp_di1)------------| | ^ ^ ipu1_di0 (ipu1_di0_pre)-- cg ------|/ ipu1_di0_sel although "cg" is not defined what it is. Another place seems to confirm the above diagram, saying that the "ldb_di0_clk_enable" gating bits controls both "ch_0_serial_clk" (presumably the ldb serial clock) and "di_0_clk_nc" (presumably the ldb di clock. If that's correct "cg" refers to the clock gating via the CCM_CCGR registers, which appear in the CCM clock tree diagram under LPCG. So... I wonder which one of these three is actually the right one...
On Tue, Jun 10, 2014 at 2:04 PM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > This diagram is drawn from the code in clk-imx6.c, and it does not > agree with what is in the SoC manuals - this is the representation > redrawn from the manuals: .... > The difference is, there is no clock gate between the LDB DI clock and > the /7 divider, but there is a clock gate on the LDB serial clock. This is the correct version. "i.MX 6Dual/6Quad Applications Processor Reference Manual - Rev. 1, 04/2013" shows it incorrectly though.
diff --git a/drivers/staging/imx-drm/imx-drm-core.c b/drivers/staging/imx-drm/imx-drm-core.c index 7da0cad27b49..c538c82f8a32 100644 --- a/drivers/staging/imx-drm/imx-drm-core.c +++ b/drivers/staging/imx-drm/imx-drm-core.c @@ -115,8 +115,7 @@ int imx_drm_panel_format_pins(struct drm_encoder *encoder, helper = &imx_crtc->imx_drm_helper_funcs; if (helper->set_interface_pix_fmt) return helper->set_interface_pix_fmt(encoder->crtc, - encoder->encoder_type, interface_pix_fmt, - hsync_pin, vsync_pin); + interface_pix_fmt, hsync_pin, vsync_pin); return 0; } EXPORT_SYMBOL_GPL(imx_drm_panel_format_pins); diff --git a/drivers/staging/imx-drm/imx-drm.h b/drivers/staging/imx-drm/imx-drm.h index a322bac55414..0bf8e5eb76e3 100644 --- a/drivers/staging/imx-drm/imx-drm.h +++ b/drivers/staging/imx-drm/imx-drm.h @@ -17,7 +17,7 @@ int imx_drm_crtc_id(struct imx_drm_crtc *crtc); struct imx_drm_crtc_helper_funcs { int (*enable_vblank)(struct drm_crtc *crtc); void (*disable_vblank)(struct drm_crtc *crtc); - int (*set_interface_pix_fmt)(struct drm_crtc *crtc, u32 encoder_type, + int (*set_interface_pix_fmt)(struct drm_crtc *crtc, u32 pix_fmt, int hsync_pin, int vsync_pin); const struct drm_crtc_helper_funcs *crtc_helper_funcs; const struct drm_crtc_funcs *crtc_funcs; diff --git a/drivers/staging/imx-drm/ipuv3-crtc.c b/drivers/staging/imx-drm/ipuv3-crtc.c index 47bec5e17358..796e9bef15f0 100644 --- a/drivers/staging/imx-drm/ipuv3-crtc.c +++ b/drivers/staging/imx-drm/ipuv3-crtc.c @@ -51,7 +51,6 @@ struct ipu_crtc { struct drm_framebuffer *newfb; int irq; u32 interface_pix_fmt; - unsigned long di_clkflags; int di_hsync_pin; int di_vsync_pin; }; @@ -146,10 +145,13 @@ static int ipu_crtc_mode_set(struct drm_crtc *crtc, int x, int y, struct drm_framebuffer *old_fb) { + struct drm_device *dev = crtc->dev; + struct drm_encoder *encoder; struct ipu_crtc *ipu_crtc = to_ipu_crtc(crtc); - int ret; struct ipu_di_signal_cfg sig_cfg = {}; + unsigned long encoder_types = 0; u32 out_pixel_fmt; + int ret; dev_dbg(ipu_crtc->dev, "%s: mode->hdisplay: %d\n", __func__, mode->hdisplay); @@ -165,6 +167,24 @@ static int ipu_crtc_mode_set(struct drm_crtc *crtc, if (mode->flags & DRM_MODE_FLAG_PVSYNC) sig_cfg.Vsync_pol = 1; + list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) + if (encoder->crtc == crtc) + encoder_types |= BIT(encoder->encoder_type); + + dev_dbg(ipu_crtc->dev, "%s: attached to encoder types 0x%x\n", + __func__, encoder_types); + + /* + * If we have DAC, TVDAC or LDB, then we need the IPU DI clock + * to be the same as the LDB DI clock. + */ + if (encoder_types & (BIT(DRM_MODE_ENCODER_DAC) | + BIT(DRM_MODE_ENCODER_TVDAC) | + BIT(DRM_MODE_ENCODER_LVDS))) + sig_cfg.clkflags = IPU_DI_CLKMODE_SYNC | IPU_DI_CLKMODE_EXT; + else + sig_cfg.clkflags = 0; + sig_cfg.enable_pol = 1; sig_cfg.clk_pol = 0; sig_cfg.width = mode->hdisplay; @@ -178,7 +198,6 @@ static int ipu_crtc_mode_set(struct drm_crtc *crtc, sig_cfg.v_sync_width = mode->vsync_end - mode->vsync_start; sig_cfg.v_end_width = mode->vsync_start - mode->vdisplay; sig_cfg.pixelclock = mode->clock * 1000; - sig_cfg.clkflags = ipu_crtc->di_clkflags; sig_cfg.v_to_h_sync = 0; @@ -277,7 +296,7 @@ static void ipu_disable_vblank(struct drm_crtc *crtc) ipu_crtc->newfb = NULL; } -static int ipu_set_interface_pix_fmt(struct drm_crtc *crtc, u32 encoder_type, +static int ipu_set_interface_pix_fmt(struct drm_crtc *crtc, u32 pixfmt, int hsync_pin, int vsync_pin) { struct ipu_crtc *ipu_crtc = to_ipu_crtc(crtc); @@ -286,19 +305,6 @@ static int ipu_set_interface_pix_fmt(struct drm_crtc *crtc, u32 encoder_type, ipu_crtc->di_hsync_pin = hsync_pin; ipu_crtc->di_vsync_pin = vsync_pin; - switch (encoder_type) { - case DRM_MODE_ENCODER_DAC: - case DRM_MODE_ENCODER_TVDAC: - case DRM_MODE_ENCODER_LVDS: - ipu_crtc->di_clkflags = IPU_DI_CLKMODE_SYNC | - IPU_DI_CLKMODE_EXT; - break; - case DRM_MODE_ENCODER_TMDS: - case DRM_MODE_ENCODER_NONE: - ipu_crtc->di_clkflags = 0; - break; - } - return 0; }