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