Re: [RESEND: RFC PATCH 3/3] pcie: keystone: add pcie driver based on designware core driver

From: Murali Karicheri
Date: Mon Apr 07 2014 - 12:39:24 EST


On 3/25/2014 12:54 PM, Jason Gunthorpe wrote:
On Tue, Mar 25, 2014 at 08:44:36AM +0100, Arnd Bergmann wrote:

I have no idea how this would work with the standard interrupt-map property,
since the legacy interrupt host is now the same device as the pci host.

Maybe it's better to move the legacy irqchip handling entirely out of
the driver and use a separate device node for the registers so it can
come with its own #interrupt-cells, and then refer to this irqchip from
the interrupt-map.
The other DW PCI-E drivers are being fixed to use interrupt-map, so I
think this driver should be fixed before it goes in as well.
Jason,

Could you send me a patch reference or branch that I can review to better understand
changes required in my driver to use the bindings you have described below?

Murali
Other PCI-E hosts have handled this particular problem with a
construction like this:

pcie@21800000 {
compatible = "ti,keystone2-pcie";
device_type = "pci";
#address-cells = <3>;
#size-cells = <2>;
#interrupt-cells = <1>;
reg = ..

pcie_intc: interrupt-controller {
interrupt-controller;
#address-cells = <0>;
#interrupt-cells = <1>;
}

interrupts = ... enough to decode INTx;

interrtup-map = <0 0 0 1 &pcie_intc 1>, // INT A
<0 0 0 2 &pcie_intc 2>, // INT B
<0 0 0 3 &pcie_intc 3>, // INT C
<0 0 0 4 &pcie_intc 4>; // INT D
}

When the legacy irq domain is created it should be bound to the
pcie_intc node, not to pcie node.

ks_pcie_map_irq should be removed.
The latest dw core driver still had map_irq() being provided by the driver.
So any patch reference will help me understand that you describe here.

Thanks

Murali

+#ifdef CONFIG_PCIE_KEYSTONE
+/*
+ * The KeyStone PCIe controller has maximum read request size of 256 bytes.
+ * Force this configuration for all EP including bridges.
+ */
+static void quirk_limit_readrequest(struct pci_dev *dev)
+{
+ pcie_set_readrq(dev, 256);
+}
+DECLARE_PCI_FIXUP_FINAL(PCI_ANY_ID, PCI_ANY_ID, quirk_limit_readrequest);
+#endif /* CONFIG_TI_KEYSTONE_PCIE */
You can't do this:

A quirk for a specific hardware must not be enabled by compile-time options.
You have to find a different way to do this.
This configuration should happen automatically by the PCI core if the
config spaces are correct. See pcie_write_mrrs, pcie_write_mps,
pcie_bus_configure_settings, etc.

Your host will need to conform to the PCI spec and provide a root
port bridge header for this to work.

Also, even in the keystone case the MRRS should not be globally forced
like this, there may be other bridges in the system with a 128 byte
MPS.

Jason

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/