[PATCH v2 0/3] mtd: nand: lpc32xx_slc: fix calculation of timing arcs from given values

[PATCH v2 0/3] mtd: nand: lpc32xx_slc: fix calculation of timing arcs from given values

[Vladimir Zapolskiy]

The changeset is intended to accelerata read/write speed of LPC32xx SLC
controller improving configuration of controller timing arcs.

Patch 2/3 is a defensive check against an unlikely case, when controller's
clock rate is too high.

Thanks to Brian Norris for review and the idea of v2 3/3 improvement.

Vladimir Zapolskiy (3):
  mtd: nand: lpc32xx_slc: improve SLCTAC_*() macro definitions
  mtd: nand: lpc32xx_slc: fix potential overflow over 4 bits
  mtd: nand: lpc32xx_slc: fix calculation of timing arcs from given values

 drivers/mtd/nand/lpc32xx_slc.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

-- 
2.1.4

[PATCH v2 1/3] mtd: nand: lpc32xx_slc: improve SLCTAC_*() macro definitions

[Vladimir Zapolskiy]

No functional change, move bitfield calculations to macro
definitions with added clock rate argument, which are in turn defined
by new common SLCTAC_CLOCKS(c, n, s) macro definition.

Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
---
Changes from v1 to v2:
* none, new change

 drivers/mtd/nand/lpc32xx_slc.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/drivers/mtd/nand/lpc32xx_slc.c b/drivers/mtd/nand/lpc32xx_slc.c
index abfec13..9ac0f3b 100644
--- a/drivers/mtd/nand/lpc32xx_slc.c
+++ b/drivers/mtd/nand/lpc32xx_slc.c
@@ -94,22 +94,25 @@
 /**********************************************************************
 * slc_tac register definitions
 **********************************************************************/
+/* Computation of clock cycles on basis of controller and device clock rates */
+#define SLCTAC_CLOCKS(c, n, s)	(((1 + (c / n)) & 0xF) << s)
+
 /* Clock setting for RDY write sample wait time in 2*n clocks */
 #define SLCTAC_WDR(n)		(((n) & 0xF) << 28)
 /* Write pulse width in clock cycles, 1 to 16 clocks */
-#define SLCTAC_WWIDTH(n)	(((n) & 0xF) << 24)
+#define SLCTAC_WWIDTH(c, n)	(SLCTAC_CLOCKS(c, n, 24))
 /* Write hold time of control and data signals, 1 to 16 clocks */
-#define SLCTAC_WHOLD(n)		(((n) & 0xF) << 20)
+#define SLCTAC_WHOLD(c, n)	(SLCTAC_CLOCKS(c, n, 20))
 /* Write setup time of control and data signals, 1 to 16 clocks */
-#define SLCTAC_WSETUP(n)	(((n) & 0xF) << 16)
+#define SLCTAC_WSETUP(c, n)	(SLCTAC_CLOCKS(c, n, 16))
 /* Clock setting for RDY read sample wait time in 2*n clocks */
 #define SLCTAC_RDR(n)		(((n) & 0xF) << 12)
 /* Read pulse width in clock cycles, 1 to 16 clocks */
-#define SLCTAC_RWIDTH(n)	(((n) & 0xF) << 8)
+#define SLCTAC_RWIDTH(c, n)	(SLCTAC_CLOCKS(c, n, 8))
 /* Read hold time of control and data signals, 1 to 16 clocks */
-#define SLCTAC_RHOLD(n)		(((n) & 0xF) << 4)
+#define SLCTAC_RHOLD(c, n)	(SLCTAC_CLOCKS(c, n, 4))
 /* Read setup time of control and data signals, 1 to 16 clocks */
-#define SLCTAC_RSETUP(n)	(((n) & 0xF) << 0)
+#define SLCTAC_RSETUP(c, n)	(SLCTAC_CLOCKS(c, n, 0))
 
 /**********************************************************************
 * slc_ecc register definitions
@@ -240,13 +243,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.1.4

Re: [PATCH v2 1/3] mtd: nand: lpc32xx_slc: improve SLCTAC_*() macro definitions

[Vladimir Zapolskiy]

On 01.10.2015 02:23, Vladimir Zapolskiy wrote:
> No functional change, move bitfield calculations to macro
> definitions with added clock rate argument, which are in turn defined
> by new common SLCTAC_CLOCKS(c, n, s) macro definition.
> 
> Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
> ---
> Changes from v1 to v2:
> * none, new change
> 
>  drivers/mtd/nand/lpc32xx_slc.c | 27 +++++++++++++++------------
>  1 file changed, 15 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/mtd/nand/lpc32xx_slc.c b/drivers/mtd/nand/lpc32xx_slc.c
> index abfec13..9ac0f3b 100644
> --- a/drivers/mtd/nand/lpc32xx_slc.c
> +++ b/drivers/mtd/nand/lpc32xx_slc.c
> @@ -94,22 +94,25 @@
>  /**********************************************************************
>  * slc_tac register definitions
>  **********************************************************************/
> +/* Computation of clock cycles on basis of controller and device clock rates */
> +#define SLCTAC_CLOCKS(c, n, s)	(((1 + (c / n)) & 0xF) << s)
> +
>  /* Clock setting for RDY write sample wait time in 2*n clocks */
>  #define SLCTAC_WDR(n)		(((n) & 0xF) << 28)
>  /* Write pulse width in clock cycles, 1 to 16 clocks */
> -#define SLCTAC_WWIDTH(n)	(((n) & 0xF) << 24)
> +#define SLCTAC_WWIDTH(c, n)	(SLCTAC_CLOCKS(c, n, 24))
>  /* Write hold time of control and data signals, 1 to 16 clocks */
> -#define SLCTAC_WHOLD(n)		(((n) & 0xF) << 20)
> +#define SLCTAC_WHOLD(c, n)	(SLCTAC_CLOCKS(c, n, 20))
>  /* Write setup time of control and data signals, 1 to 16 clocks */
> -#define SLCTAC_WSETUP(n)	(((n) & 0xF) << 16)
> +#define SLCTAC_WSETUP(c, n)	(SLCTAC_CLOCKS(c, n, 16))
>  /* Clock setting for RDY read sample wait time in 2*n clocks */
>  #define SLCTAC_RDR(n)		(((n) & 0xF) << 12)
>  /* Read pulse width in clock cycles, 1 to 16 clocks */
> -#define SLCTAC_RWIDTH(n)	(((n) & 0xF) << 8)
> +#define SLCTAC_RWIDTH(c, n)	(SLCTAC_CLOCKS(c, n, 8))
>  /* Read hold time of control and data signals, 1 to 16 clocks */
> -#define SLCTAC_RHOLD(n)		(((n) & 0xF) << 4)
> +#define SLCTAC_RHOLD(c, n)	(SLCTAC_CLOCKS(c, n, 4))
>  /* Read setup time of control and data signals, 1 to 16 clocks */
> -#define SLCTAC_RSETUP(n)	(((n) & 0xF) << 0)
> +#define SLCTAC_RSETUP(c, n)	(SLCTAC_CLOCKS(c, n, 0))
>  

Anticipating a question about not parenthesized arguments, I don't know,
if more parentheses around arguments in these macro definitions are
needed, because there is no other potential usage of them.
Please let me know, if you think different, I'll fix it.

--
With best wishes,
Vladimir

[PATCH v2 2/3] mtd: nand: lpc32xx_slc: fix potential overflow over 4 bits

[Vladimir Zapolskiy]

In case if quotient of controller clock rate to device clock rate does
not fit into 4 bit value, choose the maximum acceptable value 0xF, which
stands for 16 clocks.

Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
---
Changes from v1 to v2:
* none, new change

 drivers/mtd/nand/lpc32xx_slc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/lpc32xx_slc.c b/drivers/mtd/nand/lpc32xx_slc.c
index 9ac0f3b..a9e8a02 100644
--- a/drivers/mtd/nand/lpc32xx_slc.c
+++ b/drivers/mtd/nand/lpc32xx_slc.c
@@ -95,7 +95,7 @@
 * slc_tac register definitions
 **********************************************************************/
 /* Computation of clock cycles on basis of controller and device clock rates */
-#define SLCTAC_CLOCKS(c, n, s)	(((1 + (c / n)) & 0xF) << s)
+#define SLCTAC_CLOCKS(c, n, s)	(min_t(u32, 1 + (c / n), 0xF) << s)
 
 /* Clock setting for RDY write sample wait time in 2*n clocks */
 #define SLCTAC_WDR(n)		(((n) & 0xF) << 28)
-- 
2.1.4

[PATCH v2 3/3] mtd: nand: lpc32xx_slc: fix calculation of timing arcs from given values

[Vladimir Zapolskiy]

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, the current version of calculated clock cycles is too
conservative.

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>
---
Changes from v2 to v1:
* use less conservative "DIV_ROUND_UP(c, n) - 1" construction instead of "c / n"

 drivers/mtd/nand/lpc32xx_slc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/lpc32xx_slc.c b/drivers/mtd/nand/lpc32xx_slc.c
index a9e8a02..cbf4501 100644
--- a/drivers/mtd/nand/lpc32xx_slc.c
+++ b/drivers/mtd/nand/lpc32xx_slc.c
@@ -95,7 +95,7 @@
 * slc_tac register definitions
 **********************************************************************/
 /* Computation of clock cycles on basis of controller and device clock rates */
-#define SLCTAC_CLOCKS(c, n, s)	(min_t(u32, 1 + (c / n), 0xF) << s)
+#define SLCTAC_CLOCKS(c, n, s)	(min_t(u32, DIV_ROUND_UP(c, n) - 1, 0xF) << s)
 
 /* Clock setting for RDY write sample wait time in 2*n clocks */
 #define SLCTAC_WDR(n)		(((n) & 0xF) << 28)
-- 
2.1.4

Re: [PATCH v2 0/3] mtd: nand: lpc32xx_slc: fix calculation of timing arcs from given values

[Brian Norris]

On Thu, Oct 01, 2015 at 02:23:34AM +0300, Vladimir Zapolskiy wrote:
> The changeset is intended to accelerata read/write speed of LPC32xx SLC
> controller improving configuration of controller timing arcs.
> 
> Patch 2/3 is a defensive check against an unlikely case, when controller's
> clock rate is too high.
> 
> Thanks to Brian Norris for review and the idea of v2 3/3 improvement.
> 
> Vladimir Zapolskiy (3):
>   mtd: nand: lpc32xx_slc: improve SLCTAC_*() macro definitions
>   mtd: nand: lpc32xx_slc: fix potential overflow over 4 bits
>   mtd: nand: lpc32xx_slc: fix calculation of timing arcs from given values

All looks good to me. I won't hold my breath for Roland. Pushed to
l2-mtd.git, and if we get any further comments, we can address them
then.

Thanks,
Brian