| Message ID | 1409327670-3495-2-git-send-email-will.deacon@arm.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Aug 29, 2014 at 04:54:24PM +0100, Will Deacon 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 > set some per-instance data on their of_node. This data can then be > passed back to a new add_device function (added in a later patch) to > identify the IOMMU instance in question. > > Signed-off-by: Will Deacon <will.deacon@arm.com> > --- > drivers/iommu/of_iommu.c | 14 ++++++++++++++ > include/asm-generic/vmlinux.lds.h | 2 ++ > include/linux/of_iommu.h | 21 +++++++++++++++++++++ > 3 files changed, 37 insertions(+) I don't think this is the right direction. We've been preaching that using initcall ordering is a bad thing and people should be using deferred probe instead. Now we have a bunch of these OF tables that do the exact opposite. Note that these are nothing more than a variant of initcalls that get called at platform-specific points in time. There are also ongoing discussions about how to change the device probe order to use dependencies (such as phandles from device tree) to prevent excessive deferred probing. With that in place this would be solved in a much more generic way. Thierry
On Monday 01 September 2014 09:52:19 Thierry Reding wrote: > I don't think this is the right direction. We've been preaching that > using initcall ordering is a bad thing and people should be using > deferred probe instead. Now we have a bunch of these OF tables that do > the exact opposite. Note that these are nothing more than a variant of > initcalls that get called at platform-specific points in time. > > There are also ongoing discussions about how to change the device probe > order to use dependencies (such as phandles from device tree) to prevent > excessive deferred probing. With that in place this would be solved in a > much more generic way. We are not creating an ABI here, so it can always be changed later. For now, I think iommus are clearly in the same category as irqchips, timers, clock controllers and smp operations: we really want them to be available before we set up any other devices. I don't mind finding a more generic solution in the long run, but for now, this gets the problem out of the way, and I can't think of any realistic corner cases that would prevent it from working. Do you think that an IOMMU driver may have a dependency on another device driver that is probed later? Do you have an example of such hardware? Arnd
On Mon, Sep 01, 2014 at 03:31:28PM +0100, Arnd Bergmann wrote: > On Monday 01 September 2014 09:52:19 Thierry Reding wrote: > > I don't think this is the right direction. We've been preaching that > > using initcall ordering is a bad thing and people should be using > > deferred probe instead. Now we have a bunch of these OF tables that do > > the exact opposite. Note that these are nothing more than a variant of > > initcalls that get called at platform-specific points in time. > > > > There are also ongoing discussions about how to change the device probe > > order to use dependencies (such as phandles from device tree) to prevent > > excessive deferred probing. With that in place this would be solved in a > > much more generic way. > > We are not creating an ABI here, so it can always be changed later. > For now, I think iommus are clearly in the same category as irqchips, > timers, clock controllers and smp operations: we really want them > to be available before we set up any other devices. I'm also trying to move the SMMU driver to the generic bindings before the existing bindings start getting used. The sooner we have parsing of the generic binding in the core code, the sooner that can happen. Will
On Monday 01 September 2014 09:52:19 Thierry Reding wrote: > On Fri, Aug 29, 2014 at 04:54:24PM +0100, Will Deacon 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 > > set some per-instance data on their of_node. This data can then be > > passed back to a new add_device function (added in a later patch) to > > identify the IOMMU instance in question. > > > > Signed-off-by: Will Deacon <will.deacon@arm.com> > > --- > > > > drivers/iommu/of_iommu.c | 14 ++++++++++++++ > > include/asm-generic/vmlinux.lds.h | 2 ++ > > include/linux/of_iommu.h | 21 +++++++++++++++++++++ > > 3 files changed, 37 insertions(+) > > I don't think this is the right direction. We've been preaching that > using initcall ordering is a bad thing and people should be using > deferred probe instead. Now we have a bunch of these OF tables that do > the exact opposite. Note that these are nothing more than a variant of > initcalls that get called at platform-specific points in time. My idea to solve this problem was to defer probing of the bus master device from the add_device IOMMU operation. This obviously won't work with add_device called from the BUS_NOTIFY_ADD_DEVICE notifier, which led me to naively wonder whether we couldn't call the add_device operation from the BUS_NOTIFY_BIND_DRIVER notifier instead. > There are also ongoing discussions about how to change the device probe > order to use dependencies (such as phandles from device tree) to prevent > excessive deferred probing. With that in place this would be solved in a > much more generic way.
Hi Will, > -----Original Message----- > From: Will Deacon [mailto:will.deacon@arm.com] > Sent: Friday, August 29, 2014 9:24 PM > To: linux-arm-kernel@lists.infradead.org; iommu@lists.linux-foundation.org > Cc: arnd@arndb.de; dwmw2@infradead.org; jroedel@suse.de; > hdoyu@nvidia.com; Sethi Varun-B16395; thierry.reding@gmail.com; > laurent.pinchart@ideasonboard.com; Will Deacon > Subject: [RFC PATCH 1/7] iommu: provide early initialisation hook for IOMMU > drivers > > 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 > set some per-instance data on their of_node. This data can then be > passed back to a new add_device function (added in a later patch) to > identify the IOMMU instance in question. > > Signed-off-by: Will Deacon <will.deacon@arm.com> > --- > drivers/iommu/of_iommu.c | 14 ++++++++++++++ > include/asm-generic/vmlinux.lds.h | 2 ++ > include/linux/of_iommu.h | 21 +++++++++++++++++++++ > 3 files changed, 37 insertions(+) > > diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c > index e550ccb7634e..f9209666157c 100644 > --- a/drivers/iommu/of_iommu.c > +++ b/drivers/iommu/of_iommu.c > @@ -89,3 +89,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 int (*init_fn)(struct device_node *) = match->data; Is the init function also responsible for setting iommu_ops (per bus)? We need to take in to consideration that iommu API (iommu.c) initialization happens during "arch_init". When we setup bus iommu ops add_device would be called for devices which have already been probed. Also, as I can see from the code we have of_platform_populate which gets called right after of_iommu_init, which would in turn also lead to add_device invocation (after add_device_master_ids). I believe this would happen before iommu API initialization which would cause issues. -Varun
On Tuesday 02 September 2014 14:47:54 Varun Sethi wrote: > > +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 int (*init_fn)(struct device_node *) = match->data; > > Is the init function also responsible for setting iommu_ops (per bus)? > We need to take in to consideration that iommu API (iommu.c) > initialization happens during "arch_init". I would hope that as part as one of the next steps, we can skip the step of setting the iommu_ops per bus_type. It's really not that meaningful when only some of the devices on one bus are able to use an iommu, and some may need other ops than others. > When we setup bus iommu > ops add_device would be called for devices which have already been > probed. As soon as you move to the generic way of probing the IOMMU for DT, you should not need an add_device callback any more. Each iommu driver should do one one or the other, but not both. > Also, as I can see from the code we have of_platform_populate > which gets called right after of_iommu_init, which would in turn also > lead to add_device invocation (after add_device_master_ids). > I believe this would happen before iommu API initialization which > would cause issues. of_iommu_init should be the point where the iommu API gets initialized. Arnd
diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c index e550ccb7634e..f9209666157c 100644 --- a/drivers/iommu/of_iommu.c +++ b/drivers/iommu/of_iommu.c @@ -89,3 +89,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 int (*init_fn)(struct device_node *) = 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 5ba0360663a7..b75ede8f99ae 100644 --- a/include/asm-generic/vmlinux.lds.h +++ b/include/asm-generic/vmlinux.lds.h @@ -162,6 +162,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) @@ -495,6 +496,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/of_iommu.h b/include/linux/of_iommu.h index 51a560f34bca..29f2f3f88d6a 100644 --- a/include/linux/of_iommu.h +++ b/include/linux/of_iommu.h @@ -1,12 +1,16 @@ #ifndef __OF_IOMMU_H #define __OF_IOMMU_H +#include <linux/of.h> + #ifdef CONFIG_OF_IOMMU extern int of_get_dma_window(struct device_node *dn, const char *prefix, int index, unsigned long *busno, dma_addr_t *addr, size_t *size); +extern void of_iommu_init(void); + #else static inline int of_get_dma_window(struct device_node *dn, const char *prefix, @@ -16,6 +20,23 @@ static inline int of_get_dma_window(struct device_node *dn, const char *prefix, return -EINVAL; } +static inline void of_iommu_init(void) { } + #endif /* CONFIG_OF_IOMMU */ +static inline void of_iommu_set_data(struct device_node *np, void *data) +{ + np->data = data; +} + +static inline void *of_iommu_get_data(struct device_node *np) +{ + return np->data; +} + +extern struct of_device_id __iommu_of_table; + +#define IOMMU_OF_DECLARE(name, compat, fn) \ + OF_DECLARE_1(iommu, name, compat, fn) + #endif /* __OF_IOMMU_H */
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 set some per-instance data on their of_node. This data can then be passed back to a new add_device function (added in a later patch) to identify the IOMMU instance in question. Signed-off-by: Will Deacon <will.deacon@arm.com> --- drivers/iommu/of_iommu.c | 14 ++++++++++++++ include/asm-generic/vmlinux.lds.h | 2 ++ include/linux/of_iommu.h | 21 +++++++++++++++++++++ 3 files changed, 37 insertions(+)