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
