Subject: Re: [PATCH 03/11] driver: clk: imx: Add clock driver for imx6sll

On 12/02, Bai Ping wrote:

quoted

diff --git a/drivers/clk/imx/clk-imx6sll.c

b/drivers/clk/imx/clk-imx6sll.c new file mode 100644 index
0000000..c5219e1

--- /dev/null
+++ b/drivers/clk/imx/clk-imx6sll.c

@@ -0,0 +1,366 @@
+/*
+ * Copyright (C) 2016 Freescale Semiconductor, Inc.
+ *
+ * The code contained herein is licensed under the GNU General Public
+ * License. You may obtain a copy of the GNU General Public License
+ * Version 2 or later at the following locations:
+ *
+ * http://www.opensource.org/licenses/gpl-license.html
+ * http://www.gnu.org/copyleft/gpl.html
+ */
+
+#include <dt-bindings/clock/imx6sll-clock.h>
+#include <linux/clk.h>
+#include <linux/clkdev.h>

Is this include used?

It seems no use, I will remove it in V2.

quoted

+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/types.h>

Is there an include of clk_provider.h missing?

clk_provider.h is included in below "clk.h".

quoted

+
+#include "clk.h"
+
+#define BM_CCM_CCDR_MMDC_CH0_MASK	(0x2 << 16)
+#define CCDR	0x4
+
+static const char *pll_bypass_src_sels[] = { "osc", "dummy", };

const char * const? For all of these names.

ok, I will fix in V2.

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", }; static const
+char *pll7_bypass_sels[] = { "pll7", "pll7_bypass_src", }; static
+const char *step_sels[] = { "osc", "pll2_pfd2_396m", }; static const
+char *pll1_sw_sels[] = { "pll1_sys", "step", }; static const char
+*axi_alt_sels[] = { "pll2_pfd2_396m", "pll3_pfd1_540m", }; static
+const char *axi_sels[] = {"periph", "axi_alt_sel", }; static const
+char *periph_pre_sels[] = { "pll2_bus", "pll2_pfd2_396m",
+"pll2_pfd0_352m", "pll2_198m", }; static const char
+*periph2_pre_sels[] = { "pll2_bus", "pll2_pfd2_396m",
+"pll2_pfd0_352m", "pll4_audio_div", }; static const char
+*periph_clk2_sels[] = { "pll3_usb_otg", "osc", "osc", }; static const
+char *periph2_clk2_sels[] = { "pll3_usb_otg", "osc", }; static const
+char *periph_sels[] = { "periph_pre", "periph_clk2", }; static const
+char *periph2_sels[] = { "periph2_pre", "periph2_clk2", }; static
+const char *usdhc_sels[] = { "pll2_pfd2_396m", "pll2_pfd0_352m", };
+static const char *ssi_sels[] = {"pll3_pfd2_508m", "pll3_pfd3_454m",
+"pll4_audio_div", "dummy",}; static const char *spdif_sels[] = {
+"pll4_audio_div", "pll3_pfd2_508m", "pll5_video_div", "pll3_usb_otg",
+}; static const char *ldb_di0_div_sels[] = { "ldb_di0_div_3_5",
+"ldb_di0_div_7", }; static const char *ldb_di1_div_sels[] = {
+"ldb_di1_div_3_5", "ldb_di1_div_7", }; static const char
+*ldb_di0_sels[] = { "pll5_video_div", "pll2_pfd0_352m",
+"pll2_pfd2_396m", "pll2_pfd3_594m", "pll2_pfd1_594m",
+"pll3_pfd3_454m", }; static const char *ldb_di1_sels[] = {
+"pll3_usb_otg", "pll2_pfd0_352m", "pll2_pfd2_396m", "pll2_bus",
+"pll3_pfd3_454m", "pll3_pfd2_508m", }; static const char
+*lcdif_pre_sels[] = { "pll2_bus", "pll3_pfd3_454m", "pll5_video_div",
+"pll2_pfd0_352m", "pll2_pfd1_594m", "pll3_pfd1_540m", }; static const
+char *ecspi_sels[] = { "pll3_60m", "osc", }; static const char
+*uart_sels[] = { "pll3_80m", "osc", }; static const char
+*perclk_sels[] = { "ipg", "osc", }; static const char *lcdif_sels[] =
+{ "lcdif_podf", "ipp_di0", "ipp_di1", "ldb_di0", "ldb_di1", };
+
+static const char *epdc_pre_sels[] = { "pll2_bus", "pll3_usb_otg",
+"pll5_video_div", "pll2_pfd0_352m", "pll2_pfd2_396m",
+"pll3_pfd2_508m", }; static const char *epdc_sels[] = { "epdc_podf",
+"ipp_di0", "ipp_di1", "ldb_di0", "ldb_di1", };
+
+static struct clk *clks[IMX6SLL_CLK_END]; static struct
+clk_onecell_data clk_data;
+
+static int const clks_init_on[] __initconst = {

static const int is more preferred style.

ok, will fix in V2.

quoted

+	IMX6SLL_CLK_AIPSTZ1, IMX6SLL_CLK_AIPSTZ2,
+	IMX6SLL_CLK_OCRAM, IMX6SLL_CLK_ARM, IMX6SLL_CLK_ROM,
+	IMX6SLL_CLK_MMDC_P0_FAST, IMX6SLL_CLK_MMDC_P0_IPG, };
+
+static struct clk_div_table post_div_table[] = {

const?

ok, will fix in V2.

quoted

+	{ .val = 2, .div = 1, },
+	{ .val = 1, .div = 2, },
+	{ .val = 0, .div = 4, },
+	{ }
+};
+
+static struct clk_div_table video_div_table[] = {

const?

ok. will fix in V2.

quoted

+	{ .val = 0, .div = 1, },
+	{ .val = 1, .div = 2, },
+	{ .val = 2, .div = 1, },
+	{ .val = 3, .div = 4, },
+	{ }
+};
+
+static u32 share_count_audio;
+static u32 share_count_ssi1;
+static u32 share_count_ssi2;
+static u32 share_count_ssi3;
+
+static void __init imx6sll_clocks_init(struct device_node *ccm_node)
+{
+	struct device_node *np;
+	void __iomem *base;
+	int i;
+
+	clks[IMX6SLL_CLK_DUMMY] = imx_clk_fixed("dummy", 0);
+
+	clks[IMX6SLL_CLK_CKIL] = of_clk_get_by_name(ccm_node, "ckil");
+	clks[IMX6SLL_CLK_OSC] = of_clk_get_by_name(ccm_node, "osc");
+
+	/* ipp_di clock is external input */
+	clks[IMX6SLL_CLK_IPP_DI0] = of_clk_get_by_name(ccm_node,

"ipp_di0");

quoted

+	clks[IMX6SLL_CLK_IPP_DI1] = of_clk_get_by_name(ccm_node,

"ipp_di1");

quoted

+
+	np = of_find_compatible_node(NULL, NULL, "fsl,imx6sll-anatop");
+	base = of_iomap(np, 0);
+	WARN_ON(!base);
+
+	clks[IMX6SLL_PLL1_BYPASS_SRC] = imx_clk_mux("pll1_bypass_src",

base + 0x00, 14, 1, pll_bypass_src_sels, ARRAY_SIZE(pll_bypass_src_sels));

quoted

+	clks[IMX6SLL_PLL2_BYPASS_SRC] = imx_clk_mux("pll2_bypass_src",

base + 0x30, 14, 1, pll_bypass_src_sels, ARRAY_SIZE(pll_bypass_src_sels));

quoted

+	clks[IMX6SLL_PLL3_BYPASS_SRC] = imx_clk_mux("pll3_bypass_src",

base + 0x10, 14, 1, pll_bypass_src_sels, ARRAY_SIZE(pll_bypass_src_sels));

quoted

+	clks[IMX6SLL_PLL4_BYPASS_SRC] = imx_clk_mux("pll4_bypass_src",

base + 0x70, 14, 1, pll_bypass_src_sels, ARRAY_SIZE(pll_bypass_src_sels));

quoted

+	clks[IMX6SLL_PLL5_BYPASS_SRC] = imx_clk_mux("pll5_bypass_src",

base + 0xa0, 14, 1, pll_bypass_src_sels, ARRAY_SIZE(pll_bypass_src_sels));

quoted

+	clks[IMX6SLL_PLL6_BYPASS_SRC] = imx_clk_mux("pll6_bypass_src",

base + 0xe0, 14, 1, pll_bypass_src_sels, ARRAY_SIZE(pll_bypass_src_sels));

quoted

+	clks[IMX6SLL_PLL7_BYPASS_SRC] = imx_clk_mux("pll7_bypass_src",

base

quoted

++ 0x20, 14, 1, pll_bypass_src_sels, ARRAY_SIZE(pll_bypass_src_sels));
+
+	clks[IMX6SLL_CLK_PLL1] = imx_clk_pllv3(IMX_PLLV3_SYS,	 "pll1",

"pll1_bypass_src", base + 0x00, 0x7f);

quoted

+	clks[IMX6SLL_CLK_PLL2] = imx_clk_pllv3(IMX_PLLV3_GENERIC, "pll2",

"pll2_bypass_src", base + 0x30, 0x1);

quoted

+	clks[IMX6SLL_CLK_PLL3] = imx_clk_pllv3(IMX_PLLV3_USB,	 "pll3",

"pll3_bypass_src", base + 0x10, 0x3);

quoted

+	clks[IMX6SLL_CLK_PLL4] = imx_clk_pllv3(IMX_PLLV3_AV,	 "pll4",

"pll4_bypass_src", base + 0x70, 0x7f);

quoted

+	clks[IMX6SLL_CLK_PLL5] = imx_clk_pllv3(IMX_PLLV3_AV,	 "pll5",

"pll5_bypass_src", base + 0xa0, 0x7f);

quoted

+	clks[IMX6SLL_CLK_PLL6] = imx_clk_pllv3(IMX_PLLV3_ENET,	 "pll6",

"pll6_bypass_src", base + 0xe0, 0x3);

quoted

+	clks[IMX6SLL_CLK_PLL7] = imx_clk_pllv3(IMX_PLLV3_USB,	 "pll7",

"pll7_bypass_src", base + 0x20, 0x3);

quoted

+
+	clks[IMX6SLL_PLL1_BYPASS] = imx_clk_mux_flags("pll1_bypass", base

+ 0x00, 16, 1, pll1_bypass_sels, ARRAY_SIZE(pll1_bypass_sels),
CLK_SET_RATE_PARENT);

quoted

+	clks[IMX6SLL_PLL2_BYPASS] = imx_clk_mux_flags("pll2_bypass", base

+ 0x30, 16, 1, pll2_bypass_sels, ARRAY_SIZE(pll2_bypass_sels),
CLK_SET_RATE_PARENT);

quoted

+	clks[IMX6SLL_PLL3_BYPASS] = imx_clk_mux_flags("pll3_bypass", base

+ 0x10, 16, 1, pll3_bypass_sels, ARRAY_SIZE(pll3_bypass_sels),
CLK_SET_RATE_PARENT);

quoted

+	clks[IMX6SLL_PLL4_BYPASS] = imx_clk_mux_flags("pll4_bypass", base

+ 0x70, 16, 1, pll4_bypass_sels, ARRAY_SIZE(pll4_bypass_sels),
CLK_SET_RATE_PARENT);

quoted

+	clks[IMX6SLL_PLL5_BYPASS] = imx_clk_mux_flags("pll5_bypass", base

+ 0xa0, 16, 1, pll5_bypass_sels, ARRAY_SIZE(pll5_bypass_sels),
CLK_SET_RATE_PARENT);

quoted

+	clks[IMX6SLL_PLL6_BYPASS] = imx_clk_mux_flags("pll6_bypass", base

+ 0xe0, 16, 1, pll6_bypass_sels, ARRAY_SIZE(pll6_bypass_sels),
CLK_SET_RATE_PARENT);

quoted

+	clks[IMX6SLL_PLL7_BYPASS] = imx_clk_mux_flags("pll7_bypass", base

+

quoted

+0x20, 16, 1, pll7_bypass_sels, ARRAY_SIZE(pll7_bypass_sels),
+CLK_SET_RATE_PARENT);
+
+	/* Do not bypass PLLs initially */
+	clk_set_parent(clks[IMX6SLL_PLL1_BYPASS], clks[IMX6SLL_CLK_PLL1]);
+	clk_set_parent(clks[IMX6SLL_PLL2_BYPASS], clks[IMX6SLL_CLK_PLL2]);
+	clk_set_parent(clks[IMX6SLL_PLL3_BYPASS], clks[IMX6SLL_CLK_PLL3]);
+	clk_set_parent(clks[IMX6SLL_PLL4_BYPASS], clks[IMX6SLL_CLK_PLL4]);
+	clk_set_parent(clks[IMX6SLL_PLL5_BYPASS], clks[IMX6SLL_CLK_PLL5]);
+	clk_set_parent(clks[IMX6SLL_PLL6_BYPASS], clks[IMX6SLL_CLK_PLL6]);
+	clk_set_parent(clks[IMX6SLL_PLL7_BYPASS], clks[IMX6SLL_CLK_PLL7]);

Can we just put raw register writes here instead? I'd prefer we didn't use clk
consumer APIs to do things to the clk tree from the providers. The problem
there being that:

 1) We're trying to move away from using consumer APIs in  provider drivers.
It's ok if they're used during probe, but  inside clk_ops is not preferred.

 2) Even if you have a clk pointer, it may be "orphaned" at the  time of
registration and so calling the APIs here works now but  eventually we may
want to return an EPROBE_DEFER error in that  case and this may block that
effort.

I suppose if this is the only clk driver on this machine then this last point isn't a
concern and things are probably ok here.

Using raw register writing has an issue.  The register is modified, but it seems the clock 'parent-child' relationship can 
not match the register setting. The register setting is not bypass the pll, but in debug 'clk_summary', the
pll is bypassed.  

BR

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