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

[Jason Gunthorpe]

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.

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.

> > +#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/