Message-ID: <CAL_JsqKJyRJmwJzB1yew71Ld7BeMMat+rzhX9XtDtiFE8Dbvcw@mail.gmail.com>
Date:   Fri, 22 Sep 2023 14:08:08 -0500
From:   Rob Herring <robh+dt@...nel.org>
To:     Doug Anderson <dianders@...omium.org>
Cc:     Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>, devicetree@...r.kernel.org,
        Benjamin Tissoires <benjamin.tissoires@...hat.com>,
        Chen-Yu Tsai <wenst@...omium.org>, linux-input@...r.kernel.org,
        Jiri Kosina <jikos@...nel.org>,
        Hsin-Yi Wang <hsinyi@...omium.org>, linux-gpio@...r.kernel.org,
        linus.walleij@...aro.org,
        Dmitry Torokhov <dmitry.torokhov@...il.com>,
        Johan Hovold <johan+linaro@...nel.org>,
        andriy.shevchenko@...ux.intel.com, broonie@...nel.org,
        frowand.list@...il.com, gregkh@...uxfoundation.org,
        hdegoede@...hat.com, james.clark@....com, james@...iv.tech,
        keescook@...omium.org, linux-kernel@...r.kernel.org,
        rafael@...nel.org, tglx@...utronix.de
Subject: Re: [RFC PATCH] of: device: Support 2nd sources of probeable but
 undiscoverable devices

On Fri, Sep 22, 2023 at 12:40 PM Doug Anderson <dianders@...omium.org> wrote:
>
> Hi,
>
> On Fri, Sep 22, 2023 at 7:14 AM Rob Herring <robh+dt@...nel.org> wrote:
> >
> > > Let's attempt to do something better. Specifically, we'll allow
> > > tagging nodes in the device tree as mutually exclusive from one
> > > another. This says that only one of the components in this group is
> > > present on any given board. To make it concrete, in my proposal this
> > > looks like:
> > >
> > >   / {
> > >     tp_ex_group: trackpad-exclusion-group {
> > >     };
> >
> > Interesting way to just get a unique identifier. But it could be any
> > phandle not used by another group. So just point all the devices in a
> > group to one of the devices in the group.
>
> Fair enough.
>
>
> > >   &i2c_bus {
> > >     tp1: trackpad@10 {
> > >       ...
> > >       mutual-exclusion-group = <&tp_ex_group>;
> > >     };
> > >     tp2: trackpad@20 {
> > >       ...
> > >       mutual-exclusion-group = <&tp_ex_group>;
> > >     };
> > >     tp3: trackpad@30 {
> > >       ...
> > >       mutual-exclusion-group = <&tp_ex_group>;
> > >     };
> > >   };
> > >
> > > In Linux, we can make things work by simply only probing one of the
> > > devices in the group at a time. We can make a mutex per group and
> > > enforce locking that mutex around probe. If the first device that gets
> > > the mutex fails to probe then it won't try again. If it succeeds then
> > > it will acquire the shared resources and future devices (which we know
> > > can't be present) will fail to get the shared resources. Future
> > > patches could quiet down errors about failing to acquire shared
> > > resources or failing to probe if a device is in a
> > > mutual-exclusion-group.
> >
> > This seems like overkill to me. Do we really need groups and a mutex
> > for each group? Worst case is what? 2-3 groups of 2-3 devices?
> > Instead, what about extending "status" with another value
> > ("fail-needs-probe"? (fail-xxx is a documented value)). Currently, the
> > kernel would just ignore nodes with that status. Then we can process
> > those nodes separately 1-by-1.
>
> My worry here is that this has the potential to impact boot speed in a
> non-trivial way. While trackpads and touchscreens _are_ probable,
> their probe routines are often quite slow. This is even mentioned in
> Dmitry's initial patches adding async probe to the kernel. See commit
> 765230b5f084 ("driver-core: add asynchronous probing support for
> drivers") where he specifically brings up input devices as examples.

Perhaps then this should be solved in userspace where it can learn
which device is actually present and save that information for
subsequent boots.

> It wouldn't be absurd to have a system that has multiple sources for
> both the trackpad and the touchscreen. If we have to probe each of
> these one at a time then it could be slow. It would be quicker to be
> able to probe the trackpads (one at a time) at the same time we're
> probing the touchscreens (one at a time). Using the "fail-needs-probe"
> doesn't provide information needed to know which devices conflict with
> each other.

I would guess most of the time that's pretty evident. They are going
to be on the same bus/link. If unrelated devices are on the same bus,
then that's going to get serialized anyways (if bus accesses are what
make things slow).

We could add information on the class of device. touchscreen and
touchpad aliases or something.

> IMO this is still better than nothing, but it worries me
> to pick the less-expressive solution for the dts which means that the
> information simply isn't there and the OS can't be made better later.
>
> Thinking about this more, I guess even my proposed solution isn't
> ideal for probe speed. Let's imagine that we had:
>
>   &i2c_bus {
>     tp1: trackpad@10 {
>       compatible = "hid-over-i2c";
>       reg = <0x10>;
>       post-power-on-delay-ms = <200>;
>       ...
>       mutual-exclusion-group = <&tp1>;
>     };
>     tp2: trackpad@20 {
>       compatible = "hid-over-i2c";
>       reg = <0x20>;
>       post-power-on-delay-ms = <200>;
>       ...
>       mutual-exclusion-group = <&tp1>;
>     };
>   };
>
> With my solution, we'd power the first device up, wait 200 ms, then
> check to see if anything acks an i2c xfer at address 0x10. If it
> didn't, we'd power down. Then we'd power up the second device
> (presumably the same power rail), wait 200 ms, and check to see if
> anything acks an i2c xfer at 0x20. It would have been better to just
> power up once, wait 200 ms, then check for a device at either 0x10 or
> 0x20.
>
> I guess with more complex touchscreens this could be more important. I
> don't know if we need to try to solve it at this point, but I guess I
> could imagine a case where we truly need to take into account all
> possible devices (maybe taking the maximum of delays?) to ensure we
> don't violate power sequencing requirements for any of them while
> probing.
>
> That would lead me to suggest this:
>
>   &i2c_bus {
>     trackpad-prober {
>       compatible = "mt8173-elm-hana-trackpad-prober";
>
>       tp1: trackpad@10 {
>         compatible = "hid-over-i2c";
>         reg = <0x10>;
>         ...
>         post-power-on-delay-ms = <200>;
>       };
>       tp2: trackpad@20 {
>         compatible = "hid-over-i2c";
>         reg = <0x20>;
>         ...
>         post-power-on-delay-ms = <200>;
>       };
>     };
>   };
>
> ...but I suspect that would be insta-NAKed because it's creating a
> completely virtual device ("mt8173-elm-hana-trackpad-prober") in the
> device tree. I don't know if there's something that's functionally
> similar that would be OK?

Why do you need the intermediate node other than a convenient way to
instantiate a driver? You just need a flag in each node which needs
this special handling. Again, "status" could work well here since it
keeps the normal probe from happening. But I'm not saying you can't
have some board specific code. Sometimes you just need code to deal
with this stuff. Don't try to parameterize everything to DT
properties.

Note that the above only works with "generic" compatibles with
"generic" power sequencing properties (I won't repeat my dislike
again). If only the driver knows how to handle the device, then you
still just have to have the driver probe. If you *only* wanted to
solve the above case, I'd just make "hid-over-i2c" take a 2nd (and
3rd) I2C address in reg and have those as fallbacks.

You could always make the driver probe smarter where if your supply
was already powered on, then don't delay. Then something else could
ensure that the supply is enabled. I'm not sure if regulators have the
same issue as clocks where the clock might be on from the bootloader,
then a failed probe which gets then puts the clock turns it off.

Rob

Powered by blists - more mailing lists