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

[Jiaxun Yang]

ä 2020å5æ9æ GMT+08:00 äå1:17:30, Bjorn Helgaas <helgaas@xxxxxxxxxx> åå:
>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@xxxxxxxxxxx>
>> Reviewed-by: Rob Herring <robh@xxxxxxxxxx>
>
>> +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.

Hi,
Sorry, but that's all I can tell from the chips manual as I'm not a employee
of the vendor.

My assumption is these BAR contains the address of those system components
that already configured by firmware and we shouldn't touch it.

In fact according to my tests if we let Kernel probe these BAR then the
system will hang immediately.

Chip manual suggested OS to ensure decoding is enabled
so I'm doing like this. But without this system can also work.

Do you think I should drop it until figure out what was it actually doing?

>
>> +	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.

Ack

>
>> +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.

Ack

>
>> +	/* 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:

Ack

>
>  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;

Thanks!

-- 
Jiaxun Yang