Re: [U-Boot] [PATCH 3/4 V2] OneNAND: Add simple OneNAND SPL

2011-11-03 Thread Marek Vasut
> On 11/03/2011 11:56 AM, Marek Vasut wrote:
> >> On 11/02/2011 07:15 PM, Marek Vasut wrote:
>  On 11/01/2011 05:54 PM, Marek Vasut wrote:
> > +static void spl_onenand_get_geometry(struct spl_onenand_data *data)
> > +{
> >>> 
> >>> [...]
> >>> 
> > +   /* The page can be either 2k or 4k, avoid using DIV_ROUND_UP. */
> > +   if (data.pagesize == 2048) {
> > +   total_pages = len / 2048;
> > +   page = offset / 2048;
> > +   total_pages += !!(len & 2047);
> > +   } else if (data.pagesize == 4096) {
> > +   total_pages = len / 4096;
> > +   page = offset / 4096;
> > +   total_pages += !!(len & 4095);
> > +   }
>  
>  What's wrong with DIV_ROUND_UP?  It should produce smaller code than
>  what you've done here...
> >>> 
> >>> It pulls in aeabi_*div* functions, which won't fit into block 0 of
> >>> Onenand.
> >> 
> >> It shouldn't do that if the divisor is a constant power of 2.  The
> >> compiler will turn it into a shift, just like with the other divides in
> >> the above code fragment.
> >> 
> >> You can't use DIV_ROUND_UP directly on data.pagesize, but you can use it
> >> in each branch of the if statement instead of that awkward and slightly
> >> more expensive !!(len & 4095) construct.
> > 
> > Expensive in what way?
> 
> Compare the resulting asm code.  You're replacing this:
> 
>   a = (b + 4095) >> 12;
> 
> with this:
> 
>   a = b >> 12;
>   if (b & 4095)
>   a++;
> 
> >  Either way, I don't think this matters that much.
> 
> This is code that has to fit in 1K -- why waste instructions by writing
> code in a way that is *less* readable?

The size is the same (tested). I'll submit a patch with DIV_ROUND_UP, whatever.

> 
> -Scott
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


Re: [U-Boot] [PATCH 3/4 V2] OneNAND: Add simple OneNAND SPL

2011-11-03 Thread Scott Wood
On 11/03/2011 11:56 AM, Marek Vasut wrote:
>> On 11/02/2011 07:15 PM, Marek Vasut wrote:
 On 11/01/2011 05:54 PM, Marek Vasut wrote:
> +static void spl_onenand_get_geometry(struct spl_onenand_data *data)
> +{
>>>
>>> [...]
>>>
> + /* The page can be either 2k or 4k, avoid using DIV_ROUND_UP. */
> + if (data.pagesize == 2048) {
> + total_pages = len / 2048;
> + page = offset / 2048;
> + total_pages += !!(len & 2047);
> + } else if (data.pagesize == 4096) {
> + total_pages = len / 4096;
> + page = offset / 4096;
> + total_pages += !!(len & 4095);
> + }

 What's wrong with DIV_ROUND_UP?  It should produce smaller code than
 what you've done here...
>>>
>>> It pulls in aeabi_*div* functions, which won't fit into block 0 of
>>> Onenand.
>>
>> It shouldn't do that if the divisor is a constant power of 2.  The
>> compiler will turn it into a shift, just like with the other divides in
>> the above code fragment.
>>
>> You can't use DIV_ROUND_UP directly on data.pagesize, but you can use it
>> in each branch of the if statement instead of that awkward and slightly
>> more expensive !!(len & 4095) construct.
> 
> Expensive in what way?

Compare the resulting asm code.  You're replacing this:

a = (b + 4095) >> 12;

with this:

a = b >> 12;
if (b & 4095)
a++;

>  Either way, I don't think this matters that much.

This is code that has to fit in 1K -- why waste instructions by writing
code in a way that is *less* readable?

-Scott

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


Re: [U-Boot] [PATCH 3/4 V2] OneNAND: Add simple OneNAND SPL

2011-11-03 Thread Marek Vasut
> On 11/02/2011 07:15 PM, Marek Vasut wrote:
> >> On 11/01/2011 05:54 PM, Marek Vasut wrote:
> >>> +static void spl_onenand_get_geometry(struct spl_onenand_data *data)
> >>> +{
> > 
> > [...]
> > 
> >>> + /* The page can be either 2k or 4k, avoid using DIV_ROUND_UP. */
> >>> + if (data.pagesize == 2048) {
> >>> + total_pages = len / 2048;
> >>> + page = offset / 2048;
> >>> + total_pages += !!(len & 2047);
> >>> + } else if (data.pagesize == 4096) {
> >>> + total_pages = len / 4096;
> >>> + page = offset / 4096;
> >>> + total_pages += !!(len & 4095);
> >>> + }
> >> 
> >> What's wrong with DIV_ROUND_UP?  It should produce smaller code than
> >> what you've done here...
> > 
> > It pulls in aeabi_*div* functions, which won't fit into block 0 of
> > Onenand.
> 
> It shouldn't do that if the divisor is a constant power of 2.  The
> compiler will turn it into a shift, just like with the other divides in
> the above code fragment.
> 
> You can't use DIV_ROUND_UP directly on data.pagesize, but you can use it
> in each branch of the if statement instead of that awkward and slightly
> more expensive !!(len & 4095) construct.

Expensive in what way? Either way, I don't think this matters that much.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


Re: [U-Boot] [PATCH 3/4 V2] OneNAND: Add simple OneNAND SPL

2011-11-03 Thread Scott Wood
On 11/02/2011 07:15 PM, Marek Vasut wrote:
>> On 11/01/2011 05:54 PM, Marek Vasut wrote:
>>> +static void spl_onenand_get_geometry(struct spl_onenand_data *data)
>>> +{
> [...]
> 
>>> +   /* The page can be either 2k or 4k, avoid using DIV_ROUND_UP. */
>>> +   if (data.pagesize == 2048) {
>>> +   total_pages = len / 2048;
>>> +   page = offset / 2048;
>>> +   total_pages += !!(len & 2047);
>>> +   } else if (data.pagesize == 4096) {
>>> +   total_pages = len / 4096;
>>> +   page = offset / 4096;
>>> +   total_pages += !!(len & 4095);
>>> +   }
>>
>> What's wrong with DIV_ROUND_UP?  It should produce smaller code than
>> what you've done here...
> 
> It pulls in aeabi_*div* functions, which won't fit into block 0 of Onenand.

It shouldn't do that if the divisor is a constant power of 2.  The
compiler will turn it into a shift, just like with the other divides in
the above code fragment.

You can't use DIV_ROUND_UP directly on data.pagesize, but you can use it
in each branch of the if statement instead of that awkward and slightly
more expensive !!(len & 4095) construct.

-Scott

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


Re: [U-Boot] [PATCH 3/4 V2] OneNAND: Add simple OneNAND SPL

2011-11-02 Thread Marek Vasut
> Hi all,
> 
> Marek, did you see the onenand_ipl/onenand_read.c at u-boot? It's already
> implemented and used for OneNAND boot.
> 
> How do you think migrate this file into generic SPL framework?

That's basically what I did, with a little polishing.
> 
> Thank you,
> Kyungmin Park
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


Re: [U-Boot] [PATCH 3/4 V2] OneNAND: Add simple OneNAND SPL

2011-11-02 Thread Kyungmin Park
Hi all,

Marek, did you see the onenand_ipl/onenand_read.c at u-boot? It's already
implemented and used for OneNAND boot.

How do you think migrate this file into generic SPL framework?

Thank you,
Kyungmin Park
 
-Original Message-
From: Marek Vasut [mailto:marek.va...@gmail.com] 
Sent: Thursday, November 03, 2011 9:16 AM
To: Scott Wood
Cc: u-boot@lists.denx.de; Albert ARIBAUD; Kyungmin Park
Subject: Re: [PATCH 3/4 V2] OneNAND: Add simple OneNAND SPL

> On 11/01/2011 05:54 PM, Marek Vasut wrote:
> > +static void spl_onenand_get_geometry(struct spl_onenand_data *data)
> > +{
[...]

> > +   /* The page can be either 2k or 4k, avoid using DIV_ROUND_UP. */
> > +   if (data.pagesize == 2048) {
> > +   total_pages = len / 2048;
> > +   page = offset / 2048;
> > +   total_pages += !!(len & 2047);
> > +   } else if (data.pagesize == 4096) {
> > +   total_pages = len / 4096;
> > +   page = offset / 4096;
> > +   total_pages += !!(len & 4095);
> > +   }
> 
> What's wrong with DIV_ROUND_UP?  It should produce smaller code than
> what you've done here...

It pulls in aeabi_*div* functions, which won't fit into block 0 of Onenand.

> 
> > +   for (; page <= total_pages; page++) {
> > +   block = page / ONENAND_PAGES_PER_BLOCK;
> > +   rpage = page & (ONENAND_PAGES_PER_BLOCK - 1);
> > +   ret = spl_onenand_read_page(block, rpage, addr,
data.pagesize);
> > +   if (ret) {
> > +   total_pages++;
> > +   err |= 1;
> > +   } else
> > +   addr += data.pagesize / 4;
> > +   }
> 
> As discussed, please retain the existing block-skipping semantics.  And
> if you do skip a block, that's not an error to be propagated upward (as
> opposed to something like an uncorrectable ECC error).
> 
> If one side of an if statement requires braces, both sides should have
> them.
> 
> > diff --git a/include/onenand_uboot.h b/include/onenand_uboot.h
> > index 92279d5..fcb50ff 100644
> > --- a/include/onenand_uboot.h
> > +++ b/include/onenand_uboot.h
> > @@ -16,6 +16,8 @@
> > 
> >  #include 
> > 
> > +#ifndefCONFIG_SPL_BUILD
> > +
> 
> Please use a space rather than a tab after #define, #ifndef, etc.
> 
> >  /* Forward declarations */
> >  struct mtd_info;
> >  struct mtd_oob_ops;
> > 
> > @@ -52,4 +54,10 @@ extern int flexonenand_set_boundary(struct mtd_info
> > *mtd, int die,
> > 
> >  extern void s3c64xx_onenand_init(struct mtd_info *);
> >  extern void s3c64xx_set_width_regs(struct onenand_chip *);
> > 
> > +#else
> > +
> > +int spl_onenand_load_image(uint32_t dst, uint32_t offset, uint32_t
len);
> > +
> > +#endif
> 
> Why does this need to be ifdeffed at all?  We normally don't ifdef
> header declarations.
> 
> -Scott

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


Re: [U-Boot] [PATCH 3/4 V2] OneNAND: Add simple OneNAND SPL

2011-11-02 Thread Marek Vasut
> On 11/01/2011 05:54 PM, Marek Vasut wrote:
> > +static void spl_onenand_get_geometry(struct spl_onenand_data *data)
> > +{
[...]

> > +   /* The page can be either 2k or 4k, avoid using DIV_ROUND_UP. */
> > +   if (data.pagesize == 2048) {
> > +   total_pages = len / 2048;
> > +   page = offset / 2048;
> > +   total_pages += !!(len & 2047);
> > +   } else if (data.pagesize == 4096) {
> > +   total_pages = len / 4096;
> > +   page = offset / 4096;
> > +   total_pages += !!(len & 4095);
> > +   }
> 
> What's wrong with DIV_ROUND_UP?  It should produce smaller code than
> what you've done here...

It pulls in aeabi_*div* functions, which won't fit into block 0 of Onenand.

> 
> > +   for (; page <= total_pages; page++) {
> > +   block = page / ONENAND_PAGES_PER_BLOCK;
> > +   rpage = page & (ONENAND_PAGES_PER_BLOCK - 1);
> > +   ret = spl_onenand_read_page(block, rpage, addr, data.pagesize);
> > +   if (ret) {
> > +   total_pages++;
> > +   err |= 1;
> > +   } else
> > +   addr += data.pagesize / 4;
> > +   }
> 
> As discussed, please retain the existing block-skipping semantics.  And
> if you do skip a block, that's not an error to be propagated upward (as
> opposed to something like an uncorrectable ECC error).
> 
> If one side of an if statement requires braces, both sides should have
> them.
> 
> > diff --git a/include/onenand_uboot.h b/include/onenand_uboot.h
> > index 92279d5..fcb50ff 100644
> > --- a/include/onenand_uboot.h
> > +++ b/include/onenand_uboot.h
> > @@ -16,6 +16,8 @@
> > 
> >  #include 
> > 
> > +#ifndefCONFIG_SPL_BUILD
> > +
> 
> Please use a space rather than a tab after #define, #ifndef, etc.
> 
> >  /* Forward declarations */
> >  struct mtd_info;
> >  struct mtd_oob_ops;
> > 
> > @@ -52,4 +54,10 @@ extern int flexonenand_set_boundary(struct mtd_info
> > *mtd, int die,
> > 
> >  extern void s3c64xx_onenand_init(struct mtd_info *);
> >  extern void s3c64xx_set_width_regs(struct onenand_chip *);
> > 
> > +#else
> > +
> > +int spl_onenand_load_image(uint32_t dst, uint32_t offset, uint32_t len);
> > +
> > +#endif
> 
> Why does this need to be ifdeffed at all?  We normally don't ifdef
> header declarations.
> 
> -Scott
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


Re: [U-Boot] [PATCH 3/4 V2] OneNAND: Add simple OneNAND SPL

2011-11-02 Thread Scott Wood
On 11/01/2011 05:54 PM, Marek Vasut wrote:
> +static void spl_onenand_get_geometry(struct spl_onenand_data *data)
> +{
> + uint32_t tmp;
> + uint32_t dev_id, density;
> +
> + /* Default geometry -- 2048b page, 128k erase block. */
> + data->pagesize = 2048;
> + data->erasesize = 0x2;
> +
> + tmp = onenand_readw(ONENAND_REG_TECHNOLOGY);
> + if (tmp)
> + goto dev_4k;
> +
> + dev_id = onenand_readw(ONENAND_REG_DEVICE_ID);
> + density = dev_id >> ONENAND_DEVICE_DENSITY_SHIFT;
> + density &= ONENAND_DEVICE_DENSITY_MASK;
> +
> + if (density < ONENAND_DEVICE_DENSITY_4Gb)
> + return;
> +
> + if (dev_id & ONENAND_DEVICE_IS_DDP)
> + return;
> +
> + /* 4k device geometry -- 4096b page, 256k erase block. */
> +dev_4k:
> + data->pagesize = 4096;
> + data->erasesize = 0x4;
> +}

Drop the goto and "tmp" variable, just do:

if (!onenand_readw(ONENAND_REG_TECHNOLOGY)) {
dev_id = onenand_readw(ONENAND_REG_DEVICE_ID);
density = dev_id >> ONENAND_DEVICE_DENSITY_SHIFT;
density &= ONENAND_DEVICE_DENSITY_MASK;

if (density < ONENAND_DEVICE_DENSITY_4Gb)
return;

if (dev_id & ONENAND_DEVICE_IS_DDP)
return;
}

> +int spl_onenand_load_image(uint32_t dst, uint32_t offset, uint32_t len)

Please use the same name and arguments as nand_spl_load_image() in
nand_spl_simple.c.

> +{
> + uint32_t *addr = (uint32_t *)dst;

Why not pass it in as a pointer in the first place?  I know U-Boot is
unlkely to support being built as 64-bit any time soon, but why
introduce gratuitous 64-bit-uncleanliness?

> + struct spl_onenand_data data;
> + uint32_t total_pages;
> + uint32_t block;
> + uint32_t page, rpage;
> + int ret, err = 0;
> +
> + spl_onenand_get_geometry(&data);
> +
> + /* The page can be either 2k or 4k, avoid using DIV_ROUND_UP. */
> + if (data.pagesize == 2048) {
> + total_pages = len / 2048;
> + page = offset / 2048;
> + total_pages += !!(len & 2047);
> + } else if (data.pagesize == 4096) {
> + total_pages = len / 4096;
> + page = offset / 4096;
> + total_pages += !!(len & 4095);
> + }

What's wrong with DIV_ROUND_UP?  It should produce smaller code than
what you've done here...

> + for (; page <= total_pages; page++) {
> + block = page / ONENAND_PAGES_PER_BLOCK;
> + rpage = page & (ONENAND_PAGES_PER_BLOCK - 1);
> + ret = spl_onenand_read_page(block, rpage, addr, data.pagesize);
> + if (ret) {
> + total_pages++;
> + err |= 1;
> + } else
> + addr += data.pagesize / 4;
> + }

As discussed, please retain the existing block-skipping semantics.  And
if you do skip a block, that's not an error to be propagated upward (as
opposed to something like an uncorrectable ECC error).

If one side of an if statement requires braces, both sides should have them.

> diff --git a/include/onenand_uboot.h b/include/onenand_uboot.h
> index 92279d5..fcb50ff 100644
> --- a/include/onenand_uboot.h
> +++ b/include/onenand_uboot.h
> @@ -16,6 +16,8 @@
>  
>  #include 
>  
> +#ifndef  CONFIG_SPL_BUILD
> +

Please use a space rather than a tab after #define, #ifndef, etc.

>  /* Forward declarations */
>  struct mtd_info;
>  struct mtd_oob_ops;
> @@ -52,4 +54,10 @@ extern int flexonenand_set_boundary(struct mtd_info *mtd, 
> int die,
>  extern void s3c64xx_onenand_init(struct mtd_info *);
>  extern void s3c64xx_set_width_regs(struct onenand_chip *);
>  
> +#else
> +
> +int spl_onenand_load_image(uint32_t dst, uint32_t offset, uint32_t len);
> +
> +#endif

Why does this need to be ifdeffed at all?  We normally don't ifdef
header declarations.

-Scott

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


[U-Boot] [PATCH 3/4 V2] OneNAND: Add simple OneNAND SPL

2011-11-01 Thread Marek Vasut
This introduces small OneNAND loader, fitting into 1kB of space (smallest
possible OneNAND RAM size). Some devices equipped with such crappy chips will
use this.

Signed-off-by: Marek Vasut 
Cc: Albert ARIBAUD 
Cc: Kyungmin Park 
Cc: Scott Wood 
---
 drivers/mtd/onenand/Makefile  |4 +
 drivers/mtd/onenand/onenand_spl.c |  151 +
 include/onenand_uboot.h   |8 ++
 spl/Makefile  |1 +
 4 files changed, 164 insertions(+), 0 deletions(-)
 create mode 100644 drivers/mtd/onenand/onenand_spl.c

V2: Introduce spl_onenand_load_image() to load data from OneNAND in SPL

diff --git a/drivers/mtd/onenand/Makefile b/drivers/mtd/onenand/Makefile
index b984bd4..b090d40 100644
--- a/drivers/mtd/onenand/Makefile
+++ b/drivers/mtd/onenand/Makefile
@@ -25,8 +25,12 @@ include $(TOPDIR)/config.mk
 
 LIB:= $(obj)libonenand.o
 
+ifndef CONFIG_SPL_BUILD
 COBJS-$(CONFIG_CMD_ONENAND):= onenand_uboot.o onenand_base.o onenand_bbt.o
 COBJS-$(CONFIG_SAMSUNG_ONENAND)+= samsung.o
+else
+COBJS-y:= onenand_spl.o
+endif
 
 COBJS  := $(COBJS-y)
 SRCS   := $(COBJS:.o=.c)
diff --git a/drivers/mtd/onenand/onenand_spl.c 
b/drivers/mtd/onenand/onenand_spl.c
new file mode 100644
index 000..d887d20
--- /dev/null
+++ b/drivers/mtd/onenand/onenand_spl.c
@@ -0,0 +1,151 @@
+/*
+ * Copyright (C) 2011 Marek Vasut 
+ *
+ * Based on code:
+ * Copyright (C) 2005-2009 Samsung Electronics
+ * Kyungmin Park 
+ *
+ * See file CREDITS for list of people who contributed to this
+ * project.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ * MA 02111-1307 USA
+ */
+
+#include 
+#include 
+#include 
+#include 
+
+struct spl_onenand_data {
+   uint32_tpagesize;
+   uint32_terasesize;
+};
+
+#define ONENAND_PAGES_PER_BLOCK64
+#define onenand_block_address(block)   (block)
+#define onenand_sector_address(page)   (page << 2)
+#define onenand_buffer_address()   ((1 << 3) << 8)
+#define onenand_bufferram_address(block)   (0)
+
+static inline uint16_t onenand_readw(uint32_t addr)
+{
+   return readw(CONFIG_SYS_ONENAND_BASE + addr);
+}
+
+static inline void onenand_writew(uint16_t value, uint32_t addr)
+{
+   writew(value, CONFIG_SYS_ONENAND_BASE + addr);
+}
+
+static void spl_onenand_get_geometry(struct spl_onenand_data *data)
+{
+   uint32_t tmp;
+   uint32_t dev_id, density;
+
+   /* Default geometry -- 2048b page, 128k erase block. */
+   data->pagesize = 2048;
+   data->erasesize = 0x2;
+
+   tmp = onenand_readw(ONENAND_REG_TECHNOLOGY);
+   if (tmp)
+   goto dev_4k;
+
+   dev_id = onenand_readw(ONENAND_REG_DEVICE_ID);
+   density = dev_id >> ONENAND_DEVICE_DENSITY_SHIFT;
+   density &= ONENAND_DEVICE_DENSITY_MASK;
+
+   if (density < ONENAND_DEVICE_DENSITY_4Gb)
+   return;
+
+   if (dev_id & ONENAND_DEVICE_IS_DDP)
+   return;
+
+   /* 4k device geometry -- 4096b page, 256k erase block. */
+dev_4k:
+   data->pagesize = 4096;
+   data->erasesize = 0x4;
+}
+
+static int spl_onenand_read_page(uint32_t block, uint32_t page,
+   uint32_t *buf, int pagesize)
+{
+   const uint32_t addr = CONFIG_SYS_ONENAND_BASE + ONENAND_DATARAM;
+   uint32_t offset;
+
+   onenand_writew(onenand_block_address(block),
+   ONENAND_REG_START_ADDRESS1);
+
+   onenand_writew(onenand_bufferram_address(block),
+   ONENAND_REG_START_ADDRESS2);
+
+   onenand_writew(onenand_sector_address(page),
+   ONENAND_REG_START_ADDRESS8);
+
+   onenand_writew(onenand_buffer_address(),
+   ONENAND_REG_START_BUFFER);
+
+   onenand_writew(ONENAND_INT_CLEAR, ONENAND_REG_INTERRUPT);
+
+   onenand_writew(ONENAND_CMD_READ, ONENAND_REG_COMMAND);
+
+   while (!(onenand_readw(ONENAND_REG_INTERRUPT) & ONENAND_INT_READ))
+   continue;
+
+   /* Check for invalid block mark */
+   if (page < 2 && (onenand_readw(ONENAND_SPARERAM) != 0x))
+   return 1;
+
+   for (offset = 0; offset < pagesize; offset += 4)
+   buf[offset / 4] = readl(addr + offset);
+
+   retur