ARM: vexpress: TC2 MCPM/SPC cleanups

Otherwise we have tc2_pm platform code calling into exported driver functions, which doesn't make sense at this level.

Signed-off-by Olof Johansson <[email protected]>

---
 arch/arm/mach-vexpress/Makefile                         |  2 +-
 .../platform/vexpress => arch/arm/mach-vexpress}/spc.c  |  1 -
 arch/arm/mach-vexpress/tc2_pm.c                         |  3 ++-
 drivers/platform/Kconfig                                |  3 ---
 drivers/platform/Makefile                               |  1 -
 drivers/platform/vexpress/Kconfig                       |  9 ---------
 drivers/platform/vexpress/Makefile                      |  1 -
 include/linux/vexpress.h                                | 17 -----------------
 8 files changed, 3 insertions(+), 34 deletions(-)
 rename {drivers/platform/vexpress => arch/arm/mach-vexpress}/spc.c (99%)
 delete mode 100644 drivers/platform/vexpress/Kconfig
 delete mode 100644 drivers/platform/vexpress/Makefile

arch/arm/mach-vexpress/Makefile +1 -1

index 0853da6..36ea824 100644
@@ -7,6 +7,6 @@ ccflags-$(CONFIG_ARCH_MULTIPLATFORM) := -I$(srctree)/$(src)/include \
 obj-y					:= v2m.o
 obj-$(CONFIG_ARCH_VEXPRESS_CA9X4)	+= ct-ca9x4.o
 obj-$(CONFIG_ARCH_VEXPRESS_DCSCB)	+= dcscb.o	dcscb_setup.o
-obj-$(CONFIG_ARCH_VEXPRESS_TC2_PM)	+= tc2_pm.o
+obj-$(CONFIG_ARCH_VEXPRESS_TC2_PM)	+= tc2_pm.o spc.o
 obj-$(CONFIG_SMP)			+= platsmp.o
 obj-$(CONFIG_HOTPLUG_CPU)		+= hotplug.o

arch/arm/mach-vexpress/spc.c renamed from drivers/platform/vexpress/spc.c +0 -1

similarity index 99%
rename from drivers/platform/vexpress/spc.c
rename to arch/arm/mach-vexpress/spc.c
index aa8c2a4..34e4bf4 100644
@@ -250,4 +250,3 @@ int __init ve_spc_init(void)
 
 	return ve_spc_init_status;
 }
-early_initcall(ve_spc_init);

arch/arm/mach-vexpress/tc2_pm.c +0 -0

index dfb55d4..79cf4bc 100644

arch/arm/mach-vexpress/tc2_pm.c +1 -0

+
 /*
  * We can't use regular spinlocks. In the switcher case, it is possible
  * for an outbound CPU to call power_down() after its inbound counterpart

drivers/platform/Kconfig +0 -3

index 449010e..57811ea 100644
@@ -4,6 +4,3 @@ endif
 if GOLDFISH
 source "drivers/platform/goldfish/Kconfig"
 endif
-if ARCH_VEXPRESS
-source "drivers/platform/vexpress/Kconfig"
-endif

drivers/platform/Makefile +0 -1

index c1173d6..8a44a4c 100644
@@ -5,4 +5,3 @@
 obj-$(CONFIG_X86)		+= x86/
 obj-$(CONFIG_OLPC)		+= olpc/
 obj-$(CONFIG_GOLDFISH)		+= goldfish/
-obj-$(CONFIG_ARCH_VEXPRESS)	+= vexpress/

drivers/platform/vexpress/Kconfig deleted +0 -9

deleted file mode 100644
index d09a7c4..0000000
@@ -1,9 +0,0 @@
-config VEXPRESS_SPC
-	bool "Versatile Express SPC driver support"
-	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 meant to be used by
-	  subsystem drivers allowing to program registers controlling
-	  low-level power management features like power down flags,
-	  global and per-cpu wake-up IRQs.

drivers/platform/vexpress/Makefile deleted +0 -1

deleted file mode 100644
index d31eca2..0000000
@@ -1 +0,0 @@
-obj-$(CONFIG_VEXPRESS_SPC)	+= spc.o

include/linux/vexpress.h +0 -17

index 3e35556..617c01b 100644
@@ -124,21 +124,4 @@ void vexpress_osc_of_setup(struct device_node *node);
 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

Signature · 1 line

1.8.1.192.gc4361b8

Quoted 1 line

+#include "spc.h"

Missing. Should we add it to mach-vexpress/include/mach ?

I have not sent a patch to check what we should do first and if Pawel is ok with these changes.

Quoted 3 lines · Lorenzo Pieralisi

Quoted 1 line

+#include "spc.h"

Missing. Should we add it to mach-vexpress/include/mach ?

Oops, missed the git add on it.

No, include/mach is going away on all multiplatform platforms. Since the exposed functions are only used in the other file in the same directory, local include file is the way to go.

Quoted 2 lines

I have not sent a patch to check what we should do first and if Pawel is ok with these changes.

Not sure I'm following, but see above.

I just wanted to say that sending a patch to fix the issue was inappropriate because it depended on where the spc.h has to be created and did not know if Pawel was ok with the idea of moving spc back to mach-vexpress.

I am ok with your changes, thank you very much.

It doesn't make sense to match this to the generic compatible value, since the code is quite specific to TC2 as it is.

Signed-off-by Olof Johansson <[email protected]>

---
 arch/arm/mach-vexpress/spc.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

arch/arm/mach-vexpress/spc.c +0 -3

index 34e4bf4..30cfa9f 100644
@@ -187,18 +187,14 @@ void ve_spc_powerdown(u32 cluster, bool enable)
 	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" },

arch/arm/mach-vexpress/spc.c +2 -2

-	if (!node)
+	if (!dn)
 		return -ENODEV;
 
 	info = kzalloc(sizeof(*info), GFP_KERNEL);

@@ -207,7 +203,7 @@ static int __init ve_spc_probe(void)
 		return -ENOMEM;
 	}
 
-	info->baseaddr = of_iomap(node, 0);
+	info->baseaddr = of_iomap(dn, 0);
 	if (!info->baseaddr) {
 		pr_err(SPCLOG "unable to ioremap memory\n");
 		ret = -ENXIO;

Signature · 1 line

1.8.1.192.gc4361b8

Quoted 19 lines · Olof Johansson

It doesn't make sense to match this to the generic compatible value, since the code is quite specific to TC2 as it is.

Signed-off-by Olof Johansson <[email protected]>

---
 arch/arm/mach-vexpress/spc.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

arch/arm/mach-vexpress/spc.c +0 -3

index 34e4bf4..30cfa9f 100644
@@ -187,18 +187,14 @@ void ve_spc_powerdown(u32 cluster, bool enable)
 	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" },

arch/arm/mach-vexpress/spc.c void ve_spc_powerdown(u32 cluster, bool enable) · line 195

Quoted 10 lines

arch/arm/mach-vexpress/spc.c +3 -4

-	{},
-};
-
 static int __init ve_spc_probe(void)
 {
 	int ret;
-	struct device_node *node = of_find_matching_node(NULL, ve_spc_ids);
+	struct device_node *dn;
+
+	dn = of_find_compatible_node(NULL, "arm,vexpress-spc,v2p-ca15_a7");

Missing a parameter, should be:

of_find_compatible_node(NULL, NULL, "arm,vexpress-spc,v2p-ca15_a7");

If my changes to the bindings are acceptable we must match the new string.

Here's a short series on top of Nico's TC2 MCPM series to move the SPC driver back under mach-vexpress where it should be, and a couple of tiny cleanups to the driver.

I'll pick this up together with the base branch in arm-soc for-next, but I haven't staged it in a next/* branch so it can be rebased/changed/dropped if needed.

There's still one thing left to iron out, and that's the actual binding for the "SPC" node. Since the spc code needs a non-zero SPC_BASE, it implies that there's more than just SPC described by the device node. So either the naming and description needs to be changed, or there's need for more than one device node to describe the other register ranges. The former might be easier in this case...

Pawel, I'd appreciate a quick test from you to make sure I didn't break anything since I don't have a way to boot this myself.

Quoted 15 lines · Olof Johansson

Here's a short series on top of Nico's TC2 MCPM series to move the SPC driver back under mach-vexpress where it should be, and a couple of tiny cleanups to the driver.

I'll pick this up together with the base branch in arm-soc for-next, but I haven't staged it in a next/* branch so it can be rebased/changed/dropped if needed.

There's still one thing left to iron out, and that's the actual binding for the "SPC" node. Since the spc code needs a non-zero SPC_BASE, it implies that there's more than just SPC described by the device node. So either the naming and description needs to be changed, or there's need for more than one device node to describe the other register ranges. The former might be easier in this case...

Quoted 2 lines

Pawel, I'd appreciate a quick test from you to make sure I didn't break anything since I don't have a way to boot this myself.

Thanks Olof.

Tested, a couple of fixes needed to compile your series, comments in the relative patches.

For the bindings, you are right, how about this (commit log written just for the sake of it, it should be squashed in Nico's original series) ?

For the bindings, you are right, how about this (commit log written just for the sake of it, it should be squashed in Nico's original series) ?

Quoted 2 lines · Lorenzo Pieralisi

For the bindings, you are right, how about this (commit log written just for the sake of it, it should be squashed in Nico's original series) ?

So I finally sat down and did what I should have done long time ago... Read the spec ;-)

SCC is (was?) mainly used to set initial conditions for the cores, interconnect and all the other bits and pieces inside the test chip. It could be considered an equivalent of the classic "boot mode" pins, but as test chips have many more of them comparing to the normal SOCs, the data is being "injected" to the chip in a serial fashion *before* the

main reset is being de-asserted. This is done by an external entity, namely the DCC (daughterboard configuration controller, so simply a microcontroller living next to the test chip). Why is this detail important at all? Because traditionally the SCC was *not* available in the normal memory map, otherwise it would be a perfect candidate for the . In order to change anything one had to go through the usual Versatile Express config infrastructure. Fortunately there was no need to do this at all...

And here comes the V2P-CA15_A7, also known as TC2 ;-) where the interface was re-(or ab-?)used as a "convenient" communication channel between the test chip and the microcontroller. And the SPC is even described as "being merged" with the SCC. Uh...

Now, the bottom line. How about keeping the driver look for "arm,vexpress-spc,v2p-ca15_a7" because it's a driver for the SPC bit after all and doing the following in the tree:

scc at 7fff0000 {
        compatible = "arm,vexpress-scc,v2p-ca15_a7", "arm,vexpress-scc";
        reg = <0x7fff0000 0x1000>;
        interrupts = <0 95 4>;

        spc at b00 {
                compatible = "arm,vexpress-spc,v2p-ca15_a7", "arm,vexpress-spc";
                reg = <0xb00 0x100>;
        };
};

This, I believe, would represent the actual situation, require no change in the driver (except for the retirement of SPC_BASE which is good :-) and allowed as, if and when necessary, to drive the SCC as a MFD/syscon device.

Does it make some sense?

Quoted 6 lines · Pawel Moll

Quoted 2 lines · Lorenzo Pieralisi

For the bindings, you are right, how about this (commit log written just for the sake of it, it should be squashed in Nico's original series) ?

So I finally sat down and did what I should have done long time ago... Read the spec ;-)

Uh... Ok. Biting my tongue. :)

Is the spec public, per chance?

Quoted 5 lines

SCC is (was?) mainly used to set initial conditions for the cores, interconnect and all the other bits and pieces inside the test chip. It could be considered an equivalent of the classic "boot mode" pins, but as test chips have many more of them comparing to the normal SOCs, the data is being "injected" to the chip in a serial fashion *before* the

Quoted 8 lines

main reset is being de-asserted. This is done by an external entity, namely the DCC (daughterboard configuration controller, so simply a microcontroller living next to the test chip). Why is this detail important at all? Because traditionally the SCC was *not* available in the normal memory map, otherwise it would be a perfect candidate for the . In order to change anything one had to go through the usual Versatile Express config infrastructure. Fortunately there was no need to do this at all...

Vexpress is such a hack. :)

It's unfortunate that it ends up being the
lead platform on these kind of technologies, in some ways.

I think it makes sense to hide some of this ick in mach-vexpress, I don't think we should ever aim to empty it out.

Not based on how messy the platform architecture is.

I'd rather hide it in there than pollute
other parts of the kernel. Or at least, we should keep it as an option in some cases such as these -- there might be other parts that do make sense to move out to generic kernel.

Quoted 25 lines

And here comes the V2P-CA15_A7, also known as TC2 ;-) where the interface was re-(or ab-?)used as a "convenient" communication channel between the test chip and the microcontroller. And the SPC is even described as "being merged" with the SCC. Uh...

Now, the bottom line. How about keeping the driver look for "arm,vexpress-spc,v2p-ca15_a7" because it's a driver for the SPC bit after all and doing the following in the tree:

scc at 7fff0000 {
        compatible = "arm,vexpress-scc,v2p-ca15_a7", "arm,vexpress-scc";
        reg = <0x7fff0000 0x1000>;
        interrupts = <0 95 4>;

        spc at b00 {
                compatible = "arm,vexpress-spc,v2p-ca15_a7", "arm,vexpress-spc";
                reg = <0xb00 0x100>;
        };
};

This, I believe, would represent the actual situation, require no change in the driver (except for the retirement of SPC_BASE which is good :-) and allowed as, if and when necessary, to drive the SCC as a MFD/syscon device.

Quoted 1 line

Does it make some sense?

It does, but you need to setup the scc as a bus (with ranges, etc) for that binding to work.

I pushed the fixed-up set of patches to the branch in arm-soc, so you can find them there (olof/vexpress). Sounds like this isn't quite ready to merge if these things still need to be worked out. So feel free to take my patches, change them up as needed and send a fresh copy.

I dare to disagree. And the reason is simple - arch/arm64. It's vexpress as well, and we'll be in the same position.

Quoted 2 lines

I dare to disagree. And the reason is simple - arch/arm64. It's vexpress as well, and we'll be in the same position.

Actually, no -- PSCI will be mandatory there so there will be no need to have these kernel components.

Quoted 6 lines · Pawel Moll

Quoted 2 lines · Lorenzo Pieralisi

For the bindings, you are right, how about this (commit log written just for the sake of it, it should be squashed in Nico's original series) ?

So I finally sat down and did what I should have done long time ago... Read the spec ;-)

Uh... Ok. Biting my tongue. :)

Is the spec public, per chance?

Quoted 5 lines

SCC is (was?) mainly used to set initial conditions for the cores, interconnect and all the other bits and pieces inside the test chip. It could be considered an equivalent of the classic "boot mode" pins, but as test chips have many more of them comparing to the normal SOCs, the data is being "injected" to the chip in a serial fashion *before* the

Quoted 8 lines

main reset is being de-asserted. This is done by an external entity, namely the DCC (daughterboard configuration controller, so simply a microcontroller living next to the test chip). Why is this detail important at all? Because traditionally the SCC was *not* available in the normal memory map, otherwise it would be a perfect candidate for the . In order to change anything one had to go through the usual Versatile Express config infrastructure. Fortunately there was no need to do this at all...

Vexpress is such a hack. :) It's unfortunate that it ends up being the lead platform on these kind of technologies, in some ways.

I think it makes sense to hide some of this ick in mach-vexpress, I don't think we should ever aim to empty it out. Not based on how messy the platform architecture is. I'd rather hide it in there than pollute other parts of the kernel. Or at least, we should keep it as an option in some cases such as these -- there might be other parts that do make sense to move out to generic kernel.

Quoted 20 lines

And here comes the V2P-CA15_A7, also known as TC2 ;-) where the interface was re-(or ab-?)used as a "convenient" communication channel between the test chip and the microcontroller. And the SPC is even described as "being merged" with the SCC. Uh...

Now, the bottom line. How about keeping the driver look for "arm,vexpress-spc,v2p-ca15_a7" because it's a driver for the SPC bit after all and doing the following in the tree:

scc at 7fff0000 {
        compatible = "arm,vexpress-scc,v2p-ca15_a7", "arm,vexpress-scc";
        reg = <0x7fff0000 0x1000>;
        interrupts = <0 95 4>;

        spc at b00 {
                compatible = "arm,vexpress-spc,v2p-ca15_a7", "arm,vexpress-spc";
                reg = <0xb00 0x100>;
        };
};

Quoted 6 lines

This, I believe, would represent the actual situation, require no change in the driver (except for the retirement of SPC_BASE which is good :-) and allowed as, if and when necessary, to drive the SCC as a MFD/syscon device.

Does it make some sense?

It does, but you need to setup the scc as a bus (with ranges, etc) for that binding to work.

I pushed the fixed-up set of patches to the branch in arm-soc, so you can find them there (olof/vexpress). Sounds like this isn't quite ready to merge if these things still need to be worked out. So feel free to take my patches, change them up as needed and send a fresh copy.

Quoted 2 lines · Lorenzo Pieralisi

For the bindings, you are right, how about this (commit log written just for the sake of it, it should be squashed in Nico's original series) ?

So I finally sat down and did what I should have done long time ago... Read the spec ;-)

SCC is (was?) mainly used to set initial conditions for the cores, interconnect and all the other bits and pieces inside the test chip. It could be considered an equivalent of the classic "boot mode" pins, but as test chips have many more of them comparing to the normal SOCs, the data is being "injected" to the chip in a serial fashion *before* the main reset is being de-asserted. This is done by an external entity, namely the DCC (daughterboard configuration controller, so simply a microcontroller living next to the test chip). Why is this detail important at all? Because traditionally the SCC was *not* available in the normal memory map, otherwise it would be a perfect candidate for the . In order to change anything one had to go through the usual Versatile Express config infrastructure. Fortunately there was no need to do this at all...

And here comes the V2P-CA15_A7, also known as TC2 ;-) where the interface was re-(or ab-?)used as a "convenient" communication channel between the test chip and the microcontroller. And the SPC is even described as "being merged" with the SCC. Uh...

Now, the bottom line. How about keeping the driver look for "arm,vexpress-spc,v2p-ca15_a7" because it's a driver for the SPC bit after all and doing the following in the tree:

scc at 7fff0000 {
	compatible = "arm,vexpress-scc,v2p-ca15_a7", "arm,vexpress-scc";
	reg = <0x7fff0000 0x1000>;
	interrupts = <0 95 4>;

	spc at b00 {
		compatible = "arm,vexpress-spc,v2p-ca15_a7", "arm,vexpress-spc";
		reg = <0xb00 0x100>;
	};
};

This, I believe, would represent the actual situation, require no change in the driver (except for the retirement of SPC_BASE which is good :-) and allowed as, if and when necessary, to drive the SCC as a MFD/syscon device.

Does it make some sense?

Quoted 4 lines

And here comes the V2P-CA15_A7, also known as TC2 ;-) where the interface was re-(or ab-?)used as a "convenient" communication channel between the test chip and the microcontroller. And the SPC is even described as "being merged" with the SCC. Uh...

Yes, the "merging" of SCC and SPC is the root cause of this hopefully happy ended story, they lumped together pieces of HW that should have been kept separate.

Quoted 15 lines

Now, the bottom line. How about keeping the driver look for "arm,vexpress-spc,v2p-ca15_a7" because it's a driver for the SPC bit after all and doing the following in the tree:

scc at 7fff0000 {
	compatible = "arm,vexpress-scc,v2p-ca15_a7", "arm,vexpress-scc";
	reg = <0x7fff0000 0x1000>;
	interrupts = <0 95 4>;

	spc at b00 {
		compatible = "arm,vexpress-spc,v2p-ca15_a7", "arm,vexpress-spc";
		reg = <0xb00 0x100>;
	};
};

Quoted 4 lines

This, I believe, would represent the actual situation, require no change in the driver (except for the retirement of SPC_BASE which is good :-) and allowed as, if and when necessary, to drive the SCC as a MFD/syscon device.

I do need some SCC registers to check the cluster ID against the MPIDR in order to carry out powerdown operations. So if we do what you suggest we have to change the driver, I would avoid doing that at this stage.