* [PATCH] powerpc 4xx: DDR0_14[REDUC] decoded incorrectly
@ 2009-03-09 16:21 ` Mikhail Zolotaryov
0 siblings, 0 replies; 26+ messages in thread
From: Mikhail Zolotaryov @ 2009-03-09 16:21 UTC (permalink / raw)
To: linux-kernel; +Cc: linuxppc-dev, jwboyer
[-- 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);
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH] powerpc 4xx: DDR0_14[REDUC] decoded incorrectly
@ 2009-03-09 16:21 ` Mikhail Zolotaryov
0 siblings, 0 replies; 26+ messages in thread
From: Mikhail Zolotaryov @ 2009-03-09 16:21 UTC (permalink / raw)
To: linux-kernel; +Cc: linuxppc-dev
[-- 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);
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] powerpc 4xx: DDR0_14[REDUC] decoded incorrectly
2009-03-09 16:21 ` Mikhail Zolotaryov
@ 2009-03-09 17:12 ` Josh Boyer
-1 siblings, 0 replies; 26+ messages in thread
From: Josh Boyer @ 2009-03-09 17:12 UTC (permalink / raw)
To: Mikhail Zolotaryov; +Cc: linux-kernel, linuxppc-dev
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] powerpc 4xx: DDR0_14[REDUC] decoded incorrectly
@ 2009-03-09 17:12 ` Josh Boyer
0 siblings, 0 replies; 26+ messages in thread
From: Josh Boyer @ 2009-03-09 17:12 UTC (permalink / raw)
To: Mikhail Zolotaryov; +Cc: linuxppc-dev, linux-kernel
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] powerpc 4xx: DDR0_14[REDUC] decoded incorrectly
2009-03-09 17:12 ` Josh Boyer
@ 2009-03-09 21:17 ` Mikhail Zolotaryov
-1 siblings, 0 replies; 26+ messages in thread
From: Mikhail Zolotaryov @ 2009-03-09 21:17 UTC (permalink / raw)
To: Josh Boyer; +Cc: linux-kernel, linuxppc-dev
>> 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.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] powerpc 4xx: DDR0_14[REDUC] decoded incorrectly
@ 2009-03-09 21:17 ` Mikhail Zolotaryov
0 siblings, 0 replies; 26+ messages in thread
From: Mikhail Zolotaryov @ 2009-03-09 21:17 UTC (permalink / raw)
To: Josh Boyer; +Cc: linuxppc-dev, linux-kernel
>> 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.
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH] PowerPC 440EPx/GRx fix memory size calculation
2009-03-09 21:17 ` Mikhail Zolotaryov
(?)
@ 2009-03-10 19:50 ` Valentine Barshak
2009-03-10 20:57 ` Mikhail Zolotaryov
2009-03-11 10:37 ` Josh Boyer
-1 siblings, 2 replies; 26+ messages in thread
From: Valentine Barshak @ 2009-03-10 19:50 UTC (permalink / raw)
To: linuxppc-dev; +Cc: lebon
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 */
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] PowerPC 440EPx/GRx fix memory size calculation
2009-03-10 19:50 ` [PATCH] PowerPC 440EPx/GRx fix memory size calculation Valentine Barshak
@ 2009-03-10 20:57 ` Mikhail Zolotaryov
2009-03-11 1:40 ` Valentine
2009-03-11 10:37 ` Josh Boyer
1 sibling, 1 reply; 26+ messages in thread
From: Mikhail Zolotaryov @ 2009-03-10 20:57 UTC (permalink / raw)
To: linuxppc-dev
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.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] PowerPC 440EPx/GRx fix memory size calculation
2009-03-10 20:57 ` Mikhail Zolotaryov
@ 2009-03-11 1:40 ` Valentine
2009-03-11 2:26 ` Benjamin Herrenschmidt
2009-03-11 8:29 ` Mikhail Zolotaryov
0 siblings, 2 replies; 26+ messages in thread
From: Valentine @ 2009-03-11 1:40 UTC (permalink / raw)
To: Mikhail Zolotaryov; +Cc: linuxppc-dev
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.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] PowerPC 440EPx/GRx fix memory size calculation
2009-03-11 1:40 ` Valentine
@ 2009-03-11 2:26 ` Benjamin Herrenschmidt
2009-03-11 8:24 ` Mikhail Zolotaryov
2009-03-11 8:29 ` Mikhail Zolotaryov
1 sibling, 1 reply; 26+ messages in thread
From: Benjamin Herrenschmidt @ 2009-03-11 2:26 UTC (permalink / raw)
To: Valentine; +Cc: linuxppc-dev, Mikhail Zolotaryov
> 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.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] PowerPC 440EPx/GRx fix memory size calculation
2009-03-11 2:26 ` Benjamin Herrenschmidt
@ 2009-03-11 8:24 ` Mikhail Zolotaryov
0 siblings, 0 replies; 26+ messages in thread
From: Mikhail Zolotaryov @ 2009-03-11 8:24 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: linuxppc-dev
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.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] PowerPC 440EPx/GRx fix memory size calculation
2009-03-11 1:40 ` Valentine
2009-03-11 2:26 ` Benjamin Herrenschmidt
@ 2009-03-11 8:29 ` Mikhail Zolotaryov
1 sibling, 0 replies; 26+ messages in thread
From: Mikhail Zolotaryov @ 2009-03-11 8:29 UTC (permalink / raw)
To: Valentine; +Cc: linuxppc-dev
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) ?
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] PowerPC 440EPx/GRx fix memory size calculation
2009-03-10 19:50 ` [PATCH] PowerPC 440EPx/GRx fix memory size calculation Valentine Barshak
2009-03-10 20:57 ` Mikhail Zolotaryov
@ 2009-03-11 10:37 ` Josh Boyer
2009-03-11 19:06 ` Valentine Barshak
1 sibling, 1 reply; 26+ messages in thread
From: Josh Boyer @ 2009-03-11 10:37 UTC (permalink / raw)
To: Valentine Barshak; +Cc: linuxppc-dev, lebon
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] PowerPC 440EPx/GRx fix memory size calculation
2009-03-11 10:37 ` Josh Boyer
@ 2009-03-11 19:06 ` Valentine Barshak
2009-03-11 21:57 ` Josh Boyer
2009-03-12 6:02 ` Stefan Roese
0 siblings, 2 replies; 26+ messages in thread
From: Valentine Barshak @ 2009-03-11 19:06 UTC (permalink / raw)
To: Josh Boyer; +Cc: linuxppc-dev, Stefan Roese, lebon
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] PowerPC 440EPx/GRx fix memory size calculation
2009-03-11 19:06 ` Valentine Barshak
@ 2009-03-11 21:57 ` Josh Boyer
2009-03-11 22:08 ` Valentine
2009-03-12 6:02 ` Stefan Roese
1 sibling, 1 reply; 26+ messages in thread
From: Josh Boyer @ 2009-03-11 21:57 UTC (permalink / raw)
To: Valentine Barshak; +Cc: linuxppc-dev, Stefan Roese, lebon
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] PowerPC 440EPx/GRx fix memory size calculation
2009-03-11 21:57 ` Josh Boyer
@ 2009-03-11 22:08 ` Valentine
2009-03-11 23:07 ` Josh Boyer
0 siblings, 1 reply; 26+ messages in thread
From: Valentine @ 2009-03-11 22:08 UTC (permalink / raw)
To: Josh Boyer; +Cc: linuxppc-dev, Stefan Roese, lebon
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.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] PowerPC 440EPx/GRx fix memory size calculation
2009-03-11 22:08 ` Valentine
@ 2009-03-11 23:07 ` Josh Boyer
0 siblings, 0 replies; 26+ messages in thread
From: Josh Boyer @ 2009-03-11 23:07 UTC (permalink / raw)
To: Valentine; +Cc: linuxppc-dev, Stefan Roese, lebon
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] PowerPC 440EPx/GRx fix memory size calculation
2009-03-11 19:06 ` Valentine Barshak
2009-03-11 21:57 ` Josh Boyer
@ 2009-03-12 6:02 ` Stefan Roese
2009-03-12 7:32 ` Benjamin Herrenschmidt
2009-03-13 23:01 ` Feng Kan
1 sibling, 2 replies; 26+ messages in thread
From: Stefan Roese @ 2009-03-12 6:02 UTC (permalink / raw)
To: linuxppc-dev; +Cc: lebon
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] PowerPC 440EPx/GRx fix memory size calculation
2009-03-12 6:02 ` Stefan Roese
@ 2009-03-12 7:32 ` Benjamin Herrenschmidt
2009-03-12 8:05 ` Stefan Roese
2009-03-13 23:01 ` Feng Kan
1 sibling, 1 reply; 26+ messages in thread
From: Benjamin Herrenschmidt @ 2009-03-12 7:32 UTC (permalink / raw)
To: Stefan Roese; +Cc: linuxppc-dev, lebon
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.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] PowerPC 440EPx/GRx fix memory size calculation
2009-03-12 7:32 ` Benjamin Herrenschmidt
@ 2009-03-12 8:05 ` Stefan Roese
2009-03-12 8:12 ` Benjamin Herrenschmidt
0 siblings, 1 reply; 26+ messages in thread
From: Stefan Roese @ 2009-03-12 8:05 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: linuxppc-dev, lebon
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] PowerPC 440EPx/GRx fix memory size calculation
2009-03-12 8:05 ` Stefan Roese
@ 2009-03-12 8:12 ` Benjamin Herrenschmidt
2009-03-12 8:24 ` Stefan Roese
0 siblings, 1 reply; 26+ messages in thread
From: Benjamin Herrenschmidt @ 2009-03-12 8:12 UTC (permalink / raw)
To: Stefan Roese; +Cc: linuxppc-dev, lebon
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.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] PowerPC 440EPx/GRx fix memory size calculation
2009-03-12 8:12 ` Benjamin Herrenschmidt
@ 2009-03-12 8:24 ` Stefan Roese
2009-03-12 8:45 ` Mikhail Zolotaryov
2009-03-12 10:45 ` Josh Boyer
0 siblings, 2 replies; 26+ messages in thread
From: Stefan Roese @ 2009-03-12 8:24 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: linuxppc-dev, lebon
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] PowerPC 440EPx/GRx fix memory size calculation
2009-03-12 8:24 ` Stefan Roese
@ 2009-03-12 8:45 ` Mikhail Zolotaryov
2009-03-12 10:45 ` Josh Boyer
1 sibling, 0 replies; 26+ messages in thread
From: Mikhail Zolotaryov @ 2009-03-12 8:45 UTC (permalink / raw)
To: Stefan Roese; +Cc: linuxppc-dev
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.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] PowerPC 440EPx/GRx fix memory size calculation
2009-03-12 8:24 ` Stefan Roese
2009-03-12 8:45 ` Mikhail Zolotaryov
@ 2009-03-12 10:45 ` Josh Boyer
2009-03-12 11:02 ` Stefan Roese
1 sibling, 1 reply; 26+ messages in thread
From: Josh Boyer @ 2009-03-12 10:45 UTC (permalink / raw)
To: Stefan Roese; +Cc: lebon, linuxppc-dev
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] PowerPC 440EPx/GRx fix memory size calculation
2009-03-12 10:45 ` Josh Boyer
@ 2009-03-12 11:02 ` Stefan Roese
0 siblings, 0 replies; 26+ messages in thread
From: Stefan Roese @ 2009-03-12 11:02 UTC (permalink / raw)
To: Josh Boyer; +Cc: lebon, linuxppc-dev
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] PowerPC 440EPx/GRx fix memory size calculation
2009-03-12 6:02 ` Stefan Roese
2009-03-12 7:32 ` Benjamin Herrenschmidt
@ 2009-03-13 23:01 ` Feng Kan
1 sibling, 0 replies; 26+ messages in thread
From: Feng Kan @ 2009-03-13 23:01 UTC (permalink / raw)
To: Stefan Roese; +Cc: linuxppc-dev, lebon
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
>
>
^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2009-03-13 23:12 UTC | newest]
Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-03-09 16:21 [PATCH] powerpc 4xx: DDR0_14[REDUC] decoded incorrectly Mikhail Zolotaryov
2009-03-09 16:21 ` Mikhail Zolotaryov
2009-03-09 17:12 ` Josh Boyer
2009-03-09 17:12 ` Josh Boyer
2009-03-09 21:17 ` Mikhail Zolotaryov
2009-03-09 21:17 ` Mikhail Zolotaryov
2009-03-10 19:50 ` [PATCH] PowerPC 440EPx/GRx fix memory size calculation Valentine Barshak
2009-03-10 20:57 ` Mikhail Zolotaryov
2009-03-11 1:40 ` Valentine
2009-03-11 2:26 ` Benjamin Herrenschmidt
2009-03-11 8:24 ` Mikhail Zolotaryov
2009-03-11 8:29 ` Mikhail Zolotaryov
2009-03-11 10:37 ` Josh Boyer
2009-03-11 19:06 ` Valentine Barshak
2009-03-11 21:57 ` Josh Boyer
2009-03-11 22:08 ` Valentine
2009-03-11 23:07 ` Josh Boyer
2009-03-12 6:02 ` Stefan Roese
2009-03-12 7:32 ` Benjamin Herrenschmidt
2009-03-12 8:05 ` Stefan Roese
2009-03-12 8:12 ` Benjamin Herrenschmidt
2009-03-12 8:24 ` Stefan Roese
2009-03-12 8:45 ` Mikhail Zolotaryov
2009-03-12 10:45 ` Josh Boyer
2009-03-12 11:02 ` Stefan Roese
2009-03-13 23:01 ` Feng Kan
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.