RE: [PATCH v4] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021

["Edwin Chiu 邱垂峰" <edwin.chiu@sunplus.com>]

> -----Original Message-----
> From: Sudeep Holla <sudeep.holla@arm.com>
> Sent: Monday, February 14, 2022 6:00 PM
> To: Edwin Chiu 邱垂峰 <edwin.chiu@sunplus.com>
> Cc: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>; Edwin Chiu
> <edwinchiu0505tw@gmail.com>; robh+dt@kernel.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org; rafael@kernel.org; daniel.lezcano@linaro.org; linux-pm@vger.kernel.org
> Subject: Re: [PATCH v4] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021
> 
> On Mon, Feb 14, 2022 at 07:44:30AM +0100, Krzysztof Kozlowski wrote:
> > On 14/02/2022 03:55, Edwin Chiu 邱垂峰 wrote:
> > > Hi Krzysztof:
> > >
> > > Please see below answer.
> > >
> > >>> +static struct cpuidle_driver sp7021_idle_driver = {
> > >>> +	.name = "sp7021_idle",
> > >>> +	.owner = THIS_MODULE,
> > >>> +	/*
> > >>> +	 * State at index 0 is standby wfi and considered standard
> > >>> +	 * on all ARM platforms. If in some platforms simple wfi
> > >>> +	 * can't be used as "state 0", DT bindings must be implemented
> > >>> +	 * to work around this issue and allow installing a special
> > >>> +	 * handler for idle state index 0.
> > >>> +	 */
> > >>> +	.states[0] = {
> > >>> +		.enter                  = sp7021_enter_idle_state,
> > >>> +		.exit_latency           = 1,
> > >>> +		.target_residency       = 1,
> > >>> +		.power_usage		= UINT_MAX,
> > >>> +		.name                   = "WFI",
> > >>> +		.desc                   = "ARM WFI",
> > >>
> > >> I have impression that there is no point in having custom driver with WFI...
> > >>
> 
> +1
> 
> > >> Still the main question from Daniel and Sudeep stays: why do you
> > >> need this? You copied exactly cpuildle-arm driver, there is nothing
> > >> different here. At least I could not spot differences. Maybe except
> > >> that you use cpu_v7_do_idle explicitly.
> > >>
> 
> Please comment or answer why you can't use standard driver.

I do used compatible = "arm,idle-state" in DT and enable generic arm cpuidle driver in menuconfig.
But there have mount driver fail message due to no cpuidle_ops functions.
That is why I need added patch code to complete cpuidle driver.

> > >> Unfortunately I cannot understand the explanation here:
> > >> https://lore.kernel.org/all/0812c44f777d4026b79df2e3698294be@sphcmb
> > >> x02.sunplus.com.tw/ Why exactly cpuidle-arm does not work in your
> > >> case?
> > >>
> > > Edwin=> I mean cpuidle-arm driver can't directly use with no modified.
> > >        If someone want to use cpuidle-arm driver, below modification seems necessary.
> > >
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > >        Static int sp7021_cpuidle_suspend_enter(unsigned long index) {~}
> > >        Static int __init sp7021_cpuidle_init(struct device_node *cpu_node, int cpu) {~}
> > >        Static const struct cpuidle_ops sc_smp_ops __initconst = {
> > >             .suspend = sp7021_cpuidle_suspend_enter,
> > >             .init = sp7021_cpuidle_init,
> > >        };
> > >        CPUIDLE_METHOD_OF_DECLARE(sc_smp, "sunplus,sc-smp", &sc_smp_ops); //declare
> enable method
> > >
> > > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > ~~~~~~~~~~~~
> > >
> 
> May be. It depends on what is your enable-method. I did a quick grep and could see any support for
> sunplus platform upstream. So I am not sure what is the cpu boot/enable method used. Is it PSCI or
> something custom. You should be using standard PSCI if it is relatively new platform or you have any
> other strong reasons to use custom method. If you are using custom method, then some changes like
> above is required but that will be in the platform port and not the core cpuidle driver/framework.
> 
> In short NACK for any dedicated driver for this platform, use the generic cpuidle-arm driver with
> appropriate platform hooks(like the above one only if you choose to use custom enable method and not
> standard PSCI)
> 
> --
> Regards,
> Sudeep


Yes it seems depends on enable-method.
"psci" is one method.
But when I test "psci" in my platform, there have exception happened in operate smc.
This is another story.

According your comment, I will try to use common parts and hook custom code in platform side later.
Thanks.

邱垂峰 EdwinChiu
智能運算專案
T: +886-3-5786005 ext.2590
edwin.chiu@sunplus.com
300 新竹科學園區創新一路19號