Hi Jagan,
On 01/27/2017 10:54 AM, Jagan Teki wrote:
On Fri, Jan 27, 2017 at 6:29 PM, Eric Nelson eric@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@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)
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 & 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.
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).