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
