| Message ID | dc278dadd3614b4736f43a09b9c5d7ccd2232403.1364319776.git.michal.simek@xilinx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tuesday 26 March 2013, Michal Simek wrote: > Create separate slcr driver instead of pollute common code. > > Signed-off-by: Michal Simek <michal.simek@xilinx.com> Can't you move that code into the zynq_cpu_clk_setup function instead, and only call of_clk_init(NULL) from platform code? I would like to eventually add a default call to of_clk_init(NULL) for all platforms, and that would be a step in that direction. Arnd
Hi Arnd, On Tue, Mar 26, 2013 at 09:43:23PM +0000, Arnd Bergmann wrote: > On Tuesday 26 March 2013, Michal Simek wrote: > > Create separate slcr driver instead of pollute common code. > > > > Signed-off-by: Michal Simek <michal.simek@xilinx.com> > > Can't you move that code into the zynq_cpu_clk_setup function > instead, and only call of_clk_init(NULL) from platform code? > if you are talking about the slcr function, than moving it into a separate file is the right move. This should actually become a real driver. The slcr is master over all clock, reset, pinmux and ddr registers. And as all those registers can be locked/unlocked via a slcr register (for whatever reason one would do that), there should be one master that controls this space. > I would like to eventually add a default call to of_clk_init(NULL) > for all platforms, and that would be a step in that direction. > Regards, Steffen
On Wednesday 27 March 2013, Steffen Trumtrar wrote: > On Tue, Mar 26, 2013 at 09:43:23PM +0000, Arnd Bergmann wrote: > > On Tuesday 26 March 2013, Michal Simek wrote: > > > Create separate slcr driver instead of pollute common code. > > > > > > Signed-off-by: Michal Simek <michal.simek@xilinx.com> > > > > Can't you move that code into the zynq_cpu_clk_setup function > > instead, and only call of_clk_init(NULL) from platform code? > > > if you are talking about the slcr function, than moving it into > a separate file is the right move. This should actually become a > real driver. The slcr is master over all clock, reset, pinmux and > ddr registers. And as all those registers can be locked/unlocked > via a slcr register (for whatever reason one would do that), there > should be one master that controls this space. Ok, I see. Thanks for the explanation. Should this be using the drivers/mfd/syscon.c infrastructure then? Arnd
On Wed, Mar 27, 2013 at 09:31:26AM +0000, Arnd Bergmann wrote: > On Wednesday 27 March 2013, Steffen Trumtrar wrote: > > On Tue, Mar 26, 2013 at 09:43:23PM +0000, Arnd Bergmann wrote: > > > On Tuesday 26 March 2013, Michal Simek wrote: > > > > Create separate slcr driver instead of pollute common code. > > > > > > > > Signed-off-by: Michal Simek <michal.simek@xilinx.com> > > > > > > Can't you move that code into the zynq_cpu_clk_setup function > > > instead, and only call of_clk_init(NULL) from platform code? > > > > > if you are talking about the slcr function, than moving it into > > a separate file is the right move. This should actually become a > > real driver. The slcr is master over all clock, reset, pinmux and > > ddr registers. And as all those registers can be locked/unlocked > > via a slcr register (for whatever reason one would do that), there > > should be one master that controls this space. > > Ok, I see. Thanks for the explanation. Should this be using the > drivers/mfd/syscon.c infrastructure then? A quick look suggests that this might be the way to go. I wasn't aware of that. Thanks, Steffen
On 03/26/2013 01:43 PM, Michal Simek wrote: > Create separate slcr driver instead of pollute common code. Nitpicking: A native speaker would say "polluting common code". Philip > > Signed-off-by: Michal Simek <michal.simek@xilinx.com> > --- > arch/arm/mach-zynq/Makefile | 2 +- > arch/arm/mach-zynq/common.c | 10 +------ > arch/arm/mach-zynq/common.h | 3 ++ > arch/arm/mach-zynq/slcr.c | 69 +++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 74 insertions(+), 10 deletions(-) > create mode 100644 arch/arm/mach-zynq/slcr.c > > diff --git a/arch/arm/mach-zynq/Makefile b/arch/arm/mach-zynq/Makefile > index 320faed..13ee09b 100644 > --- a/arch/arm/mach-zynq/Makefile > +++ b/arch/arm/mach-zynq/Makefile > @@ -3,4 +3,4 @@ > # > > # Common support > -obj-y := common.o > +obj-y := common.o slcr.o > diff --git a/arch/arm/mach-zynq/common.c b/arch/arm/mach-zynq/common.c > index b53c34d..2cb94ab 100644 > --- a/arch/arm/mach-zynq/common.c > +++ b/arch/arm/mach-zynq/common.c > @@ -61,15 +61,7 @@ static void __init xilinx_init_machine(void) > > static void __init xilinx_zynq_timer_init(void) > { > - struct device_node *np; > - void __iomem *slcr; > - > - np = of_find_compatible_node(NULL, NULL, "xlnx,zynq-slcr"); > - slcr = of_iomap(np, 0); > - WARN_ON(!slcr); > - > - xilinx_zynq_clocks_init(slcr); > - > + slcr_init(); > clocksource_of_init(); > } > > diff --git a/arch/arm/mach-zynq/common.h b/arch/arm/mach-zynq/common.h > index 38727a2..e30898a 100644 > --- a/arch/arm/mach-zynq/common.h > +++ b/arch/arm/mach-zynq/common.h > @@ -17,6 +17,9 @@ > #ifndef __MACH_ZYNQ_COMMON_H__ > #define __MACH_ZYNQ_COMMON_H__ > > +extern int slcr_init(void); > + > +extern void __iomem *zynq_slcr_base; > extern void __iomem *scu_base; > > #endif > diff --git a/arch/arm/mach-zynq/slcr.c b/arch/arm/mach-zynq/slcr.c > new file mode 100644 > index 0000000..1883b70 > --- /dev/null > +++ b/arch/arm/mach-zynq/slcr.c > @@ -0,0 +1,69 @@ > +/* > + * Xilinx SLCR driver > + * > + * Copyright (c) 2011-2013 Xilinx 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. > + * > + * You should have received a copy of the GNU General Public > + * License along with this program; if not, write to the Free > + * Software Foundation, Inc., 675 Mass Ave, Cambridge, MA > + * 02139, USA. > + */ > + > +#include <linux/export.h> > +#include <linux/io.h> > +#include <linux/fs.h> > +#include <linux/interrupt.h> > +#include <linux/init.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/of_address.h> > +#include <linux/uaccess.h> > +#include <linux/platform_device.h> > +#include <linux/slab.h> > +#include <linux/string.h> > +#include <linux/clk/zynq.h> > +#include "common.h" > + > +#define SLCR_UNLOCK_MAGIC 0xDF0D > +#define SLCR_UNLOCK 0x8 /* SCLR unlock register */ > + > +void __iomem *zynq_slcr_base; > + > +/** > + * xslcr_init() > + * Returns 0 on success, negative errno otherwise. > + * > + * Called early during boot from platform code to remap SLCR area. > + */ > +int __init slcr_init(void) > +{ > + struct device_node *np; > + > + np = of_find_compatible_node(NULL, NULL, "xlnx,zynq-slcr"); > + if (!np) { > + pr_err("%s: no slcr node found\n", __func__); > + BUG(); > + } > + > + zynq_slcr_base = of_iomap(np, 0); > + if (!zynq_slcr_base) { > + pr_err("%s: Unable to map I/O memory\n", __func__); > + BUG(); > + } > + > + /* unlock the SLCR so that registers can be changed */ > + writel(SLCR_UNLOCK_MAGIC, zynq_slcr_base + SLCR_UNLOCK); > + > + pr_info("%s mapped to %p\n", np->name, zynq_slcr_base); > + > + xilinx_zynq_clocks_init(zynq_slcr_base); > + > + of_node_put(np); > + > + return 0; > +} >
Hi Philip, 2013/3/27 Philip Balister <philip@balister.org>: > On 03/26/2013 01:43 PM, Michal Simek wrote: >> >> Create separate slcr driver instead of pollute common code. > > > Nitpicking: A native speaker would say "polluting common code". Feel free to check my english. :-) M
2013/3/27 Steffen Trumtrar <s.trumtrar@pengutronix.de>: > On Wed, Mar 27, 2013 at 09:31:26AM +0000, Arnd Bergmann wrote: >> On Wednesday 27 March 2013, Steffen Trumtrar wrote: >> > On Tue, Mar 26, 2013 at 09:43:23PM +0000, Arnd Bergmann wrote: >> > > On Tuesday 26 March 2013, Michal Simek wrote: >> > > > Create separate slcr driver instead of pollute common code. >> > > > >> > > > Signed-off-by: Michal Simek <michal.simek@xilinx.com> >> > > >> > > Can't you move that code into the zynq_cpu_clk_setup function >> > > instead, and only call of_clk_init(NULL) from platform code? >> > > >> > if you are talking about the slcr function, than moving it into >> > a separate file is the right move. This should actually become a >> > real driver. The slcr is master over all clock, reset, pinmux and >> > ddr registers. And as all those registers can be locked/unlocked >> > via a slcr register (for whatever reason one would do that), there >> > should be one master that controls this space. >> >> Ok, I see. Thanks for the explanation. Should this be using the >> drivers/mfd/syscon.c infrastructure then? > > A quick look suggests that this might be the way to go. I wasn't aware of that. I have looked at syscon driver but the problem is when this driver is ready to use. It is too late because bus probing is done in init_machine but we need to unlock slcr before clk and timer init functions are called. For unlocking we have to also map it. of_clk_init(NULL) is called from zynq clock. It is no problem to move it to common code but still we have to map slcr register in the driver and pass this address to clk driver. Currently we are passing slcr reg base as xilinx_zynq_clocks_init() parameter to avoid to have extern in C file. I see that Rob in highbank clk choose a way with extern. of_iomap is done in highbank_timer_init and in the same function calls of_clk_init() and clocksource_of_init(). Syscon will be nice to use much later but not for this core stuff. But maybe you know nicer way how to handle it. Thanks, Michal
diff --git a/arch/arm/mach-zynq/Makefile b/arch/arm/mach-zynq/Makefile index 320faed..13ee09b 100644 --- a/arch/arm/mach-zynq/Makefile +++ b/arch/arm/mach-zynq/Makefile @@ -3,4 +3,4 @@ # # Common support -obj-y := common.o +obj-y := common.o slcr.o diff --git a/arch/arm/mach-zynq/common.c b/arch/arm/mach-zynq/common.c index b53c34d..2cb94ab 100644 --- a/arch/arm/mach-zynq/common.c +++ b/arch/arm/mach-zynq/common.c @@ -61,15 +61,7 @@ static void __init xilinx_init_machine(void) static void __init xilinx_zynq_timer_init(void) { - struct device_node *np; - void __iomem *slcr; - - np = of_find_compatible_node(NULL, NULL, "xlnx,zynq-slcr"); - slcr = of_iomap(np, 0); - WARN_ON(!slcr); - - xilinx_zynq_clocks_init(slcr); - + slcr_init(); clocksource_of_init(); } diff --git a/arch/arm/mach-zynq/common.h b/arch/arm/mach-zynq/common.h index 38727a2..e30898a 100644 --- a/arch/arm/mach-zynq/common.h +++ b/arch/arm/mach-zynq/common.h @@ -17,6 +17,9 @@ #ifndef __MACH_ZYNQ_COMMON_H__ #define __MACH_ZYNQ_COMMON_H__ +extern int slcr_init(void); + +extern void __iomem *zynq_slcr_base; extern void __iomem *scu_base; #endif diff --git a/arch/arm/mach-zynq/slcr.c b/arch/arm/mach-zynq/slcr.c new file mode 100644 index 0000000..1883b70 --- /dev/null +++ b/arch/arm/mach-zynq/slcr.c @@ -0,0 +1,69 @@ +/* + * Xilinx SLCR driver + * + * Copyright (c) 2011-2013 Xilinx 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. + * + * You should have received a copy of the GNU General Public + * License along with this program; if not, write to the Free + * Software Foundation, Inc., 675 Mass Ave, Cambridge, MA + * 02139, USA. + */ + +#include <linux/export.h> +#include <linux/io.h> +#include <linux/fs.h> +#include <linux/interrupt.h> +#include <linux/init.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/of_address.h> +#include <linux/uaccess.h> +#include <linux/platform_device.h> +#include <linux/slab.h> +#include <linux/string.h> +#include <linux/clk/zynq.h> +#include "common.h" + +#define SLCR_UNLOCK_MAGIC 0xDF0D +#define SLCR_UNLOCK 0x8 /* SCLR unlock register */ + +void __iomem *zynq_slcr_base; + +/** + * xslcr_init() + * Returns 0 on success, negative errno otherwise. + * + * Called early during boot from platform code to remap SLCR area. + */ +int __init slcr_init(void) +{ + struct device_node *np; + + np = of_find_compatible_node(NULL, NULL, "xlnx,zynq-slcr"); + if (!np) { + pr_err("%s: no slcr node found\n", __func__); + BUG(); + } + + zynq_slcr_base = of_iomap(np, 0); + if (!zynq_slcr_base) { + pr_err("%s: Unable to map I/O memory\n", __func__); + BUG(); + } + + /* unlock the SLCR so that registers can be changed */ + writel(SLCR_UNLOCK_MAGIC, zynq_slcr_base + SLCR_UNLOCK); + + pr_info("%s mapped to %p\n", np->name, zynq_slcr_base); + + xilinx_zynq_clocks_init(zynq_slcr_base); + + of_node_put(np); + + return 0; +}
Create separate slcr driver instead of pollute common code. Signed-off-by: Michal Simek <michal.simek@xilinx.com> --- arch/arm/mach-zynq/Makefile | 2 +- arch/arm/mach-zynq/common.c | 10 +------ arch/arm/mach-zynq/common.h | 3 ++ arch/arm/mach-zynq/slcr.c | 69 +++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 74 insertions(+), 10 deletions(-) create mode 100644 arch/arm/mach-zynq/slcr.c