From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752096AbcF1DdM (ORCPT ); Mon, 27 Jun 2016 23:33:12 -0400 Received: from mail-vk0-f65.google.com ([209.85.213.65]:35932 "EHLO mail-vk0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751749AbcF1DdL (ORCPT ); Mon, 27 Jun 2016 23:33:11 -0400 MIME-Version: 1.0 In-Reply-To: <41720b10-3713-9dd6-bf8f-4d32b6283c8c@huawei.com> References: <41720b10-3713-9dd6-bf8f-4d32b6283c8c@huawei.com> From: Pratyush Anand Date: Tue, 28 Jun 2016 09:03:09 +0530 Message-ID: Subject: Re: [PATCH] Decouple CFG and IO in Designware PCIe Driver To: "dongbo (E)" Cc: "jingoohan1@gmail.com" , "bhelgaas@google.com" , "linux-pci@vger.kernel.org" , "linux-kernel@vger.kernel.org" , Linuxarm , Zhanweitao Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Jun 27, 2016 at 6:38 PM, dongbo (E) wrote: > Hi, all. > > How about exchanging the assignments of `MEMORYs' and `CFGs/IOs'? > In other words, assign MEMEORYs to iatu0, CFGs and IOs to iatu1. > > Once the iatu0 is initialized to MEMORY accesses, its BASE_ADDR, > LIMIT and TYPE is fixed. MEMORYs match with iatu0 at first, so > they will never being treated as IOs or CFGs by mistake. This should be acceptable, so you can send it as a formal patch. However, other corner point for failure is still there. Suppose, another thread tries to write on IO space when iatu1 was programmed for CFG. ~Pratyush > > The number of viewpoints used remains two, it would be OK for > platforms only have 2 viewpoints. > > Here is the patch: > > Signed-off-by: Dong Bo > --- > drivers/pci/host/pcie-designware.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c > index aafd766..1bd88d9 100644 > --- a/drivers/pci/host/pcie-designware.c > +++ b/drivers/pci/host/pcie-designware.c > @@ -599,11 +599,11 @@ static int dw_pcie_rd_other_conf(struct pcie_port *pp, struct pci_bus *bus, > va_cfg_base = pp->va_cfg1_base; > } > > - dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX0, > + dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX1, > type, cpu_addr, > busdev, cfg_size); > ret = dw_pcie_cfg_read(va_cfg_base + where, size, val); > - dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX0, > + dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX1, > PCIE_ATU_TYPE_IO, pp->io_base, > pp->io_bus_addr, pp->io_size); > > @@ -636,11 +636,11 @@ static int dw_pcie_wr_other_conf(struct pcie_port *pp, struct pci_bus *bus, > va_cfg_base = pp->va_cfg1_base; > } > > - dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX0, > + dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX1, > type, cpu_addr, > busdev, cfg_size); > ret = dw_pcie_cfg_write(va_cfg_base + where, size, val); > - dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX0, > + dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX1, > PCIE_ATU_TYPE_IO, pp->io_base, > pp->io_bus_addr, pp->io_size); > > @@ -779,7 +779,7 @@ void dw_pcie_setup_rc(struct pcie_port *pp) > * we should not program the ATU here. > */ > if (!pp->ops->rd_other_conf) > - dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX1, > + dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX0, > PCIE_ATU_TYPE_MEM, pp->mem_base, > pp->mem_bus_addr, pp->mem_size); > > -- > 1.9.1 > > On 2016/6/27 12:37, Pratyush Anand wrote: >> On Wed, Jun 22, 2016 at 1:54 PM, dongbo (E) wrote: >>> From: Dong Bo >>> >>> In designware PCIe driver, the iatu0 is used for both CFG and IO accesses. >>> When PCIe sends CFGs to peripherals (e.g. lspci), >>> iatu0 frequently switches between CFG and IO alternatively. >>> >>> If the LIMIT of MEMORY is a value between CFGs BASE_ADDR and IOs LIMIT, >>> this probably results in a MEMORY beging matched to be an IO by mistake. >>> >>> Considering the following configurations: >>> MEMORY -> BASE_ADDR: 0xb4100000, LIMIT: 0xb4100FFF, TYPE=mem >>> CFG -> BASE_ADDR: 0xb4000000, LIMIT: 0xb4000FFF, TYPE=cfg >>> IO -> BASE_ADDR: 0xFFFFFFFF, LIMIT: 0xFFFFFFFE, TYPE=io >>> Suppose PCIe has just completed a CFG access, to switch back to IO, it set the BASE_ADDR to 0xFFFFFFFF, LIMIT 0xFFFFFFFE and TYPE to io. >>> When another CFG access come, >>> PCIe first set BASE_ADDR to 0xb4000000 to switch to CFG. >>> At this moment, a MEMORY access shows up, due to `0xb4000000 <= MEMORY BASE_ADDR <= MEMORY LIMIE <= 0xFFFFFFF, it matches with iatu0. >>> And it is treated as an IO access by mistake, then sent to perpheral. >>> >> >> Hummm...This portion of driver has always been buggy. >> >>> This patch fixes the problem by decoupling CFG and IO, reassigning iatu2 to IO. >> >> But, we can not just assign IOs to iatu2. >> IIRC then, there are atleast two platforms which have only 2 >> viewports, therefore they can not program iatu2. >> >> Jingoo,Bjorn: IMHO, we should modify this portion of code, since more >> number of platforms has 4+ viewports. Probably, we can take following >> approach: >> >> (1) Pass number of viewports through DT. If we have *atleast* 3 >> viewports then assign separate viewports to memory and IO, and share >> one with CFG0 and CFG1. >> (2) If we can have *atleast* 4 then, we may have separate for CFG0 >> and CFG1 as well. >> >> (3) If we have *only* 2 then, either we let them work as they work >> today with bug, or may be we restrict them from using IO transactions. >> So assign one to memory and share other for CFG0 and CFG1. >> >> Please let me know your opnion. >> >> ~Pratyush >> >>> >>> Signed-off-by: Dong Bo >>> --- >>> drivers/pci/host/pcie-designware.c | 14 +++++++------- >>> 1 file changed, 7 insertions(+), 7 deletions(-) >>> >>> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c >>> index aafd766..1a40305 100644 >>> --- a/drivers/pci/host/pcie-designware.c >>> +++ b/drivers/pci/host/pcie-designware.c >>> @@ -51,6 +51,7 @@ >>> #define PCIE_ATU_VIEWPORT 0x900 >>> #define PCIE_ATU_REGION_INBOUND (0x1 << 31) >>> #define PCIE_ATU_REGION_OUTBOUND (0x0 << 31) >>> +#define PCIE_ATU_REGION_INDEX2 (0x2 << 0) >>> #define PCIE_ATU_REGION_INDEX1 (0x1 << 0) >>> #define PCIE_ATU_REGION_INDEX0 (0x0 << 0) >>> #define PCIE_ATU_CR1 0x904 >>> @@ -603,9 +604,6 @@ static int dw_pcie_rd_other_conf(struct pcie_port *pp, struct pci_bus *bus, >>> type, cpu_addr, >>> busdev, cfg_size); >>> ret = dw_pcie_cfg_read(va_cfg_base + where, size, val); >>> - dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX0, >>> - PCIE_ATU_TYPE_IO, pp->io_base, >>> - pp->io_bus_addr, pp->io_size); >>> >>> return ret; >>> } >>> @@ -640,9 +638,6 @@ static int dw_pcie_wr_other_conf(struct pcie_port *pp, struct pci_bus *bus, >>> type, cpu_addr, >>> busdev, cfg_size); >>> ret = dw_pcie_cfg_write(va_cfg_base + where, size, val); >>> - dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX0, >>> - PCIE_ATU_TYPE_IO, pp->io_base, >>> - pp->io_bus_addr, pp->io_size); >>> >>> return ret; >>> } >>> @@ -778,10 +773,15 @@ void dw_pcie_setup_rc(struct pcie_port *pp) >>> * uses its own address translation component rather than ATU, so >>> * we should not program the ATU here. >>> */ >>> - if (!pp->ops->rd_other_conf) >>> + if (!pp->ops->rd_other_conf) { >>> dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX1, >>> PCIE_ATU_TYPE_MEM, pp->mem_base, >>> pp->mem_bus_addr, pp->mem_size); >>> + dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX2, >>> + PCIE_ATU_TYPE_IO, pp->io_base, >>> + pp->io_bus_addr, pp->io_size); >>> + >>> + } >>> >>> dw_pcie_wr_own_conf(pp, PCI_BASE_ADDRESS_0, 4, 0); >>> >>> -- >>> 1.9.1 >>> >> >> . >> >