Date:	Sat, 17 Aug 2013 12:26:40 +0200
From:	Tomasz Figa <tomasz.figa@...il.com>
To:	Grant Likely <grant.likely@...aro.org>
Cc:	Thierry Reding <thierry.reding@...il.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Rob Herring <rob.herring@...xeda.com>,
	Stephen Warren <swarren@...dotorg.org>,
	Hiroshi Doyu <hdoyu@...dia.com>,
	Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
	Sebastian Hesselbarth <sebastian.hesselbarth@...il.com>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [RFC 2/4] driver core: Allow early registration of devices

On Friday 16 of August 2013 23:20:55 Grant Likely wrote:
> On Fri, Aug 16, 2013 at 10:55 PM, Thierry Reding
> 
> <thierry.reding@...il.com> wrote:
> > On Fri, Aug 16, 2013 at 02:06:37PM -0700, Greg Kroah-Hartman wrote:
> >> On Fri, Aug 16, 2013 at 10:39:21PM +0200, Thierry Reding wrote:
> >> > +static DEFINE_MUTEX(device_early_mutex);
> >> > +static LIST_HEAD(device_early_list);
> >> > +static bool device_is_early = true;
> >> > +
> >> > +/*
> >> > + * Keep a list of early registered devices so that they can be
> >> > fully
> >> > + * registered at a later point in time.
> >> > + */
> >> > +static void device_early_add(struct device *dev)
> >> 
> >> __init?
> > 
> > Yes.
> > 
> >> > +{
> >> > +   mutex_lock(&device_early_mutex);
> >> > +   list_add_tail(&dev->p->early, &device_early_list);
> >> > +   mutex_unlock(&device_early_mutex);
> >> > +}
> >> > +
> >> > +/*
> >> > + * Mark the early device registration phase as completed.
> >> > + */
> >> > +int __init device_early_init(void)
> >> > +{
> >> > +   device_is_early = false;
> >> > +
> >> > +   return 0;
> >> > +}
> >> > +
> >> > +/*
> >> > + * Fixup platform devices instantiated from device tree. The
> >> > problem is + * that since early registration happens before
> >> > interrupt controllers + * have been setup, the OF core code won't
> >> > know how to map interrupts. + */
> >> > +int __init platform_device_early_fixup(struct platform_device
> >> > *pdev)
> >> 
> >> This shouldn't be in this file, because:
> >> > +/*
> >> > + * Fully register early devices.
> >> > + */
> >> > +int __init device_early_done(void)
> >> > +{
> >> > +   struct device_private *private;
> >> > +
> >> > +   list_for_each_entry(private, &device_early_list, early) {
> >> > +           struct device *dev = private->device;
> >> > +           int err;
> >> > +
> >> > +           if (dev->bus == &platform_bus_type) {
> >> 
> >> Why special case the platform bus?  We are trying to move things off
> >> of
> >> the platform bus, don't make it harder to do that :)
> > 
> > I heard about that, but I must have missed the thread where this was
> > discussed. Can you point me to it?
> > 
> >> > +                   struct platform_device *pdev =
> >> > to_platform_device(dev); +
> >> > +                   err = platform_device_early_fixup(pdev);
> >> > +                   if (err < 0)
> >> > +                           dev_err(&pdev->dev,
> >> > +                                   "failed to fixup device %s:
> >> > %d\n",
> >> > +                                   dev_name(&pdev->dev), err);
> >> > +           }
> >> 
> >> You should just have a bus callback that can be made here that, if
> >> present, can be called.  That way any bus can handle this type of
> >> thing, not just the platform one.
> > 
> > You mean something like an .early_fixup() in struct bus_type? That
> > would indeed be much cleaner. As I mentioned this is a very early
> > prototype and this particular hunk exists specifically to fixup the
> > platform devices created by the device tree helpers so that the
> > kernel actually boots to the login prompt.
> > 
> >> Not that I really like the whole idea anyway, but I doubt there's
> >> much I can do about it...
> > 
> > Well, getting feedback from you and others is precisely the reason why
> > I wanted to post this early. There must be a reason why you don't
> > like it, so perhaps you can share your thoughts and we can mould this
> > into something that you'd be more comfortable with.
> > 
> > To be honest I don't particularly like it either. It's very hackish
> > for
> > core code. But on the other hand there are a few device/driver
> > ordering
> > problems that this (or something similar) would help solve. I'm
> > certainly open to discuss alternatives and perhaps there's a much
> > cleaner way to solve the problem.
> 
> And on that note, I think we're all in agreement that it's ugly!
> Looking at it now, I don't think it is the right approach.
> 
> In the big scheme of things, there really aren't a lot of devices that
> will need this functionality. Something I don't have a good handle on
> is the set of devices needed to be created early. Yes, some of the
> clocks and power rails need to be set up, but do all of them? Yes, the
> interrupts need to be set up, but was of_irq_init() a mistake?
> Cascaded interrupt controllers really could be normal devices if there
> isn't anything critical for early boot connected to them (ie. the
> timer). If irq references were changed to be parsed at .probe() time
> instead of device registration time, the deferred probe would have
> everything needed to make sure the interrupt controllers get probed
> before the users.
> 
> Also, for the devices that are created early, is it really appropriate
> to use APIs that require a struct device? I could easily argue that
> anything done at early boot should only be the bare minimum to allow
> the kernel to get to initcalls, and so mucking about with devices is
> inappropriate because it really messes with the assumptions made in
> the design of the driver model.

Please correct me if I'm wrong, but to get to initcalls you need a timer. 
However a timer driver might need interrupts (to tick) and clocks (to get 
the frequency).

At least this is what happens on the platforms I work with. They don't 
need any special early registration method, though. We just special case 
those drivers, as Thierry pointed, so they don't use device based APIs.

However if we consider a more complex setup, let's say that the timer 
driver needs to access some special registers, which are shared with other 
drivers as well (again not a completely exotic case, as I already met this 
when working with timer and PWM drivers for older Samsung platforms and 
had to hack things a bit). One would suggest using regmap here, but it is 
a device based API.

This also connects to a different (and probably less relevant) issue of 
of_clk_init() and all the whole family of_whatever_init() helpers, which 
(all or at least some of them) must be explicitly called from platform 
code of every platform. If we could turn those drivers into normal drivers 
dealing with devices this could go away.

To clarify, I'm not really for or against doing this, I'm just adding some 
points that came to my mind while reading this thread (hopefully something 
useful).

Best regards,
Tomasz

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists