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 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
