linux-kernel - Re: [PATCH v5] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220221105151.6rvyoqyvj6ehhtnj@bogus>
Date:   Mon, 21 Feb 2022 10:51:51 +0000
From:   Sudeep Holla <sudeep.holla@....com>
To:     Edwin Chiu <edwinchiu0505tw@...il.com>
Cc:     edwin.chiu@...plus.com, robh+dt@...nel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        rafael@...nel.org, Sudeep Holla <sudeep.holla@....com>,
        daniel.lezcano@...aro.org, linux-pm@...r.kernel.org
Subject: Re: [PATCH v5] cpuidle: sunplus: Create cpuidle driver for sunplus
 sp7021

On Mon, Feb 21, 2022 at 03:26:18PM +0800, 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.
> 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@...r.kernel.org
>  S:	Maintained
>  F:	drivers/net/ethernet/dlink/sundance.c
>  
> +SUNPLUS CPUIDLE DRIVER
> +M:	Edwin Chiu <edwinchiu0505tw@...il.com>
> +S:	Maintained
> +F:	arch/arm/mach-sunplus/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/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;
> +}

You really don't need a cpuidle driver to just WFI for any states.
Add the driver when you have something non WFI in the suspend function.

> +static const struct of_device_id sp7021_idle_state_match[] = {
> +	{ .compatible = "arm,idle-state", .data = sp7021_cpu_spc },
> +	{ },
> +};

This is better than adding new driver like you did in previous version.

I did a quick check but couldn't figure out. How do cpus get switched
ON or OFF on this platform(for example during CPU hotplug) ?

-- 
Regards,
Sudeep