diff mbox

[v4] clk: qoriq: Add support for the FMan clock

Message ID 1429185945-6949-1-git-send-email-igal.liberman@freescale.com
State Changes Requested
Delegated to: Scott Wood
Headers show

Commit Message

Igal.Liberman April 16, 2015, 12:05 p.m. UTC
From: Igal Liberman <Igal.Liberman@freescale.com>

This patch depends on the following patches:
	https://patchwork.ozlabs.org/patch/461151/
	https://patchwork.ozlabs.org/patch/461155/

This patche is described by the following binding document update:
	https://patchwork.ozlabs.org/patch/461166/

v4:	- Replaced "fsl,b4-device-config" with
	  "fsl,b4860/b4420-device-config"
	- Updated error messages

v3:	Updated commit message

v2:	- Added clock maintainers
	- Cached FMan clock parent during initialization
	- Register the clock after checking if the hardware exists
	- updated error messages

Signed-off-by: Igal Liberman <Igal.Liberman@freescale.com>
---
 drivers/clk/clk-qoriq.c |  213 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 213 insertions(+)

Comments

Stephen Boyd May 6, 2015, 7:02 a.m. UTC | #1
On 04/16, Igal.Liberman wrote:
> From: Igal Liberman <Igal.Liberman@freescale.com>
> 
> This patch depends on the following patches:
> 	https://patchwork.ozlabs.org/patch/461151/
> 	https://patchwork.ozlabs.org/patch/461155/
> 
> This patche is described by the following binding document update:
> 	https://patchwork.ozlabs.org/patch/461166/
> 
> v4:	- Replaced "fsl,b4-device-config" with
> 	  "fsl,b4860/b4420-device-config"
> 	- Updated error messages
> 
> v3:	Updated commit message
> 
> v2:	- Added clock maintainers
> 	- Cached FMan clock parent during initialization
> 	- Register the clock after checking if the hardware exists
> 	- updated error messages
> 
> Signed-off-by: Igal Liberman <Igal.Liberman@freescale.com>
> ---
>  drivers/clk/clk-qoriq.c |  213 +++++++++++++++++++++++++++++++++++++++++++++++

If I try to compile this on ARM (the Kconfig for this file shows
that ARM is possible) then it fails with this error message:

  CC      drivers/clk/clk-qoriq.o
  drivers/clk/clk-qoriq.c:22:26:
  fatal error: asm/fsl_guts.h: No such file or directory
  compilation terminated.

>  1 file changed, 213 insertions(+)
> 
> diff --git a/drivers/clk/clk-qoriq.c b/drivers/clk/clk-qoriq.c
> index cda90a9..871c6df 100644
> --- a/drivers/clk/clk-qoriq.c
> +++ b/drivers/clk/clk-qoriq.c
> +
> +static u8 get_fm_clk_parent(struct clk_hw *hw)
> +{
> +	return hw->init->flags;
> +}

This is very confusing. How is flags the parent index? Please
don't abuse framework data structures. I'm actually thinking we
should replace hw->init with NULL during clk_register() to avoid
this kind of abuse...

> +
> +static const struct clk_ops fm_clk_ops = {
> +	.get_parent = get_fm_clk_parent,
> +};
> +
> +static int get_fm_clk_idx(int fm_id, int *fm_clk_idx)
> +{
> +	struct ccsr_guts __iomem *guts_regs = NULL;

Unnecessary initialization to NULL. Also, marking a structure as
__iomem is odd. Why do we need to use a struct to figure out
offsets for registers? Why not just use #defines? That would
probably also make it easy to avoid the asm include here.

> +	struct device_node *guts;
> +	uint32_t reg = 0;

s/uint32_t/u32/

Also unnecessary initialization.

> +	int clk_src = 0;
> +
> +	guts = of_find_matching_node(NULL, guts_device_ids);
> +	if (!guts) {
> +		pr_err("%s(): could not find GUTS node\n", __func__);
> +		return -ENODEV;
> +	}
> +
> +	guts_regs = of_iomap(guts, 0);
> +	of_node_put(guts);
> +	if (!guts_regs) {
> +		pr_err("%s(): ioremap of GUTS node failed\n", __func__);
> +		return -ENODEV;
> +	}
[...]
> +
> +static void __init fm_mux_init(struct device_node *np)
> +{
> +	struct clk_init_data *init;
> +	struct clk_hw *hw;
> +	int count, i, ret, fm_id = 0, fm_clk_idx;
> +	struct clk *clk;
> +
> +	init = kmalloc((sizeof(struct clk_init_data)), GFP_KERNEL);

Please remove extra parens and do sizeof(*init) so that we don't
have to care about the type matching.

> +	if (!init)
> +		return;
> +
> +	/* get the input clock source count */
> +	count = of_property_count_strings(np, "clock-names");
> +	if (count < 0) {
> +		pr_err("%s(): %s: get clock count error\n",
> +		       __func__, np->name);
> +		goto err_init;
> +	}
> +
> +	init->parent_names = kmalloc((sizeof(char *) * count), GFP_KERNEL);

Use kcalloc please

> +	if (!init->parent_names)
> +		goto err_init;
> +
> +	for (i = 0; i < count; i++)
> +		init->parent_names[i] = of_clk_get_parent_name(np, i);
> +
> +	hw = kzalloc(sizeof(*hw), GFP_KERNEL);
> +	if (!hw)
> +		goto err_name;
> +
> +	ret = of_property_read_string_index(np, "clock-output-names", 0,
> +					    &init->name);
> +	if (ret) {
> +		pr_err("%s(): %s: read clock names error\n",
> +		       __func__, np->name);
> +		goto err_clk_hw;
> +	}
> +
> +	if (!strcmp(np->name, "fm1-clk-mux"))
> +		fm_id = 1;
> +
> +	ret = get_fm_clk_idx(fm_id, &fm_clk_idx);
> +	if (ret)
> +		goto err_clk_hw;
> +
> +	init->ops = &fm_clk_ops;
> +	init->num_parents = count;
> +	/* Save clock source index */
> +	init->flags = fm_clk_idx;

Don't do this.
Scott Wood May 6, 2015, 7:34 a.m. UTC | #2
On Wed, 2015-05-06 at 00:02 -0700, Stephen Boyd wrote:
> On 04/16, Igal.Liberman wrote:
> > +static int get_fm_clk_idx(int fm_id, int *fm_clk_idx)
> > +{
> > +	struct ccsr_guts __iomem *guts_regs = NULL;
> 
> Unnecessary initialization to NULL. Also, marking a structure as
> __iomem is odd. Why do we need to use a struct to figure out
> offsets for registers? Why not just use #defines? That would
> probably also make it easy to avoid the asm include here.

Using a struct for registers is quite common:
scott@snotra:~/fsl/git/linux/upstream$ git grep struct|grep __iomem|wc -l
3005

It provides type-safety, and makes accessing the registers more natural.

> > +	struct device_node *guts;
> > +	uint32_t reg = 0;
> 
> s/uint32_t/u32/

Why?

> Also unnecessary initialization.

Given the if/else if/else if/... nature of how reg is initialized, this
seems like a useful and harmless way of making behavior predictable if
there is a bug.

-Scott
Stephen Boyd May 6, 2015, 10:25 p.m. UTC | #3
On 05/06, Scott Wood wrote:
> On Wed, 2015-05-06 at 00:02 -0700, Stephen Boyd wrote:
> > On 04/16, Igal.Liberman wrote:
> > > +static int get_fm_clk_idx(int fm_id, int *fm_clk_idx)
> > > +{
> > > +	struct ccsr_guts __iomem *guts_regs = NULL;
> > 
> > Unnecessary initialization to NULL. Also, marking a structure as
> > __iomem is odd. Why do we need to use a struct to figure out
> > offsets for registers? Why not just use #defines? That would
> > probably also make it easy to avoid the asm include here.
> 
> Using a struct for registers is quite common:
> scott@snotra:~/fsl/git/linux/upstream$ git grep struct|grep __iomem|wc -l
> 3005

$ git grep -E 'struct \w+ __iomem' | wc -l
2212

That's slightly inflated, but ok.

Within drivers/clk there aren't any though, hence my apprehension

$ git grep -E 'struct \w+ __iomem' -- drivers/clk/ | wc -l
0

> 
> It provides type-safety, and makes accessing the registers more natural.

Sure, we can leave the struct as is, but to make this compile on
ARM we need to figure something out. Move the struct definition
into include/linux/platform_data/ perhaps?

> 
> > > +	struct device_node *guts;
> > > +	uint32_t reg = 0;
> > 
> > s/uint32_t/u32/
> 
> Why?

This matches the rest of the file except for one instance of
uint32_t. I googled it and found [1], perhaps that will help.

> 
> > Also unnecessary initialization.
> 
> Given the if/else if/else if/... nature of how reg is initialized, this
> seems like a useful and harmless way of making behavior predictable if
> there is a bug.
> 

If there's a possibility of a bug due to missed initialization
perhaps it's a sign the code is too complicated and should be
broken down into smaller functions. For example, this function
could be rewritten to have a match table with function pointers
that return the fm_clk_idx.

[1] http://lkml.iu.edu/hypermail/linux/kernel/1101.3/02176.html
Scott Wood May 6, 2015, 11:01 p.m. UTC | #4
On Wed, 2015-05-06 at 15:25 -0700, Stephen Boyd wrote:
> On 05/06, Scott Wood wrote:
> > On Wed, 2015-05-06 at 00:02 -0700, Stephen Boyd wrote:
> > > On 04/16, Igal.Liberman wrote:
> > > > +static int get_fm_clk_idx(int fm_id, int *fm_clk_idx)
> > > > +{
> > > > +	struct ccsr_guts __iomem *guts_regs = NULL;
> > > 
> > > Unnecessary initialization to NULL. Also, marking a structure as
> > > __iomem is odd. Why do we need to use a struct to figure out
> > > offsets for registers? Why not just use #defines? That would
> > > probably also make it easy to avoid the asm include here.
> > 
> > Using a struct for registers is quite common:
> > scott@snotra:~/fsl/git/linux/upstream$ git grep struct|grep __iomem|wc -l
> > 3005
> 
> $ git grep -E 'struct \w+ __iomem' | wc -l
> 2212
> 
> That's slightly inflated, but ok.
> 
> Within drivers/clk there aren't any though, hence my apprehension
> 
> $ git grep -E 'struct \w+ __iomem' -- drivers/clk/ | wc -l
> 0

I'm not sure why clk should be special.  Plus, this is a struct that's
been used by other parts of the kernel since before git history began,
rather than something defined specifically for drivers/clk.

> > It provides type-safety, and makes accessing the registers more natural.
> 
> Sure, we can leave the struct as is, but to make this compile on
> ARM we need to figure something out. Move the struct definition
> into include/linux/platform_data/ perhaps?

It's register definition rather than platform data, but yes, it should
go somewhere in include/linux.  Or I suppose we could put #ifdef
CONFIG_PPC around the fman stuff.

> > > Also unnecessary initialization.
> > 
> > Given the if/else if/else if/... nature of how reg is initialized, this
> > seems like a useful and harmless way of making behavior predictable if
> > there is a bug.
> > 
> 
> If there's a possibility of a bug due to missed initialization
> perhaps it's a sign the code is too complicated and should be
> broken down into smaller functions.

Well, there's always a possibility. :-)

Though rereading this function, reg is only used in the locations where
it's set -- not after the if/else stuff -- so I no longer think this is
a particularly high risk situation.  Plus, GCC's gotten pretty
aggressive about warning about such possibilities.

>  For example, this function could be rewritten to have a match table
> with function pointers that return the fm_clk_idx.

Yes, that'd be nice.

-Scott
diff mbox

Patch

diff --git a/drivers/clk/clk-qoriq.c b/drivers/clk/clk-qoriq.c
index cda90a9..871c6df 100644
--- a/drivers/clk/clk-qoriq.c
+++ b/drivers/clk/clk-qoriq.c
@@ -19,6 +19,8 @@ 
 #include <linux/of.h>
 #include <linux/slab.h>
 
+#include <asm/fsl_guts.h>
+
 struct cmux_clk {
 	struct clk_hw hw;
 	void __iomem *reg;
@@ -155,6 +157,216 @@  err_name:
 	kfree(parent_names);
 }
 
+/* Table for matching compatible strings, for device tree
+ * guts node, for QorIQ SOCs.
+ * "fsl,qoriq-device-config-2.0" corresponds to T4 & B4
+ * SOCs. For the older SOCs "fsl,qoriq-device-config-1.0"
+ * string would be used.
+ */
+
+static const struct of_device_id guts_device_ids[] = {
+	{ .compatible = "fsl,qoriq-device-config-1.0", },
+	{ .compatible = "fsl,qoriq-device-config-2.0", },
+	{}
+};
+
+/* P2, P3, P4, P5 */
+#define FM1_CLK_SEL_SHIFT		30
+#define FM1_CLK_SEL			BIT(FM1_CLK_SEL_SHIFT)
+#define FM2_CLK_SEL_SHIFT		29
+#define FM2_CLK_SEL			BIT(FM2_CLK_SEL_SHIFT)
+#define HWA_ASYNC_DIV_SHIFT		26
+#define HWA_ASYNC_DIV			BIT(HWA_ASYNC_DIV_SHIFT)
+
+/* B4, T2 */
+#define HWA_CGA_M1_CLK_SEL_SHIFT	29
+#define HWA_CGA_M1_CLK_SEL		(BIT(HWA_CGA_M1_CLK_SEL_SHIFT + 2) |\
+					 BIT(HWA_CGA_M1_CLK_SEL_SHIFT + 1) |\
+					 BIT(HWA_CGA_M1_CLK_SEL_SHIFT))
+
+/* T4240 */
+#define HWA_CGB_M1_CLK_SEL_SHIFT	26
+#define HWA_CGB_M1_CLK_SEL		(BIT(HWA_CGB_M1_CLK_SEL_SHIFT + 2) |\
+					 BIT(HWA_CGB_M1_CLK_SEL_SHIFT + 1) |\
+					 BIT(HWA_CGB_M1_CLK_SEL_SHIFT))
+#define HWA_CGB_M2_CLK_SEL_SHIFT	3
+#define HWA_CGB_M2_CLK_SEL		(BIT(HWA_CGB_M2_CLK_SEL_SHIFT + 2) |\
+					 BIT(HWA_CGB_M2_CLK_SEL_SHIFT + 1) |\
+					 BIT(HWA_CGB_M2_CLK_SEL_SHIFT))
+
+static u8 get_fm_clk_parent(struct clk_hw *hw)
+{
+	return hw->init->flags;
+}
+
+static const struct clk_ops fm_clk_ops = {
+	.get_parent = get_fm_clk_parent,
+};
+
+static int get_fm_clk_idx(int fm_id, int *fm_clk_idx)
+{
+	struct ccsr_guts __iomem *guts_regs = NULL;
+	struct device_node *guts;
+	uint32_t reg = 0;
+	int clk_src = 0;
+
+	guts = of_find_matching_node(NULL, guts_device_ids);
+	if (!guts) {
+		pr_err("%s(): could not find GUTS node\n", __func__);
+		return -ENODEV;
+	}
+
+	guts_regs = of_iomap(guts, 0);
+	of_node_put(guts);
+	if (!guts_regs) {
+		pr_err("%s(): ioremap of GUTS node failed\n", __func__);
+		return -ENODEV;
+	}
+
+	if (of_device_is_compatible(guts, "fsl,p1023-guts") ||
+	    of_device_is_compatible(guts, "fsl,t1040-device-config")) {
+		/* P1023 and T1040 have only one optional clock source */
+		*fm_clk_idx = 0;
+	} else if (of_device_is_compatible(guts, "fsl,p2041-device-config") ||
+		   of_device_is_compatible(guts, "fsl,p3041-device-config") ||
+		   of_device_is_compatible(guts, "fsl,p4080-device-config")) {
+		/* Read RCW*/
+		reg = ioread32be(&guts_regs->rcwsr[7]);
+
+		if (fm_id == 0)
+			*fm_clk_idx = (reg & FM1_CLK_SEL) >>
+							FM1_CLK_SEL_SHIFT;
+		else
+			*fm_clk_idx = (reg & FM2_CLK_SEL) >>
+							FM2_CLK_SEL_SHIFT;
+	} else if (of_device_is_compatible(guts, "fsl,p5020-device-config") ||
+		   of_device_is_compatible(guts, "fsl,p5040-device-config")) {
+		/* Read RCW */
+		reg = ioread32be(&guts_regs->rcwsr[7]);
+
+		if (fm_id == 0)
+			clk_src = (reg & FM1_CLK_SEL) >> FM1_CLK_SEL_SHIFT;
+		else
+			clk_src = (reg & FM2_CLK_SEL) >> FM2_CLK_SEL_SHIFT;
+
+		if (clk_src == 0) {
+			*fm_clk_idx = 0;
+		} else {
+			clk_src = (reg & HWA_ASYNC_DIV) >> HWA_ASYNC_DIV_SHIFT;
+			*fm_clk_idx = clk_src + 1;
+		}
+	} else if (of_device_is_compatible(guts, "fsl,b4860-device-config") ||
+		   of_device_is_compatible(guts, "fsl,b4420-device-config") ||
+		   of_device_is_compatible(guts, "fsl,t2080-device-config")) {
+		/* Read RCW */
+		reg = ioread32be(&guts_regs->rcwsr[7]);
+
+		clk_src = (reg & HWA_CGA_M1_CLK_SEL) >>
+						HWA_CGA_M1_CLK_SEL_SHIFT;
+		*fm_clk_idx = clk_src - 1;
+	} else if (of_device_is_compatible(guts, "fsl,t4240-device-config")) {
+		if (fm_id == 0) {
+			reg = ioread32be(&guts_regs->rcwsr[7]);
+			clk_src = (reg & HWA_CGB_M1_CLK_SEL) >>
+						HWA_CGB_M1_CLK_SEL_SHIFT;
+		} else {
+			reg = ioread32be(&guts_regs->rcwsr[15]);
+			clk_src = (reg & HWA_CGB_M2_CLK_SEL) >>
+						HWA_CGB_M2_CLK_SEL_SHIFT;
+		}
+		*fm_clk_idx = clk_src - 2;
+	} else {
+		pr_err("%s(): Unsupported device! Can't determine FM clk source!\n",
+		       __func__);
+		iounmap(guts_regs);
+		return -ENODEV;
+	}
+
+	iounmap(guts_regs);
+
+	return 0;
+
+}
+
+static void __init fm_mux_init(struct device_node *np)
+{
+	struct clk_init_data *init;
+	struct clk_hw *hw;
+	int count, i, ret, fm_id = 0, fm_clk_idx;
+	struct clk *clk;
+
+	init = kmalloc((sizeof(struct clk_init_data)), GFP_KERNEL);
+	if (!init)
+		return;
+
+	/* get the input clock source count */
+	count = of_property_count_strings(np, "clock-names");
+	if (count < 0) {
+		pr_err("%s(): %s: get clock count error\n",
+		       __func__, np->name);
+		goto err_init;
+	}
+
+	init->parent_names = kmalloc((sizeof(char *) * count), GFP_KERNEL);
+	if (!init->parent_names)
+		goto err_init;
+
+	for (i = 0; i < count; i++)
+		init->parent_names[i] = of_clk_get_parent_name(np, i);
+
+	hw = kzalloc(sizeof(*hw), GFP_KERNEL);
+	if (!hw)
+		goto err_name;
+
+	ret = of_property_read_string_index(np, "clock-output-names", 0,
+					    &init->name);
+	if (ret) {
+		pr_err("%s(): %s: read clock names error\n",
+		       __func__, np->name);
+		goto err_clk_hw;
+	}
+
+	if (!strcmp(np->name, "fm1-clk-mux"))
+		fm_id = 1;
+
+	ret = get_fm_clk_idx(fm_id, &fm_clk_idx);
+	if (ret)
+		goto err_clk_hw;
+
+	init->ops = &fm_clk_ops;
+	init->num_parents = count;
+	/* Save clock source index */
+	init->flags = fm_clk_idx;
+	hw->init = init;
+
+	clk = clk_register(NULL, hw);
+	if (IS_ERR(clk)) {
+		pr_err("%s(): %s: could not register clock\n",
+		       __func__, init->name);
+		goto err_clk_hw;
+	}
+
+	ret = of_clk_add_provider(np, of_clk_src_simple_get, clk);
+	if (ret) {
+		pr_err("%s(): could not register clock provider for node: %s\n",
+		       __func__, np->name);
+		clk_unregister(clk);
+		goto err_clk_hw;
+	}
+
+	/* Free parent_names because they are reallocated when registered */
+	kfree(init->parent_names);
+
+	return;
+
+err_clk_hw:
+	kfree(hw);
+err_name:
+	kfree(init->parent_names);
+err_init:
+	kfree(init);
+}
+
 static void __init core_pll_init(struct device_node *np)
 {
 	u32 mult;
@@ -360,3 +572,4 @@  CLK_OF_DECLARE(qoriq_core_mux_1, "fsl,qoriq-core-mux-1.0", core_mux_init);
 CLK_OF_DECLARE(qoriq_core_mux_2, "fsl,qoriq-core-mux-2.0", core_mux_init);
 CLK_OF_DECLARE(qoriq_pltfrm_pll_1, "fsl,qoriq-platform-pll-1.0", pltfrm_pll_init);
 CLK_OF_DECLARE(qoriq_pltfrm_pll_2, "fsl,qoriq-platform-pll-2.0", pltfrm_pll_init);
+CLK_OF_DECLARE(qoriq_fm_mux, "fsl,fman-clk-mux", fm_mux_init);