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

[Manu Gautam]

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@xxxxxxxxxxxxxx>
>> ---
>> Â 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);
>

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