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

Commit Message

Edwin Chiu Feb. 7, 2022, 8:11 a.m. UTC

Create cpuidle driver for sunplus sp7021 chip

Signed-off-by: Edwin Chiu <[e-mail]>
---
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.

 MAINTAINERS                                   |   8 ++
 drivers/cpuidle/Kconfig.arm                   |   7 ++
 drivers/cpuidle/Makefile                      |   1 +
 drivers/cpuidle/cpuidle-sunplus.c             | 167 ++++++++++++++++++++++++++
 include/linux/platform_data/cpuidle-sunplus.h |  19 +++
 5 files changed, 202 insertions(+)
 create mode 100644 drivers/cpuidle/cpuidle-sunplus.c
 create mode 100644 include/linux/platform_data/cpuidle-sunplus.h

Comments

Krzysztof Kozlowski Feb. 7, 2022, 8:21 p.m. UTC | #1

On 07/02/2022 09:11, Edwin Chiu wrote:
> Create cpuidle driver for sunplus sp7021 chip
> 
> Signed-off-by: Edwin Chiu <[e-mail]>
> ---
> 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.
> 
>  MAINTAINERS                                   |   8 ++
>  drivers/cpuidle/Kconfig.arm                   |   7 ++
>  drivers/cpuidle/Makefile                      |   1 +
>  drivers/cpuidle/cpuidle-sunplus.c             | 167 ++++++++++++++++++++++++++
>  include/linux/platform_data/cpuidle-sunplus.h |  19 +++
>  5 files changed, 202 insertions(+)
>  create mode 100644 drivers/cpuidle/cpuidle-sunplus.c
>  create mode 100644 include/linux/platform_data/cpuidle-sunplus.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e0dca8f..a938a58 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -18252,6 +18252,14 @@ L:	[e-mail]
>  S:	Maintained
>  F:	drivers/net/ethernet/dlink/sundance.c
>  
> +SUNPLUS CPUIDLE DRIVER
> +M:	Edwin Chiu <[e-mail]>
> +S:	Maintained
> +F:	drivers/cpuidle/Kconfig.arm
> +F:	drivers/cpuidle/Makefile

This does not belong here.

> +F:	drivers/cpuidle/cpuidle-sunplus.c
> +F:	include/linux/platform_data/cpuidle-sunplus.h
> +
>  SUPERH
>  M:	Yoshinori Sato <[e-mail]>
>  M:	Rich Felker <[e-mail]>
> diff --git a/drivers/cpuidle/Kconfig.arm b/drivers/cpuidle/Kconfig.arm
> index 15d6c46..fd921c5 100644
> --- a/drivers/cpuidle/Kconfig.arm
> +++ b/drivers/cpuidle/Kconfig.arm
> @@ -118,3 +118,10 @@ config ARM_QCOM_SPM_CPUIDLE
>  	  The Subsystem Power Manager (SPM) controls low power modes for the
>  	  CPU and L2 cores. It interface with various system drivers to put
>  	  the cores in low power modes.
> +
> +config ARM_SUNPLUS_CPUIDLE
> +	bool "CPU Idle Driver For SUNPLUS SoCs"
> +	depends on !ARM64
> +	select DT_IDLE_STATES
> +	help
> +	  Select this to enable cpuidle on SP7021 processors.
> diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile
> index 26bbc5e..0a020d1 100644
> --- a/drivers/cpuidle/Makefile
> +++ b/drivers/cpuidle/Makefile
> @@ -25,6 +25,7 @@ obj-$(CONFIG_ARM_PSCI_CPUIDLE)		+= cpuidle-psci.o
>  obj-$(CONFIG_ARM_PSCI_CPUIDLE_DOMAIN)	+= cpuidle-psci-domain.o
>  obj-$(CONFIG_ARM_TEGRA_CPUIDLE)		+= cpuidle-tegra.o
>  obj-$(CONFIG_ARM_QCOM_SPM_CPUIDLE)	+= cpuidle-qcom-spm.o
> +obj-$(CONFIG_ARM_SUNPLUS_CPUIDLE)		+= cpuidle-sunplus.o
>  
>  ###############################################################################
>  # MIPS drivers
> diff --git a/drivers/cpuidle/cpuidle-sunplus.c b/drivers/cpuidle/cpuidle-sunplus.c
> new file mode 100644
> index 0000000..0e7bf43
> --- /dev/null
> +++ b/drivers/cpuidle/cpuidle-sunplus.c
> @@ -0,0 +1,167 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2011-2014, The Linux Foundation. All rights reserved.
> + * Copyright (c) 2014,2015, Linaro Ltd.
> + *
> + * SAW power controller driver
> + */
> +#define pr_fmt(fmt) "CPUidle arm: " fmt
> +
> +#include <asm/suspend.h>
> +#include <linux/cpu_cooling.h>
> +#include <linux/cpuidle.h>
> +#include <linux/cpumask.h>
> +#include <linux/cpu_pm.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/slab.h>
> +#include <linux/platform_data/cpuidle-sunplus.h>
> +
> +#include "dt_idle_states.h"
> +
> +static int sp7021_wfi_finisher(unsigned long flags)
> +{
> +	cpu_v7_do_idle();   /* idle to WFI */
> +
> +	return -1;
> +}
> +
> +static int sp7021_enter_idle_state(struct cpuidle_device *dev,
> +				struct cpuidle_driver *drv, int idx)
> +{
> +	int ret;
> +
> +  /* if idx=0, call cpu_do_idle() */

Wrong indentation.

> +	if (!idx) {
> +		cpu_v7_do_idle();
> +		return idx;
> +	}
> +
> +	/* if idx>0, call cpu_suspend() */
> +	ret = cpu_pm_enter();
> +	if (!ret) {
> +	/*
> +	 * Pass idle state index to cpuidle_suspend which in turn
> +	 * will call the CPU ops suspend protocol with idle index as a
> +	 * parameter.
> +	 */

The same.

> +		ret = cpu_suspend(idx, sp7021_wfi_finisher);
> +	}
> +	cpu_pm_exit();
> +
> +	return ret ? -1:idx;
> +}
> +
> +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...

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.

Unfortunately I cannot understand the explanation here:
https://lore.kernel.org/all/[e-mail]/
Why exactly cpuidle-arm does not work in your case?


> +	}
> +};
> +
> +static const struct of_device_id sp7021_idle_state_match[] = {
> +	{ .compatible = "sunplus,sp7021-idle-state",
> +		.data = sp7021_enter_idle_state },
> +	{ },
> +};
> +
> +/*
> + * sp7021_idle_init - Initializes sp7021 cpuidle driver
> + *
> + * Initializes sp7021 cpuidle driver for all CPUs, if any CPU fails
> + * to register cpuidle driver then rollback to cancel all CPUs
> + * registration.
> + */
> +static int __init sp7021_idle_init(void)
> +{
> +	int cpu, ret;
> +	struct cpuidle_driver *drv;
> +	struct cpuidle_device *dev;
> +
> +	drv = kmemdup(&sp7021_idle_driver, sizeof(*drv), GFP_KERNEL);
> +	if (!drv)
> +		return -ENOMEM;
> +
> +	drv->cpumask = (struct cpumask *)cpumask_of(cpu);
> +	/*
> +	 * Initialize idle states data, starting at index 1.  This
> +	 * driver is DT only, if no DT idle states are detected (ret
> +	 * == 0) let the driver initialization fail accordingly since
> +	 * there is no reason to initialize the idle driver if only
> +	 * wfi is supported.
> +	 */
> +	ret = dt_init_idle_driver(drv, sp7021_idle_state_match, 1);
> +	if (ret <= 0)
> +		return ret ? : -ENODEV;
> +
> +	ret = cpuidle_register_driver(drv);
> +	if (ret) {
> +		pr_err("Failed to register cpuidle driver\n");
> +		return ret;
> +	}
> +
> +	for_each_possible_cpu(cpu) {
> +		dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> +		if (!dev) {
> +			ret = -ENOMEM;
> +			goto out_fail;
> +		}
> +		dev->cpu = cpu;
> +
> +		ret = cpuidle_register_device(dev);
> +		if (ret) {
> +			pr_err("Failed to register cpuidle device for CPU %d\n", cpu);
> +			kfree(dev);
> +			goto out_fail;
> +		}
> +	}
> +
> +	return 0;
> +
> +out_fail:
> +	while (--cpu >= 0) {
> +		dev = per_cpu(cpuidle_devices, cpu);
> +		cpuidle_unregister_device(dev);
> +		kfree(dev);
> +	}
> +	cpuidle_unregister_driver(drv);
> +
> +	return ret;
> +}
> +
> +static int __init idle_init(void)
> +{
> +	int ret;
> +
> +	if (of_machine_is_compatible("sunplus,sp7021-achip")) {
> +		sp7021_idle_init();
> +		ret = 0;
> +	}	else
> +		ret = -1;
> +
> +	if (ret) {
> +		pr_err("failed to cpuidle init\n");
> +		return ret;
> +	}
> +
> +	return ret;
> +}
> +device_initcall(idle_init);
> +
> +MODULE_AUTHOR("Edwin Chiu <[e-mail]>");
> +MODULE_DESCRIPTION("Sunplus sp7021 cpuidle driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/platform_data/cpuidle-sunplus.h b/include/linux/platform_data/cpuidle-sunplus.h
> new file mode 100644
> index 0000000..bf87682
> --- /dev/null
> +++ b/include/linux/platform_data/cpuidle-sunplus.h
> @@ -0,0 +1,19 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.

Do not duplicate SPDX.

> + */
> +
> +#ifndef __CPUIDLE_SP7021_H
> +#define __CPUIDLE_SP7021_H
> +
> +struct cpuidle_sp7021_data {
> +	int (*cpu23_powerdown)(void);
> +	void (*pre_enter_aftr)(void);
> +	void (*post_enter_aftr)(void);

Where are these used?

> +};
> +
> +extern int cpu_v7_do_idle(void);> +
> +#endif


Best regards,
Krzysztof

Edwin Chiu 邱垂峰 Feb. 14, 2022, 2:55 a.m. UTC | #2

Hi Krzysztof:

Please see below answer.

> -----Original Message-----
> From: Krzysztof Kozlowski <[e-mail]>
> Sent: Tuesday, February 8, 2022 4:22 AM
> To: Edwin Chiu <[e-mail]>; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]; [e-mail];
> Edwin Chiu 邱垂峰 <[e-mail]>
> Subject: Re: [PATCH v4] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021
> 
> On 07/02/2022 09:11, Edwin Chiu wrote:
> > Create cpuidle driver for sunplus sp7021 chip
> >
> > Signed-off-by: Edwin Chiu <[e-mail]>
> > ---
> > 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.
> >
> >  MAINTAINERS                                   |   8 ++
> >  drivers/cpuidle/Kconfig.arm                   |   7 ++
> >  drivers/cpuidle/Makefile                      |   1 +
> >  drivers/cpuidle/cpuidle-sunplus.c             | 167 ++++++++++++++++++++++++++
> >  include/linux/platform_data/cpuidle-sunplus.h |  19 +++
> >  5 files changed, 202 insertions(+)
> >  create mode 100644 drivers/cpuidle/cpuidle-sunplus.c  create mode
> > 100644 include/linux/platform_data/cpuidle-sunplus.h
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS index e0dca8f..a938a58 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -18252,6 +18252,14 @@ L:	[e-mail]
> >  S:	Maintained
> >  F:	drivers/net/ethernet/dlink/sundance.c
> >
> > +SUNPLUS CPUIDLE DRIVER
> > +M:	Edwin Chiu <[e-mail]>
> > +S:	Maintained
> > +F:	drivers/cpuidle/Kconfig.arm
> > +F:	drivers/cpuidle/Makefile
> 
> This does not belong here.
Edwin=> ok, I will remove it. 

> > +F:	drivers/cpuidle/cpuidle-sunplus.c
> > +F:	include/linux/platform_data/cpuidle-sunplus.h
> > +
> >  SUPERH
> >  M:	Yoshinori Sato <[e-mail]>
> >  M:	Rich Felker <[e-mail]>
> > diff --git a/drivers/cpuidle/Kconfig.arm b/drivers/cpuidle/Kconfig.arm
> > index 15d6c46..fd921c5 100644
> > --- a/drivers/cpuidle/Kconfig.arm
> > +++ b/drivers/cpuidle/Kconfig.arm
> > @@ -118,3 +118,10 @@ config ARM_QCOM_SPM_CPUIDLE
> >  	  The Subsystem Power Manager (SPM) controls low power modes for the
> >  	  CPU and L2 cores. It interface with various system drivers to put
> >  	  the cores in low power modes.
> > +
> > +config ARM_SUNPLUS_CPUIDLE
> > +	bool "CPU Idle Driver For SUNPLUS SoCs"
> > +	depends on !ARM64
> > +	select DT_IDLE_STATES
> > +	help
> > +	  Select this to enable cpuidle on SP7021 processors.
> > diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile index
> > 26bbc5e..0a020d1 100644
> > --- a/drivers/cpuidle/Makefile
> > +++ b/drivers/cpuidle/Makefile
> > @@ -25,6 +25,7 @@ obj-$(CONFIG_ARM_PSCI_CPUIDLE)		+= cpuidle-psci.o
> >  obj-$(CONFIG_ARM_PSCI_CPUIDLE_DOMAIN)	+= cpuidle-psci-domain.o
> >  obj-$(CONFIG_ARM_TEGRA_CPUIDLE)		+= cpuidle-tegra.o
> >  obj-$(CONFIG_ARM_QCOM_SPM_CPUIDLE)	+= cpuidle-qcom-spm.o
> > +obj-$(CONFIG_ARM_SUNPLUS_CPUIDLE)		+= cpuidle-sunplus.o
> >
> >
> > ######################################################################
> > #########
> >  # MIPS drivers
> > diff --git a/drivers/cpuidle/cpuidle-sunplus.c
> > b/drivers/cpuidle/cpuidle-sunplus.c
> > new file mode 100644
> > index 0000000..0e7bf43
> > --- /dev/null
> > +++ b/drivers/cpuidle/cpuidle-sunplus.c
> > @@ -0,0 +1,167 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (c) 2011-2014, The Linux Foundation. All rights reserved.
> > + * Copyright (c) 2014,2015, Linaro Ltd.
> > + *
> > + * SAW power controller driver
> > + */
> > +#define pr_fmt(fmt) "CPUidle arm: " fmt
> > +
> > +#include <asm/suspend.h>
> > +#include <linux/cpu_cooling.h>
> > +#include <linux/cpuidle.h>
> > +#include <linux/cpumask.h>
> > +#include <linux/cpu_pm.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/slab.h>
> > +#include <linux/platform_data/cpuidle-sunplus.h>
> > +
> > +#include "dt_idle_states.h"
> > +
> > +static int sp7021_wfi_finisher(unsigned long flags) {
> > +	cpu_v7_do_idle();   /* idle to WFI */
> > +
> > +	return -1;
> > +}
> > +
> > +static int sp7021_enter_idle_state(struct cpuidle_device *dev,
> > +				struct cpuidle_driver *drv, int idx) {
> > +	int ret;
> > +
> > +  /* if idx=0, call cpu_do_idle() */
> 
> Wrong indentation.
Edwin=> ok, I will modify it. 

> > +	if (!idx) {
> > +		cpu_v7_do_idle();
> > +		return idx;
> > +	}
> > +
> > +	/* if idx>0, call cpu_suspend() */
> > +	ret = cpu_pm_enter();
> > +	if (!ret) {
> > +	/*
> > +	 * Pass idle state index to cpuidle_suspend which in turn
> > +	 * will call the CPU ops suspend protocol with idle index as a
> > +	 * parameter.
> > +	 */
> 
> The same.
Edwin=> ok, I will modify it.

> > +		ret = cpu_suspend(idx, sp7021_wfi_finisher);
> > +	}
> > +	cpu_pm_exit();
> > +
> > +	return ret ? -1:idx;
> > +}
> > +
> > +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...
> 
> 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.
> 
> Unfortunately I cannot understand the explanation here:
> https://lore.kernel.org/all/[e-mail]/
> 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
       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
       
       But change cpuilde-arm.c for sunplus driver seems not reasonable.
       That is why I want to submit cpuidle-sunplus.c
       Althought sunplus cpuidle only come in WFI, but it can complete the cpuidle framework.
> 
> > +	}
> > +};
> > +
> > +static const struct of_device_id sp7021_idle_state_match[] = {
> > +	{ .compatible = "sunplus,sp7021-idle-state",
> > +		.data = sp7021_enter_idle_state },
> > +	{ },
> > +};
> > +
> > +/*
> > + * sp7021_idle_init - Initializes sp7021 cpuidle driver
> > + *
> > + * Initializes sp7021 cpuidle driver for all CPUs, if any CPU fails
> > + * to register cpuidle driver then rollback to cancel all CPUs
> > + * registration.
> > + */
> > +static int __init sp7021_idle_init(void) {
> > +	int cpu, ret;
> > +	struct cpuidle_driver *drv;
> > +	struct cpuidle_device *dev;
> > +
> > +	drv = kmemdup(&sp7021_idle_driver, sizeof(*drv), GFP_KERNEL);
> > +	if (!drv)
> > +		return -ENOMEM;
> > +
> > +	drv->cpumask = (struct cpumask *)cpumask_of(cpu);
> > +	/*
> > +	 * Initialize idle states data, starting at index 1.  This
> > +	 * driver is DT only, if no DT idle states are detected (ret
> > +	 * == 0) let the driver initialization fail accordingly since
> > +	 * there is no reason to initialize the idle driver if only
> > +	 * wfi is supported.
> > +	 */
> > +	ret = dt_init_idle_driver(drv, sp7021_idle_state_match, 1);
> > +	if (ret <= 0)
> > +		return ret ? : -ENODEV;
> > +
> > +	ret = cpuidle_register_driver(drv);
> > +	if (ret) {
> > +		pr_err("Failed to register cpuidle driver\n");
> > +		return ret;
> > +	}
> > +
> > +	for_each_possible_cpu(cpu) {
> > +		dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> > +		if (!dev) {
> > +			ret = -ENOMEM;
> > +			goto out_fail;
> > +		}
> > +		dev->cpu = cpu;
> > +
> > +		ret = cpuidle_register_device(dev);
> > +		if (ret) {
> > +			pr_err("Failed to register cpuidle device for CPU %d\n", cpu);
> > +			kfree(dev);
> > +			goto out_fail;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +
> > +out_fail:
> > +	while (--cpu >= 0) {
> > +		dev = per_cpu(cpuidle_devices, cpu);
> > +		cpuidle_unregister_device(dev);
> > +		kfree(dev);
> > +	}
> > +	cpuidle_unregister_driver(drv);
> > +
> > +	return ret;
> > +}
> > +
> > +static int __init idle_init(void)
> > +{
> > +	int ret;
> > +
> > +	if (of_machine_is_compatible("sunplus,sp7021-achip")) {
> > +		sp7021_idle_init();
> > +		ret = 0;
> > +	}	else
> > +		ret = -1;
> > +
> > +	if (ret) {
> > +		pr_err("failed to cpuidle init\n");
> > +		return ret;
> > +	}
> > +
> > +	return ret;
> > +}
> > +device_initcall(idle_init);
> > +
> > +MODULE_AUTHOR("Edwin Chiu <[e-mail]>");
> > +MODULE_DESCRIPTION("Sunplus sp7021 cpuidle driver");
> > +MODULE_LICENSE("GPL");
> > diff --git a/include/linux/platform_data/cpuidle-sunplus.h
> > b/include/linux/platform_data/cpuidle-sunplus.h
> > new file mode 100644
> > index 0000000..bf87682
> > --- /dev/null
> > +++ b/include/linux/platform_data/cpuidle-sunplus.h
> > @@ -0,0 +1,19 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * This program is free software; you can redistribute it and/or
> > +modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> 
> Do not duplicate SPDX.
Edwin=> ok, I will change to 
/* SPDX-Lcense-Identifier: GPL-2.0-only */
/* 
 * SP7021 cpu dile Driver.
 * Copyright (C) Sunplus Tech /Tibbo Tech
 */

> > + */
> > +
> > +#ifndef __CPUIDLE_SP7021_H
> > +#define __CPUIDLE_SP7021_H
> > +
> > +struct cpuidle_sp7021_data {
> > +	int (*cpu23_powerdown)(void);
> > +	void (*pre_enter_aftr)(void);
> > +	void (*post_enter_aftr)(void);
> 
> Where are these used?
Edwin=> This code no used. I will remove it. 

> > +};
> > +
> > +extern int cpu_v7_do_idle(void);> +
> > +#endif
> 
> 
> Best regards,
> Krzysztof


Best regards
邱垂峰 EdwinChiu
智能運算專案
T: [phone] ext.2590
[e-mail]
300 新竹科學園區創新一路19號

Krzysztof Kozlowski Feb. 14, 2022, 6:44 a.m. UTC | #3

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...
>>
>> 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.
>>
>> Unfortunately I cannot understand the explanation here:
>> https://lore.kernel.org/all/[e-mail]/
>> 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
>        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>        
>        But change cpuilde-arm.c for sunplus driver seems not reasonable.
>        That is why I want to submit cpuidle-sunplus.c
>        Althought sunplus cpuidle only come in WFI, but it can complete the cpuidle framework.

I don't think it is correct. You can use cpuidle-arm, because it is
being always initialized with device_initcall(). 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. Duplicating is not acceptable.

Best regards,
Krzysztof

Sudeep Holla Feb. 14, 2022, 9:59 a.m. UTC | #4

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/[e-mail]/
> >> 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)

Edwin Chiu 邱垂峰 Feb. 18, 2022, 8:10 a.m. UTC | #5

> -----Original Message-----
> From: Krzysztof Kozlowski <[e-mail]>
> Sent: Monday, February 14, 2022 2:45 PM
> To: Edwin Chiu 邱垂峰 <[e-mail]>; Edwin Chiu <[e-mail]>;
> [e-mail]; [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]
> Subject: Re: [PATCH v4] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021
> 
> 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...
> >>
> >> 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.
> >>
> >> Unfortunately I cannot understand the explanation here:
> >> https://lore.kernel.org/all/0812c44f777d4026b79df2e3698294be@sphcmbx0
> >> 2.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
> >
> > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > ~~~~~~~~~~
> >
> >        But change cpuilde-arm.c for sunplus driver seems not reasonable.
> >        That is why I want to submit cpuidle-sunplus.c
> >        Althought sunplus cpuidle only come in WFI, but it can complete the cpuidle framework.
> 
> I don't think it is correct. You can use cpuidle-arm, because it is being always initialized with
> device_initcall(). 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.
> Duplicating is not acceptable.
> 
> Best regards,
> Krzysztof


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.
According your comment, I will try to use common parts and hook some custom code, later. 
Thanks.

邱垂峰 EdwinChiu
智能運算專案
T: [phone] ext.2590
[e-mail]
300 新竹科學園區創新一路19號

Krzysztof Kozlowski Feb. 18, 2022, 8:31 a.m. UTC | #6

On 18/02/2022 09:10, Edwin Chiu 邱垂峰 wrote:
> 
> 
> 
>> -----Original Message-----
>> From: Krzysztof Kozlowski <[e-mail]>
>> Sent: Monday, February 14, 2022 2:45 PM
>> To: Edwin Chiu 邱垂峰 <[e-mail]>; Edwin Chiu <[e-mail]>;
>> [e-mail]; [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; [e-mail]
>> Subject: Re: [PATCH v4] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021
>>
>> 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...
>>>>
>>>> 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.
>>>>
>>>> Unfortunately I cannot understand the explanation here:
>>>> https://lore.kernel.org/all/0812c44f777d4026b79df2e3698294be@sphcmbx0
>>>> 2.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
>>>
>>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>> ~~~~~~~~~~
>>>
>>>        But change cpuilde-arm.c for sunplus driver seems not reasonable.
>>>        That is why I want to submit cpuidle-sunplus.c
>>>        Althought sunplus cpuidle only come in WFI, but it can complete the cpuidle framework.
>>
>> I don't think it is correct. You can use cpuidle-arm, because it is being always initialized with
>> device_initcall(). 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.
>> Duplicating is not acceptable.
>>
>> Best regards,
>> Krzysztof
> 
> 
> 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.
> According your comment, I will try to use common parts and hook some custom code, later. 

I think I understood the motivation behind your driver. The cpuidle-arm
requires enable-method property which usually uses for ARMv7
CPU_METHOD_OF_DECLARE(). You don't have CPU_METHOD_OF_DECLARE().

Now the question: why can't you define CPU_METHOD_OF_DECLARE() just like
many other ARMv7 platforms do?

Best regards,
Krzysztof

Edwin Chiu 邱垂峰 Feb. 18, 2022, 8:36 a.m. UTC | #7

> -----Original Message-----
> From: Krzysztof Kozlowski <[e-mail]>
> Sent: Friday, February 18, 2022 4:31 PM
> To: Edwin Chiu 邱垂峰 <[e-mail]>; Edwin Chiu <[e-mail]>;
> [e-mail]; [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]
> Subject: Re: [PATCH v4] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021
> 
> On 18/02/2022 09:10, Edwin Chiu 邱垂峰 wrote:
> >
> >
> >
> >> -----Original Message-----
> >> From: Krzysztof Kozlowski <[e-mail]>
> >> Sent: Monday, February 14, 2022 2:45 PM
> >> To: Edwin Chiu 邱垂峰 <[e-mail]>; Edwin Chiu
> >> <[e-mail]>;
> >> [e-mail]; [e-mail];
> >> [e-mail]; [e-mail];
> >> [e-mail]; [e-mail]
> >> Subject: Re: [PATCH v4] cpuidle: sunplus: Create cpuidle driver for
> >> sunplus sp7021
> >>
> >> 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...
> >>>>
> >>>> 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.
> >>>>
> >>>> Unfortunately I cannot understand the explanation here:
> >>>> https://lore.kernel.org/all/0812c44f777d4026b79df2e3698294be@sphcmb
> >>>> x0 2.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
> >>>
> >>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >>> ~~
> >>> ~~~~~~~~~~
> >>>
> >>>        But change cpuilde-arm.c for sunplus driver seems not reasonable.
> >>>        That is why I want to submit cpuidle-sunplus.c
> >>>        Althought sunplus cpuidle only come in WFI, but it can complete the cpuidle framework.
> >>
> >> I don't think it is correct. You can use cpuidle-arm, because it is
> >> being always initialized with device_initcall(). 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.
> >> Duplicating is not acceptable.
> >>
> >> Best regards,
> >> Krzysztof
> >
> >
> > 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.
> > According your comment, I will try to use common parts and hook some custom code, later.
> 
> I think I understood the motivation behind your driver. The cpuidle-arm requires enable-method
> property which usually uses for ARMv7 CPU_METHOD_OF_DECLARE(). You don't have
> CPU_METHOD_OF_DECLARE().
> 
> Now the question: why can't you define CPU_METHOD_OF_DECLARE() just like many other ARMv7
> platforms do?
> 
> Best regards,
> Krzysztof


Yes, I will try it in next patch.
Thanks.


邱垂峰 EdwinChiu
智能運算專案
T: [phone] ext.2590
[e-mail]
300 新竹科學園區創新一路19號

Edwin Chiu 邱垂峰 Feb. 18, 2022, 8:43 a.m. UTC | #8

> -----Original Message-----
> From: Sudeep Holla <[e-mail]>
> Sent: Monday, February 14, 2022 6:00 PM
> To: Edwin Chiu 邱垂峰 <[e-mail]>
> Cc: Krzysztof Kozlowski <[e-mail]>; Edwin Chiu
> <[e-mail]>; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]; [e-mail]
> 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: [phone] ext.2590
[e-mail]
300 新竹科學園區創新一路19號

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index e0dca8f..a938a58 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18252,6 +18252,14 @@  L:	[e-mail]
 S:	Maintained
 F:	drivers/net/ethernet/dlink/sundance.c
 
+SUNPLUS CPUIDLE DRIVER
+M:	Edwin Chiu <[e-mail]>
+S:	Maintained
+F:	drivers/cpuidle/Kconfig.arm
+F:	drivers/cpuidle/Makefile
+F:	drivers/cpuidle/cpuidle-sunplus.c
+F:	include/linux/platform_data/cpuidle-sunplus.h
+
 SUPERH
 M:	Yoshinori Sato <[e-mail]>
 M:	Rich Felker <[e-mail]>
diff --git a/drivers/cpuidle/Kconfig.arm b/drivers/cpuidle/Kconfig.arm
index 15d6c46..fd921c5 100644
--- a/drivers/cpuidle/Kconfig.arm
+++ b/drivers/cpuidle/Kconfig.arm
@@ -118,3 +118,10 @@  config ARM_QCOM_SPM_CPUIDLE
 	  The Subsystem Power Manager (SPM) controls low power modes for the
 	  CPU and L2 cores. It interface with various system drivers to put
 	  the cores in low power modes.
+
+config ARM_SUNPLUS_CPUIDLE
+	bool "CPU Idle Driver For SUNPLUS SoCs"
+	depends on !ARM64
+	select DT_IDLE_STATES
+	help
+	  Select this to enable cpuidle on SP7021 processors.
diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile
index 26bbc5e..0a020d1 100644
--- a/drivers/cpuidle/Makefile
+++ b/drivers/cpuidle/Makefile
@@ -25,6 +25,7 @@  obj-$(CONFIG_ARM_PSCI_CPUIDLE)		+= cpuidle-psci.o
 obj-$(CONFIG_ARM_PSCI_CPUIDLE_DOMAIN)	+= cpuidle-psci-domain.o
 obj-$(CONFIG_ARM_TEGRA_CPUIDLE)		+= cpuidle-tegra.o
 obj-$(CONFIG_ARM_QCOM_SPM_CPUIDLE)	+= cpuidle-qcom-spm.o
+obj-$(CONFIG_ARM_SUNPLUS_CPUIDLE)		+= cpuidle-sunplus.o
 
 ###############################################################################
 # MIPS drivers
diff --git a/drivers/cpuidle/cpuidle-sunplus.c b/drivers/cpuidle/cpuidle-sunplus.c
new file mode 100644
index 0000000..0e7bf43
--- /dev/null
+++ b/drivers/cpuidle/cpuidle-sunplus.c
@@ -0,0 +1,167 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2011-2014, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2014,2015, Linaro Ltd.
+ *
+ * SAW power controller driver
+ */
+#define pr_fmt(fmt) "CPUidle arm: " fmt
+
+#include <asm/suspend.h>
+#include <linux/cpu_cooling.h>
+#include <linux/cpuidle.h>
+#include <linux/cpumask.h>
+#include <linux/cpu_pm.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/slab.h>
+#include <linux/platform_data/cpuidle-sunplus.h>
+
+#include "dt_idle_states.h"
+
+static int sp7021_wfi_finisher(unsigned long flags)
+{
+	cpu_v7_do_idle();   /* idle to WFI */
+
+	return -1;
+}
+
+static int sp7021_enter_idle_state(struct cpuidle_device *dev,
+				struct cpuidle_driver *drv, int idx)
+{
+	int ret;
+
+  /* if idx=0, call cpu_do_idle() */
+	if (!idx) {
+		cpu_v7_do_idle();
+		return idx;
+	}
+
+	/* if idx>0, call cpu_suspend() */
+	ret = cpu_pm_enter();
+	if (!ret) {
+	/*
+	 * Pass idle state index to cpuidle_suspend which in turn
+	 * will call the CPU ops suspend protocol with idle index as a
+	 * parameter.
+	 */
+		ret = cpu_suspend(idx, sp7021_wfi_finisher);
+	}
+	cpu_pm_exit();
+
+	return ret ? -1:idx;
+}
+
+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",
+	}
+};
+
+static const struct of_device_id sp7021_idle_state_match[] = {
+	{ .compatible = "sunplus,sp7021-idle-state",
+		.data = sp7021_enter_idle_state },
+	{ },
+};
+
+/*
+ * sp7021_idle_init - Initializes sp7021 cpuidle driver
+ *
+ * Initializes sp7021 cpuidle driver for all CPUs, if any CPU fails
+ * to register cpuidle driver then rollback to cancel all CPUs
+ * registration.
+ */
+static int __init sp7021_idle_init(void)
+{
+	int cpu, ret;
+	struct cpuidle_driver *drv;
+	struct cpuidle_device *dev;
+
+	drv = kmemdup(&sp7021_idle_driver, sizeof(*drv), GFP_KERNEL);
+	if (!drv)
+		return -ENOMEM;
+
+	drv->cpumask = (struct cpumask *)cpumask_of(cpu);
+	/*
+	 * Initialize idle states data, starting at index 1.  This
+	 * driver is DT only, if no DT idle states are detected (ret
+	 * == 0) let the driver initialization fail accordingly since
+	 * there is no reason to initialize the idle driver if only
+	 * wfi is supported.
+	 */
+	ret = dt_init_idle_driver(drv, sp7021_idle_state_match, 1);
+	if (ret <= 0)
+		return ret ? : -ENODEV;
+
+	ret = cpuidle_register_driver(drv);
+	if (ret) {
+		pr_err("Failed to register cpuidle driver\n");
+		return ret;
+	}
+
+	for_each_possible_cpu(cpu) {
+		dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+		if (!dev) {
+			ret = -ENOMEM;
+			goto out_fail;
+		}
+		dev->cpu = cpu;
+
+		ret = cpuidle_register_device(dev);
+		if (ret) {
+			pr_err("Failed to register cpuidle device for CPU %d\n", cpu);
+			kfree(dev);
+			goto out_fail;
+		}
+	}
+
+	return 0;
+
+out_fail:
+	while (--cpu >= 0) {
+		dev = per_cpu(cpuidle_devices, cpu);
+		cpuidle_unregister_device(dev);
+		kfree(dev);
+	}
+	cpuidle_unregister_driver(drv);
+
+	return ret;
+}
+
+static int __init idle_init(void)
+{
+	int ret;
+
+	if (of_machine_is_compatible("sunplus,sp7021-achip")) {
+		sp7021_idle_init();
+		ret = 0;
+	}	else
+		ret = -1;
+
+	if (ret) {
+		pr_err("failed to cpuidle init\n");
+		return ret;
+	}
+
+	return ret;
+}
+device_initcall(idle_init);
+
+MODULE_AUTHOR("Edwin Chiu <[e-mail]>");
+MODULE_DESCRIPTION("Sunplus sp7021 cpuidle driver");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/platform_data/cpuidle-sunplus.h b/include/linux/platform_data/cpuidle-sunplus.h
new file mode 100644
index 0000000..bf87682
--- /dev/null
+++ b/include/linux/platform_data/cpuidle-sunplus.h
@@ -0,0 +1,19 @@ 
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef __CPUIDLE_SP7021_H
+#define __CPUIDLE_SP7021_H
+
+struct cpuidle_sp7021_data {
+	int (*cpu23_powerdown)(void);
+	void (*pre_enter_aftr)(void);
+	void (*post_enter_aftr)(void);
+};
+
+extern int cpu_v7_do_idle(void);
+
+#endif