[PATCH] mtd: rawnand: denali: add DT property to specify skipped bytes in OOB

[PATCH] mtd: rawnand: denali: add DT property to specify skipped bytes in OOB

[Masahiro Yamada]

NAND devices need additional data area (OOB) for error correction,
but it is also used for Bad Block Marker (BBM).  In many cases, the
first byte in OOB is used for BBM, but the location actually depends
on chip vendors.  The NAND controller should preserve the precious
BBM to keep track of bad blocks.

In Denali IP, the SPARE_AREA_SKIP_BYTES register is used to specify
the number of bytes to skip from the start of OOB.  The ECC engine
will automatically skip the specified number of bytes when it gets
access to OOB area.

The same value for SPARE_AREA_SKIP_BYTES should be used between
firmware and the operating system if you intend to use the NAND
device across the control hand-off.

In fact, the current denali.c code expects firmware to have already
set the SPARE_AREA_SKIP_BYTES register, then reads the value out.

If no firmware (or bootloader) has initialized the controller, the
register value is zero, which is the default after power-on-reset.

In other words, the Linux driver cannot initialize the controller
by itself.  You cannot support the reset control either because
resetting the controller will get register values lost.

This commit adds a way to specify it via DT.  If the property
"denali,oob-skip-bytes" exists, the value will be set to the register.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

I thought this feature was common enough, but I could not find
any relevant property.

I added "denali," vendor prefix.  If you have a better property name
(or a better way to specify the value), please suggest it.


 Documentation/devicetree/bindings/mtd/denali-nand.txt |  3 +++
 drivers/mtd/nand/raw/denali.c                         | 14 +++++++++-----
 drivers/mtd/nand/raw/denali_dt.c                      |  6 ++++++
 3 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/mtd/denali-nand.txt b/Documentation/devicetree/bindings/mtd/denali-nand.txt
index f33da87..453faca 100644
--- a/Documentation/devicetree/bindings/mtd/denali-nand.txt
+++ b/Documentation/devicetree/bindings/mtd/denali-nand.txt
@@ -22,6 +22,9 @@ Optional properties:
       8, 16, 24  for "socionext,uniphier-denali-nand-v5a"
       8, 16      for "socionext,uniphier-denali-nand-v5b"
   - nand-ecc-maximize: see nand.txt for details
+  - denali,oob-skip-bytes: number of bytes to reserve from the start of OOB.
+    The reserved bytes should not be used for ECC or any other purpose.
+    It is generally intended to preserve bad block markers.
 
 The device tree may optionally contain sub-nodes describing partitions of the
 address space. See partition.txt for more detail.
diff --git a/drivers/mtd/nand/raw/denali.c b/drivers/mtd/nand/raw/denali.c
index d1ae968..3ef3f0d 100644
--- a/drivers/mtd/nand/raw/denali.c
+++ b/drivers/mtd/nand/raw/denali.c
@@ -1050,12 +1050,16 @@ static void denali_hw_init(struct denali_nand_info *denali)
 		denali->revision = swab16(ioread32(denali->reg + REVISION));
 
 	/*
-	 * tell driver how many bit controller will skip before
-	 * writing ECC code in OOB, this register may be already
-	 * set by firmware. So we read this value out.
-	 * if this value is 0, just let it be.
+	 * If oob_skip_bytes is specified (e.g. by DT property), set it to the
+	 * reigster. Otherwise, read the value out that may be set by firmware.
 	 */
-	denali->oob_skip_bytes = ioread32(denali->reg + SPARE_AREA_SKIP_BYTES);
+	if (denali->oob_skip_bytes)
+		iowrite32(denali->oob_skip_bytes,
+			  denali->reg + SPARE_AREA_SKIP_BYTES);
+	else
+		denali->oob_skip_bytes = ioread32(denali->reg +
+						  SPARE_AREA_SKIP_BYTES);
+
 	denali_detect_max_banks(denali);
 	iowrite32(0x0F, denali->reg + RB_PIN_ENABLED);
 	iowrite32(CHIP_EN_DONT_CARE__FLAG, denali->reg + CHIP_ENABLE_DONT_CARE);
diff --git a/drivers/mtd/nand/raw/denali_dt.c b/drivers/mtd/nand/raw/denali_dt.c
index 7c6a8a4..bc93675 100644
--- a/drivers/mtd/nand/raw/denali_dt.c
+++ b/drivers/mtd/nand/raw/denali_dt.c
@@ -78,6 +78,7 @@ static int denali_dt_probe(struct platform_device *pdev)
 	struct denali_dt *dt;
 	const struct denali_dt_data *data;
 	struct denali_nand_info *denali;
+	u32 oob_skip;
 	int ret;
 
 	dt = devm_kzalloc(dev, sizeof(*dt), GFP_KERNEL);
@@ -155,6 +156,11 @@ static int denali_dt_probe(struct platform_device *pdev)
 		denali->clk_x_rate = 200000000;
 	}
 
+	ret = of_property_read_u32(dev->of_node, "denali,oob-skip-bytes",
+				   &oob_skip);
+	if (!ret)
+		denali->oob_skip_bytes = oob_skip;
+
 	ret = denali_init(denali);
 	if (ret)
 		goto out_disable_clk_ecc;
-- 
2.7.4

Re: [PATCH] mtd: rawnand: denali: add DT property to specify skipped bytes in OOB

[Boris Brezillon]

Hi Masahiro,

On Fri,  7 Sep 2018 19:56:23 +0900
Masahiro Yamada <yamada.masahiro@socionext.com> wrote:

> NAND devices need additional data area (OOB) for error correction,
> but it is also used for Bad Block Marker (BBM).  In many cases, the
> first byte in OOB is used for BBM, but the location actually depends
> on chip vendors.  The NAND controller should preserve the precious
> BBM to keep track of bad blocks.
> 
> In Denali IP, the SPARE_AREA_SKIP_BYTES register is used to specify
> the number of bytes to skip from the start of OOB.  The ECC engine
> will automatically skip the specified number of bytes when it gets
> access to OOB area.
> 
> The same value for SPARE_AREA_SKIP_BYTES should be used between
> firmware and the operating system if you intend to use the NAND
> device across the control hand-off.
> 
> In fact, the current denali.c code expects firmware to have already
> set the SPARE_AREA_SKIP_BYTES register, then reads the value out.
> 
> If no firmware (or bootloader) has initialized the controller, the
> register value is zero, which is the default after power-on-reset.
> 
> In other words, the Linux driver cannot initialize the controller
> by itself.  You cannot support the reset control either because
> resetting the controller will get register values lost.
> 
> This commit adds a way to specify it via DT.  If the property
> "denali,oob-skip-bytes" exists, the value will be set to the register.

Hm, do we really need to make this config customizable? I mean, either
you have a large-page NAND (page > 512 bytes) and the 2 first bytes
must be reserved for the BBM or you have a small-page NAND and the BBM
is at position 4 and 5. Are you sure people configure that differently?
Don't you always have SPARE_AREA_SKIP_BYTES set to 6 or 2?

Regards,

Boris

Re: [PATCH] mtd: rawnand: denali: add DT property to specify skipped bytes in OOB

[Masahiro Yamada]

Hi Boris,

2018-09-07 23:08 GMT+09:00 Boris Brezillon <boris.brezillon@bootlin.com>:
> Hi Masahiro,
>
> On Fri,  7 Sep 2018 19:56:23 +0900
> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>
>> NAND devices need additional data area (OOB) for error correction,
>> but it is also used for Bad Block Marker (BBM).  In many cases, the
>> first byte in OOB is used for BBM, but the location actually depends
>> on chip vendors.  The NAND controller should preserve the precious
>> BBM to keep track of bad blocks.
>>
>> In Denali IP, the SPARE_AREA_SKIP_BYTES register is used to specify
>> the number of bytes to skip from the start of OOB.  The ECC engine
>> will automatically skip the specified number of bytes when it gets
>> access to OOB area.
>>
>> The same value for SPARE_AREA_SKIP_BYTES should be used between
>> firmware and the operating system if you intend to use the NAND
>> device across the control hand-off.
>>
>> In fact, the current denali.c code expects firmware to have already
>> set the SPARE_AREA_SKIP_BYTES register, then reads the value out.
>>
>> If no firmware (or bootloader) has initialized the controller, the
>> register value is zero, which is the default after power-on-reset.
>>
>> In other words, the Linux driver cannot initialize the controller
>> by itself.  You cannot support the reset control either because
>> resetting the controller will get register values lost.
>>
>> This commit adds a way to specify it via DT.  If the property
>> "denali,oob-skip-bytes" exists, the value will be set to the register.
>
> Hm, do we really need to make this config customizable? I mean, either
> you have a large-page NAND (page > 512 bytes) and the 2 first bytes
> must be reserved for the BBM or you have a small-page NAND and the BBM
> is at position 4 and 5. Are you sure people configure that differently?
> Don't you always have SPARE_AREA_SKIP_BYTES set to 6 or 2?


As I said in the patch description,
I need to use the same SPARE_AREA_SKIP_BYTES value
across firmware, boot-loader, Linux, and whatever.

I want to set the value to 8 for my platform
because the on-chip boot ROM expects 8.
I cannot change it since the boot ROM is hard-wired.


The boot ROM skips 8 bytes in OOB
when it loads images from the on-board NAND device.

So, when I update the image from U-Boot or Linux,
I need to make sure to set the register to 8.

If I update the image with a different value,
the Boot ROM fails to boot.



When the system has booted from NAND,
the register is already set to 8.  It works.

However, when the system has booted from eMMC,
the register is not initialized by anyone.
I am searching for a way to set the register to 8
in this case.


The boot ROM in SOCFPGA might expect a different value,
I am not sure.



Thanks.



> Regards,
>
> Boris
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/



-- 
Best Regards
Masahiro Yamada

Re: [PATCH] mtd: rawnand: denali: add DT property to specify skipped bytes in OOB

[Boris Brezillon]

On Fri, 7 Sep 2018 23:42:53 +0900
Masahiro Yamada <yamada.masahiro@socionext.com> wrote:

> Hi Boris,
> 
> 2018-09-07 23:08 GMT+09:00 Boris Brezillon <boris.brezillon@bootlin.com>:
> > Hi Masahiro,
> >
> > On Fri,  7 Sep 2018 19:56:23 +0900
> > Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> >  
> >> NAND devices need additional data area (OOB) for error correction,
> >> but it is also used for Bad Block Marker (BBM).  In many cases, the
> >> first byte in OOB is used for BBM, but the location actually depends
> >> on chip vendors.  The NAND controller should preserve the precious
> >> BBM to keep track of bad blocks.
> >>
> >> In Denali IP, the SPARE_AREA_SKIP_BYTES register is used to specify
> >> the number of bytes to skip from the start of OOB.  The ECC engine
> >> will automatically skip the specified number of bytes when it gets
> >> access to OOB area.
> >>
> >> The same value for SPARE_AREA_SKIP_BYTES should be used between
> >> firmware and the operating system if you intend to use the NAND
> >> device across the control hand-off.
> >>
> >> In fact, the current denali.c code expects firmware to have already
> >> set the SPARE_AREA_SKIP_BYTES register, then reads the value out.
> >>
> >> If no firmware (or bootloader) has initialized the controller, the
> >> register value is zero, which is the default after power-on-reset.
> >>
> >> In other words, the Linux driver cannot initialize the controller
> >> by itself.  You cannot support the reset control either because
> >> resetting the controller will get register values lost.
> >>
> >> This commit adds a way to specify it via DT.  If the property
> >> "denali,oob-skip-bytes" exists, the value will be set to the register.  
> >
> > Hm, do we really need to make this config customizable? I mean, either
> > you have a large-page NAND (page > 512 bytes) and the 2 first bytes
> > must be reserved for the BBM or you have a small-page NAND and the BBM
> > is at position 4 and 5. Are you sure people configure that differently?
> > Don't you always have SPARE_AREA_SKIP_BYTES set to 6 or 2?  
> 
> 
> As I said in the patch description,
> I need to use the same SPARE_AREA_SKIP_BYTES value
> across firmware, boot-loader, Linux, and whatever.
> 
> I want to set the value to 8 for my platform
> because the on-chip boot ROM expects 8.
> I cannot change it since the boot ROM is hard-wired.
> 
> 
> The boot ROM skips 8 bytes in OOB
> when it loads images from the on-board NAND device.
> 
> So, when I update the image from U-Boot or Linux,
> I need to make sure to set the register to 8.
> 
> If I update the image with a different value,
> the Boot ROM fails to boot.
> 
> 
> 
> When the system has booted from NAND,
> the register is already set to 8.  It works.
> 
> However, when the system has booted from eMMC,
> the register is not initialized by anyone.
> I am searching for a way to set the register to 8
> in this case.
> 
> 
> The boot ROM in SOCFPGA might expect a different value,
> I am not sure.

Okay, then why not having a per-compatible value if it's related to the
BootROM? Unless the BootROM is part of the FPGA and can be
reprogrammed. I'd really prefer not having a generic property that
allows you to put anything you want.

Re: [PATCH] mtd: rawnand: denali: add DT property to specify skipped bytes in OOB

[Masahiro Yamada]

Hi Boris,

2018-09-07 23:53 GMT+09:00 Boris Brezillon <boris.brezillon@bootlin.com>:
> On Fri, 7 Sep 2018 23:42:53 +0900
> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>
>> Hi Boris,
>>
>> 2018-09-07 23:08 GMT+09:00 Boris Brezillon <boris.brezillon@bootlin.com>:
>> > Hi Masahiro,
>> >
>> > On Fri,  7 Sep 2018 19:56:23 +0900
>> > Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>> >
>> >> NAND devices need additional data area (OOB) for error correction,
>> >> but it is also used for Bad Block Marker (BBM).  In many cases, the
>> >> first byte in OOB is used for BBM, but the location actually depends
>> >> on chip vendors.  The NAND controller should preserve the precious
>> >> BBM to keep track of bad blocks.
>> >>
>> >> In Denali IP, the SPARE_AREA_SKIP_BYTES register is used to specify
>> >> the number of bytes to skip from the start of OOB.  The ECC engine
>> >> will automatically skip the specified number of bytes when it gets
>> >> access to OOB area.
>> >>
>> >> The same value for SPARE_AREA_SKIP_BYTES should be used between
>> >> firmware and the operating system if you intend to use the NAND
>> >> device across the control hand-off.
>> >>
>> >> In fact, the current denali.c code expects firmware to have already
>> >> set the SPARE_AREA_SKIP_BYTES register, then reads the value out.
>> >>
>> >> If no firmware (or bootloader) has initialized the controller, the
>> >> register value is zero, which is the default after power-on-reset.
>> >>
>> >> In other words, the Linux driver cannot initialize the controller
>> >> by itself.  You cannot support the reset control either because
>> >> resetting the controller will get register values lost.
>> >>
>> >> This commit adds a way to specify it via DT.  If the property
>> >> "denali,oob-skip-bytes" exists, the value will be set to the register.
>> >
>> > Hm, do we really need to make this config customizable? I mean, either
>> > you have a large-page NAND (page > 512 bytes) and the 2 first bytes
>> > must be reserved for the BBM or you have a small-page NAND and the BBM
>> > is at position 4 and 5. Are you sure people configure that differently?
>> > Don't you always have SPARE_AREA_SKIP_BYTES set to 6 or 2?
>>
>>
>> As I said in the patch description,
>> I need to use the same SPARE_AREA_SKIP_BYTES value
>> across firmware, boot-loader, Linux, and whatever.
>>
>> I want to set the value to 8 for my platform
>> because the on-chip boot ROM expects 8.
>> I cannot change it since the boot ROM is hard-wired.
>>
>>
>> The boot ROM skips 8 bytes in OOB
>> when it loads images from the on-board NAND device.
>>
>> So, when I update the image from U-Boot or Linux,
>> I need to make sure to set the register to 8.
>>
>> If I update the image with a different value,
>> the Boot ROM fails to boot.
>>
>>
>>
>> When the system has booted from NAND,
>> the register is already set to 8.  It works.
>>
>> However, when the system has booted from eMMC,
>> the register is not initialized by anyone.
>> I am searching for a way to set the register to 8
>> in this case.
>>
>>
>> The boot ROM in SOCFPGA might expect a different value,
>> I am not sure.
>
> Okay, then why not having a per-compatible value if it's related to the
> BootROM? Unless the BootROM is part of the FPGA and can be
> reprogrammed.

FPGA is unrelated here.

Neither the boot ROM nor the Denali core is re-programmable.



I hesitate to associate the number of skipped bytes
with the compatible string because it is not a parameter
of the Denali IP.


Rather, it is the matter of "how we use the OOB",
so I want to leave room for customization like nand-ecc-strength etc.
even if the boot ROM happens to expect a particular value.


If you prefer a per-compatible value, I can do that,
but I believe the NAND core and the boot ROM are orthogonal.



> I'd really prefer not having a generic property that
> allows you to put anything you want.






-- 
Best Regards
Masahiro Yamada

Re: [PATCH] mtd: rawnand: denali: add DT property to specify skipped bytes in OOB

[Miquel Raynal]

Hi Masahiro,

Masahiro Yamada <yamada.masahiro@socionext.com> wrote on Sat, 8 Sep
2018 01:10:25 +0900:

> Hi Boris,
> 
> 2018-09-07 23:53 GMT+09:00 Boris Brezillon <boris.brezillon@bootlin.com>:
> > On Fri, 7 Sep 2018 23:42:53 +0900
> > Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> >  
> >> Hi Boris,
> >>
> >> 2018-09-07 23:08 GMT+09:00 Boris Brezillon <boris.brezillon@bootlin.com>:  
> >> > Hi Masahiro,
> >> >
> >> > On Fri,  7 Sep 2018 19:56:23 +0900
> >> > Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> >> >  
> >> >> NAND devices need additional data area (OOB) for error correction,
> >> >> but it is also used for Bad Block Marker (BBM).  In many cases, the
> >> >> first byte in OOB is used for BBM, but the location actually depends
> >> >> on chip vendors.  The NAND controller should preserve the precious
> >> >> BBM to keep track of bad blocks.
> >> >>
> >> >> In Denali IP, the SPARE_AREA_SKIP_BYTES register is used to specify
> >> >> the number of bytes to skip from the start of OOB.  The ECC engine
> >> >> will automatically skip the specified number of bytes when it gets
> >> >> access to OOB area.
> >> >>
> >> >> The same value for SPARE_AREA_SKIP_BYTES should be used between
> >> >> firmware and the operating system if you intend to use the NAND
> >> >> device across the control hand-off.
> >> >>
> >> >> In fact, the current denali.c code expects firmware to have already
> >> >> set the SPARE_AREA_SKIP_BYTES register, then reads the value out.
> >> >>
> >> >> If no firmware (or bootloader) has initialized the controller, the
> >> >> register value is zero, which is the default after power-on-reset.
> >> >>
> >> >> In other words, the Linux driver cannot initialize the controller
> >> >> by itself.  You cannot support the reset control either because
> >> >> resetting the controller will get register values lost.
> >> >>
> >> >> This commit adds a way to specify it via DT.  If the property
> >> >> "denali,oob-skip-bytes" exists, the value will be set to the register.  
> >> >
> >> > Hm, do we really need to make this config customizable? I mean, either
> >> > you have a large-page NAND (page > 512 bytes) and the 2 first bytes
> >> > must be reserved for the BBM or you have a small-page NAND and the BBM
> >> > is at position 4 and 5. Are you sure people configure that differently?
> >> > Don't you always have SPARE_AREA_SKIP_BYTES set to 6 or 2?  
> >>
> >>
> >> As I said in the patch description,
> >> I need to use the same SPARE_AREA_SKIP_BYTES value
> >> across firmware, boot-loader, Linux, and whatever.
> >>
> >> I want to set the value to 8 for my platform
> >> because the on-chip boot ROM expects 8.
> >> I cannot change it since the boot ROM is hard-wired.
> >>
> >>
> >> The boot ROM skips 8 bytes in OOB
> >> when it loads images from the on-board NAND device.
> >>
> >> So, when I update the image from U-Boot or Linux,
> >> I need to make sure to set the register to 8.
> >>
> >> If I update the image with a different value,
> >> the Boot ROM fails to boot.
> >>
> >>
> >>
> >> When the system has booted from NAND,
> >> the register is already set to 8.  It works.
> >>
> >> However, when the system has booted from eMMC,
> >> the register is not initialized by anyone.
> >> I am searching for a way to set the register to 8
> >> in this case.
> >>
> >>
> >> The boot ROM in SOCFPGA might expect a different value,
> >> I am not sure.  
> >
> > Okay, then why not having a per-compatible value if it's related to the
> > BootROM? Unless the BootROM is part of the FPGA and can be
> > reprogrammed.  
> 
> FPGA is unrelated here.
> 
> Neither the boot ROM nor the Denali core is re-programmable.
> 
> 
> 
> I hesitate to associate the number of skipped bytes
> with the compatible string because it is not a parameter
> of the Denali IP.
> 
> 
> Rather, it is the matter of "how we use the OOB",
> so I want to leave room for customization like nand-ecc-strength etc.
> even if the boot ROM happens to expect a particular value.
> 
> 
> If you prefer a per-compatible value, I can do that,
> but I believe the NAND core and the boot ROM are orthogonal.
> 
> 
> 
> > I'd really prefer not having a generic property that
> > allows you to put anything you want.  
> 
> 

While I agree that the number of skipped bytes is not a parameter of
the Denali IP, I also fear letting the opportunity to the user to use
random values in the SPARE_AREA_SKIP_BYTES registers (and have to
support them). I would also prefer a per-compatible value which is not
a perfect solution neither.


Thanks,
Miquèl

Re: [PATCH] mtd: rawnand: denali: add DT property to specify skipped bytes in OOB

[Boris Brezillon]

On Sat, 22 Sep 2018 09:41:11 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Hi Masahiro,
> 
> Masahiro Yamada <yamada.masahiro@socionext.com> wrote on Sat, 8 Sep
> 2018 01:10:25 +0900:
> 
> > Hi Boris,
> > 
> > 2018-09-07 23:53 GMT+09:00 Boris Brezillon <boris.brezillon@bootlin.com>:  
> > > On Fri, 7 Sep 2018 23:42:53 +0900
> > > Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> > >    
> > >> Hi Boris,
> > >>
> > >> 2018-09-07 23:08 GMT+09:00 Boris Brezillon <boris.brezillon@bootlin.com>:    
> > >> > Hi Masahiro,
> > >> >
> > >> > On Fri,  7 Sep 2018 19:56:23 +0900
> > >> > Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> > >> >    
> > >> >> NAND devices need additional data area (OOB) for error correction,
> > >> >> but it is also used for Bad Block Marker (BBM).  In many cases, the
> > >> >> first byte in OOB is used for BBM, but the location actually depends
> > >> >> on chip vendors.  The NAND controller should preserve the precious
> > >> >> BBM to keep track of bad blocks.
> > >> >>
> > >> >> In Denali IP, the SPARE_AREA_SKIP_BYTES register is used to specify
> > >> >> the number of bytes to skip from the start of OOB.  The ECC engine
> > >> >> will automatically skip the specified number of bytes when it gets
> > >> >> access to OOB area.
> > >> >>
> > >> >> The same value for SPARE_AREA_SKIP_BYTES should be used between
> > >> >> firmware and the operating system if you intend to use the NAND
> > >> >> device across the control hand-off.
> > >> >>
> > >> >> In fact, the current denali.c code expects firmware to have already
> > >> >> set the SPARE_AREA_SKIP_BYTES register, then reads the value out.
> > >> >>
> > >> >> If no firmware (or bootloader) has initialized the controller, the
> > >> >> register value is zero, which is the default after power-on-reset.
> > >> >>
> > >> >> In other words, the Linux driver cannot initialize the controller
> > >> >> by itself.  You cannot support the reset control either because
> > >> >> resetting the controller will get register values lost.
> > >> >>
> > >> >> This commit adds a way to specify it via DT.  If the property
> > >> >> "denali,oob-skip-bytes" exists, the value will be set to the register.    
> > >> >
> > >> > Hm, do we really need to make this config customizable? I mean, either
> > >> > you have a large-page NAND (page > 512 bytes) and the 2 first bytes
> > >> > must be reserved for the BBM or you have a small-page NAND and the BBM
> > >> > is at position 4 and 5. Are you sure people configure that differently?
> > >> > Don't you always have SPARE_AREA_SKIP_BYTES set to 6 or 2?    
> > >>
> > >>
> > >> As I said in the patch description,
> > >> I need to use the same SPARE_AREA_SKIP_BYTES value
> > >> across firmware, boot-loader, Linux, and whatever.
> > >>
> > >> I want to set the value to 8 for my platform
> > >> because the on-chip boot ROM expects 8.
> > >> I cannot change it since the boot ROM is hard-wired.
> > >>
> > >>
> > >> The boot ROM skips 8 bytes in OOB
> > >> when it loads images from the on-board NAND device.
> > >>
> > >> So, when I update the image from U-Boot or Linux,
> > >> I need to make sure to set the register to 8.
> > >>
> > >> If I update the image with a different value,
> > >> the Boot ROM fails to boot.
> > >>
> > >>
> > >>
> > >> When the system has booted from NAND,
> > >> the register is already set to 8.  It works.
> > >>
> > >> However, when the system has booted from eMMC,
> > >> the register is not initialized by anyone.
> > >> I am searching for a way to set the register to 8
> > >> in this case.

Maybe there's a solution which does not involve attaching a per-compat
value or adding a DT prop. If the FW/bootloader has not initialized this
register the value is 0, right? Why not testing the value and
assigning it to the default (8) if it's not been initialized by the
bootloader. That shouldn't break existing platforms since I don't think
0 is a valid value anyway.

	denali->oob_skip_bytes = ioread32(denali->reg +
					  SPARE_AREA_SKIP_BYTES);
	if (!denali->oob_skip_bytes) {
		denali->oob_skip_bytes = DEFAULT_OOB_SKIP_BYTES;
		iowrite32(denali->oob_skip_bytes,
			  denali->reg + SPARE_AREA_SKIP_BYTES);
	}

Re: [PATCH] mtd: rawnand: denali: add DT property to specify skipped bytes in OOB

[Masahiro Yamada]

2018-09-22 4:11 GMT-04:00 Boris Brezillon <boris.brezillon@bootlin.com>:
> On Sat, 22 Sep 2018 09:41:11 +0200
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
>> Hi Masahiro,
>>
>> Masahiro Yamada <yamada.masahiro@socionext.com> wrote on Sat, 8 Sep
>> 2018 01:10:25 +0900:
>>
>> > Hi Boris,
>> >
>> > 2018-09-07 23:53 GMT+09:00 Boris Brezillon <boris.brezillon@bootlin.com>:
>> > > On Fri, 7 Sep 2018 23:42:53 +0900
>> > > Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>> > >
>> > >> Hi Boris,
>> > >>
>> > >> 2018-09-07 23:08 GMT+09:00 Boris Brezillon <boris.brezillon@bootlin.com>:
>> > >> > Hi Masahiro,
>> > >> >
>> > >> > On Fri,  7 Sep 2018 19:56:23 +0900
>> > >> > Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>> > >> >
>> > >> >> NAND devices need additional data area (OOB) for error correction,
>> > >> >> but it is also used for Bad Block Marker (BBM).  In many cases, the
>> > >> >> first byte in OOB is used for BBM, but the location actually depends
>> > >> >> on chip vendors.  The NAND controller should preserve the precious
>> > >> >> BBM to keep track of bad blocks.
>> > >> >>
>> > >> >> In Denali IP, the SPARE_AREA_SKIP_BYTES register is used to specify
>> > >> >> the number of bytes to skip from the start of OOB.  The ECC engine
>> > >> >> will automatically skip the specified number of bytes when it gets
>> > >> >> access to OOB area.
>> > >> >>
>> > >> >> The same value for SPARE_AREA_SKIP_BYTES should be used between
>> > >> >> firmware and the operating system if you intend to use the NAND
>> > >> >> device across the control hand-off.
>> > >> >>
>> > >> >> In fact, the current denali.c code expects firmware to have already
>> > >> >> set the SPARE_AREA_SKIP_BYTES register, then reads the value out.
>> > >> >>
>> > >> >> If no firmware (or bootloader) has initialized the controller, the
>> > >> >> register value is zero, which is the default after power-on-reset.
>> > >> >>
>> > >> >> In other words, the Linux driver cannot initialize the controller
>> > >> >> by itself.  You cannot support the reset control either because
>> > >> >> resetting the controller will get register values lost.
>> > >> >>
>> > >> >> This commit adds a way to specify it via DT.  If the property
>> > >> >> "denali,oob-skip-bytes" exists, the value will be set to the register.
>> > >> >
>> > >> > Hm, do we really need to make this config customizable? I mean, either
>> > >> > you have a large-page NAND (page > 512 bytes) and the 2 first bytes
>> > >> > must be reserved for the BBM or you have a small-page NAND and the BBM
>> > >> > is at position 4 and 5. Are you sure people configure that differently?
>> > >> > Don't you always have SPARE_AREA_SKIP_BYTES set to 6 or 2?
>> > >>
>> > >>
>> > >> As I said in the patch description,
>> > >> I need to use the same SPARE_AREA_SKIP_BYTES value
>> > >> across firmware, boot-loader, Linux, and whatever.
>> > >>
>> > >> I want to set the value to 8 for my platform
>> > >> because the on-chip boot ROM expects 8.
>> > >> I cannot change it since the boot ROM is hard-wired.
>> > >>
>> > >>
>> > >> The boot ROM skips 8 bytes in OOB
>> > >> when it loads images from the on-board NAND device.
>> > >>
>> > >> So, when I update the image from U-Boot or Linux,
>> > >> I need to make sure to set the register to 8.
>> > >>
>> > >> If I update the image with a different value,
>> > >> the Boot ROM fails to boot.
>> > >>
>> > >>
>> > >>
>> > >> When the system has booted from NAND,
>> > >> the register is already set to 8.  It works.
>> > >>
>> > >> However, when the system has booted from eMMC,
>> > >> the register is not initialized by anyone.
>> > >> I am searching for a way to set the register to 8
>> > >> in this case.
>
> Maybe there's a solution which does not involve attaching a per-compat
> value or adding a DT prop. If the FW/bootloader has not initialized this
> register the value is 0, right? Why not testing the value and
> assigning it to the default (8) if it's not been initialized by the
> bootloader. That shouldn't break existing platforms since I don't think
> 0 is a valid value anyway.
>
>         denali->oob_skip_bytes = ioread32(denali->reg +
>                                           SPARE_AREA_SKIP_BYTES);
>         if (!denali->oob_skip_bytes) {
>                 denali->oob_skip_bytes = DEFAULT_OOB_SKIP_BYTES;
>                 iowrite32(denali->oob_skip_bytes,
>                           denali->reg + SPARE_AREA_SKIP_BYTES);
>         }
>


I prefer per-compatible values to a fixed default.


I'd like to set the register to 8 unless set otherwise
because the boot ROM on my platform (Socionext UniPhier SoCs)
uses that value.

Other platforms like Altera SOCFPGA may want to use a different value
(at least, I do not know what is the preferred value).



-- 
Best Regards
Masahiro Yamada

Re: [PATCH] mtd: rawnand: denali: add DT property to specify skipped bytes in OOB

[Miquel Raynal]

Hi Masahiro,

Masahiro Yamada <yamada.masahiro@socionext.com> wrote on Sun, 23 Sep
2018 06:38:40 -0400:

> 2018-09-22 4:11 GMT-04:00 Boris Brezillon <boris.brezillon@bootlin.com>:
> > On Sat, 22 Sep 2018 09:41:11 +0200
> > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >  
> >> Hi Masahiro,
> >>
> >> Masahiro Yamada <yamada.masahiro@socionext.com> wrote on Sat, 8 Sep
> >> 2018 01:10:25 +0900:
> >>  
> >> > Hi Boris,
> >> >
> >> > 2018-09-07 23:53 GMT+09:00 Boris Brezillon <boris.brezillon@bootlin.com>:  
> >> > > On Fri, 7 Sep 2018 23:42:53 +0900
> >> > > Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> >> > >  
> >> > >> Hi Boris,
> >> > >>
> >> > >> 2018-09-07 23:08 GMT+09:00 Boris Brezillon <boris.brezillon@bootlin.com>:  
> >> > >> > Hi Masahiro,
> >> > >> >
> >> > >> > On Fri,  7 Sep 2018 19:56:23 +0900
> >> > >> > Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> >> > >> >  
> >> > >> >> NAND devices need additional data area (OOB) for error correction,
> >> > >> >> but it is also used for Bad Block Marker (BBM).  In many cases, the
> >> > >> >> first byte in OOB is used for BBM, but the location actually depends
> >> > >> >> on chip vendors.  The NAND controller should preserve the precious
> >> > >> >> BBM to keep track of bad blocks.
> >> > >> >>
> >> > >> >> In Denali IP, the SPARE_AREA_SKIP_BYTES register is used to specify
> >> > >> >> the number of bytes to skip from the start of OOB.  The ECC engine
> >> > >> >> will automatically skip the specified number of bytes when it gets
> >> > >> >> access to OOB area.
> >> > >> >>
> >> > >> >> The same value for SPARE_AREA_SKIP_BYTES should be used between
> >> > >> >> firmware and the operating system if you intend to use the NAND
> >> > >> >> device across the control hand-off.
> >> > >> >>
> >> > >> >> In fact, the current denali.c code expects firmware to have already
> >> > >> >> set the SPARE_AREA_SKIP_BYTES register, then reads the value out.
> >> > >> >>
> >> > >> >> If no firmware (or bootloader) has initialized the controller, the
> >> > >> >> register value is zero, which is the default after power-on-reset.
> >> > >> >>
> >> > >> >> In other words, the Linux driver cannot initialize the controller
> >> > >> >> by itself.  You cannot support the reset control either because
> >> > >> >> resetting the controller will get register values lost.
> >> > >> >>
> >> > >> >> This commit adds a way to specify it via DT.  If the property
> >> > >> >> "denali,oob-skip-bytes" exists, the value will be set to the register.  
> >> > >> >
> >> > >> > Hm, do we really need to make this config customizable? I mean, either
> >> > >> > you have a large-page NAND (page > 512 bytes) and the 2 first bytes
> >> > >> > must be reserved for the BBM or you have a small-page NAND and the BBM
> >> > >> > is at position 4 and 5. Are you sure people configure that differently?
> >> > >> > Don't you always have SPARE_AREA_SKIP_BYTES set to 6 or 2?  
> >> > >>
> >> > >>
> >> > >> As I said in the patch description,
> >> > >> I need to use the same SPARE_AREA_SKIP_BYTES value
> >> > >> across firmware, boot-loader, Linux, and whatever.
> >> > >>
> >> > >> I want to set the value to 8 for my platform
> >> > >> because the on-chip boot ROM expects 8.
> >> > >> I cannot change it since the boot ROM is hard-wired.
> >> > >>
> >> > >>
> >> > >> The boot ROM skips 8 bytes in OOB
> >> > >> when it loads images from the on-board NAND device.
> >> > >>
> >> > >> So, when I update the image from U-Boot or Linux,
> >> > >> I need to make sure to set the register to 8.
> >> > >>
> >> > >> If I update the image with a different value,
> >> > >> the Boot ROM fails to boot.
> >> > >>
> >> > >>
> >> > >>
> >> > >> When the system has booted from NAND,
> >> > >> the register is already set to 8.  It works.
> >> > >>
> >> > >> However, when the system has booted from eMMC,
> >> > >> the register is not initialized by anyone.
> >> > >> I am searching for a way to set the register to 8
> >> > >> in this case.  
> >
> > Maybe there's a solution which does not involve attaching a per-compat
> > value or adding a DT prop. If the FW/bootloader has not initialized this
> > register the value is 0, right? Why not testing the value and
> > assigning it to the default (8) if it's not been initialized by the
> > bootloader. That shouldn't break existing platforms since I don't think
> > 0 is a valid value anyway.
> >
> >         denali->oob_skip_bytes = ioread32(denali->reg +
> >                                           SPARE_AREA_SKIP_BYTES);
> >         if (!denali->oob_skip_bytes) {
> >                 denali->oob_skip_bytes = DEFAULT_OOB_SKIP_BYTES;
> >                 iowrite32(denali->oob_skip_bytes,
> >                           denali->reg + SPARE_AREA_SKIP_BYTES);
> >         }
> >  
> 
> 
> I prefer per-compatible values to a fixed default.
> 
> 
> I'd like to set the register to 8 unless set otherwise
> because the boot ROM on my platform (Socionext UniPhier SoCs)
> uses that value.
> 
> Other platforms like Altera SOCFPGA may want to use a different value
> (at least, I do not know what is the preferred value).
> 
> 

Well, for now we can just have a default to 8, and if someone complains
have a per-compatible default?


Thanks,
Miquèl