Hi!
> From: Alan Tull <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Wed, 1 Oct 2014, Pavel Machek wrote:
> Hi!
>
> > From: Alan Tull <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
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
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
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