| Message ID | 1411590449-9794-2-git-send-email-atull@opensource.altera.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Sep 24, 2014 at 03:27:28PM -0500, atull@opensource.altera.com wrote: > diff --git a/arch/arm/mach-socfpga/platsmp.c b/arch/arm/mach-socfpga/platsmp.c > index 5356a72..1d5f8ad 100644 > --- a/arch/arm/mach-socfpga/platsmp.c > +++ b/arch/arm/mach-socfpga/platsmp.c > @@ -34,6 +34,10 @@ static int socfpga_boot_secondary(unsigned int cpu, struct task_struct *idle) > int trampoline_size = &secondary_trampoline_end - &secondary_trampoline; > > if (cpu1start_addr) { > + /* This will put CPU #1 into reset.*/ > + __raw_writel(RSTMGR_MPUMODRST_CPU1, > + rst_manager_base_addr + 0x10); If you can place CPU1 into reset, then why not place it into reset during hot unplug? > @@ -86,10 +90,12 @@ static void __init socfpga_smp_prepare_cpus(unsigned int max_cpus) > */ > static void socfpga_cpu_die(unsigned int cpu) > { > - cpu_do_idle(); > + /* Flush the L1 data cache. */ > + flush_cache_all(); Why do you think that's necessary? This potentially flushes *all* levels of the cache, including L2, which is not a nice thing to do if you have another CPU running. Secondly, the core code has already called flush_cache_louis() _twice_ for you immediately prior to calling your cpu_die() function explicitly to remove any L1 data. The only data which should remain are speculative prefetches and stack data specific to _this_ CPU (which could include dirty cache lines associated with the stack frame to enter your cpu_die function.) None of these cache lines are of any interest to other CPUs in the system, so there's no need for them to be written back prior to the CPU being reset or powered down for hot unplug.
On Wed, 24 Sep 2014, Russell King - ARM Linux wrote: Hi Russell, > On Wed, Sep 24, 2014 at 03:27:28PM -0500, atull@opensource.altera.com wrote: > > diff --git a/arch/arm/mach-socfpga/platsmp.c b/arch/arm/mach-socfpga/platsmp.c > > index 5356a72..1d5f8ad 100644 > > --- a/arch/arm/mach-socfpga/platsmp.c > > +++ b/arch/arm/mach-socfpga/platsmp.c > > @@ -34,6 +34,10 @@ static int socfpga_boot_secondary(unsigned int cpu, struct task_struct *idle) > > int trampoline_size = &secondary_trampoline_end - &secondary_trampoline; > > > > if (cpu1start_addr) { > > + /* This will put CPU #1 into reset.*/ > > + __raw_writel(RSTMGR_MPUMODRST_CPU1, > > + rst_manager_base_addr + 0x10); > > If you can place CPU1 into reset, then why not place it into reset during > hot unplug? It's a chip weirdness. We can reset CPU1 briefly, but if we leave it in reset, it results in power consumption going up. > > > @@ -86,10 +90,12 @@ static void __init socfpga_smp_prepare_cpus(unsigned int max_cpus) > > */ > > static void socfpga_cpu_die(unsigned int cpu) > > { > > - cpu_do_idle(); > > + /* Flush the L1 data cache. */ > > + flush_cache_all(); > > Why do you think that's necessary? > > This potentially flushes *all* levels of the cache, including L2, which > is not a nice thing to do if you have another CPU running. Secondly, > the core code has already called flush_cache_louis() _twice_ for you > immediately prior to calling your cpu_die() function explicitly to > remove any L1 data. > > The only data which should remain are speculative prefetches and stack > data specific to _this_ CPU (which could include dirty cache lines > associated with the stack frame to enter your cpu_die function.) > None of these cache lines are of any interest to other CPUs in the > system, so there's no need for them to be written back prior to the > CPU being reset or powered down for hot unplug. > Then it's clear we don't need that. I'll take it out in v2. Thanks for the review! Alan
Hi! > From: Alan Tull <atull@opensource.altera.com> > > Use WFI when putting CPU1 to sleep. Don't hold CPU1 in reset > since that results in increased power consumption. > > Reset CPU1 briefly during CPU1 bootup. > > This has been tested for hotplug and suspend/resume and results > in no increased power consumption. > > Signed-off-by: Alan Tull <atull@opensource.altera.com> > --- > arch/arm/mach-socfpga/core.h | 2 ++ > arch/arm/mach-socfpga/platsmp.c | 12 +++++++++--- > 2 files changed, 11 insertions(+), 3 deletions(-) > > diff --git a/arch/arm/mach-socfpga/core.h b/arch/arm/mach-socfpga/core.h > index 572b8f7..c4a0929 100644 > --- a/arch/arm/mach-socfpga/core.h > +++ b/arch/arm/mach-socfpga/core.h > @@ -28,6 +28,8 @@ > #define RSTMGR_CTRL_SWCOLDRSTREQ 0x1 /* Cold Reset */ > #define RSTMGR_CTRL_SWWARMRSTREQ 0x2 /* Warm Reset */ > > +#define RSTMGR_MPUMODRST_CPU1 0x2 /*CPU1 Reset*/ > + It would be nice to have space after /* and before */. > diff --git a/arch/arm/mach-socfpga/platsmp.c b/arch/arm/mach-socfpga/platsmp.c > index 5356a72..1d5f8ad 100644 > --- a/arch/arm/mach-socfpga/platsmp.c > +++ b/arch/arm/mach-socfpga/platsmp.c > @@ -34,6 +34,10 @@ static int socfpga_boot_secondary(unsigned int cpu, struct task_struct *idle) > int trampoline_size = &secondary_trampoline_end - &secondary_trampoline; > > if (cpu1start_addr) { > + /* This will put CPU #1 into reset.*/ Same here. > + __raw_writel(RSTMGR_MPUMODRST_CPU1, > + rst_manager_base_addr + 0x10); Would it be possible to copy reset manager description struct from u-boot and use it here, instead of raw offset? Pavel
On Wed, 1 Oct 2014, Pavel Machek wrote: > Hi! > > > From: Alan Tull <atull@opensource.altera.com> > > > > Use WFI when putting CPU1 to sleep. Don't hold CPU1 in reset > > since that results in increased power consumption. > > > > Reset CPU1 briefly during CPU1 bootup. > > > > This has been tested for hotplug and suspend/resume and results > > in no increased power consumption. > > > > Signed-off-by: Alan Tull <atull@opensource.altera.com> > > --- > > arch/arm/mach-socfpga/core.h | 2 ++ > > arch/arm/mach-socfpga/platsmp.c | 12 +++++++++--- > > 2 files changed, 11 insertions(+), 3 deletions(-) > > > > diff --git a/arch/arm/mach-socfpga/core.h b/arch/arm/mach-socfpga/core.h > > index 572b8f7..c4a0929 100644 > > --- a/arch/arm/mach-socfpga/core.h > > +++ b/arch/arm/mach-socfpga/core.h > > @@ -28,6 +28,8 @@ > > #define RSTMGR_CTRL_SWCOLDRSTREQ 0x1 /* Cold Reset */ > > #define RSTMGR_CTRL_SWWARMRSTREQ 0x2 /* Warm Reset */ > > > > +#define RSTMGR_MPUMODRST_CPU1 0x2 /*CPU1 Reset*/ > > + > > It would be nice to have space after /* and before */. Hi Pavel, I will fix the comment space here and the other place you pointed out. > > > diff --git a/arch/arm/mach-socfpga/platsmp.c b/arch/arm/mach-socfpga/platsmp.c > > index 5356a72..1d5f8ad 100644 > > --- a/arch/arm/mach-socfpga/platsmp.c > > +++ b/arch/arm/mach-socfpga/platsmp.c > > @@ -34,6 +34,10 @@ static int socfpga_boot_secondary(unsigned int cpu, struct task_struct *idle) > > int trampoline_size = &secondary_trampoline_end - &secondary_trampoline; > > > > if (cpu1start_addr) { > > + /* This will put CPU #1 into reset.*/ > > Same here. > > > + __raw_writel(RSTMGR_MPUMODRST_CPU1, > > + rst_manager_base_addr + 0x10); > > Would it be possible to copy reset manager description struct from > u-boot and use it here, instead of raw offset? I will replace this 0x10 with a macro that reflects how the register is named in the register map. Thanks for the review! Alan > Pavel > -- > (english) http://www.livejournal.com/~pavelmachek > (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html >
Hi! > > > + __raw_writel(RSTMGR_MPUMODRST_CPU1, > > > + rst_manager_base_addr + 0x10); > > > > Would it be possible to copy reset manager description struct from > > u-boot and use it here, instead of raw offset? > > I will replace this 0x10 with a macro that reflects how the register is > named in the register map. That would be better than 0x10, but even better would be just copying struct socfpga_reset_manager { u32 status; u32 ctrl; u32 counts; u32 padding1; u32 mpu_mod_reset; u32 per_mod_reset; u32 per2_mod_reset; u32 brg_mod_reset; }; from u-boot. Unlike macros, structs have advantages that typos lead to easier-to-see failure modes... (And they are easier to read/parse, too). Thanks, Pavel
On 10/1/14, 10:04 AM, Pavel Machek wrote: > Hi! > >>>> + __raw_writel(RSTMGR_MPUMODRST_CPU1, >>>> + rst_manager_base_addr + 0x10); >>> >>> Would it be possible to copy reset manager description struct from >>> u-boot and use it here, instead of raw offset? >> >> I will replace this 0x10 with a macro that reflects how the register is >> named in the register map. > > That would be better than 0x10, but even better would be just copying > > struct socfpga_reset_manager { > u32 status; > u32 ctrl; > u32 counts; > u32 padding1; > u32 mpu_mod_reset; > u32 per_mod_reset; > u32 per2_mod_reset; > u32 brg_mod_reset; > }; > > from u-boot. Unlike macros, structs have advantages that typos lead to > easier-to-see failure modes... (And they are easier to read/parse, > too). > Copying from uboot sounds good, but I already know that the CPU reset offset is different for our next SOC, Arria 10. The Arria 10 SOC should still be able to use the same MSL as Cyclone5 and Arria5, but with a few differences. One of them being, the CPU1 reset offset is at 0x20 instead of 0x10. So I think having a macro for this one register is a bit cleaner than having to define a whole new struct for Arria10. if (of_machine_is_compatible("altr,socfpga-arria10")) __raw_writel(0, rst_manager_base_addr + 0x20); else __raw_writel(0, rst_manager_base_addr + 0x10); Dinh
On Wed 2014-10-01 11:07:18, Dinh Nguyen wrote: > > > On 10/1/14, 10:04 AM, Pavel Machek wrote: > > Hi! > > > >>>> + __raw_writel(RSTMGR_MPUMODRST_CPU1, > >>>> + rst_manager_base_addr + 0x10); > >>> > >>> Would it be possible to copy reset manager description struct from > >>> u-boot and use it here, instead of raw offset? > >> > >> I will replace this 0x10 with a macro that reflects how the register is > >> named in the register map. > > > > That would be better than 0x10, but even better would be just copying > > > > struct socfpga_reset_manager { > > u32 status; > > u32 ctrl; > > u32 counts; > > u32 padding1; > > u32 mpu_mod_reset; > > u32 per_mod_reset; > > u32 per2_mod_reset; > > u32 brg_mod_reset; > > }; > > > > from u-boot. Unlike macros, structs have advantages that typos lead to > > easier-to-see failure modes... (And they are easier to read/parse, > > too). > > > > Copying from uboot sounds good, but I already know that the CPU reset > offset is different for our next SOC, Arria 10. The Arria 10 SOC should > still be able to use the same MSL as Cyclone5 and Arria5, but with a few > differences. One of them being, the CPU1 reset offset is at 0x20 instead > of 0x10. So I think having a macro for this one register is a bit > cleaner than having to define a whole new struct for Arria10. I don't think "whole new struct" is a problem. At least it will be plain to see what changed (which will get easily lost in ifdefs. struct cyclone5_reset_manager { struct socfpga_reset_manager common; u32 brg_mod_reset; } struct aria10_reset_manager { struct socfpga_reset_manager common; char filler[0x10]; u32 brg_mod_reset; } if (of_machine_is_compatible("altr,socfpga-arria10")) __raw_writel(0, (struct cyclone5_reset_manager *) rst_manager_base_addr->brg_mod_reset)); else __raw_writel(0, (struct aria10_reset_manager *) rst_manager_base_addr->brg_mod_reset)); ...does not sound that bad. (And you'll need some nice solution for u-boot, anyway...) Best regards, Pavel
On 10/1/14, 6:16 PM, Pavel Machek wrote: > On Wed 2014-10-01 11:07:18, Dinh Nguyen wrote: >> >> >> On 10/1/14, 10:04 AM, Pavel Machek wrote: >>> Hi! >>> >>>>>> + __raw_writel(RSTMGR_MPUMODRST_CPU1, >>>>>> + rst_manager_base_addr + 0x10); >>>>> >>>>> Would it be possible to copy reset manager description struct from >>>>> u-boot and use it here, instead of raw offset? >>>> >>>> I will replace this 0x10 with a macro that reflects how the register is >>>> named in the register map. >>> >>> That would be better than 0x10, but even better would be just copying >>> >>> struct socfpga_reset_manager { >>> u32 status; >>> u32 ctrl; >>> u32 counts; >>> u32 padding1; >>> u32 mpu_mod_reset; >>> u32 per_mod_reset; >>> u32 per2_mod_reset; >>> u32 brg_mod_reset; >>> }; >>> >>> from u-boot. Unlike macros, structs have advantages that typos lead to >>> easier-to-see failure modes... (And they are easier to read/parse, >>> too). >>> >> >> Copying from uboot sounds good, but I already know that the CPU reset >> offset is different for our next SOC, Arria 10. The Arria 10 SOC should >> still be able to use the same MSL as Cyclone5 and Arria5, but with a few >> differences. One of them being, the CPU1 reset offset is at 0x20 instead >> of 0x10. So I think having a macro for this one register is a bit >> cleaner than having to define a whole new struct for Arria10. > > I don't think "whole new struct" is a problem. At least it will be > plain to see what changed (which will get easily lost in ifdefs. > > struct cyclone5_reset_manager { > struct socfpga_reset_manager common; > u32 brg_mod_reset; > } > > struct aria10_reset_manager { > struct socfpga_reset_manager common; > char filler[0x10]; > u32 brg_mod_reset; > } > > if (of_machine_is_compatible("altr,socfpga-arria10")) > __raw_writel(0, (struct cyclone5_reset_manager *) rst_manager_base_addr->brg_mod_reset)); > else > __raw_writel(0, (struct aria10_reset_manager *) rst_manager_base_addr->brg_mod_reset)); > > ...does not sound that bad. (And you'll need some nice solution for > u-boot, anyway...) > That's fine. Dinh
On Thursday 02 October 2014 01:16:46 Pavel Machek wrote: > > > > > > struct socfpga_reset_manager { > > > u32 status; > > > u32 ctrl; > > > u32 counts; > > > u32 padding1; > > > u32 mpu_mod_reset; > > > u32 per_mod_reset; > > > u32 per2_mod_reset; > > > u32 brg_mod_reset; > > > }; > > > > > > from u-boot. Unlike macros, structs have advantages that typos lead to > > > easier-to-see failure modes... (And they are easier to read/parse, > > > too). > > > > > > > Copying from uboot sounds good, but I already know that the CPU reset > > offset is different for our next SOC, Arria 10. The Arria 10 SOC should > > still be able to use the same MSL as Cyclone5 and Arria5, but with a few > > differences. One of them being, the CPU1 reset offset is at 0x20 instead > > of 0x10. So I think having a macro for this one register is a bit > > cleaner than having to define a whole new struct for Arria10. > > I don't think "whole new struct" is a problem. At least it will be > plain to see what changed (which will get easily lost in ifdefs. > > struct cyclone5_reset_manager { > struct socfpga_reset_manager common; > u32 brg_mod_reset; > } > > struct aria10_reset_manager { > struct socfpga_reset_manager common; > char filler[0x10]; > u32 brg_mod_reset; > } > > if (of_machine_is_compatible("altr,socfpga-arria10")) > __raw_writel(0, (struct cyclone5_reset_manager *) rst_manager_base_addr->brg_mod_reset)); > else > __raw_writel(0, (struct aria10_reset_manager *) rst_manager_base_addr->brg_mod_reset)); > > ...does not sound that bad. (And you'll need some nice solution for > u-boot, anyway...) I think it would be better to just add more fields and access a different field based on the SoC type than cast the structs around. Also, never use __raw_writel unless you know exactly what you are doing. This should use writel, or possibly writel_relaxed. Finally, don't sprinkle of_machine_is_compatible() checks all over the place. Make the decision once when you initially probe the machine. Arnd
On Thu, 2 Oct 2014, Arnd Bergmann wrote: > On Thursday 02 October 2014 01:16:46 Pavel Machek wrote: > > > > > > > > struct socfpga_reset_manager { > > > > u32 status; > > > > u32 ctrl; > > > > u32 counts; > > > > u32 padding1; > > > > u32 mpu_mod_reset; > > > > u32 per_mod_reset; > > > > u32 per2_mod_reset; > > > > u32 brg_mod_reset; > > > > }; > > > > > > > > from u-boot. Unlike macros, structs have advantages that typos lead to > > > > easier-to-see failure modes... (And they are easier to read/parse, > > > > too). > > > > > > > > > > Copying from uboot sounds good, but I already know that the CPU reset > > > offset is different for our next SOC, Arria 10. The Arria 10 SOC should > > > still be able to use the same MSL as Cyclone5 and Arria5, but with a few > > > differences. One of them being, the CPU1 reset offset is at 0x20 instead > > > of 0x10. So I think having a macro for this one register is a bit > > > cleaner than having to define a whole new struct for Arria10. > > > > I don't think "whole new struct" is a problem. At least it will be > > plain to see what changed (which will get easily lost in ifdefs. > > > > struct cyclone5_reset_manager { > > struct socfpga_reset_manager common; > > u32 brg_mod_reset; > > } > > > > struct aria10_reset_manager { > > struct socfpga_reset_manager common; > > char filler[0x10]; > > u32 brg_mod_reset; > > } > > > > if (of_machine_is_compatible("altr,socfpga-arria10")) > > __raw_writel(0, (struct cyclone5_reset_manager *) rst_manager_base_addr->brg_mod_reset)); > > else > > __raw_writel(0, (struct aria10_reset_manager *) rst_manager_base_addr->brg_mod_reset)); > > > > ...does not sound that bad. (And you'll need some nice solution for > > u-boot, anyway...) > > I think it would be better to just add more fields and access a different > field based on the SoC type than cast the structs around. > > Also, never use __raw_writel unless you know exactly what you are doing. > This should use writel, or possibly writel_relaxed. Arnd, Pavel, I appreciate the comments. I will fix this to not be a __raw_writel. > > Finally, don't sprinkle of_machine_is_compatible() checks all over the > place. Make the decision once when you initially probe the machine. > > Arnd > The changes for aria10 are minor: a different DT plus two register changes. I'm not introducing aria10 support in this patch. This is a 16 line patch for fixing something in an established machine layer. If I have to come up with a new scheme for accessing registers, then I will need to touch other code that this patch does not intend to change. Alan
diff --git a/arch/arm/mach-socfpga/core.h b/arch/arm/mach-socfpga/core.h index 572b8f7..c4a0929 100644 --- a/arch/arm/mach-socfpga/core.h +++ b/arch/arm/mach-socfpga/core.h @@ -28,6 +28,8 @@ #define RSTMGR_CTRL_SWCOLDRSTREQ 0x1 /* Cold Reset */ #define RSTMGR_CTRL_SWWARMRSTREQ 0x2 /* Warm Reset */ +#define RSTMGR_MPUMODRST_CPU1 0x2 /*CPU1 Reset*/ + extern void socfpga_secondary_startup(void); extern void __iomem *socfpga_scu_base_addr; diff --git a/arch/arm/mach-socfpga/platsmp.c b/arch/arm/mach-socfpga/platsmp.c index 5356a72..1d5f8ad 100644 --- a/arch/arm/mach-socfpga/platsmp.c +++ b/arch/arm/mach-socfpga/platsmp.c @@ -34,6 +34,10 @@ static int socfpga_boot_secondary(unsigned int cpu, struct task_struct *idle) int trampoline_size = &secondary_trampoline_end - &secondary_trampoline; if (cpu1start_addr) { + /* This will put CPU #1 into reset.*/ + __raw_writel(RSTMGR_MPUMODRST_CPU1, + rst_manager_base_addr + 0x10); + memcpy(phys_to_virt(0), &secondary_trampoline, trampoline_size); __raw_writel(virt_to_phys(socfpga_secondary_startup), @@ -86,10 +90,12 @@ static void __init socfpga_smp_prepare_cpus(unsigned int max_cpus) */ static void socfpga_cpu_die(unsigned int cpu) { - cpu_do_idle(); + /* Flush the L1 data cache. */ + flush_cache_all(); - /* We should have never returned from idle */ - panic("cpu %d unexpectedly exit from shutdown\n", cpu); + /* Do WFI. If we wake up early, go back into WFI */ + while (1) + cpu_do_idle(); } struct smp_operations socfpga_smp_ops __initdata = {