Message-ID: <CABXOdTfB4M8AcCOVERpQwddr_N09gpKF67FxRO32S4M9JUaYEQ@mail.gmail.com>
Date:   Tue, 21 Dec 2021 12:35:47 -0800
From:   Guenter Roeck <groeck@...gle.com>
To:     Paul Menzel <pmenzel@...gen.mpg.de>
Cc:     Dmitry Torokhov <dtor@...omium.org>, Wolfram Sang <wsa@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Mika Westerberg <mika.westerberg@...ux.intel.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Tim Wawrzynczak <twawrzynczak@...omium.org>,
        coreboot@...eboot.org, Matt DeVillier <matt.devillier@...il.com>,
        Felix Singer <felixsinger@...teo.net>,
        Benson Leung <bleung@...omium.org>,
        Justin TerAvest <teravest@...omium.org>,
        Guenter Roeck <groeck@...omium.org>, linux-i2c@...r.kernel.org,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE" 
        <devicetree@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        linux-acpi@...r.kernel.org, Sangwon Jee <jeesw@...fas.com>
Subject: Re: [PATCH] CHROMIUM: i2c: Add device property for probing

On Tue, Dec 21, 2021 at 11:42 AM Paul Menzel <pmenzel@...gen.mpg.de> wrote:
>
> Dear Guenter, dear Dmitry,
>
>
> Am 21.12.21 um 17:47 schrieb Guenter Roeck:
> > On Mon, Dec 20, 2021 at 1:49 PM Dmitry Torokhov <dtor@...omium.org> wrote:
>
> >> On Mon, Dec 20, 2021 at 1:07 PM Paul Menzel <pmenzel@...gen.mpg.de> wrote:
> >>>
> >>> From: Furquan Shaikh <furquan@...gle.com>
>
> >>> Google Chromebooks are often built with devices sourced from different
> >>> vendors. These need to be probed. To deal with this, the firmware – in
> >>> this case coreboot – tags such optional devices accordingly – I think
> >>> this is commit fbf2c79b (drivers/i2c/generic: Add config for marking
> >>> device as probed) – and Chromium OS’ Linux kernel has the patch at hand
> >>> applied to act accordingly. Right after the merge, Dmitry created a
> >>> revert, which was actively discussed for two days but wasn’t applied.
> >>> That means, millions of devices shipped with such a firmware and Linux
> >>> kernel. To support these devices with upstream Linux kernel, is there an
> >>> alternative to applying the patch to the Linux kernel, and to support
> >>> the shipped devices?
> >>
> >> *sigh* I should have pushed harder, but I see it managed to
> >> proliferate even into our newer kernels. Not having this patch should
> >> not cause any problems, it can only hurt, because the i2c core has no
> >> idea how to power up and reset the device properly. The only downside
> >> of not having this patch is that we may have devices in sysfs that are
> >> not connected to actual hardware. They do now cause any problems and
> >> is how we have been shipping ARM-based devices where we also dual- and
> >> triple-source components. However if we were to have a device that
> >> switches between several addresses (let's say device in bootloader
> >> mode uses 0x10 address and in normal mode 0x20) this "probing" may
> >> result in device not being detected at all.
>
> On google/sarien, the (upstream) Linux kernel sometimes detects the
> Melfas touchscreen and sometimes not, but in never works. When it’s
> detected, the errors below are still shown.
>
> ```
> $ grep i2c voidlinux-linux-5.13.19-messages.txt
> [    9.392598] i2c i2c-7: 2/2 memory slots populated (from DMI)
> [    9.393108] i2c i2c-7: Successfully instantiated SPD at 0x50
> [    9.622151] input: MELFAS MIP4 Touchscreen as
> /devices/pci0000:00/0000:00:15.0/i2c_designware.0/i2c-8/i2c-MLFS0000:00/input/input6
> [    9.657964] cr50_i2c i2c-GOOG0005:00: cr50 TPM 2.0 (i2c 0x50 irq 114
> id 0x28)
> [    9.662309] elan_i2c i2c-ELAN0000:00: supply vcc not found, using
> dummy regulator
> [    9.773244] elan_i2c i2c-ELAN0000:00: Elan Touchpad: Module ID:
> 0x00d6, Firmware: 0x0005, Sample: 0x0009, IAP: 0x0001
> [    9.773349] input: Elan Touchpad as
> /devices/pci0000:00/0000:00:15.1/i2c_designware.1/i2c-9/i2c-ELAN0000:00/input/input7
> [   10.820307] i2c_designware i2c_designware.0: controller timed out
> [   10.820359] mip4_ts i2c-MLFS0000:00: mip4_i2c_xfer - i2c_transfer
> failed: -110 (-110)
> [   11.844523] i2c_designware i2c_designware.0: controller timed out
> [   11.844635] mip4_ts i2c-MLFS0000:00: mip4_i2c_xfer - i2c_transfer
> failed: -110 (-110)
> [   12.868376] i2c_designware i2c_designware.0: controller timed out
> [   12.868488] mip4_ts i2c-MLFS0000:00: mip4_i2c_xfer - i2c_transfer
> failed: -110 (-110)
> [   12.868570] mip4_ts i2c-MLFS0000:00: Failed to read packet info: -110
> ```
>
> Is that related to the probing stuff?
>

Difficult to say without further testing. I can see two possible
problems: The device may sometimes not be seen because it is powered
off, and/or interrupt handling may not work properly.  You could apply
the patch (commit 11cd1bd03f75 in chromeos-5.15) and see if it
improves the situation. I would also suggest applying commit
b4b55381e5cf ("CHROMIUM: Input: elants_i2c: Default to low level
interrupt for Chromebooks") from chromeos-4.19.

Guenter

> >> If we wanted to do this correctly, coreboot would have to implement
> >> full power and reset control and also add drivers for I2C controllers
> >> to be able to communicate with peripherals, and then adjust _STA
> >> methods to report "not present" when the device is indeed absent. And
> >> note that even in this case we would have issues with "morphing
> >> devices", so coreboot would also need to know how to reset device out
> >> of bootloader mode, and maybe flash firmware so device can work in
> >> normal mode.
>
> What do you mean by “bootloader mode”? coreboot also cannot flash
> anything. That’s up to the payload, and even there support for flashing
> is rare.
>
> Duncan wrote something about the ACPI _STA method idea, that ASL(?) and
> I2C do not go well together.
>
> >> However coreboot does (or did?) not want to add code to handle i2c
> >> controllers, and would like to push this knowledge to the kernel. And
> >> the kernel does know how to handle peripherals properly, but that
> >> knowledge lies in individual drivers, not i2c core.
>
> Excuse my ignorance, can you give an example driver? Does the Melfas
> touchscreen driver (`drivers/input/touchscreen/melfas_mip4.c`) support it?
>
> >> We should remove "linux,probed" from coreboot and not propagate to
> >> newer Chrome OS kernels, and keep it away from upstream.
> >
> > Revert from chromeos-5.15 is at
> > https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/3350347.
> > Everyone please feel free to comment there.
>
> Guenther, thank you for your quick response. Note, that neither Furquan,
> nor Aaron, nor Duncan work at Google anymore, so won’t comment.
> Hopefully, others from the Chromium OS/coreboot folks can chime in.
>
>
> Kind regards,
>
> Paul

Powered by blists - more mailing lists