Re: [PATCHv3 01/19] [HACK] of: dev_node has struct device pointer

[Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

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

On Fri, Oct 25, 2013 at 01:10:38AM +0100, Grant Likely wrote:
> On Thu, 24 Oct 2013 11:21:15 +0200, Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> > Hi Grant,
> > 
> > Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote @ Thu, 24 Oct 2013 10:55:31 +0200:
> > ....
> > > > diff --git a/include/linux/of.h b/include/linux/of.h
> > > > index f95aee3..638a88a 100644
> > > > --- a/include/linux/of.h
> > > > +++ b/include/linux/of.h
> > > > @@ -60,6 +60,7 @@ struct device_node {
> > > >  	struct	kref kref;
> > > >  	unsigned long _flags;
> > > >  	void	*data;
> > > > +	struct device *dev;		/* Set only after populated */
> > > 
> > > Is this being used merely to indicate that a device has been processed
> > > by of_platform_device_create()? Or do you intend to dereference this
> > > pointer? I've avoided putting the struct device in to the device_node
> > > structure up to this point simply becuase there aren't any good clues
> > > for what /kind/ of device it actually points to. I worry that bad
> > > assumptions will get made when other subsystems try to use the
> > > same pointer. ie. if one subsystem creates its own device and sets this
> > > pointer, and then of_platform_device_create() comes along behind, sees
> > > that it is already created, and then returns a platform_device pointer
> > > *for something that isn't a struct platform_device*. This is very bad.
> > > 
> > > Instead of using a pointer to the struct device, would a flag be
> > > sufficient for your purposes? Would it be fine to return NULL if the
> > > device has already been created?
> > 
> > Yes, a flag would be enough for this purpose.
> > 
> > This patch is a part of HACK to control device instanciation order. We
> > have an IOMMU device(platform) which needs to be instanciated earlier
> > than other (platform)devices so that IOMMU driver would configure them
> > as IOMMU'able device.
> 
> Ideally the drivers depending on the IOMMU would return -EPROBE_DEFER if
> the IOMMU driver isn't set up so that you don't need to play games with
> probe order. Creating certain platform devices early is a really ugly
> and fragile solution.
> 
> Besides, probe order of device drivers is far more about link order of
> the kernel than it is about when of_platform_device_create() is called.
> Fiddling with the initcall level on the IOMMU driver (while not
> recommended) may very well have the effect you desire.

This is actually "the other problem that I'm aware of that could benefit
from [interrupt resolution at probe time]". My idea was that once we had
a way within the driver core to resolve interrupt references at probe
time it could be used for potentially many other resources as well. Some
of the resources like GPIOs and regulators are obviously not something
that the core can or should be requesting, but mostly resources that you
don't actually need to control after probing (such as interrupts) would
be a good fit because otherwise people would write the same boilerplate
over and over again.

IOMMUs seem to me to be in that same category. As far as I can tell, an
IOMMU driver registers the IOMMU for a given bus, upon which every
device can simply be used (mostly transparently) with that IOMMU. While
I haven't figured out how exactly, I'm pretty sure we can take advantage
of the resolution of resources at probe time within the core to both
keep drivers from having to do anything in particular and at the same
time have code to determine if the IOMMU driver hasn't been probed yet
and return -EPROBE_DEFER appropriately.

I suspect that there will be enough differences between the various
IOMMU implementations that we won't be able to have a unified binding
(especially for how to associate devices with a particular virtual
address space), but perhaps that could be solved with something like an
.of_xlate() that IOMMU drivers implement, much like we've done with most
other subsystems too.

The binding for Tegra's IOMMU currently only uses the HW IDs (or a mask)
to put a device into a given address space, but I think that could be
easily extended to something like:

	memory-clients = <&iommu MASK>;

Or similar. If other information is required, we could encode that into
a multi-cell specifier. Perhaps we could even leave away the phandle
since typically there will only be a single IOMMU in the system?

Does that sound reasonable? Hiroshi is much more familiar with IOMMUs,
so I'd like to get his opinion on the above as well.

Grant, I wonder if it might be more useful if I concentrate on solving
this problem first, before tackling the interrupt reference resolution.
I expect this to be able to use the same core functionality, so it could
be used as a less invasive means of introducing some pieces we can later
reuse. Also this only adds new functionality and doesn't touch anything
that currently works, so there's less (i.e. no) risk of breaking any
existing functionality.

Thierry

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

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