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

[Edwin Chiu 邱垂峰]

> -----Original Message-----
> From: Sudeep Holla <sudeep.holla@xxxxxxx>
> Sent: Thursday, March 3, 2022 6:03 PM
> To: Krzysztof Kozlowski <krzk@xxxxxxxxxx>
> Cc: Edwin Chiu 邱垂峰 <edwin.chiu@xxxxxxxxxxx>; Edwin Chiu <edwinchiu0505tw@xxxxxxxxx>;
> Sudeep Holla <sudeep.holla@xxxxxxx>; robh+dt@xxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; rafael@xxxxxxxxxx; daniel.lezcano@xxxxxxxxxx; linux-pm@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v5] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021
> 
> On Thu, Mar 03, 2022 at 10:34:31AM +0100, Krzysztof Kozlowski wrote:
> > On 03/03/2022 10:01, Edwin Chiu 邱垂峰 wrote:
> > >
> > >
> > >> -----Original Message-----
> > >> From: Krzysztof Kozlowski <krzk@xxxxxxxxxx>
> > >> Sent: Tuesday, March 1, 2022 7:34 PM
> > >> To: Edwin Chiu 邱垂峰 <edwin.chiu@xxxxxxxxxxx>; Edwin Chiu
> > >> <edwinchiu0505tw@xxxxxxxxx>;
> > >> robh+dt@xxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx;
> > >> robh+linux-kernel@xxxxxxxxxxxxxxx; rafael@xxxxxxxxxx;
> > >> daniel.lezcano@xxxxxxxxxx; linux-pm@xxxxxxxxxxxxxxx
> > >> Subject: Re: [PATCH v5] cpuidle: sunplus: Create cpuidle driver for
> > >> sunplus sp7021
> > >>
> > >> On 01/03/2022 10:30, Edwin Chiu 邱垂峰 wrote:
> > >>>
> > >>>
> > >>>> -----Original Message-----
> > >>>> From: Krzysztof Kozlowski <krzk@xxxxxxxxxx>
> > >>>> Sent: Tuesday, February 22, 2022 12:48 AM
> > >>>> To: Edwin Chiu <edwinchiu0505tw@xxxxxxxxx>; Edwin Chiu 邱垂峰
> > >>>> <edwin.chiu@xxxxxxxxxxx>;
> > >>>> robh+dt@xxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx;
> > >>>> robh+linux-kernel@xxxxxxxxxxxxxxx; rafael@xxxxxxxxxx;
> > >>>> daniel.lezcano@xxxxxxxxxx; linux-pm@xxxxxxxxxxxxxxx
> > >>>> Subject: Re: [PATCH v5] cpuidle: sunplus: Create cpuidle driver
> > >>>> for sunplus sp7021
> > >>>>
> > >>>> On 21/02/2022 08:26, Edwin Chiu wrote:
> > >>>>> Create cpuidle driver for sunplus sp7021 chip
> > >>>>>
> > >>>>> Signed-off-by: Edwin Chiu <edwinchiu0505tw@xxxxxxxxx>
> > >>>>> ---
> > >>>>> Changes in v3
> > >>>>>  - Rearrangement #include sequence
> > >>>>>  - Change remark style to /*~*/
> > >>>>>  - Align author email address to same as sob
> > >>>>>  - Optimal code
> > >>>>> Changes in v4
> > >>>>>  - According Rob Herringrobh's comment
> > >>>>>    There is no need for this binding.
> > >>>>>    Just wanting a different driver is not a reason
> > >>>>>    for a duplicate schema.
> > >>>>>    So remove yaml file and submit driver again.
> > >>>>> Changes in v5
> > >>>>>  - According Krzysztof's comment
> > >>>>>    You either use appropriate compatible in DT
> > >>>>>    or add your compatible to cpuidle-arm.
> > >>>>>    Even if this did not work, then the solution is to
> > >>>>>    use common parts, not to duplicate entire driver.
> > >>>>>    According Sudeep's comment
> > >>>>>    In short NACK for any dedicated driver for this platform,
> > >>>>>    use the generic cpuidle-arm driver with appropriate platform hooks
> > >>>>>    Create cpuidle-sunplus.c in arch/arm/mach-sunplus/
> > >>>>>    for hook generic cpuidle-arm driver
> > >>>>>
> > >>>>>  MAINTAINERS                                   |  6 ++
> > >>>>>  arch/arm/mach-sunplus/cpuidle-sunplus.c       | 88 +++++++++++++++++
> > >>>>>  include/linux/platform_data/cpuidle-sunplus.h | 12 ++++
> > >>>>>  3 files changed, 106 insertions(+)  create mode 100644
> > >>>>> arch/arm/mach-sunplus/cpuidle-sunplus.c
> > >>>>>  create mode 100644
> > >>>>> include/linux/platform_data/cpuidle-sunplus.h
> > >>>>>
> > >>>>> diff --git a/MAINTAINERS b/MAINTAINERS index e0dca8f..5c96428
> > >>>>> 100644
> > >>>>> --- a/MAINTAINERS
> > >>>>> +++ b/MAINTAINERS
> > >>>>> @@ -18252,6 +18252,12 @@ L:	netdev@xxxxxxxxxxxxxxx
> > >>>>>  S:	Maintained
> > >>>>>  F:	drivers/net/ethernet/dlink/sundance.c
> > >>>>>
> > >>>>> +SUNPLUS CPUIDLE DRIVER
> > >>>>> +M:	Edwin Chiu <edwinchiu0505tw@xxxxxxxxx>
> > >>>>> +S:	Maintained
> > >>>>> +F:	arch/arm/mach-sunplus/cpuidle-sunplus.c
> > >>>>> +F:	include/linux/platform_data/cpuidle-sunplus.h
> > >>>>> +
> > >>>>>  SUPERH
> > >>>>>  M:	Yoshinori Sato <ysato@xxxxxxxxxxxxxxxxxxxx>
> > >>>>>  M:	Rich Felker <dalias@xxxxxxxx>
> > >>>>> diff --git a/arch/arm/mach-sunplus/cpuidle-sunplus.c
> > >>>>> b/arch/arm/mach-sunplus/cpuidle-sunplus.c
> > >>>>> new file mode 100644
> > >>>>> index 0000000..e9d9738
> > >>>>> --- /dev/null
> > >>>>> +++ b/arch/arm/mach-sunplus/cpuidle-sunplus.c
> > >>>>> @@ -0,0 +1,88 @@
> > >>>>> +// SPDX-License-Identifier: GPL-2.0-only
> > >>>>> +/*
> > >>>>> + * SP7021 cpu idle Driver.
> > >>>>> + * Copyright (C) Sunplus Tech / Tibbo Tech.
> > >>>>> + */
> > >>>>> +#define pr_fmt(fmt) "CPUidle arm: " fmt
> > >>>>> +
> > >>>>> +#include <linux/cpuidle.h>
> > >>>>> +#include <linux/of_device.h>
> > >>>>> +#include <linux/platform_data/cpuidle-sunplus.h>
> > >>>>> +
> > >>>>> +#include <asm/cpuidle.h>
> > >>>>> +
> > >>>>> +typedef int (*idle_fn)(void);
> > >>>>> +
> > >>>>> +static DEFINE_PER_CPU(idle_fn*, sp7021_idle_ops);
> > >>>>> +
> > >>>>> +static int sp7021_cpuidle_enter(unsigned long index) {
> > >>>>> +	return __this_cpu_read(sp7021_idle_ops)[index]();
> > >>>>> +}
> > >>>>> +static int sp7021_cpu_spc(void) {
> > >>>>> +	cpu_v7_do_idle();   /* idle to WFI */
> > >>>>> +	return 0;
> > >>>>> +}
> > >>>>> +static const struct of_device_id sp7021_idle_state_match[] = {
> > >>>>> +	{ .compatible = "arm,idle-state", .data = sp7021_cpu_spc },
> > >>>>> +	{ },
> > >>>>> +};
> > >>>>
> > >>>> This is confusing. You want to have two drivers to bind to the
> > >>>> same compatible? As I wrote in the previous messages, you should
> > >>>> simply use arm,idle-state just like few
> > >> other architectures.
> > >>>>
> > >>>>
> > >>>> Best regards,
> > >>>> Krzysztof
> > >>>
> > >>>
> > >>> The patch v5 implemented according your comment.
> > >>> Used common part of arm,idle-state.
> > >>> Create new enable-method for cpuidle.ops function.
> > >>> It only have arm cpuidle driver exist now, no two drivers to bind to the same compatible.
> > >>>
> > >>> What do you mean " simply use arm,idle-state just like few other architectures "?
> > >>>
> > >>
> > >> I mean, do it similarly (by using arm,idle-state and other related
> > >> properties) to for example ti,am4372/ti,am3352.
> > >>
> > >> Best regards,
> > >> Krzysztof
> > >
> > >
> > > The am3352 cpuidle code structure is very similar to ours.
> > > Used enable-method = "ti,am3352" and compatible = "arm,idle-state"
> > > in am33xx.dtsi Used CPUIDLE_METHOD_OF_DECLARE(pm33xx_idle,
> > > "ti,am3352", &amx3_cpuidle_ops) in pm33xx-core.c
> > >
> > > The difference are
> > > am3352
> > > amx3_idle_init(~) assign idle_states[i].wfi_flags =
> > > states[i].wfi_flags;
> > > amx3_idle_enter(~) call idle_fn(idle_state->wfi_flags)
> > >
> > > sunplus-sp7021
> > > sp7021_cpuidle_init(~) assign fns[i] = idle_fns[i];
> > > sp7021_cpuidle_enter(~) call
> > > __this_cpu_read(sp7021_idle_ops)[index]();
> > >
> > > I don't think am3352 cpuidle code architecture simpler than ours.
> > > The idle_fn function need more complex method to be assign.
> > > How do you think?
> >
> > You duplicated a driver, entire pieces of code. This is not acceptable.
> > Therefore it does not really make sense to discuss whether duplicated
> > solution seems simpler or not... We won't accept duplicated code.
> > Especially for WFI-only driver.
> >
> 
> +1 for above comment.
> 
> In addition, the reference platform am33xx* doesn't seem to support hotplug (may be I am missing to
> see but quick grep gave no results) and their idle is definitely not just WFI. So what I asked is that please
> document the chosen "sunplus,sc-smp" as bot cpu idle and hotplug methods and when you support non
> WFI states, we can revisit this. Also you must stick to this hotplug method whenever you decided to
> support it.
> 
> 
> --
> Regards,
> Sudeep


Thanks your advice.
Look like key point still only WFI function when cpuidle.
As I explain before, only enable generic ARM cpuidle driver is not work.
It need enable-method code to assign cpuidle_ops functions.
"psci" is one of enable-method, but there have problem in my side due to smc or secure code unsupported.
So I create cpuidle-sunplus.c code with "sunplus,sc-smp" to let cpuidle code complete for our source code. 
With this structure, I can add more custom low power code in the future.

What does it mean for "please document the chosen "sunplus,sc-smp" as bot cpu idle and hotplug methods" ?
Does it mean "edit yaml file"? (Previously, I submit yaml file also, but Rob say I don't need submit when I use compatible="arm,idle-state")


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