| Message ID | 1521785487-29866-2-git-send-email-mgautam@codeaurora.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Manu On Thu, Mar 22, 2018 at 11:11 PM, Manu Gautam <mgautam@codeaurora.org> 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@codeaurora.org> > --- > 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. -Doug -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@codeaurora.org> 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@codeaurora.org> >> --- >> 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. I can implement that as separate patch once dependent gcc driver patch(es) gets in. Would that be ok? -Manu
Hi Manu, On 3/23/2018 11:41 AM, Manu Gautam wrote: > QMP PHY for USB mode requires pipe_clk for calibration and PLL lock > to take place. AFAIK, that's not true. The pipe clock is the *output* of the PLL, and it's should not be needed for PLL calibration and locking. The PLL locking happens when the phy is configured and powered on. Atleast that's what the PIPE spec also says. CLK | | Y ---------------- | PLL |--------> PCLK (this is pipe clock 125/250/... MHz corresponding to the data width) ---------------- That's the reason we were enabling it after the PLL was locked. > 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@codeaurora.org> > --- > drivers/phy/qualcomm/phy-qcom-qmp.c | 33 ++++++++++++++++++++++++++++++++- > 1 file changed, 32 insertions(+), 1 deletion(-) > > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c > index 6470c5d..5d8df6a 100644 > --- a/drivers/phy/qualcomm/phy-qcom-qmp.c > +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c > @@ -1008,6 +1008,19 @@ static int qcom_qmp_phy_init(struct phy *phy) > status = pcs + cfg->regs[QPHY_PCS_READY_STATUS]; > mask = cfg->mask_pcs_ready; > > + /* > + * USB3 PHY requires pipe_clk for PLL lock and calibration. > + * Enable it from here for USB. For UFS/PCIE, it gets enabled > + * from poweron. > + */ > + if (cfg->type == PHY_TYPE_USB3) { > + ret = clk_prepare_enable(qphy->pipe_clk); As mentioned before AFAIU, pipe clock is just an output coming out of the PHY's PLL and we shouldn't try to enable pipe clock before PHY even gets initialized. We should may just try to first fix the unbalanced pipe clock enable/disable problem. Moreover, lets take care of all USB, PCIe and DP phys when we want to enable/disbale pipe clock as all of them use this clock. Best regards Vivek > + if (ret) { > + dev_err(qmp->dev, "pipe_clk enable err=%d\n", ret); > + goto err_clk_enable; > + } > + } > + > ret = readl_poll_timeout(status, val, !(val & mask), 1, > PHY_INIT_COMPLETE_TIMEOUT); > if (ret) { > @@ -1019,6 +1032,9 @@ static int qcom_qmp_phy_init(struct phy *phy) > return ret; > > err_pcs_ready: > + if (cfg->type == PHY_TYPE_USB3) > + clk_disable_unprepare(qphy->pipe_clk); > +err_clk_enable: > if (cfg->has_lane_rst) > reset_control_assert(qphy->lane_rst); > err_lane_rst: > @@ -1288,10 +1304,19 @@ static int phy_pipe_clk_register(struct qcom_qmp *qmp, struct device_node *np) > .owner = THIS_MODULE, > }; > > +/* USB PHY doesn't require power_on op */ > +static const struct phy_ops qcom_qmp_usb_phy_gen_ops = { > + .init = qcom_qmp_phy_init, > + .exit = qcom_qmp_phy_exit, > + .set_mode = qcom_qmp_phy_set_mode, > + .owner = THIS_MODULE, > +}; > + > static > int qcom_qmp_phy_create(struct device *dev, struct device_node *np, int id) > { > struct qcom_qmp *qmp = dev_get_drvdata(dev); > + const struct phy_ops *ops; > struct phy *generic_phy; > struct qmp_phy *qphy; > char prop_name[MAX_PROP_NAME]; > @@ -1354,7 +1379,13 @@ int qcom_qmp_phy_create(struct device *dev, struct device_node *np, int id) > } > } > > - generic_phy = devm_phy_create(dev, np, &qcom_qmp_phy_gen_ops); > + /* USB PHY doesn't use power_on op */ > + if (qmp->cfg->type == PHY_TYPE_USB3) > + ops = &qcom_qmp_usb_phy_gen_ops; > + else > + ops = &qcom_qmp_phy_gen_ops; > + > + generic_phy = devm_phy_create(dev, np, ops); > if (IS_ERR(generic_phy)) { > ret = PTR_ERR(generic_phy); > dev_err(dev, "failed to create qphy %d\n", ret); -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@codeaurora.org> 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@codeaurora.org> >>> --- >>> 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. 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. Best regards Vivek > > I can implement that as separate patch once dependent gcc driver > patch(es) gets in. Would that be ok? > > -Manu > -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Vivek, On 3/27/2018 12:21 PM, Vivek Gautam wrote: > Hi Manu, > > > On 3/23/2018 11:41 AM, Manu Gautam wrote: >> QMP PHY for USB mode requires pipe_clk for calibration and PLL lock >> to take place. > > AFAIK, that's not true. The pipe clock is the *output* of the PLL, and it's should not > be needed for PLL calibration and locking. The PLL locking happens when the phy is > configured and powered on. > Atleast that's what the PIPE spec also says. > > CLK > | > | > Y > ---------------- > | PLL |--------> PCLK (this is pipe clock 125/250/... MHz corresponding to the data width) > ---------------- > > That's the reason we were enabling it after the PLL was locked. I got this clarified by QMP PHY hardware designer that pipe_clk is indeed needed for PHY to complete init sequence and reflect in PHY_STATUS (mainly for calibration and I should remove PLL lock from commit subject). It might work on some platforms without this change if boot code leaves pipe clock enabled during bootup. > >> 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@codeaurora.org> >> --- >> drivers/phy/qualcomm/phy-qcom-qmp.c | 33 ++++++++++++++++++++++++++++++++- >> 1 file changed, 32 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c >> index 6470c5d..5d8df6a 100644 >> --- a/drivers/phy/qualcomm/phy-qcom-qmp.c >> +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c >> @@ -1008,6 +1008,19 @@ static int qcom_qmp_phy_init(struct phy *phy) >> status = pcs + cfg->regs[QPHY_PCS_READY_STATUS]; >> mask = cfg->mask_pcs_ready; >> + /* >> + * USB3 PHY requires pipe_clk for PLL lock and calibration. >> + * Enable it from here for USB. For UFS/PCIE, it gets enabled >> + * from poweron. >> + */ >> + if (cfg->type == PHY_TYPE_USB3) { >> + ret = clk_prepare_enable(qphy->pipe_clk); > > As mentioned before AFAIU, pipe clock is just an output coming out of the PHY's PLL > and we shouldn't try to enable pipe clock before PHY even gets initialized. > We should may just try to first fix the unbalanced pipe clock enable/disable problem. > > Moreover, lets take care of all USB, PCIe and DP phys when we want to enable/disbale > pipe clock as all of them use this clock. I will get the pipe_clk requirement for PCIE as well and do it for both (depending on gcc change). > > Best regards > Vivek > >> + if (ret) { >> + dev_err(qmp->dev, "pipe_clk enable err=%d\n", ret); >> + goto err_clk_enable; >> + } >> + } >> + >> ret = readl_poll_timeout(status, val, !(val & mask), 1, >> PHY_INIT_COMPLETE_TIMEOUT); >> if (ret) { >> @@ -1019,6 +1032,9 @@ static int qcom_qmp_phy_init(struct phy *phy) >> return ret; >> err_pcs_ready: >> + if (cfg->type == PHY_TYPE_USB3) >> + clk_disable_unprepare(qphy->pipe_clk); >> +err_clk_enable: >> if (cfg->has_lane_rst) >> reset_control_assert(qphy->lane_rst); >> err_lane_rst: >> @@ -1288,10 +1304,19 @@ static int phy_pipe_clk_register(struct qcom_qmp *qmp, struct device_node *np) >> .owner = THIS_MODULE, >> }; >> +/* USB PHY doesn't require power_on op */ >> +static const struct phy_ops qcom_qmp_usb_phy_gen_ops = { >> + .init = qcom_qmp_phy_init, >> + .exit = qcom_qmp_phy_exit, >> + .set_mode = qcom_qmp_phy_set_mode, >> + .owner = THIS_MODULE, >> +}; >> + >> static >> int qcom_qmp_phy_create(struct device *dev, struct device_node *np, int id) >> { >> struct qcom_qmp *qmp = dev_get_drvdata(dev); >> + const struct phy_ops *ops; >> struct phy *generic_phy; >> struct qmp_phy *qphy; >> char prop_name[MAX_PROP_NAME]; >> @@ -1354,7 +1379,13 @@ int qcom_qmp_phy_create(struct device *dev, struct device_node *np, int id) >> } >> } >> - generic_phy = devm_phy_create(dev, np, &qcom_qmp_phy_gen_ops); >> + /* USB PHY doesn't use power_on op */ >> + if (qmp->cfg->type == PHY_TYPE_USB3) >> + ops = &qcom_qmp_usb_phy_gen_ops; >> + else >> + ops = &qcom_qmp_phy_gen_ops; >> + >> + generic_phy = devm_phy_create(dev, np, ops); >> if (IS_ERR(generic_phy)) { >> ret = PTR_ERR(generic_phy); >> dev_err(dev, "failed to create qphy %d\n", ret); >
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@codeaurora.org> 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@codeaurora.org> >>>> --- >>>> 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. 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. > > Best regards > Vivek > >> >> I can implement that as separate patch once dependent gcc driver >> patch(es) gets in. Would that be ok? >> >> -Manu >> >
Hi, On Tue, Mar 27, 2018 at 12:50 AM, Manu Gautam <mgautam@codeaurora.org> 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@codeaurora.org> 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@codeaurora.org> >>>>> --- >>>>> 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 linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 3/28/2018 1:44 AM, Doug Anderson wrote: > Hi, > > On Tue, Mar 27, 2018 at 12:50 AM, Manu Gautam <mgautam@codeaurora.org> 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@codeaurora.org> 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@codeaurora.org> >>>>>> --- >>>>>> 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
diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c index 6470c5d..5d8df6a 100644 --- a/drivers/phy/qualcomm/phy-qcom-qmp.c +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c @@ -1008,6 +1008,19 @@ static int qcom_qmp_phy_init(struct phy *phy) status = pcs + cfg->regs[QPHY_PCS_READY_STATUS]; mask = cfg->mask_pcs_ready; + /* + * USB3 PHY requires pipe_clk for PLL lock and calibration. + * Enable it from here for USB. For UFS/PCIE, it gets enabled + * from poweron. + */ + if (cfg->type == PHY_TYPE_USB3) { + ret = clk_prepare_enable(qphy->pipe_clk); + if (ret) { + dev_err(qmp->dev, "pipe_clk enable err=%d\n", ret); + goto err_clk_enable; + } + } + ret = readl_poll_timeout(status, val, !(val & mask), 1, PHY_INIT_COMPLETE_TIMEOUT); if (ret) { @@ -1019,6 +1032,9 @@ static int qcom_qmp_phy_init(struct phy *phy) return ret; err_pcs_ready: + if (cfg->type == PHY_TYPE_USB3) + clk_disable_unprepare(qphy->pipe_clk); +err_clk_enable: if (cfg->has_lane_rst) reset_control_assert(qphy->lane_rst); err_lane_rst: @@ -1288,10 +1304,19 @@ static int phy_pipe_clk_register(struct qcom_qmp *qmp, struct device_node *np) .owner = THIS_MODULE, }; +/* USB PHY doesn't require power_on op */ +static const struct phy_ops qcom_qmp_usb_phy_gen_ops = { + .init = qcom_qmp_phy_init, + .exit = qcom_qmp_phy_exit, + .set_mode = qcom_qmp_phy_set_mode, + .owner = THIS_MODULE, +}; + static int qcom_qmp_phy_create(struct device *dev, struct device_node *np, int id) { struct qcom_qmp *qmp = dev_get_drvdata(dev); + const struct phy_ops *ops; struct phy *generic_phy; struct qmp_phy *qphy; char prop_name[MAX_PROP_NAME]; @@ -1354,7 +1379,13 @@ int qcom_qmp_phy_create(struct device *dev, struct device_node *np, int id) } } - generic_phy = devm_phy_create(dev, np, &qcom_qmp_phy_gen_ops); + /* USB PHY doesn't use power_on op */ + if (qmp->cfg->type == PHY_TYPE_USB3) + ops = &qcom_qmp_usb_phy_gen_ops; + else + ops = &qcom_qmp_phy_gen_ops; + + generic_phy = devm_phy_create(dev, np, ops); if (IS_ERR(generic_phy)) { ret = PTR_ERR(generic_phy); dev_err(dev, "failed to create qphy %d\n", ret);
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@codeaurora.org> --- drivers/phy/qualcomm/phy-qcom-qmp.c | 33 ++++++++++++++++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-)