[RFC/PATCH v2 0/5] Add cpuidle support for r8a7791

[RFC/PATCH v2 0/5] Add cpuidle support for r8a7791

[Keita Kobayashi]

Hello

These patches add cpuidle support for Renesas r8a7791 SoC.
The r8a7791 cpuidle supports WFI and the cpu power state(Core-Standby)
using Generic ARM CPU idle driver(cpuidle-arm).
Generic ARM CPU idle driver have been merged in linux-pm.git / pm+acpi-4.1-rc1 tag.

Based on : renesas.git / renesas-devel-20150413-v4.0 tag
Tested on : koelsch board

Changes from v1:
 - Change "select" to "depend on"
 - Document a new enable method of Renesas cpuidle

Keita Kobayashi (5):
  ARM: cpuidle: Add cpuidle support for R-Car Gen2 series
  ARM: shmobile: Add cpuidle_ops for R-Car cpuidle
  devicetree: bindings: Add new cpuidle enable method for Renesas R-car
    SoCs
  ARM: shmobile: dtsi: Add cpuidle parameters into each cpu for r8a7791
  ARM: shmobile: Enable Renesas R-Car cpuidle for shmobile_defconfig

 Documentation/devicetree/bindings/arm/cpus.txt |  1 +
 arch/arm/boot/dts/r8a7791.dtsi                 | 13 +++++++++++++
 arch/arm/configs/shmobile_defconfig            |  3 +++
 arch/arm/mach-shmobile/platsmp-apmu.c          | 14 +++++++++++---
 arch/arm/mach-shmobile/platsmp-apmu.h          |  1 +
 arch/arm/mach-shmobile/pm-rcar-gen2.c          | 17 +++++++++++++++++
 drivers/cpuidle/Kconfig.arm                    |  8 ++++++++
 7 files changed, 54 insertions(+), 3 deletions(-)

-- 
1.9.1

[RFC/PATCH v2 0/5] Add cpuidle support for r8a7791

[Keita Kobayashi]

Hello

These patches add cpuidle support for Renesas r8a7791 SoC.
The r8a7791 cpuidle supports WFI and the cpu power state(Core-Standby)
using Generic ARM CPU idle driver(cpuidle-arm).
Generic ARM CPU idle driver have been merged in linux-pm.git / pm+acpi-4.1-rc1 tag.

Based on : renesas.git / renesas-devel-20150413-v4.0 tag
Tested on : koelsch board

Changes from v1:
 - Change "select" to "depend on"
 - Document a new enable method of Renesas cpuidle

Keita Kobayashi (5):
  ARM: cpuidle: Add cpuidle support for R-Car Gen2 series
  ARM: shmobile: Add cpuidle_ops for R-Car cpuidle
  devicetree: bindings: Add new cpuidle enable method for Renesas R-car
    SoCs
  ARM: shmobile: dtsi: Add cpuidle parameters into each cpu for r8a7791
  ARM: shmobile: Enable Renesas R-Car cpuidle for shmobile_defconfig

 Documentation/devicetree/bindings/arm/cpus.txt |  1 +
 arch/arm/boot/dts/r8a7791.dtsi                 | 13 +++++++++++++
 arch/arm/configs/shmobile_defconfig            |  3 +++
 arch/arm/mach-shmobile/platsmp-apmu.c          | 14 +++++++++++---
 arch/arm/mach-shmobile/platsmp-apmu.h          |  1 +
 arch/arm/mach-shmobile/pm-rcar-gen2.c          | 17 +++++++++++++++++
 drivers/cpuidle/Kconfig.arm                    |  8 ++++++++
 7 files changed, 54 insertions(+), 3 deletions(-)

-- 
1.9.1

[RFC/PATCH v2 1/5] ARM: cpuidle: Add cpuidle support for R-Car Gen2 series

[Keita Kobayashi]

Define ARM_RCAR_CPUIDLE config item to enable cpuidle
support for Renesas R-Car Gen2 SoCs.

Signed-off-by: Keita Kobayashi <keita.kobayashi.ym@renesas.com>
---
 drivers/cpuidle/Kconfig.arm | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/cpuidle/Kconfig.arm b/drivers/cpuidle/Kconfig.arm
index 21340e0..1bff62e 100644
--- a/drivers/cpuidle/Kconfig.arm
+++ b/drivers/cpuidle/Kconfig.arm
@@ -74,3 +74,11 @@ config ARM_MVEBU_V7_CPUIDLE
 	depends on ARCH_MVEBU && !ARM64
 	help
 	  Select this to enable cpuidle on Armada 370, 38x and XP processors.
+
+config ARM_RCAR_CPUIDLE
+	bool "CPU Idle Driver for the R-Car SoCs"
+	depends on ARCH_RCAR_GEN2
+	depends on ARM_CPUIDLE
+	select ARM_CPU_SUSPEND
+	help
+	  Select this to enable cpuidle for R-Car SoCs
-- 
1.9.1

[RFC/PATCH v2 1/5] ARM: cpuidle: Add cpuidle support for R-Car Gen2 series

[Keita Kobayashi]

Define ARM_RCAR_CPUIDLE config item to enable cpuidle
support for Renesas R-Car Gen2 SoCs.

Signed-off-by: Keita Kobayashi <keita.kobayashi.ym@renesas.com>
---
 drivers/cpuidle/Kconfig.arm | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/cpuidle/Kconfig.arm b/drivers/cpuidle/Kconfig.arm
index 21340e0..1bff62e 100644
--- a/drivers/cpuidle/Kconfig.arm
+++ b/drivers/cpuidle/Kconfig.arm
@@ -74,3 +74,11 @@ config ARM_MVEBU_V7_CPUIDLE
 	depends on ARCH_MVEBU && !ARM64
 	help
 	  Select this to enable cpuidle on Armada 370, 38x and XP processors.
+
+config ARM_RCAR_CPUIDLE
+	bool "CPU Idle Driver for the R-Car SoCs"
+	depends on ARCH_RCAR_GEN2
+	depends on ARM_CPUIDLE
+	select ARM_CPU_SUSPEND
+	help
+	  Select this to enable cpuidle for R-Car SoCs
-- 
1.9.1

[RFC/PATCH v2 2/5] ARM: shmobile: Add cpuidle_ops for R-Car cpuidle

[Keita Kobayashi]

This patch add Core-Standby support for R-Car cpuidle.

Signed-off-by: Keita Kobayashi <keita.kobayashi.ym@renesas.com>
---
 arch/arm/mach-shmobile/platsmp-apmu.c | 14 +++++++++++---
 arch/arm/mach-shmobile/platsmp-apmu.h |  1 +
 arch/arm/mach-shmobile/pm-rcar-gen2.c | 17 +++++++++++++++++
 3 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mach-shmobile/platsmp-apmu.c b/arch/arm/mach-shmobile/platsmp-apmu.c
index f483b56..a35f97c 100644
--- a/arch/arm/mach-shmobile/platsmp-apmu.c
+++ b/arch/arm/mach-shmobile/platsmp-apmu.c
@@ -139,7 +139,8 @@ int shmobile_smp_apmu_boot_secondary(unsigned int cpu, struct task_struct *idle)
 }
 #endif
 
-#if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_SUSPEND)
+#if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_SUSPEND) || \
+    defined(CONFIG_ARM_RCAR_CPUIDLE)
 /* nicked from arch/arm/mach-exynos/hotplug.c */
 static inline void cpu_enter_lowpower_a15(void)
 {
@@ -215,7 +216,7 @@ int shmobile_smp_apmu_cpu_kill(unsigned int cpu)
 }
 #endif
 
-#if defined(CONFIG_SUSPEND)
+#if defined(CONFIG_SUSPEND) || defined(CONFIG_ARM_RCAR_CPUIDLE)
 static int shmobile_smp_apmu_do_suspend(unsigned long cpu)
 {
 	shmobile_smp_hook(cpu, virt_to_phys(cpu_resume), 0);
@@ -224,10 +225,17 @@ static int shmobile_smp_apmu_do_suspend(unsigned long cpu)
 	return 1;
 }
 
-static int shmobile_smp_apmu_enter_suspend(suspend_state_t state)
+void shmobile_smp_apmu_enter_corestandby(void)
 {
 	cpu_suspend(smp_processor_id(), shmobile_smp_apmu_do_suspend);
 	cpu_leave_lowpower();
+}
+#endif
+
+#if defined(CONFIG_SUSPEND)
+static int shmobile_smp_apmu_enter_suspend(suspend_state_t state)
+{
+	shmobile_smp_apmu_enter_corestandby();
 	return 0;
 }
 
diff --git a/arch/arm/mach-shmobile/platsmp-apmu.h b/arch/arm/mach-shmobile/platsmp-apmu.h
index 76512c9..c7dc00a 100644
--- a/arch/arm/mach-shmobile/platsmp-apmu.h
+++ b/arch/arm/mach-shmobile/platsmp-apmu.h
@@ -28,5 +28,6 @@ extern int shmobile_smp_apmu_boot_secondary(unsigned int cpu,
 					    struct task_struct *idle);
 extern void shmobile_smp_apmu_cpu_die(unsigned int cpu);
 extern int shmobile_smp_apmu_cpu_kill(unsigned int cpu);
+extern void shmobile_smp_apmu_enter_corestandby(void);
 
 #endif /* PLATSMP_APMU_H */
diff --git a/arch/arm/mach-shmobile/pm-rcar-gen2.c b/arch/arm/mach-shmobile/pm-rcar-gen2.c
index 6815781..846b8ae 100644
--- a/arch/arm/mach-shmobile/pm-rcar-gen2.c
+++ b/arch/arm/mach-shmobile/pm-rcar-gen2.c
@@ -10,11 +10,15 @@
  * for more details.
  */
 
+#include <linux/cpuidle.h>
+#include <linux/ioport.h>
 #include <linux/kernel.h>
 #include <linux/of.h>
 #include <linux/smp.h>
+#include <asm/cpuidle.h>
 #include <asm/io.h>
 #include "common.h"
+#include "platsmp-apmu.h"
 #include "pm-rcar.h"
 #include "rcar-gen2.h"
 
@@ -113,3 +117,16 @@ void __init rcar_gen2_pm_init(void)
 	rcar_gen2_sysc_init(syscier);
 	shmobile_smp_apmu_suspend_init();
 }
+
+#if defined(CONFIG_ARM_RCAR_CPUIDLE)
+static int rcar_cpuidle_enter(int cpu, unsigned long index)
+{
+	shmobile_smp_apmu_enter_corestandby();
+	return 0;
+}
+
+struct cpuidle_ops rcar_cpuidle_ops __initdata = {
+	.suspend = rcar_cpuidle_enter,
+};
+CPUIDLE_METHOD_OF_DECLARE(rcar, "renesas,rcar-idle", &rcar_cpuidle_ops);
+#endif
-- 
1.9.1

[RFC/PATCH v2 2/5] ARM: shmobile: Add cpuidle_ops for R-Car cpuidle

[Keita Kobayashi]

This patch add Core-Standby support for R-Car cpuidle.

Signed-off-by: Keita Kobayashi <keita.kobayashi.ym@renesas.com>
---
 arch/arm/mach-shmobile/platsmp-apmu.c | 14 +++++++++++---
 arch/arm/mach-shmobile/platsmp-apmu.h |  1 +
 arch/arm/mach-shmobile/pm-rcar-gen2.c | 17 +++++++++++++++++
 3 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mach-shmobile/platsmp-apmu.c b/arch/arm/mach-shmobile/platsmp-apmu.c
index f483b56..a35f97c 100644
--- a/arch/arm/mach-shmobile/platsmp-apmu.c
+++ b/arch/arm/mach-shmobile/platsmp-apmu.c
@@ -139,7 +139,8 @@ int shmobile_smp_apmu_boot_secondary(unsigned int cpu, struct task_struct *idle)
 }
 #endif
 
-#if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_SUSPEND)
+#if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_SUSPEND) || \
+    defined(CONFIG_ARM_RCAR_CPUIDLE)
 /* nicked from arch/arm/mach-exynos/hotplug.c */
 static inline void cpu_enter_lowpower_a15(void)
 {
@@ -215,7 +216,7 @@ int shmobile_smp_apmu_cpu_kill(unsigned int cpu)
 }
 #endif
 
-#if defined(CONFIG_SUSPEND)
+#if defined(CONFIG_SUSPEND) || defined(CONFIG_ARM_RCAR_CPUIDLE)
 static int shmobile_smp_apmu_do_suspend(unsigned long cpu)
 {
 	shmobile_smp_hook(cpu, virt_to_phys(cpu_resume), 0);
@@ -224,10 +225,17 @@ static int shmobile_smp_apmu_do_suspend(unsigned long cpu)
 	return 1;
 }
 
-static int shmobile_smp_apmu_enter_suspend(suspend_state_t state)
+void shmobile_smp_apmu_enter_corestandby(void)
 {
 	cpu_suspend(smp_processor_id(), shmobile_smp_apmu_do_suspend);
 	cpu_leave_lowpower();
+}
+#endif
+
+#if defined(CONFIG_SUSPEND)
+static int shmobile_smp_apmu_enter_suspend(suspend_state_t state)
+{
+	shmobile_smp_apmu_enter_corestandby();
 	return 0;
 }
 
diff --git a/arch/arm/mach-shmobile/platsmp-apmu.h b/arch/arm/mach-shmobile/platsmp-apmu.h
index 76512c9..c7dc00a 100644
--- a/arch/arm/mach-shmobile/platsmp-apmu.h
+++ b/arch/arm/mach-shmobile/platsmp-apmu.h
@@ -28,5 +28,6 @@ extern int shmobile_smp_apmu_boot_secondary(unsigned int cpu,
 					    struct task_struct *idle);
 extern void shmobile_smp_apmu_cpu_die(unsigned int cpu);
 extern int shmobile_smp_apmu_cpu_kill(unsigned int cpu);
+extern void shmobile_smp_apmu_enter_corestandby(void);
 
 #endif /* PLATSMP_APMU_H */
diff --git a/arch/arm/mach-shmobile/pm-rcar-gen2.c b/arch/arm/mach-shmobile/pm-rcar-gen2.c
index 6815781..846b8ae 100644
--- a/arch/arm/mach-shmobile/pm-rcar-gen2.c
+++ b/arch/arm/mach-shmobile/pm-rcar-gen2.c
@@ -10,11 +10,15 @@
  * for more details.
  */
 
+#include <linux/cpuidle.h>
+#include <linux/ioport.h>
 #include <linux/kernel.h>
 #include <linux/of.h>
 #include <linux/smp.h>
+#include <asm/cpuidle.h>
 #include <asm/io.h>
 #include "common.h"
+#include "platsmp-apmu.h"
 #include "pm-rcar.h"
 #include "rcar-gen2.h"
 
@@ -113,3 +117,16 @@ void __init rcar_gen2_pm_init(void)
 	rcar_gen2_sysc_init(syscier);
 	shmobile_smp_apmu_suspend_init();
 }
+
+#if defined(CONFIG_ARM_RCAR_CPUIDLE)
+static int rcar_cpuidle_enter(int cpu, unsigned long index)
+{
+	shmobile_smp_apmu_enter_corestandby();
+	return 0;
+}
+
+struct cpuidle_ops rcar_cpuidle_ops __initdata = {
+	.suspend = rcar_cpuidle_enter,
+};
+CPUIDLE_METHOD_OF_DECLARE(rcar, "renesas,rcar-idle", &rcar_cpuidle_ops);
+#endif
-- 
1.9.1

[RFC/PATCH v2 3/5] devicetree: bindings: Add new cpuidle enable method for Renesas R-car SoCs

[Keita Kobayashi]

This patch add the ARM CPUs Device Tree binding to document a new
enable method of Renesas cpuidle.

Signed-off-by: Keita Kobayashi <keita.kobayashi.ym@renesas.com>
---
 Documentation/devicetree/bindings/arm/cpus.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/arm/cpus.txt b/Documentation/devicetree/bindings/arm/cpus.txt
index 8b9e0a9..663ee11 100644
--- a/Documentation/devicetree/bindings/arm/cpus.txt
+++ b/Documentation/devicetree/bindings/arm/cpus.txt
@@ -196,6 +196,7 @@ nodes to be present and contain the properties described below.
 			    "qcom,gcc-msm8660"
 			    "qcom,kpss-acc-v1"
 			    "qcom,kpss-acc-v2"
+			    "renesas,rcar-idle"
 			    "rockchip,rk3066-smp"
 
 	- cpu-release-addr
-- 
1.9.1

[RFC/PATCH v2 3/5] devicetree: bindings: Add new cpuidle enable method for Renesas R-car SoCs

[Keita Kobayashi]

This patch add the ARM CPUs Device Tree binding to document a new
enable method of Renesas cpuidle.

Signed-off-by: Keita Kobayashi <keita.kobayashi.ym@renesas.com>
---
 Documentation/devicetree/bindings/arm/cpus.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/arm/cpus.txt b/Documentation/devicetree/bindings/arm/cpus.txt
index 8b9e0a9..663ee11 100644
--- a/Documentation/devicetree/bindings/arm/cpus.txt
+++ b/Documentation/devicetree/bindings/arm/cpus.txt
@@ -196,6 +196,7 @@ nodes to be present and contain the properties described below.
 			    "qcom,gcc-msm8660"
 			    "qcom,kpss-acc-v1"
 			    "qcom,kpss-acc-v2"
+			    "renesas,rcar-idle"
 			    "rockchip,rk3066-smp"
 
 	- cpu-release-addr
-- 
1.9.1

[RFC/PATCH v2 4/5] ARM: shmobile: dtsi: Add cpuidle parameters into each cpu for r8a7791

[Keita Kobayashi]

This patch add enable-method node and idle-states node for r8a7791.

Signed-off-by: Keita Kobayashi <keita.kobayashi.ym@renesas.com>
---
 arch/arm/boot/dts/r8a7791.dtsi | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/arch/arm/boot/dts/r8a7791.dtsi b/arch/arm/boot/dts/r8a7791.dtsi
index 3e9f824..0063615 100644
--- a/arch/arm/boot/dts/r8a7791.dtsi
+++ b/arch/arm/boot/dts/r8a7791.dtsi
@@ -46,11 +46,13 @@
 		cpu0: cpu@0 {
 			device_type = "cpu";
 			compatible = "arm,cortex-a15";
+			enable-method = "renesas,rcar-idle";
 			reg = <0>;
 			clock-frequency = <1500000000>;
 			voltage-tolerance = <1>; /* 1% */
 			clocks = <&cpg_clocks R8A7791_CLK_Z>;
 			clock-latency = <300000>; /* 300 us */
+			cpu-idle-states = <&CORE_STANDBY>;
 
 			/* kHz - uV - OPPs unknown yet */
 			operating-points = <1500000 1000000>,
@@ -64,8 +66,19 @@
 		cpu1: cpu@1 {
 			device_type = "cpu";
 			compatible = "arm,cortex-a15";
+			enable-method = "renesas,rcar-idle";
 			reg = <1>;
 			clock-frequency = <1500000000>;
+			cpu-idle-states = <&CORE_STANDBY>;
+		};
+
+		idle-states {
+			CORE_STANDBY: CoreStandby {
+				compatible = "arm,idle-state";
+				entry-latency-us = <500>;
+				exit-latency-us = <2300>;
+				min-residency-us = <2800>;
+			};
 		};
 	};
 
-- 
1.9.1

[RFC/PATCH v2 4/5] ARM: shmobile: dtsi: Add cpuidle parameters into each cpu for r8a7791

[Keita Kobayashi]

This patch add enable-method node and idle-states node for r8a7791.

Signed-off-by: Keita Kobayashi <keita.kobayashi.ym@renesas.com>
---
 arch/arm/boot/dts/r8a7791.dtsi | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/arch/arm/boot/dts/r8a7791.dtsi b/arch/arm/boot/dts/r8a7791.dtsi
index 3e9f824..0063615 100644
--- a/arch/arm/boot/dts/r8a7791.dtsi
+++ b/arch/arm/boot/dts/r8a7791.dtsi
@@ -46,11 +46,13 @@
 		cpu0: cpu@0 {
 			device_type = "cpu";
 			compatible = "arm,cortex-a15";
+			enable-method = "renesas,rcar-idle";
 			reg = <0>;
 			clock-frequency = <1500000000>;
 			voltage-tolerance = <1>; /* 1% */
 			clocks = <&cpg_clocks R8A7791_CLK_Z>;
 			clock-latency = <300000>; /* 300 us */
+			cpu-idle-states = <&CORE_STANDBY>;
 
 			/* kHz - uV - OPPs unknown yet */
 			operating-points = <1500000 1000000>,
@@ -64,8 +66,19 @@
 		cpu1: cpu@1 {
 			device_type = "cpu";
 			compatible = "arm,cortex-a15";
+			enable-method = "renesas,rcar-idle";
 			reg = <1>;
 			clock-frequency = <1500000000>;
+			cpu-idle-states = <&CORE_STANDBY>;
+		};
+
+		idle-states {
+			CORE_STANDBY: CoreStandby {
+				compatible = "arm,idle-state";
+				entry-latency-us = <500>;
+				exit-latency-us = <2300>;
+				min-residency-us = <2800>;
+			};
 		};
 	};
 
-- 
1.9.1

[RFC/PATCH v2 5/5] ARM: shmobile: Enable Renesas R-Car cpuidle for shmobile_defconfig

[Keita Kobayashi]

Signed-off-by: Keita Kobayashi <keita.kobayashi.ym@renesas.com>
---
 arch/arm/configs/shmobile_defconfig | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm/configs/shmobile_defconfig b/arch/arm/configs/shmobile_defconfig
index b58618e..c3a2f6a 100644
--- a/arch/arm/configs/shmobile_defconfig
+++ b/arch/arm/configs/shmobile_defconfig
@@ -44,6 +44,9 @@ CONFIG_CPU_FREQ_GOV_USERSPACE=y
 CONFIG_CPU_FREQ_GOV_ONDEMAND=y
 CONFIG_CPU_FREQ_GOV_CONSERVATIVE=y
 CONFIG_CPUFREQ_DT=y
+CONFIG_CPU_IDLE=y
+CONFIG_ARM_CPUIDLE=y
+CONFIG_ARM_RCAR_CPUIDLE=y
 CONFIG_VFP=y
 CONFIG_NEON=y
 # CONFIG_CORE_DUMP_DEFAULT_ELF_HEADERS is not set
-- 
1.9.1

[RFC/PATCH v2 5/5] ARM: shmobile: Enable Renesas R-Car cpuidle for shmobile_defconfig

[Keita Kobayashi]

Signed-off-by: Keita Kobayashi <keita.kobayashi.ym@renesas.com>
---
 arch/arm/configs/shmobile_defconfig | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm/configs/shmobile_defconfig b/arch/arm/configs/shmobile_defconfig
index b58618e..c3a2f6a 100644
--- a/arch/arm/configs/shmobile_defconfig
+++ b/arch/arm/configs/shmobile_defconfig
@@ -44,6 +44,9 @@ CONFIG_CPU_FREQ_GOV_USERSPACE=y
 CONFIG_CPU_FREQ_GOV_ONDEMAND=y
 CONFIG_CPU_FREQ_GOV_CONSERVATIVE=y
 CONFIG_CPUFREQ_DT=y
+CONFIG_CPU_IDLE=y
+CONFIG_ARM_CPUIDLE=y
+CONFIG_ARM_RCAR_CPUIDLE=y
 CONFIG_VFP=y
 CONFIG_NEON=y
 # CONFIG_CORE_DUMP_DEFAULT_ELF_HEADERS is not set
-- 
1.9.1

Re: [RFC/PATCH v2 0/5] Add cpuidle support for r8a7791

[Simon Horman]

Hi Kobayashi-san,

from my point of view this looks reasonable.
Is it marked as an RFC because you see some problems with it.
If not perhaps it would be best to proceed to a more formal
stage of the review process by dropping the RFC designation?

On Thu, Apr 16, 2015 at 07:35:35PM +0900, Keita Kobayashi wrote:
> Hello
> 
> These patches add cpuidle support for Renesas r8a7791 SoC.
> The r8a7791 cpuidle supports WFI and the cpu power state(Core-Standby)
> using Generic ARM CPU idle driver(cpuidle-arm).
> Generic ARM CPU idle driver have been merged in linux-pm.git / pm+acpi-4.1-rc1 tag.
> 
> Based on : renesas.git / renesas-devel-20150413-v4.0 tag
> Tested on : koelsch board
> 
> Changes from v1:
>  - Change "select" to "depend on"
>  - Document a new enable method of Renesas cpuidle
> 
> Keita Kobayashi (5):
>   ARM: cpuidle: Add cpuidle support for R-Car Gen2 series
>   ARM: shmobile: Add cpuidle_ops for R-Car cpuidle
>   devicetree: bindings: Add new cpuidle enable method for Renesas R-car
>     SoCs
>   ARM: shmobile: dtsi: Add cpuidle parameters into each cpu for r8a7791
>   ARM: shmobile: Enable Renesas R-Car cpuidle for shmobile_defconfig
> 
>  Documentation/devicetree/bindings/arm/cpus.txt |  1 +
>  arch/arm/boot/dts/r8a7791.dtsi                 | 13 +++++++++++++
>  arch/arm/configs/shmobile_defconfig            |  3 +++
>  arch/arm/mach-shmobile/platsmp-apmu.c          | 14 +++++++++++---
>  arch/arm/mach-shmobile/platsmp-apmu.h          |  1 +
>  arch/arm/mach-shmobile/pm-rcar-gen2.c          | 17 +++++++++++++++++
>  drivers/cpuidle/Kconfig.arm                    |  8 ++++++++
>  7 files changed, 54 insertions(+), 3 deletions(-)
> 
> -- 
> 1.9.1
> 

Re: [RFC/PATCH v2 0/5] Add cpuidle support for r8a7791

[Simon Horman]

Hi Kobayashi-san,

from my point of view this looks reasonable.
Is it marked as an RFC because you see some problems with it.
If not perhaps it would be best to proceed to a more formal
stage of the review process by dropping the RFC designation?

On Thu, Apr 16, 2015 at 07:35:35PM +0900, Keita Kobayashi wrote:
> Hello
> 
> These patches add cpuidle support for Renesas r8a7791 SoC.
> The r8a7791 cpuidle supports WFI and the cpu power state(Core-Standby)
> using Generic ARM CPU idle driver(cpuidle-arm).
> Generic ARM CPU idle driver have been merged in linux-pm.git / pm+acpi-4.1-rc1 tag.
> 
> Based on : renesas.git / renesas-devel-20150413-v4.0 tag
> Tested on : koelsch board
> 
> Changes from v1:
>  - Change "select" to "depend on"
>  - Document a new enable method of Renesas cpuidle
> 
> Keita Kobayashi (5):
>   ARM: cpuidle: Add cpuidle support for R-Car Gen2 series
>   ARM: shmobile: Add cpuidle_ops for R-Car cpuidle
>   devicetree: bindings: Add new cpuidle enable method for Renesas R-car
>     SoCs
>   ARM: shmobile: dtsi: Add cpuidle parameters into each cpu for r8a7791
>   ARM: shmobile: Enable Renesas R-Car cpuidle for shmobile_defconfig
> 
>  Documentation/devicetree/bindings/arm/cpus.txt |  1 +
>  arch/arm/boot/dts/r8a7791.dtsi                 | 13 +++++++++++++
>  arch/arm/configs/shmobile_defconfig            |  3 +++
>  arch/arm/mach-shmobile/platsmp-apmu.c          | 14 +++++++++++---
>  arch/arm/mach-shmobile/platsmp-apmu.h          |  1 +
>  arch/arm/mach-shmobile/pm-rcar-gen2.c          | 17 +++++++++++++++++
>  drivers/cpuidle/Kconfig.arm                    |  8 ++++++++
>  7 files changed, 54 insertions(+), 3 deletions(-)
> 
> -- 
> 1.9.1
> 

Re: [RFC/PATCH v2 1/5] ARM: cpuidle: Add cpuidle support for R-Car Gen2 series

[Magnus Damm]

Hi Kobayashi-san,

On Thu, Apr 16, 2015 at 7:35 PM, Keita Kobayashi
<keita.kobayashi.ym@renesas.com> wrote:
> Define ARM_RCAR_CPUIDLE config item to enable cpuidle
> support for Renesas R-Car Gen2 SoCs.
>
> Signed-off-by: Keita Kobayashi <keita.kobayashi.ym@renesas.com>
> ---
>  drivers/cpuidle/Kconfig.arm | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/drivers/cpuidle/Kconfig.arm b/drivers/cpuidle/Kconfig.arm
> index 21340e0..1bff62e 100644
> --- a/drivers/cpuidle/Kconfig.arm
> +++ b/drivers/cpuidle/Kconfig.arm
> @@ -74,3 +74,11 @@ config ARM_MVEBU_V7_CPUIDLE
>         depends on ARCH_MVEBU && !ARM64
>         help
>           Select this to enable cpuidle on Armada 370, 38x and XP processors.
> +
> +config ARM_RCAR_CPUIDLE
> +       bool "CPU Idle Driver for the R-Car SoCs"
> +       depends on ARCH_RCAR_GEN2
> +       depends on ARM_CPUIDLE
> +       select ARM_CPU_SUSPEND
> +       help
> +         Select this to enable cpuidle for R-Car SoCs

Thanks for your efforts. May I ask why we need a separate Kconfig
entry for this portion? It looks a bit overkill to me.

I have not tried this myself, but it seems to me that you could simply
modify arch/arm/mach-shmobile/Kconfig something like this:

config ARCH_RCAR_GEN2
 bool
 select PM_RCAR if PM || SMP
+ select ARM_CPU_SUSPEND if ARM_CPUIDLE
 select RENESAS_IRQC

and then in patch 2/5 use ARM_CPUIDLE for the #ifdefs instead of
ARM_RCAR_CPUIDLE.

I think that would simplify things if possible.

Thanks,

/ magnus

Re: [RFC/PATCH v2 1/5] ARM: cpuidle: Add cpuidle support for R-Car Gen2 series

[Magnus Damm]

Hi Kobayashi-san,

On Thu, Apr 16, 2015 at 7:35 PM, Keita Kobayashi
<keita.kobayashi.ym@renesas.com> wrote:
> Define ARM_RCAR_CPUIDLE config item to enable cpuidle
> support for Renesas R-Car Gen2 SoCs.
>
> Signed-off-by: Keita Kobayashi <keita.kobayashi.ym@renesas.com>
> ---
>  drivers/cpuidle/Kconfig.arm | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/drivers/cpuidle/Kconfig.arm b/drivers/cpuidle/Kconfig.arm
> index 21340e0..1bff62e 100644
> --- a/drivers/cpuidle/Kconfig.arm
> +++ b/drivers/cpuidle/Kconfig.arm
> @@ -74,3 +74,11 @@ config ARM_MVEBU_V7_CPUIDLE
>         depends on ARCH_MVEBU && !ARM64
>         help
>           Select this to enable cpuidle on Armada 370, 38x and XP processors.
> +
> +config ARM_RCAR_CPUIDLE
> +       bool "CPU Idle Driver for the R-Car SoCs"
> +       depends on ARCH_RCAR_GEN2
> +       depends on ARM_CPUIDLE
> +       select ARM_CPU_SUSPEND
> +       help
> +         Select this to enable cpuidle for R-Car SoCs

Thanks for your efforts. May I ask why we need a separate Kconfig
entry for this portion? It looks a bit overkill to me.

I have not tried this myself, but it seems to me that you could simply
modify arch/arm/mach-shmobile/Kconfig something like this:

config ARCH_RCAR_GEN2
 bool
 select PM_RCAR if PM || SMP
+ select ARM_CPU_SUSPEND if ARM_CPUIDLE
 select RENESAS_IRQC

and then in patch 2/5 use ARM_CPUIDLE for the #ifdefs instead of
ARM_RCAR_CPUIDLE.

I think that would simplify things if possible.

Thanks,

/ magnus

Re: [RFC/PATCH v2 3/5] devicetree: bindings: Add new cpuidle enable method for Renesas R-car SoCs

[Magnus Damm]

Hi Kobayashi-san,

On Thu, Apr 16, 2015 at 7:35 PM, Keita Kobayashi
<keita.kobayashi.ym@renesas.com> wrote:
> This patch add the ARM CPUs Device Tree binding to document a new
> enable method of Renesas cpuidle.
>
> Signed-off-by: Keita Kobayashi <keita.kobayashi.ym@renesas.com>
> ---
>  Documentation/devicetree/bindings/arm/cpus.txt | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/Documentation/devicetree/bindings/arm/cpus.txt b/Documentation/devicetree/bindings/arm/cpus.txt
> index 8b9e0a9..663ee11 100644
> --- a/Documentation/devicetree/bindings/arm/cpus.txt
> +++ b/Documentation/devicetree/bindings/arm/cpus.txt
> @@ -196,6 +196,7 @@ nodes to be present and contain the properties described below.
>                             "qcom,gcc-msm8660"
>                             "qcom,kpss-acc-v1"
>                             "qcom,kpss-acc-v2"
> +                           "renesas,rcar-idle"
>                             "rockchip,rk3066-smp"
>
>         - cpu-release-addr

I don't mind this portion so much and I can see other vendors are
using this format. And with risk of upsetting the DT police I have to
mention my doubts about why we need this particular piece of binding.

The proposed string by this patch does not seem to describe any
hardware but it is aligned to other existing stings referring to
software such as "spin-table" and "psci". From a C code implementation
point of view we would get enough information to make a good decision
by just checking SoC compatible string. At the same time, this seems
to be the standard way of doing things and standard is good.

So if we should turn this into something that describes actual
hardware, how about pointing out the APMU hardware that is actually
managing the Core Standby mode that this CPUIdle implementation is
invoking? May I suggest using "renesas,rcar-apmu"? Or perhaps to be
safe we should do per-SoC string like in other cases like
"renesas,r8a7791-apmu"? Or is there some way we can make this work
without adding a new binding?

Thanks,

/ magnus

Re: [RFC/PATCH v2 3/5] devicetree: bindings: Add new cpuidle enable method for Renesas R-car SoCs

[Magnus Damm]

Hi Kobayashi-san,

On Thu, Apr 16, 2015 at 7:35 PM, Keita Kobayashi
<keita.kobayashi.ym-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org> wrote:
> This patch add the ARM CPUs Device Tree binding to document a new
> enable method of Renesas cpuidle.
>
> Signed-off-by: Keita Kobayashi <keita.kobayashi.ym-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/arm/cpus.txt | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/Documentation/devicetree/bindings/arm/cpus.txt b/Documentation/devicetree/bindings/arm/cpus.txt
> index 8b9e0a9..663ee11 100644
> --- a/Documentation/devicetree/bindings/arm/cpus.txt
> +++ b/Documentation/devicetree/bindings/arm/cpus.txt
> @@ -196,6 +196,7 @@ nodes to be present and contain the properties described below.
>                             "qcom,gcc-msm8660"
>                             "qcom,kpss-acc-v1"
>                             "qcom,kpss-acc-v2"
> +                           "renesas,rcar-idle"
>                             "rockchip,rk3066-smp"
>
>         - cpu-release-addr

I don't mind this portion so much and I can see other vendors are
using this format. And with risk of upsetting the DT police I have to
mention my doubts about why we need this particular piece of binding.

The proposed string by this patch does not seem to describe any
hardware but it is aligned to other existing stings referring to
software such as "spin-table" and "psci". From a C code implementation
point of view we would get enough information to make a good decision
by just checking SoC compatible string. At the same time, this seems
to be the standard way of doing things and standard is good.

So if we should turn this into something that describes actual
hardware, how about pointing out the APMU hardware that is actually
managing the Core Standby mode that this CPUIdle implementation is
invoking? May I suggest using "renesas,rcar-apmu"? Or perhaps to be
safe we should do per-SoC string like in other cases like
"renesas,r8a7791-apmu"? Or is there some way we can make this work
without adding a new binding?

Thanks,

/ magnus
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH v2 3/5] devicetree: bindings: Add new cpuidle enable method for Renesas R-car SoCs

[Mark Rutland]

On Thu, Apr 16, 2015 at 11:35:38AM +0100, Keita Kobayashi wrote:
> This patch add the ARM CPUs Device Tree binding to document a new
> enable method of Renesas cpuidle.
> 
> Signed-off-by: Keita Kobayashi <keita.kobayashi.ym@renesas.com>
> ---
>  Documentation/devicetree/bindings/arm/cpus.txt | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/arm/cpus.txt b/Documentation/devicetree/bindings/arm/cpus.txt
> index 8b9e0a9..663ee11 100644
> --- a/Documentation/devicetree/bindings/arm/cpus.txt
> +++ b/Documentation/devicetree/bindings/arm/cpus.txt
> @@ -196,6 +196,7 @@ nodes to be present and contain the properties described below.
>  			    "qcom,gcc-msm8660"
>  			    "qcom,kpss-acc-v1"
>  			    "qcom,kpss-acc-v2"
> +			    "renesas,rcar-idle"

The enable-method is about how to bring the CPU up (and potentially
implies other things, like how to take it down again).

These is no code in this series to that effect, no semantics are
provided, and the name implies this is idle-specific.

So this doesn't look right, and makes no sense to me.

Mark.

Re: [RFC/PATCH v2 3/5] devicetree: bindings: Add new cpuidle enable method for Renesas R-car SoCs

[Mark Rutland]

On Thu, Apr 16, 2015 at 11:35:38AM +0100, Keita Kobayashi wrote:
> This patch add the ARM CPUs Device Tree binding to document a new
> enable method of Renesas cpuidle.
> 
> Signed-off-by: Keita Kobayashi <keita.kobayashi.ym@renesas.com>
> ---
>  Documentation/devicetree/bindings/arm/cpus.txt | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/arm/cpus.txt b/Documentation/devicetree/bindings/arm/cpus.txt
> index 8b9e0a9..663ee11 100644
> --- a/Documentation/devicetree/bindings/arm/cpus.txt
> +++ b/Documentation/devicetree/bindings/arm/cpus.txt
> @@ -196,6 +196,7 @@ nodes to be present and contain the properties described below.
>  			    "qcom,gcc-msm8660"
>  			    "qcom,kpss-acc-v1"
>  			    "qcom,kpss-acc-v2"
> +			    "renesas,rcar-idle"

The enable-method is about how to bring the CPU up (and potentially
implies other things, like how to take it down again).

These is no code in this series to that effect, no semantics are
provided, and the name implies this is idle-specific.

So this doesn't look right, and makes no sense to me.

Mark.

Re: [RFC/PATCH v2 3/5] devicetree: bindings: Add new cpuidle enable method for Renesas R-car SoCs

[Magnus Damm]

Hi Mark,

On Fri, Apr 17, 2015 at 9:46 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Thu, Apr 16, 2015 at 11:35:38AM +0100, Keita Kobayashi wrote:
>> This patch add the ARM CPUs Device Tree binding to document a new
>> enable method of Renesas cpuidle.
>>
>> Signed-off-by: Keita Kobayashi <keita.kobayashi.ym@renesas.com>
>> ---
>>  Documentation/devicetree/bindings/arm/cpus.txt | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/Documentation/devicetree/bindings/arm/cpus.txt b/Documentation/devicetree/bindings/arm/cpus.txt
>> index 8b9e0a9..663ee11 100644
>> --- a/Documentation/devicetree/bindings/arm/cpus.txt
>> +++ b/Documentation/devicetree/bindings/arm/cpus.txt
>> @@ -196,6 +196,7 @@ nodes to be present and contain the properties described below.
>>                           "qcom,gcc-msm8660"
>>                           "qcom,kpss-acc-v1"
>>                           "qcom,kpss-acc-v2"
>> +                         "renesas,rcar-idle"
>
> The enable-method is about how to bring the CPU up (and potentially
> implies other things, like how to take it down again).

Thanks for your clarification. I now understand that this is related
to SMP boot and CPU Hotplug.

> These is no code in this series to that effect, no semantics are
> provided, and the name implies this is idle-specific.
>
> So this doesn't look right, and makes no sense to me.

Ok, I somehow (incorrectly?) assumed that the following line in patch
2/5* tied into this:
+CPUIDLE_METHOD_OF_DECLARE(rcar, "renesas,rcar-idle", &rcar_cpuidle_ops);

[*] [RFC/PATCH v2 2/5] ARM: shmobile: Add cpuidle_ops for R-Car cpuidle

But if it is unrelated it should of course be dropped or reworked.

Thanks,

/ magnus

Re: [RFC/PATCH v2 3/5] devicetree: bindings: Add new cpuidle enable method for Renesas R-car SoCs

[Magnus Damm]

Hi Mark,

On Fri, Apr 17, 2015 at 9:46 PM, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:
> On Thu, Apr 16, 2015 at 11:35:38AM +0100, Keita Kobayashi wrote:
>> This patch add the ARM CPUs Device Tree binding to document a new
>> enable method of Renesas cpuidle.
>>
>> Signed-off-by: Keita Kobayashi <keita.kobayashi.ym-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>
>> ---
>>  Documentation/devicetree/bindings/arm/cpus.txt | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/Documentation/devicetree/bindings/arm/cpus.txt b/Documentation/devicetree/bindings/arm/cpus.txt
>> index 8b9e0a9..663ee11 100644
>> --- a/Documentation/devicetree/bindings/arm/cpus.txt
>> +++ b/Documentation/devicetree/bindings/arm/cpus.txt
>> @@ -196,6 +196,7 @@ nodes to be present and contain the properties described below.
>>                           "qcom,gcc-msm8660"
>>                           "qcom,kpss-acc-v1"
>>                           "qcom,kpss-acc-v2"
>> +                         "renesas,rcar-idle"
>
> The enable-method is about how to bring the CPU up (and potentially
> implies other things, like how to take it down again).

Thanks for your clarification. I now understand that this is related
to SMP boot and CPU Hotplug.

> These is no code in this series to that effect, no semantics are
> provided, and the name implies this is idle-specific.
>
> So this doesn't look right, and makes no sense to me.

Ok, I somehow (incorrectly?) assumed that the following line in patch
2/5* tied into this:
+CPUIDLE_METHOD_OF_DECLARE(rcar, "renesas,rcar-idle", &rcar_cpuidle_ops);

[*] [RFC/PATCH v2 2/5] ARM: shmobile: Add cpuidle_ops for R-Car cpuidle

But if it is unrelated it should of course be dropped or reworked.

Thanks,

/ magnus
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH v2 3/5] devicetree: bindings: Add new cpuidle enable method for Renesas R-car SoCs

[Lorenzo Pieralisi]

On Fri, Apr 17, 2015 at 02:11:24PM +0100, Magnus Damm wrote:
> Hi Mark,
> 
> On Fri, Apr 17, 2015 at 9:46 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> > On Thu, Apr 16, 2015 at 11:35:38AM +0100, Keita Kobayashi wrote:
> >> This patch add the ARM CPUs Device Tree binding to document a new
> >> enable method of Renesas cpuidle.
> >>
> >> Signed-off-by: Keita Kobayashi <keita.kobayashi.ym@renesas.com>
> >> ---
> >>  Documentation/devicetree/bindings/arm/cpus.txt | 1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/arm/cpus.txt b/Documentation/devicetree/bindings/arm/cpus.txt
> >> index 8b9e0a9..663ee11 100644
> >> --- a/Documentation/devicetree/bindings/arm/cpus.txt
> >> +++ b/Documentation/devicetree/bindings/arm/cpus.txt
> >> @@ -196,6 +196,7 @@ nodes to be present and contain the properties described below.
> >>                           "qcom,gcc-msm8660"
> >>                           "qcom,kpss-acc-v1"
> >>                           "qcom,kpss-acc-v2"
> >> +                         "renesas,rcar-idle"
> >
> > The enable-method is about how to bring the CPU up (and potentially
> > implies other things, like how to take it down again).
> 
> Thanks for your clarification. I now understand that this is related
> to SMP boot and CPU Hotplug.
> 
> > These is no code in this series to that effect, no semantics are
> > provided, and the name implies this is idle-specific.
> >
> > So this doesn't look right, and makes no sense to me.
> 
> Ok, I somehow (incorrectly?) assumed that the following line in patch
> 2/5* tied into this:
> +CPUIDLE_METHOD_OF_DECLARE(rcar, "renesas,rcar-idle", &rcar_cpuidle_ops);
> 
> [*] [RFC/PATCH v2 2/5] ARM: shmobile: Add cpuidle_ops for R-Car cpuidle
> 
> But if it is unrelated it should of course be dropped or reworked.

It is related, but Mark's remark is correct. We have to keep in mind
that an enable-method defines how a CPU is powered {up/down} and
also suspended (quiesced through idle).

It has to be defined through proper bindings and related code, adding a
random compatible string for the sake of matching does not cut it,
that is not acceptable and I stated it from the beginning.

So, to make it clear, an enable-method defines CPU operations as a
whole, cpu init, power{up/down} and suspend.

It must be documented with proper bindings that defines compatible string and
related properties.

The sooner we incorporate the CPUidle ops into SMP ops the better to
prevent this abuse from happening, an enable-method encompasses SMP
ops and CPUidle ops, actually they must be merged because they represent
the enable-method implementation as a whole.

I hope this helps.

Lorenzo

Re: [RFC/PATCH v2 3/5] devicetree: bindings: Add new cpuidle enable method for Renesas R-car SoCs

[Lorenzo Pieralisi]

On Fri, Apr 17, 2015 at 02:11:24PM +0100, Magnus Damm wrote:
> Hi Mark,
> 
> On Fri, Apr 17, 2015 at 9:46 PM, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:
> > On Thu, Apr 16, 2015 at 11:35:38AM +0100, Keita Kobayashi wrote:
> >> This patch add the ARM CPUs Device Tree binding to document a new
> >> enable method of Renesas cpuidle.
> >>
> >> Signed-off-by: Keita Kobayashi <keita.kobayashi.ym-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>
> >> ---
> >>  Documentation/devicetree/bindings/arm/cpus.txt | 1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/arm/cpus.txt b/Documentation/devicetree/bindings/arm/cpus.txt
> >> index 8b9e0a9..663ee11 100644
> >> --- a/Documentation/devicetree/bindings/arm/cpus.txt
> >> +++ b/Documentation/devicetree/bindings/arm/cpus.txt
> >> @@ -196,6 +196,7 @@ nodes to be present and contain the properties described below.
> >>                           "qcom,gcc-msm8660"
> >>                           "qcom,kpss-acc-v1"
> >>                           "qcom,kpss-acc-v2"
> >> +                         "renesas,rcar-idle"
> >
> > The enable-method is about how to bring the CPU up (and potentially
> > implies other things, like how to take it down again).
> 
> Thanks for your clarification. I now understand that this is related
> to SMP boot and CPU Hotplug.
> 
> > These is no code in this series to that effect, no semantics are
> > provided, and the name implies this is idle-specific.
> >
> > So this doesn't look right, and makes no sense to me.
> 
> Ok, I somehow (incorrectly?) assumed that the following line in patch
> 2/5* tied into this:
> +CPUIDLE_METHOD_OF_DECLARE(rcar, "renesas,rcar-idle", &rcar_cpuidle_ops);
> 
> [*] [RFC/PATCH v2 2/5] ARM: shmobile: Add cpuidle_ops for R-Car cpuidle
> 
> But if it is unrelated it should of course be dropped or reworked.

It is related, but Mark's remark is correct. We have to keep in mind
that an enable-method defines how a CPU is powered {up/down} and
also suspended (quiesced through idle).

It has to be defined through proper bindings and related code, adding a
random compatible string for the sake of matching does not cut it,
that is not acceptable and I stated it from the beginning.

So, to make it clear, an enable-method defines CPU operations as a
whole, cpu init, power{up/down} and suspend.

It must be documented with proper bindings that defines compatible string and
related properties.

The sooner we incorporate the CPUidle ops into SMP ops the better to
prevent this abuse from happening, an enable-method encompasses SMP
ops and CPUidle ops, actually they must be merged because they represent
the enable-method implementation as a whole.

I hope this helps.

Lorenzo
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH v2 1/5] ARM: cpuidle: Add cpuidle support for R-Car Gen2 series

[keita kobayashi]

Hi Magnus

(2015/04/17 21:14), Magnus Damm wrote:

> Hi Kobayashi-san,
> 
> On Thu, Apr 16, 2015 at 7:35 PM, Keita Kobayashi
> <keita.kobayashi.ym@renesas.com> wrote:
>> Define ARM_RCAR_CPUIDLE config item to enable cpuidle
>> support for Renesas R-Car Gen2 SoCs.
>>
>> Signed-off-by: Keita Kobayashi <keita.kobayashi.ym@renesas.com>
>> ---
>>  drivers/cpuidle/Kconfig.arm | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/cpuidle/Kconfig.arm b/drivers/cpuidle/Kconfig.arm
>> index 21340e0..1bff62e 100644
>> --- a/drivers/cpuidle/Kconfig.arm
>> +++ b/drivers/cpuidle/Kconfig.arm
>> @@ -74,3 +74,11 @@ config ARM_MVEBU_V7_CPUIDLE
>>         depends on ARCH_MVEBU && !ARM64
>>         help
>>           Select this to enable cpuidle on Armada 370, 38x and XP processors.
>> +
>> +config ARM_RCAR_CPUIDLE
>> +       bool "CPU Idle Driver for the R-Car SoCs"
>> +       depends on ARCH_RCAR_GEN2
>> +       depends on ARM_CPUIDLE
>> +       select ARM_CPU_SUSPEND
>> +       help
>> +         Select this to enable cpuidle for R-Car SoCs
> 
> Thanks for your efforts. May I ask why we need a separate Kconfig
> entry for this portion? It looks a bit overkill to me.
> 
> I have not tried this myself, but it seems to me that you could simply
> modify arch/arm/mach-shmobile/Kconfig something like this:
> 
> config ARCH_RCAR_GEN2
>  bool
>  select PM_RCAR if PM || SMP
> + select ARM_CPU_SUSPEND if ARM_CPUIDLE
>  select RENESAS_IRQC
> 
> and then in patch 2/5 use ARM_CPUIDLE for the #ifdefs instead of
> ARM_RCAR_CPUIDLE.
> 
> I think that would simplify things if possible.

Thank you for your proposal.
I will try it.

> 
> Thanks,
> 
> / magnus

Regards.

Keita Kobayashi

Re: [RFC/PATCH v2 1/5] ARM: cpuidle: Add cpuidle support for R-Car Gen2 series

[keita kobayashi]

Hi Magnus

(2015/04/17 21:14), Magnus Damm wrote:

> Hi Kobayashi-san,
> 
> On Thu, Apr 16, 2015 at 7:35 PM, Keita Kobayashi
> <keita.kobayashi.ym@renesas.com> wrote:
>> Define ARM_RCAR_CPUIDLE config item to enable cpuidle
>> support for Renesas R-Car Gen2 SoCs.
>>
>> Signed-off-by: Keita Kobayashi <keita.kobayashi.ym@renesas.com>
>> ---
>>  drivers/cpuidle/Kconfig.arm | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/cpuidle/Kconfig.arm b/drivers/cpuidle/Kconfig.arm
>> index 21340e0..1bff62e 100644
>> --- a/drivers/cpuidle/Kconfig.arm
>> +++ b/drivers/cpuidle/Kconfig.arm
>> @@ -74,3 +74,11 @@ config ARM_MVEBU_V7_CPUIDLE
>>         depends on ARCH_MVEBU && !ARM64
>>         help
>>           Select this to enable cpuidle on Armada 370, 38x and XP processors.
>> +
>> +config ARM_RCAR_CPUIDLE
>> +       bool "CPU Idle Driver for the R-Car SoCs"
>> +       depends on ARCH_RCAR_GEN2
>> +       depends on ARM_CPUIDLE
>> +       select ARM_CPU_SUSPEND
>> +       help
>> +         Select this to enable cpuidle for R-Car SoCs
> 
> Thanks for your efforts. May I ask why we need a separate Kconfig
> entry for this portion? It looks a bit overkill to me.
> 
> I have not tried this myself, but it seems to me that you could simply
> modify arch/arm/mach-shmobile/Kconfig something like this:
> 
> config ARCH_RCAR_GEN2
>  bool
>  select PM_RCAR if PM || SMP
> + select ARM_CPU_SUSPEND if ARM_CPUIDLE
>  select RENESAS_IRQC
> 
> and then in patch 2/5 use ARM_CPUIDLE for the #ifdefs instead of
> ARM_RCAR_CPUIDLE.
> 
> I think that would simplify things if possible.

Thank you for your proposal.
I will try it.

> 
> Thanks,
> 
> / magnus

Regards.

Keita Kobayashi

Re: [RFC/PATCH v2 3/5] devicetree: bindings: Add new cpuidle enable method for Renesas R-car SoCs

[keita kobayashi]

Hi Magnus, Mark, Lorenzo

Thank you for your comment.

(2015/04/17 23:37), Lorenzo Pieralisi wrote:

> On Fri, Apr 17, 2015 at 02:11:24PM +0100, Magnus Damm wrote:
>> Hi Mark,
>>
>> On Fri, Apr 17, 2015 at 9:46 PM, Mark Rutland <mark.rutland@arm.com> wrote:
>>> On Thu, Apr 16, 2015 at 11:35:38AM +0100, Keita Kobayashi wrote:
>>>> This patch add the ARM CPUs Device Tree binding to document a new
>>>> enable method of Renesas cpuidle.
>>>>
>>>> Signed-off-by: Keita Kobayashi <keita.kobayashi.ym@renesas.com>
>>>> ---
>>>>  Documentation/devicetree/bindings/arm/cpus.txt | 1 +
>>>>  1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/arm/cpus.txt b/Documentation/devicetree/bindings/arm/cpus.txt
>>>> index 8b9e0a9..663ee11 100644
>>>> --- a/Documentation/devicetree/bindings/arm/cpus.txt
>>>> +++ b/Documentation/devicetree/bindings/arm/cpus.txt
>>>> @@ -196,6 +196,7 @@ nodes to be present and contain the properties described below.
>>>>                           "qcom,gcc-msm8660"
>>>>                           "qcom,kpss-acc-v1"
>>>>                           "qcom,kpss-acc-v2"
>>>> +                         "renesas,rcar-idle"
>>>
>>> The enable-method is about how to bring the CPU up (and potentially
>>> implies other things, like how to take it down again).
>>
>> Thanks for your clarification. I now understand that this is related
>> to SMP boot and CPU Hotplug.
>>
>>> These is no code in this series to that effect, no semantics are
>>> provided, and the name implies this is idle-specific.
>>>
>>> So this doesn't look right, and makes no sense to me.
>>
>> Ok, I somehow (incorrectly?) assumed that the following line in patch
>> 2/5* tied into this:
>> +CPUIDLE_METHOD_OF_DECLARE(rcar, "renesas,rcar-idle", &rcar_cpuidle_ops);
>>
>> [*] [RFC/PATCH v2 2/5] ARM: shmobile: Add cpuidle_ops for R-Car cpuidle
>>
>> But if it is unrelated it should of course be dropped or reworked.
> 
> It is related, but Mark's remark is correct. We have to keep in mind
> that an enable-method defines how a CPU is powered {up/down} and
> also suspended (quiesced through idle).
> 
> It has to be defined through proper bindings and related code, adding a
> random compatible string for the sake of matching does not cut it,
> that is not acceptable and I stated it from the beginning.
> 
> So, to make it clear, an enable-method defines CPU operations as a
> whole, cpu init, power{up/down} and suspend.
> 
> It must be documented with proper bindings that defines compatible string and
> related properties.
> 
> The sooner we incorporate the CPUidle ops into SMP ops the better to
> prevent this abuse from happening, an enable-method encompasses SMP
> ops and CPUidle ops, actually they must be merged because they represent
> the enable-method implementation as a whole.
> 
> I hope this helps.
> 
> Lorenzo


I will rename "renesas,rcar-idle" in the next patch.

Regards.

Keita Kobayashi

Re: [RFC/PATCH v2 3/5] devicetree: bindings: Add new cpuidle enable method for Renesas R-car SoCs

[keita kobayashi]

Hi Magnus, Mark, Lorenzo

Thank you for your comment.

(2015/04/17 23:37), Lorenzo Pieralisi wrote:

> On Fri, Apr 17, 2015 at 02:11:24PM +0100, Magnus Damm wrote:
>> Hi Mark,
>>
>> On Fri, Apr 17, 2015 at 9:46 PM, Mark Rutland <mark.rutland@arm.com> wrote:
>>> On Thu, Apr 16, 2015 at 11:35:38AM +0100, Keita Kobayashi wrote:
>>>> This patch add the ARM CPUs Device Tree binding to document a new
>>>> enable method of Renesas cpuidle.
>>>>
>>>> Signed-off-by: Keita Kobayashi <keita.kobayashi.ym@renesas.com>
>>>> ---
>>>>  Documentation/devicetree/bindings/arm/cpus.txt | 1 +
>>>>  1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/arm/cpus.txt b/Documentation/devicetree/bindings/arm/cpus.txt
>>>> index 8b9e0a9..663ee11 100644
>>>> --- a/Documentation/devicetree/bindings/arm/cpus.txt
>>>> +++ b/Documentation/devicetree/bindings/arm/cpus.txt
>>>> @@ -196,6 +196,7 @@ nodes to be present and contain the properties described below.
>>>>                           "qcom,gcc-msm8660"
>>>>                           "qcom,kpss-acc-v1"
>>>>                           "qcom,kpss-acc-v2"
>>>> +                         "renesas,rcar-idle"
>>>
>>> The enable-method is about how to bring the CPU up (and potentially
>>> implies other things, like how to take it down again).
>>
>> Thanks for your clarification. I now understand that this is related
>> to SMP boot and CPU Hotplug.
>>
>>> These is no code in this series to that effect, no semantics are
>>> provided, and the name implies this is idle-specific.
>>>
>>> So this doesn't look right, and makes no sense to me.
>>
>> Ok, I somehow (incorrectly?) assumed that the following line in patch
>> 2/5* tied into this:
>> +CPUIDLE_METHOD_OF_DECLARE(rcar, "renesas,rcar-idle", &rcar_cpuidle_ops);
>>
>> [*] [RFC/PATCH v2 2/5] ARM: shmobile: Add cpuidle_ops for R-Car cpuidle
>>
>> But if it is unrelated it should of course be dropped or reworked.
> 
> It is related, but Mark's remark is correct. We have to keep in mind
> that an enable-method defines how a CPU is powered {up/down} and
> also suspended (quiesced through idle).
> 
> It has to be defined through proper bindings and related code, adding a
> random compatible string for the sake of matching does not cut it,
> that is not acceptable and I stated it from the beginning.
> 
> So, to make it clear, an enable-method defines CPU operations as a
> whole, cpu init, power{up/down} and suspend.
> 
> It must be documented with proper bindings that defines compatible string and
> related properties.
> 
> The sooner we incorporate the CPUidle ops into SMP ops the better to
> prevent this abuse from happening, an enable-method encompasses SMP
> ops and CPUidle ops, actually they must be merged because they represent
> the enable-method implementation as a whole.
> 
> I hope this helps.
> 
> Lorenzo


I will rename "renesas,rcar-idle" in the next patch.

Regards.

Keita Kobayashi

Re: [RFC/PATCH v2 3/5] devicetree: bindings: Add new cpuidle enable method for Renesas R-car SoCs

[Lorenzo Pieralisi]

On Mon, Apr 20, 2015 at 08:39:35AM +0100, keita kobayashi wrote:

[...]

> > It is related, but Mark's remark is correct. We have to keep in mind
> > that an enable-method defines how a CPU is powered {up/down} and
> > also suspended (quiesced through idle).
> > 
> > It has to be defined through proper bindings and related code, adding a
> > random compatible string for the sake of matching does not cut it,
> > that is not acceptable and I stated it from the beginning.
> > 
> > So, to make it clear, an enable-method defines CPU operations as a
> > whole, cpu init, power{up/down} and suspend.
> > 
> > It must be documented with proper bindings that defines compatible string and
> > related properties.
> > 
> > The sooner we incorporate the CPUidle ops into SMP ops the better to
> > prevent this abuse from happening, an enable-method encompasses SMP
> > ops and CPUidle ops, actually they must be merged because they represent
> > the enable-method implementation as a whole.
> > 
> > I hope this helps.
> > 
> > Lorenzo
> 
> 
> I will rename "renesas,rcar-idle" in the next patch.

It is not just about renaming a compatible string, it is about defining
a DT binding and related enable-method as a whole.

Thanks,
Lorenzo

Re: [RFC/PATCH v2 3/5] devicetree: bindings: Add new cpuidle enable method for Renesas R-car SoCs

[Lorenzo Pieralisi]

On Mon, Apr 20, 2015 at 08:39:35AM +0100, keita kobayashi wrote:

[...]

> > It is related, but Mark's remark is correct. We have to keep in mind
> > that an enable-method defines how a CPU is powered {up/down} and
> > also suspended (quiesced through idle).
> > 
> > It has to be defined through proper bindings and related code, adding a
> > random compatible string for the sake of matching does not cut it,
> > that is not acceptable and I stated it from the beginning.
> > 
> > So, to make it clear, an enable-method defines CPU operations as a
> > whole, cpu init, power{up/down} and suspend.
> > 
> > It must be documented with proper bindings that defines compatible string and
> > related properties.
> > 
> > The sooner we incorporate the CPUidle ops into SMP ops the better to
> > prevent this abuse from happening, an enable-method encompasses SMP
> > ops and CPUidle ops, actually they must be merged because they represent
> > the enable-method implementation as a whole.
> > 
> > I hope this helps.
> > 
> > Lorenzo
> 
> 
> I will rename "renesas,rcar-idle" in the next patch.

It is not just about renaming a compatible string, it is about defining
a DT binding and related enable-method as a whole.

Thanks,
Lorenzo