[PATCH 0/15] drm: sun4i: Add support for the HDMI controller

[PATCH 0/15] drm: sun4i: Add support for the HDMI controller

[Maxime Ripard]

Hi,

Here is an attempt at getting the HDMI controller running.

This HDMI controller is found on a number of old Allwinner SoCs (A10, A10s,
A20, A31).

This driver only supports for now the A10s because it was an easy target,
being very close to the A13 that is already supported by our DRM driver.

There's nothing out of the extraordinary there, except maybe the clock
setup. All the internal clocks (TMDS, DDC) have been modeled using the
common clock framework, the TMDS clock being the parent of the DDC one.

While this might sound overkill, other SoC have a different, external
source for the DDC clock, which will be easier to support through the clock
framework.

It's still a bit rough around the edges, as it doesn't work for all the
modes. This will need to be fixed before being merged obviously.

The IP also supports audio (through an already supported i2s controller,
and some missing configuration in the HDMI controller) and CEC. Both will
come eventually.

Let me know what you think!
Maxime

Maxime Ripard (15):
  clk: divider: Make divider_round_rate take the parent clock
  clk: sunxi-ng: Pass the parent and a pointer to the clocks round rate
  clk: sunxi-ng: div: Switch to divider_round_rate
  clk: sunxi-ng: mux: Don't just rely on the parent for CLK_SET_RATE_PARENT
  clk: sunxi-ng: sun5i: Export video PLLs
  dt-bindings: display: sun4i: Add HDMI display bindings
  dt-bindings: display: sun4i: Add allwinner,tcon-channel property
  drm/sun4i: tcon: Add channel debug
  drm/sun4i: tcon: Pass the encoder to the mode set functions
  drm/sun4i: tcon: Switch mux on only for composite
  drm/sun4i: tcon: Fix tcon channel 1 backporch calculation
  drm/sun4i: tcon: multiply the vtotal when not in interlace
  drm/sun4i: Add HDMI support
  ARM: sun5i: a10s: Add the HDMI controller node
  ARM: sun5i: a10s-olinuxino: Enable HDMI

 Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt |  32 +-
 arch/arm/boot/dts/sun5i-a10s-olinuxino-micro.dts              |  12 +-
 arch/arm/boot/dts/sun5i-a10s.dtsi                             |  34 +-
 arch/arm/boot/dts/sun5i.dtsi                                  |   1 +-
 drivers/clk/clk-divider.c                                     |  18 +-
 drivers/clk/hisilicon/clkdivider-hi6220.c                     |   5 +-
 drivers/clk/meson/clk-cpu.c                                   |   5 +-
 drivers/clk/nxp/clk-lpc32xx.c                                 |   5 +-
 drivers/clk/qcom/clk-alpha-pll.c                              |   5 +-
 drivers/clk/qcom/clk-regmap-divider.c                         |   3 +-
 drivers/clk/sunxi-ng/ccu-sun5i.h                              |   6 +-
 drivers/clk/sunxi-ng/ccu_div.c                                |  28 +-
 drivers/clk/sunxi-ng/ccu_mp.c                                 |   7 +-
 drivers/clk/sunxi-ng/ccu_mult.c                               |  11 +-
 drivers/clk/sunxi-ng/ccu_mux.c                                |  22 +-
 drivers/clk/sunxi-ng/ccu_mux.h                                |   3 +-
 drivers/clk/sunxi-ng/ccu_nkm.c                                |   7 +-
 drivers/gpu/drm/sun4i/Makefile                                |   5 +-
 drivers/gpu/drm/sun4i/sun4i_hdmi.h                            | 124 ++-
 drivers/gpu/drm/sun4i/sun4i_hdmi_ddc_clk.c                    | 128 ++-
 drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c                        | 449 +++++++-
 drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c                   | 236 ++++-
 drivers/gpu/drm/sun4i/sun4i_rgb.c                             |   2 +-
 drivers/gpu/drm/sun4i/sun4i_tcon.c                            |  25 +-
 drivers/gpu/drm/sun4i/sun4i_tcon.h                            |   4 +-
 drivers/gpu/drm/sun4i/sun4i_tv.c                              |   2 +-
 drivers/rtc/rtc-ac100.c                                       |   6 +-
 include/dt-bindings/clock/sun5i-ccu.h                         |   3 +-
 include/linux/clk-provider.h                                  |   5 +-
 29 files changed, 1103 insertions(+), 90 deletions(-)
 create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi.h
 create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi_ddc_clk.c
 create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
 create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c

base-commit: d1bee31b9da7222c6be3248d1f3b087e8cc9004c
-- 
git-series 0.8.11

[PATCH 1/15] clk: divider: Make divider_round_rate take the parent clock

[Maxime Ripard]

So far, divider_round_rate only considers the parent clock returned by
clk_hw_get_parent.

This works fine on clocks that have a single parents, this doesn't work on
muxes, since we will only consider the first parent, while other parents
may totally be able to provide a better combination.

Clocks in that case cannot use divider_round_rate, so would have to come up
with a very similar logic to work around it. Instead of having to do
something like this, and duplicate that logic everywhere, give an
additional parameter for the parent clock to consider.

Current users have been converted using the following coccinelle script

@@
identifier hw, rate, prate, table, width, flags;
@@

-long divider_round_rate(struct clk_hw *hw,
+long divider_round_rate(struct clk_hw *hw, struct clk_hw *parent,
                        unsigned long rate,
                        unsigned long *prate,
                        const struct clk_div_table *table,
                        u8 width,
                        unsigned long flags) { ... }

@@
identifier fn, hw;
expression E2, E3, E4, E5, E6;
@@
 fn (struct clk_hw *hw, ...) {
 <...
-divider_round_rate(hw, E2, E3, E4, E5, E6)
+divider_round_rate(hw, clk_hw_get_parent(hw), E2, E3, E4, E5, E6)
 ...>
}

Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
Cc: Carlo Caione <carlo-KA+7E9HrN00dnm+yROfE0A@public.gmane.org>
Cc: Kevin Hilman <khilman-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
Cc: Vladimir Zapolskiy <vz-ChpfBGZJDbMAvxtiuMwx3w@public.gmane.org>
Cc: Sylvain Lemieux <slemieux.tyco-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Andy Gross <andy.gross-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: David Brown <david.brown-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: Alessandro Zummo <a.zummo-BfzFCNDTiLLj+vYz1yj4TQ@public.gmane.org>
Cc: Alexandre Belloni <alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
Cc: linux-amlogic-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
Cc: linux-arm-msm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: linux-soc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: rtc-linux-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org
---
 drivers/clk/clk-divider.c                 | 18 ++++++++++--------
 drivers/clk/hisilicon/clkdivider-hi6220.c |  5 +++--
 drivers/clk/meson/clk-cpu.c               |  5 +++--
 drivers/clk/nxp/clk-lpc32xx.c             |  5 +++--
 drivers/clk/qcom/clk-alpha-pll.c          |  5 +++--
 drivers/clk/qcom/clk-regmap-divider.c     |  3 ++-
 drivers/clk/sunxi-ng/ccu_div.c            |  6 +++---
 drivers/rtc/rtc-ac100.c                   |  6 ++++--
 include/linux/clk-provider.h              |  5 +++--
 9 files changed, 34 insertions(+), 24 deletions(-)

diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
index 96386ffc8483..d8d7dc84956a 100644
--- a/drivers/clk/clk-divider.c
+++ b/drivers/clk/clk-divider.c
@@ -275,7 +275,8 @@ static int _next_div(const struct clk_div_table *table, int div,
 	return div;
 }
 
-static int clk_divider_bestdiv(struct clk_hw *hw, unsigned long rate,
+static int clk_divider_bestdiv(struct clk_hw *hw, struct clk_hw *parent,
+			       unsigned long rate,
 			       unsigned long *best_parent_rate,
 			       const struct clk_div_table *table, u8 width,
 			       unsigned long flags)
@@ -314,8 +315,7 @@ static int clk_divider_bestdiv(struct clk_hw *hw, unsigned long rate,
 			*best_parent_rate = parent_rate_saved;
 			return i;
 		}
-		parent_rate = clk_hw_round_rate(clk_hw_get_parent(hw),
-					       rate * i);
+		parent_rate = clk_hw_round_rate(parent, rate * i);
 		now = DIV_ROUND_UP_ULL((u64)parent_rate, i);
 		if (_is_best_div(rate, now, best, flags)) {
 			bestdiv = i;
@@ -326,19 +326,20 @@ static int clk_divider_bestdiv(struct clk_hw *hw, unsigned long rate,
 
 	if (!bestdiv) {
 		bestdiv = _get_maxdiv(table, width, flags);
-		*best_parent_rate = clk_hw_round_rate(clk_hw_get_parent(hw), 1);
+		*best_parent_rate = clk_hw_round_rate(parent, 1);
 	}
 
 	return bestdiv;
 }
 
-long divider_round_rate(struct clk_hw *hw, unsigned long rate,
+long divider_round_rate(struct clk_hw *hw, struct clk_hw *parent,
+			unsigned long rate,
 			unsigned long *prate, const struct clk_div_table *table,
 			u8 width, unsigned long flags)
 {
 	int div;
 
-	div = clk_divider_bestdiv(hw, rate, prate, table, width, flags);
+	div = clk_divider_bestdiv(hw, parent, rate, prate, table, width, flags);
 
 	return DIV_ROUND_UP_ULL((u64)*prate, div);
 }
@@ -359,8 +360,9 @@ static long clk_divider_round_rate(struct clk_hw *hw, unsigned long rate,
 		return DIV_ROUND_UP_ULL((u64)*prate, bestdiv);
 	}
 
-	return divider_round_rate(hw, rate, prate, divider->table,
-				  divider->width, divider->flags);
+	return divider_round_rate(hw, clk_hw_get_parent(hw), rate, prate,
+				  divider->table, divider->width,
+				  divider->flags);
 }
 
 int divider_get_val(unsigned long rate, unsigned long parent_rate,
diff --git a/drivers/clk/hisilicon/clkdivider-hi6220.c b/drivers/clk/hisilicon/clkdivider-hi6220.c
index a1c1f684ad58..deaa72902555 100644
--- a/drivers/clk/hisilicon/clkdivider-hi6220.c
+++ b/drivers/clk/hisilicon/clkdivider-hi6220.c
@@ -64,8 +64,9 @@ static long hi6220_clkdiv_round_rate(struct clk_hw *hw, unsigned long rate,
 {
 	struct hi6220_clk_divider *dclk = to_hi6220_clk_divider(hw);
 
-	return divider_round_rate(hw, rate, prate, dclk->table,
-				  dclk->width, CLK_DIVIDER_ROUND_CLOSEST);
+	return divider_round_rate(hw, clk_hw_get_parent(hw), rate, prate,
+				  dclk->table, dclk->width,
+				  CLK_DIVIDER_ROUND_CLOSEST);
 }
 
 static int hi6220_clkdiv_set_rate(struct clk_hw *hw, unsigned long rate,
diff --git a/drivers/clk/meson/clk-cpu.c b/drivers/clk/meson/clk-cpu.c
index f8b2b7efd016..63c44d8c7ea4 100644
--- a/drivers/clk/meson/clk-cpu.c
+++ b/drivers/clk/meson/clk-cpu.c
@@ -59,8 +59,9 @@ static long meson_clk_cpu_round_rate(struct clk_hw *hw, unsigned long rate,
 {
 	struct meson_clk_cpu *clk_cpu = to_meson_clk_cpu_hw(hw);
 
-	return divider_round_rate(hw, rate, prate, clk_cpu->div_table,
-				  MESON_N_WIDTH, CLK_DIVIDER_ROUND_CLOSEST);
+	return divider_round_rate(hw, clk_hw_get_parent(hw), rate, prate,
+				  clk_cpu->div_table, MESON_N_WIDTH,
+				  CLK_DIVIDER_ROUND_CLOSEST);
 }
 
 static int meson_clk_cpu_set_rate(struct clk_hw *hw, unsigned long rate,
diff --git a/drivers/clk/nxp/clk-lpc32xx.c b/drivers/clk/nxp/clk-lpc32xx.c
index 5b98ff9076f3..73e83a7da7d6 100644
--- a/drivers/clk/nxp/clk-lpc32xx.c
+++ b/drivers/clk/nxp/clk-lpc32xx.c
@@ -975,8 +975,9 @@ static long clk_divider_round_rate(struct clk_hw *hw, unsigned long rate,
 		return DIV_ROUND_UP(*prate, bestdiv);
 	}
 
-	return divider_round_rate(hw, rate, prate, divider->table,
-				  divider->width, divider->flags);
+	return divider_round_rate(hw, clk_hw_get_parent(hw), rate, prate,
+				  divider->table, divider->width,
+				  divider->flags);
 }
 
 static int clk_divider_set_rate(struct clk_hw *hw, unsigned long rate,
diff --git a/drivers/clk/qcom/clk-alpha-pll.c b/drivers/clk/qcom/clk-alpha-pll.c
index 47a1da3739ce..7c08afecc811 100644
--- a/drivers/clk/qcom/clk-alpha-pll.c
+++ b/drivers/clk/qcom/clk-alpha-pll.c
@@ -478,8 +478,9 @@ clk_alpha_pll_postdiv_round_rate(struct clk_hw *hw, unsigned long rate,
 {
 	struct clk_alpha_pll_postdiv *pll = to_clk_alpha_pll_postdiv(hw);
 
-	return divider_round_rate(hw, rate, prate, clk_alpha_div_table,
-				  pll->width, CLK_DIVIDER_POWER_OF_TWO);
+	return divider_round_rate(hw, clk_hw_get_parent(hw), rate, prate,
+				  clk_alpha_div_table, pll->width,
+				  CLK_DIVIDER_POWER_OF_TWO);
 }
 
 static int clk_alpha_pll_postdiv_set_rate(struct clk_hw *hw, unsigned long rate,
diff --git a/drivers/clk/qcom/clk-regmap-divider.c b/drivers/clk/qcom/clk-regmap-divider.c
index 53484912301e..63489f0f2a02 100644
--- a/drivers/clk/qcom/clk-regmap-divider.c
+++ b/drivers/clk/qcom/clk-regmap-divider.c
@@ -28,7 +28,8 @@ static long div_round_rate(struct clk_hw *hw, unsigned long rate,
 {
 	struct clk_regmap_div *divider = to_clk_regmap_div(hw);
 
-	return divider_round_rate(hw, rate, prate, NULL, divider->width,
+	return divider_round_rate(hw, clk_hw_get_parent(hw), rate, prate,
+				  NULL, divider->width,
 				  CLK_DIVIDER_ROUND_CLOSEST);
 }
 
diff --git a/drivers/clk/sunxi-ng/ccu_div.c b/drivers/clk/sunxi-ng/ccu_div.c
index 4057e6021aa9..92855c2b30bb 100644
--- a/drivers/clk/sunxi-ng/ccu_div.c
+++ b/drivers/clk/sunxi-ng/ccu_div.c
@@ -78,10 +78,10 @@ static int ccu_div_determine_rate(struct clk_hw *hw,
 	struct ccu_div *cd = hw_to_ccu_div(hw);
 
 	if (clk_hw_get_num_parents(hw) == 1) {
-		req->rate = divider_round_rate(hw, req->rate,
+		req->rate = divider_round_rate(hw, clk_hw_get_parent(hw),
+					       req->rate,
 					       &req->best_parent_rate,
-					       cd->div.table,
-					       cd->div.width,
+					       cd->div.table, cd->div.width,
 					       cd->div.flags);
 
 		req->best_parent_hw = clk_hw_get_parent(hw);
diff --git a/drivers/rtc/rtc-ac100.c b/drivers/rtc/rtc-ac100.c
index 9e336184491c..0cad1bc41aa7 100644
--- a/drivers/rtc/rtc-ac100.c
+++ b/drivers/rtc/rtc-ac100.c
@@ -153,13 +153,15 @@ static long ac100_clkout_round_rate(struct clk_hw *hw, unsigned long rate,
 	int i;
 
 	if (prate == AC100_RTC_32K_RATE)
-		return divider_round_rate(hw, rate, &prate, NULL,
+		return divider_round_rate(hw, clk_hw_get_parent(hw), rate,
+					  &prate, NULL,
 					  AC100_CLKOUT_DIV_WIDTH,
 					  CLK_DIVIDER_POWER_OF_TWO);
 
 	for (i = 0; ac100_clkout_prediv[i].div; i++) {
 		tmp_prate = DIV_ROUND_UP(prate, ac100_clkout_prediv[i].val);
-		tmp_rate = divider_round_rate(hw, rate, &tmp_prate, NULL,
+		tmp_rate = divider_round_rate(hw, clk_hw_get_parent(hw), rate,
+					      &tmp_prate, NULL,
 					      AC100_CLKOUT_DIV_WIDTH,
 					      CLK_DIVIDER_POWER_OF_TWO);
 
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index a428aec36ace..e17d4cc7660c 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -412,8 +412,9 @@ extern const struct clk_ops clk_divider_ro_ops;
 unsigned long divider_recalc_rate(struct clk_hw *hw, unsigned long parent_rate,
 		unsigned int val, const struct clk_div_table *table,
 		unsigned long flags);
-long divider_round_rate(struct clk_hw *hw, unsigned long rate,
-		unsigned long *prate, const struct clk_div_table *table,
+long divider_round_rate(struct clk_hw *hw, struct clk_hw *parent,
+		unsigned long rate, unsigned long *prate,
+		const struct clk_div_table *table,
 		u8 width, unsigned long flags);
 int divider_get_val(unsigned long rate, unsigned long parent_rate,
 		const struct clk_div_table *table, u8 width,
-- 
git-series 0.8.11

Re: [PATCH 1/15] clk: divider: Make divider_round_rate take the parent clock

[Stephen Boyd]

On 03/07, Maxime Ripard wrote:
> So far, divider_round_rate only considers the parent clock returned by
> clk_hw_get_parent.
> 
> This works fine on clocks that have a single parents, this doesn't work on
> muxes, since we will only consider the first parent, while other parents
> may totally be able to provide a better combination.
> 
> Clocks in that case cannot use divider_round_rate, so would have to come up
> with a very similar logic to work around it. Instead of having to do
> something like this, and duplicate that logic everywhere, give an
> additional parameter for the parent clock to consider.
> 
> Current users have been converted using the following coccinelle script
> 
> @@
> identifier hw, rate, prate, table, width, flags;
> @@
> 
> -long divider_round_rate(struct clk_hw *hw,
> +long divider_round_rate(struct clk_hw *hw, struct clk_hw *parent,
>                         unsigned long rate,
>                         unsigned long *prate,
>                         const struct clk_div_table *table,
>                         u8 width,
>                         unsigned long flags) { ... }
> 
> @@
> identifier fn, hw;
> expression E2, E3, E4, E5, E6;
> @@
>  fn (struct clk_hw *hw, ...) {
>  <...
> -divider_round_rate(hw, E2, E3, E4, E5, E6)
> +divider_round_rate(hw, clk_hw_get_parent(hw), E2, E3, E4, E5, E6)
>  ...>
> }

Why not introduce another function like 

	divider_round_rate_parent()
	divider_round_rate_mux()

that takes the extra parent argument? Technically, a divider is
considered to only have one parent, and if it has more than one
parent, then it is a mux and a divider.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/15] clk: divider: Make divider_round_rate take the parent clock

[Maxime Ripard]

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

Hi Stephen,

On Tue, Mar 07, 2017 at 06:11:57AM -0800, Stephen Boyd wrote:
> On 03/07, Maxime Ripard wrote:
> > So far, divider_round_rate only considers the parent clock returned by
> > clk_hw_get_parent.
> > 
> > This works fine on clocks that have a single parents, this doesn't work on
> > muxes, since we will only consider the first parent, while other parents
> > may totally be able to provide a better combination.
> > 
> > Clocks in that case cannot use divider_round_rate, so would have to come up
> > with a very similar logic to work around it. Instead of having to do
> > something like this, and duplicate that logic everywhere, give an
> > additional parameter for the parent clock to consider.
> > 
> > Current users have been converted using the following coccinelle script
> > 
> > @@
> > identifier hw, rate, prate, table, width, flags;
> > @@
> > 
> > -long divider_round_rate(struct clk_hw *hw,
> > +long divider_round_rate(struct clk_hw *hw, struct clk_hw *parent,
> >                         unsigned long rate,
> >                         unsigned long *prate,
> >                         const struct clk_div_table *table,
> >                         u8 width,
> >                         unsigned long flags) { ... }
> > 
> > @@
> > identifier fn, hw;
> > expression E2, E3, E4, E5, E6;
> > @@
> >  fn (struct clk_hw *hw, ...) {
> >  <...
> > -divider_round_rate(hw, E2, E3, E4, E5, E6)
> > +divider_round_rate(hw, clk_hw_get_parent(hw), E2, E3, E4, E5, E6)
> >  ...>
> > }
> 
> Why not introduce another function like 
> 
> 	divider_round_rate_parent()
> 	divider_round_rate_mux()
> 
> that takes the extra parent argument? Technically, a divider is
> considered to only have one parent, and if it has more than one
> parent, then it is a mux and a divider.

Yes, that would work too, without needing the cross tree change. I'll
do that in the next version.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[PATCH 2/15] clk: sunxi-ng: Pass the parent and a pointer to the clocks round rate

[Maxime Ripard]

The clocks might need to modify their parent clocks. In order to make that
possible, give them access to the parent clock being evaluated, and to a
pointer to the parent rate so that they can modify it if needed.

Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 drivers/clk/sunxi-ng/ccu_div.c  |  7 ++++---
 drivers/clk/sunxi-ng/ccu_mp.c   |  7 ++++---
 drivers/clk/sunxi-ng/ccu_mult.c | 11 ++++++-----
 drivers/clk/sunxi-ng/ccu_mux.c  |  8 +++++---
 drivers/clk/sunxi-ng/ccu_mux.h  |  3 ++-
 drivers/clk/sunxi-ng/ccu_nkm.c  |  7 ++++---
 6 files changed, 25 insertions(+), 18 deletions(-)

diff --git a/drivers/clk/sunxi-ng/ccu_div.c b/drivers/clk/sunxi-ng/ccu_div.c
index 92855c2b30bb..7f8b06e38636 100644
--- a/drivers/clk/sunxi-ng/ccu_div.c
+++ b/drivers/clk/sunxi-ng/ccu_div.c
@@ -14,7 +14,8 @@
 #include "ccu_div.h"
 
 static unsigned long ccu_div_round_rate(struct ccu_mux_internal *mux,
-					unsigned long parent_rate,
+					struct clk_hw *parent,
+					unsigned long *parent_rate,
 					unsigned long rate,
 					void *data)
 {
@@ -26,10 +27,10 @@ static unsigned long ccu_div_round_rate(struct ccu_mux_internal *mux,
 	 * several parents, while we might be called to evaluate
 	 * several different parents.
 	 */
-	val = divider_get_val(rate, parent_rate, cd->div.table, cd->div.width,
+	val = divider_get_val(rate, *parent_rate, cd->div.table, cd->div.width,
 			      cd->div.flags);
 
-	return divider_recalc_rate(&cd->common.hw, parent_rate, val,
+	return divider_recalc_rate(&cd->common.hw, *parent_rate, val,
 				   cd->div.table, cd->div.flags);
 }
 
diff --git a/drivers/clk/sunxi-ng/ccu_mp.c b/drivers/clk/sunxi-ng/ccu_mp.c
index b583f186a804..de02e6c386d8 100644
--- a/drivers/clk/sunxi-ng/ccu_mp.c
+++ b/drivers/clk/sunxi-ng/ccu_mp.c
@@ -41,7 +41,8 @@ static void ccu_mp_find_best(unsigned long parent, unsigned long rate,
 }
 
 static unsigned long ccu_mp_round_rate(struct ccu_mux_internal *mux,
-				       unsigned long parent_rate,
+				       struct clk_hw *hw,
+				       unsigned long *parent_rate,
 				       unsigned long rate,
 				       void *data)
 {
@@ -52,9 +53,9 @@ static unsigned long ccu_mp_round_rate(struct ccu_mux_internal *mux,
 	max_m = cmp->m.max ?: 1 << cmp->m.width;
 	max_p = cmp->p.max ?: 1 << ((1 << cmp->p.width) - 1);
 
-	ccu_mp_find_best(parent_rate, rate, max_m, max_p, &m, &p);
+	ccu_mp_find_best(*parent_rate, rate, max_m, max_p, &m, &p);
 
-	return parent_rate / p / m;
+	return *parent_rate / p / m;
 }
 
 static void ccu_mp_disable(struct clk_hw *hw)
diff --git a/drivers/clk/sunxi-ng/ccu_mult.c b/drivers/clk/sunxi-ng/ccu_mult.c
index 8724c01171b1..76d17162366f 100644
--- a/drivers/clk/sunxi-ng/ccu_mult.c
+++ b/drivers/clk/sunxi-ng/ccu_mult.c
@@ -33,9 +33,10 @@ static void ccu_mult_find_best(unsigned long parent, unsigned long rate,
 }
 
 static unsigned long ccu_mult_round_rate(struct ccu_mux_internal *mux,
-					unsigned long parent_rate,
-					unsigned long rate,
-					void *data)
+					 struct clk_hw *parent,
+					 unsigned long *parent_rate,
+					 unsigned long rate,
+					 void *data)
 {
 	struct ccu_mult *cm = data;
 	struct _ccu_mult _cm;
@@ -47,9 +48,9 @@ static unsigned long ccu_mult_round_rate(struct ccu_mux_internal *mux,
 	else
 		_cm.max = (1 << cm->mult.width) + cm->mult.offset - 1;
 
-	ccu_mult_find_best(parent_rate, rate, &_cm);
+	ccu_mult_find_best(*parent_rate, rate, &_cm);
 
-	return parent_rate * _cm.mult;
+	return *parent_rate * _cm.mult;
 }
 
 static void ccu_mult_disable(struct clk_hw *hw)
diff --git a/drivers/clk/sunxi-ng/ccu_mux.c b/drivers/clk/sunxi-ng/ccu_mux.c
index c6bb1f523232..bae735e252b6 100644
--- a/drivers/clk/sunxi-ng/ccu_mux.c
+++ b/drivers/clk/sunxi-ng/ccu_mux.c
@@ -61,7 +61,8 @@ int ccu_mux_helper_determine_rate(struct ccu_common *common,
 				  struct ccu_mux_internal *cm,
 				  struct clk_rate_request *req,
 				  unsigned long (*round)(struct ccu_mux_internal *,
-							 unsigned long,
+							 struct clk_hw *,
+							 unsigned long *,
 							 unsigned long,
 							 void *),
 				  void *data)
@@ -80,7 +81,8 @@ int ccu_mux_helper_determine_rate(struct ccu_common *common,
 		ccu_mux_helper_adjust_parent_for_prediv(common, cm, -1,
 							&adj_parent_rate);
 
-		best_rate = round(cm, adj_parent_rate, req->rate, data);
+		best_rate = round(cm, best_parent, &adj_parent_rate,
+				  req->rate, data);
 
 		goto out;
 	}
@@ -109,7 +111,7 @@ int ccu_mux_helper_determine_rate(struct ccu_common *common,
 		ccu_mux_helper_adjust_parent_for_prediv(common, cm, i,
 							&adj_parent_rate);
 
-		tmp_rate = round(cm, adj_parent_rate, req->rate, data);
+		tmp_rate = round(cm, parent, &adj_parent_rate, req->rate, data);
 		if (tmp_rate == req->rate) {
 			best_parent = parent;
 			best_parent_rate = parent_rate;
diff --git a/drivers/clk/sunxi-ng/ccu_mux.h b/drivers/clk/sunxi-ng/ccu_mux.h
index 47aba3a48245..4be56eee2bfd 100644
--- a/drivers/clk/sunxi-ng/ccu_mux.h
+++ b/drivers/clk/sunxi-ng/ccu_mux.h
@@ -86,7 +86,8 @@ int ccu_mux_helper_determine_rate(struct ccu_common *common,
 				  struct ccu_mux_internal *cm,
 				  struct clk_rate_request *req,
 				  unsigned long (*round)(struct ccu_mux_internal *,
-							 unsigned long,
+							 struct clk_hw *,
+							 unsigned long *,
 							 unsigned long,
 							 void *),
 				  void *data);
diff --git a/drivers/clk/sunxi-ng/ccu_nkm.c b/drivers/clk/sunxi-ng/ccu_nkm.c
index 71f81e95a061..c5e010eb5991 100644
--- a/drivers/clk/sunxi-ng/ccu_nkm.c
+++ b/drivers/clk/sunxi-ng/ccu_nkm.c
@@ -102,7 +102,8 @@ static unsigned long ccu_nkm_recalc_rate(struct clk_hw *hw,
 }
 
 static unsigned long ccu_nkm_round_rate(struct ccu_mux_internal *mux,
-					unsigned long parent_rate,
+					struct clk_hw *hw,
+					unsigned long *parent_rate,
 					unsigned long rate,
 					void *data)
 {
@@ -116,9 +117,9 @@ static unsigned long ccu_nkm_round_rate(struct ccu_mux_internal *mux,
 	_nkm.min_m = 1;
 	_nkm.max_m = nkm->m.max ?: 1 << nkm->m.width;
 
-	ccu_nkm_find_best(parent_rate, rate, &_nkm);
+	ccu_nkm_find_best(*parent_rate, rate, &_nkm);
 
-	return parent_rate * _nkm.n * _nkm.k / _nkm.m;
+	return *parent_rate * _nkm.n * _nkm.k / _nkm.m;
 }
 
 static int ccu_nkm_determine_rate(struct clk_hw *hw,
-- 
git-series 0.8.11

[PATCH 3/15] clk: sunxi-ng: div: Switch to divider_round_rate

[Maxime Ripard]

divider_round_rate already evaluates changing the parent rate if
CLK_SET_RATE_PARENT is set. Now that we can do that on muxes too, let's
just use it.

Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 drivers/clk/sunxi-ng/ccu_div.c | 25 ++-----------------------
 1 file changed, 2 insertions(+), 23 deletions(-)

diff --git a/drivers/clk/sunxi-ng/ccu_div.c b/drivers/clk/sunxi-ng/ccu_div.c
index 7f8b06e38636..0ccdd3dcc20c 100644
--- a/drivers/clk/sunxi-ng/ccu_div.c
+++ b/drivers/clk/sunxi-ng/ccu_div.c
@@ -20,18 +20,9 @@ static unsigned long ccu_div_round_rate(struct ccu_mux_internal *mux,
 					void *data)
 {
 	struct ccu_div *cd = data;
-	unsigned long val;
-
-	/*
-	 * We can't use divider_round_rate that assumes that there's
-	 * several parents, while we might be called to evaluate
-	 * several different parents.
-	 */
-	val = divider_get_val(rate, *parent_rate, cd->div.table, cd->div.width,
-			      cd->div.flags);
 
-	return divider_recalc_rate(&cd->common.hw, *parent_rate, val,
-				   cd->div.table, cd->div.flags);
+	return divider_round_rate(&cd->common.hw, parent, rate, parent_rate,
+				  cd->div.table, cd->div.width, cd->div.flags);
 }
 
 static void ccu_div_disable(struct clk_hw *hw)
@@ -78,18 +69,6 @@ static int ccu_div_determine_rate(struct clk_hw *hw,
 {
 	struct ccu_div *cd = hw_to_ccu_div(hw);
 
-	if (clk_hw_get_num_parents(hw) == 1) {
-		req->rate = divider_round_rate(hw, clk_hw_get_parent(hw),
-					       req->rate,
-					       &req->best_parent_rate,
-					       cd->div.table, cd->div.width,
-					       cd->div.flags);
-
-		req->best_parent_hw = clk_hw_get_parent(hw);
-
-		return 0;
-	}
-
 	return ccu_mux_helper_determine_rate(&cd->common, &cd->mux,
 					     req, ccu_div_round_rate, cd);
 }
-- 
git-series 0.8.11

[PATCH 4/15] clk: sunxi-ng: mux: Don't just rely on the parent for CLK_SET_RATE_PARENT

[Maxime Ripard]

The current code only rely on the parent to change its rate in the case
where CLK_SET_RATE_PARENT is set.

However, some clock rates might be obtained only through a modification of
the parent and the clock divider. Just rely on the round rate of the clocks
to give us the best computation that might be achieved for a given rate.

round_rate functions now need to honor CLK_SET_RATE_PARENT, but either the
functions already do that if they modify the parent, or don't modify the
praents at all.

Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 drivers/clk/sunxi-ng/ccu_mux.c | 14 +-------------
 1 file changed, 1 insertion(+), 13 deletions(-)

diff --git a/drivers/clk/sunxi-ng/ccu_mux.c b/drivers/clk/sunxi-ng/ccu_mux.c
index bae735e252b6..58b6e349a0ed 100644
--- a/drivers/clk/sunxi-ng/ccu_mux.c
+++ b/drivers/clk/sunxi-ng/ccu_mux.c
@@ -95,19 +95,7 @@ int ccu_mux_helper_determine_rate(struct ccu_common *common,
 		if (!parent)
 			continue;
 
-		if (clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) {
-			struct clk_rate_request parent_req = *req;
-			int ret = __clk_determine_rate(parent, &parent_req);
-
-			if (ret)
-				continue;
-
-			parent_rate = parent_req.rate;
-		} else {
-			parent_rate = clk_hw_get_rate(parent);
-		}
-
-		adj_parent_rate = parent_rate;
+		adj_parent_rate = parent_rate = clk_hw_get_rate(parent);
 		ccu_mux_helper_adjust_parent_for_prediv(common, cm, i,
 							&adj_parent_rate);
 
-- 
git-series 0.8.11

[PATCH 5/15] clk: sunxi-ng: sun5i: Export video PLLs

[Maxime Ripard]

The video PLLs are used directly by the HDMI controller. Export them so
that we can use them in our DT node.

Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 drivers/clk/sunxi-ng/ccu-sun5i.h      | 6 ++++--
 include/dt-bindings/clock/sun5i-ccu.h | 3 +++
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/sunxi-ng/ccu-sun5i.h b/drivers/clk/sunxi-ng/ccu-sun5i.h
index 8144487eb7ca..16f7aa92957e 100644
--- a/drivers/clk/sunxi-ng/ccu-sun5i.h
+++ b/drivers/clk/sunxi-ng/ccu-sun5i.h
@@ -28,15 +28,17 @@
 #define CLK_PLL_AUDIO_4X	6
 #define CLK_PLL_AUDIO_8X	7
 #define CLK_PLL_VIDEO0		8
-#define CLK_PLL_VIDEO0_2X	9
+
+/* The PLL_VIDEO0_2X is exported for HDMI */
+
 #define CLK_PLL_VE		10
 #define CLK_PLL_DDR_BASE	11
 #define CLK_PLL_DDR		12
 #define CLK_PLL_DDR_OTHER	13
 #define CLK_PLL_PERIPH		14
 #define CLK_PLL_VIDEO1		15
-#define CLK_PLL_VIDEO1_2X	16
 
+/* The PLL_VIDEO0_2X is exported for HDMI */
 /* The CPU clock is exported */
 
 #define CLK_AXI			18
diff --git a/include/dt-bindings/clock/sun5i-ccu.h b/include/dt-bindings/clock/sun5i-ccu.h
index aeb2e2f781fb..81f34d477aeb 100644
--- a/include/dt-bindings/clock/sun5i-ccu.h
+++ b/include/dt-bindings/clock/sun5i-ccu.h
@@ -19,6 +19,9 @@
 
 #define CLK_HOSC		1
 
+#define CLK_PLL_VIDEO0_2X	9
+
+#define CLK_PLL_VIDEO1_2X	16
 #define CLK_CPU			17
 
 #define CLK_AHB_OTG		23
-- 
git-series 0.8.11

Re: [PATCH 5/15] clk: sunxi-ng: sun5i: Export video PLLs

[Julian Calaby]

Hi Maxime,

On Tue, Mar 7, 2017 at 7:56 PM, Maxime Ripard
<maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> The video PLLs are used directly by the HDMI controller. Export them so
> that we can use them in our DT node.
>
> Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
>  drivers/clk/sunxi-ng/ccu-sun5i.h      | 6 ++++--
>  include/dt-bindings/clock/sun5i-ccu.h | 3 +++
>  2 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/clk/sunxi-ng/ccu-sun5i.h b/drivers/clk/sunxi-ng/ccu-sun5i.h
> index 8144487eb7ca..16f7aa92957e 100644
> --- a/drivers/clk/sunxi-ng/ccu-sun5i.h
> +++ b/drivers/clk/sunxi-ng/ccu-sun5i.h
> @@ -28,15 +28,17 @@
>  #define CLK_PLL_AUDIO_4X       6
>  #define CLK_PLL_AUDIO_8X       7
>  #define CLK_PLL_VIDEO0         8
> -#define CLK_PLL_VIDEO0_2X      9
> +
> +/* The PLL_VIDEO0_2X is exported for HDMI */
> +
>  #define CLK_PLL_VE             10
>  #define CLK_PLL_DDR_BASE       11
>  #define CLK_PLL_DDR            12
>  #define CLK_PLL_DDR_OTHER      13
>  #define CLK_PLL_PERIPH         14
>  #define CLK_PLL_VIDEO1         15
> -#define CLK_PLL_VIDEO1_2X      16
>
> +/* The PLL_VIDEO0_2X is exported for HDMI */

PLL_VIDEO*1*_2X, right?

>  /* The CPU clock is exported */
>
>  #define CLK_AXI                        18

Thanks,

-- 
Julian Calaby

Email: julian.calaby-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
Profile: http://www.google.com/profiles/julian.calaby/

Re: [linux-sunxi] [PATCH 5/15] clk: sunxi-ng: sun5i: Export video PLLs

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 1539 bytes --]

Hi Julian,

On Tue, Mar 07, 2017 at 09:21:19PM +1100, Julian Calaby wrote:
> Hi Maxime,
> 
> On Tue, Mar 7, 2017 at 7:56 PM, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
> > The video PLLs are used directly by the HDMI controller. Export them so
> > that we can use them in our DT node.
> >
> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > ---
> >  drivers/clk/sunxi-ng/ccu-sun5i.h      | 6 ++++--
> >  include/dt-bindings/clock/sun5i-ccu.h | 3 +++
> >  2 files changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/clk/sunxi-ng/ccu-sun5i.h b/drivers/clk/sunxi-ng/ccu-sun5i.h
> > index 8144487eb7ca..16f7aa92957e 100644
> > --- a/drivers/clk/sunxi-ng/ccu-sun5i.h
> > +++ b/drivers/clk/sunxi-ng/ccu-sun5i.h
> > @@ -28,15 +28,17 @@
> >  #define CLK_PLL_AUDIO_4X       6
> >  #define CLK_PLL_AUDIO_8X       7
> >  #define CLK_PLL_VIDEO0         8
> > -#define CLK_PLL_VIDEO0_2X      9
> > +
> > +/* The PLL_VIDEO0_2X is exported for HDMI */
> > +
> >  #define CLK_PLL_VE             10
> >  #define CLK_PLL_DDR_BASE       11
> >  #define CLK_PLL_DDR            12
> >  #define CLK_PLL_DDR_OTHER      13
> >  #define CLK_PLL_PERIPH         14
> >  #define CLK_PLL_VIDEO1         15
> > -#define CLK_PLL_VIDEO1_2X      16
> >
> > +/* The PLL_VIDEO0_2X is exported for HDMI */
> 
> PLL_VIDEO*1*_2X, right?

Good catch, I'll fix it. Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 6/15] dt-bindings: display: sun4i: Add HDMI display bindings

[Maxime Ripard]

One of the possible output of the display pipeline, on the SoCs that have
it, is the HDMI controller.

Add a binding for it.

Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt | 21 +++++++-
 1 file changed, 21 insertions(+), 0 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt b/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt
index b82c00449468..4b280672658e 100644
--- a/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt
+++ b/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt
@@ -4,6 +4,27 @@ Allwinner A10 Display Pipeline
 The Allwinner A10 Display pipeline is composed of several components
 that are going to be documented below:
 
+HDMI Encoder
+------------
+
+The HDMI Encoder supports the HDMI video and audio outputs, and does
+CEC. It is one end of the pipeline.
+
+Required properties:
+  - compatible: value must be one of:
+    * allwinner,sun5i-a10s-hdmi
+  - reg: base address and size of memory-mapped region
+  - clocks: phandles to the clocks feeding the HDMI encoder
+    * ahb: the HDMI interface clock
+    * mod: the HDMI module clock
+    * pll-0: the first video PLL
+    * pll-1: the second video PLL
+  - clock-names: the clock names mentioned above
+
+  - ports: A ports node with endpoint definitions as defined in
+    Documentation/devicetree/bindings/media/video-interfaces.txt. The
+    first port should be the input endpoint.
+
 TV Encoder
 ----------
 
-- 
git-series 0.8.11

Re: [PATCH 6/15] dt-bindings: display: sun4i: Add HDMI display bindings

[Chen-Yu Tsai]

On Tue, Mar 7, 2017 at 4:56 PM, Maxime Ripard
<maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> One of the possible output of the display pipeline, on the SoCs that have
> it, is the HDMI controller.
>
> Add a binding for it.
>
> Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

Acked-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>

TODO: A31 will also need a DDC clock.

Re: [PATCH 6/15] dt-bindings: display: sun4i: Add HDMI display bindings

[Rob Herring]

On Tue, Mar 07, 2017 at 09:56:25AM +0100, Maxime Ripard wrote:
> One of the possible output of the display pipeline, on the SoCs that have
> it, is the HDMI controller.
> 
> Add a binding for it.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt | 21 +++++++-
>  1 file changed, 21 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt b/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt
> index b82c00449468..4b280672658e 100644
> --- a/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt
> +++ b/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt
> @@ -4,6 +4,27 @@ Allwinner A10 Display Pipeline
>  The Allwinner A10 Display pipeline is composed of several components
>  that are going to be documented below:
>  
> +HDMI Encoder
> +------------
> +
> +The HDMI Encoder supports the HDMI video and audio outputs, and does
> +CEC. It is one end of the pipeline.
> +
> +Required properties:
> +  - compatible: value must be one of:
> +    * allwinner,sun5i-a10s-hdmi
> +  - reg: base address and size of memory-mapped region
> +  - clocks: phandles to the clocks feeding the HDMI encoder
> +    * ahb: the HDMI interface clock
> +    * mod: the HDMI module clock
> +    * pll-0: the first video PLL
> +    * pll-1: the second video PLL
> +  - clock-names: the clock names mentioned above
> +
> +  - ports: A ports node with endpoint definitions as defined in
> +    Documentation/devicetree/bindings/media/video-interfaces.txt. The
> +    first port should be the input endpoint.

You need an output port to an HDMI connector node and an audio port.

> +
>  TV Encoder
>  ----------
>  
> -- 
> git-series 0.8.11

Re: [PATCH 6/15] dt-bindings: display: sun4i: Add HDMI display bindings

[Maxime Ripard]

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

Hi Rob,

On Wed, Mar 15, 2017 at 12:26:22PM -0500, Rob Herring wrote:
> > +HDMI Encoder
> > +------------
> > +
> > +The HDMI Encoder supports the HDMI video and audio outputs, and does
> > +CEC. It is one end of the pipeline.
> > +
> > +Required properties:
> > +  - compatible: value must be one of:
> > +    * allwinner,sun5i-a10s-hdmi
> > +  - reg: base address and size of memory-mapped region
> > +  - clocks: phandles to the clocks feeding the HDMI encoder
> > +    * ahb: the HDMI interface clock
> > +    * mod: the HDMI module clock
> > +    * pll-0: the first video PLL
> > +    * pll-1: the second video PLL
> > +  - clock-names: the clock names mentioned above
> > +
> > +  - ports: A ports node with endpoint definitions as defined in
> > +    Documentation/devicetree/bindings/media/video-interfaces.txt. The
> > +    first port should be the input endpoint.
> 
> You need an output port to an HDMI connector node and an audio port.

I started to look at the audio, and I can't find a use for an audio
port in the OF graph. As far as I understand, we will be using the
hdmi-codec, that still requires an ASoC card to create the link
between our i2s controller and the HDMI controller.

This work perfectly for us, but as far as I know, the simple-card
stuff only requires a phandle, and not an OF graph endpoint, right?

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH 6/15] dt-bindings: display: sun4i: Add HDMI display bindings

[Chen-Yu Tsai]

On Tue, Mar 7, 2017 at 4:56 PM, Maxime Ripard
<maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> One of the possible output of the display pipeline, on the SoCs that have
> it, is the HDMI controller.
>
> Add a binding for it.
>
> Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt | 21 +++++++-
>  1 file changed, 21 insertions(+), 0 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt b/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt
> index b82c00449468..4b280672658e 100644
> --- a/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt
> +++ b/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt
> @@ -4,6 +4,27 @@ Allwinner A10 Display Pipeline
>  The Allwinner A10 Display pipeline is composed of several components
>  that are going to be documented below:
>
> +HDMI Encoder
> +------------
> +
> +The HDMI Encoder supports the HDMI video and audio outputs, and does
> +CEC. It is one end of the pipeline.
> +
> +Required properties:
> +  - compatible: value must be one of:
> +    * allwinner,sun5i-a10s-hdmi
> +  - reg: base address and size of memory-mapped region
> +  - clocks: phandles to the clocks feeding the HDMI encoder
> +    * ahb: the HDMI interface clock
> +    * mod: the HDMI module clock
> +    * pll-0: the first video PLL
> +    * pll-1: the second video PLL
> +  - clock-names: the clock names mentioned above

The audio part needs a DMA handle. May we add this from day one?

Thanks
ChenYu

> +
> +  - ports: A ports node with endpoint definitions as defined in
> +    Documentation/devicetree/bindings/media/video-interfaces.txt. The
> +    first port should be the input endpoint.
> +
>  TV Encoder
>  ----------
>
> --
> git-series 0.8.11

[PATCH 8/15] drm/sun4i: tcon: Add channel debug

[Maxime Ripard]

While all functions have debug logs, the channel enable and disable are not
logged. Make sure this is the case.

Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 drivers/gpu/drm/sun4i/sun4i_tcon.c | 4 ++++
 1 file changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
index 505520baa585..7461ae107e54 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
@@ -55,6 +55,8 @@ EXPORT_SYMBOL(sun4i_tcon_enable);
 
 void sun4i_tcon_channel_disable(struct sun4i_tcon *tcon, int channel)
 {
+	DRM_DEBUG_DRIVER("Disabling TCON channel %d\n", channel);
+
 	/* Disable the TCON's channel */
 	if (channel == 0) {
 		regmap_update_bits(tcon->regs, SUN4I_TCON0_CTL_REG,
@@ -72,6 +74,8 @@ EXPORT_SYMBOL(sun4i_tcon_channel_disable);
 
 void sun4i_tcon_channel_enable(struct sun4i_tcon *tcon, int channel)
 {
+	DRM_DEBUG_DRIVER("Enabling TCON channel %d\n", channel);
+
 	/* Enable the TCON's channel */
 	if (channel == 0) {
 		regmap_update_bits(tcon->regs, SUN4I_TCON0_CTL_REG,
-- 
git-series 0.8.11

Re: [PATCH 8/15] drm/sun4i: tcon: Add channel debug

[Chen-Yu Tsai]

On Tue, Mar 7, 2017 at 4:56 PM, Maxime Ripard
<maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> While all functions have debug logs, the channel enable and disable are not
> logged. Make sure this is the case.
>
> Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

Acked-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>

[PATCH 9/15] drm/sun4i: tcon: Pass the encoder to the mode set functions

[Maxime Ripard]

The mode set function need some changes based on which encoder is being
used. Make sure we can differentiate between our encoders by passing the
encoder structure asking for the mode set.

Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 drivers/gpu/drm/sun4i/sun4i_rgb.c  | 2 +-
 drivers/gpu/drm/sun4i/sun4i_tcon.c | 4 ++--
 drivers/gpu/drm/sun4i/sun4i_tcon.h | 4 ++--
 drivers/gpu/drm/sun4i/sun4i_tv.c   | 2 +-
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_rgb.c b/drivers/gpu/drm/sun4i/sun4i_rgb.c
index 1147451eb993..1d4a59a44d04 100644
--- a/drivers/gpu/drm/sun4i/sun4i_rgb.c
+++ b/drivers/gpu/drm/sun4i/sun4i_rgb.c
@@ -173,7 +173,7 @@ static void sun4i_rgb_encoder_mode_set(struct drm_encoder *encoder,
 	struct sun4i_rgb *rgb = drm_encoder_to_sun4i_rgb(encoder);
 	struct sun4i_tcon *tcon = rgb->tcon;
 
-	sun4i_tcon0_mode_set(tcon, mode);
+	sun4i_tcon0_mode_set(tcon, encoder, mode);
 
 	clk_set_rate(tcon->dclk, mode->crtc_clock * 1000);
 
diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
index 7461ae107e54..d2335f109601 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
@@ -127,7 +127,7 @@ static int sun4i_tcon_get_clk_delay(struct drm_display_mode *mode,
 	return delay;
 }
 
-void sun4i_tcon0_mode_set(struct sun4i_tcon *tcon,
+void sun4i_tcon0_mode_set(struct sun4i_tcon *tcon, struct drm_encoder *encoder,
 			  struct drm_display_mode *mode)
 {
 	unsigned int bp, hsync, vsync;
@@ -200,7 +200,7 @@ void sun4i_tcon0_mode_set(struct sun4i_tcon *tcon,
 }
 EXPORT_SYMBOL(sun4i_tcon0_mode_set);
 
-void sun4i_tcon1_mode_set(struct sun4i_tcon *tcon,
+void sun4i_tcon1_mode_set(struct sun4i_tcon *tcon, struct drm_encoder *encoder,
 			  struct drm_display_mode *mode)
 {
 	unsigned int bp, hsync, vsync;
diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.h b/drivers/gpu/drm/sun4i/sun4i_tcon.h
index f636343a935d..95b7e76eb1f8 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.h
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.h
@@ -190,9 +190,9 @@ void sun4i_tcon_enable_vblank(struct sun4i_tcon *tcon, bool enable);
 /* Mode Related Controls */
 void sun4i_tcon_switch_interlace(struct sun4i_tcon *tcon,
 				 bool enable);
-void sun4i_tcon0_mode_set(struct sun4i_tcon *tcon,
+void sun4i_tcon0_mode_set(struct sun4i_tcon *tcon, struct drm_encoder *encoder,
 			  struct drm_display_mode *mode);
-void sun4i_tcon1_mode_set(struct sun4i_tcon *tcon,
+void sun4i_tcon1_mode_set(struct sun4i_tcon *tcon, struct drm_encoder *encoder,
 			  struct drm_display_mode *mode);
 
 #endif /* __SUN4I_TCON_H__ */
diff --git a/drivers/gpu/drm/sun4i/sun4i_tv.c b/drivers/gpu/drm/sun4i/sun4i_tv.c
index 32ed5fdf0c4d..2d36df092a6a 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tv.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tv.c
@@ -389,7 +389,7 @@ static void sun4i_tv_mode_set(struct drm_encoder *encoder,
 	struct sun4i_tcon *tcon = drv->tcon;
 	const struct tv_mode *tv_mode = sun4i_tv_find_tv_by_mode(mode);
 
-	sun4i_tcon1_mode_set(tcon, mode);
+	sun4i_tcon1_mode_set(tcon, encoder, mode);
 
 	/* Enable and map the DAC to the output */
 	regmap_update_bits(tv->regs, SUN4I_TVE_EN_REG,
-- 
git-series 0.8.11

[PATCH 10/15] drm/sun4i: tcon: Switch mux on only for composite

[Maxime Ripard]

Even though that mux is undocumented, it seems like it needs to be set to 1
when using composite, and 0 when using HDMI.

Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 drivers/gpu/drm/sun4i/sun4i_tcon.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
index d2335f109601..93249c5ab1e4 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
@@ -268,11 +268,16 @@ void sun4i_tcon1_mode_set(struct sun4i_tcon *tcon, struct drm_encoder *encoder,
 			   SUN4I_TCON_GCTL_IOMAP_MASK,
 			   SUN4I_TCON_GCTL_IOMAP_TCON1);
 
+	if (encoder->encoder_type == DRM_MODE_ENCODER_TVDAC)
+		val = 1;
+	else
+		val = 0;
+
 	/*
 	 * FIXME: Undocumented bits
 	 */
 	if (tcon->quirks->has_unknown_mux)
-		regmap_write(tcon->regs, SUN4I_TCON_MUX_CTRL_REG, 1);
+		regmap_write(tcon->regs, SUN4I_TCON_MUX_CTRL_REG, val);
 }
 EXPORT_SYMBOL(sun4i_tcon1_mode_set);
 
-- 
git-series 0.8.11

Re: [PATCH 10/15] drm/sun4i: tcon: Switch mux on only for composite

[Chen-Yu Tsai]

On Tue, Mar 7, 2017 at 4:56 PM, Maxime Ripard
<maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> Even though that mux is undocumented, it seems like it needs to be set to 1
> when using composite, and 0 when using HDMI.
>
> Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
>  drivers/gpu/drm/sun4i/sun4i_tcon.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> index d2335f109601..93249c5ab1e4 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> @@ -268,11 +268,16 @@ void sun4i_tcon1_mode_set(struct sun4i_tcon *tcon, struct drm_encoder *encoder,
>                            SUN4I_TCON_GCTL_IOMAP_MASK,
>                            SUN4I_TCON_GCTL_IOMAP_TCON1);
>
> +       if (encoder->encoder_type == DRM_MODE_ENCODER_TVDAC)
> +               val = 1;
> +       else
> +               val = 0;
> +
>         /*
>          * FIXME: Undocumented bits
>          */
>         if (tcon->quirks->has_unknown_mux)
> -               regmap_write(tcon->regs, SUN4I_TCON_MUX_CTRL_REG, 1);
> +               regmap_write(tcon->regs, SUN4I_TCON_MUX_CTRL_REG, val);

We might want to do this the other way around, i.e. exporting

    int sun4i_tcon_mux_set(struct drm_device *drm, int encoder_type,
                           int pipeline)

and have downstream encoders call it. For the A31, the mux is not exclusively
used for channel 1; there is a mux setting for MIPI DSI as well, but AFAIK
DSI is connected to channel 0.

Additionally, the mux registers are only valid in the first TCON, meaning
it must available be active in 2 pipeline chips. It's also why we'd pass
"struct drm_device *" instead of "struct sun4i_tcon *".


Regards
ChenYu

>  }
>  EXPORT_SYMBOL(sun4i_tcon1_mode_set);
>
> --
> git-series 0.8.11

Re: [PATCH 10/15] drm/sun4i: tcon: Switch mux on only for composite

[Stefan Monnier]

>> +       if (encoder->encoder_type == DRM_MODE_ENCODER_TVDAC)
>> +               val = 1;
>> +       else
>> +               val = 0;

Isn't this better written as

           val = (encoder->encoder_type == DRM_MODE_ENCODER_TVDAC);


-- Stefan

Re: [PATCH 10/15] drm/sun4i: tcon: Switch mux on only for composite

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 2310 bytes --]

On Wed, Mar 08, 2017 at 11:51:39AM +0800, Chen-Yu Tsai wrote:
> On Tue, Mar 7, 2017 at 4:56 PM, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
> > Even though that mux is undocumented, it seems like it needs to be set to 1
> > when using composite, and 0 when using HDMI.
> >
> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > ---
> >  drivers/gpu/drm/sun4i/sun4i_tcon.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > index d2335f109601..93249c5ab1e4 100644
> > --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > @@ -268,11 +268,16 @@ void sun4i_tcon1_mode_set(struct sun4i_tcon *tcon, struct drm_encoder *encoder,
> >                            SUN4I_TCON_GCTL_IOMAP_MASK,
> >                            SUN4I_TCON_GCTL_IOMAP_TCON1);
> >
> > +       if (encoder->encoder_type == DRM_MODE_ENCODER_TVDAC)
> > +               val = 1;
> > +       else
> > +               val = 0;
> > +
> >         /*
> >          * FIXME: Undocumented bits
> >          */
> >         if (tcon->quirks->has_unknown_mux)
> > -               regmap_write(tcon->regs, SUN4I_TCON_MUX_CTRL_REG, 1);
> > +               regmap_write(tcon->regs, SUN4I_TCON_MUX_CTRL_REG, val);
> 
> We might want to do this the other way around, i.e. exporting
> 
>     int sun4i_tcon_mux_set(struct drm_device *drm, int encoder_type,
>                            int pipeline)
> 
> and have downstream encoders call it. For the A31, the mux is not exclusively
> used for channel 1; there is a mux setting for MIPI DSI as well, but AFAIK
> DSI is connected to channel 0.

We could make it part of sun4i_tcon_channel_enable too, though. What
do you think?

> Additionally, the mux registers are only valid in the first TCON, meaning
> it must available be active in 2 pipeline chips. It's also why we'd pass
> "struct drm_device *" instead of "struct sun4i_tcon *".

Hmmmm. That's going to be tricky to support. Has this been confirmed
somehow? Is the register used for something else on TCON1?

Thanks,
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Re: [PATCH 10/15] drm/sun4i: tcon: Switch mux on only for composite

[Chen-Yu Tsai]

On Thu, Mar 9, 2017 at 6:58 PM, Maxime Ripard
<maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> On Wed, Mar 08, 2017 at 11:51:39AM +0800, Chen-Yu Tsai wrote:
>> On Tue, Mar 7, 2017 at 4:56 PM, Maxime Ripard
>> <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
>> > Even though that mux is undocumented, it seems like it needs to be set to 1
>> > when using composite, and 0 when using HDMI.
>> >
>> > Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
>> > ---
>> >  drivers/gpu/drm/sun4i/sun4i_tcon.c | 7 ++++++-
>> >  1 file changed, 6 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
>> > index d2335f109601..93249c5ab1e4 100644
>> > --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
>> > +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
>> > @@ -268,11 +268,16 @@ void sun4i_tcon1_mode_set(struct sun4i_tcon *tcon, struct drm_encoder *encoder,
>> >                            SUN4I_TCON_GCTL_IOMAP_MASK,
>> >                            SUN4I_TCON_GCTL_IOMAP_TCON1);
>> >
>> > +       if (encoder->encoder_type == DRM_MODE_ENCODER_TVDAC)
>> > +               val = 1;
>> > +       else
>> > +               val = 0;
>> > +
>> >         /*
>> >          * FIXME: Undocumented bits
>> >          */
>> >         if (tcon->quirks->has_unknown_mux)
>> > -               regmap_write(tcon->regs, SUN4I_TCON_MUX_CTRL_REG, 1);
>> > +               regmap_write(tcon->regs, SUN4I_TCON_MUX_CTRL_REG, val);
>>
>> We might want to do this the other way around, i.e. exporting
>>
>>     int sun4i_tcon_mux_set(struct drm_device *drm, int encoder_type,
>>                            int pipeline)
>>
>> and have downstream encoders call it. For the A31, the mux is not exclusively
>> used for channel 1; there is a mux setting for MIPI DSI as well, but AFAIK
>> DSI is connected to channel 0.
>
> We could make it part of sun4i_tcon_channel_enable too, though. What
> do you think?

We still need some way of figuring out what mux value to set for those
cases. Let's keep your solution for now. We can work on it later when
we have an actual use case to deal with.

>
>> Additionally, the mux registers are only valid in the first TCON, meaning
>> it must available be active in 2 pipeline chips. It's also why we'd pass
>> "struct drm_device *" instead of "struct sun4i_tcon *".
>
> Hmmmm. That's going to be tricky to support. Has this been confirmed
> somehow? Is the register used for something else on TCON1?

At this point, the only reference is Allwinner's kernel, and the old 3.4
kernel for A10/A20. I could try getting HDMI working on the A31 to get
some real results.

FWIW, the registers do not seem to be aliased across the two TCONs.

Regards
ChenYu

Re: [linux-sunxi] Re: [PATCH 10/15] drm/sun4i: tcon: Switch mux on only for composite

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 871 bytes --]

On Thu, Mar 09, 2017 at 07:31:27PM +0800, Chen-Yu Tsai wrote:
> >> Additionally, the mux registers are only valid in the first TCON, meaning
> >> it must available be active in 2 pipeline chips. It's also why we'd pass
> >> "struct drm_device *" instead of "struct sun4i_tcon *".
> >
> > Hmmmm. That's going to be tricky to support. Has this been confirmed
> > somehow? Is the register used for something else on TCON1?
> 
> At this point, the only reference is Allwinner's kernel, and the old 3.4
> kernel for A10/A20. I could try getting HDMI working on the A31 to get
> some real results.
> 
> FWIW, the registers do not seem to be aliased across the two TCONs.

Then maybe we don't need to care, and we can just always write to the
mux?

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 11/15] drm/sun4i: tcon: Fix tcon channel 1 backporch calculation

[Maxime Ripard]

It seems like what's called a backporch in the datasheet is actually the
backporch plus the sync period. Fix that in our driver.

Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 drivers/gpu/drm/sun4i/sun4i_tcon.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
index 93249c5ab1e4..e44217fb4f6f 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
@@ -240,15 +240,14 @@ void sun4i_tcon1_mode_set(struct sun4i_tcon *tcon, struct drm_encoder *encoder,
 		     SUN4I_TCON1_BASIC2_Y(mode->crtc_vdisplay));
 
 	/* Set horizontal display timings */
-	bp = mode->crtc_htotal - mode->crtc_hsync_end;
+	bp = mode->crtc_htotal - mode->crtc_hsync_start;
 	DRM_DEBUG_DRIVER("Setting horizontal total %d, backporch %d\n",
 			 mode->htotal, bp);
 	regmap_write(tcon->regs, SUN4I_TCON1_BASIC3_REG,
 		     SUN4I_TCON1_BASIC3_H_TOTAL(mode->crtc_htotal) |
 		     SUN4I_TCON1_BASIC3_H_BACKPORCH(bp));
 
-	/* Set vertical display timings */
-	bp = mode->crtc_vtotal - mode->crtc_vsync_end;
+	bp = mode->crtc_vtotal - mode->crtc_vsync_start;
 	DRM_DEBUG_DRIVER("Setting vertical total %d, backporch %d\n",
 			 mode->vtotal, bp);
 	regmap_write(tcon->regs, SUN4I_TCON1_BASIC4_REG,
-- 
git-series 0.8.11

Re: [PATCH 11/15] drm/sun4i: tcon: Fix tcon channel 1 backporch calculation

[Chen-Yu Tsai]

On Tue, Mar 7, 2017 at 4:56 PM, Maxime Ripard
<maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> It seems like what's called a backporch in the datasheet is actually the
> backporch plus the sync period. Fix that in our driver.
>
> Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
>  drivers/gpu/drm/sun4i/sun4i_tcon.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> index 93249c5ab1e4..e44217fb4f6f 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> @@ -240,15 +240,14 @@ void sun4i_tcon1_mode_set(struct sun4i_tcon *tcon, struct drm_encoder *encoder,
>                      SUN4I_TCON1_BASIC2_Y(mode->crtc_vdisplay));
>
>         /* Set horizontal display timings */
> -       bp = mode->crtc_htotal - mode->crtc_hsync_end;
> +       bp = mode->crtc_htotal - mode->crtc_hsync_start;
>         DRM_DEBUG_DRIVER("Setting horizontal total %d, backporch %d\n",
>                          mode->htotal, bp);
>         regmap_write(tcon->regs, SUN4I_TCON1_BASIC3_REG,
>                      SUN4I_TCON1_BASIC3_H_TOTAL(mode->crtc_htotal) |
>                      SUN4I_TCON1_BASIC3_H_BACKPORCH(bp));
>
> -       /* Set vertical display timings */

Why remove the comment?

Otherwise,

Acked-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>

> -       bp = mode->crtc_vtotal - mode->crtc_vsync_end;
> +       bp = mode->crtc_vtotal - mode->crtc_vsync_start;
>         DRM_DEBUG_DRIVER("Setting vertical total %d, backporch %d\n",
>                          mode->vtotal, bp);
>         regmap_write(tcon->regs, SUN4I_TCON1_BASIC4_REG,
> --
> git-series 0.8.11

Re: [PATCH 11/15] drm/sun4i: tcon: Fix tcon channel 1 backporch calculation

[Maxime Ripard]

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

On Wed, Mar 08, 2017 at 12:25:59PM +0800, Chen-Yu Tsai wrote:
> On Tue, Mar 7, 2017 at 4:56 PM, Maxime Ripard
> <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> > It seems like what's called a backporch in the datasheet is actually the
> > backporch plus the sync period. Fix that in our driver.
> >
> > Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> > ---
> >  drivers/gpu/drm/sun4i/sun4i_tcon.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > index 93249c5ab1e4..e44217fb4f6f 100644
> > --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > @@ -240,15 +240,14 @@ void sun4i_tcon1_mode_set(struct sun4i_tcon *tcon, struct drm_encoder *encoder,
> >                      SUN4I_TCON1_BASIC2_Y(mode->crtc_vdisplay));
> >
> >         /* Set horizontal display timings */
> > -       bp = mode->crtc_htotal - mode->crtc_hsync_end;
> > +       bp = mode->crtc_htotal - mode->crtc_hsync_start;
> >         DRM_DEBUG_DRIVER("Setting horizontal total %d, backporch %d\n",
> >                          mode->htotal, bp);
> >         regmap_write(tcon->regs, SUN4I_TCON1_BASIC3_REG,
> >                      SUN4I_TCON1_BASIC3_H_TOTAL(mode->crtc_htotal) |
> >                      SUN4I_TCON1_BASIC3_H_BACKPORCH(bp));
> >
> > -       /* Set vertical display timings */
> 
> Why remove the comment?

I have no idea :)

This will be fixed. Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[PATCH 12/15] drm/sun4i: tcon: multiply the vtotal when not in interlace

[Maxime Ripard]

It appears that the total vertical resolution needs to be doubled when
we're not in interlaced. Make sure that is the case.

Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 drivers/gpu/drm/sun4i/sun4i_tcon.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
index e44217fb4f6f..515fa56c1e6a 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
@@ -248,10 +248,13 @@ void sun4i_tcon1_mode_set(struct sun4i_tcon *tcon, struct drm_encoder *encoder,
 		     SUN4I_TCON1_BASIC3_H_BACKPORCH(bp));
 
 	bp = mode->crtc_vtotal - mode->crtc_vsync_start;
+	val = mode->crtc_vtotal;
+	if (!(mode->flags & DRM_MODE_FLAG_INTERLACE))
+		val = val * 2;
 	DRM_DEBUG_DRIVER("Setting vertical total %d, backporch %d\n",
 			 mode->vtotal, bp);
 	regmap_write(tcon->regs, SUN4I_TCON1_BASIC4_REG,
-		     SUN4I_TCON1_BASIC4_V_TOTAL(mode->vtotal) |
+		     SUN4I_TCON1_BASIC4_V_TOTAL(val) |
 		     SUN4I_TCON1_BASIC4_V_BACKPORCH(bp));
 
 	/* Set Hsync and Vsync length */
-- 
git-series 0.8.11

Re: [PATCH 12/15] drm/sun4i: tcon: multiply the vtotal when not in interlace

[Chen-Yu Tsai]

Hi,

On Tue, Mar 7, 2017 at 4:56 PM, Maxime Ripard
<maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> It appears that the total vertical resolution needs to be doubled when
> we're not in interlaced. Make sure that is the case.

This is true for both channels, though we handle them differently.

>
> Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
>  drivers/gpu/drm/sun4i/sun4i_tcon.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> index e44217fb4f6f..515fa56c1e6a 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> @@ -248,10 +248,13 @@ void sun4i_tcon1_mode_set(struct sun4i_tcon *tcon, struct drm_encoder *encoder,
>                      SUN4I_TCON1_BASIC3_H_BACKPORCH(bp));
>
>         bp = mode->crtc_vtotal - mode->crtc_vsync_start;
> +       val = mode->crtc_vtotal;
> +       if (!(mode->flags & DRM_MODE_FLAG_INTERLACE))
> +               val = val * 2;
>         DRM_DEBUG_DRIVER("Setting vertical total %d, backporch %d\n",
>                          mode->vtotal, bp);
>         regmap_write(tcon->regs, SUN4I_TCON1_BASIC4_REG,
> -                    SUN4I_TCON1_BASIC4_V_TOTAL(mode->vtotal) |
> +                    SUN4I_TCON1_BASIC4_V_TOTAL(val) |

For channel 0, the SUN4I_TCON0_BASIC2_V_TOTAL macro multiplies the passed in
value by 2. I think we should do the same for channel 1, and instead halve the
value passed in if we are outputting interlaced data. I think this makes more
sense because:

1) The register description for both channels are the same. Handling them
   consistently will result in less confusion, such as this one.

2) The definition of interlaced modes is a frame is separated into odd and
   even fields, with each field contains half the number of lines of the
   full frame. One field is displayed during each VSYNC cycle. The TCON does
   not know whether it's interlaced video or not. It only knows the display
   timings. In this case, the number of horizontal lines per cycle is key.

Regards
ChenYu

>                      SUN4I_TCON1_BASIC4_V_BACKPORCH(bp));
>
>         /* Set Hsync and Vsync length */
> --
> git-series 0.8.11

Re: [PATCH 12/15] drm/sun4i: tcon: multiply the vtotal when not in interlace

[Maxime Ripard]

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

On Tue, Mar 07, 2017 at 06:05:17PM +0800, Chen-Yu Tsai wrote:
> Hi,
> 
> On Tue, Mar 7, 2017 at 4:56 PM, Maxime Ripard
> <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> > It appears that the total vertical resolution needs to be doubled when
> > we're not in interlaced. Make sure that is the case.
> 
> This is true for both channels, though we handle them differently.
> 
> >
> > Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> > ---
> >  drivers/gpu/drm/sun4i/sun4i_tcon.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > index e44217fb4f6f..515fa56c1e6a 100644
> > --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > @@ -248,10 +248,13 @@ void sun4i_tcon1_mode_set(struct sun4i_tcon *tcon, struct drm_encoder *encoder,
> >                      SUN4I_TCON1_BASIC3_H_BACKPORCH(bp));
> >
> >         bp = mode->crtc_vtotal - mode->crtc_vsync_start;
> > +       val = mode->crtc_vtotal;
> > +       if (!(mode->flags & DRM_MODE_FLAG_INTERLACE))
> > +               val = val * 2;
> >         DRM_DEBUG_DRIVER("Setting vertical total %d, backporch %d\n",
> >                          mode->vtotal, bp);
> >         regmap_write(tcon->regs, SUN4I_TCON1_BASIC4_REG,
> > -                    SUN4I_TCON1_BASIC4_V_TOTAL(mode->vtotal) |
> > +                    SUN4I_TCON1_BASIC4_V_TOTAL(val) |
> 
> For channel 0, the SUN4I_TCON0_BASIC2_V_TOTAL macro multiplies the passed in
> value by 2. I think we should do the same for channel 1, and instead halve the
> value passed in if we are outputting interlaced data. I think this makes more
> sense because:
> 
> 1) The register description for both channels are the same. Handling them
>    consistently will result in less confusion, such as this one.
> 
> 2) The definition of interlaced modes is a frame is separated into odd and
>    even fields, with each field contains half the number of lines of the
>    full frame. One field is displayed during each VSYNC cycle. The TCON does
>    not know whether it's interlaced video or not. It only knows the display
>    timings. In this case, the number of horizontal lines per cycle is key.

Hmm, yes, you're right. I'll fix it in the next release.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[PATCH 13/15] drm/sun4i: Add HDMI support

[Maxime Ripard]

The earlier Allwinner SoCs (A10, A10s, A20, A31) have an embedded HDMI
controller.

That HDMI controller is able to do audio and CEC, but those have been left
out for now.

Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 drivers/gpu/drm/sun4i/Makefile              |   5 +-
 drivers/gpu/drm/sun4i/sun4i_hdmi.h          | 124 ++++++-
 drivers/gpu/drm/sun4i/sun4i_hdmi_ddc_clk.c  | 128 ++++++-
 drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c      | 449 +++++++++++++++++++++-
 drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c | 236 +++++++++++-
 5 files changed, 942 insertions(+), 0 deletions(-)
 create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi.h
 create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi_ddc_clk.c
 create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
 create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c

diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile
index 59b757350a1f..68a0f6244a59 100644
--- a/drivers/gpu/drm/sun4i/Makefile
+++ b/drivers/gpu/drm/sun4i/Makefile
@@ -7,7 +7,12 @@ sun4i-tcon-y += sun4i_dotclock.o
 sun4i-tcon-y += sun4i_crtc.o
 sun4i-tcon-y += sun4i_layer.o
 
+sun4i-drm-hdmi-y += sun4i_hdmi_enc.o
+sun4i-drm-hdmi-y += sun4i_hdmi_ddc_clk.o
+sun4i-drm-hdmi-y += sun4i_hdmi_tmds_clk.o
+
 obj-$(CONFIG_DRM_SUN4I)		+= sun4i-drm.o sun4i-tcon.o
 obj-$(CONFIG_DRM_SUN4I)		+= sun4i_backend.o
 obj-$(CONFIG_DRM_SUN4I)		+= sun6i_drc.o
+obj-$(CONFIG_DRM_SUN4I)		+= sun4i-drm-hdmi.o
 obj-$(CONFIG_DRM_SUN4I)		+= sun4i_tv.o
diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi.h b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
new file mode 100644
index 000000000000..2ad25b8fd3cd
--- /dev/null
+++ b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
@@ -0,0 +1,124 @@
+/*
+ * Copyright (C) 2016 Maxime Ripard
+ *
+ * Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ */
+
+#ifndef _SUN4I_HDMI_H_
+#define _SUN4I_HDMI_H_
+
+#include <drm/drm_connector.h>
+#include <drm/drm_encoder.h>
+
+#define SUN4I_HDMI_CTRL_REG		0x004
+#define SUN4I_HDMI_CTRL_ENABLE			BIT(31)
+
+#define SUN4I_HDMI_IRQ_REG		0x008
+#define SUN4I_HDMI_IRQ_STA_MASK			0x73
+#define SUN4I_HDMI_IRQ_STA_FIFO_OF		BIT(1)
+#define SUN4I_HDMI_IRQ_STA_FIFO_UF		BIT(0)
+
+#define SUN4I_HDMI_HPD_REG		0x00c
+#define SUN4I_HDMI_HPD_HIGH			BIT(0)
+
+#define SUN4I_HDMI_VID_CTRL_REG		0x010
+#define SUN4I_HDMI_VID_CTRL_ENABLE		BIT(31)
+#define SUN4I_HDMI_VID_CTRL_HDMI_MODE		BIT(30)
+
+#define SUN4I_HDMI_VID_TIMING_ACT_REG	0x014
+#define SUN4I_HDMI_VID_TIMING_BP_REG	0x018
+#define SUN4I_HDMI_VID_TIMING_FP_REG	0x01c
+#define SUN4I_HDMI_VID_TIMING_SPW_REG	0x020
+
+#define SUN4I_HDMI_VID_TIMING_X(x)		((((x) - 1) & GENMASK(11, 0)))
+#define SUN4I_HDMI_VID_TIMING_Y(y)		((((y) - 1) & GENMASK(11, 0)) << 16)
+
+#define SUN4I_HDMI_VID_TIMING_POL_REG	0x024
+#define SUN4I_HDMI_VID_TIMING_POL_TX_CLK        (0x3e0 << 16)
+#define SUN4I_HDMI_VID_TIMING_POL_VSYNC		BIT(1)
+#define SUN4I_HDMI_VID_TIMING_POL_HSYNC		BIT(0)
+
+#define SUN4I_HDMI_AVI_INFOFRAME_REG(n)	(0x080 + (n))
+
+#define SUN4I_HDMI_PAD_CTRL0_REG	0x200
+
+#define SUN4I_HDMI_PAD_CTRL1_REG	0x204
+#define SUN4I_HDMI_PAD_CTRL1_HALVE_CLK		BIT(6)
+
+#define SUN4I_HDMI_PLL_CTRL_REG		0x208
+#define SUN4I_HDMI_PLL_CTRL_DIV(n)		((n) << 4)
+#define SUN4I_HDMI_PLL_CTRL_DIV_MASK		GENMASK(7, 4)
+
+#define SUN4I_HDMI_PLL_DBG0_REG		0x20c
+#define SUN4I_HDMI_PLL_DBG0_TMDS_PARENT(n)	(((n) & 1) << 21)
+#define SUN4I_HDMI_PLL_DBG0_TMDS_PARENT_MASK	BIT(21)
+#define SUN4I_HDMI_PLL_DBG0_TMDS_PARENT_SHIFT	21
+
+#define SUN4I_HDMI_PKT_CTRL_REG(n)	(0x2f0 + (4 * (n)))
+#define SUN4I_HDMI_PKT_CTRL_TYPE(n, t)		((t) << (((n) % 4) * 4))
+
+#define SUN4I_HDMI_UNKNOWN_REG		0x300
+#define SUN4I_HDMI_UNKNOWN_INPUT_SYNC		BIT(27)
+
+#define SUN4I_HDMI_DDC_CTRL_REG		0x500
+#define SUN4I_HDMI_DDC_CTRL_ENABLE		BIT(31)
+#define SUN4I_HDMI_DDC_CTRL_START_CMD		BIT(30)
+#define SUN4I_HDMI_DDC_CTRL_RESET		BIT(0)
+
+#define SUN4I_HDMI_DDC_ADDR_REG		0x504
+#define SUN4I_HDMI_DDC_ADDR_SEGMENT(seg)	(((seg) & 0xff) << 24)
+#define SUN4I_HDMI_DDC_ADDR_EDDC(addr)		(((addr) & 0xff) << 16)
+#define SUN4I_HDMI_DDC_ADDR_OFFSET(off)		(((off) & 0xff) << 8)
+#define SUN4I_HDMI_DDC_ADDR_SLAVE(addr)		((addr) & 0xff)
+
+#define SUN4I_HDMI_DDC_FIFO_CTRL_REG	0x510
+#define SUN4I_HDMI_DDC_FIFO_CTRL_CLEAR		BIT(31)
+	
+#define SUN4I_HDMI_DDC_FIFO_DATA_REG	0x518
+#define SUN4I_HDMI_DDC_BYTE_COUNT_REG	0x51c
+
+#define SUN4I_HDMI_DDC_CMD_REG		0x520
+#define SUN4I_HDMI_DDC_CMD_EXPLICIT_EDDC_READ	6
+
+#define SUN4I_HDMI_DDC_CLK_REG		0x528
+#define SUN4I_HDMI_DDC_CLK_M(m)			(((m) & 0x7) << 3)
+#define SUN4I_HDMI_DDC_CLK_N(n)			((n) & 0x7)
+
+#define SUN4I_HDMI_DDC_LINE_CTRL_REG	0x540
+#define SUN4I_HDMI_DDC_LINE_CTRL_SDA_ENABLE	BIT(9)
+#define SUN4I_HDMI_DDC_LINE_CTRL_SCL_ENABLE	BIT(8)
+
+#define SUN4I_HDMI_DDC_FIFO_SIZE	16
+
+enum sun4i_hdmi_pkt_type {
+	SUN4I_HDMI_PKT_AVI = 2,
+	SUN4I_HDMI_PKT_END = 15,
+};
+
+struct sun4i_hdmi {
+	struct drm_connector	connector;
+	struct drm_encoder	encoder;
+	struct device		*dev;
+
+	void __iomem		*base;
+	struct clk		*bus_clk;
+	struct clk		*ddc_clk;
+	struct clk		*mod_clk;
+	struct clk		*pll0_clk;
+	struct clk		*pll1_clk;
+	struct clk		*tmds_clk;
+
+	struct sun4i_drv	*drv;
+
+	bool			hdmi_monitor;
+};
+
+int sun4i_ddc_create(struct sun4i_hdmi *hdmi, struct clk *clk);
+int sun4i_tmds_create(struct sun4i_hdmi *hdmi);
+
+#endif /* _SUN4I_HDMI_H_ */
diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_ddc_clk.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_ddc_clk.c
new file mode 100644
index 000000000000..5125b14ea7a5
--- /dev/null
+++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_ddc_clk.c
@@ -0,0 +1,128 @@
+/*
+ * Copyright (C) 2016 Free Electrons
+ * Copyright (C) 2016 NextThing Co
+ *
+ * Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ */
+
+#include <linux/clk-provider.h>
+
+#include "sun4i_tcon.h"
+#include "sun4i_hdmi.h"
+
+struct sun4i_ddc {
+	struct clk_hw		hw;
+	struct sun4i_hdmi	*hdmi;
+};
+
+static inline struct sun4i_ddc *hw_to_ddc(struct clk_hw *hw)
+{
+	return container_of(hw, struct sun4i_ddc, hw);
+}
+
+static unsigned long sun4i_ddc_calc_divider(unsigned long rate,
+					    unsigned long parent_rate,
+					    u8 *m, u8 *n)
+{
+	unsigned long best_rate = 0;
+	u8 best_m = 0, best_n = 0, _m, _n;
+
+	for (_m = 0; _m < 8; _m++) {
+		for (_n = 0; _n < 8; _n++) {
+			unsigned long tmp_rate;
+
+			tmp_rate = (((parent_rate / 2) / 10) >> _n) / (_m + 1);
+
+			if (tmp_rate > rate)
+				continue;
+
+			if (abs(rate - tmp_rate) < abs(rate - best_rate)) {
+				best_rate = tmp_rate;
+				best_m = _m;
+				best_n = _n;
+			}
+		}
+	}
+
+	if (m && n) {
+		*m = best_m;
+		*n = best_n;
+	}
+
+	return best_rate;
+}
+
+static long sun4i_ddc_round_rate(struct clk_hw *hw, unsigned long rate,
+				 unsigned long *prate)
+{
+	return sun4i_ddc_calc_divider(rate, *prate, NULL, NULL);
+}
+
+static unsigned long sun4i_ddc_recalc_rate(struct clk_hw *hw,
+					    unsigned long parent_rate)
+{
+	struct sun4i_ddc *ddc = hw_to_ddc(hw);
+	u32 reg;
+	u8 m, n;
+
+	reg = readl(ddc->hdmi->base + SUN4I_HDMI_DDC_CLK_REG);
+	m = (reg >> 3) & 0x7;
+	n = reg & 0x7;
+
+	return (((parent_rate / 2) / 10) >> n) / (m + 1);
+}
+
+static int sun4i_ddc_set_rate(struct clk_hw *hw, unsigned long rate,
+			      unsigned long parent_rate)
+{
+	struct sun4i_ddc *ddc = hw_to_ddc(hw);
+	u8 div_m, div_n;
+
+	sun4i_ddc_calc_divider(rate, parent_rate, &div_m, &div_n);
+
+	writel(SUN4I_HDMI_DDC_CLK_M(div_m) | SUN4I_HDMI_DDC_CLK_N(div_n),
+	       ddc->hdmi->base + SUN4I_HDMI_DDC_CLK_REG);
+
+	return 0;
+}
+
+static const struct clk_ops sun4i_ddc_ops = {
+	.recalc_rate	= sun4i_ddc_recalc_rate,
+	.round_rate	= sun4i_ddc_round_rate,
+	.set_rate	= sun4i_ddc_set_rate,
+};
+
+int sun4i_ddc_create(struct sun4i_hdmi *hdmi, struct clk *parent)
+{
+	struct clk_init_data init;
+	struct sun4i_ddc *ddc;
+	const char *parent_name;
+
+	parent_name = __clk_get_name(parent);
+	if (!parent_name)
+		return -ENODEV;
+
+	ddc = devm_kzalloc(hdmi->dev, sizeof(*ddc), GFP_KERNEL);
+	if (!ddc)
+		return -ENOMEM;
+
+	init.name = "hdmi-ddc";
+	init.ops = &sun4i_ddc_ops;
+	init.parent_names = &parent_name;
+	init.num_parents = 1;
+	init.flags = CLK_SET_RATE_PARENT;
+
+	ddc->hdmi = hdmi;
+	ddc->hw.init = &init;
+
+	hdmi->ddc_clk = devm_clk_register(hdmi->dev, &ddc->hw);
+	if (IS_ERR(hdmi->ddc_clk))
+		return PTR_ERR(hdmi->ddc_clk);
+
+	return 0;
+}
diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
new file mode 100644
index 000000000000..33175308c2ed
--- /dev/null
+++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
@@ -0,0 +1,449 @@
+/*
+ * Copyright (C) 2016 Maxime Ripard
+ *
+ * Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ */
+
+#include <drm/drmP.h>
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_crtc_helper.h>
+#include <drm/drm_edid.h>
+#include <drm/drm_encoder.h>
+#include <drm/drm_panel.h>
+
+#include <linux/clk.h>
+#include <linux/component.h>
+#include <linux/iopoll.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+
+#include "sun4i_backend.h"
+#include "sun4i_drv.h"
+#include "sun4i_hdmi.h"
+#include "sun4i_tcon.h"
+
+static inline struct sun4i_hdmi *
+drm_encoder_to_sun4i_hdmi(struct drm_encoder *encoder)
+{
+	return container_of(encoder, struct sun4i_hdmi,
+			    encoder);
+}
+
+static inline struct sun4i_hdmi *
+drm_connector_to_sun4i_hdmi(struct drm_connector *connector)
+{
+	return container_of(connector, struct sun4i_hdmi,
+			    connector);
+}
+
+static int sun4i_hdmi_setup_avi_infoframes(struct sun4i_hdmi *hdmi,
+					   struct drm_display_mode *mode)
+{
+	struct hdmi_avi_infoframe frame;
+	u8 buffer[17];
+	int i, ret;
+
+	ret = drm_hdmi_avi_infoframe_from_display_mode(&frame, mode);
+	if (ret < 0) {
+		DRM_ERROR("Failed to get infoframes from mode\n");
+		return ret;
+	}
+
+	ret = hdmi_avi_infoframe_pack(&frame, buffer, sizeof(buffer));
+	if (ret < 0) {
+		DRM_ERROR("Failed to pack infoframes\n");
+		return ret;
+	}
+
+	for (i = 0; i < sizeof(buffer); i++)
+		writeb(buffer[i], hdmi->base + SUN4I_HDMI_AVI_INFOFRAME_REG(i));
+
+	return 0;
+}
+
+static void sun4i_hdmi_disable(struct drm_encoder *encoder)
+{
+	struct sun4i_hdmi *hdmi = drm_encoder_to_sun4i_hdmi(encoder);
+	struct sun4i_drv *drv = hdmi->drv;
+	struct sun4i_tcon *tcon = drv->tcon;
+	u32 val;
+
+	DRM_DEBUG_DRIVER("Disabling the HDMI Output\n");
+
+	val = readl(hdmi->base + SUN4I_HDMI_VID_CTRL_REG);
+	val &= ~SUN4I_HDMI_VID_CTRL_ENABLE;
+	writel(val, hdmi->base + SUN4I_HDMI_VID_CTRL_REG);
+
+	sun4i_tcon_channel_disable(tcon, 1);
+}
+
+static void sun4i_hdmi_enable(struct drm_encoder *encoder)
+{
+	struct drm_display_mode *mode = &encoder->crtc->state->adjusted_mode;
+	struct sun4i_hdmi *hdmi = drm_encoder_to_sun4i_hdmi(encoder);
+	struct sun4i_drv *drv = hdmi->drv;
+	struct sun4i_tcon *tcon = drv->tcon;
+	u32 val = 0;
+
+	DRM_DEBUG_DRIVER("Enabling the HDMI Output\n");
+
+	sun4i_tcon_channel_enable(tcon, 1);
+
+	sun4i_hdmi_setup_avi_infoframes(hdmi, mode);
+	val |= SUN4I_HDMI_PKT_CTRL_TYPE(0, SUN4I_HDMI_PKT_AVI);
+	val |= SUN4I_HDMI_PKT_CTRL_TYPE(1, SUN4I_HDMI_PKT_END);
+	writel(val, hdmi->base + SUN4I_HDMI_PKT_CTRL_REG(0));
+
+	val = SUN4I_HDMI_VID_CTRL_ENABLE;
+	if (hdmi->hdmi_monitor)
+		val |= SUN4I_HDMI_VID_CTRL_HDMI_MODE;
+
+	writel(val, hdmi->base + SUN4I_HDMI_VID_CTRL_REG);
+}
+
+static void sun4i_hdmi_mode_set(struct drm_encoder *encoder,
+				struct drm_display_mode *mode,
+				struct drm_display_mode *adjusted_mode)
+{
+	struct sun4i_hdmi *hdmi = drm_encoder_to_sun4i_hdmi(encoder);
+	struct sun4i_drv *drv = hdmi->drv;
+	struct sun4i_tcon *tcon = drv->tcon;
+	unsigned int x, y;
+	u32 val;
+
+	sun4i_tcon1_mode_set(tcon, encoder, mode);
+	clk_set_rate(tcon->sclk1, mode->crtc_clock * 1000);
+	clk_set_rate(hdmi->tmds_clk, mode->crtc_clock * 1000);
+
+	/* Set input sync enable */
+	writel(SUN4I_HDMI_UNKNOWN_INPUT_SYNC,
+	       hdmi->base + SUN4I_HDMI_UNKNOWN_REG);
+
+	/* Setup timing registers */
+	writel(SUN4I_HDMI_VID_TIMING_X(mode->hdisplay) |
+	       SUN4I_HDMI_VID_TIMING_Y(mode->vdisplay),
+	       hdmi->base + SUN4I_HDMI_VID_TIMING_ACT_REG);
+
+	x = mode->htotal - mode->hsync_start;
+	y = mode->vtotal - mode->vsync_start;
+	writel(SUN4I_HDMI_VID_TIMING_X(x) | SUN4I_HDMI_VID_TIMING_Y(y),
+	       hdmi->base + SUN4I_HDMI_VID_TIMING_BP_REG);
+
+	x = mode->hsync_start - mode->hdisplay;
+	y = mode->vsync_start - mode->vdisplay;
+	writel(SUN4I_HDMI_VID_TIMING_X(x) | SUN4I_HDMI_VID_TIMING_Y(y),
+	       hdmi->base + SUN4I_HDMI_VID_TIMING_FP_REG);
+
+	x = mode->hsync_end - mode->hsync_start;
+	y = mode->vsync_end - mode->vsync_start;
+	writel(SUN4I_HDMI_VID_TIMING_X(x) | SUN4I_HDMI_VID_TIMING_Y(y),
+	       hdmi->base + SUN4I_HDMI_VID_TIMING_SPW_REG);
+
+	val = SUN4I_HDMI_VID_TIMING_POL_TX_CLK;
+	if (mode->flags & DRM_MODE_FLAG_PHSYNC)
+		val |= SUN4I_HDMI_VID_TIMING_POL_HSYNC;
+
+	if (mode->flags & DRM_MODE_FLAG_PVSYNC)
+		val |= SUN4I_HDMI_VID_TIMING_POL_VSYNC;
+
+	writel(val, hdmi->base + SUN4I_HDMI_VID_TIMING_POL_REG);
+}
+
+static struct drm_encoder_helper_funcs sun4i_hdmi_helper_funcs = {
+	.disable	= sun4i_hdmi_disable,
+	.enable		= sun4i_hdmi_enable,
+	.mode_set	= sun4i_hdmi_mode_set,
+};
+
+static struct drm_encoder_funcs sun4i_hdmi_funcs = {
+	.destroy	= drm_encoder_cleanup,
+};
+
+static int sun4i_hdmi_read_sub_block(struct sun4i_hdmi *hdmi,
+				     unsigned int blk, unsigned int offset,
+				     u8 *buf, unsigned int count)
+{
+	unsigned long reg;
+	int i;
+
+	reg = readl(hdmi->base + SUN4I_HDMI_DDC_FIFO_CTRL_REG);
+	writel(reg | SUN4I_HDMI_DDC_FIFO_CTRL_CLEAR,
+	       hdmi->base + SUN4I_HDMI_DDC_FIFO_CTRL_REG);
+	writel(SUN4I_HDMI_DDC_ADDR_SEGMENT(offset >> 8) |
+	       SUN4I_HDMI_DDC_ADDR_EDDC(0x60) |
+	       SUN4I_HDMI_DDC_ADDR_OFFSET(offset) |
+	       SUN4I_HDMI_DDC_ADDR_SLAVE(0x50),
+	       hdmi->base + SUN4I_HDMI_DDC_ADDR_REG);
+
+	writel(count, hdmi->base + SUN4I_HDMI_DDC_BYTE_COUNT_REG);
+	writel(SUN4I_HDMI_DDC_CMD_EXPLICIT_EDDC_READ,
+	       hdmi->base + SUN4I_HDMI_DDC_CMD_REG);
+
+	reg = readl(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG);
+	writel(reg | SUN4I_HDMI_DDC_CTRL_START_CMD,
+	       hdmi->base + SUN4I_HDMI_DDC_CTRL_REG);
+
+	if (readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG, reg,
+			       !(reg & SUN4I_HDMI_DDC_CTRL_START_CMD),
+			       100, 2000))
+		return -EIO;
+
+	for (i = 0; i < count; i++)
+		buf[i] = readb(hdmi->base + SUN4I_HDMI_DDC_FIFO_DATA_REG);
+
+	return 0;
+}
+
+static int sun4i_hdmi_read_edid_block(void *data, u8 *buf, unsigned int blk,
+				      size_t length)
+{
+	struct sun4i_hdmi *hdmi = data;
+	int retry = 2, i;
+
+	do {
+		for (i = 0; i < length; i += SUN4I_HDMI_DDC_FIFO_SIZE) {
+			unsigned char offset = blk * EDID_LENGTH + i;
+			unsigned int count = min((unsigned int)SUN4I_HDMI_DDC_FIFO_SIZE,
+						 length - i);
+			int ret;
+
+			ret = sun4i_hdmi_read_sub_block(hdmi, blk, offset,
+							buf + i, count);
+			if (ret)
+				return ret;
+		}
+	} while (!drm_edid_block_valid(buf, blk, true, NULL) && (retry--));
+
+	return 0;
+}
+
+static int sun4i_hdmi_get_modes(struct drm_connector *connector)
+{
+	struct sun4i_hdmi *hdmi = drm_connector_to_sun4i_hdmi(connector);
+	unsigned long reg;
+	struct edid *edid;
+	int ret;
+	
+	/* Reset i2c controller */
+	writel(SUN4I_HDMI_DDC_CTRL_ENABLE | SUN4I_HDMI_DDC_CTRL_RESET,
+	       hdmi->base + SUN4I_HDMI_DDC_CTRL_REG);
+	if (readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG, reg,
+			       !(reg & SUN4I_HDMI_DDC_CTRL_RESET),
+			       100, 2000))
+		return -EIO;
+
+	writel(SUN4I_HDMI_DDC_LINE_CTRL_SDA_ENABLE |
+	       SUN4I_HDMI_DDC_LINE_CTRL_SCL_ENABLE,
+	       hdmi->base + SUN4I_HDMI_DDC_LINE_CTRL_REG);
+
+	clk_set_rate(hdmi->ddc_clk, 100000);
+
+	edid = drm_do_get_edid(connector, sun4i_hdmi_read_edid_block, hdmi);
+	if (!edid)
+		return 0;
+
+	hdmi->hdmi_monitor = drm_detect_hdmi_monitor(edid);
+	DRM_DEBUG_DRIVER("Monitor is %s monitor\n",
+			 hdmi->hdmi_monitor ? "an HDMI" : "a DVI");
+
+	drm_mode_connector_update_edid_property(connector, edid);
+	ret = drm_add_edid_modes(connector, edid);
+	kfree(edid);
+
+	return ret;
+}
+
+static struct drm_connector_helper_funcs sun4i_hdmi_connector_helper_funcs = {
+	.get_modes	= sun4i_hdmi_get_modes,
+};
+
+static enum drm_connector_status
+sun4i_hdmi_connector_detect(struct drm_connector *connector, bool force)
+{
+	struct sun4i_hdmi *hdmi = drm_connector_to_sun4i_hdmi(connector);
+	unsigned long reg;
+
+	if (readl_poll_timeout(hdmi->base + SUN4I_HDMI_HPD_REG, reg,
+			       reg & SUN4I_HDMI_HPD_HIGH,
+			       0, 500000))
+		return connector_status_disconnected;
+
+	return connector_status_connected;
+}
+
+static struct drm_connector_funcs sun4i_hdmi_connector_funcs = {
+	.dpms			= drm_atomic_helper_connector_dpms,
+	.detect			= sun4i_hdmi_connector_detect,
+	.fill_modes		= drm_helper_probe_single_connector_modes,
+	.destroy		= drm_connector_cleanup,
+	.reset			= drm_atomic_helper_connector_reset,
+	.atomic_duplicate_state	= drm_atomic_helper_connector_duplicate_state,
+	.atomic_destroy_state	= drm_atomic_helper_connector_destroy_state,
+};
+
+static int sun4i_hdmi_bind(struct device *dev, struct device *master,
+			 void *data)
+{
+	struct drm_device *drm = data;
+	struct sun4i_drv *drv = drm->dev_private;
+	struct sun4i_hdmi *hdmi = dev_get_drvdata(dev);
+	int ret;
+
+	hdmi->drv = drv;
+	drm_encoder_helper_add(&hdmi->encoder,
+			       &sun4i_hdmi_helper_funcs);
+	ret = drm_encoder_init(drm,
+			       &hdmi->encoder,
+			       &sun4i_hdmi_funcs,
+			       DRM_MODE_ENCODER_TMDS,
+			       NULL);
+	if (ret) {
+		dev_err(dev, "Couldn't initialise the HDMI encoder\n");
+		return ret;
+	}
+
+	hdmi->encoder.possible_crtcs = BIT(0);
+
+	drm_connector_helper_add(&hdmi->connector,
+				 &sun4i_hdmi_connector_helper_funcs);
+	ret = drm_connector_init(drm, &hdmi->connector,
+				 &sun4i_hdmi_connector_funcs,
+				 DRM_MODE_CONNECTOR_HDMIA);
+	if (ret) {
+		dev_err(dev,
+			"Couldn't initialise the Composite connector\n");
+		goto err_cleanup_connector;
+	}
+	hdmi->connector.interlace_allowed = true;
+
+	drm_mode_connector_attach_encoder(&hdmi->connector, &hdmi->encoder);
+
+	return 0;
+
+err_cleanup_connector:
+	drm_encoder_cleanup(&hdmi->encoder);
+	return ret;
+}
+
+static void sun4i_hdmi_unbind(struct device *dev, struct device *master,
+			    void *data)
+{
+	struct sun4i_hdmi *hdmi = dev_get_drvdata(dev);
+
+	drm_connector_cleanup(&hdmi->connector);
+	drm_encoder_cleanup(&hdmi->encoder);
+}
+
+static struct component_ops sun4i_hdmi_ops = {
+	.bind	= sun4i_hdmi_bind,
+	.unbind	= sun4i_hdmi_unbind,
+};
+
+static int sun4i_hdmi_probe(struct platform_device *pdev)
+{
+	struct sun4i_hdmi *hdmi;
+	struct resource *res;
+	int ret;
+
+	hdmi = devm_kzalloc(&pdev->dev, sizeof(*hdmi), GFP_KERNEL);
+	if (!hdmi)
+		return -ENOMEM;
+	dev_set_drvdata(&pdev->dev, hdmi);
+	hdmi->dev = &pdev->dev;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	hdmi->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(hdmi->base)) {
+		dev_err(&pdev->dev, "Couldn't map the HDMI encoder registers\n");
+		return PTR_ERR(hdmi->base);
+	}
+
+	hdmi->bus_clk = devm_clk_get(&pdev->dev, "ahb");
+	if (IS_ERR(hdmi->bus_clk)) {
+		dev_err(&pdev->dev, "Couldn't get the HDMI bus clock\n");
+		return PTR_ERR(hdmi->bus_clk);
+	}
+	clk_prepare_enable(hdmi->bus_clk);
+
+	hdmi->mod_clk = devm_clk_get(&pdev->dev, "mod");
+	if (IS_ERR(hdmi->mod_clk)) {
+		dev_err(&pdev->dev, "Couldn't get the HDMI mod clock\n");
+		return PTR_ERR(hdmi->mod_clk);
+	}
+	clk_prepare_enable(hdmi->mod_clk);
+
+	hdmi->pll0_clk = devm_clk_get(&pdev->dev, "pll-0");
+	if (IS_ERR(hdmi->pll0_clk)) {
+		dev_err(&pdev->dev, "Couldn't get the HDMI PLL 0 clock\n");
+		return PTR_ERR(hdmi->pll0_clk);
+	}
+
+	hdmi->pll1_clk = devm_clk_get(&pdev->dev, "pll-1");
+	if (IS_ERR(hdmi->pll1_clk)) {
+		dev_err(&pdev->dev, "Couldn't get the HDMI PLL 1 clock\n");
+		return PTR_ERR(hdmi->pll1_clk);
+	}
+
+	ret = sun4i_tmds_create(hdmi);
+	if (ret) {
+		dev_err(&pdev->dev, "Couldn't create the TMDS clock\n");
+		return ret;
+	}
+
+	writel(SUN4I_HDMI_CTRL_ENABLE, hdmi->base + SUN4I_HDMI_CTRL_REG);
+
+#define SUN4I_HDMI_PAD_CTRL0 0xfe800000
+	
+	writel(SUN4I_HDMI_PAD_CTRL0, hdmi->base + SUN4I_HDMI_PAD_CTRL0_REG);
+
+	/* TODO: defines */
+	writel((6 << 3) | (2 << 10) | BIT(14) | BIT(15) |
+	       BIT(19) | BIT(20) | BIT(22) | BIT(23),
+	       hdmi->base + SUN4I_HDMI_PAD_CTRL1_REG);
+
+	/* TODO: defines */
+	writel((8 << 0) | (7 << 8) | (239 << 12) | (7 << 17) | (4 << 20) |
+	       BIT(25) | BIT(27) | BIT(28) | BIT(29) | BIT(30) | BIT(31),
+	       hdmi->base + SUN4I_HDMI_PLL_CTRL_REG);
+
+	ret = sun4i_ddc_create(hdmi, hdmi->tmds_clk);
+	if (ret) {
+		dev_err(&pdev->dev, "Couldn't create the DDC clock\n");
+		return ret;
+	}
+
+	return component_add(&pdev->dev, &sun4i_hdmi_ops);
+}
+
+static int sun4i_hdmi_remove(struct platform_device *pdev)
+{
+	component_del(&pdev->dev, &sun4i_hdmi_ops);
+
+	return 0;
+}
+
+static const struct of_device_id sun4i_hdmi_of_table[] = {
+	{ .compatible = "allwinner,sun5i-a10s-hdmi" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, sun4i_hdmi_of_table);
+
+static struct platform_driver sun4i_hdmi_driver = {
+	.probe		= sun4i_hdmi_probe,
+	.remove		= sun4i_hdmi_remove,
+	.driver		= {
+		.name		= "sun4i-hdmi",
+		.of_match_table	= sun4i_hdmi_of_table,
+	},
+};
+module_platform_driver(sun4i_hdmi_driver);
+
+MODULE_AUTHOR("Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>");
+MODULE_DESCRIPTION("Allwinner A10 HDMI Driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c
new file mode 100644
index 000000000000..40f48f1d4685
--- /dev/null
+++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c
@@ -0,0 +1,236 @@
+/*
+ * Copyright (C) 2016 Free Electrons
+ * Copyright (C) 2016 NextThing Co
+ *
+ * Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ */
+
+#include <linux/clk-provider.h>
+
+#include "sun4i_tcon.h"
+#include "sun4i_hdmi.h"
+
+struct sun4i_tmds {
+	struct clk_hw		hw;
+	struct sun4i_hdmi	*hdmi;
+};
+
+static inline struct sun4i_tmds *hw_to_tmds(struct clk_hw *hw)
+{
+	return container_of(hw, struct sun4i_tmds, hw);
+}
+
+
+static unsigned long sun4i_tmds_calc_divider(unsigned long rate,
+					     unsigned long parent_rate,
+					     u8 *div,
+					     bool *half)
+{
+	unsigned long best_rate = 0;
+	u8 best_m = 0, m;
+	bool is_double;
+
+	for (m = 1; m < 16; m++) {
+		u8 d;
+
+		for (d = 1; d < 3; d++) {
+			unsigned long tmp_rate;
+
+			tmp_rate = parent_rate / m / d;
+
+			if (tmp_rate > rate)
+				continue;
+
+			if (!best_rate ||
+			    (rate - tmp_rate) < (rate - best_rate)) {
+				best_rate = tmp_rate;
+				best_m = m;
+				is_double = d;
+			}
+		}
+	}
+
+	if (div && half) {
+		*div = best_m;
+		*half = is_double;
+	}
+
+	return best_rate;
+}
+
+
+static int sun4i_tmds_determine_rate(struct clk_hw *hw,
+				     struct clk_rate_request *req)
+{
+	struct clk_hw *parent;
+	unsigned long best_parent = 0;
+	unsigned long rate = req->rate;
+	int best_div = 1, best_half = 1;
+	int i, j;
+
+	printk("%s %d rate %lu\n", __func__, __LINE__, rate);
+
+	/*
+	 * We only consider PLL3, since the TCON is very likely to be
+	 * clocked from it, and to have the same rate than our HDMI
+	 * clock, so we should not need to do anything.
+	 */
+
+	parent = clk_hw_get_parent_by_index(hw, 0);
+	if (!parent)
+		return -EINVAL;
+
+	for (i = 1; i < 3; i++) {
+		for (j = 1; j < 16; j++) {
+			unsigned long ideal = rate * i * j;
+			unsigned long rounded;
+
+			rounded = clk_hw_round_rate(parent, ideal);
+
+			if (rounded == ideal) {
+				best_parent = rounded;
+				best_half = i;
+				best_div = j;
+				goto out;
+			}
+
+			if (abs(rate - rounded / i) <
+			    abs(rate - best_parent / best_div)) {
+				best_parent = rounded;
+				best_div = i;
+			}
+		}
+	}
+
+out:
+	req->rate = best_parent / best_half / best_div;
+	req->best_parent_rate = best_parent;
+	req->best_parent_hw = parent;
+
+	printk("%s %d rate %lu parent rate %lu (%s) div %d half %d\n",
+	       __func__, __LINE__, req->rate, req->best_parent_rate,
+	       clk_hw_get_name(req->best_parent_hw),
+	       best_div, best_half);
+
+	return 0;
+}
+
+static unsigned long sun4i_tmds_recalc_rate(struct clk_hw *hw,
+					    unsigned long parent_rate)
+{
+	struct sun4i_tmds *tmds = hw_to_tmds(hw);
+	u32 reg;
+
+	reg = readl(tmds->hdmi->base + SUN4I_HDMI_PAD_CTRL1_REG);
+	if (reg & SUN4I_HDMI_PAD_CTRL1_HALVE_CLK)
+		parent_rate /= 2;
+
+	reg = readl(tmds->hdmi->base + SUN4I_HDMI_PLL_CTRL_REG);
+	reg = (reg >> 4) & 0xf;
+	if (!reg)
+		reg = 1;
+
+	return parent_rate / reg;
+}
+
+static int sun4i_tmds_set_rate(struct clk_hw *hw, unsigned long rate,
+			       unsigned long parent_rate)
+{
+	struct sun4i_tmds *tmds = hw_to_tmds(hw);
+	bool half;
+	u32 reg;
+	u8 div;
+
+	sun4i_tmds_calc_divider(rate, parent_rate, &div, &half);
+
+	printk("%s %d rate %lu parent rate %lu div %d half %s\n",
+	       __func__, __LINE__, rate, parent_rate, div,
+	       half ? "yes" : "no");
+
+	reg = readl(tmds->hdmi->base + SUN4I_HDMI_PAD_CTRL1_REG);
+	reg &= ~SUN4I_HDMI_PAD_CTRL1_HALVE_CLK;
+	if (half)
+		reg |= SUN4I_HDMI_PAD_CTRL1_HALVE_CLK;
+	writel(reg, tmds->hdmi->base + SUN4I_HDMI_PAD_CTRL1_REG);
+
+	reg = readl(tmds->hdmi->base + SUN4I_HDMI_PLL_CTRL_REG);
+	reg &= ~SUN4I_HDMI_PLL_CTRL_DIV_MASK;
+	writel(reg | SUN4I_HDMI_PLL_CTRL_DIV(div),
+	       tmds->hdmi->base + SUN4I_HDMI_PLL_CTRL_REG);
+
+	return 0;
+}
+
+static u8 sun4i_tmds_get_parent(struct clk_hw *hw)
+{
+	struct sun4i_tmds *tmds = hw_to_tmds(hw);
+	u32 reg;
+
+	reg = readl(tmds->hdmi->base + SUN4I_HDMI_PLL_DBG0_REG);
+	return ((reg & SUN4I_HDMI_PLL_DBG0_TMDS_PARENT_MASK) >>
+		SUN4I_HDMI_PLL_DBG0_TMDS_PARENT_SHIFT);
+}
+
+static int sun4i_tmds_set_parent(struct clk_hw *hw, u8 index)
+{
+	struct sun4i_tmds *tmds = hw_to_tmds(hw);
+	u32 reg;
+
+	if (index > 1)
+		return -EINVAL;
+
+	reg = readl(tmds->hdmi->base + SUN4I_HDMI_PLL_DBG0_REG);
+	reg &= ~SUN4I_HDMI_PLL_DBG0_TMDS_PARENT_MASK;
+	writel(reg | SUN4I_HDMI_PLL_DBG0_TMDS_PARENT(index),
+	       tmds->hdmi->base + SUN4I_HDMI_PLL_DBG0_REG);
+
+	return 0;
+}
+
+static const struct clk_ops sun4i_tmds_ops = {
+	.determine_rate	= sun4i_tmds_determine_rate,
+	.recalc_rate	= sun4i_tmds_recalc_rate,
+	.set_rate	= sun4i_tmds_set_rate,
+
+	.get_parent	= sun4i_tmds_get_parent,
+	.set_parent	= sun4i_tmds_set_parent,
+};
+
+int sun4i_tmds_create(struct sun4i_hdmi *hdmi)
+{
+	struct clk_init_data init;
+	struct sun4i_tmds *tmds;
+	const char *parents[2];
+
+	parents[0] = __clk_get_name(hdmi->pll0_clk);
+	if (!parents[0])
+		return -ENODEV;
+
+	parents[1] = __clk_get_name(hdmi->pll1_clk);
+	if (!parents[1])
+		return -ENODEV;
+
+	tmds = devm_kzalloc(hdmi->dev, sizeof(*tmds), GFP_KERNEL);
+	if (!tmds)
+		return -ENOMEM;
+
+	init.name = "hdmi-tmds";
+	init.ops = &sun4i_tmds_ops;
+	init.parent_names = parents;
+	init.num_parents = 2;
+	init.flags = CLK_SET_RATE_PARENT;
+
+	tmds->hdmi = hdmi;
+	tmds->hw.init = &init;
+
+	hdmi->tmds_clk = devm_clk_register(hdmi->dev, &tmds->hw);
+	if (IS_ERR(hdmi->tmds_clk))
+		return PTR_ERR(hdmi->tmds_clk);
+
+	return 0;
+}
-- 
git-series 0.8.11

Re: [PATCH 13/15] drm/sun4i: Add HDMI support

[Chen-Yu Tsai]

Hi,

On Tue, Mar 7, 2017 at 4:56 PM, Maxime Ripard
<maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> The earlier Allwinner SoCs (A10, A10s, A20, A31) have an embedded HDMI
> controller.
>
> That HDMI controller is able to do audio and CEC, but those have been left
> out for now.
>
> Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
>  drivers/gpu/drm/sun4i/Makefile              |   5 +-
>  drivers/gpu/drm/sun4i/sun4i_hdmi.h          | 124 ++++++-
>  drivers/gpu/drm/sun4i/sun4i_hdmi_ddc_clk.c  | 128 ++++++-
>  drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c      | 449 +++++++++++++++++++++-
>  drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c | 236 +++++++++++-
>  5 files changed, 942 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi.h
>  create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi_ddc_clk.c
>  create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
>  create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c

Applying patch #9608371 using 'git am'
Description: [13/15] drm/sun4i: Add HDMI support
Applying: drm/sun4i: Add HDMI support
.git/rebase-apply/patch:116: trailing whitespace.

.git/rebase-apply/patch:531: trailing whitespace.

.git/rebase-apply/patch:701: trailing whitespace.

warning: 3 lines add whitespace errors.

> diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile
> index 59b757350a1f..68a0f6244a59 100644
> --- a/drivers/gpu/drm/sun4i/Makefile
> +++ b/drivers/gpu/drm/sun4i/Makefile
> @@ -7,7 +7,12 @@ sun4i-tcon-y += sun4i_dotclock.o
>  sun4i-tcon-y += sun4i_crtc.o
>  sun4i-tcon-y += sun4i_layer.o
>
> +sun4i-drm-hdmi-y += sun4i_hdmi_enc.o
> +sun4i-drm-hdmi-y += sun4i_hdmi_ddc_clk.o
> +sun4i-drm-hdmi-y += sun4i_hdmi_tmds_clk.o
> +
>  obj-$(CONFIG_DRM_SUN4I)                += sun4i-drm.o sun4i-tcon.o
>  obj-$(CONFIG_DRM_SUN4I)                += sun4i_backend.o
>  obj-$(CONFIG_DRM_SUN4I)                += sun6i_drc.o
> +obj-$(CONFIG_DRM_SUN4I)                += sun4i-drm-hdmi.o
>  obj-$(CONFIG_DRM_SUN4I)                += sun4i_tv.o
> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi.h b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
> new file mode 100644
> index 000000000000..2ad25b8fd3cd
> --- /dev/null
> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
> @@ -0,0 +1,124 @@
> +/*
> + * Copyright (C) 2016 Maxime Ripard
> + *
> + * Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + */
> +
> +#ifndef _SUN4I_HDMI_H_
> +#define _SUN4I_HDMI_H_
> +
> +#include <drm/drm_connector.h>
> +#include <drm/drm_encoder.h>
> +
> +#define SUN4I_HDMI_CTRL_REG            0x004
> +#define SUN4I_HDMI_CTRL_ENABLE                 BIT(31)
> +
> +#define SUN4I_HDMI_IRQ_REG             0x008
> +#define SUN4I_HDMI_IRQ_STA_MASK                        0x73
> +#define SUN4I_HDMI_IRQ_STA_FIFO_OF             BIT(1)
> +#define SUN4I_HDMI_IRQ_STA_FIFO_UF             BIT(0)
> +
> +#define SUN4I_HDMI_HPD_REG             0x00c
> +#define SUN4I_HDMI_HPD_HIGH                    BIT(0)
> +
> +#define SUN4I_HDMI_VID_CTRL_REG                0x010
> +#define SUN4I_HDMI_VID_CTRL_ENABLE             BIT(31)
> +#define SUN4I_HDMI_VID_CTRL_HDMI_MODE          BIT(30)
> +
> +#define SUN4I_HDMI_VID_TIMING_ACT_REG  0x014
> +#define SUN4I_HDMI_VID_TIMING_BP_REG   0x018
> +#define SUN4I_HDMI_VID_TIMING_FP_REG   0x01c
> +#define SUN4I_HDMI_VID_TIMING_SPW_REG  0x020
> +
> +#define SUN4I_HDMI_VID_TIMING_X(x)             ((((x) - 1) & GENMASK(11, 0)))
> +#define SUN4I_HDMI_VID_TIMING_Y(y)             ((((y) - 1) & GENMASK(11, 0)) << 16)
> +
> +#define SUN4I_HDMI_VID_TIMING_POL_REG  0x024
> +#define SUN4I_HDMI_VID_TIMING_POL_TX_CLK        (0x3e0 << 16)
> +#define SUN4I_HDMI_VID_TIMING_POL_VSYNC                BIT(1)
> +#define SUN4I_HDMI_VID_TIMING_POL_HSYNC                BIT(0)
> +
> +#define SUN4I_HDMI_AVI_INFOFRAME_REG(n)        (0x080 + (n))
> +
> +#define SUN4I_HDMI_PAD_CTRL0_REG       0x200
> +
> +#define SUN4I_HDMI_PAD_CTRL1_REG       0x204
> +#define SUN4I_HDMI_PAD_CTRL1_HALVE_CLK         BIT(6)
> +
> +#define SUN4I_HDMI_PLL_CTRL_REG                0x208
> +#define SUN4I_HDMI_PLL_CTRL_DIV(n)             ((n) << 4)
> +#define SUN4I_HDMI_PLL_CTRL_DIV_MASK           GENMASK(7, 4)
> +
> +#define SUN4I_HDMI_PLL_DBG0_REG                0x20c
> +#define SUN4I_HDMI_PLL_DBG0_TMDS_PARENT(n)     (((n) & 1) << 21)
> +#define SUN4I_HDMI_PLL_DBG0_TMDS_PARENT_MASK   BIT(21)
> +#define SUN4I_HDMI_PLL_DBG0_TMDS_PARENT_SHIFT  21
> +
> +#define SUN4I_HDMI_PKT_CTRL_REG(n)     (0x2f0 + (4 * (n)))
> +#define SUN4I_HDMI_PKT_CTRL_TYPE(n, t)         ((t) << (((n) % 4) * 4))
> +
> +#define SUN4I_HDMI_UNKNOWN_REG         0x300
> +#define SUN4I_HDMI_UNKNOWN_INPUT_SYNC          BIT(27)
> +
> +#define SUN4I_HDMI_DDC_CTRL_REG                0x500
> +#define SUN4I_HDMI_DDC_CTRL_ENABLE             BIT(31)
> +#define SUN4I_HDMI_DDC_CTRL_START_CMD          BIT(30)
> +#define SUN4I_HDMI_DDC_CTRL_RESET              BIT(0)
> +
> +#define SUN4I_HDMI_DDC_ADDR_REG                0x504
> +#define SUN4I_HDMI_DDC_ADDR_SEGMENT(seg)       (((seg) & 0xff) << 24)
> +#define SUN4I_HDMI_DDC_ADDR_EDDC(addr)         (((addr) & 0xff) << 16)
> +#define SUN4I_HDMI_DDC_ADDR_OFFSET(off)                (((off) & 0xff) << 8)
> +#define SUN4I_HDMI_DDC_ADDR_SLAVE(addr)                ((addr) & 0xff)
> +
> +#define SUN4I_HDMI_DDC_FIFO_CTRL_REG   0x510
> +#define SUN4I_HDMI_DDC_FIFO_CTRL_CLEAR         BIT(31)
> +
> +#define SUN4I_HDMI_DDC_FIFO_DATA_REG   0x518
> +#define SUN4I_HDMI_DDC_BYTE_COUNT_REG  0x51c
> +
> +#define SUN4I_HDMI_DDC_CMD_REG         0x520
> +#define SUN4I_HDMI_DDC_CMD_EXPLICIT_EDDC_READ  6
> +
> +#define SUN4I_HDMI_DDC_CLK_REG         0x528
> +#define SUN4I_HDMI_DDC_CLK_M(m)                        (((m) & 0x7) << 3)
> +#define SUN4I_HDMI_DDC_CLK_N(n)                        ((n) & 0x7)
> +
> +#define SUN4I_HDMI_DDC_LINE_CTRL_REG   0x540
> +#define SUN4I_HDMI_DDC_LINE_CTRL_SDA_ENABLE    BIT(9)
> +#define SUN4I_HDMI_DDC_LINE_CTRL_SCL_ENABLE    BIT(8)
> +
> +#define SUN4I_HDMI_DDC_FIFO_SIZE       16
> +
> +enum sun4i_hdmi_pkt_type {
> +       SUN4I_HDMI_PKT_AVI = 2,
> +       SUN4I_HDMI_PKT_END = 15,
> +};
> +
> +struct sun4i_hdmi {
> +       struct drm_connector    connector;
> +       struct drm_encoder      encoder;
> +       struct device           *dev;
> +
> +       void __iomem            *base;
> +       struct clk              *bus_clk;
> +       struct clk              *ddc_clk;
> +       struct clk              *mod_clk;
> +       struct clk              *pll0_clk;
> +       struct clk              *pll1_clk;
> +       struct clk              *tmds_clk;
> +
> +       struct sun4i_drv        *drv;
> +
> +       bool                    hdmi_monitor;
> +};
> +
> +int sun4i_ddc_create(struct sun4i_hdmi *hdmi, struct clk *clk);
> +int sun4i_tmds_create(struct sun4i_hdmi *hdmi);
> +
> +#endif /* _SUN4I_HDMI_H_ */
> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_ddc_clk.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_ddc_clk.c
> new file mode 100644
> index 000000000000..5125b14ea7a5
> --- /dev/null
> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_ddc_clk.c
> @@ -0,0 +1,128 @@
> +/*
> + * Copyright (C) 2016 Free Electrons
> + * Copyright (C) 2016 NextThing Co
> + *
> + * Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + */
> +
> +#include <linux/clk-provider.h>
> +
> +#include "sun4i_tcon.h"
> +#include "sun4i_hdmi.h"
> +
> +struct sun4i_ddc {
> +       struct clk_hw           hw;
> +       struct sun4i_hdmi       *hdmi;
> +};
> +
> +static inline struct sun4i_ddc *hw_to_ddc(struct clk_hw *hw)
> +{
> +       return container_of(hw, struct sun4i_ddc, hw);
> +}
> +
> +static unsigned long sun4i_ddc_calc_divider(unsigned long rate,
> +                                           unsigned long parent_rate,
> +                                           u8 *m, u8 *n)
> +{
> +       unsigned long best_rate = 0;
> +       u8 best_m = 0, best_n = 0, _m, _n;
> +
> +       for (_m = 0; _m < 8; _m++) {
> +               for (_n = 0; _n < 8; _n++) {
> +                       unsigned long tmp_rate;
> +
> +                       tmp_rate = (((parent_rate / 2) / 10) >> _n) / (_m + 1);
> +
> +                       if (tmp_rate > rate)
> +                               continue;
> +
> +                       if (abs(rate - tmp_rate) < abs(rate - best_rate)) {
> +                               best_rate = tmp_rate;
> +                               best_m = _m;
> +                               best_n = _n;
> +                       }
> +               }
> +       }
> +
> +       if (m && n) {
> +               *m = best_m;
> +               *n = best_n;
> +       }
> +
> +       return best_rate;
> +}
> +
> +static long sun4i_ddc_round_rate(struct clk_hw *hw, unsigned long rate,
> +                                unsigned long *prate)
> +{
> +       return sun4i_ddc_calc_divider(rate, *prate, NULL, NULL);
> +}
> +
> +static unsigned long sun4i_ddc_recalc_rate(struct clk_hw *hw,
> +                                           unsigned long parent_rate)
> +{
> +       struct sun4i_ddc *ddc = hw_to_ddc(hw);
> +       u32 reg;
> +       u8 m, n;
> +
> +       reg = readl(ddc->hdmi->base + SUN4I_HDMI_DDC_CLK_REG);
> +       m = (reg >> 3) & 0x7;
> +       n = reg & 0x7;
> +
> +       return (((parent_rate / 2) / 10) >> n) / (m + 1);
> +}
> +
> +static int sun4i_ddc_set_rate(struct clk_hw *hw, unsigned long rate,
> +                             unsigned long parent_rate)
> +{
> +       struct sun4i_ddc *ddc = hw_to_ddc(hw);
> +       u8 div_m, div_n;
> +
> +       sun4i_ddc_calc_divider(rate, parent_rate, &div_m, &div_n);
> +
> +       writel(SUN4I_HDMI_DDC_CLK_M(div_m) | SUN4I_HDMI_DDC_CLK_N(div_n),
> +              ddc->hdmi->base + SUN4I_HDMI_DDC_CLK_REG);
> +
> +       return 0;
> +}
> +
> +static const struct clk_ops sun4i_ddc_ops = {
> +       .recalc_rate    = sun4i_ddc_recalc_rate,
> +       .round_rate     = sun4i_ddc_round_rate,
> +       .set_rate       = sun4i_ddc_set_rate,
> +};
> +
> +int sun4i_ddc_create(struct sun4i_hdmi *hdmi, struct clk *parent)
> +{
> +       struct clk_init_data init;
> +       struct sun4i_ddc *ddc;
> +       const char *parent_name;
> +
> +       parent_name = __clk_get_name(parent);
> +       if (!parent_name)
> +               return -ENODEV;
> +
> +       ddc = devm_kzalloc(hdmi->dev, sizeof(*ddc), GFP_KERNEL);
> +       if (!ddc)
> +               return -ENOMEM;
> +
> +       init.name = "hdmi-ddc";
> +       init.ops = &sun4i_ddc_ops;
> +       init.parent_names = &parent_name;
> +       init.num_parents = 1;
> +       init.flags = CLK_SET_RATE_PARENT;

I don't think this is really needed. It probably doesn't hurt though,
since DDC is used when HDMI is not used for displaying, but it might
affect any upstream PLLs, which theoretically may affect other users
of said PLLs. The DDC clock is slow enough that we should be able to
generate a usable clock rate anyway.

> +
> +       ddc->hdmi = hdmi;
> +       ddc->hw.init = &init;
> +
> +       hdmi->ddc_clk = devm_clk_register(hdmi->dev, &ddc->hw);
> +       if (IS_ERR(hdmi->ddc_clk))
> +               return PTR_ERR(hdmi->ddc_clk);
> +
> +       return 0;
> +}
> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
> new file mode 100644
> index 000000000000..33175308c2ed
> --- /dev/null
> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
> @@ -0,0 +1,449 @@
> +/*
> + * Copyright (C) 2016 Maxime Ripard
> + *
> + * Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + */
> +
> +#include <drm/drmP.h>
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_crtc_helper.h>
> +#include <drm/drm_edid.h>
> +#include <drm/drm_encoder.h>
> +#include <drm/drm_panel.h>
> +
> +#include <linux/clk.h>
> +#include <linux/component.h>
> +#include <linux/iopoll.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +
> +#include "sun4i_backend.h"
> +#include "sun4i_drv.h"
> +#include "sun4i_hdmi.h"
> +#include "sun4i_tcon.h"
> +
> +static inline struct sun4i_hdmi *
> +drm_encoder_to_sun4i_hdmi(struct drm_encoder *encoder)
> +{
> +       return container_of(encoder, struct sun4i_hdmi,
> +                           encoder);
> +}
> +
> +static inline struct sun4i_hdmi *
> +drm_connector_to_sun4i_hdmi(struct drm_connector *connector)
> +{
> +       return container_of(connector, struct sun4i_hdmi,
> +                           connector);
> +}
> +
> +static int sun4i_hdmi_setup_avi_infoframes(struct sun4i_hdmi *hdmi,
> +                                          struct drm_display_mode *mode)
> +{
> +       struct hdmi_avi_infoframe frame;
> +       u8 buffer[17];
> +       int i, ret;
> +
> +       ret = drm_hdmi_avi_infoframe_from_display_mode(&frame, mode);
> +       if (ret < 0) {
> +               DRM_ERROR("Failed to get infoframes from mode\n");
> +               return ret;
> +       }
> +
> +       ret = hdmi_avi_infoframe_pack(&frame, buffer, sizeof(buffer));
> +       if (ret < 0) {
> +               DRM_ERROR("Failed to pack infoframes\n");
> +               return ret;
> +       }
> +
> +       for (i = 0; i < sizeof(buffer); i++)
> +               writeb(buffer[i], hdmi->base + SUN4I_HDMI_AVI_INFOFRAME_REG(i));
> +
> +       return 0;
> +}
> +
> +static void sun4i_hdmi_disable(struct drm_encoder *encoder)
> +{
> +       struct sun4i_hdmi *hdmi = drm_encoder_to_sun4i_hdmi(encoder);
> +       struct sun4i_drv *drv = hdmi->drv;
> +       struct sun4i_tcon *tcon = drv->tcon;
> +       u32 val;
> +
> +       DRM_DEBUG_DRIVER("Disabling the HDMI Output\n");
> +
> +       val = readl(hdmi->base + SUN4I_HDMI_VID_CTRL_REG);
> +       val &= ~SUN4I_HDMI_VID_CTRL_ENABLE;
> +       writel(val, hdmi->base + SUN4I_HDMI_VID_CTRL_REG);
> +
> +       sun4i_tcon_channel_disable(tcon, 1);
> +}
> +
> +static void sun4i_hdmi_enable(struct drm_encoder *encoder)
> +{
> +       struct drm_display_mode *mode = &encoder->crtc->state->adjusted_mode;
> +       struct sun4i_hdmi *hdmi = drm_encoder_to_sun4i_hdmi(encoder);
> +       struct sun4i_drv *drv = hdmi->drv;
> +       struct sun4i_tcon *tcon = drv->tcon;
> +       u32 val = 0;
> +
> +       DRM_DEBUG_DRIVER("Enabling the HDMI Output\n");
> +
> +       sun4i_tcon_channel_enable(tcon, 1);
> +
> +       sun4i_hdmi_setup_avi_infoframes(hdmi, mode);
> +       val |= SUN4I_HDMI_PKT_CTRL_TYPE(0, SUN4I_HDMI_PKT_AVI);
> +       val |= SUN4I_HDMI_PKT_CTRL_TYPE(1, SUN4I_HDMI_PKT_END);
> +       writel(val, hdmi->base + SUN4I_HDMI_PKT_CTRL_REG(0));
> +
> +       val = SUN4I_HDMI_VID_CTRL_ENABLE;
> +       if (hdmi->hdmi_monitor)
> +               val |= SUN4I_HDMI_VID_CTRL_HDMI_MODE;
> +
> +       writel(val, hdmi->base + SUN4I_HDMI_VID_CTRL_REG);
> +}
> +
> +static void sun4i_hdmi_mode_set(struct drm_encoder *encoder,
> +                               struct drm_display_mode *mode,
> +                               struct drm_display_mode *adjusted_mode)
> +{
> +       struct sun4i_hdmi *hdmi = drm_encoder_to_sun4i_hdmi(encoder);
> +       struct sun4i_drv *drv = hdmi->drv;
> +       struct sun4i_tcon *tcon = drv->tcon;
> +       unsigned int x, y;
> +       u32 val;
> +
> +       sun4i_tcon1_mode_set(tcon, encoder, mode);
> +       clk_set_rate(tcon->sclk1, mode->crtc_clock * 1000);
> +       clk_set_rate(hdmi->tmds_clk, mode->crtc_clock * 1000);
> +
> +       /* Set input sync enable */
> +       writel(SUN4I_HDMI_UNKNOWN_INPUT_SYNC,
> +              hdmi->base + SUN4I_HDMI_UNKNOWN_REG);
> +
> +       /* Setup timing registers */
> +       writel(SUN4I_HDMI_VID_TIMING_X(mode->hdisplay) |
> +              SUN4I_HDMI_VID_TIMING_Y(mode->vdisplay),
> +              hdmi->base + SUN4I_HDMI_VID_TIMING_ACT_REG);
> +
> +       x = mode->htotal - mode->hsync_start;
> +       y = mode->vtotal - mode->vsync_start;

I'm a bit skeptical about this one. All the other parameters are not
inclusive of other, why would this one be different? Shouldn't it
be "Xtotal - Xsync_end" instead?

> +       writel(SUN4I_HDMI_VID_TIMING_X(x) | SUN4I_HDMI_VID_TIMING_Y(y),
> +              hdmi->base + SUN4I_HDMI_VID_TIMING_BP_REG);
> +
> +       x = mode->hsync_start - mode->hdisplay;
> +       y = mode->vsync_start - mode->vdisplay;
> +       writel(SUN4I_HDMI_VID_TIMING_X(x) | SUN4I_HDMI_VID_TIMING_Y(y),
> +              hdmi->base + SUN4I_HDMI_VID_TIMING_FP_REG);
> +
> +       x = mode->hsync_end - mode->hsync_start;
> +       y = mode->vsync_end - mode->vsync_start;
> +       writel(SUN4I_HDMI_VID_TIMING_X(x) | SUN4I_HDMI_VID_TIMING_Y(y),
> +              hdmi->base + SUN4I_HDMI_VID_TIMING_SPW_REG);
> +
> +       val = SUN4I_HDMI_VID_TIMING_POL_TX_CLK;
> +       if (mode->flags & DRM_MODE_FLAG_PHSYNC)
> +               val |= SUN4I_HDMI_VID_TIMING_POL_HSYNC;
> +
> +       if (mode->flags & DRM_MODE_FLAG_PVSYNC)
> +               val |= SUN4I_HDMI_VID_TIMING_POL_VSYNC;
> +
> +       writel(val, hdmi->base + SUN4I_HDMI_VID_TIMING_POL_REG);

You don't handle the interlaced video here, even though you set

    hdmi->connector.interlace_allowed = true

later.

The double clock and double scan flags aren't handled either, though
I don't understand which one is supposed to represent the need for the
HDMI pixel repeater. AFAIK this is required for resolutions with pixel
clocks lower than 25 MHz, the lower limit of HDMI's TMDS link.

> +}
> +
> +static struct drm_encoder_helper_funcs sun4i_hdmi_helper_funcs = {
> +       .disable        = sun4i_hdmi_disable,
> +       .enable         = sun4i_hdmi_enable,
> +       .mode_set       = sun4i_hdmi_mode_set,
> +};
> +
> +static struct drm_encoder_funcs sun4i_hdmi_funcs = {
> +       .destroy        = drm_encoder_cleanup,
> +};
> +
> +static int sun4i_hdmi_read_sub_block(struct sun4i_hdmi *hdmi,
> +                                    unsigned int blk, unsigned int offset,
> +                                    u8 *buf, unsigned int count)
> +{
> +       unsigned long reg;
> +       int i;
> +
> +       reg = readl(hdmi->base + SUN4I_HDMI_DDC_FIFO_CTRL_REG);
> +       writel(reg | SUN4I_HDMI_DDC_FIFO_CTRL_CLEAR,
> +              hdmi->base + SUN4I_HDMI_DDC_FIFO_CTRL_REG);
> +       writel(SUN4I_HDMI_DDC_ADDR_SEGMENT(offset >> 8) |
> +              SUN4I_HDMI_DDC_ADDR_EDDC(0x60) |
> +              SUN4I_HDMI_DDC_ADDR_OFFSET(offset) |
> +              SUN4I_HDMI_DDC_ADDR_SLAVE(0x50),

You can use DDC_ADDR from drm_edid.h.

> +              hdmi->base + SUN4I_HDMI_DDC_ADDR_REG);
> +
> +       writel(count, hdmi->base + SUN4I_HDMI_DDC_BYTE_COUNT_REG);
> +       writel(SUN4I_HDMI_DDC_CMD_EXPLICIT_EDDC_READ,
> +              hdmi->base + SUN4I_HDMI_DDC_CMD_REG);
> +
> +       reg = readl(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG);
> +       writel(reg | SUN4I_HDMI_DDC_CTRL_START_CMD,
> +              hdmi->base + SUN4I_HDMI_DDC_CTRL_REG);
> +
> +       if (readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG, reg,
> +                              !(reg & SUN4I_HDMI_DDC_CTRL_START_CMD),
> +                              100, 2000))
> +               return -EIO;
> +
> +       for (i = 0; i < count; i++)
> +               buf[i] = readb(hdmi->base + SUN4I_HDMI_DDC_FIFO_DATA_REG);
> +
> +       return 0;
> +}
> +
> +static int sun4i_hdmi_read_edid_block(void *data, u8 *buf, unsigned int blk,
> +                                     size_t length)
> +{
> +       struct sun4i_hdmi *hdmi = data;
> +       int retry = 2, i;
> +
> +       do {
> +               for (i = 0; i < length; i += SUN4I_HDMI_DDC_FIFO_SIZE) {
> +                       unsigned char offset = blk * EDID_LENGTH + i;
> +                       unsigned int count = min((unsigned int)SUN4I_HDMI_DDC_FIFO_SIZE,
> +                                                length - i);
> +                       int ret;
> +
> +                       ret = sun4i_hdmi_read_sub_block(hdmi, blk, offset,
> +                                                       buf + i, count);
> +                       if (ret)
> +                               return ret;
> +               }
> +       } while (!drm_edid_block_valid(buf, blk, true, NULL) && (retry--));
> +
> +       return 0;
> +}
> +
> +static int sun4i_hdmi_get_modes(struct drm_connector *connector)
> +{
> +       struct sun4i_hdmi *hdmi = drm_connector_to_sun4i_hdmi(connector);
> +       unsigned long reg;
> +       struct edid *edid;
> +       int ret;
> +
> +       /* Reset i2c controller */
> +       writel(SUN4I_HDMI_DDC_CTRL_ENABLE | SUN4I_HDMI_DDC_CTRL_RESET,
> +              hdmi->base + SUN4I_HDMI_DDC_CTRL_REG);
> +       if (readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG, reg,
> +                              !(reg & SUN4I_HDMI_DDC_CTRL_RESET),
> +                              100, 2000))
> +               return -EIO;
> +
> +       writel(SUN4I_HDMI_DDC_LINE_CTRL_SDA_ENABLE |
> +              SUN4I_HDMI_DDC_LINE_CTRL_SCL_ENABLE,
> +              hdmi->base + SUN4I_HDMI_DDC_LINE_CTRL_REG);
> +
> +       clk_set_rate(hdmi->ddc_clk, 100000);
> +
> +       edid = drm_do_get_edid(connector, sun4i_hdmi_read_edid_block, hdmi);
> +       if (!edid)
> +               return 0;
> +
> +       hdmi->hdmi_monitor = drm_detect_hdmi_monitor(edid);
> +       DRM_DEBUG_DRIVER("Monitor is %s monitor\n",
> +                        hdmi->hdmi_monitor ? "an HDMI" : "a DVI");
> +
> +       drm_mode_connector_update_edid_property(connector, edid);
> +       ret = drm_add_edid_modes(connector, edid);
> +       kfree(edid);
> +
> +       return ret;
> +}
> +
> +static struct drm_connector_helper_funcs sun4i_hdmi_connector_helper_funcs = {
> +       .get_modes      = sun4i_hdmi_get_modes,
> +};
> +
> +static enum drm_connector_status
> +sun4i_hdmi_connector_detect(struct drm_connector *connector, bool force)
> +{
> +       struct sun4i_hdmi *hdmi = drm_connector_to_sun4i_hdmi(connector);
> +       unsigned long reg;
> +
> +       if (readl_poll_timeout(hdmi->base + SUN4I_HDMI_HPD_REG, reg,
> +                              reg & SUN4I_HDMI_HPD_HIGH,
> +                              0, 500000))

We shouldn't need to do polling here. It should just return the status
at the instance it's called. Instead we should have a worker that does
polling to check if something is plugged or unplugged. I don't see any
interrupt bits for this though. :(

> +               return connector_status_disconnected;
> +
> +       return connector_status_connected;
> +}
> +
> +static struct drm_connector_funcs sun4i_hdmi_connector_funcs = {
> +       .dpms                   = drm_atomic_helper_connector_dpms,
> +       .detect                 = sun4i_hdmi_connector_detect,
> +       .fill_modes             = drm_helper_probe_single_connector_modes,
> +       .destroy                = drm_connector_cleanup,
> +       .reset                  = drm_atomic_helper_connector_reset,
> +       .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> +       .atomic_destroy_state   = drm_atomic_helper_connector_destroy_state,
> +};
> +
> +static int sun4i_hdmi_bind(struct device *dev, struct device *master,
> +                        void *data)
> +{
> +       struct drm_device *drm = data;
> +       struct sun4i_drv *drv = drm->dev_private;
> +       struct sun4i_hdmi *hdmi = dev_get_drvdata(dev);
> +       int ret;
> +
> +       hdmi->drv = drv;
> +       drm_encoder_helper_add(&hdmi->encoder,
> +                              &sun4i_hdmi_helper_funcs);
> +       ret = drm_encoder_init(drm,
> +                              &hdmi->encoder,
> +                              &sun4i_hdmi_funcs,
> +                              DRM_MODE_ENCODER_TMDS,
> +                              NULL);
> +       if (ret) {
> +               dev_err(dev, "Couldn't initialise the HDMI encoder\n");
> +               return ret;
> +       }
> +
> +       hdmi->encoder.possible_crtcs = BIT(0);

You can use drm_of_find_possible_crtcs() now. See the TV encoder driver.

> +
> +       drm_connector_helper_add(&hdmi->connector,
> +                                &sun4i_hdmi_connector_helper_funcs);
> +       ret = drm_connector_init(drm, &hdmi->connector,
> +                                &sun4i_hdmi_connector_funcs,
> +                                DRM_MODE_CONNECTOR_HDMIA);
> +       if (ret) {
> +               dev_err(dev,
> +                       "Couldn't initialise the Composite connector\n");

Wrong connector.

> +               goto err_cleanup_connector;
> +       }
> +       hdmi->connector.interlace_allowed = true;
> +
> +       drm_mode_connector_attach_encoder(&hdmi->connector, &hdmi->encoder);
> +
> +       return 0;
> +
> +err_cleanup_connector:
> +       drm_encoder_cleanup(&hdmi->encoder);
> +       return ret;
> +}
> +
> +static void sun4i_hdmi_unbind(struct device *dev, struct device *master,
> +                           void *data)
> +{
> +       struct sun4i_hdmi *hdmi = dev_get_drvdata(dev);
> +
> +       drm_connector_cleanup(&hdmi->connector);
> +       drm_encoder_cleanup(&hdmi->encoder);
> +}
> +
> +static struct component_ops sun4i_hdmi_ops = {
> +       .bind   = sun4i_hdmi_bind,
> +       .unbind = sun4i_hdmi_unbind,
> +};
> +
> +static int sun4i_hdmi_probe(struct platform_device *pdev)
> +{
> +       struct sun4i_hdmi *hdmi;
> +       struct resource *res;
> +       int ret;
> +
> +       hdmi = devm_kzalloc(&pdev->dev, sizeof(*hdmi), GFP_KERNEL);
> +       if (!hdmi)
> +               return -ENOMEM;
> +       dev_set_drvdata(&pdev->dev, hdmi);
> +       hdmi->dev = &pdev->dev;
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       hdmi->base = devm_ioremap_resource(&pdev->dev, res);
> +       if (IS_ERR(hdmi->base)) {
> +               dev_err(&pdev->dev, "Couldn't map the HDMI encoder registers\n");
> +               return PTR_ERR(hdmi->base);
> +       }
> +
> +       hdmi->bus_clk = devm_clk_get(&pdev->dev, "ahb");
> +       if (IS_ERR(hdmi->bus_clk)) {
> +               dev_err(&pdev->dev, "Couldn't get the HDMI bus clock\n");
> +               return PTR_ERR(hdmi->bus_clk);
> +       }
> +       clk_prepare_enable(hdmi->bus_clk);
> +
> +       hdmi->mod_clk = devm_clk_get(&pdev->dev, "mod");
> +       if (IS_ERR(hdmi->mod_clk)) {
> +               dev_err(&pdev->dev, "Couldn't get the HDMI mod clock\n");
> +               return PTR_ERR(hdmi->mod_clk);
> +       }
> +       clk_prepare_enable(hdmi->mod_clk);
> +
> +       hdmi->pll0_clk = devm_clk_get(&pdev->dev, "pll-0");
> +       if (IS_ERR(hdmi->pll0_clk)) {
> +               dev_err(&pdev->dev, "Couldn't get the HDMI PLL 0 clock\n");
> +               return PTR_ERR(hdmi->pll0_clk);
> +       }
> +
> +       hdmi->pll1_clk = devm_clk_get(&pdev->dev, "pll-1");
> +       if (IS_ERR(hdmi->pll1_clk)) {
> +               dev_err(&pdev->dev, "Couldn't get the HDMI PLL 1 clock\n");
> +               return PTR_ERR(hdmi->pll1_clk);
> +       }
> +
> +       ret = sun4i_tmds_create(hdmi);
> +       if (ret) {
> +               dev_err(&pdev->dev, "Couldn't create the TMDS clock\n");
> +               return ret;
> +       }
> +
> +       writel(SUN4I_HDMI_CTRL_ENABLE, hdmi->base + SUN4I_HDMI_CTRL_REG);
> +
> +#define SUN4I_HDMI_PAD_CTRL0 0xfe800000
> +
> +       writel(SUN4I_HDMI_PAD_CTRL0, hdmi->base + SUN4I_HDMI_PAD_CTRL0_REG);
> +
> +       /* TODO: defines */
> +       writel((6 << 3) | (2 << 10) | BIT(14) | BIT(15) |
> +              BIT(19) | BIT(20) | BIT(22) | BIT(23),
> +              hdmi->base + SUN4I_HDMI_PAD_CTRL1_REG);
> +
> +       /* TODO: defines */
> +       writel((8 << 0) | (7 << 8) | (239 << 12) | (7 << 17) | (4 << 20) |
> +              BIT(25) | BIT(27) | BIT(28) | BIT(29) | BIT(30) | BIT(31),
> +              hdmi->base + SUN4I_HDMI_PLL_CTRL_REG);

FYI some bits in this register look a lot like the MIPI PLL on the A33.
Bit 31 looks like the enable bit.

> +
> +       ret = sun4i_ddc_create(hdmi, hdmi->tmds_clk);
> +       if (ret) {
> +               dev_err(&pdev->dev, "Couldn't create the DDC clock\n");
> +               return ret;
> +       }

We do all this in the bind function for all the other components.
Any particular reason to do it differently here?

> +
> +       return component_add(&pdev->dev, &sun4i_hdmi_ops);
> +}
> +
> +static int sun4i_hdmi_remove(struct platform_device *pdev)
> +{
> +       component_del(&pdev->dev, &sun4i_hdmi_ops);
> +
> +       return 0;
> +}
> +
> +static const struct of_device_id sun4i_hdmi_of_table[] = {
> +       { .compatible = "allwinner,sun5i-a10s-hdmi" },
> +       { }
> +};
> +MODULE_DEVICE_TABLE(of, sun4i_hdmi_of_table);
> +
> +static struct platform_driver sun4i_hdmi_driver = {
> +       .probe          = sun4i_hdmi_probe,
> +       .remove         = sun4i_hdmi_remove,
> +       .driver         = {
> +               .name           = "sun4i-hdmi",
> +               .of_match_table = sun4i_hdmi_of_table,
> +       },
> +};
> +module_platform_driver(sun4i_hdmi_driver);
> +
> +MODULE_AUTHOR("Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>");
> +MODULE_DESCRIPTION("Allwinner A10 HDMI Driver");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c
> new file mode 100644
> index 000000000000..40f48f1d4685
> --- /dev/null
> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c
> @@ -0,0 +1,236 @@
> +/*
> + * Copyright (C) 2016 Free Electrons
> + * Copyright (C) 2016 NextThing Co
> + *
> + * Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + */
> +
> +#include <linux/clk-provider.h>
> +
> +#include "sun4i_tcon.h"
> +#include "sun4i_hdmi.h"
> +
> +struct sun4i_tmds {
> +       struct clk_hw           hw;
> +       struct sun4i_hdmi       *hdmi;
> +};
> +
> +static inline struct sun4i_tmds *hw_to_tmds(struct clk_hw *hw)
> +{
> +       return container_of(hw, struct sun4i_tmds, hw);
> +}
> +
> +
> +static unsigned long sun4i_tmds_calc_divider(unsigned long rate,
> +                                            unsigned long parent_rate,
> +                                            u8 *div,
> +                                            bool *half)
> +{
> +       unsigned long best_rate = 0;
> +       u8 best_m = 0, m;
> +       bool is_double;
> +
> +       for (m = 1; m < 16; m++) {
> +               u8 d;
> +
> +               for (d = 1; d < 3; d++) {
> +                       unsigned long tmp_rate;
> +
> +                       tmp_rate = parent_rate / m / d;
> +
> +                       if (tmp_rate > rate)
> +                               continue;
> +
> +                       if (!best_rate ||
> +                           (rate - tmp_rate) < (rate - best_rate)) {
> +                               best_rate = tmp_rate;
> +                               best_m = m;
> +                               is_double = d;
> +                       }
> +               }
> +       }
> +
> +       if (div && half) {
> +               *div = best_m;
> +               *half = is_double;
> +       }
> +
> +       return best_rate;
> +}
> +
> +
> +static int sun4i_tmds_determine_rate(struct clk_hw *hw,
> +                                    struct clk_rate_request *req)
> +{
> +       struct clk_hw *parent;
> +       unsigned long best_parent = 0;
> +       unsigned long rate = req->rate;
> +       int best_div = 1, best_half = 1;
> +       int i, j;
> +
> +       printk("%s %d rate %lu\n", __func__, __LINE__, rate);

Stray printk?

> +
> +       /*
> +        * We only consider PLL3, since the TCON is very likely to be
> +        * clocked from it, and to have the same rate than our HDMI
> +        * clock, so we should not need to do anything.
> +        */
> +
> +       parent = clk_hw_get_parent_by_index(hw, 0);
> +       if (!parent)
> +               return -EINVAL;
> +
> +       for (i = 1; i < 3; i++) {
> +               for (j = 1; j < 16; j++) {
> +                       unsigned long ideal = rate * i * j;
> +                       unsigned long rounded;
> +
> +                       rounded = clk_hw_round_rate(parent, ideal);
> +
> +                       if (rounded == ideal) {
> +                               best_parent = rounded;
> +                               best_half = i;
> +                               best_div = j;
> +                               goto out;
> +                       }
> +
> +                       if (abs(rate - rounded / i) <
> +                           abs(rate - best_parent / best_div)) {
> +                               best_parent = rounded;
> +                               best_div = i;
> +                       }
> +               }
> +       }
> +
> +out:
> +       req->rate = best_parent / best_half / best_div;
> +       req->best_parent_rate = best_parent;
> +       req->best_parent_hw = parent;
> +
> +       printk("%s %d rate %lu parent rate %lu (%s) div %d half %d\n",
> +              __func__, __LINE__, req->rate, req->best_parent_rate,
> +              clk_hw_get_name(req->best_parent_hw),
> +              best_div, best_half);

Stray printk?

> +
> +       return 0;
> +}
> +
> +static unsigned long sun4i_tmds_recalc_rate(struct clk_hw *hw,
> +                                           unsigned long parent_rate)
> +{
> +       struct sun4i_tmds *tmds = hw_to_tmds(hw);
> +       u32 reg;
> +
> +       reg = readl(tmds->hdmi->base + SUN4I_HDMI_PAD_CTRL1_REG);
> +       if (reg & SUN4I_HDMI_PAD_CTRL1_HALVE_CLK)
> +               parent_rate /= 2;
> +
> +       reg = readl(tmds->hdmi->base + SUN4I_HDMI_PLL_CTRL_REG);
> +       reg = (reg >> 4) & 0xf;
> +       if (!reg)
> +               reg = 1;
> +
> +       return parent_rate / reg;
> +}
> +
> +static int sun4i_tmds_set_rate(struct clk_hw *hw, unsigned long rate,
> +                              unsigned long parent_rate)
> +{
> +       struct sun4i_tmds *tmds = hw_to_tmds(hw);
> +       bool half;
> +       u32 reg;
> +       u8 div;
> +
> +       sun4i_tmds_calc_divider(rate, parent_rate, &div, &half);
> +
> +       printk("%s %d rate %lu parent rate %lu div %d half %s\n",
> +              __func__, __LINE__, rate, parent_rate, div,
> +              half ? "yes" : "no");

Stray printk?

> +
> +       reg = readl(tmds->hdmi->base + SUN4I_HDMI_PAD_CTRL1_REG);
> +       reg &= ~SUN4I_HDMI_PAD_CTRL1_HALVE_CLK;
> +       if (half)
> +               reg |= SUN4I_HDMI_PAD_CTRL1_HALVE_CLK;
> +       writel(reg, tmds->hdmi->base + SUN4I_HDMI_PAD_CTRL1_REG);
> +
> +       reg = readl(tmds->hdmi->base + SUN4I_HDMI_PLL_CTRL_REG);
> +       reg &= ~SUN4I_HDMI_PLL_CTRL_DIV_MASK;
> +       writel(reg | SUN4I_HDMI_PLL_CTRL_DIV(div),
> +              tmds->hdmi->base + SUN4I_HDMI_PLL_CTRL_REG);
> +
> +       return 0;
> +}
> +
> +static u8 sun4i_tmds_get_parent(struct clk_hw *hw)
> +{
> +       struct sun4i_tmds *tmds = hw_to_tmds(hw);
> +       u32 reg;
> +
> +       reg = readl(tmds->hdmi->base + SUN4I_HDMI_PLL_DBG0_REG);
> +       return ((reg & SUN4I_HDMI_PLL_DBG0_TMDS_PARENT_MASK) >>
> +               SUN4I_HDMI_PLL_DBG0_TMDS_PARENT_SHIFT);
> +}
> +
> +static int sun4i_tmds_set_parent(struct clk_hw *hw, u8 index)
> +{
> +       struct sun4i_tmds *tmds = hw_to_tmds(hw);
> +       u32 reg;
> +
> +       if (index > 1)
> +               return -EINVAL;
> +
> +       reg = readl(tmds->hdmi->base + SUN4I_HDMI_PLL_DBG0_REG);
> +       reg &= ~SUN4I_HDMI_PLL_DBG0_TMDS_PARENT_MASK;
> +       writel(reg | SUN4I_HDMI_PLL_DBG0_TMDS_PARENT(index),
> +              tmds->hdmi->base + SUN4I_HDMI_PLL_DBG0_REG);
> +
> +       return 0;
> +}
> +
> +static const struct clk_ops sun4i_tmds_ops = {
> +       .determine_rate = sun4i_tmds_determine_rate,
> +       .recalc_rate    = sun4i_tmds_recalc_rate,
> +       .set_rate       = sun4i_tmds_set_rate,
> +
> +       .get_parent     = sun4i_tmds_get_parent,
> +       .set_parent     = sun4i_tmds_set_parent,
> +};
> +
> +int sun4i_tmds_create(struct sun4i_hdmi *hdmi)
> +{
> +       struct clk_init_data init;
> +       struct sun4i_tmds *tmds;
> +       const char *parents[2];
> +
> +       parents[0] = __clk_get_name(hdmi->pll0_clk);
> +       if (!parents[0])
> +               return -ENODEV;
> +
> +       parents[1] = __clk_get_name(hdmi->pll1_clk);
> +       if (!parents[1])
> +               return -ENODEV;
> +
> +       tmds = devm_kzalloc(hdmi->dev, sizeof(*tmds), GFP_KERNEL);
> +       if (!tmds)
> +               return -ENOMEM;
> +
> +       init.name = "hdmi-tmds";
> +       init.ops = &sun4i_tmds_ops;
> +       init.parent_names = parents;
> +       init.num_parents = 2;
> +       init.flags = CLK_SET_RATE_PARENT;
> +
> +       tmds->hdmi = hdmi;
> +       tmds->hw.init = &init;
> +
> +       hdmi->tmds_clk = devm_clk_register(hdmi->dev, &tmds->hw);
> +       if (IS_ERR(hdmi->tmds_clk))
> +               return PTR_ERR(hdmi->tmds_clk);
> +
> +       return 0;
> +}
> --

I also compared the manuals of A20 and A31, and the existing U-boot driver.

So far it looks like the DDC bits are quite different. We could probably
use regfields to work around it, but the DDC clock formula is completely
different. The TMDS clock pre-divider is also different, your usual sun4i
vs sun6i factor offset. Last, the initial values for the 3 PLL related
registers are different.

I'm currently working on an A31 variant for this, basically just copying
the DDC and TMDS bits.

Regards
ChenYu

> git-series 0.8.11
>
> --
> You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
> For more options, visit https://groups.google.com/d/optout.

Re: [linux-sunxi] [PATCH 13/15] drm/sun4i: Add HDMI support

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 7563 bytes --]

Hi Chen-Yu,

On Fri, Apr 21, 2017 at 11:17:17PM +0800, Chen-Yu Tsai wrote:
> Hi,
> 
> On Tue, Mar 7, 2017 at 4:56 PM, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
> > The earlier Allwinner SoCs (A10, A10s, A20, A31) have an embedded HDMI
> > controller.
> >
> > That HDMI controller is able to do audio and CEC, but those have been left
> > out for now.
> >
> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > ---
> >  drivers/gpu/drm/sun4i/Makefile              |   5 +-
> >  drivers/gpu/drm/sun4i/sun4i_hdmi.h          | 124 ++++++-
> >  drivers/gpu/drm/sun4i/sun4i_hdmi_ddc_clk.c  | 128 ++++++-
> >  drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c      | 449 +++++++++++++++++++++-
> >  drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c | 236 +++++++++++-
> >  5 files changed, 942 insertions(+), 0 deletions(-)
> >  create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi.h
> >  create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi_ddc_clk.c
> >  create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
> >  create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c
> 
> Applying patch #9608371 using 'git am'
> Description: [13/15] drm/sun4i: Add HDMI support
> Applying: drm/sun4i: Add HDMI support
> .git/rebase-apply/patch:116: trailing whitespace.
> 
> .git/rebase-apply/patch:531: trailing whitespace.
> 
> .git/rebase-apply/patch:701: trailing whitespace.
> 
> warning: 3 lines add whitespace errors.

Fixed.

> > +int sun4i_ddc_create(struct sun4i_hdmi *hdmi, struct clk *parent)
> > +{
> > +       struct clk_init_data init;
> > +       struct sun4i_ddc *ddc;
> > +       const char *parent_name;
> > +
> > +       parent_name = __clk_get_name(parent);
> > +       if (!parent_name)
> > +               return -ENODEV;
> > +
> > +       ddc = devm_kzalloc(hdmi->dev, sizeof(*ddc), GFP_KERNEL);
> > +       if (!ddc)
> > +               return -ENOMEM;
> > +
> > +       init.name = "hdmi-ddc";
> > +       init.ops = &sun4i_ddc_ops;
> > +       init.parent_names = &parent_name;
> > +       init.num_parents = 1;
> > +       init.flags = CLK_SET_RATE_PARENT;
> 
> I don't think this is really needed. It probably doesn't hurt though,
> since DDC is used when HDMI is not used for displaying, but it might
> affect any upstream PLLs, which theoretically may affect other users
> of said PLLs. The DDC clock is slow enough that we should be able to
> generate a usable clock rate anyway.

Good point, I removed it.

> > +       writel(SUN4I_HDMI_VID_TIMING_X(mode->hdisplay) |
> > +              SUN4I_HDMI_VID_TIMING_Y(mode->vdisplay),
> > +              hdmi->base + SUN4I_HDMI_VID_TIMING_ACT_REG);
> > +
> > +       x = mode->htotal - mode->hsync_start;
> > +       y = mode->vtotal - mode->vsync_start;
> 
> I'm a bit skeptical about this one. All the other parameters are not
> inclusive of other, why would this one be different? Shouldn't it
> be "Xtotal - Xsync_end" instead?

By the usual meaning of backporch, you're right. However, Allwinner's
seems to have it's own, which is actually the backporch + sync length.

We also have that on all the other connectors (and TCON), and this was
confirmed at the time using a scope on an RGB signal.

> 
> > +       writel(SUN4I_HDMI_VID_TIMING_X(x) | SUN4I_HDMI_VID_TIMING_Y(y),
> > +              hdmi->base + SUN4I_HDMI_VID_TIMING_BP_REG);
> > +
> > +       x = mode->hsync_start - mode->hdisplay;
> > +       y = mode->vsync_start - mode->vdisplay;
> > +       writel(SUN4I_HDMI_VID_TIMING_X(x) | SUN4I_HDMI_VID_TIMING_Y(y),
> > +              hdmi->base + SUN4I_HDMI_VID_TIMING_FP_REG);
> > +
> > +       x = mode->hsync_end - mode->hsync_start;
> > +       y = mode->vsync_end - mode->vsync_start;
> > +       writel(SUN4I_HDMI_VID_TIMING_X(x) | SUN4I_HDMI_VID_TIMING_Y(y),
> > +              hdmi->base + SUN4I_HDMI_VID_TIMING_SPW_REG);
> > +
> > +       val = SUN4I_HDMI_VID_TIMING_POL_TX_CLK;
> > +       if (mode->flags & DRM_MODE_FLAG_PHSYNC)
> > +               val |= SUN4I_HDMI_VID_TIMING_POL_HSYNC;
> > +
> > +       if (mode->flags & DRM_MODE_FLAG_PVSYNC)
> > +               val |= SUN4I_HDMI_VID_TIMING_POL_VSYNC;
> > +
> > +       writel(val, hdmi->base + SUN4I_HDMI_VID_TIMING_POL_REG);
> 
> You don't handle the interlaced video here, even though you set
> 
>     hdmi->connector.interlace_allowed = true
> 
> later.

I'll fix that.

> The double clock and double scan flags aren't handled either, though
> I don't understand which one is supposed to represent the need for the
> HDMI pixel repeater. AFAIK this is required for resolutions with pixel
> clocks lower than 25 MHz, the lower limit of HDMI's TMDS link.

I'm not sure about this one though. I'd like to keep things quite
simple for now and build up on that once the basis is working. Is it
common in the wild?

> > +              hdmi->base + SUN4I_HDMI_DDC_FIFO_CTRL_REG);
> > +       writel(SUN4I_HDMI_DDC_ADDR_SEGMENT(offset >> 8) |
> > +              SUN4I_HDMI_DDC_ADDR_EDDC(0x60) |
> > +              SUN4I_HDMI_DDC_ADDR_OFFSET(offset) |
> > +              SUN4I_HDMI_DDC_ADDR_SLAVE(0x50),
> 
> You can use DDC_ADDR from drm_edid.h.

Done.

> > +static enum drm_connector_status
> > +sun4i_hdmi_connector_detect(struct drm_connector *connector, bool force)
> > +{
> > +       struct sun4i_hdmi *hdmi = drm_connector_to_sun4i_hdmi(connector);
> > +       unsigned long reg;
> > +
> > +       if (readl_poll_timeout(hdmi->base + SUN4I_HDMI_HPD_REG, reg,
> > +                              reg & SUN4I_HDMI_HPD_HIGH,
> > +                              0, 500000))
> 
> We shouldn't need to do polling here. It should just return the status
> at the instance it's called. Instead we should have a worker that does
> polling to check if something is plugged or unplugged. I don't see any
> interrupt bits for this though. :(

As far as I know, polling in detect is okay. Why would you want to
remove it?

> > +       ret = drm_encoder_init(drm,
> > +                              &hdmi->encoder,
> > +                              &sun4i_hdmi_funcs,
> > +                              DRM_MODE_ENCODER_TMDS,
> > +                              NULL);
> > +       if (ret) {
> > +               dev_err(dev, "Couldn't initialise the HDMI encoder\n");
> > +               return ret;
> > +       }
> > +
> > +       hdmi->encoder.possible_crtcs = BIT(0);
> 
> You can use drm_of_find_possible_crtcs() now. See the TV encoder driver.

Ack.

> > +
> > +       drm_connector_helper_add(&hdmi->connector,
> > +                                &sun4i_hdmi_connector_helper_funcs);
> > +       ret = drm_connector_init(drm, &hdmi->connector,
> > +                                &sun4i_hdmi_connector_funcs,
> > +                                DRM_MODE_CONNECTOR_HDMIA);
> > +       if (ret) {
> > +               dev_err(dev,
> > +                       "Couldn't initialise the Composite connector\n");
> 
> Wrong connector.

Fixed.

> > +       ret = sun4i_ddc_create(hdmi, hdmi->tmds_clk);
> > +       if (ret) {
> > +               dev_err(&pdev->dev, "Couldn't create the DDC clock\n");
> > +               return ret;
> > +       }
> 
> We do all this in the bind function for all the other components.
> Any particular reason to do it differently here?

Not really, I'll change it.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 13/15] drm/sun4i: Add HDMI support

[Chen-Yu Tsai]

On Wed, Apr 26, 2017 at 2:50 PM, Maxime Ripard
<maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> Hi Chen-Yu,
>
> On Fri, Apr 21, 2017 at 11:17:17PM +0800, Chen-Yu Tsai wrote:
>> Hi,
>>
>> On Tue, Mar 7, 2017 at 4:56 PM, Maxime Ripard
>> <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
>> > The earlier Allwinner SoCs (A10, A10s, A20, A31) have an embedded HDMI
>> > controller.
>> >
>> > That HDMI controller is able to do audio and CEC, but those have been left
>> > out for now.
>> >
>> > Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
>> > ---
>> >  drivers/gpu/drm/sun4i/Makefile              |   5 +-
>> >  drivers/gpu/drm/sun4i/sun4i_hdmi.h          | 124 ++++++-
>> >  drivers/gpu/drm/sun4i/sun4i_hdmi_ddc_clk.c  | 128 ++++++-
>> >  drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c      | 449 +++++++++++++++++++++-
>> >  drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c | 236 +++++++++++-
>> >  5 files changed, 942 insertions(+), 0 deletions(-)
>> >  create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi.h
>> >  create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi_ddc_clk.c
>> >  create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
>> >  create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c
>>
>> Applying patch #9608371 using 'git am'
>> Description: [13/15] drm/sun4i: Add HDMI support
>> Applying: drm/sun4i: Add HDMI support
>> .git/rebase-apply/patch:116: trailing whitespace.
>>
>> .git/rebase-apply/patch:531: trailing whitespace.
>>
>> .git/rebase-apply/patch:701: trailing whitespace.
>>
>> warning: 3 lines add whitespace errors.
>
> Fixed.
>
>> > +int sun4i_ddc_create(struct sun4i_hdmi *hdmi, struct clk *parent)
>> > +{
>> > +       struct clk_init_data init;
>> > +       struct sun4i_ddc *ddc;
>> > +       const char *parent_name;
>> > +
>> > +       parent_name = __clk_get_name(parent);
>> > +       if (!parent_name)
>> > +               return -ENODEV;
>> > +
>> > +       ddc = devm_kzalloc(hdmi->dev, sizeof(*ddc), GFP_KERNEL);
>> > +       if (!ddc)
>> > +               return -ENOMEM;
>> > +
>> > +       init.name = "hdmi-ddc";
>> > +       init.ops = &sun4i_ddc_ops;
>> > +       init.parent_names = &parent_name;
>> > +       init.num_parents = 1;
>> > +       init.flags = CLK_SET_RATE_PARENT;
>>
>> I don't think this is really needed. It probably doesn't hurt though,
>> since DDC is used when HDMI is not used for displaying, but it might
>> affect any upstream PLLs, which theoretically may affect other users
>> of said PLLs. The DDC clock is slow enough that we should be able to
>> generate a usable clock rate anyway.
>
> Good point, I removed it.
>
>> > +       writel(SUN4I_HDMI_VID_TIMING_X(mode->hdisplay) |
>> > +              SUN4I_HDMI_VID_TIMING_Y(mode->vdisplay),
>> > +              hdmi->base + SUN4I_HDMI_VID_TIMING_ACT_REG);
>> > +
>> > +       x = mode->htotal - mode->hsync_start;
>> > +       y = mode->vtotal - mode->vsync_start;
>>
>> I'm a bit skeptical about this one. All the other parameters are not
>> inclusive of other, why would this one be different? Shouldn't it
>> be "Xtotal - Xsync_end" instead?
>
> By the usual meaning of backporch, you're right. However, Allwinner's
> seems to have it's own, which is actually the backporch + sync length.
>
> We also have that on all the other connectors (and TCON), and this was
> confirmed at the time using a scope on an RGB signal.

Yes. On the later SoCs such as the A31, the user manual actually has
timing diagrams showing this.

Unlike the TCON, the HDMI controller's timings lists the front porch
separately, instead of an all inclusive Xtotal. This is what made me
look twice. This should be easy to confirm though. Since the HDMI modes
are well known and can be exactly reproduced on our hardware, we can
just check for any distortions or refresh rate errors.

>
>>
>> > +       writel(SUN4I_HDMI_VID_TIMING_X(x) | SUN4I_HDMI_VID_TIMING_Y(y),
>> > +              hdmi->base + SUN4I_HDMI_VID_TIMING_BP_REG);
>> > +
>> > +       x = mode->hsync_start - mode->hdisplay;
>> > +       y = mode->vsync_start - mode->vdisplay;
>> > +       writel(SUN4I_HDMI_VID_TIMING_X(x) | SUN4I_HDMI_VID_TIMING_Y(y),
>> > +              hdmi->base + SUN4I_HDMI_VID_TIMING_FP_REG);
>> > +
>> > +       x = mode->hsync_end - mode->hsync_start;
>> > +       y = mode->vsync_end - mode->vsync_start;
>> > +       writel(SUN4I_HDMI_VID_TIMING_X(x) | SUN4I_HDMI_VID_TIMING_Y(y),
>> > +              hdmi->base + SUN4I_HDMI_VID_TIMING_SPW_REG);
>> > +
>> > +       val = SUN4I_HDMI_VID_TIMING_POL_TX_CLK;
>> > +       if (mode->flags & DRM_MODE_FLAG_PHSYNC)
>> > +               val |= SUN4I_HDMI_VID_TIMING_POL_HSYNC;
>> > +
>> > +       if (mode->flags & DRM_MODE_FLAG_PVSYNC)
>> > +               val |= SUN4I_HDMI_VID_TIMING_POL_VSYNC;
>> > +
>> > +       writel(val, hdmi->base + SUN4I_HDMI_VID_TIMING_POL_REG);
>>
>> You don't handle the interlaced video here, even though you set
>>
>>     hdmi->connector.interlace_allowed = true
>>
>> later.
>
> I'll fix that.
>
>> The double clock and double scan flags aren't handled either, though
>> I don't understand which one is supposed to represent the need for the
>> HDMI pixel repeater. AFAIK this is required for resolutions with pixel
>> clocks lower than 25 MHz, the lower limit of HDMI's TMDS link.
>
> I'm not sure about this one though. I'd like to keep things quite
> simple for now and build up on that once the basis is working. Is it
> common in the wild?

If you drive the display at SDTV resolutions, then yes. Mode lines from
my HDMI monitor:

  720x576i 50 720 732 795 864 576 580 586 625 flags: nhsync, nvsync,
interlace, dblclk; type: driver
  720x480i 60 720 739 801 858 480 488 494 525 flags: nhsync, nvsync,
interlace, dblclk; type: driver
  720x480i 60 720 739 801 858 480 488 494 525 flags: nhsync, nvsync,
interlace, dblclk; type: driver

AFAIK these are standard modes that all devices should support. Whether
they are used daily is another thing. Maybe block modes with dblclk
in .mode_fixup for now?

>> > +              hdmi->base + SUN4I_HDMI_DDC_FIFO_CTRL_REG);
>> > +       writel(SUN4I_HDMI_DDC_ADDR_SEGMENT(offset >> 8) |
>> > +              SUN4I_HDMI_DDC_ADDR_EDDC(0x60) |
>> > +              SUN4I_HDMI_DDC_ADDR_OFFSET(offset) |
>> > +              SUN4I_HDMI_DDC_ADDR_SLAVE(0x50),
>>
>> You can use DDC_ADDR from drm_edid.h.
>
> Done.

There's also DDC_SEGMENT_ADDR (which is 0x30) you can use to replace 0x60.
The 1 bit shift is probably something related to I2C.

>> > +static enum drm_connector_status
>> > +sun4i_hdmi_connector_detect(struct drm_connector *connector, bool force)
>> > +{
>> > +       struct sun4i_hdmi *hdmi = drm_connector_to_sun4i_hdmi(connector);
>> > +       unsigned long reg;
>> > +
>> > +       if (readl_poll_timeout(hdmi->base + SUN4I_HDMI_HPD_REG, reg,
>> > +                              reg & SUN4I_HDMI_HPD_HIGH,
>> > +                              0, 500000))
>>
>> We shouldn't need to do polling here. It should just return the status
>> at the instance it's called. Instead we should have a worker that does
>> polling to check if something is plugged or unplugged. I don't see any
>> interrupt bits for this though. :(
>
> As far as I know, polling in detect is okay. Why would you want to
> remove it?

Hmm, I guess it only serves to debounce the detection, i.e. extend the
time period of validity from the instance the function is run to the
instance plus 500 ms.

To be clear I'm not against it. However this only really works when
the DRM subsystem is brought up. We still need something else for
hotplugging, which is what I was arguing for.


Regards
ChenYu

>> > +       ret = drm_encoder_init(drm,
>> > +                              &hdmi->encoder,
>> > +                              &sun4i_hdmi_funcs,
>> > +                              DRM_MODE_ENCODER_TMDS,
>> > +                              NULL);
>> > +       if (ret) {
>> > +               dev_err(dev, "Couldn't initialise the HDMI encoder\n");
>> > +               return ret;
>> > +       }
>> > +
>> > +       hdmi->encoder.possible_crtcs = BIT(0);
>>
>> You can use drm_of_find_possible_crtcs() now. See the TV encoder driver.
>
> Ack.
>
>> > +
>> > +       drm_connector_helper_add(&hdmi->connector,
>> > +                                &sun4i_hdmi_connector_helper_funcs);
>> > +       ret = drm_connector_init(drm, &hdmi->connector,
>> > +                                &sun4i_hdmi_connector_funcs,
>> > +                                DRM_MODE_CONNECTOR_HDMIA);
>> > +       if (ret) {
>> > +               dev_err(dev,
>> > +                       "Couldn't initialise the Composite connector\n");
>>
>> Wrong connector.
>
> Fixed.
>
>> > +       ret = sun4i_ddc_create(hdmi, hdmi->tmds_clk);
>> > +       if (ret) {
>> > +               dev_err(&pdev->dev, "Couldn't create the DDC clock\n");
>> > +               return ret;
>> > +       }
>>
>> We do all this in the bind function for all the other components.
>> Any particular reason to do it differently here?
>
> Not really, I'll change it.
>
> Thanks!
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com

Re: [PATCH 13/15] drm/sun4i: Add HDMI support

[Maxime Ripard]

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

On Wed, Apr 26, 2017 at 03:59:28PM +0800, Chen-Yu Tsai wrote:
> >> > +       writel(SUN4I_HDMI_VID_TIMING_X(mode->hdisplay) |
> >> > +              SUN4I_HDMI_VID_TIMING_Y(mode->vdisplay),
> >> > +              hdmi->base + SUN4I_HDMI_VID_TIMING_ACT_REG);
> >> > +
> >> > +       x = mode->htotal - mode->hsync_start;
> >> > +       y = mode->vtotal - mode->vsync_start;
> >>
> >> I'm a bit skeptical about this one. All the other parameters are not
> >> inclusive of other, why would this one be different? Shouldn't it
> >> be "Xtotal - Xsync_end" instead?
> >
> > By the usual meaning of backporch, you're right. However, Allwinner's
> > seems to have it's own, which is actually the backporch + sync length.
> >
> > We also have that on all the other connectors (and TCON), and this was
> > confirmed at the time using a scope on an RGB signal.
> 
> Yes. On the later SoCs such as the A31, the user manual actually has
> timing diagrams showing this.
> 
> Unlike the TCON, the HDMI controller's timings lists the front porch
> separately, instead of an all inclusive Xtotal. This is what made me
> look twice. This should be easy to confirm though. Since the HDMI modes
> are well known and can be exactly reproduced on our hardware, we can
> just check for any distortions or refresh rate errors.

This isn't as trivial as that. Screens usually have some tolerancies
on the timings, which will probably make it go unnoticed, even though
they are wrong.

> >>
> >> > +       writel(SUN4I_HDMI_VID_TIMING_X(x) | SUN4I_HDMI_VID_TIMING_Y(y),
> >> > +              hdmi->base + SUN4I_HDMI_VID_TIMING_BP_REG);
> >> > +
> >> > +       x = mode->hsync_start - mode->hdisplay;
> >> > +       y = mode->vsync_start - mode->vdisplay;
> >> > +       writel(SUN4I_HDMI_VID_TIMING_X(x) | SUN4I_HDMI_VID_TIMING_Y(y),
> >> > +              hdmi->base + SUN4I_HDMI_VID_TIMING_FP_REG);
> >> > +
> >> > +       x = mode->hsync_end - mode->hsync_start;
> >> > +       y = mode->vsync_end - mode->vsync_start;
> >> > +       writel(SUN4I_HDMI_VID_TIMING_X(x) | SUN4I_HDMI_VID_TIMING_Y(y),
> >> > +              hdmi->base + SUN4I_HDMI_VID_TIMING_SPW_REG);
> >> > +
> >> > +       val = SUN4I_HDMI_VID_TIMING_POL_TX_CLK;
> >> > +       if (mode->flags & DRM_MODE_FLAG_PHSYNC)
> >> > +               val |= SUN4I_HDMI_VID_TIMING_POL_HSYNC;
> >> > +
> >> > +       if (mode->flags & DRM_MODE_FLAG_PVSYNC)
> >> > +               val |= SUN4I_HDMI_VID_TIMING_POL_VSYNC;
> >> > +
> >> > +       writel(val, hdmi->base + SUN4I_HDMI_VID_TIMING_POL_REG);
> >>
> >> You don't handle the interlaced video here, even though you set
> >>
> >>     hdmi->connector.interlace_allowed = true
> >>
> >> later.
> >
> > I'll fix that.
> >
> >> The double clock and double scan flags aren't handled either, though
> >> I don't understand which one is supposed to represent the need for the
> >> HDMI pixel repeater. AFAIK this is required for resolutions with pixel
> >> clocks lower than 25 MHz, the lower limit of HDMI's TMDS link.
> >
> > I'm not sure about this one though. I'd like to keep things quite
> > simple for now and build up on that once the basis is working. Is it
> > common in the wild?
> 
> If you drive the display at SDTV resolutions, then yes. Mode lines from
> my HDMI monitor:
> 
>   720x576i 50 720 732 795 864 576 580 586 625 flags: nhsync, nvsync,
> interlace, dblclk; type: driver
>   720x480i 60 720 739 801 858 480 488 494 525 flags: nhsync, nvsync,
> interlace, dblclk; type: driver
>   720x480i 60 720 739 801 858 480 488 494 525 flags: nhsync, nvsync,
> interlace, dblclk; type: driver
> 
> AFAIK these are standard modes that all devices should support. Whether
> they are used daily is another thing. Maybe block modes with dblclk
> in .mode_fixup for now?

That would rather be atomic_check and / or mode_valid, but yeah, I can
do that.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[PATCH 14/15] ARM: sun5i: a10s: Add the HDMI controller node

[Maxime Ripard]

The A10s has an HDMI controller connected to the second TCON channel. Add
it to our DT.

Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 arch/arm/boot/dts/sun5i-a10s.dtsi | 34 ++++++++++++++++++++++++++++++++-
 arch/arm/boot/dts/sun5i.dtsi      |  1 +-
 2 files changed, 35 insertions(+), 0 deletions(-)

diff --git a/arch/arm/boot/dts/sun5i-a10s.dtsi b/arch/arm/boot/dts/sun5i-a10s.dtsi
index 074485782a4a..3482c9d2b120 100644
--- a/arch/arm/boot/dts/sun5i-a10s.dtsi
+++ b/arch/arm/boot/dts/sun5i-a10s.dtsi
@@ -72,7 +72,33 @@
 		};
 	};
 
+	display-engine {
+		compatible = "allwinner,sun5i-a10s-display-engine",
+			     "allwinner,sun5i-a13-display-engine";
+		allwinner,pipelines = <&fe0>;
+	};
+
 	soc@01c00000 {
+		hdmi0: hdmi@01c16000 {
+			compatible = "allwinner,sun5i-a10s-hdmi";
+			reg = <0x01c16000 0x1000>;
+			clocks = <&ccu CLK_AHB_HDMI>, <&ccu CLK_HDMI>,
+				 <&ccu CLK_PLL_VIDEO0_2X>,
+				 <&ccu CLK_PLL_VIDEO1_2X>;
+			clock-names = "ahb", "mod", "pll-0", "pll-1";
+			status = "disabled";
+
+			port {
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				hdmi0_in_tcon0: endpoint@0 {
+					reg = <0>;
+					remote-endpoint = <&tcon0_out_hdmi0>;
+				};
+			};
+		};
+
 		pwm: pwm@01c20e00 {
 			compatible = "allwinner,sun5i-a10s-pwm";
 			reg = <0x01c20e00 0xc>;
@@ -129,3 +155,11 @@
 
 &sram_a {
 };
+
+&tcon0_out {
+	tcon0_out_hdmi0: endpoint@2 {
+		reg = <2>;
+		remote-endpoint = <&hdmi0_in_tcon0>;
+		allwinner,tcon-channel = <1>;
+	};
+};
diff --git a/arch/arm/boot/dts/sun5i.dtsi b/arch/arm/boot/dts/sun5i.dtsi
index f3b6e19244f9..3d009b2aa42a 100644
--- a/arch/arm/boot/dts/sun5i.dtsi
+++ b/arch/arm/boot/dts/sun5i.dtsi
@@ -273,6 +273,7 @@
 					tcon0_out_tve0: endpoint@1 {
 						reg = <1>;
 						remote-endpoint = <&tve0_in_tcon0>;
+						allwinner,tcon-channel = <1>;
 					};
 				};
 			};
-- 
git-series 0.8.11

Re: [PATCH 14/15] ARM: sun5i: a10s: Add the HDMI controller node

[Chen-Yu Tsai]

Hi,

On Tue, Mar 7, 2017 at 4:56 PM, Maxime Ripard
<maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> The A10s has an HDMI controller connected to the second TCON channel. Add
> it to our DT.
>
> Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
>  arch/arm/boot/dts/sun5i-a10s.dtsi | 34 ++++++++++++++++++++++++++++++++-
>  arch/arm/boot/dts/sun5i.dtsi      |  1 +-
>  2 files changed, 35 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/boot/dts/sun5i-a10s.dtsi b/arch/arm/boot/dts/sun5i-a10s.dtsi
> index 074485782a4a..3482c9d2b120 100644
> --- a/arch/arm/boot/dts/sun5i-a10s.dtsi
> +++ b/arch/arm/boot/dts/sun5i-a10s.dtsi
> @@ -72,7 +72,33 @@
>                 };
>         };
>
> +       display-engine {
> +               compatible = "allwinner,sun5i-a10s-display-engine",
> +                            "allwinner,sun5i-a13-display-engine";
> +               allwinner,pipelines = <&fe0>;
> +       };
> +
>         soc@01c00000 {
> +               hdmi0: hdmi@01c16000 {

Nit: is the 0 suffix needed? I don't see any indication that there is
a second controller.

> +                       compatible = "allwinner,sun5i-a10s-hdmi";
> +                       reg = <0x01c16000 0x1000>;
> +                       clocks = <&ccu CLK_AHB_HDMI>, <&ccu CLK_HDMI>,
> +                                <&ccu CLK_PLL_VIDEO0_2X>,
> +                                <&ccu CLK_PLL_VIDEO1_2X>;
> +                       clock-names = "ahb", "mod", "pll-0", "pll-1";
> +                       status = "disabled";
> +
> +                       port {
> +                               #address-cells = <1>;
> +                               #size-cells = <0>;
> +
> +                               hdmi0_in_tcon0: endpoint@0 {
> +                                       reg = <0>;
> +                                       remote-endpoint = <&tcon0_out_hdmi0>;
> +                               };
> +                       };
> +               };
> +
>                 pwm: pwm@01c20e00 {
>                         compatible = "allwinner,sun5i-a10s-pwm";
>                         reg = <0x01c20e00 0xc>;
> @@ -129,3 +155,11 @@
>
>  &sram_a {
>  };
> +
> +&tcon0_out {
> +       tcon0_out_hdmi0: endpoint@2 {
> +               reg = <2>;
> +               remote-endpoint = <&hdmi0_in_tcon0>;
> +               allwinner,tcon-channel = <1>;
> +       };
> +};
> diff --git a/arch/arm/boot/dts/sun5i.dtsi b/arch/arm/boot/dts/sun5i.dtsi
> index f3b6e19244f9..3d009b2aa42a 100644
> --- a/arch/arm/boot/dts/sun5i.dtsi
> +++ b/arch/arm/boot/dts/sun5i.dtsi
> @@ -273,6 +273,7 @@
>                                         tcon0_out_tve0: endpoint@1 {
>                                                 reg = <1>;
>                                                 remote-endpoint = <&tve0_in_tcon0>;
> +                                               allwinner,tcon-channel = <1>;

This looks like a separate patch, probably following the binding change?

Regards
ChenYu

>                                         };
>                                 };
>                         };
> --
> git-series 0.8.11
>
> --
> You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
> For more options, visit https://groups.google.com/d/optout.

Re: [PATCH 14/15] ARM: sun5i: a10s: Add the HDMI controller node

[Maxime Ripard]

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

1;4601;0c
On Wed, Mar 08, 2017 at 11:35:39AM +0800, Chen-Yu Tsai wrote:
> Hi,
> 
> On Tue, Mar 7, 2017 at 4:56 PM, Maxime Ripard
> <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> > The A10s has an HDMI controller connected to the second TCON channel. Add
> > it to our DT.
> >
> > Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> > ---
> >  arch/arm/boot/dts/sun5i-a10s.dtsi | 34 ++++++++++++++++++++++++++++++++-
> >  arch/arm/boot/dts/sun5i.dtsi      |  1 +-
> >  2 files changed, 35 insertions(+), 0 deletions(-)
> >
> > diff --git a/arch/arm/boot/dts/sun5i-a10s.dtsi b/arch/arm/boot/dts/sun5i-a10s.dtsi
> > index 074485782a4a..3482c9d2b120 100644
> > --- a/arch/arm/boot/dts/sun5i-a10s.dtsi
> > +++ b/arch/arm/boot/dts/sun5i-a10s.dtsi
> > @@ -72,7 +72,33 @@
> >                 };
> >         };
> >
> > +       display-engine {
> > +               compatible = "allwinner,sun5i-a10s-display-engine",
> > +                            "allwinner,sun5i-a13-display-engine";
> > +               allwinner,pipelines = <&fe0>;
> > +       };
> > +
> >         soc@01c00000 {
> > +               hdmi0: hdmi@01c16000 {
> 
> Nit: is the 0 suffix needed? I don't see any indication that there is
> a second controller.
> 
> > +                       compatible = "allwinner,sun5i-a10s-hdmi";
> > +                       reg = <0x01c16000 0x1000>;
> > +                       clocks = <&ccu CLK_AHB_HDMI>, <&ccu CLK_HDMI>,
> > +                                <&ccu CLK_PLL_VIDEO0_2X>,
> > +                                <&ccu CLK_PLL_VIDEO1_2X>;
> > +                       clock-names = "ahb", "mod", "pll-0", "pll-1";
> > +                       status = "disabled";
> > +
> > +                       port {
> > +                               #address-cells = <1>;
> > +                               #size-cells = <0>;
> > +
> > +                               hdmi0_in_tcon0: endpoint@0 {
> > +                                       reg = <0>;
> > +                                       remote-endpoint = <&tcon0_out_hdmi0>;
> > +                               };
> > +                       };
> > +               };
> > +
> >                 pwm: pwm@01c20e00 {
> >                         compatible = "allwinner,sun5i-a10s-pwm";
> >                         reg = <0x01c20e00 0xc>;
> > @@ -129,3 +155,11 @@
> >
> >  &sram_a {
> >  };
> > +
> > +&tcon0_out {
> > +       tcon0_out_hdmi0: endpoint@2 {
> > +               reg = <2>;
> > +               remote-endpoint = <&hdmi0_in_tcon0>;
> > +               allwinner,tcon-channel = <1>;
> > +       };
> > +};
> > diff --git a/arch/arm/boot/dts/sun5i.dtsi b/arch/arm/boot/dts/sun5i.dtsi
> > index f3b6e19244f9..3d009b2aa42a 100644
> > --- a/arch/arm/boot/dts/sun5i.dtsi
> > +++ b/arch/arm/boot/dts/sun5i.dtsi
> > @@ -273,6 +273,7 @@
> >                                         tcon0_out_tve0: endpoint@1 {
> >                                                 reg = <1>;
> >                                                 remote-endpoint = <&tve0_in_tcon0>;
> > +                                               allwinner,tcon-channel = <1>;
> 
> This looks like a separate patch, probably following the binding
> change?

I don't know, the binding says that without anything specified, reg
would be used. I was assuming that we only needed it once we had the
new endpoint to make it consistent, therefore it didn't need an extra
patch.

But I can definitely create one if you want.
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH 14/15] ARM: sun5i: a10s: Add the HDMI controller node

[Chen-Yu Tsai]

On Thu, Mar 9, 2017 at 6:59 PM, Maxime Ripard
<maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> 1;4601;0c
> On Wed, Mar 08, 2017 at 11:35:39AM +0800, Chen-Yu Tsai wrote:
>> Hi,
>>
>> On Tue, Mar 7, 2017 at 4:56 PM, Maxime Ripard
>> <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
>> > The A10s has an HDMI controller connected to the second TCON channel. Add
>> > it to our DT.
>> >
>> > Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
>> > ---
>> >  arch/arm/boot/dts/sun5i-a10s.dtsi | 34 ++++++++++++++++++++++++++++++++-
>> >  arch/arm/boot/dts/sun5i.dtsi      |  1 +-
>> >  2 files changed, 35 insertions(+), 0 deletions(-)
>> >
>> > diff --git a/arch/arm/boot/dts/sun5i-a10s.dtsi b/arch/arm/boot/dts/sun5i-a10s.dtsi
>> > index 074485782a4a..3482c9d2b120 100644
>> > --- a/arch/arm/boot/dts/sun5i-a10s.dtsi
>> > +++ b/arch/arm/boot/dts/sun5i-a10s.dtsi
>> > @@ -72,7 +72,33 @@
>> >                 };
>> >         };
>> >
>> > +       display-engine {
>> > +               compatible = "allwinner,sun5i-a10s-display-engine",
>> > +                            "allwinner,sun5i-a13-display-engine";
>> > +               allwinner,pipelines = <&fe0>;
>> > +       };
>> > +
>> >         soc@01c00000 {
>> > +               hdmi0: hdmi@01c16000 {
>>
>> Nit: is the 0 suffix needed? I don't see any indication that there is
>> a second controller.
>>
>> > +                       compatible = "allwinner,sun5i-a10s-hdmi";
>> > +                       reg = <0x01c16000 0x1000>;
>> > +                       clocks = <&ccu CLK_AHB_HDMI>, <&ccu CLK_HDMI>,
>> > +                                <&ccu CLK_PLL_VIDEO0_2X>,
>> > +                                <&ccu CLK_PLL_VIDEO1_2X>;
>> > +                       clock-names = "ahb", "mod", "pll-0", "pll-1";
>> > +                       status = "disabled";
>> > +
>> > +                       port {
>> > +                               #address-cells = <1>;
>> > +                               #size-cells = <0>;
>> > +
>> > +                               hdmi0_in_tcon0: endpoint@0 {
>> > +                                       reg = <0>;
>> > +                                       remote-endpoint = <&tcon0_out_hdmi0>;
>> > +                               };
>> > +                       };
>> > +               };
>> > +
>> >                 pwm: pwm@01c20e00 {
>> >                         compatible = "allwinner,sun5i-a10s-pwm";
>> >                         reg = <0x01c20e00 0xc>;
>> > @@ -129,3 +155,11 @@
>> >
>> >  &sram_a {
>> >  };
>> > +
>> > +&tcon0_out {
>> > +       tcon0_out_hdmi0: endpoint@2 {
>> > +               reg = <2>;
>> > +               remote-endpoint = <&hdmi0_in_tcon0>;
>> > +               allwinner,tcon-channel = <1>;
>> > +       };
>> > +};
>> > diff --git a/arch/arm/boot/dts/sun5i.dtsi b/arch/arm/boot/dts/sun5i.dtsi
>> > index f3b6e19244f9..3d009b2aa42a 100644
>> > --- a/arch/arm/boot/dts/sun5i.dtsi
>> > +++ b/arch/arm/boot/dts/sun5i.dtsi
>> > @@ -273,6 +273,7 @@
>> >                                         tcon0_out_tve0: endpoint@1 {
>> >                                                 reg = <1>;
>> >                                                 remote-endpoint = <&tve0_in_tcon0>;
>> > +                                               allwinner,tcon-channel = <1>;
>>
>> This looks like a separate patch, probably following the binding
>> change?
>
> I don't know, the binding says that without anything specified, reg
> would be used. I was assuming that we only needed it once we had the
> new endpoint to make it consistent, therefore it didn't need an extra
> patch.
>
> But I can definitely create one if you want.

It just seems a bit out of place, that's all. Mentioning it in the
commit message would be good enough for me.

ChenYu

> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com

[PATCH 15/15] ARM: sun5i: a10s-olinuxino: Enable HDMI

[Maxime Ripard]

The A10s Olinuxino has an HDMI connector. Make sure we can use it.

Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 arch/arm/boot/dts/sun5i-a10s-olinuxino-micro.dts | 12 ++++++++++++
 1 file changed, 12 insertions(+), 0 deletions(-)

diff --git a/arch/arm/boot/dts/sun5i-a10s-olinuxino-micro.dts b/arch/arm/boot/dts/sun5i-a10s-olinuxino-micro.dts
index baee64d61f6d..3102c27b04df 100644
--- a/arch/arm/boot/dts/sun5i-a10s-olinuxino-micro.dts
+++ b/arch/arm/boot/dts/sun5i-a10s-olinuxino-micro.dts
@@ -77,6 +77,10 @@
 	};
 };
 
+&be0 {
+	status = "okay";
+};
+
 &ehci0 {
 	status = "okay";
 };
@@ -92,6 +96,10 @@
 	status = "okay";
 };
 
+&hdmi0 {
+	status = "okay";
+};
+
 &i2c0 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&i2c0_pins_a>;
@@ -249,6 +257,10 @@
 	status = "okay";
 };
 
+&tcon0 {
+	status = "okay";
+};
+
 &uart0 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&uart0_pins_a>;
-- 
git-series 0.8.11

Re: [PATCH 15/15] ARM: sun5i: a10s-olinuxino: Enable HDMI

[Chen-Yu Tsai]

On Tue, Mar 7, 2017 at 4:56 PM, Maxime Ripard
<maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> The A10s Olinuxino has an HDMI connector. Make sure we can use it.
>
> Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

Acked-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>

[PATCH 7/15] dt-bindings: display: sun4i: Add allwinner, tcon-channel property

[Maxime Ripard]

The Allwinner Timings Controller has two, mutually exclusive, channels.
When the binding has been introduced, it was assumed that there would be
only a single user per channel in the system.

While this is likely for the channel 0 which only connects to LCD displays,
it turns out that the channel 1 can be connected to multiple controllers in
the SoC (HDMI and TV encoders for example). And while the simultaneous use
of HDMI and TV outputs cannot be achieved, switching from one to the other
at runtime definitely sounds plausible.

Add an extra property, allwinner,tcon-channel, to specify for a given
endpoint which TCON channel it is connected to, while falling back to the
previous mechanism if that property is missing.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt | 11 ++++---
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt b/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt
index 4b280672658e..18d445723c3e 100644
--- a/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt
+++ b/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt
@@ -68,10 +68,13 @@ Required properties:
   Documentation/devicetree/bindings/media/video-interfaces.txt. The
   first port should be the input endpoint, the second one the output
 
-  The output should have two endpoints. The first is the block
-  connected to the TCON channel 0 (usually a panel or a bridge), the
-  second the block connected to the TCON channel 1 (usually the TV
-  encoder)
+  The output may have multiple endpoints. The TCON has two channels,
+  usually with the first channel being used for the panels interfaces
+  (RGB, LVDS, etc.), and the second being used for the outputs that
+  require another controller (TV Encoder, HDMI, etc.). The endpoints
+  will take an extra property, allwinner,tcon-channel, to specify the
+  channel the endpoint is associated to. If that property is not
+  present, the endpoint number will be used as the channel number.
 
 On SoCs other than the A33, there is one more clock required:
    - 'tcon-ch1': The clock driving the TCON channel 1
-- 
git-series 0.8.11
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 7/15] dt-bindings: display: sun4i: Add allwinner,tcon-channel property

[Chen-Yu Tsai]

On Tue, Mar 7, 2017 at 4:56 PM, Maxime Ripard
<maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> The Allwinner Timings Controller has two, mutually exclusive, channels.
> When the binding has been introduced, it was assumed that there would be
> only a single user per channel in the system.
>
> While this is likely for the channel 0 which only connects to LCD displays,
> it turns out that the channel 1 can be connected to multiple controllers in
> the SoC (HDMI and TV encoders for example). And while the simultaneous use
> of HDMI and TV outputs cannot be achieved, switching from one to the other
> at runtime definitely sounds plausible.
>
> Add an extra property, allwinner,tcon-channel, to specify for a given
> endpoint which TCON channel it is connected to, while falling back to the
> previous mechanism if that property is missing.
>
> Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

Acked-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>

Re: [PATCH 7/15] dt-bindings: display: sun4i: Add allwinner,tcon-channel property

[Rob Herring]

On Tue, Mar 07, 2017 at 09:56:26AM +0100, Maxime Ripard wrote:
> The Allwinner Timings Controller has two, mutually exclusive, channels.
> When the binding has been introduced, it was assumed that there would be
> only a single user per channel in the system.
> 
> While this is likely for the channel 0 which only connects to LCD displays,
> it turns out that the channel 1 can be connected to multiple controllers in
> the SoC (HDMI and TV encoders for example). And while the simultaneous use
> of HDMI and TV outputs cannot be achieved, switching from one to the other
> at runtime definitely sounds plausible.
> 
> Add an extra property, allwinner,tcon-channel, to specify for a given
> endpoint which TCON channel it is connected to, while falling back to the
> previous mechanism if that property is missing.

I think perhaps TCON channels should have been ports rather than 
endpoints. The fact that the channels are mutually exclusive can be 
handled in the driver and doesn't really matter in the binding. How 
painful would it be to rework things to move TCON channel 1 from port 0, 
endpoint 1 to port 1? 

Rob

Re: [PATCH 7/15] dt-bindings: display: sun4i: Add allwinner,tcon-channel property

[Chen-Yu Tsai]

On Thu, Mar 16, 2017 at 1:37 AM, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Tue, Mar 07, 2017 at 09:56:26AM +0100, Maxime Ripard wrote:
>> The Allwinner Timings Controller has two, mutually exclusive, channels.
>> When the binding has been introduced, it was assumed that there would be
>> only a single user per channel in the system.
>>
>> While this is likely for the channel 0 which only connects to LCD displays,
>> it turns out that the channel 1 can be connected to multiple controllers in
>> the SoC (HDMI and TV encoders for example). And while the simultaneous use
>> of HDMI and TV outputs cannot be achieved, switching from one to the other
>> at runtime definitely sounds plausible.
>>
>> Add an extra property, allwinner,tcon-channel, to specify for a given
>> endpoint which TCON channel it is connected to, while falling back to the
>> previous mechanism if that property is missing.
>
> I think perhaps TCON channels should have been ports rather than
> endpoints. The fact that the channels are mutually exclusive can be
> handled in the driver and doesn't really matter in the binding. How
> painful would it be to rework things to move TCON channel 1 from port 0,
> endpoint 1 to port 1?

Having a separate output port for channel 1 was one option we discussed.
However it wouldn't work well with the kernel's of_graph based crtc
detection (drm_of_find_possible_crtcs / drm_of_find_possible_crtcs),
which has the crtcs bound via the output port. As the logic is used
by multiple drivers, I'm not sure it's easy to rework or test.

Also, we still have to support old device trees using channel 1 from
output port 0 endpoint 1. This is the TV encoder on sun5i (A10s/A13/R8).

Regards
ChenYu

Re: [PATCH 7/15] dt-bindings: display: sun4i: Add allwinner,tcon-channel property

[Maxime Ripard]

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

On Fri, Mar 17, 2017 at 11:34:45AM +0800, Chen-Yu Tsai wrote:
> On Thu, Mar 16, 2017 at 1:37 AM, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> > On Tue, Mar 07, 2017 at 09:56:26AM +0100, Maxime Ripard wrote:
> >> The Allwinner Timings Controller has two, mutually exclusive, channels.
> >> When the binding has been introduced, it was assumed that there would be
> >> only a single user per channel in the system.
> >>
> >> While this is likely for the channel 0 which only connects to LCD displays,
> >> it turns out that the channel 1 can be connected to multiple controllers in
> >> the SoC (HDMI and TV encoders for example). And while the simultaneous use
> >> of HDMI and TV outputs cannot be achieved, switching from one to the other
> >> at runtime definitely sounds plausible.
> >>
> >> Add an extra property, allwinner,tcon-channel, to specify for a given
> >> endpoint which TCON channel it is connected to, while falling back to the
> >> previous mechanism if that property is missing.
> >
> > I think perhaps TCON channels should have been ports rather than
> > endpoints. The fact that the channels are mutually exclusive can be
> > handled in the driver and doesn't really matter in the binding. How
> > painful would it be to rework things to move TCON channel 1 from port 0,
> > endpoint 1 to port 1?
> 
> Having a separate output port for channel 1 was one option we discussed.
> However it wouldn't work well with the kernel's of_graph based crtc
> detection (drm_of_find_possible_crtcs / drm_of_find_possible_crtcs),
> which has the crtcs bound via the output port. As the logic is used
> by multiple drivers, I'm not sure it's easy to rework or test.

Can't we use a different logic than drm_of_find_possible_crtcs to fill
the same role?

> Also, we still have to support old device trees using channel 1 from
> output port 0 endpoint 1. This is the TV encoder on sun5i (A10s/A13/R8).

We could probably work something out if we go that way to deal with
old DTs though.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH 0/15] drm: sun4i: Add support for the HDMI controller

[Stefan Monnier]

> Here is an attempt at getting the HDMI controller running.
> This HDMI controller is found on a number of old Allwinner SoCs (A10, A10s,
> A20, A31).

Thank you thank you thank you.


        Stefan "I need it on the A20, so we're not quite
                there yet, but it's great to see progress on this front"

Re: [PATCH 7/15] dt-bindings: display: sun4i: Add allwinner,tcon-channel property

[Icenowy Zheng]

2017年3月27日 上午5:11于 Maxime Ripard <maxime.ripard@free-electrons.com>写道：
>
> On Fri, Mar 17, 2017 at 11:34:45AM +0800, Chen-Yu Tsai wrote: 
> > On Thu, Mar 16, 2017 at 1:37 AM, Rob Herring <robh@kernel.org> wrote: 
> > > On Tue, Mar 07, 2017 at 09:56:26AM +0100, Maxime Ripard wrote: 
> > >> The Allwinner Timings Controller has two, mutually exclusive, channels. 
> > >> When the binding has been introduced, it was assumed that there would be 
> > >> only a single user per channel in the system. 
> > >> 
> > >> While this is likely for the channel 0 which only connects to LCD displays, 
> > >> it turns out that the channel 1 can be connected to multiple controllers in 
> > >> the SoC (HDMI and TV encoders for example). And while the simultaneous use 
> > >> of HDMI and TV outputs cannot be achieved, switching from one to the other 
> > >> at runtime definitely sounds plausible. 
> > >> 
> > >> Add an extra property, allwinner,tcon-channel, to specify for a given 
> > >> endpoint which TCON channel it is connected to, while falling back to the 
> > >> previous mechanism if that property is missing. 
> > > 
> > > I think perhaps TCON channels should have been ports rather than 
> > > endpoints. The fact that the channels are mutually exclusive can be 
> > > handled in the driver and doesn't really matter in the binding. How 
> > > painful would it be to rework things to move TCON channel 1 from port 0, 
> > > endpoint 1 to port 1? 
> > 
> > Having a separate output port for channel 1 was one option we discussed. 
> > However it wouldn't work well with the kernel's of_graph based crtc 
> > detection (drm_of_find_possible_crtcs / drm_of_find_possible_crtcs), 
> > which has the crtcs bound via the output port. As the logic is used 
> > by multiple drivers, I'm not sure it's easy to rework or test. 
>
> Can't we use a different logic than drm_of_find_possible_crtcs to fill 
> the same role? 
>
> > Also, we still have to support old device trees using channel 1 from 
> > output port 0 endpoint 1. This is the TV encoder on sun5i (A10s/A13/R8). 

And from A83T on we will face channel-1 only TCONs., which do not
have channel 0 at all in hardware.

>
> We could probably work something out if we go that way to deal with 
> old DTs though. 
>
> Maxime 
>
> -- 
> Maxime Ripard, Free Electrons 
> Embedded Linux and Kernel engineering 
> http://free-electrons.com
>
> _______________________________________________ 
> linux-arm-kernel mailing list 
> linux-arm-kernel@lists.infradead.org 
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 7/15] dt-bindings: display: sun4i: Add allwinner,tcon-channel property

[Maxime Ripard]

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

On Tue, Mar 28, 2017 at 06:05:08PM +0800, Icenowy Zheng wrote:
> 
> 2017年3月27日 上午5:11于 Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>写道：
> >
> > On Fri, Mar 17, 2017 at 11:34:45AM +0800, Chen-Yu Tsai wrote: 
> > > On Thu, Mar 16, 2017 at 1:37 AM, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote: 
> > > > On Tue, Mar 07, 2017 at 09:56:26AM +0100, Maxime Ripard wrote: 
> > > >> The Allwinner Timings Controller has two, mutually exclusive, channels. 
> > > >> When the binding has been introduced, it was assumed that there would be 
> > > >> only a single user per channel in the system. 
> > > >> 
> > > >> While this is likely for the channel 0 which only connects to LCD displays, 
> > > >> it turns out that the channel 1 can be connected to multiple controllers in 
> > > >> the SoC (HDMI and TV encoders for example). And while the simultaneous use 
> > > >> of HDMI and TV outputs cannot be achieved, switching from one to the other 
> > > >> at runtime definitely sounds plausible. 
> > > >> 
> > > >> Add an extra property, allwinner,tcon-channel, to specify for a given 
> > > >> endpoint which TCON channel it is connected to, while falling back to the 
> > > >> previous mechanism if that property is missing. 
> > > > 
> > > > I think perhaps TCON channels should have been ports rather than 
> > > > endpoints. The fact that the channels are mutually exclusive can be 
> > > > handled in the driver and doesn't really matter in the binding. How 
> > > > painful would it be to rework things to move TCON channel 1 from port 0, 
> > > > endpoint 1 to port 1? 
> > > 
> > > Having a separate output port for channel 1 was one option we discussed. 
> > > However it wouldn't work well with the kernel's of_graph based crtc 
> > > detection (drm_of_find_possible_crtcs / drm_of_find_possible_crtcs), 
> > > which has the crtcs bound via the output port. As the logic is used 
> > > by multiple drivers, I'm not sure it's easy to rework or test. 
> >
> > Can't we use a different logic than drm_of_find_possible_crtcs to fill 
> > the same role? 
> >
> > > Also, we still have to support old device trees using channel 1 from 
> > > output port 0 endpoint 1. This is the TV encoder on sun5i (A10s/A13/R8). 
> 
> And from A83T on we will face channel-1 only TCONs., which do not
> have channel 0 at all in hardware.

Yes, but those patches have not been merged yet, so we don't have to
care about the backward compatibility.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

-- 
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 7/15] dt-bindings: display: sun4i: Add allwinner,tcon-channel property

[Icenowy Zheng]

2017年3月27日 上午5:11于 Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>写道：
>
> On Fri, Mar 17, 2017 at 11:34:45AM +0800, Chen-Yu Tsai wrote: 
> > On Thu, Mar 16, 2017 at 1:37 AM, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote: 
> > > On Tue, Mar 07, 2017 at 09:56:26AM +0100, Maxime Ripard wrote: 
> > >> The Allwinner Timings Controller has two, mutually exclusive, channels. 
> > >> When the binding has been introduced, it was assumed that there would be 
> > >> only a single user per channel in the system. 
> > >> 
> > >> While this is likely for the channel 0 which only connects to LCD displays, 
> > >> it turns out that the channel 1 can be connected to multiple controllers in 
> > >> the SoC (HDMI and TV encoders for example). And while the simultaneous use 
> > >> of HDMI and TV outputs cannot be achieved, switching from one to the other 
> > >> at runtime definitely sounds plausible. 
> > >> 
> > >> Add an extra property, allwinner,tcon-channel, to specify for a given 
> > >> endpoint which TCON channel it is connected to, while falling back to the 
> > >> previous mechanism if that property is missing. 
> > > 
> > > I think perhaps TCON channels should have been ports rather than 
> > > endpoints. The fact that the channels are mutually exclusive can be 
> > > handled in the driver and doesn't really matter in the binding. How 
> > > painful would it be to rework things to move TCON channel 1 from port 0, 
> > > endpoint 1 to port 1? 
> > 
> > Having a separate output port for channel 1 was one option we discussed. 
> > However it wouldn't work well with the kernel's of_graph based crtc 
> > detection (drm_of_find_possible_crtcs / drm_of_find_possible_crtcs), 
> > which has the crtcs bound via the output port. As the logic is used 
> > by multiple drivers, I'm not sure it's easy to rework or test. 
>
> Can't we use a different logic than drm_of_find_possible_crtcs to fill 
> the same role? 
>
> > Also, we still have to support old device trees using channel 1 from 
> > output port 0 endpoint 1. This is the TV encoder on sun5i (A10s/A13/R8). 

And from A83T on we will face channel-1 only TCONs., which do not
have channel 0 at all in hardware.

>
> We could probably work something out if we go that way to deal with 
> old DTs though. 
>
> Maxime 
>
> -- 
> Maxime Ripard, Free Electrons 
> Embedded Linux and Kernel engineering 
> http://free-electrons.com
>
> _______________________________________________ 
> linux-arm-kernel mailing list 
> linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org 
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel 

-- 
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.