linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [Patch v3 0/6] Introduce LMh driver for Qualcomm SoCs
@ 2021-07-08 12:06 Thara Gopinath
  2021-07-08 12:06 ` [Patch v3 1/6] firmware: qcom_scm: Introduce SCM calls to access LMh Thara Gopinath
                   ` (6 more replies)
  0 siblings, 7 replies; 29+ messages in thread
From: Thara Gopinath @ 2021-07-08 12:06 UTC (permalink / raw)
  To: agross, bjorn.andersson, rui.zhang, daniel.lezcano, viresh.kumar,
	rjw, robh+dt
  Cc: tdas, mka, linux-arm-msm, linux-pm, linux-kernel, devicetree

Limits Management Hardware(LMh) is a hardware infrastructure on some
Qualcomm SoCs that can enforce temperature and current limits as programmed
by software for certain IPs like CPU. On many newer SoCs LMh is configured
by firmware/TZ and no programming is needed from the kernel side. But on
certain SoCs like sdm845 the firmware does not do a complete programming of
the h/w block. On such SoCs kernel software has to explicitly set up the
temperature limits and turn on various monitoring and enforcing algorithms
on the hardware.

Introduce support for enabling and programming various limit settings and
monitoring capabilities of Limits Management Hardware(LMh) associated with
cpu clusters. Also introduce support in cpufreq hardware driver to monitor
the interrupt associated with cpu frequency throttling so that this
information can be conveyed to the schdeuler via thermal pressure
interface.

With this patch series following cpu performance improvement(30-70%) is
observed on sdm845. The reasoning here is that without LMh being programmed
properly from the kernel, the default settings were enabling thermal
mitigation for CPUs at too low a temperature (around 70-75 degree C).  This
in turn meant that many a time CPUs were never actually allowed to hit the
maximum possible/required frequencies.

UnixBench whets and dhry (./Run whets dhry)
System Benchmarks Index Score

                Without LMh Support             With LMh Support
1 copy test     1353.7                          1773.2

8 copy tests    4473.6                          7402.3

Sysbench cpu
sysbench cpu --threads=8 --time=60 --cpu-max-prime=100000 run

                Without LMh Support             With LMh Support
Events per
second                  355                             614

Avg Latency(ms)         21.84                           13.02

v2->v3:
	- Included patch adding dt binding documentation for LMh nodes.
	- Rebased to v5.13

Thara Gopinath (6):
  firmware: qcom_scm: Introduce SCM calls to access LMh
  thermal: qcom: Add support for LMh driver
  cpufreq: qcom-cpufreq-hw: Add dcvs interrupt support
  arm64: boot: dts: qcom: sdm45: Add support for LMh node
  arm64: boot: dts: qcom: sdm845: Remove cpufreq cooling devices for CPU
    thermal zones
  dt-bindings: thermal: Add dt binding for QCOM LMh

 .../devicetree/bindings/thermal/qcom-lmh.yaml | 100 ++++++++
 arch/arm64/boot/dts/qcom/sdm845.dtsi          | 162 ++----------
 drivers/cpufreq/qcom-cpufreq-hw.c             | 118 +++++++++
 drivers/firmware/qcom_scm.c                   |  58 +++++
 drivers/firmware/qcom_scm.h                   |   4 +
 drivers/thermal/qcom/Kconfig                  |  10 +
 drivers/thermal/qcom/Makefile                 |   1 +
 drivers/thermal/qcom/lmh.c                    | 239 ++++++++++++++++++
 include/linux/qcom_scm.h                      |  14 +
 9 files changed, 570 insertions(+), 136 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/thermal/qcom-lmh.yaml
 create mode 100644 drivers/thermal/qcom/lmh.c

-- 
2.25.1


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

* [Patch v3 1/6] firmware: qcom_scm: Introduce SCM calls to access LMh
  2021-07-08 12:06 [Patch v3 0/6] Introduce LMh driver for Qualcomm SoCs Thara Gopinath
@ 2021-07-08 12:06 ` Thara Gopinath
  2021-07-10  4:02   ` Bjorn Andersson
  2021-07-08 12:06 ` [Patch v3 2/6] thermal: qcom: Add support for LMh driver Thara Gopinath
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 29+ messages in thread
From: Thara Gopinath @ 2021-07-08 12:06 UTC (permalink / raw)
  To: agross, bjorn.andersson, rui.zhang, daniel.lezcano, viresh.kumar,
	rjw, robh+dt
  Cc: tdas, mka, linux-arm-msm, linux-pm, linux-kernel, devicetree

Introduce SCM calls to access/configure limits management hardware(LMH).

Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
---

v2->v3:
	Added freeing of payload_buf after the scm call in qcom_scm_lmh_dcvsh as per
	Matthias review comments.

v1->v2:
	Changed the input parameters in qcom_scm_lmh_dcvsh from payload_buf and
	payload_size to payload_fn, payload_reg, payload_val as per Bjorn's review
	comments.

 drivers/firmware/qcom_scm.c | 58 +++++++++++++++++++++++++++++++++++++
 drivers/firmware/qcom_scm.h |  4 +++
 include/linux/qcom_scm.h    | 14 +++++++++
 3 files changed, 76 insertions(+)

diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index ee9cb545e73b..a8d236603e90 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -1147,6 +1147,64 @@ int qcom_scm_qsmmu500_wait_safe_toggle(bool en)
 }
 EXPORT_SYMBOL(qcom_scm_qsmmu500_wait_safe_toggle);
 
+bool qcom_scm_lmh_dcvsh_available(void)
+{
+	return __qcom_scm_is_call_available(__scm->dev, QCOM_SCM_SVC_LMH, QCOM_SCM_LMH_LIMIT_DCVSH);
+}
+EXPORT_SYMBOL(qcom_scm_lmh_dcvsh_available);
+
+int qcom_scm_lmh_profile_change(u32 profile_id)
+{
+	struct qcom_scm_desc desc = {
+		.svc = QCOM_SCM_SVC_LMH,
+		.cmd = QCOM_SCM_LMH_LIMIT_PROFILE_CHANGE,
+		.arginfo = QCOM_SCM_ARGS(1, QCOM_SCM_VAL),
+		.args[0] = profile_id,
+		.owner = ARM_SMCCC_OWNER_SIP,
+	};
+
+	return qcom_scm_call(__scm->dev, &desc, NULL);
+}
+EXPORT_SYMBOL(qcom_scm_lmh_profile_change);
+
+int qcom_scm_lmh_dcvsh(u32 payload_fn, u32 payload_reg, u32 payload_val,
+		       u64 limit_node, u32 node_id, u64 version)
+{
+	dma_addr_t payload_phys;
+	u32 *payload_buf;
+	int ret, payload_size = 5 * sizeof(u32);
+
+	struct qcom_scm_desc desc = {
+		.svc = QCOM_SCM_SVC_LMH,
+		.cmd = QCOM_SCM_LMH_LIMIT_DCVSH,
+		.arginfo = QCOM_SCM_ARGS(5, QCOM_SCM_RO, QCOM_SCM_VAL, QCOM_SCM_VAL,
+					QCOM_SCM_VAL, QCOM_SCM_VAL),
+		.args[1] = payload_size,
+		.args[2] = limit_node,
+		.args[3] = node_id,
+		.args[4] = version,
+		.owner = ARM_SMCCC_OWNER_SIP,
+	};
+
+	payload_buf = dma_alloc_coherent(__scm->dev, payload_size, &payload_phys, GFP_KERNEL);
+	if (!payload_buf)
+		return -ENOMEM;
+
+	payload_buf[0] = payload_fn;
+	payload_buf[1] = 0;
+	payload_buf[2] = payload_reg;
+	payload_buf[3] = 1;
+	payload_buf[4] = payload_val;
+
+	desc.args[0] = payload_phys;
+
+	ret = qcom_scm_call(__scm->dev, &desc, NULL);
+
+	dma_free_coherent(__scm->dev, payload_size, payload_buf, payload_phys);
+	return ret;
+}
+EXPORT_SYMBOL(qcom_scm_lmh_dcvsh);
+
 static int qcom_scm_find_dload_address(struct device *dev, u64 *addr)
 {
 	struct device_node *tcsr;
diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
index 632fe3142462..d92156ceb3ac 100644
--- a/drivers/firmware/qcom_scm.h
+++ b/drivers/firmware/qcom_scm.h
@@ -114,6 +114,10 @@ extern int scm_legacy_call(struct device *dev, const struct qcom_scm_desc *desc,
 #define QCOM_SCM_SVC_HDCP		0x11
 #define QCOM_SCM_HDCP_INVOKE		0x01
 
+#define QCOM_SCM_SVC_LMH			0x13
+#define QCOM_SCM_LMH_LIMIT_PROFILE_CHANGE	0x01
+#define QCOM_SCM_LMH_LIMIT_DCVSH		0x10
+
 #define QCOM_SCM_SVC_SMMU_PROGRAM		0x15
 #define QCOM_SCM_SMMU_CONFIG_ERRATA1		0x03
 #define QCOM_SCM_SMMU_CONFIG_ERRATA1_CLIENT_ALL	0x02
diff --git a/include/linux/qcom_scm.h b/include/linux/qcom_scm.h
index 0165824c5128..c0475d1c9885 100644
--- a/include/linux/qcom_scm.h
+++ b/include/linux/qcom_scm.h
@@ -109,6 +109,12 @@ extern int qcom_scm_hdcp_req(struct qcom_scm_hdcp_req *req, u32 req_cnt,
 			     u32 *resp);
 
 extern int qcom_scm_qsmmu500_wait_safe_toggle(bool en);
+
+extern int qcom_scm_lmh_dcvsh(u32 payload_fn, u32 payload_reg, u32 payload_val,
+			      u64 limit_node, u32 node_id, u64 version);
+extern int qcom_scm_lmh_profile_change(u32 profile_id);
+extern bool qcom_scm_lmh_dcvsh_available(void);
+
 #else
 
 #include <linux/errno.h>
@@ -170,5 +176,13 @@ static inline int qcom_scm_hdcp_req(struct qcom_scm_hdcp_req *req, u32 req_cnt,
 
 static inline int qcom_scm_qsmmu500_wait_safe_toggle(bool en)
 		{ return -ENODEV; }
+
+static inline int qcom_scm_lmh_dcvsh(u32 payload_fn, u32 payload_reg, u32 payload_val,
+				     u64 limit_node, u32 node_id, u64 version)
+		{ return -ENODEV; }
+
+static inline int qcom_scm_lmh_profile_change(u32 profile_id) { return -ENODEV; }
+
+static inline bool qcom_scm_lmh_dcvsh_available(void) { return -ENODEV; }
 #endif
 #endif
-- 
2.25.1


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

* [Patch v3 2/6] thermal: qcom: Add support for LMh driver
  2021-07-08 12:06 [Patch v3 0/6] Introduce LMh driver for Qualcomm SoCs Thara Gopinath
  2021-07-08 12:06 ` [Patch v3 1/6] firmware: qcom_scm: Introduce SCM calls to access LMh Thara Gopinath
@ 2021-07-08 12:06 ` Thara Gopinath
  2021-07-10  4:15   ` Bjorn Andersson
  2021-07-08 12:06 ` [Patch v3 3/6] cpufreq: qcom-cpufreq-hw: Add dcvs interrupt support Thara Gopinath
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 29+ messages in thread
From: Thara Gopinath @ 2021-07-08 12:06 UTC (permalink / raw)
  To: agross, bjorn.andersson, rui.zhang, daniel.lezcano, viresh.kumar,
	rjw, robh+dt
  Cc: tdas, mka, linux-arm-msm, linux-pm, linux-kernel, devicetree

Driver enabling various pieces of Limits Management Hardware(LMh) for cpu
cluster0 and cpu cluster1 namely kick starting monitoring of temperature,
current, battery current violations, enabling reliability algorithm and
setting up various temperature limits.

The following has been explained in the cover letter. I am including this
here so that this remains in the commit message as well.

LMh is a hardware infrastructure on some Qualcomm SoCs that can enforce
temperature and current limits as programmed by software for certain IPs
like CPU. On many newer LMh is configured by firmware/TZ and no programming
is needed from the kernel side. But on certain SoCs like sdm845 the
firmware does not do a complete programming of the h/w. On such soc's
kernel software has to explicitly set up the temperature limits and turn on
various monitoring and enforcing algorithms on the hardware.

Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
---

v2->v3:
	- Rearranged enabling of various LMh subfunction and removed returning
	  on error in enabling any one subfunction as the different pieces can
	  operate and thus be enabled independently.
	- Other minor cosmetic fixes.

v1->v2:
	- Cosmetic and spelling fixes from review comments from Randy Dunlap
	- Added irq_disable to lmh_irq_ops and removed disabling of irq from
	  lmh_handle_irq. Now cpufreq explicitly disables irq prior to
	  handling it as per Bjorn's suggestion.
	- Rebased to new version of qcom_scm_lmh_dcvsh as changed in patch 1.
	- Removed generic dt compatible string and introduced platform specific one
	  as per Bjorn's suggestion.
	- Take arm, low and high temp thresholds for LMh from dt properties instead of
	  #defines in the driver as per Daniel's suggestion.
	- Other minor fixes.
 drivers/thermal/qcom/Kconfig  |  10 ++
 drivers/thermal/qcom/Makefile |   1 +
 drivers/thermal/qcom/lmh.c    | 239 ++++++++++++++++++++++++++++++++++
 3 files changed, 250 insertions(+)
 create mode 100644 drivers/thermal/qcom/lmh.c

diff --git a/drivers/thermal/qcom/Kconfig b/drivers/thermal/qcom/Kconfig
index 8d5ac2df26dc..7d942f71e532 100644
--- a/drivers/thermal/qcom/Kconfig
+++ b/drivers/thermal/qcom/Kconfig
@@ -31,3 +31,13 @@ config QCOM_SPMI_TEMP_ALARM
 	  trip points. The temperature reported by the thermal sensor reflects the
 	  real time die temperature if an ADC is present or an estimate of the
 	  temperature based upon the over temperature stage value.
+
+config QCOM_LMH
+	tristate "Qualcomm Limits Management Hardware"
+	depends on ARCH_QCOM
+	help
+	  This enables initialization of Qualcomm limits management
+	  hardware(LMh). LMh allows for hardware-enforced mitigation for cpus based on
+	  input from temperature and current sensors.  On many newer Qualcomm SoCs
+	  LMh is configured in the firmware and this feature need not be enabled.
+	  However, on certain SoCs like sdm845 LMh has to be configured from kernel.
diff --git a/drivers/thermal/qcom/Makefile b/drivers/thermal/qcom/Makefile
index 252ea7d9da0b..0fa2512042e7 100644
--- a/drivers/thermal/qcom/Makefile
+++ b/drivers/thermal/qcom/Makefile
@@ -5,3 +5,4 @@ qcom_tsens-y			+= tsens.o tsens-v2.o tsens-v1.o tsens-v0_1.o \
 				   tsens-8960.o
 obj-$(CONFIG_QCOM_SPMI_ADC_TM5)	+= qcom-spmi-adc-tm5.o
 obj-$(CONFIG_QCOM_SPMI_TEMP_ALARM)	+= qcom-spmi-temp-alarm.o
+obj-$(CONFIG_QCOM_LMH)		+= lmh.o
diff --git a/drivers/thermal/qcom/lmh.c b/drivers/thermal/qcom/lmh.c
new file mode 100644
index 000000000000..a7b1eb308642
--- /dev/null
+++ b/drivers/thermal/qcom/lmh.c
@@ -0,0 +1,239 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+/*
+ * Copyright (C) 2021, Linaro Limited. All rights reserved.
+ */
+#include <linux/module.h>
+#include <linux/interrupt.h>
+#include <linux/irqdomain.h>
+#include <linux/err.h>
+#include <linux/platform_device.h>
+#include <linux/of_platform.h>
+#include <linux/slab.h>
+#include <linux/qcom_scm.h>
+
+#define LMH_NODE_DCVS			0x44435653
+#define LMH_CLUSTER0_NODE_ID		0x6370302D
+#define LMH_CLUSTER1_NODE_ID		0x6370312D
+
+#define LMH_SUB_FN_THERMAL		0x54484D4C
+#define LMH_SUB_FN_CRNT			0x43524E54
+#define LMH_SUB_FN_REL			0x52454C00
+#define LMH_SUB_FN_BCL			0x42434C00
+
+#define LMH_ALGO_MODE_ENABLE		0x454E424C
+#define LMH_TH_HI_THRESHOLD		0x48494748
+#define LMH_TH_LOW_THRESHOLD		0x4C4F5700
+#define LMH_TH_ARM_THRESHOLD		0x41524D00
+
+#define LMH_REG_DCVS_INTR_CLR		0x8
+
+struct lmh_hw_data {
+	void __iomem *base;
+	struct irq_domain *domain;
+	int irq;
+	u32 cpu_id;
+};
+
+static irqreturn_t lmh_handle_irq(int hw_irq, void *data)
+{
+	struct lmh_hw_data *lmh_data = data;
+	int irq = irq_find_mapping(lmh_data->domain, 0);
+
+	/* Call the cpufreq driver to handle the interrupt */
+	if (irq)
+		generic_handle_irq(irq);
+
+	return 0;
+}
+
+static void lmh_enable_interrupt(struct irq_data *d)
+{
+	struct lmh_hw_data *lmh_data = irq_data_get_irq_chip_data(d);
+
+	/* Clear the existing interrupt */
+	writel(0xff, lmh_data->base + LMH_REG_DCVS_INTR_CLR);
+	enable_irq(lmh_data->irq);
+}
+
+static void lmh_disable_interrupt(struct irq_data *d)
+{
+	struct lmh_hw_data *lmh_data = irq_data_get_irq_chip_data(d);
+
+	disable_irq_nosync(lmh_data->irq);
+}
+
+static struct irq_chip lmh_irq_chip = {
+	.name           = "lmh",
+	.irq_enable	= lmh_enable_interrupt,
+	.irq_disable	= lmh_disable_interrupt
+};
+
+static int lmh_irq_map(struct irq_domain *d, unsigned int irq, irq_hw_number_t hw)
+{
+	struct lmh_hw_data *lmh_data = d->host_data;
+
+	irq_set_chip_and_handler(irq, &lmh_irq_chip, handle_simple_irq);
+	irq_set_chip_data(irq, lmh_data);
+
+	return 0;
+}
+
+static const struct irq_domain_ops lmh_irq_ops = {
+	.map = lmh_irq_map,
+	.xlate = irq_domain_xlate_onecell,
+};
+
+static int lmh_probe(struct platform_device *pdev)
+{
+	struct device *dev;
+	struct device_node *np;
+	struct lmh_hw_data *lmh_data;
+	u32 node_id;
+	int temp_low, temp_high, temp_arm, ret;
+
+	dev = &pdev->dev;
+	np = dev->of_node;
+	if (!np)
+		return -EINVAL;
+
+	lmh_data = devm_kzalloc(dev, sizeof(*lmh_data), GFP_KERNEL);
+	if (!lmh_data)
+		return -ENOMEM;
+
+	lmh_data->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(lmh_data->base))
+		return PTR_ERR(lmh_data->base);
+
+	ret = of_property_read_u32(np, "qcom,lmh-cpu-id", &lmh_data->cpu_id);
+	if (ret) {
+		dev_err(dev, "missing qcom,lmh-cpu-id property\n");
+		return ret;
+	}
+
+	ret = of_property_read_u32(np, "qcom,lmh-temperature-high", &temp_high);
+	if (ret) {
+		dev_err(dev, "missing qcom,lmh-temperature-high property\n");
+		return ret;
+	}
+
+	ret = of_property_read_u32(np, "qcom,lmh-temperature-low", &temp_low);
+	if (ret) {
+		dev_err(dev, "missing qcom,lmh-temperature-low property\n");
+		return ret;
+	}
+
+	ret = of_property_read_u32(np, "qcom,lmh-temperature-arm", &temp_arm);
+	if (ret) {
+		dev_err(dev, "missing qcom,lmh-temperature-arm property\n");
+		return ret;
+	}
+
+	/*
+	 * Only sdm845 has lmh hardware currently enabled from hlos. If this is needed
+	 * for other platforms, revisit this to check if the <cpu-id, node-id> should be part
+	 * of a dt match table.
+	 */
+	if (lmh_data->cpu_id == 0) {
+		node_id = LMH_CLUSTER0_NODE_ID;
+	} else if (lmh_data->cpu_id == 4) {
+		node_id = LMH_CLUSTER1_NODE_ID;
+	} else {
+		dev_err(dev, "Wrong CPU id associated with LMh node\n");
+		return -EINVAL;
+	}
+
+	platform_set_drvdata(pdev, lmh_data);
+
+	if (!qcom_scm_lmh_dcvsh_available())
+		return -EINVAL;
+
+	ret = qcom_scm_lmh_dcvsh(LMH_SUB_FN_CRNT, LMH_ALGO_MODE_ENABLE, 1,
+				 LMH_NODE_DCVS, node_id, 0);
+	if (ret)
+		dev_err(dev, "Error %d enabling current subfunction\n", ret);
+
+	ret = qcom_scm_lmh_dcvsh(LMH_SUB_FN_REL, LMH_ALGO_MODE_ENABLE, 1,
+				 LMH_NODE_DCVS, node_id, 0);
+	if (ret)
+		dev_err(dev, "Error %d enabling reliability subfunction\n", ret);
+
+	ret = qcom_scm_lmh_dcvsh(LMH_SUB_FN_BCL, LMH_ALGO_MODE_ENABLE, 1,
+				 LMH_NODE_DCVS, node_id, 0);
+	if (ret)
+		dev_err(dev, "Error %d enabling BCL subfunction\n", ret);
+
+	ret = qcom_scm_lmh_dcvsh(LMH_SUB_FN_THERMAL, LMH_ALGO_MODE_ENABLE, 1,
+				 LMH_NODE_DCVS, node_id, 0);
+	if (ret) {
+		dev_err(dev, "Error %d enabling thermal subfunction\n", ret);
+		return ret;
+	}
+
+	ret = qcom_scm_lmh_profile_change(0x1);
+	if (ret) {
+		dev_err(dev, "Error %d changing profile\n", ret);
+		return ret;
+	}
+
+	/* Set default thermal trips */
+	ret = qcom_scm_lmh_dcvsh(LMH_SUB_FN_THERMAL, LMH_TH_ARM_THRESHOLD, temp_arm,
+				 LMH_NODE_DCVS, node_id, 0);
+	if (ret) {
+		dev_err(dev, "Error setting thermal ARM threshold%d\n", ret);
+		return ret;
+	}
+
+	ret = qcom_scm_lmh_dcvsh(LMH_SUB_FN_THERMAL, LMH_TH_HI_THRESHOLD, temp_high,
+				 LMH_NODE_DCVS, node_id, 0);
+	if (ret) {
+		dev_err(dev, "Error setting thermal HI threshold%d\n", ret);
+		return ret;
+	}
+
+	ret = qcom_scm_lmh_dcvsh(LMH_SUB_FN_THERMAL, LMH_TH_LOW_THRESHOLD, temp_low,
+				 LMH_NODE_DCVS, node_id, 0);
+	if (ret) {
+		dev_err(dev, "Error setting thermal ARM threshold%d\n", ret);
+		return ret;
+	}
+
+	lmh_data->irq = platform_get_irq(pdev, 0);
+	lmh_data->domain = irq_domain_add_linear(np, 1, &lmh_irq_ops, lmh_data);
+	if (!lmh_data->domain) {
+		dev_err(dev, "Error adding irq_domain\n");
+		return -EINVAL;
+	}
+
+	ret = devm_request_irq(dev, lmh_data->irq, lmh_handle_irq,
+			       IRQF_TRIGGER_HIGH | IRQF_ONESHOT | IRQF_NO_SUSPEND,
+			       "lmh-irq", lmh_data);
+	if (ret) {
+		dev_err(dev, "Error %d registering irq %x\n", ret, lmh_data->irq);
+		irq_domain_remove(lmh_data->domain);
+		return ret;
+	}
+
+	/* Disable the irq and let cpufreq enable it when ready to handle the interrupt */
+	disable_irq(lmh_data->irq);
+
+	return 0;
+}
+
+static const struct of_device_id lmh_table[] = {
+	{ .compatible = "qcom,sdm845-lmh", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, lmh_table);
+
+static struct platform_driver lmh_driver = {
+	.probe = lmh_probe,
+	.driver = {
+		.name = "qcom-lmh",
+		.of_match_table = lmh_table,
+	},
+};
+module_platform_driver(lmh_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("QCOM LMh driver");
-- 
2.25.1


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

* [Patch v3 3/6] cpufreq: qcom-cpufreq-hw: Add dcvs interrupt support
  2021-07-08 12:06 [Patch v3 0/6] Introduce LMh driver for Qualcomm SoCs Thara Gopinath
  2021-07-08 12:06 ` [Patch v3 1/6] firmware: qcom_scm: Introduce SCM calls to access LMh Thara Gopinath
  2021-07-08 12:06 ` [Patch v3 2/6] thermal: qcom: Add support for LMh driver Thara Gopinath
@ 2021-07-08 12:06 ` Thara Gopinath
  2021-07-09  6:46   ` Viresh Kumar
  2021-07-10  4:57   ` Bjorn Andersson
  2021-07-08 12:06 ` [Patch v3 4/6] arm64: boot: dts: qcom: sdm45: Add support for LMh node Thara Gopinath
                   ` (3 subsequent siblings)
  6 siblings, 2 replies; 29+ messages in thread
From: Thara Gopinath @ 2021-07-08 12:06 UTC (permalink / raw)
  To: agross, bjorn.andersson, rui.zhang, daniel.lezcano, viresh.kumar,
	rjw, robh+dt
  Cc: tdas, mka, linux-arm-msm, linux-pm, linux-kernel, devicetree

Add interrupt support to notify the kernel of h/w initiated frequency
throttling by LMh. Convey this to scheduler via thermal presssure
interface.

Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
---

v2->v3:
	- Cosmetic fixes from review comments on the list.
	- Moved all LMh initializations to qcom_cpufreq_hw_lmh_init.
	- Added freeing of LMh interrupt and cancelling the polling worker to
	  qcom_cpufreq_hw_cpu_exit as per Viresh's suggestion.
	- LMh interrupts are now tied to cpu dev and not cpufreq dev. This will be
	  useful for further generation of SoCs where the same interrupt signals
	  multiple cpu clusters.

v1->v2:
	- Introduced qcom_cpufreq_hw_lmh_init to consolidate LMh related initializations
	  as per Viresh's review comment.
	- Moved the piece of code restarting polling/re-enabling LMh interrupt to
	  qcom_lmh_dcvs_notify therby simplifying isr and timer callback as per Viresh's
	  suggestion.
	- Droped cpus from qcom_cpufreq_data and instead using cpus from cpufreq_policy in
	  qcom_lmh_dcvs_notify as per Viresh's review comment.
	- Dropped dt property qcom,support-lmh as per Bjorn's suggestion.
	- Other minor/cosmetic fixes

 drivers/cpufreq/qcom-cpufreq-hw.c | 118 ++++++++++++++++++++++++++++++
 1 file changed, 118 insertions(+)

diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
index f86859bf76f1..bb5fc700d913 100644
--- a/drivers/cpufreq/qcom-cpufreq-hw.c
+++ b/drivers/cpufreq/qcom-cpufreq-hw.c
@@ -7,6 +7,7 @@
 #include <linux/cpufreq.h>
 #include <linux/init.h>
 #include <linux/interconnect.h>
+#include <linux/interrupt.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/of_address.h>
@@ -22,10 +23,13 @@
 #define CLK_HW_DIV			2
 #define LUT_TURBO_IND			1
 
+#define HZ_PER_KHZ			1000
+
 struct qcom_cpufreq_soc_data {
 	u32 reg_enable;
 	u32 reg_freq_lut;
 	u32 reg_volt_lut;
+	u32 reg_current_vote;
 	u32 reg_perf_state;
 	u8 lut_row_size;
 };
@@ -33,7 +37,10 @@ struct qcom_cpufreq_soc_data {
 struct qcom_cpufreq_data {
 	void __iomem *base;
 	struct resource *res;
+	struct delayed_work lmh_dcvs_poll_work;
 	const struct qcom_cpufreq_soc_data *soc_data;
+	struct cpufreq_policy *policy;
+	int lmh_dcvs_irq;
 };
 
 static unsigned long cpu_hw_rate, xo_rate;
@@ -251,10 +258,84 @@ static void qcom_get_related_cpus(int index, struct cpumask *m)
 	}
 }
 
+static inline unsigned long qcom_lmh_vote_to_freq(u32 val)
+{
+	return (val & 0x3FF) * 19200;
+}
+
+static void qcom_lmh_dcvs_notify(struct qcom_cpufreq_data *data)
+{
+	struct cpufreq_policy *policy = data->policy;
+	struct dev_pm_opp *opp;
+	struct device *dev;
+	unsigned long max_capacity, capacity, freq_hz, throttled_freq;
+	unsigned int val, freq;
+
+	/*
+	 * Get the h/w throttled frequency, normalize it using the
+	 * registered opp table and use it to calculate thermal pressure.
+	 */
+	val = readl_relaxed(data->base + data->soc_data->reg_current_vote);
+	freq = qcom_lmh_vote_to_freq(val);
+	freq_hz = freq * HZ_PER_KHZ;
+
+	dev = get_cpu_device(cpumask_first(policy->cpus));
+	opp = dev_pm_opp_find_freq_floor(dev, &freq_hz);
+	if (IS_ERR(opp) && PTR_ERR(opp) == -ERANGE)
+		opp = dev_pm_opp_find_freq_ceil(dev, &freq_hz);
+
+	throttled_freq = freq_hz / HZ_PER_KHZ;
+
+	/* Update thermal pressure */
+
+	max_capacity = arch_scale_cpu_capacity(cpumask_first(policy->cpus));
+	capacity = throttled_freq * max_capacity;
+	capacity /= policy->cpuinfo.max_freq;
+
+	/* Don't pass boost capacity to scheduler */
+	if (capacity > max_capacity)
+		capacity = max_capacity;
+
+	arch_set_thermal_pressure(policy->cpus, max_capacity - capacity);
+
+	/*
+	 * If h/w throttled frequency is higher than what cpufreq has requested for, stop
+	 * polling and switch back to interrupt mechanism
+	 */
+
+	if (throttled_freq >= qcom_cpufreq_hw_get(cpumask_first(policy->cpus)))
+		/* Clear the existing interrupts and enable it back */
+		enable_irq(data->lmh_dcvs_irq);
+	else
+		mod_delayed_work(system_highpri_wq, &data->lmh_dcvs_poll_work,
+				 msecs_to_jiffies(10));
+}
+
+static void qcom_lmh_dcvs_poll(struct work_struct *work)
+{
+	struct qcom_cpufreq_data *data;
+
+	data = container_of(work, struct qcom_cpufreq_data, lmh_dcvs_poll_work.work);
+
+	qcom_lmh_dcvs_notify(data);
+}
+
+static irqreturn_t qcom_lmh_dcvs_handle_irq(int irq, void *data)
+{
+	struct qcom_cpufreq_data *c_data = data;
+
+	/* Disable interrupt and enable polling */
+	disable_irq_nosync(c_data->lmh_dcvs_irq);
+	qcom_lmh_dcvs_notify(c_data);
+
+	return 0;
+}
+
 static const struct qcom_cpufreq_soc_data qcom_soc_data = {
 	.reg_enable = 0x0,
 	.reg_freq_lut = 0x110,
 	.reg_volt_lut = 0x114,
+	.reg_current_vote = 0x704,
 	.reg_perf_state = 0x920,
 	.lut_row_size = 32,
 };
@@ -274,6 +355,35 @@ static const struct of_device_id qcom_cpufreq_hw_match[] = {
 };
 MODULE_DEVICE_TABLE(of, qcom_cpufreq_hw_match);
 
+static int qcom_cpufreq_hw_lmh_init(struct cpufreq_policy *policy, int index)
+{
+	struct qcom_cpufreq_data *data = policy->driver_data;
+	struct platform_device *pdev = cpufreq_get_driver_data();
+	struct device *cpu_dev = get_cpu_device(policy->cpu);
+	char irq_name[15];
+	int ret;
+
+	/*
+	 * Look for LMh interrupt. If no interrupt line is specified /
+	 * if there is an error, allow cpufreq to be enabled as usual.
+	 */
+	data->lmh_dcvs_irq = platform_get_irq(pdev, index);
+	if (data->lmh_dcvs_irq <= 0)
+		return data->lmh_dcvs_irq == -EPROBE_DEFER ? -EPROBE_DEFER : 0;
+
+	snprintf(irq_name, sizeof(irq_name), "dcvsh-irq-%u", policy->cpu);
+	ret = devm_request_irq(cpu_dev, data->lmh_dcvs_irq, qcom_lmh_dcvs_handle_irq,
+			       0, irq_name, data);
+	if (ret) {
+		dev_err(&pdev->dev, "Error %d registering irq %x\n", ret, data->lmh_dcvs_irq);
+		return 0;
+	}
+	data->policy = policy;
+	INIT_DEFERRABLE_WORK(&data->lmh_dcvs_poll_work, qcom_lmh_dcvs_poll);
+
+	return 0;
+}
+
 static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
 {
 	struct platform_device *pdev = cpufreq_get_driver_data();
@@ -370,6 +480,10 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
 			dev_warn(cpu_dev, "failed to enable boost: %d\n", ret);
 	}
 
+	ret = qcom_cpufreq_hw_lmh_init(policy, index);
+	if (ret)
+		goto error;
+
 	return 0;
 error:
 	kfree(data);
@@ -389,6 +503,10 @@ static int qcom_cpufreq_hw_cpu_exit(struct cpufreq_policy *policy)
 
 	dev_pm_opp_remove_all_dynamic(cpu_dev);
 	dev_pm_opp_of_cpumask_remove_table(policy->related_cpus);
+	if (data->lmh_dcvs_irq > 0) {
+		devm_free_irq(cpu_dev, data->lmh_dcvs_irq, data);
+		cancel_delayed_work_sync(&data->lmh_dcvs_poll_work);
+	}
 	kfree(policy->freq_table);
 	kfree(data);
 	iounmap(base);
-- 
2.25.1


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

* [Patch v3 4/6] arm64: boot: dts: qcom: sdm45: Add support for LMh node
  2021-07-08 12:06 [Patch v3 0/6] Introduce LMh driver for Qualcomm SoCs Thara Gopinath
                   ` (2 preceding siblings ...)
  2021-07-08 12:06 ` [Patch v3 3/6] cpufreq: qcom-cpufreq-hw: Add dcvs interrupt support Thara Gopinath
@ 2021-07-08 12:06 ` Thara Gopinath
  2021-07-10  4:17   ` Bjorn Andersson
  2021-07-19 16:33   ` Bjorn Andersson
  2021-07-08 12:06 ` [Patch v3 5/6] arm64: boot: dts: qcom: sdm845: Remove cpufreq cooling devices for CPU thermal zones Thara Gopinath
                   ` (2 subsequent siblings)
  6 siblings, 2 replies; 29+ messages in thread
From: Thara Gopinath @ 2021-07-08 12:06 UTC (permalink / raw)
  To: agross, bjorn.andersson, rui.zhang, daniel.lezcano, viresh.kumar,
	rjw, robh+dt
  Cc: tdas, mka, linux-arm-msm, linux-pm, linux-kernel, devicetree

Add LMh nodes for cpu cluster0 and cpu cluster1. Also add interrupt
support in cpufreq node to capture the LMh interrupt and let the scheduler
know of the max frequency throttling.

Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
---

v2->v3:
	- Changed the LMh low and high trip to 94500 and 95000 mC from
	  74500 and 75000 mC. This was a bug that got introduced in v2.
v1->v2:
	- Dropped dt property qcom,support-lmh as per Bjorn's review comments.
	- Changed lmh compatible from generic to platform specific.
	- Introduced properties specifying arm, low and high temp thresholds for LMh
	  as per Daniel's suggestion.

 arch/arm64/boot/dts/qcom/sdm845.dtsi | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index 0a86fe71a66d..4da6b8f3dd7b 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -3646,6 +3646,30 @@ swm: swm@c85 {
 			};
 		};
 
+		lmh_cluster1: lmh@17d70800 {
+			compatible = "qcom,sdm845-lmh";
+			reg = <0 0x17d70800 0 0x401>;
+			interrupts = <GIC_SPI 33 IRQ_TYPE_LEVEL_HIGH>;
+			qcom,lmh-cpu-id = <0x4>;
+			qcom,lmh-temperature-arm = <65000>;
+			qcom,lmh-temperature-low = <94500>;
+			qcom,lmh-temperature-high = <95000>;
+			interrupt-controller;
+			#interrupt-cells = <1>;
+		};
+
+		lmh_cluster0: lmh@17d78800 {
+			compatible = "qcom,sdm845-lmh";
+			reg = <0 0x17d78800 0 0x401>;
+			interrupts = <GIC_SPI 32 IRQ_TYPE_LEVEL_HIGH>;
+			qcom,lmh-cpu-id = <0x0>;
+			qcom,lmh-temperature-arm = <65000>;
+			qcom,lmh-temperature-low = <94500>;
+			qcom,lmh-temperature-high = <95000>;
+			interrupt-controller;
+			#interrupt-cells = <1>;
+		};
+
 		sound: sound {
 		};
 
@@ -4911,6 +4935,8 @@ cpufreq_hw: cpufreq@17d43000 {
 			reg = <0 0x17d43000 0 0x1400>, <0 0x17d45800 0 0x1400>;
 			reg-names = "freq-domain0", "freq-domain1";
 
+			interrupts-extended = <&lmh_cluster0 0>, <&lmh_cluster1 0>;
+
 			clocks = <&rpmhcc RPMH_CXO_CLK>, <&gcc GPLL0>;
 			clock-names = "xo", "alternate";
 
-- 
2.25.1


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

* [Patch v3 5/6] arm64: boot: dts: qcom: sdm845: Remove cpufreq cooling devices for CPU thermal zones
  2021-07-08 12:06 [Patch v3 0/6] Introduce LMh driver for Qualcomm SoCs Thara Gopinath
                   ` (3 preceding siblings ...)
  2021-07-08 12:06 ` [Patch v3 4/6] arm64: boot: dts: qcom: sdm45: Add support for LMh node Thara Gopinath
@ 2021-07-08 12:06 ` Thara Gopinath
  2021-07-10  4:17   ` Bjorn Andersson
  2021-07-08 12:06 ` [Patch v3 6/6] dt-bindings: thermal: Add dt binding for QCOM LMh Thara Gopinath
  2021-07-22  3:14 ` [Patch v3 0/6] Introduce LMh driver for Qualcomm SoCs Steev Klimaszewski
  6 siblings, 1 reply; 29+ messages in thread
From: Thara Gopinath @ 2021-07-08 12:06 UTC (permalink / raw)
  To: agross, bjorn.andersson, rui.zhang, daniel.lezcano, viresh.kumar,
	rjw, robh+dt
  Cc: tdas, mka, linux-arm-msm, linux-pm, linux-kernel, devicetree

Now that Limits h/w is enabled to monitor thermal events around cpus and
throttle the cpu frequencies, remove cpufreq cooling device for the CPU
thermal zones which does software throttling of cpu frequencies.

Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
---

v2->v3:
	- Improved the subject header and descrption to better reflect the
	  patch as per Matthias's review comments.

v1->v2:
	Removing only cooling maps for cpu specific thermal zones keeping the
	trip point definitions intact as per Daniel's suggestion. This is to
	ensure that thermal zone temparature and trip violation information is
	available to any userspace daemon monitoring these zones.

 arch/arm64/boot/dts/qcom/sdm845.dtsi | 136 ---------------------------
 1 file changed, 136 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index 4da6b8f3dd7b..6185fff8859a 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -4994,23 +4994,6 @@ cpu0_crit: cpu_crit {
 					type = "critical";
 				};
 			};
-
-			cooling-maps {
-				map0 {
-					trip = <&cpu0_alert0>;
-					cooling-device = <&CPU0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
-							 <&CPU1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
-							 <&CPU2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
-							 <&CPU3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
-				};
-				map1 {
-					trip = <&cpu0_alert1>;
-					cooling-device = <&CPU0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
-							 <&CPU1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
-							 <&CPU2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
-							 <&CPU3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
-				};
-			};
 		};
 
 		cpu1-thermal {
@@ -5038,23 +5021,6 @@ cpu1_crit: cpu_crit {
 					type = "critical";
 				};
 			};
-
-			cooling-maps {
-				map0 {
-					trip = <&cpu1_alert0>;
-					cooling-device = <&CPU0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
-							 <&CPU1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
-							 <&CPU2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
-							 <&CPU3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
-				};
-				map1 {
-					trip = <&cpu1_alert1>;
-					cooling-device = <&CPU0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
-							 <&CPU1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
-							 <&CPU2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
-							 <&CPU3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
-				};
-			};
 		};
 
 		cpu2-thermal {
@@ -5082,23 +5048,6 @@ cpu2_crit: cpu_crit {
 					type = "critical";
 				};
 			};
-
-			cooling-maps {
-				map0 {
-					trip = <&cpu2_alert0>;
-					cooling-device = <&CPU0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
-							 <&CPU1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
-							 <&CPU2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
-							 <&CPU3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
-				};
-				map1 {
-					trip = <&cpu2_alert1>;
-					cooling-device = <&CPU0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
-							 <&CPU1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
-							 <&CPU2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
-							 <&CPU3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
-				};
-			};
 		};
 
 		cpu3-thermal {
@@ -5126,23 +5075,6 @@ cpu3_crit: cpu_crit {
 					type = "critical";
 				};
 			};
-
-			cooling-maps {
-				map0 {
-					trip = <&cpu3_alert0>;
-					cooling-device = <&CPU0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
-							 <&CPU1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
-							 <&CPU2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
-							 <&CPU3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
-				};
-				map1 {
-					trip = <&cpu3_alert1>;
-					cooling-device = <&CPU0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
-							 <&CPU1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
-							 <&CPU2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
-							 <&CPU3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
-				};
-			};
 		};
 
 		cpu4-thermal {
@@ -5170,23 +5102,6 @@ cpu4_crit: cpu_crit {
 					type = "critical";
 				};
 			};
-
-			cooling-maps {
-				map0 {
-					trip = <&cpu4_alert0>;
-					cooling-device = <&CPU4 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
-							 <&CPU5 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
-							 <&CPU6 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
-							 <&CPU7 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
-				};
-				map1 {
-					trip = <&cpu4_alert1>;
-					cooling-device = <&CPU4 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
-							 <&CPU5 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
-							 <&CPU6 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
-							 <&CPU7 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
-				};
-			};
 		};
 
 		cpu5-thermal {
@@ -5214,23 +5129,6 @@ cpu5_crit: cpu_crit {
 					type = "critical";
 				};
 			};
-
-			cooling-maps {
-				map0 {
-					trip = <&cpu5_alert0>;
-					cooling-device = <&CPU4 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
-							 <&CPU5 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
-							 <&CPU6 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
-							 <&CPU7 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
-				};
-				map1 {
-					trip = <&cpu5_alert1>;
-					cooling-device = <&CPU4 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
-							 <&CPU5 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
-							 <&CPU6 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
-							 <&CPU7 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
-				};
-			};
 		};
 
 		cpu6-thermal {
@@ -5258,23 +5156,6 @@ cpu6_crit: cpu_crit {
 					type = "critical";
 				};
 			};
-
-			cooling-maps {
-				map0 {
-					trip = <&cpu6_alert0>;
-					cooling-device = <&CPU4 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
-							 <&CPU5 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
-							 <&CPU6 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
-							 <&CPU7 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
-				};
-				map1 {
-					trip = <&cpu6_alert1>;
-					cooling-device = <&CPU4 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
-							 <&CPU5 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
-							 <&CPU6 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
-							 <&CPU7 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
-				};
-			};
 		};
 
 		cpu7-thermal {
@@ -5302,23 +5183,6 @@ cpu7_crit: cpu_crit {
 					type = "critical";
 				};
 			};
-
-			cooling-maps {
-				map0 {
-					trip = <&cpu7_alert0>;
-					cooling-device = <&CPU4 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
-							 <&CPU5 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
-							 <&CPU6 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
-							 <&CPU7 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
-				};
-				map1 {
-					trip = <&cpu7_alert1>;
-					cooling-device = <&CPU4 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
-							 <&CPU5 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
-							 <&CPU6 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
-							 <&CPU7 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
-				};
-			};
 		};
 
 		aoss0-thermal {
-- 
2.25.1


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

* [Patch v3 6/6] dt-bindings: thermal: Add dt binding for QCOM LMh
  2021-07-08 12:06 [Patch v3 0/6] Introduce LMh driver for Qualcomm SoCs Thara Gopinath
                   ` (4 preceding siblings ...)
  2021-07-08 12:06 ` [Patch v3 5/6] arm64: boot: dts: qcom: sdm845: Remove cpufreq cooling devices for CPU thermal zones Thara Gopinath
@ 2021-07-08 12:06 ` Thara Gopinath
  2021-07-10  4:21   ` Bjorn Andersson
  2021-07-12 17:32   ` Rob Herring
  2021-07-22  3:14 ` [Patch v3 0/6] Introduce LMh driver for Qualcomm SoCs Steev Klimaszewski
  6 siblings, 2 replies; 29+ messages in thread
From: Thara Gopinath @ 2021-07-08 12:06 UTC (permalink / raw)
  To: agross, bjorn.andersson, rui.zhang, daniel.lezcano, viresh.kumar,
	rjw, robh+dt
  Cc: tdas, mka, linux-arm-msm, linux-pm, linux-kernel, devicetree

Add dt binding documentation to describe Qualcomm
Limits Management Hardware node.

Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
---
 .../devicetree/bindings/thermal/qcom-lmh.yaml | 100 ++++++++++++++++++
 1 file changed, 100 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/thermal/qcom-lmh.yaml

diff --git a/Documentation/devicetree/bindings/thermal/qcom-lmh.yaml b/Documentation/devicetree/bindings/thermal/qcom-lmh.yaml
new file mode 100644
index 000000000000..7f62bd3d543d
--- /dev/null
+++ b/Documentation/devicetree/bindings/thermal/qcom-lmh.yaml
@@ -0,0 +1,100 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright 2021 Linaro Ltd.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/thermal/qcom-lmh.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm Limits Management Hardware(LMh)
+
+maintainers:
+  - Thara Gopinath <thara.gopinath@linaro.org>
+
+description:
+  Limits Management Hardware(LMh) is a hardware infrastructure on some
+  Qualcomm SoCs that can enforce temperature and current limits as
+  programmed by software for certain IPs like CPU.
+
+properties:
+  compatible:
+    enum:
+      - qcom,sdm845-lmh
+
+  reg:
+    items:
+      - description: core registers
+
+  interrupts:
+    maxItems: 1
+
+  '#interrupt-cells':
+    const: 1
+
+  interrupt-controller: true
+
+  qcom,lmh-cpu-id:
+    description:
+      CPU id of the first cpu in the LMh cluster
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+  qcom,lmh-temperature-arm:
+    description:
+      An integer expressing temperature threshold in millicelsius at which
+      the LMh thermal FSM is engaged.
+    $ref: /schemas/types.yaml#/definitions/int32
+
+  qcom,lmh-temperature-low:
+    description:
+      An integer expressing temperature threshold in millicelsius at which
+      the LMh thermal FSM is engaged.
+    $ref: /schemas/types.yaml#/definitions/int32
+
+  qcom,lmh-temperature-high:
+    description:
+      An integer expressing temperature threshold in millicelsius at which
+      the LMh thermal FSM is engaged.
+    $ref: /schemas/types.yaml#/definitions/int32
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - #interrupt-cells
+  - interrupt-controller
+  - qcom,lmh-cpu-id
+  - qcom,lmh-temperature-arm
+  - qcom,lmh-temperature-low
+  - qcom,lmh-temperature-high
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/clock/qcom,rpmh.h>
+    #include <dt-bindings/interconnect/qcom,sdm845.h>
+
+    lmh_cluster1: lmh@17d70800 {
+      compatible = "qcom,sdm845-lmh";
+      reg = <0 0x17d70800 0 0x401>;
+      interrupts = <GIC_SPI 33 IRQ_TYPE_LEVEL_HIGH>;
+      qcom,lmh-cpu-id = <0x4>;
+      qcom,lmh-temperature-arm = <65000>;
+      qcom,lmh-temperature-low = <94500>;
+      qcom,lmh-temperature-high = <95000>;
+      interrupt-controller;
+      #interrupt-cells = <1>;
+    };
+  - |
+    lmh_cluster0: lmh@17d78800 {
+      compatible = "qcom,sdm845-lmh";
+      reg = <0 0x17d78800 0 0x401>;
+      interrupts = <GIC_SPI 32 IRQ_TYPE_LEVEL_HIGH>;
+      qcom,lmh-cpu-id = <0x0>;
+      qcom,lmh-temperature-arm = <65000>;
+      qcom,lmh-temperature-low = <94500>;
+      qcom,lmh-temperature-high = <95000>;
+      interrupt-controller;
+      #interrupt-cells = <1>;
+    };
+  - |
-- 
2.25.1


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

* Re: [Patch v3 3/6] cpufreq: qcom-cpufreq-hw: Add dcvs interrupt support
  2021-07-08 12:06 ` [Patch v3 3/6] cpufreq: qcom-cpufreq-hw: Add dcvs interrupt support Thara Gopinath
@ 2021-07-09  6:46   ` Viresh Kumar
  2021-07-09 15:37     ` Thara Gopinath
  2021-07-10  4:57   ` Bjorn Andersson
  1 sibling, 1 reply; 29+ messages in thread
From: Viresh Kumar @ 2021-07-09  6:46 UTC (permalink / raw)
  To: Thara Gopinath
  Cc: agross, bjorn.andersson, rui.zhang, daniel.lezcano, rjw, robh+dt,
	tdas, mka, linux-arm-msm, linux-pm, linux-kernel, devicetree

On 08-07-21, 08:06, Thara Gopinath wrote:
>  static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
>  {
>  	struct platform_device *pdev = cpufreq_get_driver_data();
> @@ -370,6 +480,10 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
>  			dev_warn(cpu_dev, "failed to enable boost: %d\n", ret);
>  	}
>  
> +	ret = qcom_cpufreq_hw_lmh_init(policy, index);

You missed unregistering EM here (which is also missing from exit,
which you need to fix first in a separate patch).

> +	if (ret)
> +		goto error;
> +
>  	return 0;
>  error:
>  	kfree(data);
> @@ -389,6 +503,10 @@ static int qcom_cpufreq_hw_cpu_exit(struct cpufreq_policy *policy)
>  
>  	dev_pm_opp_remove_all_dynamic(cpu_dev);
>  	dev_pm_opp_of_cpumask_remove_table(policy->related_cpus);
> +	if (data->lmh_dcvs_irq > 0) {
> +		devm_free_irq(cpu_dev, data->lmh_dcvs_irq, data);

Why using devm variants here and while requesting the irq ? 

> +		cancel_delayed_work_sync(&data->lmh_dcvs_poll_work);
> +	}

Please move this to qcom_cpufreq_hw_lmh_exit() or something.

Now with sequence of disabling interrupt, etc, I see a potential
problem.

CPU0                                    CPU1

qcom_cpufreq_hw_cpu_exit()
-> devm_free_irq();
                                        qcom_lmh_dcvs_poll()
                                        -> qcom_lmh_dcvs_notify()
                                          -> enable_irq()

-> cancel_delayed_work_sync();


What will happen if enable_irq() gets called after freeing the irq ?
Not sure, but it looks like you will hit this then from manage.c:

WARN(!desc->irq_data.chip, KERN_ERR "enable_irq before
                                     setup/request_irq: irq %u\n", irq))

?

You got a chicken n egg problem :)

-- 
viresh

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

* Re: [Patch v3 3/6] cpufreq: qcom-cpufreq-hw: Add dcvs interrupt support
  2021-07-09  6:46   ` Viresh Kumar
@ 2021-07-09 15:37     ` Thara Gopinath
  2021-07-12  4:35       ` Viresh Kumar
  2021-07-12  4:41       ` Viresh Kumar
  0 siblings, 2 replies; 29+ messages in thread
From: Thara Gopinath @ 2021-07-09 15:37 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: agross, bjorn.andersson, rui.zhang, daniel.lezcano, rjw, robh+dt,
	tdas, mka, linux-arm-msm, linux-pm, linux-kernel, devicetree



On 7/9/21 2:46 AM, Viresh Kumar wrote:
> On 08-07-21, 08:06, Thara Gopinath wrote:
>>   static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
>>   {
>>   	struct platform_device *pdev = cpufreq_get_driver_data();
>> @@ -370,6 +480,10 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
>>   			dev_warn(cpu_dev, "failed to enable boost: %d\n", ret);
>>   	}
>>   
>> +	ret = qcom_cpufreq_hw_lmh_init(policy, index);
> 
> You missed unregistering EM here (which is also missing from exit,
> which you need to fix first in a separate patch).

Hi!

So how exactly do you do this? I checked other users of the api and I do 
not see any free. I would say if needed, it should be a separate patch 
and outside of this series.

> 
>> +	if (ret)
>> +		goto error;
>> +
>>   	return 0;
>>   error:
>>   	kfree(data);
>> @@ -389,6 +503,10 @@ static int qcom_cpufreq_hw_cpu_exit(struct cpufreq_policy *policy)
>>   
>>   	dev_pm_opp_remove_all_dynamic(cpu_dev);
>>   	dev_pm_opp_of_cpumask_remove_table(policy->related_cpus);
>> +	if (data->lmh_dcvs_irq > 0) {
>> +		devm_free_irq(cpu_dev, data->lmh_dcvs_irq, data);
> 
> Why using devm variants here and while requesting the irq ?
> 
>> +		cancel_delayed_work_sync(&data->lmh_dcvs_poll_work);
>> +	}
> 
> Please move this to qcom_cpufreq_hw_lmh_exit() or something.

Ok.

> 
> Now with sequence of disabling interrupt, etc, I see a potential
> problem.
> 
> CPU0                                    CPU1
> 
> qcom_cpufreq_hw_cpu_exit()
> -> devm_free_irq();
>                                          qcom_lmh_dcvs_poll()
>                                          -> qcom_lmh_dcvs_notify()
>                                            -> enable_irq()
> 
> -> cancel_delayed_work_sync();
> 
> 
> What will happen if enable_irq() gets called after freeing the irq ?
> Not sure, but it looks like you will hit this then from manage.c:
> 
> WARN(!desc->irq_data.chip, KERN_ERR "enable_irq before
>                                       setup/request_irq: irq %u\n", irq))
> 
> ?
> 
> You got a chicken n egg problem :)

Yes indeed! But also it is a very rare chicken and egg problem.
The scenario here is that the cpus are busy and running load causing a 
thermal overrun and lmh is engaged. At the same time for this issue to 
be hit the cpu is trying to exit/disable cpufreq. Calling 
cancel_delayed_work_sync first could solve this issue, right ? 
cancel_delayed_work_sync guarantees the work not to be pending even if
it requeues itself on return. So once the delayed work is cancelled, the 
interrupts can be safely disabled. Thoughts ?


> 

-- 
Warm Regards
Thara (She/Her/Hers)

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

* Re: [Patch v3 1/6] firmware: qcom_scm: Introduce SCM calls to access LMh
  2021-07-08 12:06 ` [Patch v3 1/6] firmware: qcom_scm: Introduce SCM calls to access LMh Thara Gopinath
@ 2021-07-10  4:02   ` Bjorn Andersson
  0 siblings, 0 replies; 29+ messages in thread
From: Bjorn Andersson @ 2021-07-10  4:02 UTC (permalink / raw)
  To: Thara Gopinath
  Cc: agross, rui.zhang, daniel.lezcano, viresh.kumar, rjw, robh+dt,
	tdas, mka, linux-arm-msm, linux-pm, linux-kernel, devicetree

On Thu 08 Jul 07:06 CDT 2021, Thara Gopinath wrote:

> Introduce SCM calls to access/configure limits management hardware(LMH).
> 
> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> ---
> 
> v2->v3:
> 	Added freeing of payload_buf after the scm call in qcom_scm_lmh_dcvsh as per
> 	Matthias review comments.
> 
> v1->v2:
> 	Changed the input parameters in qcom_scm_lmh_dcvsh from payload_buf and
> 	payload_size to payload_fn, payload_reg, payload_val as per Bjorn's review
> 	comments.
> 
>  drivers/firmware/qcom_scm.c | 58 +++++++++++++++++++++++++++++++++++++
>  drivers/firmware/qcom_scm.h |  4 +++
>  include/linux/qcom_scm.h    | 14 +++++++++
>  3 files changed, 76 insertions(+)
> 
> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> index ee9cb545e73b..a8d236603e90 100644
> --- a/drivers/firmware/qcom_scm.c
> +++ b/drivers/firmware/qcom_scm.c
> @@ -1147,6 +1147,64 @@ int qcom_scm_qsmmu500_wait_safe_toggle(bool en)
>  }
>  EXPORT_SYMBOL(qcom_scm_qsmmu500_wait_safe_toggle);
>  
> +bool qcom_scm_lmh_dcvsh_available(void)
> +{
> +	return __qcom_scm_is_call_available(__scm->dev, QCOM_SCM_SVC_LMH, QCOM_SCM_LMH_LIMIT_DCVSH);
> +}
> +EXPORT_SYMBOL(qcom_scm_lmh_dcvsh_available);
> +
> +int qcom_scm_lmh_profile_change(u32 profile_id)
> +{
> +	struct qcom_scm_desc desc = {
> +		.svc = QCOM_SCM_SVC_LMH,
> +		.cmd = QCOM_SCM_LMH_LIMIT_PROFILE_CHANGE,
> +		.arginfo = QCOM_SCM_ARGS(1, QCOM_SCM_VAL),
> +		.args[0] = profile_id,
> +		.owner = ARM_SMCCC_OWNER_SIP,
> +	};
> +
> +	return qcom_scm_call(__scm->dev, &desc, NULL);
> +}
> +EXPORT_SYMBOL(qcom_scm_lmh_profile_change);
> +
> +int qcom_scm_lmh_dcvsh(u32 payload_fn, u32 payload_reg, u32 payload_val,
> +		       u64 limit_node, u32 node_id, u64 version)
> +{
> +	dma_addr_t payload_phys;
> +	u32 *payload_buf;
> +	int ret, payload_size = 5 * sizeof(u32);
> +
> +	struct qcom_scm_desc desc = {
> +		.svc = QCOM_SCM_SVC_LMH,
> +		.cmd = QCOM_SCM_LMH_LIMIT_DCVSH,
> +		.arginfo = QCOM_SCM_ARGS(5, QCOM_SCM_RO, QCOM_SCM_VAL, QCOM_SCM_VAL,
> +					QCOM_SCM_VAL, QCOM_SCM_VAL),
> +		.args[1] = payload_size,
> +		.args[2] = limit_node,
> +		.args[3] = node_id,
> +		.args[4] = version,
> +		.owner = ARM_SMCCC_OWNER_SIP,
> +	};
> +
> +	payload_buf = dma_alloc_coherent(__scm->dev, payload_size, &payload_phys, GFP_KERNEL);
> +	if (!payload_buf)
> +		return -ENOMEM;
> +
> +	payload_buf[0] = payload_fn;
> +	payload_buf[1] = 0;
> +	payload_buf[2] = payload_reg;
> +	payload_buf[3] = 1;
> +	payload_buf[4] = payload_val;
> +
> +	desc.args[0] = payload_phys;
> +
> +	ret = qcom_scm_call(__scm->dev, &desc, NULL);
> +
> +	dma_free_coherent(__scm->dev, payload_size, payload_buf, payload_phys);
> +	return ret;
> +}
> +EXPORT_SYMBOL(qcom_scm_lmh_dcvsh);
> +
>  static int qcom_scm_find_dload_address(struct device *dev, u64 *addr)
>  {
>  	struct device_node *tcsr;
> diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
> index 632fe3142462..d92156ceb3ac 100644
> --- a/drivers/firmware/qcom_scm.h
> +++ b/drivers/firmware/qcom_scm.h
> @@ -114,6 +114,10 @@ extern int scm_legacy_call(struct device *dev, const struct qcom_scm_desc *desc,
>  #define QCOM_SCM_SVC_HDCP		0x11
>  #define QCOM_SCM_HDCP_INVOKE		0x01
>  
> +#define QCOM_SCM_SVC_LMH			0x13
> +#define QCOM_SCM_LMH_LIMIT_PROFILE_CHANGE	0x01
> +#define QCOM_SCM_LMH_LIMIT_DCVSH		0x10
> +
>  #define QCOM_SCM_SVC_SMMU_PROGRAM		0x15
>  #define QCOM_SCM_SMMU_CONFIG_ERRATA1		0x03
>  #define QCOM_SCM_SMMU_CONFIG_ERRATA1_CLIENT_ALL	0x02
> diff --git a/include/linux/qcom_scm.h b/include/linux/qcom_scm.h
> index 0165824c5128..c0475d1c9885 100644
> --- a/include/linux/qcom_scm.h
> +++ b/include/linux/qcom_scm.h
> @@ -109,6 +109,12 @@ extern int qcom_scm_hdcp_req(struct qcom_scm_hdcp_req *req, u32 req_cnt,
>  			     u32 *resp);
>  
>  extern int qcom_scm_qsmmu500_wait_safe_toggle(bool en);
> +
> +extern int qcom_scm_lmh_dcvsh(u32 payload_fn, u32 payload_reg, u32 payload_val,
> +			      u64 limit_node, u32 node_id, u64 version);
> +extern int qcom_scm_lmh_profile_change(u32 profile_id);
> +extern bool qcom_scm_lmh_dcvsh_available(void);
> +
>  #else
>  
>  #include <linux/errno.h>
> @@ -170,5 +176,13 @@ static inline int qcom_scm_hdcp_req(struct qcom_scm_hdcp_req *req, u32 req_cnt,
>  
>  static inline int qcom_scm_qsmmu500_wait_safe_toggle(bool en)
>  		{ return -ENODEV; }
> +
> +static inline int qcom_scm_lmh_dcvsh(u32 payload_fn, u32 payload_reg, u32 payload_val,
> +				     u64 limit_node, u32 node_id, u64 version)
> +		{ return -ENODEV; }
> +
> +static inline int qcom_scm_lmh_profile_change(u32 profile_id) { return -ENODEV; }
> +
> +static inline bool qcom_scm_lmh_dcvsh_available(void) { return -ENODEV; }
>  #endif
>  #endif
> -- 
> 2.25.1
> 

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

* Re: [Patch v3 2/6] thermal: qcom: Add support for LMh driver
  2021-07-08 12:06 ` [Patch v3 2/6] thermal: qcom: Add support for LMh driver Thara Gopinath
@ 2021-07-10  4:15   ` Bjorn Andersson
  2021-07-13  0:49     ` Thara Gopinath
  0 siblings, 1 reply; 29+ messages in thread
From: Bjorn Andersson @ 2021-07-10  4:15 UTC (permalink / raw)
  To: Thara Gopinath
  Cc: agross, rui.zhang, daniel.lezcano, viresh.kumar, rjw, robh+dt,
	tdas, mka, linux-arm-msm, linux-pm, linux-kernel, devicetree

On Thu 08 Jul 07:06 CDT 2021, Thara Gopinath wrote:

> Driver enabling various pieces of Limits Management Hardware(LMh) for cpu
> cluster0 and cpu cluster1 namely kick starting monitoring of temperature,
> current, battery current violations, enabling reliability algorithm and
> setting up various temperature limits.
> 
> The following has been explained in the cover letter. I am including this
> here so that this remains in the commit message as well.
> 
> LMh is a hardware infrastructure on some Qualcomm SoCs that can enforce
> temperature and current limits as programmed by software for certain IPs
> like CPU. On many newer LMh is configured by firmware/TZ and no programming
> is needed from the kernel side. But on certain SoCs like sdm845 the
> firmware does not do a complete programming of the h/w. On such soc's
> kernel software has to explicitly set up the temperature limits and turn on
> various monitoring and enforcing algorithms on the hardware.
> 
> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
> ---
> 
> v2->v3:
> 	- Rearranged enabling of various LMh subfunction and removed returning
> 	  on error in enabling any one subfunction as the different pieces can
> 	  operate and thus be enabled independently.
> 	- Other minor cosmetic fixes.
> 
> v1->v2:
> 	- Cosmetic and spelling fixes from review comments from Randy Dunlap
> 	- Added irq_disable to lmh_irq_ops and removed disabling of irq from
> 	  lmh_handle_irq. Now cpufreq explicitly disables irq prior to
> 	  handling it as per Bjorn's suggestion.
> 	- Rebased to new version of qcom_scm_lmh_dcvsh as changed in patch 1.
> 	- Removed generic dt compatible string and introduced platform specific one
> 	  as per Bjorn's suggestion.
> 	- Take arm, low and high temp thresholds for LMh from dt properties instead of
> 	  #defines in the driver as per Daniel's suggestion.
> 	- Other minor fixes.
>  drivers/thermal/qcom/Kconfig  |  10 ++
>  drivers/thermal/qcom/Makefile |   1 +
>  drivers/thermal/qcom/lmh.c    | 239 ++++++++++++++++++++++++++++++++++
>  3 files changed, 250 insertions(+)
>  create mode 100644 drivers/thermal/qcom/lmh.c
> 
> diff --git a/drivers/thermal/qcom/Kconfig b/drivers/thermal/qcom/Kconfig
> index 8d5ac2df26dc..7d942f71e532 100644
> --- a/drivers/thermal/qcom/Kconfig
> +++ b/drivers/thermal/qcom/Kconfig
> @@ -31,3 +31,13 @@ config QCOM_SPMI_TEMP_ALARM
>  	  trip points. The temperature reported by the thermal sensor reflects the
>  	  real time die temperature if an ADC is present or an estimate of the
>  	  temperature based upon the over temperature stage value.
> +
> +config QCOM_LMH
> +	tristate "Qualcomm Limits Management Hardware"
> +	depends on ARCH_QCOM
> +	help
> +	  This enables initialization of Qualcomm limits management
> +	  hardware(LMh). LMh allows for hardware-enforced mitigation for cpus based on
> +	  input from temperature and current sensors.  On many newer Qualcomm SoCs
> +	  LMh is configured in the firmware and this feature need not be enabled.
> +	  However, on certain SoCs like sdm845 LMh has to be configured from kernel.
> diff --git a/drivers/thermal/qcom/Makefile b/drivers/thermal/qcom/Makefile
> index 252ea7d9da0b..0fa2512042e7 100644
> --- a/drivers/thermal/qcom/Makefile
> +++ b/drivers/thermal/qcom/Makefile
> @@ -5,3 +5,4 @@ qcom_tsens-y			+= tsens.o tsens-v2.o tsens-v1.o tsens-v0_1.o \
>  				   tsens-8960.o
>  obj-$(CONFIG_QCOM_SPMI_ADC_TM5)	+= qcom-spmi-adc-tm5.o
>  obj-$(CONFIG_QCOM_SPMI_TEMP_ALARM)	+= qcom-spmi-temp-alarm.o
> +obj-$(CONFIG_QCOM_LMH)		+= lmh.o
> diff --git a/drivers/thermal/qcom/lmh.c b/drivers/thermal/qcom/lmh.c
> new file mode 100644
> index 000000000000..a7b1eb308642
> --- /dev/null
> +++ b/drivers/thermal/qcom/lmh.c
> @@ -0,0 +1,239 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +/*
> + * Copyright (C) 2021, Linaro Limited. All rights reserved.
> + */
> +#include <linux/module.h>
> +#include <linux/interrupt.h>
> +#include <linux/irqdomain.h>
> +#include <linux/err.h>
> +#include <linux/platform_device.h>
> +#include <linux/of_platform.h>
> +#include <linux/slab.h>
> +#include <linux/qcom_scm.h>
> +
> +#define LMH_NODE_DCVS			0x44435653
> +#define LMH_CLUSTER0_NODE_ID		0x6370302D
> +#define LMH_CLUSTER1_NODE_ID		0x6370312D
> +
> +#define LMH_SUB_FN_THERMAL		0x54484D4C
> +#define LMH_SUB_FN_CRNT			0x43524E54
> +#define LMH_SUB_FN_REL			0x52454C00
> +#define LMH_SUB_FN_BCL			0x42434C00
> +
> +#define LMH_ALGO_MODE_ENABLE		0x454E424C
> +#define LMH_TH_HI_THRESHOLD		0x48494748
> +#define LMH_TH_LOW_THRESHOLD		0x4C4F5700
> +#define LMH_TH_ARM_THRESHOLD		0x41524D00
> +
> +#define LMH_REG_DCVS_INTR_CLR		0x8
> +
> +struct lmh_hw_data {
> +	void __iomem *base;
> +	struct irq_domain *domain;
> +	int irq;
> +	u32 cpu_id;

cpu_id seems to only be used in lmh_probe(), how about making it a local
variable?

> +};
> +
> +static irqreturn_t lmh_handle_irq(int hw_irq, void *data)
> +{
> +	struct lmh_hw_data *lmh_data = data;
> +	int irq = irq_find_mapping(lmh_data->domain, 0);
> +
> +	/* Call the cpufreq driver to handle the interrupt */
> +	if (irq)
> +		generic_handle_irq(irq);
> +
> +	return 0;
> +}
> +
> +static void lmh_enable_interrupt(struct irq_data *d)
> +{
> +	struct lmh_hw_data *lmh_data = irq_data_get_irq_chip_data(d);
> +
> +	/* Clear the existing interrupt */
> +	writel(0xff, lmh_data->base + LMH_REG_DCVS_INTR_CLR);
> +	enable_irq(lmh_data->irq);
> +}
> +
> +static void lmh_disable_interrupt(struct irq_data *d)
> +{
> +	struct lmh_hw_data *lmh_data = irq_data_get_irq_chip_data(d);
> +
> +	disable_irq_nosync(lmh_data->irq);
> +}
> +
> +static struct irq_chip lmh_irq_chip = {
> +	.name           = "lmh",
> +	.irq_enable	= lmh_enable_interrupt,
> +	.irq_disable	= lmh_disable_interrupt
> +};
> +
> +static int lmh_irq_map(struct irq_domain *d, unsigned int irq, irq_hw_number_t hw)
> +{
> +	struct lmh_hw_data *lmh_data = d->host_data;
> +
> +	irq_set_chip_and_handler(irq, &lmh_irq_chip, handle_simple_irq);
> +	irq_set_chip_data(irq, lmh_data);
> +
> +	return 0;
> +}
> +
> +static const struct irq_domain_ops lmh_irq_ops = {
> +	.map = lmh_irq_map,
> +	.xlate = irq_domain_xlate_onecell,
> +};
> +
> +static int lmh_probe(struct platform_device *pdev)
> +{
> +	struct device *dev;
> +	struct device_node *np;
> +	struct lmh_hw_data *lmh_data;
> +	u32 node_id;
> +	int temp_low, temp_high, temp_arm, ret;
> +
> +	dev = &pdev->dev;
> +	np = dev->of_node;

How about initialize these as you declare you variables?

> +	if (!np)

There's no reasonable way to probe this driver with !dev->of_node, so
you can skip this check.

> +		return -EINVAL;
> +
> +	lmh_data = devm_kzalloc(dev, sizeof(*lmh_data), GFP_KERNEL);
> +	if (!lmh_data)
> +		return -ENOMEM;
> +
> +	lmh_data->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(lmh_data->base))
> +		return PTR_ERR(lmh_data->base);
> +
> +	ret = of_property_read_u32(np, "qcom,lmh-cpu-id", &lmh_data->cpu_id);
> +	if (ret) {
> +		dev_err(dev, "missing qcom,lmh-cpu-id property\n");
> +		return ret;
> +	}
> +
> +	ret = of_property_read_u32(np, "qcom,lmh-temperature-high", &temp_high);
> +	if (ret) {
> +		dev_err(dev, "missing qcom,lmh-temperature-high property\n");
> +		return ret;
> +	}
> +
> +	ret = of_property_read_u32(np, "qcom,lmh-temperature-low", &temp_low);
> +	if (ret) {
> +		dev_err(dev, "missing qcom,lmh-temperature-low property\n");
> +		return ret;
> +	}
> +
> +	ret = of_property_read_u32(np, "qcom,lmh-temperature-arm", &temp_arm);
> +	if (ret) {
> +		dev_err(dev, "missing qcom,lmh-temperature-arm property\n");
> +		return ret;
> +	}
> +
> +	/*
> +	 * Only sdm845 has lmh hardware currently enabled from hlos. If this is needed
> +	 * for other platforms, revisit this to check if the <cpu-id, node-id> should be part
> +	 * of a dt match table.
> +	 */
> +	if (lmh_data->cpu_id == 0) {
> +		node_id = LMH_CLUSTER0_NODE_ID;
> +	} else if (lmh_data->cpu_id == 4) {
> +		node_id = LMH_CLUSTER1_NODE_ID;
> +	} else {
> +		dev_err(dev, "Wrong CPU id associated with LMh node\n");
> +		return -EINVAL;
> +	}
> +
> +	platform_set_drvdata(pdev, lmh_data);

I don't see any get_drvdat(), so you can probably skip this?

> +
> +	if (!qcom_scm_lmh_dcvsh_available())
> +		return -EINVAL;
> +
> +	ret = qcom_scm_lmh_dcvsh(LMH_SUB_FN_CRNT, LMH_ALGO_MODE_ENABLE, 1,
> +				 LMH_NODE_DCVS, node_id, 0);
> +	if (ret)
> +		dev_err(dev, "Error %d enabling current subfunction\n", ret);
> +
> +	ret = qcom_scm_lmh_dcvsh(LMH_SUB_FN_REL, LMH_ALGO_MODE_ENABLE, 1,
> +				 LMH_NODE_DCVS, node_id, 0);
> +	if (ret)
> +		dev_err(dev, "Error %d enabling reliability subfunction\n", ret);
> +
> +	ret = qcom_scm_lmh_dcvsh(LMH_SUB_FN_BCL, LMH_ALGO_MODE_ENABLE, 1,
> +				 LMH_NODE_DCVS, node_id, 0);
> +	if (ret)
> +		dev_err(dev, "Error %d enabling BCL subfunction\n", ret);
> +
> +	ret = qcom_scm_lmh_dcvsh(LMH_SUB_FN_THERMAL, LMH_ALGO_MODE_ENABLE, 1,
> +				 LMH_NODE_DCVS, node_id, 0);
> +	if (ret) {
> +		dev_err(dev, "Error %d enabling thermal subfunction\n", ret);
> +		return ret;
> +	}
> +
> +	ret = qcom_scm_lmh_profile_change(0x1);
> +	if (ret) {
> +		dev_err(dev, "Error %d changing profile\n", ret);
> +		return ret;
> +	}
> +
> +	/* Set default thermal trips */
> +	ret = qcom_scm_lmh_dcvsh(LMH_SUB_FN_THERMAL, LMH_TH_ARM_THRESHOLD, temp_arm,
> +				 LMH_NODE_DCVS, node_id, 0);
> +	if (ret) {
> +		dev_err(dev, "Error setting thermal ARM threshold%d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = qcom_scm_lmh_dcvsh(LMH_SUB_FN_THERMAL, LMH_TH_HI_THRESHOLD, temp_high,
> +				 LMH_NODE_DCVS, node_id, 0);
> +	if (ret) {
> +		dev_err(dev, "Error setting thermal HI threshold%d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = qcom_scm_lmh_dcvsh(LMH_SUB_FN_THERMAL, LMH_TH_LOW_THRESHOLD, temp_low,
> +				 LMH_NODE_DCVS, node_id, 0);
> +	if (ret) {
> +		dev_err(dev, "Error setting thermal ARM threshold%d\n", ret);
> +		return ret;
> +	}
> +
> +	lmh_data->irq = platform_get_irq(pdev, 0);
> +	lmh_data->domain = irq_domain_add_linear(np, 1, &lmh_irq_ops, lmh_data);
> +	if (!lmh_data->domain) {
> +		dev_err(dev, "Error adding irq_domain\n");
> +		return -EINVAL;
> +	}
> +

As written now, you might get interrupts before you get to disable_irq()
below. Instead of the disable_irq() you can add this before request_irq:

	irq_set_status_flags(lmh_dat->irq, IRQ_NOAUTOEN);

> +	ret = devm_request_irq(dev, lmh_data->irq, lmh_handle_irq,
> +			       IRQF_TRIGGER_HIGH | IRQF_ONESHOT | IRQF_NO_SUSPEND,

Skip IRQF_TRIGGER_HIGH, as the flags will be merged with the properties
from DT.

> +			       "lmh-irq", lmh_data);
> +	if (ret) {
> +		dev_err(dev, "Error %d registering irq %x\n", ret, lmh_data->irq);
> +		irq_domain_remove(lmh_data->domain);
> +		return ret;
> +	}
> +
> +	/* Disable the irq and let cpufreq enable it when ready to handle the interrupt */
> +	disable_irq(lmh_data->irq);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id lmh_table[] = {
> +	{ .compatible = "qcom,sdm845-lmh", },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, lmh_table);
> +
> +static struct platform_driver lmh_driver = {
> +	.probe = lmh_probe,

I think you at least need to irq_domain_remove() during .remove, but
unless we have a clear understanding about how to stop the algorithm
(without causing harmful side effects) it might be better to add
.suppress_bind_attrs = true in .driver...

Regards,
Bjorn

> +	.driver = {
> +		.name = "qcom-lmh",
> +		.of_match_table = lmh_table,
> +	},
> +};
> +module_platform_driver(lmh_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("QCOM LMh driver");
> -- 
> 2.25.1
> 

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

* Re: [Patch v3 4/6] arm64: boot: dts: qcom: sdm45: Add support for LMh node
  2021-07-08 12:06 ` [Patch v3 4/6] arm64: boot: dts: qcom: sdm45: Add support for LMh node Thara Gopinath
@ 2021-07-10  4:17   ` Bjorn Andersson
  2021-07-19 16:33   ` Bjorn Andersson
  1 sibling, 0 replies; 29+ messages in thread
From: Bjorn Andersson @ 2021-07-10  4:17 UTC (permalink / raw)
  To: Thara Gopinath
  Cc: agross, rui.zhang, daniel.lezcano, viresh.kumar, rjw, robh+dt,
	tdas, mka, linux-arm-msm, linux-pm, linux-kernel, devicetree

On Thu 08 Jul 07:06 CDT 2021, Thara Gopinath wrote:

> Add LMh nodes for cpu cluster0 and cpu cluster1. Also add interrupt
> support in cpufreq node to capture the LMh interrupt and let the scheduler
> know of the max frequency throttling.
> 

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
> ---
> 
> v2->v3:
> 	- Changed the LMh low and high trip to 94500 and 95000 mC from
> 	  74500 and 75000 mC. This was a bug that got introduced in v2.
> v1->v2:
> 	- Dropped dt property qcom,support-lmh as per Bjorn's review comments.
> 	- Changed lmh compatible from generic to platform specific.
> 	- Introduced properties specifying arm, low and high temp thresholds for LMh
> 	  as per Daniel's suggestion.
> 
>  arch/arm64/boot/dts/qcom/sdm845.dtsi | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> index 0a86fe71a66d..4da6b8f3dd7b 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> @@ -3646,6 +3646,30 @@ swm: swm@c85 {
>  			};
>  		};
>  
> +		lmh_cluster1: lmh@17d70800 {
> +			compatible = "qcom,sdm845-lmh";
> +			reg = <0 0x17d70800 0 0x401>;
> +			interrupts = <GIC_SPI 33 IRQ_TYPE_LEVEL_HIGH>;
> +			qcom,lmh-cpu-id = <0x4>;
> +			qcom,lmh-temperature-arm = <65000>;
> +			qcom,lmh-temperature-low = <94500>;
> +			qcom,lmh-temperature-high = <95000>;
> +			interrupt-controller;
> +			#interrupt-cells = <1>;
> +		};
> +
> +		lmh_cluster0: lmh@17d78800 {
> +			compatible = "qcom,sdm845-lmh";
> +			reg = <0 0x17d78800 0 0x401>;
> +			interrupts = <GIC_SPI 32 IRQ_TYPE_LEVEL_HIGH>;
> +			qcom,lmh-cpu-id = <0x0>;
> +			qcom,lmh-temperature-arm = <65000>;
> +			qcom,lmh-temperature-low = <94500>;
> +			qcom,lmh-temperature-high = <95000>;
> +			interrupt-controller;
> +			#interrupt-cells = <1>;
> +		};
> +
>  		sound: sound {
>  		};
>  
> @@ -4911,6 +4935,8 @@ cpufreq_hw: cpufreq@17d43000 {
>  			reg = <0 0x17d43000 0 0x1400>, <0 0x17d45800 0 0x1400>;
>  			reg-names = "freq-domain0", "freq-domain1";
>  
> +			interrupts-extended = <&lmh_cluster0 0>, <&lmh_cluster1 0>;
> +
>  			clocks = <&rpmhcc RPMH_CXO_CLK>, <&gcc GPLL0>;
>  			clock-names = "xo", "alternate";
>  
> -- 
> 2.25.1
> 

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

* Re: [Patch v3 5/6] arm64: boot: dts: qcom: sdm845: Remove cpufreq cooling devices for CPU thermal zones
  2021-07-08 12:06 ` [Patch v3 5/6] arm64: boot: dts: qcom: sdm845: Remove cpufreq cooling devices for CPU thermal zones Thara Gopinath
@ 2021-07-10  4:17   ` Bjorn Andersson
  0 siblings, 0 replies; 29+ messages in thread
From: Bjorn Andersson @ 2021-07-10  4:17 UTC (permalink / raw)
  To: Thara Gopinath
  Cc: agross, rui.zhang, daniel.lezcano, viresh.kumar, rjw, robh+dt,
	tdas, mka, linux-arm-msm, linux-pm, linux-kernel, devicetree

On Thu 08 Jul 07:06 CDT 2021, Thara Gopinath wrote:

> Now that Limits h/w is enabled to monitor thermal events around cpus and
> throttle the cpu frequencies, remove cpufreq cooling device for the CPU
> thermal zones which does software throttling of cpu frequencies.
> 

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
> ---
> 
> v2->v3:
> 	- Improved the subject header and descrption to better reflect the
> 	  patch as per Matthias's review comments.
> 
> v1->v2:
> 	Removing only cooling maps for cpu specific thermal zones keeping the
> 	trip point definitions intact as per Daniel's suggestion. This is to
> 	ensure that thermal zone temparature and trip violation information is
> 	available to any userspace daemon monitoring these zones.
> 
>  arch/arm64/boot/dts/qcom/sdm845.dtsi | 136 ---------------------------
>  1 file changed, 136 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> index 4da6b8f3dd7b..6185fff8859a 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> @@ -4994,23 +4994,6 @@ cpu0_crit: cpu_crit {
>  					type = "critical";
>  				};
>  			};
> -
> -			cooling-maps {
> -				map0 {
> -					trip = <&cpu0_alert0>;
> -					cooling-device = <&CPU0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> -							 <&CPU1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> -							 <&CPU2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> -							 <&CPU3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
> -				};
> -				map1 {
> -					trip = <&cpu0_alert1>;
> -					cooling-device = <&CPU0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> -							 <&CPU1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> -							 <&CPU2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> -							 <&CPU3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
> -				};
> -			};
>  		};
>  
>  		cpu1-thermal {
> @@ -5038,23 +5021,6 @@ cpu1_crit: cpu_crit {
>  					type = "critical";
>  				};
>  			};
> -
> -			cooling-maps {
> -				map0 {
> -					trip = <&cpu1_alert0>;
> -					cooling-device = <&CPU0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> -							 <&CPU1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> -							 <&CPU2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> -							 <&CPU3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
> -				};
> -				map1 {
> -					trip = <&cpu1_alert1>;
> -					cooling-device = <&CPU0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> -							 <&CPU1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> -							 <&CPU2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> -							 <&CPU3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
> -				};
> -			};
>  		};
>  
>  		cpu2-thermal {
> @@ -5082,23 +5048,6 @@ cpu2_crit: cpu_crit {
>  					type = "critical";
>  				};
>  			};
> -
> -			cooling-maps {
> -				map0 {
> -					trip = <&cpu2_alert0>;
> -					cooling-device = <&CPU0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> -							 <&CPU1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> -							 <&CPU2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> -							 <&CPU3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
> -				};
> -				map1 {
> -					trip = <&cpu2_alert1>;
> -					cooling-device = <&CPU0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> -							 <&CPU1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> -							 <&CPU2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> -							 <&CPU3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
> -				};
> -			};
>  		};
>  
>  		cpu3-thermal {
> @@ -5126,23 +5075,6 @@ cpu3_crit: cpu_crit {
>  					type = "critical";
>  				};
>  			};
> -
> -			cooling-maps {
> -				map0 {
> -					trip = <&cpu3_alert0>;
> -					cooling-device = <&CPU0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> -							 <&CPU1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> -							 <&CPU2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> -							 <&CPU3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
> -				};
> -				map1 {
> -					trip = <&cpu3_alert1>;
> -					cooling-device = <&CPU0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> -							 <&CPU1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> -							 <&CPU2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> -							 <&CPU3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
> -				};
> -			};
>  		};
>  
>  		cpu4-thermal {
> @@ -5170,23 +5102,6 @@ cpu4_crit: cpu_crit {
>  					type = "critical";
>  				};
>  			};
> -
> -			cooling-maps {
> -				map0 {
> -					trip = <&cpu4_alert0>;
> -					cooling-device = <&CPU4 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> -							 <&CPU5 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> -							 <&CPU6 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> -							 <&CPU7 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
> -				};
> -				map1 {
> -					trip = <&cpu4_alert1>;
> -					cooling-device = <&CPU4 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> -							 <&CPU5 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> -							 <&CPU6 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> -							 <&CPU7 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
> -				};
> -			};
>  		};
>  
>  		cpu5-thermal {
> @@ -5214,23 +5129,6 @@ cpu5_crit: cpu_crit {
>  					type = "critical";
>  				};
>  			};
> -
> -			cooling-maps {
> -				map0 {
> -					trip = <&cpu5_alert0>;
> -					cooling-device = <&CPU4 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> -							 <&CPU5 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> -							 <&CPU6 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> -							 <&CPU7 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
> -				};
> -				map1 {
> -					trip = <&cpu5_alert1>;
> -					cooling-device = <&CPU4 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> -							 <&CPU5 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> -							 <&CPU6 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> -							 <&CPU7 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
> -				};
> -			};
>  		};
>  
>  		cpu6-thermal {
> @@ -5258,23 +5156,6 @@ cpu6_crit: cpu_crit {
>  					type = "critical";
>  				};
>  			};
> -
> -			cooling-maps {
> -				map0 {
> -					trip = <&cpu6_alert0>;
> -					cooling-device = <&CPU4 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> -							 <&CPU5 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> -							 <&CPU6 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> -							 <&CPU7 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
> -				};
> -				map1 {
> -					trip = <&cpu6_alert1>;
> -					cooling-device = <&CPU4 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> -							 <&CPU5 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> -							 <&CPU6 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> -							 <&CPU7 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
> -				};
> -			};
>  		};
>  
>  		cpu7-thermal {
> @@ -5302,23 +5183,6 @@ cpu7_crit: cpu_crit {
>  					type = "critical";
>  				};
>  			};
> -
> -			cooling-maps {
> -				map0 {
> -					trip = <&cpu7_alert0>;
> -					cooling-device = <&CPU4 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> -							 <&CPU5 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> -							 <&CPU6 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> -							 <&CPU7 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
> -				};
> -				map1 {
> -					trip = <&cpu7_alert1>;
> -					cooling-device = <&CPU4 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> -							 <&CPU5 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> -							 <&CPU6 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> -							 <&CPU7 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
> -				};
> -			};
>  		};
>  
>  		aoss0-thermal {
> -- 
> 2.25.1
> 

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

* Re: [Patch v3 6/6] dt-bindings: thermal: Add dt binding for QCOM LMh
  2021-07-08 12:06 ` [Patch v3 6/6] dt-bindings: thermal: Add dt binding for QCOM LMh Thara Gopinath
@ 2021-07-10  4:21   ` Bjorn Andersson
  2021-07-13  0:54     ` Thara Gopinath
  2021-07-12 17:32   ` Rob Herring
  1 sibling, 1 reply; 29+ messages in thread
From: Bjorn Andersson @ 2021-07-10  4:21 UTC (permalink / raw)
  To: Thara Gopinath
  Cc: agross, rui.zhang, daniel.lezcano, viresh.kumar, rjw, robh+dt,
	tdas, mka, linux-arm-msm, linux-pm, linux-kernel, devicetree

On Thu 08 Jul 07:06 CDT 2021, Thara Gopinath wrote:

> Add dt binding documentation to describe Qualcomm
> Limits Management Hardware node.
> 
> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
> ---
>  .../devicetree/bindings/thermal/qcom-lmh.yaml | 100 ++++++++++++++++++
>  1 file changed, 100 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/thermal/qcom-lmh.yaml
> 
> diff --git a/Documentation/devicetree/bindings/thermal/qcom-lmh.yaml b/Documentation/devicetree/bindings/thermal/qcom-lmh.yaml
> new file mode 100644
> index 000000000000..7f62bd3d543d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/thermal/qcom-lmh.yaml
> @@ -0,0 +1,100 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright 2021 Linaro Ltd.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/thermal/qcom-lmh.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm Limits Management Hardware(LMh)
> +
> +maintainers:
> +  - Thara Gopinath <thara.gopinath@linaro.org>
> +
> +description:
> +  Limits Management Hardware(LMh) is a hardware infrastructure on some
> +  Qualcomm SoCs that can enforce temperature and current limits as
> +  programmed by software for certain IPs like CPU.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - qcom,sdm845-lmh
> +
> +  reg:
> +    items:
> +      - description: core registers
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  '#interrupt-cells':
> +    const: 1
> +
> +  interrupt-controller: true
> +
> +  qcom,lmh-cpu-id:
> +    description:
> +      CPU id of the first cpu in the LMh cluster
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +
> +  qcom,lmh-temperature-arm:
> +    description:
> +      An integer expressing temperature threshold in millicelsius at which
> +      the LMh thermal FSM is engaged.

Do we know (by any public source) what "arm", "low" and "high" means
beyond that they somehow pokes the state machine?

> +    $ref: /schemas/types.yaml#/definitions/int32
> +
> +  qcom,lmh-temperature-low:
> +    description:
> +      An integer expressing temperature threshold in millicelsius at which
> +      the LMh thermal FSM is engaged.
> +    $ref: /schemas/types.yaml#/definitions/int32
> +
> +  qcom,lmh-temperature-high:
> +    description:
> +      An integer expressing temperature threshold in millicelsius at which
> +      the LMh thermal FSM is engaged.
> +    $ref: /schemas/types.yaml#/definitions/int32
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - #interrupt-cells
> +  - interrupt-controller
> +  - qcom,lmh-cpu-id
> +  - qcom,lmh-temperature-arm
> +  - qcom,lmh-temperature-low
> +  - qcom,lmh-temperature-high
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/clock/qcom,rpmh.h>
> +    #include <dt-bindings/interconnect/qcom,sdm845.h>

I don't see why you need qcom,rpmh.h or the interconnect include in this
example.

> +
> +    lmh_cluster1: lmh@17d70800 {
> +      compatible = "qcom,sdm845-lmh";
> +      reg = <0 0x17d70800 0 0x401>;

#address- and #size-cells are 1 in the wrapper that validates the
examples, so drop the two zeros.

> +      interrupts = <GIC_SPI 33 IRQ_TYPE_LEVEL_HIGH>;
> +      qcom,lmh-cpu-id = <0x4>;
> +      qcom,lmh-temperature-arm = <65000>;
> +      qcom,lmh-temperature-low = <94500>;
> +      qcom,lmh-temperature-high = <95000>;
> +      interrupt-controller;
> +      #interrupt-cells = <1>;
> +    };
> +  - |

This is a different example from the one above, if you intended that,
don't you need the #include of arm-gic.h here as well?

Regards,
Bjorn

> +    lmh_cluster0: lmh@17d78800 {
> +      compatible = "qcom,sdm845-lmh";
> +      reg = <0 0x17d78800 0 0x401>;
> +      interrupts = <GIC_SPI 32 IRQ_TYPE_LEVEL_HIGH>;
> +      qcom,lmh-cpu-id = <0x0>;
> +      qcom,lmh-temperature-arm = <65000>;
> +      qcom,lmh-temperature-low = <94500>;
> +      qcom,lmh-temperature-high = <95000>;
> +      interrupt-controller;
> +      #interrupt-cells = <1>;
> +    };
> +  - |
> -- 
> 2.25.1
> 

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

* Re: [Patch v3 3/6] cpufreq: qcom-cpufreq-hw: Add dcvs interrupt support
  2021-07-08 12:06 ` [Patch v3 3/6] cpufreq: qcom-cpufreq-hw: Add dcvs interrupt support Thara Gopinath
  2021-07-09  6:46   ` Viresh Kumar
@ 2021-07-10  4:57   ` Bjorn Andersson
  2021-07-13  1:09     ` Thara Gopinath
  1 sibling, 1 reply; 29+ messages in thread
From: Bjorn Andersson @ 2021-07-10  4:57 UTC (permalink / raw)
  To: Thara Gopinath
  Cc: agross, rui.zhang, daniel.lezcano, viresh.kumar, rjw, robh+dt,
	tdas, mka, linux-arm-msm, linux-pm, linux-kernel, devicetree

On Thu 08 Jul 07:06 CDT 2021, Thara Gopinath wrote:

> Add interrupt support to notify the kernel of h/w initiated frequency
> throttling by LMh. Convey this to scheduler via thermal presssure
> interface.
> 
> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
> ---
> 
> v2->v3:
> 	- Cosmetic fixes from review comments on the list.
> 	- Moved all LMh initializations to qcom_cpufreq_hw_lmh_init.
> 	- Added freeing of LMh interrupt and cancelling the polling worker to
> 	  qcom_cpufreq_hw_cpu_exit as per Viresh's suggestion.
> 	- LMh interrupts are now tied to cpu dev and not cpufreq dev. This will be
> 	  useful for further generation of SoCs where the same interrupt signals
> 	  multiple cpu clusters.
> 
> v1->v2:
> 	- Introduced qcom_cpufreq_hw_lmh_init to consolidate LMh related initializations
> 	  as per Viresh's review comment.
> 	- Moved the piece of code restarting polling/re-enabling LMh interrupt to
> 	  qcom_lmh_dcvs_notify therby simplifying isr and timer callback as per Viresh's
> 	  suggestion.
> 	- Droped cpus from qcom_cpufreq_data and instead using cpus from cpufreq_policy in
> 	  qcom_lmh_dcvs_notify as per Viresh's review comment.
> 	- Dropped dt property qcom,support-lmh as per Bjorn's suggestion.
> 	- Other minor/cosmetic fixes
> 
>  drivers/cpufreq/qcom-cpufreq-hw.c | 118 ++++++++++++++++++++++++++++++
>  1 file changed, 118 insertions(+)
> 
> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> index f86859bf76f1..bb5fc700d913 100644
> --- a/drivers/cpufreq/qcom-cpufreq-hw.c
> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> @@ -7,6 +7,7 @@
>  #include <linux/cpufreq.h>
>  #include <linux/init.h>
>  #include <linux/interconnect.h>
> +#include <linux/interrupt.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/of_address.h>
> @@ -22,10 +23,13 @@
>  #define CLK_HW_DIV			2
>  #define LUT_TURBO_IND			1
>  
> +#define HZ_PER_KHZ			1000
> +
>  struct qcom_cpufreq_soc_data {
>  	u32 reg_enable;
>  	u32 reg_freq_lut;
>  	u32 reg_volt_lut;
> +	u32 reg_current_vote;
>  	u32 reg_perf_state;
>  	u8 lut_row_size;
>  };
> @@ -33,7 +37,10 @@ struct qcom_cpufreq_soc_data {
>  struct qcom_cpufreq_data {
>  	void __iomem *base;
>  	struct resource *res;
> +	struct delayed_work lmh_dcvs_poll_work;

How about dropping "lmh" from this variable name?

Perhaps "throttle_work" or something like that?

>  	const struct qcom_cpufreq_soc_data *soc_data;
> +	struct cpufreq_policy *policy;
> +	int lmh_dcvs_irq;

throttle_irq ?

>  };
>  
>  static unsigned long cpu_hw_rate, xo_rate;
> @@ -251,10 +258,84 @@ static void qcom_get_related_cpus(int index, struct cpumask *m)
>  	}
>  }
>  
> +static inline unsigned long qcom_lmh_vote_to_freq(u32 val)
> +{
> +	return (val & 0x3FF) * 19200;
> +}
> +
> +static void qcom_lmh_dcvs_notify(struct qcom_cpufreq_data *data)
> +{
> +	struct cpufreq_policy *policy = data->policy;
> +	struct dev_pm_opp *opp;
> +	struct device *dev;
> +	unsigned long max_capacity, capacity, freq_hz, throttled_freq;
> +	unsigned int val, freq;
> +
> +	/*
> +	 * Get the h/w throttled frequency, normalize it using the
> +	 * registered opp table and use it to calculate thermal pressure.
> +	 */
> +	val = readl_relaxed(data->base + data->soc_data->reg_current_vote);

I would find it cleaner to move the readl() into the helper function, as
you don't care about the register value, only the resulting frequency.

> +	freq = qcom_lmh_vote_to_freq(val);
> +	freq_hz = freq * HZ_PER_KHZ;
> +
> +	dev = get_cpu_device(cpumask_first(policy->cpus));
> +	opp = dev_pm_opp_find_freq_floor(dev, &freq_hz);
> +	if (IS_ERR(opp) && PTR_ERR(opp) == -ERANGE)
> +		opp = dev_pm_opp_find_freq_ceil(dev, &freq_hz);
> +
> +	throttled_freq = freq_hz / HZ_PER_KHZ;
> +
> +	/* Update thermal pressure */
> +
> +	max_capacity = arch_scale_cpu_capacity(cpumask_first(policy->cpus));
> +	capacity = throttled_freq * max_capacity;
> +	capacity /= policy->cpuinfo.max_freq;

Perhaps, to avoid overflows if this is ever used on a 32-bit platform
use:

	mult_frac(max_capacity, throttled_freq, policy->cpuinfo.max_freq)

> +
> +	/* Don't pass boost capacity to scheduler */
> +	if (capacity > max_capacity)
> +		capacity = max_capacity;
> +
> +	arch_set_thermal_pressure(policy->cpus, max_capacity - capacity);
> +
> +	/*
> +	 * If h/w throttled frequency is higher than what cpufreq has requested for, stop
> +	 * polling and switch back to interrupt mechanism
> +	 */
> +
> +	if (throttled_freq >= qcom_cpufreq_hw_get(cpumask_first(policy->cpus)))
> +		/* Clear the existing interrupts and enable it back */
> +		enable_irq(data->lmh_dcvs_irq);
> +	else
> +		mod_delayed_work(system_highpri_wq, &data->lmh_dcvs_poll_work,
> +				 msecs_to_jiffies(10));
> +}
> +
> +static void qcom_lmh_dcvs_poll(struct work_struct *work)
> +{
> +	struct qcom_cpufreq_data *data;
> +
> +	data = container_of(work, struct qcom_cpufreq_data, lmh_dcvs_poll_work.work);
> +
> +	qcom_lmh_dcvs_notify(data);
> +}
> +
> +static irqreturn_t qcom_lmh_dcvs_handle_irq(int irq, void *data)
> +{
> +	struct qcom_cpufreq_data *c_data = data;
> +
> +	/* Disable interrupt and enable polling */
> +	disable_irq_nosync(c_data->lmh_dcvs_irq);
> +	qcom_lmh_dcvs_notify(c_data);
> +
> +	return 0;
> +}
> +
>  static const struct qcom_cpufreq_soc_data qcom_soc_data = {
>  	.reg_enable = 0x0,
>  	.reg_freq_lut = 0x110,
>  	.reg_volt_lut = 0x114,
> +	.reg_current_vote = 0x704,
>  	.reg_perf_state = 0x920,
>  	.lut_row_size = 32,
>  };
> @@ -274,6 +355,35 @@ static const struct of_device_id qcom_cpufreq_hw_match[] = {
>  };
>  MODULE_DEVICE_TABLE(of, qcom_cpufreq_hw_match);
>  
> +static int qcom_cpufreq_hw_lmh_init(struct cpufreq_policy *policy, int index)
> +{
> +	struct qcom_cpufreq_data *data = policy->driver_data;
> +	struct platform_device *pdev = cpufreq_get_driver_data();
> +	struct device *cpu_dev = get_cpu_device(policy->cpu);
> +	char irq_name[15];
> +	int ret;
> +
> +	/*
> +	 * Look for LMh interrupt. If no interrupt line is specified /
> +	 * if there is an error, allow cpufreq to be enabled as usual.
> +	 */
> +	data->lmh_dcvs_irq = platform_get_irq(pdev, index);
> +	if (data->lmh_dcvs_irq <= 0)
> +		return data->lmh_dcvs_irq == -EPROBE_DEFER ? -EPROBE_DEFER : 0;
> +
> +	snprintf(irq_name, sizeof(irq_name), "dcvsh-irq-%u", policy->cpu);
> +	ret = devm_request_irq(cpu_dev, data->lmh_dcvs_irq, qcom_lmh_dcvs_handle_irq,
> +			       0, irq_name, data);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Error %d registering irq %x\n", ret, data->lmh_dcvs_irq);

The irq number here won't have any meaning, and %x wouldn't be suitable.

How about ..."Error registering %s: %d\n", irq_name, ret); ?

> +		return 0;

This sounds like a problem, wouldn't it be suitable to treat it as a
problem?

> +	}
> +	data->policy = policy;

Afaict, no one is going to access data->policy unless devm_request_irq()
succeeds and if it does and the interrupt fires immediately it would be
too late to set it here. So better move it earlier.

> +	INIT_DEFERRABLE_WORK(&data->lmh_dcvs_poll_work, qcom_lmh_dcvs_poll);

What if the interrupt fires before you initialize the work? Better move
this higher up.

> +
> +	return 0;
> +}
> +
>  static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
>  {
>  	struct platform_device *pdev = cpufreq_get_driver_data();
> @@ -370,6 +480,10 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
>  			dev_warn(cpu_dev, "failed to enable boost: %d\n", ret);
>  	}
>  
> +	ret = qcom_cpufreq_hw_lmh_init(policy, index);
> +	if (ret)
> +		goto error;
> +
>  	return 0;
>  error:
>  	kfree(data);
> @@ -389,6 +503,10 @@ static int qcom_cpufreq_hw_cpu_exit(struct cpufreq_policy *policy)
>  
>  	dev_pm_opp_remove_all_dynamic(cpu_dev);
>  	dev_pm_opp_of_cpumask_remove_table(policy->related_cpus);
> +	if (data->lmh_dcvs_irq > 0) {
> +		devm_free_irq(cpu_dev, data->lmh_dcvs_irq, data);

As init/exit are called multiple times you should avoid the devm
variants.

Regards,
Bjorn

> +		cancel_delayed_work_sync(&data->lmh_dcvs_poll_work);
> +	}
>  	kfree(policy->freq_table);
>  	kfree(data);
>  	iounmap(base);
> -- 
> 2.25.1
> 

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

* Re: [Patch v3 3/6] cpufreq: qcom-cpufreq-hw: Add dcvs interrupt support
  2021-07-09 15:37     ` Thara Gopinath
@ 2021-07-12  4:35       ` Viresh Kumar
  2021-07-12  4:41       ` Viresh Kumar
  1 sibling, 0 replies; 29+ messages in thread
From: Viresh Kumar @ 2021-07-12  4:35 UTC (permalink / raw)
  To: Thara Gopinath, Lukasz Luba
  Cc: agross, bjorn.andersson, rui.zhang, daniel.lezcano, rjw, robh+dt,
	tdas, mka, linux-arm-msm, linux-pm, linux-kernel, devicetree

+Lukasz,

On 09-07-21, 11:37, Thara Gopinath wrote:
> On 7/9/21 2:46 AM, Viresh Kumar wrote:
> > On 08-07-21, 08:06, Thara Gopinath wrote:
> > >   static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
> > >   {
> > >   	struct platform_device *pdev = cpufreq_get_driver_data();
> > > @@ -370,6 +480,10 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
> > >   			dev_warn(cpu_dev, "failed to enable boost: %d\n", ret);
> > >   	}
> > > +	ret = qcom_cpufreq_hw_lmh_init(policy, index);
> > 
> > You missed unregistering EM here (which is also missing from exit,
> > which you need to fix first in a separate patch).
> 
> Hi!
> 
> So how exactly do you do this? I checked other users of the api and I do not
> see any free.

Lukasz,

I don't see the cpufreq drivers ever calling dev_pm_opp_of_unregister_em(), and
even if they called, it would translate to em_dev_unregister_perf_domain(),
which has this:

	if (_is_cpu_device(dev))
		return;

I am not sure what's going on here, can you help ?

-- 
viresh

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

* Re: [Patch v3 3/6] cpufreq: qcom-cpufreq-hw: Add dcvs interrupt support
  2021-07-09 15:37     ` Thara Gopinath
  2021-07-12  4:35       ` Viresh Kumar
@ 2021-07-12  4:41       ` Viresh Kumar
  2021-07-13  1:18         ` Thara Gopinath
  1 sibling, 1 reply; 29+ messages in thread
From: Viresh Kumar @ 2021-07-12  4:41 UTC (permalink / raw)
  To: Thara Gopinath
  Cc: agross, bjorn.andersson, rui.zhang, daniel.lezcano, rjw, robh+dt,
	tdas, mka, linux-arm-msm, linux-pm, linux-kernel, devicetree

On 09-07-21, 11:37, Thara Gopinath wrote:
> On 7/9/21 2:46 AM, Viresh Kumar wrote:
> > > @@ -389,6 +503,10 @@ static int qcom_cpufreq_hw_cpu_exit(struct cpufreq_policy *policy)
> > >   	dev_pm_opp_remove_all_dynamic(cpu_dev);
> > >   	dev_pm_opp_of_cpumask_remove_table(policy->related_cpus);
> > > +	if (data->lmh_dcvs_irq > 0) {
> > > +		devm_free_irq(cpu_dev, data->lmh_dcvs_irq, data);
> > 
> > Why using devm variants here and while requesting the irq ?

Missed this one ?

> > 
> > > +		cancel_delayed_work_sync(&data->lmh_dcvs_poll_work);
> > > +	}
> > 
> > Please move this to qcom_cpufreq_hw_lmh_exit() or something.
> 
> Ok.
> 
> > 
> > Now with sequence of disabling interrupt, etc, I see a potential
> > problem.
> > 
> > CPU0                                    CPU1
> > 
> > qcom_cpufreq_hw_cpu_exit()
> > -> devm_free_irq();
> >                                          qcom_lmh_dcvs_poll()
> >                                          -> qcom_lmh_dcvs_notify()
> >                                            -> enable_irq()
> > 
> > -> cancel_delayed_work_sync();
> > 
> > 
> > What will happen if enable_irq() gets called after freeing the irq ?
> > Not sure, but it looks like you will hit this then from manage.c:
> > 
> > WARN(!desc->irq_data.chip, KERN_ERR "enable_irq before
> >                                       setup/request_irq: irq %u\n", irq))
> > 
> > ?
> > 
> > You got a chicken n egg problem :)
> 
> Yes indeed! But also it is a very rare chicken and egg problem.
> The scenario here is that the cpus are busy and running load causing a
> thermal overrun and lmh is engaged. At the same time for this issue to be
> hit the cpu is trying to exit/disable cpufreq.

Yes, it is a very specific case but it needs to be resolved anyway. You don't
want to get this ever :)

> Calling
> cancel_delayed_work_sync first could solve this issue, right ?
> cancel_delayed_work_sync guarantees the work not to be pending even if
> it requeues itself on return. So once the delayed work is cancelled, the
> interrupts can be safely disabled. Thoughts ?

I don't think even that would provide such guarantees to you here, as there is
a chance the work gets queued again because of an interrupt that triggers right
after you cancel the work.

The basic way of solving such issues is that once you cancel something, you need
to guarantee that it doesn't get triggered again, no matter what.

The problem here I see is with your design itself, both delayed work and irq can
enable each other, so no matter which one you disable first, won't be
sufficient. You need to fix that design somehow.

-- 
viresh

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

* Re: [Patch v3 6/6] dt-bindings: thermal: Add dt binding for QCOM LMh
  2021-07-08 12:06 ` [Patch v3 6/6] dt-bindings: thermal: Add dt binding for QCOM LMh Thara Gopinath
  2021-07-10  4:21   ` Bjorn Andersson
@ 2021-07-12 17:32   ` Rob Herring
  1 sibling, 0 replies; 29+ messages in thread
From: Rob Herring @ 2021-07-12 17:32 UTC (permalink / raw)
  To: Thara Gopinath
  Cc: agross, bjorn.andersson, rui.zhang, daniel.lezcano, viresh.kumar,
	rjw, tdas, mka, linux-arm-msm, linux-pm, linux-kernel,
	devicetree

On Thu, Jul 08, 2021 at 08:06:56AM -0400, Thara Gopinath wrote:
> Add dt binding documentation to describe Qualcomm
> Limits Management Hardware node.
> 
> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
> ---
>  .../devicetree/bindings/thermal/qcom-lmh.yaml | 100 ++++++++++++++++++
>  1 file changed, 100 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/thermal/qcom-lmh.yaml
> 
> diff --git a/Documentation/devicetree/bindings/thermal/qcom-lmh.yaml b/Documentation/devicetree/bindings/thermal/qcom-lmh.yaml
> new file mode 100644
> index 000000000000..7f62bd3d543d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/thermal/qcom-lmh.yaml
> @@ -0,0 +1,100 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright 2021 Linaro Ltd.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/thermal/qcom-lmh.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm Limits Management Hardware(LMh)
> +
> +maintainers:
> +  - Thara Gopinath <thara.gopinath@linaro.org>
> +
> +description:
> +  Limits Management Hardware(LMh) is a hardware infrastructure on some
> +  Qualcomm SoCs that can enforce temperature and current limits as
> +  programmed by software for certain IPs like CPU.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - qcom,sdm845-lmh
> +
> +  reg:
> +    items:
> +      - description: core registers
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  '#interrupt-cells':
> +    const: 1
> +
> +  interrupt-controller: true
> +
> +  qcom,lmh-cpu-id:
> +    description:
> +      CPU id of the first cpu in the LMh cluster
> +    $ref: /schemas/types.yaml#/definitions/uint32

The way we reference other nodes in DT is phandles. 'cpus' is already 
somewhat established for this case.

> +
> +  qcom,lmh-temperature-arm:
> +    description:
> +      An integer expressing temperature threshold in millicelsius at which

Use unit suffix when you have units.

> +      the LMh thermal FSM is engaged.
> +    $ref: /schemas/types.yaml#/definitions/int32
> +
> +  qcom,lmh-temperature-low:
> +    description:
> +      An integer expressing temperature threshold in millicelsius at which
> +      the LMh thermal FSM is engaged.
> +    $ref: /schemas/types.yaml#/definitions/int32
> +
> +  qcom,lmh-temperature-high:
> +    description:
> +      An integer expressing temperature threshold in millicelsius at which
> +      the LMh thermal FSM is engaged.

What's the difference in the 3 properties because the description is the 
same.

> +    $ref: /schemas/types.yaml#/definitions/int32
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - #interrupt-cells
> +  - interrupt-controller
> +  - qcom,lmh-cpu-id
> +  - qcom,lmh-temperature-arm
> +  - qcom,lmh-temperature-low
> +  - qcom,lmh-temperature-high
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/clock/qcom,rpmh.h>
> +    #include <dt-bindings/interconnect/qcom,sdm845.h>
> +
> +    lmh_cluster1: lmh@17d70800 {
> +      compatible = "qcom,sdm845-lmh";
> +      reg = <0 0x17d70800 0 0x401>;
> +      interrupts = <GIC_SPI 33 IRQ_TYPE_LEVEL_HIGH>;
> +      qcom,lmh-cpu-id = <0x4>;
> +      qcom,lmh-temperature-arm = <65000>;
> +      qcom,lmh-temperature-low = <94500>;
> +      qcom,lmh-temperature-high = <95000>;
> +      interrupt-controller;

What devices is this an interrupt controller for?

> +      #interrupt-cells = <1>;
> +    };
> +  - |
> +    lmh_cluster0: lmh@17d78800 {
> +      compatible = "qcom,sdm845-lmh";
> +      reg = <0 0x17d78800 0 0x401>;
> +      interrupts = <GIC_SPI 32 IRQ_TYPE_LEVEL_HIGH>;
> +      qcom,lmh-cpu-id = <0x0>;
> +      qcom,lmh-temperature-arm = <65000>;
> +      qcom,lmh-temperature-low = <94500>;
> +      qcom,lmh-temperature-high = <95000>;
> +      interrupt-controller;
> +      #interrupt-cells = <1>;
> +    };
> +  - |
> -- 
> 2.25.1
> 
> 

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

* Re: [Patch v3 2/6] thermal: qcom: Add support for LMh driver
  2021-07-10  4:15   ` Bjorn Andersson
@ 2021-07-13  0:49     ` Thara Gopinath
  0 siblings, 0 replies; 29+ messages in thread
From: Thara Gopinath @ 2021-07-13  0:49 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: agross, rui.zhang, daniel.lezcano, viresh.kumar, rjw, robh+dt,
	tdas, mka, linux-arm-msm, linux-pm, linux-kernel, devicetree

Hi Bjorn,

Thanks for the review

On 7/10/21 12:15 AM, Bjorn Andersson wrote:
> On Thu 08 Jul 07:06 CDT 2021, Thara Gopinath wrote:
> 
>> Driver enabling various pieces of Limits Management Hardware(LMh) for cpu
>> cluster0 and cpu cluster1 namely kick starting monitoring of temperature,
>> current, battery current violations, enabling reliability algorithm and
>> setting up various temperature limits.
>>
>> The following has been explained in the cover letter. I am including this
>> here so that this remains in the commit message as well.
>>
>> LMh is a hardware infrastructure on some Qualcomm SoCs that can enforce
>> temperature and current limits as programmed by software for certain IPs
>> like CPU. On many newer LMh is configured by firmware/TZ and no programming
>> is needed from the kernel side. But on certain SoCs like sdm845 the
>> firmware does not do a complete programming of the h/w. On such soc's
>> kernel software has to explicitly set up the temperature limits and turn on
>> various monitoring and enforcing algorithms on the hardware.
>>
>> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
>> ---
>>
>> v2->v3:
>> 	- Rearranged enabling of various LMh subfunction and removed returning
>> 	  on error in enabling any one subfunction as the different pieces can
>> 	  operate and thus be enabled independently.
>> 	- Other minor cosmetic fixes.
>>
>> v1->v2:
>> 	- Cosmetic and spelling fixes from review comments from Randy Dunlap
>> 	- Added irq_disable to lmh_irq_ops and removed disabling of irq from
>> 	  lmh_handle_irq. Now cpufreq explicitly disables irq prior to
>> 	  handling it as per Bjorn's suggestion.
>> 	- Rebased to new version of qcom_scm_lmh_dcvsh as changed in patch 1.
>> 	- Removed generic dt compatible string and introduced platform specific one
>> 	  as per Bjorn's suggestion.
>> 	- Take arm, low and high temp thresholds for LMh from dt properties instead of
>> 	  #defines in the driver as per Daniel's suggestion.
>> 	- Other minor fixes.
>>   drivers/thermal/qcom/Kconfig  |  10 ++
>>   drivers/thermal/qcom/Makefile |   1 +
>>   drivers/thermal/qcom/lmh.c    | 239 ++++++++++++++++++++++++++++++++++
>>   3 files changed, 250 insertions(+)
>>   create mode 100644 drivers/thermal/qcom/lmh.c
>>
>> diff --git a/drivers/thermal/qcom/Kconfig b/drivers/thermal/qcom/Kconfig
>> index 8d5ac2df26dc..7d942f71e532 100644
>> --- a/drivers/thermal/qcom/Kconfig
>> +++ b/drivers/thermal/qcom/Kconfig
>> @@ -31,3 +31,13 @@ config QCOM_SPMI_TEMP_ALARM
>>   	  trip points. The temperature reported by the thermal sensor reflects the
>>   	  real time die temperature if an ADC is present or an estimate of the
>>   	  temperature based upon the over temperature stage value.
>> +
>> +config QCOM_LMH
>> +	tristate "Qualcomm Limits Management Hardware"
>> +	depends on ARCH_QCOM
>> +	help
>> +	  This enables initialization of Qualcomm limits management
>> +	  hardware(LMh). LMh allows for hardware-enforced mitigation for cpus based on
>> +	  input from temperature and current sensors.  On many newer Qualcomm SoCs
>> +	  LMh is configured in the firmware and this feature need not be enabled.
>> +	  However, on certain SoCs like sdm845 LMh has to be configured from kernel.
>> diff --git a/drivers/thermal/qcom/Makefile b/drivers/thermal/qcom/Makefile
>> index 252ea7d9da0b..0fa2512042e7 100644
>> --- a/drivers/thermal/qcom/Makefile
>> +++ b/drivers/thermal/qcom/Makefile
>> @@ -5,3 +5,4 @@ qcom_tsens-y			+= tsens.o tsens-v2.o tsens-v1.o tsens-v0_1.o \
>>   				   tsens-8960.o
>>   obj-$(CONFIG_QCOM_SPMI_ADC_TM5)	+= qcom-spmi-adc-tm5.o
>>   obj-$(CONFIG_QCOM_SPMI_TEMP_ALARM)	+= qcom-spmi-temp-alarm.o
>> +obj-$(CONFIG_QCOM_LMH)		+= lmh.o
>> diff --git a/drivers/thermal/qcom/lmh.c b/drivers/thermal/qcom/lmh.c
>> new file mode 100644
>> index 000000000000..a7b1eb308642
>> --- /dev/null
>> +++ b/drivers/thermal/qcom/lmh.c
>> @@ -0,0 +1,239 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +
>> +/*
>> + * Copyright (C) 2021, Linaro Limited. All rights reserved.
>> + */
>> +#include <linux/module.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/irqdomain.h>
>> +#include <linux/err.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/slab.h>
>> +#include <linux/qcom_scm.h>
>> +
>> +#define LMH_NODE_DCVS			0x44435653
>> +#define LMH_CLUSTER0_NODE_ID		0x6370302D
>> +#define LMH_CLUSTER1_NODE_ID		0x6370312D
>> +
>> +#define LMH_SUB_FN_THERMAL		0x54484D4C
>> +#define LMH_SUB_FN_CRNT			0x43524E54
>> +#define LMH_SUB_FN_REL			0x52454C00
>> +#define LMH_SUB_FN_BCL			0x42434C00
>> +
>> +#define LMH_ALGO_MODE_ENABLE		0x454E424C
>> +#define LMH_TH_HI_THRESHOLD		0x48494748
>> +#define LMH_TH_LOW_THRESHOLD		0x4C4F5700
>> +#define LMH_TH_ARM_THRESHOLD		0x41524D00
>> +
>> +#define LMH_REG_DCVS_INTR_CLR		0x8
>> +
>> +struct lmh_hw_data {
>> +	void __iomem *base;
>> +	struct irq_domain *domain;
>> +	int irq;
>> +	u32 cpu_id;
> 
> cpu_id seems to only be used in lmh_probe(), how about making it a local
> variable?

yes, it makes sense. I will make it local.

> 
>> +};
>> +
>> +static irqreturn_t lmh_handle_irq(int hw_irq, void *data)
>> +{
>> +	struct lmh_hw_data *lmh_data = data;
>> +	int irq = irq_find_mapping(lmh_data->domain, 0);
>> +
>> +	/* Call the cpufreq driver to handle the interrupt */
>> +	if (irq)
>> +		generic_handle_irq(irq);
>> +
>> +	return 0;
>> +}
>> +
>> +static void lmh_enable_interrupt(struct irq_data *d)
>> +{
>> +	struct lmh_hw_data *lmh_data = irq_data_get_irq_chip_data(d);
>> +
>> +	/* Clear the existing interrupt */
>> +	writel(0xff, lmh_data->base + LMH_REG_DCVS_INTR_CLR);
>> +	enable_irq(lmh_data->irq);
>> +}
>> +
>> +static void lmh_disable_interrupt(struct irq_data *d)
>> +{
>> +	struct lmh_hw_data *lmh_data = irq_data_get_irq_chip_data(d);
>> +
>> +	disable_irq_nosync(lmh_data->irq);
>> +}
>> +
>> +static struct irq_chip lmh_irq_chip = {
>> +	.name           = "lmh",
>> +	.irq_enable	= lmh_enable_interrupt,
>> +	.irq_disable	= lmh_disable_interrupt
>> +};
>> +
>> +static int lmh_irq_map(struct irq_domain *d, unsigned int irq, irq_hw_number_t hw)
>> +{
>> +	struct lmh_hw_data *lmh_data = d->host_data;
>> +
>> +	irq_set_chip_and_handler(irq, &lmh_irq_chip, handle_simple_irq);
>> +	irq_set_chip_data(irq, lmh_data);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct irq_domain_ops lmh_irq_ops = {
>> +	.map = lmh_irq_map,
>> +	.xlate = irq_domain_xlate_onecell,
>> +};
>> +
>> +static int lmh_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev;
>> +	struct device_node *np;
>> +	struct lmh_hw_data *lmh_data;
>> +	u32 node_id;
>> +	int temp_low, temp_high, temp_arm, ret;
>> +
>> +	dev = &pdev->dev;
>> +	np = dev->of_node;
> 
> How about initialize these as you declare you variables?

ok.

> 
>> +	if (!np)
> 
> There's no reasonable way to probe this driver with !dev->of_node, so
> you can skip this check.

ok.

> 
>> +		return -EINVAL;
>> +
>> +	lmh_data = devm_kzalloc(dev, sizeof(*lmh_data), GFP_KERNEL);
>> +	if (!lmh_data)
>> +		return -ENOMEM;
>> +
>> +	lmh_data->base = devm_platform_ioremap_resource(pdev, 0);
>> +	if (IS_ERR(lmh_data->base))
>> +		return PTR_ERR(lmh_data->base);
>> +
>> +	ret = of_property_read_u32(np, "qcom,lmh-cpu-id", &lmh_data->cpu_id);
>> +	if (ret) {
>> +		dev_err(dev, "missing qcom,lmh-cpu-id property\n");
>> +		return ret;
>> +	}
>> +
>> +	ret = of_property_read_u32(np, "qcom,lmh-temperature-high", &temp_high);
>> +	if (ret) {
>> +		dev_err(dev, "missing qcom,lmh-temperature-high property\n");
>> +		return ret;
>> +	}
>> +
>> +	ret = of_property_read_u32(np, "qcom,lmh-temperature-low", &temp_low);
>> +	if (ret) {
>> +		dev_err(dev, "missing qcom,lmh-temperature-low property\n");
>> +		return ret;
>> +	}
>> +
>> +	ret = of_property_read_u32(np, "qcom,lmh-temperature-arm", &temp_arm);
>> +	if (ret) {
>> +		dev_err(dev, "missing qcom,lmh-temperature-arm property\n");
>> +		return ret;
>> +	}
>> +
>> +	/*
>> +	 * Only sdm845 has lmh hardware currently enabled from hlos. If this is needed
>> +	 * for other platforms, revisit this to check if the <cpu-id, node-id> should be part
>> +	 * of a dt match table.
>> +	 */
>> +	if (lmh_data->cpu_id == 0) {
>> +		node_id = LMH_CLUSTER0_NODE_ID;
>> +	} else if (lmh_data->cpu_id == 4) {
>> +		node_id = LMH_CLUSTER1_NODE_ID;
>> +	} else {
>> +		dev_err(dev, "Wrong CPU id associated with LMh node\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	platform_set_drvdata(pdev, lmh_data);
> 
> I don't see any get_drvdat(), so you can probably skip this?

Yes. I will remove it. I think it is  stray remaining from one of the 
earlier revisions.

> 
>> +
>> +	if (!qcom_scm_lmh_dcvsh_available())
>> +		return -EINVAL;
>> +
>> +	ret = qcom_scm_lmh_dcvsh(LMH_SUB_FN_CRNT, LMH_ALGO_MODE_ENABLE, 1,
>> +				 LMH_NODE_DCVS, node_id, 0);
>> +	if (ret)
>> +		dev_err(dev, "Error %d enabling current subfunction\n", ret);
>> +
>> +	ret = qcom_scm_lmh_dcvsh(LMH_SUB_FN_REL, LMH_ALGO_MODE_ENABLE, 1,
>> +				 LMH_NODE_DCVS, node_id, 0);
>> +	if (ret)
>> +		dev_err(dev, "Error %d enabling reliability subfunction\n", ret);
>> +
>> +	ret = qcom_scm_lmh_dcvsh(LMH_SUB_FN_BCL, LMH_ALGO_MODE_ENABLE, 1,
>> +				 LMH_NODE_DCVS, node_id, 0);
>> +	if (ret)
>> +		dev_err(dev, "Error %d enabling BCL subfunction\n", ret);
>> +
>> +	ret = qcom_scm_lmh_dcvsh(LMH_SUB_FN_THERMAL, LMH_ALGO_MODE_ENABLE, 1,
>> +				 LMH_NODE_DCVS, node_id, 0);
>> +	if (ret) {
>> +		dev_err(dev, "Error %d enabling thermal subfunction\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	ret = qcom_scm_lmh_profile_change(0x1);
>> +	if (ret) {
>> +		dev_err(dev, "Error %d changing profile\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	/* Set default thermal trips */
>> +	ret = qcom_scm_lmh_dcvsh(LMH_SUB_FN_THERMAL, LMH_TH_ARM_THRESHOLD, temp_arm,
>> +				 LMH_NODE_DCVS, node_id, 0);
>> +	if (ret) {
>> +		dev_err(dev, "Error setting thermal ARM threshold%d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	ret = qcom_scm_lmh_dcvsh(LMH_SUB_FN_THERMAL, LMH_TH_HI_THRESHOLD, temp_high,
>> +				 LMH_NODE_DCVS, node_id, 0);
>> +	if (ret) {
>> +		dev_err(dev, "Error setting thermal HI threshold%d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	ret = qcom_scm_lmh_dcvsh(LMH_SUB_FN_THERMAL, LMH_TH_LOW_THRESHOLD, temp_low,
>> +				 LMH_NODE_DCVS, node_id, 0);
>> +	if (ret) {
>> +		dev_err(dev, "Error setting thermal ARM threshold%d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	lmh_data->irq = platform_get_irq(pdev, 0);
>> +	lmh_data->domain = irq_domain_add_linear(np, 1, &lmh_irq_ops, lmh_data);
>> +	if (!lmh_data->domain) {
>> +		dev_err(dev, "Error adding irq_domain\n");
>> +		return -EINVAL;
>> +	}
>> +
> 
> As written now, you might get interrupts before you get to disable_irq()
> below. Instead of the disable_irq() you can add this before request_irq:
> 
> 	irq_set_status_flags(lmh_dat->irq, IRQ_NOAUTOEN);
> 
>> +	ret = devm_request_irq(dev, lmh_data->irq, lmh_handle_irq,
>> +			       IRQF_TRIGGER_HIGH | IRQF_ONESHOT | IRQF_NO_SUSPEND,
> 
> Skip IRQF_TRIGGER_HIGH, as the flags will be merged with the properties
> from DT.
> 
>> +			       "lmh-irq", lmh_data);
>> +	if (ret) {
>> +		dev_err(dev, "Error %d registering irq %x\n", ret, lmh_data->irq);
>> +		irq_domain_remove(lmh_data->domain);
>> +		return ret;
>> +	}
>> +
>> +	/* Disable the irq and let cpufreq enable it when ready to handle the interrupt */
>> +	disable_irq(lmh_data->irq);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id lmh_table[] = {
>> +	{ .compatible = "qcom,sdm845-lmh", },
>> +	{}
>> +};
>> +MODULE_DEVICE_TABLE(of, lmh_table);
>> +
>> +static struct platform_driver lmh_driver = {
>> +	.probe = lmh_probe,
> 
> I think you at least need to irq_domain_remove() during .remove, but
> unless we have a clear understanding about how to stop the algorithm
> (without causing harmful side effects) it might be better to add
> .suppress_bind_attrs = true in .driver...

sounds good. Like you said, I am not sure what is the right way to 
disable the algorithm. So I will add suppress_bind_attrs = true to 
prevent user space from doing something silly.

-- 
Warm Regards
Thara (She/Her/Hers)

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

* Re: [Patch v3 6/6] dt-bindings: thermal: Add dt binding for QCOM LMh
  2021-07-10  4:21   ` Bjorn Andersson
@ 2021-07-13  0:54     ` Thara Gopinath
  0 siblings, 0 replies; 29+ messages in thread
From: Thara Gopinath @ 2021-07-13  0:54 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: agross, rui.zhang, daniel.lezcano, viresh.kumar, rjw, robh+dt,
	tdas, mka, linux-arm-msm, linux-pm, linux-kernel, devicetree



On 7/10/21 12:21 AM, Bjorn Andersson wrote:
> On Thu 08 Jul 07:06 CDT 2021, Thara Gopinath wrote:
> 
>> Add dt binding documentation to describe Qualcomm
>> Limits Management Hardware node.
>>
>> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
>> ---
>>   .../devicetree/bindings/thermal/qcom-lmh.yaml | 100 ++++++++++++++++++
>>   1 file changed, 100 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/thermal/qcom-lmh.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/thermal/qcom-lmh.yaml b/Documentation/devicetree/bindings/thermal/qcom-lmh.yaml
>> new file mode 100644
>> index 000000000000..7f62bd3d543d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/thermal/qcom-lmh.yaml
>> @@ -0,0 +1,100 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +# Copyright 2021 Linaro Ltd.
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/thermal/qcom-lmh.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Qualcomm Limits Management Hardware(LMh)
>> +
>> +maintainers:
>> +  - Thara Gopinath <thara.gopinath@linaro.org>
>> +
>> +description:
>> +  Limits Management Hardware(LMh) is a hardware infrastructure on some
>> +  Qualcomm SoCs that can enforce temperature and current limits as
>> +  programmed by software for certain IPs like CPU.
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - qcom,sdm845-lmh
>> +
>> +  reg:
>> +    items:
>> +      - description: core registers
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  '#interrupt-cells':
>> +    const: 1
>> +
>> +  interrupt-controller: true
>> +
>> +  qcom,lmh-cpu-id:
>> +    description:
>> +      CPU id of the first cpu in the LMh cluster
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +
>> +  qcom,lmh-temperature-arm:
>> +    description:
>> +      An integer expressing temperature threshold in millicelsius at which
>> +      the LMh thermal FSM is engaged.
> 
> Do we know (by any public source) what "arm", "low" and "high" means
> beyond that they somehow pokes the state machine?

Not from public documentation. I know what these thresholds means, 
atleast to some extent. Though I will never claim to be an expert in 
this! There is an error in description of qcom,lmh-temperature-low and 
qcom,lmh-temperature-high below. I copied
and forgot to change the description. I will fix it.

> 
>> +    $ref: /schemas/types.yaml#/definitions/int32
>> +
>> +  qcom,lmh-temperature-low:
>> +    description:
>> +      An integer expressing temperature threshold in millicelsius at which
>> +      the LMh thermal FSM is engaged.
>> +    $ref: /schemas/types.yaml#/definitions/int32
>> +
>> +  qcom,lmh-temperature-high:
>> +    description:
>> +      An integer expressing temperature threshold in millicelsius at which
>> +      the LMh thermal FSM is engaged.
>> +    $ref: /schemas/types.yaml#/definitions/int32
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - interrupts
>> +  - #interrupt-cells
>> +  - interrupt-controller
>> +  - qcom,lmh-cpu-id
>> +  - qcom,lmh-temperature-arm
>> +  - qcom,lmh-temperature-low
>> +  - qcom,lmh-temperature-high
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>> +    #include <dt-bindings/clock/qcom,rpmh.h>
>> +    #include <dt-bindings/interconnect/qcom,sdm845.h>
> 
> I don't see why you need qcom,rpmh.h or the interconnect include in this
> example.

I could have sworn make dt-bindings check failed. But maybe only The 
first include is needed. I will remove the other two.

> 
>> +
>> +    lmh_cluster1: lmh@17d70800 {
>> +      compatible = "qcom,sdm845-lmh";
>> +      reg = <0 0x17d70800 0 0x401>;
> 
> #address- and #size-cells are 1 in the wrapper that validates the
> examples, so drop the two zeros.

Ok.

> 
>> +      interrupts = <GIC_SPI 33 IRQ_TYPE_LEVEL_HIGH>;
>> +      qcom,lmh-cpu-id = <0x4>;
>> +      qcom,lmh-temperature-arm = <65000>;
>> +      qcom,lmh-temperature-low = <94500>;
>> +      qcom,lmh-temperature-high = <95000>;
>> +      interrupt-controller;
>> +      #interrupt-cells = <1>;
>> +    };
>> +  - |
> 
> This is a different example from the one above, if you intended that,
> don't you need the #include of arm-gic.h here as well?

Again make dt-bindings check did not fail. It is a different example.
So I am not sure of the norm here. Is one example good enough ?

> 
> Regards,
> Bjorn
> 
>> +    lmh_cluster0: lmh@17d78800 {
>> +      compatible = "qcom,sdm845-lmh";
>> +      reg = <0 0x17d78800 0 0x401>;
>> +      interrupts = <GIC_SPI 32 IRQ_TYPE_LEVEL_HIGH>;
>> +      qcom,lmh-cpu-id = <0x0>;
>> +      qcom,lmh-temperature-arm = <65000>;
>> +      qcom,lmh-temperature-low = <94500>;
>> +      qcom,lmh-temperature-high = <95000>;
>> +      interrupt-controller;
>> +      #interrupt-cells = <1>;
>> +    };
>> +  - |
>> -- 
>> 2.25.1
>>

-- 
Warm Regards
Thara (She/Her/Hers)

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

* Re: [Patch v3 3/6] cpufreq: qcom-cpufreq-hw: Add dcvs interrupt support
  2021-07-10  4:57   ` Bjorn Andersson
@ 2021-07-13  1:09     ` Thara Gopinath
  0 siblings, 0 replies; 29+ messages in thread
From: Thara Gopinath @ 2021-07-13  1:09 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: agross, rui.zhang, daniel.lezcano, viresh.kumar, rjw, robh+dt,
	tdas, mka, linux-arm-msm, linux-pm, linux-kernel, devicetree



On 7/10/21 12:57 AM, Bjorn Andersson wrote:
> On Thu 08 Jul 07:06 CDT 2021, Thara Gopinath wrote:
> 
>> Add interrupt support to notify the kernel of h/w initiated frequency
>> throttling by LMh. Convey this to scheduler via thermal presssure
>> interface.
>>
>> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
>> ---
>>
>> v2->v3:
>> 	- Cosmetic fixes from review comments on the list.
>> 	- Moved all LMh initializations to qcom_cpufreq_hw_lmh_init.
>> 	- Added freeing of LMh interrupt and cancelling the polling worker to
>> 	  qcom_cpufreq_hw_cpu_exit as per Viresh's suggestion.
>> 	- LMh interrupts are now tied to cpu dev and not cpufreq dev. This will be
>> 	  useful for further generation of SoCs where the same interrupt signals
>> 	  multiple cpu clusters.
>>
>> v1->v2:
>> 	- Introduced qcom_cpufreq_hw_lmh_init to consolidate LMh related initializations
>> 	  as per Viresh's review comment.
>> 	- Moved the piece of code restarting polling/re-enabling LMh interrupt to
>> 	  qcom_lmh_dcvs_notify therby simplifying isr and timer callback as per Viresh's
>> 	  suggestion.
>> 	- Droped cpus from qcom_cpufreq_data and instead using cpus from cpufreq_policy in
>> 	  qcom_lmh_dcvs_notify as per Viresh's review comment.
>> 	- Dropped dt property qcom,support-lmh as per Bjorn's suggestion.
>> 	- Other minor/cosmetic fixes
>>
>>   drivers/cpufreq/qcom-cpufreq-hw.c | 118 ++++++++++++++++++++++++++++++
>>   1 file changed, 118 insertions(+)
>>
>> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
>> index f86859bf76f1..bb5fc700d913 100644
>> --- a/drivers/cpufreq/qcom-cpufreq-hw.c
>> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
>> @@ -7,6 +7,7 @@
>>   #include <linux/cpufreq.h>
>>   #include <linux/init.h>
>>   #include <linux/interconnect.h>
>> +#include <linux/interrupt.h>
>>   #include <linux/kernel.h>
>>   #include <linux/module.h>
>>   #include <linux/of_address.h>
>> @@ -22,10 +23,13 @@
>>   #define CLK_HW_DIV			2
>>   #define LUT_TURBO_IND			1
>>   
>> +#define HZ_PER_KHZ			1000
>> +
>>   struct qcom_cpufreq_soc_data {
>>   	u32 reg_enable;
>>   	u32 reg_freq_lut;
>>   	u32 reg_volt_lut;
>> +	u32 reg_current_vote;
>>   	u32 reg_perf_state;
>>   	u8 lut_row_size;
>>   };
>> @@ -33,7 +37,10 @@ struct qcom_cpufreq_soc_data {
>>   struct qcom_cpufreq_data {
>>   	void __iomem *base;
>>   	struct resource *res;
>> +	struct delayed_work lmh_dcvs_poll_work;
> 
> How about dropping "lmh" from this variable name?
> 
> Perhaps "throttle_work" or something like that?
> 
>>   	const struct qcom_cpufreq_soc_data *soc_data;
>> +	struct cpufreq_policy *policy;
>> +	int lmh_dcvs_irq;
> 
> throttle_irq ?

sounds good!

> 
>>   };
>>   
>>   static unsigned long cpu_hw_rate, xo_rate;
>> @@ -251,10 +258,84 @@ static void qcom_get_related_cpus(int index, struct cpumask *m)
>>   	}
>>   }
>>   
>> +static inline unsigned long qcom_lmh_vote_to_freq(u32 val)
>> +{
>> +	return (val & 0x3FF) * 19200;
>> +}
>> +
>> +static void qcom_lmh_dcvs_notify(struct qcom_cpufreq_data *data)
>> +{
>> +	struct cpufreq_policy *policy = data->policy;
>> +	struct dev_pm_opp *opp;
>> +	struct device *dev;
>> +	unsigned long max_capacity, capacity, freq_hz, throttled_freq;
>> +	unsigned int val, freq;
>> +
>> +	/*
>> +	 * Get the h/w throttled frequency, normalize it using the
>> +	 * registered opp table and use it to calculate thermal pressure.
>> +	 */
>> +	val = readl_relaxed(data->base + data->soc_data->reg_current_vote);
> 
> I would find it cleaner to move the readl() into the helper function, as
> you don't care about the register value, only the resulting frequency.

Ok..

> 
>> +	freq = qcom_lmh_vote_to_freq(val);
>> +	freq_hz = freq * HZ_PER_KHZ;
>> +
>> +	dev = get_cpu_device(cpumask_first(policy->cpus));
>> +	opp = dev_pm_opp_find_freq_floor(dev, &freq_hz);
>> +	if (IS_ERR(opp) && PTR_ERR(opp) == -ERANGE)
>> +		opp = dev_pm_opp_find_freq_ceil(dev, &freq_hz);
>> +
>> +	throttled_freq = freq_hz / HZ_PER_KHZ;
>> +
>> +	/* Update thermal pressure */
>> +
>> +	max_capacity = arch_scale_cpu_capacity(cpumask_first(policy->cpus));
>> +	capacity = throttled_freq * max_capacity;
>> +	capacity /= policy->cpuinfo.max_freq;
> 
> Perhaps, to avoid overflows if this is ever used on a 32-bit platform
> use:
> 
> 	mult_frac(max_capacity, throttled_freq, policy->cpuinfo.max_freq)

yep. sounds good.

> 
>> +
>> +	/* Don't pass boost capacity to scheduler */
>> +	if (capacity > max_capacity)
>> +		capacity = max_capacity;
>> +
>> +	arch_set_thermal_pressure(policy->cpus, max_capacity - capacity);
>> +
>> +	/*
>> +	 * If h/w throttled frequency is higher than what cpufreq has requested for, stop
>> +	 * polling and switch back to interrupt mechanism
>> +	 */
>> +
>> +	if (throttled_freq >= qcom_cpufreq_hw_get(cpumask_first(policy->cpus)))
>> +		/* Clear the existing interrupts and enable it back */
>> +		enable_irq(data->lmh_dcvs_irq);
>> +	else
>> +		mod_delayed_work(system_highpri_wq, &data->lmh_dcvs_poll_work,
>> +				 msecs_to_jiffies(10));
>> +}
>> +
>> +static void qcom_lmh_dcvs_poll(struct work_struct *work)
>> +{
>> +	struct qcom_cpufreq_data *data;
>> +
>> +	data = container_of(work, struct qcom_cpufreq_data, lmh_dcvs_poll_work.work);
>> +
>> +	qcom_lmh_dcvs_notify(data);
>> +}
>> +
>> +static irqreturn_t qcom_lmh_dcvs_handle_irq(int irq, void *data)
>> +{
>> +	struct qcom_cpufreq_data *c_data = data;
>> +
>> +	/* Disable interrupt and enable polling */
>> +	disable_irq_nosync(c_data->lmh_dcvs_irq);
>> +	qcom_lmh_dcvs_notify(c_data);
>> +
>> +	return 0;
>> +}
>> +
>>   static const struct qcom_cpufreq_soc_data qcom_soc_data = {
>>   	.reg_enable = 0x0,
>>   	.reg_freq_lut = 0x110,
>>   	.reg_volt_lut = 0x114,
>> +	.reg_current_vote = 0x704,
>>   	.reg_perf_state = 0x920,
>>   	.lut_row_size = 32,
>>   };
>> @@ -274,6 +355,35 @@ static const struct of_device_id qcom_cpufreq_hw_match[] = {
>>   };
>>   MODULE_DEVICE_TABLE(of, qcom_cpufreq_hw_match);
>>   
>> +static int qcom_cpufreq_hw_lmh_init(struct cpufreq_policy *policy, int index)
>> +{
>> +	struct qcom_cpufreq_data *data = policy->driver_data;
>> +	struct platform_device *pdev = cpufreq_get_driver_data();
>> +	struct device *cpu_dev = get_cpu_device(policy->cpu);
>> +	char irq_name[15];
>> +	int ret;
>> +
>> +	/*
>> +	 * Look for LMh interrupt. If no interrupt line is specified /
>> +	 * if there is an error, allow cpufreq to be enabled as usual.
>> +	 */
>> +	data->lmh_dcvs_irq = platform_get_irq(pdev, index);
>> +	if (data->lmh_dcvs_irq <= 0)
>> +		return data->lmh_dcvs_irq == -EPROBE_DEFER ? -EPROBE_DEFER : 0;
>> +
>> +	snprintf(irq_name, sizeof(irq_name), "dcvsh-irq-%u", policy->cpu);
>> +	ret = devm_request_irq(cpu_dev, data->lmh_dcvs_irq, qcom_lmh_dcvs_handle_irq,
>> +			       0, irq_name, data);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "Error %d registering irq %x\n", ret, data->lmh_dcvs_irq);
> 
> The irq number here won't have any meaning, and %x wouldn't be suitable.
> 
> How about ..."Error registering %s: %d\n", irq_name, ret); ?

ok.

> 
>> +		return 0;
> 
> This sounds like a problem, wouldn't it be suitable to treat it as a
> problem?

I thought a lot about this. My point is even if LMh does not get enabled 
due to some reason, cpufreq should be enabled. If I return an error back 
from here, cpufreq will be disabled.


> 
>> +	}
>> +	data->policy = policy;
> 
> Afaict, no one is going to access data->policy unless devm_request_irq()
> succeeds and if it does and the interrupt fires immediately it would be
> too late to set it here. So better move it earlier.
> 
>> +	INIT_DEFERRABLE_WORK(&data->lmh_dcvs_poll_work, qcom_lmh_dcvs_poll);
> 
> What if the interrupt fires before you initialize the work? Better move
> this higher up.

I will move this and the data->policy = policy above before requesting 
the interrupt.

> 
>> +
>> +	return 0;
>> +}
>> +
>>   static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
>>   {
>>   	struct platform_device *pdev = cpufreq_get_driver_data();
>> @@ -370,6 +480,10 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
>>   			dev_warn(cpu_dev, "failed to enable boost: %d\n", ret);
>>   	}
>>   
>> +	ret = qcom_cpufreq_hw_lmh_init(policy, index);
>> +	if (ret)
>> +		goto error;
>> +
>>   	return 0;
>>   error:
>>   	kfree(data);
>> @@ -389,6 +503,10 @@ static int qcom_cpufreq_hw_cpu_exit(struct cpufreq_policy *policy)
>>   
>>   	dev_pm_opp_remove_all_dynamic(cpu_dev);
>>   	dev_pm_opp_of_cpumask_remove_table(policy->related_cpus);
>> +	if (data->lmh_dcvs_irq > 0) {
>> +		devm_free_irq(cpu_dev, data->lmh_dcvs_irq, data);
> 
> As init/exit are called multiple times you should avoid the devm
> variants.

Yes. I think Viresh was also mentioning this. I will move to non devm 
version.

> 
> Regards,
> Bjorn
> 
>> +		cancel_delayed_work_sync(&data->lmh_dcvs_poll_work);
>> +	}
>>   	kfree(policy->freq_table);
>>   	kfree(data);
>>   	iounmap(base);
>> -- 
>> 2.25.1
>>

-- 
Warm Regards
Thara (She/Her/Hers)

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

* Re: [Patch v3 3/6] cpufreq: qcom-cpufreq-hw: Add dcvs interrupt support
  2021-07-12  4:41       ` Viresh Kumar
@ 2021-07-13  1:18         ` Thara Gopinath
  2021-07-13  3:18           ` Viresh Kumar
  0 siblings, 1 reply; 29+ messages in thread
From: Thara Gopinath @ 2021-07-13  1:18 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: agross, bjorn.andersson, rui.zhang, daniel.lezcano, rjw, robh+dt,
	tdas, mka, linux-arm-msm, linux-pm, linux-kernel, devicetree



On 7/12/21 12:41 AM, Viresh Kumar wrote:
> On 09-07-21, 11:37, Thara Gopinath wrote:
>> On 7/9/21 2:46 AM, Viresh Kumar wrote:
>>>> @@ -389,6 +503,10 @@ static int qcom_cpufreq_hw_cpu_exit(struct cpufreq_policy *policy)
>>>>    	dev_pm_opp_remove_all_dynamic(cpu_dev);
>>>>    	dev_pm_opp_of_cpumask_remove_table(policy->related_cpus);
>>>> +	if (data->lmh_dcvs_irq > 0) {
>>>> +		devm_free_irq(cpu_dev, data->lmh_dcvs_irq, data);
>>>
>>> Why using devm variants here and while requesting the irq ?
> 
> Missed this one ?

Yep. I just replied to Bjorn's email on this. I will move to non devm 
version.

> 
>>>
>>>> +		cancel_delayed_work_sync(&data->lmh_dcvs_poll_work);
>>>> +	}
>>>
>>> Please move this to qcom_cpufreq_hw_lmh_exit() or something.
>>
>> Ok.
>>
>>>
>>> Now with sequence of disabling interrupt, etc, I see a potential
>>> problem.
>>>
>>> CPU0                                    CPU1
>>>
>>> qcom_cpufreq_hw_cpu_exit()
>>> -> devm_free_irq();
>>>                                           qcom_lmh_dcvs_poll()
>>>                                           -> qcom_lmh_dcvs_notify()
>>>                                             -> enable_irq()
>>>
>>> -> cancel_delayed_work_sync();
>>>
>>>
>>> What will happen if enable_irq() gets called after freeing the irq ?
>>> Not sure, but it looks like you will hit this then from manage.c:
>>>
>>> WARN(!desc->irq_data.chip, KERN_ERR "enable_irq before
>>>                                        setup/request_irq: irq %u\n", irq))
>>>
>>> ?
>>>
>>> You got a chicken n egg problem :)
>>
>> Yes indeed! But also it is a very rare chicken and egg problem.
>> The scenario here is that the cpus are busy and running load causing a
>> thermal overrun and lmh is engaged. At the same time for this issue to be
>> hit the cpu is trying to exit/disable cpufreq.
> 
> Yes, it is a very specific case but it needs to be resolved anyway. You don't
> want to get this ever :)
> 
>> Calling
>> cancel_delayed_work_sync first could solve this issue, right ?
>> cancel_delayed_work_sync guarantees the work not to be pending even if
>> it requeues itself on return. So once the delayed work is cancelled, the
>> interrupts can be safely disabled. Thoughts ?
> 
> I don't think even that would provide such guarantees to you here, as there is
> a chance the work gets queued again because of an interrupt that triggers right
> after you cancel the work.
> 
> The basic way of solving such issues is that once you cancel something, you need
> to guarantee that it doesn't get triggered again, no matter what.
> 
> The problem here I see is with your design itself, both delayed work and irq can
> enable each other, so no matter which one you disable first, won't be
> sufficient. You need to fix that design somehow.

So I really need the interrupt to fire and then the timer to kick in and 
take up the monitoring. I can think of introducing a variable 
is_disabled which is updated and read under a spinlock. 
qcom_cpufreq_hw_cpu_exit can hold the spinlock and set is_disabled to 
true prior to cancelling the work queue or disabling the interrupt. 
Before re-enabling the interrupt or re-queuing the work in 
qcom_lmh_dcvs_notify, is_disabled can be read and checked.

But does this problem not exist in target_index , fast_switch etc also ? 
One cpu can be disabling and the other one can be updating the target 
right?

> 

-- 
Warm Regards
Thara (She/Her/Hers)

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

* Re: [Patch v3 3/6] cpufreq: qcom-cpufreq-hw: Add dcvs interrupt support
  2021-07-13  1:18         ` Thara Gopinath
@ 2021-07-13  3:18           ` Viresh Kumar
  2021-07-14 12:37             ` Thara Gopinath
  0 siblings, 1 reply; 29+ messages in thread
From: Viresh Kumar @ 2021-07-13  3:18 UTC (permalink / raw)
  To: Thara Gopinath
  Cc: agross, bjorn.andersson, rui.zhang, daniel.lezcano, rjw, robh+dt,
	tdas, mka, linux-arm-msm, linux-pm, linux-kernel, devicetree

On 12-07-21, 21:18, Thara Gopinath wrote:
> So I really need the interrupt to fire and then the timer to kick in and
> take up the monitoring. I can think of introducing a variable is_disabled
> which is updated and read under a spinlock. qcom_cpufreq_hw_cpu_exit can
> hold the spinlock and set is_disabled to true prior to cancelling the work
> queue or disabling the interrupt. Before re-enabling the interrupt or
> re-queuing the work in qcom_lmh_dcvs_notify, is_disabled can be read and
> checked.

Or you can make the lmh_dcvs_poll_work item a pointer and mark it NULL in exit,
with proper locking etc.

> But does this problem not exist in target_index , fast_switch etc also ? One
> cpu can be disabling and the other one can be updating the target right?

The race doesn't happen there as cpufreq_unregister_driver() takes care of
stopping everything before removing the policy. To be more precise, governor's
->stop() function is responsible for making sure that frequency won't be updated
any further.

-- 
viresh

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

* Re: [Patch v3 3/6] cpufreq: qcom-cpufreq-hw: Add dcvs interrupt support
  2021-07-13  3:18           ` Viresh Kumar
@ 2021-07-14 12:37             ` Thara Gopinath
  0 siblings, 0 replies; 29+ messages in thread
From: Thara Gopinath @ 2021-07-14 12:37 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: agross, bjorn.andersson, rui.zhang, daniel.lezcano, rjw, robh+dt,
	tdas, mka, linux-arm-msm, linux-pm, linux-kernel, devicetree



On 7/12/21 11:18 PM, Viresh Kumar wrote:
> On 12-07-21, 21:18, Thara Gopinath wrote:
>> So I really need the interrupt to fire and then the timer to kick in and
>> take up the monitoring. I can think of introducing a variable is_disabled
>> which is updated and read under a spinlock. qcom_cpufreq_hw_cpu_exit can
>> hold the spinlock and set is_disabled to true prior to cancelling the work
>> queue or disabling the interrupt. Before re-enabling the interrupt or
>> re-queuing the work in qcom_lmh_dcvs_notify, is_disabled can be read and
>> checked.
> 
> Or you can make the lmh_dcvs_poll_work item a pointer and mark it NULL in exit,
> with proper locking etc.

Yes it could work. I will spin the next version with either this or 
introducing a new variable with locking.

> 
>> But does this problem not exist in target_index , fast_switch etc also ? One
>> cpu can be disabling and the other one can be updating the target right?
> 
> The race doesn't happen there as cpufreq_unregister_driver() takes care of
> stopping everything before removing the policy. To be more precise, governor's
> ->stop() function is responsible for making sure that frequency won't be updated
> any further.
> 

-- 
Warm Regards
Thara (She/Her/Hers)

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

* Re: [Patch v3 4/6] arm64: boot: dts: qcom: sdm45: Add support for LMh node
  2021-07-08 12:06 ` [Patch v3 4/6] arm64: boot: dts: qcom: sdm45: Add support for LMh node Thara Gopinath
  2021-07-10  4:17   ` Bjorn Andersson
@ 2021-07-19 16:33   ` Bjorn Andersson
  2021-07-19 22:44     ` Thara Gopinath
  1 sibling, 1 reply; 29+ messages in thread
From: Bjorn Andersson @ 2021-07-19 16:33 UTC (permalink / raw)
  To: Thara Gopinath
  Cc: agross, rui.zhang, daniel.lezcano, viresh.kumar, rjw, robh+dt,
	tdas, mka, linux-arm-msm, linux-pm, linux-kernel, devicetree

On Thu 08 Jul 07:06 CDT 2021, Thara Gopinath wrote:

> Add LMh nodes for cpu cluster0 and cpu cluster1. Also add interrupt
> support in cpufreq node to capture the LMh interrupt and let the scheduler
> know of the max frequency throttling.
> 

Just noticed, could you please drop "boot: " from $subject and add the
missing '8', as you're resubmitting the series.

Regards,
Bjorn

> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
> ---
> 
> v2->v3:
> 	- Changed the LMh low and high trip to 94500 and 95000 mC from
> 	  74500 and 75000 mC. This was a bug that got introduced in v2.
> v1->v2:
> 	- Dropped dt property qcom,support-lmh as per Bjorn's review comments.
> 	- Changed lmh compatible from generic to platform specific.
> 	- Introduced properties specifying arm, low and high temp thresholds for LMh
> 	  as per Daniel's suggestion.
> 
>  arch/arm64/boot/dts/qcom/sdm845.dtsi | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> index 0a86fe71a66d..4da6b8f3dd7b 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> @@ -3646,6 +3646,30 @@ swm: swm@c85 {
>  			};
>  		};
>  
> +		lmh_cluster1: lmh@17d70800 {
> +			compatible = "qcom,sdm845-lmh";
> +			reg = <0 0x17d70800 0 0x401>;
> +			interrupts = <GIC_SPI 33 IRQ_TYPE_LEVEL_HIGH>;
> +			qcom,lmh-cpu-id = <0x4>;
> +			qcom,lmh-temperature-arm = <65000>;
> +			qcom,lmh-temperature-low = <94500>;
> +			qcom,lmh-temperature-high = <95000>;
> +			interrupt-controller;
> +			#interrupt-cells = <1>;
> +		};
> +
> +		lmh_cluster0: lmh@17d78800 {
> +			compatible = "qcom,sdm845-lmh";
> +			reg = <0 0x17d78800 0 0x401>;
> +			interrupts = <GIC_SPI 32 IRQ_TYPE_LEVEL_HIGH>;
> +			qcom,lmh-cpu-id = <0x0>;
> +			qcom,lmh-temperature-arm = <65000>;
> +			qcom,lmh-temperature-low = <94500>;
> +			qcom,lmh-temperature-high = <95000>;
> +			interrupt-controller;
> +			#interrupt-cells = <1>;
> +		};
> +
>  		sound: sound {
>  		};
>  
> @@ -4911,6 +4935,8 @@ cpufreq_hw: cpufreq@17d43000 {
>  			reg = <0 0x17d43000 0 0x1400>, <0 0x17d45800 0 0x1400>;
>  			reg-names = "freq-domain0", "freq-domain1";
>  
> +			interrupts-extended = <&lmh_cluster0 0>, <&lmh_cluster1 0>;
> +
>  			clocks = <&rpmhcc RPMH_CXO_CLK>, <&gcc GPLL0>;
>  			clock-names = "xo", "alternate";
>  
> -- 
> 2.25.1
> 

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

* Re: [Patch v3 4/6] arm64: boot: dts: qcom: sdm45: Add support for LMh node
  2021-07-19 16:33   ` Bjorn Andersson
@ 2021-07-19 22:44     ` Thara Gopinath
  0 siblings, 0 replies; 29+ messages in thread
From: Thara Gopinath @ 2021-07-19 22:44 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: agross, rui.zhang, daniel.lezcano, viresh.kumar, rjw, robh+dt,
	tdas, mka, linux-arm-msm, linux-pm, linux-kernel, devicetree



On 7/19/21 12:33 PM, Bjorn Andersson wrote:
> On Thu 08 Jul 07:06 CDT 2021, Thara Gopinath wrote:
> 
>> Add LMh nodes for cpu cluster0 and cpu cluster1. Also add interrupt
>> support in cpufreq node to capture the LMh interrupt and let the scheduler
>> know of the max frequency throttling.
>>
> 
> Just noticed, could you please drop "boot: " from $subject and add the
> missing '8', as you're resubmitting the series.

Sure..

Warm Regards
Thara
> 
> Regards,
> Bjorn
> 
>> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
>> ---
>>
>> v2->v3:
>> 	- Changed the LMh low and high trip to 94500 and 95000 mC from
>> 	  74500 and 75000 mC. This was a bug that got introduced in v2.
>> v1->v2:
>> 	- Dropped dt property qcom,support-lmh as per Bjorn's review comments.
>> 	- Changed lmh compatible from generic to platform specific.
>> 	- Introduced properties specifying arm, low and high temp thresholds for LMh
>> 	  as per Daniel's suggestion.
>>
>>   arch/arm64/boot/dts/qcom/sdm845.dtsi | 26 ++++++++++++++++++++++++++
>>   1 file changed, 26 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
>> index 0a86fe71a66d..4da6b8f3dd7b 100644
>> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
>> @@ -3646,6 +3646,30 @@ swm: swm@c85 {
>>   			};
>>   		};
>>   
>> +		lmh_cluster1: lmh@17d70800 {
>> +			compatible = "qcom,sdm845-lmh";
>> +			reg = <0 0x17d70800 0 0x401>;
>> +			interrupts = <GIC_SPI 33 IRQ_TYPE_LEVEL_HIGH>;
>> +			qcom,lmh-cpu-id = <0x4>;
>> +			qcom,lmh-temperature-arm = <65000>;
>> +			qcom,lmh-temperature-low = <94500>;
>> +			qcom,lmh-temperature-high = <95000>;
>> +			interrupt-controller;
>> +			#interrupt-cells = <1>;
>> +		};
>> +
>> +		lmh_cluster0: lmh@17d78800 {
>> +			compatible = "qcom,sdm845-lmh";
>> +			reg = <0 0x17d78800 0 0x401>;
>> +			interrupts = <GIC_SPI 32 IRQ_TYPE_LEVEL_HIGH>;
>> +			qcom,lmh-cpu-id = <0x0>;
>> +			qcom,lmh-temperature-arm = <65000>;
>> +			qcom,lmh-temperature-low = <94500>;
>> +			qcom,lmh-temperature-high = <95000>;
>> +			interrupt-controller;
>> +			#interrupt-cells = <1>;
>> +		};
>> +
>>   		sound: sound {
>>   		};
>>   
>> @@ -4911,6 +4935,8 @@ cpufreq_hw: cpufreq@17d43000 {
>>   			reg = <0 0x17d43000 0 0x1400>, <0 0x17d45800 0 0x1400>;
>>   			reg-names = "freq-domain0", "freq-domain1";
>>   
>> +			interrupts-extended = <&lmh_cluster0 0>, <&lmh_cluster1 0>;
>> +
>>   			clocks = <&rpmhcc RPMH_CXO_CLK>, <&gcc GPLL0>;
>>   			clock-names = "xo", "alternate";
>>   
>> -- 
>> 2.25.1
>>



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

* Re: [Patch v3 0/6] Introduce LMh driver for Qualcomm SoCs
  2021-07-08 12:06 [Patch v3 0/6] Introduce LMh driver for Qualcomm SoCs Thara Gopinath
                   ` (5 preceding siblings ...)
  2021-07-08 12:06 ` [Patch v3 6/6] dt-bindings: thermal: Add dt binding for QCOM LMh Thara Gopinath
@ 2021-07-22  3:14 ` Steev Klimaszewski
  2021-07-27 15:29   ` Thara Gopinath
  6 siblings, 1 reply; 29+ messages in thread
From: Steev Klimaszewski @ 2021-07-22  3:14 UTC (permalink / raw)
  To: Thara Gopinath, agross, bjorn.andersson, rui.zhang,
	daniel.lezcano, viresh.kumar, rjw, robh+dt
  Cc: tdas, mka, linux-arm-msm, linux-pm, linux-kernel, devicetree

Hi Thara!

On 7/8/21 7:06 AM, Thara Gopinath wrote:
> Limits Management Hardware(LMh) is a hardware infrastructure on some
> Qualcomm SoCs that can enforce temperature and current limits as programmed
> by software for certain IPs like CPU. On many newer SoCs LMh is configured
> by firmware/TZ and no programming is needed from the kernel side. But on
> certain SoCs like sdm845 the firmware does not do a complete programming of
> the h/w block. On such SoCs kernel software has to explicitly set up the
> temperature limits and turn on various monitoring and enforcing algorithms
> on the hardware.
>
> Introduce support for enabling and programming various limit settings and
> monitoring capabilities of Limits Management Hardware(LMh) associated with
> cpu clusters. Also introduce support in cpufreq hardware driver to monitor
> the interrupt associated with cpu frequency throttling so that this
> information can be conveyed to the schdeuler via thermal pressure
> interface.
>
> With this patch series following cpu performance improvement(30-70%) is
> observed on sdm845. The reasoning here is that without LMh being programmed
> properly from the kernel, the default settings were enabling thermal
> mitigation for CPUs at too low a temperature (around 70-75 degree C).  This
> in turn meant that many a time CPUs were never actually allowed to hit the
> maximum possible/required frequencies.
>
> UnixBench whets and dhry (./Run whets dhry)
> System Benchmarks Index Score
>
>                 Without LMh Support             With LMh Support
> 1 copy test     1353.7                          1773.2
>
> 8 copy tests    4473.6                          7402.3
>
> Sysbench cpu
> sysbench cpu --threads=8 --time=60 --cpu-max-prime=100000 run
>
>                 Without LMh Support             With LMh Support
> Events per
> second                  355                             614
>
> Avg Latency(ms)         21.84                           13.02
>
> v2->v3:
> 	- Included patch adding dt binding documentation for LMh nodes.
> 	- Rebased to v5.13
>
> Thara Gopinath (6):
>   firmware: qcom_scm: Introduce SCM calls to access LMh
>   thermal: qcom: Add support for LMh driver
>   cpufreq: qcom-cpufreq-hw: Add dcvs interrupt support
>   arm64: boot: dts: qcom: sdm45: Add support for LMh node
>   arm64: boot: dts: qcom: sdm845: Remove cpufreq cooling devices for CPU
>     thermal zones
>   dt-bindings: thermal: Add dt binding for QCOM LMh
>
>  .../devicetree/bindings/thermal/qcom-lmh.yaml | 100 ++++++++
>  arch/arm64/boot/dts/qcom/sdm845.dtsi          | 162 ++----------
>  drivers/cpufreq/qcom-cpufreq-hw.c             | 118 +++++++++
>  drivers/firmware/qcom_scm.c                   |  58 +++++
>  drivers/firmware/qcom_scm.h                   |   4 +
>  drivers/thermal/qcom/Kconfig                  |  10 +
>  drivers/thermal/qcom/Makefile                 |   1 +
>  drivers/thermal/qcom/lmh.c                    | 239 ++++++++++++++++++
>  include/linux/qcom_scm.h                      |  14 +
>  9 files changed, 570 insertions(+), 136 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/thermal/qcom-lmh.yaml
>  create mode 100644 drivers/thermal/qcom/lmh.c
>
I've been using these patches on a 5.13 kernel
(https://github.com/steev/linux/tree/linux-5.13.y - while trying to
track down a different issue, while playing a video on youtube, as well
as compressing a 9.2GB file with xz, I got the following

|Jul 21 21:44:21 limitless kernel: [ 5438.914591] EXT4-fs (loop0p1):
mounting ext3 file system using the ext4 subsystem |

|Jul 21 21:44:21 limitless kernel: [ 5438.920817] EXT4-fs (loop0p1):
mounted filesystem with ordered data mode. Opts: (null). Quota mode:
none. |

|Jul 21 21:45:56 limitless kernel: [ 5533.893290] BUG: scheduling while
atomic: swapper/0/0/0x00010000 |

|Jul 21 21:45:56 limitless kernel: [ 5533.893333] Modules linked in:
dm_mod loop tcp_diag inet_diag aes_ce_ccm rfcomm algif_hash
algif_skcipher af_alg bnep lz4 lz4_compress q6asm_dai zram q6routing
zsmalloc q6afe_dai q6adm q6asm q6afe q6dsp_common q6core snd_soc_wsa881x
regmap_sdw snd_soc_wcd934x soundwire_qcom gpio_wcd934x binfmt_misc
venus_enc venus_dec videobuf2_dma_contig nls_ascii nls_cp437 vfat fat
wcd934x regmap_slimbus qrtr_smd fastrpc apr aes_ce_blk crypto_simd
cryptd aes_ce_cipher crct10dif_ce ghash_ce gf128mul sha2_ce uvcvideo
videobuf2_vmalloc videobuf2_memops hci_uart ath10k_snoc sha256_arm64
sha1_ce btqca ath10k_core ath btrtl mac80211 btbcm btintel
snd_soc_sdm845 snd_soc_rt5663 snd_soc_qcom_common snd_soc_rl6231
soundwire_bus pm8941_pwrkey qcom_spmi_adc5 bluetooth qcom_vadc_common
snd_soc_core venus_core snd_compress snd_pcm_dmaengine snd_pcm
v4l2_mem2mem snd_timer snd qcom_spmi_temp_alarm videobuf2_v4l2 soundcore
industrialio videobuf2_common videodev joydev mc hid_multitouch
ecdh_generic ecc cfg80211 |

|Jul 21 21:45:56 limitless kernel: [ 5533.893841]  qcom_rng sg rfkill
qrtr ns libarc4 qcom_q6v5_mss slim_qcom_ngd_ctrl qcom_q6v5_pas evdev
pdr_interface qcom_pil_info qcom_q6v5 slimbus qcom_sysmon rmtfs_mem
bam_dma qcom_wdt fuse configfs ip_tables x_tables autofs4 ext4 mbcache
jbd2 msm rtc_pm8xxx llcc_qcom ti_sn65dsi86 ocmem i2c_hid_of
drm_kms_helper ipa qcom_common qmi_helpers mdt_loader camcc_sdm845
panel_simple drm gpio_keys |

|Jul 21 21:45:56 limitless kernel: [ 5533.894072] CPU: 0 PID: 0 Comm:
swapper/0 Tainted: G        W         5.13.4 #1 |

|Jul 21 21:45:56 limitless kernel: [ 5533.894087] Hardware name: LENOVO
81JL/LNVNB161216, BIOS 9UCN33WW(V2.06) 06/ 4/2019 |

|Jul 21 21:45:56 limitless kernel: [ 5533.894098] Call trace: |

|Jul 21 21:45:56 limitless kernel: [
5533.894103]  dump_backtrace+0x0/0x1e4 |

|Jul 21 21:45:56 limitless kernel: [ 5533.894133]  show_stack+0x24/0x30 |

|Jul 21 21:45:56 limitless kernel: [ 5533.894150]  dump_stack+0xd0/0x12c |

|Jul 21 21:45:56 limitless kernel: [
5533.894168]  __schedule_bug+0x68/0x80 |

|Jul 21 21:45:56 limitless kernel: [ 5533.894186]  __schedule+0x74c/0x8ac |

|Jul 21 21:45:56 limitless kernel: [ 5533.894202]  schedule+0x54/0xe0 |

|Jul 21 21:45:56 limitless kernel: [
5533.894217]  schedule_preempt_disabled+0x1c/0x2c |

|Jul 21 21:45:56 limitless kernel: [
5533.894234]  __mutex_lock.constprop.0+0x55c/0x590 |

|Jul 21 21:45:56 limitless kernel: [
5533.894247]  __mutex_lock_slowpath+0x1c/0x30 |

|Jul 21 21:45:56 limitless kernel: [ 5533.894260]  mutex_lock+0x6c/0x80 |

|Jul 21 21:45:56 limitless kernel: [
5533.894271]  dev_pm_opp_find_freq_floor+0x4c/0x200 |

|Jul 21 21:45:56 limitless kernel: [
5533.894287]  qcom_lmh_dcvs_notify+0x74/0x170 |

|Jul 21 21:45:56 limitless kernel: [
5533.894301]  qcom_lmh_dcvs_handle_irq+0x30/0x44 |

|Jul 21 21:45:56 limitless kernel: [
5533.894313]  __handle_irq_event_percpu+0x68/0x210 |

|Jul 21 21:45:56 limitless kernel: [
5533.894328]  handle_irq_event+0x6c/0x1b0 |

|Jul 21 21:45:56 limitless kernel: [
5533.894340]  handle_simple_irq+0xc8/0x170 |

|Jul 21 21:45:56 limitless kernel: [
5533.894355]  generic_handle_irq+0x3c/0x5c |

|Jul 21 21:45:56 limitless kernel: [
5533.894368]  lmh_handle_irq+0x40/0x50 |

|Jul 21 21:45:56 limitless kernel: [
5533.894383]  __handle_irq_event_percpu+0x68/0x210 |

|Jul 21 21:45:56 limitless kernel: [
5533.894395]  handle_irq_event+0x6c/0x1b0 |

|Jul 21 21:45:56 limitless kernel: [
5533.894407]  handle_fasteoi_irq+0xcc/0x1fc |

|Jul 21 21:45:56 limitless kernel: [
5533.894421]  __handle_domain_irq+0x88/0xec |

|Jul 21 21:45:56 limitless kernel: [
5533.894433]  gic_handle_irq+0xc8/0x148 |

|Jul 21 21:45:56 limitless kernel: [ 5533.894446]  el1_irq+0xbc/0x140 |

|Jul 21 21:45:56 limitless kernel: [
5533.894458]  arch_local_irq_enable+0xc/0x14 |

|Jul 21 21:45:56 limitless kernel: [ 5533.894473]  __schedule+0x2fc/0x8ac |

|Jul 21 21:45:56 limitless kernel: [
5533.894489]  schedule_idle+0x34/0x54 |

|Jul 21 21:45:56 limitless kernel: [ 5533.894504]  do_idle+0x1a4/0x2b0 |

|Jul 21 21:45:56 limitless kernel: [
5533.894520]  cpu_startup_entry+0x34/0xa0 |

|Jul 21 21:45:56 limitless kernel: [ 5533.894536]  rest_init+0xcc/0xdc |

|Jul 21 21:45:56 limitless kernel: [
5533.894550]  arch_call_rest_init+0x1c/0x28 |

|Jul 21 21:45:56 limitless kernel: [
5533.894567]  start_kernel+0x5b4/0x5ec |

|Jul 21 21:45:56 limitless kernel: [ 5533.894954] ------------[ cut here
]------------ |

|Jul 21 21:45:56 limitless kernel: [ 5533.894966] irq 156 handler
qcom_lmh_dcvs_handle_irq+0x0/0x44 enabled interrupts |

|Jul 21 21:45:56 limitless kernel: [ 5533.895005] WARNING: CPU: 0 PID: 0
at kernel/irq/handle.c:159 __handle_irq_event_percpu+0x1d8/0x210 |

|Jul 21 21:45:56 limitless kernel: [ 5533.895028] Modules linked in:
dm_mod loop tcp_diag inet_diag aes_ce_ccm rfcomm algif_hash
algif_skcipher af_alg bnep lz4 lz4_compress q6asm_dai zram q6routing
zsmalloc q6afe_dai q6adm q6asm q6afe q6dsp_common q6core snd_soc_wsa881x
regmap_sdw snd_soc_wcd934x soundwire_qcom gpio_wcd934x binfmt_misc
venus_enc venus_dec videobuf2_dma_contig nls_ascii nls_cp437 vfat fat
wcd934x regmap_slimbus qrtr_smd fastrpc apr aes_ce_blk crypto_simd
cryptd aes_ce_cipher crct10dif_ce ghash_ce gf128mul sha2_ce uvcvideo
videobuf2_vmalloc videobuf2_memops hci_uart ath10k_snoc sha256_arm64
sha1_ce btqca ath10k_core ath btrtl mac80211 btbcm btintel
snd_soc_sdm845 snd_soc_rt5663 snd_soc_qcom_common snd_soc_rl6231
soundwire_bus pm8941_pwrkey qcom_spmi_adc5 bluetooth qcom_vadc_common
snd_soc_core venus_core snd_compress snd_pcm_dmaengine snd_pcm
v4l2_mem2mem snd_timer snd qcom_spmi_temp_alarm videobuf2_v4l2 soundcore
industrialio videobuf2_common videodev joydev mc hid_multitouch
ecdh_generic ecc cfg80211 |

|Jul 21 21:45:56 limitless kernel: [ 5533.895508]  qcom_rng sg rfkill
qrtr ns libarc4 qcom_q6v5_mss slim_qcom_ngd_ctrl qcom_q6v5_pas evdev
pdr_interface qcom_pil_info qcom_q6v5 slimbus qcom_sysmon rmtfs_mem
bam_dma qcom_wdt fuse configfs ip_tables x_tables autofs4 ext4 mbcache
jbd2 msm rtc_pm8xxx llcc_qcom ti_sn65dsi86 ocmem i2c_hid_of
drm_kms_helper ipa qcom_common qmi_helpers mdt_loader camcc_sdm845
panel_simple drm gpio_keys |

|Jul 21 21:45:56 limitless kernel: [ 5533.895732] CPU: 0 PID: 0 Comm:
swapper/0 Tainted: G        W         5.13.4 #1 |

|Jul 21 21:45:56 limitless kernel: [ 5533.895747] Hardware name: LENOVO
81JL/LNVNB161216, BIOS 9UCN33WW(V2.06) 06/ 4/2019 |

|Jul 21 21:45:56 limitless kernel: [ 5533.895756] pstate: 60400005 (nZCv
daif +PAN -UAO -TCO BTYPE=--) |

|Jul 21 21:45:56 limitless kernel: [ 5533.895771] pc :
__handle_irq_event_percpu+0x1d8/0x210 |

|Jul 21 21:45:56 limitless kernel: [ 5533.895785] lr :
__handle_irq_event_percpu+0x1d8/0x210 |

|Jul 21 21:45:56 limitless kernel: [ 5533.895798] sp : ffff800010003de0 |

|Jul 21 21:45:56 limitless kernel: [ 5533.895805] x29: ffff800010003de0
x28: ffffc75e330a3b00 x27: ffffc75e32c02000 |

|Jul 21 21:45:56 limitless kernel: [ 5533.895831] x26: ffff4900496e0600
x25: ffffc75e32c02000 x24: ffffc75e33099a20 |

|Jul 21 21:45:56 limitless kernel: [ 5533.895856] x23: 000000000000009c
x22: ffff800010003e74 x21: 0000000000000000 |

|Jul 21 21:45:56 limitless kernel: [ 5533.895881] x20: 0000000000000000
x19: ffff490049c02900 x18: 00000000fffffffb |

|Jul 21 21:45:56 limitless kernel: [ 5533.895905] x17: 0000000000000000
x16: 0000000000000000 x15: 0000000000000020 |

|Jul 21 21:45:56 limitless kernel: [ 5533.895929] x14: 727265746e692064
x13: 656c62616e652034 x12: 3478302f3078302b |

|Jul 21 21:45:56 limitless kernel: [ 5533.895954] x11: ffffc75e3311a9b0
x10: 00000000fffff000 x9 : ffffc75e31abe5a0 |

|Jul 21 21:45:56 limitless kernel: [ 5533.895978] x8 : ffffc75e330c29b0
x7 : ffffc75e3311a9b0 x6 : 0000000000000000 |

|Jul 21 21:45:56 limitless kernel: [ 5533.896002] x5 : ffff4901b36fa9c8
x4 : ffff800010003bd0 x3 : 0000000000000001 |

|Jul 21 21:45:56 limitless kernel: [ 5533.896026] x2 : 0000000000000000
x1 : 0000000000000000 x0 : ffffc75e330a3b00 |

|Jul 21 21:45:56 limitless kernel: [ 5533.896050] Call trace: |

|Jul 21 21:45:56 limitless kernel: [
5533.896057]  __handle_irq_event_percpu+0x1d8/0x210 |

|Jul 21 21:45:56 limitless kernel: [
5533.896072]  handle_irq_event+0x6c/0x1b0 |

|Jul 21 21:45:56 limitless kernel: [
5533.896085]  handle_simple_irq+0xc8/0x170 |

|Jul 21 21:45:56 limitless kernel: [
5533.896101]  generic_handle_irq+0x3c/0x5c |

|Jul 21 21:45:56 limitless kernel: [
5533.896113]  lmh_handle_irq+0x40/0x50 |

|Jul 21 21:45:56 limitless kernel: [
5533.896128]  __handle_irq_event_percpu+0x68/0x210 |

|Jul 21 21:45:56 limitless kernel: [
5533.896141]  handle_irq_event+0x6c/0x1b0 |

|Jul 21 21:45:56 limitless kernel: [
5533.896153]  handle_fasteoi_irq+0xcc/0x1fc |

|Jul 21 21:45:56 limitless kernel: [
5533.896168]  __handle_domain_irq+0x88/0xec |

|Jul 21 21:45:56 limitless kernel: [
5533.896180]  gic_handle_irq+0xc8/0x148 |

|Jul 21 21:45:56 limitless kernel: [ 5533.896193]  el1_irq+0xbc/0x140 |

|Jul 21 21:45:56 limitless kernel: [
5533.896206]  arch_local_irq_enable+0xc/0x14 |

|Jul 21 21:45:56 limitless kernel: [ 5533.896221]  __schedule+0x2fc/0x8ac |

|Jul 21 21:45:56 limitless kernel: [
5533.896238]  schedule_idle+0x34/0x54 |

|Jul 21 21:45:56 limitless kernel: [ 5533.896254]  do_idle+0x1a4/0x2b0 |

|Jul 21 21:45:56 limitless kernel: [
5533.896269]  cpu_startup_entry+0x34/0xa0 |

|Jul 21 21:45:56 limitless kernel: [ 5533.896285]  rest_init+0xcc/0xdc |

|Jul 21 21:45:56 limitless kernel: [
5533.896299]  arch_call_rest_init+0x1c/0x28 |

|Jul 21 21:45:56 limitless kernel: [
5533.896316]  start_kernel+0x5b4/0x5ec |

|Jul 21 21:45:56 limitless kernel: [ 5533.896332] ---[ end trace
9b7875032d5e8e07 ]--- |

|Jul 21 21:45:56 limitless kernel: [ 5533.896573] BUG: scheduling while
atomic: swapper/0/0/0xffff0000 |

|Jul 21 21:45:56 limitless kernel: [ 5533.896583] Modules linked in:
dm_mod loop tcp_diag inet_diag aes_ce_ccm rfcomm algif_hash
algif_skcipher af_alg bnep lz4 lz4_compress q6asm_dai zram q6routing
zsmalloc q6afe_dai q6adm q6asm q6afe q6dsp_common q6core snd_soc_wsa881x
regmap_sdw snd_soc_wcd934x soundwire_qcom gpio_wcd934x binfmt_misc
venus_enc venus_dec videobuf2_dma_contig nls_ascii nls_cp437 vfat fat
wcd934x regmap_slimbus qrtr_smd fastrpc apr aes_ce_blk crypto_simd
cryptd aes_ce_cipher crct10dif_ce ghash_ce gf128mul sha2_ce uvcvideo
videobuf2_vmalloc videobuf2_memops hci_uart ath10k_snoc sha256_arm64
sha1_ce btqca ath10k_core ath btrtl mac80211 btbcm btintel
snd_soc_sdm845 snd_soc_rt5663 snd_soc_qcom_common snd_soc_rl6231
soundwire_bus pm8941_pwrkey qcom_spmi_adc5 bluetooth qcom_vadc_common
snd_soc_core venus_core snd_compress snd_pcm_dmaengine snd_pcm
v4l2_mem2mem snd_timer snd qcom_spmi_temp_alarm videobuf2_v4l2 soundcore
industrialio videobuf2_common videodev joydev mc hid_multitouch
ecdh_generic ecc cfg80211 |

|Jul 21 21:45:56 limitless kernel: [ 5533.896746]  qcom_rng sg rfkill
qrtr ns libarc4 qcom_q6v5_mss slim_qcom_ngd_ctrl qcom_q6v5_pas evdev
pdr_interface qcom_pil_info qcom_q6v5 slimbus qcom_sysmon rmtfs_mem
bam_dma qcom_wdt fuse configfs ip_tables x_tables autofs4 ext4 mbcache
jbd2 msm rtc_pm8xxx llcc_qcom ti_sn65dsi86 ocmem i2c_hid_of
drm_kms_helper ipa qcom_common qmi_helpers mdt_loader camcc_sdm845
panel_simple drm gpio_keys |

|Jul 21 21:45:56 limitless kernel: [ 5533.896819] CPU: 0 PID: 0 Comm:
swapper/0 Tainted: G        W         5.13.4 #1 |

|Jul 21 21:45:56 limitless kernel: [ 5533.896824] Hardware name: LENOVO
81JL/LNVNB161216, BIOS 9UCN33WW(V2.06) 06/ 4/2019 |

|Jul 21 21:45:56 limitless kernel: [ 5533.896827] Call trace: |

|Jul 21 21:45:56 limitless kernel: [
5533.896829]  dump_backtrace+0x0/0x1e4 |

|Jul 21 21:45:56 limitless kernel: [ 5533.896838]  show_stack+0x24/0x30 |

|Jul 21 21:45:56 limitless kernel: [ 5533.896843]  dump_stack+0xd0/0x12c |

|Jul 21 21:45:56 limitless kernel: [
5533.896849]  __schedule_bug+0x68/0x80 |

|Jul 21 21:45:56 limitless kernel: [ 5533.896855]  __schedule+0x74c/0x8ac |

|Jul 21 21:45:56 limitless kernel: [
5533.896860]  schedule_idle+0x34/0x54 |

|Jul 21 21:45:56 limitless kernel: [ 5533.896866]  do_idle+0x1a4/0x2b0 |

|Jul 21 21:45:56 limitless kernel: [
5533.896871]  cpu_startup_entry+0x34/0xa0 |

|Jul 21 21:45:56 limitless kernel: [ 5533.896876]  rest_init+0xcc/0xdc |

|Jul 21 21:45:56 limitless kernel: [
5533.896881]  arch_call_rest_init+0x1c/0x28 |

|Jul 21 21:45:56 limitless kernel: [
5533.896887]  start_kernel+0x5b4/0x5ec|

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

* Re: [Patch v3 0/6] Introduce LMh driver for Qualcomm SoCs
  2021-07-22  3:14 ` [Patch v3 0/6] Introduce LMh driver for Qualcomm SoCs Steev Klimaszewski
@ 2021-07-27 15:29   ` Thara Gopinath
  2021-07-27 17:43     ` Steev Klimaszewski
  0 siblings, 1 reply; 29+ messages in thread
From: Thara Gopinath @ 2021-07-27 15:29 UTC (permalink / raw)
  To: Steev Klimaszewski, agross, bjorn.andersson, rui.zhang,
	daniel.lezcano, viresh.kumar, rjw, robh+dt
  Cc: tdas, mka, linux-arm-msm, linux-pm, linux-kernel, devicetree



On 7/21/21 11:14 PM, Steev Klimaszewski wrote:
> Hi Thara!
> 
> On 7/8/21 7:06 AM, Thara Gopinath wrote:
>> Limits Management Hardware(LMh) is a hardware infrastructure on some
>> Qualcomm SoCs that can enforce temperature and current limits as programmed
>> by software for certain IPs like CPU. On many newer SoCs LMh is configured
>> by firmware/TZ and no programming is needed from the kernel side. But on
>> certain SoCs like sdm845 the firmware does not do a complete programming of
>> the h/w block. On such SoCs kernel software has to explicitly set up the
>> temperature limits and turn on various monitoring and enforcing algorithms
>> on the hardware.
>>
>> Introduce support for enabling and programming various limit settings and
>> monitoring capabilities of Limits Management Hardware(LMh) associated with
>> cpu clusters. Also introduce support in cpufreq hardware driver to monitor
>> the interrupt associated with cpu frequency throttling so that this
>> information can be conveyed to the schdeuler via thermal pressure
>> interface.
>>
>> With this patch series following cpu performance improvement(30-70%) is
>> observed on sdm845. The reasoning here is that without LMh being programmed
>> properly from the kernel, the default settings were enabling thermal
>> mitigation for CPUs at too low a temperature (around 70-75 degree C).  This
>> in turn meant that many a time CPUs were never actually allowed to hit the
>> maximum possible/required frequencies.
>>
>> UnixBench whets and dhry (./Run whets dhry)
>> System Benchmarks Index Score
>>
>>                  Without LMh Support             With LMh Support
>> 1 copy test     1353.7                          1773.2
>>
>> 8 copy tests    4473.6                          7402.3
>>
>> Sysbench cpu
>> sysbench cpu --threads=8 --time=60 --cpu-max-prime=100000 run
>>
>>                  Without LMh Support             With LMh Support
>> Events per
>> second                  355                             614
>>
>> Avg Latency(ms)         21.84                           13.02
>>
>> v2->v3:
>> 	- Included patch adding dt binding documentation for LMh nodes.
>> 	- Rebased to v5.13
>>
>> Thara Gopinath (6):
>>    firmware: qcom_scm: Introduce SCM calls to access LMh
>>    thermal: qcom: Add support for LMh driver
>>    cpufreq: qcom-cpufreq-hw: Add dcvs interrupt support
>>    arm64: boot: dts: qcom: sdm45: Add support for LMh node
>>    arm64: boot: dts: qcom: sdm845: Remove cpufreq cooling devices for CPU
>>      thermal zones
>>    dt-bindings: thermal: Add dt binding for QCOM LMh
>>
>>   .../devicetree/bindings/thermal/qcom-lmh.yaml | 100 ++++++++
>>   arch/arm64/boot/dts/qcom/sdm845.dtsi          | 162 ++----------
>>   drivers/cpufreq/qcom-cpufreq-hw.c             | 118 +++++++++
>>   drivers/firmware/qcom_scm.c                   |  58 +++++
>>   drivers/firmware/qcom_scm.h                   |   4 +
>>   drivers/thermal/qcom/Kconfig                  |  10 +
>>   drivers/thermal/qcom/Makefile                 |   1 +
>>   drivers/thermal/qcom/lmh.c                    | 239 ++++++++++++++++++
>>   include/linux/qcom_scm.h                      |  14 +
>>   9 files changed, 570 insertions(+), 136 deletions(-)
>>   create mode 100644 Documentation/devicetree/bindings/thermal/qcom-lmh.yaml
>>   create mode 100644 drivers/thermal/qcom/lmh.c
>>
> I've been using these patches on a 5.13 kernel
> (https://github.com/steev/linux/tree/linux-5.13.y - while trying to
> track down a different issue, while playing a video on youtube, as well
> as compressing a 9.2GB file with xz, I got the following

Hi Steev,

Thanks for testing this. I was unable to reproduce this. I have posted 
v4 moving the interrupt handling in qcom-cpufreq-hw to threaded 
interrupt handler and hopefully this should fix the issue. It will be 
great if you can test and let me know.

-- 
Warm Regards
Thara (She/Her/Hers)





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

* Re: [Patch v3 0/6] Introduce LMh driver for Qualcomm SoCs
  2021-07-27 15:29   ` Thara Gopinath
@ 2021-07-27 17:43     ` Steev Klimaszewski
  0 siblings, 0 replies; 29+ messages in thread
From: Steev Klimaszewski @ 2021-07-27 17:43 UTC (permalink / raw)
  To: Thara Gopinath, agross, bjorn.andersson, rui.zhang,
	daniel.lezcano, viresh.kumar, rjw, robh+dt
  Cc: tdas, mka, linux-arm-msm, linux-pm, linux-kernel, devicetree


On 7/27/21 10:29 AM, Thara Gopinath wrote:
>
>
> On 7/21/21 11:14 PM, Steev Klimaszewski wrote:
>> Hi Thara!
>>
>> On 7/8/21 7:06 AM, Thara Gopinath wrote:
>>> Limits Management Hardware(LMh) is a hardware infrastructure on some
>>> Qualcomm SoCs that can enforce temperature and current limits as
>>> programmed
>>> by software for certain IPs like CPU. On many newer SoCs LMh is
>>> configured
>>> by firmware/TZ and no programming is needed from the kernel side.
>>> But on
>>> certain SoCs like sdm845 the firmware does not do a complete
>>> programming of
>>> the h/w block. On such SoCs kernel software has to explicitly set up
>>> the
>>> temperature limits and turn on various monitoring and enforcing
>>> algorithms
>>> on the hardware.
>>>
>>> Introduce support for enabling and programming various limit
>>> settings and
>>> monitoring capabilities of Limits Management Hardware(LMh)
>>> associated with
>>> cpu clusters. Also introduce support in cpufreq hardware driver to
>>> monitor
>>> the interrupt associated with cpu frequency throttling so that this
>>> information can be conveyed to the schdeuler via thermal pressure
>>> interface.
>>>
>>> With this patch series following cpu performance improvement(30-70%) is
>>> observed on sdm845. The reasoning here is that without LMh being
>>> programmed
>>> properly from the kernel, the default settings were enabling thermal
>>> mitigation for CPUs at too low a temperature (around 70-75 degree
>>> C).  This
>>> in turn meant that many a time CPUs were never actually allowed to
>>> hit the
>>> maximum possible/required frequencies.
>>>
>>> UnixBench whets and dhry (./Run whets dhry)
>>> System Benchmarks Index Score
>>>
>>>                  Without LMh Support             With LMh Support
>>> 1 copy test     1353.7                          1773.2
>>>
>>> 8 copy tests    4473.6                          7402.3
>>>
>>> Sysbench cpu
>>> sysbench cpu --threads=8 --time=60 --cpu-max-prime=100000 run
>>>
>>>                  Without LMh Support             With LMh Support
>>> Events per
>>> second                  355                             614
>>>
>>> Avg Latency(ms)         21.84                           13.02
>>>
>>> v2->v3:
>>>     - Included patch adding dt binding documentation for LMh nodes.
>>>     - Rebased to v5.13
>>>
>>> Thara Gopinath (6):
>>>    firmware: qcom_scm: Introduce SCM calls to access LMh
>>>    thermal: qcom: Add support for LMh driver
>>>    cpufreq: qcom-cpufreq-hw: Add dcvs interrupt support
>>>    arm64: boot: dts: qcom: sdm45: Add support for LMh node
>>>    arm64: boot: dts: qcom: sdm845: Remove cpufreq cooling devices
>>> for CPU
>>>      thermal zones
>>>    dt-bindings: thermal: Add dt binding for QCOM LMh
>>>
>>>   .../devicetree/bindings/thermal/qcom-lmh.yaml | 100 ++++++++
>>>   arch/arm64/boot/dts/qcom/sdm845.dtsi          | 162 ++----------
>>>   drivers/cpufreq/qcom-cpufreq-hw.c             | 118 +++++++++
>>>   drivers/firmware/qcom_scm.c                   |  58 +++++
>>>   drivers/firmware/qcom_scm.h                   |   4 +
>>>   drivers/thermal/qcom/Kconfig                  |  10 +
>>>   drivers/thermal/qcom/Makefile                 |   1 +
>>>   drivers/thermal/qcom/lmh.c                    | 239
>>> ++++++++++++++++++
>>>   include/linux/qcom_scm.h                      |  14 +
>>>   9 files changed, 570 insertions(+), 136 deletions(-)
>>>   create mode 100644
>>> Documentation/devicetree/bindings/thermal/qcom-lmh.yaml
>>>   create mode 100644 drivers/thermal/qcom/lmh.c
>>>
>> I've been using these patches on a 5.13 kernel
>> (https://github.com/steev/linux/tree/linux-5.13.y - while trying to
>> track down a different issue, while playing a video on youtube, as well
>> as compressing a 9.2GB file with xz, I got the following
>
> Hi Steev,
>
> Thanks for testing this. I was unable to reproduce this. I have posted
> v4 moving the interrupt handling in qcom-cpufreq-hw to threaded
> interrupt handler and hopefully this should fix the issue. It will be
> great if you can test and let me know.
>
Hi Thara,

I've been testing v4 for a little bit here, and so far I can't seem to
get it to reproduce anymore.  I will keep trying but fingers crossed
that that did the trick.

For setup, I'm using https://github.com/steev/linux/tree/linux-5.13.y
with the "distro_defconfig" configuration here on my c630s.  I'm also
running https://github.com/steev/scheduler as a systemd service.  So far
I've been able to sleep/suspend without issue while running "make
-j$(nproc) deb-pkg" in those kernel sources as well as `xz
--memlimit-compress=50 -T 4 imagefile.img" on a 9.2GB file at the same
time.  One system is running the Budgie desktop on top of Xorg, and the
other is running Gnome 3.38 on top of Wayland.

-- steev


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

end of thread, other threads:[~2021-07-27 17:43 UTC | newest]

Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-08 12:06 [Patch v3 0/6] Introduce LMh driver for Qualcomm SoCs Thara Gopinath
2021-07-08 12:06 ` [Patch v3 1/6] firmware: qcom_scm: Introduce SCM calls to access LMh Thara Gopinath
2021-07-10  4:02   ` Bjorn Andersson
2021-07-08 12:06 ` [Patch v3 2/6] thermal: qcom: Add support for LMh driver Thara Gopinath
2021-07-10  4:15   ` Bjorn Andersson
2021-07-13  0:49     ` Thara Gopinath
2021-07-08 12:06 ` [Patch v3 3/6] cpufreq: qcom-cpufreq-hw: Add dcvs interrupt support Thara Gopinath
2021-07-09  6:46   ` Viresh Kumar
2021-07-09 15:37     ` Thara Gopinath
2021-07-12  4:35       ` Viresh Kumar
2021-07-12  4:41       ` Viresh Kumar
2021-07-13  1:18         ` Thara Gopinath
2021-07-13  3:18           ` Viresh Kumar
2021-07-14 12:37             ` Thara Gopinath
2021-07-10  4:57   ` Bjorn Andersson
2021-07-13  1:09     ` Thara Gopinath
2021-07-08 12:06 ` [Patch v3 4/6] arm64: boot: dts: qcom: sdm45: Add support for LMh node Thara Gopinath
2021-07-10  4:17   ` Bjorn Andersson
2021-07-19 16:33   ` Bjorn Andersson
2021-07-19 22:44     ` Thara Gopinath
2021-07-08 12:06 ` [Patch v3 5/6] arm64: boot: dts: qcom: sdm845: Remove cpufreq cooling devices for CPU thermal zones Thara Gopinath
2021-07-10  4:17   ` Bjorn Andersson
2021-07-08 12:06 ` [Patch v3 6/6] dt-bindings: thermal: Add dt binding for QCOM LMh Thara Gopinath
2021-07-10  4:21   ` Bjorn Andersson
2021-07-13  0:54     ` Thara Gopinath
2021-07-12 17:32   ` Rob Herring
2021-07-22  3:14 ` [Patch v3 0/6] Introduce LMh driver for Qualcomm SoCs Steev Klimaszewski
2021-07-27 15:29   ` Thara Gopinath
2021-07-27 17:43     ` Steev Klimaszewski

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