| Message ID | 1443458338-23552-1-git-send-email-vz@mleia.com |
|---|---|
| State | Changes Requested |
| Headers | show |
+ 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? Brian
(snip patch) Also, why [PATCH 1/5]? I don't see the other 4.
On 01.10.2015 00:13, Brian Norris wrote: > (snip patch) > > Also, why [PATCH 1/5]? I don't see the other 4. > Sorry for confusion, this is a leftover from my local series of various fixes, only this one patch is addressed to MTD. If you want, I can resend the change removing 1/5. -- With best wishes, Vladimir
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
On Thu, Oct 01, 2015 at 12:41:44AM +0300, Vladimir Zapolskiy wrote: > On 01.10.2015 00:13, Brian Norris wrote: > > (snip patch) > > > > Also, why [PATCH 1/5]? I don't see the other 4. > > > > Sorry for confusion, this is a leftover from my local series of various > fixes, only this one patch is addressed to MTD. If you want, I can > resend the change removing 1/5. Eh, no big deal. Just need to make sure I'm not missing things (e.g., caught in spam filters). But in the future, independent changes (especially when not sent to the same recipients) probably work better when sent standalone.
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)); }
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(-)