linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Shawn Lin <shawn.lin@rock-chips.com>
To: Brian Norris <briannorris@chromium.org>,
	Kishon Vijay Abraham I <kishon@ti.com>
Cc: shawn.lin@rock-chips.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: Fri, 24 Jun 2016 09:37:41 +0800	[thread overview]
Message-ID: <180dc842-3daf-a8b2-b333-fb51e4bc4f2a@rock-chips.com> (raw)
In-Reply-To: <20160623233737.GB21157@google.com>

在 2016/6/24 7:37, Brian Norris 写道:
> 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?

It will be called after rockchip_pcie_init_port for phy to disable
the unused lanes.

>
> 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.

>
>>> 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.

>
> 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
>
>
>


-- 
Best Regards
Shawn Lin

  reply	other threads:[~2016-06-24  1: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
2016-06-24  1:37           ` Shawn Lin [this message]
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=180dc842-3daf-a8b2-b333-fb51e4bc4f2a@rock-chips.com \
    --to=shawn.lin@rock-chips.com \
    --cc=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=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).