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