Re: [PATCH v2 1/9] drivers/char: Add support for Xue USB3 debugger

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.07.2022 12:45, Marek Marczykowski-Górecki wrote:
> On Tue, Jul 12, 2022 at 05:59:51PM +0200, Jan Beulich wrote:
>> On 06.07.2022 17:32, Marek Marczykowski-Górecki wrote:
>>> +static bool __init xue_init_xhc(struct xue *xue)
>>> +{
>>> +    uint32_t bar0;
>>> +    uint64_t bar1;
>>> +    uint64_t devfn;
>>> +
>>> +    /*
>>> +     * 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);
>>> +        uint32_t hdr = pci_conf_read8(sbdf, PCI_HEADER_TYPE);
>>> +
>>> +        if ( hdr == 0 || hdr == 0x80 )
>>> +        {
>>> +            if ( (pci_conf_read32(sbdf, PCI_CLASS_REVISION) >> 8) == 
>>> XUE_XHC_CLASSC )
>>> +            {
>>> +                xue->sbdf = sbdf;
>>> +                break;
>>> +            }
>>> +        }
>>> +    }
>>> +
>>> +    if ( !xue->sbdf.sbdf )
>>> +    {
>>> +        xue_error("Compatible xHC not found on bus 0\n");
>>> +        return false;
>>> +    }
>>> +
>>> +    /* ...we found it, so parse the BAR and map the registers */
>>> +    bar0 = pci_conf_read32(xue->sbdf, PCI_BASE_ADDRESS_0);
>>> +    bar1 = pci_conf_read32(xue->sbdf, PCI_BASE_ADDRESS_1);
>>> +
>>> +    /* IO BARs not allowed; BAR must be 64-bit */
>>> +    if ( (bar0 & PCI_BASE_ADDRESS_SPACE) != PCI_BASE_ADDRESS_SPACE_MEMORY 
>>> ||
>>> +         (bar0 & PCI_BASE_ADDRESS_MEM_TYPE_MASK) != 
>>> PCI_BASE_ADDRESS_MEM_TYPE_64 )
>>> +        return false;
>>> +
>>> +    pci_conf_write32(xue->sbdf, PCI_BASE_ADDRESS_0, 0xFFFFFFFF);
>>> +    xue->xhc_mmio_size = ~(pci_conf_read32(xue->sbdf, PCI_BASE_ADDRESS_0) 
>>> & 0xFFFFFFF0) + 1;
>>> +    pci_conf_write32(xue->sbdf, PCI_BASE_ADDRESS_0, bar0);
>>
>> Why is a 64-bit BAR required when you size only the low 32 bits?
> 
> xHCI spec says the first BAR is required to be 64-bit, so I'm checking
> this assumption to handle just this one case. But then, the size is 64K
> in practice (and xue_sys_map_xhc() checks for that), so just 32 bits are
> enough. Anyway, I can add sizing the whole thing, for consistency.
> 
>> Also you need to disable memory decoding around this (and
>> somewhere you also need to explicitly enable it, assuming here
>> you would afterwards restore the original value of the command
>> register). 
> 
> Actually, this is a good place to enable memory decoding.

It might seem so, I agree, but then upon encountering a later error
you'll need more precautions so you would able to restore the command
register to its original value. I think it's easier / clearer when
you keep command register save/restore to within functions.

Jan