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

login
register
mail settings
Submitter Hiroshi Doyu
Date Dec. 4, 2012, 6:37 p.m.
Message ID <1354646264-12126-1-git-send-email-hdoyu@nvidia.com>
Download mbox | patch
Permalink /patch/203717/
State Superseded, archived
Headers show

Comments

Hiroshi Doyu - Dec. 4, 2012, 6:37 p.m.
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(-)
Mark Zhang - Dec. 5, 2012, 6:29 a.m.
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
Stephen Warren - Dec. 5, 2012, 9:11 p.m.
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
Hiroshi Doyu - Dec. 7, 2012, 9:32 a.m.
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
Hiroshi Doyu - Dec. 7, 2012, 10:12 a.m.
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
Stephen Warren - Dec. 10, 2012, 5:43 p.m.
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

Patch

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);
 }