On Fri, Jan 27, 2017 at 6:29 PM, Eric Nelson <e...@nelint.com> wrote: > Hi Jagan, > > On 01/27/2017 10:21 AM, Jagan Teki wrote: >> 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/ >>> > > <snip> > >>>> 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 & 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) { >> > > 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