[U-Boot] RFC: porting u-boot to sequoia based nand booting board

[U-Boot] RFC: porting u-boot to sequoia based nand booting board

[Alex Waterman]

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

-- 
Alex Waterman
Computer Engineer
Phone: 215-896-4920
Email: awaterman at dawning.com

[U-Boot] RFC: porting u-boot to sequoia based nand booting board

[Stefan Roese]

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 at denx.de

[U-Boot] RFC: porting u-boot to sequoia based nand booting board

[Alex Waterman]

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 Waterman
Computer Engineer
Phone: 215-896-4920
Email: awaterman at dawning.com

[U-Boot] RFC: porting u-boot to sequoia based nand booting board

[Stefan Roese]

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 at denx.de

[U-Boot] RFC: porting u-boot to sequoia based nand booting board

[Scott Wood]

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

[U-Boot] RFC: porting u-boot to sequoia based nand booting board

[Alex Waterman]

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

-- 
Alex Waterman
Computer Engineer
Phone: 215-896-4920
Email: awaterman at dawning.com

[U-Boot] RFC: porting u-boot to sequoia based nand booting board

[Scott Wood]

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

[U-Boot] RFC: porting u-boot to sequoia based nand booting board

[Alex Waterman]

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


-- 
Alex Waterman
Computer Engineer
Phone: 215-896-4920
Email: awaterman at dawning.com

[U-Boot] RFC: porting u-boot to sequoia based nand booting board

[Scott Wood]

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

[U-Boot] RFC: porting u-boot to sequoia based nand booting board

[Alex Waterman]

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

-- 
Alex Waterman
Computer Engineer
Phone: 215-896-4920
Email: awaterman at dawning.com