diff mbox

mtd: nand: Disable subpage writes for drivers without ecc->hwctl

Message ID 20980858CB6D3A4BAE95CA194937D5E73EAC2C82@DBDE04.ent.ti.com
State Superseded
Headers show

Commit Message

pekon gupta April 14, 2014, 5:55 a.m. UTC 
>From: Scott Wood [mailto:scottwood@freescale.com]
>>On Fri, 2014-04-11 at 06:35 +0000, Gupta, Pekon wrote:
>> As per [1] and [2], If the subpage is _not_ supported then VID-header offset
>> should be aligned to  PAGE_SIZE boundary. And it has implications on LEB size
>> calculation too. I don't know how it was working earlier but if the earlier
>> UBI images were using 512 as VID Header offset even without subpage
>> Support then it's incorrect configuration.
>
>I believe what was happening before was full page writes with only
>subpage content (the rest left at 0xff).
>
Yes, that is possible, sorry I missed that nand_do_write_ops() does 0xff padding
for non-page aligned writes. In that case replicating fsl_elbc_write_page() for
chip->ecc.write_subpage should also work and maintain backward compatibility.

(not compile tested)
------------------------------------

------------------------------------

with regards, pekon

Comments

Helmut Schaa April 14, 2014, 6:18 a.m. UTC | #1 
On 14. April 2014 07:55:30 MESZ, "Gupta, Pekon" <pekon@ti.com> wrote:
>>From: Scott Wood [mailto:scottwood@freescale.com]
>>>On Fri, 2014-04-11 at 06:35 +0000, Gupta, Pekon wrote:
>>> As per [1] and [2], If the subpage is _not_ supported then
>VID-header offset
>>> should be aligned to  PAGE_SIZE boundary. And it has implications on
>LEB size
>>> calculation too. I don't know how it was working earlier but if the
>earlier
>>> UBI images were using 512 as VID Header offset even without subpage
>>> Support then it's incorrect configuration.
>>
>>I believe what was happening before was full page writes with only
>>subpage content (the rest left at 0xff).
>>
>Yes, that is possible, sorry I missed that nand_do_write_ops() does
>0xff padding
>for non-page aligned writes. In that case replicating
>fsl_elbc_write_page() for
>chip->ecc.write_subpage should also work and maintain backward
>compatibility.
>
>(not compile tested)

JFI, I'm in vacation till next week and don't have access to the affected system. Will do a test once I'm back.
Helmut

>------------------------------------
>diff --git a/drivers/mtd/nand/fsl_elbc_nand.c
>b/drivers/mtd/nand/fsl_elbc_nand.c
>index a21252c..cc79ce4 100644
>--- a/drivers/mtd/nand/fsl_elbc_nand.c
>+++ b/drivers/mtd/nand/fsl_elbc_nand.c
>@@ -723,6 +723,19 @@ static int fsl_elbc_write_page(struct mtd_info
>*mtd, struct nand_chip *chip,
>        return 0;
> }
>
>+/* ECC will be calculated automatically, and errors will be detected
>in
>+ * waitfunc.
>+ */
>+static int fsl_elbc_write_subpage(struct mtd_info *mtd, struct
>nand_chip *chip,
>+                               uint32_t offset, uint32_t data_len,
>+                               const uint8_t *buf, int oob_required)
>+{
>+       fsl_elbc_write_buf(mtd, buf, mtd->writesize);
>+       fsl_elbc_write_buf(mtd, chip->oob_poi, mtd->oobsize);
>+
>+       return 0;
>+}
>+
> static int fsl_elbc_chip_init(struct fsl_elbc_mtd *priv)
> {
>        struct fsl_lbc_ctrl *ctrl = priv->ctrl;
>@@ -762,6 +775,7 @@ static int fsl_elbc_chip_init(struct fsl_elbc_mtd
>*priv)
>
>        chip->ecc.read_page = fsl_elbc_read_page;
>        chip->ecc.write_page = fsl_elbc_write_page;
>+       chip->ecc.write_subpage = fsl_elbc_write_subpage;
>
>        /* If CS Base Register selects full hardware ECC then use it */
>        if ((in_be32(&lbc->bank[priv->bank].br) & BR_DECC) ==
>
>------------------------------------
>
>with regards, pekon
pekon gupta May 5, 2014, 8:46 a.m. UTC | #2 
Hi,

>From: Helmut Schaa [mailto:helmut.schaa@googlemail.com]
>>On 14. April 2014 07:55:30 MESZ, "Gupta, Pekon" <pekon@ti.com>
>>>From: Scott Wood [mailto:scottwood@freescale.com]
>>>>On Fri, 2014-04-11 at 06:35 +0000, Gupta, Pekon wrote:

>>>> As per [1] and [2], If the subpage is _not_ supported then
>>VID-header offset
>>>> should be aligned to  PAGE_SIZE boundary. And it has
>implications on
>>LEB size
>>>> calculation too. I don't know how it was working earlier but if the
>>earlier
>>>> UBI images were using 512 as VID Header offset even without
>subpage
>>>> Support then it's incorrect configuration.
>>>
>>>I believe what was happening before was full page writes with only
>>>subpage content (the rest left at 0xff).
>>>
>>Yes, that is possible, sorry I missed that nand_do_write_ops() does
>>0xff padding
>>for non-page aligned writes. In that case replicating
>>fsl_elbc_write_page() for
>>chip->ecc.write_subpage should also work and maintain backward
>>compatibility.
>>
>>(not compile tested)
>
>JFI, I'm in vacation till next week and don't have access to the affected
>system. Will do a test once I'm back.
>Helmut
>

Any update on below patch? Does this maintain compatibility to
existing UBIFS images ?

with regards, Pekon



>>------------------------------------
>>diff --git a/drivers/mtd/nand/fsl_elbc_nand.c
>>b/drivers/mtd/nand/fsl_elbc_nand.c
>>index a21252c..cc79ce4 100644
>>--- a/drivers/mtd/nand/fsl_elbc_nand.c
>>+++ b/drivers/mtd/nand/fsl_elbc_nand.c
>>@@ -723,6 +723,19 @@ static int fsl_elbc_write_page(struct
>mtd_info
>>*mtd, struct nand_chip *chip,
>>        return 0;
>> }
>>
>>+/* ECC will be calculated automatically, and errors will be detected
>>in
>>+ * waitfunc.
>>+ */
>>+static int fsl_elbc_write_subpage(struct mtd_info *mtd, struct
>>nand_chip *chip,
>>+                               uint32_t offset, uint32_t data_len,
>>+                               const uint8_t *buf, int oob_required)
>>+{
>>+       fsl_elbc_write_buf(mtd, buf, mtd->writesize);
>>+       fsl_elbc_write_buf(mtd, chip->oob_poi, mtd->oobsize);
>>+
>>+       return 0;
>>+}
>>+
>> static int fsl_elbc_chip_init(struct fsl_elbc_mtd *priv)
>> {
>>        struct fsl_lbc_ctrl *ctrl = priv->ctrl;
>>@@ -762,6 +775,7 @@ static int fsl_elbc_chip_init(struct
>fsl_elbc_mtd
>>*priv)
>>
>>        chip->ecc.read_page = fsl_elbc_read_page;
>>        chip->ecc.write_page = fsl_elbc_write_page;
>>+       chip->ecc.write_subpage = fsl_elbc_write_subpage;
>>
>>        /* If CS Base Register selects full hardware ECC then use it */
>>        if ((in_be32(&lbc->bank[priv->bank].br) & BR_DECC) ==
>>
>>------------------------------------
Helmut Schaa May 5, 2014, 4:08 p.m. UTC | #3 
Hi Pekon,

On Mon, May 5, 2014 at 10:46 AM, Gupta, Pekon <pekon@ti.com> wrote:
> Any update on below patch? Does this maintain compatibility to
> existing UBIFS images ?

Patch works fine with a VID header offset of 512 and without the crash :)

Feel free to add my

Reported-by: Helmut Schaa <helmut.schaa@googlemail>
Tested-by: Helmut Schaa <helmut.schaa@googlemail>

when sending the patch.

Thanks,
Helmut

> with regards, Pekon
>
>
>
>>>------------------------------------
>>>diff --git a/drivers/mtd/nand/fsl_elbc_nand.c
>>>b/drivers/mtd/nand/fsl_elbc_nand.c
>>>index a21252c..cc79ce4 100644
>>>--- a/drivers/mtd/nand/fsl_elbc_nand.c
>>>+++ b/drivers/mtd/nand/fsl_elbc_nand.c
>>>@@ -723,6 +723,19 @@ static int fsl_elbc_write_page(struct
>>mtd_info
>>>*mtd, struct nand_chip *chip,
>>>        return 0;
>>> }
>>>
>>>+/* ECC will be calculated automatically, and errors will be detected
>>>in
>>>+ * waitfunc.
>>>+ */
>>>+static int fsl_elbc_write_subpage(struct mtd_info *mtd, struct
>>>nand_chip *chip,
>>>+                               uint32_t offset, uint32_t data_len,
>>>+                               const uint8_t *buf, int oob_required)
>>>+{
>>>+       fsl_elbc_write_buf(mtd, buf, mtd->writesize);
>>>+       fsl_elbc_write_buf(mtd, chip->oob_poi, mtd->oobsize);
>>>+
>>>+       return 0;
>>>+}
>>>+
>>> static int fsl_elbc_chip_init(struct fsl_elbc_mtd *priv)
>>> {
>>>        struct fsl_lbc_ctrl *ctrl = priv->ctrl;
>>>@@ -762,6 +775,7 @@ static int fsl_elbc_chip_init(struct
>>fsl_elbc_mtd
>>>*priv)
>>>
>>>        chip->ecc.read_page = fsl_elbc_read_page;
>>>        chip->ecc.write_page = fsl_elbc_write_page;
>>>+       chip->ecc.write_subpage = fsl_elbc_write_subpage;
>>>
>>>        /* If CS Base Register selects full hardware ECC then use it */
>>>        if ((in_be32(&lbc->bank[priv->bank].br) & BR_DECC) ==
>>>
>>>------------------------------------
diff mbox

Patch

diff --git a/drivers/mtd/nand/fsl_elbc_nand.c b/drivers/mtd/nand/fsl_elbc_nand.c
index a21252c..cc79ce4 100644
--- a/drivers/mtd/nand/fsl_elbc_nand.c
+++ b/drivers/mtd/nand/fsl_elbc_nand.c
@@ -723,6 +723,19 @@  static int fsl_elbc_write_page(struct mtd_info *mtd, struct nand_chip *chip,
        return 0;
 }

+/* ECC will be calculated automatically, and errors will be detected in
+ * waitfunc.
+ */
+static int fsl_elbc_write_subpage(struct mtd_info *mtd, struct nand_chip *chip,
+                               uint32_t offset, uint32_t data_len,
+                               const uint8_t *buf, int oob_required)
+{
+       fsl_elbc_write_buf(mtd, buf, mtd->writesize);
+       fsl_elbc_write_buf(mtd, chip->oob_poi, mtd->oobsize);
+
+       return 0;
+}
+
 static int fsl_elbc_chip_init(struct fsl_elbc_mtd *priv)
 {
        struct fsl_lbc_ctrl *ctrl = priv->ctrl;
@@ -762,6 +775,7 @@  static int fsl_elbc_chip_init(struct fsl_elbc_mtd *priv)

        chip->ecc.read_page = fsl_elbc_read_page;
        chip->ecc.write_page = fsl_elbc_write_page;
+       chip->ecc.write_subpage = fsl_elbc_write_subpage;

        /* If CS Base Register selects full hardware ECC then use it */
        if ((in_be32(&lbc->bank[priv->bank].br) & BR_DECC) ==