From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail.bootlin.com ([62.4.15.54]) by bombadil.infradead.org with esmtp (Exim 4.90_1 #2 (Red Hat Linux)) id 1g42ol-0000Q2-QL for linux-mtd@lists.infradead.org; Sun, 23 Sep 2018 11:44:54 +0000 Date: Sun, 23 Sep 2018 13:44:37 +0200 From: Miquel Raynal To: Masahiro Yamada Cc: Boris Brezillon , Mark Rutland , DTML , Dinh Nguyen , Richard Weinberger , Linux Kernel Mailing List , Rob Herring , linux-mtd , Brian Norris , David Woodhouse , Marek Vasut Subject: Re: [PATCH] mtd: rawnand: denali: add DT property to specify skipped bytes in OOB Message-ID: <20180923134437.283b5dda@xps13> In-Reply-To: References: <1536317783-4942-1-git-send-email-yamada.masahiro@socionext.com> <20180907160822.319047c8@bbrezillon> <20180907165348.3e0027ee@bbrezillon> <20180922094111.1c2969e8@xps13> <20180922101116.7c9b4767@bbrezillon> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Hi Masahiro, Masahiro Yamada wrote on Sun, 23 Sep 2018 06:38:40 -0400: > 2018-09-22 4:11 GMT-04:00 Boris Brezillon : > > On Sat, 22 Sep 2018 09:41:11 +0200 > > Miquel Raynal wrote: > > =20 > >> Hi Masahiro, > >> > >> Masahiro Yamada wrote on Sat, 8 Sep > >> 2018 01:10:25 +0900: > >> =20 > >> > Hi Boris, > >> > > >> > 2018-09-07 23:53 GMT+09:00 Boris Brezillon : =20 > >> > > On Fri, 7 Sep 2018 23:42:53 +0900 > >> > > Masahiro Yamada wrote: > >> > > =20 > >> > >> Hi Boris, > >> > >> > >> > >> 2018-09-07 23:08 GMT+09:00 Boris Brezillon : =20 > >> > >> > Hi Masahiro, > >> > >> > > >> > >> > On Fri, 7 Sep 2018 19:56:23 +0900 > >> > >> > Masahiro Yamada wrote: > >> > >> > =20 > >> > >> >> NAND devices need additional data area (OOB) for error correct= ion, > >> > >> >> 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 d= epends > >> > >> >> on chip vendors. The NAND controller should preserve the prec= ious > >> > >> >> BBM to keep track of bad blocks. > >> > >> >> > >> > >> >> In Denali IP, the SPARE_AREA_SKIP_BYTES register is used to sp= ecify > >> > >> >> the number of bytes to skip from the start of OOB. The ECC en= gine > >> > >> >> 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 al= ready > >> > >> >> set the SPARE_AREA_SKIP_BYTES register, then reads the value o= ut. > >> > >> >> > >> > >> >> If no firmware (or bootloader) has initialized the controller,= the > >> > >> >> register value is zero, which is the default after power-on-re= set. > >> > >> >> > >> > >> >> In other words, the Linux driver cannot initialize the control= ler > >> > >> >> 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 r= egister. =20 > >> > >> > > >> > >> > 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 b= ytes > >> > >> > 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 diff= erently? > >> > >> > Don't you always have SPARE_AREA_SKIP_BYTES set to 6 or 2? =20 > >> > >> > >> > >> > >> > >> 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. =20 > > > > 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 =3D ioread32(denali->reg + > > SPARE_AREA_SKIP_BYTES); > > if (!denali->oob_skip_bytes) { > > denali->oob_skip_bytes =3D DEFAULT_OOB_SKIP_BYTES; > > iowrite32(denali->oob_skip_bytes, > > denali->reg + SPARE_AREA_SKIP_BYTES); > > } > > =20 >=20 >=20 > I prefer per-compatible values to a fixed default. >=20 >=20 > 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. >=20 > Other platforms like Altera SOCFPGA may want to use a different value > (at least, I do not know what is the preferred value). >=20 >=20 Well, for now we can just have a default to 8, and if someone complains have a per-compatible default? Thanks, Miqu=C3=A8l