[U-Boot,v4,2/7] rockchip: clk: rk3399: fix off-by one during rate calculation in i2c/spi_set_rate

Message ID 1492718755-14331-3-git-send-email-philipp.tomsich@theobroma-systems.com
State Accepted
Headers show

Commit Message

Philipp Tomsich April 20, 2017, 8:05 p.m.
For the RK3399, i2c_set_rate (and by extension: our spi_set_rate,
which had been mindlessly following the template of the i2c_set_rate
implementation) miscalculates the rate returned due to a off-by-one
error resulting from the following sequence of events:
  1. calculates 'src_div := src_freq / target_freq'
  2. stores 'src_div - 1' into the register (the actual divider applied
     in hardware is biased by adding 1)
  3. returns the result of the DIV_RATE(src_freq, src_div) macro, which
     expects the (decremented) divider from the hardware-register and
     implictly adds 1 (i.e. 'DIV_RATE(freq, div) := freq / (div + 1)')

This can be observed with the SPI driver, which sets a rate of 99MHz
based on the GPLL frequency of 594MHz: the hardware generates a clock
of 99MHz (src_div is 6, the bitfield in the register correctly reads 5),
but reports a frequency of 84MHz (594 / 7) on return.

To fix, we have two options:
 * either we bias (i.e. "DIV_RATE(GPLL, src_div - 1)"), which doesn't
   make for a particularily nice read
 * we simply call the i2c/spi_get_rate function (introducing additional
   overhead for the additional register-read), which reads the divider
   from the register and then passes it through the DIV_RATE macro

Given that this code is not time-critical, the more readable solution
(i.e. calling the appropriate get_rate function) is implemented in this
change.

Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
Tested-by: Klaus Goger <klaus.goger@theobroma-systems.com>
Acked-by: Simon Glass <sjg@chromium.org>

---

Changes in v4: None
Changes in v3: None
Changes in v2:
- fixes an off-by-one for the RK3399 that cause the SPI module input
  clock to be misstated as 84MHz (even though it was running at 99MHz)

 drivers/clk/rockchip/clk_rk3399.c | 5 ++---
 1 file changed, 2 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:
> For the RK3399, i2c_set_rate (and by extension: our spi_set_rate,
> which had been mindlessly following the template of the i2c_set_rate
> implementation) miscalculates the rate returned due to a off-by-one
> error resulting from the following sequence of events:
>   1. calculates 'src_div := src_freq / target_freq'
>   2. stores 'src_div - 1' into the register (the actual divider applied
>      in hardware is biased by adding 1)
>   3. returns the result of the DIV_RATE(src_freq, src_div) macro, which
>      expects the (decremented) divider from the hardware-register and
>      implictly adds 1 (i.e. 'DIV_RATE(freq, div) := freq / (div + 1)')
>
> This can be observed with the SPI driver, which sets a rate of 99MHz
> based on the GPLL frequency of 594MHz: the hardware generates a clock
> of 99MHz (src_div is 6, the bitfield in the register correctly reads 5),
> but reports a frequency of 84MHz (594 / 7) on return.
>
> To fix, we have two options:
>  * either we bias (i.e. "DIV_RATE(GPLL, src_div - 1)"), which doesn't
>    make for a particularily nice read
>  * we simply call the i2c/spi_get_rate function (introducing additional
>    overhead for the additional register-read), which reads the divider
>    from the register and then passes it through the DIV_RATE macro
>
> Given that this code is not time-critical, the more readable solution
> (i.e. calling the appropriate get_rate function) is implemented in this
> change.
>
> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
> Tested-by: Klaus Goger <klaus.goger@theobroma-systems.com>
> Acked-by: Simon Glass <sjg@chromium.org>
>
> ---
>
> Changes in v4: None
> Changes in v3: None
> Changes in v2:
> - fixes an off-by-one for the RK3399 that cause the SPI module input
>   clock to be misstated as 84MHz (even though it was running at 99MHz)
>
>  drivers/clk/rockchip/clk_rk3399.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)

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

Patch

diff --git a/drivers/clk/rockchip/clk_rk3399.c b/drivers/clk/rockchip/clk_rk3399.c
index da62757..ac658b9 100644
--- a/drivers/clk/rockchip/clk_rk3399.c
+++ b/drivers/clk/rockchip/clk_rk3399.c
@@ -606,7 +606,7 @@  static ulong rk3399_i2c_set_clk(struct rk3399_cru *cru, ulong clk_id, uint hz)
 		return -EINVAL;
 	}
 
-	return DIV_TO_RATE(GPLL_HZ, src_clk_div);
+	return rk3399_i2c_get_clk(cru, clk_id);
 }
 
 /*
@@ -695,8 +695,7 @@  static ulong rk3399_spi_set_clk(struct rk3399_cru *cru, ulong clk_id, uint hz)
 		     ((src_clk_div << spiclk->div_shift) |
 		      (CLK_SPI_PLL_SEL_GPLL << spiclk->sel_shift)));
 
-
-	return DIV_TO_RATE(GPLL_HZ, src_clk_div);
+	return rk3399_spi_get_clk(cru, clk_id);
 }
 
 static ulong rk3399_vop_set_clk(struct rk3399_cru *cru, ulong clk_id, u32 hz)