[PATCH v2 00/19] i.MX bootsource bugfixes, refactoring and VFxxx support

[PATCH v2 00/19] i.MX bootsource bugfixes, refactoring and VFxxx support

[Andrey Smirnov]

Everyone:

This patchset is the result of my work on adding support for
bootsource detection of VFxxx as well as fixing a number of
bugs/unsupported corner cases in similar i.MX7 specific code.

NOTE: While VFxxx support is added in this series the code integrating
it into the SoC initalization sequnce is not included. Patch for that
is dependent on recently submitted "i.MX reset reason detection
support" and I didn't want to intertwine two otherwise independent
submissions.

Changes since [v1]:

	- Patchset converted to use FIELD_GET macro (which was ported
          from the kernel)

	- Spelling fixes

[v1] https://www.spinics.net/lists/u-boot-v2/msg32506.html

Feedback is wellcome!

Thanks,
Andrey Smirnov

Andrey Smirnov (19):
  ARM: i.MX: boot: Coalesce copy-pasted code
  include: Port linux/build_bug.h from Linux kernel
  include: Port linux/bitfield.h from Linux kernel
  ARM: i.MX: Add a function to extract BMOD value
  ARM: i.MX: Simplify serial bootsource detection for i.MX6 and 7
  ARM: i.MX: Account for unprogrammed fuses on i.MX6 and i.MX7
  ARM: i.MX7: boot: Add code to handle SD/MMC manufacture mode
  ARM: i.MX7: boot: Remove incorrect NAND bootsource detection
  ARM: i.MX7: boot: Fix SPI-NOR/QSPI boot source mixup
  ARM: i.MX: boot: Remove unnecessary returns
  ARM: i.MX: boot: Move magic values into small functions
  ARM: i.MX: boot: Share code to detect NAND as a boot source
  ARM: i.MX: boot: Check for NAND boot only if necessary on i.MX53, 6
  ARM: i.MX53: boot: Move magic numbers info utility functions
  ARM: i.MX6: boot: Move magic numbers into utility functions
  ARM: i.MX7: boot: Move magic numbers into utility functions
  bootsource: Add BOOTSOURCE_CAN
  ARM: VFxxx: Implement code to detect bootsource
  ARM: i.MX6: boot: Return BOOTSOURCE_SPI_NOR, not BOOTSOURCE_SPI

 arch/arm/boards/datamodul-edm-qmx6/board.c |   2 +-
 arch/arm/boards/dfi-fs700-m60/board.c      |   2 +-
 arch/arm/boards/phytec-som-imx6/board.c    |   2 +-
 arch/arm/boards/zii-imx6q-rdu2/lowlevel.c  |   2 +-
 arch/arm/mach-imx/boot.c                   | 436 ++++++++++++++++++++---------
 arch/arm/mach-imx/include/mach/generic.h   |   2 +
 arch/arm/mach-imx/xload.c                  |   2 +-
 common/bootsource.c                        |   1 +
 include/bootsource.h                       |   1 +
 include/linux/bitfield.h                   | 152 ++++++++++
 include/linux/bug.h                        |  25 +-
 include/linux/build_bug.h                  |  83 ++++++
 12 files changed, 550 insertions(+), 160 deletions(-)
 create mode 100644 include/linux/bitfield.h
 create mode 100644 include/linux/build_bug.h

-- 
2.14.3


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 01/19] ARM: i.MX: boot: Coalesce copy-pasted code

[Andrey Smirnov]

All of the instances of imx*_boot_save_loc() do exactly the same thing with
the only difference being SoC-specific imx*_get_boot_source
call. Convert the code into a generic function taking function pointer
+ a macro to take care of the boilerplate.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 arch/arm/mach-imx/boot.c | 85 ++++++++++--------------------------------------
 1 file changed, 17 insertions(+), 68 deletions(-)

diff --git a/arch/arm/mach-imx/boot.c b/arch/arm/mach-imx/boot.c
index 72597f5e2..4657fa017 100644
--- a/arch/arm/mach-imx/boot.c
+++ b/arch/arm/mach-imx/boot.c
@@ -89,17 +89,6 @@ void imx25_get_boot_source(enum bootsource *src, int *instance)
 				    (val >> MX25_CCM_RCSR_MEM_TYPE_SHIFT) & 0x3);
 }
 
-void imx25_boot_save_loc(void)
-{
-	enum bootsource src = BOOTSOURCE_UNKNOWN;
-	int instance = BOOTSOURCE_INSTANCE_UNKNOWN;
-
-	imx25_get_boot_source(&src, &instance);
-
-	bootsource_set(src);
-	bootsource_set_instance(instance);
-}
-
 void imx35_get_boot_source(enum bootsource *src, int *instance)
 {
 	void __iomem *ccm_base = IOMEM(MX35_CCM_BASE_ADDR);
@@ -110,17 +99,6 @@ void imx35_get_boot_source(enum bootsource *src, int *instance)
 				    (val >> MX35_CCM_RCSR_MEM_TYPE_SHIFT) & 0x3);
 }
 
-void imx35_boot_save_loc(void)
-{
-	enum bootsource src = BOOTSOURCE_UNKNOWN;
-	int instance = BOOTSOURCE_INSTANCE_UNKNOWN;
-
-	imx35_get_boot_source(&src, &instance);
-
-	bootsource_set(src);
-	bootsource_set_instance(instance);
-}
-
 #define IMX27_SYSCTRL_GPCR	0x18
 #define IMX27_GPCR_BOOT_SHIFT			16
 #define IMX27_GPCR_BOOT_MASK			(0xf << IMX27_GPCR_BOOT_SHIFT)
@@ -157,17 +135,6 @@ void imx27_get_boot_source(enum bootsource *src, int *instance)
 	}
 }
 
-void imx27_boot_save_loc(void)
-{
-	enum bootsource src = BOOTSOURCE_UNKNOWN;
-	int instance = BOOTSOURCE_INSTANCE_UNKNOWN;
-
-	imx27_get_boot_source(&src, &instance);
-
-	bootsource_set(src);
-	bootsource_set_instance(instance);
-}
-
 #define IMX51_SRC_SBMR			0x4
 #define IMX51_SBMR_BT_MEM_TYPE_SHIFT	7
 #define IMX51_SBMR_BT_MEM_CTL_SHIFT	0
@@ -201,17 +168,6 @@ void imx51_get_boot_source(enum bootsource *src, int *instance)
 	}
 }
 
-void imx51_boot_save_loc(void)
-{
-	enum bootsource src = BOOTSOURCE_UNKNOWN;
-	int instance = BOOTSOURCE_INSTANCE_UNKNOWN;
-
-	imx51_get_boot_source(&src, &instance);
-
-	bootsource_set(src);
-	bootsource_set_instance(instance);
-}
-
 #define IMX53_SRC_SBMR	0x4
 void imx53_get_boot_source(enum bootsource *src, int *instance)
 {
@@ -260,17 +216,6 @@ void imx53_get_boot_source(enum bootsource *src, int *instance)
 	}
 }
 
-void imx53_boot_save_loc(void)
-{
-	enum bootsource src = BOOTSOURCE_UNKNOWN;
-	int instance = BOOTSOURCE_INSTANCE_UNKNOWN;
-
-	imx53_get_boot_source(&src, &instance);
-
-	bootsource_set(src);
-	bootsource_set_instance(instance);
-}
-
 #define IMX6_SRC_SBMR1	0x04
 #define IMX6_SRC_SBMR2	0x1c
 
@@ -337,17 +282,6 @@ internal_boot:
 	return;
 }
 
-void imx6_boot_save_loc(void)
-{
-	enum bootsource src = BOOTSOURCE_UNKNOWN;
-	int instance = BOOTSOURCE_INSTANCE_UNKNOWN;
-
-	imx6_get_boot_source(&src, &instance);
-
-	bootsource_set(src);
-	bootsource_set_instance(instance);
-}
-
 #define IMX7_SRC_SBMR1	0x58
 #define IMX7_SRC_SBMR2	0x70
 
@@ -406,13 +340,28 @@ internal_boot:
 	return;
 }
 
-void imx7_boot_save_loc(void)
+static void
+imx_boot_save_loc(void (*get_boot_source)(enum bootsource *, int *))
 {
 	enum bootsource src = BOOTSOURCE_UNKNOWN;
 	int instance = BOOTSOURCE_INSTANCE_UNKNOWN;
 
-	imx7_get_boot_source(&src, &instance);
+	get_boot_source(&src, &instance);
 
 	bootsource_set(src);
 	bootsource_set_instance(instance);
 }
+
+#define IMX_BOOT_SAVE_LOC(soc)					\
+	void soc##_boot_save_loc(void)				\
+	{							\
+		imx_boot_save_loc(soc##_get_boot_source);	\
+	}
+
+IMX_BOOT_SAVE_LOC(imx25)
+IMX_BOOT_SAVE_LOC(imx27)
+IMX_BOOT_SAVE_LOC(imx35)
+IMX_BOOT_SAVE_LOC(imx51)
+IMX_BOOT_SAVE_LOC(imx53)
+IMX_BOOT_SAVE_LOC(imx6)
+IMX_BOOT_SAVE_LOC(imx7)
-- 
2.14.3


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 02/19] include: Port linux/build_bug.h from Linux kernel

[Andrey Smirnov]

Port linux/build_bug.h needed by some of more recent kernel code.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 include/linux/bug.h       | 25 +-------------
 include/linux/build_bug.h | 83 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 84 insertions(+), 24 deletions(-)
 create mode 100644 include/linux/build_bug.h

diff --git a/include/linux/bug.h b/include/linux/bug.h
index 7295618c9..8367a11ec 100644
--- a/include/linux/bug.h
+++ b/include/linux/bug.h
@@ -2,29 +2,6 @@
 #define _LINUX_BUG_H
 
 #include <asm-generic/bug.h>
-
-#ifdef __CHECKER__
-#define BUILD_BUG_ON_NOT_POWER_OF_2(n) (0)
-#define BUILD_BUG_ON_ZERO(e) (0)
-#define BUILD_BUG_ON_NULL(e) ((void*)0)
-#define BUILD_BUG_ON(condition) (0)
-#else /* __CHECKER__ */
-
-/* Force a compilation error if a constant expression is not a power of 2 */
-#define BUILD_BUG_ON_NOT_POWER_OF_2(n)			\
-	BUILD_BUG_ON((n) == 0 || (((n) & ((n) - 1)) != 0))
-
-/* Force a compilation error if condition is true, but also produce a
-   result (of value 0 and type size_t), so the expression can be used
-   e.g. in a structure initializer (or where-ever else comma expressions
-   aren't permitted). */
-#define BUILD_BUG_ON_ZERO(e) (sizeof(struct { int:-!!(e); }))
-#define BUILD_BUG_ON_NULL(e) ((void *)sizeof(struct { int:-!!(e); }))
-
-/* Force a compilation error if condition is true */
-#define BUILD_BUG_ON(condition) ((void)BUILD_BUG_ON_ZERO(condition))
-
-#endif
-
+#include <linux/build_bug.h>
 
 #endif	/* _LINUX_BUG_H */
diff --git a/include/linux/build_bug.h b/include/linux/build_bug.h
new file mode 100644
index 000000000..43d1fd50d
--- /dev/null
+++ b/include/linux/build_bug.h
@@ -0,0 +1,83 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_BUILD_BUG_H
+#define _LINUX_BUILD_BUG_H
+
+#include <linux/compiler.h>
+
+#ifdef __CHECKER__
+#define __BUILD_BUG_ON_NOT_POWER_OF_2(n) (0)
+#define BUILD_BUG_ON_NOT_POWER_OF_2(n) (0)
+#define BUILD_BUG_ON_ZERO(e) (0)
+#define BUILD_BUG_ON_INVALID(e) (0)
+#define BUILD_BUG_ON_MSG(cond, msg) (0)
+#define BUILD_BUG_ON(condition) (0)
+#define BUILD_BUG() (0)
+#else /* __CHECKER__ */
+
+/* Force a compilation error if a constant expression is not a power of 2 */
+#define __BUILD_BUG_ON_NOT_POWER_OF_2(n)	\
+	BUILD_BUG_ON(((n) & ((n) - 1)) != 0)
+#define BUILD_BUG_ON_NOT_POWER_OF_2(n)			\
+	BUILD_BUG_ON((n) == 0 || (((n) & ((n) - 1)) != 0))
+
+/*
+ * Force a compilation error if condition is true, but also produce a
+ * result (of value 0 and type size_t), so the expression can be used
+ * e.g. in a structure initializer (or where-ever else comma expressions
+ * aren't permitted).
+ */
+#define BUILD_BUG_ON_ZERO(e) (sizeof(struct { int:(-!!(e)); }))
+
+/*
+ * BUILD_BUG_ON_INVALID() permits the compiler to check the validity of the
+ * expression but avoids the generation of any code, even if that expression
+ * has side-effects.
+ */
+#define BUILD_BUG_ON_INVALID(e) ((void)(sizeof((__force long)(e))))
+
+/**
+ * BUILD_BUG_ON_MSG - break compile if a condition is true & emit supplied
+ *		      error message.
+ * @condition: the condition which the compiler should know is false.
+ *
+ * See BUILD_BUG_ON for description.
+ */
+#define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
+
+/**
+ * BUILD_BUG_ON - break compile if a condition is true.
+ * @condition: the condition which the compiler should know is false.
+ *
+ * If you have some code which relies on certain constants being equal, or
+ * some other compile-time-evaluated condition, you should use BUILD_BUG_ON to
+ * detect if someone changes it.
+ *
+ * The implementation uses gcc's reluctance to create a negative array, but gcc
+ * (as of 4.4) only emits that error for obvious cases (e.g. not arguments to
+ * inline functions).  Luckily, in 4.3 they added the "error" function
+ * attribute just for this type of case.  Thus, we use a negative sized array
+ * (should always create an error on gcc versions older than 4.4) and then call
+ * an undefined function with the error attribute (should always create an
+ * error on gcc 4.3 and later).  If for some reason, neither creates a
+ * compile-time error, we'll still have a link-time error, which is harder to
+ * track down.
+ */
+#ifndef __OPTIMIZE__
+#define BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
+#else
+#define BUILD_BUG_ON(condition) \
+	BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition)
+#endif
+
+/**
+ * BUILD_BUG - break compile if used.
+ *
+ * If you have some code that you expect the compiler to eliminate at
+ * build time, you should use BUILD_BUG to detect if it is
+ * unexpectedly used.
+ */
+#define BUILD_BUG() BUILD_BUG_ON_MSG(1, "BUILD_BUG failed")
+
+#endif	/* __CHECKER__ */
+
+#endif	/* _LINUX_BUILD_BUG_H */
-- 
2.14.3


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 03/19] include: Port linux/bitfield.h from Linux kernel

[Andrey Smirnov]

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 include/linux/bitfield.h | 152 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 152 insertions(+)
 create mode 100644 include/linux/bitfield.h

diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
new file mode 100644
index 000000000..cf2588d81
--- /dev/null
+++ b/include/linux/bitfield.h
@@ -0,0 +1,152 @@
+/*
+ * Copyright (C) 2014 Felix Fietkau <nbd@nbd.name>
+ * Copyright (C) 2004 - 2009 Ivo van Doorn <IvDoorn@gmail.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2
+ * as published by the Free Software Foundation
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef _LINUX_BITFIELD_H
+#define _LINUX_BITFIELD_H
+
+#include <linux/build_bug.h>
+#include <asm/byteorder.h>
+
+/*
+ * Bitfield access macros
+ *
+ * FIELD_{GET,PREP} macros take as first parameter shifted mask
+ * from which they extract the base mask and shift amount.
+ * Mask must be a compilation time constant.
+ *
+ * Example:
+ *
+ *  #define REG_FIELD_A  GENMASK(6, 0)
+ *  #define REG_FIELD_B  BIT(7)
+ *  #define REG_FIELD_C  GENMASK(15, 8)
+ *  #define REG_FIELD_D  GENMASK(31, 16)
+ *
+ * Get:
+ *  a = FIELD_GET(REG_FIELD_A, reg);
+ *  b = FIELD_GET(REG_FIELD_B, reg);
+ *
+ * Set:
+ *  reg = FIELD_PREP(REG_FIELD_A, 1) |
+ *	  FIELD_PREP(REG_FIELD_B, 0) |
+ *	  FIELD_PREP(REG_FIELD_C, c) |
+ *	  FIELD_PREP(REG_FIELD_D, 0x40);
+ *
+ * Modify:
+ *  reg &= ~REG_FIELD_C;
+ *  reg |= FIELD_PREP(REG_FIELD_C, c);
+ */
+
+#define __bf_shf(x) (__builtin_ffsll(x) - 1)
+
+#define __BF_FIELD_CHECK(_mask, _reg, _val, _pfx)			\
+	({								\
+		BUILD_BUG_ON_MSG(!__builtin_constant_p(_mask),		\
+				 _pfx "mask is not constant");		\
+		BUILD_BUG_ON_MSG(!(_mask), _pfx "mask is zero");	\
+		BUILD_BUG_ON_MSG(__builtin_constant_p(_val) ?		\
+				 ~((_mask) >> __bf_shf(_mask)) & (_val) : 0, \
+				 _pfx "value too large for the field"); \
+		BUILD_BUG_ON_MSG((_mask) > (typeof(_reg))~0ull,		\
+				 _pfx "type of reg too small for mask"); \
+		__BUILD_BUG_ON_NOT_POWER_OF_2((_mask) +			\
+					      (1ULL << __bf_shf(_mask))); \
+	})
+
+/**
+ * FIELD_FIT() - check if value fits in the field
+ * @_mask: shifted mask defining the field's length and position
+ * @_val:  value to test against the field
+ *
+ * Return: true if @_val can fit inside @_mask, false if @_val is too big.
+ */
+#define FIELD_FIT(_mask, _val)						\
+	({								\
+		__BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_FIT: ");	\
+		!((((typeof(_mask))_val) << __bf_shf(_mask)) & ~(_mask)); \
+	})
+
+/**
+ * FIELD_PREP() - prepare a bitfield element
+ * @_mask: shifted mask defining the field's length and position
+ * @_val:  value to put in the field
+ *
+ * FIELD_PREP() masks and shifts up the value.  The result should
+ * be combined with other fields of the bitfield using logical OR.
+ */
+#define FIELD_PREP(_mask, _val)						\
+	({								\
+		__BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: ");	\
+		((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask);	\
+	})
+
+/**
+ * FIELD_GET() - extract a bitfield element
+ * @_mask: shifted mask defining the field's length and position
+ * @_reg:  value of entire bitfield
+ *
+ * FIELD_GET() extracts the field specified by @_mask from the
+ * bitfield passed in as @_reg by masking and shifting it down.
+ */
+#define FIELD_GET(_mask, _reg)						\
+	({								\
+		__BF_FIELD_CHECK(_mask, _reg, 0U, "FIELD_GET: ");	\
+		(typeof(_mask))(((_reg) & (_mask)) >> __bf_shf(_mask));	\
+	})
+
+extern void __compiletime_warning("value doesn't fit into mask")
+__field_overflow(void);
+extern void __compiletime_error("bad bitfield mask")
+__bad_mask(void);
+static __always_inline u64 field_multiplier(u64 field)
+{
+	if ((field | (field - 1)) & ((field | (field - 1)) + 1))
+		__bad_mask();
+	return field & -field;
+}
+static __always_inline u64 field_mask(u64 field)
+{
+	return field / field_multiplier(field);
+}
+#define ____MAKE_OP(type,base,to,from)					\
+static __always_inline __##type type##_encode_bits(base v, base field)	\
+{									\
+        if (__builtin_constant_p(v) &&	(v & ~field_multiplier(field)))	\
+			    __field_overflow();				\
+	return to((v & field_mask(field)) * field_multiplier(field));	\
+}									\
+static __always_inline __##type type##_replace_bits(__##type old,	\
+					base val, base field)		\
+{									\
+	return (old & ~to(field)) | type##_encode_bits(val, field);	\
+}									\
+static __always_inline void type##p_replace_bits(__##type *p,		\
+					base val, base field)		\
+{									\
+	*p = (*p & ~to(field)) | type##_encode_bits(val, field);	\
+}									\
+static __always_inline base type##_get_bits(__##type v, base field)	\
+{									\
+	return (from(v) & field)/field_multiplier(field);		\
+}
+#define __MAKE_OP(size)							\
+	____MAKE_OP(le##size,u##size,cpu_to_le##size,le##size##_to_cpu)	\
+	____MAKE_OP(be##size,u##size,cpu_to_be##size,be##size##_to_cpu)	\
+	____MAKE_OP(u##size,u##size,,)
+__MAKE_OP(16)
+__MAKE_OP(32)
+__MAKE_OP(64)
+#undef __MAKE_OP
+#undef ____MAKE_OP
+
+#endif
-- 
2.14.3


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 04/19] ARM: i.MX: Add a function to extract BMOD value

[Andrey Smirnov]

The location of BMDO field in SBMR/SBMR2 registers is consistent
across all i.MX SoCs starting from i.MX53. Add simple helper function
imx53_get_bmod and use it to avoid code duplication.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 arch/arm/mach-imx/boot.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/arch/arm/mach-imx/boot.c b/arch/arm/mach-imx/boot.c
index 4657fa017..9f5ddf629 100644
--- a/arch/arm/mach-imx/boot.c
+++ b/arch/arm/mach-imx/boot.c
@@ -15,6 +15,7 @@
 #include <bootsource.h>
 #include <environment.h>
 #include <init.h>
+#include <linux/bitfield.h>
 #include <magicvar.h>
 
 #include <io.h>
@@ -169,12 +170,19 @@ void imx51_get_boot_source(enum bootsource *src, int *instance)
 }
 
 #define IMX53_SRC_SBMR	0x4
+#define SRC_SBMR_BMOD	GENMASK(25, 24)
+
+static unsigned int imx53_get_bmod(uint32_t r)
+{
+	return FIELD_GET(SRC_SBMR_BMOD, r);
+}
+
 void imx53_get_boot_source(enum bootsource *src, int *instance)
 {
 	void __iomem *src_base = IOMEM(MX53_SRC_BASE_ADDR);
 	uint32_t cfg1 = readl(src_base + IMX53_SRC_SBMR);
 
-	if (((cfg1 >> 24) & 0x3) == 0x3) {
+	if (imx53_get_bmod(cfg1) == 0x3) {
 		*src = BOOTSOURCE_USB;
 		*instance = 0;
 		return;
@@ -225,12 +233,8 @@ void imx6_get_boot_source(enum bootsource *src, int *instance)
 	uint32_t sbmr1 = readl(src_base + IMX6_SRC_SBMR1);
 	uint32_t sbmr2 = readl(src_base + IMX6_SRC_SBMR2);
 	uint32_t boot_cfg_4_2_0;
-	int boot_mode;
-
-	/* BMOD[1:0] */
-	boot_mode = (sbmr2 >> 24) & 0x3;
 
-	switch (boot_mode) {
+	switch (imx53_get_bmod(sbmr2)) {
 	case 0: /* Fuses, fall through */
 	case 2: /* internal boot */
 		goto internal_boot;
@@ -290,12 +294,8 @@ void imx7_get_boot_source(enum bootsource *src, int *instance)
 	void __iomem *src_base = IOMEM(MX7_SRC_BASE_ADDR);
 	uint32_t sbmr1 = readl(src_base + IMX7_SRC_SBMR1);
 	uint32_t sbmr2 = readl(src_base + IMX7_SRC_SBMR2);
-	int boot_mode;
-
-	/* BMOD[1:0] */
-	boot_mode = (sbmr2 >> 24) & 0x3;
 
-	switch (boot_mode) {
+	switch (imx53_get_bmod(sbmr2)) {
 	case 0: /* Fuses, fall through */
 	case 2: /* internal boot */
 		goto internal_boot;
-- 
2.14.3


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 05/19] ARM: i.MX: Simplify serial bootsource detection for i.MX6 and 7

[Andrey Smirnov]

The algorithm to detect serial mode can be shared between i.M6 and
i.MX7 as wall as simplified a bit by replacing swich goto/break/return
termination logic with more trivial if statements.

This commit also sets the stage for additional improvements in the
commits that follow.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 arch/arm/mach-imx/boot.c | 50 +++++++++++++++++++++++-------------------------
 1 file changed, 24 insertions(+), 26 deletions(-)

diff --git a/arch/arm/mach-imx/boot.c b/arch/arm/mach-imx/boot.c
index 9f5ddf629..dc385ceae 100644
--- a/arch/arm/mach-imx/boot.c
+++ b/arch/arm/mach-imx/boot.c
@@ -226,6 +226,18 @@ void imx53_get_boot_source(enum bootsource *src, int *instance)
 
 #define IMX6_SRC_SBMR1	0x04
 #define IMX6_SRC_SBMR2	0x1c
+#define IMX6_BMOD_SERIAL	0b01
+#define IMX6_BMOD_RESERVED	0b11
+
+static bool imx6_bootsource_reserved(uint32_t sbmr2)
+{
+	return imx53_get_bmod(sbmr2) == IMX6_BMOD_RESERVED;
+}
+
+static bool imx6_bootsource_serial(uint32_t sbmr2)
+{
+	return imx53_get_bmod(sbmr2) == IMX6_BMOD_SERIAL;
+}
 
 void imx6_get_boot_source(enum bootsource *src, int *instance)
 {
@@ -234,20 +246,13 @@ void imx6_get_boot_source(enum bootsource *src, int *instance)
 	uint32_t sbmr2 = readl(src_base + IMX6_SRC_SBMR2);
 	uint32_t boot_cfg_4_2_0;
 
-	switch (imx53_get_bmod(sbmr2)) {
-	case 0: /* Fuses, fall through */
-	case 2: /* internal boot */
-		goto internal_boot;
-	case 1: /* Serial Downloader */
-		*src = BOOTSOURCE_SERIAL;
-		break;
-	case 3: /* reserved */
-		break;
-	};
-
-	return;
+	if (imx6_bootsource_reserved(sbmr2))
+		return;
 
-internal_boot:
+	if (imx6_bootsource_serial(sbmr2)) {
+		*src = BOOTSOURCE_SERIAL;
+		return;
+	}
 
 	/* BOOT_CFG1[7:4] */
 	switch ((sbmr1 >> 4) & 0xf) {
@@ -295,20 +300,13 @@ void imx7_get_boot_source(enum bootsource *src, int *instance)
 	uint32_t sbmr1 = readl(src_base + IMX7_SRC_SBMR1);
 	uint32_t sbmr2 = readl(src_base + IMX7_SRC_SBMR2);
 
-	switch (imx53_get_bmod(sbmr2)) {
-	case 0: /* Fuses, fall through */
-	case 2: /* internal boot */
-		goto internal_boot;
-	case 1: /* Serial Downloader */
-		*src = BOOTSOURCE_SERIAL;
-		break;
-	case 3: /* reserved */
-		break;
-	};
-
-	return;
+	if (imx6_bootsource_reserved(sbmr2))
+		return;
 
-internal_boot:
+	if (imx6_bootsource_serial(sbmr2)) {
+		*src = BOOTSOURCE_SERIAL;
+		return;
+	}
 
 	switch ((sbmr1 >> 12) & 0xf) {
 	case 1:
-- 
2.14.3


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 06/19] ARM: i.MX: Account for unprogrammed fuses on i.MX6 and i.MX7

[Andrey Smirnov]

On both i.MX6 and i.MX7 (also true for VFxxx) there's an additional
path that leads mask ROM to switch into serial bootloader mode. Add
code to support it.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 arch/arm/mach-imx/boot.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-imx/boot.c b/arch/arm/mach-imx/boot.c
index dc385ceae..7c59c2181 100644
--- a/arch/arm/mach-imx/boot.c
+++ b/arch/arm/mach-imx/boot.c
@@ -228,6 +228,8 @@ void imx53_get_boot_source(enum bootsource *src, int *instance)
 #define IMX6_SRC_SBMR2	0x1c
 #define IMX6_BMOD_SERIAL	0b01
 #define IMX6_BMOD_RESERVED	0b11
+#define IMX6_BMOD_FUSES		0b00
+#define BT_FUSE_SEL		BIT(4)
 
 static bool imx6_bootsource_reserved(uint32_t sbmr2)
 {
@@ -236,7 +238,14 @@ static bool imx6_bootsource_reserved(uint32_t sbmr2)
 
 static bool imx6_bootsource_serial(uint32_t sbmr2)
 {
-	return imx53_get_bmod(sbmr2) == IMX6_BMOD_SERIAL;
+	return imx53_get_bmod(sbmr2) == IMX6_BMOD_SERIAL ||
+		/*
+		 * If boot from fuses is selected and fuses are not
+		 * programmed by setting BT_FUSE_SEL, ROM code will
+		 * fallback to serial mode
+		 */
+	       (imx53_get_bmod(sbmr2) == IMX6_BMOD_FUSES &&
+		!(sbmr2 & BT_FUSE_SEL));
 }
 
 void imx6_get_boot_source(enum bootsource *src, int *instance)
-- 
2.14.3


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 07/19] ARM: i.MX7: boot: Add code to handle SD/MMC manufacture mode

[Andrey Smirnov]

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 arch/arm/mach-imx/boot.c | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/arch/arm/mach-imx/boot.c b/arch/arm/mach-imx/boot.c
index 7c59c2181..e2696a934 100644
--- a/arch/arm/mach-imx/boot.c
+++ b/arch/arm/mach-imx/boot.c
@@ -303,6 +303,19 @@ void imx6_get_boot_source(enum bootsource *src, int *instance)
 #define IMX7_SRC_SBMR1	0x58
 #define IMX7_SRC_SBMR2	0x70
 
+#define IMX_BOOT_SW_INFO_POINTER_ADDR	0x000001E8
+#define IMX_BOOT_SW_INFO_BDT_SD		0x1
+
+struct imx_boot_sw_info {
+	uint8_t  reserved_1;
+	uint8_t  boot_device_instance;
+	uint8_t  boot_device_type;
+	uint8_t  reserved_2;
+	uint32_t frequency_hz[4]; /* Various frequencies (ARM, AXI,
+				   * DDR, etc.). Not used */
+	uint32_t reserved_3[3];
+} __packed;
+
 void imx7_get_boot_source(enum bootsource *src, int *instance)
 {
 	void __iomem *src_base = IOMEM(MX7_SRC_BASE_ADDR);
@@ -313,6 +326,33 @@ void imx7_get_boot_source(enum bootsource *src, int *instance)
 		return;
 
 	if (imx6_bootsource_serial(sbmr2)) {
+		/*
+		 * On i.MX7 ROM code will try to bood from uSDHC1
+		 * before entering serial mode. It doesn't seem to be
+		 * reflected in the contents of SBMR1 in any way when
+		 * that happens, so we check "Boot_SW_Info" structure
+		 * (per 6.6.14 Boot information for software) to see
+		 * if that really happened.
+		 *
+		 * FIXME: This behaviour can be inhibited by
+		 * DISABLE_SDMMC_MFG, but location of that fuse
+		 * doesn't seem to be documented anywhere. Once that
+		 * is known it should be taken into account here.
+		 */
+		const struct imx_boot_sw_info *info;
+
+		info = (const void *)readl(IMX_BOOT_SW_INFO_POINTER_ADDR);
+
+		if (info->boot_device_type == IMX_BOOT_SW_INFO_BDT_SD) {
+			*src = BOOTSOURCE_MMC;
+			/*
+			 * We are expecting to only ever boot from
+			 * uSDHC1 here
+			 */
+			WARN_ON(*instance = info->boot_device_instance);
+			return;
+		}
+
 		*src = BOOTSOURCE_SERIAL;
 		return;
 	}
-- 
2.14.3


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 08/19] ARM: i.MX7: boot: Remove incorrect NAND bootsource detection

[Andrey Smirnov]

i.MX7 differs from i.MX6 and i.MX53 and bit 7 in SBMR is not used to
signify boot from NAND.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 arch/arm/mach-imx/boot.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/arch/arm/mach-imx/boot.c b/arch/arm/mach-imx/boot.c
index e2696a934..5484812a6 100644
--- a/arch/arm/mach-imx/boot.c
+++ b/arch/arm/mach-imx/boot.c
@@ -380,10 +380,6 @@ void imx7_get_boot_source(enum bootsource *src, int *instance)
 		break;
 	}
 
-	/* BOOT_CFG1[7:0] */
-	if (sbmr1 & (1 << 7))
-		*src = BOOTSOURCE_NAND;
-
 	return;
 }
 
-- 
2.14.3


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 09/19] ARM: i.MX7: boot: Fix SPI-NOR/QSPI boot source mixup

[Andrey Smirnov]

As per "Table 6-33. Boot device selection" from "i.MX 7Dual
Applications Processor Reference Manual, Rev. 1, 01/2018" QSPI is
encoded with 0b0100 and Serial ROM with 0b0110. Fix the original code
to use correct values.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 arch/arm/mach-imx/boot.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-imx/boot.c b/arch/arm/mach-imx/boot.c
index 5484812a6..f9d730f6d 100644
--- a/arch/arm/mach-imx/boot.c
+++ b/arch/arm/mach-imx/boot.c
@@ -366,11 +366,11 @@ void imx7_get_boot_source(enum bootsource *src, int *instance)
 	case 3:
 		*src = BOOTSOURCE_NAND;
 		break;
-	case 4:
+	case 6:
 		*src = BOOTSOURCE_SPI_NOR,
 		*instance = (sbmr1 >> 9 & 0x7);
 		break;
-	case 6:
+	case 4:
 		*src = BOOTSOURCE_SPI; /* Really: qspi */
 		break;
 	case 5:
-- 
2.14.3


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 10/19] ARM: i.MX: boot: Remove unnecessary returns

[Andrey Smirnov]

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 arch/arm/mach-imx/boot.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/arch/arm/mach-imx/boot.c b/arch/arm/mach-imx/boot.c
index f9d730f6d..4bb26d942 100644
--- a/arch/arm/mach-imx/boot.c
+++ b/arch/arm/mach-imx/boot.c
@@ -296,8 +296,6 @@ void imx6_get_boot_source(enum bootsource *src, int *instance)
 	/* BOOT_CFG1[7:0] */
 	if (sbmr1 & (1 << 7))
 		*src = BOOTSOURCE_NAND;
-
-	return;
 }
 
 #define IMX7_SRC_SBMR1	0x58
@@ -379,8 +377,6 @@ void imx7_get_boot_source(enum bootsource *src, int *instance)
 	default:
 		break;
 	}
-
-	return;
 }
 
 static void
-- 
2.14.3


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 11/19] ARM: i.MX: boot: Move magic values into small functions

[Andrey Smirnov]

Move code to extract appropiate BOOT_CFG bits to decode bootsource on
i.MX53, 6, and 7 into small functions for clarity and to allow sharing
the code between i.MX53 and i.MX6.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 arch/arm/mach-imx/boot.c | 44 ++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 40 insertions(+), 4 deletions(-)

diff --git a/arch/arm/mach-imx/boot.c b/arch/arm/mach-imx/boot.c
index 4bb26d942..61ac8dadf 100644
--- a/arch/arm/mach-imx/boot.c
+++ b/arch/arm/mach-imx/boot.c
@@ -172,11 +172,38 @@ void imx51_get_boot_source(enum bootsource *src, int *instance)
 #define IMX53_SRC_SBMR	0x4
 #define SRC_SBMR_BMOD	GENMASK(25, 24)
 
+#define __BOOT_CFG(n, m, l)	GENMASK((m) + ((n) - 1) * 8, \
+					(l) + ((n) - 1) * 8)
+#define BOOT_CFG1(m, l)		__BOOT_CFG(1, m, l)
+
+#define ___BOOT_CFG(n, i)	__BOOT_CFG(n, i, i)
+#define __MAKE_BOOT_CFG_BITS(idx)					\
+	enum {								\
+		BOOT_CFG##idx##_0 = ___BOOT_CFG(idx, 0),		\
+		BOOT_CFG##idx##_1 = ___BOOT_CFG(idx, 1),		\
+		BOOT_CFG##idx##_2 = ___BOOT_CFG(idx, 2),		\
+		BOOT_CFG##idx##_3 = ___BOOT_CFG(idx, 3),		\
+		BOOT_CFG##idx##_4 = ___BOOT_CFG(idx, 4),		\
+		BOOT_CFG##idx##_5 = ___BOOT_CFG(idx, 5),		\
+		BOOT_CFG##idx##_6 = ___BOOT_CFG(idx, 6),		\
+		BOOT_CFG##idx##_7 = ___BOOT_CFG(idx, 7),		\
+	};
+
+__MAKE_BOOT_CFG_BITS(1)
+#undef __MAKE_BOOT_CFG
+#undef ___BOOT_CFG
+
+
 static unsigned int imx53_get_bmod(uint32_t r)
 {
 	return FIELD_GET(SRC_SBMR_BMOD, r);
 }
 
+static int imx53_bootsource_internal(uint32_t r)
+{
+	return FIELD_GET(BOOT_CFG1(7, 4), r);
+}
+
 void imx53_get_boot_source(enum bootsource *src, int *instance)
 {
 	void __iomem *src_base = IOMEM(MX53_SRC_BASE_ADDR);
@@ -188,7 +215,7 @@ void imx53_get_boot_source(enum bootsource *src, int *instance)
 		return;
 	}
 
-	switch ((cfg1 & 0xff) >> 4) {
+	switch (imx53_bootsource_internal(cfg1)) {
 	case 2:
 		*src = BOOTSOURCE_HD;
 		break;
@@ -263,8 +290,7 @@ void imx6_get_boot_source(enum bootsource *src, int *instance)
 		return;
 	}
 
-	/* BOOT_CFG1[7:4] */
-	switch ((sbmr1 >> 4) & 0xf) {
+	switch (imx53_bootsource_internal(sbmr1)) {
 	case 2:
 		*src = BOOTSOURCE_HD;
 		break;
@@ -301,9 +327,19 @@ void imx6_get_boot_source(enum bootsource *src, int *instance)
 #define IMX7_SRC_SBMR1	0x58
 #define IMX7_SRC_SBMR2	0x70
 
+/*
+ * Re-defined to match the naming in reference manual
+ */
+#define BOOT_CFG(m, l)	BOOT_CFG1(m, l)
+
 #define IMX_BOOT_SW_INFO_POINTER_ADDR	0x000001E8
 #define IMX_BOOT_SW_INFO_BDT_SD		0x1
 
+static unsigned int imx7_bootsource_internal(uint32_t r)
+{
+	return FIELD_GET(BOOT_CFG(15, 12), r);
+}
+
 struct imx_boot_sw_info {
 	uint8_t  reserved_1;
 	uint8_t  boot_device_instance;
@@ -355,7 +391,7 @@ void imx7_get_boot_source(enum bootsource *src, int *instance)
 		return;
 	}
 
-	switch ((sbmr1 >> 12) & 0xf) {
+	switch (imx7_bootsource_internal(sbmr1)) {
 	case 1:
 	case 2:
 		*src = BOOTSOURCE_MMC;
-- 
2.14.3


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 12/19] ARM: i.MX: boot: Share code to detect NAND as a boot source

[Andrey Smirnov]

Share code to detect NAND as a boot source between i.MX53 and i.MX6
which behave the same in that aspect.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 arch/arm/mach-imx/boot.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mach-imx/boot.c b/arch/arm/mach-imx/boot.c
index 61ac8dadf..a5dff77df 100644
--- a/arch/arm/mach-imx/boot.c
+++ b/arch/arm/mach-imx/boot.c
@@ -204,6 +204,11 @@ static int imx53_bootsource_internal(uint32_t r)
 	return FIELD_GET(BOOT_CFG1(7, 4), r);
 }
 
+static bool imx53_bootsource_nand(uint32_t r)
+{
+	return FIELD_GET(BOOT_CFG1_7, r);
+}
+
 void imx53_get_boot_source(enum bootsource *src, int *instance)
 {
 	void __iomem *src_base = IOMEM(MX53_SRC_BASE_ADDR);
@@ -235,7 +240,7 @@ void imx53_get_boot_source(enum bootsource *src, int *instance)
 		break;
 	}
 
-	if (cfg1 & (1 << 7))
+	if (imx53_bootsource_nand(cfg1))
 		*src = BOOTSOURCE_NAND;
 
 
@@ -319,8 +324,7 @@ void imx6_get_boot_source(enum bootsource *src, int *instance)
 		break;
 	}
 
-	/* BOOT_CFG1[7:0] */
-	if (sbmr1 & (1 << 7))
+	if (imx53_bootsource_nand(sbmr1))
 		*src = BOOTSOURCE_NAND;
 }
 
-- 
2.14.3


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 13/19] ARM: i.MX: boot: Check for NAND boot only if necessary on i.MX53, 6

[Andrey Smirnov]

We don't need to check if the boot source is NAND in cases when we
already know for a fact that we booted from something else. To avoid
that, move the NAND check to be done inside of default branch of
the preceeding switch statement.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 arch/arm/mach-imx/boot.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/arch/arm/mach-imx/boot.c b/arch/arm/mach-imx/boot.c
index a5dff77df..f84be5c68 100644
--- a/arch/arm/mach-imx/boot.c
+++ b/arch/arm/mach-imx/boot.c
@@ -237,13 +237,11 @@ void imx53_get_boot_source(enum bootsource *src, int *instance)
 		*src = BOOTSOURCE_MMC;
 		break;
 	default:
+		if (imx53_bootsource_nand(cfg1))
+			*src = BOOTSOURCE_NAND;
 		break;
 	}
 
-	if (imx53_bootsource_nand(cfg1))
-		*src = BOOTSOURCE_NAND;
-
-
 	switch (*src) {
 	case BOOTSOURCE_MMC:
 	case BOOTSOURCE_SPI:
@@ -321,11 +319,10 @@ void imx6_get_boot_source(enum bootsource *src, int *instance)
 		*instance = (sbmr1 >> 11) & 0x3;
 		break;
 	default:
+		if (imx53_bootsource_nand(sbmr1))
+			*src = BOOTSOURCE_NAND;
 		break;
 	}
-
-	if (imx53_bootsource_nand(sbmr1))
-		*src = BOOTSOURCE_NAND;
 }
 
 #define IMX7_SRC_SBMR1	0x58
-- 
2.14.3


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 14/19] ARM: i.MX53: boot: Move magic numbers info utility functions

[Andrey Smirnov]

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 arch/arm/mach-imx/boot.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/arch/arm/mach-imx/boot.c b/arch/arm/mach-imx/boot.c
index f84be5c68..799d5ec93 100644
--- a/arch/arm/mach-imx/boot.c
+++ b/arch/arm/mach-imx/boot.c
@@ -171,10 +171,12 @@ void imx51_get_boot_source(enum bootsource *src, int *instance)
 
 #define IMX53_SRC_SBMR	0x4
 #define SRC_SBMR_BMOD	GENMASK(25, 24)
+#define IMX53_BMOD_SERIAL	0b11
 
 #define __BOOT_CFG(n, m, l)	GENMASK((m) + ((n) - 1) * 8, \
 					(l) + ((n) - 1) * 8)
 #define BOOT_CFG1(m, l)		__BOOT_CFG(1, m, l)
+#define BOOT_CFG3(m, l)		__BOOT_CFG(3, m, l)
 
 #define ___BOOT_CFG(n, i)	__BOOT_CFG(n, i, i)
 #define __MAKE_BOOT_CFG_BITS(idx)					\
@@ -204,17 +206,27 @@ static int imx53_bootsource_internal(uint32_t r)
 	return FIELD_GET(BOOT_CFG1(7, 4), r);
 }
 
+static int imx53_port_select(uint32_t r)
+{
+	return FIELD_GET(BOOT_CFG3(5, 4), r);
+}
+
 static bool imx53_bootsource_nand(uint32_t r)
 {
 	return FIELD_GET(BOOT_CFG1_7, r);
 }
 
+static enum bootsource imx53_bootsource_serial_rom(uint32_t r)
+{
+	return BOOT_CFG1(r, 3) ? BOOTSOURCE_SPI : BOOTSOURCE_I2C;
+}
+
 void imx53_get_boot_source(enum bootsource *src, int *instance)
 {
 	void __iomem *src_base = IOMEM(MX53_SRC_BASE_ADDR);
 	uint32_t cfg1 = readl(src_base + IMX53_SRC_SBMR);
 
-	if (imx53_get_bmod(cfg1) == 0x3) {
+	if (imx53_get_bmod(cfg1) == IMX53_BMOD_SERIAL) {
 		*src = BOOTSOURCE_USB;
 		*instance = 0;
 		return;
@@ -225,10 +237,7 @@ void imx53_get_boot_source(enum bootsource *src, int *instance)
 		*src = BOOTSOURCE_HD;
 		break;
 	case 3:
-		if (cfg1 & (1 << 3))
-			*src = BOOTSOURCE_SPI;
-		else
-			*src = BOOTSOURCE_I2C;
+		*src = imx53_bootsource_serial_rom(cfg1);
 		break;
 	case 4:
 	case 5:
@@ -246,7 +255,7 @@ void imx53_get_boot_source(enum bootsource *src, int *instance)
 	case BOOTSOURCE_MMC:
 	case BOOTSOURCE_SPI:
 	case BOOTSOURCE_I2C:
-		*instance = (cfg1 >> 20) & 0x3;
+		*instance = imx53_port_select(cfg1);
 		break;
 	default:
 		*instance = 0;
-- 
2.14.3


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 15/19] ARM: i.MX6: boot: Move magic numbers into utility functions

[Andrey Smirnov]

Move magic numbers and algorithm for determining Serial ROM bootsource
and boot instance into utility functions. Add a comment on the logic
behing the latter while at it.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 arch/arm/mach-imx/boot.c | 67 ++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 53 insertions(+), 14 deletions(-)

diff --git a/arch/arm/mach-imx/boot.c b/arch/arm/mach-imx/boot.c
index 799d5ec93..60307ffb6 100644
--- a/arch/arm/mach-imx/boot.c
+++ b/arch/arm/mach-imx/boot.c
@@ -176,7 +176,9 @@ void imx51_get_boot_source(enum bootsource *src, int *instance)
 #define __BOOT_CFG(n, m, l)	GENMASK((m) + ((n) - 1) * 8, \
 					(l) + ((n) - 1) * 8)
 #define BOOT_CFG1(m, l)		__BOOT_CFG(1, m, l)
+#define BOOT_CFG2(m, l)		__BOOT_CFG(2, m, l)
 #define BOOT_CFG3(m, l)		__BOOT_CFG(3, m, l)
+#define BOOT_CFG4(m, l)		__BOOT_CFG(4, m, l)
 
 #define ___BOOT_CFG(n, i)	__BOOT_CFG(n, i, i)
 #define __MAKE_BOOT_CFG_BITS(idx)					\
@@ -192,6 +194,8 @@ void imx51_get_boot_source(enum bootsource *src, int *instance)
 	};
 
 __MAKE_BOOT_CFG_BITS(1)
+__MAKE_BOOT_CFG_BITS(2)
+__MAKE_BOOT_CFG_BITS(4)
 #undef __MAKE_BOOT_CFG
 #undef ___BOOT_CFG
 
@@ -287,12 +291,57 @@ static bool imx6_bootsource_serial(uint32_t sbmr2)
 		!(sbmr2 & BT_FUSE_SEL));
 }
 
+static int __imx6_bootsource_serial_rom(uint32_t r)
+{
+	return FIELD_GET(BOOT_CFG4(2, 0), r);
+}
+
+/*
+ * Serial ROM bootsource on i.MX6 are as follows:
+ *
+ *	000 - ECSPI-1
+ *	001 - ECSPI-2
+ *	010 - ECSPI-3
+ *	011 - ECSPI-4
+ *	100 - ECSPI-5
+ *	101 - I2C1
+ *	110 - I2C2
+ *	111 - I2C3
+ *
+ * There's no single bit that would tell us we are booting from I2C or
+ * SPI, so we just have to compare the "source" agains the value for
+ * I2C1 for both: calculating bootsource and boot instance.
+ */
+#define IMX6_BOOTSOURCE_SERIAL_ROM_I2C1	0b101
+
+static enum bootsource imx6_bootsource_serial_rom(uint32_t sbmr)
+{
+	const int source = __imx6_bootsource_serial_rom(sbmr);
+
+	return source < IMX6_BOOTSOURCE_SERIAL_ROM_I2C1 ?
+		BOOTSOURCE_SPI : BOOTSOURCE_I2C;
+}
+
+static int imx6_boot_instance_serial_rom(uint32_t sbmr)
+{
+	const int source = __imx6_bootsource_serial_rom(sbmr);
+
+	if (source < IMX6_BOOTSOURCE_SERIAL_ROM_I2C1)
+		return source;
+
+	return source - IMX6_BOOTSOURCE_SERIAL_ROM_I2C1;
+}
+
+static int imx6_boot_instance_mmc(uint32_t r)
+{
+	return FIELD_GET(BOOT_CFG2(4, 3), r);
+}
+
 void imx6_get_boot_source(enum bootsource *src, int *instance)
 {
 	void __iomem *src_base = IOMEM(MX6_SRC_BASE_ADDR);
 	uint32_t sbmr1 = readl(src_base + IMX6_SRC_SBMR1);
 	uint32_t sbmr2 = readl(src_base + IMX6_SRC_SBMR2);
-	uint32_t boot_cfg_4_2_0;
 
 	if (imx6_bootsource_reserved(sbmr2))
 		return;
@@ -307,25 +356,15 @@ void imx6_get_boot_source(enum bootsource *src, int *instance)
 		*src = BOOTSOURCE_HD;
 		break;
 	case 3:
-		/* BOOT_CFG4[2:0] */
-		boot_cfg_4_2_0 = (sbmr1 >> 24) & 0x7;
-
-		if (boot_cfg_4_2_0 > 4) {
-			*src = BOOTSOURCE_I2C;
-			*instance = boot_cfg_4_2_0 - 5;
-		} else {
-			*src = BOOTSOURCE_SPI;
-			*instance = boot_cfg_4_2_0;
-		}
+		*src = imx6_bootsource_serial_rom(sbmr1);
+		*instance = imx6_boot_instance_serial_rom(sbmr1);
 		break;
 	case 4:
 	case 5:
 	case 6:
 	case 7:
 		*src = BOOTSOURCE_MMC;
-
-		/* BOOT_CFG2[4:3] */
-		*instance = (sbmr1 >> 11) & 0x3;
+		*instance = imx6_boot_instance_mmc(sbmr1);
 		break;
 	default:
 		if (imx53_bootsource_nand(sbmr1))
-- 
2.14.3


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 16/19] ARM: i.MX7: boot: Move magic numbers into utility functions

[Andrey Smirnov]

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 arch/arm/mach-imx/boot.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-imx/boot.c b/arch/arm/mach-imx/boot.c
index 60307ffb6..3f67c2510 100644
--- a/arch/arm/mach-imx/boot.c
+++ b/arch/arm/mach-imx/boot.c
@@ -389,6 +389,16 @@ static unsigned int imx7_bootsource_internal(uint32_t r)
 	return FIELD_GET(BOOT_CFG(15, 12), r);
 }
 
+static int imx7_boot_instance_spi_nor(uint32_t r)
+{
+	return FIELD_GET(BOOT_CFG(11, 9), r);
+}
+
+static int imx7_boot_instance_mmc(uint32_t r)
+{
+	return FIELD_GET(BOOT_CFG(11, 10), r);
+}
+
 struct imx_boot_sw_info {
 	uint8_t  reserved_1;
 	uint8_t  boot_device_instance;
@@ -444,14 +454,14 @@ void imx7_get_boot_source(enum bootsource *src, int *instance)
 	case 1:
 	case 2:
 		*src = BOOTSOURCE_MMC;
-		*instance = (sbmr1 >> 10 & 0x3);
+		*instance = imx7_boot_instance_mmc(sbmr1);
 		break;
 	case 3:
 		*src = BOOTSOURCE_NAND;
 		break;
 	case 6:
 		*src = BOOTSOURCE_SPI_NOR,
-		*instance = (sbmr1 >> 9 & 0x7);
+		*instance = imx7_boot_instance_spi_nor(sbmr1);
 		break;
 	case 4:
 		*src = BOOTSOURCE_SPI; /* Really: qspi */
-- 
2.14.3


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 17/19] bootsource: Add BOOTSOURCE_CAN

[Andrey Smirnov]

Add BOOTSOURCE_CAN for SoCs that can boot from CAN interface.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 common/bootsource.c  | 1 +
 include/bootsource.h | 1 +
 2 files changed, 2 insertions(+)

diff --git a/common/bootsource.c b/common/bootsource.c
index 707b07924..78ecd8267 100644
--- a/common/bootsource.c
+++ b/common/bootsource.c
@@ -36,6 +36,7 @@ static const char *bootsource_str[] = {
 	[BOOTSOURCE_HD] = "harddisk",
 	[BOOTSOURCE_USB] = "usb",
 	[BOOTSOURCE_NET] = "net",
+	[BOOTSOURCE_CAN] = "can",
 };
 
 static enum bootsource bootsource = BOOTSOURCE_UNKNOWN;
diff --git a/include/bootsource.h b/include/bootsource.h
index c6d3b3a98..064f6b9a2 100644
--- a/include/bootsource.h
+++ b/include/bootsource.h
@@ -16,6 +16,7 @@ enum bootsource {
 	BOOTSOURCE_HD,
 	BOOTSOURCE_USB,
 	BOOTSOURCE_NET,
+	BOOTSOURCE_CAN,
 };
 
 #define BOOTSOURCE_INSTANCE_UNKNOWN	-1
-- 
2.14.3


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 18/19] ARM: VFxxx: Implement code to detect bootsource

[Andrey Smirnov]

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 arch/arm/mach-imx/boot.c                 | 91 ++++++++++++++++++++++++++++++++
 arch/arm/mach-imx/include/mach/generic.h |  2 +
 2 files changed, 93 insertions(+)

diff --git a/arch/arm/mach-imx/boot.c b/arch/arm/mach-imx/boot.c
index 3f67c2510..f8a04d8f3 100644
--- a/arch/arm/mach-imx/boot.c
+++ b/arch/arm/mach-imx/boot.c
@@ -27,6 +27,7 @@
 #include <mach/imx53-regs.h>
 #include <mach/imx6-regs.h>
 #include <mach/imx7-regs.h>
+#include <mach/vf610-regs.h>
 
 /* [CTRL][TYPE] */
 static const enum bootsource locations[4][4] = {
@@ -474,6 +475,95 @@ void imx7_get_boot_source(enum bootsource *src, int *instance)
 	}
 }
 
+static int vf610_boot_instance_spi(uint32_t r)
+{
+	return FIELD_GET(BOOT_CFG1_1, r);
+}
+
+static int vf610_boot_instance_nor(uint32_t r)
+{
+	return FIELD_GET(BOOT_CFG1_3, r);
+}
+
+/*
+ * Vybrid's Serial ROM boot sources (BOOT_CFG4[2:0]) are as follows:
+ *
+ *	000 - SPI0
+ *	001 - SPI1
+ *	010 - SPI2
+ *	011 - SPI3
+ *	100 - I2C0
+ *	101 - I2C1
+ *	110 - I2C2
+ *	111 - I2C3
+ *
+ * Which we can neatly divide in two halves and use MSb to detect if
+ * bootsource is I2C or SPI EEPROM and 2 LSbs directly as boot
+ * insance.
+ */
+static enum bootsource vf610_bootsource_serial_rom(uint32_t r)
+{
+	return FIELD_GET(BOOT_CFG4_2, r) ? BOOTSOURCE_I2C : BOOTSOURCE_SPI_NOR;
+}
+
+static int vf610_boot_instance_serial_rom(uint32_t r)
+{
+	return __imx6_bootsource_serial_rom(r) & 0b11;
+}
+
+static int vf610_boot_instance_can(uint32_t r)
+{
+	return FIELD_GET(BOOT_CFG1_0, r);
+}
+
+static int vf610_boot_instance_mmc(uint32_t r)
+{
+	return FIELD_GET(BOOT_CFG2_3, r);
+}
+
+void vf610_get_boot_source(enum bootsource *src, int *instance)
+{
+	void __iomem *src_base = IOMEM(VF610_SRC_BASE_ADDR);
+	uint32_t sbmr1 = readl(src_base + IMX6_SRC_SBMR1);
+	uint32_t sbmr2 = readl(src_base + IMX6_SRC_SBMR2);
+
+	if (imx6_bootsource_reserved(sbmr2))
+		return;
+
+	if (imx6_bootsource_serial(sbmr2)) {
+		*src = BOOTSOURCE_SERIAL;
+		return;
+	}
+
+	switch (imx53_bootsource_internal(sbmr1)) {
+	case 0:
+		*src = BOOTSOURCE_SPI; /* Really: qspi */
+		*instance = vf610_boot_instance_spi(sbmr1);
+		break;
+	case 1:
+		*src = BOOTSOURCE_NOR;
+		*instance = vf610_boot_instance_nor(sbmr1);
+		break;
+	case 2:
+		*src = vf610_bootsource_serial_rom(sbmr1);
+		*instance = vf610_boot_instance_serial_rom(sbmr1);
+		break;
+	case 3:
+		*src = BOOTSOURCE_CAN;
+		*instance = vf610_boot_instance_can(sbmr1);
+		break;
+	case 6:
+	case 7:
+		*src = BOOTSOURCE_MMC;
+		*instance = vf610_boot_instance_mmc(sbmr1);
+		break;
+	default:
+		if (imx53_bootsource_nand(sbmr1))
+			*src = BOOTSOURCE_NAND;
+		break;
+	}
+}
+
 static void
 imx_boot_save_loc(void (*get_boot_source)(enum bootsource *, int *))
 {
@@ -499,3 +589,4 @@ IMX_BOOT_SAVE_LOC(imx51)
 IMX_BOOT_SAVE_LOC(imx53)
 IMX_BOOT_SAVE_LOC(imx6)
 IMX_BOOT_SAVE_LOC(imx7)
+IMX_BOOT_SAVE_LOC(vf610)
diff --git a/arch/arm/mach-imx/include/mach/generic.h b/arch/arm/mach-imx/include/mach/generic.h
index cb5675185..ad9d9cb02 100644
--- a/arch/arm/mach-imx/include/mach/generic.h
+++ b/arch/arm/mach-imx/include/mach/generic.h
@@ -15,6 +15,7 @@ void imx51_boot_save_loc(void);
 void imx53_boot_save_loc(void);
 void imx6_boot_save_loc(void);
 void imx7_boot_save_loc(void);
+void vf610_boot_save_loc(void);
 
 void imx25_get_boot_source(enum bootsource *src, int *instance);
 void imx35_get_boot_source(enum bootsource *src, int *instance);
@@ -22,6 +23,7 @@ void imx51_get_boot_source(enum bootsource *src, int *instance);
 void imx53_get_boot_source(enum bootsource *src, int *instance);
 void imx6_get_boot_source(enum bootsource *src, int *instance);
 void imx7_get_boot_source(enum bootsource *src, int *instance);
+void vf610_get_boot_source(enum bootsource *src, int *instance);
 
 int imx1_init(void);
 int imx21_init(void);
-- 
2.14.3


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 19/19] ARM: i.MX6: boot: Return BOOTSOURCE_SPI_NOR, not BOOTSOURCE_SPI

[Andrey Smirnov]

We use BOOTSOURCE_SPI to denote, among other things, QSPI on i.MX7 and
VFxxx, whereas on i.MX6 it is used to mean SPI-NOR. To make functions
like imx_xload() work consistently across various i.MX platforms use
already existent BOOTSOURCE_SPI_NOR constant to mean booting from
SPI-NOR on i.MX6 as well.

Replace all in-tree code that relying on the old value as well.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 arch/arm/boards/datamodul-edm-qmx6/board.c | 2 +-
 arch/arm/boards/dfi-fs700-m60/board.c      | 2 +-
 arch/arm/boards/phytec-som-imx6/board.c    | 2 +-
 arch/arm/boards/zii-imx6q-rdu2/lowlevel.c  | 2 +-
 arch/arm/mach-imx/boot.c                   | 2 +-
 arch/arm/mach-imx/xload.c                  | 2 +-
 6 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/arm/boards/datamodul-edm-qmx6/board.c b/arch/arm/boards/datamodul-edm-qmx6/board.c
index 043a93461..d93c940e3 100644
--- a/arch/arm/boards/datamodul-edm-qmx6/board.c
+++ b/arch/arm/boards/datamodul-edm-qmx6/board.c
@@ -132,7 +132,7 @@ static int realq7_device_init(void)
 		}
 		break;
 	default:
-	case BOOTSOURCE_SPI:
+	case BOOTSOURCE_SPI_NOR:
 		of_device_enable_path("/chosen/environment-spi");
 		break;
 	}
diff --git a/arch/arm/boards/dfi-fs700-m60/board.c b/arch/arm/boards/dfi-fs700-m60/board.c
index bef4612d9..2cb8e3106 100644
--- a/arch/arm/boards/dfi-fs700-m60/board.c
+++ b/arch/arm/boards/dfi-fs700-m60/board.c
@@ -105,7 +105,7 @@ static int dfi_fs700_m60_init(void)
 
 	phy_register_fixup_for_uid(PHY_ID_AR8031, AR_PHY_ID_MASK, ar8031_phy_fixup);
 
-	if (bootsource_get() == BOOTSOURCE_SPI)
+	if (bootsource_get() == BOOTSOURCE_SPI_NOR)
 		flag_spi |= BBU_HANDLER_FLAG_DEFAULT;
 	else
 		flag_mmc |= BBU_HANDLER_FLAG_DEFAULT;
diff --git a/arch/arm/boards/phytec-som-imx6/board.c b/arch/arm/boards/phytec-som-imx6/board.c
index 717a22963..ecb22f956 100644
--- a/arch/arm/boards/phytec-som-imx6/board.c
+++ b/arch/arm/boards/phytec-som-imx6/board.c
@@ -152,7 +152,7 @@ static int physom_imx6_devices_init(void)
 		environment_path = basprintf("/chosen/environment-nand");
 		envdev = "NAND flash";
 		break;
-	case BOOTSOURCE_SPI:
+	case BOOTSOURCE_SPI_NOR:
 		environment_path = basprintf("/chosen/environment-spinor");
 		envdev = "SPI NOR flash";
 		break;
diff --git a/arch/arm/boards/zii-imx6q-rdu2/lowlevel.c b/arch/arm/boards/zii-imx6q-rdu2/lowlevel.c
index 22ffdf85e..3b4dbd212 100644
--- a/arch/arm/boards/zii-imx6q-rdu2/lowlevel.c
+++ b/arch/arm/boards/zii-imx6q-rdu2/lowlevel.c
@@ -290,7 +290,7 @@ static noinline void rdu2_sram_setup(void)
 		write_regs(imx6q_dcd, ARRAY_SIZE(imx6q_dcd));
 
 	imx6_get_boot_source(&bootsrc, &instance);
-	if (bootsrc == BOOTSOURCE_SPI)
+	if (bootsrc == BOOTSOURCE_SPI_NOR)
 		imx6_spi_start_image(0);
 	else
 		imx6_esdhc_start_image(instance);
diff --git a/arch/arm/mach-imx/boot.c b/arch/arm/mach-imx/boot.c
index f8a04d8f3..bb5318510 100644
--- a/arch/arm/mach-imx/boot.c
+++ b/arch/arm/mach-imx/boot.c
@@ -320,7 +320,7 @@ static enum bootsource imx6_bootsource_serial_rom(uint32_t sbmr)
 	const int source = __imx6_bootsource_serial_rom(sbmr);
 
 	return source < IMX6_BOOTSOURCE_SERIAL_ROM_I2C1 ?
-		BOOTSOURCE_SPI : BOOTSOURCE_I2C;
+		BOOTSOURCE_SPI_NOR : BOOTSOURCE_I2C;
 }
 
 static int imx6_boot_instance_serial_rom(uint32_t sbmr)
diff --git a/arch/arm/mach-imx/xload.c b/arch/arm/mach-imx/xload.c
index 16d56ab28..921e9ade2 100644
--- a/arch/arm/mach-imx/xload.c
+++ b/arch/arm/mach-imx/xload.c
@@ -24,7 +24,7 @@ static __noreturn int imx_xload(void)
 		pr_info("booting from MMC\n");
 		buf = bootstrap_read_disk("disk0.0", "fat");
 		break;
-	case BOOTSOURCE_SPI:
+	case BOOTSOURCE_SPI_NOR:
 		pr_info("booting from SPI\n");
 		buf = bootstrap_read_devfs("dataflash0", false,
 					   SZ_256K, SZ_1M, SZ_1M);
-- 
2.14.3


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 01/19] ARM: i.MX: boot: Coalesce copy-pasted code

[Sascha Hauer]

On Mon, Apr 16, 2018 at 12:31:39PM -0700, Andrey Smirnov wrote:
> All of the instances of imx*_boot_save_loc() do exactly the same thing with
> the only difference being SoC-specific imx*_get_boot_source
> call. Convert the code into a generic function taking function pointer
> + a macro to take care of the boilerplate.
> 
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> ---
>  arch/arm/mach-imx/boot.c | 85 ++++++++++--------------------------------------
>  1 file changed, 17 insertions(+), 68 deletions(-)
> 
> +imx_boot_save_loc(void (*get_boot_source)(enum bootsource *, int *))
>  {
>  	enum bootsource src = BOOTSOURCE_UNKNOWN;
>  	int instance = BOOTSOURCE_INSTANCE_UNKNOWN;
>  
> -	imx7_get_boot_source(&src, &instance);
> +	get_boot_source(&src, &instance);
>  
>  	bootsource_set(src);
>  	bootsource_set_instance(instance);
>  }
> +
> +#define IMX_BOOT_SAVE_LOC(soc)					\
> +	void soc##_boot_save_loc(void)				\
> +	{							\
> +		imx_boot_save_loc(soc##_get_boot_source);	\
> +	}
> +
> +IMX_BOOT_SAVE_LOC(imx25)
> +IMX_BOOT_SAVE_LOC(imx27)
> +IMX_BOOT_SAVE_LOC(imx35)
> +IMX_BOOT_SAVE_LOC(imx51)
> +IMX_BOOT_SAVE_LOC(imx53)
> +IMX_BOOT_SAVE_LOC(imx6)
> +IMX_BOOT_SAVE_LOC(imx7)

I do not really like this patch. Yes, it saves a few lines of code, but
with the cost of making it less readable.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 00/19] i.MX bootsource bugfixes, refactoring and VFxxx support

[Sascha Hauer]

On Mon, Apr 16, 2018 at 12:31:38PM -0700, Andrey Smirnov wrote:
> Everyone:
> 
> This patchset is the result of my work on adding support for
> bootsource detection of VFxxx as well as fixing a number of
> bugs/unsupported corner cases in similar i.MX7 specific code.
> 
> NOTE: While VFxxx support is added in this series the code integrating
> it into the SoC initalization sequnce is not included. Patch for that
> is dependent on recently submitted "i.MX reset reason detection
> support" and I didn't want to intertwine two otherwise independent
> submissions.

Applied, thanks. Please send the missing vybrid patch so I can integrate
it here.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 01/19] ARM: i.MX: boot: Coalesce copy-pasted code

[Andrey Smirnov]

On Mon, Apr 16, 2018 at 11:58 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> On Mon, Apr 16, 2018 at 12:31:39PM -0700, Andrey Smirnov wrote:
>> All of the instances of imx*_boot_save_loc() do exactly the same thing with
>> the only difference being SoC-specific imx*_get_boot_source
>> call. Convert the code into a generic function taking function pointer
>> + a macro to take care of the boilerplate.
>>
>> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
>> ---
>>  arch/arm/mach-imx/boot.c | 85 ++++++++++--------------------------------------
>>  1 file changed, 17 insertions(+), 68 deletions(-)
>>
>> +imx_boot_save_loc(void (*get_boot_source)(enum bootsource *, int *))
>>  {
>>       enum bootsource src = BOOTSOURCE_UNKNOWN;
>>       int instance = BOOTSOURCE_INSTANCE_UNKNOWN;
>>
>> -     imx7_get_boot_source(&src, &instance);
>> +     get_boot_source(&src, &instance);
>>
>>       bootsource_set(src);
>>       bootsource_set_instance(instance);
>>  }
>> +
>> +#define IMX_BOOT_SAVE_LOC(soc)                                       \
>> +     void soc##_boot_save_loc(void)                          \
>> +     {                                                       \
>> +             imx_boot_save_loc(soc##_get_boot_source);       \
>> +     }
>> +
>> +IMX_BOOT_SAVE_LOC(imx25)
>> +IMX_BOOT_SAVE_LOC(imx27)
>> +IMX_BOOT_SAVE_LOC(imx35)
>> +IMX_BOOT_SAVE_LOC(imx51)
>> +IMX_BOOT_SAVE_LOC(imx53)
>> +IMX_BOOT_SAVE_LOC(imx6)
>> +IMX_BOOT_SAVE_LOC(imx7)
>
> I do not really like this patch. Yes, it saves a few lines of code, but
> with the cost of making it less readable.
>

Can you elaborate on what part is less readable? IMHO, replacing 68
line of mostly copy pasted code with 17 of a generic function is a bit
more than "saving a few lines of code", so I think it's worth trying
to find a solution that would be acceptable from both perspectives:
conciseness and readability.

Would replacing function pointer with a switch (imx_cpu_type) and
expanding IMX_BOOT_SAVE_LOC() make this patch readable enough to be
accepted?

Thanks,
Andrey Smirnov

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 01/19] ARM: i.MX: boot: Coalesce copy-pasted code

[Sascha Hauer]

On Tue, Apr 17, 2018 at 08:20:46AM -0700, Andrey Smirnov wrote:
> On Mon, Apr 16, 2018 at 11:58 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > On Mon, Apr 16, 2018 at 12:31:39PM -0700, Andrey Smirnov wrote:
> >> All of the instances of imx*_boot_save_loc() do exactly the same thing with
> >> the only difference being SoC-specific imx*_get_boot_source
> >> call. Convert the code into a generic function taking function pointer
> >> + a macro to take care of the boilerplate.
> >>
> >> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> >> ---
> >>  arch/arm/mach-imx/boot.c | 85 ++++++++++--------------------------------------
> >>  1 file changed, 17 insertions(+), 68 deletions(-)
> >>
> >> +imx_boot_save_loc(void (*get_boot_source)(enum bootsource *, int *))
> >>  {
> >>       enum bootsource src = BOOTSOURCE_UNKNOWN;
> >>       int instance = BOOTSOURCE_INSTANCE_UNKNOWN;
> >>
> >> -     imx7_get_boot_source(&src, &instance);
> >> +     get_boot_source(&src, &instance);
> >>
> >>       bootsource_set(src);
> >>       bootsource_set_instance(instance);
> >>  }
> >> +
> >> +#define IMX_BOOT_SAVE_LOC(soc)                                       \
> >> +     void soc##_boot_save_loc(void)                          \
> >> +     {                                                       \
> >> +             imx_boot_save_loc(soc##_get_boot_source);       \
> >> +     }
> >> +
> >> +IMX_BOOT_SAVE_LOC(imx25)
> >> +IMX_BOOT_SAVE_LOC(imx27)
> >> +IMX_BOOT_SAVE_LOC(imx35)
> >> +IMX_BOOT_SAVE_LOC(imx51)
> >> +IMX_BOOT_SAVE_LOC(imx53)
> >> +IMX_BOOT_SAVE_LOC(imx6)
> >> +IMX_BOOT_SAVE_LOC(imx7)
> >
> > I do not really like this patch. Yes, it saves a few lines of code, but
> > with the cost of making it less readable.
> >
> 
> Can you elaborate on what part is less readable? IMHO, replacing 68
> line of mostly copy pasted code with 17 of a generic function is a bit
> more than "saving a few lines of code", so I think it's worth trying
> to find a solution that would be acceptable from both perspectives:
> conciseness and readability.

I think it's bad when grepping for a function name only reveals the
callers, maybe the declaraion, but not the definition of a function.
Using macros to define functions is sometimes very effective (and done
in barebox at other places), but not very nice to look at.

> 
> Would replacing function pointer with a switch (imx_cpu_type) and
> expanding IMX_BOOT_SAVE_LOC() make this patch readable enough to be
> accepted?

Not sure what exactly you mean, but the following should be fine. With
a bit of violating the coding style this is not longer than your solution,
but without hiding the function names behind a macro:

void vf610_boot_save_loc(void) { boot_save_loc(vf610_get_boot_source); }

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 01/19] ARM: i.MX: boot: Coalesce copy-pasted code

[Andrey Smirnov]

On Wed, Apr 18, 2018 at 1:15 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> On Tue, Apr 17, 2018 at 08:20:46AM -0700, Andrey Smirnov wrote:
>> On Mon, Apr 16, 2018 at 11:58 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
>> > On Mon, Apr 16, 2018 at 12:31:39PM -0700, Andrey Smirnov wrote:
>> >> All of the instances of imx*_boot_save_loc() do exactly the same thing with
>> >> the only difference being SoC-specific imx*_get_boot_source
>> >> call. Convert the code into a generic function taking function pointer
>> >> + a macro to take care of the boilerplate.
>> >>
>> >> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
>> >> ---
>> >>  arch/arm/mach-imx/boot.c | 85 ++++++++++--------------------------------------
>> >>  1 file changed, 17 insertions(+), 68 deletions(-)
>> >>
>> >> +imx_boot_save_loc(void (*get_boot_source)(enum bootsource *, int *))
>> >>  {
>> >>       enum bootsource src = BOOTSOURCE_UNKNOWN;
>> >>       int instance = BOOTSOURCE_INSTANCE_UNKNOWN;
>> >>
>> >> -     imx7_get_boot_source(&src, &instance);
>> >> +     get_boot_source(&src, &instance);
>> >>
>> >>       bootsource_set(src);
>> >>       bootsource_set_instance(instance);
>> >>  }
>> >> +
>> >> +#define IMX_BOOT_SAVE_LOC(soc)                                       \
>> >> +     void soc##_boot_save_loc(void)                          \
>> >> +     {                                                       \
>> >> +             imx_boot_save_loc(soc##_get_boot_source);       \
>> >> +     }
>> >> +
>> >> +IMX_BOOT_SAVE_LOC(imx25)
>> >> +IMX_BOOT_SAVE_LOC(imx27)
>> >> +IMX_BOOT_SAVE_LOC(imx35)
>> >> +IMX_BOOT_SAVE_LOC(imx51)
>> >> +IMX_BOOT_SAVE_LOC(imx53)
>> >> +IMX_BOOT_SAVE_LOC(imx6)
>> >> +IMX_BOOT_SAVE_LOC(imx7)
>> >
>> > I do not really like this patch. Yes, it saves a few lines of code, but
>> > with the cost of making it less readable.
>> >
>>
>> Can you elaborate on what part is less readable? IMHO, replacing 68
>> line of mostly copy pasted code with 17 of a generic function is a bit
>> more than "saving a few lines of code", so I think it's worth trying
>> to find a solution that would be acceptable from both perspectives:
>> conciseness and readability.
>
> I think it's bad when grepping for a function name only reveals the
> callers, maybe the declaraion, but not the definition of a function.
> Using macros to define functions is sometimes very effective (and done
> in barebox at other places), but not very nice to look at.
>
>>
>> Would replacing function pointer with a switch (imx_cpu_type) and
>> expanding IMX_BOOT_SAVE_LOC() make this patch readable enough to be
>> accepted?
>
> Not sure what exactly you mean, but the following should be fine. With
> a bit of violating the coding style this is not longer than your solution,
> but without hiding the function names behind a macro:
>
> void vf610_boot_save_loc(void) { boot_save_loc(vf610_get_boot_source); }
>

That's what I meant by "expanding IMX_BOOT_SAVE_LOC()". I'll update
the patch and re-submit it.

Thanks,
Andrey Smirnov

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox