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

[Brian Norris]

Hi Kishon,

On Mon, Jun 27, 2016 at 10:54:19AM +0530, Kishon Vijay Abraham I wrote:
> On Friday 24 June 2016 05:07 AM, Brian Norris wrote:
> > 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:
> >>>>> 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).

[...]

> That's pretty much what I had in mind. Thanks for putting this down in an
> elaborate manner.

No problem. Glad we understand your suggestion now.

It remains to be seen whether we can reasonably utilize this suggestion
though. According to Shawn, the PCIe controller doesn't have anough
information to be able to utilize this model effectively. But I have my
doubts, which I've posted in reply to Shawn.

Regards,
Brian