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

[Bin Meng]

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.

>
> 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