From: Miquel Raynal <miquel.raynal@bootlin.com>
To: 赵仪峰 <yifeng.zhao@rock-chips.com>
Cc: "Johan Jonker" <jbx6244@gmail.com>, richard <richard@nod.at>,
vigneshr <vigneshr@ti.com>, robh+dt <robh+dt@kernel.org>,
devicetree <devicetree@vger.kernel.org>,
HeikoStübner <heiko@sntech.de>,
linux-kernel <linux-kernel@vger.kernel.org>,
linux-rockchip <linux-rockchip@lists.infradead.org>,
linux-mtd <linux-mtd@lists.infradead.org>,
linux-arm-kernel <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH v6 2/8] mtd: rawnand: rockchip: NFC drivers for RK3308, RK2928 and others
Date: Mon, 15 Jun 2020 12:18:41 +0200 [thread overview]
Message-ID: <20200615121841.566b81f5@xps13> (raw)
In-Reply-To: <2020061517300662418531@rock-chips.com>
Hi 赵仪峰,
赵仪峰 <yifeng.zhao@rock-chips.com> wrote on Mon, 15 Jun 2020 17:34:14
+0800:
> Hi Johan,
>
> Johan Jonker <jbx6244@gmail.com> wrote on Sat, 13 Jun 2020 15:31:52
> +0200:
> >Hi Yifeng, Miquel,
> >
> >Some more comments about swap();
> >
> >On 6/9/20 9:40 AM, Yifeng Zhao wrote:
> >
> >[..]
> >
> >> +static int rk_nfc_ooblayout_free(struct mtd_info *mtd, int section,
> >> + struct mtd_oob_region *oob_region)
> >> +{
> >> + struct nand_chip *chip = mtd_to_nand(mtd);
> >> +
> >
> >> + if (section >= chip->ecc.steps)
> >> + return -ERANGE;
> >
> >Given:
> >
> >NFC_SYS_DATA_SIZE = 4
> >chip->ecc.steps = 8
> >section [0..7]
> >
> >Total free OOB size advertised to the MTD framework is:
> >
> >ecc.steps * NFC_SYS_DATA_SIZE - 1 BBM
> >8 * 4 - 1 = 31 bytes
> >
> >With link address in OOB byte [0..3] this become:
> >31 - 4 = 27 bytes
> >
> >Does that give data lost?
> >Should we limit the number of free OOB bytes by 4 more to be save?
> >Is my calculation correct?
> >See further questions about this below.
> >
> >> +
> >> + if (!section) {
> >> + /* The first byte is bad block mask flag. */
> >> + oob_region->length = NFC_SYS_DATA_SIZE - 1;
> >> + oob_region->offset = 1;
> >> + } else {
> >> + oob_region->length = NFC_SYS_DATA_SIZE;
> >> + oob_region->offset = section * NFC_SYS_DATA_SIZE;
> >> + }
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int rk_nfc_ooblayout_ecc(struct mtd_info *mtd, int section,
> >> + struct mtd_oob_region *oob_region)
> >> +{
> >> + struct nand_chip *chip = mtd_to_nand(mtd);
> >> +
> >
> >> + if (section)
> >> + return -ERANGE;
> >
> >With the formule above a section > 0 does not alow ECC.
> >
> >Just a question about the MTD inner working for Miquel:
> >
> >With ooblayout_free using 8 steps and this just 1 does it still generate
> >the correct ECC? Does it calculate ECC over 1024B or over 8*1024B ?
> >
> >Should we move the text that explains the layout closer to these
> >functions and add a little more text to explain why we choose this
> >particular layout?
> >
> >/*
> > * NFC Page Data Layout:
> > * 1024 Bytes Data + 4Bytes sys data + 28Bytes~124Bytes ecc +
> > * 1024 Bytes Data + 4Bytes sys data + 28Bytes~124Bytes ecc +
> > * ......
> > * NAND Page Data Layout:
> > * 1024 * n Data + m Bytes oob
> > * Original Bad Block Mask Location:
> > * First byte of oob(spare).
> > * nand_chip->oob_poi data layout:
> > * 4Bytes sys data + .... + 4Bytes sys data + ecc data.
> > */
> >
> >We expect now ECC data after n steps * 4 OOB bytes,
> >but are we still using it with HW ECC or only for raw?
> >
> >> +
> >> + oob_region->offset = NFC_SYS_DATA_SIZE * chip->ecc.steps;
> >> + oob_region->length = mtd->oobsize - oob_region->offset;
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static const struct mtd_ooblayout_ops rk_nfc_ooblayout_ops = {
> >> + .free = rk_nfc_ooblayout_free,
> >> + .ecc = rk_nfc_ooblayout_ecc,
> >> +};
> >
> >[..]
> >
> >> +static int rk_nfc_write_page(struct mtd_info *mtd, struct nand_chip *chip,
> >> + const u8 *buf, int page, int raw)
> >> +{
> >> + struct rk_nfc *nfc = nand_get_controller_data(chip);
> >> + struct rk_nfc_nand_chip *rk_nand = to_rk_nand(chip);
> >> + struct nand_ecc_ctrl *ecc = &chip->ecc;
> >> + int oob_step = (ecc->bytes > 60) ? NFC_MAX_OOB_PER_STEP :
> >> + NFC_MIN_OOB_PER_STEP;
> >> + int pages_per_blk = mtd->erasesize / mtd->writesize;
> >> + int ret = 0, i, boot_rom_mode = 0;
> >> + dma_addr_t dma_data, dma_oob;
> >> + u32 reg;
> >> + u8 *oob;
> >> +
> >> + nand_prog_page_begin_op(chip, page, 0, NULL, 0);
> >> +
> >> + if (!raw) {
> >> + memcpy(nfc->page_buf, buf, mtd->writesize);
> >> + memset(nfc->oob_buf, 0xff, oob_step * ecc->steps);
> >> +
> >> + /*
> >> + * The first 8(some devices are 4 or 16) blocks in use by
> >> + * the boot ROM and the first 32 bits of oob need to link
> >> + * to the next page address in the same block.
> >> + * Config the ECC algorithm supported by the boot ROM.
> >> + */
> >> + if (page < pages_per_blk * rk_nand->boot_blks &&
> >> + chip->options & NAND_IS_BOOT_MEDIUM) {
> >> + boot_rom_mode = 1;
> >> + if (rk_nand->boot_ecc != ecc->strength)
> >> + rk_nfc_hw_ecc_setup(chip, ecc,
> >> + rk_nand->boot_ecc);
> >> + }
> >> +
> >> + /*
> >> + * Swap the first oob with the seventh oob and bad block
> >> + * mask is saved at the seventh oob.
> >> + */
> >> + swap(chip->oob_poi[0], chip->oob_poi[7]);
> >
> >Add more info on why this is swapped.
> >
> >LA[0..3] is a link address that the BBM shouldn't over write.
> >For Yifeng: Is there an other reason?
>
> No other reason,this swap ops only for the link address.
>
> >Before swap:
> >
> >BBM OOB1 OOB2 OOB3, OOB4 OOB5 OOB6 OOB7, OOB8 ....
> >
> >After swap:
> >
> >OOB7 OOB1 OOB2 OOB3, OOB4 OOB5 OOB6 BBM , OOB8 ....
> >
> >If (!i && boot_rom_mode):
> >
> >LA0 LA1 LA2 LA3 , OOB4 OOB5 OOB6 BBM , OOB8 ....
> >
> >Read back after swap again:
> >
> >BBM LA1 LA2 LA3 , OOB4 OOB5 OOB6 LA0 , OOB8 ....
> >
> >Question:
> >Are data OOB7 OOB1 OOB2 OOB3 lost now?
> >Is this correct?
>
> Yes, the data OOB7 OOB1 OOB2 OOB3 will lost in the blocks which used for the boot ROM.
>
> >#################################################
> >Proposal:
> >Should we reduce the free OOB size by 4
> >and shift everything 4 bytes to recover all bytes?
> >Replace the first 4 bytes with 0XFF or LA[0..3].
> >
> >Normal:
> >0xFF 0XFF 0XFF 0xFF, BBM OOB1 OOB2 OOB3, OOB4
> >
> >If (!i && boot_rom_mode):
> >LA0 LA1 LA2 LA3 , BBM OOB1 OOB2 OOB3, OOB4
> >
> >Question for Miquel and Yifeng:
> >Does this work? Could you test?
>
> I want to modify the drivers in next version:
> The data swap ops only done for the blocks which used for the boot ROM,In this way,
> the specially processed code will not affect the rest blocks.
> For Miquel and Yifeng:
> Is this OK?
So I guess this linking property is only for the BootROM? I am not sure
what is best but I guess keeping the same layout everywhere is better.
Johan proposal would be good to try.
Thanks,
Miquèl
next prev parent reply other threads:[~2020-06-15 10:18 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-06-09 7:40 [PATCH v6 0/8] Add Rockchip NFC drivers for RK3308 and others Yifeng Zhao
2020-06-09 7:40 ` [PATCH v6 1/8] dt-bindings: mtd: Describe Rockchip RK3xxx NAND flash controller Yifeng Zhao
2020-06-10 10:50 ` Johan Jonker
2020-06-17 21:14 ` Rob Herring
2020-06-17 21:15 ` Rob Herring
2020-06-09 7:40 ` [PATCH v6 2/8] mtd: rawnand: rockchip: NFC drivers for RK3308, RK2928 and others Yifeng Zhao
2020-06-09 9:23 ` kernel test robot
2020-06-09 14:48 ` kernel test robot
2020-06-09 16:10 ` Johan Jonker
2020-06-09 20:18 ` Johan Jonker
2020-06-11 7:53 ` Miquel Raynal
2020-06-13 13:31 ` Johan Jonker
2020-06-15 7:47 ` Miquel Raynal
2020-06-15 9:34 ` 赵仪峰
2020-06-15 10:18 ` Miquel Raynal [this message]
2020-06-09 7:40 ` [PATCH v6 3/8] MAINTAINERS: add maintainers to rockchip nfc Yifeng Zhao
2020-06-09 7:47 ` [PATCH v6 4/8] arm64: dts: rockchip: Add nfc dts for RK3308 SOC Yifeng Zhao
2020-06-09 7:49 ` [PATCH v6 5/8] arm64: dts: rockchip: Add nfc dts for PX30 SOC Yifeng Zhao
2020-06-09 7:49 ` [PATCH v6 6/8] arm: dts: rockchip: Add nfc dts for RV1108 SOC Yifeng Zhao
2020-06-09 7:49 ` [PATCH v6 7/8] arm: dts: rockchip: Add nfc dts for RK2928 and other SOC Yifeng Zhao
2020-06-09 7:49 ` [PATCH v6 8/8] arm: dts: rockchip: Add nfc dts for RK3036 SOC Yifeng Zhao
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200615121841.566b81f5@xps13 \
--to=miquel.raynal@bootlin.com \
--cc=devicetree@vger.kernel.org \
--cc=heiko@sntech.de \
--cc=jbx6244@gmail.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mtd@lists.infradead.org \
--cc=linux-rockchip@lists.infradead.org \
--cc=richard@nod.at \
--cc=robh+dt@kernel.org \
--cc=vigneshr@ti.com \
--cc=yifeng.zhao@rock-chips.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).