Re: [U-Boot] [PATCH v5 09/13] imx: imx7d: Add SoC system support

23 Aug 2015

Hi Adrian,
On 11/08/2015 18:19, Adrian Alonso wrote:
...

Add SoC system support, Misc arch dependent functions for
system bring up:
s_init: system init enable clock base settings
enable_caches: configures Cortex-A7 L2 caches
get_boot_device: identifies boot device

Signed-off-by: Adrian Alonso aalonso@freescale.com
Signed-off-by: Peng Fan Peng.Fan@freescale.com
Signed-off-by: Ye.Li B37916@freescale.com

Despite the SOC is different, I see several functions that can be
factorized :
...
Changes for V2: Split from patch imx: imx7d: initial arch level support
Changes for V3: Resend
Changes for V4: Resend
Changes for V5: Resend
arch/arm/cpu/armv7/mx7/soc.c                | 360 ++++++++++++++++++++++++++++
 arch/arm/include/asm/arch-imx/cpu.h         |   1 +
 arch/arm/include/asm/imx-common/boot_mode.h |  21 ++
 3 files changed, 382 insertions(+)
 create mode 100644 arch/arm/cpu/armv7/mx7/soc.c

diff --git a/arch/arm/cpu/armv7/mx7/soc.c b/arch/arm/cpu/armv7/mx7/soc.c
new file mode 100644
index 0000000..4213dad
--- /dev/null
+++ b/arch/arm/cpu/armv7/mx7/soc.c
@@ -0,0 +1,360 @@
+/*


Copyright (C) 2015 Freescale Semiconductor, Inc.







SPDX-License-Identifier:	GPL-2.0+


*/


+#include <common.h>
+#include <asm/armv7.h>
+#include <asm/errno.h>
+#include <asm/io.h>
+#include <asm/arch/imx-regs.h>
+#include <asm/arch/clock.h>
+#include <asm/arch/sys_proto.h>
+#include <asm/imx-common/boot_mode.h>
+#include <asm/imx-common/dma.h>
+#include <stdbool.h>
+#include <asm/arch/crm_regs.h>
+#include <dm.h>
+#include <imx_thermal.h>
+#ifdef CONFIG_VIDEO_MXS
+#include <mxsfb.h>
+#endif



+struct src *src_reg = (struct src *)SRC_BASE_ADDR;



+#if defined(CONFIG_IMX_THERMAL)
+static const struct imx_thermal_plat imx7_thermal_plat = {

.regs = (void *)ANATOP_BASE_ADDR,
.fuse_bank = 3,
.fuse_word = 3,

+};



+U_BOOT_DEVICE(imx7_thermal) = {

.name = "imx_thermal",
.platdata = &imx7_thermal_plat,

+};
+#endif



+u32 get_cpu_rev(void)
+{

struct mxc_ccm_anatop_reg *ccm_anatop = (struct mxc_ccm_anatop_reg *)
				 ANATOP_BASE_ADDR;


u32 reg = readl(&ccm_anatop->digprog);
u32 type = (reg >> 16) & 0xff;

reg &= 0xff;
return (type << 12) | reg;

+}
...



+#ifdef CONFIG_REVISION_TAG
+u32 __weak get_board_rev(void)
+{

u32 cpurev = get_cpu_rev();
u32 type = ((cpurev >> 12) & 0xff);

if (type == MXC_CPU_MX7D)
cpurev = (MXC_CPU_MX7D) << 12 | (cpurev & 0xFFF);



return cpurev;

+}
I do not understand the code - I understand you take it from i.MX6, but
i.MX6 has some exceptions. If you shift left and shift right cpurev you
get the same...
If I am not wron, this code is identical as:
u32 __weak get_board_rev(void)
{
    return get_cpu_rev();
}
And then identical to mx5. Can we factorize it ?
...
+#endif



+static void init_aips(void)
+{

struct aipstz_regs *aips1, *aips2, *aips3;

aips1 = (struct aipstz_regs *)AIPS1_ON_BASE_ADDR;
aips2 = (struct aipstz_regs *)AIPS2_ON_BASE_ADDR;
aips3 = (struct aipstz_regs *)AIPS3_ON_BASE_ADDR;

/*
* Set all MPROTx to be non-bufferable, trusted for R/W,


* not forced to user-mode.


*/


writel(0x77777777, &aips1->mprot0);
writel(0x77777777, &aips1->mprot1);
writel(0x77777777, &aips2->mprot0);
writel(0x77777777, &aips2->mprot1);
writel(0x77777777, &aips3->mprot0);
writel(0x77777777, &aips3->mprot1);

/*
* Set all OPACRx to be non-bufferable, not require


* supervisor privilege level for access,allow for


* write access and untrusted master access.


*/


writel(0x00000000, &aips1->opacr0);
writel(0x00000000, &aips1->opacr1);
writel(0x00000000, &aips1->opacr2);
writel(0x00000000, &aips1->opacr3);
writel(0x00000000, &aips1->opacr4);
writel(0x00000000, &aips2->opacr0);
writel(0x00000000, &aips2->opacr1);
writel(0x00000000, &aips2->opacr2);
writel(0x00000000, &aips2->opacr3);
writel(0x00000000, &aips2->opacr4);
writel(0x00000000, &aips3->opacr0);
writel(0x00000000, &aips3->opacr1);
writel(0x00000000, &aips3->opacr2);
writel(0x00000000, &aips3->opacr3);
writel(0x00000000, &aips3->opacr4);

+}
This is also identical to i.MX6 - common ?
...



+static void imx_set_pcie_phy_power_down(void)
+{

/* TODO */

+}
I suggest you drop it and you add with a following up patch when it is
defined.
...



+static void imx_set_wdog_powerdown(bool enable)
+{

struct wdog_regs *wdog1 = (struct wdog_regs *)WDOG1_BASE_ADDR;
struct wdog_regs *wdog2 = (struct wdog_regs *)WDOG2_BASE_ADDR;
struct wdog_regs *wdog3 = (struct wdog_regs *)WDOG3_BASE_ADDR;
struct wdog_regs *wdog4 = (struct wdog_regs *)WDOG4_BASE_ADDR;

writew(enable, &wdog1->wmcr);
writew(enable, &wdog2->wmcr);
writew(enable, &wdog3->wmcr);
writew(enable, &wdog4->wmcr);

+}



This is a further candidate, even if wdo3 and wdog4 are specific here.
...
+static void set_epdc_qos(void)
+{
+#define REGS_QOS_BASE     QOSC_IPS_BASE_ADDR
+#define REGS_QOS_EPDC     (QOSC_IPS_BASE_ADDR + 0x3400)
+#define REGS_QOS_PXP0     (QOSC_IPS_BASE_ADDR + 0x2C00)
+#define REGS_QOS_PXP1     (QOSC_IPS_BASE_ADDR + 0x3C00)


writel(0, REGS_QOS_BASE);  /*  Disable clkgate & soft_reset */
writel(0, REGS_QOS_BASE + 0x60);  /*  Enable all masters */
writel(0, REGS_QOS_EPDC);   /*  Disable clkgate & soft_reset */
writel(0, REGS_QOS_PXP0);   /*  Disable clkgate & soft_reset */
writel(0, REGS_QOS_PXP1);   /*  Disable clkgate & soft_reset */

writel(0x0f020722, REGS_QOS_EPDC + 0xd0);   /*  WR, init = 7 with red flag */
writel(0x0f020722, REGS_QOS_EPDC + 0xe0);   /*  RD,  init = 7 with red flag */

writel(1, REGS_QOS_PXP0);   /*  OT_CTRL_EN =1 */
writel(1, REGS_QOS_PXP1);   /*  OT_CTRL_EN =1 */

writel(0x0f020222, REGS_QOS_PXP0 + 0x50);   /*  WR,  init = 2 with red flag */
writel(0x0f020222, REGS_QOS_PXP1 + 0x50);   /*  WR,  init = 2 with red flag */
writel(0x0f020222, REGS_QOS_PXP0 + 0x60);   /*  rD,  init = 2 with red flag */
writel(0x0f020222, REGS_QOS_PXP1 + 0x60);   /*  rD,  init = 2 with red flag */
writel(0x0f020422, REGS_QOS_PXP0 + 0x70);   /*  tOTAL,  init = 4 with red flag */
writel(0x0f020422, REGS_QOS_PXP1 + 0x70);   /*  TOTAL,  init = 4 with red flag */

There are two issues here:
- reg + offset is forbidden in u-boot code. You need a structure.
- constants should be defined. Comment is great, but it is often not
correctly update. Better to add defines and avoid hexadecimal constants.
...


writel(0xe080, IOMUXC_GPR_BASE_ADDR + 0x0034); /* EPDC AW/AR CACHE ENABLE */

Ditto
...
+}



+int arch_cpu_init(void)
+{

init_aips();

/* Disable PDE bit of WMCR register */
imx_set_wdog_powerdown(false);

imx_set_pcie_phy_power_down();


+#ifdef CONFIG_APBH_DMA

/* Start APBH DMA */
mxs_dma_init();

+#endif


set_epdc_qos();

return 0;

+}



+#ifdef CONFIG_BOARD_POSTCLK_INIT
+int board_postclk_init(void)
+{

/*
* We do not need to set LDO_SOC as i.mx6, since LDO_ARM and LDO_SOC


* does not exist. Check "Figure 7-9. i.MX7Dual Power Diagram"


*/


return 0;

+}
+#endif
Ok, but you do not share configuration file with i.MX6. If you do, you
need this. CONFIG_BOARD_POSTCLK_INIT should never be set by i.MX7, as
you do not need, and this empty function is not needed.
...



+#ifdef CONFIG_SERIAL_TAG
+void get_board_serial(struct tag_serialnr *serialnr)
+{

struct ocotp_regs *ocotp = (struct ocotp_regs *)OCOTP_BASE_ADDR;
struct fuse_bank *bank = &ocotp->bank[0];
struct fuse_bank0_regs *fuse =
(struct fuse_bank0_regs *)bank->fuse_regs;



serialnr->low = fuse->tester0;
serialnr->high = fuse->tester1;

+}
+#endif
Interesting - is the serial delivered with the SOC by Freescale ?
...



+#ifndef CONFIG_SYS_DCACHE_OFF
+void enable_caches(void)
+{
+#if defined(CONFIG_SYS_ARM_CACHE_WRITETHROUGH)

enum dcache_option option = DCACHE_WRITETHROUGH;

+#else

enum dcache_option option = DCACHE_WRITEBACK;

+#endif


/* Avoid random hang when download by usb */
invalidate_dcache_all();

/* Enable D-cache. I-cache is already enabled in start.S */
dcache_enable();

/* Enable caching on OCRAM and ROM */
mmu_set_region_dcache_behaviour(ROMCP_ARB_BASE_ADDR,
			ROMCP_ARB_END_ADDR,


			option);


mmu_set_region_dcache_behaviour(IRAM_BASE_ADDR,
			IRAM_SIZE,


			option);



+}
+#endif
Identical to i.MX6, please do not duplicate - we need a common one.
...



+#if defined(CONFIG_FEC_MXC)
+void imx_get_mac_from_fuse(int dev_id, unsigned char *mac)
+{

struct ocotp_regs *ocotp = (struct ocotp_regs *)OCOTP_BASE_ADDR;
struct fuse_bank *bank = &ocotp->bank[9];
struct fuse_bank9_regs *fuse =
(struct fuse_bank9_regs *)bank->fuse_regs;



if (0 == dev_id) {
u32 value = readl(&fuse->mac_addr1);


mac[0] = (value >> 8);


mac[1] = value;



value = readl(&fuse->mac_addr0);


mac[2] = value >> 24;


mac[3] = value >> 16;


mac[4] = value >> 8;


mac[5] = value;


} else {
u32 value = readl(&fuse->mac_addr2);


mac[0] = value >> 24;


mac[1] = value >> 16;


mac[2] = value >> 8;


mac[3] = value;



value = readl(&fuse->mac_addr1);


mac[4] = value >> 24;


mac[5] = value >> 16;


}

+}
+#endif



+void boot_mode_apply(uint32_t cfg_val)
+{

uint32_t reg;
writel(cfg_val, &src_reg->gpr9);
reg = readl(&src_reg->gpr10);
if (cfg_val)
reg |= 1 << 28;


else
reg &= ~(1 << 28);


writel(reg, &src_reg->gpr10);

+}
Identical to i.MX6, no duplication, thanks !
...



+void set_wdog_reset(struct wdog_regs *wdog)
+{

u32 reg = readw(&wdog->wcr);
/*
* Output WDOG_B signal to reset external pmic or POR_B decided by


* the board desgin. Without external reset, the peripherals/DDR/


* PMIC are not reset, that may cause system working abnormal.


*/


reg = readw(&wdog->wcr);
reg |= 1 << 3;
/*
* WDZST bit is write-once only bit. Align this bit in kernel,


* otherwise kernel code will have no chance to set this bit.


*/


reg |= 1 << 0;
writew(reg, &wdog->wcr);

+}
I do not understand if this is common to all i.MX7 or is it related to
the MX7D Sabresd only. Comments suggest that is board related, and it is
called by your board_late_init(), not from cpu code.
If this is hardware related to the board, code should be moved in board
directory.
...



+/*


cfg_val will be used for



Boot_cfg4[7:0]:Boot_cfg3[7:0]:Boot_cfg2[7:0]:Boot_cfg1[7:0]



After reset, if GPR10[28] is 1, ROM will copy GPR9[25:0]



to SBMR1, which will determine the boot device.


*/

+const struct boot_mode soc_boot_modes[] = {

{"ecspi1:0",	MAKE_CFGVAL(0x00, 0x60, 0x00, 0x00)},
{"ecspi1:1",	MAKE_CFGVAL(0x40, 0x62, 0x00, 0x00)},
{"ecspi1:2",	MAKE_CFGVAL(0x80, 0x64, 0x00, 0x00)},
{"ecspi1:3",	MAKE_CFGVAL(0xc0, 0x66, 0x00, 0x00)},

{"weim",	MAKE_CFGVAL(0x00, 0x50, 0x00, 0x00)},
{"qspi1",	MAKE_CFGVAL(0x10, 0x40, 0x00, 0x00)},
/* 4 bit bus width */
{"usdhc1",	MAKE_CFGVAL(0x10, 0x10, 0x00, 0x00)},
{"usdhc2",	MAKE_CFGVAL(0x10, 0x14, 0x00, 0x00)},
{"usdhc3",	MAKE_CFGVAL(0x10, 0x18, 0x00, 0x00)},
{"mmc1",	MAKE_CFGVAL(0x10, 0x20, 0x00, 0x00)},
{"mmc2",	MAKE_CFGVAL(0x10, 0x24, 0x00, 0x00)},
{"mmc3",	MAKE_CFGVAL(0x10, 0x28, 0x00, 0x00)},
{NULL,		0},

+};



+enum boot_device get_boot_device(void)
+{

struct bootrom_sw_info **p =
(struct bootrom_sw_info **)ROM_SW_INFO_ADDR;



enum boot_device boot_dev = SD1_BOOT;
u8 boot_type = (*p)->boot_dev_type;
u8 boot_instance = (*p)->boot_dev_instance;

switch (boot_type) {
case BOOT_TYPE_SD:
boot_dev = boot_instance + SD1_BOOT;


break;


case BOOT_TYPE_MMC:
boot_dev = boot_instance + MMC1_BOOT;


break;


case BOOT_TYPE_NAND:
boot_dev = NAND_BOOT;


break;


case BOOT_TYPE_QSPI:
boot_dev = QSPI_BOOT;


break;


case BOOT_TYPE_WEIM:
boot_dev = WEIM_NOR_BOOT;


break;


case BOOT_TYPE_SPINOR:
boot_dev = SPI_NOR_BOOT;


break;


default:
break;


}

return boot_dev;

+}



+void s_init(void)
+{
+#if !defined CONFIG_SPL_BUILD

/* Enable SMP mode for CPU0, by setting bit 6 of Auxiliary Ctl reg */
asm volatile(
	"mrc p15, 0, r0, c1, c0, 1\n"


	"orr r0, r0, #1 << 6\n"


	"mcr p15, 0, r0, c1, c0, 1\n");



+#endif

/* clock configuration. */
clock_init();

return;

+}



+void reset_misc(void)
+{
+#ifdef CONFIG_VIDEO_MXS

lcdif_power_down();

+#endif
+}
diff --git a/arch/arm/include/asm/arch-imx/cpu.h b/arch/arm/include/asm/arch-imx/cpu.h
index c7f9fff..f4327ac 100644
--- a/arch/arm/include/asm/arch-imx/cpu.h
+++ b/arch/arm/include/asm/arch-imx/cpu.h
@@ -15,6 +15,7 @@
 #define MXC_CPU_MX6D		0x67
 #define MXC_CPU_MX6DP		0x68
 #define MXC_CPU_MX6QP		0x69
+#define MXC_CPU_MX7D		0x72
#define CS0_128					0
 #define CS0_64M_CS1_64M				1
diff --git a/arch/arm/include/asm/imx-common/boot_mode.h b/arch/arm/include/asm/imx-common/boot_mode.h
index de0205c..a8239f2 100644
--- a/arch/arm/include/asm/imx-common/boot_mode.h
+++ b/arch/arm/include/asm/imx-common/boot_mode.h
@@ -9,6 +9,27 @@
 #define MAKE_CFGVAL(cfg1, cfg2, cfg3, cfg4) \
   ((cfg4) << 24) | ((cfg3) << 16) | ((cfg2) << 8) | (cfg1)
+enum boot_device {

WEIM_NOR_BOOT,
ONE_NAND_BOOT,
PATA_BOOT,
SATA_BOOT,
I2C_BOOT,
SPI_NOR_BOOT,
SD1_BOOT,
SD2_BOOT,
SD3_BOOT,
SD4_BOOT,
MMC1_BOOT,
MMC2_BOOT,
MMC3_BOOT,
MMC4_BOOT,
NAND_BOOT,
QSPI_BOOT,
UNKNOWN_BOOT,
BOOT_DEV_NUM = UNKNOWN_BOOT,

+};
There are already some defines. They are in include/spl.h, set for all
architecture. They should not be redefined for a single SOC.
...



struct boot_mode {
   const char *name;
   unsigned cfg_val;
Best regards,
Stefano Babic
-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic@denx.de
=====================================================================

    