Re: [PATCH v8 2/3] mtd: nand: Qualcomm NAND controller driver

[Archit Taneja <architt@codeaurora.org>]

Hi Brian,

On 02/04/2016 04:09 PM, Boris Brezillon wrote:
> Hi Archit,
>
> On Wed,  3 Feb 2016 14:29:50 +0530
> Archit Taneja <architt@codeaurora.org> wrote:
>
>> The Qualcomm NAND controller is found in SoCs like IPQ806x, MSM7xx,
>> MDM9x15 series.
>>
>> It exists as a sub block inside the IPs EBI2 (External Bus Interface 2)
>> and QPIC (Qualcomm Parallel Interface Controller). These IPs provide a
>> broader interface for external slow peripheral devices such as LCD and
>> NAND/NOR flash memory or SRAM like interfaces.
>>
>> We add support for the NAND controller found within EBI2. For the SoCs
>> of our interest, we only use the NAND controller within EBI2. Therefore,
>> it's safe for us to assume that the NAND controller is a standalone block
>> within the SoC.
>>
>> The controller supports 512B, 2kB, 4kB and 8kB page 8-bit and 16-bit NAND
>> flash devices. It contains a HW ECC block that supports BCH ECC (4, 8 and
>> 16 bit correction/step) and RS ECC(4 bit correction/step) that covers main
>> and spare data. The controller contains an internal 512 byte page buffer
>> to which we read/write via DMA. The EBI2 type NAND controller uses ADM DMA
>> for register read/write and data transfers. The controller performs page
>> reads and writes at a codeword/step level of 512 bytes. It can support up
>> to 2 external chips of different configurations.
>>
>> The driver prepares register read and write configuration descriptors for
>> each codeword, followed by data descriptors to read or write data from the
>> controller's internal buffer. It uses a single ADM DMA channel that we get
>> via dmaengine API. The controller requires 2 ADM CRCIs for command and
>> data flow control. These are passed via DT.
>>
>> The ecc layout used by the controller is syndrome like, but we can't use
>> the standard syndrome ecc ops because of several reasons. First, the amount
>> of data bytes covered by ecc isn't same in each step. Second, writing to
>> free oob space requires us writing to the entire step in which the oob
>> lies. This forces us to create our own ecc ops.
>>
>> One more difference is how the controller accesses the bad block marker.
>> The controller ignores reading the marker when ECC is enabled. ECC needs
>> to be explicity disabled to read or write to the bad block marker. The
>> nand_bbt helpers library hence can't access BBMs for the controller.
>> For now, we skip the creation of BBT and populate chip->block_bad and
>> chip->block_markbad helpers instead.
>>
>> Reviewed-by: Andy Gross <agross@codeaurora.org>
>> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
>> Signed-off-by: Archit Taneja <architt@codeaurora.org>
>> ---
>> v8:
>>    - Use nand_check_erased_ecc_chunk in the right manner as
>>      suggested by Boris (place the check when we see uncorrectable
>>      errors).
>>    - Rewrite the empty_page_fixup code such that we check for a
>>      codeword being erased rather than the entire page. This simplifies
>>      the code and we can now place it in parse_read_errors().
>>    - Introduce raw page access functions. This results in making some
>>      modifications in the existing ECC page access ops too, since the
>>      layout now also considers the real/dummy bad block markers. Explained
>>      in comments.
>>
>> v7:
>>    - Incorporated missing/new comments by Boris
>>    - Cleaned up some strict checkpatch warnings
>>
>> v6:
>>    - Fix up erased page parsing. Use nand_check_erased_ecc_chunk to
>>      return corrected bitflips in an erased page.
>>    - Fix whitespace issues
>>    - Update compatible tring to something more specific
>>
>> v5:
>>    - split chip/controller structs
>>    - simplify layout by considering reserved bytes as part of ECC
>>    - create ecc layouts automatically
>>    - implement block_bad and block_markbad chip ops instead of
>>    - read_oob_raw/write_oob_raw ecc ops to access BBMs.
>>    - Add NAND_SKIP_BBTSCAN flag until we get badblockbits support.
>>    - misc clean ups
>>
>> v4:
>>    - Shrink submit_descs
>>    - add desc list node at the end of dma_prep_desc
>>    - Endianness and warning fixes
>>    - Add Stephen's Signed-off since he provided a patch to fix
>>      endianness problems
>>
>> v3:
>>    - Refactor dma functions for maximum reuse
>>    - Use dma_slave_confing on stack
>>    - optimize and clean upempty_page_fixup using memchr_inv
>>    - ensure portability with dma register reads using le32_* funcs
>>    - use NAND_USE_BOUNCE_BUFFER instead of doing it ourselves
>>    - fix handling of return values of dmaengine funcs
>>    - constify wherever possible
>>    - Remove dependency on ADM DMA in Kconfig
>>    - Misc fixes and clean ups
>>
>> v2:
>>    - Use new BBT flag that allows us to read BBM in raw mode
>>    - reduce memcpy-s in the driver
>>    - some refactor and clean ups because of above changes
>>
>>   drivers/mtd/nand/Kconfig      |    7 +
>>   drivers/mtd/nand/Makefile     |    1 +
>>   drivers/mtd/nand/qcom_nandc.c | 2224 +++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 2232 insertions(+)
>>   create mode 100644 drivers/mtd/nand/qcom_nandc.c
>>
>
> [...]
>
>> +static int qcom_nand_host_setup(struct qcom_nand_host *host)
>> +{
>> +	struct nand_chip *chip = &host->chip;
>> +	struct mtd_info *mtd = nand_to_mtd(chip);
>> +	struct nand_ecc_ctrl *ecc = &chip->ecc;
>> +	struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
>> +	int cwperpage, bad_block_byte;
>> +	bool wide_bus;
>> +	int ecc_mode = 1;
>> +
>> +	/*
>> +	 * the controller requires each step consists of 512 bytes of data.
>> +	 * bail out if DT has populated a wrong step size.
>> +	 */
>> +	if (ecc->size != NANDC_STEP_SIZE) {
>> +		dev_err(nandc->dev, "invalid ecc size\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	wide_bus = chip->options & NAND_BUSWIDTH_16 ? true : false;
>> +
>> +	if (ecc->strength >= 8) {
>> +		/* 8 bit ECC defaults to BCH ECC on all platforms */
>> +		host->bch_enabled = true;
>> +		ecc_mode = 1;
>> +
>> +		if (wide_bus) {
>> +			host->ecc_bytes_hw = 14;
>> +			host->spare_bytes = 0;
>> +			host->bbm_size = 2;
>> +		} else {
>> +			host->ecc_bytes_hw = 13;
>> +			host->spare_bytes = 2;
>> +			host->bbm_size = 1;
>> +		}
>> +	} else {
>> +		/*
>> +		 * if the controller supports BCH for 4 bit ECC, the controller
>> +		 * uses lesser bytes for ECC. If RS is used, the ECC bytes is
>> +		 * always 10 bytes
>> +		 */
>> +		if (nandc->ecc_modes & ECC_BCH_4BIT) {
>> +			/* BCH */
>> +			host->bch_enabled = true;
>> +			ecc_mode = 0;
>> +
>> +			if (wide_bus) {
>> +				host->ecc_bytes_hw = 8;
>> +				host->spare_bytes = 2;
>> +				host->bbm_size = 2;
>> +			} else {
>> +				host->ecc_bytes_hw = 7;
>> +				host->spare_bytes = 4;
>> +				host->bbm_size = 1;
>> +			}
>> +		} else {
>> +			/* RS */
>> +			host->ecc_bytes_hw = 10;
>> +
>> +			if (wide_bus) {
>> +				host->spare_bytes = 0;
>> +				host->bbm_size = 2;
>> +			} else {
>> +				host->spare_bytes = 1;
>> +				host->bbm_size = 1;
>> +			}
>> +		}
>> +	}
>> +
>> +	/*
>> +	 * we consider ecc->bytes as the sum of all the non-data content in a
>> +	 * step. It gives us a clean representation of the oob area (even if
>> +	 * all the bytes aren't used for ECC).It is always 16 bytes for 8 bit
>> +	 * ECC and 12 bytes for 4 bit ECC
>> +	 */
>> +	ecc->bytes = host->ecc_bytes_hw + host->spare_bytes + host->bbm_size;
>> +
>> +	ecc->read_page		= qcom_nandc_read_page;
>> +	ecc->read_page_raw	= qcom_nandc_read_page_raw;
>> +	ecc->read_oob		= qcom_nandc_read_oob;
>> +	ecc->write_page		= qcom_nandc_write_page;
>> +	ecc->write_page_raw	= qcom_nandc_write_page_raw;
>> +	ecc->write_oob		= qcom_nandc_write_oob;
>
> You should probably also implement ->{read, write}_oob_raw(), otherwise
> the core set them to ecc->{read, write}_oob(), which is not exactly the
> same. Anyway, let's keep that as things that as future improvements.
> The rest looks good to me.
>
> Reviewed-by: Boris Brezillon <boris.brezillon@free-electrons.com>

If the patchset looks okay to you, could you please take this for the
4.6 merge window?

Thanks,
Archit

>
> Thanks for your patience, and all the reworks you've done.
>
> Best Regards,
>
> Boris
>

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum, hosted by The Linux Foundation