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

[Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>]

On Mon, Sep 29, 2014 at 05:16:25PM +0100, Lina Iyer wrote:

[...]

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

I wanted to say that instead of referring to L2 you can refer to the
computing system as a whole, it is a computing subsystem clock gating.

Instead of referring to L2, refer to cache hierarchy, generically.

It is just a nitpick to make your life easier in the future.

[...]

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

You are welcome, thanks for you patience in converting the driver to the
new DT API.

Lorenzo