Re: [PATCH V5 1/7] imx-drm: imx-hdmi: split imx soc specific code from imx-hdmi

2014-11-10 Thread Zubair Lutfullah Kakakhel

Hi Andy,

A few comments inline.

On 08/11/14 05:28, Andy Yan wrote:
 imx6 and rockchip rk3288 and JZ4780 (Ingenic Xburst/MIPS)
 use the interface compatible Designware HDMI IP, but they
 also have some lightly difference, such as phy pll configuration,
 register width, 4K support, clk useage, and the crtc mux configuration
 is also platform specific.
 
 To reuse the imx hdmi driver, split the platform specific code out
 to dw_hdmi-imx.c.
 
 Signed-off-by: Andy Yan andy@rock-chips.com
 ---
  drivers/staging/imx-drm/Makefile  |   2 +-
  drivers/staging/imx-drm/dw_hdmi-imx.c | 214 
  drivers/staging/imx-drm/imx-hdmi.c| 257 
 --
  drivers/staging/imx-drm/imx-hdmi.h|  43 ++
  4 files changed, 320 insertions(+), 196 deletions(-)
  create mode 100644 drivers/staging/imx-drm/dw_hdmi-imx.c
 
 diff --git a/drivers/staging/imx-drm/Makefile 
 b/drivers/staging/imx-drm/Makefile
 index 582c438..809027d 100644
 --- a/drivers/staging/imx-drm/Makefile
 +++ b/drivers/staging/imx-drm/Makefile
 @@ -9,4 +9,4 @@ obj-$(CONFIG_DRM_IMX_LDB) += imx-ldb.o
  
  imx-ipuv3-crtc-objs  := ipuv3-crtc.o ipuv3-plane.o
  obj-$(CONFIG_DRM_IMX_IPUV3)  += imx-ipuv3-crtc.o
 -obj-$(CONFIG_DRM_IMX_HDMI) += imx-hdmi.o
 +obj-$(CONFIG_DRM_IMX_HDMI) += imx-hdmi.o dw_hdmi-imx.o
 diff --git a/drivers/staging/imx-drm/dw_hdmi-imx.c 
 b/drivers/staging/imx-drm/dw_hdmi-imx.c
 new file mode 100644
 index 000..5422679
 --- /dev/null
 +++ b/drivers/staging/imx-drm/dw_hdmi-imx.c
 @@ -0,0 +1,214 @@
 +/*
 + * This program is free software; you can redistribute it and/or modify
 + * it under the terms of the GNU General Public License version 2 as
 + * published by the Free Software Foundation.
 + */

Please add the old freescale copyrights and a comment on how this file is 
derived from.
And a note on platform specific file for imx hdmi using dwc hdmi drm bridge.

 +#include linux/module.h
 +#include linux/platform_device.h
 +#include linux/mfd/syscon.h
 +#include linux/mfd/syscon/imx6q-iomuxc-gpr.h
 +#include video/imx-ipu-v3.h
 +#include linux/regmap.h
 +#include linux/clk.h
 +
 +#include imx-drm.h
 +#include imx-hdmi.h
 +
 +struct imx_hdmi_priv {
 + struct device *dev;
 + struct clk *isfr_clk;
 + struct clk *iahb_clk;
 + struct regmap *regmap;
 +};
 +
 +static const struct mpll_config imx_mpll_cfg[] = {
 + {
 + 4525, {
 + { 0x01e0, 0x },
 + { 0x21e1, 0x },
 + { 0x41e2, 0x }
 + },
 + }, {
 + 9250, {
 + { 0x0140, 0x0005 },
 + { 0x2141, 0x0005 },
 + { 0x4142, 0x0005 },
 + },
 + }, {
 + 14850, {
 + { 0x00a0, 0x000a },
 + { 0x20a1, 0x000a },
 + { 0x40a2, 0x000a },
 + },
 + }, {
 + ~0UL, {
 + { 0x00a0, 0x000a },
 + { 0x2001, 0x000f },
 + { 0x4002, 0x000f },
 + },
 + }
 +};
 +
 +static const struct curr_ctrl imx_cur_ctr[] = {
 + /*  pixelclk bpp8bpp10   bpp12 */
 + {
 + 5400, { 0x091c, 0x091c, 0x06dc },
 + }, {
 + 5840, { 0x091c, 0x06dc, 0x06dc },
 + }, {
 + 7200, { 0x06dc, 0x06dc, 0x091c },
 + }, {
 + 7425, { 0x06dc, 0x0b5c, 0x091c },
 + }, {
 + 11880, { 0x091c, 0x091c, 0x06dc },
 + }, {
 + 21600, { 0x06dc, 0x0b5c, 0x091c },
 + }
 +};

These pll clocks. Are they completely different?

I assume they are dependent on display resolutions. The old 
ones should remain the same. With new additions for 4K clock
rates e.g.

 +
 +static int imx_hdmi_parse_dt(struct imx_hdmi_priv *hdmi)
 +{
 + struct device_node *np = hdmi-dev-of_node;
 +
 + hdmi-regmap = syscon_regmap_lookup_by_phandle(np, gpr);
 + if (IS_ERR(hdmi-regmap)) {
 + dev_err(hdmi-dev, Unable to get gpr\n);
 + return PTR_ERR(hdmi-regmap);
 + }
 +
 + hdmi-isfr_clk = devm_clk_get(hdmi-dev, isfr);
 + if (IS_ERR(hdmi-isfr_clk)) {
 + dev_err(hdmi-dev, Unable to get HDMI isfr clk\n);
 + return PTR_ERR(hdmi-isfr_clk);
 + }
 +
 + hdmi-iahb_clk = devm_clk_get(hdmi-dev, iahb);
 + if (IS_ERR(hdmi-iahb_clk)) {
 + dev_err(hdmi-dev, Unable to get HDMI iahb clk\n);
 + return PTR_ERR(hdmi-iahb_clk);
 + }
 +

These two clocks. This is how they are structured on the jz4780 as well.

From an earlier patch series

yes, this IP core need to be clocked. But different Soc has different
usage of the clk, on freescale imx platform one clk is used for isfr, one is 
used for iahb,
but on rockchip rk3288, one clk is used for control logic , the another is used 
for hdcp.
I am not sure how are the clocks on jz4780

They might be 

Re: [PATCH V5 1/7] imx-drm: imx-hdmi: split imx soc specific code from imx-hdmi

2014-11-10 Thread Andy Yan

Hi Zubair:
thanks very much for your comments.
On 2014年11月10日 18:51, Zubair Lutfullah Kakakhel wrote:

Hi Andy,

A few comments inline.

On 08/11/14 05:28, Andy Yan wrote:

imx6 and rockchip rk3288 and JZ4780 (Ingenic Xburst/MIPS)
use the interface compatible Designware HDMI IP, but they
also have some lightly difference, such as phy pll configuration,
register width, 4K support, clk useage, and the crtc mux configuration
is also platform specific.

To reuse the imx hdmi driver, split the platform specific code out
to dw_hdmi-imx.c.

Signed-off-by: Andy Yan andy@rock-chips.com
---
  drivers/staging/imx-drm/Makefile  |   2 +-
  drivers/staging/imx-drm/dw_hdmi-imx.c | 214 
  drivers/staging/imx-drm/imx-hdmi.c| 257 --
  drivers/staging/imx-drm/imx-hdmi.h|  43 ++
  4 files changed, 320 insertions(+), 196 deletions(-)
  create mode 100644 drivers/staging/imx-drm/dw_hdmi-imx.c

diff --git a/drivers/staging/imx-drm/Makefile b/drivers/staging/imx-drm/Makefile
index 582c438..809027d 100644
--- a/drivers/staging/imx-drm/Makefile
+++ b/drivers/staging/imx-drm/Makefile
@@ -9,4 +9,4 @@ obj-$(CONFIG_DRM_IMX_LDB) += imx-ldb.o
  
  imx-ipuv3-crtc-objs  := ipuv3-crtc.o ipuv3-plane.o

  obj-$(CONFIG_DRM_IMX_IPUV3)   += imx-ipuv3-crtc.o
-obj-$(CONFIG_DRM_IMX_HDMI) += imx-hdmi.o
+obj-$(CONFIG_DRM_IMX_HDMI) += imx-hdmi.o dw_hdmi-imx.o
diff --git a/drivers/staging/imx-drm/dw_hdmi-imx.c 
b/drivers/staging/imx-drm/dw_hdmi-imx.c
new file mode 100644
index 000..5422679
--- /dev/null
+++ b/drivers/staging/imx-drm/dw_hdmi-imx.c
@@ -0,0 +1,214 @@
+/*
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */

Please add the old freescale copyrights and a comment on how this file is 
derived from.

  ok, I will add it in V7

And a note on platform specific file for imx hdmi using dwc hdmi drm bridge.
   sorry, I am not clear which specific file you means. Would you 
please tell

   me more about the note?

+#include linux/module.h
+#include linux/platform_device.h
+#include linux/mfd/syscon.h
+#include linux/mfd/syscon/imx6q-iomuxc-gpr.h
+#include video/imx-ipu-v3.h
+#include linux/regmap.h
+#include linux/clk.h
+
+#include imx-drm.h
+#include imx-hdmi.h
+
+struct imx_hdmi_priv {
+   struct device *dev;
+   struct clk *isfr_clk;
+   struct clk *iahb_clk;
+   struct regmap *regmap;
+};
+
+static const struct mpll_config imx_mpll_cfg[] = {
+   {
+   4525, {
+   { 0x01e0, 0x },
+   { 0x21e1, 0x },
+   { 0x41e2, 0x }
+   },
+   }, {
+   9250, {
+   { 0x0140, 0x0005 },
+   { 0x2141, 0x0005 },
+   { 0x4142, 0x0005 },
+   },
+   }, {
+   14850, {
+   { 0x00a0, 0x000a },
+   { 0x20a1, 0x000a },
+   { 0x40a2, 0x000a },
+   },
+   }, {
+   ~0UL, {
+   { 0x00a0, 0x000a },
+   { 0x2001, 0x000f },
+   { 0x4002, 0x000f },
+   },
+   }
+};
+
+static const struct curr_ctrl imx_cur_ctr[] = {
+   /*  pixelclk bpp8bpp10   bpp12 */
+   {
+   5400, { 0x091c, 0x091c, 0x06dc },
+   }, {
+   5840, { 0x091c, 0x06dc, 0x06dc },
+   }, {
+   7200, { 0x06dc, 0x06dc, 0x091c },
+   }, {
+   7425, { 0x06dc, 0x0b5c, 0x091c },
+   }, {
+   11880, { 0x091c, 0x091c, 0x06dc },
+   }, {
+   21600, { 0x06dc, 0x0b5c, 0x091c },
+   }
+};

These pll clocks. Are they completely different?

I assume they are dependent on display resolutions. The old
ones should remain the same. With new additions for 4K clock
rates e.g.


even the 1080P on rockchip platform, we need a different
configuration, otherwise we can't get the best SI.

+
+static int imx_hdmi_parse_dt(struct imx_hdmi_priv *hdmi)
+{
+   struct device_node *np = hdmi-dev-of_node;
+
+   hdmi-regmap = syscon_regmap_lookup_by_phandle(np, gpr);
+   if (IS_ERR(hdmi-regmap)) {
+   dev_err(hdmi-dev, Unable to get gpr\n);
+   return PTR_ERR(hdmi-regmap);
+   }
+
+   hdmi-isfr_clk = devm_clk_get(hdmi-dev, isfr);
+   if (IS_ERR(hdmi-isfr_clk)) {
+   dev_err(hdmi-dev, Unable to get HDMI isfr clk\n);
+   return PTR_ERR(hdmi-isfr_clk);
+   }
+
+   hdmi-iahb_clk = devm_clk_get(hdmi-dev, iahb);
+   if (IS_ERR(hdmi-iahb_clk)) {
+   dev_err(hdmi-dev, Unable to get HDMI iahb clk\n);
+   return PTR_ERR(hdmi-iahb_clk);
+   }
+

These two clocks. This is how they are structured on the