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

quoted

2011/6/22 Russell King - ARM Linux [off-list ref]:
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 at 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

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help