| Message ID | 20090310195013.GA27835@ru.mvista.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Josh Boyer |
| Headers | show |
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.
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.
> 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.
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.
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) ?
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
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
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
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.
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
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
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.
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
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.
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
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.
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
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
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
>
>
--- 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 */