On 03/08/2017 06:26 PM, Ley Foon Tan wrote: > Rename some of variables and fixes on clock calculation. >
Why? Please be more specific with why this change is necessary? > Signed-off-by: Tien Fong Chee <[email protected]> > Signed-off-by: Ley Foon Tan <[email protected]> > --- > arch/arm/mach-socfpga/clock_manager_gen5.c | 126 > +++++++++++++++++------------ > 1 file changed, 76 insertions(+), 50 deletions(-) > > diff --git a/arch/arm/mach-socfpga/clock_manager_gen5.c > b/arch/arm/mach-socfpga/clock_manager_gen5.c > index bca27df..bef0adb 100644 > --- a/arch/arm/mach-socfpga/clock_manager_gen5.c > +++ b/arch/arm/mach-socfpga/clock_manager_gen5.c > @@ -1,5 +1,5 @@ > /* > - * Copyright (C) 2013-2017 Altera Corporation <www.altera.com> > + * Copyright (C) 2013-2017 Altera Corporation <www.altera.com> > * > * SPDX-License-Identifier: GPL-2.0+ > */ > @@ -66,7 +66,6 @@ static void cm_write_with_phase(u32 value, > * set source main and peripheral clocks > * Ungate clocks > */ > - > void cm_basic_init(const struct cm_config * const cfg) > { > unsigned long end; > @@ -307,56 +306,73 @@ void cm_basic_init(const struct cm_config * const cfg) > > static unsigned int cm_get_main_vco_clk_hz(void) > { > - u32 reg, clock; > + u32 src_hz, numer, denom, vco; > > /* get the main VCO clock */ > - reg = readl(&clock_manager_base->main_pll.vco); > - clock = cm_get_osc_clk_hz(1); > - clock /= ((reg & CLKMGR_MAINPLLGRP_VCO_DENOM_MASK) >> > - CLKMGR_MAINPLLGRP_VCO_DENOM_OFFSET) + 1; > - clock *= ((reg & CLKMGR_MAINPLLGRP_VCO_NUMER_MASK) >> > - CLKMGR_MAINPLLGRP_VCO_NUMER_OFFSET) + 1; > + vco = readl(&clock_manager_base->main_pll.vco); > > - return clock; > + numer = ((vco & CLKMGR_MAINPLLGRP_VCO_NUMER_MASK) >> > + CLKMGR_MAINPLLGRP_VCO_NUMER_OFFSET); > + denom = ((vco & CLKMGR_MAINPLLGRP_VCO_DENOM_MASK) >> > + CLKMGR_MAINPLLGRP_VCO_DENOM_OFFSET); > + > + src_hz = cm_get_osc_clk_hz(1); > + > + vco = src_hz; > + vco /= (1 + denom); > + vco *= (1 + numer); > + > + return vco; It seems like this change is beyond just "Rename some of variables and fixes on clock calculation". Could probably be it's own patch? > } > > static unsigned int cm_get_per_vco_clk_hz(void) > { > - u32 reg, clock = 0; > + u32 src_hz = 0; > + u32 clk_src = 0; > + u32 numer = 0; > + u32 denom = 0; > + u32 vco = 0; > > /* identify PER PLL clock source */ > - reg = readl(&clock_manager_base->per_pll.vco); > - reg = (reg & CLKMGR_PERPLLGRP_VCO_SSRC_MASK) >> > + clk_src = readl(&clock_manager_base->per_pll.vco); > + clk_src = (clk_src & CLKMGR_PERPLLGRP_VCO_SSRC_MASK) >> > CLKMGR_PERPLLGRP_VCO_SSRC_OFFSET; > - if (reg == CLKMGR_VCO_SSRC_EOSC1) > - clock = cm_get_osc_clk_hz(1); > - else if (reg == CLKMGR_VCO_SSRC_EOSC2) > - clock = cm_get_osc_clk_hz(2); > - else if (reg == CLKMGR_VCO_SSRC_F2S) > - clock = cm_get_f2s_per_ref_clk_hz(); > + if (clk_src == CLKMGR_VCO_SSRC_EOSC1) > + src_hz = cm_get_osc_clk_hz(1); > + else if (clk_src == CLKMGR_VCO_SSRC_EOSC2) > + src_hz = cm_get_osc_clk_hz(2); > + else if (clk_src == CLKMGR_VCO_SSRC_F2S) > + src_hz = cm_get_f2s_per_ref_clk_hz(); > > /* get the PER VCO clock */ > - reg = readl(&clock_manager_base->per_pll.vco); > - clock /= ((reg & CLKMGR_PERPLLGRP_VCO_DENOM_MASK) >> > - CLKMGR_PERPLLGRP_VCO_DENOM_OFFSET) + 1; > - clock *= ((reg & CLKMGR_PERPLLGRP_VCO_NUMER_MASK) >> > - CLKMGR_PERPLLGRP_VCO_NUMER_OFFSET) + 1; > + vco = readl(&clock_manager_base->per_pll.vco); > > - return clock; > + numer = (vco & CLKMGR_PERPLLGRP_VCO_NUMER_MASK) >> > + CLKMGR_PERPLLGRP_VCO_NUMER_OFFSET; > + > + denom = (vco & CLKMGR_PERPLLGRP_VCO_DENOM_MASK) >> > + CLKMGR_PERPLLGRP_VCO_DENOM_OFFSET; > + > + vco = src_hz; > + vco /= (1 + denom); > + vco *= (1 + numer); > + > + return vco; > } Ditto.. > > unsigned long cm_get_mpu_clk_hz(void) > { > - u32 reg, clock; > + u32 reg, clk_hz; > > - clock = cm_get_main_vco_clk_hz(); > + clk_hz = cm_get_main_vco_clk_hz(); > > /* get the MPU clock */ > reg = readl(&clock_manager_base->altera.mpuclk); > - clock /= (reg + 1); > + clk_hz /= (reg + 1); > reg = readl(&clock_manager_base->main_pll.mpuclk); > - clock /= (reg + 1); > - return clock; > + clk_hz /= (reg + 1); > + > + return clk_hz; > } > > unsigned long cm_get_sdram_clk_hz(void) > @@ -392,7 +408,8 @@ unsigned long cm_get_sdram_clk_hz(void) > > unsigned int cm_get_l4_sp_clk_hz(void) > { > - u32 reg, clock = 0; > + u32 clock = 0; > + u32 reg; > > /* identify the source of L4 SP clock */ > reg = readl(&clock_manager_base->main_pll.l4src); > @@ -426,37 +443,45 @@ unsigned int cm_get_l4_sp_clk_hz(void) > > unsigned int cm_get_mmc_controller_clk_hz(void) > { > - u32 reg, clock = 0; > + u32 clk_hz = 0; > + u32 clk_input = 0; > > - /* identify the source of MMC clock */ > - reg = readl(&clock_manager_base->per_pll.src); > - reg = (reg & CLKMGR_PERPLLGRP_SRC_SDMMC_MASK) >> > + clk_input = readl(&clock_manager_base->per_pll.src); > + clk_input = (clk_input & CLKMGR_PERPLLGRP_SRC_SDMMC_MASK) >> > CLKMGR_PERPLLGRP_SRC_SDMMC_OFFSET; > > - if (reg == CLKMGR_SDMMC_CLK_SRC_F2S) { > - clock = cm_get_f2s_per_ref_clk_hz(); > - } else if (reg == CLKMGR_SDMMC_CLK_SRC_MAIN) { > - clock = cm_get_main_vco_clk_hz(); > + switch (clk_input) { > + case CLKMGR_SDMMC_CLK_SRC_F2S: > + clk_hz = cm_get_f2s_per_ref_clk_hz(); > + break; > + > + case CLKMGR_SDMMC_CLK_SRC_MAIN: > + clk_hz = cm_get_main_vco_clk_hz(); > > /* get the SDMMC clock */ > - reg = readl(&clock_manager_base->main_pll.mainnandsdmmcclk); > - clock /= (reg + 1); > - } else if (reg == CLKMGR_SDMMC_CLK_SRC_PER) { > - clock = cm_get_per_vco_clk_hz(); > + clk_input = > + readl(&clock_manager_base->main_pll.mainnandsdmmcclk); > + clk_hz /= (clk_input + 1); > + break; > + > + case CLKMGR_SDMMC_CLK_SRC_PER: > + clk_hz = cm_get_per_vco_clk_hz(); > > /* get the SDMMC clock */ > - reg = readl(&clock_manager_base->per_pll.pernandsdmmcclk); > - clock /= (reg + 1); > + clk_input = > + readl(&clock_manager_base->per_pll.pernandsdmmcclk); > + clk_hz /= (clk_input + 1); > + break; > } > > - /* further divide by 4 as we have fixed divider at wrapper */ > - clock /= 4; > - return clock; > + return clk_hz / 4; > } > > + > unsigned int cm_get_qspi_controller_clk_hz(void) > { > - u32 reg, clock = 0; > + u32 clock = 0; > + u32 reg; > Why is this necessary? > /* identify the source of QSPI clock */ > reg = readl(&clock_manager_base->per_pll.src); > @@ -484,7 +509,8 @@ unsigned int cm_get_qspi_controller_clk_hz(void) > > unsigned int cm_get_spi_controller_clk_hz(void) > { > - u32 reg, clock = 0; > + u32 clock = 0; > + u32 reg; > Ditto.. Dinh _______________________________________________ U-Boot mailing list [email protected] https://lists.denx.de/listinfo/u-boot
