Re: [v3 1/1] iommu/tegra: smmu: bus_notifier registers platform IOMMU devices

2012-12-19 Thread Mark Zhang
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 0x4000;
 
 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 hd...@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);
  }
  
 
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [v3 1/1] iommu/tegra: smmu: bus_notifier registers platform IOMMU devices

2012-12-10 Thread Stephen Warren
On 12/07/2012 03:12 AM, Hiroshi Doyu wrote:
 Stephen Warren swar...@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 0x4000;

 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.

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [v3 1/1] iommu/tegra: smmu: bus_notifier registers platform IOMMU devices

2012-12-07 Thread Hiroshi Doyu
Mark Zhang nvmarkzh...@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 0x4000;
  
  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 hd...@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);
+   }
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [v3 1/1] iommu/tegra: smmu: bus_notifier registers platform IOMMU devices

2012-12-07 Thread Hiroshi Doyu
Stephen Warren swar...@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 0x4000;
 
  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.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu