From: Bruce Suen <bruce_suen@163.com>
To: Miquel Raynal <miquel.raynal@bootlin.com>
Cc: linux-mtd@lists.infradead.org, richard@nod.at, vigneshr@ti.com
Subject: Re: [PATCH] mtd: spinand: Add support for XTX XT26xxxDxxxxx
Date: Tue, 10 Oct 2023 23:09:11 -0400 [thread overview]
Message-ID: <c8dace54-bb25-957f-d7b3-8c705b480cdc@163.com> (raw)
In-Reply-To: <20231009145839.0651dd29@xps-13>
Hi Miquel,
Thanks for your review.
On 10/9/23 08:58, Miquel Raynal wrote:
> Hi Bruce,
>
> bruce_suen@163.com wrote on Sat, 7 Oct 2023 05:09:54 -0400:
>
>> Add Support XTX Technology XT26G01DXXXXX, XT26G11DXXXXX, XT26Q01DXXXXX,
>> XT26G02DXXXXX, XT26G12DXXXXX, XT26Q02DXXXXX, XT26G04DXXXXX, and
>> XT26Q04DXXXXX SPI NAND.
>>
>> These are 3V/1.8V 1G/2G/4Gbit serial SLC NAND flash device with on-die
>> ECC(8bit strength per 512bytes).
>>
>> Datasheet Links:
>> - http://www.xtxtech.com/download/?AId=458
>> - http://www.xtxtech.com/download/?AId=495
>>
>> Signed-off-by: Bruce Suen <bruce_suen@163.com>
>> ---
>> drivers/mtd/nand/spi/xtx.c | 135 +++++++++++++++++++++++++++++++++++++
>> 1 file changed, 135 insertions(+)
>>
>> diff --git a/drivers/mtd/nand/spi/xtx.c b/drivers/mtd/nand/spi/xtx.c
>> index 3911520f7..ddc69ef76 100644
>> --- a/drivers/mtd/nand/spi/xtx.c
>> +++ b/drivers/mtd/nand/spi/xtx.c
>> @@ -15,6 +15,13 @@
>> #define XT26G0XA_STATUS_ECC_8_CORRECTED (3 << 4)
>> #define XT26G0XA_STATUS_ECC_UNCOR_ERROR (2 << 4)
>>
>> +#define XT26XXXD_STATUS_ECC_MASK GENMASK(7, 4)
>> +#define XT26XXXD_STATUS_ECC_NO_DETECTED (0 << 4)
>> +#define XT26XXXD_STATUS_ECC_1_7_CORRECTED (1 << 4)
>> +#define XT26XXXD_STATUS_ECC_8_CORRECTED (3 << 4)
>> +#define XT26XXXD_STATUS_ECC_UNCOR_ERROR (2 << 4)
>> +
>> static SPINAND_OP_VARIANTS(read_cache_variants,
>> SPINAND_PAGE_READ_FROM_CACHE_QUADIO_OP(0, 1, NULL, 0),
>> SPINAND_PAGE_READ_FROM_CACHE_X4_OP(0, 1, NULL, 0),
>> @@ -84,6 +91,54 @@ static int xt26g0xa_ecc_get_status(struct spinand_device *spinand,
>> return status >> 2;
>> }
>>
>> +
>> +static int xt26xxxd_ooblayout_ecc(struct mtd_info *mtd, int section,
>> + struct mtd_oob_region *region)
>> +{
>> + if (section)
>> + return -ERANGE;
>> +
>> + region->offset = mtd->oobsize / 2;
>> + region->length = mtd->oobsize / 2;
> This is strange, are you sure this is always the case? Isn't the offset
> related to the number of bytes used by the ECC engine strength rather
> than a position in oobsize?
I'm sure for this,for XT26XXXD, the half of oob cannot be used by user
although ecc engine does not use up half of the oob.
>
>> +
>> + return 0;
>> +}
>> +
>> +static int xt26xxxd_ooblayout_free(struct mtd_info *mtd, int section,
>> + struct mtd_oob_region *region)
>> +{
>> + if (section)
>> + return -ERANGE;
>> +
>> + region->offset = 1;
> Please reserve 2 bytes for the BBM.
I'm confused about this.I see some drivers reserve 1 byte for BBM, some
drivers reserve 2 bytes for BBM.
I can reserve 2 bytes for the BBM in next version of patch.
>
>> + region->length = mtd->oobsize / 2 - 1;
> "- 2"
>
>> +
>> + return 0;
>> +}
>> +
>> +static const struct mtd_ooblayout_ops xt26xxxd_ooblayout = {
>> + .ecc = xt26xxxd_ooblayout_ecc,
>> + .free = xt26xxxd_ooblayout_free,
>> +};
>> +
>> +static int xt26xxxd_ecc_get_status(struct spinand_device *spinand,
>> + u8 status)
>> +{
>> + switch (status & STATUS_ECC_MASK) {
>> + case XT26XXXD_STATUS_ECC_NO_DETECTED:
>> + return 0;
>> + case XT26XXXD_STATUS_ECC_UNCOR_ERROR:
>> + return -EBADMSG;
>> + case XT26XXXD_STATUS_ECC_1_7_CORRECTED:
>> + return 4+((status & XT26XXXD_STATUS_ECC_MASK) >> 6);
> Can you justify the calculation?
AS described on page 42 of datasheet:
"ECCS3/ECCS2/ECCS1/ECCS0 provides ECC Status as follows:
XX00b= No bit errors were detected during the previous read algorithm.
0001b= Bit errors(≤4) were detected and corrected.
0101b= Bit errors(=5) were detected and corrected.
1001b= Bit errors(=6) were detected and corrected.
1101b= Bit errors(=7) were detected and corrected.
XX10b= Bit errors greater than ECC capability (8 bits) and not corrected
XX11b= Bit errors reach ECC capability (8 bits) and corrected"
ECCS1,ECCS0 = 00: No bit errors
ECCS1,ECCS0 = 10: bit errors > 8
ECCS1,ECCS0 = 11: bit errors = 8
ECCS1,ECCS0 = 01: 1<=bit errors<=7
so I use 4 cases to represent it.
for ECCS1,ECCS0 = 11, bitfilps can be subdivided by ECCS3 ECCS2:
ECCS3 ECCS2 = 00: bitfip<=4=4+0, I return 4 for this;
ECCS3 ECCS2 = 01: bitfip=5=4+1, I return 5 for this;
ECCS3 ECCS2 = 10: bitfip=6=4+2, I return 6 for this;
ECCS3 ECCS2 = 11: bitfip=7=4+3, I return 7 for this;
so,I use "4+((status & XT26XXXD_STATUS_ECC_MASK) >> 6)" to represent it.
>
> Also please run checkpatch.pl --strict.
I run checkpatch.pl without --strict,there are no checks.
I run checkpatch.pl --strict,there are 5 checks.
I'm sorry for that,I will fix it in next version of patch.
>> + case XT26XXXD_STATUS_ECC_8_CORRECTED:
>> + return 8;
>> + default:
>> + break;
>> + }
>> +
>> + return -EINVAL;
>> +}
>> static const struct spinand_info xtx_spinand_table[] = {
>> SPINAND_INFO("XT26G01A",
>> SPINAND_ID(SPINAND_READID_METHOD_OPCODE_ADDR, 0xE1),
>> @@ -115,6 +170,86 @@ static const struct spinand_info xtx_spinand_table[] = {
>> SPINAND_HAS_QE_BIT,
>> SPINAND_ECCINFO(&xt26g0xa_ooblayout,
>> xt26g0xa_ecc_get_status)),
>> + SPINAND_INFO("XT26G01D",
>> + SPINAND_ID(SPINAND_READID_METHOD_OPCODE_ADDR, 0x31),
>> + NAND_MEMORG(1, 2048, 128, 64, 1024, 20, 1, 1, 1),
>> + NAND_ECCREQ(8, 512),
>> + SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
>> + &write_cache_variants,
>> + &update_cache_variants),
>> + 0,
>> + SPINAND_ECCINFO(&xt26xxxd_ooblayout,
>> + xt26xxxd_ecc_get_status)),
>> + SPINAND_INFO("XT26G11D",
>> + SPINAND_ID(SPINAND_READID_METHOD_OPCODE_ADDR, 0x34),
>> + NAND_MEMORG(1, 2048, 128, 64, 1024, 20, 1, 1, 1),
>> + NAND_ECCREQ(8, 512),
>> + SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
>> + &write_cache_variants,
>> + &update_cache_variants),
>> + SPINAND_HAS_QE_BIT,
>> + SPINAND_ECCINFO(&xt26xxxd_ooblayout,
>> + xt26xxxd_ecc_get_status)),
>> + SPINAND_INFO("XT26Q01D",
>> + SPINAND_ID(SPINAND_READID_METHOD_OPCODE_ADDR, 0x51),
>> + NAND_MEMORG(1, 2048, 128, 64, 1024, 20, 1, 1, 1),
>> + NAND_ECCREQ(8, 512),
>> + SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
>> + &write_cache_variants,
>> + &update_cache_variants),
>> + 0,
>> + SPINAND_ECCINFO(&xt26xxxd_ooblayout,
>> + xt26xxxd_ecc_get_status)),
>> + SPINAND_INFO("XT26G02D",
>> + SPINAND_ID(SPINAND_READID_METHOD_OPCODE_ADDR, 0x32),
>> + NAND_MEMORG(1, 2048, 128, 64, 2048, 40, 1, 1, 1),
>> + NAND_ECCREQ(8, 512),
>> + SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
>> + &write_cache_variants,
>> + &update_cache_variants),
>> + 0,
>> + SPINAND_ECCINFO(&xt26xxxd_ooblayout,
>> + xt26xxxd_ecc_get_status)),
>> + SPINAND_INFO("XT26G12D",
>> + SPINAND_ID(SPINAND_READID_METHOD_OPCODE_ADDR, 0x35),
>> + NAND_MEMORG(1, 2048, 128, 64, 2048, 40, 1, 1, 1),
>> + NAND_ECCREQ(8, 512),
>> + SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
>> + &write_cache_variants,
>> + &update_cache_variants),
>> + SPINAND_HAS_QE_BIT,
>> + SPINAND_ECCINFO(&xt26xxxd_ooblayout,
>> + xt26xxxd_ecc_get_status)),
>> + SPINAND_INFO("XT26Q02D",
>> + SPINAND_ID(SPINAND_READID_METHOD_OPCODE_ADDR, 0x52),
>> + NAND_MEMORG(1, 2048, 128, 64, 2048, 40, 1, 1, 1),
>> + NAND_ECCREQ(8, 512),
>> + SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
>> + &write_cache_variants,
>> + &update_cache_variants),
>> + 0,
>> + SPINAND_ECCINFO(&xt26xxxd_ooblayout,
>> + xt26xxxd_ecc_get_status)),
>> + SPINAND_INFO("XT26G04D",
>> + SPINAND_ID(SPINAND_READID_METHOD_OPCODE_ADDR, 0x33),
>> + NAND_MEMORG(1, 4096, 256, 64, 2048, 40, 1, 1, 1),
>> + NAND_ECCREQ(8, 512),
>> + SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
>> + &write_cache_variants,
>> + &update_cache_variants),
>> + 0,
>> + SPINAND_ECCINFO(&xt26xxxd_ooblayout,
>> + xt26xxxd_ecc_get_status)),
>> + SPINAND_INFO("XT26Q04D",
>> + SPINAND_ID(SPINAND_READID_METHOD_OPCODE_ADDR, 0x53),
>> + NAND_MEMORG(1, 4096, 256, 64, 2048, 40, 1, 1, 1),
>> + NAND_ECCREQ(8, 512),
>> + SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
>> + &write_cache_variants,
>> + &update_cache_variants),
>> + 0,
>> + SPINAND_ECCINFO(&xt26xxxd_ooblayout,
>> + xt26xxxd_ecc_get_status)),
>> };
>>
>> static const struct spinand_manufacturer_ops xtx_spinand_manuf_ops = {
>
> Thanks,
> Miquèl
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
next prev parent reply other threads:[~2023-10-11 3:09 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-10-07 9:09 [PATCH] mtd: spinand: Add support for XTX XT26xxxDxxxxx Bruce Suen
2023-10-09 12:58 ` Miquel Raynal
2023-10-11 3:09 ` Bruce Suen [this message]
2023-10-11 7:09 ` Miquel Raynal
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=c8dace54-bb25-957f-d7b3-8c705b480cdc@163.com \
--to=bruce_suen@163.com \
--cc=linux-mtd@lists.infradead.org \
--cc=miquel.raynal@bootlin.com \
--cc=richard@nod.at \
--cc=vigneshr@ti.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