| Message ID | 1414767696-23211-2-git-send-email-ulrich.hecht+renesas@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
Hi Ulrich, On Fri, Oct 31, 2014 at 4:01 PM, Ulrich Hecht <ulrich.hecht+renesas@gmail.com> wrote: > Support for setting the parent at initialization time based on the current > hardware configuration in DIV6 clocks with selectable parents as found in > the r8a73a4, r8a7740, sh73a0, and other SoCs. Thanks for your patch! > --- a/drivers/clk/shmobile/clk-div6.c > +++ b/drivers/clk/shmobile/clk-div6.c > @@ -32,6 +32,9 @@ struct div6_clock { > struct clk_hw hw; > void __iomem *reg; > unsigned int div; > + int src_shift; > + int src_width; unsigned int or u32 (both) > + u8 *parents; > }; > > #define to_div6_clock(_hw) container_of(_hw, struct div6_clock, hw) > +static unsigned char cpg_div6_clock_get_parent(struct clk_hw *hw) Please return u8, cfr. clk_ops.get_parent() > +{ > + struct div6_clock *clock = to_div6_clock(hw); > + u8 hw_index; > + int i; unsigned int > + if (clock->src_width == 0) > + return 0; > + > + hw_index = (clk_readl(clock->reg) >> clock->src_shift) & > + (BIT(clock->src_width) - 1); > + for (i = 0; i < hw->init->num_parents; i++) { Is hw->init still valid here? I believe it's a pointer to local stack variable "struct clk_init_data init" in cpg_div6_clock_init(), which was never copied? Use __clk_get_num_parents(hw->clk) instead, cfr. clk-mux? > + if (clock->parents[i] == hw_index) > + return i; > + } > + > + pr_err("%s: %s DIV6 clock set to invalid parent %d\n", %u for u8 > + __func__, hw->init->name, hw_index); > + return 0; > +} It's a bit unfortunate that clk_ops.get_parent() cannot return an error code, as u8 is positive. Perhaps it should return int instead? However, as clk_mux_get_parent() does return -EINVAL if the clock can't be found, perhaps it's best to mimic that behavior? __clk_init_parent() calls clk_get_parent_by_index(), which does check if the index fits within the allowed range. __clk_init() uses the returned value to index parent_names[] without any checking. Oops... Mike? > +static int cpg_div6_clock_set_parent(struct clk_hw *hw, u8 index) > +{ > + struct div6_clock *clock = to_div6_clock(hw); > + u8 hw_index; > + u32 mask; > + > + if (index >= hw->init->num_parents) Is hw->init still valid here? Use __clk_get_num_parents(hw->clk) instead, cfr. clk-mux? > + return -EINVAL; > + > + mask = ~((BIT(clock->src_width) - 1) << clock->src_shift); > + hw_index = clock->parents[index]; > + > + clk_writel((clk_readl(clock->reg) & mask) | > + (hw_index << clock->src_shift), clock->reg); > > return 0; > } > @@ -115,12 +164,17 @@ static const struct clk_ops cpg_div6_clock_ops = { > > static void __init cpg_div6_clock_init(struct device_node *np) > { > + const char **parent_names; > struct clk_init_data init; > struct div6_clock *clock; > - const char *parent_name; > const char *name; > struct clk *clk; > + int valid_parents; > + int num_parents; unsigned int (both) > + u32 src_shift; > + u32 src_width; > int ret; > + int i; unsigned int > @@ -129,6 +183,24 @@ static void __init cpg_div6_clock_init(struct device_node *np) > return; > } > > + num_parents = of_clk_get_parent_count(np); > + if (num_parents < 1) { > + pr_err("%s: no parent found for %s DIV6 clock\n", > + __func__, np->name); > + return; > + } > + > + clock->parents = kmalloc_array(num_parents, sizeof(*clock->parents), > + GFP_KERNEL); > + parent_names = kmalloc_array(num_parents, sizeof(*parent_names), > + GFP_KERNEL); > + Please drop the blank line above. > + if (!parent_names) { > + pr_err("%s: failed to allocate %s parent name list\n", > + __func__, np->name); There's no need to print an error in case of allocation failures. > + return; > + } > + > /* Remap the clock register and read the divisor. Disabling the > * clock overwrites the divisor, so we need to cache its value for the > * enable operation. > @@ -150,19 +222,38 @@ static void __init cpg_div6_clock_init(struct device_node *np) > goto error; > } > > - parent_name = of_clk_get_parent_name(np, 0); > - if (parent_name == NULL) { > - pr_err("%s: failed to get %s DIV6 clock parent name\n", > - __func__, np->name); > - goto error; > + > + for (i = 0, valid_parents = 0; i < num_parents; i++) { > + const char *name = of_clk_get_parent_name(np, i); > + > + if (name) { > + parent_names[valid_parents] = name; > + clock->parents[valid_parents] = i; > + valid_parents++; > + } > + } > + > + if (!of_property_read_u32(np, "renesas,src-shift", &src_shift)) { > + if (!of_property_read_u32(np, "renesas,src-width", > + &src_width)) { > + clock->src_shift = src_shift; > + clock->src_width = src_width; > + } else { > + pr_err("%s: renesas,src-shift without renesas,src-width in %s\n", > + __func__, np->name); > + goto error; > + } > + } else { > + clock->src_shift = clock->src_width = 0; > } The above construct does allow renesas,src-width without renesas,src-shift. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- 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
Hi Ulrich, Thank you for the patch. Mark Rutland has asked a couple of interesting questions in his review of v4, which I don't think you have answered. I've proposed alternative bindings that wouldn't require the shift and width attributes. Could you please comment on that ? http://www.spinics.net/lists/linux-sh/msg34936.html On Friday 31 October 2014 16:01:35 Ulrich Hecht wrote: > Support for setting the parent at initialization time based on the current > hardware configuration in DIV6 clocks with selectable parents as found in > the r8a73a4, r8a7740, sh73a0, and other SoCs. > > Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com> > --- > drivers/clk/shmobile/clk-div6.c | 117 ++++++++++++++++++++++++++++++++----- > 1 file changed, 105 insertions(+), 12 deletions(-) > > diff --git a/drivers/clk/shmobile/clk-div6.c > b/drivers/clk/shmobile/clk-div6.c index f065f69..ea44988 100644 > --- a/drivers/clk/shmobile/clk-div6.c > +++ b/drivers/clk/shmobile/clk-div6.c > @@ -32,6 +32,9 @@ struct div6_clock { > struct clk_hw hw; > void __iomem *reg; > unsigned int div; > + int src_shift; > + int src_width; > + u8 *parents; > }; > > #define to_div6_clock(_hw) container_of(_hw, struct div6_clock, hw) > @@ -39,8 +42,11 @@ struct div6_clock { > static int cpg_div6_clock_enable(struct clk_hw *hw) > { > struct div6_clock *clock = to_div6_clock(hw); > + u32 val; > > - clk_writel(CPG_DIV6_DIV(clock->div - 1), clock->reg); > + val = (clk_readl(clock->reg) & ~(CPG_DIV6_DIV_MASK | CPG_DIV6_CKSTP)) > + | CPG_DIV6_DIV(clock->div - 1); > + clk_writel(val, clock->reg); > > return 0; > } > @@ -52,7 +58,7 @@ static void cpg_div6_clock_disable(struct clk_hw *hw) > /* DIV6 clocks require the divisor field to be non-zero when stopping > * the clock. > */ > - clk_writel(CPG_DIV6_CKSTP | CPG_DIV6_DIV(CPG_DIV6_DIV_MASK), > + clk_writel(clk_readl(clock->reg) | CPG_DIV6_CKSTP | CPG_DIV6_DIV_MASK, > clock->reg); > } > > @@ -94,12 +100,53 @@ static int cpg_div6_clock_set_rate(struct clk_hw *hw, > unsigned long rate, { > struct div6_clock *clock = to_div6_clock(hw); > unsigned int div = cpg_div6_clock_calc_div(rate, parent_rate); > + u32 val; > > clock->div = div; > > + val = clk_readl(clock->reg) & ~CPG_DIV6_DIV_MASK; > /* Only program the new divisor if the clock isn't stopped. */ > - if (!(clk_readl(clock->reg) & CPG_DIV6_CKSTP)) > - clk_writel(CPG_DIV6_DIV(clock->div - 1), clock->reg); > + if (!(val & CPG_DIV6_CKSTP)) > + clk_writel(val | CPG_DIV6_DIV(clock->div - 1), clock->reg); > + > + return 0; > +} > + > +static unsigned char cpg_div6_clock_get_parent(struct clk_hw *hw) > +{ > + struct div6_clock *clock = to_div6_clock(hw); > + u8 hw_index; > + int i; > + > + if (clock->src_width == 0) > + return 0; > + > + hw_index = (clk_readl(clock->reg) >> clock->src_shift) & > + (BIT(clock->src_width) - 1); > + for (i = 0; i < hw->init->num_parents; i++) { > + if (clock->parents[i] == hw_index) > + return i; > + } > + > + pr_err("%s: %s DIV6 clock set to invalid parent %d\n", > + __func__, hw->init->name, hw_index); > + return 0; > +} > + > +static int cpg_div6_clock_set_parent(struct clk_hw *hw, u8 index) > +{ > + struct div6_clock *clock = to_div6_clock(hw); > + u8 hw_index; > + u32 mask; > + > + if (index >= hw->init->num_parents) > + return -EINVAL; > + > + mask = ~((BIT(clock->src_width) - 1) << clock->src_shift); > + hw_index = clock->parents[index]; > + > + clk_writel((clk_readl(clock->reg) & mask) | > + (hw_index << clock->src_shift), clock->reg); > > return 0; > } > @@ -108,6 +155,8 @@ static const struct clk_ops cpg_div6_clock_ops = { > .enable = cpg_div6_clock_enable, > .disable = cpg_div6_clock_disable, > .is_enabled = cpg_div6_clock_is_enabled, > + .get_parent = cpg_div6_clock_get_parent, > + .set_parent = cpg_div6_clock_set_parent, > .recalc_rate = cpg_div6_clock_recalc_rate, > .round_rate = cpg_div6_clock_round_rate, > .set_rate = cpg_div6_clock_set_rate, > @@ -115,12 +164,17 @@ static const struct clk_ops cpg_div6_clock_ops = { > > static void __init cpg_div6_clock_init(struct device_node *np) > { > + const char **parent_names; > struct clk_init_data init; > struct div6_clock *clock; > - const char *parent_name; > const char *name; > struct clk *clk; > + int valid_parents; > + int num_parents; > + u32 src_shift; > + u32 src_width; > int ret; > + int i; > > clock = kzalloc(sizeof(*clock), GFP_KERNEL); > if (!clock) { > @@ -129,6 +183,24 @@ static void __init cpg_div6_clock_init(struct > device_node *np) return; > } > > + num_parents = of_clk_get_parent_count(np); > + if (num_parents < 1) { > + pr_err("%s: no parent found for %s DIV6 clock\n", > + __func__, np->name); > + return; > + } > + > + clock->parents = kmalloc_array(num_parents, sizeof(*clock->parents), > + GFP_KERNEL); > + parent_names = kmalloc_array(num_parents, sizeof(*parent_names), > + GFP_KERNEL); > + > + if (!parent_names) { > + pr_err("%s: failed to allocate %s parent name list\n", > + __func__, np->name); > + return; > + } > + > /* Remap the clock register and read the divisor. Disabling the > * clock overwrites the divisor, so we need to cache its value for the > * enable operation. > @@ -150,19 +222,38 @@ static void __init cpg_div6_clock_init(struct > device_node *np) goto error; > } > > - parent_name = of_clk_get_parent_name(np, 0); > - if (parent_name == NULL) { > - pr_err("%s: failed to get %s DIV6 clock parent name\n", > - __func__, np->name); > - goto error; > + > + for (i = 0, valid_parents = 0; i < num_parents; i++) { > + const char *name = of_clk_get_parent_name(np, i); > + > + if (name) { > + parent_names[valid_parents] = name; > + clock->parents[valid_parents] = i; > + valid_parents++; > + } > + } > + > + if (!of_property_read_u32(np, "renesas,src-shift", &src_shift)) { > + if (!of_property_read_u32(np, "renesas,src-width", > + &src_width)) { > + clock->src_shift = src_shift; > + clock->src_width = src_width; > + } else { > + pr_err("%s: renesas,src-shift without renesas,src-width in %s\n", > + __func__, np->name); > + goto error; > + } > + } else { > + clock->src_shift = clock->src_width = 0; > } > > + > /* Register the clock. */ > init.name = name; > init.ops = &cpg_div6_clock_ops; > init.flags = CLK_IS_BASIC; > - init.parent_names = &parent_name; > - init.num_parents = 1; > + init.parent_names = parent_names; > + init.num_parents = valid_parents; > > clock->hw.init = &init; > > @@ -175,11 +266,13 @@ static void __init cpg_div6_clock_init(struct > device_node *np) > > of_clk_add_provider(np, of_clk_src_simple_get, clk); > > + kfree(parent_names); > return; > > error: > if (clock->reg) > iounmap(clock->reg); > + kfree(parent_names); > kfree(clock); > } > CLK_OF_DECLARE(cpg_div6_clk, "renesas,cpg-div6-clock", > cpg_div6_clock_init);
diff --git a/drivers/clk/shmobile/clk-div6.c b/drivers/clk/shmobile/clk-div6.c index f065f69..ea44988 100644 --- a/drivers/clk/shmobile/clk-div6.c +++ b/drivers/clk/shmobile/clk-div6.c @@ -32,6 +32,9 @@ struct div6_clock { struct clk_hw hw; void __iomem *reg; unsigned int div; + int src_shift; + int src_width; + u8 *parents; }; #define to_div6_clock(_hw) container_of(_hw, struct div6_clock, hw) @@ -39,8 +42,11 @@ struct div6_clock { static int cpg_div6_clock_enable(struct clk_hw *hw) { struct div6_clock *clock = to_div6_clock(hw); + u32 val; - clk_writel(CPG_DIV6_DIV(clock->div - 1), clock->reg); + val = (clk_readl(clock->reg) & ~(CPG_DIV6_DIV_MASK | CPG_DIV6_CKSTP)) + | CPG_DIV6_DIV(clock->div - 1); + clk_writel(val, clock->reg); return 0; } @@ -52,7 +58,7 @@ static void cpg_div6_clock_disable(struct clk_hw *hw) /* DIV6 clocks require the divisor field to be non-zero when stopping * the clock. */ - clk_writel(CPG_DIV6_CKSTP | CPG_DIV6_DIV(CPG_DIV6_DIV_MASK), + clk_writel(clk_readl(clock->reg) | CPG_DIV6_CKSTP | CPG_DIV6_DIV_MASK, clock->reg); } @@ -94,12 +100,53 @@ static int cpg_div6_clock_set_rate(struct clk_hw *hw, unsigned long rate, { struct div6_clock *clock = to_div6_clock(hw); unsigned int div = cpg_div6_clock_calc_div(rate, parent_rate); + u32 val; clock->div = div; + val = clk_readl(clock->reg) & ~CPG_DIV6_DIV_MASK; /* Only program the new divisor if the clock isn't stopped. */ - if (!(clk_readl(clock->reg) & CPG_DIV6_CKSTP)) - clk_writel(CPG_DIV6_DIV(clock->div - 1), clock->reg); + if (!(val & CPG_DIV6_CKSTP)) + clk_writel(val | CPG_DIV6_DIV(clock->div - 1), clock->reg); + + return 0; +} + +static unsigned char cpg_div6_clock_get_parent(struct clk_hw *hw) +{ + struct div6_clock *clock = to_div6_clock(hw); + u8 hw_index; + int i; + + if (clock->src_width == 0) + return 0; + + hw_index = (clk_readl(clock->reg) >> clock->src_shift) & + (BIT(clock->src_width) - 1); + for (i = 0; i < hw->init->num_parents; i++) { + if (clock->parents[i] == hw_index) + return i; + } + + pr_err("%s: %s DIV6 clock set to invalid parent %d\n", + __func__, hw->init->name, hw_index); + return 0; +} + +static int cpg_div6_clock_set_parent(struct clk_hw *hw, u8 index) +{ + struct div6_clock *clock = to_div6_clock(hw); + u8 hw_index; + u32 mask; + + if (index >= hw->init->num_parents) + return -EINVAL; + + mask = ~((BIT(clock->src_width) - 1) << clock->src_shift); + hw_index = clock->parents[index]; + + clk_writel((clk_readl(clock->reg) & mask) | + (hw_index << clock->src_shift), clock->reg); return 0; } @@ -108,6 +155,8 @@ static const struct clk_ops cpg_div6_clock_ops = { .enable = cpg_div6_clock_enable, .disable = cpg_div6_clock_disable, .is_enabled = cpg_div6_clock_is_enabled, + .get_parent = cpg_div6_clock_get_parent, + .set_parent = cpg_div6_clock_set_parent, .recalc_rate = cpg_div6_clock_recalc_rate, .round_rate = cpg_div6_clock_round_rate, .set_rate = cpg_div6_clock_set_rate, @@ -115,12 +164,17 @@ static const struct clk_ops cpg_div6_clock_ops = { static void __init cpg_div6_clock_init(struct device_node *np) { + const char **parent_names; struct clk_init_data init; struct div6_clock *clock; - const char *parent_name; const char *name; struct clk *clk; + int valid_parents; + int num_parents; + u32 src_shift; + u32 src_width; int ret; + int i; clock = kzalloc(sizeof(*clock), GFP_KERNEL); if (!clock) { @@ -129,6 +183,24 @@ static void __init cpg_div6_clock_init(struct device_node *np) return; } + num_parents = of_clk_get_parent_count(np); + if (num_parents < 1) { + pr_err("%s: no parent found for %s DIV6 clock\n", + __func__, np->name); + return; + } + + clock->parents = kmalloc_array(num_parents, sizeof(*clock->parents), + GFP_KERNEL); + parent_names = kmalloc_array(num_parents, sizeof(*parent_names), + GFP_KERNEL); + + if (!parent_names) { + pr_err("%s: failed to allocate %s parent name list\n", + __func__, np->name); + return; + } + /* Remap the clock register and read the divisor. Disabling the * clock overwrites the divisor, so we need to cache its value for the * enable operation. @@ -150,19 +222,38 @@ static void __init cpg_div6_clock_init(struct device_node *np) goto error; } - parent_name = of_clk_get_parent_name(np, 0); - if (parent_name == NULL) { - pr_err("%s: failed to get %s DIV6 clock parent name\n", - __func__, np->name); - goto error; + + for (i = 0, valid_parents = 0; i < num_parents; i++) { + const char *name = of_clk_get_parent_name(np, i); + + if (name) { + parent_names[valid_parents] = name; + clock->parents[valid_parents] = i; + valid_parents++; + } + } + + if (!of_property_read_u32(np, "renesas,src-shift", &src_shift)) { + if (!of_property_read_u32(np, "renesas,src-width", + &src_width)) { + clock->src_shift = src_shift; + clock->src_width = src_width; + } else { + pr_err("%s: renesas,src-shift without renesas,src-width in %s\n", + __func__, np->name); + goto error; + } + } else { + clock->src_shift = clock->src_width = 0; } + /* Register the clock. */ init.name = name; init.ops = &cpg_div6_clock_ops; init.flags = CLK_IS_BASIC; - init.parent_names = &parent_name; - init.num_parents = 1; + init.parent_names = parent_names; + init.num_parents = valid_parents; clock->hw.init = &init; @@ -175,11 +266,13 @@ static void __init cpg_div6_clock_init(struct device_node *np) of_clk_add_provider(np, of_clk_src_simple_get, clk); + kfree(parent_names); return; error: if (clock->reg) iounmap(clock->reg); + kfree(parent_names); kfree(clock); } CLK_OF_DECLARE(cpg_div6_clk, "renesas,cpg-div6-clock", cpg_div6_clock_init);
Support for setting the parent at initialization time based on the current hardware configuration in DIV6 clocks with selectable parents as found in the r8a73a4, r8a7740, sh73a0, and other SoCs. Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com> --- drivers/clk/shmobile/clk-div6.c | 117 +++++++++++++++++++++++++++++++++++----- 1 file changed, 105 insertions(+), 12 deletions(-)