Reparenting a platform device

Reparenting a platform device

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 2880 bytes --]

Hi,

I have a device tree where I have a GART device and a DRM device which uses
the GART. The GART is implemented by an IOMMU driver (tegra-gart) and
requires the user device to be a child of the GART device (it explicitly
checks for this when the user device is attached).

I've tried two alternatives to achieve this: create the GART device in the
user driver's .probe() function and explicitly set the DRM device's parent
to the resulting platform device like so:

	gart = platform_device_alloc(...);
	...
	pdev->dev.parent = &gart->dev;

The alternative is to use the device tree to look up the GART device node and
resolve it to the corresponding struct device:

	gart_node = of_parse_phandle(drm->dev->of_node, "gart-parent", 0);
	gart = bus_find_device(drm->dev->bus, NULL, gart_node, match_of_node);

Where match_of_node matches each struct device's .of_node field to the
gart_node.

Both of these variants seem to work, and the DRM device can be properly
attached to the GART device. However, after the DRM driver's .probe() exits,
I get the following error:

	[   25.579261] ------------[ cut here ]------------
	[   25.583895] WARNING: at /home/thierry.reding/src/kernel/linux-ipmp.git/kernel/mutex-debug.c:78 debug_mutex_unlock+0x114/0x128()
	[   25.595352] Modules linked in: tegra_drm(+) cfbfillrect cfbimgblt cfbcopyarea drm_kms_helper drm fb pwm_tegra pwm_bl backlight
	[   25.606821] [<c0013c54>] (unwind_backtrace+0x0/0xf8) from [<c0027bcc>] (warn_slowpath_common+0x4c/0x64)
	[   25.616207] [<c0027bcc>] (warn_slowpath_common+0x4c/0x64) from [<c0027c00>] (warn_slowpath_null+0x1c/0x24)
	[   25.625853] [<c0027c00>] (warn_slowpath_null+0x1c/0x24) from [<c00667fc>] (debug_mutex_unlock+0x114/0x128)
	[   25.635508] [<c00667fc>] (debug_mutex_unlock+0x114/0x128) from [<c03bee2c>] (__mutex_unlock_slowpath+0x84/0x140)
	[   25.645680] [<c03bee2c>] (__mutex_unlock_slowpath+0x84/0x140) from [<c02149b4>] (__driver_attach+0x78/0x90)
	[   25.655412] [<c02149b4>] (__driver_attach+0x78/0x90) from [<c0213128>] (bus_for_each_dev+0x50/0x7c)
	[   25.664449] [<c0213128>] (bus_for_each_dev+0x50/0x7c) from [<c0214038>] (bus_add_driver+0x174/0x234)
	[   25.673572] [<c0214038>] (bus_add_driver+0x174/0x234) from [<c0214e7c>] (driver_register+0x78/0x12c)
	[   25.682696] [<c0214e7c>] (driver_register+0x78/0x12c) from [<c0008620>] (do_one_initcall+0x34/0x174)
	[   25.691826] [<c0008620>] (do_one_initcall+0x34/0x174) from [<c006dab0>] (sys_init_module+0xc20/0x18e0)
	[   25.701131] [<c006dab0>] (sys_init_module+0xc20/0x18e0) from [<c000df00>] (ret_fast_syscall+0x0/0x30)
	[   25.710336] ---[ end trace f64968a4a9d9fe8b ]---

I may be misinterpreting things, but this looks to be caused by the
reparenting. So the question is whether what I'm trying to do is valid or
if there is some other way to properly make the GART device the parent of
the DRM device.

Thanks,
Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: Reparenting a platform device

[Stephen Warren]

On 04/05/2012 02:42 AM, Thierry Reding wrote:
> Hi,
> 
> I have a device tree where I have a GART device and a DRM device which uses
> the GART. The GART is implemented by an IOMMU driver (tegra-gart) and
> requires the user device to be a child of the GART device (it explicitly
> checks for this when the user device is attached).

Isn't this wrong?

I would expect the device parent/child relationship to reflect the
CPU-initiated register access bus topology.

A device's interaction with an IOMMU is an aspect of a device's
initiating accesses itself, not CPU-initiated register accesses.

> I've tried two alternatives to achieve this: create the GART device in the
> user driver's .probe() function and explicitly set the DRM device's parent
> to the resulting platform device like so:
> 
> 	gart = platform_device_alloc(...);
> 	...
> 	pdev->dev.parent = &gart->dev;

I guess that won't work when there's more than one device affected by
the IOMMU?

> The alternative is to use the device tree to look up the GART device node and
> resolve it to the corresponding struct device:
> 
> 	gart_node = of_parse_phandle(drm->dev->of_node, "gart-parent", 0);

That seems more logical to me.

> 	gart = bus_find_device(drm->dev->bus, NULL, gart_node, match_of_node);

That part should probably be encapsulated into the IOMMU subsystem? In
fact, even the of_parse_phandle should perhaps be hidden inside some
IOMMU iommu_get() call, that can use DT as a data source, or some other
data structure set up by board files.

Re: Reparenting a platform device

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 2979 bytes --]

* Stephen Warren wrote:
> On 04/05/2012 02:42 AM, Thierry Reding wrote:
> > Hi,
> > 
> > I have a device tree where I have a GART device and a DRM device which uses
> > the GART. The GART is implemented by an IOMMU driver (tegra-gart) and
> > requires the user device to be a child of the GART device (it explicitly
> > checks for this when the user device is attached).
> 
> Isn't this wrong?
> 
> I would expect the device parent/child relationship to reflect the
> CPU-initiated register access bus topology.
> 
> A device's interaction with an IOMMU is an aspect of a device's
> initiating accesses itself, not CPU-initiated register accesses.

Actually I have no idea why this was made a requirement. Maybe Hiroshi can
comment on this. The driver really only needs this to basically obtain a
pointer to itself. The MSM I/O MMU implementation does something similar,
though, and goes on to register actual child devices (they are instantiated
in arch/arm/mach-msm/devices-iommu.c). Each of those devices is then assigned
a specific memory area it seems.

> > I've tried two alternatives to achieve this: create the GART device in the
> > user driver's .probe() function and explicitly set the DRM device's parent
> > to the resulting platform device like so:
> > 
> > 	gart = platform_device_alloc(...);
> > 	...
> > 	pdev->dev.parent = &gart->dev;
> 
> I guess that won't work when there's more than one device affected by
> the IOMMU?

I don't think having more than one device using the IOMMU will work properly
anyway in the context of the Tegra GART driver because there is not means to
allocate specific regions of the GART aperture to individual devices. So
really the one and only client actually needs to manage the allocations from
the GART aperture.

I'm also not sure if it makes much sense to use the GART from anything other
than the DRM driver.

> > The alternative is to use the device tree to look up the GART device node and
> > resolve it to the corresponding struct device:
> > 
> > 	gart_node = of_parse_phandle(drm->dev->of_node, "gart-parent", 0);
> 
> That seems more logical to me.
> 
> > 	gart = bus_find_device(drm->dev->bus, NULL, gart_node, match_of_node);
> 
> That part should probably be encapsulated into the IOMMU subsystem? In
> fact, even the of_parse_phandle should perhaps be hidden inside some
> IOMMU iommu_get() call, that can use DT as a data source, or some other
> data structure set up by board files.

Both of these methods don't work too well because they actually require a
reordering of the device hierarchy at runtime. For some reason this leads to
the mutex becoming confused and the warning that I posted. I came up with a
third alternative that does the reparenting in board-dt-tegra20.c after the
call to of_platform_populate() which works similar to the second version and
gets rid of the warning.

I'm still not at all happy with it, though.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 190 bytes --]

_______________________________________________
iommu mailing list
iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: Reparenting a platform device

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1942 bytes --]

* Thierry Reding wrote:
> * Stephen Warren wrote:
> > On 04/05/2012 02:42 AM, Thierry Reding wrote:
> > > Hi,
> > > 
> > > I have a device tree where I have a GART device and a DRM device which uses
> > > the GART. The GART is implemented by an IOMMU driver (tegra-gart) and
> > > requires the user device to be a child of the GART device (it explicitly
> > > checks for this when the user device is attached).
> > 
> > Isn't this wrong?
> > 
> > I would expect the device parent/child relationship to reflect the
> > CPU-initiated register access bus topology.
> > 
> > A device's interaction with an IOMMU is an aspect of a device's
> > initiating accesses itself, not CPU-initiated register accesses.
> 
> Actually I have no idea why this was made a requirement. Maybe Hiroshi can
> comment on this. The driver really only needs this to basically obtain a
> pointer to itself. The MSM I/O MMU implementation does something similar,
> though, and goes on to register actual child devices (they are instantiated
> in arch/arm/mach-msm/devices-iommu.c). Each of those devices is then assigned
> a specific memory area it seems.
[...]
> I don't think having more than one device using the IOMMU will work properly
> anyway in the context of the Tegra GART driver because there is not means to
> allocate specific regions of the GART aperture to individual devices. So
> really the one and only client actually needs to manage the allocations from
> the GART aperture.

I've been thinking about this some more and it occurred to me that maybe it
would be best to add an additional layer between the GART and the clients
which could manage the allocations. Such a virtual device could actually be
registered as a child of the GART device and allow individual drivers to
request mappings from it so that there is a single instance handing them out
and keep them consistent.

How does that sound?

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: Reparenting a platform device

[Stephen Warren]

On 04/05/2012 10:23 AM, Thierry Reding wrote:
> * Stephen Warren wrote:
>> On 04/05/2012 02:42 AM, Thierry Reding wrote:
>>> Hi,
>>>
>>> I have a device tree where I have a GART device and a DRM device which uses
>>> the GART. The GART is implemented by an IOMMU driver (tegra-gart) and
>>> requires the user device to be a child of the GART device (it explicitly
>>> checks for this when the user device is attached).
>>
>> Isn't this wrong?
>>
>> I would expect the device parent/child relationship to reflect the
>> CPU-initiated register access bus topology.
>>
>> A device's interaction with an IOMMU is an aspect of a device's
>> initiating accesses itself, not CPU-initiated register accesses.
> 
> Actually I have no idea why this was made a requirement. Maybe Hiroshi can
> comment on this. The driver really only needs this to basically obtain a
> pointer to itself. The MSM I/O MMU implementation does something similar,
> though, and goes on to register actual child devices (they are instantiated
> in arch/arm/mach-msm/devices-iommu.c). Each of those devices is then assigned
> a specific memory area it seems.
> 
>>> I've tried two alternatives to achieve this: create the GART device in the
>>> user driver's .probe() function and explicitly set the DRM device's parent
>>> to the resulting platform device like so:
>>>
>>> 	gart = platform_device_alloc(...);
>>> 	...
>>> 	pdev->dev.parent = &gart->dev;
>>
>> I guess that won't work when there's more than one device affected by
>> the IOMMU?
> 
> I don't think having more than one device using the IOMMU will work properly
> anyway in the context of the Tegra GART driver because there is not means to
> allocate specific regions of the GART aperture to individual devices. So
> really the one and only client actually needs to manage the allocations from
> the GART aperture.
> 
> I'm also not sure if it makes much sense to use the GART from anything other
> than the DRM driver.

I was thinking about this mostly from a Tegra30 perspective, where there
are multiple devices affected by the SMMU, which is more capable that
the Tegra20 GART, and can presumably support these multiple clients
pretty independently. For Tegra20, I wouldn't be surprised if the DRM
driver was the only client.

Re: Reparenting a platform device

[Stephen Warren]

On 04/05/2012 12:08 PM, Thierry Reding wrote:
> * Thierry Reding wrote:
>> * Stephen Warren wrote:
>>> On 04/05/2012 02:42 AM, Thierry Reding wrote:
>>>> Hi,
>>>>
>>>> I have a device tree where I have a GART device and a DRM device which uses
>>>> the GART. The GART is implemented by an IOMMU driver (tegra-gart) and
>>>> requires the user device to be a child of the GART device (it explicitly
>>>> checks for this when the user device is attached).
>>>
>>> Isn't this wrong?
>>>
>>> I would expect the device parent/child relationship to reflect the
>>> CPU-initiated register access bus topology.
>>>
>>> A device's interaction with an IOMMU is an aspect of a device's
>>> initiating accesses itself, not CPU-initiated register accesses.
>>
>> Actually I have no idea why this was made a requirement. Maybe Hiroshi can
>> comment on this. The driver really only needs this to basically obtain a
>> pointer to itself. The MSM I/O MMU implementation does something similar,
>> though, and goes on to register actual child devices (they are instantiated
>> in arch/arm/mach-msm/devices-iommu.c). Each of those devices is then assigned
>> a specific memory area it seems.
> [...]
>> I don't think having more than one device using the IOMMU will work properly
>> anyway in the context of the Tegra GART driver because there is not means to
>> allocate specific regions of the GART aperture to individual devices. So
>> really the one and only client actually needs to manage the allocations from
>> the GART aperture.
> 
> I've been thinking about this some more and it occurred to me that maybe it
> would be best to add an additional layer between the GART and the clients
> which could manage the allocations. Such a virtual device could actually be
> registered as a child of the GART device and allow individual drivers to
> request mappings from it so that there is a single instance handing them out
> and keep them consistent.
> 
> How does that sound?

I must admit I'm not at all familiar with the IOMMU APIs, but isn't the
IOMMU driver/subsystem itself what is managing all the allocations and
handing them out to clients? And client drivers do things like asking
for N pages of memory mapped into their aperture? If that is true, I'm
not sure what the purpose is of the intermediate device you propose.
Sorry for being somewhat clueless.

Re: Reparenting a platform device

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1346 bytes --]

* Stephen Warren wrote:
> I must admit I'm not at all familiar with the IOMMU APIs, but isn't the
> IOMMU driver/subsystem itself what is managing all the allocations and
> handing them out to clients? And client drivers do things like asking
> for N pages of memory mapped into their aperture? If that is true, I'm
> not sure what the purpose is of the intermediate device you propose.
> Sorry for being somewhat clueless.

That was my impression too at first. But it seems like all the IOMMU
subsystem does is map individual pages. So you pass it a physical address of
a page along with the IO virtual address to map it to. That's at least the
way it is handled for Tegra 2 GART (and from a quick look also by the Tegra 3
SMMU). For the SMMU the situation may be different because it may not have a
fixed aperture that needs to contain the IO virtual addresses, though I must
admit I haven't looked at Tegra 3 in enough detail to judge this.

So this intermediate device would be purely an allocator for the GART
aperture and handle the actual mapping via the IOMMU. This would probably be
really simple and is in fact now done in the DRM driver. The nice thing if
this would be a separate device is that it would be easy to make it a child
of the IOMMU and wouldn't be as counter-intuitive as making the DRM device
the IOMMU's child.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 190 bytes --]

_______________________________________________
iommu mailing list
iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: Reparenting a platform device

[Stephen Warren]

On 04/06/2012 01:20 AM, Thierry Reding wrote:
> * Stephen Warren wrote:
>> I must admit I'm not at all familiar with the IOMMU APIs, but isn't the
>> IOMMU driver/subsystem itself what is managing all the allocations and
>> handing them out to clients? And client drivers do things like asking
>> for N pages of memory mapped into their aperture? If that is true, I'm
>> not sure what the purpose is of the intermediate device you propose.
>> Sorry for being somewhat clueless.
> 
> That was my impression too at first. But it seems like all the IOMMU
> subsystem does is map individual pages. So you pass it a physical address of
> a page along with the IO virtual address to map it to. That's at least the
> way it is handled for Tegra 2 GART (and from a quick look also by the Tegra 3
> SMMU). For the SMMU the situation may be different because it may not have a
> fixed aperture that needs to contain the IO virtual addresses, though I must
> admit I haven't looked at Tegra 3 in enough detail to judge this.
> 
> So this intermediate device would be purely an allocator for the GART
> aperture and handle the actual mapping via the IOMMU. This would probably be
> really simple and is in fact now done in the DRM driver. The nice thing if
> this would be a separate device is that it would be easy to make it a child
> of the IOMMU and wouldn't be as counter-intuitive as making the DRM device
> the IOMMU's child.

OK, I see.

I'll defer to Hiroshi here; he's far more familiar with all this than I am.

My only comment is that I'd be surprised to see any kind of memory
allocator represented as a driver (as opposed to a utility module) since
it's not really representing an actual hardware block.

Re: Reparenting a platform device

[Grant Likely]

On Thu, 5 Apr 2012 10:42:58 +0200, Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org> wrote:
> Hi,
> 
> I have a device tree where I have a GART device and a DRM device which uses
> the GART. The GART is implemented by an IOMMU driver (tegra-gart) and
> requires the user device to be a child of the GART device (it explicitly
> checks for this when the user device is attached).
> 
> I've tried two alternatives to achieve this: create the GART device in the
> user driver's .probe() function and explicitly set the DRM device's parent
> to the resulting platform device like so:
> 
> 	gart = platform_device_alloc(...);
> 	...
> 	pdev->dev.parent = &gart->dev;

Yeah, don't *ever* try to do this.  The device hierarchy is a complex
data structure which must never be directly manipulated.

> 
> The alternative is to use the device tree to look up the GART device node and
> resolve it to the corresponding struct device:
> 
> 	gart_node = of_parse_phandle(drm->dev->of_node, "gart-parent", 0);
> 	gart = bus_find_device(drm->dev->bus, NULL, gart_node, match_of_node);
> 
> Where match_of_node matches each struct device's .of_node field to the
> gart_node.
> 
> Both of these variants seem to work, and the DRM device can be properly
> attached to the GART device. However, after the DRM driver's .probe() exits,
> I get the following error:

I don't understand what you're trying to describe here as the 2nd
option.

Regardless, reparenting should not ben the solution at all.  What does
the device tree that you envision look like for this?  What devices
are created, and what drivers bind to them?

g.

Re: Reparenting a platform device

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 2803 bytes --]

* Grant Likely wrote:
> On Thu, 5 Apr 2012 10:42:58 +0200, Thierry Reding <thierry.reding@avionic-design.de> wrote:
> > Hi,
> > 
> > I have a device tree where I have a GART device and a DRM device which uses
> > the GART. The GART is implemented by an IOMMU driver (tegra-gart) and
> > requires the user device to be a child of the GART device (it explicitly
> > checks for this when the user device is attached).
> > 
> > I've tried two alternatives to achieve this: create the GART device in the
> > user driver's .probe() function and explicitly set the DRM device's parent
> > to the resulting platform device like so:
> > 
> > 	gart = platform_device_alloc(...);
> > 	...
> > 	pdev->dev.parent = &gart->dev;
> 
> Yeah, don't *ever* try to do this.  The device hierarchy is a complex
> data structure which must never be directly manipulated.
> 
> > 
> > The alternative is to use the device tree to look up the GART device node and
> > resolve it to the corresponding struct device:
> > 
> > 	gart_node = of_parse_phandle(drm->dev->of_node, "gart-parent", 0);
> > 	gart = bus_find_device(drm->dev->bus, NULL, gart_node, match_of_node);
> > 
> > Where match_of_node matches each struct device's .of_node field to the
> > gart_node.
> > 
> > Both of these variants seem to work, and the DRM device can be properly
> > attached to the GART device. However, after the DRM driver's .probe() exits,
> > I get the following error:
> 
> I don't understand what you're trying to describe here as the 2nd
> option.
> 
> Regardless, reparenting should not ben the solution at all.  What does
> the device tree that you envision look like for this?  What devices
> are created, and what drivers bind to them?

The reason why I need to reparent at all is because the IOMMU driver requires
the user to be a child of the IOMMU device. If you look at the driver in
drivers/iommu/tegra-gart.c you'll see that it references dev->parent in
several places, most notably in the gart_iommu_attach_dev() function. So
there's really only two options that I can see: 1) create a virtual device
that is a child of the GART and is in charge of the actual allocations from
the GART and have the DRM driver use that interface or 2) change the GART
driver's behaviour in a way that the parent/child relationship is no longer a
requirement.

1) has the advantage of providing a central allocation manager for the GART
and will allow to register multiple clients with the GART. 2) does not have
that advantage.

Another alternative may be to allow only a single device to attach to the
GART that doesn't have to be a child of the GART. That way the DRM could take
care of GART aperture allocations, which seems to be the most logical place
to do that anyway.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 190 bytes --]

_______________________________________________
iommu mailing list
iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: Reparenting a platform device

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 2063 bytes --]

* Stephen Warren wrote:
> On 04/06/2012 01:20 AM, Thierry Reding wrote:
> > * Stephen Warren wrote:
> >> I must admit I'm not at all familiar with the IOMMU APIs, but isn't the
> >> IOMMU driver/subsystem itself what is managing all the allocations and
> >> handing them out to clients? And client drivers do things like asking
> >> for N pages of memory mapped into their aperture? If that is true, I'm
> >> not sure what the purpose is of the intermediate device you propose.
> >> Sorry for being somewhat clueless.
> > 
> > That was my impression too at first. But it seems like all the IOMMU
> > subsystem does is map individual pages. So you pass it a physical address of
> > a page along with the IO virtual address to map it to. That's at least the
> > way it is handled for Tegra 2 GART (and from a quick look also by the Tegra 3
> > SMMU). For the SMMU the situation may be different because it may not have a
> > fixed aperture that needs to contain the IO virtual addresses, though I must
> > admit I haven't looked at Tegra 3 in enough detail to judge this.
> > 
> > So this intermediate device would be purely an allocator for the GART
> > aperture and handle the actual mapping via the IOMMU. This would probably be
> > really simple and is in fact now done in the DRM driver. The nice thing if
> > this would be a separate device is that it would be easy to make it a child
> > of the IOMMU and wouldn't be as counter-intuitive as making the DRM device
> > the IOMMU's child.
> 
> OK, I see.
> 
> I'll defer to Hiroshi here; he's far more familiar with all this than I am.
> 
> My only comment is that I'd be surprised to see any kind of memory
> allocator represented as a driver (as opposed to a utility module) since
> it's not really representing an actual hardware block.

I agree. In that case I don't see any other solution than to remove the
requirement of the parent/child relationship from the GART driver. It seems
like the SMMU driver doesn't have such a requirement so it should be fine.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 190 bytes --]

_______________________________________________
iommu mailing list
iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: Reparenting a platform device

[Hiroshi Doyu]

From: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
Subject: Re: Reparenting a platform device
Date: Sun, 8 Apr 2012 16:21:41 +0200
Message-ID: <20120408142141.GA25303-RM9K5IK7kjIyiCvfTdI0JKcOhU4Rzj621B7CTYaBSLdn68oJJulU0Q@public.gmane.org>

> * PGP Signed by an unknown key
> 
> * Stephen Warren wrote:
> > On 04/06/2012 01:20 AM, Thierry Reding wrote:
> > > * Stephen Warren wrote:
> > >> I must admit I'm not at all familiar with the IOMMU APIs, but isn't the
> > >> IOMMU driver/subsystem itself what is managing all the allocations and
> > >> handing them out to clients? And client drivers do things like asking
> > >> for N pages of memory mapped into their aperture? If that is true, I'm
> > >> not sure what the purpose is of the intermediate device you propose.
> > >> Sorry for being somewhat clueless.
> > > 
> > > That was my impression too at first. But it seems like all the IOMMU
> > > subsystem does is map individual pages. So you pass it a physical address of
> > > a page along with the IO virtual address to map it to. That's at least the
> > > way it is handled for Tegra 2 GART (and from a quick look also by the Tegra 3
> > > SMMU). For the SMMU the situation may be different because it may not have a
> > > fixed aperture that needs to contain the IO virtual addresses, though I must
> > > admit I haven't looked at Tegra 3 in enough detail to judge this.
> > > 
> > > So this intermediate device would be purely an allocator for the GART
> > > aperture and handle the actual mapping via the IOMMU. This would probably be
> > > really simple and is in fact now done in the DRM driver. The nice thing if
> > > this would be a separate device is that it would be easy to make it a child
> > > of the IOMMU and wouldn't be as counter-intuitive as making the DRM device
> > > the IOMMU's child.
> > 
> > OK, I see.
> > 
> > I'll defer to Hiroshi here; he's far more familiar with all this than I am.
> > 
> > My only comment is that I'd be surprised to see any kind of memory
> > allocator represented as a driver (as opposed to a utility module) since
> > it's not really representing an actual hardware block.
> 
> I agree. In that case I don't see any other solution than to remove the
> requirement of the parent/child relationship from the GART driver. It seems
> like the SMMU driver doesn't have such a requirement so it should be fine.

Right. I missed one patch to send out. Probably we need the following
patch for the above.

>From f7a56c85a5dfe41d23746d28b2ecbfb66ef034b5 Mon Sep 17 00:00:00 2001
From: Vandana Salve <vsalve-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Date: Wed, 14 Mar 2012 18:17:53 +0530
Subject: [PATCH 1/1] iommu: tegra/gart: use correct gart_device

Pass the correct gart device pointer.

Reviewed-by: Vandana Salve <vsalve-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Tested-by: Vandana Salve <vsalve-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Reviewed-by: Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Reviewed-by: Bharat Nihalani <bnihalani-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Hiroshi DOYU <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/iommu/tegra-gart.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/iommu/tegra-gart.c b/drivers/iommu/tegra-gart.c
index e8d50a2..c33557c 100644
--- a/drivers/iommu/tegra-gart.c
+++ b/drivers/iommu/tegra-gart.c
@@ -158,7 +158,7 @@ static int gart_iommu_attach_dev(struct iommu_domain *domain,
 	struct gart_client *client, *c;
 	int err = 0;
 
-	gart = dev_get_drvdata(dev->parent);
+	gart = gart_handle;
 	if (!gart)
 		return -EINVAL;
 	domain->priv = gart;
-- 
1.7.5.4

Re: Reparenting a platform device

[Grant Likely]

On Sat, 7 Apr 2012 13:35:10 +0200, Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org> wrote:
> * Grant Likely wrote:
> > On Thu, 5 Apr 2012 10:42:58 +0200, Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org> wrote:
> > > Hi,
> > > 
> > > I have a device tree where I have a GART device and a DRM device which uses
> > > the GART. The GART is implemented by an IOMMU driver (tegra-gart) and
> > > requires the user device to be a child of the GART device (it explicitly
> > > checks for this when the user device is attached).
> > > 
> > > I've tried two alternatives to achieve this: create the GART device in the
> > > user driver's .probe() function and explicitly set the DRM device's parent
> > > to the resulting platform device like so:
> > > 
> > > 	gart = platform_device_alloc(...);
> > > 	...
> > > 	pdev->dev.parent = &gart->dev;
> > 
> > Yeah, don't *ever* try to do this.  The device hierarchy is a complex
> > data structure which must never be directly manipulated.
> > 
> > > 
> > > The alternative is to use the device tree to look up the GART device node and
> > > resolve it to the corresponding struct device:
> > > 
> > > 	gart_node = of_parse_phandle(drm->dev->of_node, "gart-parent", 0);
> > > 	gart = bus_find_device(drm->dev->bus, NULL, gart_node, match_of_node);
> > > 
> > > Where match_of_node matches each struct device's .of_node field to the
> > > gart_node.
> > > 
> > > Both of these variants seem to work, and the DRM device can be properly
> > > attached to the GART device. However, after the DRM driver's .probe() exits,
> > > I get the following error:
> > 
> > I don't understand what you're trying to describe here as the 2nd
> > option.
> > 
> > Regardless, reparenting should not ben the solution at all.  What does
> > the device tree that you envision look like for this?  What devices
> > are created, and what drivers bind to them?
> 
> The reason why I need to reparent at all is because the IOMMU driver requires
> the user to be a child of the IOMMU device. If you look at the driver in
> drivers/iommu/tegra-gart.c you'll see that it references dev->parent in
> several places, most notably in the gart_iommu_attach_dev() function. So
> there's really only two options that I can see: 1) create a virtual device
> that is a child of the GART and is in charge of the actual allocations from
> the GART and have the DRM driver use that interface or 2) change the GART
> driver's behaviour in a way that the parent/child relationship is no longer a
> requirement.

Either is fine by me.

> 1) has the advantage of providing a central allocation manager for the GART
> and will allow to register multiple clients with the GART. 2) does not have
> that advantage.
> 
> Another alternative may be to allow only a single device to attach to the
> GART that doesn't have to be a child of the GART. That way the DRM could take
> care of GART aperture allocations, which seems to be the most logical place
> to do that anyway.

That also works.  As long as nothing messes about with odd reparenting
then I'm happy.

g.