[V4,08/12] boot_constraint: Manage deferrable constraints

Message ID 88c7a0a6421d267c118f501ea1e920b04649002d.1509284255.git.viresh.kumar@linaro.org
State New
Headers show
Series
  • Untitled series #5557
Related show

Commit Message

Viresh Kumar Oct. 29, 2017, 1:48 p.m.
It is possible that some of the resources aren't available at the time
constraints are getting set and the boot constraints core will return
-EPROBE_DEFER for them. In order to retry adding the constraints at a
later point of time (after the resource is added and before any of its
users come up), this patch proposes two things:

- Each constraint is represented by a virtual platform device, so that
  it is re-probed again until the time all the dependencies aren't met.
  The platform device is removed along with the constraint, with help of
  the free_resources() callback.

- Enable early defer probing support by calling
  driver_enable_deferred_probe(), so that the core retries probing
  deferred devices every time any device is bound to a driver. This
  makes sure that the constraint is set before any of the users of the
  resources come up.

This is tested on ARM64 Hikey board where probe was deferred for a
device.

Tested-by: Rajendra Nayak <rnayak@codeaurora.org>

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

---
 drivers/base/dd.c                         |  12 ++
 drivers/boot_constraints/Makefile         |   2 +-
 drivers/boot_constraints/deferrable_dev.c | 235 ++++++++++++++++++++++++++++++
 include/linux/boot_constraint.h           |  14 ++
 4 files changed, 262 insertions(+), 1 deletion(-)
 create mode 100644 drivers/boot_constraints/deferrable_dev.c

-- 
2.15.0.rc1.236.g92ea95045093

Comments

Rob Herring Oct. 31, 2017, 4:20 p.m. | #1
On Sun, Oct 29, 2017 at 8:48 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> It is possible that some of the resources aren't available at the time

> constraints are getting set and the boot constraints core will return

> -EPROBE_DEFER for them. In order to retry adding the constraints at a

> later point of time (after the resource is added and before any of its

> users come up), this patch proposes two things:

>

> - Each constraint is represented by a virtual platform device, so that

>   it is re-probed again until the time all the dependencies aren't met.

>   The platform device is removed along with the constraint, with help of

>   the free_resources() callback.

>

> - Enable early defer probing support by calling

>   driver_enable_deferred_probe(), so that the core retries probing

>   deferred devices every time any device is bound to a driver. This

>   makes sure that the constraint is set before any of the users of the

>   resources come up.


What is the effect on boot time? It's highly platform dependent, but
the worst case could be pretty bad I think.

I don't see how this handles the case you mentioned where the amba
pclk gets disabled. It only works if the constraint device is added
before any others, but that is done with initcall level games.

Rob
Viresh Kumar Nov. 1, 2017, 2:29 a.m. | #2
On 31 October 2017 at 16:20, Rob Herring <robh+dt@kernel.org> wrote:
> What is the effect on boot time? It's highly platform dependent, but

> the worst case could be pretty bad I think.


Yeah, it can increase considerably here and I have plans for that, just
that i didn't wanted to get them in the first iteration to keep things simple.

What we can (should?) do is, that the boot constraint framework can hook
into other frameworks like regulators/clk/PM, etc, so that whenever a new
clk/regulator is added to those frameworks, they check for pending
requests from boot constraint framework. If found, they can call a callback
of the boot constraint framework which will set the constraints to the resource
before anyone else gets a chance. At that point we can remove the early
defer probing support that this patch is adding. And things would be quite fast
then.

> I don't see how this handles the case you mentioned where the amba

> pclk gets disabled. It only works if the constraint device is added

> before any others, but that is done with initcall level games.


Yeah, so as I said earlier, the basic idea is that these constraints must get
set before any user driver (for constrained devices) comes up. And the
only way to do that is by making sure the constraints get added at early
initcall levels. The same is done for all the three example drivers I have
added.

The amba-pclk thing isn't a issue then, as that stuff happens only at probe
and not when the amba device is created.

--
viresh
Greg KH Dec. 13, 2017, 9:53 a.m. | #3
On Sun, Oct 29, 2017 at 07:18:56PM +0530, Viresh Kumar wrote:
> It is possible that some of the resources aren't available at the time

> constraints are getting set and the boot constraints core will return

> -EPROBE_DEFER for them. In order to retry adding the constraints at a

> later point of time (after the resource is added and before any of its

> users come up), this patch proposes two things:

> 

> - Each constraint is represented by a virtual platform device, so that

>   it is re-probed again until the time all the dependencies aren't met.

>   The platform device is removed along with the constraint, with help of

>   the free_resources() callback.

> 

> - Enable early defer probing support by calling

>   driver_enable_deferred_probe(), so that the core retries probing

>   deferred devices every time any device is bound to a driver. This

>   makes sure that the constraint is set before any of the users of the

>   resources come up.

> 

> This is tested on ARM64 Hikey board where probe was deferred for a

> device.

> 

> Tested-by: Rajendra Nayak <rnayak@codeaurora.org>

> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

> ---

>  drivers/base/dd.c                         |  12 ++

>  drivers/boot_constraints/Makefile         |   2 +-

>  drivers/boot_constraints/deferrable_dev.c | 235 ++++++++++++++++++++++++++++++

>  include/linux/boot_constraint.h           |  14 ++

>  4 files changed, 262 insertions(+), 1 deletion(-)

>  create mode 100644 drivers/boot_constraints/deferrable_dev.c

> 

> diff --git a/drivers/base/dd.c b/drivers/base/dd.c

> index 4eec27fe2b2b..19eff5d08b9a 100644

> --- a/drivers/base/dd.c

> +++ b/drivers/base/dd.c

> @@ -228,6 +228,18 @@ void device_unblock_probing(void)

>  	driver_deferred_probe_trigger();

>  }

>  

> +/**

> + * driver_enable_deferred_probe() - Enable probing of deferred devices

> + *

> + * We don't want to get in the way when the bulk of drivers are getting probed

> + * and so deferred probe is disabled in the beginning. Enable it now because we

> + * need it.

> + */

> +void driver_enable_deferred_probe(void)

> +{

> +	driver_deferred_probe_enable = true;

> +}

> +

>  /**

>   * deferred_probe_initcall() - Enable probing of deferred devices

>   *

> diff --git a/drivers/boot_constraints/Makefile b/drivers/boot_constraints/Makefile

> index b7ade1a7afb5..a765094623a3 100644

> --- a/drivers/boot_constraints/Makefile

> +++ b/drivers/boot_constraints/Makefile

> @@ -1,3 +1,3 @@

>  # Makefile for device boot constraints

>  

> -obj-y := clk.o core.o pm.o supply.o

> +obj-y := clk.o deferrable_dev.o core.o pm.o supply.o

> diff --git a/drivers/boot_constraints/deferrable_dev.c b/drivers/boot_constraints/deferrable_dev.c

> new file mode 100644

> index 000000000000..04056f317aff

> --- /dev/null

> +++ b/drivers/boot_constraints/deferrable_dev.c

> @@ -0,0 +1,235 @@

> +/*

> + * Copyright (C) 2017 Linaro.

> + * Viresh Kumar <viresh.kumar@linaro.org>

> + *

> + * This file is released under the GPLv2.

> + */

> +

> +#define pr_fmt(fmt) "Boot Constraints: " fmt


Hey, you use this one!

But you shouldn't :)

> +/* This only creates platform devices for now */

> +static void add_deferrable_of_single(struct device_node *np,

> +				     struct dev_boot_constraint *constraints,

> +				     int count)

> +{

> +	struct device *dev;

> +	int ret;

> +

> +	if (!of_device_is_available(np))

> +		return;

> +

> +	ret = of_platform_bus_create(np, NULL, NULL, NULL, false);

> +	if (ret)

> +		return;

> +

> +	if (of_device_is_compatible(np, "arm,primecell")) {


Why is "arm,primecell" in the core code here?

> +		struct amba_device *adev = of_find_amba_device_by_node(np);

> +

> +		if (!adev) {

> +			pr_err("Failed to find amba dev: %s\n", np->full_name);


Never use pr_* when you have a valid struct device to use.  Don't you
have one from the struct device_node * passed in here?

> +			return;

> +		}

> +		dev = &adev->dev;

> +	} else {

> +		struct platform_device *pdev = of_find_device_by_node(np);

> +

> +		if (!pdev) {

> +			pr_err("Failed to find pdev: %s\n", np->full_name);


Same here.

thanks,

greg k-h
Viresh Kumar Dec. 13, 2017, 10:27 a.m. | #4
On 13-12-17, 10:53, Greg Kroah-Hartman wrote:
> On Sun, Oct 29, 2017 at 07:18:56PM +0530, Viresh Kumar wrote:

> > +static void add_deferrable_of_single(struct device_node *np,

> > +				     struct dev_boot_constraint *constraints,

> > +				     int count)

> > +{

> > +	struct device *dev;

> > +	int ret;

> > +

> > +	if (!of_device_is_available(np))

> > +		return;

> > +

> > +	ret = of_platform_bus_create(np, NULL, NULL, NULL, false);

> > +	if (ret)

> > +		return;

> > +

> > +	if (of_device_is_compatible(np, "arm,primecell")) {

> 

> Why is "arm,primecell" in the core code here?


All we need here is a struct device pointer to add constraints. But how we get
the device node depends on what bus type the device corresponds to. Currently
this only support amba and platform devices, but we may need to get spi, i2c,
etc later on.

How do you suggest to keep this stuff out of core here ? Are you asking me to
add a generic API in the OF core to find the struct device pointer using a node
pointer ?

> > +		struct amba_device *adev = of_find_amba_device_by_node(np);

> > +

> > +		if (!adev) {

> > +			pr_err("Failed to find amba dev: %s\n", np->full_name);

> 

> Never use pr_* when you have a valid struct device to use. 


Sure. I agree.

> Don't you

> have one from the struct device_node * passed in here?


The struct device_node doesn't contain a struct device * unfortunately. Will it
be acceptable to add one ? That will solve some controversial part of this
function for sure :)

-- 
viresh
Russell King - ARM Linux admin Dec. 13, 2017, 10:33 a.m. | #5
On Wed, Dec 13, 2017 at 03:57:07PM +0530, Viresh Kumar wrote:
> On 13-12-17, 10:53, Greg Kroah-Hartman wrote:

> > On Sun, Oct 29, 2017 at 07:18:56PM +0530, Viresh Kumar wrote:

> > > +static void add_deferrable_of_single(struct device_node *np,

> > > +				     struct dev_boot_constraint *constraints,

> > > +				     int count)

> > > +{

> > > +	struct device *dev;

> > > +	int ret;

> > > +

> > > +	if (!of_device_is_available(np))

> > > +		return;

> > > +

> > > +	ret = of_platform_bus_create(np, NULL, NULL, NULL, false);

> > > +	if (ret)

> > > +		return;

> > > +

> > > +	if (of_device_is_compatible(np, "arm,primecell")) {

> > 

> > Why is "arm,primecell" in the core code here?

> 

> All we need here is a struct device pointer to add constraints. But how we get

> the device node depends on what bus type the device corresponds to. Currently

> this only support amba and platform devices, but we may need to get spi, i2c,

> etc later on.

> 

> How do you suggest to keep this stuff out of core here ? Are you asking me to

> add a generic API in the OF core to find the struct device pointer using a node

> pointer ?


Why do we need this?  Why can't we lookup the "struct device" by DT
node, and then look at the device's bus type and decide what to do
from that?

Wouldn't a better solution be to use fwnode stuff for this, and
make the bus-type handling a property of the bus type itself,
pushing the bus specific code into the bus layer?

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
Viresh Kumar Dec. 13, 2017, 2:39 p.m. | #6
On 13-12-17, 10:33, Russell King - ARM Linux wrote:
> On Wed, Dec 13, 2017 at 03:57:07PM +0530, Viresh Kumar wrote:

> > On 13-12-17, 10:53, Greg Kroah-Hartman wrote:

> > > On Sun, Oct 29, 2017 at 07:18:56PM +0530, Viresh Kumar wrote:

> > > > +static void add_deferrable_of_single(struct device_node *np,

> > > > +				     struct dev_boot_constraint *constraints,

> > > > +				     int count)

> > > > +{

> > > > +	struct device *dev;

> > > > +	int ret;

> > > > +

> > > > +	if (!of_device_is_available(np))

> > > > +		return;

> > > > +

> > > > +	ret = of_platform_bus_create(np, NULL, NULL, NULL, false);

> > > > +	if (ret)

> > > > +		return;

> > > > +

> > > > +	if (of_device_is_compatible(np, "arm,primecell")) {

> > > 

> > > Why is "arm,primecell" in the core code here?

> > 

> > All we need here is a struct device pointer to add constraints. But how we get

> > the device node depends on what bus type the device corresponds to. Currently

> > this only support amba and platform devices, but we may need to get spi, i2c,

> > etc later on.

> > 

> > How do you suggest to keep this stuff out of core here ? Are you asking me to

> > add a generic API in the OF core to find the struct device pointer using a node

> > pointer ?

> 

> Why do we need this?  Why can't we lookup the "struct device" by DT

> node, and then look at the device's bus type and decide what to do

> from that?


My requirement is only to get the struct device * for the DT node and I don't
really need to get into the bus specific details at all. I was not sure if there
is a way to lookup for the "struct device" by its DT node currently and so
depended on helpers like of_find_device_by_node(). Can you please point me to
the routine (or the way we can traverse all devices) ?

> Wouldn't a better solution be to use fwnode stuff for this, and

> make the bus-type handling a property of the bus type itself,

> pushing the bus specific code into the bus layer?


As I said earlier, I don't really need to work at the bus level. I just need the
device structure and so that may not be required.

-- 
viresh

Patch

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 4eec27fe2b2b..19eff5d08b9a 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -228,6 +228,18 @@  void device_unblock_probing(void)
 	driver_deferred_probe_trigger();
 }
 
+/**
+ * driver_enable_deferred_probe() - Enable probing of deferred devices
+ *
+ * We don't want to get in the way when the bulk of drivers are getting probed
+ * and so deferred probe is disabled in the beginning. Enable it now because we
+ * need it.
+ */
+void driver_enable_deferred_probe(void)
+{
+	driver_deferred_probe_enable = true;
+}
+
 /**
  * deferred_probe_initcall() - Enable probing of deferred devices
  *
diff --git a/drivers/boot_constraints/Makefile b/drivers/boot_constraints/Makefile
index b7ade1a7afb5..a765094623a3 100644
--- a/drivers/boot_constraints/Makefile
+++ b/drivers/boot_constraints/Makefile
@@ -1,3 +1,3 @@ 
 # Makefile for device boot constraints
 
-obj-y := clk.o core.o pm.o supply.o
+obj-y := clk.o deferrable_dev.o core.o pm.o supply.o
diff --git a/drivers/boot_constraints/deferrable_dev.c b/drivers/boot_constraints/deferrable_dev.c
new file mode 100644
index 000000000000..04056f317aff
--- /dev/null
+++ b/drivers/boot_constraints/deferrable_dev.c
@@ -0,0 +1,235 @@ 
+/*
+ * Copyright (C) 2017 Linaro.
+ * Viresh Kumar <viresh.kumar@linaro.org>
+ *
+ * This file is released under the GPLv2.
+ */
+
+#define pr_fmt(fmt) "Boot Constraints: " fmt
+
+#include <linux/amba/bus.h>
+#include <linux/err.h>
+#include <linux/idr.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#include "core.h"
+
+static DEFINE_IDA(pdev_index);
+
+void driver_enable_deferred_probe(void);
+
+struct boot_constraint_pdata {
+	struct device *dev;
+	struct dev_boot_constraint constraint;
+	int probe_failed;
+	int index;
+};
+
+static void boot_constraint_remove(void *data)
+{
+	struct platform_device *pdev = data;
+	struct boot_constraint_pdata *pdata = dev_get_platdata(&pdev->dev);
+
+	ida_simple_remove(&pdev_index, pdata->index);
+	kfree(pdata->constraint.data);
+	platform_device_unregister(pdev);
+}
+
+/*
+ * A platform device is added for each and every constraint, to handle
+ * -EPROBE_DEFER properly.
+ */
+static int boot_constraint_probe(struct platform_device *pdev)
+{
+	struct boot_constraint_pdata *pdata = dev_get_platdata(&pdev->dev);
+	struct dev_boot_constraint_info info;
+	int ret;
+
+	if (WARN_ON(!pdata))
+		return -EINVAL;
+
+	info.constraint = pdata->constraint;
+	info.free_resources = boot_constraint_remove;
+	info.free_resources_data = pdev;
+
+	ret = dev_boot_constraint_add(pdata->dev, &info);
+	if (ret) {
+		if (ret == -EPROBE_DEFER)
+			driver_enable_deferred_probe();
+		else
+			pdata->probe_failed = ret;
+	}
+
+	return ret;
+}
+
+static struct platform_driver boot_constraint_driver = {
+	.driver = {
+		.name = "boot-constraints-dev",
+	},
+	.probe = boot_constraint_probe,
+};
+
+static int __init boot_constraint_init(void)
+{
+	return platform_driver_register(&boot_constraint_driver);
+}
+core_initcall(boot_constraint_init);
+
+static int boot_constraint_add_dev(struct device *dev,
+				   struct dev_boot_constraint *constraint)
+{
+	struct boot_constraint_pdata pdata = {
+		.dev = dev,
+		.constraint.type = constraint->type,
+	};
+	struct platform_device *pdev;
+	struct boot_constraint_pdata *pdev_pdata;
+	int size, ret;
+
+	switch (constraint->type) {
+	case DEV_BOOT_CONSTRAINT_CLK:
+		size = sizeof(struct dev_boot_constraint_clk_info);
+		break;
+	case DEV_BOOT_CONSTRAINT_PM:
+		size = 0;
+		break;
+	case DEV_BOOT_CONSTRAINT_SUPPLY:
+		size = sizeof(struct dev_boot_constraint_supply_info);
+		break;
+	default:
+		dev_err(dev, "%s: Constraint type (%d) not supported\n",
+			__func__, constraint->type);
+		return -EINVAL;
+	}
+
+	/* Will be freed from boot_constraint_remove() */
+	pdata.constraint.data = kmemdup(constraint->data, size, GFP_KERNEL);
+	if (!pdata.constraint.data)
+		return -ENOMEM;
+
+	ret = ida_simple_get(&pdev_index, 0, 256, GFP_KERNEL);
+	if (ret < 0) {
+		dev_err(dev, "failed to allocate index (%d)\n", ret);
+		goto free;
+	}
+
+	pdata.index = ret;
+
+	pdev = platform_device_register_data(NULL, "boot-constraints-dev", ret,
+					     &pdata, sizeof(pdata));
+	if (IS_ERR(pdev)) {
+		dev_err(dev, "%s: Failed to create pdev (%ld)\n", __func__,
+			PTR_ERR(pdev));
+		ret = PTR_ERR(pdev);
+		goto ida_remove;
+	}
+
+	/* Release resources if probe has failed */
+	pdev_pdata = dev_get_platdata(&pdev->dev);
+	if (pdev_pdata->probe_failed) {
+		ret = pdev_pdata->probe_failed;
+		goto remove_pdev;
+	}
+
+	return 0;
+
+remove_pdev:
+	platform_device_unregister(pdev);
+ida_remove:
+	ida_simple_remove(&pdev_index, pdata.index);
+free:
+	kfree(pdata.constraint.data);
+
+	return ret;
+}
+
+static int dev_boot_constraint_add_deferrable(struct device *dev,
+			struct dev_boot_constraint *constraints, int count)
+{
+	int ret, i;
+
+	for (i = 0; i < count; i++) {
+		ret = boot_constraint_add_dev(dev, &constraints[i]);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+/* This only creates platform devices for now */
+static void add_deferrable_of_single(struct device_node *np,
+				     struct dev_boot_constraint *constraints,
+				     int count)
+{
+	struct device *dev;
+	int ret;
+
+	if (!of_device_is_available(np))
+		return;
+
+	ret = of_platform_bus_create(np, NULL, NULL, NULL, false);
+	if (ret)
+		return;
+
+	if (of_device_is_compatible(np, "arm,primecell")) {
+		struct amba_device *adev = of_find_amba_device_by_node(np);
+
+		if (!adev) {
+			pr_err("Failed to find amba dev: %s\n", np->full_name);
+			return;
+		}
+		dev = &adev->dev;
+	} else {
+		struct platform_device *pdev = of_find_device_by_node(np);
+
+		if (!pdev) {
+			pr_err("Failed to find pdev: %s\n", np->full_name);
+			return;
+		}
+		dev = &pdev->dev;
+	}
+
+	ret = dev_boot_constraint_add_deferrable(dev, constraints, count);
+	if (ret)
+		dev_err(dev, "Failed to add boot constraint (%d)\n", ret);
+}
+
+/* Not all compatible device nodes may have boot constraints */
+static bool node_has_boot_constraints(struct device_node *np,
+				      struct dev_boot_constraint_of *oconst)
+{
+	int i;
+
+	if (!oconst->dev_names)
+		return true;
+
+	for (i = 0; i < oconst->dev_names_count; i++) {
+		if (!strcmp(oconst->dev_names[i], kbasename(np->full_name)))
+			return true;
+	}
+
+	return false;
+}
+
+void dev_boot_constraint_add_deferrable_of(struct dev_boot_constraint_of *oconst,
+					   int count)
+{
+	struct device_node *np;
+	int i;
+
+	for (i = 0; i < count; i++) {
+		for_each_compatible_node(np, NULL, oconst[i].compat) {
+			if (!node_has_boot_constraints(np, &oconst[i]))
+				continue;
+
+			add_deferrable_of_single(np, oconst[i].constraints,
+						 oconst[i].count);
+		}
+	}
+}
+EXPORT_SYMBOL_GPL(dev_boot_constraint_add_deferrable_of);
diff --git a/include/linux/boot_constraint.h b/include/linux/boot_constraint.h
index 637fe9d65536..c110b36e490f 100644
--- a/include/linux/boot_constraint.h
+++ b/include/linux/boot_constraint.h
@@ -35,6 +35,15 @@  struct dev_boot_constraint {
 	void *data;
 };
 
+struct dev_boot_constraint_of {
+	const char *compat;
+	struct dev_boot_constraint *constraints;
+	unsigned int count;
+
+	const char **dev_names;
+	unsigned int dev_names_count;
+};
+
 struct dev_boot_constraint_info {
 	struct dev_boot_constraint constraint;
 
@@ -47,12 +56,17 @@  struct dev_boot_constraint_info {
 int dev_boot_constraint_add(struct device *dev,
 			    struct dev_boot_constraint_info *info);
 void dev_boot_constraints_remove(struct device *dev);
+void dev_boot_constraint_add_deferrable_of(struct dev_boot_constraint_of *oconst,
+					   int count);
 #else
 static inline
 int dev_boot_constraint_add(struct device *dev,
 			    struct dev_boot_constraint_info *info)
 { return -EINVAL; }
 static inline void dev_boot_constraints_remove(struct device *dev) {}
+void dev_boot_constraint_add_deferrable_of(struct dev_boot_constraint_of *oconst,
+					   int count)
+{ }
 #endif /* CONFIG_DEV_BOOT_CONSTRAINTS */
 
 #endif /* _LINUX_BOOT_CONSTRAINT_H */