[coreboot] Re: [PATCH] CHROMIUM: i2c: Add device property for probing
On Mon, Dec 20, 2021 at 1:49 PM Dmitry Torokhov wrote: > > Hi Paul, > > On Mon, Dec 20, 2021 at 1:07 PM Paul Menzel wrote: > > > > From: Furquan Shaikh > > > > Dear Linux folks, > > > > > > 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. > > 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. > > 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. > > 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. Guenter ___ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-le...@coreboot.org
[coreboot] Re: [PATCH] CHROMIUM: i2c: Add device property for probing
On Tue, Dec 21, 2021 at 11:42 AM Paul Menzel 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 wrote:
>
> >> On Mon, Dec 20, 2021 at 1:07 PM Paul Menzel wrote:
> >>>
> >>> From: Furquan Shaikh
>
> >>> 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/pci:00/:00:15.0/i2c_designware.0/i2c-8/i2c-MLFS: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-ELAN:00: supply vcc not found, using
> dummy regulator
> [9.773244] elan_i2c i2c-ELAN:00: Elan Touchpad: Module ID:
> 0x00d6, Firmware: 0x0005, Sample: 0x0009, IAP: 0x0001
> [9.773349] input: Elan Touchpad as
> /devices/pci:00/:00:15.1/i2c_designware.1/i2c-9/i2c-ELAN:00/input/input7
> [ 10.820307] i2c_designware i2c_designware.0: controller timed out
> [ 10.820359] mip4_ts i2c-MLFS:00: mip4_i2c_xfer - i2c_transfer
> failed: -110 (-110)
> [ 11.844523] i2c_designware i2c_designware.0: controller timed out
> [ 11.844635] mip4_ts i2c-MLFS:00: mip4_i2c_xfer - i2c_transfer
> failed: -110 (-110)
> [ 12.868376] i2c_designware i2c_designware.0: controller timed out
> [ 12.868488] mip4_ts i2c-MLFS:00: mip4_i2c_xfer - i2c_transfer
> failed: -110 (-110)
> [ 12.868570] mip4_ts i2c-MLFS: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
[coreboot] Re: [PATCH] CHROMIUM: i2c: Add device property for probing
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 wrote: On Mon, Dec 20, 2021 at 1:07 PM Paul Menzel wrote: From: Furquan Shaikh 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/pci:00/:00:15.0/i2c_designware.0/i2c-8/i2c-MLFS: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-ELAN:00: supply vcc not found, using dummy regulator [9.773244] elan_i2c i2c-ELAN:00: Elan Touchpad: Module ID: 0x00d6, Firmware: 0x0005, Sample: 0x0009, IAP: 0x0001 [9.773349] input: Elan Touchpad as /devices/pci:00/:00:15.1/i2c_designware.1/i2c-9/i2c-ELAN:00/input/input7 [ 10.820307] i2c_designware i2c_designware.0: controller timed out [ 10.820359] mip4_ts i2c-MLFS:00: mip4_i2c_xfer - i2c_transfer failed: -110 (-110) [ 11.844523] i2c_designware i2c_designware.0: controller timed out [ 11.844635] mip4_ts i2c-MLFS:00: mip4_i2c_xfer - i2c_transfer failed: -110 (-110) [ 12.868376] i2c_designware i2c_designware.0: controller timed out [ 12.868488] mip4_ts i2c-MLFS:00: mip4_i2c_xfer - i2c_transfer failed: -110 (-110) [ 12.868570] mip4_ts i2c-MLFS:00: Failed to read packet info: -110 ``` Is that related to the probing stuff? 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[0.00] Linux version 5.13.19_1 (voidlinux@voidlinux) (gcc (GCC) 10.2.1 20201203, GNU ld (GNU Binutils) 2.35.1) #1 SMP Sat Sep 18 18:18:26
[coreboot] Re: Question about compressed FIT payloads
On 18.12.2021 18:07, Nico Huber wrote: On 15.12.21 13:24, Krystian Hebel wrote: Hi Nico, On 15.12.2021 12:21, Nico Huber wrote: Hi Krystian, On 14.12.21 13:00, Krystian Hebel wrote: For our work on POWER9 coreboot port we were using Skiboot [1] packed into FIT payload. I might just miss it because I never worked with FIT, but maybe I'm not the only one: Can you elaborate why you chose FIT? Reading through your mail (and without further knowledge) it would just seem like the wrong choice. Good point, I spent so much time in this project that I treat too many things as obvious. Don't hesitate to ask if I omit something important. Skiboot must be supplied with information about hardware. Some of that is generated by code based on current configuration (e.g. number of cores, their IDs, memory amount and associativity), but a lot is always the same for a given platform or even whole architecture (BMC sensors, interrupt controller, register address space, LPC controller). Isn't this all or mostly information that coreboot has already available? It seems much like the situation on x86 with ACPI. We also had static ACPI tables that redundantly stored infor- mation at first. However, static ACPI tables were never treated first-class, sometimes even like an alien, and were prone to rot. Today, we try a good compromise between runtime generation and static tables (which can be extended at runtime). Not everything is defined in coreboot sources, AFAICT only XSCOM, LPC and one out of 3 I2C buses are used, rest (almost 600 lines in dts) is just static data. There is a lot of stuff that coreboot doesn't have to worry about, but Skiboot and OS do. This can be passed either in HDAT structure (Hostboot data, proprietary and mostly undocumented format in supposedly open firmware) or, as we learned after asking on OpenPower-Firmware mailing list, using FDT: https://lists.ozlabs.org/pipermail/openpower-firmware/2021-May/000641.html In our current setup that second, constant part is supplied as FDT in FIT image so it can be added as .dts file, which is easier to read and understand than C code that would be used for creating these nodes. Hmmm, I read above mailinglist thread. But I still miss the link to FIT. coreboot would have a dtb (doesn't matter if it generates it at runtime or a file in CBFS) and a payload (in CBFS or some other flash partition), right? Why do you need FIT? We don't _need_ FIT. We also don't need to implement new way of loading the payload if existing one is working as expected, more or less. Do you suggest removing FIT from coreboot completely? :) FIT code already does the heavy lifting: it parses dtb to modifiable format, appends firmware, compat and memory nodes (although the latter are added in different format than Skiboot expects and we have to modify it - both formats are compatible with spec, but not with each other...), gives opportunity for fixups to DT before repacking it, sets entry point and argument. Doing the same with dtb loaded from CBFS is possible, but it would result in mostly duplicated code. The only pieces of FIT that our code doesn't use are overlays and initramfs. Another option could be to split fit_payload() into two halves, first one would just load FIT from CBFS and choose appropriate config, second would be given kernel, initramfs and dtb as 'struct region' and work from there. That way we could mix various ways of obtaining pieces of the puzzle without duplicating code. Note that this would still leave bugs (normally I would called it "lack of implementation", but documentation says explicitly that this is supported) in the FIT decompression unresolved. -- Krystian Hebel Firmware Engineer https://3mdeb.com | @3mdeb_com ___ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-le...@coreboot.org
[coreboot] Re: [PATCH] CHROMIUM: i2c: Add device property for probing
Hi Paul, On Mon, Dec 20, 2021 at 1:07 PM Paul Menzel wrote: > > From: Furquan Shaikh > > Dear Linux folks, > > > 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. 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. 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. We should remove "linux,probed" from coreboot and not propagate to newer Chrome OS kernels, and keep it away from upstream. Thanks, Dmitry ___ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-le...@coreboot.org
[coreboot] Re: 915G support
Thanks, I'll check them out. пн, 20 дек. 2021 г., 11:35 Kyösti Mälkki : > On Fri, Dec 17, 2021 at 9:40 AM Nico Huber wrote: > > > > Hi, > > > > On 15.12.21 14:25, Sid_key wrote: > > > Hello, is 915G supported? > > > > alas not, and it never was, AFAICT. i945 is supported and i855 was once. > > I think I did a lot of the SerialICE development with 915G back in > 2013(?). in serialice tree you can find some support files for > Commell LV-672 and MSI MS-7133. I believe I had one or both of them > reach payload with coreboot, but I don't remember ever publishing that > work. I can check if I have those repositories somewhere in my > backups. > > Kyösti > ___ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-le...@coreboot.org
