From patchwork Mon Jun 27 23:23:01 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Brian Norris X-Patchwork-Id: 9201533 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 864316075F for ; Mon, 27 Jun 2016 23:23:33 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 7800A285BA for ; Mon, 27 Jun 2016 23:23:33 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 6C499285CF; Mon, 27 Jun 2016 23:23:33 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.1 required=2.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_MED,T_DKIM_INVALID autolearn=ham version=3.3.1 Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.9]) (using TLSv1.2 with cipher AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id E5D99285BA for ; Mon, 27 Jun 2016 23:23:32 +0000 (UTC) Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.85_2 #1 (Red Hat Linux)) id 1bHfsH-00017T-JJ; Mon, 27 Jun 2016 23:23:29 +0000 Received: from mail-pa0-x231.google.com ([2607:f8b0:400e:c03::231]) by bombadil.infradead.org with esmtps (Exim 4.85_2 #1 (Red Hat Linux)) id 1bHfsE-00016A-3j for linux-rockchip@lists.infradead.org; Mon, 27 Jun 2016 23:23:28 +0000 Received: by mail-pa0-x231.google.com with SMTP id wo6so63298778pac.3 for ; Mon, 27 Jun 2016 16:23:05 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to :user-agent; bh=jmWH1Ei9y3brD8h/TyFyHlZmygMs10szQqBLUNB8GH8=; b=ekob/aEGcXZ5XpvVYjOV2xlnZXpj1oCywqvkA+wDrsbM0B/KCI5NAGkNoA+1TGWsKM mUGG9ZFmw5GHb72wT1k+IBmGrmhi10ggwOM3u/DxuKBOE8JkBcGo4BDtTTGitL+VMuG9 KcvcJdbCjnr9XwE5wIlTa7Shf9OV9Pg+bsCsM= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to:user-agent; bh=jmWH1Ei9y3brD8h/TyFyHlZmygMs10szQqBLUNB8GH8=; b=V7xPCdcT18ac4pUSVD0aO0L5uP+Gm4/N7w03lrj2cJr3/c7Qe3z5v9JMiajBVfHTMT K1Jjyy6l0ekYcYkKpaljVkXIhum8PembvXACUDppMxz8jvQiq+WKsA8iQfIfqiwZQK7Q yBKy9vC4yUhFUaOv+gRlikVaXVqrNBmWMKnrt06U7bHy24xIiN9nydsX7Fce7HPJhZLQ BnZ3BDKHl9KKGg1SRiGPrcsajYQyFRoq4Wl1ZO0jaOBghOcLZdxQyLe1PmF1bQm1tXeR mbfnl9/o5MVhmMYbrz6Uc3JKbGF+Mu0ME0r8QTiC4X+CLTHi9pC5EYbwVj7HW7LNwg5Q nGpw== X-Gm-Message-State: ALyK8tKiiv+z05PuKpQyO9s7p7dVmo2qqRz3FsxH3YiQ4n8m726WGKl7YedLOMfVIcKfZtAo X-Received: by 10.66.25.38 with SMTP id z6mr38798030paf.11.1467069784695; Mon, 27 Jun 2016 16:23:04 -0700 (PDT) Received: from google.com ([2620:0:1000:1301:a19f:9863:84a5:b24a]) by smtp.gmail.com with ESMTPSA id uj5sm2127087pac.28.2016.06.27.16.23.03 (version=TLS1_2 cipher=AES128-SHA bits=128/128); Mon, 27 Jun 2016 16:23:03 -0700 (PDT) Date: Mon, 27 Jun 2016 16:23:01 -0700 From: Brian Norris To: Shawn Lin Subject: Re: [PATCH v3 2/2] phy: add a driver for the Rockchip SoC internal PCIe PHY Message-ID: <20160627232301.GA101555@google.com> References: <1466040179-30973-1-git-send-email-shawn.lin@rock-chips.com> <5763F665.6070403@ti.com> <57678ED2.9000707@ti.com> <428a1393-c6b4-163c-ceec-9b79fdd8ad4a@rock-chips.com> <20160623233737.GB21157@google.com> <180dc842-3daf-a8b2-b333-fb51e4bc4f2a@rock-chips.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <180dc842-3daf-a8b2-b333-fb51e4bc4f2a@rock-chips.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20160627_162326_188947_9EE14254 X-CRM114-Status: GOOD ( 28.99 ) X-BeenThere: linux-rockchip@lists.infradead.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: Upstream kernel work for Rockchip platforms List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: devicetree@vger.kernel.org, Heiko Stuebner , Wenrui Li , Doug Anderson , linux-kernel@vger.kernel.org, linux-rockchip@lists.infradead.org, Rob Herring , Kishon Vijay Abraham I Sender: "Linux-rockchip" Errors-To: linux-rockchip-bounces+patchwork-linux-rockchip=patchwork.kernel.org@lists.infradead.org X-Virus-Scanned: ClamAV using ClamSMTP Hi Shawn, On Fri, Jun 24, 2016 at 09:37:41AM +0800, Shawn Lin wrote: > 在 2016/6/24 7:37, Brian Norris 写道: > >On Thu, Jun 23, 2016 at 10:30:17AM +0800, Shawn Lin wrote: > >>在 2016/6/20 14:36, Kishon Vijay Abraham I 写道: > >>>On Monday 20 June 2016 06:28 AM, Shawn Lin wrote: > >>>>On 2016/6/17 21:08, Kishon Vijay Abraham I wrote: > >>>>>On Thursday 16 June 2016 06:52 AM, Shawn Lin wrote: > >>>>>>+void rockchip_pcie_phy_laneoff(struct phy *phy) > >>>>>>+{ > >>>>>>+ u32 status; > >>>>>>+ struct rockchip_pcie_phy *rk_phy = phy_get_drvdata(phy); > >>>>>>+ int i; > >>>>>>+ > >>>>>>+ for (i = 0; i < PHY_MAX_LANE_NUM; i++) { > >>>>>>+ status = phy_rd_cfg(rk_phy, PHY_LANE_A_STATUS + i); > >>>>>>+ if (!((status >> PHY_LANE_RX_DET_SHIFT) & > >>>>>>+ PHY_LANE_RX_DET_TH)) > >>>>>>+ pr_debug("lane %d is used\n", i); > >>>>>>+ else > >>>>>>+ regmap_write(rk_phy->reg_base, > >>>>>>+ rk_phy->phy_data->pcie_laneoff, > >>>>>>+ HIWORD_UPDATE(PHY_LANE_IDLE_OFF, > >>>>>>+ PHY_LANE_IDLE_MASK, > >>>>>>+ PHY_LANE_IDLE_A_SHIFT + i)); > >>>>>>+ } > >>>>>>+} > >>>>>>+EXPORT_SYMBOL_GPL(rockchip_pcie_phy_laneoff); > > > >Shawn, I can't find an example of how you planned to use this (though I > >can make educated guesses). As such, it's possible there's some > >misunderstanding. Maybe you can include a sample patch for the PCIe > >controller driver? > > It will be called after rockchip_pcie_init_port for phy to disable > the unused lanes. A real, working patch would be nice. FWIW, when I try the following diff, my PCIe WiFi dies gloriously: > >Related: it might make sense to have the PCIe controller and PHY > >drivers/bindings all in the same patch series (with proper threading, > >which we already talked about off-list). > > > >>>>>Er.. don't use export symbols from phy driver. I think it would be nice if you > >>>>>can model the driver in such a way that the PCIe driver can control individual > >>>>>phy's. > >>>>> > >>>> > >>>>Yes, I was trying to look for a way not to export symbols from > >>>>phy... But I failed to find it as there at least need three > >>>>interaction between controller and phy which made me believe we > >>>>at least need to export one symbol without adding new API for phy. > > > >My interpretation of the above is that Shawn means we might turn off up > >to 3 different lanes (i.e., 3 of 4 supported lanes might be unused). > > yes, pcie drivers support up to 4 lanes. But the device may only > support x2. This is beyound the awareness of PCIe controller, so pcie > controller can't tell which one(s) should be turned off. Are you sure the PCIe controller can't know? I'm pretty sure it has know how many lanes are in use. And your controller even has this coded in it: status = pcie_read(port, PCIE_CORE_CTRL_MGMT_BASE); status = 0x1 << ((status >> PCIE_CORE_PL_CONF_LANE_SHIFT) & PCIE_CORE_PL_CONF_LANE_MASK); dev_dbg(port->dev, "current link width is x%d\n", status); So, doesn't that mean that out of lanes {0, 1, 2, 3}, that every lane other than 0 <= lane < width is unused? So you can just disable those, and have the same effect as the rockchip_pcie_phy_laneoff() function? > >>>That can be managed by implementing a small state machine within the PHY driver. > >> > >>I don't understand your point of implementing a small state machine > >>within the PHY driver. > > > >I'm not 100% sure I understand, but I think I have a reasonable > >interpretation below. > > > >>Do you mean I need to call vaarious of power_on/off and count the > >>on/off times to decide the state machine? > >> > >>I would appreciate it If you could elaborate this a bit more or > >>show me a example. :) > > > >My interpretation: rather than associating a single PCIe controller > >device with a single struct phy that controls up to 4 lanes, Kishon is > >suggesting you should have this driver implement 4 phy objects, one for > >each lane. You'd need to add #phy-cells = <1> to the DT binding, and > >implement an ->of_xlate() hook so we can associate/address them > >properly. Then the PCIe controller would call phy_power_off() on each > >lane that's not used. > > As I say above, even we have 4 phy objects, PCIe controller still > doesn't know which one(s) to be turned off, so you have to call 4 times > phy_power_off if I understand it correctly. This doesn't look > okay to me. Brian diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c index 661d6e04af71..fbb3dd8da0c7 100644 --- a/drivers/pci/host/pcie-rockchip.c +++ b/drivers/pci/host/pcie-rockchip.c @@ -373,6 +373,8 @@ static struct pci_ops rockchip_pcie_ops = { .write = rockchip_pcie_wr_conf, }; +extern void rockchip_pcie_phy_laneoff(struct phy *phy); + /** * rockchip_pcie_init_port - Initialize hardware * @port: PCIe port information @@ -533,6 +535,8 @@ static int rockchip_pcie_init_port(struct rockchip_pcie_port *port) pcie_write(port, 0x0, PCIE_CORE_AXI_CONF_BASE + PCIE_CORE_OB_REGION_DESC1); + rockchip_pcie_phy_laneoff(port->phy); + return 0; }