2021-01-28 21:42:11

by Md Sadre Alam

Subject: [PATCH V4] mtd: rawnand: qcom: update last code word register

From QPIC version 2.0 onwards new register got added to
read last codeword. This change will add the READ_LOCATION_LAST_CW_n
register.

For first three code word READ_LOCATION_n register will be
use.For last code word READ_LOCATION_LAST_CW_n register will be
use.

Signed-off-by: Md Sadre Alam <[email protected]>
---
[V4]
* Modified condition for nandc_set_read_loc_last() in qcom_nandc_read_cw_raw().
* Added one additional argument "last_cw" to the function config_nand_cw_read()
to handle last code word condition.
* Changed total number of last code word register "NAND_READ_LOCATION_LAST_CW_0" to 4
while doing code word configuration.
drivers/mtd/nand/raw/qcom_nandc.c | 110 +++++++++++++++++++++++++++++---------
1 file changed, 84 insertions(+), 26 deletions(-)

diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
index 667e4bf..9484be8 100644
--- a/drivers/mtd/nand/raw/qcom_nandc.c
+++ b/drivers/mtd/nand/raw/qcom_nandc.c
@@ -48,6 +48,10 @@
#define NAND_READ_LOCATION_1 0xf24
#define NAND_READ_LOCATION_2 0xf28
#define NAND_READ_LOCATION_3 0xf2c
+#define NAND_READ_LOCATION_LAST_CW_0 0xf40
+#define NAND_READ_LOCATION_LAST_CW_1 0xf44
+#define NAND_READ_LOCATION_LAST_CW_2 0xf48
+#define NAND_READ_LOCATION_LAST_CW_3 0xf4c

/* dummy register offsets, used by write_reg_dma */
#define NAND_DEV_CMD1_RESTORE 0xdead
@@ -187,6 +191,12 @@ nandc_set_reg(nandc, NAND_READ_LOCATION_##reg, \
((size) << READ_LOCATION_SIZE) | \
((is_last) << READ_LOCATION_LAST))

+#define nandc_set_read_loc_last(nandc, reg, offset, size, is_last) \
+nandc_set_reg(nandc, NAND_READ_LOCATION_LAST_CW_##reg, \
+ ((offset) << READ_LOCATION_OFFSET) | \
+ ((size) << READ_LOCATION_SIZE) | \
+ ((is_last) << READ_LOCATION_LAST))
+
/*
* Returns the actual register address for all NAND_DEV_ registers
* (i.e. NAND_DEV_CMD0, NAND_DEV_CMD1, NAND_DEV_CMD2 and NAND_DEV_CMD_VLD)
@@ -316,6 +326,10 @@ struct nandc_regs {
__le32 read_location1;
__le32 read_location2;
__le32 read_location3;
+ __le32 read_location_last0;
+ __le32 read_location_last1;
+ __le32 read_location_last2;
+ __le32 read_location_last3;

__le32 erased_cw_detect_cfg_clr;
__le32 erased_cw_detect_cfg_set;
@@ -644,6 +658,14 @@ static __le32 *offset_to_nandc_reg(struct nandc_regs *regs, int offset)
return &regs->read_location2;
case NAND_READ_LOCATION_3:
return &regs->read_location3;
+ case NAND_READ_LOCATION_LAST_CW_0:
+ return &regs->read_location_last0;
+ case NAND_READ_LOCATION_LAST_CW_1:
+ return &regs->read_location_last1;
+ case NAND_READ_LOCATION_LAST_CW_2:
+ return &regs->read_location_last2;
+ case NAND_READ_LOCATION_LAST_CW_3:
+ return &regs->read_location_last3;
default:
return NULL;
}
@@ -719,9 +741,14 @@ static void update_rw_regs(struct qcom_nand_host *host, int num_cw, bool read)
nandc_set_reg(nandc, NAND_READ_STATUS, host->clrreadstatus);
nandc_set_reg(nandc, NAND_EXEC_CMD, 1);

- if (read)
- nandc_set_read_loc(nandc, 0, 0, host->use_ecc ?
- host->cw_data : host->cw_size, 1);
+ if (read) {
+ if (nandc->props->qpic_v2)
+ nandc_set_read_loc_last(nandc, 0, 0, host->use_ecc ?
+ host->cw_data : host->cw_size, 1);
+ else
+ nandc_set_read_loc(nandc, 0, 0, host->use_ecc ?
+ host->cw_data : host->cw_size, 1);
+ }
}

/*
@@ -1094,11 +1121,16 @@ static void config_nand_page_read(struct qcom_nand_controller *nandc)
* before reading each codeword in NAND page.
*/
static void
-config_nand_cw_read(struct qcom_nand_controller *nandc, bool use_ecc)
+config_nand_cw_read(struct qcom_nand_controller *nandc, bool use_ecc, bool last_cw)
{
- if (nandc->props->is_bam)
- write_reg_dma(nandc, NAND_READ_LOCATION_0, 4,
- NAND_BAM_NEXT_SGL);
+ if (nandc->props->is_bam) {
+ if (nandc->props->qpic_v2 && last_cw)
+ write_reg_dma(nandc, NAND_READ_LOCATION_LAST_CW_0, 4,
+ NAND_BAM_NEXT_SGL);
+ else
+ write_reg_dma(nandc, NAND_READ_LOCATION_0, 4,
+ NAND_BAM_NEXT_SGL);
+ }

write_reg_dma(nandc, NAND_FLASH_CMD, 1, NAND_BAM_NEXT_SGL);
write_reg_dma(nandc, NAND_EXEC_CMD, 1, NAND_BAM_NEXT_SGL);
@@ -1118,10 +1150,10 @@ config_nand_cw_read(struct qcom_nand_controller *nandc, bool use_ecc)
*/
static void
config_nand_single_cw_page_read(struct qcom_nand_controller *nandc,
- bool use_ecc)
+ bool use_ecc, bool last_cw)
{
config_nand_page_read(nandc);
- config_nand_cw_read(nandc, use_ecc);
+ config_nand_cw_read(nandc, use_ecc, last_cw);
}

/*
@@ -1215,7 +1247,7 @@ static int nandc_param(struct qcom_nand_host *host)
nandc->buf_count = 512;
memset(nandc->data_buffer, 0xff, nandc->buf_count);

- config_nand_single_cw_page_read(nandc, false);
+ config_nand_single_cw_page_read(nandc, false, false);

read_data_dma(nandc, FLASH_BUF_ACC, nandc->data_buffer,
nandc->buf_count, 0);
@@ -1633,19 +1665,32 @@ qcom_nandc_read_cw_raw(struct mtd_info *mtd, struct nand_chip *chip,
}

if (nandc->props->is_bam) {
- nandc_set_read_loc(nandc, 0, read_loc, data_size1, 0);
- read_loc += data_size1;
+ if (nandc->props->qpic_v2 && cw == (ecc->steps - 1)) {
+ nandc_set_read_loc_last(nandc, 0, read_loc, data_size1, 0);
+ read_loc += data_size1;
+
+ nandc_set_read_loc_last(nandc, 1, read_loc, oob_size1, 0);
+ read_loc += oob_size1;

- nandc_set_read_loc(nandc, 1, read_loc, oob_size1, 0);
- read_loc += oob_size1;
+ nandc_set_read_loc_last(nandc, 2, read_loc, data_size2, 0);
+ read_loc += data_size2;

- nandc_set_read_loc(nandc, 2, read_loc, data_size2, 0);
- read_loc += data_size2;
+ nandc_set_read_loc_last(nandc, 3, read_loc, oob_size2, 1);
+ } else {
+ nandc_set_read_loc(nandc, 0, read_loc, data_size1, 0);
+ read_loc += data_size1;
+
+ nandc_set_read_loc(nandc, 1, read_loc, oob_size1, 0);
+ read_loc += oob_size1;

- nandc_set_read_loc(nandc, 3, read_loc, oob_size2, 1);
+ nandc_set_read_loc(nandc, 2, read_loc, data_size2, 0);
+ read_loc += data_size2;
+
+ nandc_set_read_loc(nandc, 3, read_loc, oob_size2, 1);
+ }
}

- config_nand_cw_read(nandc, false);
+ config_nand_cw_read(nandc, false, cw == ecc->steps - 1 ? true : false);

read_data_dma(nandc, reg_off, data_buf, data_size1, 0);
reg_off += data_size1;
@@ -1873,18 +1918,31 @@ static int read_page_ecc(struct qcom_nand_host *host, u8 *data_buf,

if (nandc->props->is_bam) {
if (data_buf && oob_buf) {
- nandc_set_read_loc(nandc, 0, 0, data_size, 0);
- nandc_set_read_loc(nandc, 1, data_size,
- oob_size, 1);
+ if (nandc->props->qpic_v2 && i == (ecc->steps - 1)) {
+ nandc_set_read_loc_last(nandc, 0, 0, data_size, 0);
+ nandc_set_read_loc_last(nandc, 1, data_size,
+ oob_size, 1);
+ } else {
+ nandc_set_read_loc(nandc, 0, 0, data_size, 0);
+ nandc_set_read_loc(nandc, 1, data_size,
+ oob_size, 1);
+ }
} else if (data_buf) {
- nandc_set_read_loc(nandc, 0, 0, data_size, 1);
+ if (nandc->props->qpic_v2 && i == (ecc->steps - 1))
+ nandc_set_read_loc_last(nandc, 0, 0, data_size, 1);
+ else
+ nandc_set_read_loc(nandc, 0, 0, data_size, 1);
} else {
- nandc_set_read_loc(nandc, 0, data_size,
- oob_size, 1);
+ if (nandc->props->qpic_v2 && i == (ecc->steps - 1))
+ nandc_set_read_loc_last(nandc, 0, data_size,
+ oob_size, 1);
+ else
+ nandc_set_read_loc(nandc, 0, data_size,
+ oob_size, 1);
}
}

- config_nand_cw_read(nandc, true);
+ config_nand_cw_read(nandc, true, i == ecc->steps - 1 ? true : false);

if (data_buf)
read_data_dma(nandc, FLASH_BUF_ACC, data_buf,
@@ -1946,7 +2004,7 @@ static int copy_last_cw(struct qcom_nand_host *host, int page)
set_address(host, host->cw_size * (ecc->steps - 1), page);
update_rw_regs(host, 1, true);

- config_nand_single_cw_page_read(nandc, host->use_ecc);
+ config_nand_single_cw_page_read(nandc, host->use_ecc, true);

read_data_dma(nandc, FLASH_BUF_ACC, nandc->data_buffer, size, 0);

--
2.7.4

2021-02-10 09:13:36

by Manivannan Sadhasivam

Subject: Re: [PATCH V4] mtd: rawnand: qcom: update last code word register

On Fri, Jan 29, 2021 at 03:09:19AM +0530, Md Sadre Alam wrote:
> From QPIC version 2.0 onwards new register got added to
> read last codeword. This change will add the READ_LOCATION_LAST_CW_n
> register.
>
> For first three code word READ_LOCATION_n register will be
> use.For last code word READ_LOCATION_LAST_CW_n register will be
> use.
>
> Signed-off-by: Md Sadre Alam <[email protected]>

Reviewed-by: Manivannan Sadhasivam <[email protected]>

Thanks,
Mani

> ---
> [V4]
> * Modified condition for nandc_set_read_loc_last() in qcom_nandc_read_cw_raw().
> * Added one additional argument "last_cw" to the function config_nand_cw_read()
> to handle last code word condition.
> * Changed total number of last code word register "NAND_READ_LOCATION_LAST_CW_0" to 4
> while doing code word configuration.
> drivers/mtd/nand/raw/qcom_nandc.c | 110 +++++++++++++++++++++++++++++---------
> 1 file changed, 84 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
> index 667e4bf..9484be8 100644
> --- a/drivers/mtd/nand/raw/qcom_nandc.c
> +++ b/drivers/mtd/nand/raw/qcom_nandc.c
> @@ -48,6 +48,10 @@
> #define NAND_READ_LOCATION_1 0xf24
> #define NAND_READ_LOCATION_2 0xf28
> #define NAND_READ_LOCATION_3 0xf2c
> +#define NAND_READ_LOCATION_LAST_CW_0 0xf40
> +#define NAND_READ_LOCATION_LAST_CW_1 0xf44
> +#define NAND_READ_LOCATION_LAST_CW_2 0xf48
> +#define NAND_READ_LOCATION_LAST_CW_3 0xf4c
>
> /* dummy register offsets, used by write_reg_dma */
> #define NAND_DEV_CMD1_RESTORE 0xdead
> @@ -187,6 +191,12 @@ nandc_set_reg(nandc, NAND_READ_LOCATION_##reg, \
> ((size) << READ_LOCATION_SIZE) | \
> ((is_last) << READ_LOCATION_LAST))
>
> +#define nandc_set_read_loc_last(nandc, reg, offset, size, is_last) \
> +nandc_set_reg(nandc, NAND_READ_LOCATION_LAST_CW_##reg, \
> + ((offset) << READ_LOCATION_OFFSET) | \
> + ((size) << READ_LOCATION_SIZE) | \
> + ((is_last) << READ_LOCATION_LAST))
> +
> /*
> * Returns the actual register address for all NAND_DEV_ registers
> * (i.e. NAND_DEV_CMD0, NAND_DEV_CMD1, NAND_DEV_CMD2 and NAND_DEV_CMD_VLD)
> @@ -316,6 +326,10 @@ struct nandc_regs {
> __le32 read_location1;
> __le32 read_location2;
> __le32 read_location3;
> + __le32 read_location_last0;
> + __le32 read_location_last1;
> + __le32 read_location_last2;
> + __le32 read_location_last3;
>
> __le32 erased_cw_detect_cfg_clr;
> __le32 erased_cw_detect_cfg_set;
> @@ -644,6 +658,14 @@ static __le32 *offset_to_nandc_reg(struct nandc_regs *regs, int offset)
> return &regs->read_location2;
> case NAND_READ_LOCATION_3:
> return &regs->read_location3;
> + case NAND_READ_LOCATION_LAST_CW_0:
> + return &regs->read_location_last0;
> + case NAND_READ_LOCATION_LAST_CW_1:
> + return &regs->read_location_last1;
> + case NAND_READ_LOCATION_LAST_CW_2:
> + return &regs->read_location_last2;
> + case NAND_READ_LOCATION_LAST_CW_3:
> + return &regs->read_location_last3;
> default:
> return NULL;
> }
> @@ -719,9 +741,14 @@ static void update_rw_regs(struct qcom_nand_host *host, int num_cw, bool read)
> nandc_set_reg(nandc, NAND_READ_STATUS, host->clrreadstatus);
> nandc_set_reg(nandc, NAND_EXEC_CMD, 1);
>
> - if (read)
> - nandc_set_read_loc(nandc, 0, 0, host->use_ecc ?
> - host->cw_data : host->cw_size, 1);
> + if (read) {
> + if (nandc->props->qpic_v2)
> + nandc_set_read_loc_last(nandc, 0, 0, host->use_ecc ?
> + host->cw_data : host->cw_size, 1);
> + else
> + nandc_set_read_loc(nandc, 0, 0, host->use_ecc ?
> + host->cw_data : host->cw_size, 1);
> + }
> }
>
> /*
> @@ -1094,11 +1121,16 @@ static void config_nand_page_read(struct qcom_nand_controller *nandc)
> * before reading each codeword in NAND page.
> */
> static void
> -config_nand_cw_read(struct qcom_nand_controller *nandc, bool use_ecc)
> +config_nand_cw_read(struct qcom_nand_controller *nandc, bool use_ecc, bool last_cw)
> {
> - if (nandc->props->is_bam)
> - write_reg_dma(nandc, NAND_READ_LOCATION_0, 4,
> - NAND_BAM_NEXT_SGL);
> + if (nandc->props->is_bam) {
> + if (nandc->props->qpic_v2 && last_cw)
> + write_reg_dma(nandc, NAND_READ_LOCATION_LAST_CW_0, 4,
> + NAND_BAM_NEXT_SGL);
> + else
> + write_reg_dma(nandc, NAND_READ_LOCATION_0, 4,
> + NAND_BAM_NEXT_SGL);
> + }
>
> write_reg_dma(nandc, NAND_FLASH_CMD, 1, NAND_BAM_NEXT_SGL);
> write_reg_dma(nandc, NAND_EXEC_CMD, 1, NAND_BAM_NEXT_SGL);
> @@ -1118,10 +1150,10 @@ config_nand_cw_read(struct qcom_nand_controller *nandc, bool use_ecc)
> */
> static void
> config_nand_single_cw_page_read(struct qcom_nand_controller *nandc,
> - bool use_ecc)
> + bool use_ecc, bool last_cw)
> {
> config_nand_page_read(nandc);
> - config_nand_cw_read(nandc, use_ecc);
> + config_nand_cw_read(nandc, use_ecc, last_cw);
> }
>
> /*
> @@ -1215,7 +1247,7 @@ static int nandc_param(struct qcom_nand_host *host)
> nandc->buf_count = 512;
> memset(nandc->data_buffer, 0xff, nandc->buf_count);
>
> - config_nand_single_cw_page_read(nandc, false);
> + config_nand_single_cw_page_read(nandc, false, false);
>
> read_data_dma(nandc, FLASH_BUF_ACC, nandc->data_buffer,
> nandc->buf_count, 0);
> @@ -1633,19 +1665,32 @@ qcom_nandc_read_cw_raw(struct mtd_info *mtd, struct nand_chip *chip,
> }
>
> if (nandc->props->is_bam) {
> - nandc_set_read_loc(nandc, 0, read_loc, data_size1, 0);
> - read_loc += data_size1;
> + if (nandc->props->qpic_v2 && cw == (ecc->steps - 1)) {
> + nandc_set_read_loc_last(nandc, 0, read_loc, data_size1, 0);
> + read_loc += data_size1;
> +
> + nandc_set_read_loc_last(nandc, 1, read_loc, oob_size1, 0);
> + read_loc += oob_size1;
>
> - nandc_set_read_loc(nandc, 1, read_loc, oob_size1, 0);
> - read_loc += oob_size1;
> + nandc_set_read_loc_last(nandc, 2, read_loc, data_size2, 0);
> + read_loc += data_size2;
>
> - nandc_set_read_loc(nandc, 2, read_loc, data_size2, 0);
> - read_loc += data_size2;
> + nandc_set_read_loc_last(nandc, 3, read_loc, oob_size2, 1);
> + } else {
> + nandc_set_read_loc(nandc, 0, read_loc, data_size1, 0);
> + read_loc += data_size1;
> +
> + nandc_set_read_loc(nandc, 1, read_loc, oob_size1, 0);
> + read_loc += oob_size1;
>
> - nandc_set_read_loc(nandc, 3, read_loc, oob_size2, 1);
> + nandc_set_read_loc(nandc, 2, read_loc, data_size2, 0);
> + read_loc += data_size2;
> +
> + nandc_set_read_loc(nandc, 3, read_loc, oob_size2, 1);
> + }
> }
>
> - config_nand_cw_read(nandc, false);
> + config_nand_cw_read(nandc, false, cw == ecc->steps - 1 ? true : false);
>
> read_data_dma(nandc, reg_off, data_buf, data_size1, 0);
> reg_off += data_size1;
> @@ -1873,18 +1918,31 @@ static int read_page_ecc(struct qcom_nand_host *host, u8 *data_buf,
>
> if (nandc->props->is_bam) {
> if (data_buf && oob_buf) {
> - nandc_set_read_loc(nandc, 0, 0, data_size, 0);
> - nandc_set_read_loc(nandc, 1, data_size,
> - oob_size, 1);
> + if (nandc->props->qpic_v2 && i == (ecc->steps - 1)) {
> + nandc_set_read_loc_last(nandc, 0, 0, data_size, 0);
> + nandc_set_read_loc_last(nandc, 1, data_size,
> + oob_size, 1);
> + } else {
> + nandc_set_read_loc(nandc, 0, 0, data_size, 0);
> + nandc_set_read_loc(nandc, 1, data_size,
> + oob_size, 1);
> + }
> } else if (data_buf) {
> - nandc_set_read_loc(nandc, 0, 0, data_size, 1);
> + if (nandc->props->qpic_v2 && i == (ecc->steps - 1))
> + nandc_set_read_loc_last(nandc, 0, 0, data_size, 1);
> + else
> + nandc_set_read_loc(nandc, 0, 0, data_size, 1);
> } else {
> - nandc_set_read_loc(nandc, 0, data_size,
> - oob_size, 1);
> + if (nandc->props->qpic_v2 && i == (ecc->steps - 1))
> + nandc_set_read_loc_last(nandc, 0, data_size,
> + oob_size, 1);
> + else
> + nandc_set_read_loc(nandc, 0, data_size,
> + oob_size, 1);
> }
> }
>
> - config_nand_cw_read(nandc, true);
> + config_nand_cw_read(nandc, true, i == ecc->steps - 1 ? true : false);
>
> if (data_buf)
> read_data_dma(nandc, FLASH_BUF_ACC, data_buf,
> @@ -1946,7 +2004,7 @@ static int copy_last_cw(struct qcom_nand_host *host, int page)
> set_address(host, host->cw_size * (ecc->steps - 1), page);
> update_rw_regs(host, 1, true);
>
> - config_nand_single_cw_page_read(nandc, host->use_ecc);
> + config_nand_single_cw_page_read(nandc, host->use_ecc, true);
>
> read_data_dma(nandc, FLASH_BUF_ACC, nandc->data_buffer, size, 0);
>
> --
> 2.7.4
>

2021-02-11 14:23:17

by Miquel Raynal

Subject: Re: [PATCH V4] mtd: rawnand: qcom: update last code word register

Hello,

Manivannan Sadhasivam <[email protected]> wrote on Wed,
10 Feb 2021 14:31:44 +0530:

> On Fri, Jan 29, 2021 at 03:09:19AM +0530, Md Sadre Alam wrote:
> > From QPIC version 2.0 onwards new register got added to
> > read last codeword. This change will add the READ_LOCATION_LAST_CW_n
> > register.
> >
> > For first three code word READ_LOCATION_n register will be
> > use.For last code word READ_LOCATION_LAST_CW_n register will be
> > use.

Sorry for the late notice, I think the patch is fine but if you don't
mind I would like to propose a small change that should simplify your
patch a lot, see below.

> >
> > Signed-off-by: Md Sadre Alam <[email protected]>
>
> Reviewed-by: Manivannan Sadhasivam <[email protected]>
>
> Thanks,
> Mani
>
> > ---
> > [V4]
> > * Modified condition for nandc_set_read_loc_last() in qcom_nandc_read_cw_raw().
> > * Added one additional argument "last_cw" to the function config_nand_cw_read()
> > to handle last code word condition.
> > * Changed total number of last code word register "NAND_READ_LOCATION_LAST_CW_0" to 4
> > while doing code word configuration.
> > drivers/mtd/nand/raw/qcom_nandc.c | 110 +++++++++++++++++++++++++++++---------
> > 1 file changed, 84 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
> > index 667e4bf..9484be8 100644
> > --- a/drivers/mtd/nand/raw/qcom_nandc.c
> > +++ b/drivers/mtd/nand/raw/qcom_nandc.c
> > @@ -48,6 +48,10 @@
> > #define NAND_READ_LOCATION_1 0xf24
> > #define NAND_READ_LOCATION_2 0xf28
> > #define NAND_READ_LOCATION_3 0xf2c
> > +#define NAND_READ_LOCATION_LAST_CW_0 0xf40
> > +#define NAND_READ_LOCATION_LAST_CW_1 0xf44
> > +#define NAND_READ_LOCATION_LAST_CW_2 0xf48
> > +#define NAND_READ_LOCATION_LAST_CW_3 0xf4c
> >
> > /* dummy register offsets, used by write_reg_dma */
> > #define NAND_DEV_CMD1_RESTORE 0xdead
> > @@ -187,6 +191,12 @@ nandc_set_reg(nandc, NAND_READ_LOCATION_##reg, \
> > ((size) << READ_LOCATION_SIZE) | \
> > ((is_last) << READ_LOCATION_LAST))
> >
> > +#define nandc_set_read_loc_last(nandc, reg, offset, size, is_last) \
> > +nandc_set_reg(nandc, NAND_READ_LOCATION_LAST_CW_##reg, \
> > + ((offset) << READ_LOCATION_OFFSET) | \
> > + ((size) << READ_LOCATION_SIZE) | \
> > + ((is_last) << READ_LOCATION_LAST))
> > +

You could rename the macro nandc_set_read_loc() into
nandc_set_read_loc_first() or anything else that make sense, then have
a helper which does:

nandc_set_read_loc()
{
if (condition for first)
return nandc_set_read_loc_first();
else
return nandc_set_read_loc_last();
}

And in the rest of your patch you won't have to touch anything else.

Thanks,
Miquèl

2021-02-11 19:34:31

by Md Sadre Alam

Subject: Re: [PATCH V4] mtd: rawnand: qcom: update last code word register

On 2021-02-11 19:37, Miquel Raynal wrote:
> Hello,
>
> Manivannan Sadhasivam <[email protected]> wrote on Wed,
> 10 Feb 2021 14:31:44 +0530:
>
>> On Fri, Jan 29, 2021 at 03:09:19AM +0530, Md Sadre Alam wrote:
>> > From QPIC version 2.0 onwards new register got added to
>> > read last codeword. This change will add the READ_LOCATION_LAST_CW_n
>> > register.
>> >
>> > For first three code word READ_LOCATION_n register will be
>> > use.For last code word READ_LOCATION_LAST_CW_n register will be
>> > use.
>
> Sorry for the late notice, I think the patch is fine but if you don't
> mind I would like to propose a small change that should simplify your
> patch a lot, see below.
>
>> >
>> > Signed-off-by: Md Sadre Alam <[email protected]>
>>
>> Reviewed-by: Manivannan Sadhasivam <[email protected]>
>>
>> Thanks,
>> Mani
>>
>> > ---
>> > [V4]
>> > * Modified condition for nandc_set_read_loc_last() in qcom_nandc_read_cw_raw().
>> > * Added one additional argument "last_cw" to the function config_nand_cw_read()
>> > to handle last code word condition.
>> > * Changed total number of last code word register "NAND_READ_LOCATION_LAST_CW_0" to 4
>> > while doing code word configuration.
>> > drivers/mtd/nand/raw/qcom_nandc.c | 110 +++++++++++++++++++++++++++++---------
>> > 1 file changed, 84 insertions(+), 26 deletions(-)
>> >
>> > diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
>> > index 667e4bf..9484be8 100644
>> > --- a/drivers/mtd/nand/raw/qcom_nandc.c
>> > +++ b/drivers/mtd/nand/raw/qcom_nandc.c
>> > @@ -48,6 +48,10 @@
>> > #define NAND_READ_LOCATION_1 0xf24
>> > #define NAND_READ_LOCATION_2 0xf28
>> > #define NAND_READ_LOCATION_3 0xf2c
>> > +#define NAND_READ_LOCATION_LAST_CW_0 0xf40
>> > +#define NAND_READ_LOCATION_LAST_CW_1 0xf44
>> > +#define NAND_READ_LOCATION_LAST_CW_2 0xf48
>> > +#define NAND_READ_LOCATION_LAST_CW_3 0xf4c
>> >
>> > /* dummy register offsets, used by write_reg_dma */
>> > #define NAND_DEV_CMD1_RESTORE 0xdead
>> > @@ -187,6 +191,12 @@ nandc_set_reg(nandc, NAND_READ_LOCATION_##reg, \
>> > ((size) << READ_LOCATION_SIZE) | \
>> > ((is_last) << READ_LOCATION_LAST))
>> >
>> > +#define nandc_set_read_loc_last(nandc, reg, offset, size, is_last) \
>> > +nandc_set_reg(nandc, NAND_READ_LOCATION_LAST_CW_##reg, \
>> > + ((offset) << READ_LOCATION_OFFSET) | \
>> > + ((size) << READ_LOCATION_SIZE) | \
>> > + ((is_last) << READ_LOCATION_LAST))
>> > +
>
> You could rename the macro nandc_set_read_loc() into
> nandc_set_read_loc_first() or anything else that make sense, then have
> a helper which does:
>
> nandc_set_read_loc()
> {
> if (condition for first)
> return nandc_set_read_loc_first();
> else
> return nandc_set_read_loc_last();
> }
>

Yes this is more precise way & simplify the patch a lot.
But for this i have to change these two macro as a function.

nandc_set_read_loc() & nandc_set_read_loc_last().

Since for last code word register we are using Token Pasting
Operator##.

So if i am implementing like the below.

/* helper to configure location register values */
static void nandc_set_read_loc(struct qcom_nand_controller *nandc, int
reg,
int offset, int size, int is_last, bool last_cw)
{
if (last_cw)
return nandc_set_read_loc_last(nandc, reg, offset,
size, is_last);
else
return nandc_set_read_loc_first(nandc, reg, offset,
size, is_last);
}

So here for macro expansion reg should be a value not a variable else
it will be expended like
NAND_READ_LOCATION_LAST_CW_reg instead of
NAND_READ_LOCATION_LAST_CW_0,1,2,3 etc.

the call for nandc_set_read_loc() as nandc_set_read_loc(nandc, 0,
read_loc, data_size1, 0, true); ---> for last code word.
nandc_set_read_loc(nandc, 0, read_loc, data_size1, 0, false); ---> for
first three code wrod.


So is this ok for you to convert these two macro into function ?

> And in the rest of your patch you won't have to touch anything else.
>
> Thanks,
> Miquèl

2021-02-12 08:24:12

by Miquel Raynal

Subject: Re: [PATCH V4] mtd: rawnand: qcom: update last code word register

Hello,

[email protected] wrote on Fri, 12 Feb 2021 01:00:47 +0530:

> On 2021-02-11 19:37, Miquel Raynal wrote:
> > Hello,
> >
> > Manivannan Sadhasivam <[email protected]> wrote on Wed,
> > 10 Feb 2021 14:31:44 +0530:
> >
> >> On Fri, Jan 29, 2021 at 03:09:19AM +0530, Md Sadre Alam wrote:
> >> > From QPIC version 2.0 onwards new register got added to
> >> > read last codeword. This change will add the READ_LOCATION_LAST_CW_n
> >> > register.
> >> >
> >> > For first three code word READ_LOCATION_n register will be
> >> > use.For last code word READ_LOCATION_LAST_CW_n register will be
> >> > use.
> >
> > Sorry for the late notice, I think the patch is fine but if you don't
> > mind I would like to propose a small change that should simplify your
> > patch a lot, see below.
> >
> >> >
> >> > Signed-off-by: Md Sadre Alam <[email protected]>
> >> >> Reviewed-by: Manivannan Sadhasivam <[email protected]>
> >> >> Thanks,
> >> Mani
> >> >> > ---
> >> > [V4]
> >> > * Modified condition for nandc_set_read_loc_last() in qcom_nandc_read_cw_raw().
> >> > * Added one additional argument "last_cw" to the function config_nand_cw_read()
> >> > to handle last code word condition.
> >> > * Changed total number of last code word register "NAND_READ_LOCATION_LAST_CW_0" to 4
> >> > while doing code word configuration.
> >> > drivers/mtd/nand/raw/qcom_nandc.c | 110 +++++++++++++++++++++++++++++---------
> >> > 1 file changed, 84 insertions(+), 26 deletions(-)
> >> >
> >> > diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
> >> > index 667e4bf..9484be8 100644
> >> > --- a/drivers/mtd/nand/raw/qcom_nandc.c
> >> > +++ b/drivers/mtd/nand/raw/qcom_nandc.c
> >> > @@ -48,6 +48,10 @@
> >> > #define NAND_READ_LOCATION_1 0xf24
> >> > #define NAND_READ_LOCATION_2 0xf28
> >> > #define NAND_READ_LOCATION_3 0xf2c
> >> > +#define NAND_READ_LOCATION_LAST_CW_0 0xf40
> >> > +#define NAND_READ_LOCATION_LAST_CW_1 0xf44
> >> > +#define NAND_READ_LOCATION_LAST_CW_2 0xf48
> >> > +#define NAND_READ_LOCATION_LAST_CW_3 0xf4c
> >> >
> >> > /* dummy register offsets, used by write_reg_dma */
> >> > #define NAND_DEV_CMD1_RESTORE 0xdead
> >> > @@ -187,6 +191,12 @@ nandc_set_reg(nandc, NAND_READ_LOCATION_##reg, \
> >> > ((size) << READ_LOCATION_SIZE) | \
> >> > ((is_last) << READ_LOCATION_LAST))
> >> >
> >> > +#define nandc_set_read_loc_last(nandc, reg, offset, size, is_last) \
> >> > +nandc_set_reg(nandc, NAND_READ_LOCATION_LAST_CW_##reg, \
> >> > + ((offset) << READ_LOCATION_OFFSET) | \
> >> > + ((size) << READ_LOCATION_SIZE) | \
> >> > + ((is_last) << READ_LOCATION_LAST))
> >> > +
> >
> > You could rename the macro nandc_set_read_loc() into
> > nandc_set_read_loc_first() or anything else that make sense, then have
> > a helper which does:
> >
> > nandc_set_read_loc()
> > {
> > if (condition for first)
> > return nandc_set_read_loc_first();
> > else
> > return nandc_set_read_loc_last();
> > }
> >
>
> Yes this is more precise way & simplify the patch a lot.
> But for this i have to change these two macro as a function.
>
> nandc_set_read_loc() & nandc_set_read_loc_last().
>
> Since for last code word register we are using Token Pasting Operator##.
>
> So if i am implementing like the below.
>
> /* helper to configure location register values */
> static void nandc_set_read_loc(struct qcom_nand_controller *nandc, int reg,
> int offset, int size, int is_last, bool last_cw)
> {
> if (last_cw)
> return nandc_set_read_loc_last(nandc, reg, offset, size, is_last);
> else
> return nandc_set_read_loc_first(nandc, reg, offset, size, is_last);
> }
>
> So here for macro expansion reg should be a value not a variable else it will be expended like
> NAND_READ_LOCATION_LAST_CW_reg instead of NAND_READ_LOCATION_LAST_CW_0,1,2,3 etc.

I know it involves a little bit more computation but I wonder if using
funcs instead of macros here would not be nicer? Perhaps something like:

loc = is_last ? NAND_READ_LOCATION /* 0xf20 */ : NAND_READ_LOCATION_LAST /* 0xf40 */;
loc += reg * 2;

> the call for nandc_set_read_loc() as nandc_set_read_loc(nandc, 0, read_loc, data_size1, 0, true); ---> for last code word.
> nandc_set_read_loc(nandc, 0, read_loc, data_size1, 0, false); ---> for first three code wrod.

I think it's best to forward 'cw' as a parameter and do the
computation of is_last locally.

> So is this ok for you to convert these two macro into function ?
>
> > And in the rest of your patch you won't have to touch anything else.
> >
> > Thanks,
> > Miquèl

Thanks,
Miquèl

2021-02-14 21:23:52

by Md Sadre Alam

Subject: Re: [PATCH V4] mtd: rawnand: qcom: update last code word register

On 2021-02-12 13:49, Miquel Raynal wrote:
> Hello,
>
> [email protected] wrote on Fri, 12 Feb 2021 01:00:47 +0530:
>
>> On 2021-02-11 19:37, Miquel Raynal wrote:
>> > Hello,
>> >
>> > Manivannan Sadhasivam <[email protected]> wrote on Wed,
>> > 10 Feb 2021 14:31:44 +0530:
>> >
>> >> On Fri, Jan 29, 2021 at 03:09:19AM +0530, Md Sadre Alam wrote:
>> >> > From QPIC version 2.0 onwards new register got added to
>> >> > read last codeword. This change will add the READ_LOCATION_LAST_CW_n
>> >> > register.
>> >> >
>> >> > For first three code word READ_LOCATION_n register will be
>> >> > use.For last code word READ_LOCATION_LAST_CW_n register will be
>> >> > use.
>> >
>> > Sorry for the late notice, I think the patch is fine but if you don't
>> > mind I would like to propose a small change that should simplify your
>> > patch a lot, see below.
>> >
>> >> >
>> >> > Signed-off-by: Md Sadre Alam <[email protected]>
>> >> >> Reviewed-by: Manivannan Sadhasivam <[email protected]>
>> >> >> Thanks,
>> >> Mani
>> >> >> > ---
>> >> > [V4]
>> >> > * Modified condition for nandc_set_read_loc_last() in qcom_nandc_read_cw_raw().
>> >> > * Added one additional argument "last_cw" to the function config_nand_cw_read()
>> >> > to handle last code word condition.
>> >> > * Changed total number of last code word register "NAND_READ_LOCATION_LAST_CW_0" to 4
>> >> > while doing code word configuration.
>> >> > drivers/mtd/nand/raw/qcom_nandc.c | 110 +++++++++++++++++++++++++++++---------
>> >> > 1 file changed, 84 insertions(+), 26 deletions(-)
>> >> >
>> >> > diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
>> >> > index 667e4bf..9484be8 100644
>> >> > --- a/drivers/mtd/nand/raw/qcom_nandc.c
>> >> > +++ b/drivers/mtd/nand/raw/qcom_nandc.c
>> >> > @@ -48,6 +48,10 @@
>> >> > #define NAND_READ_LOCATION_1 0xf24
>> >> > #define NAND_READ_LOCATION_2 0xf28
>> >> > #define NAND_READ_LOCATION_3 0xf2c
>> >> > +#define NAND_READ_LOCATION_LAST_CW_0 0xf40
>> >> > +#define NAND_READ_LOCATION_LAST_CW_1 0xf44
>> >> > +#define NAND_READ_LOCATION_LAST_CW_2 0xf48
>> >> > +#define NAND_READ_LOCATION_LAST_CW_3 0xf4c
>> >> >
>> >> > /* dummy register offsets, used by write_reg_dma */
>> >> > #define NAND_DEV_CMD1_RESTORE 0xdead
>> >> > @@ -187,6 +191,12 @@ nandc_set_reg(nandc, NAND_READ_LOCATION_##reg, \
>> >> > ((size) << READ_LOCATION_SIZE) | \
>> >> > ((is_last) << READ_LOCATION_LAST))
>> >> >
>> >> > +#define nandc_set_read_loc_last(nandc, reg, offset, size, is_last) \
>> >> > +nandc_set_reg(nandc, NAND_READ_LOCATION_LAST_CW_##reg, \
>> >> > + ((offset) << READ_LOCATION_OFFSET) | \
>> >> > + ((size) << READ_LOCATION_SIZE) | \
>> >> > + ((is_last) << READ_LOCATION_LAST))
>> >> > +
>> >
>> > You could rename the macro nandc_set_read_loc() into
>> > nandc_set_read_loc_first() or anything else that make sense, then have
>> > a helper which does:
>> >
>> > nandc_set_read_loc()
>> > {
>> > if (condition for first)
>> > return nandc_set_read_loc_first();
>> > else
>> > return nandc_set_read_loc_last();
>> > }
>> >
>>
>> Yes this is more precise way & simplify the patch a lot.
>> But for this i have to change these two macro as a function.
>>
>> nandc_set_read_loc() & nandc_set_read_loc_last().
>>
>> Since for last code word register we are using Token Pasting
>> Operator##.
>>
>> So if i am implementing like the below.
>>
>> /* helper to configure location register values */
>> static void nandc_set_read_loc(struct qcom_nand_controller *nandc,
>> int reg,
>> int offset, int size, int is_last, bool last_cw)
>> {
>> if (last_cw)
>> return nandc_set_read_loc_last(nandc, reg, offset,
>> size, is_last);
>> else
>> return nandc_set_read_loc_first(nandc, reg, offset,
>> size, is_last);
>> }
>>
>> So here for macro expansion reg should be a value not a variable
>> else it will be expended like
>> NAND_READ_LOCATION_LAST_CW_reg instead of
>> NAND_READ_LOCATION_LAST_CW_0,1,2,3 etc.
>
> I know it involves a little bit more computation but I wonder if using
> funcs instead of macros here would not be nicer? Perhaps something
> like:
>
> loc = is_last ? NAND_READ_LOCATION /* 0xf20 */ :
> NAND_READ_LOCATION_LAST /* 0xf40 */;
> loc += reg * 2;

I have added a helper function to update location register value in V5
patch.
Please check the patch.
>
>> the call for nandc_set_read_loc() as nandc_set_read_loc(nandc, 0,
>> read_loc, data_size1, 0, true); ---> for last code word.
>> nandc_set_read_loc(nandc, 0, read_loc, data_size1, 0, false); --->
>> for first three code wrod.
>
> I think it's best to forward 'cw' as a parameter and do the
> computation of is_last locally.
>
>> So is this ok for you to convert these two macro into function ?
>>
>> > And in the rest of your patch you won't have to touch anything else.
>> >
>> > Thanks,
>> > Miquèl
>
> Thanks,
> Miquèl