On 01/10, Jacky Bai wrote:

quoted

quoted

+#include <linux/types.h>
+
+#include "clk.h"
+
+#define CCM_ANALOG_PLL_BYPASS		(0x1 << 16)
+#define BM_CCM_CCDR_MMDC_CH0_MASK	(0x2 << 16)
+#define CCDR	0x4
+#define xPLL_CLR(offset)		 (offset + 0x8)
+
+static const char *pll_bypass_src_sels[] = { "osc", "dummy", };

All these should be const char * const unless something is wrong.

If changed to 'const char * const', it vill has argument type mismatch error, as imx_clk_* wrapper function
has argument type 'const char *'. 

Hmm that's unfortunate.

quoted

quoted

+static const char *pll1_bypass_sels[] = { "pll1", "pll1_bypass_src",
+}; static const char *pll2_bypass_sels[] = { "pll2",
+"pll2_bypass_src", }; static const char *pll3_bypass_sels[] = {
+"pll3", "pll3_bypass_src", }; static const char *pll4_bypass_sels[] =
+{ "pll4", "pll4_bypass_src", }; static const char *pll5_bypass_sels[]
+= { "pll5", "pll5_bypass_src", }; static const char
+*pll6_bypass_sels[] = { "pll6", "pll6_bypass_src", };

[...]

quoted

+	clks[IMX6SLL_CLK_USDHC3]	= imx_clk_gate2("usdhc3",

	"usdhc3_podf",	 base + 0x80,	6);

quoted

+
+	/* mask handshake of mmdc */
+	writel_relaxed(BM_CCM_CCDR_MMDC_CH0_MASK, base + CCDR);
+
+	for (i = 0; i < ARRAY_SIZE(clks); i++)
+		if (IS_ERR(clks[i]))
+			pr_err("i.MX6SLL clk %d: register failed with %ld\n", i,
+PTR_ERR(clks[i]));
+
+	clk_data.clks = clks;
+	clk_data.clk_num = ARRAY_SIZE(clks);
+	of_clk_add_provider(np, of_clk_src_onecell_get, &clk_data);
+
+	/* set perclk to from OSC */
+	clk_set_parent(clks[IMX6SLL_CLK_PERCLK_SEL],

clks[IMX6SLL_CLK_OSC]);

Can this be done with assigned-clocks in DT?

Ok, I will move it to assigned-clocks in DT.

quoted

quoted

+
+	for (i = 0; i < ARRAY_SIZE(clks_init_on); i++)
+		clk_prepare_enable(clks[clks_init_on[i]]);

Critical clocks?

Yes, these clocks must be always on. 

quoted

quoted

+
+	if (IS_ENABLED(CONFIG_USB_MXS_PHY)) {
+		clk_prepare_enable(clks[IMX6SLL_CLK_USBPHY1_GATE]);
+		clk_prepare_enable(clks[IMX6SLL_CLK_USBPHY2_GATE]);

The phy driver can't enable these?

The reason why we enable these two clks here is in below commit
commit a5120e89e7e187a91852896f586876c7a2030804
Author: Peter Chen [off-list ref]
Date:   Fri Jan 18 10:38:05 2013 +0800
      ARM i.MX6: change mxs usbphy clock usage

So can we mark these clks with CLK_IS_CRITICAL flag then instead?
Or are they disabled out of the bootloader?

quoted

quoted

+	}
+
+	/* Lower the AHB clock rate before changing the clock source. */
+	clk_set_rate(clks[IMX6SLL_CLK_AHB], 99000000);
+
+	/* Change periph_pre clock to pll2_bus to adjust AXI rate to 264MHz */
+	clk_set_parent(clks[IMX6SLL_CLK_PERIPH_CLK2_SEL],

clks[IMX6SLL_CLK_PLL3_USB_OTG]);

quoted

+	clk_set_parent(clks[IMX6SLL_CLK_PERIPH],

clks[IMX6SLL_CLK_PERIPH_CLK2]);

quoted

+	clk_set_parent(clks[IMX6SLL_CLK_PERIPH_PRE],

clks[IMX6SLL_CLK_PLL2_BUS]);

quoted

+	clk_set_parent(clks[IMX6SLL_CLK_PERIPH],
+clks[IMX6SLL_CLK_PERIPH_PRE]);
+
+	clk_set_rate(clks[IMX6SLL_CLK_AHB], 132000000);

assigned-clocks for rates now? Or perhaps we shouldn't be exposing these as
clks if they have some sort of complicated rate sequence switch that we can't
guarantee with the clk_ops we have today.

These clks will be used by some peripherals, so we need to expose these clocks.
And the above parent and rate swith sequence is not very easy to be handled in assigned-clocks,
So we leave it in this place. 

How do we guarantee that the rate switch doesn't happen later on,
requiring this coordinated sequence of clk operations?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project