[U-Boot] [U-Boot,4/4] rockchip: clk: remove RATE_TO_DIV

[Philipp Tomsich]

On Wed, 26 Jul 2017, Kever Yang wrote:

> Use DIV_ROUND_UP instead RATE_TO_DIV for all Rockchip SoC
> clock driver.

I think nobody is happier than me to see this go ;-)
However, there's a few comments below.

> Signed-off-by: Kever Yang <kever.yang at rock-chips.com>
> Acked-by: Philipp Tomsich <philipp.tomsich at theobroma-systems.com>
> ---
>
> drivers/clk/rockchip/clk_rk3188.c | 7 ++-----
> drivers/clk/rockchip/clk_rk322x.c | 3 ---
> drivers/clk/rockchip/clk_rk3288.c | 5 +----
> drivers/clk/rockchip/clk_rk3368.c | 7 ++-----
> drivers/clk/rockchip/clk_rk3399.c | 2 +-
> drivers/clk/rockchip/clk_rv1108.c | 3 ---
> 6 files changed, 6 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/clk/rockchip/clk_rk3188.c b/drivers/clk/rockchip/clk_rk3188.c
> index ecfd840..e1f0b65 100644
> --- a/drivers/clk/rockchip/clk_rk3188.c
> +++ b/drivers/clk/rockchip/clk_rk3188.c
> @@ -71,9 +71,6 @@ enum {
> 	SOCSTS_GPLL_LOCK	= 1 << 8,
> };
>
> -#define RATE_TO_DIV(input_rate, output_rate) \
> -	((input_rate) / (output_rate) - 1);
> -
> #define DIV_TO_RATE(input_rate, div)	((input_rate) / ((div) + 1))
>
> #define PLL_DIVISORS(hz, _nr, _no) {\
> @@ -350,7 +347,7 @@ static ulong rockchip_spi_get_clk(struct rk3188_cru *cru, uint gclk_rate,
> static ulong rockchip_spi_set_clk(struct rk3188_cru *cru, uint gclk_rate,
> 				  int periph, uint freq)
> {
> -	int src_clk_div = RATE_TO_DIV(gclk_rate, freq);
> +	int src_clk_div = DIV_ROUND_UP(gclk_rate, freq) - 1;
>
> 	switch (periph) {
> 	case SCLK_SPI0:
> @@ -400,7 +397,7 @@ static void rkclk_init(struct rk3188_cru *cru, struct rk3188_grf *grf,
> 	 * reparent aclk_cpu_pre from apll to gpll
> 	 * set up dependent divisors for PCLK/HCLK and ACLK clocks.
> 	 */
> -	aclk_div = RATE_TO_DIV(GPLL_HZ, CPU_ACLK_HZ);
> +	aclk_div = DIV_ROUND_UP(GPLL_HZ, CPU_ACLK_HZ);
> 	assert((aclk_div + 1) * CPU_ACLK_HZ == GPLL_HZ && aclk_div < 0x1f);
>
> 	rk_clrsetreg(&cru->cru_clksel_con[0],
> diff --git a/drivers/clk/rockchip/clk_rk322x.c b/drivers/clk/rockchip/clk_rk322x.c
> index b50a852..43b3f4b 100644
> --- a/drivers/clk/rockchip/clk_rk322x.c
> +++ b/drivers/clk/rockchip/clk_rk322x.c
> @@ -26,9 +26,6 @@ enum {
> 	OUTPUT_MIN_HZ	= 24 * 1000000,
> };
>
> -#define RATE_TO_DIV(input_rate, output_rate) \
> -	((input_rate) / (output_rate) - 1);
> -
> #define DIV_TO_RATE(input_rate, div)	((input_rate) / ((div) + 1))
>
> #define PLL_DIVISORS(hz, _refdiv, _postdiv1, _postdiv2) {\
> diff --git a/drivers/clk/rockchip/clk_rk3288.c b/drivers/clk/rockchip/clk_rk3288.c
> index adcc0a6..8f6df55 100644
> --- a/drivers/clk/rockchip/clk_rk3288.c
> +++ b/drivers/clk/rockchip/clk_rk3288.c
> @@ -118,9 +118,6 @@ enum {
> 	SOCSTS_NPLL_LOCK	= 1 << 9,
> };
>
> -#define RATE_TO_DIV(input_rate, output_rate) \
> -	((input_rate) / (output_rate) - 1);
> -
> #define DIV_TO_RATE(input_rate, div)	((input_rate) / ((div) + 1))
>
> #define PLL_DIVISORS(hz, _nr, _no) {\
> @@ -608,7 +605,7 @@ static ulong rockchip_spi_set_clk(struct rk3288_cru *cru, uint gclk_rate,
> 	int src_clk_div;
>
> 	debug("%s: clk_general_rate=%u\n", __func__, gclk_rate);
> -	src_clk_div = RATE_TO_DIV(gclk_rate, freq);
> +	src_clk_div = DIV_ROUND_UP(gclk_rate, freq) - 1;

Shouldn't you check that this didn't overflow the div-field?

> 	switch (periph) {
> 	case SCLK_SPI0:
> 		rk_clrsetreg(&cru->cru_clksel_con[25],
> diff --git a/drivers/clk/rockchip/clk_rk3368.c b/drivers/clk/rockchip/clk_rk3368.c
> index 52cad38..c067fa1 100644
> --- a/drivers/clk/rockchip/clk_rk3368.c
> +++ b/drivers/clk/rockchip/clk_rk3368.c
> @@ -30,9 +30,6 @@ struct pll_div {
> #define GPLL_HZ		(576 * 1000 * 1000)
> #define CPLL_HZ		(400 * 1000 * 1000)
>
> -#define RATE_TO_DIV(input_rate, output_rate) \
> -		((input_rate) / (output_rate) - 1);
> -
> #define DIV_TO_RATE(input_rate, div)    ((input_rate) / ((div) + 1))
>
> #define PLL_DIVISORS(hz, _nr, _no) { \
> @@ -171,7 +168,7 @@ static ulong rk3368_mmc_set_clk(struct rk3368_cru *cru,
> 	u32 con_id;
> 	u32 gpll_rate = rkclk_pll_get_rate(cru, GPLL);
>
> -	div = RATE_TO_DIV(gpll_rate, rate << 1);
> +	div = DIV_ROUND_UP(gpll_rate, rate << 1) - 1;
>
> 	switch (clk_id) {
> 	case SCLK_SDMMC:
> @@ -188,7 +185,7 @@ static ulong rk3368_mmc_set_clk(struct rk3368_cru *cru,
> 	}
>
> 	if (div > 0x3f) {
> -		div = RATE_TO_DIV(OSC_HZ, rate);
> +		div = DIV_ROUND_UP(OSC_HZ, rate) - 1;

This does not proect us against overflowing the div-field.
Shouldn't you check whether the divider fits into the field (i.e. max is 
0x3f) and clamp it (i.e. use saturated arithmetic)?

Let's assume someone requested a rate of 369kHz, then you'd set a divider 
or 1 (or 2?) instead of 64 (which is the closest representable one and 
would yield 375kHz).

> 		rk_clrsetreg(&cru->clksel_con[con_id],
> 			     MMC_PLL_SEL_MASK | MMC_CLK_DIV_MASK,
> 			     (MMC_PLL_SEL_24M << MMC_PLL_SEL_SHIFT) |
> diff --git a/drivers/clk/rockchip/clk_rk3399.c b/drivers/clk/rockchip/clk_rk3399.c
> index 40cbfea..e3f663a 100644
> --- a/drivers/clk/rockchip/clk_rk3399.c
> +++ b/drivers/clk/rockchip/clk_rk3399.c
> @@ -676,7 +676,7 @@ static ulong rk3399_spi_set_clk(struct rk3399_cru *cru, ulong clk_id, uint hz)
> 	const struct spi_clkreg *spiclk = NULL;
> 	int src_clk_div;
>
> -	src_clk_div = RATE_TO_DIV(GPLL_HZ, hz);
> +	src_clk_div = DIV_ROUND_UP(GPLL_HZ, hz) - 1;
> 	assert(src_clk_div < 127);
>
> 	switch (clk_id) {
> diff --git a/drivers/clk/rockchip/clk_rv1108.c b/drivers/clk/rockchip/clk_rv1108.c
> index 818293d..cf966bb 100644
> --- a/drivers/clk/rockchip/clk_rv1108.c
> +++ b/drivers/clk/rockchip/clk_rv1108.c
> @@ -25,9 +25,6 @@ enum {
> 	OUTPUT_MIN_HZ	= 24 * 1000000,
> };
>
> -#define RATE_TO_DIV(input_rate, output_rate) \
> -	((input_rate) / (output_rate) - 1);
> -
> #define DIV_TO_RATE(input_rate, div)	((input_rate) / ((div) + 1))
>
> #define PLL_DIVISORS(hz, _refdiv, _postdiv1, _postdiv2) {\
>