| Message ID | 1611869959-5109-1-git-send-email-mdalam@codeaurora.org |
|---|---|
| State | Changes Requested |
| Delegated to: | Miquel Raynal |
| Headers | show |
| Series | [V4] mtd: rawnand: qcom: update last code word register | expand |
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 <mdalam@codeaurora.org> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> 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 ®s->read_location2; > case NAND_READ_LOCATION_3: > return ®s->read_location3; > + case NAND_READ_LOCATION_LAST_CW_0: > + return ®s->read_location_last0; > + case NAND_READ_LOCATION_LAST_CW_1: > + return ®s->read_location_last1; > + case NAND_READ_LOCATION_LAST_CW_2: > + return ®s->read_location_last2; > + case NAND_READ_LOCATION_LAST_CW_3: > + return ®s->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 >
Hello, Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> 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 <mdalam@codeaurora.org> > > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > 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
On 2021-02-11 19:37, Miquel Raynal wrote: > Hello, > > Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> 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 <mdalam@codeaurora.org> >> >> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> >> >> 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
Hello, mdalam@codeaurora.org wrote on Fri, 12 Feb 2021 01:00:47 +0530: > On 2021-02-11 19:37, Miquel Raynal wrote: > > Hello, > > > > Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> 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 <mdalam@codeaurora.org> > >> >> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > >> >> 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
On 2021-02-12 13:49, Miquel Raynal wrote: > Hello, > > mdalam@codeaurora.org wrote on Fri, 12 Feb 2021 01:00:47 +0530: > >> On 2021-02-11 19:37, Miquel Raynal wrote: >> > Hello, >> > >> > Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> 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 <mdalam@codeaurora.org> >> >> >> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> >> >> >> 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
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 ®s->read_location2; case NAND_READ_LOCATION_3: return ®s->read_location3; + case NAND_READ_LOCATION_LAST_CW_0: + return ®s->read_location_last0; + case NAND_READ_LOCATION_LAST_CW_1: + return ®s->read_location_last1; + case NAND_READ_LOCATION_LAST_CW_2: + return ®s->read_location_last2; + case NAND_READ_LOCATION_LAST_CW_3: + return ®s->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);
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 <mdalam@codeaurora.org> --- [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(-)