Re: [RFC PATCH 2/3] drivers/char: search all buses for xhci

[Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

On Wed, Dec 14, 2022 at 11:51:31AM +0100, Jan Beulich wrote:
> On 11.12.2022 03:10, Marek Marczykowski-Górecki wrote:
> > --- a/xen/drivers/char/xhci-dbc.c
> > +++ b/xen/drivers/char/xhci-dbc.c
> > @@ -286,39 +286,87 @@ static void *dbc_sys_map_xhc(uint64_t phys, size_t 
> > size)
> >      return fix_to_virt(FIX_XHCI_END);
> >  }
> >  
> > +static void xhci_bios_handoff(struct dbc *dbc)
> > +{
> > +    const uint32_t __iomem *xcap;
> > +    uint32_t xcap_val;
> > +    uint32_t next;
> > +    uint32_t id = 0;
> > +    const void __iomem *mmio = dbc->xhc_mmio;
> > +    const uint32_t __iomem *hccp1 = mmio + 0x10;
> > +    const uint32_t LEGACY_ID = 0x1;
> > +    int ttl = 48;
> > +    int timeout = 10000;
> > +
> > +    xcap = mmio;
> > +    /*
> > +     * This is initially an offset to the first capability. All the offsets
> > +     * (both in HCCP1 and then next capability pointer) are dword-based.
> > +     */
> > +    next = (readl(hccp1) & 0xFFFF0000) >> 16;
> > +
> > +    while ( id != LEGACY_ID && next && ttl-- )
> > +    {
> > +        xcap += next;
> > +        xcap_val = readl(xcap);
> > +        id = xcap_val & 0xFF;
> > +        next = (xcap_val & 0xFF00) >> 8;
> > +    }
> > +
> > +    if ( id != LEGACY_ID )
> > +        return;
> > +
> > +    xcap_val = readl(xcap);
> > +#define XHCI_HC_BIOS_OWNED (1U << 16)
> > +#define XHCI_HC_OS_OWNED (1U << 24)
> > +    if (xcap_val & XHCI_HC_BIOS_OWNED) {
> > +        dbc_error("bios owned\n");
> > +        writeb(1, (uint8_t*)xcap + 3);
> > +        while ((readl(xcap) & (XHCI_HC_BIOS_OWNED | XHCI_HC_OS_OWNED)) != 
> > XHCI_HC_OS_OWNED)
> > +        {
> > +            cpu_relax();
> > +            if (!--timeout)
> > +                break;
> > +        }
> > +        if (!timeout)
> > +            dbc_error("handoff timeout\n");
> > +        xcap_val = readl(xcap + 1);
> > +        xcap_val &= ((0x7 << 1) + (0xff << 5) + (0x7 << 17)); // 
> > XHCI_LEGACY_DISABLE_SMI
> > +        xcap_val |= (0x7 << 29); // XHCI_LEGACY_SMI_EVENTS
> > +        writel(xcap_val, xcap + 1);
> > +    }
> > +}
> > +
> 
> Unused new function (introducing a build failure at this point of the series)?

Oh, sorry, it wasn't supposed to be included in this patch.

> >  static bool __init dbc_init_xhc(struct dbc *dbc)
> >  {
> >      uint32_t bar0;
> >      uint64_t bar1;
> >      uint64_t bar_val;
> >      uint64_t bar_size;
> > -    uint64_t devfn;
> > +    unsigned int bus, devfn;
> >      uint16_t cmd;
> >      size_t xhc_mmio_size;
> >  
> >      if ( dbc->sbdf.sbdf == 0 )
> >      {
> > -        /*
> > -         * Search PCI bus 0 for the xHC. All the host controllers 
> > supported so
> > -         * far are part of the chipset and are on bus 0.
> > -         */
> > -        for ( devfn = 0; devfn < 256; devfn++ )
> > -        {
> > -            pci_sbdf_t sbdf = PCI_SBDF(0, 0, devfn);
> > -            uint8_t hdr = pci_conf_read8(sbdf, PCI_HEADER_TYPE);
> > -
> > -            if ( hdr == 0 || hdr == 0x80 )
> > +        for ( bus = 0; bus < 0x100; bus++ )
> 
> Hex here and ...
> 
> > +            for ( devfn = 0; devfn < 256; devfn++ )
> 
> ... dec here looks odd. Now that you want to fully iterate segment 0,
> may I suggest to move the function closer to ehci-dbgp.c:find_dbgp(),
> making use of pci_device_detect() and iterating over busses, slots,
> and functions separately?

Ok, will look into it.

But still, this all is not enough to get it working on AMD, so I'd wait
with v2 until it starts being functional.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

Attachment: signature.asc
Description: PGP signature