Patchwork [PATCHv7,04/12] driver/core: populate devices in order for IOMMUs

login
register
mail settings
Submitter Hiroshi Doyu
Date Dec. 12, 2013, 7:57 a.m.
Message ID <1386835033-4701-5-git-send-email-hdoyu@nvidia.com>
Download mbox | patch
Permalink /patch/300558/
State Superseded, archived
Headers show

Comments

Hiroshi Doyu - Dec. 12, 2013, 7:57 a.m.
IOMMU devices on the bus need to be poplulated first, then iommu
master devices are done later.

With CONFIG_OF_IOMMU, "iommus=" DT binding would be used to identify
whether a device can be an iommu msater or not. If a device can, we'll
defer to populate that device till an iommu device is populated. Then,
those deferred iommu master devices are populated and configured with
help of the already populated iommu device.

Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
This is related to the following discussion:
  [RFC PATCH] Documentation: devicetree: add description for generic bus properties
  http://lists.infradead.org/pipermail/linux-arm-kernel/2013-November/215042.html

v6:
Spinned off only driver core part from:
  [PATCHv5 2/9] driver/core: populate devices in order for IOMMUs

v5:
Use "iommus=" binding instread of arm,smmu's "#stream-id-cells".

v4:
This is newly added, and the successor of the following RFC:
  [RFC][PATCHv3+ 1/2] driver/core: Add of_iommu_attach()
  http://lists.linuxfoundation.org/pipermail/iommu/2013-November/006914.html

Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
---
 drivers/base/dd.c | 5 +++++
 1 file changed, 5 insertions(+)
Grant Likely - Dec. 12, 2013, 11:39 a.m.
On Thu, 12 Dec 2013 09:57:05 +0200, Hiroshi Doyu <hdoyu@nvidia.com> wrote:
> IOMMU devices on the bus need to be poplulated first, then iommu
> master devices are done later.
> 
> With CONFIG_OF_IOMMU, "iommus=" DT binding would be used to identify
> whether a device can be an iommu msater or not. If a device can, we'll
> defer to populate that device till an iommu device is populated. Then,
> those deferred iommu master devices are populated and configured with
> help of the already populated iommu device.
> 
> Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
> This is related to the following discussion:
>   [RFC PATCH] Documentation: devicetree: add description for generic bus properties
>   http://lists.infradead.org/pipermail/linux-arm-kernel/2013-November/215042.html
> 
> v6:
> Spinned off only driver core part from:
>   [PATCHv5 2/9] driver/core: populate devices in order for IOMMUs
> 
> v5:
> Use "iommus=" binding instread of arm,smmu's "#stream-id-cells".
> 
> v4:
> This is newly added, and the successor of the following RFC:
>   [RFC][PATCHv3+ 1/2] driver/core: Add of_iommu_attach()
>   http://lists.linuxfoundation.org/pipermail/iommu/2013-November/006914.html
> 
> Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
> ---
>  drivers/base/dd.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 0605176..0605f52 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -25,6 +25,7 @@
>  #include <linux/async.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/pinctrl/devinfo.h>
> +#include <linux/of_iommu.h>
>  
>  #include "base.h"
>  #include "power/power.h"
> @@ -273,6 +274,10 @@ static int really_probe(struct device *dev, struct device_driver *drv)
>  
>  	dev->driver = drv;
>  
> +	ret = of_iommu_attach(dev);
> +	if (ret)
> +		goto probe_failed;
> +

As discussed before, I really don't think hooking in to dd.c is the
right thing to do here, and certainly not as a device tree specific
function. ACPI or PCI described devices may have the same constraints
and those won't have DT descriptions.

I don't think we've got a good solution yet for this problem. :-(

g.
--
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
Greg KH - Dec. 13, 2013, 2:14 a.m.
On Thu, Dec 12, 2013 at 11:39:20AM +0000, Grant Likely wrote:
> On Thu, 12 Dec 2013 09:57:05 +0200, Hiroshi Doyu <hdoyu@nvidia.com> wrote:
> > IOMMU devices on the bus need to be poplulated first, then iommu
> > master devices are done later.
> > 
> > With CONFIG_OF_IOMMU, "iommus=" DT binding would be used to identify
> > whether a device can be an iommu msater or not. If a device can, we'll
> > defer to populate that device till an iommu device is populated. Then,
> > those deferred iommu master devices are populated and configured with
> > help of the already populated iommu device.
> > 
> > Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > ---
> > This is related to the following discussion:
> >   [RFC PATCH] Documentation: devicetree: add description for generic bus properties
> >   http://lists.infradead.org/pipermail/linux-arm-kernel/2013-November/215042.html
> > 
> > v6:
> > Spinned off only driver core part from:
> >   [PATCHv5 2/9] driver/core: populate devices in order for IOMMUs
> > 
> > v5:
> > Use "iommus=" binding instread of arm,smmu's "#stream-id-cells".
> > 
> > v4:
> > This is newly added, and the successor of the following RFC:
> >   [RFC][PATCHv3+ 1/2] driver/core: Add of_iommu_attach()
> >   http://lists.linuxfoundation.org/pipermail/iommu/2013-November/006914.html
> > 
> > Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
> > ---
> >  drivers/base/dd.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> > index 0605176..0605f52 100644
> > --- a/drivers/base/dd.c
> > +++ b/drivers/base/dd.c
> > @@ -25,6 +25,7 @@
> >  #include <linux/async.h>
> >  #include <linux/pm_runtime.h>
> >  #include <linux/pinctrl/devinfo.h>
> > +#include <linux/of_iommu.h>
> >  
> >  #include "base.h"
> >  #include "power/power.h"
> > @@ -273,6 +274,10 @@ static int really_probe(struct device *dev, struct device_driver *drv)
> >  
> >  	dev->driver = drv;
> >  
> > +	ret = of_iommu_attach(dev);
> > +	if (ret)
> > +		goto probe_failed;
> > +
> 
> As discussed before, I really don't think hooking in to dd.c is the
> right thing to do here, and certainly not as a device tree specific
> function. ACPI or PCI described devices may have the same constraints
> and those won't have DT descriptions.

I agree, this shouldn't be in the driver core.

greg k-h
--
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
Thierry Reding - Dec. 14, 2013, 12:24 p.m.
On Thu, Dec 12, 2013 at 06:14:02PM -0800, Greg KH wrote:
> On Thu, Dec 12, 2013 at 11:39:20AM +0000, Grant Likely wrote:
> > On Thu, 12 Dec 2013 09:57:05 +0200, Hiroshi Doyu <hdoyu@nvidia.com> wrote:
> > > IOMMU devices on the bus need to be poplulated first, then iommu
> > > master devices are done later.
> > > 
> > > With CONFIG_OF_IOMMU, "iommus=" DT binding would be used to identify
> > > whether a device can be an iommu msater or not. If a device can, we'll
> > > defer to populate that device till an iommu device is populated. Then,
> > > those deferred iommu master devices are populated and configured with
> > > help of the already populated iommu device.
> > > 
> > > Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
> > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > ---
> > > This is related to the following discussion:
> > >   [RFC PATCH] Documentation: devicetree: add description for generic bus properties
> > >   http://lists.infradead.org/pipermail/linux-arm-kernel/2013-November/215042.html
> > > 
> > > v6:
> > > Spinned off only driver core part from:
> > >   [PATCHv5 2/9] driver/core: populate devices in order for IOMMUs
> > > 
> > > v5:
> > > Use "iommus=" binding instread of arm,smmu's "#stream-id-cells".
> > > 
> > > v4:
> > > This is newly added, and the successor of the following RFC:
> > >   [RFC][PATCHv3+ 1/2] driver/core: Add of_iommu_attach()
> > >   http://lists.linuxfoundation.org/pipermail/iommu/2013-November/006914.html
> > > 
> > > Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
> > > ---
> > >  drivers/base/dd.c | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > > 
> > > diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> > > index 0605176..0605f52 100644
> > > --- a/drivers/base/dd.c
> > > +++ b/drivers/base/dd.c
> > > @@ -25,6 +25,7 @@
> > >  #include <linux/async.h>
> > >  #include <linux/pm_runtime.h>
> > >  #include <linux/pinctrl/devinfo.h>
> > > +#include <linux/of_iommu.h>
> > >  
> > >  #include "base.h"
> > >  #include "power/power.h"
> > > @@ -273,6 +274,10 @@ static int really_probe(struct device *dev, struct device_driver *drv)
> > >  
> > >  	dev->driver = drv;
> > >  
> > > +	ret = of_iommu_attach(dev);
> > > +	if (ret)
> > > +		goto probe_failed;
> > > +
> > 
> > As discussed before, I really don't think hooking in to dd.c is the
> > right thing to do here, and certainly not as a device tree specific
> > function. ACPI or PCI described devices may have the same constraints
> > and those won't have DT descriptions.
> 
> I agree, this shouldn't be in the driver core.

Okay, so what would be an alternative? Grant's objection makes sense and
we could easily just wrap the call to of_iommu_attach() within a generic
iommu_attach() that could decide at runtime which exact implementation
to call, depending on whether the device is DT, ACPI, PCI or whatnot.

If we don't want something like that in the core either, then the only
other alternative would be to call this from each driver. However given
the desire to handle IOMMUs completely transparently for device drivers
that would be missing the point.

Perhaps moving this into platform_drv_probe() would be more acceptable?
That's still somewhat core, but maybe suburban enough.

Thierry
Hiroshi Doyu - Dec. 14, 2013, 2:28 p.m.
Thierry Reding <thierry.reding@gmail.com> wrote @ Sat, 14 Dec 2013 13:24:22 +0100:

> * PGP Signed by an unknown key
> 
> On Thu, Dec 12, 2013 at 06:14:02PM -0800, Greg KH wrote:
> > On Thu, Dec 12, 2013 at 11:39:20AM +0000, Grant Likely wrote:
> > > On Thu, 12 Dec 2013 09:57:05 +0200, Hiroshi Doyu <hdoyu@nvidia.com> wrote:
> > > > IOMMU devices on the bus need to be poplulated first, then iommu
> > > > master devices are done later.
> > > > 
> > > > With CONFIG_OF_IOMMU, "iommus=" DT binding would be used to identify
> > > > whether a device can be an iommu msater or not. If a device can, we'll
> > > > defer to populate that device till an iommu device is populated. Then,
> > > > those deferred iommu master devices are populated and configured with
> > > > help of the already populated iommu device.
> > > > 
> > > > Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
> > > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > ---
> > > > This is related to the following discussion:
> > > >   [RFC PATCH] Documentation: devicetree: add description for generic bus properties
> > > >   http://lists.infradead.org/pipermail/linux-arm-kernel/2013-November/215042.html
> > > > 
> > > > v6:
> > > > Spinned off only driver core part from:
> > > >   [PATCHv5 2/9] driver/core: populate devices in order for IOMMUs
> > > > 
> > > > v5:
> > > > Use "iommus=" binding instread of arm,smmu's "#stream-id-cells".
> > > > 
> > > > v4:
> > > > This is newly added, and the successor of the following RFC:
> > > >   [RFC][PATCHv3+ 1/2] driver/core: Add of_iommu_attach()
> > > >   http://lists.linuxfoundation.org/pipermail/iommu/2013-November/006914.html
> > > > 
> > > > Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
> > > > ---
> > > >  drivers/base/dd.c | 5 +++++
> > > >  1 file changed, 5 insertions(+)
> > > > 
> > > > diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> > > > index 0605176..0605f52 100644
> > > > --- a/drivers/base/dd.c
> > > > +++ b/drivers/base/dd.c
> > > > @@ -25,6 +25,7 @@
> > > >  #include <linux/async.h>
> > > >  #include <linux/pm_runtime.h>
> > > >  #include <linux/pinctrl/devinfo.h>
> > > > +#include <linux/of_iommu.h>
> > > >  
> > > >  #include "base.h"
> > > >  #include "power/power.h"
> > > > @@ -273,6 +274,10 @@ static int really_probe(struct device *dev, struct device_driver *drv)
> > > >  
> > > >  	dev->driver = drv;
> > > >  
> > > > +	ret = of_iommu_attach(dev);
> > > > +	if (ret)
> > > > +		goto probe_failed;
> > > > +
> > > 
> > > As discussed before, I really don't think hooking in to dd.c is the
> > > right thing to do here, and certainly not as a device tree specific
> > > function. ACPI or PCI described devices may have the same constraints
> > > and those won't have DT descriptions.
> > 
> > I agree, this shouldn't be in the driver core.
> 
> Okay, so what would be an alternative? Grant's objection makes sense and
> we could easily just wrap the call to of_iommu_attach() within a generic
> iommu_attach() that could decide at runtime which exact implementation
> to call, depending on whether the device is DT, ACPI, PCI or whatnot.
> 
> If we don't want something like that in the core either, then the only
> other alternative would be to call this from each driver. However given
> the desire to handle IOMMUs completely transparently for device drivers
> that would be missing the point.

What about using "bus_notifier" to send -EPROBE_DEFER?

The current bus_notifier framework doesn't have the ability to defer
the probe, but I may think that this change is acceptable relatively.

The fundamental problem is that IOMMU doesn't follow the exact bus
model like "chained IOMMU" cases, but this discussion may take long to
be solved. I think that "bus_notifier" with send -EPROBE_DEFER would
cover most of the normal cases, like normal IOMMU device probe
population order.
--
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. 16, 2013, 6:26 p.m.
On 12/12/2013 07:14 PM, Greg KH wrote:
> On Thu, Dec 12, 2013 at 11:39:20AM +0000, Grant Likely wrote:
>> On Thu, 12 Dec 2013 09:57:05 +0200, Hiroshi Doyu <hdoyu@nvidia.com> wrote:
>>> IOMMU devices on the bus need to be poplulated first, then iommu
>>> master devices are done later.
>>>
>>> With CONFIG_OF_IOMMU, "iommus=" DT binding would be used to identify
>>> whether a device can be an iommu msater or not. If a device can, we'll
>>> defer to populate that device till an iommu device is populated. Then,
>>> those deferred iommu master devices are populated and configured with
>>> help of the already populated iommu device.

>>> This is related to the following discussion:
>>>   [RFC PATCH] Documentation: devicetree: add description for generic bus properties
>>>   http://lists.infradead.org/pipermail/linux-arm-kernel/2013-November/215042.html

>>> diff --git a/drivers/base/dd.c b/drivers/base/dd.c

>>> @@ -273,6 +274,10 @@ static int really_probe(struct device *dev, struct device_driver *drv)
>>>  
>>>  	dev->driver = drv;
>>>  
>>> +	ret = of_iommu_attach(dev);
>>> +	if (ret)
>>> +		goto probe_failed;
>>> +
>>
>> As discussed before, I really don't think hooking in to dd.c is the
>> right thing to do here, and certainly not as a device tree specific
>> function. ACPI or PCI described devices may have the same constraints
>> and those won't have DT descriptions.
> 
> I agree, this shouldn't be in the driver core.

I don't think I agree. It'd greatly simplify driver probe() routines if
the driver core could acquire/set up as many resources as it could on
behalf of drivers. It'd be nice if it pre-mapped any registers, acquired
clocks, regulators, ... That way, we wouldn't need to write all that
code in each individual driver's probe() routine. Now, in many cases
this is rather difficult since there's currently no way for the driver
core to know which resources a driver needs, but when the driver core
can/does know this, shouldn't it simplify the life of drivers?
Longer-term, perhaps drivers can provide the driver core with some
specification of the resources it needs (such as a list of clock,
regulator, ... names), to fill in the missing information.
--
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/base/dd.c b/drivers/base/dd.c
index 0605176..0605f52 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -25,6 +25,7 @@ 
 #include <linux/async.h>
 #include <linux/pm_runtime.h>
 #include <linux/pinctrl/devinfo.h>
+#include <linux/of_iommu.h>
 
 #include "base.h"
 #include "power/power.h"
@@ -273,6 +274,10 @@  static int really_probe(struct device *dev, struct device_driver *drv)
 
 	dev->driver = drv;
 
+	ret = of_iommu_attach(dev);
+	if (ret)
+		goto probe_failed;
+
 	/* If using pinctrl, bind pins now before probing */
 	ret = pinctrl_bind_pins(dev);
 	if (ret)