diff mbox series

[RFC,v2,30/31] kvx: Add power controller driver

Message ID 20230120141002.2442-31-ysionneau@kalray.eu (mailing list archive)
State Not Applicable
Headers show
Series Upstream kvx Linux port | expand

Commit Message

Yann Sionneau Jan. 20, 2023, 2:10 p.m. UTC
From: Jules Maselbas <jmaselbas@kalray.eu>

The Power Controller (pwr-ctrl) control 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@kalray.eu>
Signed-off-by: Julian Vetter <jvetter@kalray.eu>
Co-developed-by: Louis Morhet <lmorhet@kalray.eu>
Signed-off-by: Louis Morhet <lmorhet@kalray.eu>
Co-developed-by: Marius Gligor <mgligor@kalray.eu>
Signed-off-by: Marius Gligor <mgligor@kalray.eu>
Signed-off-by: Jules Maselbas <jmaselbas@kalray.eu>
Signed-off-by: Yann Sionneau <ysionneau@kalray.eu>
---

Notes:
    V1 -> V2: new patch

 arch/kvx/include/asm/pwr_ctrl.h | 45 ++++++++++++++++
 arch/kvx/platform/Makefile      |  6 +++
 arch/kvx/platform/pwr_ctrl.c    | 91 +++++++++++++++++++++++++++++++++
 3 files changed, 142 insertions(+)
 create mode 100644 arch/kvx/include/asm/pwr_ctrl.h
 create mode 100644 arch/kvx/platform/Makefile
 create mode 100644 arch/kvx/platform/pwr_ctrl.c

Comments

Krzysztof Kozlowski Jan. 22, 2023, 11:54 a.m. UTC | #1
On 20/01/2023 15:10, Yann Sionneau wrote:
> From: Jules Maselbas <jmaselbas@kalray.eu>
> 
> The Power Controller (pwr-ctrl) control cores reset and wake-up
> procedure.


> +
> +static struct device_node * __init get_pwr_ctrl_node(void)
> +{
> +	const phandle *ph;
> +	struct device_node *cpu;
> +	struct device_node *node;
> +
> +	cpu = of_get_cpu_node(raw_smp_processor_id(), NULL);
> +	if (!cpu) {
> +		pr_err("Failed to get CPU node\n");
> +		return NULL;
> +	}
> +
> +	ph = of_get_property(cpu, "power-controller", NULL);
> +	if (!ph) {
> +		pr_err("Failed to get power-controller phandle\n");
> +		return NULL;
> +	}
> +
> +	node = of_find_node_by_phandle(be32_to_cpup(ph));
> +	if (!node) {
> +		pr_err("Failed to get power-controller node\n");
> +		return NULL;
> +	}
> +
> +	return node;
> +}
> +
> +int __init kvx_pwr_ctrl_probe(void)
> +{
> +	struct device_node *ctrl;
> +
> +	ctrl = get_pwr_ctrl_node();
> +	if (!ctrl) {
> +		pr_err("Failed to get power controller node\n");
> +		return -EINVAL;
> +	}
> +
> +	if (!of_device_is_compatible(ctrl, "kalray,kvx-pwr-ctrl")) {
> +		pr_err("Failed to get power controller node\n");

No. Drivers go to drivers, not to arch directory. This should be a
proper driver instead of some fake stub doing its own driver matching.
You need to rework this.

Best regards,
Krzysztof
Yann Sionneau April 15, 2024, 2:08 p.m. UTC | #2
Hello Krzysztof, Arnd, all,

On 1/22/23 12:54, Krzysztof Kozlowski wrote:
> On 20/01/2023 15:10, Yann Sionneau wrote:
>> From: Jules Maselbas <jmaselbas@kalray.eu>
>>
>> The Power Controller (pwr-ctrl) control cores reset and wake-up
>> procedure.
>> +
>> +int __init kvx_pwr_ctrl_probe(void)
>> +{
>> +	struct device_node *ctrl;
>> +
>> +	ctrl = get_pwr_ctrl_node();
>> +	if (!ctrl) {
>> +		pr_err("Failed to get power controller node\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (!of_device_is_compatible(ctrl, "kalray,kvx-pwr-ctrl")) {
>> +		pr_err("Failed to get power controller node\n");
> No. Drivers go to drivers, not to arch directory. This should be a
> proper driver instead of some fake stub doing its own driver matching.
> You need to rework this.

I am working on a v3 patchset, therefore I am working on a solution for 
this "pwr-ctrl" driver that needs to go somewhere else than arch/kvx/.

The purpose of this "driver" is just to expose a void 
kvx_pwr_ctrl_cpu_poweron(unsigned int cpu) function, used by 
kernel/smpboot.c function __cpu_up() in order to start secondary CPUs in 
SMP config.

Doing this, on our SoC, requires writing 3 registers in a memory-mapped 
device named "power controller".

I made some researches in drivers/ but I am not sure yet what's a good 
place that fits what our device is doing (booting secondary CPUs).

* drivers/power/reset seems to be for resetting the entire SoC

* drivers/power/supply seems to be to control power supplies ICs/periph.

* drivers/reset seems to be for device reset

* drivers/pmdomain maybe ?

* drivers/soc ?

* drivers/platform ?

* drivers/misc ?

What do you think?

Thanks.

Regards,
Arnd Bergmann April 15, 2024, 3:30 p.m. UTC | #3
On Mon, Apr 15, 2024, at 16:08, Yann Sionneau wrote:
> On 1/22/23 12:54, Krzysztof Kozlowski wrote:
>> On 20/01/2023 15:10, Yann Sionneau wrote:
>>> From: Jules Maselbas <jmaselbas@kalray.eu>
>>>
>>> The Power Controller (pwr-ctrl) control cores reset and wake-up
>>> procedure.
>>> +
>>> +int __init kvx_pwr_ctrl_probe(void)
>>> +{
>>> +	struct device_node *ctrl;
>>> +
>>> +	ctrl = get_pwr_ctrl_node();
>>> +	if (!ctrl) {
>>> +		pr_err("Failed to get power controller node\n");
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	if (!of_device_is_compatible(ctrl, "kalray,kvx-pwr-ctrl")) {
>>> +		pr_err("Failed to get power controller node\n");
>> No. Drivers go to drivers, not to arch directory. This should be a
>> proper driver instead of some fake stub doing its own driver matching.
>> You need to rework this.
>
> I am working on a v3 patchset, therefore I am working on a solution for 
> this "pwr-ctrl" driver that needs to go somewhere else than arch/kvx/.
>
> The purpose of this "driver" is just to expose a void 
> kvx_pwr_ctrl_cpu_poweron(unsigned int cpu) function, used by 
> kernel/smpboot.c function __cpu_up() in order to start secondary CPUs in 
> SMP config.
>
> Doing this, on our SoC, requires writing 3 registers in a memory-mapped 
> device named "power controller".
>
> I made some researches in drivers/ but I am not sure yet what's a good 
> place that fits what our device is doing (booting secondary CPUs).
>
> * drivers/power/reset seems to be for resetting the entire SoC
>
> * drivers/power/supply seems to be to control power supplies ICs/periph.
>
> * drivers/reset seems to be for device reset
>
> * drivers/pmdomain maybe ?

Right, I don't think any of the above are appropriate

> * drivers/soc ?
>
> * drivers/platform ?
>
> * drivers/misc ?

Not drivers/misc, that is mainly for things with a user-space
interface.

drivers/soc is mainly for drivers used by other drivers, but
this would work, especially if you expect to have multiple
SoC variants that all use the same architecture code but
incompatible register layouts

drivers/platform is really for things outside of the SoC
that are used for managing the system, especially across
architectures, so I don't think that is a good fit.

Traditionally we had this code in arch/{arm,mips,powerpc,sh,x86}
and we never created a drier subsystem for it since newer
targets (arm64, riscv, newer arm, most x86) all use a method
that is specified as part of the ISA or firmware interface.

The question what you expect to see with future hardware
iterations: if you think all arch/kvx/ hardware will use the
same code for maybe at least the next five years, I would
suggest you keep it in arch/kvx/kernel/smp.c, but if you
know or expect other implementations to be needed, I can
merge it as a driver through drivers/soc/.

     Arnd
Krzysztof Kozlowski April 17, 2024, 7:20 p.m. UTC | #4
On 15/04/2024 16:08, Yann Sionneau wrote:
> Hello Krzysztof, Arnd, all,
> 
> On 1/22/23 12:54, Krzysztof Kozlowski wrote:
>> On 20/01/2023 15:10, Yann Sionneau wrote:
>>> From: Jules Maselbas <jmaselbas@kalray.eu>
>>>
>>> The Power Controller (pwr-ctrl) control cores reset and wake-up
>>> procedure.
>>> +
>>> +int __init kvx_pwr_ctrl_probe(void)
>>> +{
>>> +	struct device_node *ctrl;
>>> +
>>> +	ctrl = get_pwr_ctrl_node();
>>> +	if (!ctrl) {
>>> +		pr_err("Failed to get power controller node\n");
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	if (!of_device_is_compatible(ctrl, "kalray,kvx-pwr-ctrl")) {
>>> +		pr_err("Failed to get power controller node\n");
>> No. Drivers go to drivers, not to arch directory. This should be a
>> proper driver instead of some fake stub doing its own driver matching.
>> You need to rework this.
> 
> I am working on a v3 patchset, therefore I am working on a solution for 
> this "pwr-ctrl" driver that needs to go somewhere else than arch/kvx/.
> 
> The purpose of this "driver" is just to expose a void 
> kvx_pwr_ctrl_cpu_poweron(unsigned int cpu) function, used by 
> kernel/smpboot.c function __cpu_up() in order to start secondary CPUs in 
> SMP config.

I might be missing here some bigger picture and maybe my original
comment was no appropriate, but IIUC, you might now create dependencies
between arch code and drivers. That's also fragile.

> 
> Doing this, on our SoC, requires writing 3 registers in a memory-mapped 
> device named "power controller".
> 
> I made some researches in drivers/ but I am not sure yet what's a good 
> place that fits what our device is doing (booting secondary CPUs).
> 
> * drivers/power/reset seems to be for resetting the entire SoC
> 
> * drivers/power/supply seems to be to control power supplies ICs/periph.
> 
> * drivers/reset seems to be for device reset
> 
> * drivers/pmdomain maybe ?
> 
> * drivers/soc ?
> 

Bringup of CPU? Then I would vote for here. You also have existing
example: r9a06g032-smp.c

But anyway the point is to make it clear - either it is a driver or core
code. Not both. The original code was not looking like any other CPU
bringup code.

Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/arch/kvx/include/asm/pwr_ctrl.h b/arch/kvx/include/asm/pwr_ctrl.h
new file mode 100644
index 000000000000..25f403ba935a
--- /dev/null
+++ b/arch/kvx/include/asm/pwr_ctrl.h
@@ -0,0 +1,45 @@ 
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (C) 2017-2023 Kalray Inc.
+ * Author(s): Clement Leger
+ *            Marius Gligor
+ */
+
+#ifndef _ASM_KVX_PWR_CTRL_H
+#define _ASM_KVX_PWR_CTRL_H
+
+#ifndef __ASSEMBLY__
+
+int kvx_pwr_ctrl_probe(void);
+
+void kvx_pwr_ctrl_cpu_poweron(unsigned int cpu);
+
+#endif
+
+/* 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_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_OFFSET \
+		(KVX_PWR_CTRL_GLOBAL_OFFSET + \
+		 KVX_PWR_CTRL_GLOBAL_SET_OFFSET)
+
+#endif /* _ASM_KVX_PWR_CTRL_H */
diff --git a/arch/kvx/platform/Makefile b/arch/kvx/platform/Makefile
new file mode 100644
index 000000000000..c7d0abb15c27
--- /dev/null
+++ b/arch/kvx/platform/Makefile
@@ -0,0 +1,6 @@ 
+# SPDX-License-Identifier: GPL-2.0-only
+#
+# Copyright (C) 2018-2023 Kalray Inc.
+#
+
+obj-$(CONFIG_SMP) += pwr_ctrl.o
diff --git a/arch/kvx/platform/pwr_ctrl.c b/arch/kvx/platform/pwr_ctrl.c
new file mode 100644
index 000000000000..ee35d04845ae
--- /dev/null
+++ b/arch/kvx/platform/pwr_ctrl.c
@@ -0,0 +1,91 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2017-2023 Kalray Inc.
+ * Author(s): Clement Leger
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/io.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/of_platform.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;
+
+/**
+ * kvx_pwr_ctrl_cpu_poweron() - Wakeup a cpu
+ * @cpu: cpu to wakeup
+ */
+void kvx_pwr_ctrl_cpu_poweron(unsigned int cpu)
+{
+	/* Set PE boot address */
+	writeq((unsigned long long)kvx_start,
+			kvx_pwr_controller.regs + KVX_PWR_CTRL_RESET_PC_OFFSET);
+	/* Wake up processor ! */
+	writeq(1ULL << cpu,
+	       kvx_pwr_controller.regs + PWR_CTRL_WUP_SET_OFFSET);
+	/* Then clear wakeup to allow processor to sleep */
+	writeq(1ULL << cpu,
+	       kvx_pwr_controller.regs + PWR_CTRL_WUP_CLEAR_OFFSET);
+}
+
+static struct device_node * __init get_pwr_ctrl_node(void)
+{
+	const phandle *ph;
+	struct device_node *cpu;
+	struct device_node *node;
+
+	cpu = of_get_cpu_node(raw_smp_processor_id(), NULL);
+	if (!cpu) {
+		pr_err("Failed to get CPU node\n");
+		return NULL;
+	}
+
+	ph = of_get_property(cpu, "power-controller", NULL);
+	if (!ph) {
+		pr_err("Failed to get power-controller phandle\n");
+		return NULL;
+	}
+
+	node = of_find_node_by_phandle(be32_to_cpup(ph));
+	if (!node) {
+		pr_err("Failed to get power-controller node\n");
+		return NULL;
+	}
+
+	return node;
+}
+
+int __init kvx_pwr_ctrl_probe(void)
+{
+	struct device_node *ctrl;
+
+	ctrl = get_pwr_ctrl_node();
+	if (!ctrl) {
+		pr_err("Failed to get power controller node\n");
+		return -EINVAL;
+	}
+
+	if (!of_device_is_compatible(ctrl, "kalray,kvx-pwr-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;
+	}
+
+	return 0;
+}