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

[Eric Nelson]

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.

>>
>>>  -     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).