On Mon, Sep 12, 2011 at 11:46:34AM +0200, Sascha Hauer wrote:

On Tue, Sep 06, 2011 at 05:58:37PM +0800, Shawn Guo wrote:

quoted

It adds a number of core drivers support for imx6q, including clock,
General Power Controller (gpc), Multi Mode DDR Controller(mmdc) and
System Reset Controller (src).

Signed-off-by: Ranjani Vaidyanathan <redacted>
Signed-off-by: Shawn Guo <redacted>
---

diff --git a/arch/arm/mach-imx/src.c b/arch/arm/mach-imx/src.c
new file mode 100644
index 0000000..95fdd65
--- /dev/null
+++ b/arch/arm/mach-imx/src.c

@@ -0,0 +1,52 @@
+/*
+ * Copyright 2011 Freescale Semiconductor, Inc.
+ * Copyright 2011 Linaro Ltd.
+ *
+ * The code contained herein is licensed under the GNU General Public
+ * License. You may obtain a copy of the GNU General Public License
+ * Version 2 or later at the following locations:
+ *
+ * http://www.opensource.org/licenses/gpl-license.html
+ * http://www.gnu.org/copyleft/gpl.html
+ */
+
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <asm/unified.h>
+
+#define SRC_SCR				0x000
+#define SRC_GPR1			0x020
+#define BP_SRC_SCR_CORE1_RST		14
+#define BP_SRC_SCR_CORE1_ENABLE		22
+
+static void __iomem *src_base;
+
+void imx_enable_cpu(int cpu, bool enable)
+{
+	u32 mask, val;
+
+	mask = 1 << (BP_SRC_SCR_CORE1_ENABLE + cpu - 1);
+	val = __raw_readl(src_base + SRC_SCR);
+	val = enable ? val | mask : val & ~mask;
+	__raw_writel(val, src_base + SRC_SCR);
+}
+
+void imx_set_cpu_jump(int cpu, void *jump_addr)
+{
+	__raw_writel(BSYM(virt_to_phys(jump_addr)),
+		     src_base + SRC_GPR1 + cpu * 8);
+}
+
+static int __init imx_src_init(void)
+{
+	struct device_node *np;
+
+	np = of_find_compatible_node(NULL, NULL, "fsl,imx6q-src");
+	src_base = of_iomap(np, 0);
+	WARN_ON(!src_base);
+
+	return 0;
+}
+early_initcall(imx_src_init);

What I'm concerned about is that we carefully removed all assumptions
about which SoC the code runs on in the past. Now with this patchset
many of them come back. Here we have a initcall without any check
whether we really run on i.MX6.

The "check" has been done on Kconfig level as below.

config SOC_IMX6Q
        bool "i.MX6 Quad support"
        select HAVE_IMX_SRC

obj-$(CONFIG_HAVE_IMX_SRC) += src.o

Above we have a imx_enable_cpu function
but this is really a imx6 specific function.

I would say this is a imx smp specific function.

Nevertheless it is called
from generic code in arch/arm/mach-imx/platsmp.c.

We've been there before and it was hard to remove all the implicit
assumptions on which SoC we are on. Please do not reintroduce it.

We create platform driver for (talking to) specific device (IP block),
and select the driver for SoCs that have the device.  Anything wrong
with this approach?  Or I missed your point here?

-- 
Regards,
Shawn