From mboxrd@z Thu Jan  1 00:00:00 1970
From: Eric Nelson <eric@nelint.com>
Date: Fri, 27 Jan 2017 10:29:08 -0700
Subject: [U-Boot] [PATCH v2 03/15] imx: Use IMX6_BMODE_* macros instead
 of numericals
In-Reply-To: <CAD6G_RS5cufvqQs3siP7tJgW6Y6Jb8oQGEPC8Wond9j+OaAPLw@mail.gmail.com>
References: <1485526363-3834-1-git-send-email-jagan@openedev.com>
 <1485526363-3834-4-git-send-email-jagan@openedev.com>
 <453208c2-8231-38a0-f018-64bf9622a5de@nelint.com>
 <CAD6G_RS5cufvqQs3siP7tJgW6Y6Jb8oQGEPC8Wond9j+OaAPLw@mail.gmail.com>
Message-ID: <0dafd744-cdad-bfc6-b347-6879efaac740@nelint.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Hi Jagan,

On 01/27/2017 10:21 AM, Jagan Teki wrote:
> On Fri, Jan 27, 2017 at 4:18 PM, Eric Nelson <eric@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.

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

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

>>
>>> +#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

> thanks!
> 

No. Thank you for the patch. This was pretty contorted previously.

Regards,


Eric