Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1490829 > unrolled thread
| Started by | Scott Wood <oss@buserror.net> |
|---|---|
| First post | 2016-09-25 09:30 +0200 |
| Last post | 2016-10-03 19:30 +0200 |
| Articles | 5 — 2 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [v3,4/5] powerpc/pm: support deep sleep feature on T104x Scott Wood <oss@buserror.net> - 2016-09-25 09:30 +0200
Re: [v3,4/5] powerpc/pm: support deep sleep feature on T104x "C.H. Zhao" <chenhui.zhao@nxp.com> - 2016-09-27 23:20 +0200
Re: [v3,4/5] powerpc/pm: support deep sleep feature on T104x Scott Wood <oss@buserror.net> - 2016-09-28 22:10 +0200
Re: [v3,4/5] powerpc/pm: support deep sleep feature on T104x "C.H. Zhao" <chenhui.zhao@nxp.com> - 2016-09-29 12:30 +0200
Re: [v3,4/5] powerpc/pm: support deep sleep feature on T104x Scott Wood <oss@buserror.net> - 2016-10-03 19:30 +0200
| From | Scott Wood <oss@buserror.net> |
|---|---|
| Date | 2016-09-25 09:30 +0200 |
| Subject | Re: [v3,4/5] powerpc/pm: support deep sleep feature on T104x |
| Message-ID | <slc2R-3CK-13@gated-at.bofh.it> |
On Tue, Aug 02, 2016 at 07:59:31PM +0800, Chenhui Zhao wrote:
> T104x has deep sleep feature, which can switch off most parts of
> the SoC when it is in deep sleep mode. This way, it becomes more
> energy-efficient.
>
> The DDR controller will also be powered off in deep sleep. Therefore,
> the last stage (the latter part of fsl_dp_enter_low) will run without DDR
> access. This piece of code and related TLBs are prefetched in advance.
>
> Due to the different initialization code between 32-bit and 64-bit, they
> have separate resume entry and precedure.
>
> The feature supports 32-bit and 64-bit kernel mode.
>
> Signed-off-by: Chenhui Zhao <chenhui.zhao@nxp.com>
> ---
> arch/powerpc/include/asm/fsl_pm.h | 24 ++
> arch/powerpc/kernel/asm-offsets.c | 12 +
> arch/powerpc/kernel/fsl_booke_entry_mapping.S | 10 +
> arch/powerpc/kernel/head_64.S | 2 +-
> arch/powerpc/platforms/85xx/Makefile | 1 +
> arch/powerpc/platforms/85xx/deepsleep.c | 278 ++++++++++++++
> arch/powerpc/platforms/85xx/qoriq_pm.c | 25 ++
> arch/powerpc/platforms/85xx/t104x_deepsleep.S | 531 ++++++++++++++++++++++++++
> arch/powerpc/sysdev/fsl_rcpm.c | 8 +-
> 9 files changed, 889 insertions(+), 2 deletions(-)
> create mode 100644 arch/powerpc/platforms/85xx/deepsleep.c
> create mode 100644 arch/powerpc/platforms/85xx/t104x_deepsleep.S
>
> diff --git a/arch/powerpc/include/asm/fsl_pm.h b/arch/powerpc/include/asm/fsl_pm.h
> index e05049b..48c2631 100644
> --- a/arch/powerpc/include/asm/fsl_pm.h
> +++ b/arch/powerpc/include/asm/fsl_pm.h
> @@ -20,6 +20,7 @@
>
> #define PLAT_PM_SLEEP 20
> #define PLAT_PM_LPM20 30
> +#define PLAT_PM_LPM35 40
>
> #define FSL_PM_SLEEP (1 << 0)
> #define FSL_PM_DEEP_SLEEP (1 << 1)
> @@ -48,4 +49,27 @@ extern const struct fsl_pm_ops *qoriq_pm_ops;
>
> int __init fsl_rcpm_init(void);
>
> +#ifdef CONFIG_FSL_QORIQ_PM
> +int fsl_enter_deepsleep(void);
> +int fsl_deepsleep_init(void);
> +#else
> +static inline int fsl_enter_deepsleep(void) { return -1; }
> +static inline int fsl_deepsleep_init(void) { return -1; }
> +#endif
Please return proper error codes.
Where can fsl_deepsleep_init() be called without CONFIG_FSL_QORIQ_PM?
> +
> +extern void fsl_dp_enter_low(void *priv);
> +extern void fsl_booke_deep_sleep_resume(void);
> +
> +struct fsl_iomap {
> + void *ccsr_scfg_base;
> + void *ccsr_rcpm_base;
> + void *ccsr_ddr_base;
> + void *ccsr_gpio1_base;
> + void *ccsr_cpc_base;
> + void *dcsr_epu_base;
> + void *dcsr_npc_base;
> + void *dcsr_rcpm_base;
> + void *cpld_base;
> + void *fpga_base;
> +};
__iomem
> #endif /* __PPC_FSL_PM_H */
> diff --git a/arch/powerpc/kernel/asm-offsets.c b/arch/powerpc/kernel/asm-offsets.c
> index 9ea0955..cc488f9 100644
> --- a/arch/powerpc/kernel/asm-offsets.c
> +++ b/arch/powerpc/kernel/asm-offsets.c
> @@ -68,6 +68,10 @@
> #include "../mm/mmu_decl.h"
> #endif
>
> +#ifdef CONFIG_FSL_QORIQ_PM
> +#include <asm/fsl_pm.h>
> +#endif
I know this file ifdefs headers a lot, but it's generally not good
practice. Does including this file cause any harm on other platforms?
> diff --git a/arch/powerpc/kernel/fsl_booke_entry_mapping.S b/arch/powerpc/kernel/fsl_booke_entry_mapping.S
> index 83dd0f6..659b059 100644
> --- a/arch/powerpc/kernel/fsl_booke_entry_mapping.S
> +++ b/arch/powerpc/kernel/fsl_booke_entry_mapping.S
> @@ -173,6 +173,10 @@ skpinv: addi r6,r6,1 /* Increment */
> lis r6,MAS2_VAL(PAGE_OFFSET, BOOK3E_PAGESZ_64M, M_IF_NEEDED)@h
> ori r6,r6,MAS2_VAL(PAGE_OFFSET, BOOK3E_PAGESZ_64M, M_IF_NEEDED)@l
> mtspr SPRN_MAS2,r6
> +#ifdef ENTRY_DEEPSLEEP_SETUP
> + LOAD_REG_IMMEDIATE(r8, MEMORY_START)
> + ori r8,r8,(MAS3_SX|MAS3_SW|MAS3_SR)
> +#endif
> mtspr SPRN_MAS3,r8
> tlbwe
>
Have you tried this with a relocatable kernel?
> +static void fsl_dp_set_resume_pointer(void)
> +{
> + u32 resume_addr;
> +
> + /* the bootloader will finally jump to this address to return kernel */
> +#ifdef CONFIG_PPC32
> + resume_addr = (u32)(__pa(fsl_booke_deep_sleep_resume));
> +#else
> + resume_addr = (u32)(__pa(*(u64 *)fsl_booke_deep_sleep_resume)
> + & 0xffffffff);
> +#endif
Why are you masking the physical address by 0xffffffff? Besides the
(u32) cast accomplishing the same thing, wouldn't it be a problem if
(e.g. due to a relocatable kernel) the address is above 4 GiB?
> +static void fsl_dp_pins_setup(void)
> +{
> + u32 mask = BIT(31 - fsl_gpio_mcke);
> +
> + /* set GPIO1_29 as an output pin (not open-drain), and output 0 */
> + clrbits32(fsl_dp_priv.ccsr_gpio1_base + CCSR_GPIO1_GPDAT, mask);
> + clrbits32(fsl_dp_priv.ccsr_gpio1_base + CCSR_GPIO1_GPODR, mask);
> + setbits32(fsl_dp_priv.ccsr_gpio1_base + CCSR_GPIO1_GPDIR, mask);
> +
> + /* wait for the stabilization of GPIO1_29 */
> + udelay(10);
> +
> + /* enable the functionality of pins relevant to deep sleep */
> + if (fsl_dp_priv.cpld_base) {
> + setbits8(fsl_dp_priv.cpld_base + QORIQ_CPLD_MISCCSR,
> + QORIQ_CPLD_MISCCSR_SLEEPEN);
> + } else if (fsl_dp_priv.fpga_base) {
> + setbits8(fsl_dp_priv.fpga_base + QIXIS_PWR_CTL2,
> + QIXIS_PWR_CTL2_PCTL);
> + }
> +}
Please use callbacks to handle board-specific things.
> +/* reset time base to prevent from overflow */
> +#define DELAY(count) \
> + li r3, count; \
> + li r4, 0; \
> + mtspr SPRN_TBWL, r4; \
> +101: mfspr r4, SPRN_TBRL; \
> + cmpw r4, r3; \
> + blt 101b
Please find a better way of dealing with overflow than writing to the
timebase.
-Scott
[toc] | [next] | [standalone]
| From | "C.H. Zhao" <chenhui.zhao@nxp.com> |
|---|---|
| Date | 2016-09-27 23:20 +0200 |
| Message-ID | <sm7Xb-6ec-19@gated-at.bofh.it> |
| In reply to | #1490829 |
From: Scott Wood <oss@buserror.net>
Sent: Sunday, September 25, 2016 3:24 PM
To: C.H. Zhao
Cc: linuxppc-dev@lists.ozlabs.org; linux-kernel@vger.kernel.org; z.chenhui@gmail.com; Jason Jin
Subject: Re: [v3,4/5] powerpc/pm: support deep sleep feature on T104x
On Tue, Aug 02, 2016 at 07:59:31PM +0800, Chenhui Zhao wrote:
> T104x has deep sleep feature, which can switch off most parts of
> the SoC when it is in deep sleep mode. This way, it becomes more
> energy-efficient.
>
> The DDR controller will also be powered off in deep sleep. Therefore,
> the last stage (the latter part of fsl_dp_enter_low) will run without DDR
> access. This piece of code and related TLBs are prefetched in advance.
>
> Due to the different initialization code between 32-bit and 64-bit, they
> have separate resume entry and precedure.
>
> The feature supports 32-bit and 64-bit kernel mode.
>
> Signed-off-by: Chenhui Zhao <chenhui.zhao@nxp.com>
> ---
> arch/powerpc/include/asm/fsl_pm.h | 24 ++
> arch/powerpc/kernel/asm-offsets.c | 12 +
> arch/powerpc/kernel/fsl_booke_entry_mapping.S | 10 +
> arch/powerpc/kernel/head_64.S | 2 +-
> arch/powerpc/platforms/85xx/Makefile | 1 +
> arch/powerpc/platforms/85xx/deepsleep.c | 278 ++++++++++++++
> arch/powerpc/platforms/85xx/qoriq_pm.c | 25 ++
> arch/powerpc/platforms/85xx/t104x_deepsleep.S | 531 ++++++++++++++++++++++++++
> arch/powerpc/sysdev/fsl_rcpm.c | 8 +-
> 9 files changed, 889 insertions(+), 2 deletions(-)
> create mode 100644 arch/powerpc/platforms/85xx/deepsleep.c
> create mode 100644 arch/powerpc/platforms/85xx/t104x_deepsleep.S
>
> diff --git a/arch/powerpc/include/asm/fsl_pm.h b/arch/powerpc/include/asm/fsl_pm.h
> index e05049b..48c2631 100644
> --- a/arch/powerpc/include/asm/fsl_pm.h
> +++ b/arch/powerpc/include/asm/fsl_pm.h
> @@ -20,6 +20,7 @@
>
> #define PLAT_PM_SLEEP 20
> #define PLAT_PM_LPM20 30
> +#define PLAT_PM_LPM35 40
>
> #define FSL_PM_SLEEP (1 << 0)
> #define FSL_PM_DEEP_SLEEP (1 << 1)
> @@ -48,4 +49,27 @@ extern const struct fsl_pm_ops *qoriq_pm_ops;
>
> int __init fsl_rcpm_init(void);
>
> +#ifdef CONFIG_FSL_QORIQ_PM
> +int fsl_enter_deepsleep(void);
> +int fsl_deepsleep_init(void);
> +#else
> +static inline int fsl_enter_deepsleep(void) { return -1; }
> +static inline int fsl_deepsleep_init(void) { return -1; }
> +#endif
Please return proper error codes.
Where can fsl_deepsleep_init() be called without CONFIG_FSL_QORIQ_PM?
[Chenhui] I can get rid of the ifdef here. And add it in arch/powerpc/sysdev/fsl_rcpm.c.
> +
> +extern void fsl_dp_enter_low(void *priv);
> +extern void fsl_booke_deep_sleep_resume(void);
> +
> +struct fsl_iomap {
> + void *ccsr_scfg_base;
> + void *ccsr_rcpm_base;
> + void *ccsr_ddr_base;
> + void *ccsr_gpio1_base;
> + void *ccsr_cpc_base;
> + void *dcsr_epu_base;
> + void *dcsr_npc_base;
> + void *dcsr_rcpm_base;
> + void *cpld_base;
> + void *fpga_base;
> +};
__iomem
[Chenhui] Yes. Will add it.
> #endif /* __PPC_FSL_PM_H */
> diff --git a/arch/powerpc/kernel/asm-offsets.c b/arch/powerpc/kernel/asm-offsets.c
> index 9ea0955..cc488f9 100644
> --- a/arch/powerpc/kernel/asm-offsets.c
> +++ b/arch/powerpc/kernel/asm-offsets.c
> @@ -68,6 +68,10 @@
> #include "../mm/mmu_decl.h"
> #endif
>
> +#ifdef CONFIG_FSL_QORIQ_PM
> +#include <asm/fsl_pm.h>
> +#endif
I know this file ifdefs headers a lot, but it's generally not good
practice. Does including this file cause any harm on other platforms?
[Chenhui] Not at all. Will remove it.
> diff --git a/arch/powerpc/kernel/fsl_booke_entry_mapping.S b/arch/powerpc/kernel/fsl_booke_entry_mapping.S
> index 83dd0f6..659b059 100644
> --- a/arch/powerpc/kernel/fsl_booke_entry_mapping.S
> +++ b/arch/powerpc/kernel/fsl_booke_entry_mapping.S
> @@ -173,6 +173,10 @@ skpinv: addi r6,r6,1 /* Increment */
> lis r6,MAS2_VAL(PAGE_OFFSET, BOOK3E_PAGESZ_64M, M_IF_NEEDED)@h
> ori r6,r6,MAS2_VAL(PAGE_OFFSET, BOOK3E_PAGESZ_64M, M_IF_NEEDED)@l
> mtspr SPRN_MAS2,r6
> +#ifdef ENTRY_DEEPSLEEP_SETUP
> + LOAD_REG_IMMEDIATE(r8, MEMORY_START)
> + ori r8,r8,(MAS3_SX|MAS3_SW|MAS3_SR)
> +#endif
> mtspr SPRN_MAS3,r8
> tlbwe
>
Have you tried this with a relocatable kernel?
[Chenhui] Not yet. Not sure whether it has been supported on QorIQ platform.
> +static void fsl_dp_set_resume_pointer(void)
> +{
> + u32 resume_addr;
> +
> + /* the bootloader will finally jump to this address to return kernel */
> +#ifdef CONFIG_PPC32
> + resume_addr = (u32)(__pa(fsl_booke_deep_sleep_resume));
> +#else
> + resume_addr = (u32)(__pa(*(u64 *)fsl_booke_deep_sleep_resume)
> + & 0xffffffff);
> +#endif
Why are you masking the physical address by 0xffffffff? Besides the
(u32) cast accomplishing the same thing, wouldn't it be a problem if
(e.g. due to a relocatable kernel) the address is above 4 GiB?
[Chenhui] Here, I assumed kernel is below 4 GiB. Maybe I should add a comment here.
> +static void fsl_dp_pins_setup(void)
> +{
> + u32 mask = BIT(31 - fsl_gpio_mcke);
> +
> + /* set GPIO1_29 as an output pin (not open-drain), and output 0 */
> + clrbits32(fsl_dp_priv.ccsr_gpio1_base + CCSR_GPIO1_GPDAT, mask);
> + clrbits32(fsl_dp_priv.ccsr_gpio1_base + CCSR_GPIO1_GPODR, mask);
> + setbits32(fsl_dp_priv.ccsr_gpio1_base + CCSR_GPIO1_GPDIR, mask);
> +
> + /* wait for the stabilization of GPIO1_29 */
> + udelay(10);
> +
> + /* enable the functionality of pins relevant to deep sleep */
> + if (fsl_dp_priv.cpld_base) {
> + setbits8(fsl_dp_priv.cpld_base + QORIQ_CPLD_MISCCSR,
> + QORIQ_CPLD_MISCCSR_SLEEPEN);
> + } else if (fsl_dp_priv.fpga_base) {
> + setbits8(fsl_dp_priv.fpga_base + QIXIS_PWR_CTL2,
> + QIXIS_PWR_CTL2_PCTL);
> + }
> +}
Please use callbacks to handle board-specific things.
[Chenhui] Yes. Will do it as you said.
> +/* reset time base to prevent from overflow */
> +#define DELAY(count) \
> + li r3, count; \
> + li r4, 0; \
> + mtspr SPRN_TBWL, r4; \
> +101: mfspr r4, SPRN_TBRL; \
> + cmpw r4, r3; \
> + blt 101b
Please find a better way of dealing with overflow than writing to the
timebase.
-Scott
[Chenhui] OK. Let me try other way. Thank you for your time.
[toc] | [prev] | [next] | [standalone]
| From | Scott Wood <oss@buserror.net> |
|---|---|
| Date | 2016-09-28 22:10 +0200 |
| Message-ID | <smtkZ-2SB-15@gated-at.bofh.it> |
| In reply to | #1492179 |
On Tue, 2016-09-27 at 11:05 +0000, C.H. Zhao wrote:
> From: Scott Wood <oss@buserror.net>
> Sent: Sunday, September 25, 2016 3:24 PM
> To: C.H. Zhao
> Cc: linuxppc-dev@lists.ozlabs.org; linux-kernel@vger.kernel.org; z.chenhui@g
> mail.com; Jason Jin
> Subject: Re: [v3,4/5] powerpc/pm: support deep sleep feature on T104x
>
> On Tue, Aug 02, 2016 at 07:59:31PM +0800, Chenhui Zhao wrote:
> >
> > T104x has deep sleep feature, which can switch off most parts of
> > the SoC when it is in deep sleep mode. This way, it becomes more
> > energy-efficient.
> >
> > The DDR controller will also be powered off in deep sleep. Therefore,
> > the last stage (the latter part of fsl_dp_enter_low) will run without DDR
> > access. This piece of code and related TLBs are prefetched in advance.
> >
> > Due to the different initialization code between 32-bit and 64-bit, they
> > have separate resume entry and precedure.
> >
> > The feature supports 32-bit and 64-bit kernel mode.
> >
> > Signed-off-by: Chenhui Zhao <chenhui.zhao@nxp.com>
> > ---
> > arch/powerpc/include/asm/fsl_pm.h | 24 ++
> > arch/powerpc/kernel/asm-offsets.c | 12 +
> > arch/powerpc/kernel/fsl_booke_entry_mapping.S | 10 +
> > arch/powerpc/kernel/head_64.S | 2 +-
> > arch/powerpc/platforms/85xx/Makefile | 1 +
> > arch/powerpc/platforms/85xx/deepsleep.c | 278 ++++++++++++++
> > arch/powerpc/platforms/85xx/qoriq_pm.c | 25 ++
> > arch/powerpc/platforms/85xx/t104x_deepsleep.S | 531
> > ++++++++++++++++++++++++++
> > arch/powerpc/sysdev/fsl_rcpm.c | 8 +-
> > 9 files changed, 889 insertions(+), 2 deletions(-)
> > create mode 100644 arch/powerpc/platforms/85xx/deepsleep.c
> > create mode 100644 arch/powerpc/platforms/85xx/t104x_deepsleep.S
> >
> > diff --git a/arch/powerpc/include/asm/fsl_pm.h
> > b/arch/powerpc/include/asm/fsl_pm.h
> > index e05049b..48c2631 100644
> > --- a/arch/powerpc/include/asm/fsl_pm.h
> > +++ b/arch/powerpc/include/asm/fsl_pm.h
> > @@ -20,6 +20,7 @@
> >
> > #define PLAT_PM_SLEEP 20
> > #define PLAT_PM_LPM20 30
> > +#define PLAT_PM_LPM35 40
> >
> > #define FSL_PM_SLEEP (1 << 0)
> > #define FSL_PM_DEEP_SLEEP (1 << 1)
> > @@ -48,4 +49,27 @@ extern const struct fsl_pm_ops *qoriq_pm_ops;
> >
> > int __init fsl_rcpm_init(void);
> >
> > +#ifdef CONFIG_FSL_QORIQ_PM
> > +int fsl_enter_deepsleep(void);
> > +int fsl_deepsleep_init(void);
> > +#else
> > +static inline int fsl_enter_deepsleep(void) { return -1; }
> > +static inline int fsl_deepsleep_init(void) { return -1; }
> > +#endif
> Please return proper error codes.
>
> Where can fsl_deepsleep_init() be called without CONFIG_FSL_QORIQ_PM?
>
> [Chenhui] I can get rid of the ifdef here. And add it
> in arch/powerpc/sysdev/fsl_rcpm.c.
No, this is the right place for the ifdef for functions that are called from
code that doesn't depend on CONFIG_FSL_QORIQ_PM. But fsl_deepsleep_init() is
called from deepsleep.c which is only built with CONFIG_FSL_QORIQ_PM, and it's
hard to picture a scenario where it would be called from elsewhere.
> > diff --git a/arch/powerpc/kernel/fsl_booke_entry_mapping.S
> > b/arch/powerpc/kernel/fsl_booke_entry_mapping.S
> > index 83dd0f6..659b059 100644
> > --- a/arch/powerpc/kernel/fsl_booke_entry_mapping.S
> > +++ b/arch/powerpc/kernel/fsl_booke_entry_mapping.S
> > @@ -173,6 +173,10 @@ skpinv: addi r6,r6,1 /*
> > Increment */
> > lis r6,MAS2_VAL(PAGE_OFFSET, BOOK3E_PAGESZ_64M, M_IF_NEEDED)@h
> > ori r6,r6,MAS2_VAL(PAGE_OFFSET, BOOK3E_PAGESZ_64M,
> > M_IF_NEEDED)@l
> > mtspr SPRN_MAS2,r6
> > +#ifdef ENTRY_DEEPSLEEP_SETUP
> > + LOAD_REG_IMMEDIATE(r8, MEMORY_START)
> > + ori r8,r8,(MAS3_SX|MAS3_SW|MAS3_SR)
> > +#endif
> > mtspr SPRN_MAS3,r8
> > tlbwe
> >
> Have you tried this with a relocatable kernel?
>
> [Chenhui] Not yet. Not sure whether it has been supported on QorIQ platform.
It is supported, and deep sleep needs to work with it.
> > +static void fsl_dp_set_resume_pointer(void)
> > +{
> > + u32 resume_addr;
> > +
> > + /* the bootloader will finally jump to this address to return kernel
> > */
> > +#ifdef CONFIG_PPC32
> > + resume_addr = (u32)(__pa(fsl_booke_deep_sleep_resume));
> > +#else
> > + resume_addr = (u32)(__pa(*(u64 *)fsl_booke_deep_sleep_resume)
> > + & 0xffffffff);
> > +#endif
> Why are you masking the physical address by 0xffffffff? Besides the
> (u32) cast accomplishing the same thing, wouldn't it be a problem if
> (e.g. due to a relocatable kernel) the address is above 4 GiB?
>
> [Chenhui] Here, I assumed kernel is below 4 GiB. Maybe I should add a
> comment here.
It needs a fix rather than a comment, unless you can show that the relocatable
mechanism doesn't support kernels over 4 GiB (I don't remember of the top of
my head whether it does).
-Scott
[toc] | [prev] | [next] | [standalone]
| From | "C.H. Zhao" <chenhui.zhao@nxp.com> |
|---|---|
| Date | 2016-09-29 12:30 +0200 |
| Message-ID | <smGLf-2Ok-9@gated-at.bofh.it> |
| In reply to | #1492944 |
From: Scott Wood <oss@buserror.net>
Sent: Thursday, September 29, 2016 4:03 AM
To: C.H. Zhao
Cc: linuxppc-dev@lists.ozlabs.org; linux-kernel@vger.kernel.org; z.chenhui@gmail.com; Jason Jin
Subject: Re: [v3,4/5] powerpc/pm: support deep sleep feature on T104x
On Tue, 2016-09-27 at 11:05 +0000, C.H. Zhao wrote:
> From: Scott Wood <oss@buserror.net>
> Sent: Sunday, September 25, 2016 3:24 PM
> To: C.H. Zhao
> Cc: linuxppc-dev@lists.ozlabs.org; linux-kernel@vger.kernel.org; z.chenhui@g
> mail.com; Jason Jin
> Subject: Re: [v3,4/5] powerpc/pm: support deep sleep feature on T104x
>
> On Tue, Aug 02, 2016 at 07:59:31PM +0800, Chenhui Zhao wrote:
> >
> > T104x has deep sleep feature, which can switch off most parts of
> > the SoC when it is in deep sleep mode. This way, it becomes more
> > energy-efficient.
> >
> > The DDR controller will also be powered off in deep sleep. Therefore,
> > the last stage (the latter part of fsl_dp_enter_low) will run without DDR
> > access. This piece of code and related TLBs are prefetched in advance.
> >
> > Due to the different initialization code between 32-bit and 64-bit, they
> > have separate resume entry and precedure.
> >
> > The feature supports 32-bit and 64-bit kernel mode.
> >
> > Signed-off-by: Chenhui Zhao <chenhui.zhao@nxp.com>
> > ---
> > arch/powerpc/include/asm/fsl_pm.h | 24 ++
> > arch/powerpc/kernel/asm-offsets.c | 12 +
> > arch/powerpc/kernel/fsl_booke_entry_mapping.S | 10 +
> > arch/powerpc/kernel/head_64.S | 2 +-
> > arch/powerpc/platforms/85xx/Makefile | 1 +
> > arch/powerpc/platforms/85xx/deepsleep.c | 278 ++++++++++++++
> > arch/powerpc/platforms/85xx/qoriq_pm.c | 25 ++
> > arch/powerpc/platforms/85xx/t104x_deepsleep.S | 531
> > ++++++++++++++++++++++++++
> > arch/powerpc/sysdev/fsl_rcpm.c | 8 +-
> > 9 files changed, 889 insertions(+), 2 deletions(-)
> > create mode 100644 arch/powerpc/platforms/85xx/deepsleep.c
> > create mode 100644 arch/powerpc/platforms/85xx/t104x_deepsleep.S
> >
> > diff --git a/arch/powerpc/include/asm/fsl_pm.h
> > b/arch/powerpc/include/asm/fsl_pm.h
> > index e05049b..48c2631 100644
> > --- a/arch/powerpc/include/asm/fsl_pm.h
> > +++ b/arch/powerpc/include/asm/fsl_pm.h
> > @@ -20,6 +20,7 @@
> >
> > #define PLAT_PM_SLEEP 20
> > #define PLAT_PM_LPM20 30
> > +#define PLAT_PM_LPM35 40
> >
> > #define FSL_PM_SLEEP (1 << 0)
> > #define FSL_PM_DEEP_SLEEP (1 << 1)
> > @@ -48,4 +49,27 @@ extern const struct fsl_pm_ops *qoriq_pm_ops;
> >
> > int __init fsl_rcpm_init(void);
> >
> > +#ifdef CONFIG_FSL_QORIQ_PM
> > +int fsl_enter_deepsleep(void);
> > +int fsl_deepsleep_init(void);
> > +#else
> > +static inline int fsl_enter_deepsleep(void) { return -1; }
> > +static inline int fsl_deepsleep_init(void) { return -1; }
> > +#endif
> Please return proper error codes.
>
> Where can fsl_deepsleep_init() be called without CONFIG_FSL_QORIQ_PM?
>
> [Chenhui] I can get rid of the ifdef here. And add it
> in arch/powerpc/sysdev/fsl_rcpm.c.
No, this is the right place for the ifdef for functions that are called from
code that doesn't depend on CONFIG_FSL_QORIQ_PM. But fsl_deepsleep_init() is
called from deepsleep.c which is only built with CONFIG_FSL_QORIQ_PM, and it's
hard to picture a scenario where it would be called from elsewhere.
[Chenhui] You are right. No need to enclose fsl_deepsleep_init() in the ifdef.
But regarding fsl_enter_deepsleep(), it is called in rcpm_v2_plat_enter_sleep()
in arch/powerpc/sysdev/fsl_rcpm.c. It still needs to be enclosed in the ifdef.
I would change it like:
int fsl_deepsleep_init(void);
#ifdef CONFIG_FSL_QORIQ_PM
int fsl_enter_deepsleep(void);
#else
static inline int fsl_enter_deepsleep(void) { return -EINVAL; }
#endif
> > diff --git a/arch/powerpc/kernel/fsl_booke_entry_mapping.S
> > b/arch/powerpc/kernel/fsl_booke_entry_mapping.S
> > index 83dd0f6..659b059 100644
> > --- a/arch/powerpc/kernel/fsl_booke_entry_mapping.S
> > +++ b/arch/powerpc/kernel/fsl_booke_entry_mapping.S
> > @@ -173,6 +173,10 @@ skpinv: addi r6,r6,1 /*
> > Increment */
> > lis r6,MAS2_VAL(PAGE_OFFSET, BOOK3E_PAGESZ_64M, M_IF_NEEDED)@h
> > ori r6,r6,MAS2_VAL(PAGE_OFFSET, BOOK3E_PAGESZ_64M,
> > M_IF_NEEDED)@l
> > mtspr SPRN_MAS2,r6
> > +#ifdef ENTRY_DEEPSLEEP_SETUP
> > + LOAD_REG_IMMEDIATE(r8, MEMORY_START)
> > + ori r8,r8,(MAS3_SX|MAS3_SW|MAS3_SR)
> > +#endif
> > mtspr SPRN_MAS3,r8
> > tlbwe
> >
> Have you tried this with a relocatable kernel?
>
> [Chenhui] Not yet. Not sure whether it has been supported on QorIQ platform.
It is supported, and deep sleep needs to work with it.
[Chenhui] OK. I'm going to work something out.
> > +static void fsl_dp_set_resume_pointer(void)
> > +{
> > + u32 resume_addr;
> > +
> > + /* the bootloader will finally jump to this address to return kernel
> > */
> > +#ifdef CONFIG_PPC32
> > + resume_addr = (u32)(__pa(fsl_booke_deep_sleep_resume));
> > +#else
> > + resume_addr = (u32)(__pa(*(u64 *)fsl_booke_deep_sleep_resume)
> > + & 0xffffffff);
> > +#endif
> Why are you masking the physical address by 0xffffffff? Besides the
> (u32) cast accomplishing the same thing, wouldn't it be a problem if
> (e.g. due to a relocatable kernel) the address is above 4 GiB?
>
> [Chenhui] Here, I assumed kernel is below 4 GiB. Maybe I should add a
> comment here.
It needs a fix rather than a comment, unless you can show that the relocatable
mechanism doesn't support kernels over 4 GiB (I don't remember of the top of
my head whether it does).
-Scott
[Chenhui] OK. I'm going to work something out.
[toc] | [prev] | [next] | [standalone]
| From | Scott Wood <oss@buserror.net> |
|---|---|
| Date | 2016-10-03 19:30 +0200 |
| Message-ID | <sofdT-7SG-5@gated-at.bofh.it> |
| In reply to | #1493267 |
On Thu, 2016-09-29 at 10:21 +0000, C.H. Zhao wrote:
>
> From: Scott Wood <oss@buserror.net>
> Sent: Thursday, September 29, 2016 4:03 AM
> To: C.H. Zhao
> Cc: linuxppc-dev@lists.ozlabs.org; linux-kernel@vger.kernel.org; z.chenhui@g
> mail.com; Jason Jin
> Subject: Re: [v3,4/5] powerpc/pm: support deep sleep feature on T104x
>
> On Tue, 2016-09-27 at 11:05 +0000, C.H. Zhao wrote:
> >
> > From: Scott Wood <oss@buserror.net>
> > Sent: Sunday, September 25, 2016 3:24 PM
> > To: C.H. Zhao
> > Cc: linuxppc-dev@lists.ozlabs.org; linux-kernel@vger.kernel.org; z.chenhui
> > @g
> > mail.com; Jason Jin
> > Subject: Re: [v3,4/5] powerpc/pm: support deep sleep feature on T104x
> >
> > On Tue, Aug 02, 2016 at 07:59:31PM +0800, Chenhui Zhao wrote:
> > >
> > >
> > > T104x has deep sleep feature, which can switch off most parts of
> > > the SoC when it is in deep sleep mode. This way, it becomes more
> > > energy-efficient.
> > >
> > > The DDR controller will also be powered off in deep sleep. Therefore,
> > > the last stage (the latter part of fsl_dp_enter_low) will run without
> > > DDR
> > > access. This piece of code and related TLBs are prefetched in advance.
> > >
> > > Due to the different initialization code between 32-bit and 64-bit, they
> > > have separate resume entry and precedure.
> > >
> > > The feature supports 32-bit and 64-bit kernel mode.
> > >
> > > Signed-off-by: Chenhui Zhao <chenhui.zhao@nxp.com>
> > > ---
> > > arch/powerpc/include/asm/fsl_pm.h | 24 ++
> > > arch/powerpc/kernel/asm-offsets.c | 12 +
> > > arch/powerpc/kernel/fsl_booke_entry_mapping.S | 10 +
> > > arch/powerpc/kernel/head_64.S | 2 +-
> > > arch/powerpc/platforms/85xx/Makefile | 1 +
> > > arch/powerpc/platforms/85xx/deepsleep.c | 278 ++++++++++++++
> > > arch/powerpc/platforms/85xx/qoriq_pm.c | 25 ++
> > > arch/powerpc/platforms/85xx/t104x_deepsleep.S | 531
> > > ++++++++++++++++++++++++++
> > > arch/powerpc/sysdev/fsl_rcpm.c | 8 +-
> > > 9 files changed, 889 insertions(+), 2 deletions(-)
> > > create mode 100644 arch/powerpc/platforms/85xx/deepsleep.c
> > > create mode 100644 arch/powerpc/platforms/85xx/t104x_deepsleep.S
> > >
> > > diff --git a/arch/powerpc/include/asm/fsl_pm.h
> > > b/arch/powerpc/include/asm/fsl_pm.h
> > > index e05049b..48c2631 100644
> > > --- a/arch/powerpc/include/asm/fsl_pm.h
> > > +++ b/arch/powerpc/include/asm/fsl_pm.h
> > > @@ -20,6 +20,7 @@
> > >
> > > #define PLAT_PM_SLEEP 20
> > > #define PLAT_PM_LPM20 30
> > > +#define PLAT_PM_LPM35 40
> > >
> > > #define FSL_PM_SLEEP (1 << 0)
> > > #define FSL_PM_DEEP_SLEEP (1 << 1)
> > > @@ -48,4 +49,27 @@ extern const struct fsl_pm_ops *qoriq_pm_ops;
> > >
> > > int __init fsl_rcpm_init(void);
> > >
> > > +#ifdef CONFIG_FSL_QORIQ_PM
> > > +int fsl_enter_deepsleep(void);
> > > +int fsl_deepsleep_init(void);
> > > +#else
> > > +static inline int fsl_enter_deepsleep(void) { return -1; }
> > > +static inline int fsl_deepsleep_init(void) { return -1; }
> > > +#endif
> > Please return proper error codes.
> >
> > Where can fsl_deepsleep_init() be called without CONFIG_FSL_QORIQ_PM?
> >
> > [Chenhui] I can get rid of the ifdef here. And add it
> > in arch/powerpc/sysdev/fsl_rcpm.c.
> No, this is the right place for the ifdef for functions that are called from
> code that doesn't depend on CONFIG_FSL_QORIQ_PM. But fsl_deepsleep_init()
> is
> called from deepsleep.c which is only built with CONFIG_FSL_QORIQ_PM, and
> it's
> hard to picture a scenario where it would be called from elsewhere.
>
>
> [Chenhui] You are right. No need to enclose fsl_deepsleep_init() in the
> ifdef.
> But regarding fsl_enter_deepsleep(), it is called in
> rcpm_v2_plat_enter_sleep()
> in arch/powerpc/sysdev/fsl_rcpm.c. It still needs to be
> enclosed in the ifdef.
Right. That's why I specifically asked about fsl_deepsleep_init(). :-)
-Scott
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web