From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751677AbcF0FZC (ORCPT ); Mon, 27 Jun 2016 01:25:02 -0400 Received: from devils.ext.ti.com ([198.47.26.153]:44944 "EHLO devils.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750782AbcF0FZA (ORCPT ); Mon, 27 Jun 2016 01:25:00 -0400 Subject: Re: [PATCH v3 2/2] phy: add a driver for the Rockchip SoC internal PCIe PHY To: Brian Norris , Shawn Lin 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> CC: , Heiko Stuebner , Wenrui Li , Doug Anderson , , , Rob Herring From: Kishon Vijay Abraham I Message-ID: <5770B883.9030907@ti.com> Date: Mon, 27 Jun 2016 10:54:19 +0530 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.7.2 MIME-Version: 1.0 In-Reply-To: <20160623233737.GB21157@google.com> Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, On Friday 24 June 2016 05:07 AM, Brian Norris wrote: > Hi, > > 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: >>>>>> This patch to add a generic PHY driver for rockchip PCIe PHY. >>>>>> Access the PHY via registers provided by GRF (general register >>>>>> files) module. >>>>>> >>>>>> Signed-off-by: Shawn Lin >>>>>> --- >>>>>> >>>>>> Changes in v3: None >>>>>> Changes in v2: None >>>>>> > [...] >>>>>> diff --git a/drivers/phy/phy-rockchip-pcie.c b/drivers/phy/phy-rockchip-pcie.c >>>>>> new file mode 100644 >>>>>> index 0000000..bc6cd17 >>>>>> --- /dev/null >>>>>> +++ b/drivers/phy/phy-rockchip-pcie.c >>>>>> @@ -0,0 +1,378 @@ > > [...] > >>>>>> +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? > > 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). > >>> 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. > > The state machine would come into play because you have additional power > savings to utilize, but only when all PHYs are off. So the state machine > would just track how many of the lane PHYs are still on, and when the > count reaches 0, you call reset_control_assert(rk_phy->phy_rst). > > The DT for this would be: > > pcie0: pcie@f8000000 { > compatible = "rockchip,rk3399-pcie"; > ... > phys = <&pcie_phy 0>, <&pcie_phy 1>, <&pcie_phy 2>, <&pcie_phy 3>; > phy-names = "pcie-lane0", "pcie-lane1", "pcie-lane2", "pcie-lane3"; > ... > }; > > pcie_phy: pcie-phy { > compatible = "rockchip,rk3399-pcie-phy"; > ... > #phy-cells = <1>; > ... > }; > > (See Documentation/devicetree/bindings/phy/phy-bindings.txt for the > #phy-cells explanation.) > > Is that close to what you're suggesting, Kishon? Seems reasonable enough > to me, even if it's slightly more complicated. That's pretty much what I had in mind. Thanks for putting this down in an elaborate manner. Thanks Kishon