On Thu, Sep 02, 2021 at 11:19:23AM +0530, Md Sadre Alam wrote:
> From QPIC V2 onwards there is a separate register to read
> last code word "QPIC_NAND_READ_LOCATION_LAST_CW_n".
>
> qcom_nandc_read_cw_raw() is used to read only one code word
> at a time. If we will configure number of code words to 1 in
> in QPIC_NAND_DEV0_CFG0 register then QPIC controller thinks
> its reading the last code word, since from QPIC V2 onwards
> we are having separate register to read the last code word,
> we have to configure "QPIC_NAND_READ_LOCATION_LAST_CW_n"
> register to fetch data from controller buffer to system
> memory.
>
> Fixes: 503ee5aa ("mtd: rawnand: qcom: update last code word register")
Commit hash should be of 12 digits.
> Cc: [email protected]
> Signed-off-by: Md Sadre Alam <[email protected]>
> ---
> [V4]
> * Added commit message
> * Added changelog
>
> In commit 503ee5aa added QPIC V2 support. Since QPIC V2 onwards there
> is separate register to get last CW data. If total number of CW configred is 1
> then , QPIC controller treat this as last CW and software shhould copy data to memory
> via QPIC_NAND_READ_LOCATION_LAST_CW_n register instead of QPIC_NAND_READ_LOCATION_n.
> Since in raw read we are configuring total number of CW 1, this change fixes
> this if total number of CW 1 then make this as last CW. raw_cw = ecc->steps - 1;
> since ecc->steps holds total number of CWs.
>
This is not a changelog. Changelog should mention what has been changed
between versions from v1 to v4. For example:
Changes in v4:
* Incorporated AAA comments from X
* Changed a local variable
Changes in v3:
* Incorporated BBB comments from X
Changes in v2:
* Incorporated CCC comments from X
> drivers/mtd/nand/raw/qcom_nandc.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
> index ef0bade..04e6f7b 100644
> --- a/drivers/mtd/nand/raw/qcom_nandc.c
> +++ b/drivers/mtd/nand/raw/qcom_nandc.c
> @@ -1676,13 +1676,17 @@ qcom_nandc_read_cw_raw(struct mtd_info *mtd, struct nand_chip *chip,
> struct nand_ecc_ctrl *ecc = &chip->ecc;
> int data_size1, data_size2, oob_size1, oob_size2;
> int ret, reg_off = FLASH_BUF_ACC, read_loc = 0;
> + int raw_cw = cw;
>
> nand_read_page_op(chip, page, 0, NULL, 0);
> host->use_ecc = false;
>
> + if (nandc->props->qpic_v2)
> + raw_cw = ecc->steps - 1;
> +
Sorry, I don't understand how it can work. For reading a codeword like
0, you are reading the last codeword here if the controller is v2. And
you'll continue to read the last codeword only for any codeword
requested :/
Thanks,
Mani
> clear_bam_transaction(nandc);
> set_address(host, host->cw_size * cw, page);
> - update_rw_regs(host, 1, true, cw);
> + update_rw_regs(host, 1, true, raw_cw);
> config_nand_page_read(chip);
>
> data_size1 = mtd->writesize - host->cw_size * (ecc->steps - 1);
> @@ -1711,7 +1715,7 @@ qcom_nandc_read_cw_raw(struct mtd_info *mtd, struct nand_chip *chip,
> nandc_set_read_loc(chip, cw, 3, read_loc, oob_size2, 1);
> }
>
> - config_nand_cw_read(chip, false, cw);
> + config_nand_cw_read(chip, false, raw_cw);
>
> read_data_dma(nandc, reg_off, data_buf, data_size1, 0);
> reg_off += data_size1;
> --
> 2.7.4
>
On 2021-09-02 14:47, Manivannan Sadhasivam wrote:
> On Thu, Sep 02, 2021 at 11:19:23AM +0530, Md Sadre Alam wrote:
>> From QPIC V2 onwards there is a separate register to read
>> last code word "QPIC_NAND_READ_LOCATION_LAST_CW_n".
>>
>> qcom_nandc_read_cw_raw() is used to read only one code word
>> at a time. If we will configure number of code words to 1 in
>> in QPIC_NAND_DEV0_CFG0 register then QPIC controller thinks
>> its reading the last code word, since from QPIC V2 onwards
>> we are having separate register to read the last code word,
>> we have to configure "QPIC_NAND_READ_LOCATION_LAST_CW_n"
>> register to fetch data from controller buffer to system
>> memory.
>>
>> Fixes: 503ee5aa ("mtd: rawnand: qcom: update last code word register")
>
> Commit hash should be of 12 digits.
Updated in patch V5
>
>> Cc: [email protected]
>> Signed-off-by: Md Sadre Alam <[email protected]>
>> ---
>> [V4]
>> * Added commit message
>> * Added changelog
>>
>> In commit 503ee5aa added QPIC V2 support. Since QPIC V2 onwards there
>> is separate register to get last CW data. If total number of CW
>> configred is 1
>> then , QPIC controller treat this as last CW and software shhould
>> copy data to memory
>> via QPIC_NAND_READ_LOCATION_LAST_CW_n register instead of
>> QPIC_NAND_READ_LOCATION_n.
>> Since in raw read we are configuring total number of CW 1, this
>> change fixes
>> this if total number of CW 1 then make this as last CW. raw_cw =
>> ecc->steps - 1;
>> since ecc->steps holds total number of CWs.
>>
>
> This is not a changelog. Changelog should mention what has been changed
> between versions from v1 to v4. For example:
>
> Changes in v4:
>
> * Incorporated AAA comments from X
> * Changed a local variable
>
> Changes in v3:
>
> * Incorporated BBB comments from X
>
> Changes in v2:
>
> * Incorporated CCC comments from X
Updated in patch V5
>
>> drivers/mtd/nand/raw/qcom_nandc.c | 8 ++++++--
>> 1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/raw/qcom_nandc.c
>> b/drivers/mtd/nand/raw/qcom_nandc.c
>> index ef0bade..04e6f7b 100644
>> --- a/drivers/mtd/nand/raw/qcom_nandc.c
>> +++ b/drivers/mtd/nand/raw/qcom_nandc.c
>> @@ -1676,13 +1676,17 @@ qcom_nandc_read_cw_raw(struct mtd_info *mtd,
>> struct nand_chip *chip,
>> struct nand_ecc_ctrl *ecc = &chip->ecc;
>> int data_size1, data_size2, oob_size1, oob_size2;
>> int ret, reg_off = FLASH_BUF_ACC, read_loc = 0;
>> + int raw_cw = cw;
>>
>> nand_read_page_op(chip, page, 0, NULL, 0);
>> host->use_ecc = false;
>>
>> + if (nandc->props->qpic_v2)
>> + raw_cw = ecc->steps - 1;
>> +
>
> Sorry, I don't understand how it can work. For reading a codeword like
> 0, you are reading the last codeword here if the controller is v2. And
> you'll continue to read the last codeword only for any codeword
> requested :/
This case is only applicable when we will configure number of code
words to 1.
These registers "QPIC_NAND_READ_LOCATION_LAST_CW_n" &
"QPIC_NAND_READ_LOCATION_n"
are only used to copy data into memory i.e our local buffer. So this
last CW interpretation from
QPIC controller to memory not for Flash to QPIC controller.
If number of CW configured to 1 means QPIC will treat this a last CW ,
so copy the data from QPIC to local memory
buffer , we have to use QPIC_NAND_READ_LOCATION_LAST_CW_n register
instead of QPIC_NAND_READ_LOCATION_n regsiter,
Since there is separate register from QPIC V2 onwards to read get last
CW "QPIC_NAND_READ_LOCATION_LAST_CW_n".
>
> Thanks,
> Mani
>
>> clear_bam_transaction(nandc);
>> set_address(host, host->cw_size * cw, page);
>> - update_rw_regs(host, 1, true, cw);
>> + update_rw_regs(host, 1, true, raw_cw);
>> config_nand_page_read(chip);
>>
>> data_size1 = mtd->writesize - host->cw_size * (ecc->steps - 1);
>> @@ -1711,7 +1715,7 @@ qcom_nandc_read_cw_raw(struct mtd_info *mtd,
>> struct nand_chip *chip,
>> nandc_set_read_loc(chip, cw, 3, read_loc, oob_size2, 1);
>> }
>>
>> - config_nand_cw_read(chip, false, cw);
>> + config_nand_cw_read(chip, false, raw_cw);
>>
>> read_data_dma(nandc, reg_off, data_buf, data_size1, 0);
>> reg_off += data_size1;
>> --
>> 2.7.4
>>