Patchwork [RFC,17/17] clk: zynq: remove call to of_clk_init

login
register
mail settings
Submitter Soren Brinkmann
Date Aug. 23, 2013, 5:19 p.m.
Message ID <bfd173dc-3be5-421c-b7f7-af181bfb7032@DB8EHSMHS026.ehs.local>
Download mbox | patch
Permalink /patch/269500/
State New
Headers show

Comments

Soren Brinkmann - Aug. 23, 2013, 5:19 p.m.
Hi Sebastian, Steffen,

On Fri, Aug 23, 2013 at 11:30:18AM +0200, Sebastian Hesselbarth wrote:
> On 08/23/13 02:59, Sören Brinkmann wrote:
> >On Thu, Aug 22, 2013 at 05:26:47PM -0700, Sören Brinkmann wrote:
> >>On Tue, Aug 20, 2013 at 04:04:31AM +0200, Sebastian Hesselbarth wrote:
> >>>With arch/arm calling of_clk_init(NULL) from time_init(), we can now
> >>>remove it from corresponding drivers/clk code.
> >>
> >>I think that would break Zynq.
> >>If I see this correctly you call of_clk_init() from common code,
> >>_before_ the SOC specific time init function is called.
> >>The problem is, that we have code setting up a global pointer which is
> >>required by zynq_clk_setup() which is triggered when of_clk_init() is
> >>called.
[ ... ]
> thanks for looking into this. I also had a look at the files in
> question. Based on Steffen's proposal, I prepared a diff that should do
> the trick. It moves zynq_slcr_init() to early_init, instead of reusing
> another hook that has magic cow powers (it calls irqchip_init that zynq
> also wants sooner or later).
> 
> Also, it removes zynq_clock_init() and let zynq_clk_setup() map the
> register itself by finding the node and use of_iomap(). I realized that
> clock registers are quite separated within slcr, so you can consider
> to have your own node for the clk-provider. As Steffen is proposing
> this but mentioned incompatible DT changes, I chose that intermediate
> step above.
> 
> It would be great, if you test the diff and prepare a patch out of
> it, that I pick-up in the patch set. That way, we also have your
> Signed-off on it.

I looked into this. Looks like init_early() happens to early. I suspect
slab is missing to make zynq_slcr_init() work. So, I moved it into
init_irq(). Is there any init_call() type which is called at the correct
time?

I looked briefly into syscon and regmap, and that does actually look
promising and to really fix this mess, I guess we have to wait a little
until Steffen finishes his work on it.

To facilitate Sebastian's series I came up with the patch below.
The problem I have is, I do not really want the clkc to map the
registers. They are in the SLCR and the SLCR driver is doing it, hence
we should work with what that driver provides - which ideally would be
based on regmap and syscon, but we're not there yet. Hence I somehow
need to pass the SLCR pointer to the clkc. To avoid accessing the global
pointer directly I kept the zynq_clock_init() routine which is called
from zynq_slcr_init().

That is the best I could come up with quickly and w/o investing a lot of
time to figure out the regmap and syscon stuff, which seems to be handled
by Steffen already, anyway.
It is essentially a stripped down version of Sebastian's proposal.

	Sören

-----8<--------------------8<-------------------8<-------------------

From bb7a02dad9cc578caf1e21a1b7f45ed602676bfa Mon Sep 17 00:00:00 2001
From: Soren Brinkmann <soren.brinkmann@xilinx.com>
Date: Fri, 23 Aug 2013 09:27:11 -0700
Subject: [PATCH RFC] arm: zynq: Don't call of_clk_init()

of_clk_init() has been moved to be called from common code, therefore
remove it from Zynq's clock init routine.
Since the Zynq's clock setup routine relies on an initialized SLCR,
zynq_slcr_init() is moved to init_irq() (note: it must be before
init_time() but after slab is available, hence init_early() does not
work).

Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
---
 arch/arm/mach-zynq/common.c | 9 ++++-----
 drivers/clk/zynq/clkc.c     | 4 +++-
 2 files changed, 7 insertions(+), 6 deletions(-)
Sebastian Hesselbarth - Aug. 23, 2013, 5:44 p.m.
On 08/23/13 19:19, Sören Brinkmann wrote:
> On Fri, Aug 23, 2013 at 11:30:18AM +0200, Sebastian Hesselbarth wrote:
>> On 08/23/13 02:59, Sören Brinkmann wrote:
>>> On Thu, Aug 22, 2013 at 05:26:47PM -0700, Sören Brinkmann wrote:
>>>> On Tue, Aug 20, 2013 at 04:04:31AM +0200, Sebastian Hesselbarth wrote:
>>>>> With arch/arm calling of_clk_init(NULL) from time_init(), we can now
>>>>> remove it from corresponding drivers/clk code.
>>>>
>>>> I think that would break Zynq.
>>>> If I see this correctly you call of_clk_init() from common code,
>>>> _before_ the SOC specific time init function is called.
>>>> The problem is, that we have code setting up a global pointer which is
>>>> required by zynq_clk_setup() which is triggered when of_clk_init() is
>>>> called.
> [ ... ]
>> thanks for looking into this. I also had a look at the files in
>> question. Based on Steffen's proposal, I prepared a diff that should do
>> the trick. It moves zynq_slcr_init() to early_init, instead of reusing
>> another hook that has magic cow powers (it calls irqchip_init that zynq
>> also wants sooner or later).
>>
>> Also, it removes zynq_clock_init() and let zynq_clk_setup() map the
>> register itself by finding the node and use of_iomap(). I realized that
>> clock registers are quite separated within slcr, so you can consider
>> to have your own node for the clk-provider. As Steffen is proposing
>> this but mentioned incompatible DT changes, I chose that intermediate
>> step above.
>>
>> It would be great, if you test the diff and prepare a patch out of
>> it, that I pick-up in the patch set. That way, we also have your
>> Signed-off on it.
>
> I looked into this. Looks like init_early() happens to early. I suspect
> slab is missing to make zynq_slcr_init() work. So, I moved it into
> init_irq(). Is there any init_call() type which is called at the correct
> time?

Sören,

I mistakenly assumed init_early is after mm, so of course my proposal
does not work as it should. I am fine with moving it to init_irq() until
you find the best solution (or until we have the same "mess" with
default init_irq hook).

> I looked briefly into syscon and regmap, and that does actually look
> promising and to really fix this mess, I guess we have to wait a little
> until Steffen finishes his work on it.

IIRC, both syscon and regmap will require you to have devices ready.
I haven't followed all recent discussions about early device
registration. Anyway, it will not help much in the current approach
to get rid of custom .init_timer and maybe .init_irq later.

> To facilitate Sebastian's series I came up with the patch below.
> The problem I have is, I do not really want the clkc to map the
> registers. They are in the SLCR and the SLCR driver is doing it, hence
> we should work with what that driver provides - which ideally would be
> based on regmap and syscon, but we're not there yet. Hence I somehow
> need to pass the SLCR pointer to the clkc. To avoid accessing the global
> pointer directly I kept the zynq_clock_init() routine which is called
> from zynq_slcr_init().

For this patch set I'd be fine with the proposal below. For the short
run, you could consider to hide register accesses to slcr by providing
zynq_slcr_readl/writel instead of passing just the base address.

But again, that will require either custom .init_time or .init_irq
to set up slcr before clocks.

> That is the best I could come up with quickly and w/o investing a lot of
> time to figure out the regmap and syscon stuff, which seems to be handled
> by Steffen already, anyway.
> It is essentially a stripped down version of Sebastian's proposal.

If there are no general objections, I take that one for the real patch
set.

Sebastian
Steffen Trumtrar - Aug. 23, 2013, 11:22 p.m.
On Fri, Aug 23, 2013 at 07:44:03PM +0200, Sebastian Hesselbarth wrote:
> On 08/23/13 19:19, Sören Brinkmann wrote:
> >On Fri, Aug 23, 2013 at 11:30:18AM +0200, Sebastian Hesselbarth wrote:
> >>On 08/23/13 02:59, Sören Brinkmann wrote:
> >>>On Thu, Aug 22, 2013 at 05:26:47PM -0700, Sören Brinkmann wrote:
> >>>>On Tue, Aug 20, 2013 at 04:04:31AM +0200, Sebastian Hesselbarth wrote:
> >>>>>With arch/arm calling of_clk_init(NULL) from time_init(), we can now
> >>>>>remove it from corresponding drivers/clk code.
> >>>>
> >>>>I think that would break Zynq.
> >>>>If I see this correctly you call of_clk_init() from common code,
> >>>>_before_ the SOC specific time init function is called.
> >>>>The problem is, that we have code setting up a global pointer which is
> >>>>required by zynq_clk_setup() which is triggered when of_clk_init() is
> >>>>called.
> >[ ... ]
> >>thanks for looking into this. I also had a look at the files in
> >>question. Based on Steffen's proposal, I prepared a diff that should do
> >>the trick. It moves zynq_slcr_init() to early_init, instead of reusing
> >>another hook that has magic cow powers (it calls irqchip_init that zynq
> >>also wants sooner or later).
> >>
> >>Also, it removes zynq_clock_init() and let zynq_clk_setup() map the
> >>register itself by finding the node and use of_iomap(). I realized that
> >>clock registers are quite separated within slcr, so you can consider
> >>to have your own node for the clk-provider. As Steffen is proposing
> >>this but mentioned incompatible DT changes, I chose that intermediate
> >>step above.
> >>
> >>It would be great, if you test the diff and prepare a patch out of
> >>it, that I pick-up in the patch set. That way, we also have your
> >>Signed-off on it.
> >
> >I looked into this. Looks like init_early() happens to early. I suspect
> >slab is missing to make zynq_slcr_init() work. So, I moved it into
> >init_irq(). Is there any init_call() type which is called at the correct
> >time?
> 
> Sören,
> 
> I mistakenly assumed init_early is after mm, so of course my proposal
> does not work as it should. I am fine with moving it to init_irq() until
> you find the best solution (or until we have the same "mess" with
> default init_irq hook).
> 
> >I looked briefly into syscon and regmap, and that does actually look
> >promising and to really fix this mess, I guess we have to wait a little
> >until Steffen finishes his work on it.
> 
> IIRC, both syscon and regmap will require you to have devices ready.
> I haven't followed all recent discussions about early device
> registration. Anyway, it will not help much in the current approach
> to get rid of custom .init_timer and maybe .init_irq later.
> 

Yes. The syscon driver can not be used that early. It only makes sense
for later stages. Therefore the slcr has to be mapped early to unlock
and than remapped later. But with the current drivers, it is not essential
to have the slcr as a seperate driver.

> >To facilitate Sebastian's series I came up with the patch below.
> >The problem I have is, I do not really want the clkc to map the
> >registers. They are in the SLCR and the SLCR driver is doing it, hence
> >we should work with what that driver provides - which ideally would be
> >based on regmap and syscon, but we're not there yet. Hence I somehow
> >need to pass the SLCR pointer to the clkc. To avoid accessing the global
> >pointer directly I kept the zynq_clock_init() routine which is called
> >from zynq_slcr_init().
> 
> For this patch set I'd be fine with the proposal below. For the short
> run, you could consider to hide register accesses to slcr by providing
> zynq_slcr_readl/writel instead of passing just the base address.
> 
> But again, that will require either custom .init_time or .init_irq
> to set up slcr before clocks.
> 
> >That is the best I could come up with quickly and w/o investing a lot of
> >time to figure out the regmap and syscon stuff, which seems to be handled
> >by Steffen already, anyway.
> >It is essentially a stripped down version of Sebastian's proposal.
> 
> If there are no general objections, I take that one for the real patch
> set.

I haven't tested this, but I hope Sören did. For the time being, I am
okay with it.

Regards,
Steffen
Soren Brinkmann - Aug. 26, 2013, 3:20 p.m.
On Fri, Aug 23, 2013 at 07:44:03PM +0200, Sebastian Hesselbarth wrote:
> On 08/23/13 19:19, Sören Brinkmann wrote:
> >On Fri, Aug 23, 2013 at 11:30:18AM +0200, Sebastian Hesselbarth wrote:
> >>On 08/23/13 02:59, Sören Brinkmann wrote:
> >>>On Thu, Aug 22, 2013 at 05:26:47PM -0700, Sören Brinkmann wrote:
> >>>>On Tue, Aug 20, 2013 at 04:04:31AM +0200, Sebastian Hesselbarth wrote:
> >>>>>With arch/arm calling of_clk_init(NULL) from time_init(), we can now
> >>>>>remove it from corresponding drivers/clk code.
> >>>>
> >>>>I think that would break Zynq.
> >>>>If I see this correctly you call of_clk_init() from common code,
> >>>>_before_ the SOC specific time init function is called.
> >>>>The problem is, that we have code setting up a global pointer which is
> >>>>required by zynq_clk_setup() which is triggered when of_clk_init() is
> >>>>called.
> >[ ... ]
> >>thanks for looking into this. I also had a look at the files in
> >>question. Based on Steffen's proposal, I prepared a diff that should do
> >>the trick. It moves zynq_slcr_init() to early_init, instead of reusing
> >>another hook that has magic cow powers (it calls irqchip_init that zynq
> >>also wants sooner or later).
> >>
> >>Also, it removes zynq_clock_init() and let zynq_clk_setup() map the
> >>register itself by finding the node and use of_iomap(). I realized that
> >>clock registers are quite separated within slcr, so you can consider
> >>to have your own node for the clk-provider. As Steffen is proposing
> >>this but mentioned incompatible DT changes, I chose that intermediate
> >>step above.
> >>
> >>It would be great, if you test the diff and prepare a patch out of
> >>it, that I pick-up in the patch set. That way, we also have your
> >>Signed-off on it.
> >
> >I looked into this. Looks like init_early() happens to early. I suspect
> >slab is missing to make zynq_slcr_init() work. So, I moved it into
> >init_irq(). Is there any init_call() type which is called at the correct
> >time?
> 
> Sören,
> 
> I mistakenly assumed init_early is after mm, so of course my proposal
> does not work as it should. I am fine with moving it to init_irq() until
> you find the best solution (or until we have the same "mess" with
> default init_irq hook).

Looking at these two hooks. If the SOC provides init_irq(), the common
code does nothing, but calling it. Ergo, the SOC is now responsible for
otherwise commonly called code like of_irq_init().

It's probably an idea to design the common init_time() function the same
way. That way SOCs that don't need any custom init at that stage get
everything for free. And SOCs like Zynq have to still call of_clk_init()
manually, but can ensure that dependencies like our SLCR init are
satisfied before calling it.

Just an idea, not sure if it makes sense since I didn't look beyond Zynq
too much on this.

	Sören

Patch

diff --git a/arch/arm/mach-zynq/common.c b/arch/arm/mach-zynq/common.c
index 5f25256..f28046e 100644
--- a/arch/arm/mach-zynq/common.c
+++ b/arch/arm/mach-zynq/common.c
@@ -19,10 +19,9 @@ 
 #include <linux/cpumask.h>
 #include <linux/platform_device.h>
 #include <linux/clk.h>
-#include <linux/clk/zynq.h>
-#include <linux/clocksource.h>
 #include <linux/of_address.h>
 #include <linux/of_irq.h>
+#include <linux/irqchip.h>
 #include <linux/of_platform.h>
 #include <linux/of.h>
 
@@ -58,10 +57,10 @@  static void __init zynq_init_machine(void)
 	of_platform_bus_probe(NULL, zynq_of_bus_ids, NULL);
 }
 
-static void __init zynq_timer_init(void)
+static void __init zynq_init_irq(void)
 {
+	irqchip_init();
 	zynq_slcr_init();
-	clocksource_of_init();
 }
 
 static struct map_desc zynq_cortex_a9_scu_map __initdata = {
@@ -104,8 +103,8 @@  static const char * const zynq_dt_match[] = {
 DT_MACHINE_START(XILINX_EP107, "Xilinx Zynq Platform")
 	.smp		= smp_ops(zynq_smp_ops),
 	.map_io		= zynq_map_io,
+	.init_irq	= zynq_init_irq,
 	.init_machine	= zynq_init_machine,
-	.init_time	= zynq_timer_init,
 	.dt_compat	= zynq_dt_match,
 	.restart	= zynq_system_reset,
 MACHINE_END
diff --git a/drivers/clk/zynq/clkc.c b/drivers/clk/zynq/clkc.c
index 089d3e3..53b851e 100644
--- a/drivers/clk/zynq/clkc.c
+++ b/drivers/clk/zynq/clkc.c
@@ -206,6 +206,9 @@  static void __init zynq_clk_setup(struct device_node *np)
 
 	pr_info("Zynq clock init\n");
 
+	if (WARN_ON(!zynq_slcr_base_priv))
+		return;
+
 	/* get clock output names from DT */
 	for (i = 0; i < clk_max; i++) {
 		if (of_property_read_string_index(np, "clock-output-names",
@@ -532,5 +535,4 @@  CLK_OF_DECLARE(zynq_clkc, "xlnx,ps7-clkc", zynq_clk_setup);
 void __init zynq_clock_init(void __iomem *slcr_base)
 {
 	zynq_slcr_base_priv = slcr_base;
-	of_clk_init(NULL);
 }