[PATCH] ARM: CSR: Adding CSR SiRFprimaII board support
Russell King - ARM Linux
linux at arm.linux.org.uk
Tue Jun 21 12:22:28 EDT 2011
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.
> diff --git a/arch/arm/Makefile b/arch/arm/Makefile
> index f5b2b39..79e6edf 100644
> --- a/arch/arm/Makefile
> +++ b/arch/arm/Makefile
> @@ -191,6 +191,7 @@ machine-$(CONFIG_ARCH_VEXPRESS) := vexpress
> machine-$(CONFIG_ARCH_VT8500) := vt8500
> machine-$(CONFIG_ARCH_W90X900) := w90x900
> machine-$(CONFIG_ARCH_NUC93X) := nuc93x
> +machine-$(CONFIG_ARCH_PRIMA2) := prima2
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.
> diff --git a/arch/arm/boot/dts/prima2-cb.dts b/arch/arm/boot/dts/prima2-cb.dts
> new file mode 100644
> index 0000000..6e8b17c
> --- /dev/null
> +++ b/arch/arm/boot/dts/prima2-cb.dts
> @@ -0,0 +1,44 @@
> +/dts-v1/;
> +/ {
> + model = "SIRF Prima2 EVB";
> + compatible = "sirf,prima2-cb", "sirf,prima2";
> + #address-cells = <1>;
> + #size-cells = <1>;
> + interrupt-parent = <&intc>;
> +
> + memory {
> + reg = <0x00000000 0x20000000>;
> + };
> +
> + chosen {
> + bootargs = "mem=512M real_root=/dev/mmcblk0p2 console=ttyS1 earlyprintk";
> + linux,stdout-path = &uart1;
> + };
> +
> + amba {
You declare that you have an AMBA bus, does it have primcells on it?
Should you be selecting ARM_AMBA in your kconfig?
> diff --git a/arch/arm/mach-prima2/include/mach/entry-macro.S b/arch/arm/mach-prima2/include/mach/entry-macro.S
> new file mode 100644
> index 0000000..af5611b
> --- /dev/null
> +++ b/arch/arm/mach-prima2/include/mach/entry-macro.S
> @@ -0,0 +1,28 @@
> +/*
> + * arch/arm/mach-prima2/include/mach/entry-macro.S
> + *
> + * Copyright (c) 2011 Cambridge Silicon Radio Limited, a CSR plc group company.
> + *
> + * Licensed under GPLv2 or later.
> + */
> +
> +#include <mach/hardware.h>
> +
> +#define SIRFSOC_INT_ID 0x38
> +
> + .macro get_irqnr_and_base, irqnr, irqstat, base, tmp
> + ldr \base, =SIRFSOC_INTR_VA_BASE
Consider using get_irqnr_preamble to load the base address only once per
IRQ exception.
> diff --git a/arch/arm/mach-prima2/include/mach/isa-dma.h b/arch/arm/mach-prima2/include/mach/isa-dma.h
> new file mode 100644
> index 0000000..f07e264
> --- /dev/null
> +++ b/arch/arm/mach-prima2/include/mach/isa-dma.h
> @@ -0,0 +1,14 @@
> +/*
> + * arch/arm/mach-prima2/include/mach/io.h
> + *
> + * Copyright (c) 2011 Cambridge Silicon Radio Limited, a CSR plc group company.
> + *
> + * Licensed under GPLv2 or later.
> + */
> +
> +#ifndef __MACH_PRIMA2_ISADMA_H
> +#define __MACH_PRIMA2_ISADMA_H
> +
> +#define MAX_DMA_CHANNELS 32
> +
> +#endif
You don't need this if you don't define ISA_DMA_API.
> diff --git a/arch/arm/mach-prima2/include/mach/uncompress.h b/arch/arm/mach-prima2/include/mach/uncompress.h
> new file mode 100644
> index 0000000..e08d7b8
> --- /dev/null
> +++ b/arch/arm/mach-prima2/include/mach/uncompress.h
> @@ -0,0 +1,42 @@
> +/*
> + * arch/arm/mach-prima2/include/mach/uncompress.h
> + *
> + * Copyright (c) 2011 Cambridge Silicon Radio Limited, a CSR plc group company.
> + *
> + * Licensed under GPLv2 or later.
> + */
> +
> +#ifndef __ASM_ARCH_UNCOMPRESS_H
> +#define __ASM_ARCH_UNCOMPRESS_H
> +
> +#include <linux/bitops.h>
> +#include <linux/io.h>
> +#include <asm/processor.h>
> +#include <mach/hardware.h>
> +#include <mach/uart.h>
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.
> diff --git a/arch/arm/mach-prima2/irq.c b/arch/arm/mach-prima2/irq.c
> new file mode 100644
> index 0000000..af65481
> --- /dev/null
> +++ b/arch/arm/mach-prima2/irq.c
> @@ -0,0 +1,81 @@
...
> +#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?
> diff --git a/arch/arm/mach-prima2/timer.c b/arch/arm/mach-prima2/timer.c
> new file mode 100644
> index 0000000..f73928f
> --- /dev/null
> +++ b/arch/arm/mach-prima2/timer.c
> @@ -0,0 +1,181 @@
...
> +/* 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.
More information about the linux-arm-kernel
mailing list