On 07/02/2022 09:11, 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.
>
> 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: netdev@xxxxxxxxxxxxxxx
> S: Maintained
> F: drivers/net/ethernet/dlink/sundance.c
>
> +SUNPLUS CPUIDLE DRIVER
> +M: Edwin Chiu <edwinchiu0505tw@xxxxxxxxx>
> +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 <ysato@xxxxxxxxxxxxxxxxxxxx>
> M: Rich Felker <dalias@xxxxxxxx>
> 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/0812c44f777d4026b79df2e3698294be@xxxxxxxxxxxxxxxxxxxxxxxx/
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 <edwinchiu0505tw@xxxxxxxxx>");
> +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
