[U-Boot] [PATCH 1/2] KW: Move the memory register definitions into kirkwood.h

[Marek Vasut]

Dear Gerlando Falauto,

> Dear Marek Vasut,
> 
> On 06/29/2012 12:08 PM, Marek Vasut wrote:
> > Dear Gerlando Falauto,
> > 
> >> Dear Maruk,
> 
> sorry about my misspelling!
> 
> >> I am currently trying to address the exact same issue (SDRAM size
> >> detection and fixup).
> >> My idea was however (as opposed to moving register definition as you
> >> did), to add a fixup function to dram.c, say:
> >> 
> >> /*
> >> 
> >>    * kw_sdram_bs - writes SDRAM Bank size
> >>    */
> >> 
> >> void kw_sdram_bs_set(enum memory_bank bank, u32 size)
> >> {
> >> 
> >> 	/* Read current register value */
> >> 	u32 reg = readl(KW_REG_CPUCS_WIN_SZ(bank));
> >> 	
> >> 	printf("Current value: %x\n", reg);
> >> 	/* Clear window size */
> >> 	reg&= ~KW_REG_CPUCS_WIN_SIZE(0xFF);
> >> 	
> >> 	/* Set new window size */
> >> 	reg |= KW_REG_CPUCS_WIN_SIZE((size - 1)>>  24);
> >> 	
> >> 	printf("Writing: %x\n", reg);
> >> 	writel(reg, KW_REG_CPUCS_WIN_SZ(bank));
> >> 
> >> }
> >> 
> >> which would then be called to fix the window size according to the total
> >> memory size as reported by get_ram_size().
> > 
> > Read up at [1].
> > 
> > http://comments.gmane.org/gmane.comp.boot-loaders.u-boot/133991
> 
> Did read up, not sure I understand.
> If I get it right, *this* thread and patchset follows the feedback from
> the above thread.
> 
> What I am trying to say is: we have a function (available to all
> kirkwood boards) which reads the value of the window size register for a
> given memory bank and returns its size.
> Now we want to do the opposite, write the window size register according
> to the detected SDRAM size.
> Why should we make up our own (board-specific) register manipulation
> tweaks, as opposed to having a function do it for us in dram.c as its
> companion?
> Reasons behind it, are board-specific, so in your case it would turn into:
> 
> +int board_early_init_f(void)
> +{
> +	unsigned long size = get_ram_size(PHYS_SDRAM_1, PHYS_SDRAM_1_SIZE);
> +
> +	/* 256MB module, adjust BAR register */
> +	if (size == 256 * 1024 * 1024)
> +		kw_sdram_bs_set(0, size);
> 
> Which I find cleaner.
> Even the if part could be also removed.
> And would work as-is on any board needing it (ours, for instance).
> 
> What do you think?

I'm all right with it. But Prafulla is the maintainer, so you better negotiate 
with him :-)

> Thank you for your patience,
> Gerlando

Best regards,
Marek Vasut