Patchwork [4/4] ARM: imx6q: remove board specific CLKO setup

login
register
mail settings
Submitter Shawn Guo
Date July 18, 2013, 7:31 a.m.
Message ID <1374132670-791-5-git-send-email-shawn.guo@linaro.org>
Download mbox | patch
Permalink /patch/260013/
State New
Headers show

Comments

Shawn Guo - July 18, 2013, 7:31 a.m.
The CLKO is widely used by imx6q board designs to clock audio codec.
Since most codecs accept 24 MHz frequency, let's initially set up CLKO
with OSC24M (cko <-- cko2 <-- osc).  Then those board specific CLKO
setup for audio codec can be removed.

The board dts files also need an update on cko reference in codec node.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 arch/arm/boot/dts/imx6q-sabrelite.dts    |    2 +-
 arch/arm/boot/dts/imx6qdl-sabresd.dtsi   |    2 +-
 arch/arm/boot/dts/imx6qdl-wandboard.dtsi |    2 +-
 arch/arm/mach-imx/clk-imx6q.c            |   11 +++++
 arch/arm/mach-imx/mach-imx6q.c           |   69 ------------------------------
 5 files changed, 14 insertions(+), 72 deletions(-)
Sascha Hauer - July 18, 2013, 8:36 a.m.
On Thu, Jul 18, 2013 at 03:31:10PM +0800, Shawn Guo wrote:
> The CLKO is widely used by imx6q board designs to clock audio codec.
> Since most codecs accept 24 MHz frequency, let's initially set up CLKO
> with OSC24M (cko <-- cko2 <-- osc).  Then those board specific CLKO
> setup for audio codec can be removed.
> 
> The board dts files also need an update on cko reference in codec node.
> 
> +	/*
> +	 * Let's initially set up CLKO with OSC24M, since this configuration
> +	 * is widely used by imx6q board designs to clock audio codec.
> +	 */
> +	ret = clk_set_parent(clk[cko2_sel], clk[osc]);
> +	if (!ret)
> +		ret = clk_set_parent(clk[cko], clk[cko2]);
> +	if (ret)
> +		pr_warn("failed to set up CLKO: %d\n", ret);


I don't really like that the clock setup code fiddles with clocks it has
no knowledge about just because it happens to be correct for several
boards.

I currently have no example where this bites me, so I think I can live
with it for now. I just wonder what I should do when this clock is used
for something vital to the system (clocking an FPGA for example?).

Sascha
Shawn Guo - July 18, 2013, 11:55 a.m.
On Thu, Jul 18, 2013 at 10:36:03AM +0200, Sascha Hauer wrote:
> On Thu, Jul 18, 2013 at 03:31:10PM +0800, Shawn Guo wrote:
> > The CLKO is widely used by imx6q board designs to clock audio codec.
> > Since most codecs accept 24 MHz frequency, let's initially set up CLKO
> > with OSC24M (cko <-- cko2 <-- osc).  Then those board specific CLKO
> > setup for audio codec can be removed.
> > 
> > The board dts files also need an update on cko reference in codec node.
> > 
> > +	/*
> > +	 * Let's initially set up CLKO with OSC24M, since this configuration
> > +	 * is widely used by imx6q board designs to clock audio codec.
> > +	 */
> > +	ret = clk_set_parent(clk[cko2_sel], clk[osc]);
> > +	if (!ret)
> > +		ret = clk_set_parent(clk[cko], clk[cko2]);
> > +	if (ret)
> > +		pr_warn("failed to set up CLKO: %d\n", ret);
> 
> 
> I don't really like that the clock setup code fiddles with clocks it has
> no knowledge about just because it happens to be correct for several
> boards.

I have to admit it's not the best way to handle it, and I can understand
your concern.  What I like about it is we can remove those board
specific hooks from mach-imx6q.c at least for now.

> 
> I currently have no example where this bites me, so I think I can live
> with it for now. I just wonder what I should do when this clock is used
> for something vital to the system (clocking an FPGA for example?).

Go back to set up CLKO in mach-imx6q.c with board specific hook for the
system?

I think I chose to change the default CLKO setup for audio use case, as
all the users we've seen so far is audio codec.  I can still have this
setup done in mach-imx6q.c with of_machine_is_compatible() check on all
those board.  But that also means we need to register lookups for those
CLKO clocks and call clk_get_sys() to get them.

Shawn

Patch

diff --git a/arch/arm/boot/dts/imx6q-sabrelite.dts b/arch/arm/boot/dts/imx6q-sabrelite.dts
index e0f4cd0..3530280 100644
--- a/arch/arm/boot/dts/imx6q-sabrelite.dts
+++ b/arch/arm/boot/dts/imx6q-sabrelite.dts
@@ -167,7 +167,7 @@ 
 	codec: sgtl5000@0a {
 		compatible = "fsl,sgtl5000";
 		reg = <0x0a>;
-		clocks = <&clks 169>;
+		clocks = <&clks 201>;
 		VDDA-supply = <&reg_2p5v>;
 		VDDIO-supply = <&reg_3p3v>;
 	};
diff --git a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
index 166b9ae..75b36b2 100644
--- a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
@@ -109,7 +109,7 @@ 
 	codec: wm8962@1a {
 		compatible = "wlf,wm8962";
 		reg = <0x1a>;
-		clocks = <&clks 169>;
+		clocks = <&clks 201>;
 		DCVDD-supply = <&reg_audio>;
 		DBVDD-supply = <&reg_audio>;
 		AVDD-supply = <&reg_audio>;
diff --git a/arch/arm/boot/dts/imx6qdl-wandboard.dtsi b/arch/arm/boot/dts/imx6qdl-wandboard.dtsi
index 7a46f64..77188cb 100644
--- a/arch/arm/boot/dts/imx6qdl-wandboard.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-wandboard.dtsi
@@ -60,7 +60,7 @@ 
 	codec: sgtl5000@0a {
 		compatible = "fsl,sgtl5000";
 		reg = <0x0a>;
-		clocks = <&clks 169>;
+		clocks = <&clks 201>;
 		VDDA-supply = <&reg_2p5v>;
 		VDDIO-supply = <&reg_3p3v>;
 	};
diff --git a/arch/arm/mach-imx/clk-imx6q.c b/arch/arm/mach-imx/clk-imx6q.c
index ed19b33..97f2702 100644
--- a/arch/arm/mach-imx/clk-imx6q.c
+++ b/arch/arm/mach-imx/clk-imx6q.c
@@ -288,6 +288,7 @@  static void __init imx6q_clocks_init(struct device_node *ccm_node)
 	struct device_node *np;
 	void __iomem *base;
 	int i, irq;
+	int ret;
 
 	clk[dummy] = imx_clk_fixed("dummy", 0);
 	clk[ckil] = imx_obtain_fixed_clock("ckil", 0);
@@ -591,6 +592,16 @@  static void __init imx6q_clocks_init(struct device_node *ccm_node)
 		clk_prepare_enable(clk[usbphy2_gate]);
 	}
 
+	/*
+	 * Let's initially set up CLKO with OSC24M, since this configuration
+	 * is widely used by imx6q board designs to clock audio codec.
+	 */
+	ret = clk_set_parent(clk[cko2_sel], clk[osc]);
+	if (!ret)
+		ret = clk_set_parent(clk[cko], clk[cko2]);
+	if (ret)
+		pr_warn("failed to set up CLKO: %d\n", ret);
+
 	/* Set initial power mode */
 	imx6q_set_lpm(WAIT_CLOCKED);
 
diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c
index 9f06cc8..2460d49 100644
--- a/arch/arm/mach-imx/mach-imx6q.c
+++ b/arch/arm/mach-imx/mach-imx6q.c
@@ -159,30 +159,6 @@  static int ar8031_phy_fixup(struct phy_device *dev)
 	return 0;
 }
 
-static void __init imx6q_sabrelite_cko1_setup(void)
-{
-	struct clk *cko1_sel, *ahb, *cko1;
-	unsigned long rate;
-
-	cko1_sel = clk_get_sys(NULL, "cko1_sel");
-	ahb = clk_get_sys(NULL, "ahb");
-	cko1 = clk_get_sys(NULL, "cko1");
-	if (IS_ERR(cko1_sel) || IS_ERR(ahb) || IS_ERR(cko1)) {
-		pr_err("cko1 setup failed!\n");
-		goto put_clk;
-	}
-	clk_set_parent(cko1_sel, ahb);
-	rate = clk_round_rate(cko1, 16000000);
-	clk_set_rate(cko1, rate);
-put_clk:
-	if (!IS_ERR(cko1_sel))
-		clk_put(cko1_sel);
-	if (!IS_ERR(ahb))
-		clk_put(ahb);
-	if (!IS_ERR(cko1))
-		clk_put(cko1);
-}
-
 #define PHY_ID_AR8031	0x004dd074
 
 static void __init imx6q_enet_phy_init(void)
@@ -197,45 +173,6 @@  static void __init imx6q_enet_phy_init(void)
 	}
 }
 
-static void __init imx6q_sabresd_cko1_setup(void)
-{
-	struct clk *cko1_sel, *pll4, *pll4_post, *cko1;
-	unsigned long rate;
-
-	cko1_sel = clk_get_sys(NULL, "cko1_sel");
-	pll4 = clk_get_sys(NULL, "pll4_audio");
-	pll4_post = clk_get_sys(NULL, "pll4_post_div");
-	cko1 = clk_get_sys(NULL, "cko1");
-	if (IS_ERR(cko1_sel) || IS_ERR(pll4)
-			|| IS_ERR(pll4_post) || IS_ERR(cko1)) {
-		pr_err("cko1 setup failed!\n");
-		goto put_clk;
-	}
-	/*
-	 * Setting pll4 at 768MHz (24MHz * 32)
-	 * So its child clock can get 24MHz easily
-	 */
-	clk_set_rate(pll4, 768000000);
-
-	clk_set_parent(cko1_sel, pll4_post);
-	rate = clk_round_rate(cko1, 24000000);
-	clk_set_rate(cko1, rate);
-put_clk:
-	if (!IS_ERR(cko1_sel))
-		clk_put(cko1_sel);
-	if (!IS_ERR(pll4_post))
-		clk_put(pll4_post);
-	if (!IS_ERR(pll4))
-		clk_put(pll4);
-	if (!IS_ERR(cko1))
-		clk_put(cko1);
-}
-
-static void __init imx6q_sabresd_init(void)
-{
-	imx6q_sabresd_cko1_setup();
-}
-
 static void __init imx6q_1588_init(void)
 {
 	struct regmap *gpr;
@@ -256,12 +193,6 @@  static void __init imx6q_usb_init(void)
 
 static void __init imx6q_init_machine(void)
 {
-	if (of_machine_is_compatible("fsl,imx6q-sabrelite"))
-		imx6q_sabrelite_cko1_setup();
-	else if (of_machine_is_compatible("fsl,imx6q-sabresd") ||
-			of_machine_is_compatible("fsl,imx6dl-sabresd"))
-		imx6q_sabresd_init();
-
 	imx6q_enet_phy_init();
 
 	of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);