[PATCH] mtd: nand: add ->setup_data_interface() support for Marvell NFCv1

[Miquel Raynal]

Hi Boris,

On Tue, 16 Jan 2018 10:00:08 +0100
Boris Brezillon <boris.brezillon at free-electrons.com> wrote:

> On Mon, 15 Jan 2018 23:32:54 +0100
> Miquel Raynal <miquel.raynal at free-electrons.com> wrote:
> 
> > First generation of Marvell NAND flash controllers (eg. embedded in
> > PXA boards) did not make use of the NAND core hook
> > ->setup_data_interface() to setup controller timings. Add support
> > for it.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal at free-electrons.com>
> > ---
> >  drivers/mtd/nand/marvell_nand.c | 39
> > ++++++++++++++++++++++++++++----------- 1 file changed, 28
> > insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/marvell_nand.c
> > b/drivers/mtd/nand/marvell_nand.c index 10f108cc08e2..7db870ae1b07
> > 100644 --- a/drivers/mtd/nand/marvell_nand.c
> > +++ b/drivers/mtd/nand/marvell_nand.c
> > @@ -379,6 +379,8 @@ struct marvell_nfc_timings {
> >   * return the number of clock periods.
> >   */
> >  #define TO_CYCLES(ps, period_ns) (DIV_ROUND_UP(ps / 1000,
> > period_ns)) +#define TO_CYCLES64(ps, period_ns)
> > (DIV_ROUND_UP_ULL(div_u64(ps, 1000), \
> > +						     period_ns))
> >  
> >  /**
> >   * NAND driver structure filled during the parsing of the
> > ->exec_op() subop @@ -2239,31 +2241,47 @@ static int
> > marvell_nfc_setup_data_interface(struct mtd_info *mtd, int chipnr,
> > nfc_tmg.tRHW = TO_CYCLES(max_t(int, sdr->tRHW_min, sdr->tCCS_min),
> > period_ns); 
> > -	/* Use WAIT_MODE (wait for RB line) instead of only
> > relying on delays */
> > -	nfc_tmg.tR = TO_CYCLES(sdr->tWB_max, period_ns);
> > +	/*
> > +	 * NFCv2: Use WAIT_MODE (wait for RB line), do not rely
> > only on delays.
> > +	 * NFCv1: No WAIT_MODE, tR must be maximal.
> > +	 */
> > +	if (nfc->caps->is_nfcv2)
> > +		nfc_tmg.tR = TO_CYCLES(sdr->tWB_max, period_ns);
> > +	else
> > +		nfc_tmg.tR = TO_CYCLES64(sdr->tR_max, period_ns)
> > -  
> 
> Should be TO_CYCLES64(sdr->tR_max + sdr->tWB_max, period_ns) here.

I was not sure, as tWB is much much smaller than tR if it would
have a real impact, but that is more accurate.

> 
> > +			nfc_tmg.tCH - 3;
> > +
> > +	if (nfc_tmg.tR < 0)  
> 
> ->tR is an unsigned int, so this test always returns false...  
> 
> How about:
> 
> 	if (nfc->caps->is_nfcv2) {
> 		nfc_tmg.tR = TO_CYCLES(sdr->tWB_max, period_ns);
> 	} else {
> 		nfc_tmg.tR = TO_CYCLES64(sdr->tR_max - sdr->tWB_max,
> 					 period_ns);
> 		if (nfc_tmg.tR + 3 > nfc_tmg.tCH)
> 			nfc_tmg.tR > nfc_tmg.tCH - 3;
> 		else
> 			nfc_tmg.tR = 0;
> 	}
> 

I will s/nfc_tmg.tR > nfc_tmg.tCH/nfc_tmg.tR = nfc_tmg.tCH/
But sure.

> > +		return -EINVAL;
> >  
> >  	if (chipnr < 0)
> >  		return 0;
> >  
> >  	marvell_nand->ndtr0 =
> >  		NDTR0_TRP(nfc_tmg.tRP) |
> > -		NDTR0_TRH(nfc_tmg.tRH) |
> >  		NDTR0_ETRP(nfc_tmg.tRP) |
> > +		NDTR0_TRH(nfc_tmg.tRH) |  
> 
> Not a big deal, but it seems you reordered lines for no real reason
> here.

That is right, there is no meaning changing it here.

> 
> >  		NDTR0_TWP(nfc_tmg.tWP) |
> >  		NDTR0_TWH(nfc_tmg.tWH) |
> >  		NDTR0_TCS(nfc_tmg.tCS) |
> > -		NDTR0_TCH(nfc_tmg.tCH) |
> > -		NDTR0_RD_CNT_DEL(read_delay) |
> > -		NDTR0_SELCNTR |
> > -		NDTR0_TADL(nfc_tmg.tADL);
> > +		NDTR0_TCH(nfc_tmg.tCH);
> >  
> >  	marvell_nand->ndtr1 =
> >  		NDTR1_TAR(nfc_tmg.tAR) |
> >  		NDTR1_TWHR(nfc_tmg.tWHR) |
> > -		NDTR1_TRHW(nfc_tmg.tRHW) |
> > -		NDTR1_WAIT_MODE |
> >  		NDTR1_TR(nfc_tmg.tR);
> >  
> > +	if (nfc->caps->is_nfcv2) {
> > +		marvell_nand->ndtr0 |=
> > +			NDTR0_RD_CNT_DEL(read_delay) |
> > +			NDTR0_SELCNTR |
> > +			NDTR0_TADL(nfc_tmg.tADL);
> > +
> > +		marvell_nand->ndtr1 |=
> > +			NDTR1_TRHW(nfc_tmg.tRHW) |
> > +			NDTR1_WAIT_MODE;
> > +	}
> > +
> >  	return 0;
> >  }
> >  
> > @@ -2398,8 +2416,7 @@ static int marvell_nand_chip_init(struct
> > device *dev, struct marvell_nfc *nfc, 
> >  	chip->exec_op = marvell_nfc_exec_op;
> >  	chip->select_chip = marvell_nfc_select_chip;
> > -	if (nfc->caps->is_nfcv2 &&
> > -	    !of_property_read_bool(np, "marvell,nand-keep-config"))
> > +	if (!of_property_read_bool(np, "marvell,nand-keep-config"))
> >  		chip->setup_data_interface =
> > marvell_nfc_setup_data_interface; 
> >  	mtd = nand_to_mtd(chip);  
> 

Thanks,
Miquèl