| Message ID | 1416395748-10731-19-git-send-email-m.szyprowski@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Marek, Thank you for the patch. On Wednesday 19 November 2014 12:15:47 Marek Szyprowski wrote: > This patch introduces IOMMU_OF_DECLARE-based initialization to the > driver, which replaces subsys_initcall-based procedure. > exynos_iommu_of_setup ensures that each sysmmu controller is probed > before its master device. > > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > --- > drivers/iommu/exynos-iommu.c | 28 +++++++++++++++++++++++++++- > 1 file changed, 27 insertions(+), 1 deletion(-) > > diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c > index cd28dc09db39..88f9afe641a0 100644 > --- a/drivers/iommu/exynos-iommu.c > +++ b/drivers/iommu/exynos-iommu.c [snip] > @@ -1125,4 +1134,21 @@ err_reg_driver: > kmem_cache_destroy(lv2table_kmem_cache); > return ret; > } > -subsys_initcall(exynos_iommu_init); > + > +static int __init exynos_iommu_of_setup(struct device_node *np) > +{ > + struct platform_device *pdev; > + > + if (!init_done) > + exynos_iommu_init(); > + > + pdev = of_platform_device_create(np, NULL, platform_bus_type.dev_root); > + if (IS_ERR(pdev)) > + return PTR_ERR(pdev); If we end up having to create the IOMMU platform devices from within the drivers, the introduction of IOMMU_OF_DECLARE starts to feel like a workaround to me. I wonder whether it wouldn't then be better to let the driver core instantiate the IOMMU platform device from DT as for all other devices, and use device notifiers to defer probe of the bus masters until the required IOMMU(s) are registered. Will, what's your opinion on that ? > + > + of_iommu_set_ops(np, &exynos_iommu_ops); > + return 0; > +} > + > +IOMMU_OF_DECLARE(exynos_iommu_of, "samsung,exynos-sysmmu", > + exynos_iommu_of_setup);
On Sun, Dec 14, 2014 at 02:45:36PM +0200, Laurent Pinchart wrote: > Hi Marek, > > Thank you for the patch. > > On Wednesday 19 November 2014 12:15:47 Marek Szyprowski wrote: > > This patch introduces IOMMU_OF_DECLARE-based initialization to the > > driver, which replaces subsys_initcall-based procedure. > > exynos_iommu_of_setup ensures that each sysmmu controller is probed > > before its master device. > > > > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > > --- > > drivers/iommu/exynos-iommu.c | 28 +++++++++++++++++++++++++++- > > 1 file changed, 27 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c > > index cd28dc09db39..88f9afe641a0 100644 > > --- a/drivers/iommu/exynos-iommu.c > > +++ b/drivers/iommu/exynos-iommu.c > > [snip] > > > @@ -1125,4 +1134,21 @@ err_reg_driver: > > kmem_cache_destroy(lv2table_kmem_cache); > > return ret; > > } > > -subsys_initcall(exynos_iommu_init); > > + > > +static int __init exynos_iommu_of_setup(struct device_node *np) > > +{ > > + struct platform_device *pdev; > > + > > + if (!init_done) > > + exynos_iommu_init(); > > + > > + pdev = of_platform_device_create(np, NULL, platform_bus_type.dev_root); > > + if (IS_ERR(pdev)) > > + return PTR_ERR(pdev); > > If we end up having to create the IOMMU platform devices from within the > drivers, the introduction of IOMMU_OF_DECLARE starts to feel like a workaround > to me. I wonder whether it wouldn't then be better to let the driver core > instantiate the IOMMU platform device from DT as for all other devices, and > use device notifiers to defer probe of the bus masters until the required > IOMMU(s) are registered. Notifiers don't work very well for this. Notifier blocks are supposed to return a very limited number of values, so sneaking in a -EPROBE_DEFER isn't likely to work out very well. This was in fact one of Hiroshi's proposals over a year ago and got refused because of those reasons. The next solution was to introduce a function, not very much unlike the of_iommu_configure() that would be called in the core prior to calling into the driver's ->probe() callback so that it could handle this at probe time (as opposed to device creation time). That way the core can easily defer probe if the IOMMU is not there yet. At the same time it can simply use the driver model without requiring per-architecture hacks or workarounds. Note that there is really no need for any of this configuration or initialization to happen at device creation time. Drivers won't be able to use the IOMMU or DMA APIs until their .probe(), so handling this any earlier is completely unnecessary. Thierry
On Sun, Dec 14, 2014 at 12:45:36PM +0000, Laurent Pinchart wrote: > On Wednesday 19 November 2014 12:15:47 Marek Szyprowski wrote: > > This patch introduces IOMMU_OF_DECLARE-based initialization to the > > driver, which replaces subsys_initcall-based procedure. > > exynos_iommu_of_setup ensures that each sysmmu controller is probed > > before its master device. > > > > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > > --- > > drivers/iommu/exynos-iommu.c | 28 +++++++++++++++++++++++++++- > > 1 file changed, 27 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c > > index cd28dc09db39..88f9afe641a0 100644 > > --- a/drivers/iommu/exynos-iommu.c > > +++ b/drivers/iommu/exynos-iommu.c > > [snip] > > > @@ -1125,4 +1134,21 @@ err_reg_driver: > > kmem_cache_destroy(lv2table_kmem_cache); > > return ret; > > } > > -subsys_initcall(exynos_iommu_init); > > + > > +static int __init exynos_iommu_of_setup(struct device_node *np) > > +{ > > + struct platform_device *pdev; > > + > > + if (!init_done) > > + exynos_iommu_init(); > > + > > + pdev = of_platform_device_create(np, NULL, platform_bus_type.dev_root); > > + if (IS_ERR(pdev)) > > + return PTR_ERR(pdev); > > If we end up having to create the IOMMU platform devices from within the > drivers, the introduction of IOMMU_OF_DECLARE starts to feel like a workaround > to me. I wonder whether it wouldn't then be better to let the driver core > instantiate the IOMMU platform device from DT as for all other devices, and > use device notifiers to defer probe of the bus masters until the required > IOMMU(s) are registered. > > Will, what's your opinion on that ? Creating the platform device manually for the IOMMU is indeed grotty, but I don't really understand why it's needed. Interrupt controllers, for example, seem to get by without one. Will
Hi Will, On Monday 15 December 2014 17:17:00 Will Deacon wrote: > On Sun, Dec 14, 2014 at 12:45:36PM +0000, Laurent Pinchart wrote: > > On Wednesday 19 November 2014 12:15:47 Marek Szyprowski wrote: > > > This patch introduces IOMMU_OF_DECLARE-based initialization to the > > > driver, which replaces subsys_initcall-based procedure. > > > exynos_iommu_of_setup ensures that each sysmmu controller is probed > > > before its master device. > > > > > > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > > > --- > > > > > > drivers/iommu/exynos-iommu.c | 28 +++++++++++++++++++++++++++- > > > 1 file changed, 27 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c > > > index cd28dc09db39..88f9afe641a0 100644 > > > --- a/drivers/iommu/exynos-iommu.c > > > +++ b/drivers/iommu/exynos-iommu.c > > > > [snip] > > > > > @@ -1125,4 +1134,21 @@ err_reg_driver: > > > kmem_cache_destroy(lv2table_kmem_cache); > > > return ret; > > > > > > } > > > > > > -subsys_initcall(exynos_iommu_init); > > > + > > > +static int __init exynos_iommu_of_setup(struct device_node *np) > > > +{ > > > + struct platform_device *pdev; > > > + > > > + if (!init_done) > > > + exynos_iommu_init(); > > > + > > > + pdev = of_platform_device_create(np, NULL, > > > platform_bus_type.dev_root); > > > + if (IS_ERR(pdev)) > > > + return PTR_ERR(pdev); > > > > If we end up having to create the IOMMU platform devices from within the > > drivers, the introduction of IOMMU_OF_DECLARE starts to feel like a > > workaround to me. I wonder whether it wouldn't then be better to let the > > driver core instantiate the IOMMU platform device from DT as for all > > other devices, and use device notifiers to defer probe of the bus masters > > until the required IOMMU(s) are registered. > > > > Will, what's your opinion on that ? > > Creating the platform device manually for the IOMMU is indeed grotty, but I > don't really understand why it's needed. Interrupt controllers, for example, > seem to get by without one. There's several reasons, one of the most compelling ones I can think of at the moment is runtime PM. IRQ controllers close to the CPU use CPU PM notifiers instead. Note that IRQ controllers that are further away from the CPU (such as GPIO-based IRQ controllers) are real platform devices and use runtime PM. IOMMUs are not as low-level as system interrupt controllers or system clocks. I'm beginning to agree with Thierry that they should be treated as normal platform devices as they're not required earlier than probe time of their bus master devices.
On Mon, Dec 15, 2014 at 05:27:30PM +0000, Laurent Pinchart wrote: > On Monday 15 December 2014 17:17:00 Will Deacon wrote: > > On Sun, Dec 14, 2014 at 12:45:36PM +0000, Laurent Pinchart wrote: > > > On Wednesday 19 November 2014 12:15:47 Marek Szyprowski wrote: > > > > +static int __init exynos_iommu_of_setup(struct device_node *np) > > > > +{ > > > > + struct platform_device *pdev; > > > > + > > > > + if (!init_done) > > > > + exynos_iommu_init(); > > > > + > > > > + pdev = of_platform_device_create(np, NULL, > > > > platform_bus_type.dev_root); > > > > + if (IS_ERR(pdev)) > > > > + return PTR_ERR(pdev); > > > > > > If we end up having to create the IOMMU platform devices from within the > > > drivers, the introduction of IOMMU_OF_DECLARE starts to feel like a > > > workaround to me. I wonder whether it wouldn't then be better to let the > > > driver core instantiate the IOMMU platform device from DT as for all > > > other devices, and use device notifiers to defer probe of the bus masters > > > until the required IOMMU(s) are registered. > > > > > > Will, what's your opinion on that ? > > > > Creating the platform device manually for the IOMMU is indeed grotty, but I > > don't really understand why it's needed. Interrupt controllers, for example, > > seem to get by without one. > > There's several reasons, one of the most compelling ones I can think of at the > moment is runtime PM. IRQ controllers close to the CPU use CPU PM notifiers > instead. Note that IRQ controllers that are further away from the CPU (such as > GPIO-based IRQ controllers) are real platform devices and use runtime PM. Ok, that's a good point, but the IOMMU will still probe later anyway, right? > IOMMUs are not as low-level as system interrupt controllers or system clocks. > I'm beginning to agree with Thierry that they should be treated as normal > platform devices as they're not required earlier than probe time of their bus > master devices. Well, I think you'd have to propose patches for discussion since I'm certainly not wed to the current approach; I just want something that allows of_{dma,iommu}_configure to run with the information it needs. The usual answer is `we should model buses properly', but that's really not practical afaict. Will
Hi Will, On Monday 15 December 2014 17:43:02 Will Deacon wrote: > On Mon, Dec 15, 2014 at 05:27:30PM +0000, Laurent Pinchart wrote: > > On Monday 15 December 2014 17:17:00 Will Deacon wrote: > > > On Sun, Dec 14, 2014 at 12:45:36PM +0000, Laurent Pinchart wrote: > > > > On Wednesday 19 November 2014 12:15:47 Marek Szyprowski wrote: > > > > > +static int __init exynos_iommu_of_setup(struct device_node *np) > > > > > +{ > > > > > + struct platform_device *pdev; > > > > > + > > > > > + if (!init_done) > > > > > + exynos_iommu_init(); > > > > > + > > > > > + pdev = of_platform_device_create(np, NULL, > > > > > platform_bus_type.dev_root); > > > > > + if (IS_ERR(pdev)) > > > > > + return PTR_ERR(pdev); > > > > > > > > If we end up having to create the IOMMU platform devices from within > > > > the drivers, the introduction of IOMMU_OF_DECLARE starts to feel like > > > > a workaround to me. I wonder whether it wouldn't then be better to let > > > > the driver core instantiate the IOMMU platform device from DT as for > > > > all other devices, and use device notifiers to defer probe of the bus > > > > masters until the required IOMMU(s) are registered. > > > > > > > > Will, what's your opinion on that ? > > > > > > Creating the platform device manually for the IOMMU is indeed grotty, > > > but I don't really understand why it's needed. Interrupt controllers, > > > for example, seem to get by without one. > > > > There's several reasons, one of the most compelling ones I can think of at > > the moment is runtime PM. IRQ controllers close to the CPU use CPU PM > > notifiers instead. Note that IRQ controllers that are further away from > > the CPU (such as GPIO-based IRQ controllers) are real platform devices > > and use runtime PM. > > Ok, that's a good point, but the IOMMU will still probe later anyway, right? That depends on the driver implementation, using OF node match an IOMMU driver doesn't have to register a struct driver. Assuming we require IOMMU drivers to register a struct driver, a platform device should then be probed at a later time. However, if we wait until the IOMMU gets probed to initialize runtime PM and make it functional, we'll be back in square one if the IOMMU gets probed after the bus master, as the bus master could start issuing bus requests at probe time with the IOMMU not powered yet. > > IOMMUs are not as low-level as system interrupt controllers or system > > clocks. I'm beginning to agree with Thierry that they should be treated > > as normal platform devices as they're not required earlier than probe > > time of their bus master devices. > > Well, I think you'd have to propose patches for discussion since I'm > certainly not wed to the current approach; I just want something that > allows of_{dma,iommu}_configure to run with the information it needs. Do we need of_dma_configure() to run when the device is created, or could we postpone it to just before probe time ? > The usual answer is `we should model buses properly', but that's really > not practical afaict.
On Mon, Dec 15, 2014 at 05:53:48PM +0000, Laurent Pinchart wrote: > Hi Will, Hello again :) > On Monday 15 December 2014 17:43:02 Will Deacon wrote: > > On Mon, Dec 15, 2014 at 05:27:30PM +0000, Laurent Pinchart wrote: > > > On Monday 15 December 2014 17:17:00 Will Deacon wrote: > > > > Creating the platform device manually for the IOMMU is indeed grotty, > > > > but I don't really understand why it's needed. Interrupt controllers, > > > > for example, seem to get by without one. > > > > > > There's several reasons, one of the most compelling ones I can think of at > > > the moment is runtime PM. IRQ controllers close to the CPU use CPU PM > > > notifiers instead. Note that IRQ controllers that are further away from > > > the CPU (such as GPIO-based IRQ controllers) are real platform devices > > > and use runtime PM. > > > > Ok, that's a good point, but the IOMMU will still probe later anyway, right? > > That depends on the driver implementation, using OF node match an IOMMU driver > doesn't have to register a struct driver. Assuming we require IOMMU drivers to > register a struct driver, a platform device should then be probed at a later > time. > > However, if we wait until the IOMMU gets probed to initialize runtime PM and > make it functional, we'll be back in square one if the IOMMU gets probed after > the bus master, as the bus master could start issuing bus requests at probe > time with the IOMMU not powered yet. True, but couldn't the early init code do enough to get the thing functional? That said, I'm showing my ignorance here as I'm not familiar with the PM code (and the software models I have for the SMMU clearly don't implement anything in this regard). > > > IOMMUs are not as low-level as system interrupt controllers or system > > > clocks. I'm beginning to agree with Thierry that they should be treated > > > as normal platform devices as they're not required earlier than probe > > > time of their bus master devices. > > > > Well, I think you'd have to propose patches for discussion since I'm > > certainly not wed to the current approach; I just want something that > > allows of_{dma,iommu}_configure to run with the information it needs. > > Do we need of_dma_configure() to run when the device is created, or could we > postpone it to just before probe time ? I'm not sure I can answer that one... Arnd? Will
On Monday 15 December 2014 18:13:23 Will Deacon wrote: > On Mon, Dec 15, 2014 at 05:53:48PM +0000, Laurent Pinchart wrote: > > Hi Will, > > Hello again :) > > > On Monday 15 December 2014 17:43:02 Will Deacon wrote: > > > On Mon, Dec 15, 2014 at 05:27:30PM +0000, Laurent Pinchart wrote: > > > > On Monday 15 December 2014 17:17:00 Will Deacon wrote: > > > > > Creating the platform device manually for the IOMMU is indeed > > > > > grotty, but I don't really understand why it's needed. Interrupt > > > > > controllers, for example, seem to get by without one. > > > > > > > > There's several reasons, one of the most compelling ones I can think > > > > of at the moment is runtime PM. IRQ controllers close to the CPU use > > > > CPU PM notifiers instead. Note that IRQ controllers that are further > > > > away from the CPU (such as GPIO-based IRQ controllers) are real > > > > platform devices and use runtime PM. > > > > > > Ok, that's a good point, but the IOMMU will still probe later anyway, > > > right? > > > > That depends on the driver implementation, using OF node match an IOMMU > > driver doesn't have to register a struct driver. Assuming we require > > IOMMU drivers to register a struct driver, a platform device should then > > be probed at a later time. > > > > However, if we wait until the IOMMU gets probed to initialize runtime PM > > and make it functional, we'll be back in square one if the IOMMU gets > > probed after the bus master, as the bus master could start issuing bus > > requests at probe time with the IOMMU not powered yet. > > True, but couldn't the early init code do enough to get the thing > functional? That said, I'm showing my ignorance here as I'm not familiar > with the PM code (and the software models I have for the SMMU clearly don't > implement anything in this regard). We're reaching the limits of my knowledge as well. If the IOMMU is in a power domain different than the bus masters the driver would at least need to ensure that the power domain is turned on, which might be a bit hackish without runtime PM. > > > > IOMMUs are not as low-level as system interrupt controllers or system > > > > clocks. I'm beginning to agree with Thierry that they should be > > > > treated as normal platform devices as they're not required earlier > > > > than probe time of their bus master devices. > > > > > > Well, I think you'd have to propose patches for discussion since I'm > > > certainly not wed to the current approach; I just want something that > > > allows of_{dma,iommu}_configure to run with the information it needs. > > > > Do we need of_dma_configure() to run when the device is created, or could > > we postpone it to just before probe time ? > > I'm not sure I can answer that one... Arnd?
Hello, On 2014-12-15 19:19, Laurent Pinchart wrote: > On Monday 15 December 2014 18:13:23 Will Deacon wrote: >> On Mon, Dec 15, 2014 at 05:53:48PM +0000, Laurent Pinchart wrote: >>> On Monday 15 December 2014 17:43:02 Will Deacon wrote: >>>> On Mon, Dec 15, 2014 at 05:27:30PM +0000, Laurent Pinchart wrote: >>>>> On Monday 15 December 2014 17:17:00 Will Deacon wrote: >>>>>> Creating the platform device manually for the IOMMU is indeed >>>>>> grotty, but I don't really understand why it's needed. Interrupt >>>>>> controllers, for example, seem to get by without one. >>>>> There's several reasons, one of the most compelling ones I can think >>>>> of at the moment is runtime PM. IRQ controllers close to the CPU use >>>>> CPU PM notifiers instead. Note that IRQ controllers that are further >>>>> away from the CPU (such as GPIO-based IRQ controllers) are real >>>>> platform devices and use runtime PM. >>>> Ok, that's a good point, but the IOMMU will still probe later anyway, >>>> right? >>> That depends on the driver implementation, using OF node match an IOMMU >>> driver doesn't have to register a struct driver. Assuming we require >>> IOMMU drivers to register a struct driver, a platform device should then >>> be probed at a later time. >>> >>> However, if we wait until the IOMMU gets probed to initialize runtime PM >>> and make it functional, we'll be back in square one if the IOMMU gets >>> probed after the bus master, as the bus master could start issuing bus >>> requests at probe time with the IOMMU not powered yet. >> True, but couldn't the early init code do enough to get the thing >> functional? That said, I'm showing my ignorance here as I'm not familiar >> with the PM code (and the software models I have for the SMMU clearly don't >> implement anything in this regard). > We're reaching the limits of my knowledge as well. If the IOMMU is in a power > domain different than the bus masters the driver would at least need to ensure > that the power domain is turned on, which might be a bit hackish without > runtime PM. In case of Exynos SoC both IOMMU and master device are in the same power domain. During iommu_attach() call driver needs to have power domain enabled, but power domain driver is probed from arch_initcall(). I wanted to move power domain initialization earlier to ensure that it will happen before IOMMU driver probe, that not really a problem. Right now it usually works, because power domains are enabled by bootloader. However please not that I would really like to have something merged. The discussion about iommu controllers lasts for about 2 years and we still don't have ANYTHING working in mainline, so lets merge the of_iommu glue and then check how the remaining issues can be solved. Best regards
On Monday 15 December 2014 18:13:23 Will Deacon wrote: > > > > IOMMUs are not as low-level as system interrupt controllers or system > > > > clocks. I'm beginning to agree with Thierry that they should be treated > > > > as normal platform devices as they're not required earlier than probe > > > > time of their bus master devices. > > > > > > Well, I think you'd have to propose patches for discussion since I'm > > > certainly not wed to the current approach; I just want something that > > > allows of_{dma,iommu}_configure to run with the information it needs. > > > > Do we need of_dma_configure() to run when the device is created, or could we > > postpone it to just before probe time ? > > I'm not sure I can answer that one... Arnd? I believe we could postpone it to probe time, but I'd rather not. The way I see the arguments in favor, we have mainly cosmetic arguments: - Doing it at probe time would eliminate the ugly section magic hack - iommu drivers could be implemented as loadable modules with platform drivers, for consistency with most other drivers On the other hand, I see: - DMA configuration is traditionally done at device creation time, and changing that is more likely to introduce bugs than leaving it where it is. - On a lot of machines, the IOMMU is optional, and the probe function cannot know the difference between an IOMMU driver that is left out of the kernel and one that will be initialized later, using a fixed entry point for initializing the IOMMU makes the behavior consistent There is a third option in theory, which is to only enable the IOMMU as part of dma_set_mask(). I've done that in the past on powerpc, but the new approach seems cleaner. Arnd
Hi Arnd, On Tuesday 16 December 2014 12:40:28 Arnd Bergmann wrote: > On Monday 15 December 2014 18:13:23 Will Deacon wrote: > >>>> IOMMUs are not as low-level as system interrupt controllers or > >>>> system clocks. I'm beginning to agree with Thierry that they should > >>>> be treated as normal platform devices as they're not required > >>>> earlier than probe time of their bus master devices. > >>> > >>> Well, I think you'd have to propose patches for discussion since I'm > >>> certainly not wed to the current approach; I just want something that > >>> allows of_{dma,iommu}_configure to run with the information it needs. > >> > >> Do we need of_dma_configure() to run when the device is created, or > >> could we postpone it to just before probe time ? > > > > I'm not sure I can answer that one... Arnd? > > I believe we could postpone it to probe time, but I'd rather not. > The way I see the arguments in favor, we have mainly cosmetic arguments: > > - Doing it at probe time would eliminate the ugly section magic hack > - iommu drivers could be implemented as loadable modules with platform > drivers, for consistency with most other drivers The main argument, from my point of view, is that handling IOMMUs are normal platform devices allow using all the kernel infrastructure that depends on a struct device. The dev_* print helpers are nice to have, but what would make a big difference is the ability to use runtime PM. > On the other hand, I see: > > - DMA configuration is traditionally done at device creation time, and > changing that is more likely to introduce bugs than leaving it > where it is. > - On a lot of machines, the IOMMU is optional, and the probe function > cannot know the difference between an IOMMU driver that is left out > of the kernel and one that will be initialized later, using a fixed > entry point for initializing the IOMMU makes the behavior consistent I'm not advocating for IOMMU support being built as a loadable module. It might be nice from a development point of view, but that's about it. > There is a third option in theory, which is to only enable the IOMMU > as part of dma_set_mask(). I've done that in the past on powerpc, but > the new approach seems cleaner.
On Tuesday 16 December 2014 14:07:11 Laurent Pinchart wrote: > Hi Arnd, > > On Tuesday 16 December 2014 12:40:28 Arnd Bergmann wrote: > > On Monday 15 December 2014 18:13:23 Will Deacon wrote: > > >>>> IOMMUs are not as low-level as system interrupt controllers or > > >>>> system clocks. I'm beginning to agree with Thierry that they should > > >>>> be treated as normal platform devices as they're not required > > >>>> earlier than probe time of their bus master devices. > > >>> > > >>> Well, I think you'd have to propose patches for discussion since I'm > > >>> certainly not wed to the current approach; I just want something that > > >>> allows of_{dma,iommu}_configure to run with the information it needs. > > >> > > >> Do we need of_dma_configure() to run when the device is created, or > > >> could we postpone it to just before probe time ? > > > > > > I'm not sure I can answer that one... Arnd? > > > > I believe we could postpone it to probe time, but I'd rather not. > > The way I see the arguments in favor, we have mainly cosmetic arguments: > > > > - Doing it at probe time would eliminate the ugly section magic hack > > - iommu drivers could be implemented as loadable modules with platform > > drivers, for consistency with most other drivers > > The main argument, from my point of view, is that handling IOMMUs are normal > platform devices allow using all the kernel infrastructure that depends on a > struct device. The dev_* print helpers are nice to have, but what would make a > big difference is the ability to use runtime PM. Right, I agree that would be nice. > > On the other hand, I see: > > > > - DMA configuration is traditionally done at device creation time, and > > changing that is more likely to introduce bugs than leaving it > > where it is. > > - On a lot of machines, the IOMMU is optional, and the probe function > > cannot know the difference between an IOMMU driver that is left out > > of the kernel and one that will be initialized later, using a fixed > > entry point for initializing the IOMMU makes the behavior consistent > > I'm not advocating for IOMMU support being built as a loadable module. It > might be nice from a development point of view, but that's about it. We'd still have to deal with deferred probing, or with ordering the iommu initcalls before the dma master initcalls in some other way, so the problem is not that different from loadable modules. Do you have an idea for how to solve that? Arnd
Hi Arnd, On Tuesday 16 December 2014 13:10:59 Arnd Bergmann wrote: > On Tuesday 16 December 2014 14:07:11 Laurent Pinchart wrote: > > On Tuesday 16 December 2014 12:40:28 Arnd Bergmann wrote: > > > On Monday 15 December 2014 18:13:23 Will Deacon wrote: > > > >>>> IOMMUs are not as low-level as system interrupt controllers or > > > >>>> system clocks. I'm beginning to agree with Thierry that they should > > > >>>> be treated as normal platform devices as they're not required > > > >>>> earlier than probe time of their bus master devices. > > > >>> > > > >>> Well, I think you'd have to propose patches for discussion since I'm > > > >>> certainly not wed to the current approach; I just want something > > > >>> that allows of_{dma,iommu}_configure to run with the information it > > > >>> needs. > > > >> > > > >> Do we need of_dma_configure() to run when the device is created, or > > > >> could we postpone it to just before probe time ? > > > > > > > > I'm not sure I can answer that one... Arnd? > > > > > > I believe we could postpone it to probe time, but I'd rather not. > > > The way I see the arguments in favor, we have mainly cosmetic arguments: > > > > > > - Doing it at probe time would eliminate the ugly section magic hack > > > - iommu drivers could be implemented as loadable modules with platform > > > drivers, for consistency with most other drivers > > > > The main argument, from my point of view, is that handling IOMMUs are > > normal platform devices allow using all the kernel infrastructure that > > depends on a struct device. The dev_* print helpers are nice to have, but > > what would make a big difference is the ability to use runtime PM. > > Right, I agree that would be nice. > > > > On the other hand, I see: > > > > > > - DMA configuration is traditionally done at device creation time, and > > > changing that is more likely to introduce bugs than leaving it > > > where it is. > > > > > > - On a lot of machines, the IOMMU is optional, and the probe function > > > cannot know the difference between an IOMMU driver that is left out > > > of the kernel and one that will be initialized later, using a fixed > > > entry point for initializing the IOMMU makes the behavior consistent > > > > I'm not advocating for IOMMU support being built as a loadable module. It > > might be nice from a development point of view, but that's about it. > > We'd still have to deal with deferred probing, or with ordering the iommu > initcalls before the dma master initcalls in some other way, so the > problem is not that different from loadable modules. Do you have an idea > for how to solve that? If we forbid the IOMMU driver from being compiled as a module can't we just rely on deferred probing ? The bus master driver will just be reprobed after the IOMMU gets probed, like for other devices. This could fail in case the IOMMU device permanently fails to probe. There would be something very wrong in the system in that case, Enabling the bus masters totally transparently without IOMMU support could not be such a good idea.
On Wednesday 17 December 2014 01:24:42 Laurent Pinchart wrote: > > If we forbid the IOMMU driver from being compiled as a module can't we just > rely on deferred probing ? The bus master driver will just be reprobed after > the IOMMU gets probed, like for other devices. > > This could fail in case the IOMMU device permanently fails to probe. There > would be something very wrong in the system in that case, Enabling the bus > masters totally transparently without IOMMU support could not be such a good > idea. I believe in the majority of cases today, the IOMMU is entirely optional. There are valid reasons for not including the IOMMU driver in the kernel, e.g. when you know that all the machines with that driver can DMA to all of their RAM and you want to avoid the overhead of IOTLB misses. Arnd
Hi Arnd, On Wednesday 17 December 2014 15:27:36 Arnd Bergmann wrote: > On Wednesday 17 December 2014 01:24:42 Laurent Pinchart wrote: > > If we forbid the IOMMU driver from being compiled as a module can't we > > just rely on deferred probing ? The bus master driver will just be > > reprobed after the IOMMU gets probed, like for other devices. > > > > This could fail in case the IOMMU device permanently fails to probe. There > > would be something very wrong in the system in that case, Enabling the bus > > masters totally transparently without IOMMU support could not be such a > > good idea. > > I believe in the majority of cases today, the IOMMU is entirely optional. > There are valid reasons for not including the IOMMU driver in the kernel, > e.g. when you know that all the machines with that driver can DMA to > all of their RAM and you want to avoid the overhead of IOTLB misses. Should that really be controlled by compiling the IOMMU driver out, wouldn't it be better to disable the IOMMU devices in DT in that case ?
Am Mittwoch, den 17.12.2014, 15:27 +0100 schrieb Arnd Bergmann: > On Wednesday 17 December 2014 01:24:42 Laurent Pinchart wrote: > > > > If we forbid the IOMMU driver from being compiled as a module can't we just > > rely on deferred probing ? The bus master driver will just be reprobed after > > the IOMMU gets probed, like for other devices. > > > > This could fail in case the IOMMU device permanently fails to probe. There > > would be something very wrong in the system in that case, Enabling the bus > > masters totally transparently without IOMMU support could not be such a good > > idea. > > I believe in the majority of cases today, the IOMMU is entirely optional. > There are valid reasons for not including the IOMMU driver in the kernel, > e.g. when you know that all the machines with that driver can DMA to > all of their RAM and you want to avoid the overhead of IOTLB misses. > This is similar to the problems we discussed with the componentized device framework and in the end everybody agreed on a simple rule: if the device node is enabled in the DT there must be a driver bound to it before other devices depending on this node can be probed. If we apply the same logic to the IOMMU situation we get two possibilities to allow bypassing the IOMMU: 1. completely disable the IOMMU node in the DT 2. leave node enabled in DT, but add a bypass property Obviously the second option still requires to have the IOMMU driver available, but is more flexible. Right now everything depends on the IOMMU being in passthrough mode at reset with no driver bound. If a device comes around where this isn't the case thing will break with the current assumptions or the first option. Having the IOMMU node enabled in DT with no driver available to the kernel seems like an invalid configuration which should be expected to break. Exactly the same thing as with componentized devices... Regards, Lucas
On Wednesday 17 December 2014 16:39:02 Laurent Pinchart wrote: > > On Wednesday 17 December 2014 15:27:36 Arnd Bergmann wrote: > > On Wednesday 17 December 2014 01:24:42 Laurent Pinchart wrote: > > > If we forbid the IOMMU driver from being compiled as a module can't we > > > just rely on deferred probing ? The bus master driver will just be > > > reprobed after the IOMMU gets probed, like for other devices. > > > > > > This could fail in case the IOMMU device permanently fails to probe. There > > > would be something very wrong in the system in that case, Enabling the bus > > > masters totally transparently without IOMMU support could not be such a > > > good idea. > > > > I believe in the majority of cases today, the IOMMU is entirely optional. > > There are valid reasons for not including the IOMMU driver in the kernel, > > e.g. when you know that all the machines with that driver can DMA to > > all of their RAM and you want to avoid the overhead of IOTLB misses. > > Should that really be controlled by compiling the IOMMU driver out, wouldn't > it be better to disable the IOMMU devices in DT in that case ? It's a policy decision that should only depend on the user. Modifying the DT is wrong here IMHO because the device is still connected to the IOMMU in hardware and we should correctly represent that. You can normally set 'iommu=off' or 'iommu=force' on the command line to force it on or off rather than letting the IOMMU driver decide whether the device turns it on. Not enabling the IOMMU at all should really just behave the same way as 'iommu=off', anything else would be very confusing. Arnd
On Wednesday 17 December 2014 15:53:25 Lucas Stach wrote: > Am Mittwoch, den 17.12.2014, 15:27 +0100 schrieb Arnd Bergmann: > > On Wednesday 17 December 2014 01:24:42 Laurent Pinchart wrote: > > > > > > If we forbid the IOMMU driver from being compiled as a module can't we just > > > rely on deferred probing ? The bus master driver will just be reprobed after > > > the IOMMU gets probed, like for other devices. > > > > > > This could fail in case the IOMMU device permanently fails to probe. There > > > would be something very wrong in the system in that case, Enabling the bus > > > masters totally transparently without IOMMU support could not be such a good > > > idea. > > > > I believe in the majority of cases today, the IOMMU is entirely optional. > > There are valid reasons for not including the IOMMU driver in the kernel, > > e.g. when you know that all the machines with that driver can DMA to > > all of their RAM and you want to avoid the overhead of IOTLB misses. > > > This is similar to the problems we discussed with the componentized > device framework and in the end everybody agreed on a simple rule: > if the device node is enabled in the DT there must be a driver bound to > it before other devices depending on this node can be probed. > > If we apply the same logic to the IOMMU situation we get two > possibilities to allow bypassing the IOMMU: > 1. completely disable the IOMMU node in the DT > 2. leave node enabled in DT, but add a bypass property > > Obviously the second option still requires to have the IOMMU driver > available, but is more flexible. Right now everything depends on the > IOMMU being in passthrough mode at reset with no driver bound. If a > device comes around where this isn't the case thing will break with the > current assumptions or the first option. > > Having the IOMMU node enabled in DT with no driver available to the > kernel seems like an invalid configuration which should be expected to > break. Exactly the same thing as with componentized devices... One differences here is that for the IOMMU, we should generally be able to fall back to swiotlb (currently only on ARM64, not ARM32, until someone adds support). I can see your point regarding machines that have a mandatory IOMMU with no fallback when there is no driver, but we can support them by making the iommu driver selected through Kconfig for that platform, while still allowing other platforms to work with drivers left out of the kernel. Arnd
Hi Arnd, On Wednesday 17 December 2014 16:41:33 Arnd Bergmann wrote: > On Wednesday 17 December 2014 16:39:02 Laurent Pinchart wrote: > > On Wednesday 17 December 2014 15:27:36 Arnd Bergmann wrote: > > > On Wednesday 17 December 2014 01:24:42 Laurent Pinchart wrote: > > > > If we forbid the IOMMU driver from being compiled as a module can't we > > > > just rely on deferred probing ? The bus master driver will just be > > > > reprobed after the IOMMU gets probed, like for other devices. > > > > > > > > This could fail in case the IOMMU device permanently fails to probe. > > > > There > > > > would be something very wrong in the system in that case, Enabling the > > > > bus > > > > masters totally transparently without IOMMU support could not be such > > > > a > > > > good idea. > > > > > > I believe in the majority of cases today, the IOMMU is entirely > > > optional. > > > There are valid reasons for not including the IOMMU driver in the > > > kernel, > > > e.g. when you know that all the machines with that driver can DMA to > > > all of their RAM and you want to avoid the overhead of IOTLB misses. > > > > Should that really be controlled by compiling the IOMMU driver out, > > wouldn't it be better to disable the IOMMU devices in DT in that case ? > > It's a policy decision that should only depend on the user. Modifying the > DT is wrong here IMHO because the device is still connected to the IOMMU > in hardware and we should correctly represent that. I was thinking about setting status = "disabled" on the IOMMU nodes, not removing the IOMMU references in the bus master nodes. > You can normally set 'iommu=off' or 'iommu=force' on the command line > to force it on or off rather than letting the IOMMU driver decide whether > the device turns it on. Not enabling the IOMMU at all should really just > behave the same way as 'iommu=off', anything else would be very confusing. Setting iommu=off sounds fine to me. The IOMMU core would then return a "no IOMMU present" error instead of -EPROBE_DEFER, and probing of the bus masters would continue without IOMMU support.
On Wednesday 17 December 2014 18:02:51 Laurent Pinchart wrote: > On Wednesday 17 December 2014 16:41:33 Arnd Bergmann wrote: > > On Wednesday 17 December 2014 16:39:02 Laurent Pinchart wrote: > > > On Wednesday 17 December 2014 15:27:36 Arnd Bergmann wrote: > > > > On Wednesday 17 December 2014 01:24:42 Laurent Pinchart wrote: > > > > > If we forbid the IOMMU driver from being compiled as a module can't we > > > > > just rely on deferred probing ? The bus master driver will just be > > > > > reprobed after the IOMMU gets probed, like for other devices. > > > > > > > > > > This could fail in case the IOMMU device permanently fails to probe. > > > > > There > > > > > would be something very wrong in the system in that case, Enabling the > > > > > bus > > > > > masters totally transparently without IOMMU support could not be such > > > > > a > > > > > good idea. > > > > > > > > I believe in the majority of cases today, the IOMMU is entirely > > > > optional. > > > > There are valid reasons for not including the IOMMU driver in the > > > > kernel, > > > > e.g. when you know that all the machines with that driver can DMA to > > > > all of their RAM and you want to avoid the overhead of IOTLB misses. > > > > > > Should that really be controlled by compiling the IOMMU driver out, > > > wouldn't it be better to disable the IOMMU devices in DT in that case ? > > > > It's a policy decision that should only depend on the user. Modifying the > > DT is wrong here IMHO because the device is still connected to the IOMMU > > in hardware and we should correctly represent that. > > I was thinking about setting status = "disabled" on the IOMMU nodes, not > removing the IOMMU references in the bus master nodes. But that still requires a modification of the DT. The hardware is the same, so I don't see why we should update the dtb based on kernel configuration. Arnd
Hi Arnd, On Wednesday 17 December 2014 22:58:47 Arnd Bergmann wrote: > On Wednesday 17 December 2014 18:02:51 Laurent Pinchart wrote: > > On Wednesday 17 December 2014 16:41:33 Arnd Bergmann wrote: > >> On Wednesday 17 December 2014 16:39:02 Laurent Pinchart wrote: > >>> On Wednesday 17 December 2014 15:27:36 Arnd Bergmann wrote: > >>>> On Wednesday 17 December 2014 01:24:42 Laurent Pinchart wrote: > >>>>> If we forbid the IOMMU driver from being compiled as a module > >>>>> can't we just rely on deferred probing ? The bus master driver > >>>>> will just be reprobed after the IOMMU gets probed, like for other > >>>>> devices. > >>>>> > >>>>> This could fail in case the IOMMU device permanently fails to > >>>>> probe. There would be something very wrong in the system in that > >>>>> case, Enabling the bus masters totally transparently without IOMMU > >>>>> support could not be such a good idea. > >>>> > >>>> I believe in the majority of cases today, the IOMMU is entirely > >>>> optional. There are valid reasons for not including the IOMMU driver > >>>> in the kernel, e.g. when you know that all the machines with that > >>>> driver can DMA to all of their RAM and you want to avoid the > >>>> overhead of IOTLB misses. > >>> > >>> Should that really be controlled by compiling the IOMMU driver out, > >>> wouldn't it be better to disable the IOMMU devices in DT in that case > >>> ? > >> > >> It's a policy decision that should only depend on the user. Modifying > >> the DT is wrong here IMHO because the device is still connected to the > >> IOMMU in hardware and we should correctly represent that. > > > > I was thinking about setting status = "disabled" on the IOMMU nodes, not > > removing the IOMMU references in the bus master nodes. > > But that still requires a modification of the DT. The hardware is the > same, so I don't see why we should update the dtb based on kernel > configuration. It wouldn't be the first time we encode configuration information in DT, but I agree it's not an optimal solution. Setting iommu=off on the kernel command line is better, and should be easy to implement.
Hi Arnd, On Wednesday 17 December 2014 16:56:53 Arnd Bergmann wrote: > On Wednesday 17 December 2014 15:53:25 Lucas Stach wrote: > > Am Mittwoch, den 17.12.2014, 15:27 +0100 schrieb Arnd Bergmann: > >> On Wednesday 17 December 2014 01:24:42 Laurent Pinchart wrote: > >>> If we forbid the IOMMU driver from being compiled as a module can't we > >>> just rely on deferred probing ? The bus master driver will just be > >>> reprobed after the IOMMU gets probed, like for other devices. > >>> > >>> This could fail in case the IOMMU device permanently fails to probe. > >>> There would be something very wrong in the system in that case, > >>> Enabling the bus masters totally transparently without IOMMU support > >>> could not be such a good idea. > >> > >> I believe in the majority of cases today, the IOMMU is entirely > >> optional. There are valid reasons for not including the IOMMU driver in > >> the kernel, e.g. when you know that all the machines with that driver > >> can DMA to all of their RAM and you want to avoid the overhead of IOTLB > >> misses. > > > > This is similar to the problems we discussed with the componentized > > device framework and in the end everybody agreed on a simple rule: > > if the device node is enabled in the DT there must be a driver bound to > > it before other devices depending on this node can be probed. > > > > If we apply the same logic to the IOMMU situation we get two > > possibilities to allow bypassing the IOMMU: > > 1. completely disable the IOMMU node in the DT > > 2. leave node enabled in DT, but add a bypass property > > > > Obviously the second option still requires to have the IOMMU driver > > available, but is more flexible. Right now everything depends on the > > IOMMU being in passthrough mode at reset with no driver bound. If a > > device comes around where this isn't the case thing will break with the > > current assumptions or the first option. > > > > Having the IOMMU node enabled in DT with no driver available to the > > kernel seems like an invalid configuration which should be expected to > > break. Exactly the same thing as with componentized devices... > > One differences here is that for the IOMMU, we should generally be able > to fall back to swiotlb Or to nothing at all if the device can DMA to the allocated memory directly. > (currently only on ARM64, not ARM32, until someone adds support). I can see > your point regarding machines that have a mandatory IOMMU with no fallback > when there is no driver, but we can support them by making the iommu driver > selected through Kconfig for that platform, while still allowing other > platforms to work with drivers left out of the kernel. The question is how to tell the kernel not to wait for an IOMMU that will never be there. Would a kernel command line argument be an acceptable solution or do we need something more automatic ?
On Thursday 18 December 2014 22:36:14 Laurent Pinchart wrote: > > > (currently only on ARM64, not ARM32, until someone adds support). I can see > > your point regarding machines that have a mandatory IOMMU with no fallback > > when there is no driver, but we can support them by making the iommu driver > > selected through Kconfig for that platform, while still allowing other > > platforms to work with drivers left out of the kernel. > > The question is how to tell the kernel not to wait for an IOMMU that will > never be there. Would a kernel command line argument be an acceptable solution > or do we need something more automatic ? I would hope that we can find an automatic solution without relying on the command line. Unfortunately, I also remembered one case in support of what Lucas mentioned and that would break this: There is at least one SoC that uses cache-coherent DMA only when the IOMMU (ARM SMMU IIRC)is enabled, but is non-coherent otherwise. We can't really express that in DT, so we actually do rely on the IOMMU driver to be present on this machine when any "iommus" properties are used, as they would always come in combination with "dma-coherent" properties that are otherwise wrong. We can still enforce this by requiring the smmu driver to be built into the kernel on this platform, but simply saying that the device cannot support DMA as long as there is an iommus property but no driver for it does make a lot of sense. Note that there are more than two ways that the kernel could treat the situation of probing a device with a valid iommus reference but no driver loaded for the iommu: a) assume all iommu drivers are initialized early, so use linear or swiotlb dma_map_ops, and probe the driver normally. This breaks the scenario with conditionally coherent devices, and requires doing the iommu init early b) assume all iommu drivers are initialized early, so disallow any DMA by setting a zero dma_mask but allow the driver to be probed using polling I/O mode (useful for slow devices like UART or SPI) This breaks devices that require DMA but can fall back to linear or swiotlb mappings, and requires doing the iommu init early. c) defer probing until an iommu driver is gets initialized. This breaks both the cases where we could fall back to a sane behavior without iommu, but it would let us use a proper driver with regular power management etc. Arnd
diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c index cd28dc09db39..88f9afe641a0 100644 --- a/drivers/iommu/exynos-iommu.c +++ b/drivers/iommu/exynos-iommu.c @@ -13,16 +13,21 @@ #endif #include <linux/clk.h> +#include <linux/dma-mapping.h> #include <linux/err.h> #include <linux/io.h> #include <linux/iommu.h> #include <linux/interrupt.h> #include <linux/list.h> +#include <linux/of.h> +#include <linux/of_iommu.h> +#include <linux/of_platform.h> #include <linux/platform_device.h> #include <linux/pm_runtime.h> #include <linux/slab.h> #include <asm/cacheflush.h> +#include <asm/dma-iommu.h> #include <asm/pgtable.h> typedef u32 sysmmu_iova_t; @@ -1084,6 +1089,8 @@ static const struct iommu_ops exynos_iommu_ops = { .pgsize_bitmap = SECT_SIZE | LPAGE_SIZE | SPAGE_SIZE, }; +static int init_done; + static int __init exynos_iommu_init(void) { int ret; @@ -1116,6 +1123,8 @@ static int __init exynos_iommu_init(void) goto err_set_iommu; } + init_done = true; + return 0; err_set_iommu: kmem_cache_free(lv2table_kmem_cache, zero_lv2_table); @@ -1125,4 +1134,21 @@ err_reg_driver: kmem_cache_destroy(lv2table_kmem_cache); return ret; } -subsys_initcall(exynos_iommu_init); + +static int __init exynos_iommu_of_setup(struct device_node *np) +{ + struct platform_device *pdev; + + if (!init_done) + exynos_iommu_init(); + + pdev = of_platform_device_create(np, NULL, platform_bus_type.dev_root); + if (IS_ERR(pdev)) + return PTR_ERR(pdev); + + of_iommu_set_ops(np, &exynos_iommu_ops); + return 0; +} + +IOMMU_OF_DECLARE(exynos_iommu_of, "samsung,exynos-sysmmu", + exynos_iommu_of_setup);
This patch introduces IOMMU_OF_DECLARE-based initialization to the driver, which replaces subsys_initcall-based procedure. exynos_iommu_of_setup ensures that each sysmmu controller is probed before its master device. Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> --- drivers/iommu/exynos-iommu.c | 28 +++++++++++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-)