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

[Manu Gautam <mgautam@codeaurora.org>]

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);
>

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