[1/4] ARM: exynos: Apply little core workaround only under secure firmware
diff mbox series

Message ID 20200616081230.31198-2-m.szyprowski@samsung.com
State Superseded
Headers show
Series
  • Restore big.LITTLE cpuidle driver for Exynos
Related show

Commit Message

Marek Szyprowski June 16, 2020, 8:12 a.m. UTC
The additional soft-reset call during little core power up was needed
to properly boot all cores on the Exynos5422-based boards with secure
firmware (like Odroid XU3/XU4 family). This however broke big.LITTLE
CPUidle driver, which worked only on boards without secure firmware
(like Peach-Pit/Pi Chromebooks).

Apply the workaround only when board is running under secure firmware.

Fixes: 833b 5794 e330 ("ARM: EXYNOS: reset Little cores when cpu is up")
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 arch/arm/mach-exynos/mcpm-exynos.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

Comments

Lukasz Luba June 17, 2020, 4:26 p.m. UTC | #1
Hi Marek,

I've give it a try with hotplug torture tests and has only one a minor
comment.

On 6/16/20 9:12 AM, Marek Szyprowski wrote:
> The additional soft-reset call during little core power up was needed
> to properly boot all cores on the Exynos5422-based boards with secure
> firmware (like Odroid XU3/XU4 family). This however broke big.LITTLE
> CPUidle driver, which worked only on boards without secure firmware
> (like Peach-Pit/Pi Chromebooks).
> 
> Apply the workaround only when board is running under secure firmware.
> 
> Fixes: 833b 5794 e330 ("ARM: EXYNOS: reset Little cores when cpu is up")
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>   arch/arm/mach-exynos/mcpm-exynos.c | 10 +++++++---
>   1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/mach-exynos/mcpm-exynos.c b/arch/arm/mach-exynos/mcpm-exynos.c
> index 9a681b421ae1..cd861c57d5ad 100644
> --- a/arch/arm/mach-exynos/mcpm-exynos.c
> +++ b/arch/arm/mach-exynos/mcpm-exynos.c
> @@ -26,6 +26,7 @@
>   #define EXYNOS5420_USE_L2_COMMON_UP_STATE	BIT(30)
>   
>   static void __iomem *ns_sram_base_addr __ro_after_init;
> +static bool secure_firmware __ro_after_init;
>   
>   /*
>    * The common v7_exit_coherency_flush API could not be used because of the
> @@ -58,15 +59,16 @@ static void __iomem *ns_sram_base_addr __ro_after_init;
>   static int exynos_cpu_powerup(unsigned int cpu, unsigned int cluster)
>   {
>   	unsigned int cpunr = cpu + (cluster * EXYNOS5420_CPUS_PER_CLUSTER);
> +	bool state;
>   
>   	pr_debug("%s: cpu %u cluster %u\n", __func__, cpu, cluster);
>   	if (cpu >= EXYNOS5420_CPUS_PER_CLUSTER ||
>   		cluster >= EXYNOS5420_NR_CLUSTERS)
>   		return -EINVAL;
>   
> -	if (!exynos_cpu_power_state(cpunr)) {
> -		exynos_cpu_power_up(cpunr);
> -
> +	state = exynos_cpu_power_state(cpunr);
> +	exynos_cpu_power_up(cpunr);

I can see that you have moved this call up, probably to avoid more
'if-else' stuff. I just wanted to notify you that this function
'exynos_cpu_powerup' is called twice when cpu is going up:
1. by the already running cpu i.e. CPU0 and the 'state' is 0 for i.e.
CPU2
2. by the newly starting cpu i.e. CPU2 by running
'secondary_start_kernel' and the state is 3.

In this scenario the 'exynos_cpu_power_up' will be called twice.
I have checked in hotplug that this is not causing any issues, but
thought maybe it's worth share it with you. Maybe you can double check
in TRM that this is not causing anything.

> +	if (!state && secure_firmware) {
>   		/*
>   		 * This assumes the cluster number of the big cores(Cortex A15)
>   		 * is 0 and the Little cores(Cortex A7) is 1.
> @@ -258,6 +260,8 @@ static int __init exynos_mcpm_init(void)
>   		return -ENOMEM;
>   	}
>   
> +	secure_firmware = exynos_secure_firmware_available();
> +
>   	/*
>   	 * To increase the stability of KFC reset we need to program
>   	 * the PMU SPARE3 register
> 

Other than that, the patch set looks good to me.

Regards,
Lukasz
Krzysztof Kozlowski June 22, 2020, 5:19 p.m. UTC | #2
On Wed, Jun 17, 2020 at 05:26:58PM +0100, Lukasz Luba wrote:
> Hi Marek,
> 
> I've give it a try with hotplug torture tests and has only one a minor
> comment.
> 
> On 6/16/20 9:12 AM, Marek Szyprowski wrote:
> > The additional soft-reset call during little core power up was needed
> > to properly boot all cores on the Exynos5422-based boards with secure
> > firmware (like Odroid XU3/XU4 family). This however broke big.LITTLE
> > CPUidle driver, which worked only on boards without secure firmware
> > (like Peach-Pit/Pi Chromebooks).
> > 
> > Apply the workaround only when board is running under secure firmware.
> > 
> > Fixes: 833b 5794 e330 ("ARM: EXYNOS: reset Little cores when cpu is up")

Fix the Fixes tag (in case of resend, otherwise I'll do it).

> > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > ---
> >   arch/arm/mach-exynos/mcpm-exynos.c | 10 +++++++---
> >   1 file changed, 7 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/arm/mach-exynos/mcpm-exynos.c b/arch/arm/mach-exynos/mcpm-exynos.c
> > index 9a681b421ae1..cd861c57d5ad 100644
> > --- a/arch/arm/mach-exynos/mcpm-exynos.c
> > +++ b/arch/arm/mach-exynos/mcpm-exynos.c
> > @@ -26,6 +26,7 @@
> >   #define EXYNOS5420_USE_L2_COMMON_UP_STATE	BIT(30)
> >   static void __iomem *ns_sram_base_addr __ro_after_init;
> > +static bool secure_firmware __ro_after_init;
> >   /*
> >    * The common v7_exit_coherency_flush API could not be used because of the
> > @@ -58,15 +59,16 @@ static void __iomem *ns_sram_base_addr __ro_after_init;
> >   static int exynos_cpu_powerup(unsigned int cpu, unsigned int cluster)
> >   {
> >   	unsigned int cpunr = cpu + (cluster * EXYNOS5420_CPUS_PER_CLUSTER);
> > +	bool state;
> >   	pr_debug("%s: cpu %u cluster %u\n", __func__, cpu, cluster);
> >   	if (cpu >= EXYNOS5420_CPUS_PER_CLUSTER ||
> >   		cluster >= EXYNOS5420_NR_CLUSTERS)
> >   		return -EINVAL;
> > -	if (!exynos_cpu_power_state(cpunr)) {
> > -		exynos_cpu_power_up(cpunr);
> > -
> > +	state = exynos_cpu_power_state(cpunr);
> > +	exynos_cpu_power_up(cpunr);
> 
> I can see that you have moved this call up, probably to avoid more
> 'if-else' stuff. I just wanted to notify you that this function
> 'exynos_cpu_powerup' is called twice when cpu is going up:
> 1. by the already running cpu i.e. CPU0 and the 'state' is 0 for i.e.
> CPU2
> 2. by the newly starting cpu i.e. CPU2 by running
> 'secondary_start_kernel' and the state is 3.
> 
> In this scenario the 'exynos_cpu_power_up' will be called twice.
> I have checked in hotplug that this is not causing any issues, but
> thought maybe it's worth share it with you. Maybe you can double check
> in TRM that this is not causing anything.

This brings the old code, before 833b5794e33. I wonder why? I understood
that only soft-reset should be skipped.

Best regards,
Krzysztof
Marek Szyprowski June 29, 2020, 8:54 a.m. UTC | #3
Hi Krzysztof,

On 22.06.2020 19:19, Krzysztof Kozlowski wrote:
> On Wed, Jun 17, 2020 at 05:26:58PM +0100, Lukasz Luba wrote:
>> I've give it a try with hotplug torture tests and has only one a minor
>> comment.
>>
>> On 6/16/20 9:12 AM, Marek Szyprowski wrote:
>>> The additional soft-reset call during little core power up was needed
>>> to properly boot all cores on the Exynos5422-based boards with secure
>>> firmware (like Odroid XU3/XU4 family). This however broke big.LITTLE
>>> CPUidle driver, which worked only on boards without secure firmware
>>> (like Peach-Pit/Pi Chromebooks).
>>>
>>> Apply the workaround only when board is running under secure firmware.
>>>
>>> Fixes: 833b 5794 e330 ("ARM: EXYNOS: reset Little cores when cpu is up")
> Fix the Fixes tag (in case of resend, otherwise I'll do it).
>
>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>> ---
>>>    arch/arm/mach-exynos/mcpm-exynos.c | 10 +++++++---
>>>    1 file changed, 7 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-exynos/mcpm-exynos.c b/arch/arm/mach-exynos/mcpm-exynos.c
>>> index 9a681b421ae1..cd861c57d5ad 100644
>>> --- a/arch/arm/mach-exynos/mcpm-exynos.c
>>> +++ b/arch/arm/mach-exynos/mcpm-exynos.c
>>> @@ -26,6 +26,7 @@
>>>    #define EXYNOS5420_USE_L2_COMMON_UP_STATE	BIT(30)
>>>    static void __iomem *ns_sram_base_addr __ro_after_init;
>>> +static bool secure_firmware __ro_after_init;
>>>    /*
>>>     * The common v7_exit_coherency_flush API could not be used because of the
>>> @@ -58,15 +59,16 @@ static void __iomem *ns_sram_base_addr __ro_after_init;
>>>    static int exynos_cpu_powerup(unsigned int cpu, unsigned int cluster)
>>>    {
>>>    	unsigned int cpunr = cpu + (cluster * EXYNOS5420_CPUS_PER_CLUSTER);
>>> +	bool state;
>>>    	pr_debug("%s: cpu %u cluster %u\n", __func__, cpu, cluster);
>>>    	if (cpu >= EXYNOS5420_CPUS_PER_CLUSTER ||
>>>    		cluster >= EXYNOS5420_NR_CLUSTERS)
>>>    		return -EINVAL;
>>> -	if (!exynos_cpu_power_state(cpunr)) {
>>> -		exynos_cpu_power_up(cpunr);
>>> -
>>> +	state = exynos_cpu_power_state(cpunr);
>>> +	exynos_cpu_power_up(cpunr);
>> I can see that you have moved this call up, probably to avoid more
>> 'if-else' stuff. I just wanted to notify you that this function
>> 'exynos_cpu_powerup' is called twice when cpu is going up:
>> 1. by the already running cpu i.e. CPU0 and the 'state' is 0 for i.e.
>> CPU2
>> 2. by the newly starting cpu i.e. CPU2 by running
>> 'secondary_start_kernel' and the state is 3.
>>
>> In this scenario the 'exynos_cpu_power_up' will be called twice.
>> I have checked in hotplug that this is not causing any issues, but
>> thought maybe it's worth share it with you. Maybe you can double check
>> in TRM that this is not causing anything.
> This brings the old code, before 833b5794e33. I wonder why? I understood
> that only soft-reset should be skipped.

Because otherwise the Peach boards hangs during the cpuidle. I didn't 
analyze the code that much to judge if it is really necessary in all 
cases, I only restored what worked initially. I can add a comment about 
that to the commit log if needed.

Best regards
Krzysztof Kozlowski June 29, 2020, 9:13 a.m. UTC | #4
On Mon, Jun 29, 2020 at 10:54:27AM +0200, Marek Szyprowski wrote:
> Hi Krzysztof,
> 
> On 22.06.2020 19:19, Krzysztof Kozlowski wrote:
> > On Wed, Jun 17, 2020 at 05:26:58PM +0100, Lukasz Luba wrote:
> >> I've give it a try with hotplug torture tests and has only one a minor
> >> comment.
> >>
> >> On 6/16/20 9:12 AM, Marek Szyprowski wrote:
> >>> The additional soft-reset call during little core power up was needed
> >>> to properly boot all cores on the Exynos5422-based boards with secure
> >>> firmware (like Odroid XU3/XU4 family). This however broke big.LITTLE
> >>> CPUidle driver, which worked only on boards without secure firmware
> >>> (like Peach-Pit/Pi Chromebooks).
> >>>
> >>> Apply the workaround only when board is running under secure firmware.
> >>>
> >>> Fixes: 833b 5794 e330 ("ARM: EXYNOS: reset Little cores when cpu is up")
> > Fix the Fixes tag (in case of resend, otherwise I'll do it).
> >
> >>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> >>> ---
> >>>    arch/arm/mach-exynos/mcpm-exynos.c | 10 +++++++---
> >>>    1 file changed, 7 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/arch/arm/mach-exynos/mcpm-exynos.c b/arch/arm/mach-exynos/mcpm-exynos.c
> >>> index 9a681b421ae1..cd861c57d5ad 100644
> >>> --- a/arch/arm/mach-exynos/mcpm-exynos.c
> >>> +++ b/arch/arm/mach-exynos/mcpm-exynos.c
> >>> @@ -26,6 +26,7 @@
> >>>    #define EXYNOS5420_USE_L2_COMMON_UP_STATE	BIT(30)
> >>>    static void __iomem *ns_sram_base_addr __ro_after_init;
> >>> +static bool secure_firmware __ro_after_init;
> >>>    /*
> >>>     * The common v7_exit_coherency_flush API could not be used because of the
> >>> @@ -58,15 +59,16 @@ static void __iomem *ns_sram_base_addr __ro_after_init;
> >>>    static int exynos_cpu_powerup(unsigned int cpu, unsigned int cluster)
> >>>    {
> >>>    	unsigned int cpunr = cpu + (cluster * EXYNOS5420_CPUS_PER_CLUSTER);
> >>> +	bool state;
> >>>    	pr_debug("%s: cpu %u cluster %u\n", __func__, cpu, cluster);
> >>>    	if (cpu >= EXYNOS5420_CPUS_PER_CLUSTER ||
> >>>    		cluster >= EXYNOS5420_NR_CLUSTERS)
> >>>    		return -EINVAL;
> >>> -	if (!exynos_cpu_power_state(cpunr)) {
> >>> -		exynos_cpu_power_up(cpunr);
> >>> -
> >>> +	state = exynos_cpu_power_state(cpunr);
> >>> +	exynos_cpu_power_up(cpunr);
> >> I can see that you have moved this call up, probably to avoid more
> >> 'if-else' stuff. I just wanted to notify you that this function
> >> 'exynos_cpu_powerup' is called twice when cpu is going up:
> >> 1. by the already running cpu i.e. CPU0 and the 'state' is 0 for i.e.
> >> CPU2
> >> 2. by the newly starting cpu i.e. CPU2 by running
> >> 'secondary_start_kernel' and the state is 3.
> >>
> >> In this scenario the 'exynos_cpu_power_up' will be called twice.
> >> I have checked in hotplug that this is not causing any issues, but
> >> thought maybe it's worth share it with you. Maybe you can double check
> >> in TRM that this is not causing anything.
> > This brings the old code, before 833b5794e33. I wonder why? I understood
> > that only soft-reset should be skipped.
> 
> Because otherwise the Peach boards hangs during the cpuidle. I didn't 
> analyze the code that much to judge if it is really necessary in all 
> cases, I only restored what worked initially. I can add a comment about 
> that to the commit log if needed.

Yes, please mention this in commit msg.

Best regards,
Krzysztof

Patch
diff mbox series

diff --git a/arch/arm/mach-exynos/mcpm-exynos.c b/arch/arm/mach-exynos/mcpm-exynos.c
index 9a681b421ae1..cd861c57d5ad 100644
--- a/arch/arm/mach-exynos/mcpm-exynos.c
+++ b/arch/arm/mach-exynos/mcpm-exynos.c
@@ -26,6 +26,7 @@ 
 #define EXYNOS5420_USE_L2_COMMON_UP_STATE	BIT(30)
 
 static void __iomem *ns_sram_base_addr __ro_after_init;
+static bool secure_firmware __ro_after_init;
 
 /*
  * The common v7_exit_coherency_flush API could not be used because of the
@@ -58,15 +59,16 @@  static void __iomem *ns_sram_base_addr __ro_after_init;
 static int exynos_cpu_powerup(unsigned int cpu, unsigned int cluster)
 {
 	unsigned int cpunr = cpu + (cluster * EXYNOS5420_CPUS_PER_CLUSTER);
+	bool state;
 
 	pr_debug("%s: cpu %u cluster %u\n", __func__, cpu, cluster);
 	if (cpu >= EXYNOS5420_CPUS_PER_CLUSTER ||
 		cluster >= EXYNOS5420_NR_CLUSTERS)
 		return -EINVAL;
 
-	if (!exynos_cpu_power_state(cpunr)) {
-		exynos_cpu_power_up(cpunr);
-
+	state = exynos_cpu_power_state(cpunr);
+	exynos_cpu_power_up(cpunr);
+	if (!state && secure_firmware) {
 		/*
 		 * This assumes the cluster number of the big cores(Cortex A15)
 		 * is 0 and the Little cores(Cortex A7) is 1.
@@ -258,6 +260,8 @@  static int __init exynos_mcpm_init(void)
 		return -ENOMEM;
 	}
 
+	secure_firmware = exynos_secure_firmware_available();
+
 	/*
 	 * To increase the stability of KFC reset we need to program
 	 * the PMU SPARE3 register