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 <shawn.lin@xxxxxxxxxxxxxx>
>>>>>> ---
>>>>>>
>>>>>> 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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
