diff mbox

[U-Boot,21/51] common: Extend board-specific DT fixup

Message ID 20170714125537.14895-22-mario.six@gdsys.cc
State Rejected, archived
Delegated to: Mario Six
Headers show

Commit Message

Mario Six July 14, 2017, 12:55 p.m. UTC 
Commit 2a79275 ("dm: Add callback to modify the device tree")
implemented a board-specific callback that can modify U-Boot's device
tree before relocation to accomodate a range of hardware variations of
certain boards.

However, this approach only turns out to be useful when the copy of the
device tree before relocation is actually writeable. If e.g. a device
boots out of a NOR flash, this scheme does not work, since the
modification of the flash's contents is not possible, and the unmodified
device tree is relocated and used by U-Boot.

To circumvent this problem, we split the modification process into two
phases.

In phase one we only collect information about the board by querying the
hardware (reading GPIO values, probing I2C chips, etc.), and store the
obtained information in a special, board-specific structure that is part
of the global data structure. This phase runs prior to relocation, and
utilizes the pre-relocation DM to query hardware information.

In phase two, we read the information back from the structure, and do
the actual manipulation of the device tree. This phase occurs *after*
the relocation of the U-Boot image, but before the driver model is
initialized from the device tree. Since the device tree lives in RAM
alongside the U-Boot image after relocation, the tree is definitely
writeable at this point.

Each phase is implemented by a board-specific callback function: phase
one by board_fix_fdt_get_info, which takes no arguments, and phase two
by board_fix_fdt_change, which takes the writeable device tree blob as
its sole argument. Since the structure where the gathered hardware
information is stored is necessarily board-dependent, we create a
include file where the boards utilizing the functionality can define
their individually needed structures.

This commit also adapts the controlcenterdc board's fixup function to
the new scheme.

Signed-off-by: Mario Six <mario.six@gdsys.cc>
---

 board/gdsys/a38x/controlcenterdc.c |  20 +++++-
 common/board_f.c                   |   6 +-
 common/board_r.c                   |  10 +++
 doc/driver-model/fdt-fixup.txt     | 129 ++++++++++++++++++++++++-------------
 include/asm-generic/global_data.h  |   7 ++
 include/common.h                   |   3 +-
 include/fdt_fixup.h                |   5 ++
 7 files changed, 130 insertions(+), 50 deletions(-)
 create mode 100644 include/fdt_fixup.h

Comments

Simon Glass July 19, 2017, 9:05 a.m. UTC | #1 
Hi Mario,

On 14 July 2017 at 05:55, Mario Six <mario.six@gdsys.cc> wrote:
> Commit 2a79275 ("dm: Add callback to modify the device tree")
> implemented a board-specific callback that can modify U-Boot's device
> tree before relocation to accomodate a range of hardware variations of
> certain boards.
>
> However, this approach only turns out to be useful when the copy of the
> device tree before relocation is actually writeable. If e.g. a device
> boots out of a NOR flash, this scheme does not work, since the
> modification of the flash's contents is not possible, and the unmodified
> device tree is relocated and used by U-Boot.
>
> To circumvent this problem, we split the modification process into two
> phases.
>
> In phase one we only collect information about the board by querying the
> hardware (reading GPIO values, probing I2C chips, etc.), and store the
> obtained information in a special, board-specific structure that is part
> of the global data structure. This phase runs prior to relocation, and
> utilizes the pre-relocation DM to query hardware information.
>
> In phase two, we read the information back from the structure, and do
> the actual manipulation of the device tree. This phase occurs *after*
> the relocation of the U-Boot image, but before the driver model is
> initialized from the device tree. Since the device tree lives in RAM
> alongside the U-Boot image after relocation, the tree is definitely
> writeable at this point.
>
> Each phase is implemented by a board-specific callback function: phase
> one by board_fix_fdt_get_info, which takes no arguments, and phase two
> by board_fix_fdt_change, which takes the writeable device tree blob as
> its sole argument. Since the structure where the gathered hardware
> information is stored is necessarily board-dependent, we create a
> include file where the boards utilizing the functionality can define
> their individually needed structures.
>
> This commit also adapts the controlcenterdc board's fixup function to
> the new schem

Thanks for the clear explanation.

I have two ideas:

- Can we move the call to the existing function to after reloc_fdt(),
when the DT is in RAM?

- Or can we move this board to use a livetree and adjust the DT using
livetree functions instead? That might be more efficient.

Regards,
Simon
Mario Six July 25, 2017, 8:40 a.m. UTC | #2 
Hi Simon,

On Wed, Jul 19, 2017 at 11:05 AM, Simon Glass <sjg@chromium.org> wrote:
> Hi Mario,
>
> On 14 July 2017 at 05:55, Mario Six <mario.six@gdsys.cc> wrote:
>> Commit 2a79275 ("dm: Add callback to modify the device tree")
>> implemented a board-specific callback that can modify U-Boot's device
>> tree before relocation to accomodate a range of hardware variations of
>> certain boards.
>>
>> However, this approach only turns out to be useful when the copy of the
>> device tree before relocation is actually writeable. If e.g. a device
>> boots out of a NOR flash, this scheme does not work, since the
>> modification of the flash's contents is not possible, and the unmodified
>> device tree is relocated and used by U-Boot.
>>
>> To circumvent this problem, we split the modification process into two
>> phases.
>>
>> In phase one we only collect information about the board by querying the
>> hardware (reading GPIO values, probing I2C chips, etc.), and store the
>> obtained information in a special, board-specific structure that is part
>> of the global data structure. This phase runs prior to relocation, and
>> utilizes the pre-relocation DM to query hardware information.
>>
>> In phase two, we read the information back from the structure, and do
>> the actual manipulation of the device tree. This phase occurs *after*
>> the relocation of the U-Boot image, but before the driver model is
>> initialized from the device tree. Since the device tree lives in RAM
>> alongside the U-Boot image after relocation, the tree is definitely
>> writeable at this point.
>>
>> Each phase is implemented by a board-specific callback function: phase
>> one by board_fix_fdt_get_info, which takes no arguments, and phase two
>> by board_fix_fdt_change, which takes the writeable device tree blob as
>> its sole argument. Since the structure where the gathered hardware
>> information is stored is necessarily board-dependent, we create a
>> include file where the boards utilizing the functionality can define
>> their individually needed structures.
>>
>> This commit also adapts the controlcenterdc board's fixup function to
>> the new schem
>
> Thanks for the clear explanation.
>
> I have two ideas:
>
> - Can we move the call to the existing function to after reloc_fdt(),
> when the DT is in RAM?
>

That might work, but I'm not sure if it was one of the scenarios which didn't
work for some reason that I tried out before. I'll have to test it.

> - Or can we move this board to use a livetree and adjust the DT using
> livetree functions instead? That might be more efficient.
>

That was actually plan A, but even after applying some of your recent livetree
patches (for ns16550 and such), the boot got stuck very early on, so I didn't
follow this option more than that. It would, of course, be the cleanest
solution, but considering the amount of drivers that would need conversion, and
the absence of actual DT manipulation functions (include/dm/write.h doesn't
exist yet), it's a pretty daunting task.

But I'll look into it.

> Regards,
> Simon
>

Best regards,

Mario
diff mbox

Patch

diff --git a/board/gdsys/a38x/controlcenterdc.c b/board/gdsys/a38x/controlcenterdc.c
index 32168d3576..05e18bac4c 100644
--- a/board/gdsys/a38x/controlcenterdc.c
+++ b/board/gdsys/a38x/controlcenterdc.c
@@ -236,7 +236,7 @@  int board_late_init(void)
 	return 0;
 }
 
-int board_fix_fdt(void *rw_fdt_blob)
+int board_fix_fdt_get_info(void)
 {
 	struct udevice *bus = NULL;
 	uint k;
@@ -254,7 +254,23 @@  int board_fix_fdt(void *rw_fdt_blob)
 		snprintf(name, 64,
 			 "/soc/internal-regs/i2c@11000/pca9698@%02x", k);
 
-		if (!dm_i2c_simple_probe(bus, k))
+		gd->board_fixup_data.chip_exists[k - 0x21] = dm_i2c_simple_probe(bus, k);
+	}
+
+	return 0;
+}
+
+int board_fix_fdt_change(void *rw_fdt_blob)
+{
+	struct of_board_fixup_data data = gd->board_fixup_data;
+	uint k;
+	char name[64];
+
+	for (k = 0x21; k <= 0x26; k++) {
+		snprintf(name, 64,
+			 "/soc/internal-regs/i2c@11000/pca9698@%02x", k);
+
+		if (!data.chip_exists[k - 0x21])
 			fdt_disable_by_ofname(rw_fdt_blob, name);
 	}
 
diff --git a/common/board_f.c b/common/board_f.c
index b258a1a73c..b835bd22bc 100644
--- a/common/board_f.c
+++ b/common/board_f.c
@@ -660,9 +660,9 @@  static int setup_reloc(void)
 }
 
 #ifdef CONFIG_OF_BOARD_FIXUP
-static int fix_fdt(void)
+static int fix_fdt_get_info(void)
 {
-	return board_fix_fdt((void *)gd->fdt_blob);
+	return board_fix_fdt_get_info();
 }
 #endif
 
@@ -887,7 +887,7 @@  static const init_fnc_t init_sequence_f[] = {
 #endif
 	display_new_sp,
 #ifdef CONFIG_OF_BOARD_FIXUP
-	fix_fdt,
+	fix_fdt_get_info,
 #endif
 	INIT_FUNC_WATCHDOG_RESET
 	reloc_fdt,
diff --git a/common/board_r.c b/common/board_r.c
index 199cadbed1..25f27f0843 100644
--- a/common/board_r.c
+++ b/common/board_r.c
@@ -694,6 +694,13 @@  static int run_main_loop(void)
 	return 0;
 }
 
+#ifdef CONFIG_OF_BOARD_FIXUP
+static int fix_fdt_change(void)
+{
+	return board_fix_fdt_change((void *)gd->fdt_blob);
+}
+#endif
+
 /*
  * Over time we hope to remove these functions with code fragments and
  * stub funtcions, and instead call the relevant function directly.
@@ -731,6 +738,9 @@  static init_fnc_t init_sequence_r[] = {
 #ifdef CONFIG_OF_LIVE
 	initr_of_live,
 #endif
+#ifdef CONFIG_OF_BOARD_FIXUP
+	fix_fdt_change,
+#endif
 #ifdef CONFIG_DM
 	initr_dm,
 #endif
diff --git a/doc/driver-model/fdt-fixup.txt b/doc/driver-model/fdt-fixup.txt
index 70344bd2c3..9a9467edd8 100644
--- a/doc/driver-model/fdt-fixup.txt
+++ b/doc/driver-model/fdt-fixup.txt
@@ -52,81 +52,122 @@  tree (at least after the relocation) would greatly simplify the solution of
 this problem, it is a non-negligible task to implement it, an a interim
 solution is needed to address the problem at least in the medium-term.
 
-Hence, we propose a solution to this problem by offering a board-specific
-call-back function, which is passed a writeable pointer to the device tree.
-This function is called before the device tree is relocated, and specifically
-before the main U-Boot's driver model is instantiated, hence the main U-Boot
-"sees" all modifications to the device tree made in this function. Furthermore,
-we have the pre-relocation driver model at our disposal at this stage, which
-means that we can query the hardware for the existence and variety of the
-components easily.
+Hence, we propose a solution to this problem by offering a set of two board-specific
+call-back functions:
+
+ * One function which is called prior to relocation that gathers information
+   about the device using the pre-relocation DM instance and stores the
+   collected information in a special data structure within the global data
+   section; using the pre-relocation driver model means that we can query the
+   hardware for the existence and variety of the components easily.
+ * A second function which is called just after relocation, but before the
+   post-relocation DM is instantiated; it is passed a writeable pointer to the
+   device tree, so that it can exectute the actual modifications of the device
+   tree based upon the information gathered by the first function.
+
+Notice that since the "modification function" is called prior to the DM
+instantiation, the main U-Boot "sees" all modifications to the device tree made
+by the function.
 
 2. Implementation
 -----------------
 
 To take advantage of the pre-relocation device tree manipulation mechanism,
-boards have to implement the function board_fix_fdt, which has the following
-signature:
+boards have to implement the functions board_fix_fdt_get_info and
+board_fix_fdt_change, which have the following signatures:
 
-int board_fix_fdt (void *rw_fdt_blob)
+int board_fix_fdt_get_info (void)
+int board_fix_fdt_change (void *rw_fdt_blob)
 
-The passed-in void pointer is a writeable pointer to the device tree, which can
-be used to manipulate the device tree using e.g. functions from
-include/fdt_support.h. The return value should either be 0 in case of
-successful execution of the device tree manipulation or something else for a
+The void pointer passed to board_fix_fdt_change is a writeable pointer to the
+device tree, which can be used to manipulate the device tree using e.g.
+functions from include/fdt_support.h. The return value for both functions
+should either be 0 in case of successful execution of the device information
+gathering and device tree manipulation respectively or something else for a
 failure. Note that returning a non-null value from the function will
 unrecoverably halt the boot process, as with any function from init_sequence_f
-(in common/board_f.c).
+(in common/board_f.c) or init_sequence_r (in common/board_r.c).  .
 
-Furthermore, the Kconfig option OF_BOARD_FIXUP has to be set for the function
+Furthermore, the Kconfig option OF_BOARD_FIXUP has to be set for the functions
 to be called:
 
 Device Tree Control
 -> [*] Board-specific manipulation of Device Tree
 
-+----------------------------------------------------------+
-| WARNING: The actual manipulation of the device tree has  |
-| to be the _last_ set of operations in board_fix_fdt!     |
-| Since the pre-relocation driver model does not adapt to  |
-| changes made to the device tree either, its references   |
-| into the device tree will be invalid after manipulating  |
-| it, and unpredictable behavior might occur when          |
-| functions that rely on them are executed!                |
-+----------------------------------------------------------+
+The transfer of the collected device information is accomplished via the
+board_fixup_data sub-structure in the global data structure. The layout of this
+structure is board-defined, and every user of the mechanism should define a
+suitable set of fields in include/fdt_fixup.h surrounded by preprocessor #ifdef
+instructions. For example, a board with the target Kconfig variable
+CONFIG_TARGET_SAMPLEBOARD that needs to check the existence of two I2C devices
+and has to determine a board variant identified by an int, might add the
+following member declarations for itself:
 
-Hence, the recommended layout of the board_fixup_fdt call-back function is the
-following:
+#ifdef CONFIG_TARGET_SAMPLEBOARD
+    bool chip1_present;
+    bool chip2_present;
+    int variant;
+#endif
 
-int board_fix_fdt(void *rw_fdt_blob)
+A schemetic implementation of the two functions might then look like the following:
+
+int board_fix_fdt_get_info(void)
 {
 	/* Collect information about device's hardware and store them in e.g.
-	   local variables */
+       local variables, e.g. chip1_present, chip2_present, variant. Return a
+       non-zero value if the information gathering failed. */
+
+    /* Store the collected information in the sub-structure, so they will be
+       available to board_fix_fdt_change: */
 
-	/* Do device tree manipulation using the values previously collected */
+    gd->board_fixup_data.chip1_present = chip1_present;
+    gd->board_fixup_data.chip2_present = chip2_present;
+    gd->board_fixup_data.variant = variant;
 
-	/* Return 0 on successful manipulation and non-zero otherwise */
+    return 0;
 }
 
-If this convention is kept, both an "additive" approach, meaning that nodes for
-detected components are added to the device tree, as well as a "subtractive"
-approach, meaning that nodes for absent components are removed from the tree,
-as well as a combination of both approaches should work.
+int board_fix_fdt_change(void *rw_fdt_blob)
+{
+    /* Read back the information from the sub-structure: */
+
+    bool chip1_present = gd->board_fixup_data.chip1_present;
+    bool chip2_present = gd->board_fixup_data.chip2_present;
+    int variant = gd->board_fixup_data.variant;
+
+    /* Employ device tree manipulation based on the information: */
+
+    if (chip1_present) {
+        ...
+    }
+
+    if (chip2_present) {
+        ...
+    }
+
+    switch(variant)
+        ...
+    }
+}
+
+This convention of split functions also enables that both an "additive"
+approach for DT manipulation, meaning that nodes for detected components are
+added to the device tree, as well as a "subtractive" approach, meaning that
+nodes for absent components are removed from the tree, as well as a combination
+of both approaches should work.
 
 3. Example
 ----------
 
-The controlcenterdc board (board/gdsys/a38x/controlcenterdc.c) features a
-board_fix_fdt function, in which six GPIO expanders (which might be present or
+The controlcenterdc board (board/gdsys/a38x/controlcenterdc.c) features
+board_fix_fdt functions, in which six GPIO expanders (which might be present or
 not, since they are on daughter boards) on a I2C bus are queried for, and
 subsequently deactivated in the device tree if they are not present.
 
-Note that the dm_i2c_simple_probe function does not use the device tree, hence
-it is safe to call it after the tree has already been manipulated.
-
 4. Work to be done
 ------------------
 
-* The application of device tree overlay should be possible in board_fixup_fdt,
-  but has not been tested at this stage.
+* The application of device tree overlay should be possible in
+  board_fixup_fdt_change, but has not been tested at this stage.
 
-2017-01-06, Mario Six <mario.six@gdsys.cc>
+2017-07-04, Mario Six <mario.six@gdsys.cc>
diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h
index fb90be9d3e..731a4163a9 100644
--- a/include/asm-generic/global_data.h
+++ b/include/asm-generic/global_data.h
@@ -24,6 +24,10 @@ 
 #include <membuff.h>
 #include <linux/list.h>
 
+#ifdef CONFIG_OF_BOARD_FIXUP
+#include <fdt_fixup.h>
+#endif
+
 typedef struct global_data {
 	bd_t *bd;
 	unsigned long flags;
@@ -75,6 +79,9 @@  typedef struct global_data {
 #ifdef CONFIG_OF_LIVE
 	struct device_node *of_root;
 #endif
+#ifdef CONFIG_OF_BOARD_FIXUP
+	struct of_board_fixup_data board_fixup_data;
+#endif
 	struct jt_funcs *jt;		/* jump table */
 	char env_buf[32];		/* buffer for getenv() before reloc. */
 #ifdef CONFIG_TRACE
diff --git a/include/common.h b/include/common.h
index 751665f8a4..80a004d4b4 100644
--- a/include/common.h
+++ b/include/common.h
@@ -416,7 +416,8 @@  extern ssize_t spi_write (uchar *, int, uchar *, int);
 
 /* $(BOARD)/$(BOARD).c */
 int board_early_init_f (void);
-int board_fix_fdt (void *rw_fdt_blob); /* manipulate the U-Boot fdt before its relocation */
+int board_fix_fdt_get_info (void); /* manipulate the U-Boot fdt before its relocation */
+int board_fix_fdt_change (void *rw_fdt_blob); /* manipulate the U-Boot fdt before its relocation */
 int board_late_init (void);
 int board_postclk_init (void); /* after clocks/timebase, before env/serial */
 int board_early_init_r (void);
diff --git a/include/fdt_fixup.h b/include/fdt_fixup.h
new file mode 100644
index 0000000000..9390f0a633
--- /dev/null
+++ b/include/fdt_fixup.h
@@ -0,0 +1,5 @@ 
+struct of_board_fixup_data {
+#ifdef CONFIG_TARGET_CONTROLCENTERDC
+	bool chip_exists[6];
+#endif
+};