[permalink] [raw]

Subject: [RFC 12/17] ARM: socfpga: remove custom .init_time hook

2013-08-20 02:07:16

by Sebastian Hesselbarth

[permalink] [raw]

2013-08-20 02:08:37

by Sebastian Hesselbarth

[permalink] [raw]

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.
>>
>> Let me try to illustrate the current call graph:
>>
>> time_init()
>> zynq_timer_init() // this machines init_time()
>> zynq_slcr_init() // setup System Level Control Registers including a global pointer
>> zynq_clock_init()
>> of_clk_init()
>> zynq_clk_setup() // requires pointer setup in zynq_slcr_init()
>> ...
>>
>> IIUC, your series would change this to:
>> time_init()
>> of_clk_init()
>> zynq_clk_setup() // SLCR pointer is not setup/NULL
>> ...
>> zynq_timer_init()
>> zynq_slcr_init() // now the pointer becomes valid
>
> I guess we could move zynq_slcr_init() into init_irq(). I'll give that a
> shot tomorrow.

Sören,

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.

Sebastian

Attachments:

arm-of_clk_init-zynq-proposal.diff (3.04 kB)

2013-08-23 10:06:04

by Gregory CLEMENT

[permalink] [raw]

Subject: Re: [RFC 08/17] ARM: mvebu: remove custom .init_time hook

Hi Sebastian,

On 20/08/2013 04:04, Sebastian Hesselbarth wrote:
> With arch/arm calling of_clk_init(NULL) from time_init(), we can now
> remove custom .init_time hooks.
>

As a feared it won't work on Armada XP. You moved coherency_init(), and
l2x0_of_init() in the init_machine hook. However init_machine is an
arch_initcall, and as all the initcall it is called at the end of the
do_basic_setup() which is called after smp_prepare_cpus(). The issue is
that smp_prepare_cpus need coherency (and I think L2 cache) already enable.

Regards,

> Signed-off-by: Sebastian Hesselbarth <[email protected]>
> ---
> Notes:
> - coherency, mbus, and cache init are moved to .init_machine hook
> - time-armada-370-xp is converted to clocksource_of_init compatible init
>
> Cc: Russell King <[email protected]>
> Cc: Arnd Bergmann <[email protected]>
> Cc: Gregory Clement <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> ---
> arch/arm/mach-mvebu/armada-370-xp.c | 11 +----------
> drivers/clocksource/time-armada-370-xp.c | 7 +++----
> include/linux/time-armada-370-xp.h | 16 ----------------
> 3 files changed, 4 insertions(+), 30 deletions(-)
> delete mode 100644 include/linux/time-armada-370-xp.h
>
> diff --git a/arch/arm/mach-mvebu/armada-370-xp.c b/arch/arm/mach-mvebu/armada-370-xp.c
> index 829b573..139f572 100644
> --- a/arch/arm/mach-mvebu/armada-370-xp.c
> +++ b/arch/arm/mach-mvebu/armada-370-xp.c
> @@ -14,11 +14,9 @@
>
> #include <linux/kernel.h>
> #include <linux/init.h>
> -#include <linux/clk-provider.h>
> #include <linux/of_address.h>
> #include <linux/of_platform.h>
> #include <linux/io.h>
> -#include <linux/time-armada-370-xp.h>
> #include <linux/dma-mapping.h>
> #include <linux/mbus.h>
> #include <asm/hardware/cache-l2x0.h>
> @@ -34,19 +32,13 @@ static void __init armada_370_xp_map_io(void)
> debug_ll_io_init();
> }
>
> -static void __init armada_370_xp_timer_and_clk_init(void)
> +static void __init armada_370_xp_dt_init(void)
> {
> - of_clk_init(NULL);
> - armada_370_xp_timer_init();
> coherency_init();
> BUG_ON(mvebu_mbus_dt_init());
> #ifdef CONFIG_CACHE_L2X0
> l2x0_of_init(0, ~0UL);
> #endif
> -}
> -
> -static void __init armada_370_xp_dt_init(void)
> -{
> of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
> }
>
> @@ -59,7 +51,6 @@ DT_MACHINE_START(ARMADA_XP_DT, "Marvell Armada 370/XP (Device Tree)")
> .smp = smp_ops(armada_xp_smp_ops),
> .init_machine = armada_370_xp_dt_init,
> .map_io = armada_370_xp_map_io,
> - .init_time = armada_370_xp_timer_and_clk_init,
> .restart = mvebu_restart,
> .dt_compat = armada_370_xp_dt_compat,
> MACHINE_END
> diff --git a/drivers/clocksource/time-armada-370-xp.c b/drivers/clocksource/time-armada-370-xp.c
> index 847cab6..67904cf 100644
> --- a/drivers/clocksource/time-armada-370-xp.c
> +++ b/drivers/clocksource/time-armada-370-xp.c
> @@ -30,7 +30,6 @@
> #include <linux/module.h>
> #include <linux/sched_clock.h>
> #include <linux/percpu.h>
> -#include <linux/time-armada-370-xp.h>
>
> /*
> * Timer block registers.
> @@ -217,13 +216,11 @@ static struct notifier_block armada_370_xp_timer_cpu_nb = {
> .notifier_call = armada_370_xp_timer_cpu_notify,
> };
>
> -void __init armada_370_xp_timer_init(void)
> +void __init armada_370_xp_timer_init(struct device_node *np)
> {
> u32 u;
> - struct device_node *np;
> int res;
>
> - np = of_find_compatible_node(NULL, NULL, "marvell,armada-370-xp-timer");
> timer_base = of_iomap(np, 0);
> WARN_ON(!timer_base);
> local_base = of_iomap(np, 1);
> @@ -293,3 +290,5 @@ void __init armada_370_xp_timer_init(void)
> if (!res)
> armada_370_xp_timer_setup(this_cpu_ptr(armada_370_xp_evt));
> }
> +CLOCKSOURCE_OF_DECLARE(armada_370_xp_timer, "marvell,armada-370-xp-timer",
> + armada_370_xp_timer_init);
> diff --git a/include/linux/time-armada-370-xp.h b/include/linux/time-armada-370-xp.h
> deleted file mode 100644
> index 6fb0856..0000000
> --- a/include/linux/time-armada-370-xp.h
> +++ /dev/null
> @@ -1,16 +0,0 @@
> -/*
> - * Marvell Armada 370/XP SoC timer handling.
> - *
> - * Copyright (C) 2012 Marvell
> - *
> - * Lior Amsalem <[email protected]>
> - * Gregory CLEMENT <[email protected]>
> - * Thomas Petazzoni <[email protected]>
> - *
> - */
> -#ifndef __TIME_ARMADA_370_XPPRCMU_H
> -#define __TIME_ARMADA_370_XPPRCMU_H
> -
> -void armada_370_xp_timer_init(void);
> -
> -#endif
>

--
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

2013-08-23 10:32:37

On 08/23/2013 09:32 AM, Steffen Trumtrar wrote:
> Hi!
>
> On Thu, Aug 22, 2013 at 05:59:36PM -0700, S?ren Brinkmann wrote:
>> On Thu, Aug 22, 2013 at 05:26:47PM -0700, S?ren Brinkmann wrote:
>>> Hi Sebastian,
>>>
>>> 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.
>>>
>>> Let me try to illustrate the current call graph:
>>>
>>> time_init()
>>> zynq_timer_init() // this machines init_time()
>>> zynq_slcr_init() // setup System Level Control Registers including a global pointer
>>> zynq_clock_init()
>>> of_clk_init()
>>> zynq_clk_setup() // requires pointer setup in zynq_slcr_init()
>>> ...
>>>
>>> IIUC, your series would change this to:
>>> time_init()
>>> of_clk_init()
>>> zynq_clk_setup() // SLCR pointer is not setup/NULL
>>> ...
>>> zynq_timer_init()
>>> zynq_slcr_init() // now the pointer becomes valid
>>
>> I guess we could move zynq_slcr_init() into init_irq(). I'll give that a
>> shot tomorrow.
>>
>
> I propose getting rid of the whole global pointer and let the clkc map the
> address itself instead.
>
> Then there is no need to shuffle stuff around in the initcalls.
> I have some WIP patches (not rebased on next and not even tested with it,
> but with v3.11-rc4)
>
> The dtsi would be something like:
>
> control-register@f8000000 {

This name is incorrect - it still should be slcr (system level control registers)

> compatible = "simple-bus";

I expect that syscon compatible should be here not in the lock part because
you want to map the whole reg space.

> #address-cells = <1>;
> #size-cells = <1>;
> reg = <0xf8000000 0x1000>;
> ranges;
>
> slcr: slcr@f8000000 {

we should use different name here - lock/locks/etc.

> compatible = "xlnx,zynq-slcr", "syscon";
> reg = <0xf8000000 0x10>;
> };

>
> clkc: clkc@f8000100 {
> #clock-cells = <1>;
> compatible = "xlnx,ps7-clkc";
> reg = <0xf8000100 0x100>;
> ps-clk-frequency = <33333333>;
> xlnx,slcr = <&slcr>;

Currently there is no code which handles locks that's why I think at least
for now it is not necessary to extend binding which feature which is not used.

> clock-output-names = "armpll", "ddrpll", "iopll", "cpu_6or4x",
> "cpu_3or2x", "cpu_2x", "cpu_1x", "ddr2x", "ddr3x",
> "dci", "lqspi", "smc", "pcap", "gem0", "gem1",
> "fclk0", "fclk1", "fclk2", "fclk3", "can0", "can1",
> "sdio0", "sdio1", "uart0", "uart1", "spi0", "spi1",
> "dma", "usb0_aper", "usb1_aper", "gem0_aper",
> "gem1_aper", "sdio0_aper", "sdio1_aper",
> "spi0_aper", "spi1_aper", "can0_aper", "can1_aper",
> "i2c0_aper", "i2c1_aper", "uart0_aper", "uart1_aper",
> "gpio_aper", "lqspi_aper", "smc_aper", "swdt",
> "dbg_trc", "dbg_apb";
> };
>
> mio: pinmux@f8000700 {
> compatible = "xlnx,ps7-pinctrl";

Have you created any driver for pinmuxing stuff?

I agree with Soren - let's fix the current problem and then when Steffen has patches with syscon
we can look at them.

If there is any discussion about early syscon registration please let me know.

Thanks,
Michal

--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: http://www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform

Attachments:

signature.asc (263.00 B)
OpenPGP digital signature

2013-08-26 12:07:26

by Steffen Trumtrar

[permalink] [raw]

Subject: Re: [RFC 17/17] clk: zynq: remove call to of_clk_init

Hi Michal!

On Mon, Aug 26, 2013 at 01:15:11PM +0200, Michal Simek wrote:
> On 08/23/2013 09:32 AM, Steffen Trumtrar wrote:
> > Hi!
> >
> > On Thu, Aug 22, 2013 at 05:59:36PM -0700, S?ren Brinkmann wrote:
> >> On Thu, Aug 22, 2013 at 05:26:47PM -0700, S?ren Brinkmann wrote:
> >>> Hi Sebastian,
> >>>
> >>> 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.
> >>>
> >>> Let me try to illustrate the current call graph:
> >>>
> >>> time_init()
> >>> zynq_timer_init() // this machines init_time()
> >>> zynq_slcr_init() // setup System Level Control Registers including a global pointer
> >>> zynq_clock_init()
> >>> of_clk_init()
> >>> zynq_clk_setup() // requires pointer setup in zynq_slcr_init()
> >>> ...
> >>>
> >>> IIUC, your series would change this to:
> >>> time_init()
> >>> of_clk_init()
> >>> zynq_clk_setup() // SLCR pointer is not setup/NULL
> >>> ...
> >>> zynq_timer_init()
> >>> zynq_slcr_init() // now the pointer becomes valid
> >>
> >> I guess we could move zynq_slcr_init() into init_irq(). I'll give that a
> >> shot tomorrow.
> >>
> >
> > I propose getting rid of the whole global pointer and let the clkc map the
> > address itself instead.
> >
> > Then there is no need to shuffle stuff around in the initcalls.
> > I have some WIP patches (not rebased on next and not even tested with it,
> > but with v3.11-rc4)
> >
> > The dtsi would be something like:
> >
> > control-register@f8000000 {
>
> This name is incorrect - it still should be slcr (system level control registers)
>

Agreed. I actually already changed it back...

> > compatible = "simple-bus";
>
> I expect that syscon compatible should be here not in the lock part because
> you want to map the whole reg space.
>

and have done this...

> > #address-cells = <1>;
> > #size-cells = <1>;
> > reg = <0xf8000000 0x1000>;
> > ranges;
> >
> > slcr: slcr@f8000000 {
>
> we should use different name here - lock/locks/etc.
>

and this actually, too.

> > compatible = "xlnx,zynq-slcr", "syscon";
> > reg = <0xf8000000 0x10>;
> > };
>
>
>
> >
> > clkc: clkc@f8000100 {
> > #clock-cells = <1>;
> > compatible = "xlnx,ps7-clkc";
> > reg = <0xf8000100 0x100>;
> > ps-clk-frequency = <33333333>;
> > xlnx,slcr = <&slcr>;
>
> Currently there is no code which handles locks that's why I think at least
> for now it is not necessary to extend binding which feature which is not used.
>

Sure. You are right.

> > clock-output-names = "armpll", "ddrpll", "iopll", "cpu_6or4x",
> > "cpu_3or2x", "cpu_2x", "cpu_1x", "ddr2x", "ddr3x",
> > "dci", "lqspi", "smc", "pcap", "gem0", "gem1",
> > "fclk0", "fclk1", "fclk2", "fclk3", "can0", "can1",
> > "sdio0", "sdio1", "uart0", "uart1", "spi0", "spi1",
> > "dma", "usb0_aper", "usb1_aper", "gem0_aper",
> > "gem1_aper", "sdio0_aper", "sdio1_aper",
> > "spi0_aper", "spi1_aper", "can0_aper", "can1_aper",
> > "i2c0_aper", "i2c1_aper", "uart0_aper", "uart1_aper",
> > "gpio_aper", "lqspi_aper", "smc_aper", "swdt",
> > "dbg_trc", "dbg_apb";
> > };
> >
> > mio: pinmux@f8000700 {
> > compatible = "xlnx,ps7-pinctrl";
>
> Have you created any driver for pinmuxing stuff?
>

At the moment it is just a regmap. That means one could hammer the values directly
into /sys/kernel/debug/regmap/pinmux@...

I first opted for the pinctrl-single driver, but the MIO has three (?) extra registers
which do not map very well to the "one pin one register" setup of that driver.

> I agree with Soren - let's fix the current problem and then when Steffen has patches with syscon
> we can look at them.
>
> If there is any discussion about early syscon registration please let me know.
>

Where I'm stuck at the moment is: if I map the whole register space to the
parent node, how do I get its mapped address in the clkc?
AFAIK the phandle is pretty useless in this early stage. In the pinmuxing
case I should be able to find the registered driver for the lock via the
phandle.
If I remember correctly, there where some patches floating around for exactly
this problem. At the moment, I think the global pointer has to stay, until
those patches are mainline, if ever. Or I'm missing some obvious way of how
to get the base address of the slcr, that I can pass to the clock framework.
It doesn't need to be syscon, as the clock framework can't use it anyway.

Regards,
Steffen

--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |

2013-08-26 12:53:42

by Sebastian Hesselbarth

[permalink] [raw]

Subject: Re: [RFC 17/17] clk: zynq: remove call to of_clk_init

On 08/26/13 14:07, Steffen Trumtrar wrote:
> On Mon, Aug 26, 2013 at 01:15:11PM +0200, Michal Simek wrote:
>> I agree with Soren - let's fix the current problem and then when Steffen has patches with syscon
>> we can look at them.
>>
>> If there is any discussion about early syscon registration please let me know.
>>
>
> Where I'm stuck at the moment is: if I map the whole register space to the
> parent node, how do I get its mapped address in the clkc?

Steffen,

if slcr is such an essential part of the SoC, you can choose to provide
zynq_slcr_readl/writel callbacks. You can then use those callback in the
clock driver without knowing the base address. Also, it allows you to
hide slcr specific locking details from subsequent drivers using the
callbacks.

> AFAIK the phandle is pretty useless in this early stage. In the pinmuxing
> case I should be able to find the registered driver for the lock via the
> phandle.

About the slcr phandle, yes, it is useless. IMHO phandles should only
be used if the referenced node is not known in advance, e.g. for gpios
or clocks. As you will always reference slcr node (and only that node)
you can just use of_find_compatible_node instead. But with the callbacks
above, you will solve it without exploiting DT but common non-static
functions instead.

Sebastian

2013-08-26 15:14:37

2013-08-27 21:30:20

by Sebastian Hesselbarth

[permalink] [raw]

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 <[email protected]>
>>>
>>> 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.