Re: [PATCH] PCI: apple: Configure link speeds properly

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 24/11/2021 11.23, Rob Herring wrote:
+#include "../pci.h"
+/* Apple PCIe is based on DesignWare IP and shares some registers */
+#include "dwc/pcie-designware.h"

I'm starting to regret this not being part of the DWC driver...

Main issue is the DWC driver seems to have a pretty hard-coded assumption of one port per controller, plus does a bunch of stuff differently for the higher layers. It seems Apple used the DWC PHY/LTSSM bits, then rolled their own upper layer.

+/* The offset of the PCIe capabilities structure in bridge config space */
+#define PCIE_CAP_BASE          0x70

This offset is discoverable, so don't hardcode it.

Sure, it just means I have to reinvent the PCI capability lookup wheel again. I'd love to use the regular accessors, but the infrastructure isn't up to the point where we can do that yet yere. DWC also reinvents this wheel, but we can't reuse that code because it pokes these registers through a separate reg range, not config space (even though it seems like they should be the same thing? I'm not sure what's going on in the DWC devices... for the Apple controller it's just the ECAM).

+       max_gen = of_pci_get_max_link_speed(port->np);
+       if (max_gen < 0) {
+               dev_err(port->pcie->dev, "max link speed not specified\n");

Better to fail than limp along in gen1? Though you don't check the
return value...

Usually, the DT property is there to limit the speed when there's a
board limitation.

The default *setting* is actually Gen4, but without PCIE_LINK_WIDTH_SPEED_CONTROL poked it always trains at Gen1. Might make more sense to only set the LNKCTL field if max-link-speed is specified, and unconditionally poke that bit. That'll get us Gen4 by default (or even presumably Gen5 in future controllers, if everything else stays compatible).

--
Hector Martin (marcan@xxxxxxxxx)
Public Key: https://mrcn.st/pub



[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux