| Message ID | 20121128.154832.539666140149950229.hdoyu@nvidia.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 11/28/2012 06:48 AM, Hiroshi Doyu wrote: > Hiroshi Doyu <hdoyu@nvidia.com> wrote @ Mon, 24 Sep 2012 14:50:14 +0300 (EEST): > ... >>>>> On Mon, 2012-09-24 at 12:04 +0300, Hiroshi Doyu wrote: >>>>>> diff --git a/drivers/base/platform.c b/drivers/base/platform.c >>>>>> index a1a7225..9eae3be 100644 >>>>>> --- a/drivers/base/platform.c >>>>>> +++ b/drivers/base/platform.c >>>>>> @@ -21,6 +21,8 @@ >>>>>> #include <linux/slab.h> >>>>>> #include <linux/pm_runtime.h> >>>>>> >>>>>> +#include <asm/dma-iommu.h> >>>>>> + >>>>>> #include "base.h" >>>>>> >>>>>> #define to_platform_driver(drv) (container_of((drv), struct >>>>>> platform_driver, \ >>>>>> @@ -305,8 +307,19 @@ int platform_device_add(struct platform_device >>>>>> *pdev) >>>>>> dev_name(&pdev->dev), dev_name(pdev->dev.parent)); >>>>>> >>>>>> ret = device_add(&pdev->dev); >>>>>> - if (ret == 0) >>>>>> - return ret; >>>>>> + if (ret) >>>>>> + goto failed; >>>>>> + >>>>>> +#ifdef CONFIG_PLATFORM_ENABLE_IOMMU >>>>>> + if (platform_bus_type.map && !pdev->dev.archdata.mapping) { >>>>>> + ret = arm_iommu_attach_device(&pdev->dev, >>>>>> + platform_bus_type.map); >>>>>> + if (ret) >>>>>> + goto failed; >>>>> >>>>> This is horrible ... you're adding an architecture specific callback >>>>> into our generic code; that's really a no-no. If the concept of >>>>> CONFIG_PLATFORM_ENABE_IOMMU is useful to more than just arm, then this >>>>> could become a generic callback. >>>> >>>> As mentioned in the original, this is a heck to explain what is >>>> needed. I am looking for some generic solution for how to specify >>>> IOMMU info for each platform devices. I'm guessing that some other SoC >>>> may have the similar requirements on the above. As you mentioned, this >>>> solution should be a generic, not arch specific. >>> >>> Please read more about bus notifiers. IMHO a good example is provided in >>> the following thread: >>> http://www.mail-archive.com/linux-samsung-soc@vger.kernel.org/msg12238.html >> >> This bus notifier seems enough flexible to afford the variation of >> IOMMU map info, like Tegra ASID, which could be platform-specific, and >> the other could be common too. There's already iommu_bus_notifier >> too. I'll try to implement something base on this. > > Experimentally implemented as below. With the followig patch, each > device could specify its own map in DT, and automatically the device > would be attached to the map. > > There is a case that some devices share a map. This patch doesn't > suppor such case yet. > > From 8cb75bb6f3a8535a077e0e85265f87c1f1289bfd Mon Sep 17 00:00:00 2001 > From: Hiroshi Doyu <hdoyu@nvidia.com> > Date: Wed, 28 Nov 2012 14:47:04 +0200 > Subject: [PATCH 1/1] ARM: tegra: bus_notifier registers IOMMU devices > > platform_bus notifier registers IOMMU devices if dma-window is > specified. > > Its format is: > dma-window = <"start" "size">; > ex) > dma-window = <0x12345000 0x8000>; > > Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com> > --- > arch/arm/mach-tegra/board-dt-tegra30.c | 40 ++++++++++++++++++++++++++++++++ Shouldn't this patch be to the IOMMU driver itself, not the core Tegra code? -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 28 Nov 2012 19:07:46 +0100 Stephen Warren <swarren@wwwdotorg.org> wrote: ...... > >>> Please read more about bus notifiers. IMHO a good example is provided in > >>> the following thread: > >>> http://www.mail-archive.com/linux-samsung-soc@vger.kernel.org/msg12238.html > >> > >> This bus notifier seems enough flexible to afford the variation of > >> IOMMU map info, like Tegra ASID, which could be platform-specific, and > >> the other could be common too. There's already iommu_bus_notifier > >> too. I'll try to implement something base on this. > > > > Experimentally implemented as below. With the followig patch, each > > device could specify its own map in DT, and automatically the device > > would be attached to the map. > > > > There is a case that some devices share a map. This patch doesn't > > suppor such case yet. > > > > From 8cb75bb6f3a8535a077e0e85265f87c1f1289bfd Mon Sep 17 00:00:00 2001 > > From: Hiroshi Doyu <hdoyu@nvidia.com> > > Date: Wed, 28 Nov 2012 14:47:04 +0200 > > Subject: [PATCH 1/1] ARM: tegra: bus_notifier registers IOMMU devices > > > > platform_bus notifier registers IOMMU devices if dma-window is > > specified. > > > > Its format is: > > dma-window = <"start" "size">; > > ex) > > dma-window = <0x12345000 0x8000>; > > > > Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com> > > --- > > arch/arm/mach-tegra/board-dt-tegra30.c | 40 ++++++++++++++++++++++++++++++++ > > Shouldn't this patch be to the IOMMU driver itself, not the core Tegra code? That could be possible and cleaner. I'll check if it works. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Nov 28, 2012 at 02:48:32PM +0100, Hiroshi Doyu wrote: [...] > From: Hiroshi Doyu <hdoyu@nvidia.com> > Date: Wed, 28 Nov 2012 14:47:04 +0200 > Subject: [PATCH 1/1] ARM: tegra: bus_notifier registers IOMMU devices > > platform_bus notifier registers IOMMU devices if dma-window is > specified. > > Its format is: > dma-window = <"start" "size">; > ex) > dma-window = <0x12345000 0x8000>; > > Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com> > --- > arch/arm/mach-tegra/board-dt-tegra30.c | 40 ++++++++++++++++++++++++++++++++ > 1 file changed, 40 insertions(+) > > diff --git a/arch/arm/mach-tegra/board-dt-tegra30.c b/arch/arm/mach-tegra/board-dt-tegra30.c > index a2b6cf1..570d718 100644 > --- a/arch/arm/mach-tegra/board-dt-tegra30.c > +++ b/arch/arm/mach-tegra/board-dt-tegra30.c > @@ -30,9 +30,11 @@ > #include <linux/of_fdt.h> > #include <linux/of_irq.h> > #include <linux/of_platform.h> > +#include <linux/of_iommu.h> > > #include <asm/mach/arch.h> > #include <asm/hardware/gic.h> > +#include <asm/dma-iommu.h> > > #include "board.h" > #include "clock.h" > @@ -86,10 +88,48 @@ static __initdata struct tegra_clk_init_table tegra_dt_clk_init_table[] = { > { NULL, NULL, 0, 0}, > }; > > +#ifdef CONFIG_ARM_DMA_USE_IOMMU > +static int tegra_iommu_device_notifier(struct notifier_block *nb, > + unsigned long event, void *_dev) > +{ > + struct dma_iommu_mapping *map = NULL; > + struct device *dev = _dev; > + dma_addr_t base; > + size_t size; > + int err; > + > + switch (event) { > + case BUS_NOTIFY_ADD_DEVICE: > + err = of_get_dma_window(dev->of_node, NULL, 0, NULL, &base, > + &size); > + if (!err) > + map = arm_iommu_create_mapping(&platform_bus_type, > + base, size, 0); > + if (IS_ERR_OR_NULL(map)) > + break; > + if (arm_iommu_attach_device(dev, map)) > + dev_err(dev, "Failed to attach %s\n", dev_name(dev)); > + dev_dbg(dev, "Attached %s to map %p\n", dev_name(dev), map); > + break; > + } > + return NOTIFY_DONE; > +} > +#else > +#define tegra_iommu_device_notifier NULL > +#endif > + > +static struct notifier_block tegra_iommu_device_nb = { > + .notifier_call = tegra_iommu_device_notifier, > +}; You don't need this extra protection since you use IS_ENABLED below and these are all static variables. The whole point of IS_ENABLED is to allow full compile coverage while leaving it up to the compiler to eliminate dead code. Thierry
On 11/28/2012 09:48 PM, Hiroshi Doyu wrote: > Hiroshi Doyu <hdoyu@nvidia.com> wrote @ Mon, 24 Sep 2012 14:50:14 +0300 (EEST): > ... >>>>> On Mon, 2012-09-24 at 12:04 +0300, Hiroshi Doyu wrote: >>>>>> diff --git a/drivers/base/platform.c b/drivers/base/platform.c >>>>>> index a1a7225..9eae3be 100644 >>>>>> --- a/drivers/base/platform.c >>>>>> +++ b/drivers/base/platform.c >>>>>> @@ -21,6 +21,8 @@ >>>>>> #include <linux/slab.h> >>>>>> #include <linux/pm_runtime.h> >>>>>> >>>>>> +#include <asm/dma-iommu.h> >>>>>> + >>>>>> #include "base.h" >>>>>> >>>>>> #define to_platform_driver(drv) (container_of((drv), struct >>>>>> platform_driver, \ >>>>>> @@ -305,8 +307,19 @@ int platform_device_add(struct platform_device >>>>>> *pdev) >>>>>> dev_name(&pdev->dev), dev_name(pdev->dev.parent)); >>>>>> >>>>>> ret = device_add(&pdev->dev); >>>>>> - if (ret == 0) >>>>>> - return ret; >>>>>> + if (ret) >>>>>> + goto failed; >>>>>> + >>>>>> +#ifdef CONFIG_PLATFORM_ENABLE_IOMMU >>>>>> + if (platform_bus_type.map && !pdev->dev.archdata.mapping) { >>>>>> + ret = arm_iommu_attach_device(&pdev->dev, >>>>>> + platform_bus_type.map); >>>>>> + if (ret) >>>>>> + goto failed; >>>>> >>>>> This is horrible ... you're adding an architecture specific callback >>>>> into our generic code; that's really a no-no. If the concept of >>>>> CONFIG_PLATFORM_ENABE_IOMMU is useful to more than just arm, then this >>>>> could become a generic callback. >>>> >>>> As mentioned in the original, this is a heck to explain what is >>>> needed. I am looking for some generic solution for how to specify >>>> IOMMU info for each platform devices. I'm guessing that some other SoC >>>> may have the similar requirements on the above. As you mentioned, this >>>> solution should be a generic, not arch specific. >>> >>> Please read more about bus notifiers. IMHO a good example is provided in >>> the following thread: >>> http://www.mail-archive.com/linux-samsung-soc@vger.kernel.org/msg12238.html >> >> This bus notifier seems enough flexible to afford the variation of >> IOMMU map info, like Tegra ASID, which could be platform-specific, and >> the other could be common too. There's already iommu_bus_notifier >> too. I'll try to implement something base on this. > > Experimentally implemented as below. With the followig patch, each > device could specify its own map in DT, and automatically the device > would be attached to the map. > > There is a case that some devices share a map. This patch doesn't > suppor such case yet. > > From 8cb75bb6f3a8535a077e0e85265f87c1f1289bfd Mon Sep 17 00:00:00 2001 > From: Hiroshi Doyu <hdoyu@nvidia.com> > Date: Wed, 28 Nov 2012 14:47:04 +0200 > Subject: [PATCH 1/1] ARM: tegra: bus_notifier registers IOMMU devices > > platform_bus notifier registers IOMMU devices if dma-window is > specified. > > Its format is: > dma-window = <"start" "size">; > ex) > dma-window = <0x12345000 0x8000>; > > Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com> > --- > arch/arm/mach-tegra/board-dt-tegra30.c | 40 ++++++++++++++++++++++++++++++++ > 1 file changed, 40 insertions(+) > > diff --git a/arch/arm/mach-tegra/board-dt-tegra30.c b/arch/arm/mach-tegra/board-dt-tegra30.c > index a2b6cf1..570d718 100644 > --- a/arch/arm/mach-tegra/board-dt-tegra30.c > +++ b/arch/arm/mach-tegra/board-dt-tegra30.c > @@ -30,9 +30,11 @@ > #include <linux/of_fdt.h> > #include <linux/of_irq.h> > #include <linux/of_platform.h> > +#include <linux/of_iommu.h> > > #include <asm/mach/arch.h> > #include <asm/hardware/gic.h> > +#include <asm/dma-iommu.h> > > #include "board.h" > #include "clock.h" > @@ -86,10 +88,48 @@ static __initdata struct tegra_clk_init_table tegra_dt_clk_init_table[] = { > { NULL, NULL, 0, 0}, > }; > > +#ifdef CONFIG_ARM_DMA_USE_IOMMU > +static int tegra_iommu_device_notifier(struct notifier_block *nb, > + unsigned long event, void *_dev) > +{ > + struct dma_iommu_mapping *map = NULL; > + struct device *dev = _dev; > + dma_addr_t base; > + size_t size; > + int err; > + > + switch (event) { > + case BUS_NOTIFY_ADD_DEVICE: > + err = of_get_dma_window(dev->of_node, NULL, 0, NULL, &base, > + &size); > + if (!err) > + map = arm_iommu_create_mapping(&platform_bus_type, > + base, size, 0); > + if (IS_ERR_OR_NULL(map)) > + break; > + if (arm_iommu_attach_device(dev, map)) Add "arm_iommu_release_mapping" here. And finally we see this patch, that's great. :) > + dev_err(dev, "Failed to attach %s\n", dev_name(dev)); > + dev_dbg(dev, "Attached %s to map %p\n", dev_name(dev), map); > + break; > + } > + return NOTIFY_DONE; > +} > +#else > +#define tegra_iommu_device_notifier NULL > +#endif > + > +static struct notifier_block tegra_iommu_device_nb = { > + .notifier_call = tegra_iommu_device_notifier, > +}; > + > static void __init tegra30_dt_init(void) > { > tegra_clk_init_from_table(tegra_dt_clk_init_table); > > + if (IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)) > + bus_register_notifier(&platform_bus_type, > + &tegra_iommu_device_nb); > + > of_platform_populate(NULL, of_default_bus_match_table, > tegra30_auxdata_lookup, NULL); > } > -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Mark Zhang <nvmarkzhang@gmail.com> wrote @ Fri, 30 Nov 2012 05:59:32 +0100: > On 11/28/2012 09:48 PM, Hiroshi Doyu wrote: > > Hiroshi Doyu <hdoyu@nvidia.com> wrote @ Mon, 24 Sep 2012 14:50:14 +0300 (EEST): > > ... > >>>>> On Mon, 2012-09-24 at 12:04 +0300, Hiroshi Doyu wrote: > >>>>>> diff --git a/drivers/base/platform.c b/drivers/base/platform.c > >>>>>> index a1a7225..9eae3be 100644 > >>>>>> --- a/drivers/base/platform.c > >>>>>> +++ b/drivers/base/platform.c > >>>>>> @@ -21,6 +21,8 @@ > >>>>>> #include <linux/slab.h> > >>>>>> #include <linux/pm_runtime.h> > >>>>>> > >>>>>> +#include <asm/dma-iommu.h> > >>>>>> + > >>>>>> #include "base.h" > >>>>>> > >>>>>> #define to_platform_driver(drv) (container_of((drv), struct > >>>>>> platform_driver, \ > >>>>>> @@ -305,8 +307,19 @@ int platform_device_add(struct platform_device > >>>>>> *pdev) > >>>>>> dev_name(&pdev->dev), dev_name(pdev->dev.parent)); > >>>>>> > >>>>>> ret = device_add(&pdev->dev); > >>>>>> - if (ret == 0) > >>>>>> - return ret; > >>>>>> + if (ret) > >>>>>> + goto failed; > >>>>>> + > >>>>>> +#ifdef CONFIG_PLATFORM_ENABLE_IOMMU > >>>>>> + if (platform_bus_type.map && !pdev->dev.archdata.mapping) { > >>>>>> + ret = arm_iommu_attach_device(&pdev->dev, > >>>>>> + platform_bus_type.map); > >>>>>> + if (ret) > >>>>>> + goto failed; > >>>>> > >>>>> This is horrible ... you're adding an architecture specific callback > >>>>> into our generic code; that's really a no-no. If the concept of > >>>>> CONFIG_PLATFORM_ENABE_IOMMU is useful to more than just arm, then this > >>>>> could become a generic callback. > >>>> > >>>> As mentioned in the original, this is a heck to explain what is > >>>> needed. I am looking for some generic solution for how to specify > >>>> IOMMU info for each platform devices. I'm guessing that some other SoC > >>>> may have the similar requirements on the above. As you mentioned, this > >>>> solution should be a generic, not arch specific. > >>> > >>> Please read more about bus notifiers. IMHO a good example is provided in > >>> the following thread: > >>> http://www.mail-archive.com/linux-samsung-soc@vger.kernel.org/msg12238.html > >> > >> This bus notifier seems enough flexible to afford the variation of > >> IOMMU map info, like Tegra ASID, which could be platform-specific, and > >> the other could be common too. There's already iommu_bus_notifier > >> too. I'll try to implement something base on this. > > > > Experimentally implemented as below. With the followig patch, each > > device could specify its own map in DT, and automatically the device > > would be attached to the map. > > > > There is a case that some devices share a map. This patch doesn't > > suppor such case yet. > > > > From 8cb75bb6f3a8535a077e0e85265f87c1f1289bfd Mon Sep 17 00:00:00 2001 > > From: Hiroshi Doyu <hdoyu@nvidia.com> > > Date: Wed, 28 Nov 2012 14:47:04 +0200 > > Subject: [PATCH 1/1] ARM: tegra: bus_notifier registers IOMMU devices > > > > platform_bus notifier registers IOMMU devices if dma-window is > > specified. > > > > Its format is: > > dma-window = <"start" "size">; > > ex) > > dma-window = <0x12345000 0x8000>; > > > > Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com> > > --- > > arch/arm/mach-tegra/board-dt-tegra30.c | 40 ++++++++++++++++++++++++++++++++ > > 1 file changed, 40 insertions(+) > > > > diff --git a/arch/arm/mach-tegra/board-dt-tegra30.c b/arch/arm/mach-tegra/board-dt-tegra30.c > > index a2b6cf1..570d718 100644 > > --- a/arch/arm/mach-tegra/board-dt-tegra30.c > > +++ b/arch/arm/mach-tegra/board-dt-tegra30.c > > @@ -30,9 +30,11 @@ > > #include <linux/of_fdt.h> > > #include <linux/of_irq.h> > > #include <linux/of_platform.h> > > +#include <linux/of_iommu.h> > > > > #include <asm/mach/arch.h> > > #include <asm/hardware/gic.h> > > +#include <asm/dma-iommu.h> > > > > #include "board.h" > > #include "clock.h" > > @@ -86,10 +88,48 @@ static __initdata struct tegra_clk_init_table tegra_dt_clk_init_table[] = { > > { NULL, NULL, 0, 0}, > > }; > > > > +#ifdef CONFIG_ARM_DMA_USE_IOMMU > > +static int tegra_iommu_device_notifier(struct notifier_block *nb, > > + unsigned long event, void *_dev) > > +{ > > + struct dma_iommu_mapping *map = NULL; > > + struct device *dev = _dev; > > + dma_addr_t base; > > + size_t size; > > + int err; > > + > > + switch (event) { > > + case BUS_NOTIFY_ADD_DEVICE: > > + err = of_get_dma_window(dev->of_node, NULL, 0, NULL, &base, > > + &size); > > + if (!err) > > + map = arm_iommu_create_mapping(&platform_bus_type, > > + base, size, 0); > > + if (IS_ERR_OR_NULL(map)) > > + break; > > + if (arm_iommu_attach_device(dev, map)) > > Add "arm_iommu_release_mapping" here. Yes. > And finally we see this patch, that's great. :) I'll move the location of patch to drivers/iommu/tegra-smmu.c and repost. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/arm/mach-tegra/board-dt-tegra30.c b/arch/arm/mach-tegra/board-dt-tegra30.c index a2b6cf1..570d718 100644 --- a/arch/arm/mach-tegra/board-dt-tegra30.c +++ b/arch/arm/mach-tegra/board-dt-tegra30.c @@ -30,9 +30,11 @@ #include <linux/of_fdt.h> #include <linux/of_irq.h> #include <linux/of_platform.h> +#include <linux/of_iommu.h> #include <asm/mach/arch.h> #include <asm/hardware/gic.h> +#include <asm/dma-iommu.h> #include "board.h" #include "clock.h" @@ -86,10 +88,48 @@ static __initdata struct tegra_clk_init_table tegra_dt_clk_init_table[] = { { NULL, NULL, 0, 0}, }; +#ifdef CONFIG_ARM_DMA_USE_IOMMU +static int tegra_iommu_device_notifier(struct notifier_block *nb, + unsigned long event, void *_dev) +{ + struct dma_iommu_mapping *map = NULL; + struct device *dev = _dev; + dma_addr_t base; + size_t size; + int err; + + switch (event) { + case BUS_NOTIFY_ADD_DEVICE: + err = of_get_dma_window(dev->of_node, NULL, 0, NULL, &base, + &size); + if (!err) + map = arm_iommu_create_mapping(&platform_bus_type, + base, size, 0); + if (IS_ERR_OR_NULL(map)) + break; + if (arm_iommu_attach_device(dev, map)) + dev_err(dev, "Failed to attach %s\n", dev_name(dev)); + dev_dbg(dev, "Attached %s to map %p\n", dev_name(dev), map); + break; + } + return NOTIFY_DONE; +} +#else +#define tegra_iommu_device_notifier NULL +#endif + +static struct notifier_block tegra_iommu_device_nb = { + .notifier_call = tegra_iommu_device_notifier, +}; + static void __init tegra30_dt_init(void) { tegra_clk_init_from_table(tegra_dt_clk_init_table); + if (IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)) + bus_register_notifier(&platform_bus_type, + &tegra_iommu_device_nb); + of_platform_populate(NULL, of_default_bus_match_table, tegra30_auxdata_lookup, NULL); }