Re: [PATCH v7 5/7] qcom: cpuidle: Add cpuidle driver for QCOM cpus

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Sep 29 2014 at 09:31 -0600, Lorenzo Pieralisi wrote:
Hi Lina,

On Sat, Sep 27, 2014 at 01:58:13AM +0100, Lina Iyer wrote:
+The idle states supported by the QCOM SoC are defined as -
+
+    * WFI
+    * Retention
+    * Standalone Power Collapse (Standalone PC or SPC)
+    * Power Collapse (PC)
+
+WFI: WFI does a little more in addition to architectural clock gating.  ARM

This may be misleading. Call it PlatformWFI or something like that, not WFI if
that's not what it is.

Hmm.. Not elegant. Let me see what I can do.

+processors when execute the wfi instruction will gate their internal clocks.
+QCOM cpus use this instruction as a trigger for the SPM state machine. Usually
+with a cpu entering WFI, the SPM is configured to do clock-gating as well. The
+SPM state machine waits for the interrrupt to trigger the core back in to

s/interrrupt/interrupt/

+active. When all CPUs in the SoC, clock gate using the ARM wfi instruction, the
+second level cache usually can also clock gate sensing no cpu activity. When a
+cpu is ready to run, it needs the cache to be active before starting execution.
+Allowing the SPM to execute the clock gating statemachine and waiting for

s/statemachine/state machine/

You are defining a generic binding for Qualcomm idle states, so it should
not be tied to a specific cache level (ie L2 gating), otherwise if the
same state shows up in a future system with L3 you are back to square
one.

"Platform WFI" or something like that ? You got what I mean.

I am not, I am just explaining the difference between Architectural and
Platform WFI and how the WFI on the core, can help L2 enter shallower
idle states, which is true for L3 as well (provided there is enough time
and we are not allowed to do power down states).

+interrupt on behalf of the processor has a benefit of guaranteeing that the
+system state is conducive for the core to resume execution.
+
+Retention: Retention is a low power state where the core is clockgated and the
+memory and the registers associated with the core are retained.  The voltage
+may be reduced to the minimum value needed to keep the processor registers
+active. Retention is triggered when the core executes wfi instruction. The SPM

I think that saying "..is triggered when the core executes wfi instruction"
is not necessary. I am not aware of any other _proper_ way of entering
an ARM idle state other than executing wfi and I hope I will never have to
to become aware of one.

Hopefully so :)

+should be configured to execute the retention sequence and would wait for
+interrupt, before restoring the cpu to execution state. Retention may have a
+slightly higher latency than WFI.
+
+Standalone PC: A cpu can power down and warmboot if there is a sufficient time
+between now and the next know wake up. SPC mode is used to indicate a core

s/know/known/

+entering a power down state without consulting any other cpu or the system
+resources. This helps save power only on that core. Like WFI and Retention, the
+core executes wfi and the SPM programmed to do SPC would use the cpu control
+logic to power down the core's supply and restore it back when woken up by an
+interrupt.  Applying power and reseting the core causes the core to warmboot

s/reseting/resetting/

+back into secure mode which trampolines the control back to the kernel. To
+enter a power down state the kernel needs to call into the secure layer which
+would then execute the ARM wfi instruction. Failing to do so, would result in a
+crash enforced by the warm boot code in the secure layer. On a SoC with
+write-back L1 cache, the cache would need to be flushed.
+Power Collapse: This state is similiar to the SPC mode, but distinguishes

s/similiar/similar/

+itself in the fact that the cpu acknowledges and permits the SoC to enter

s/in the fact that/in that/

+deeper sleep modes. In a hierarchical power domain SoC, this means L2 and other
+caches can be flushed, system bus, clocks - lowered, and SoC main XO turned off
+and voltages reduced, provided all cpus enter this state. In other words, it is
+a coupled idle state.  Since the span of low power modes possible at this state

Careful with the wording here. "Coupled" idle states are defined in the
kernel for systems where the CPUs must enter power down with a specific
ordering. I do not think "Power Collapse" is a "coupled" state from this
perspective, it seems to me more like a "cluster" state, a state that is
entered when all (not necessarily coordinated) CPUs in the cluster enter
that state. Feel free to call it a hierarchical state, if it makes sense
to you.

Okay. I thought I changed it from coupled to cluster.. I will change it
to cluster.

+is vast, the exit latency and the residency of this low power mode would be
+considered high even though at a cpu level, this essentially is cpu power down.
+The SPM in this state also may handshake with the Resource power manager
+processor in the SoC to indicate a complete subsystem shut down.
+
+The idle-state for QCOM SoCs are distinguished by the compatible property of
+the node. They indicate to the cpuidle driver the entry point to use for

What node ? Please be specific. Moreover, the compatible string has a
standard DT meaning and it does not indicate anything. This is a DT idle states
binding and that's where it should stop, anything else is a kernel
implementation detail, or put it differently, it must not define how the
kernel translates that compatible property into data structures/control
code.

Hmm. I'll take it out.

+cpuidle. The devicetree representation of the idle state should be -
+
+Required properties:
+
+- compatible: Must be "arm,idle-state"
+		and one of -
+			"qcom,idle-state-wfi",
+			"qcom,idle-state-ret",
+			"qcom,idle-state-spc",
+			"qcom,idle-state-pc",
+
+static struct cpuidle_driver qcom_cpuidle_driver = {
+	.name	= "qcom_cpuidle",
+	.owner	= THIS_MODULE,
+};
+
+static const struct of_device_id qcom_idle_state_match[] __initconst = {

If it can be built as a module, __initconst should be removed (and
that's true for my Exynos patch too).

Yes, fixing it.
+	{ .compatible = "qcom,idle-state-wfi", .data = qcom_lpm_enter_wfi },
+	{ .compatible = "qcom,idle-state-spc", .data = qcom_lpm_enter_spc },
+	{ },
+};
+
+static int qcom_cpuidle_probe(struct platform_device *pdev)
+{
+	struct cpuidle_driver *drv = &qcom_cpuidle_driver;
+	int ret;
+
+	qcom_idle_enter = (void *)(pdev->dev.platform_data);

Casting a void* to a void* does not seem that useful to me, and that's valid
for other CPUidle drivers too, the cast can be removed unless you explain
to me what it is for.

Sure, I dont need it.
Thanks for your time, Lorenzo.

Lina

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




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux