[PATCH] powerpc 4xx: DDR0_14[REDUC] decoded incorrectly

[PATCH] powerpc 4xx: DDR0_14[REDUC] decoded incorrectly

[Mikhail Zolotaryov]

[-- Attachment #1: Type: text/plain, Size: 434 bytes --]

Hi,

according to the PPC440EPx Embedded Processor User Manual (rev 1.14) 
DDR0_14[REDUC] bit has the following meaning:

REDUC - Enable the half data path feature of the controller
0 = Standard operation using full 64/72-bit memory bus
1 = Memory data path width is 32/40 bits

However, "1" is decoded as 64 bit (8 byte), "0" - as 32 bit (4 byte) bus 
by the kernel code. My understanding is this is not correct - patch 
attached.



[-- Attachment #2: patch-ibm4xx_denali_fixup_memsize.patch --]
[-- Type: text/plain, Size: 412 bytes --]

--- linux.orig/arch/powerpc/boot/4xx.c	2009-03-09 17:55:01.000000000 +0200
+++ linux/arch/powerpc/boot/4xx.c	2009-03-09 17:58:07.000000000 +0200
@@ -193,9 +193,9 @@
 	val = SDRAM0_READ(DDR0_14);
 
 	if (DDR_GET_VAL(val, DDR_REDUC, DDR_REDUC_SHIFT))
-		dpath = 8; /* 64 bits */
-	else
 		dpath = 4; /* 32 bits */
+	else
+		dpath = 8; /* 64 bits */
 
 	/* get address pins (rows) */
  	val = SDRAM0_READ(DDR0_42);

[PATCH] powerpc 4xx: DDR0_14[REDUC] decoded incorrectly

[Mikhail Zolotaryov]

[-- Attachment #1: Type: text/plain, Size: 434 bytes --]

Hi,

according to the PPC440EPx Embedded Processor User Manual (rev 1.14) 
DDR0_14[REDUC] bit has the following meaning:

REDUC - Enable the half data path feature of the controller
0 = Standard operation using full 64/72-bit memory bus
1 = Memory data path width is 32/40 bits

However, "1" is decoded as 64 bit (8 byte), "0" - as 32 bit (4 byte) bus 
by the kernel code. My understanding is this is not correct - patch 
attached.



[-- Attachment #2: patch-ibm4xx_denali_fixup_memsize.patch --]
[-- Type: text/plain, Size: 412 bytes --]

--- linux.orig/arch/powerpc/boot/4xx.c	2009-03-09 17:55:01.000000000 +0200
+++ linux/arch/powerpc/boot/4xx.c	2009-03-09 17:58:07.000000000 +0200
@@ -193,9 +193,9 @@
 	val = SDRAM0_READ(DDR0_14);
 
 	if (DDR_GET_VAL(val, DDR_REDUC, DDR_REDUC_SHIFT))
-		dpath = 8; /* 64 bits */
-	else
 		dpath = 4; /* 32 bits */
+	else
+		dpath = 8; /* 64 bits */
 
 	/* get address pins (rows) */
  	val = SDRAM0_READ(DDR0_42);

Re: [PATCH] powerpc 4xx: DDR0_14[REDUC] decoded incorrectly

[Josh Boyer]

On Mon, Mar 09, 2009 at 06:21:36PM +0200, Mikhail Zolotaryov wrote:
> Hi,
>
> according to the PPC440EPx Embedded Processor User Manual (rev 1.14)  
> DDR0_14[REDUC] bit has the following meaning:
>
> REDUC - Enable the half data path feature of the controller
> 0 = Standard operation using full 64/72-bit memory bus
> 1 = Memory data path width is 32/40 bits
>
> However, "1" is decoded as 64 bit (8 byte), "0" - as 32 bit (4 byte) bus  
> by the kernel code. My understanding is this is not correct - patch  
> attached.

You are missing the Signed-off-by tag that is needed.

Aside from that, I'm curious if you found this just through inspection, or
if you actually had a problem because of it.  Your fix seems correct, yet
I've had no problems with my boards at all.

josh

Re: [PATCH] powerpc 4xx: DDR0_14[REDUC] decoded incorrectly

[Josh Boyer]

On Mon, Mar 09, 2009 at 06:21:36PM +0200, Mikhail Zolotaryov wrote:
> Hi,
>
> according to the PPC440EPx Embedded Processor User Manual (rev 1.14)  
> DDR0_14[REDUC] bit has the following meaning:
>
> REDUC - Enable the half data path feature of the controller
> 0 = Standard operation using full 64/72-bit memory bus
> 1 = Memory data path width is 32/40 bits
>
> However, "1" is decoded as 64 bit (8 byte), "0" - as 32 bit (4 byte) bus  
> by the kernel code. My understanding is this is not correct - patch  
> attached.

You are missing the Signed-off-by tag that is needed.

Aside from that, I'm curious if you found this just through inspection, or
if you actually had a problem because of it.  Your fix seems correct, yet
I've had no problems with my boards at all.

josh

Re: [PATCH] powerpc 4xx: DDR0_14[REDUC] decoded incorrectly

[Mikhail Zolotaryov]

>> However, "1" is decoded as 64 bit (8 byte), "0" - as 32 bit (4 byte) bus  
>> by the kernel code. My understanding is this is not correct - patch  
>> attached.
>
> You are missing the Signed-off-by tag that is needed.
>
I'm sorry.

Signed-off-by: Mikhail Zolotaryov <lebon@lebon.org.ua>

> Aside from that, I'm curious if you found this just through inspection, or
> if you actually had a problem because of it.  Your fix seems correct, yet
> I've had no problems with my boards at all.
>

I'm actually working on custom PowerPC 440EPX board development here and 
have found the problem: without applying changes described, Linux 
detects memory size incorrectly and is unable to start. The rest of 
memory size computation process seems to be correct.

I have checked Sequoia evaluation board with U-Boot 2009.1 and confirm 
that original kernel code reported memory size correctly. So, for my 
understanding, if computation algorithm was wrong but result was correct 
- the problem is source data i.e. initial DDR configuration parameters 
loaded by U-Boot (please don't give a damn that memory size is always 
reported correctly by U-Boot - it's hard-coded constant there). I have 
checked Sequoia board schematics (DES0211_11_SCH_11.pdf, page 5, unit 
U1D) and noted that BankSel#1 is not connected, while bootloader memory 
configuration is (board/amcc/sequoia/sdram.c):
        mtsdram(DDR0_10, 0x00000300);
i.e. both Chip Selects used.

If I change this line to:
        mtsdram(DDR0_10, 0x00000100);
memory is accessible, patched kernel detects memory size correctly and 
kernel seems to be working well.

Please check my considerations, possibly it may be necessary to request 
additional information from board manufacturer.

Re: [PATCH] powerpc 4xx: DDR0_14[REDUC] decoded incorrectly

[Mikhail Zolotaryov]

>> However, "1" is decoded as 64 bit (8 byte), "0" - as 32 bit (4 byte) bus  
>> by the kernel code. My understanding is this is not correct - patch  
>> attached.
>
> You are missing the Signed-off-by tag that is needed.
>
I'm sorry.

Signed-off-by: Mikhail Zolotaryov <lebon@lebon.org.ua>

> Aside from that, I'm curious if you found this just through inspection, or
> if you actually had a problem because of it.  Your fix seems correct, yet
> I've had no problems with my boards at all.
>

I'm actually working on custom PowerPC 440EPX board development here and 
have found the problem: without applying changes described, Linux 
detects memory size incorrectly and is unable to start. The rest of 
memory size computation process seems to be correct.

I have checked Sequoia evaluation board with U-Boot 2009.1 and confirm 
that original kernel code reported memory size correctly. So, for my 
understanding, if computation algorithm was wrong but result was correct 
- the problem is source data i.e. initial DDR configuration parameters 
loaded by U-Boot (please don't give a damn that memory size is always 
reported correctly by U-Boot - it's hard-coded constant there). I have 
checked Sequoia board schematics (DES0211_11_SCH_11.pdf, page 5, unit 
U1D) and noted that BankSel#1 is not connected, while bootloader memory 
configuration is (board/amcc/sequoia/sdram.c):
        mtsdram(DDR0_10, 0x00000300);
i.e. both Chip Selects used.

If I change this line to:
        mtsdram(DDR0_10, 0x00000100);
memory is accessible, patched kernel detects memory size correctly and 
kernel seems to be working well.

Please check my considerations, possibly it may be necessary to request 
additional information from board manufacturer.

[PATCH] PowerPC 440EPx/GRx fix memory size calculation

[Valentine Barshak]

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>


--- 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 */

Re: [PATCH] PowerPC 440EPx/GRx fix memory size calculation

[Mikhail Zolotaryov]

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.

Re: [PATCH] PowerPC 440EPx/GRx fix memory size calculation

[Valentine]

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.

Re: [PATCH] PowerPC 440EPx/GRx fix memory size calculation

[Benjamin Herrenschmidt]

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

Re: [PATCH] PowerPC 440EPx/GRx fix memory size calculation

[Mikhail Zolotaryov]

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.

Re: [PATCH] PowerPC 440EPx/GRx fix memory size calculation

[Mikhail Zolotaryov]

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

Re: [PATCH] PowerPC 440EPx/GRx fix memory size calculation

[Josh Boyer]

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

Re: [PATCH] PowerPC 440EPx/GRx fix memory size calculation

[Valentine Barshak]

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

Re: [PATCH] PowerPC 440EPx/GRx fix memory size calculation

[Josh Boyer]

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

Re: [PATCH] PowerPC 440EPx/GRx fix memory size calculation

[Valentine]

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.

Re: [PATCH] PowerPC 440EPx/GRx fix memory size calculation

[Josh Boyer]

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

Re: [PATCH] PowerPC 440EPx/GRx fix memory size calculation

[Stefan Roese]

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

Re: [PATCH] PowerPC 440EPx/GRx fix memory size calculation

[Benjamin Herrenschmidt]

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.

Re: [PATCH] PowerPC 440EPx/GRx fix memory size calculation

[Stefan Roese]

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

Re: [PATCH] PowerPC 440EPx/GRx fix memory size calculation

[Benjamin Herrenschmidt]

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.

Re: [PATCH] PowerPC 440EPx/GRx fix memory size calculation

[Stefan Roese]

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

Re: [PATCH] PowerPC 440EPx/GRx fix memory size calculation

[Mikhail Zolotaryov]

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.

Re: [PATCH] PowerPC 440EPx/GRx fix memory size calculation

[Josh Boyer]

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

Re: [PATCH] PowerPC 440EPx/GRx fix memory size calculation

[Stefan Roese]

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

Re: [PATCH] PowerPC 440EPx/GRx fix memory size calculation

[Feng Kan]

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