Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Sat, 09 May 2020 01:28:24 +0800
From:   Jiaxun Yang <jiaxun.yang@flygoat.com>
To:     Bjorn Helgaas <helgaas@kernel.org>
CC:     maz@kernel.org, Rob Herring <robh@kernel.org>,
        Bjorn Helgaas <bhelgaas@google.com>,
        Rob Herring <robh+dt@kernel.org>,
        Thomas Bogendoerfer <tsbogend@alpha.franken.de>,
        Huacai Chen <chenhc@lemote.com>,
        Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
        Paul Burton <paulburton@kernel.org>, linux-pci@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-mips@vger.kernel.org
Subject: Re: [PATCH v8 2/5] PCI: Add Loongson PCI Controller support
User-Agent: K-9 Mail for Android
Reply-to: jiaxun.yang@flygoat.com
In-Reply-To: <20200508171730.GA77036@bjorn-Precision-5520>
References: <20200508171730.GA77036@bjorn-Precision-5520>
Message-ID: <2C23FC3E-6BBF-47EB-8EE6-89EAC5A28104@flygoat.com>
MIME-Version: 1.0
Content-Type: text/plain;
 charset=utf-8
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


=E4=BA=8E 2020=E5=B9=B45=E6=9C=889=E6=97=A5 GMT+08:00 =E4=B8=8A=E5=8D=881:=
17:30, Bjorn Helgaas <helgaas@kernel=2Eorg> =E5=86=99=E5=88=B0:
>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=2E
>>=20
>> The RS780E part of code was previously located at
>> arch/mips/pci/ops-loongson3=2Ec and now it can use generic PCI
>> driver implementation=2E
>>=20
>> Signed-off-by: Jiaxun Yang <jiaxun=2Eyang@flygoat=2Ecom>
>> Reviewed-by: Rob Herring <robh@kernel=2Eorg>
>
>> +static void system_bus_quirk(struct pci_dev *pdev)
>> +{
>> +	u16 tmp;
>> +
>> +	/*=20
>> +	 * System buses on Loongson system contain garbage in BARs
>> +	 * but their decoding need to be enabled to ensure devices
>> +	 * under system buses are reachable=2E In most cases it should
>> +	 * be done by the firmware=2E
>
>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=2E
>
>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=2E

Hi,
Sorry, but that's all I can tell from the chips manual as I'm not a employ=
ee
of the vendor=2E

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

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

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

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

>
>> +	pdev->mmio_always_on =3D 1;
>> +	pdev->non_compliant_bars =3D 1;
>> +	/* Enable MEM & IO Decoding */
>> +	pci_read_config_word(pdev, PCI_COMMAND, &tmp);
>> +	tmp |=3D PCI_COMMAND_IO | PCI_COMMAND_MEMORY;
>> +	pci_write_config_word(pdev, PCI_COMMAND, tmp);
>
>
>> +}
>> +
>
>Omit this blank line=2E

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 =3D dev->bus;
>> +	struct pci_dev *bridge;
>> +	static const struct pci_device_id bridge_devids[] =3D {
>> +		{ 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=2E

Ack

>
>> +	/* look for the matching bridge */
>> +	while (!pci_is_root_bus(bus)) {
>> +		bridge =3D bus->self;
>> +		bus =3D bus->parent;
>> +		/*
>> +		 * Some Loongson PCIe ports have a h/w limitation of
>> +		 * 256 bytes maximum read request size=2E They can't handle
>> +		 * anything larger than this=2E So force this limit on
>> +		 * any devices attached under these ports=2E
>> +		 */
>> +		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 d=
evfn,
>> +			       int where)
>> +{
>> +	unsigned char busnum =3D bus->number;
>> +	struct pci_host_bridge *bridge =3D pci_find_host_bridge(bus);
>> +	struct loongson_pci *priv =3D  pci_host_bridge_priv(bridge);
>> +
>> +	/*
>> +	 * Do not read more than one device on the bus other than
>> +	 * the host bridge=2E
>
>s/host bridge/root bus/ ?
>
>IIUC, the test below assumes the root bus is bus 0, which is not
>necessarily the case=2E  Many other =2E*_map_bus() implementations have
>similar tests for devices on the root bus:

Ack

>
>  al_pcie_map_bus(=2E=2E=2E)
>  {
>    if (bus->number =3D=3D cfg->busr=2Estart) {
>
>> +	if (priv->flags & FLAG_DEV_FIX && bus->primary !=3D 0 &&
>> +		PCI_SLOT(devfn) > 0)
>> +		return NULL;

Thanks!

--=20
Jiaxun Yang