[PATCH 1/2] nand: nand_bbt: Export nand_update_bbt

[PATCH 1/2] nand: nand_bbt: Export nand_update_bbt

[Fabio Estevam]

From: Fabio Estevam <fabio.estevam@freescale.com>

When building MTD_NAND_GPMI_NAND as module, the following error shows up:

ERROR: "nand_update_bbt" [drivers/mtd/nand/gpmi-nand/gpmi_nand.ko] undefined!

Export nand_update_bbt to fix it.

Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
 drivers/mtd/nand/nand_bbt.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/mtd/nand/nand_bbt.c b/drivers/mtd/nand/nand_bbt.c
index 2d1d2fa..f4b6417 100644
--- a/drivers/mtd/nand/nand_bbt.c
+++ b/drivers/mtd/nand/nand_bbt.c
@@ -1403,3 +1403,4 @@ int nand_isbad_bbt(struct mtd_info *mtd, loff_t offs, int allowbbt)
 
 EXPORT_SYMBOL(nand_scan_bbt);
 EXPORT_SYMBOL(nand_default_bbt);
+EXPORT_SYMBOL(nand_update_bbt);
-- 
1.7.1

[PATCH 2/2] nand: Kconfig: Allow MTD_NAND_GPMI_NAND to be built as module

[Fabio Estevam]

From: Fabio Estevam <fabio.estevam@freescale.com>

Allow MTD_NAND_GPMI_NAND to be built as module.

Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
 drivers/mtd/nand/Kconfig |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
index f4e81a7..533eee7 100644
--- a/drivers/mtd/nand/Kconfig
+++ b/drivers/mtd/nand/Kconfig
@@ -461,7 +461,7 @@ config MTD_NAND_NANDSIM
 	  MTD nand layer.
 
 config MTD_NAND_GPMI_NAND
-        bool "GPMI NAND Flash Controller driver"
+        tristate "GPMI NAND Flash Controller driver"
         depends on MTD_NAND && (SOC_IMX23 || SOC_IMX28 || SOC_IMX6Q)
         help
 	 Enables NAND Flash support for IMX23 or IMX28.
-- 
1.7.1

[PATCH 1/2] nand: nand_bbt: Export nand_update_bbt

[Fabio Estevam]

From: Fabio Estevam <fabio.estevam@freescale.com>

When building MTD_NAND_GPMI_NAND as module, the following error shows up:

ERROR: "nand_update_bbt" [drivers/mtd/nand/gpmi-nand/gpmi_nand.ko] undefined!

Export nand_update_bbt to fix it.

Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
 drivers/mtd/nand/nand_bbt.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/mtd/nand/nand_bbt.c b/drivers/mtd/nand/nand_bbt.c
index 2d1d2fa..f4b6417 100644
--- a/drivers/mtd/nand/nand_bbt.c
+++ b/drivers/mtd/nand/nand_bbt.c
@@ -1403,3 +1403,4 @@ int nand_isbad_bbt(struct mtd_info *mtd, loff_t offs, int allowbbt)
 
 EXPORT_SYMBOL(nand_scan_bbt);
 EXPORT_SYMBOL(nand_default_bbt);
+EXPORT_SYMBOL(nand_update_bbt);
-- 
1.7.1

Re: [PATCH 1/2] nand: nand_bbt: Export nand_update_bbt

[Fabio Estevam]

Artem,

On Sat, Jul 28, 2012 at 7:30 PM, Fabio Estevam <festevam@gmail.com> wrote:
> From: Fabio Estevam <fabio.estevam@freescale.com>
>
> When building MTD_NAND_GPMI_NAND as module, the following error shows up:
>
> ERROR: "nand_update_bbt" [drivers/mtd/nand/gpmi-nand/gpmi_nand.ko] undefined!
>
> Export nand_update_bbt to fix it.
>
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>

Any comments on this series, please?

Re: [PATCH 1/2] nand: nand_bbt: Export nand_update_bbt

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 973 bytes --]

On Tue, 2012-08-14 at 19:50 -0300, Fabio Estevam wrote:
> Artem,
> 
> On Sat, Jul 28, 2012 at 7:30 PM, Fabio Estevam <festevam@gmail.com> wrote:
> > From: Fabio Estevam <fabio.estevam@freescale.com>
> >
> > When building MTD_NAND_GPMI_NAND as module, the following error shows up:
> >
> > ERROR: "nand_update_bbt" [drivers/mtd/nand/gpmi-nand/gpmi_nand.ko] undefined!
> >
> > Export nand_update_bbt to fix it.
> >
> > Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> 
> Any comments on this series, please?

If you need something to be merged upstream urgently, please, bug dwmw2
because I do not merge MTD stuff to Linus anyway. You guys can reach him
at the #mtd channel in OFTC faster than by e-mail.

Otherwise, I will come to this patch later. I have a lot of not
responded e-mails/patches in my mailbox, and I start looking at the
oldest first. So at some point I will reach this one as well.

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/2] nand: nand_bbt: Export nand_update_bbt

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 544 bytes --]

On Sat, 2012-07-28 at 19:29 -0300, Fabio Estevam wrote:
> From: Fabio Estevam <fabio.estevam@freescale.com>
> 
> When building MTD_NAND_GPMI_NAND as module, the following error shows up:
> 
> ERROR: "nand_update_bbt" [drivers/mtd/nand/gpmi-nand/gpmi_nand.ko] undefined!
> 
> Export nand_update_bbt to fix it.
> 
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>

Why this driver redefined ->block_markbad() at all, it is not supposed
to do this. We should fix the driver instead.

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/2] nand: nand_bbt: Export nand_update_bbt

[Huang Shijie]

On Thu, Aug 23, 2012 at 11:08 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> On Sat, 2012-07-28 at 19:29 -0300, Fabio Estevam wrote:
>> From: Fabio Estevam <fabio.estevam@freescale.com>
>>
>> When building MTD_NAND_GPMI_NAND as module, the following error shows up:
>>
>> ERROR: "nand_update_bbt" [drivers/mtd/nand/gpmi-nand/gpmi_nand.ko] undefined!
>>
>> Export nand_update_bbt to fix it
>>
>> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
>
> Why this driver redefined ->block_markbad() at all, it is not supposed
For hardware reason, in mx23, the bad block mark is stored in the
first byte of the nand page;
in mx28/mx50/mx6q, the bad block mark is stored in the first byte of the OOB.

That's why the driver redefined the ->block_markbad().

Is there any need to build the gpmi nand as a module?

Best Regards
Huang Shijie

> to do this. We should fix the driver instead.
>
> --
> Best Regards,
> Artem Bityutskiy
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>

Re: [PATCH 1/2] nand: nand_bbt: Export nand_update_bbt

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 3607 bytes --]

On Thu, 2012-08-23 at 11:36 -0400, Huang Shijie wrote:
> On Thu, Aug 23, 2012 at 11:08 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> > On Sat, 2012-07-28 at 19:29 -0300, Fabio Estevam wrote:
> >> From: Fabio Estevam <fabio.estevam@freescale.com>
> >>
> >> When building MTD_NAND_GPMI_NAND as module, the following error shows up:
> >>
> >> ERROR: "nand_update_bbt" [drivers/mtd/nand/gpmi-nand/gpmi_nand.ko] undefined!
> >>
> >> Export nand_update_bbt to fix it
> >>
> >> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> >
> > Why this driver redefined ->block_markbad() at all, it is not supposed
> For hardware reason, in mx23, the bad block mark is stored in the
> first byte of the nand page;
> in mx28/mx50/mx6q, the bad block mark is stored in the first byte of the OOB.
> 
> That's why the driver redefined the ->block_markbad().

The default function seem to do the same as your function does. You
select where you keep your OOB using chip options, and the default
function does the right thing, no?

From 5cdff78da6dde1e2eef472dcbbe7ca8f7fd64061 Mon Sep 17 00:00:00 2001
From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
Date: Fri, 24 Aug 2012 09:26:29 +0300
Subject: [PATCH] gpmi-nand: use the default implementation of block_markbad

Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
---
 drivers/mtd/nand/gpmi-nand/gpmi-nand.c |   46 --------------------------------
 1 file changed, 46 deletions(-)

diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
index 8c0d2f0..8e193fb 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
@@ -1179,51 +1179,6 @@ gpmi_ecc_write_oob(struct mtd_info *mtd, struct nand_chip *chip, int page)
 	return -EPERM;
 }
 
-static int gpmi_block_markbad(struct mtd_info *mtd, loff_t ofs)
-{
-	struct nand_chip *chip = mtd->priv;
-	struct gpmi_nand_data *this = chip->priv;
-	int block, ret = 0;
-	uint8_t *block_mark;
-	int column, page, status, chipnr;
-
-	/* Get block number */
-	block = (int)(ofs >> chip->bbt_erase_shift);
-	if (chip->bbt)
-		chip->bbt[block >> 2] |= 0x01 << ((block & 0x03) << 1);
-
-	/* Do we have a flash based bad block table ? */
-	if (chip->bbt_options & NAND_BBT_USE_FLASH)
-		ret = nand_update_bbt(mtd, ofs);
-	else {
-		chipnr = (int)(ofs >> chip->chip_shift);
-		chip->select_chip(mtd, chipnr);
-
-		column = this->swap_block_mark ? mtd->writesize : 0;
-
-		/* Write the block mark. */
-		block_mark = this->data_buffer_dma;
-		block_mark[0] = 0; /* bad block marker */
-
-		/* Shift to get page */
-		page = (int)(ofs >> chip->page_shift);
-
-		chip->cmdfunc(mtd, NAND_CMD_SEQIN, column, page);
-		chip->write_buf(mtd, block_mark, 1);
-		chip->cmdfunc(mtd, NAND_CMD_PAGEPROG, -1, -1);
-
-		status = chip->waitfunc(mtd, chip);
-		if (status & NAND_STATUS_FAIL)
-			ret = -EIO;
-
-		chip->select_chip(mtd, -1);
-	}
-	if (!ret)
-		mtd->ecc_stats.badblocks++;
-
-	return ret;
-}
-
 static int nand_boot_set_geometry(struct gpmi_nand_data *this)
 {
 	struct boot_rom_geometry *geometry = &this->rom_geometry;
@@ -1562,7 +1517,6 @@ static int __devinit gpmi_nfc_init(struct gpmi_nand_data *this)
 	chip->ecc.write_oob	= gpmi_ecc_write_oob;
 	chip->scan_bbt		= gpmi_scan_bbt;
 	chip->badblock_pattern	= &gpmi_bbt_descr;
-	chip->block_markbad	= gpmi_block_markbad;
 	chip->options		|= NAND_NO_SUBPAGE_WRITE;
 	chip->ecc.mode		= NAND_ECC_HW;
 	chip->ecc.size		= 1;
-- 
1.7.10.4

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/2] nand: nand_bbt: Export nand_update_bbt

[Huang Shijie]

于 2012年08月24日 14:41, Artem Bityutskiy 写道:
> The default function seem to do the same as your function does. You
> select where you keep your OOB using chip options, and the default
> function does the right thing, no?
The key issue is that the mx23 should copies the bad block mark into the 
"first byte of the nand page",
while the other chips does do this.

I don't think the default function do the same thing. You see:
   nand_default_block_markbad() --> nand_do_write_oob() --> 
chip->ecc.write_oob() -->gpmi_ecc_write_oob()

The gpmi_ecc_write_oob() does nothing,but return -EPERM.

Huang Shijie

Re: [PATCH 1/2] nand: nand_bbt: Export nand_update_bbt

[Huang Shijie]

On Fri, Aug 24, 2012 at 6:35 PM, Huang Shijie <b32955@freescale.com> wrote:
> 于 2012年08月24日 14:41, Artem Bityutskiy 写道:
>
>> The default function seem to do the same as your function does. You
>> select where you keep your OOB using chip options, and the default
>> function does the right thing, no?
>
> The key issue is that the mx23 should copies the bad block mark into the
> "first byte of the nand page",
> while the other chips does do this.

s/does/does not/

>
> I don't think the default function do the same thing. You see:
>   nand_default_block_markbad() --> nand_do_write_oob() -->
> chip->ecc.write_oob() -->gpmi_ecc_write_oob()
>
> The gpmi_ecc_write_oob() does nothing,but return -EPERM.
>
> Huang Shijie
>
>
>
>

Re: [PATCH 1/2] nand: nand_bbt: Export nand_update_bbt

[Brian Norris]

Hi,

>From Artem: "Why this driver redefined ->block_markbad() at all, it is
not supposed
to do this. We should fix the driver instead."

Why is ->block_markbad() a function pointer, then, if the driver is
not allowed to redefine it? Admittedly, it often doesn't make sense to
override it; plus, if you override ->block_markbad(), you probably
want to also override ->block_bad(). But nand_base.c doesn't really
have good infrastructure for that.

So, by the current, somewhat messy and incomplete state of the
nand_base + nand_bbt code, it seems to be "supported" to override
->block_markbad() if you really need to. But some more work could be
done to improve this.

On Fri, Aug 24, 2012 at 3:35 AM, Huang Shijie <b32955@freescale.com> wrote:
> 于 2012年08月24日 14:41, Artem Bityutskiy 写道:
>
>> The default function seem to do the same as your function does. You
>> select where you keep your OOB using chip options, and the default
>> function does the right thing, no?
>
> I don't think the default function do the same thing. You see:
>   nand_default_block_markbad() --> nand_do_write_oob() -->
> chip->ecc.write_oob() -->gpmi_ecc_write_oob()
>
> The gpmi_ecc_write_oob() does nothing,but return -EPERM.

Yeah, that's kind of strange. So you never have room in OOB even for
bad block markers? How do you identify factory-marked bad blocks?
Wouldn't they be indistinguishable from your ECC area? Or do you have
free OOB space in which you could actually implement write_oob()
properly?

None of my comments are really a disagreement with your patch. Your
driver has a strange way of dealing with ECC + bad block markers, and
assuming you figured out your swap_block_mark code safely (I didn't
really study this) then I think it's OK ("supported") to provide your
own ->block_markbad() function.

Brian

Re: [PATCH 1/2] nand: nand_bbt: Export nand_update_bbt

[Huang Shijie]

On Fri, Aug 24, 2012 at 6:57 PM, Brian Norris
<computersforpeace@gmail.com> wrote:

>>> The default function seem to do the same as your function does. You
>>> select where you keep your OOB using chip options, and the default
>>> function does the right thing, no?
>>
>> I don't think the default function do the same thing. You see:
>>   nand_default_block_markbad() --> nand_do_write_oob() -->
>> chip->ecc.write_oob() -->gpmi_ecc_write_oob()
>>
>> The gpmi_ecc_write_oob() does nothing,but return -EPERM.
>
> Yeah, that's kind of strange. So you never have room in OOB even fo

yes. The different ROMs of mx23/mx28/mx50/mx6q make it strange.

In mx28/mx50/mx6q, the first byte of OOB is used to store the bad
block marker. These chips's ROMs support the so-called `swap` feature which can
swaps the bad block marker to a safe place.

But the mx23's ROM does not support the swap feature, so it really has
no room in OOB to
store the bad block marker. When the mx23 first scan a nand chip, the
factory-marked
 bad block marker is copied to the first byte of the NAND page. When
the chip boots from
the nand, the ROM of mx23 will  read the first byte of the NAND page
to check whether this block is a bad block. Why copy to the first
byte? the mx23 use the first 10 byte of the nand page as a so-called
"metadata".

> bad block markers? How do you identify factory-marked bad blocks?
> Wouldn't they be indistinguishable from your ECC area? Or do you have
> free OOB space in which you could actually implement write_oob()
> properly?

The original old gpmi-nand driver does implement the write_oob() which
only allow the
block_markbad() run through. In another word, if the write_oob() is
called by the block_markbad, it will
grant the operation. All the other operations are denied. In order to
achieve this target, the old
code used an ugly hack code, it hooked the mtd->block_markbad(), such as:
         .....................................................
               mtd->hooked_block_markbad = mtd->block_markbad();
               mtd->block_markbad =              gpmi_block_markbad();

               nand->ecc.write_oob = mil_ecc_write_oob;
         .....................................................

             gpmi_block_markbad()
          {
               this->marking_a_block_bad = true;
              mtd->hooked_block_markbad();
              this->marking_a_block_bad = false;
           }

        mil_ecc_write_oob()
        {
            if (!this->marking_a_block_bad)
                    return;
            ..............................................
        }

     As i described above, the mil_ecc_write_oob() will write 0 to the
first byte of the OOB in mx28/mx50/mx6q;
the mil_ecc_write_oob() will write 0 to the first byte of the nand page in mx23.

For you see, the implementation of the write_oob() is too ugly. I
fininally found if i implement the nand->block_markbad(), the code
will very tidy and clean. The current code realizes the same feature
as
the old code, but the current code is very simple.

what's more is that the nand->block_markbad is `REPLACEABLE`. so i do
not break any rule. :)


I hope my poor english describes this issue clearly.

thanks
Huang Shijie

Re: [PATCH 1/2] nand: nand_bbt: Export nand_update_bbt

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 862 bytes --]

On Thu, 2012-08-23 at 11:36 -0400, Huang Shijie wrote:
> > Why this driver redefined ->block_markbad() at all, it is not supposed
> For hardware reason, in mx23, the bad block mark is stored in the
> first byte of the nand page;
> in mx28/mx50/mx6q, the bad block mark is stored in the first byte of the OOB.
> 
> That's why the driver redefined the ->block_markbad().

OK. Would you please tell about the driver some more, I am particularly
interested how th mx23 case works.

* So the bad block marker is the first byte of the eraseblock set to 0? 
* What if the user data stars with zero? Or you hide the first NAND page
  from the user?
* Can you point me to the code where you check if the eraseblock is bad
  or not at the initialization time (sorry, I could find myself,
  by trying to save time).

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/2] nand: nand_bbt: Export nand_update_bbt

[Huang Shijie]

于 2012年08月27日 23:25, Artem Bityutskiy 写道:
> On Thu, 2012-08-23 at 11:36 -0400, Huang Shijie wrote:
>>> Why this driver redefined ->block_markbad() at all, it is not supposed
>> For hardware reason, in mx23, the bad block mark is stored in the
>> first byte of the nand page;
>> in mx28/mx50/mx6q, the bad block mark is stored in the first byte of the OOB.
>>
>> That's why the driver redefined the ->block_markbad().
> OK. Would you please tell about the driver some more, I am particularly
> interested how th mx23 case works.
>
> * So the bad block marker is the first byte of the eraseblock set to 0?
yes.

> * What if the user data stars with zero? Or you hide the first NAND page
>    from the user?

Please see the picture in the common_nfc_set_geometry().
It's a real nand page layout.
The `M` is metadata, it's about 10 byte len.
The bad block marker is stored in the metadata, not the the
  `data` section which save the user's data.

We do not hide the first NAND page.

> * Can you point me to the code where you check if the eraseblock is bad
>    or not at the initialization time (sorry, I could find myself,
>    by trying to save time).
>
Please see the mx23_boot_init().

When mx23 reads a new nand chip in the first time, it will scan all
the nand chip. If it finds a bad block, it will call the 
nand->block_markbad() which
is just the gpmi_block_markbad(). the gpmi_block_markbad() will mark the 
first byte
of the nand page to 0 (the mx23 does not support the `swap` feature). So 
NAND boot mode,
the ROM of mx23 will check the first byte of the NAND page, if it finds 
a 0, it knows that
this is a bad block.

thanks
Huang Shijie

Re: [PATCH 1/2] nand: nand_bbt: Export nand_update_bbt

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 461 bytes --]

On Sat, 2012-07-28 at 19:29 -0300, Fabio Estevam wrote:
> From: Fabio Estevam <fabio.estevam@freescale.com>
> 
> When building MTD_NAND_GPMI_NAND as module, the following error shows up:
> 
> ERROR: "nand_update_bbt" [drivers/mtd/nand/gpmi-nand/gpmi_nand.ko] undefined!
> 
> Export nand_update_bbt to fix it.
> 
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>

Pushed both to l2-mtd.git, thanks!

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]