[U-Boot] [PATCH v5 09/13] imx: imx7d: Add SoC system support
Alonso Adrian
aalonso at freescale.com
Thu Aug 27 01:54:36 CEST 2015
Hi Stefano,
> -----Original Message-----
> From: Stefano Babic [mailto:sbabic at denx.de]
> Sent: Sunday, August 23, 2015 1:21 PM
> To: Alonso Lazcano Adrian-B38018 <aalonso at freescale.com>; u-
> boot at lists.denx.de; sbabic at denx.de; Estevam Fabio-R49496
> <Fabio.Estevam at freescale.com>
> Cc: otavio at ossystems.com.br; Li Frank-B20596 <Frank.Li at freescale.com>;
> Garg Nitin-B37173 <nitin.garg at freescale.com>
> Subject: Re: [PATCH v5 09/13] imx: imx7d: Add SoC system support
>
> 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 at freescale.com>
> > Signed-off-by: Peng Fan <Peng.Fan at freescale.com>
> > Signed-off-by: Ye.Li <B37916 at 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 ?
[Adrian] I was reviewing if get_board_rev can be reused across mx5, mx6 and mx7; and seems that is not as simple, the problem
Is from MX6 and MX7 who's CPU revision ID are shared across different CPU's for example iMX6 Dual and Quad share the same ID (0x63)
And those ara the exceptions the you mention, when cpu type is iMX6 Dual get_board_rev overrides and returns iMX6Q ID so we can pass
The proper TAG that kernel would expect. So iMX7 get_board_rev is likely to have the issue of shared CPU ID's; so no point in factorize.
>
> > +#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 ?
[Adrian] See patch arm: imx: imx-common: init: move arch init common setup
And let me know what u think.
>
> > +
> > +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.
[Adrian] Ok, will do that
>
> > +
> > +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.
[Adrian] Sure see arm: imx: imx-common: init: move arch init common setup
>
> > +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.
[Adrian] Ok, will address this.
>
> > +
> > + 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.
>
[Adrian] Good catch :)
> > +
> > +#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 ?
[Adrian] Yes iMX7 will have a unique serial ID from factory.
>
> > +
> > +#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.
[Adrian] See patch arm: imx: common rework cache settings for imx6
>
> > +
> > +#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 !
[Adrian] See patch: arm: imx: imx-common: init: move arch init common setup
>
> > +
> > +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.
[Adrian] Ok, I can check on this, as far I remember the enumeration matches
iMX6/iMX7 boot device ID's that's likely why we are redeclaring it, but let me check.
>
> > +
> > 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 at denx.de
> ============================================================
> =========
More information about the U-Boot
mailing list