From patchwork Tue Dec 3 20:14:47 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Tim Harvey X-Patchwork-Id: 3278621 X-Patchwork-Delegate: bhelgaas@google.com Return-Path: X-Original-To: patchwork-linux-pci@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 6BB59C0D4A for ; Tue, 3 Dec 2013 20:15:00 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 99A2720460 for ; Tue, 3 Dec 2013 20:14:56 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id C521420458 for ; Tue, 3 Dec 2013 20:14:54 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753755Ab3LCUOy (ORCPT ); Tue, 3 Dec 2013 15:14:54 -0500 Received: from mail-we0-f182.google.com ([74.125.82.182]:49753 "EHLO mail-we0-f182.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755140Ab3LCUOw (ORCPT ); Tue, 3 Dec 2013 15:14:52 -0500 Received: by mail-we0-f182.google.com with SMTP id q59so14343066wes.27 for ; Tue, 03 Dec 2013 12:14:50 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type; bh=/VwunmK1TpkT7/N9kEZkXJpAGF8CxTMwAC97tp24b1Q=; b=Ym+nU9ZKmKPNBLv9kjnNl/LHAD6++q7zpD/Ibv3hzNv7ZRhZVhiOLsbwdCDEIvMRrn 1OpXqz5Wfe/bhaGoBNHonbg/6F3hIwD1U0WxJvuyKTuVenpzFcK+5fPT2wIr7t01eKG/ +BLeB0RnzTTsq7OctaTNxXpUPGwHcb9vrndkQZ2WZ3IqzIH858g2EKF1tHmbr4KNurH1 n5q6GT3w833d5JWDVGqRqRxb+Mb4/UtcZw9NiBpYfryQabgT56+Y2eKksxcTt097E1Lm m1HU7dFx4321wslyhy6+cnLF3ypTbOH+NF9OUGqNHHubR/gdJ7u/enTqODX90JqJrh9Q MHSg== X-Gm-Message-State: ALoCoQkDOG0ybHJ1ZWofHGKfGtOJ9tTx3AZc/urgGC/nyUPyUjf+rK1xig25D7eUvC3BQB/W9+/K MIME-Version: 1.0 X-Received: by 10.180.13.142 with SMTP id h14mr4152357wic.3.1386101687351; Tue, 03 Dec 2013 12:14:47 -0800 (PST) Received: by 10.194.139.114 with HTTP; Tue, 3 Dec 2013 12:14:47 -0800 (PST) In-Reply-To: <201311270944.09383.marex@denx.de> References: <1385500248-6551-1-git-send-email-marex@denx.de> <1385500248-6551-2-git-send-email-marex@denx.de> <20131127043009.GB12773@pratyush-vbox> <201311270944.09383.marex@denx.de> Date: Tue, 3 Dec 2013 12:14:47 -0800 Message-ID: Subject: Re: [PATCH 2/7] [RFC] PCI: imx6: remove outbound io/mem ATU region mapping From: Tim Harvey To: Marek Vasut Cc: Pratyush Anand , "linux-arm-kernel@lists.infradead.org" , "linux-pci@vger.kernel.org" , Bjorn Helgaas , Frank Li , Harro Haan , Jingoo Han , Mohit KUMAR DCG , Richard Zhu , Sascha Hauer , Sean Cross , Shawn Guo , Siva Reddy Kallam , Srikanth T Shivanand , Troy Kisky , Yinghai Lu Sender: linux-pci-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pci@vger.kernel.org X-Spam-Status: No, score=-6.9 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Wed, Nov 27, 2013 at 12:44 AM, Marek Vasut wrote: > Dear Pratyush Anand, > >> Dear Marek Vasut, >> >> Please replace imx6 with designware in subject line of this patch. > > OK, this one has to be fixed anyway. We need to figure out why do we even need > this one. > >> On Wed, Nov 27, 2013 at 05:10:43AM +0800, Marek Vasut wrote: >> > From: Tim Harvey >> > >> > The IMX6 iATU is used for address translation between the AXI bus >> > address space and PCI address space. This is used for type0 and type1 >> > config cycles but is not necessary for outbound io/mem regions. >> > >> > This patch removes the calls that inappropriately re-configures the ATU >> > viewport for outbound memory and IO after config cycles and removes them >> > altogether as they are not necessary. >> >> Yes, they are not necessary for all the cases where translation is one >> to one. So so for sure all the platform till now introduced should >> work. >> But, what about a platform where memory translation is not one to one? >> >> Existing code should work with all sort of memory translation on a >> platform having atleast two viewports capable of programming any type >> of outbound transaction. > > Full ACK, that's why it's called RFC. > >> > This resolves issues with PCI devices behind switches and has been tested >> > with a Gige device behind a PLX PEX860x switch. More testing is needed >> > for other configurations. >> >> I do not understand if MX6 has 4 Outbound Viewport then how this patch >> helps? >> -- PCI devices behind switches would not have been working because >> CFG1 transaction would not have been correct. >> -- It works with this patch. This patch changes viewport for CFG1 from >> 1 to 0. >> -- Can it be possible that MX6 has some restriction on viewport >> programming capability. I mean,like only viewport0 can be programmed >> for CFG0/1? > > Tim ? > > Here is the MX6 datasheet [1], the section 48.3.9.1.1 and 48.3.9.1.2 describe > the iATU configuration on MX6. My understanding from this description is that > the MX6 has 4 inbound and 4 outbound iATU regions. Am I wrong ? > > [1] http://cache.freescale.com/files/32bit/doc/ref_manual/IMX6DQRM.pdf > >> Otherwise I do not find any convincing reason that why this patch >> helps in resolving issues with PCI devices behind switches. In my configuration I have a PLX switch off the IMX root complex with several devices behind it. I find that the devices enumerate correctly but as soon as data is transferred to or from (not sure which) the device the system hangs (INFO: rcu_sched detected stalls on CPUs/tasks: { 0} (detected by 3, t=2135 jiffies, g=20, c=19, q=66)). My current test case is a GigE ethernet device and when I connect a network to the device I hange when a packet is responded to. I can't claim to fully understand PCI resource mappings or the iATU and I don't understand why dw_pcie_rd_other_conf/dw_pcie_wr_other_conf need to change the viewport then change it back instead of using multiple viewports (perhaps because some hardware may not have more than the 2 viewports currently being used?). The current driver uses viewport0 for cfg0 and mem and viewport1 for cfg1 and io. If I remove the call to dw_pcie_prog_viewport_io_outbound to reconfigure viewport1 for io after its altered for type1 cfg cycles, devices behind the bridge work for me: perhaps break devices that use IO resources? Thanks, Tim >> >> >> Regards >> Pratyush >> >> > From: Tim Harvey >> > Signed-off-by: Tim Harvey >> > Cc: Bjorn Helgaas >> > Cc: Frank Li >> > Cc: Harro Haan >> > Cc: Jingoo Han >> > Cc: Mohit KUMAR >> > Cc: Pratyush Anand >> > Cc: Richard Zhu >> > Cc: Sascha Hauer >> > Cc: Sean Cross >> > Cc: Shawn Guo >> > Cc: Siva Reddy Kallam >> > Cc: Srikanth T Shivanand >> > Cc: Tim Harvey >> > Cc: Troy Kisky >> > Cc: Yinghai Lu >> > --- >> > >> > drivers/pci/host/pcie-designware.c | 41 >> > +++----------------------------------- 1 file changed, 3 insertions(+), >> > 38 deletions(-) >> > >> > diff --git a/drivers/pci/host/pcie-designware.c >> > b/drivers/pci/host/pcie-designware.c index ed9b11b..01d76ad 100644 >> > --- a/drivers/pci/host/pcie-designware.c >> > +++ b/drivers/pci/host/pcie-designware.c >> > @@ -46,7 +46,6 @@ >> > >> > #define PCIE_ATU_VIEWPORT 0x900 >> > #define PCIE_ATU_REGION_INBOUND (0x1 << 31) >> > #define PCIE_ATU_REGION_OUTBOUND (0x0 << 31) >> > >> > -#define PCIE_ATU_REGION_INDEX1 (0x1 << 0) >> > >> > #define PCIE_ATU_REGION_INDEX0 (0x0 << 0) >> > #define PCIE_ATU_CR1 0x904 >> > #define PCIE_ATU_TYPE_MEM (0x0 << 0) >> > >> > @@ -494,8 +493,8 @@ static void dw_pcie_prog_viewport_cfg0(struct >> > pcie_port *pp, u32 busdev) >> > >> > static void dw_pcie_prog_viewport_cfg1(struct pcie_port *pp, u32 busdev) >> > { >> > >> > - /* Program viewport 1 : OUTBOUND : CFG1 */ >> > - dw_pcie_writel_rc(pp, PCIE_ATU_REGION_OUTBOUND | >> > PCIE_ATU_REGION_INDEX1, + /* Program viewport 0 : OUTBOUND : CFG1 */ >> > + dw_pcie_writel_rc(pp, PCIE_ATU_REGION_OUTBOUND | >> > PCIE_ATU_REGION_INDEX0, >> > >> > PCIE_ATU_VIEWPORT); >> > >> > dw_pcie_writel_rc(pp, PCIE_ATU_TYPE_CFG1, PCIE_ATU_CR1); >> > dw_pcie_writel_rc(pp, PCIE_ATU_ENABLE, PCIE_ATU_CR2); >> > >> > @@ -505,38 +504,8 @@ static void dw_pcie_prog_viewport_cfg1(struct >> > pcie_port *pp, u32 busdev) >> > >> > PCIE_ATU_LIMIT); >> > >> > dw_pcie_writel_rc(pp, busdev, PCIE_ATU_LOWER_TARGET); >> > dw_pcie_writel_rc(pp, 0, PCIE_ATU_UPPER_TARGET); >> > >> > -} >> > - >> > -static void dw_pcie_prog_viewport_mem_outbound(struct pcie_port *pp) >> > -{ >> > - /* Program viewport 0 : OUTBOUND : MEM */ >> > - dw_pcie_writel_rc(pp, PCIE_ATU_REGION_OUTBOUND | >> > PCIE_ATU_REGION_INDEX0, - PCIE_ATU_VIEWPORT); >> > - dw_pcie_writel_rc(pp, PCIE_ATU_TYPE_MEM, PCIE_ATU_CR1); >> > - dw_pcie_writel_rc(pp, PCIE_ATU_ENABLE, PCIE_ATU_CR2); >> > - dw_pcie_writel_rc(pp, pp->mem_base, PCIE_ATU_LOWER_BASE); >> > - dw_pcie_writel_rc(pp, (pp->mem_base >> 32), PCIE_ATU_UPPER_BASE); >> > - dw_pcie_writel_rc(pp, pp->mem_base + pp->config.mem_size - 1, >> > - PCIE_ATU_LIMIT); >> > - dw_pcie_writel_rc(pp, pp->config.mem_bus_addr, PCIE_ATU_LOWER_TARGET); >> > - dw_pcie_writel_rc(pp, upper_32_bits(pp->config.mem_bus_addr), >> > - PCIE_ATU_UPPER_TARGET); >> > -} >> > - >> > -static void dw_pcie_prog_viewport_io_outbound(struct pcie_port *pp) >> > -{ >> > - /* Program viewport 1 : OUTBOUND : IO */ >> > - dw_pcie_writel_rc(pp, PCIE_ATU_REGION_OUTBOUND | >> > PCIE_ATU_REGION_INDEX1, - PCIE_ATU_VIEWPORT); >> > - dw_pcie_writel_rc(pp, PCIE_ATU_TYPE_IO, PCIE_ATU_CR1); >> > + dw_pcie_writel_rc(pp, PCIE_ATU_TYPE_CFG1, PCIE_ATU_CR1); >> > >> > dw_pcie_writel_rc(pp, PCIE_ATU_ENABLE, PCIE_ATU_CR2); >> > >> > - dw_pcie_writel_rc(pp, pp->io_base, PCIE_ATU_LOWER_BASE); >> > - dw_pcie_writel_rc(pp, (pp->io_base >> 32), PCIE_ATU_UPPER_BASE); >> > - dw_pcie_writel_rc(pp, pp->io_base + pp->config.io_size - 1, >> > - PCIE_ATU_LIMIT); >> > - dw_pcie_writel_rc(pp, pp->config.io_bus_addr, PCIE_ATU_LOWER_TARGET); >> > - dw_pcie_writel_rc(pp, upper_32_bits(pp->config.io_bus_addr), >> > - PCIE_ATU_UPPER_TARGET); >> > >> > } >> > >> > static int dw_pcie_rd_other_conf(struct pcie_port *pp, struct pci_bus >> > *bus, >> > >> > @@ -552,11 +521,9 @@ static int dw_pcie_rd_other_conf(struct pcie_port >> > *pp, struct pci_bus *bus, >> > >> > if (bus->parent->number == pp->root_bus_nr) { >> > >> > dw_pcie_prog_viewport_cfg0(pp, busdev); >> > ret = cfg_read(pp->va_cfg0_base + address, where, size, val); >> > >> > - dw_pcie_prog_viewport_mem_outbound(pp); >> > >> > } else { >> > >> > dw_pcie_prog_viewport_cfg1(pp, busdev); >> > ret = cfg_read(pp->va_cfg1_base + address, where, size, val); >> > >> > - dw_pcie_prog_viewport_io_outbound(pp); >> > >> > } >> > >> > return ret; >> > >> > @@ -575,11 +542,9 @@ static int dw_pcie_wr_other_conf(struct pcie_port >> > *pp, struct pci_bus *bus, >> > >> > if (bus->parent->number == pp->root_bus_nr) { >> > >> > dw_pcie_prog_viewport_cfg0(pp, busdev); >> > ret = cfg_write(pp->va_cfg0_base + address, where, size, val); >> > >> > - dw_pcie_prog_viewport_mem_outbound(pp); >> > >> > } else { >> > >> > dw_pcie_prog_viewport_cfg1(pp, busdev); >> > ret = cfg_write(pp->va_cfg1_base + address, where, size, val); >> > >> > - dw_pcie_prog_viewport_io_outbound(pp); >> > >> > } >> > >> > return ret; --- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designwa index e33b68b..a064bc5 100644 --- a/drivers/pci/host/pcie-designware.c +++ b/drivers/pci/host/pcie-designware.c @@ -556,7 +556,7 @@ static int dw_pcie_rd_other_conf(struct pcie_port *pp, struc } else { dw_pcie_prog_viewport_cfg1(pp, busdev); ret = cfg_read(pp->va_cfg1_base + address, where, size, val); - dw_pcie_prog_viewport_io_outbound(pp); +// dw_pcie_prog_viewport_io_outbound(pp); } return ret; @@ -579,7 +579,7 @@ static int dw_pcie_wr_other_conf(struct pcie_port *pp, struc } else { dw_pcie_prog_viewport_cfg1(pp, busdev); ret = cfg_write(pp->va_cfg1_base + address, where, size, val); - dw_pcie_prog_viewport_io_outbound(pp); +// dw_pcie_prog_viewport_io_outbound(pp); } Can anyone explain whats going on here? I would think this would