Re: [U-Boot] [PATCH 1/5] nand: Use common read function instead of verify_buf()
On Tue, 2015-01-27 at 17:47 -0600, Peter Tyser wrote: Hi Scott, I waffled about removing it, but leaned towards leaving it in because: - I didn't want to change the existing U-Boot behavior for other users. A google of 'u-boot nand write' shows a lot of examples that don't include verification of writes, and they should if we remove auto-verification. How many configs actually enable this option? I don't see many beyond the FSL PPC boards (which are so full of copy-and-paste that it probably wasn't deliberate). Yeah, the majority are FSL 83xx and 85xx, with 2 or so random ARM boards. - The reason it was removed in Linux was Both UBI and JFFS2 are able to read verify what they wrote already. There are also MTD tests which do this verification. I thought U-Boot was more likely than Linux to use raw NAND writes without a filesystem, so leaving it in U- Boot made sense since the UBI/JFFS2 logic didn't apply as much here. Right, though raw writes ought to be limited to blocks that aren't written often enough to fail. - I didn't think a lot of people would know they have to explicitly verify NAND contents after a write, since they'd assume it was like other memories that aren't as lossy. - The penalty of slightly different code from Linux and a small performance hit was worth the gain of auto-verification to me. I viewed consolidating it into one small chunk of code as a happy medium. The davinci patches show that there can still be driver dependencies depending on what the driver overrides. I'm not hugely opposed, but it seems like it would be better to do it at a higher level (e.g. in nand_util.c with a flag to enable, and either make support mandatory, or if you try to use that command variant without support it fails rather than silently not verifying). That seems like a good idea. How about: - Remove all CONFIG_MTD_NAND_VERIFY_WRITE references - Add a new flag WITH_WR_VERIFY and have nand_write_skip_bad() in nand_util.c verify writes only when it is set. - Update the calls to nand_write_skip_bad() in cmd_nand.c to include the new WITH_WR_VERIFY flag. I'd vote to enable it for all boards, but let me know if you disagree. That would make all nand write commands verify writes, with the exception of nand write.raw. Any opinion on if this should also be verified? I only use it for development/testing, so don't have a strong opinion. raw refers to the absence of ECC, and I'd rather not overload it to mean don't verify. Should it also be possible to request non-raw non-verified accesses? Or should we always verify and wait until someone complains about performance? What about DFU and other non-cmd_nand NAND accesses? -Scott ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/5] nand: Use common read function instead of verify_buf()
On Thu, 2015-01-29 at 17:02 -0600, Scott Wood wrote: On Tue, 2015-01-27 at 17:47 -0600, Peter Tyser wrote: Hi Scott, I waffled about removing it, but leaned towards leaving it in because: - I didn't want to change the existing U-Boot behavior for other users. A google of 'u-boot nand write' shows a lot of examples that don't include verification of writes, and they should if we remove auto-verification. How many configs actually enable this option? I don't see many beyond the FSL PPC boards (which are so full of copy-and-paste that it probably wasn't deliberate). Yeah, the majority are FSL 83xx and 85xx, with 2 or so random ARM boards. - The reason it was removed in Linux was Both UBI and JFFS2 are able to read verify what they wrote already. There are also MTD tests which do this verification. I thought U-Boot was more likely than Linux to use raw NAND writes without a filesystem, so leaving it in U- Boot made sense since the UBI/JFFS2 logic didn't apply as much here. Right, though raw writes ought to be limited to blocks that aren't written often enough to fail. - I didn't think a lot of people would know they have to explicitly verify NAND contents after a write, since they'd assume it was like other memories that aren't as lossy. - The penalty of slightly different code from Linux and a small performance hit was worth the gain of auto-verification to me. I viewed consolidating it into one small chunk of code as a happy medium. The davinci patches show that there can still be driver dependencies depending on what the driver overrides. I'm not hugely opposed, but it seems like it would be better to do it at a higher level (e.g. in nand_util.c with a flag to enable, and either make support mandatory, or if you try to use that command variant without support it fails rather than silently not verifying). That seems like a good idea. How about: - Remove all CONFIG_MTD_NAND_VERIFY_WRITE references - Add a new flag WITH_WR_VERIFY and have nand_write_skip_bad() in nand_util.c verify writes only when it is set. - Update the calls to nand_write_skip_bad() in cmd_nand.c to include the new WITH_WR_VERIFY flag. I'd vote to enable it for all boards, but let me know if you disagree. That would make all nand write commands verify writes, with the exception of nand write.raw. Any opinion on if this should also be verified? I only use it for development/testing, so don't have a strong opinion. raw refers to the absence of ECC, and I'd rather not overload it to mean don't verify. Should it also be possible to request non-raw non-verified accesses? Or should we always verify and wait until someone complains about performance? OK, I'll add verification to the nand write.raw functionality too. I'd lean towards always verifying and waiting until/if someone complains about performance. I doubt many (any?) people are doing timing critical writes in U-Boot. I think the argument that writes should be verified carries some water, and it'd be nice to not make the command arguments more complicated than they already are. What about DFU and other non-cmd_nand NAND accesses? Are there other non-cmd NAND accesses other than DFU? None jumped out at me. I'm not too familiar with DFU, but in theory the DFU programming utilities could already be doing their own verification. I took a quick look at the dfu-util source, and it doesn't appear to be doing its own. I'd vote to verify the DFU writes too, since even more than nand write its performance shouldn't be very critical. I'll break DFU verification out into a separate patch, and you or others can ACK or reject it then. Let me know if the above sounds good and I'll make the changes. Thanks, Peter ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/5] nand: Use common read function instead of verify_buf()
On Thu, 2015-01-29 at 17:37 -0600, Peter Tyser wrote: On Thu, 2015-01-29 at 17:02 -0600, Scott Wood wrote: On Tue, 2015-01-27 at 17:47 -0600, Peter Tyser wrote: Hi Scott, I waffled about removing it, but leaned towards leaving it in because: - I didn't want to change the existing U-Boot behavior for other users. A google of 'u-boot nand write' shows a lot of examples that don't include verification of writes, and they should if we remove auto-verification. How many configs actually enable this option? I don't see many beyond the FSL PPC boards (which are so full of copy-and-paste that it probably wasn't deliberate). Yeah, the majority are FSL 83xx and 85xx, with 2 or so random ARM boards. - The reason it was removed in Linux was Both UBI and JFFS2 are able to read verify what they wrote already. There are also MTD tests which do this verification. I thought U-Boot was more likely than Linux to use raw NAND writes without a filesystem, so leaving it in U- Boot made sense since the UBI/JFFS2 logic didn't apply as much here. Right, though raw writes ought to be limited to blocks that aren't written often enough to fail. - I didn't think a lot of people would know they have to explicitly verify NAND contents after a write, since they'd assume it was like other memories that aren't as lossy. - The penalty of slightly different code from Linux and a small performance hit was worth the gain of auto-verification to me. I viewed consolidating it into one small chunk of code as a happy medium. The davinci patches show that there can still be driver dependencies depending on what the driver overrides. I'm not hugely opposed, but it seems like it would be better to do it at a higher level (e.g. in nand_util.c with a flag to enable, and either make support mandatory, or if you try to use that command variant without support it fails rather than silently not verifying). That seems like a good idea. How about: - Remove all CONFIG_MTD_NAND_VERIFY_WRITE references - Add a new flag WITH_WR_VERIFY and have nand_write_skip_bad() in nand_util.c verify writes only when it is set. - Update the calls to nand_write_skip_bad() in cmd_nand.c to include the new WITH_WR_VERIFY flag. I'd vote to enable it for all boards, but let me know if you disagree. That would make all nand write commands verify writes, with the exception of nand write.raw. Any opinion on if this should also be verified? I only use it for development/testing, so don't have a strong opinion. raw refers to the absence of ECC, and I'd rather not overload it to mean don't verify. Should it also be possible to request non-raw non-verified accesses? Or should we always verify and wait until someone complains about performance? OK, I'll add verification to the nand write.raw functionality too. I'd lean towards always verifying and waiting until/if someone complains about performance. I doubt many (any?) people are doing timing critical writes in U-Boot. I think the argument that writes should be verified carries some water, and it'd be nice to not make the command arguments more complicated than they already are. What about DFU and other non-cmd_nand NAND accesses? Are there other non-cmd NAND accesses other than DFU? None jumped out at me. There's ubi/jffs2/yaffs2, which are responsible for their own verification, and a read-only access in compulab board code, but otherwise maybe not. The MTD interface is harder to grep for, though. I'm not too familiar with DFU, but in theory the DFU programming utilities could already be doing their own verification. I took a quick look at the dfu-util source, and it doesn't appear to be doing its own. I'd vote to verify the DFU writes too, since even more than nand write its performance shouldn't be very critical. I'll break DFU verification out into a separate patch, and you or others can ACK or reject it then. Let me know if the above sounds good and I'll make the changes. Fine with me. -Scott ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/5] nand: Use common read function instead of verify_buf()
Hi Scott, I waffled about removing it, but leaned towards leaving it in because: - I didn't want to change the existing U-Boot behavior for other users. A google of 'u-boot nand write' shows a lot of examples that don't include verification of writes, and they should if we remove auto-verification. How many configs actually enable this option? I don't see many beyond the FSL PPC boards (which are so full of copy-and-paste that it probably wasn't deliberate). Yeah, the majority are FSL 83xx and 85xx, with 2 or so random ARM boards. - The reason it was removed in Linux was Both UBI and JFFS2 are able to read verify what they wrote already. There are also MTD tests which do this verification. I thought U-Boot was more likely than Linux to use raw NAND writes without a filesystem, so leaving it in U- Boot made sense since the UBI/JFFS2 logic didn't apply as much here. Right, though raw writes ought to be limited to blocks that aren't written often enough to fail. - I didn't think a lot of people would know they have to explicitly verify NAND contents after a write, since they'd assume it was like other memories that aren't as lossy. - The penalty of slightly different code from Linux and a small performance hit was worth the gain of auto-verification to me. I viewed consolidating it into one small chunk of code as a happy medium. The davinci patches show that there can still be driver dependencies depending on what the driver overrides. I'm not hugely opposed, but it seems like it would be better to do it at a higher level (e.g. in nand_util.c with a flag to enable, and either make support mandatory, or if you try to use that command variant without support it fails rather than silently not verifying). That seems like a good idea. How about: - Remove all CONFIG_MTD_NAND_VERIFY_WRITE references - Add a new flag WITH_WR_VERIFY and have nand_write_skip_bad() in nand_util.c verify writes only when it is set. - Update the calls to nand_write_skip_bad() in cmd_nand.c to include the new WITH_WR_VERIFY flag. I'd vote to enable it for all boards, but let me know if you disagree. That would make all nand write commands verify writes, with the exception of nand write.raw. Any opinion on if this should also be verified? I only use it for development/testing, so don't have a strong opinion. Regards, Peter ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/5] nand: Use common read function instead of verify_buf()
On Mon, 2015-01-26 at 17:17 -0600, Peter Tyser wrote: On Mon, 2015-01-26 at 16:33 -0600, Scott Wood wrote: On Mon, 2015-01-26 at 16:24 -0600, Peter Tyser wrote: The driver-specific verify_buf() function can be replaced with the standard read_page_raw() function to verify writes. This will allow verify_buf() to be removed from individual drivers. verify_buf() is no longer supported in mainline Linux, so it is a pain to continue supporting. Any reason not to just remove this feature from U-Boot, as Linux has done? The Linux change for reference: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=657f28f8811c92724db10d18bbbec70d540147d6 I waffled about removing it, but leaned towards leaving it in because: - I didn't want to change the existing U-Boot behavior for other users. A google of 'u-boot nand write' shows a lot of examples that don't include verification of writes, and they should if we remove auto-verification. How many configs actually enable this option? I don't see many beyond the FSL PPC boards (which are so full of copy-and-paste that it probably wasn't deliberate). - The reason it was removed in Linux was Both UBI and JFFS2 are able to read verify what they wrote already. There are also MTD tests which do this verification. I thought U-Boot was more likely than Linux to use raw NAND writes without a filesystem, so leaving it in U- Boot made sense since the UBI/JFFS2 logic didn't apply as much here. Right, though raw writes ought to be limited to blocks that aren't written often enough to fail. - I didn't think a lot of people would know they have to explicitly verify NAND contents after a write, since they'd assume it was like other memories that aren't as lossy. - The penalty of slightly different code from Linux and a small performance hit was worth the gain of auto-verification to me. I viewed consolidating it into one small chunk of code as a happy medium. The davinci patches show that there can still be driver dependencies depending on what the driver overrides. I'm not hugely opposed, but it seems like it would be better to do it at a higher level (e.g. in nand_util.c with a flag to enable, and either make support mandatory, or if you try to use that command variant without support it fails rather than silently not verifying). -Scott ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/5] nand: Use common read function instead of verify_buf()
On Mon, 2015-01-26 at 16:24 -0600, Peter Tyser wrote: The driver-specific verify_buf() function can be replaced with the standard read_page_raw() function to verify writes. This will allow verify_buf() to be removed from individual drivers. verify_buf() is no longer supported in mainline Linux, so it is a pain to continue supporting. Any reason not to just remove this feature from U-Boot, as Linux has done? -Scott ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/5] nand: Use common read function instead of verify_buf()
On Mon, 2015-01-26 at 16:33 -0600, Scott Wood wrote: On Mon, 2015-01-26 at 16:24 -0600, Peter Tyser wrote: The driver-specific verify_buf() function can be replaced with the standard read_page_raw() function to verify writes. This will allow verify_buf() to be removed from individual drivers. verify_buf() is no longer supported in mainline Linux, so it is a pain to continue supporting. Any reason not to just remove this feature from U-Boot, as Linux has done? The Linux change for reference: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=657f28f8811c92724db10d18bbbec70d540147d6 I waffled about removing it, but leaned towards leaving it in because: - I didn't want to change the existing U-Boot behavior for other users. A google of 'u-boot nand write' shows a lot of examples that don't include verification of writes, and they should if we remove auto-verification. - The reason it was removed in Linux was Both UBI and JFFS2 are able to read verify what they wrote already. There are also MTD tests which do this verification. I thought U-Boot was more likely than Linux to use raw NAND writes without a filesystem, so leaving it in U- Boot made sense since the UBI/JFFS2 logic didn't apply as much here. - I didn't think a lot of people would know they have to explicitly verify NAND contents after a write, since they'd assume it was like other memories that aren't as lossy. - The penalty of slightly different code from Linux and a small performance hit was worth the gain of auto-verification to me. I viewed consolidating it into one small chunk of code as a happy medium. The philosophical side of me said to remove it altogether, so I can see that perspective too. Peter ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH 1/5] nand: Use common read function instead of verify_buf()
The driver-specific verify_buf() function can be replaced with the
standard read_page_raw() function to verify writes. This will allow
verify_buf() to be removed from individual drivers. verify_buf() is no
longer supported in mainline Linux, so it is a pain to continue
supporting.
Signed-off-by: Peter Tyser pty...@xes-inc.com
---
drivers/mtd/nand/nand_base.c | 12 +++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 63bdf65..788846a 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -2394,6 +2394,7 @@ static int nand_write_page(struct mtd_info *mtd, struct
nand_chip *chip,
int oob_required, int page, int cached, int raw)
{
int status, subpage;
+ __maybe_unused uint8_t *vfy_buf;
if (!(chip-options NAND_NO_SUBPAGE_WRITE)
chip-ecc.write_subpage)
@@ -2443,10 +2444,19 @@ static int nand_write_page(struct mtd_info *mtd, struct
nand_chip *chip,
#ifdef __UBOOT__
#if defined(CONFIG_MTD_NAND_VERIFY_WRITE)
+ vfy_buf = malloc(mtd-writesize);
+ if (!vfy_buf)
+ return -ENOMEM;
+
/* Send command to read back the data */
chip-cmdfunc(mtd, NAND_CMD_READ0, 0, page);
+ chip-ecc.read_page_raw(mtd, chip, vfy_buf, oob_required, page);
+
+ status = memcmp(buf, vfy_buf, mtd-writesize);
- if (chip-verify_buf(mtd, buf, mtd-writesize))
+ free(vfy_buf);
+
+ if (status)
return -EIO;
/* Make sure the next page prog is preceded by a status read */
--
1.9.1
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
