Re: [PATCHv5 2/9] driver/core: populate devices in order for IOMMUs

[Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>]

On 11/22/2013 12:41 AM, Grant Likely wrote:
> On Thu, 21 Nov 2013 12:04:18 -0700, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
>> On 11/21/2013 06:15 AM, Grant Likely wrote:
>>> On Tue, 19 Nov 2013 11:33:06 +0200, Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> 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. Once
>>>> an iommu device is populated, "dev->bus->iommu_ops" is set in the
>>>> bus. Then, those defered iommu master devices are populated and
>>>> configured for IOMMU with help of the already populated iommu device
>>>> via iommu_ops->add_device(). Multiple IOMMUs can be listed on this
>>>> "iommus" binding so that a device can have multiple IOMMUs attached.
>>>>
>>>> Signed-off-by: Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>>> ---
>>>> 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
>>>> ---
>>>>  drivers/base/dd.c        |  5 +++++
>>>>  drivers/iommu/of_iommu.c | 22 ++++++++++++++++++++++
>>>>  include/linux/of_iommu.h |  7 +++++++
>>>>  3 files changed, 34 insertions(+)
>>>>
>>>> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
>>>> index 35fa368..6e892d4 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)
>>>
>>> I'm very concerned about this approach. Hooking into the core probe
>>> path for things like this is not going to scale well. I'm not thrilled
>>> with the pinctrl hook being here either, but that is already merged. :-(
>>> Also, hooking in here is going affect every single device device driver
>>> probe path, and a large number of them are never, ever, going to have
>>> iommu interactions.
>>>
>>> There needs to be a less invasive way of doing what you want. I still
>>> feel like the individual device drivers themselves need to be aware that
>>> they might be hooking through an IOMMU. Or, if they are hooking through
>>> a bus like PCIe, then have the PCIe bus defer creating child devices
>>> until the IOMMU is configured and in place.
>>
>> I general though, couldn't any MMIO on-SoC device potentially be
>> affected by an IOMMU? The point of putting the call to of_iommu_attach()
>> here rather than inside individual driver's probe() is to avoid
>> requiring "every" driver having to paste more boiler-plate into probe().
> 
> It seems more that IOMMU attachment is closer to being a property of the
> bus rather than a property of the device itself. In that context it
> would make more sense for the bus device to hold off child device
> registration or probing until the IOMMU is available. That keeps the
> logic out of both the core code and the individual device drivers.

The bus structure that DT and Linux know about is the register bus.
There's no reason that devices have to emit their master transactions
onto that same bus, or onto only that same bus.