From: Eric Nelson <eric@nelint.com>
To: u-boot@lists.denx.de
Subject: [U-Boot] [PATCH v2 03/15] imx: Use IMX6_BMODE_* macros instead of numericals
Date: Fri, 27 Jan 2017 10:29:08 -0700 [thread overview]
Message-ID: <0dafd744-cdad-bfc6-b347-6879efaac740@nelint.com> (raw)
In-Reply-To: <CAD6G_RS5cufvqQs3siP7tJgW6Y6Jb8oQGEPC8Wond9j+OaAPLw@mail.gmail.com>
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
next prev parent reply other threads:[~2017-01-27 17:29 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-01-27 14:12 [U-Boot] [PATC v2 00/15] imx6: Engicam i.CoreM6/Is.IoT eMMC boot support Jagan Teki
2017-01-27 14:12 ` [U-Boot] [PATCH v2 01/15] imx6: Add imx6_src_get_boot_mode Jagan Teki
2017-01-27 14:12 ` [U-Boot] [PATCH v2 02/15] imx: spl: Update NAND bootmode detection bit Jagan Teki
2017-01-27 14:12 ` [U-Boot] [PATCH v2 03/15] imx: Use IMX6_BMODE_* macros instead of numericals Jagan Teki
2017-01-27 15:18 ` Eric Nelson
2017-01-27 17:21 ` Jagan Teki
2017-01-27 17:29 ` Eric Nelson [this message]
2017-01-27 17:54 ` Jagan Teki
2017-01-27 18:10 ` Eric Nelson
2017-01-27 18:26 ` Jagan Teki
2017-01-27 14:12 ` [U-Boot] [PATCH v2 04/15] imx6: Add src_base structure define macro Jagan Teki
2017-01-27 14:12 ` [U-Boot] [PATCH v2 05/15] imx6: isiotmx6ul: Update SPL board boot order for eMMC Jagan Teki
2017-01-27 14:12 ` [U-Boot] [PATCH v2 06/15] i.MX6UL: isiot: Add eMMC boot support Jagan Teki
2017-01-27 14:12 ` [U-Boot] [PATCH v2 07/15] i.MX6UL: isiot: Add modeboot env via board_late_init Jagan Teki
2017-01-27 14:12 ` [U-Boot] [PATCH v2 08/15] i.MX6UL: isiot: Add mmc_late_init Jagan Teki
2017-01-27 14:12 ` [U-Boot] [PATCH v2 09/15] i.MX6UL: isiot: Switch the mmc env based on devno Jagan Teki
2017-01-27 14:12 ` [U-Boot] [PATCH v2 10/15] arm: dts: imx6qdl-icore-rqs: Add eMMC node Jagan Teki
2017-01-27 14:12 ` [U-Boot] [PATCH v2 11/15] imx6: icorem6_rqs: Update SPL board boot order for eMMC Jagan Teki
2017-01-27 14:12 ` [U-Boot] [PATCH v2 12/15] imx6: icorem6_rqs: Add eMMC boot support Jagan Teki
2017-01-27 14:12 ` [U-Boot] [PATCH v2 13/15] i.MX6Q: icorem6_rqs: Add modeboot env via board_late_init Jagan Teki
2017-01-27 14:12 ` [U-Boot] [PATCH v2 14/15] i.MX6Q: icorem6_rqs: Add mmc_late_init Jagan Teki
2017-01-27 14:12 ` [U-Boot] [PATCH v2 15/15] i.MX6Q: isiot: Switch the mmc env based on devno Jagan Teki
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=0dafd744-cdad-bfc6-b347-6879efaac740@nelint.com \
--to=eric@nelint.com \
--cc=u-boot@lists.denx.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox