linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v1 0/5] Add cpuidle support for Tegra194
@ 2021-03-04  6:08 Sowjanya Komatineni
  2021-03-04  6:08 ` [PATCH v1 1/5] MAINTAINERS: Add Tegra CPUIDLE driver section Sowjanya Komatineni
                   ` (4 more replies)
  0 siblings, 5 replies; 19+ messages in thread
From: Sowjanya Komatineni @ 2021-03-04  6:08 UTC (permalink / raw)
  To: thierry.reding, jonathanh, skomatineni, daniel.lezcano, robh+dt
  Cc: ksitaraman, sanjayc, linux-tegra, linux-kernel, linux-pm, devicetree

This series adds cpuidle support for Tegra194 carmel CPUs.

MCE firmware is responsible for deciding on CPU idle power state
based on state information and MCE firmware background work.

Tegra MCE ARI driver is the interface driver to communicate with
MCE firmware from the kernel.

CPU idle driver passes idle state information to MCE through Tegra
MCE driver and requests idle state transition to MCE happens through
PSCI CPU suspend.

This series includes below patches
- Add CPUIDLE section to MAINTAINERS
- Add Tegra MCE ARI driver to communicate with MCE firmware from kernel
- Add dt-bindings for Tegra194 cpu idle states
- Add cpuidle driver to support Tegra194 CPUs idle state management
- Update Tegra194 device tree with cpuidle support to Tegra194 CPUs.


Sowjanya Komatineni (5):
  MAINTAINERS: Add Tegra CPUIDLE driver section
  firmware: tegra: Add Tegra194 MCE ARI driver
  dt-bindings: arm: Add cpu-idle-states to Tegra194 CPU nodes
  cpuidle: Add Tegra194 cpuidle driver
  arm64: dts: tegra194: Add CPU idle states

 .../bindings/arm/nvidia,tegra194-ccplex.yaml       |  53 ++++
 MAINTAINERS                                        |  12 +
 arch/arm64/boot/dts/nvidia/tegra194.dtsi           |  28 ++
 drivers/cpuidle/Kconfig.arm                        |  10 +
 drivers/cpuidle/Makefile                           |   1 +
 drivers/cpuidle/cpuidle-tegra194.c                 | 319 +++++++++++++++++++++
 drivers/firmware/tegra/Kconfig                     |  11 +
 drivers/firmware/tegra/Makefile                    |   4 +
 drivers/firmware/tegra/mce-tegra194.c              | 155 ++++++++++
 drivers/firmware/tegra/mce.c                       |  88 ++++++
 include/soc/tegra/mce.h                            |  32 +++
 include/soc/tegra/t194_nvg.h                       |  56 ++++
 12 files changed, 769 insertions(+)
 create mode 100644 drivers/cpuidle/cpuidle-tegra194.c
 create mode 100644 drivers/firmware/tegra/mce-tegra194.c
 create mode 100644 drivers/firmware/tegra/mce.c
 create mode 100644 include/soc/tegra/mce.h
 create mode 100644 include/soc/tegra/t194_nvg.h

-- 
2.7.4


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v1 1/5] MAINTAINERS: Add Tegra CPUIDLE driver section
  2021-03-04  6:08 [PATCH v1 0/5] Add cpuidle support for Tegra194 Sowjanya Komatineni
@ 2021-03-04  6:08 ` Sowjanya Komatineni
  2021-03-04  8:01   ` Daniel Lezcano
  2021-03-04  6:08 ` [PATCH v1 2/5] firmware: tegra: Add Tegra194 MCE ARI driver Sowjanya Komatineni
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 19+ messages in thread
From: Sowjanya Komatineni @ 2021-03-04  6:08 UTC (permalink / raw)
  To: thierry.reding, jonathanh, skomatineni, daniel.lezcano, robh+dt
  Cc: ksitaraman, sanjayc, linux-tegra, linux-kernel, linux-pm, devicetree

Add Tegra CPUIDLE driver section with maintainers and mailing list
entries.

Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
---
 MAINTAINERS | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index cac8429..277fcfd 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4679,6 +4679,18 @@ S:	Supported
 F:	drivers/cpuidle/cpuidle-psci.h
 F:	drivers/cpuidle/cpuidle-psci-domain.c
 
+CPUIDLE DRIVER - TEGRA194
+M:	Thierry Reding <thierry.reding@gmail.com>
+M:	Jonathan Hunter <jonathanh@nvidia.com>
+M:	Krishna Sitaraman <ksitaraman@nvidia.com>
+M:	Sanjay Chandrashekara <sanjayc@nvidia.com>
+M:	Sowjanya Komatineni <skomatineni@nvidia.com>
+L:	linux-pm@vger.kernel.org
+L:	linux-tegra@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/arm/nvidia,tegra194-ccplex.yaml
+F:	drivers/cpuidle/cpuidle-tegra194.c
+
 CRAMFS FILESYSTEM
 M:	Nicolas Pitre <nico@fluxnic.net>
 S:	Maintained
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v1 2/5] firmware: tegra: Add Tegra194 MCE ARI driver
  2021-03-04  6:08 [PATCH v1 0/5] Add cpuidle support for Tegra194 Sowjanya Komatineni
  2021-03-04  6:08 ` [PATCH v1 1/5] MAINTAINERS: Add Tegra CPUIDLE driver section Sowjanya Komatineni
@ 2021-03-04  6:08 ` Sowjanya Komatineni
  2021-03-04  6:08 ` [PATCH v1 3/5] dt-bindings: arm: Add cpu-idle-states to Tegra194 CPU nodes Sowjanya Komatineni
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 19+ messages in thread
From: Sowjanya Komatineni @ 2021-03-04  6:08 UTC (permalink / raw)
  To: thierry.reding, jonathanh, skomatineni, daniel.lezcano, robh+dt
  Cc: ksitaraman, sanjayc, linux-tegra, linux-kernel, linux-pm, devicetree

Tegra MCE Abstract Request Interface (ARI) driver manages all NVG
requests to MCE firmware running in the background.

This patch adds Tegra194 MCE interface driver for communicating with
MCE firmware on CPU state configurations and state transition requests
from the CPU idle driver.

Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
---
 drivers/firmware/tegra/Kconfig        |  11 +++
 drivers/firmware/tegra/Makefile       |   4 +
 drivers/firmware/tegra/mce-tegra194.c | 155 ++++++++++++++++++++++++++++++++++
 drivers/firmware/tegra/mce.c          |  88 +++++++++++++++++++
 include/soc/tegra/mce.h               |  32 +++++++
 include/soc/tegra/t194_nvg.h          |  56 ++++++++++++
 6 files changed, 346 insertions(+)
 create mode 100644 drivers/firmware/tegra/mce-tegra194.c
 create mode 100644 drivers/firmware/tegra/mce.c
 create mode 100644 include/soc/tegra/mce.h
 create mode 100644 include/soc/tegra/t194_nvg.h

diff --git a/drivers/firmware/tegra/Kconfig b/drivers/firmware/tegra/Kconfig
index 1c8ba1f..a14ef1c 100644
--- a/drivers/firmware/tegra/Kconfig
+++ b/drivers/firmware/tegra/Kconfig
@@ -23,4 +23,15 @@ config TEGRA_BPMP
 	  This driver manages the IPC interface between host CPU and the
 	  firmware running on BPMP.
 
+config TEGRA_MCE
+	bool "Tegra MCE driver"
+	depends on ARCH_TEGRA_194_SOC
+	help
+	  MCE (Micro Codec Engine) firmware is in charge of CPUs power state
+	  transitions.
+
+	  Tegra MCE driver is an interface driver to communicate with MCE
+	  firmware for all the CPU power state change requests from CPU idle
+	  driver.
+
 endmenu
diff --git a/drivers/firmware/tegra/Makefile b/drivers/firmware/tegra/Makefile
index 49c87e0..2c0417e 100644
--- a/drivers/firmware/tegra/Makefile
+++ b/drivers/firmware/tegra/Makefile
@@ -6,3 +6,7 @@ tegra-bpmp-$(CONFIG_ARCH_TEGRA_194_SOC)	+= bpmp-tegra186.o
 tegra-bpmp-$(CONFIG_DEBUG_FS)	+= bpmp-debugfs.o
 obj-$(CONFIG_TEGRA_BPMP)	+= tegra-bpmp.o
 obj-$(CONFIG_TEGRA_IVC)		+= ivc.o
+
+tegra-mce-y			= mce.o
+tegra-mce-$(CONFIG_ARCH_TEGRA_194_SOC)	+= mce-tegra194.o
+obj-$(CONFIG_TEGRA_MCE)		+= tegra-mce.o
diff --git a/drivers/firmware/tegra/mce-tegra194.c b/drivers/firmware/tegra/mce-tegra194.c
new file mode 100644
index 0000000..5cec761d
--- /dev/null
+++ b/drivers/firmware/tegra/mce-tegra194.c
@@ -0,0 +1,155 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2021, NVIDIA CORPORATION.
+ */
+
+#include <linux/init.h>
+#include <linux/module.h>
+#include <soc/tegra/t194_nvg.h>
+#include <soc/tegra/mce.h>
+
+#include <asm/smp_plat.h>
+
+/* Issue a NVG request with data */
+static noinline notrace void nvg_send_req_data(u64 req, u64 data)
+{
+	asm volatile ("msr s3_0_c15_c1_2, %0\n"
+		      "msr s3_0_c15_c1_3, %1\n"
+		      :: "r" (req), "r" (data));
+}
+
+/* Issue a NVG request with no data */
+static noinline notrace void nvg_send_req(u64 req)
+{
+	asm volatile ("msr s3_0_c15_c1_2, %0\n" :: "r" (req));
+}
+
+/* Issue a NVG request to read the command response */
+static noinline notrace u64 nvg_get_response(void)
+{
+	u64 ret;
+
+	asm volatile ("mrs %0, s3_0_c15_c1_3" : "=r" (ret));
+
+	return ret;
+}
+
+static int tegra194_mce_update_cstate_info(u32 cluster, u32 ccplex, u32 system,
+					   u8 force, u32 wake_mask, bool valid)
+{
+	nvg_cstate_info_channel_t cstate_info = { 0 };
+
+	/* disable preemption */
+	preempt_disable();
+
+	/* update CLUSTER_CSTATE? */
+	if (cluster) {
+		cstate_info.bits.cluster_state = cluster;
+		cstate_info.bits.update_cluster = 1;
+	}
+
+	/* update CCPLEX_CSTATE? */
+	if (ccplex) {
+		cstate_info.bits.cg_cstate = ccplex;
+		cstate_info.bits.update_cg = 1;
+	}
+
+	/* update SYSTEM_CSTATE? */
+	if (system) {
+		cstate_info.bits.system_cstate = system;
+		cstate_info.bits.update_system = 1;
+	}
+
+	/* update wake mask value? */
+	if (valid)
+		cstate_info.bits.update_wake_mask = 1;
+
+	/* set the wake mask */
+	cstate_info.bits.wake_mask = wake_mask;
+
+	/* set the updated cstate info */
+	nvg_send_req_data(TEGRA_NVG_CHANNEL_CSTATE_INFO, cstate_info.flat);
+
+	/* enable preemption */
+	preempt_enable();
+
+	return 0;
+}
+
+static int tegra194_mce_update_crossover_time(u32 type, u32 time)
+{
+	if (type != TEGRA_NVG_CHANNEL_CROSSOVER_C6_LOWER_BOUND &&
+	    type != TEGRA_NVG_CHANNEL_CROSSOVER_CC6_LOWER_BOUND &&
+	    type != TEGRA_NVG_CHANNEL_CROSSOVER_CG7_LOWER_BOUND) {
+		pr_err("%s: unknown crossover type (%d)\n", __func__, type);
+		return -EINVAL;
+	}
+
+	/* disable pre-emption*/
+	preempt_disable();
+
+	nvg_send_req_data(type, (u64)time);
+
+	/* enable pre-emption */
+	preempt_enable();
+
+	return 0;
+}
+
+static int tegra194_mce_read_cstate_stats(u32 state, u64 *stats)
+{
+	if (!stats)
+		return -EINVAL;
+
+	/* disable preemption */
+	preempt_disable();
+
+	nvg_send_req_data(TEGRA_NVG_CHANNEL_CSTATE_STAT_QUERY_REQUEST, (u64)state);
+	nvg_send_req(TEGRA_NVG_CHANNEL_CSTATE_STAT_QUERY_VALUE);
+	*stats = nvg_get_response();
+
+	/* enable preemption */
+	preempt_enable();
+
+	return 0;
+}
+
+static int tegra194_mce_read_versions(u32 *major, u32 *minor)
+{
+	u64 version;
+
+	if (!major || !minor)
+		return -EINVAL;
+
+	/* disable preemption */
+	preempt_disable();
+
+	nvg_send_req(TEGRA_NVG_CHANNEL_VERSION);
+	version = nvg_get_response();
+	*minor = (u32)version;
+	*major = (u32)(version >> 32);
+
+	/* enable preemption */
+	preempt_enable();
+
+	return 0;
+}
+
+static struct tegra_mce_ops t194_mce_ops = {
+	.update_cstate_info = tegra194_mce_update_cstate_info,
+	.update_crossover_time = tegra194_mce_update_crossover_time,
+	.read_cstate_stats = tegra194_mce_read_cstate_stats,
+	.read_versions = tegra194_mce_read_versions,
+};
+
+static int __init tegra194_mce_early_init(void)
+{
+	tegra_mce_set_ops(&t194_mce_ops);
+
+	return 0;
+}
+early_initcall(tegra194_mce_early_init);
+
+MODULE_DESCRIPTION("NVIDIA Tegra194 MCE driver");
+MODULE_AUTHOR("NVIDIA Corporation");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/firmware/tegra/mce.c b/drivers/firmware/tegra/mce.c
new file mode 100644
index 0000000..6d949ff
--- /dev/null
+++ b/drivers/firmware/tegra/mce.c
@@ -0,0 +1,88 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2021, NVIDIA CORPORATION.
+ */
+
+#include <linux/module.h>
+#include <soc/tegra/mce.h>
+
+static struct tegra_mce_ops *mce_ops;
+
+void tegra_mce_set_ops(struct tegra_mce_ops *tegra_plat_mce_ops)
+{
+	mce_ops = tegra_plat_mce_ops;
+}
+
+/**
+ * Specify deepest cluster/ccplex/system states allowed.
+ *
+ * @cluster:	deepest cluster-wide state
+ * @ccplex:	deepest ccplex-wide state
+ * @system:	deepest system-wide state
+ * @force:	forced system state
+ * @wake_mask:	wake mask to be updated
+ * @valid:	is wake_mask applicable?
+ *
+ * Returns 0 if success.
+ */
+int tegra_mce_update_cstate_info(u32 cluster, u32 ccplex, u32 system,
+				 u8 force, u32 wake_mask, bool valid)
+{
+	if (mce_ops && mce_ops->update_cstate_info)
+		return mce_ops->update_cstate_info(cluster, ccplex, system, force,
+						   wake_mask, valid);
+	else
+		return -EOPNOTSUPP;
+}
+EXPORT_SYMBOL_GPL(tegra_mce_update_cstate_info);
+
+/**
+ * Update threshold for one specific c-state crossover
+ *
+ * @type: type of state crossover.
+ * @time: idle time threshold.
+ *
+ * Returns 0 if success.
+ */
+int tegra_mce_update_crossover_time(u32 type, u32 time)
+{
+	if (mce_ops && mce_ops->update_crossover_time)
+		return mce_ops->update_crossover_time(type, time);
+	else
+		return -EOPNOTSUPP;
+}
+EXPORT_SYMBOL_GPL(tegra_mce_update_crossover_time);
+
+/**
+ * Query the runtime stats of a specific cstate
+ *
+ * @state: c-state of the stats.
+ * @stats: output integer to hold the stats.
+ *
+ * Returns 0 if success.
+ */
+int tegra_mce_read_cstate_stats(u32 state, u64 *stats)
+{
+	if (mce_ops && mce_ops->read_cstate_stats)
+		return mce_ops->read_cstate_stats(state, stats);
+	else
+		return -EOPNOTSUPP;
+}
+EXPORT_SYMBOL_GPL(tegra_mce_read_cstate_stats);
+
+/**
+ * Read out MCE API major/minor versions
+ *
+ * @major: output for major number.
+ * @minor: output for minor number.
+ *
+ * Returns 0 if success.
+ */
+int tegra_mce_read_versions(u32 *major, u32 *minor)
+{
+	if (mce_ops && mce_ops->read_versions)
+		return mce_ops->read_versions(major, minor);
+	else
+		return -EOPNOTSUPP;
+}
+EXPORT_SYMBOL_GPL(tegra_mce_read_versions);
diff --git a/include/soc/tegra/mce.h b/include/soc/tegra/mce.h
new file mode 100644
index 0000000..d4be89f
--- /dev/null
+++ b/include/soc/tegra/mce.h
@@ -0,0 +1,32 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (C) 2021 NVIDIA Corporation
+ */
+
+#ifndef __SOC_TEGRA_MCE_H__
+#define __SOC_TEGRA_MCE_H__
+
+/*
+ * For correct version validation, below two defines need to be
+ * updated whenever there is a new ARI implementation.
+ */
+#define CUR_ARI_VER_MAJOR	1
+#define CUR_ARI_VER_MINOR	2
+
+int tegra_mce_update_cstate_info(u32 cluster, u32 ccplex, u32 system,
+				 u8 force, u32 wake_mask, bool valid);
+int tegra_mce_update_crossover_time(u32 type, u32 time);
+int tegra_mce_read_cstate_stats(u32 state, u64 *stats);
+int tegra_mce_read_versions(u32 *major, u32 *minor);
+
+struct tegra_mce_ops {
+	int (*update_cstate_info)(u32 cluster, u32 ccplex, u32 system,
+				  u8 force, u32 wake_mask, bool valid);
+	int (*update_crossover_time)(u32 type, u32 time);
+	int (*read_cstate_stats)(u32 state, u64 *stats);
+	int (*read_versions)(u32 *major, u32 *minor);
+};
+
+void tegra_mce_set_ops(struct tegra_mce_ops *mce_ops);
+
+#endif /* __SOC_TEGRA_MCE_H__ */
diff --git a/include/soc/tegra/t194_nvg.h b/include/soc/tegra/t194_nvg.h
new file mode 100644
index 0000000..3a5d558
--- /dev/null
+++ b/include/soc/tegra/t194_nvg.h
@@ -0,0 +1,56 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (C) 2021 NVIDIA Corporation
+ */
+
+#ifndef __T194_NVG_H__
+#define __T194_NVG_H__
+
+/* Header for the NVIDIA Generic interface (NVG) */
+
+/*
+ * Major version increments may break backwards compatibility and binary
+ * compatibility. Minor version increments occur when there is only new
+ * functionality.
+ */
+enum {
+	TEGRA_NVG_VERSION_MAJOR = 6,
+	TEGRA_NVG_VERSION_MINOR = 6,
+};
+
+enum {
+	TEGRA_NVG_CHANNEL_VERSION				= 0,
+	TEGRA_NVG_CHANNEL_CSTATE_INFO				= 4,
+	TEGRA_NVG_CHANNEL_CROSSOVER_C6_LOWER_BOUND		= 5,
+	TEGRA_NVG_CHANNEL_CROSSOVER_CC6_LOWER_BOUND		= 6,
+	TEGRA_NVG_CHANNEL_CROSSOVER_CG7_LOWER_BOUND		= 8,
+	TEGRA_NVG_CHANNEL_CSTATE_STAT_QUERY_REQUEST		= 10,
+	TEGRA_NVG_CHANNEL_CSTATE_STAT_QUERY_VALUE		= 11,
+};
+
+enum {
+	TEGRA_NVG_CORE_C0 = 0,
+	TEGRA_NVG_CORE_C1 = 1,
+	TEGRA_NVG_CORE_C6 = 6,
+};
+
+/* NVG Data subformats */
+typedef union {
+	u64 flat;
+	struct nvg_cstate_info_channel_t {
+		uint32_t cluster_state		: 3;
+		uint32_t reserved_6_3		: 4;
+		uint32_t update_cluster		: 1;
+		uint32_t cg_cstate		: 3;
+		uint32_t reserved_14_11		: 4;
+		uint32_t update_cg		: 1;
+		uint32_t system_cstate		: 4;
+		uint32_t reserved_22_20		: 3;
+		uint32_t update_system		: 1;
+		uint32_t reserved_30_24		: 7;
+		uint32_t update_wake_mask	: 1;
+		uint32_t wake_mask		: 32;
+	} bits;
+} nvg_cstate_info_channel_t;
+
+#endif /* __T194_NVG_H__ */
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v1 3/5] dt-bindings: arm: Add cpu-idle-states to Tegra194 CPU nodes
  2021-03-04  6:08 [PATCH v1 0/5] Add cpuidle support for Tegra194 Sowjanya Komatineni
  2021-03-04  6:08 ` [PATCH v1 1/5] MAINTAINERS: Add Tegra CPUIDLE driver section Sowjanya Komatineni
  2021-03-04  6:08 ` [PATCH v1 2/5] firmware: tegra: Add Tegra194 MCE ARI driver Sowjanya Komatineni
@ 2021-03-04  6:08 ` Sowjanya Komatineni
  2021-03-04 20:47   ` Rob Herring
  2021-03-08  4:37   ` Sudeep Holla
  2021-03-04  6:08 ` [PATCH v1 4/5] cpuidle: Add Tegra194 cpuidle driver Sowjanya Komatineni
  2021-03-04  6:08 ` [PATCH v1 5/5] arm64: dts: tegra194: Add CPU idle states Sowjanya Komatineni
  4 siblings, 2 replies; 19+ messages in thread
From: Sowjanya Komatineni @ 2021-03-04  6:08 UTC (permalink / raw)
  To: thierry.reding, jonathanh, skomatineni, daniel.lezcano, robh+dt
  Cc: ksitaraman, sanjayc, linux-tegra, linux-kernel, linux-pm, devicetree

This patch adds cpu-idle-states and corresponding state nodes to
Tegra194 CPU in dt-binding document

Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
---
 .../bindings/arm/nvidia,tegra194-ccplex.yaml       | 53 ++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/nvidia,tegra194-ccplex.yaml b/Documentation/devicetree/bindings/arm/nvidia,tegra194-ccplex.yaml
index c9675c4..e1a5005 100644
--- a/Documentation/devicetree/bindings/arm/nvidia,tegra194-ccplex.yaml
+++ b/Documentation/devicetree/bindings/arm/nvidia,tegra194-ccplex.yaml
@@ -30,6 +30,36 @@ properties:
       Specifies the bpmp node that needs to be queried to get
       operating point data for all CPUs.
 
+  cluster-deepest-power-state:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: CPU cluster deepest power state ID.
+
+patternProperties:
+  "^[a-z0-9]+$":
+    type: object
+    description: |
+      CPU core idle state nodes.
+      Refer to Documentation/devicetree/bindings/arm/idle-states.yaml
+
+    properties:
+      compatible:
+        enum:
+          - nvidia,tegra194-cpuidle-core
+
+  cpu_crossover_thresholds:
+    type: object
+    description: CPU idle states crossover threshold time in uSec.
+
+    patternProperties:
+      "^[a-z0-9]+$":
+        type: object
+
+        properties:
+          crossover_c1_c6:
+            $ref: /schemas/types.yaml#/definitions/uint32
+          crossover_cc1_cc6:
+            $ref: /schemas/types.yaml#/definitions/uint32
+
 additionalProperties: true
 
 examples:
@@ -39,12 +69,14 @@ examples:
       nvidia,bpmp = <&bpmp>;
       #address-cells = <1>;
       #size-cells = <0>;
+      cluster-deepest-power-state = <0x6>;
 
       cpu0_0: cpu@0 {
         compatible = "nvidia,tegra194-carmel";
         device_type = "cpu";
         reg = <0x0>;
         enable-method = "psci";
+        cpu-idle-states = <&C6>;
       };
 
       cpu0_1: cpu@1 {
@@ -52,6 +84,7 @@ examples:
         device_type = "cpu";
         reg = <0x001>;
         enable-method = "psci";
+        cpu-idle-states = <&C6>;
       };
 
       cpu1_0: cpu@100 {
@@ -59,6 +92,7 @@ examples:
         device_type = "cpu";
         reg = <0x100>;
         enable-method = "psci";
+        cpu-idle-states = <&C6>;
       };
 
       cpu1_1: cpu@101 {
@@ -66,6 +100,25 @@ examples:
         device_type = "cpu";
         reg = <0x101>;
         enable-method = "psci";
+        cpu-idle-states = <&C6>;
+      };
+
+      cpu_core_power_states {
+       C6: c6 {
+         compatible = "nvidia,tegra194-cpuidle-core";
+         idle-state-name = "CPU powergated, state retained";
+         wakeup-latency-us = <2000>;
+         min-residency-us = <30000>;
+         arm,psci-suspend-param = <0x6>;
+         status = "okay";
+       };
+     };
+
+     cpu_crossover_thresholds {
+      thresholds {
+        crossover_c1_c6         = <30000>;
+        crossover_cc1_cc6       = <80000>;
       };
+     };
     };
 ...
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v1 4/5] cpuidle: Add Tegra194 cpuidle driver
  2021-03-04  6:08 [PATCH v1 0/5] Add cpuidle support for Tegra194 Sowjanya Komatineni
                   ` (2 preceding siblings ...)
  2021-03-04  6:08 ` [PATCH v1 3/5] dt-bindings: arm: Add cpu-idle-states to Tegra194 CPU nodes Sowjanya Komatineni
@ 2021-03-04  6:08 ` Sowjanya Komatineni
  2021-03-05 13:50   ` Dmitry Osipenko
  2021-03-04  6:08 ` [PATCH v1 5/5] arm64: dts: tegra194: Add CPU idle states Sowjanya Komatineni
  4 siblings, 1 reply; 19+ messages in thread
From: Sowjanya Komatineni @ 2021-03-04  6:08 UTC (permalink / raw)
  To: thierry.reding, jonathanh, skomatineni, daniel.lezcano, robh+dt
  Cc: ksitaraman, sanjayc, linux-tegra, linux-kernel, linux-pm, devicetree

This patch adds cpuidle driver for Tegra194.

Tegra194 Carmel CPU supports two core idle state C1 (clock gated)
and C6 (power gated with state restoration) and one cluster idle
state CC6 (power gated).

MCE firmware makes decision on core/cluster power state transition
based on its background tasks and states information provided by
CPU idle driver.

CPU idle driver provides deepest cluster power state, core power
state transition request, estimated time of next wake-up and states
crossover thresholds to MCE firmware through Tegra mce driver.

Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
---
 drivers/cpuidle/Kconfig.arm        |  10 ++
 drivers/cpuidle/Makefile           |   1 +
 drivers/cpuidle/cpuidle-tegra194.c | 319 +++++++++++++++++++++++++++++++++++++
 3 files changed, 330 insertions(+)
 create mode 100644 drivers/cpuidle/cpuidle-tegra194.c

diff --git a/drivers/cpuidle/Kconfig.arm b/drivers/cpuidle/Kconfig.arm
index 0844fad..e9adad1 100644
--- a/drivers/cpuidle/Kconfig.arm
+++ b/drivers/cpuidle/Kconfig.arm
@@ -105,6 +105,16 @@ config ARM_TEGRA_CPUIDLE
 	help
 	  Select this to enable cpuidle for NVIDIA Tegra20/30/114/124 SoCs.
 
+config ARM_TEGRA194_CPUIDLE
+	tristate "CPU Idle Driver for NVIDIA Tegra194 SoC"
+	depends on ARCH_TEGRA_194_SOC
+	select ARM_CPU_SUSPEND
+	select DT_IDLE_STATES
+	select TEGRA_MCE
+	default y
+	help
+	  Select this to enable cpuidle for NVIDIA Tegra194 SoC.
+
 config ARM_QCOM_SPM_CPUIDLE
 	bool "CPU Idle Driver for Qualcomm Subsystem Power Manager (SPM)"
 	depends on (ARCH_QCOM || COMPILE_TEST) && !ARM64
diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile
index 26bbc5e..4d89578 100644
--- a/drivers/cpuidle/Makefile
+++ b/drivers/cpuidle/Makefile
@@ -24,6 +24,7 @@ obj-$(CONFIG_ARM_CPUIDLE)		+= cpuidle-arm.o
 obj-$(CONFIG_ARM_PSCI_CPUIDLE)		+= cpuidle-psci.o
 obj-$(CONFIG_ARM_PSCI_CPUIDLE_DOMAIN)	+= cpuidle-psci-domain.o
 obj-$(CONFIG_ARM_TEGRA_CPUIDLE)		+= cpuidle-tegra.o
+obj-$(CONFIG_ARM_TEGRA194_CPUIDLE)      += cpuidle-tegra194.o
 obj-$(CONFIG_ARM_QCOM_SPM_CPUIDLE)	+= cpuidle-qcom-spm.o
 
 ###############################################################################
diff --git a/drivers/cpuidle/cpuidle-tegra194.c b/drivers/cpuidle/cpuidle-tegra194.c
new file mode 100644
index 0000000..4ae67ce
--- /dev/null
+++ b/drivers/cpuidle/cpuidle-tegra194.c
@@ -0,0 +1,319 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * CPU idle driver for Tegra194 CPUs
+ *
+ * Copyright (c) 2020-2021, NVIDIA Corporation.
+ */
+
+#include <asm/arch_timer.h>
+#include <asm/cpu.h>
+#include <asm/cpuidle.h>
+#include <asm/cputype.h>
+#include <asm/suspend.h>
+
+#include <linux/cpuhotplug.h>
+#include <linux/cpuidle.h>
+#include <linux/cpu_pm.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_platform.h>
+#include <linux/psci.h>
+#include <linux/suspend.h>
+
+#include <soc/tegra/mce.h>
+#include <soc/tegra/t194_nvg.h>
+
+#include "cpuidle-psci.h"
+#include "dt_idle_states.h"
+
+#define T194_NVG_CROSSOVER_C6		TEGRA_NVG_CHANNEL_CROSSOVER_C6_LOWER_BOUND
+#define T194_NVG_CROSSOVER_CC6		TEGRA_NVG_CHANNEL_CROSSOVER_CC6_LOWER_BOUND
+
+#define PSCI_PSTATE_ID_MASK		0xf
+#define PSCI_PSTATE_WKTIM_MASK		0x0ffffff0
+#define PSCI_PSTATE_WKTIM_SHIFT		4
+
+/*
+ * BG_TIME is margin added to target_residency so that actual HW has better
+ * chance entering deep idle state instead of getting back to shallower one.
+ * Units in us.
+ */
+#define BG_TIME				2000
+
+static DEFINE_PER_CPU_READ_MOSTLY(u32 *, psci_power_state);
+
+static struct cpuidle_driver t194_cpu_idle_driver;
+static enum cpuhp_state hp_state;
+static u32 deepest_cc_state;
+static u32 tsc_per_usec;
+
+static bool check_mce_version(void)
+{
+	u32 mce_version_major, mce_version_minor;
+	int err;
+
+	err = tegra_mce_read_versions(&mce_version_major, &mce_version_minor);
+	if (!err && mce_version_major >= TEGRA_NVG_VERSION_MAJOR)
+		return true;
+	else
+		return false;
+}
+
+static int
+t194_cpu_enter_state(struct cpuidle_device *dev, struct cpuidle_driver *drv, int index)
+{
+	u32 *state = __this_cpu_read(psci_power_state);
+	u32 power_state = state[index];
+	u32 wake_time;
+	int ret;
+
+	/*
+	 * MCE firmware does the state transition based on requested idle state,
+	 * state crossover thresholds and target residency time along with its
+	 * background work.
+	 * Pass the state target_residency time along with state ID to MCE
+	 * firmware through PSCI power-state value.
+	 *
+	 * LSB 8 bits of wake time is lost and only 24 MSB bits of wake time can fit
+	 * into additional bits of state value.
+	 */
+	wake_time = (drv->states[index].target_residency + BG_TIME) * tsc_per_usec;
+	power_state |= ((wake_time >> PSCI_PSTATE_WKTIM_SHIFT) & PSCI_PSTATE_WKTIM_MASK);
+
+	if ((power_state & PSCI_PSTATE_ID_MASK) == TEGRA_NVG_CORE_C6)
+		ret = CPU_PM_CPU_IDLE_ENTER_RETENTION_PARAM(psci_cpu_suspend_enter, index,
+							    power_state);
+	else
+		ret = CPU_PM_CPU_IDLE_ENTER_PARAM(psci_cpu_suspend_enter, index, power_state);
+
+	return ret;
+}
+
+static struct cpuidle_driver t194_cpu_idle_driver = {
+	.name = "tegra194_cpuidle_driver",
+	.owner = THIS_MODULE,
+	.states[0] = {
+		.enter			= t194_cpu_enter_state,
+		.exit_latency		= 1,
+		.target_residency	= 1,
+		.power_usage		= UINT_MAX,
+		.flags			= 0,
+		.name			= "c1",
+		.desc			= "CPU clock gated",
+	}
+};
+
+static const struct of_device_id t194_cpuidle_of_match[] = {
+	{ .compatible = "nvidia,tegra194-cpuidle-core", .data = t194_cpu_enter_state },
+	{ },
+};
+
+struct xover_table {
+	char *name;
+	int index;
+};
+
+static void send_crossover(void *data)
+{
+	struct device_node *of_states = (struct device_node *)data;
+	struct device_node *child;
+	u32 value;
+	int i;
+
+	struct xover_table table1[] = {
+		{"crossover_c1_c6", T194_NVG_CROSSOVER_C6},
+		{"crossover_cc1_cc6", T194_NVG_CROSSOVER_CC6},
+	};
+
+	/* pass the state crossover thresholds to MCE firmware */
+	for_each_child_of_node(of_states, child)
+		for (i = 0; i < ARRAY_SIZE(table1); i++) {
+			if (of_property_read_u32(child, table1[i].name, &value) == 0)
+				tegra_mce_update_crossover_time(table1[i].index,
+								value * tsc_per_usec);
+	}
+}
+
+static int crossover_init(struct device_node *cpunode)
+{
+	struct device_node *cpu_xover;
+
+	cpu_xover = of_get_child_by_name(cpunode, "cpu_crossover_thresholds");
+	if (!cpu_xover)
+		pr_warn("cpuidle: %s: missing crossover thresholds in dt\n", __func__);
+	else
+		on_each_cpu_mask(cpu_online_mask, send_crossover, cpu_xover, 1);
+
+	return 0;
+}
+
+static void program_cc_state(void *data)
+{
+	u32 *cc_state = (u32 *)data;
+
+	tegra_mce_update_cstate_info(*cc_state, 0, 0, 0, 0, 0);
+}
+
+static int
+tegra_suspend_notify_callback(struct notifier_block *nb, unsigned long action, void *pcpu)
+{
+	switch (action) {
+	case PM_POST_SUSPEND:
+	/*
+	 * Re-program deepest allowed cluster and cluster group power state
+	 * after system resumes from SC7
+	 */
+		on_each_cpu_mask(cpu_online_mask, program_cc_state, &deepest_cc_state, 1);
+		break;
+	}
+
+	return NOTIFY_OK;
+}
+
+static struct notifier_block suspend_notifier = {
+	.notifier_call = tegra_suspend_notify_callback,
+};
+
+static int tegra_cpu_online(unsigned int cpu)
+{
+	/*
+	 * Program deepest allowed cluster and cluster group power state
+	 * after a core in that cluster is onlined.
+	 */
+	smp_call_function_single(cpu, program_cc_state, &deepest_cc_state, 1);
+
+	return 0;
+}
+
+static int tegra_cpuidle_psci_dt_init(struct device *dev, struct cpuidle_driver *drv)
+{
+	unsigned int state_count = drv->state_count;
+	struct device_node *cpu_node, *state_node;
+	int i, cpu, state_idx = 1, ret = 0;
+	u32 *psci_states;
+
+	psci_states = devm_kcalloc(dev, state_count, sizeof(*psci_states), GFP_KERNEL);
+	if (!psci_states)
+		return -ENOMEM;
+
+	cpu_node = of_cpu_device_node_get(cpumask_first(drv->cpumask));
+	for (i = 1; ; i++) {
+		state_node = of_get_cpu_state_node(cpu_node, i - 1);
+		if (!state_node)
+			break;
+
+		if (!of_device_is_available(state_node)) {
+			of_node_put(state_node);
+			continue;
+		}
+
+		ret = psci_dt_parse_state_node(state_node, &psci_states[state_idx++]);
+		of_node_put(state_node);
+		if (ret)
+			goto put_cpunode;
+	}
+
+	of_node_put(cpu_node);
+
+	for_each_online_cpu(cpu)
+		per_cpu(psci_power_state, cpu) = psci_states;
+
+	return 0;
+
+put_cpunode:
+	of_node_put(cpu_node);
+	return ret;
+}
+
+static int __init tegra194_cpuidle_probe(struct platform_device *pdev)
+{
+	struct cpumask *cpumask;
+	int cpu, ret;
+
+	if (!check_mce_version()) {
+		pr_err("cpuidle: incompatible MCE version, cannot register driver\n");
+		return -ENODEV;
+	}
+
+	tsc_per_usec = arch_timer_get_cntfrq() / 1000000;
+
+	cpumask = devm_kzalloc(&pdev->dev, cpumask_size(), GFP_KERNEL);
+	for_each_online_cpu(cpu)
+		cpumask_set_cpu(cpu, cpumask);
+	t194_cpu_idle_driver.cpumask = cpumask;
+
+	/*
+	 * CCPLEX MCE firmware does core/cluster state transitions based on idle
+	 * thresholds along with requested state and target residency time.
+	 * So, state idle time crossover thresholds should be provided to MCE
+	 * firmware.
+	 */
+	crossover_init(pdev->dev.of_node);
+
+	ret = of_property_read_u32(pdev->dev.of_node,
+				   "cluster-deepest-power-state", &deepest_cc_state);
+	on_each_cpu_mask(cpu_online_mask, program_cc_state, &deepest_cc_state, 1);
+
+	ret = dt_init_idle_driver(&t194_cpu_idle_driver, t194_cpuidle_of_match, 1);
+	if (ret <= 0) {
+		pr_err("cpuidle: failed to init idle driver states\n");
+		ret = -ENODEV;
+		goto probe_exit;
+	}
+
+	/* initialize cpuidle states psci power-state from DT */
+	ret = tegra_cpuidle_psci_dt_init(&pdev->dev, &t194_cpu_idle_driver);
+	if (ret)
+		goto probe_exit;
+
+	ret = cpuidle_register(&t194_cpu_idle_driver, NULL);
+	if (ret) {
+		pr_err("cpuidle: failed to register cpuidle driver\n");
+		goto probe_exit;
+	}
+
+	ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "tegra_cpu:online", tegra_cpu_online, NULL);
+	if (ret < 0) {
+		pr_err("cpuidle: failed to setup cpu hotplug state callbacks\n");
+		goto cpuhp_error;
+	}
+
+	hp_state = ret;
+
+	register_pm_notifier(&suspend_notifier);
+
+	return 0;
+
+cpuhp_error:
+	cpuidle_unregister(&t194_cpu_idle_driver);
+probe_exit:
+	return ret;
+}
+
+static int tegra194_cpuidle_remove(struct platform_device *pdev)
+{
+	unregister_pm_notifier(&suspend_notifier);
+	cpuhp_remove_state(hp_state);
+	cpuidle_unregister(&t194_cpu_idle_driver);
+	kfree(t194_cpu_idle_driver.cpumask);
+
+	return 0;
+}
+
+static const struct of_device_id tegra194_cpuidle_of_match[] = {
+	{ .compatible = "nvidia,tegra194-ccplex" },
+	{ /* sentinel */ }
+};
+
+static struct platform_driver tegra194_cpuidle_driver __refdata = {
+	.driver = {
+		.owner = THIS_MODULE,
+		.name = "cpuidle-tegra194",
+		.of_match_table = of_match_ptr(tegra194_cpuidle_of_match),
+	},
+	.probe	= tegra194_cpuidle_probe,
+	.remove	= tegra194_cpuidle_remove,
+};
+
+module_platform_driver(tegra194_cpuidle_driver);
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v1 5/5] arm64: dts: tegra194: Add CPU idle states
  2021-03-04  6:08 [PATCH v1 0/5] Add cpuidle support for Tegra194 Sowjanya Komatineni
                   ` (3 preceding siblings ...)
  2021-03-04  6:08 ` [PATCH v1 4/5] cpuidle: Add Tegra194 cpuidle driver Sowjanya Komatineni
@ 2021-03-04  6:08 ` Sowjanya Komatineni
  4 siblings, 0 replies; 19+ messages in thread
From: Sowjanya Komatineni @ 2021-03-04  6:08 UTC (permalink / raw)
  To: thierry.reding, jonathanh, skomatineni, daniel.lezcano, robh+dt
  Cc: ksitaraman, sanjayc, linux-tegra, linux-kernel, linux-pm, devicetree

This patch adds CPU core and cluster idle states to Tegra194
device tree

Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
---
 arch/arm64/boot/dts/nvidia/tegra194.dtsi | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/arch/arm64/boot/dts/nvidia/tegra194.dtsi b/arch/arm64/boot/dts/nvidia/tegra194.dtsi
index 9449156..f9c2731 100644
--- a/arch/arm64/boot/dts/nvidia/tegra194.dtsi
+++ b/arch/arm64/boot/dts/nvidia/tegra194.dtsi
@@ -2155,12 +2155,14 @@
 		nvidia,bpmp = <&bpmp>;
 		#address-cells = <1>;
 		#size-cells = <0>;
+		cluster-deepest-power-state = <0x6>;
 
 		cpu0_0: cpu@0 {
 			compatible = "nvidia,tegra194-carmel";
 			device_type = "cpu";
 			reg = <0x000>;
 			enable-method = "psci";
+			cpu-idle-states = <&C6>;
 			i-cache-size = <131072>;
 			i-cache-line-size = <64>;
 			i-cache-sets = <512>;
@@ -2175,6 +2177,7 @@
 			device_type = "cpu";
 			reg = <0x001>;
 			enable-method = "psci";
+			cpu-idle-states = <&C6>;
 			i-cache-size = <131072>;
 			i-cache-line-size = <64>;
 			i-cache-sets = <512>;
@@ -2189,6 +2192,7 @@
 			device_type = "cpu";
 			reg = <0x100>;
 			enable-method = "psci";
+			cpu-idle-states = <&C6>;
 			i-cache-size = <131072>;
 			i-cache-line-size = <64>;
 			i-cache-sets = <512>;
@@ -2203,6 +2207,7 @@
 			device_type = "cpu";
 			reg = <0x101>;
 			enable-method = "psci";
+			cpu-idle-states = <&C6>;
 			i-cache-size = <131072>;
 			i-cache-line-size = <64>;
 			i-cache-sets = <512>;
@@ -2217,6 +2222,7 @@
 			device_type = "cpu";
 			reg = <0x200>;
 			enable-method = "psci";
+			cpu-idle-states = <&C6>;
 			i-cache-size = <131072>;
 			i-cache-line-size = <64>;
 			i-cache-sets = <512>;
@@ -2231,6 +2237,7 @@
 			device_type = "cpu";
 			reg = <0x201>;
 			enable-method = "psci";
+			cpu-idle-states = <&C6>;
 			i-cache-size = <131072>;
 			i-cache-line-size = <64>;
 			i-cache-sets = <512>;
@@ -2245,6 +2252,7 @@
 			device_type = "cpu";
 			reg = <0x300>;
 			enable-method = "psci";
+			cpu-idle-states = <&C6>;
 			i-cache-size = <131072>;
 			i-cache-line-size = <64>;
 			i-cache-sets = <512>;
@@ -2259,6 +2267,7 @@
 			device_type = "cpu";
 			reg = <0x301>;
 			enable-method = "psci";
+			cpu-idle-states = <&C6>;
 			i-cache-size = <131072>;
 			i-cache-line-size = <64>;
 			i-cache-sets = <512>;
@@ -2343,12 +2352,31 @@
 			cache-line-size = <64>;
 			cache-sets = <4096>;
 		};
+
+		cpu_core_power_states {
+			C6: c6 {
+				compatible = "nvidia,tegra194-cpuidle-core";
+				idle-state-name = "CPU powergated, state retained";
+				wakeup-latency-us = <2000>;
+				min-residency-us = <30000>;
+				arm,psci-suspend-param = <0x6>;
+				status = "okay";
+			};
+		};
+
+		cpu_crossover_thresholds {
+			thresholds {
+				crossover_c1_c6 = <30000>;
+				crossover_cc1_cc6 = <80000>;
+			};
+		};
 	};
 
 	psci {
 		compatible = "arm,psci-1.0";
 		status = "okay";
 		method = "smc";
+		cpu_suspend = <0xC4000001>;
 	};
 
 	sound {
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [PATCH v1 1/5] MAINTAINERS: Add Tegra CPUIDLE driver section
  2021-03-04  6:08 ` [PATCH v1 1/5] MAINTAINERS: Add Tegra CPUIDLE driver section Sowjanya Komatineni
@ 2021-03-04  8:01   ` Daniel Lezcano
  0 siblings, 0 replies; 19+ messages in thread
From: Daniel Lezcano @ 2021-03-04  8:01 UTC (permalink / raw)
  To: Sowjanya Komatineni, thierry.reding, jonathanh, robh+dt
  Cc: ksitaraman, sanjayc, linux-tegra, linux-kernel, linux-pm, devicetree

On 04/03/2021 07:08, Sowjanya Komatineni wrote:
> Add Tegra CPUIDLE driver section with maintainers and mailing list
> entries.
> 
> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
> ---
>  MAINTAINERS | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index cac8429..277fcfd 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4679,6 +4679,18 @@ S:	Supported
>  F:	drivers/cpuidle/cpuidle-psci.h
>  F:	drivers/cpuidle/cpuidle-psci-domain.c
>  
> +CPUIDLE DRIVER - TEGRA194
> +M:	Thierry Reding <thierry.reding@gmail.com>
> +M:	Jonathan Hunter <jonathanh@nvidia.com>
> +M:	Krishna Sitaraman <ksitaraman@nvidia.com>
> +M:	Sanjay Chandrashekara <sanjayc@nvidia.com>
> +M:	Sowjanya Komatineni <skomatineni@nvidia.com>

It does not make sense to have so many maintainers for a single file.


> +L:	linux-pm@vger.kernel.org
> +L:	linux-tegra@vger.kernel.org
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/arm/nvidia,tegra194-ccplex.yaml
> +F:	drivers/cpuidle/cpuidle-tegra194.c
> +
>  CRAMFS FILESYSTEM
>  M:	Nicolas Pitre <nico@fluxnic.net>
>  S:	Maintained
> 


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v1 3/5] dt-bindings: arm: Add cpu-idle-states to Tegra194 CPU nodes
  2021-03-04  6:08 ` [PATCH v1 3/5] dt-bindings: arm: Add cpu-idle-states to Tegra194 CPU nodes Sowjanya Komatineni
@ 2021-03-04 20:47   ` Rob Herring
  2021-03-08  4:37   ` Sudeep Holla
  1 sibling, 0 replies; 19+ messages in thread
From: Rob Herring @ 2021-03-04 20:47 UTC (permalink / raw)
  To: Sowjanya Komatineni
  Cc: linux-pm, devicetree, jonathanh, linux-kernel, linux-tegra,
	sanjayc, thierry.reding, robh+dt, daniel.lezcano, ksitaraman

On Wed, 03 Mar 2021 22:08:10 -0800, Sowjanya Komatineni wrote:
> This patch adds cpu-idle-states and corresponding state nodes to
> Tegra194 CPU in dt-binding document
> 
> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
> ---
>  .../bindings/arm/nvidia,tegra194-ccplex.yaml       | 53 ++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/arm/nvidia,tegra194-ccplex.example.dt.yaml: cpus: compatible: ['nvidia,tegra194-ccplex'] is not of type 'object'
	From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/arm/nvidia,tegra194-ccplex.yaml

See https://patchwork.ozlabs.org/patch/1447108

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v1 4/5] cpuidle: Add Tegra194 cpuidle driver
  2021-03-04  6:08 ` [PATCH v1 4/5] cpuidle: Add Tegra194 cpuidle driver Sowjanya Komatineni
@ 2021-03-05 13:50   ` Dmitry Osipenko
  0 siblings, 0 replies; 19+ messages in thread
From: Dmitry Osipenko @ 2021-03-05 13:50 UTC (permalink / raw)
  To: Sowjanya Komatineni, thierry.reding, jonathanh, daniel.lezcano, robh+dt
  Cc: ksitaraman, sanjayc, linux-tegra, linux-kernel, linux-pm, devicetree

04.03.2021 09:08, Sowjanya Komatineni пишет:
...
> +static int __init tegra194_cpuidle_probe(struct platform_device *pdev)
> +{
> +	struct cpumask *cpumask;
> +	int cpu, ret;
> +
> +	if (!check_mce_version()) {
> +		pr_err("cpuidle: incompatible MCE version, cannot register driver\n");

Should be dev_err() everywhere.

> +		return -ENODEV;
> +	}
> +
> +	tsc_per_usec = arch_timer_get_cntfrq() / 1000000;
> +
> +	cpumask = devm_kzalloc(&pdev->dev, cpumask_size(), GFP_KERNEL);
> +	for_each_online_cpu(cpu)
> +		cpumask_set_cpu(cpu, cpumask);

cpumask_copy(..)?

> +	t194_cpu_idle_driver.cpumask = cpumask;

Depending on 'online' mask instead of the 'present' mask looks odd. Is
this really intended to be so?

...
> +static int tegra194_cpuidle_remove(struct platform_device *pdev)
> +{
> +	unregister_pm_notifier(&suspend_notifier);
> +	cpuhp_remove_state(hp_state);
> +	cpuidle_unregister(&t194_cpu_idle_driver);
> +	kfree(t194_cpu_idle_driver.cpumask);

kfree() of a managed resource.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v1 3/5] dt-bindings: arm: Add cpu-idle-states to Tegra194 CPU nodes
  2021-03-04  6:08 ` [PATCH v1 3/5] dt-bindings: arm: Add cpu-idle-states to Tegra194 CPU nodes Sowjanya Komatineni
  2021-03-04 20:47   ` Rob Herring
@ 2021-03-08  4:37   ` Sudeep Holla
  2021-03-08 18:32     ` Sowjanya Komatineni
  1 sibling, 1 reply; 19+ messages in thread
From: Sudeep Holla @ 2021-03-08  4:37 UTC (permalink / raw)
  To: Sowjanya Komatineni
  Cc: thierry.reding, jonathanh, daniel.lezcano, robh+dt, ksitaraman,
	sanjayc, Sudeep Holla, linux-tegra, linux-kernel, linux-pm,
	devicetree

On Wed, Mar 03, 2021 at 10:08:10PM -0800, Sowjanya Komatineni wrote:
> This patch adds cpu-idle-states and corresponding state nodes to
> Tegra194 CPU in dt-binding document
>

I see that this platform has PSCI support. Can you care to explain why
you need additional DT bindings and driver for PSCI based CPU suspend.
Until the reasons are convincing, consider NACK from my side for this
driver and DT bindings. You should be really using those bindings and
the driver may be with minor changes there.

-- 
Regards,
Sudeep

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v1 3/5] dt-bindings: arm: Add cpu-idle-states to Tegra194 CPU nodes
  2021-03-08  4:37   ` Sudeep Holla
@ 2021-03-08 18:32     ` Sowjanya Komatineni
  2021-03-10 23:19       ` Sowjanya Komatineni
  2021-03-11  2:52       ` Sudeep Holla
  0 siblings, 2 replies; 19+ messages in thread
From: Sowjanya Komatineni @ 2021-03-08 18:32 UTC (permalink / raw)
  To: Sudeep Holla
  Cc: thierry.reding, jonathanh, daniel.lezcano, robh+dt, ksitaraman,
	sanjayc, linux-tegra, linux-kernel, linux-pm, devicetree


On 3/7/21 8:37 PM, Sudeep Holla wrote:
> On Wed, Mar 03, 2021 at 10:08:10PM -0800, Sowjanya Komatineni wrote:
>> This patch adds cpu-idle-states and corresponding state nodes to
>> Tegra194 CPU in dt-binding document
>>
> I see that this platform has PSCI support. Can you care to explain why
> you need additional DT bindings and driver for PSCI based CPU suspend.
> Until the reasons are convincing, consider NACK from my side for this
> driver and DT bindings. You should be really using those bindings and
> the driver may be with minor changes there.
>
MCE firmware is in charge of state transition for Tegra194 carmel CPUs.

For run-time state transitions, need to provide state request along with 
its residency time to MCE firmware which is running in the background.

State min residency is updated into power_state value along with state 
id that is passed to psci_cpu_suspend_enter

Also states cross-over idle times need to be provided to MCE firmware.

MCE firmware decides on state transition based on these inputs along 
with its background work load.

So, Tegra specific CPU idle driver is required mainly to provide 
cross-over thresholds from DT and run time idle state information to MCE 
firmware through Tegra MCE communication APIs.

Allowing cross-over threshold through DT allows users to vary idle time 
thresholds for state transitions based on different use-cases.


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v1 3/5] dt-bindings: arm: Add cpu-idle-states to Tegra194 CPU nodes
  2021-03-08 18:32     ` Sowjanya Komatineni
@ 2021-03-10 23:19       ` Sowjanya Komatineni
  2021-03-11  2:52       ` Sudeep Holla
  1 sibling, 0 replies; 19+ messages in thread
From: Sowjanya Komatineni @ 2021-03-10 23:19 UTC (permalink / raw)
  To: Sudeep Holla
  Cc: thierry.reding, jonathanh, daniel.lezcano, robh+dt, ksitaraman,
	sanjayc, linux-tegra, linux-kernel, linux-pm, devicetree


On 3/8/21 10:32 AM, Sowjanya Komatineni wrote:
>
> On 3/7/21 8:37 PM, Sudeep Holla wrote:
>> On Wed, Mar 03, 2021 at 10:08:10PM -0800, Sowjanya Komatineni wrote:
>>> This patch adds cpu-idle-states and corresponding state nodes to
>>> Tegra194 CPU in dt-binding document
>>>
>> I see that this platform has PSCI support. Can you care to explain why
>> you need additional DT bindings and driver for PSCI based CPU suspend.
>> Until the reasons are convincing, consider NACK from my side for this
>> driver and DT bindings. You should be really using those bindings and
>> the driver may be with minor changes there.
>>
> MCE firmware is in charge of state transition for Tegra194 carmel CPUs.
>
> For run-time state transitions, need to provide state request along 
> with its residency time to MCE firmware which is running in the 
> background.
>
> State min residency is updated into power_state value along with state 
> id that is passed to psci_cpu_suspend_enter
>
> Also states cross-over idle times need to be provided to MCE firmware.
>
> MCE firmware decides on state transition based on these inputs along 
> with its background work load.
>
> So, Tegra specific CPU idle driver is required mainly to provide 
> cross-over thresholds from DT and run time idle state information to 
> MCE firmware through Tegra MCE communication APIs.
>
> Allowing cross-over threshold through DT allows users to vary idle 
> time thresholds for state transitions based on different use-cases.
>
Hi Sudeep,

Can you please let me know if you have any more concerns for having this 
Tegra specific cpuidle driver?

Thanks

Sowjanya


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v1 3/5] dt-bindings: arm: Add cpu-idle-states to Tegra194 CPU nodes
  2021-03-08 18:32     ` Sowjanya Komatineni
  2021-03-10 23:19       ` Sowjanya Komatineni
@ 2021-03-11  2:52       ` Sudeep Holla
  2021-03-11 21:11         ` Sowjanya Komatineni
  1 sibling, 1 reply; 19+ messages in thread
From: Sudeep Holla @ 2021-03-11  2:52 UTC (permalink / raw)
  To: Sowjanya Komatineni
  Cc: thierry.reding, jonathanh, Sudeep Holla, daniel.lezcano, robh+dt,
	ksitaraman, sanjayc, linux-tegra, linux-kernel, linux-pm,
	devicetree

On Mon, Mar 08, 2021 at 10:32:17AM -0800, Sowjanya Komatineni wrote:
>
> On 3/7/21 8:37 PM, Sudeep Holla wrote:
> > On Wed, Mar 03, 2021 at 10:08:10PM -0800, Sowjanya Komatineni wrote:
> > > This patch adds cpu-idle-states and corresponding state nodes to
> > > Tegra194 CPU in dt-binding document
> > >
> > I see that this platform has PSCI support. Can you care to explain why
> > you need additional DT bindings and driver for PSCI based CPU suspend.
> > Until the reasons are convincing, consider NACK from my side for this
> > driver and DT bindings. You should be really using those bindings and
> > the driver may be with minor changes there.
> >
> MCE firmware is in charge of state transition for Tegra194 carmel CPUs.
>

Sure, but I assume only TF-A talks to MCE and not any OSPM/Linux kernel.

> For run-time state transitions, need to provide state request along with its
> residency time to MCE firmware which is running in the background.
>

Sounds similar to x86 mwait, perhaps we need to extend PSCI if we need
to make this firmware PSCI compliant or just say it is not and implement
completely independent implementation. I am not saying that is acceptable
ATM but I prefer not to mix some implementation to make it look like
PSCI compliant.

> State min residency is updated into power_state value along with state id
> that is passed to psci_cpu_suspend_enter
>

Sounds like a hack/workaround. I would prefer to standardise that. IIUC
the power_state is more static and derived from DT. I don't like to
overload that TBH. Need to check with authors of that binding.

> Also states cross-over idle times need to be provided to MCE firmware.
>

New requirements if this has to be PSCI compliant.

> MCE firmware decides on state transition based on these inputs along with
> its background work load.
>
> So, Tegra specific CPU idle driver is required mainly to provide cross-over
> thresholds from DT and run time idle state information to MCE firmware
> through Tegra MCE communication APIs.
>

I am worried if different vendors will come up with different custom
solution for this. We need to either standardise this is Linux/DT or
in PSCI.

> Allowing cross-over threshold through DT allows users to vary idle time
> thresholds for state transitions based on different use-cases.
>

Sounds like policy and not platform specific to be in DT, but I will leave
that to DT maintainers.

--
Regards,
Sudeep

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v1 3/5] dt-bindings: arm: Add cpu-idle-states to Tegra194 CPU nodes
  2021-03-11  2:52       ` Sudeep Holla
@ 2021-03-11 21:11         ` Sowjanya Komatineni
  2021-03-16  5:38           ` Sudeep Holla
       [not found]           ` <08ac26c1-8257-4c6d-d274-595fee28a00f@nvidia.com>
  0 siblings, 2 replies; 19+ messages in thread
From: Sowjanya Komatineni @ 2021-03-11 21:11 UTC (permalink / raw)
  To: Sudeep Holla
  Cc: thierry.reding, jonathanh, daniel.lezcano, robh+dt, ksitaraman,
	sanjayc, linux-tegra, linux-kernel, linux-pm, devicetree


On 3/10/21 6:52 PM, Sudeep Holla wrote:
> On Mon, Mar 08, 2021 at 10:32:17AM -0800, Sowjanya Komatineni wrote:
>> On 3/7/21 8:37 PM, Sudeep Holla wrote:
>>> On Wed, Mar 03, 2021 at 10:08:10PM -0800, Sowjanya Komatineni wrote:
>>>> This patch adds cpu-idle-states and corresponding state nodes to
>>>> Tegra194 CPU in dt-binding document
>>>>
>>> I see that this platform has PSCI support. Can you care to explain why
>>> you need additional DT bindings and driver for PSCI based CPU suspend.
>>> Until the reasons are convincing, consider NACK from my side for this
>>> driver and DT bindings. You should be really using those bindings and
>>> the driver may be with minor changes there.
>>>
>> MCE firmware is in charge of state transition for Tegra194 carmel CPUs.
>>
> Sure, but I assume only TF-A talks to MCE and not any OSPM/Linux kernel.
No. Tegra194 CPU idle driver works with MCE firmware running in 
background so cpuidle kernel driver also talks to MCE firmware directly 
on state information.
>
>> For run-time state transitions, need to provide state request along with its
>> residency time to MCE firmware which is running in the background.
>>
> Sounds similar to x86 mwait, perhaps we need to extend PSCI if we need
> to make this firmware PSCI compliant or just say it is not and implement
> completely independent implementation. I am not saying that is acceptable
> ATM but I prefer not to mix some implementation to make it look like
> PSCI compliant.
>
>> State min residency is updated into power_state value along with state id
>> that is passed to psci_cpu_suspend_enter
>>
> Sounds like a hack/workaround. I would prefer to standardise that. IIUC
> the power_state is more static and derived from DT. I don't like to
> overload that TBH. Need to check with authors of that binding.

Passing state idle time to ATF along with state to enter is Tegra 
specific as ATF firmware updates idle time to Tegra MCE firmware which 
will be used for deciding on state transition along with other 
information and background load.

Not sure if this need to be standardized but will try to find alternate 
way to update idle time without misusing power-state value.

Will discuss on this internally and get back.

>
>> Also states cross-over idle times need to be provided to MCE firmware.
>>
> New requirements if this has to be PSCI compliant.

Updating cross-over idle times from DT to MCE firmware directly from 
cpuidle kernel driver with corresponding MCE ARI commands is again Tegra 
specific.

>
>> MCE firmware decides on state transition based on these inputs along with
>> its background work load.
>>
>> So, Tegra specific CPU idle driver is required mainly to provide cross-over
>> thresholds from DT and run time idle state information to MCE firmware
>> through Tegra MCE communication APIs.
>>
> I am worried if different vendors will come up with different custom
> solution for this. We need to either standardise this is Linux/DT or
> in PSCI.
>
>> Allowing cross-over threshold through DT allows users to vary idle time
>> thresholds for state transitions based on different use-cases.
>>
> Sounds like policy and not platform specific to be in DT, but I will leave
> that to DT maintainers.

cross-over idle times are based on supported CPU core and cluster states 
and updating these from DT to Tegra MCE firmware running in the 
background is Tegra specific.

>
> --
> Regards,
> Sudeep

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v1 3/5] dt-bindings: arm: Add cpu-idle-states to Tegra194 CPU nodes
       [not found]             ` <4b21f4c7-19cd-fcea-dd1b-9203be60a523@nvidia.com>
@ 2021-03-15 19:26               ` Sowjanya Komatineni
  2021-03-16  7:18               ` Sudeep Holla
  1 sibling, 0 replies; 19+ messages in thread
From: Sowjanya Komatineni @ 2021-03-15 19:26 UTC (permalink / raw)
  To: Sudeep Holla
  Cc: thierry.reding, jonathanh, daniel.lezcano, robh+dt, ksitaraman,
	sanjayc, linux-tegra, linux-kernel, linux-pm, devicetree

Re-sending as it went out as HTML instead of plain text.


On 3/15/21 11:13 AM, Sowjanya Komatineni wrote:
>
> Hi Sudeep,
>
> I see you are one of the maintainer of PSCI driver. Please add any 
> other right persons if you think should also agree/comment.
>
> Can you please comment on below 2 items based on your feedback?
>
> 1. Can you please suggest on proper way of generalizing to pass state 
> residency time run-time along with state during state enter?
>
> Not sure if any other drivers need this but for Tegra as MCE firmware 
> is in-charge of states enter and decisions, passing run-time state 
> residency from kernel to ATF is required and agree on not using 
> power_state value for this which is against PSCI spec.
>
> 2. Regarding state thresholds, although state thresholds are policy 
> related in Tegra cpu idle perspective these thresholds are platform 
> specific based on use case and mainly for MCE firmware usage to decide 
> on state transitions for core and core clusters as well.
>
> As these are Tegra platform specific, Please comment if any other 
> concerns in having this configured by Tegra CPU Idle kernel driver.
>
> Based on my understanding only above issue-1 is PSCI compliant 
> related. Please confirm.
>
> Thanks
>
> Sowjanya
>
>
> On 3/12/21 2:27 PM, Sowjanya Komatineni wrote:
>>
>> Hi Sudeep,
>>
>> To make our driver PSCI compliant below are few updates to be done
>>
>> 1. Standardize passing residency time run-time thru PSCI to ATF.
>>
>>     From PSCI specification I only see PSCI_STAT_RESIDENCY and 
>> PSCI_STAT_COUNT optional functions for PSCI1.0/PSCI1.1
>>
>>    Can you please help add right people to explore on possible ways 
>> to add PSCI function for passing corresponding state residency time 
>> to ATF?
>>
>> 2. Tegra CPU Idle support definitely need to pass deepest cluster 
>> state and threshold to MCE firmware from DT and looks like we can 
>> move this implementation to ATF
>>
>>      With both of the above implementation changes Tegra194 driver 
>> will be PSCI compliant.
>>
>> Thanks
>>
>> Sowjanya
>>
>>
>> On 3/11/21 1:11 PM, Sowjanya Komatineni wrote:
>>>
>>> On 3/10/21 6:52 PM, Sudeep Holla wrote:
>>>> On Mon, Mar 08, 2021 at 10:32:17AM -0800, Sowjanya Komatineni wrote:
>>>>> On 3/7/21 8:37 PM, Sudeep Holla wrote:
>>>>>> On Wed, Mar 03, 2021 at 10:08:10PM -0800, Sowjanya Komatineni wrote:
>>>>>>> This patch adds cpu-idle-states and corresponding state nodes to
>>>>>>> Tegra194 CPU in dt-binding document
>>>>>>>
>>>>>> I see that this platform has PSCI support. Can you care to 
>>>>>> explain why
>>>>>> you need additional DT bindings and driver for PSCI based CPU 
>>>>>> suspend.
>>>>>> Until the reasons are convincing, consider NACK from my side for 
>>>>>> this
>>>>>> driver and DT bindings. You should be really using those bindings 
>>>>>> and
>>>>>> the driver may be with minor changes there.
>>>>>>
>>>>> MCE firmware is in charge of state transition for Tegra194 carmel 
>>>>> CPUs.
>>>>>
>>>> Sure, but I assume only TF-A talks to MCE and not any OSPM/Linux 
>>>> kernel.
>>> No. Tegra194 CPU idle driver works with MCE firmware running in 
>>> background so cpuidle kernel driver also talks to MCE firmware 
>>> directly on state information.
>>>>
>>>>> For run-time state transitions, need to provide state request 
>>>>> along with its
>>>>> residency time to MCE firmware which is running in the background.
>>>>>
>>>> Sounds similar to x86 mwait, perhaps we need to extend PSCI if we need
>>>> to make this firmware PSCI compliant or just say it is not and 
>>>> implement
>>>> completely independent implementation. I am not saying that is 
>>>> acceptable
>>>> ATM but I prefer not to mix some implementation to make it look like
>>>> PSCI compliant.
>>>>
>>>>> State min residency is updated into power_state value along with 
>>>>> state id
>>>>> that is passed to psci_cpu_suspend_enter
>>>>>
>>>> Sounds like a hack/workaround. I would prefer to standardise that. 
>>>> IIUC
>>>> the power_state is more static and derived from DT. I don't like to
>>>> overload that TBH. Need to check with authors of that binding.
>>>
>>> Passing state idle time to ATF along with state to enter is Tegra 
>>> specific as ATF firmware updates idle time to Tegra MCE firmware 
>>> which will be used for deciding on state transition along with other 
>>> information and background load.
>>>
>>> Not sure if this need to be standardized but will try to find 
>>> alternate way to update idle time without misusing power-state value.
>>>
>>> Will discuss on this internally and get back.
>>>
>>>>
>>>>> Also states cross-over idle times need to be provided to MCE 
>>>>> firmware.
>>>>>
>>>> New requirements if this has to be PSCI compliant.
>>>
>>> Updating cross-over idle times from DT to MCE firmware directly from 
>>> cpuidle kernel driver with corresponding MCE ARI commands is again 
>>> Tegra specific.
>>>
>>>>
>>>>> MCE firmware decides on state transition based on these inputs 
>>>>> along with
>>>>> its background work load.
>>>>>
>>>>> So, Tegra specific CPU idle driver is required mainly to provide 
>>>>> cross-over
>>>>> thresholds from DT and run time idle state information to MCE 
>>>>> firmware
>>>>> through Tegra MCE communication APIs.
>>>>>
>>>> I am worried if different vendors will come up with different custom
>>>> solution for this. We need to either standardise this is Linux/DT or
>>>> in PSCI.
>>>>
>>>>> Allowing cross-over threshold through DT allows users to vary idle 
>>>>> time
>>>>> thresholds for state transitions based on different use-cases.
>>>>>
>>>> Sounds like policy and not platform specific to be in DT, but I 
>>>> will leave
>>>> that to DT maintainers.
>>>
>>> cross-over idle times are based on supported CPU core and cluster 
>>> states and updating these from DT to Tegra MCE firmware running in 
>>> the background is Tegra specific.
>>>
>>>>
>>>> -- 
>>>> Regards,
>>>> Sudeep

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v1 3/5] dt-bindings: arm: Add cpu-idle-states to Tegra194 CPU nodes
  2021-03-11 21:11         ` Sowjanya Komatineni
@ 2021-03-16  5:38           ` Sudeep Holla
       [not found]           ` <08ac26c1-8257-4c6d-d274-595fee28a00f@nvidia.com>
  1 sibling, 0 replies; 19+ messages in thread
From: Sudeep Holla @ 2021-03-16  5:38 UTC (permalink / raw)
  To: Sowjanya Komatineni
  Cc: thierry.reding, jonathanh, daniel.lezcano, robh+dt,
	Lorenzo Pieralisi, Sudeep Holla, ksitaraman, sanjayc,
	linux-tegra, linux-kernel, linux-pm, devicetree

+Lorenzo

Hi Sowjanya,

Sorry for the delayed response. I am still in vacation 😉

On Thu, Mar 11, 2021 at 01:11:37PM -0800, Sowjanya Komatineni wrote:
>
> On 3/10/21 6:52 PM, Sudeep Holla wrote:
> > On Mon, Mar 08, 2021 at 10:32:17AM -0800, Sowjanya Komatineni wrote:
> > > On 3/7/21 8:37 PM, Sudeep Holla wrote:
> > > > On Wed, Mar 03, 2021 at 10:08:10PM -0800, Sowjanya Komatineni wrote:
> > > > > This patch adds cpu-idle-states and corresponding state nodes to
> > > > > Tegra194 CPU in dt-binding document
> > > > >
> > > > I see that this platform has PSCI support. Can you care to explain why
> > > > you need additional DT bindings and driver for PSCI based CPU suspend.
> > > > Until the reasons are convincing, consider NACK from my side for this
> > > > driver and DT bindings. You should be really using those bindings and
> > > > the driver may be with minor changes there.
> > > >
> > > MCE firmware is in charge of state transition for Tegra194 carmel CPUs.
> > >
> > Sure, but I assume only TF-A talks to MCE and not any OSPM/Linux kernel.
>
> No. Tegra194 CPU idle driver works with MCE firmware running in background
> so cpuidle kernel driver also talks to MCE firmware directly on state
> information.

If that is the case I wouldn't term this as PSCI compliant firmware and
wouldn't attempt to use PSCI CPU idle driver. Now if we would what to allow
non-PSCI idle driver for Arm64 is entirely different question that deserves
a separate discussion IMO.

> >
> > > For run-time state transitions, need to provide state request along with its
> > > residency time to MCE firmware which is running in the background.
> > >
> > Sounds similar to x86 mwait, perhaps we need to extend PSCI if we need
> > to make this firmware PSCI compliant or just say it is not and implement
> > completely independent implementation. I am not saying that is acceptable
> > ATM but I prefer not to mix some implementation to make it look like
> > PSCI compliant.
> >
> > > State min residency is updated into power_state value along with state id
> > > that is passed to psci_cpu_suspend_enter
> > >
> > Sounds like a hack/workaround. I would prefer to standardise that. IIUC
> > the power_state is more static and derived from DT. I don't like to
> > overload that TBH. Need to check with authors of that binding.
>
> Passing state idle time to ATF along with state to enter is Tegra specific
> as ATF firmware updates idle time to Tegra MCE firmware which will be used
> for deciding on state transition along with other information and background
> load.
>

So far we don't have any platform specific PSCI in OSPM and I prefer to keep
it that way.

> Not sure if this need to be standardized but will try to find alternate way
> to update idle time without misusing power-state value.
>

Sure, we can always review and see if any alternatives are acceptable, but
I am bit nervous to tie this as PSCI if it is not strictly spec compliant.

> Will discuss on this internally and get back.
>

Thanks.

> >
> > > Also states cross-over idle times need to be provided to MCE firmware.
> > >
> > New requirements if this has to be PSCI compliant.
>
> Updating cross-over idle times from DT to MCE firmware directly from cpuidle
> kernel driver with corresponding MCE ARI commands is again Tegra specific.
>

So all there are platform specific but static information you need from DT ?
If so, what can't it be made part of TF-A and OSPM can avoid interfering
with that info completely. My understanding was that OSPM provides runtime
hints like x86 mwait. If that's not the case, I am failing to understand
the need for OSPM to pass such static information from DT to the firmware.
Why can't that be just part of the firmware to begin with ?

> >
> > > MCE firmware decides on state transition based on these inputs along with
> > > its background work load.
> > >

What do you mean by this *"background work load"* ?

--
Regards,
Sudeep

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v1 3/5] dt-bindings: arm: Add cpu-idle-states to Tegra194 CPU nodes
       [not found]           ` <08ac26c1-8257-4c6d-d274-595fee28a00f@nvidia.com>
@ 2021-03-16  6:57             ` Sudeep Holla
       [not found]             ` <4b21f4c7-19cd-fcea-dd1b-9203be60a523@nvidia.com>
  1 sibling, 0 replies; 19+ messages in thread
From: Sudeep Holla @ 2021-03-16  6:57 UTC (permalink / raw)
  To: Sowjanya Komatineni
  Cc: thierry.reding, jonathanh, daniel.lezcano, robh+dt,
	Lorenzo Pieralisi, Sudeep Holla, ksitaraman, sanjayc,
	linux-tegra, linux-kernel, linux-pm, devicetree

On Fri, Mar 12, 2021 at 02:27:30PM -0800, Sowjanya Komatineni wrote:
> Hi Sudeep,
>
> To make our driver PSCI compliant below are few updates to be done
>
> 1. Standardize passing residency time run-time thru PSCI to ATF.
>

Yes that was my initial understanding, but your previous response was
confusing. I should have read this first.

>     From PSCI specification I only see PSCI_STAT_RESIDENCY and
> PSCI_STAT_COUNT optional functions for PSCI1.0/PSCI1.1
>

Indeed, we don't have any support to pass run-time residency hints in PSCI
today.

>    Can you please help add right people to explore on possible ways to add
> PSCI function for passing corresponding state residency time to ATF?
>

Before we jump to implementation in TF-A we need to get agreement to get this
added to the specification to support in OSPM/Linux. TF-A is just one
implementation of PSCI and may not be only one.

Formally you can raise any specification related queries to
https://developer.arm.com/support or support@arm.com. I will ask the author
of PSCI specification internally to take a look at this thread and chime in
if they can.

> 2. Tegra CPU Idle support definitely need to pass deepest cluster state and
> threshold to MCE firmware from DT and looks like we can move this
> implementation to ATF
>

Yes, I just asked the same question as response to your earlier email. Thanks
for confirming that it can be moved out of OSPM/Linux and DT

>      With both of the above implementation changes Tegra194 driver will be
> PSCI compliant.
>

We still need to get agreement on the specification front 😉.

--
Regards,
Sudeep

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v1 3/5] dt-bindings: arm: Add cpu-idle-states to Tegra194 CPU nodes
       [not found]             ` <4b21f4c7-19cd-fcea-dd1b-9203be60a523@nvidia.com>
  2021-03-15 19:26               ` Sowjanya Komatineni
@ 2021-03-16  7:18               ` Sudeep Holla
  2021-03-16 11:24                 ` Sowjanya Komatineni
  1 sibling, 1 reply; 19+ messages in thread
From: Sudeep Holla @ 2021-03-16  7:18 UTC (permalink / raw)
  To: Sowjanya Komatineni
  Cc: thierry.reding, jonathanh, daniel.lezcano, robh+dt, ksitaraman,
	sanjayc, Lorenzo Pieralisi, Sudeep Holla, linux-tegra,
	linux-kernel, linux-pm, devicetree

On Mon, Mar 15, 2021 at 11:13:24AM -0700, Sowjanya Komatineni wrote:
> Hi Sudeep,
>
> I see you are one of the maintainer of PSCI driver. Please add any other
> right persons if you think should also agree/comment.
>
> Can you please comment on below 2 items based on your feedback?
>
> 1. Can you please suggest on proper way of generalizing to pass state
> residency time run-time along with state during state enter?
>
> Not sure if any other drivers need this but for Tegra as MCE firmware is
> in-charge of states enter and decisions, passing run-time state residency
> from kernel to ATF is required and agree on not using power_state value for
> this which is against PSCI spec.
>

Yes, I prefer you need to get this added in the PSCI specification.
I have passed this thread to the author of the specification.

> 2. Regarding state thresholds, although state thresholds are policy related
> in Tegra cpu idle perspective these thresholds are platform specific based
> on use case and mainly for MCE firmware usage to decide on state transitions
> for core and core clusters as well.
>
From previous emails, I gather these can be moved to firmware and need not be
there in DT ?

> As these are Tegra platform specific, Please comment if any other concerns
> in having this configured by Tegra CPU Idle kernel driver.
>

I prefer not to have Tegra specific idle driver if we can get the necessary
changes in PSCI spec. We must then have just one PSCI idle driver in the
kernel.


> Based on my understanding only above issue-1 is PSCI compliant related.
> Please confirm.
>

Correct.

--
Regards,
Sudeep

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v1 3/5] dt-bindings: arm: Add cpu-idle-states to Tegra194 CPU nodes
  2021-03-16  7:18               ` Sudeep Holla
@ 2021-03-16 11:24                 ` Sowjanya Komatineni
  0 siblings, 0 replies; 19+ messages in thread
From: Sowjanya Komatineni @ 2021-03-16 11:24 UTC (permalink / raw)
  To: Sudeep Holla
  Cc: thierry.reding, jonathanh, daniel.lezcano, robh+dt, ksitaraman,
	sanjayc, Lorenzo Pieralisi, linux-tegra, linux-kernel, linux-pm,
	devicetree


On 3/16/21 12:18 AM, Sudeep Holla wrote:
> On Mon, Mar 15, 2021 at 11:13:24AM -0700, Sowjanya Komatineni wrote:
>> Hi Sudeep,
>>
>> I see you are one of the maintainer of PSCI driver. Please add any other
>> right persons if you think should also agree/comment.
>>
>> Can you please comment on below 2 items based on your feedback?
>>
>> 1. Can you please suggest on proper way of generalizing to pass state
>> residency time run-time along with state during state enter?
>>
>> Not sure if any other drivers need this but for Tegra as MCE firmware is
>> in-charge of states enter and decisions, passing run-time state residency
>> from kernel to ATF is required and agree on not using power_state value for
>> this which is against PSCI spec.
>>
> Yes, I prefer you need to get this added in the PSCI specification.
> I have passed this thread to the author of the specification.
Thanks Sudeep.
>
>> 2. Regarding state thresholds, although state thresholds are policy related
>> in Tegra cpu idle perspective these thresholds are platform specific based
>> on use case and mainly for MCE firmware usage to decide on state transitions
>> for core and core clusters as well.
>>
>  From previous emails, I gather these can be moved to firmware and need not be
> there in DT ?

Yes we can move state thresholds programming from kernel driver to ATF 
but we still need to use DT properties for this to allow users to tweak 
for their use-cases.

With DT access in ATF this can be done. But checking internally on 
having DTB access in ATF as currently we don't support that.

>
>> As these are Tegra platform specific, Please comment if any other concerns
>> in having this configured by Tegra CPU Idle kernel driver.
>>
> I prefer not to have Tegra specific idle driver if we can get the necessary
> changes in PSCI spec. We must then have just one PSCI idle driver in the
> kernel.

Agree by adding state residency run-time propagation along with power 
state to PSCI specification and moving state thresholds update to MCE 
from kernel driver to AT looks like we can use same PSCI cpu idle driver 
although we will be using state thresholds additional DT properties 
under cpu nodes which will be used by ATF firmware once we decide on no 
concerns to allow DTB access in ATF.


>> Based on my understanding only above issue-1 is PSCI compliant related.
>> Please confirm.
>>
> Correct.
>
> --
> Regards,
> Sudeep

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2021-03-16 11:25 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-04  6:08 [PATCH v1 0/5] Add cpuidle support for Tegra194 Sowjanya Komatineni
2021-03-04  6:08 ` [PATCH v1 1/5] MAINTAINERS: Add Tegra CPUIDLE driver section Sowjanya Komatineni
2021-03-04  8:01   ` Daniel Lezcano
2021-03-04  6:08 ` [PATCH v1 2/5] firmware: tegra: Add Tegra194 MCE ARI driver Sowjanya Komatineni
2021-03-04  6:08 ` [PATCH v1 3/5] dt-bindings: arm: Add cpu-idle-states to Tegra194 CPU nodes Sowjanya Komatineni
2021-03-04 20:47   ` Rob Herring
2021-03-08  4:37   ` Sudeep Holla
2021-03-08 18:32     ` Sowjanya Komatineni
2021-03-10 23:19       ` Sowjanya Komatineni
2021-03-11  2:52       ` Sudeep Holla
2021-03-11 21:11         ` Sowjanya Komatineni
2021-03-16  5:38           ` Sudeep Holla
     [not found]           ` <08ac26c1-8257-4c6d-d274-595fee28a00f@nvidia.com>
2021-03-16  6:57             ` Sudeep Holla
     [not found]             ` <4b21f4c7-19cd-fcea-dd1b-9203be60a523@nvidia.com>
2021-03-15 19:26               ` Sowjanya Komatineni
2021-03-16  7:18               ` Sudeep Holla
2021-03-16 11:24                 ` Sowjanya Komatineni
2021-03-04  6:08 ` [PATCH v1 4/5] cpuidle: Add Tegra194 cpuidle driver Sowjanya Komatineni
2021-03-05 13:50   ` Dmitry Osipenko
2021-03-04  6:08 ` [PATCH v1 5/5] arm64: dts: tegra194: Add CPU idle states Sowjanya Komatineni

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).