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

Also add the CPUCS register definition.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Prafulla Wadaskar <prafulla@marvell.com>
Cc: Wolfgang Denk <wd@denx.de>
---
 arch/arm/cpu/arm926ejs/kirkwood/dram.c        |    2 --
 arch/arm/include/asm/arch-kirkwood/kirkwood.h |    9 +++++++++
 2 files changed, 9 insertions(+), 2 deletions(-)

Dear Maruk,

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().

What do you think?

Thank you,
Gerlando

Dear Gerlando Falauto,

> Dear Maruk,
> 
> 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

> What do you think?
> 
> Thank you,
> Gerlando

Best regards,
Marek Vasut

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?

Thank you for your patience,
Gerlando

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