Message-ID: <CAGXv+5EGq=OAymON-wj0OzQjK1VxRzdvgJgbHxaYPKP_pmTN2A@mail.gmail.com>
Date: Tue, 15 Oct 2024 13:22:13 +0800
From: Chen-Yu Tsai <wenst@...omium.org>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc: Rob Herring <robh@...nel.org>, Saravana Kannan <saravanak@...gle.com>, 
	Matthias Brugger <matthias.bgg@...il.com>, 
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>, Wolfram Sang <wsa@...nel.org>, 
	Benson Leung <bleung@...omium.org>, Tzung-Bi Shih <tzungbi@...nel.org>, 
	chrome-platform@...ts.linux.dev, devicetree@...r.kernel.org, 
	linux-arm-kernel@...ts.infradead.org, linux-mediatek@...ts.infradead.org, 
	linux-kernel@...r.kernel.org, Douglas Anderson <dianders@...omium.org>, 
	Johan Hovold <johan@...nel.org>, Jiri Kosina <jikos@...nel.org>, linux-i2c@...r.kernel.org
Subject: Re: [PATCH v8 4/8] i2c: Introduce OF component probe function

On Mon, Oct 14, 2024 at 7:16 PM Andy Shevchenko
<andriy.shevchenko@...ux.intel.com> wrote:
>
> On Mon, Oct 14, 2024 at 11:53:47AM +0800, Chen-Yu Tsai wrote:
> > On Thu, Oct 10, 2024 at 11:16 PM Andy Shevchenko
> > <andriy.shevchenko@...ux.intel.com> wrote:
> > > On Tue, Oct 08, 2024 at 03:34:23PM +0800, Chen-Yu Tsai wrote:
>
> ...
>
> > > Fresh reading of the commit message make me think why the firmware or
> > > bootloader on such a device can't form a dynamic OF (overlay?) to fulfill
> > > the need?
> >
> > The firmware / bootloader on existing devices are practically not upgradable.
> > On the other hand, the kernel is very easy to upgrade or swap out.
> >
> > For said shipped devices, there is also nothing to key the detection
> > off of besides actually powering things up and doing I2C transfers,
> > which takes time that the firmware has little to spare. We (ChromeOS)
> > require that the bootloader jump into the kernel within 1 second of
> > power on. That includes DRAM calibration, whatever essential hardware
> > initialization, and loading and uncompressing the kernel. Anything
> > non-essential that can be done in the kernel is going to get deferred
> > to the kernel.
> >
> > Also, due to project timelines oftentimes the devices are shipped with a
> > downstream kernel with downstream device trees. We don't want to tie the
> > firmware too tightly to the device tree in case the downstream stuff gets
> > reworked when upstreamed.
>
> Okay, I was always under impression that DT has at least one nice feature in
> comparison with ACPI that it can be replaced / updated in much quicker /
> independent manner. What you are telling seems like the same issue that

That depends on which camp one is in. Some folks advocate for shipping the
DT with the firmware. In that case you are limited by how easy the firmware
is to upgrade. Or they would leave an option for the firmware to load a
newer DT from disk.

The other camp is shipping the DT with the kernel image, which ChromeOS and
some Linux distros do.  Then the DT is as easy to upgrade as the kernel.
Opponents would argue that the DT is a hardware description and should
be separate from the kernel. However as with all hardware bringup
development, one can't really describe something that hasn't been
developed and gone through review.

However here I am specifically talking about the firmware _code_ being tied
to the DT. To implement the probing feature in firmware, one needs to either
add a lot more code about what devices the system might have, or implement
the equivalent of this series in firmware, or something in between. This
is a lot of code that depends on the structure of the DT it was developed
against, which most likely was downstream at that point.

Say someone screwed up some DT structure downstream at the time of release,
such as the node name or address prefix, and that was fixed upstream. The
new upstream DT no longer has the structure the firmware is expecting, and
the firmware might not be able to handle it anymore, resulting in some
peripheral no longer getting probed or enabled. And you don't have a way
to upgrade the firmware to fix this.

> ACPI-based platforms have. However, there they usually put all possible devices
> into DSDT and firmware enables them via run-time (ACPI) variables. Are you
> trying to implement something similar here?

Yes, that sounds similar. However for us the DT is tied to the kernel, not
the firmware, i.e. not provided by the firmware. There's also no UEFI in
our case.

> ...
>
> > > Another question is that we have the autoprobing mechanism for I2C for ages,
> > > why that one can't be (re-)used / extended to cover these cases?
> >
> > I haven't looked into it very much, but a quick read of
> > Documentation/i2c/instantiating-devices.rst suggests that it's solving
> > a different problem?
> >
> > In our case, we know that it is just one of a handful of possible
> > devices that we already described in the device tree. We don't need
> > to probe the full address range nor the full range of drivers. We
> > already have a hacky workaround in place, but that mangles the
> > device tree in a way that doesn't really match the hardware.
> >
> > The components that we are handling don't seem to have any hardware
> > ID register, nor do their drivers implement the .detect() callback.
> > There's also power sequencing (regulator and GPIO lines) and interrupt
> > lines from the device tree that need to be handled, something that is
> > missing in the autoprobe path.
> >
> > Based on the above I don't think the existing autoprobe is a good fit.
> > Trying to shoehorn it in is likely going to be a mess.
> >
> > Doug's original cover letter describes the problem in more detail,
> > including why we think this should be done in the kernel, not the
> > firmware:
> > https://lore.kernel.org/all/20230921102420.RFC.1.I9dddd99ccdca175e3ceb1b9fa1827df0928c5101@changeid/
>
> Perhaps it needs to be summarised to cover at least this question along with
> the above?

For the first question, it boils down to we think that the firmware should
be simple and do as little as possible. It also should not be tied to a
specific DT, so editing or fixing up the DT in firmware is something we
avoid. The firmware can do overlays if they are provided, however in this
particular case, the identifiers used by the bootloader don't cover the
variations of the I2C-based second source components.

For why autoprobing isn't a good fit, I believe my answer above covers it.

ChenYu

Powered by blists - more mailing lists