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