[U-Boot] [PATCH v2 03/15] imx: Use IMX6_BMODE_* macros instead of numericals

[Jagan Teki]

On Fri, Jan 27, 2017 at 7:10 PM, Eric Nelson <eric at 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 <eric at 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 <eric at 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.