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

[Jagan Teki]

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)

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