diff mbox

[v2,7/7] ARM: EXYNOS: cpuidle: add secure firmware support to AFTR mode code

Message ID 1401712543-14281-8-git-send-email-b.zolnierkie@samsung.com (mailing list archive)
State New, archived
Headers show

Commit Message

Bartlomiej Zolnierkiewicz June 2, 2014, 12:35 p.m. UTC 
* Use do_idle firmware method instead of cpu_do_idle() on boards with
  secure firmware enabled.

* Use sysram_ns_base_addr + 0x24 address for exynos_boot_vector_addr()
  and sysram_ns_base_addr + 0x20 one for exynos_boot_vector_flag() on
  boards with secure firmware enabled.

This patch fixes hang on an attempt to enter AFTR mode for TRATS2
board (which uses EXYNOS4412 SoC with secure firmware enabled).

This patch shouldn't cause any functionality changes on boards that
don't use secure firmware.

Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 arch/arm/mach-exynos/pm.c        | 8 ++++++--
 drivers/cpuidle/cpuidle-exynos.c | 7 ++++++-
 2 files changed, 12 insertions(+), 3 deletions(-)

Comments

Tomasz Figa June 2, 2014, 1:15 p.m. UTC | #1 
Hi,

On 02.06.2014 14:35, Bartlomiej Zolnierkiewicz wrote:
> * Use do_idle firmware method instead of cpu_do_idle() on boards with
>   secure firmware enabled.
> 
> * Use sysram_ns_base_addr + 0x24 address for exynos_boot_vector_addr()
>   and sysram_ns_base_addr + 0x20 one for exynos_boot_vector_flag() on
>   boards with secure firmware enabled.
> 
> This patch fixes hang on an attempt to enter AFTR mode for TRATS2
> board (which uses EXYNOS4412 SoC with secure firmware enabled).
> 
> This patch shouldn't cause any functionality changes on boards that
> don't use secure firmware.
> 
> Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  arch/arm/mach-exynos/pm.c        | 8 ++++++--
>  drivers/cpuidle/cpuidle-exynos.c | 7 ++++++-
>  2 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/mach-exynos/pm.c b/arch/arm/mach-exynos/pm.c
> index 0fb9a5a..62a0a5e 100644
> --- a/arch/arm/mach-exynos/pm.c
> +++ b/arch/arm/mach-exynos/pm.c
> @@ -169,7 +169,9 @@ int exynos_cluster_power_state(int cluster)
>  
>  static inline void __iomem *exynos_boot_vector_addr(void)
>  {
> -	if (samsung_rev() == EXYNOS4210_REV_1_1)
> +	if (firmware_run())
> +		return sysram_ns_base_addr + 0x24;
> +	else if (samsung_rev() == EXYNOS4210_REV_1_1)

Aha, so this is the use case for the function added by patch 1/7.

Well, I don't see the need to do it this way and complicate the API. As
I mentioned in my comments to patches 2/7 and 5/7, more general firmware
operations should be taking care of setting those registers to
appropriate values and so there shouldn't be any need to use them
directly outside the implementation of firmware ops.

[snip]

>  static int idle_finisher(unsigned long flags)
>  {
>  	exynos_enter_aftr();
> -	cpu_do_idle();
> +	if (firmware_run())
> +		/* no need to check the return value on EXYNOS SoCs */
> +		call_firmware_op(do_idle, FW_DO_IDLE_AFTR);
> +	else
> +		cpu_do_idle();

This could be done just by

	if (call_firmware_op(do_idle, FW_DO_IDLE_AFTR) == -ENOSYS)
		cpu_do_idle();

which is 3 lines less than with a function that is suppose to simplify
the code.

Best regards,
Tomasz
Bartlomiej Zolnierkiewicz June 2, 2014, 1:40 p.m. UTC | #2 
On Monday, June 02, 2014 03:15:07 PM Tomasz Figa wrote:
> Hi,
> 
> On 02.06.2014 14:35, Bartlomiej Zolnierkiewicz wrote:
> > * Use do_idle firmware method instead of cpu_do_idle() on boards with
> >   secure firmware enabled.
> > 
> > * Use sysram_ns_base_addr + 0x24 address for exynos_boot_vector_addr()
> >   and sysram_ns_base_addr + 0x20 one for exynos_boot_vector_flag() on
> >   boards with secure firmware enabled.
> > 
> > This patch fixes hang on an attempt to enter AFTR mode for TRATS2
> > board (which uses EXYNOS4412 SoC with secure firmware enabled).
> > 
> > This patch shouldn't cause any functionality changes on boards that
> > don't use secure firmware.
> > 
> > Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> > Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
> > ---
> >  arch/arm/mach-exynos/pm.c        | 8 ++++++--
> >  drivers/cpuidle/cpuidle-exynos.c | 7 ++++++-
> >  2 files changed, 12 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/arm/mach-exynos/pm.c b/arch/arm/mach-exynos/pm.c
> > index 0fb9a5a..62a0a5e 100644
> > --- a/arch/arm/mach-exynos/pm.c
> > +++ b/arch/arm/mach-exynos/pm.c
> > @@ -169,7 +169,9 @@ int exynos_cluster_power_state(int cluster)
> >  
> >  static inline void __iomem *exynos_boot_vector_addr(void)
> >  {
> > -	if (samsung_rev() == EXYNOS4210_REV_1_1)
> > +	if (firmware_run())
> > +		return sysram_ns_base_addr + 0x24;
> > +	else if (samsung_rev() == EXYNOS4210_REV_1_1)
> 
> Aha, so this is the use case for the function added by patch 1/7.
> 
> Well, I don't see the need to do it this way and complicate the API. As
> I mentioned in my comments to patches 2/7 and 5/7, more general firmware
> operations should be taking care of setting those registers to
> appropriate values and so there shouldn't be any need to use them
> directly outside the implementation of firmware ops.

More general firmware operations would handle the secure firmware case
fine but how would you like to handle a fallback case given that you
cannot use samsung_rev() etc. in drivers/cpuidle/cpuidle-exynos.c?

> [snip]
> 
> >  static int idle_finisher(unsigned long flags)
> >  {
> >  	exynos_enter_aftr();
> > -	cpu_do_idle();
> > +	if (firmware_run())
> > +		/* no need to check the return value on EXYNOS SoCs */
> > +		call_firmware_op(do_idle, FW_DO_IDLE_AFTR);
> > +	else
> > +		cpu_do_idle();
> 
> This could be done just by
> 
> 	if (call_firmware_op(do_idle, FW_DO_IDLE_AFTR) == -ENOSYS)
> 		cpu_do_idle();
> 
> which is 3 lines less than with a function that is suppose to simplify
> the code.

OK.

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
Daniel Lezcano June 2, 2014, 4:28 p.m. UTC | #3 
On 06/02/2014 02:35 PM, Bartlomiej Zolnierkiewicz wrote:
> * Use do_idle firmware method instead of cpu_do_idle() on boards with
>    secure firmware enabled.
>
> * Use sysram_ns_base_addr + 0x24 address for exynos_boot_vector_addr()
>    and sysram_ns_base_addr + 0x20 one for exynos_boot_vector_flag() on
>    boards with secure firmware enabled.
>
> This patch fixes hang on an attempt to enter AFTR mode for TRATS2
> board (which uses EXYNOS4412 SoC with secure firmware enabled).
>
> This patch shouldn't cause any functionality changes on boards that
> don't use secure firmware.
>
> Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>   arch/arm/mach-exynos/pm.c        | 8 ++++++--
>   drivers/cpuidle/cpuidle-exynos.c | 7 ++++++-
>   2 files changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm/mach-exynos/pm.c b/arch/arm/mach-exynos/pm.c
> index 0fb9a5a..62a0a5e 100644
> --- a/arch/arm/mach-exynos/pm.c
> +++ b/arch/arm/mach-exynos/pm.c
> @@ -169,7 +169,9 @@ int exynos_cluster_power_state(int cluster)
>
>   static inline void __iomem *exynos_boot_vector_addr(void)
>   {
> -	if (samsung_rev() == EXYNOS4210_REV_1_1)
> +	if (firmware_run())
> +		return sysram_ns_base_addr + 0x24;
> +	else if (samsung_rev() == EXYNOS4210_REV_1_1)
>   		return S5P_INFORM7;
>   	else if (samsung_rev() == EXYNOS4210_REV_1_0)
>   		return sysram_base_addr + 0x24;
> @@ -178,7 +180,9 @@ static inline void __iomem *exynos_boot_vector_addr(void)
>
>   static inline void __iomem *exynos_boot_vector_flag(void)
>   {
> -	if (samsung_rev() == EXYNOS4210_REV_1_1)
> +	if (firmware_run())
> +		return sysram_ns_base_addr + 0x20;
> +	else if (samsung_rev() == EXYNOS4210_REV_1_1)
>   		return S5P_INFORM6;
>   	else if (samsung_rev() == EXYNOS4210_REV_1_0)
>   		return sysram_base_addr + 0x20;
> diff --git a/drivers/cpuidle/cpuidle-exynos.c b/drivers/cpuidle/cpuidle-exynos.c
> index 7c01512..f90a4a0 100644
> --- a/drivers/cpuidle/cpuidle-exynos.c
> +++ b/drivers/cpuidle/cpuidle-exynos.c
> @@ -17,13 +17,18 @@
>   #include <asm/proc-fns.h>
>   #include <asm/suspend.h>
>   #include <asm/cpuidle.h>
> +#include <asm/firmware.h>
>
>   static void (*exynos_enter_aftr)(void);
>
>   static int idle_finisher(unsigned long flags)
>   {
>   	exynos_enter_aftr();
> -	cpu_do_idle();
> +	if (firmware_run())
> +		/* no need to check the return value on EXYNOS SoCs */
> +		call_firmware_op(do_idle, FW_DO_IDLE_AFTR);
> +	else
> +		cpu_do_idle();

Why not move this code into the exynos_enter_aftr() function, so 
preventing more dependency ?


>   	return 1;
>   }
>
diff mbox

Patch

diff --git a/arch/arm/mach-exynos/pm.c b/arch/arm/mach-exynos/pm.c
index 0fb9a5a..62a0a5e 100644
--- a/arch/arm/mach-exynos/pm.c
+++ b/arch/arm/mach-exynos/pm.c
@@ -169,7 +169,9 @@  int exynos_cluster_power_state(int cluster)
 
 static inline void __iomem *exynos_boot_vector_addr(void)
 {
-	if (samsung_rev() == EXYNOS4210_REV_1_1)
+	if (firmware_run())
+		return sysram_ns_base_addr + 0x24;
+	else if (samsung_rev() == EXYNOS4210_REV_1_1)
 		return S5P_INFORM7;
 	else if (samsung_rev() == EXYNOS4210_REV_1_0)
 		return sysram_base_addr + 0x24;
@@ -178,7 +180,9 @@  static inline void __iomem *exynos_boot_vector_addr(void)
 
 static inline void __iomem *exynos_boot_vector_flag(void)
 {
-	if (samsung_rev() == EXYNOS4210_REV_1_1)
+	if (firmware_run())
+		return sysram_ns_base_addr + 0x20;
+	else if (samsung_rev() == EXYNOS4210_REV_1_1)
 		return S5P_INFORM6;
 	else if (samsung_rev() == EXYNOS4210_REV_1_0)
 		return sysram_base_addr + 0x20;
diff --git a/drivers/cpuidle/cpuidle-exynos.c b/drivers/cpuidle/cpuidle-exynos.c
index 7c01512..f90a4a0 100644
--- a/drivers/cpuidle/cpuidle-exynos.c
+++ b/drivers/cpuidle/cpuidle-exynos.c
@@ -17,13 +17,18 @@ 
 #include <asm/proc-fns.h>
 #include <asm/suspend.h>
 #include <asm/cpuidle.h>
+#include <asm/firmware.h>
 
 static void (*exynos_enter_aftr)(void);
 
 static int idle_finisher(unsigned long flags)
 {
 	exynos_enter_aftr();
-	cpu_do_idle();
+	if (firmware_run())
+		/* no need to check the return value on EXYNOS SoCs */
+		call_firmware_op(do_idle, FW_DO_IDLE_AFTR);
+	else
+		cpu_do_idle();
 
 	return 1;
 }