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

[mdalam]

On 2021-02-15 14:10, Miquel Raynal wrote:
> Hello,
>
> Md Sadre Alam <mdalam@xxxxxxxxxxxxxx> wrote on Mon, 15 Feb 2021
> 02:47:31 +0530:
>
> > 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@xxxxxxxxxxxxxx>
> > ---
> > [V5]
> >  * Added helper function to update location register value.
>
>
> Please don't forget the "v5" in the message object.
>
> >  /*
> > @@ -1094,11 +1141,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);
>
> I guess write_reg_dma should be updated as well.

  Updated in V6 patch , please check.
> [...]
>
> > -	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 +1938,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)) {
>
> I would like the helper to handle this condition. I would prefer to
> avoid yet an extra indentation level.

  Updated in V6 patch , please check.
> > +					nandc_set_read_loc(chip, i, 0, 0, data_size, 0);
> > +					nandc_set_read_loc(chip, i, 1, data_size,
> > +							   oob_size, 1);
> > +				} else {
> > +					nandc_set_read_loc(chip, i, 0, 0, data_size, 0);
> > +					nandc_set_read_loc(chip, i, 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(chip, i, 0, 0, data_size, 1);
> > +				else
> > +					nandc_set_read_loc(chip, i, 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(chip, i, 0, data_size,
> > +							   oob_size, 1);
> > +				else
> > +					nandc_set_read_loc(chip, i, 0, data_size,
> > +							   oob_size, 1);
> >  			}
> >  		}
> >
> > -		config_nand_cw_read(nandc, true);
> > +		config_nand_cw_read(nandc, true, i == ecc->steps - 1 ? true : 
> > false);
>
> 	i == (ecc->steps - 1)
>
> is already a boolean, you don't need
>
> 	"? true : false"

   Updated in V6 patch.

> >  		if (data_buf)
> >  			read_data_dma(nandc, FLASH_BUF_ACC, data_buf,
> > @@ -1946,7 +2024,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);
>
> Maybe it's best to just forward the codeword and let the code that
> needs to know if it is the last one or not do the comparison.

  Updated in V6 patch, please check.
> >  	read_data_dma(nandc, FLASH_BUF_ACC, nandc->data_buffer, size, 0);
>
> Thanks,
> Miquèl