[U-Boot] [PATCH 3/3] Kirkwood: Add support for Ka-Ro TK71

[Prafulla Wadaskar]

> -----Original Message-----
> From: Marek Vasut [mailto:marex at denx.de]
> Sent: 26 June 2012 18:15
> To: Prafulla Wadaskar
> Cc: u-boot at lists.denx.de; Wolfgang Denk
> Subject: Re: [PATCH 3/3] Kirkwood: Add support for Ka-Ro TK71
> 
> Dear Prafulla Wadaskar,
> 
> > > -----Original Message-----
> > > From: Marek Vasut [mailto:marex at denx.de]
> > > Sent: 26 June 2012 17:45
> > > To: u-boot at lists.denx.de
> > > Cc: Marek Vasut; Prafulla Wadaskar; Wolfgang Denk
> > > Subject: [PATCH 3/3] Kirkwood: Add support for Ka-Ro TK71
> > >
> > > Signed-off-by: Marek Vasut <marex at denx.de>
> > > Cc: Prafulla Wadaskar <prafulla at marvell.com>
> > > Cc: Wolfgang Denk <wd at denx.de>
> > > ---
> > >
> > >  board/karo/tk71/Makefile     |   45 +++++++++++
> > >  board/karo/tk71/kwbimage.cfg |  174
...snip...
> > > +int board_init(void)
> > > +{
> > > +	/*
> > > +	 * arch number of board
> > > +	 */
> > > +	gd->bd->bi_arch_number = CONFIG_MACH_TYPE;
> > > +
> > > +	/* adress of boot parameters */
> > > +	gd->bd->bi_boot_params = kw_sdram_bar(0) + 0x100;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +void kw_adjust_dram(void)
> > > +{
> > > +	unsigned long size = get_ram_size(PHYS_SDRAM_1,
> > > PHYS_SDRAM_1_SIZE);
> > > +
> > > +	/* 256MB module, adjust BAR register */
> > > +	if (size == 256 * 1024 * 1024)
> > > +		writel(0x0ffffff1, KW_REG_CPUCS_WIN_SZ(0));
> >
> > No hard coding please, please use register structures (preferred)
> 
> Do we have any such structures defined ?

There are few structures in place but not for DRAM, let's do it, this was missing part in early patches.

> 
> > > +
> > > +	gd->bd->bi_dram[0].size = size;
> > > +	gd->ram_size = size;
> > > +}
> >
> > You have created kwbimage.cfg in this, it makes more sense to set
> 256MB
> > size by default in kwbimage.cfg. Or preferred use any existing
> > kwbimage.cfg (preferred)
> 
> I don't think I follow. I always believe these are for setting up the
> platform,
> so at least one per platform is necessary.

Ideally YES, but if we have similar platforms and delta is very small like tuning DRAM size, then for such things we can reuse existing one by supporting additional post configuration using board_early_init_f()

This is something positive to avoid code duplication in a cleaner way.

> 
> > The main idea of having such weak functions is to avoid code
> duplication
> > (kwbimage.cfg) for small manageable changes.
> 
> But then, isn't this a bit out of scope of this patchset? Unifying
> kwbimages I

Yes, so please keep you board support patch separate (standalone)