[U-Boot] question about U-Boot's cpu/ppc4xx/44x_spd_ddr2.c

[Adam Graham]

Stefan, Carolyn, 

> -----Original Message-----
> From: Adam Graham 
> Sent: Monday, January 19, 2009 6:41 PM
> To: Stefan Roese; u-boot at lists.denx.de
> Cc: carolyn.j.smith at tektronix.com; Adam Graham
> Subject: RE: [U-Boot] question about U-Boot's 
> cpu/ppc4xx/44x_spd_ddr2.c
> 
> Thanks Stefan for handling this and thank you Carolyn for 
> finding this bug.
>  
> 
> > -----Original Message-----
> > From: Stefan Roese [mailto:sr at denx.de]
> > Sent: Monday, January 19, 2009 8:10 AM
> > To: u-boot at lists.denx.de
> > Cc: carolyn.j.smith at tektronix.com; Adam Graham
> > Subject: Re: [U-Boot] question about U-Boot's 
> > cpu/ppc4xx/44x_spd_ddr2.c
> > 
> > Hi Carolyn,
> > 
> > On Thursday 15 January 2009, carolyn.j.smith at tektronix.com wrote:
> > > I have a question about some of the code in U-Boot's 
> > > cpu/ppc4xx/44x_spd_ddr2.c, in particular the program_codt
> > function and
> > > these lines of code:
> > >
> > >         mfsdram(SDRAM_CODT, codt);
> > >         codt |= (SDRAM_CODT_IO_NMODE
> > >                  & (~SDRAM_CODT_DQS_SINGLE_END
> > >                     & ~SDRAM_CODT_CKSE_SINGLE_END
> > >                     & ~SDRAM_CODT_FEEBBACK_RCV_SINGLE_END
> > >                     & ~SDRAM_CODT_FEEBBACK_DRV_SINGLE_END));
> > >
> > > What is the intention of this code? As far as I can tell, 
> all it is 
> > > doing is or'ing SDRAM_CODT_IO_NMODE (= 0x00000001) into the codt 
> > > variable and the
> > >
> > >                  & (~SDRAM_CODT_DQS_SINGLE_END
> > >                     & ~SDRAM_CODT_CKSE_SINGLE_END
> > >                     & ~SDRAM_CODT_FEEBBACK_RCV_SINGLE_END
> > >                     & ~SDRAM_CODT_FEEBBACK_DRV_SINGLE_END)
> > >
> > > part of it has no real effect.
> > 
> > Good catch.
> 
> Yes, good catch.
> 
> 
> > 
> > > Was the intention to turn on the SDRAM_CODT_IO_NMODE bit
> > and turn off
> > > the SDRAM_CODT_DQS_SINGLE_END, SDRAM_CODT_CKSE_SINGLE_END, 
> > > SDRAM_CODT_FEEBBACK_RCV_SINGLE_END and 
> > > SDRAM_CODT_FEEBBACK_DRV_SINGLE_END
> > > bits in which case the code should be something like this?
> > >
> > >         codt |= SDRAM_CODT_IO_NMODE;
> > >
> > >         codt &= ~(SDRAM_CODT_DQS_SINGLE_END |
> > SDRAM_CODT_CKSE_SINGLE_END |
> > >                   SDRAM_CODT_FEEBBACK_RCV_SINGLE_END | 
> > > SDRAM_CODT_FEEBBACK_DRV_SINGLE_END);
> > 
> > Not being the original author of this code, I can only 
> guess that this 
> > is what this code *should* do. Please send a patch to fix this.
> > 
> 
> Yes, this is the intent of the code.  Basically the intent is 
> as Carolyn proposed which is to reset the bits for 
> SDRAM_CODT_DQS_SINGLE_END, SDRAM_CODT_CKSE_SINGLE_END, 
> SDRAM_CODT_FEEBBACK_RCV_SINGLE_END, and 
> SDRAM_CODT_FEEBBACK_DRV_SINGLE_END, and then to set the bit 
> for SDRAM_CODT_IO_NMODE.  Putting this in 2 operations is the 
> right thing to do, it is easier to read the code, and it 
> conveys the intention.
> 
> Your code above is the fix for a patch.
> 
> 
> > > Also the 460EX manual I have (revision 1.16 - November 17,
> > 2008) shows
> > > bits
> > > 29 and 30 of the CODT register as Reserved (in which case they 
> > > shouldn't be
> > > modified) while U-Boot has them as
> > >
> > > #define SDRAM_CODT_FEEBBACK_RCV_SINGLE_END      0x00000004
> > > #define SDRAM_CODT_FEEBBACK_DRV_SINGLE_END      0x00000002
> > >
> > > Do I have an out-of-date manual or are they reserved only for the 
> > > 460EX and not for other processors using this code?
> > 
> > No, you seem to be correct here too. I couldn't find those 
> defines in 
> > the 460EX users manual (same version) or in the current 
> 440SPe users 
> > manual.
> 
> It does appear that for the 460EX/GT SDRAM_CODT register bits 
> 29 and 30 are documented as reserved.  These bits are defined 
> in the PPC405EX chip.  I'll ask the PPC460EX/GT chip 
> designers about these bits 29 and 30 and why they are defined 
> in the PPC405EX and not the PPC460EX/GT and send a follow up 
> email and we can then #ifdef the code appropriately.  The 
> PPC405EX and the PPC460EX/GT share a common IBM SDRAM 
> controller core IP, and as such the code in the file 
> 44x_spd_ddr2.c will apply to both chips.

It is in fact the case for all the AMCC 4xx SoC chips that have the IBM
SDRAM memory controller core IP, the SDRAM_CODT register, bits 29 and 30
are reserved bits.  Thanks Carolyn for finding this issue.  We will
update the 44x_spd_ddr2.c file and send out a patch shortly for this
SDRAM_CODT register bits 29-30 issue.

Best,
Adam
AMCC

> 
> Best,
> Adam
> AMCC
> 
> 
> > 
> > Perhaps somebody from AMCC could jump in here. Adam? Any ideas on 
> > this?
> > 
> > Best regards,
> > Stefan
> > 
> > 
> =====================================================================
> > DENX Software Engineering GmbH,     MD: Wolfgang Denk & 
> Detlev Zundel
> > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 
> Groebenzell, Germany
> > Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: 
> > office at denx.de
> > 
> =====================================================================
> >