Patchwork [RFC] early init and DT platform devices allocation/registration

login
register
mail settings
Submitter Hiroshi Doyu
Date June 28, 2013, 8:49 a.m.
Message ID <20130628.114915.1341075505557760886.hdoyu@nvidia.com>
Download mbox | patch
Permalink /patch/255310/
State Not Applicable, archived
Headers show

Comments

Hiroshi Doyu - June 28, 2013, 8:49 a.m.
Grant Likely <grant.likely@secretlab.ca> wrote @ Wed, 26 Jun 2013 12:03:20 +0200:

> On Wed, Jun 26, 2013 at 7:00 AM, Hiroshi Doyu <hdoyu@nvidia.com> wrote:
> > Grant Likely <grant.likely@secretlab.ca> wrote @ Tue, 25 Jun 2013 19:52:33 +0200:
> >
> >> > Here's my workaround. I need to call of_detach_node() with OF_DYNAMIC
> >> > to avoid duplicated device registration.
> >>
> >> Gah! my eyes!
> >>
> >> Don't do that. It is incredibly problematic. Look at inhibiting
> >> duplicate device creation instead.
> >
> > I may not follow this thread correctly, but could anyone point out the
> > above "inhibiting duplicate device creation" if there's already such
> > solution?
> 
> No, the solution doesn't exist yet, but it wouldn't be hard to
> implement. What you need to do is to add a struct device pointer to
> struct device_node, and set the pointer to the struct device when
> of_platform_device_create creates a device. (it would also need to be
> set for early_platform_device creation, but that's not something that
> should affect you). You would also add a check to
> of_platform_device_create to check if the device pointer is already
> set. If it is, then skip creation of the device.

Implemented as Grant suggested. At least this works for our case,
where IOMMU needs to be instanciated earlier than other device[1].
early_platform_device case still need to be covered.

[1] https://lists.ozlabs.org/pipermail/devicetree-discuss/2013-June/036685.html

From da563c72f7fc37fedd4b7e3a957f41a484a19788 Mon Sep 17 00:00:00 2001
From: Hiroshi Doyu <hdoyu@nvidia.com>
Date: Fri, 28 Jun 2013 11:43:20 +0300
Subject: [PATCH RFC 1/1] of: dev_node has struct device pointer

To prevent of_platform_populate() from trying to populate duplicate
devices if a device has been already populated.

Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
---
 drivers/of/base.c     |   23 +++++++++++++++++++++++
 drivers/of/platform.c |    8 ++++++++
 include/linux/of.h    |   16 ++++++++++++++++
 3 files changed, 47 insertions(+)
Thierry Reding - June 28, 2013, 10:38 a.m.
On Fri, Jun 28, 2013 at 10:49:15AM +0200, Hiroshi Doyu wrote:
> Grant Likely <grant.likely@secretlab.ca> wrote @ Wed, 26 Jun 2013 12:03:20 +0200:
> 
> > On Wed, Jun 26, 2013 at 7:00 AM, Hiroshi Doyu <hdoyu@nvidia.com> wrote:
> > > Grant Likely <grant.likely@secretlab.ca> wrote @ Tue, 25 Jun 2013 19:52:33 +0200:
> > >
> > >> > Here's my workaround. I need to call of_detach_node() with OF_DYNAMIC
> > >> > to avoid duplicated device registration.
> > >>
> > >> Gah! my eyes!
> > >>
> > >> Don't do that. It is incredibly problematic. Look at inhibiting
> > >> duplicate device creation instead.
> > >
> > > I may not follow this thread correctly, but could anyone point out the
> > > above "inhibiting duplicate device creation" if there's already such
> > > solution?
> > 
> > No, the solution doesn't exist yet, but it wouldn't be hard to
> > implement. What you need to do is to add a struct device pointer to
> > struct device_node, and set the pointer to the struct device when
> > of_platform_device_create creates a device. (it would also need to be
> > set for early_platform_device creation, but that's not something that
> > should affect you). You would also add a check to
> > of_platform_device_create to check if the device pointer is already
> > set. If it is, then skip creation of the device.
> 
> Implemented as Grant suggested. At least this works for our case,
> where IOMMU needs to be instanciated earlier than other device[1].
> early_platform_device case still need to be covered.

I think we arrived at a different conclusion in another branch of this
thread. With the patch below every driver needs to explicitly allocate
the platform device and set the struct device_node's .dev field, which
has other side-effects such as the device hierarchy getting messed up.

A better alternative would be to have of_platform_populate() run early
such that the .dev field in the struct device_node can be set by core
code, which would not require every driver to be changed.

I'm not sure exactly what needs to be done to make this work, however.
Grant, can you provide some guidance here as to how this may be fixed?
Where would we have to call of_platform_populate() from and what makes
this break with the current implementation?

Thierry
Grant Likely - June 28, 2013, 12:27 p.m.
On Fri, Jun 28, 2013 at 11:38 AM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Fri, Jun 28, 2013 at 10:49:15AM +0200, Hiroshi Doyu wrote:
>> Grant Likely <grant.likely@secretlab.ca> wrote @ Wed, 26 Jun 2013 12:03:20 +0200:
>>
>> > On Wed, Jun 26, 2013 at 7:00 AM, Hiroshi Doyu <hdoyu@nvidia.com> wrote:
>> > > Grant Likely <grant.likely@secretlab.ca> wrote @ Tue, 25 Jun 2013 19:52:33 +0200:
>> > >
>> > >> > Here's my workaround. I need to call of_detach_node() with OF_DYNAMIC
>> > >> > to avoid duplicated device registration.
>> > >>
>> > >> Gah! my eyes!
>> > >>
>> > >> Don't do that. It is incredibly problematic. Look at inhibiting
>> > >> duplicate device creation instead.
>> > >
>> > > I may not follow this thread correctly, but could anyone point out the
>> > > above "inhibiting duplicate device creation" if there's already such
>> > > solution?
>> >
>> > No, the solution doesn't exist yet, but it wouldn't be hard to
>> > implement. What you need to do is to add a struct device pointer to
>> > struct device_node, and set the pointer to the struct device when
>> > of_platform_device_create creates a device. (it would also need to be
>> > set for early_platform_device creation, but that's not something that
>> > should affect you). You would also add a check to
>> > of_platform_device_create to check if the device pointer is already
>> > set. If it is, then skip creation of the device.
>>
>> Implemented as Grant suggested. At least this works for our case,
>> where IOMMU needs to be instanciated earlier than other device[1].
>> early_platform_device case still need to be covered.
>
> I think we arrived at a different conclusion in another branch of this
> thread. With the patch below every driver needs to explicitly allocate
> the platform device and set the struct device_node's .dev field, which
> has other side-effects such as the device hierarchy getting messed up.
>
> A better alternative would be to have of_platform_populate() run early
> such that the .dev field in the struct device_node can be set by core
> code, which would not require every driver to be changed.
>
> I'm not sure exactly what needs to be done to make this work, however.
> Grant, can you provide some guidance here as to how this may be fixed?
> Where would we have to call of_platform_populate() from and what makes
> this break with the current implementation?

Try it and see!  :-) I cannot give a definite answer, but I suspect
that it will fall down when registering the devices on to the
platform_bus because it isn't initialized yet. If called early, the
code would need to hold the platform_device in some kind of deferred
list until the platform bus was initialize, and then have a cleanup
function at initcall time to finish the registration of all early
devices.

g.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thierry Reding - Aug. 15, 2013, 2:23 p.m.
On Fri, Jun 28, 2013 at 01:27:15PM +0100, Grant Likely wrote:
> On Fri, Jun 28, 2013 at 11:38 AM, Thierry Reding
> <thierry.reding@gmail.com> wrote:
> > On Fri, Jun 28, 2013 at 10:49:15AM +0200, Hiroshi Doyu wrote:
> >> Grant Likely <grant.likely@secretlab.ca> wrote @ Wed, 26 Jun 2013 12:03:20 +0200:
> >>
> >> > On Wed, Jun 26, 2013 at 7:00 AM, Hiroshi Doyu <hdoyu@nvidia.com> wrote:
> >> > > Grant Likely <grant.likely@secretlab.ca> wrote @ Tue, 25 Jun 2013 19:52:33 +0200:
> >> > >
> >> > >> > Here's my workaround. I need to call of_detach_node() with OF_DYNAMIC
> >> > >> > to avoid duplicated device registration.
> >> > >>
> >> > >> Gah! my eyes!
> >> > >>
> >> > >> Don't do that. It is incredibly problematic. Look at inhibiting
> >> > >> duplicate device creation instead.
> >> > >
> >> > > I may not follow this thread correctly, but could anyone point out the
> >> > > above "inhibiting duplicate device creation" if there's already such
> >> > > solution?
> >> >
> >> > No, the solution doesn't exist yet, but it wouldn't be hard to
> >> > implement. What you need to do is to add a struct device pointer to
> >> > struct device_node, and set the pointer to the struct device when
> >> > of_platform_device_create creates a device. (it would also need to be
> >> > set for early_platform_device creation, but that's not something that
> >> > should affect you). You would also add a check to
> >> > of_platform_device_create to check if the device pointer is already
> >> > set. If it is, then skip creation of the device.
> >>
> >> Implemented as Grant suggested. At least this works for our case,
> >> where IOMMU needs to be instanciated earlier than other device[1].
> >> early_platform_device case still need to be covered.
> >
> > I think we arrived at a different conclusion in another branch of this
> > thread. With the patch below every driver needs to explicitly allocate
> > the platform device and set the struct device_node's .dev field, which
> > has other side-effects such as the device hierarchy getting messed up.
> >
> > A better alternative would be to have of_platform_populate() run early
> > such that the .dev field in the struct device_node can be set by core
> > code, which would not require every driver to be changed.
> >
> > I'm not sure exactly what needs to be done to make this work, however.
> > Grant, can you provide some guidance here as to how this may be fixed?
> > Where would we have to call of_platform_populate() from and what makes
> > this break with the current implementation?
> 
> Try it and see!  :-) I cannot give a definite answer, but I suspect
> that it will fall down when registering the devices on to the
> platform_bus because it isn't initialized yet. If called early, the
> code would need to hold the platform_device in some kind of deferred
> list until the platform bus was initialize, and then have a cleanup
> function at initcall time to finish the registration of all early
> devices.

So what I did is move the call to of_platform_populate() on Tegra from
.init_machine() to .init_early(). The problem with .init_early() is that
the Slab allocator isn't available yet. One can use bootmem to work
around that, but it involves sprinkling a lot of slab_is_available() and
alloc_bootmem() where otherwise just kmalloc() (or one of its variants)
would be called.

What I ended up doing was implement early_kmalloc() and early_kfree() to
replace some occurrences in code that was called at some point from
of_platform_populate(). That got me quite far, though I haven't fully
completed the conversion. I'm not sure if it's really worth it either
because that code is unlikely to ever get merged.

Instead I'll try to move the call to of_platform_populate() shortly
after the slab has been initialized, which happens to be around the time
that .init_irq() is called and see if that makes things any easier.

Thierry

Patch

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 5c54279..99062dd 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -230,6 +230,29 @@  const void *of_get_property(const struct device_node *np, const char *name,
 }
 EXPORT_SYMBOL(of_get_property);
 
+struct device *of_get_device(const struct device_node *node)
+{
+	struct device *dev;
+	unsigned long flags;
+
+	raw_spin_lock_irqsave(&devtree_lock, flags);
+	dev = node->dev;
+	raw_spin_unlock_irqrestore(&devtree_lock, flags);
+
+	return dev;
+}
+EXPORT_SYMBOL(of_get_device);
+
+void of_set_device(struct device_node *node, struct device *dev)
+{
+	unsigned long flags;
+
+	raw_spin_lock_irqsave(&devtree_lock, flags);
+	node->dev = dev;
+	raw_spin_unlock_irqrestore(&devtree_lock, flags);
+}
+EXPORT_SYMBOL(of_set_device);
+
 /** Checks if the given "compat" string matches one of the strings in
  * the device's "compatible" property
  */
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index e0a6514..a8f6b09 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -203,10 +203,17 @@  struct platform_device *of_platform_device_create_pdata(
 					struct device *parent)
 {
 	struct platform_device *dev;
+	struct device *tmp;
 
 	if (!of_device_is_available(np))
 		return NULL;
 
+	tmp = of_get_device(np);
+	if (tmp) {
+		dev_info(tmp, "Already populated\n");
+		return to_platform_device(tmp);
+	}
+
 	dev = of_device_alloc(np, bus_id, parent);
 	if (!dev)
 		return NULL;
@@ -228,6 +235,7 @@  struct platform_device *of_platform_device_create_pdata(
 		return NULL;
 	}
 
+	of_set_device(np, &dev->dev);
 	return dev;
 }
 
diff --git a/include/linux/of.h b/include/linux/of.h
index 1fd08ca..b548522 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -60,6 +60,9 @@  struct device_node {
 	struct	kref kref;
 	unsigned long _flags;
 	void	*data;
+
+	struct device *dev;		/* Set only after populated */
+
 #if defined(CONFIG_SPARC)
 	const char *path_component_name;
 	unsigned int unique_id;
@@ -268,6 +271,8 @@  extern const void *of_get_property(const struct device_node *node,
 				int *lenp);
 #define for_each_property_of_node(dn, pp) \
 	for (pp = dn->properties; pp != NULL; pp = pp->next)
+extern struct device *of_get_device(const struct device_node *node);
+extern void of_set_device(struct device_node *node, struct device *dev);
 
 extern int of_n_addr_cells(struct device_node *np);
 extern int of_n_size_cells(struct device_node *np);
@@ -459,6 +464,17 @@  static inline const void *of_get_property(const struct device_node *node,
 	return NULL;
 }
 
+static inline struct device *of_get_device(const struct device_node *node)
+{
+	return NULL;
+}
+
+static inline void of_set_device(const struct device_node *node,
+				 struct device *dev);
+{
+	return -ENOSYS;
+}
+
 static inline int of_property_read_u64(const struct device_node *np,
 				       const char *propname, u64 *out_value)
 {