[U-Boot] [PATCH v8] Marvell Kirkwood family SOC support

Wed May 20 11:00:23 CEST 2009

Dear Wolfgand Denk

Thanks for your review comments 

> -----Original Message-----
> From: Wolfgang Denk [mailto:wd at denx.de] 
> Sent: Wednesday, May 20, 2009 3:29 AM
> To: Prafulla Wadaskar
> Cc: u-boot at 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 at 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 at denx.de "One lawyer can steal more than a hundred men with guns."
> - The Godfather
>