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

From: Edwin Chiu 邱垂峰
Date: Sun Feb 13 2022 - 22:21:42 EST

Next message: Hongxing Zhu: "RE: [PATCH v6 3/8] PCI: imx6: Move imx6_pcie_clk_disable() earlier"
Previous message: Randy Dunlap: "Re: [tip:locking/core 1/2] vmlinux.o: warning: objtool: mce_start()+0x69: call to ftrace_likely_update() leaves .noinstr.text section"
Next in thread: Krzysztof Kozlowski: "Re: [PATCH v4] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Krzysztof:

Please see below answer.

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxxxxx>
> Sent: Tuesday, February 8, 2022 4:22 AM
> To: Edwin Chiu <edwinchiu0505tw@xxxxxxxxx>; robh+dt@xxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; rafael@xxxxxxxxxx; daniel.lezcano@xxxxxxxxxx; linux-pm@xxxxxxxxxxxxxxx;
> Edwin Chiu 邱垂峰 <edwin.chiu@xxxxxxxxxxx>
> 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 <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.
Edwin=> ok, I will remove it.

> > +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.
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/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
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

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 <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.
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: +886-3-5786005 ext.2590
edwin.chiu@xxxxxxxxxxx
300 新竹科學園區創新一路19號

Next message: Hongxing Zhu: "RE: [PATCH v6 3/8] PCI: imx6: Move imx6_pcie_clk_disable() earlier"
Previous message: Randy Dunlap: "Re: [tip:locking/core 1/2] vmlinux.o: warning: objtool: mce_start()+0x69: call to ftrace_likely_update() leaves .noinstr.text section"
Next in thread: Krzysztof Kozlowski: "Re: [PATCH v4] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]