[U-Boot] [PATCH v2 03/15] imx: Use IMX6_BMODE_* macros instead of numericals
Use meaningful meacros IMX6_BMODE_*, instead of numerical number in boot mode detection code. Cc: Stefano Babic Cc: Tim Harvey Signed-off-by: Jagan Teki --- 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 & 0x00FF) >> 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 & 0x0008) >> 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 & 0x0700) >> 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_MASKGENMASK(7, 0) +#defineIMX6_BMODE_SHIFTBIT(2) +#define IMX6_BMODE_EMI_MASKBIT(3) +#define IMX6_BMODE_EMI_SHIFT GENMASK(1, 0) +#define IMX6_BMODE_SERIAL_MASK GENMASK(26, 24) +#define IMX6_BMODE_SERIAL_SHIFTGENMASK(4, 3) + +enum imx6_bmode_serial { + IMX6_BMODE_ECSPI1, + IMX6_BMODE_ECSPI2, + IMX6_BMODE_ECSPI3, + IMX6_BMODE_ECSPI4, + IMX6_BMODE_ECSPI5, + IMX6_BMODE_I2C1, + IMX6_BMODE_I2C2, + IMX6_BMODE_I2C3, +}; + +enum imx6_bmode_emi { + IMX6_BMODE_ONENAND, + IMX6_BMODE_NOR, +}; + +enum imx6_bmode { + IMX6_BMODE_EMI, + IMX6_BMODE_SATA = 0x2, + IMX6_BMODE_SERIAL, + IMX6_BMODE_SD, + IMX6_BMODE_ESD, + IMX6_BMODE_MMC, + IMX6_BMODE_EMMC, + IMX6_BMODE_NAND, +}; + static inline u8 imx6_is_bmode_from_gpr9(void) { struct src *psrc = (struct src *)SRC_BASE_ADDR; -- 1.9.1 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 03/15] imx: Use IMX6_BMODE_* macros instead of numericals
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 > Cc: Tim Harvey > Signed-off-by: Jagan Teki > --- > 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 & 0x00FF) >> 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 & 0x0008) >> 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 & 0x0700) >> 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: > +#define IMX6_BMODE_SHIFTBIT(2) > +#define IMX6_BMODE_EMI_MASK BIT(3) Ditto (number, not mask): > +#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. > +#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. > +enum imx6_bmode_serial { > + IMX6_BMODE_ECSPI1, > + IMX6_BMODE_ECSPI2, > + IMX6_BMODE_ECSPI3, > + IMX6_BMODE_ECSPI4, > + IMX6_BMODE_ECSPI5, > + IMX6_BMODE_I2C1, > + IMX6_BMODE_I2C2, > + IMX6_BMODE_I2C3, > +}; > + > +enum imx6_bmode_emi { > + IMX6_BMODE_ONENAND, > + IMX6_BMODE_NOR, > +}; > + > +enum imx6_bmode { > + IMX6_BMODE_EMI, Especially when doing things like this: > + IMX6_BMODE_SATA = 0x2, > + IMX6_BMODE_SERIAL, > + IMX6_BMODE_SD, > + IMX6_BMODE_ESD, > + IMX6_BMODE_MMC, > + IMX6_BMODE_EMMC, > + IMX6_BMODE_NAND, > +}; > + > static inline u8 imx6_is_bmode_from_gpr9(void) > { > struct src *psrc = (struct src *)SRC_BASE_ADDR; > ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 03/15] imx: Use IMX6_BMODE_* macros instead of numericals
On Fri, Jan 27, 2017 at 4:18 PM, Eric Nelson 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 >> Cc: Tim Harvey >> Signed-off-by: Jagan Teki >> --- >> 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 & 0x00FF) >> 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 & 0x0008) >> 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 & 0x0700) >> 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 & 0x00FF) >> 4) { + switch ((reg & IMX6_BMODE_MASK) >> IMX6_BMODE_SHIFT) { >> +#define IMX6_BMODE_SHIFTBIT(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 & 0x0008) >> 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
Re: [U-Boot] [PATCH v2 03/15] imx: Use IMX6_BMODE_* macros instead of numericals
Hi Jagan, On 01/27/2017 10:21 AM, Jagan Teki wrote: > On Fri, Jan 27, 2017 at 4:18 PM, Eric Nelson 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/ >> >>> 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 I was referring to IMX6_BMODE_SHIFT. > - switch ((reg & 0x00FF) >> 4) { > + switch ((reg & IMX6_BMODE_MASK) >> IMX6_BMODE_SHIFT) { > >>> +#define IMX6_BMODE_SHIFTBIT(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 & 0x0008) >> 3) > + switch ((reg & IMX6_BMODE_EMI_MASK) >> IMX6_BMODE_EMI_SHIFT) { > Again, I'm referring to the _SHIFT macro below: >>> +#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. > I2C is also serial ROM or serial NOR. BMODE_SERIAL just seems to have multiple meanings. >> >>> +#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. > My comment here was referring to the use of enums for imx6_bmode, imx6_bmode_emi, and imx6_bmode_serial. If you use macros, it's easier to see that, for instance IMX6_BMODE_EMMC == 7 > thanks! > No. Thank you for the patch. This was pretty contorted previously. Regards, Eric ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 03/15] imx: Use IMX6_BMODE_* macros instead of numericals
On Fri, Jan 27, 2017 at 6:29 PM, Eric Nelson wrote: > Hi Jagan, > > On 01/27/2017 10:21 AM, Jagan Teki wrote: >> On Fri, Jan 27, 2017 at 4:18 PM, Eric Nelson 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/ >>> > > > 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 > > I was referring to IMX6_BMODE_SHIFT. Sorry, I thought you replied on in-line to my messages and I'm trying to use bitops for possible vlaue BIT(2) make the value 4 (1 << 2) > >> - switch ((reg & 0x00FF) >> 4) { >> + switch ((reg & IMX6_BMODE_MASK) >> IMX6_BMODE_SHIFT) { >> +#define IMX6_BMODE_SHIFTBIT(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 & 0x0008) >> 3) >> + switch ((reg & IMX6_BMODE_EMI_MASK) >> IMX6_BMODE_EMI_SHIFT) { >> > > Again, I'm referring to the _SHIFT macro below: Same comment as above. > +#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. >> > > I2C is also serial ROM or serial NOR. > > BMODE_SERIAL just seems to have multiple meanings. SERIAL_ROM may be better because SERIAL_NOR look spi-nor flash. > >>> +#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. >> > > My comment here was referring to the use of enums for > imx6_bmode, imx6_bmode_emi, and imx6_bmode_serial. > > If you use macros, it's easier to see that, for instance > IMX6_BMODE_EMMC == 7 May be this is true with imx6_bmode but the rest have serial in nature, but again enum make code compile time retain and good for for code readable instead of assigning values unlike macro.s > >> thanks! >> > > No. Thank you for the patch. This was pretty contorted previously. :) 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
Re: [U-Boot] [PATCH v2 03/15] imx: Use IMX6_BMODE_* macros instead of numericals
Hi Jagan, On 01/27/2017 10:54 AM, Jagan Teki wrote: > On Fri, Jan 27, 2017 at 6:29 PM, Eric Nelson wrote: >> Hi Jagan, >> >> On 01/27/2017 10:21 AM, Jagan Teki wrote: >>> On Fri, Jan 27, 2017 at 4:18 PM, Eric Nelson 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/ >> >> >> > 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 >> >> I was referring to IMX6_BMODE_SHIFT. > > Sorry, I thought you replied on in-line to my messages and I'm trying > to use bitops for possible vlaue BIT(2) make the value 4 (1 << 2) > Methinks you tries too hard! Bitops don't help when you're referring to a bit **position**, only when referring to a mask. >> >>> - switch ((reg & 0x00FF) >> 4) { >>> + switch ((reg & IMX6_BMODE_MASK) >> IMX6_BMODE_SHIFT) { >>> > +#define IMX6_BMODE_SHIFTBIT(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 & 0x0008) >> 3) >>> + switch ((reg & IMX6_BMODE_EMI_MASK) >> IMX6_BMODE_EMI_SHIFT) >>> { >>> >> >> Again, I'm referring to the _SHIFT macro below: > > Same comment as above. > >> > +#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. >>> >> >> I2C is also serial ROM or serial NOR. >> >> BMODE_SERIAL just seems to have multiple meanings. > > SERIAL_ROM may be better because SERIAL_NOR look spi-nor flash. > Okay by me. >> > +#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. >>> >> >> My comment here was referring to the use of enums for >> imx6_bmode, imx6_bmode_emi, and imx6_bmode_serial. >> >> If you use macros, it's easier to see that, for instance >> IMX6_BMODE_EMMC == 7 > > May be this is true with imx6_bmode but the rest have serial in > nature, but again enum make code compile time retain and good for for > code readable instead of assigning values unlike macro.s > If these were likely to be used more widely, I might agree, but opinions vary. My main thought is that having the numbers handy would make it easier to compare against the reference manual (which I haven't) or even the constants you just replaced (which I also haven't done). ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 03/15] imx: Use IMX6_BMODE_* macros instead of numericals
On Fri, Jan 27, 2017 at 7:10 PM, Eric Nelson wrote: > Hi Jagan, > > On 01/27/2017 10:54 AM, Jagan Teki wrote: >> On Fri, Jan 27, 2017 at 6:29 PM, Eric Nelson wrote: >>> Hi Jagan, >>> >>> On 01/27/2017 10:21 AM, Jagan Teki wrote: On Fri, Jan 27, 2017 at 4:18 PM, Eric Nelson 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/ > >>> >>> >>> >> 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 >>> >>> I was referring to IMX6_BMODE_SHIFT. >> >> Sorry, I thought you replied on in-line to my messages and I'm trying >> to use bitops for possible vlaue BIT(2) make the value 4 (1 << 2) >> > > Methinks you tries too hard! > > Bitops don't help when you're referring to a bit **position**, only > when referring to a mask. OK, will fix. > >>> - switch ((reg & 0x00FF) >> 4) { + switch ((reg & IMX6_BMODE_MASK) >> IMX6_BMODE_SHIFT) { >> +#define IMX6_BMODE_SHIFTBIT(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 & 0x0008) >> 3) + switch ((reg & IMX6_BMODE_EMI_MASK) >> IMX6_BMODE_EMI_SHIFT) { >>> >>> Again, I'm referring to the _SHIFT macro below: >> >> Same comment as above. >> >>> >> +#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. >>> >>> I2C is also serial ROM or serial NOR. >>> >>> BMODE_SERIAL just seems to have multiple meanings. >> >> SERIAL_ROM may be better because SERIAL_NOR look spi-nor flash. >> > > Okay by me. > >>> > >> +#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. >>> >>> My comment here was referring to the use of enums for >>> imx6_bmode, imx6_bmode_emi, and imx6_bmode_serial. >>> >>> If you use macros, it's easier to see that, for instance >>> IMX6_BMODE_EMMC == 7 >> >> May be this is true with imx6_bmode but the rest have serial in >> nature, but again enum make code compile time retain and good for for >> code readable instead of assigning values unlike macro.s >> > > If these were likely to be used more widely, I might agree, but > opinions vary. > > My main thought is that having the numbers handy would make > it easier to compare against the reference manual (which I haven't) > or even the constants you just replaced (which I also haven't done). Ok, then I will assign the values directly for imx6_bmode. 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