[U-Boot,2/8] orion5x: Fix wrong address in orion5x_sdram_bar

This code intends to read the SDRAM controller base address registers
but is instead reading the CPU window base address registers.

Signed-off-by: Michael Spang <mspang@csclub.uwaterloo.ca>
---
 arch/arm/cpu/arm926ejs/orion5x/dram.c       |    2 +-
 arch/arm/include/asm/arch-orion5x/orion5x.h |    1 +
 2 files changed, 2 insertions(+), 1 deletions(-)

Hi Michael,

Le 29/01/2011 02:00, Michael Spang a écrit :
> This code intends to read the SDRAM controller base address registers
> but is instead reading the CPU window base address registers.

Side note: IIUC this change is not required since the CPU Window 
registers match the SDRAM controller registers on orion5x in U-boot; but 
it is fine if only for the sake of correctness, and assuming it works 
for other orion5x boards (testing underway for edminiv2).

> Signed-off-by: Michael Spang<mspang@csclub.uwaterloo.ca>
> ---
>   arch/arm/cpu/arm926ejs/orion5x/dram.c       |    2 +-
>   arch/arm/include/asm/arch-orion5x/orion5x.h |    1 +
>   2 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/arch/arm/cpu/arm926ejs/orion5x/dram.c b/arch/arm/cpu/arm926ejs/orion5x/dram.c
> index b749282..7f3a318 100644
> --- a/arch/arm/cpu/arm926ejs/orion5x/dram.c
> +++ b/arch/arm/cpu/arm926ejs/orion5x/dram.c
> @@ -38,7 +38,7 @@ u32 orion5x_sdram_bar(enum memory_bank bank)
>   {
>   	struct orion5x_ddr_addr_decode_registers *winregs =
>   		(struct orion5x_ddr_addr_decode_registers *)

Please remove the 'ddr_' part of the struct name while you're at it: 
this struct is not DDR specific, it also covers CPU window register.

> -		ORION5X_CPU_WIN_BASE;
> +		ORION5X_SDRAM_CTRL_BASE;
>
>   	u32 result = 0;
>   	u32 enable = 0x01&  winregs[bank].size;
> diff --git a/arch/arm/include/asm/arch-orion5x/orion5x.h b/arch/arm/include/asm/arch-orion5x/orion5x.h
> index e3d3f76..f262ad1 100644
> --- a/arch/arm/include/asm/arch-orion5x/orion5x.h
> +++ b/arch/arm/include/asm/arch-orion5x/orion5x.h
> @@ -42,6 +42,7 @@
>   #define ORION5X_REGISTER(x)			(ORION5X_REGS_PHY_BASE + x)
>
>   /* Documented registers */
> +#define ORION5X_SDRAM_CTRL_BASE			(ORION5X_REGISTER(0x01500))
>   #define ORION5X_TWSI_BASE			(ORION5X_REGISTER(0x11000))
>   #define ORION5X_UART0_BASE			(ORION5X_REGISTER(0x12000))
>   #define ORION5X_UART1_BASE			(ORION5X_REGISTER(0x12100))

Amicalement,

On Sat, Jan 29, 2011 at 2:24 AM, Albert ARIBAUD <albert.aribaud@free.fr> wrote:
>> diff --git a/arch/arm/cpu/arm926ejs/orion5x/dram.c
>> b/arch/arm/cpu/arm926ejs/orion5x/dram.c
>> index b749282..7f3a318 100644
>> --- a/arch/arm/cpu/arm926ejs/orion5x/dram.c
>> +++ b/arch/arm/cpu/arm926ejs/orion5x/dram.c
>> @@ -38,7 +38,7 @@ u32 orion5x_sdram_bar(enum memory_bank bank)
>>  {
>>        struct orion5x_ddr_addr_decode_registers *winregs =
>>                (struct orion5x_ddr_addr_decode_registers *)
>
> Please remove the 'ddr_' part of the struct name while you're at it: this
> struct is not DDR specific, it also covers CPU window register.

Done.

Michael

On 2011/01/29 9:24 AM, Albert ARIBAUD wrote:
> Hi Michael,
> 
> Le 29/01/2011 02:00, Michael Spang a écrit :
>> This code intends to read the SDRAM controller base address registers
>> but is instead reading the CPU window base address registers.
> 
> Side note: IIUC this change is not required since the CPU Window 
> registers match the SDRAM controller registers on orion5x in U-boot; but 
> it is fine if only for the sake of correctness, and assuming it works 
> for other orion5x boards (testing underway for edminiv2).

I have this same change in my patch set for the DNS323, for what it is
worth. I must have needed it, but I can't remember exactly what the
behaviour was without it. :-)

Rogan

On Fri, Mar 18, 2011 at 1:57 AM, Rogan Dawes <rogan@dawes.za.net> wrote:
> On 2011/01/29 9:24 AM, Albert ARIBAUD wrote:
>> Hi Michael,
>>
>> Le 29/01/2011 02:00, Michael Spang a écrit :
>>> This code intends to read the SDRAM controller base address registers
>>> but is instead reading the CPU window base address registers.
>>
>> Side note: IIUC this change is not required since the CPU Window
>> registers match the SDRAM controller registers on orion5x in U-boot; but
>> it is fine if only for the sake of correctness, and assuming it works
>> for other orion5x boards (testing underway for edminiv2).
>
> I have this same change in my patch set for the DNS323, for what it is
> worth. I must have needed it, but I can't remember exactly what the
> behaviour was without it. :-)
>
> Rogan
>

I was having trouble remembering what was wrong as well. It turns out
the CPU window registers are not actually laid out in the way that the
structure and code expect. The "cpu address map registers" have the
layout

struct {
    u32 control;
    u32 base;
};

which is not the same as the SD-RAM registers:

struct {
    u32 base;
    u32 size;
};

The result is that the test to see if the window is enabled fails,
even though it is enabled. This works by coincidence for the TS-7800,
because the window being disabled causes the correct base address of 0
to be returned.

Michael