Date:   Mon, 14 Feb 2022 02:55:52 +0000
From:   Edwin Chiu 邱垂峰 <edwin.chiu@...plus.com>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...onical.com>,
        Edwin Chiu <edwinchiu0505tw@...il.com>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "rafael@...nel.org" <rafael@...nel.org>,
        "daniel.lezcano@...aro.org" <daniel.lezcano@...aro.org>,
        "linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>
CC:     Edwin Chiu 邱垂峰 <edwin.chiu@...plus.com>
Subject: RE: [PATCH v4] cpuidle: sunplus: Create cpuidle driver for sunplus
 sp7021

Hi Krzysztof:

Please see below answer.

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@...onical.com>
> Sent: Tuesday, February 8, 2022 4:22 AM
> To: Edwin Chiu <edwinchiu0505tw@...il.com>; robh+dt@...nel.org; devicetree@...r.kernel.org;
> linux-kernel@...r.kernel.org; rafael@...nel.org; daniel.lezcano@...aro.org; linux-pm@...r.kernel.org;
> Edwin Chiu 邱垂峰 <edwin.chiu@...plus.com>
> 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@...il.com>
> > ---
> > 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@...r.kernel.org
> >  S:	Maintained
> >  F:	drivers/net/ethernet/dlink/sundance.c
> >
> > +SUNPLUS CPUIDLE DRIVER
> > +M:	Edwin Chiu <edwinchiu0505tw@...il.com>
> > +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@...rs.sourceforge.jp>
> >  M:	Rich Felker <dalias@...c.org>
> > 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@sphcmbx02.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.
> 
> > +	}
> > +};
> > +
> > +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@...il.com>");
> > +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@...plus.com
300 新竹科學園區創新一路19號

Powered by blists - more mailing lists