[U-Boot,v4,4/7] rockchip: spi: rewrite rkspi_set_clk for a more conservative baudrate setting

Message ID 1492718755-14331-5-git-send-email-philipp.tomsich@theobroma-systems.com
State Accepted
Commit 9fc354e246fe466ad634617e12d092d6053503ed
Headers show

Commit Message

Philipp Tomsich April 20, 2017, 8:05 p.m.
The baudrate in rkspi was calculated by using an integer division
(which implicitly discarded any fractional result), then rounding to
an even number and finally clamping to 0xfffe using a bitwise AND
operator.  This introduced two issues:
1) for very small baudrates (overflowing the 0xfffe range), the
   bitwise-AND generates rather random-looking (wildly varying)
   actual output bitrates
2) for higher baudrates, the calculation tends to 'err towards a
   higher baudrate' with the actual error increasing as the dividers
   become very small. E.g., with a 99MHz input clock, a request
   for a 20MBit baudrate (99/20 = 4.95), a 24.75 MBit would be use
   (which amounts to a 23.75% error)... for a 34 MBit request this
   would be an actual outbout of 49.5 Mbit (i.e. a 45% error).

This change rewrites the divider selection (i.e. baudrate calculation)
by making sure that
a) for the normal case: the largest representable baudrate below the
   requested rate will be chosen;
b) for the denormal case (i.e. when the divider can no longer be
   represented), the lowest representable baudrate is chosen.

Even though the denormal case (b) may be of little concern in real
world applications (even with a 198MHz input clock, this will only
happen at below approx. 3kHz/3kBit), our board-verification team kept
complaining.

Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
Tested-by: Klaus Goger <klaus.goger@theobroma-systems.com>
---

Changes in v4:
- added in v4 after receiving complaints from the board-verification
  team

Changes in v3: None
Changes in v2: None

 drivers/spi/rk_spi.c | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

Comments

Simon Glass April 20, 2017, 9:05 p.m. | #1
On 20 April 2017 at 14:05, Philipp Tomsich
<philipp.tomsich@theobroma-systems.com> wrote:
> The baudrate in rkspi was calculated by using an integer division
> (which implicitly discarded any fractional result), then rounding to
> an even number and finally clamping to 0xfffe using a bitwise AND
> operator.  This introduced two issues:
> 1) for very small baudrates (overflowing the 0xfffe range), the
>    bitwise-AND generates rather random-looking (wildly varying)
>    actual output bitrates
> 2) for higher baudrates, the calculation tends to 'err towards a
>    higher baudrate' with the actual error increasing as the dividers
>    become very small. E.g., with a 99MHz input clock, a request
>    for a 20MBit baudrate (99/20 = 4.95), a 24.75 MBit would be use
>    (which amounts to a 23.75% error)... for a 34 MBit request this
>    would be an actual outbout of 49.5 Mbit (i.e. a 45% error).
>
> This change rewrites the divider selection (i.e. baudrate calculation)
> by making sure that
> a) for the normal case: the largest representable baudrate below the
>    requested rate will be chosen;
> b) for the denormal case (i.e. when the divider can no longer be
>    represented), the lowest representable baudrate is chosen.
>
> Even though the denormal case (b) may be of little concern in real
> world applications (even with a 198MHz input clock, this will only
> happen at below approx. 3kHz/3kBit), our board-verification team kept
> complaining.
>
> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
> Tested-by: Klaus Goger <klaus.goger@theobroma-systems.com>
> ---
>
> Changes in v4:
> - added in v4 after receiving complaints from the board-verification
>   team
>
> Changes in v3: None
> Changes in v2: None
>
>  drivers/spi/rk_spi.c | 25 ++++++++++++++++++++++---
>  1 file changed, 22 insertions(+), 3 deletions(-)

Applied to u-boot-rockchip/next, thanks!

Patch

diff --git a/drivers/spi/rk_spi.c b/drivers/spi/rk_spi.c
index 0c627d9..050eacb 100644
--- a/drivers/spi/rk_spi.c
+++ b/drivers/spi/rk_spi.c
@@ -79,12 +79,31 @@  static void rkspi_enable_chip(struct rockchip_spi *regs, bool enable)
 
 static void rkspi_set_clk(struct rockchip_spi_priv *priv, uint speed)
 {
-	uint clk_div;
+	/*
+	 * We should try not to exceed the speed requested by the caller:
+	 * when selecting a divider, we need to make sure we round up.
+	 */
+	uint clk_div = DIV_ROUND_UP(priv->input_rate, speed);
+
+	/* The baudrate register (BAUDR) is defined as a 32bit register where
+	 * the upper 16bit are reserved and having 'Fsclk_out' in the lower
+	 * 16bits with 'Fsclk_out' defined as follows:
+	 *
+	 *   Fsclk_out = Fspi_clk/ SCKDV
+	 *   Where SCKDV is any even value between 2 and 65534.
+	 */
+	if (clk_div > 0xfffe) {
+		clk_div = 0xfffe;
+		debug("%s: can't divide down to %d hz (actual will be %d hz)\n",
+		      __func__, speed, priv->input_rate / clk_div);
+	}
+
+	/* Round up to the next even 16bit number */
+	clk_div = (clk_div + 1) & 0xfffe;
 
-	clk_div = clk_get_divisor(priv->input_rate, speed);
 	debug("spi speed %u, div %u\n", speed, clk_div);
 
-	writel(clk_div, &priv->regs->baudr);
+	clrsetbits_le32(&priv->regs->baudr, 0xffff, clk_div);
 	priv->last_speed_hz = speed;
 }