ARM: exynos: cpuidle: Move the driver to drivers/cpuidle

The code to initiate and exit the powerdown sequence is the same in pm.c and cpuidle.c.

Let's split the common part in the pm.c and reuse it from the cpu_pm notifier.

That is one more step forward to make the cpuidle driver arch indenpendant.

Signed-off-by Daniel Lezcano <[email protected]>

Reviewed-by Viresh Kumar <[email protected]>

Reviewed-by Bartlomiej Zolnierkiewicz <[email protected]>

---
 arch/arm/mach-exynos/cpuidle.c |   22 ----------------------
 arch/arm/mach-exynos/pm.c      |   24 ++++++++++++++++++++----
 2 files changed, 20 insertions(+), 26 deletions(-)

arch/arm/mach-exynos/cpuidle.c +0 -22

index e6d813d..02609ac 100644
@@ -28,7 +28,6 @@
 #include <mach/map.h>
 
 #include "common.h"
-#include "regs-pmu.h"
 
 static int idle_finisher(unsigned long flags)
 {

@@ -42,31 +41,10 @@ static int exynos_enter_core0_aftr(struct cpuidle_device *dev,
                                struct cpuidle_driver *drv,
                                int index)
 {
-       unsigned long tmp;
-
-       /* Setting Central Sequence Register for power down mode */
-       tmp = __raw_readl(S5P_CENTRAL_SEQ_CONFIGURATION);
-       tmp &= ~S5P_CENTRAL_LOWPWR_CFG;
-       __raw_writel(tmp, S5P_CENTRAL_SEQ_CONFIGURATION);
-
        cpu_pm_enter();
        cpu_suspend(0, idle_finisher);
        cpu_pm_exit();
 
-       /*
-        * If PMU failed while entering sleep mode, WFI will be
-        * ignored by PMU and then exiting cpu_do_idle().
-        * S5P_CENTRAL_LOWPWR_CFG bit will not be set automatically
-        * in this situation.
-        */
-       tmp = __raw_readl(S5P_CENTRAL_SEQ_CONFIGURATION);
-       if (!(tmp & S5P_CENTRAL_LOWPWR_CFG)) {
-               tmp |= S5P_CENTRAL_LOWPWR_CFG;
-               __raw_writel(tmp, S5P_CENTRAL_SEQ_CONFIGURATION);
-               /* Clear wakeup state register */
-               __raw_writel(0x0, S5P_WAKEUP_STAT);
-       }
-
        return index;
 }
 

arch/arm/mach-exynos/pm.c +19 -4

index 50b6b4d..6d9ef69 100644
@@ -213,15 +213,21 @@ static void exynos_pm_prepare(void)
        __raw_writel(virt_to_phys(exynos_cpu_resume), S5P_INFORM0);
 }
 
-static int exynos_pm_suspend(void)
+static void exynos_pm_central_suspend(void)
 {
        unsigned long tmp;
 
        /* Setting Central Sequence Register for power down mode */
-
        tmp = __raw_readl(S5P_CENTRAL_SEQ_CONFIGURATION);
        tmp &= ~S5P_CENTRAL_LOWPWR_CFG;
        __raw_writel(tmp, S5P_CENTRAL_SEQ_CONFIGURATION);
+}
+
+static int exynos_pm_suspend(void)
+{
+       unsigned long tmp;
+
+       exynos_pm_central_suspend();
 
        /* Setting SEQ_OPTION register */
 

@@ -234,7 +240,7 @@ static int exynos_pm_suspend(void)
        return 0;
 }
 
-static void exynos_pm_resume(void)
+static int exynos_pm_central_resume(void)
 {
        unsigned long tmp;
 

@@ -251,9 +257,17 @@ static void exynos_pm_resume(void)
                /* clear the wakeup state register */
                __raw_writel(0x0, S5P_WAKEUP_STAT);
                /* No need to perform below restore code */
-               goto early_wakeup;
+               return -1;
        }
 
+       return 0;
+}
+
+static void exynos_pm_resume(void)
+{
+       if (exynos_pm_central_resume())
+               goto early_wakeup;
+
        if (!soc_is_exynos5250())
                exynos_cpu_restore_register();
 

@@ -359,6 +373,7 @@ static int exynos_cpu_pm_notifier(struct notifier_block *self,
        switch (cmd) {
        case CPU_PM_ENTER:
                if (cpu == 0) {
+                       exynos_pm_central_suspend();
                        exynos_cpu_save_register();
                }
                break;

arch/arm/mach-exynos/pm.c +0 -1

                }
                break;
        }
--

1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

On Fri, Apr 11, 2014 at 4:10 PM, Daniel Lezcano
<[email protected]> wrote:

arch/arm/mach-exynos/pm.c static int exynos_cpu_pm_notifier(struct notifier_block *self, · line 386

Quoted 13 lines

arch/arm/mach-exynos/pm.c +2 -0

@@ -359,6 +373,7 @@ static int exynos_cpu_pm_notifier(struct notifier_block *self,
        switch (cmd) {
        case CPU_PM_ENTER:
                if (cpu == 0) {
+                       exynos_pm_central_suspend();
                        exynos_cpu_save_register();
                }
                break;

@@ -368,6 +383,7 @@ static int exynos_cpu_pm_notifier(struct notifier_block *self,
                        if (!soc_is_exynos5250())
                                scu_enable(S5P_VA_SCU);
                        exynos_cpu_restore_register();
+                       exynos_pm_central_resume();

This notifier is called for system wide suspend and cpuidle.

In case of Exynos cpuidle only AFTR and LPA state need to program central_sequencer and store/restore the registers.

But in 5420 (core-power-down), this is not required, and causing the regression.

Hence need to remove this notifier, or need to find a way to differentiate the cpuidle state.

Hi Chander,

Quoted 2 lines · Chander Kashyap

On Fri, Apr 11, 2014 at 4:10 PM, Daniel Lezcano
<[email protected]> wrote:

[snip]

Quoted 16 lines

arch/arm/mach-exynos/pm.c static int exynos_cpu_pm_notifier(struct notifier_block *self, · line 386

Quoted 13 lines

arch/arm/mach-exynos/pm.c +2 -0

@@ -359,6 +373,7 @@ static int exynos_cpu_pm_notifier(struct notifier_block *self,
        switch (cmd) {
        case CPU_PM_ENTER:
                if (cpu == 0) {
+                       exynos_pm_central_suspend();
                        exynos_cpu_save_register();
                }
                break;

@@ -368,6 +383,7 @@ static int exynos_cpu_pm_notifier(struct notifier_block *self,
                        if (!soc_is_exynos5250())
                                scu_enable(S5P_VA_SCU);
                        exynos_cpu_restore_register();
+                       exynos_pm_central_resume();

This notifier is called for system wide suspend and cpuidle.

Quoted 7 lines

In case of Exynos cpuidle only AFTR and LPA state need to program central_sequencer and store/restore the registers.

But in 5420 (core-power-down), this is not required, and causing the regression.

Hence need to remove this notifier, or need to find a way to differentiate the cpuidle state.

This patch is already present in v3.16. Moreover, Exynos5420 cpuidle has not been merged yet. This means that this issue is not a regression and I believe any further work on this should be carried out as further patches on top of this change.

Anyway, this change has introduced a regression, though, but in another area - it broke suspend, at least on Exynos4-based devices, because now certain steps are performed twice. I've sent a patch for 3.16-rc3 to fix this by dropping custom suspend-specific syscore ops, effectively moving most of the handling to CPU PM notifier, which also matches requirements of AFTR and lower power states. See [1].

[1]
http://www.mail-archive.com/linux-samsung-soc at vger.kernel.org/msg32935.html

However, in this case, moving back to suspend-specific syscore_ops and simply duplicating some code for lower cpuidle states might be a better option. Care to send a patch (fix for 3.16, replacing mine) or I should do it?

Best regards,
Tomasz
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

No more dependency on the arch code. The platform_data field is used to set the PM callback as the other cpuidle drivers.

Signed-off-by Daniel Lezcano <[email protected]>

Reviewed-by Viresh Kumar <[email protected]>

Reviewed-by Bartlomiej Zolnierkiewicz <[email protected]>

arch/arm/mach-exynos/exynos.c +0 -0

index fe8dac8..d22f0e4 100644

Quoted 6 lines · Daniel Lezcano

No more dependency on the arch code. The platform_data field is used to set the PM callback as the other cpuidle drivers.

Signed-off-by Daniel Lezcano <[email protected]>

Reviewed-by Viresh Kumar <[email protected]>

Reviewed-by Bartlomiej Zolnierkiewicz <[email protected]>

This has just shown up in linux-next and broken randconfig builds.

Quoted 5 lines

arch/arm/mach-exynos/exynos.c +0 -0

index fe8dac8..d22f0e4 100644

arch/arm/mach-exynos/exynos.c void exynos_restart(enum reboot_mode mode, const char *cmd) · line 228

Quoted 10 lines

arch/arm/mach-exynos/exynos.c +3 -2

@@ -221,8 +221,9 @@ void exynos_restart(enum reboot_mode mode, const char *cmd)
 }
 
 static struct platform_device exynos_cpuidle = {
-       .name           = "exynos_cpuidle",
-       .id             = -1,
+       .name              = "exynos_cpuidle",
+       .dev.platform_data = exynos_enter_aftr,
+       .id                = -1,
 };
 

This is wrong on many levels, can we please do this properly?

* The exynos_enter_aftr function is compiled conditionally, so you can't just reference it from generic code, or you get a link error.

* 'static struct platform_device ...' has been deprecated for at least a decade, stop doing that. For any platform devices that get registered, there is platform_device_register_simple().

* There shouldn't need to be a platform_device to start with, this should all come from DT. We can't do this on arm64 anyway, so any code that may be shared between arm32 and arm64 should have proper abstractions.

Hi Arnd,

Quoted 9 lines · Arnd Bergmann

Quoted 6 lines · Daniel Lezcano

No more dependency on the arch code. The platform_data field is used to set the PM callback as the other cpuidle drivers.

Signed-off-by Daniel Lezcano <[email protected]>

Reviewed-by Viresh Kumar <[email protected]>

Reviewed-by Bartlomiej Zolnierkiewicz <[email protected]>

This has just shown up in linux-next and broken randconfig builds.

Quoted 18 lines

Quoted 5 lines

arch/arm/mach-exynos/exynos.c +0 -0

index fe8dac8..d22f0e4 100644

arch/arm/mach-exynos/exynos.c void exynos_restart(enum reboot_mode mode, const char *cmd) · line 228

Quoted 10 lines

arch/arm/mach-exynos/exynos.c +3 -2

@@ -221,8 +221,9 @@ void exynos_restart(enum reboot_mode mode, const char *cmd)
 }
 
 static struct platform_device exynos_cpuidle = {
-       .name           = "exynos_cpuidle",
-       .id             = -1,
+       .name              = "exynos_cpuidle",
+       .dev.platform_data = exynos_enter_aftr,
+       .id                = -1,
 };
 

This is wrong on many levels, can we please do this properly?

Quoted 2 lines

* The exynos_enter_aftr function is compiled conditionally, so you can't just reference it from generic code, or you get a link error.

+1

Quoted 3 lines

* 'static struct platform_device ...' has been deprecated for at least a decade, stop doing that. For any platform devices that get registered, there is platform_device_register_simple().

+0.5

The missing 0.5 is because you can't pass platform data using platform_device_register_simple(). There is platform_device_register_resndata(), though.

Quoted 3 lines

* There shouldn't need to be a platform_device to start with, this should all come from DT. We can't do this on arm64 anyway, so any code that may be shared between arm32 and arm64 should have proper abstractions.

-1

Exynos cpuidle is not a device on the SoC, so I don't think there is any way to represent it in DT. The only thing I could see this is matching root node with a central SoC driver that instantiates specific subdevices, such as cpufreq and cpuidle, but I don't see any available infrastructure for this.

Hi Arnd,

Quoted 28 lines · Arnd Bergmann

Quoted 6 lines · Daniel Lezcano

No more dependency on the arch code. The platform_data field is used to set the PM callback as the other cpuidle drivers.

Signed-off-by Daniel Lezcano <[email protected]>

Reviewed-by Viresh Kumar <[email protected]>

Reviewed-by Bartlomiej Zolnierkiewicz <[email protected]>

This has just shown up in linux-next and broken randconfig builds.

Quoted 5 lines

arch/arm/mach-exynos/exynos.c +0 -0

index fe8dac8..d22f0e4 100644

arch/arm/mach-exynos/exynos.c void exynos_restart(enum reboot_mode mode, const char *cmd) · line 228

Quoted 10 lines

arch/arm/mach-exynos/exynos.c +3 -2

@@ -221,8 +221,9 @@ void exynos_restart(enum reboot_mode mode, const char *cmd)
  }
 
  static struct platform_device exynos_cpuidle = {
-       .name           = "exynos_cpuidle",
-       .id             = -1,
+       .name              = "exynos_cpuidle",
+       .dev.platform_data = exynos_enter_aftr,
+       .id                = -1,
  };
 

This is wrong on many levels, can we please do this properly?

Quoted 2 lines

* The exynos_enter_aftr function is compiled conditionally, so you can't just reference it from generic code, or you get a link error.

+1

Quoted 3 lines

* 'static struct platform_device ...' has been deprecated for at least a decade, stop doing that. For any platform devices that get registered, there is platform_device_register_simple().

+0.5

The missing 0.5 is because you can't pass platform data using platform_device_register_simple(). There is platform_device_register_resndata(), though.

Quoted 3 lines

* There shouldn't need to be a platform_device to start with, this should all come from DT. We can't do this on arm64 anyway, so any code that may be shared between arm32 and arm64 should have proper abstractions.

-1

Exynos cpuidle is not a device on the SoC, so I don't think there is any way to represent it in DT. The only thing I could see this is matching root node with a central SoC driver that instantiates specific subdevices, such as cpufreq and cpuidle, but I don't see any available infrastructure for this.

Quoted 31 lines · Tomasz Figa

Hi Arnd,

Quoted 28 lines · Arnd Bergmann

Quoted 6 lines · Daniel Lezcano

No more dependency on the arch code. The platform_data field is used to set the PM callback as the other cpuidle drivers.

Signed-off-by Daniel Lezcano <[email protected]>

Reviewed-by Viresh Kumar <[email protected]>

Reviewed-by Bartlomiej Zolnierkiewicz <[email protected]>

This has just shown up in linux-next and broken randconfig builds.

Quoted 5 lines

arch/arm/mach-exynos/exynos.c +0 -0

index fe8dac8..d22f0e4 100644

arch/arm/mach-exynos/exynos.c void exynos_restart(enum reboot_mode mode, const char *cmd) · line 228

Quoted 10 lines

arch/arm/mach-exynos/exynos.c +3 -2

@@ -221,8 +221,9 @@ void exynos_restart(enum reboot_mode mode, const char *cmd)
  }
 
  static struct platform_device exynos_cpuidle = {
-       .name           = "exynos_cpuidle",
-       .id             = -1,
+       .name              = "exynos_cpuidle",
+       .dev.platform_data = exynos_enter_aftr,
+       .id                = -1,
  };
 

This is wrong on many levels, can we please do this properly?

Quoted 4 lines

Quoted 2 lines

* The exynos_enter_aftr function is compiled conditionally, so you can't just reference it from generic code, or you get a link error.

+1

That is true but still we have a link error without this patch. We shouldn't register and declare this structure if CONFIG_PM / CONFIG_CPU_IDLE are not set.

Quoted 14 lines

Quoted 3 lines

* 'static struct platform_device ...' has been deprecated for at least a decade, stop doing that. For any platform devices that get registered, there is platform_device_register_simple().

+0.5

The missing 0.5 is because you can't pass platform data using platform_device_register_simple(). There is platform_device_register_resndata(), though.

Quoted 3 lines

* There shouldn't need to be a platform_device to start with, this should all come from DT. We can't do this on arm64 anyway, so any code that may be shared between arm32 and arm64 should have proper abstractions.

Quoted 7 lines

-1

Exynos cpuidle is not a device on the SoC, so I don't think there is any way to represent it in DT. The only thing I could see this is matching root node with a central SoC driver that instantiates specific subdevices, such as cpufreq and cpuidle, but I don't see any available infrastructure for this.

There is a RFC for defining generic idle states [1].

The idea behind using the platform driver framework is to unify the code across the different drivers and separate the PM / cpuidle code.

By this way, we can move the different drivers to drivers/cpuidle and store them in a single place. That make easier the tracking, the review and the maintenance.

I am ok to by using platform_device_register_resndata() but I would prefer to do that a bit later by converting the other drivers too. That will be easier if we have them grouped in a single directory (this is what does this patchset at the end).

As there are some more work based on this patchset and the link error could be fixed as an independent patch, I would recommend to re-integrate it in the tree as asked by Bartlomiej.

Arnd, Kukjin, Daniel,

Quoted 72 lines · Daniel Lezcano

Quoted 31 lines · Tomasz Figa

Hi Arnd,

Quoted 28 lines · Arnd Bergmann

Quoted 6 lines · Daniel Lezcano

No more dependency on the arch code. The platform_data field is used to set the PM callback as the other cpuidle drivers.

Signed-off-by Daniel Lezcano<[email protected]>

Reviewed-by Viresh Kumar<[email protected]>

Reviewed-by Bartlomiej Zolnierkiewicz<[email protected]>

This has just shown up in linux-next and broken randconfig builds.

Quoted 5 lines

arch/arm/mach-exynos/exynos.c +0 -0

index fe8dac8..d22f0e4 100644

arch/arm/mach-exynos/exynos.c void exynos_restart(enum reboot_mode mode, const char *cmd) · line 228

Quoted 10 lines

arch/arm/mach-exynos/exynos.c +3 -2

@@ -221,8 +221,9 @@ void exynos_restart(enum reboot_mode mode, const char *cmd)
   }
 
   static struct platform_device exynos_cpuidle = {
-       .name           = "exynos_cpuidle",
-       .id             = -1,
+       .name              = "exynos_cpuidle",
+       .dev.platform_data = exynos_enter_aftr,
+       .id                = -1,
   };
 

This is wrong on many levels, can we please do this properly?

Quoted 4 lines

Quoted 2 lines

* The exynos_enter_aftr function is compiled conditionally, so you can't just reference it from generic code, or you get a link error.

+1

That is true but still we have a link error without this patch. We shouldn't register and declare this structure if CONFIG_PM / CONFIG_CPU_IDLE are not set.

Quoted 14 lines

Quoted 3 lines

* 'static struct platform_device ...' has been deprecated for at least a decade, stop doing that. For any platform devices that get registered, there is platform_device_register_simple().

+0.5

The missing 0.5 is because you can't pass platform data using platform_device_register_simple(). There is platform_device_register_resndata(), though.

Quoted 3 lines

* There shouldn't need to be a platform_device to start with, this should all come from DT. We can't do this on arm64 anyway, so any code that may be shared between arm32 and arm64 should have proper abstractions.

Quoted 7 lines

-1

Exynos cpuidle is not a device on the SoC, so I don't think there is any way to represent it in DT. The only thing I could see this is matching root node with a central SoC driver that instantiates specific subdevices, such as cpufreq and cpuidle, but I don't see any available infrastructure for this.

There is a RFC for defining generic idle states [1].

The idea behind using the platform driver framework is to unify the code across the different drivers and separate the PM / cpuidle code.

By this way, we can move the different drivers to drivers/cpuidle and store them in a single place. That make easier the tracking, the review and the maintenance.

Quoted 8 lines

I am ok to by using platform_device_register_resndata() but I would prefer to do that a bit later by converting the other drivers too. That will be easier if we have them grouped in a single directory (this is what does this patchset at the end).

As there are some more work based on this patchset and the link error could be fixed as an independent patch, I would recommend to re-integrate it in the tree as asked by Bartlomiej.

In general, it would be nice to have everything done properly, but I'd consider Daniel's series as a huge improvement already and a nice intermediate step towards further clean-up.

So based on the comments quoted above, instead of stalling the development, I'd suggest to accept this series and then move forward.

Quoted 29 lines

Quoted 16 lines

Quoted 6 lines

Exynos cpuidle is not a device on the SoC, so I don't think there is any
way to represent it in DT. The only thing I could see this is matching root node with a central SoC driver that instantiates specific subdevices, such as cpufreq and cpuidle, but I don't see any available infrastructure for this.

There is a RFC for defining generic idle states [1].

The idea behind using the platform driver framework is to unify the code across the different drivers and separate the PM / cpuidle code.

By this way, we can move the different drivers to drivers/cpuidle and store them in a single place. That make easier the tracking, the review and the maintenance.

Quoted 8 lines

I am ok to by using platform_device_register_resndata() but I would prefer to do that a bit later by converting the other drivers too. That will be easier if we have them grouped in a single directory (this is what does this patchset at the end).

As there are some more work based on this patchset and the link error could be fixed as an independent patch, I would recommend to re-integrate it in the tree as asked by Bartlomiej.

In general, it would be nice to have everything done properly, but I'd consider Daniel's series as a huge improvement already and a nice intermediate step towards further clean-up.

Quoted 3 lines

So based on the comments quoted above, instead of stalling the development, I'd suggest to accept this series and then move forward.

I'm fine.

Arnd, how about you?

- Kukjin

On 05/15/2014 10:40 PM, Kukjin Kim wrote:

[ ... ]

Quoted 15 lines

Quoted 15 lines

Quoted 15 lines

Quoted 6 lines

Exynos cpuidle is not a device on the SoC, so I don't think there is any
way to represent it in DT. The only thing I could see this is matching root node with a central SoC driver that instantiates specific subdevices, such as cpufreq and cpuidle, but I don't see any available infrastructure for this.

There is a RFC for defining generic idle states [1].

The idea behind using the platform driver framework is to unify the code across the different drivers and separate the PM / cpuidle code.

By this way, we can move the different drivers to drivers/cpuidle and store them in a single place. That make easier the tracking, the review and the maintenance.

Quoted 16 lines

Quoted 16 lines

Quoted 8 lines

I am ok to by using platform_device_register_resndata() but I would prefer to do that a bit later by converting the other drivers too. That will be easier if we have them grouped in a single directory (this is what does this patchset at the end).

As there are some more work based on this patchset and the link error could be fixed as an independent patch, I would recommend to re-integrate it in the tree as asked by Bartlomiej.

In general, it would be nice to have everything done properly, but I'd consider Daniel's series as a huge improvement already and a nice intermediate step towards further clean-up.

So based on the comments quoted above, instead of stalling the development, I'd suggest to accept this series and then move forward.

Quoted 5 lines

I'm fine.

Arnd, how about you?

- Kukjin

Arnd ?

Quoted 16 lines · On Wednesday 21 May 2014 09:15:34 Daniel Lezcano wrote:

On 05/15/2014 10:40 PM, Kukjin Kim wrote:

[ ... ]

Quoted 12 lines

Quoted 12 lines

Quoted 12 lines

Quoted 6 lines

Exynos cpuidle is not a device on the SoC, so I don't think there is any
way to represent it in DT. The only thing I could see this is matching root node with a central SoC driver that instantiates specific subdevices, such as cpufreq and cpuidle, but I don't see any available infrastructure for this.

There is a RFC for defining generic idle states [1].

The idea behind using the platform driver framework is to unify the code across the different drivers and separate the PM / cpuidle code.

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

By this way, we can move the different drivers to drivers/cpuidle and store them in a single place. That make easier the tracking, the review and the maintenance.

Yes, that would be great. I only looked briefly at the series now, doesn't that require the use of psci?

That's not a bad idea of course, but it
doesn't solve the problem I raised here.

Quoted 18 lines

Quoted 18 lines

Quoted 9 lines

Quoted 8 lines

I am ok to by using platform_device_register_resndata() but I would prefer to do that a bit later by converting the other drivers too. That will be easier if we have them grouped in a single directory (this is what does this patchset at the end).

As there are some more work based on this patchset and the link error could be fixed as an independent patch, I would recommend to re-integrate it in the tree as asked by Bartlomiej.

Quoted 7 lines

In general, it would be nice to have everything done properly, but I'd consider Daniel's series as a huge improvement already and a nice intermediate step towards further clean-up.

So based on the comments quoted above, instead of stalling the development, I'd suggest to accept this series and then move forward.

I'm fine.

Quoted 5 lines

Quoted 3 lines

Arnd, how about you?

- Kukjin

Arnd ?

Sorry for the delay.

Yes, let's do it this way once more, but please
come up with something better for the future that doesn't tie the cpuidle method to the root compatible string as this does.

Quoted 17 lines · Arnd Bergmann

Quoted 16 lines · On Wednesday 21 May 2014 09:15:34 Daniel Lezcano wrote:

On 05/15/2014 10:40 PM, Kukjin Kim wrote:

[ ... ]

Quoted 12 lines

Quoted 12 lines

Quoted 12 lines

Quoted 6 lines

Exynos cpuidle is not a device on the SoC, so I don't think there is any
way to represent it in DT. The only thing I could see this is matching root node with a central SoC driver that instantiates specific subdevices, such as cpufreq and cpuidle, but I don't see any available infrastructure for this.

There is a RFC for defining generic idle states [1].

The idea behind using the platform driver framework is to unify the code across the different drivers and separate the PM / cpuidle code.

Quoted 6 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

By this way, we can move the different drivers to drivers/cpuidle and store them in a single place. That make easier the tracking, the review and the maintenance.

Yes, that would be great. I only looked briefly at the series now, doesn't that require the use of psci?

No, because PSCI is for some specific platform (eg. calxeda), all the other drivers are legacy and manually handling the PM via some low level callbacks. This is why all drivers were implemented all over the place making so difficult to maintain them. Little by little, we split the PM callbacks from the idle algorithm so reducing the platform dependency with the generic code.

The PSCI implements such PM callbacks in the firmware directly and are accessed through an API. PSCI is, let's say some kindof nextgen cpuidle. It is similar than mwait on Intel. But if a new platform does not have such firmware, then the cpuidle driver will have the legacy format.

Quoted 5 lines · On Wednesday 21 May 2014 09:15:34 Daniel Lezcano wrote:

On 05/15/2014 10:40 PM, Kukjin Kim wrote:

[ ... ]

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Exynos cpuidle is not a device on the SoC, so I don't think there

is

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

any
way to represent it in DT. The only thing I could see this is

matching

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

root node with a central SoC driver that instantiates specific subdevices, such as cpufreq and cpuidle, but I don't see any

available

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 1 line

infrastructure for this.

There is a RFC for defining generic idle states [1].

The idea behind using the platform driver framework is to unify the

code

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

across the different drivers and separate the PM / cpuidle code.

By this way, we can move the different drivers to drivers/cpuidle

and

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

store them in a single place. That make easier the tracking, the

review

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

and the maintenance.

Yes, that would be great. I only looked briefly at the series now, doesn't that require the use of psci? That's not a bad idea of course, but it doesn't solve the problem I raised here.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

I am ok to by using platform_device_register_resndata() but I would prefer to do that a bit later by converting the other drivers too.

That

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

will be easier if we have them grouped in a single directory (this

is

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

what does this patchset at the end).

As there are some more work based on this patchset and the link

error

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 2 lines

could be fixed as an independent patch, I would recommend to re-integrate it in the tree as asked by Bartlomiej.

In general, it would be nice to have everything done properly, but

I'd

Quoted 10 lines

Quoted 10 lines

Quoted 6 lines

consider Daniel's series as a huge improvement already and a nice intermediate step towards further clean-up.

So based on the comments quoted above, instead of stalling the development, I'd suggest to accept this series and then move forward.

I'm fine.

Arnd, how about you?

Quoted 3 lines

Quoted 1 line

- Kukjin

Arnd ?

Sorry for the delay. Yes, let's do it this way once more, but please come up with something better for the future that doesn't tie the cpuidle method to the root compatible string as this does.

Quoted 6 lines · Daniel Lezcano

No more dependency on the arch code. The platform_data field is used to set the PM callback as the other cpuidle drivers.

Signed-off-by Daniel Lezcano<[email protected]>

Reviewed-by Viresh Kumar<[email protected]>

Reviewed-by Bartlomiej Zolnierkiewicz<[email protected]>

This has just shown up in linux-next and broken randconfig builds.

Changelog:

V5:
* Removed patch : "Move clock setup to pm.c"
* Added patch : "Move arm core power down clock to exynos5250 common code"
V4:
* Took into account Tomasz's comments
* Fixed missing call in for central suspend
* Passed parameter to the wakeup mask function
* Moved wakeup mask, boot vector and aftr state into a single function
* Used this function as callback for platform data
* Moved S5P_CHECK_AFTR/S5P_CHECK_SLEEP into pm.c
* Set boot vector only one time
* Splitted some patches to make them more readable
V3:
* Added patch : "ARM: exynos: cpuidle: Disable cpuidle for 5440"
* Removed patch : "ARM: exynos: config: Enable cpuidle"
* Removed default ARM_EXYNOS4210_CPUIDLE=y
* Added comment about bug fix side effect 'for_each_possible_cpu'
V2:
* Added comment in changelog for calls order (5/17)
* Call the powerdown only for cpu0 in the pm notifier
* Set the pm notifier for all boards

V1: initial post

This patchset relies on the cpm_pm notifier to initiate the powerdown sequence operations from pm.c instead cpuidle.c. Thus the cpuidle driver is no longer dependent from arch specific code as everything is called from the pm.c file.

The patchset applies on top of linux-samsung/for-next.

Tested on exynos4: 4210
Tested on exynos5: 5250 (without AFTR)

Amit Daniel Kachhap (1):
ARM: EXYNOS: Move arm core power down clock to exynos5250 common
clock

Daniel Lezcano (19):
ARM: exynos: cpuidle: Prevent forward declaration
ARM: exynos: cpuidle: Use cpuidle_register
ARM: exynos: cpuidle: Change function name prefix
ARM: exynos: cpuidle: Encapsulate register access inside a function
ARM: exynos: cpuidle: Move some code inside the idle_finisher
ARM: exynos: cpuidle: Fix S5P_WAKEUP_STAT call
ARM: exynos: cpuidle: Use the cpu_pm notifier
ARM: exynos: cpuidle: Move scu_enable in the cpu_pm notifier
ARM: exynos: cpuidle: Remove ifdef for scu_enable
ARM: exynos: cpuidle: Pass wakeup mask parameter to function
ARM: exynos: cpuidle: Encapsulate boot vector code into a function
ARM: exynos: cpuidle: Disable cpuidle for 5440
ARM: exynos: cpuidle: Encapsulate the AFTR code into a function
ARM: exynos: cpuidle: Move the AFTR state function into pm.c
ARM: exynos: cpuidle: Move the power sequence call in the cpu_pm
notifier
ARM: exynos: cpuidle: Move S5P_CHECK_SLEEP into pm.c
ARM: exynos: cpuidle: Pass the AFTR callback to the platform_data
ARM: exynos: cpuidle: Cleanup all unneeded headers from cpuidle.c
ARM: exynos: cpuidle: Move the driver to drivers/cpuidle directory

  arch/arm/mach-exynos/Makefile        |    1 -
  arch/arm/mach-exynos/common.h        |    1 +
  arch/arm/mach-exynos/cpuidle.c       |  255 ----------------------------------
  arch/arm/mach-exynos/exynos.c        |    8 +-
  arch/arm/mach-exynos/pm.c            |  152 ++++++++++++++++----
  arch/arm/mach-exynos/regs-pmu.h      |    2 -
  drivers/clk/samsung/clk-exynos5250.c |   42 ++++++
  drivers/cpuidle/Kconfig.arm          |    6 +
  drivers/cpuidle/Makefile             |    1 +
  drivers/cpuidle/cpuidle-exynos.c     |   98 +++++++++++++
  10 files changed, 277 insertions(+), 289 deletions(-)

delete mode 100644 arch/arm/mach-exynos/cpuidle.c create mode 100644 drivers/cpuidle/cpuidle-exynos.c

Hi Kukjin,

I believe I addressed all the comments. Is it possible to take this patchset for next ?

Quoted 5 lines · Daniel Lezcano

Hi Kukjin,

I believe I addressed all the comments. Is it possible to take this patchset for next ?

+1.

Also when applying you might add

Reviewed-by Tomasz Figa <[email protected]>

to any patches that don't have it yet.

Quoted 8 lines · Tomasz Figa

Quoted 5 lines · Daniel Lezcano

Hi Kukjin,

I believe I addressed all the comments. Is it possible to take this patchset for next ?

Sure ;-)

Quoted 4 lines

+1.

Also when applying you might add

Quoted 4 lines

Reviewed-by Tomasz Figa<[email protected]>

to any patches that don't have it yet.

Tomasz, thanks for your review.

I will take this series, "moving exynos-cpuidle into drivers/cpuidle" into samsung tree if Rafael is OK on that.

Quoted 21 lines · Kukjin Kim

Quoted 8 lines · Tomasz Figa

Quoted 5 lines · Daniel Lezcano

Hi Kukjin,

I believe I addressed all the comments. Is it possible to take this patchset for next ?

Sure ;-)

Quoted 8 lines

+1.

Also when applying you might add

Reviewed-by Tomasz Figa<[email protected]>

to any patches that don't have it yet.

Tomasz, thanks for your review.

Quoted 3 lines

I will take this series, "moving exynos-cpuidle into drivers/cpuidle" into samsung tree if Rafael is OK on that.

Daniel,

Can you please check/test the functionality your series with using my for-next because there were merge conflicts with mcpm-exynos stuff...?

Quoted 21 lines · Kukjin Kim

Quoted 8 lines · Tomasz Figa

Quoted 5 lines · Daniel Lezcano

Hi Kukjin,

I believe I addressed all the comments. Is it possible to take this patchset for next ?

Sure ;-)

Quoted 8 lines

+1.

Also when applying you might add

Reviewed-by Tomasz Figa<[email protected]>

to any patches that don't have it yet.

Tomasz, thanks for your review.

Quoted 3 lines

I will take this series, "moving exynos-cpuidle into drivers/cpuidle" into samsung tree if Rafael is OK on that.

Daniel,

Can you please check/test the functionality your series with using my for-next because there were merge conflicts with mcpm-exynos stuff...?

Quoted 22 lines · Kukjin Kim

Quoted 21 lines · Kukjin Kim

Quoted 8 lines · Tomasz Figa

Quoted 5 lines · Daniel Lezcano

Hi Kukjin,

I believe I addressed all the comments. Is it possible to take this patchset for next ?

Sure ;-)

Quoted 8 lines

+1.

Also when applying you might add

Reviewed-by Tomasz Figa<[email protected]>

to any patches that don't have it yet.

Tomasz, thanks for your review.

Quoted 7 lines

Quoted 3 lines

I will take this series, "moving exynos-cpuidle into drivers/cpuidle" into samsung tree if Rafael is OK on that.

Daniel,

Can you please check/test the functionality your series with using my for-next because there were merge conflicts with mcpm-exynos stuff...?

Hi Kukjin,

I tested the latest tree. Unfortunately it panics when unplugging cpu1:

[    3.124189] Unable to handle kernel paging request at virtual address
f8400024
[    3.129950] pgd = c0004000
[    3.132626] [f8400024] *pgd=6f7f7841, *pte=00000000, *ppte=00000000
[    3.138877] Internal error: Oops: 827 [#1] PREEMPT SMP ARM
[    3.192782] r3 : f8400024  r2 : f8180800  r1 : ee836e44  r0 : f8400024
[    3.199293] Flags: nZCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM
Segment kernel
[    3.206673] Control: 10c5387d  Table: 6e37c04a  DAC: 00000015
[    3.212398] Process swapper/0 (pid: 0, stack limit = 0xc0510240)
[    3.218388] Stack: (0xc0511ef4 to 0xc0512000)
[    3.222728] 1ee0: 00000030 c02b20f8 ee836e40
[    3.230894] 1f00: c001234c 6e880000 c0511f34 40018a80 00000000
00000000 00000000 00000015
[    3.239053] 1f20: 4000404a 10c5387d 00000041 00f00000 00000000
00000000 c02b20e4 edc4a540
[    3.247212] 1f40: c038dacc eefc5cf8 c050ecf0 c0543210 00000000
c0012460 00000001 c0543210
[    3.255371] 1f60: eefc5cf8 c02b2148 b9f92927 00000000 c054326c
c02b0968 b9f92927 00000000
[    3.263530] 1f80: c0510000 c0518480 c038dacc c0510000 c0510000
c0518480 c038dacc eefc5cf8
[    3.271689] 1fa0: c0543210 c004e990 c0511fb4 c03873b8 00000000
c04f90c8 00000000 c04d4b18
[    3.279848] 1fc0: ffffffff ffffffff c04d457c 00000000 00000000
c04f90c8 00000000 10c5387d
[    3.288007] 1fe0: c0518410 c04f90c4 c051bd5c 4000406a 00000000
40008074 00000000 00000000
[    3.296184] [<c0019c5c>] (exynos_enter_aftr) from [<c02b20f8>]
(idle_finisher+0x14/0x20)
[    3.304247] [<c02b20f8>] (idle_finisher) from [<c001234c>]
(cpu_suspend_abort+0x0/0x14)
[    3.312226] [<c001234c>] (cpu_suspend_abort) from [<00000000>] ( 
(null))
[    3.318994] Code: e34f3840 e3500010 11a00002 01a00003 (e5804000)
[    3.325069] ---[ end trace fca911f75a18c040 ]---

After git bisecting I falls on this commit:

commit b3205dea8fbf6db9b1e46a0dad19a0712fdff44f

Author: Sachin Kamat <[email protected]>
Date:   Tue May 13 07:13:44 2014 +0900

ARM: EXYNOS: Map SYSRAM through generic DT bindings

Instead of hardcoding the SYSRAM details for each SoC, pass this information through device tree (DT) and make the code SoC agnostic. Generic DT SRAM bindings are used for achieving this.

Signed-off-by Sachin Kamat <[email protected]>

Acked-by Arnd Bergmann <[email protected]>

Acked-by Heiko Stuebner <[email protected]>

Reviewed-by Tomasz Figa <[email protected]>

Signed-off-by Kukjin Kim <[email protected]>

... which is before my series is applied.

So I am not able to tell yet if my series is correctly rebased or not.

And before someone asks me, yes I updated the dtb :)

Hi Daniel,

Quoted 93 lines · Daniel Lezcano

Quoted 22 lines · Kukjin Kim

Quoted 21 lines · Kukjin Kim

Quoted 8 lines · Tomasz Figa

Quoted 5 lines · Daniel Lezcano

Hi Kukjin,

I believe I addressed all the comments. Is it possible to take this patchset for next ?

Sure ;-)

Quoted 8 lines

+1.

Also when applying you might add

Reviewed-by Tomasz Figa<[email protected]>

to any patches that don't have it yet.

Tomasz, thanks for your review.

Quoted 7 lines

Quoted 3 lines

I will take this series, "moving exynos-cpuidle into drivers/cpuidle" into samsung tree if Rafael is OK on that.

Daniel,

Can you please check/test the functionality your series with using my for-next because there were merge conflicts with mcpm-exynos stuff...?

Hi Kukjin,

I tested the latest tree. Unfortunately it panics when unplugging cpu1:

[    3.124189] Unable to handle kernel paging request at virtual address
f8400024
[    3.129950] pgd = c0004000
[    3.132626] [f8400024] *pgd=6f7f7841, *pte=00000000, *ppte=00000000
[    3.138877] Internal error: Oops: 827 [#1] PREEMPT SMP ARM
[    3.192782] r3 : f8400024  r2 : f8180800  r1 : ee836e44  r0 : f8400024
[    3.199293] Flags: nZCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM
Segment kernel
[    3.206673] Control: 10c5387d  Table: 6e37c04a  DAC: 00000015
[    3.212398] Process swapper/0 (pid: 0, stack limit = 0xc0510240)
[    3.218388] Stack: (0xc0511ef4 to 0xc0512000)
[    3.222728] 1ee0: 00000030 c02b20f8 ee836e40
[    3.230894] 1f00: c001234c 6e880000 c0511f34 40018a80 00000000
00000000 00000000 00000015
[    3.239053] 1f20: 4000404a 10c5387d 00000041 00f00000 00000000
00000000 c02b20e4 edc4a540
[    3.247212] 1f40: c038dacc eefc5cf8 c050ecf0 c0543210 00000000
c0012460 00000001 c0543210
[    3.255371] 1f60: eefc5cf8 c02b2148 b9f92927 00000000 c054326c
c02b0968 b9f92927 00000000
[    3.263530] 1f80: c0510000 c0518480 c038dacc c0510000 c0510000
c0518480 c038dacc eefc5cf8
[    3.271689] 1fa0: c0543210 c004e990 c0511fb4 c03873b8 00000000
c04f90c8 00000000 c04d4b18
[    3.279848] 1fc0: ffffffff ffffffff c04d457c 00000000 00000000
c04f90c8 00000000 10c5387d
[    3.288007] 1fe0: c0518410 c04f90c4 c051bd5c 4000406a 00000000
40008074 00000000 00000000
[    3.296184] [<c0019c5c>] (exynos_enter_aftr) from [<c02b20f8>]
(idle_finisher+0x14/0x20)
[    3.304247] [<c02b20f8>] (idle_finisher) from [<c001234c>]
(cpu_suspend_abort+0x0/0x14)
[    3.312226] [<c001234c>] (cpu_suspend_abort) from [<00000000>] (
(null))
[    3.318994] Code: e34f3840 e3500010 11a00002 01a00003 (e5804000)
[    3.325069] ---[ end trace fca911f75a18c040 ]---

After git bisecting I falls on this commit:

commit b3205dea8fbf6db9b1e46a0dad19a0712fdff44f

Author: Sachin Kamat <[email protected]>
Date:   Tue May 13 07:13:44 2014 +0900

ARM: EXYNOS: Map SYSRAM through generic DT bindings

Instead of hardcoding the SYSRAM details for each SoC, pass this information through device tree (DT) and make the code SoC agnostic. Generic DT SRAM bindings are used for achieving this.

Signed-off-by Sachin Kamat <[email protected]>

Acked-by Arnd Bergmann <[email protected]>

Acked-by Heiko Stuebner <[email protected]>

Reviewed-by Tomasz Figa <[email protected]>

Signed-off-by Kukjin Kim <[email protected]>

Quoted 5 lines

... which is before my series is applied.

So I am not able to tell yet if my series is correctly rebased or not.

And before someone asks me, yes I updated the dtb :)

The driver seemed to be working fine for me on Exynos4210-TRATS board (with right bootloader, which supports AFTR).

Still, a quick look at the code reveals use of S5P_VA_SYSRAM macro, in case of certain SoC revisions, which is not valid any longer, after SYSRAM started to be mapped dynamically. As you can see in platsmp.c, the new dynamic mapping is stored in sysram_base_addr variable, which is static right now.

My proposed fix would be to make it non-static, declare it in one of existing private headers (common.h probably) and use it in pm.c instead of S5P_VA_SYSRAM.

A look at the code reveals use of S5P_VA_SYSRAM macro, in case of certain SoC revisions, which is not valid any longer, after SYSRAM started to be mapped dynamically. The new dynamic mapping is stored in sysram_base_addr variable, which is declared static in platsmp.c

This fix makes sysram_base_addr non-static, declared it in common.h and used in pm.c instead of S5P_VA_SYSRAM.

Signed-off-by Daniel Lezcano <[email protected]>

Suggested-by Tomasz Figa <[email protected]>

---
  arch/arm/mach-exynos/common.h  |    1 +
  arch/arm/mach-exynos/platsmp.c |    2 +-
  arch/arm/mach-exynos/pm.c      |    4 ++--
  3 files changed, 4 insertions(+), 3 deletions(-)

arch/arm/mach-exynos/common.h +1 -0

index e2d0954..a012bc1 100644
@@ -88,6 +88,7 @@ void mct_init(void __iomem *base, int irq_g0, int irq_l0, int irq_l1);
 
  struct map_desc;
  extern void __iomem *sysram_ns_base_addr;
+extern void __iomem *sysram_base_addr;
  void exynos_init_io(void);
  void exynos_restart(enum reboot_mode mode, const char *cmd);
  void exynos_cpuidle_init(void);

arch/arm/mach-exynos/platsmp.c +1 -1

index 9c16da2..f2bea78 100644
@@ -32,7 +32,7 @@
 
  extern void exynos4_secondary_startup(void);
 
-static void __iomem *sysram_base_addr;
+void __iomem *sysram_base_addr;
  void __iomem *sysram_ns_base_addr;
 
  static void __init exynos_smp_prepare_sysram(void)

arch/arm/mach-exynos/pm.c +2 -2

index d10c351..87c0d34 100644
@@ -168,10 +168,10 @@ int exynos_cluster_power_state(int cluster)
 
  #define EXYNOS_BOOT_VECTOR_ADDR	(samsung_rev() == EXYNOS4210_REV_1_1 ? \
  			S5P_INFORM7 : (samsung_rev() == EXYNOS4210_REV_1_0 ? \
-			(S5P_VA_SYSRAM + 0x24) : S5P_INFORM0))
+			(sysram_base_addr + 0x24) : S5P_INFORM0))
  #define EXYNOS_BOOT_VECTOR_FLAG	(samsung_rev() == EXYNOS4210_REV_1_1 ? \
  			S5P_INFORM6 : (samsung_rev() == EXYNOS4210_REV_1_0 ? \
-			(S5P_VA_SYSRAM + 0x20) : S5P_INFORM1))
+			(sysram_base_addr + 0x20) : S5P_INFORM1))
 
  #define S5P_CHECK_AFTR  0xFCBA0D10
  #define S5P_CHECK_SLEEP 0x00000BAD

Quoted 12 lines · Daniel Lezcano

A look at the code reveals use of S5P_VA_SYSRAM macro, in case of certain SoC
revisions, which is not valid any longer, after SYSRAM started to be mapped
dynamically. The new dynamic mapping is stored in sysram_base_addr variable,
which is declared static in platsmp.c

This fix makes sysram_base_addr non-static, declared it in common.h and used
in pm.c instead of S5P_VA_SYSRAM.

Quoted 2 lines

Signed-off-by Daniel Lezcano <[email protected]>

Suggested-by Tomasz Figa <[email protected]>

Hi Kukjin,

with this fix I confirm cpuidle is working well with the AFTR state on the exynos 4210.

A look at the code reveals use of S5P_VA_SYSRAM macro, in case of certain SoC revisions, which is not valid any longer, after SYSRAM started to be mapped dynamically. The new dynamic mapping is stored in sysram_base_addr variable, which is declared static in platsmp.c

This fix makes sysram_base_addr non-static, declared it in common.h and used in pm.c instead of S5P_VA_SYSRAM.

Signed-off-by Daniel Lezcano <[email protected]>

Suggested-by Tomasz Figa <[email protected]>

---
 arch/arm/mach-exynos/common.h  |    1 +
 arch/arm/mach-exynos/platsmp.c |    2 +-
 arch/arm/mach-exynos/pm.c      |    4 ++--
 3 files changed, 4 insertions(+), 3 deletions(-)