Hi,


On 3/28/2018 1:44 AM, Doug Anderson wrote:

Hi,

On Tue, Mar 27, 2018 at 12:50 AM, Manu Gautam [off-list ref] wrote:

quoted

Hi,


On 3/27/2018 12:26 PM, Vivek Gautam wrote:

quoted

On 3/27/2018 10:37 AM, Manu Gautam wrote:

quoted

Hi Doug,


On 3/27/2018 9:56 AM, Doug Anderson wrote:

quoted

Manu

On Thu, Mar 22, 2018 at 11:11 PM, Manu Gautam [off-list ref] wrote:

quoted

QMP PHY for USB mode requires pipe_clk for calibration and PLL lock
to take place. This clock is output from PHY to GCC clock_ctl and then
fed back to QMP PHY and is available from PHY only after PHY is reset
and initialized, hence it can't be enabled too early in initialization
sequence.

Signed-off-by: Manu Gautam <redacted>
---
  drivers/phy/qualcomm/phy-qcom-qmp.c | 33 ++++++++++++++++++++++++++++++++-
  1 file changed, 32 insertions(+), 1 deletion(-)

So it's now new with this patch, but it's more obvious with this
patch.  It seems like "UFS/PCIE" is kinda broken w/ respect to how it
controls its clock.  Specifically:

* If you init the PHY but don't power it on, then you "exit" the PHY:
you'll disable/unprepare "pipe_clk" even though you never
prepare/enabled it.

* If you init the PHY, power it on, power it off, power it on, and
exit the PHY: you'll leave the clock prepared one extra time.

Specifically I'd expect: for UFS/PCIE the disable/unprepare should be
symmetric with the enable/prepare and should be in "power off", not in
exit.

...or did I miss something?


Interestingly, your patch fixes this problem for USB3 (where init/exit
are now symmetric), but leaves the problem there for UFS/PCIE.

Thanks for review.
One of the reason why pipe_clk is disabled as part of phy_exit is that
halt_check from clk_disable reports error if called after PHY has been
powered down or phy_exit.
I believe that warning should be ignored in qcom gcc-clock driver
(for applicable platforms) by using BRANCH_HALT_DELAY as halt_check
for pipe_clk and performing clk_disable from power_off for UFS/PCIE.

UFS doesn't use PIPE clock.

Just to confirm: we no longer need to do this "BRANCH_HALT_DELAY" now
that we've figured everything out, right?

That is still needed as PHY might take some time to generate pipe_clk
after its PLL is locked (or while initialization sequence is carried out).
Performing clk_enable will throw a warning. Hence, it is better to
have halt_check that will allow to club pipe_clk with other clocks and
enable it at the beginning of phy_init.

quoted

Yes, UFS PHY doesn't use one. But similar to pipe_clk there are rx/tx symbol_clk
output from PHY that is used by UFS controller. I will update code comments
to not refer UFS for pipe_clk.

quoted

But considering for PCIe, if we disable pipe clock when phy is still running, then
it shouldn't be a problem. We should also not see the halt warning as the gcc
driver should be able to just turn the gate off.
The reason why it will throw that error is when the parent clock to that gate
is gated, i.e. the pipe clock is not flowing on that branch.

I got the confirmation that pipe_clk is needed for PCIE as well for its
initialization to happen successfully. So we do need clock driver change
to fix this in PHY driver.

So basically if I'm understanding this correctly:

* Both USB and PCIE need the clk_enable() in qcom_qmp_phy_init()

* UFS doesn't even use a pipe clock (pipe_clk is NULL and thus these
calls are no-ops).

So that means the next version of this code will simply get rid of
qcom_qmp_phy_poweron() and we can now use the same phy_ops for both
everything again?  That also makes everything symmetric and gets rid
of the possible imbalance of clock enable/disable, so I'm happy.

Yes.

Actually: I'll also throw out a drastic idea here.  Maybe instead of
having a NULL power_on/power_off, we should have a NULL init/exit.
Does anything break if all the stuff that happens today in
qcom_qmp_phy_com_init() happens at power_on() time instead of init()
time?  I suggest this because:

* It sounds like init() is supposed to be for initialization that can
happen _before_ power on of the PHY.

* Any initialization that happens after the PHY has been powered on
seems expected to just be in the power_on() function after the
regulator was enabled.


Presumably moving this stuff to power_on could save you some power in
some cases (since the client of the PHY presumably turns power off to
the PHY with the idea of saving power).

This could be ok for DWC3 USB core driver which uses both phy_init and
power_on together on init/suspend.
But it looks like ufs-qcom and pcie-qcom (mainly ufs) handle power_on
and phy_init differently. They also reset core while running init/power_on.
Changing power_on/init could affect those cores.
Regarding power_management, I think we can just update ufc/pcie
qcom glue drivers to either use phy_init/exit for power_management.
Or PHY runtime_PM if client doesn't want to power_down or reset phy
during suspend/resume.

-Doug

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project