* [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: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: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: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: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: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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).