Add support for the SPMP8000 SoC and Letcool board

This is unneeded and causes an abort on the SPMP8000 platform.

Signed-off-by Zoltan Devai <[email protected]>

---
?arch/arm/common/vic.c | ? ?1 -
?1 files changed, 0 insertions(+), 1 deletions(-)

arch/arm/common/vic.c +0 -0

index b22b83d..651c77d 100644

2011/10/9 Zoltan Devai <[email protected]>:

Quoted 12 lines

This is unneeded and causes an abort on the SPMP8000 platform.

Signed-off-by Zoltan Devai <[email protected]>

---
?arch/arm/common/vic.c | ? ?1 -
?1 files changed, 0 insertions(+), 1 deletions(-)

arch/arm/common/vic.c +0 -0

index b22b83d..651c77d 100644

arch/arm/common/vic.c static void __init vic_disable(void __iomem *base) · line 279

Quoted 7 lines

arch/arm/common/vic.c +0 -1

@@ -274,7 +274,6 @@ static void __init vic_disable(void __iomem *base)
 ? ? ? ?writel(0, base + VIC_INT_SELECT);
 ? ? ? ?writel(0, base + VIC_INT_ENABLE);
 ? ? ? ?writel(~0, base + VIC_INT_ENABLE_CLEAR);
- ? ? ? writel(0, base + VIC_IRQ_STATUS);
 ? ? ? ?writel(0, base + VIC_ITCR);
 ? ? ? ?writel(~0, base + VIC_INT_SOFT_CLEAR);
 ?}

Indeed.

This patch stands on its own, please put it into Russell's patch tracker at http://www.arm.linux.org.uk/developer/patches/

Acked-by Linus Walleij <[email protected]>

arch/arm/mach-spmp8000/Makefile new file +8 -0

+obj-y 			:= core.o
+obj-y			+= timer.o
+obj-y			+= pinmux.o
+obj-y			+= clock.o
+obj-y			+= clkdev.o
+obj-y			+= board_letcool.o
+obj-y			+= adc.o
+obj-y			+= pwm.o

arch/arm/mach-spmp8000/core.c new file +41 -0

new file mode 100644
index 0000000..ba05614
@@ -0,0 +1,103 @@
+/*
+ * SPMP8000 machines core functions
+ *
+ * Copyright (C) 2011 Zoltan Devai <[email protected]>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <asm/mach/map.h>
+#include <asm/hardware/vic.h>
+#include <mach/hardware.h>
+#include <mach/scu.h>
+
+/* Static mappings:
+ * SCU: timer needs clk support which is inited in init_early
+ * UART: needed for earlyprintk
+ */
+static struct map_desc io_desc[] __initdata = {
+	{
+		.virtual	= IO_ADDRESS(SPMP8000_SCU_A_BASE),
+		.pfn		= __phys_to_pfn(SPMP8000_SCU_A_BASE),
+		.length		= SPMP8000_SCU_A_SIZE,
+		.type		= MT_DEVICE,
+	}, {
+		.virtual	= IO_ADDRESS(SPMP8000_SCU_B_BASE),
+		.pfn		= __phys_to_pfn(SPMP8000_SCU_B_BASE),
+		.length		= SPMP8000_SCU_B_SIZE,
+		.type		= MT_DEVICE,
+	}, {
+		.virtual	= IO_ADDRESS(SPMP8000_SCU_C_BASE),
+		.pfn		= __phys_to_pfn(SPMP8000_SCU_C_BASE),
+		.length		= SPMP8000_SCU_C_SIZE,
+		.type		= MT_DEVICE,
+	},
+#ifdef CONFIG_DEBUG_LL

arch/arm/mach-spmp8000/core.c new file +10 -0

+/* Make the lookup of SCU base and clk enable registers easy for the clk
+ * code, as they are not at the same offset in each controller */
+struct scu_reg_t scu_regs[3] = {
+	{	/* SCU_A */
+		.base = IO_PTR(SPMP8000_SCU_A_BASE),
+		.clken = IO_PTR(SPMP8000_SCU_A_BASE) + SCU_A_PERI_CLKEN,
+	},
+	{	/* SCU_B */
+		.base = IO_PTR(SPMP8000_SCU_B_BASE),
+		.clken = IO_PTR(SPMP8000_SCU_B_BASE) + SCU_B_PERI_CLKEN,

arch/arm/mach-spmp8000/core.c new file +21 -0

+	"93010000.dma",		/* APBDMA_A */
+	"92b00000.dma",		/* APBDMA_C */
+};
+
+void __init spmp8000_map_io(void)
+{
+	iotable_init(io_desc, ARRAY_SIZE(io_desc));
+}
+
+void __init spmp8000_init_irq(void)
+{
+	struct device_node *np;
+	int ret;
+
+	np = of_find_compatible_node(NULL, NULL, "arm,pl192");
+	if (!np)
+		panic("Can't find VIC0 interrupt controller\n");
+
+	ret = vic_of_init(np, NULL);
+	if (ret)
+		panic("Can't init VIC0 interrupt controller\n");

arch/arm/mach-spmp8000/core.c new file +1 -0

+}

arch/arm/mach-spmp8000/include/mach/core.h new file +6 -0

new file mode 100644
index 0000000..fa1d522
@@ -0,0 +1,29 @@
+/*
+ * SPMP8000 generic includes
+ *
+ * Copyright (C) 2011 Zoltan Devai <[email protected]>
+ *
+ * This file is licensed under the terms of the GNU General Public

arch/arm/mach-spmp8000/include/mach/core.h new file +13 -0

+extern struct sys_timer spmp8000_sys_timer;
+
+/* clkdev.c */
+extern void __init spmp8000_init_clkdev(void);
+extern void spmp8000_update_arm_freqs(void); /* For cpufreq driver */
+
+/* pinmux.c */
+extern void spmp8000_pinmux_pgc_set(int pc, unsigned int conf);
+extern unsigned int spmp8000_pinmux_pgc_get(int pc);
+extern void spmp8000_pinmux_pgs_set(unsigned int pg, unsigned int func);
+extern int spmp8000_pinmux_pgs_get(int pg);
+
+#endif /* __MACH_SPMP8000_GENERIC_H__ */

arch/arm/mach-spmp8000/include/mach/dma.h new file +1 -0

new file mode 100644
index 0000000..44bc9f2
@@ -0,0 +1,45 @@
+/*

arch/arm/mach-spmp8000/include/mach/dma.h new file +7 -0

+
+#ifndef __MACH_SPMP8000_DMA_H__
+#define __MACH_SPMP8000_DMA_H__
+
+enum spmp8000_dma_controller {
+	SPMP8000_APBDMA_A	= 0,
+	SPMP8000_APBDMA_C,

arch/arm/mach-spmp8000/include/mach/dma.h new file +4 -0

+
+/* Driver parameters */
+#define SPMP8000_APBDMA_MAX_PERIODS	64
+

arch/arm/mach-spmp8000/include/mach/gpio.h new file +9 -0

new file mode 100644
index 0000000..3eafac2
@@ -0,0 +1,21 @@
+/*
+ * SPMP8000 machines gpio support
+ *
+ * Copyright (C) 2011 Zoltan Devai <[email protected]>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */

arch/arm/mach-spmp8000/include/mach/irqs.h new file +9 -0

new file mode 100644
index 0000000..7f305d3
@@ -0,0 +1,21 @@
+/*
+ * SPMP8000 irqs.h
+ *
+ * Copyright (C) 2011 Zoltan Devai <[email protected]>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */

arch/arm/mach-spmp8000/include/mach/regs-timer.h new file +1 -0

new file mode 100644
index 0000000..90735df
@@ -0,0 +1,32 @@
+/*

arch/arm/mach-spmp8000/include/mach/scu.h new file +12 -0

@@ -0,0 +1,146 @@
+/*
+ * SPMP8000 System Control Unit register definitions
+ * SCUs are a random collection of reset, clock, gpio, pinmux, etc. controllers
+ * so that these definitions are needed at several places.
+ *
+ * Copyright (C) 2011 Zoltan Devai <[email protected]>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+

arch/arm/mach-spmp8000/include/mach/system.h new file +28 -0

new file mode 100644
index 0000000..be53ff3
@@ -0,0 +1,45 @@
+/*
+ * SPMP8000 system.h
+ *
+ * Copyright (C) 2011 Zoltan Devai <[email protected]>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#ifndef __MACH_SPMP8000_SYSTEM_H__
+#define __MACH_SPMP8000_SYSTEM_H__
+
+#include <linux/kernel.h>
+#include <linux/io.h>
+
+#define SPMP8000_WDT_BASE	0x90001000
+#define SPMP8000_WDT_SIZE	0x1000
+
+#define SPMP8000_WDT_CTR	0x00
+#define SPMP8000_WDT_CTR_TE	BIT(0)
+#define SPMP8000_WDT_CTR_RE	BIT(3)
+
+static inline void arch_idle(void)
+{
+	cpu_do_idle();
+}
+

arch/arm/mach-spmp8000/include/mach/system.h new file +5 -0

+
+	/* Trigger a watchdog reset */
+	writel(SPMP8000_WDT_CTR_RE | SPMP8000_WDT_CTR_TE,
+			base + SPMP8000_WDT_CTR);
+}

arch/arm/mach-spmp8000/timer.c new file +34 -0

new file mode 100644
index 0000000..7d5d0eb
@@ -0,0 +1,160 @@
+/*
+ * SPMP8000 machines timer functions
+ *
+ * Copyright (C) 2011 Zoltan Devai <[email protected]>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/kernel.h>
+#include <linux/irq.h>
+#include <linux/io.h>
+#include <linux/err.h>
+#include <linux/bitops.h>
+#include <linux/interrupt.h>
+#include <linux/clockchips.h>
+#include <linux/clk.h>
+#include <asm/mach/time.h>
+
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+
+#include <mach/hardware.h>
+#include <mach/regs-timer.h>
+#include <mach/irqs.h>
+
+static unsigned long clkrate;
+static const unsigned int tickrate = 1012500;
+
+/* The TIMER_B block is used as system timer
+ * T2: Clocksource
+ * T1: Clockevent

arch/arm/mach-spmp8000/timer.c new file +7 -0

+static void __iomem *cs_base;
+static void __iomem *ce_base;
+
+static void tmrb_set_mode(enum clock_event_mode mode,
+				struct clock_event_device *dev)
+{
+	switch (mode) {

arch/arm/mach-spmp8000/timer.c new file +50 -0

+	case CLOCK_EVT_MODE_SHUTDOWN:
+	case CLOCK_EVT_MODE_UNUSED:
+		/* Disable timer */
+		writel(0, ce_base + SPMP8000_TMRB_CTR);
+		break;
+	case CLOCK_EVT_MODE_RESUME:
+		BUG();
+		break;
+	}
+}
+
+static struct clock_event_device tmrb1_clkevt = {
+	.name		= "tmrb1",
+	.features	= CLOCK_EVT_FEAT_PERIODIC,
+	.rating		= 200,
+	.set_mode	= tmrb_set_mode,
+};
+
+static irqreturn_t tmrb1_isr(int irq, void *dev_id)
+{
+	tmrb1_clkevt.event_handler(&tmrb1_clkevt);
+
+	/* Clear IRQ */
+	writel(0, ce_base + SPMP8000_TMRB_ISR);
+
+	return IRQ_HANDLED;
+};
+
+static struct irqaction tmrb1_irq = {
+	.name		= "tmrb1_irq",
+	.flags		= IRQF_TIMER | IRQF_IRQPOLL,
+	.handler	= tmrb1_isr,
+};
+
+static void __init spmp8000_sys_timer_init(void)
+{
+	struct device_node *np;
+	unsigned int irq;
+	struct clk *clk;
+	void *tmrb_base;
+
+	np = of_find_compatible_node(NULL, NULL, "sunplus,spmp8000-timer");
+	if (!np)
+		panic("spmp8000: unable to find timer node in dtb\n");
+
+	tmrb_base = of_iomap(np, 0);
+	if (!tmrb_base)
+		panic("spmp8000: unable to map timer cpu registers\n");
+
+	irq = of_irq_to_resource(np, CE_TIMER, NULL);

arch/arm/mach-spmp8000/timer.c new file +36 -0

+	clkrate = clk_get_rate(clk);
+
+	/* Clocksource */
+	/* Disable timer */
+	writel(0, cs_base + SPMP8000_TMRB_CTR);
+
+	/* Reset counter value
+	 * Not really possible unless setting end-1 LDR value and waiting
+	 * until the counter reaches that */
+
+	/* Prescale timer */
+	writel((clkrate / tickrate) - 1, cs_base + SPMP8000_TMRB_PSR);
+
+	/* Register the clocksource */
+	clocksource_mmio_init(cs_base + SPMP8000_TMRB_VLR, "tmrb2",
+				tickrate, 200, 16, clocksource_mmio_readl_up);
+
+	/* Configure as free running (0 - 0xFFFF), up counter, enable timer */
+	writel(SPMP8000_TMRB_CTR_TE | SPMP8000_TMRB_CTR_UD,
+		cs_base + SPMP8000_TMRB_CTR);
+
+	/* Clockevent */
+	setup_irq(irq, &tmrb1_irq);
+
+	/* Disable timer */
+	writel(0, ce_base + SPMP8000_TMRB_CTR);
+
+	/* Prescale timer */
+	writel((clkrate / tickrate) - 1, ce_base + SPMP8000_TMRB_PSR);
+
+	clockevents_register_device(&tmrb1_clkevt);
+}
+
+struct sys_timer spmp8000_sys_timer = {
+	.init		= spmp8000_sys_timer_init,
+};

A couple of minor comments inline but this looks really good!

[...]

arch/arm/mach-spmp8000/Makefile line 3

Quoted 9 lines

arch/arm/mach-spmp8000/Makefile new file +3 -0

new file mode 100644
index 0000000..9d35cca
@@ -0,0 +1,11 @@
+#
+# Makefile for the linux kernel.
+#

Nit: I don't think this comment adds a great deal of value.

Quoted 62 lines

arch/arm/mach-spmp8000/Makefile new file +8 -0

+obj-y ? ? ? ? ? ? ? ? ? ? ? ?:= core.o
+obj-y ? ? ? ? ? ? ? ? ? ? ? ?+= timer.o
+obj-y ? ? ? ? ? ? ? ? ? ? ? ?+= pinmux.o
+obj-y ? ? ? ? ? ? ? ? ? ? ? ?+= clock.o
+obj-y ? ? ? ? ? ? ? ? ? ? ? ?+= clkdev.o
+obj-y ? ? ? ? ? ? ? ? ? ? ? ?+= board_letcool.o
+obj-y ? ? ? ? ? ? ? ? ? ? ? ?+= adc.o
+obj-y ? ? ? ? ? ? ? ? ? ? ? ?+= pwm.o

arch/arm/mach-spmp8000/core.c new file +47 -0

new file mode 100644
index 0000000..ba05614
@@ -0,0 +1,103 @@
+/*
+ * SPMP8000 machines core functions
+ *
+ * Copyright (C) 2011 Zoltan Devai <[email protected]>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <asm/mach/map.h>
+#include <asm/hardware/vic.h>
+#include <mach/hardware.h>
+#include <mach/scu.h>
+
+/* Static mappings:
+ * SCU: timer needs clk support which is inited in init_early
+ * UART: needed for earlyprintk
+ */
+static struct map_desc io_desc[] __initdata = {
+ ? ? {
+ ? ? ? ? ? ? .virtual ? ? ? ?= IO_ADDRESS(SPMP8000_SCU_A_BASE),
+ ? ? ? ? ? ? .pfn ? ? ? ? ? ?= __phys_to_pfn(SPMP8000_SCU_A_BASE),
+ ? ? ? ? ? ? .length ? ? ? ? = SPMP8000_SCU_A_SIZE,
+ ? ? ? ? ? ? .type ? ? ? ? ? = MT_DEVICE,
+ ? ? }, {
+ ? ? ? ? ? ? .virtual ? ? ? ?= IO_ADDRESS(SPMP8000_SCU_B_BASE),
+ ? ? ? ? ? ? .pfn ? ? ? ? ? ?= __phys_to_pfn(SPMP8000_SCU_B_BASE),
+ ? ? ? ? ? ? .length ? ? ? ? = SPMP8000_SCU_B_SIZE,
+ ? ? ? ? ? ? .type ? ? ? ? ? = MT_DEVICE,
+ ? ? }, {
+ ? ? ? ? ? ? .virtual ? ? ? ?= IO_ADDRESS(SPMP8000_SCU_C_BASE),
+ ? ? ? ? ? ? .pfn ? ? ? ? ? ?= __phys_to_pfn(SPMP8000_SCU_C_BASE),
+ ? ? ? ? ? ? .length ? ? ? ? = SPMP8000_SCU_C_SIZE,
+ ? ? ? ? ? ? .type ? ? ? ? ? = MT_DEVICE,
+ ? ? },
+#ifdef CONFIG_DEBUG_LL
+ ? ? {
+ ? ? ? ? ? ? .virtual ? ? ? ?= IO_ADDRESS(SPMP8000_UARTC0_BASE),
+ ? ? ? ? ? ? .pfn ? ? ? ? ? ?= __phys_to_pfn(SPMP8000_UARTC0_BASE),
+ ? ? ? ? ? ? .length ? ? ? ? = SPMP8000_UARTC0_SIZE,
+ ? ? ? ? ? ? .type ? ? ? ? ? = MT_DEVICE,
+ ? ? },

arch/arm/mach-spmp8000/core.c

Quoted 4 lines

arch/arm/mach-spmp8000/core.c new file +4 -0

+#endif
+};
+
+#define IO_PTR(x) ? ?((void __iomem *)IO_ADDRESS(x))

Generally IO_ADDRESS returns a void __iomem pointer so it would be easier if you could have the cast in there (though it may need to be enclosed in #ifndef __ASSEMBLY__ conditionals).

Quoted 14 lines

arch/arm/mach-spmp8000/core.c new file +14 -0

+/* Make the lookup of SCU base and clk enable registers easy for the clk
+ * code, as they are not at the same offset in each controller */
+struct scu_reg_t scu_regs[3] = {
+ ? ? { ? ? ? /* SCU_A */
+ ? ? ? ? ? ? .base = IO_PTR(SPMP8000_SCU_A_BASE),
+ ? ? ? ? ? ? .clken = IO_PTR(SPMP8000_SCU_A_BASE) + SCU_A_PERI_CLKEN,
+ ? ? },
+ ? ? { ? ? ? /* SCU_B */
+ ? ? ? ? ? ? .base = IO_PTR(SPMP8000_SCU_B_BASE),
+ ? ? ? ? ? ? .clken = IO_PTR(SPMP8000_SCU_B_BASE) + SCU_B_PERI_CLKEN,
+ ? ? },
+ ? ? { ? ? ? /* SCU_C */
+ ? ? ? ? ? ? .base = IO_PTR(SPMP8000_SCU_C_BASE),
+ ? ? ? ? ? ? .clken = IO_PTR(SPMP8000_SCU_C_BASE) + SCU_C_PERI_CLKEN,

arch/arm/mach-spmp8000/core.c line 71

Quoted 6 lines

arch/arm/mach-spmp8000/core.c new file +6 -0

+ ? ? },
+};
+
+/* DMA controller names to be used for the filter
+ * function of the DMA-Engine API. */
+char *spmp8000_dma_controller_names[] = {

const char * const?

Quoted 21 lines

arch/arm/mach-spmp8000/core.c new file +21 -0

+ ? ? "93010000.dma", ? ? ? ? /* APBDMA_A */
+ ? ? "92b00000.dma", ? ? ? ? /* APBDMA_C */
+};
+
+void __init spmp8000_map_io(void)
+{
+ ? ? iotable_init(io_desc, ARRAY_SIZE(io_desc));
+}
+
+void __init spmp8000_init_irq(void)
+{
+ ? ? struct device_node *np;
+ ? ? int ret;
+
+ ? ? np = of_find_compatible_node(NULL, NULL, "arm,pl192");
+ ? ? if (!np)
+ ? ? ? ? ? ? panic("Can't find VIC0 interrupt controller\n");
+
+ ? ? ret = vic_of_init(np, NULL);
+ ? ? if (ret)
+ ? ? ? ? ? ? panic("Can't init VIC0 interrupt controller\n");

arch/arm/mach-spmp8000/core.c line 102

Quoted 10 lines

arch/arm/mach-spmp8000/core.c new file +10 -0

+
+ ? ? np = of_find_compatible_node(np, NULL, "arm,pl192");
+ ? ? if (!np)
+ ? ? ? ? ? ? panic("Can't find VIC1 interrupt controller\n");
+
+ ? ? ret = vic_of_init(np, NULL);
+ ? ? if (ret)
+ ? ? ? ? ? ? panic("Can't init VIC1 interrupt controller\n");
+
+ ? ? of_node_put(np);

So with Rob Herring's of_irq_init patches you should be able to reduce this to:

? ? ? ?static const struct of_device_id spmp8000_vic_matches[] = { ? ? ? ? ? ? ? ?{ .compatible = "arm,pl192", .data = vic_of_init }, ? ? ? ?};

? ? ? ?of_irq_init(spmp8000_vic_matches);

and that will handle both VIC's and ordering.

Quoted 17 lines

arch/arm/mach-spmp8000/core.c new file +1 -0

+}

arch/arm/mach-spmp8000/include/mach/core.h new file +10 -0

new file mode 100644
index 0000000..fa1d522
@@ -0,0 +1,29 @@
+/*
+ * SPMP8000 generic includes
+ *
+ * Copyright (C) 2011 Zoltan Devai <[email protected]>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+

arch/arm/mach-spmp8000/include/mach/core.h line 16

Quoted 6 lines

arch/arm/mach-spmp8000/include/mach/core.h new file +6 -0

+#ifndef __MACH_SPMP8000_GENERIC_H__
+#define __MACH_SPMP8000_GENERIC_H__
+
+/* core.c */
+extern void __init spmp8000_map_io(void);
+extern void __init spmp8000_init_irq(void);

Nit: you don't need the __init attributes in the header files.

Quoted 13 lines

arch/arm/mach-spmp8000/include/mach/core.h new file +13 -0

+extern struct sys_timer spmp8000_sys_timer;
+
+/* clkdev.c */
+extern void __init spmp8000_init_clkdev(void);
+extern void spmp8000_update_arm_freqs(void); /* For cpufreq driver */
+
+/* pinmux.c */
+extern void spmp8000_pinmux_pgc_set(int pc, unsigned int conf);
+extern unsigned int spmp8000_pinmux_pgc_get(int pc);
+extern void spmp8000_pinmux_pgs_set(unsigned int pg, unsigned int func);
+extern int spmp8000_pinmux_pgs_get(int pg);
+
+#endif /* __MACH_SPMP8000_GENERIC_H__ */

[...]

Quoted 12 lines

arch/arm/mach-spmp8000/include/mach/dma.h new file +6 -0

new file mode 100644
index 0000000..44bc9f2
@@ -0,0 +1,45 @@
+/*
+ * SPMP8000 dma.h
+ *
+ * Copyright (C) 2011 Zoltan Devai <[email protected]>
+ *
+ * This file is licensed under the terms of the GNU General Public

arch/arm/mach-spmp8000/include/mach/dma.h line 11

Quoted 5 lines

arch/arm/mach-spmp8000/include/mach/dma.h new file +5 -0

+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+#include <linux/module.h>
+#include <linux/dmaengine.h>

It may be worth splitting the DMA part out into a separate patch unless this really is required for basic functionality.

Quoted 13 lines

arch/arm/mach-spmp8000/include/mach/dma.h new file +13 -0

+
+#ifndef __MACH_SPMP8000_DMA_H__
+#define __MACH_SPMP8000_DMA_H__
+
+enum spmp8000_dma_controller {
+ ? ? SPMP8000_APBDMA_A ? ? ? = 0,
+ ? ? SPMP8000_APBDMA_C,
+};
+
+extern char *spmp8000_dma_controller_names[];
+
+struct spmp8000_dma_params {
+ ? ? char ? ? ? ? ? ? ? ? ? ? ? ? ? ?*dma_controller;

arch/arm/mach-spmp8000/include/mach/dma.h line 28

Quoted 4 lines

arch/arm/mach-spmp8000/include/mach/dma.h new file +4 -0

+ ? ? dma_addr_t ? ? ? ? ? ? ? ? ? ? ?dma_address;
+ ? ? enum dma_slave_buswidth ? ? ? ? dma_width;
+ ? ? int ? ? ? ? ? ? ? ? ? ? ? ? ? ? maxburst;
+};

dmaengine has dma_slave_config that could be used for this, but I don't see any of this file being used in this series so perhaps it's worth adding in after the core stuff has been merged?

Quoted 4 lines

arch/arm/mach-spmp8000/include/mach/dma.h new file +4 -0

+
+/* Driver parameters */
+#define SPMP8000_APBDMA_MAX_PERIODS ?64
+

arch/arm/mach-spmp8000/include/mach/dma.h

Quoted 10 lines

arch/arm/mach-spmp8000/include/mach/dma.h new file +10 -0

+static bool spmp8000_dma_filter(struct dma_chan *chan, const char *name)
+{
+ ? ? int ret;
+
+ ? ? ret = strcmp(dev_name(chan->device->dev), name);
+
+ ? ? if (ret)
+ ? ? ? ? ? ? return false;
+
+ ? ? return true;

I think you can reduce this function to just

? ? ? ?return !strcmp(dev_name(chan->device->dev), name);

and the compiler will do the right thing with int->bool conversion.

Quoted 22 lines

arch/arm/mach-spmp8000/include/mach/gpio.h new file +15 -0

new file mode 100644
index 0000000..3eafac2
@@ -0,0 +1,21 @@
+/*
+ * SPMP8000 machines gpio support
+ *
+ * Copyright (C) 2011 Zoltan Devai <[email protected]>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#ifndef __MACH_SPMP8000_GPIO_H__
+#define __MACH_SPMP8000_GPIO_H__
+
+#include <asm-generic/gpio.h>
+

arch/arm/mach-spmp8000/include/mach/gpio.h line 19

Quoted 4 lines

arch/arm/mach-spmp8000/include/mach/gpio.h new file +4 -0

+#define gpio_get_value ? ? ? ? ? ? ? __gpio_get_value
+#define gpio_set_value ? ? ? ? ? ? ? __gpio_set_value
+#define gpio_cansleep ? ? ? ? ? ? ? ?__gpio_cansleep
+#define gpio_to_irq ? ? ? ? ?__gpio_to_irq

Russell has a patch (ARM: gpio: make trivial GPIOLIB implementation the default) in for-next that means you shouldn't need these definitions anymore.

Quoted 22 lines

arch/arm/mach-spmp8000/include/mach/irqs.h new file +15 -0

new file mode 100644
index 0000000..7f305d3
@@ -0,0 +1,21 @@
+/*
+ * SPMP8000 irqs.h
+ *
+ * Copyright (C) 2011 Zoltan Devai <[email protected]>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#ifndef __MACH_SPMP8000_IRQS_H__
+#define __MACH_SPMP8000_IRQS_H__
+
+#define SPMP8000_HW_IRQS ? ? ? ? ? ? 64
+#define SPMP8000_GPIO_IRQS_START ? ? SPMP8000_HW_IRQS

arch/arm/mach-spmp8000/include/mach/irqs.h line 19

Quoted 4 lines

arch/arm/mach-spmp8000/include/mach/irqs.h new file +4 -0

+#define SPMP8000_GPIO_IRQS ? ? ? ? ? (16 + 32)
+#define SPMP8000_TOTAL_IRQS ? ? ? ? ?(SPMP8000_HW_IRQS + SPMP8000_GPIO_IRQS)
+
+#define NR_IRQS ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?SPMP8000_TOTAL_IRQS

I think that if you use my VIC patches then you can actually define NR_IRQS to 0 as all IRQ descs are dynamically allocated (otherwise you'll be reserving a bunch of irq_descs that never get used).

Quoted 15 lines

arch/arm/mach-spmp8000/include/mach/regs-timer.h new file +8 -0

new file mode 100644
index 0000000..90735df
@@ -0,0 +1,32 @@
+/*
+ * SPMP8000 timer support
+ *
+ * Copyright (C) 2011 Zoltan Devai <[email protected]>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.

arch/arm/mach-spmp8000/include/mach/regs-timer.h line 11

Quoted 3 lines

arch/arm/mach-spmp8000/include/mach/regs-timer.h new file +3 -0

+ *
+ * These timer reg definitions are used by the timer and pwm drivers
+ */

If these definitions are only being used inside arch/arm/mach-spmp8000 then I'd put the headers in there rather than the arch/arm/mach-spmp8000/include/mach subdir so that the visibility is restricted to the mach code.

Quoted 20 lines

arch/arm/mach-spmp8000/include/mach/regs-timer.h new file +20 -0

+#ifndef __MACH_SPMP8000_REGS_TIMER_H__
+#define __MACH_SPMP8000_REGS_TIMER_H__
+
+#define SPMP8000_TMRB(tnum) ?(tnum * 0x20)
+#define SPMP8000_TMRB_CTR ? ?0x00
+#define SPMP8000_TMRB_CTR_TE BIT(0)
+#define SPMP8000_TMRB_CTR_IE BIT(1)
+#define SPMP8000_TMRB_CTR_OE BIT(2)
+#define SPMP8000_TMRB_CTR_PWMON ? ? ?BIT(3)
+#define SPMP8000_TMRB_CTR_UD BIT(4)
+#define SPMP8000_TMRB_CTR_UDS ? ? ? ?BIT(5)
+#define SPMP8000_TMRB_CTR_OM BIT(6)
+#define SPMP8000_TMRB_CTR_ES_SH ? ? ?8
+#define SPMP8000_TMRB_CTR_M_SH ? ? ? 10
+#define SPMP8000_TMRB_PSR ? ?0x04
+#define SPMP8000_TMRB_LDR ? ?0x08
+#define SPMP8000_TMRB_VLR ? ?0x08
+#define SPMP8000_TMRB_ISR ? ?0x0C
+#define SPMP8000_TMRB_CMP ? ?0x10
+

arch/arm/mach-spmp8000/include/mach/scu.h

Quoted 6 lines

arch/arm/mach-spmp8000/include/mach/regs-timer.h new file +1 -0

+#endif /* __MACH_SPMP8000_REGS_TIMER_H__ */

arch/arm/mach-spmp8000/include/mach/scu.h new file +0 -0

new file mode 100644
index 0000000..e3a98d4

Same here as the timer regs stuff.

Quoted 17 lines

arch/arm/mach-spmp8000/include/mach/scu.h new file +16 -0

@@ -0,0 +1,146 @@
+/*
+ * SPMP8000 System Control Unit register definitions
+ * SCUs are a random collection of reset, clock, gpio, pinmux, etc. controllers
+ * so that these definitions are needed at several places.
+ *
+ * Copyright (C) 2011 Zoltan Devai <[email protected]>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/bitops.h>
+#include <mach/hardware.h>
+
+#ifndef __MACH_SPMP8000_SCU_H__

arch/arm/mach-spmp8000/include/mach/scu.h line 22

Quoted 6 lines

arch/arm/mach-spmp8000/include/mach/scu.h new file +6 -0

+#define __MACH_SPMP8000_SCU_H__
+
+/* Used by to the clock code to directly access the SCU base and clock
+ * enabling registers, without looking up the according registers first.
+ */
+struct scu_reg_t {

Generally speaking _t isn't used for structs.

arch/arm/mach-spmp8000/include/mach/scu.h line 24

Quoted 2 lines

arch/arm/mach-spmp8000/include/mach/scu.h new file +2 -0

+ ? ? void *base;
+ ? ? void *clken;

Shouldn't these be void __iomem *?

Quoted 8 lines

arch/arm/mach-spmp8000/include/mach/scu.h new file +8 -0

+};
+
+extern struct scu_reg_t scu_regs[];
+
+#define REG_SCU_BASE(x) ? ? ? ? ? ? ?scu_regs[x].base
+#define REG_SCU_CLKEN(x) ? ? scu_regs[x].clken
+#define REG_SCU_A_ID ? ? ? ? 0
+#define REG_SCU_B_ID ? ? ? ? 1

arch/arm/mach-spmp8000/include/mach/scu.h line 36

Quoted 4 lines

arch/arm/mach-spmp8000/include/mach/scu.h new file +4 -0

+#define REG_SCU_C_ID ? ? ? ? 2
+#define REG_SCU_A(x) ? ? ? ? (scu_regs[REG_SCU_A_ID].base + x)
+#define REG_SCU_B(x) ? ? ? ? (scu_regs[REG_SCU_B_ID].base + x)
+#define REG_SCU_C(x) ? ? ? ? (scu_regs[REG_SCU_C_ID].base + x)

macros that assume there is a variable in scope with a given name (scu_regs) are generally frowned upon as it's difficult to see what's going on and can be a little error prone.

Quoted 40 lines

arch/arm/mach-spmp8000/include/mach/system.h new file +33 -0

new file mode 100644
index 0000000..be53ff3
@@ -0,0 +1,45 @@
+/*
+ * SPMP8000 system.h
+ *
+ * Copyright (C) 2011 Zoltan Devai <[email protected]>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#ifndef __MACH_SPMP8000_SYSTEM_H__
+#define __MACH_SPMP8000_SYSTEM_H__
+
+#include <linux/kernel.h>
+#include <linux/io.h>
+
+#define SPMP8000_WDT_BASE ? ?0x90001000
+#define SPMP8000_WDT_SIZE ? ?0x1000
+
+#define SPMP8000_WDT_CTR ? ? 0x00
+#define SPMP8000_WDT_CTR_TE ?BIT(0)
+#define SPMP8000_WDT_CTR_RE ?BIT(3)
+
+static inline void arch_idle(void)
+{
+ ? ? cpu_do_idle();
+}
+
+static inline void arch_reset(char mode, const char *cmd)
+{
+ ? ? void *base;
+
+ ? ? base = ioremap(SPMP8000_WDT_BASE, SPMP8000_WDT_SIZE);

arch/arm/mach-spmp8000/include/mach/system.h line 38

Quoted 5 lines

arch/arm/mach-spmp8000/include/mach/system.h new file +5 -0

+ ? ? if (!base) {
+ ? ? ? ? ? ? pr_err("spmp8000: Can't ioremap watchdog regs for reset. "
+ ? ? ? ? ? ? ? ? ? ? "Halt.");
+ ? ? ? ? ? ? while (1);
+ ? ? }

It may be worth doing the ioremap earlier when the system is in a known good state with all functions available rather than at reset time.

Quoted 5 lines

arch/arm/mach-spmp8000/include/mach/system.h new file +5 -0

+
+ ? ? /* Trigger a watchdog reset */
+ ? ? writel(SPMP8000_WDT_CTR_RE | SPMP8000_WDT_CTR_TE,
+ ? ? ? ? ? ? ? ? ? ? base + SPMP8000_WDT_CTR);
+}

[...]

Quoted 46 lines

arch/arm/mach-spmp8000/timer.c new file +39 -0

new file mode 100644
index 0000000..7d5d0eb
@@ -0,0 +1,160 @@
+/*
+ * SPMP8000 machines timer functions
+ *
+ * Copyright (C) 2011 Zoltan Devai <[email protected]>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/kernel.h>
+#include <linux/irq.h>
+#include <linux/io.h>
+#include <linux/err.h>
+#include <linux/bitops.h>
+#include <linux/interrupt.h>
+#include <linux/clockchips.h>
+#include <linux/clk.h>
+#include <asm/mach/time.h>
+
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+
+#include <mach/hardware.h>
+#include <mach/regs-timer.h>
+#include <mach/irqs.h>
+
+static unsigned long clkrate;
+static const unsigned int tickrate = 1012500;
+
+/* The TIMER_B block is used as system timer
+ * T2: Clocksource
+ * T1: Clockevent
+ * T0: PWM for LCD backlight
+ * T3,4: Could be used as additional clockevent devices
+ * Timer constraints:
+ * - WDT: Can't clear the irq directly, only by resetting the whole counter
+ * ? in the ISR, which means that IRQs will jitter

arch/arm/mach-spmp8000/timer.c line 44

Quoted 5 lines

arch/arm/mach-spmp8000/timer.c new file +5 -0

+ * - Timer_B: Can't reset the timer value, so at start, the first IRQ
+ * ? will happen at some random time.
+ */
+#define ? ? ? ? ? ? ?CS_TIMER ? ? ? ?2
+#define ? ? ? ? ? ? ?CE_TIMER ? ? ? ?1

Nit: removing the extra spaces between the define and the CS_TIMER... might make it easier to grep for.

Quoted 12 lines

arch/arm/mach-spmp8000/timer.c new file +12 -0

+static void __iomem *cs_base;
+static void __iomem *ce_base;
+
+static void tmrb_set_mode(enum clock_event_mode mode,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct clock_event_device *dev)
+{
+ ? ? switch (mode) {
+ ? ? case CLOCK_EVT_MODE_PERIODIC:
+ ? ? ? ? ? ? writel((tickrate / HZ), ce_base + SPMP8000_TMRB_LDR);
+ ? ? ? ? ? ? /* Configure as periodic, down counter, IEN, enable timer */
+ ? ? ? ? ? ? writel(SPMP8000_TMRB_CTR_TE | SPMP8000_TMRB_CTR_IE |
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? (1 << SPMP8000_TMRB_CTR_M_SH),

arch/arm/mach-spmp8000/timer.c line 61

Quoted 5 lines

arch/arm/mach-spmp8000/timer.c new file +5 -0

+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ce_base + SPMP8000_TMRB_CTR);
+ ? ? ? ? ? ? break;
+ ? ? case CLOCK_EVT_MODE_ONESHOT:
+ ? ? ? ? ? ? BUG();
+ ? ? ? ? ? ? break;

This case can be folded into the CLOCK_EVT_MODE_RESUME case (and changed to be a default case).

Quoted 55 lines

arch/arm/mach-spmp8000/timer.c new file +55 -0

+ ? ? case CLOCK_EVT_MODE_SHUTDOWN:
+ ? ? case CLOCK_EVT_MODE_UNUSED:
+ ? ? ? ? ? ? /* Disable timer */
+ ? ? ? ? ? ? writel(0, ce_base + SPMP8000_TMRB_CTR);
+ ? ? ? ? ? ? break;
+ ? ? case CLOCK_EVT_MODE_RESUME:
+ ? ? ? ? ? ? BUG();
+ ? ? ? ? ? ? break;
+ ? ? }
+}
+
+static struct clock_event_device tmrb1_clkevt = {
+ ? ? .name ? ? ? ? ? = "tmrb1",
+ ? ? .features ? ? ? = CLOCK_EVT_FEAT_PERIODIC,
+ ? ? .rating ? ? ? ? = 200,
+ ? ? .set_mode ? ? ? = tmrb_set_mode,
+};
+
+static irqreturn_t tmrb1_isr(int irq, void *dev_id)
+{
+ ? ? tmrb1_clkevt.event_handler(&tmrb1_clkevt);
+
+ ? ? /* Clear IRQ */
+ ? ? writel(0, ce_base + SPMP8000_TMRB_ISR);
+
+ ? ? return IRQ_HANDLED;
+};
+
+static struct irqaction tmrb1_irq = {
+ ? ? .name ? ? ? ? ? = "tmrb1_irq",
+ ? ? .flags ? ? ? ? ?= IRQF_TIMER | IRQF_IRQPOLL,
+ ? ? .handler ? ? ? ?= tmrb1_isr,
+};
+
+static void __init spmp8000_sys_timer_init(void)
+{
+ ? ? struct device_node *np;
+ ? ? unsigned int irq;
+ ? ? struct clk *clk;
+ ? ? void *tmrb_base;
+
+ ? ? np = of_find_compatible_node(NULL, NULL, "sunplus,spmp8000-timer");
+ ? ? if (!np)
+ ? ? ? ? ? ? panic("spmp8000: unable to find timer node in dtb\n");
+
+ ? ? tmrb_base = of_iomap(np, 0);
+ ? ? if (!tmrb_base)
+ ? ? ? ? ? ? panic("spmp8000: unable to map timer cpu registers\n");
+
+ ? ? irq = of_irq_to_resource(np, CE_TIMER, NULL);
+ ? ? if (irq == NO_IRQ)
+ ? ? ? ? ? ? panic("spmp8000: unable to get interrupts of timer\n");
+
+ ? ? of_node_put(np);
+

arch/arm/mach-spmp8000/timer.c line 121

Quoted 5 lines

arch/arm/mach-spmp8000/timer.c new file +5 -0

+ ? ? cs_base = tmrb_base + SPMP8000_TMRB(CS_TIMER);
+ ? ? ce_base = tmrb_base + SPMP8000_TMRB(CE_TIMER);
+
+ ? ? clk = clk_get(NULL, "arm_apb");
+ ? ? if (IS_ERR_OR_NULL(clk))

NULL can actually be a valid clk cookie so this should just be IS_ERR(clk).

arch/arm/mach-spmp8000/timer.c line 124

Quoted 3 lines

arch/arm/mach-spmp8000/timer.c new file +3 -0

+ ? ? ? ? ? ? panic("spmp8000: Can't get clock for timer device");
+
+ ? ? clk_enable(clk);

Should probably check for success/failure here.

Quoted 36 lines

arch/arm/mach-spmp8000/timer.c new file +36 -0

+ ? ? clkrate = clk_get_rate(clk);
+
+ ? ? /* Clocksource */
+ ? ? /* Disable timer */
+ ? ? writel(0, cs_base + SPMP8000_TMRB_CTR);
+
+ ? ? /* Reset counter value
+ ? ? ?* Not really possible unless setting end-1 LDR value and waiting
+ ? ? ?* until the counter reaches that */
+
+ ? ? /* Prescale timer */
+ ? ? writel((clkrate / tickrate) - 1, cs_base + SPMP8000_TMRB_PSR);
+
+ ? ? /* Register the clocksource */
+ ? ? clocksource_mmio_init(cs_base + SPMP8000_TMRB_VLR, "tmrb2",
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? tickrate, 200, 16, clocksource_mmio_readl_up);
+
+ ? ? /* Configure as free running (0 - 0xFFFF), up counter, enable timer */
+ ? ? writel(SPMP8000_TMRB_CTR_TE | SPMP8000_TMRB_CTR_UD,
+ ? ? ? ? ? ? cs_base + SPMP8000_TMRB_CTR);
+
+ ? ? /* Clockevent */
+ ? ? setup_irq(irq, &tmrb1_irq);
+
+ ? ? /* Disable timer */
+ ? ? writel(0, ce_base + SPMP8000_TMRB_CTR);
+
+ ? ? /* Prescale timer */
+ ? ? writel((clkrate / tickrate) - 1, ce_base + SPMP8000_TMRB_PSR);
+
+ ? ? clockevents_register_device(&tmrb1_clkevt);
+}
+
+struct sys_timer spmp8000_sys_timer = {
+ ? ? .init ? ? ? ? ? = spmp8000_sys_timer_init,
+};

2011/10/9 Jamie Iles <[email protected]>:

Quoted 1 line

A couple of minor comments inline but this looks really good!

Thanks for reviewing! Comments inline.

Quoted 1 line · Zoltan Devai

Quoted 7 lines

Quoted 7 lines

+#ifdef CONFIG_DEBUG_LL
+ ? ? {
+ ? ? ? ? ? ? .virtual ? ? ? ?= IO_ADDRESS(SPMP8000_UARTC0_BASE),
+ ? ? ? ? ? ? .pfn ? ? ? ? ? ?= __phys_to_pfn(SPMP8000_UARTC0_BASE),
+ ? ? ? ? ? ? .length ? ? ? ? = SPMP8000_UARTC0_SIZE,
+ ? ? ? ? ? ? .type ? ? ? ? ? = MT_DEVICE,
+ ? ? },

Quoted 8 lines

arch/arm/mach-spmp8000/core.c

Quoted 4 lines

arch/arm/mach-spmp8000/core.c new file +4 -0

+#endif
+};
+
+#define IO_PTR(x) ? ?((void __iomem *)IO_ADDRESS(x))

Generally IO_ADDRESS returns a void __iomem pointer so it would be easier if you could have the cast in there (though it may need to be enclosed in #ifndef __ASSEMBLY__ conditionals).

Every instance I've seen returns an int, mainly because it's used for the static mappings above.
So, if I make IO_ADDRESS return void __iomem, I'd have to make a macro for casting it back to int, and we'd be where we started.

Quoted 13 lines

Quoted 13 lines

arch/arm/mach-spmp8000/include/mach/dma.h new file +13 -0

+
+#ifndef __MACH_SPMP8000_DMA_H__
+#define __MACH_SPMP8000_DMA_H__
+
+enum spmp8000_dma_controller {
+ ? ? SPMP8000_APBDMA_A ? ? ? = 0,
+ ? ? SPMP8000_APBDMA_C,
+};
+
+extern char *spmp8000_dma_controller_names[];
+
+struct spmp8000_dma_params {
+ ? ? char ? ? ? ? ? ? ? ? ? ? ? ? ? ?*dma_controller;

Quoted 8 lines

Quoted 4 lines

+ ? ? dma_addr_t ? ? ? ? ? ? ? ? ? ? ?dma_address;
+ ? ? enum dma_slave_buswidth ? ? ? ? dma_width;
+ ? ? int ? ? ? ? ? ? ? ? ? ? ? ? ? ? maxburst; +};

dmaengine has dma_slave_config that could be used for this, but I don't see any of this file being used in this series so perhaps it's worth adding in after the core stuff has been merged?

Will split.
This struct is used by the i2s driver to pass info to the pcm driver on how to set up the dma controller. Seems to be the way to do.

Which tree and branch should I base my work on ? I'm quite confused by the all the options, and it seems like if I choose some devel-stable tree, then I don't get the new features and my work is outdated from the beginning, but for-next branches are quite diverged.
Is there a good strategy ?

Quoted 40 lines

Quoted 40 lines

arch/arm/mach-spmp8000/include/mach/system.h new file +33 -0

new file mode 100644
index 0000000..be53ff3
@@ -0,0 +1,45 @@
+/*
+ * SPMP8000 system.h
+ *
+ * Copyright (C) 2011 Zoltan Devai <[email protected]>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#ifndef __MACH_SPMP8000_SYSTEM_H__
+#define __MACH_SPMP8000_SYSTEM_H__
+
+#include <linux/kernel.h>
+#include <linux/io.h>
+
+#define SPMP8000_WDT_BASE ? ?0x90001000
+#define SPMP8000_WDT_SIZE ? ?0x1000
+
+#define SPMP8000_WDT_CTR ? ? 0x00
+#define SPMP8000_WDT_CTR_TE ?BIT(0)
+#define SPMP8000_WDT_CTR_RE ?BIT(3)
+
+static inline void arch_idle(void)
+{
+ ? ? cpu_do_idle();
+}
+
+static inline void arch_reset(char mode, const char *cmd)
+{
+ ? ? void *base;
+
+ ? ? base = ioremap(SPMP8000_WDT_BASE, SPMP8000_WDT_SIZE);

Quoted 8 lines

arch/arm/mach-spmp8000/include/mach/system.h line 38

Quoted 5 lines

arch/arm/mach-spmp8000/include/mach/system.h new file +5 -0

+ ? ? if (!base) {
+ ? ? ? ? ? ? pr_err("spmp8000: Can't ioremap watchdog regs for reset. "
+ ? ? ? ? ? ? ? ? ? ? "Halt.");
+ ? ? ? ? ? ? while (1);
+ ? ? }

It may be worth doing the ioremap earlier when the system is in a known good state with all functions available rather than at reset time.

Any suggestion where the best place would be ?
I can only think of either the timer init or the board init, but neither seemed to be appropriate.

arch/arm/mach-spmp8000/core.c new file +12 -0

+
+/* Static mappings:
+ * SCU: timer needs clk support which is inited in init_early
+ * UART: needed for earlyprintk
+ */
+static struct map_desc io_desc[] __initdata = {
+	{
+		.virtual	= IO_ADDRESS(SPMP8000_SCU_A_BASE),
+		.pfn		= __phys_to_pfn(SPMP8000_SCU_A_BASE),
+		.length		= SPMP8000_SCU_A_SIZE,
+		.type		= MT_DEVICE,
+	}, {

arch/arm/mach-spmp8000/include/mach/dma.h new file +23 -0

+#ifndef __MACH_SPMP8000_DMA_H__
+#define __MACH_SPMP8000_DMA_H__
+
+enum spmp8000_dma_controller {
+	SPMP8000_APBDMA_A	= 0,
+	SPMP8000_APBDMA_C,
+};
+
+extern char *spmp8000_dma_controller_names[];
+
+struct spmp8000_dma_params {
+	char				*dma_controller;
+	dma_addr_t			dma_address;
+	enum dma_slave_buswidth		dma_width;
+	int				maxburst;
+};
+
+/* Driver parameters */
+#define SPMP8000_APBDMA_MAX_PERIODS	64
+
+static bool spmp8000_dma_filter(struct dma_chan *chan, const char *name)
+{
+	int ret;

arch/arm/mach-spmp8000/include/mach/gpio.h new file +1 -0

new file mode 100644
 
+#ifndef __MACH_SPMP8000_GPIO_H__

+++ b/arch/arm/mach-spmp8000/include/mach/regs-timer.h

arch/arm/mach-spmp8000/include/mach/gpio.h new file +22 -0

@@ -0,0 +1,32 @@
+/*
+ * SPMP8000 timer support
+ *
+ * Copyright (C) 2011 Zoltan Devai <[email protected]>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ *
+ * These timer reg definitions are used by the timer and pwm drivers
+ */
+#ifndef __MACH_SPMP8000_REGS_TIMER_H__
+#define __MACH_SPMP8000_REGS_TIMER_H__
+
+#define SPMP8000_TMRB(tnum)	(tnum * 0x20)
+#define SPMP8000_TMRB_CTR	0x00
+#define SPMP8000_TMRB_CTR_TE	BIT(0)
+#define SPMP8000_TMRB_CTR_IE	BIT(1)
+#define SPMP8000_TMRB_CTR_OE	BIT(2)
+#define SPMP8000_TMRB_CTR_PWMON	BIT(3)
+#define SPMP8000_TMRB_CTR_UD	BIT(4)
+#define SPMP8000_TMRB_CTR_UDS	BIT(5)

arch/arm/mach-spmp8000/include/mach/scu.h new file +9 -0

new file mode 100644
index 0000000..e3a98d4
@@ -0,0 +1,146 @@
+/*
+ * SPMP8000 System Control Unit register definitions
+ * SCUs are a random collection of reset, clock, gpio, pinmux, etc. controllers
+ * so that these definitions are needed at several places.
 
+struct scu_reg_t {
+	void *base;
+	void *clken;
+};
+

arch/arm/mach-spmp8000/include/mach/system.h new file +8 -0

new file mode 100644
index 0000000..be53ff3
@@ -0,0 +1,45 @@
+/*
+ * SPMP8000 system.h
+ *
+ * Copyright (C) 2011 Zoltan Devai <[email protected]>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.

arch/arm/mach-spmp8000/include/mach/system.h new file +14 -0

+#define SPMP8000_WDT_CTR	0x00
+#define SPMP8000_WDT_CTR_TE	BIT(0)
+#define SPMP8000_WDT_CTR_RE	BIT(3)
+
+static inline void arch_idle(void)
+{
+	cpu_do_idle();
+}
+
+static inline void arch_reset(char mode, const char *cmd)
+{
+	void *base;
+
+	base = ioremap(SPMP8000_WDT_BASE, SPMP8000_WDT_SIZE);

Quoted 20 lines

(quoted diff) +20 -0

+
+/* Static mappings:
+ * SCU: timer needs clk support which is inited in init_early
+ * UART: needed for earlyprintk
+ */
+static struct map_desc io_desc[] __initdata = {
+ ? ? {
+ ? ? ? ? ? ? .virtual ? ? ? ?= IO_ADDRESS(SPMP8000_SCU_A_BASE),
+ ? ? ? ? ? ? .pfn ? ? ? ? ? ?= __phys_to_pfn(SPMP8000_SCU_A_BASE),
+ ? ? ? ? ? ? .length ? ? ? ? = SPMP8000_SCU_A_SIZE,
+ ? ? ? ? ? ? .type ? ? ? ? ? = MT_DEVICE,
+ ? ? }, {
+ ? ? ? ? ? ? .virtual ? ? ? ?= IO_ADDRESS(SPMP8000_SCU_B_BASE),
+ ? ? ? ? ? ? .pfn ? ? ? ? ? ?= __phys_to_pfn(SPMP8000_SCU_B_BASE),
+ ? ? ? ? ? ? .length ? ? ? ? = SPMP8000_SCU_B_SIZE,
+ ? ? ? ? ? ? .type ? ? ? ? ? = MT_DEVICE,
+ ? ? }, {
+ ? ? ? ? ? ? .virtual ? ? ? ?= IO_ADDRESS(SPMP8000_SCU_C_BASE),
+ ? ? ? ? ? ? .pfn ? ? ? ? ? ?= __phys_to_pfn(SPMP8000_SCU_C_BASE),
+ ? ? ? ? ? ? .length ? ? ? ? = SPMP8000_SCU_C_SIZE,

Quoted 2 lines

+ ? ? ? ? ? ? .type ? ? ? ? ? = MT_DEVICE,
+ ? ? },

With Nicolas Pitre's rework of the MMIO space handling, I think it would be nice to just use a single area that spans all of the devices so you can do an optimized ioremap and huge TLBs. Unfortunately that series won't make it into 3.2, but if you just set up a large area now, it will automatically work.

Quoted 4 lines

+#ifdef CONFIG_DEBUG_LL
+ ? ? {
+ ? ? ? ? ? ? .virtual ? ? ? ?= IO_ADDRESS(SPMP8000_UARTC0_BASE),
+ ? ? ? ? ? ? .pfn ? ? ? ? ? ?= __phys_to_pfn(SPMP8000_UARTC0_BASE),

Review

Quoted 6 lines

+ ? ? ? ? ? ? .length ? ? ? ? = SPMP8000_UARTC0_SIZE,
+ ? ? ? ? ? ? .type ? ? ? ? ? = MT_DEVICE,
+ ? ? },

(quoted diff) +2 -0

+#endif
+};

And it would be nice to move this into a separate file that handles the early debug code, as prima2 does.

Quoted 7 lines

arch/arm/mach-spmp8000/include/mach/dma.h new file +7 -0

+#ifndef __MACH_SPMP8000_DMA_H__
+#define __MACH_SPMP8000_DMA_H__
+
+enum spmp8000_dma_controller {
+ ? ? SPMP8000_APBDMA_A ? ? ? = 0,
+ ? ? SPMP8000_APBDMA_C,
+};

Please separate the dma controller out for now into a separate patch. We really need to have proper device tree bindings so we can avoid the silly filter functions and do something better. That needs some more discussion and any help on the interface is very much appreciated.

Quoted 4 lines

arch/arm/mach-spmp8000/include/mach/gpio.h new file +1 -0

new file mode 100644
 
+#ifndef __MACH_SPMP8000_GPIO_H__

arch/arm/mach-spmp8000/include/mach/gpio.h line 21

Quoted 10 lines

arch/arm/mach-spmp8000/include/mach/gpio.h new file +10 -0

+#define __MACH_SPMP8000_GPIO_H__
+
+#include <asm-generic/gpio.h>
+
+#define gpio_get_value ? ? ? ? ? ? ? __gpio_get_value
+#define gpio_set_value ? ? ? ? ? ? ? __gpio_set_value
+#define gpio_cansleep ? ? ? ? ? ? ? ?__gpio_cansleep
+#define gpio_to_irq ? ? ? ? ?__gpio_to_irq
+
+#endif /* __MACH_SPMP8000_GPIO_H__ */

I think this can become an empty file now, with Russell's latest cleanup going into 3.2.

arch/arm/mach-spmp8000/include/mach/gpio.h

Quoted 2 lines

arch/arm/mach-spmp8000/include/mach/gpio.h new file +2 -0

+/* Vitual to physical translation of statically mapped space */
+#define IO_ADDRESS(x) ? ? ? ? ? ? ? ?((x) | 0xF0000000)

Better make this return a 'void __iomem *', e.g. by doing

#define IO_ADDRESS(x) ? (((x) & 0x0fffffff) + ((void __iomem *)0xf0000000)

Quoted 4 lines

arch/arm/mach-spmp8000/include/mach/gpio.h new file +4 -0

+/* Needed for the static mappings and uncompress.h */
+#define SPMP8000_UARTC0_BASE 0x92B04000
+#define SPMP8000_UARTC0_SIZE SZ_4K
+#define SPMP8000_SCU_C_BASE ?0x92005000

arch/arm/mach-spmp8000/include/mach/gpio.h

Quoted 5 lines

arch/arm/mach-spmp8000/include/mach/gpio.h new file +5 -0

+#define SPMP8000_SCU_C_SIZE ?SZ_4K
+#define SPMP8000_SCU_A_BASE ?0x93007000
+#define SPMP8000_SCU_A_SIZE ?SZ_4K
+#define SPMP8000_SCU_B_BASE ?0x90005000
+#define SPMP8000_SCU_B_SIZE ?SZ_4K

Do you need these all so early that you cannot get the location from the device tree?

arch/arm/mach-spmp8000/include/mach/gpio.h

Quoted 6 lines

arch/arm/mach-spmp8000/include/mach/gpio.h new file +6 -0

+/* FIXME This should go away */
+#define IO_SPACE_LIMIT ?0
+
+#define __mem_pci(a) ? ?(a)
+#define __io(a) ? ? ? ? __typesafe_io(a)
+

I think you should better define __io() to NULL.

Quoted 28 lines

+++ b/arch/arm/mach-spmp8000/include/mach/regs-timer.h

arch/arm/mach-spmp8000/include/mach/gpio.h new file +26 -0

@@ -0,0 +1,32 @@
+/*
+ * SPMP8000 timer support
+ *
+ * Copyright (C) 2011 Zoltan Devai <[email protected]>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ *
+ * These timer reg definitions are used by the timer and pwm drivers
+ */
+#ifndef __MACH_SPMP8000_REGS_TIMER_H__
+#define __MACH_SPMP8000_REGS_TIMER_H__
+
+#define SPMP8000_TMRB(tnum) ?(tnum * 0x20)
+#define SPMP8000_TMRB_CTR ? ?0x00
+#define SPMP8000_TMRB_CTR_TE BIT(0)
+#define SPMP8000_TMRB_CTR_IE BIT(1)
+#define SPMP8000_TMRB_CTR_OE BIT(2)
+#define SPMP8000_TMRB_CTR_PWMON ? ? ?BIT(3)
+#define SPMP8000_TMRB_CTR_UD BIT(4)
+#define SPMP8000_TMRB_CTR_UDS ? ? ? ?BIT(5)
+#define SPMP8000_TMRB_CTR_OM BIT(6)
+#define SPMP8000_TMRB_CTR_ES_SH ? ? ?8
+#define SPMP8000_TMRB_CTR_M_SH ? ? ? 10
+#define SPMP8000_TMRB_PSR ? ?0x04

arch/arm/mach-spmp8000/include/mach/gpio.h line 32

Quoted 6 lines

arch/arm/mach-spmp8000/include/mach/gpio.h new file +6 -0

+#define SPMP8000_TMRB_LDR ? ?0x08
+#define SPMP8000_TMRB_VLR ? ?0x08
+#define SPMP8000_TMRB_ISR ? ?0x0C
+#define SPMP8000_TMRB_CMP ? ?0x10
+
+#endif /* __MACH_SPMP8000_REGS_TIMER_H__ */

No need for this header, just move the definitions into the timer driver.

Quoted 22 lines

arch/arm/mach-spmp8000/include/mach/scu.h new file +15 -0

new file mode 100644
index 0000000..e3a98d4
@@ -0,0 +1,146 @@
+/*
+ * SPMP8000 System Control Unit register definitions
+ * SCUs are a random collection of reset, clock, gpio, pinmux, etc. controllers
+ * so that these definitions are needed at several places.
 
+struct scu_reg_t {
+ ? ? void *base;
+ ? ? void *clken;
+};
+
+extern struct scu_reg_t scu_regs[];
+
+#define REG_SCU_BASE(x) ? ? ? ? ? ? ?scu_regs[x].base
+#define REG_SCU_CLKEN(x) ? ? scu_regs[x].clken
+#define REG_SCU_A_ID ? ? ? ? 0
+#define REG_SCU_B_ID ? ? ? ? 1

arch/arm/mach-spmp8000/include/mach/scu.h

Quoted 4 lines

arch/arm/mach-spmp8000/include/mach/scu.h new file +4 -0

+#define REG_SCU_C_ID ? ? ? ? 2
+#define REG_SCU_A(x) ? ? ? ? (scu_regs[REG_SCU_A_ID].base + x)
+#define REG_SCU_B(x) ? ? ? ? (scu_regs[REG_SCU_B_ID].base + x)
+#define REG_SCU_C(x) ? ? ? ? (scu_regs[REG_SCU_C_ID].base + x)

I would prefer having ?this in core.c, so the base addresses can remain private to that file instead of exporting them to all of the kernel.

arch/arm/mach-spmp8000/include/mach/scu.h

Quoted 9 lines

arch/arm/mach-spmp8000/include/mach/scu.h new file +9 -0

+#define SCU_A_PERI_RST ? ? ? ? ? ? ? 0x00
+#define SCU_A_PERI_CLKEN ? ? 0x04
+#define SCU_A_PERI_DGCLKEN ? 0x0C
+
+#define SCU_A_APLL_CFG ? ? ? ? ? ? ? 0x44
+#define APLL_CFG_P ? ? ? ? ? BIT(0)
+#define APLL_CFG_S ? ? ? ? ? BIT(1)
+#define APLL_CFG_F ? ? ? ? ? BIT(2)
+#define APLL_CFG_E ? ? ? ? ? BIT(3)

The BIT() definition is really just mean for kernel-internal bitops, not so much for device registers.

My recommended style of this is

#define APLL_CFG_P ? ? ? ? ? ? ?0x00000001
#define APLL_CFG_S ? ? ? ? ? ? ?0x00000002
#define APLL_CFG_F ? ? ? ? ? ? ?0x00000004
#define APLL_CFG_E ? ? ? ? ? ? ?0x00000008

Quoted 14 lines

arch/arm/mach-spmp8000/include/mach/system.h new file +8 -0

new file mode 100644
index 0000000..be53ff3
@@ -0,0 +1,45 @@
+/*
+ * SPMP8000 system.h
+ *
+ * Copyright (C) 2011 Zoltan Devai <[email protected]>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.

arch/arm/mach-spmp8000/include/mach/system.h line 18

Quoted 10 lines

arch/arm/mach-spmp8000/include/mach/system.h new file +10 -0

+ */
+
+#ifndef __MACH_SPMP8000_SYSTEM_H__
+#define __MACH_SPMP8000_SYSTEM_H__
+
+#include <linux/kernel.h>
+#include <linux/io.h>
+
+#define SPMP8000_WDT_BASE ? ?0x90001000
+#define SPMP8000_WDT_SIZE ? ?0x1000

These have to be removed. For device drivers, you should normally use of_iomap to get at the registers.

Quoted 14 lines

arch/arm/mach-spmp8000/include/mach/system.h new file +14 -0

+#define SPMP8000_WDT_CTR ? ? 0x00
+#define SPMP8000_WDT_CTR_TE ?BIT(0)
+#define SPMP8000_WDT_CTR_RE ?BIT(3)
+
+static inline void arch_idle(void)
+{
+ ? ? cpu_do_idle();
+}
+
+static inline void arch_reset(char mode, const char *cmd)
+{
+ ? ? void *base;
+
+ ? ? base = ioremap(SPMP8000_WDT_BASE, SPMP8000_WDT_SIZE);

arch/arm/mach-spmp8000/include/mach/system.h line 42

Quoted 10 lines

arch/arm/mach-spmp8000/include/mach/system.h new file +10 -0

+ ? ? if (!base) {
+ ? ? ? ? ? ? pr_err("spmp8000: Can't ioremap watchdog regs for reset. "
+ ? ? ? ? ? ? ? ? ? ? "Halt.");
+ ? ? ? ? ? ? while (1);
+ ? ? }
+
+ ? ? /* Trigger a watchdog reset */
+ ? ? writel(SPMP8000_WDT_CTR_RE | SPMP8000_WDT_CTR_TE,
+ ? ? ? ? ? ? ? ? ? ? base + SPMP8000_WDT_CTR);
+}

Using the watchdog for arch_reset is clever, but unfortunately not too common otherwise, so you have to play tricks. I would try locating the device and ioremapping it from core.c and then export a function to set the register that is used both by the arch_reset function and by the watchdog driver. The latter can still bind to the platform_device, but it then doesn't have to do an extra of_iomap.

2011/10/11 Arnd Bergmann <[email protected]>:

Quoted 2 lines

I think this can become an empty file now, with Russell's latest cleanup going into 3.2.

I didn't find that in either your or Russell's tree, where should I look for it ?

Quoted 6 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+/* Vitual to physical translation of statically mapped space */
+#define IO_ADDRESS(x) ? ? ? ? ? ? ? ?((x) | 0xF0000000)

Better make this return a 'void __iomem *', e.g. by doing

#define IO_ADDRESS(x) ? (((x) & 0x0fffffff) + ((void __iomem *)0xf0000000)

This is used by map_desc, so it needs to return an int.
I use IO_PTR for the pointer version, but maybe the naming is misleading.

Quoted 8 lines

Review

Quoted 6 lines

(quoted diff) +6 -0

+/* FIXME This should go away */
+#define IO_SPACE_LIMIT ?0
+
+#define __mem_pci(a) ? ?(a)
+#define __io(a) ? ? ? ? __typesafe_io(a)
+

I think you should better define __io() to NULL.

OK

2011/10/11 Arnd Bergmann <[email protected]>:

Quoted 21 lines · Zoltan Devai

Quoted 20 lines

arch/arm/mach-spmp8000/core.c new file +20 -0

+
+/* Static mappings:
+ * SCU: timer needs clk support which is inited in init_early
+ * UART: needed for earlyprintk
+ */
+static struct map_desc io_desc[] __initdata = {
+     {
+             .virtual        = IO_ADDRESS(SPMP8000_SCU_A_BASE),
+             .pfn            = __phys_to_pfn(SPMP8000_SCU_A_BASE),
+             .length         = SPMP8000_SCU_A_SIZE,
+             .type           = MT_DEVICE,
+     }, {
+             .virtual        = IO_ADDRESS(SPMP8000_SCU_B_BASE),
+             .pfn            = __phys_to_pfn(SPMP8000_SCU_B_BASE),
+             .length         = SPMP8000_SCU_B_SIZE,
+             .type           = MT_DEVICE,
+     }, {
+             .virtual        = IO_ADDRESS(SPMP8000_SCU_C_BASE),
+             .pfn            = __phys_to_pfn(SPMP8000_SCU_C_BASE),
+             .length         = SPMP8000_SCU_C_SIZE,

Quoted 8 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+             .type           = MT_DEVICE,
+     },

With Nicolas Pitre's rework of the MMIO space handling, I think it would be nice to just use a single area that spans all of the devices so you can do an optimized ioremap and huge TLBs. Unfortunately that series won't make it into 3.2, but if you just set up a large area now, it will automatically work.

What do you mean by 'all of the devices' ?
These three, or all MMIO peripherals ?
These are at addresses 0x90005000, 0x92005000 and 0x93007000. So either a 50 or 256 MB area, but both seem like a waste of virtual memory space.

Quoted 4 lines

Quoted 4 lines

arch/arm/mach-spmp8000/core.c new file +4 -0

+#ifdef CONFIG_DEBUG_LL
+     {
+             .virtual        = IO_ADDRESS(SPMP8000_UARTC0_BASE),
+             .pfn            = __phys_to_pfn(SPMP8000_UARTC0_BASE),

Quoted 8 lines

arch/arm/mach-spmp8000/core.c line 49

Quoted 6 lines

arch/arm/mach-spmp8000/core.c new file +5 -0

+             .length         = SPMP8000_UARTC0_SIZE,
+             .type           = MT_DEVICE,
+     },
+#endif
+};

And it would be nice to move this into a separate file that handles the early debug code, as prima2 does.

Prima2 doesn't have other static mappings. In my case, that would mean a separate map_desc table in lluart.c and an #ifdef CONFIG_DEBUG_LL in the map_io call.
Is it worth it ?

BTW, the static mappings are only needed to be able to set up the clk stuff in init_early. That in turn is only needed for the timer code to determinate its input clock.
If I would hard-code the timer clock rate in its driver (the bootloader is very unlikely to change), then I could init the clk driver from machine_init and get rid of these mappings.
How about that ?

BTW2,
shouldn't the timer driver also go to drivers/clocksource ?

Quoted 15 lines

Quoted 15 lines

arch/arm/mach-spmp8000/include/mach/regs-timer.h new file +15 -0

+#ifndef __MACH_SPMP8000_REGS_TIMER_H__
+#define __MACH_SPMP8000_REGS_TIMER_H__
+
+#define SPMP8000_TMRB(tnum)  (tnum * 0x20)
+#define SPMP8000_TMRB_CTR    0x00
+#define SPMP8000_TMRB_CTR_TE BIT(0)
+#define SPMP8000_TMRB_CTR_IE BIT(1)
+#define SPMP8000_TMRB_CTR_OE BIT(2)
+#define SPMP8000_TMRB_CTR_PWMON      BIT(3)
+#define SPMP8000_TMRB_CTR_UD BIT(4)
+#define SPMP8000_TMRB_CTR_UDS        BIT(5)
+#define SPMP8000_TMRB_CTR_OM BIT(6)
+#define SPMP8000_TMRB_CTR_ES_SH      8
+#define SPMP8000_TMRB_CTR_M_SH       10
+#define SPMP8000_TMRB_PSR    0x04

Quoted 8 lines

arch/arm/mach-spmp8000/include/mach/regs-timer.h line 32

Quoted 6 lines

arch/arm/mach-spmp8000/include/mach/regs-timer.h new file +6 -0

+#define SPMP8000_TMRB_LDR    0x08
+#define SPMP8000_TMRB_VLR    0x08
+#define SPMP8000_TMRB_ISR    0x0C
+#define SPMP8000_TMRB_CMP    0x10
+
+#endif /* __MACH_SPMP8000_REGS_TIMER_H__ */

No need for this header, just move the definitions into the timer driver.

It's also used by the pwm driver (pwm is just a timer with a HW output pin) as the header indicates. Not a lots of lines, so I can include it in both if you think that's more appropriate.

Quoted 9 lines

The BIT() definition is really just mean for kernel-internal bitops, not so much for device registers.

My recommended style of this is

#define APLL_CFG_P              0x00000001
#define APLL_CFG_S              0x00000002
#define APLL_CFG_F              0x00000004
#define APLL_CFG_E              0x00000008

Are you sure about this ?
I recall reading a recommendation to use BIT() as its less error-prone, easier matched with the datasheet and properly typed. Also:
kernel$ grep -R " BIT(" drivers/ | wc -l
2186

Quoted 30 lines · Zoltan Devai

2011/10/11 Arnd Bergmann <[email protected]>:

Quoted 21 lines · Zoltan Devai

Quoted 20 lines

arch/arm/mach-spmp8000/core.c new file +20 -0

+
+/* Static mappings:
+ * SCU: timer needs clk support which is inited in init_early
+ * UART: needed for earlyprintk
+ */
+static struct map_desc io_desc[] __initdata = {
+     {
+             .virtual        = IO_ADDRESS(SPMP8000_SCU_A_BASE),
+             .pfn            = __phys_to_pfn(SPMP8000_SCU_A_BASE),
+             .length         = SPMP8000_SCU_A_SIZE,
+             .type           = MT_DEVICE,
+     }, {
+             .virtual        = IO_ADDRESS(SPMP8000_SCU_B_BASE),
+             .pfn            = __phys_to_pfn(SPMP8000_SCU_B_BASE),
+             .length         = SPMP8000_SCU_B_SIZE,
+             .type           = MT_DEVICE,
+     }, {
+             .virtual        = IO_ADDRESS(SPMP8000_SCU_C_BASE),
+             .pfn            = __phys_to_pfn(SPMP8000_SCU_C_BASE),
+             .length         = SPMP8000_SCU_C_SIZE,

Quoted 8 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+             .type           = MT_DEVICE,
+     },

With Nicolas Pitre's rework of the MMIO space handling, I think it would be nice to just use a single area that spans all of the devices so you can do an optimized ioremap and huge TLBs. Unfortunately that series won't make it into 3.2, but if you just set up a large area now, it will automatically work.

Quoted 5 lines

What do you mean by 'all of the devices' ?
These three, or all MMIO peripherals ?
These are at addresses 0x90005000, 0x92005000 and 0x93007000. So either a 50 or 256 MB area, but both seem like a waste of virtual memory space.

I was thinking of mapping everything that your IO_ADDRESS macro covers, which should be something like 240 MB starting at 0x90000000/0xf0000000. On most ARM9 platforms, virtual memory is not really contraint at all because there is not all that much physical memory. Also, you currently reserve the entire 0xf0000000 segment for MMIO ranges, so I would expect that you can keep doing that after the cleanup.

Quoted 12 lines

Quoted 4 lines

Quoted 4 lines

arch/arm/mach-spmp8000/core.c new file +4 -0

+#ifdef CONFIG_DEBUG_LL
+     {
+             .virtual        = IO_ADDRESS(SPMP8000_UARTC0_BASE),
+             .pfn            = __phys_to_pfn(SPMP8000_UARTC0_BASE),

Quoted 8 lines

arch/arm/mach-spmp8000/core.c line 49

Quoted 6 lines

arch/arm/mach-spmp8000/core.c new file +5 -0

+             .length         = SPMP8000_UARTC0_SIZE,
+             .type           = MT_DEVICE,
+     },
+#endif
+};

And it would be nice to move this into a separate file that handles the early debug code, as prima2 does.

Quoted 4 lines

Prima2 doesn't have other static mappings. In my case, that would mean a separate map_desc table in lluart.c and an #ifdef CONFIG_DEBUG_LL in the map_io call.
Is it worth it ?

Yes, it's worth it. Don't do the #ifdef in the map_io call though, do it in the header file that declares the function prototype.

Quoted 7 lines

BTW, the static mappings are only needed to be able to set up the clk stuff in init_early. That in turn is only needed for the timer code to determinate its input clock.
If I would hard-code the timer clock rate in its driver (the bootloader is very unlikely to change), then I could init the clk driver from machine_init and get rid of these mappings.
How about that ?

Maybe you can just pass the clock rate in the device tree then. If the boot loader changes it, it can still pass the correct rate and you don't need the early mappings for functional reasons any more.

Quoted 9 lines · Arnd Bergmann

Quoted 7 lines · Zoltan Devai

BTW, the static mappings are only needed to be able to set up the clk stuff in init_early. That in turn is only needed for the timer code to determinate its input clock.
If I would hard-code the timer clock rate in its driver (the bootloader is very unlikely to change), then I could init the clk driver from machine_init and get rid of these mappings.
How about that ?

Quoted 3 lines

Maybe you can just pass the clock rate in the device tree then. If the boot loader changes it, it can still pass the correct rate and you don't need the early mappings for functional reasons any more.

please don't update the DT from the bootloader, I'll recommand you to limit your bootloader to just update the strict minimun (same as ATAG) and touch noting else