| Message ID | 1397724379-15398-4-git-send-email-maxime.ripard@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thursday 17 April 2014, Maxime Ripard wrote: > Since we start to have a lot of clocks to protect, some of them in a few boards > only, it becomes difficult to handle the clock protection without having to add > per machine exceptions. > > Move these where they belong, in the machine definition code. > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> I don't like the fact that these are required to be hardcoded anywhere in source code. > +#include <linux/clk.h> > #include <linux/init.h> > #include <linux/of_platform.h> > > @@ -19,9 +20,17 @@ > > static void __init sun4i_dt_init(void) > { > + struct clk *clk; > + > sunxi_setup_restart(); > > of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL); > + > + /* Make sure the clocks we absolutely need are enabled */ > + /* DDR clock */ > + clk = clk_get(NULL, "pll5_ddr"); > + if (!IS_ERR(clk)) > + clk_prepare_enable(clk); > } Isn't there already DT syntax to do the same? If not, should there be? Arnd
On Wed, Apr 23, 2014 at 02:39:02PM +0200, Arnd Bergmann wrote: > On Thursday 17 April 2014, Maxime Ripard wrote: > > Since we start to have a lot of clocks to protect, some of them in a few boards > > only, it becomes difficult to handle the clock protection without having to add > > per machine exceptions. > > > > Move these where they belong, in the machine definition code. > > > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > > I don't like the fact that these are required to be hardcoded > anywhere in source code. I agree, but that would also mean having a driver for everything that would need a clock: a CPU, the RAM. I'm not sure we want that either. > > +#include <linux/clk.h> > > #include <linux/init.h> > > #include <linux/of_platform.h> > > > > @@ -19,9 +20,17 @@ > > > > static void __init sun4i_dt_init(void) > > { > > + struct clk *clk; > > + > > sunxi_setup_restart(); > > > > of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL); > > + > > + /* Make sure the clocks we absolutely need are enabled */ > > + /* DDR clock */ > > + clk = clk_get(NULL, "pll5_ddr"); > > + if (!IS_ERR(clk)) > > + clk_prepare_enable(clk); > > } > > Isn't there already DT syntax to do the same? If not, should there be? I don't think there is, and I gave some thought about it too. But something a la regulator-always-on wouldn't work with clocks with multiple outputs (like pll5), because you might need to leave only one of the output enabled, but not the others, and I couldn't think of a nice way to do so. If you have one, I'd be happy to implement it. Maxime
On Wednesday 23 April 2014 15:17:20 Maxime Ripard wrote: > > > +#include <linux/clk.h> > > > #include <linux/init.h> > > > #include <linux/of_platform.h> > > > > > > @@ -19,9 +20,17 @@ > > > > > > static void __init sun4i_dt_init(void) > > > { > > > + struct clk *clk; > > > + > > > sunxi_setup_restart(); > > > > > > of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL); > > > + > > > + /* Make sure the clocks we absolutely need are enabled */ > > > + /* DDR clock */ > > > + clk = clk_get(NULL, "pll5_ddr"); > > > + if (!IS_ERR(clk)) > > > + clk_prepare_enable(clk); > > > } > > > > Isn't there already DT syntax to do the same? If not, should there be? > > I don't think there is, and I gave some thought about it too. But > something a la regulator-always-on wouldn't work with clocks with > multiple outputs (like pll5), because you might need to leave only one > of the output enabled, but not the others, and I couldn't think of a > nice way to do so. > > If you have one, I'd be happy to implement it. We had a discussion a while ago about encoding default settings for clock providers in the clock provider nodes. I don't remember the details unfortunately. Mike, can you explain how this should be done? Arnd
Quoting Arnd Bergmann (2014-04-23 06:31:06) > On Wednesday 23 April 2014 15:17:20 Maxime Ripard wrote: > > > > +#include <linux/clk.h> > > > > #include <linux/init.h> > > > > #include <linux/of_platform.h> > > > > > > > > @@ -19,9 +20,17 @@ > > > > > > > > static void __init sun4i_dt_init(void) > > > > { > > > > + struct clk *clk; > > > > + > > > > sunxi_setup_restart(); > > > > > > > > of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL); > > > > + > > > > + /* Make sure the clocks we absolutely need are enabled */ > > > > + /* DDR clock */ > > > > + clk = clk_get(NULL, "pll5_ddr"); > > > > + if (!IS_ERR(clk)) > > > > + clk_prepare_enable(clk); > > > > } > > > > > > Isn't there already DT syntax to do the same? If not, should there be? > > > > I don't think there is, and I gave some thought about it too. But > > something a la regulator-always-on wouldn't work with clocks with > > multiple outputs (like pll5), because you might need to leave only one > > of the output enabled, but not the others, and I couldn't think of a > > nice way to do so. > > > > If you have one, I'd be happy to implement it. > > We had a discussion a while ago about encoding default settings for clock > providers in the clock provider nodes. I don't remember the details > unfortunately. > > Mike, can you explain how this should be done? The default clock settings are still in discussion. My perspective is that the default settings are best used by the consuming device. E.g. an MMC controller that consumes a clock signal should specify in DT the clock that it consumes (already does this) and the preferred rate that it wants that clock, as well as the preferred parent if that clock is a mux. Of course clock providers themselves are devices and could specify such defaults for the clocks that they create. This approach is a minor bastardization of the default clock settings since the clock provider device isn't really consuming the clock, but it is helpful when no such driver or DT node exists to do the same. It is worth pointing out that the above approach amounts to the exact same hard coding of clk_enable requirements as is done in Maxime's patch, but with the added problem that the default are hidden away in DT somewhere which is a debugging nightmare for someone trying to figure out why a clock is set at a certain rate. Finally, to answer the question more directly, I completely oppose having something analogous to a regulator-always-on for a clock that can gate. Please always use the clk.h API for this in Linux kernel driver C code: clk_get() clk_prepare() clk_enable() In Maxime's case he is doing the right thing (using the clk.h API) but he has no memory driver to do it, so he is putting that stuff in the clk provider driver. I'm fine with that solution, until the day that he does have a memory driver and then it needs to migrate out to that code. Regards, Mike > > Arnd
diff --git a/arch/arm/mach-sunxi/sun4i.c b/arch/arm/mach-sunxi/sun4i.c index fc28b89b3378..3276e63587fb 100644 --- a/arch/arm/mach-sunxi/sun4i.c +++ b/arch/arm/mach-sunxi/sun4i.c @@ -10,6 +10,7 @@ * warranty of any kind, whether express or implied. */ +#include <linux/clk.h> #include <linux/init.h> #include <linux/of_platform.h> @@ -19,9 +20,17 @@ static void __init sun4i_dt_init(void) { + struct clk *clk; + sunxi_setup_restart(); of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL); + + /* Make sure the clocks we absolutely need are enabled */ + /* DDR clock */ + clk = clk_get(NULL, "pll5_ddr"); + if (!IS_ERR(clk)) + clk_prepare_enable(clk); } static const char * const sun4i_board_dt_compat[] = { diff --git a/arch/arm/mach-sunxi/sun5i.c b/arch/arm/mach-sunxi/sun5i.c index 623a95ad93b7..990dcfd42681 100644 --- a/arch/arm/mach-sunxi/sun5i.c +++ b/arch/arm/mach-sunxi/sun5i.c @@ -10,6 +10,7 @@ * warranty of any kind, whether express or implied. */ +#include <linux/clk.h> #include <linux/init.h> #include <linux/of_platform.h> @@ -19,9 +20,22 @@ static void __init sun5i_dt_init(void) { + struct clk *clk; + sunxi_setup_restart(); of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL); + + /* Make sure the clocks we absolutely need are enabled */ + /* Memory bus clock */ + clk = clk_get(NULL, "mbus"); + if (!IS_ERR(clk)) + clk_prepare_enable(clk); + + /* DDR clock */ + clk = clk_get(NULL, "pll5_ddr"); + if (!IS_ERR(clk)) + clk_prepare_enable(clk); } static const char * const sun5i_board_dt_compat[] = { diff --git a/arch/arm/mach-sunxi/sun7i.c b/arch/arm/mach-sunxi/sun7i.c index 2e6a8ee1966b..48a090b91a90 100644 --- a/arch/arm/mach-sunxi/sun7i.c +++ b/arch/arm/mach-sunxi/sun7i.c @@ -10,6 +10,7 @@ * warranty of any kind, whether express or implied. */ +#include <linux/clk.h> #include <linux/init.h> #include <linux/of_platform.h> @@ -19,9 +20,22 @@ static void __init sun7i_dt_init(void) { + struct clk *clk; + sunxi_setup_restart(); of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL); + + /* Make sure the clocks we absolutely need are enabled */ + /* Memory bus clock */ + clk = clk_get(NULL, "mbus"); + if (!IS_ERR(clk)) + clk_prepare_enable(clk); + + /* DDR clock */ + clk = clk_get(NULL, "pll5_ddr"); + if (!IS_ERR(clk)) + clk_prepare_enable(clk); } static const char * const sun7i_board_dt_compat[] = { diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c index b1fde0f89bc1..b06200079f06 100644 --- a/drivers/clk/sunxi/clk-sunxi.c +++ b/drivers/clk/sunxi/clk-sunxi.c @@ -1285,27 +1285,6 @@ static void __init of_sunxi_table_clock_setup(const struct of_device_id *clk_mat } } -/** - * System clock protection - * - * By enabling these critical clocks, we prevent their accidental gating - * by the framework - */ -static void __init sunxi_clock_protect(void) -{ - struct clk *clk; - - /* memory bus clock - sun5i+ */ - clk = clk_get(NULL, "mbus"); - if (!IS_ERR(clk)) - clk_prepare_enable(clk); - - /* DDR clock - sun4i+ */ - clk = clk_get(NULL, "pll5_ddr"); - if (!IS_ERR(clk)) - clk_prepare_enable(clk); -} - static void __init sunxi_init_clocks(void) { /* Register factor clocks */ @@ -1322,9 +1301,6 @@ static void __init sunxi_init_clocks(void) /* Register gate clocks */ of_sunxi_table_clock_setup(clk_gates_match, sunxi_gates_clk_setup); - - /* Enable core system clocks */ - sunxi_clock_protect(); } CLK_OF_DECLARE(sun4i_a10_clk_init, "allwinner,sun4i-a10", sunxi_init_clocks); CLK_OF_DECLARE(sun5i_a10s_clk_init, "allwinner,sun5i-a10s", sunxi_init_clocks);
Since we start to have a lot of clocks to protect, some of them in a few boards only, it becomes difficult to handle the clock protection without having to add per machine exceptions. Move these where they belong, in the machine definition code. Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> --- arch/arm/mach-sunxi/sun4i.c | 9 +++++++++ arch/arm/mach-sunxi/sun5i.c | 14 ++++++++++++++ arch/arm/mach-sunxi/sun7i.c | 14 ++++++++++++++ drivers/clk/sunxi/clk-sunxi.c | 24 ------------------------ 4 files changed, 37 insertions(+), 24 deletions(-)