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

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

[carolyn.j.smith at tektronix.com]

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.

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);

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?

Thanks,
Carolyn

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

[Stefan Roese]

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.

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

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

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
=====================================================================

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

[Adam Graham]

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.

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 
> =====================================================================
> 

[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
> > 
> =====================================================================
> >