Re: [PATCH 15/18] common: add initial barebox deep-probe support

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

On 9/28/20 5:50 PM, Marco Felsch wrote:
> The barebox 'deep probe' or 'probe on demand' mechanism is the answer of
> unwanted -EPROBE_DEFER failures. The EPROBE_DEFER error code was
> introduced by commit ab3da15bc14c ("base: Introduce deferred probing")
> and since then it causes a few problems.
> 
> The error is returned if either the device is not yet present or the
> driver is not yet registered. This makes sense on linux systems where
> modules and hot-plug devices are used very often but not for barebox.
> The module support is rarely used and devices aren't hot pluggable.
> 
> The current barebox behaviour populates all devices before the drivers
> are registered so all devices are present during the driver
> registration. So the driver probe() function gets called immediately
> after the driver registration and causes the -EPROBE_DEFER error if this
> driver depends on an other not yet registered driver.
> 
> To get rid of the EPROBE_DEFER error code we need to reorder the device
> population and the driver registration. All drivers must be registered
> first. In an ideal world all driver can be registered by the same
> initcall level. Then devices are getting populated which causes calling
> the driver probe() function but this time resources/devices are created
> on demand if not yet available.
> 
> Dependencies between devices are normally expressed as references to
> other device nodes. With deep probe barebox provides helper functions
> which take a device node and probe the device behind that node if
> necessary. This means instead of returning -EPROBE_DEFER, we can now
> make the desired resources available once we need them.
> 
> If the resource can't be created we are returning -ENODEV since we are
> not supporting hot-plugging. Dropping EPROBE_DEFER is the long-term
> goal, avoid initcall shifting is the short-term goal.
> 
> Call it deep-probe since the on-demand device creation can greate very
> deep stacks. This commit adds the initial support for: spi, i2c, reset,
> regulator and clk resource on-demand creation. The deep-probe mechanism
> must be enabled for each board to avoid breaking changes using
> deep_probe_enable(). This can be changed later after all boards are
> converted to the new mechanism.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
>  common/Makefile          |   1 +
>  common/deep-probe.c      |  39 +++++++++++++++
>  drivers/base/driver.c    |  11 ++++-
>  drivers/clk/clk.c        |   5 ++
>  drivers/i2c/i2c.c        |   6 +++
>  drivers/of/base.c        |  13 ++++-
>  drivers/of/platform.c    | 101 ++++++++++++++++++++++++++++++++++++++-
>  drivers/regulator/core.c |   6 +++
>  drivers/reset/core.c     |   5 ++
>  drivers/spi/spi.c        |   2 +
>  include/deep-probe.h     |  17 +++++++
>  include/of.h             |  37 +++++++++++++-
>  12 files changed, 238 insertions(+), 5 deletions(-)
>  create mode 100644 common/deep-probe.c
>  create mode 100644 include/deep-probe.h
> 
> diff --git a/common/Makefile b/common/Makefile
> index c3ae3ca1b9..8525240422 100644
> --- a/common/Makefile
> +++ b/common/Makefile
> @@ -3,6 +3,7 @@ obj-y				+= memory_display.o
>  pbl-$(CONFIG_PBL_CONSOLE)	+= memory_display.o
>  obj-y				+= clock.o
>  obj-y				+= console_common.o
> +obj-y				+= deep-probe.o
>  obj-y				+= startup.o
>  obj-y				+= misc.o
>  obj-pbl-y			+= memsize.o
> diff --git a/common/deep-probe.c b/common/deep-probe.c
> new file mode 100644
> index 0000000000..05ce4352b1
> --- /dev/null
> +++ b/common/deep-probe.c
> @@ -0,0 +1,39 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <common.h>
> +#include <deep-probe.h>

Shouldn't this rather be under drivers/of/ ?
maybe also name it something starting with of_ ?

> +#include <of.h>
> +
> +struct deep_probe_entry {
> +	struct list_head entry;
> +	const char *compatible;
> +};
> +
> +static LIST_HEAD(boards);
> +
> +int deep_probe_add_board(const char *machine)
> +{
> +	struct deep_probe_entry *new_machine;
> +
> +	new_machine = xzalloc(sizeof(*new_machine));
> +	if (!new_machine)
> +		return -ENOMEM;

xzalloc can't fail.

> +
> +	new_machine->compatible = machine;
> +	list_add(&new_machine->entry, &boards);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(deep_probe_add_board);
> +
> +bool deep_probe_is_supported(void)
> +{
> +	struct deep_probe_entry *machine;
> +
> +	list_for_each_entry(machine, &boards, entry) {
> +		if (of_machine_is_compatible(machine->compatible))
> +			return true;
> +	}
> +	return false;
> +}
> +EXPORT_SYMBOL_GPL(deep_probe_is_supported);
> diff --git a/drivers/base/driver.c b/drivers/base/driver.c
> index 412db6c406..b797655c15 100644
> --- a/drivers/base/driver.c
> +++ b/drivers/base/driver.c
> @@ -21,6 +21,7 @@
>  
>  #include <common.h>
>  #include <command.h>
> +#include <deep-probe.h>
>  #include <driver.h>
>  #include <malloc.h>
>  #include <console.h>
> @@ -95,7 +96,15 @@ int device_probe(struct device_d *dev)
>  	if (ret == -EPROBE_DEFER) {
>  		list_del(&dev->active);
>  		list_add(&dev->active, &deferred);
> -		dev_dbg(dev, "probe deferred\n");
> +
> +		/*
> +		 * -EPROBE_DEFER should never appear on a deep-probe machine so
> +		 * inform the user immediately.
> +		 */
> +		if (deep_probe_is_supported())
> +			dev_warn(dev, "probe deferred\n");
> +		else
> +			dev_dbg(dev, "probe deferred\n");
>  		return ret;
>  	}
>  
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index b04d44593b..218317d00d 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -437,9 +437,14 @@ EXPORT_SYMBOL_GPL(of_clk_del_provider);
>  
>  struct clk *of_clk_get_from_provider(struct of_phandle_args *clkspec)
>  {
> +	struct device_d *dev;
>  	struct of_clk_provider *provider;
>  	struct clk *clk = ERR_PTR(-EPROBE_DEFER);
>  
> +	dev = of_device_create_on_demand(clkspec->np);
> +	if (IS_ERR(dev))
> +		return ERR_CAST(dev);
> +
>  	/* Check if we have such a provider in our array */
>  	list_for_each_entry(provider, &of_clk_providers, link) {
>  		if (provider->node == clkspec->np)
> diff --git a/drivers/i2c/i2c.c b/drivers/i2c/i2c.c
> index 57d8c7017f..dbc0bd4f0b 100644
> --- a/drivers/i2c/i2c.c
> +++ b/drivers/i2c/i2c.c
> @@ -406,6 +406,7 @@ static struct i2c_client *i2c_new_device(struct i2c_adapter *adapter,
>  		return NULL;
>  	}
>  	client->dev.info = i2c_info;
> +	chip->of_node->dev = &client->dev;

Are you sure, you can always assume of_node != NULL here?

>  
>  	return client;
>  }
> @@ -547,8 +548,13 @@ struct i2c_adapter *i2c_get_adapter(int busnum)
>  
>  struct i2c_adapter *of_find_i2c_adapter_by_node(struct device_node *node)
>  {
> +	struct device_d *dev;
>  	struct i2c_adapter *adap;
>  
> +	dev = of_device_create_on_demand(node);
> +	if (IS_ERR(dev))
> +		return ERR_CAST(dev);
> +
>  	for_each_i2c_adapter(adap)
>  		if (adap->dev.device_node == node)
>  			return adap;
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 861871b221..cea6d5b1aa 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -15,6 +15,7 @@
>   * GNU General Public License for more details.
>   */
>  #include <common.h>
> +#include <deep-probe.h>
>  #include <of.h>
>  #include <of_address.h>
>  #include <errno.h>
> @@ -1567,6 +1568,15 @@ int of_set_root_node(struct device_node *node)
>  	return 0;
>  }
>  
> +static int barebox_of_populate(void)
> +{
> +	if (IS_ENABLED(CONFIG_OFDEVICE) && deep_probe_is_supported())
> +		of_probe();
> +
> +	return 0;
> +}
> +of_populate_initcall(barebox_of_populate);
> +
>  void barebox_register_of(struct device_node *root)
>  {
>  	if (root_node)
> @@ -1577,7 +1587,8 @@ void barebox_register_of(struct device_node *root)
>  
>  	if (IS_ENABLED(CONFIG_OFDEVICE)) {
>  		of_clk_init(root, NULL);
> -		of_probe();
> +		if (!deep_probe_is_supported())
> +			of_probe();
>  	}
>  }
>  
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index 21c7cce1a5..c837adf8e8 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -15,6 +15,7 @@
>   * GNU General Public License for more details.
>   */
>  #include <common.h>
> +#include <deep-probe.h>
>  #include <malloc.h>
>  #include <of.h>
>  #include <of_address.h>
> @@ -29,6 +30,11 @@
>  struct device_d *of_find_device_by_node(struct device_node *np)
>  {
>  	struct device_d *dev;
> +
> +	dev = of_device_create_on_demand(np);
> +	if (IS_ERR(dev))
> +		return NULL;
> +
>  	for_each_device(dev)
>  		if (dev->device_node == np)
>  			return dev;
> @@ -106,6 +112,9 @@ struct device_d *of_platform_device_create(struct device_node *np,
>  	if (!of_device_is_available(np))
>  		return NULL;
>  
> +	if (np->dev)
> +		return np->dev;
> +

Unsure if this is correct. Seek down to the "check if all address resources match" comment.
It's possible that devices are registered multiple times and are coalesced later on.
I can't say for sure though, where we are doing this at the moment.

>  	/* count the io resources */
>  	if (of_can_translate_address(np))
>  		while (of_address_to_resource(np, num_reg, &temp_res) == 0)
> @@ -170,8 +179,10 @@ struct device_d *of_platform_device_create(struct device_node *np,
>  		(num_reg) ? &dev->resource[0].start : &resinval);
>  
>  	ret = platform_device_register(dev);
> -	if (!ret)
> +	if (!ret) {
> +		np->dev = dev;
>  		return dev;
> +	}
>  
>  	free(dev);
>  	if (num_reg)
> @@ -252,6 +263,9 @@ static struct device_d *of_amba_device_create(struct device_node *np)
>  	if (!of_device_is_available(np))
>  		return NULL;
>  
> +	if (np->dev)
> +		return np->dev;
> +
>  	dev = xzalloc(sizeof(*dev));
>  
>  	/* setup generic device info */
> @@ -275,6 +289,8 @@ static struct device_d *of_amba_device_create(struct device_node *np)
>  	if (ret)
>  		goto amba_err_free;
>  
> +	np->dev = &dev->dev;
> +
>  	return &dev->dev;
>  
>  amba_err_free:
> @@ -364,3 +380,86 @@ int of_platform_populate(struct device_node *root,
>  	return rc;
>  }
>  EXPORT_SYMBOL_GPL(of_platform_populate);
> +
> +struct device_d *of_device_create_on_demand(struct device_node *np)
> +{
> +	struct device_node *parent;
> +	struct device_d *parent_dev, *dev;
> +
> +	if (!deep_probe_is_supported())
> +		return NULL;
> +
> +	parent = of_get_parent(np);
> +	if (!parent)
> +		return NULL;
> +
> +	/* Create all parent devices needed for the requested device */
> +	parent_dev = parent->dev ? : of_device_create_on_demand(parent);
> +	if (IS_ERR(parent_dev))
> +		return parent_dev;
> +
> +	/*
> +	 * Parent devices like i2c/spi controllers are populating their own
> +	 * devices. So it can be that the requested device already exist after
> +	 * the parent device creation.
> +	 */
> +	if (np->dev)
> +		return np->dev;
> +
> +	pr_debug("%s: Create %s (%s) on demand\n", __func__,
> +		 np->name, np->full_name);
> +
> +	if (of_device_is_compatible(np, "arm,primecell"))
> +		dev = of_amba_device_create(np);
> +	else
> +		dev = of_platform_device_create(np, parent_dev);
> +
> +	return dev ? : ERR_PTR(-ENODEV);
> +}
> +EXPORT_SYMBOL_GPL(of_device_create_on_demand);
> +
> +struct device_d *of_device_create_on_demand_by_alias(const char *alias)
> +{
> +	struct device_node *dev_node;
> +
> +	dev_node = of_find_node_by_alias(NULL, alias);
> +	return of_device_create_on_demand(dev_node);
> +}
> +EXPORT_SYMBOL_GPL(of_device_create_on_demand_by_alias);
> +
> +struct device_d *
> +of_device_create_on_demand_by_dev_id(struct device_node *np,
> +				     const struct of_device_id *ids)
> +{

I find it surprising that this works recursively, while the previous
by_alias doesn't

> +	struct device_node *child;
> +
> +	if (of_match_node(ids, np))
> +		return of_device_create_on_demand(np);
> +
> +	for_each_child_of_node(np, child) {
> +		struct device_d *dev;
> +
> +		dev = of_device_create_on_demand_by_dev_id(child, ids);
> +		if (!IS_ERR(dev))
> +			return dev;
> +	}
> +
> +	return ERR_PTR(-ENODEV);
> +}
> +EXPORT_SYMBOL_GPL(of_device_create_on_demand_by_dev_id);
> +
> +int of_devices_create_on_demand_by_property(const char *property_name)
> +{
> +	struct device_node *node;
> +
> +	for_each_node_with_property(node, property_name) {
> +		struct device_d *dev;
> +
> +		dev = of_device_create_on_demand(node);
> +		if (IS_ERR(dev))
> +			return PTR_ERR(dev);
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(of_devices_create_on_demand_by_property);
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index 6ea21a4609..25973acaf0 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -175,6 +175,7 @@ int of_regulator_register(struct regulator_dev *rd, struct device_node *node)
>  		return PTR_ERR(ri);
>  
>  	ri->node = node;
> +	node->dev = rd->dev;
>  
>  	of_property_read_u32(node, "regulator-enable-ramp-delay",
>  			&ri->enable_time_us);
> @@ -188,6 +189,7 @@ int of_regulator_register(struct regulator_dev *rd, struct device_node *node)
>  
>  static struct regulator_internal *of_regulator_get(struct device_d *dev, const char *supply)
>  {
> +	struct device_d *dev_ondemand;
>  	char *propname;
>  	struct regulator_internal *ri;
>  	struct device_node *node;
> @@ -222,6 +224,10 @@ static struct regulator_internal *of_regulator_get(struct device_d *dev, const c
>  		goto out;
>  	}
>  
> +	dev_ondemand = of_device_create_on_demand(node);
> +	if (IS_ERR(dev_ondemand))
> +		return ERR_CAST(dev_ondemand);
> +
>  	list_for_each_entry(ri, &regulator_list, list) {
>  		if (ri->node == node) {
>  			dev_dbg(dev, "Using %s regulator from %s\n",
> diff --git a/drivers/reset/core.c b/drivers/reset/core.c
> index 99b9c80655..874de11b0c 100644
> --- a/drivers/reset/core.c
> +++ b/drivers/reset/core.c
> @@ -153,6 +153,7 @@ static struct reset_control *of_reset_control_get(struct device_node *node,
>  	struct reset_control *rstc = ERR_PTR(-ENODEV);
>  	struct reset_controller_dev *r, *rcdev;
>  	struct of_phandle_args args;
> +	struct device_d *dev;
>  	int index = 0;
>  	int rstc_id;
>  	int ret;
> @@ -168,6 +169,10 @@ static struct reset_control *of_reset_control_get(struct device_node *node,
>  	if (ret)
>  		return ERR_PTR(ret);
>  
> +	dev = of_device_create_on_demand(args.np);
> +	if (IS_ERR(dev))
> +		return ERR_CAST(dev);
> +
>  	rcdev = NULL;
>  	list_for_each_entry(r, &reset_controller_list, list) {
>  		if (args.np == r->of_node) {
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index 8421d9d7c1..d1d3bdcc41 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -107,6 +107,8 @@ struct spi_device *spi_new_device(struct spi_controller *ctrl,
>  	if (status)
>  		goto fail;
>  
> +	chip->device_node->dev = &proxy->dev;
> +
>  	return proxy;
>  fail:
>  	free(proxy);
> diff --git a/include/deep-probe.h b/include/deep-probe.h
> new file mode 100644
> index 0000000000..f9a1f61fde
> --- /dev/null
> +++ b/include/deep-probe.h
> @@ -0,0 +1,17 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#ifndef __DEEP_PROBE_H
> +#define __DEEP_PROBE_H
> +
> +#include <linux/types.h>
> +
> +int deep_probe_add_board(const char *machine);
> +bool deep_probe_is_supported(void);
> +
> +#define deep_probe_enable(func,board)				\
> +	static int __init func##_deep_probe_register(void)	\
> +	{							\
> +		return deep_probe_add_board(board);		\
> +	}							\
> +	pure_initcall(func##_deep_probe_register)
> +
> +#endif /* __DEEP_PROBE_H */
> diff --git a/include/of.h b/include/of.h
> index 1b3ceaff40..0f8d6f7546 100644
> --- a/include/of.h
> +++ b/include/of.h
> @@ -35,6 +35,7 @@ struct device_node {
>  	struct list_head parent_list;
>  	struct list_head list;
>  	phandle phandle;
> +	struct device_d *dev;
>  };
>  
>  struct of_device_id {
> @@ -266,6 +267,13 @@ extern struct device_d *of_device_enable_and_register_by_name(const char *name);
>  extern struct device_d *of_device_enable_and_register_by_alias(
>  							const char *alias);
>  
> +extern struct device_d *of_device_create_on_demand(struct device_node *np);
> +extern struct device_d *of_device_create_on_demand_by_alias(const char *alias);
> +extern struct device_d *
> +of_device_create_on_demand_by_dev_id(struct device_node *np,
> +				     const struct of_device_id *ids);
> +extern int of_devices_create_on_demand_by_property(const char *property_name);
> +
>  struct cdev *of_parse_partition(struct cdev *cdev, struct device_node *node);
>  int of_parse_partitions(struct cdev *cdev, struct device_node *node);
>  int of_partitions_register_fixup(struct cdev *cdev);
> @@ -331,12 +339,37 @@ static inline int of_set_root_node(struct device_node *node)
>  	return -ENOSYS;
>  }
>  
> -static inline struct device_d *of_platform_device_create(struct device_node *np,
> -							 struct device_d *parent)
> +static inline struct device_d *
> +of_platform_device_create(struct device_node *np, struct device_d *parent)
> +{
> +	return NULL;
> +}
> +
> +static inline struct device_d *
> +of_device_create_on_demand(struct device_node *np);

Extra semicolon. Could you compile test without CONFIG_OFTREE?

> +{
> +	return NULL;
> +}
> +
> +static inline struct device_d *
> +of_device_create_on_demand_by_alias(struct device_node *np);
>  {
>  	return NULL;
>  }
>  
> +static inline struct device_d *
> +of_device_create_on_demand_by_dev_id(struct device_node *np,
> +				     const struct of_device_id *ids)
> +{
> +	return NULL;
> +}
> +
> +static inline int
> +of_devices_create_on_demand_by_property(const char *property_name)
> +{
> +	return 0;
> +}
> +
>  static inline int of_bus_n_addr_cells(struct device_node *np)
>  {
>  	return 0;
> 

-- 
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