On Fri, Jan 27, 2017 at 4:18 PM, Eric Nelson <e...@nelint.com> wrote: > Hi Jagan, > > Love this patch! This was long overdue. > > On 01/27/2017 07:12 AM, Jagan Teki wrote: >> Use meaningful meacros IMX6_BMODE_*, instead of numerical >> number in boot mode detection code. > > s/meacros/macros/ > >> >> Cc: Stefano Babic <sba...@denx.de> >> Cc: Tim Harvey <thar...@gateworks.com> >> Signed-off-by: Jagan Teki <ja...@openedev.com> >> --- >> arch/arm/imx-common/spl.c | 39 >> ++++++++++++++++++----------- >> arch/arm/include/asm/imx-common/sys_proto.h | 34 +++++++++++++++++++++++++ >> 2 files changed, 58 insertions(+), 15 deletions(-) >> >> diff --git a/arch/arm/imx-common/spl.c b/arch/arm/imx-common/spl.c >> index fc3704b..38789b2 100644 >> --- a/arch/arm/imx-common/spl.c >> +++ b/arch/arm/imx-common/spl.c >> @@ -30,39 +30,48 @@ u32 spl_boot_device(void) >> if ((((bmode >> 24) & 0x03) == 0x01) || /* Serial Downloader */ >> (imx6_is_bmode_from_gpr9() && (reg == 1))) >> return BOOT_DEVICE_UART; >> + >> /* BOOT_CFG1[7:4] - see IMX6DQRM Table 8-8 */ >> - switch ((reg & 0x000000FF) >> 4) { >> + switch ((reg & IMX6_BMODE_MASK) >> IMX6_BMODE_SHIFT) { >> /* EIM: See 8.5.1, Table 8-9 */ >> - case 0x0: >> + case IMX6_BMODE_EMI: >> /* BOOT_CFG1[3]: NOR/OneNAND Selection */ >> - if ((reg & 0x00000008) >> 3) >> + switch ((reg & IMX6_BMODE_EMI_MASK) >> IMX6_BMODE_EMI_SHIFT) { >> + case IMX6_BMODE_ONENAND: >> return BOOT_DEVICE_ONENAND; >> - else >> + case IMX6_BMODE_NOR: >> return BOOT_DEVICE_NOR; >> - break; >> + } >> /* SATA: See 8.5.4, Table 8-20 */ >> - case 0x2: >> + case IMX6_BMODE_SATA: >> return BOOT_DEVICE_SATA; >> /* Serial ROM: See 8.5.5.1, Table 8-22 */ >> - case 0x3: >> + case IMX6_BMODE_SERIAL: >> /* BOOT_CFG4[2:0] */ >> - switch ((reg & 0x07000000) >> 24) { >> - case 0x0 ... 0x4: >> + switch ((reg & IMX6_BMODE_SERIAL_MASK) >> >> + IMX6_BMODE_SERIAL_SHIFT) { >> + case IMX6_BMODE_ECSPI1: >> + case IMX6_BMODE_ECSPI2: >> + case IMX6_BMODE_ECSPI3: >> + case IMX6_BMODE_ECSPI4: >> + case IMX6_BMODE_ECSPI5: >> return BOOT_DEVICE_SPI; >> - case 0x5 ... 0x7: >> + case IMX6_BMODE_I2C1: >> + case IMX6_BMODE_I2C2: >> + case IMX6_BMODE_I2C3: >> return BOOT_DEVICE_I2C; >> } >> break; >> /* SD/eSD: 8.5.3, Table 8-15 */ >> - case 0x4: >> - case 0x5: >> + case IMX6_BMODE_SD: >> + case IMX6_BMODE_ESD: >> return BOOT_DEVICE_MMC1; >> /* MMC/eMMC: 8.5.3 */ >> - case 0x6: >> - case 0x7: >> + case IMX6_BMODE_MMC: >> + case IMX6_BMODE_EMMC: >> return BOOT_DEVICE_MMC1; >> /* NAND Flash: 8.5.2, Table 8-10 */ >> - case 0x8: >> + case IMX6_BMODE_NAND: >> return BOOT_DEVICE_NAND; >> } >> return BOOT_DEVICE_NONE; >> diff --git a/arch/arm/include/asm/imx-common/sys_proto.h >> b/arch/arm/include/asm/imx-common/sys_proto.h >> index 99e3869..d0cf3f1 100644 >> --- a/arch/arm/include/asm/imx-common/sys_proto.h >> +++ b/arch/arm/include/asm/imx-common/sys_proto.h >> @@ -42,6 +42,40 @@ >> #ifdef CONFIG_MX6 >> #define IMX6_SRC_GPR10_BMODE BIT(28) >> >> +#define IMX6_BMODE_MASK GENMASK(7, 0) > > This is a number (4), not a mask:
This is 0xff not 4 - switch ((reg & 0x000000FF) >> 4) { + switch ((reg & IMX6_BMODE_MASK) >> IMX6_BMODE_SHIFT) { >> +#define IMX6_BMODE_SHIFT BIT(2) >> +#define IMX6_BMODE_EMI_MASK BIT(3) > > Ditto (number, not mask): The reason for calling this as mask as the reg value is and'ing to mask value of 8 (which is last 0 and 1 bits) - if ((reg & 0x00000008) >> 3) + switch ((reg & IMX6_BMODE_EMI_MASK) >> IMX6_BMODE_EMI_SHIFT) { >> +#define IMX6_BMODE_EMI_SHIFT GENMASK(1, 0) > > Since there's also a "serial download mode", I'd prefer that these > say "SERIAL_NOR" to avoid confusion. Since serial here is also denoting I2C better to have SERIAL and we can use 'serial download' as SERIAL_DOWNLOAD or something similar. > >> +#define IMX6_BMODE_SERIAL_MASK GENMASK(26, 24) >> +#define IMX6_BMODE_SERIAL_SHIFT GENMASK(4, 3) >> + > > I'd prefer macros for these because they'd show the > values directly, making a comparison with the RM > easier. But these macro's making bit functioning smooth. thanks! -- Jagan Teki Free Software Engineer | www.openedev.com U-Boot, Linux | Upstream Maintainer Hyderabad, India. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot