Re: [RFC PATCH v3 2/2] drivers: mfd: vexpress: add Serial Power Controller (SPC) support
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 0x7FFF 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH v3 2/2] drivers: mfd: vexpress: add Serial Power Controller (SPC) support
On Thu, 2013-06-13 at 23:52 +0100, Olof Johansson wrote: > > + reg = <0 0x7FFF 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ł -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH v3 2/2] drivers: mfd: vexpress: add Serial Power Controller (SPC) support
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 ".
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
> > Cc: Pawel Moll
> > Cc: Nicolas Pitre
> > Cc: Amit Kucheria
> > Cc: Jon Medhurst
> > Signed-off-by: Achin Gupta
> > Signed-off-by: Lorenzo Pieralisi
> > Signed-off-by: Sudeep KarkadaNagesha
> > Reviewed-by: Nicolas Pitre
> > ---
> > 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 000..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:
> > + Definition: must be
> > + "arm,vexpress-spc,v2p-ca15_a7","arm,vexpress-spc"
> > + - reg:
> > + Usage: required
> > + Value type:
> > + Definition: A standard property that specifies the base
> > address
> > + and the size of the SPC address space
> > + - interrupts:
> > + Usage: required
> > + Value type:
> > + Definition: SPC interrupt configuration. A standard property
> > + that follows ePAPR interrupts specifications
> > +
> > +Example:
> > +
> > +spc: spc@7fff {
> > + compatible = "arm,vexpress-spc,v2p-ca15_a7","arm,vexpress-spc";
>
> Nit: space after comma between strings.
>
> > + reg = <0 0x7FFF 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.
> > +
> >
Re: [RFC PATCH v3 2/2] drivers: mfd: vexpress: add Serial Power Controller (SPC) support
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 sa...@linux.intel.com
Cc: Pawel Moll pawel.m...@arm.com
Cc: Nicolas Pitre nicolas.pi...@linaro.org
Cc: Amit Kucheria amit.kuche...@linaro.org
Cc: Jon Medhurst t...@linaro.org
Signed-off-by: Achin Gupta achin.gu...@arm.com
Signed-off-by: Lorenzo Pieralisi lorenzo.pieral...@arm.com
Signed-off-by: Sudeep KarkadaNagesha sudeep.karkadanage...@arm.com
Reviewed-by: Nicolas Pitre n...@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 000..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@7fff {
+ compatible = arm,vexpress-spc,v2p-ca15_a7,arm,vexpress-spc;
Nit: space after comma between strings.
+ reg = 0 0x7FFF 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
Re: [RFC PATCH v3 2/2] drivers: mfd: vexpress: add Serial Power Controller (SPC) support
On Thu, 2013-06-13 at 23:52 +0100, Olof Johansson wrote: + reg = 0 0x7FFF 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ł -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH v3 2/2] drivers: mfd: vexpress: add Serial Power Controller (SPC) support
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 0x7FFF 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH v3 2/2] drivers: mfd: vexpress: add Serial Power Controller (SPC) support
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH v3 2/2] drivers: mfd: vexpress: add Serial Power Controller (SPC) support
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 ".
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
> Cc: Pawel Moll
> Cc: Nicolas Pitre
> Cc: Amit Kucheria
> Cc: Jon Medhurst
> Signed-off-by: Achin Gupta
> Signed-off-by: Lorenzo Pieralisi
> Signed-off-by: Sudeep KarkadaNagesha
> Reviewed-by: Nicolas Pitre
> ---
> 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 000..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:
> + Definition: must be
> + "arm,vexpress-spc,v2p-ca15_a7","arm,vexpress-spc"
> + - reg:
> + Usage: required
> + Value type:
> + Definition: A standard property that specifies the base address
> + and the size of the SPC address space
> + - interrupts:
> + Usage: required
> + Value type:
> + Definition: SPC interrupt configuration. A standard property
> + that follows ePAPR interrupts specifications
> +
> +Example:
> +
> +spc: spc@7fff {
> + compatible = "arm,vexpress-spc,v2p-ca15_a7","arm,vexpress-spc";
Nit: space after comma between strings.
> + reg = <0 0x7FFF 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) +=
Re: [RFC PATCH v3 2/2] drivers: mfd: vexpress: add Serial Power Controller (SPC) support
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 = _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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH v3 2/2] drivers: mfd: vexpress: add Serial Power Controller (SPC) support
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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH v3 2/2] drivers: mfd: vexpress: add Serial Power Controller (SPC) support
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 sa...@linux.intel.com
Cc: Pawel Moll pawel.m...@arm.com
Cc: Nicolas Pitre nicolas.pi...@linaro.org
Cc: Amit Kucheria amit.kuche...@linaro.org
Cc: Jon Medhurst t...@linaro.org
Signed-off-by: Achin Gupta achin.gu...@arm.com
Signed-off-by: Lorenzo Pieralisi lorenzo.pieral...@arm.com
Signed-off-by: Sudeep KarkadaNagesha sudeep.karkadanage...@arm.com
Reviewed-by: Nicolas Pitre n...@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 000..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@7fff {
+ compatible = arm,vexpress-spc,v2p-ca15_a7,arm,vexpress-spc;
Nit: space after comma between strings.
+ reg = 0 0x7FFF 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 @@
Re: [RFC PATCH v3 2/2] drivers: mfd: vexpress: add Serial Power Controller (SPC) support
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH v3 2/2] drivers: mfd: vexpress: add Serial Power Controller (SPC) support
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 = _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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH v3 2/2] drivers: mfd: vexpress: add Serial Power Controller (SPC) support
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 = _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/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH v3 2/2] drivers: mfd: vexpress: add Serial Power Controller (SPC) support
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/
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH v3 2/2] drivers: mfd: vexpress: add Serial Power Controller (SPC) support
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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
