Dear Wolfgand Denk
Thanks for your review comments
-----Original Message----- From: Wolfgang Denk [mailto:wd@denx.de] Sent: Wednesday, May 20, 2009 3:29 AM To: Prafulla Wadaskar Cc: u-boot@lists.denx.de; Ashish Karkare; Prabhanjan Sarnaik; Ronen Shitrit Subject: Re: [U-Boot] [PATCH v8] Marvell Kirkwood family SOC support
Dear Prafulla Wadaskar,
In message 1242763678-13724-1-git-send-email-prafulla@marvell.com you wrote:
Kirkwood family controllers are highly integrated SOCs based on Feroceon-88FR131/Sheeva-88SV131 cpu core.
...
+/*
- Window Size
- Used with the Base register to set the address window
size and location.
- Must be programmed from LSB to MSB as sequence of 1’s
followed
+by
- sequence of 0’s. The number of 1’s specifies the
size of the
+window in
- 64 KByte granularity (e.g., a value of 0x00FF specifies
256 = 16 MByte).
- NOTE: A value of 0x0 specifies 64-KByte size.
- */
You have a number of strange special characters here. Please try and restrict yourself to plain ASCII text in normal C comments.
I checked the patch that I send across and associated source code too. I didn’t find the above special chars in it I am using git-send-email to send the patches and vim as my editor I wonder how these special characters appeared in the patch !!!! I will check this issue with my system admin
- for (i = 0; i < (sizeval / 0x10000); i++) {
j |= (1 << i);- }
No curly braces for single line statements, please.
Sorry missed this one, corrected..
- struct kwwin_registers *winregs = (struct kwwin_registers
+*)KW_CPU_WIN_BASE;
Line too long.
+/*
- kw_config_gpio - GPIO configuration */ void kw_config_gpio(u32
+gpp0_oe_val, u32 gpp1_oe_val, u32 gpp0_oe, u32 gpp1_oe) {
- struct kwgpio_registers *gpio0reg = (struct
kwgpio_registers *)KW_GPIO0_BASE;
- struct kwgpio_registers *gpio1reg = (struct kwgpio_registers
+*)KW_GPIO1_BASE;
More too long lines. Pleasse check everywhere.
Generally I execute Lindent, I missed it this time, I will do it
- writel(gpp0_oe, (u32)&gpio0reg->oe);
- writel(gpp1_oe, (u32)&gpio1reg->oe);
Why are you using these casts here? The whole purpose of using a C struct to access device registers is to enable type checking by the C compiler, but you sabotage this with these casts. Please don't do that. This comment applies to the whole patch.
This was done to remove build warnings in some context I will remove them
...
- cntmrctrl = readl(CNTMR_CTRL_REG);
- cntmrctrl |= CTCR_ARM_TIMER_EN(UBOOT_CNTR); /*
enable cnt\timer */
Are you sure you want to have a TAB character in this comment? What's "cnt imer" ? :-)
Not really :-) it was for counter/timer, slash was a confusion. Removed....
diff --git a/drivers/serial/serial.c
b/drivers/serial/serial.c index
966df9a..dd5f332 100644 --- a/drivers/serial/serial.c +++ b/drivers/serial/serial.c @@ -27,6 +27,9 @@ #ifdef CONFIG_NS87308 #include <ns87308.h> #endif +#ifdef CONFIG_KIRKWOOD +#include <asm/arch/kirkwood.h> +#endif
What exactly is this needed for?
CONFIG_SYS_NS16550_CLK is defined as CONFIG_SYS_TCLK which defined in the soc specific header file
In my earlier versions I had included arch specific header file in board_config header file But in the review comments it has asked to remove Hence above include is done
- writel(0x00000002, KW_REG_SPI_CTRL);
- /* program spi clock prescaller using max_hz */
- data = ((CONFIG_SYS_TCLK / 2) / max_hz) & 0x0000000f;
- debug("data = 0x%08x \n", data);
- writel(0x00000210 | data, KW_REG_SPI_CONFIG);
- writel(0x00000001, KW_REG_SPI_IRQ_CAUSE);
- writel(0x00000000, KW_REG_SPI_IRQ_MASK);
What does these magic constants mean?
- /* program mpp registers to select SPI_CSn */
- if (cs)
writel((readl((u32)&mppreg[0]) & 0x0fffffff) |0x20000000, (u32)&mppreg[0]);- else
writel((readl((u32)&mppreg[0]) & 0xfffffff0) |0x00000002, (u32)&mppreg[0]);Ot these?
I will provide definitions for magic numbers
+int spi_xfer(struct spi_slave *slave, unsigned int bitlen,
const void *dout,
void *din, unsigned long flags)...
for (tm = 0, isread = 0; tm < KW_SPI_TIMEOUT; ++tm) {if (readl(KW_REG_SPI_IRQ_CAUSE)) {isread = 1;tmpdin = readl(KW_REG_SPI_DATA_IN);debug("*** spi_xfer: din %08X... %08x read\n",
din, tmpdin);Indentation by TABs only, please.
Indentation is done by Lindent. Do you mean to do it manually?
+#define INTREG_BASE 0xd0000000 +#define KW_REGISTER(x) (KW_REGS_PHY_BASE + x) +#define KW_OFFSET_REG (INTREG_BASE + 0x20080)
+/* undocumented registers */ +#define KW_REG_UNDOC_0x1470 (KW_REGISTER(0x1470)) +#define KW_REG_UNDOC_0x1478 (KW_REGISTER(0x1478))
+#define KW_UART0_BASE (KW_REGISTER(0x12000)) +#define KW_UART1_BASE (KW_REGISTER(0x13000)) +#define KW_MPP_BASE (KW_REGISTER(0x10000)) +#define KW_GPIO0_BASE (KW_REGISTER(0x10100)) +#define KW_GPIO1_BASE (KW_REGISTER(0x10140)) +#define KW_CPU_WIN_BASE (KW_REGISTER(0x20000)) +#define KW_CPU_REG_BASE (KW_REGISTER(0x20100)) +#define KW_TIMER_BASE (KW_REGISTER(0x20300)) +#define KW_REG_PCIE_BASE (KW_REGISTER(0x40000)) +#define KW_EGIGA0_BASE (KW_REGISTER(0x72000)) +#define KW_EGIGA1_BASE (KW_REGISTER(0x76000))
Use a C struct?
These are the Base address referred by register structures. Generally this type of declaration used for other cpu/socs. May you point any reference for this?
Regards.. Prafulla . .
Best regards,
Wolfgang Denk
-- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de "One lawyer can steal more than a hundred men with guns."
- The Godfather