[PATCH RFC v2 00/16] ARM: provide common arch init for DT clocks

[PATCH RFC v2 00/16] ARM: provide common arch init for DT clocks

[Sebastian Hesselbarth]

This RFC converts arch/arm to provide a common arch init for DT clock
providers. Currently, the call to of_clk_init(NULL) to initialize DT
clock providers is spread among several mach-dirs. Since most machs
require DT clocks initialized before timers, no initcall can be used.

By adding of_clk_init(NULL) to ARM time_init(), we can remove all
mach-specific .init_time hooks that basically called of_clk_init before
starting timers.

Based on the previous version, this one now includes patches provided by
Stephen Warren for mach-tegra and Soren Brinkmann for mach-zynq.

With mach-tegra now requiring clocks as late as the other machs, the
patch to protect of_clk_init() from being called twice, has been dropped.

The RFCv2 is based on next-20130827 and has been compile tested for
multi_v7_defconfig and mach-dove. All single patches have also been sent
to the respective maintainers and corresponding mailing lists.

Before going for a real patch set after v3.11 drops, I prefer to have
Tested-by or Acked-by for at least mach-imx, mach-mxs, mach-sti, and
mach-vexpress. Others are trivial, prepared by maintainers (mach-tegra
and mach-zynq), or have already been Acked-by (mach-highbank).

Also, I'd like to know if the patches should be grouped by mach- or
rather been split into mach- preparation and .init_time removal.

Sebastian Hesselbarth (14):
  ARM: call clk_of_init from time_init
  ARM: dove: remove custom .init_time hook
  ARM: exynos: remove custom .init_time hook
  ARM: highbank: remove custom .init_time hook
  ARM: imx: remove custom .init_time hook
  ARM: kirkwood: remove custom .init_time hook
  ARM: mvebu: remove custom .init_time hook
  ARM: mxs: remove custom .init_time hook
  ARM: nspire: remove custom .init_time hook
  ARM: rockchip: remove custom .init_time hook
  ARM: socfpga: remove call to of_clk_init
  ARM: sti: remove custom .init_time hook
  ARM: vexpress: remove custom .init_time hook
  clk: vt8500: remove call to of_clk_init

Soren Brinkmann (1):
  ARM: zynq: Don't call of_clk_init()

Stephen Warren (1):
  ARM: tegra: split tegra_pmc_init() in two

 arch/arm/kernel/time.c                 |   24 +++++++++++-------
 arch/arm/mach-dove/board-dt.c          |   17 +------------
 arch/arm/mach-exynos/common.c          |    7 -----
 arch/arm/mach-exynos/common.h          |    1 -
 arch/arm/mach-exynos/mach-exynos4-dt.c |    1 -
 arch/arm/mach-exynos/mach-exynos5-dt.c |    1 -
 arch/arm/mach-highbank/highbank.c      |   23 +++++------------
 arch/arm/mach-imx/clk-imx51-imx53.c    |   29 +++++++--------------
 arch/arm/mach-imx/common.h             |    4 ---
 arch/arm/mach-imx/imx51-dt.c           |    6 ----
 arch/arm/mach-imx/mach-imx53.c         |    6 ----
 arch/arm/mach-imx/mach-imx6q.c         |   14 ++--------
 arch/arm/mach-imx/mach-imx6sl.c        |    7 -----
 arch/arm/mach-imx/mach-vf610.c         |    9 -------
 arch/arm/mach-kirkwood/board-dt.c      |    8 ------
 arch/arm/mach-mvebu/armada-370-xp.c    |    2 -
 arch/arm/mach-mxs/mach-mxs.c           |   13 ----------
 arch/arm/mach-nspire/nspire.c          |    9 -------
 arch/arm/mach-rockchip/rockchip.c      |    9 -------
 arch/arm/mach-socfpga/socfpga.c        |    2 -
 arch/arm/mach-sti/board-dt.c           |   10 +++----
 arch/arm/mach-tegra/common.c           |    4 +--
 arch/arm/mach-tegra/pmc.c              |   41 +++++++++++++++++--------------
 arch/arm/mach-tegra/pmc.h              |    1 +
 arch/arm/mach-tegra/tegra.c            |    5 ++-
 arch/arm/mach-vexpress/v2m.c           |   14 +----------
 arch/arm/mach-zynq/common.c            |    9 +++----
 drivers/clk/clk-highbank.c             |   10 +++++--
 drivers/clk/clk-vt8500.c               |    2 -
 drivers/clk/mxs/clk-imx23.c            |   16 +++++-------
 drivers/clk/mxs/clk-imx28.c            |   16 +++++-------
 drivers/clk/zynq/clkc.c                |    4 ++-
 include/linux/clk/mxs.h                |    2 -
 33 files changed, 95 insertions(+), 231 deletions(-)

---
Cc: Mike Turquette <mturquette@linaro.org>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: linux-tegra@vger.kernel.org
Cc: kernel@stlinux.com
Cc: linux-samsung-soc@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
-- 
1.7.2.5

[PATCH RFC v2 01/16] ARM: call clk_of_init from time_init

[Sebastian Hesselbarth]

Most DT ARM machs require common clock providers initialized before timers.
Currently, arch/arm machs use .init_time to call clk_of_init right before
clocksource_of_init. This prevents to remove that hook and use the default
hook instead. clk_of_init is safe to call for non-DT platforms, so add
the call to ARM arch time_init by default. While at it, also reorder includes
alphabetically.

Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
Changelog:
v1->v2:
- reorder includes alphabetically

Cc: Russell King <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
Cc: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
Cc: linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: kernel-F5mvAk5X5gdBDgjK7y7TUQ@public.gmane.org
Cc: linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
---
 arch/arm/kernel/time.c |   24 ++++++++++++++----------
 1 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/arch/arm/kernel/time.c b/arch/arm/kernel/time.c
index 98aee32..dd1028e 100644
--- a/arch/arm/kernel/time.c
+++ b/arch/arm/kernel/time.c
@@ -11,25 +11,26 @@
  *  This file contains the ARM-specific time handling details:
  *  reading the RTC at bootup, etc...
  */
+#include <linux/clk-provider.h>
+#include <linux/clocksource.h>
+#include <linux/errno.h>
 #include <linux/export.h>
-#include <linux/kernel.h>
-#include <linux/interrupt.h>
-#include <linux/time.h>
 #include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/kernel.h>
+#include <linux/profile.h>
 #include <linux/sched.h>
+#include <linux/sched_clock.h>
 #include <linux/smp.h>
+#include <linux/time.h>
 #include <linux/timex.h>
-#include <linux/errno.h>
-#include <linux/profile.h>
 #include <linux/timer.h>
-#include <linux/clocksource.h>
-#include <linux/irq.h>
-#include <linux/sched_clock.h>
 
-#include <asm/thread_info.h>
-#include <asm/stacktrace.h>
 #include <asm/mach/arch.h>
 #include <asm/mach/time.h>
+#include <asm/stacktrace.h>
+#include <asm/thread_info.h>
 
 #if defined(CONFIG_RTC_DRV_CMOS) || defined(CONFIG_RTC_DRV_CMOS_MODULE) || \
     defined(CONFIG_NVRAM) || defined(CONFIG_NVRAM_MODULE)
@@ -116,6 +117,9 @@ int __init register_persistent_clock(clock_access_fn read_boot,
 
 void __init time_init(void)
 {
+	/* initalize common clocks before timers */
+	of_clk_init(NULL);
+
 	if (machine_desc->init_time)
 		machine_desc->init_time();
 	else
-- 
1.7.2.5

[PATCH RFC v2 13/16] ARM: tegra: split tegra_pmc_init() in two

[Sebastian Hesselbarth]

From: Stephen Warren <swarren@nvidia.com>

Tegra's board file currently initializes clocks much earlier than those
for most other ARM SoCs. The reason is:

* The PMC HW block is involved in the path of some interrupts (i.e. it
inverts, or not, the IRQ input pin dedicated to the PMIC).

* So, that part of the PMC must be initialized early so that the IRQ
polarity is correct.

* The PMC initialization is currently monolithic, and the PMC has some
clock inputs, so the init routine ends up calling of_clk_get_by_name(),
and hence clocks must be set up early too.

In order to defer clock initialization to the more typical location,
split out the portions of tegra_pmc_init() that are truly IRQ-related
into a separate tegra_pmc_init_irq(), which can be called from the
machine descriptor's .init_irq() function, and defer the rest until
the machine descriptor's .init_machine() function. With arch/arm calling
of_clk_init(NULL) from time_init() this also allows the removal of
.init_time() hook.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
Changelog:
v1->v2:
- took Stephen Warren's patch provided to separate Tegra's pmc_init
- sqashed in .init_time removal and reworded patch text

Cc: Stephen Warren <swarren@wwwdotorg.org>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: linux-tegra@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
 arch/arm/mach-tegra/common.c |    4 +---
 arch/arm/mach-tegra/pmc.c    |   41 ++++++++++++++++++++++-------------------
 arch/arm/mach-tegra/pmc.h    |    1 +
 arch/arm/mach-tegra/tegra.c  |    5 +++--
 4 files changed, 27 insertions(+), 24 deletions(-)

diff --git a/arch/arm/mach-tegra/common.c b/arch/arm/mach-tegra/common.c
index 94a119a..58dc91c5 100644
--- a/arch/arm/mach-tegra/common.c
+++ b/arch/arm/mach-tegra/common.c
@@ -24,7 +24,6 @@
 #include <linux/delay.h>
 #include <linux/reboot.h>
 #include <linux/irqchip.h>
-#include <linux/clk-provider.h>
 
 #include <asm/hardware/cache-l2x0.h>
 
@@ -61,8 +60,7 @@ u32 tegra_uart_config[4] = {
 #ifdef CONFIG_OF
 void __init tegra_dt_init_irq(void)
 {
-	of_clk_init(NULL);
-	tegra_pmc_init();
+	tegra_pmc_init_irq();
 	tegra_init_irq();
 	irqchip_init();
 	tegra_legacy_irq_syscore_init();
diff --git a/arch/arm/mach-tegra/pmc.c b/arch/arm/mach-tegra/pmc.c
index 8acb881..7916ff9 100644
--- a/arch/arm/mach-tegra/pmc.c
+++ b/arch/arm/mach-tegra/pmc.c
@@ -285,13 +285,10 @@ static const struct of_device_id matches[] __initconst = {
 	{ }
 };
 
-static void __init tegra_pmc_parse_dt(void)
+void __init tegra_pmc_init_irq(void)
 {
 	struct device_node *np;
-	u32 prop;
-	enum tegra_suspend_mode suspend_mode;
-	u32 core_good_time[2] = {0, 0};
-	u32 lp0_vec[2] = {0, 0};
+	u32 val;
 
 	np = of_find_matching_node(NULL, matches);
 	BUG_ON(!np);
@@ -300,6 +297,26 @@ static void __init tegra_pmc_parse_dt(void)
 
 	tegra_pmc_invert_interrupt = of_property_read_bool(np,
 				     "nvidia,invert-interrupt");
+
+	val = tegra_pmc_readl(PMC_CTRL);
+	if (tegra_pmc_invert_interrupt)
+		val |= PMC_CTRL_INTR_LOW;
+	else
+		val &= ~PMC_CTRL_INTR_LOW;
+	tegra_pmc_writel(val, PMC_CTRL);
+}
+
+void __init tegra_pmc_init(void)
+{
+	struct device_node *np;
+	u32 prop;
+	enum tegra_suspend_mode suspend_mode;
+	u32 core_good_time[2] = {0, 0};
+	u32 lp0_vec[2] = {0, 0};
+
+	np = of_find_matching_node(NULL, matches);
+	BUG_ON(!np);
+
 	tegra_pclk = of_clk_get_by_name(np, "pclk");
 	WARN_ON(IS_ERR(tegra_pclk));
 
@@ -365,17 +382,3 @@ static void __init tegra_pmc_parse_dt(void)
 
 	pmc_pm_data.suspend_mode = suspend_mode;
 }
-
-void __init tegra_pmc_init(void)
-{
-	u32 val;
-
-	tegra_pmc_parse_dt();
-
-	val = tegra_pmc_readl(PMC_CTRL);
-	if (tegra_pmc_invert_interrupt)
-		val |= PMC_CTRL_INTR_LOW;
-	else
-		val &= ~PMC_CTRL_INTR_LOW;
-	tegra_pmc_writel(val, PMC_CTRL);
-}
diff --git a/arch/arm/mach-tegra/pmc.h b/arch/arm/mach-tegra/pmc.h
index 549f8c7..4d5f8f3 100644
--- a/arch/arm/mach-tegra/pmc.h
+++ b/arch/arm/mach-tegra/pmc.h
@@ -39,6 +39,7 @@ bool tegra_pmc_cpu_is_powered(int cpuid);
 int tegra_pmc_cpu_power_on(int cpuid);
 int tegra_pmc_cpu_remove_clamping(int cpuid);
 
+void tegra_pmc_init_irq(void);
 void tegra_pmc_init(void);
 
 #endif
diff --git a/arch/arm/mach-tegra/tegra.c b/arch/arm/mach-tegra/tegra.c
index 5b86055..2e21928 100644
--- a/arch/arm/mach-tegra/tegra.c
+++ b/arch/arm/mach-tegra/tegra.c
@@ -16,7 +16,6 @@
  *
  */
 
-#include <linux/clocksource.h>
 #include <linux/kernel.h>
 #include <linux/init.h>
 #include <linux/platform_device.h>
@@ -44,6 +43,7 @@
 #include "common.h"
 #include "fuse.h"
 #include "iomap.h"
+#include "pmc.h"
 
 static void __init tegra_dt_init(void)
 {
@@ -51,6 +51,8 @@ static void __init tegra_dt_init(void)
 	struct soc_device *soc_dev;
 	struct device *parent = NULL;
 
+	tegra_pmc_init();
+
 	tegra_clocks_apply_init_table();
 
 	soc_dev_attr = kzalloc(sizeof(*soc_dev_attr), GFP_KERNEL);
@@ -119,7 +121,6 @@ DT_MACHINE_START(TEGRA_DT, "NVIDIA Tegra SoC (Flattened Device Tree)")
 	.smp		= smp_ops(tegra_smp_ops),
 	.init_early	= tegra_init_early,
 	.init_irq	= tegra_dt_init_irq,
-	.init_time	= clocksource_of_init,
 	.init_machine	= tegra_dt_init,
 	.init_late	= tegra_dt_init_late,
 	.restart	= tegra_assert_system_reset,
-- 
1.7.2.5

Re: [PATCH RFC v2 13/16] ARM: tegra: split tegra_pmc_init() in two

[Stephen Warren]

On 08/27/2013 03:28 PM, Sebastian Hesselbarth wrote:
> From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> Tegra's board file currently initializes clocks much earlier than those
> for most other ARM SoCs. The reason is:
> 
> * The PMC HW block is involved in the path of some interrupts (i.e. it
> inverts, or not, the IRQ input pin dedicated to the PMIC).
> 
> * So, that part of the PMC must be initialized early so that the IRQ
> polarity is correct.
> 
> * The PMC initialization is currently monolithic, and the PMC has some
> clock inputs, so the init routine ends up calling of_clk_get_by_name(),
> and hence clocks must be set up early too.
> 
> In order to defer clock initialization to the more typical location,
> split out the portions of tegra_pmc_init() that are truly IRQ-related
> into a separate tegra_pmc_init_irq(), which can be called from the
> machine descriptor's .init_irq() function, and defer the rest until
> the machine descriptor's .init_machine() function. With arch/arm calling
> of_clk_init(NULL) from time_init() this also allows the removal of
> .init_time() hook.
> 
> Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
> Changelog:
> v1->v2:
> - took Stephen Warren's patch provided to separate Tegra's pmc_init
> - sqashed in .init_time removal and reworded patch text

I think it'd be better to keep the 2 patches separate so the two logical
changes are in different patches. I suppose it isn't a huge deal though.

Either way, on this patch, your S-o-b line is missing above.

Re: [PATCH RFC v2 13/16] ARM: tegra: split tegra_pmc_init() in two

[Sebastian Hesselbarth]

On 08/27/13 23:59, Stephen Warren wrote:
> On 08/27/2013 03:28 PM, Sebastian Hesselbarth wrote:
>> From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>
>> Tegra's board file currently initializes clocks much earlier than those
>> for most other ARM SoCs. The reason is:
>>
>> * The PMC HW block is involved in the path of some interrupts (i.e. it
>> inverts, or not, the IRQ input pin dedicated to the PMIC).
>>
>> * So, that part of the PMC must be initialized early so that the IRQ
>> polarity is correct.
>>
>> * The PMC initialization is currently monolithic, and the PMC has some
>> clock inputs, so the init routine ends up calling of_clk_get_by_name(),
>> and hence clocks must be set up early too.
>>
>> In order to defer clock initialization to the more typical location,
>> split out the portions of tegra_pmc_init() that are truly IRQ-related
>> into a separate tegra_pmc_init_irq(), which can be called from the
>> machine descriptor's .init_irq() function, and defer the rest until
>> the machine descriptor's .init_machine() function. With arch/arm calling
>> of_clk_init(NULL) from time_init() this also allows the removal of
>> .init_time() hook.
>>
>> Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> ---
>> Changelog:
>> v1->v2:
>> - took Stephen Warren's patch provided to separate Tegra's pmc_init
>> - sqashed in .init_time removal and reworded patch text
>
> I think it'd be better to keep the 2 patches separate so the two logical
> changes are in different patches. I suppose it isn't a huge deal though.
>
> Either way, on this patch, your S-o-b line is missing above.

Stephen,

I was already wondering here, if I should separate the patches into 
preparation and actual removal. I put that question into the cover
letter and removed it from this patch text ;)

As you suggest to use preparation/removal, I will take that approach
for the final patch set. Will take you original unmodified patch then.

Sebastian

Re: [PATCH RFC v2 01/16] ARM: call clk_of_init from time_init

[Sören Brinkmann]

On Tue, Aug 27, 2013 at 11:27:55PM +0200, Sebastian Hesselbarth wrote:
> Most DT ARM machs require common clock providers initialized before timers.
> Currently, arch/arm machs use .init_time to call clk_of_init right before
> clocksource_of_init. This prevents to remove that hook and use the default
> hook instead. clk_of_init is safe to call for non-DT platforms, so add
> the call to ARM arch time_init by default. While at it, also reorder includes
> alphabetically.
> 
> Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> ---
> Changelog:
> v1->v2:
> - reorder includes alphabetically
> 
> Cc: Russell King <linux@arm.linux.org.uk>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: linux-tegra@vger.kernel.org
> Cc: kernel@stlinux.com
> Cc: linux-samsung-soc@vger.kernel.org
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  arch/arm/kernel/time.c |   24 ++++++++++++++----------
>  1 files changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/arm/kernel/time.c b/arch/arm/kernel/time.c
> index 98aee32..dd1028e 100644
> --- a/arch/arm/kernel/time.c
> +++ b/arch/arm/kernel/time.c
> @@ -11,25 +11,26 @@
[ ... ]
>  void __init time_init(void)
>  {
> +	/* initalize common clocks before timers */
> +	of_clk_init(NULL);
> +
>  	if (machine_desc->init_time)
>  		machine_desc->init_time();
>  	else

This forces zynq to move some initialization our clock code relies on to
init_irq(). Also, the current code already takes an approach of
doing either common init or machine specific init.
I think it might be better to move the call to of_clk_init() down into
the else branch of the if-else.
Though, this probably contradicts the purpose of the whole series.

	Sören

Re: [PATCH RFC v2 01/16] ARM: call clk_of_init from time_init

[Sebastian Hesselbarth]

On 08/28/13 00:19, Sören Brinkmann wrote:
> On Tue, Aug 27, 2013 at 11:27:55PM +0200, Sebastian Hesselbarth wrote:
>> Most DT ARM machs require common clock providers initialized before timers.
>> Currently, arch/arm machs use .init_time to call clk_of_init right before
>> clocksource_of_init. This prevents to remove that hook and use the default
>> hook instead. clk_of_init is safe to call for non-DT platforms, so add
>> the call to ARM arch time_init by default. While at it, also reorder includes
>> alphabetically.
>>
>> Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
>> ---
>> Changelog:
>> v1->v2:
>> - reorder includes alphabetically
>>
>> Cc: Russell King <linux@arm.linux.org.uk>
>> Cc: Arnd Bergmann <arnd@arndb.de>
>> Cc: linux-tegra@vger.kernel.org
>> Cc: kernel@stlinux.com
>> Cc: linux-samsung-soc@vger.kernel.org
>> Cc: linux-arm-kernel@lists.infradead.org
>> Cc: linux-kernel@vger.kernel.org
>> ---
>>   arch/arm/kernel/time.c |   24 ++++++++++++++----------
>>   1 files changed, 14 insertions(+), 10 deletions(-)
>>
>> diff --git a/arch/arm/kernel/time.c b/arch/arm/kernel/time.c
>> index 98aee32..dd1028e 100644
>> --- a/arch/arm/kernel/time.c
>> +++ b/arch/arm/kernel/time.c
>> @@ -11,25 +11,26 @@
> [ ... ]
>>   void __init time_init(void)
>>   {
>> +	/* initalize common clocks before timers */
>> +	of_clk_init(NULL);
>> +
>>   	if (machine_desc->init_time)
>>   		machine_desc->init_time();
>>   	else
>
> This forces zynq to move some initialization our clock code relies on to
> init_irq(). Also, the current code already takes an approach of
> doing either common init or machine specific init.

Soeren,

you know that patch 16/16 takes care of zynq's clock init?

It's your own patch you provided from the last RFC. Looking at it, it
moves  zynq_sclr_init() to .init_irq and removes the call to
of_clk_init() from zynq_clock_init() which is called by
zynq_sclr_init().

Isn't that solving the above issues for mach-zynq?

> I think it might be better to move the call to of_clk_init() down into
> the else branch of the if-else.

Possibly, yes. But we could also unconditionally call of_clk_init() at
the beginning of time_init() and call clocksource_of_init() at the end.
That will make .init_time() to some fixup hook between initialization of 
clocks and timers.

> Though, this probably contradicts the purpose of the whole series.

Hmm, the purpose was to allow most platforms to remove a custom
.init_time hook only calling of_clk_init() and clocksource_of_init().

If we move of_clk_init() above also in the else-branch the purpose
is still met.

Sebastian

Re: [PATCH RFC v2 01/16] ARM: call clk_of_init from time_init

[Sören Brinkmann]

On Wed, Aug 28, 2013 at 12:58:39AM +0200, Sebastian Hesselbarth wrote:
> On 08/28/13 00:19, Sören Brinkmann wrote:
> >On Tue, Aug 27, 2013 at 11:27:55PM +0200, Sebastian Hesselbarth wrote:
> >>Most DT ARM machs require common clock providers initialized before timers.
> >>Currently, arch/arm machs use .init_time to call clk_of_init right before
> >>clocksource_of_init. This prevents to remove that hook and use the default
> >>hook instead. clk_of_init is safe to call for non-DT platforms, so add
> >>the call to ARM arch time_init by default. While at it, also reorder includes
> >>alphabetically.
> >>
> >>Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> >>---
> >>Changelog:
> >>v1->v2:
> >>- reorder includes alphabetically
> >>
> >>Cc: Russell King <linux@arm.linux.org.uk>
> >>Cc: Arnd Bergmann <arnd@arndb.de>
> >>Cc: linux-tegra@vger.kernel.org
> >>Cc: kernel@stlinux.com
> >>Cc: linux-samsung-soc@vger.kernel.org
> >>Cc: linux-arm-kernel@lists.infradead.org
> >>Cc: linux-kernel@vger.kernel.org
> >>---
> >>  arch/arm/kernel/time.c |   24 ++++++++++++++----------
> >>  1 files changed, 14 insertions(+), 10 deletions(-)
> >>
> >>diff --git a/arch/arm/kernel/time.c b/arch/arm/kernel/time.c
> >>index 98aee32..dd1028e 100644
> >>--- a/arch/arm/kernel/time.c
> >>+++ b/arch/arm/kernel/time.c
> >>@@ -11,25 +11,26 @@
> >[ ... ]
> >>  void __init time_init(void)
> >>  {
> >>+	/* initalize common clocks before timers */
> >>+	of_clk_init(NULL);
> >>+
> >>  	if (machine_desc->init_time)
> >>  		machine_desc->init_time();
> >>  	else
> >
> >This forces zynq to move some initialization our clock code relies on to
> >init_irq(). Also, the current code already takes an approach of
> >doing either common init or machine specific init.
> 
> Soeren,
> 
> you know that patch 16/16 takes care of zynq's clock init?
> 
> It's your own patch you provided from the last RFC. Looking at it, it
> moves  zynq_sclr_init() to .init_irq and removes the call to
> of_clk_init() from zynq_clock_init() which is called by
> zynq_sclr_init().
> 
> Isn't that solving the above issues for mach-zynq?

Yes, I know. This alternative approach came to me after I sent my patch
and took a closer look at init_irq(). As you said, we move our problem
just to an earlier boot stage. Which wouldn't be true if a strict
if-else would allow us to explicitly call everything in the right order
- which init_irq by the way does.
I mentioned it in an email in the original thread yesterday. But since
there is a v2 now, I just thought to bring it up here now.

> 
> >I think it might be better to move the call to of_clk_init() down into
> >the else branch of the if-else.
> 
> Possibly, yes. But we could also unconditionally call of_clk_init() at
> the beginning of time_init() and call clocksource_of_init() at the end.
> That will make .init_time() to some fixup hook between
> initialization of clocks and timers.

Right, the question is what is the common case? What do SOCs use the
custom init_time() hook for?

If SOCs use it for things additionally to clock init where 
execution order doesn't matter, your patch works perfectly well and is
the preferred solution.

If they use it just to call of_clk_init() and of_clocksource_init(),
those hooks can go away and it would work in the else branch or outside,
equally well.

If they use it like Zynq, to do some required SOC specific init which must
be done before clock init, it would make more sense to have it in the
else branch. That way the SOC intentionally replaces init_time()
with its own implementation and has to take care of calling everything in the
right order.

Another approach might be to move the custom init_time() hook before the
call to of_clk_init().

As I said, it depends on how this hook is used.

	Sören

Re: [PATCH RFC v2 01/16] ARM: call clk_of_init from time_init

[Arnd Bergmann]

On Wednesday 28 August 2013, Sebastian Hesselbarth wrote:
> >
> > This forces zynq to move some initialization our clock code relies on to
> > init_irq(). Also, the current code already takes an approach of
> > doing either common init or machine specific init.
> 
> Soeren,
> 
> you know that patch 16/16 takes care of zynq's clock init?
> 
> It's your own patch you provided from the last RFC. Looking at it, it
> moves  zynq_sclr_init() to .init_irq and removes the call to
> of_clk_init() from zynq_clock_init() which is called by
> zynq_sclr_init().
> 
> Isn't that solving the above issues for mach-zynq?

Please be careful with the patch ordering here. The patch series should
be bisectable, i.e. no patch should ever knowingly break any of the
platforms, with the fix getting added in a later patch.

You should be able to do that by cleaning up all platforms to not
rely on ordering first, then add this patch, and finally remove
the other calls.

	Arnd

Re: [PATCH RFC v2 13/16] ARM: tegra: split tegra_pmc_init() in two

[Stephen Warren]

On 08/27/2013 03:28 PM, Sebastian Hesselbarth wrote:
> From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> Tegra's board file currently initializes clocks much earlier than those
> for most other ARM SoCs. The reason is:
> 
> * The PMC HW block is involved in the path of some interrupts (i.e. it
> inverts, or not, the IRQ input pin dedicated to the PMIC).
> 
> * So, that part of the PMC must be initialized early so that the IRQ
> polarity is correct.
> 
> * The PMC initialization is currently monolithic, and the PMC has some
> clock inputs, so the init routine ends up calling of_clk_get_by_name(),
> and hence clocks must be set up early too.
> 
> In order to defer clock initialization to the more typical location,
> split out the portions of tegra_pmc_init() that are truly IRQ-related
> into a separate tegra_pmc_init_irq(), which can be called from the
> machine descriptor's .init_irq() function, and defer the rest until
> the machine descriptor's .init_machine() function. With arch/arm calling
> of_clk_init(NULL) from time_init() this also allows the removal of
> .init_time() hook.

Sebastian, I assume you're targeting v3.13 or later for this patch
series? If so, it might be a good idea if I apply this patch myself to
the Tegra tree so that I can base any future Tegra patches on top of it
to avoid any possible conflicts. I can put this patch first in the
cleanup branch so you can merge it into whatever tree you use for the
rest of this series. Does that work for you? If so, let me know. Thanks.

Re: [PATCH RFC v2 13/16] ARM: tegra: split tegra_pmc_init() in two

[Sebastian Hesselbarth]

On 09/11/2013 09:56 PM, Stephen Warren wrote:
> On 08/27/2013 03:28 PM, Sebastian Hesselbarth wrote:
>> From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>
>> Tegra's board file currently initializes clocks much earlier than those
>> for most other ARM SoCs. The reason is:
>>
>> * The PMC HW block is involved in the path of some interrupts (i.e. it
>> inverts, or not, the IRQ input pin dedicated to the PMIC).
>>
>> * So, that part of the PMC must be initialized early so that the IRQ
>> polarity is correct.
>>
>> * The PMC initialization is currently monolithic, and the PMC has some
>> clock inputs, so the init routine ends up calling of_clk_get_by_name(),
>> and hence clocks must be set up early too.
>>
>> In order to defer clock initialization to the more typical location,
>> split out the portions of tegra_pmc_init() that are truly IRQ-related
>> into a separate tegra_pmc_init_irq(), which can be called from the
>> machine descriptor's .init_irq() function, and defer the rest until
>> the machine descriptor's .init_machine() function. With arch/arm calling
>> of_clk_init(NULL) from time_init() this also allows the removal of
>> .init_time() hook.
>
> Sebastian, I assume you're targeting v3.13 or later for this patch
> series? If so, it might be a good idea if I apply this patch myself to
> the Tegra tree so that I can base any future Tegra patches on top of it
> to avoid any possible conflicts. I can put this patch first in the
> cleanup branch so you can merge it into whatever tree you use for the
> rest of this series. Does that work for you? If so, let me know. Thanks.

Yes, I was waiting for v3.12-rc1 to drop to have something stable with
the new machs inside. I haven't made up my mind who will finally take
the patches but I guess it's either arm-soc or each individual
maintainer.

Currently, the patch set includes your original patch and another one to
remove .init_time when the arch-wide default callback is available. If
you want to take it now, I can add the corresponding dependency to the
cover letter and drop the patch from my set.

If Arnd, Olof, and Mike say that current mainline head is stable with
respect to arm-soc and clk, I can also send it today or tomorrow.

Sebastian

Re: [PATCH RFC v2 13/16] ARM: tegra: split tegra_pmc_init() in two

[Stephen Warren]

On 09/12/2013 12:21 AM, Sebastian Hesselbarth wrote:
> On 09/11/2013 09:56 PM, Stephen Warren wrote:
>> On 08/27/2013 03:28 PM, Sebastian Hesselbarth wrote:
>>> From: Stephen Warren <swarren@nvidia.com>
>>>
>>> Tegra's board file currently initializes clocks much earlier than those
>>> for most other ARM SoCs. The reason is:
>>>
>>> * The PMC HW block is involved in the path of some interrupts (i.e. it
>>> inverts, or not, the IRQ input pin dedicated to the PMIC).
>>>
>>> * So, that part of the PMC must be initialized early so that the IRQ
>>> polarity is correct.
>>>
>>> * The PMC initialization is currently monolithic, and the PMC has some
>>> clock inputs, so the init routine ends up calling of_clk_get_by_name(),
>>> and hence clocks must be set up early too.
>>>
>>> In order to defer clock initialization to the more typical location,
>>> split out the portions of tegra_pmc_init() that are truly IRQ-related
>>> into a separate tegra_pmc_init_irq(), which can be called from the
>>> machine descriptor's .init_irq() function, and defer the rest until
>>> the machine descriptor's .init_machine() function. With arch/arm calling
>>> of_clk_init(NULL) from time_init() this also allows the removal of
>>> .init_time() hook.
>>
>> Sebastian, I assume you're targeting v3.13 or later for this patch
>> series? If so, it might be a good idea if I apply this patch myself to
>> the Tegra tree so that I can base any future Tegra patches on top of it
>> to avoid any possible conflicts. I can put this patch first in the
>> cleanup branch so you can merge it into whatever tree you use for the
>> rest of this series. Does that work for you? If so, let me know. Thanks.
> 
> Yes, I was waiting for v3.12-rc1 to drop to have something stable with
> the new machs inside. I haven't made up my mind who will finally take
> the patches but I guess it's either arm-soc or each individual
> maintainer.

OK, I'll take this one patch through the Tegra tree, making sure it's
first and based on v3.12-rc1, and I'll create a tag for you to pull into
your branch as needed.

> Currently, the patch set includes your original patch and another one to
> remove .init_time when the arch-wide default callback is available. If
> you want to take it now, I can add the corresponding dependency to the
> cover letter and drop the patch from my set.

Sounds good. From memory, that second patch you mentioned can go through
any tree you want; I don't think it will cause any conflicts since it's
tiny.