[U-Boot-Users] [PATCH 3/7 v6] ARM: Add arm1176 core with S3C6400 SoC

[Guennadi Liakhovetski]

On Sat, 9 Aug 2008, Jean-Christophe PLAGNIOL-VILLARD wrote:

> > +
> > +void dcache_disable (void)
> > +{
> > +	ulong reg;
> > +
> > +	reg = read_p15_c1 ();
> > +	cp_delay ();
> > +	reg &= ~C1_DC;
> > +	write_p15_c1 (reg);
> why not  as the other implementation?

ok

> > +		/*printf("Calculated %lu timer_load_val\n", timer_load_val);*/
> please remove if not need

ok

> please add some empty line to be more readable
> > +	/* load value for 10 ms timeout */
> > +	lastdec = timers->TCNTB4 = timer_load_val;
> > +	/* auto load, manual update of Timer 4 */
> > +	timers->TCON = (timers->TCON & ~0x00700000) | TCON_4_AUTO |
> > +		TCON_4_UPDATE;
> > +	/* auto load, start Timer 4 */
> > +	timers->TCON = (timers->TCON & ~0x00700000) | TCON_4_AUTO | COUNT_4_ON;
> > +	timestamp = 0;

ok

> > +static ulong get_PLLCLK(int pllreg)
> please not uppercase

These are s3c* common functions declared in include/common.h, chenging 
their declarations is outside of the scope of this patchset (see below)

> > +	if (pllreg == APLL)
> > +		r = APLL_CON_REG;
> > +	else if (pllreg == MPLL)
> > +		r = MPLL_CON_REG;
> > +	else if (pllreg == EPLL)
> > +		r = EPLL_CON0_REG;
> > +	else
> > +		hang();
> please move to switch implementation

ok

> > +	printf("\nCPU:     S3C6400@%luMHz\n", get_ARMCLK() / 1000000);
> > +	printf("         Fclk = %luMHz, Hclk = %luMHz, Pclk = %luMHz ",
> > +	       get_FCLK() / 1000000, get_HCLK() / 1000000,
> > +	       get_PCLK() / 1000000);
> maybe a macro like HZ_TO_MHZ(x) could be helpfull to avoid typo

Don't think such metric conversions deserve a macro.

> please add space between parameter
> > +	mrs	r0,cpsr
> > +	bic	r0,r0,#0x3f
> > +	orr	r0,r0,#0xd3
> > +	msr	cpsr,r0

ok

> > diff --git a/include/common.h b/include/common.h
> > index 06ed278..ba87322 100644
> > --- a/include/common.h
> > +++ b/include/common.h
> > @@ -491,7 +491,8 @@ int	prt_mpc8220_clks (void);
> >  ulong	get_OPB_freq (void);
> >  ulong	get_PCI_freq (void);
> >  #endif
> > -#if defined(CONFIG_S3C2400) || defined(CONFIG_S3C2410) || defined(CONFIG_LH7A40X)
> > +#if defined(CONFIG_S3C2400) || defined(CONFIG_S3C2410) || \
> > +	defined(CONFIG_LH7A40X) || defined(CONFIG_S3C6400)
> Is it possible to have a better and simpler ifdef?

It is possible and desirable to remove these declarations

> >  void	s3c2410_irq(void);
> >  #define ARM920_IRQ_CALLBACK s3c2410_irq
> >  ulong	get_FCLK (void);

from this header completely. Don't understand what they are doing in 
include/common.h. However, this is outside of the scope of this patchset. 
Please, if you will be fixing this, do this after this patchset.

> > +#define NFADDR           	(ELFIN_NAND_BASE+NFADDR_OFFSET)
>                  ^^^^^^^^^^^
> please remove whitesapce

ok

> btw on all macro please add space beetwen operator like
> > +#define NFCONF_REG		__REG(ELFIN_NAND_BASE+NFCONF_OFFSET)
> to
> #define NFCONF_REG		__REG(ELFIN_NAND_BASE + NFCONF_OFFSET)

ok

> > +#define Startup_AMDIV		400
> for macro I'll prefer upercase
> > +#define Startup_MDIV		400
> > +#define Startup_PDIV		6
> > +#define Startup_SDIV		1

ok

> > +typedef vu_char		S3C64XX_REG8;
> > +typedef vu_short	S3C64XX_REG16;
> > +typedef vu_long		S3C64XX_REG32;
> I'll prefer you use the type directly

ok

> > +} /*__attribute__((__packed__))*/ s3c64xx_uart;
> why do you remove the packed attribute?

put it back. Makes no _practical_ difference in this case.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de