* [PATCH v2 1/8] of: platform: Keep track of populated platform devices
2020-09-30 8:47 [PATCH v2 0/8] Barebox Deep-Probe Marco Felsch
@ 2020-09-30 8:47 ` Marco Felsch
2020-10-02 5:15 ` Sascha Hauer
2020-09-30 8:47 ` [PATCH v2 2/8] of: base: move memory init from DT to initcall Marco Felsch
` (6 subsequent siblings)
7 siblings, 1 reply; 20+ messages in thread
From: Marco Felsch @ 2020-09-30 8:47 UTC (permalink / raw)
To: barebox
After checking the linux history I found no state where it was explicite
allowed to register a platform device twice. In the early days there was
an bug and in some cases linux did populated the same device again. This
was fixed in 2014 by commit c6e126de43e7 ("of: Keep track of populated
platform devices") and since then this wasn't possible anymore.
The for_each_device() solution had two issues:
1) We are looping over all devices each time
of_platform_device_create() gets called.
2) The logic around 'if (!dev->resource)' then 'continue' seems to be
broken. This suggest that we can populate a device without
resource(s) first and adding resource(s) later which isn't the case.
Instead the current logic will populate the same device again but
this time (maybe) with resources. So the caller can add the same
device as many times as possible without resources.
3) The device_node gets modified if the new device_node to add has the
same resources region.
Add a device reference to the device_node to track an registered device
seems to:
1) Simplify the code.
2) Align the logic with the current linux state with the exception that
we are returning the already created device.
3) Avoid looping over each device all the time.
4) Fix the memory leak which can occure if of_platform_device_create()
gets called for the same device without any resources.
I added the missing check for amba devices too while on it.
Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
v2:
- new patch
drivers/of/platform.c | 51 +++++++++++++++++--------------------------
include/of.h | 1 +
2 files changed, 21 insertions(+), 31 deletions(-)
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 21c7cce1a5..01de6f98af 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -101,11 +101,18 @@ struct device_d *of_platform_device_create(struct device_node *np,
struct device_d *dev;
struct resource *res = NULL, temp_res;
resource_size_t resinval;
- int i, j, ret, num_reg = 0, match;
+ int i, ret, num_reg = 0;
if (!of_device_is_available(np))
return NULL;
+ /*
+ * Linux uses the OF_POPULATED flag to skip already populated/created
+ * devices.
+ */
+ if (np->dev)
+ return np->dev;
+
/* count the io resources */
if (of_can_translate_address(np))
while (of_address_to_resource(np, num_reg, &temp_res) == 0)
@@ -121,35 +128,6 @@ struct device_d *of_platform_device_create(struct device_node *np,
return NULL;
}
}
-
- /*
- * A device may already be registered as platform_device.
- * Instead of registering the same device again, just
- * add this node to the existing device.
- */
- for_each_device(dev) {
- if (!dev->resource)
- continue;
-
- for (i = 0, match = 0; i < num_reg; i++)
- for (j = 0; j < dev->num_resources; j++)
- if (dev->resource[j].start ==
- res[i].start &&
- dev->resource[j].end ==
- res[i].end) {
- match++;
- break;
- }
-
- /* check if all address resources match */
- if (match == num_reg) {
- debug("connecting %s to %s\n",
- np->name, dev_name(dev));
- dev->device_node = np;
- free(res);
- return dev;
- }
- }
}
/* setup generic device info */
@@ -170,8 +148,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 +232,13 @@ static struct device_d *of_amba_device_create(struct device_node *np)
if (!of_device_is_available(np))
return NULL;
+ /*
+ * Linux uses the OF_POPULATED flag to skip already populated/created
+ * devices.
+ */
+ if (np->dev)
+ return np->dev;
+
dev = xzalloc(sizeof(*dev));
/* setup generic device info */
@@ -275,6 +262,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:
diff --git a/include/of.h b/include/of.h
index fc36f7a21a..f2dad9a6c2 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 {
--
2.20.1
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v2 1/8] of: platform: Keep track of populated platform devices
2020-09-30 8:47 ` [PATCH v2 1/8] of: platform: Keep track of populated platform devices Marco Felsch
@ 2020-10-02 5:15 ` Sascha Hauer
2020-10-02 5:47 ` Marco Felsch
0 siblings, 1 reply; 20+ messages in thread
From: Sascha Hauer @ 2020-10-02 5:15 UTC (permalink / raw)
To: Marco Felsch; +Cc: barebox
On Wed, Sep 30, 2020 at 10:47:09AM +0200, Marco Felsch wrote:
> After checking the linux history I found no state where it was explicite
> allowed to register a platform device twice. In the early days there was
> an bug and in some cases linux did populated the same device again. This
> was fixed in 2014 by commit c6e126de43e7 ("of: Keep track of populated
> platform devices") and since then this wasn't possible anymore.
>
> The for_each_device() solution had two issues:
> 1) We are looping over all devices each time
> of_platform_device_create() gets called.
> 2) The logic around 'if (!dev->resource)' then 'continue' seems to be
> broken. This suggest that we can populate a device without
> resource(s) first and adding resource(s) later which isn't the case.
> Instead the current logic will populate the same device again but
> this time (maybe) with resources. So the caller can add the same
> device as many times as possible without resources.
> 3) The device_node gets modified if the new device_node to add has the
> same resources region.
>
> Add a device reference to the device_node to track an registered device
> seems to:
> 1) Simplify the code.
> 2) Align the logic with the current linux state with the exception that
> we are returning the already created device.
> 3) Avoid looping over each device all the time.
> 4) Fix the memory leak which can occure if of_platform_device_create()
> gets called for the same device without any resources.
>
> I added the missing check for amba devices too while on it.
>
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
> v2:
> - new patch
>
> drivers/of/platform.c | 51 +++++++++++++++++--------------------------
> include/of.h | 1 +
> 2 files changed, 21 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index 21c7cce1a5..01de6f98af 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -101,11 +101,18 @@ struct device_d *of_platform_device_create(struct device_node *np,
> struct device_d *dev;
> struct resource *res = NULL, temp_res;
> resource_size_t resinval;
> - int i, j, ret, num_reg = 0, match;
> + int i, ret, num_reg = 0;
>
> if (!of_device_is_available(np))
> return NULL;
>
> + /*
> + * Linux uses the OF_POPULATED flag to skip already populated/created
> + * devices.
> + */
> + if (np->dev)
> + return np->dev;
> +
> /* count the io resources */
> if (of_can_translate_address(np))
> while (of_address_to_resource(np, num_reg, &temp_res) == 0)
> @@ -121,35 +128,6 @@ struct device_d *of_platform_device_create(struct device_node *np,
> return NULL;
> }
> }
> -
> - /*
> - * A device may already be registered as platform_device.
> - * Instead of registering the same device again, just
> - * add this node to the existing device.
> - */
> - for_each_device(dev) {
> - if (!dev->resource)
> - continue;
> -
> - for (i = 0, match = 0; i < num_reg; i++)
> - for (j = 0; j < dev->num_resources; j++)
> - if (dev->resource[j].start ==
> - res[i].start &&
> - dev->resource[j].end ==
> - res[i].end) {
> - match++;
> - break;
> - }
> -
> - /* check if all address resources match */
> - if (match == num_reg) {
> - debug("connecting %s to %s\n",
> - np->name, dev_name(dev));
> - dev->device_node = np;
> - free(res);
> - return dev;
> - }
> - }
This code is for the case when platform code has registered a device
using platform_device_register() which is also in the device tree. When
the device tree gets populated afterwards then we would have two
devices for the same resources. The code you are removing here catches
this case and instead of registering a second device for the same
resources, the existing device only gets the pointer to the corresponding
node.
That said we can probably remove this code which was useful in the early
barebox device tree days, but the code you are adding doesn't replace
it. This should really be two patches.
Sascha
--
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
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v2 1/8] of: platform: Keep track of populated platform devices
2020-10-02 5:15 ` Sascha Hauer
@ 2020-10-02 5:47 ` Marco Felsch
0 siblings, 0 replies; 20+ messages in thread
From: Marco Felsch @ 2020-10-02 5:47 UTC (permalink / raw)
To: Sascha Hauer; +Cc: barebox
On 20-10-02 07:15, Sascha Hauer wrote:
> On Wed, Sep 30, 2020 at 10:47:09AM +0200, Marco Felsch wrote:
> > After checking the linux history I found no state where it was explicite
> > allowed to register a platform device twice. In the early days there was
> > an bug and in some cases linux did populated the same device again. This
> > was fixed in 2014 by commit c6e126de43e7 ("of: Keep track of populated
> > platform devices") and since then this wasn't possible anymore.
> >
> > The for_each_device() solution had two issues:
> > 1) We are looping over all devices each time
> > of_platform_device_create() gets called.
> > 2) The logic around 'if (!dev->resource)' then 'continue' seems to be
> > broken. This suggest that we can populate a device without
> > resource(s) first and adding resource(s) later which isn't the case.
> > Instead the current logic will populate the same device again but
> > this time (maybe) with resources. So the caller can add the same
> > device as many times as possible without resources.
> > 3) The device_node gets modified if the new device_node to add has the
> > same resources region.
> >
> > Add a device reference to the device_node to track an registered device
> > seems to:
> > 1) Simplify the code.
> > 2) Align the logic with the current linux state with the exception that
> > we are returning the already created device.
> > 3) Avoid looping over each device all the time.
> > 4) Fix the memory leak which can occure if of_platform_device_create()
> > gets called for the same device without any resources.
> >
> > I added the missing check for amba devices too while on it.
> >
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > ---
> > v2:
> > - new patch
> >
> > drivers/of/platform.c | 51 +++++++++++++++++--------------------------
> > include/of.h | 1 +
> > 2 files changed, 21 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> > index 21c7cce1a5..01de6f98af 100644
> > --- a/drivers/of/platform.c
> > +++ b/drivers/of/platform.c
> > @@ -101,11 +101,18 @@ struct device_d *of_platform_device_create(struct device_node *np,
> > struct device_d *dev;
> > struct resource *res = NULL, temp_res;
> > resource_size_t resinval;
> > - int i, j, ret, num_reg = 0, match;
> > + int i, ret, num_reg = 0;
> >
> > if (!of_device_is_available(np))
> > return NULL;
> >
> > + /*
> > + * Linux uses the OF_POPULATED flag to skip already populated/created
> > + * devices.
> > + */
> > + if (np->dev)
> > + return np->dev;
> > +
> > /* count the io resources */
> > if (of_can_translate_address(np))
> > while (of_address_to_resource(np, num_reg, &temp_res) == 0)
> > @@ -121,35 +128,6 @@ struct device_d *of_platform_device_create(struct device_node *np,
> > return NULL;
> > }
> > }
> > -
> > - /*
> > - * A device may already be registered as platform_device.
> > - * Instead of registering the same device again, just
> > - * add this node to the existing device.
> > - */
> > - for_each_device(dev) {
> > - if (!dev->resource)
> > - continue;
> > -
> > - for (i = 0, match = 0; i < num_reg; i++)
> > - for (j = 0; j < dev->num_resources; j++)
> > - if (dev->resource[j].start ==
> > - res[i].start &&
> > - dev->resource[j].end ==
> > - res[i].end) {
> > - match++;
> > - break;
> > - }
> > -
> > - /* check if all address resources match */
> > - if (match == num_reg) {
> > - debug("connecting %s to %s\n",
> > - np->name, dev_name(dev));
> > - dev->device_node = np;
> > - free(res);
> > - return dev;
> > - }
> > - }
>
> This code is for the case when platform code has registered a device
> using platform_device_register() which is also in the device tree. When
> the device tree gets populated afterwards then we would have two
> devices for the same resources.
Ah okay, thanks for the clarification :) I see now.
> The code you are removing here catches
> this case and instead of registering a second device for the same
> resources, the existing device only gets the pointer to the corresponding
> node.
Do you think that this use-case is still valid?
> That said we can probably remove this code which was useful in the early
> barebox device tree days, but the code you are adding doesn't replace
> it. This should really be two patches.
Make sense with your explanation.
Regards,
Marco
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2 2/8] of: base: move memory init from DT to initcall
2020-09-30 8:47 [PATCH v2 0/8] Barebox Deep-Probe Marco Felsch
2020-09-30 8:47 ` [PATCH v2 1/8] of: platform: Keep track of populated platform devices Marco Felsch
@ 2020-09-30 8:47 ` Marco Felsch
2020-09-30 8:47 ` [PATCH v2 3/8] of: base: move clock init from of_probe() to barebox_register_of() Marco Felsch
` (5 subsequent siblings)
7 siblings, 0 replies; 20+ messages in thread
From: Marco Felsch @ 2020-09-30 8:47 UTC (permalink / raw)
To: barebox
From: Lucas Stach <dev@lynxeye.de>
Instead of calling it from of_probe, convert it to a initcall at
the appropriate level. This allows to move of_probe to later in
the init sequence while keeping the memory init at the same place,
which is important as many other drivers need the valid memory area
to be set up properly.
Signed-off-by: Lucas Stach <dev@lynxeye.de>
Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
v2:
- no changes
drivers/of/base.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/of/base.c b/drivers/of/base.c
index 4e88af7b22..83e4d0e9b5 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -2144,10 +2144,13 @@ const struct of_device_id of_default_bus_match_table[] = {
}
};
-static void of_probe_memory(void)
+static int of_probe_memory(void)
{
struct device_node *memory = root_node;
+ if (!IS_ENABLED(CONFIG_OFDEVICE))
+ return 0;
+
/* Parse all available node with "memory" device_type */
while (1) {
memory = of_find_node_by_type(memory, "memory");
@@ -2156,7 +2159,10 @@ static void of_probe_memory(void)
of_add_memory(memory, false);
}
+
+ return 0;
}
+mem_initcall(of_probe_memory);
static void of_platform_device_create_root(struct device_node *np)
{
@@ -2186,8 +2192,6 @@ int of_probe(void)
if (of_model)
barebox_set_model(of_model);
- of_probe_memory();
-
firmware = of_find_node_by_path("/firmware");
if (firmware)
of_platform_populate(firmware, NULL, NULL);
--
2.20.1
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
^ permalink raw reply [flat|nested] 20+ messages in thread* [PATCH v2 3/8] of: base: move clock init from of_probe() to barebox_register_of()
2020-09-30 8:47 [PATCH v2 0/8] Barebox Deep-Probe Marco Felsch
2020-09-30 8:47 ` [PATCH v2 1/8] of: platform: Keep track of populated platform devices Marco Felsch
2020-09-30 8:47 ` [PATCH v2 2/8] of: base: move memory init from DT to initcall Marco Felsch
@ 2020-09-30 8:47 ` Marco Felsch
2020-09-30 8:47 ` [PATCH v2 4/8] initcall: add of_populate_initcall Marco Felsch
` (4 subsequent siblings)
7 siblings, 0 replies; 20+ messages in thread
From: Marco Felsch @ 2020-09-30 8:47 UTC (permalink / raw)
To: barebox
This is required for the new deep-probe mechanism. Barebox deep-probe
calls of_probe() very late and so clock drivers using the old
CLK_OF_DECLARE_DRIVER() mechanism are added very late. This would break
the deep-probe approach.
Move of_clk_init() and call it right before the the of_probe() should
have no impact because of_clk_init() only depends on an unflatten dtb.
Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
v2:
- no changes
drivers/of/base.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/of/base.c b/drivers/of/base.c
index 83e4d0e9b5..8ba151dbde 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1575,8 +1575,10 @@ void barebox_register_of(struct device_node *root)
of_fix_tree(root);
of_set_root_node(root);
- if (IS_ENABLED(CONFIG_OFDEVICE))
+ if (IS_ENABLED(CONFIG_OFDEVICE)) {
+ of_clk_init(root, NULL);
of_probe();
+ }
}
void barebox_register_fdt(const void *dtb)
@@ -2198,7 +2200,6 @@ int of_probe(void)
of_platform_device_create_root(root_node);
- of_clk_init(root_node, NULL);
of_platform_populate(root_node, of_default_bus_match_table, NULL);
return 0;
--
2.20.1
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
^ permalink raw reply [flat|nested] 20+ messages in thread* [PATCH v2 4/8] initcall: add of_populate_initcall
2020-09-30 8:47 [PATCH v2 0/8] Barebox Deep-Probe Marco Felsch
` (2 preceding siblings ...)
2020-09-30 8:47 ` [PATCH v2 3/8] of: base: move clock init from of_probe() to barebox_register_of() Marco Felsch
@ 2020-09-30 8:47 ` Marco Felsch
2020-10-02 5:53 ` Ahmad Fatoum
2020-09-30 8:47 ` [PATCH v2 5/8] common: add initial barebox deep-probe support Marco Felsch
` (3 subsequent siblings)
7 siblings, 1 reply; 20+ messages in thread
From: Marco Felsch @ 2020-09-30 8:47 UTC (permalink / raw)
To: barebox
Add dedicated initcall for of_probe() which is required for the following
device-on-demand/deep-probe mechanism.
Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
v2:
- no changes
include/asm-generic/barebox.lds.h | 1 +
include/init.h | 10 +++++++---
2 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/include/asm-generic/barebox.lds.h b/include/asm-generic/barebox.lds.h
index 6971e2c1d2..a7d32160d1 100644
--- a/include/asm-generic/barebox.lds.h
+++ b/include/asm-generic/barebox.lds.h
@@ -33,6 +33,7 @@
KEEP(*(.initcall.12)) \
KEEP(*(.initcall.13)) \
KEEP(*(.initcall.14)) \
+ KEEP(*(.initcall.15)) \
__barebox_initcalls_end = .;
#define BAREBOX_EXITCALLS \
diff --git a/include/init.h b/include/init.h
index 2d61bc8963..1bd5c94b14 100644
--- a/include/init.h
+++ b/include/init.h
@@ -47,6 +47,9 @@ typedef void (*exitcall_t)(void);
* initializes variables that couldn't be statically initialized.
*
* This only exists for built-in code, not for modules.
+ *
+ * The only purpose for "of_populate" is to call of_probe() other functions are
+ * not allowed.
*/
#define pure_initcall(fn) __define_initcall("0",fn,0)
@@ -61,9 +64,10 @@ typedef void (*exitcall_t)(void);
#define fs_initcall(fn) __define_initcall("9",fn,9)
#define device_initcall(fn) __define_initcall("10",fn,10)
#define crypto_initcall(fn) __define_initcall("11",fn,11)
-#define late_initcall(fn) __define_initcall("12",fn,12)
-#define environment_initcall(fn) __define_initcall("13",fn,13)
-#define postenvironment_initcall(fn) __define_initcall("14",fn,14)
+#define of_populate_initcall(fn) __define_initcall("12",fn,15)
+#define late_initcall(fn) __define_initcall("13",fn,12)
+#define environment_initcall(fn) __define_initcall("14",fn,13)
+#define postenvironment_initcall(fn) __define_initcall("15",fn,14)
#define early_exitcall(fn) __define_exitcall("0",fn,0)
#define predevshutdown_exitcall(fn) __define_exitcall("1",fn,1)
--
2.20.1
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v2 4/8] initcall: add of_populate_initcall
2020-09-30 8:47 ` [PATCH v2 4/8] initcall: add of_populate_initcall Marco Felsch
@ 2020-10-02 5:53 ` Ahmad Fatoum
2020-10-20 16:18 ` Marco Felsch
0 siblings, 1 reply; 20+ messages in thread
From: Ahmad Fatoum @ 2020-10-02 5:53 UTC (permalink / raw)
To: Marco Felsch, barebox
On 9/30/20 10:47 AM, Marco Felsch wrote:
> Add dedicated initcall for of_probe() which is required for the following
> device-on-demand/deep-probe mechanism.
>
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
> v2:
> - no changes
>
> include/asm-generic/barebox.lds.h | 1 +
> include/init.h | 10 +++++++---
> 2 files changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/include/asm-generic/barebox.lds.h b/include/asm-generic/barebox.lds.h
> index 6971e2c1d2..a7d32160d1 100644
> --- a/include/asm-generic/barebox.lds.h
> +++ b/include/asm-generic/barebox.lds.h
> @@ -33,6 +33,7 @@
> KEEP(*(.initcall.12)) \
> KEEP(*(.initcall.13)) \
> KEEP(*(.initcall.14)) \
> + KEEP(*(.initcall.15)) \
> __barebox_initcalls_end = .;
>
> #define BAREBOX_EXITCALLS \
> diff --git a/include/init.h b/include/init.h
> index 2d61bc8963..1bd5c94b14 100644
> --- a/include/init.h
> +++ b/include/init.h
> @@ -47,6 +47,9 @@ typedef void (*exitcall_t)(void);
> * initializes variables that couldn't be statically initialized.
> *
> * This only exists for built-in code, not for modules.
> + *
> + * The only purpose for "of_populate" is to call of_probe() other functions are
> + * not allowed.
> */
> #define pure_initcall(fn) __define_initcall("0",fn,0)
>
> @@ -61,9 +64,10 @@ typedef void (*exitcall_t)(void);
> #define fs_initcall(fn) __define_initcall("9",fn,9)
> #define device_initcall(fn) __define_initcall("10",fn,10)
> #define crypto_initcall(fn) __define_initcall("11",fn,11)
> -#define late_initcall(fn) __define_initcall("12",fn,12)
> -#define environment_initcall(fn) __define_initcall("13",fn,13)
> -#define postenvironment_initcall(fn) __define_initcall("14",fn,14)
> +#define of_populate_initcall(fn) __define_initcall("12",fn,15)
s/15/12/ ? (Likewise below)
> +#define late_initcall(fn) __define_initcall("13",fn,12)
> +#define environment_initcall(fn) __define_initcall("14",fn,13)
> +#define postenvironment_initcall(fn) __define_initcall("15",fn,14)
>
> #define early_exitcall(fn) __define_exitcall("0",fn,0)
> #define predevshutdown_exitcall(fn) __define_exitcall("1",fn,1)
>
--
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
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v2 4/8] initcall: add of_populate_initcall
2020-10-02 5:53 ` Ahmad Fatoum
@ 2020-10-20 16:18 ` Marco Felsch
2020-10-20 16:50 ` Ahmad Fatoum
0 siblings, 1 reply; 20+ messages in thread
From: Marco Felsch @ 2020-10-20 16:18 UTC (permalink / raw)
To: Ahmad Fatoum; +Cc: barebox
Hi Ahmad,
On 20-10-02 07:53, Ahmad Fatoum wrote:
>
>
> On 9/30/20 10:47 AM, Marco Felsch wrote:
...
> > @@ -61,9 +64,10 @@ typedef void (*exitcall_t)(void);
> > #define fs_initcall(fn) __define_initcall("9",fn,9)
> > #define device_initcall(fn) __define_initcall("10",fn,10)
> > #define crypto_initcall(fn) __define_initcall("11",fn,11)
> > -#define late_initcall(fn) __define_initcall("12",fn,12)
> > -#define environment_initcall(fn) __define_initcall("13",fn,13)
> > -#define postenvironment_initcall(fn) __define_initcall("14",fn,14)
> > +#define of_populate_initcall(fn) __define_initcall("12",fn,15)
>
> s/15/12/ ? (Likewise below)
I would like to keep it before late_initcall() due to the impact of that
initcall and following environment_initcall(). At least the
environment_initcall() assumes that devices are available to setup the
/env correctly. Therefore I added it before late_initcall(). I can give
it a try to move it behind the late_inicall if you want.
Regards,
Marco
> > +#define late_initcall(fn) __define_initcall("13",fn,12)
> > +#define environment_initcall(fn) __define_initcall("14",fn,13)
> > +#define postenvironment_initcall(fn) __define_initcall("15",fn,14)
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v2 4/8] initcall: add of_populate_initcall
2020-10-20 16:18 ` Marco Felsch
@ 2020-10-20 16:50 ` Ahmad Fatoum
2020-10-20 20:08 ` Marco Felsch
0 siblings, 1 reply; 20+ messages in thread
From: Ahmad Fatoum @ 2020-10-20 16:50 UTC (permalink / raw)
To: Marco Felsch; +Cc: barebox
Hi,
On 10/20/20 6:18 PM, Marco Felsch wrote:
> Hi Ahmad,
>
> On 20-10-02 07:53, Ahmad Fatoum wrote:
>>
>>
>> On 9/30/20 10:47 AM, Marco Felsch wrote:
>
> ...
>
>>> @@ -61,9 +64,10 @@ typedef void (*exitcall_t)(void);
>>> #define fs_initcall(fn) __define_initcall("9",fn,9)
>>> #define device_initcall(fn) __define_initcall("10",fn,10)
>>> #define crypto_initcall(fn) __define_initcall("11",fn,11)
>>> -#define late_initcall(fn) __define_initcall("12",fn,12)
>>> -#define environment_initcall(fn) __define_initcall("13",fn,13)
>>> -#define postenvironment_initcall(fn) __define_initcall("14",fn,14)
>>> +#define of_populate_initcall(fn) __define_initcall("12",fn,15)
>>
>> s/15/12/ ? (Likewise below)
>
> I would like to keep it before late_initcall() due to the impact of that
> initcall and following environment_initcall(). At least the
> environment_initcall() assumes that devices are available to setup the
> /env correctly. Therefore I added it before late_initcall(). I can give
> it a try to move it behind the late_inicall if you want.
Case in point! s/15/12/ doesn't change the initcall order, it only ensures
that you don't expand to __initcall_##fn##15 __section(.initcall.12) which
is confusing. Apparently it confused you to think I mean changing the
actual init level too :-)
>
> Regards,
> Marco
>
>>> +#define late_initcall(fn) __define_initcall("13",fn,12)
>>> +#define environment_initcall(fn) __define_initcall("14",fn,13)
>>> +#define postenvironment_initcall(fn) __define_initcall("15",fn,14)
>
--
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
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v2 4/8] initcall: add of_populate_initcall
2020-10-20 16:50 ` Ahmad Fatoum
@ 2020-10-20 20:08 ` Marco Felsch
0 siblings, 0 replies; 20+ messages in thread
From: Marco Felsch @ 2020-10-20 20:08 UTC (permalink / raw)
To: Ahmad Fatoum; +Cc: barebox
On 20-10-20 18:50, Ahmad Fatoum wrote:
> Hi,
>
> On 10/20/20 6:18 PM, Marco Felsch wrote:
> > Hi Ahmad,
> >
> > On 20-10-02 07:53, Ahmad Fatoum wrote:
> >>
> >>
> >> On 9/30/20 10:47 AM, Marco Felsch wrote:
> >
> > ...
> >
> >>> @@ -61,9 +64,10 @@ typedef void (*exitcall_t)(void);
> >>> #define fs_initcall(fn) __define_initcall("9",fn,9)
> >>> #define device_initcall(fn) __define_initcall("10",fn,10)
> >>> #define crypto_initcall(fn) __define_initcall("11",fn,11)
> >>> -#define late_initcall(fn) __define_initcall("12",fn,12)
> >>> -#define environment_initcall(fn) __define_initcall("13",fn,13)
> >>> -#define postenvironment_initcall(fn) __define_initcall("14",fn,14)
> >>> +#define of_populate_initcall(fn) __define_initcall("12",fn,15)
> >>
> >> s/15/12/ ? (Likewise below)
> >
> > I would like to keep it before late_initcall() due to the impact of that
> > initcall and following environment_initcall(). At least the
> > environment_initcall() assumes that devices are available to setup the
> > /env correctly. Therefore I added it before late_initcall(). I can give
> > it a try to move it behind the late_inicall if you want.
>
> Case in point! s/15/12/ doesn't change the initcall order, it only ensures
> that you don't expand to __initcall_##fn##15 __section(.initcall.12) which
> is confusing. Apparently it confused you to think I mean changing the
> actual init level too :-)
Arg.. damn now I see. Of course I will align it. Didn't saw the 15 on
the end.. Thanks :)
Regards,
Marco
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2 5/8] common: add initial barebox deep-probe support
2020-09-30 8:47 [PATCH v2 0/8] Barebox Deep-Probe Marco Felsch
` (3 preceding siblings ...)
2020-09-30 8:47 ` [PATCH v2 4/8] initcall: add of_populate_initcall Marco Felsch
@ 2020-09-30 8:47 ` Marco Felsch
2020-10-01 10:13 ` Marco Felsch
2020-10-02 6:10 ` Ahmad Fatoum
2020-09-30 8:47 ` [PATCH v2 6/8] ARM: i.MX: esdctl: add " Marco Felsch
` (2 subsequent siblings)
7 siblings, 2 replies; 20+ messages in thread
From: Marco Felsch @ 2020-09-30 8:47 UTC (permalink / raw)
To: barebox
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>
---
v2:
- add deep-probe state to verify it once
- use a linker list array instead of initcalls and runtime allocation
- make of_device_create_on_demand an internal function
- convert to of_device_ensure_probed* and of_devices_ensure_probed*
- add missing function docs
- force the existence of the driver to avoid EPROBE_DEFER (see long-term
goal)
- return int instead of device_d
- fix stub funcs
- add BUG_ON() to track undefined behaviour
- add comment to of_platform_device_create() to make it clear that
calling it again for an already populated device is not allowed
- i2c: only set of_node->dev if it is available
common/Makefile | 1 +
common/deep-probe.c | 36 +++++++
drivers/base/driver.c | 11 +-
drivers/clk/clk.c | 5 +
drivers/i2c/i2c.c | 8 ++
drivers/of/base.c | 13 ++-
drivers/of/platform.c | 165 ++++++++++++++++++++++++++++++
drivers/regulator/core.c | 6 ++
drivers/reset/core.c | 4 +
drivers/spi/spi.c | 2 +
include/asm-generic/barebox.lds.h | 10 +-
include/deep-probe.h | 26 +++++
include/of.h | 32 +++++-
13 files changed, 314 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..af53abdc74
--- /dev/null
+++ b/common/deep-probe.c
@@ -0,0 +1,36 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <common.h>
+#include <deep-probe.h>
+#include <of.h>
+
+enum deep_probe_state {
+ DEEP_PROBE_UNKONW,
+ DEEP_PROBE_SUPPORTED,
+ DEEP_PROBE_NOT_SUPPORTED
+};
+
+static enum deep_probe_state boardstate;
+
+bool deep_probe_is_supported(void)
+{
+ struct deep_probe_entry *board;
+
+ if (boardstate == DEEP_PROBE_NOT_SUPPORTED)
+ return false;
+ else if (boardstate == DEEP_PROBE_SUPPORTED)
+ return true;
+
+ /* determine boardstate */
+ for (board = &__barebox_deep_probe_start;
+ board != &__barebox_deep_probe_end; board++) {
+ if (of_machine_is_compatible(board->compatible)) {
+ boardstate = DEEP_PROBE_SUPPORTED;
+ return true;
+ }
+ }
+
+ boardstate = DEEP_PROBE_NOT_SUPPORTED;
+ 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..e40dfae9ce 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -439,6 +439,11 @@ struct clk *of_clk_get_from_provider(struct of_phandle_args *clkspec)
{
struct of_clk_provider *provider;
struct clk *clk = ERR_PTR(-EPROBE_DEFER);
+ int ret;
+
+ ret = of_device_ensure_probed(clkspec->np);
+ if (ret)
+ return ERR_PTR(ret);
/* Check if we have such a provider in our array */
list_for_each_entry(provider, &of_clk_providers, link) {
diff --git a/drivers/i2c/i2c.c b/drivers/i2c/i2c.c
index 57d8c7017f..12aac42794 100644
--- a/drivers/i2c/i2c.c
+++ b/drivers/i2c/i2c.c
@@ -407,6 +407,9 @@ static struct i2c_client *i2c_new_device(struct i2c_adapter *adapter,
}
client->dev.info = i2c_info;
+ if (chip->of_node)
+ chip->of_node->dev = &client->dev;
+
return client;
}
@@ -548,6 +551,11 @@ struct i2c_adapter *i2c_get_adapter(int busnum)
struct i2c_adapter *of_find_i2c_adapter_by_node(struct device_node *node)
{
struct i2c_adapter *adap;
+ int ret;
+
+ ret = of_device_ensure_probed(node);
+ if (ret)
+ return ERR_PTR(ret);
for_each_i2c_adapter(adap)
if (adap->dev.device_node == node)
diff --git a/drivers/of/base.c b/drivers/of/base.c
index 8ba151dbde..a2f6c39ad1 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 01de6f98af..0368b1485a 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,12 @@
struct device_d *of_find_device_by_node(struct device_node *np)
{
struct device_d *dev;
+ int ret;
+
+ ret = of_device_ensure_probed(np);
+ if (ret)
+ return NULL;
+
for_each_device(dev)
if (dev->device_node == np)
return dev;
@@ -353,3 +360,161 @@ int of_platform_populate(struct device_node *root,
return rc;
}
EXPORT_SYMBOL_GPL(of_platform_populate);
+
+static struct device_d *of_device_create_on_demand(struct device_node *np)
+{
+ struct device_node *parent;
+ struct device_d *parent_dev, *dev;
+
+ 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);
+}
+
+/**
+ * of_device_ensure_probed() - ensures that a device is probed
+ *
+ * @np: the device_node handle which should be probed
+ *
+ * Ensures that the device is populated and probed so frameworks can make use of
+ * it.
+ *
+ * Return: %0 on success
+ * %-ENODEV if either the device can't be populated, the driver is
+ * missing or the driver probe returns an error.
+ */
+int of_device_ensure_probed(struct device_node *np)
+{
+ struct device_d *dev;
+
+ if (!deep_probe_is_supported())
+ return 0;
+
+ dev = of_device_create_on_demand(np);
+ if (IS_ERR(dev))
+ return PTR_ERR(dev);
+
+ BUG_ON(!dev);
+
+ /*
+ * The deep-probe mechanism relies on the fact that all necessary
+ * drivers are added before the device creation. Furthermore deep-probe
+ * is the answer of the EPROBE_DEFER errno so we must ensure that the
+ * driver was probed succesfully after the device creation. Both
+ * requirments are fullfilled if 'dev->driver' is not NULL.
+ */
+ if (!dev->driver)
+ return -ENODEV;
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(of_device_ensure_probed);
+
+/**
+ * of_device_ensure_probed_by_alias() - ensures that a device is probed
+ *
+ * @alias: the alias string to search for a device
+ *
+ * The function search for a given alias string and ensures that the device is
+ * populated and probed if found.
+ *
+ * Return: %0 on success
+ * %-ENODEV if either the device can't be populated, the driver is
+ * missing or the driver probe returns an error
+ * %-EINVAL if alias can't be found
+ */
+int of_device_ensure_probed_by_alias(const char *alias)
+{
+ struct device_node *dev_node;
+
+ dev_node = of_find_node_by_alias(NULL, alias);
+ if (!dev_node)
+ return -EINVAL;
+
+ return of_device_ensure_probed(dev_node);
+}
+EXPORT_SYMBOL_GPL(of_device_ensure_probed_by_alias);
+
+/**
+ * of_devices_ensure_probed_by_dev_id() - ensures that a devices are probed
+ *
+ * @np: the start point device_node handle
+ * @ids: the matching 'struct of_device_id' ids
+ *
+ * The function start searching the device tree from @np and populates and
+ * probes devices which matches @ids.
+ *
+ * Return: %0 on success
+ * %-ENODEV if either the device wasn't found, can't be populated,
+ * the driver is missing or the driver probe returns an error
+ */
+int of_devices_ensure_probed_by_dev_id(struct device_node *np,
+ const struct of_device_id *ids)
+{
+ struct device_node *child;
+
+ if (of_match_node(ids, np))
+ return of_device_ensure_probed(np);
+
+ for_each_child_of_node(np, child) {
+ int ret;
+
+ ret = of_devices_ensure_probed_by_dev_id(child, ids);
+ if (!ret)
+ return 0;
+ }
+
+ return -ENODEV;
+}
+EXPORT_SYMBOL_GPL(of_devices_ensure_probed_by_dev_id);
+
+/**
+ * of_devices_ensure_probed_by_property() - ensures that a devices are probed
+ *
+ * @property_name: The property name to search for
+ *
+ * The function start searching the whole device tree and populates and probe
+ * devices which matches @property_name.
+ *
+ * Return: %0 on success
+ * %-ENODEV if either the device wasn't found, can't be populated,
+ * the driver is missing or the driver probe returns an error
+ */
+int of_devices_ensure_probed_by_property(const char *property_name)
+{
+ struct device_node *node;
+
+ for_each_node_with_property(node, property_name) {
+ int ret;
+
+ ret = of_device_ensure_probed(node);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(of_devices_ensure_probed_by_property);
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 6ea21a4609..803b66bd0c 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);
@@ -191,6 +192,7 @@ static struct regulator_internal *of_regulator_get(struct device_d *dev, const c
char *propname;
struct regulator_internal *ri;
struct device_node *node;
+ int ret;
propname = basprintf("%s-supply", supply);
@@ -222,6 +224,10 @@ static struct regulator_internal *of_regulator_get(struct device_d *dev, const c
goto out;
}
+ ret = of_device_ensure_probed(node);
+ if (ret)
+ return ERR_PTR(ret);
+
list_for_each_entry(ri, ®ulator_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..ee2de58c34 100644
--- a/drivers/reset/core.c
+++ b/drivers/reset/core.c
@@ -168,6 +168,10 @@ static struct reset_control *of_reset_control_get(struct device_node *node,
if (ret)
return ERR_PTR(ret);
+ ret = of_device_ensure_probed(args.np);
+ if (ret)
+ return ERR_PTR(ret);
+
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/asm-generic/barebox.lds.h b/include/asm-generic/barebox.lds.h
index a7d32160d1..c5f9d97547 100644
--- a/include/asm-generic/barebox.lds.h
+++ b/include/asm-generic/barebox.lds.h
@@ -114,6 +114,13 @@
KEEP(*(.rsa_keys.rodata.*)); \
__rsa_keys_end = .; \
+#define BAREBOX_DEEP_PROBE \
+ STRUCT_ALIGN(); \
+ __barebox_deep_probe_start = .; \
+ KEEP(*(SORT_BY_NAME(.barebox_deep_probe*))) \
+ __barebox_deep_probe_end = .;
+
+
#ifdef CONFIG_CONSTRUCTORS
#define KERNEL_CTORS() . = ALIGN(8); \
__ctors_start = .; \
@@ -136,7 +143,8 @@
BAREBOX_CLK_TABLE \
BAREBOX_DTB \
BAREBOX_RSA_KEYS \
- BAREBOX_PCI_FIXUP
+ BAREBOX_PCI_FIXUP \
+ BAREBOX_DEEP_PROBE
#if defined(CONFIG_ARCH_BAREBOX_MAX_BARE_INIT_SIZE) && \
CONFIG_ARCH_BAREBOX_MAX_BARE_INIT_SIZE < CONFIG_BAREBOX_MAX_BARE_INIT_SIZE
diff --git a/include/deep-probe.h b/include/deep-probe.h
new file mode 100644
index 0000000000..4f6673a6f1
--- /dev/null
+++ b/include/deep-probe.h
@@ -0,0 +1,26 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+#ifndef __DEEP_PROBE_H
+#define __DEEP_PROBE_H
+
+#include <linux/stringify.h>
+#include <linux/types.h>
+
+struct deep_probe_entry {
+ const char *compatible;
+};
+
+bool deep_probe_is_supported(void);
+
+extern struct deep_probe_entry __barebox_deep_probe_start;
+extern struct deep_probe_entry __barebox_deep_probe_end;
+
+#define _DEEP_PROBE_ENTRY(_entry) \
+ __barebox_deep_probe_ ## _entry
+
+#define BAREBOX_DEEP_PROBE_ENABLE(_entry,_compatible) \
+ const struct deep_probe_entry _DEEP_PROBE_ENTRY(_entry) \
+ __attribute__ ((unused,section (".barebox_deep_probe_" __stringify(_entry)))) = { \
+ .compatible = _compatible, \
+ }
+
+#endif /* __DEEP_PROBE_H */
diff --git a/include/of.h b/include/of.h
index f2dad9a6c2..3042ce261d 100644
--- a/include/of.h
+++ b/include/of.h
@@ -267,6 +267,12 @@ 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 int of_device_ensure_probed(struct device_node *np);
+extern int of_device_ensure_probed_by_alias(const char *alias);
+extern int of_devices_ensure_probed_by_property(const char *property_name);
+extern int of_devices_ensure_probed_by_dev_id(struct device_node *np,
+ const struct of_device_id *ids);
+
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);
@@ -332,12 +338,34 @@ 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 int of_device_ensure_probed(struct device_node *np)
+{
+ return 0;
+}
+
+static inline int of_device_ensure_probed_by_alias(const char *alias)
+{
+ return 0;
+}
+
+static inline int of_devices_ensure_probed_by_property(const char *property_name)
+{
+ return 0;
+}
+
+static inline int
+of_devices_ensure_probed_by_dev_id(struct device_node *np,
+ const struct of_device_id *ids)
+{
+ return 0;
+}
+
static inline int of_bus_n_addr_cells(struct device_node *np)
{
return 0;
--
2.20.1
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v2 5/8] common: add initial barebox deep-probe support
2020-09-30 8:47 ` [PATCH v2 5/8] common: add initial barebox deep-probe support Marco Felsch
@ 2020-10-01 10:13 ` Marco Felsch
2020-10-02 6:10 ` Ahmad Fatoum
1 sibling, 0 replies; 20+ messages in thread
From: Marco Felsch @ 2020-10-01 10:13 UTC (permalink / raw)
To: barebox
Hi,
On 20-09-30 10:47, Marco Felsch wrote:
> +#define BAREBOX_DEEP_PROBE_ENABLE(_entry,_compatible) \
> + const struct deep_probe_entry _DEEP_PROBE_ENTRY(_entry) \
> + __attribute__ ((unused,section (".barebox_deep_probe_" __stringify(_entry)))) = { \
> + .compatible = _compatible, \
> + }
I forgot to change this to use the __UNIQUE_ID. I will send a fixup if
that is the the only finding in this round, if it is okay for all.
Regards,
Marco
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v2 5/8] common: add initial barebox deep-probe support
2020-09-30 8:47 ` [PATCH v2 5/8] common: add initial barebox deep-probe support Marco Felsch
2020-10-01 10:13 ` Marco Felsch
@ 2020-10-02 6:10 ` Ahmad Fatoum
2020-10-02 6:11 ` Ahmad Fatoum
2020-10-02 7:09 ` Marco Felsch
1 sibling, 2 replies; 20+ messages in thread
From: Ahmad Fatoum @ 2020-10-02 6:10 UTC (permalink / raw)
To: Marco Felsch, barebox
On 9/30/20 10:47 AM, 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>
> ---
> v2:
> - add deep-probe state to verify it once
> - use a linker list array instead of initcalls and runtime allocation
> - make of_device_create_on_demand an internal function
> - convert to of_device_ensure_probed* and of_devices_ensure_probed*
> - add missing function docs
> - force the existence of the driver to avoid EPROBE_DEFER (see long-term
> goal)
> - return int instead of device_d
> - fix stub funcs
> - add BUG_ON() to track undefined behaviour
> - add comment to of_platform_device_create() to make it clear that
> calling it again for an already populated device is not allowed
> - i2c: only set of_node->dev if it is available
>
> common/Makefile | 1 +
> common/deep-probe.c | 36 +++++++
> drivers/base/driver.c | 11 +-
> drivers/clk/clk.c | 5 +
> drivers/i2c/i2c.c | 8 ++
> drivers/of/base.c | 13 ++-
> drivers/of/platform.c | 165 ++++++++++++++++++++++++++++++
> drivers/regulator/core.c | 6 ++
> drivers/reset/core.c | 4 +
> drivers/spi/spi.c | 2 +
> include/asm-generic/barebox.lds.h | 10 +-
> include/deep-probe.h | 26 +++++
> include/of.h | 32 +++++-
> 13 files changed, 314 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..af53abdc74
> --- /dev/null
> +++ b/common/deep-probe.c
> @@ -0,0 +1,36 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <common.h>
> +#include <deep-probe.h>
> +#include <of.h>
> +
> +enum deep_probe_state {
> + DEEP_PROBE_UNKONW,
UNKNOWN*
> + DEEP_PROBE_SUPPORTED,
> + DEEP_PROBE_NOT_SUPPORTED
> +};
> +
> +static enum deep_probe_state boardstate;
> +
> +bool deep_probe_is_supported(void)
> +{
> + struct deep_probe_entry *board;
> +
> + if (boardstate == DEEP_PROBE_NOT_SUPPORTED)
> + return false;
> + else if (boardstate == DEEP_PROBE_SUPPORTED)
> + return true;
If you set UNKNOWN to -ENOSYS, SUPPORTED to 1 and NOT_SUPPORTED to 0,
you could just do if (boardstate >= 0) return boardstate; here
(Even if you want to keep it verbose, I like the enum constants having
expectable values)
> +
> + /* determine boardstate */
> + for (board = &__barebox_deep_probe_start;
> + board != &__barebox_deep_probe_end; board++) {
> + if (of_machine_is_compatible(board->compatible)) {
> + boardstate = DEEP_PROBE_SUPPORTED;
> + return true;
> + }
> + }
> +
> + boardstate = DEEP_PROBE_NOT_SUPPORTED;
> + 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..e40dfae9ce 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -439,6 +439,11 @@ struct clk *of_clk_get_from_provider(struct of_phandle_args *clkspec)
> {
> struct of_clk_provider *provider;
> struct clk *clk = ERR_PTR(-EPROBE_DEFER);
> + int ret;
> +
> + ret = of_device_ensure_probed(clkspec->np);
> + if (ret)
> + return ERR_PTR(ret);
>
> /* Check if we have such a provider in our array */
> list_for_each_entry(provider, &of_clk_providers, link) {
> diff --git a/drivers/i2c/i2c.c b/drivers/i2c/i2c.c
> index 57d8c7017f..12aac42794 100644
> --- a/drivers/i2c/i2c.c
> +++ b/drivers/i2c/i2c.c
> @@ -407,6 +407,9 @@ static struct i2c_client *i2c_new_device(struct i2c_adapter *adapter,
> }
> client->dev.info = i2c_info;
>
> + if (chip->of_node)
> + chip->of_node->dev = &client->dev;
> +
> return client;
> }
>
> @@ -548,6 +551,11 @@ struct i2c_adapter *i2c_get_adapter(int busnum)
> struct i2c_adapter *of_find_i2c_adapter_by_node(struct device_node *node)
> {
> struct i2c_adapter *adap;
> + int ret;
> +
> + ret = of_device_ensure_probed(node);
> + if (ret)
> + return ERR_PTR(ret);
>
> for_each_i2c_adapter(adap)
> if (adap->dev.device_node == node)
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 8ba151dbde..a2f6c39ad1 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 of_probe(); ?
> +
> + return 0;
> +}
> +of_populate_initcall(barebox_of_populate);
This function's name should reflect that it's deep probe specific
> +
> 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 01de6f98af..0368b1485a 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,12 @@
> struct device_d *of_find_device_by_node(struct device_node *np)
> {
> struct device_d *dev;
> + int ret;
> +
> + ret = of_device_ensure_probed(np);
> + if (ret)
> + return NULL;
> +
If you associate a dev with the np on deep probe, can't you just
return it deep_probe_is_supported() ?
> for_each_device(dev)
> if (dev->device_node == np)
> return dev;
> @@ -353,3 +360,161 @@ int of_platform_populate(struct device_node *root,
> return rc;
> }
> EXPORT_SYMBOL_GPL(of_platform_populate);
> +
> +static struct device_d *of_device_create_on_demand(struct device_node *np)
> +{
> + struct device_node *parent;
> + struct device_d *parent_dev, *dev;
> +
> + 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);
> +}
> +
> +/**
> + * of_device_ensure_probed() - ensures that a device is probed
> + *
> + * @np: the device_node handle which should be probed
> + *
> + * Ensures that the device is populated and probed so frameworks can make use of
> + * it.
> + *
> + * Return: %0 on success
> + * %-ENODEV if either the device can't be populated, the driver is
> + * missing or the driver probe returns an error.
> + */
> +int of_device_ensure_probed(struct device_node *np)
> +{
> + struct device_d *dev;
> +
> + if (!deep_probe_is_supported())
> + return 0;
> +
> + dev = of_device_create_on_demand(np);
> + if (IS_ERR(dev))
> + return PTR_ERR(dev);
> +
> + BUG_ON(!dev);
> +
> + /*
> + * The deep-probe mechanism relies on the fact that all necessary
> + * drivers are added before the device creation. Furthermore deep-probe
> + * is the answer of the EPROBE_DEFER errno so we must ensure that the
answer to*
> + * driver was probed succesfully after the device creation. Both
successfully
> + * requirments are fullfilled if 'dev->driver' is not NULL.
requirements, fulfilled
> + */
> + if (!dev->driver)
> + return -ENODEV;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(of_device_ensure_probed);
> +
> +/**
> + * of_device_ensure_probed_by_alias() - ensures that a device is probed
> + *
> + * @alias: the alias string to search for a device
> + *
> + * The function search for a given alias string and ensures that the device is
> + * populated and probed if found.
> + *
> + * Return: %0 on success
> + * %-ENODEV if either the device can't be populated, the driver is
> + * missing or the driver probe returns an error
I don't think it would be nice to just pass along driver probe errors as-is.
> + * %-EINVAL if alias can't be found
> + */
> +int of_device_ensure_probed_by_alias(const char *alias)
> +{
> + struct device_node *dev_node;
> +
> + dev_node = of_find_node_by_alias(NULL, alias);
> + if (!dev_node)
> + return -EINVAL;
> +
> + return of_device_ensure_probed(dev_node);
> +}
> +EXPORT_SYMBOL_GPL(of_device_ensure_probed_by_alias);
> +
> +/**
> + * of_devices_ensure_probed_by_dev_id() - ensures that a devices are probed
> + *
> + * @np: the start point device_node handle
> + * @ids: the matching 'struct of_device_id' ids
> + *
> + * The function start searching the device tree from @np and populates and
> + * probes devices which matches @ids.
> + *
> + * Return: %0 on success
> + * %-ENODEV if either the device wasn't found, can't be populated,
> + * the driver is missing or the driver probe returns an error
Likewise
> + */
> +int of_devices_ensure_probed_by_dev_id(struct device_node *np,
> + const struct of_device_id *ids)
> +{
> + struct device_node *child;
> +
> + if (of_match_node(ids, np))
> + return of_device_ensure_probed(np);
> +
> + for_each_child_of_node(np, child) {
> + int ret;
> +
> + ret = of_devices_ensure_probed_by_dev_id(child, ids);
> + if (!ret)
> + return 0;
> + }
> +
> + return -ENODEV;
> +}
> +EXPORT_SYMBOL_GPL(of_devices_ensure_probed_by_dev_id);
> +
> +/**
> + * of_devices_ensure_probed_by_property() - ensures that a devices are probed
> + *
> + * @property_name: The property name to search for
> + *
> + * The function start searching the whole device tree and populates and probe
> + * devices which matches @property_name.
> + *
> + * Return: %0 on success
> + * %-ENODEV if either the device wasn't found, can't be populated,
> + * the driver is missing or the driver probe returns an error
Likewise
> + */
> +int of_devices_ensure_probed_by_property(const char *property_name)
> +{
> + struct device_node *node;
> +
> + for_each_node_with_property(node, property_name) {
> + int ret;
> +
> + ret = of_device_ensure_probed(node);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(of_devices_ensure_probed_by_property);
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index 6ea21a4609..803b66bd0c 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);
> @@ -191,6 +192,7 @@ static struct regulator_internal *of_regulator_get(struct device_d *dev, const c
> char *propname;
> struct regulator_internal *ri;
> struct device_node *node;
> + int ret;
>
> propname = basprintf("%s-supply", supply);
>
> @@ -222,6 +224,10 @@ static struct regulator_internal *of_regulator_get(struct device_d *dev, const c
> goto out;
> }
>
> + ret = of_device_ensure_probed(node);
> + if (ret)
> + return ERR_PTR(ret);
> +
> list_for_each_entry(ri, ®ulator_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..ee2de58c34 100644
> --- a/drivers/reset/core.c
> +++ b/drivers/reset/core.c
> @@ -168,6 +168,10 @@ static struct reset_control *of_reset_control_get(struct device_node *node,
> if (ret)
> return ERR_PTR(ret);
>
> + ret = of_device_ensure_probed(args.np);
> + if (ret)
> + return ERR_PTR(ret);
> +
> 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/asm-generic/barebox.lds.h b/include/asm-generic/barebox.lds.h
> index a7d32160d1..c5f9d97547 100644
> --- a/include/asm-generic/barebox.lds.h
> +++ b/include/asm-generic/barebox.lds.h
> @@ -114,6 +114,13 @@
> KEEP(*(.rsa_keys.rodata.*)); \
> __rsa_keys_end = .; \
>
> +#define BAREBOX_DEEP_PROBE \
> + STRUCT_ALIGN(); \
> + __barebox_deep_probe_start = .; \
> + KEEP(*(SORT_BY_NAME(.barebox_deep_probe*))) \
> + __barebox_deep_probe_end = .;
> +
> +
> #ifdef CONFIG_CONSTRUCTORS
> #define KERNEL_CTORS() . = ALIGN(8); \
> __ctors_start = .; \
> @@ -136,7 +143,8 @@
> BAREBOX_CLK_TABLE \
> BAREBOX_DTB \
> BAREBOX_RSA_KEYS \
> - BAREBOX_PCI_FIXUP
> + BAREBOX_PCI_FIXUP \
> + BAREBOX_DEEP_PROBE
>
> #if defined(CONFIG_ARCH_BAREBOX_MAX_BARE_INIT_SIZE) && \
> CONFIG_ARCH_BAREBOX_MAX_BARE_INIT_SIZE < CONFIG_BAREBOX_MAX_BARE_INIT_SIZE
> diff --git a/include/deep-probe.h b/include/deep-probe.h
> new file mode 100644
> index 0000000000..4f6673a6f1
> --- /dev/null
> +++ b/include/deep-probe.h
> @@ -0,0 +1,26 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#ifndef __DEEP_PROBE_H
> +#define __DEEP_PROBE_H
> +
> +#include <linux/stringify.h>
> +#include <linux/types.h>
> +
> +struct deep_probe_entry {
> + const char *compatible;
> +};
> +
> +bool deep_probe_is_supported(void);
> +
> +extern struct deep_probe_entry __barebox_deep_probe_start;
> +extern struct deep_probe_entry __barebox_deep_probe_end;
> +
> +#define _DEEP_PROBE_ENTRY(_entry) \
> + __barebox_deep_probe_ ## _entry
> +
> +#define BAREBOX_DEEP_PROBE_ENABLE(_entry,_compatible) \
> + const struct deep_probe_entry _DEEP_PROBE_ENTRY(_entry) \
> + __attribute__ ((unused,section (".barebox_deep_probe_" __stringify(_entry)))) = { \
> + .compatible = _compatible, \
> + }
> +
> +#endif /* __DEEP_PROBE_H */
> diff --git a/include/of.h b/include/of.h
> index f2dad9a6c2..3042ce261d 100644
> --- a/include/of.h
> +++ b/include/of.h
> @@ -267,6 +267,12 @@ 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 int of_device_ensure_probed(struct device_node *np);
> +extern int of_device_ensure_probed_by_alias(const char *alias);
> +extern int of_devices_ensure_probed_by_property(const char *property_name);
> +extern int of_devices_ensure_probed_by_dev_id(struct device_node *np,
> + const struct of_device_id *ids);
> +
> 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);
> @@ -332,12 +338,34 @@ 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)
Unrelated change?
> {
> return NULL;
> }
>
> +static inline int of_device_ensure_probed(struct device_node *np)
> +{
> + return 0;
> +}
> +
> +static inline int of_device_ensure_probed_by_alias(const char *alias)
> +{
> + return 0;
> +}
> +
> +static inline int of_devices_ensure_probed_by_property(const char *property_name)
> +{
> + return 0;
> +}
> +
> +static inline int
> +of_devices_ensure_probed_by_dev_id(struct device_node *np,
> + const struct of_device_id *ids)
> +{
> + 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
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v2 5/8] common: add initial barebox deep-probe support
2020-10-02 6:10 ` Ahmad Fatoum
@ 2020-10-02 6:11 ` Ahmad Fatoum
2020-10-02 7:09 ` Marco Felsch
1 sibling, 0 replies; 20+ messages in thread
From: Ahmad Fatoum @ 2020-10-02 6:11 UTC (permalink / raw)
To: Marco Felsch, barebox
On 10/2/20 8:10 AM, Ahmad Fatoum wrote:
> I don't think it would be nice to just pass along driver probe errors as-is.
s/don't//
--
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 5/8] common: add initial barebox deep-probe support
2020-10-02 6:10 ` Ahmad Fatoum
2020-10-02 6:11 ` Ahmad Fatoum
@ 2020-10-02 7:09 ` Marco Felsch
2020-10-02 7:18 ` Ahmad Fatoum
1 sibling, 1 reply; 20+ messages in thread
From: Marco Felsch @ 2020-10-02 7:09 UTC (permalink / raw)
To: Ahmad Fatoum; +Cc: barebox
Hi Ahmad,
On 20-10-02 08:10, Ahmad Fatoum wrote:
> > +enum deep_probe_state {
> > + DEEP_PROBE_UNKONW,
>
> UNKNOWN*
Yep.
> > + DEEP_PROBE_SUPPORTED,
> > + DEEP_PROBE_NOT_SUPPORTED
> > +};
> > +
> > +static enum deep_probe_state boardstate;
> > +
> > +bool deep_probe_is_supported(void)
> > +{
> > + struct deep_probe_entry *board;
> > +
> > + if (boardstate == DEEP_PROBE_NOT_SUPPORTED)
> > + return false;
> > + else if (boardstate == DEEP_PROBE_SUPPORTED)
> > + return true;
>
> If you set UNKNOWN to -ENOSYS, SUPPORTED to 1 and NOT_SUPPORTED to 0,
> you could just do if (boardstate >= 0) return boardstate; here
> (Even if you want to keep it verbose, I like the enum constants having
> expectable values)
IMHO enums should abstract the value to provide a more readyble code.
Here it isn't that hard to follow but in general I'm not a fan of using
enums with '(boardstate >= 0)'. I use such constructs only if it really
necessary e.g. state-machines.
> > +static int barebox_of_populate(void)
> > +{
> > + if (IS_ENABLED(CONFIG_OFDEVICE) && deep_probe_is_supported())
> > + of_probe();
>
> return of_probe(); ?
Good point but this will change the logic since barebox_register_of() is
void.
> > +
> > + return 0;
> > +}
> > +of_populate_initcall(barebox_of_populate);
>
> This function's name should reflect that it's deep probe specific
I think the deep_probe_is_supported() reflects that. The long-term goal
should be to remove the deep_probe_is_supported() and call of_probe()
only in this initcall.
> > +
> > 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 01de6f98af..0368b1485a 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,12 @@
> > struct device_d *of_find_device_by_node(struct device_node *np)
> > {
> > struct device_d *dev;
> > + int ret;
> > +
> > + ret = of_device_ensure_probed(np);
> > + if (ret)
> > + return NULL;
> > +
>
> If you associate a dev with the np on deep probe, can't you just
> return it deep_probe_is_supported() ?
Sry. don't get this. This function has a few users e.g. the
chipidea-imx.c to find the required sub-devices. We need to ensure that
those devices are probed and available if this isn't done yet in case of
deep_probe_is_supported() returns true.
> > + /*
> > + * The deep-probe mechanism relies on the fact that all necessary
> > + * drivers are added before the device creation. Furthermore deep-probe
> > + * is the answer of the EPROBE_DEFER errno so we must ensure that the
>
> answer to*
>
> > + * driver was probed succesfully after the device creation. Both
>
> successfully
>
> > + * requirments are fullfilled if 'dev->driver' is not NULL.
>
> requirements, fulfilled
Will fix those typos in v3. Thanks.
> > +/**
> > + * of_device_ensure_probed_by_alias() - ensures that a device is probed
> > + *
> > + * @alias: the alias string to search for a device
> > + *
> > + * The function search for a given alias string and ensures that the device is
> > + * populated and probed if found.
> > + *
> > + * Return: %0 on success
> > + * %-ENODEV if either the device can't be populated, the driver is
> > + * missing or the driver probe returns an error
>
> I don't think it would be nice to just pass along driver probe errors as-is.
We can't distinguish between those failures yet, pls check the match()
function in drivers/base/driver.c. Can we address this later?
> > -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)
>
> Unrelated change?
Yep, will drop that one.
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v2 5/8] common: add initial barebox deep-probe support
2020-10-02 7:09 ` Marco Felsch
@ 2020-10-02 7:18 ` Ahmad Fatoum
0 siblings, 0 replies; 20+ messages in thread
From: Ahmad Fatoum @ 2020-10-02 7:18 UTC (permalink / raw)
To: Marco Felsch; +Cc: barebox
Hi,
On 10/2/20 9:09 AM, Marco Felsch wrote:
> Hi Ahmad,
>
> On 20-10-02 08:10, Ahmad Fatoum wrote:
>
>>> +enum deep_probe_state {
>>> + DEEP_PROBE_UNKONW,
>>
>> UNKNOWN*
>
> Yep.
>
>>> + DEEP_PROBE_SUPPORTED,
>>> + DEEP_PROBE_NOT_SUPPORTED
>>> +};
>>> +
>>> +static enum deep_probe_state boardstate;
>>> +
>>> +bool deep_probe_is_supported(void)
>>> +{
>>> + struct deep_probe_entry *board;
>>> +
>>> + if (boardstate == DEEP_PROBE_NOT_SUPPORTED)
>>> + return false;
>>> + else if (boardstate == DEEP_PROBE_SUPPORTED)
>>> + return true;
>>
>> If you set UNKNOWN to -ENOSYS, SUPPORTED to 1 and NOT_SUPPORTED to 0,
>> you could just do if (boardstate >= 0) return boardstate; here
>> (Even if you want to keep it verbose, I like the enum constants having
>> expectable values)
>
> IMHO enums should abstract the value to provide a more readyble code.
> Here it isn't that hard to follow but in general I'm not a fan of using
> enums with '(boardstate >= 0)'. I use such constructs only if it really
> necessary e.g. state-machines.
Ok.
>
>>> +static int barebox_of_populate(void)
>>> +{
>>> + if (IS_ENABLED(CONFIG_OFDEVICE) && deep_probe_is_supported())
>>> + of_probe();
>>
>> return of_probe(); ?
>
> Good point but this will change the logic since barebox_register_of() is
> void.
Failed initcalls AFAIK only result in an error message, so no logic change there.
>>> +
>>> + return 0;
>>> +}
>>> +of_populate_initcall(barebox_of_populate);
>>
>> This function's name should reflect that it's deep probe specific
>
> I think the deep_probe_is_supported() reflects that. The long-term goal
> should be to remove the deep_probe_is_supported() and call of_probe()
> only in this initcall.
I see.
>
>>> +
>>> 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 01de6f98af..0368b1485a 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,12 @@
>>> struct device_d *of_find_device_by_node(struct device_node *np)
>>> {
>>> struct device_d *dev;
>>> + int ret;
>>> +
>>> + ret = of_device_ensure_probed(np);
>>> + if (ret)
>>> + return NULL;
>>> +
>>
>> If you associate a dev with the np on deep probe, can't you just
>> return it deep_probe_is_supported() ?
>
> Sry. don't get this. This function has a few users e.g. the
> chipidea-imx.c to find the required sub-devices. We need to ensure that
> those devices are probed and available if this isn't done yet in case of
> deep_probe_is_supported() returns true.
My impresson was that after of_device_ensure_probed, np->dev
is populated for some device nodes. If it's, couldn't we just return that
instead of iterating?
>>> + /*
>>> + * The deep-probe mechanism relies on the fact that all necessary
>>> + * drivers are added before the device creation. Furthermore deep-probe
>>> + * is the answer of the EPROBE_DEFER errno so we must ensure that the
>>
>> answer to*
>>
>>> + * driver was probed succesfully after the device creation. Both
>>
>> successfully
>>
>>> + * requirments are fullfilled if 'dev->driver' is not NULL.
>>
>> requirements, fulfilled
>
> Will fix those typos in v3. Thanks.
>
>>> +/**
>>> + * of_device_ensure_probed_by_alias() - ensures that a device is probed
>>> + *
>>> + * @alias: the alias string to search for a device
>>> + *
>>> + * The function search for a given alias string and ensures that the device is
>>> + * populated and probed if found.
>>> + *
>>> + * Return: %0 on success
>>> + * %-ENODEV if either the device can't be populated, the driver is
>>> + * missing or the driver probe returns an error
>>
>> I don't think it would be nice to just pass along driver probe errors as-is.
>
> We can't distinguish between those failures yet, pls check the match()
> function in drivers/base/driver.c. Can we address this later?
Ok.
>
>>> -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)
>>
>> Unrelated change?
>
> Yep, will drop that one.
>
--
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2 6/8] ARM: i.MX: esdctl: add deep-probe support
2020-09-30 8:47 [PATCH v2 0/8] Barebox Deep-Probe Marco Felsch
` (4 preceding siblings ...)
2020-09-30 8:47 ` [PATCH v2 5/8] common: add initial barebox deep-probe support Marco Felsch
@ 2020-09-30 8:47 ` Marco Felsch
2020-09-30 8:47 ` [PATCH v2 7/8] ARM: stm32mp: ddrctrl: " Marco Felsch
2020-09-30 8:47 ` [PATCH v2 8/8] ARM: boards: mx6-sabrelite: " Marco Felsch
7 siblings, 0 replies; 20+ messages in thread
From: Marco Felsch @ 2020-09-30 8:47 UTC (permalink / raw)
To: barebox
In case of deep-probe we have to ensure that the memory device is
available after the mem_initcall().
Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
v2:
- convert to of_devices_ensure_probed_by_dev_id()
- drop use-less deep-probe include
arch/arm/mach-imx/esdctl.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/arch/arm/mach-imx/esdctl.c b/arch/arm/mach-imx/esdctl.c
index 426a96a3c4..1a3af9718b 100644
--- a/arch/arm/mach-imx/esdctl.c
+++ b/arch/arm/mach-imx/esdctl.c
@@ -703,7 +703,18 @@ static struct driver_d imx_esdctl_driver = {
.of_compatible = DRV_OF_COMPAT(imx_esdctl_dt_ids),
};
-mem_platform_driver(imx_esdctl_driver);
+static int imx_esdctl_init(void)
+{
+ int ret;
+
+ ret = platform_driver_register(&imx_esdctl_driver);
+ if (ret)
+ return ret;
+
+ return of_devices_ensure_probed_by_dev_id(of_get_root_node(),
+ imx_esdctl_dt_ids);
+}
+mem_initcall(imx_esdctl_init);
/*
* The i.MX SoCs usually have two SDRAM chipselects. The following
--
2.20.1
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
^ permalink raw reply [flat|nested] 20+ messages in thread* [PATCH v2 7/8] ARM: stm32mp: ddrctrl: add deep-probe support
2020-09-30 8:47 [PATCH v2 0/8] Barebox Deep-Probe Marco Felsch
` (5 preceding siblings ...)
2020-09-30 8:47 ` [PATCH v2 6/8] ARM: i.MX: esdctl: add " Marco Felsch
@ 2020-09-30 8:47 ` Marco Felsch
2020-09-30 8:47 ` [PATCH v2 8/8] ARM: boards: mx6-sabrelite: " Marco Felsch
7 siblings, 0 replies; 20+ messages in thread
From: Marco Felsch @ 2020-09-30 8:47 UTC (permalink / raw)
To: barebox
In case of deep-probe we have to ensure that the memory device is
available after the mem_initcall().
Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
v2:
- convert to of_devices_ensure_probed_by_dev_id()
- drop use-less deep-probe include
arch/arm/mach-stm32mp/ddrctrl.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/arch/arm/mach-stm32mp/ddrctrl.c b/arch/arm/mach-stm32mp/ddrctrl.c
index 646fe4401a..a7a87ef8f8 100644
--- a/arch/arm/mach-stm32mp/ddrctrl.c
+++ b/arch/arm/mach-stm32mp/ddrctrl.c
@@ -148,4 +148,15 @@ static struct driver_d stm32mp1_ddr_driver = {
.of_compatible = DRV_OF_COMPAT(stm32mp1_ddr_dt_ids),
};
-mem_platform_driver(stm32mp1_ddr_driver);
+static int stm32mp1_ddr_init(void)
+{
+ int ret;
+
+ ret = platform_driver_register(&stm32mp1_ddr_driver);
+ if (ret)
+ return ret;
+
+ return of_devices_ensure_probed_by_dev_id(of_get_root_node(),
+ stm32mp1_ddr_dt_ids);
+}
+mem_initcall(stm32mp1_ddr_init);
--
2.20.1
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
^ permalink raw reply [flat|nested] 20+ messages in thread* [PATCH v2 8/8] ARM: boards: mx6-sabrelite: add deep-probe support
2020-09-30 8:47 [PATCH v2 0/8] Barebox Deep-Probe Marco Felsch
` (6 preceding siblings ...)
2020-09-30 8:47 ` [PATCH v2 7/8] ARM: stm32mp: ddrctrl: " Marco Felsch
@ 2020-09-30 8:47 ` Marco Felsch
7 siblings, 0 replies; 20+ messages in thread
From: Marco Felsch @ 2020-09-30 8:47 UTC (permalink / raw)
To: barebox
Explicit request the required gpio resources instead of relying on their
existence based on the initcall level.
Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
v2:
- convert to new macro name
- convert to new api
.../boards/freescale-mx6-sabrelite/board.c | 24 ++++++++++++-------
1 file changed, 15 insertions(+), 9 deletions(-)
diff --git a/arch/arm/boards/freescale-mx6-sabrelite/board.c b/arch/arm/boards/freescale-mx6-sabrelite/board.c
index 1b39ef82c6..c3d06b34f4 100644
--- a/arch/arm/boards/freescale-mx6-sabrelite/board.c
+++ b/arch/arm/boards/freescale-mx6-sabrelite/board.c
@@ -13,7 +13,9 @@
#include <mach/bbu.h>
#include <asm/armlinux.h>
#include <generated/mach-types.h>
+#include <of.h>
#include <partition.h>
+#include <deep-probe.h>
#include <linux/phy.h>
#include <asm/io.h>
#include <asm/mmu.h>
@@ -98,10 +100,6 @@ static int sabrelite_ksz9021rn_setup(void)
{
int ret;
- if (!of_machine_is_compatible("fsl,imx6q-sabrelite") &&
- !of_machine_is_compatible("fsl,imx6dl-sabrelite"))
- return 0;
-
mxc_iomux_v3_setup_multiple_pads(sabrelite_enet_gpio_pads,
ARRAY_SIZE(sabrelite_enet_gpio_pads));
@@ -118,11 +116,6 @@ static int sabrelite_ksz9021rn_setup(void)
return 0;
}
-/*
- * Do this before the fec initializes but after our
- * gpios are available.
- */
-fs_initcall(sabrelite_ksz9021rn_setup);
static void sabrelite_ehci_init(void)
{
@@ -134,10 +127,20 @@ static void sabrelite_ehci_init(void)
static int sabrelite_devices_init(void)
{
+ int ret;
+
if (!of_machine_is_compatible("fsl,imx6q-sabrelite") &&
!of_machine_is_compatible("fsl,imx6dl-sabrelite"))
return 0;
+ ret = of_devices_ensure_probed_by_property("gpio-controller");
+ if (ret)
+ return ret;
+
+ ret = sabrelite_ksz9021rn_setup();
+ if (ret)
+ return ret;
+
sabrelite_ehci_init();
armlinux_set_architecture(3769);
@@ -163,3 +166,6 @@ static int sabrelite_coredevices_init(void)
return 0;
}
coredevice_initcall(sabrelite_coredevices_init);
+
+BAREBOX_DEEP_PROBE_ENABLE(imx6q_sabrelite,"fsl,imx6q-sabrelite");
+BAREBOX_DEEP_PROBE_ENABLE(imx6dl_sabrelite,"fsl,imx6dl-sabrelite");
--
2.20.1
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
^ permalink raw reply [flat|nested] 20+ messages in thread