Re: [PATCH v3 2/2] phy: add a driver for the Rockchip SoC internal PCIe PHY

[Shawn Lin <shawn.lin@rock-chips.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