* [RFC PATCH v5 0/1] drivers: mfd: Versatile Express SPC support @ 2013-07-16 16:05 Lorenzo Pieralisi 2013-07-16 16:05 ` [RFC PATCH v5 1/1] drivers: mfd: vexpress: add Serial Power Controller (SPC) support Lorenzo Pieralisi ` (3 more replies) 0 siblings, 4 replies; 20+ messages in thread From: Lorenzo Pieralisi @ 2013-07-16 16:05 UTC (permalink / raw) To: linux-kernel, linux-arm-kernel, devicetree-discuss Cc: Lorenzo Pieralisi, Samuel Ortiz, Olof Johansson, Pawel Moll, Nicolas Pitre, Amit Kucheria, Jon Medhurst, Achin Gupta, Sudeep KarkadaNagesha Hello, version v5 of VExpress SPC driver, please read on the changelog for major changes and explanations. The probing scheme is unchanged, since after trying the early platform devices approach it appeared that the end result was no better than the current one. The only clean solution relies either on changing how secondaries are brought up in the kernel (later than now) or enable early platform device registration through DT. Please check this thread for the related discussion: https://lists.ozlabs.org/pipermail/devicetree-discuss/2013-June/036542.html The interface was adapted to regmap and again reverted to old driver for the following reasons: - Power down registers locking is hairy and requires arch spinlocks in the MCPM back end to work properly, normal spinlocks cannot be used - Regmap adds unnecessary code to manage SPC since it is just a bunch of registers used to control power management flags, the overhead is just not worth it (talking about power down registers, not the vexpress config interface) - The locking scheme behind regmap requires all registers in the map to be protected with the same lock, which is not exactly what we want here - Given the reasons above, adding a regmap interface buys us nothing from a driver readability and maintainability perspective (again just talking about the power interface, a few registers) because for the SPC it would simply not be used /drivers/mfd is probably not the right place for this code as it stands (but probably will be when the entire driver, with DVFS and config interface, is complete). Thank you for the review in advance, Lorenzo This patch is v5 of a previous posting: http://lists.infradead.org/pipermail/linux-arm-kernel/2013-June/177150.html v5 changes: - Completely removed vexpress-config interface waiting for refactoring based on regmap - Removed frequency scaling interface and operating points programming retrieval - Trimmed the driver code and DT bindings v4 changes: - Applied review comments (trimmed function names, added comments, refactored some APIs) - Added comments throughout the set - Fixed irq handler bug in checking the transaction status - Improved commit log to explain early init synchro scheme - Created a single static structure for variables dynamically allocated to remove usage of static - Improved Kconfig entry v3 changes: - added __refdata to spc_check_loaded pointer - removed some exported symbols - added node pointer check in vexpress_spc_init() v2 changes: - Dropped timeout interface patch - Converted interfaces to non-timeout ones, integrated and retested - Removed mutex used at init - Refactored code to work around init sections warning - Fixed two minor bugs Lorenzo Pieralisi (1): drivers: mfd: vexpress: add Serial Power Controller (SPC) support Documentation/devicetree/bindings/mfd/vexpress-spc.txt | 36 ++ drivers/mfd/Kconfig | 10 + drivers/mfd/Makefile | 1 + drivers/mfd/vexpress-spc.c | 253 ++++++++++ include/linux/vexpress.h | 17 + 5 files changed, 317 insertions(+) create mode 100644 Documentation/devicetree/bindings/mfd/vexpress-spc.txt create mode 100644 drivers/mfd/vexpress-spc.c -- 1.8.2.2 ^ permalink raw reply [flat|nested] 20+ messages in thread
* [RFC PATCH v5 1/1] drivers: mfd: vexpress: add Serial Power Controller (SPC) support 2013-07-16 16:05 [RFC PATCH v5 0/1] drivers: mfd: Versatile Express SPC support Lorenzo Pieralisi @ 2013-07-16 16:05 ` Lorenzo Pieralisi 2013-07-16 20:05 ` Rob Herring 2013-07-17 3:26 ` [RFC PATCH v5 0/1] drivers: mfd: Versatile Express SPC support Nicolas Pitre ` (2 subsequent siblings) 3 siblings, 1 reply; 20+ messages in thread From: Lorenzo Pieralisi @ 2013-07-16 16:05 UTC (permalink / raw) To: linux-kernel, linux-arm-kernel, devicetree-discuss Cc: Lorenzo Pieralisi, Samuel Ortiz, Olof Johansson, Pawel Moll, Amit Kucheria, Jon Medhurst, Achin Gupta, Sudeep KarkadaNagesha, Nicolas Pitre The TC2 versatile express core tile integrates a logic block that provides the interface between the dual cluster test-chip and the M3 microcontroller that carries out power management. The logic block, called Serial Power Controller (SPC), contains several memory mapped registers to control among other things low-power states, wake-up irqs and per-CPU jump addresses registers. This patch provides a driver that enables run-time control of features implemented by the SPC power management control logic. The SPC control logic is required to be programmed very early in the boot process to reset secondary CPUs on the TC2 testchip, set-up jump addresses and wake-up IRQs for power management. Hence, waiting for core changes to be made in the device core code to enable early registration of platform devices, the driver puts in place an early init scheme that allows kernel drivers to initialize the SPC driver directly from the components requiring it, if their initialization routine is called before the driver init function by the boot process. Device tree bindings documentation for the SPC component is provided with the patchset. Cc: Samuel Ortiz <sameo@linux.intel.com> Cc: Olof Johansson <olof@lixom.net> Cc: Pawel Moll <pawel.moll@arm.com> 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> --- Documentation/devicetree/bindings/mfd/vexpress-spc.txt | 36 ++ drivers/mfd/Kconfig | 10 + drivers/mfd/Makefile | 1 + drivers/mfd/vexpress-spc.c | 253 ++++++++++ include/linux/vexpress.h | 17 + 5 files changed, 317 insertions(+) diff --git a/Documentation/devicetree/bindings/mfd/vexpress-spc.txt b/Documentation/devicetree/bindings/mfd/vexpress-spc.txt new file mode 100644 index 0000000..1614725 --- /dev/null +++ b/Documentation/devicetree/bindings/mfd/vexpress-spc.txt @@ -0,0 +1,36 @@ +* ARM Versatile Express Serial Power Controller device tree bindings + +Latest ARM development boards implement a power management interface (serial +power controller - SPC) that is capable of managing power states transitions, +wake-up IRQs and resume addresses for ARM multiprocessor testchips. +The serial controller can be programmed through a memory mapped interface +that enables communication between firmware running on the microcontroller +managing power states and the application processors. + +The SPC DT bindings are defined as follows: + +- spc node + + - compatible: + Usage: required + Value type: <stringlist> + Definition: must be + "arm,vexpress-spc,v2p-ca15_a7", "arm,vexpress-spc" + - reg: + Usage: required + Value type: <prop-encode-array> + Definition: A standard property that specifies the base address + and the size of the SPC address space + - interrupts: + Usage: required + Value type: <prop-encoded-array> + Definition: SPC interrupt configuration. A standard property + that follows ePAPR interrupts specifications + +Example: + +spc: spc@7fff0000 { + compatible = "arm,vexpress-spc,v2p-ca15_a7", "arm,vexpress-spc"; + reg = <0x7fff0000 0x1000>; + interrupts = <0 95 4>; +}; diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index 6959b8d..ebd23f4 100644 --- a/drivers/mfd/Kconfig +++ b/drivers/mfd/Kconfig @@ -1149,3 +1149,13 @@ config VEXPRESS_CONFIG help Platform configuration infrastructure for the ARM Ltd. Versatile Express. + +config VEXPRESS_SPC + bool "Versatile Express SPC driver support" + depends on ARM + help + The Serial Power Controller (SPC) for ARM Ltd. test chips, is + an IP that provides a memory mapped interface to power controller + HW. The driver provides an API abstraction allowing to program + registers controlling low-level power management features like power + down flags, global and per-cpu wake-up IRQs. diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile index 718e94a..3a01203 100644 --- a/drivers/mfd/Makefile +++ b/drivers/mfd/Makefile @@ -153,5 +153,6 @@ obj-$(CONFIG_MFD_SEC_CORE) += sec-core.o sec-irq.o obj-$(CONFIG_MFD_SYSCON) += syscon.o obj-$(CONFIG_MFD_LM3533) += lm3533-core.o lm3533-ctrlbank.o obj-$(CONFIG_VEXPRESS_CONFIG) += vexpress-config.o vexpress-sysreg.o +obj-$(CONFIG_VEXPRESS_SPC) += vexpress-spc.o obj-$(CONFIG_MFD_RETU) += retu-mfd.o obj-$(CONFIG_MFD_AS3711) += as3711.o diff --git a/drivers/mfd/vexpress-spc.c b/drivers/mfd/vexpress-spc.c new file mode 100644 index 0000000..aa8c2a4 --- /dev/null +++ b/drivers/mfd/vexpress-spc.c @@ -0,0 +1,253 @@ +/* + * Versatile Express Serial Power Controller (SPC) support + * + * Copyright (C) 2013 ARM Ltd. + * + * Authors: Sudeep KarkadaNagesha <sudeep.karkadanagesha@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/of_address.h> +#include <linux/slab.h> +#include <linux/vexpress.h> + +#include <asm/cacheflush.h> + +#define SPCLOG "vexpress-spc: " + +/* SCC conf registers */ +#define A15_CONF 0x400 +#define SYS_INFO 0x700 + +/* SPC registers base */ +#define SPC_BASE 0xB00 + +/* SPC wake-up IRQs status and mask */ +#define WAKE_INT_MASK (SPC_BASE + 0x24) +#define WAKE_INT_RAW (SPC_BASE + 0x28) +#define WAKE_INT_STAT (SPC_BASE + 0x2c) +/* SPC power down registers */ +#define A15_PWRDN_EN (SPC_BASE + 0x30) +#define A7_PWRDN_EN (SPC_BASE + 0x34) +/* SPC per-CPU mailboxes */ +#define A15_BX_ADDR0 (SPC_BASE + 0x68) +#define A7_BX_ADDR0 (SPC_BASE + 0x78) + +/* wake-up interrupt masks */ +#define GBL_WAKEUP_INT_MSK (0x3 << 10) + +/* TC2 static dual-cluster configuration */ +#define MAX_CLUSTERS 2 + +struct ve_spc_drvdata { + void __iomem *baseaddr; + /* + * A15s cluster identifier + * It corresponds to A15 processors MPIDR[15:8] bitfield + */ + u32 a15_clusid; +}; + +static struct ve_spc_drvdata *info; + +static inline bool cluster_is_a15(u32 cluster) +{ + return cluster == info->a15_clusid; +} + +/** + * ve_spc_global_wakeup_irq() + * + * Function to set/clear global wakeup IRQs. Not protected by locking since + * it might be used in code paths where normal cacheable locks are not + * working. Locking must be provided by the caller to ensure atomicity. + * + * @set: if true, global wake-up IRQs are set, if false they are cleared + */ +void ve_spc_global_wakeup_irq(bool set) +{ + u32 reg; + + reg = readl_relaxed(info->baseaddr + WAKE_INT_MASK); + + if (set) + reg |= GBL_WAKEUP_INT_MSK; + else + reg &= ~GBL_WAKEUP_INT_MSK; + + writel_relaxed(reg, info->baseaddr + WAKE_INT_MASK); +} + +/** + * ve_spc_cpu_wakeup_irq() + * + * Function to set/clear per-CPU wake-up IRQs. Not protected by locking since + * it might be used in code paths where normal cacheable locks are not + * working. Locking must be provided by the caller to ensure atomicity. + * + * @cluster: mpidr[15:8] bitfield describing cluster affinity level + * @cpu: mpidr[7:0] bitfield describing cpu affinity level + * @set: if true, wake-up IRQs are set, if false they are cleared + */ +void ve_spc_cpu_wakeup_irq(u32 cluster, u32 cpu, bool set) +{ + u32 mask, reg; + + if (cluster >= MAX_CLUSTERS) + return; + + mask = 1 << cpu; + + if (!cluster_is_a15(cluster)) + mask <<= 4; + + reg = readl_relaxed(info->baseaddr + WAKE_INT_MASK); + + if (set) + reg |= mask; + else + reg &= ~mask; + + writel_relaxed(reg, info->baseaddr + WAKE_INT_MASK); +} + +/** + * ve_spc_set_resume_addr() - set the jump address used for warm boot + * + * @cluster: mpidr[15:8] bitfield describing cluster affinity level + * @cpu: mpidr[7:0] bitfield describing cpu affinity level + * @addr: physical resume address + */ +void ve_spc_set_resume_addr(u32 cluster, u32 cpu, u32 addr) +{ + void __iomem *baseaddr; + + if (cluster >= MAX_CLUSTERS) + return; + + if (cluster_is_a15(cluster)) + baseaddr = info->baseaddr + A15_BX_ADDR0 + (cpu << 2); + else + baseaddr = info->baseaddr + A7_BX_ADDR0 + (cpu << 2); + + writel_relaxed(addr, baseaddr); +} + +/** + * ve_spc_get_nr_cpus() - get number of cpus in a cluster + * + * @cluster: mpidr[15:8] bitfield describing cluster affinity level + * + * Return: > 0 number of cpus in the cluster + * or 0 if cluster number invalid + */ +u32 ve_spc_get_nr_cpus(u32 cluster) +{ + u32 val; + + if (cluster >= MAX_CLUSTERS) + return 0; + + val = readl_relaxed(info->baseaddr + SYS_INFO); + val = cluster_is_a15(cluster) ? (val >> 16) : (val >> 20); + return val & 0xf; +} + +/** + * ve_spc_powerdown() + * + * Function to enable/disable cluster powerdown. Not protected by locking + * since it might be used in code paths where normal cacheable locks are not + * working. Locking must be provided by the caller to ensure atomicity. + * + * @cluster: mpidr[15:8] bitfield describing cluster affinity level + * @enable: if true enables powerdown, if false disables it + */ +void ve_spc_powerdown(u32 cluster, bool enable) +{ + u32 pwdrn_reg; + + if (cluster >= MAX_CLUSTERS) + return; + + pwdrn_reg = cluster_is_a15(cluster) ? A15_PWRDN_EN : A7_PWRDN_EN; + writel_relaxed(enable, info->baseaddr + pwdrn_reg); +} + +static const struct of_device_id ve_spc_ids[] __initconst = { + { .compatible = "arm,vexpress-spc,v2p-ca15_a7" }, + { .compatible = "arm,vexpress-spc" }, + {}, +}; + +static int __init ve_spc_probe(void) +{ + int ret; + struct device_node *node = of_find_matching_node(NULL, ve_spc_ids); + + if (!node) + return -ENODEV; + + info = kzalloc(sizeof(*info), GFP_KERNEL); + if (!info) { + pr_err(SPCLOG "unable to allocate mem\n"); + return -ENOMEM; + } + + info->baseaddr = of_iomap(node, 0); + if (!info->baseaddr) { + pr_err(SPCLOG "unable to ioremap memory\n"); + ret = -ENXIO; + goto mem_free; + } + + info->a15_clusid = readl_relaxed(info->baseaddr + A15_CONF) & 0xf; + + /* + * Multi-cluster systems may need this data when non-coherent, during + * cluster power-up/power-down. Make sure driver info reaches main + * memory. + */ + sync_cache_w(info); + sync_cache_w(&info); + pr_info("vexpress-spc loaded at %p\n", info->baseaddr); + return 0; + +mem_free: + kfree(info); + return ret; +} + +/** + * ve_spc_init() + * + * Function exported to manage pre early_initcall initialization. + * SPC code is needed very early in the boot process to bring CPUs out of + * reset and initialize power management back-end so an init interface is + * provided to platform code to allow early initialization. The init + * interface can be removed as soon as the DT layer and platform bus allow + * platform device creation and probing before SMP boot. + */ +int __init ve_spc_init(void) +{ + static int ve_spc_init_status __initdata = -EAGAIN; + + if (ve_spc_init_status == -EAGAIN) + ve_spc_init_status = ve_spc_probe(); + + return ve_spc_init_status; +} +early_initcall(ve_spc_init); diff --git a/include/linux/vexpress.h b/include/linux/vexpress.h index 50368e0..d0106ef 100644 --- a/include/linux/vexpress.h +++ b/include/linux/vexpress.h @@ -132,4 +132,21 @@ void vexpress_clk_of_register_spc(void); void vexpress_clk_init(void __iomem *sp810_base); void vexpress_clk_of_init(void); +/* SPC */ + +#ifdef CONFIG_VEXPRESS_SPC +int __init ve_spc_init(void); +void ve_spc_global_wakeup_irq(bool set); +void ve_spc_cpu_wakeup_irq(u32 cluster, u32 cpu, bool set); +void ve_spc_set_resume_addr(u32 cluster, u32 cpu, u32 addr); +u32 ve_spc_get_nr_cpus(u32 cluster); +void ve_spc_powerdown(u32 cluster, bool enable); +#else +static inline int ve_spc_init(void) { return -ENODEV; } +static inline void ve_spc_global_wakeup_irq(bool set) { } +static inline void ve_spc_cpu_wakeup_irq(u32 cluster, u32 cpu, bool set) { } +static inline void ve_spc_set_resume_addr(u32 cluster, u32 cpu, u32 addr) { } +static inline u32 ve_spc_get_nr_cpus(u32 cluster) { return 0; } +static inline void ve_spc_powerdown(u32 cluster, bool enable) { } +#endif #endif -- 1.8.2.2 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [RFC PATCH v5 1/1] drivers: mfd: vexpress: add Serial Power Controller (SPC) support 2013-07-16 16:05 ` [RFC PATCH v5 1/1] drivers: mfd: vexpress: add Serial Power Controller (SPC) support Lorenzo Pieralisi @ 2013-07-16 20:05 ` Rob Herring 2013-07-16 23:32 ` Nicolas Pitre 0 siblings, 1 reply; 20+ messages in thread From: Rob Herring @ 2013-07-16 20:05 UTC (permalink / raw) To: Lorenzo Pieralisi Cc: linux-kernel, linux-arm-kernel, devicetree-discuss, Jon Medhurst, Nicolas Pitre, Samuel Ortiz, Pawel Moll, Achin Gupta, Amit Kucheria On 07/16/2013 11:05 AM, 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, wake-up irqs and per-CPU jump addresses registers. > > This patch provides a driver that enables run-time control of features > implemented by the SPC power management control logic. > > The SPC control logic is required to be programmed very early in the boot > process to reset secondary CPUs on the TC2 testchip, set-up jump addresses and > wake-up IRQs for power management. Hence, waiting for core changes to be > made in the device core code to enable early registration of platform > devices, the driver puts in place an early init scheme that allows kernel > drivers to initialize the SPC driver directly from the components requiring > it, if their initialization routine is called before the driver init > function by the boot process. > > Device tree bindings documentation for the SPC component is provided with > the patchset. Just curious, wouldn't a TC2 PSCI implementation eliminate the need for most/all of this code? Rob > > Cc: Samuel Ortiz <sameo@linux.intel.com> > Cc: Olof Johansson <olof@lixom.net> > Cc: Pawel Moll <pawel.moll@arm.com> > 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> > --- > Documentation/devicetree/bindings/mfd/vexpress-spc.txt | 36 ++ > drivers/mfd/Kconfig | 10 + > drivers/mfd/Makefile | 1 + > drivers/mfd/vexpress-spc.c | 253 ++++++++++ > include/linux/vexpress.h | 17 + > 5 files changed, 317 insertions(+) > > diff --git a/Documentation/devicetree/bindings/mfd/vexpress-spc.txt b/Documentation/devicetree/bindings/mfd/vexpress-spc.txt > new file mode 100644 > index 0000000..1614725 > --- /dev/null > +++ b/Documentation/devicetree/bindings/mfd/vexpress-spc.txt > @@ -0,0 +1,36 @@ > +* ARM Versatile Express Serial Power Controller device tree bindings > + > +Latest ARM development boards implement a power management interface (serial > +power controller - SPC) that is capable of managing power states transitions, > +wake-up IRQs and resume addresses for ARM multiprocessor testchips. > +The serial controller can be programmed through a memory mapped interface > +that enables communication between firmware running on the microcontroller > +managing power states and the application processors. > + > +The SPC DT bindings are defined as follows: > + > +- spc node > + > + - compatible: > + Usage: required > + Value type: <stringlist> > + Definition: must be > + "arm,vexpress-spc,v2p-ca15_a7", "arm,vexpress-spc" > + - reg: > + Usage: required > + Value type: <prop-encode-array> > + Definition: A standard property that specifies the base address > + and the size of the SPC address space > + - interrupts: > + Usage: required > + Value type: <prop-encoded-array> > + Definition: SPC interrupt configuration. A standard property > + that follows ePAPR interrupts specifications > + > +Example: > + > +spc: spc@7fff0000 { > + compatible = "arm,vexpress-spc,v2p-ca15_a7", "arm,vexpress-spc"; > + reg = <0x7fff0000 0x1000>; > + interrupts = <0 95 4>; > +}; > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > index 6959b8d..ebd23f4 100644 > --- a/drivers/mfd/Kconfig > +++ b/drivers/mfd/Kconfig > @@ -1149,3 +1149,13 @@ config VEXPRESS_CONFIG > help > Platform configuration infrastructure for the ARM Ltd. > Versatile Express. > + > +config VEXPRESS_SPC > + bool "Versatile Express SPC driver support" > + depends on ARM > + help > + The Serial Power Controller (SPC) for ARM Ltd. test chips, is > + an IP that provides a memory mapped interface to power controller > + HW. The driver provides an API abstraction allowing to program > + registers controlling low-level power management features like power > + down flags, global and per-cpu wake-up IRQs. > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile > index 718e94a..3a01203 100644 > --- a/drivers/mfd/Makefile > +++ b/drivers/mfd/Makefile > @@ -153,5 +153,6 @@ obj-$(CONFIG_MFD_SEC_CORE) += sec-core.o sec-irq.o > obj-$(CONFIG_MFD_SYSCON) += syscon.o > obj-$(CONFIG_MFD_LM3533) += lm3533-core.o lm3533-ctrlbank.o > obj-$(CONFIG_VEXPRESS_CONFIG) += vexpress-config.o vexpress-sysreg.o > +obj-$(CONFIG_VEXPRESS_SPC) += vexpress-spc.o > obj-$(CONFIG_MFD_RETU) += retu-mfd.o > obj-$(CONFIG_MFD_AS3711) += as3711.o > diff --git a/drivers/mfd/vexpress-spc.c b/drivers/mfd/vexpress-spc.c > new file mode 100644 > index 0000000..aa8c2a4 > --- /dev/null > +++ b/drivers/mfd/vexpress-spc.c > @@ -0,0 +1,253 @@ > +/* > + * Versatile Express Serial Power Controller (SPC) support > + * > + * Copyright (C) 2013 ARM Ltd. > + * > + * Authors: Sudeep KarkadaNagesha <sudeep.karkadanagesha@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/of_address.h> > +#include <linux/slab.h> > +#include <linux/vexpress.h> > + > +#include <asm/cacheflush.h> > + > +#define SPCLOG "vexpress-spc: " > + > +/* SCC conf registers */ > +#define A15_CONF 0x400 > +#define SYS_INFO 0x700 > + > +/* SPC registers base */ > +#define SPC_BASE 0xB00 > + > +/* SPC wake-up IRQs status and mask */ > +#define WAKE_INT_MASK (SPC_BASE + 0x24) > +#define WAKE_INT_RAW (SPC_BASE + 0x28) > +#define WAKE_INT_STAT (SPC_BASE + 0x2c) > +/* SPC power down registers */ > +#define A15_PWRDN_EN (SPC_BASE + 0x30) > +#define A7_PWRDN_EN (SPC_BASE + 0x34) > +/* SPC per-CPU mailboxes */ > +#define A15_BX_ADDR0 (SPC_BASE + 0x68) > +#define A7_BX_ADDR0 (SPC_BASE + 0x78) > + > +/* wake-up interrupt masks */ > +#define GBL_WAKEUP_INT_MSK (0x3 << 10) > + > +/* TC2 static dual-cluster configuration */ > +#define MAX_CLUSTERS 2 > + > +struct ve_spc_drvdata { > + void __iomem *baseaddr; > + /* > + * A15s cluster identifier > + * It corresponds to A15 processors MPIDR[15:8] bitfield > + */ > + u32 a15_clusid; > +}; > + > +static struct ve_spc_drvdata *info; > + > +static inline bool cluster_is_a15(u32 cluster) > +{ > + return cluster == info->a15_clusid; > +} > + > +/** > + * ve_spc_global_wakeup_irq() > + * > + * Function to set/clear global wakeup IRQs. Not protected by locking since > + * it might be used in code paths where normal cacheable locks are not > + * working. Locking must be provided by the caller to ensure atomicity. > + * > + * @set: if true, global wake-up IRQs are set, if false they are cleared > + */ > +void ve_spc_global_wakeup_irq(bool set) > +{ > + u32 reg; > + > + reg = readl_relaxed(info->baseaddr + WAKE_INT_MASK); > + > + if (set) > + reg |= GBL_WAKEUP_INT_MSK; > + else > + reg &= ~GBL_WAKEUP_INT_MSK; > + > + writel_relaxed(reg, info->baseaddr + WAKE_INT_MASK); > +} > + > +/** > + * ve_spc_cpu_wakeup_irq() > + * > + * Function to set/clear per-CPU wake-up IRQs. Not protected by locking since > + * it might be used in code paths where normal cacheable locks are not > + * working. Locking must be provided by the caller to ensure atomicity. > + * > + * @cluster: mpidr[15:8] bitfield describing cluster affinity level > + * @cpu: mpidr[7:0] bitfield describing cpu affinity level > + * @set: if true, wake-up IRQs are set, if false they are cleared > + */ > +void ve_spc_cpu_wakeup_irq(u32 cluster, u32 cpu, bool set) > +{ > + u32 mask, reg; > + > + if (cluster >= MAX_CLUSTERS) > + return; > + > + mask = 1 << cpu; > + > + if (!cluster_is_a15(cluster)) > + mask <<= 4; > + > + reg = readl_relaxed(info->baseaddr + WAKE_INT_MASK); > + > + if (set) > + reg |= mask; > + else > + reg &= ~mask; > + > + writel_relaxed(reg, info->baseaddr + WAKE_INT_MASK); > +} > + > +/** > + * ve_spc_set_resume_addr() - set the jump address used for warm boot > + * > + * @cluster: mpidr[15:8] bitfield describing cluster affinity level > + * @cpu: mpidr[7:0] bitfield describing cpu affinity level > + * @addr: physical resume address > + */ > +void ve_spc_set_resume_addr(u32 cluster, u32 cpu, u32 addr) > +{ > + void __iomem *baseaddr; > + > + if (cluster >= MAX_CLUSTERS) > + return; > + > + if (cluster_is_a15(cluster)) > + baseaddr = info->baseaddr + A15_BX_ADDR0 + (cpu << 2); > + else > + baseaddr = info->baseaddr + A7_BX_ADDR0 + (cpu << 2); > + > + writel_relaxed(addr, baseaddr); > +} > + > +/** > + * ve_spc_get_nr_cpus() - get number of cpus in a cluster > + * > + * @cluster: mpidr[15:8] bitfield describing cluster affinity level > + * > + * Return: > 0 number of cpus in the cluster > + * or 0 if cluster number invalid > + */ > +u32 ve_spc_get_nr_cpus(u32 cluster) > +{ > + u32 val; > + > + if (cluster >= MAX_CLUSTERS) > + return 0; > + > + val = readl_relaxed(info->baseaddr + SYS_INFO); > + val = cluster_is_a15(cluster) ? (val >> 16) : (val >> 20); > + return val & 0xf; > +} > + > +/** > + * ve_spc_powerdown() > + * > + * Function to enable/disable cluster powerdown. Not protected by locking > + * since it might be used in code paths where normal cacheable locks are not > + * working. Locking must be provided by the caller to ensure atomicity. > + * > + * @cluster: mpidr[15:8] bitfield describing cluster affinity level > + * @enable: if true enables powerdown, if false disables it > + */ > +void ve_spc_powerdown(u32 cluster, bool enable) > +{ > + u32 pwdrn_reg; > + > + if (cluster >= MAX_CLUSTERS) > + return; > + > + pwdrn_reg = cluster_is_a15(cluster) ? A15_PWRDN_EN : A7_PWRDN_EN; > + writel_relaxed(enable, info->baseaddr + pwdrn_reg); > +} > + > +static const struct of_device_id ve_spc_ids[] __initconst = { > + { .compatible = "arm,vexpress-spc,v2p-ca15_a7" }, > + { .compatible = "arm,vexpress-spc" }, > + {}, > +}; > + > +static int __init ve_spc_probe(void) > +{ > + int ret; > + struct device_node *node = of_find_matching_node(NULL, ve_spc_ids); > + > + if (!node) > + return -ENODEV; > + > + info = kzalloc(sizeof(*info), GFP_KERNEL); > + if (!info) { > + pr_err(SPCLOG "unable to allocate mem\n"); > + return -ENOMEM; > + } > + > + info->baseaddr = of_iomap(node, 0); > + if (!info->baseaddr) { > + pr_err(SPCLOG "unable to ioremap memory\n"); > + ret = -ENXIO; > + goto mem_free; > + } > + > + info->a15_clusid = readl_relaxed(info->baseaddr + A15_CONF) & 0xf; > + > + /* > + * Multi-cluster systems may need this data when non-coherent, during > + * cluster power-up/power-down. Make sure driver info reaches main > + * memory. > + */ > + sync_cache_w(info); > + sync_cache_w(&info); > + pr_info("vexpress-spc loaded at %p\n", info->baseaddr); > + return 0; > + > +mem_free: > + kfree(info); > + return ret; > +} > + > +/** > + * ve_spc_init() > + * > + * Function exported to manage pre early_initcall initialization. > + * SPC code is needed very early in the boot process to bring CPUs out of > + * reset and initialize power management back-end so an init interface is > + * provided to platform code to allow early initialization. The init > + * interface can be removed as soon as the DT layer and platform bus allow > + * platform device creation and probing before SMP boot. > + */ > +int __init ve_spc_init(void) > +{ > + static int ve_spc_init_status __initdata = -EAGAIN; > + > + if (ve_spc_init_status == -EAGAIN) > + ve_spc_init_status = ve_spc_probe(); > + > + return ve_spc_init_status; > +} > +early_initcall(ve_spc_init); > diff --git a/include/linux/vexpress.h b/include/linux/vexpress.h > index 50368e0..d0106ef 100644 > --- a/include/linux/vexpress.h > +++ b/include/linux/vexpress.h > @@ -132,4 +132,21 @@ void vexpress_clk_of_register_spc(void); > void vexpress_clk_init(void __iomem *sp810_base); > void vexpress_clk_of_init(void); > > +/* SPC */ > + > +#ifdef CONFIG_VEXPRESS_SPC > +int __init ve_spc_init(void); > +void ve_spc_global_wakeup_irq(bool set); > +void ve_spc_cpu_wakeup_irq(u32 cluster, u32 cpu, bool set); > +void ve_spc_set_resume_addr(u32 cluster, u32 cpu, u32 addr); > +u32 ve_spc_get_nr_cpus(u32 cluster); > +void ve_spc_powerdown(u32 cluster, bool enable); > +#else > +static inline int ve_spc_init(void) { return -ENODEV; } > +static inline void ve_spc_global_wakeup_irq(bool set) { } > +static inline void ve_spc_cpu_wakeup_irq(u32 cluster, u32 cpu, bool set) { } > +static inline void ve_spc_set_resume_addr(u32 cluster, u32 cpu, u32 addr) { } > +static inline u32 ve_spc_get_nr_cpus(u32 cluster) { return 0; } > +static inline void ve_spc_powerdown(u32 cluster, bool enable) { } > +#endif > #endif > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC PATCH v5 1/1] drivers: mfd: vexpress: add Serial Power Controller (SPC) support 2013-07-16 20:05 ` Rob Herring @ 2013-07-16 23:32 ` Nicolas Pitre 0 siblings, 0 replies; 20+ messages in thread From: Nicolas Pitre @ 2013-07-16 23:32 UTC (permalink / raw) To: Rob Herring Cc: Lorenzo Pieralisi, linux-kernel, linux-arm-kernel, devicetree-discuss, Jon Medhurst, Samuel Ortiz, Pawel Moll, Achin Gupta, Amit Kucheria On Tue, 16 Jul 2013, Rob Herring wrote: > On 07/16/2013 11:05 AM, 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, wake-up irqs and per-CPU jump addresses registers. > > > > This patch provides a driver that enables run-time control of features > > implemented by the SPC power management control logic. > > > > The SPC control logic is required to be programmed very early in the boot > > process to reset secondary CPUs on the TC2 testchip, set-up jump addresses and > > wake-up IRQs for power management. Hence, waiting for core changes to be > > made in the device core code to enable early registration of platform > > devices, the driver puts in place an early init scheme that allows kernel > > drivers to initialize the SPC driver directly from the components requiring > > it, if their initialization routine is called before the driver init > > function by the boot process. > > > > Device tree bindings documentation for the SPC component is provided with > > the patchset. > > Just curious, wouldn't a TC2 PSCI implementation eliminate the need for > most/all of this code? There is a PSCI equivalent for the above already, in the sense that there is a simple MCPM backend that bypass most of the MCPM race avoidance code paths and simply calls into PSCI instead. But not all the world is going to be PSCI, and therefore we need to ensure good support for non-PSCI platforms as well. This is why TC2 supports both, and this also ensure that the PSCI implementation, which won't be part of the kernel and therefore unlikely to get the same level of scrutiny, is properly implemented and doesn't introduce any regression when compared to the non PSCI case. Remember that TC2 is multi-cluster which means that the PSCI implementation has to carry the same amount of complexity as the whole in-kernel MCPM layer and that doesn't make me overly confident it is going to always be right. All this to say that we do need this code despite PSCI availability. Nicolas ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC PATCH v5 0/1] drivers: mfd: Versatile Express SPC support 2013-07-16 16:05 [RFC PATCH v5 0/1] drivers: mfd: Versatile Express SPC support Lorenzo Pieralisi 2013-07-16 16:05 ` [RFC PATCH v5 1/1] drivers: mfd: vexpress: add Serial Power Controller (SPC) support Lorenzo Pieralisi @ 2013-07-17 3:26 ` Nicolas Pitre 2013-07-17 9:18 ` Pawel Moll 2013-07-17 21:07 ` Samuel Ortiz 3 siblings, 0 replies; 20+ messages in thread From: Nicolas Pitre @ 2013-07-17 3:26 UTC (permalink / raw) To: Lorenzo Pieralisi Cc: linux-kernel, linux-arm-kernel, devicetree-discuss, Samuel Ortiz, Olof Johansson, Pawel Moll, Amit Kucheria, Jon Medhurst, Achin Gupta, Sudeep KarkadaNagesha On Tue, 16 Jul 2013, Lorenzo Pieralisi wrote: > Hello, > > version v5 of VExpress SPC driver, please read on the changelog for major > changes and explanations. > > The probing scheme is unchanged, since after trying the early platform > devices approach it appeared that the end result was no better than the > current one. The only clean solution relies either on changing how > secondaries are brought up in the kernel (later than now) or enable > early platform device registration through DT. Please check this > thread for the related discussion: > > https://lists.ozlabs.org/pipermail/devicetree-discuss/2013-June/036542.html > > The interface was adapted to regmap and again reverted to old driver for > the following reasons: > > - Power down registers locking is hairy and requires arch spinlocks in > the MCPM back end to work properly, normal spinlocks cannot be used > - Regmap adds unnecessary code to manage SPC since it is just a bunch of > registers used to control power management flags, the overhead is just > not worth it (talking about power down registers, not the vexpress config > interface) > - The locking scheme behind regmap requires all registers in the map > to be protected with the same lock, which is not exactly what we want > here > - Given the reasons above, adding a regmap interface buys us nothing from > a driver readability and maintainability perspective (again just talking > about the power interface, a few registers) because for the SPC it would > simply not be used > > /drivers/mfd is probably not the right place for this code as it stands (but > probably will be when the entire driver, with DVFS and config interface, is > complete). > > Thank you for the review in advance, > Lorenzo I've integrated this patch in my MCPM backend for TC2 patch series. ACKs from concerned/interested people would be appreciated so I could send this for ARM-SOC inclusion right away. Nicolas ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC PATCH v5 0/1] drivers: mfd: Versatile Express SPC support 2013-07-16 16:05 [RFC PATCH v5 0/1] drivers: mfd: Versatile Express SPC support Lorenzo Pieralisi 2013-07-16 16:05 ` [RFC PATCH v5 1/1] drivers: mfd: vexpress: add Serial Power Controller (SPC) support Lorenzo Pieralisi 2013-07-17 3:26 ` [RFC PATCH v5 0/1] drivers: mfd: Versatile Express SPC support Nicolas Pitre @ 2013-07-17 9:18 ` Pawel Moll 2013-07-17 10:44 ` Lorenzo Pieralisi 2013-07-17 12:33 ` Nicolas Pitre 2013-07-17 21:07 ` Samuel Ortiz 3 siblings, 2 replies; 20+ messages in thread From: Pawel Moll @ 2013-07-17 9:18 UTC (permalink / raw) To: Lorenzo Pieralisi Cc: linux-kernel, linux-arm-kernel, devicetree-discuss, Samuel Ortiz, Olof Johansson, Nicolas Pitre, Amit Kucheria, Jon Medhurst, Achin Gupta, Sudeep KarkadaNagesha On Tue, 2013-07-16 at 17:05 +0100, Lorenzo Pieralisi wrote: > /drivers/mfd is probably not the right place for this code as it stands (but > probably will be when the entire driver, with DVFS and config interface, is > complete). Not that it really matters now, but my vexpress-sysreg rework will - most likely - leave only skeleton in the MFD (registering mfd_cells) and other stuff is going to be spread all around. Then I'm planning to move the remaining of the vexpress-specific initialization to drivers/platform/arm/vexpress.c, so maybe sticking vexpress-spc.c to this (non-existing yet) directory would be the right thing to do? Other than that: Acked-by: Pawel Moll <pawel.moll@arm.com> Thanks! Paweł ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC PATCH v5 0/1] drivers: mfd: Versatile Express SPC support 2013-07-17 9:18 ` Pawel Moll @ 2013-07-17 10:44 ` Lorenzo Pieralisi 2013-07-17 12:33 ` Nicolas Pitre 1 sibling, 0 replies; 20+ messages in thread From: Lorenzo Pieralisi @ 2013-07-17 10:44 UTC (permalink / raw) To: Pawel Moll Cc: linux-kernel, linux-arm-kernel, devicetree-discuss, Samuel Ortiz, Olof Johansson, Nicolas Pitre, Amit Kucheria, Jon Medhurst, Achin Gupta, Sudeep KarkadaNagesha On Wed, Jul 17, 2013 at 10:18:25AM +0100, Pawel Moll wrote: > On Tue, 2013-07-16 at 17:05 +0100, Lorenzo Pieralisi wrote: > > /drivers/mfd is probably not the right place for this code as it stands (but > > probably will be when the entire driver, with DVFS and config interface, is > > complete). > > Not that it really matters now, but my vexpress-sysreg rework will - > most likely - leave only skeleton in the MFD (registering mfd_cells) and > other stuff is going to be spread all around. Then I'm planning to move > the remaining of the vexpress-specific initialization to > drivers/platform/arm/vexpress.c, so maybe sticking vexpress-spc.c to > this (non-existing yet) directory would be the right thing to do? Done. I do not think there is a point in splitting the patch to create the dir and make infrastructure, so I squashed everything in the original patch. I have not added any maintainer for that dir/file, I guess it can wait till you finish the rework so that you can add yourself there. If that's all I need to change I do not even think that reposting is necessary. It does matter though, since it implies changes on who is in charge of ack/nack'ing this code, if it is no more an mfd matter. I will wait to check all interested/concerned parties opinions, that are always welcome. > Other than that: > > Acked-by: Pawel Moll <pawel.moll@arm.com> Thank you !!! Lorenzo ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC PATCH v5 0/1] drivers: mfd: Versatile Express SPC support 2013-07-17 9:18 ` Pawel Moll 2013-07-17 10:44 ` Lorenzo Pieralisi @ 2013-07-17 12:33 ` Nicolas Pitre 2013-07-17 13:29 ` Pawel Moll 1 sibling, 1 reply; 20+ messages in thread From: Nicolas Pitre @ 2013-07-17 12:33 UTC (permalink / raw) To: Pawel Moll Cc: Lorenzo Pieralisi, linux-kernel, linux-arm-kernel, devicetree-discuss, Samuel Ortiz, Olof Johansson, Amit Kucheria, Jon Medhurst, Achin Gupta, Sudeep KarkadaNagesha On Wed, 17 Jul 2013, Pawel Moll wrote: > On Tue, 2013-07-16 at 17:05 +0100, Lorenzo Pieralisi wrote: > > /drivers/mfd is probably not the right place for this code as it stands (but > > probably will be when the entire driver, with DVFS and config interface, is > > complete). > > Not that it really matters now, but my vexpress-sysreg rework will - > most likely - leave only skeleton in the MFD (registering mfd_cells) and > other stuff is going to be spread all around. Then I'm planning to move > the remaining of the vexpress-specific initialization to > drivers/platform/arm/vexpress.c, so maybe sticking vexpress-spc.c to > this (non-existing yet) directory would be the right thing to do? I don't like this idea. We worked hard to shrink platform specific directories such as arch/arm/mach-*/ as much as possible. Simply moving stuff to drivers/platform/arm/* doesn't make the situation any much better. If this is really miscelaneous code that really doesn't fit anywhere else, it should rather go into drivers/misc/ as a last resort. Nicolas ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC PATCH v5 0/1] drivers: mfd: Versatile Express SPC support 2013-07-17 12:33 ` Nicolas Pitre @ 2013-07-17 13:29 ` Pawel Moll 2013-07-17 14:16 ` Nicolas Pitre 0 siblings, 1 reply; 20+ messages in thread From: Pawel Moll @ 2013-07-17 13:29 UTC (permalink / raw) To: Nicolas Pitre Cc: Lorenzo Pieralisi, linux-kernel, linux-arm-kernel, devicetree-discuss, Samuel Ortiz, Olof Johansson, Amit Kucheria, Jon Medhurst, Achin Gupta, Sudeep KarkadaNagesha On Wed, 2013-07-17 at 13:33 +0100, Nicolas Pitre wrote: > If this is really miscelaneous code that really doesn't fit > anywhere else, it should rather go into drivers/misc/ as a last resort. Interestingly enough drivers/misc was my first choice for all the vexpress stuff, but it wasn't received well... Anyway, the SPC driver as it is now seem to be a "power management system driver". Maybe a relevant directory would be in place? Wouldn't PSCI belong there as well? (there are two psci.c files in arch/arm and arch/arm64, surprisingly similar ones ;-) The bottom line is: today it is not an MFD driver. Paweł ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC PATCH v5 0/1] drivers: mfd: Versatile Express SPC support 2013-07-17 13:29 ` Pawel Moll @ 2013-07-17 14:16 ` Nicolas Pitre 2013-07-17 14:20 ` Pawel Moll 0 siblings, 1 reply; 20+ messages in thread From: Nicolas Pitre @ 2013-07-17 14:16 UTC (permalink / raw) To: Pawel Moll Cc: Lorenzo Pieralisi, linux-kernel, linux-arm-kernel, devicetree-discuss, Samuel Ortiz, Olof Johansson, Amit Kucheria, Jon Medhurst, Achin Gupta, Sudeep KarkadaNagesha On Wed, 17 Jul 2013, Pawel Moll wrote: > On Wed, 2013-07-17 at 13:33 +0100, Nicolas Pitre wrote: > > If this is really miscelaneous code that really doesn't fit > > anywhere else, it should rather go into drivers/misc/ as a last resort. > > Interestingly enough drivers/misc was my first choice for all the > vexpress stuff, but it wasn't received well... > > Anyway, the SPC driver as it is now seem to be a "power management > system driver". Maybe a relevant directory would be in place? Wouldn't > PSCI belong there as well? (there are two psci.c files in arch/arm and > arch/arm64, surprisingly similar ones ;-) > > The bottom line is: today it is not an MFD driver. But we know it will, right? So better save some churn by storing the initial code where it would end up anyway once complete. Nicolas ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC PATCH v5 0/1] drivers: mfd: Versatile Express SPC support 2013-07-17 14:16 ` Nicolas Pitre @ 2013-07-17 14:20 ` Pawel Moll 2013-07-17 15:57 ` Nicolas Pitre 2013-07-17 21:10 ` Samuel Ortiz 0 siblings, 2 replies; 20+ messages in thread From: Pawel Moll @ 2013-07-17 14:20 UTC (permalink / raw) To: Nicolas Pitre Cc: Lorenzo Pieralisi, linux-kernel, linux-arm-kernel, devicetree-discuss, Samuel Ortiz, Olof Johansson, Amit Kucheria, Jon Medhurst, Achin Gupta, Sudeep KarkadaNagesha On Wed, 2013-07-17 at 15:16 +0100, Nicolas Pitre wrote: > On Wed, 17 Jul 2013, Pawel Moll wrote: > > > On Wed, 2013-07-17 at 13:33 +0100, Nicolas Pitre wrote: > > > If this is really miscelaneous code that really doesn't fit > > > anywhere else, it should rather go into drivers/misc/ as a last resort. > > > > Interestingly enough drivers/misc was my first choice for all the > > vexpress stuff, but it wasn't received well... > > > > Anyway, the SPC driver as it is now seem to be a "power management > > system driver". Maybe a relevant directory would be in place? Wouldn't > > PSCI belong there as well? (there are two psci.c files in arch/arm and > > arch/arm64, surprisingly similar ones ;-) > > > > The bottom line is: today it is not an MFD driver. > > But we know it will, right? So better save some churn by storing the > initial code where it would end up anyway once complete. Not in that form, no. The code living in mfd will just register mfd_cells while "functional" parts are going to live elsewhere. This is how I understand what Samuel asked me to do and that's what is happening to vexpress-sysreg now. Paweł ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC PATCH v5 0/1] drivers: mfd: Versatile Express SPC support 2013-07-17 14:20 ` Pawel Moll @ 2013-07-17 15:57 ` Nicolas Pitre 2013-07-17 17:00 ` Russell King - ARM Linux 2013-07-17 21:10 ` Samuel Ortiz 1 sibling, 1 reply; 20+ messages in thread From: Nicolas Pitre @ 2013-07-17 15:57 UTC (permalink / raw) To: Pawel Moll Cc: Lorenzo Pieralisi, linux-kernel, linux-arm-kernel, devicetree-discuss, Samuel Ortiz, Olof Johansson, Amit Kucheria, Jon Medhurst, Achin Gupta, Sudeep KarkadaNagesha On Wed, 17 Jul 2013, Pawel Moll wrote: > On Wed, 2013-07-17 at 15:16 +0100, Nicolas Pitre wrote: > > On Wed, 17 Jul 2013, Pawel Moll wrote: > > > > > On Wed, 2013-07-17 at 13:33 +0100, Nicolas Pitre wrote: > > > > If this is really miscelaneous code that really doesn't fit > > > > anywhere else, it should rather go into drivers/misc/ as a last resort. > > > > > > Interestingly enough drivers/misc was my first choice for all the > > > vexpress stuff, but it wasn't received well... > > > > > > Anyway, the SPC driver as it is now seem to be a "power management > > > system driver". Maybe a relevant directory would be in place? Wouldn't > > > PSCI belong there as well? (there are two psci.c files in arch/arm and > > > arch/arm64, surprisingly similar ones ;-) > > > > > > The bottom line is: today it is not an MFD driver. > > > > But we know it will, right? So better save some churn by storing the > > initial code where it would end up anyway once complete. > > Not in that form, no. The code living in mfd will just register > mfd_cells while "functional" parts are going to live elsewhere. This is > how I understand what Samuel asked me to do and that's what is happening > to vexpress-sysreg now. A drivers/pm/ or drivers/power/ could be created, but that implies sort of a more defined subsystem with a common API and the SPC code doesn't fit that as it is only providing services to actual drivers on top of it. The sanest location at this point might simply be drivers/platform/vexpress_spc.c or drivers/platform/vexpress/spc.c depending on whether or not more such driver glue is expected in the vexpress future. No point putting "arm" in the path, especially if this is later reused on arm64. Nicolas ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC PATCH v5 0/1] drivers: mfd: Versatile Express SPC support 2013-07-17 15:57 ` Nicolas Pitre @ 2013-07-17 17:00 ` Russell King - ARM Linux 2013-07-17 18:29 ` Nicolas Pitre 0 siblings, 1 reply; 20+ messages in thread From: Russell King - ARM Linux @ 2013-07-17 17:00 UTC (permalink / raw) To: Nicolas Pitre Cc: Pawel Moll, Jon Medhurst, Lorenzo Pieralisi, Samuel Ortiz, Sudeep KarkadaNagesha, devicetree-discuss, linux-kernel, Amit Kucheria, Olof Johansson, Achin Gupta, linux-arm-kernel On Wed, Jul 17, 2013 at 11:57:55AM -0400, Nicolas Pitre wrote: > The sanest location at this point might simply be > drivers/platform/vexpress_spc.c or drivers/platform/vexpress/spc.c > depending on whether or not more such driver glue is expected in the > vexpress future. No point putting "arm" in the path, especially if this > is later reused on arm64. You wouldn't be making that argument if it were arch/arm64 and arch/arm32 - you'd probably be arguing that "arm" made perfect sense. Don't get too hung up on names please, it's really not worth the time and effort being soo pedantic, and being soo pedantic leads to "pointless churn" when someone comes along and wants to pedantically change the names because it no longer matches the users. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC PATCH v5 0/1] drivers: mfd: Versatile Express SPC support 2013-07-17 17:00 ` Russell King - ARM Linux @ 2013-07-17 18:29 ` Nicolas Pitre 2013-07-17 21:23 ` Samuel Ortiz 0 siblings, 1 reply; 20+ messages in thread From: Nicolas Pitre @ 2013-07-17 18:29 UTC (permalink / raw) To: Russell King - ARM Linux Cc: Pawel Moll, Jon Medhurst, Lorenzo Pieralisi, Samuel Ortiz, Sudeep KarkadaNagesha, devicetree-discuss, linux-kernel, Amit Kucheria, Olof Johansson, Achin Gupta, linux-arm-kernel On Wed, 17 Jul 2013, Russell King - ARM Linux wrote: > On Wed, Jul 17, 2013 at 11:57:55AM -0400, Nicolas Pitre wrote: > > The sanest location at this point might simply be > > drivers/platform/vexpress_spc.c or drivers/platform/vexpress/spc.c > > depending on whether or not more such driver glue is expected in the > > vexpress future. No point putting "arm" in the path, especially if this > > is later reused on arm64. > > You wouldn't be making that argument if it were arch/arm64 and arch/arm32 - > you'd probably be arguing that "arm" made perfect sense. Well... in a sense: yes. But in the end, having per arch directories under drivers/ is silly. We already have per arch directories under arch/already. > Don't get too hung up on names please, it's really not worth the time > and effort being soo pedantic, and being soo pedantic leads to "pointless > churn" when someone comes along and wants to pedantically change the > names because it no longer matches the users. At this point I don't really care about the name. I just want the damn thing merged upstream. But after several iterations to either fit one or another maintainers taste, each rework ends up in that maintainer saying: "Now that you've reworked the code, I still don't like it since this no longer fits in my subsystem tree." In fact what we'd need at this point is drivers/code_that_no_subsystem_maintainers_wants/. This is becoming overly ridiculous. Nicolas ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC PATCH v5 0/1] drivers: mfd: Versatile Express SPC support 2013-07-17 18:29 ` Nicolas Pitre @ 2013-07-17 21:23 ` Samuel Ortiz 2013-07-17 22:22 ` Nicolas Pitre 0 siblings, 1 reply; 20+ messages in thread From: Samuel Ortiz @ 2013-07-17 21:23 UTC (permalink / raw) To: Nicolas Pitre Cc: Russell King - ARM Linux, Pawel Moll, Jon Medhurst, Lorenzo Pieralisi, Sudeep KarkadaNagesha, devicetree-discuss, linux-kernel, Amit Kucheria, Olof Johansson, Achin Gupta, linux-arm-kernel Hi Nicolas, On Wed, Jul 17, 2013 at 02:29:02PM -0400, Nicolas Pitre wrote: > On Wed, 17 Jul 2013, Russell King - ARM Linux wrote: > > > On Wed, Jul 17, 2013 at 11:57:55AM -0400, Nicolas Pitre wrote: > > > The sanest location at this point might simply be > > > drivers/platform/vexpress_spc.c or drivers/platform/vexpress/spc.c > > > depending on whether or not more such driver glue is expected in the > > > vexpress future. No point putting "arm" in the path, especially if this > > > is later reused on arm64. > > > > You wouldn't be making that argument if it were arch/arm64 and arch/arm32 - > > you'd probably be arguing that "arm" made perfect sense. > > Well... in a sense: yes. But in the end, having per arch directories > under drivers/ is silly. We already have per arch directories under > arch/already. > > > Don't get too hung up on names please, it's really not worth the time > > and effort being soo pedantic, and being soo pedantic leads to "pointless > > churn" when someone comes along and wants to pedantically change the > > names because it no longer matches the users. > > At this point I don't really care about the name. I just want the damn > thing merged upstream. But after several iterations to either fit one > or another maintainers taste, each rework ends up in that maintainer > saying: "Now that you've reworked the code, I still don't like it since > this no longer fits in my subsystem tree." FWIW, we asked Pawel to rework the sysreg and config parts of the vexpress driver, make it an actual MFD driver, and spread the remaining bits of the code into their respective subsystems. I don't think this is an eccentric requirement. > In fact what we'd need at this point is > drivers/code_that_no_subsystem_maintainers_wants/. Which is what some people think drivers/mfd/ is... I don't mind merging Lorenzo's SPC driver as it is if he can explain to me how it will eventually evolve into an actual MFD driver. If that's not the case, I don't see how I could justify merging it through the MFD tree. Cheers, Samuel. -- Intel Open Source Technology Centre http://oss.intel.com/ ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC PATCH v5 0/1] drivers: mfd: Versatile Express SPC support 2013-07-17 21:23 ` Samuel Ortiz @ 2013-07-17 22:22 ` Nicolas Pitre 2013-07-17 22:47 ` Samuel Ortiz 0 siblings, 1 reply; 20+ messages in thread From: Nicolas Pitre @ 2013-07-17 22:22 UTC (permalink / raw) To: Samuel Ortiz Cc: Russell King - ARM Linux, Pawel Moll, Jon Medhurst, Lorenzo Pieralisi, Sudeep KarkadaNagesha, devicetree-discuss, linux-kernel, Amit Kucheria, Olof Johansson, Achin Gupta, linux-arm-kernel On Wed, 17 Jul 2013, Samuel Ortiz wrote: > On Wed, Jul 17, 2013 at 02:29:02PM -0400, Nicolas Pitre wrote: > > At this point I don't really care about the name. I just want the damn > > thing merged upstream. But after several iterations to either fit one > > or another maintainers taste, each rework ends up in that maintainer > > saying: "Now that you've reworked the code, I still don't like it since > > this no longer fits in my subsystem tree." > FWIW, we asked Pawel to rework the sysreg and config parts of the > vexpress driver, make it an actual MFD driver, and spread the remaining > bits of the code into their respective subsystems. I don't think > this is an eccentric requirement. I agree. However the code that Lorenzo just posted can't be deprived of any more sysreg/config parts. They are simply nonexistent. Even the larger code you reviewed before is completely useless without _additional_ drivers to go on top which are indeed waiting after this code to be merged before they are submitted to their respective subsystems. And those additional drivers are way more interesting than this dumb register access arbitrator. Because this is fundamentally the only thing it does. > > In fact what we'd need at this point is > > drivers/code_that_no_subsystem_maintainers_wants/. > Which is what some people think drivers/mfd/ is... Does mfd still stand for "Multi Function Device"? > I don't mind merging Lorenzo's SPC driver as it is if he can explain to > me how it will eventually evolve into an actual MFD driver. If that's > not the case, I don't see how I could justify merging it through the > MFD tree. Maybe the misunderstanding is about what actually is a MFD driver. Given your persisting reluctance, I may only conclude that this is indeed not a MFD driver after all. So I'll follow existing precedents in the kernel and move Lorenzo's code to drivers/platform/vexpress/. Nicolas ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC PATCH v5 0/1] drivers: mfd: Versatile Express SPC support 2013-07-17 22:22 ` Nicolas Pitre @ 2013-07-17 22:47 ` Samuel Ortiz 0 siblings, 0 replies; 20+ messages in thread From: Samuel Ortiz @ 2013-07-17 22:47 UTC (permalink / raw) To: Nicolas Pitre Cc: Russell King - ARM Linux, Pawel Moll, Jon Medhurst, Lorenzo Pieralisi, Sudeep KarkadaNagesha, devicetree-discuss, linux-kernel, Amit Kucheria, Olof Johansson, Achin Gupta, linux-arm-kernel On Wed, Jul 17, 2013 at 06:22:46PM -0400, Nicolas Pitre wrote: > On Wed, 17 Jul 2013, Samuel Ortiz wrote: > > > On Wed, Jul 17, 2013 at 02:29:02PM -0400, Nicolas Pitre wrote: > > > At this point I don't really care about the name. I just want the damn > > > thing merged upstream. But after several iterations to either fit one > > > or another maintainers taste, each rework ends up in that maintainer > > > saying: "Now that you've reworked the code, I still don't like it since > > > this no longer fits in my subsystem tree." > > FWIW, we asked Pawel to rework the sysreg and config parts of the > > vexpress driver, make it an actual MFD driver, and spread the remaining > > bits of the code into their respective subsystems. I don't think > > this is an eccentric requirement. > > I agree. However the code that Lorenzo just posted can't be deprived > of any more sysreg/config parts. Yes, and I appreciate Lorenzo's effort here. > Even the larger code you reviewed before is completely useless without > _additional_ drivers to go on top which are indeed waiting after this > code to be merged before they are submitted to their respective > subsystems. Right. And merging this code in the right place is exactly what we're doing here. > And those additional drivers are way more interesting than this dumb > register access arbitrator. Yes, they're a lot smarter hence the requirement to have them live into their respective subsystems where the right maintainer can provide relevant reviews on them. > > I don't mind merging Lorenzo's SPC driver as it is if he can explain to > > me how it will eventually evolve into an actual MFD driver. If that's > > not the case, I don't see how I could justify merging it through the > > MFD tree. > > Maybe the misunderstanding is about what actually is a MFD driver. That's possible. I agree it should be documented properly. > So I'll follow existing precedents in the kernel and move Lorenzo's code > to drivers/platform/vexpress/. Thanks for that. Cheers, Samuel. -- Intel Open Source Technology Centre http://oss.intel.com/ ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC PATCH v5 0/1] drivers: mfd: Versatile Express SPC support 2013-07-17 14:20 ` Pawel Moll 2013-07-17 15:57 ` Nicolas Pitre @ 2013-07-17 21:10 ` Samuel Ortiz 1 sibling, 0 replies; 20+ messages in thread From: Samuel Ortiz @ 2013-07-17 21:10 UTC (permalink / raw) To: Pawel Moll Cc: Nicolas Pitre, Lorenzo Pieralisi, linux-kernel, linux-arm-kernel, devicetree-discuss, Olof Johansson, Amit Kucheria, Jon Medhurst, Achin Gupta, Sudeep KarkadaNagesha Hi Pawel, On Wed, Jul 17, 2013 at 03:20:11PM +0100, Pawel Moll wrote: > On Wed, 2013-07-17 at 15:16 +0100, Nicolas Pitre wrote: > > On Wed, 17 Jul 2013, Pawel Moll wrote: > > > > > On Wed, 2013-07-17 at 13:33 +0100, Nicolas Pitre wrote: > > > > If this is really miscelaneous code that really doesn't fit > > > > anywhere else, it should rather go into drivers/misc/ as a last resort. > > > > > > Interestingly enough drivers/misc was my first choice for all the > > > vexpress stuff, but it wasn't received well... > > > > > > Anyway, the SPC driver as it is now seem to be a "power management > > > system driver". Maybe a relevant directory would be in place? Wouldn't > > > PSCI belong there as well? (there are two psci.c files in arch/arm and > > > arch/arm64, surprisingly similar ones ;-) > > > > > > The bottom line is: today it is not an MFD driver. > > > > But we know it will, right? So better save some churn by storing the > > initial code where it would end up anyway once complete. > > Not in that form, no. The code living in mfd will just register > mfd_cells while "functional" parts are going to live elsewhere. This is > how I understand what Samuel asked me to do and that's what is happening > to vexpress-sysreg now. Very good, I'll happily apply such changes. If I understand the IP correctly, the SPC driver will stay as a set of runtime APIs for controlling the SPC features. If that's the case, then misc sounds like a more appropriate place for this driver. drivers/power/ really is for power supplies and charging drivers. Cheers, Samuel. -- Intel Open Source Technology Centre http://oss.intel.com/ ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC PATCH v5 0/1] drivers: mfd: Versatile Express SPC support 2013-07-16 16:05 [RFC PATCH v5 0/1] drivers: mfd: Versatile Express SPC support Lorenzo Pieralisi ` (2 preceding siblings ...) 2013-07-17 9:18 ` Pawel Moll @ 2013-07-17 21:07 ` Samuel Ortiz 2013-07-18 9:28 ` Lorenzo Pieralisi 3 siblings, 1 reply; 20+ messages in thread From: Samuel Ortiz @ 2013-07-17 21:07 UTC (permalink / raw) To: Lorenzo Pieralisi Cc: linux-kernel, linux-arm-kernel, devicetree-discuss, Olof Johansson, Pawel Moll, Nicolas Pitre, Amit Kucheria, Jon Medhurst, Achin Gupta, Sudeep KarkadaNagesha Hi Lorenzo, On Tue, Jul 16, 2013 at 05:05:42PM +0100, Lorenzo Pieralisi wrote: > Hello, > > version v5 of VExpress SPC driver, please read on the changelog for major > changes and explanations. > > The probing scheme is unchanged, since after trying the early platform > devices approach it appeared that the end result was no better than the > current one. The only clean solution relies either on changing how > secondaries are brought up in the kernel (later than now) or enable > early platform device registration through DT. Please check this > thread for the related discussion: > > https://lists.ozlabs.org/pipermail/devicetree-discuss/2013-June/036542.html > > The interface was adapted to regmap and again reverted to old driver for > the following reasons: > > - Power down registers locking is hairy and requires arch spinlocks in > the MCPM back end to work properly, normal spinlocks cannot be used > - Regmap adds unnecessary code to manage SPC since it is just a bunch of > registers used to control power management flags, the overhead is just > not worth it (talking about power down registers, not the vexpress config > interface) > - The locking scheme behind regmap requires all registers in the map > to be protected with the same lock, which is not exactly what we want > here > - Given the reasons above, adding a regmap interface buys us nothing from > a driver readability and maintainability perspective (again just talking > about the power interface, a few registers) because for the SPC it would > simply not be used > > /drivers/mfd is probably not the right place for this code as it stands (but > probably will be when the entire driver, with DVFS and config interface, is > complete). Could you please elaborate on how will the SPC driver extend into an MFD driver? Cheers, Samuel. -- Intel Open Source Technology Centre http://oss.intel.com/ ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC PATCH v5 0/1] drivers: mfd: Versatile Express SPC support 2013-07-17 21:07 ` Samuel Ortiz @ 2013-07-18 9:28 ` Lorenzo Pieralisi 0 siblings, 0 replies; 20+ messages in thread From: Lorenzo Pieralisi @ 2013-07-18 9:28 UTC (permalink / raw) To: Samuel Ortiz Cc: linux-kernel, linux-arm-kernel, devicetree-discuss, Olof Johansson, Pawel Moll, Nicolas Pitre, Amit Kucheria, Jon Medhurst, Achin Gupta, Sudeep KarkadaNagesha Hi Samuel, On Wed, Jul 17, 2013 at 10:07:00PM +0100, Samuel Ortiz wrote: > Hi Lorenzo, > > On Tue, Jul 16, 2013 at 05:05:42PM +0100, Lorenzo Pieralisi wrote: > > Hello, > > > > version v5 of VExpress SPC driver, please read on the changelog for major > > changes and explanations. > > > > The probing scheme is unchanged, since after trying the early platform > > devices approach it appeared that the end result was no better than the > > current one. The only clean solution relies either on changing how > > secondaries are brought up in the kernel (later than now) or enable > > early platform device registration through DT. Please check this > > thread for the related discussion: > > > > https://lists.ozlabs.org/pipermail/devicetree-discuss/2013-June/036542.html > > > > The interface was adapted to regmap and again reverted to old driver for > > the following reasons: > > > > - Power down registers locking is hairy and requires arch spinlocks in > > the MCPM back end to work properly, normal spinlocks cannot be used > > - Regmap adds unnecessary code to manage SPC since it is just a bunch of > > registers used to control power management flags, the overhead is just > > not worth it (talking about power down registers, not the vexpress config > > interface) > > - The locking scheme behind regmap requires all registers in the map > > to be protected with the same lock, which is not exactly what we want > > here > > - Given the reasons above, adding a regmap interface buys us nothing from > > a driver readability and maintainability perspective (again just talking > > about the power interface, a few registers) because for the SPC it would > > simply not be used > > > > /drivers/mfd is probably not the right place for this code as it stands (but > > probably will be when the entire driver, with DVFS and config interface, is > > complete). > Could you please elaborate on how will the SPC driver extend into an MFD > driver? Reading through the thread I noticed Nico explained details properly, I was about to mention a possible solution to the directory issue but I am pretty sure that what he did will turn out for the best. Usually, or better, historically, these pieces of code that program PMICs lived in arch/arm/mach-* directories and that's something we could have done as well (create a static mapping and write some functions to peek and poke a few registers), but we thought that it was not the proper way to go. On top of that, the SPC is part of a component whose register space maps disparate functions (config interface for voltage, clocks, energy probes, frequency scaling and power states management) and basically that's the reason we struggled to partition it properly (with further complexity implied by the way requests - config and frequency scaling - have to be serialized). I hope the end result is reasonable, and overall I think it was a debate that was worth having. Thank you, Lorenzo ^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2013-07-18 9:28 UTC | newest] Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-07-16 16:05 [RFC PATCH v5 0/1] drivers: mfd: Versatile Express SPC support Lorenzo Pieralisi 2013-07-16 16:05 ` [RFC PATCH v5 1/1] drivers: mfd: vexpress: add Serial Power Controller (SPC) support Lorenzo Pieralisi 2013-07-16 20:05 ` Rob Herring 2013-07-16 23:32 ` Nicolas Pitre 2013-07-17 3:26 ` [RFC PATCH v5 0/1] drivers: mfd: Versatile Express SPC support Nicolas Pitre 2013-07-17 9:18 ` Pawel Moll 2013-07-17 10:44 ` Lorenzo Pieralisi 2013-07-17 12:33 ` Nicolas Pitre 2013-07-17 13:29 ` Pawel Moll 2013-07-17 14:16 ` Nicolas Pitre 2013-07-17 14:20 ` Pawel Moll 2013-07-17 15:57 ` Nicolas Pitre 2013-07-17 17:00 ` Russell King - ARM Linux 2013-07-17 18:29 ` Nicolas Pitre 2013-07-17 21:23 ` Samuel Ortiz 2013-07-17 22:22 ` Nicolas Pitre 2013-07-17 22:47 ` Samuel Ortiz 2013-07-17 21:10 ` Samuel Ortiz 2013-07-17 21:07 ` Samuel Ortiz 2013-07-18 9:28 ` Lorenzo Pieralisi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).