Re: [RFC PATCH v3 34/37] kvx: Add power controller driver

[Krzysztof Kozlowski <krzk@kernel.org>]

On 22/07/2024 11:41, ysionneau@kalrayinc.com wrote:
> From: Yann Sionneau <ysionneau@kalrayinc.com>
> 
> The Power Controller (pwr-ctrl) controls cores reset and wake-up
> procedure.
> 
> Co-developed-by: Clement Leger <clement@clement-leger.fr>
> Signed-off-by: Clement Leger <clement@clement-leger.fr>
> Co-developed-by: Julian Vetter <jvetter@kalrayinc.com>
> Signed-off-by: Julian Vetter <jvetter@kalrayinc.com>
> Co-developed-by: Louis Morhet <lmorhet@kalrayinc.com>
> Signed-off-by: Louis Morhet <lmorhet@kalrayinc.com>
> Co-developed-by: Marius Gligor <mgligor@kalrayinc.com>
> Signed-off-by: Marius Gligor <mgligor@kalrayinc.com>
> Signed-off-by: Jules Maselbas <jmaselbas@zdiv.net>
> Signed-off-by: Yann Sionneau <ysionneau@kalrayinc.com>
> ---
> 
> Notes:
> V1 -> V2: new patch
> V2 -> V3:
> - Moved driver from arch/kvx/platform to drivers/soc/kvx/
>   see discussions there:
>   - https://lore.kernel.org/bpf/Y8qlOpYgDefMPqWH@zx2c4.com/T/#m722d8f7c7501615251e4f97705198f5485865ce2
> - indent
> - add missing static qualifier
> - driver now registers a cpu_method/smp_op via CPU_METHOD_OF_DECLARE
>   like arm and sh, it puts a struct into a __cpu_method_of_table ELF section.
>   the smp_ops is used by smpboot.c if its name matches the DT 'cpus' node
>   enable-method property.
> ---
>  arch/kvx/include/asm/pwr_ctrl.h     | 57 ++++++++++++++++++++
>  drivers/soc/Kconfig                 |  1 +
>  drivers/soc/Makefile                |  1 +
>  drivers/soc/kvx/Kconfig             | 10 ++++
>  drivers/soc/kvx/Makefile            |  2 +
>  drivers/soc/kvx/coolidge_pwr_ctrl.c | 84 +++++++++++++++++++++++++++++
>  6 files changed, 155 insertions(+)
>  create mode 100644 arch/kvx/include/asm/pwr_ctrl.h
>  create mode 100644 drivers/soc/kvx/Kconfig
>  create mode 100644 drivers/soc/kvx/Makefile
>  create mode 100644 drivers/soc/kvx/coolidge_pwr_ctrl.c
> 
> diff --git a/arch/kvx/include/asm/pwr_ctrl.h b/arch/kvx/include/asm/pwr_ctrl.h
> new file mode 100644
> index 0000000000000..715eddd45a88c
> --- /dev/null
> +++ b/arch/kvx/include/asm/pwr_ctrl.h
> @@ -0,0 +1,57 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (C) 2017-2024 Kalray Inc.
> + * Author(s): Clement Leger
> + *            Marius Gligor
> + *            Julian Vetter
> + *            Yann Sionneau
> + */
> +
> +#ifndef _ASM_KVX_PWR_CTRL_H
> +#define _ASM_KVX_PWR_CTRL_H
> +
> +#ifndef __ASSEMBLY__
> +
> +static int kvx_pwr_ctrl_probe(void);
> +
> +int kvx_pwr_ctrl_cpu_poweron(unsigned int cpu);
> +
> +#endif
> +
> +#define PWR_CTRL_ADDR                           0xA40000
> +
> +/* Power controller vector register definitions */
> +#define KVX_PWR_CTRL_VEC_OFFSET                 0x1000
> +#define KVX_PWR_CTRL_VEC_WUP_SET_OFFSET         0x10
> +#define KVX_PWR_CTRL_VEC_WUP_CLEAR_OFFSET       0x20
> +
> +/* Power controller PE reset PC register definitions */
> +#define KVX_PWR_CTRL_RESET_PC_OFFSET            0x2000
> +
> +/* Power controller global register definitions */
> +#define KVX_PWR_CTRL_GLOBAL_OFFSET              0x4040
> +
> +#define KVX_PWR_CTRL_GLOBAL_SET_OFFSET          0x10
> +#define KVX_PWR_CTRL_GLOBAL_CLEAR_OFFSET        0x20
> +#define KVX_PWR_CTRL_GLOBAL_SET_PE_EN_SHIFT     0x1
> +
> +#define PWR_CTRL_WUP_SET_OFFSET  \
> +		(KVX_PWR_CTRL_VEC_OFFSET + \
> +		 KVX_PWR_CTRL_VEC_WUP_SET_OFFSET)
> +
> +#define PWR_CTRL_WUP_CLEAR_OFFSET  \
> +		(KVX_PWR_CTRL_VEC_OFFSET + \
> +		 KVX_PWR_CTRL_VEC_WUP_CLEAR_OFFSET)
> +
> +#define PWR_CTRL_GLOBAL_CONFIG_SET_OFFSET \
> +		(KVX_PWR_CTRL_GLOBAL_OFFSET + \
> +		 KVX_PWR_CTRL_GLOBAL_SET_OFFSET)
> +
> +#define PWR_CTRL_GLOBAL_CONFIG_CLEAR_OFFSET \
> +		(KVX_PWR_CTRL_GLOBAL_OFFSET + \
> +		 KVX_PWR_CTRL_GLOBAL_CLEAR_OFFSET)
> +
> +#define PWR_CTRL_GLOBAL_CONFIG_PE_EN \
> +	(1 << KVX_PWR_CTRL_GLOBAL_SET_PE_EN_SHIFT)
> +
> +#endif /* _ASM_KVX_PWR_CTRL_H */
> diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig
> index 5d924e946507b..f28078620da14 100644
> --- a/drivers/soc/Kconfig
> +++ b/drivers/soc/Kconfig
> @@ -12,6 +12,7 @@ source "drivers/soc/fujitsu/Kconfig"
>  source "drivers/soc/hisilicon/Kconfig"
>  source "drivers/soc/imx/Kconfig"
>  source "drivers/soc/ixp4xx/Kconfig"
> +source "drivers/soc/kvx/Kconfig"
>  source "drivers/soc/litex/Kconfig"
>  source "drivers/soc/loongson/Kconfig"
>  source "drivers/soc/mediatek/Kconfig"
> diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
> index fb2bd31387d07..240e148eaaff8 100644
> --- a/drivers/soc/Makefile
> +++ b/drivers/soc/Makefile
> @@ -16,6 +16,7 @@ obj-$(CONFIG_ARCH_GEMINI)	+= gemini/
>  obj-y				+= hisilicon/
>  obj-y				+= imx/
>  obj-y				+= ixp4xx/
> +obj-$(CONFIG_KVX)		+= kvx/
>  obj-$(CONFIG_SOC_XWAY)		+= lantiq/
>  obj-$(CONFIG_LITEX_SOC_CONTROLLER) += litex/
>  obj-y				+= loongson/
> diff --git a/drivers/soc/kvx/Kconfig b/drivers/soc/kvx/Kconfig
> new file mode 100644
> index 0000000000000..96d05efe4bfb5
> --- /dev/null
> +++ b/drivers/soc/kvx/Kconfig
> @@ -0,0 +1,10 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +config COOLIDGE_POWER_CONTROLLER
> +	bool "Coolidge power controller"
> +	default n
> +	depends on KVX
> +	help
> +	  The Kalray Coolidge Power Controller is used to manage the power
> +	  state of secondary CPU cores. Currently only powering up is
> +	  supported.
> diff --git a/drivers/soc/kvx/Makefile b/drivers/soc/kvx/Makefile
> new file mode 100644
> index 0000000000000..c7b0b3e99eabc
> --- /dev/null
> +++ b/drivers/soc/kvx/Makefile
> @@ -0,0 +1,2 @@
> +# SPDX-License-Identifier: GPL-2.0
> +obj-$(CONFIG_COOLIDGE_POWER_CONTROLLER)	+= coolidge_pwr_ctrl.o
> diff --git a/drivers/soc/kvx/coolidge_pwr_ctrl.c b/drivers/soc/kvx/coolidge_pwr_ctrl.c
> new file mode 100644
> index 0000000000000..67af3e446d0e7
> --- /dev/null
> +++ b/drivers/soc/kvx/coolidge_pwr_ctrl.c
> @@ -0,0 +1,84 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2017-2024 Kalray Inc.
> + * Author(s): Clement Leger
> + *            Yann Sionneau
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/of_platform.h>
> +#include <linux/slab.h>
> +#include <linux/smp.h>
> +#include <linux/types.h>
> +
> +#include <asm/pwr_ctrl.h>
> +#include <asm/symbols.h>
> +
> +struct kvx_pwr_ctrl {
> +	void __iomem *regs;
> +};
> +
> +static struct kvx_pwr_ctrl kvx_pwr_controller;
> +
> +static bool pwr_ctrl_not_initialized = true;

Do not use inverted meanings.

> +
> +/**
> + * kvx_pwr_ctrl_cpu_poweron() - Wakeup a cpu
> + * @cpu: cpu to wakeup
> + */
> +int __init kvx_pwr_ctrl_cpu_poweron(unsigned int cpu)
> +{
> +	int ret = 0;
> +
> +	if (pwr_ctrl_not_initialized) {
> +		pr_err("KVX power controller not initialized!\n");
> +		return -ENODEV;
> +	}
> +
> +	/* Set PE boot address */
> +	writeq((unsigned long long)kvx_start,

Addresses use kernel_ulong_t

> +			kvx_pwr_controller.regs + KVX_PWR_CTRL_RESET_PC_OFFSET);
> +	/* Wake up processor ! */
> +	writeq(1ULL << cpu,

That's BIT

> +	       kvx_pwr_controller.regs + PWR_CTRL_WUP_SET_OFFSET);
> +	/* Then clear wakeup to allow processor to sleep */
> +	writeq(1ULL << cpu,

BIT

> +	       kvx_pwr_controller.regs + PWR_CTRL_WUP_CLEAR_OFFSET);
> +
> +	return ret;
> +}
> +
> +static const struct smp_operations coolidge_smp_ops __initconst = {
> +	.smp_boot_secondary = kvx_pwr_ctrl_cpu_poweron,
> +};
> +
> +static int __init kvx_pwr_ctrl_probe(void)

That's not a probe, please rename to avoid confusion. Or make it a
proper device driver.

> +{
> +	struct device_node *ctrl;
> +
> +	ctrl = of_find_compatible_node(NULL, NULL, "kalray,coolidge-pwr-ctrl");
> +	if (!ctrl) {
> +		pr_err("Failed to get power controller node\n");
> +		return -EINVAL;
> +	}
> +
> +	kvx_pwr_controller.regs = of_iomap(ctrl, 0);
> +	if (!kvx_pwr_controller.regs) {
> +		pr_err("Failed ioremap\n");
> +		return -EINVAL;
> +	}
> +
> +	pwr_ctrl_not_initialized = false;
> +	pr_info("KVX power controller probed\n");
> +
> +	return 0;
> +}
> +
> +CPU_METHOD_OF_DECLARE(coolidge_pwr_ctrl, "kalray,coolidge-pwr-ctrl",
> +		      &coolidge_smp_ops);
> +
> +early_initcall(kvx_pwr_ctrl_probe);

Best regards,
Krzysztof