Re: [Ksummit-discuss] [TECH TOPIC] Driver model/resources, ACPI, DT, etc (sigh)

[Darren Vincent Hart <darren@dvhart.com>]

On Tue, 2014-05-13 at 23:51 +0200, Rafael J. Wysocki wrote:
> On Tuesday, May 13, 2014 02:28:05 PM Olof Johansson wrote:
> > On Tue, May 13, 2014 at 2:40 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > > On Tuesday, May 13, 2014 02:14:42 PM Olof Johansson wrote:
> > >> On Tue, May 06, 2014 at 03:35:41PM +0200, Arnd Bergmann wrote:
> > >> > On Tuesday 06 May 2014 14:18:56 Rafael J. Wysocki wrote:
> > >> > > On Monday, May 05, 2014 09:31:04 PM Benjamin Herrenschmidt wrote:
> > >> > > > On Mon, 2014-05-05 at 13:32 +0200, Rafael J. Wysocki wrote:
> > >> > > > > The interpretation - yes, the translation - not necessarily.  By putting
> > >> > > > > the translation part into drivers we'll duplicate a lot of code used for
> > >> > > > > that.  In my opinion there needs to be a layer doing the translation
> > >> > > > > automatically.
> > >> > > >
> > >> > > > I'm afraid I don't understand. How can you do the translation
> > >> > > > automatically if you have fundamentally different representation of
> > >> > > > device-specific concepts ?
> > >> > >
> > >> > > I'm not entirely sure how fundamentally different it is to be honest, but in
> > >> > > any case I'm talking about the situation in which the hardware is the same
> > >> > > and the driver is the same, only the firmware interface is different.  In
> > >> > > that case either the firmware supplies the information the driver needs, or
> > >> > > it doesn't (and as long as the information is supplied, the format doesn't
> > >> > > really matter, because it only is a matter of translation).
> > >> >
> > >> > One example for something that is fundamentally different between an
> > >> > embedded system using DT and any ACPI system is how a PCI(e) host bridge
> > >> > gets treated, we are currently having long discussions about this elsewhere.
> > >> >
> > >> > With ACPI (or a server running OF for that matter), you assume that the
> > >> > PCI host is operational and all buses are fully probed and then you
> > >> > have methods for doing hotplugging, you just look at the device that
> > >> > are already initialized by the firmware.
> > >> >
> > >> > With embedded systems, you (in general, some steps may get skipped
> > >> > in practice) go through a lot more steps:
> > >> > - detect the presence of the host bridge device
> > >> > - enable clocks, voltages, phys, resets, etc.
> > >> > - set up inbound and outbound MMIO and PIO address translation windows
> > >> > - perform a full bus scan and assign bus and device resources
> > >> >
> > >> > We probably woulnd't go through that hassle on ACPI systems. I think
> > >> > we shouldn't attempt any translation here, and keep the code completely
> > >> > separate, with the common parts being handled by the PCI core as we
> > >> > do today.
> > >> >
> > >> > An example about a subsystem where translate complex information
> > >> > from both DT and ACPI into a common format is the dmaengine: We can
> > >> > have references to dma channels in completely incompatible ways
> > >> > but still use the same dma_request_slave_channel() interface for
> > >> > both. The ACPI path currently is lacking a bit (e.g. there is no
> > >> > way to specify the channel name in ACPI, so the code makes
> > >> > assumptions that don't hold true in general), but that's fixable.
> > >> >
> > >> > I assume we will see more of the second kind in the future, and this
> > >> > would be limited to embedded systems on both ACPI and DT, since you
> > >> > shouldn't see dma slave devices on general purpose systems with
> > >> > either.
> > >> >
> > >> > > Bottom line: I'd like to avoid modifying drivers that use the of_ interface
> > >> > > today if possible.  Instead, I'd prefer to modify the firmware layer so that
> > >> > > those drivers can get what they ask for regardless of what firmware interface
> > >> > > the platform is using.
> > >> >
> > >> > Looking at the most trivial example:
> > >> >
> > >> > bool of_property_read_bool(const struct device_node *np,                                                                        const char *propname);
> > >> >
> > >> > We should definitely be able to have an interface like this for
> > >> > ACPI, but you don't have a 'struct device_node' pointer then.
> > >> > We can add a new
> > >> >
> > >> > bool dev_property_read_bool(const struct device *dev,
> > >> >                             const char *propname);
> > >>
> > >> >
> > >> > that handles both, but then you have to change every driver
> > >> > that is already using of_property_read_bool() and that can
> > >> > get used with ACPI.
> > >>
> > >> It's actually not that easy in practice. There's not a one-to-one
> > >> correspondence between device tree nodes and struct devices in the
> > >> kernel. There are many bindings that use subnodes for sections of their
> > >> information, and so on.
> > >>
> > >> > An advantage of the dev_property_read_bool()
> > >> > interface however is that it would also work for platform
> > >> > devices that are created in platform code using
> > >> > platform_device_register_full() or similar if we add a way
> > >> > to define simple properties in C code. That would also simplify
> > >> > a lot of drivers that use both (simple) platform_data and DT
> > >> > today.
> > >>
> > >> I'm not a fan of this "grand unified abstraction layer". Driver writes are
> > >> already complaining that things are too complicated with the move to device
> > >> tree, and we're making things even more abstracted and complicated here. People
> > >> will need to peel back even more layers to figure out what's actually going on.
> > >>
> > >>
> > >> For the truly trivial stuff, there are some quite simple ways to handle this:
> > >>
> > >> 1. Make sure that anything that can be a named IORESOURCE_* is so. Which could
> > >>    mean moving over GPIO and possibly clocks to that, and use those for lookup.
> > >>
> > >> 2. If Rafael's arguments are to be believed, that the translation should be
> > >>    trivial and easy to do, then we might as well reuse the structures and APIs
> > >>    that are already in the kernel. In other words, allocate a struct
> > >>    device_node, populate it with the properties from the ACPI structure, and
> > >>    attach it to the struct device. Then there's no need for any churn in the
> > >>    drivers (until they need ACPI-specific tweaks, which they will all need).
> > >
> > > The problem with that is that we have either device_node or acpi_node.companion
> > > under struct device and they are mutually exclusive.  So this isn't going to fly.
> > > [And it doesn't make sense to duplicate information already available from
> > > acpi_node.companion this way IMHO.]
> > >
> > > Also device references are problematic here, because both interfaces use
> > > different ways to do that (phandles vs object paths in the ACPI namespace).
> > >
> > > So actually, what Arnd proposed seems to be the simplest approach.
> > 
> > Again, that suggestion assumes a 1:1 mapping between device nodes and
> > struct devices, which does not exist.
> 
> I suppose your concern is that there may be device nodes without struct device,
> right?  So drivers may need to access those device nodes directly?
> 
> Can you please point me to code where this is the case?

gpio-keys.c is a good example, and the one I chose for the MinnowBoard
example last year.

This is a really good point. My thinking was that we iterate through the
devices - but I had my ACPI glasses on at the time  and wasn't thinking
about how that iteration would work for DT in with the properties
abstraction.

The ACPI sub-devices do have something in common with the DT sub-nodes,
they are not themselves "compatible" with a specific driver, nor are the
known by the acpi_platform match table.

So in both cases we have effectively a hierarchy of properties. Could we
not support such a concept in the properties interface? The basic
signature might be something more like:

device_get_property(dev, NULL, "foo") // for the top level properties

We could have some kind of a foreach iterator for children, and

device_get_property(dev, child, "baz")

Thoughts?

-- 
Darren Hart
Intel Open Source Technology Center