Re: [PATCH 2/2] hw/usb: Add TI TUSB73X0 XHCI controller model

["Nicholas Piggin" <npiggin@gmail.com>]

On Mon Nov 11, 2024 at 1:39 AM AEST, Philippe Mathieu-Daudé wrote:
> Hi Nick,
>
> On 10/11/24 05:00, Nicholas Piggin wrote:
> > This controller is accepted by IBM Power firmware when the subsystem IDs
> > are set to Power servers. Firmware is picky about device support so the
> > NEC driver does not work.
> > 
> > The TI HW has some interesting differences from NEC, notably a separate
> > BAR for MSIX, and PM capabilities. The spec is freely available without
> > sign-up.
> > 
> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > ---
> >   include/hw/pci/pci_ids.h |  1 +
> >   include/hw/usb/xhci.h    |  1 +
> >   hw/usb/hcd-xhci-ti.c     | 94 ++++++++++++++++++++++++++++++++++++++++
> >   hw/usb/Kconfig           |  5 +++
> >   hw/usb/meson.build       |  1 +
> >   5 files changed, 102 insertions(+)
> >   create mode 100644 hw/usb/hcd-xhci-ti.c
> > 
> > diff --git a/include/hw/pci/pci_ids.h b/include/hw/pci/pci_ids.h
> > index f1a53fea8d..fdb692db51 100644
> > --- a/include/hw/pci/pci_ids.h
> > +++ b/include/hw/pci/pci_ids.h
> > @@ -182,6 +182,7 @@
> >   #define PCI_VENDOR_ID_HP                 0x103c
> >   
> >   #define PCI_VENDOR_ID_TI                 0x104c
> > +#define PCI_DEVICE_ID_TI_TUSB73X0        0x8241
> >   
> >   #define PCI_VENDOR_ID_MOTOROLA           0x1057
> >   #define PCI_DEVICE_ID_MOTOROLA_MPC106    0x0002
> > diff --git a/include/hw/usb/xhci.h b/include/hw/usb/xhci.h
> > index 5c90e1373e..203ec1fca3 100644
> > --- a/include/hw/usb/xhci.h
> > +++ b/include/hw/usb/xhci.h
> > @@ -3,6 +3,7 @@
> >   
> >   #define TYPE_XHCI "base-xhci"
> >   #define TYPE_NEC_XHCI "nec-usb-xhci"
> > +#define TYPE_TI_XHCI "ti-usb-xhci"
> >   #define TYPE_QEMU_XHCI "qemu-xhci"
> >   #define TYPE_XHCI_SYSBUS "sysbus-xhci"
> >   
> > diff --git a/hw/usb/hcd-xhci-ti.c b/hw/usb/hcd-xhci-ti.c
> > new file mode 100644
> > index 0000000000..a3f7ef8ba2
> > --- /dev/null
> > +++ b/hw/usb/hcd-xhci-ti.c
> > @@ -0,0 +1,94 @@
> > +/*
> > + * USB xHCI controller emulation
> > + * Datasheet https://www.ti.com/product/TUSB7340
> > + *
> > + * Copyright (c) 2011 Securiforest
> > + * Date: 2011-05-11 ;  Author: Hector Martin <hector@marcansoft.com>
> > + * Based on usb-xhci-nec.c, emulates TI TUSB73X0
> > + *
> > + * This library is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU Lesser General Public
> > + * License as published by the Free Software Foundation; either
> > + * version 2.1 of the License, or (at your option) any later version.
> > + *
> > + * This library is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > + * Lesser General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU Lesser General Public
> > + * License along with this library; if not, see <http://www.gnu.org/licenses/>.
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "hw/usb.h"
> > +#include "qemu/module.h"
> > +#include "hw/pci/pci.h"
> > +#include "hw/qdev-properties.h"
> > +
> > +#include "hcd-xhci-pci.h"
> > +
> > +OBJECT_DECLARE_SIMPLE_TYPE(XHCITiState, TI_XHCI)
> > +
> > +struct XHCITiState {
> > +    /*< private >*/
> > +    XHCIPciState parent_obj;
> > +    /*< public >*/
> > +    uint32_t flags;
> > +    uint32_t intrs;
> > +    uint32_t slots;
> > +};
> > +
> > +static Property ti_xhci_properties[] = {
> > +    DEFINE_PROP_ON_OFF_AUTO("msi", XHCIPciState, msi, ON_OFF_AUTO_AUTO),
> > +    DEFINE_PROP_ON_OFF_AUTO("msix", XHCIPciState, msix, ON_OFF_AUTO_AUTO),
> > +    DEFINE_PROP_UINT32("intrs", XHCITiState, intrs, 8),
> > +    DEFINE_PROP_UINT32("slots", XHCITiState, slots, XHCI_MAXSLOTS),
> > +    DEFINE_PROP_END_OF_LIST(),
> > +};
> > +
> > +static void ti_xhci_instance_init(Object *obj)
> > +{
> > +    XHCIPciState *pci = XHCI_PCI(obj);
> > +    XHCITiState *ti = TI_XHCI(obj);
> > +
> > +    pci->xhci.flags    = ti->flags;
>
> ti->flags doesn't seem initialized / used.

Aha, because you removed the last flags from nec driver and I
copied this from there :P

Good catch. Shall we just remove the flags from XHCINecState?

Thanks,
Nick