[PATCH] ARM: CSR: Adding CSR SiRFprimaII board support
Barry Song
21cnbao at gmail.com
Wed Jun 22 03:36:42 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?
>
>> 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.
i don't need ISA_DMA at all. i was totally wrong for this. actually i
only need a DMA zone due to DMA address can't
be larger than 256MB for prima2 SD controller. So "#define
ARM_DMA_ZONE_SIZE SZ_256M" is enough for me.
>
>> 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
>
> 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.
<mach/uart.h>, io.h, mach/map.h with memory mapping are needed here.
others are redundant here.
>
>> 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?
agree, then i select GENERIC_IRQ_CHIP.
>
>> 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);
agree. it makes the codes clearer a lot for sys level clock which
doesn't bind with a device.
>
> 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.
agree. to avoid device name issues, the current clock.c only include
system timers like pll, cystal and bus level. i didn't place device level clocks
like uart/lcd/gps/gpu/mmc and so on yet.
then all of them should be changed to use dev_id but not con_id. then we get
them by clk_get_sys().
so the problem still need to be solved since device names coming from DT is not
that much "stable" as statically registered devices. the name from DT
isn't the same
as what board support code currently uses for statically registered
platform_devices.
Grant has two threads for this:
1. of: add clock providers to provide clk mapping in DT
http://www.mail-archive.com/devicetree-discuss@lists.ozlabs.org/msg03016.html
2. " If Linux needs specific devices to have specific names, then it
can pass in a lookup table that matches DT nodes to device names,
which is considerably simpler and it leaves the platform setup code in
control over how devices get instantiated."
both seems not ready yet?
>
>> +
>> + 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.
agree. clk_get() has no chance to return NULL.
>
>> +
>> + 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.
agree.
>
>> +
>> + 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()
sorry for typo. it's my misoperation by copy/paste the file from another file.
it actually is "#define CLOCK_TICK_RATE 1000000"
>
>> +
>> + if (setup_irq(sirfsoc_timer_irq.irq, &sirfsoc_timer_irq))
>> + BUG();
>
> BUG_ON() here too.
Agree.
>
> _______________________________________________
> 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