Discussion:
[RFC PATCH v5 0/1] drivers: mfd: Versatile Express SPC support
(too old to reply)
Lorenzo Pieralisi
2013-07-16 16:05:42 UTC
Permalink
Hello,

version v5 of VExpress SPC driver, please read on the changelog for major
changes and explanations.

The probing scheme is unchanged, since after trying the early platform
devices approach it appeared that the end result was no better than the
current one. The only clean solution relies either on changing how
secondaries are brought up in the kernel (later than now) or enable
early platform device registration through DT. Please check this
thread for the related discussion:

https://lists.ozlabs.org/pipermail/devicetree-discuss/2013-June/036542.html

The interface was adapted to regmap and again reverted to old driver for
the following reasons:

- Power down registers locking is hairy and requires arch spinlocks in
the MCPM back end to work properly, normal spinlocks cannot be used
- Regmap adds unnecessary code to manage SPC since it is just a bunch of
registers used to control power management flags, the overhead is just
not worth it (talking about power down registers, not the vexpress config
interface)
- The locking scheme behind regmap requires all registers in the map
to be protected with the same lock, which is not exactly what we want
here
- Given the reasons above, adding a regmap interface buys us nothing from
a driver readability and maintainability perspective (again just talking
about the power interface, a few registers) because for the SPC it would
simply not be used

/drivers/mfd is probably not the right place for this code as it stands (but
probably will be when the entire driver, with DVFS and config interface, is
complete).

Thank you for the review in advance,
Lorenzo

This patch is v5 of a previous posting:

http://lists.infradead.org/pipermail/linux-arm-kernel/2013-June/177150.html

v5 changes:

- Completely removed vexpress-config interface waiting for refactoring
based on regmap
- Removed frequency scaling interface and operating points programming
retrieval
- Trimmed the driver code and DT bindings

v4 changes:
- Applied review comments (trimmed function names, added comments, refactored
some APIs)
- Added comments throughout the set
- Fixed irq handler bug in checking the transaction status
- Improved commit log to explain early init synchro scheme
- Created a single static structure for variables dynamically allocated to
remove usage of static
- Improved Kconfig entry

v3 changes:

- added __refdata to spc_check_loaded pointer
- removed some exported symbols
- added node pointer check in vexpress_spc_init()

v2 changes:

- Dropped timeout interface patch
- Converted interfaces to non-timeout ones, integrated and retested
- Removed mutex used at init
- Refactored code to work around init sections warning
- Fixed two minor bugs

Lorenzo Pieralisi (1):
drivers: mfd: vexpress: add Serial Power Controller (SPC) support

Documentation/devicetree/bindings/mfd/vexpress-spc.txt | 36 ++
drivers/mfd/Kconfig | 10 +
drivers/mfd/Makefile | 1 +
drivers/mfd/vexpress-spc.c | 253 ++++++++++
include/linux/vexpress.h | 17 +
5 files changed, 317 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mfd/vexpress-spc.txt
create mode 100644 drivers/mfd/vexpress-spc.c
--
1.8.2.2
Lorenzo Pieralisi
2013-07-16 16:05:43 UTC
Permalink
The TC2 versatile express core tile integrates a logic block that provides the
interface between the dual cluster test-chip and the M3 microcontroller that
carries out power management. The logic block, called Serial Power Controller
(SPC), contains several memory mapped registers to control among other things
low-power states, wake-up irqs and per-CPU jump addresses registers.

This patch provides a driver that enables run-time control of features
implemented by the SPC power management control logic.

The SPC control logic is required to be programmed very early in the boot
process to reset secondary CPUs on the TC2 testchip, set-up jump addresses and
wake-up IRQs for power management. Hence, waiting for core changes to be
made in the device core code to enable early registration of platform
devices, the driver puts in place an early init scheme that allows kernel
drivers to initialize the SPC driver directly from the components requiring
it, if their initialization routine is called before the driver init
function by the boot process.

Device tree bindings documentation for the SPC component is provided with
the patchset.

Cc: Samuel Ortiz <sameo-VuQAYsv1563Yd54FQh9/***@public.gmane.org>
Cc: Olof Johansson <olof-***@public.gmane.org>
Cc: Pawel Moll <pawel.moll-***@public.gmane.org>
Cc: Amit Kucheria <amit.kucheria-QSEj5FYQhm4dnm+***@public.gmane.org>
Cc: Jon Medhurst <tixy-QSEj5FYQhm4dnm+***@public.gmane.org>
Signed-off-by: Achin Gupta <achin.gupta-***@public.gmane.org>
Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi-***@public.gmane.org>
Signed-off-by: Sudeep KarkadaNagesha <Sudeep.KarkadaNagesha-***@public.gmane.org>
---
Documentation/devicetree/bindings/mfd/vexpress-spc.txt | 36 ++
drivers/mfd/Kconfig | 10 +
drivers/mfd/Makefile | 1 +
drivers/mfd/vexpress-spc.c | 253 ++++++++++
include/linux/vexpress.h | 17 +
5 files changed, 317 insertions(+)

diff --git a/Documentation/devicetree/bindings/mfd/vexpress-spc.txt b/Documentation/devicetree/bindings/mfd/vexpress-spc.txt
new file mode 100644
index 0000000..1614725
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/vexpress-spc.txt
@@ -0,0 +1,36 @@
+* ARM Versatile Express Serial Power Controller device tree bindings
+
+Latest ARM development boards implement a power management interface (serial
+power controller - SPC) that is capable of managing power states transitions,
+wake-up IRQs and resume addresses for ARM multiprocessor testchips.
+The serial controller can be programmed through a memory mapped interface
+that enables communication between firmware running on the microcontroller
+managing power states and the application processors.
+
+The SPC DT bindings are defined as follows:
+
+- spc node
+
+ - compatible:
+ Usage: required
+ Value type: <stringlist>
+ Definition: must be
+ "arm,vexpress-spc,v2p-ca15_a7", "arm,vexpress-spc"
+ - reg:
+ Usage: required
+ Value type: <prop-encode-array>
+ Definition: A standard property that specifies the base address
+ and the size of the SPC address space
+ - interrupts:
+ Usage: required
+ Value type: <prop-encoded-array>
+ Definition: SPC interrupt configuration. A standard property
+ that follows ePAPR interrupts specifications
+
+Example:
+
+spc: ***@7fff0000 {
+ compatible = "arm,vexpress-spc,v2p-ca15_a7", "arm,vexpress-spc";
+ reg = <0x7fff0000 0x1000>;
+ interrupts = <0 95 4>;
+};
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 6959b8d..ebd23f4 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1149,3 +1149,13 @@ config VEXPRESS_CONFIG
help
Platform configuration infrastructure for the ARM Ltd.
Versatile Express.
+
+config VEXPRESS_SPC
+ bool "Versatile Express SPC driver support"
+ depends on ARM
+ help
+ The Serial Power Controller (SPC) for ARM Ltd. test chips, is
+ an IP that provides a memory mapped interface to power controller
+ HW. The driver provides an API abstraction allowing to program
+ registers controlling low-level power management features like power
+ down flags, global and per-cpu wake-up IRQs.
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 718e94a..3a01203 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -153,5 +153,6 @@ obj-$(CONFIG_MFD_SEC_CORE) += sec-core.o sec-irq.o
obj-$(CONFIG_MFD_SYSCON) += syscon.o
obj-$(CONFIG_MFD_LM3533) += lm3533-core.o lm3533-ctrlbank.o
obj-$(CONFIG_VEXPRESS_CONFIG) += vexpress-config.o vexpress-sysreg.o
+obj-$(CONFIG_VEXPRESS_SPC) += vexpress-spc.o
obj-$(CONFIG_MFD_RETU) += retu-mfd.o
obj-$(CONFIG_MFD_AS3711) += as3711.o
diff --git a/drivers/mfd/vexpress-spc.c b/drivers/mfd/vexpress-spc.c
new file mode 100644
index 0000000..aa8c2a4
--- /dev/null
+++ b/drivers/mfd/vexpress-spc.c
@@ -0,0 +1,253 @@
+/*
+ * Versatile Express Serial Power Controller (SPC) support
+ *
+ * Copyright (C) 2013 ARM Ltd.
+ *
+ * Authors: Sudeep KarkadaNagesha <sudeep.karkadanagesha-***@public.gmane.org>
+ * Achin Gupta <achin.gupta-***@public.gmane.org>
+ * Lorenzo Pieralisi <lorenzo.pieralisi-***@public.gmane.org>
+ *
+ * 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.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/of_address.h>
+#include <linux/slab.h>
+#include <linux/vexpress.h>
+
+#include <asm/cacheflush.h>
+
+#define SPCLOG "vexpress-spc: "
+
+/* SCC conf registers */
+#define A15_CONF 0x400
+#define SYS_INFO 0x700
+
+/* SPC registers base */
+#define SPC_BASE 0xB00
+
+/* SPC wake-up IRQs status and mask */
+#define WAKE_INT_MASK (SPC_BASE + 0x24)
+#define WAKE_INT_RAW (SPC_BASE + 0x28)
+#define WAKE_INT_STAT (SPC_BASE + 0x2c)
+/* SPC power down registers */
+#define A15_PWRDN_EN (SPC_BASE + 0x30)
+#define A7_PWRDN_EN (SPC_BASE + 0x34)
+/* SPC per-CPU mailboxes */
+#define A15_BX_ADDR0 (SPC_BASE + 0x68)
+#define A7_BX_ADDR0 (SPC_BASE + 0x78)
+
+/* wake-up interrupt masks */
+#define GBL_WAKEUP_INT_MSK (0x3 << 10)
+
+/* TC2 static dual-cluster configuration */
+#define MAX_CLUSTERS 2
+
+struct ve_spc_drvdata {
+ void __iomem *baseaddr;
+ /*
+ * A15s cluster identifier
+ * It corresponds to A15 processors MPIDR[15:8] bitfield
+ */
+ u32 a15_clusid;
+};
+
+static struct ve_spc_drvdata *info;
+
+static inline bool cluster_is_a15(u32 cluster)
+{
+ return cluster == info->a15_clusid;
+}
+
+/**
+ * ve_spc_global_wakeup_irq()
+ *
+ * Function to set/clear global wakeup IRQs. Not protected by locking since
+ * it might be used in code paths where normal cacheable locks are not
+ * working. Locking must be provided by the caller to ensure atomicity.
+ *
+ * @set: if true, global wake-up IRQs are set, if false they are cleared
+ */
+void ve_spc_global_wakeup_irq(bool set)
+{
+ u32 reg;
+
+ reg = readl_relaxed(info->baseaddr + WAKE_INT_MASK);
+
+ if (set)
+ reg |= GBL_WAKEUP_INT_MSK;
+ else
+ reg &= ~GBL_WAKEUP_INT_MSK;
+
+ writel_relaxed(reg, info->baseaddr + WAKE_INT_MASK);
+}
+
+/**
+ * ve_spc_cpu_wakeup_irq()
+ *
+ * Function to set/clear per-CPU wake-up IRQs. Not protected by locking since
+ * it might be used in code paths where normal cacheable locks are not
+ * working. Locking must be provided by the caller to ensure atomicity.
+ *
+ * @cluster: mpidr[15:8] bitfield describing cluster affinity level
+ * @cpu: mpidr[7:0] bitfield describing cpu affinity level
+ * @set: if true, wake-up IRQs are set, if false they are cleared
+ */
+void ve_spc_cpu_wakeup_irq(u32 cluster, u32 cpu, bool set)
+{
+ u32 mask, reg;
+
+ if (cluster >= MAX_CLUSTERS)
+ return;
+
+ mask = 1 << cpu;
+
+ if (!cluster_is_a15(cluster))
+ mask <<= 4;
+
+ reg = readl_relaxed(info->baseaddr + WAKE_INT_MASK);
+
+ if (set)
+ reg |= mask;
+ else
+ reg &= ~mask;
+
+ writel_relaxed(reg, info->baseaddr + WAKE_INT_MASK);
+}
+
+/**
+ * ve_spc_set_resume_addr() - set the jump address used for warm boot
+ *
+ * @cluster: mpidr[15:8] bitfield describing cluster affinity level
+ * @cpu: mpidr[7:0] bitfield describing cpu affinity level
+ * @addr: physical resume address
+ */
+void ve_spc_set_resume_addr(u32 cluster, u32 cpu, u32 addr)
+{
+ void __iomem *baseaddr;
+
+ if (cluster >= MAX_CLUSTERS)
+ return;
+
+ if (cluster_is_a15(cluster))
+ baseaddr = info->baseaddr + A15_BX_ADDR0 + (cpu << 2);
+ else
+ baseaddr = info->baseaddr + A7_BX_ADDR0 + (cpu << 2);
+
+ writel_relaxed(addr, baseaddr);
+}
+
+/**
+ * ve_spc_get_nr_cpus() - get number of cpus in a cluster
+ *
+ * @cluster: mpidr[15:8] bitfield describing cluster affinity level
+ *
+ * Return: > 0 number of cpus in the cluster
+ * or 0 if cluster number invalid
+ */
+u32 ve_spc_get_nr_cpus(u32 cluster)
+{
+ u32 val;
+
+ if (cluster >= MAX_CLUSTERS)
+ return 0;
+
+ val = readl_relaxed(info->baseaddr + SYS_INFO);
+ val = cluster_is_a15(cluster) ? (val >> 16) : (val >> 20);
+ return val & 0xf;
+}
+
+/**
+ * ve_spc_powerdown()
+ *
+ * Function to enable/disable cluster powerdown. Not protected by locking
+ * since it might be used in code paths where normal cacheable locks are not
+ * working. Locking must be provided by the caller to ensure atomicity.
+ *
+ * @cluster: mpidr[15:8] bitfield describing cluster affinity level
+ * @enable: if true enables powerdown, if false disables it
+ */
+void ve_spc_powerdown(u32 cluster, bool enable)
+{
+ u32 pwdrn_reg;
+
+ if (cluster >= MAX_CLUSTERS)
+ return;
+
+ pwdrn_reg = cluster_is_a15(cluster) ? A15_PWRDN_EN : A7_PWRDN_EN;
+ writel_relaxed(enable, info->baseaddr + pwdrn_reg);
+}
+
+static const struct of_device_id ve_spc_ids[] __initconst = {
+ { .compatible = "arm,vexpress-spc,v2p-ca15_a7" },
+ { .compatible = "arm,vexpress-spc" },
+ {},
+};
+
+static int __init ve_spc_probe(void)
+{
+ int ret;
+ struct device_node *node = of_find_matching_node(NULL, ve_spc_ids);
+
+ if (!node)
+ return -ENODEV;
+
+ info = kzalloc(sizeof(*info), GFP_KERNEL);
+ if (!info) {
+ pr_err(SPCLOG "unable to allocate mem\n");
+ return -ENOMEM;
+ }
+
+ info->baseaddr = of_iomap(node, 0);
+ if (!info->baseaddr) {
+ pr_err(SPCLOG "unable to ioremap memory\n");
+ ret = -ENXIO;
+ goto mem_free;
+ }
+
+ info->a15_clusid = readl_relaxed(info->baseaddr + A15_CONF) & 0xf;
+
+ /*
+ * Multi-cluster systems may need this data when non-coherent, during
+ * cluster power-up/power-down. Make sure driver info reaches main
+ * memory.
+ */
+ sync_cache_w(info);
+ sync_cache_w(&info);
+ pr_info("vexpress-spc loaded at %p\n", info->baseaddr);
+ return 0;
+
+mem_free:
+ kfree(info);
+ return ret;
+}
+
+/**
+ * ve_spc_init()
+ *
+ * Function exported to manage pre early_initcall initialization.
+ * SPC code is needed very early in the boot process to bring CPUs out of
+ * reset and initialize power management back-end so an init interface is
+ * provided to platform code to allow early initialization. The init
+ * interface can be removed as soon as the DT layer and platform bus allow
+ * platform device creation and probing before SMP boot.
+ */
+int __init ve_spc_init(void)
+{
+ static int ve_spc_init_status __initdata = -EAGAIN;
+
+ if (ve_spc_init_status == -EAGAIN)
+ ve_spc_init_status = ve_spc_probe();
+
+ return ve_spc_init_status;
+}
+early_initcall(ve_spc_init);
diff --git a/include/linux/vexpress.h b/include/linux/vexpress.h
index 50368e0..d0106ef 100644
--- a/include/linux/vexpress.h
+++ b/include/linux/vexpress.h
@@ -132,4 +132,21 @@ void vexpress_clk_of_register_spc(void);
void vexpress_clk_init(void __iomem *sp810_base);
void vexpress_clk_of_init(void);

+/* SPC */
+
+#ifdef CONFIG_VEXPRESS_SPC
+int __init ve_spc_init(void);
+void ve_spc_global_wakeup_irq(bool set);
+void ve_spc_cpu_wakeup_irq(u32 cluster, u32 cpu, bool set);
+void ve_spc_set_resume_addr(u32 cluster, u32 cpu, u32 addr);
+u32 ve_spc_get_nr_cpus(u32 cluster);
+void ve_spc_powerdown(u32 cluster, bool enable);
+#else
+static inline int ve_spc_init(void) { return -ENODEV; }
+static inline void ve_spc_global_wakeup_irq(bool set) { }
+static inline void ve_spc_cpu_wakeup_irq(u32 cluster, u32 cpu, bool set) { }
+static inline void ve_spc_set_resume_addr(u32 cluster, u32 cpu, u32 addr) { }
+static inline u32 ve_spc_get_nr_cpus(u32 cluster) { return 0; }
+static inline void ve_spc_powerdown(u32 cluster, bool enable) { }
+#endif
#endif
--
1.8.2.2
Rob Herring
2013-07-16 20:05:25 UTC
Permalink
Post by Lorenzo Pieralisi
The TC2 versatile express core tile integrates a logic block that provides the
interface between the dual cluster test-chip and the M3 microcontroller that
carries out power management. The logic block, called Serial Power Controller
(SPC), contains several memory mapped registers to control among other things
low-power states, wake-up irqs and per-CPU jump addresses registers.
This patch provides a driver that enables run-time control of features
implemented by the SPC power management control logic.
The SPC control logic is required to be programmed very early in the boot
process to reset secondary CPUs on the TC2 testchip, set-up jump addresses and
wake-up IRQs for power management. Hence, waiting for core changes to be
made in the device core code to enable early registration of platform
devices, the driver puts in place an early init scheme that allows kernel
drivers to initialize the SPC driver directly from the components requiring
it, if their initialization routine is called before the driver init
function by the boot process.
Device tree bindings documentation for the SPC component is provided with
the patchset.
Just curious, wouldn't a TC2 PSCI implementation eliminate the need for
most/all of this code?

Rob
Post by Lorenzo Pieralisi
---
Documentation/devicetree/bindings/mfd/vexpress-spc.txt | 36 ++
drivers/mfd/Kconfig | 10 +
drivers/mfd/Makefile | 1 +
drivers/mfd/vexpress-spc.c | 253 ++++++++++
include/linux/vexpress.h | 17 +
5 files changed, 317 insertions(+)
diff --git a/Documentation/devicetree/bindings/mfd/vexpress-spc.txt b/Documentation/devicetree/bindings/mfd/vexpress-spc.txt
new file mode 100644
index 0000000..1614725
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/vexpress-spc.txt
@@ -0,0 +1,36 @@
+* ARM Versatile Express Serial Power Controller device tree bindings
+
+Latest ARM development boards implement a power management interface (serial
+power controller - SPC) that is capable of managing power states transitions,
+wake-up IRQs and resume addresses for ARM multiprocessor testchips.
+The serial controller can be programmed through a memory mapped interface
+that enables communication between firmware running on the microcontroller
+managing power states and the application processors.
+
+
+- spc node
+
+ Usage: required
+ Value type: <stringlist>
+ Definition: must be
+ "arm,vexpress-spc,v2p-ca15_a7", "arm,vexpress-spc"
+ Usage: required
+ Value type: <prop-encode-array>
+ Definition: A standard property that specifies the base address
+ and the size of the SPC address space
+ Usage: required
+ Value type: <prop-encoded-array>
+ Definition: SPC interrupt configuration. A standard property
+ that follows ePAPR interrupts specifications
+
+
+ compatible = "arm,vexpress-spc,v2p-ca15_a7", "arm,vexpress-spc";
+ reg = <0x7fff0000 0x1000>;
+ interrupts = <0 95 4>;
+};
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 6959b8d..ebd23f4 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1149,3 +1149,13 @@ config VEXPRESS_CONFIG
help
Platform configuration infrastructure for the ARM Ltd.
Versatile Express.
+
+config VEXPRESS_SPC
+ bool "Versatile Express SPC driver support"
+ depends on ARM
+ help
+ The Serial Power Controller (SPC) for ARM Ltd. test chips, is
+ an IP that provides a memory mapped interface to power controller
+ HW. The driver provides an API abstraction allowing to program
+ registers controlling low-level power management features like power
+ down flags, global and per-cpu wake-up IRQs.
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 718e94a..3a01203 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -153,5 +153,6 @@ obj-$(CONFIG_MFD_SEC_CORE) += sec-core.o sec-irq.o
obj-$(CONFIG_MFD_SYSCON) += syscon.o
obj-$(CONFIG_MFD_LM3533) += lm3533-core.o lm3533-ctrlbank.o
obj-$(CONFIG_VEXPRESS_CONFIG) += vexpress-config.o vexpress-sysreg.o
+obj-$(CONFIG_VEXPRESS_SPC) += vexpress-spc.o
obj-$(CONFIG_MFD_RETU) += retu-mfd.o
obj-$(CONFIG_MFD_AS3711) += as3711.o
diff --git a/drivers/mfd/vexpress-spc.c b/drivers/mfd/vexpress-spc.c
new file mode 100644
index 0000000..aa8c2a4
--- /dev/null
+++ b/drivers/mfd/vexpress-spc.c
@@ -0,0 +1,253 @@
+/*
+ * Versatile Express Serial Power Controller (SPC) support
+ *
+ * Copyright (C) 2013 ARM Ltd.
+ *
+ *
+ * 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.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/of_address.h>
+#include <linux/slab.h>
+#include <linux/vexpress.h>
+
+#include <asm/cacheflush.h>
+
+#define SPCLOG "vexpress-spc: "
+
+/* SCC conf registers */
+#define A15_CONF 0x400
+#define SYS_INFO 0x700
+
+/* SPC registers base */
+#define SPC_BASE 0xB00
+
+/* SPC wake-up IRQs status and mask */
+#define WAKE_INT_MASK (SPC_BASE + 0x24)
+#define WAKE_INT_RAW (SPC_BASE + 0x28)
+#define WAKE_INT_STAT (SPC_BASE + 0x2c)
+/* SPC power down registers */
+#define A15_PWRDN_EN (SPC_BASE + 0x30)
+#define A7_PWRDN_EN (SPC_BASE + 0x34)
+/* SPC per-CPU mailboxes */
+#define A15_BX_ADDR0 (SPC_BASE + 0x68)
+#define A7_BX_ADDR0 (SPC_BASE + 0x78)
+
+/* wake-up interrupt masks */
+#define GBL_WAKEUP_INT_MSK (0x3 << 10)
+
+/* TC2 static dual-cluster configuration */
+#define MAX_CLUSTERS 2
+
+struct ve_spc_drvdata {
+ void __iomem *baseaddr;
+ /*
+ * A15s cluster identifier
+ * It corresponds to A15 processors MPIDR[15:8] bitfield
+ */
+ u32 a15_clusid;
+};
+
+static struct ve_spc_drvdata *info;
+
+static inline bool cluster_is_a15(u32 cluster)
+{
+ return cluster == info->a15_clusid;
+}
+
+/**
+ * ve_spc_global_wakeup_irq()
+ *
+ * Function to set/clear global wakeup IRQs. Not protected by locking since
+ * it might be used in code paths where normal cacheable locks are not
+ * working. Locking must be provided by the caller to ensure atomicity.
+ *
+ */
+void ve_spc_global_wakeup_irq(bool set)
+{
+ u32 reg;
+
+ reg = readl_relaxed(info->baseaddr + WAKE_INT_MASK);
+
+ if (set)
+ reg |= GBL_WAKEUP_INT_MSK;
+ else
+ reg &= ~GBL_WAKEUP_INT_MSK;
+
+ writel_relaxed(reg, info->baseaddr + WAKE_INT_MASK);
+}
+
+/**
+ * ve_spc_cpu_wakeup_irq()
+ *
+ * Function to set/clear per-CPU wake-up IRQs. Not protected by locking since
+ * it might be used in code paths where normal cacheable locks are not
+ * working. Locking must be provided by the caller to ensure atomicity.
+ *
+ */
+void ve_spc_cpu_wakeup_irq(u32 cluster, u32 cpu, bool set)
+{
+ u32 mask, reg;
+
+ if (cluster >= MAX_CLUSTERS)
+ return;
+
+ mask = 1 << cpu;
+
+ if (!cluster_is_a15(cluster))
+ mask <<= 4;
+
+ reg = readl_relaxed(info->baseaddr + WAKE_INT_MASK);
+
+ if (set)
+ reg |= mask;
+ else
+ reg &= ~mask;
+
+ writel_relaxed(reg, info->baseaddr + WAKE_INT_MASK);
+}
+
+/**
+ * ve_spc_set_resume_addr() - set the jump address used for warm boot
+ *
+ */
+void ve_spc_set_resume_addr(u32 cluster, u32 cpu, u32 addr)
+{
+ void __iomem *baseaddr;
+
+ if (cluster >= MAX_CLUSTERS)
+ return;
+
+ if (cluster_is_a15(cluster))
+ baseaddr = info->baseaddr + A15_BX_ADDR0 + (cpu << 2);
+ else
+ baseaddr = info->baseaddr + A7_BX_ADDR0 + (cpu << 2);
+
+ writel_relaxed(addr, baseaddr);
+}
+
+/**
+ * ve_spc_get_nr_cpus() - get number of cpus in a cluster
+ *
+ *
+ * Return: > 0 number of cpus in the cluster
+ * or 0 if cluster number invalid
+ */
+u32 ve_spc_get_nr_cpus(u32 cluster)
+{
+ u32 val;
+
+ if (cluster >= MAX_CLUSTERS)
+ return 0;
+
+ val = readl_relaxed(info->baseaddr + SYS_INFO);
+ val = cluster_is_a15(cluster) ? (val >> 16) : (val >> 20);
+ return val & 0xf;
+}
+
+/**
+ * ve_spc_powerdown()
+ *
+ * Function to enable/disable cluster powerdown. Not protected by locking
+ * since it might be used in code paths where normal cacheable locks are not
+ * working. Locking must be provided by the caller to ensure atomicity.
+ *
+ */
+void ve_spc_powerdown(u32 cluster, bool enable)
+{
+ u32 pwdrn_reg;
+
+ if (cluster >= MAX_CLUSTERS)
+ return;
+
+ pwdrn_reg = cluster_is_a15(cluster) ? A15_PWRDN_EN : A7_PWRDN_EN;
+ writel_relaxed(enable, info->baseaddr + pwdrn_reg);
+}
+
+static const struct of_device_id ve_spc_ids[] __initconst = {
+ { .compatible = "arm,vexpress-spc,v2p-ca15_a7" },
+ { .compatible = "arm,vexpress-spc" },
+ {},
+};
+
+static int __init ve_spc_probe(void)
+{
+ int ret;
+ struct device_node *node = of_find_matching_node(NULL, ve_spc_ids);
+
+ if (!node)
+ return -ENODEV;
+
+ info = kzalloc(sizeof(*info), GFP_KERNEL);
+ if (!info) {
+ pr_err(SPCLOG "unable to allocate mem\n");
+ return -ENOMEM;
+ }
+
+ info->baseaddr = of_iomap(node, 0);
+ if (!info->baseaddr) {
+ pr_err(SPCLOG "unable to ioremap memory\n");
+ ret = -ENXIO;
+ goto mem_free;
+ }
+
+ info->a15_clusid = readl_relaxed(info->baseaddr + A15_CONF) & 0xf;
+
+ /*
+ * Multi-cluster systems may need this data when non-coherent, during
+ * cluster power-up/power-down. Make sure driver info reaches main
+ * memory.
+ */
+ sync_cache_w(info);
+ sync_cache_w(&info);
+ pr_info("vexpress-spc loaded at %p\n", info->baseaddr);
+ return 0;
+
+ kfree(info);
+ return ret;
+}
+
+/**
+ * ve_spc_init()
+ *
+ * Function exported to manage pre early_initcall initialization.
+ * SPC code is needed very early in the boot process to bring CPUs out of
+ * reset and initialize power management back-end so an init interface is
+ * provided to platform code to allow early initialization. The init
+ * interface can be removed as soon as the DT layer and platform bus allow
+ * platform device creation and probing before SMP boot.
+ */
+int __init ve_spc_init(void)
+{
+ static int ve_spc_init_status __initdata = -EAGAIN;
+
+ if (ve_spc_init_status == -EAGAIN)
+ ve_spc_init_status = ve_spc_probe();
+
+ return ve_spc_init_status;
+}
+early_initcall(ve_spc_init);
diff --git a/include/linux/vexpress.h b/include/linux/vexpress.h
index 50368e0..d0106ef 100644
--- a/include/linux/vexpress.h
+++ b/include/linux/vexpress.h
@@ -132,4 +132,21 @@ void vexpress_clk_of_register_spc(void);
void vexpress_clk_init(void __iomem *sp810_base);
void vexpress_clk_of_init(void);
+/* SPC */
+
+#ifdef CONFIG_VEXPRESS_SPC
+int __init ve_spc_init(void);
+void ve_spc_global_wakeup_irq(bool set);
+void ve_spc_cpu_wakeup_irq(u32 cluster, u32 cpu, bool set);
+void ve_spc_set_resume_addr(u32 cluster, u32 cpu, u32 addr);
+u32 ve_spc_get_nr_cpus(u32 cluster);
+void ve_spc_powerdown(u32 cluster, bool enable);
+#else
+static inline int ve_spc_init(void) { return -ENODEV; }
+static inline void ve_spc_global_wakeup_irq(bool set) { }
+static inline void ve_spc_cpu_wakeup_irq(u32 cluster, u32 cpu, bool set) { }
+static inline void ve_spc_set_resume_addr(u32 cluster, u32 cpu, u32 addr) { }
+static inline u32 ve_spc_get_nr_cpus(u32 cluster) { return 0; }
+static inline void ve_spc_powerdown(u32 cluster, bool enable) { }
+#endif
#endif
Nicolas Pitre
2013-07-16 23:32:48 UTC
Permalink
Post by Rob Herring
Post by Lorenzo Pieralisi
The TC2 versatile express core tile integrates a logic block that provides the
interface between the dual cluster test-chip and the M3 microcontroller that
carries out power management. The logic block, called Serial Power Controller
(SPC), contains several memory mapped registers to control among other things
low-power states, wake-up irqs and per-CPU jump addresses registers.
This patch provides a driver that enables run-time control of features
implemented by the SPC power management control logic.
The SPC control logic is required to be programmed very early in the boot
process to reset secondary CPUs on the TC2 testchip, set-up jump addresses and
wake-up IRQs for power management. Hence, waiting for core changes to be
made in the device core code to enable early registration of platform
devices, the driver puts in place an early init scheme that allows kernel
drivers to initialize the SPC driver directly from the components requiring
it, if their initialization routine is called before the driver init
function by the boot process.
Device tree bindings documentation for the SPC component is provided with
the patchset.
Just curious, wouldn't a TC2 PSCI implementation eliminate the need for
most/all of this code?
There is a PSCI equivalent for the above already, in the sense that
there is a simple MCPM backend that bypass most of the MCPM race
avoidance code paths and simply calls into PSCI instead. But not all
the world is going to be PSCI, and therefore we need to ensure good
support for non-PSCI platforms as well.

This is why TC2 supports both, and this also ensure that the PSCI
implementation, which won't be part of the kernel and therefore unlikely
to get the same level of scrutiny, is properly implemented and doesn't
introduce any regression when compared to the non PSCI case.

Remember that TC2 is multi-cluster which means that the PSCI
implementation has to carry the same amount of complexity as the whole
in-kernel MCPM layer and that doesn't make me overly confident it is
going to always be right.

All this to say that we do need this code despite PSCI availability.


Nicolas
Nicolas Pitre
2013-07-17 03:26:49 UTC
Permalink
Post by Lorenzo Pieralisi
Hello,
version v5 of VExpress SPC driver, please read on the changelog for major
changes and explanations.
The probing scheme is unchanged, since after trying the early platform
devices approach it appeared that the end result was no better than the
current one. The only clean solution relies either on changing how
secondaries are brought up in the kernel (later than now) or enable
early platform device registration through DT. Please check this
https://lists.ozlabs.org/pipermail/devicetree-discuss/2013-June/036542.html
The interface was adapted to regmap and again reverted to old driver for
- Power down registers locking is hairy and requires arch spinlocks in
the MCPM back end to work properly, normal spinlocks cannot be used
- Regmap adds unnecessary code to manage SPC since it is just a bunch of
registers used to control power management flags, the overhead is just
not worth it (talking about power down registers, not the vexpress config
interface)
- The locking scheme behind regmap requires all registers in the map
to be protected with the same lock, which is not exactly what we want
here
- Given the reasons above, adding a regmap interface buys us nothing from
a driver readability and maintainability perspective (again just talking
about the power interface, a few registers) because for the SPC it would
simply not be used
/drivers/mfd is probably not the right place for this code as it stands (but
probably will be when the entire driver, with DVFS and config interface, is
complete).
Thank you for the review in advance,
Lorenzo
I've integrated this patch in my MCPM backend for TC2 patch series.

ACKs from concerned/interested people would be appreciated so I could
send this for ARM-SOC inclusion right away.


Nicolas
Pawel Moll
2013-07-17 09:18:25 UTC
Permalink
/drivers/mfd is probably not the right place for this code as it stan=
ds (but
probably will be when the entire driver, with DVFS and config interfa=
ce, is
complete).
Not that it really matters now, but my vexpress-sysreg rework will -
most likely - leave only skeleton in the MFD (registering mfd_cells) an=
d
other stuff is going to be spread all around. Then I'm planning to move
the remaining of the vexpress-specific initialization to
drivers/platform/arm/vexpress.c, so maybe sticking vexpress-spc.c to
this (non-existing yet) directory would be the right thing to do?

Other than that:

Acked-by: Pawel Moll <***@arm.com>

Thanks!

Pawe=C5=82
Lorenzo Pieralisi
2013-07-17 10:44:17 UTC
Permalink
Post by Pawel Moll
Post by Lorenzo Pieralisi
/drivers/mfd is probably not the right place for this code as it stands (but
probably will be when the entire driver, with DVFS and config interface, is
complete).
Not that it really matters now, but my vexpress-sysreg rework will -
most likely - leave only skeleton in the MFD (registering mfd_cells) and
other stuff is going to be spread all around. Then I'm planning to move
the remaining of the vexpress-specific initialization to
drivers/platform/arm/vexpress.c, so maybe sticking vexpress-spc.c to
this (non-existing yet) directory would be the right thing to do?
Done. I do not think there is a point in splitting the patch to create
the dir and make infrastructure, so I squashed everything in the
original patch. I have not added any maintainer for that dir/file, I
guess it can wait till you finish the rework so that you can add
yourself there.

If that's all I need to change I do not even think that reposting is
necessary.

It does matter though, since it implies changes on who is in charge of
ack/nack'ing this code, if it is no more an mfd matter.

I will wait to check all interested/concerned parties opinions, that are
always welcome.
Thank you !!!
Lorenzo
Nicolas Pitre
2013-07-17 12:33:02 UTC
Permalink
Post by Pawel Moll
Post by Lorenzo Pieralisi
/drivers/mfd is probably not the right place for this code as it stands (but
probably will be when the entire driver, with DVFS and config interface, is
complete).
Not that it really matters now, but my vexpress-sysreg rework will -
most likely - leave only skeleton in the MFD (registering mfd_cells) and
other stuff is going to be spread all around. Then I'm planning to move
the remaining of the vexpress-specific initialization to
drivers/platform/arm/vexpress.c, so maybe sticking vexpress-spc.c to
this (non-existing yet) directory would be the right thing to do?
I don't like this idea. We worked hard to shrink platform specific
directories such as arch/arm/mach-*/ as much as possible. Simply moving
stuff to drivers/platform/arm/* doesn't make the situation any much
better.

If this is really miscelaneous code that really doesn't fit
anywhere else, it should rather go into drivers/misc/ as a last resort.


Nicolas
Pawel Moll
2013-07-17 13:29:46 UTC
Permalink
If this is really miscelaneous code that really doesn't fit=20
anywhere else, it should rather go into drivers/misc/ as a last resor=
t.

Interestingly enough drivers/misc was my first choice for all the
vexpress stuff, but it wasn't received well...

Anyway, the SPC driver as it is now seem to be a "power management
system driver". Maybe a relevant directory would be in place? Wouldn't
PSCI belong there as well? (there are two psci.c files in arch/arm and
arch/arm64, surprisingly similar ones ;-)

The bottom line is: today it is not an MFD driver.

Pawe=C5=82
Nicolas Pitre
2013-07-17 14:16:25 UTC
Permalink
Post by Pawel Moll
Post by Nicolas Pitre
If this is really miscelaneous code that really doesn't fit
anywhere else, it should rather go into drivers/misc/ as a last resort.
Interestingly enough drivers/misc was my first choice for all the
vexpress stuff, but it wasn't received well...
Anyway, the SPC driver as it is now seem to be a "power management
system driver". Maybe a relevant directory would be in place? Wouldn't
PSCI belong there as well? (there are two psci.c files in arch/arm and
arch/arm64, surprisingly similar ones ;-)
The bottom line is: today it is not an MFD driver.
But we know it will, right? So better save some churn by storing the
initial code where it would end up anyway once complete.


Nicolas
Pawel Moll
2013-07-17 14:20:11 UTC
Permalink
=20
Post by Pawel Moll
If this is really miscelaneous code that really doesn't fit=20
anywhere else, it should rather go into drivers/misc/ as a last r=
esort.
Post by Pawel Moll
=20
Interestingly enough drivers/misc was my first choice for all the
vexpress stuff, but it wasn't received well...
=20
Anyway, the SPC driver as it is now seem to be a "power management
system driver". Maybe a relevant directory would be in place? Would=
n't
Post by Pawel Moll
PSCI belong there as well? (there are two psci.c files in arch/arm =
and
Post by Pawel Moll
arch/arm64, surprisingly similar ones ;-)
=20
The bottom line is: today it is not an MFD driver.
=20
But we know it will, right? So better save some churn by storing th=
e=20
initial code where it would end up anyway once complete.
Not in that form, no. The code living in mfd will just register
mfd_cells while "functional" parts are going to live elsewhere. This is
how I understand what Samuel asked me to do and that's what is happenin=
g
to vexpress-sysreg now.

Pawe=C5=82
Nicolas Pitre
2013-07-17 15:57:55 UTC
Permalink
Post by Pawel Moll
Post by Nicolas Pitre
Post by Pawel Moll
Post by Nicolas Pitre
If this is really miscelaneous code that really doesn't fit
anywhere else, it should rather go into drivers/misc/ as a last resort.
Interestingly enough drivers/misc was my first choice for all the
vexpress stuff, but it wasn't received well...
Anyway, the SPC driver as it is now seem to be a "power management
system driver". Maybe a relevant directory would be in place? Wouldn't
PSCI belong there as well? (there are two psci.c files in arch/arm and
arch/arm64, surprisingly similar ones ;-)
The bottom line is: today it is not an MFD driver.
But we know it will, right? So better save some churn by storing the
initial code where it would end up anyway once complete.
Not in that form, no. The code living in mfd will just register
mfd_cells while "functional" parts are going to live elsewhere. This is
how I understand what Samuel asked me to do and that's what is happening
to vexpress-sysreg now.
A drivers/pm/ or drivers/power/ could be created, but that implies sort
of a more defined subsystem with a common API and the SPC code doesn't
fit that as it is only providing services to actual drivers on top of
it.

The sanest location at this point might simply be
drivers/platform/vexpress_spc.c or drivers/platform/vexpress/spc.c
depending on whether or not more such driver glue is expected in the
vexpress future. No point putting "arm" in the path, especially if this
is later reused on arm64.


Nicolas
Russell King - ARM Linux
2013-07-17 17:00:39 UTC
Permalink
Post by Nicolas Pitre
The sanest location at this point might simply be
drivers/platform/vexpress_spc.c or drivers/platform/vexpress/spc.c
depending on whether or not more such driver glue is expected in the
vexpress future. No point putting "arm" in the path, especially if this
is later reused on arm64.
You wouldn't be making that argument if it were arch/arm64 and arch/arm32 -
you'd probably be arguing that "arm" made perfect sense.

Don't get too hung up on names please, it's really not worth the time
and effort being soo pedantic, and being soo pedantic leads to "pointless
churn" when someone comes along and wants to pedantically change the
names because it no longer matches the users.
Nicolas Pitre
2013-07-17 18:29:02 UTC
Permalink
Post by Russell King - ARM Linux
Post by Nicolas Pitre
The sanest location at this point might simply be
drivers/platform/vexpress_spc.c or drivers/platform/vexpress/spc.c
depending on whether or not more such driver glue is expected in the
vexpress future. No point putting "arm" in the path, especially if this
is later reused on arm64.
You wouldn't be making that argument if it were arch/arm64 and arch/arm32 -
you'd probably be arguing that "arm" made perfect sense.
Well... in a sense: yes. But in the end, having per arch directories
under drivers/ is silly. We already have per arch directories under
arch/already.
Post by Russell King - ARM Linux
Don't get too hung up on names please, it's really not worth the time
and effort being soo pedantic, and being soo pedantic leads to "pointless
churn" when someone comes along and wants to pedantically change the
names because it no longer matches the users.
At this point I don't really care about the name. I just want the damn
thing merged upstream. But after several iterations to either fit one
or another maintainers taste, each rework ends up in that maintainer
saying: "Now that you've reworked the code, I still don't like it since
this no longer fits in my subsystem tree."

In fact what we'd need at this point is
drivers/code_that_no_subsystem_maintainers_wants/. This is becoming
overly ridiculous.


Nicolas
Samuel Ortiz
2013-07-17 21:23:43 UTC
Permalink
Hi Nicolas,
Post by Nicolas Pitre
Post by Russell King - ARM Linux
Post by Nicolas Pitre
The sanest location at this point might simply be
drivers/platform/vexpress_spc.c or drivers/platform/vexpress/spc.c
depending on whether or not more such driver glue is expected in the
vexpress future. No point putting "arm" in the path, especially if this
is later reused on arm64.
You wouldn't be making that argument if it were arch/arm64 and arch/arm32 -
you'd probably be arguing that "arm" made perfect sense.
Well... in a sense: yes. But in the end, having per arch directories
under drivers/ is silly. We already have per arch directories under
arch/already.
Post by Russell King - ARM Linux
Don't get too hung up on names please, it's really not worth the time
and effort being soo pedantic, and being soo pedantic leads to "pointless
churn" when someone comes along and wants to pedantically change the
names because it no longer matches the users.
At this point I don't really care about the name. I just want the damn
thing merged upstream. But after several iterations to either fit one
or another maintainers taste, each rework ends up in that maintainer
saying: "Now that you've reworked the code, I still don't like it since
this no longer fits in my subsystem tree."
FWIW, we asked Pawel to rework the sysreg and config parts of the
vexpress driver, make it an actual MFD driver, and spread the remaining
bits of the code into their respective subsystems. I don't think
this is an eccentric requirement.
Post by Nicolas Pitre
In fact what we'd need at this point is
drivers/code_that_no_subsystem_maintainers_wants/.
Which is what some people think drivers/mfd/ is...
I don't mind merging Lorenzo's SPC driver as it is if he can explain to
me how it will eventually evolve into an actual MFD driver. If that's
not the case, I don't see how I could justify merging it through the
MFD tree.

Cheers,
Samuel.
--
Intel Open Source Technology Centre
http://oss.intel.com/
Nicolas Pitre
2013-07-17 22:22:46 UTC
Permalink
Post by Samuel Ortiz
Post by Nicolas Pitre
At this point I don't really care about the name. I just want the damn
thing merged upstream. But after several iterations to either fit one
or another maintainers taste, each rework ends up in that maintainer
saying: "Now that you've reworked the code, I still don't like it since
this no longer fits in my subsystem tree."
FWIW, we asked Pawel to rework the sysreg and config parts of the
vexpress driver, make it an actual MFD driver, and spread the remaining
bits of the code into their respective subsystems. I don't think
this is an eccentric requirement.
I agree. However the code that Lorenzo just posted can't be deprived
of any more sysreg/config parts. They are simply nonexistent.

Even the larger code you reviewed before is completely useless without
_additional_ drivers to go on top which are indeed waiting after this
code to be merged before they are submitted to their respective
subsystems.

And those additional drivers are way more interesting than this dumb
register access arbitrator. Because this is fundamentally the only
thing it does.
Post by Samuel Ortiz
Post by Nicolas Pitre
In fact what we'd need at this point is
drivers/code_that_no_subsystem_maintainers_wants/.
Which is what some people think drivers/mfd/ is...
Does mfd still stand for "Multi Function Device"?
Post by Samuel Ortiz
I don't mind merging Lorenzo's SPC driver as it is if he can explain to
me how it will eventually evolve into an actual MFD driver. If that's
not the case, I don't see how I could justify merging it through the
MFD tree.
Maybe the misunderstanding is about what actually is a MFD driver.
Given your persisting reluctance, I may only conclude that this is
indeed not a MFD driver after all.

So I'll follow existing precedents in the kernel and move Lorenzo's code
to drivers/platform/vexpress/.


Nicolas
Samuel Ortiz
2013-07-17 22:47:29 UTC
Permalink
Post by Nicolas Pitre
Post by Samuel Ortiz
Post by Nicolas Pitre
At this point I don't really care about the name. I just want the damn
thing merged upstream. But after several iterations to either fit one
or another maintainers taste, each rework ends up in that maintainer
saying: "Now that you've reworked the code, I still don't like it since
this no longer fits in my subsystem tree."
FWIW, we asked Pawel to rework the sysreg and config parts of the
vexpress driver, make it an actual MFD driver, and spread the remaining
bits of the code into their respective subsystems. I don't think
this is an eccentric requirement.
I agree. However the code that Lorenzo just posted can't be deprived
of any more sysreg/config parts.
Yes, and I appreciate Lorenzo's effort here.
Post by Nicolas Pitre
Even the larger code you reviewed before is completely useless without
_additional_ drivers to go on top which are indeed waiting after this
code to be merged before they are submitted to their respective
subsystems.
Right. And merging this code in the right place is exactly what we're
doing here.
Post by Nicolas Pitre
And those additional drivers are way more interesting than this dumb
register access arbitrator.
Yes, they're a lot smarter hence the requirement to have them live into
their respective subsystems where the right maintainer can provide
relevant reviews on them.
Post by Nicolas Pitre
Post by Samuel Ortiz
I don't mind merging Lorenzo's SPC driver as it is if he can explain to
me how it will eventually evolve into an actual MFD driver. If that's
not the case, I don't see how I could justify merging it through the
MFD tree.
Maybe the misunderstanding is about what actually is a MFD driver.
That's possible. I agree it should be documented properly.
Post by Nicolas Pitre
So I'll follow existing precedents in the kernel and move Lorenzo's code
to drivers/platform/vexpress/.
Thanks for that.

Cheers,
Samuel.
--
Intel Open Source Technology Centre
http://oss.intel.com/
Samuel Ortiz
2013-07-17 21:10:21 UTC
Permalink
Hi Pawel,
Post by Pawel Moll
Post by Nicolas Pitre
Post by Pawel Moll
Post by Nicolas Pitre
If this is really miscelaneous code that really doesn't fit
anywhere else, it should rather go into drivers/misc/ as a last resort.
Interestingly enough drivers/misc was my first choice for all the
vexpress stuff, but it wasn't received well...
Anyway, the SPC driver as it is now seem to be a "power management
system driver". Maybe a relevant directory would be in place? Wouldn't
PSCI belong there as well? (there are two psci.c files in arch/arm and
arch/arm64, surprisingly similar ones ;-)
The bottom line is: today it is not an MFD driver.
But we know it will, right? So better save some churn by storing the
initial code where it would end up anyway once complete.
Not in that form, no. The code living in mfd will just register
mfd_cells while "functional" parts are going to live elsewhere. This is
how I understand what Samuel asked me to do and that's what is happening
to vexpress-sysreg now.
Very good, I'll happily apply such changes.
If I understand the IP correctly, the SPC driver will stay as a set of
runtime APIs for controlling the SPC features. If that's the case, then
misc sounds like a more appropriate place for this driver.
drivers/power/ really is for power supplies and charging drivers.

Cheers,
Samuel.
--
Intel Open Source Technology Centre
http://oss.intel.com/
Samuel Ortiz
2013-07-17 21:07:00 UTC
Permalink
Hi Lorenzo,
Post by Lorenzo Pieralisi
Hello,
version v5 of VExpress SPC driver, please read on the changelog for major
changes and explanations.
The probing scheme is unchanged, since after trying the early platform
devices approach it appeared that the end result was no better than the
current one. The only clean solution relies either on changing how
secondaries are brought up in the kernel (later than now) or enable
early platform device registration through DT. Please check this
https://lists.ozlabs.org/pipermail/devicetree-discuss/2013-June/036542.html
The interface was adapted to regmap and again reverted to old driver for
- Power down registers locking is hairy and requires arch spinlocks in
the MCPM back end to work properly, normal spinlocks cannot be used
- Regmap adds unnecessary code to manage SPC since it is just a bunch of
registers used to control power management flags, the overhead is just
not worth it (talking about power down registers, not the vexpress config
interface)
- The locking scheme behind regmap requires all registers in the map
to be protected with the same lock, which is not exactly what we want
here
- Given the reasons above, adding a regmap interface buys us nothing from
a driver readability and maintainability perspective (again just talking
about the power interface, a few registers) because for the SPC it would
simply not be used
/drivers/mfd is probably not the right place for this code as it stands (but
probably will be when the entire driver, with DVFS and config interface, is
complete).
Could you please elaborate on how will the SPC driver extend into an MFD
driver?

Cheers,
Samuel.
--
Intel Open Source Technology Centre
http://oss.intel.com/
Lorenzo Pieralisi
2013-07-18 09:28:09 UTC
Permalink
Hi Samuel,
Post by Samuel Ortiz
Hi Lorenzo,
Post by Lorenzo Pieralisi
Hello,
version v5 of VExpress SPC driver, please read on the changelog for major
changes and explanations.
The probing scheme is unchanged, since after trying the early platform
devices approach it appeared that the end result was no better than the
current one. The only clean solution relies either on changing how
secondaries are brought up in the kernel (later than now) or enable
early platform device registration through DT. Please check this
https://lists.ozlabs.org/pipermail/devicetree-discuss/2013-June/036542.html
The interface was adapted to regmap and again reverted to old driver for
- Power down registers locking is hairy and requires arch spinlocks in
the MCPM back end to work properly, normal spinlocks cannot be used
- Regmap adds unnecessary code to manage SPC since it is just a bunch of
registers used to control power management flags, the overhead is just
not worth it (talking about power down registers, not the vexpress config
interface)
- The locking scheme behind regmap requires all registers in the map
to be protected with the same lock, which is not exactly what we want
here
- Given the reasons above, adding a regmap interface buys us nothing from
a driver readability and maintainability perspective (again just talking
about the power interface, a few registers) because for the SPC it would
simply not be used
/drivers/mfd is probably not the right place for this code as it stands (but
probably will be when the entire driver, with DVFS and config interface, is
complete).
Could you please elaborate on how will the SPC driver extend into an MFD
driver?
Reading through the thread I noticed Nico explained details properly, I
was about to mention a possible solution to the directory issue but I am
pretty sure that what he did will turn out for the best.

Usually, or better, historically, these pieces of code that program
PMICs lived in arch/arm/mach-* directories and that's something we could
have done as well (create a static mapping and write some functions to
peek and poke a few registers), but we thought that it was not the proper
way to go.

On top of that, the SPC is part of a component whose register space maps
disparate functions (config interface for voltage, clocks, energy probes,
frequency scaling and power states management) and basically that's the
reason we struggled to partition it properly (with further complexity
implied by the way requests - config and frequency scaling - have to be
serialized).

I hope the end result is reasonable, and overall I think it was a debate
that was worth having.

Thank you,
Lorenzo

Continue reading on narkive:
Loading...