| Message ID | 1416500463-20965-2-git-send-email-ulrich.hecht+renesas@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Commit | bc33e436745e3bdce9312981a97422720bb606d9 |
| Delegated to: | Simon Horman |
| Headers | show |
Hi Ulrich, Thank you for the patch. Most of the comments I've sent for "[PATCH v2 1/6] clk: shmobile: r8a73a4 common clock framework implementation" apply here too. Let's discuss them in the r8a73a4 mail thread. Please see below for additional comments. On Thursday 20 November 2014 17:20:58 Ulrich Hecht wrote: > Driver for the SH73A0's clocks that are too specific to be supported by a > generic driver. > > Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com> > Acked-by: Mike Turquette <mturquette@linaro.org> > Tested-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- > .../bindings/clock/renesas,sh73a0-cpg-clocks.txt | 35 ++++ > drivers/clk/shmobile/Makefile | 1 + > drivers/clk/shmobile/clk-sh73a0.c | 204 ++++++++++++++++++ > 3 files changed, 240 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/clock/renesas,sh73a0-cpg-clocks.txt > create mode 100644 drivers/clk/shmobile/clk-sh73a0.c > > diff --git > a/Documentation/devicetree/bindings/clock/renesas,sh73a0-cpg-clocks.txt > b/Documentation/devicetree/bindings/clock/renesas,sh73a0-cpg-clocks.txt new > file mode 100644 > index 0000000..a8978ec > --- /dev/null > +++ b/Documentation/devicetree/bindings/clock/renesas,sh73a0-cpg-clocks.txt > @@ -0,0 +1,35 @@ > +These bindings should be considered EXPERIMENTAL for now. > + > +* Renesas SH73A0 Clock Pulse Generator (CPG) > + > +The CPG generates core clocks for the SH73A0 SoC. It includes four PLLs > +and several fixed ratio dividers. > + > +Required Properties: > + > + - compatible: Must be "renesas,sh73a0-cpg-clocks" > + > + - reg: Base address and length of the memory resource used by the CPG > + > + - clocks: Reference to the parent clocks ("extal1" and "extal2") > + > + - #clock-cells: Must be 1 > + > + - clock-output-names: The names of the clocks. Supported clocks are > "main", + "pll0", "pll1", "pll2", "pll3", "dsi0phy", "dsi1phy", "zg", > "m3", "b", + "m1", "m2", "z", "zx", and "hp". > + > + > +Example > +------- > + > + cpg_clocks: cpg_clocks@e6150000 { > + compatible = "renesas,sh73a0-cpg-clocks"; > + reg = <0 0xe6150000 0 0x10000>; > + clocks = <&extal1_clk>, <&extal2_clk>; > + #clock-cells = <1>; > + clock-output-names = "main", "pll0", "pll1", "pll2", > + "pll3", "dsi0phy", "dsi1phy", > + "zg", "m3", "b", "m1", "m2", > + "z", "zx", "hp"; > + }; > diff --git a/drivers/clk/shmobile/Makefile b/drivers/clk/shmobile/Makefile > index 960bf22..f83980f 100644 > --- a/drivers/clk/shmobile/Makefile > +++ b/drivers/clk/shmobile/Makefile > @@ -5,5 +5,6 @@ obj-$(CONFIG_ARCH_R8A7779) += clk-r8a7779.o > obj-$(CONFIG_ARCH_R8A7790) += clk-rcar-gen2.o > obj-$(CONFIG_ARCH_R8A7791) += clk-rcar-gen2.o > obj-$(CONFIG_ARCH_R8A7794) += clk-rcar-gen2.o > +obj-$(CONFIG_ARCH_SH73A0) += clk-sh73a0.o > obj-$(CONFIG_ARCH_SHMOBILE_MULTI) += clk-div6.o > obj-$(CONFIG_ARCH_SHMOBILE_MULTI) += clk-mstp.o > diff --git a/drivers/clk/shmobile/clk-sh73a0.c > b/drivers/clk/shmobile/clk-sh73a0.c new file mode 100644 > index 0000000..ebd51d1 > --- /dev/null > +++ b/drivers/clk/shmobile/clk-sh73a0.c > @@ -0,0 +1,204 @@ > +/* > + * sh73a0 Core CPG Clocks > + * > + * Copyright (C) 2014 Ulrich Hecht > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; version 2 of the License. > + */ > + > +#include <linux/clk-provider.h> > +#include <linux/clkdev.h> > +#include <linux/clk/shmobile.h> > +#include <linux/init.h> > +#include <linux/kernel.h> > +#include <linux/of.h> > +#include <linux/of_address.h> > +#include <linux/spinlock.h> > + > +struct sh73a0_cpg { > + struct clk_onecell_data data; > + spinlock_t lock; > + void __iomem *reg; > +}; > + > +#define CPG_FRQCRA 0x00 > +#define CPG_FRQCRB 0x04 > +#define CPG_SD0CKCR 0x74 > +#define CPG_SD1CKCR 0x78 > +#define CPG_SD2CKCR 0x7c > +#define CPG_PLLECR 0xd0 > +#define CPG_PLL0CR 0xd8 > +#define CPG_PLL1CR 0x28 > +#define CPG_PLL2CR 0x2c > +#define CPG_PLL3CR 0xdc > +#define CPG_CKSCR 0xc0 > +#define CPG_DSI0PHYCR 0x6c > +#define CPG_DSI1PHYCR 0x70 > + > +#define CLK_ENABLE_ON_INIT BIT(0) > + > +struct div4_clk { > + const char *name; > + const char *parent; > + unsigned int reg; > + unsigned int shift; > + unsigned int mask; > + int flags; > +}; > + > +static struct div4_clk div4_clks[] = { > + { "zg", "pll0", CPG_FRQCRA, 16, 0x0d7f, CLK_ENABLE_ON_INIT }, According to the datasheet the ZGFC value 1010 results in a x1/5 factor, which doesn't match the value in the div4_div_table below. I wonder if it could be an error in the datasheet though. > + { "m3", "pll1", CPG_FRQCRA, 12, 0x1dff, CLK_ENABLE_ON_INIT }, > + { "b", "pll1", CPG_FRQCRA, 8, 0x0dff, CLK_ENABLE_ON_INIT }, > + { "m1", "pll1", CPG_FRQCRA, 4, 0x1dff, 0 }, > + { "m2", "pll1", CPG_FRQCRA, 0, 0x1dff, 0 }, > + { "z", "pll0", CPG_FRQCRB, 24, 0x097f, 0 }, > + { "zx", "pll1", CPG_FRQCRB, 12, 0x0dff, 0 }, > + { "hp", "pll1", CPG_FRQCRB, 4, 0x0dff, 0 }, > + { NULL, 0, 0, 0, 0 }, > +}; > + > +static const struct clk_div_table div4_div_table[] = { > + { 0, 2 }, { 1, 3 }, { 2, 4 }, { 3, 6 }, { 4, 8 }, { 5, 12 }, > + { 6, 16 }, { 7, 18 }, { 8, 24 }, { 10, 36 }, { 11, 48 }, > + { 12, 7 }, { 0, 0 } > +}; > + > +static struct clk * __init > +sh73a0_cpg_register_clock(struct device_node *np, struct sh73a0_cpg *cpg, > + const char *name) > +{ > + const struct clk_div_table *table = NULL; > + const char *parent_name; > + unsigned int shift, reg; > + unsigned int mult = 1; > + unsigned int div = 1; > + > + if (!strcmp(name, "main")) { > + /* extal1, extal1_div2, extal2, extal2_div2 */ > + u32 parent_idx = (clk_readl(cpg->reg + CPG_CKSCR) >> 28) & 3; > + > + parent_name = of_clk_get_parent_name(np, parent_idx >> 1); > + div = (parent_idx & 1) + 1; > + } else if (!strncmp(name, "pll", 3)) { > + void __iomem *enable_reg = cpg->reg; > + u32 enable_bit = name[3] - '0'; > + > + parent_name = "main"; > + switch (enable_bit) { > + case 0: > + enable_reg += CPG_PLL0CR; > + break; > + case 1: > + enable_reg += CPG_PLL1CR; > + break; > + case 2: > + enable_reg += CPG_PLL2CR; > + break; > + case 3: > + enable_reg += CPG_PLL3CR; > + break; > + default: > + return ERR_PTR(-EINVAL); > + } > + if (clk_readl(cpg->reg + CPG_PLLECR) & BIT(enable_bit)) { > + mult = ((clk_readl(enable_reg) >> 24) & 0x3f) + 1; > + /* handle CFG bit for PLL1 and PLL2 */ > + if (enable_bit == 1 || enable_bit == 2) > + if (clk_readl(enable_reg) & BIT(20)) Nitpicking, I would have cached the enable_reg value. > + mult *= 2; > + } > + } else if (!strcmp(name, "dsi0phy") || !strcmp(name, "dsi1phy")) { > + u32 phy_no = name[3] - '0'; > + void __iomem *dsi_reg = cpg->reg + > + (phy_no ? CPG_DSI1PHYCR : CPG_DSI0PHYCR); > + > + parent_name = phy_no ? "dsi1pck" : "dsi0pck"; > + div = __raw_readl(dsi_reg); > + if (!(div & 0xb8000)) If I'm not mistaken the PLL is disabled if the MANUAL bit is set and the BYPSL is clear. The above check would consider the PLL to be enabled. > + div = 1; > + else > + div = (div & 0x3f) + 1; > + } else { > + struct div4_clk *c; > + > + for (c = div4_clks; c->name; c++) { > + if (!strcmp(name, c->name)) { > + parent_name = c->parent; > + table = div4_div_table; > + reg = c->reg; > + shift = c->shift; > + break; > + } > + } > + if (!c->name) > + return ERR_PTR(-EINVAL); > + } > + > + if (!table) { > + return clk_register_fixed_factor(NULL, name, parent_name, 0, > + mult, div); > + } else { > + return clk_register_divider_table(NULL, name, parent_name, 0, > + cpg->reg + reg, shift, 4, 0, > + table, &cpg->lock); > + } > +} > + > +static void __init sh73a0_cpg_clocks_init(struct device_node *np) > +{ > + struct sh73a0_cpg *cpg; > + struct clk **clks; > + unsigned int i; > + int num_clks; > + > + num_clks = of_property_count_strings(np, "clock-output-names"); > + if (num_clks < 0) { > + pr_err("%s: failed to count clocks\n", __func__); > + return; > + } > + > + cpg = kzalloc(sizeof(*cpg), GFP_KERNEL); > + clks = kcalloc(num_clks, sizeof(*clks), GFP_KERNEL); > + if (cpg == NULL || clks == NULL) { > + /* We're leaking memory on purpose, there's no point in cleaning > + * up as the system won't boot anyway. > + */ > + return; > + } > + > + spin_lock_init(&cpg->lock); > + > + cpg->data.clks = clks; > + cpg->data.clk_num = num_clks; > + > + cpg->reg = of_iomap(np, 0); > + if (WARN_ON(cpg->reg == NULL)) > + return; > + > + /* Set SDHI clocks to a known state */ > + clk_writel(0x108, cpg->reg + CPG_SD0CKCR); > + clk_writel(0x108, cpg->reg + CPG_SD1CKCR); > + clk_writel(0x108, cpg->reg + CPG_SD2CKCR); Why is this needed ? > + for (i = 0; i < num_clks; ++i) { > + const char *name; > + struct clk *clk; > + > + of_property_read_string_index(np, "clock-output-names", i, > + &name); > + > + clk = sh73a0_cpg_register_clock(np, cpg, name); > + if (IS_ERR(clk)) > + pr_err("%s: failed to register %s %s clock (%ld)\n", > + __func__, np->name, name, PTR_ERR(clk)); > + else > + cpg->data.clks[i] = clk; > + } > + > + of_clk_add_provider(np, of_clk_src_onecell_get, &cpg->data); > +} > +CLK_OF_DECLARE(sh73a0_cpg_clks, "renesas,sh73a0-cpg-clocks", > + sh73a0_cpg_clocks_init);
On Tue, Nov 25, 2014 at 1:04 PM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > On Thursday 20 November 2014 17:20:58 Ulrich Hecht wrote: [...] >> + mult *= 2; >> + } >> + } else if (!strcmp(name, "dsi0phy") || !strcmp(name, "dsi1phy")) { >> + u32 phy_no = name[3] - '0'; >> + void __iomem *dsi_reg = cpg->reg + >> + (phy_no ? CPG_DSI1PHYCR : CPG_DSI0PHYCR); >> + >> + parent_name = phy_no ? "dsi1pck" : "dsi0pck"; >> + div = __raw_readl(dsi_reg); >> + if (!(div & 0xb8000)) > > If I'm not mistaken the PLL is disabled if the MANUAL bit is set and the BYPSL > is clear. The above check would consider the PLL to be enabled. That is a translation of the corresponding part of the legacy driver, where it is appropriately labeled "/* FIXME */"... I think it is sufficient to examine PLLE here. We don't really care why the PLL is on or off at this point. More embarrassing is the fact that that should be "mult" up there... (This error also exists in the legacy driver.) CU Uli -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/clock/renesas,sh73a0-cpg-clocks.txt b/Documentation/devicetree/bindings/clock/renesas,sh73a0-cpg-clocks.txt new file mode 100644 index 0000000..a8978ec --- /dev/null +++ b/Documentation/devicetree/bindings/clock/renesas,sh73a0-cpg-clocks.txt @@ -0,0 +1,35 @@ +These bindings should be considered EXPERIMENTAL for now. + +* Renesas SH73A0 Clock Pulse Generator (CPG) + +The CPG generates core clocks for the SH73A0 SoC. It includes four PLLs +and several fixed ratio dividers. + +Required Properties: + + - compatible: Must be "renesas,sh73a0-cpg-clocks" + + - reg: Base address and length of the memory resource used by the CPG + + - clocks: Reference to the parent clocks ("extal1" and "extal2") + + - #clock-cells: Must be 1 + + - clock-output-names: The names of the clocks. Supported clocks are "main", + "pll0", "pll1", "pll2", "pll3", "dsi0phy", "dsi1phy", "zg", "m3", "b", + "m1", "m2", "z", "zx", and "hp". + + +Example +------- + + cpg_clocks: cpg_clocks@e6150000 { + compatible = "renesas,sh73a0-cpg-clocks"; + reg = <0 0xe6150000 0 0x10000>; + clocks = <&extal1_clk>, <&extal2_clk>; + #clock-cells = <1>; + clock-output-names = "main", "pll0", "pll1", "pll2", + "pll3", "dsi0phy", "dsi1phy", + "zg", "m3", "b", "m1", "m2", + "z", "zx", "hp"; + }; diff --git a/drivers/clk/shmobile/Makefile b/drivers/clk/shmobile/Makefile index 960bf22..f83980f 100644 --- a/drivers/clk/shmobile/Makefile +++ b/drivers/clk/shmobile/Makefile @@ -5,5 +5,6 @@ obj-$(CONFIG_ARCH_R8A7779) += clk-r8a7779.o obj-$(CONFIG_ARCH_R8A7790) += clk-rcar-gen2.o obj-$(CONFIG_ARCH_R8A7791) += clk-rcar-gen2.o obj-$(CONFIG_ARCH_R8A7794) += clk-rcar-gen2.o +obj-$(CONFIG_ARCH_SH73A0) += clk-sh73a0.o obj-$(CONFIG_ARCH_SHMOBILE_MULTI) += clk-div6.o obj-$(CONFIG_ARCH_SHMOBILE_MULTI) += clk-mstp.o diff --git a/drivers/clk/shmobile/clk-sh73a0.c b/drivers/clk/shmobile/clk-sh73a0.c new file mode 100644 index 0000000..ebd51d1 --- /dev/null +++ b/drivers/clk/shmobile/clk-sh73a0.c @@ -0,0 +1,204 @@ +/* + * sh73a0 Core CPG Clocks + * + * Copyright (C) 2014 Ulrich Hecht + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + */ + +#include <linux/clk-provider.h> +#include <linux/clkdev.h> +#include <linux/clk/shmobile.h> +#include <linux/init.h> +#include <linux/kernel.h> +#include <linux/of.h> +#include <linux/of_address.h> +#include <linux/spinlock.h> + +struct sh73a0_cpg { + struct clk_onecell_data data; + spinlock_t lock; + void __iomem *reg; +}; + +#define CPG_FRQCRA 0x00 +#define CPG_FRQCRB 0x04 +#define CPG_SD0CKCR 0x74 +#define CPG_SD1CKCR 0x78 +#define CPG_SD2CKCR 0x7c +#define CPG_PLLECR 0xd0 +#define CPG_PLL0CR 0xd8 +#define CPG_PLL1CR 0x28 +#define CPG_PLL2CR 0x2c +#define CPG_PLL3CR 0xdc +#define CPG_CKSCR 0xc0 +#define CPG_DSI0PHYCR 0x6c +#define CPG_DSI1PHYCR 0x70 + +#define CLK_ENABLE_ON_INIT BIT(0) + +struct div4_clk { + const char *name; + const char *parent; + unsigned int reg; + unsigned int shift; + unsigned int mask; + int flags; +}; + +static struct div4_clk div4_clks[] = { + { "zg", "pll0", CPG_FRQCRA, 16, 0x0d7f, CLK_ENABLE_ON_INIT }, + { "m3", "pll1", CPG_FRQCRA, 12, 0x1dff, CLK_ENABLE_ON_INIT }, + { "b", "pll1", CPG_FRQCRA, 8, 0x0dff, CLK_ENABLE_ON_INIT }, + { "m1", "pll1", CPG_FRQCRA, 4, 0x1dff, 0 }, + { "m2", "pll1", CPG_FRQCRA, 0, 0x1dff, 0 }, + { "z", "pll0", CPG_FRQCRB, 24, 0x097f, 0 }, + { "zx", "pll1", CPG_FRQCRB, 12, 0x0dff, 0 }, + { "hp", "pll1", CPG_FRQCRB, 4, 0x0dff, 0 }, + { NULL, 0, 0, 0, 0 }, +}; + +static const struct clk_div_table div4_div_table[] = { + { 0, 2 }, { 1, 3 }, { 2, 4 }, { 3, 6 }, { 4, 8 }, { 5, 12 }, + { 6, 16 }, { 7, 18 }, { 8, 24 }, { 10, 36 }, { 11, 48 }, + { 12, 7 }, { 0, 0 } +}; + +static struct clk * __init +sh73a0_cpg_register_clock(struct device_node *np, struct sh73a0_cpg *cpg, + const char *name) +{ + const struct clk_div_table *table = NULL; + const char *parent_name; + unsigned int shift, reg; + unsigned int mult = 1; + unsigned int div = 1; + + if (!strcmp(name, "main")) { + /* extal1, extal1_div2, extal2, extal2_div2 */ + u32 parent_idx = (clk_readl(cpg->reg + CPG_CKSCR) >> 28) & 3; + + parent_name = of_clk_get_parent_name(np, parent_idx >> 1); + div = (parent_idx & 1) + 1; + } else if (!strncmp(name, "pll", 3)) { + void __iomem *enable_reg = cpg->reg; + u32 enable_bit = name[3] - '0'; + + parent_name = "main"; + switch (enable_bit) { + case 0: + enable_reg += CPG_PLL0CR; + break; + case 1: + enable_reg += CPG_PLL1CR; + break; + case 2: + enable_reg += CPG_PLL2CR; + break; + case 3: + enable_reg += CPG_PLL3CR; + break; + default: + return ERR_PTR(-EINVAL); + } + if (clk_readl(cpg->reg + CPG_PLLECR) & BIT(enable_bit)) { + mult = ((clk_readl(enable_reg) >> 24) & 0x3f) + 1; + /* handle CFG bit for PLL1 and PLL2 */ + if (enable_bit == 1 || enable_bit == 2) + if (clk_readl(enable_reg) & BIT(20)) + mult *= 2; + } + } else if (!strcmp(name, "dsi0phy") || !strcmp(name, "dsi1phy")) { + u32 phy_no = name[3] - '0'; + void __iomem *dsi_reg = cpg->reg + + (phy_no ? CPG_DSI1PHYCR : CPG_DSI0PHYCR); + + parent_name = phy_no ? "dsi1pck" : "dsi0pck"; + div = __raw_readl(dsi_reg); + if (!(div & 0xb8000)) + div = 1; + else + div = (div & 0x3f) + 1; + } else { + struct div4_clk *c; + + for (c = div4_clks; c->name; c++) { + if (!strcmp(name, c->name)) { + parent_name = c->parent; + table = div4_div_table; + reg = c->reg; + shift = c->shift; + break; + } + } + if (!c->name) + return ERR_PTR(-EINVAL); + } + + if (!table) { + return clk_register_fixed_factor(NULL, name, parent_name, 0, + mult, div); + } else { + return clk_register_divider_table(NULL, name, parent_name, 0, + cpg->reg + reg, shift, 4, 0, + table, &cpg->lock); + } +} + +static void __init sh73a0_cpg_clocks_init(struct device_node *np) +{ + struct sh73a0_cpg *cpg; + struct clk **clks; + unsigned int i; + int num_clks; + + num_clks = of_property_count_strings(np, "clock-output-names"); + if (num_clks < 0) { + pr_err("%s: failed to count clocks\n", __func__); + return; + } + + cpg = kzalloc(sizeof(*cpg), GFP_KERNEL); + clks = kcalloc(num_clks, sizeof(*clks), GFP_KERNEL); + if (cpg == NULL || clks == NULL) { + /* We're leaking memory on purpose, there's no point in cleaning + * up as the system won't boot anyway. + */ + return; + } + + spin_lock_init(&cpg->lock); + + cpg->data.clks = clks; + cpg->data.clk_num = num_clks; + + cpg->reg = of_iomap(np, 0); + if (WARN_ON(cpg->reg == NULL)) + return; + + /* Set SDHI clocks to a known state */ + clk_writel(0x108, cpg->reg + CPG_SD0CKCR); + clk_writel(0x108, cpg->reg + CPG_SD1CKCR); + clk_writel(0x108, cpg->reg + CPG_SD2CKCR); + + for (i = 0; i < num_clks; ++i) { + const char *name; + struct clk *clk; + + of_property_read_string_index(np, "clock-output-names", i, + &name); + + clk = sh73a0_cpg_register_clock(np, cpg, name); + if (IS_ERR(clk)) + pr_err("%s: failed to register %s %s clock (%ld)\n", + __func__, np->name, name, PTR_ERR(clk)); + else + cpg->data.clks[i] = clk; + } + + of_clk_add_provider(np, of_clk_src_onecell_get, &cpg->data); +} +CLK_OF_DECLARE(sh73a0_cpg_clks, "renesas,sh73a0-cpg-clocks", + sh73a0_cpg_clocks_init);