[PATCH v4 2/9] usb: xhci: Use only 32-bit accesses in xhci_writeq/xhci_readq

[Matthias Brugger]

On 26/05/2020 10:07, Bin Meng wrote:
> Hi Simon,
> 
> On Tue, May 26, 2020 at 5:41 AM Simon Glass <sjg at chromium.org> wrote:
>>
>> Hi Sylwester,
>>
>> On Mon, 25 May 2020 at 11:42, Sylwester Nawrocki <s.nawrocki at samsung.com> wrote:
>>>
>>> Hi Simon,
>>>
>>> On 25.05.2020 19:04, Simon Glass wrote:
>>>> On Mon, 25 May 2020 at 10:57, Sylwester Nawrocki <s.nawrocki at samsung.com> wrote:
>>>>> On 25.05.2020 16:57, Simon Glass wrote:
>>>>>> On Mon, 25 May 2020 at 05:40, Sylwester Nawrocki <s.nawrocki at samsung.com> wrote:
>>>>>>>
>>>>>>> There might be hardware configurations where 64-bit data accesses
>>>>>>> to XHCI registers are not supported properly.  This patch removes
>>>>>>> the readq/writeq so always two 32-bit accesses are used to read/write
>>>>>>> 64-bit XHCI registers, similarly as it is done in Linux kernel.
>>>>>>>
>>>>>>> This patch fixes operation of the XHCI controller on RPI4 Broadcom
>>>>>>> BCM2711 SoC based board, where the VL805 USB XHCI controller is
>>>>>>> connected to the PCIe Root Complex, which is attached to the system
>>>>>>> through the SCB bridge.
>>>>>>>
>>>>>>> Even though the architecture is 64-bit the PCIe BAR is 32-bit and likely
>>>>>>> the 64-bit wide register accesses initiated by the CPU are not properly
>>>>>>> translated to a sequence of 32-bit PCIe accesses.
>>>>>>> xhci_readq(), for example, always returns same value in upper and lower
>>>>>>> 32-bits, e.g. 0xabcd1234abcd1234 instead of 0x00000000abcd1234.
>>>>>
>>>>>> Then I think this should be done with a quirk flag, enabled for this
>>>>>> particular device via the compatible string. It should not be an #if,
>>>>>> but an if().
>>>>>
>>>>> Thanks for your comments. I will check and see how this could be done.
>>>>> It might not be so straightforward since the XHCI controller is a PCI
>>>>> device matched by the pci_device_id so we would need to be looking
>>>>> at the compatible string of the PCI controller to set the quirk in
>>>>> the xhci layer. It's the PCI bridge that introduces the limitation,
>>>>> not the VL805 XHCI controller chip.
>>>>
>>>> OK then it should be modelled as such.
>>>>
>>>> How is this done in Linux?
>>>
>>> In Linux simply always two 32-bit accesses are used for 64-bit registers
>>> read/write.
> 
> This was discussed during review of the previous version of this
> patch, and I think aligning to what Linux does is fine. Previously we
> discussed adding an Kconfig option to control this, but I feel that's
> not good. Having a quirk flag to detect this is a dynamic approach,
> compared to the static Kconfig option, but overall I don't see a need
> not to align with Linux xHCI driver.
> 

My understanding is, that we will keep the approach of 32-bit accesses. My plan
was to take the whole series through my treen.

Bin is that fine with you?

Regards,
Matthias

>>
>> Well the USB maintainer (Marek) might be OK with that, not sure.
>>
>>>
>>> And the quirks in the generic PCI XHCI driver are based on the PCI vendor
>>> and the PCI device ID, so it's not helpful. I couldn't find any reference
>>> to the parent PCI bridge there.
>>
>> In xhci_pci_probe() you can look at the PCI vendor/device  with something like:
>>
>>
>> struct pci_child_platdata *plat = dev_get_parent_platdata(dev);   //
>> see comments for that struct in pci.h
>>
>> int quirks = 0;
>> if (plat->vendor == xxx && plat->device == xxx)
>>    quirks |= SOMETHING
>> xhci_register(...., quirks);  // add a new param
>>
>> in xhci_register() you can store the quirk in ctrl.
>>
>>>
>>>> You can add a quirk in the PCI controller and then XHCI can check its
>>>> parent's platdata to see the flag, perhaps, since the parent will
>>>> always be UCLASS_PCI.
>>>
>>> OK, I imagined something like that.
>>>
>>>> You can always add the device to the devicetree if needed, and then
>>>> you get a compatible string.
>>>
>>> Will have a look, I wasn't aware we could add a node just for such purpose
>>> without negative side effects.
>>
>> So long as you get the 'reg' property correct (i.e. same bus, device,
>> function) then you are OK. See pci-info.rst for docs
>>
> 
> Regards,
> Bin
>