[U-Boot] [U-BOOT][PATCH 1/5] MX53: Add initial support for the Freescale MX53
Stefano Babic
sbabic at denx.de
Fri Jun 18 11:23:18 CEST 2010
Jason Liu wrote:
> The patch add initial support for the Freescale i.MX53 processor
> (family arm cortex_a8).
>
> Signed-off-by:Jason Liu <r64343 at freescale.com>
Hi Jason,
> ---
> arch/arm/cpu/arm_cortexa8/mx53/Makefile | 48 +++
> arch/arm/cpu/arm_cortexa8/mx53/clock.c | 458 ++++++++++++++++++++++++
> arch/arm/cpu/arm_cortexa8/mx53/iomux.c | 151 ++++++++
> arch/arm/cpu/arm_cortexa8/mx53/lowlevel_init.S | 220 ++++++++++++
> arch/arm/cpu/arm_cortexa8/mx53/soc.c | 93 +++++
> arch/arm/cpu/arm_cortexa8/mx53/speed.c | 39 ++
> arch/arm/cpu/arm_cortexa8/mx53/timer.c | 189 ++++++++++
> arch/arm/cpu/arm_cortexa8/mx53/u-boot.lds | 62 ++++
> arch/arm/include/asm/arch-mx53/clock.h | 51 +++
> 9 files changed, 1311 insertions(+), 0 deletions(-)
You duplicated the structure we currently have under mx51 with new
files, that sometimes are *identical* or have only slight differences
with the original ones. However, we must avoid such kind of duplication
and merge the small changes in a single file.
Agree that the name mx51 is confusing when we introduce another cpu of
the same family, and probably we should change it from mx51 to mx5, as
done in kernel. But again, then cpu/mx5 contains everything related to
the cpu family and we do not to have a directory for each single cpu.
>
> diff --git a/arch/arm/cpu/arm_cortexa8/mx53/Makefile b/arch/arm/cpu/arm_cortexa8/mx53/Makefile
> new file mode 100644
> index 0000000..7cfaa2c
> --- /dev/null
> +++ b/arch/arm/cpu/arm_cortexa8/mx53/Makefile
This file is identical to mx51/Makefile. Do not introduce new copies of
files.
> +#########################################################################
> diff --git a/arch/arm/cpu/arm_cortexa8/mx53/clock.c b/arch/arm/cpu/arm_cortexa8/mx53/clock.c
> new file mode 100644
> index 0000000..93bcb20
The file is quite identical to mx51/clock.c, except some small changes.
We have to merge them.
> +enum pll_clocks {
> + PLL1_CLOCK = 0,
> + PLL2_CLOCK,
> + PLL3_CLOCK,
> + PLL4_CLOCK,
As I see, mx53 have an additional PLL (PLL4) source as the MX51. We can
use the same structure for both.
> +static u32 __decode_pll(struct mxc_pll_reg *pll, u32 infreq)
> +{
> + u32 mfi, mfn, mfd, pd;
> +
> + mfn = __raw_readl(&pll->mfn);
> + mfd = __raw_readl(&pll->mfd) + 1;
> + mfi = __raw_readl(&pll->op);
> + pd = (mfi & 0xF) + 1;
> + mfi = (mfi >> 4) & 0xF;
> + mfi = (mfi >= 5) ? mfi : 5;
> +
> + return ((4 * (infreq / 1000) * (mfi * mfd + mfn)) / (mfd * pd)) * 1000;
> +}
This function is identical to the mx51 function.
> +
> +static u32 __get_mcu_main_clk(void)
> +{
> + u32 reg, freq;
> + reg = (__raw_readl(&mxc_ccm->cacrr) & MXC_CCM_CACRR_ARM_PODF_MASK) >>
> + MXC_CCM_CACRR_ARM_PODF_OFFSET;
> + freq = __decode_pll(mxc_plls[PLL1_CLOCK], CONFIG_MX53_HCLK_FREQ);
For most functions, the differences is due only to the usage of a
different constant. Can we do in another way ? We have already a
CONFIG_MX51 switch, and, if we do not find a cleverer solution, we could
add some kind of CONFIG_MX53 and put the #ifdef where we define this
constants to get a different value.
> +static u32 __get_periph_clk(void)
> +{
> + u32 reg;
> + reg = __raw_readl(&mxc_ccm->cbcdr);
> + if (!(reg & MXC_CCM_CBCDR_PERIPH_CLK_SEL))
> + return __decode_pll(mxc_plls[PLL2_CLOCK], CONFIG_MX53_HCLK_FREQ);
> +
> + reg = __raw_readl(&mxc_ccm->cbcmr);
> + switch ((reg & MXC_CCM_CBCMR_PERIPH_CLK_SEL_MASK) >>
> + MXC_CCM_CBCMR_PERIPH_CLK_SEL_OFFSET) {
> + case 0:
> + return __decode_pll(mxc_plls[PLL1_CLOCK], CONFIG_MX53_HCLK_FREQ);
Ditto
> +static u32 __get_ipg_clk(void)
> +{
> + u32 ahb_podf, ipg_podf;
> +
> + ahb_podf = __raw_readl(&mxc_ccm->cbcdr);
> + ipg_podf = (ahb_podf & MXC_CCM_CBCDR_IPG_PODF_MASK) >>
> + MXC_CCM_CBCDR_IPG_PODF_OFFSET;
> + ahb_podf = (ahb_podf & MXC_CCM_CBCDR_AHB_PODF_MASK) >>
> + MXC_CCM_CBCDR_AHB_PODF_OFFSET;
> + return __get_periph_clk() / ((ahb_podf + 1) * (ipg_podf + 1));
> +}
Except for variable names, identical to mx51 function
> +static u32 __get_uart_clk(void)
> +{
> + u32 freq = 0, 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_MX53_HCLK_FREQ);
Ditto
> +
> +
> +static u32 __get_cspi_clk(void)
> +{
Again
> +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);
> +}
These new function can be used for MX51 as well. It makes more sense to
get a single file for all MX5 family.
> +static u32 __get_esdhc1_clk(void)
> +{
> + u32 ret_val = 0, div, pre_pdf, pdf;
> + u32 cscmr1 = __raw_readl(&mxc_ccm->cscmr1);
> + u32 cscdr1 = __raw_readl(&mxc_ccm->cscdr1);
> + u32 esdh1_clk_sel;
> +
> + esdh1_clk_sel = (cscmr1 & MXC_CCM_CSCMR1_ESDHC1_MSHC1_CLK_SEL_MASK) \
> + >> MXC_CCM_CSCMR1_ESDHC1_MSHC1_CLK_SEL_OFFSET;
__get_esdhc1_clk(void), __get_esdhc2_clk(void) and _get_esdhc3_clk(void)
are the same function, they use only a different constant to access the
registers. can we factorize them ?
> +/*
> + * Dump some core clockes.
> + */
> +int do_mx53_showclocks(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
> +{
> + u32 freq;
> + freq = __decode_pll(mxc_plls[PLL1_CLOCK], CONFIG_MX53_HCLK_FREQ);
> + printf("mx53 pll1: %dMHz\n", freq / 1000000);
> + freq = __decode_pll(mxc_plls[PLL2_CLOCK], CONFIG_MX53_HCLK_FREQ);
> + printf("mx53 pll2: %dMHz\n", freq / 1000000);
> + freq = __decode_pll(mxc_plls[PLL3_CLOCK], CONFIG_MX53_HCLK_FREQ);
> + printf("mx53 pll3: %dMHz\n", freq / 1000000);
Again, this function must be merged with do_mx51_showclocks().
> diff --git a/arch/arm/cpu/arm_cortexa8/mx53/iomux.c b/arch/arm/cpu/arm_cortexa8/mx53/iomux.c
> new file mode 100644
> index 0000000..a5d9bbe
> --- /dev/null
> +++ b/arch/arm/cpu/arm_cortexa8/mx53/iomux.c
This file seems a previous version of iomux.c for MX51. There are only a
few differences with the MX51 counterparts, and contains some issue that
were reported by ML and solved in the actual version. You should use
mx51/iomux.c adding your changes.
> diff --git a/arch/arm/cpu/arm_cortexa8/mx53/lowlevel_init.S b/arch/arm/cpu/arm_cortexa8/mx53/lowlevel_init.S
> new file mode 100644
> index 0000000..e32ec86
> --- /dev/null
> +++ b/arch/arm/cpu/arm_cortexa8/mx53/lowlevel_init.S
Again, we should use only one for both cpus.
> +.macro setup_pll pll, freq
> + ldr r0, =\pll
> + ldr r1, =0x00001232
> + str r1, [r0, #PLL_DP_CTL]
> + mov r1, #0x2
> + str r1, [r0, #PLL_DP_CONFIG]
> +
> + ldr r1, W_DP_OP_\freq
> + str r1, [r0, #PLL_DP_OP]
> + str r1, [r0, #PLL_DP_HFS_OP]
> +
> + ldr r1, W_DP_MFD_\freq
> + str r1, [r0, #PLL_DP_MFD]
> + str r1, [r0, #PLL_DP_HFS_MFD]
> +
> + ldr r1, W_DP_MFN_\freq
> + str r1, [r0, #PLL_DP_MFN]
> + str r1, [r0, #PLL_DP_HFS_MFN]
> +
> + ldr r1, =0x00001232
> + str r1, [r0, #PLL_DP_CTL]
> +1: ldr r1, [r0, #PLL_DP_CTL]
> + ands r1, r1, #0x1
> + beq 1b
> +.endm
can we use the same macro for MX51, too ?
> +.section ".text.init", "x"
> +
> +.globl lowlevel_init
> +lowlevel_init:
> +
> +#ifdef ENABLE_IMPRECISE_ABORT
> + mrs r1, spsr /* save old spsr */
> + mrs r0, cpsr /* read out the cpsr */
> + bic r0, r0, #0x100 /* clear the A bit */
> + msr spsr, r0 /* update spsr */
> + add lr, pc, #0x8 /* update lr */
> + movs pc, lr /* update cpsr */
> + nop
> + nop
> + nop
> + nop
> + msr spsr, r1 /* restore old spsr */
> +#endif
I do not find ENABLE_IMPRECISE_ABORT at all. It is not defined, and this
seems dead code. Can you explain why and if this code is needed ? In
this case, you have to define ENABLE_IMPRECISE_ABORT somewhere.
> + /* ARM errata ID #468414 */
> + mrc 15, 0, r1, c1, c0, 1
> + orr r1, r1, #(1 << 5) /* enable L1NEON bit */
> + mcr 15, 0, r1, c1, c0, 1
This seems useful for MX51, too. It is a pity to get this correction
only for one specifical CPU.
> +++ b/arch/arm/cpu/arm_cortexa8/mx53/soc.c
It must be merged with mx51/soc.c
> diff --git a/arch/arm/cpu/arm_cortexa8/mx53/speed.c b/arch/arm/cpu/arm_cortexa8/mx53/speed.c
> +int get_clocks(void)
> +{
> + DECLARE_GLOBAL_DATA_PTR;
> +
> +#ifdef CONFIG_FSL_ESDHC
> + gd->sdhc_clk = mxc_get_clock(MXC_ESDHC_CLK);
> +#endif
Except for the constant, identical to mx51/speed.c. It must be merged.
> diff --git a/arch/arm/cpu/arm_cortexa8/mx53/timer.c b/arch/arm/cpu/arm_cortexa8/mx53/timer.c
Again, it must be merged with mx51/timer.c
> diff --git a/arch/arm/cpu/arm_cortexa8/mx53/u-boot.lds b/arch/arm/cpu/arm_cortexa8/mx53/u-boot.lds
> + {
> + board/freescale/mx53evk/flash_header.o
During the review of MX51, it was accepted another solution to link the
DCD and generally the boot structure to u-boot code. Check the imximage
and doc/README.imximage. The current solution is consistent with other
architectures, too, such as Kirchwood. Linking the DCD table with
flash_header is not accepted.
And u-boot.lds can be the same for MX51 and MX53.
> diff --git a/arch/arm/include/asm/arch-mx53/clock.h b/arch/arm/include/asm/arch-mx53/clock.h
> new file mode 100644
> index 0000000..e986462
> --- /dev/null
> +++ b/arch/arm/include/asm/arch-mx53/clock.h
> +
> +enum mxc_clock {
> + MXC_ARM_CLK = 0,
> + MXC_PER_CLK,
> + MXC_AHB_CLK,
> + MXC_IPG_CLK,
> + MXC_IPG_PERCLK,
> + MXC_UART_CLK,
> + MXC_CSPI_CLK,
> + MXC_AXI_A_CLK,
> + MXC_AXI_B_CLK,
> + MXC_EMI_SLOW_CLK,
> + MXC_DDR_CLK,
> + MXC_ESDHC_CLK,
> + MXC_ESDHC2_CLK,
> + MXC_ESDHC3_CLK,
> + MXC_ESDHC4_CLK,
> +};
There are some additional enum as for MX51. Again, we can use only one
file for both. And I propose to change arch/arm/include/asm/arch-mx51 to
arch/arm/include/asm/arch-mx5.
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 at denx.de
=====================================================================
More information about the U-Boot
mailing list