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

Krzysztof Kozlowski
2024-07-22T12:37:41Z

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