Hi Abhishek,
On Fri, 25 May 2018 17:51:43 +0530, Abhishek Sahu
<abs...@codeaurora.org> wrote:
> This patch does minor code reorganization for raw reads.
> Currently the raw read is required for complete page but for
> subsequent patches related with erased codeword bit flips
> detection, only few CW should be read. So, this patch adds
> helper function and introduces the read CW bitmask which
> specifies which CW reads are required in complete page.
>
> Signed-off-by: Abhishek Sahu <abs...@codeaurora.org>
> ---
> * Changes from v2:
> NONE
>
> * Changes from v1:
> 1. Included more detail in function comment
>
> drivers/mtd/nand/raw/qcom_nandc.c | 197
> ++++++++++++++++++++++++--------------
> 1 file changed, 123 insertions(+), 74 deletions(-)
>
> diff --git a/drivers/mtd/nand/raw/qcom_nandc.c
> b/drivers/mtd/nand/raw/qcom_nandc.c
> index 87f900e..34143a4 100644
> --- a/drivers/mtd/nand/raw/qcom_nandc.c
> +++ b/drivers/mtd/nand/raw/qcom_nandc.c
> @@ -1588,6 +1588,127 @@ static int check_flash_errors(struct qcom_nand_host
> *host, int cw_cnt)
> }
>
> /*
> + * Helper to perform the page raw read operation. The read_cw_mask will be
> + * used to specify the codewords (CW) for which the data should be read. The
> + * single page contains multiple CW.
> + *
> + * Normally other NAND controllers store the data in main area and
> + * ecc bytes in OOB area. So, if page size is 2048+64 then 2048
> + * data bytes will go in main area followed by ECC bytes. The QCOM NAND
> + * controller follows different layout in which the data+OOB is internally
> + * divided in 528/532 bytes CW and each CW contains 516 bytes followed by
> + * ECC parity bytes for that CW. By this, 4 available OOB bytes per CW
> + * will also be protected with ECC.
> + *
> + * For each CW read, following are the 2 steps:
> + * 1. Read the codeword bytes from NAND chip to NAND controller internal HW
> + * buffer.
> + * 2. Copy all these bytes from this HW buffer to actual buffer.
> + *
> + * Sometime, only few CW data is required in complete page. The read_cw_mask
> + * specifies which CW in a page needs to be read. Start address will be
> + * determined with this CW mask to skip unnecessary data copy from NAND
> + * flash device. Then, actual data copy from NAND controller HW internal
> buffer
> + * to data buffer will be done only for the CWs, which have the mask set.
> + */
> +static int
> +nandc_read_page_raw(struct mtd_info *mtd, struct nand_chip *chip,
> + u8 *data_buf, u8 *oob_buf,
> + int page, unsigned long read_cw_mask)
Please prefix the helper with "qcom_nandc"
> +{
> + struct qcom_nand_host *host = to_qcom_nand_host(chip);
> + struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
> + struct nand_ecc_ctrl *ecc = &chip->ecc;
> + int i, ret;
> + int read_loc, start_step, last_step;
> +
> + nand_read_page_op(chip, page, 0, NULL, 0);
> +
> + host->use_ecc = false;
> + start_step = ffs(read_cw_mask) - 1;
> + last_step = fls(read_cw_mask);
> +
> + clear_bam_transaction(nandc);
> + set_address(host, host->cw_size * start_step, page);
> + update_rw_regs(host, last_step - start_step, true);
> + config_nand_page_read(nandc);
> +
> + for (i = start_step; i < last_step; i++) {
This comment applies for both patches 15 and 16:
I would really prefer having a qcom_nandc_read_cw_raw() that reads only
one CW. From qcom_nandc_read_page_raw() you would loop over all the CW
calling qcom_nandc_read_cw_raw() helper (it's raw reads, we don't care
about performances) and from ->read_page_raw() you would check
CW with uncorrectable errors for being blank with that helper. You
would avoid the not-so-nice logic where you read all the CW between the
first bad one and the last bad one.
Thanks,
Miquèl
