Re: [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  wrote:
> Hi Jagan,
>
> On 01/27/2017 10:54 AM, Jagan Teki wrote:
>> On Fri, Jan 27, 2017 at 6:29 PM, Eric Nelson  wrote:
>>> Hi Jagan,
>>>
>>> On 01/27/2017 10:21 AM, Jagan Teki wrote:
 On Fri, Jan 27, 2017 at 4:18 PM, Eric Nelson  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/
>
>>>
>>> 
>>>
>> 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 & 0x00FF) >> 4) {
  + switch ((reg & IMX6_BMODE_MASK) >> IMX6_BMODE_SHIFT) {

>> +#define  IMX6_BMODE_SHIFTBIT(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 & 0x0008) >> 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

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

>>
>> 
>>
> 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 & 0x00FF) >> 4) {
>>>  + switch ((reg & IMX6_BMODE_MASK) >> IMX6_BMODE_SHIFT) {
>>>
> +#define  IMX6_BMODE_SHIFTBIT(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 & 0x0008) >> 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).

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [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  wrote:
> Hi Jagan,
>
> On 01/27/2017 10:21 AM, Jagan Teki wrote:
>> On Fri, Jan 27, 2017 at 4:18 PM, Eric Nelson  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/
>>>
>
> 
>
 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 & 0x00FF) >> 4) {
>>  + switch ((reg & IMX6_BMODE_MASK) >> IMX6_BMODE_SHIFT) {
>>
 +#define  IMX6_BMODE_SHIFTBIT(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 & 0x0008) >> 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.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

[Eric Nelson]

Hi Jagan,

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



>>> 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 & 0x00FF) >> 4) {
>  + switch ((reg & IMX6_BMODE_MASK) >> IMX6_BMODE_SHIFT) {
> 
>>> +#define  IMX6_BMODE_SHIFTBIT(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 & 0x0008) >> 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
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

[Jagan Teki]

On Fri, Jan 27, 2017 at 4:18 PM, Eric Nelson  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/
>
>>
>> Cc: Stefano Babic 
>> Cc: Tim Harvey 
>> Signed-off-by: Jagan Teki 
>> ---
>>  arch/arm/imx-common/spl.c   | 39 
>> ++---
>>  arch/arm/include/asm/imx-common/sys_proto.h | 34 +
>>  2 files changed, 58 insertions(+), 15 deletions(-)
>>
>> diff --git a/arch/arm/imx-common/spl.c b/arch/arm/imx-common/spl.c
>> index fc3704b..38789b2 100644
>> --- a/arch/arm/imx-common/spl.c
>> +++ b/arch/arm/imx-common/spl.c
>> @@ -30,39 +30,48 @@ u32 spl_boot_device(void)
>>   if bmode >> 24) & 0x03)  == 0x01) || /* Serial Downloader */
>>   (imx6_is_bmode_from_gpr9() && (reg == 1)))
>>   return BOOT_DEVICE_UART;
>> +
>>   /* BOOT_CFG1[7:4] - see IMX6DQRM Table 8-8 */
>> - switch ((reg & 0x00FF) >> 4) {
>> + switch ((reg & IMX6_BMODE_MASK) >> IMX6_BMODE_SHIFT) {
>>/* EIM: See 8.5.1, Table 8-9 */
>> - case 0x0:
>> + case IMX6_BMODE_EMI:
>>   /* BOOT_CFG1[3]: NOR/OneNAND Selection */
>> - if ((reg & 0x0008) >> 3)
>> + switch ((reg & IMX6_BMODE_EMI_MASK) >> IMX6_BMODE_EMI_SHIFT) {
>> + case IMX6_BMODE_ONENAND:
>>   return BOOT_DEVICE_ONENAND;
>> - else
>> + case IMX6_BMODE_NOR:
>>   return BOOT_DEVICE_NOR;
>> - break;
>> + }
>>   /* SATA: See 8.5.4, Table 8-20 */
>> - case 0x2:
>> + case IMX6_BMODE_SATA:
>>   return BOOT_DEVICE_SATA;
>>   /* Serial ROM: See 8.5.5.1, Table 8-22 */
>> - case 0x3:
>> + case IMX6_BMODE_SERIAL:
>>   /* BOOT_CFG4[2:0] */
>> - switch ((reg & 0x0700) >> 24) {
>> - case 0x0 ... 0x4:
>> + switch ((reg & IMX6_BMODE_SERIAL_MASK) >>
>> + IMX6_BMODE_SERIAL_SHIFT) {
>> + case IMX6_BMODE_ECSPI1:
>> + case IMX6_BMODE_ECSPI2:
>> + case IMX6_BMODE_ECSPI3:
>> + case IMX6_BMODE_ECSPI4:
>> + case IMX6_BMODE_ECSPI5:
>>   return BOOT_DEVICE_SPI;
>> - case 0x5 ... 0x7:
>> + case IMX6_BMODE_I2C1:
>> + case IMX6_BMODE_I2C2:
>> + case IMX6_BMODE_I2C3:
>>   return BOOT_DEVICE_I2C;
>>   }
>>   break;
>>   /* SD/eSD: 8.5.3, Table 8-15  */
>> - case 0x4:
>> - case 0x5:
>> + case IMX6_BMODE_SD:
>> + case IMX6_BMODE_ESD:
>>   return BOOT_DEVICE_MMC1;
>>   /* MMC/eMMC: 8.5.3 */
>> - case 0x6:
>> - case 0x7:
>> + case IMX6_BMODE_MMC:
>> + case IMX6_BMODE_EMMC:
>>   return BOOT_DEVICE_MMC1;
>>   /* NAND Flash: 8.5.2, Table 8-10 */
>> - case 0x8:
>> + case IMX6_BMODE_NAND:
>>   return BOOT_DEVICE_NAND;
>>   }
>>   return BOOT_DEVICE_NONE;
>> 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
 - switch ((reg & 0x00FF) >> 4) {
 + switch ((reg & IMX6_BMODE_MASK) >> IMX6_BMODE_SHIFT) {

>> +#define  IMX6_BMODE_SHIFTBIT(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 & 0x0008) >> 3)
 + switch ((reg & IMX6_BMODE_EMI_MASK) >> IMX6_BMODE_EMI_SHIFT) {

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

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

thanks!
-- 
Jagan Teki
Free Software Engineer | www.openedev.com
U-Boot, Linux | Upstream Maintainer
Hyderabad, India.
___
U-Boot mailing list

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

[Eric Nelson]

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/

> 
> Cc: Stefano Babic 
> Cc: Tim Harvey 
> Signed-off-by: Jagan Teki 
> ---
>  arch/arm/imx-common/spl.c   | 39 
> ++---
>  arch/arm/include/asm/imx-common/sys_proto.h | 34 +
>  2 files changed, 58 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/arm/imx-common/spl.c b/arch/arm/imx-common/spl.c
> index fc3704b..38789b2 100644
> --- a/arch/arm/imx-common/spl.c
> +++ b/arch/arm/imx-common/spl.c
> @@ -30,39 +30,48 @@ u32 spl_boot_device(void)
>   if bmode >> 24) & 0x03)  == 0x01) || /* Serial Downloader */
>   (imx6_is_bmode_from_gpr9() && (reg == 1)))
>   return BOOT_DEVICE_UART;
> +
>   /* BOOT_CFG1[7:4] - see IMX6DQRM Table 8-8 */
> - switch ((reg & 0x00FF) >> 4) {
> + switch ((reg & IMX6_BMODE_MASK) >> IMX6_BMODE_SHIFT) {
>/* EIM: See 8.5.1, Table 8-9 */
> - case 0x0:
> + case IMX6_BMODE_EMI:
>   /* BOOT_CFG1[3]: NOR/OneNAND Selection */
> - if ((reg & 0x0008) >> 3)
> + switch ((reg & IMX6_BMODE_EMI_MASK) >> IMX6_BMODE_EMI_SHIFT) {
> + case IMX6_BMODE_ONENAND:
>   return BOOT_DEVICE_ONENAND;
> - else
> + case IMX6_BMODE_NOR:
>   return BOOT_DEVICE_NOR;
> - break;
> + }
>   /* SATA: See 8.5.4, Table 8-20 */
> - case 0x2:
> + case IMX6_BMODE_SATA:
>   return BOOT_DEVICE_SATA;
>   /* Serial ROM: See 8.5.5.1, Table 8-22 */
> - case 0x3:
> + case IMX6_BMODE_SERIAL:
>   /* BOOT_CFG4[2:0] */
> - switch ((reg & 0x0700) >> 24) {
> - case 0x0 ... 0x4:
> + switch ((reg & IMX6_BMODE_SERIAL_MASK) >>
> + IMX6_BMODE_SERIAL_SHIFT) {
> + case IMX6_BMODE_ECSPI1:
> + case IMX6_BMODE_ECSPI2:
> + case IMX6_BMODE_ECSPI3:
> + case IMX6_BMODE_ECSPI4:
> + case IMX6_BMODE_ECSPI5:
>   return BOOT_DEVICE_SPI;
> - case 0x5 ... 0x7:
> + case IMX6_BMODE_I2C1:
> + case IMX6_BMODE_I2C2:
> + case IMX6_BMODE_I2C3:
>   return BOOT_DEVICE_I2C;
>   }
>   break;
>   /* SD/eSD: 8.5.3, Table 8-15  */
> - case 0x4:
> - case 0x5:
> + case IMX6_BMODE_SD:
> + case IMX6_BMODE_ESD:
>   return BOOT_DEVICE_MMC1;
>   /* MMC/eMMC: 8.5.3 */
> - case 0x6:
> - case 0x7:
> + case IMX6_BMODE_MMC:
> + case IMX6_BMODE_EMMC:
>   return BOOT_DEVICE_MMC1;
>   /* NAND Flash: 8.5.2, Table 8-10 */
> - case 0x8:
> + case IMX6_BMODE_NAND:
>   return BOOT_DEVICE_NAND;
>   }
>   return BOOT_DEVICE_NONE;
> 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:
> +#define  IMX6_BMODE_SHIFTBIT(2)
> +#define IMX6_BMODE_EMI_MASK  BIT(3)

Ditto (number, not mask):
> +#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.

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

> +enum imx6_bmode_serial {
> + IMX6_BMODE_ECSPI1,
> + IMX6_BMODE_ECSPI2,
> + IMX6_BMODE_ECSPI3,
> + IMX6_BMODE_ECSPI4,
> + IMX6_BMODE_ECSPI5,
> + IMX6_BMODE_I2C1,
> + IMX6_BMODE_I2C2,
> + IMX6_BMODE_I2C3,
> +};
> +
> +enum imx6_bmode_emi {
> + IMX6_BMODE_ONENAND,
> + IMX6_BMODE_NOR,
> +};
> +
> +enum imx6_bmode {
> + IMX6_BMODE_EMI,

Especially when doing things like this:

> + IMX6_BMODE_SATA = 0x2,
> + IMX6_BMODE_SERIAL,
> + IMX6_BMODE_SD,
> + IMX6_BMODE_ESD,
> + IMX6_BMODE_MMC,
> + IMX6_BMODE_EMMC,
> + IMX6_BMODE_NAND,
> +};
> +
>  static inline u8 imx6_is_bmode_from_gpr9(void)
>  {
>   struct src *psrc = (struct src *)SRC_BASE_ADDR;
> 

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

[Jagan Teki]

Use meaningful meacros IMX6_BMODE_*, instead of numerical
number in boot mode detection code.

Cc: Stefano Babic 
Cc: Tim Harvey 
Signed-off-by: Jagan Teki 
---
 arch/arm/imx-common/spl.c   | 39 ++---
 arch/arm/include/asm/imx-common/sys_proto.h | 34 +
 2 files changed, 58 insertions(+), 15 deletions(-)

diff --git a/arch/arm/imx-common/spl.c b/arch/arm/imx-common/spl.c
index fc3704b..38789b2 100644
--- a/arch/arm/imx-common/spl.c
+++ b/arch/arm/imx-common/spl.c
@@ -30,39 +30,48 @@ u32 spl_boot_device(void)
if bmode >> 24) & 0x03)  == 0x01) || /* Serial Downloader */
(imx6_is_bmode_from_gpr9() && (reg == 1)))
return BOOT_DEVICE_UART;
+
/* BOOT_CFG1[7:4] - see IMX6DQRM Table 8-8 */
-   switch ((reg & 0x00FF) >> 4) {
+   switch ((reg & IMX6_BMODE_MASK) >> IMX6_BMODE_SHIFT) {
 /* EIM: See 8.5.1, Table 8-9 */
-   case 0x0:
+   case IMX6_BMODE_EMI:
/* BOOT_CFG1[3]: NOR/OneNAND Selection */
-   if ((reg & 0x0008) >> 3)
+   switch ((reg & IMX6_BMODE_EMI_MASK) >> IMX6_BMODE_EMI_SHIFT) {
+   case IMX6_BMODE_ONENAND:
return BOOT_DEVICE_ONENAND;
-   else
+   case IMX6_BMODE_NOR:
return BOOT_DEVICE_NOR;
-   break;
+   }
/* SATA: See 8.5.4, Table 8-20 */
-   case 0x2:
+   case IMX6_BMODE_SATA:
return BOOT_DEVICE_SATA;
/* Serial ROM: See 8.5.5.1, Table 8-22 */
-   case 0x3:
+   case IMX6_BMODE_SERIAL:
/* BOOT_CFG4[2:0] */
-   switch ((reg & 0x0700) >> 24) {
-   case 0x0 ... 0x4:
+   switch ((reg & IMX6_BMODE_SERIAL_MASK) >>
+   IMX6_BMODE_SERIAL_SHIFT) {
+   case IMX6_BMODE_ECSPI1:
+   case IMX6_BMODE_ECSPI2:
+   case IMX6_BMODE_ECSPI3:
+   case IMX6_BMODE_ECSPI4:
+   case IMX6_BMODE_ECSPI5:
return BOOT_DEVICE_SPI;
-   case 0x5 ... 0x7:
+   case IMX6_BMODE_I2C1:
+   case IMX6_BMODE_I2C2:
+   case IMX6_BMODE_I2C3:
return BOOT_DEVICE_I2C;
}
break;
/* SD/eSD: 8.5.3, Table 8-15  */
-   case 0x4:
-   case 0x5:
+   case IMX6_BMODE_SD:
+   case IMX6_BMODE_ESD:
return BOOT_DEVICE_MMC1;
/* MMC/eMMC: 8.5.3 */
-   case 0x6:
-   case 0x7:
+   case IMX6_BMODE_MMC:
+   case IMX6_BMODE_EMMC:
return BOOT_DEVICE_MMC1;
/* NAND Flash: 8.5.2, Table 8-10 */
-   case 0x8:
+   case IMX6_BMODE_NAND:
return BOOT_DEVICE_NAND;
}
return BOOT_DEVICE_NONE;
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_MASKGENMASK(7, 0)
+#defineIMX6_BMODE_SHIFTBIT(2)
+#define IMX6_BMODE_EMI_MASKBIT(3)
+#define IMX6_BMODE_EMI_SHIFT   GENMASK(1, 0)
+#define IMX6_BMODE_SERIAL_MASK GENMASK(26, 24)
+#define IMX6_BMODE_SERIAL_SHIFTGENMASK(4, 3)
+
+enum imx6_bmode_serial {
+   IMX6_BMODE_ECSPI1,
+   IMX6_BMODE_ECSPI2,
+   IMX6_BMODE_ECSPI3,
+   IMX6_BMODE_ECSPI4,
+   IMX6_BMODE_ECSPI5,
+   IMX6_BMODE_I2C1,
+   IMX6_BMODE_I2C2,
+   IMX6_BMODE_I2C3,
+};
+
+enum imx6_bmode_emi {
+   IMX6_BMODE_ONENAND,
+   IMX6_BMODE_NOR,
+};
+
+enum imx6_bmode {
+   IMX6_BMODE_EMI,
+   IMX6_BMODE_SATA = 0x2,
+   IMX6_BMODE_SERIAL,
+   IMX6_BMODE_SD,
+   IMX6_BMODE_ESD,
+   IMX6_BMODE_MMC,
+   IMX6_BMODE_EMMC,
+   IMX6_BMODE_NAND,
+};
+
 static inline u8 imx6_is_bmode_from_gpr9(void)
 {
struct src *psrc = (struct src *)SRC_BASE_ADDR;
-- 
1.9.1

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot