Patchwork [U-Boot,07/10] mx5 clocks: Fix get_uart_clk()

login
register
mail settings
Submitter Benoît Thébaudeau
Date Aug. 14, 2012, 6:07 p.m.
Message ID <100121725.2408596.1344967663150.JavaMail.root@advansee.com>
Download mbox | patch
Permalink /patch/177408/
State Changes Requested
Delegated to: Stefano Babic
Headers show

Comments

Benoît Thébaudeau - Aug. 14, 2012, 6:07 p.m.
Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com>
Cc: Stefano Babic <sbabic@denx.de>
---
 .../arch/arm/cpu/armv7/mx5/clock.c                 |   45 ++++++++++++--------
 1 file changed, 27 insertions(+), 18 deletions(-)
Stefano Babic - Aug. 20, 2012, 9:52 a.m.
On 14/08/2012 20:07, Benoît Thébaudeau wrote:
> Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com>
> Cc: Stefano Babic <sbabic@denx.de>
> ---

Hi Benoît,

>  .../arch/arm/cpu/armv7/mx5/clock.c                 |   45 ++++++++++++--------
>  1 file changed, 27 insertions(+), 18 deletions(-)
> 

You should add in the commit message which is the bug you found and fixed.

> diff --git u-boot-4d3c95f.orig/arch/arm/cpu/armv7/mx5/clock.c u-boot-4d3c95f/arch/arm/cpu/armv7/mx5/clock.c
> index b13c55a..75a6eae 100644
> --- u-boot-4d3c95f.orig/arch/arm/cpu/armv7/mx5/clock.c
> +++ u-boot-4d3c95f/arch/arm/cpu/armv7/mx5/clock.c
> @@ -378,31 +378,40 @@ static u32 get_ipg_per_clk(void)
>  	return freq / ((pred1 + 1) * (pred2 + 1) * (podf + 1));
>  }
>  
> +/* Get the output clock rate of a standard PLL MUX for peripherals. */
> +static u32 get_standard_pll_sel_clk(u32 clk_sel)

Why should we pass a parameter to this function ? Can it be used in
another context ? I do not think so, because it decodes the value in the
cscmr1 register.

I think it is better if you move the reading of the cscmr1 register
inside this function and you drop the unneeded parameter.

> +{
> +	u32 freq;
> +
> +	switch (clk_sel & 0x3) {
> +	case 0:
> +		freq = decode_pll(mxc_plls[PLL1_CLOCK], CONFIG_SYS_MX5_HCLK);
> +		break;
> +	case 1:
> +		freq = decode_pll(mxc_plls[PLL2_CLOCK], CONFIG_SYS_MX5_HCLK);
> +		break;
> +	case 2:
> +		freq = decode_pll(mxc_plls[PLL3_CLOCK], CONFIG_SYS_MX5_HCLK);
> +		break;
> +	case 3:
> +		freq = get_lp_apm();
> +		break;

Ok, this is the fix, describe it.

> +	}
> +
> +	return freq;
> +}
> +
>  /*
>   * Get the rate of uart clk.
>   */
>  static u32 get_uart_clk(void)
>  {
> -	unsigned int freq, reg, pred, podf;
> +	unsigned int clk_sel, freq, reg, pred, podf;
>  
>  	reg = __raw_readl(&mxc_ccm->cscmr1);
> -	switch ((reg & MXC_CCM_CSCMR1_UART_CLK_SEL_MASK) >>
> -		MXC_CCM_CSCMR1_UART_CLK_SEL_OFFSET) {
> -	case 0x0:
> -		freq = decode_pll(mxc_plls[PLL1_CLOCK],
> -				    CONFIG_SYS_MX5_HCLK);
> -		break;
> -	case 0x1:
> -		freq = decode_pll(mxc_plls[PLL2_CLOCK],
> -				    CONFIG_SYS_MX5_HCLK);
> -		break;
> -	case 0x2:
> -		freq = decode_pll(mxc_plls[PLL3_CLOCK],
> -				    CONFIG_SYS_MX5_HCLK);
> -		break;
> -	default:
> -		return 66500000;
> -	}
> +	clk_sel = (reg & MXC_CCM_CSCMR1_UART_CLK_SEL_MASK) >>
> +			MXC_CCM_CSCMR1_UART_CLK_SEL_OFFSET;
> +	freq = get_standard_pll_sel_clk(clk_sel);
>  
>  	reg = __raw_readl(&mxc_ccm->cscdr1);
>  
> 

Best regards,
Stefano
Stefano Babic - Aug. 20, 2012, 10:05 a.m.
On 20/08/2012 11:52, Stefano Babic wrote:
> Why should we pass a parameter to this function ? Can it be used in
> another context ? I do not think so, because it decodes the value in the
> cscmr1 register.
> 
> I think it is better if you move the reading of the cscmr1 register
> inside this function and you drop the unneeded parameter.

Never mind - I had not yet read the next patch. Discharge this comment.

Best regards,
Stefano

Patch

diff --git u-boot-4d3c95f.orig/arch/arm/cpu/armv7/mx5/clock.c u-boot-4d3c95f/arch/arm/cpu/armv7/mx5/clock.c
index b13c55a..75a6eae 100644
--- u-boot-4d3c95f.orig/arch/arm/cpu/armv7/mx5/clock.c
+++ u-boot-4d3c95f/arch/arm/cpu/armv7/mx5/clock.c
@@ -378,31 +378,40 @@  static u32 get_ipg_per_clk(void)
 	return freq / ((pred1 + 1) * (pred2 + 1) * (podf + 1));
 }
 
+/* Get the output clock rate of a standard PLL MUX for peripherals. */
+static u32 get_standard_pll_sel_clk(u32 clk_sel)
+{
+	u32 freq;
+
+	switch (clk_sel & 0x3) {
+	case 0:
+		freq = decode_pll(mxc_plls[PLL1_CLOCK], CONFIG_SYS_MX5_HCLK);
+		break;
+	case 1:
+		freq = decode_pll(mxc_plls[PLL2_CLOCK], CONFIG_SYS_MX5_HCLK);
+		break;
+	case 2:
+		freq = decode_pll(mxc_plls[PLL3_CLOCK], CONFIG_SYS_MX5_HCLK);
+		break;
+	case 3:
+		freq = get_lp_apm();
+		break;
+	}
+
+	return freq;
+}
+
 /*
  * Get the rate of uart clk.
  */
 static u32 get_uart_clk(void)
 {
-	unsigned int freq, reg, pred, podf;
+	unsigned int clk_sel, freq, reg, pred, podf;
 
 	reg = __raw_readl(&mxc_ccm->cscmr1);
-	switch ((reg & MXC_CCM_CSCMR1_UART_CLK_SEL_MASK) >>
-		MXC_CCM_CSCMR1_UART_CLK_SEL_OFFSET) {
-	case 0x0:
-		freq = decode_pll(mxc_plls[PLL1_CLOCK],
-				    CONFIG_SYS_MX5_HCLK);
-		break;
-	case 0x1:
-		freq = decode_pll(mxc_plls[PLL2_CLOCK],
-				    CONFIG_SYS_MX5_HCLK);
-		break;
-	case 0x2:
-		freq = decode_pll(mxc_plls[PLL3_CLOCK],
-				    CONFIG_SYS_MX5_HCLK);
-		break;
-	default:
-		return 66500000;
-	}
+	clk_sel = (reg & MXC_CCM_CSCMR1_UART_CLK_SEL_MASK) >>
+			MXC_CCM_CSCMR1_UART_CLK_SEL_OFFSET;
+	freq = get_standard_pll_sel_clk(clk_sel);
 
 	reg = __raw_readl(&mxc_ccm->cscdr1);