Re: [PATCH 1/5] mtd: nand: lpc32xx_slc: fix calculation of timing arcs from given values

[Brian Norris <computersforpeace@gmail.com>]

On Thu, Oct 01, 2015 at 12:17:57AM +0300, Vladimir Zapolskiy wrote:
> Hi Brian,
> 
> On 01.10.2015 00:12, Brian Norris wrote:
> > + Roland, who authored the driver
> > 
> > On Mon, Sep 28, 2015 at 07:38:58PM +0300, Vladimir Zapolskiy wrote:
> >> According to LPC32xx User's Manual all values measured in clock cycles
> >> are programmable from 1 to 16 clocks (4 bits) starting from 0 in
> >> bitfield. Correctness of 0 bitfield value (i.e. programmed 1 clock
> >> timing) is proven with actual NAND chip devices.
> >>
> >> Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
> >> ---
> >>  drivers/mtd/nand/lpc32xx_slc.c | 12 ++++++------
> >>  1 file changed, 6 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/mtd/nand/lpc32xx_slc.c b/drivers/mtd/nand/lpc32xx_slc.c
> >> index abfec13..5a3c6e0 100644
> >> --- a/drivers/mtd/nand/lpc32xx_slc.c
> >> +++ b/drivers/mtd/nand/lpc32xx_slc.c
> >> @@ -240,13 +240,13 @@ static void lpc32xx_nand_setup(struct lpc32xx_nand_host *host)
> >>  
> >>  	/* Compute clock setup values */
> >>  	tmp = SLCTAC_WDR(host->ncfg->wdr_clks) |
> >> -		SLCTAC_WWIDTH(1 + (clkrate / host->ncfg->wwidth)) |
> >> -		SLCTAC_WHOLD(1 + (clkrate / host->ncfg->whold)) |
> >> -		SLCTAC_WSETUP(1 + (clkrate / host->ncfg->wsetup)) |
> >> +		SLCTAC_WWIDTH(clkrate / host->ncfg->wwidth) |
> >> +		SLCTAC_WHOLD(clkrate / host->ncfg->whold) |
> >> +		SLCTAC_WSETUP(clkrate / host->ncfg->wsetup) |
> >>  		SLCTAC_RDR(host->ncfg->rdr_clks) |
> >> -		SLCTAC_RWIDTH(1 + (clkrate / host->ncfg->rwidth)) |
> >> -		SLCTAC_RHOLD(1 + (clkrate / host->ncfg->rhold)) |
> >> -		SLCTAC_RSETUP(1 + (clkrate / host->ncfg->rsetup));
> >> +		SLCTAC_RWIDTH(clkrate / host->ncfg->rwidth) |
> >> +		SLCTAC_RHOLD(clkrate / host->ncfg->rhold) |
> >> +		SLCTAC_RSETUP(clkrate / host->ncfg->rsetup);
> >>  	writel(tmp, SLC_TAC(host->io_base));
> >>  }
> >>  
> > 
> > Hmm, I'm guessing this was done to ensure we didn't round down too much.
> > Perhaps this should be using DIV_ROUND_UP() instead, so we can get an
> > exact match where possible, but not set too low of a clock rate by
> > rounding down?
> 
> unfortunately bare DIV_ROUND_UP() can not help here, because 0 is a
> valid resulting bitfield value here, if some timing is lower than clkrate.

OK, so if I understand it correctly, you're saying that 0 means 1
period, 1 means 2 periods, etc.? Then I guess your patch would be OK but
still conservative. I can take it, if you'd like. I'd like an ack from
Roland if possible though.

According to my reading, you still look convservative for some clock
rates. What if clock rate is exactly divisible by W_WIDTH, for instance?
Then you have:

  clkrate / wwidth = X periods

But programming a value of "X" would mean "X + 1" periods, which is 1
too much.

Maybe you actually need something like this?

	tmp = SLCTAC_WDR(DIV_ROUND_UP(clkrate, host->ncfg->wwidth) - 1) | 
		...;

Brian