diff mbox

[V3,3/3] ARM: clk-imx6q: Add missing lvds and anaclk clock to the clock tree

Message ID 9ecf6480464cffb3b4347ad3fd8ec5f07462a0fc.1407481023.git.shengjiu.wang@freescale.com
State New
Headers show

Commit Message

Shengjiu Wang Aug. 8, 2014, 7:02 a.m. UTC 
anaclk1 and anaclk2 is the clock source for lvds1_in and lvds2_in. lvds1_in
and lvds2_in can be used to provide external clock source to the internal
pll, such as pll4_audio and pll5_video.
pll4_audio and pll5_video can have multiple source, not only "osc", so
add them.

Signed-off-by: Nicolin Chen <Guangyu.Chen@freescale.com>
Signed-off-by: Shengjiu Wang <shengjiu.wang@freescale.com>
---
 arch/arm/mach-imx/clk-imx6q.c             |   18 ++++++++++++++----
 include/dt-bindings/clock/imx6qdl-clock.h |    8 +++++++-
 2 files changed, 21 insertions(+), 5 deletions(-)

Comments

Shawn Guo Aug. 9, 2014, 1:58 p.m. UTC | #1 
On Fri, Aug 08, 2014 at 03:02:49PM +0800, Shengjiu Wang wrote:
> @@ -176,8 +182,12 @@ static void __init imx6q_clocks_init(struct device_node *ccm_node)
>  	 * the "output_enable" bit as a gate, even though it's really just
>  	 * enabling clock output.
>  	 */
> -	clk[IMX6QDL_CLK_LVDS1_GATE] = imx_clk_gate("lvds1_gate", "lvds1_sel", base + 0x160, 10);
> -	clk[IMX6QDL_CLK_LVDS2_GATE] = imx_clk_gate("lvds2_gate", "lvds2_sel", base + 0x160, 11);
> +	clk[IMX6QDL_CLK_LVDS1_GATE] = imx_clk_gate2("lvds1_gate", "lvds1_sel", base + 0x160, 10);
> +	clk[IMX6QDL_CLK_LVDS2_GATE] = imx_clk_gate2("lvds2_gate", "lvds2_sel", base + 0x160, 11);

I do not think you can simply change to use imx_clk_gate2() here.  It's
designed for those CCGR gate clocks, each of which is controlled by two
bits.

Shawn

> +	clk[IMX6QDL_CLK_LVDS1_IN] = imx_clk_gate2("lvds1_in", "anaclk1", base + 0x160, 12);
> +	clk[IMX6QDL_CLK_LVDS2_IN] = imx_clk_gate2("lvds2_in", "anaclk2", base + 0x160, 13);
> +	imx_clk_gate2_exclusive(clk[IMX6QDL_CLK_LVDS1_GATE], clk[IMX6QDL_CLK_LVDS1_IN]);
> +	imx_clk_gate2_exclusive(clk[IMX6QDL_CLK_LVDS2_GATE], clk[IMX6QDL_CLK_LVDS2_IN]);
>  
>  	/*                                            name              parent_name        reg       idx */
>  	clk[IMX6QDL_CLK_PLL2_PFD0_352M] = imx_clk_pfd("pll2_pfd0_352m", "pll2_bus",     base + 0x100, 0);
Shengjiu Wang Aug. 11, 2014, 3:09 a.m. UTC | #2 
On Sat, Aug 09, 2014 at 09:58:42PM +0800, Shawn Guo wrote:
> On Fri, Aug 08, 2014 at 03:02:49PM +0800, Shengjiu Wang wrote:
> > @@ -176,8 +182,12 @@ static void __init imx6q_clocks_init(struct device_node *ccm_node)
> >  	 * the "output_enable" bit as a gate, even though it's really just
> >  	 * enabling clock output.
> >  	 */
> > -	clk[IMX6QDL_CLK_LVDS1_GATE] = imx_clk_gate("lvds1_gate", "lvds1_sel", base + 0x160, 10);
> > -	clk[IMX6QDL_CLK_LVDS2_GATE] = imx_clk_gate("lvds2_gate", "lvds2_sel", base + 0x160, 11);
> > +	clk[IMX6QDL_CLK_LVDS1_GATE] = imx_clk_gate2("lvds1_gate", "lvds1_sel", base + 0x160, 10);
> > +	clk[IMX6QDL_CLK_LVDS2_GATE] = imx_clk_gate2("lvds2_gate", "lvds2_sel", base + 0x160, 11);
> 
> I do not think you can simply change to use imx_clk_gate2() here.  It's
> designed for those CCGR gate clocks, each of which is controlled by two
> bits.
> 
> Shawn
>
As Lucas Stach's suggestion, we need to do add some method for mutually exclusive clock, 
lvds1_gate with lvds1_in, lvds2_gate with lvds2_in. I add imx_clk_gate2_exclusive() function in clk-gate2.c.
So I change imx_clk_gate() to imx_clk_gate2() here.
As you said, this is not good solution. So I need your suggestion, how can I do?
First, is it allowable that to add imx_clk_gate2_exclusive() function, is there a more better way?
second, or should I change the clk-gate.c to add exclusive control?

Thanks
wang shengjiu

> > +	clk[IMX6QDL_CLK_LVDS1_IN] = imx_clk_gate2("lvds1_in", "anaclk1", base + 0x160, 12);
> > +	clk[IMX6QDL_CLK_LVDS2_IN] = imx_clk_gate2("lvds2_in", "anaclk2", base + 0x160, 13);
> > +	imx_clk_gate2_exclusive(clk[IMX6QDL_CLK_LVDS1_GATE], clk[IMX6QDL_CLK_LVDS1_IN]);
> > +	imx_clk_gate2_exclusive(clk[IMX6QDL_CLK_LVDS2_GATE], clk[IMX6QDL_CLK_LVDS2_IN]);
> >  
> >  	/*                                            name              parent_name        reg       idx */
> >  	clk[IMX6QDL_CLK_PLL2_PFD0_352M] = imx_clk_pfd("pll2_pfd0_352m", "pll2_bus",     base + 0x100, 0);
Shawn Guo Aug. 18, 2014, 6:06 a.m. UTC | #3 
On Mon, Aug 11, 2014 at 11:09:36AM +0800, Shengjiu Wang wrote:
> On Sat, Aug 09, 2014 at 09:58:42PM +0800, Shawn Guo wrote:
> > On Fri, Aug 08, 2014 at 03:02:49PM +0800, Shengjiu Wang wrote:
> > > @@ -176,8 +182,12 @@ static void __init imx6q_clocks_init(struct device_node *ccm_node)
> > >  	 * the "output_enable" bit as a gate, even though it's really just
> > >  	 * enabling clock output.
> > >  	 */
> > > -	clk[IMX6QDL_CLK_LVDS1_GATE] = imx_clk_gate("lvds1_gate", "lvds1_sel", base + 0x160, 10);
> > > -	clk[IMX6QDL_CLK_LVDS2_GATE] = imx_clk_gate("lvds2_gate", "lvds2_sel", base + 0x160, 11);
> > > +	clk[IMX6QDL_CLK_LVDS1_GATE] = imx_clk_gate2("lvds1_gate", "lvds1_sel", base + 0x160, 10);
> > > +	clk[IMX6QDL_CLK_LVDS2_GATE] = imx_clk_gate2("lvds2_gate", "lvds2_sel", base + 0x160, 11);
> > 
> > I do not think you can simply change to use imx_clk_gate2() here.  It's
> > designed for those CCGR gate clocks, each of which is controlled by two
> > bits.
> > 
> > Shawn
> >
> As Lucas Stach's suggestion, we need to do add some method for mutually exclusive clock, 
> lvds1_gate with lvds1_in, lvds2_gate with lvds2_in. I add imx_clk_gate2_exclusive() function in clk-gate2.c.
> So I change imx_clk_gate() to imx_clk_gate2() here.
> As you said, this is not good solution.

It's not just a "not good" solution but wrong and broken one.  The net
result of that is if you call clk_enable() on lvds1_gate, both bit 10
and 11 will be set.

> So I need your suggestion, how can I do?

I guess we will need a new clock type to handle such mutually exclusive
clocks, rather than patching clk-gate2.

> First, is it allowable that to add imx_clk_gate2_exclusive() function, is there a more better way?

Again, this is completely wrong.

> second, or should I change the clk-gate.c to add exclusive control?

If such mutually exclusive clocks are somehow common across different
clock controllers, we can propose to change clk-gate.c for handling
them.  But I'm not sure this is a common case.

Shawn
Shengjiu Wang Aug. 25, 2014, 7:40 a.m. UTC | #4 
On Mon, Aug 18, 2014 at 02:06:07PM +0800, Shawn Guo wrote:
> On Mon, Aug 11, 2014 at 11:09:36AM +0800, Shengjiu Wang wrote:
> > On Sat, Aug 09, 2014 at 09:58:42PM +0800, Shawn Guo wrote:
> > > On Fri, Aug 08, 2014 at 03:02:49PM +0800, Shengjiu Wang wrote:
> > > > @@ -176,8 +182,12 @@ static void __init imx6q_clocks_init(struct device_node *ccm_node)
> > > >  	 * the "output_enable" bit as a gate, even though it's really just
> > > >  	 * enabling clock output.
> > > >  	 */
> > > > -	clk[IMX6QDL_CLK_LVDS1_GATE] = imx_clk_gate("lvds1_gate", "lvds1_sel", base + 0x160, 10);
> > > > -	clk[IMX6QDL_CLK_LVDS2_GATE] = imx_clk_gate("lvds2_gate", "lvds2_sel", base + 0x160, 11);
> > > > +	clk[IMX6QDL_CLK_LVDS1_GATE] = imx_clk_gate2("lvds1_gate", "lvds1_sel", base + 0x160, 10);
> > > > +	clk[IMX6QDL_CLK_LVDS2_GATE] = imx_clk_gate2("lvds2_gate", "lvds2_sel", base + 0x160, 11);
> > > 
> > > I do not think you can simply change to use imx_clk_gate2() here.  It's
> > > designed for those CCGR gate clocks, each of which is controlled by two
> > > bits.
> > > 
> > > Shawn
> > >
> > As Lucas Stach's suggestion, we need to do add some method for mutually exclusive clock, 
> > lvds1_gate with lvds1_in, lvds2_gate with lvds2_in. I add imx_clk_gate2_exclusive() function in clk-gate2.c.
> > So I change imx_clk_gate() to imx_clk_gate2() here.
> > As you said, this is not good solution.
> 
> It's not just a "not good" solution but wrong and broken one.  The net
> result of that is if you call clk_enable() on lvds1_gate, both bit 10
> and 11 will be set.
> 
> > So I need your suggestion, how can I do?
> 
> I guess we will need a new clock type to handle such mutually exclusive
> clocks, rather than patching clk-gate2.
> 
Could you please help to implement this feature?

Furthermore, I'd like to drop patch 2 and patch 3, wait the implementation from
you.

Could you please review the patch 1?  do you have any comments?

Wang Shengjiu

> > First, is it allowable that to add imx_clk_gate2_exclusive() function, is there a more better way?
> 
> Again, this is completely wrong.
> 
> > second, or should I change the clk-gate.c to add exclusive control?
> 
> If such mutually exclusive clocks are somehow common across different
> clock controllers, we can propose to change clk-gate.c for handling
> them.  But I'm not sure this is a common case.
> 
> Shawn
Shawn Guo Aug. 25, 2014, 11:21 a.m. UTC | #5 
On Mon, Aug 25, 2014 at 03:40:20PM +0800, Shengjiu Wang wrote:
> On Mon, Aug 18, 2014 at 02:06:07PM +0800, Shawn Guo wrote:
> > On Mon, Aug 11, 2014 at 11:09:36AM +0800, Shengjiu Wang wrote:
> > > On Sat, Aug 09, 2014 at 09:58:42PM +0800, Shawn Guo wrote:
> > > > On Fri, Aug 08, 2014 at 03:02:49PM +0800, Shengjiu Wang wrote:
> > > > > @@ -176,8 +182,12 @@ static void __init imx6q_clocks_init(struct device_node *ccm_node)
> > > > >  	 * the "output_enable" bit as a gate, even though it's really just
> > > > >  	 * enabling clock output.
> > > > >  	 */
> > > > > -	clk[IMX6QDL_CLK_LVDS1_GATE] = imx_clk_gate("lvds1_gate", "lvds1_sel", base + 0x160, 10);
> > > > > -	clk[IMX6QDL_CLK_LVDS2_GATE] = imx_clk_gate("lvds2_gate", "lvds2_sel", base + 0x160, 11);
> > > > > +	clk[IMX6QDL_CLK_LVDS1_GATE] = imx_clk_gate2("lvds1_gate", "lvds1_sel", base + 0x160, 10);
> > > > > +	clk[IMX6QDL_CLK_LVDS2_GATE] = imx_clk_gate2("lvds2_gate", "lvds2_sel", base + 0x160, 11);
> > > > 
> > > > I do not think you can simply change to use imx_clk_gate2() here.  It's
> > > > designed for those CCGR gate clocks, each of which is controlled by two
> > > > bits.
> > > > 
> > > > Shawn
> > > >
> > > As Lucas Stach's suggestion, we need to do add some method for mutually exclusive clock, 
> > > lvds1_gate with lvds1_in, lvds2_gate with lvds2_in. I add imx_clk_gate2_exclusive() function in clk-gate2.c.
> > > So I change imx_clk_gate() to imx_clk_gate2() here.
> > > As you said, this is not good solution.
> > 
> > It's not just a "not good" solution but wrong and broken one.  The net
> > result of that is if you call clk_enable() on lvds1_gate, both bit 10
> > and 11 will be set.
> > 
> > > So I need your suggestion, how can I do?
> > 
> > I guess we will need a new clock type to handle such mutually exclusive
> > clocks, rather than patching clk-gate2.
> > 
> Could you please help to implement this feature?

Okay, I will give it a try soon.

> 
> Furthermore, I'd like to drop patch 2 and patch 3, wait the implementation from
> you.
> 
> Could you please review the patch 1?  do you have any comments?

Just applied #1.

Shawn
diff mbox

Patch

diff --git a/arch/arm/mach-imx/clk-imx6q.c b/arch/arm/mach-imx/clk-imx6q.c
index 1d6dd59..c1f285c 100644
--- a/arch/arm/mach-imx/clk-imx6q.c
+++ b/arch/arm/mach-imx/clk-imx6q.c
@@ -73,6 +73,7 @@  static const char *lvds_sels[] = {
 	"pll4_audio", "pll5_video", "pll8_mlb", "enet_ref",
 	"pcie_ref_125m", "sata_ref_100m",
 };
+static const char *pll_av_sels[] = { "osc", "lvds1_in", "lvds2_in", "dummy", };
 
 static struct clk *clk[IMX6QDL_CLK_END];
 static struct clk_onecell_data clk_data;
@@ -119,6 +120,9 @@  static void __init imx6q_clocks_init(struct device_node *ccm_node)
 	clk[IMX6QDL_CLK_CKIL] = imx_obtain_fixed_clock("ckil", 0);
 	clk[IMX6QDL_CLK_CKIH] = imx_obtain_fixed_clock("ckih1", 0);
 	clk[IMX6QDL_CLK_OSC] = imx_obtain_fixed_clock("osc", 0);
+	/* Clock source from external clock via ANACLK1/2 PADs */
+	clk[IMX6QDL_CLK_ANACLK1] = imx_obtain_fixed_clock("anaclk1", 0);
+	clk[IMX6QDL_CLK_ANACLK2] = imx_obtain_fixed_clock("anaclk2", 0);
 
 	np = of_find_compatible_node(NULL, NULL, "fsl,imx6q-anatop");
 	base = of_iomap(np, 0);
@@ -136,8 +140,8 @@  static void __init imx6q_clocks_init(struct device_node *ccm_node)
 	clk[IMX6QDL_CLK_PLL1_SYS]      = imx_clk_pllv3(IMX_PLLV3_SYS,	"pll1_sys",	"osc", base,        0x7f);
 	clk[IMX6QDL_CLK_PLL2_BUS]      = imx_clk_pllv3(IMX_PLLV3_GENERIC,	"pll2_bus",	"osc", base + 0x30, 0x1);
 	clk[IMX6QDL_CLK_PLL3_USB_OTG]  = imx_clk_pllv3(IMX_PLLV3_USB,	"pll3_usb_otg",	"osc", base + 0x10, 0x3);
-	clk[IMX6QDL_CLK_PLL4_AUDIO]    = imx_clk_pllv3(IMX_PLLV3_AV,	"pll4_audio",	"osc", base + 0x70, 0x7f);
-	clk[IMX6QDL_CLK_PLL5_VIDEO]    = imx_clk_pllv3(IMX_PLLV3_AV,	"pll5_video",	"osc", base + 0xa0, 0x7f);
+	clk[IMX6QDL_CLK_PLL4_AUDIO]    = imx_clk_pllv3(IMX_PLLV3_AV,	"pll4_audio",	"pll4_sel", base + 0x70, 0x7f);
+	clk[IMX6QDL_CLK_PLL5_VIDEO]    = imx_clk_pllv3(IMX_PLLV3_AV,	"pll5_video",	"pll5_sel", base + 0xa0, 0x7f);
 	clk[IMX6QDL_CLK_PLL6_ENET]     = imx_clk_pllv3(IMX_PLLV3_ENET,	"pll6_enet",	"osc", base + 0xe0, 0x3);
 	clk[IMX6QDL_CLK_PLL7_USB_HOST] = imx_clk_pllv3(IMX_PLLV3_USB,	"pll7_usb_host","osc", base + 0x20, 0x3);
 
@@ -169,6 +173,8 @@  static void __init imx6q_clocks_init(struct device_node *ccm_node)
 
 	clk[IMX6QDL_CLK_LVDS1_SEL] = imx_clk_mux("lvds1_sel", base + 0x160, 0, 5, lvds_sels, ARRAY_SIZE(lvds_sels));
 	clk[IMX6QDL_CLK_LVDS2_SEL] = imx_clk_mux("lvds2_sel", base + 0x160, 5, 5, lvds_sels, ARRAY_SIZE(lvds_sels));
+	clk[IMX6QDL_CLK_PLL4_SEL]  = imx_clk_mux("pll4_sel",  base + 0x70, 14, 2, pll_av_sels, ARRAY_SIZE(pll_av_sels));
+	clk[IMX6QDL_CLK_PLL5_SEL]  = imx_clk_mux("pll5_sel",  base + 0xa0, 14, 2, pll_av_sels, ARRAY_SIZE(pll_av_sels));
 
 	/*
 	 * lvds1_gate and lvds2_gate are pseudo-gates.  Both can be
@@ -176,8 +182,12 @@  static void __init imx6q_clocks_init(struct device_node *ccm_node)
 	 * the "output_enable" bit as a gate, even though it's really just
 	 * enabling clock output.
 	 */
-	clk[IMX6QDL_CLK_LVDS1_GATE] = imx_clk_gate("lvds1_gate", "lvds1_sel", base + 0x160, 10);
-	clk[IMX6QDL_CLK_LVDS2_GATE] = imx_clk_gate("lvds2_gate", "lvds2_sel", base + 0x160, 11);
+	clk[IMX6QDL_CLK_LVDS1_GATE] = imx_clk_gate2("lvds1_gate", "lvds1_sel", base + 0x160, 10);
+	clk[IMX6QDL_CLK_LVDS2_GATE] = imx_clk_gate2("lvds2_gate", "lvds2_sel", base + 0x160, 11);
+	clk[IMX6QDL_CLK_LVDS1_IN] = imx_clk_gate2("lvds1_in", "anaclk1", base + 0x160, 12);
+	clk[IMX6QDL_CLK_LVDS2_IN] = imx_clk_gate2("lvds2_in", "anaclk2", base + 0x160, 13);
+	imx_clk_gate2_exclusive(clk[IMX6QDL_CLK_LVDS1_GATE], clk[IMX6QDL_CLK_LVDS1_IN]);
+	imx_clk_gate2_exclusive(clk[IMX6QDL_CLK_LVDS2_GATE], clk[IMX6QDL_CLK_LVDS2_IN]);
 
 	/*                                            name              parent_name        reg       idx */
 	clk[IMX6QDL_CLK_PLL2_PFD0_352M] = imx_clk_pfd("pll2_pfd0_352m", "pll2_bus",     base + 0x100, 0);
diff --git a/include/dt-bindings/clock/imx6qdl-clock.h b/include/dt-bindings/clock/imx6qdl-clock.h
index 323e865..5519526 100644
--- a/include/dt-bindings/clock/imx6qdl-clock.h
+++ b/include/dt-bindings/clock/imx6qdl-clock.h
@@ -220,6 +220,12 @@ 
 #define IMX6QDL_CLK_LVDS2_GATE			207
 #define IMX6QDL_CLK_ESAI_IPG			208
 #define IMX6QDL_CLK_ESAI_MEM			209
-#define IMX6QDL_CLK_END				210
+#define IMX6QDL_CLK_LVDS1_IN			210
+#define IMX6QDL_CLK_LVDS2_IN			211
+#define IMX6QDL_CLK_ANACLK1			212
+#define IMX6QDL_CLK_ANACLK2			213
+#define IMX6QDL_CLK_PLL4_SEL			214
+#define IMX6QDL_CLK_PLL5_SEL			215
+#define IMX6QDL_CLK_END				216
 
 #endif /* __DT_BINDINGS_CLOCK_IMX6QDL_H */