| Message ID | 1536317783-4942-1-git-send-email-yamada.masahiro@socionext.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Miquel Raynal |
| Headers | show |
| Series | mtd: rawnand: denali: add DT property to specify skipped bytes in OOB | expand |
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
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/
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.
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.
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
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); }
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).
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
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;
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(-)