Re: [PATCH v3 1/6] phy: qcom-qmp: Enable pipe_clk before checking USB3 PHY_STATUS

[Manu Gautam]

Hi,


On 3/28/2018 1:44 AM, Doug Anderson wrote:
> Hi,
>
> On Tue, Mar 27, 2018 at 12:50 AM, Manu Gautam <mgautam@xxxxxxxxxxxxxx> wrote:
>> Hi,
>>
>>
>> On 3/27/2018 12:26 PM, Vivek Gautam wrote:
>>>
>>> On 3/27/2018 10:37 AM, Manu Gautam wrote:
>>>> Hi Doug,
>>>>
>>>>
>>>> On 3/27/2018 9:56 AM, Doug Anderson wrote:
>>>>> Manu
>>>>>
>>>>> On Thu, Mar 22, 2018 at 11:11 PM, Manu Gautam <mgautam@xxxxxxxxxxxxxx> wrote:
>>>>>> 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 <mgautam@xxxxxxxxxxxxxx>
>>>>>> ---
>>>>>>   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.

>
>
>> 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.
>>
>>> 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