[PATCH] ARM: CSR: Adding CSR SiRFprimaII board support
Barry Song
21cnbao at gmail.com
Thu Jun 23 22:28:34 EDT 2011
2011/6/22 Russell King - ARM Linux <linux at arm.linux.org.uk>:
> 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?
Russell,
the chip has no primecells on it. And it seems we don't need ARM_AMBA.
PrimaII uses AXI protocol, but has no AMBA IP controller. and many IP
are self-defined. Nothing in the chip really has AMBA periph id.
its layout is like
AXI(0-0x3FFFFFFF)
memory
AXI(0x40000000-0xC0000000)
CPUIF(sirf-iobus)
INTC
MEMC
LCD
VPP
GRAPHIC
MULTIMEDIA
GPS
UART
...
hard-coded PCI bridge
SD/MMC
rtc-iobrg
RTC
>
>> 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.
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
More information about the linux-arm-kernel
mailing list