* [PATCH 1/5] mtd: nand: lpc32xx_slc: fix calculation of timing arcs from given values @ 2015-09-28 16:38 Vladimir Zapolskiy 2015-09-30 21:12 ` Brian Norris 0 siblings, 1 reply; 8+ messages in thread From: Vladimir Zapolskiy @ 2015-09-28 16:38 UTC (permalink / raw) To: David Woodhouse, Brian Norris; +Cc: linux-mtd 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)); } -- 2.5.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/5] mtd: nand: lpc32xx_slc: fix calculation of timing arcs from given values 2015-09-28 16:38 [PATCH 1/5] mtd: nand: lpc32xx_slc: fix calculation of timing arcs from given values Vladimir Zapolskiy @ 2015-09-30 21:12 ` Brian Norris 2015-09-30 21:13 ` Brian Norris 2015-09-30 21:17 ` Vladimir Zapolskiy 0 siblings, 2 replies; 8+ messages in thread From: Brian Norris @ 2015-09-30 21:12 UTC (permalink / raw) To: Vladimir Zapolskiy; +Cc: David Woodhouse, linux-mtd, Roland Stigge + 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/5] mtd: nand: lpc32xx_slc: fix calculation of timing arcs from given values 2015-09-30 21:12 ` Brian Norris @ 2015-09-30 21:13 ` Brian Norris 2015-09-30 21:41 ` Vladimir Zapolskiy 2015-09-30 21:17 ` Vladimir Zapolskiy 1 sibling, 1 reply; 8+ messages in thread From: Brian Norris @ 2015-09-30 21:13 UTC (permalink / raw) To: Vladimir Zapolskiy; +Cc: David Woodhouse, linux-mtd, Roland Stigge (snip patch) Also, why [PATCH 1/5]? I don't see the other 4. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/5] mtd: nand: lpc32xx_slc: fix calculation of timing arcs from given values 2015-09-30 21:13 ` Brian Norris @ 2015-09-30 21:41 ` Vladimir Zapolskiy 2015-09-30 21:48 ` Brian Norris 0 siblings, 1 reply; 8+ messages in thread From: Vladimir Zapolskiy @ 2015-09-30 21:41 UTC (permalink / raw) To: Brian Norris; +Cc: David Woodhouse, linux-mtd, Roland Stigge 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/5] mtd: nand: lpc32xx_slc: fix calculation of timing arcs from given values 2015-09-30 21:41 ` Vladimir Zapolskiy @ 2015-09-30 21:48 ` Brian Norris 0 siblings, 0 replies; 8+ messages in thread From: Brian Norris @ 2015-09-30 21:48 UTC (permalink / raw) To: Vladimir Zapolskiy; +Cc: David Woodhouse, linux-mtd, Roland Stigge 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/5] mtd: nand: lpc32xx_slc: fix calculation of timing arcs from given values 2015-09-30 21:12 ` Brian Norris 2015-09-30 21:13 ` Brian Norris @ 2015-09-30 21:17 ` Vladimir Zapolskiy 2015-09-30 21:46 ` Brian Norris 1 sibling, 1 reply; 8+ messages in thread From: Vladimir Zapolskiy @ 2015-09-30 21:17 UTC (permalink / raw) To: Brian Norris; +Cc: David Woodhouse, linux-mtd, Roland Stigge 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. -- With best wishes, Vladimir ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/5] mtd: nand: lpc32xx_slc: fix calculation of timing arcs from given values 2015-09-30 21:17 ` Vladimir Zapolskiy @ 2015-09-30 21:46 ` Brian Norris 2015-09-30 22:09 ` Vladimir Zapolskiy 0 siblings, 1 reply; 8+ messages in thread From: Brian Norris @ 2015-09-30 21:46 UTC (permalink / raw) To: Vladimir Zapolskiy; +Cc: David Woodhouse, linux-mtd, Roland Stigge 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/5] mtd: nand: lpc32xx_slc: fix calculation of timing arcs from given values 2015-09-30 21:46 ` Brian Norris @ 2015-09-30 22:09 ` Vladimir Zapolskiy 0 siblings, 0 replies; 8+ messages in thread From: Vladimir Zapolskiy @ 2015-09-30 22:09 UTC (permalink / raw) To: Brian Norris; +Cc: David Woodhouse, linux-mtd, Roland Stigge On 01.10.2015 00:46, Brian Norris wrote: > 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. Right, your understanding is correct. > I can take it, if you'd like. I'd like an ack from > Roland if possible though. Sure. > 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) | > ...; I think this is a valid correction of the corner case. And actually I would prefer to grab it, plus I found another overflow problem in SLCTAC_*(n) definitions, so I'll send v2 shortly. -- With best wishes, Vladimir ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2015-09-30 22:09 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-09-28 16:38 [PATCH 1/5] mtd: nand: lpc32xx_slc: fix calculation of timing arcs from given values Vladimir Zapolskiy 2015-09-30 21:12 ` Brian Norris 2015-09-30 21:13 ` Brian Norris 2015-09-30 21:41 ` Vladimir Zapolskiy 2015-09-30 21:48 ` Brian Norris 2015-09-30 21:17 ` Vladimir Zapolskiy 2015-09-30 21:46 ` Brian Norris 2015-09-30 22:09 ` Vladimir Zapolskiy
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.