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

[mdalam]

On 2021-02-24 01:13, mdalam@xxxxxxxxxxxxxx wrote:
> On 2021-02-23 22:04, Miquel Raynal wrote:
> > Hello,
> >
> > Md Sadre Alam <mdalam@xxxxxxxxxxxxxx> wrote on Tue, 23 Feb 2021
> > 01:34:27 +0530:
> >
> > > From QPIC version 2.0 onwards new register got added to read last
> >
> >                                a new
> >
> > > codeword. This change will add the READ_LOCATION_LAST_CW_n register.
> >
> >             Add support for this 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.
> >
> > "
> > In the case of QPIC v2, codewords 0, 1 and 2 will be accessed through
> > READ_LOCATION_n, while codeword 3 will be accessed through
> > READ_LOCATION_LAST_CW_n.
> > "
> >
> > When I read my own sentence, I feel that there is something wrong.
> > If there are only 4 codewords, I guess a QPIC v2 is able to use
> > READ_LOCATION_3 or READ_LOCATION_LAST_CW_0 interchangeably. Isn't it?
> >
> > I guess the point of having these "last_cw_n" registers is to support
> > up to 8 codewords, am I wrong? If this the case, the current patch
> > completely fails doing that I don't get the point of such change.
>
> This register is only use to read last code word.
>
> I have address all the comments from all the previous sub sequent
> patches and pushed
> all patches in only one series.
>
> Please check.

 The registers READ_LOCATION & READ_LOCATION_LAST are not associated 
with number of code words.
 These two registers are used to access the location inside a code word. 
So whether we are having 4 code words
 or 8 code words it doesn't matter. If we wanted access the location 
within normal code word we have to
 use READ_LOCATION register and if we wanted to access location in last 
code word then we have to use
 READ_LOCATION_LAST.
> > > Signed-off-by: Md Sadre Alam <mdalam@xxxxxxxxxxxxxx>
> > > ---
> >
> > [...]
> >
> > >  /* helper to configure address register values */
> > > @@ -700,8 +727,9 @@ static void set_address(struct qcom_nand_host 
> > > *host, u16 column, int page)
> > >   *
> > >   * @num_cw:		number of steps for the read/write operation
> > >   * @read:		read or write operation
> > > + * @cw	:		which code word
> > >   */
> > > -static void update_rw_regs(struct qcom_nand_host *host, int num_cw, 
> > > bool read)
> > > +static void update_rw_regs(struct qcom_nand_host *host, int num_cw, 
> > > bool read, int cw)
> > >  {
> > >  	struct nand_chip *chip = &host->chip;
> > >  	struct qcom_nand_controller *nandc = 
> > > get_qcom_nand_controller(chip);
> > > @@ -740,7 +768,7 @@ static void update_rw_regs(struct qcom_nand_host 
> > > *host, int num_cw, bool read)
> > >  	nandc_set_reg(nandc, NAND_EXEC_CMD, 1);
> > >
> > >  	if (read)
> > > -		nandc_set_read_loc(chip, 0, 0, 0, host->use_ecc ?
> > > +		nandc_set_read_loc(chip, cw, 0, 0, host->use_ecc ?
> > >  				   host->cw_data : host->cw_size, 1);
> > >  }
> > >
> > > @@ -1111,18 +1139,34 @@ static void config_nand_page_read(struct 
> > > nand_chip *chip)
> > >  		      NAND_ERASED_CW_SET | NAND_BAM_NEXT_SGL);
> > >  }
> > >
> > > +/* helper to check which location register should be use for this
> >
> >     /*
> >      * Check which location...
> >
> > > + * code word. NAND_READ_LOCATION or NAND_READ_LOCATION_LAST_CW
> > > + */
> > > +static bool config_loc_last_reg(struct nand_chip *chip, int cw)
> > > +{
> > > +	struct qcom_nand_controller *nandc = 
> > > get_qcom_nand_controller(chip);
> > > +	struct nand_ecc_ctrl *ecc = &chip->ecc;
> > > +
> > > +	if (nandc->props->qpic_v2 && qcom_nandc_is_last_cw(ecc, cw))
> > > +		return true;
> >
> > Not sure this is really useful, it's probably better to drop this
> > helper and just use...
> >
> > > +
> > > +	return false;
> > > +}
> > >  /*
> > >   * Helper to prepare DMA descriptors for configuring registers
> > >   * before reading each codeword in NAND page.
> > >   */
> > >  static void
> > > -config_nand_cw_read(struct nand_chip *chip, bool use_ecc)
> > > +config_nand_cw_read(struct nand_chip *chip, bool use_ecc, int cw)
> > >  {
> > >  	struct qcom_nand_controller *nandc = 
> > > get_qcom_nand_controller(chip);
> > > +	int reg = NAND_READ_LOCATION_0;
> > > +
> > > +	if (config_loc_last_reg(chip, cw))
> >
> > ...     if (nandc->props->qpic_v2 && qcom_nandc_is_lastcw()) here.
> >
> > > +		reg = NAND_READ_LOCATION_LAST_CW_0;
> > >
> > >  	if (nandc->props->is_bam)
> > > -		write_reg_dma(nandc, NAND_READ_LOCATION_0, 4,
> > > -			      NAND_BAM_NEXT_SGL);
> > > +		write_reg_dma(nandc, reg, 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);
> > > @@ -1142,12 +1186,12 @@ config_nand_cw_read(struct nand_chip *chip, 
> > > bool use_ecc)
> >
> > Thanks,
> > Miquèl