| Message ID | 1458587984-6313-3-git-send-email-wsa@the-dreams.de (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Simon Horman |
| Headers | show |
Hi Wolfram, On Mon, Mar 21, 2016 at 8:19 PM, Wolfram Sang <wsa@the-dreams.de> wrote: > From: Wolfram Sang <wsa+renesas@sang-engineering.com> > > Gen3 has two clocks (OSC and R) which look like a DIV6 clock but their > divider value is read-only and depends on MD pins at bootup. Add support > for such clocks by reading the value and adding a fixed clock. I may be blind, but I couldn't find the register to read the OSC divider from? > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > --- > drivers/clk/shmobile/renesas-cpg-mssr.c | 18 ++++++++++++------ > drivers/clk/shmobile/renesas-cpg-mssr.h | 3 +++ > 2 files changed, 15 insertions(+), 6 deletions(-) > > diff --git a/drivers/clk/shmobile/renesas-cpg-mssr.c b/drivers/clk/shmobile/renesas-cpg-mssr.c > index 58e24b326a48bb..3e4d2609cc0292 100644 > --- a/drivers/clk/shmobile/renesas-cpg-mssr.c > +++ b/drivers/clk/shmobile/renesas-cpg-mssr.c > @@ -274,13 +275,18 @@ static void __init cpg_mssr_register_core_clk(const struct cpg_core_clk *core, > } > > parent_name = __clk_get_name(parent); > - if (core->type == CLK_TYPE_FF) { > - clk = clk_register_fixed_factor(NULL, core->name, > - parent_name, 0, > - core->mult, core->div); > - } else { > + > + if (core->type == CLK_TYPE_DIV6_RO) > + /* Multiply with the DIV6 register value */ > + div *= (readl(priv->base + core->offset) & 0x3f) + 1; > + > + if (core->type == CLK_TYPE_DIV6P1) { > clk = cpg_div6_register(core->name, 1, &parent_name, > priv->base + core->offset); > + } else { > + clk = clk_register_fixed_factor(NULL, core->name, > + parent_name, 0, > + core->mult, div); > } Please use a switch() instead of if / else. It avoids people wondering where the "core->type == CLK_TYPE_FF" case went to, until they realize it's covered by the else. 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
> > - if (core->type == CLK_TYPE_FF) { > > - clk = clk_register_fixed_factor(NULL, core->name, > > - parent_name, 0, > > - core->mult, core->div); > > - } else { > > + > > + if (core->type == CLK_TYPE_DIV6_RO) > > + /* Multiply with the DIV6 register value */ > > + div *= (readl(priv->base + core->offset) & 0x3f) + 1; > > + > > + if (core->type == CLK_TYPE_DIV6P1) { > > clk = cpg_div6_register(core->name, 1, &parent_name, > > priv->base + core->offset); > > + } else { > > + clk = clk_register_fixed_factor(NULL, core->name, > > + parent_name, 0, > > + core->mult, div); > > } > > Please use a switch() instead of if / else. It avoids people wondering where > the "core->type == CLK_TYPE_FF" case went to, until they realize it's covered > by the else. Are you sure? It is as you say when reading the diff. But looking at the source file after the patch, I think this is easily understandable?
Hi Wolfram, On Wed, Mar 23, 2016 at 4:26 PM, Wolfram Sang <wsa@the-dreams.de> wrote: >> > - if (core->type == CLK_TYPE_FF) { >> > - clk = clk_register_fixed_factor(NULL, core->name, >> > - parent_name, 0, >> > - core->mult, core->div); >> > - } else { >> > + >> > + if (core->type == CLK_TYPE_DIV6_RO) >> > + /* Multiply with the DIV6 register value */ >> > + div *= (readl(priv->base + core->offset) & 0x3f) + 1; >> > + >> > + if (core->type == CLK_TYPE_DIV6P1) { >> > clk = cpg_div6_register(core->name, 1, &parent_name, >> > priv->base + core->offset); >> > + } else { >> > + clk = clk_register_fixed_factor(NULL, core->name, >> > + parent_name, 0, >> > + core->mult, div); >> > } >> >> Please use a switch() instead of if / else. It avoids people wondering where >> the "core->type == CLK_TYPE_FF" case went to, until they realize it's covered >> by the else. > > Are you sure? It is as you say when reading the diff. But looking at the > source file after the patch, I think this is easily understandable? Sorry, you're right again. 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
> > Are you sure? It is as you say when reading the diff. But looking at the > > source file after the patch, I think this is easily understandable? > > Sorry, you're right again. Thanks. And you were right, too. Seems like I can simply use clk_get_rate() in the other patch for reading EXTALR after all :)
diff --git a/drivers/clk/shmobile/renesas-cpg-mssr.c b/drivers/clk/shmobile/renesas-cpg-mssr.c index 58e24b326a48bb..3e4d2609cc0292 100644 --- a/drivers/clk/shmobile/renesas-cpg-mssr.c +++ b/drivers/clk/shmobile/renesas-cpg-mssr.c @@ -253,7 +253,7 @@ static void __init cpg_mssr_register_core_clk(const struct cpg_core_clk *core, { struct clk *clk = NULL, *parent; struct device *dev = priv->dev; - unsigned int id = core->id; + unsigned int id = core->id, div = core->div; const char *parent_name; WARN_DEBUG(id >= priv->num_core_clks); @@ -266,6 +266,7 @@ static void __init cpg_mssr_register_core_clk(const struct cpg_core_clk *core, case CLK_TYPE_FF: case CLK_TYPE_DIV6P1: + case CLK_TYPE_DIV6_RO: WARN_DEBUG(core->parent >= priv->num_core_clks); parent = priv->clks[core->parent]; if (IS_ERR(parent)) { @@ -274,13 +275,18 @@ static void __init cpg_mssr_register_core_clk(const struct cpg_core_clk *core, } parent_name = __clk_get_name(parent); - if (core->type == CLK_TYPE_FF) { - clk = clk_register_fixed_factor(NULL, core->name, - parent_name, 0, - core->mult, core->div); - } else { + + if (core->type == CLK_TYPE_DIV6_RO) + /* Multiply with the DIV6 register value */ + div *= (readl(priv->base + core->offset) & 0x3f) + 1; + + if (core->type == CLK_TYPE_DIV6P1) { clk = cpg_div6_register(core->name, 1, &parent_name, priv->base + core->offset); + } else { + clk = clk_register_fixed_factor(NULL, core->name, + parent_name, 0, + core->mult, div); } break; diff --git a/drivers/clk/shmobile/renesas-cpg-mssr.h b/drivers/clk/shmobile/renesas-cpg-mssr.h index cad3c7d1b0c6f0..0d1e3e811e79bf 100644 --- a/drivers/clk/shmobile/renesas-cpg-mssr.h +++ b/drivers/clk/shmobile/renesas-cpg-mssr.h @@ -37,6 +37,7 @@ enum clk_types { CLK_TYPE_IN, /* External Clock Input */ CLK_TYPE_FF, /* Fixed Factor Clock */ CLK_TYPE_DIV6P1, /* DIV6 Clock with 1 parent clock */ + CLK_TYPE_DIV6_RO, /* DIV6 Clock read only with extra divisor */ /* Custom definitions start here */ CLK_TYPE_CUSTOM, @@ -53,6 +54,8 @@ enum clk_types { DEF_BASE(_name, _id, CLK_TYPE_FF, _parent, .div = _div, .mult = _mult) #define DEF_DIV6P1(_name, _id, _parent, _offset) \ DEF_BASE(_name, _id, CLK_TYPE_DIV6P1, _parent, .offset = _offset) +#define DEF_DIV6_RO(_name, _id, _parent, _offset, _div) \ + DEF_BASE(_name, _id, CLK_TYPE_DIV6_RO, _parent, .offset = _offset, .div = _div, .mult = 1) /* * Definitions of Module Clocks