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

[Simon Glass]

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.

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