[PATCH RFC 0/5] Add support for PDC timer for wake-ups

[PATCH RFC 0/5] Add support for PDC timer for wake-ups

[Raju P.L.S.S.S.N]

RPMH based targets require that the next wake-up timer value needs
to be programmed to PDC (Power Domain Controller) which has its
own timer and is in an always on power domain. PDC wakes-up
the RSC and sets up the resources back in active state before the
processor is woken up by a timer interrupt. In order to query next
timer wake-up, the patch-set depends on patch - Export next wakeup
time of a CPU[1].

The kernel does not notify that the CPU powering down is the last
CPU. So in this version, next wake-up is programmed to PDC each time
when a CPU goes to power collapse. The current approach can be
revisited in future if OS-initiated support becomes available that
enables certain actions to be taken when last core enters deepest low
power mode.

Please review these patches. Your inputs would be greatly appreciated.

Thanks,
Raju.

Dependencies:
 [1].https://lore.kernel.org/patchwork/patch/1019432/

Raju P.L.S.S.S.N (5):
  drivers: qcom: rpmh-rsc: Add regmap for RSC controller
  drivers: qcom: rpmh-pdc-timer: add PDC timer support for RPMH based
    SoCs
  dt-bindings: Add PDC timer bindings for Qualcomm SoCs
  drivers: qcom: rpmh-pdc-timer: Add power management ops
  arm64: dts: msm: add PDC timer for apps_rsc for SDM845

 .../devicetree/bindings/soc/qcom/rpmh-rsc.txt |  29 +++
 arch/arm64/boot/dts/qcom/sdm845.dtsi          |   9 +
 drivers/soc/qcom/Kconfig                      |   9 +
 drivers/soc/qcom/Makefile                     |   1 +
 drivers/soc/qcom/rpmh-pdc-timer.c             | 205 ++++++++++++++++++
 drivers/soc/qcom/rpmh-rsc.c                   |  14 ++
 6 files changed, 267 insertions(+)
 create mode 100644 drivers/soc/qcom/rpmh-pdc-timer.c

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of the Code Aurora Forum, hosted by The Linux Foundation.

[PATCH RFC 1/5] drivers: qcom: rpmh-rsc: Add regmap for RSC controller

[Raju P.L.S.S.S.N]

The RSC controller has dedicated control registers to
send next wakeup timer vale to PDC. The timer value
needs to be programmed by sleep driver client. So add
regmap for the controller.

Signed-off-by: Raju P.L.S.S.S.N <rplsssn@codeaurora.org>
---
 drivers/soc/qcom/rpmh-rsc.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
index 75bd9a83aef0..269fd0866647 100644
--- a/drivers/soc/qcom/rpmh-rsc.c
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -15,6 +15,7 @@
 #include <linux/of_irq.h>
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>
+#include <linux/regmap.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
 
@@ -61,6 +62,13 @@
 #define CMD_STATUS_ISSUED		BIT(8)
 #define CMD_STATUS_COMPL		BIT(16)
 
+static const struct regmap_config rsc_regmap_config = {
+	.reg_bits = 32,
+	.reg_stride = 4,
+	.val_bits = 32,
+	.fast_io = true,
+};
+
 static u32 read_tcs_reg(struct rsc_drv *drv, int reg, int tcs_id, int cmd_id)
 {
 	return readl_relaxed(drv->tcs_base + reg + RSC_DRV_TCS_OFFSET * tcs_id +
@@ -534,6 +542,7 @@ static int rpmh_probe_tcs_config(struct platform_device *pdev,
 	struct tcs_group *tcs;
 	struct resource *res;
 	void __iomem *base;
+	struct regmap *regmap;
 	char drv_id[10] = {0};
 
 	snprintf(drv_id, ARRAY_SIZE(drv_id), "drv-%d", drv->id);
@@ -542,6 +551,11 @@ static int rpmh_probe_tcs_config(struct platform_device *pdev,
 	if (IS_ERR(base))
 		return PTR_ERR(base);
 
+	regmap = devm_regmap_init_mmio(&pdev->dev, base,
+					&rsc_regmap_config);
+	if (IS_ERR(regmap))
+		return PTR_ERR(regmap);
+
 	ret = of_property_read_u32(dn, "qcom,tcs-offset", &offset);
 	if (ret)
 		return ret;
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of the Code Aurora Forum, hosted by The Linux Foundation.

[PATCH RFC 2/5] drivers: qcom: rpmh-pdc-timer: add PDC timer support for RPMH based SoCs

[Raju P.L.S.S.S.N]

RPMH based targets require that the next wake-up timer value needs
to be programmed to PDC (Power Domain Controller) which has its
own timer and is in an always on power domain. PDC wakes-up
the RSC and sets up the resources back in active state before the
processor is woken up by a timer interrupt.

Signed-off-by: Raju P.L.S.S.S.N <rplsssn@codeaurora.org>
---
 drivers/soc/qcom/Kconfig          |   9 ++
 drivers/soc/qcom/Makefile         |   1 +
 drivers/soc/qcom/rpmh-pdc-timer.c | 181 ++++++++++++++++++++++++++++++
 3 files changed, 191 insertions(+)
 create mode 100644 drivers/soc/qcom/rpmh-pdc-timer.c

diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index 684cb51694d1..d04724ea5490 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -103,6 +103,15 @@ config QCOM_RPMH
 	  of hardware components aggregate requests for these resources and
 	  help apply the aggregated state on the resource.
 
+config QCOM_RPMH_PDC_TIMER
+	bool "Qualcomm PDC Timer for RPM-Hardened based SoCs"
+	depends on CPU_PM
+	help
+	  Support for QCOM platform next wakeup timer programming when
+	  application processor enters SoC level deepest low power mode.
+	  The wakeup is programmed to PDC (Power Domain Controller)
+	  timer which is in always on power domain.
+
 config QCOM_SMEM
 	tristate "Qualcomm Shared Memory Manager (SMEM)"
 	depends on ARCH_QCOM || COMPILE_TEST
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index f25b54cd6cf8..2ddb7e4f9098 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -12,6 +12,7 @@ obj-$(CONFIG_QCOM_RMTFS_MEM)	+= rmtfs_mem.o
 obj-$(CONFIG_QCOM_RPMH)		+= qcom_rpmh.o
 qcom_rpmh-y			+= rpmh-rsc.o
 qcom_rpmh-y			+= rpmh.o
+obj-$(CONFIG_QCOM_RPMH_PDC_TIMER) += rpmh-pdc-timer.o
 obj-$(CONFIG_QCOM_SMD_RPM)	+= smd-rpm.o
 obj-$(CONFIG_QCOM_SMEM) +=	smem.o
 obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o
diff --git a/drivers/soc/qcom/rpmh-pdc-timer.c b/drivers/soc/qcom/rpmh-pdc-timer.c
new file mode 100644
index 000000000000..108ea4a2df89
--- /dev/null
+++ b/drivers/soc/qcom/rpmh-pdc-timer.c
@@ -0,0 +1,181 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018, The Linux Foundation. All rights reserved.
+ */
+
+#include <linux/cpu_pm.h>
+#include <linux/kernel.h>
+#include <linux/ktime.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/tick.h>
+#include <asm/arch_timer.h>
+
+#include <soc/qcom/rpmh.h>
+
+#define ARCH_TIMER_HZ (19200000)
+#define PDC_TIME_VALID_SHIFT	31
+#define PDC_TIME_UPPER_MASK	0xFFFFFF
+
+static struct regmap *rsc_regmap;
+static resource_size_t cmd0_data_offset;
+static resource_size_t cmd1_data_offset;
+static uint64_t pdc_wakeup = ~0ULL;
+static raw_spinlock_t pdc_wakeup_lock;
+
+/* convert micro sec to ticks or clock cycles
+ *
+ *   time in cycles = (time in sec) * Freq
+ *                  = (time in sec) * 19200000
+ *
+ * However, while converting micro sec to sec,
+ * there is a possibility of round of errors.
+ * So round of errors are minimized by finding
+ * nano sec.
+ */
+static uint64_t us_to_cycles(uint64_t time_us)
+{
+	uint64_t sec, nsec, time_cycles;
+
+	sec = time_us;
+	do_div(sec, USEC_PER_SEC);
+	nsec = time_us - sec * USEC_PER_SEC;
+
+	if (nsec > 0) {
+		nsec = nsec * NSEC_PER_USEC;
+		do_div(nsec, NSEC_PER_SEC);
+	}
+
+	sec += nsec;
+
+	time_cycles = (u64)sec * ARCH_TIMER_HZ;
+
+	return time_cycles;
+}
+
+/*
+ * Find next wakeup of a cpu and convert into
+ * cycles.
+ */
+static uint64_t get_next_wakeup_cycles(int cpu)
+{
+	ktime_t next_wakeup;
+	uint64_t next_wakeup_cycles;
+
+	next_wakeup = tick_nohz_get_next_wakeup(cpu);
+
+	/*
+	 * Find the relative wakeup from current time
+	 * in kernel time scale
+	 */
+	next_wakeup = ktime_sub(next_wakeup, ktime_get());
+
+	next_wakeup_cycles = us_to_cycles(ktime_to_us(next_wakeup));
+
+	/*
+	 * Add current time in arch timer scale.
+	 * This is needed as PDC match value is programmed
+	 * with absolute value, not the relative value
+	 * from current time
+	 */
+	next_wakeup_cycles += arch_counter_get_cntvct();
+
+	return next_wakeup_cycles;
+}
+
+static void setup_pdc_wakeup_timer(uint64_t wakeup_cycles)
+{
+	u32 data0, data1;
+
+	data0 =  (wakeup_cycles >> 32) & PDC_TIME_UPPER_MASK;
+	data0 |= 1 << PDC_TIME_VALID_SHIFT;
+	data1 = (wakeup_cycles & 0xFFFFFFFF);
+
+	regmap_write(rsc_regmap, cmd0_data_offset, data0);
+	regmap_write(rsc_regmap, cmd1_data_offset, data1);
+
+}
+
+static int cpu_pm_notifier(struct notifier_block *b,
+			       unsigned long cmd, void *v)
+{
+	uint64_t cpu_next_wakeup;
+
+	switch (cmd) {
+	case CPU_PM_ENTER:
+		cpu_next_wakeup = get_next_wakeup_cycles(smp_processor_id());
+		raw_spin_lock(&pdc_wakeup_lock);
+		/*
+		 * If PDC wakeup is expired or
+		 * If current cpu next wakeup is early,
+		 * program the same to pdc wakeup
+		 */
+		if ((pdc_wakeup < arch_counter_get_cntvct()) ||
+				(cpu_next_wakeup < pdc_wakeup)) {
+			pdc_wakeup = cpu_next_wakeup;
+			setup_pdc_wakeup_timer(pdc_wakeup);
+		}
+		raw_spin_unlock(&pdc_wakeup_lock);
+		break;
+	default:
+		return NOTIFY_DONE;
+	}
+
+	return NOTIFY_OK;
+}
+
+static struct notifier_block cpu_pm_notifier_block = {
+	.notifier_call = cpu_pm_notifier,
+	.priority = -1, /* Should be last in the order of notifications */
+};
+
+static int pdc_timer_probe(struct platform_device *pdev)
+{
+	struct device *pdc_timer_dev = &pdev->dev;
+	struct resource *res = NULL;
+
+	if (IS_ERR_OR_NULL(pdc_timer_dev)) {
+		pr_err("%s fail\n", __func__);
+		return PTR_ERR(pdc_timer_dev);
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		pr_err("res not found\n");
+		return -ENODEV;
+	}
+	cmd0_data_offset = res->start;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	if (!res) {
+		pr_err("res not found\n");
+		return -ENODEV;
+	}
+	cmd1_data_offset =  res->start;
+
+	rsc_regmap = dev_get_regmap(pdc_timer_dev->parent, NULL);
+	if (!rsc_regmap) {
+		pr_err("regmap for parent is not found\n");
+		return -ENODEV;
+	}
+
+	raw_spin_lock_init(&pdc_wakeup_lock);
+	cpu_pm_register_notifier(&cpu_pm_notifier_block);
+
+	return 0;
+}
+
+static const struct of_device_id pdc_timer_drv_match[] = {
+	{ .compatible = "qcom,pdc-timer", },
+	{ }
+};
+
+static struct platform_driver pdc_timer_driver = {
+	.probe = pdc_timer_probe,
+	.driver = {
+		.name = KBUILD_MODNAME,
+		.of_match_table = pdc_timer_drv_match,
+	},
+};
+builtin_platform_driver(pdc_timer_driver);
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of the Code Aurora Forum, hosted by The Linux Foundation.

[PATCH RFC 3/5] dt-bindings: Add PDC timer bindings for Qualcomm SoCs

[Raju P.L.S.S.S.N]

Add device binding documentation for Qualcomm Technology Inc's PDC timer.
The driver is used for programming next wake-up timer value when processor
enters SoC level deepest low power state.

Cc: devicetree@vger.kernel.org
Signed-off-by: Raju P.L.S.S.S.N <rplsssn@codeaurora.org>
---
 .../devicetree/bindings/soc/qcom/rpmh-rsc.txt | 29 +++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/Documentation/devicetree/bindings/soc/qcom/rpmh-rsc.txt b/Documentation/devicetree/bindings/soc/qcom/rpmh-rsc.txt
index 9b86d1eff219..f24afb45d0d9 100644
--- a/Documentation/devicetree/bindings/soc/qcom/rpmh-rsc.txt
+++ b/Documentation/devicetree/bindings/soc/qcom/rpmh-rsc.txt
@@ -30,6 +30,12 @@ will be an aggregate of the sleep votes from each of those subsystems. Clients
 may request a sleep value for their shared resources in addition to the active
 mode requests.
 
+When the processor enters deepest low power mode, the next wake-up timer value
+needs to be programmed to PDC (Power Domain Controller) through RSC registers
+dedicated for this purpose. PDC timer is specified as child node of RSC with
+register offets to program timer match value.
+
+
 Properties:
 
 - compatible:
@@ -86,6 +92,20 @@ Properties:
 Drivers that want to use the RSC to communicate with RPMH must specify their
 bindings as child nodes of the RSC controllers they wish to communicate with.
 
+If an RSC needs to program next wake-up in the PDC timer, it must specify the
+binding as child node with the following properties:
+
+Properties:
+- compatible:
+    Usage: required
+	Value type: <string>
+	Definition: must be "qcom,pdc-timer".
+
+- reg:
+    Usage: required
+	Value type: <prop-encoded-array>
+	Definition: Specifies the offset of the control register.
+
 Example 1:
 
 For a TCS whose RSC base address is is 0x179C0000 and is at a DRV id of 2, the
@@ -103,6 +123,9 @@ TCS-OFFSET: 0xD00
 		      <0x179d0000 0x10000>,
 		      <0x179e0000 0x10000>;
 		reg-names = "drv-0", "drv-1", "drv-2";
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges;
 		interrupts = <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>,
 			     <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>,
 			     <GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>;
@@ -112,6 +135,12 @@ TCS-OFFSET: 0xD00
 				  <SLEEP_TCS   3>,
 				  <WAKE_TCS    3>,
 				  <CONTROL_TCS 1>;
+
+		pdc_timer@38 {
+			compatible = "qcom,pdc-timer";
+			reg = <0x38 0x1>,
+			      <0x40 0x1>;
+		}
 	};
 
 Example 2:
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of the Code Aurora Forum, hosted by The Linux Foundation.

[PATCH RFC 4/5] drivers: qcom: rpmh-pdc-timer: Add power management ops

[Raju P.L.S.S.S.N]

Add suspend power management ops so that the PDC timer is programmed
to highest match value when system is suspended.

Signed-off-by: Raju P.L.S.S.S.N <rplsssn@codeaurora.org>
---
 drivers/soc/qcom/Kconfig          |  2 +-
 drivers/soc/qcom/rpmh-pdc-timer.c | 24 ++++++++++++++++++++++++
 2 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index d04724ea5490..c6ef41a06c97 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -105,7 +105,7 @@ config QCOM_RPMH
 
 config QCOM_RPMH_PDC_TIMER
 	bool "Qualcomm PDC Timer for RPM-Hardened based SoCs"
-	depends on CPU_PM
+	depends on CPU_PM && PM_SLEEP
 	help
 	  Support for QCOM platform next wakeup timer programming when
 	  application processor enters SoC level deepest low power mode.
diff --git a/drivers/soc/qcom/rpmh-pdc-timer.c b/drivers/soc/qcom/rpmh-pdc-timer.c
index 108ea4a2df89..ebb643924a3e 100644
--- a/drivers/soc/qcom/rpmh-pdc-timer.c
+++ b/drivers/soc/qcom/rpmh-pdc-timer.c
@@ -23,6 +23,7 @@ static resource_size_t cmd0_data_offset;
 static resource_size_t cmd1_data_offset;
 static uint64_t pdc_wakeup = ~0ULL;
 static raw_spinlock_t pdc_wakeup_lock;
+static int suspended;
 
 /* convert micro sec to ticks or clock cycles
  *
@@ -102,6 +103,9 @@ static int cpu_pm_notifier(struct notifier_block *b,
 {
 	uint64_t cpu_next_wakeup;
 
+	if (suspended)
+		return NOTIFY_DONE;
+
 	switch (cmd) {
 	case CPU_PM_ENTER:
 		cpu_next_wakeup = get_next_wakeup_cycles(smp_processor_id());
@@ -130,6 +134,25 @@ static struct notifier_block cpu_pm_notifier_block = {
 	.priority = -1, /* Should be last in the order of notifications */
 };
 
+static int pdc_timer_suspend(struct device *dev)
+{
+	suspended = true;
+	raw_spin_lock(&pdc_wakeup_lock);
+	pdc_wakeup = ~0ULL;
+	setup_pdc_wakeup_timer(pdc_wakeup);
+	raw_spin_unlock(&pdc_wakeup_lock);
+	return 0;
+}
+
+static int pdc_timer_resume(struct device *dev)
+{
+	suspended = false;
+	return 0;
+}
+static const struct dev_pm_ops pdc_timer_dev_pm_ops = {
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pdc_timer_suspend, pdc_timer_resume)
+};
+
 static int pdc_timer_probe(struct platform_device *pdev)
 {
 	struct device *pdc_timer_dev = &pdev->dev;
@@ -176,6 +199,7 @@ static struct platform_driver pdc_timer_driver = {
 	.driver = {
 		.name = KBUILD_MODNAME,
 		.of_match_table = pdc_timer_drv_match,
+		.pm = &pdc_timer_dev_pm_ops,
 	},
 };
 builtin_platform_driver(pdc_timer_driver);
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of the Code Aurora Forum, hosted by The Linux Foundation.

[PATCH RFC 5/5] arm64: dts: msm: add PDC timer for apps_rsc for SDM845

[Raju P.L.S.S.S.N]

Add PDC timer node for apps_rsc to program next wake-up
timer value.

Signed-off-by: Raju P.L.S.S.S.N <rplsssn@codeaurora.org>
---
 arch/arm64/boot/dts/qcom/sdm845.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index b256357e4db1..c73b717ed8ea 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -1372,6 +1372,9 @@
 			      <0x179d0000 0x10000>,
 			      <0x179e0000 0x10000>;
 			reg-names = "drv-0", "drv-1", "drv-2";
+			#address-cells = <1>;
+			#size-cells = <1>;
+			ranges;
 			interrupts = <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>,
 				     <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>,
 				     <GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>;
@@ -1382,6 +1385,12 @@
 					  <WAKE_TCS    3>,
 					  <CONTROL_TCS 1>;
 
+			pdc_timer@38 {
+				compatible = "qcom,pdc-timer";
+				reg = <0x38 0x1>,
+				      <0x40 0x1>;
+			};
+
 			rpmhcc: clock-controller {
 				compatible = "qcom,sdm845-rpmh-clk";
 				#clock-cells = <1>;
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of the Code Aurora Forum, hosted by The Linux Foundation.

Re: [PATCH RFC 3/5] dt-bindings: Add PDC timer bindings for Qualcomm SoCs

[Stephen Boyd]

Quoting Raju P.L.S.S.S.N (2018-12-21 03:59:44)
> diff --git a/Documentation/devicetree/bindings/soc/qcom/rpmh-rsc.txt b/Documentation/devicetree/bindings/soc/qcom/rpmh-rsc.txt
> index 9b86d1eff219..f24afb45d0d9 100644
> --- a/Documentation/devicetree/bindings/soc/qcom/rpmh-rsc.txt
> +++ b/Documentation/devicetree/bindings/soc/qcom/rpmh-rsc.txt
> @@ -30,6 +30,12 @@ will be an aggregate of the sleep votes from each of those subsystems. Clients
>  may request a sleep value for their shared resources in addition to the active
>  mode requests.
>  
> +When the processor enters deepest low power mode, the next wake-up timer value
> +needs to be programmed to PDC (Power Domain Controller) through RSC registers
> +dedicated for this purpose. PDC timer is specified as child node of RSC with
> +register offets to program timer match value.

That's great info, but I have no idea why it's in the DT binding
document.

> +
> +
>  Properties:
>  
>  - compatible:
> @@ -86,6 +92,20 @@ Properties:
>  Drivers that want to use the RSC to communicate with RPMH must specify their
>  bindings as child nodes of the RSC controllers they wish to communicate with.
>  
> +If an RSC needs to program next wake-up in the PDC timer, it must specify the
> +binding as child node with the following properties:
> +
> +Properties:
> +- compatible:
> +    Usage: required
> +       Value type: <string>
> +       Definition: must be "qcom,pdc-timer".
> +
> +- reg:
> +    Usage: required
> +       Value type: <prop-encoded-array>
> +       Definition: Specifies the offset of the control register.
> +
>  Example 1:
>  
>  For a TCS whose RSC base address is is 0x179C0000 and is at a DRV id of 2, the
> @@ -103,6 +123,9 @@ TCS-OFFSET: 0xD00
>                       <0x179d0000 0x10000>,
>                       <0x179e0000 0x10000>;
>                 reg-names = "drv-0", "drv-1", "drv-2";
> +               #address-cells = <1>;
> +               #size-cells = <1>;
> +               ranges;
>                 interrupts = <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>,
>                              <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>,
>                              <GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>;
> @@ -112,6 +135,12 @@ TCS-OFFSET: 0xD00
>                                   <SLEEP_TCS   3>,
>                                   <WAKE_TCS    3>,
>                                   <CONTROL_TCS 1>;
> +
> +               pdc_timer@38 {
> +                       compatible = "qcom,pdc-timer";
> +                       reg = <0x38 0x1>,
> +                             <0x40 0x1>;

I don't understand this whole binding. Why can't the pdc timer be
programmed within the rpmh driver? This looks like a node is being added
as a child just to make a platform driver and device match up in the
linux kernel. And that in turn causes a regmap to need to be created?
Sorry, it just looks really bad.

At least for the regulators we have a semi-good reason to have a subnode
because of the pmic-id property that we need to match up to the right
pmic. The argument for the rpmh clock node is unclear to me so we should
probably get rid of that node entirely. I can't figure out why that
wasn't just a #clock-cells at the toplevel rsc node. The same goes for
the interconnect and rpmhpd bindings. It's all sub-nodes to placate
linux device driver model.

And now that I've had to look at the rpmh binding again I'm disappointed
that we have SoC data stored in there with the qcom,tcs-offset and
qcom,tcs-config properties. Just make an SoC compatible like
qcom,rpmh-rsc-sdm845" and figure these things out that way. And please
tell hardware folks to stop moving the register regions around in the
future so that there doesn't have to be so many variants in the driver
and compatible string space.

Re: [PATCH RFC 3/5] dt-bindings: Add PDC timer bindings for Qualcomm SoCs

[Raju P L S S S N]

On 12/22/2018 1:09 PM, Stephen Boyd wrote:
>> +If an RSC needs to program next wake-up in the PDC timer, it must specify the
>> +binding as child node with the following properties:
>> +
>> +Properties:
>> +- compatible:
>> +    Usage: required
>> +       Value type: <string>
>> +       Definition: must be "qcom,pdc-timer".
>> +
>> +- reg:
>> +    Usage: required
>> +       Value type: <prop-encoded-array>
>> +       Definition: Specifies the offset of the control register.
>> +
>>   Example 1:
>>   
>>   For a TCS whose RSC base address is is 0x179C0000 and is at a DRV id of 2, the
>> @@ -103,6 +123,9 @@ TCS-OFFSET: 0xD00
>>                        <0x179d0000 0x10000>,
>>                        <0x179e0000 0x10000>;
>>                  reg-names = "drv-0", "drv-1", "drv-2";
>> +               #address-cells = <1>;
>> +               #size-cells = <1>;
>> +               ranges;
>>                  interrupts = <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>,
>>                               <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>,
>>                               <GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>;
>> @@ -112,6 +135,12 @@ TCS-OFFSET: 0xD00
>>                                    <SLEEP_TCS   3>,
>>                                    <WAKE_TCS    3>,
>>                                    <CONTROL_TCS 1>;
>> +
>> +               pdc_timer@38 {
>> +                       compatible = "qcom,pdc-timer";
>> +                       reg = <0x38 0x1>,
>> +                             <0x40 0x1>;
> I don't understand this whole binding. Why can't the pdc timer be
> programmed within the rpmh driver? This looks like a node is being added
> as a child just to make a platform driver and device match up in the
> linux kernel. And that in turn causes a regmap to need to be created?
> Sorry, it just looks really bad.


There are two RSC devices in SoC one for application processor subsystem 
& other display subsystem. Both RSC contain registers for PDC timers 
(one for each subsystem). But only for application processor the PDC 
timer needs to be programmed when application processor enters 
sleep/suspend. As the driver is common between both RSC devices, this 
approach is taken. Do you have any other suggestions to distinguish 
between the two? Perhaps, by additional compatible string?

Thanks for the review.

- Raju

Re: [PATCH RFC 3/5] dt-bindings: Add PDC timer bindings for Qualcomm SoCs

[Stephen Boyd]

Quoting Raju P L S S S N (2018-12-26 01:44:43)
> 
> 
> On 12/22/2018 1:09 PM, Stephen Boyd wrote:
> >> +If an RSC needs to program next wake-up in the PDC timer, it must specify the
> >> +binding as child node with the following properties:
> >> +
> >> +Properties:
> >> +- compatible:
> >> +    Usage: required
> >> +       Value type: <string>
> >> +       Definition: must be "qcom,pdc-timer".
> >> +
> >> +- reg:
> >> +    Usage: required
> >> +       Value type: <prop-encoded-array>
> >> +       Definition: Specifies the offset of the control register.
> >> +
> >>   Example 1:
> >>   
> >>   For a TCS whose RSC base address is is 0x179C0000 and is at a DRV id of 2, the
> >> @@ -103,6 +123,9 @@ TCS-OFFSET: 0xD00
> >>                        <0x179d0000 0x10000>,
> >>                        <0x179e0000 0x10000>;
> >>                  reg-names = "drv-0", "drv-1", "drv-2";
> >> +               #address-cells = <1>;
> >> +               #size-cells = <1>;
> >> +               ranges;
> >>                  interrupts = <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>,
> >>                               <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>,
> >>                               <GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>;
> >> @@ -112,6 +135,12 @@ TCS-OFFSET: 0xD00
> >>                                    <SLEEP_TCS   3>,
> >>                                    <WAKE_TCS    3>,
> >>                                    <CONTROL_TCS 1>;
> >> +
> >> +               pdc_timer@38 {
> >> +                       compatible = "qcom,pdc-timer";
> >> +                       reg = <0x38 0x1>,
> >> +                             <0x40 0x1>;
> > I don't understand this whole binding. Why can't the pdc timer be
> > programmed within the rpmh driver? This looks like a node is being added
> > as a child just to make a platform driver and device match up in the
> > linux kernel. And that in turn causes a regmap to need to be created?
> > Sorry, it just looks really bad.
> 
> 
> There are two RSC devices in SoC one for application processor subsystem 
> & other display subsystem. Both RSC contain registers for PDC timers 
> (one for each subsystem).

When is the timer programmed on the display subsystem's RSC? It's hard
to give advice without all the information.

> But only for application processor the PDC 
> timer needs to be programmed when application processor enters 
> sleep/suspend. As the driver is common between both RSC devices, this 
> approach is taken. Do you have any other suggestions to distinguish 
> between the two? Perhaps, by additional compatible string?
> 

Maybe compatible? I sort of doubt it though. Do all RSCs have a PDC
timer?

I would think that it would make sense for the application processor's
RSC timer to be programmed from the broadcast timer mechanism in the
kernel so that timers during idle work and suspend turns off the timer
appropriately with a shutdown hook. I guess the PDC can't tell you the
time though? It looks like a shadow (and limited) version of the ARM
architected MMIO timer that we already program for the broadcast timer
mechanism. Is that because even the MMIO timer can't wakeup the system
in deep idle?  Assuming that's true, it means the ARM MMIO timer can't
always be used as the system wide broadcast mechanism because we need to
augment it with the PDC timer to get the actual wakeup.

Maybe we should be adding hooks into the broadcast timer mechanism to
program this wakeup event hardware in addition to the ARM MMIO timer. Or
we should stop using the ARM MMIO timer on these systems and read the
system register based physical time in the RSC timer driver and register
this 64-bit PDC register as the broadcast timer. So the time reading
would be through sysreg and the wakeup programming would be done by
writing the PDC timer. The assumption would be that we have access to
the physical time registers (which sounds like the assumption we have to
make).

Do we get an interrupt somewhere from the RSC hardware when the timer
fires? Or does that just cause a system wakeup event without any pending
irq and then another irq (like the ARM architected timer) just happens
to be pending around the same time? If we get an interrupt somehow then
I would prefer to drop the ARM MMIO timer and do this hybrid broadcast
timer approach.

How the RSC is used in general by other devices, like display, is not
clear to me. We don't have a "wakeup event" framework in the kernel that
device drivers like the display driver can grab a reference to and
program some system wide wakeup for. That sounds like something new that
could be handled entirely in the display driver with direct register
writes, or it could be some qcom specific API/framework that eventually
calls down into the same RSC driver that knows what offsets to write
into in the display RSC's register space, or it could be an entirely
generic framework like clk or regulator frameworks that could be used by
anything. BTW, are we using the display RSC yet?

Re: [PATCH RFC 3/5] dt-bindings: Add PDC timer bindings for Qualcomm SoCs

[Raju P L S S S N]

On 12/29/2018 3:08 AM, Stephen Boyd wrote:
> Quoting Raju P L S S S N (2018-12-26 01:44:43)
>>
>>
>> On 12/22/2018 1:09 PM, Stephen Boyd wrote:
>>>> +If an RSC needs to program next wake-up in the PDC timer, it must specify the
>>>> +binding as child node with the following properties:
>>>> +
>>>> +Properties:
>>>> +- compatible:
>>>> +    Usage: required
>>>> +       Value type: <string>
>>>> +       Definition: must be "qcom,pdc-timer".
>>>> +
>>>> +- reg:
>>>> +    Usage: required
>>>> +       Value type: <prop-encoded-array>
>>>> +       Definition: Specifies the offset of the control register.
>>>> +
>>>>    Example 1:
>>>>    
>>>>    For a TCS whose RSC base address is is 0x179C0000 and is at a DRV id of 2, the
>>>> @@ -103,6 +123,9 @@ TCS-OFFSET: 0xD00
>>>>                         <0x179d0000 0x10000>,
>>>>                         <0x179e0000 0x10000>;
>>>>                   reg-names = "drv-0", "drv-1", "drv-2";
>>>> +               #address-cells = <1>;
>>>> +               #size-cells = <1>;
>>>> +               ranges;
>>>>                   interrupts = <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>,
>>>>                                <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>,
>>>>                                <GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>;
>>>> @@ -112,6 +135,12 @@ TCS-OFFSET: 0xD00
>>>>                                     <SLEEP_TCS   3>,
>>>>                                     <WAKE_TCS    3>,
>>>>                                     <CONTROL_TCS 1>;
>>>> +
>>>> +               pdc_timer@38 {
>>>> +                       compatible = "qcom,pdc-timer";
>>>> +                       reg = <0x38 0x1>,
>>>> +                             <0x40 0x1>;
>>> I don't understand this whole binding. Why can't the pdc timer be
>>> programmed within the rpmh driver? This looks like a node is being added
>>> as a child just to make a platform driver and device match up in the
>>> linux kernel. And that in turn causes a regmap to need to be created?
>>> Sorry, it just looks really bad.
>>
>>
>> There are two RSC devices in SoC one for application processor subsystem
>> & other display subsystem. Both RSC contain registers for PDC timers
>> (one for each subsystem).
> 
> When is the timer programmed on the display subsystem's RSC? It's hard
> to give advice without all the information.

For display subsystem RSC, hardware sleep solver takes care of timer 
programming for wakeup when the subsystem goes to Power collapse.

> 
>> But only for application processor the PDC
>> timer needs to be programmed when application processor enters
>> sleep/suspend. As the driver is common between both RSC devices, this
>> approach is taken. Do you have any other suggestions to distinguish
>> between the two? Perhaps, by additional compatible string?
>>
> 
> Maybe compatible? I sort of doubt it though. Do all RSCs have a PDC
> timer?

Yes. all RSCs have their own PDC timer.

> 
> I would think that it would make sense for the application processor's
> RSC timer to be programmed from the broadcast timer mechanism in the
> kernel so that timers during idle work and suspend turns off the timer
> appropriately with a shutdown hook. I guess the PDC can't tell you the
> time though? It looks like a shadow (and limited) version of the ARM
> architected MMIO timer that we already program for the broadcast timer
> mechanism. Is that because even the MMIO timer can't wakeup the system
> in deep idle?  Assuming that's true, it means the ARM MMIO timer can't
> always be used as the system wide broadcast mechanism because we need to
> augment it with the PDC timer to get the actual wakeup.
> 

Yes. this is correct.

> Maybe we should be adding hooks into the broadcast timer mechanism to
> program this wakeup event hardware in addition to the ARM MMIO timer. Or
> we should stop using the ARM MMIO timer on these systems and read the
> system register based physical time in the RSC timer driver and register
> this 64-bit PDC register as the broadcast timer. So the time reading
> would be through sysreg and the wakeup programming would be done by
> writing the PDC timer. The assumption would be that we have access to
> the physical time registers (which sounds like the assumption we have to
> make).

There are no physical timer registers available in RSC for this purpose.

> 
> Do we get an interrupt somewhere from the RSC hardware when the timer
> fires? Or does that just cause a system wakeup event without any pending
> irq and then another irq (like the ARM architected timer) just happens
> to be pending around the same time? If we get an interrupt somehow then
> I would prefer to drop the ARM MMIO timer and do this hybrid broadcast
> timer approach.

There is no interrupt for PDC timeout. It just causes system wakeup 
without a pending irq. ARM MMIO is necessary for irq.

> 
> How the RSC is used in general by other devices, like display, is not
> clear to me. We don't have a "wakeup event" framework in the kernel that
> device drivers like the display driver can grab a reference to and
> program some system wide wakeup for. That sounds like something new that
> could be handled entirely in the display driver with direct register
> writes, or it could be some qcom specific API/framework that eventually
> calls down into the same RSC driver that knows what offsets to write
> into in the display RSC's register space, or it could be an entirely
> generic framework like clk or regulator frameworks that could be used by
> anything. BTW, are we using the display RSC yet?
> 

Only display subsystem RSC is programmed along with CPU RSC in Linux. 
display RSC instance is not present in upstream but it is present in 
downstream and used for resource communication purpose only.

Re: [PATCH RFC 3/5] dt-bindings: Add PDC timer bindings for Qualcomm SoCs

[Stephen Boyd]

Quoting Raju P L S S S N (2019-01-03 04:22:58)
> 
> On 12/29/2018 3:08 AM, Stephen Boyd wrote:
> > Quoting Raju P L S S S N (2018-12-26 01:44:43)
> >>
> >> There are two RSC devices in SoC one for application processor subsystem
> >> & other display subsystem. Both RSC contain registers for PDC timers
> >> (one for each subsystem).
> > 
> > When is the timer programmed on the display subsystem's RSC? It's hard
> > to give advice without all the information.
> 
> For display subsystem RSC, hardware sleep solver takes care of timer 
> programming for wakeup when the subsystem goes to Power collapse.

So the display subsystem doesn't need to program their PDC in their RSC
from software?

> > 
> > I would think that it would make sense for the application processor's
> > RSC timer to be programmed from the broadcast timer mechanism in the
> > kernel so that timers during idle work and suspend turns off the timer
> > appropriately with a shutdown hook. I guess the PDC can't tell you the
> > time though? It looks like a shadow (and limited) version of the ARM
> > architected MMIO timer that we already program for the broadcast timer
> > mechanism. Is that because even the MMIO timer can't wakeup the system
> > in deep idle?  Assuming that's true, it means the ARM MMIO timer can't
> > always be used as the system wide broadcast mechanism because we need to
> > augment it with the PDC timer to get the actual wakeup.
> > 
> 
> Yes. this is correct.
> 
> > Maybe we should be adding hooks into the broadcast timer mechanism to
> > program this wakeup event hardware in addition to the ARM MMIO timer. Or
> > we should stop using the ARM MMIO timer on these systems and read the
> > system register based physical time in the RSC timer driver and register
> > this 64-bit PDC register as the broadcast timer. So the time reading
> > would be through sysreg and the wakeup programming would be done by
> > writing the PDC timer. The assumption would be that we have access to
> > the physical time registers (which sounds like the assumption we have to
> > make).
> 
> There are no physical timer registers available in RSC for this purpose.
> 
> > 
> > Do we get an interrupt somewhere from the RSC hardware when the timer
> > fires? Or does that just cause a system wakeup event without any pending
> > irq and then another irq (like the ARM architected timer) just happens
> > to be pending around the same time? If we get an interrupt somehow then
> > I would prefer to drop the ARM MMIO timer and do this hybrid broadcast
> > timer approach.
> 
> There is no interrupt for PDC timeout. It just causes system wakeup 
> without a pending irq. ARM MMIO is necessary for irq.

Does that system wakeup cause the CPUs to be enabled? So the sysreg
based timer in the CPU would start working? Or does it only make the
system come out of a deep enough idle state to make an always on domain
work that only contains the MMIO based ARM architected timer?

I'd hope that each RSC's PDC timer wakes up the owner of the RSC so that
we can use the sysreg based timers and ignore the MMIO based timers
here. This isn't a very important distinction to make though, so if you
have to use the MMIO timers then it just means some extra DT things need
to be done to relate the MMIO timers with the RSC that has the timer
that needs to be programmed too.

Either way we would need a way to either hook the ARM architected timer
driver in the kernel, or reimplement the bit of code needed to implement
the clockevent based on the ARM architected timer that programs the ARM
timer and the PDC timer together.

> 
> > 
> > How the RSC is used in general by other devices, like display, is not
> > clear to me. We don't have a "wakeup event" framework in the kernel that
> > device drivers like the display driver can grab a reference to and
> > program some system wide wakeup for. That sounds like something new that
> > could be handled entirely in the display driver with direct register
> > writes, or it could be some qcom specific API/framework that eventually
> > calls down into the same RSC driver that knows what offsets to write
> > into in the display RSC's register space, or it could be an entirely
> > generic framework like clk or regulator frameworks that could be used by
> > anything. BTW, are we using the display RSC yet?
> > 
> 
> Only display subsystem RSC is programmed along with CPU RSC in Linux. 
> display RSC instance is not present in upstream but it is present in 
> downstream and used for resource communication purpose only.

From the comment above it sounds like the display RSC handles the wakeup
programming in hardware? So there isn't a need to add a 'wakeup event'
framework or anything like that. Please correct me if I'm wrong.

Re: [PATCH RFC 3/5] dt-bindings: Add PDC timer bindings for Qualcomm SoCs

[Raju P L S S S N]

On 1/4/2019 2:49 AM, Stephen Boyd wrote:
> Quoting Raju P L S S S N (2019-01-03 04:22:58)
>>
>> On 12/29/2018 3:08 AM, Stephen Boyd wrote:
>>> Quoting Raju P L S S S N (2018-12-26 01:44:43)
>>>>
>>>> There are two RSC devices in SoC one for application processor subsystem
>>>> & other display subsystem. Both RSC contain registers for PDC timers
>>>> (one for each subsystem).
>>>
>>> When is the timer programmed on the display subsystem's RSC? It's hard
>>> to give advice without all the information.
>>
>> For display subsystem RSC, hardware sleep solver takes care of timer
>> programming for wakeup when the subsystem goes to Power collapse.
> 
> So the display subsystem doesn't need to program their PDC in their RSC
> from software?
Yes.

> 
>>>
>>> I would think that it would make sense for the application processor's
>>> RSC timer to be programmed from the broadcast timer mechanism in the
>>> kernel so that timers during idle work and suspend turns off the timer
>>> appropriately with a shutdown hook. I guess the PDC can't tell you the
>>> time though? It looks like a shadow (and limited) version of the ARM
>>> architected MMIO timer that we already program for the broadcast timer
>>> mechanism. Is that because even the MMIO timer can't wakeup the system
>>> in deep idle?  Assuming that's true, it means the ARM MMIO timer can't
>>> always be used as the system wide broadcast mechanism because we need to
>>> augment it with the PDC timer to get the actual wakeup.
>>>
>>
>> Yes. this is correct.
>>
>>> Maybe we should be adding hooks into the broadcast timer mechanism to
>>> program this wakeup event hardware in addition to the ARM MMIO timer. Or
>>> we should stop using the ARM MMIO timer on these systems and read the
>>> system register based physical time in the RSC timer driver and register
>>> this 64-bit PDC register as the broadcast timer. So the time reading
>>> would be through sysreg and the wakeup programming would be done by
>>> writing the PDC timer. The assumption would be that we have access to
>>> the physical time registers (which sounds like the assumption we have to
>>> make).
>>
>> There are no physical timer registers available in RSC for this purpose.
>>
>>>
>>> Do we get an interrupt somewhere from the RSC hardware when the timer
>>> fires? Or does that just cause a system wakeup event without any pending
>>> irq and then another irq (like the ARM architected timer) just happens
>>> to be pending around the same time? If we get an interrupt somehow then
>>> I would prefer to drop the ARM MMIO timer and do this hybrid broadcast
>>> timer approach.
>>
>> There is no interrupt for PDC timeout. It just causes system wakeup
>> without a pending irq. ARM MMIO is necessary for irq.
> 
> Does that system wakeup cause the CPUs to be enabled? So the sysreg
> based timer in the CPU would start working? Or does it only make the
> system come out of a deep enough idle state to make an always on domain
> work that only contains the MMIO based ARM architected timer?

It only makes the system come out of deep sleep state for MMIO timer to 
function.

> 
> I'd hope that each RSC's PDC timer wakes up the owner of the RSC so that
> we can use the sysreg based timers and ignore the MMIO based timers
> here. This isn't a very important distinction to make though, so if you
> have to use the MMIO timers then it just means some extra DT things need
> to be done to relate the MMIO timers with the RSC that has the timer
> that needs to be programmed too.

> 
> Either way we would need a way to either hook the ARM architected timer
> driver in the kernel, or reimplement the bit of code needed to implement
> the clockevent based on the ARM architected timer that programs the ARM
> timer and the PDC timer together.
> 

Could you please provide some more details on the implementation?


>>
>>>
>>> How the RSC is used in general by other devices, like display, is not
>>> clear to me. We don't have a "wakeup event" framework in the kernel that
>>> device drivers like the display driver can grab a reference to and
>>> program some system wide wakeup for. That sounds like something new that
>>> could be handled entirely in the display driver with direct register
>>> writes, or it could be some qcom specific API/framework that eventually
>>> calls down into the same RSC driver that knows what offsets to write
>>> into in the display RSC's register space, or it could be an entirely
>>> generic framework like clk or regulator frameworks that could be used by
>>> anything. BTW, are we using the display RSC yet?
>>>
>>
>> Only display subsystem RSC is programmed along with CPU RSC in Linux.
>> display RSC instance is not present in upstream but it is present in
>> downstream and used for resource communication purpose only.
> 
>  From the comment above it sounds like the display RSC handles the wakeup
> programming in hardware? So there isn't a need to add a 'wakeup event'
> framework or anything like that. Please correct me if I'm wrong.

Yes. There is no need for any framework. From RSC driver point of view 
there is a need to differentiate apps_rsc vs display_rsc as SW programs 
PDC timer only in apps_rsc case. How about having a dt property to 
capture this?

Thanks,
Raju

Re: [PATCH RFC 3/5] dt-bindings: Add PDC timer bindings for Qualcomm SoCs

[Raju P L S S S N]

On 1/7/2019 9:47 PM, Raju P L S S S N wrote:
> 
> 
> On 1/4/2019 2:49 AM, Stephen Boyd wrote:
>> Quoting Raju P L S S S N (2019-01-03 04:22:58)
>>>
>>> On 12/29/2018 3:08 AM, Stephen Boyd wrote:
>>>> Quoting Raju P L S S S N (2018-12-26 01:44:43)
>>>>>
>>>>> There are two RSC devices in SoC one for application processor 
>>>>> subsystem
>>>>> & other display subsystem. Both RSC contain registers for PDC timers
>>>>> (one for each subsystem).
>>>>
>>>> When is the timer programmed on the display subsystem's RSC? It's hard
>>>> to give advice without all the information.
>>>
>>> For display subsystem RSC, hardware sleep solver takes care of timer
>>> programming for wakeup when the subsystem goes to Power collapse.
>>
>> So the display subsystem doesn't need to program their PDC in their RSC
>> from software?
> Yes.
> 
>>
>>>>
>>>> I would think that it would make sense for the application processor's
>>>> RSC timer to be programmed from the broadcast timer mechanism in the
>>>> kernel so that timers during idle work and suspend turns off the timer
>>>> appropriately with a shutdown hook. I guess the PDC can't tell you the
>>>> time though? It looks like a shadow (and limited) version of the ARM
>>>> architected MMIO timer that we already program for the broadcast timer
>>>> mechanism. Is that because even the MMIO timer can't wakeup the system
>>>> in deep idle?  Assuming that's true, it means the ARM MMIO timer can't
>>>> always be used as the system wide broadcast mechanism because we 
>>>> need to
>>>> augment it with the PDC timer to get the actual wakeup.
>>>>
>>>
>>> Yes. this is correct.
>>>
>>>> Maybe we should be adding hooks into the broadcast timer mechanism to
>>>> program this wakeup event hardware in addition to the ARM MMIO 
>>>> timer. Or
>>>> we should stop using the ARM MMIO timer on these systems and read the
>>>> system register based physical time in the RSC timer driver and 
>>>> register
>>>> this 64-bit PDC register as the broadcast timer. So the time reading
>>>> would be through sysreg and the wakeup programming would be done by
>>>> writing the PDC timer. The assumption would be that we have access to
>>>> the physical time registers (which sounds like the assumption we 
>>>> have to
>>>> make).
>>>
>>> There are no physical timer registers available in RSC for this purpose.
>>>
>>>>
>>>> Do we get an interrupt somewhere from the RSC hardware when the timer
>>>> fires? Or does that just cause a system wakeup event without any 
>>>> pending
>>>> irq and then another irq (like the ARM architected timer) just happens
>>>> to be pending around the same time? If we get an interrupt somehow then
>>>> I would prefer to drop the ARM MMIO timer and do this hybrid broadcast
>>>> timer approach.
>>>
>>> There is no interrupt for PDC timeout. It just causes system wakeup
>>> without a pending irq. ARM MMIO is necessary for irq.
>>
>> Does that system wakeup cause the CPUs to be enabled? So the sysreg
>> based timer in the CPU would start working? Or does it only make the
>> system come out of a deep enough idle state to make an always on domain
>> work that only contains the MMIO based ARM architected timer?
> 
> It only makes the system come out of deep sleep state for MMIO timer to 
> function.
> 
>>
>> I'd hope that each RSC's PDC timer wakes up the owner of the RSC so that
>> we can use the sysreg based timers and ignore the MMIO based timers
>> here. This isn't a very important distinction to make though, so if you
>> have to use the MMIO timers then it just means some extra DT things need
>> to be done to relate the MMIO timers with the RSC that has the timer
>> that needs to be programmed too.
> 
>>
>> Either way we would need a way to either hook the ARM architected timer
>> driver in the kernel, or reimplement the bit of code needed to implement
>> the clockevent based on the ARM architected timer that programs the ARM
>> timer and the PDC timer together.
>>
> 
> Could you please provide some more details on the implementation?

Hi Stephen,

Regardless of implementation options about application processor 
subsytem PDC timer, I think there is a need to differentiate the fact 
that for application processor subsystem PDC timer programming is done 
by SW but not for display processor subsystem as HW sleep solver takes 
care of PDC timer during sleep entry/exit. How about having a dt 
property like qcom,pdc-timer-mode = "solver"/"sw" ? The current patch 
introduced client based model using regmap to achieve this 
differentiation between these two subsystems. By using the dt property, 
once rpmh-src driver instance for application subsystem can have PDC 
timer programing implemented. Let me know if there is another way.

For implementation of PDC timer, I see the following based on above 
discussion -
1. Take the existing cpu_pm_notify approach and move the current series 
approach of programing PDC timer registers to RSC driver. This wouldn't 
involve any changes in clock_event_framework/broadcast framework.

2. Add api hooks (like reading the next wake up programmed) to ARM 
architecture timer driver so that the value is copied to PDC timer using 
the api with in RSC driver based on cpu_pm_notifications.

3. Changes in clockevent to program both ARM mem timer & PDC timer 
together. Could you please share some more details on this?


Please let me know if the first approach is ok.

Thanks,
Raju.

> 
> 
>>>
>>>>
>>>> How the RSC is used in general by other devices, like display, is not
>>>> clear to me. We don't have a "wakeup event" framework in the kernel 
>>>> that
>>>> device drivers like the display driver can grab a reference to and
>>>> program some system wide wakeup for. That sounds like something new 
>>>> that
>>>> could be handled entirely in the display driver with direct register
>>>> writes, or it could be some qcom specific API/framework that eventually
>>>> calls down into the same RSC driver that knows what offsets to write
>>>> into in the display RSC's register space, or it could be an entirely
>>>> generic framework like clk or regulator frameworks that could be 
>>>> used by
>>>> anything. BTW, are we using the display RSC yet?
>>>>
>>>
>>> Only display subsystem RSC is programmed along with CPU RSC in Linux.
>>> display RSC instance is not present in upstream but it is present in
>>> downstream and used for resource communication purpose only.
>>
>>  From the comment above it sounds like the display RSC handles the wakeup
>> programming in hardware? So there isn't a need to add a 'wakeup event'
>> framework or anything like that. Please correct me if I'm wrong.
> 
> Yes. There is no need for any framework. From RSC driver point of view 
> there is a need to differentiate apps_rsc vs display_rsc as SW programs 
> PDC timer only in apps_rsc case. How about having a dt property to 
> capture this?
> 
> Thanks,
> Raju

Re: [PATCH RFC 3/5] dt-bindings: Add PDC timer bindings for Qualcomm SoCs

[Stephen Boyd]

Quoting Raju P L S S S N (2019-01-08 21:34:32)
> 
> 
> On 1/7/2019 9:47 PM, Raju P L S S S N wrote:
> > 
> > 
> > On 1/4/2019 2:49 AM, Stephen Boyd wrote:
> >>
> >> I'd hope that each RSC's PDC timer wakes up the owner of the RSC so that
> >> we can use the sysreg based timers and ignore the MMIO based timers
> >> here. This isn't a very important distinction to make though, so if you
> >> have to use the MMIO timers then it just means some extra DT things need
> >> to be done to relate the MMIO timers with the RSC that has the timer
> >> that needs to be programmed too.
> > 
> >>
> >> Either way we would need a way to either hook the ARM architected timer
> >> driver in the kernel, or reimplement the bit of code needed to implement
> >> the clockevent based on the ARM architected timer that programs the ARM
> >> timer and the PDC timer together.
> >>
> > 
> > Could you please provide some more details on the implementation?
> 
> Hi Stephen,
> 
> Regardless of implementation options about application processor 
> subsytem PDC timer, I think there is a need to differentiate the fact 
> that for application processor subsystem PDC timer programming is done 
> by SW but not for display processor subsystem as HW sleep solver takes 
> care of PDC timer during sleep entry/exit. How about having a dt 
> property like qcom,pdc-timer-mode = "solver"/"sw" ? The current patch 
> introduced client based model using regmap to achieve this 
> differentiation between these two subsystems. By using the dt property, 
> once rpmh-src driver instance for application subsystem can have PDC 
> timer programing implemented. Let me know if there is another way.
> 
> For implementation of PDC timer, I see the following based on above 
> discussion -
> 1. Take the existing cpu_pm_notify approach and move the current series 
> approach of programing PDC timer registers to RSC driver. This wouldn't 
> involve any changes in clock_event_framework/broadcast framework.
> 
> 2. Add api hooks (like reading the next wake up programmed) to ARM 
> architecture timer driver so that the value is copied to PDC timer using 
> the api with in RSC driver based on cpu_pm_notifications.
> 
> 3. Changes in clockevent to program both ARM mem timer & PDC timer 
> together. Could you please share some more details on this?
> 
> 
> Please let me know if the first approach is ok.
> 

The first approach requires that we expose internals of the clockevent
and broadcast timer information to drivers. From my perspective it looks
like a weird kludge to workaround the fact that the broadcast timer
doesn't actually work on this platform. That's why I'm suggesting that
you fix the broadcast timer on your platform to actually work, and do
that within the clockevent/broadcast layers instead of indirecting that
through cpu_pm notifiers.

That could be done by making a PDC clockevent that has some DT binding
of a property pointing to an MMIO timer frame and then reimplementing
the MMIO timer code in the PDC driver on top of the frame/register
region it pulls out of there. Or it could be written in reverse by
having the generic MMIO timer driver point to the PDC somehow and
implement some platform specific API to pass that information to the
real wakeup programming part in PDC.

I'm leaning toward the first approach where PDC is the clockevent and
that uses the MMIO timer on the backend to do what it needs to program a
wakeup. That way you can mandate the usage of the physical timer and
keep this quirk away from the ARM timer driver. It also makes the idea
of a qcom,pdc-timer-mode sort of useless because the PDC will have a
property that points to the timer frame and that will mean "use this for
broadcast wakeup". I'm not sure how the ARM folks feel about this
though. It would probably require some sort of ARM timer API that lets
us program the MMIO timer frame from the PDC driver. So exporting
arch_timer_reg_write() and making that always inlined to optimize hot
paths would be required.

Re: [PATCH RFC 3/5] dt-bindings: Add PDC timer bindings for Qualcomm SoCs

[Raju P L S S S N]

On 1/9/2019 11:16 PM, Stephen Boyd wrote:
> Quoting Raju P L S S S N (2019-01-08 21:34:32)
>>
>>
>> On 1/7/2019 9:47 PM, Raju P L S S S N wrote:
>>>
>>>
>>> On 1/4/2019 2:49 AM, Stephen Boyd wrote:
>>>>
>>>> I'd hope that each RSC's PDC timer wakes up the owner of the RSC so that
>>>> we can use the sysreg based timers and ignore the MMIO based timers
>>>> here. This isn't a very important distinction to make though, so if you
>>>> have to use the MMIO timers then it just means some extra DT things need
>>>> to be done to relate the MMIO timers with the RSC that has the timer
>>>> that needs to be programmed too.
>>>
>>>>
>>>> Either way we would need a way to either hook the ARM architected timer
>>>> driver in the kernel, or reimplement the bit of code needed to implement
>>>> the clockevent based on the ARM architected timer that programs the ARM
>>>> timer and the PDC timer together.
>>>>
>>>
>>> Could you please provide some more details on the implementation?
>>
>> Hi Stephen,
>>
>> Regardless of implementation options about application processor
>> subsytem PDC timer, I think there is a need to differentiate the fact
>> that for application processor subsystem PDC timer programming is done
>> by SW but not for display processor subsystem as HW sleep solver takes
>> care of PDC timer during sleep entry/exit. How about having a dt
>> property like qcom,pdc-timer-mode = "solver"/"sw" ? The current patch
>> introduced client based model using regmap to achieve this
>> differentiation between these two subsystems. By using the dt property,
>> once rpmh-src driver instance for application subsystem can have PDC
>> timer programing implemented. Let me know if there is another way.
>>
>> For implementation of PDC timer, I see the following based on above
>> discussion -
>> 1. Take the existing cpu_pm_notify approach and move the current series
>> approach of programing PDC timer registers to RSC driver. This wouldn't
>> involve any changes in clock_event_framework/broadcast framework.
>>
>> 2. Add api hooks (like reading the next wake up programmed) to ARM
>> architecture timer driver so that the value is copied to PDC timer using
>> the api with in RSC driver based on cpu_pm_notifications.
>>
>> 3. Changes in clockevent to program both ARM mem timer & PDC timer
>> together. Could you please share some more details on this?
>>
>>
>> Please let me know if the first approach is ok.
>>
> 
> The first approach requires that we expose internals of the clockevent
> and broadcast timer information to drivers. From my perspective it looks
> like a weird kludge to workaround the fact that the broadcast timer
> doesn't actually work on this platform. That's why I'm suggesting that
> you fix the broadcast timer on your platform to actually work, and do
> that within the clockevent/broadcast layers instead of indirecting that
> through cpu_pm notifiers.
> 
> That could be done by making a PDC clockevent that has some DT binding
> of a property pointing to an MMIO timer frame and then reimplementing
> the MMIO timer code in the PDC driver on top of the frame/register
> region it pulls out of there. Or it could be written in reverse by
> having the generic MMIO timer driver point to the PDC somehow and
> implement some platform specific API to pass that information to the
> real wakeup programming part in PDC.
> 
> I'm leaning toward the first approach where PDC is the clockevent and
> that uses the MMIO timer on the backend to do what it needs to program a
> wakeup. That way you can mandate the usage of the physical timer and
> keep this quirk away from the ARM timer driver. It also makes the idea
> of a qcom,pdc-timer-mode sort of useless because the PDC will have a
> property that points to the timer frame and that will mean "use this for
> broadcast wakeup". I'm not sure how the ARM folks feel about this
> though. It would probably require some sort of ARM timer API that lets
> us program the MMIO timer frame from the PDC driver. So exporting
> arch_timer_reg_write() and making that always inlined to optimize hot
> paths would be required.
> 

Regarding the first approach -
If PDC clk_evt_dev is created and registered to broadcast framework, it 
means it will replace MMIO timer event device in broadcast framework (as 
only one broadcast event timer can exist). So in registration phase, the 
tick broadcast framework would configure broadcat event handler for 
PDC(clk_evt_dev).

Even if we have some way of exporting arch_timer_reg_write() to program 
the wakeup for MMIO timer, the irq handler for MMIO timer is responsible 
to invoke event_handler. However, since MMIO timer is replaced by PDC 
timer event device, event handler will not be able to inform broadcast 
framework. What can be done about this?

Re: [PATCH RFC 3/5] dt-bindings: Add PDC timer bindings for Qualcomm SoCs

[Stephen Boyd]

Quoting Raju P L S S S N (2019-01-10 08:58:41)
> 
> 
> On 1/9/2019 11:16 PM, Stephen Boyd wrote:
> > That could be done by making a PDC clockevent that has some DT binding
> > of a property pointing to an MMIO timer frame and then reimplementing
> > the MMIO timer code in the PDC driver on top of the frame/register
> > region it pulls out of there. Or it could be written in reverse by
> > having the generic MMIO timer driver point to the PDC somehow and
> > implement some platform specific API to pass that information to the
> > real wakeup programming part in PDC.
> > 
> > I'm leaning toward the first approach where PDC is the clockevent and
> > that uses the MMIO timer on the backend to do what it needs to program a
> > wakeup. That way you can mandate the usage of the physical timer and
> > keep this quirk away from the ARM timer driver. It also makes the idea
> > of a qcom,pdc-timer-mode sort of useless because the PDC will have a
> > property that points to the timer frame and that will mean "use this for
> > broadcast wakeup". I'm not sure how the ARM folks feel about this
> > though. It would probably require some sort of ARM timer API that lets
> > us program the MMIO timer frame from the PDC driver. So exporting
> > arch_timer_reg_write() and making that always inlined to optimize hot
> > paths would be required.
> > 
> 
> Regarding the first approach -
> If PDC clk_evt_dev is created and registered to broadcast framework, it 
> means it will replace MMIO timer event device in broadcast framework (as 
> only one broadcast event timer can exist). So in registration phase, the 
> tick broadcast framework would configure broadcat event handler for 
> PDC(clk_evt_dev).

Yes all the MMIO timer frames would need to be marked as "disabled" I
suppose, or somehow we would need to tell the MMIO timer driver to not
register the MMIO timer frame from there because it's used by the PDC
now. Maybe that would require changing the MMIO timer's compatible
string to be something different.

> 
> Even if we have some way of exporting arch_timer_reg_write() to program 
> the wakeup for MMIO timer, the irq handler for MMIO timer is responsible 
> to invoke event_handler. However, since MMIO timer is replaced by PDC 
> timer event device, event handler will not be able to inform broadcast 
> framework. What can be done about this?
> 

It sounds like you imagine that the MMIO timer frame will still be
active and in use by the ARM architected timer driver? I wasn't thinking
that would be the case. I was thinking that the PDC code would basically
reimplement all of the ARM timer code again by calling some shared timer
code so that we avoid code duplication. Including the irq handler that
would be registered for by the PDC driver and flow from that piece of
code instead of the ARM timer code.