[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 <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/
>
>>
>> Cc: Stefano Babic <sbabic at denx.de>
>> Cc: Tim Harvey <tharvey at gateworks.com>
>> Signed-off-by: Jagan Teki <jagan at openedev.com>
>> ---
>>  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 & 0x000000FF) >> 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 & 0x00000008) >> 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 & 0x07000000) >> 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 & 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) {

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