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

[Kishon Vijay Abraham I]

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