Message-ID: <CAD=FV=UjVAgT-febtj4=UZ2GQp01D-ern2Ff9+ODcHeQBOsdTQ@mail.gmail.com>
Date:   Thu, 28 Sep 2023 16:21:16 -0700
From:   Doug Anderson <dianders@...omium.org>
To:     Rob Herring <robh+dt@...nel.org>
Cc:     Jeff LaBundy <jeff@...undy.com>,
        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

Hi,

On Thu, Sep 28, 2023 at 1:38 PM Rob Herring <robh+dt@...nel.org> wrote:
>
> > About the best you could do would be to add a board-specific driver
> > that understood that it could power up the rails, wait the maximum
> > amount of time that all possible touchscreens might need, and then
> > look for i2c ACKs. I'm still hoping to hear from Rob about how I would
> > get a board-specific driver to load on a DT system so I can
> > investigate / prototype this.
>
> foo_initcall()
> {
>   if (of_machine_is_compatible(...))
>     platform_device_create();
> }
>
> That chunk would have to be built in and if that's part of the driver
> module, autoloading wouldn't work.
>
> We could have a match table of board compatibles and driver names. I'm
> not worried about that list being big, so I'm happy to stick that in
> drivers/of/.

Ah, got it. So your proposal is that we don't add anything to the
device tree but we just probe the hardware manager based on the top
level compatible string. I guess it could work. It wouldn't mesh
amazingly well with the current Chromebook rev/sku stuff in the
top-level compatible without being a bit of a jumble. It could
probably work with some sort of wildcarding (I'd assume glob-style is
enough?). So essentially:

static const struct hw_prober_map[] {
  { .glob = "google,lazor*", .func = lazor_hw_prober_init },
  { .glob = "google,homestar*", .func = homestar_hw_prober_init },
  ...
};

for (i = 0; i < ARRAY_SIZE(hw_prober_map), i++) {
  if (of_machine_is_compatible_glob(hw_prober_map[i].glob)
    hw_prober_map[i].func();
}

If that got to be too big to be built-in then I guess we could always
figure out a way to move stuff to modules and have them auto-loaded.
For now the driver could be in
"drivers/platform/chrome/cros_hwprober.c" or something?


Hmmm, I guess one issue of doing this, though, is that it's going to
be more of a pain to find the DT nodes associated with the resources
we want to enable, right? Since there's no DT note associated with the
"HW prober" driver we don't just have phandles to them. Do we just use
the whole "status" concept and search the whole DT for
"fail-needs-probe" type statuses? Like if we have an elan vs. goodix
touchscreen and we have a realtek vs. synaptic trackpad then we have
statuses like:

status = "fail-needs-probe-touchscreen-elan";
status = "fail-needs-probe-touchscreen-goodix";
status = "fail-needs-probe-trackpad-realtek";
status = "fail-needs-probe-trackpad-synaptic";

...or did you have something else in mind? I'd rather not have the HW
prober driver need to hardcode full DT paths for the devices it's
looking for.


> > > This solution seems a bit confusing to me, and would require more edits
> > > to the dts each time a second source is added. It also means one would
> > > have to write a small platform driver for each group of devices, correct?
> >
> > No matter what we need to add something to the dts each time a second
> > source is added, right?
>
> That was my thought.

OK, cool.


> There is the case of the devices are almost the
> same, so we lie. That's what you are doing for displays IIRC.

Well, we used to do it for display, but it kept biting us. That's why
we created the generic "edp-panel", right? In any case, I'd tend to
keep it as one node per possible device and have HW prober just update
the status.


> > While it's true that we'd end up with some extra drivers, if we do it
> > correctly we don't necessarily need a driver for each group of devices
> > nor even a driver per board. If several boards have very similar
> > probing requirements then, even if they have unique "compatible"
> > strings they could still end up using the same Linux driver.
> >
> > I've actually been talking offline with folks on ChromeOS more about
> > this problem as well. Chen-Yu actually pointed at a patch series (that
> > never landed, I guess) that has some similar ideas [1]. I guess in
> > that case Hans was actually constructing device tree properties
> > manually in the driver. I was thinking more of having all of the
> > options listed in the device tree and then doing something that only
> > causes some of them to probe.
> >
> > If Rob was OK with it, I guess I could have some sort of top-level
> > "hwmanager" node like Hans did and then have phandle links to all the
> > hardware that are managed by it. Then I could just change those to
> > "okay"?
>
> That's really just making the mutex node link the other direction. The
> devices link to the common mutex node vs. the hwmanager node(s) links
> to all the devices. That's really just picking the paint colors.

I don't think the HW Manager concept is the same as the common mutex
at all, so I probably didn't explain it properly.

With the mutex approach the idea is that you simply keep probing each
device one at a time until one succeeds and the mutex keeps them all
from probing at the same time.

With the hardware manager approach you run a bit of board-specific
code that understands which devices are available and can probe for
them in a way that's safer and more efficient. It's safer because it
can take into account the timing requirements of all the possible
devices to ensure that none of their power sequences are violated.
Imagine two touchscreens that each have two power rails and a reset
line. The power sequences are:

TS1:
1. Power up VCC
2. Wait 0 ms (ensure ordering between VCC and VCCIO)
3. Power up VCCIO
4. Wait 100 ms
5. Deassert reset
6. Wait 50 ms.
7. Talk I2C

TS2:
1. Power up VCC
2. Wait 10 ms
3. Power up VCCIO
4. Wait 50 ms.
5. Deassert reset
6. Wait 100 ms
7. Talk I2C

With the "mutex" approach then when we try probing TS1 we'll violate
TS2's specs (not enough delay between VCC and VCCIO). When we try
probing TS2 we'll violate TS1's specs (not enough time between VCCIO
and deasserting reset).

With the a board-specific hardware manager we could know that, for all
possible touchscreens on this board, we can always safely probe for
them with:
1. Power up VCC
2. Wait 10 ms
3. Power up VCCIO
4. Wait 100 ms.
5. Deassert reset
6. Wait 100 ms
7. Talk I2C

Once we've realized which touchscreen is actually present then all
future power ons (like after suspend/resume) can be faster, but this
would be safer for the initial probe.

The above is not only safer but also more efficient. If, in the mutex
case, we probed TS1 first but actually had TS2 then we'd spend 100 +
50 + 10 + 50 + 100 = 310 ms. With the hardware manager we'd probe for
both touchscreens in step 7 and thus we'd only take 10 + 100 + 100 =
210 ms.

The issue with the hardware manager is that we'd then run the normal
driver probe and, unless we could somehow give it a hint, it would
need to re-run through the power sequence again. In your other
response you suggested that the normal driver could just detect that
its regulator was already on and it could skip the regulator power
sequence. I'm not convinced that's a reliable hint. If nothing else
there are some boards the touchscreen regulator is shared and/or
always-on but that doesn't mean someone has properly power sequenced
the "reset" GPIO. I feel like we'd want a more explicit hint, but
that's more something to solve in the Linux driver model and not
something to solve in DT bindings.


> > Ideally, though, this could somehow use device tree "overlays" I
> > guess. That seems like almost a perfect fit. I guess the issue here,
> > though, is that I'd want the overlays bundled together with the
> > original DT and then the board-specific "hardware prober" driver to
> > actually apply the overlays after probing. Does that seem sensible?
>
> BTW, there was an idea long ago from maintainer emeritus Likely to
> append overlays to the base DTB for the kernel to apply.
>
> How would that help you here? Are you going to have an overlay for
> each device that enables it? It's much easier to just call
> of_update_property() to change "status".

Ah, OK. Somehow I assumed that using overlays would be more palatable.
If it's OK to just update the property then that seems fine to me.

...although one other reason I thought to use overlays is I think you
mentioned there was code to make late-arriving devices probe, but I'm
sure that can be handled.

---

So I guess the overall summary is: I'm strongly leaning towards
prototyping the "HW prober" approach. Hopefully that sounds OK.

-Doug

Powered by blists - more mailing lists