| Message ID | 20130320152915.GA32083@amd.pavel.ucw.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Mar 20, 2013 at 04:29:15PM +0100, Pavel Machek wrote: > Hi! > > > From: Dinh Nguyen <dinguyen@altera.com> > > > > Enable a cold or warm reset to the HW from userspace. > > > > Also fix a few sparse errors: > > > > warning: symbol 'sys_manager_base_addr' was not declared. Should it be static? > > warning: symbol 'rst_manager_base_addr' was not declared. Should it be static? > > > > Signed-off-by: Dinh Nguyen <dinguyen@altera.com> > > Tested-by: Pavel Machek <pavel@denx.de> > > Would it make sense to apply something like this? Struct looks cleaner > than offset defines... Structs used to define offsets is technically dodgy according to the C standards, so I personally really do not like this practice, and I will not use it ever myself where offsets matter. Your use is fine though, but only because we know how the compiler behaves on ARM - and that's a very important point with doing this. We're reliant on that compiler behaviour if we go down this path.
On Wed, Mar 20, 2013 at 04:29:15PM +0100, Pavel Machek wrote: > Hi! > > > From: Dinh Nguyen <dinguyen@altera.com> > > > > Enable a cold or warm reset to the HW from userspace. > > > > Also fix a few sparse errors: > > > > warning: symbol 'sys_manager_base_addr' was not declared. Should it be static? > > warning: symbol 'rst_manager_base_addr' was not declared. Should it be static? > > > > Signed-off-by: Dinh Nguyen <dinguyen@altera.com> > > Tested-by: Pavel Machek <pavel@denx.de> > > Would it make sense to apply something like this? Struct looks cleaner > than offset defines... > > Thanks, > Pavel > > Switch reset manager to using struct (not defines), cleanups. > > Convert SMP code to use the struct instead of open-coded numbers. > > Also none of the code is time-critical, so it > does not make sense to use __raw_writel variants. > > Signed-off-by: Pavel Machek <pavel@denx.de> > > diff --git a/arch/arm/mach-socfpga/core.h b/arch/arm/mach-socfpga/core.h > index d2a251f..f4b6048 100644 > --- a/arch/arm/mach-socfpga/core.h > +++ b/arch/arm/mach-socfpga/core.h > @@ -20,19 +20,21 @@ > #ifndef __MACH_CORE_H > #define __MACH_CORE_H > > -#define SOCFPGA_RSTMGR_CTRL 0x04 > -#define SOCFPGA_RSTMGR_MODPERRST 0x14 > -#define SOCFPGA_RSTMGR_BRGMODRST 0x1c > - > -/* System Manager bits */ > -#define RSTMGR_CTRL_SWCOLDRSTREQ 0x1 /* Cold Reset */ > -#define RSTMGR_CTRL_SWWARMRSTREQ 0x2 /* Warm Reset */ > -/*MPU Module Reset Register */ > -#define RSTMGR_MPUMODRST_CPU0 0x1 /*CPU0 Reset*/ > -#define RSTMGR_MPUMODRST_CPU1 0x2 /*CPU1 Reset*/ > -#define RSTMGR_MPUMODRST_WDS 0x4 /*Watchdog Reset*/ > -#define RSTMGR_MPUMODRST_SCUPER 0x8 /*SCU and periphs reset*/ > -#define RSTMGR_MPUMODRST_L2 0x10 /*L2 Cache reset*/ > +struct socfpga_rstmgr_hw { > + u32 unk; > + u32 ctrl; /* 0x04 */ > + u32 unk2, unk3; > +/* MPU Module Reset Register */ > +#define RSTMGR_MPUMODRST_CPU0 0x1 /* CPU0 Reset */ > +#define RSTMGR_MPUMODRST_CPU1 0x2 /* CPU1 Reset */ > +#define RSTMGR_MPUMODRST_WDS 0x4 /* Watchdog Reset */ > +#define RSTMGR_MPUMODRST_SCUPER 0x8 /* SCU and periphs reset */ > +#define RSTMGR_MPUMODRST_L2 0x10 /* L2 Cache reset */ > + u32 mpumodrst; /* 0x10 */ > + u32 modperrst; /* 0x14 */ > + u32 unk5; > + u32 bgrmodrst; /* 0x1c */ > +}; As Russell already replied, struct used to represent register layout is normally frowned upon in drivers. If you want to tighten up the code in this case, make a local helper function that just takes the register offset instead, i.e.: > - temp = __raw_readl(rst_manager_base_addr + SOCFPGA_RSTMGR_CTRL); > + temp = readl(&rst_manager_base_addr->ctrl); temp = rst_readl(SOCFPGA_RSTMGR_CTRL); (and then have rst_readl/rst_writel do the math based on base address). -Olof
Hi! > > +struct socfpga_rstmgr_hw { > > + u32 unk; > > + u32 ctrl; /* 0x04 */ > > + u32 unk2, unk3; > > +/* MPU Module Reset Register */ > > +#define RSTMGR_MPUMODRST_CPU0 0x1 /* CPU0 Reset */ > > +#define RSTMGR_MPUMODRST_CPU1 0x2 /* CPU1 Reset */ > > +#define RSTMGR_MPUMODRST_WDS 0x4 /* Watchdog Reset */ > > +#define RSTMGR_MPUMODRST_SCUPER 0x8 /* SCU and periphs reset */ > > +#define RSTMGR_MPUMODRST_L2 0x10 /* L2 Cache reset */ > > + u32 mpumodrst; /* 0x10 */ > > + u32 modperrst; /* 0x14 */ > > + u32 unk5; > > + u32 bgrmodrst; /* 0x1c */ > > +}; > > As Russell already replied, struct used to represent register layout is > normally frowned upon in drivers. Well... as Russell also said, this is arch-specific code, so it is actually ok. > If you want to tighten up the code in this case, make a local helper > function that just takes the register offset instead, i.e.: > > > - temp = __raw_readl(rst_manager_base_addr + SOCFPGA_RSTMGR_CTRL); > > + temp = readl(&rst_manager_base_addr->ctrl); > > temp = rst_readl(SOCFPGA_RSTMGR_CTRL); > > (and then have rst_readl/rst_writel do the math based on base > address). That does not prevent mistake such as rst_readl(SOCFPGA_SYSMGR_CTRL)... and with number of different subsystems on socfpga (each with separate base address), I fear that might be an issue. Unfortunately, C does not check type of enums, so they can't be used to solve that. Any other ideas? Pavel
On Wed, Apr 3, 2013 at 1:32 PM, Pavel Machek <pavel@denx.de> wrote: > Hi! > >> > +struct socfpga_rstmgr_hw { >> > + u32 unk; >> > + u32 ctrl; /* 0x04 */ >> > + u32 unk2, unk3; >> > +/* MPU Module Reset Register */ >> > +#define RSTMGR_MPUMODRST_CPU0 0x1 /* CPU0 Reset */ >> > +#define RSTMGR_MPUMODRST_CPU1 0x2 /* CPU1 Reset */ >> > +#define RSTMGR_MPUMODRST_WDS 0x4 /* Watchdog Reset */ >> > +#define RSTMGR_MPUMODRST_SCUPER 0x8 /* SCU and periphs reset */ >> > +#define RSTMGR_MPUMODRST_L2 0x10 /* L2 Cache reset */ >> > + u32 mpumodrst; /* 0x10 */ >> > + u32 modperrst; /* 0x14 */ >> > + u32 unk5; >> > + u32 bgrmodrst; /* 0x1c */ >> > +}; >> >> As Russell already replied, struct used to represent register layout is >> normally frowned upon in drivers. > > Well... as Russell also said, this is arch-specific code, so it is > actually ok. Which is why I said "normally frowned upon" instead of "always frowned upon". >> If you want to tighten up the code in this case, make a local helper >> function that just takes the register offset instead, i.e.: >> >> > - temp = __raw_readl(rst_manager_base_addr + SOCFPGA_RSTMGR_CTRL); >> > + temp = readl(&rst_manager_base_addr->ctrl); >> >> temp = rst_readl(SOCFPGA_RSTMGR_CTRL); >> >> (and then have rst_readl/rst_writel do the math based on base >> address). > > That does not prevent mistake such as > rst_readl(SOCFPGA_SYSMGR_CTRL)... and with number of different > subsystems on socfpga (each with separate base address), I fear that > might be an issue. > > Unfortunately, C does not check type of enums, so they can't be used > to solve that. That's no different from accidentally doing readl(&sys_manager_base_addr->ctrl) instead of rst_manager_base_addr->ctrl. -Olof
Hi Pavel, On Wed, 2013-04-03 at 14:12 -0700, Olof Johansson wrote: > On Wed, Apr 3, 2013 at 1:32 PM, Pavel Machek <pavel@denx.de> wrote: > > Hi! > > > >> > +struct socfpga_rstmgr_hw { > >> > + u32 unk; > >> > + u32 ctrl; /* 0x04 */ > >> > + u32 unk2, unk3; > >> > +/* MPU Module Reset Register */ > >> > +#define RSTMGR_MPUMODRST_CPU0 0x1 /* CPU0 Reset */ > >> > +#define RSTMGR_MPUMODRST_CPU1 0x2 /* CPU1 Reset */ > >> > +#define RSTMGR_MPUMODRST_WDS 0x4 /* Watchdog Reset */ > >> > +#define RSTMGR_MPUMODRST_SCUPER 0x8 /* SCU and periphs reset */ > >> > +#define RSTMGR_MPUMODRST_L2 0x10 /* L2 Cache reset */ > >> > + u32 mpumodrst; /* 0x10 */ > >> > + u32 modperrst; /* 0x14 */ > >> > + u32 unk5; > >> > + u32 bgrmodrst; /* 0x1c */ > >> > +}; > >> > >> As Russell already replied, struct used to represent register layout is > >> normally frowned upon in drivers. > > > > Well... as Russell also said, this is arch-specific code, so it is > > actually ok. > > Which is why I said "normally frowned upon" instead of "always frowned upon". > > >> If you want to tighten up the code in this case, make a local helper > >> function that just takes the register offset instead, i.e.: > >> > >> > - temp = __raw_readl(rst_manager_base_addr + SOCFPGA_RSTMGR_CTRL); > >> > + temp = readl(&rst_manager_base_addr->ctrl); > >> > >> temp = rst_readl(SOCFPGA_RSTMGR_CTRL); > >> > >> (and then have rst_readl/rst_writel do the math based on base > >> address). > > > > That does not prevent mistake such as > > rst_readl(SOCFPGA_SYSMGR_CTRL)... and with number of different > > subsystems on socfpga (each with separate base address), I fear that > > might be an issue. > > > > Unfortunately, C does not check type of enums, so they can't be used > > to solve that. > > That's no different from accidentally doing > readl(&sys_manager_base_addr->ctrl) instead of > rst_manager_base_addr->ctrl. > I also don't really like to use structs for register offsets. So unless, there's some standard to use them in machine code, I'm not going to use them in mach-socfpga. I apologize for not getting to a v3 for this patch series with Mike's recommended change for using composite clocks as quickly as I would have like. I hope to have a v3 by next week. Dinh > > -Olof >
diff --git a/arch/arm/mach-socfpga/core.h b/arch/arm/mach-socfpga/core.h index d2a251f..f4b6048 100644 --- a/arch/arm/mach-socfpga/core.h +++ b/arch/arm/mach-socfpga/core.h @@ -20,19 +20,21 @@ #ifndef __MACH_CORE_H #define __MACH_CORE_H -#define SOCFPGA_RSTMGR_CTRL 0x04 -#define SOCFPGA_RSTMGR_MODPERRST 0x14 -#define SOCFPGA_RSTMGR_BRGMODRST 0x1c - -/* System Manager bits */ -#define RSTMGR_CTRL_SWCOLDRSTREQ 0x1 /* Cold Reset */ -#define RSTMGR_CTRL_SWWARMRSTREQ 0x2 /* Warm Reset */ -/*MPU Module Reset Register */ -#define RSTMGR_MPUMODRST_CPU0 0x1 /*CPU0 Reset*/ -#define RSTMGR_MPUMODRST_CPU1 0x2 /*CPU1 Reset*/ -#define RSTMGR_MPUMODRST_WDS 0x4 /*Watchdog Reset*/ -#define RSTMGR_MPUMODRST_SCUPER 0x8 /*SCU and periphs reset*/ -#define RSTMGR_MPUMODRST_L2 0x10 /*L2 Cache reset*/ +struct socfpga_rstmgr_hw { + u32 unk; + u32 ctrl; /* 0x04 */ + u32 unk2, unk3; +/* MPU Module Reset Register */ +#define RSTMGR_MPUMODRST_CPU0 0x1 /* CPU0 Reset */ +#define RSTMGR_MPUMODRST_CPU1 0x2 /* CPU1 Reset */ +#define RSTMGR_MPUMODRST_WDS 0x4 /* Watchdog Reset */ +#define RSTMGR_MPUMODRST_SCUPER 0x8 /* SCU and periphs reset */ +#define RSTMGR_MPUMODRST_L2 0x10 /* L2 Cache reset */ + u32 mpumodrst; /* 0x10 */ + u32 modperrst; /* 0x14 */ + u32 unk5; + u32 bgrmodrst; /* 0x1c */ +}; extern void socfpga_secondary_startup(void); extern void __iomem *socfpga_scu_base_addr; @@ -41,7 +43,7 @@ extern void socfpga_init_clocks(void); extern void socfpga_sysmgr_init(void); extern void __iomem *sys_manager_base_addr; -extern void __iomem *rst_manager_base_addr; +extern struct socfpga_rstmgr_hw __iomem *rst_manager_base_addr; extern struct smp_operations socfpga_smp_ops; extern char secondary_trampoline, secondary_trampoline_end; diff --git a/arch/arm/mach-socfpga/socfpga.c b/arch/arm/mach-socfpga/socfpga.c index b41a945..81b8f1e 100644 --- a/arch/arm/mach-socfpga/socfpga.c +++ b/arch/arm/mach-socfpga/socfpga.c @@ -28,7 +28,7 @@ void __iomem *socfpga_scu_base_addr = ((void __iomem *)(SOCFPGA_SCU_VIRT_BASE)); void __iomem *sys_manager_base_addr; -void __iomem *rst_manager_base_addr; +struct socfpga_rstmgr_hw __iomem *rst_manager_base_addr; unsigned long cpu1start_addr; static struct map_desc scu_io_desc __initdata = { @@ -89,13 +89,13 @@ static void socfpga_cyclone5_restart(char mode, const char *cmd) { u32 temp; - temp = __raw_readl(rst_manager_base_addr + SOCFPGA_RSTMGR_CTRL); + temp = readl(&rst_manager_base_addr->ctrl); if (mode == 'h') - temp |= RSTMGR_CTRL_SWCOLDRSTREQ; + temp |= 1; /* RSTMGR_CTRL_SWCOLDRSTREQ, cold reset */ else - temp |= RSTMGR_CTRL_SWWARMRSTREQ; - __raw_writel(temp, rst_manager_base_addr + SOCFPGA_RSTMGR_CTRL); + temp |= 2; /* RSTMGR_CTRL_SWWARMRSTREQ, warm reset */ + writel(temp, &rst_manager_base_addr->ctrl); } static void __init socfpga_cyclone5_init(void) diff --git a/arch/arm/mach-socfpga/platsmp.c b/arch/arm/mach-socfpga/platsmp.c index c75c33d..822a93e 100644 --- a/arch/arm/mach-socfpga/platsmp.c +++ b/arch/arm/mach-socfpga/platsmp.c @@ -47,7 +47,7 @@ static int __cpuinit socfpga_boot_secondary(unsigned int cpu, struct task_struct if (cpu1start_addr) { memcpy(phys_to_virt(0), &secondary_trampoline, trampoline_size); - __raw_writel(virt_to_phys(socfpga_secondary_startup), + writel(virt_to_phys(socfpga_secondary_startup), (sys_manager_base_addr + (cpu1start_addr & 0x000000ff))); flush_cache_all(); @@ -55,7 +55,7 @@ static int __cpuinit socfpga_boot_secondary(unsigned int cpu, struct task_struct outer_clean_range(0, trampoline_size); /* This will release CPU #1 out of reset.*/ - __raw_writel(0, rst_manager_base_addr + 0x10); + writel(0, &rst_manager_base_addr->mpumodrst); } return 0; @@ -101,7 +101,7 @@ static void socfpga_cpu_die(unsigned int cpu) flush_cache_all(); /* This will put CPU1 into reset.*/ - __raw_writel(RSTMGR_MPUMODRST_CPU1, rst_manager_base_addr + 0x10); + writel(RSTMGR_MPUMODRST_CPU1, &rst_manager_base_addr->mpumodrst); cpu_do_idle();