| Message ID | 100121725.2408596.1344967663150.JavaMail.root@advansee.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Stefano Babic |
| Headers | show |
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
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
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);
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(-)