Dear Wolfgand Denk Thanks for your review comments
> -----Original Message----- > From: Wolfgang Denk [mailto:w...@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-prafu...@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: > w...@denx.de "One lawyer can steal more than a hundred men with guns." > - The Godfather > _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
