| Message ID | 20230227140758.1575-4-kaehndan@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | Firmware Support for USB-HID Devices and CP2112 | expand |
On Mon, Feb 27, 2023 at 08:07:58AM -0600, Danny Kaehn wrote: > Bind I2C and GPIO interfaces to subnodes with names > "i2c" and "gpio" if they exist, respectively. This > allows the GPIO and I2C controllers to be described > in firmware as usual. Additionally, support configuring the > I2C bus speed from the clock-frequency device property. A bit shorten indentation... Nevertheless what I realized now is that this change, despite being OF independent by used APIs, still OF-only. Would it be possible to allow indexed access to child nodes as well, so if there are no names, we may still be able to use firmware nodes from the correct children? P.S. The problem with ACPI is that "name" of the child node will be in capital letters as it's in accordance with the specification.
On Mon, Feb 27, 2023 at 5:07 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Mon, Feb 27, 2023 at 08:07:58AM -0600, Danny Kaehn wrote: > > Bind I2C and GPIO interfaces to subnodes with names > > "i2c" and "gpio" if they exist, respectively. This > > allows the GPIO and I2C controllers to be described > > in firmware as usual. Additionally, support configuring the > > I2C bus speed from the clock-frequency device property. > > A bit shorten indentation... > > Nevertheless what I realized now is that this change, despite being OF > independent by used APIs, still OF-only. I assumed this would practically be the case -- not because of the casing reason you gave (wasn't aware of that, thanks for the FYI), but because it doesn't seem that there's any way to describe USB devices connected to a USB port in ACPI, at least as far as I can tell (ACPI is still largely a black box to me). But it seems reasonable that we should try to use the interface in a way so that it could be described using ACPI at some point (assuming that it isn't currently possible). > > Would it be possible to allow indexed access to child nodes as well, so if > there are no names, we may still be able to use firmware nodes from the correct > children? > Sure, you mean to fallback to using child nodes by index rather than by name in the case that that device_get_named_child_node() fails? Would we need to somehow verify that those nodes are the nodes we expect them to be? (a.e. node 0 is actually the i2c-controller, node 1 is actually the gpio-controller). I don't see a reason why not, though I am curious if there is precedence for this strategy, a.e. in other drivers that use named child nodes. In my initial search through the kernel, I don't think I found anything like this -- does that mean those drivers also inherently won't work with ACPI? The only driver I can find which uses device_get_named_child_node and has an acpi_device_id is drivers/platform/x86/intel/chtwc_int33fe.c > P.S. The problem with ACPI is that "name" of the child node will be in capital > letters as it's in accordance with the specification. > Knowing that this is the limitation, some other potential resolutions to potentially consider might be: - Uppercase the names of the child nodes for the DT binding -- it appears that the child node that chtwc_int33fe.c (the driver mentioned earlier) accesses does indeed have an upper-cased name -- though that driver doesn't have an of_device_id (makes sense, x86...). It seems named child nodes are always lowercase in DT bindings -- not sure if that's a rule, or just how it currently happens to be. - Do a case invariant compare on the names (and/or check for both lowercase and uppercase) - Remove the use of child nodes, and combine the i2c and gpio nodes into the cp2112's fwnode. I didn't do this initially because I wanted to avoid namespace collisions between GPIO hogs and i2c child devices, and thought that logically made sense to keep them separate, but that was before knowing this limitation of ACPI. What are your / others' thoughts? Thanks, Danny Kaehn > -- > With Best Regards, > Andy Shevchenko > >
+Cc: Hans (as some DT/ACPI interesting cases are being discussed). On Tue, Feb 28, 2023 at 01:05:54PM -0600, Daniel Kaehn wrote: > On Mon, Feb 27, 2023 at 5:07 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Mon, Feb 27, 2023 at 08:07:58AM -0600, Danny Kaehn wrote: > > > Bind I2C and GPIO interfaces to subnodes with names > > > "i2c" and "gpio" if they exist, respectively. This > > > allows the GPIO and I2C controllers to be described > > > in firmware as usual. Additionally, support configuring the > > > I2C bus speed from the clock-frequency device property. > > > > A bit shorten indentation... > > > > Nevertheless what I realized now is that this change, despite being OF > > independent by used APIs, still OF-only. > > I assumed this would practically be the case -- not because of the casing > reason you gave (wasn't aware of that, thanks for the FYI), but because it > doesn't seem that there's any way to describe USB devices connected to > a USB port in ACPI, at least as far as I can tell (ACPI is still largely a black > box to me). That's not true :-) Microsoft created a schema that is not part of the specification, but let's say a standard de facto. Linux supports that and I even played with it [1] to get connection of the arbitrary device to USB-2-GPIO/I²C/SPI adapter. > But it seems reasonable that we should try to use the interface > in a way so that it could be described using ACPI at some point (assuming > that it isn't currently possible). > > > Would it be possible to allow indexed access to child nodes as well, so if > > there are no names, we may still be able to use firmware nodes from the correct > > children? > > Sure, you mean to fallback to using child nodes by index rather than by name > in the case that that device_get_named_child_node() fails? Would we need to > somehow verify that those nodes are the nodes we expect them to be? (a.e. > node 0 is actually the i2c-controller, node 1 is actually the > gpio-controller). Something like that, but I don't know if we can actually validate this without having a preliminary agreement (hard-coded values) that index 0 is always let's say I²C controller and so on. > I don't see a reason why not, though I am curious if there is > precedence for this > strategy, a.e. in other drivers that use named child nodes. In my initial search > through the kernel, I don't think I found anything like this -- does that mean > those drivers also inherently won't work with ACPI? If they are relying on names, yes, they won't work. It might be that some of them have specific ACPI approach where different description is in use. > The only driver I can find which uses device_get_named_child_node and has > an acpi_device_id is drivers/platform/x86/intel/chtwc_int33fe.c Yes, and you may notice the capitalization of the name. Also note, that relying on the name in ACPI like now is quite fragile due to no common standard between OEMs on how to name the same hardware nodes, they are free in that. > > P.S. The problem with ACPI is that "name" of the child node will be in capital > > letters as it's in accordance with the specification. > > Knowing that this is the limitation, some other potential resolutions > to potentially > consider might be: > > - Uppercase the names of the child nodes for the DT binding -- it appears > that the child node that chtwc_int33fe.c (the driver mentioned earlier) > accesses does indeed have an upper-cased name -- though that driver doesn't > have an of_device_id (makes sense, x86...). It seems named child nodes are > always lowercase in DT bindings -- not sure if that's a rule, or just how > it currently happens to be. Not an option AFAIK, the DT and ACPI specifications are requiring conflicting naming schema. Possible way is to lowering case for ACPI names, but I dunno. We need more opinions on this. > - Do a case invariant compare on the names (and/or check for both lowercase > and uppercase) For ACPI it will be always capital. For DT I have no clue if they allow capital letters there, but I assume they don't. > - Remove the use of child nodes, and combine the i2c and gpio nodes into the > cp2112's fwnode. I didn't do this initially because I wanted to avoid > namespace collisions between GPIO hogs and i2c child devices, and thought > that logically made sense to keep them separate, but that was before > knowing this limitation of ACPI. Seems to me not an option at all, we need to define hardware as is. If it combines two devices under a hood, should be two devices under a hood in DT/ACPI. [1]: https://stackoverflow.com/a/60855157/2511795
On Wed, Mar 01, 2023 at 04:59:07PM +0200, Andy Shevchenko wrote: > On Tue, Feb 28, 2023 at 01:05:54PM -0600, Daniel Kaehn wrote: > > On Mon, Feb 27, 2023 at 5:07 PM Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > > On Mon, Feb 27, 2023 at 08:07:58AM -0600, Danny Kaehn wrote: > > > > Bind I2C and GPIO interfaces to subnodes with names > > > > "i2c" and "gpio" if they exist, respectively. This > > > > allows the GPIO and I2C controllers to be described > > > > in firmware as usual. Additionally, support configuring the > > > > I2C bus speed from the clock-frequency device property. > > > > > > A bit shorten indentation... > > > > > > Nevertheless what I realized now is that this change, despite being OF > > > independent by used APIs, still OF-only. > > > > I assumed this would practically be the case -- not because of the casing > > reason you gave (wasn't aware of that, thanks for the FYI), but because it > > doesn't seem that there's any way to describe USB devices connected to > > a USB port in ACPI, at least as far as I can tell (ACPI is still largely a black > > box to me). > > That's not true :-) > > Microsoft created a schema that is not part of the specification, but let's > say a standard de facto. Linux supports that and I even played with it [1] > to get connection of the arbitrary device to USB-2-GPIO/I²C/SPI adapter. > > > But it seems reasonable that we should try to use the interface > > in a way so that it could be described using ACPI at some point (assuming > > that it isn't currently possible). > > > > > Would it be possible to allow indexed access to child nodes as well, so if > > > there are no names, we may still be able to use firmware nodes from the correct > > > children? > > > > Sure, you mean to fallback to using child nodes by index rather than by name > > in the case that that device_get_named_child_node() fails? Would we need to > > somehow verify that those nodes are the nodes we expect them to be? (a.e. > > node 0 is actually the i2c-controller, node 1 is actually the > > gpio-controller). > > Something like that, but I don't know if we can actually validate this without > having a preliminary agreement (hard-coded values) that index 0 is always let's > say I²C controller and so on. > > > I don't see a reason why not, though I am curious if there is > > precedence for this > > strategy, a.e. in other drivers that use named child nodes. In my initial search > > through the kernel, I don't think I found anything like this -- does that mean > > those drivers also inherently won't work with ACPI? > > If they are relying on names, yes, they won't work. It might be that some of them > have specific ACPI approach where different description is in use. > > > The only driver I can find which uses device_get_named_child_node and has > > an acpi_device_id is drivers/platform/x86/intel/chtwc_int33fe.c > > Yes, and you may notice the capitalization of the name. Also note, that relying > on the name in ACPI like now is quite fragile due to no common standard between > OEMs on how to name the same hardware nodes, they are free in that. Forgot to mention, that you need to take into consideration the drivers that are: 1) can be compiled with CONFIG_OF=n (i.e. using agnostic APIs); 2) having OF ID table; 3) being platform independent (no Kconfig dependency to something that's known to be an ARCH/CPU/etc one). All of those can be instantiated in ACPI via special PRP0001 ACPI ID [2]. > > > P.S. The problem with ACPI is that "name" of the child node will be in capital > > > letters as it's in accordance with the specification. > > > > Knowing that this is the limitation, some other potential resolutions > > to potentially > > consider might be: > > > > - Uppercase the names of the child nodes for the DT binding -- it appears > > that the child node that chtwc_int33fe.c (the driver mentioned earlier) > > accesses does indeed have an upper-cased name -- though that driver doesn't > > have an of_device_id (makes sense, x86...). It seems named child nodes are > > always lowercase in DT bindings -- not sure if that's a rule, or just how > > it currently happens to be. > > Not an option AFAIK, the DT and ACPI specifications are requiring conflicting > naming schema. > > Possible way is to lowering case for ACPI names, but I dunno. We need more > opinions on this. > > > - Do a case invariant compare on the names (and/or check for both lowercase > > and uppercase) > > For ACPI it will be always capital. For DT I have no clue if they allow capital > letters there, but I assume they don't. > > > - Remove the use of child nodes, and combine the i2c and gpio nodes into the > > cp2112's fwnode. I didn't do this initially because I wanted to avoid > > namespace collisions between GPIO hogs and i2c child devices, and thought > > that logically made sense to keep them separate, but that was before > > knowing this limitation of ACPI. > > Seems to me not an option at all, we need to define hardware as is. If it > combines two devices under a hood, should be two devices under a hood in > DT/ACPI. > > [1]: https://stackoverflow.com/a/60855157/2511795 [2]: https://docs.kernel.org/firmware-guide/acpi/enumeration.html#device-tree-namespace-link-device-id
On Mar 01 2023, Andy Shevchenko wrote: > +Cc: Hans (as some DT/ACPI interesting cases are being discussed). > > On Tue, Feb 28, 2023 at 01:05:54PM -0600, Daniel Kaehn wrote: > > On Mon, Feb 27, 2023 at 5:07 PM Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > > On Mon, Feb 27, 2023 at 08:07:58AM -0600, Danny Kaehn wrote: > > > > Bind I2C and GPIO interfaces to subnodes with names > > > > "i2c" and "gpio" if they exist, respectively. This > > > > allows the GPIO and I2C controllers to be described > > > > in firmware as usual. Additionally, support configuring the > > > > I2C bus speed from the clock-frequency device property. > > > > > > A bit shorten indentation... > > > > > > Nevertheless what I realized now is that this change, despite being OF > > > independent by used APIs, still OF-only. > > > > I assumed this would practically be the case -- not because of the casing > > reason you gave (wasn't aware of that, thanks for the FYI), but because it > > doesn't seem that there's any way to describe USB devices connected to > > a USB port in ACPI, at least as far as I can tell (ACPI is still largely a black > > box to me). > > That's not true :-) > > Microsoft created a schema that is not part of the specification, but let's > say a standard de facto. Linux supports that and I even played with it [1] > to get connection of the arbitrary device to USB-2-GPIO/I²C/SPI adapter. > > > But it seems reasonable that we should try to use the interface > > in a way so that it could be described using ACPI at some point (assuming > > that it isn't currently possible). > > > > > Would it be possible to allow indexed access to child nodes as well, so if > > > there are no names, we may still be able to use firmware nodes from the correct > > > children? > > > > Sure, you mean to fallback to using child nodes by index rather than by name > > in the case that that device_get_named_child_node() fails? Would we need to > > somehow verify that those nodes are the nodes we expect them to be? (a.e. > > node 0 is actually the i2c-controller, node 1 is actually the > > gpio-controller). > > Something like that, but I don't know if we can actually validate this without > having a preliminary agreement (hard-coded values) that index 0 is always let's > say I²C controller and so on. > > > I don't see a reason why not, though I am curious if there is > > precedence for this > > strategy, a.e. in other drivers that use named child nodes. In my initial search > > through the kernel, I don't think I found anything like this -- does that mean > > those drivers also inherently won't work with ACPI? > > If they are relying on names, yes, they won't work. It might be that some of them > have specific ACPI approach where different description is in use. > > > The only driver I can find which uses device_get_named_child_node and has > > an acpi_device_id is drivers/platform/x86/intel/chtwc_int33fe.c > > Yes, and you may notice the capitalization of the name. Also note, that relying > on the name in ACPI like now is quite fragile due to no common standard between > OEMs on how to name the same hardware nodes, they are free in that. > > > > P.S. The problem with ACPI is that "name" of the child node will be in capital > > > letters as it's in accordance with the specification. > > > > Knowing that this is the limitation, some other potential resolutions > > to potentially > > consider might be: > > > > - Uppercase the names of the child nodes for the DT binding -- it appears > > that the child node that chtwc_int33fe.c (the driver mentioned earlier) > > accesses does indeed have an upper-cased name -- though that driver doesn't > > have an of_device_id (makes sense, x86...). It seems named child nodes are > > always lowercase in DT bindings -- not sure if that's a rule, or just how > > it currently happens to be. > > Not an option AFAIK, the DT and ACPI specifications are requiring conflicting > naming schema. > > Possible way is to lowering case for ACPI names, but I dunno. We need more > opinions on this. > > > - Do a case invariant compare on the names (and/or check for both lowercase > > and uppercase) > > For ACPI it will be always capital. For DT I have no clue if they allow capital > letters there, but I assume they don't. > > > - Remove the use of child nodes, and combine the i2c and gpio nodes into the > > cp2112's fwnode. I didn't do this initially because I wanted to avoid > > namespace collisions between GPIO hogs and i2c child devices, and thought > > that logically made sense to keep them separate, but that was before > > knowing this limitation of ACPI. > > Seems to me not an option at all, we need to define hardware as is. If it > combines two devices under a hood, should be two devices under a hood in > DT/ACPI. > > [1]: https://stackoverflow.com/a/60855157/2511795 Thanks Andy for your help here, and thanks for that link. I am trying to test Danny's patch as I want to use it for my HID CI, being an owner of a CP2112 device myself. The current setup is using out of the tree patches [2] which are implementing a platform i2c-hid support and some manual addition of a I2C-HID device on top of it. This works fine but gets busted every now and then when the tree sees a change that conflicts with these patches. So with Danny's series, I thought I could have an SSDT override to declare that very same device instead of patching my kernel before testing it. Of course, it gets tricky because I need to run that under qemu. I am currently stuck at the "sharing the firmware_node from usb with HID" step and I'd like to know if you could help me. On my laptop, if I plug the CP2112 (without using a USB hub), I can get: $> ls -l /sys/bus/hid/devices/0003:10C4:EA90.* lrwxrwxrwx. 1 root root 0 Mar 2 17:02 /sys/bus/hid/devices/0003:10C4:EA90.0079 -> ../../../devices/pci0000:00/0000:00:14.0/usb2/2-9/2-9:1.0/0003:10C4:EA90.0079 $> ls -l /sys/bus/usb/devices/2-9*/firmware_node lrwxrwxrwx. 1 root root 0 Mar 2 17:03 /sys/bus/usb/devices/2-9:1.0/firmware_node -> ../../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:15/device:16/device:25 lrwxrwxrwx. 1 root root 0 Mar 2 17:02 /sys/bus/usb/devices/2-9/firmware_node -> ../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:15/device:16/device:25 So AFAIU the USB device is properly assigned a firmware node. My dsdt also shows the "Device (RHUB)" and I guess everything is fine. However, playing with qemu is not so easy. I am running qemu with the following arguments (well, almost because I have a wrapper script on top of it and I also run the compiled kernel from the current tree): #> qemu-system-x86_64 -bios /usr/share/edk2/ovmf/OVMF_CODE.fd \ -netdev user,id=hostnet0 \ -device virtio-net-pci,netdev=hostnet0 \ -m 4G \ -enable-kvm \ -cpu host \ -device qemu-xhci -usb \ -device 'usb-host,vendorid=0x10c4,productid=0xea90' \ -cdrom ~/Downloads/Fedora-Workstation-Live-x86_64-37-1.7.iso And this is what I get: #> ls -l /sys/bus/hid/devices/0003:10C4:EA90.* lrwxrwxrwx 1 root root 0 Mar 2 16:10 /sys/bus/hid/devices/0003:10C4:EA90.0001 -> ../../../devices/pci0000:00/0000:00:06.0/usb2/2-1/2-1:1.0/0003:10C4:EA90.0001 #> ls -l /sys/bus/usb/devices/2-1*/firmware_node ls: cannot access '/sys/bus/usb/devices/2-1*/firmware_node': No such file or directory Looking at the DSDT, I do not see any reference to the USB hub, so I wonder if the firmware_node needs to be populated first in the DSDT. Also note that if I plug the CP2112 over a docking station, I lose the firmware_node sysfs entries on the host too. Do you think it would be achievable to emulate that over qemu and use a mainline kernel without patches? Cheers, Benjamin [2] https://gitlab.freedesktop.org/bentiss/gitlab-kernel-ci/-/tree/master/VM
On Thu, Mar 02, 2023 at 06:06:06PM +0100, Benjamin Tissoires wrote: > On Mar 01 2023, Andy Shevchenko wrote: > > On Tue, Feb 28, 2023 at 01:05:54PM -0600, Daniel Kaehn wrote: ... > > [1]: https://stackoverflow.com/a/60855157/2511795 > > Thanks Andy for your help here, and thanks for that link. > > I am trying to test Danny's patch as I want to use it for my HID CI, > being an owner of a CP2112 device myself. > > The current setup is using out of the tree patches [2] which are > implementing a platform i2c-hid support and some manual addition of a > I2C-HID device on top of it. This works fine but gets busted every now > and then when the tree sees a change that conflicts with these patches. > > So with Danny's series, I thought I could have an SSDT override to > declare that very same device instead of patching my kernel before > testing it. > > Of course, it gets tricky because I need to run that under qemu. > > I am currently stuck at the "sharing the firmware_node from usb with > HID" step and I'd like to know if you could help me. > > On my laptop, if I plug the CP2112 (without using a USB hub), I can get: > > $> ls -l /sys/bus/hid/devices/0003:10C4:EA90.* > lrwxrwxrwx. 1 root root 0 Mar 2 17:02 /sys/bus/hid/devices/0003:10C4:EA90.0079 -> ../../../devices/pci0000:00/0000:00:14.0/usb2/2-9/2-9:1.0/0003:10C4:EA90.0079 > $> ls -l /sys/bus/usb/devices/2-9*/firmware_node > lrwxrwxrwx. 1 root root 0 Mar 2 17:03 /sys/bus/usb/devices/2-9:1.0/firmware_node -> ../../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:15/device:16/device:25 > lrwxrwxrwx. 1 root root 0 Mar 2 17:02 /sys/bus/usb/devices/2-9/firmware_node -> ../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:15/device:16/device:25 > > So AFAIU the USB device is properly assigned a firmware node. My dsdt > also shows the "Device (RHUB)" and I guess everything is fine. Yes, so far so good. > However, playing with qemu is not so easy. > > I am running qemu with the following arguments (well, almost because I > have a wrapper script on top of it and I also run the compiled kernel > from the current tree): > > #> qemu-system-x86_64 -bios /usr/share/edk2/ovmf/OVMF_CODE.fd \ > -netdev user,id=hostnet0 \ > -device virtio-net-pci,netdev=hostnet0 \ > -m 4G \ > -enable-kvm \ > -cpu host \ > -device qemu-xhci -usb \ > -device 'usb-host,vendorid=0x10c4,productid=0xea90' \ > -cdrom ~/Downloads/Fedora-Workstation-Live-x86_64-37-1.7.iso Side question, where can I get those blobs from (EDKII and Fedora Live CD)? I'm using Debian unstable. > And this is what I get: > > #> ls -l /sys/bus/hid/devices/0003:10C4:EA90.* > lrwxrwxrwx 1 root root 0 Mar 2 16:10 /sys/bus/hid/devices/0003:10C4:EA90.0001 -> ../../../devices/pci0000:00/0000:00:06.0/usb2/2-1/2-1:1.0/0003:10C4:EA90.0001 > > #> ls -l /sys/bus/usb/devices/2-1*/firmware_node > ls: cannot access '/sys/bus/usb/devices/2-1*/firmware_node': No such file or directory > > Looking at the DSDT, I do not see any reference to the USB hub, so I > wonder if the firmware_node needs to be populated first in the DSDT. So, where QEMU takes DSDT (ACPI tables in general) from? Can you patch that? I believe that's the problem in qemu. > Also note that if I plug the CP2112 over a docking station, I lose the > firmware_node sysfs entries on the host too. This seems like a lack of firmware node propagating in the USB hub code in the Linux kernel. > Do you think it would be achievable to emulate that over qemu and use a > mainline kernel without patches? As long as qemu provides correct DSDT it should work I assume. > [2] https://gitlab.freedesktop.org/bentiss/gitlab-kernel-ci/-/tree/master/VM
On Mon, Mar 6, 2023 at 11:49 AM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Thu, Mar 02, 2023 at 06:06:06PM +0100, Benjamin Tissoires wrote: > > On Mar 01 2023, Andy Shevchenko wrote: > > > On Tue, Feb 28, 2023 at 01:05:54PM -0600, Daniel Kaehn wrote: > > ... > > > > [1]: https://stackoverflow.com/a/60855157/2511795 > > > > Thanks Andy for your help here, and thanks for that link. > > > > I am trying to test Danny's patch as I want to use it for my HID CI, > > being an owner of a CP2112 device myself. > > > > The current setup is using out of the tree patches [2] which are > > implementing a platform i2c-hid support and some manual addition of a > > I2C-HID device on top of it. This works fine but gets busted every now > > and then when the tree sees a change that conflicts with these patches. > > > > So with Danny's series, I thought I could have an SSDT override to > > declare that very same device instead of patching my kernel before > > testing it. > > > > Of course, it gets tricky because I need to run that under qemu. > > > > I am currently stuck at the "sharing the firmware_node from usb with > > HID" step and I'd like to know if you could help me. > > > > On my laptop, if I plug the CP2112 (without using a USB hub), I can get: > > > > $> ls -l /sys/bus/hid/devices/0003:10C4:EA90.* > > lrwxrwxrwx. 1 root root 0 Mar 2 17:02 /sys/bus/hid/devices/0003:10C4:EA90.0079 -> ../../../devices/pci0000:00/0000:00:14.0/usb2/2-9/2-9:1.0/0003:10C4:EA90.0079 > > $> ls -l /sys/bus/usb/devices/2-9*/firmware_node > > lrwxrwxrwx. 1 root root 0 Mar 2 17:03 /sys/bus/usb/devices/2-9:1.0/firmware_node -> ../../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:15/device:16/device:25 > > lrwxrwxrwx. 1 root root 0 Mar 2 17:02 /sys/bus/usb/devices/2-9/firmware_node -> ../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:15/device:16/device:25 > > > > So AFAIU the USB device is properly assigned a firmware node. My dsdt > > also shows the "Device (RHUB)" and I guess everything is fine. > > Yes, so far so good. > > > However, playing with qemu is not so easy. > > > > I am running qemu with the following arguments (well, almost because I > > have a wrapper script on top of it and I also run the compiled kernel > > from the current tree): > > > > #> qemu-system-x86_64 -bios /usr/share/edk2/ovmf/OVMF_CODE.fd \ > > -netdev user,id=hostnet0 \ > > -device virtio-net-pci,netdev=hostnet0 \ > > -m 4G \ > > -enable-kvm \ > > -cpu host \ > > -device qemu-xhci -usb \ > > -device 'usb-host,vendorid=0x10c4,productid=0xea90' \ > > -cdrom ~/Downloads/Fedora-Workstation-Live-x86_64-37-1.7.iso > > Side question, where can I get those blobs from (EDKII and Fedora Live CD)? > I'm using Debian unstable. You can install the ovmf package in debian[3], which should have a similar file. For the Fedora livecd -> https://getfedora.org/fr/workstation/download/ but any other distribution with a recent enough kernel should show the same. > > > > And this is what I get: > > > > #> ls -l /sys/bus/hid/devices/0003:10C4:EA90.* > > lrwxrwxrwx 1 root root 0 Mar 2 16:10 /sys/bus/hid/devices/0003:10C4:EA90.0001 -> ../../../devices/pci0000:00/0000:00:06.0/usb2/2-1/2-1:1.0/0003:10C4:EA90.0001 > > > > #> ls -l /sys/bus/usb/devices/2-1*/firmware_node > > ls: cannot access '/sys/bus/usb/devices/2-1*/firmware_node': No such file or directory > > > > Looking at the DSDT, I do not see any reference to the USB hub, so I > > wonder if the firmware_node needs to be populated first in the DSDT. > > So, where QEMU takes DSDT (ACPI tables in general) from? Can you patch that? > I believe that's the problem in qemu. That's a good question and it's one I am not sure I have the answer to. I would have assumed that the DSDT was in the OVMF firmware, but given that we can arbitrarily add command line arguments, I believe it probably just provides a baseline and then we are screwed. The OVMF bios is compiled only once, so I doubt there is any mechanism to enable/disable a component in the DSDT, or make it dynamically generated. > > > Also note that if I plug the CP2112 over a docking station, I lose the > > firmware_node sysfs entries on the host too. > > This seems like a lack of firmware node propagating in the USB hub code in > the Linux kernel. That would make a lot of sense. FWIW, in the VM I see a firmware node on the pci controller itself: #> ls -l /sys/devices/pci0000\:00/0000\:00\:06.0/firmware_node lrwxrwxrwx 1 root root 0 Mar 6 12:24 /sys/devices/pci0000:00/0000:00:06.0/firmware_node -> ../../LNXSYSTM:00/LNXSYBUS:00/PNP0A03:00/device:07 And one the host, through a USB hub: #> ls -l /sys/bus/hid/devices/0003:10C4:EA90.* lrwxrwxrwx. 1 root root 0 Mar 6 13:26 /sys/bus/hid/devices/0003:10C4:EA90.007C -> ../../../devices/pci0000:00/0000:00:14.0/usb2/2-8/2-8.2/2-8.2.4/2-8.2.4:1.0/0003:10C4:EA90.007C #> ls -l /sys/bus/usb/devices/2-8*/firmware_node lrwxrwxrwx. 1 root root 0 Mar 2 16:53 /sys/bus/usb/devices/2-8:1.0/firmware_node -> ../../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:15/device:16/device:1e lrwxrwxrwx. 1 root root 0 Mar 2 16:53 /sys/bus/usb/devices/2-8/firmware_node -> ../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:15/device:16/device:1e Note that the firmware node propagation stopped at 2-8, and 2.8.2 is not having a firmware node. > > > Do you think it would be achievable to emulate that over qemu and use a > > mainline kernel without patches? > > As long as qemu provides correct DSDT it should work I assume. Just to be sure I understand, for this to work, we need the DSDT to export a "Device(RHUB)"? Or if we fix the USB fw_node propagation, could we just overwrite "\_SB_.PCI0.S30_"? "\_SB_.PCI0.S30_" is the name the ACPI is giving to the USB port in my VM case AFAIU. Cheers, Benjamin > > > [2] https://gitlab.freedesktop.org/bentiss/gitlab-kernel-ci/-/tree/master/VM [3] https://packages.debian.org/buster/all/ovmf/filelist
On Mon, Mar 06, 2023 at 01:36:51PM +0100, Benjamin Tissoires wrote: > On Mon, Mar 6, 2023 at 11:49 AM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Thu, Mar 02, 2023 at 06:06:06PM +0100, Benjamin Tissoires wrote: > > > On Mar 01 2023, Andy Shevchenko wrote: > > > > On Tue, Feb 28, 2023 at 01:05:54PM -0600, Daniel Kaehn wrote: ... [1]: https://stackoverflow.com/a/60855157/2511795 > > > Thanks Andy for your help here, and thanks for that link. > > > > > > I am trying to test Danny's patch as I want to use it for my HID CI, > > > being an owner of a CP2112 device myself. > > > > > > The current setup is using out of the tree patches [2] which are > > > implementing a platform i2c-hid support and some manual addition of a > > > I2C-HID device on top of it. This works fine but gets busted every now > > > and then when the tree sees a change that conflicts with these patches. > > > > > > So with Danny's series, I thought I could have an SSDT override to > > > declare that very same device instead of patching my kernel before > > > testing it. > > > > > > Of course, it gets tricky because I need to run that under qemu. > > > > > > I am currently stuck at the "sharing the firmware_node from usb with > > > HID" step and I'd like to know if you could help me. > > > > > > On my laptop, if I plug the CP2112 (without using a USB hub), I can get: > > > > > > $> ls -l /sys/bus/hid/devices/0003:10C4:EA90.* > > > lrwxrwxrwx. 1 root root 0 Mar 2 17:02 /sys/bus/hid/devices/0003:10C4:EA90.0079 -> ../../../devices/pci0000:00/0000:00:14.0/usb2/2-9/2-9:1.0/0003:10C4:EA90.0079 > > > $> ls -l /sys/bus/usb/devices/2-9*/firmware_node > > > lrwxrwxrwx. 1 root root 0 Mar 2 17:03 /sys/bus/usb/devices/2-9:1.0/firmware_node -> ../../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:15/device:16/device:25 > > > lrwxrwxrwx. 1 root root 0 Mar 2 17:02 /sys/bus/usb/devices/2-9/firmware_node -> ../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:15/device:16/device:25 > > > > > > So AFAIU the USB device is properly assigned a firmware node. My dsdt > > > also shows the "Device (RHUB)" and I guess everything is fine. > > > > Yes, so far so good. > > > > > However, playing with qemu is not so easy. > > > > > > I am running qemu with the following arguments (well, almost because I > > > have a wrapper script on top of it and I also run the compiled kernel > > > from the current tree): > > > > > > #> qemu-system-x86_64 -bios /usr/share/edk2/ovmf/OVMF_CODE.fd \ > > > -netdev user,id=hostnet0 \ > > > -device virtio-net-pci,netdev=hostnet0 \ > > > -m 4G \ > > > -enable-kvm \ > > > -cpu host \ > > > -device qemu-xhci -usb \ > > > -device 'usb-host,vendorid=0x10c4,productid=0xea90' \ > > > -cdrom ~/Downloads/Fedora-Workstation-Live-x86_64-37-1.7.iso > > > > Side question, where can I get those blobs from (EDKII and Fedora Live CD)? > > I'm using Debian unstable. > > You can install the ovmf package in debian[3], which should have a > similar file. > For the Fedora livecd -> https://getfedora.org/fr/workstation/download/ > but any other distribution with a recent enough kernel should show the > same. Thank you! > > > And this is what I get: > > > > > > #> ls -l /sys/bus/hid/devices/0003:10C4:EA90.* > > > lrwxrwxrwx 1 root root 0 Mar 2 16:10 /sys/bus/hid/devices/0003:10C4:EA90.0001 -> ../../../devices/pci0000:00/0000:00:06.0/usb2/2-1/2-1:1.0/0003:10C4:EA90.0001 > > > > > > #> ls -l /sys/bus/usb/devices/2-1*/firmware_node > > > ls: cannot access '/sys/bus/usb/devices/2-1*/firmware_node': No such file or directory > > > > > > Looking at the DSDT, I do not see any reference to the USB hub, so I > > > wonder if the firmware_node needs to be populated first in the DSDT. > > > > So, where QEMU takes DSDT (ACPI tables in general) from? Can you patch that? > > I believe that's the problem in qemu. > > That's a good question and it's one I am not sure I have the answer to. > I would have assumed that the DSDT was in the OVMF firmware, but given > that we can arbitrarily add command line arguments, I believe it > probably just provides a baseline and then we are screwed. The OVMF bios > is compiled only once, so I doubt there is any mechanism to > enable/disable a component in the DSDT, or make it dynamically > generated. We have two ways of filling missing parts: 1) update the original source of DSDT (firmware or bootloader, whichever provides that); 2) adding an overlay. The 2) works _if and only if_ there is *no* existing object in the tables. In such cases, you can simply provide a *full* hierarchy. See an example of PCI devices in the kernel documentation on how to do that. I believe something similar can be done for USB. > > > Also note that if I plug the CP2112 over a docking station, I lose the > > > firmware_node sysfs entries on the host too. > > > > This seems like a lack of firmware node propagating in the USB hub code in > > the Linux kernel. > > That would make a lot of sense. > > FWIW, in the VM I see a firmware node on the pci controller itself: > #> ls -l /sys/devices/pci0000\:00/0000\:00\:06.0/firmware_node > lrwxrwxrwx 1 root root 0 Mar 6 12:24 /sys/devices/pci0000:00/0000:00:06.0/firmware_node -> ../../LNXSYSTM:00/LNXSYBUS:00/PNP0A03:00/device:07 > > And one the host, through a USB hub: > > #> ls -l /sys/bus/hid/devices/0003:10C4:EA90.* > lrwxrwxrwx. 1 root root 0 Mar 6 13:26 /sys/bus/hid/devices/0003:10C4:EA90.007C -> ../../../devices/pci0000:00/0000:00:14.0/usb2/2-8/2-8.2/2-8.2.4/2-8.2.4:1.0/0003:10C4:EA90.007C > #> ls -l /sys/bus/usb/devices/2-8*/firmware_node > lrwxrwxrwx. 1 root root 0 Mar 2 16:53 /sys/bus/usb/devices/2-8:1.0/firmware_node -> ../../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:15/device:16/device:1e > lrwxrwxrwx. 1 root root 0 Mar 2 16:53 /sys/bus/usb/devices/2-8/firmware_node -> ../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:15/device:16/device:1e > > Note that the firmware node propagation stopped at 2-8, and 2.8.2 is not > having a firmware node. It would be nice if you can run `grep -H 15 /sys/bus/acpi/devices/*/status`, filter out unneeded ones, and for the rest also print their paths: `cat filtered_list_of_acpi_devs | while read p; do grep -H . $p/path; done` With this we will see what devices are actually present and up and running in the system and what their paths in the ACPI namespace. > > > Do you think it would be achievable to emulate that over qemu and use a > > > mainline kernel without patches? > > > > As long as qemu provides correct DSDT it should work I assume. > > Just to be sure I understand, for this to work, we need the DSDT to > export a "Device(RHUB)"? Not sure I understand the term "export" here. We need a description of the (to describe) missing parts. > Or if we fix the USB fw_node propagation, could we just overwrite > "\_SB_.PCI0.S30_"? "\_SB_.PCI0.S30_" is the name the ACPI is giving to > the USB port in my VM case AFAIU. I have no idea what is the S30 node. [2] https://gitlab.freedesktop.org/bentiss/gitlab-kernel-ci/-/tree/master/VM [3] https://packages.debian.org/buster/all/ovmf/filelist
On Mon, Mar 6, 2023 at 2:07 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Mon, Mar 06, 2023 at 01:36:51PM +0100, Benjamin Tissoires wrote: > > On Mon, Mar 6, 2023 at 11:49 AM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > On Thu, Mar 02, 2023 at 06:06:06PM +0100, Benjamin Tissoires wrote: > > > > On Mar 01 2023, Andy Shevchenko wrote: > > > > > On Tue, Feb 28, 2023 at 01:05:54PM -0600, Daniel Kaehn wrote: > > ... > > [1]: https://stackoverflow.com/a/60855157/2511795 > > > > > Thanks Andy for your help here, and thanks for that link. > > > > > > > > I am trying to test Danny's patch as I want to use it for my HID CI, > > > > being an owner of a CP2112 device myself. > > > > > > > > The current setup is using out of the tree patches [2] which are > > > > implementing a platform i2c-hid support and some manual addition of a > > > > I2C-HID device on top of it. This works fine but gets busted every now > > > > and then when the tree sees a change that conflicts with these patches. > > > > > > > > So with Danny's series, I thought I could have an SSDT override to > > > > declare that very same device instead of patching my kernel before > > > > testing it. > > > > > > > > Of course, it gets tricky because I need to run that under qemu. > > > > > > > > I am currently stuck at the "sharing the firmware_node from usb with > > > > HID" step and I'd like to know if you could help me. > > > > > > > > On my laptop, if I plug the CP2112 (without using a USB hub), I can get: > > > > > > > > $> ls -l /sys/bus/hid/devices/0003:10C4:EA90.* > > > > lrwxrwxrwx. 1 root root 0 Mar 2 17:02 /sys/bus/hid/devices/0003:10C4:EA90.0079 -> ../../../devices/pci0000:00/0000:00:14.0/usb2/2-9/2-9:1.0/0003:10C4:EA90.0079 > > > > $> ls -l /sys/bus/usb/devices/2-9*/firmware_node > > > > lrwxrwxrwx. 1 root root 0 Mar 2 17:03 /sys/bus/usb/devices/2-9:1.0/firmware_node -> ../../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:15/device:16/device:25 > > > > lrwxrwxrwx. 1 root root 0 Mar 2 17:02 /sys/bus/usb/devices/2-9/firmware_node -> ../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:15/device:16/device:25 > > > > > > > > So AFAIU the USB device is properly assigned a firmware node. My dsdt > > > > also shows the "Device (RHUB)" and I guess everything is fine. > > > > > > Yes, so far so good. > > > > > > > However, playing with qemu is not so easy. > > > > > > > > I am running qemu with the following arguments (well, almost because I > > > > have a wrapper script on top of it and I also run the compiled kernel > > > > from the current tree): > > > > > > > > #> qemu-system-x86_64 -bios /usr/share/edk2/ovmf/OVMF_CODE.fd \ > > > > -netdev user,id=hostnet0 \ > > > > -device virtio-net-pci,netdev=hostnet0 \ > > > > -m 4G \ > > > > -enable-kvm \ > > > > -cpu host \ > > > > -device qemu-xhci -usb \ > > > > -device 'usb-host,vendorid=0x10c4,productid=0xea90' \ > > > > -cdrom ~/Downloads/Fedora-Workstation-Live-x86_64-37-1.7.iso > > > > > > Side question, where can I get those blobs from (EDKII and Fedora Live CD)? > > > I'm using Debian unstable. > > > > You can install the ovmf package in debian[3], which should have a > > similar file. > > For the Fedora livecd -> https://getfedora.org/fr/workstation/download/ > > but any other distribution with a recent enough kernel should show the > > same. > > Thank you! > > > > > And this is what I get: > > > > > > > > #> ls -l /sys/bus/hid/devices/0003:10C4:EA90.* > > > > lrwxrwxrwx 1 root root 0 Mar 2 16:10 /sys/bus/hid/devices/0003:10C4:EA90.0001 -> ../../../devices/pci0000:00/0000:00:06.0/usb2/2-1/2-1:1.0/0003:10C4:EA90.0001 > > > > > > > > #> ls -l /sys/bus/usb/devices/2-1*/firmware_node > > > > ls: cannot access '/sys/bus/usb/devices/2-1*/firmware_node': No such file or directory > > > > > > > > Looking at the DSDT, I do not see any reference to the USB hub, so I > > > > wonder if the firmware_node needs to be populated first in the DSDT. > > > > > > So, where QEMU takes DSDT (ACPI tables in general) from? Can you patch that? > > > I believe that's the problem in qemu. > > > > That's a good question and it's one I am not sure I have the answer to. > > I would have assumed that the DSDT was in the OVMF firmware, but given > > that we can arbitrarily add command line arguments, I believe it > > probably just provides a baseline and then we are screwed. The OVMF bios > > is compiled only once, so I doubt there is any mechanism to > > enable/disable a component in the DSDT, or make it dynamically > > generated. > > We have two ways of filling missing parts: > 1) update the original source of DSDT (firmware or bootloader, > whichever provides that); > 2) adding an overlay. > > The 2) works _if and only if_ there is *no* existing object in the tables. > In such cases, you can simply provide a *full* hierarchy. See an example of > PCI devices in the kernel documentation on how to do that. I believe something > similar can be done for USB. Please find attached the dsdt from the Qemu VM. And after looking at it, your comments below, I think I am understanding what is happening (on the qemu side at least): #> grep PCI0.S /sys/bus/acpi/devices/*/path /sys/bus/acpi/devices/device:02/path:\_SB_.PCI0.S00_ /sys/bus/acpi/devices/device:03/path:\_SB_.PCI0.S10_ /sys/bus/acpi/devices/device:04/path:\_SB_.PCI0.S18_ /sys/bus/acpi/devices/device:05/path:\_SB_.PCI0.S20_ /sys/bus/acpi/devices/device:06/path:\_SB_.PCI0.S28_ /sys/bus/acpi/devices/device:07/path:\_SB_.PCI0.S30_ /sys/bus/acpi/devices/device:08/path:\_SB_.PCI0.S38_ /sys/bus/acpi/devices/device:09/path:\_SB_.PCI0.S40_ /sys/bus/acpi/devices/device:0a/path:\_SB_.PCI0.S48_ /sys/bus/acpi/devices/device:0b/path:\_SB_.PCI0.S50_ /sys/bus/acpi/devices/device:0c/path:\_SB_.PCI0.S58_ /sys/bus/acpi/devices/device:0d/path:\_SB_.PCI0.S60_ /sys/bus/acpi/devices/device:0e/path:\_SB_.PCI0.S68_ /sys/bus/acpi/devices/device:0f/path:\_SB_.PCI0.S70_ /sys/bus/acpi/devices/device:10/path:\_SB_.PCI0.S78_ /sys/bus/acpi/devices/device:11/path:\_SB_.PCI0.S80_ /sys/bus/acpi/devices/device:12/path:\_SB_.PCI0.S88_ /sys/bus/acpi/devices/device:13/path:\_SB_.PCI0.S90_ /sys/bus/acpi/devices/device:14/path:\_SB_.PCI0.S98_ /sys/bus/acpi/devices/device:15/path:\_SB_.PCI0.SA0_ /sys/bus/acpi/devices/device:16/path:\_SB_.PCI0.SA8_ /sys/bus/acpi/devices/device:17/path:\_SB_.PCI0.SB0_ /sys/bus/acpi/devices/device:18/path:\_SB_.PCI0.SB8_ /sys/bus/acpi/devices/device:19/path:\_SB_.PCI0.SC0_ /sys/bus/acpi/devices/device:1a/path:\_SB_.PCI0.SC8_ /sys/bus/acpi/devices/device:1b/path:\_SB_.PCI0.SD0_ /sys/bus/acpi/devices/device:1c/path:\_SB_.PCI0.SD8_ /sys/bus/acpi/devices/device:1d/path:\_SB_.PCI0.SE0_ /sys/bus/acpi/devices/device:1e/path:\_SB_.PCI0.SE8_ /sys/bus/acpi/devices/device:1f/path:\_SB_.PCI0.SF0_ /sys/bus/acpi/devices/device:20/path:\_SB_.PCI0.SF8_ And those translate on the DSDT as (for the S30/S38 chunk I am interested in): Device (S30) { Name (_ADR, 0x00060000) // _ADR: Address Name (ASUN, 0x06) Method (_DSM, 4, Serialized) // _DSM: Device-Specific Method { Return (PDSM (Arg0, Arg1, Arg2, Arg3, BSEL, ASUN)) } Name (_SUN, 0x06) // _SUN: Slot User Number Method (_EJ0, 1, NotSerialized) // _EJx: Eject Device, x=0-9 { PCEJ (BSEL, _SUN) } } Device (S38) { Name (_ADR, 0x00070000) // _ADR: Address Name (ASUN, 0x07) Method (_DSM, 4, Serialized) // _DSM: Device-Specific Method { Return (PDSM (Arg0, Arg1, Arg2, Arg3, BSEL, ASUN)) } Name (_SUN, 0x07) // _SUN: Slot User Number Method (_EJ0, 1, NotSerialized) // _EJx: Eject Device, x=0-9 { PCEJ (BSEL, _SUN) } } The forwarded USB node is actually on device:07 -> S30_, and as much as I'd like it to be a regular USB hub, this looks like a virtio node entry that allows to forward a physical device to the VM. So IMO, the missing piece might rely on the virtio-usb code which doesn't export the firmware node, which means I can not extend the device with an SSDT overlay ATM because the USB node doesn't have the fw_node. > > > > > Also note that if I plug the CP2112 over a docking station, I lose the > > > > firmware_node sysfs entries on the host too. > > > > > > This seems like a lack of firmware node propagating in the USB hub code in > > > the Linux kernel. > > > > That would make a lot of sense. > > > > FWIW, in the VM I see a firmware node on the pci controller itself: > > #> ls -l /sys/devices/pci0000\:00/0000\:00\:06.0/firmware_node > > lrwxrwxrwx 1 root root 0 Mar 6 12:24 /sys/devices/pci0000:00/0000:00:06.0/firmware_node -> ../../LNXSYSTM:00/LNXSYBUS:00/PNP0A03:00/device:07 > > > > And one the host, through a USB hub: > > > > #> ls -l /sys/bus/hid/devices/0003:10C4:EA90.* > > lrwxrwxrwx. 1 root root 0 Mar 6 13:26 /sys/bus/hid/devices/0003:10C4:EA90.007C -> ../../../devices/pci0000:00/0000:00:14.0/usb2/2-8/2-8.2/2-8.2.4/2-8.2.4:1.0/0003:10C4:EA90.007C > > #> ls -l /sys/bus/usb/devices/2-8*/firmware_node > > lrwxrwxrwx. 1 root root 0 Mar 2 16:53 /sys/bus/usb/devices/2-8:1.0/firmware_node -> ../../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:15/device:16/device:1e > > lrwxrwxrwx. 1 root root 0 Mar 2 16:53 /sys/bus/usb/devices/2-8/firmware_node -> ../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:15/device:16/device:1e > > > > Note that the firmware node propagation stopped at 2-8, and 2.8.2 is not > > having a firmware node. > > It would be nice if you can run `grep -H 15 /sys/bus/acpi/devices/*/status`, This command (both on the host and on the VM) does not show any USB device or even the PCI USB controller itself (PNP0A08 or PNP0A03). > filter out unneeded ones, and for the rest also print their paths: > `cat filtered_list_of_acpi_devs | while read p; do grep -H . $p/path; done` see above for the VM case, and in the host: #> grep XHC /sys/bus/acpi/devices/*/path /sys/bus/acpi/devices/device:15/path:\_SB_.PCI0.XHC_ /sys/bus/acpi/devices/device:16/path:\_SB_.PCI0.XHC_.RHUB /sys/bus/acpi/devices/device:17/path:\_SB_.PCI0.XHC_.RHUB.HS01 /sys/bus/acpi/devices/device:18/path:\_SB_.PCI0.XHC_.RHUB.HS02 /sys/bus/acpi/devices/device:19/path:\_SB_.PCI0.XHC_.RHUB.HS03 /sys/bus/acpi/devices/device:1a/path:\_SB_.PCI0.XHC_.RHUB.HS04 /sys/bus/acpi/devices/device:1b/path:\_SB_.PCI0.XHC_.RHUB.HS05 /sys/bus/acpi/devices/device:1c/path:\_SB_.PCI0.XHC_.RHUB.HS06 /sys/bus/acpi/devices/device:1d/path:\_SB_.PCI0.XHC_.RHUB.HS07 /sys/bus/acpi/devices/device:1e/path:\_SB_.PCI0.XHC_.RHUB.HS08 /sys/bus/acpi/devices/device:1f/path:\_SB_.PCI0.XHC_.RHUB.SS01 /sys/bus/acpi/devices/device:20/path:\_SB_.PCI0.XHC_.RHUB.SS02 /sys/bus/acpi/devices/device:21/path:\_SB_.PCI0.XHC_.RHUB.SS03 /sys/bus/acpi/devices/device:22/path:\_SB_.PCI0.XHC_.RHUB.SS04 /sys/bus/acpi/devices/device:23/path:\_SB_.PCI0.XHC_.RHUB.SS05 /sys/bus/acpi/devices/device:24/path:\_SB_.PCI0.XHC_.RHUB.SS06 /sys/bus/acpi/devices/device:25/path:\_SB_.PCI0.XHC_.RHUB.HS09 /sys/bus/acpi/devices/device:26/path:\_SB_.PCI0.XHC_.RHUB.HS10 /sys/bus/acpi/devices/device:27/path:\_SB_.PCI0.XHC_.RHUB.USR1 /sys/bus/acpi/devices/device:28/path:\_SB_.PCI0.XHC_.RHUB.USR2 /sys/bus/acpi/devices/device:85/path:\_SB_.PCI0.TXHC /sys/bus/acpi/devices/device:86/path:\_SB_.PCI0.TXHC.RHUB /sys/bus/acpi/devices/device:87/path:\_SB_.PCI0.TXHC.RHUB.HS01 /sys/bus/acpi/devices/device:88/path:\_SB_.PCI0.TXHC.RHUB.SS01 /sys/bus/acpi/devices/device:89/path:\_SB_.PCI0.TXHC.RHUB.SS02 /sys/bus/acpi/devices/device:8a/path:\_SB_.PCI0.TXHC.RHUB.SS03 /sys/bus/acpi/devices/device:8b/path:\_SB_.PCI0.TXHC.RHUB.SS04 Which is coherent with the ../../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:15/device:16/device:1e I get when looking at the USB port. > > With this we will see what devices are actually present and up and running > in the system and what their paths in the ACPI namespace. So it seems that the USB hub functionality is not creating fw_nodes for its children. But I am not sure this is a battle we want to fight right now, because it doesn't make a lot of sense IMO to add an SSDT overlay on a hub. > > > > > Do you think it would be achievable to emulate that over qemu and use a > > > > mainline kernel without patches? > > > > > > As long as qemu provides correct DSDT it should work I assume. > > > > Just to be sure I understand, for this to work, we need the DSDT to > > export a "Device(RHUB)"? > > Not sure I understand the term "export" here. We need a description > of the (to describe) missing parts. Yes, I meant "to describe" it. > > > Or if we fix the USB fw_node propagation, could we just overwrite > > "\_SB_.PCI0.S30_"? "\_SB_.PCI0.S30_" is the name the ACPI is giving to > > the USB port in my VM case AFAIU. > > I have no idea what is the S30 node. That gave me the hint I needed, I think. The problem must be in the virtio drivers, where it doesn't attach the fw_node to the components it creates. We probably need kind of the same patch Danny is sending in 2/3 in this series, but for virtio. Cheers, Benjamin > > [2] https://gitlab.freedesktop.org/bentiss/gitlab-kernel-ci/-/tree/master/VM > [3] https://packages.debian.org/buster/all/ovmf/filelist > > -- > With Best Regards, > Andy Shevchenko > >
On Mon, Mar 06, 2023 at 03:48:18PM +0100, Benjamin Tissoires wrote: > > > On Mon, Mar 6, 2023 at 2:07 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > > On Mon, Mar 06, 2023 at 01:36:51PM +0100, Benjamin Tissoires wrote: > > > On Mon, Mar 6, 2023 at 11:49 AM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > > On Thu, Mar 02, 2023 at 06:06:06PM +0100, Benjamin Tissoires wrote: > > > > > On Mar 01 2023, Andy Shevchenko wrote: > > > > > > On Tue, Feb 28, 2023 at 01:05:54PM -0600, Daniel Kaehn wrote: > > > > ... > > > > [1]: https://stackoverflow.com/a/60855157/2511795 > > > > > > > Thanks Andy for your help here, and thanks for that link. > > > > > > > > > > I am trying to test Danny's patch as I want to use it for my HID CI, > > > > > being an owner of a CP2112 device myself. > > > > > > > > > > The current setup is using out of the tree patches [2] which are > > > > > implementing a platform i2c-hid support and some manual addition of a > > > > > I2C-HID device on top of it. This works fine but gets busted every now > > > > > and then when the tree sees a change that conflicts with these patches. > > > > > > > > > > So with Danny's series, I thought I could have an SSDT override to > > > > > declare that very same device instead of patching my kernel before > > > > > testing it. > > > > > > > > > > Of course, it gets tricky because I need to run that under qemu. > > > > > > > > > > I am currently stuck at the "sharing the firmware_node from usb with > > > > > HID" step and I'd like to know if you could help me. > > > > > > > > > > On my laptop, if I plug the CP2112 (without using a USB hub), I can get: > > > > > > > > > > $> ls -l /sys/bus/hid/devices/0003:10C4:EA90.* > > > > > lrwxrwxrwx. 1 root root 0 Mar 2 17:02 /sys/bus/hid/devices/0003:10C4:EA90.0079 -> ../../../devices/pci0000:00/0000:00:14.0/usb2/2-9/2-9:1.0/0003:10C4:EA90.0079 > > > > > $> ls -l /sys/bus/usb/devices/2-9*/firmware_node > > > > > lrwxrwxrwx. 1 root root 0 Mar 2 17:03 /sys/bus/usb/devices/2-9:1.0/firmware_node -> ../../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:15/device:16/device:25 > > > > > lrwxrwxrwx. 1 root root 0 Mar 2 17:02 /sys/bus/usb/devices/2-9/firmware_node -> ../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:15/device:16/device:25 > > > > > > > > > > So AFAIU the USB device is properly assigned a firmware node. My dsdt > > > > > also shows the "Device (RHUB)" and I guess everything is fine. > > > > > > > > Yes, so far so good. > > > > > > > > > However, playing with qemu is not so easy. > > > > > > > > > > I am running qemu with the following arguments (well, almost because I > > > > > have a wrapper script on top of it and I also run the compiled kernel > > > > > from the current tree): > > > > > > > > > > #> qemu-system-x86_64 -bios /usr/share/edk2/ovmf/OVMF_CODE.fd \ > > > > > -netdev user,id=hostnet0 \ > > > > > -device virtio-net-pci,netdev=hostnet0 \ > > > > > -m 4G \ > > > > > -enable-kvm \ > > > > > -cpu host \ > > > > > -device qemu-xhci -usb \ > > > > > -device 'usb-host,vendorid=0x10c4,productid=0xea90' \ > > > > > -cdrom ~/Downloads/Fedora-Workstation-Live-x86_64-37-1.7.iso > > > > > > > > Side question, where can I get those blobs from (EDKII and Fedora Live CD)? > > > > I'm using Debian unstable. > > > > > > You can install the ovmf package in debian[3], which should have a > > > similar file. > > > For the Fedora livecd -> https://getfedora.org/fr/workstation/download/ > > > but any other distribution with a recent enough kernel should show the > > > same. > > > > Thank you! > > > > > > > And this is what I get: > > > > > > > > > > #> ls -l /sys/bus/hid/devices/0003:10C4:EA90.* > > > > > lrwxrwxrwx 1 root root 0 Mar 2 16:10 /sys/bus/hid/devices/0003:10C4:EA90.0001 -> ../../../devices/pci0000:00/0000:00:06.0/usb2/2-1/2-1:1.0/0003:10C4:EA90.0001 > > > > > > > > > > #> ls -l /sys/bus/usb/devices/2-1*/firmware_node > > > > > ls: cannot access '/sys/bus/usb/devices/2-1*/firmware_node': No such file or directory > > > > > > > > > > Looking at the DSDT, I do not see any reference to the USB hub, so I > > > > > wonder if the firmware_node needs to be populated first in the DSDT. > > > > > > > > So, where QEMU takes DSDT (ACPI tables in general) from? Can you patch that? > > > > I believe that's the problem in qemu. > > > > > > That's a good question and it's one I am not sure I have the answer to. > > > I would have assumed that the DSDT was in the OVMF firmware, but given > > > that we can arbitrarily add command line arguments, I believe it > > > probably just provides a baseline and then we are screwed. The OVMF bios > > > is compiled only once, so I doubt there is any mechanism to > > > enable/disable a component in the DSDT, or make it dynamically > > > generated. > > > > We have two ways of filling missing parts: > > 1) update the original source of DSDT (firmware or bootloader, > > whichever provides that); > > 2) adding an overlay. > > > > The 2) works _if and only if_ there is *no* existing object in the tables. > > In such cases, you can simply provide a *full* hierarchy. See an example of > > PCI devices in the kernel documentation on how to do that. I believe something > > similar can be done for USB. > > Please find attached the dsdt from the Qemu VM. Thank you! > And after looking at it, your comments below, I think I am understanding > what is happening (on the qemu side at least): > > #> grep PCI0.S /sys/bus/acpi/devices/*/path > /sys/bus/acpi/devices/device:02/path:\_SB_.PCI0.S00_ > /sys/bus/acpi/devices/device:03/path:\_SB_.PCI0.S10_ > /sys/bus/acpi/devices/device:04/path:\_SB_.PCI0.S18_ > /sys/bus/acpi/devices/device:05/path:\_SB_.PCI0.S20_ > /sys/bus/acpi/devices/device:06/path:\_SB_.PCI0.S28_ > /sys/bus/acpi/devices/device:07/path:\_SB_.PCI0.S30_ > /sys/bus/acpi/devices/device:08/path:\_SB_.PCI0.S38_ > /sys/bus/acpi/devices/device:09/path:\_SB_.PCI0.S40_ > /sys/bus/acpi/devices/device:0a/path:\_SB_.PCI0.S48_ > /sys/bus/acpi/devices/device:0b/path:\_SB_.PCI0.S50_ > /sys/bus/acpi/devices/device:0c/path:\_SB_.PCI0.S58_ > /sys/bus/acpi/devices/device:0d/path:\_SB_.PCI0.S60_ > /sys/bus/acpi/devices/device:0e/path:\_SB_.PCI0.S68_ > /sys/bus/acpi/devices/device:0f/path:\_SB_.PCI0.S70_ > /sys/bus/acpi/devices/device:10/path:\_SB_.PCI0.S78_ > /sys/bus/acpi/devices/device:11/path:\_SB_.PCI0.S80_ > /sys/bus/acpi/devices/device:12/path:\_SB_.PCI0.S88_ > /sys/bus/acpi/devices/device:13/path:\_SB_.PCI0.S90_ > /sys/bus/acpi/devices/device:14/path:\_SB_.PCI0.S98_ > /sys/bus/acpi/devices/device:15/path:\_SB_.PCI0.SA0_ > /sys/bus/acpi/devices/device:16/path:\_SB_.PCI0.SA8_ > /sys/bus/acpi/devices/device:17/path:\_SB_.PCI0.SB0_ > /sys/bus/acpi/devices/device:18/path:\_SB_.PCI0.SB8_ > /sys/bus/acpi/devices/device:19/path:\_SB_.PCI0.SC0_ > /sys/bus/acpi/devices/device:1a/path:\_SB_.PCI0.SC8_ > /sys/bus/acpi/devices/device:1b/path:\_SB_.PCI0.SD0_ > /sys/bus/acpi/devices/device:1c/path:\_SB_.PCI0.SD8_ > /sys/bus/acpi/devices/device:1d/path:\_SB_.PCI0.SE0_ > /sys/bus/acpi/devices/device:1e/path:\_SB_.PCI0.SE8_ > /sys/bus/acpi/devices/device:1f/path:\_SB_.PCI0.SF0_ > /sys/bus/acpi/devices/device:20/path:\_SB_.PCI0.SF8_ Ah, not much to get out of it. From DSDT _ADR() you may deduct the PCI BDF of each device in the topology. > And those translate on the DSDT as (for the S30/S38 chunk I am > interested in): > > Device (S30) > { > Name (_ADR, 0x00060000) // _ADR: Address In PCI this is 00:06.0 > Name (ASUN, 0x06) > Method (_DSM, 4, Serialized) // _DSM: Device-Specific Method > { > Return (PDSM (Arg0, Arg1, Arg2, Arg3, BSEL, ASUN)) > } > > Name (_SUN, 0x06) // _SUN: Slot User Number > Method (_EJ0, 1, NotSerialized) // _EJx: Eject Device, x=0-9 > { > PCEJ (BSEL, _SUN) > } > } > > Device (S38) > { > Name (_ADR, 0x00070000) // _ADR: Address 00:07.0 respectively. > Name (ASUN, 0x07) > Method (_DSM, 4, Serialized) // _DSM: Device-Specific Method > { > Return (PDSM (Arg0, Arg1, Arg2, Arg3, BSEL, ASUN)) > } > > Name (_SUN, 0x07) // _SUN: Slot User Number > Method (_EJ0, 1, NotSerialized) // _EJx: Eject Device, x=0-9 > { > PCEJ (BSEL, _SUN) > } > } > > The forwarded USB node is actually on device:07 -> S30_, and as much as > I'd like it to be a regular USB hub, this looks like a virtio node entry > that allows to forward a physical device to the VM. > > So IMO, the missing piece might rely on the virtio-usb code which > doesn't export the firmware node, which means I can not extend the > device with an SSDT overlay ATM because the USB node doesn't have the > fw_node. Ah, that very much may explain this! > > > > > Also note that if I plug the CP2112 over a docking station, I lose the > > > > > firmware_node sysfs entries on the host too. > > > > > > > > This seems like a lack of firmware node propagating in the USB hub code in > > > > the Linux kernel. > > > > > > That would make a lot of sense. > > > > > > FWIW, in the VM I see a firmware node on the pci controller itself: > > > #> ls -l /sys/devices/pci0000\:00/0000\:00\:06.0/firmware_node > > > lrwxrwxrwx 1 root root 0 Mar 6 12:24 /sys/devices/pci0000:00/0000:00:06.0/firmware_node -> ../../LNXSYSTM:00/LNXSYBUS:00/PNP0A03:00/device:07 > > > > > > And one the host, through a USB hub: > > > > > > #> ls -l /sys/bus/hid/devices/0003:10C4:EA90.* > > > lrwxrwxrwx. 1 root root 0 Mar 6 13:26 /sys/bus/hid/devices/0003:10C4:EA90.007C -> ../../../devices/pci0000:00/0000:00:14.0/usb2/2-8/2-8.2/2-8.2.4/2-8.2.4:1.0/0003:10C4:EA90.007C > > > #> ls -l /sys/bus/usb/devices/2-8*/firmware_node > > > lrwxrwxrwx. 1 root root 0 Mar 2 16:53 /sys/bus/usb/devices/2-8:1.0/firmware_node -> ../../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:15/device:16/device:1e > > > lrwxrwxrwx. 1 root root 0 Mar 2 16:53 /sys/bus/usb/devices/2-8/firmware_node -> ../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:15/device:16/device:1e > > > > > > Note that the firmware node propagation stopped at 2-8, and 2.8.2 is not > > > having a firmware node. > > > > It would be nice if you can run `grep -H 15 /sys/bus/acpi/devices/*/status`, > > This command (both on the host and on the VM) does not show any USB > device or even the PCI USB controller itself (PNP0A08 or PNP0A03). > > > filter out unneeded ones, and for the rest also print their paths: > > `cat filtered_list_of_acpi_devs | while read p; do grep -H . $p/path; done` > > see above for the VM case, and in the host: > > #> grep XHC /sys/bus/acpi/devices/*/path > /sys/bus/acpi/devices/device:15/path:\_SB_.PCI0.XHC_ > /sys/bus/acpi/devices/device:16/path:\_SB_.PCI0.XHC_.RHUB > /sys/bus/acpi/devices/device:17/path:\_SB_.PCI0.XHC_.RHUB.HS01 > /sys/bus/acpi/devices/device:18/path:\_SB_.PCI0.XHC_.RHUB.HS02 > /sys/bus/acpi/devices/device:19/path:\_SB_.PCI0.XHC_.RHUB.HS03 > /sys/bus/acpi/devices/device:1a/path:\_SB_.PCI0.XHC_.RHUB.HS04 > /sys/bus/acpi/devices/device:1b/path:\_SB_.PCI0.XHC_.RHUB.HS05 > /sys/bus/acpi/devices/device:1c/path:\_SB_.PCI0.XHC_.RHUB.HS06 > /sys/bus/acpi/devices/device:1d/path:\_SB_.PCI0.XHC_.RHUB.HS07 > /sys/bus/acpi/devices/device:1e/path:\_SB_.PCI0.XHC_.RHUB.HS08 > /sys/bus/acpi/devices/device:1f/path:\_SB_.PCI0.XHC_.RHUB.SS01 > /sys/bus/acpi/devices/device:20/path:\_SB_.PCI0.XHC_.RHUB.SS02 > /sys/bus/acpi/devices/device:21/path:\_SB_.PCI0.XHC_.RHUB.SS03 > /sys/bus/acpi/devices/device:22/path:\_SB_.PCI0.XHC_.RHUB.SS04 > /sys/bus/acpi/devices/device:23/path:\_SB_.PCI0.XHC_.RHUB.SS05 > /sys/bus/acpi/devices/device:24/path:\_SB_.PCI0.XHC_.RHUB.SS06 > /sys/bus/acpi/devices/device:25/path:\_SB_.PCI0.XHC_.RHUB.HS09 > /sys/bus/acpi/devices/device:26/path:\_SB_.PCI0.XHC_.RHUB.HS10 > /sys/bus/acpi/devices/device:27/path:\_SB_.PCI0.XHC_.RHUB.USR1 > /sys/bus/acpi/devices/device:28/path:\_SB_.PCI0.XHC_.RHUB.USR2 > /sys/bus/acpi/devices/device:85/path:\_SB_.PCI0.TXHC > /sys/bus/acpi/devices/device:86/path:\_SB_.PCI0.TXHC.RHUB > /sys/bus/acpi/devices/device:87/path:\_SB_.PCI0.TXHC.RHUB.HS01 > /sys/bus/acpi/devices/device:88/path:\_SB_.PCI0.TXHC.RHUB.SS01 > /sys/bus/acpi/devices/device:89/path:\_SB_.PCI0.TXHC.RHUB.SS02 > /sys/bus/acpi/devices/device:8a/path:\_SB_.PCI0.TXHC.RHUB.SS03 > /sys/bus/acpi/devices/device:8b/path:\_SB_.PCI0.TXHC.RHUB.SS04 > > Which is coherent with the ../../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:15/device:16/device:1e > I get when looking at the USB port. > > > With this we will see what devices are actually present and up and running > > in the system and what their paths in the ACPI namespace. > > So it seems that the USB hub functionality is not creating fw_nodes for > its children. But I am not sure this is a battle we want to fight right > now, because it doesn't make a lot of sense IMO to add an SSDT overlay > on a hub. The description of the attachable devices should really be in the overlays if user wants them, but it's another story. > > > > > Do you think it would be achievable to emulate that over qemu and use a > > > > > mainline kernel without patches? > > > > > > > > As long as qemu provides correct DSDT it should work I assume. > > > > > > Just to be sure I understand, for this to work, we need the DSDT to > > > export a "Device(RHUB)"? > > > > Not sure I understand the term "export" here. We need a description > > of the (to describe) missing parts. > > Yes, I meant "to describe" it. > > > > Or if we fix the USB fw_node propagation, could we just overwrite > > > "\_SB_.PCI0.S30_"? "\_SB_.PCI0.S30_" is the name the ACPI is giving to > > > the USB port in my VM case AFAIU. > > > > I have no idea what is the S30 node. > > That gave me the hint I needed, I think. The problem must be in the > virtio drivers, where it doesn't attach the fw_node to the components it > creates. We probably need kind of the same patch Danny is sending in 2/3 > in this series, but for virtio. Sounds like that, indeed. > > [2] https://gitlab.freedesktop.org/bentiss/gitlab-kernel-ci/-/tree/master/VM > > [3] https://packages.debian.org/buster/all/ovmf/filelist
Hello,
Sorry about the radio silence from me --
I've been trying to get this working on my end as well.
I was able to get my passed-through USB device on a qemu system to
have a firmware_node by
using the "Upgrading ACPI tables via initrd" kernel feature [1]. In
case this provides helpful information,
the below describes what I did.
This was using the default yocto core-image-minimal image and
qemu-system-x86_64.
I invoke qemu with the convenience "runqemu" script, as follows:
runqemu nographic qemuparams="
-initrd ../acpi-overlay/instrumented_initrd
-device 'usb-host,vendorid=0x10c4,productid=0xea90'
-pflash ./build/tmp/work/core2-64-poky-linux/ovmf/edk2-stable202211-r0/ovmf/ovmf.fd
"
Which invokes qemu with something like the following (sorry about the
long lines..):
qemu-system-x86_64 \
-device virtio-net-pci,netdev=net0,mac=52:54:00:12:34:02 \
-netdev tap,id=net0,ifname=tap0,script=no,downscript=no \
-object rng-random,filename=/dev/urandom,id=rng0 \
-device virtio-rng-pci,rng=rng0 \
-drive file=/home/kaehnd/src/local/x86/build/tmp/deploy/images/qemux86-64/core-image-minimal-qemux86-64-20230306143252.rootfs.ext4,if=virtio,format=raw
\
-usb -device usb-tablet -usb -device usb-kbd -cpu IvyBridge \
-machine q35,i8042=off -smp 4 -m 256 \
-device 'usb-host,vendorid=0x10c4,productid=0xea90' \
-serial mon:stdio -serial null -nographic \
-kernel /home/kaehnd/src/local/x86/build/tmp/deploy/images/qemux86-64/bzImage
\
-append 'root=/dev/vda
rw ip=192.168.7.2::192.168.7.1:255.255.255.0::eth0:off:8.8.8.8
console=ttyS0 console=ttyS1 oprofile.timer=1
tsc=reliable no_timer_check rcupdate.rcu_expedited=1 '
The sysfs path tree for the CP2112 was as follows:
#> ls -l /sys/bus/hid/devices/0003:10C4:EA90.0003
lrwxrwxrwx 1 root root 0 Mar 6 19:24
/sys/bus/hid/devices/0003:10C4:EA90.0003 ->
../../../devices/pci0000:00/0000:00:1d.1/usb3/3-1/3-1:1.0/0003:10C4:EA90.0003
Out of the box, firmware_node files existed only through what I assume
is the PCI bus:
/sys/devices/pci0000:00
It's ACPI path:
#> cat /sys/devices/pci0000:00/firmware_node/path
\_SB_.PCI0
Using the instructions at [1], I grabbed the dsdt table, and modified
it as follows.
Underneath the PCI0 node, I added the following ASL
```
Device (SE9)
{
Name (_ADR, 0x001D0001) // _ADR: Address
Device (RHUB)
{
Name (_ADR, Zero)
Device (CP2) // the USB-hid & CP2112 shared node
{
Name (_ADR, One)
}
}
}
```
If I'm understanding correctly, this adds the SE9 device as function 1
of PCI device 0x1d,
then RHUB as the USB controller it provides, and finally, CP2 as the
USB device attached to port 1 of the controller.
With this as the loaded dsdt table, the USB device now has a firmware_node :)
#> cat /sys/bus/usb/devices/3-1:1.0/firmware_node/path
\_SB_.PCI0.SE9_.RHUB.CP2_
After applying my patches, the HID device also references this node:
#> cat /sys/bus/hid/devices/0003:10C4:EA90.0003/firmware_node/path
\_SB_.PCI0.SE9_.RHUB.CP2_
With this all said -- I noticed iasl prints this statement when trying
to create a node with a lowercase name:
"At least one lower case letter found in NameSeg, ASL is case
insensitive - converting to upper case (GPIO)"
I wonder if this suggests that adding a call to toupper() to
acpi_fwnode_get_named_child_node would be
an appropriate solution for the node name casing issue....
[1] https://www.kernel.org/doc/html/latest/admin-guide/acpi/initrd_table_override.html
On Mon, Mar 06, 2023 at 01:40:16PM -0600, Daniel Kaehn wrote: ... > Device (SE9) > { > Name (_ADR, 0x001D0001) // _ADR: Address > Device (RHUB) > { > Name (_ADR, Zero) > Device (CP2) // the USB-hid & CP2112 shared node > { > Name (_ADR, One) > } > } > } > > If I'm understanding correctly, this adds the SE9 device as function 1 > of PCI device 0x1d, To be precise this does not add the device. It adds a description of the companion device in case the real one will appear on the PCI bus with BDF 00:1d.1. > then RHUB as the USB controller it provides, and finally, CP2 as the > USB device attached to port 1 of the controller. > > With this as the loaded dsdt table, the USB device now has a firmware_node :) > #> cat /sys/bus/usb/devices/3-1:1.0/firmware_node/path > \_SB_.PCI0.SE9_.RHUB.CP2_ > > After applying my patches, the HID device also references this node: > #> cat /sys/bus/hid/devices/0003:10C4:EA90.0003/firmware_node/path > \_SB_.PCI0.SE9_.RHUB.CP2_ > > With this all said -- I noticed iasl prints this statement when trying > to create a node with a lowercase name: > "At least one lower case letter found in NameSeg, ASL is case > insensitive - converting to upper case (GPIO)" Yes, because it should be in the upper case. > I wonder if this suggests that adding a call to toupper() to > acpi_fwnode_get_named_child_node would be > an appropriate solution for the node name casing issue.... I dunno. You need to ask in the linux-acpi@ mailing list. To me this is corner case that can't be easily solved (because two different specifications treat it differently. You also need to ask DT people about capital letters there. And my guts tell me that it's probably also carved in the spec as "must be lower case" or alike.
On Mar 06 2023, Andy Shevchenko wrote: > On Mon, Mar 06, 2023 at 01:40:16PM -0600, Daniel Kaehn wrote: > > ... > > > Device (SE9) > > { > > Name (_ADR, 0x001D0001) // _ADR: Address > > Device (RHUB) > > { > > Name (_ADR, Zero) > > Device (CP2) // the USB-hid & CP2112 shared node > > { > > Name (_ADR, One) > > } > > } > > } > > > > If I'm understanding correctly, this adds the SE9 device as function 1 > > of PCI device 0x1d, > > To be precise this does not add the device. It adds a description of > the companion device in case the real one will appear on the PCI bus > with BDF 00:1d.1. > > > then RHUB as the USB controller it provides, and finally, CP2 as the > > USB device attached to port 1 of the controller. > > > > With this as the loaded dsdt table, the USB device now has a firmware_node :) > > #> cat /sys/bus/usb/devices/3-1:1.0/firmware_node/path > > \_SB_.PCI0.SE9_.RHUB.CP2_ > > > > After applying my patches, the HID device also references this node: > > #> cat /sys/bus/hid/devices/0003:10C4:EA90.0003/firmware_node/path > > \_SB_.PCI0.SE9_.RHUB.CP2_ > > Great! Thanks a lot for that. Turns out that with both of your inputs I can also do the same, but without the need for OVMF and DSDT patching, with just an SSDT override. Turns out that the override documentation [1] mentions "This option allows loading of user defined SSDTs from initrd and it is useful when the system does not support EFI or ..." FWIW, I am attaching my full DSDT override in case it is valuable: (on my system, the default USB controller (non-xhc) is at PCI address 1.2, which explains the slight difference). It can be loaded in the same way you are overriding the full DSDT, but with just that compilation output: --- DefinitionBlock ("cp2112.aml", "SSDT", 5, "", "CP2112", 1) { External (_SB_.PCI0, DeviceObj) Scope (\_SB_.PCI0) { Device (USB0) { Name (_ADR, 0x00010002) // _ADR: Address Device (RHUB) { Name (_ADR, Zero) Device (CP21) // the USB-hid & CP2112 shared node { Name (_ADR, One) Device (I2C) { Name (_ADR, Zero) Name (_STA, 0x0F) } Device (GPIO) { Name (_ADR, One) Name (_STA, 0x0F) } } } } } Scope (\_SB_.PCI0.USB0.RHUB.CP21.I2C) { Device (TPD0) { Name (_HID, "RMI40001") Name (_CID, "PNP0C50") Name (_STA, 0x0F) Name (SBFB, ResourceTemplate () { I2cSerialBusV2 (0x00c, ControllerInitiated, 100000, AddressingMode7Bit, "\\_SB_.PCI0.USB0.RHUB.CP21.I2C", 0x00, ResourceConsumer,, Exclusive, ) }) Name (SBFG, ResourceTemplate () { GpioInt (Level, ActiveLow, Exclusive, PullDefault, 0x0000, "\\_SB_.PCI0.USB0.RHUB.CP21.GPIO", 0x00, ResourceConsumer, , ) { // Pin list 0x0002 } }) Method(_CRS, 0x0, NotSerialized) { Return (ConcatenateResTemplate (SBFB, SBFG)) } Method(_DSM, 0x4, Serialized) { // DSM UUID switch (ToBuffer (Arg0)) { // ACPI DSM UUID for HIDI2C case (ToUUID ("3CDFF6F7-4267-4555-AD05-B30A3D8938DE")) { // DSM Function switch (ToInteger (Arg2)) { // Function 0: Query function, return based on revision case(0) { // DSM Revision switch (ToInteger (Arg1)) { // Revision 1: Function 1 supported case (1) { Return (Buffer (One) { 0x03 }) } default { // Revision 2+: no functions supported Return (Buffer (One) { 0x00 }) } } } // Function 1 : HID Function case(1) { // HID Descriptor Address Return (0x0020) } default { // Functions 2+: not supported Return (Buffer (One) { 0x00 }) } } } default { // No other GUIDs supported Return (Buffer (One) { 0x00 }) } } } } } } --- This almost works. Almost because the I2C device is correctly created, but I have an issue with the GpioInt call which is not properly set by the kernel and which returns -EDEFER. /o\ > > With this all said -- I noticed iasl prints this statement when trying > > to create a node with a lowercase name: > > "At least one lower case letter found in NameSeg, ASL is case > > insensitive - converting to upper case (GPIO)" > > Yes, because it should be in the upper case. > > > I wonder if this suggests that adding a call to toupper() to > > acpi_fwnode_get_named_child_node would be > > an appropriate solution for the node name casing issue.... > > I dunno. You need to ask in the linux-acpi@ mailing list. > To me this is corner case that can't be easily solved > (because two different specifications treat it differently. > > You also need to ask DT people about capital letters there. > And my guts tell me that it's probably also carved in the spec > as "must be lower case" or alike. FWIW while trying to enable this, at some point I named the I2C and the GPIO entries "I2C0" and "GPI0" (with the number '0', not the letter 'o'), and it was not working as you would expect. It is commonly accepted in the ACPI world that the names do not carry meaning AFAICT, and so I think I agree with Andy's initial comment regarding using indexes, not names to also fetch the I2C and GPIO nodes. You can probably have a fallback mechanism for when "i2c" is not present, or simply check if you are in DT or not and use the names only if we are in DT. Thanks a lot to both of you, this will be tremendously helpful to me. Cheers, Benjamin [1] https://www.kernel.org/doc/html/latest/admin-guide/acpi/ssdt-overlays.html#loading-acpi-ssdts-from-initrd
On Tue, Mar 7, 2023 at 7:17 AM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > On Mar 06 2023, Andy Shevchenko wrote: > > On Mon, Mar 06, 2023 at 01:40:16PM -0600, Daniel Kaehn wrote: > > > > ... > > > > > Device (SE9) > > > { > > > Name (_ADR, 0x001D0001) // _ADR: Address > > > Device (RHUB) > > > { > > > Name (_ADR, Zero) > > > Device (CP2) // the USB-hid & CP2112 shared node > > > { > > > Name (_ADR, One) > > > } > > > } > > > } > > > > > > If I'm understanding correctly, this adds the SE9 device as function 1 > > > of PCI device 0x1d, > > > > To be precise this does not add the device. It adds a description of > > the companion device in case the real one will appear on the PCI bus > > with BDF 00:1d.1. > > > > > then RHUB as the USB controller it provides, and finally, CP2 as the > > > USB device attached to port 1 of the controller. > > > > > > With this as the loaded dsdt table, the USB device now has a firmware_node :) > > > #> cat /sys/bus/usb/devices/3-1:1.0/firmware_node/path > > > \_SB_.PCI0.SE9_.RHUB.CP2_ > > > > > > After applying my patches, the HID device also references this node: > > > #> cat /sys/bus/hid/devices/0003:10C4:EA90.0003/firmware_node/path > > > \_SB_.PCI0.SE9_.RHUB.CP2_ > > > > > Great! Thanks a lot for that. Turns out that with both of your inputs I > can also do the same, but without the need for OVMF and DSDT patching, > with just an SSDT override. > Ah, interesting.. I tried the SSDT override route initially, but tried applying it through EFI variables and through configfs, neither of which worked since they appeared to be applied after the relevant drivers were already loaded (at least that was my suspicion). I wonder if loading the overlay through the initramfs was the key. > Turns out that the override documentation [1] mentions "This option > allows loading of user defined SSDTs from initrd and it is useful when > the system does not support EFI or ..." > > FWIW, I am attaching my full DSDT override in case it is valuable: > (on my system, the default USB controller (non-xhc) is at PCI address > 1.2, which explains the slight difference). It can be loaded in the same > way you are overriding the full DSDT, but with just that compilation > output: > > --- > DefinitionBlock ("cp2112.aml", "SSDT", 5, "", "CP2112", 1) > { > External (_SB_.PCI0, DeviceObj) > > Scope (\_SB_.PCI0) > { > Device (USB0) > { > Name (_ADR, 0x00010002) // _ADR: Address > Device (RHUB) > { > Name (_ADR, Zero) > Device (CP21) // the USB-hid & CP2112 shared node > { > Name (_ADR, One) > Device (I2C) > { > Name (_ADR, Zero) > Name (_STA, 0x0F) > } > > Device (GPIO) > { > Name (_ADR, One) > Name (_STA, 0x0F) > } > } > } > } > } To get this to work -- I assume you had to change the driver to look for uppercase "GPIO" and "I2C", or some similar change? > > Scope (\_SB_.PCI0.USB0.RHUB.CP21.I2C) > { > Device (TPD0) > { > Name (_HID, "RMI40001") > Name (_CID, "PNP0C50") > Name (_STA, 0x0F) > > Name (SBFB, ResourceTemplate () > { > I2cSerialBusV2 (0x00c, ControllerInitiated, 100000, > AddressingMode7Bit, "\\_SB_.PCI0.USB0.RHUB.CP21.I2C", > 0x00, ResourceConsumer,, Exclusive, > ) > }) > Name (SBFG, ResourceTemplate () > { > GpioInt (Level, ActiveLow, Exclusive, PullDefault, 0x0000, > "\\_SB_.PCI0.USB0.RHUB.CP21.GPIO", 0x00, ResourceConsumer, , > ) > { // Pin list > 0x0002 > } > }) > Method(_CRS, 0x0, NotSerialized) > { > Return (ConcatenateResTemplate (SBFB, SBFG)) > } > > Method(_DSM, 0x4, Serialized) > { > // DSM UUID > switch (ToBuffer (Arg0)) > { > // ACPI DSM UUID for HIDI2C > case (ToUUID ("3CDFF6F7-4267-4555-AD05-B30A3D8938DE")) > { > // DSM Function > switch (ToInteger (Arg2)) > { > // Function 0: Query function, return based on revision > case(0) > { > // DSM Revision > switch (ToInteger (Arg1)) > { > // Revision 1: Function 1 supported > case (1) > { > Return (Buffer (One) { 0x03 }) > } > > default > { > // Revision 2+: no functions supported > Return (Buffer (One) { 0x00 }) > } > } > } > > // Function 1 : HID Function > case(1) > { > // HID Descriptor Address > Return (0x0020) > } > > default > { > // Functions 2+: not supported > Return (Buffer (One) { 0x00 }) > } > } > } > > default > { > // No other GUIDs supported > Return (Buffer (One) { 0x00 }) > } > } > } > } > } > } > --- > > This almost works. Almost because the I2C device is correctly created, > but I have an issue with the GpioInt call which is not properly set by > the kernel and which returns -EDEFER. /o\ > Ahh, yep, I've had this issue as well -- I suspect the issue you're having is that the CP2112 driver initializes the i2c controller before the gpiochip, and if any i2c devices on the bus depend on the CP2112's gpio, the probe will never succeed! I have made and been testing with a patch to fix this, but since it was midway through submitting this series, thought it might be bad practice to "tack on" additional patches to a patchset mid-review (since it only causes an issue in some (admittedly fairly common) use-cases) so I was going to send it as an individual patch once (if) these were applied. If you think that would be necessary to include for these to merge, I'd be happy to append it to this review. I also have another patch which adds i2c bus recovery to the driver, but that seems independent enough that it should be sent on its own. > > > With this all said -- I noticed iasl prints this statement when trying > > > to create a node with a lowercase name: > > > "At least one lower case letter found in NameSeg, ASL is case > > > insensitive - converting to upper case (GPIO)" > > > > Yes, because it should be in the upper case. > > > > > I wonder if this suggests that adding a call to toupper() to > > > acpi_fwnode_get_named_child_node would be > > > an appropriate solution for the node name casing issue.... > > > > I dunno. You need to ask in the linux-acpi@ mailing list. > > To me this is corner case that can't be easily solved > > (because two different specifications treat it differently. > > > > You also need to ask DT people about capital letters there. > > And my guts tell me that it's probably also carved in the spec > > as "must be lower case" or alike. > > FWIW while trying to enable this, at some point I named the I2C and the > GPIO entries "I2C0" and "GPI0" (with the number '0', not the letter > 'o'), and it was not working as you would expect. > > It is commonly accepted in the ACPI world that the names do not carry > meaning AFAICT, and so I think I agree with Andy's initial comment > regarding using indexes, not names to also fetch the I2C and GPIO nodes. > You can probably have a fallback mechanism for when "i2c" is not > present, or simply check if you are in DT or not and use the names only > if we are in DT. More and more, after actually seeing and working with ACPI, I suspect that you both are right. Maybe (hopefully) though, there is some unified way that can be made to do this, so that individual drivers won't have to directly code for / be aware of the differences in the firmware languages (at least, that seemed to be the intent of the fw_node/device api in the first place). Maybe a `device_get_child_by_name_or_index` (terribly long name) sort of function might fill in that gap? I plan to send an email asking this question more generically to ACPI & DT folks as Andy suggested, so hopefully there will be some ideas. > > Thanks a lot to both of you, this will be tremendously helpful to me. > > Cheers, > Benjamin > Thanks for testing this out! Glad that ACPI support ended up being worked into this after all :)
On Mar 07 2023, Daniel Kaehn wrote: > On Tue, Mar 7, 2023 at 7:17 AM Benjamin Tissoires > <benjamin.tissoires@redhat.com> wrote: > > > > On Mar 06 2023, Andy Shevchenko wrote: > > > On Mon, Mar 06, 2023 at 01:40:16PM -0600, Daniel Kaehn wrote: > > > > > > ... > > > > > > > Device (SE9) > > > > { > > > > Name (_ADR, 0x001D0001) // _ADR: Address > > > > Device (RHUB) > > > > { > > > > Name (_ADR, Zero) > > > > Device (CP2) // the USB-hid & CP2112 shared node > > > > { > > > > Name (_ADR, One) > > > > } > > > > } > > > > } > > > > > > > > If I'm understanding correctly, this adds the SE9 device as function 1 > > > > of PCI device 0x1d, > > > > > > To be precise this does not add the device. It adds a description of > > > the companion device in case the real one will appear on the PCI bus > > > with BDF 00:1d.1. > > > > > > > then RHUB as the USB controller it provides, and finally, CP2 as the > > > > USB device attached to port 1 of the controller. > > > > > > > > With this as the loaded dsdt table, the USB device now has a firmware_node :) > > > > #> cat /sys/bus/usb/devices/3-1:1.0/firmware_node/path > > > > \_SB_.PCI0.SE9_.RHUB.CP2_ > > > > > > > > After applying my patches, the HID device also references this node: > > > > #> cat /sys/bus/hid/devices/0003:10C4:EA90.0003/firmware_node/path > > > > \_SB_.PCI0.SE9_.RHUB.CP2_ > > > > > > > > Great! Thanks a lot for that. Turns out that with both of your inputs I > > can also do the same, but without the need for OVMF and DSDT patching, > > with just an SSDT override. > > > Ah, interesting.. I tried the SSDT override route initially, but tried > applying it > through EFI variables and through configfs, neither of which worked since > they appeared to be applied after the relevant drivers were already loaded > (at least that was my suspicion). I wonder if loading the overlay through the > initramfs was the key. Yeah, there were a few items missing from a "blank" qemu: - loading the SSDT overlay in the initramfs so it's seen before anything else - actually define the description of the device in the DSDT at the matching PCI bus address - your patch :) > > > Turns out that the override documentation [1] mentions "This option > > allows loading of user defined SSDTs from initrd and it is useful when > > the system does not support EFI or ..." > > > > FWIW, I am attaching my full DSDT override in case it is valuable: > > (on my system, the default USB controller (non-xhc) is at PCI address > > 1.2, which explains the slight difference). It can be loaded in the same > > way you are overriding the full DSDT, but with just that compilation > > output: > > > > --- > > DefinitionBlock ("cp2112.aml", "SSDT", 5, "", "CP2112", 1) > > { > > External (_SB_.PCI0, DeviceObj) > > > > Scope (\_SB_.PCI0) > > { > > Device (USB0) > > { > > Name (_ADR, 0x00010002) // _ADR: Address > > Device (RHUB) > > { > > Name (_ADR, Zero) > > Device (CP21) // the USB-hid & CP2112 shared node > > { > > Name (_ADR, One) > > Device (I2C) > > { > > Name (_ADR, Zero) > > Name (_STA, 0x0F) > > } > > > > Device (GPIO) > > { > > Name (_ADR, One) > > Name (_STA, 0x0F) > > } > > } > > } > > } > > } > > To get this to work -- I assume you had to change the driver to look > for uppercase > "GPIO" and "I2C", or some similar change? > > > > > > Scope (\_SB_.PCI0.USB0.RHUB.CP21.I2C) > > { > > Device (TPD0) > > { > > Name (_HID, "RMI40001") > > Name (_CID, "PNP0C50") > > Name (_STA, 0x0F) > > > > Name (SBFB, ResourceTemplate () > > { > > I2cSerialBusV2 (0x00c, ControllerInitiated, 100000, > > AddressingMode7Bit, "\\_SB_.PCI0.USB0.RHUB.CP21.I2C", > > 0x00, ResourceConsumer,, Exclusive, > > ) > > }) > > Name (SBFG, ResourceTemplate () > > { > > GpioInt (Level, ActiveLow, Exclusive, PullDefault, 0x0000, > > "\\_SB_.PCI0.USB0.RHUB.CP21.GPIO", 0x00, ResourceConsumer, , > > ) > > { // Pin list > > 0x0002 > > } > > }) > > Method(_CRS, 0x0, NotSerialized) > > { > > Return (ConcatenateResTemplate (SBFB, SBFG)) > > } > > > > Method(_DSM, 0x4, Serialized) > > { > > // DSM UUID > > switch (ToBuffer (Arg0)) > > { > > // ACPI DSM UUID for HIDI2C > > case (ToUUID ("3CDFF6F7-4267-4555-AD05-B30A3D8938DE")) > > { > > // DSM Function > > switch (ToInteger (Arg2)) > > { > > // Function 0: Query function, return based on revision > > case(0) > > { > > // DSM Revision > > switch (ToInteger (Arg1)) > > { > > // Revision 1: Function 1 supported > > case (1) > > { > > Return (Buffer (One) { 0x03 }) > > } > > > > default > > { > > // Revision 2+: no functions supported > > Return (Buffer (One) { 0x00 }) > > } > > } > > } > > > > // Function 1 : HID Function > > case(1) > > { > > // HID Descriptor Address > > Return (0x0020) > > } > > > > default > > { > > // Functions 2+: not supported > > Return (Buffer (One) { 0x00 }) > > } > > } > > } > > > > default > > { > > // No other GUIDs supported > > Return (Buffer (One) { 0x00 }) > > } > > } > > } > > } > > } > > } > > --- > > > > This almost works. Almost because the I2C device is correctly created, > > but I have an issue with the GpioInt call which is not properly set by > > the kernel and which returns -EDEFER. /o\ > > > > Ahh, yep, I've had this issue as well -- I suspect the issue you're > having is that the > CP2112 driver initializes the i2c controller before the gpiochip, and > if any i2c devices > on the bus depend on the CP2112's gpio, the probe will never succeed! > I have made > and been testing with a patch to fix this, but since it was midway > through submitting > this series, thought it might be bad practice to "tack on" additional > patches to a patchset > mid-review (since it only causes an issue in some (admittedly fairly > common) use-cases) > so I was going to send it as an individual patch once (if) these were applied. > I don't think this is the issue. When the device is initially probed, the I2C acpi implementation tries to attach an IRQ, but failed at it, returning -EPROBE_DEFER, which makes the device being retried a few times. After I get my shell available I even get the pr_err I included few seconds later, for a last attempt by the kernel to bind it when everything has settled. So I can see that the device gets probed, and that all ACPI resources are tried to get the IRQ. Right now, I see that it's attempting to bind to the acpi resource in acpi_dev_resource_interrupt() (in file drivers/acpi/resources.c), but instead of having a ACPI_RESOURCE_TYPE_EXTENDED_IRQ I only get a ACPI_RESOURCE_TYPE_GPIO for the GpioInt() definition in the _CRS method. So I am missing the proper transition from GpioInt to IRQ in the acpi. Note that I tried applying what was describe at Documentation/firmware_guide/acpi/gpio-properties.rst but the _DSD method doesn't seem to be properly applied to the CP2112 GPIO, which is highly suspicious. > If you think that would be necessary to include for these to merge, > I'd be happy to append > it to this review. I also have another patch which adds i2c bus > recovery to the driver, but > that seems independent enough that it should be sent on its own. As I mentioned above I don't think the issue is in the ordering of the I2C vs gpio resources. > > > > > With this all said -- I noticed iasl prints this statement when trying > > > > to create a node with a lowercase name: > > > > "At least one lower case letter found in NameSeg, ASL is case > > > > insensitive - converting to upper case (GPIO)" > > > > > > Yes, because it should be in the upper case. > > > > > > > I wonder if this suggests that adding a call to toupper() to > > > > acpi_fwnode_get_named_child_node would be > > > > an appropriate solution for the node name casing issue.... > > > > > > I dunno. You need to ask in the linux-acpi@ mailing list. > > > To me this is corner case that can't be easily solved > > > (because two different specifications treat it differently. > > > > > > You also need to ask DT people about capital letters there. > > > And my guts tell me that it's probably also carved in the spec > > > as "must be lower case" or alike. > > > > FWIW while trying to enable this, at some point I named the I2C and the > > GPIO entries "I2C0" and "GPI0" (with the number '0', not the letter > > 'o'), and it was not working as you would expect. > > > > It is commonly accepted in the ACPI world that the names do not carry > > meaning AFAICT, and so I think I agree with Andy's initial comment > > regarding using indexes, not names to also fetch the I2C and GPIO nodes. > > You can probably have a fallback mechanism for when "i2c" is not > > present, or simply check if you are in DT or not and use the names only > > if we are in DT. > > More and more, after actually seeing and working with ACPI, I suspect that > you both are right. Maybe (hopefully) though, there is some unified way that can > be made to do this, so that individual drivers won't have to directly code for / > be aware of the differences in the firmware languages (at least, that seemed > to be the intent of the fw_node/device api in the first place). Maybe a > `device_get_child_by_name_or_index` (terribly long name) sort of function > might fill in that gap? I don't know. Though the _DSD documentation I mentioned above looks very similar to what you are describing in the DT case, so maybe the ACPI folks will just tell us "why don't you use XXXX?" and we will have the solution :) (we can't be the first to have that same issue TBH) > > I plan to send an email asking this question more generically to ACPI & DT folks > as Andy suggested, so hopefully there will be some ideas. > > > > > Thanks a lot to both of you, this will be tremendously helpful to me. > > > > Cheers, > > Benjamin > > > Thanks for testing this out! Glad that ACPI support ended up being worked into > this after all :) Cheers, Benjamin
On Tue, Mar 07, 2023 at 02:17:06PM +0100, Benjamin Tissoires wrote: > On Mar 06 2023, Andy Shevchenko wrote: > > On Mon, Mar 06, 2023 at 01:40:16PM -0600, Daniel Kaehn wrote: ... > This almost works. Almost because the I2C device is correctly created, > but I have an issue with the GpioInt call which is not properly set by > the kernel and which returns -EDEFER. /o\ You can try to utilize _DEP method, but I dunno if it implemented that way in the Linux that helps your case. > > > With this all said -- I noticed iasl prints this statement when trying > > > to create a node with a lowercase name: > > > "At least one lower case letter found in NameSeg, ASL is case > > > insensitive - converting to upper case (GPIO)" > > > > Yes, because it should be in the upper case. > > > > > I wonder if this suggests that adding a call to toupper() to > > > acpi_fwnode_get_named_child_node would be > > > an appropriate solution for the node name casing issue.... > > > > I dunno. You need to ask in the linux-acpi@ mailing list. > > To me this is corner case that can't be easily solved > > (because two different specifications treat it differently. > > > > You also need to ask DT people about capital letters there. > > And my guts tell me that it's probably also carved in the spec > > as "must be lower case" or alike. > > FWIW while trying to enable this, at some point I named the I2C and the > GPIO entries "I2C0" and "GPI0" (with the number '0', not the letter > 'o'), and it was not working as you would expect. > > It is commonly accepted in the ACPI world that the names do not carry > meaning AFAICT, and so I think I agree with Andy's initial comment > regarding using indexes, not names to also fetch the I2C and GPIO nodes. > You can probably have a fallback mechanism for when "i2c" is not > present, or simply check if you are in DT or not and use the names only > if we are in DT. The solution is to provide in the main node the list of cell names, that way you will always know the indices: Device (DEV) { _DSD "cell-names" { "i2c", "gpio" } // index of the name is the // index of the cell Device (I2C0) { } Device (GPI0) { } } Problem solved. > Thanks a lot to both of you, this will be tremendously helpful to me. You're welcome! > [1] https://www.kernel.org/doc/html/latest/admin-guide/acpi/ssdt-overlays.html#loading-acpi-ssdts-from-initrd
On Tue, Mar 07, 2023 at 07:53:20AM -0600, Daniel Kaehn wrote: > On Tue, Mar 7, 2023 at 7:17 AM Benjamin Tissoires > <benjamin.tissoires@redhat.com> wrote: ... > More and more, after actually seeing and working with ACPI, I suspect that > you both are right. Maybe (hopefully) though, there is some unified way that can > be made to do this, so that individual drivers won't have to directly code for / > be aware of the differences in the firmware languages (at least, that seemed > to be the intent of the fw_node/device api in the first place). Maybe a > `device_get_child_by_name_or_index` (terribly long name) sort of function > might fill in that gap? Look also at the solution I proposed in response to Benjamin in my previous email.
On Tue, Mar 07, 2023 at 03:48:52PM +0100, Benjamin Tissoires wrote: > On Mar 07 2023, Daniel Kaehn wrote: ... > So I can see that the device gets probed, and that all ACPI resources > are tried to get the IRQ. > Right now, I see that it's attempting to bind to the acpi resource in > acpi_dev_resource_interrupt() (in file drivers/acpi/resources.c), but > instead of having a ACPI_RESOURCE_TYPE_EXTENDED_IRQ I only get a > ACPI_RESOURCE_TYPE_GPIO for the GpioInt() definition in the _CRS method. > > So I am missing the proper transition from GpioInt to IRQ in the acpi. I'm not sure I understand what this means. The Linux kernel takes either Interrupt() resource (which is IOxAPIC / GIC / etc) or GpioInt() (which is GPIO based). In both cases I²C framework submits this into client's IRQ field.
On Tue, Mar 7, 2023 at 12:14 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Tue, Mar 07, 2023 at 02:17:06PM +0100, Benjamin Tissoires wrote: > > On Mar 06 2023, Andy Shevchenko wrote: > > It is commonly accepted in the ACPI world that the names do not carry > > meaning AFAICT, and so I think I agree with Andy's initial comment > > regarding using indexes, not names to also fetch the I2C and GPIO nodes. > > You can probably have a fallback mechanism for when "i2c" is not > > present, or simply check if you are in DT or not and use the names only > > if we are in DT. > > The solution is to provide in the main node the list of cell names, that way > you will always know the indices: > > Device (DEV) { > _DSD > "cell-names" { "i2c", "gpio" } // index of the name is the > // index of the cell > > Device (I2C0) { > } > > Device (GPI0) { > } > } > > Problem solved. > Just to make sure I'm understanding you correctly: Are you proposing that specifically this driver directly reads "cell-names" from the ACPI DSD to implement this indexing? Or are you proposing a kernel-wide mechanism of "overriding" a fwnode name with ACPI? (assuming this doesn't exist already, and I'm not just missing it in the kernel source) Or are you proposing something else entirely? (apologies if this should be obvious -- throwing up the ACPI newbie card again here :) ) In any case, would this be something I should post to the email chain with DT and ACPI folks for opinions before I start to implement it? Thanks, Danny Kaehn
On Tue, Mar 07, 2023 at 01:57:27PM -0600, Daniel Kaehn wrote: > On Tue, Mar 7, 2023 at 12:14 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Tue, Mar 07, 2023 at 02:17:06PM +0100, Benjamin Tissoires wrote: > > > On Mar 06 2023, Andy Shevchenko wrote: ... > > > It is commonly accepted in the ACPI world that the names do not carry > > > meaning AFAICT, and so I think I agree with Andy's initial comment > > > regarding using indexes, not names to also fetch the I2C and GPIO nodes. > > > You can probably have a fallback mechanism for when "i2c" is not > > > present, or simply check if you are in DT or not and use the names only > > > if we are in DT. > > > > The solution is to provide in the main node the list of cell names, that way > > you will always know the indices: > > > > Device (DEV) { > > _DSD > > "cell-names" { "i2c", "gpio" } // index of the name is the > > // index of the cell > > > > Device (I2C0) { > > } > > > > Device (GPI0) { > > } > > } > > > > Problem solved. > > Just to make sure I'm understanding you correctly: > > Are you proposing that specifically this driver directly reads "cell-names" > from the ACPI DSD to implement this indexing? Or are you proposing a > kernel-wide mechanism of "overriding" a fwnode name with ACPI? > (assuming this doesn't exist already, and I'm not just missing it in > the kernel source) > > Or are you proposing something else entirely? > (apologies if this should be obvious -- throwing up the ACPI newbie > card again here :) ) Out of the three listed above I'm proposing the first one. (Maybe it can be called 'reg-names', but it depends on the hardware) Also look into hierarchical _DSD (it is similar to the children of the device node without being separate devices). > In any case, would this be something I should post to the email chain > with DT and ACPI > folks for opinions before I start to implement it? May be, the -names approach is already used widely for clock, interrupt, reset, phy and nvmem. So it's something we know about.
On Mar 07 2023, Andy Shevchenko wrote: > On Tue, Mar 07, 2023 at 03:48:52PM +0100, Benjamin Tissoires wrote: > > On Mar 07 2023, Daniel Kaehn wrote: > > ... > > > So I can see that the device gets probed, and that all ACPI resources > > are tried to get the IRQ. > > Right now, I see that it's attempting to bind to the acpi resource in > > acpi_dev_resource_interrupt() (in file drivers/acpi/resources.c), but > > instead of having a ACPI_RESOURCE_TYPE_EXTENDED_IRQ I only get a > > ACPI_RESOURCE_TYPE_GPIO for the GpioInt() definition in the _CRS method. > > > > So I am missing the proper transition from GpioInt to IRQ in the acpi. > > I'm not sure I understand what this means. > > The Linux kernel takes either Interrupt() resource (which is > IOxAPIC / GIC / etc) or GpioInt() (which is GPIO based). > > In both cases I²C framework submits this into client's IRQ field. > I finally managed to get past the retrieval of the GpioInt. Turns out that the function acpi_get_gpiod() matches on the parent of the gpio_chip (gc->parent), which means that, with the current code and my SSDT, it matches on the HID CP2112 ACPI node, not the GPIO one. For reference (with lots of boiler plate removed): Device (CP21) { // the USB-hid & CP2112 shared node Device (GPIO) { Name (_ADR, One) Name (_STA, 0x0F) Name (_DSD, Package () { ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), Package () { Package () { "gpio-line-names", Package () { "", "", "irq-rmi4", "", "power", // set to 1 with gpio-hog above "", "", "", ""}}, } }) } Scope (\_SB_.PCI0.USB0.RHUB.CP21.I2C) { Device (TPD0) { Name (_HID, "RMI40001") Name (_CID, "PNP0C50") Name (_STA, 0x0F) Name (SBFB, ResourceTemplate () { I2cSerialBusV2 (0x00c, ControllerInitiated, 100000, AddressingMode7Bit, "\\_SB_.PCI0.USB0.RHUB.CP21.I2C", 0x00, ResourceConsumer,, Exclusive, ) }) Name (SBFG, ResourceTemplate () { GpioInt (Level, ActiveLow, Exclusive, PullDefault, 0x0000, "\\_SB_.PCI0.USB0.RHUB.CP21", 0x00, ResourceConsumer, , ) { // Pin list 0x0002 } }) --- But if I refer "\\_SB_.PCI0.USB0.RHUB.CP21.GPIO", the IRQ is never assigned. With the parent (CP21), it works. So I wonder if the cp2112 driver is correctly assigning the gc->parent field. Cheers, Benjamin
On Wed, Mar 8, 2023 at 9:26 AM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > On Mar 07 2023, Andy Shevchenko wrote: > > On Tue, Mar 07, 2023 at 03:48:52PM +0100, Benjamin Tissoires wrote: > > > On Mar 07 2023, Daniel Kaehn wrote: > > > > ... > > > > > So I can see that the device gets probed, and that all ACPI resources > > > are tried to get the IRQ. > > > Right now, I see that it's attempting to bind to the acpi resource in > > > acpi_dev_resource_interrupt() (in file drivers/acpi/resources.c), but > > > instead of having a ACPI_RESOURCE_TYPE_EXTENDED_IRQ I only get a > > > ACPI_RESOURCE_TYPE_GPIO for the GpioInt() definition in the _CRS method. > > > > > > So I am missing the proper transition from GpioInt to IRQ in the acpi. > > > > I'm not sure I understand what this means. > > > > The Linux kernel takes either Interrupt() resource (which is > > IOxAPIC / GIC / etc) or GpioInt() (which is GPIO based). > > > > In both cases I²C framework submits this into client's IRQ field. > > > > I finally managed to get past the retrieval of the GpioInt. > > Turns out that the function acpi_get_gpiod() matches on the parent of > the gpio_chip (gc->parent), which means that, with the current code and > my SSDT, it matches on the HID CP2112 ACPI node, not the GPIO one. > > For reference (with lots of boiler plate removed): > > Device (CP21) { // the USB-hid & CP2112 shared node > Device (GPIO) { > Name (_ADR, One) > Name (_STA, 0x0F) > > Name (_DSD, Package () { > ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), > Package () { > Package () { "gpio-line-names", Package () { > "", > "", > "irq-rmi4", > "", > "power", // set to 1 with gpio-hog above > "", > "", > "", > ""}}, > } > }) > } > > Scope (\_SB_.PCI0.USB0.RHUB.CP21.I2C) > { > Device (TPD0) > { > Name (_HID, "RMI40001") > Name (_CID, "PNP0C50") > Name (_STA, 0x0F) > > Name (SBFB, ResourceTemplate () > { > I2cSerialBusV2 (0x00c, ControllerInitiated, 100000, > AddressingMode7Bit, "\\_SB_.PCI0.USB0.RHUB.CP21.I2C", > 0x00, ResourceConsumer,, Exclusive, > ) > }) > Name (SBFG, ResourceTemplate () > { > GpioInt (Level, ActiveLow, Exclusive, PullDefault, 0x0000, > "\\_SB_.PCI0.USB0.RHUB.CP21", 0x00, ResourceConsumer, , > ) > { // Pin list > 0x0002 > } > }) > --- > > But if I refer "\\_SB_.PCI0.USB0.RHUB.CP21.GPIO", the IRQ is never assigned. > With the parent (CP21), it works. > > So I wonder if the cp2112 driver is correctly assigning the gc->parent > field. Did you make a change to the CP2112 driver patch to look for uppercase "I2C" and "GPIO"? Otherwise, it won't assign those child nodes appropriately, and the gpiochip code will use the parent node by default if the gpiochip's fwnode isn't assigned (I believe). If that was indeed your problem all along (and I'm not missing something else), sorry about that -- I made a comment above, but didn't add much spacing around it to make it stand out (since I noticed you didn't reply to that part in your response) > To get this to work -- I assume you had to change the driver to look > for uppercase > "GPIO" and "I2C", or some similar change? Thanks, Danny Kaehn
On Mar 08 2023, Daniel Kaehn wrote: > On Wed, Mar 8, 2023 at 9:26 AM Benjamin Tissoires > <benjamin.tissoires@redhat.com> wrote: > > But if I refer "\\_SB_.PCI0.USB0.RHUB.CP21.GPIO", the IRQ is never assigned. > > With the parent (CP21), it works. > > > > So I wonder if the cp2112 driver is correctly assigning the gc->parent > > field. > > > Did you make a change to the CP2112 driver patch to look for uppercase > "I2C" and "GPIO"? yes, sorry I should have mentioned it. This is the only modification I have compared to the upstream kernel plus your patch series. > Otherwise, it won't assign those child nodes appropriately, and the > gpiochip code will use > the parent node by default if the gpiochip's fwnode isn't assigned (I believe). I don't think it's a fwnode issue, but a problem with the assignment of the parent of the gc: --- dev->gc.parent = &hdev->dev; --- Because the function acpi_gpiochip_find() in drivers/gpio/gpiolib-acpi.c compares the acpi handle returned by fetching the ACPI path ("\\_SB_.PCI0.USB0.RHUB.CP21.GPIO") and the one of gc->parent, which in the hid-cp2112 case is the HID device itself. > > If that was indeed your problem all along (and I'm not missing > something else), sorry about that -- > I made a comment above, but didn't add much spacing around it to make > it stand out (since I noticed you didn't reply to that part in your response) Yeah, sorry I should have been explicit about this. For reference, I am appending the full SSDT override which works. Even if you don't have an i2c-hid device connected, this should at least call the probe function in i2c-hid-core.c, which is a proof that the ACPI binding is properly done (the first SMBus read will fail with a timeout) Also, I played around with the _DSD that Andy was mentioning (and some others), hopefully this will help you getting the mapping from the "cell-names" to the fwnode child index faster. Cheers, Benjamin --- DefinitionBlock ("cp2112.aml", "SSDT", 5, "", "CP2112", 1) { External (_SB_.PCI0, DeviceObj) Scope (\_SB_.PCI0) { Device (USB0) { Name (_ADR, 0x00010002) // _ADR: Address Device (RHUB) { Name (_ADR, Zero) Device (CP21) // the USB-hid & CP2112 shared node { Name (_ADR, One) Name (_DSD, Package () { ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), Package () { Package () { "cell-names", Package () { "i2c", "gpio", } } } }) Device (I2C) { Name (_ADR, Zero) Name (_STA, 0x0F) } Device (GPIO) { Name (_ADR, One) Name (_STA, 0x0F) Name (_DSD, Package () { ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), Package () { Package () { "gpio-hog", 1 }, Package () { "gpios", Package () { 4, 0 } }, Package () { "output-high", 1 }, Package () { "line-name", "gpio4-pullup" }, }, ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), Package () { Package () { "gpio-line-names", Package () { "", "", "irq-rmi4", "", "power", // set to 1 with gpio-hog above "", "", "", ""}}, } }) } } } } } Scope (\_SB_.PCI0.USB0.RHUB.CP21.I2C) { Device (TPD0) { Name (_HID, "RMI40001") Name (_CID, "PNP0C50") Name (_STA, 0x0F) Name (SBFB, ResourceTemplate () { I2cSerialBusV2 (0x2c, ControllerInitiated, 100000, AddressingMode7Bit, "\\_SB_.PCI0.USB0.RHUB.CP21.I2C", 0x00, ResourceConsumer,, Exclusive, ) }) Name (SBFG, ResourceTemplate () { GpioInt (Level, ActiveLow, Exclusive, PullDefault, 0x0000, "\\_SB_.PCI0.USB0.RHUB.CP21", 0x00, ResourceConsumer, , ) { // Pin list 0x0002 } }) Name (_DSD, Package () { ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), Package () { Package () { "irq-gpios", Package () { ^TPD0, 1, 0, 0 } }, } }) Method(_CRS, 0x0, NotSerialized) { Return (ConcatenateResTemplate (SBFG, SBFB)) } Method(_DSM, 0x4, Serialized) { // DSM UUID switch (ToBuffer (Arg0)) { // ACPI DSM UUID for HIDI2C case (ToUUID ("3CDFF6F7-4267-4555-AD05-B30A3D8938DE")) { // DSM Function switch (ToInteger (Arg2)) { // Function 0: Query function, return based on revision case(0) { // DSM Revision switch (ToInteger (Arg1)) { // Revision 1: Function 1 supported case (1) { Return (Buffer (One) { 0x03 }) } default { // Revision 2+: no functions supported Return (Buffer (One) { 0x00 }) } } } // Function 1 : HID Function case(1) { // HID Descriptor Address Return (0x0020) } default { // Functions 2+: not supported Return (Buffer (One) { 0x00 }) } } } default { // No other GUIDs supported Return (Buffer (One) { 0x00 }) } } } } } } ---
On Wed, Mar 08, 2023 at 04:26:11PM +0100, Benjamin Tissoires wrote: > On Mar 07 2023, Andy Shevchenko wrote: > > On Tue, Mar 07, 2023 at 03:48:52PM +0100, Benjamin Tissoires wrote: > > > On Mar 07 2023, Daniel Kaehn wrote: ... > So I wonder if the cp2112 driver is correctly assigning the gc->parent > field. Seems it needs custom fwnode gc->fwnode = child_of_cp_which_is_gpio;
On Wed, Mar 08, 2023 at 04:55:27PM +0100, Benjamin Tissoires wrote: > On Mar 08 2023, Daniel Kaehn wrote: > > On Wed, Mar 8, 2023 at 9:26 AM Benjamin Tissoires > > <benjamin.tissoires@redhat.com> wrote: > > > But if I refer "\\_SB_.PCI0.USB0.RHUB.CP21.GPIO", the IRQ is never assigned. > > > With the parent (CP21), it works. > > > > > > So I wonder if the cp2112 driver is correctly assigning the gc->parent > > > field. > > Did you make a change to the CP2112 driver patch to look for uppercase > > "I2C" and "GPIO"? > > yes, sorry I should have mentioned it. This is the only modification I > have compared to the upstream kernel plus your patch series. > > > Otherwise, it won't assign those child nodes appropriately, and the > > gpiochip code will use > > the parent node by default if the gpiochip's fwnode isn't assigned (I believe). > > I don't think it's a fwnode issue, but a problem with the assignment of > the parent of the gc: > --- > dev->gc.parent = &hdev->dev; > --- I don't think so. The parent should point to the _physical_ device, which is CP2112, which is correct in my opinion. > Because the function acpi_gpiochip_find() in drivers/gpio/gpiolib-acpi.c > compares the acpi handle returned by fetching the ACPI path > ("\\_SB_.PCI0.USB0.RHUB.CP21.GPIO") and the one of gc->parent, which in > the hid-cp2112 case is the HID device itself. We have specifically gc->fwnode for cases like this. ... > Device (CP21) // the USB-hid & CP2112 shared node > { > Name (_ADR, One) > Name (_DSD, Package () > { > ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), > Package () { > Package () { "cell-names", Package () { "i2c", "gpio" } > } Yeah, looking at this, I think it still fragile. First of all, either this is missing, or simply wrong. We would need to access indices. ACPI _ADR is in the specification. As much as with PCI it may be considered reliable. So, that said, forget about it, and simply use _ADR as indicator of the node. See how MFD (in the Linux kernel) cares about this. Ex. Diolan DLN-2 driver. > }) > > Device (I2C) > { > Name (_ADR, Zero) > Name (_STA, 0x0F) > } > > Device (GPIO) > { > Name (_ADR, One) > Name (_STA, 0x0F) > > Name (_DSD, Package () { > ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), > Package () { > Package () { "gpio-hog", 1 }, > Package () { "gpios", Package () { 4, 0 } }, > Package () { "output-high", 1 }, > Package () { "line-name", "gpio4-pullup" }, > }, > ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), > Package () { > Package () { "gpio-line-names", Package () { > "", > "", > "irq-rmi4", > "", > "power", // set to 1 with gpio-hog above > "", > "", > "", > ""}}, > } > }) > } > }
On Wed, Mar 08, 2023 at 06:30:46PM +0200, Andy Shevchenko wrote: > On Wed, Mar 08, 2023 at 04:55:27PM +0100, Benjamin Tissoires wrote: > > On Mar 08 2023, Daniel Kaehn wrote: > > > On Wed, Mar 8, 2023 at 9:26 AM Benjamin Tissoires > > > <benjamin.tissoires@redhat.com> wrote: ... > > ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), > > Package () { > > Package () { "cell-names", Package () { "i2c", "gpio" } > > } > > Yeah, looking at this, I think it still fragile. First of all, either this is > missing, or simply wrong. We would need to access indices. ACPI _ADR is in the > specification. As much as with PCI it may be considered reliable. > > So, that said, forget about it, and simply use _ADR as indicator of the node. > See how MFD (in the Linux kernel) cares about this. Ex. Diolan DLN-2 driver. And that said, maybe CP2112 should simply re-use what MFD _already_ provides?
On Wed, Mar 8, 2023 at 10:36 AM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Wed, Mar 08, 2023 at 06:30:46PM +0200, Andy Shevchenko wrote: > > On Wed, Mar 08, 2023 at 04:55:27PM +0100, Benjamin Tissoires wrote: > > > On Mar 08 2023, Daniel Kaehn wrote: > > > > On Wed, Mar 8, 2023 at 9:26 AM Benjamin Tissoires > > > > <benjamin.tissoires@redhat.com> wrote: > > ... > > > > ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), > > > Package () { > > > Package () { "cell-names", Package () { "i2c", "gpio" } > > > } > > > > Yeah, looking at this, I think it still fragile. First of all, either this is > > missing, or simply wrong. We would need to access indices. ACPI _ADR is in the > > specification. As much as with PCI it may be considered reliable. > > > > So, that said, forget about it, and simply use _ADR as indicator of the node. > > See how MFD (in the Linux kernel) cares about this. Ex. Diolan DLN-2 driver. > > And that said, maybe CP2112 should simply re-use what MFD _already_ provides? Great point -- it definitely seems like this driver belongs in the mfd directory to begin with. It seems like aside from rewriting the CP2112 driver into an mfd driver and two platform drivers, my route forward for now would be to just do something like this (not yet tested): + struct acpi_device *adev = ACPI_COMPANION(&hdev->dev); + if (adev) + ACPI_COMPANION_SET(&dev->adap.dev, acpi_find_child_device(adev, 0x0, false)); + else + device_set_node(&dev->adap.dev, device_get_named_child_node(&hdev->dev, "i2c")); (and the same for the gpiochip) The follow-up question -- does there exist something analogous to DT bindings for ACPI devices, other than the ACPI spec itself, where this should be documented? Or will consumers truly have to read the driver code to determine that _ADR 0 is I2C and _ADR 1 is GPIO? (I haven't seen anything in my search so far -- but knowing that it truly doesn't exist would make me respect people developing embedded ACPI-based systems all the more!) Thanks for your patience in working through all of this, especially considering how long of an email chain this has become! Thanks, Danny Kaehn
On Mar 08 2023, Andy Shevchenko wrote: > On Wed, Mar 08, 2023 at 04:55:27PM +0100, Benjamin Tissoires wrote: > > On Mar 08 2023, Daniel Kaehn wrote: > > > On Wed, Mar 8, 2023 at 9:26 AM Benjamin Tissoires > > > <benjamin.tissoires@redhat.com> wrote: > > > > But if I refer "\\_SB_.PCI0.USB0.RHUB.CP21.GPIO", the IRQ is never assigned. > > > > With the parent (CP21), it works. > > > > > > > > So I wonder if the cp2112 driver is correctly assigning the gc->parent > > > > field. > > > > Did you make a change to the CP2112 driver patch to look for uppercase > > > "I2C" and "GPIO"? > > > > yes, sorry I should have mentioned it. This is the only modification I > > have compared to the upstream kernel plus your patch series. > > > > > Otherwise, it won't assign those child nodes appropriately, and the > > > gpiochip code will use > > > the parent node by default if the gpiochip's fwnode isn't assigned (I believe). > > > > I don't think it's a fwnode issue, but a problem with the assignment of > > the parent of the gc: > > --- > > dev->gc.parent = &hdev->dev; > > --- > > I don't think so. The parent should point to the _physical_ device, which is > CP2112, which is correct in my opinion. > Right. I tend to agree, and then the problem seems to be relying in gpiolib-acpi.c > > Because the function acpi_gpiochip_find() in drivers/gpio/gpiolib-acpi.c > > compares the acpi handle returned by fetching the ACPI path > > ("\\_SB_.PCI0.USB0.RHUB.CP21.GPIO") and the one of gc->parent, which in > > the hid-cp2112 case is the HID device itself. > > We have specifically gc->fwnode for cases like this. Looks like gpiolib-acpi.c doesn't care about fwnode at all. if I do the following: --- diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c index d8a421ce26a8..5aebc266426b 100644 --- a/drivers/gpio/gpiolib-acpi.c +++ b/drivers/gpio/gpiolib-acpi.c @@ -126,7 +126,7 @@ static bool acpi_gpio_deferred_req_irqs_done; static int acpi_gpiochip_find(struct gpio_chip *gc, void *data) { - return gc->parent && device_match_acpi_handle(gc->parent, data); + return ACPI_HANDLE_FWNODE(gc->fwnode) == data; } /** --- I can now directly reference the GPIO ACPI node in my GpioInt() declaration. And AFAICT this should be safe to do because gpiolib ensure that gc->fwnode is set, using the one from the parent if it is not set previously. I need to check if this works with my icelake laptop, and if so I'll send it to the list. The reason the intel gpios are working (the only one I checked) is because the \\SB.GPI0 node refers to the pinctrl controller (driver pinctrl-icelake.c in my case, which then creates a subdevice for handling the gpio). > > ... > Cheers, Benjamin
On Wed, Mar 08, 2023 at 12:32:07PM -0600, Daniel Kaehn wrote: > On Wed, Mar 8, 2023 at 10:36 AM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Wed, Mar 08, 2023 at 06:30:46PM +0200, Andy Shevchenko wrote: > > > On Wed, Mar 08, 2023 at 04:55:27PM +0100, Benjamin Tissoires wrote: > > > > On Mar 08 2023, Daniel Kaehn wrote: > > > > > On Wed, Mar 8, 2023 at 9:26 AM Benjamin Tissoires > > > > > <benjamin.tissoires@redhat.com> wrote: ... > > > > ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), > > > > Package () { > > > > Package () { "cell-names", Package () { "i2c", "gpio" } > > > > } > > > > > > Yeah, looking at this, I think it still fragile. First of all, either this is > > > missing, or simply wrong. We would need to access indices. ACPI _ADR is in the > > > specification. As much as with PCI it may be considered reliable. > > > > > > So, that said, forget about it, and simply use _ADR as indicator of the node. > > > See how MFD (in the Linux kernel) cares about this. Ex. Diolan DLN-2 driver. > > > > And that said, maybe CP2112 should simply re-use what MFD _already_ provides? > > Great point -- it definitely seems like this driver belongs in the mfd > directory to begin with. It can be iteratively converted later on. > It seems like aside from rewriting the CP2112 driver into an mfd > driver and two platform drivers, > my route forward for now would be to just do something like this (not > yet tested): > > + struct acpi_device *adev = ACPI_COMPANION(&hdev->dev); > + if (adev) > + ACPI_COMPANION_SET(&dev->adap.dev, acpi_find_child_device(adev, > 0x0, false)); ACPI_COMPANION_SET() is something different to simple device_set_node(). I would expect that in this driver we simply use the child fwnode as is. But since you are not using so called secondary fwnode, I believe it's fine for now. > + else > + device_set_node(&dev->adap.dev, > device_get_named_child_node(&hdev->dev, "i2c")); > > (and the same for the gpiochip) > > The follow-up question -- does there exist something analogous to DT > bindings for ACPI devices, > other than the ACPI spec itself, where this should be documented? Or > will consumers truly have to > read the driver code to determine that _ADR 0 is I2C and _ADR 1 is > GPIO? (I haven't seen anything > in my search so far -- but knowing that it truly doesn't exist would > make me respect people developing > embedded ACPI-based systems all the more!) See how the acpi_get_local_address() is used in the 3 users of it. Ideally we need a new callback in the fwnode ops to return either (least) 32-bit of _ADR or "reg" property. Dunno, if "reg" is actually what suits here. That said, I would do something like (pseudo-code) device_for_each_child_node() { if (name == $NAME) $NAME->fwnode = child; else if (_ADR = $INDEX) $NAME->fwnode = child; } > Thanks for your patience in working through all of this, especially > considering how long of an email > chain this has become! You're welcome!
On Thu, Mar 09, 2023 at 01:43:02PM +0200, Andy Shevchenko wrote: > On Wed, Mar 08, 2023 at 12:32:07PM -0600, Daniel Kaehn wrote: > > On Wed, Mar 8, 2023 at 10:36 AM Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > > On Wed, Mar 08, 2023 at 06:30:46PM +0200, Andy Shevchenko wrote: > > > > On Wed, Mar 08, 2023 at 04:55:27PM +0100, Benjamin Tissoires wrote: > > > > > On Mar 08 2023, Daniel Kaehn wrote: > > > > > > On Wed, Mar 8, 2023 at 9:26 AM Benjamin Tissoires > > > > > > <benjamin.tissoires@redhat.com> wrote: ... > > > > > ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), > > > > > Package () { > > > > > Package () { "cell-names", Package () { "i2c", "gpio" } > > > > > } > > > > > > > > Yeah, looking at this, I think it still fragile. First of all, either this is > > > > missing, or simply wrong. We would need to access indices. ACPI _ADR is in the > > > > specification. As much as with PCI it may be considered reliable. > > > > > > > > So, that said, forget about it, and simply use _ADR as indicator of the node. > > > > See how MFD (in the Linux kernel) cares about this. Ex. Diolan DLN-2 driver. > > > > > > And that said, maybe CP2112 should simply re-use what MFD _already_ provides? > > > > Great point -- it definitely seems like this driver belongs in the mfd > > directory to begin with. > > It can be iteratively converted later on. > > > It seems like aside from rewriting the CP2112 driver into an mfd > > driver and two platform drivers, > > my route forward for now would be to just do something like this (not > > yet tested): > > > > + struct acpi_device *adev = ACPI_COMPANION(&hdev->dev); > > + if (adev) > > + ACPI_COMPANION_SET(&dev->adap.dev, acpi_find_child_device(adev, > > 0x0, false)); > > ACPI_COMPANION_SET() is something different to simple device_set_node(). > I would expect that in this driver we simply use the child fwnode as is. > But since you are not using so called secondary fwnode, I believe it's > fine for now. > > > + else > > + device_set_node(&dev->adap.dev, > > device_get_named_child_node(&hdev->dev, "i2c")); > > > > (and the same for the gpiochip) > > The follow-up question -- does there exist something analogous to DT > > bindings for ACPI devices, > > other than the ACPI spec itself, where this should be documented? Or > > will consumers truly have to > > read the driver code to determine that _ADR 0 is I2C and _ADR 1 is > > GPIO? (I haven't seen anything > > in my search so far -- but knowing that it truly doesn't exist would > > make me respect people developing > > embedded ACPI-based systems all the more!) The below misplaced, so here is the answer to the followup. The _DSD heavily relies on the DT schemas and other standards such as MIPI. For many cases there are no standards or any developed approaches. Feel free to add a piece of documentation for the devices that are utilising _ADR in ACPI (we have at least I²C/GPIO controller on Intel Quark — drivers/mfd/intel_quark_i2c_gpio.c, and mentioned earlier the Diolan DLN-2 — drivers/mfd/dln2.c). > See how the acpi_get_local_address() is used in the 3 users of it. > > Ideally we need a new callback in the fwnode ops to return either > (least) 32-bit of _ADR or "reg" property. > > Dunno, if "reg" is actually what suits here. > > That said, I would do something like (pseudo-code) > > device_for_each_child_node() { > if (name == $NAME) > $NAME->fwnode = child; > else if (_ADR = $INDEX) > $NAME->fwnode = child; > } > > > Thanks for your patience in working through all of this, especially > > considering how long of an email > > chain this has become! > > You're welcome!
On Thu, Mar 09, 2023 at 10:38:11AM +0100, Benjamin Tissoires wrote: > On Mar 08 2023, Andy Shevchenko wrote: ... > Looks like gpiolib-acpi.c doesn't care about fwnode at all. > > if I do the following: > > --- > diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c > index d8a421ce26a8..5aebc266426b 100644 > --- a/drivers/gpio/gpiolib-acpi.c > +++ b/drivers/gpio/gpiolib-acpi.c > @@ -126,7 +126,7 @@ static bool acpi_gpio_deferred_req_irqs_done; > > static int acpi_gpiochip_find(struct gpio_chip *gc, void *data) > { > - return gc->parent && device_match_acpi_handle(gc->parent, data); > + return ACPI_HANDLE_FWNODE(gc->fwnode) == data; > } > > /** > --- This seems a legit fix. > I can now directly reference the GPIO ACPI node in my GpioInt() > declaration. And AFAICT this should be safe to do because gpiolib ensure > that gc->fwnode is set, using the one from the parent if it is not set > previously. > > I need to check if this works with my icelake laptop, and if so I'll > send it to the list. > > The reason the intel gpios are working (the only one I checked) is > because the \\SB.GPI0 node refers to the pinctrl controller (driver > pinctrl-icelake.c in my case, which then creates a subdevice for > handling the gpio). Good catch!
diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c index 27cadadda7c9..491e3c83af12 100644 --- a/drivers/hid/hid-cp2112.c +++ b/drivers/hid/hid-cp2112.c @@ -1234,6 +1234,7 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id) u8 buf[3]; struct cp2112_smbus_config_report config; struct gpio_irq_chip *girq; + struct i2c_timings timings; int ret; dev = devm_kzalloc(&hdev->dev, sizeof(*dev), GFP_KERNEL); @@ -1292,6 +1293,10 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id) goto err_power_normal; } + device_set_node(&dev->adap.dev, device_get_named_child_node(&hdev->dev, "i2c")); + i2c_parse_fw_timings(&dev->adap.dev, &timings, true); + + config.clock_speed = cpu_to_be32(timings.bus_freq_hz); config.retry_time = cpu_to_be16(1); ret = cp2112_hid_output(hdev, (u8 *)&config, sizeof(config), @@ -1300,7 +1305,7 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id) hid_err(hdev, "error setting SMBus config\n"); if (ret >= 0) ret = -EIO; - goto err_power_normal; + goto err_free_i2c_of; } hid_set_drvdata(hdev, (void *)dev); @@ -1322,7 +1327,7 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id) if (ret) { hid_err(hdev, "error registering i2c adapter\n"); - goto err_power_normal; + goto err_free_i2c_of; } hid_dbg(hdev, "adapter registered\n"); @@ -1336,6 +1341,7 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id) dev->gc.ngpio = 8; dev->gc.can_sleep = 1; dev->gc.parent = &hdev->dev; + dev->gc.fwnode = device_get_named_child_node(&hdev->dev, "gpio"); dev->irq.name = "cp2112-gpio"; dev->irq.irq_startup = cp2112_gpio_irq_startup; @@ -1376,7 +1382,10 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id) err_gpiochip_remove: gpiochip_remove(&dev->gc); err_free_i2c: + fwnode_handle_put(dev->gc.fwnode); i2c_del_adapter(&dev->adap); +err_free_i2c_of: + fwnode_handle_put(dev_fwnode(&dev->adap.dev)); err_power_normal: hid_hw_power(hdev, PM_HINT_NORMAL); err_hid_close: @@ -1391,6 +1400,8 @@ static void cp2112_remove(struct hid_device *hdev) struct cp2112_device *dev = hid_get_drvdata(hdev); int i; + fwnode_handle_put(dev->gc.fwnode); + fwnode_handle_put(dev_fwnode(&dev->adap.dev)); sysfs_remove_group(&hdev->dev.kobj, &cp2112_attr_group); i2c_del_adapter(&dev->adap);
Bind I2C and GPIO interfaces to subnodes with names "i2c" and "gpio" if they exist, respectively. This allows the GPIO and I2C controllers to be described in firmware as usual. Additionally, support configuring the I2C bus speed from the clock-frequency device property. Signed-off-by: Danny Kaehn <kaehndan@gmail.com> --- drivers/hid/hid-cp2112.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-)