Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753135AbdL1Bjf (ORCPT ); Wed, 27 Dec 2017 20:39:35 -0500 Received: from Mailgw01.mediatek.com ([1.203.163.78]:25584 "EHLO mailgw01.mediatek.com" rhost-flags-OK-FAIL-OK-FAIL) by vger.kernel.org with ESMTP id S1752757AbdL1Bjd (ORCPT ); Wed, 27 Dec 2017 20:39:33 -0500 X-UUID: e37ac74c5fdb4188935ff91c875c739b-20171228 Message-ID: <1514425152.25872.39.camel@mhfsdcap03> Subject: Re: [PATCH v5 2/2] PCI: mediatek: Set up class type and vendor ID for MT7622 From: Honghui Zhang To: Bjorn Helgaas CC: , , , , , , , , , , , , , , , , Date: Thu, 28 Dec 2017 09:39:12 +0800 In-Reply-To: <20171227184542.GA79892@bhelgaas-glaptop.roam.corp.google.com> References: <1514336394-17747-1-git-send-email-honghui.zhang@mediatek.com> <1514336394-17747-3-git-send-email-honghui.zhang@mediatek.com> <20171227184542.GA79892@bhelgaas-glaptop.roam.corp.google.com> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.2.3-0ubuntu6 Content-Transfer-Encoding: 7bit MIME-Version: 1.0 X-MTK: N Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4240 Lines: 115 On Wed, 2017-12-27 at 12:45 -0600, Bjorn Helgaas wrote: > On Wed, Dec 27, 2017 at 08:59:54AM +0800, honghui.zhang@mediatek.com wrote: > > From: Honghui Zhang > > > > The hardware default value of IDs and class type is not correct, > > fix that by setup the correct values before start up. > > > > Signed-off-by: Honghui Zhang > > --- > > drivers/pci/host/pcie-mediatek.c | 12 ++++++++++++ > > include/linux/pci_ids.h | 2 ++ > > 2 files changed, 14 insertions(+) > > > > diff --git a/drivers/pci/host/pcie-mediatek.c b/drivers/pci/host/pcie-mediatek.c > > index fc29a9a..62aac0ea 100644 > > --- a/drivers/pci/host/pcie-mediatek.c > > +++ b/drivers/pci/host/pcie-mediatek.c > > @@ -74,6 +74,10 @@ > > > > /* PCIe V2 per-port registers */ > > #define PCIE_MSI_VECTOR 0x0c0 > > + > > +#define PCIE_CONF_ID 0x100 > > +#define PCIE_CONF_CLASS 0x104 > > + > > #define PCIE_INT_MASK 0x420 > > #define INTX_MASK GENMASK(19, 16) > > #define INTX_SHIFT 16 > > @@ -393,6 +397,14 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port) > > val |= PCIE_CSR_LTSSM_EN(port->slot) | > > PCIE_CSR_ASPM_L1_EN(port->slot); > > writel(val, pcie->base + PCIE_SYS_CFG_V2); > > + > > + /* Set up vendor ID and device ID for MT7622*/ > > + val = PCI_VENDOR_ID_MEDIATEK; > > + writel(val, port->base + PCIE_CONF_ID); > > + > > + /* Set up class code for MT7622 */ > > + val = PCI_CLASS_BRIDGE_PCI << 16; > > + writel(val, port->base + PCIE_CONF_CLASS); > > 1) Your comments mention MT7622 specifically, but this code is run for > both mt2712-pcie and mt7622-pcie. If this code is safe and necessary > for both mt2712-pcie and mt7622-pcie, please remove the mention of > MT7622. Hmm, the code snippet added here will only be executed by MT7622, since MT2712 will not enter this "if (pcie->base) {" condition. Should the mention of MT7622 must be removed in this case? > > 2) The first comment mentions both "vendor ID and device ID" but you > don't write the device ID. Since this code applies to both > mt2712-pcie and mt7622-pcie, my guess is that you don't *want* to > write the device ID. If that's the case, please fix the comment. > My bad, I did not check the comments carefully. Thanks. > 3) If you only need to set the vendor ID, you're performing a 32-bit > write (writel()) to update a 16-bit value. Please use writew() > instead. > Ok, thanks, I guess I could use the following code snippet in the next version: val = readl(port->base + PCIE_CONF_VENDOR_ID) val &= ~GENMASK(15, 0); val |= PCI_VENDOR_ID_MEDIATEK; writel(val, port->base + PCIE_CONF_VENDOR_ID); > 4) If you only need to set the vendor ID, please use a definition like > "PCIE_CONF_VENDOR_ID" instead of the ambiguous "PCIE_CONF_ID". > > 5) If you only need to set the vendor ID, please update the changelog > to mention "vendor ID" specifically instead of the ambiguous "IDs". > 6) Please add a space before the closing "*/" of the first comment. > > 7) PCI_CLASS_BRIDGE_PCI is for a PCI-to-PCI bridge, i.e., one that has > PCI on both the primary (upstream) side and the secondary (downstream) > side. That kind of bridge has a type 1 config header (see > PCI_HEADER_TYPE) and the PCI_PRIMARY_BUS and PCI_SECONDARY_BUS > registers tell us the bus number of the primary and secondary sides. > > I don't believe this device is a PCI-to-PCI bridge. I think it's a > *host* bridge that has some non-PCI interface on the upstream side and > should have a type 0 config header. If that's the case you should use > PCI_CLASS_BRIDGE_HOST instead. > Thanks very much for your help with the review, I will fix the other issue in the next version. > > } > > > > /* Assert all reset signals */ > > diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h > > index ab20dc5..2480b0e 100644 > > --- a/include/linux/pci_ids.h > > +++ b/include/linux/pci_ids.h > > @@ -2113,6 +2113,8 @@ > > > > #define PCI_VENDOR_ID_MYRICOM 0x14c1 > > > > +#define PCI_VENDOR_ID_MEDIATEK 0x14c3 > > + > > #define PCI_VENDOR_ID_TITAN 0x14D2 > > #define PCI_DEVICE_ID_TITAN_010L 0x8001 > > #define PCI_DEVICE_ID_TITAN_100L 0x8010 > > -- > > 2.6.4 > >