linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Brian Norris <briannorris@chromium.org>
To: Shawn Lin <shawn.lin@rock-chips.com>,
	Kishon Vijay Abraham I <kishon@ti.com>
Cc: Kishon Vijay Abraham I <kishon@ti.com>,
	devicetree@vger.kernel.org, Heiko Stuebner <heiko@sntech.de>,
	Wenrui Li <wenrui.li@rock-chips.com>,
	Doug Anderson <dianders@chromium.org>,
	linux-kernel@vger.kernel.org, linux-rockchip@lists.infradead.org,
	Rob Herring <robh+dt@kernel.org>
Subject: Re: [PATCH v3 2/2] phy: add a driver for the Rockchip SoC internal PCIe PHY
Date: Thu, 23 Jun 2016 16:37:38 -0700	[thread overview]
Message-ID: <20160623233737.GB21157@google.com> (raw)
In-Reply-To: <428a1393-c6b4-163c-ceec-9b79fdd8ad4a@rock-chips.com>

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 <shawn.lin@rock-chips.com>
> >>>>---
> >>>>
> >>>>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.

Brian

  parent reply	other threads:[~2016-06-23 23:37 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-06-16  1:22 Shawn Lin
2016-06-17 13:08 ` Kishon Vijay Abraham I
2016-06-20  0:58   ` Shawn Lin
2016-06-20  6:36     ` Kishon Vijay Abraham I
     [not found]       ` <428a1393-c6b4-163c-ceec-9b79fdd8ad4a@rock-chips.com>
2016-06-23 23:37         ` Brian Norris [this message]
2016-06-24  1:37           ` Shawn Lin
2016-06-27 23:23             ` Brian Norris
2016-06-27  5:24           ` Kishon Vijay Abraham I
2016-06-27 23:24             ` Brian Norris

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20160623233737.GB21157@google.com \
    --to=briannorris@chromium.org \
    --cc=devicetree@vger.kernel.org \
    --cc=dianders@chromium.org \
    --cc=heiko@sntech.de \
    --cc=kishon@ti.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-rockchip@lists.infradead.org \
    --cc=robh+dt@kernel.org \
    --cc=shawn.lin@rock-chips.com \
    --cc=wenrui.li@rock-chips.com \
    --subject='Re: [PATCH v3 2/2] phy: add a driver for the Rockchip SoC internal PCIe PHY' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).