[PATCH v4 0/3] introduce board-driver support

[PATCH v4 0/3] introduce board-driver support

[Oleksij Rempel]

changes v4:
- rework devinfo to print DT node without child nodes

changes v3:
- move devinfo patch to the first place
- devinfo: provide better commit message 

changes v2:
- move root device registration to the of_probe()
- port one board as example
- fix issue with devinfo

Oleksij Rempel (3):
  devinfo: do not dump the device node for the root node
  of: base: register DT root as device
  ARM: embest-riotboard: port board file to the driver model

 arch/arm/boards/embest-riotboard/board.c | 18 ++++++++++++-----
 commands/devinfo.c                       |  2 +-
 drivers/of/base.c                        | 25 ++++++++++++++++++------
 include/of.h                             |  1 +
 4 files changed, 34 insertions(+), 12 deletions(-)

-- 
2.27.0


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

[PATCH v4 1/3] devinfo: do not dump the device node for the root node

[Oleksij Rempel]

Calling the devinfo against a device which is linked to some device tree
node will result a device tree dump of this node. For example:

barebox@Protonic PRTI6Q board:/ devinfo devinfo ldb.of
Bus: platform
Device node: /ldb
ldb {

        #address-cells = <0x1>;
        #size-cells = <0x0>;
        compatible = "fsl,imx6q-ldb", "fsl,imx53-ldb";
        gpr = <0x5>;
        status = "okay";
        clocks = <0x4 0x21 0x4 0x22 0x4 0x27 0x4 0x28 0x4 0x29 0x4 0x2a 0x4 0x87 0x4 0x88>;
        clock-names = "di0_pll", "di1_pll", "di0_sel", "di1_sel", "di2_sel", "di3_sel", "di0", "di1";
        lvds-channel@0 {
                #address-cells = <0x1>;
                #size-cells = <0x0>;
                reg = <0x0>;
                status = "okay";
                port@0 {
                        reg = <0x0>;
                        endpoint {
                                remote-endpoint = <0x6>;
                                phandle = <0x56>;
                        };
                };
                port@1 {
                        reg = <0x1>;
                        endpoint {
                                remote-endpoint = <0x7>;
                                phandle = <0x5a>;
                        };
                };
                port@2 {
                        reg = <0x2>;
                        endpoint {
                                remote-endpoint = <0x8>;
                                phandle = <0x60>;
                        };
                };
.........

Calling same command on a device which is linked to the root node of
device tree, for example "machine.of", will trigger a dump of complete
device tree. Since the same functionality is provided by the "of_dump"
command, it is better to limit devinfo to print only exactly requested
node, without printing the child nodes. After this patch we would get
following output:

barebox@Protonic PRTI6Q board:/ devinfo ldb.of
Bus: platform
Device node: /ldb
ldb {
        #address-cells = <0x1>;
        #size-cells = <0x0>;
        compatible = "fsl,imx6q-ldb", "fsl,imx53-ldb";
        gpr = <0x5>;
        status = "okay";
        clocks = <0x4 0x21 0x4 0x22 0x4 0x27 0x4 0x28 0x4 0x29 0x4 0x2a 0x4 0x87 0x4 0x88>;
        clock-names = "di0_pll", "di1_pll", "di0_sel", "di1_sel", "di2_sel", "di3_sel", "di0", "di1";
};

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 commands/devinfo.c |  2 +-
 drivers/of/base.c  | 20 ++++++++++++++------
 include/of.h       |  1 +
 3 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/commands/devinfo.c b/commands/devinfo.c
index 81956b1cc0..7036545cdb 100644
--- a/commands/devinfo.c
+++ b/commands/devinfo.c
@@ -101,7 +101,7 @@ static int do_devinfo(int argc, char *argv[])
 #ifdef CONFIG_OFDEVICE
 		if (dev->device_node) {
 			printf("Device node: %s\n", dev->device_node->full_name);
-			of_print_nodes(dev->device_node, 0);
+			of_print_node(dev->device_node, 0);
 		}
 #endif
 	}
diff --git a/drivers/of/base.c b/drivers/of/base.c
index 4c633bcd49..4754fcb98f 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1793,7 +1793,8 @@ int of_property_read_string_helper(const struct device_node *np,
 	return i <= 0 ? -ENODATA : i;
 }
 
-static void __of_print_nodes(struct device_node *node, int indent, const char *prefix)
+static void __of_print_nodes(struct device_node *node, int indent,
+			     const char *prefix, bool single)
 {
 	struct device_node *n;
 	struct property *p;
@@ -1815,8 +1816,10 @@ static void __of_print_nodes(struct device_node *node, int indent, const char *p
 		printf(";\n");
 	}
 
-	list_for_each_entry(n, &node->children, parent_list) {
-		__of_print_nodes(n, indent + 1, prefix);
+	if (!single) {
+		list_for_each_entry(n, &node->children, parent_list) {
+			__of_print_nodes(n, indent + 1, prefix, false);
+		}
 	}
 
 	printf("%s%*s};\n", prefix, indent * 8, "");
@@ -1824,7 +1827,12 @@ static void __of_print_nodes(struct device_node *node, int indent, const char *p
 
 void of_print_nodes(struct device_node *node, int indent)
 {
-	__of_print_nodes(node, indent, NULL);
+	__of_print_nodes(node, indent, NULL, false);
+}
+
+void of_print_node(struct device_node *node, int indent)
+{
+	__of_print_nodes(node, indent, NULL, true);
 }
 
 static void __of_print_property(struct property *p, int indent)
@@ -1934,14 +1942,14 @@ void of_diff(struct device_node *a, struct device_node *b, int indent)
 			of_diff(ca, cb, indent + 1);
 		} else {
 			of_print_parents(a, &printed);
-			__of_print_nodes(ca, indent, "-");
+			__of_print_nodes(ca, indent, "-", false);
 		}
 	}
 
 	for_each_child_of_node(b, cb) {
 		if (!of_get_child_by_name(a, cb->name)) {
 			of_print_parents(a, &printed);
-			__of_print_nodes(cb, indent, "+");
+			__of_print_nodes(cb, indent, "+", false);
 		}
 	}
 
diff --git a/include/of.h b/include/of.h
index 08bbeaf4d2..820a8ea0ed 100644
--- a/include/of.h
+++ b/include/of.h
@@ -104,6 +104,7 @@ static inline const void *of_property_get_value(struct property *pp)
 void of_print_property(const void *data, int len);
 void of_print_cmdline(struct device_node *root);
 
+void of_print_node(struct device_node *node, int indent);
 void of_print_nodes(struct device_node *node, int indent);
 void of_diff(struct device_node *a, struct device_node *b, int indent);
 int of_probe(void);
-- 
2.27.0


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

[PATCH v4 2/3] of: base: register DT root as device

[Oleksij Rempel]

A usual board file contains at least one of_machine_is_compatible().
Some of the have a rather long list with complicated version logic.

To avoid own implementation for driver management, register the root node
of device tree as platform device. So, the main platform bus can attach
proper board driver. After this patch a typical board.c file can reuse
existing driver infrastructure.

After this patch, you will be able to see all registered board drivers
with drvinfo as fallow:
...
board-embest-riot
board-protonic-imx6
    machine.of
...

With devinfo, you'll be able to get some board specific information,
if this is implemented:
barebox@Protonic PRTI6Q board:/ devinfo machine.of
Driver: board-protonic-imx6
Bus: platform
Parameters:
  boardid: 0 (type: uint32)
  boardrev: 1 (type: uint32)

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/of/base.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 4754fcb98f..b76e424da0 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -2141,6 +2141,7 @@ static void of_probe_memory(void)
 int of_probe(void)
 {
 	struct device_node *firmware;
+	struct device_d *dev;
 
 	if(!root_node)
 		return -ENODEV;
@@ -2157,6 +2158,10 @@ int of_probe(void)
 	if (firmware)
 		of_platform_populate(firmware, NULL, NULL);
 
+	dev = of_platform_device_create(root_node, NULL);
+	if (dev)
+		dev_set_name(dev, "%s.of", "machine");
+
 	of_clk_init(root_node, NULL);
 	of_platform_populate(root_node, of_default_bus_match_table, NULL);
 
-- 
2.27.0


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

[PATCH v4 3/3] ARM: embest-riotboard: port board file to the driver model

[Oleksij Rempel]

This patch can be used as example for the new board-driver
functionality.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 arch/arm/boards/embest-riotboard/board.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/arch/arm/boards/embest-riotboard/board.c b/arch/arm/boards/embest-riotboard/board.c
index 2e0cc9f0ab..746ecf0562 100644
--- a/arch/arm/boards/embest-riotboard/board.c
+++ b/arch/arm/boards/embest-riotboard/board.c
@@ -51,11 +51,8 @@ static int ar8035_phy_fixup(struct phy_device *dev)
 	return 0;
 }
 
-static int riotboard_device_init(void)
+static int riotboard_probe(struct device_d *dev)
 {
-	if (!of_machine_is_compatible("riot,imx6s-riotboard"))
-		return 0;
-
 	phy_register_fixup_for_uid(0x004dd072, 0xffffffef, ar8035_phy_fixup);
 
 	imx6_bbu_internal_mmc_register_handler("emmc", "/dev/mmc3.barebox",
@@ -65,4 +62,15 @@ static int riotboard_device_init(void)
 
 	return 0;
 }
-device_initcall(riotboard_device_init);
+
+static const struct of_device_id riotboard_of_match[] = {
+	{ .compatible = "riot,imx6s-riotboard" },
+	{},
+};
+
+static struct driver_d riotboard_driver = {
+	.name = "board-embest-riot",
+	.probe = riotboard_probe,
+	.of_compatible = DRV_OF_COMPAT(riotboard_of_match),
+};
+device_platform_driver(riotboard_driver);
-- 
2.27.0


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

Re: [PATCH v4 1/3] devinfo: do not dump the device node for the root node

[Lucas Stach]

Am Montag, den 20.07.2020, 08:39 +0200 schrieb Oleksij Rempel:
> Calling the devinfo against a device which is linked to some device tree
> node will result a device tree dump of this node. For example:
> 
> barebox@Protonic PRTI6Q board:/ devinfo devinfo ldb.of
> Bus: platform
> Device node: /ldb
> ldb {
> 
>         #address-cells = <0x1>;
>         #size-cells = <0x0>;
>         compatible = "fsl,imx6q-ldb", "fsl,imx53-ldb";
>         gpr = <0x5>;
>         status = "okay";
>         clocks = <0x4 0x21 0x4 0x22 0x4 0x27 0x4 0x28 0x4 0x29 0x4 0x2a 0x4 0x87 0x4 0x88>;
>         clock-names = "di0_pll", "di1_pll", "di0_sel", "di1_sel", "di2_sel", "di3_sel", "di0", "di1";
>         lvds-channel@0 {
>                 #address-cells = <0x1>;
>                 #size-cells = <0x0>;
>                 reg = <0x0>;
>                 status = "okay";
>                 port@0 {
>                         reg = <0x0>;
>                         endpoint {
>                                 remote-endpoint = <0x6>;
>                                 phandle = <0x56>;
>                         };
>                 };
>                 port@1 {
>                         reg = <0x1>;
>                         endpoint {
>                                 remote-endpoint = <0x7>;
>                                 phandle = <0x5a>;
>                         };
>                 };
>                 port@2 {
>                         reg = <0x2>;
>                         endpoint {
>                                 remote-endpoint = <0x8>;
>                                 phandle = <0x60>;
>                         };
>                 };
> .........
> 
> Calling same command on a device which is linked to the root node of
> device tree, for example "machine.of", will trigger a dump of complete
> device tree. Since the same functionality is provided by the "of_dump"
> command, it is better to limit devinfo to print only exactly requested
> node, without printing the child nodes. After this patch we would get
> following output:
> 
> barebox@Protonic PRTI6Q board:/ devinfo ldb.of
> Bus: platform
> Device node: /ldb
> ldb {
>         #address-cells = <0x1>;
>         #size-cells = <0x0>;
>         compatible = "fsl,imx6q-ldb", "fsl,imx53-ldb";
>         gpr = <0x5>;
>         status = "okay";
>         clocks = <0x4 0x21 0x4 0x22 0x4 0x27 0x4 0x28 0x4 0x29 0x4 0x2a 0x4 0x87 0x4 0x88>;
>         clock-names = "di0_pll", "di1_pll", "di0_sel", "di1_sel", "di2_sel", "di3_sel", "di0", "di1";
> };

Bad example, the port nodes are actually part of the LDB device and are
useful information when dumping the info of the LDB.

Maybe this wasn't clear enough, but in my last mail I said "don't dump
subnodes with a compatible" on purpose. If we just stop iterating when
the subnode has its own compatible, we still dump subnodes like the
port nodes, which are logically part of the parent OF node, but still
don't dump subnodes which likely have a device on their own.

Regards,
Lucas

> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  commands/devinfo.c |  2 +-
>  drivers/of/base.c  | 20 ++++++++++++++------
>  include/of.h       |  1 +
>  3 files changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/commands/devinfo.c b/commands/devinfo.c
> index 81956b1cc0..7036545cdb 100644
> --- a/commands/devinfo.c
> +++ b/commands/devinfo.c
> @@ -101,7 +101,7 @@ static int do_devinfo(int argc, char *argv[])
>  #ifdef CONFIG_OFDEVICE
>  		if (dev->device_node) {
>  			printf("Device node: %s\n", dev->device_node->full_name);
> -			of_print_nodes(dev->device_node, 0);
> +			of_print_node(dev->device_node, 0);
>  		}
>  #endif
>  	}
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 4c633bcd49..4754fcb98f 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -1793,7 +1793,8 @@ int of_property_read_string_helper(const struct device_node *np,
>  	return i <= 0 ? -ENODATA : i;
>  }
>  
> -static void __of_print_nodes(struct device_node *node, int indent, const char *prefix)
> +static void __of_print_nodes(struct device_node *node, int indent,
> +			     const char *prefix, bool single)
>  {
>  	struct device_node *n;
>  	struct property *p;
> @@ -1815,8 +1816,10 @@ static void __of_print_nodes(struct device_node *node, int indent, const char *p
>  		printf(";\n");
>  	}
>  
> -	list_for_each_entry(n, &node->children, parent_list) {
> -		__of_print_nodes(n, indent + 1, prefix);
> +	if (!single) {
> +		list_for_each_entry(n, &node->children, parent_list) {
> +			__of_print_nodes(n, indent + 1, prefix, false);
> +		}
>  	}
>  
>  	printf("%s%*s};\n", prefix, indent * 8, "");
> @@ -1824,7 +1827,12 @@ static void __of_print_nodes(struct device_node *node, int indent, const char *p
>  
>  void of_print_nodes(struct device_node *node, int indent)
>  {
> -	__of_print_nodes(node, indent, NULL);
> +	__of_print_nodes(node, indent, NULL, false);
> +}
> +
> +void of_print_node(struct device_node *node, int indent)
> +{
> +	__of_print_nodes(node, indent, NULL, true);
>  }
>  
>  static void __of_print_property(struct property *p, int indent)
> @@ -1934,14 +1942,14 @@ void of_diff(struct device_node *a, struct device_node *b, int indent)
>  			of_diff(ca, cb, indent + 1);
>  		} else {
>  			of_print_parents(a, &printed);
> -			__of_print_nodes(ca, indent, "-");
> +			__of_print_nodes(ca, indent, "-", false);
>  		}
>  	}
>  
>  	for_each_child_of_node(b, cb) {
>  		if (!of_get_child_by_name(a, cb->name)) {
>  			of_print_parents(a, &printed);
> -			__of_print_nodes(cb, indent, "+");
> +			__of_print_nodes(cb, indent, "+", false);
>  		}
>  	}
>  
> diff --git a/include/of.h b/include/of.h
> index 08bbeaf4d2..820a8ea0ed 100644
> --- a/include/of.h
> +++ b/include/of.h
> @@ -104,6 +104,7 @@ static inline const void *of_property_get_value(struct property *pp)
>  void of_print_property(const void *data, int len);
>  void of_print_cmdline(struct device_node *root);
>  
> +void of_print_node(struct device_node *node, int indent);
>  void of_print_nodes(struct device_node *node, int indent);
>  void of_diff(struct device_node *a, struct device_node *b, int indent);
>  int of_probe(void);


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

Re: [PATCH v4 2/3] of: base: register DT root as device

[Ahmad Fatoum]

Hello Oleksij,

On 7/20/20 8:39 AM, Oleksij Rempel wrote:
> A usual board file contains at least one of_machine_is_compatible().
> Some of the have a rather long list with complicated version logic.
> 
> To avoid own implementation for driver management, register the root node
> of device tree as platform device. So, the main platform bus can attach
> proper board driver. After this patch a typical board.c file can reuse
> existing driver infrastructure.
> 
> After this patch, you will be able to see all registered board drivers
> with drvinfo as fallow:

this patch breaks boot on the Linux Automation MC-1.
of_platform_device_create() uses of_device_make_bus_id() to generate
a name, but that function is a no-op for the root node.

This results in a platform device with dev->name == NULL, which
inside register_device() -> get_device_by_name_id() -> strcmp
leads to a NULL pointer dereference.

You should probably not use of_platform_device_create, but use
platform_device_register directly and set a suitable name
upfront in the struct device_d you initialize.

Cheers
Ahmad

> ...
> board-embest-riot
> board-protonic-imx6
>     machine.of
> ...
> 
> With devinfo, you'll be able to get some board specific information,
> if this is implemented:
> barebox@Protonic PRTI6Q board:/ devinfo machine.of
> Driver: board-protonic-imx6
> Bus: platform
> Parameters:
>   boardid: 0 (type: uint32)
>   boardrev: 1 (type: uint32)
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  drivers/of/base.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 4754fcb98f..b76e424da0 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -2141,6 +2141,7 @@ static void of_probe_memory(void)
>  int of_probe(void)
>  {
>  	struct device_node *firmware;
> +	struct device_d *dev;
>  
>  	if(!root_node)
>  		return -ENODEV;
> @@ -2157,6 +2158,10 @@ int of_probe(void)
>  	if (firmware)
>  		of_platform_populate(firmware, NULL, NULL);
>  
> +	dev = of_platform_device_create(root_node, NULL);
> +	if (dev)
> +		dev_set_name(dev, "%s.of", "machine");
> +
>  	of_clk_init(root_node, NULL);
>  	of_platform_populate(root_node, of_default_bus_match_table, NULL);
>  
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
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