Hi, Stefano
-----Original Message----- From: Stefano Babic [mailto:sbabic@denx.de] Sent: 2010年6月18日 17:23 To: Liu Hui-R64343 Cc: wd@denx.de; u-boot@lists.denx.de Subject: Re: [U-Boot] [U-BOOT][PATCH 1/5] MX53: Add initial support for the Freescale MX53
Jason Liu wrote:
The patch add initial support for the Freescale i.MX53 processor (family arm cortex_a8).
Signed-off-by:Jason Liu r64343@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.
Yes, agree. I will do some changes to incorporate mx51/mx53 and later mx5x by changing the mx51 to mx5.
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.
Yes,
+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.
Yes, agree.
+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.
Yes,
+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.
Yes, I will merge it in.
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 ?
In fact, MX53 has some big difference is that it support 400MHz DDR which lead to the ddr clock init part not like MX51, but I will try to do it in one file.
+.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 */nopnopnop- 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.
Yes, It's obsolete. I will remove it.
- /* 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.oDuring 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.
In fact, MX53 has much change on the flash header which means the flash header for mx51 does not applied to mx53. It will need much change. And later MX5X does not guarantee to be the same as MX53.If link DCD with flash header not accept, then I will change related files such as tools/imximage.c.
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.
OK, agree.
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 =====================================================================