[U-Boot] [PATCH V2 13/14] ARM: AM43xx: GP_EVM: Add support for DDR3

[Vaibhav Bedia]

On Mon, Nov 25, 2013 at 12:18 AM, Lokesh Vutla <lokeshvutla at ti.com> wrote:
> On Friday 22 November 2013 02:22 AM, Vaibhav Bedia wrote:
>> On Thu, Nov 21, 2013 at 1:18 AM, Lokesh Vutla <lokeshvutla at ti.com> wrote:
>> [...]
>>
>>> -/*
>>> - * Get SDRAM type connected to EMIF.
>>> - * Assuming similar SDRAM parts are connected to both EMIF's
>>> - * which is typically the case. So it is sufficient to get
>>> - * SDRAM type from EMIF1.
>>> - */
>>> -u32 emif_sdram_type()
>>> -{
>>> -       struct emif_reg_struct *emif = (struct emif_reg_struct *)EMIF1_BASE;
>>> -
>>> -       return (readl(&emif->emif_sdram_config) &
>>> -               EMIF_REG_SDRAM_TYPE_MASK) >> EMIF_REG_SDRAM_TYPE_SHIFT;
>>> -}
>>> -
>>>  static inline u32 get_mr(u32 base, u32 cs, u32 mr_addr)
>>>  {
>>>         u32 mr;
>>> diff --git a/arch/arm/include/asm/arch-am33xx/ddr_defs.h b/arch/arm/include/asm/arch-am33xx/ddr_defs.h
>>> index c98ab7f..646e50f 100644
>>> --- a/arch/arm/include/asm/arch-am33xx/ddr_defs.h
>>> +++ b/arch/arm/include/asm/arch-am33xx/ddr_defs.h
>>> @@ -138,6 +138,14 @@
>>>  #define  LPDDR2_DATA2_IOCTRL_VALUE   0x20000294
>>>  #define  LPDDR2_DATA3_IOCTRL_VALUE   0x20000294
>>>
>>> +#define  DDR3_ADDRCTRL_WD0_IOCTRL_VALUE 0x00000000
>>> +#define  DDR3_ADDRCTRL_WD1_IOCTRL_VALUE 0x00000000
>>> +#define  DDR3_ADDRCTRL_IOCTRL_VALUE   0x84
>>> +#define  DDR3_DATA0_IOCTRL_VALUE   0x84
>>> +#define  DDR3_DATA1_IOCTRL_VALUE   0x84
>>> +#define  DDR3_DATA2_IOCTRL_VALUE   0x84
>>> +#define  DDR3_DATA3_IOCTRL_VALUE   0x84
>>> +
>>>  /**
>>>   * Configure DMM
>>>   */
>>> diff --git a/arch/arm/include/asm/emif.h b/arch/arm/include/asm/emif.h
>>> index ce6b229..b4a8c9f 100644
>>> --- a/arch/arm/include/asm/emif.h
>>> +++ b/arch/arm/include/asm/emif.h
>>> @@ -1151,6 +1151,20 @@ static inline u32 get_emif_rev(u32 base)
>>>                 >> EMIF_REG_MAJOR_REVISION_SHIFT;
>>>  }
>>>
>>> +/*
>>> + * Get SDRAM type connected to EMIF.
>>> + * Assuming similar SDRAM parts are connected to both EMIF's
>>> + * which is typically the case. So it is sufficient to get
>>> + * SDRAM type from EMIF1.
>>> + */
>>> +static inline u32 emif_sdram_type(void)
>>> +{
>>> +       struct emif_reg_struct *emif = (struct emif_reg_struct *)EMIF1_BASE;
>>> +
>>> +       return (readl(&emif->emif_sdram_config) &
>>> +               EMIF_REG_SDRAM_TYPE_MASK) >> EMIF_REG_SDRAM_TYPE_SHIFT;
>>> +}
>>> +
>>
>> This change don't make any sense to me. How are the EMIF register bits
>> any indication
>> of what memory time is used especially for DDR2/3?
> What is not making sense here ?
> If you go and read EMIF spec, it is clearly written that on coming out of reset
> HW sequence starts according to the value populated in SDRAM config register.
> As far as I am concerned this is the best way to differentiate between Memories.
>

Take a moment to think about where that register value comes from. Does it
somehow automagically get reconfigured when the chip is put in a board with
LPDDR2 vs DDR2 vs DDR3? While you are at it also look at the JEDEC specs
to figure out is there's some way to probe the DDR2/3 memory types.

> Can you tell me a better way to differentiate between memories in emif file ?
> Definitely  I should not use board_is_foo() functions.
>

AFAIK there is none and hence this way of trying to get the memory type
is broken.

Moreover, my understanding was that one of the prime functions of the EEPROM
board data was to enable differentiation between the memory types.