| Message ID | 1462797111-14271-3-git-send-email-joel@jms.id.au (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 05/09, Joel Stanley wrote: > diff --git a/drivers/clk/aspeed/clk-g4.c b/drivers/clk/aspeed/clk-g4.c > new file mode 100644 > index 000000000000..91677e9177f5 > --- /dev/null > +++ b/drivers/clk/aspeed/clk-g4.c > @@ -0,0 +1,109 @@ > +/* > + * Copyright 2016 IBM Corporation > + * > + * 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; either version > + * 2 of the License, or (at your option) any later version. > + */ > + > +#include <linux/clk.h> Hopefully this include isn't needed. > +#include <linux/clk-provider.h> > +#include <linux/io.h> > +#include <linux/of_address.h> > +#include <linux/clkdev.h> > + > +static void __init aspeed_of_hpll_clk_init(struct device_node *node) > +{ > + struct clk *clk, *clkin_clk; > + void __iomem *base; > + int reg, rate, clkin; > + const char *name = node->name; > + const char *parent_name; > + const int rates[][4] = { > + {384, 360, 336, 408}, > + {400, 375, 350, 425}, > + }; > + > + of_property_read_string(node, "clock-output-names", &name); > + parent_name = of_clk_get_parent_name(node, 0); > + > + base = of_iomap(node, 0); > + if (!base) { > + pr_err("%s: of_iomap failed\n", node->full_name); > + return; > + } > + reg = readl(base); > + iounmap(base); > + > + clkin_clk = of_clk_get(node, 0); > + if (IS_ERR(clkin_clk)) { > + pr_err("%s: of_clk_get failed\n", node->full_name); > + return; > + } > + > + clkin = clk_get_rate(clkin_clk); > + > + reg = (reg >> 8) & 0x2; > + > + if (clkin == 48000000 || clkin == 24000000) > + rate = rates[0][reg] * 1000000; > + else if (clkin == 25000000) > + rate = rates[1][reg] * 1000000; > + else { > + pr_err("%s: unknown clkin frequency %dHz\n", > + node->full_name, clkin); > + WARN_ON(1); > + } > + > + clk = clk_register_fixed_rate(NULL, name, parent_name, 0, rate); Please write a proper clk driver that sets clkin to be the parent of this clk registered here and then calculates the frequency based on the parent clk rate and the strapping registers via the recalc rate callback. Also please move this to a platform driver instead of using CLK_OF_DECLARE assuming this isn't required for any timers in the system. > + if (IS_ERR(clk)) { > + pr_err("%s: failed to register clock\n", node->full_name); > + return; > + } > + > + clk_register_clkdev(clk, NULL, name); Do you have clkdev users? If not then please drop this. > + of_clk_add_provider(node, of_clk_src_simple_get, clk); > +} > +CLK_OF_DECLARE(aspeed_hpll_clock, "aspeed,g4-hpll-clock", > + aspeed_of_hpll_clk_init); > + > +static void __init aspeed_of_apb_clk_init(struct device_node *node) > +{ > + struct clk *clk, *hpll_clk; > + void __iomem *base; > + int reg, rate; > + const char *name = node->name; > + const char *parent_name; > + > + of_property_read_string(node, "clock-output-names", &name); > + parent_name = of_clk_get_parent_name(node, 0); > + > + base = of_iomap(node, 0); > + if (!base) { > + pr_err("%s: of_iomap failed\n", node->full_name); > + return; > + } > + reg = readl(base); > + iounmap(base); > + > + hpll_clk = of_clk_get(node, 0); > + if (IS_ERR(hpll_clk)) { > + pr_err("%s: of_clk_get failed\n", node->full_name); > + return; > + } > + > + reg = (reg >> 23) & 0x3; > + rate = clk_get_rate(hpll_clk) / (2 + 2 * reg); > + > + clk = clk_register_fixed_rate(NULL, name, parent_name, 0, rate); > + if (IS_ERR(clk)) { > + pr_err("%s: failed to register clock\n", node->full_name); > + return; > + } > + > + clk_register_clkdev(clk, NULL, name); > + of_clk_add_provider(node, of_clk_src_simple_get, clk); > +} > +CLK_OF_DECLARE(aspeed_apb_clock, "aspeed,g4-apb-clock", > + aspeed_of_apb_clk_init); Following on Rob's comment, please combine this into one driver instead of having different DT nodes for different clks that are all really part of one clock controller hw block.
On Tue, May 10, 2016 at 8:19 AM, Stephen Boyd <sboyd@codeaurora.org> wrote: > On 05/09, Joel Stanley wrote: >> diff --git a/drivers/clk/aspeed/clk-g4.c b/drivers/clk/aspeed/clk-g4.c >> new file mode 100644 >> index 000000000000..91677e9177f5 >> --- /dev/null >> +++ b/drivers/clk/aspeed/clk-g4.c >> @@ -0,0 +1,109 @@ >> +/* >> + * Copyright 2016 IBM Corporation >> + * >> + * 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; either version >> + * 2 of the License, or (at your option) any later version. >> + */ >> + >> +#include <linux/clk.h> > > Hopefully this include isn't needed. It was required for clk_get_rate in aspeed_of_pclk_clk_init. As I mention below, I reworked it to avoid this. > >> +#include <linux/clk-provider.h> >> +#include <linux/io.h> >> +#include <linux/of_address.h> >> +#include <linux/clkdev.h> >> + >> +static void __init aspeed_of_hpll_clk_init(struct device_node *node) >> +{ >> + struct clk *clk, *clkin_clk; >> + void __iomem *base; >> + int reg, rate, clkin; >> + const char *name = node->name; >> + const char *parent_name; >> + const int rates[][4] = { >> + {384, 360, 336, 408}, >> + {400, 375, 350, 425}, >> + }; >> + >> + of_property_read_string(node, "clock-output-names", &name); >> + parent_name = of_clk_get_parent_name(node, 0); >> + >> + base = of_iomap(node, 0); >> + if (!base) { >> + pr_err("%s: of_iomap failed\n", node->full_name); >> + return; >> + } >> + reg = readl(base); >> + iounmap(base); >> + >> + clkin_clk = of_clk_get(node, 0); >> + if (IS_ERR(clkin_clk)) { >> + pr_err("%s: of_clk_get failed\n", node->full_name); >> + return; >> + } >> + >> + clkin = clk_get_rate(clkin_clk); >> + >> + reg = (reg >> 8) & 0x2; >> + >> + if (clkin == 48000000 || clkin == 24000000) >> + rate = rates[0][reg] * 1000000; >> + else if (clkin == 25000000) >> + rate = rates[1][reg] * 1000000; >> + else { >> + pr_err("%s: unknown clkin frequency %dHz\n", >> + node->full_name, clkin); >> + WARN_ON(1); >> + } >> + >> + clk = clk_register_fixed_rate(NULL, name, parent_name, 0, rate); > > Please write a proper clk driver that sets clkin to be the parent > of this clk registered here and then calculates the frequency > based on the parent clk rate and the strapping registers via the > recalc rate callback. After a few goes I understood what you meant here. Is the pclk part okay? I modified the pclk part use a fixed factor clock instead of the fixed clock, so I could drop the pclk_get_rate call below. > Also please move this to a platform driver > instead of using CLK_OF_DECLARE assuming this isn't required for > any timers in the system. Can you clarify here? We do use the rate of pclk (the apb clock) in the clocksource driver to calculate our count_per_tick value. > >> + if (IS_ERR(clk)) { >> + pr_err("%s: failed to register clock\n", node->full_name); >> + return; >> + } >> + >> + clk_register_clkdev(clk, NULL, name); > > Do you have clkdev users? If not then please drop this. I think the answer is no. I will drop. >> + of_clk_add_provider(node, of_clk_src_simple_get, clk); >> +} >> +CLK_OF_DECLARE(aspeed_hpll_clock, "aspeed,g4-hpll-clock", >> + aspeed_of_hpll_clk_init); >> + >> +static void __init aspeed_of_apb_clk_init(struct device_node *node) >> +{ >> + struct clk *clk, *hpll_clk; >> + void __iomem *base; >> + int reg, rate; >> + const char *name = node->name; >> + const char *parent_name; >> + >> + of_property_read_string(node, "clock-output-names", &name); >> + parent_name = of_clk_get_parent_name(node, 0); >> + >> + base = of_iomap(node, 0); >> + if (!base) { >> + pr_err("%s: of_iomap failed\n", node->full_name); >> + return; >> + } >> + reg = readl(base); >> + iounmap(base); >> + >> + hpll_clk = of_clk_get(node, 0); >> + if (IS_ERR(hpll_clk)) { >> + pr_err("%s: of_clk_get failed\n", node->full_name); >> + return; >> + } >> + >> + reg = (reg >> 23) & 0x3; >> + rate = clk_get_rate(hpll_clk) / (2 + 2 * reg); >> + >> + clk = clk_register_fixed_rate(NULL, name, parent_name, 0, rate); >> + if (IS_ERR(clk)) { >> + pr_err("%s: failed to register clock\n", node->full_name); >> + return; >> + } >> + >> + clk_register_clkdev(clk, NULL, name); >> + of_clk_add_provider(node, of_clk_src_simple_get, clk); >> +} >> +CLK_OF_DECLARE(aspeed_apb_clock, "aspeed,g4-apb-clock", >> + aspeed_of_apb_clk_init); > > Following on Rob's comment, please combine this into one driver > instead of having different DT nodes for different clks that are > all really part of one clock controller hw block. Ok, I have had a go at this. In our case the clock registers are part of the "SCU" IP; a collection of disparate functions that happen to include clock control. Is syscon the right thing to use here? regmap = syscon_regmap_lookup_by_compatible("aspeed,ast2400-syscon-scu"); ret = regmap_read(hpll->regmap, 0x70, ®); Cheers, Joel
On 05/10, Joel Stanley wrote: > On Tue, May 10, 2016 at 8:19 AM, Stephen Boyd <sboyd@codeaurora.org> wrote: > > Please write a proper clk driver that sets clkin to be the parent > > of this clk registered here and then calculates the frequency > > based on the parent clk rate and the strapping registers via the > > recalc rate callback. > > After a few goes I understood what you meant here. > > Is the pclk part okay? I modified the pclk part use a fixed factor > clock instead of the fixed clock, so I could drop the pclk_get_rate > call below. Sorry what's the pclk part? Using a fixed factor is OK, as long as we don't have to call clk_get_rate() in this driver it's a better design. > > > Also please move this to a platform driver > > instead of using CLK_OF_DECLARE assuming this isn't required for > > any timers in the system. > > Can you clarify here? We do use the rate of pclk (the apb clock) in > the clocksource driver to calculate our count_per_tick value. Ok. I was hoping that this wasn't providing any clks required in the timer driver. Even if that's true, we could just register the part of the clk tree that needs to be ready for the timer from CLK_OF_DECLARE() and then register the other clks from a regular platform driver. Some people have already started doing this, so there are some examples around (clk/nxp/clk-lpc18xx-creg.c is one example). > > Following on Rob's comment, please combine this into one driver > > instead of having different DT nodes for different clks that are > > all really part of one clock controller hw block. > > Ok, I have had a go at this. In our case the clock registers are part > of the "SCU" IP; a collection of disparate functions that happen to > include clock control. Is syscon the right thing to use here? > > regmap = syscon_regmap_lookup_by_compatible("aspeed,ast2400-syscon-scu"); > ret = regmap_read(hpll->regmap, 0x70, ®); > In that case I don't have a problem with a toplevel MFD node in DT with a child node for the clk part and a child node for other logical driver parts. Or it could all be one MFD driver and node that knows to add a clk device as a child of the MFD purely in software so that we can fork control of the clk part over to the drivers/clk directory. If you have the clk driver attach to some child device then you can always get the regmap from the parent MFD via the dev->parent pointer. Maybe Rob can chime in here too with the proper design, because I've seen both styles.
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile index 46869d696e4d..8eaaef013a28 100644 --- a/drivers/clk/Makefile +++ b/drivers/clk/Makefile @@ -84,3 +84,4 @@ obj-$(CONFIG_X86) += x86/ obj-$(CONFIG_ARCH_ZX) += zte/ obj-$(CONFIG_ARCH_ZYNQ) += zynq/ obj-$(CONFIG_H8300) += h8300/ +obj-$(CONFIG_ARCH_ASPEED) += aspeed/ diff --git a/drivers/clk/aspeed/Makefile b/drivers/clk/aspeed/Makefile new file mode 100644 index 000000000000..d3457fbe3019 --- /dev/null +++ b/drivers/clk/aspeed/Makefile @@ -0,0 +1 @@ +obj-$(CONFIG_MACH_ASPEED_G4) += clk-g4.o diff --git a/drivers/clk/aspeed/clk-g4.c b/drivers/clk/aspeed/clk-g4.c new file mode 100644 index 000000000000..91677e9177f5 --- /dev/null +++ b/drivers/clk/aspeed/clk-g4.c @@ -0,0 +1,109 @@ +/* + * Copyright 2016 IBM Corporation + * + * 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; either version + * 2 of the License, or (at your option) any later version. + */ + +#include <linux/clk.h> +#include <linux/clk-provider.h> +#include <linux/io.h> +#include <linux/of_address.h> +#include <linux/clkdev.h> + +static void __init aspeed_of_hpll_clk_init(struct device_node *node) +{ + struct clk *clk, *clkin_clk; + void __iomem *base; + int reg, rate, clkin; + const char *name = node->name; + const char *parent_name; + const int rates[][4] = { + {384, 360, 336, 408}, + {400, 375, 350, 425}, + }; + + of_property_read_string(node, "clock-output-names", &name); + parent_name = of_clk_get_parent_name(node, 0); + + base = of_iomap(node, 0); + if (!base) { + pr_err("%s: of_iomap failed\n", node->full_name); + return; + } + reg = readl(base); + iounmap(base); + + clkin_clk = of_clk_get(node, 0); + if (IS_ERR(clkin_clk)) { + pr_err("%s: of_clk_get failed\n", node->full_name); + return; + } + + clkin = clk_get_rate(clkin_clk); + + reg = (reg >> 8) & 0x2; + + if (clkin == 48000000 || clkin == 24000000) + rate = rates[0][reg] * 1000000; + else if (clkin == 25000000) + rate = rates[1][reg] * 1000000; + else { + pr_err("%s: unknown clkin frequency %dHz\n", + node->full_name, clkin); + WARN_ON(1); + } + + clk = clk_register_fixed_rate(NULL, name, parent_name, 0, rate); + if (IS_ERR(clk)) { + pr_err("%s: failed to register clock\n", node->full_name); + return; + } + + clk_register_clkdev(clk, NULL, name); + of_clk_add_provider(node, of_clk_src_simple_get, clk); +} +CLK_OF_DECLARE(aspeed_hpll_clock, "aspeed,g4-hpll-clock", + aspeed_of_hpll_clk_init); + +static void __init aspeed_of_apb_clk_init(struct device_node *node) +{ + struct clk *clk, *hpll_clk; + void __iomem *base; + int reg, rate; + const char *name = node->name; + const char *parent_name; + + of_property_read_string(node, "clock-output-names", &name); + parent_name = of_clk_get_parent_name(node, 0); + + base = of_iomap(node, 0); + if (!base) { + pr_err("%s: of_iomap failed\n", node->full_name); + return; + } + reg = readl(base); + iounmap(base); + + hpll_clk = of_clk_get(node, 0); + if (IS_ERR(hpll_clk)) { + pr_err("%s: of_clk_get failed\n", node->full_name); + return; + } + + reg = (reg >> 23) & 0x3; + rate = clk_get_rate(hpll_clk) / (2 + 2 * reg); + + clk = clk_register_fixed_rate(NULL, name, parent_name, 0, rate); + if (IS_ERR(clk)) { + pr_err("%s: failed to register clock\n", node->full_name); + return; + } + + clk_register_clkdev(clk, NULL, name); + of_clk_add_provider(node, of_clk_src_simple_get, clk); +} +CLK_OF_DECLARE(aspeed_apb_clock, "aspeed,g4-apb-clock", + aspeed_of_apb_clk_init);
A basic driver to create fixed rate clock devices from strapping registers. The clocks on the ast2400 are derived from an external oscillator and the frequency of this can be derived from the strapping of the processor. The frequency of internal clocks can be derived from other registers in the SCU (System Control Unit). Signed-off-by: Joel Stanley <joel@jms.id.au> --- drivers/clk/Makefile | 1 + drivers/clk/aspeed/Makefile | 1 + drivers/clk/aspeed/clk-g4.c | 109 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 111 insertions(+) create mode 100644 drivers/clk/aspeed/Makefile create mode 100644 drivers/clk/aspeed/clk-g4.c