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

[Sudeep Holla]

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.

> >> Unfortunately I cannot understand the explanation here:
> >> https://lore.kernel.org/all/0812c44f777d4026b79df2e3698294be@xxxxxxxxxxxxxxxxxxxxxxxx/
> >> 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