Patchwork PowerPC 440EPx/GRx fix memory size calculation

login
register
mail settings
Submitter Valentine Barshak
Date March 10, 2009, 7:50 p.m.
Message ID <20090310195013.GA27835@ru.mvista.com>
Download mbox | patch
Permalink /patch/24265/
State Superseded
Delegated to: Josh Boyer
Headers show

Comments

Valentine Barshak - March 10, 2009, 7:50 p.m.
I was just going to submit a patch for that too.
Indeed, the denali_fixup_memsize() miscalculated a couple of address
field widths. We were lucky to eventually get the right result,
because the effect of the first error was killed by the other one.
According to the AMCC 440EPX/GRX user manual,
the Chip Select width is always fixed at 1 bit no matter
what is actually read from register DDR_10.
The workaround is to use a predefined chipselect value for 440EPx/GRx.
Also, setting the REDUC bit (REDUC = 1) enables 32-bit data path.
If REDUC = 0, full data path of 64 bits is used.

Signed-off-by: Valentine Barshak <vbarshak@ru.mvista.com>
Signed-off-by: Mikhail Zolotaryov <lebon@lebon.org.ua>
Mikhail Zolotaryov - March 10, 2009, 8:57 p.m.
Valentine Barshak wrote:
> According to the AMCC 440EPX/GRX user manual,
> the Chip Select width is always fixed at 1 bit no matter
> what is actually read from register DDR_10.

Well, from my point of view original kernel code is correct in this part.

Adding one bit into memory address means multiplying memory size by 2 
i.e. cs=2. The question is: is Chip Select bit used in memory address. 
ChipSelect input of memory chip enables or disabled it, so if we have 
only one BankSel installed/connected (DDR0_10[22:23] is 01 or 10) 
there's no need to use Chip Select bit in an address. On the contrary, 
if both BankSel lines are connected (DDR0_10[22:23] is 11), to let 
memory controller know which memory rank to use, Chip Select bit is 
added into memory address. (and yes, if DDR0_10[22:23] is 00 - no ranks 
installed, memory size is 0, cs=0)

Original kernel code use exactly the same logic as I described above. 
Please suggest if it's wrong.
Valentine Barshak - March 11, 2009, 1:40 a.m.
Mikhail Zolotaryov wrote:
> Valentine Barshak wrote:
>> According to the AMCC 440EPX/GRX user manual,
>> the Chip Select width is always fixed at 1 bit no matter
>> what is actually read from register DDR_10.
>
> Well, from my point of view original kernel code is correct in this part.
>
> Adding one bit into memory address means multiplying memory size by 2 
> i.e. cs=2. The question is: is Chip Select bit used in memory address. 
> ChipSelect input of memory chip enables or disabled it, so if we have 
> only one BankSel installed/connected (DDR0_10[22:23] is 01 or 10) 
> there's no need to use Chip Select bit in an address. On the contrary, 
> if both BankSel lines are connected (DDR0_10[22:23] is 11), to let 
> memory controller know which memory rank to use, Chip Select bit is 
> added into memory address. (and yes, if DDR0_10[22:23] is 00 - no 
> ranks installed, memory size is 0, cs=0)
Yes, you could phrase it that way. According to the PPC440EPx manual, 
the total memory size is calculated based on the following formula:
memsize = cs * (1 << (col+row)) * bank * dpath;
So, if both chipselects are used, we add an extra bit to the memory 
address to distinguish between these chipselects.
There's nothing wrong with this part of the code.
The problem is that the controller is hardwired to use only one 
chipselect, even if both are enabled in the DDR0_10 on PPC440EPx/GRx 
processors.
So, the patch provides a workaround to always use single cs for 
440EPx/GRx (use predefined value instead of reading DDR0_10).
>
> Original kernel code use exactly the same logic as I described above. 
> Please suggest if it's wrong.
Thanks,
Valentine.
Benjamin Herrenschmidt - March 11, 2009, 2:26 a.m.
> Yes, you could phrase it that way. According to the PPC440EPx manual, 
> the total memory size is calculated based on the following formula:
> memsize = cs * (1 << (col+row)) * bank * dpath;
> So, if both chipselects are used, we add an extra bit to the memory 
> address to distinguish between these chipselects.
> There's nothing wrong with this part of the code.
> The problem is that the controller is hardwired to use only one 
> chipselect, even if both are enabled in the DDR0_10 on PPC440EPx/GRx 
> processors.
> So, the patch provides a workaround to always use single cs for 
> 440EPx/GRx (use predefined value instead of reading DDR0_10).

Mikhail, can you verify that Valentine's patch works for you ?

Cheers,
Ben.
Mikhail Zolotaryov - March 11, 2009, 8:24 a.m.
Benjamin Herrenschmidt wrote:
>> The problem is that the controller is hardwired to use only one 
>> chipselect, even if both are enabled in the DDR0_10 on PPC440EPx/GRx 
>> processors
>
> Mikhail, can you verify that Valentine's patch works for you ?

Ben, unfortunately on my board(s) I don't have both bits enabled in 
DDR0_10 i.e. I'll have cs=1 calculated even by original Linux code.
Mikhail Zolotaryov - March 11, 2009, 8:29 a.m.
Valentine wrote:
> The problem is that the controller is hardwired to use only one 
> chipselect, even if both are enabled in the DDR0_10 on PPC440EPx/GRx 
> processors.
>
It's new information for me. Is this problem described by some ERRATA or 
manual, could you please point me to the document (and page) ?
Josh Boyer - March 11, 2009, 10:37 a.m.
On Tue, Mar 10, 2009 at 10:50:13PM +0300, Valentine Barshak wrote:
>I was just going to submit a patch for that too.
>Indeed, the denali_fixup_memsize() miscalculated a couple of address
>field widths. We were lucky to eventually get the right result,
>because the effect of the first error was killed by the other one.
>According to the AMCC 440EPX/GRX user manual,
>the Chip Select width is always fixed at 1 bit no matter
>what is actually read from register DDR_10.
>The workaround is to use a predefined chipselect value for 440EPx/GRx.
>Also, setting the REDUC bit (REDUC = 1) enables 32-bit data path.
>If REDUC = 0, full data path of 64 bits is used.
>
>Signed-off-by: Valentine Barshak <vbarshak@ru.mvista.com>
>Signed-off-by: Mikhail Zolotaryov <lebon@lebon.org.ua>

I've been looking over this one a bit more.  At the moment, I'm inclined
to queue this up in my -next branch.  I would like to see if Mikhail
could test it though, and have Valentine answer the question in the hard
wired part.

josh
Valentine Barshak - March 11, 2009, 7:06 p.m.
Josh Boyer wrote:
> On Tue, Mar 10, 2009 at 10:50:13PM +0300, Valentine Barshak wrote:
>> I was just going to submit a patch for that too.
>> Indeed, the denali_fixup_memsize() miscalculated a couple of address
>> field widths. We were lucky to eventually get the right result,
>> because the effect of the first error was killed by the other one.
>> According to the AMCC 440EPX/GRX user manual,
>> the Chip Select width is always fixed at 1 bit no matter
>> what is actually read from register DDR_10.
>> The workaround is to use a predefined chipselect value for 440EPx/GRx.
>> Also, setting the REDUC bit (REDUC = 1) enables 32-bit data path.
>> If REDUC = 0, full data path of 64 bits is used.
>>
>> Signed-off-by: Valentine Barshak <vbarshak@ru.mvista.com>
>> Signed-off-by: Mikhail Zolotaryov <lebon@lebon.org.ua>
> 
> I've been looking over this one a bit more.  At the moment, I'm inclined
> to queue this up in my -next branch.  I would like to see if Mikhail
> could test it though, and have Valentine answer the question in the hard
> wired part.

I've been looking at the docs once again and actually I couldn't find an 
   explanation there. And I don't have that e-mail from AMCC support 
that I got a while back regarding the issue anymore.
There might have been some misunderstanding.
The docs (PPC440EPX UM 19.2 Device Address Mapping) say that the chip 
select field width is always fixed at one bit, but this doesn't actually 
  mean that there's always one chip select used.
The patch works fine on Sequoia and another Sequoia-like board with 1GB 
RAM installed, but it might not work with 2GB RAM. I've tried to play 
with DDR0_10 settings and Sequoia works fine regardless of what's 
actually written to DDR0_10.
So, probably the best way would be to fix that in u-boot
amcc/sequoia/sdram.c by doing mtsdram(DDR0_10, 0x00000100); instead of 
mtsdram(DDR0_10, 0x00000300);
Sorry, for confusion, but after reviewing the docs, I think that
only REDUC interpretation has to be fixed. The chips select part should 
be fixed in u-boot sdram code for Sequoia as was originally proposed by 
Mikhail.

Stefan, could you please take a look?

Thanks,
Valentine.

> 
> josh
Josh Boyer - March 11, 2009, 9:57 p.m.
On Wed, Mar 11, 2009 at 10:06:11PM +0300, Valentine Barshak wrote:
> Josh Boyer wrote:
>> On Tue, Mar 10, 2009 at 10:50:13PM +0300, Valentine Barshak wrote:
>>> I was just going to submit a patch for that too.
>>> Indeed, the denali_fixup_memsize() miscalculated a couple of address
>>> field widths. We were lucky to eventually get the right result,
>>> because the effect of the first error was killed by the other one.
>>> According to the AMCC 440EPX/GRX user manual,
>>> the Chip Select width is always fixed at 1 bit no matter
>>> what is actually read from register DDR_10.
>>> The workaround is to use a predefined chipselect value for 440EPx/GRx.
>>> Also, setting the REDUC bit (REDUC = 1) enables 32-bit data path.
>>> If REDUC = 0, full data path of 64 bits is used.
>>>
>>> Signed-off-by: Valentine Barshak <vbarshak@ru.mvista.com>
>>> Signed-off-by: Mikhail Zolotaryov <lebon@lebon.org.ua>
>>
>> I've been looking over this one a bit more.  At the moment, I'm inclined
>> to queue this up in my -next branch.  I would like to see if Mikhail
>> could test it though, and have Valentine answer the question in the hard
>> wired part.
>
> I've been looking at the docs once again and actually I couldn't find an  
>   explanation there. And I don't have that e-mail from AMCC support that 
> I got a while back regarding the issue anymore.
> There might have been some misunderstanding.
> The docs (PPC440EPX UM 19.2 Device Address Mapping) say that the chip  
> select field width is always fixed at one bit, but this doesn't actually  
>  mean that there's always one chip select used.
> The patch works fine on Sequoia and another Sequoia-like board with 1GB  
> RAM installed, but it might not work with 2GB RAM. I've tried to play  
> with DDR0_10 settings and Sequoia works fine regardless of what's  
> actually written to DDR0_10.
> So, probably the best way would be to fix that in u-boot
> amcc/sequoia/sdram.c by doing mtsdram(DDR0_10, 0x00000100); instead of  
> mtsdram(DDR0_10, 0x00000300);
> Sorry, for confusion, but after reviewing the docs, I think that
> only REDUC interpretation has to be fixed. The chips select part should  
> be fixed in u-boot sdram code for Sequoia as was originally proposed by  
> Mikhail.

Ok, so we're back to using Mikhail's original patch then?

josh
Valentine Barshak - March 11, 2009, 10:08 p.m.
Josh Boyer wrote:
> On Wed, Mar 11, 2009 at 10:06:11PM +0300, Valentine Barshak wrote:
>> Josh Boyer wrote:
>>> On Tue, Mar 10, 2009 at 10:50:13PM +0300, Valentine Barshak wrote:
>>>> I was just going to submit a patch for that too.
>>>> Indeed, the denali_fixup_memsize() miscalculated a couple of address
>>>> field widths. We were lucky to eventually get the right result,
>>>> because the effect of the first error was killed by the other one.
>>>> According to the AMCC 440EPX/GRX user manual,
>>>> the Chip Select width is always fixed at 1 bit no matter
>>>> what is actually read from register DDR_10.
>>>> The workaround is to use a predefined chipselect value for 440EPx/GRx.
>>>> Also, setting the REDUC bit (REDUC = 1) enables 32-bit data path.
>>>> If REDUC = 0, full data path of 64 bits is used.
>>>>
>>>> Signed-off-by: Valentine Barshak <vbarshak@ru.mvista.com>
>>>> Signed-off-by: Mikhail Zolotaryov <lebon@lebon.org.ua>
>>> I've been looking over this one a bit more.  At the moment, I'm inclined
>>> to queue this up in my -next branch.  I would like to see if Mikhail
>>> could test it though, and have Valentine answer the question in the hard
>>> wired part.
>> I've been looking at the docs once again and actually I couldn't find an  
>>   explanation there. And I don't have that e-mail from AMCC support that 
>> I got a while back regarding the issue anymore.
>> There might have been some misunderstanding.
>> The docs (PPC440EPX UM 19.2 Device Address Mapping) say that the chip  
>> select field width is always fixed at one bit, but this doesn't actually  
>>  mean that there's always one chip select used.
>> The patch works fine on Sequoia and another Sequoia-like board with 1GB  
>> RAM installed, but it might not work with 2GB RAM. I've tried to play  
>> with DDR0_10 settings and Sequoia works fine regardless of what's  
>> actually written to DDR0_10.
>> So, probably the best way would be to fix that in u-boot
>> amcc/sequoia/sdram.c by doing mtsdram(DDR0_10, 0x00000100); instead of  
>> mtsdram(DDR0_10, 0x00000300);
>> Sorry, for confusion, but after reviewing the docs, I think that
>> only REDUC interpretation has to be fixed. The chips select part should  
>> be fixed in u-boot sdram code for Sequoia as was originally proposed by  
>> Mikhail.
> 
> Ok, so we're back to using Mikhail's original patch then?
> 
> josh

Yes, but until u-boot is fixed this will break Sequoia/Rainier support.
Thanks,
Valentine.
Josh Boyer - March 11, 2009, 11:07 p.m.
On Thu, Mar 12, 2009 at 01:08:59AM +0300, Valentine wrote:
>>> So, probably the best way would be to fix that in u-boot
>>> amcc/sequoia/sdram.c by doing mtsdram(DDR0_10, 0x00000100); instead 
>>> of  mtsdram(DDR0_10, 0x00000300);
>>> Sorry, for confusion, but after reviewing the docs, I think that
>>> only REDUC interpretation has to be fixed. The chips select part 
>>> should  be fixed in u-boot sdram code for Sequoia as was originally 
>>> proposed by  Mikhail.
>>
>> Ok, so we're back to using Mikhail's original patch then?
>>
>> josh
>
> Yes, but until u-boot is fixed this will break Sequoia/Rainier support.

Well, that's sort of a problem.  The wrapper will have to deal with both
a fixed and unfixed u-boot because not everyone will update their u-boot
with the fix.

So we need a patch for the wrapper that works in all cases.

josh
Stefan Roese - March 12, 2009, 6:02 a.m.
On Wednesday 11 March 2009, Valentine Barshak wrote:
> I've been looking at the docs once again and actually I couldn't find an
>    explanation there. And I don't have that e-mail from AMCC support
> that I got a while back regarding the issue anymore.
> There might have been some misunderstanding.
> The docs (PPC440EPX UM 19.2 Device Address Mapping) say that the chip
> select field width is always fixed at one bit, but this doesn't actually
>   mean that there's always one chip select used.
> The patch works fine on Sequoia and another Sequoia-like board with 1GB
> RAM installed, but it might not work with 2GB RAM. I've tried to play
> with DDR0_10 settings and Sequoia works fine regardless of what's
> actually written to DDR0_10.
> So, probably the best way would be to fix that in u-boot
> amcc/sequoia/sdram.c by doing mtsdram(DDR0_10, 0x00000100); instead of
> mtsdram(DDR0_10, 0x00000300);
> Sorry, for confusion, but after reviewing the docs, I think that
> only REDUC interpretation has to be fixed. The chips select part should
> be fixed in u-boot sdram code for Sequoia as was originally proposed by
> Mikhail.
>
> Stefan, could you please take a look?

I'll apply the U-Boot patch today. But as Josh pointed out, we should try to 
find a way for the bootwrapper to work in all cases.

Best regards,
Stefan
Benjamin Herrenschmidt - March 12, 2009, 7:32 a.m.
On Thu, 2009-03-12 at 07:02 +0100, Stefan Roese wrote:
> 
> I'll apply the U-Boot patch today. But as Josh pointed out, we should
> try to 
> find a way for the bootwrapper to work in all cases.

uboot is passing some kind of bt_t to the wrapper or a full
device-tree ?

Ben.
Stefan Roese - March 12, 2009, 8:05 a.m.
On Thursday 12 March 2009, Benjamin Herrenschmidt wrote:
> On Thu, 2009-03-12 at 07:02 +0100, Stefan Roese wrote:
> > I'll apply the U-Boot patch today. But as Josh pointed out, we should
> > try to
> > find a way for the bootwrapper to work in all cases.
>
> uboot is passing some kind of bt_t to the wrapper or a full
> device-tree ?

Both is possible. Older U-Boot versions only passed the bd_t struct to the 
kernel. For those U-Boot's the wrapper is needed. More recent U-Boot versions 
support passing a device-tree blob to the kernel. U-Boot patches the correct 
memory size in this blob.

As a matter of fact, I never used the wrapper before. U-Boot supports passing 
the device-tree blob to Linux since quite some time now.

Best regards,
Stefan
Benjamin Herrenschmidt - March 12, 2009, 8:12 a.m.
On Thu, 2009-03-12 at 09:05 +0100, Stefan Roese wrote:
> 
> Both is possible. Older U-Boot versions only passed the bd_t struct to the 
> kernel. For those U-Boot's the wrapper is needed. More recent U-Boot versions 
> support passing a device-tree blob to the kernel. U-Boot patches the correct 
> memory size in this blob.
> 
> As a matter of fact, I never used the wrapper before. U-Boot supports passing 
> the device-tree blob to Linux since quite some time now.

Yes, that's also how I use it on canyonlands... now, the wrapper could
probably be used to look at the bd_t anyways, no ? Either get the mem
size from there or some flag or version in there can indicate if it's
been "fixed".

Ben.
Stefan Roese - March 12, 2009, 8:24 a.m.
On Thursday 12 March 2009, Benjamin Herrenschmidt wrote:
> On Thu, 2009-03-12 at 09:05 +0100, Stefan Roese wrote:
> > Both is possible. Older U-Boot versions only passed the bd_t struct to
> > the kernel. For those U-Boot's the wrapper is needed. More recent U-Boot
> > versions support passing a device-tree blob to the kernel. U-Boot patches
> > the correct memory size in this blob.
> >
> > As a matter of fact, I never used the wrapper before. U-Boot supports
> > passing the device-tree blob to Linux since quite some time now.
>
> Yes, that's also how I use it on canyonlands... now, the wrapper could
> probably be used to look at the bd_t anyways, no ?

Sure.

> Either get the mem 
> size from there or some flag or version in there can indicate if it's
> been "fixed".

I don't think that we have some flag and/or version information in the bd_info 
struct. And extending this struct doesn't sound like a good idea to me.

Best regards,
Stefan
Mikhail Zolotaryov - March 12, 2009, 8:45 a.m.
Stefan Roese wrote:
>> Either get the mem 
>> size from there or some flag or version in there can indicate if it's
>> been "fixed".
>
> I don't think that we have some flag and/or version information in the bd_info 
> struct. And extending this struct doesn't sound like a good idea to me.

May I suggest an easier way ?

The problem we currently have is some evaluation board(s), we know them, 
use wrong DDR configuration parameters, so do as U-Boot does - simply 
hardcode memory size for these particular board(s), don't calculate, but 
use patched function to calculate memory size for all other boards, 
including variety of customers' made. To be absolutely sure, we can 
check board revision register - it's theoretically possible that future 
board revisions will have more or less memory installed.

This way we can avoid U-Boot to Linux compatibility issues.
Josh Boyer - March 12, 2009, 10:45 a.m.
On Thu, Mar 12, 2009 at 09:24:13AM +0100, Stefan Roese wrote:
>On Thursday 12 March 2009, Benjamin Herrenschmidt wrote:
>> On Thu, 2009-03-12 at 09:05 +0100, Stefan Roese wrote:
>> > Both is possible. Older U-Boot versions only passed the bd_t struct to
>> > the kernel. For those U-Boot's the wrapper is needed. More recent U-Boot
>> > versions support passing a device-tree blob to the kernel. U-Boot patches
>> > the correct memory size in this blob.
>> >
>> > As a matter of fact, I never used the wrapper before. U-Boot supports
>> > passing the device-tree blob to Linux since quite some time now.
>>
>> Yes, that's also how I use it on canyonlands... now, the wrapper could
>> probably be used to look at the bd_t anyways, no ?
>
>Sure.

Do newer U-Boot versions pass both the dtb and the bd_t?  If not, the wrapper
would have to look for one, then the other and not get confused.

>> Either get the mem 
>> size from there or some flag or version in there can indicate if it's
>> been "fixed".
>
>I don't think that we have some flag and/or version information in the bd_info 
>struct. And extending this struct doesn't sound like a good idea to me.

Yeah, we've already had some issues pop up in the past where the bd_t wasn't
correct for a board in the U-Boot version that shipped with it (like the acadia
boards).  There's not much that can be done to fix it.

josh
Stefan Roese - March 12, 2009, 11:02 a.m.
On Thursday 12 March 2009, Josh Boyer wrote:
> >> Yes, that's also how I use it on canyonlands... now, the wrapper could
> >> probably be used to look at the bd_t anyways, no ?
> >
> >Sure.
>
> Do newer U-Boot versions pass both the dtb and the bd_t?

Both is possible. The user can choose by using different boot commands (with 
or without device tree blob). When using the wrapper, the boot command has to 
be without the device tree and therefor the bd_t is passed to the kernel.

Best regards,
Stefan
fkan@amcc.com - March 13, 2009, 11:01 p.m.
Hi Guys:
     Sequoia uses on board discrete memory with one rank. So one chip 
select would be fine.
Turning on both won't matter, since the other cs is never used.

Feng Kan

 Stefan Roese wrote:
> On Wednesday 11 March 2009, Valentine Barshak wrote:
>   
>> I've been looking at the docs once again and actually I couldn't find an
>>    explanation there. And I don't have that e-mail from AMCC support
>> that I got a while back regarding the issue anymore.
>> There might have been some misunderstanding.
>> The docs (PPC440EPX UM 19.2 Device Address Mapping) say that the chip
>> select field width is always fixed at one bit, but this doesn't actually
>>   mean that there's always one chip select used.
>> The patch works fine on Sequoia and another Sequoia-like board with 1GB
>> RAM installed, but it might not work with 2GB RAM. I've tried to play
>> with DDR0_10 settings and Sequoia works fine regardless of what's
>> actually written to DDR0_10.
>> So, probably the best way would be to fix that in u-boot
>> amcc/sequoia/sdram.c by doing mtsdram(DDR0_10, 0x00000100); instead of
>> mtsdram(DDR0_10, 0x00000300);
>> Sorry, for confusion, but after reviewing the docs, I think that
>> only REDUC interpretation has to be fixed. The chips select part should
>> be fixed in u-boot sdram code for Sequoia as was originally proposed by
>> Mikhail.
>>
>> Stefan, could you please take a look?
>>     
>
> I'll apply the U-Boot patch today. But as Josh pointed out, we should try to 
> find a way for the bootwrapper to work in all cases.
>
> Best regards,
> Stefan
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev
>
>

Patch

--- a/arch/powerpc/boot/4xx.c	2008-04-26 02:18:34.000000000 +0400
+++ b/arch/powerpc/boot/4xx.c	2008-10-26 01:40:27.000000000 +0400
@@ -173,15 +173,20 @@  void ibm4xx_denali_fixup_memsize(void)
 	max_col = DDR_GET_VAL(val, DDR_MAX_COL_REG, DDR_MAX_COL_REG_SHIFT);
 	max_row = DDR_GET_VAL(val, DDR_MAX_ROW_REG, DDR_MAX_ROW_REG_SHIFT);
 
-	/* get CS value */
-	val = SDRAM0_READ(DDR0_10);
-
-	val = DDR_GET_VAL(val, DDR_CS_MAP, DDR_CS_MAP_SHIFT);
-	cs = 0;
-	while (val) {
-		if (val & 0x1)
-			cs++;
-		val = val >> 1;
+	/* 440EPx/GRx chipselect always fixed at 1 bit */
+	if ((mfpvr() & 0xf0000ff0) == 0x200008D0)
+		cs = 1;
+	else {
+		/* get CS value */
+		val = SDRAM0_READ(DDR0_10);
+		val = DDR_GET_VAL(val, DDR_CS_MAP, DDR_CS_MAP_SHIFT);
+
+		cs = 0;
+		while (val) {
+			if (val & 0x1)
+				cs++;
+			val = val >> 1;
+		}
 	}
 
 	if (!cs)
@@ -192,7 +197,7 @@  void ibm4xx_denali_fixup_memsize(void)
 	/* get data path bytes */
 	val = SDRAM0_READ(DDR0_14);
 
-	if (DDR_GET_VAL(val, DDR_REDUC, DDR_REDUC_SHIFT))
+	if (!DDR_GET_VAL(val, DDR_REDUC, DDR_REDUC_SHIFT))
 		dpath = 8; /* 64 bits */
 	else
 		dpath = 4; /* 32 bits */