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

[mdalam]

On 2021-02-12 13:49, Miquel Raynal wrote:
> Hello,
>
> mdalam@xxxxxxxxxxxxxx wrote on Fri, 12 Feb 2021 01:00:47 +0530:
>
> > On 2021-02-11 19:37, Miquel Raynal wrote:
> > > Hello,
> > >
> > > Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx> 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@xxxxxxxxxxxxxx>
> > >> >> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
> > >> >> 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