| Message ID | 1354646264-12126-1-git-send-email-hdoyu@nvidia.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 12/05/2012 02:37 AM, Hiroshi Doyu wrote: > Most of platform devices are IOMMU'able in Tegra30 SoC. Registering > all IOMMU'able devices manually isn't nice. This patch allows > platform bus_notifier to register IOMMU devices. Map info can be > passed from DT. Info format is: > > dma-window = <0 0x40000000>; > > TODO: > A map can be shared with mutiple devices. This info also could be > passed from DT or default map can be set in advance. > > Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com> > --- > v3: Added bus_unregister_notifier > --- > drivers/iommu/tegra-smmu.c | 48 +++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 47 insertions(+), 1 deletion(-) > > diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c > index 25c1210..8fec9be 100644 > --- a/drivers/iommu/tegra-smmu.c > +++ b/drivers/iommu/tegra-smmu.c > @@ -38,6 +38,7 @@ > > #include <asm/page.h> > #include <asm/cacheflush.h> > +#include <asm/dma-iommu.h> > > enum smmu_hwgrp { > HWGRP_AFI, > @@ -1273,13 +1274,58 @@ static struct platform_driver tegra_smmu_driver = { > }, > }; > > +static int tegra_smmu_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)) { > + arm_iommu_release_mapping(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); If "arm_iommu_attach_device" fails, the "dev_err" and "dev_dbg" will all be called. > + break; > + default: > + break; > + } > + return NOTIFY_DONE; > +} > + > +static struct notifier_block tegra_smmu_device_nb = { > + .notifier_call = tegra_smmu_device_notifier, > +}; > + > static int __devinit tegra_smmu_init(void) > { > - return platform_driver_register(&tegra_smmu_driver); > + int err; > + > + err = platform_driver_register(&tegra_smmu_driver); > + if (err) > + return err; > + if (IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)) > + bus_register_notifier(&platform_bus_type, > + &tegra_smmu_device_nb); > + return 0; > } > > static void __exit tegra_smmu_exit(void) > { > + if (IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)) > + bus_unregister_notifier(&platform_bus_type, > + &tegra_smmu_device_nb); Okay. Here unregister the notifier, but where is the "arm_iommu_release_mapping" for every IOMMU'able devices? > platform_driver_unregister(&tegra_smmu_driver); > } > > -- 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 12/04/2012 11:37 AM, Hiroshi Doyu wrote: > Most of platform devices are IOMMU'able in Tegra30 SoC. Registering > all IOMMU'able devices manually isn't nice. This patch allows > platform bus_notifier to register IOMMU devices. Map info can be > passed from DT. Info format is: > > dma-window = <0 0x40000000>; > > TODO: > A map can be shared with mutiple devices. This info also could be > passed from DT or default map can be set in advance. > +static int tegra_smmu_device_notifier(struct notifier_block *nb, > + unsigned long event, void *_dev) > + 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); So, this ends up attempting to hook up the Tegra SMMU for /any/ node on a platform bus that has a dma-window property. That seems a little dangerous. I guess it's not true for Tegra30, but what if there are multiple buses on the SoC, each with a separate IOMMU - using this approach would mean the two IOMMU drivers end up fighting to register mappings for devices on each-others buses. I think the solution here is one of: a) Explicitly represent the bus structure in DT (something we don't do on Tegra at present), and have the bus itself indicate which IOMMU device is the IOMMU for that bus. b) Add another property to each device that has a dma-window, containing the phandle of the IOMMU that handles its accesses. The code above could then validate that it was the IOMMU for each device as it was registered. Probably, devices should not be registered until the IOMMU that serves them is available. Otherwise, can't a device probe, start making IO accesses using a physical/bus memory map, and then later have that yanked out and replaced by the IOMMU'd memory map. How is that co-ordinated? That somewhat argues for each device explicitly setting up the IOMMU registration, or the driver core handling the dependencies. Finally, what is the implication of the TODO above? Given the limited number of ASIDs on Tegra30, isn't that something we need to solve before deploying this patch. Presumably solving it requires a DT binding to be designed and reviewed? -- 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 @ Wed, 5 Dec 2012 07:29:42 +0100: > On 12/05/2012 02:37 AM, Hiroshi Doyu wrote: > > Most of platform devices are IOMMU'able in Tegra30 SoC. Registering > > all IOMMU'able devices manually isn't nice. This patch allows > > platform bus_notifier to register IOMMU devices. Map info can be > > passed from DT. Info format is: > > > > dma-window = <0 0x40000000>; > > > > TODO: > > A map can be shared with mutiple devices. This info also could be > > passed from DT or default map can be set in advance. > > > > Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com> > > --- > > v3: Added bus_unregister_notifier > > --- > > drivers/iommu/tegra-smmu.c | 48 +++++++++++++++++++++++++++++++++++++++++++- > > 1 file changed, 47 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c > > index 25c1210..8fec9be 100644 > > --- a/drivers/iommu/tegra-smmu.c > > +++ b/drivers/iommu/tegra-smmu.c > > @@ -38,6 +38,7 @@ > > > > #include <asm/page.h> > > #include <asm/cacheflush.h> > > +#include <asm/dma-iommu.h> > > > > enum smmu_hwgrp { > > HWGRP_AFI, > > @@ -1273,13 +1274,58 @@ static struct platform_driver tegra_smmu_driver = { > > }, > > }; > > > > +static int tegra_smmu_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)) { > > + arm_iommu_release_mapping(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); > > If "arm_iommu_attach_device" fails, the "dev_err" and "dev_dbg" will all > be called. > > > + break; > > + default: > > + break; > > + } > > + return NOTIFY_DONE; > > +} > > + > > +static struct notifier_block tegra_smmu_device_nb = { > > + .notifier_call = tegra_smmu_device_notifier, > > +}; > > + > > static int __devinit tegra_smmu_init(void) > > { > > - return platform_driver_register(&tegra_smmu_driver); > > + int err; > > + > > + err = platform_driver_register(&tegra_smmu_driver); > > + if (err) > > + return err; > > + if (IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)) > > + bus_register_notifier(&platform_bus_type, > > + &tegra_smmu_device_nb); > > + return 0; > > } > > > > static void __exit tegra_smmu_exit(void) > > { > > + if (IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)) > > + bus_unregister_notifier(&platform_bus_type, > > + &tegra_smmu_device_nb); > > Okay. Here unregister the notifier, but where is the > "arm_iommu_release_mapping" for every IOMMU'able devices? True. I think that this could be solved with: static void __exit tegra_smmu_exit(void) { + if (IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)) { + bus_for_each_dev(&platform_bus_type, NULL, NULL, + tegra_smmu_remove_map); + bus_unregister_notifier(&platform_bus_type, + &tegra_smmu_device_nb); + } -- 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
Stephen Warren <swarren@wwwdotorg.org> wrote @ Wed, 5 Dec 2012 22:11:40 +0100: > On 12/04/2012 11:37 AM, Hiroshi Doyu wrote: > > Most of platform devices are IOMMU'able in Tegra30 SoC. Registering > > all IOMMU'able devices manually isn't nice. This patch allows > > platform bus_notifier to register IOMMU devices. Map info can be > > passed from DT. Info format is: > > > > dma-window = <0 0x40000000>; > > > > TODO: > > A map can be shared with mutiple devices. This info also could be > > passed from DT or default map can be set in advance. > > > +static int tegra_smmu_device_notifier(struct notifier_block *nb, > > + unsigned long event, void *_dev) > > > + 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); > > So, this ends up attempting to hook up the Tegra SMMU for /any/ node on > a platform bus that has a dma-window property. That seems a little > dangerous. I guess it's not true for Tegra30, but what if there are > multiple buses on the SoC, each with a separate IOMMU - using this > approach would mean the two IOMMU drivers end up fighting to register > mappings for devices on each-others buses. > > I think the solution here is one of: > > a) Explicitly represent the bus structure in DT (something we don't do > on Tegra at present), and have the bus itself indicate which IOMMU > device is the IOMMU for that bus. AHB/APB/PPSB bus don't do much currently. New bus drivers for them would be used just for device registration. I'm not so sure if this direction is right or not. Could they live with the existing platfrom_driver? Any example would be appreciated if any. > b) Add another property to each device that has a dma-window, containing > the phandle of the IOMMU that handles its accesses. The code above could > then validate that it was the IOMMU for each device as it was registered. This sounds reasonably flexible because its only between dt and bus_notifier. > Probably, devices should not be registered until the IOMMU that serves > them is available. Otherwise, can't a device probe, start making IO > accesses using a physical/bus memory map, and then later have that > yanked out and replaced by the IOMMU'd memory map. How is that > co-ordinated? That somewhat argues for each device explicitly setting up > the IOMMU registration, or the driver core handling the > dependencies. I think that enabling/disabling IOMMU _tranparently_ to drivers may be beneficial basically because we don't want to deal with drivers with IOMMU enabled separately from the normal case. For dynamic enabling/disabling of IOMMU, *dma_ops in struct dev_archdata will be overwritten, and there may be inconsistency for the type of dma allocated buffers. Has there been other arch/platfrom to handle dynamic enabling/disalbing IOMMU this already, even for each drivers? > Finally, what is the implication of the TODO above? Given the limited > number of ASIDs on Tegra30, isn't that something we need to solve before > deploying this patch. Presumably solving it requires a DT binding to be > designed and reviewed? I sent this to merge already becuase additional phandle to a map could handle those TODO list flexibly. I'll post some examples for phandle to a map/a iommu if no objections for this direction. -- 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 12/07/2012 03:12 AM, Hiroshi Doyu wrote: > Stephen Warren <swarren@wwwdotorg.org> wrote @ Wed, 5 Dec 2012 22:11:40 +0100: > >> On 12/04/2012 11:37 AM, Hiroshi Doyu wrote: >>> Most of platform devices are IOMMU'able in Tegra30 SoC. Registering >>> all IOMMU'able devices manually isn't nice. This patch allows >>> platform bus_notifier to register IOMMU devices. Map info can be >>> passed from DT. Info format is: >>> >>> dma-window = <0 0x40000000>; >>> >>> TODO: >>> A map can be shared with mutiple devices. This info also could be >>> passed from DT or default map can be set in advance. >> >>> +static int tegra_smmu_device_notifier(struct notifier_block *nb, >>> + unsigned long event, void *_dev) >> >>> + 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); >> >> So, this ends up attempting to hook up the Tegra SMMU for /any/ node on >> a platform bus that has a dma-window property. That seems a little >> dangerous. I guess it's not true for Tegra30, but what if there are >> multiple buses on the SoC, each with a separate IOMMU - using this >> approach would mean the two IOMMU drivers end up fighting to register >> mappings for devices on each-others buses. >> >> I think the solution here is one of: >> >> a) Explicitly represent the bus structure in DT (something we don't do >> on Tegra at present), and have the bus itself indicate which IOMMU >> device is the IOMMU for that bus. > > AHB/APB/PPSB bus don't do much currently. New bus drivers for them > would be used just for device registration. I'm not so sure if this > direction is right or not. Could they live with the existing > platfrom_driver? Any example would be appreciated if any. I wasn't really thinking of creating a whole new driver for the bus; the existing simple-bus will automatically enumerate/instantiate all its children, and hence allow describing (at least part of) the SoC bus structure without writing any additional code. This appears to be used quite a bit already in arch/arm/boot/dts/*. ... >> Probably, devices should not be registered until the IOMMU that serves >> them is available. Otherwise, can't a device probe, start making IO >> accesses using a physical/bus memory map, and then later have that >> yanked out and replaced by the IOMMU'd memory map. How is that >> co-ordinated? That somewhat argues for each device explicitly setting up >> the IOMMU registration, or the driver core handling the >> dependencies. > > I think that enabling/disabling IOMMU _tranparently_ to drivers may be > beneficial basically because we don't want to deal with drivers with > IOMMU enabled separately from the normal case. > > For dynamic enabling/disabling of IOMMU, *dma_ops in struct > dev_archdata will be overwritten, and there may be inconsistency for > the type of dma allocated buffers. Has there been other arch/platfrom > to handle dynamic enabling/disalbing IOMMU this already, even for each > drivers? I don't know. -- 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/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c index 25c1210..8fec9be 100644 --- a/drivers/iommu/tegra-smmu.c +++ b/drivers/iommu/tegra-smmu.c @@ -38,6 +38,7 @@ #include <asm/page.h> #include <asm/cacheflush.h> +#include <asm/dma-iommu.h> enum smmu_hwgrp { HWGRP_AFI, @@ -1273,13 +1274,58 @@ static struct platform_driver tegra_smmu_driver = { }, }; +static int tegra_smmu_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)) { + arm_iommu_release_mapping(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; + default: + break; + } + return NOTIFY_DONE; +} + +static struct notifier_block tegra_smmu_device_nb = { + .notifier_call = tegra_smmu_device_notifier, +}; + static int __devinit tegra_smmu_init(void) { - return platform_driver_register(&tegra_smmu_driver); + int err; + + err = platform_driver_register(&tegra_smmu_driver); + if (err) + return err; + if (IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)) + bus_register_notifier(&platform_bus_type, + &tegra_smmu_device_nb); + return 0; } static void __exit tegra_smmu_exit(void) { + if (IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)) + bus_unregister_notifier(&platform_bus_type, + &tegra_smmu_device_nb); platform_driver_unregister(&tegra_smmu_driver); }
Most of platform devices are IOMMU'able in Tegra30 SoC. Registering all IOMMU'able devices manually isn't nice. This patch allows platform bus_notifier to register IOMMU devices. Map info can be passed from DT. Info format is: dma-window = <0 0x40000000>; TODO: A map can be shared with mutiple devices. This info also could be passed from DT or default map can be set in advance. Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com> --- v3: Added bus_unregister_notifier --- drivers/iommu/tegra-smmu.c | 48 +++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 47 insertions(+), 1 deletion(-)