mbox series

[v4,0/6] USB keyboard improvements for asahi / desktop systems

Message ID 20240404-asahi-keyboards-v4-0-2266033feaff@jannau.net
Headers show
Series USB keyboard improvements for asahi / desktop systems | expand

Message

Janne Grunau via B4 Relay April 4, 2024, 6:25 a.m. UTC
Apple USB Keyboards from 2021 need quirks to be useable. The boot HID
keyboard protocol is unfortunately not described in the first interface
descriptor but the second. This needs several changes. The USB keyboard
driver has to look at all (2) interface descriptors during probing.
Since I didn't want to rebuild the USB driver probe code the Apple
keyboards are bound to the keyboard driver via USB vendor and product
IDs.
To make the keyboards useable on Apple silicon devices the xhci driver
needs to initializes rings for the endpoints of the first two interface
descriptors. If this is causes concerns regarding regressions or memory
use the USB_MAX_ACTIVE_INTERFACES define could be turned into a CONFIG
option.
Even after this changes the keyboards still do not probe successfully
since they apparently do not behave HID standard compliant. They only
generate reports on key events. This leads the final check whether the
keyboard is operational to fail unless the user presses keys during the
probe. Skip this check for known keyboards.
Keychron seems to emulate Apple keyboards (some models even "re-use"
Apple's USB vendor ID) so apply this quirk as well.

Some devices like Yubikeys emulate a keyboard. since u-boot only binds a
single keyboard block this kind of devices from the USB keyboard driver.

Signed-off-by: Janne Grunau <j@jannau.net>
---
Changes in v4:
- collects "Reviewed-by:" tagDITME: describe what is new in this series revision.
- adds comment about usb_ignorelist parse errors
- Link to v3: https://lore.kernel.org/r/20240322-asahi-keyboards-v3-0-3106dd4c4e19@jannau.net

Changes in v3:
- collected "Reviewed-by:" tags
- rename usb_blocklist to usb_ignorelist
- use BIT macro for USB KBD quirk bit
- refactor usb_device_is_blocked() to use 0 / negated errors as return
  value, sed -e 's/block/ignore/', simplify it and add comments
- rewritten usb_ignorelist documentation
- Link to v2: https://lore.kernel.org/r/20240317-asahi-keyboards-v2-0-d3f4b8384f68@jannau.net

Changes in v2:
- rewritten commit message for "[PATCH 2/6] usb: xhci: Set up endpoints for the first 2 interfaces"
- Replaced the usb keyboard Yubikey block with an env based USB device blocklist
- Use "-EINVAL" as return value in "[PATCH 3/6] usb: xhci: Abort transfers with unallocated rings"
- added "Reviewed-by:" tags
- Link to v1: https://lore.kernel.org/r/20240221-asahi-keyboards-v1-0-814b2e741790@jannau.net

---
Janne Grunau (6):
      usb: xhci: refactor xhci_set_configuration
      usb: xhci: Set up endpoints for the first 2 interfaces
      usb: xhci: Abort transfers with unallocated rings
      usb: Add environment based device ignorelist
      usb: kbd: support Apple Magic Keyboards (2021)
      usb: kbd: Add probe quirk for Apple and Keychron keyboards

 common/usb.c                 |  64 ++++++++++++++++++++++
 common/usb_kbd.c             |  59 ++++++++++++++++++--
 doc/usage/environment.rst    |  13 +++++
 drivers/usb/host/xhci-ring.c |   5 ++
 drivers/usb/host/xhci.c      | 126 +++++++++++++++++++++++++++----------------
 include/env_default.h        |  11 ++++
 include/usb.h                |   6 +++
 7 files changed, 235 insertions(+), 49 deletions(-)
---
base-commit: 37345abb97ef0dd9c50a03b2a72617612dcae585
change-id: 20240218-asahi-keyboards-f2ddaf0022b2

Best regards,

Comments

Marek Vasut April 5, 2024, 2:52 p.m. UTC | #1
On 4/4/24 8:25 AM, Janne Grunau via B4 Relay wrote:
> Apple USB Keyboards from 2021 need quirks to be useable. The boot HID
> keyboard protocol is unfortunately not described in the first interface
> descriptor but the second. This needs several changes. The USB keyboard
> driver has to look at all (2) interface descriptors during probing.
> Since I didn't want to rebuild the USB driver probe code the Apple
> keyboards are bound to the keyboard driver via USB vendor and product
> IDs.
> To make the keyboards useable on Apple silicon devices the xhci driver
> needs to initializes rings for the endpoints of the first two interface
> descriptors. If this is causes concerns regarding regressions or memory
> use the USB_MAX_ACTIVE_INTERFACES define could be turned into a CONFIG
> option.
> Even after this changes the keyboards still do not probe successfully
> since they apparently do not behave HID standard compliant. They only
> generate reports on key events. This leads the final check whether the
> keyboard is operational to fail unless the user presses keys during the
> probe. Skip this check for known keyboards.
> Keychron seems to emulate Apple keyboards (some models even "re-use"
> Apple's USB vendor ID) so apply this quirk as well.
> 
> Some devices like Yubikeys emulate a keyboard. since u-boot only binds a
> single keyboard block this kind of devices from the USB keyboard driver.
> 
> Signed-off-by: Janne Grunau <j@jannau.net>

I picked the series, but CI indicates build errors, can you have a look ?

https://source.denx.de/u-boot/custodians/u-boot-usb/-/pipelines/20215

Thanks !
Janne Grunau April 5, 2024, 7:05 p.m. UTC | #2
On Fri, Apr 05, 2024 at 04:52:32PM +0200, Marek Vasut wrote:
> On 4/4/24 8:25 AM, Janne Grunau via B4 Relay wrote:
> > Apple USB Keyboards from 2021 need quirks to be useable. The boot HID
> > keyboard protocol is unfortunately not described in the first interface
> > descriptor but the second. This needs several changes. The USB keyboard
> > driver has to look at all (2) interface descriptors during probing.
> > Since I didn't want to rebuild the USB driver probe code the Apple
> > keyboards are bound to the keyboard driver via USB vendor and product
> > IDs.
> > To make the keyboards useable on Apple silicon devices the xhci driver
> > needs to initializes rings for the endpoints of the first two interface
> > descriptors. If this is causes concerns regarding regressions or memory
> > use the USB_MAX_ACTIVE_INTERFACES define could be turned into a CONFIG
> > option.
> > Even after this changes the keyboards still do not probe successfully
> > since they apparently do not behave HID standard compliant. They only
> > generate reports on key events. This leads the final check whether the
> > keyboard is operational to fail unless the user presses keys during the
> > probe. Skip this check for known keyboards.
> > Keychron seems to emulate Apple keyboards (some models even "re-use"
> > Apple's USB vendor ID) so apply this quirk as well.
> > 
> > Some devices like Yubikeys emulate a keyboard. since u-boot only binds a
> > single keyboard block this kind of devices from the USB keyboard driver.
> > 
> > Signed-off-by: Janne Grunau <j@jannau.net>
> 
> I picked the series, but CI indicates build errors, can you have a look ?
> 
> https://source.denx.de/u-boot/custodians/u-boot-usb/-/pipelines/20215

The issue seems to be that the field dev in struct usb_device exists
only for DM_USB. That means we can't use dev_dbg.
Either take the following fixup patch or I can resend the series.

Thanks

Janne
From 57d54303eb2b60e92bd478e4250a9cc63cfc277e Mon Sep 17 00:00:00 2001
From: Janne Grunau <j@jannau.net>
Date: Fri, 5 Apr 2024 21:00:44 +0200
Subject: [PATCH 1/1] fixup! usb: Add environment based device ignorelist

---
 common/usb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/common/usb.c b/common/usb.c
index 44db07742e..8bc85c58b2 100644
--- a/common/usb.c
+++ b/common/usb.c
@@ -1146,7 +1146,7 @@ int usb_select_config(struct usb_device *dev)
 	err = usb_device_is_ignored(dev->descriptor.idVendor,
 				    dev->descriptor.idProduct);
 	if (err == -ENODEV) {
-		dev_dbg(dev->dev, "Ignoring USB device 0x%x:0x%x\n",
+		debug("Ignoring USB device 0x%x:0x%x\n",
 			dev->descriptor.idVendor, dev->descriptor.idProduct);
 		return err;
 	} else if (err == -EINVAL) {
Marek Vasut April 6, 2024, 6:52 p.m. UTC | #3
On 4/5/24 9:05 PM, Janne Grunau wrote:
> On Fri, Apr 05, 2024 at 04:52:32PM +0200, Marek Vasut wrote:
>> On 4/4/24 8:25 AM, Janne Grunau via B4 Relay wrote:
>>> Apple USB Keyboards from 2021 need quirks to be useable. The boot HID
>>> keyboard protocol is unfortunately not described in the first interface
>>> descriptor but the second. This needs several changes. The USB keyboard
>>> driver has to look at all (2) interface descriptors during probing.
>>> Since I didn't want to rebuild the USB driver probe code the Apple
>>> keyboards are bound to the keyboard driver via USB vendor and product
>>> IDs.
>>> To make the keyboards useable on Apple silicon devices the xhci driver
>>> needs to initializes rings for the endpoints of the first two interface
>>> descriptors. If this is causes concerns regarding regressions or memory
>>> use the USB_MAX_ACTIVE_INTERFACES define could be turned into a CONFIG
>>> option.
>>> Even after this changes the keyboards still do not probe successfully
>>> since they apparently do not behave HID standard compliant. They only
>>> generate reports on key events. This leads the final check whether the
>>> keyboard is operational to fail unless the user presses keys during the
>>> probe. Skip this check for known keyboards.
>>> Keychron seems to emulate Apple keyboards (some models even "re-use"
>>> Apple's USB vendor ID) so apply this quirk as well.
>>>
>>> Some devices like Yubikeys emulate a keyboard. since u-boot only binds a
>>> single keyboard block this kind of devices from the USB keyboard driver.
>>>
>>> Signed-off-by: Janne Grunau <j@jannau.net>
>>
>> I picked the series, but CI indicates build errors, can you have a look ?
>>
>> https://source.denx.de/u-boot/custodians/u-boot-usb/-/pipelines/20215
> 
> The issue seems to be that the field dev in struct usb_device exists
> only for DM_USB. That means we can't use dev_dbg.
> Either take the following fixup patch or I can resend the series.

I squashed the extra patch in, but I think the CI still complains:

Pipeline #20236 ( 
https://source.denx.de/u-boot/custodians/u-boot-usb/-/pipelines/20236 ) 
triggered by Marek Vasut ( https://source.denx.de/marex )
had 1 failed job.

Job #812215 ( 
https://source.denx.de/u-boot/custodians/u-boot-usb/-/jobs/812215/raw )
Janne Grunau April 6, 2024, 8:04 p.m. UTC | #4
On Sat, Apr 06, 2024 at 08:52:17PM +0200, Marek Vasut wrote:
> On 4/5/24 9:05 PM, Janne Grunau wrote:
> > On Fri, Apr 05, 2024 at 04:52:32PM +0200, Marek Vasut wrote:
> >> On 4/4/24 8:25 AM, Janne Grunau via B4 Relay wrote:
> >>> Apple USB Keyboards from 2021 need quirks to be useable. The boot HID
> >>> keyboard protocol is unfortunately not described in the first interface
> >>> descriptor but the second. This needs several changes. The USB keyboard
> >>> driver has to look at all (2) interface descriptors during probing.
> >>> Since I didn't want to rebuild the USB driver probe code the Apple
> >>> keyboards are bound to the keyboard driver via USB vendor and product
> >>> IDs.
> >>> To make the keyboards useable on Apple silicon devices the xhci driver
> >>> needs to initializes rings for the endpoints of the first two interface
> >>> descriptors. If this is causes concerns regarding regressions or memory
> >>> use the USB_MAX_ACTIVE_INTERFACES define could be turned into a CONFIG
> >>> option.
> >>> Even after this changes the keyboards still do not probe successfully
> >>> since they apparently do not behave HID standard compliant. They only
> >>> generate reports on key events. This leads the final check whether the
> >>> keyboard is operational to fail unless the user presses keys during the
> >>> probe. Skip this check for known keyboards.
> >>> Keychron seems to emulate Apple keyboards (some models even "re-use"
> >>> Apple's USB vendor ID) so apply this quirk as well.
> >>>
> >>> Some devices like Yubikeys emulate a keyboard. since u-boot only binds a
> >>> single keyboard block this kind of devices from the USB keyboard driver.
> >>>
> >>> Signed-off-by: Janne Grunau <j@jannau.net>
> >>
> >> I picked the series, but CI indicates build errors, can you have a look ?
> >>
> >> https://source.denx.de/u-boot/custodians/u-boot-usb/-/pipelines/20215
> > 
> > The issue seems to be that the field dev in struct usb_device exists
> > only for DM_USB. That means we can't use dev_dbg.
> > Either take the following fixup patch or I can resend the series.
> 
> I squashed the extra patch in, but I think the CI still complains:
> 
> Pipeline #20236 ( 
> https://source.denx.de/u-boot/custodians/u-boot-usb/-/pipelines/20236 ) 
> triggered by Marek Vasut ( https://source.denx.de/marex )
> had 1 failed job.
> 
> Job #812215 ( 
> https://source.denx.de/u-boot/custodians/u-boot-usb/-/jobs/812215/raw )

env_get() missing without CONFIG_ENV_SUPPORT. I'm too accustomed to the
kernel's stub functions. Best option seems to to just #if the
functionality out in this case. See attached fixup patch.

Sorry,

Janne
From 214ec2aad1ed42d6e1256bbc7eaeff84e17443e0 Mon Sep 17 00:00:00 2001
From: Janne Grunau <j@jannau.net>
Date: Sat, 6 Apr 2024 21:46:44 +0200
Subject: [PATCH 1/1] fixup! usb: Add environment based device ignorelist

---
 common/usb.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/common/usb.c b/common/usb.c
index 8bc85c58b2..52f77431b0 100644
--- a/common/usb.c
+++ b/common/usb.c
@@ -1085,6 +1085,7 @@ static int usb_prepare_device(struct usb_device *dev, int addr, bool do_read,
 	return 0;
 }
 
+#if CONFIG_IS_ENABLED(ENV_SUPPORT)
 static int usb_device_is_ignored(u16 id_vendor, u16 id_product)
 {
 	ulong vid, pid;
@@ -1126,6 +1127,7 @@ static int usb_device_is_ignored(u16 id_vendor, u16 id_product)
 
 	return 0;
 }
+#endif
 
 int usb_select_config(struct usb_device *dev)
 {
@@ -1142,6 +1144,7 @@ int usb_select_config(struct usb_device *dev)
 	le16_to_cpus(&dev->descriptor.idProduct);
 	le16_to_cpus(&dev->descriptor.bcdDevice);
 
+#if CONFIG_IS_ENABLED(ENV_SUPPORT)
 	/* ignore devices from usb_ignorelist */
 	err = usb_device_is_ignored(dev->descriptor.idVendor,
 				    dev->descriptor.idProduct);
@@ -1162,6 +1165,7 @@ int usb_select_config(struct usb_device *dev)
 	} else if (err < 0) {
 		return err;
 	}
+#endif
 
 	/*
 	 * Kingston DT Ultimate 32GB USB 3.0 seems to be extremely sensitive
Marek Vasut April 7, 2024, 1:05 a.m. UTC | #5
On 4/6/24 10:04 PM, Janne Grunau wrote:
> On Sat, Apr 06, 2024 at 08:52:17PM +0200, Marek Vasut wrote:
>> On 4/5/24 9:05 PM, Janne Grunau wrote:
>>> On Fri, Apr 05, 2024 at 04:52:32PM +0200, Marek Vasut wrote:
>>>> On 4/4/24 8:25 AM, Janne Grunau via B4 Relay wrote:
>>>>> Apple USB Keyboards from 2021 need quirks to be useable. The boot HID
>>>>> keyboard protocol is unfortunately not described in the first interface
>>>>> descriptor but the second. This needs several changes. The USB keyboard
>>>>> driver has to look at all (2) interface descriptors during probing.
>>>>> Since I didn't want to rebuild the USB driver probe code the Apple
>>>>> keyboards are bound to the keyboard driver via USB vendor and product
>>>>> IDs.
>>>>> To make the keyboards useable on Apple silicon devices the xhci driver
>>>>> needs to initializes rings for the endpoints of the first two interface
>>>>> descriptors. If this is causes concerns regarding regressions or memory
>>>>> use the USB_MAX_ACTIVE_INTERFACES define could be turned into a CONFIG
>>>>> option.
>>>>> Even after this changes the keyboards still do not probe successfully
>>>>> since they apparently do not behave HID standard compliant. They only
>>>>> generate reports on key events. This leads the final check whether the
>>>>> keyboard is operational to fail unless the user presses keys during the
>>>>> probe. Skip this check for known keyboards.
>>>>> Keychron seems to emulate Apple keyboards (some models even "re-use"
>>>>> Apple's USB vendor ID) so apply this quirk as well.
>>>>>
>>>>> Some devices like Yubikeys emulate a keyboard. since u-boot only binds a
>>>>> single keyboard block this kind of devices from the USB keyboard driver.
>>>>>
>>>>> Signed-off-by: Janne Grunau <j@jannau.net>
>>>>
>>>> I picked the series, but CI indicates build errors, can you have a look ?
>>>>
>>>> https://source.denx.de/u-boot/custodians/u-boot-usb/-/pipelines/20215
>>>
>>> The issue seems to be that the field dev in struct usb_device exists
>>> only for DM_USB. That means we can't use dev_dbg.
>>> Either take the following fixup patch or I can resend the series.
>>
>> I squashed the extra patch in, but I think the CI still complains:
>>
>> Pipeline #20236 (
>> https://source.denx.de/u-boot/custodians/u-boot-usb/-/pipelines/20236 )
>> triggered by Marek Vasut ( https://source.denx.de/marex )
>> had 1 failed job.
>>
>> Job #812215 (
>> https://source.denx.de/u-boot/custodians/u-boot-usb/-/jobs/812215/raw )
> 
> env_get() missing without CONFIG_ENV_SUPPORT. I'm too accustomed to the
> kernel's stub functions. Best option seems to to just #if the
> functionality out in this case. See attached fixup patch.
> 
> Sorry,

No worries.

Does it work if you do this instead ?

  static int usb_device_is_ignored(u16 id_vendor, u16 id_product)
  {
         ulong vid, pid;
+       /* No env support, nothing can be ignored */
+       if (CONFIG_IS_ENABLED(ENV_SUPPORT))
+           return 0;

That way, the function is always compiled and if it is unreachable, then 
compiler throws it out. This should improve code compile coverage.
Janne Grunau April 8, 2024, 7:46 a.m. UTC | #6
On Sun, Apr 07, 2024 at 03:05:59AM +0200, Marek Vasut wrote:
> On 4/6/24 10:04 PM, Janne Grunau wrote:
> > On Sat, Apr 06, 2024 at 08:52:17PM +0200, Marek Vasut wrote:
> >> On 4/5/24 9:05 PM, Janne Grunau wrote:
> >>> On Fri, Apr 05, 2024 at 04:52:32PM +0200, Marek Vasut wrote:
> >>>> On 4/4/24 8:25 AM, Janne Grunau via B4 Relay wrote:
> >>>>> Apple USB Keyboards from 2021 need quirks to be useable. The boot HID
> >>>>> keyboard protocol is unfortunately not described in the first interface
> >>>>> descriptor but the second. This needs several changes. The USB keyboard
> >>>>> driver has to look at all (2) interface descriptors during probing.
> >>>>> Since I didn't want to rebuild the USB driver probe code the Apple
> >>>>> keyboards are bound to the keyboard driver via USB vendor and product
> >>>>> IDs.
> >>>>> To make the keyboards useable on Apple silicon devices the xhci driver
> >>>>> needs to initializes rings for the endpoints of the first two interface
> >>>>> descriptors. If this is causes concerns regarding regressions or memory
> >>>>> use the USB_MAX_ACTIVE_INTERFACES define could be turned into a CONFIG
> >>>>> option.
> >>>>> Even after this changes the keyboards still do not probe successfully
> >>>>> since they apparently do not behave HID standard compliant. They only
> >>>>> generate reports on key events. This leads the final check whether the
> >>>>> keyboard is operational to fail unless the user presses keys during the
> >>>>> probe. Skip this check for known keyboards.
> >>>>> Keychron seems to emulate Apple keyboards (some models even "re-use"
> >>>>> Apple's USB vendor ID) so apply this quirk as well.
> >>>>>
> >>>>> Some devices like Yubikeys emulate a keyboard. since u-boot only binds a
> >>>>> single keyboard block this kind of devices from the USB keyboard driver.
> >>>>>
> >>>>> Signed-off-by: Janne Grunau <j@jannau.net>
> >>>>
> >>>> I picked the series, but CI indicates build errors, can you have a look ?
> >>>>
> >>>> https://source.denx.de/u-boot/custodians/u-boot-usb/-/pipelines/20215
> >>>
> >>> The issue seems to be that the field dev in struct usb_device exists
> >>> only for DM_USB. That means we can't use dev_dbg.
> >>> Either take the following fixup patch or I can resend the series.
> >>
> >> I squashed the extra patch in, but I think the CI still complains:
> >>
> >> Pipeline #20236 (
> >> https://source.denx.de/u-boot/custodians/u-boot-usb/-/pipelines/20236 )
> >> triggered by Marek Vasut ( https://source.denx.de/marex )
> >> had 1 failed job.
> >>
> >> Job #812215 (
> >> https://source.denx.de/u-boot/custodians/u-boot-usb/-/jobs/812215/raw )
> > 
> > env_get() missing without CONFIG_ENV_SUPPORT. I'm too accustomed to the
> > kernel's stub functions. Best option seems to to just #if the
> > functionality out in this case. See attached fixup patch.
> > 
> > Sorry,
> 
> No worries.
> 
> Does it work if you do this instead ?
> 
>   static int usb_device_is_ignored(u16 id_vendor, u16 id_product)
>   {
>          ulong vid, pid;
> +       /* No env support, nothing can be ignored */
> +       if (CONFIG_IS_ENABLED(ENV_SUPPORT))
> +           return 0;
> 
> That way, the function is always compiled and if it is unreachable, then 
> compiler throws it out. This should improve code compile coverage.

Seems to work here with a broken imx8 config from the CI. Is it ok to
rely on dead code elimination? Apparently it is, build with KCFLAGS=-O0
has already several other missing symbols.

See attached fixup

Janne
From ab3b825d7bc0571cfb38eb80a1e449e51d5d2f6d Mon Sep 17 00:00:00 2001
From: Janne Grunau <j@jannau.net>
Date: Mon, 8 Apr 2024 09:44:54 +0200
Subject: [PATCH 1/1] fixup! usb: Add environment based device ignorelist

---
 common/usb.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/common/usb.c b/common/usb.c
index 8bc85c58b28c..99e6b857c74c 100644
--- a/common/usb.c
+++ b/common/usb.c
@@ -1089,7 +1089,13 @@ static int usb_device_is_ignored(u16 id_vendor, u16 id_product)
 {
 	ulong vid, pid;
 	char *end;
-	const char *cur = env_get("usb_ignorelist");
+	const char *cur = NULL;
+
+	/* ignore list depends on env support */
+	if (!CONFIG_IS_ENABLED(ENV_SUPPORT))
+		return 0;
+
+	cur = env_get("usb_ignorelist");
 
 	/* parse "usb_ignorelist" strictly */
 	while (cur && cur[0] != '\0') {
Marek Vasut April 12, 2024, 12:53 p.m. UTC | #7
On 4/8/24 9:46 AM, Janne Grunau wrote:
> On Sun, Apr 07, 2024 at 03:05:59AM +0200, Marek Vasut wrote:
>> On 4/6/24 10:04 PM, Janne Grunau wrote:
>>> On Sat, Apr 06, 2024 at 08:52:17PM +0200, Marek Vasut wrote:
>>>> On 4/5/24 9:05 PM, Janne Grunau wrote:
>>>>> On Fri, Apr 05, 2024 at 04:52:32PM +0200, Marek Vasut wrote:
>>>>>> On 4/4/24 8:25 AM, Janne Grunau via B4 Relay wrote:
>>>>>>> Apple USB Keyboards from 2021 need quirks to be useable. The boot HID
>>>>>>> keyboard protocol is unfortunately not described in the first interface
>>>>>>> descriptor but the second. This needs several changes. The USB keyboard
>>>>>>> driver has to look at all (2) interface descriptors during probing.
>>>>>>> Since I didn't want to rebuild the USB driver probe code the Apple
>>>>>>> keyboards are bound to the keyboard driver via USB vendor and product
>>>>>>> IDs.
>>>>>>> To make the keyboards useable on Apple silicon devices the xhci driver
>>>>>>> needs to initializes rings for the endpoints of the first two interface
>>>>>>> descriptors. If this is causes concerns regarding regressions or memory
>>>>>>> use the USB_MAX_ACTIVE_INTERFACES define could be turned into a CONFIG
>>>>>>> option.
>>>>>>> Even after this changes the keyboards still do not probe successfully
>>>>>>> since they apparently do not behave HID standard compliant. They only
>>>>>>> generate reports on key events. This leads the final check whether the
>>>>>>> keyboard is operational to fail unless the user presses keys during the
>>>>>>> probe. Skip this check for known keyboards.
>>>>>>> Keychron seems to emulate Apple keyboards (some models even "re-use"
>>>>>>> Apple's USB vendor ID) so apply this quirk as well.
>>>>>>>
>>>>>>> Some devices like Yubikeys emulate a keyboard. since u-boot only binds a
>>>>>>> single keyboard block this kind of devices from the USB keyboard driver.
>>>>>>>
>>>>>>> Signed-off-by: Janne Grunau <j@jannau.net>
>>>>>>
>>>>>> I picked the series, but CI indicates build errors, can you have a look ?
>>>>>>
>>>>>> https://source.denx.de/u-boot/custodians/u-boot-usb/-/pipelines/20215
>>>>>
>>>>> The issue seems to be that the field dev in struct usb_device exists
>>>>> only for DM_USB. That means we can't use dev_dbg.
>>>>> Either take the following fixup patch or I can resend the series.
>>>>
>>>> I squashed the extra patch in, but I think the CI still complains:
>>>>
>>>> Pipeline #20236 (
>>>> https://source.denx.de/u-boot/custodians/u-boot-usb/-/pipelines/20236 )
>>>> triggered by Marek Vasut ( https://source.denx.de/marex )
>>>> had 1 failed job.
>>>>
>>>> Job #812215 (
>>>> https://source.denx.de/u-boot/custodians/u-boot-usb/-/jobs/812215/raw )
>>>
>>> env_get() missing without CONFIG_ENV_SUPPORT. I'm too accustomed to the
>>> kernel's stub functions. Best option seems to to just #if the
>>> functionality out in this case. See attached fixup patch.
>>>
>>> Sorry,
>>
>> No worries.
>>
>> Does it work if you do this instead ?
>>
>>    static int usb_device_is_ignored(u16 id_vendor, u16 id_product)
>>    {
>>           ulong vid, pid;
>> +       /* No env support, nothing can be ignored */
>> +       if (CONFIG_IS_ENABLED(ENV_SUPPORT))
>> +           return 0;
>>
>> That way, the function is always compiled and if it is unreachable, then
>> compiler throws it out. This should improve code compile coverage.
> 
> Seems to work here with a broken imx8 config from the CI. Is it ok to
> rely on dead code elimination? Apparently it is, build with KCFLAGS=-O0
> has already several other missing symbols.
> 
> See attached fixup

Thanks, squashed, let's see how CI likes this.
Marek Vasut April 12, 2024, 6:26 p.m. UTC | #8
On 4/12/24 2:53 PM, Marek Vasut wrote:

Hi,

>> Seems to work here with a broken imx8 config from the CI. Is it ok to
>> rely on dead code elimination? Apparently it is, build with KCFLAGS=-O0
>> has already several other missing symbols.
>>
>> See attached fixup
> 
> Thanks, squashed, let's see how CI likes this.

I think we are getting closer, but still ...

Pipeline #20308 has failed!

Project: USB U-Boot Custodian Tree ( 
https://source.denx.de/u-boot/custodians/u-boot-usb )
Branch: master ( 
https://source.denx.de/u-boot/custodians/u-boot-usb/-/commits/master )

Commit: 63f6a449 ( 
https://source.denx.de/u-boot/custodians/u-boot-usb/-/commit/63f6a449bffe46beca89580d3efa48e5d041025c 
)
Commit Message: usb: kbd: Add probe quirk for Apple and Keychro...
Commit Author: Janne Grunau
Committed by: Marek Vasut ( https://source.denx.de/marex )


Pipeline #20308 ( 
https://source.denx.de/u-boot/custodians/u-boot-usb/-/pipelines/20308 ) 
triggered by Marek Vasut ( https://source.denx.de/marex )
had 1 failed job.

Job #815411 ( 
https://source.denx.de/u-boot/custodians/u-boot-usb/-/jobs/815411/raw )

Stage: world build
Name: build all 64bit ARM platforms
Tom Rini April 12, 2024, 6:37 p.m. UTC | #9
On Fri, Apr 12, 2024 at 08:26:18PM +0200, Marek Vasut wrote:
> On 4/12/24 2:53 PM, Marek Vasut wrote:
> 
> Hi,
> 
> > > Seems to work here with a broken imx8 config from the CI. Is it ok to
> > > rely on dead code elimination? Apparently it is, build with KCFLAGS=-O0
> > > has already several other missing symbols.
> > > 
> > > See attached fixup
> > 
> > Thanks, squashed, let's see how CI likes this.
> 
> I think we are getting closer, but still ...
> 
> Pipeline #20308 has failed!
> 
> Project: USB U-Boot Custodian Tree (
> https://source.denx.de/u-boot/custodians/u-boot-usb )
> Branch: master (
> https://source.denx.de/u-boot/custodians/u-boot-usb/-/commits/master )
> 
> Commit: 63f6a449 ( https://source.denx.de/u-boot/custodians/u-boot-usb/-/commit/63f6a449bffe46beca89580d3efa48e5d041025c
> )
> Commit Message: usb: kbd: Add probe quirk for Apple and Keychro...
> Commit Author: Janne Grunau
> Committed by: Marek Vasut ( https://source.denx.de/marex )
> 
> 
> Pipeline #20308 (
> https://source.denx.de/u-boot/custodians/u-boot-usb/-/pipelines/20308 )
> triggered by Marek Vasut ( https://source.denx.de/marex )
> had 1 failed job.
> 
> Job #815411 (
> https://source.denx.de/u-boot/custodians/u-boot-usb/-/jobs/815411/raw )
> 
> Stage: world build
> Name: build all 64bit ARM platforms

That looks like the machine doing the build failed, I've kicked the
retry button.
Marek Vasut April 13, 2024, 12:59 a.m. UTC | #10
On 4/12/24 8:37 PM, Tom Rini wrote:
> On Fri, Apr 12, 2024 at 08:26:18PM +0200, Marek Vasut wrote:
>> On 4/12/24 2:53 PM, Marek Vasut wrote:
>>
>> Hi,
>>
>>>> Seems to work here with a broken imx8 config from the CI. Is it ok to
>>>> rely on dead code elimination? Apparently it is, build with KCFLAGS=-O0
>>>> has already several other missing symbols.
>>>>
>>>> See attached fixup
>>>
>>> Thanks, squashed, let's see how CI likes this.
>>
>> I think we are getting closer, but still ...
>>
>> Pipeline #20308 has failed!
>>
>> Project: USB U-Boot Custodian Tree (
>> https://source.denx.de/u-boot/custodians/u-boot-usb )
>> Branch: master (
>> https://source.denx.de/u-boot/custodians/u-boot-usb/-/commits/master )
>>
>> Commit: 63f6a449 ( https://source.denx.de/u-boot/custodians/u-boot-usb/-/commit/63f6a449bffe46beca89580d3efa48e5d041025c
>> )
>> Commit Message: usb: kbd: Add probe quirk for Apple and Keychro...
>> Commit Author: Janne Grunau
>> Committed by: Marek Vasut ( https://source.denx.de/marex )
>>
>>
>> Pipeline #20308 (
>> https://source.denx.de/u-boot/custodians/u-boot-usb/-/pipelines/20308 )
>> triggered by Marek Vasut ( https://source.denx.de/marex )
>> had 1 failed job.
>>
>> Job #815411 (
>> https://source.denx.de/u-boot/custodians/u-boot-usb/-/jobs/815411/raw )
>>
>> Stage: world build
>> Name: build all 64bit ARM platforms
> 
> That looks like the machine doing the build failed, I've kicked the
> retry button.

And now it passed, PR is coming real soon now, thanks all !