Re: [PATCH v2 0/7] Add xHCI USB support

[Patrick Rudolph <patrick.rudolph@9elements.com>]

Hi Glenn,
yes it's the same patch series, but has been cleaned and improved a lot.
I've addressed most of the comments received earlier.

As stated in the commit message, USB 3.1 and USB 3.2 are not
tested, as I lack hardware to test this. I'm not going to look into this
any further into this as it works fine with USB 3.0 compliant xhci controllers.

I'l fix the remaining issues and publish a new patch series.

Kind regards,
Patrick Rudolph

On Fri, Feb 19, 2021 at 7:11 PM Glenn Washburn
<development@efficientek.com> wrote:
>
> Hi Patrick,
>
> Thanks for the contribution. I think this would be a great addition to
> GRUB. However, there are a few issues I see at the moment.
>
> On Mon,  7 Dec 2020 08:41:20 +0100
> Patrick Rudolph <patrick.rudolph@9elements.com> wrote:
>
> > Add basic support for xHCI USB controllers and non root xHCI hubs.
> > The motivation is to use this code on platforms that do not provide
> > user input by runtime services (like BIOS or UEFI platform) do.
> > This is the case when GRUB is used as coreboot payload for example.
> >
> > The code is based on seabios implementation, but has been heavily
> > modified to match grubs internals.
>
> Are these the same changes as in https://github.com/Nitrokey/grub.git
> referenced in a previous email? I'm asking because there were some
> comments that were unaddressed. So, if it is, could you please address
> Thomas's email (I haven't checked to see if they are still relevant if
> so)? (see: https://marc.info/?l=grub-devel&m=159769164332293)
>
> Searching in the mailing list, I found this attempt in early 2017.
> Having not looked at the code at all, could this help in addressing
> some of Thomas's concerns?
> https://github.com/bjornfor/grub/tree/add-coreboot-xhci-driver-2nd-attempt-v2
>
> I've run your code through the GitLab CI I've configured, and there was
> a build failure on x86_64-efi. Looks like some implicit type casting
> issues. I believe it failed for both gcc 8.1 and 10.1. Here is the
> error log when using gcc 10.1:
> https://gitlab.com/gwashburn/grub/-/jobs/1025317154#L594
>
> Also, I think that any xhci support should be accompanied by passing
> make check tests.  See uhci_test for an example of how this might be
> done. Since you've already tested on qemu, I think you're 90% of the
> way there in making some tests. Both of the ones listed before would be
> nice. If you'd like some guidance or help, let me know.
>
> Glenn
>
> > Changes done in version 2:
> >  * Code cleanup
> >  * Code style fixes
> >  * Don't leak memory buffers
> >  * Compile without warnings
> >  * Add more defines
> >  * Add more helper functions
> >  * Don't assume a 1:1 virtual to physical mapping
> >  * Flush cachelines after writing buffers
> >  * Don't use hardcoded page size
> >  * Proper scratchpad register setup
> >  * xHCI bios ownership handoff
> >
> > Changes done in version 3:
> >  * Several bug fixes for real hardware
> >  * Fixed a race condition detecting events, which doesn't appear on
> >    qemu based xHCI controllers
> >  * Don't accidently disable USB3.0 devices after first command
> >  * Support arbitrary protocol speed IDs
> >  * Coding style cleanup
> >  * Support for non root SuperSpeed hubs
> >
> > Changes Tested:
> >  * Qemu system x86_64
> >    * virtual USB HID keyboard (usb-kbd)
> >    * virtual USB HID mass storage (usb-storage)
> >  * Intel C246 integrated xHCI (Supermicro X11SSH-F)
> >    * iKVM HID keyboard
> >    * USB3 HID mass storage (controller root port)
> >    * External USB HID keyboard
> >
> > TODO:
> >  * Test on more hardware
> >  * Test on USB3 hubs
> >  * Support for USB 3.1 and USB 3.2 controllers
> >
> > Patrick Rudolph (7):
> >   grub-core/bus/usb: Parse SuperSpeed companion descriptors
> >   usb: Add enum for xHCI
> >   usbtrans: Set default maximum packet size
> >   grub-core/bus/usb: Add function pointer for attach/detach events
> >   grub-core/bus/usb/usbhub: Add new private fields for xHCI controller
> >   grub-core/bus/usb: Add xhci support
> >   grub-core/bus/usb/usbhub: Add xHCI non root hub support
> >
> >  Makefile.am                       |    2 +-
> >  grub-core/Makefile.core.def       |    7 +
> >  grub-core/bus/usb/ehci.c          |    2 +
> >  grub-core/bus/usb/ohci.c          |    2 +
> >  grub-core/bus/usb/serial/common.c |    2 +-
> >  grub-core/bus/usb/uhci.c          |    2 +
> >  grub-core/bus/usb/usb.c           |   44 +-
> >  grub-core/bus/usb/usbhub.c        |   95 +-
> >  grub-core/bus/usb/usbtrans.c      |    6 +-
> >  grub-core/bus/usb/xhci-pci.c      |  195 +++
> >  grub-core/bus/usb/xhci.c          | 2496
> > +++++++++++++++++++++++++++++ grub-core/commands/usbtest.c      |
> > 2 +- grub-core/disk/usbms.c            |    2 +-
> >  grub-core/term/usb_keyboard.c     |    2 +-
> >  include/grub/usb.h                |   18 +-
> >  include/grub/usbdesc.h            |   12 +-
> >  include/grub/usbtrans.h           |    4 +
> >  17 files changed, 2852 insertions(+), 41 deletions(-)
> >  create mode 100644 grub-core/bus/usb/xhci-pci.c
> >  create mode 100644 grub-core/bus/usb/xhci.c
> >