[PATCH] ARM: CSR: Adding CSR SiRFprimaII board support

Some comments inline, nothing really serious...

On Mon, Jun 20, 2011 at 12:53:22AM -0700, Barry Song wrote:

+config ARCH_PRIMA2
+	bool "CSR SiRFSoC PRIMA2 ARM Cortex A9 Platform"
+	select CPU_V7
+	select GENERIC_TIME
+	select GENERIC_CLOCKEVENTS
+	select CLKDEV_LOOKUP
+	select USE_OF
+	select ISA_DMA_API

Do you really provide the old ISA DMA API?  Unless you're intending to use
existing ISA drivers with their ISA DMA, you shouldn't define this.

The comment at the start of this list says:

# Machine directory name.  This list is sorted alphanumerically
# by CONFIG_* macro name.

and I thank the NUC93x people for also missing it.  Please ignore the
NUC93x entry and place yours appropriately.

You declare that you have an AMBA bus, does it have primcells on it?
Should you be selecting ARM_AMBA in your kconfig?

Consider using get_irqnr_preamble to load the base address only once per
IRQ exception.

You don't need this if you don't define ISA_DMA_API.

If you include all that, then you're in for problems with the decompressor.
The decompressor is a _really_ limited environment, and most stuff from
linux/ or platfform stuff will not be available.

...

+#define SIRFSOC_INT_PENDING0            0x0000
+#define SIRFSOC_INT_PENDING1            0x0004
+#define SIRFSOC_INT_IRQ_PENDING0        0x0008
+#define SIRFSOC_INT_IRQ_PENDING1        0x000C
+#define SIRFSOC_INT_FIQ_PENDING0        0x0010
+#define SIRFSOC_INT_FIQ_PENDING1        0x0014
+#define SIRFSOC_INT_RISC_MASK0          0x0018
+#define SIRFSOC_INT_RISC_MASK1          0x001C
+#define SIRFSOC_INT_RISC_LEVEL0         0x0020
+#define SIRFSOC_INT_RISC_LEVEL1         0x0024
+
+static void sirfsoc_irq_ack(struct irq_data *d)
+{
+}
+
+static void sirfsoc_irq_mask(struct irq_data *d)
+{
+	unsigned long mask;
+
+	mask = __raw_readl(SIRFSOC_INTR_VA_BASE + SIRFSOC_INT_RISC_MASK0 + (d->irq / 32) * 4) &
+		~(1 << ( d->irq % 32));
+	__raw_writel(mask, SIRFSOC_INTR_VA_BASE + SIRFSOC_INT_RISC_MASK0 + (d->irq / 32) * 4);
+}
+
+static void sirfsoc_irq_unmask(struct irq_data *d)
+{
+	unsigned long mask;
+
+	mask = __raw_readl(SIRFSOC_INTR_VA_BASE + SIRFSOC_INT_RISC_MASK0 + (d->irq / 32) * 4) |
+		(1 << ( d->irq % 32));
+	__raw_writel(mask, SIRFSOC_INTR_VA_BASE + SIRFSOC_INT_RISC_MASK0 + (d->irq / 32) * 4);
+}
+
+int sirfsoc_irq_settype(struct irq_data *d, unsigned int type)
+{
+	/*
+	 * Interrupt handler doesnt support setting trigger type
+	 */
+	if (type == IRQ_TYPE_NONE)
+		return 0;
+
+	return -EINVAL;
+}
+
+static struct irq_chip sirfsoc_irq_chip = {
+	.name = "SiRF SoC",
+	.irq_ack = sirfsoc_irq_ack,
+	.irq_mask = sirfsoc_irq_mask,
+	.irq_unmask = sirfsoc_irq_unmask,
+	.irq_set_type = sirfsoc_irq_settype,
+};

Can you use the recently introduced generic irqchips stuff for this?

...

+/* initialize the kernel jiffy timer source */
+static void __init sirfsoc_timer_init(void)
+{
+	unsigned long rate;
+	/* timer's input clock is io clock */
+	struct clk *clk = clk_get(NULL, "io");

Rather than going down this broken path of specifying clock names as
connection IDs, it would be better to obtain it by function:
	struct clk *clk = clk_get_sys("timer", NULL);

Experience shows that people think that naming the clock signals themselves
and passing strings around to drivers is easier.  After a few years they
find that what they thought was easy, is actually inflexible and has
become a huge problem which they need to rework.  (Samsung folk are
currently going through this pain.)

So please, don't fall into the trap of "lets give each clock signal a name
and look up only by clock name".  Use the device names as the primary
matching and don't allow yourself to get into the trap of passing clock
names around.

+
+	BUG_ON(IS_ERR_OR_NULL(clk));

	BUG_ON(IS_ERR(clk));

If clk_get() may return NULL, then clk_get_rate() should be able to eat
that value without choking.

+
+	rate = clk_get_rate(clk);
+	clk_put(clk);

It's much preferable not to clk_put() a clock which you're going to
continue using.

+
+	BUG_ON(rate < CLOCK_TICK_RATE);
+	BUG_ON(rate % CLOCK_TICK_RATE);
+
+	__raw_writel(rate / CLOCK_TICK_RATE / 2 - 1, SIRFSOC_TIMER_VA_BASE + SIRFSOC_TIMER_DIV);
+	__raw_writel(0, SIRFSOC_TIMER_VA_BASE + SIRFSOC_TIMER_COUNTER_LO);
+	__raw_writel(0, SIRFSOC_TIMER_VA_BASE + SIRFSOC_TIMER_COUNTER_HI);
+	__raw_writel(BIT(0), SIRFSOC_TIMER_VA_BASE + SIRFSOC_TIMER_STATUS);
+
+	if (clocksource_register_hz(&sirfsoc_clocksource, CLOCK_TICK_RATE))
+		BUG();

Confused.  CLOCK_TICK_RATE is defined to be 100 * HZ, so 10kHz.  Do
your timers really tick at 10kHz?  That seems needlessly slow for a
64-bit counter.  Also consider BUG_ON()

+
+	if (setup_irq(sirfsoc_timer_irq.irq, &sirfsoc_timer_irq))
+		BUG();

BUG_ON() here too.