diff mbox series

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

Message ID 20200616081230.31198-2-m.szyprowski@samsung.com
State New
Headers show
Series [1/4] ARM: exynos: Apply little core workaround only under secure firmware | expand

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(-)

-- 
2.17.1

Comments

Krzysztof Kozlowski June 22, 2020, 5:19 p.m. UTC | #1 
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 | #2 
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 | #3 
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
diff mbox series

Patch

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