Message-ID: <CAL_JsqKTMjj0x6Zq3t5GsWvAC1qN10r333v9uGXkJq_DjioRmA@mail.gmail.com>
Date:   Thu, 27 Oct 2016 12:30:09 -0500
From:   Rob Herring <robh@...nel.org>
To:     Hans de Goede <hdegoede@...hat.com>
Cc:     Antoine Tenart <antoine.tenart@...e-electrons.com>,
        Maxime Ripard <maxime.ripard@...e-electrons.com>,
        Pantelis Antoniou <pantelis.antoniou@...sulko.com>,
        Mark Rutland <mark.rutland@....com>,
        Stephen Boyd <sboyd@...eaurora.org>,
        Thomas Petazzoni <thomas.petazzoni@...e-electrons.com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        Frank Rowand <frowand.list@...il.com>,
        Dmitry Shmidt <dimitrysh@...gle.com>
Subject: Re: [RFC PATCH 0/5] Add an overlay manager to handle board capes

On Thu, Oct 27, 2016 at 10:13 AM, Hans de Goede <hdegoede@...hat.com> wrote:
> Hi,
>
> On 27-10-16 15:41, Rob Herring wrote:
>>
>> Please Cc the maintainers of drivers/of/.
>>
>> + Frank R, Hans, Dmitry S
>>
>> On Wed, Oct 26, 2016 at 9:57 AM, Antoine Tenart
>> <antoine.tenart@...e-electrons.com> wrote:
>>>
>>> Hi all,
>>>
>>> Many boards now come with dips and compatible capes; among others the
>>> C.H.I.P, or Beaglebones. All these boards have a kernel implementing an
>>> out-of-tree "cape manager" which is used to detected capes, retrieve
>>> their description and apply a corresponding overlay. This series is an
>>> attempt to start a discussion, with an implementation of such a manager
>>> which is somehow generic (i.e. formats or cape detectors can be added).
>>> Other use cases could make use of this manager to dynamically load dt
>>> overlays based on some input / hw presence.
>>
>>
>> I'd like to see an input source be the kernel command line and/or a DT
>> chosen property. Another overlay manager was proposed not to long
>> ago[1] as well. There's also the Allwinner tablet use case from Hans
>> where i2c devices are probed and detected. That's not using overlays
>> currently, but maybe could.
>
>
> Actually I'm currently thinking in a different direction, which I
> think will be good for the boards where some ICs are frequently
> replaced by 2nd (and 3th and 4th) sources, rather then that we're
> dealing with an extension connector with capes / daughter boards.
>
> Although there is some overlap I'm starting to think that we need to
> treat these 2 cases differently. Let me quickly copy and paste
> the basic idea I've for the 2nd source touchscreen / accelerometer
> chip case:
>
> """
> The kernel actually already has a detect() method in struct i2c_driver,
> we could use that (we would need to implement it in drivers which do not
> have it yet). Note on second thought it seems it may be better to use
> probe() for this, see below.
>
> Then we could have something like this in dt:
>
> &i2c0 {
>     touchscreen1: gsl1680@40 {
>         reg = <0x40>;
>         compatible = "silead,gsl1680";
>         enable-gpios = <&pio 7 1 GPIO_ACTIVE_HIGH>; /* PH1 */
>         status = "disabled";
>     };
>
>     touchscreen2: ektf2127@15 {
>         reg = <0x15>;

Do you ever have different devices with the same address? That would
be somewhat problematic as really these should be
"touchscreen@<addr>".

>         compatible = "elan,ektf2127";
>         enable-gpios = <&pio 7 1 GPIO_ACTIVE_HIGH>; /* PH1 */
>         status = "disabled";
>     };
>
>     i2c-probe-stop-at-first-match-0 = <&touchscreen1>, <&touchscreen2>;
>     i2c-probe-stop-at-first-match-1 = <&accelerometer1>, <&accelerometer2>;
> }
>
> Which would make the i2c subsys call detect (*) on each device, until
> a device is found. Likewise we could have a "i2c-probe-all" property
> which also walks a list of phandles but does not stop on the first
> match.
>
> ...
>
> *) Yes this sounds Linux specific, but it really is just "execute
> to-be-probed
> device compatible specific detection method"
> """

Yeah, not a fan of these properties at first glance. Why can't you
just fail probe on the non-existent devices?


> This does not 100% solve all q8 issues (see the "Add Allwinner Q8 tablets
> hardware manager" thread), but does solve quite a bit of the use-case
> and this matches what many vendor os-images (typically android) are
> actually doing for these kind of boards.

BTW, I've been meaning to ask you if you are looking at the Android
side of things as well?

> As for the bits this does not solve, those are mostly board specific details
> which cannot be probed at all, and on x86 are typically solved in the device
> driver by doing a dmi check to identify the board and then apply a board
> specific workaround in the driver.
>
> I've come to believe that we should similarly delegate dealing this to
> device
> drivers in the devicetree case. Note that dt should still of course fully
> describe the hardware for normal hardware, the driver would just need to
> care
> about weird board quirks in certain exceptions.

Which is fine IMO, though I do think we should look at those cases
carefully to ensure they stay the exception.

> A more interesting problem here is that dt does not have something like
> DMI, there is the machine compatible, but that typically does not contain
> board revision info (where as DMI often does). I believe that this is
> actually something which should be fixed at the bootloader level
> have it prepend a new machine compatible which contains revision info.
>
> Hmm, if we make the bootloader prepend a new machine compatible which
> contains
> revision info, we could then trigger quirks on this and in some cases avoid
> the need for dealing with board quirks in the driver ...

That would work. Board and chip versions both need better handling in
kernel IMO.

QCom has a whole scheme around version numbering in compatible
strings. (Unfortunately, bootloaders only support their previous way
of doing things.)

> Note this is all very specific to dealing with board (revision) variants,
> for add-ons having the bootloader add info to the machine compatible does
> not seem the right solution.

Agreed.

Rob

Powered by blists - more mailing lists