| Message ID | 1305101022-22546-1-git-send-email-jason.hui@linaro.org |
|---|---|
| State | Changes Requested |
| Delegated to: | Stefano Babic |
| Headers | show |
On 05/11/2011 10:03 AM, Jason Liu wrote: > diff --git a/arch/arm/cpu/armv7/mx5/clock.c b/arch/arm/cpu/armv7/mx5/clock.c > index 0b04a88..04d9f71 100644 > --- a/arch/arm/cpu/armv7/mx5/clock.c > +++ b/arch/arm/cpu/armv7/mx5/clock.c > @@ -24,6 +24,7 @@ > */ > > +#define AHB_CLK_ROOT 133333333 > +#define SZ_DEC_1M 1000000 Is this define used only to get the value in Mhz from the PLL clock ? If it is the case, the name is quite confusing, as it refers to a size. If it is not the case, please explain. > +#define PLL_PD_MAX 16 /* Actual pd+1 */ > +#define PLL_MFI_MAX 15 > +#define PLL_MFI_MIN 5 > +#define ARM_DIV_MAX 8 > +#define IPG_DIV_MAX 4 > +#define AHB_DIV_MAX 8 > +#define EMI_DIV_MAX 8 > +#define NFC_DIV_MAX 8 Definitions for clock registers are in the crm_regs.h file. These are the maximum values for some fields in the registers. Should they not be put inside the crm_regs.h ? > + > +struct fixed_pll_mfd { > + u32 ref_clk_hz; > + u32 mfd; > +}; > + > +const struct fixed_pll_mfd fixed_mfd[] = { > + {CONFIG_SYS_MX5_HCLK, 24 * 16}, > +}; Not understood the need of an array (I have not said it is wrong, simply I have not understood !). You use in the code (this is another issue) "ref" as parameter for your functions for the reference clock, but is seems to me that the only possible value is CONFIG_SYS_MX5_HCLK. Are there other use case for this array, that makes sense to define and maybe to extend it ? Can you add a reference to the manual explaining where these values are coming from ? > + > +struct pll_param { > + u32 pd; > + u32 mfi; > + u32 mfn; > + u32 mfd; > +}; > + > +#define PLL_FREQ_MAX(ref_clk) (4 * (ref_clk) * PLL_MFI_MAX) > +#define PLL_FREQ_MIN(ref_clk) \ > + ((2 * (ref_clk) * (PLL_MFI_MIN - 1)) / PLL_PD_MAX) I understand what it is done here, but only after I have finally found where it is described in the manual. Can you add useful comments here and reference to the manual, too ? Such as describing these values are for the registers DP_OP, DP_MFN and DP-MFD, and a reference to the formula (Eqn. 22-1) helps to understand it. > +#define MAX_DDR_CLK 420000000 > +#define NFC_CLK_MAX 34000000 Where do these values come from ? I understand they are computed values, depending on pll clock. It seems to me (at least for DDR clock) they are absolute maximum rates, but it could be that MAX_DDR_CLK could be set to a lower value depending on the PLL value. Is it correct ? In other words : should be possible to set a PLL (you provide an API for changing it) to a lower value, and then even the defines you set here do not correspond to the real maximum value ? > + > /* > * The API of get mxc clockes. > */ > @@ -245,10 +369,12 @@ unsigned int mxc_get_clock(enum mxc_clock clk) > case MXC_UART_CLK: > return get_uart_clk(); > case MXC_CSPI_CLK: > - return imx_get_cspiclk(); > + return get_cspi_clk(); > case MXC_FEC_CLK: > return decode_pll(mxc_plls[PLL1_CLOCK], > CONFIG_SYS_MX5_HCLK); > + case MXC_DDR_CLK: > + return get_ddr_clk(); You extended the enum for the clocks, as I see also MXC_NFC_CLK. You should add the MXC_NFC_CLK case, too. Is it worth to export the other getter functions, too (get_axi_a/b_clk, get_ahb_clk) ? > /* > + * Clock config code start here > + */ > + > +/* precondition: m>0 and n>0. Let g=gcd(m,n). */ > +static int gcd(int m, int n) > +{ > + int t; > + while (m > 0) { > + if (n > m) { > + t = m; > + m = n; > + n = t; > + } /* swap */ > + m -= n; > + } > + return n; > +} This function has nothing to do with MX5 code. This is a general function, and should be add to lib/. I think you have to remove it from here and put it in a separate patch. Add a comment to explain you are computing the greatest common divisor. Why do you not have taken the implementation from linux ? It uses the Euclid method (using a reminder) and it takes less iterations to get the result as this implementation. > + > +/* > + * This is to calculate various parameters based on reference clock and > + * targeted clock based on the equation: > + * t_clk = 2*ref_freq*(mfi + mfn/(mfd+1))/(pd+1) Where does this formula come from ? > + * This calculation is based on a fixed MFD value for simplicity. > + * > + * @param ref reference clock freq in Hz > + * @param target targeted clock in Hz > + * @param pll pll_param structure. > + * > + * @return 0 if successful; non-zero otherwise. > + */ > +static int calc_pll_params(u32 ref, u32 target, struct pll_param *pll) > +{ > + u64 pd, mfi = 1, mfn, mfd, t1; > + u32 n_target = target; > + u32 n_ref = ref, i; In your code you pass always a struct pll_param *pll previously filled with zeroes. You can at least move the memset inside this function, if it is used by all callers. > + for (i = 0; i < ARRAY_SIZE(fixed_mfd); i++) { > + if (fixed_mfd[i].ref_clk_hz == ref) { > + mfd = fixed_mfd[i].mfd; > + break; > + } > + } > + > + if (i == ARRAY_SIZE(fixed_mfd)) > + return -1; > + > + /* Use n_target and n_ref to avoid overflow */ > + for (pd = 1; pd <= PLL_PD_MAX; pd++) { > + t1 = n_target * pd; > + do_div(t1, (4 * n_ref)); > + mfi = t1; > + if (mfi > PLL_MFI_MAX) > + return -1; > + else if (mfi < 5) > + continue; > + break; > + } > + /* Now got pd and mfi already */ > + /* > + mfn = (((n_target * pd) / 4 - n_ref * mfi) * mfd) / n_ref; > + */ Wrong multiline comment. Can you explain to me (or telling where is described) this formula ? > +#ifdef CMD_CLOCK_DEBUG Use debug() instead of this. Do not add further personal DEBUG_* macro. > +#define calc_div(tgt_clk, src_clk, limit) ({ \ > + u32 v = 0; \ > + if (((src_clk) % (tgt_clk)) <= 100) \ > + v = (src_clk) / (tgt_clk); \ > + else \ > + v = ((src_clk) / (tgt_clk)) + 1;\ > + if (v > limit) \ > + v = limit; \ > + (v - 1); \ > + }) Please move the macro at the beginning of the file. I do not know if there are some rules in u-boot to returning values from macro as you do here. My personal taste is to use a function in this case. > + > +#define CHANGE_PLL_SETTINGS(pll, pd, fi, fn, fd) \ > + { \ > + __raw_writel(0x1232, &pll->ctrl); \ > + __raw_writel(0x2, &pll->config); \ > + __raw_writel((((pd) - 1) << 0) | ((fi) << 4), \ > + &pll->op); \ > + __raw_writel(fn, &(pll->mfn)); \ > + __raw_writel((fd) - 1, &pll->mfd); \ > + __raw_writel((((pd) - 1) << 0) | ((fi) << 4), \ > + &pll->hfs_op); \ > + __raw_writel(fn, &pll->hfs_mfn); \ > + __raw_writel((fd) - 1, &pll->hfs_mfd); \ > + __raw_writel(0x1232, &pll->ctrl); \ > + while (!__raw_readl(&pll->ctrl) & 0x1) \ > + ;\ > + } I have tried to understand, I give up now. Can you please add useful comments for this part and explain also what the magic number 0x1232 is supposed to do. Is there a part in the manual explaining this ? Can you add in the comment ? And move the macro at the beginning of the file. > + > +static int config_pll_clk(enum pll_clocks index, struct pll_param *pll_param) > +{ > + u32 ccsr = __raw_readl(&mxc_ccm->ccsr); > + struct mxc_pll_reg *pll = mxc_plls[index]; > + > + switch (index) { > + case PLL1_CLOCK: > + /* Switch ARM to PLL2 clock */ > + __raw_writel(ccsr | 0x4, &mxc_ccm->ccsr); > + CHANGE_PLL_SETTINGS(pll, pll_param->pd, > + pll_param->mfi, pll_param->mfn, > + pll_param->mfd); > + /* Switch back */ > + __raw_writel(ccsr & ~0x4, &mxc_ccm->ccsr); Offsets and fields for CCM module are defined in crm_regs.h. I see there are no definition for the Clcock Switch Register (ccsr). Please add them instead of fix constants. The same in the rest of the function. It make also easier to understand that your code switches clocks to another PLL before changing frequency. > +/* Config CPU clock */ > +static int config_core_clk(u32 ref, u32 freq) > +{ > + int ret = 0; > + struct pll_param pll_param; See my comments for the ref parameter later. is it correct or must be always CONFIG_SYS_MX5_HCLK ? > + > +/* Config main_bus_clock for periphs */ > +static int config_periph_clk(u32 ref, u32 freq) > +{ > + int ret = 0; > + struct pll_param pll_param; > + > + memset(&pll_param, 0, sizeof(struct pll_param)); > + > + if (__raw_readl(&mxc_ccm->cbcdr) & MXC_CCM_CBCDR_PERIPH_CLK_SEL) { > + ret = calc_pll_params(ref, freq, &pll_param); > + if (ret != 0) { > + printf("Error:Can't find pll parameters: %d\n", > + ret); > + return ret; > + } > + switch ((__raw_readl(&mxc_ccm->cbcmr) & \ > + MXC_CCM_CBCMR_PERIPH_CLK_SEL_MASK) >> \ > + MXC_CCM_CBCMR_PERIPH_CLK_SEL_OFFSET) { > + case 0: > + return config_pll_clk(PLL1_CLOCK, &pll_param); > + break; > + case 1: > + return config_pll_clk(PLL3_CLOCK, &pll_param); > + break; > + default: > + return -1; > + } > + } else { > + u32 old_cbcmr = __raw_readl(&mxc_ccm->cbcmr); > + u32 new_cbcdr = calc_per_cbcdr_val(freq); > + u32 old_nfc = get_nfc_clk(); > + > + /* Switch peripheral to PLL3 */ > + __raw_writel(0x00015154, &mxc_ccm->cbcmr); > + __raw_writel(0x02888945, &mxc_ccm->cbcdr); Use defines to set registers. Or read-modify-write, as you changed only PERIPH_APM_SEL from the reset value. However, you reset to 0 DDR_CLK_SEL. I see you switch values back at the end of the function, but my doubt is if it is correct to set in this transition DDR_CLK_SEL always to zero. Can it work with all boards or there are configurations where it does not work ? > + > +static int config_ddr_clk(u32 emi_clk) > +{ > + u32 clk_src; > + s32 shift = 0, clk_sel, div = 1; > + u32 cbcmr = __raw_readl(&mxc_ccm->cbcmr); > + u32 cbcdr = __raw_readl(&mxc_ccm->cbcdr); > + > + if (emi_clk > MAX_DDR_CLK) { > + printf("Warning:DDR clock should not exceed %d MHz\n", > + MAX_DDR_CLK / SZ_DEC_1M); > + emi_clk = MAX_DDR_CLK; > + } > + > + clk_src = get_periph_clk(); > + /* Find DDR clock input */ > + clk_sel = (cbcmr >> 10) & 0x3; Add #defines for this. This must be fixed globally. > + __raw_writel(0x0, &mxc_ccm->ccdr); > + > + return 0; > +} > + > +/*! Drop the ! from comment > + * This function assumes the expected core clock has to be changed by > + * modifying the PLL. This is NOT true always but for most of the times, > + * it is. So it assumes the PLL output freq is the same as the expected > + * core clock (presc=1) unless the core clock is less than PLL_FREQ_MIN. > + * In the latter case, it will try to increase the presc value until > + * (presc*core_clk) is greater than PLL_FREQ_MIN. It then makes call to > + * calc_pll_params() and obtains the values of PD, MFI,MFN, MFD based > + * on the targeted PLL and reference input clock to the PLL. Lastly, > + * it sets the register based on these values along with the dividers. > + * Note 1) There is no value checking for the passed-in divider values > + * so the caller has to make sure those values are sensible. > + * 2) Also adjust the NFC divider such that the NFC clock doesn't > + * exceed NFC_CLK_MAX. > + * 3) IPU HSP clock is independent of AHB clock. Even it can go up to > + * 177MHz for higher voltage, this function fixes the max to 133MHz. > + * 4) This function should not have allowed diag_printf() calls since > + * the serial driver has been stoped. But leave then here to allow > + * easy debugging by NOT calling the cyg_hal_plf_serial_stop(). ??? The code is clear taken from another context. Which is diag_printf ? Does point 4 apply here ? > + * > + * @param ref pll input reference clock (24MHz) Reference clock is defined as CONFIG_SYS_MX5_HCLK. Is it really possible to pass a parameter to this function different as CONFIG_SYS_MX5_HCLK ? What happens if CONFIG_SYS_MX5_HCLK and the ref parameter in this function have two different values ? It seems to me this is not a use case, and then we must avoid it. If I am right, you should drop the ref parameter and use CONFIG_SYS_MX5_HCLK. > + * @param freq core clock in Hz If the freq is in Hz, why do you multiply it for SZ_DEC_1M ? It seems to me that freq is in Mhz, not Hz. Please be consistent. > + * @param clk clock type, e.g CPU_CLK, DDR_CLK, etc. > + * @return 0 if successful; non-zero otherwise > + */ > +int mxc_set_clock(u32 ref, u32 freq, enum mxc_clock clk) > +{ > + freq *= SZ_DEC_1M; > + > + switch (clk) { > + case MXC_ARM_CLK: > + if (config_core_clk(ref, freq)) > + return -1; > + break; > + case MXC_PERIPH_CLK: > + if (config_periph_clk(ref, freq)) > + return -1; > + break; > + case MXC_DDR_CLK: > + if (config_ddr_clk(freq)) > + return -1; > + break; > + case MXC_NFC_CLK: > + if (config_nfc_clk(freq)) > + return -1; > + break; > + default: > + printf("Warning:Unsupported or invalid clock type\n"); This is the error case, you must not return 0 as you do. > +/* > * Dump some core clockes. > */ > int do_mx5_showclocks(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) > @@ -281,6 +825,7 @@ int do_mx5_showclocks(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) > printf("pll3: %dMHz\n", freq / 1000000); > printf("ipg clock : %dHz\n", mxc_get_clock(MXC_IPG_CLK)); > printf("ipg per clock : %dHz\n", mxc_get_clock(MXC_IPG_PERCLK)); > + printf("ddr clock : %dHz\n", mxc_get_clock(MXC_DDR_CLK)); What about the other clocks you added in your patch (MXC_NFC_CLK / MXC_PERIPH_CLK) ? > /* Define the bits in register CBCDR */ > +#define MXC_CCM_CBCDR_DDR_HIFREQ_SEL (0x1 << 30) > +#define MXC_CCM_CBCDR_DDR_PODF_MASK (0x7 << 27) > +#define MXC_CCM_CBCDR_DDR_PODF_OFFSET 27 > #define MXC_CCM_CBCDR_EMI_CLK_SEL (0x1 << 26) > #define MXC_CCM_CBCDR_PERIPH_CLK_SEL (0x1 << 25) > #define MXC_CCM_CBCDR_EMI_PODF_OFFSET 22 Agree with these changes in this file. Please extend this file with the missing constants you use in your code. Best regards, Stefano Babic
Hi, Stefano, 2011/5/16 Stefano Babic <sbabic@denx.de>: > On 05/11/2011 10:03 AM, Jason Liu wrote: > >> diff --git a/arch/arm/cpu/armv7/mx5/clock.c b/arch/arm/cpu/armv7/mx5/clock.c >> index 0b04a88..04d9f71 100644 >> --- a/arch/arm/cpu/armv7/mx5/clock.c >> +++ b/arch/arm/cpu/armv7/mx5/clock.c >> @@ -24,6 +24,7 @@ >> */ > >> >> +#define AHB_CLK_ROOT 133333333 >> +#define SZ_DEC_1M 1000000 > > Is this define used only to get the value in Mhz from the PLL clock ? If > it is the case, the name is quite confusing, as it refers to a size. If > it is not the case, please explain. I will get rid of the definition for AHB_CLK_ROOT and it's no need to define here since it only use once in the code. I will rename the SZ_DEC_1M or get rid of it also. > >> +#define PLL_PD_MAX 16 /* Actual pd+1 */ >> +#define PLL_MFI_MAX 15 >> +#define PLL_MFI_MIN 5 >> +#define ARM_DIV_MAX 8 >> +#define IPG_DIV_MAX 4 >> +#define AHB_DIV_MAX 8 >> +#define EMI_DIV_MAX 8 >> +#define NFC_DIV_MAX 8 > > Definitions for clock registers are in the crm_regs.h file. These are > the maximum values for some fields in the registers. Should they not be > put inside the crm_regs.h ? Yes, make sense, I will put it to crm_regs.h file. > >> + >> +struct fixed_pll_mfd { >> + u32 ref_clk_hz; >> + u32 mfd; >> +}; >> + >> +const struct fixed_pll_mfd fixed_mfd[] = { >> + {CONFIG_SYS_MX5_HCLK, 24 * 16}, >> +}; > > Not understood the need of an array (I have not said it is wrong, simply > I have not understood !). You use in the code (this is another issue) > "ref" as parameter for your functions for the reference clock, but is > seems to me that the only possible value is CONFIG_SYS_MX5_HCLK. > I use array here just for the case we will have another element in the array (currently we only have one) in the future. Currently, we use 24MHz as ref. > Are there other use case for this array, that makes sense to define and > maybe to extend it ? Just in case the PLL ref clock is not from 24MHZ. > > Can you add a reference to the manual explaining where these values are > coming from ? Do you mean the value 24 * 16? If yes, it's just for the simpler calculation. > >> + >> +struct pll_param { >> + u32 pd; >> + u32 mfi; >> + u32 mfn; >> + u32 mfd; >> +}; >> + >> +#define PLL_FREQ_MAX(ref_clk) (4 * (ref_clk) * PLL_MFI_MAX) >> +#define PLL_FREQ_MIN(ref_clk) \ >> + ((2 * (ref_clk) * (PLL_MFI_MIN - 1)) / PLL_PD_MAX) > > I understand what it is done here, but only after I have finally found > where it is described in the manual. Can you add useful comments here > and reference to the manual, too ? Such as describing these values are > for the registers DP_OP, DP_MFN and DP-MFD, and a reference to the > formula (Eqn. 22-1) helps to understand it. OK, I will add the comments here. > >> +#define MAX_DDR_CLK 420000000 >> +#define NFC_CLK_MAX 34000000 > > Where do these values come from ? I understand they are computed values, > depending on pll clock. It seems to me (at least for DDR clock) they are > absolute maximum rates, but it could be that MAX_DDR_CLK could be set to > a lower value depending on the PLL value. Is it correct ? In other words > : should be possible to set a PLL (you provide an API for changing it) > to a lower value, and then even the defines you set here do not > correspond to the real maximum value ? Yes, this is the absolute maximum rate. When you clk_api to config clock, it should not exceed the max value, for example, if (emi_clk > MAX_DDR_CLK) { printf("DDR clock should be less than" "%d MHz, assuming max value \n", ... > >> + >> /* >> * The API of get mxc clockes. >> */ >> @@ -245,10 +369,12 @@ unsigned int mxc_get_clock(enum mxc_clock clk) >> case MXC_UART_CLK: >> return get_uart_clk(); >> case MXC_CSPI_CLK: >> - return imx_get_cspiclk(); >> + return get_cspi_clk(); >> case MXC_FEC_CLK: >> return decode_pll(mxc_plls[PLL1_CLOCK], >> CONFIG_SYS_MX5_HCLK); >> + case MXC_DDR_CLK: >> + return get_ddr_clk(); > > You extended the enum for the clocks, as I see also MXC_NFC_CLK. You > should add the MXC_NFC_CLK case, too. OK, I will add it. > > Is it worth to export the other getter functions, too (get_axi_a/b_clk, > get_ahb_clk) ? Yes, I think it's valuable to export it and print it, thus user can know clearly what the axi_a/b and ahb clk rate are. > >> /* >> + * Clock config code start here >> + */ >> + >> +/* precondition: m>0 and n>0. Let g=gcd(m,n). */ >> +static int gcd(int m, int n) >> +{ >> + int t; >> + while (m > 0) { >> + if (n > m) { >> + t = m; >> + m = n; >> + n = t; >> + } /* swap */ >> + m -= n; >> + } >> + return n; >> +} > > This function has nothing to do with MX5 code. This is a general > function, and should be add to lib/. I think you have to remove it from > here and put it in a separate patch. I don't think it should be put to lib since only the mx5 clock code use it here. > > Add a comment to explain you are computing the greatest common divisor. > Why do you not have taken the implementation from linux ? It uses the > Euclid method (using a reminder) and it takes less iterations to get the > result as this implementation. OK, I will check linux implementation. Do you know does u-boot also has such kind of function already, I did not find it? > >> + >> +/* >> + * This is to calculate various parameters based on reference clock and >> + * targeted clock based on the equation: >> + * t_clk = 2*ref_freq*(mfi + mfn/(mfd+1))/(pd+1) > > Where does this formula come from ? The comments is not correct, I will fix it, it should be 4*ref_freq not 2 * ref_req. The reference manual: 12.2.2.3 DPLL Operation Register, elvis user guide, Registers DP_OP, DP_MFN, and DP_MFD calculate the output frequency by the formula: The chapter number will not the same as you, but you still can find it in the reference manual. > >> + * This calculation is based on a fixed MFD value for simplicity. >> + * >> + * @param ref reference clock freq in Hz >> + * @param target targeted clock in Hz >> + * @param pll pll_param structure. >> + * >> + * @return 0 if successful; non-zero otherwise. >> + */ >> +static int calc_pll_params(u32 ref, u32 target, struct pll_param *pll) >> +{ >> + u64 pd, mfi = 1, mfn, mfd, t1; >> + u32 n_target = target; >> + u32 n_ref = ref, i; > > In your code you pass always a struct pll_param *pll previously filled > with zeroes. You can at least move the memset inside this function, if > it is used by all callers. Yes, good point. > >> + for (i = 0; i < ARRAY_SIZE(fixed_mfd); i++) { >> + if (fixed_mfd[i].ref_clk_hz == ref) { >> + mfd = fixed_mfd[i].mfd; >> + break; >> + } >> + } >> + >> + if (i == ARRAY_SIZE(fixed_mfd)) >> + return -1; >> + >> + /* Use n_target and n_ref to avoid overflow */ >> + for (pd = 1; pd <= PLL_PD_MAX; pd++) { >> + t1 = n_target * pd; >> + do_div(t1, (4 * n_ref)); >> + mfi = t1; >> + if (mfi > PLL_MFI_MAX) >> + return -1; >> + else if (mfi < 5) >> + continue; >> + break; >> + } >> + /* Now got pd and mfi already */ >> + /* >> + mfn = (((n_target * pd) / 4 - n_ref * mfi) * mfd) / n_ref; >> + */ > > Wrong multiline comment. Can you explain to me (or telling where is > described) this formula ? Yes, I will fix the multi-line comments. As for the formula, it was based on the the reference manual: 12.2.2.3 DPLL Operation Register, elvis user guide, Registers DP_OP, DP_MFN, and DP_MFD calculate the output frequency by the formula: > > >> +#ifdef CMD_CLOCK_DEBUG > > Use debug() instead of this. Do not add further personal DEBUG_* macro. > >> +#define calc_div(tgt_clk, src_clk, limit) ({ \ >> + u32 v = 0; \ >> + if (((src_clk) % (tgt_clk)) <= 100) \ >> + v = (src_clk) / (tgt_clk); \ >> + else \ >> + v = ((src_clk) / (tgt_clk)) + 1;\ >> + if (v > limit) \ >> + v = limit; \ >> + (v - 1); \ >> + }) > > Please move the macro at the beginning of the file. I do not know if > there are some rules in u-boot to returning values from macro as you do > here. My personal taste is to use a function in this case. OK, I will move it. > >> + >> +#define CHANGE_PLL_SETTINGS(pll, pd, fi, fn, fd) \ >> + { \ >> + __raw_writel(0x1232, &pll->ctrl); \ >> + __raw_writel(0x2, &pll->config); \ >> + __raw_writel((((pd) - 1) << 0) | ((fi) << 4), \ >> + &pll->op); \ >> + __raw_writel(fn, &(pll->mfn)); \ >> + __raw_writel((fd) - 1, &pll->mfd); \ >> + __raw_writel((((pd) - 1) << 0) | ((fi) << 4), \ >> + &pll->hfs_op); \ >> + __raw_writel(fn, &pll->hfs_mfn); \ >> + __raw_writel((fd) - 1, &pll->hfs_mfd); \ >> + __raw_writel(0x1232, &pll->ctrl); \ >> + while (!__raw_readl(&pll->ctrl) & 0x1) \ >> + ;\ >> + } > > I have tried to understand, I give up now. Can you please add useful > comments for this part and explain also what the magic number 0x1232 is > supposed to do. Is there a part in the manual explaining this ? Can you > add in the comment ? > And move the macro at the beginning of the file. Reference manual tell it clear and you need read the 12.2.2.1 DPLL Control Register but you reminder me to put some macro definition for the pain of reading. > >> + >> +static int config_pll_clk(enum pll_clocks index, struct pll_param *pll_param) >> +{ >> + u32 ccsr = __raw_readl(&mxc_ccm->ccsr); >> + struct mxc_pll_reg *pll = mxc_plls[index]; >> + >> + switch (index) { >> + case PLL1_CLOCK: >> + /* Switch ARM to PLL2 clock */ >> + __raw_writel(ccsr | 0x4, &mxc_ccm->ccsr); >> + CHANGE_PLL_SETTINGS(pll, pll_param->pd, >> + pll_param->mfi, pll_param->mfn, >> + pll_param->mfd); >> + /* Switch back */ >> + __raw_writel(ccsr & ~0x4, &mxc_ccm->ccsr); > > Offsets and fields for CCM module are defined in crm_regs.h. I see there > are no definition for the Clcock Switch Register (ccsr). Please add them > instead of fix constants. The same in the rest of the function. OK, > > It make also easier to understand that your code switches clocks to > another PLL before changing frequency. > >> +/* Config CPU clock */ >> +static int config_core_clk(u32 ref, u32 freq) >> +{ >> + int ret = 0; >> + struct pll_param pll_param; > > See my comments for the ref parameter later. is it correct or must be > always CONFIG_SYS_MX5_HCLK ? > >> + >> +/* Config main_bus_clock for periphs */ >> +static int config_periph_clk(u32 ref, u32 freq) >> +{ >> + int ret = 0; >> + struct pll_param pll_param; >> + >> + memset(&pll_param, 0, sizeof(struct pll_param)); >> + >> + if (__raw_readl(&mxc_ccm->cbcdr) & MXC_CCM_CBCDR_PERIPH_CLK_SEL) { >> + ret = calc_pll_params(ref, freq, &pll_param); >> + if (ret != 0) { >> + printf("Error:Can't find pll parameters: %d\n", >> + ret); >> + return ret; >> + } >> + switch ((__raw_readl(&mxc_ccm->cbcmr) & \ >> + MXC_CCM_CBCMR_PERIPH_CLK_SEL_MASK) >> \ >> + MXC_CCM_CBCMR_PERIPH_CLK_SEL_OFFSET) { >> + case 0: >> + return config_pll_clk(PLL1_CLOCK, &pll_param); >> + break; >> + case 1: >> + return config_pll_clk(PLL3_CLOCK, &pll_param); >> + break; >> + default: >> + return -1; >> + } >> + } else { >> + u32 old_cbcmr = __raw_readl(&mxc_ccm->cbcmr); >> + u32 new_cbcdr = calc_per_cbcdr_val(freq); >> + u32 old_nfc = get_nfc_clk(); >> + >> + /* Switch peripheral to PLL3 */ >> + __raw_writel(0x00015154, &mxc_ccm->cbcmr); >> + __raw_writel(0x02888945, &mxc_ccm->cbcdr); > > Use defines to set registers. Or read-modify-write, as you changed only > PERIPH_APM_SEL from the reset value. However, you reset to 0 > DDR_CLK_SEL. I see you switch values back at the end of the function, > but my doubt is if it is correct to set in this transition DDR_CLK_SEL > always to zero. Can it work with all boards or there are configurations > where it does not work ? I think it should work since DDR_CLK_SEL:00 derive clock from axi a is the default value when boot up. > >> + >> +static int config_ddr_clk(u32 emi_clk) >> +{ >> + u32 clk_src; >> + s32 shift = 0, clk_sel, div = 1; >> + u32 cbcmr = __raw_readl(&mxc_ccm->cbcmr); >> + u32 cbcdr = __raw_readl(&mxc_ccm->cbcdr); >> + >> + if (emi_clk > MAX_DDR_CLK) { >> + printf("Warning:DDR clock should not exceed %d MHz\n", >> + MAX_DDR_CLK / SZ_DEC_1M); >> + emi_clk = MAX_DDR_CLK; >> + } >> + >> + clk_src = get_periph_clk(); >> + /* Find DDR clock input */ >> + clk_sel = (cbcmr >> 10) & 0x3; > > Add #defines for this. This must be fixed globally. > >> + __raw_writel(0x0, &mxc_ccm->ccdr); >> + >> + return 0; >> +} >> + >> +/*! > > Drop the ! from comment Sure. > >> + * This function assumes the expected core clock has to be changed by >> + * modifying the PLL. This is NOT true always but for most of the times, >> + * it is. So it assumes the PLL output freq is the same as the expected >> + * core clock (presc=1) unless the core clock is less than PLL_FREQ_MIN. >> + * In the latter case, it will try to increase the presc value until >> + * (presc*core_clk) is greater than PLL_FREQ_MIN. It then makes call to >> + * calc_pll_params() and obtains the values of PD, MFI,MFN, MFD based >> + * on the targeted PLL and reference input clock to the PLL. Lastly, >> + * it sets the register based on these values along with the dividers. >> + * Note 1) There is no value checking for the passed-in divider values >> + * so the caller has to make sure those values are sensible. >> + * 2) Also adjust the NFC divider such that the NFC clock doesn't >> + * exceed NFC_CLK_MAX. >> + * 3) IPU HSP clock is independent of AHB clock. Even it can go up to >> + * 177MHz for higher voltage, this function fixes the max to 133MHz. >> + * 4) This function should not have allowed diag_printf() calls since >> + * the serial driver has been stoped. But leave then here to allow >> + * easy debugging by NOT calling the cyg_hal_plf_serial_stop(). > > ??? > > The code is clear taken from another context. Which is diag_printf ? > Does point 4 apply here ? Yes, good catch. I will remove it. > >> + * >> + * @param ref pll input reference clock (24MHz) > > Reference clock is defined as CONFIG_SYS_MX5_HCLK. Is it really possible > to pass a parameter to this function different as CONFIG_SYS_MX5_HCLK ? > What happens if CONFIG_SYS_MX5_HCLK and the ref parameter in this > function have two different values ? It seems to me this is not a use > case, and then we must avoid it. If I am right, you should drop the ref > parameter and use CONFIG_SYS_MX5_HCLK. As the reference manual tell, the pll can have 4 different clock source. The most common use case is using 24Mhz OSC as the input. > >> + * @param freq core clock in Hz > > If the freq is in Hz, why do you multiply it for SZ_DEC_1M ? > It seems to me that freq is in Mhz, not Hz. Please be consistent. Will fix the comments. > >> + * @param clk clock type, e.g CPU_CLK, DDR_CLK, etc. >> + * @return 0 if successful; non-zero otherwise >> + */ >> +int mxc_set_clock(u32 ref, u32 freq, enum mxc_clock clk) >> +{ >> + freq *= SZ_DEC_1M; >> + >> + switch (clk) { >> + case MXC_ARM_CLK: >> + if (config_core_clk(ref, freq)) >> + return -1; >> + break; >> + case MXC_PERIPH_CLK: >> + if (config_periph_clk(ref, freq)) >> + return -1; >> + break; >> + case MXC_DDR_CLK: >> + if (config_ddr_clk(freq)) >> + return -1; >> + break; >> + case MXC_NFC_CLK: >> + if (config_nfc_clk(freq)) >> + return -1; >> + break; >> + default: >> + printf("Warning:Unsupported or invalid clock type\n"); > > This is the error case, you must not return 0 as you do. OK, > >> +/* >> * Dump some core clockes. >> */ >> int do_mx5_showclocks(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) >> @@ -281,6 +825,7 @@ int do_mx5_showclocks(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) >> printf("pll3: %dMHz\n", freq / 1000000); >> printf("ipg clock : %dHz\n", mxc_get_clock(MXC_IPG_CLK)); >> printf("ipg per clock : %dHz\n", mxc_get_clock(MXC_IPG_PERCLK)); >> + printf("ddr clock : %dHz\n", mxc_get_clock(MXC_DDR_CLK)); > > What about the other clocks you added in your patch (MXC_NFC_CLK / > MXC_PERIPH_CLK) ? I will add the these clock print. > >> /* Define the bits in register CBCDR */ >> +#define MXC_CCM_CBCDR_DDR_HIFREQ_SEL (0x1 << 30) >> +#define MXC_CCM_CBCDR_DDR_PODF_MASK (0x7 << 27) >> +#define MXC_CCM_CBCDR_DDR_PODF_OFFSET 27 >> #define MXC_CCM_CBCDR_EMI_CLK_SEL (0x1 << 26) >> #define MXC_CCM_CBCDR_PERIPH_CLK_SEL (0x1 << 25) >> #define MXC_CCM_CBCDR_EMI_PODF_OFFSET 22 > > Agree with these changes in this file. Please extend this file with the > missing constants you use in your code. OK, > > Best regards, > Stefano Babic > > -- > ===================================================================== > DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office@denx.de > ===================================================================== > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot >
On 05/17/2011 08:25 AM, Jason Liu wrote: > Hi, Stefano, Hi Jason, > I use array here just for the case we will have another element in the array > (currently we only have one) in the future. Currently, we use 24MHz as ref. Understood, thanks. >>> /* >>> + * Clock config code start here >>> + */ >>> + >>> +/* precondition: m>0 and n>0. Let g=gcd(m,n). */ >>> +static int gcd(int m, int n) >>> +{ >>> + int t; >>> + while (m > 0) { >>> + if (n > m) { >>> + t = m; >>> + m = n; >>> + n = t; >>> + } /* swap */ >>> + m -= n; >>> + } >>> + return n; >>> +} >> >> This function has nothing to do with MX5 code. This is a general >> function, and should be add to lib/. I think you have to remove it from >> here and put it in a separate patch. > > I don't think it should be put to lib since only the mx5 clock code use it here. This function computes the GCD of two numbers, and has nothing to do with MX5 and clock. It is a general function, that others can use. > OK, I will check linux implementation. Do you know does u-boot also has such > kind of function already, I did not find it? No, this function is not yet implemented in u-boot. However, it could be that there is in some drivers an analog function doing the same. For this reason and to avoid multiple instances of "quite" same code doing "quite" the same thing, I suggest to move this function from here from strictly related IMX code to a general lib. Let's see what Wolfgang whinks about it. >>> + * t_clk = 2*ref_freq*(mfi + mfn/(mfd+1))/(pd+1) >> >> Where does this formula come from ? > > The comments is not correct, I will fix it, it should be 4*ref_freq > not 2 * ref_req. > The reference manual: 12.2.2.3 DPLL Operation Register, elvis user guide, > Registers DP_OP, DP_MFN, and DP_MFD calculate the output frequency by > the formula: > > The chapter number will not the same as you, but you still can find it > in the reference manual. Ok. I think it is better to add a comment with the chapter title, as to set a reference number, because this is changing. I will check in the manual, thanks for hint. >> Use defines to set registers. Or read-modify-write, as you changed only >> PERIPH_APM_SEL from the reset value. However, you reset to 0 >> DDR_CLK_SEL. I see you switch values back at the end of the function, >> but my doubt is if it is correct to set in this transition DDR_CLK_SEL >> always to zero. Can it work with all boards or there are configurations >> where it does not work ? > > I think it should work since DDR_CLK_SEL:00 derive clock from axi a > is the default value when boot up. Then I think it is enough you add as comment your explanation, and mention that the code does not support if the clock is not derived from axi-a. If someone adds a custom board with this set-up (and I do not know if it will be a use case), he is at least warned that must adapt this function. >>> + * >>> + * @param ref pll input reference clock (24MHz) >> >> Reference clock is defined as CONFIG_SYS_MX5_HCLK. Is it really possible >> to pass a parameter to this function different as CONFIG_SYS_MX5_HCLK ? >> What happens if CONFIG_SYS_MX5_HCLK and the ref parameter in this >> function have two different values ? It seems to me this is not a use >> case, and then we must avoid it. If I am right, you should drop the ref >> parameter and use CONFIG_SYS_MX5_HCLK. > > As the reference manual tell, the pll can have 4 different clock > source. The most > common use case is using 24Mhz OSC as the input. Understood, thanks. Best regards, Stefano Babic
diff --git a/arch/arm/cpu/armv7/mx5/clock.c b/arch/arm/cpu/armv7/mx5/clock.c index 0b04a88..04d9f71 100644 --- a/arch/arm/cpu/armv7/mx5/clock.c +++ b/arch/arm/cpu/armv7/mx5/clock.c @@ -24,6 +24,7 @@ */ #include <common.h> +#include <div64.h> #include <asm/io.h> #include <asm/errno.h> #include <asm/arch/imx-regs.h> @@ -34,6 +35,7 @@ enum pll_clocks { PLL1_CLOCK = 0, PLL2_CLOCK, PLL3_CLOCK, + PLL4_CLOCK, PLL_CLOCKS, }; @@ -41,8 +43,42 @@ struct mxc_pll_reg *mxc_plls[PLL_CLOCKS] = { [PLL1_CLOCK] = (struct mxc_pll_reg *)PLL1_BASE_ADDR, [PLL2_CLOCK] = (struct mxc_pll_reg *)PLL2_BASE_ADDR, [PLL3_CLOCK] = (struct mxc_pll_reg *)PLL3_BASE_ADDR, + [PLL4_CLOCK] = (struct mxc_pll_reg *)PLL4_BASE_ADDR, }; +#define AHB_CLK_ROOT 133333333 +#define SZ_DEC_1M 1000000 +#define PLL_PD_MAX 16 /* Actual pd+1 */ +#define PLL_MFI_MAX 15 +#define PLL_MFI_MIN 5 +#define ARM_DIV_MAX 8 +#define IPG_DIV_MAX 4 +#define AHB_DIV_MAX 8 +#define EMI_DIV_MAX 8 +#define NFC_DIV_MAX 8 + +struct fixed_pll_mfd { + u32 ref_clk_hz; + u32 mfd; +}; + +const struct fixed_pll_mfd fixed_mfd[] = { + {CONFIG_SYS_MX5_HCLK, 24 * 16}, +}; + +struct pll_param { + u32 pd; + u32 mfi; + u32 mfn; + u32 mfd; +}; + +#define PLL_FREQ_MAX(ref_clk) (4 * (ref_clk) * PLL_MFI_MAX) +#define PLL_FREQ_MIN(ref_clk) \ + ((2 * (ref_clk) * (PLL_MFI_MIN - 1)) / PLL_PD_MAX) +#define MAX_DDR_CLK 420000000 +#define NFC_CLK_MAX 34000000 + struct mxc_ccm_reg *mxc_ccm = (struct mxc_ccm_reg *)MXC_CCM_BASE; /* @@ -175,7 +211,7 @@ static u32 get_uart_clk(void) /* * This function returns the low power audio clock. */ -u32 get_lp_apm(void) +static u32 get_lp_apm(void) { u32 ret_val = 0; u32 ccsr = __raw_readl(&mxc_ccm->ccsr); @@ -191,7 +227,7 @@ u32 get_lp_apm(void) /* * get cspi clock rate. */ -u32 imx_get_cspiclk(void) +static u32 get_cspi_clk(void) { u32 ret_val = 0, pdf, pre_pdf, clk_sel; u32 cscmr1 = __raw_readl(&mxc_ccm->cscmr1); @@ -228,6 +264,94 @@ u32 imx_get_cspiclk(void) return ret_val; } +static u32 get_axi_a_clk(void) +{ + u32 cbcdr = __raw_readl(&mxc_ccm->cbcdr); + u32 pdf = (cbcdr & MXC_CCM_CBCDR_AXI_A_PODF_MASK) \ + >> MXC_CCM_CBCDR_AXI_A_PODF_OFFSET; + + return get_periph_clk() / (pdf + 1); +} + +static u32 get_axi_b_clk(void) +{ + u32 cbcdr = __raw_readl(&mxc_ccm->cbcdr); + u32 pdf = (cbcdr & MXC_CCM_CBCDR_AXI_B_PODF_MASK) \ + >> MXC_CCM_CBCDR_AXI_B_PODF_OFFSET; + + return get_periph_clk() / (pdf + 1); +} + +static u32 get_ahb_clk(void) +{ + u32 cbcdr = __raw_readl(&mxc_ccm->cbcdr); + u32 pdf = (cbcdr & MXC_CCM_CBCDR_AHB_PODF_MASK) \ + >> MXC_CCM_CBCDR_AHB_PODF_OFFSET; + + return get_periph_clk() / (pdf + 1); +} + +static u32 get_emi_slow_clk(void) +{ + u32 cbcdr = __raw_readl(&mxc_ccm->cbcdr); + u32 emi_clk_sel = cbcdr & MXC_CCM_CBCDR_EMI_CLK_SEL; + u32 pdf = (cbcdr & MXC_CCM_CBCDR_EMI_PODF_MASK) \ + >> MXC_CCM_CBCDR_EMI_PODF_OFFSET; + + if (emi_clk_sel) + return get_ahb_clk() / (pdf + 1); + + return get_periph_clk() / (pdf + 1); +} + +static u32 get_nfc_clk(void) +{ + u32 cbcdr = __raw_readl(&mxc_ccm->cbcdr); + u32 pdf = (cbcdr & MXC_CCM_CBCDR_NFC_PODF_MASK) \ + >> MXC_CCM_CBCDR_NFC_PODF_OFFSET; + + return get_emi_slow_clk() / (pdf + 1); +} + +static u32 get_ddr_clk(void) +{ + u32 ret_val = 0; + u32 cbcmr = __raw_readl(&mxc_ccm->cbcmr); + u32 ddr_clk_sel = (cbcmr & MXC_CCM_CBCMR_DDR_CLK_SEL_MASK) \ + >> MXC_CCM_CBCMR_DDR_CLK_SEL_OFFSET; +#ifdef CONFIG_MX51 + u32 cbcdr = __raw_readl(&mxc_ccm->cbcdr); + if (cbcdr & MXC_CCM_CBCDR_DDR_HIFREQ_SEL) { + u32 ddr_clk_podf = (cbcdr & MXC_CCM_CBCDR_DDR_PODF_MASK) >> \ + MXC_CCM_CBCDR_DDR_PODF_OFFSET; + + ret_val = decode_pll(mxc_plls[PLL1_CLOCK], CONFIG_SYS_MX5_HCLK); + ret_val /= ddr_clk_podf + 1; + + return ret_val; + } +#endif + switch (ddr_clk_sel) { + case 0: + ret_val = get_axi_a_clk(); + break; + case 1: + ret_val = get_axi_b_clk(); + break; + case 2: + ret_val = get_emi_slow_clk(); + break; + case 3: + ret_val = get_ahb_clk(); + break; + default: + break; + } + + return ret_val; +} + + /* * The API of get mxc clockes. */ @@ -245,10 +369,12 @@ unsigned int mxc_get_clock(enum mxc_clock clk) case MXC_UART_CLK: return get_uart_clk(); case MXC_CSPI_CLK: - return imx_get_cspiclk(); + return get_cspi_clk(); case MXC_FEC_CLK: return decode_pll(mxc_plls[PLL1_CLOCK], CONFIG_SYS_MX5_HCLK); + case MXC_DDR_CLK: + return get_ddr_clk(); default: break; } @@ -267,6 +393,424 @@ u32 imx_get_fecclk(void) } /* + * Clock config code start here + */ + +/* precondition: m>0 and n>0. Let g=gcd(m,n). */ +static int gcd(int m, int n) +{ + int t; + while (m > 0) { + if (n > m) { + t = m; + m = n; + n = t; + } /* swap */ + m -= n; + } + return n; +} + +/* + * This is to calculate various parameters based on reference clock and + * targeted clock based on the equation: + * t_clk = 2*ref_freq*(mfi + mfn/(mfd+1))/(pd+1) + * This calculation is based on a fixed MFD value for simplicity. + * + * @param ref reference clock freq in Hz + * @param target targeted clock in Hz + * @param pll pll_param structure. + * + * @return 0 if successful; non-zero otherwise. + */ +static int calc_pll_params(u32 ref, u32 target, struct pll_param *pll) +{ + u64 pd, mfi = 1, mfn, mfd, t1; + u32 n_target = target; + u32 n_ref = ref, i; + + /* + * Make sure targeted freq is in the valid range. + * Otherwise the following calculation might be wrong!!! + */ + if (n_target < PLL_FREQ_MIN(ref) || + n_target > PLL_FREQ_MAX(ref)) { + printf("Targeted peripheral clock should be" + "within [%d - %d]\n", + PLL_FREQ_MIN(ref) / SZ_DEC_1M, + PLL_FREQ_MAX(ref) / SZ_DEC_1M); + return -1; + } + + for (i = 0; i < ARRAY_SIZE(fixed_mfd); i++) { + if (fixed_mfd[i].ref_clk_hz == ref) { + mfd = fixed_mfd[i].mfd; + break; + } + } + + if (i == ARRAY_SIZE(fixed_mfd)) + return -1; + + /* Use n_target and n_ref to avoid overflow */ + for (pd = 1; pd <= PLL_PD_MAX; pd++) { + t1 = n_target * pd; + do_div(t1, (4 * n_ref)); + mfi = t1; + if (mfi > PLL_MFI_MAX) + return -1; + else if (mfi < 5) + continue; + break; + } + /* Now got pd and mfi already */ + /* + mfn = (((n_target * pd) / 4 - n_ref * mfi) * mfd) / n_ref; + */ + t1 = n_target * pd; + do_div(t1, 4); + t1 -= n_ref * mfi; + t1 *= mfd; + do_div(t1, n_ref); + mfn = t1; +#ifdef CMD_CLOCK_DEBUG + printf("ref=%d, target=%d, pd=%d," "mfi=%d,mfn=%d, mfd=%d\n", + ref, n_target, (u32)pd, (u32)mfi, (u32)mfn, (u32)mfd); +#endif + i = 1; + if (mfn != 0) + i = gcd(mfd, mfn); + pll->pd = (u32)pd; + pll->mfi = (u32)mfi; + do_div(mfn, i); + pll->mfn = (u32)mfn; + do_div(mfd, i); + pll->mfd = (u32)mfd; + + return 0; +} + +#define calc_div(tgt_clk, src_clk, limit) ({ \ + u32 v = 0; \ + if (((src_clk) % (tgt_clk)) <= 100) \ + v = (src_clk) / (tgt_clk); \ + else \ + v = ((src_clk) / (tgt_clk)) + 1;\ + if (v > limit) \ + v = limit; \ + (v - 1); \ + }) + +static u32 calc_per_cbcdr_val(u32 per_clk) +{ + u32 cbcdr = __raw_readl(&mxc_ccm->cbcdr); + u32 tmp_clk = 0, div = 0, clk_sel = 0; + + cbcdr &= ~MXC_CCM_CBCDR_PERIPH_CLK_SEL; + + /* emi_slow_podf divider */ + tmp_clk = get_emi_slow_clk(); + clk_sel = cbcdr & MXC_CCM_CBCDR_EMI_CLK_SEL; + if (clk_sel) { + div = calc_div(tmp_clk, per_clk, 8); + cbcdr &= ~MXC_CCM_CBCDR_EMI_PODF_MASK; + cbcdr |= (div << MXC_CCM_CBCDR_EMI_PODF_OFFSET); + } + + /* axi_b_podf divider */ + tmp_clk = get_axi_b_clk(); + div = calc_div(tmp_clk, per_clk, 8); + cbcdr &= ~MXC_CCM_CBCDR_AXI_B_PODF_MASK; + cbcdr |= (div << MXC_CCM_CBCDR_AXI_B_PODF_OFFSET); + + /* axi_b_podf divider */ + tmp_clk = get_axi_a_clk(); + div = calc_div(tmp_clk, per_clk, 8); + cbcdr &= ~MXC_CCM_CBCDR_AXI_A_PODF_MASK; + cbcdr |= (div << MXC_CCM_CBCDR_AXI_A_PODF_OFFSET); + + /* ahb podf divider */ + tmp_clk = AHB_CLK_ROOT; + div = calc_div(tmp_clk, per_clk, 8); + cbcdr &= ~MXC_CCM_CBCDR_AHB_PODF_MASK; + cbcdr |= (div << MXC_CCM_CBCDR_AHB_PODF_OFFSET); + + return cbcdr; +} + +#define CHANGE_PLL_SETTINGS(pll, pd, fi, fn, fd) \ + { \ + __raw_writel(0x1232, &pll->ctrl); \ + __raw_writel(0x2, &pll->config); \ + __raw_writel((((pd) - 1) << 0) | ((fi) << 4), \ + &pll->op); \ + __raw_writel(fn, &(pll->mfn)); \ + __raw_writel((fd) - 1, &pll->mfd); \ + __raw_writel((((pd) - 1) << 0) | ((fi) << 4), \ + &pll->hfs_op); \ + __raw_writel(fn, &pll->hfs_mfn); \ + __raw_writel((fd) - 1, &pll->hfs_mfd); \ + __raw_writel(0x1232, &pll->ctrl); \ + while (!__raw_readl(&pll->ctrl) & 0x1) \ + ;\ + } + +static int config_pll_clk(enum pll_clocks index, struct pll_param *pll_param) +{ + u32 ccsr = __raw_readl(&mxc_ccm->ccsr); + struct mxc_pll_reg *pll = mxc_plls[index]; + + switch (index) { + case PLL1_CLOCK: + /* Switch ARM to PLL2 clock */ + __raw_writel(ccsr | 0x4, &mxc_ccm->ccsr); + CHANGE_PLL_SETTINGS(pll, pll_param->pd, + pll_param->mfi, pll_param->mfn, + pll_param->mfd); + /* Switch back */ + __raw_writel(ccsr & ~0x4, &mxc_ccm->ccsr); + break; + case PLL2_CLOCK: + /* Switch to pll2 bypass clock */ + __raw_writel(ccsr | 0x2, &mxc_ccm->ccsr); + CHANGE_PLL_SETTINGS(pll, pll_param->pd, + pll_param->mfi, pll_param->mfn, + pll_param->mfd); + /* Switch back */ + __raw_writel(ccsr & ~0x2, &mxc_ccm->ccsr); + break; + case PLL3_CLOCK: + /* Switch to pll3 bypass clock */ + __raw_writel(ccsr | 0x1, &mxc_ccm->ccsr); + CHANGE_PLL_SETTINGS(pll, pll_param->pd, + pll_param->mfi, pll_param->mfn, + pll_param->mfd); + /* Switch back */ + __raw_writel(ccsr & ~0x1, &mxc_ccm->ccsr); + break; + case PLL4_CLOCK: + /* Switch to pll4 bypass clock */ + __raw_writel(ccsr | 0x20, &mxc_ccm->ccsr); + CHANGE_PLL_SETTINGS(pll, pll_param->pd, + pll_param->mfi, pll_param->mfn, + pll_param->mfd); + /* Switch back */ + __raw_writel(ccsr & ~0x20, &mxc_ccm->ccsr); + break; + default: + return -1; + } + + return 0; +} + +/* Config CPU clock */ +static int config_core_clk(u32 ref, u32 freq) +{ + int ret = 0; + struct pll_param pll_param; + + memset(&pll_param, 0, sizeof(struct pll_param)); + + /* The case that periph uses PLL1 is not considered here */ + ret = calc_pll_params(ref, freq, &pll_param); + if (ret != 0) { + printf("Error:Can't find pll parameters: %d\n", ret); + return ret; + } + + return config_pll_clk(PLL1_CLOCK, &pll_param); +} + +static int config_nfc_clk(u32 nfc_clk) +{ + u32 reg; + u32 parent_rate = get_emi_slow_clk(); + u32 div = parent_rate / nfc_clk; + + if (nfc_clk <= 0) + return -1; + if (div == 0) + div++; + if (parent_rate / div > NFC_CLK_MAX) + div++; + reg = __raw_readl(&mxc_ccm->cbcdr); + reg &= ~MXC_CCM_CBCDR_NFC_PODF_MASK; + reg |= (div - 1) << MXC_CCM_CBCDR_NFC_PODF_OFFSET; + __raw_writel(reg, &mxc_ccm->cbcdr); + while (__raw_readl(&mxc_ccm->cdhipr) != 0) + ; + return 0; +} + +/* Config main_bus_clock for periphs */ +static int config_periph_clk(u32 ref, u32 freq) +{ + int ret = 0; + struct pll_param pll_param; + + memset(&pll_param, 0, sizeof(struct pll_param)); + + if (__raw_readl(&mxc_ccm->cbcdr) & MXC_CCM_CBCDR_PERIPH_CLK_SEL) { + ret = calc_pll_params(ref, freq, &pll_param); + if (ret != 0) { + printf("Error:Can't find pll parameters: %d\n", + ret); + return ret; + } + switch ((__raw_readl(&mxc_ccm->cbcmr) & \ + MXC_CCM_CBCMR_PERIPH_CLK_SEL_MASK) >> \ + MXC_CCM_CBCMR_PERIPH_CLK_SEL_OFFSET) { + case 0: + return config_pll_clk(PLL1_CLOCK, &pll_param); + break; + case 1: + return config_pll_clk(PLL3_CLOCK, &pll_param); + break; + default: + return -1; + } + } else { + u32 old_cbcmr = __raw_readl(&mxc_ccm->cbcmr); + u32 new_cbcdr = calc_per_cbcdr_val(freq); + u32 old_nfc = get_nfc_clk(); + + /* Switch peripheral to PLL3 */ + __raw_writel(0x00015154, &mxc_ccm->cbcmr); + __raw_writel(0x02888945, &mxc_ccm->cbcdr); + + /* Make sure change is effective */ + while (__raw_readl(&mxc_ccm->cdhipr) != 0) + ; + + /* Setup PLL2 */ + ret = calc_pll_params(ref, freq, &pll_param); + if (ret != 0) { + printf("Error:Can't find pll parameters: %d\n", + ret); + return ret; + } + config_pll_clk(PLL2_CLOCK, &pll_param); + + /* Switch peripheral back */ + __raw_writel(new_cbcdr, &mxc_ccm->cbcdr); + __raw_writel(old_cbcmr, &mxc_ccm->cbcmr); + + /* Make sure change is effective */ + while (__raw_readl(&mxc_ccm->cdhipr) != 0) + ; + /* restore to old NFC clock */ + config_nfc_clk(old_nfc); + } + + return 0; +} + +static int config_ddr_clk(u32 emi_clk) +{ + u32 clk_src; + s32 shift = 0, clk_sel, div = 1; + u32 cbcmr = __raw_readl(&mxc_ccm->cbcmr); + u32 cbcdr = __raw_readl(&mxc_ccm->cbcdr); + + if (emi_clk > MAX_DDR_CLK) { + printf("Warning:DDR clock should not exceed %d MHz\n", + MAX_DDR_CLK / SZ_DEC_1M); + emi_clk = MAX_DDR_CLK; + } + + clk_src = get_periph_clk(); + /* Find DDR clock input */ + clk_sel = (cbcmr >> 10) & 0x3; + switch (clk_sel) { + case 0: + shift = 16; + break; + case 1: + shift = 19; + break; + case 2: + shift = 22; + break; + case 3: + shift = 10; + break; + default: + return -1; + } + + if ((clk_src % emi_clk) < 10000000) + div = clk_src / emi_clk; + else + div = (clk_src / emi_clk) + 1; + if (div > 8) + div = 8; + + cbcdr = cbcdr & ~(0x7 << shift); + cbcdr |= ((div - 1) << shift); + __raw_writel(cbcdr, &mxc_ccm->cbcdr); + while (__raw_readl(&mxc_ccm->cdhipr) != 0) + ; + __raw_writel(0x0, &mxc_ccm->ccdr); + + return 0; +} + +/*! + * This function assumes the expected core clock has to be changed by + * modifying the PLL. This is NOT true always but for most of the times, + * it is. So it assumes the PLL output freq is the same as the expected + * core clock (presc=1) unless the core clock is less than PLL_FREQ_MIN. + * In the latter case, it will try to increase the presc value until + * (presc*core_clk) is greater than PLL_FREQ_MIN. It then makes call to + * calc_pll_params() and obtains the values of PD, MFI,MFN, MFD based + * on the targeted PLL and reference input clock to the PLL. Lastly, + * it sets the register based on these values along with the dividers. + * Note 1) There is no value checking for the passed-in divider values + * so the caller has to make sure those values are sensible. + * 2) Also adjust the NFC divider such that the NFC clock doesn't + * exceed NFC_CLK_MAX. + * 3) IPU HSP clock is independent of AHB clock. Even it can go up to + * 177MHz for higher voltage, this function fixes the max to 133MHz. + * 4) This function should not have allowed diag_printf() calls since + * the serial driver has been stoped. But leave then here to allow + * easy debugging by NOT calling the cyg_hal_plf_serial_stop(). + * + * @param ref pll input reference clock (24MHz) + * @param freq core clock in Hz + * @param clk clock type, e.g CPU_CLK, DDR_CLK, etc. + * @return 0 if successful; non-zero otherwise + */ +int mxc_set_clock(u32 ref, u32 freq, enum mxc_clock clk) +{ + freq *= SZ_DEC_1M; + + switch (clk) { + case MXC_ARM_CLK: + if (config_core_clk(ref, freq)) + return -1; + break; + case MXC_PERIPH_CLK: + if (config_periph_clk(ref, freq)) + return -1; + break; + case MXC_DDR_CLK: + if (config_ddr_clk(freq)) + return -1; + break; + case MXC_NFC_CLK: + if (config_nfc_clk(freq)) + return -1; + break; + default: + printf("Warning:Unsupported or invalid clock type\n"); + } + + return 0; +} + + +/* * Dump some core clockes. */ int do_mx5_showclocks(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) @@ -281,6 +825,7 @@ int do_mx5_showclocks(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) printf("pll3: %dMHz\n", freq / 1000000); printf("ipg clock : %dHz\n", mxc_get_clock(MXC_IPG_CLK)); printf("ipg per clock : %dHz\n", mxc_get_clock(MXC_IPG_PERCLK)); + printf("ddr clock : %dHz\n", mxc_get_clock(MXC_DDR_CLK)); return 0; } diff --git a/arch/arm/include/asm/arch-mx5/clock.h b/arch/arm/include/asm/arch-mx5/clock.h index 1f8a537..bcecd45 100644 --- a/arch/arm/include/asm/arch-mx5/clock.h +++ b/arch/arm/include/asm/arch-mx5/clock.h @@ -32,6 +32,9 @@ enum mxc_clock { MXC_UART_CLK, MXC_CSPI_CLK, MXC_FEC_CLK, + MXC_DDR_CLK, + MXC_NFC_CLK, + MXC_PERIPH_CLK, }; unsigned int imx_decode_pll(unsigned int pll, unsigned int f_ref); @@ -39,5 +42,6 @@ unsigned int imx_decode_pll(unsigned int pll, unsigned int f_ref); u32 imx_get_uartclk(void); u32 imx_get_fecclk(void); unsigned int mxc_get_clock(enum mxc_clock clk); +int mxc_set_clock(u32 ref, u32 freq, u32 clk_type); #endif /* __ASM_ARCH_CLOCK_H */ diff --git a/arch/arm/include/asm/arch-mx5/crm_regs.h b/arch/arm/include/asm/arch-mx5/crm_regs.h index 4ed8eb3..735e4bd 100644 --- a/arch/arm/include/asm/arch-mx5/crm_regs.h +++ b/arch/arm/include/asm/arch-mx5/crm_regs.h @@ -76,6 +76,9 @@ struct mxc_ccm_reg { u32 CCGR4; u32 CCGR5; u32 CCGR6; /* 0x0080 */ +#ifdef CONFIG_MX53 + u32 CCGR7; /* 0x0084 */ +#endif u32 cmeor; }; @@ -84,6 +87,9 @@ struct mxc_ccm_reg { #define MXC_CCM_CACRR_ARM_PODF_MASK 0x7 /* Define the bits in register CBCDR */ +#define MXC_CCM_CBCDR_DDR_HIFREQ_SEL (0x1 << 30) +#define MXC_CCM_CBCDR_DDR_PODF_MASK (0x7 << 27) +#define MXC_CCM_CBCDR_DDR_PODF_OFFSET 27 #define MXC_CCM_CBCDR_EMI_CLK_SEL (0x1 << 26) #define MXC_CCM_CBCDR_PERIPH_CLK_SEL (0x1 << 25) #define MXC_CCM_CBCDR_EMI_PODF_OFFSET 22 diff --git a/arch/arm/include/asm/arch-mx5/imx-regs.h b/arch/arm/include/asm/arch-mx5/imx-regs.h index a1849f8..896a229 100644 --- a/arch/arm/include/asm/arch-mx5/imx-regs.h +++ b/arch/arm/include/asm/arch-mx5/imx-regs.h @@ -98,6 +98,7 @@ #define PLL1_BASE_ADDR (AIPS2_BASE_ADDR + 0x00080000) #define PLL2_BASE_ADDR (AIPS2_BASE_ADDR + 0x00084000) #define PLL3_BASE_ADDR (AIPS2_BASE_ADDR + 0x00088000) +#define PLL4_BASE_ADDR (AIPS2_BASE_ADDR + 0x0008c000) #define AHBMAX_BASE_ADDR (AIPS2_BASE_ADDR + 0x00094000) #define IIM_BASE_ADDR (AIPS2_BASE_ADDR + 0x00098000) #define CSU_BASE_ADDR (AIPS2_BASE_ADDR + 0x0009C000)
Add clock config interface support, so that we can configure CPU or DDR clock in the later init Signed-off-by: Jason Liu <jason.hui@linaro.org> --- arch/arm/cpu/armv7/mx5/clock.c | 551 +++++++++++++++++++++++++++++- arch/arm/include/asm/arch-mx5/clock.h | 4 + arch/arm/include/asm/arch-mx5/crm_regs.h | 6 + arch/arm/include/asm/arch-mx5/imx-regs.h | 1 + 4 files changed, 559 insertions(+), 3 deletions(-)