Re: [PATCH v8 2/5] PCI: Add Loongson PCI Controller support

Bjorn Helgaas
2020-05-08T17:17:34Z

On Fri, May 08, 2020 at 07:34:02PM +0800, Jiaxun Yang wrote:
> This controller can be found on Loongson-2K SoC, Loongson-3
> systems with RS780E/LS7A PCH.
>
> The RS780E part of code was previously located at
> arch/mips/pci/ops-loongson3.c and now it can use generic PCI
> driver implementation.
>
> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
> Reviewed-by: Rob Herring <robh@kernel.org>

> +static void system_bus_quirk(struct pci_dev *pdev)
> +{
> + u16 tmp;
> +
> + /*
> + * System buses on Loongson system contain garbage in BARs
> + * but their decoding need to be enabled to ensure devices
> + * under system buses are reachable. In most cases it should
> + * be done by the firmware.

This isn't a very satisfying explanation because devices that have
decoding enabled can interfere with other devices in the system, and I
can't tell whether that's a problem here.

What happens when you turn on MEM/IO decoding below? Does the device
decode any address space? How do we know what it is? Is it related
to the BAR contents?

I'm a little dubious about the need for the PCI_COMMAND write because
the previous version didn't do it (since it incorrectly wrote to
PCI_STATUS), and I assume that version worked.

> + pdev->mmio_always_on = 1;
> + pdev->non_compliant_bars = 1;
> + /* Enable MEM & IO Decoding */
> + pci_read_config_word(pdev, PCI_COMMAND, &tmp);
> + tmp |= PCI_COMMAND_IO | PCI_COMMAND_MEMORY;
> + pci_write_config_word(pdev, PCI_COMMAND, tmp);

> +}
> +

Omit this blank line.

> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON,
> + DEV_LS2K_APB, system_bus_quirk);
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON,
> + DEV_LS7A_CONF, system_bus_quirk);
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON,
> + DEV_LS7A_LPC, system_bus_quirk);
> +
> +static void loongson_mrrs_quirk(struct pci_dev *dev)
> +{
> + struct pci_bus *bus = dev->bus;
> + struct pci_dev *bridge;
> + static const struct pci_device_id bridge_devids[] = {
> + { PCI_VDEVICE(LOONGSON, DEV_PCIE_PORT_0) },
> + { PCI_VDEVICE(LOONGSON, DEV_PCIE_PORT_1) },
> + { PCI_VDEVICE(LOONGSON, DEV_PCIE_PORT_2) },
> + { 0, },
> + };
> +
> +

Remove one of these blank lines.

> + /* look for the matching bridge */
> + while (!pci_is_root_bus(bus)) {
> + bridge = bus->self;
> + bus = bus->parent;
> + /*
> + * Some Loongson PCIe ports have a h/w limitation of
> + * 256 bytes maximum read request size. They can't handle
> + * anything larger than this. So force this limit on
> + * any devices attached under these ports.
> + */
> + if (pci_match_id(bridge_devids, bridge)) {
> + if (pcie_get_readrq(dev) > 256) {
> + pci_info(dev, "limiting MRRS to 256\n");
> + pcie_set_readrq(dev, 256);
> + }
> + break;
> + }
> + }
> +}
> +DECLARE_PCI_FIXUP_ENABLE(PCI_ANY_ID, PCI_ANY_ID, loongson_mrrs_quirk);

> +void __iomem *pci_loongson_map_bus(struct pci_bus *bus, unsigned int devfn,
> + int where)
> +{
> + unsigned char busnum = bus->number;
> + struct pci_host_bridge *bridge = pci_find_host_bridge(bus);
> + struct loongson_pci *priv = pci_host_bridge_priv(bridge);
> +
> + /*
> + * Do not read more than one device on the bus other than
> + * the host bridge.

s/host bridge/root bus/ ?

IIUC, the test below assumes the root bus is bus 0, which is not
necessarily the case. Many other .*_map_bus() implementations have
similar tests for devices on the root bus:

al_pcie_map_bus(...)
{
if (bus->number == cfg->busr.start) {

> + if (priv->flags & FLAG_DEV_FIX && bus->primary != 0 &&
> + PCI_SLOT(devfn) > 0)
> + return NULL;