From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752431AbcF1NuK (ORCPT ); Tue, 28 Jun 2016 09:50:10 -0400 Received: from mail-pa0-f66.google.com ([209.85.220.66]:35449 "EHLO mail-pa0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752373AbcF1NuG (ORCPT ); Tue, 28 Jun 2016 09:50:06 -0400 From: "Jingoo Han" To: "'Pratyush Anand'" , "'dongbo \(E\)'" , Cc: , , "'Linuxarm'" , "'Zhanweitao'" , "'Jingoo Han'" References: In-Reply-To: Subject: Re: [PATCH] Decouple CFG and IO in Designware PCIe Driver Date: Tue, 28 Jun 2016 22:50:01 +0900 Message-ID: <002801d1d143$fa837bf0$ef8a73d0$@com> MIME-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit X-Mailer: Microsoft Office Outlook 12.0 Thread-Index: AdHQLakfK+5V3JYmTaSS0/An+rUGhABFXVfQ Content-Language: ko Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Monday, June 27, 2016 1:38 PM, 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. I agree with your opinion. The number of viewports should be passed through DT, because this number is designated by hardware configuration at the level of design. Anyway, I think that most SoCs using Designware PCIe would support more than 3 view points. So, the current code should be modified in order to support more view points. Thank you. Best regards, Jingoo Han > > ~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 > >