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