Re: [U-Boot] [PATCH 1/2] ARM: DRA: EMIF: Change DDR3 settings to use hw leveling

2013-11-07 Thread Tom Rini
On Thu, Nov 07, 2013 at 08:17:39PM +0530, Sricharan R wrote:

 Currently the DDR3 memory on DRA7 ES1.0 evm board is enabled using
 software leveling. This was done since hardware leveling was not
 working. Now that the right sequence to do hw leveling is identified,
 use it. This is required for EMIF clockdomain to idle and come back
 during lowpower usecases.
[snip]
 @@ -210,54 +210,76 @@ static void ddr3_leveling(u32 base, const struct 
 emif_regs *regs)
  {
   struct emif_reg_struct *emif = (struct emif_reg_struct *)base;
  
 - /* keep sdram in self-refresh */
[snip]
 + if (omap_revision() != DRA752_ES1_0){
 + /* keep sdram in self-refresh */
[snip]
 + } else {
 + u32 fifo_reg;
[snip]
 + /* Disable leveling */
 + writel(0x0, emif-emif_rd_wr_lvl_rmp_ctl);
 + }

Two things here.  First, it seems that now ddr3_leveling has one
sequence for not DRA752_ES1_0 (so likely to get wrong when used on
DRA752 whatever comes after ES1.0) and another for DRA752_ES1_0.  This
would be because the it's different EMIF blocks and related HW, so it's
a different sequence, yes?  Second, the comment at the end of this
function about Disable leveling seems misleading, but maybe it's just
me.  We're saying leveling sequence is complete now, yes?

For the second issue, we can expand / clarify the comment.  For the
first issue, maybe we shouldn't have a single ddr3_leveling function
but per family ones?  There's nothing in common between the two cases
here, so we're just gaining an indentation level when we could be
excluding code from the final binary instead (either ifdef or maybe just
separate files?).

-- 
Tom


signature.asc
Description: Digital signature
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


Re: [U-Boot] [PATCH 1/2] ARM: DRA: EMIF: Change DDR3 settings to use hw leveling

2013-11-07 Thread Sricharan R
Hi Tom,

On Thursday 07 November 2013 08:33 PM, Tom Rini wrote:
 On Thu, Nov 07, 2013 at 08:17:39PM +0530, Sricharan R wrote:

 Currently the DDR3 memory on DRA7 ES1.0 evm board is enabled using
 software leveling. This was done since hardware leveling was not
 working. Now that the right sequence to do hw leveling is identified,
 use it. This is required for EMIF clockdomain to idle and come back
 during lowpower usecases.
 [snip]
 @@ -210,54 +210,76 @@ static void ddr3_leveling(u32 base, const struct 
 emif_regs *regs)
  {
  struct emif_reg_struct *emif = (struct emif_reg_struct *)base;
  
 -/* keep sdram in self-refresh */
 [snip]
 +if (omap_revision() != DRA752_ES1_0){
 +/* keep sdram in self-refresh */
 [snip]
 +} else {
 +u32 fifo_reg;
 [snip]
 +/* Disable leveling */
 +writel(0x0, emif-emif_rd_wr_lvl_rmp_ctl);
 +}
 Two things here.  First, it seems that now ddr3_leveling has one
 sequence for not DRA752_ES1_0 (so likely to get wrong when used on
 DRA752 whatever comes after ES1.0) and another for DRA752_ES1_0.  This
 would be because the it's different EMIF blocks and related HW, so it's
 a different sequence, yes?  Second, the comment at the end of this
 function about Disable leveling seems misleading, but maybe it's just
 me.  We're saying leveling sequence is complete now, yes?

 For the second issue, we can expand / clarify the comment.  For the
 first issue, maybe we shouldn't have a single ddr3_leveling function
 but per family ones?  There's nothing in common between the two cases
 here, so we're just gaining an indentation level when we could be
 excluding code from the final binary instead (either ifdef or maybe just
 separate files?).

 Yes, i also thought about having a separate function for OMAP5/DRA.
 I will do that. For the point about disable leveling, it was intentional.
 
 After we are done with a leveling once during boot, we do not intend to
 enable it anymore. We see that the PHY misbehaves by triggering a wrong
 leveling sequence when EMIF hits idle, which results in wrong delay values
 if it is left enabled. I will add this comment in V2

Regards,
 Sricharan

 

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot