Patchwork [U-Boot,4/9] imx: nand: Place BBT patterns into free OOB region

login
register
mail settings
Submitter Timo Ketola
Date April 13, 2012, 11:20 a.m.
Message ID <1334316061-26019-5-git-send-email-timo@exertus.fi>
Download mbox | patch
Permalink /patch/152283/
State Changes Requested
Delegated to: Stefano Babic
Headers show

Comments

Timo Ketola - April 13, 2012, 11:20 a.m.
First two bytes of the first OOB of erase block are reserved for factory
bad block marking, usually.

Signed-off-by: Timo Ketola <timo@exertus.fi>
---
 drivers/mtd/nand/mxc_nand.c |   35 +++++++++++++++++++++++++++++++++++
 1 files changed, 35 insertions(+), 0 deletions(-)
Scott Wood - April 13, 2012, 5:19 p.m.
On 04/13/2012 06:20 AM, Timo Ketola wrote:
> First two bytes of the first OOB of erase block are reserved for factory
> bad block marking, usually.
> 
> Signed-off-by: Timo Ketola <timo@exertus.fi>
> ---
>  drivers/mtd/nand/mxc_nand.c |   35 +++++++++++++++++++++++++++++++++++
>  1 files changed, 35 insertions(+), 0 deletions(-)

So what happened before?  The default is at offset 8, which doesn't
conflict with the bad block marker.  It seems the actual issue is a
conflict with ECC?

And NAND_USE_FLASH_BBT wasn't defined before, so a better subject line
for this patch would be "nand/mxc: support flash-based BBT".

> diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
> index 35e89a0..73813a2 100644
> --- a/drivers/mtd/nand/mxc_nand.c
> +++ b/drivers/mtd/nand/mxc_nand.c
> @@ -1302,12 +1302,47 @@ static void mxc_setup_config1(void)
>  #define mxc_setup_config1()
>  #endif
>  
> +#ifdef CONFIG_SYS_NAND_USE_FLASH_BBT
> +
> +static u8 bbt_pattern[] = {'B', 'b', 't', '0' };
> +static u8 mirror_pattern[] = {'1', 't', 'b', 'B' };
> +
> +static struct nand_bbt_descr bbt_main_descr = {
> +	.options = NAND_BBT_LASTBLOCK | NAND_BBT_CREATE | NAND_BBT_WRITE |
> +		   NAND_BBT_2BIT | NAND_BBT_VERSION | NAND_BBT_PERCHIP,
> +	.offs =	2,
> +	.len = 4,
> +	.veroffs = 6,
> +	.maxblocks = 4,
> +	.pattern = bbt_pattern,
> +};
> +
> +static struct nand_bbt_descr bbt_mirror_descr = {
> +	.options = NAND_BBT_LASTBLOCK | NAND_BBT_CREATE | NAND_BBT_WRITE |
> +		   NAND_BBT_2BIT | NAND_BBT_VERSION | NAND_BBT_PERCHIP,
> +	.offs =	2,
> +	.len = 4,
> +	.veroffs = 6,
> +	.maxblocks = 4,
> +	.pattern = mirror_pattern,
> +};
> +
> +#endif
> +

Won't veroffs = 6 conflict with ECC in the MXC_NFC_V1 case?

What about 8-bit small page support, in which case the bad block marker
is at offset 5?

>  int board_nand_init(struct nand_chip *this)
>  {
>  	struct mtd_info *mtd;
>  	uint16_t tmp;
>  	int err = 0;
>  
> +#ifdef CONFIG_SYS_NAND_USE_FLASH_BBT
> +
> +	this->options = NAND_USE_FLASH_BBT;
> +	this->bbt_td = &bbt_main_descr;
> +	this->bbt_md = &bbt_mirror_descr;
> +
> +#endif

Please remove those blank lines inside the ifdef.

-Scott
Timo Ketola - April 13, 2012, 6:12 p.m.
On 13.04.2012 20:19, Scott Wood wrote:
> On 04/13/2012 06:20 AM, Timo Ketola wrote:
>> First two bytes of the first OOB of erase block are reserved for factory
>> bad block marking, usually.
>>
>> Signed-off-by: Timo Ketola<timo@exertus.fi>
>> ---
>>   drivers/mtd/nand/mxc_nand.c |   35 +++++++++++++++++++++++++++++++++++
>>   1 files changed, 35 insertions(+), 0 deletions(-)
>
> So what happened before?  The default is at offset 8, which doesn't
> conflict with the bad block marker.  It seems the actual issue is a
> conflict with ECC?

You seem to be right. I think I was badly confused with the kernel behaviour.

> And NAND_USE_FLASH_BBT wasn't defined before, so a better subject line
> for this patch would be "nand/mxc: support flash-based BBT".

Most probably right too.

> Won't veroffs = 6 conflict with ECC in the MXC_NFC_V1 case?

Seems to.

> What about 8-bit small page support, in which case the bad block marker
> is at offset 5?

What about putting into the block

#if defined(MXC_NFC_V1)
#ifndef CONFIG_SYS_NAND_LARGEPAGE

defines for pattern and version offsets and use them in bbt_*_descr 
initializations? Or should they be in board configuration file?

>> +#ifdef CONFIG_SYS_NAND_USE_FLASH_BBT
>> +
>> +	this->options = NAND_USE_FLASH_BBT;
>> +	this->bbt_td =&bbt_main_descr;
>> +	this->bbt_md =&bbt_mirror_descr;
>> +
>> +#endif
>
> Please remove those blank lines inside the ifdef.

Ok

--

Timo
Scott Wood - April 13, 2012, 6:17 p.m.
On 04/13/2012 01:12 PM, Timo Ketola wrote:
> On 13.04.2012 20:19, Scott Wood wrote:
>> On 04/13/2012 06:20 AM, Timo Ketola wrote:
>>> First two bytes of the first OOB of erase block are reserved for factory
>>> bad block marking, usually.
>>>
>>> Signed-off-by: Timo Ketola<timo@exertus.fi>
>>> ---
>>>   drivers/mtd/nand/mxc_nand.c |   35 +++++++++++++++++++++++++++++++++++
>>>   1 files changed, 35 insertions(+), 0 deletions(-)
>>
>> So what happened before?  The default is at offset 8, which doesn't
>> conflict with the bad block marker.  It seems the actual issue is a
>> conflict with ECC?
> 
> You seem to be right. I think I was badly confused with the kernel
> behaviour.

It looks like Linux wants the BBT to be at offset zero.  Is there any
plan to fix that?  The two really should match...

>> What about 8-bit small page support, in which case the bad block marker
>> is at offset 5?
> 
> What about putting into the block
> 
> #if defined(MXC_NFC_V1)
> #ifndef CONFIG_SYS_NAND_LARGEPAGE
> 
> defines for pattern and version offsets and use them in bbt_*_descr
> initializations?

Sure.

> Or should they be in board configuration file?

I don't think it belongs in the board config file (unless there's
existing behavior that has to be matched for compatibility on a specific
board).

-Scott
Timo Ketola - April 13, 2012, 6:39 p.m.
On 13.04.2012 21:17, Scott Wood wrote:
> It looks like Linux wants the BBT to be at offset zero.

I have not dug too deeply into the BBT logic in kernel but maybe it could be 
possible to place BBT patterns over the factory markers. Then, when the code 
scans for BBT blocks, it should ignore factory markers and react only on BBT 
patterns. But I don't really know if this is the idea in kernel.

--

Timo
Timo Ketola - April 16, 2012, 6:41 a.m.
On 13.04.2012 21:17, Scott Wood wrote:
> It looks like Linux wants the BBT to be at offset zero.  Is there any
> plan to fix that?  The two really should match...

Somewhere in the process I got an impression that BBT couldn't be placed over 
the area where factory markers would be. But you made me think and I reverted 
the offset in my kernel back to zero and changed U-Boot accordingly. It seems 
to work perfectly like that. So I think now that the offset at zero is right.

Should I post v3 patchset now or only this one again or minimize noise and wait 
for more comments?

--

Timo
Scott Wood - April 16, 2012, 2:43 p.m.
On 04/16/2012 01:41 AM, Timo Ketola wrote:
> Should I post v3 patchset now or only this one again or minimize noise
> and wait for more comments?

Whichever you'd prefer.

-Scott

Patch

diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
index 35e89a0..73813a2 100644
--- a/drivers/mtd/nand/mxc_nand.c
+++ b/drivers/mtd/nand/mxc_nand.c
@@ -1302,12 +1302,47 @@  static void mxc_setup_config1(void)
 #define mxc_setup_config1()
 #endif
 
+#ifdef CONFIG_SYS_NAND_USE_FLASH_BBT
+
+static u8 bbt_pattern[] = {'B', 'b', 't', '0' };
+static u8 mirror_pattern[] = {'1', 't', 'b', 'B' };
+
+static struct nand_bbt_descr bbt_main_descr = {
+	.options = NAND_BBT_LASTBLOCK | NAND_BBT_CREATE | NAND_BBT_WRITE |
+		   NAND_BBT_2BIT | NAND_BBT_VERSION | NAND_BBT_PERCHIP,
+	.offs =	2,
+	.len = 4,
+	.veroffs = 6,
+	.maxblocks = 4,
+	.pattern = bbt_pattern,
+};
+
+static struct nand_bbt_descr bbt_mirror_descr = {
+	.options = NAND_BBT_LASTBLOCK | NAND_BBT_CREATE | NAND_BBT_WRITE |
+		   NAND_BBT_2BIT | NAND_BBT_VERSION | NAND_BBT_PERCHIP,
+	.offs =	2,
+	.len = 4,
+	.veroffs = 6,
+	.maxblocks = 4,
+	.pattern = mirror_pattern,
+};
+
+#endif
+
 int board_nand_init(struct nand_chip *this)
 {
 	struct mtd_info *mtd;
 	uint16_t tmp;
 	int err = 0;
 
+#ifdef CONFIG_SYS_NAND_USE_FLASH_BBT
+
+	this->options = NAND_USE_FLASH_BBT;
+	this->bbt_td = &bbt_main_descr;
+	this->bbt_md = &bbt_mirror_descr;
+
+#endif
+
 	/* structures must be linked */
 	mtd = &host->mtd;
 	mtd->priv = this;