[U-Boot] RFC: porting u-boot to sequoia based nand booting board
Dear List,
I am working on porting U-Boot to a sequoia based PPC440 board. It boots off NAND flash via the NDFC on the PPC440. Our NAND chip has a 16 bit bus which has presented some minor problems.
The NDFC code is pretty much what we need except for a few functions that I made some changes to. In particular the two functions that I changed are board_nand_init() and board_nand_select_device(). However, I would like to use the rest of the NDFC code. All of the functions in the NDFC code are declared static so I can't access them outside of ndfc.c; is there a reason for that? Does it save code space?
At the moment, in the source tree I have, I just copied ndfc.c to a different source file, made the changes I needed, and compiled that. But I don't like the idea of duplicating a significant amount of code. Any suggestions on how I should proceed with this?
Likewise much of our board specific support files are copies from the sequoia code. Would it be better to symlink to the ones that are not modified? Or is it OK to just copy the relevant source files into our own board directory and make minor adjustments as needed?
Regards, Alex
Hi Alex,
On Tuesday 17 May 2011 15:00:45 Alex Waterman wrote:
I am working on porting U-Boot to a sequoia based PPC440 board. It boots off NAND flash via the NDFC on the PPC440. Our NAND chip has a 16 bit bus which has presented some minor problems.
Yes, until now, all 4xx boards use 8bit NAND chips.
The NDFC code is pretty much what we need except for a few functions that I made some changes to. In particular the two functions that I changed are board_nand_init() and board_nand_select_device().
What changes did you have to make? Some 8/16 bit related changes? Or something else?
However, I would like to use the rest of the NDFC code.
It should be the goal, to use *only* the common NDFC code. If you have found some bugs, or you need some changes for 16bit devices, we should try to integrate them into the common code.
All of the functions in the NDFC code are declared static so I can't access them outside of ndfc.c; is there a reason for that? Does it save code space?
One idea behind the static declarations is to not pollute the namespace.
At the moment, in the source tree I have, I just copied ndfc.c to a different source file, made the changes I needed, and compiled that.
Ughhh!
But I don't like the idea of duplicating a significant amount of code. Any suggestions on how I should proceed with this?
Yes, please see above. Please explain what changes you need exactly and we will see, if we can integrate them into the common driver.
Likewise much of our board specific support files are copies from the sequoia code. Would it be better to symlink to the ones that are not modified? Or is it OK to just copy the relevant source files into our own board directory and make minor adjustments as needed?
Usually we copy those files into a new board directly. When the similarities are too big, then we should probably think about splitting to common parts into some other directory (arch/powerpc/cpu/ppc4xx/ppc440epx.c ???).
Best regards, Stefan
-- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-0 Fax: (+49)-8142-66989-80 Email: office@denx.de
Stefan,
On 05/17/2011 09:41 AM, Stefan Roese wrote:
What changes did you have to make? Some 8/16 bit related changes? Or something else?
I changed the hard coded value of EBC0_CFG; I forced the chip->options field to have the 16 bit bus width flag set; in board_nand_select_device() I modified the value written to the NDFC_CCR register; I think that's primarily it. The hard coded registers could be made into CONFIG_SYS defines I think, but the bus width setting... Could we make a CONFIG_SYS define that specifies the width of the bus since during the spl we don't have access to the nand chip table?
It should be the goal, to use *only* the common NDFC code. If you have found some bugs, or you need some changes for 16bit devices, we should try to integrate them into the common code.
I have seen issues with the nand_read_byte16() function in nand_base.c; it seems like the cpu_to_le16() should be the other way around: le16_to_cpu(). Other than that no bugs as far as I am aware.
One idea behind the static declarations is to not pollute the namespace.
Ahh, understood.
Likewise much of our board specific support files are copies from the sequoia code. Would it be better to symlink to the ones that are not modified? Or is it OK to just copy the relevant source files into our own board directory and make minor adjustments as needed?
Usually we copy those files into a new board directly. When the similarities are too big, then we should probably think about splitting to common parts into some other directory (arch/powerpc/cpu/ppc4xx/ppc440epx.c ???).
OK, I see. Most of the code in board/amcc/sequoia is the same except for some of the sdram initialization and the sequoia.c file (which I renamed to tiger.c). Also the linker script differs slightly. How would we go about making those board files common (init.S, chip_config.c)?
Regards, Alex
Alex,
On Tuesday 17 May 2011 16:11:14 Alex Waterman wrote:
What changes did you have to make? Some 8/16 bit related changes? Or something else?
I changed the hard coded value of EBC0_CFG; I forced the chip->options field to have the 16 bit bus width flag set; in board_nand_select_device() I modified the value written to the NDFC_CCR register; I think that's primarily it. The hard coded registers could be made into CONFIG_SYS defines I think, but the bus width setting... Could we make a CONFIG_SYS define that specifies the width of the bus since during the spl we don't have access to the nand chip table?
I see no problem with such an approach. How about CONFIG_SYS_NDFC_16BIT? If its not defined, then the currently used 8bit configuration is shall be selected.
It should be the goal, to use *only* the common NDFC code. If you have found some bugs, or you need some changes for 16bit devices, we should try to integrate them into the common code.
I have seen issues with the nand_read_byte16() function in nand_base.c; it seems like the cpu_to_le16() should be the other way around: le16_to_cpu(). Other than that no bugs as far as I am aware.
Hmmm. Not sure here. Perhaps this is PPC4xx specific. A better solution with less impact on other platforms using nand_base.c would be to implement a nand_read_byte() function in ndfc.c when CONFIG_SYS_NDFC_16BIT is defined. This way the default implementation from nand_base will be overridden.
Likewise much of our board specific support files are copies from the sequoia code. Would it be better to symlink to the ones that are not modified? Or is it OK to just copy the relevant source files into our own board directory and make minor adjustments as needed?
Usually we copy those files into a new board directly. When the similarities are too big, then we should probably think about splitting to common parts into some other directory (arch/powerpc/cpu/ppc4xx/ppc440epx.c ???).
OK, I see. Most of the code in board/amcc/sequoia is the same except for some of the sdram initialization and the sequoia.c file (which I renamed to tiger.c). Also the linker script differs slightly. How would we go about making those board files common (init.S, chip_config.c)?
Hard to tell without being able to look at your code and the differences. I suggest that you first create a patch with the NAND changes mentioned above and with your new board support (tiger) in a separate directory. We can discuss the details then on the list: if and how the sequoia code could/should be re-used.
Best regards, Stefan
-- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-0 Fax: (+49)-8142-66989-80 Email: office@denx.de
On Tue, 17 May 2011 10:11:14 -0400 Alex Waterman awaterman@dawning.com wrote:
I have seen issues with the nand_read_byte16() function in nand_base.c; it seems like the cpu_to_le16() should be the other way around: le16_to_cpu(). Other than that no bugs as far as I am aware.
What is the specific problem you're seeing? The use of these endian macros is a bit abusive and ugly (what's really wanted is native-endian I/O accessors -- readw() has an implicit le16_to_cpu()), and should have been done internally to the read_word() implementation rather than made part of the API, but functionally it should be correct.
-Scott
Scott,
On 05/17/2011 01:05 PM, Scott Wood wrote:
On Tue, 17 May 2011 10:11:14 -0400 Alex Waterman awaterman@dawning.com wrote:
I have seen issues with the nand_read_byte16() function in nand_base.c; it seems like the cpu_to_le16() should be the other way around: le16_to_cpu(). Other than that no bugs as far as I am aware.
What is the specific problem you're seeing? The use of these endian macros is a bit abusive and ugly (what's really wanted is native-endian I/O accessors -- readw() has an implicit le16_to_cpu()), and should have been done internally to the read_word() implementation rather than made part of the API, but functionally it should be correct.
When I was getting our NAND to work, it seemed like that function was always returning 0. I fixed it by writing a read_byte() function like this:
/* * Read a byte from the NDFC. */ static uint8_t tiger_read_byte(struct mtd_info *mtd){
uint16_t word; struct nand_chip *chip = mtd->priv;
word = readw(chip->IO_ADDR_R);
return (uint8_t) word;
}
It looked to me like the readw() function was returning the data in the correct CPU endianness (at least for PPC) and that the cpu_to_le16() was swapping the bytes such that the cast down to a uint8_t was getting the unset high order byte from the 16 bit read.
Regards, Alex
On Tue, 17 May 2011 13:49:44 -0400 Alex Waterman awaterman@dawning.com wrote:
Scott,
On 05/17/2011 01:05 PM, Scott Wood wrote:
On Tue, 17 May 2011 10:11:14 -0400 Alex Waterman awaterman@dawning.com wrote:
I have seen issues with the nand_read_byte16() function in nand_base.c; it seems like the cpu_to_le16() should be the other way around: le16_to_cpu(). Other than that no bugs as far as I am aware.
What is the specific problem you're seeing? The use of these endian macros is a bit abusive and ugly (what's really wanted is native-endian I/O accessors -- readw() has an implicit le16_to_cpu()), and should have been done internally to the read_word() implementation rather than made part of the API, but functionally it should be correct.
When I was getting our NAND to work, it seemed like that function was always returning 0. I fixed it by writing a read_byte() function like this:
/*
- Read a byte from the NDFC.
*/ static uint8_t tiger_read_byte(struct mtd_info *mtd){
uint16_t word; struct nand_chip *chip = mtd->priv;
word = readw(chip->IO_ADDR_R);
return (uint8_t) word;
}
It looked to me like the readw() function was returning the data in the correct CPU endianness (at least for PPC) and that the cpu_to_le16() was swapping the bytes such that the cast down to a uint8_t was getting the unset high order byte from the 16 bit read.
If readw() is returning the bytes in the correct endianness, then that means the register is little-endian.
It's not clear to me what the default assumption in nand_base.c is, though. read_byte16() suggests the default is native endian, but read_buf() and read_word() suggest it's little endian. :-(
Is there any currently working host-big-endian platform with 16-bit NAND that doesn't override these functions?
BTW, as for read_word(), looking at its only user, I think nand_block_bad() should be checking a 16-bit bad block marker on 16-bit NAND, rather than the low byte. Why is there a separate mechanism for checking bad block markers than the one in nand_bbt.c (not the bbt itself, but the code used to read the markers to create the bbt)? As long as a BBT is used, I don't think read_word() will ever matter.
-Scott
Scott,
If readw() is returning the bytes in the correct endianness, then that means the register is little-endian.
It's not clear to me what the default assumption in nand_base.c is, though. read_byte16() suggests the default is native endian, but read_buf() and read_word() suggest it's little endian. :-(
I am not sure either; the nand_read_byte16() function confuses me quite a bit. I really have no idea whether what I have done is wrong and an evil hack or if I found a typo or something.
Is there any currently working host-big-endian platform with 16-bit NAND that doesn't override these functions?
I really can't say. I haven't looked through the source of any of the other boards. As Stefan said, all previous 4xx based boards use 8 bit NAND so the possibility that there might be a problem with the 16 bit code on 4xx based boards doesn't surprise me.
BTW, as for read_word(), looking at its only user, I think nand_block_bad() should be checking a 16-bit bad block marker on 16-bit NAND, rather than the low byte. Why is there a separate mechanism for checking bad block markers than the one in nand_bbt.c (not the bbt itself, but the code used to read the markers to create the bbt)? As long as a BBT is used, I don't think read_word() will ever matter.
I think it may have something to do with the NAND SPL loader. Since the NDFC code is being used by the SPL, it has to be small. Pulling in the all of the nand_bbt.c code for just the code that checks if the block is actually bad may (probably) have been to much for the SPL. Thats my guess at least. If the 16 bit chips used 0xffff as a bad block marker then yeah, I suppose the endianness wouldn't matter. Is that something that we can do? Just choose to use 0xffff? Will that not potentially mess up JFFS support or what not from the Linux kernel that gets booted?
Regards, Alex
On Tue, 17 May 2011 17:15:50 -0400 Alex Waterman awaterman@dawning.com wrote:
Is there any currently working host-big-endian platform with 16-bit NAND that doesn't override these functions?
I really can't say. I haven't looked through the source of any of the other boards. As Stefan said, all previous 4xx based boards use 8 bit NAND so the possibility that there might be a problem with the 16 bit code on 4xx based boards doesn't surprise me.
I think the issue is broader than 4xx.
BTW, as for read_word(), looking at its only user, I think nand_block_bad() should be checking a 16-bit bad block marker on 16-bit NAND, rather than the low byte. Why is there a separate mechanism for checking bad block markers than the one in nand_bbt.c (not the bbt itself, but the code used to read the markers to create the bbt)? As long as a BBT is used, I don't think read_word() will ever matter.
I think it may have something to do with the NAND SPL loader. Since the NDFC code is being used by the SPL, it has to be small.
Nothing in nand_base.c is used by SPL. SPL has its own code for this, which currently just does a readb() (broken on 16-bit?).
Pulling in the all of the nand_bbt.c code for just the code that checks if the block is actually bad may (probably) have been to much for the SPL. Thats my guess at least. If the 16 bit chips used 0xffff as a bad block marker then yeah, I suppose the endianness wouldn't matter. Is that something that we can do? Just choose to use 0xffff? Will that not potentially mess up JFFS support or what not from the Linux kernel that gets booted?
It's not really our choice, it's what the manufacturer uses (unless you want to get into rewriting the markers before first use...). The one chip datasheet I looked at claimed the bad block marker was any value other than 0xffff on 16-bit, so checking just one of the bytes would be wrong.
-Scott
Scott,
Nothing in nand_base.c is used by SPL. SPL has its own code for this, which currently just does a readb() (broken on 16-bit?).
Oh you were talking about the bad block function in nand_base.c... That makes more sense now :). And yeah, I suppose the spl bad block check is broken. If it does not check the full 16 bits of data then some bad blocks may be incorrectly read as good.
It's not really our choice, it's what the manufacturer uses (unless you want to get into rewriting the markers before first use...). The one chip datasheet I looked at claimed the bad block marker was any value other than 0xffff on 16-bit, so checking just one of the bytes would be wrong.
My NAND data sheet says that the bad block mark is 0x000 for x16. However it says a little before that one should check for any non 0xffff value in the bad block marker. So it would seem that 16 bit devices should do a 16 bit check but under normal conditions an 8bit check would probably work...
I looked at the nand_block_bad() function in nand_base.c and it does the same cpu_to_le16() stuff that nand_read_byte16() does. I wonder if there is something to that? It seems to me that if its doing an 8 bit check for 0x0000 or 0xffff then it doesn't matter the endianness at all. Maybe that code is trying to make up for other code incorrectly writing only a single byte for the bad block marker?
Regards, Alex
participants (3)
-
Alex Waterman -
Scott Wood -
Stefan Roese