[RFC PATCH v3 0/2] drivers: mfd: Versatile Express SPC support

[RFC PATCH v3 0/2] drivers: mfd: Versatile Express SPC support

[Lorenzo Pieralisi]

This patch is v3 of a previous posting:

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

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

This patch series introduces support for the Versatile Express Serial
Power Controller (SPC) present in ARM Versatile Express TC2 core tiles.
SPC driver is a fundamental component of TC2 power management and allows
to carry out C-state management and DVFS for A15 and A7 clusters.

First patch provides changes required by SPC to comply with the
Versatile Express config API, second patch is the SPC driver implementation.

Code extensively exercised through CPUidle and CPUfreq power states and
operating point transitions.

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

Pawel Moll (1):
  drivers: mfd: refactor the vexpress config bridge API

 Documentation/devicetree/bindings/mfd/vexpress-spc.txt |  35 +
 drivers/mfd/Kconfig                                    |   7 +
 drivers/mfd/Makefile                                   |   1 +
 drivers/mfd/vexpress-config.c                          |  61 +-
 drivers/mfd/vexpress-spc.c                             | 633 ++++++++++
 drivers/mfd/vexpress-sysreg.c                          |   2 +-
 include/linux/vexpress.h                               |  59 +-
 7 files changed, 770 insertions(+), 28 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mfd/vexpress-spc.txt
 create mode 100644 drivers/mfd/vexpress-spc.c

-- 
1.8.2.2

[RFC PATCH v3 1/2] drivers: mfd: refactor the vexpress config bridge API

[Lorenzo Pieralisi]

From: Pawel Moll <pawel.moll@arm.com>

The introduction of Serial Power Controller (SPC) requires the vexpress
config interface to change slightly since the SPC memory mapped interface
can be used as configuration bus but also for operating points
programming and retrieval. The helper that allocates the bridge functions
requires an additional parameter allowing to request component specific
functions that need not be initialized through device tree bindings but
just using simple look-up and statically defined constants.

This patch introduces the necessary changes to the vexpress config layer
to cater for the new vexpress bridge interface requirements.

Cc: Samuel Ortiz <sameo@linux.intel.com>
Cc: Achin Gupta <achin.gupta@arm.com>
Cc: Sudeep KarkadaNagesha <Sudeep.KarkadaNagesha@arm.com>
Cc: Pawel Moll <pawel.moll@arm.com>
Cc: Nicolas Pitre <nicolas.pitre@linaro.org>
Cc: Amit Kucheria <amit.kucheria@linaro.org>
Cc: Jon Medhurst <tixy@linaro.org>
Signed-off-by: Pawel Moll <pawel.moll@arm.com>
Acked-by: Nicolas Pitre <nico@linaro.org>
---
 drivers/mfd/vexpress-config.c | 61 ++++++----
 drivers/mfd/vexpress-sysreg.c |  2 +-
 include/linux/vexpress.h      | 16 ++-
 3 files changed, 51 insertions(+), 28 deletions(-)

diff --git a/drivers/mfd/vexpress-config.c b/drivers/mfd/vexpress-config.c
index 84ce6b9..1af2b0e 100644
--- a/drivers/mfd/vexpress-config.c
+++ b/drivers/mfd/vexpress-config.c
@@ -86,29 +86,13 @@ void vexpress_config_bridge_unregister(struct vexpress_config_bridge *bridge)
 }
 EXPORT_SYMBOL(vexpress_config_bridge_unregister);
 
-
-struct vexpress_config_func {
-	struct vexpress_config_bridge *bridge;
-	void *func;
-};
-
-struct vexpress_config_func *__vexpress_config_func_get(struct device *dev,
-		struct device_node *node)
+static struct vexpress_config_bridge *
+		vexpress_config_bridge_find(struct device_node *node)
 {
-	struct device_node *bridge_node;
-	struct vexpress_config_func *func;
 	int i;
+	struct vexpress_config_bridge *res = NULL;
+	struct device_node *bridge_node = of_node_get(node);
 
-	if (WARN_ON(dev && node && dev->of_node != node))
-		return NULL;
-	if (dev && !node)
-		node = dev->of_node;
-
-	func = kzalloc(sizeof(*func), GFP_KERNEL);
-	if (!func)
-		return NULL;
-
-	bridge_node = of_node_get(node);
 	while (bridge_node) {
 		const __be32 *prop = of_get_property(bridge_node,
 				"arm,vexpress,config-bridge", NULL);
@@ -129,13 +113,46 @@ struct vexpress_config_func *__vexpress_config_func_get(struct device *dev,
 
 		if (test_bit(i, vexpress_config_bridges_map) &&
 				bridge->node == bridge_node) {
-			func->bridge = bridge;
-			func->func = bridge->info->func_get(dev, node);
+			res = bridge;
 			break;
 		}
 	}
 	mutex_unlock(&vexpress_config_bridges_mutex);
 
+	return res;
+}
+
+
+struct vexpress_config_func {
+	struct vexpress_config_bridge *bridge;
+	void *func;
+};
+
+struct vexpress_config_func *__vexpress_config_func_get(
+		struct vexpress_config_bridge *bridge,
+		struct device *dev,
+		struct device_node *node,
+		const char *id)
+{
+	struct vexpress_config_func *func;
+
+	if (WARN_ON(dev && node && dev->of_node != node))
+		return NULL;
+	if (dev && !node)
+		node = dev->of_node;
+
+	if (!bridge)
+		bridge = vexpress_config_bridge_find(node);
+	if (!bridge)
+		return NULL;
+
+	func = kzalloc(sizeof(*func), GFP_KERNEL);
+	if (!func)
+		return NULL;
+
+	func->bridge = bridge;
+	func->func = bridge->info->func_get(dev, node, id);
+
 	if (!func->func) {
 		of_node_put(node);
 		kfree(func);
diff --git a/drivers/mfd/vexpress-sysreg.c b/drivers/mfd/vexpress-sysreg.c
index 96a020b..d2599aa 100644
--- a/drivers/mfd/vexpress-sysreg.c
+++ b/drivers/mfd/vexpress-sysreg.c
@@ -165,7 +165,7 @@ static u32 *vexpress_sysreg_config_data;
 static int vexpress_sysreg_config_tries;
 
 static void *vexpress_sysreg_config_func_get(struct device *dev,
-		struct device_node *node)
+		struct device_node *node, const char *id)
 {
 	struct vexpress_sysreg_config_func *config_func;
 	u32 site;
diff --git a/include/linux/vexpress.h b/include/linux/vexpress.h
index 6e7980d..50368e0 100644
--- a/include/linux/vexpress.h
+++ b/include/linux/vexpress.h
@@ -68,7 +68,8 @@
  */
 struct vexpress_config_bridge_info {
 	const char *name;
-	void *(*func_get)(struct device *dev, struct device_node *node);
+	void *(*func_get)(struct device *dev, struct device_node *node,
+			  const char *id);
 	void (*func_put)(void *func);
 	int (*func_exec)(void *func, int offset, bool write, u32 *data);
 };
@@ -87,12 +88,17 @@ void vexpress_config_complete(struct vexpress_config_bridge *bridge,
 
 struct vexpress_config_func;
 
-struct vexpress_config_func *__vexpress_config_func_get(struct device *dev,
-		struct device_node *node);
+struct vexpress_config_func *__vexpress_config_func_get(
+		struct vexpress_config_bridge *bridge,
+		struct device *dev,
+		struct device_node *node,
+		const char *id);
+#define vexpress_config_func_get(bridge, id) \
+		__vexpress_config_func_get(bridge, NULL, NULL, id)
 #define vexpress_config_func_get_by_dev(dev) \
-		__vexpress_config_func_get(dev, NULL)
+		__vexpress_config_func_get(NULL, dev, NULL, NULL)
 #define vexpress_config_func_get_by_node(node) \
-		__vexpress_config_func_get(NULL, node)
+		__vexpress_config_func_get(NULL, NULL, node, NULL)
 void vexpress_config_func_put(struct vexpress_config_func *func);
 
 /* Both may sleep! */
-- 
1.8.2.2

[RFC PATCH v3 2/2] drivers: mfd: vexpress: add Serial Power Controller (SPC) support

[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, operating points and reset control.

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

The driver also provides a bridge interface through the vexpress config
infrastructure. Operations allowing to read/write operating points are
made to go via the same interface as configuration transactions so that
all requests to M3 are serialized.

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

Cc: Samuel Ortiz <sameo-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Cc: Pawel Moll <pawel.moll-5wv7dgnIgG8@public.gmane.org>
Cc: Nicolas Pitre <nicolas.pitre-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: Amit Kucheria <amit.kucheria-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: Jon Medhurst <tixy-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Signed-off-by: Achin Gupta <achin.gupta-5wv7dgnIgG8@public.gmane.org>
Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi-5wv7dgnIgG8@public.gmane.org>
Signed-off-by: Sudeep KarkadaNagesha <Sudeep.KarkadaNagesha-5wv7dgnIgG8@public.gmane.org>
Reviewed-by: Nicolas Pitre <nico-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 Documentation/devicetree/bindings/mfd/vexpress-spc.txt |  35 +
 drivers/mfd/Kconfig                                    |   7 +
 drivers/mfd/Makefile                                   |   1 +
 drivers/mfd/vexpress-spc.c                             | 633 ++++++++++
 include/linux/vexpress.h                               |  43 +
 5 files changed, 719 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..1d71dc2
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/vexpress-spc.txt
@@ -0,0 +1,35 @@
+* 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/voltage and
+operating point transitions, through memory mapped registers interface.
+
+On testchips like TC2 it also provides a configuration interface that can
+be used to read/write values which cannot be read/written through simple
+memory mapped reads/writes.
+
+- 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: spc@7fff0000 {
+	compatible = "arm,vexpress-spc,v2p-ca15_a7","arm,vexpress-spc";
+	reg = <0 0x7FFF0000 0 0x1000>;
+	interrupts = <0 95 4>;
+};
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index d54e985..391eda1 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1148,3 +1148,10 @@ 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
+	depends on VEXPRESS_CONFIG
+	help
+	  Serial Power Controller driver for ARM Ltd. test chips.
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..0c6718a
--- /dev/null
+++ b/drivers/mfd/vexpress-spc.c
@@ -0,0 +1,633 @@
+/*
+ * Versatile Express Serial Power Controller (SPC) support
+ *
+ * Copyright (C) 2013 ARM Ltd.
+ *
+ * Authors: Sudeep KarkadaNagesha <sudeep.karkadanagesha-5wv7dgnIgG8@public.gmane.org>
+ *          Achin Gupta           <achin.gupta-5wv7dgnIgG8@public.gmane.org>
+ *          Lorenzo Pieralisi     <lorenzo.pieralisi-5wv7dgnIgG8@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/interrupt.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/slab.h>
+#include <linux/vexpress.h>
+
+#include <asm/cacheflush.h>
+
+#define SCC_CFGREG19		0x120
+#define SCC_CFGREG20		0x124
+#define A15_CONF		0x400
+#define A7_CONF			0x500
+#define SYS_INFO		0x700
+#define PERF_LVL_A15		0xB00
+#define PERF_REQ_A15		0xB04
+#define PERF_LVL_A7		0xB08
+#define PERF_REQ_A7		0xB0c
+#define SYS_CFGCTRL		0xB10
+#define SYS_CFGCTRL_REQ		0xB14
+#define PWC_STATUS		0xB18
+#define PWC_FLAG		0xB1c
+#define WAKE_INT_MASK		0xB24
+#define WAKE_INT_RAW		0xB28
+#define WAKE_INT_STAT		0xB2c
+#define A15_PWRDN_EN		0xB30
+#define A7_PWRDN_EN		0xB34
+#define A7_PWRDNACK		0xB54
+#define A15_BX_ADDR0		0xB68
+#define SYS_CFG_WDATA		0xB70
+#define SYS_CFG_RDATA		0xB74
+#define A7_BX_ADDR0		0xB78
+
+#define GBL_WAKEUP_INT_MSK	(0x3 << 10)
+
+#define CLKF_SHIFT		16
+#define CLKF_MASK		0x1FFF
+#define CLKR_SHIFT		0
+#define CLKR_MASK		0x3F
+#define CLKOD_SHIFT		8
+#define CLKOD_MASK		0xF
+
+#define OPP_FUNCTION		6
+#define OPP_BASE_DEVICE		0x300
+#define OPP_A15_OFFSET		0x4
+#define OPP_A7_OFFSET		0xc
+
+#define SYS_CFGCTRL_START	(1 << 31)
+#define SYS_CFGCTRL_WRITE	(1 << 30)
+#define SYS_CFGCTRL_FUNC(n)	(((n) & 0x3f) << 20)
+#define SYS_CFGCTRL_DEVICE(n)	(((n) & 0xfff) << 0)
+
+#define MAX_OPPS	8
+#define MAX_CLUSTERS	2
+
+enum {
+	A15_OPP_TYPE		= 0,
+	A7_OPP_TYPE		= 1,
+	SYS_CFGCTRL_TYPE	= 2,
+	INVALID_TYPE
+};
+
+#define STAT_COMPLETE(type)	((1 << 0) << (type << 2))
+#define STAT_ERR(type)		((1 << 1) << (type << 2))
+#define RESPONSE_MASK(type)	(STAT_COMPLETE(type) | STAT_ERR(type))
+
+struct vexpress_spc_drvdata {
+	void __iomem *baseaddr;
+	u32 a15_clusid;
+	int irq;
+	u32 cur_req_type;
+	u32 freqs[MAX_CLUSTERS][MAX_OPPS];
+	int freqs_cnt[MAX_CLUSTERS];
+};
+
+enum spc_func_type {
+	CONFIG_FUNC = 0,
+	PERF_FUNC   = 1,
+};
+
+struct vexpress_spc_func {
+	enum spc_func_type type;
+	u32 function;
+	u32 device;
+};
+
+static struct vexpress_spc_drvdata *info;
+static u32 *vexpress_spc_config_data;
+static struct vexpress_config_bridge *vexpress_spc_config_bridge;
+static struct vexpress_config_func *opp_func, *perf_func;
+
+static int vexpress_spc_load_result = -EAGAIN;
+
+static bool vexpress_spc_initialized(void)
+{
+	return vexpress_spc_load_result == 0;
+}
+
+/**
+ * vexpress_spc_write_resume_reg() - 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 vexpress_spc_write_resume_reg(u32 cluster, u32 cpu, u32 addr)
+{
+	void __iomem *baseaddr;
+
+	if (WARN_ON_ONCE(cluster >= MAX_CLUSTERS))
+		return;
+
+	if (cluster != info->a15_clusid)
+		baseaddr = info->baseaddr + A7_BX_ADDR0 + (cpu << 2);
+	else
+		baseaddr = info->baseaddr + A15_BX_ADDR0 + (cpu << 2);
+
+	writel_relaxed(addr, baseaddr);
+}
+
+/**
+ * vexpress_spc_get_nb_cpus() - get number of cpus in a cluster
+ *
+ * @cluster: mpidr[15:8] bitfield describing cluster affinity level
+ *
+ * Return: number of cpus in the cluster
+ *         -EINVAL if cluster number invalid
+ */
+int vexpress_spc_get_nb_cpus(u32 cluster)
+{
+	u32 val;
+
+	if (WARN_ON_ONCE(cluster >= MAX_CLUSTERS))
+		return -EINVAL;
+
+	val = readl_relaxed(info->baseaddr + SYS_INFO);
+	val = (cluster != info->a15_clusid) ? (val >> 20) : (val >> 16);
+	return val & 0xf;
+}
+EXPORT_SYMBOL_GPL(vexpress_spc_get_nb_cpus);
+
+/**
+ * vexpress_spc_get_performance - get current performance level of cluster
+ * @cluster: mpidr[15:8] bitfield describing cluster affinity level
+ * @freq: pointer to the performance level to be assigned
+ *
+ * Return: 0 on success
+ *         < 0 on read error
+ */
+int vexpress_spc_get_performance(u32 cluster, u32 *freq)
+{
+	u32 perf_cfg_reg;
+	int perf, ret;
+
+	if (!vexpress_spc_initialized() || (cluster >= MAX_CLUSTERS))
+		return -EINVAL;
+
+	perf_cfg_reg = cluster != info->a15_clusid ? PERF_LVL_A7 : PERF_LVL_A15;
+	ret = vexpress_config_read(perf_func, perf_cfg_reg, &perf);
+
+	if (!ret)
+		*freq = info->freqs[cluster][perf];
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(vexpress_spc_get_performance);
+
+/**
+ * vexpress_spc_get_perf_index - get performance level corresponding to
+ *				 a frequency
+ * @cluster: mpidr[15:8] bitfield describing cluster affinity level
+ * @freq: frequency to be looked-up
+ *
+ * Return: perf level index on success
+ *         -EINVAL on error
+ */
+static int vexpress_spc_find_perf_index(u32 cluster, u32 freq)
+{
+	int idx;
+
+	for (idx = 0; idx < info->freqs_cnt[cluster]; idx++)
+		if (info->freqs[cluster][idx] == freq)
+			break;
+	return (idx == info->freqs_cnt[cluster]) ? -EINVAL : idx;
+}
+
+/**
+ * vexpress_spc_set_performance - set current performance level of cluster
+ *
+ * @cluster: mpidr[15:8] bitfield describing cluster affinity level
+ * @freq: performance level to be programmed
+ *
+ * Returns: 0 on success
+ *          < 0 on write error
+ */
+int vexpress_spc_set_performance(u32 cluster, u32 freq)
+{
+	int ret, perf, offset;
+
+	if (!vexpress_spc_initialized() || (cluster >= MAX_CLUSTERS))
+		return -EINVAL;
+
+	offset = (cluster != info->a15_clusid) ? PERF_LVL_A7 : PERF_LVL_A15;
+
+	perf = vexpress_spc_find_perf_index(cluster, freq);
+
+	if (perf < 0 || perf >= MAX_OPPS)
+		return -EINVAL;
+
+	ret = vexpress_config_write(perf_func, offset, perf);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(vexpress_spc_set_performance);
+
+static void vexpress_spc_set_wake_intr(u32 mask)
+{
+	writel_relaxed(mask & VEXPRESS_SPC_WAKE_INTR_MASK,
+		       info->baseaddr + WAKE_INT_MASK);
+}
+
+static inline void reg_bitmask(u32 *reg, u32 mask, bool set)
+{
+	if (set)
+		*reg |= mask;
+	else
+		*reg &= ~mask;
+}
+
+/**
+ * vexpress_spc_set_global_wakeup_intr()
+ *
+ * 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 vexpress_spc_set_global_wakeup_intr(bool set)
+{
+	u32 wake_int_mask_reg = 0;
+
+	wake_int_mask_reg = readl_relaxed(info->baseaddr + WAKE_INT_MASK);
+	reg_bitmask(&wake_int_mask_reg, GBL_WAKEUP_INT_MSK, set);
+	vexpress_spc_set_wake_intr(wake_int_mask_reg);
+}
+
+/**
+ * vexpress_spc_set_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.
+ *
+ * @cpu: mpidr[7:0] bitfield describing cpu affinity level
+ * @cluster: mpidr[15:8] bitfield describing cluster affinity level
+ * @set: if true, wake-up IRQs are set, if false they are cleared
+ */
+void vexpress_spc_set_cpu_wakeup_irq(u32 cpu, u32 cluster, bool set)
+{
+	u32 mask = 0;
+	u32 wake_int_mask_reg = 0;
+
+	mask = 1 << cpu;
+	if (info->a15_clusid != cluster)
+		mask <<= 4;
+
+	wake_int_mask_reg = readl_relaxed(info->baseaddr + WAKE_INT_MASK);
+	reg_bitmask(&wake_int_mask_reg, mask, set);
+	vexpress_spc_set_wake_intr(wake_int_mask_reg);
+}
+
+/**
+ * vexpress_spc_powerdown_enable()
+ *
+ * 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 vexpress_spc_powerdown_enable(u32 cluster, bool enable)
+{
+	u32 pwdrn_reg = 0;
+
+	if (cluster >= MAX_CLUSTERS)
+		return;
+	pwdrn_reg = cluster != info->a15_clusid ? A7_PWRDN_EN : A15_PWRDN_EN;
+	writel_relaxed(enable, info->baseaddr + pwdrn_reg);
+}
+
+irqreturn_t vexpress_spc_irq_handler(int irq, void *data)
+{
+	int ret;
+	u32 status = readl_relaxed(info->baseaddr + PWC_STATUS);
+
+	if (!(status & RESPONSE_MASK(info->cur_req_type)))
+		return IRQ_NONE;
+
+	if ((status == STAT_COMPLETE(SYS_CFGCTRL_TYPE))
+			&& vexpress_spc_config_data) {
+		*vexpress_spc_config_data =
+				readl_relaxed(info->baseaddr + SYS_CFG_RDATA);
+		vexpress_spc_config_data = NULL;
+	}
+
+	ret = STAT_COMPLETE(info->cur_req_type) ? 0 : -EIO;
+	info->cur_req_type = INVALID_TYPE;
+	vexpress_config_complete(vexpress_spc_config_bridge, ret);
+	return IRQ_HANDLED;
+}
+
+/**
+ * Based on the firmware documentation, this is always fixed to 20
+ * All the 4 OSC: A15 PLL0/1, A7 PLL0/1 must be programmed same
+ * values for both control and value registers.
+ * This function uses A15 PLL 0 registers to compute multiple factor
+ * F out = F in * (CLKF + 1) / ((CLKOD + 1) * (CLKR + 1))
+ */
+static inline int __get_mult_factor(void)
+{
+	int i_div, o_div, f_div;
+	u32 tmp;
+
+	tmp = readl(info->baseaddr + SCC_CFGREG19);
+	f_div = (tmp >> CLKF_SHIFT) & CLKF_MASK;
+
+	tmp = readl(info->baseaddr + SCC_CFGREG20);
+	o_div = (tmp >> CLKOD_SHIFT) & CLKOD_MASK;
+	i_div = (tmp >> CLKR_SHIFT) & CLKR_MASK;
+
+	return (f_div + 1) / ((o_div + 1) * (i_div + 1));
+}
+
+/**
+ * vexpress_spc_populate_opps() - initialize opp tables from microcontroller
+ *
+ * @cluster: mpidr[15:8] bitfield describing cluster affinity level
+ *
+ * Return: 0 on success
+ *         < 0 on error
+ */
+static int vexpress_spc_populate_opps(u32 cluster)
+{
+	u32 data = 0, ret, i, offset;
+	int mult_fact = __get_mult_factor();
+
+	if (WARN_ON_ONCE(cluster >= MAX_CLUSTERS))
+		return -EINVAL;
+
+	offset = cluster != info->a15_clusid ? OPP_A7_OFFSET : OPP_A15_OFFSET;
+	for (i = 0; i < MAX_OPPS; i++) {
+		ret = vexpress_config_read(opp_func, i + offset, &data);
+		if (!ret)
+			info->freqs[cluster][i] = (data & 0xFFFFF) * mult_fact;
+		else
+			break;
+	}
+
+	info->freqs_cnt[cluster] = i;
+	return ret;
+}
+
+/**
+ * vexpress_spc_get_freq_table() - Retrieve a pointer to the frequency
+ *				   table for a given cluster
+ *
+ * @cluster: mpidr[15:8] bitfield describing cluster affinity level
+ * @fptr: pointer to be initialized
+ * Return: operating points count on success
+ *         -EINVAL on pointer error
+ */
+int vexpress_spc_get_freq_table(u32 cluster, u32 **fptr)
+{
+	if (WARN_ON_ONCE(!fptr || cluster >= MAX_CLUSTERS))
+		return -EINVAL;
+	*fptr = info->freqs[cluster];
+	return info->freqs_cnt[cluster];
+}
+EXPORT_SYMBOL_GPL(vexpress_spc_get_freq_table);
+
+static void *vexpress_spc_func_get(struct device *dev,
+		struct device_node *node, const char *id)
+{
+	struct vexpress_spc_func *spc_func;
+	u32 func_device[2];
+	int err = 0;
+
+	spc_func = kzalloc(sizeof(*spc_func), GFP_KERNEL);
+	if (!spc_func)
+		return NULL;
+
+	if (strcmp(id, "opp") == 0) {
+		spc_func->type = CONFIG_FUNC;
+		spc_func->function = OPP_FUNCTION;
+		spc_func->device = OPP_BASE_DEVICE;
+	} else if (strcmp(id, "perf") == 0) {
+		spc_func->type = PERF_FUNC;
+	} else if (node) {
+		of_node_get(node);
+		err = of_property_read_u32_array(node,
+				"arm,vexpress-sysreg,func", func_device,
+				ARRAY_SIZE(func_device));
+		of_node_put(node);
+		spc_func->type = CONFIG_FUNC;
+		spc_func->function = func_device[0];
+		spc_func->device = func_device[1];
+	}
+
+	if (WARN_ON(err)) {
+		kfree(spc_func);
+		return NULL;
+	}
+
+	pr_debug("func 0x%p = 0x%x, %d %d\n", spc_func,
+					     spc_func->function,
+					     spc_func->device,
+					     spc_func->type);
+
+	return spc_func;
+}
+
+static void vexpress_spc_func_put(void *func)
+{
+	kfree(func);
+}
+
+static int vexpress_spc_func_exec(void *func, int offset, bool write,
+				  u32 *data)
+{
+	struct vexpress_spc_func *spc_func = func;
+	u32 command;
+
+	if (!data)
+		return -EINVAL;
+	/*
+	 * Setting and retrieval of operating points is not part of
+	 * DCC config interface. It was made to go through the same
+	 * code path so that requests to the M3 can be serialized
+	 * properly with config reads/writes through the common
+	 * vexpress config interface
+	 */
+	switch (spc_func->type) {
+	case PERF_FUNC:
+		if (write) {
+			info->cur_req_type = (offset == PERF_LVL_A15) ?
+					A15_OPP_TYPE : A7_OPP_TYPE;
+			writel_relaxed(*data, info->baseaddr + offset);
+			return VEXPRESS_CONFIG_STATUS_WAIT;
+		} else {
+			*data = readl_relaxed(info->baseaddr + offset);
+			return VEXPRESS_CONFIG_STATUS_DONE;
+		}
+	case CONFIG_FUNC:
+		info->cur_req_type = SYS_CFGCTRL_TYPE;
+
+		command = SYS_CFGCTRL_START;
+		command |= write ? SYS_CFGCTRL_WRITE : 0;
+		command |= SYS_CFGCTRL_FUNC(spc_func->function);
+		command |= SYS_CFGCTRL_DEVICE(spc_func->device + offset);
+
+		pr_debug("command %x\n", command);
+
+		if (!write)
+			vexpress_spc_config_data = data;
+		else
+			writel_relaxed(*data, info->baseaddr + SYS_CFG_WDATA);
+		writel_relaxed(command, info->baseaddr + SYS_CFGCTRL);
+
+		return VEXPRESS_CONFIG_STATUS_WAIT;
+	default:
+		return -EINVAL;
+	}
+}
+
+struct vexpress_config_bridge_info vexpress_spc_config_bridge_info = {
+	.name = "vexpress-spc",
+	.func_get = vexpress_spc_func_get,
+	.func_put = vexpress_spc_func_put,
+	.func_exec = vexpress_spc_func_exec,
+};
+
+static const struct of_device_id vexpress_spc_ids[] __initconst = {
+	{ .compatible = "arm,vexpress-spc,v2p-ca15_a7" },
+	{ .compatible = "arm,vexpress-spc" },
+	{},
+};
+
+static int __init vexpress_spc_init(void)
+{
+	int ret;
+	struct device_node *node = of_find_matching_node(NULL,
+							 vexpress_spc_ids);
+
+	if (!node)
+		return -ENODEV;
+
+	info = kzalloc(sizeof(*info), GFP_KERNEL);
+	if (!info) {
+		pr_err("%s: unable to allocate mem\n", __func__);
+		return -ENOMEM;
+	}
+	info->cur_req_type = INVALID_TYPE;
+
+	info->baseaddr = of_iomap(node, 0);
+	if (WARN_ON(!info->baseaddr)) {
+		ret = -ENXIO;
+		goto mem_free;
+	}
+
+	info->irq = irq_of_parse_and_map(node, 0);
+
+	if (WARN_ON(!info->irq)) {
+		ret = -ENXIO;
+		goto unmap;
+	}
+
+	readl_relaxed(info->baseaddr + PWC_STATUS);
+
+	ret = request_irq(info->irq, vexpress_spc_irq_handler,
+		IRQF_DISABLED | IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
+		"arm-spc", info);
+
+	if (ret) {
+		pr_err("IRQ %d request failed\n", info->irq);
+		ret = -ENODEV;
+		goto unmap;
+	}
+
+	info->a15_clusid = readl_relaxed(info->baseaddr + A15_CONF) & 0xf;
+
+	vexpress_spc_config_bridge = vexpress_config_bridge_register(
+			node, &vexpress_spc_config_bridge_info);
+
+	if (WARN_ON(!vexpress_spc_config_bridge)) {
+		ret = -ENODEV;
+		goto unmap;
+	}
+
+	opp_func = vexpress_config_func_get(vexpress_spc_config_bridge, "opp");
+	perf_func =
+		vexpress_config_func_get(vexpress_spc_config_bridge, "perf");
+
+	if (!opp_func || !perf_func) {
+		ret = -ENODEV;
+		goto unmap;
+	}
+
+	if (vexpress_spc_populate_opps(0) || vexpress_spc_populate_opps(1)) {
+		if (info->irq)
+			free_irq(info->irq, info);
+		pr_err("failed to build OPP table\n");
+		ret = -ENODEV;
+		goto unmap;
+	}
+	/*
+	 * Multi-cluster systems may need this data when non-coherent, during
+	 * cluster power-up/power-down. Make sure it reaches main memory:
+	 */
+	sync_cache_w(info);
+	sync_cache_w(&info);
+	pr_info("vexpress-spc loaded at %p\n", info->baseaddr);
+	return 0;
+
+unmap:
+	iounmap(info->baseaddr);
+
+mem_free:
+	kfree(info);
+	return ret;
+}
+
+static bool __init __vexpress_spc_check_loaded(void);
+/*
+ * Pointer spc_check_loaded is swapped after init hence it is safe
+ * to initialize it to a function in the __init section
+ */
+static bool (*spc_check_loaded)(void) __refdata = &__vexpress_spc_check_loaded;
+
+static bool __init __vexpress_spc_check_loaded(void)
+{
+	if (vexpress_spc_load_result == -EAGAIN)
+		vexpress_spc_load_result = vexpress_spc_init();
+	spc_check_loaded = &vexpress_spc_initialized;
+	return vexpress_spc_initialized();
+}
+
+/*
+ * Function exported to manage early_initcall ordering.
+ * SPC code is needed very early in the boot process
+ * to bring CPUs out of reset and initialize power
+ * management back-end. After boot swap pointers to
+ * make the functionality check available to loadable
+ * modules, when early boot init functions have been
+ * already freed from kernel address space.
+ */
+bool vexpress_spc_check_loaded(void)
+{
+	return spc_check_loaded();
+}
+EXPORT_SYMBOL_GPL(vexpress_spc_check_loaded);
+
+static int __init vexpress_spc_early_init(void)
+{
+	__vexpress_spc_check_loaded();
+	return vexpress_spc_load_result;
+}
+early_initcall(vexpress_spc_early_init);
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Serial Power Controller (SPC) support");
diff --git a/include/linux/vexpress.h b/include/linux/vexpress.h
index 50368e0..c1191ab 100644
--- a/include/linux/vexpress.h
+++ b/include/linux/vexpress.h
@@ -132,4 +132,47 @@ void vexpress_clk_of_register_spc(void);
 void vexpress_clk_init(void __iomem *sp810_base);
 void vexpress_clk_of_init(void);
 
+/* SPC */
+
+#define	VEXPRESS_SPC_WAKE_INTR_IRQ(cluster, cpu) \
+			(1 << (4 * (cluster) + (cpu)))
+#define	VEXPRESS_SPC_WAKE_INTR_FIQ(cluster, cpu) \
+			(1 << (7 * (cluster) + (cpu)))
+#define	VEXPRESS_SPC_WAKE_INTR_SWDOG		(1 << 10)
+#define	VEXPRESS_SPC_WAKE_INTR_GTIMER		(1 << 11)
+#define	VEXPRESS_SPC_WAKE_INTR_MASK		0xFFF
+
+#ifdef CONFIG_VEXPRESS_SPC
+extern bool vexpress_spc_check_loaded(void);
+extern void vexpress_spc_set_cpu_wakeup_irq(u32 cpu, u32 cluster, bool set);
+extern void vexpress_spc_set_global_wakeup_intr(bool set);
+extern int vexpress_spc_get_freq_table(u32 cluster, u32 **fptr);
+extern int vexpress_spc_get_performance(u32 cluster, u32 *freq);
+extern int vexpress_spc_set_performance(u32 cluster, u32 freq);
+extern void vexpress_spc_write_resume_reg(u32 cluster, u32 cpu, u32 addr);
+extern int vexpress_spc_get_nb_cpus(u32 cluster);
+extern void vexpress_spc_powerdown_enable(u32 cluster, bool enable);
+#else
+static inline bool vexpress_spc_check_loaded(void) { return false; }
+static inline void vexpress_spc_set_cpu_wakeup_irq(u32 cpu, u32 cluster,
+						   bool set) { }
+static inline void vexpress_spc_set_global_wakeup_intr(bool set) { }
+static inline int vexpress_spc_get_freq_table(u32 cluster, u32 **fptr)
+{
+	return -ENODEV;
+}
+static inline int vexpress_spc_get_performance(u32 cluster, u32 *freq)
+{
+	return -ENODEV;
+}
+static inline int vexpress_spc_set_performance(u32 cluster, u32 freq)
+{
+	return -ENODEV;
+}
+static inline void vexpress_spc_write_resume_reg(u32 cluster,
+						 u32 cpu, u32 addr) { }
+static inline int vexpress_spc_get_nb_cpus(u32 cluster) { return -ENODEV; }
+static inline void vexpress_spc_powerdown_enable(u32 cluster, bool enable) { }
+#endif
+
 #endif
-- 
1.8.2.2

Re: [RFC PATCH v3 2/2] drivers: mfd: vexpress: add Serial Power Controller (SPC) support

[Samuel Ortiz]

Hi Lorenzo,

I don't particularily like this code, but I guess most of my dislike
comes from the whole bridge interface API and how that forces you into
implementing pretty much static code.
A few nitpicks:

On Thu, Jun 06, 2013 at 10:59:23AM +0100, Lorenzo Pieralisi wrote:
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index d54e985..391eda1 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1148,3 +1148,10 @@ 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
> +	depends on VEXPRESS_CONFIG
> +	help
Please provide a detailed help entry here. 


> +	  Serial Power Controller driver for ARM Ltd. test chips.
> 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
So you have Versatile Express platforms that will not need SPC ? i.e.
why isn't all that stuff under a generic CONFIG_VEXPRESS symbol ?



> +static struct vexpress_spc_drvdata *info;
> +static u32 *vexpress_spc_config_data;
> +static struct vexpress_config_bridge *vexpress_spc_config_bridge;
> +static struct vexpress_config_func *opp_func, *perf_func;
> +
> +static int vexpress_spc_load_result = -EAGAIN;
As I said, quite static...

> +irqreturn_t vexpress_spc_irq_handler(int irq, void *data)
missing a static here ?


> +static bool __init __vexpress_spc_check_loaded(void);
> +/*
> + * Pointer spc_check_loaded is swapped after init hence it is safe
> + * to initialize it to a function in the __init section
> + */
> +static bool (*spc_check_loaded)(void) __refdata = &__vexpress_spc_check_loaded;
> +
> +static bool __init __vexpress_spc_check_loaded(void)
> +{
> +	if (vexpress_spc_load_result == -EAGAIN)
> +		vexpress_spc_load_result = vexpress_spc_init();
> +	spc_check_loaded = &vexpress_spc_initialized;
> +	return vexpress_spc_initialized();
> +}
> +
> +/*
> + * Function exported to manage early_initcall ordering.
> + * SPC code is needed very early in the boot process
> + * to bring CPUs out of reset and initialize power
> + * management back-end. After boot swap pointers to
> + * make the functionality check available to loadable
> + * modules, when early boot init functions have been
> + * already freed from kernel address space.
> + */
> +bool vexpress_spc_check_loaded(void)
> +{
> +	return spc_check_loaded();
> +}
> +EXPORT_SYMBOL_GPL(vexpress_spc_check_loaded);
That one and the previous function look really nasty to me.
The simple fact that you need a static variable in your code to check if
your module is loaded sounds really fishy.

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

Re: [RFC PATCH v3 2/2] drivers: mfd: vexpress: add Serial Power Controller (SPC) support

[Nicolas Pitre]

On Thu, 13 Jun 2013, Samuel Ortiz wrote:

> > +static struct vexpress_spc_drvdata *info;
> > +static u32 *vexpress_spc_config_data;
> > +static struct vexpress_config_bridge *vexpress_spc_config_bridge;
> > +static struct vexpress_config_func *opp_func, *perf_func;
> > +
> > +static int vexpress_spc_load_result = -EAGAIN;
> As I said, quite static...
> 
> > +static bool __init __vexpress_spc_check_loaded(void);
> > +/*
> > + * Pointer spc_check_loaded is swapped after init hence it is safe
> > + * to initialize it to a function in the __init section
> > + */
> > +static bool (*spc_check_loaded)(void) __refdata = &__vexpress_spc_check_loaded;
> > +
> > +static bool __init __vexpress_spc_check_loaded(void)
> > +{
> > +	if (vexpress_spc_load_result == -EAGAIN)
> > +		vexpress_spc_load_result = vexpress_spc_init();
> > +	spc_check_loaded = &vexpress_spc_initialized;
> > +	return vexpress_spc_initialized();
> > +}
> > +
> > +/*
> > + * Function exported to manage early_initcall ordering.
> > + * SPC code is needed very early in the boot process
> > + * to bring CPUs out of reset and initialize power
> > + * management back-end. After boot swap pointers to
> > + * make the functionality check available to loadable
> > + * modules, when early boot init functions have been
> > + * already freed from kernel address space.
> > + */
> > +bool vexpress_spc_check_loaded(void)
> > +{
> > +	return spc_check_loaded();
> > +}
> > +EXPORT_SYMBOL_GPL(vexpress_spc_check_loaded);
> That one and the previous function look really nasty to me.
> The simple fact that you need a static variable in your code to check if
> your module is loaded sounds really fishy.

Maybe a bit of context might explain why things are done this way.

This code is, amongst other things, needed to properly bring up 
secondary CPUs during boot.  This means that it has to be initialized at 
the early_initcall level before SMP is initialized.  So that rules out 
most of the device driver niceties which are not initialized yet, and 
that also means that this can't be made into a loadable module.

To make things even more subtle, this code is needed to implement the 
backend for the MCPM layer through which the actual bringup of secondary 
CPUs is done.  So that MCPM backend is itself also initialized at the 
early_initcall level, but we don't know and can't enforce the order 
those different things will be initialized.  So the MCPM backend calls 
vexpress_spc_check_loaded() to initialize it if it has not been 
initialized yet, or return false if there is no SPC on the booted 
system.

Yet this code is also the entry point for some operating point changes 
requested by the cpufreq driver, and that one can be modular.  And the 
cpufreq driver also has to test for the presence of a SPC via 
vexpress_spc_check_loaded().

So that's the main reason why there is a static state variable, and why 
there is a switch of function pointed by spc_check_loaded to let the 
init code go after boot and still be able to be referenced by modules 
after boot.


Nicolas

Re: [RFC PATCH v3 2/2] drivers: mfd: vexpress: add Serial Power Controller (SPC) support

[Lorenzo Pieralisi]

Hi Samuel,

first things first, thanks a lot for having a look.

On Thu, Jun 13, 2013 at 01:01:43AM +0100, Samuel Ortiz wrote:
> Hi Lorenzo,
> 
> I don't particularily like this code, but I guess most of my dislike
> comes from the whole bridge interface API and how that forces you into
> implementing pretty much static code.

I do not particularly like it either; you have to grant us though, as Nico
explained, that the usage of this piece of hardware very early at boot is
forcing us to find a solution that is not necessarily easy to implement.

> A few nitpicks:
> 
> On Thu, Jun 06, 2013 at 10:59:23AM +0100, Lorenzo Pieralisi wrote:
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > index d54e985..391eda1 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -1148,3 +1148,10 @@ 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
> > +	depends on VEXPRESS_CONFIG
> > +	help
> Please provide a detailed help entry here. 

Ok.

> > +	  Serial Power Controller driver for ARM Ltd. test chips.
> > 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
> So you have Versatile Express platforms that will not need SPC ? i.e.
> why isn't all that stuff under a generic CONFIG_VEXPRESS symbol ?

You answered your own question, the Serial Power Controller aka SPC is
present only in one of the many coretiles that can be stacked on top
of the versatile express motherboard, so it requires a specific entry
unless we want to compile it in for all vexpress platforms.

> > +static struct vexpress_spc_drvdata *info;
> > +static u32 *vexpress_spc_config_data;
> > +static struct vexpress_config_bridge *vexpress_spc_config_bridge;
> > +static struct vexpress_config_func *opp_func, *perf_func;
> > +
> > +static int vexpress_spc_load_result = -EAGAIN;
> As I said, quite static...

I will have a look and see if I can improve it, I could include some of
those variables in the driver data and alloc them dynamically.

> > +irqreturn_t vexpress_spc_irq_handler(int irq, void *data)
> missing a static here ?

Were not there enough :-) ? Correct, I will fix it.

> > +static bool __init __vexpress_spc_check_loaded(void);
> > +/*
> > + * Pointer spc_check_loaded is swapped after init hence it is safe
> > + * to initialize it to a function in the __init section
> > + */
> > +static bool (*spc_check_loaded)(void) __refdata = &__vexpress_spc_check_loaded;
> > +
> > +static bool __init __vexpress_spc_check_loaded(void)
> > +{
> > +	if (vexpress_spc_load_result == -EAGAIN)
> > +		vexpress_spc_load_result = vexpress_spc_init();
> > +	spc_check_loaded = &vexpress_spc_initialized;
> > +	return vexpress_spc_initialized();
> > +}
> > +
> > +/*
> > + * Function exported to manage early_initcall ordering.
> > + * SPC code is needed very early in the boot process
> > + * to bring CPUs out of reset and initialize power
> > + * management back-end. After boot swap pointers to
> > + * make the functionality check available to loadable
> > + * modules, when early boot init functions have been
> > + * already freed from kernel address space.
> > + */
> > +bool vexpress_spc_check_loaded(void)
> > +{
> > +	return spc_check_loaded();
> > +}
> > +EXPORT_SYMBOL_GPL(vexpress_spc_check_loaded);
> That one and the previous function look really nasty to me.
> The simple fact that you need a static variable in your code to check if
> your module is loaded sounds really fishy.

Nico explained the reasons behind this nasty hack, because that's what it
is. The only solution is resorting to vexpress platform code to initialize
this driver directly (providing a static virtual memory mapping since that
has to happen very early) to remove all needs for early_initcall
synchronization and remove that variable. It won't look nicer though.

I will review the code again to see how I can improve it.

Thanks a lot,
Lorenzo

Re: [RFC PATCH v3 2/2] drivers: mfd: vexpress: add Serial Power Controller (SPC) support

[Olof Johansson]

Hi,

Overall this driver looks like it just needs more cooking
time. It's... gritty.  Complicated when it should be simple and
layered. Naming is nonobvious, and overall it's hard to glance at a
function and say "ah, it does <x>".

I think some of this might be because of naming conventions. Lots of long
prefixes, subsystem indirection, etc. I wish I had a straight answer to
what would make it better, but just more overall polish.

So, I have a bunch of comments below. Most of these are related to
readability, which is one of the most important things of new code
these days.

Please find a shorter suitable prefix than vexpress_spc_.* too, it's
way too long.

On Thu, Jun 06, 2013 at 10:59:23AM +0100, Lorenzo Pieralisi wrote:
> 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, operating points and reset control.
> 
> This patch provides a driver that enables run-time control of features
> implemented by the SPC control logic.
> 
> The driver also provides a bridge interface through the vexpress config
> infrastructure. Operations allowing to read/write operating points are
> made to go via the same interface as configuration transactions so that
> all requests to M3 are serialized.
> 
> Device tree bindings documentation for the SPC component is provided with
> the patchset.
> 
> Cc: Samuel Ortiz <sameo@linux.intel.com>
> Cc: Pawel Moll <pawel.moll@arm.com>
> Cc: Nicolas Pitre <nicolas.pitre@linaro.org>
> Cc: Amit Kucheria <amit.kucheria@linaro.org>
> Cc: Jon Medhurst <tixy@linaro.org>
> Signed-off-by: Achin Gupta <achin.gupta@arm.com>
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Signed-off-by: Sudeep KarkadaNagesha <Sudeep.KarkadaNagesha@arm.com>
> Reviewed-by: Nicolas Pitre <nico@linaro.org>
> ---
>  Documentation/devicetree/bindings/mfd/vexpress-spc.txt |  35 +
>  drivers/mfd/Kconfig                                    |   7 +
>  drivers/mfd/Makefile                                   |   1 +
>  drivers/mfd/vexpress-spc.c                             | 633 ++++++++++
>  include/linux/vexpress.h                               |  43 +
>  5 files changed, 719 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..1d71dc2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/vexpress-spc.txt
> @@ -0,0 +1,35 @@
> +* 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/voltage and
> +operating point transitions, through memory mapped registers interface.
> +
> +On testchips like TC2 it also provides a configuration interface that can
> +be used to read/write values which cannot be read/written through simple
> +memory mapped reads/writes.

A configuration interface for what? Just having it as a PMIC doesn't warrant it
being an MFD, really.

> +- 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: spc@7fff0000 {
> +	compatible = "arm,vexpress-spc,v2p-ca15_a7","arm,vexpress-spc";

Nit: space after comma between strings.

> +	reg = <0 0x7FFF0000 0 0x1000>;

#size-cells 2 on the parent bus? That's somewhat unusual. We tend to prefer
lowercase hex.

> +	interrupts = <0 95 4>;
> +};
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index d54e985..391eda1 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1148,3 +1148,10 @@ 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
> +	depends on VEXPRESS_CONFIG
> +	help
> +	  Serial Power Controller driver for ARM Ltd. test chips.

One more line as to what conditions you'd like to have this enabled for would
be good.

> 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..0c6718a
> --- /dev/null
> +++ b/drivers/mfd/vexpress-spc.c
> @@ -0,0 +1,633 @@
> +/*
> + * Versatile Express Serial Power Controller (SPC) support
> + *
> + * Copyright (C) 2013 ARM Ltd.
> + *
> + * Authors: Sudeep KarkadaNagesha <sudeep.karkadanagesha@arm.com>
> + *          Achin Gupta           <achin.gupta@arm.com>
> + *          Lorenzo Pieralisi     <lorenzo.pieralisi@arm.com>
> + *
> + * 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/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/slab.h>
> +#include <linux/vexpress.h>
> +
> +#include <asm/cacheflush.h>
> +
> +#define SCC_CFGREG19		0x120
> +#define SCC_CFGREG20		0x124
> +#define A15_CONF		0x400
> +#define A7_CONF			0x500
> +#define SYS_INFO		0x700
> +#define PERF_LVL_A15		0xB00
> +#define PERF_REQ_A15		0xB04
> +#define PERF_LVL_A7		0xB08
> +#define PERF_REQ_A7		0xB0c
> +#define SYS_CFGCTRL		0xB10
> +#define SYS_CFGCTRL_REQ		0xB14
> +#define PWC_STATUS		0xB18
> +#define PWC_FLAG		0xB1c
> +#define WAKE_INT_MASK		0xB24
> +#define WAKE_INT_RAW		0xB28
> +#define WAKE_INT_STAT		0xB2c
> +#define A15_PWRDN_EN		0xB30
> +#define A7_PWRDN_EN		0xB34
> +#define A7_PWRDNACK		0xB54
> +#define A15_BX_ADDR0		0xB68
> +#define SYS_CFG_WDATA		0xB70
> +#define SYS_CFG_RDATA		0xB74
> +#define A7_BX_ADDR0		0xB78

These are register offsets? A shared shrot prefix could make that more obvious
when reading the code below.

> +
> +#define GBL_WAKEUP_INT_MSK	(0x3 << 10)
> +
> +#define CLKF_SHIFT		16
> +#define CLKF_MASK		0x1FFF
> +#define CLKR_SHIFT		0
> +#define CLKR_MASK		0x3F
> +#define CLKOD_SHIFT		8
> +#define CLKOD_MASK		0xF

What registers are these for? Having them defined right below the register
helps the mental grouping.

> +
> +#define OPP_FUNCTION		6
> +#define OPP_BASE_DEVICE		0x300
> +#define OPP_A15_OFFSET		0x4
> +#define OPP_A7_OFFSET		0xc
> +
> +#define SYS_CFGCTRL_START	(1 << 31)
> +#define SYS_CFGCTRL_WRITE	(1 << 30)
> +#define SYS_CFGCTRL_FUNC(n)	(((n) & 0x3f) << 20)
> +#define SYS_CFGCTRL_DEVICE(n)	(((n) & 0xfff) << 0)
> +
> +#define MAX_OPPS	8
> +#define MAX_CLUSTERS	2
> +
> +enum {
> +	A15_OPP_TYPE		= 0,
> +	A7_OPP_TYPE		= 1,
> +	SYS_CFGCTRL_TYPE	= 2,
> +	INVALID_TYPE
> +};

Some brief notes about what the OPPs are here would be beneficial.

Also, prefixing makes more sense:

OPP_TYPE_A15
OPP_TYPE_A7
...


> +#define STAT_COMPLETE(type)	((1 << 0) << (type << 2))
> +#define STAT_ERR(type)		((1 << 1) << (type << 2))
> +#define RESPONSE_MASK(type)	(STAT_COMPLETE(type) | STAT_ERR(type))
> +
> +struct vexpress_spc_drvdata {
> +	void __iomem *baseaddr;
> +	u32 a15_clusid;
> +	int irq;
> +	u32 cur_req_type;
> +	u32 freqs[MAX_CLUSTERS][MAX_OPPS];
> +	int freqs_cnt[MAX_CLUSTERS];

Please document the non-obvious ones. Is a15 the dynamic cluster id of the A15
cluster perhaps?

> +};
> +
> +enum spc_func_type {
> +	CONFIG_FUNC = 0,
> +	PERF_FUNC   = 1,
> +};
> +
> +struct vexpress_spc_func {
> +	enum spc_func_type type;
> +	u32 function;
> +	u32 device;
> +};
> +
> +static struct vexpress_spc_drvdata *info;
> +static u32 *vexpress_spc_config_data;
> +static struct vexpress_config_bridge *vexpress_spc_config_bridge;
> +static struct vexpress_config_func *opp_func, *perf_func;
> +
> +static int vexpress_spc_load_result = -EAGAIN;

I think Sam already commented on some of these.

> +
> +static bool vexpress_spc_initialized(void)
> +{
> +	return vexpress_spc_load_result == 0;
> +}
> +
> +/**
> + * vexpress_spc_write_resume_reg() - 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 vexpress_spc_write_resume_reg(u32 cluster, u32 cpu, u32 addr)
> +{
> +	void __iomem *baseaddr;
> +
> +	if (WARN_ON_ONCE(cluster >= MAX_CLUSTERS))
> +		return;

Do you really need these in every function down the call chain?

> +
> +	if (cluster != info->a15_clusid)

You make this test in a bunch of places, I think a helper would be beneficial.

cluster_is_a15(cluster), or whatever.

> +		baseaddr = info->baseaddr + A7_BX_ADDR0 + (cpu << 2);
> +	else
> +		baseaddr = info->baseaddr + A15_BX_ADDR0 + (cpu << 2);
> +
> +	writel_relaxed(addr, baseaddr);
> +}
> +
> +/**
> + * vexpress_spc_get_nb_cpus() - get number of cpus in a cluster
> + *
> + * @cluster: mpidr[15:8] bitfield describing cluster affinity level
> + *
> + * Return: number of cpus in the cluster
> + *         -EINVAL if cluster number invalid
> + */
> +int vexpress_spc_get_nb_cpus(u32 cluster)

Nit: nb_cpus? nr_cpus is more common.

> +{
> +	u32 val;
> +
> +	if (WARN_ON_ONCE(cluster >= MAX_CLUSTERS))
> +		return -EINVAL;
> +
> +	val = readl_relaxed(info->baseaddr + SYS_INFO);
> +	val = (cluster != info->a15_clusid) ? (val >> 20) : (val >> 16);
> +	return val & 0xf;
> +}
> +EXPORT_SYMBOL_GPL(vexpress_spc_get_nb_cpus);
> +
> +/**
> + * vexpress_spc_get_performance - get current performance level of cluster
> + * @cluster: mpidr[15:8] bitfield describing cluster affinity level
> + * @freq: pointer to the performance level to be assigned

Can't you return the perf level instead, with negative for error?

> + *
> + * Return: 0 on success
> + *         < 0 on read error
> + */
> +int vexpress_spc_get_performance(u32 cluster, u32 *freq)

This seems to be a get_freq function, should be named accordingly.


> +{
> +	u32 perf_cfg_reg;
> +	int perf, ret;
> +
> +	if (!vexpress_spc_initialized() || (cluster >= MAX_CLUSTERS))
> +		return -EINVAL;
> +
> +	perf_cfg_reg = cluster != info->a15_clusid ? PERF_LVL_A7 : PERF_LVL_A15;
> +	ret = vexpress_config_read(perf_func, perf_cfg_reg, &perf);
> +
> +	if (!ret)
> +		*freq = info->freqs[cluster][perf];
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(vexpress_spc_get_performance);
> +
> +/**
> + * vexpress_spc_get_perf_index - get performance level corresponding to
> + *				 a frequency
> + * @cluster: mpidr[15:8] bitfield describing cluster affinity level
> + * @freq: frequency to be looked-up
> + *
> + * Return: perf level index on success
> + *         -EINVAL on error
> + */

Can there be such a thing as overdocumenting stuff? Maybe. :-) For trivial
helpers that don't export an interface it's much better if the code is so
obvious to read than needing a defined interface.

I think you'd be better off without the helper here, since it's only used
a single time.

> +static int vexpress_spc_find_perf_index(u32 cluster, u32 freq)
> +{
> +	int idx;
> +
> +	for (idx = 0; idx < info->freqs_cnt[cluster]; idx++)
> +		if (info->freqs[cluster][idx] == freq)
> +			break;
> +	return (idx == info->freqs_cnt[cluster]) ? -EINVAL : idx;
> +}
> +
> +/**
> + * vexpress_spc_set_performance - set current performance level of cluster
> + *
> + * @cluster: mpidr[15:8] bitfield describing cluster affinity level
> + * @freq: performance level to be programmed

Performance level, or target frequency frequency? Either the help is
wrong, or the name is confusing.

> + *
> + * Returns: 0 on success
> + *          < 0 on write error
> + */
> +int vexpress_spc_set_performance(u32 cluster, u32 freq)

Again, set_freq() seems more appropriate.

> +{
> +	int ret, perf, offset;
> +
> +	if (!vexpress_spc_initialized() || (cluster >= MAX_CLUSTERS))
> +		return -EINVAL;
> +
> +	offset = (cluster != info->a15_clusid) ? PERF_LVL_A7 : PERF_LVL_A15;
> +
> +	perf = vexpress_spc_find_perf_index(cluster, freq);
> +
> +	if (perf < 0 || perf >= MAX_OPPS)
> +		return -EINVAL;
> +
> +	ret = vexpress_config_write(perf_func, offset, perf);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(vexpress_spc_set_performance);
> +
> +static void vexpress_spc_set_wake_intr(u32 mask)
> +{
> +	writel_relaxed(mask & VEXPRESS_SPC_WAKE_INTR_MASK,
> +		       info->baseaddr + WAKE_INT_MASK);
> +}
> +
> +static inline void reg_bitmask(u32 *reg, u32 mask, bool set)
> +{
> +	if (set)
> +		*reg |= mask;
> +	else
> +		*reg &= ~mask;
> +}
> +
> +/**
> + * vexpress_spc_set_global_wakeup_intr()
> + *
> + * 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 vexpress_spc_set_global_wakeup_intr(bool set)

vexpress_spc_set_foo, which passes in "set" which can either be true or false? Say what?

> +{
> +	u32 wake_int_mask_reg = 0;
> +
> +	wake_int_mask_reg = readl_relaxed(info->baseaddr + WAKE_INT_MASK);
> +	reg_bitmask(&wake_int_mask_reg, GBL_WAKEUP_INT_MSK, set);

This helper just obfuscates, in my opinion.

> +	vexpress_spc_set_wake_intr(wake_int_mask_reg);
> +}
> +
> +/**
> + * vexpress_spc_set_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.
> + *
> + * @cpu: mpidr[7:0] bitfield describing cpu affinity level
> + * @cluster: mpidr[15:8] bitfield describing cluster affinity level
> + * @set: if true, wake-up IRQs are set, if false they are cleared
> + */
> +void vexpress_spc_set_cpu_wakeup_irq(u32 cpu, u32 cluster, bool set)

Same here, set in name vs set argument.

> +{
> +	u32 mask = 0;
> +	u32 wake_int_mask_reg = 0;
> +
> +	mask = 1 << cpu;
> +	if (info->a15_clusid != cluster)
> +		mask <<= 4;
> +
> +	wake_int_mask_reg = readl_relaxed(info->baseaddr + WAKE_INT_MASK);
> +	reg_bitmask(&wake_int_mask_reg, mask, set);
> +	vexpress_spc_set_wake_intr(wake_int_mask_reg);
> +}
> +
> +/**
> + * vexpress_spc_powerdown_enable()
> + *
> + * 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 vexpress_spc_powerdown_enable(u32 cluster, bool enable)

Again with the enable in name vs argument.

> +{
> +	u32 pwdrn_reg = 0;
> +
> +	if (cluster >= MAX_CLUSTERS)
> +		return;
> +	pwdrn_reg = cluster != info->a15_clusid ? A7_PWRDN_EN : A15_PWRDN_EN;
> +	writel_relaxed(enable, info->baseaddr + pwdrn_reg);
> +}
> +
> +irqreturn_t vexpress_spc_irq_handler(int irq, void *data)
> +{
> +	int ret;
> +	u32 status = readl_relaxed(info->baseaddr + PWC_STATUS);

Why readl_relaxed() here? Can't you use a normal readl()?

> +	if (!(status & RESPONSE_MASK(info->cur_req_type)))
> +		return IRQ_NONE;

"pending" is maybe a better term to use than "current" here. I.e. "pending_req"
as a variable name instead.

> +
> +	if ((status == STAT_COMPLETE(SYS_CFGCTRL_TYPE))
> +			&& vexpress_spc_config_data) {
> +		*vexpress_spc_config_data =
> +				readl_relaxed(info->baseaddr + SYS_CFG_RDATA);
> +		vexpress_spc_config_data = NULL;
> +	}
> +
> +	ret = STAT_COMPLETE(info->cur_req_type) ? 0 : -EIO;
> +	info->cur_req_type = INVALID_TYPE;

INVALID_TYPE is 3, so the RESPONSE_MASK() check above will pass. That's
probably not desireable.

> +	vexpress_config_complete(vexpress_spc_config_bridge, ret);
> +	return IRQ_HANDLED;
> +}
> +
> +/**
> + * Based on the firmware documentation, this is always fixed to 20

What is always fixed to 20? the multiplication factor? So why do we need to get
it?

> + * All the 4 OSC: A15 PLL0/1, A7 PLL0/1 must be programmed same
> + * values for both control and value registers.
> + * This function uses A15 PLL 0 registers to compute multiple factor
> + * F out = F in * (CLKF + 1) / ((CLKOD + 1) * (CLKR + 1))
> + */
> +static inline int __get_mult_factor(void)

__get_opp_freq_unit()?

> +{
> +	int i_div, o_div, f_div;
> +	u32 tmp;
> +
> +	tmp = readl(info->baseaddr + SCC_CFGREG19);
> +	f_div = (tmp >> CLKF_SHIFT) & CLKF_MASK;
> +
> +	tmp = readl(info->baseaddr + SCC_CFGREG20);
> +	o_div = (tmp >> CLKOD_SHIFT) & CLKOD_MASK;
> +	i_div = (tmp >> CLKR_SHIFT) & CLKR_MASK;
> +
> +	return (f_div + 1) / ((o_div + 1) * (i_div + 1));
> +}
> +
> +/**
> + * vexpress_spc_populate_opps() - initialize opp tables from microcontroller
> + *
> + * @cluster: mpidr[15:8] bitfield describing cluster affinity level
> + *
> + * Return: 0 on success
> + *         < 0 on error
> + */
> +static int vexpress_spc_populate_opps(u32 cluster)
> +{
> +	u32 data = 0, ret, i, offset;
> +	int mult_fact = __get_mult_factor();
> +
> +	if (WARN_ON_ONCE(cluster >= MAX_CLUSTERS))
> +		return -EINVAL;
> +
> +	offset = cluster != info->a15_clusid ? OPP_A7_OFFSET : OPP_A15_OFFSET;
> +	for (i = 0; i < MAX_OPPS; i++) {
> +		ret = vexpress_config_read(opp_func, i + offset, &data);

offset + i

> +		if (!ret)
> +			info->freqs[cluster][i] = (data & 0xFFFFF) * mult_fact;
> +		else
> +			break;
> +	}
> +
> +	info->freqs_cnt[cluster] = i;
> +	return ret;
> +}
> +
> +/**
> + * vexpress_spc_get_freq_table() - Retrieve a pointer to the frequency
> + *				   table for a given cluster
> + *
> + * @cluster: mpidr[15:8] bitfield describing cluster affinity level
> + * @fptr: pointer to be initialized
> + * Return: operating points count on success
> + *         -EINVAL on pointer error
> + */
> +int vexpress_spc_get_freq_table(u32 cluster, u32 **fptr)
> +{
> +	if (WARN_ON_ONCE(!fptr || cluster >= MAX_CLUSTERS))
> +		return -EINVAL;
> +	*fptr = info->freqs[cluster];
> +	return info->freqs_cnt[cluster];
> +}
> +EXPORT_SYMBOL_GPL(vexpress_spc_get_freq_table);

Exporting the pointer to the table is a little scary since the caller
could then modify it. Would be nicer to pass in an array that is
populated.


I feel like a hypocrite for having to say this, but here's there's a need for
docs. You've documented the short trivial functions and not some of the long
ones... :)

> +static void *vexpress_spc_func_get(struct device *dev,
> +		struct device_node *node, const char *id)
> +{
> +	struct vexpress_spc_func *spc_func;
> +	u32 func_device[2];
> +	int err = 0;
> +
> +	spc_func = kzalloc(sizeof(*spc_func), GFP_KERNEL);
> +	if (!spc_func)
> +		return NULL;
> +
> +	if (strcmp(id, "opp") == 0) {
> +		spc_func->type = CONFIG_FUNC;
> +		spc_func->function = OPP_FUNCTION;
> +		spc_func->device = OPP_BASE_DEVICE;
> +	} else if (strcmp(id, "perf") == 0) {
> +		spc_func->type = PERF_FUNC;
> +	} else if (node) {
> +		of_node_get(node);
> +		err = of_property_read_u32_array(node,
> +				"arm,vexpress-sysreg,func", func_device,
> +				ARRAY_SIZE(func_device));
> +		of_node_put(node);
> +		spc_func->type = CONFIG_FUNC;
> +		spc_func->function = func_device[0];
> +		spc_func->device = func_device[1];
> +	}
> +
> +	if (WARN_ON(err)) {
> +		kfree(spc_func);
> +		return NULL;
> +	}
> +
> +	pr_debug("func 0x%p = 0x%x, %d %d\n", spc_func,
> +					     spc_func->function,
> +					     spc_func->device,
> +					     spc_func->type);
> +
> +	return spc_func;
> +}
> +
> +static void vexpress_spc_func_put(void *func)
> +{
> +	kfree(func);
> +}
> +
> +static int vexpress_spc_func_exec(void *func, int offset, bool write,
> +				  u32 *data)
> +{
> +	struct vexpress_spc_func *spc_func = func;
> +	u32 command;
> +
> +	if (!data)
> +		return -EINVAL;
> +	/*
> +	 * Setting and retrieval of operating points is not part of
> +	 * DCC config interface. It was made to go through the same
> +	 * code path so that requests to the M3 can be serialized
> +	 * properly with config reads/writes through the common
> +	 * vexpress config interface
> +	 */
> +	switch (spc_func->type) {
> +	case PERF_FUNC:
> +		if (write) {
> +			info->cur_req_type = (offset == PERF_LVL_A15) ?
> +					A15_OPP_TYPE : A7_OPP_TYPE;
> +			writel_relaxed(*data, info->baseaddr + offset);
> +			return VEXPRESS_CONFIG_STATUS_WAIT;
> +		} else {
> +			*data = readl_relaxed(info->baseaddr + offset);
> +			return VEXPRESS_CONFIG_STATUS_DONE;
> +		}
> +	case CONFIG_FUNC:
> +		info->cur_req_type = SYS_CFGCTRL_TYPE;
> +
> +		command = SYS_CFGCTRL_START;
> +		command |= write ? SYS_CFGCTRL_WRITE : 0;
> +		command |= SYS_CFGCTRL_FUNC(spc_func->function);
> +		command |= SYS_CFGCTRL_DEVICE(spc_func->device + offset);
> +
> +		pr_debug("command %x\n", command);
> +
> +		if (!write)
> +			vexpress_spc_config_data = data;
> +		else
> +			writel_relaxed(*data, info->baseaddr + SYS_CFG_WDATA);
> +		writel_relaxed(command, info->baseaddr + SYS_CFGCTRL);
> +
> +		return VEXPRESS_CONFIG_STATUS_WAIT;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +struct vexpress_config_bridge_info vexpress_spc_config_bridge_info = {
> +	.name = "vexpress-spc",
> +	.func_get = vexpress_spc_func_get,
> +	.func_put = vexpress_spc_func_put,
> +	.func_exec = vexpress_spc_func_exec,
> +};

Caveat: I'm running out of time reviewing this one driver, and didn't look into
the vexpress_config_bridge stuff. Seems like a pretty awkward interface but
I didn't look closer.

> +
> +static const struct of_device_id vexpress_spc_ids[] __initconst = {
> +	{ .compatible = "arm,vexpress-spc,v2p-ca15_a7" },
> +	{ .compatible = "arm,vexpress-spc" },
> +	{},
> +};
> +
> +static int __init vexpress_spc_init(void)
> +{
> +	int ret;
> +	struct device_node *node = of_find_matching_node(NULL,
> +							 vexpress_spc_ids);
> +
> +	if (!node)
> +		return -ENODEV;
> +
> +	info = kzalloc(sizeof(*info), GFP_KERNEL);
> +	if (!info) {
> +		pr_err("%s: unable to allocate mem\n", __func__);
> +		return -ENOMEM;
> +	}
> +	info->cur_req_type = INVALID_TYPE;
> +
> +	info->baseaddr = of_iomap(node, 0);
> +	if (WARN_ON(!info->baseaddr)) {

pr_err() something or other.

> +		ret = -ENXIO;
> +		goto mem_free;
> +	}
> +
> +	info->irq = irq_of_parse_and_map(node, 0);
> +
> +	if (WARN_ON(!info->irq)) {

pr_err() something or other.

> +		ret = -ENXIO;
> +		goto unmap;
> +	}
> +
> +	readl_relaxed(info->baseaddr + PWC_STATUS);
> +
> +	ret = request_irq(info->irq, vexpress_spc_irq_handler,
> +		IRQF_DISABLED | IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
> +		"arm-spc", info);
> +
> +	if (ret) {
> +		pr_err("IRQ %d request failed\n", info->irq);
> +		ret = -ENODEV;
> +		goto unmap;
> +	}
> +
> +	info->a15_clusid = readl_relaxed(info->baseaddr + A15_CONF) & 0xf;
> +
> +	vexpress_spc_config_bridge = vexpress_config_bridge_register(
> +			node, &vexpress_spc_config_bridge_info);
> +
> +	if (WARN_ON(!vexpress_spc_config_bridge)) {

pr_err()

> +		ret = -ENODEV;
> +		goto unmap;
> +	}
> +
> +	opp_func = vexpress_config_func_get(vexpress_spc_config_bridge, "opp");
> +	perf_func =
> +		vexpress_config_func_get(vexpress_spc_config_bridge, "perf");

Your identifiers are so long that you can't fit a simple call on minimum
indentation without wrapping the line. That should be a pretty strong indicator
that it needs to be shortened.

> +
> +	if (!opp_func || !perf_func) {

...pr_err()

> +		ret = -ENODEV;
> +		goto unmap;
> +	}
> +
> +	if (vexpress_spc_populate_opps(0) || vexpress_spc_populate_opps(1)) {
> +		if (info->irq)
> +			free_irq(info->irq, info);
> +		pr_err("failed to build OPP table\n");
> +		ret = -ENODEV;
> +		goto unmap;
> +	}
> +	/*
> +	 * Multi-cluster systems may need this data when non-coherent, during
> +	 * cluster power-up/power-down. Make sure it reaches main memory:
> +	 */
> +	sync_cache_w(info);
> +	sync_cache_w(&info);
> +	pr_info("vexpress-spc loaded at %p\n", info->baseaddr);
> +	return 0;
> +
> +unmap:
> +	iounmap(info->baseaddr);
> +
> +mem_free:
> +	kfree(info);
> +	return ret;
> +}
> +
> +static bool __init __vexpress_spc_check_loaded(void);
> +/*
> + * Pointer spc_check_loaded is swapped after init hence it is safe
> + * to initialize it to a function in the __init section
> + */
> +static bool (*spc_check_loaded)(void) __refdata = &__vexpress_spc_check_loaded;
> +
> +static bool __init __vexpress_spc_check_loaded(void)
> +{
> +	if (vexpress_spc_load_result == -EAGAIN)
> +		vexpress_spc_load_result = vexpress_spc_init();
> +	spc_check_loaded = &vexpress_spc_initialized;
> +	return vexpress_spc_initialized();
> +}

Whoa. A "check_foo" function with side effects. Red flag.

So, you only want to try to run vexpress_spc_init() on the very first call to
spc_check_loaded()? Just do so in vexpress_spc_initalized() instead and keep
a static bool first_call = true; variable instead. Still confusing and with
side effects though.

> +
> +/*
> + * Function exported to manage early_initcall ordering.
> + * SPC code is needed very early in the boot process
> + * to bring CPUs out of reset and initialize power
> + * management back-end. After boot swap pointers to
> + * make the functionality check available to loadable
> + * modules, when early boot init functions have been
> + * already freed from kernel address space.
> + */
> +bool vexpress_spc_check_loaded(void)
> +{
> +	return spc_check_loaded();
> +}
> +EXPORT_SYMBOL_GPL(vexpress_spc_check_loaded);

Or, you do that test I mentioned right above here instead.

> +
> +static int __init vexpress_spc_early_init(void)
> +{
> +	__vexpress_spc_check_loaded();
> +	return vexpress_spc_load_result;
> +}
> +early_initcall(vexpress_spc_early_init);
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Serial Power Controller (SPC) support");
> diff --git a/include/linux/vexpress.h b/include/linux/vexpress.h
> index 50368e0..c1191ab 100644
> --- a/include/linux/vexpress.h
> +++ b/include/linux/vexpress.h
> @@ -132,4 +132,47 @@ void vexpress_clk_of_register_spc(void);
>  void vexpress_clk_init(void __iomem *sp810_base);
>  void vexpress_clk_of_init(void);
>  
> +/* SPC */
> +
> +#define	VEXPRESS_SPC_WAKE_INTR_IRQ(cluster, cpu) \
> +			(1 << (4 * (cluster) + (cpu)))
> +#define	VEXPRESS_SPC_WAKE_INTR_FIQ(cluster, cpu) \
> +			(1 << (7 * (cluster) + (cpu)))
> +#define	VEXPRESS_SPC_WAKE_INTR_SWDOG		(1 << 10)
> +#define	VEXPRESS_SPC_WAKE_INTR_GTIMER		(1 << 11)
> +#define	VEXPRESS_SPC_WAKE_INTR_MASK		0xFFF
> +
> +#ifdef CONFIG_VEXPRESS_SPC
> +extern bool vexpress_spc_check_loaded(void);
> +extern void vexpress_spc_set_cpu_wakeup_irq(u32 cpu, u32 cluster, bool set);
> +extern void vexpress_spc_set_global_wakeup_intr(bool set);
> +extern int vexpress_spc_get_freq_table(u32 cluster, u32 **fptr);
> +extern int vexpress_spc_get_performance(u32 cluster, u32 *freq);
> +extern int vexpress_spc_set_performance(u32 cluster, u32 freq);
> +extern void vexpress_spc_write_resume_reg(u32 cluster, u32 cpu, u32 addr);
> +extern int vexpress_spc_get_nb_cpus(u32 cluster);
> +extern void vexpress_spc_powerdown_enable(u32 cluster, bool enable);
> +#else
> +static inline bool vexpress_spc_check_loaded(void) { return false; }
> +static inline void vexpress_spc_set_cpu_wakeup_irq(u32 cpu, u32 cluster,
> +						   bool set) { }
> +static inline void vexpress_spc_set_global_wakeup_intr(bool set) { }
> +static inline int vexpress_spc_get_freq_table(u32 cluster, u32 **fptr)
> +{
> +	return -ENODEV;
> +}
> +static inline int vexpress_spc_get_performance(u32 cluster, u32 *freq)
> +{
> +	return -ENODEV;
> +}
> +static inline int vexpress_spc_set_performance(u32 cluster, u32 freq)
> +{
> +	return -ENODEV;
> +}
> +static inline void vexpress_spc_write_resume_reg(u32 cluster,
> +						 u32 cpu, u32 addr) { }
> +static inline int vexpress_spc_get_nb_cpus(u32 cluster) { return -ENODEV; }
> +static inline void vexpress_spc_powerdown_enable(u32 cluster, bool enable) { }
> +#endif
> +
>  #endif
> -- 
> 1.8.2.2
> 
> 
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [RFC PATCH v3 2/2] drivers: mfd: vexpress: add Serial Power Controller (SPC) support

[Nicolas Pitre]

On Thu, 13 Jun 2013, Olof Johansson wrote:

> > +	u32 status = readl_relaxed(info->baseaddr + PWC_STATUS);
> 
> Why readl_relaxed() here? Can't you use a normal readl()?

Unfortunately, on ARM readl_relaxed() _is_ the normal readl() because 
the actual readl() may have side effects.  See commit 79f64dbf68c8.


Nicolas

Re: [RFC PATCH v3 2/2] drivers: mfd: vexpress: add Serial Power Controller (SPC) support

[Pawel Moll]

On Thu, 2013-06-13 at 23:52 +0100, Olof Johansson wrote:
> > +     reg = <0 0x7FFF0000 0 0x1000>;
> 
> #size-cells 2 on the parent bus? That's somewhat unusual. 

LPAE == 40 bit physical addresses == potential > 32 bit sizes (memory
blocks > 4GB)

Paweł


_______________________________________________
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [RFC PATCH v3 2/2] drivers: mfd: vexpress: add Serial Power Controller (SPC) support

[Olof Johansson]

On Fri, Jun 14, 2013 at 02:04:00PM +0100, Pawel Moll wrote:
> On Thu, 2013-06-13 at 23:52 +0100, Olof Johansson wrote:
> > > +     reg = <0 0x7FFF0000 0 0x1000>;
> > 
> > #size-cells 2 on the parent bus? That's somewhat unusual. 
> 
> LPAE == 40 bit physical addresses == potential > 32 bit sizes (memory
> blocks > 4GB)

Yeah, I guess that's most commonly needed for the /memory nodes -- having
devices with more than 4GB of register space is quite unusual.

It doesn't really matter much, but it saves some padding of 0s if you
pick a smaller value.

I thought we used 2/1 for address/size-cells on PA Semi hardware, but I just
checked and we had 2/2. Maybe it was Apple that used 2/1.

Anyway, no big deal.


-Olof

Re: [RFC PATCH v3 2/2] drivers: mfd: vexpress: add Serial Power Controller (SPC) support

[Lorenzo Pieralisi]

Hi Olof,

thank you very much for having a look.

On Thu, Jun 13, 2013 at 11:52:33PM +0100, Olof Johansson wrote:
> Hi,
> 
> Overall this driver looks like it just needs more cooking
> time. It's... gritty.  Complicated when it should be simple and
> layered. Naming is nonobvious, and overall it's hard to glance at a
> function and say "ah, it does <x>".

It is hard to make it clear too, and I am not on the defensive, it is
not a standard component following a standard interface, but point taken
I will do my best to improve it according to your reviews.

> I think some of this might be because of naming conventions. Lots of long
> prefixes, subsystem indirection, etc. I wish I had a straight answer to
> what would make it better, but just more overall polish.
> 
> So, I have a bunch of comments below. Most of these are related to
> readability, which is one of the most important things of new code
> these days.
> 
> Please find a shorter suitable prefix than vexpress_spc_.* too, it's
> way too long.

Ok.

> On Thu, Jun 06, 2013 at 10:59:23AM +0100, Lorenzo Pieralisi wrote:
> > 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, operating points and reset control.
> >
> > This patch provides a driver that enables run-time control of features
> > implemented by the SPC control logic.
> >
> > The driver also provides a bridge interface through the vexpress config
> > infrastructure. Operations allowing to read/write operating points are
> > made to go via the same interface as configuration transactions so that
> > all requests to M3 are serialized.
> >
> > Device tree bindings documentation for the SPC component is provided with
> > the patchset.
> >
> > Cc: Samuel Ortiz <sameo@linux.intel.com>
> > Cc: Pawel Moll <pawel.moll@arm.com>
> > Cc: Nicolas Pitre <nicolas.pitre@linaro.org>
> > Cc: Amit Kucheria <amit.kucheria@linaro.org>
> > Cc: Jon Medhurst <tixy@linaro.org>
> > Signed-off-by: Achin Gupta <achin.gupta@arm.com>
> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > Signed-off-by: Sudeep KarkadaNagesha <Sudeep.KarkadaNagesha@arm.com>
> > Reviewed-by: Nicolas Pitre <nico@linaro.org>
> > ---
> >  Documentation/devicetree/bindings/mfd/vexpress-spc.txt |  35 +
> >  drivers/mfd/Kconfig                                    |   7 +
> >  drivers/mfd/Makefile                                   |   1 +
> >  drivers/mfd/vexpress-spc.c                             | 633 ++++++++++
> >  include/linux/vexpress.h                               |  43 +
> >  5 files changed, 719 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..1d71dc2
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mfd/vexpress-spc.txt
> > @@ -0,0 +1,35 @@
> > +* 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/voltage and
> > +operating point transitions, through memory mapped registers interface.
> > +
> > +On testchips like TC2 it also provides a configuration interface that can
> > +be used to read/write values which cannot be read/written through simple
> > +memory mapped reads/writes.
> 
> A configuration interface for what? Just having it as a PMIC doesn't warrant it
> being an MFD, really.

Ok, description added.

> > +- 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: spc@7fff0000 {
> > +     compatible = "arm,vexpress-spc,v2p-ca15_a7","arm,vexpress-spc";
> 
> Nit: space after comma between strings.
> 
> > +     reg = <0 0x7FFF0000 0 0x1000>;
> 
> #size-cells 2 on the parent bus? That's somewhat unusual. We tend to prefer
> lowercase hex.
> 

Ok on both counts.

> > +     interrupts = <0 95 4>;
> > +};
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > index d54e985..391eda1 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -1148,3 +1148,10 @@ 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
> > +     depends on VEXPRESS_CONFIG
> > +     help
> > +       Serial Power Controller driver for ARM Ltd. test chips.
> 
> One more line as to what conditions you'd like to have this enabled for would
> be good.

Done.

> > 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..0c6718a
> > --- /dev/null
> > +++ b/drivers/mfd/vexpress-spc.c
> > @@ -0,0 +1,633 @@
> > +/*
> > + * Versatile Express Serial Power Controller (SPC) support
> > + *
> > + * Copyright (C) 2013 ARM Ltd.
> > + *
> > + * Authors: Sudeep KarkadaNagesha <sudeep.karkadanagesha@arm.com>
> > + *          Achin Gupta           <achin.gupta@arm.com>
> > + *          Lorenzo Pieralisi     <lorenzo.pieralisi@arm.com>
> > + *
> > + * 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/interrupt.h>
> > +#include <linux/module.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_irq.h>
> > +#include <linux/slab.h>
> > +#include <linux/vexpress.h>
> > +
> > +#include <asm/cacheflush.h>
> > +
> > +#define SCC_CFGREG19         0x120
> > +#define SCC_CFGREG20         0x124
> > +#define A15_CONF             0x400
> > +#define A7_CONF                      0x500
> > +#define SYS_INFO             0x700
> > +#define PERF_LVL_A15         0xB00
> > +#define PERF_REQ_A15         0xB04
> > +#define PERF_LVL_A7          0xB08
> > +#define PERF_REQ_A7          0xB0c
> > +#define SYS_CFGCTRL          0xB10
> > +#define SYS_CFGCTRL_REQ              0xB14
> > +#define PWC_STATUS           0xB18
> > +#define PWC_FLAG             0xB1c
> > +#define WAKE_INT_MASK                0xB24
> > +#define WAKE_INT_RAW         0xB28
> > +#define WAKE_INT_STAT                0xB2c
> > +#define A15_PWRDN_EN         0xB30
> > +#define A7_PWRDN_EN          0xB34
> > +#define A7_PWRDNACK          0xB54
> > +#define A15_BX_ADDR0         0xB68
> > +#define SYS_CFG_WDATA                0xB70
> > +#define SYS_CFG_RDATA                0xB74
> > +#define A7_BX_ADDR0          0xB78
> 
> These are register offsets? A shared shrot prefix could make that more obvious
> when reading the code below.

Done.

> > +
> > +#define GBL_WAKEUP_INT_MSK   (0x3 << 10)
> > +
> > +#define CLKF_SHIFT           16
> > +#define CLKF_MASK            0x1FFF
> > +#define CLKR_SHIFT           0
> > +#define CLKR_MASK            0x3F
> > +#define CLKOD_SHIFT          8
> > +#define CLKOD_MASK           0xF
> 
> What registers are these for? Having them defined right below the register
> helps the mental grouping.

CLK* removed, multiplier hardcoded, see below. I will comment on the
wake up mask.

> > +
> > +#define OPP_FUNCTION         6
> > +#define OPP_BASE_DEVICE              0x300
> > +#define OPP_A15_OFFSET               0x4
> > +#define OPP_A7_OFFSET                0xc
> > +
> > +#define SYS_CFGCTRL_START    (1 << 31)
> > +#define SYS_CFGCTRL_WRITE    (1 << 30)
> > +#define SYS_CFGCTRL_FUNC(n)  (((n) & 0x3f) << 20)
> > +#define SYS_CFGCTRL_DEVICE(n)        (((n) & 0xfff) << 0)
> > +
> > +#define MAX_OPPS     8
> > +#define MAX_CLUSTERS 2
> > +
> > +enum {
> > +     A15_OPP_TYPE            = 0,
> > +     A7_OPP_TYPE             = 1,
> > +     SYS_CFGCTRL_TYPE        = 2,
> > +     INVALID_TYPE
> > +};
> 
> Some brief notes about what the OPPs are here would be beneficial.
> 
> Also, prefixing makes more sense:
> 
> OPP_TYPE_A15
> OPP_TYPE_A7
> ...

Ok.

> > +#define STAT_COMPLETE(type)  ((1 << 0) << (type << 2))
> > +#define STAT_ERR(type)               ((1 << 1) << (type << 2))
> > +#define RESPONSE_MASK(type)  (STAT_COMPLETE(type) | STAT_ERR(type))
> > +
> > +struct vexpress_spc_drvdata {
> > +     void __iomem *baseaddr;
> > +     u32 a15_clusid;
> > +     int irq;
> > +     u32 cur_req_type;
> > +     u32 freqs[MAX_CLUSTERS][MAX_OPPS];
> > +     int freqs_cnt[MAX_CLUSTERS];
> 
> Please document the non-obvious ones. Is a15 the dynamic cluster id of the A15
> cluster perhaps?

It is A15-MPIDR[15:8], I will document it.

> > +};
> > +
> > +enum spc_func_type {
> > +     CONFIG_FUNC = 0,
> > +     PERF_FUNC   = 1,
> > +};
> > +
> > +struct vexpress_spc_func {
> > +     enum spc_func_type type;
> > +     u32 function;
> > +     u32 device;
> > +};
> > +
> > +static struct vexpress_spc_drvdata *info;
> > +static u32 *vexpress_spc_config_data;
> > +static struct vexpress_config_bridge *vexpress_spc_config_bridge;
> > +static struct vexpress_config_func *opp_func, *perf_func;
> > +
> > +static int vexpress_spc_load_result = -EAGAIN;
> 
> I think Sam already commented on some of these.

Most of them are dynamically allocated now.

> > +
> > +static bool vexpress_spc_initialized(void)
> > +{
> > +     return vexpress_spc_load_result == 0;
> > +}
> > +
> > +/**
> > + * vexpress_spc_write_resume_reg() - 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 vexpress_spc_write_resume_reg(u32 cluster, u32 cpu, u32 addr)
> > +{
> > +     void __iomem *baseaddr;
> > +
> > +     if (WARN_ON_ONCE(cluster >= MAX_CLUSTERS))
> > +             return;
> 
> Do you really need these in every function down the call chain?

I will remove them.

> > +
> > +     if (cluster != info->a15_clusid)
> 
> You make this test in a bunch of places, I think a helper would be beneficial.
> 
> cluster_is_a15(cluster), or whatever.

Done.

> > +             baseaddr = info->baseaddr + A7_BX_ADDR0 + (cpu << 2);
> > +     else
> > +             baseaddr = info->baseaddr + A15_BX_ADDR0 + (cpu << 2);
> > +
> > +     writel_relaxed(addr, baseaddr);
> > +}
> > +
> > +/**
> > + * vexpress_spc_get_nb_cpus() - get number of cpus in a cluster
> > + *
> > + * @cluster: mpidr[15:8] bitfield describing cluster affinity level
> > + *
> > + * Return: number of cpus in the cluster
> > + *         -EINVAL if cluster number invalid
> > + */
> > +int vexpress_spc_get_nb_cpus(u32 cluster)
> 
> Nit: nb_cpus? nr_cpus is more common.

Ok.

> > +{
> > +     u32 val;
> > +
> > +     if (WARN_ON_ONCE(cluster >= MAX_CLUSTERS))
> > +             return -EINVAL;
> > +
> > +     val = readl_relaxed(info->baseaddr + SYS_INFO);
> > +     val = (cluster != info->a15_clusid) ? (val >> 20) : (val >> 16);
> > +     return val & 0xf;
> > +}
> > +EXPORT_SYMBOL_GPL(vexpress_spc_get_nb_cpus);
> > +
> > +/**
> > + * vexpress_spc_get_performance - get current performance level of cluster
> > + * @cluster: mpidr[15:8] bitfield describing cluster affinity level
> > + * @freq: pointer to the performance level to be assigned
> 
> Can't you return the perf level instead, with negative for error?

Ok.

> > + *
> > + * Return: 0 on success
> > + *         < 0 on read error
> > + */
> > +int vexpress_spc_get_performance(u32 cluster, u32 *freq)
> 
> This seems to be a get_freq function, should be named accordingly.

Right, those are leftovers from the conversion from performance level to
frequencies, I will change them.

> > +{
> > +     u32 perf_cfg_reg;
> > +     int perf, ret;
> > +
> > +     if (!vexpress_spc_initialized() || (cluster >= MAX_CLUSTERS))
> > +             return -EINVAL;
> > +
> > +     perf_cfg_reg = cluster != info->a15_clusid ? PERF_LVL_A7 : PERF_LVL_A15;
> > +     ret = vexpress_config_read(perf_func, perf_cfg_reg, &perf);
> > +
> > +     if (!ret)
> > +             *freq = info->freqs[cluster][perf];
> > +
> > +     return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(vexpress_spc_get_performance);
> > +
> > +/**
> > + * vexpress_spc_get_perf_index - get performance level corresponding to
> > + *                            a frequency
> > + * @cluster: mpidr[15:8] bitfield describing cluster affinity level
> > + * @freq: frequency to be looked-up
> > + *
> > + * Return: perf level index on success
> > + *         -EINVAL on error
> > + */
> 
> Can there be such a thing as overdocumenting stuff? Maybe. :-) For trivial
> helpers that don't export an interface it's much better if the code is so
> obvious to read than needing a defined interface.

Agreed.

> I think you'd be better off without the helper here, since it's only used
> a single time.

I am not sure, it is called every time a DVFS OPP is set, maybe it is better
wrapped in a function.

> > +static int vexpress_spc_find_perf_index(u32 cluster, u32 freq)
> > +{
> > +     int idx;
> > +
> > +     for (idx = 0; idx < info->freqs_cnt[cluster]; idx++)
> > +             if (info->freqs[cluster][idx] == freq)
> > +                     break;
> > +     return (idx == info->freqs_cnt[cluster]) ? -EINVAL : idx;
> > +}
> > +
> > +/**
> > + * vexpress_spc_set_performance - set current performance level of cluster
> > + *
> > + * @cluster: mpidr[15:8] bitfield describing cluster affinity level
> > + * @freq: performance level to be programmed
> 
> Performance level, or target frequency frequency? Either the help is
> wrong, or the name is confusing.

You are right, see above.

> > + *
> > + * Returns: 0 on success
> > + *          < 0 on write error
> > + */
> > +int vexpress_spc_set_performance(u32 cluster, u32 freq)
> 
> Again, set_freq() seems more appropriate.

Ditto.

> > +{
> > +     int ret, perf, offset;
> > +
> > +     if (!vexpress_spc_initialized() || (cluster >= MAX_CLUSTERS))
> > +             return -EINVAL;
> > +
> > +     offset = (cluster != info->a15_clusid) ? PERF_LVL_A7 : PERF_LVL_A15;
> > +
> > +     perf = vexpress_spc_find_perf_index(cluster, freq);
> > +
> > +     if (perf < 0 || perf >= MAX_OPPS)
> > +             return -EINVAL;
> > +
> > +     ret = vexpress_config_write(perf_func, offset, perf);
> > +
> > +     return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(vexpress_spc_set_performance);
> > +
> > +static void vexpress_spc_set_wake_intr(u32 mask)
> > +{
> > +     writel_relaxed(mask & VEXPRESS_SPC_WAKE_INTR_MASK,
> > +                    info->baseaddr + WAKE_INT_MASK);
> > +}
> > +
> > +static inline void reg_bitmask(u32 *reg, u32 mask, bool set)
> > +{
> > +     if (set)
> > +             *reg |= mask;
> > +     else
> > +             *reg &= ~mask;
> > +}
> > +
> > +/**
> > + * vexpress_spc_set_global_wakeup_intr()
> > + *
> > + * 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 vexpress_spc_set_global_wakeup_intr(bool set)
> 
> vexpress_spc_set_foo, which passes in "set" which can either be true or false? Say what?

Eh, again leftovers from API refactoring.

> > +{
> > +     u32 wake_int_mask_reg = 0;
> > +
> > +     wake_int_mask_reg = readl_relaxed(info->baseaddr + WAKE_INT_MASK);
> > +     reg_bitmask(&wake_int_mask_reg, GBL_WAKEUP_INT_MSK, set);
> 
> This helper just obfuscates, in my opinion.

I will have a look.

> > +     vexpress_spc_set_wake_intr(wake_int_mask_reg);
> > +}
> > +
> > +/**
> > + * vexpress_spc_set_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.
> > + *
> > + * @cpu: mpidr[7:0] bitfield describing cpu affinity level
> > + * @cluster: mpidr[15:8] bitfield describing cluster affinity level
> > + * @set: if true, wake-up IRQs are set, if false they are cleared
> > + */
> > +void vexpress_spc_set_cpu_wakeup_irq(u32 cpu, u32 cluster, bool set)
> 
> Same here, set in name vs set argument.

Correct.

> > +{
> > +     u32 mask = 0;
> > +     u32 wake_int_mask_reg = 0;
> > +
> > +     mask = 1 << cpu;
> > +     if (info->a15_clusid != cluster)
> > +             mask <<= 4;
> > +
> > +     wake_int_mask_reg = readl_relaxed(info->baseaddr + WAKE_INT_MASK);
> > +     reg_bitmask(&wake_int_mask_reg, mask, set);
> > +     vexpress_spc_set_wake_intr(wake_int_mask_reg);
> > +}
> > +
> > +/**
> > + * vexpress_spc_powerdown_enable()
> > + *
> > + * 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 vexpress_spc_powerdown_enable(u32 cluster, bool enable)
> 
> Again with the enable in name vs argument.

Ditto.

> > +{
> > +     u32 pwdrn_reg = 0;
> > +
> > +     if (cluster >= MAX_CLUSTERS)
> > +             return;
> > +     pwdrn_reg = cluster != info->a15_clusid ? A7_PWRDN_EN : A15_PWRDN_EN;
> > +     writel_relaxed(enable, info->baseaddr + pwdrn_reg);
> > +}
> > +
> > +irqreturn_t vexpress_spc_irq_handler(int irq, void *data)
> > +{
> > +     int ret;
> > +     u32 status = readl_relaxed(info->baseaddr + PWC_STATUS);
> 
> Why readl_relaxed() here? Can't you use a normal readl()?

Nico replied to this.

> > +     if (!(status & RESPONSE_MASK(info->cur_req_type)))
> > +             return IRQ_NONE;
> 
> "pending" is maybe a better term to use than "current" here. I.e. "pending_req"
> as a variable name instead.

Ok.

> > +
> > +     if ((status == STAT_COMPLETE(SYS_CFGCTRL_TYPE))
> > +                     && vexpress_spc_config_data) {
> > +             *vexpress_spc_config_data =
> > +                             readl_relaxed(info->baseaddr + SYS_CFG_RDATA);
> > +             vexpress_spc_config_data = NULL;
> > +     }
> > +
> > +     ret = STAT_COMPLETE(info->cur_req_type) ? 0 : -EIO;

This is a bug, fixed, STAT_COMPLETE should be checked against status.

> > +     info->cur_req_type = INVALID_TYPE;
> 
> INVALID_TYPE is 3, so the RESPONSE_MASK() check above will pass. That's
> probably not desireable.

Not sure I follow you here, but it helped me unearth the bug above.

> > +     vexpress_config_complete(vexpress_spc_config_bridge, ret);
> > +     return IRQ_HANDLED;
> > +}
> > +
> > +/**
> > + * Based on the firmware documentation, this is always fixed to 20
> 
> What is always fixed to 20? the multiplication factor? So why do we need to get
> it?

Removed.

> > + * All the 4 OSC: A15 PLL0/1, A7 PLL0/1 must be programmed same
> > + * values for both control and value registers.
> > + * This function uses A15 PLL 0 registers to compute multiple factor
> > + * F out = F in * (CLKF + 1) / ((CLKOD + 1) * (CLKR + 1))
> > + */
> > +static inline int __get_mult_factor(void)
> 
> __get_opp_freq_unit()?

Gone :-)

> > +{
> > +     int i_div, o_div, f_div;
> > +     u32 tmp;
> > +
> > +     tmp = readl(info->baseaddr + SCC_CFGREG19);
> > +     f_div = (tmp >> CLKF_SHIFT) & CLKF_MASK;
> > +
> > +     tmp = readl(info->baseaddr + SCC_CFGREG20);
> > +     o_div = (tmp >> CLKOD_SHIFT) & CLKOD_MASK;
> > +     i_div = (tmp >> CLKR_SHIFT) & CLKR_MASK;
> > +
> > +     return (f_div + 1) / ((o_div + 1) * (i_div + 1));
> > +}
> > +
> > +/**
> > + * vexpress_spc_populate_opps() - initialize opp tables from microcontroller
> > + *
> > + * @cluster: mpidr[15:8] bitfield describing cluster affinity level
> > + *
> > + * Return: 0 on success
> > + *         < 0 on error
> > + */
> > +static int vexpress_spc_populate_opps(u32 cluster)
> > +{
> > +     u32 data = 0, ret, i, offset;
> > +     int mult_fact = __get_mult_factor();
> > +
> > +     if (WARN_ON_ONCE(cluster >= MAX_CLUSTERS))
> > +             return -EINVAL;
> > +
> > +     offset = cluster != info->a15_clusid ? OPP_A7_OFFSET : OPP_A15_OFFSET;
> > +     for (i = 0; i < MAX_OPPS; i++) {
> > +             ret = vexpress_config_read(opp_func, i + offset, &data);
> 
> offset + i

Ok.

> > +             if (!ret)
> > +                     info->freqs[cluster][i] = (data & 0xFFFFF) * mult_fact;
> > +             else
> > +                     break;
> > +     }
> > +
> > +     info->freqs_cnt[cluster] = i;
> > +     return ret;
> > +}
> > +
> > +/**
> > + * vexpress_spc_get_freq_table() - Retrieve a pointer to the frequency
> > + *                              table for a given cluster
> > + *
> > + * @cluster: mpidr[15:8] bitfield describing cluster affinity level
> > + * @fptr: pointer to be initialized
> > + * Return: operating points count on success
> > + *         -EINVAL on pointer error
> > + */
> > +int vexpress_spc_get_freq_table(u32 cluster, u32 **fptr)
> > +{
> > +     if (WARN_ON_ONCE(!fptr || cluster >= MAX_CLUSTERS))
> > +             return -EINVAL;
> > +     *fptr = info->freqs[cluster];
> > +     return info->freqs_cnt[cluster];
> > +}
> > +EXPORT_SYMBOL_GPL(vexpress_spc_get_freq_table);
> 
> Exporting the pointer to the table is a little scary since the caller
> could then modify it. Would be nicer to pass in an array that is
> populated.

Probably better.

> I feel like a hypocrite for having to say this, but here's there's a need for
> docs. You've documented the short trivial functions and not some of the long
> ones... :)

I will do that.

> > +static void *vexpress_spc_func_get(struct device *dev,
> > +             struct device_node *node, const char *id)
> > +{
> > +     struct vexpress_spc_func *spc_func;
> > +     u32 func_device[2];
> > +     int err = 0;
> > +
> > +     spc_func = kzalloc(sizeof(*spc_func), GFP_KERNEL);
> > +     if (!spc_func)
> > +             return NULL;
> > +
> > +     if (strcmp(id, "opp") == 0) {
> > +             spc_func->type = CONFIG_FUNC;
> > +             spc_func->function = OPP_FUNCTION;
> > +             spc_func->device = OPP_BASE_DEVICE;
> > +     } else if (strcmp(id, "perf") == 0) {
> > +             spc_func->type = PERF_FUNC;
> > +     } else if (node) {
> > +             of_node_get(node);
> > +             err = of_property_read_u32_array(node,
> > +                             "arm,vexpress-sysreg,func", func_device,
> > +                             ARRAY_SIZE(func_device));
> > +             of_node_put(node);
> > +             spc_func->type = CONFIG_FUNC;
> > +             spc_func->function = func_device[0];
> > +             spc_func->device = func_device[1];
> > +     }
> > +
> > +     if (WARN_ON(err)) {
> > +             kfree(spc_func);
> > +             return NULL;
> > +     }
> > +
> > +     pr_debug("func 0x%p = 0x%x, %d %d\n", spc_func,
> > +                                          spc_func->function,
> > +                                          spc_func->device,
> > +                                          spc_func->type);
> > +
> > +     return spc_func;
> > +}
> > +
> > +static void vexpress_spc_func_put(void *func)
> > +{
> > +     kfree(func);
> > +}
> > +
> > +static int vexpress_spc_func_exec(void *func, int offset, bool write,
> > +                               u32 *data)
> > +{
> > +     struct vexpress_spc_func *spc_func = func;
> > +     u32 command;
> > +
> > +     if (!data)
> > +             return -EINVAL;
> > +     /*
> > +      * Setting and retrieval of operating points is not part of
> > +      * DCC config interface. It was made to go through the same
> > +      * code path so that requests to the M3 can be serialized
> > +      * properly with config reads/writes through the common
> > +      * vexpress config interface
> > +      */
> > +     switch (spc_func->type) {
> > +     case PERF_FUNC:
> > +             if (write) {
> > +                     info->cur_req_type = (offset == PERF_LVL_A15) ?
> > +                                     A15_OPP_TYPE : A7_OPP_TYPE;
> > +                     writel_relaxed(*data, info->baseaddr + offset);
> > +                     return VEXPRESS_CONFIG_STATUS_WAIT;
> > +             } else {
> > +                     *data = readl_relaxed(info->baseaddr + offset);
> > +                     return VEXPRESS_CONFIG_STATUS_DONE;
> > +             }
> > +     case CONFIG_FUNC:
> > +             info->cur_req_type = SYS_CFGCTRL_TYPE;
> > +
> > +             command = SYS_CFGCTRL_START;
> > +             command |= write ? SYS_CFGCTRL_WRITE : 0;
> > +             command |= SYS_CFGCTRL_FUNC(spc_func->function);
> > +             command |= SYS_CFGCTRL_DEVICE(spc_func->device + offset);
> > +
> > +             pr_debug("command %x\n", command);
> > +
> > +             if (!write)
> > +                     vexpress_spc_config_data = data;
> > +             else
> > +                     writel_relaxed(*data, info->baseaddr + SYS_CFG_WDATA);
> > +             writel_relaxed(command, info->baseaddr + SYS_CFGCTRL);
> > +
> > +             return VEXPRESS_CONFIG_STATUS_WAIT;
> > +     default:
> > +             return -EINVAL;
> > +     }
> > +}
> > +
> > +struct vexpress_config_bridge_info vexpress_spc_config_bridge_info = {
> > +     .name = "vexpress-spc",
> > +     .func_get = vexpress_spc_func_get,
> > +     .func_put = vexpress_spc_func_put,
> > +     .func_exec = vexpress_spc_func_exec,
> > +};
> 
> Caveat: I'm running out of time reviewing this one driver, and didn't look into
> the vexpress_config_bridge stuff. Seems like a pretty awkward interface but
> I didn't look closer.

Pawel commented on that code earlier.

> > +
> > +static const struct of_device_id vexpress_spc_ids[] __initconst = {
> > +     { .compatible = "arm,vexpress-spc,v2p-ca15_a7" },
> > +     { .compatible = "arm,vexpress-spc" },
> > +     {},
> > +};
> > +
> > +static int __init vexpress_spc_init(void)
> > +{
> > +     int ret;
> > +     struct device_node *node = of_find_matching_node(NULL,
> > +                                                      vexpress_spc_ids);
> > +
> > +     if (!node)
> > +             return -ENODEV;
> > +
> > +     info = kzalloc(sizeof(*info), GFP_KERNEL);
> > +     if (!info) {
> > +             pr_err("%s: unable to allocate mem\n", __func__);
> > +             return -ENOMEM;
> > +     }
> > +     info->cur_req_type = INVALID_TYPE;
> > +
> > +     info->baseaddr = of_iomap(node, 0);
> > +     if (WARN_ON(!info->baseaddr)) {
> 
> pr_err() something or other.

Yeah, added for all error conditions below.

> > +             ret = -ENXIO;
> > +             goto mem_free;
> > +     }
> > +
> > +     info->irq = irq_of_parse_and_map(node, 0);
> > +
> > +     if (WARN_ON(!info->irq)) {
> 
> pr_err() something or other.
> 
> > +             ret = -ENXIO;
> > +             goto unmap;
> > +     }
> > +
> > +     readl_relaxed(info->baseaddr + PWC_STATUS);
> > +
> > +     ret = request_irq(info->irq, vexpress_spc_irq_handler,
> > +             IRQF_DISABLED | IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
> > +             "arm-spc", info);
> > +
> > +     if (ret) {
> > +             pr_err("IRQ %d request failed\n", info->irq);
> > +             ret = -ENODEV;
> > +             goto unmap;
> > +     }
> > +
> > +     info->a15_clusid = readl_relaxed(info->baseaddr + A15_CONF) & 0xf;
> > +
> > +     vexpress_spc_config_bridge = vexpress_config_bridge_register(
> > +                     node, &vexpress_spc_config_bridge_info);
> > +
> > +     if (WARN_ON(!vexpress_spc_config_bridge)) {
> 
> pr_err()
> 
> > +             ret = -ENODEV;
> > +             goto unmap;
> > +     }
> > +
> > +     opp_func = vexpress_config_func_get(vexpress_spc_config_bridge, "opp");
> > +     perf_func =
> > +             vexpress_config_func_get(vexpress_spc_config_bridge, "perf");
> 
> Your identifiers are so long that you can't fit a simple call on minimum
> indentation without wrapping the line. That should be a pretty strong indicator
> that it needs to be shortened.

Ok, point taken.

> > +
> > +     if (!opp_func || !perf_func) {
> 
> ...pr_err()
> 
> > +             ret = -ENODEV;
> > +             goto unmap;
> > +     }
> > +
> > +     if (vexpress_spc_populate_opps(0) || vexpress_spc_populate_opps(1)) {
> > +             if (info->irq)
> > +                     free_irq(info->irq, info);
> > +             pr_err("failed to build OPP table\n");
> > +             ret = -ENODEV;
> > +             goto unmap;
> > +     }
> > +     /*
> > +      * Multi-cluster systems may need this data when non-coherent, during
> > +      * cluster power-up/power-down. Make sure it reaches main memory:
> > +      */
> > +     sync_cache_w(info);
> > +     sync_cache_w(&info);
> > +     pr_info("vexpress-spc loaded at %p\n", info->baseaddr);
> > +     return 0;
> > +
> > +unmap:
> > +     iounmap(info->baseaddr);
> > +
> > +mem_free:
> > +     kfree(info);
> > +     return ret;
> > +}
> > +
> > +static bool __init __vexpress_spc_check_loaded(void);
> > +/*
> > + * Pointer spc_check_loaded is swapped after init hence it is safe
> > + * to initialize it to a function in the __init section
> > + */
> > +static bool (*spc_check_loaded)(void) __refdata = &__vexpress_spc_check_loaded;
> > +
> > +static bool __init __vexpress_spc_check_loaded(void)
> > +{
> > +     if (vexpress_spc_load_result == -EAGAIN)
> > +             vexpress_spc_load_result = vexpress_spc_init();
> > +     spc_check_loaded = &vexpress_spc_initialized;
> > +     return vexpress_spc_initialized();
> > +}
> 
> Whoa. A "check_foo" function with side effects. Red flag.
> 
> So, you only want to try to run vexpress_spc_init() on the very first call to
> spc_check_loaded()? Just do so in vexpress_spc_initalized() instead and keep
> a static bool first_call = true; variable instead. Still confusing and with
> side effects though.

Well, I think the best thing to do is to call it eg spc_init(). That's what it
does anyway and there is no need for syntactic sugar that just hides side
effects.

> > +
> > +/*
> > + * Function exported to manage early_initcall ordering.
> > + * SPC code is needed very early in the boot process
> > + * to bring CPUs out of reset and initialize power
> > + * management back-end. After boot swap pointers to
> > + * make the functionality check available to loadable
> > + * modules, when early boot init functions have been
> > + * already freed from kernel address space.
> > + */
> > +bool vexpress_spc_check_loaded(void)
> > +{
> > +     return spc_check_loaded();
> > +}
> > +EXPORT_SYMBOL_GPL(vexpress_spc_check_loaded);
> 
> Or, you do that test I mentioned right above here instead.

Commented above.

Thank you very much for the review.
Lorenzo

Re: [RFC PATCH v3 0/2] drivers: mfd: Versatile Express SPC support

[Lorenzo Pieralisi]

Hi Samuel,

if nobody has objections I think this set is ready to get merged. As
Nico mentioned in:

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

since we would like to get it merged through the ARM SoC tree owing to
dependencies between this code and ARM power management back-ends, your ack
would be appreciated, if you think it is worth it of course.

Thank you very much indeed,
Lorenzo

On Thu, Jun 06, 2013 at 10:59:21AM +0100, Lorenzo Pieralisi wrote:
> This patch is v3 of a previous posting:
> 
> http://lists.infradead.org/pipermail/linux-arm-kernel/2013-June/173464.html
> 
> 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
> 
> This patch series introduces support for the Versatile Express Serial
> Power Controller (SPC) present in ARM Versatile Express TC2 core tiles.
> SPC driver is a fundamental component of TC2 power management and allows
> to carry out C-state management and DVFS for A15 and A7 clusters.
> 
> First patch provides changes required by SPC to comply with the
> Versatile Express config API, second patch is the SPC driver implementation.
> 
> Code extensively exercised through CPUidle and CPUfreq power states and
> operating point transitions.
> 
> Lorenzo Pieralisi (1):
>   drivers: mfd: vexpress: add Serial Power Controller (SPC) support
> 
> Pawel Moll (1):
>   drivers: mfd: refactor the vexpress config bridge API
> 
>  Documentation/devicetree/bindings/mfd/vexpress-spc.txt |  35 +
>  drivers/mfd/Kconfig                                    |   7 +
>  drivers/mfd/Makefile                                   |   1 +
>  drivers/mfd/vexpress-config.c                          |  61 +-
>  drivers/mfd/vexpress-spc.c                             | 633 ++++++++++
>  drivers/mfd/vexpress-sysreg.c                          |   2 +-
>  include/linux/vexpress.h                               |  59 +-
>  7 files changed, 770 insertions(+), 28 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/mfd/vexpress-spc.txt
>  create mode 100644 drivers/mfd/vexpress-spc.c
> 
> -- 
> 1.8.2.2
> 

Re: [RFC PATCH v3 0/2] drivers: mfd: Versatile Express SPC support

[Samuel Ortiz]

Hi Lorenzo,

On Tue, Jun 11, 2013 at 10:05:45AM +0100, Lorenzo Pieralisi wrote:
> Hi Samuel,
> 
> if nobody has objections I think this set is ready to get merged. As
> Nico mentioned in:
> 
> http://lists.infradead.org/pipermail/linux-arm-kernel/2013-June/173541.html
> 
> since we would like to get it merged through the ARM SoC tree owing to
> dependencies between this code and ARM power management back-ends, your ack
> would be appreciated, if you think it is worth it of course.
I'm fine with it going through the arm-soc tree, but please prepare a
branch for me to pull from.
AFAIR, we got a merge conflict during the last merge window with the
vexpress driver, I want to avoid that for the next one.

Now, about the driver itself, besides the really odd code design, the
static variables all over the place, the nasty init hacks and the
unneeded long function names, someone should refresh my memory and explain
to me why is this guy under mfd. I can see it somehow supports IP blocks
providing different functions, but the design is not sharing anything with
most of the rest of the mfd drivers.
Not only that, but the whole vexpress-config code design is not the
nicest piece of code I've ever seen. And I'm usually not picky. e.g. the
whole vexpress-config ad-hoc API is awkward and I wonder if it could be
implemented as a bus instead.

FWIW I take the blame here for not reviewing the initial driver
submission that Arnd kindly sent to me...But now that I'm looking at it,
I think it really is on the edge of being staging material. Any thought
on that ?

Cheers,
Samuel.

> Thank you very much indeed,
> Lorenzo
> 
> On Thu, Jun 06, 2013 at 10:59:21AM +0100, Lorenzo Pieralisi wrote:
> > This patch is v3 of a previous posting:
> > 
> > http://lists.infradead.org/pipermail/linux-arm-kernel/2013-June/173464.html
> > 
> > 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
> > 
> > This patch series introduces support for the Versatile Express Serial
> > Power Controller (SPC) present in ARM Versatile Express TC2 core tiles.
> > SPC driver is a fundamental component of TC2 power management and allows
> > to carry out C-state management and DVFS for A15 and A7 clusters.
> > 
> > First patch provides changes required by SPC to comply with the
> > Versatile Express config API, second patch is the SPC driver implementation.
> > 
> > Code extensively exercised through CPUidle and CPUfreq power states and
> > operating point transitions.
> > 
> > Lorenzo Pieralisi (1):
> >   drivers: mfd: vexpress: add Serial Power Controller (SPC) support
> > 
> > Pawel Moll (1):
> >   drivers: mfd: refactor the vexpress config bridge API
> > 
> >  Documentation/devicetree/bindings/mfd/vexpress-spc.txt |  35 +
> >  drivers/mfd/Kconfig                                    |   7 +
> >  drivers/mfd/Makefile                                   |   1 +
> >  drivers/mfd/vexpress-config.c                          |  61 +-
> >  drivers/mfd/vexpress-spc.c                             | 633 ++++++++++
> >  drivers/mfd/vexpress-sysreg.c                          |   2 +-
> >  include/linux/vexpress.h                               |  59 +-
> >  7 files changed, 770 insertions(+), 28 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/mfd/vexpress-spc.txt
> >  create mode 100644 drivers/mfd/vexpress-spc.c
> > 
> > -- 
> > 1.8.2.2
> > 
> 

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

Re: [RFC PATCH v3 0/2] drivers: mfd: Versatile Express SPC support

[Pawel Moll]

On Thu, 2013-06-13 at 01:13 +0100, Samuel Ortiz wrote:
> Now, about the driver itself, besides the really odd code design, the
> static variables all over the place, the nasty init hacks and the
> unneeded long function names, someone should refresh my memory and explain
> to me why is this guy under mfd. I can see it somehow supports IP blocks
> providing different functions, but the design is not sharing anything with
> most of the rest of the mfd drivers.

I belive the vexpress-sysreg.c is a Multi Function Device by all means.
It does so many things that only a water fountain is missing ;-)

If you feel strongly about it, I'm ready to split it into mfd_cells and
move the gpio and leds code into separate drivers, however I'm not
convinced that it's worth the effort.

Now, as to the vexpress-config.c... The first time I've posted the
series, all parts lived in "driver/misc(/vexpress)", but (if I remember
correctly) Arnd had some feelings about "misc" existence at all... I was
thinking about a separate directory for random "system/platform/machine
configuration" drivers, but the idea didn't get any traction.

> Not only that, but the whole vexpress-config code design is not the
> nicest piece of code I've ever seen. And I'm usually not picky. e.g. the
> whole vexpress-config ad-hoc API is awkward and I wonder if it could be
> implemented as a bus instead.

Funny you mention this :-) Again, the first version actually was a
vexpress-config bus. The feedback was - a whole bus_type is over the top
(I'm simplifying the letter slightly but this was the spirit).

> FWIW I take the blame here for not reviewing the initial driver
> submission that Arnd kindly sent to me...But now that I'm looking at it,
> I think it really is on the edge of being staging material. Any thought
> on that ?

I'm more than happy to improve it. The infrastructure (as in: the
hardware) itself is slightly strange and the code pretty much reflects
the situation. There is also a very good reason for some of the oddities
like static bridges array etc - the infrastructure must be functional
very early, long before slab is available (this also caused a lot of
issues with the bus-based implementation, as the device model does
kmalloc all over the place).

So to summarize - I'm open to any suggestions and ready to spend time on
this stuff.

Regards and thanks for your time!

Pawel

Re: [RFC PATCH v3 0/2] drivers: mfd: Versatile Express SPC support

[Samuel Ortiz]

Hi Pawell,

On Thu, Jun 13, 2013 at 10:45:57AM +0100, Pawel Moll wrote:
> On Thu, 2013-06-13 at 01:13 +0100, Samuel Ortiz wrote:
> > Now, about the driver itself, besides the really odd code design, the
> > static variables all over the place, the nasty init hacks and the
> > unneeded long function names, someone should refresh my memory and explain
> > to me why is this guy under mfd. I can see it somehow supports IP blocks
> > providing different functions, but the design is not sharing anything with
> > most of the rest of the mfd drivers.
> 
> I belive the vexpress-sysreg.c is a Multi Function Device by all means.
> It does so many things that only a water fountain is missing ;-)
> 
> If you feel strongly about it, I'm ready to split it into mfd_cells and
> move the gpio and leds code into separate drivers, however I'm not
> convinced that it's worth the effort.
Well, after seeing your last patch for ifdef'ing the GPIO and LED code,
I think it is worth the effort.

> Now, as to the vexpress-config.c... The first time I've posted the
> series, all parts lived in "driver/misc(/vexpress)", but (if I remember
> correctly) Arnd had some feelings about "misc" existence at all... I was
> thinking about a separate directory for random "system/platform/machine
> configuration" drivers, but the idea didn't get any traction.
drivers/misc would already have been a nicer option imo.

 
> > Not only that, but the whole vexpress-config code design is not the
> > nicest piece of code I've ever seen. And I'm usually not picky. e.g. the
> > whole vexpress-config ad-hoc API is awkward and I wonder if it could be
> > implemented as a bus instead.
> 
> Funny you mention this :-) Again, the first version actually was a
> vexpress-config bus. The feedback was - a whole bus_type is over the top
> (I'm simplifying the letter slightly but this was the spirit).
I think it would make sense to have it under drivers/bus/. It might be a
little over the top, but when I look at the current code I'd be really
happy to read an over-the-top bus driver instead. At least we'd know
straight away what youre trying to achieve with this code and it would
probably remove a fair chunk of the weird bridge API (the registering
and the function reference stuff).
Do you have a reference for the patch first version ?


> > FWIW I take the blame here for not reviewing the initial driver
> > submission that Arnd kindly sent to me...But now that I'm looking at it,
> > I think it really is on the edge of being staging material. Any thought
> > on that ?
> 
> I'm more than happy to improve it. The infrastructure (as in: the
> hardware) itself is slightly strange and the code pretty much reflects
> the situation. There is also a very good reason for some of the oddities
> like static bridges array etc - the infrastructure must be functional
> very early, long before slab is available (this also caused a lot of
> issues with the bus-based implementation, as the device model does
> kmalloc all over the place).
> 
> So to summarize - I'm open to any suggestions and ready to spend time on
> this stuff.
I'd say splitting the sysreg driver and leaving only the MFD bits in the
MFD driver would be a first step.
Also, re-considering the bus implementation for the config part would
also be interesting. I'd be interested in looking at your first version
of the patch.


> Regards and thanks for your time!
Thanks for your understanding.

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

Re: [RFC PATCH v3 0/2] drivers: mfd: Versatile Express SPC support

[Pawel Moll]

Morning, Samuel,

On Tue, 2013-06-18 at 10:09 +0100, Samuel Ortiz wrote:
> Hi Pawell,

Double l in the wrong place ;-)

> > If you feel strongly about it, I'm ready to split it into mfd_cells and
> > move the gpio and leds code into separate drivers, however I'm not
> > convinced that it's worth the effort.
> Well, after seeing your last patch for ifdef'ing the GPIO and LED code,
> I think it is worth the effort.

Good point. But as this - obviously - won't happen on time for 3.11, I
hope you would be kind enough to take the #ifdef patch in for now.

> > Now, as to the vexpress-config.c... The first time I've posted the
> > series, all parts lived in "driver/misc(/vexpress)", but (if I remember
> > correctly) Arnd had some feelings about "misc" existence at all... I was
> > thinking about a separate directory for random "system/platform/machine
> > configuration" drivers, but the idea didn't get any traction.
> drivers/misc would already have been a nicer option imo.

Ok. Quite conveniently Arnd is the driver/misc maintainer so I'll get
first-hand feedback on this.

> > > Not only that, but the whole vexpress-config code design is not the
> > > nicest piece of code I've ever seen. And I'm usually not picky. e.g. the
> > > whole vexpress-config ad-hoc API is awkward and I wonder if it could be
> > > implemented as a bus instead.
> > 
> > Funny you mention this :-) Again, the first version actually was a
> > vexpress-config bus. The feedback was - a whole bus_type is over the top
> > (I'm simplifying the letter slightly but this was the spirit).
> I think it would make sense to have it under drivers/bus/. It might be a
> little over the top, but when I look at the current code I'd be really
> happy to read an over-the-top bus driver instead. At least we'd know
> straight away what youre trying to achieve with this code and it would
> probably remove a fair chunk of the weird bridge API (the registering
> and the function reference stuff).
> Do you have a reference for the patch first version ?

http://thread.gmane.org/gmane.linux.ports.arm.kernel/185014/focus=185019

> > So to summarize - I'm open to any suggestions and ready to spend time on
> > this stuff.
> I'd say splitting the sysreg driver and leaving only the MFD bits in the
> MFD driver would be a first step.
> Also, re-considering the bus implementation for the config part would
> also be interesting. 

Ok, so what I'll do:

1. Split vexpress-sysreg into
* gpio driver
* leds driver
* the rest (still in mfd though)

2. Move the vexpress-sysreg "platform management" functions into misc
(unless we get any better place for it)

3. Move vexpress-config into drivers/bus as it is (however I see no one
in MAINTAINERS for this directory)

4. *Try* to use more of the standard bus (aka bus_type) infrastructure,
however this will be the trickiest part of this all - as I've mentioned
the code must be functional before SLAB is up...

You shall see some patches before 3.11-rc1.

Paweł

Re: [RFC PATCH v3 0/2] drivers: mfd: Versatile Express SPC support

[Samuel Ortiz]

Hi Pawel,

On Tue, Jun 18, 2013 at 10:29:42AM +0100, Pawel Moll wrote:
> On Tue, 2013-06-18 at 10:09 +0100, Samuel Ortiz wrote:
> > Hi Pawell,
> 
> Double l in the wrong place ;-)
Apologies...

 
> > > If you feel strongly about it, I'm ready to split it into mfd_cells and
> > > move the gpio and leds code into separate drivers, however I'm not
> > > convinced that it's worth the effort.
> > Well, after seeing your last patch for ifdef'ing the GPIO and LED code,
> > I think it is worth the effort.
> 
> Good point. But as this - obviously - won't happen on time for 3.11, I
> hope you would be kind enough to take the #ifdef patch in for now.
I see that you guys are willing to improve this stuff, so I can take it,
yes.


> > > Now, as to the vexpress-config.c... The first time I've posted the
> > > series, all parts lived in "driver/misc(/vexpress)", but (if I remember
> > > correctly) Arnd had some feelings about "misc" existence at all... I was
> > > thinking about a separate directory for random "system/platform/machine
> > > configuration" drivers, but the idea didn't get any traction.
> > drivers/misc would already have been a nicer option imo.
> 
> Ok. Quite conveniently Arnd is the driver/misc maintainer so I'll get
> first-hand feedback on this.
> 
> > > > Not only that, but the whole vexpress-config code design is not the
> > > > nicest piece of code I've ever seen. And I'm usually not picky. e.g. the
> > > > whole vexpress-config ad-hoc API is awkward and I wonder if it could be
> > > > implemented as a bus instead.
> > > 
> > > Funny you mention this :-) Again, the first version actually was a
> > > vexpress-config bus. The feedback was - a whole bus_type is over the top
> > > (I'm simplifying the letter slightly but this was the spirit).
> > I think it would make sense to have it under drivers/bus/. It might be a
> > little over the top, but when I look at the current code I'd be really
> > happy to read an over-the-top bus driver instead. At least we'd know
> > straight away what youre trying to achieve with this code and it would
> > probably remove a fair chunk of the weird bridge API (the registering
> > and the function reference stuff).
> > Do you have a reference for the patch first version ?
> 
> http://thread.gmane.org/gmane.linux.ports.arm.kernel/185014/focus=185019


 
> > > So to summarize - I'm open to any suggestions and ready to spend time on
> > > this stuff.
> > I'd say splitting the sysreg driver and leaving only the MFD bits in the
> > MFD driver would be a first step.
> > Also, re-considering the bus implementation for the config part would
> > also be interesting. 
> 
> Ok, so what I'll do:
> 
> 1. Split vexpress-sysreg into
> * gpio driver
> * leds driver
> * the rest (still in mfd though)
Sounds good to me.


> 2. Move the vexpress-sysreg "platform management" functions into misc
> (unless we get any better place for it)
This is for Arnd and Greg to decide I suppose.


> 3. Move vexpress-config into drivers/bus as it is (however I see no one
> in MAINTAINERS for this directory)
ISTR that Arnd originally created that directory, so he may help here.
Arnd also had some concerns about implementing this code as a bus,
mostly about it not being a discoverable bus. IMHO that's a valid
concern, and this is why you ended up putting it under MFD which can be
seen as some sort of platform devices bus. But I still believe the bus
API would make this code look cleaner and easier to maintain.

> 4. *Try* to use more of the standard bus (aka bus_type) infrastructure,
> however this will be the trickiest part of this all - as I've mentioned
> the code must be functional before SLAB is up...
> 
> You shall see some patches before 3.11-rc1.
Ok, we'll have plenty of time to have it ready for the 3.12 merge window
then.

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

Re: [RFC PATCH v3 0/2] drivers: mfd: Versatile Express SPC support

[Arnd Bergmann]

On Wednesday 19 June 2013, Samuel Ortiz wrote:
> > 2. Move the vexpress-sysreg "platform management" functions into misc
> > (unless we get any better place for it)
> This is for Arnd and Greg to decide I suppose.


I think when vexpress-sysreg was created, we didn't have the syscon driver
yet, otherwise I think we should have used that, and put separate
drivers on top.

Not sure if it's too late for changing it to that now, given that
we already have a binding.

It seems we should use the same code for versatile and realview, or
at least it will only need small modifications to apply to all of
these platforms.

What I think could be helpful here is:

* export a "syscon" for the low-level registers

* add a gpio driver based on the syscon interface, and move the gpiochip
  implementation there

* move the low-leve "config" code from the sysreg driver into the
  vexpress-config driver and make it use the syscon.

* move the other global functions from the driver into the callers
  and use syscon there.

That would end up eliminating the sysreg driver, aside from maybe
a one-line change to the syscon driver to allow it to probe the
right device.

> > 3. Move vexpress-config into drivers/bus as it is (however I see no one
> > in MAINTAINERS for this directory)
> ISTR that Arnd originally created that directory, so he may help here.
> Arnd also had some concerns about implementing this code as a bus,
> mostly about it not being a discoverable bus. IMHO that's a valid
> concern, and this is why you ended up putting it under MFD which can be
> seen as some sort of platform devices bus. But I still believe the bus
> API would make this code look cleaner and easier to maintain.

Sorry, I don't see why it would be a bus. I assume that there is code
missing somewhere that is not yet merged, right?

	Arnd

Re: [RFC PATCH v3 0/2] drivers: mfd: Versatile Express SPC support

[Pawel Moll]

On Wed, 2013-06-19 at 13:37 +0100, Arnd Bergmann wrote:
> I think when vexpress-sysreg was created, we didn't have the syscon driver
> yet, otherwise I think we should have used that, and put separate
> drivers on top.
> 
> Not sure if it's too late for changing it to that now, given that
> we already have a binding.

I will have a look try to use the syscon stuff for generic configuration
bits and pieces...

> That would end up eliminating the sysreg driver, aside from maybe
> a one-line change to the syscon driver to allow it to probe the
> right device.

... but sysreg does more than just that. In particular it provides a
pseudo-gpio controller (I don't think you want to hide this behind the
syscon) *and* it can act as a bridge to the configuration bus - see
below. In short - no, I don't think sysreg driver can disappear. It can
be reduced in size, yes.

> > > 3. Move vexpress-config into drivers/bus as it is (however I see no one
> > > in MAINTAINERS for this directory)
> > ISTR that Arnd originally created that directory, so he may help here.
> > Arnd also had some concerns about implementing this code as a bus,
> > mostly about it not being a discoverable bus. IMHO that's a valid
> > concern, and this is why you ended up putting it under MFD which can be
> > seen as some sort of platform devices bus. But I still believe the bus
> > API would make this code look cleaner and easier to maintain.
> 
> Sorry, I don't see why it would be a bus. I assume that there is code
> missing somewhere that is not yet merged, right?

Well, different VE components (configuration microcontrollers on
motherboard and daughterboards in particular) talk to each other over a
bus (an SPI derivative, in case you were wondering). So there is a bus.
A non-discoverable one, but it does 42 (approximately ;-) different
things. We already have: clk, hwmon, regulator and reset drivers using
it.

And, to make things more complicated, the SPC in question, can act as a
bridge to *some* of the functions as well. What's a difference? About
190ms, in at least one case - accessing the energy monitor data (hwmon)
can take up to 200ms going through sysreg and about 10ms going through
SPC. And there are people interesting in getting this numbers as often
as possible. But (obviously, to make things even more complex() only the
daughterboard's components can be accessed through it. Eg. the
motherboard clock generators must still be accessed through sysregs.
Hope you see why the problem is not trivial.

Paweł

Re: [RFC PATCH v3 0/2] drivers: mfd: Versatile Express SPC support

[Arnd Bergmann]

On Wednesday 19 June 2013, Pawel Moll wrote:
> > That would end up eliminating the sysreg driver, aside from maybe
> > a one-line change to the syscon driver to allow it to probe the
> > right device.
> 
> ... but sysreg does more than just that. In particular it provides a
> pseudo-gpio controller (I don't think you want to hide this behind the
> syscon) and it can act as a bridge to the configuration bus - see
> below. In short - no, I don't think sysreg driver can disappear. It can
> be reduced in size, yes.

As I said, the gpio part can be a separate driver that just handles
gpio, and I think the configuration bridge can be part of the
vexpress-config driver, building on top of syscon. I'm not completely
sure about the latter part.

> > > > 3. Move vexpress-config into drivers/bus as it is (however I see no one
> > > > in MAINTAINERS for this directory)
> > > ISTR that Arnd originally created that directory, so he may help here.
> > > Arnd also had some concerns about implementing this code as a bus,
> > > mostly about it not being a discoverable bus. IMHO that's a valid
> > > concern, and this is why you ended up putting it under MFD which can be
> > > seen as some sort of platform devices bus. But I still believe the bus
> > > API would make this code look cleaner and easier to maintain.
> > 
> > Sorry, I don't see why it would be a bus. I assume that there is code
> > missing somewhere that is not yet merged, right?
> 
> Well, different VE components (configuration microcontrollers on
> motherboard and daughterboards in particular) talk to each other over a
> bus (an SPI derivative, in case you were wondering). So there is a bus.
> A non-discoverable one, but it does 42 (approximately ;-) different
> things. We already have: clk, hwmon, regulator and reset drivers using
> it.

Ah, got it. In this case I think what you are looking for is a custom
'regmap' interface that abstracts those devices. Regmap can already
cover i2c, spi and mmio based sets of registers (syscon is one example
for mmio), and IIRC there is a simple way of extending it to other
register-level interfaces like this one.

> And, to make things more complicated, the SPC in question, can act as a
> bridge to some of the functions as well. What's a difference? About
> 190ms, in at least one case - accessing the energy monitor data (hwmon)
> can take up to 200ms going through sysreg and about 10ms going through
> SPC. And there are people interesting in getting this numbers as often
> as possible. But (obviously, to make things even more complex() only the
> daughterboard's components can be accessed through it. Eg. the
> motherboard clock generators must still be accessed through sysregs.
> Hope you see why the problem is not trivial.

Yes, it definitely needs some detailed analysis, but I think regmap is
a good fit to simplify this code. Please have a look at that and tell
me if you see problems with it.

	Arnd

Re: [RFC PATCH v3 0/2] drivers: mfd: Versatile Express SPC support

[Pawel Moll]

On Wed, 2013-06-19 at 16:03 +0100, Arnd Bergmann wrote:
> On Wednesday 19 June 2013, Pawel Moll wrote:
> > ... but sysreg does more than just that. In particular it provides a
> > pseudo-gpio controller (I don't think you want to hide this behind the
> > syscon) and it can act as a bridge to the configuration bus - see
> > below. In short - no, I don't think sysreg driver can disappear. It can
> > be reduced in size, yes.
> 
> As I said, the gpio part can be a separate driver that just handles
> gpio, 

I've already promised Samuel that.

> and I think the configuration bridge can be part of the
> vexpress-config driver, building on top of syscon. I'm not completely
> sure about the latter part.

I don't think so, no. I appreciate what you are trying to say, but I
object to mixing ideas for the sake of reducing number of files. The
config infrastructure is completely independent from the sysreg. Each of
them can exist on its own. The only common thing is a (slightly baroque)
communication interface between those two.

> > > > > 3. Move vexpress-config into drivers/bus as it is (however I see no one
> > > > > in MAINTAINERS for this directory)
> > > > ISTR that Arnd originally created that directory, so he may help here.
> > > > Arnd also had some concerns about implementing this code as a bus,
> > > > mostly about it not being a discoverable bus. IMHO that's a valid
> > > > concern, and this is why you ended up putting it under MFD which can be
> > > > seen as some sort of platform devices bus. But I still believe the bus
> > > > API would make this code look cleaner and easier to maintain.
> > > 
> > > Sorry, I don't see why it would be a bus. I assume that there is code
> > > missing somewhere that is not yet merged, right?
> > 
> > Well, different VE components (configuration microcontrollers on
> > motherboard and daughterboards in particular) talk to each other over a
> > bus (an SPI derivative, in case you were wondering). So there is a bus.
> > A non-discoverable one, but it does 42 (approximately ;-) different
> > things. We already have: clk, hwmon, regulator and reset drivers using
> > it.
> 
> Ah, got it. In this case I think what you are looking for is a custom
> 'regmap' interface that abstracts those devices. Regmap can already
> cover i2c, spi and mmio based sets of registers (syscon is one example
> for mmio), and IIRC there is a simple way of extending it to other
> register-level interfaces like this one.

This is an interesting idea. The thing that worries me the most is its
early (pre-slab) availability, but I will give it a try.

Paweł