| Message ID | 1368684648-20826-3-git-send-email-b35083@freescale.com |
|---|---|
| State | New |
| Headers | show |
On Thursday 16 May 2013 14:10:46 Jingchang Lu wrote: > index a402248..b9c01a1 100644 > --- a/arch/arm/mach-imx/Kconfig > +++ b/arch/arm/mach-imx/Kconfig > @@ -835,6 +835,21 @@ config SOC_IMX6SL > help > This enables support for Freescale i.MX6 SoloLite processor. > > +config SOC_MVF600 > + bool "Vybrid Family MVF600 support" > + select CPU_V7 > + select ARM_GIC > + select CLKSRC_OF > + select PINCTRL > + select PINCTRL_MVF600 > + select MVF600_PIT_TIMER > + select PL310_ERRATA_588369 if CACHE_PL310 > + select PL310_ERRATA_727915 if CACHE_PL310 > + select PL310_ERRATA_769419 if CACHE_PL310 > + > + help > + This enable support for Freescale Vybrid MVF600 processor. > + > endif Shouldn't that 'depends on ARCH_MULTI_V7'? Can you describe how much Vybrid is actually like MXC? Do you actually use most of the mach-imx code? > diff --git a/arch/arm/mach-imx/common.h b/arch/arm/mach-imx/common.h > index 208e76f..55cdba0 100644 > --- a/arch/arm/mach-imx/common.h > +++ b/arch/arm/mach-imx/common.h > @@ -70,6 +70,7 @@ extern int mx51_clocks_init_dt(void); > extern int mx53_clocks_init_dt(void); > extern int mx6q_clocks_init(void); > extern int imx6sl_clocks_init(void); > +extern int mvf600_clocks_init(void); > extern struct platform_device *mxc_register_gpio(char *name, int id, > resource_size_t iobase, resource_size_t iosize, int irq, int irq_high); > extern void mxc_set_cpu_type(unsigned int type); This hunk belongs into the first patch, right? Actually I think you should move that driver to drivers/clk and use of_clk_init(NULL) to initialize it. > +static void __init mvf600_init_machine(void) > +{ > + mxc_arch_reset_init_dt(); I don't see the mxc_arch_reset_init_dt function in 3.10-rc1. Where does it get introduced? It would be nice if we could integrate that into the watchdog driver, so that driver just registers a pm_restart function when it gets loaded. It is a rather small driver, so it would not hurt to always load it. > + of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL); > +} It would be nicer if you could just use the default init_machine. > +static void __init mvf600_init_irq(void) > +{ > + struct device_node *np; > + void __iomem *mscm_base; > + int i; > + > + l2x0_of_init(0, ~0UL); > + > + np = of_find_compatible_node(NULL, NULL, "fsl,mvf600-mscm"); > + mscm_base = of_iomap(np, 0); > + if (!mscm_base) > + return; > + > + /* route all shared peripheral interrupts to CP0 */ > + for (i = 0; i < 111; i++) > + __raw_writew(1, mscm_base + 0x880 + 2 * i); > + > + iounmap(mscm_base); > + > + irqchip_init(); > +} What is the mscm? Shouldn't the boot loader have set this up correctly? If you can remove that code from the kernel, you can use the default irqchip_init call. > +static void __init mvf600_init_time(void) > +{ > + mvf600_clocks_init(); > + clocksource_of_init(); > +} I would like to call of_clk_init(NULL) unconditionally on all machines in 3.11, so this function could also go away. > +static const char *mvf600_dt_compat[] __initdata = { > + "fsl,mvf600", > + NULL, > +}; > + > +DT_MACHINE_START(VYBRID_VF6XX, "Freescale Vybrid MVF600 (Device Tree)") > + .init_irq = mvf600_init_irq, > + .init_time = mvf600_init_time, > + .init_machine = mvf600_init_machine, > + .dt_compat = mvf600_dt_compat, > + .restart = mxc_restart, > +MACHINE_END If we can do all of the above, we can actually remove the entire machine descriptor here, since all its members are NULL. Arnd
Arnd, Allow me help respond a little bit. On Thu, May 16, 2013 at 12:29:28PM +0200, Arnd Bergmann wrote: > On Thursday 16 May 2013 14:10:46 Jingchang Lu wrote: > > > index a402248..b9c01a1 100644 > > --- a/arch/arm/mach-imx/Kconfig > > +++ b/arch/arm/mach-imx/Kconfig > > @@ -835,6 +835,21 @@ config SOC_IMX6SL > > help > > This enables support for Freescale i.MX6 SoloLite processor. > > > > +config SOC_MVF600 > > + bool "Vybrid Family MVF600 support" > > + select CPU_V7 > > + select ARM_GIC > > + select CLKSRC_OF > > + select PINCTRL > > + select PINCTRL_MVF600 > > + select MVF600_PIT_TIMER > > + select PL310_ERRATA_588369 if CACHE_PL310 > > + select PL310_ERRATA_727915 if CACHE_PL310 > > + select PL310_ERRATA_769419 if CACHE_PL310 > > + > > + help > > + This enable support for Freescale Vybrid MVF600 processor. > > + > > endif > > Shouldn't that 'depends on ARCH_MULTI_V7'? > SOC_MVF600 is added as a sub-item of ARCH_MXC which already handles the ARCH_MULTI_* dependency. > Can you describe how much Vybrid is actually like MXC? Do you actually > use most of the mach-imx code? > Jingchang mentioned some IP blocks shared between IMX and Vybrid. Right now, mvf600 reuses mxc_restart() and some amount of clk code clk-pllv3.c, clk-gate2.c clk.c etc. in arch/arm/mach-imx. > > diff --git a/arch/arm/mach-imx/common.h b/arch/arm/mach-imx/common.h > > index 208e76f..55cdba0 100644 > > --- a/arch/arm/mach-imx/common.h > > +++ b/arch/arm/mach-imx/common.h > > @@ -70,6 +70,7 @@ extern int mx51_clocks_init_dt(void); > > extern int mx53_clocks_init_dt(void); > > extern int mx6q_clocks_init(void); > > extern int imx6sl_clocks_init(void); > > +extern int mvf600_clocks_init(void); > > extern struct platform_device *mxc_register_gpio(char *name, int id, > > resource_size_t iobase, resource_size_t iosize, int irq, int irq_high); > > extern void mxc_set_cpu_type(unsigned int type); > > This hunk belongs into the first patch, right? > The declaration is only used in this patch by mach-mvf600.c. > Actually I think you should move that driver to drivers/clk and use > of_clk_init(NULL) to initialize it. > The mvf600 clock driver uses a lot of base clk support from mach-imx, and can not be moved into drivers/clk as a single driver. Right now, in IMX clock drivers, we call of_clk_init() to only register fixed rate clocks, since all the other clocks are not represented in device tree. > > +static void __init mvf600_init_machine(void) > > +{ > > + mxc_arch_reset_init_dt(); > > I don't see the mxc_arch_reset_init_dt function in 3.10-rc1. Where does it > get introduced? > It gets introduced by my series below. And I guess Jiangchang is basing his series on my imx/soc branch. http://thread.gmane.org/gmane.linux.ports.arm.kernel/235986/focus=236091 > It would be nice if we could integrate that into the watchdog driver, > so that driver just registers a pm_restart function when it gets loaded. > It is a rather small driver, so it would not hurt to always load it. > Sound like a good idea. We will consider it as another cleanup task for mach-imx. > > + of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL); > > +} > > It would be nicer if you could just use the default init_machine. > > > +static void __init mvf600_init_irq(void) > > +{ > > + struct device_node *np; > > + void __iomem *mscm_base; > > + int i; > > + > > + l2x0_of_init(0, ~0UL); > > + > > + np = of_find_compatible_node(NULL, NULL, "fsl,mvf600-mscm"); > > + mscm_base = of_iomap(np, 0); > > + if (!mscm_base) > > + return; > > + > > + /* route all shared peripheral interrupts to CP0 */ > > + for (i = 0; i < 111; i++) > > + __raw_writew(1, mscm_base + 0x880 + 2 * i); > > + > > + iounmap(mscm_base); > > + > > + irqchip_init(); > > +} > > What is the mscm? Shouldn't the boot loader have set this up correctly? > If you can remove that code from the kernel, you can use the default > irqchip_init call. > Yeah, and if we find another way for l2x0_of_init() call. > > +static void __init mvf600_init_time(void) > > +{ > > + mvf600_clocks_init(); > > + clocksource_of_init(); > > +} > > I would like to call of_clk_init(NULL) unconditionally on all machines > in 3.11, so this function could also go away. > of_clk_init() has been called in mvf600_clocks_init(), but as explained as above, it only registers fixed rate clocks and can not save mvf600_clocks_init() call right now where most of mvf600 clocks gets registered. > > +static const char *mvf600_dt_compat[] __initdata = { > > + "fsl,mvf600", > > + NULL, > > +}; > > + > > +DT_MACHINE_START(VYBRID_VF6XX, "Freescale Vybrid MVF600 (Device Tree)") > > + .init_irq = mvf600_init_irq, > > + .init_time = mvf600_init_time, > > + .init_machine = mvf600_init_machine, > > + .dt_compat = mvf600_dt_compat, > > + .restart = mxc_restart, > > +MACHINE_END > > If we can do all of the above, we can actually remove the entire machine > descriptor here, since all its members are NULL. > Yeah, we understand the goal, and this will be the goal of mach-imx cleanup. Shawn
On Thu, May 16, 2013 at 02:10:46PM +0800, Jingchang Lu wrote: > Add initial support for Vybrid MVF600 SoC. > > Signed-off-by: Jingchang Lu <b35083@freescale.com> > --- > v3: > remove unused #include header lines. There are more ... > +#include <linux/io.h> > +#include <linux/delay.h> > +#include <linux/of.h> > +#include <linux/of_address.h> > +#include <linux/of_platform.h> > +#include <linux/clk.h> > +#include <linux/clocksource.h> > +#include <linux/irqchip.h> > +#include <linux/irqchip/arm-gic.h> > +#include <asm/mach/arch.h> > +#include <asm/hardware/cache-l2x0.h> > +#include <asm/system_misc.h> #include <linux/delay.h> #include <linux/clk.h> #include <linux/irqchip/arm-gic.h> #include <asm/system_misc.h> I do not see how these 4 headers are needed. Shawn
On Friday 17 May 2013, Shawn Guo wrote: > On Thu, May 16, 2013 at 12:29:28PM +0200, Arnd Bergmann wrote: > > On Thursday 16 May 2013 14:10:46 Jingchang Lu wrote: > > > +config SOC_MVF600 > > > > Shouldn't that 'depends on ARCH_MULTI_V7'? > > > SOC_MVF600 is added as a sub-item of ARCH_MXC which already handles the > ARCH_MULTI_* dependency. Ah, makes sense. > > Can you describe how much Vybrid is actually like MXC? Do you actually > > use most of the mach-imx code? > > > Jingchang mentioned some IP blocks shared between IMX and Vybrid. Right > now, mvf600 reuses mxc_restart() and some amount of clk code > clk-pllv3.c, clk-gate2.c clk.c etc. in arch/arm/mach-imx. Ok. > > Actually I think you should move that driver to drivers/clk and use > > of_clk_init(NULL) to initialize it. > > > The mvf600 clock driver uses a lot of base clk support from mach-imx, > and can not be moved into drivers/clk as a single driver. Right now, in > IMX clock drivers, we call of_clk_init() to only register fixed rate > clocks, since all the other clocks are not represented in device tree. What are your plans for this in the long run? > > It would be nice if we could integrate that into the watchdog driver, > > so that driver just registers a pm_restart function when it gets loaded. > > It is a rather small driver, so it would not hurt to always load it. > > > Sound like a good idea. We will consider it as another cleanup task for > mach-imx. Ok > > What is the mscm? Shouldn't the boot loader have set this up correctly? > > If you can remove that code from the kernel, you can use the default > > irqchip_init call. > > > Yeah, and if we find another way for l2x0_of_init() call. Good point. That is something we need to do anyway. > > > +static const char *mvf600_dt_compat[] __initdata = { > > > + "fsl,mvf600", > > > + NULL, > > > +}; > > > + > > > +DT_MACHINE_START(VYBRID_VF6XX, "Freescale Vybrid MVF600 (Device Tree)") > > > + .init_irq = mvf600_init_irq, > > > + .init_time = mvf600_init_time, > > > + .init_machine = mvf600_init_machine, > > > + .dt_compat = mvf600_dt_compat, > > > + .restart = mxc_restart, > > > +MACHINE_END > > > > If we can do all of the above, we can actually remove the entire machine > > descriptor here, since all its members are NULL. > > > Yeah, we understand the goal, and this will be the goal of mach-imx > cleanup. Ok. I guess we just won't be there for 3.11 then. Arnd
On Friday 17 May 2013, Arnd Bergmann wrote: > > > > +static const char *mvf600_dt_compat[] __initdata = { > > > > + "fsl,mvf600", > > > > + NULL, > > > > +}; > > > > + > > > > +DT_MACHINE_START(VYBRID_VF6XX, "Freescale Vybrid MVF600 (Device Tree)") > > > > + .init_irq = mvf600_init_irq, > > > > + .init_time = mvf600_init_time, > > > > + .init_machine = mvf600_init_machine, > > > > + .dt_compat = mvf600_dt_compat, > > > > + .restart = mxc_restart, > > > > +MACHINE_END > > > > > > If we can do all of the above, we can actually remove the entire machine > > > descriptor here, since all its members are NULL. > > > > > Yeah, we understand the goal, and this will be the goal of mach-imx > > cleanup. > > Ok. I guess we just won't be there for 3.11 then. To clarify: what I mean is we should try to remove all the callbacks we can here, but merge the MVF600 port anyway, even if we still need a couple of them. Arnd
On Fri, May 17, 2013 at 02:29:52PM +0200, Arnd Bergmann wrote: > > > Actually I think you should move that driver to drivers/clk and use > > > of_clk_init(NULL) to initialize it. > > > > > The mvf600 clock driver uses a lot of base clk support from mach-imx, > > and can not be moved into drivers/clk as a single driver. Right now, in > > IMX clock drivers, we call of_clk_init() to only register fixed rate > > clocks, since all the other clocks are not represented in device tree. > > What are your plans for this in the long run? > We can move all the IMX clock drivers into drivers/clk at some point when necessary. But I do not have a plan to register all the clocks by merely calling of_clk_init(), because doing that would mean we have to represent all these clocks in device tree. For imx6q example, it's about 200 ~ 300 nodes addition to DTB. Device tree maintainers are against to the idea. They are perfectly fine with having clock driver in kernel to represent/register these SoC internal clocks to clk framework. Shawn
On Friday 17 May 2013 21:06:05 Shawn Guo wrote: > On Fri, May 17, 2013 at 02:29:52PM +0200, Arnd Bergmann wrote: > > > > Actually I think you should move that driver to drivers/clk and use > > > > of_clk_init(NULL) to initialize it. > > > > > > > The mvf600 clock driver uses a lot of base clk support from mach-imx, > > > and can not be moved into drivers/clk as a single driver. Right now, in > > > IMX clock drivers, we call of_clk_init() to only register fixed rate > > > clocks, since all the other clocks are not represented in device tree. > > > > What are your plans for this in the long run? > > > We can move all the IMX clock drivers into drivers/clk at some point > when necessary. But I do not have a plan to register all the clocks > by merely calling of_clk_init(), because doing that would mean we have > to represent all these clocks in device tree. For imx6q example, it's > about 200 ~ 300 nodes addition to DTB. Device tree maintainers are > against to the idea. They are perfectly fine with having clock driver > in kernel to represent/register these SoC internal clocks to clk > framework. Can't we move the driver to drivers/clk and have it initialized through of_clk_init() without representing all clocks in the DT? For all I can tell, CLK_OF_DECLARE() requires only the clock provider to be described in DT, but not the actual clocks. Arnd
On Fri, May 17, 2013 at 03:17:31PM +0200, Arnd Bergmann wrote: > Can't we move the driver to drivers/clk and have it initialized through > of_clk_init() without representing all clocks in the DT? > > For all I can tell, CLK_OF_DECLARE() requires only the clock provider > to be described in DT, but not the actual clocks. Clock provider is actually a "clk". For fixed rate clock example, of_fixed_factor_clk_setup() firstly parses DT to get all the necessary data and then calls clk_register_fixed_factor() to register the clk to framework, lastly calls of_clk_add_provider() to register the clk as a provider. Shawn
On Thu, May 16, 2013 at 02:10:46PM +0800, Jingchang Lu wrote: > +static void __init mvf600_init_irq(void) > +{ > + struct device_node *np; > + void __iomem *mscm_base; > + int i; > + > + l2x0_of_init(0, ~0UL); > + > + np = of_find_compatible_node(NULL, NULL, "fsl,mvf600-mscm"); > + mscm_base = of_iomap(np, 0); > + if (!mscm_base) > + return; > + > + /* route all shared peripheral interrupts to CP0 */ > + for (i = 0; i < 111; i++) > + __raw_writew(1, mscm_base + 0x880 + 2 * i); I second Arnd's comment on this. Can we have this setup done in bootloader? Shawn > + > + iounmap(mscm_base); > + > + irqchip_init(); > +}
On Fri, May 17, 2013 at 03:17:31PM +0200, Arnd Bergmann wrote: > On Friday 17 May 2013 21:06:05 Shawn Guo wrote: > > On Fri, May 17, 2013 at 02:29:52PM +0200, Arnd Bergmann wrote: > > > > > Actually I think you should move that driver to drivers/clk and use > > > > > of_clk_init(NULL) to initialize it. > > > > > > > > > The mvf600 clock driver uses a lot of base clk support from mach-imx, > > > > and can not be moved into drivers/clk as a single driver. Right now, in > > > > IMX clock drivers, we call of_clk_init() to only register fixed rate > > > > clocks, since all the other clocks are not represented in device tree. > > > > > > What are your plans for this in the long run? > > > > > We can move all the IMX clock drivers into drivers/clk at some point > > when necessary. But I do not have a plan to register all the clocks > > by merely calling of_clk_init(), because doing that would mean we have > > to represent all these clocks in device tree. For imx6q example, it's > > about 200 ~ 300 nodes addition to DTB. Device tree maintainers are > > against to the idea. They are perfectly fine with having clock driver > > in kernel to represent/register these SoC internal clocks to clk > > framework. > > Can't we move the driver to drivers/clk and have it initialized through > of_clk_init() without representing all clocks in the DT? Sorry. I just get aware of that I misunderstood your comment. Yes, declaring the clock initialization function with CLK_OF_DECLARE(), we can use of_clk_init() to have them invoked properly. I just sent a patch to change imx6 clock driver to do that. Shawn
diff --git a/arch/arm/mach-imx/Kconfig b/arch/arm/mach-imx/Kconfig index a402248..b9c01a1 100644 --- a/arch/arm/mach-imx/Kconfig +++ b/arch/arm/mach-imx/Kconfig @@ -835,6 +835,21 @@ config SOC_IMX6SL help This enables support for Freescale i.MX6 SoloLite processor. +config SOC_MVF600 + bool "Vybrid Family MVF600 support" + select CPU_V7 + select ARM_GIC + select CLKSRC_OF + select PINCTRL + select PINCTRL_MVF600 + select MVF600_PIT_TIMER + select PL310_ERRATA_588369 if CACHE_PL310 + select PL310_ERRATA_727915 if CACHE_PL310 + select PL310_ERRATA_769419 if CACHE_PL310 + + help + This enable support for Freescale Vybrid MVF600 processor. + endif source "arch/arm/mach-imx/devices/Kconfig" diff --git a/arch/arm/mach-imx/Makefile b/arch/arm/mach-imx/Makefile index 2a4754d..28d8b4c 100644 --- a/arch/arm/mach-imx/Makefile +++ b/arch/arm/mach-imx/Makefile @@ -112,4 +112,6 @@ obj-$(CONFIG_MACH_EUKREA_MBIMXSD51_BASEBOARD) += eukrea_mbimxsd51-baseboard.o obj-$(CONFIG_MACH_IMX51_DT) += imx51-dt.o obj-$(CONFIG_SOC_IMX53) += mach-imx53.o +obj-$(CONFIG_SOC_MVF600) += clk-mvf600.o mach-mvf600.o + obj-y += devices/ diff --git a/arch/arm/mach-imx/common.h b/arch/arm/mach-imx/common.h index 208e76f..55cdba0 100644 --- a/arch/arm/mach-imx/common.h +++ b/arch/arm/mach-imx/common.h @@ -70,6 +70,7 @@ extern int mx51_clocks_init_dt(void); extern int mx53_clocks_init_dt(void); extern int mx6q_clocks_init(void); extern int imx6sl_clocks_init(void); +extern int mvf600_clocks_init(void); extern struct platform_device *mxc_register_gpio(char *name, int id, resource_size_t iobase, resource_size_t iosize, int irq, int irq_high); extern void mxc_set_cpu_type(unsigned int type); diff --git a/arch/arm/mach-imx/mach-mvf600.c b/arch/arm/mach-imx/mach-mvf600.c new file mode 100644 index 0000000..deffe74 --- /dev/null +++ b/arch/arm/mach-imx/mach-mvf600.c @@ -0,0 +1,70 @@ +/* + * Copyright 2012-2013 Freescale Semiconductor, Inc. + * + * 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/io.h> +#include <linux/delay.h> +#include <linux/of.h> +#include <linux/of_address.h> +#include <linux/of_platform.h> +#include <linux/clk.h> +#include <linux/clocksource.h> +#include <linux/irqchip.h> +#include <linux/irqchip/arm-gic.h> +#include <asm/mach/arch.h> +#include <asm/hardware/cache-l2x0.h> +#include <asm/system_misc.h> + +#include "common.h" + +static void __init mvf600_init_machine(void) +{ + mxc_arch_reset_init_dt(); + of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL); +} + +static void __init mvf600_init_irq(void) +{ + struct device_node *np; + void __iomem *mscm_base; + int i; + + l2x0_of_init(0, ~0UL); + + np = of_find_compatible_node(NULL, NULL, "fsl,mvf600-mscm"); + mscm_base = of_iomap(np, 0); + if (!mscm_base) + return; + + /* route all shared peripheral interrupts to CP0 */ + for (i = 0; i < 111; i++) + __raw_writew(1, mscm_base + 0x880 + 2 * i); + + iounmap(mscm_base); + + irqchip_init(); +} + +static void __init mvf600_init_time(void) +{ + mvf600_clocks_init(); + clocksource_of_init(); +} + +static const char *mvf600_dt_compat[] __initdata = { + "fsl,mvf600", + NULL, +}; + +DT_MACHINE_START(VYBRID_VF6XX, "Freescale Vybrid MVF600 (Device Tree)") + .init_irq = mvf600_init_irq, + .init_time = mvf600_init_time, + .init_machine = mvf600_init_machine, + .dt_compat = mvf600_dt_compat, + .restart = mxc_restart, +MACHINE_END
Add initial support for Vybrid MVF600 SoC. Signed-off-by: Jingchang Lu <b35083@freescale.com> --- v3: remove unused #include header lines. use common mxc restart function. arch/arm/mach-imx/Kconfig | 15 +++++++++ arch/arm/mach-imx/Makefile | 2 ++ arch/arm/mach-imx/common.h | 1 + arch/arm/mach-imx/mach-mvf600.c | 70 +++++++++++++++++++++++++++++++++++++++++ 4 files changed, 88 insertions(+) create mode 100644 arch/arm/mach-imx/mach-mvf600.c