| Message ID | 76d3fea8e633c122a4bc8bded96814a085a2f239.1364319776.git.michal.simek@xilinx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tuesday 26 March 2013, Michal Simek wrote: > +void __iomem *scu_base; > + This cannot be a global variable in a multiplatform kernel. Can you make it static? Arnd
2013/3/26 Arnd Bergmann <arnd@arndb.de>: > On Tuesday 26 March 2013, Michal Simek wrote: >> +void __iomem *scu_base; >> + > > This cannot be a global variable in a multiplatform kernel. Can you > make it static? This global variable is shared because I am using it in smp code and also it will be used in pm code we have. What is it another way how to handle this? Because the same problem could be with zynq_slcr_base. Or the rule is just to use zynq_ prefix for all global variables? Thanks, Michal
On Wednesday 27 March 2013, Michal Simek wrote: > 2013/3/26 Arnd Bergmann <arnd@arndb.de>: > > On Tuesday 26 March 2013, Michal Simek wrote: > >> +void __iomem *scu_base; > >> + > > > > This cannot be a global variable in a multiplatform kernel. Can you > > make it static? > > This global variable is shared because I am using it in smp code > and also it will be used in pm code we have. > > What is it another way how to handle this? > > Because the same problem could be with zynq_slcr_base. > > Or the rule is just to use zynq_ prefix for all global variables? > Yes. The best solution is to mke variables local to some context and only pointed to by other data structures. If that doesn't work, you should try using a static variable in one file and move all code using it there. If that doesn't work, you need a global variable, but that must be uniquely named. Arnd
2013/3/27 Arnd Bergmann <arnd@arndb.de>: > On Wednesday 27 March 2013, Michal Simek wrote: >> 2013/3/26 Arnd Bergmann <arnd@arndb.de>: >> > On Tuesday 26 March 2013, Michal Simek wrote: >> >> +void __iomem *scu_base; >> >> + >> > >> > This cannot be a global variable in a multiplatform kernel. Can you >> > make it static? >> >> This global variable is shared because I am using it in smp code >> and also it will be used in pm code we have. >> >> What is it another way how to handle this? >> >> Because the same problem could be with zynq_slcr_base. >> >> Or the rule is just to use zynq_ prefix for all global variables? >> > > Yes. The best solution is to mke variables local to some context and > only pointed to by other data structures. If that doesn't work, you > should try using a static variable in one file and move all code using > it there. If that doesn't work, you need a global variable, but that > must be uniquely named. FYI: I have looked at some code and I saw that Rob is using scu_base_addr in highbank. And then pointing to it in cpuidle-calxeda.c. Moving everything to one file is probably impossible. And in connection to symbols/functions/variables. It means that all specific soc functions in mach should also use specific prefix for everything. Thanks, Michal
On Wednesday 27 March 2013, Michal Simek wrote: > FYI: I have looked at some code and I saw that Rob is using scu_base_addr > in highbank. And then pointing to it in cpuidle-calxeda.c. Yes, the point is that it works as long as only one person uses that identifier, so we should either not use it at all, or have a single global definition shared by all ARM platforms. > Moving everything to one file is probably impossible. > > And in connection to symbols/functions/variables. It means that all > specific soc functions in mach > should also use specific prefix for everything. That is a good rule, although for static symbols it is not a bug if they don't have a prefix. Arnd
2013/3/27 Arnd Bergmann <arnd@arndb.de>: > On Wednesday 27 March 2013, Michal Simek wrote: >> FYI: I have looked at some code and I saw that Rob is using scu_base_addr >> in highbank. And then pointing to it in cpuidle-calxeda.c. > > Yes, the point is that it works as long as only one person uses that > identifier, so we should either not use it at all, or have a single > global definition shared by all ARM platforms. yep. >> Moving everything to one file is probably impossible. >> >> And in connection to symbols/functions/variables. It means that all >> specific soc functions in mach >> should also use specific prefix for everything. > > That is a good rule, although for static symbols it is not a bug > if they don't have a prefix. ok. Let me check all my patches and Add there zynq_ prefix everywhere. BTW: Have you added to zero day testing system arm multi platform or is there any arm multi platform config which developer should run to be sure that armv7 code can be compiled for armv6 and also that there is no problem with symbols? Thanks, Michal
On Wed, Mar 27, 2013 at 11:09:45AM +0100, Michal Simek wrote: > 2013/3/27 Arnd Bergmann <arnd@arndb.de>: > > On Wednesday 27 March 2013, Michal Simek wrote: > >> FYI: I have looked at some code and I saw that Rob is using scu_base_addr > >> in highbank. And then pointing to it in cpuidle-calxeda.c. > > > > Yes, the point is that it works as long as only one person uses that > > identifier, so we should either not use it at all, or have a single > > global definition shared by all ARM platforms. > > yep. > > >> Moving everything to one file is probably impossible. > >> > >> And in connection to symbols/functions/variables. It means that all > >> specific soc functions in mach > >> should also use specific prefix for everything. > > > > That is a good rule, although for static symbols it is not a bug > > if they don't have a prefix. > > ok. Let me check all my patches and Add there zynq_ prefix everywhere. > While you're at, how about getting rid of the xilinx in the names and use just zynq_* ? I would also suggest s/xttc/ttc/g in your timer patch. It is up to you, but I think the x doesn't make any sense anymore. Regards, Steffen
On Wednesday 27 March 2013, Michal Simek wrote: > BTW: Have you added to zero day testing system arm multi platform > or is there any arm multi platform config which developer should run > to be sure that armv7 code can be compiled for armv6 and also > that there is no problem with symbols? I'm only occasionally doing build tested right now, not all the time. Arnd
2013/3/27 Arnd Bergmann <arnd@arndb.de>: > On Wednesday 27 March 2013, Michal Simek wrote: >> BTW: Have you added to zero day testing system arm multi platform >> or is there any arm multi platform config which developer should run >> to be sure that armv7 code can be compiled for armv6 and also >> that there is no problem with symbols? > > I'm only occasionally doing build tested right now, not all the time. What about to provide this config to Fengguang to add this to zero day testing system which will check this all the time? Thanks, Michal
2013/3/27 Steffen Trumtrar <s.trumtrar@pengutronix.de>: > On Wed, Mar 27, 2013 at 11:09:45AM +0100, Michal Simek wrote: >> 2013/3/27 Arnd Bergmann <arnd@arndb.de>: >> > On Wednesday 27 March 2013, Michal Simek wrote: >> >> FYI: I have looked at some code and I saw that Rob is using scu_base_addr >> >> in highbank. And then pointing to it in cpuidle-calxeda.c. >> > >> > Yes, the point is that it works as long as only one person uses that >> > identifier, so we should either not use it at all, or have a single >> > global definition shared by all ARM platforms. >> >> yep. >> >> >> Moving everything to one file is probably impossible. >> >> >> >> And in connection to symbols/functions/variables. It means that all >> >> specific soc functions in mach >> >> should also use specific prefix for everything. >> > >> > That is a good rule, although for static symbols it is not a bug >> > if they don't have a prefix. >> >> ok. Let me check all my patches and Add there zynq_ prefix everywhere. >> > > While you're at, how about getting rid of the xilinx in the names and use > just zynq_* ? yep. That's what I am going to do. > I would also suggest s/xttc/ttc/g in your timer patch. It is up to you, > but I think the x doesn't make any sense anymore. Agree. I don't know the historical reason for this. Also in xilinx git repo there was PSS instead of PS everywhere. Thanks, Michal
diff --git a/arch/arm/mach-zynq/common.c b/arch/arm/mach-zynq/common.c index 68e0907..b53c34d 100644 --- a/arch/arm/mach-zynq/common.c +++ b/arch/arm/mach-zynq/common.c @@ -33,10 +33,13 @@ #include <asm/mach-types.h> #include <asm/page.h> #include <asm/pgtable.h> +#include <asm/smp_scu.h> #include <asm/hardware/cache-l2x0.h> #include "common.h" +void __iomem *scu_base; + static struct of_device_id zynq_of_bus_ids[] __initdata = { { .compatible = "simple-bus", }, {} @@ -56,17 +59,6 @@ static void __init xilinx_init_machine(void) of_platform_bus_probe(NULL, zynq_of_bus_ids, NULL); } -#define SCU_PERIPH_PHYS 0xF8F00000 -#define SCU_PERIPH_SIZE SZ_8K -#define SCU_PERIPH_VIRT (VMALLOC_END - SCU_PERIPH_SIZE) - -static struct map_desc scu_desc __initdata = { - .virtual = SCU_PERIPH_VIRT, - .pfn = __phys_to_pfn(SCU_PERIPH_PHYS), - .length = SCU_PERIPH_SIZE, - .type = MT_DEVICE, -}; - static void __init xilinx_zynq_timer_init(void) { struct device_node *np; @@ -81,13 +73,31 @@ static void __init xilinx_zynq_timer_init(void) clocksource_of_init(); } +static struct map_desc zynq_cortex_a9_scu_map __initdata = { + .length = SZ_256, + .type = MT_DEVICE, +}; + +static void __init zynq_scu_map_io(void) +{ + unsigned long base; + + base = scu_a9_get_base(); + zynq_cortex_a9_scu_map.pfn = __phys_to_pfn(base); + /* Expected address is in vmalloc area that's why simple assign here */ + zynq_cortex_a9_scu_map.virtual = base; + iotable_init(&zynq_cortex_a9_scu_map, 1); + scu_base = (void __iomem *)base; + BUG_ON(!scu_base); +} + /** * xilinx_map_io() - Create memory mappings needed for early I/O. */ static void __init xilinx_map_io(void) { debug_ll_io_init(); - iotable_init(&scu_desc, 1); + zynq_scu_map_io(); } static const char *xilinx_dt_match[] = { diff --git a/arch/arm/mach-zynq/common.h b/arch/arm/mach-zynq/common.h index 5050bb1..38727a2 100644 --- a/arch/arm/mach-zynq/common.h +++ b/arch/arm/mach-zynq/common.h @@ -17,4 +17,6 @@ #ifndef __MACH_ZYNQ_COMMON_H__ #define __MACH_ZYNQ_COMMON_H__ +extern void __iomem *scu_base; + #endif
Use Cortex a9 cp15 to read scu baseaddress. Signed-off-by: Michal Simek <michal.simek@xilinx.com> --- v2: Remove dynamic mapping because it is probably not needed (code taken from vexpress platsmp code) Remove scu node from DTS Use zynq_scu_map_io() instead of scu_init() as Steffen suggested Add comment to scu_init to be sure that we know what we are doing here --- arch/arm/mach-zynq/common.c | 34 ++++++++++++++++++++++------------ arch/arm/mach-zynq/common.h | 2 ++ 2 files changed, 24 insertions(+), 12 deletions(-)