Hi all,

Le 06/12/2017 à 12:32, Lorenzo Pieralisi a écrit :

On Mon, Dec 04, 2017 at 06:49:12PM +0000, Ard Biesheuvel wrote:

[...]

quoted

quoted

quoted

quoted

quoted

+static int cdns_pcie_host_init_root_port(struct cdns_pcie_rc *rc)
+{
+     const struct cdns_pcie_rc_data *data = rc->data;
+     struct cdns_pcie *pcie = &rc->pcie;
+     u8 pbn, sbn, subn;
+     u32 value, ctrl;
+
+     /*
+      * Set the root complex BAR configuration register:
+      * - disable both BAR0 and BAR1.
+      * - enable Prefetchable Memory Base and Limit registers in type 1
+      *   config space (64 bits).
+      * - enable IO Base and Limit registers in type 1 config
+      *   space (32 bits).
+      */
+     ctrl = CDNS_PCIE_LM_BAR_CFG_CTRL_DISABLED;
+     value = CDNS_PCIE_LM_RC_BAR_CFG_BAR0_CTRL(ctrl) |
+             CDNS_PCIE_LM_RC_BAR_CFG_BAR1_CTRL(ctrl) |
+             CDNS_PCIE_LM_RC_BAR_CFG_PREFETCH_MEM_ENABLE |
+             CDNS_PCIE_LM_RC_BAR_CFG_PREFETCH_MEM_64BITS |
+             CDNS_PCIE_LM_RC_BAR_CFG_IO_ENABLE |
+             CDNS_PCIE_LM_RC_BAR_CFG_IO_32BITS;
+     cdns_pcie_writel(pcie, CDNS_PCIE_LM_RC_BAR_CFG, value);
+
+     /* Set root port configuration space */
+     if (data->vendor_id != 0xffff)
+             cdns_pcie_rp_writew(pcie, PCI_VENDOR_ID, data->vendor_id);
+     if (data->device_id != 0xffff)
+             cdns_pcie_rp_writew(pcie, PCI_DEVICE_ID, data->device_id);
+
+     cdns_pcie_rp_writeb(pcie, PCI_CLASS_REVISION, 0);
+     cdns_pcie_rp_writeb(pcie, PCI_CLASS_PROG, 0);
+     cdns_pcie_rp_writew(pcie, PCI_CLASS_DEVICE, PCI_CLASS_BRIDGE_PCI);
+
+     pbn = rc->bus_range->start;
+     sbn = pbn + 1; /* Single root port. */
+     subn = rc->bus_range->end;
+     cdns_pcie_rp_writeb(pcie, PCI_PRIMARY_BUS, pbn);
+     cdns_pcie_rp_writeb(pcie, PCI_SECONDARY_BUS, sbn);
+     cdns_pcie_rp_writeb(pcie, PCI_SUBORDINATE_BUS, subn);

Again - I do not have the datasheet for this device therefore I would
kindly ask you how this works; it seems to me that what you are doing
here is done through normal configuration cycles in an ECAM compliant
system to program the RP PRIMARY/SECONDARY/SUBORDINATE bus - I would
like to understand why this code is needed.

I will test without those lines to test whether I can remove them.

At first, the PCIe controller was tested by Cadence team: there was code
in their bootloader to initialize the hardware (building the AXI <-> PCIe
mappings, ...): the bootloader used to set the primary, secondary and
subordinate bus numbers in the root port PCI config space.

Also there was a hardware trick to redirect accesses of the lowest
addresses in the AXI bus to the APB bus so the PCI configuration space of
the root port could have been accessed from the AXI bus too.

The AXI <-> PCIe mapping being done by the bootloader and the root port
config space being accessible from the AXI bus, it was possible to use
the pci-host-generic driver.

That's what I was getting at. Ard (CC'ed) implemented a firmware set-up
(even though it was for a different IP but maybe it applies here) that
allows the kernel to use the pci-host-generic driver to initialize the
PCI controller:

https://marc.info/?l=linux-pci&m=150360022626351&w=2

I want to understand if there is an IP initialization sequence whereby
this IP can be made to work in an ECAM compliant way and therefore
reuse (most of) the pci-host-generic driver code.

I think the Synopsys case is probably very similar. There are some
registers that look like the config space of a root port, but in
reality, every memory access that hits a live host bridge window is
forwarded onto the link, regardless of the values of the bridge BARs.
That is why in the quoted case, we can get away with ignoring the root
port altogether, rather than jumping through hoops to make the IP
block's PCI config space registers appear at B/D/F 0/0/0, while still
having to filter type 0 config TLPs going onto the link (which is
arguably the job of the root port to begin with)

So if this IP does implement a proper root port (i.e., one where the
bridge BARs are actually taken into account, and where type 0 config
TLPs are in fact filtered), I strongly recommend mapping its config
space registers in an ECAM compliant matter, which implies no
accessors in the OS.

However, given the observation above, this IP does not appear to
filter type 0 config TLPs to devfn > 0 downstream of the root port
either.

Unfortunately that matches my understanding too, let's wait for
Cyrille's reply on my query.

quoted

quoted

quoted

However, the hardware trick won't be included in the final design since
Cadence now wants to perform all PCI configuration space accesses through
a small 4KB window at a fixed address on the AXI bus.

I would like to understand what the HW "trick" (if you can disclose it)
was, because if there is a chance to reuse the pci-host-generic driver
for this IP I want to take it (yes it may entail some firmware set-up in
the bootloader) - was it a HW trick or a specific IP SW configuration ?

I will have to ask for details to Cadence designers if needed but when I
asked them about it, they explained me that AXI bus accesses in a small
window (I guess 4KB width) were redirected to the APB bus where lay the
registers for the root port PCI configuration space.

I was some hardware trick which won't be included in the final design, so
we can't enable or disable it by software.

Actually, this is requirement from the Cadence's customer that the host
driver can access the PCI config space of any device in sub ordinates
buses through a small memory area on the AXI bus. For some reason, they
don't want an ECAM compliant controller.

I don't know the reason but my guess is that they don't want to waste to
much space allocated to the PCIe controller on the AXI bus, likely on a
32-bit platform. As I said, this is such an assumption.

quoted

quoted

quoted

Also, we now want all initialisations to be done by the linux driver
instead of the bootloader.

That's a choice, I do not necessarily agree with it and I think we
should aim for more standardization on the PCI host bridge set-up
at firmware->kernel handover on DT platforms.

It was another requirement of Cadence's customer that the PCIe host
controller initialization is done by the Linux driver rather than by
some boot loader.

Best regards,

Cyrille

quoted

Well, for one, it means this IP will never be supported by ACPI, which
seems like a huge downside to me.

Yes it is - that's exactly where my comments were heading.

Thanks,
Lorenzo

-- 
Cyrille Pitchen, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com