Re: [PATCH v6 1/8] iommu: provide early initialisation hook for IOMMU drivers
On Fri, Dec 5, 2014 at 12:35 PM, Robin Murphy robin.mur...@arm.com wrote: Hi Will, On 05/12/14 12:10, Will Deacon wrote: [...] Do you expect drivers to modify that *priv pointer after the ops structure is registered? I'd be very surprised if that was the use case. It's fine for the driver to register a non-const version, but once it is registered, the infrastructure can treat it as const from then on. Possibly not - certainly my current port of the ARM SMMU which makes use of *priv is only ever reading it - although we did also wave around reasons for mutable ops like dynamically changing the pgsize_bitmap and possibly even swizzling individual ops for runtime reconfiguration. On consideration though, I'd agree that things like that are mad enough to stay well within individual drivers if they did ever happen, and certainly shouldn't apply to this bit of the infrastructure at any rate. I certainly need to update the pgsize_bitmap at runtime because I don't know the supported page sizes until I've both (a) probed the hardware and (b) allocated page tables for a domain. We've already discussed moving the pgsize_bitmap out of the ops, but moving it somewhere where it remains const doesn't really help. We can safely cast the call to get_ops in the SMMU driver though, since we'll know that we put a mutable per-instance ops in there in the first place. At least that way drivers that aren't taking advantage and just pass their static const ops around shouldn't provoke warnings. I deliberately didn't touch anything beyond get_ops as that would be too disruptive. Can I just take the patch that Grant acked, in the interest of getting something merged? As you say, there's plenty of planned changes in this area anyway. I plan to send Olof a pull request this afternoon. Grant, Thierry? Personally I'm not fussed either way - the sooner something goes in, the sooner I can carry on working at replacing it :D I've already acked it. Why are we still talking about it? :-D g. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 1/8] iommu: provide early initialisation hook for IOMMU drivers
On Fri, Dec 5, 2014 at 1:18 PM, Thierry Reding thierry.red...@gmail.com wrote: On Fri, Dec 05, 2014 at 01:06:52PM +, Grant Likely wrote: On Fri, Dec 5, 2014 at 12:35 PM, Robin Murphy robin.mur...@arm.com wrote: Hi Will, On 05/12/14 12:10, Will Deacon wrote: [...] Do you expect drivers to modify that *priv pointer after the ops structure is registered? I'd be very surprised if that was the use case. It's fine for the driver to register a non-const version, but once it is registered, the infrastructure can treat it as const from then on. Possibly not - certainly my current port of the ARM SMMU which makes use of *priv is only ever reading it - although we did also wave around reasons for mutable ops like dynamically changing the pgsize_bitmap and possibly even swizzling individual ops for runtime reconfiguration. On consideration though, I'd agree that things like that are mad enough to stay well within individual drivers if they did ever happen, and certainly shouldn't apply to this bit of the infrastructure at any rate. I certainly need to update the pgsize_bitmap at runtime because I don't know the supported page sizes until I've both (a) probed the hardware and (b) allocated page tables for a domain. We've already discussed moving the pgsize_bitmap out of the ops, but moving it somewhere where it remains const doesn't really help. We can safely cast the call to get_ops in the SMMU driver though, since we'll know that we put a mutable per-instance ops in there in the first place. At least that way drivers that aren't taking advantage and just pass their static const ops around shouldn't provoke warnings. I deliberately didn't touch anything beyond get_ops as that would be too disruptive. Can I just take the patch that Grant acked, in the interest of getting something merged? As you say, there's plenty of planned changes in this area anyway. I plan to send Olof a pull request this afternoon. Grant, Thierry? Personally I'm not fussed either way - the sooner something goes in, the sooner I can carry on working at replacing it :D I've already acked it. Why are we still talking about it? :-D Am I missing something? Why is there a need to rush things? Are there actually drivers that depend on this that will be merged during the 3.19 merge window? It seems like that'd be cutting it really close given where we are in the release cycle. If that's not the case, why even bother getting this hack into 3.19 if nobody uses it and we're going to change it in 3.20 anyway? I also acked the non-hack version, the patch that doesn't try to make everything const. I assumed that was the one that we are talking about merging. g. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 1/8] iommu: provide early initialisation hook for IOMMU drivers
On Thu, Dec 4, 2014 at 11:19 AM, Arnd Bergmann a...@arndb.de wrote:
On Thursday 04 December 2014 10:21:27 Will Deacon wrote:
On Thu, Dec 04, 2014 at 10:10:17AM +, Arnd Bergmann wrote:
On Thursday 04 December 2014 09:49:53 Will Deacon wrote:
On Wed, Dec 03, 2014 at 07:57:50PM +, Arnd Bergmann wrote:
On Tuesday 02 December 2014 14:16:57 Grant Likely wrote:
On Mon, Dec 1, 2014 at 11:54 PM, Rob Herring robherri...@gmail.com
wrote:
On Mon, Dec 1, 2014 at 10:57 AM, Will Deacon will.dea...@arm.com
wrote:
+static inline void of_iommu_set_ops(struct device_node *np,
+ const struct iommu_ops *ops)
+{
+ np-data = (struct iommu_ops *)ops;
+}
+
+static inline struct iommu_ops *of_iommu_get_ops(struct
device_node *np)
+{
+ return np-data;
+}
This may collide with other users. While use of it is rare, PPC
uses
it in its PCI code. The OF_DYNAMIC code frees it but never actually
sets it. There may be some coming usage with the DT overlay code or
that's just a bug. Pantelis or Grant can comment. If not, I think
we
really should try to get rid of this pointer rather than expand
it's
usage.
I didn't see a user of this. I'm guessing that is coming in a SMMU
patch?
Good catch. This is not good. The data pointer should be avoided
since
there are no controls over its use. Until a better solution can be
implemented, probably the safest thing to do is add a struct
iommu_ops
pointer to struct device_node. However, assuming that only a small
portion of nodes will actually have iommu_ops set, I'd rather see a
separate registry that matches device_nodes to iommu_ops.
Fair enough. Will, can you take a copy of drivers/dma/of-dma.c and
adapt it as needed? It should be exactly what we need to start
out and can be extended and generalized later.
Sure, I'll add this to my list of stuff to do for 3.20.
Does that mean the we don't get any of the patches for 3.19 despite the
Acks?
Hmm, I don't know how useful they are without the get/set ops and I don't
think I can get those ready for 3.19 given where we currently are.
Grant's suggestion of adding an iommu_ops pointer to device_node would work
as a temporary hack, but anything more advanced is going to need proper
review.
Right. I guess it doesn't hurt much if we put the new pointer inside
#ifdef CONFIG_OF_IOMMU, then at least there is no significant size
increase in most DT based platforms.
Yes, I can live with that hack on the proviso that it will be removed by v3.20
Oh, and please put an ugly /* */ comment block in the #ifdef
CONFIG_OF_IOMMU section that makes it really clear that it is an ugly
hack and will be removed in the next release. I don't want anyone
getting ideas that adding pointers to struct device_node is a good
idea.
g.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 1/8] iommu: provide early initialisation hook for IOMMU drivers
On Thu, Dec 4, 2014 at 12:26 PM, Robin Murphy robin.mur...@arm.com wrote:
Hi Arnd,
On 03/12/14 19:57, Arnd Bergmann wrote:
[...]
Good catch. This is not good. The data pointer should be avoided since
there are no controls over its use. Until a better solution can be
implemented, probably the safest thing to do is add a struct iommu_ops
pointer to struct device_node. However, assuming that only a small
portion of nodes will actually have iommu_ops set, I'd rather see a
separate registry that matches device_nodes to iommu_ops.
Fair enough. Will, can you take a copy of drivers/dma/of-dma.c and
adapt it as needed? It should be exactly what we need to start
out and can be extended and generalized later.
I'm quite keen to see this series go in, since I'm depending on it to make
arm64 IOMMU DMA ops just work. Will and I came to the conclusion the other
day that we pretty much need to build up some kind of bus abstraction based
on the probe data in order to be able to assign IOMMU groups correctly,
which can also subsume this particular problem in the long run. Since I've
made a start on that already, I've hacked the following short-term fix out
of it. Tested on my Juno - admittedly with only two SMMUs and one master
(EHCI) being probed, but it didn't blow up or regress anything.
Regards,
Robin.
---8---
From 1f3d2612682c239e53f2c20e2ac5d19ef3f5387c Mon Sep 17 00:00:00 2001
Message-Id:
1f3d2612682c239e53f2c20e2ac5d19ef3f5387c.1417695078.git.robin.mur...@arm.com
From: Robin Murphy robin.mur...@arm.com
Date: Thu, 4 Dec 2014 11:53:13 +
Subject: [PATCH] iommu: store DT-probed IOMMU data privately
Since the data pointer in the DT node is public and may be overwritten
by conflicting code, move the DT-probed IOMMU ops to a private list
where they will be safe.
Signed-off-by: Robin Murphy robin.mur...@arm.com
Looks reasonable to me. Comments below...
---
drivers/iommu/of_iommu.c | 38 ++
include/linux/of_iommu.h | 12 ++--
2 files changed, 40 insertions(+), 10 deletions(-)
diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index 73236d3..5cd451c 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -94,6 +94,44 @@ int of_get_dma_window(struct device_node *dn, const char
*prefix, int index,
}
EXPORT_SYMBOL_GPL(of_get_dma_window);
+struct of_iommu_node {
+ struct hlist_node list;
+ struct device_node *np;
+ const struct iommu_ops *ops;
+};
+static HLIST_HEAD(of_iommu_list);
Just use a list_head. hlist_head merely saves one pointer in this case.
+static DEFINE_SPINLOCK(of_iommu_lock);
+
+void of_iommu_set_ops(struct device_node *np, const struct iommu_ops *ops)
+{
+ struct of_iommu_node *iommu = kmalloc(sizeof(*iommu), GFP_KERNEL);
kzalloc()
+
+ if (!iommu)
+ return;
Shouldn't there be a WARN() here on failure? I don't think failing
silently is desired.
+
+ INIT_HLIST_NODE(iommu-list);
+ iommu-np = np;
+ iommu-ops = ops;
+ spin_lock(of_iommu_lock);
+ hlist_add_head(iommu-list, of_iommu_list);
+ spin_unlock(of_iommu_lock);
+}
+
+struct iommu_ops *of_iommu_get_ops(struct device_node *np)
+{
+ struct of_iommu_node *node;
+ const struct iommu_ops *ops = NULL;
+
+ spin_lock(of_iommu_lock);
+ hlist_for_each_entry(node, of_iommu_list, list)
+ if (node-np == np) {
+ ops = node-ops;
+ break;
+ }
+ spin_unlock(of_iommu_lock);
+ return (struct iommu_ops *)ops;
The cast looks fishy. If you need to use a cast, then the data types
are probably wrong. If you drop the const from *ops here and in the
structure then it should probably work fine. Due to the way it is
being used, there isn't any advantage to using const (unless you
changes of_iommu_get_ops() to return a const pointer, then const would
make sense).
+}
+
struct iommu_ops *of_iommu_configure(struct device *dev)
{
struct of_phandle_args iommu_spec;
diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h
index d03abbb..e27c53a 100644
--- a/include/linux/of_iommu.h
+++ b/include/linux/of_iommu.h
@@ -31,16 +31,8 @@ static inline struct iommu_ops *of_iommu_configure(struct
device *dev)
#endif /* CONFIG_OF_IOMMU */
-static inline void of_iommu_set_ops(struct device_node *np,
- const struct iommu_ops *ops)
-{
- np-data = (struct iommu_ops *)ops;
-}
-
-static inline struct iommu_ops *of_iommu_get_ops(struct device_node *np)
-{
- return np-data;
-}
+void of_iommu_set_ops(struct device_node *np, const struct iommu_ops *ops);
+struct iommu_ops *of_iommu_get_ops(struct device_node *np);
extern struct of_device_id __iommu_of_table;
--
1.9.1
___
iommu mailing list
Re: [PATCH v6 1/8] iommu: provide early initialisation hook for IOMMU drivers
On Thu, Dec 4, 2014 at 11:52 AM, Will Deacon will.dea...@arm.com wrote:
On Thu, Dec 04, 2014 at 11:25:35AM +, Grant Likely wrote:
On Thu, Dec 4, 2014 at 11:19 AM, Arnd Bergmann a...@arndb.de wrote:
On Thursday 04 December 2014 10:21:27 Will Deacon wrote:
Sure, I'll add this to my list of stuff to do for 3.20.
Does that mean the we don't get any of the patches for 3.19 despite the
Acks?
Hmm, I don't know how useful they are without the get/set ops and I don't
think I can get those ready for 3.19 given where we currently are.
Grant's suggestion of adding an iommu_ops pointer to device_node would
work
as a temporary hack, but anything more advanced is going to need proper
review.
Right. I guess it doesn't hurt much if we put the new pointer inside
#ifdef CONFIG_OF_IOMMU, then at least there is no significant size
increase in most DT based platforms.
Yes, I can live with that hack on the proviso that it will be removed by
v3.20
Oh, and please put an ugly /* */ comment block in the #ifdef
CONFIG_OF_IOMMU section that makes it really clear that it is an ugly
hack and will be removed in the next release. I don't want anyone
getting ideas that adding pointers to struct device_node is a good
idea.
Something like the mess below?
Yes... Although it looks like Robin's patch does what is needed
without the hack.
g.
Will
---8
diff --git a/include/linux/of.h b/include/linux/of.h
index 29f0adc5f3e4..6f85c02bc1a6 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -43,6 +43,9 @@ struct property {
#if defined(CONFIG_SPARC)
struct of_irq_controller;
#endif
+#ifdef CONFIG_OF_IOMMU
+struct iommu_ops;
+#endif
struct device_node {
const char *name;
@@ -65,6 +68,19 @@ struct device_node {
unsigned int unique_id;
struct of_irq_controller *irq_trans;
#endif
+#ifdef CONFIG_OF_IOMMU
+/*
+ * HACK! HACK! HACK!
+ *
+ * This is a temporary hack to associate a device_node for an
+ * IOMMU with its set of iommu_ops so that we can probe its upstream DMA
+ * masters on the platform bus by parsing the iommus property directly.
+ *
+ * This is going away in 3.20. Please use the of_iommu_{get,set}_ops
+ * functions to get hold of this data.
+ */
+ struct iommu_ops *__iommu_ops_use_accessors;
+#endif
};
#define MAX_PHANDLE_ARGS 16
diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h
index d03abbb11c34..392ec5f212db 100644
--- a/include/linux/of_iommu.h
+++ b/include/linux/of_iommu.h
@@ -14,6 +14,17 @@ extern int of_get_dma_window(struct device_node *dn, const
char *prefix,
extern void of_iommu_init(void);
extern struct iommu_ops *of_iommu_configure(struct device *dev);
+static inline void of_iommu_set_ops(struct device_node *np,
+ const struct iommu_ops *ops)
+{
+ np-__iommu_ops_use_accessors = ops;
+}
+
+static inline struct iommu_ops *of_iommu_get_ops(struct device_node *np)
+{
+ return np-__iommu_ops_use_accessors;
+}
+
#else
static inline int of_get_dma_window(struct device_node *dn, const char
*prefix,
@@ -29,19 +40,15 @@ static inline struct iommu_ops *of_iommu_configure(struct
device *dev)
return NULL;
}
-#endif /* CONFIG_OF_IOMMU */
-
static inline void of_iommu_set_ops(struct device_node *np,
- const struct iommu_ops *ops)
-{
- np-data = (struct iommu_ops *)ops;
-}
-
+ const struct iommu_ops *ops) { }
static inline struct iommu_ops *of_iommu_get_ops(struct device_node *np)
{
- return np-data;
+ return NULL;
}
+#endif /* CONFIG_OF_IOMMU */
+
extern struct of_device_id __iommu_of_table;
typedef int (*of_iommu_init_fn)(struct device_node *);
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 1/8] iommu: provide early initialisation hook for IOMMU drivers
On Thu, Dec 4, 2014 at 1:43 PM, Robin Murphy robin.mur...@arm.com wrote:
Hi Grant, thanks for the advice - silly micro-optimisations removed, and
I'll make a note to do so from my in-development code, too ;)
I didn't much like the casting either, so rather than push it elsewhere or
out to the caller I've just changed the prototype to obviate it completely.
Since we're also expecting to churn this again to use something more
suitable than iommu_ops as the private data, I think keeping things simple
wins over const-correctness for now.
Thanks,
Robin
---8---
From b2e8c91ac49bef4008661e4628cd6b7249d84af5 Mon Sep 17 00:00:00 2001
Message-Id:
b2e8c91ac49bef4008661e4628cd6b7249d84af5.1417698001.git.robin.mur...@arm.com
From: Robin Murphy robin.mur...@arm.com
Date: Thu, 4 Dec 2014 11:53:13 +
Subject: [PATCH v2] iommu: store DT-probed IOMMU data privately
Since the data pointer in the DT node is public and may be overwritten
by conflicting code, move the DT-probed IOMMU ops to a private list
where they will be safe.
Signed-off-by: Robin Murphy robin.mur...@arm.com
Acked-by: Grant Likely grant.lik...@linaro.org
---
drivers/iommu/of_iommu.c | 40
include/linux/of_iommu.h | 12 ++--
2 files changed, 42 insertions(+), 10 deletions(-)
diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index 73236d3..c7078f6 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -94,6 +94,46 @@ int of_get_dma_window(struct device_node *dn, const char
*prefix, int index,
}
EXPORT_SYMBOL_GPL(of_get_dma_window);
+struct of_iommu_node {
+ struct list_head list;
+ struct device_node *np;
+ struct iommu_ops *ops;
+};
+static LIST_HEAD(of_iommu_list);
+static DEFINE_SPINLOCK(of_iommu_lock);
+
+void of_iommu_set_ops(struct device_node *np, struct iommu_ops *ops)
+{
+ struct of_iommu_node *iommu = kzalloc(sizeof(*iommu), GFP_KERNEL);
+
+ if (!iommu) {
+ __WARN();
+ return;
+ }
+
+ INIT_LIST_HEAD(iommu-list);
+ iommu-np = np;
+ iommu-ops = ops;
+ spin_lock(of_iommu_lock);
+ list_add_tail(iommu-list, of_iommu_list);
+ spin_unlock(of_iommu_lock);
+}
+
+struct iommu_ops *of_iommu_get_ops(struct device_node *np)
+{
+ struct of_iommu_node *node;
+ struct iommu_ops *ops = NULL;
+
+ spin_lock(of_iommu_lock);
+ list_for_each_entry(node, of_iommu_list, list)
+ if (node-np == np) {
+ ops = node-ops;
+ break;
+ }
+ spin_unlock(of_iommu_lock);
+ return ops;
+}
+
struct iommu_ops *of_iommu_configure(struct device *dev)
{
struct of_phandle_args iommu_spec;
diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h
index d03abbb..16c7554 100644
--- a/include/linux/of_iommu.h
+++ b/include/linux/of_iommu.h
@@ -31,16 +31,8 @@ static inline struct iommu_ops *of_iommu_configure(struct
device *dev)
#endif /* CONFIG_OF_IOMMU */
-static inline void of_iommu_set_ops(struct device_node *np,
- const struct iommu_ops *ops)
-{
- np-data = (struct iommu_ops *)ops;
-}
-
-static inline struct iommu_ops *of_iommu_get_ops(struct device_node *np)
-{
- return np-data;
-}
+void of_iommu_set_ops(struct device_node *np, struct iommu_ops *ops);
+struct iommu_ops *of_iommu_get_ops(struct device_node *np);
extern struct of_device_id __iommu_of_table;
--
1.9.1
On 04/12/14 12:42, Grant Likely wrote:
On Thu, Dec 4, 2014 at 12:26 PM, Robin Murphy robin.mur...@arm.com
wrote:
Hi Arnd,
On 03/12/14 19:57, Arnd Bergmann wrote:
[...]
Good catch. This is not good. The data pointer should be avoided since
there are no controls over its use. Until a better solution can be
implemented, probably the safest thing to do is add a struct iommu_ops
pointer to struct device_node. However, assuming that only a small
portion of nodes will actually have iommu_ops set, I'd rather see a
separate registry that matches device_nodes to iommu_ops.
Fair enough. Will, can you take a copy of drivers/dma/of-dma.c and
adapt it as needed? It should be exactly what we need to start
out and can be extended and generalized later.
I'm quite keen to see this series go in, since I'm depending on it to
make
arm64 IOMMU DMA ops just work. Will and I came to the conclusion the
other
day that we pretty much need to build up some kind of bus abstraction
based
on the probe data in order to be able to assign IOMMU groups correctly,
which can also subsume this particular problem in the long run. Since
I've
made a start on that already, I've hacked the following short-term fix
out
of it. Tested on my Juno - admittedly with only two SMMUs and one master
(EHCI) being probed, but it didn't blow up or regress anything.
Regards,
Robin
Re: [PATCH v6 1/8] iommu: provide early initialisation hook for IOMMU drivers
On Mon, Dec 1, 2014 at 11:54 PM, Rob Herring robherri...@gmail.com wrote:
Adding Grant and Pantelis...
On Mon, Dec 1, 2014 at 10:57 AM, Will Deacon will.dea...@arm.com wrote:
IOMMU drivers must be initialised before any of their upstream devices,
otherwise the relevant iommu_ops won't be configured for the bus in
question. To solve this, a number of IOMMU drivers use initcalls to
initialise the driver before anything has a chance to be probed.
Whilst this solves the immediate problem, it leaves the job of probing
the IOMMU completely separate from the iommu_ops to configure the IOMMU,
which are called on a per-bus basis and require the driver to figure out
exactly which instance of the IOMMU is being requested. In particular,
the add_device callback simply passes a struct device to the driver,
which then has to parse firmware tables or probe buses to identify the
relevant IOMMU instance.
This patch takes the first step in addressing this problem by adding an
early initialisation pass for IOMMU drivers, giving them the ability to
store some per-instance data in their iommu_ops structure and store that
in their of_node. This can later be used when parsing OF masters to
identify the IOMMU instance in question.
Acked-by: Arnd Bergmann a...@arndb.de
Acked-by: Joerg Roedel jroe...@suse.de
Acked-by: Marek Szyprowski m.szyprow...@samsung.com
Tested-by: Robin Murphy robin.mur...@arm.com
Signed-off-by: Will Deacon will.dea...@arm.com
---
drivers/iommu/of_iommu.c | 17 +
include/asm-generic/vmlinux.lds.h | 2 ++
include/linux/iommu.h | 2 ++
include/linux/of_iommu.h | 25 +
4 files changed, 46 insertions(+)
diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index e550ccb7634e..89b903406968 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -22,6 +22,9 @@
#include linux/of.h
#include linux/of_iommu.h
+static const struct of_device_id __iommu_of_table_sentinel
+ __used __section(__iommu_of_table_end);
+
/**
* of_get_dma_window - Parse *dma-window property and returns 0 if found.
*
@@ -89,3 +92,17 @@ int of_get_dma_window(struct device_node *dn, const char
*prefix, int index,
return 0;
}
EXPORT_SYMBOL_GPL(of_get_dma_window);
+
+void __init of_iommu_init(void)
+{
+ struct device_node *np;
+ const struct of_device_id *match, *matches = __iommu_of_table;
+
+ for_each_matching_node_and_match(np, matches, match) {
+ const of_iommu_init_fn init_fn = match-data;
+
+ if (init_fn(np))
+ pr_err(Failed to initialise IOMMU %s\n,
+ of_node_full_name(np));
+ }
+}
diff --git a/include/asm-generic/vmlinux.lds.h
b/include/asm-generic/vmlinux.lds.h
index aa70cbda327c..bee5d683074d 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -164,6 +164,7 @@
#define CLKSRC_OF_TABLES() OF_TABLE(CONFIG_CLKSRC_OF, clksrc)
#define IRQCHIP_OF_MATCH_TABLE() OF_TABLE(CONFIG_IRQCHIP, irqchip)
#define CLK_OF_TABLES()OF_TABLE(CONFIG_COMMON_CLK, clk)
+#define IOMMU_OF_TABLES() OF_TABLE(CONFIG_OF_IOMMU, iommu)
#define RESERVEDMEM_OF_TABLES()OF_TABLE(CONFIG_OF_RESERVED_MEM,
reservedmem)
#define CPU_METHOD_OF_TABLES() OF_TABLE(CONFIG_SMP, cpu_method)
#define EARLYCON_OF_TABLES() OF_TABLE(CONFIG_SERIAL_EARLYCON, earlycon)
@@ -497,6 +498,7 @@
CLK_OF_TABLES() \
RESERVEDMEM_OF_TABLES() \
CLKSRC_OF_TABLES() \
+ IOMMU_OF_TABLES() \
CPU_METHOD_OF_TABLES() \
KERNEL_DTB()\
IRQCHIP_OF_MATCH_TABLE()\
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index e6a7c9ff72f2..7b83f9f8e11d 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -103,6 +103,7 @@ enum iommu_attr {
* @domain_get_attr: Query domain attributes
* @domain_set_attr: Change domain attributes
* @pgsize_bitmap: bitmap of supported page sizes
+ * @priv: per-instance data private to the iommu driver
*/
struct iommu_ops {
bool (*capable)(enum iommu_cap);
@@ -133,6 +134,7 @@ struct iommu_ops {
u32 (*domain_get_windows)(struct iommu_domain *domain);
unsigned long pgsize_bitmap;
+ void *priv;
};
#define IOMMU_GROUP_NOTIFY_ADD_DEVICE 1 /* Device added */
diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h
index 51a560f34bca..5762cdc8effe 100644
--- a/include/linux/of_iommu.h
+++ b/include/linux/of_iommu.h
@@ -1,12 +1,17 @@
#ifndef __OF_IOMMU_H
Re: [PATCHv6+ 01/13] of: introduce of_property_for_earch_phandle_with_args()
On Wed, 11 Dec 2013 14:33:38 +0100, Hiroshi Doyu hd...@nvidia.com wrote:
Hi Grant,
Grant Likely grant.lik...@linaro.org wrote @ Wed, 11 Dec 2013 14:28:45
+0100:
On Thu, 21 Nov 2013 11:57:00 -0700, Stephen Warren swar...@wwwdotorg.org
wrote:
On 11/21/2013 10:17 AM, Hiroshi Doyu wrote:
Iterating over a property containing a list of phandles with arguments
is a common operation for device drivers. This patch adds a new
of_property_for_each_phandle_with_args() macro to make the iteration
simpler.
Signed-off-by: Hiroshi Doyu hd...@nvidia.com
---
v6+:
Use the description, which Grant Likely proposed, to be full enough
that a future reader can figure out why a patch was written.
http://lists.linuxfoundation.org/pipermail/iommu/2013-November/007062.html
...
That's right, I forgot I said that. Yes please fix the implementation.
Here's the latest. I'll include this with the next v7 series.
Can I get your Acked-by with this?
--8
From 8f7c0404aa68f0e8dbe0babc240590f6528ecc1f Mon Sep 17 00:00:00 2001
From: Hiroshi Doyu hd...@nvidia.com
Date: Fri, 15 Nov 2013 10:52:53 +0200
Subject: [PATCH] of: introduce of_property_for_each_phandle_with_args()
Iterating over a property containing a list of phandles with arguments
is a common operation for device drivers. This patch adds a new
of_property_for_each_phandle_with_args() macro to make the iteration
simpler.
Signed-off-by: Hiroshi Doyu hd...@nvidia.com
Cc: Rob Herring robherri...@gmail.com
---
v7:
Fixed some minors pointed by Rob and Stephen.
v6:
Iterate without intrducing a new struct.
v6+++:
Introduced a new struct of_phandle_iter to keep the state when
iterating over the list.
v6++:
Optimized to avoid O(n^2), suggested by Stephen Warren.
http://lists.linuxfoundation.org/pipermail/iommu/2013-November/007066.html
I didn't introduce any struct to hold params and state here.
v6+:
Use the description, which Grant Likely proposed, to be full enough
that a future reader can figure out why a patch was written.
v5:
New patch for v5.
Signed-off-by: Hiroshi Doyu hd...@nvidia.com
---
drivers/of/base.c | 46 ++
include/linux/of.h | 32
2 files changed, 78 insertions(+)
diff --git a/drivers/of/base.c b/drivers/of/base.c
index f807d0e..cd4ab05 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1201,6 +1201,52 @@ void of_print_phandle_args(const char *msg, const
struct of_phandle_args *args)
printk(\n);
}
+const __be32 *of_phandle_iter_next(const char *cells_name, int cell_count,
+const __be32 *cur, const __be32 *end,
+struct of_phandle_args *out_args)
Having to pass in cells_name, cell_count, cur and end each time seems a
little odd. Can a state structure be used instead?
struct of_phandle_iter_state {
const char *cells_name;
int cells_count;
const __be32 *cur;
const __be32 *end;
struct of_phandle_args out_args;
}
Make the caller provide one of those and fill it in with the init
function.
+{
+ struct device_node *dn;
+ int i;
+
+ if (!cells_name !cell_count)
+ return NULL;
+
+ if (!cur || (cur = end))
+ return NULL;
+
+ dn = of_find_node_by_phandle(be32_to_cpup(cur++));
+ if (!dn)
+ return NULL;
+
+ if (cells_name)
+ if (of_property_read_u32(dn, cells_name, cell_count))
+ return NULL;
+
+ out_args-np = dn;
+ out_args-args_count = cell_count;
+ for (i = 0; i cell_count; i++)
+ out_args-args[i] = be32_to_cpup(cur++);
+
+ return cur;
+}
+EXPORT_SYMBOL_GPL(of_phandle_iter_next);
+
+const __be32 *of_phandle_iter_init(const struct device_node *np,
+const char *list_name,
+const __be32 **end)
+{
+ size_t bytes;
+ const __be32 *cur;
+
+ cur = of_get_property(np, list_name, bytes);
+ *end = cur;
+ if (bytes)
+ *end += bytes / sizeof(*cur);
+
+ return cur;
+}
+EXPORT_SYMBOL_GPL(of_phandle_iter_init);
+
static int __of_parse_phandle_with_args(const struct device_node *np,
const char *list_name,
const char *cells_name,
diff --git a/include/linux/of.h b/include/linux/of.h
index 276c546..4345582 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -303,6 +303,14 @@ extern int of_parse_phandle_with_fixed_args(const struct
device_node *np,
extern int of_count_phandle_with_args(const struct device_node *np,
const char *list_name, const char *cells_name);
+extern const __be32 *of_phandle_iter_init(const struct device_node *np,
+ const char
Re: [PATCHv6+ 01/13] of: introduce of_property_for_earch_phandle_with_args()
On Thu, 21 Nov 2013 18:17:20 +0100, Hiroshi Doyu hd...@nvidia.com wrote: Iterating over a property containing a list of phandles with arguments is a common operation for device drivers. This patch adds a new of_property_for_each_phandle_with_args() macro to make the iteration simpler. Signed-off-by: Hiroshi Doyu hd...@nvidia.com Acked-by: Grant Likely grant.lik...@linaro.org This patch can be merged with the rest of the series. g. --- v6+: Use the description, which Grant Likely proposed, to be full enough that a future reader can figure out why a patch was written. http://lists.linuxfoundation.org/pipermail/iommu/2013-November/007062.html v5: New patch for v5. --- include/linux/of.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/include/linux/of.h b/include/linux/of.h index 276c546..131fef5 100644 --- a/include/linux/of.h +++ b/include/linux/of.h @@ -613,6 +613,9 @@ static inline int of_property_read_u32(const struct device_node *np, s; \ s = of_prop_next_string(prop, s)) +#define of_property_for_each_phandle_with_args(np, list, cells, i, args) \ + for (i = 0; !of_parse_phandle_with_args(np, list, cells, i, args); i++) + #if defined(CONFIG_PROC_FS) defined(CONFIG_PROC_DEVICETREE) extern void proc_device_tree_add_node(struct device_node *, struct proc_dir_entry *); extern void proc_device_tree_add_prop(struct proc_dir_entry *pde, struct property *prop); -- 1.8.1.5 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCHv6+ 01/13] of: introduce of_property_for_earch_phandle_with_args()
On Thu, 21 Nov 2013 11:57:00 -0700, Stephen Warren swar...@wwwdotorg.org wrote: On 11/21/2013 10:17 AM, Hiroshi Doyu wrote: Iterating over a property containing a list of phandles with arguments is a common operation for device drivers. This patch adds a new of_property_for_each_phandle_with_args() macro to make the iteration simpler. Signed-off-by: Hiroshi Doyu hd...@nvidia.com --- v6+: Use the description, which Grant Likely proposed, to be full enough that a future reader can figure out why a patch was written. http://lists.linuxfoundation.org/pipermail/iommu/2013-November/007062.html This new version only addresses one of the concerns that Grant had, namely the commit message. diff --git a/include/linux/of.h b/include/linux/of.h +#define of_property_for_each_phandle_with_args(np, list, cells, i, args) \ + for (i = 0; !of_parse_phandle_with_args(np, list, cells, i, args); i++) + Grant also wanted the actual implementation fixed so that it wasn't so inefficient. What this current patch does is basically: for every entry in the property: for every entry in the property before the current index: parse the phandle+specifier That's roughly O(n^2). (n is # entries in the property) Instead, what should happen is: for every entry in the property: parse the phandle+specifier yield the result That's roughly O(n). In other words, an implementation more along the lines of include/linux/of.h's: #define of_property_for_each_u32(np, propname, prop, p, u) \ for (prop = of_find_property(np, propname, NULL), \ p = of_prop_next_u32(prop, NULL, u); \ p; \ p = of_prop_next_u32(prop, p, u)) ... so you'd need functions like of_prop_first_specifier() and of_prop_next_specifier(), and perhaps some associated set of state variables, perhaps with all the state wrapped into a single struct for simplicity. That's right, I forgot I said that. Yes please fix the implementation. g. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCHv5 1/9] of: introduce of_property_for_earch_phandle_with_args()
On Thu, 21 Nov 2013 15:12:18 +0200, Hiroshi Doyu hd...@nvidia.com wrote:
On Thu, 21 Nov 2013 13:43:28 +0100
Grant Likely grant.lik...@linaro.org wrote:
On Tue, 19 Nov 2013 11:33:05 +0200, Hiroshi Doyu hd...@nvidia.com wrote:
The following pattern of code is tempting:
for (i = 0; !of_parse_phandle_with_args(np, list, cells, i, args); i++)
Signed-off-by: Hiroshi Doyu hd...@nvidia.com
That's a very minimal commit message. Can you elaborate please.
The above can be:
The following pattern of code is tempting to add a new member for
of_property_for_each_*() family as an idiom.
for (i = 0;
!of_parse_phandle_with_args(np, list, cells, i, args); i++)
do something with args;
I really do like commit messages to be full enough that a future reader
can figure out why a patch was written. ie:
Iterating over a property containing a list of phandles with
arguments is a common operation for device drivers. This patch
adds a new of_property_for_each_phandle_with_args() macro to
make the iteration simpler.
g.
Actual usage is here:
int i;
struct of_phandle_args args;
of_property_for_each_phandle_with_args(dev-of_node, iommus,
#iommu-cells, i, args) {
pr_debug(%s(i=%d) %s\n, __func__, i, dev_name(dev));
if (!of_find_iommu_by_node(args.np))
return -EPROBE_DEFER;
Is this acceptable?
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCHv5 1/9] of: introduce of_property_for_earch_phandle_with_args()
On Tue, 19 Nov 2013 11:33:05 +0200, Hiroshi Doyu hd...@nvidia.com wrote: The following pattern of code is tempting: for (i = 0; !of_parse_phandle_with_args(np, list, cells, i, args); i++) Signed-off-by: Hiroshi Doyu hd...@nvidia.com That's a very minimal commit message. Can you elaborate please. --- v5: New patch for v5. --- include/linux/of.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/include/linux/of.h b/include/linux/of.h index 276c546..131fef5 100644 --- a/include/linux/of.h +++ b/include/linux/of.h @@ -613,6 +613,9 @@ static inline int of_property_read_u32(const struct device_node *np, s; \ s = of_prop_next_string(prop, s)) +#define of_property_for_each_phandle_with_args(np, list, cells, i, args) \ + for (i = 0; !of_parse_phandle_with_args(np, list, cells, i, args); i++) + That works, but pretty darn inefficient. We need an iterator version of of_parse_phandle_with_args() to loop over all the entries. #if defined(CONFIG_PROC_FS) defined(CONFIG_PROC_DEVICETREE) extern void proc_device_tree_add_node(struct device_node *, struct proc_dir_entry *); extern void proc_device_tree_add_prop(struct proc_dir_entry *pde, struct property *prop); -- 1.8.1.5 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCHv5 2/9] driver/core: populate devices in order for IOMMUs
On Thu, 21 Nov 2013 12:04:18 -0700, Stephen Warren swar...@wwwdotorg.org wrote: On 11/21/2013 06:15 AM, Grant Likely wrote: On Tue, 19 Nov 2013 11:33:06 +0200, Hiroshi Doyu hd...@nvidia.com wrote: IOMMU devices on the bus need to be poplulated first, then iommu master devices are done later. With CONFIG_OF_IOMMU, iommus= DT binding would be used to identify whether a device can be an iommu msater or not. If a device can, we'll defer to populate that device till an iommu device is populated. Once an iommu device is populated, dev-bus-iommu_ops is set in the bus. Then, those defered iommu master devices are populated and configured for IOMMU with help of the already populated iommu device via iommu_ops-add_device(). Multiple IOMMUs can be listed on this iommus binding so that a device can have multiple IOMMUs attached. Signed-off-by: Hiroshi Doyu hd...@nvidia.com --- v5: Use iommus= binding instread of arm,smmu's #stream-id-cells. v4: This is newly added, and the successor of the following RFC: [RFC][PATCHv3+ 1/2] driver/core: Add of_iommu_attach() http://lists.linuxfoundation.org/pipermail/iommu/2013-November/006914.html --- drivers/base/dd.c| 5 + drivers/iommu/of_iommu.c | 22 ++ include/linux/of_iommu.h | 7 +++ 3 files changed, 34 insertions(+) diff --git a/drivers/base/dd.c b/drivers/base/dd.c index 35fa368..6e892d4 100644 --- a/drivers/base/dd.c +++ b/drivers/base/dd.c @@ -25,6 +25,7 @@ #include linux/async.h #include linux/pm_runtime.h #include linux/pinctrl/devinfo.h +#include linux/of_iommu.h #include base.h #include power/power.h @@ -273,6 +274,10 @@ static int really_probe(struct device *dev, struct device_driver *drv) dev-driver = drv; + ret = of_iommu_attach(dev); + if (ret) + goto probe_failed; + /* If using pinctrl, bind pins now before probing */ ret = pinctrl_bind_pins(dev); if (ret) I'm very concerned about this approach. Hooking into the core probe path for things like this is not going to scale well. I'm not thrilled with the pinctrl hook being here either, but that is already merged. :-( Also, hooking in here is going affect every single device device driver probe path, and a large number of them are never, ever, going to have iommu interactions. There needs to be a less invasive way of doing what you want. I still feel like the individual device drivers themselves need to be aware that they might be hooking through an IOMMU. Or, if they are hooking through a bus like PCIe, then have the PCIe bus defer creating child devices until the IOMMU is configured and in place. I general though, couldn't any MMIO on-SoC device potentially be affected by an IOMMU? The point of putting the call to of_iommu_attach() here rather than inside individual driver's probe() is to avoid requiring every driver having to paste more boiler-plate into probe(). It seems more that IOMMU attachment is closer to being a property of the bus rather than a property of the device itself. In that context it would make more sense for the bus device to hold off child device registration or probing until the IOMMU is available. That keeps the logic out of both the core code and the individual device drivers. g. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCHv3 01/19] [HACK] of: dev_node has struct device pointer
On Wed, 30 Oct 2013 15:58:58 -0600, Stephen Warren swar...@wwwdotorg.org wrote: On 10/25/2013 03:11 AM, Thierry Reding wrote: ... So my proposed solution for the IOMMU case is to treat it the same as any other resources. Perhaps resource isn't the right word, but at the core the issue is the same. A device requires the services of an IOMMU so that it can be put into the correct address space. If the IOMMU is not available yet it cannot do that, so we simply return -EPROBE_DEFER and cause the probe to be retried later. Personally, I view deferred probe as being used when one device requires either a resource /or/ a service provided by another, not /just/ when there's a resource dependency. Hence, I think it fits perfectly here. How are those two things different? :-) g. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCHv3 01/19] [HACK] of: dev_node has struct device pointer
On Mon, 28 Oct 2013 08:31:34 +0100, Thierry Reding thierry.red...@gmail.com wrote: On Fri, Oct 25, 2013 at 08:01:36PM +0100, Grant Likely wrote: On Fri, 25 Oct 2013 12:49:38 +0200, Thierry Reding thierry.red...@gmail.com wrote: On Fri, Oct 25, 2013 at 11:49:05AM +0200, Hiroshi Doyu wrote: Thierry Reding thierry.red...@gmail.com wrote @ Fri, 25 Oct 2013 11:11:05 +0200: This is actually the other problem that I'm aware of that could benefit from [interrupt resolution at probe time]. My idea was that once we had a way within the driver core to resolve interrupt references at probe time it could be used for potentially many other resources as well. Some of the resources like GPIOs and regulators are obviously not something that the core can or should be requesting, but mostly resources that you don't actually need to control after probing (such as interrupts) would be a good fit because otherwise people would write the same boilerplate over and over again. IOMMUs seem to me to be in that same category. As far as I can tell, an IOMMU driver registers the IOMMU for a given bus, upon which every device can simply be used (mostly transparently) with that IOMMU. While I haven't figured out how exactly, I'm pretty sure we can take advantage of the resolution of resources at probe time within the core to both keep drivers from having to do anything in particular and at the same time have code to determine if the IOMMU driver hasn't been probed yet and return -EPROBE_DEFER appropriately. Can you explain the above a bit more? Originally I thought that what Grant suggested would work ok with this patch. I think the objection to these patches is that they special case the instantiation of some devices. It's not a proper solution because it implies various things. For example merely instantiating the IOMMU device earlier than others is only going to work *if* the driver is actually probed before the drivers of other devices. If you want to build the driver as a module for example, probe order becomes entirely non-deterministic. I understand the above limitation. What I thought for the solution is that I can make use of iommu_bus even before IOMMU is instanciated. iommu_bus has its notifier which calls iommu_ops()-add_device(). This could return -EPROBE_DEFER when IOMMU isn't ready. Only the problem is the current bus_notifier doesn't have the ability to return error. I'll see if it can be modified. Even with this, at least IOMMU *driver* needs to be init'ed enough earlier to prevent devices from being registered without IOMMU. The way notifiers work is that they run completely hidden from whatever triggers them. For instance you register the IOMMU bus notifier from the IOMMU driver (by calling bus_set_iommu()). That registers a function to be called when some event happens on that bus. When a device's driver is probed successfully, the driver core will notify the bus, which causes the IOMMU callback to be run. Some of this code runs before the driver has successfully been probed, so I imagine it would be possible to use it to abort probing. But that's not possible at least with the current code. Instead of handling such dependencies implicitly by making sure all resource providers are probed earlier than any of their consumers, the dependencies are handled more explicitly, which turns out to simplify things a lot. There's some additional work required in the core, but if done consistently no driver needs to care about the dependencies and it no longer matters where the resources come from. The problem is reduced to essentially this: while (!resource_available()) load_more_drivers(); So my proposed solution for the IOMMU case is to treat it the same as any other resources. Perhaps resource isn't the right word, but at the core the issue is the same. A device requires the services of an IOMMU so that it can be put into the correct address space. If the IOMMU is not available yet it cannot do that, so we simply return -EPROBE_DEFER and cause the probe to be retried later. This looks somewhat similar to the above iommu_bus notifier. Is there any way to implement the same mechanism rather than using bus? Yes, I think it should be possible to get this to work without using the bus notifier at all. I can try to code something up but wanted to wait for feedback from Grant first. I've lost track. What feedback are you waiting for from me? I've not dug
Re: [PATCHv3 01/19] [HACK] of: dev_node has struct device pointer
On Thu, 24 Oct 2013 04:36:30 -0500, Kumar Gala ga...@codeaurora.org wrote:
On Oct 24, 2013, at 4:21 AM, Hiroshi Doyu wrote:
Hi Grant,
Grant Likely grant.lik...@linaro.org wrote @ Thu, 24 Oct 2013 10:55:31
+0200:
diff --git a/include/linux/of.h b/include/linux/of.h
index f95aee3..638a88a 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -60,6 +60,7 @@ struct device_node {
struct kref kref;
unsigned long _flags;
void*data;
+ struct device *dev; /* Set only after populated */
Is this being used merely to indicate that a device has been processed
by of_platform_device_create()? Or do you intend to dereference this
pointer? I've avoided putting the struct device in to the device_node
structure up to this point simply becuase there aren't any good clues
for what /kind/ of device it actually points to. I worry that bad
assumptions will get made when other subsystems try to use the
same pointer. ie. if one subsystem creates its own device and sets this
pointer, and then of_platform_device_create() comes along behind, sees
that it is already created, and then returns a platform_device pointer
*for something that isn't a struct platform_device*. This is very bad.
Instead of using a pointer to the struct device, would a flag be
sufficient for your purposes? Would it be fine to return NULL if the
device has already been created?
Yes, a flag would be enough for this purpose.
This patch is a part of HACK to control device instanciation order. We
have an IOMMU device(platform) which needs to be instanciated earlier
than other (platform)devices so that IOMMU driver would configure them
as IOMMU'able device.
Is there any better way to control device instanciation order from DT?
I was also thinking being able to call of_platform_populate multiple times
and have explicit lists to control device init order might be a workable
solution. So might be worth continuing down this path to make device nodes
that have already be created.
Does that actually solve the problem though? If the multiple calls to
of_platform_populate are all before the driver initcall, then the order
of probing is most likely going to be more influenced by kernel link
order.
g.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCHv3 01/19] [HACK] of: dev_node has struct device pointer
On Fri, 25 Oct 2013 12:49:38 +0200, Thierry Reding thierry.red...@gmail.com wrote: On Fri, Oct 25, 2013 at 11:49:05AM +0200, Hiroshi Doyu wrote: Thierry Reding thierry.red...@gmail.com wrote @ Fri, 25 Oct 2013 11:11:05 +0200: This is actually the other problem that I'm aware of that could benefit from [interrupt resolution at probe time]. My idea was that once we had a way within the driver core to resolve interrupt references at probe time it could be used for potentially many other resources as well. Some of the resources like GPIOs and regulators are obviously not something that the core can or should be requesting, but mostly resources that you don't actually need to control after probing (such as interrupts) would be a good fit because otherwise people would write the same boilerplate over and over again. IOMMUs seem to me to be in that same category. As far as I can tell, an IOMMU driver registers the IOMMU for a given bus, upon which every device can simply be used (mostly transparently) with that IOMMU. While I haven't figured out how exactly, I'm pretty sure we can take advantage of the resolution of resources at probe time within the core to both keep drivers from having to do anything in particular and at the same time have code to determine if the IOMMU driver hasn't been probed yet and return -EPROBE_DEFER appropriately. Can you explain the above a bit more? Originally I thought that what Grant suggested would work ok with this patch. I think the objection to these patches is that they special case the instantiation of some devices. It's not a proper solution because it implies various things. For example merely instantiating the IOMMU device earlier than others is only going to work *if* the driver is actually probed before the drivers of other devices. If you want to build the driver as a module for example, probe order becomes entirely non-deterministic. I understand the above limitation. What I thought for the solution is that I can make use of iommu_bus even before IOMMU is instanciated. iommu_bus has its notifier which calls iommu_ops()-add_device(). This could return -EPROBE_DEFER when IOMMU isn't ready. Only the problem is the current bus_notifier doesn't have the ability to return error. I'll see if it can be modified. Even with this, at least IOMMU *driver* needs to be init'ed enough earlier to prevent devices from being registered without IOMMU. The way notifiers work is that they run completely hidden from whatever triggers them. For instance you register the IOMMU bus notifier from the IOMMU driver (by calling bus_set_iommu()). That registers a function to be called when some event happens on that bus. When a device's driver is probed successfully, the driver core will notify the bus, which causes the IOMMU callback to be run. Some of this code runs before the driver has successfully been probed, so I imagine it would be possible to use it to abort probing. But that's not possible at least with the current code. Instead of handling such dependencies implicitly by making sure all resource providers are probed earlier than any of their consumers, the dependencies are handled more explicitly, which turns out to simplify things a lot. There's some additional work required in the core, but if done consistently no driver needs to care about the dependencies and it no longer matters where the resources come from. The problem is reduced to essentially this: while (!resource_available()) load_more_drivers(); So my proposed solution for the IOMMU case is to treat it the same as any other resources. Perhaps resource isn't the right word, but at the core the issue is the same. A device requires the services of an IOMMU so that it can be put into the correct address space. If the IOMMU is not available yet it cannot do that, so we simply return -EPROBE_DEFER and cause the probe to be retried later. This looks somewhat similar to the above iommu_bus notifier. Is there any way to implement the same mechanism rather than using bus? Yes, I think it should be possible to get this to work without using the bus notifier at all. I can try to code something up but wanted to wait for feedback from Grant first. I've lost track. What feedback are you waiting for from me? I've not dug into this entire series so I may not provide clueful feedback. g. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: Reparenting a platform device
On Sat, 7 Apr 2012 13:35:10 +0200, Thierry Reding thierry.red...@avionic-design.de wrote: * Grant Likely wrote: On Thu, 5 Apr 2012 10:42:58 +0200, Thierry Reding thierry.red...@avionic-design.de wrote: Hi, I have a device tree where I have a GART device and a DRM device which uses the GART. The GART is implemented by an IOMMU driver (tegra-gart) and requires the user device to be a child of the GART device (it explicitly checks for this when the user device is attached). I've tried two alternatives to achieve this: create the GART device in the user driver's .probe() function and explicitly set the DRM device's parent to the resulting platform device like so: gart = platform_device_alloc(...); ... pdev-dev.parent = gart-dev; Yeah, don't *ever* try to do this. The device hierarchy is a complex data structure which must never be directly manipulated. The alternative is to use the device tree to look up the GART device node and resolve it to the corresponding struct device: gart_node = of_parse_phandle(drm-dev-of_node, gart-parent, 0); gart = bus_find_device(drm-dev-bus, NULL, gart_node, match_of_node); Where match_of_node matches each struct device's .of_node field to the gart_node. Both of these variants seem to work, and the DRM device can be properly attached to the GART device. However, after the DRM driver's .probe() exits, I get the following error: I don't understand what you're trying to describe here as the 2nd option. Regardless, reparenting should not ben the solution at all. What does the device tree that you envision look like for this? What devices are created, and what drivers bind to them? The reason why I need to reparent at all is because the IOMMU driver requires the user to be a child of the IOMMU device. If you look at the driver in drivers/iommu/tegra-gart.c you'll see that it references dev-parent in several places, most notably in the gart_iommu_attach_dev() function. So there's really only two options that I can see: 1) create a virtual device that is a child of the GART and is in charge of the actual allocations from the GART and have the DRM driver use that interface or 2) change the GART driver's behaviour in a way that the parent/child relationship is no longer a requirement. Either is fine by me. 1) has the advantage of providing a central allocation manager for the GART and will allow to register multiple clients with the GART. 2) does not have that advantage. Another alternative may be to allow only a single device to attach to the GART that doesn't have to be a child of the GART. That way the DRM could take care of GART aperture allocations, which seems to be the most logical place to do that anyway. That also works. As long as nothing messes about with odd reparenting then I'm happy. g. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: Reparenting a platform device
On Thu, 5 Apr 2012 10:42:58 +0200, Thierry Reding thierry.red...@avionic-design.de wrote: Hi, I have a device tree where I have a GART device and a DRM device which uses the GART. The GART is implemented by an IOMMU driver (tegra-gart) and requires the user device to be a child of the GART device (it explicitly checks for this when the user device is attached). I've tried two alternatives to achieve this: create the GART device in the user driver's .probe() function and explicitly set the DRM device's parent to the resulting platform device like so: gart = platform_device_alloc(...); ... pdev-dev.parent = gart-dev; Yeah, don't *ever* try to do this. The device hierarchy is a complex data structure which must never be directly manipulated. The alternative is to use the device tree to look up the GART device node and resolve it to the corresponding struct device: gart_node = of_parse_phandle(drm-dev-of_node, gart-parent, 0); gart = bus_find_device(drm-dev-bus, NULL, gart_node, match_of_node); Where match_of_node matches each struct device's .of_node field to the gart_node. Both of these variants seem to work, and the DRM device can be properly attached to the GART device. However, after the DRM driver's .probe() exits, I get the following error: I don't understand what you're trying to describe here as the 2nd option. Regardless, reparenting should not ben the solution at all. What does the device tree that you envision look like for this? What devices are created, and what drivers bind to them? g. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
