by Md Sadre Alam

[permalink] [raw]

Subject: Re: [PATCH v6 6/8] spi: spi-qpic: add driver for QCOM SPI NAND flash Interface

On 5/22/2024 6:03 PM, Miquel Raynal wrote:
> Hi,
>
>>>> +static int qcom_spi_ooblayout_ecc(struct mtd_info *mtd, int section,
>>>> + struct mtd_oob_region *oobregion)
>>>> +{
>>>> + struct nand_device *nand = mtd_to_nanddev(mtd);
>>>> + struct qcom_nand_controller *snandc = nand_to_qcom_snand(nand);
>>>> + struct qpic_ecc *qecc = snandc->qspi->ecc;
>>>> +
>>>> + if (section > 1)
>>>> + return -ERANGE;
>>>> +
>>>> + if (!section) {
>>>> + oobregion->length = (qecc->bytes * (qecc->steps - 1)) + qecc->bbm_size;
>>>> + oobregion->offset = 0;
>>>
>>> No, offset 0 is for the BBM. This is wrong.
>>> The whole oob layout looks really really wrong.
>>>
>>> ECC bytes are where the ECC engine puts its bytes in the OOB area.
>>> Free bytes start after the BBM and fill the gaps until the end of the
>>> area, except where there are ECC bytes.
>> QPIC NAND controller having its own page layout with ecc and without ecc.
>> The same layout we are using in raw nand driver as well, so i used the
>> same here. The below info is already there in qcom raw nand driver file
>> in page layout info.
>>
>> QPIC NAND controller layout as below:
>>
>> Layout with ECC enabled:
>>
>> |----------------------| |---------------------------------|
>> | xx.......yy| | *********xx.......yy|
>> | DATA xx..ECC..yy| | DATA **SPARE**xx..ECC..yy|
>> | (516) xx.......yy| | (516-n*4) **(n*4)**xx.......yy|
>> | xx.......yy| | *********xx.......yy|
>> |----------------------| |---------------------------------|
>> codeword 1,2..n-1 codeword n
>> <---(528/532 Bytes)--> <-------(528/532 Bytes)--------->
>>
>> n = Number of codewords in the page
>> . = ECC bytes
>> * = Spare/free bytes
>> x = Unused byte(s)
>> y = Reserved byte(s)
>>
>> 2K page: n = 4, spare = 16 bytes
>> 4K page: n = 8, spare = 32 bytes
>> 8K page: n = 16, spare = 64 bytes
>>
>> the qcom nand controller operates at a sub page/codeword level. each
>> codeword is 528 and 532 bytes for 4 bit and 8 bit ECC modes respectively.
>> the number of ECC bytes vary based on the ECC strength and the bus width.
>>
>> the first n - 1 codewords contains 516 bytes of user data, the remaining
>> 12/16 bytes consist of ECC and reserved data. The nth codeword contains
>> both user data and spare(oobavail) bytes that sum up to 516 bytes.
>>
>> When we access a page with ECC enabled, the reserved bytes(s) are not
>> accessible at all. When reading, we fill up these unreadable positions
>> with 0xffs. When writing, the controller skips writing the inaccessible
>> bytes.
>>
>> Layout with ECC disabled:
>>
>> |------------------------------| |---------------------------------------|
>> | yy xx.......| | bb *********xx.......|
>> | DATA1 yy DATA2 xx..ECC..| | DATA1 bb DATA2 **SPARE**xx..ECC..|
>> | (size1) yy (size2) xx.......| | (size1) bb (size2) **(n*4)**xx.......|
>> | yy xx.......| | bb *********xx.......|
>> |------------------------------| |---------------------------------------|
>> codeword 1,2..n-1 codeword n
>> <-------(528/532 Bytes)------> <-----------(528/532 Bytes)----------->
>>
>> n = Number of codewords in the page
>> . = ECC bytes
>> * = Spare/free bytes
>> x = Unused byte(s)
>> y = Dummy Bad Bock byte(s)
>> b = Real Bad Block byte(s)
>> size1/size2 = function of codeword size and 'n'
>>
>> when the ECC block is disabled, one reserved byte (or two for 16 bit bus
>> width) is now accessible. For the first n - 1 codewords, these are dummy Bad
>> Block Markers. In the last codeword, this position contains the real BBM
>>
>> In order to have a consistent layout between RAW and ECC modes, we assume
>> the following OOB layout arrangement:
>>
>> |-----------| |--------------------|
>> |yyxx.......| |bb*********xx.......|
>> |yyxx..ECC..| |bb*FREEOOB*xx..ECC..|
>> |yyxx.......| |bb*********xx.......|
>> |yyxx.......| |bb*********xx.......|
>> |-----------| |--------------------|
>> first n - 1 nth OOB region
>> OOB regions
>>
>> n = Number of codewords in the page
>> . = ECC bytes
>> * = FREE OOB bytes
>> y = Dummy bad block byte(s) (inaccessible when ECC enabled)
>> x = Unused byte(s)
>> b = Real bad block byte(s) (inaccessible when ECC enabled)
>>
>> This layout is read as is when ECC is disabled. When ECC is enabled, the
>> inaccessible Bad Block byte(s) are ignored when we write to a page/oob,
>> and assumed as 0xffs when we read a page/oob. The ECC, unused and
>> dummy/real bad block bytes are grouped as ecc bytes (i.e, ecc->bytes is
>> the sum of the three).
>
> Thanks for the detailed explanation (which would benefit from being
> added somewhere in a comment, maybe at the top of the file).
Ok
>
> Unfortunately, these ooblayout callbacks do work on a flat <data><oob>
> layout, not on the hardware ECC engine layout. So whatever the real
> physical position of the bad block marker within the NAND array, these
> markers will always be at offset 0 and 1 in the OOB final buffer.
Ok , will fix in next patch.
>
> Same applies to the spare and ECC bytes. These layouts are totally
> wrong and must be fixed. If the layouts are the same in both raw/spi
> cases, maybe they should be part of the common file?
Ok , will fix in next patch.
>
>>>> + } else {
>>>> + oobregion->length = qecc->ecc_bytes_hw + qecc->spare_bytes;
>>>> + oobregion->offset = mtd->oobsize - oobregion->length;
>>>> + }
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int qcom_spi_ooblayout_free(struct mtd_info *mtd, int section,
>>>> + struct mtd_oob_region *oobregion)
>>>> +{
>>>> + struct nand_device *nand = mtd_to_nanddev(mtd);
>>>> + struct qcom_nand_controller *snandc = nand_to_qcom_snand(nand);
>>>> + struct qpic_ecc *qecc = snandc->qspi->ecc;
>>>> +
>>>> + if (section)
>>>> + return -ERANGE;
>>>> +
>>>> + oobregion->length = qecc->steps * 4;
>>>> + oobregion->offset = ((qecc->steps - 1) * qecc->bytes) + qecc->bbm_size;
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>
>>> ...
>>>
>>>> +static int qcom_spi_ecc_prepare_io_req_pipelined(struct nand_device *nand,
>>>> + struct nand_page_io_req *req)
>>>> +{
>>>> + struct qcom_nand_controller *snandc = nand_to_qcom_snand(nand);
>>>> + struct qpic_ecc *ecc_cfg = nand_to_ecc_ctx(nand);
>>>> + struct mtd_info *mtd = nanddev_to_mtd(nand);
>>>> +
>>>> + snandc->qspi->ecc = ecc_cfg;
>>>> + snandc->qspi->pagesize = mtd->writesize;
>>>> + snandc->qspi->raw_rw = false;
>>>> + snandc->qspi->oob_rw = false;
>>>> + snandc->qspi->page_rw = false;
>>>> +
>>>> + if (req->datalen)
>>>> + snandc->qspi->page_rw = true;
>>>> +
>>>> + if (req->ooblen) {
>>>> + snandc->qspi->oob_rw = true;
>>>> + if (req->ooblen == BAD_BLOCK_MARKER_SIZE)
>>>> + snandc->qspi->read_last_cw = true;
>>>
>>> ???
>> As per QPIC controller layout , the actual babd block marker will
>> be present in last code word. Thats why i have added this check.
>> to read only last codeword for bad block check.
>
> You need to comply with the request. If ooblen is != 0, you need to
> read the codeword(s) where the oob is. Please don't try to be smarter
> than that. Checking the _value_ of ooblen is an optimization I don't
> think is worth.
Ok, will try to cleanup all the indirection in next patch.
>
>>>
>>>> + }
>>>> +
>>>> + if (req->mode == MTD_OPS_RAW)
>>>> + snandc->qspi->raw_rw = true;
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int qcom_spi_ecc_finish_io_req_pipelined(struct nand_device *nand,
>>>> + struct nand_page_io_req *req)
>>>> +{
>>>> + struct qcom_nand_controller *snandc = nand_to_qcom_snand(nand);
>>>> + struct mtd_info *mtd = nanddev_to_mtd(nand);
>>>> +
>>>> + if (req->mode == MTD_OPS_RAW || req->type != NAND_PAGE_READ)
>>>> + return 0;
>>>> +
>>>> + if (snandc->qspi->ecc_stats.failed)
>>>> + mtd->ecc_stats.failed += snandc->qspi->ecc_stats.failed;
>>>> + mtd->ecc_stats.corrected += snandc->qspi->ecc_stats.corrected;
>>>
>>> Seems strange
>> In flash error check for each code word i am updating the error value.
>> So on finishing on io i am assigning that error to mtd variables so that
>> upper layer check for error.
>
> You don't clear the qspi ecc_stats so this cannot work properly.
I am clearing in the qcom_spi_check_error() api, before reading status for the next page.

snandc->qspi->ecc_stats.failed = 0;
snandc->qspi->ecc_stats.corrected = 0;
>
> Plus, I would welcome an else statement for incrementing the corrected
> field.
Ok
>
>>>
>>>> +
>>>> + if (snandc->qspi->ecc_stats.failed)
>>>> + return -EBADMSG;
>>>> + else
>>>> + return snandc->qspi->ecc_stats.bitflips;
>>>> +}
>>>> +
>>>> +static struct nand_ecc_engine_ops qcom_spi_ecc_engine_ops_pipelined = {
>>>> + .init_ctx = qcom_spi_ecc_init_ctx_pipelined,
>>>> + .cleanup_ctx = qcom_spi_ecc_cleanup_ctx_pipelined,
>>>> + .prepare_io_req = qcom_spi_ecc_prepare_io_req_pipelined,
>>>> + .finish_io_req = qcom_spi_ecc_finish_io_req_pipelined,
>>>> +};
>>>> +
>>>
>>> ...
>>>
>>>> +static int qcom_spi_read_page_raw(struct qcom_nand_controller *snandc,
>>>> + const struct spi_mem_op *op)
>>>> +{
>>>> + struct qpic_ecc *ecc_cfg = snandc->qspi->ecc;
>>>> + u8 *data_buf = NULL, *oob_buf = NULL;
>>>> + int ret, cw;
>>>> + u32 num_cw = snandc->qspi->num_cw;
>>>> +
>>>> + if (snandc->qspi->page_rw)
>>>
>>> I don't like this indirection very much. Can't you simplify this and
>>> just follow the spi-mem op instead of constantly trying to add
>>> additional stuff?
>> This indirection needed due to QPIC controller will not take all the instruction
>> one-by-one , once we will set CMD_EXEC = 1, then it will execute all the instruction
>> at once.
>
> The spi_mem_op structure already describes the whole operation. Why do
> you split the operation in sub routines if you can't actually do that?
Ok , will try to cleanup in next patch.
>
>>>
>>> The hardware is already quite complex, but it feels like your adding
>>> yet another pile of unnecessary complexity.
>> Yes hardware is complex. let me check if i can further optimize as per spi-mem op
>> as you suggested.
>>>
>>>> + data_buf = op->data.buf.in;
>>>> +
>>>> + if (snandc->qspi->oob_rw)
>>>> + oob_buf = op->data.buf.in;
>
> ...
>
>>>> +static int qcom_spi_write_page_cache(struct qcom_nand_controller *snandc,
>>>> + const struct spi_mem_op *op)
>>>> +{
>>>> + struct qpic_snand_op s_op = {};
>>>> + u32 cmd;
>>>> +
>>>> + cmd = qcom_spi_cmd_mapping(snandc, op->cmd.opcode);
>>>
>>> I've asked for switch cases to return an error in case they could not
>>> handle the request. If you don't check the returned values, it
>>> does not make any sense.
>> Ok, will fix in next patch.
>>>
>>>> + s_op.cmd_reg = cmd;
>>>> +
>>>> + if (op->cmd.opcode == SPINAND_PROGRAM_LOAD) {
>>>> + if (snandc->qspi->page_rw)
>>>> + snandc->qspi->data_buf = (u8 *)op->data.buf.out;
>>>
>>> What you do here does not write anything in a page cache.
>> No here just updating the buffer , actual write will happen in program_execute.
>> This is due to QPIC controller will not take all the instruction one-by-one.
>> once we will set CMD_EXEC = 1, then it will execute all the instruction
>> at once. So accumulating all the instruction and then executing at once in
>> program_execute.
>>>
>>> I also don't understand why you would have to check against the
>>> SPINAND_PROGRAM_LOAD opcode.
>> Because the actual write will happen in program_execute. and here
>> PROGRAM_EXECUTE command will also land, so that added the check.
>>>
>>>> + }
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int qcom_spi_send_cmdaddr(struct qcom_nand_controller *snandc,
>>>> + const struct spi_mem_op *op)
>>>> +{
>>>> + struct qpic_snand_op s_op = {};
>>>> + u32 cmd;
>>>> + int ret, opcode;
>>>> +
>>>> + cmd = qcom_spi_cmd_mapping(snandc, op->cmd.opcode);
>>>> +
>>>> + s_op.cmd_reg = cmd;
>>>> + s_op.addr1_reg = op->addr.val;
>>>> + s_op.addr2_reg = 0;
>>>> +
>>>> + opcode = op->cmd.opcode;
>>>> +
>>>> + switch (opcode) {
>>>> + case SPINAND_WRITE_EN:
>>>> + return 0;
>>>> + case SPINAND_PROGRAM_EXECUTE:
>>>> + s_op.addr1_reg = op->addr.val << 16;
>>>> + s_op.addr2_reg = op->addr.val >> 16 & 0xff;
>>>> + snandc->qspi->addr1 = s_op.addr1_reg;
>>>> + snandc->qspi->addr2 = s_op.addr2_reg;
>>>> + snandc->qspi->cmd = cmd;
>>>> + return qcom_spi_program_execute(snandc, op);
>>>> + case SPINAND_READ:
>>>> + s_op.addr1_reg = (op->addr.val << 16);
>>>> + s_op.addr2_reg = op->addr.val >> 16 & 0xff;
>>>> + snandc->qspi->addr1 = s_op.addr1_reg;
>>>> + snandc->qspi->addr2 = s_op.addr2_reg;
>>>> + snandc->qspi->cmd = cmd;
>>>> + return 0;
>>>> + case SPINAND_ERASE:
>>>> + s_op.addr2_reg = (op->addr.val >> 16) & 0xffff;
>>>> + s_op.addr1_reg = op->addr.val;
>>>> + snandc->qspi->addr1 = (s_op.addr1_reg << 16);
>>>> + snandc->qspi->addr2 = s_op.addr2_reg;
>>>> + snandc->qspi->cmd = cmd;
>>>> + qcom_spi_block_erase(snandc);
>>>> + return 0;
>>>> + default:
>>>> + break;
>>>> + }
>>>> +
>>>> + snandc->buf_count = 0;
>>>> + snandc->buf_start = 0;
>>>> + qcom_clear_read_regs(snandc);
>>>> + qcom_clear_bam_transaction(snandc);
>>>> +
>>>> + snandc->regs->cmd = s_op.cmd_reg;
>>>> + snandc->regs->exec = 1;
>>>> + snandc->regs->addr0 = s_op.addr1_reg;
>>>> + snandc->regs->addr1 = s_op.addr2_reg;
>>>> +
>>>> + qcom_write_reg_dma(snandc, &snandc->regs->cmd, NAND_FLASH_CMD, 3, NAND_BAM_NEXT_SGL);
>>>> + qcom_write_reg_dma(snandc, &snandc->regs->exec, NAND_EXEC_CMD, 1, NAND_BAM_NEXT_SGL);
>>>> +
>>>> + ret = qcom_submit_descs(snandc);
>
> And you really don't want to check the validity of the opcode with what
> you support before submitting the descriptors?
Ok , will do in next patch.
>
>>>> + if (ret)
>>>> + dev_err(snandc->dev, "failure in sbumitting cmd descriptor\n");
>>>
>>> typo
>> Ok , will fix in next patch.
>>>
>>>> +
>>>> + return ret;
>>>> +}
>>>> +
>>>> +static int qcom_spi_io_op(struct qcom_nand_controller *snandc, const struct spi_mem_op *op)
>>>> +{
>>>> + int ret, val, opcode;
>>>> + bool copy = false, copy_ftr = false;
>>>> +
>>>> + ret = qcom_spi_send_cmdaddr(snandc, op);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + snandc->buf_count = 0;
>>>> + snandc->buf_start = 0;
>>>> + qcom_clear_read_regs(snandc);
>>>> + qcom_clear_bam_transaction(snandc);
>>>> + opcode = op->cmd.opcode;
>>>> +
>>>> + switch (opcode) {
>>>> + case SPINAND_READID:
>>>> + snandc->buf_count = 4;
>>>> + qcom_read_reg_dma(snandc, NAND_READ_ID, 1, NAND_BAM_NEXT_SGL);
>>>> + copy = true;
>>>> + break;
>>>> + case SPINAND_GET_FEATURE:
>>>> + snandc->buf_count = 4;
>>>> + qcom_read_reg_dma(snandc, NAND_FLASH_FEATURES, 1, NAND_BAM_NEXT_SGL);
>>>> + copy_ftr = true;
>>>> + break;
>>>> + case SPINAND_SET_FEATURE:
>>>> + snandc->regs->flash_feature = *(u32 *)op->data.buf.out;
>>>> + qcom_write_reg_dma(snandc, &snandc->regs->flash_feature,
>>>> + NAND_FLASH_FEATURES, 1, NAND_BAM_NEXT_SGL);
>>>> + break;
>>>> + case SPINAND_RESET:
>>>> + return 0;
>>>> + case SPINAND_PROGRAM_EXECUTE:
>>>> + return 0;
>>>> + case SPINAND_WRITE_EN:
>>>> + return 0;
>>>> + case SPINAND_ERASE:
>>>> + return 0;
>>>> + case SPINAND_READ:
>>>> + return 0;
>>>
>>> You can stack the cases
>> Ok
>>>
>>>> + default:
>>>> + return -EOPNOTSUPP;
>>>> + }
>>>> +
>>>> + ret = qcom_submit_descs(snandc);
>>>> + if (ret)
>>>> + dev_err(snandc->dev, "failure in submitting descriptor for:%d\n", opcode);
>>>> +
>>>> + if (copy) {
>>>> + qcom_nandc_dev_to_mem(snandc, true);
>>>> + memcpy(op->data.buf.in, snandc->reg_read_buf, snandc->buf_count);
>>>> + }
>>>> +
>>>> + if (copy_ftr) {
>>>> + qcom_nandc_dev_to_mem(snandc, true);
>>>> + val = le32_to_cpu(*(__le32 *)snandc->reg_read_buf);
>>>> + val >>= 8;
>>>> + memcpy(op->data.buf.in, &val, snandc->buf_count);
>>>> + }
>>>> +
>>>> + return ret;
>>>> +}
>
> Thanks,
> Miquèl