On 11/29/2024 4:18 PM, Krzysztof Kozlowski wrote:
> On 29/11/2024 08:57, Xiangxu Yin wrote:
>> Extended DP support for QCS615 USB or DP phy. Differentiated between
>> USBC and DP PHY using the match table’s type, dynamically generating
>> different types of cfg and layout attributes during initialization based
>> on this type. Static variables are stored in cfg, while parsed values
>> are organized into the layout structure.
>>
>> Signed-off-by: Xiangxu Yin <quic_xiang...@quicinc.com>
>> ---
>> drivers/phy/qualcomm/phy-qcom-qmp-dp-phy.h | 1 +
>> drivers/phy/qualcomm/phy-qcom-qmp-usbc.c | 1453
>> ++++++++++++++++++++++++----
>> 2 files changed, 1254 insertions(+), 200 deletions(-)
>
>
>
> ...
>
>> + /* program default setting first */
>> + writel(0x2A, tx + QSERDES_V3_TX_TX_DRV_LVL);
>> + writel(0x20, tx + QSERDES_V3_TX_TX_EMP_POST1_LVL);
>> + writel(0x2A, tx2 + QSERDES_V3_TX_TX_DRV_LVL);
>> + writel(0x20, tx2 + QSERDES_V3_TX_TX_EMP_POST1_LVL);
>> +
>> + writel(voltage_swing_cfg, tx + QSERDES_V3_TX_TX_DRV_LVL);
>> + writel(pre_emphasis_cfg, tx + QSERDES_V3_TX_TX_EMP_POST1_LVL);
>> + writel(voltage_swing_cfg, tx2 + QSERDES_V3_TX_TX_DRV_LVL);
>> + writel(pre_emphasis_cfg, tx2 + QSERDES_V3_TX_TX_EMP_POST1_LVL);
>> +
>> + return 0;
>> +}
>> +
>> +static int qcs615_qmp_configure_dp_phy(struct qmp_usbc *qmp)
>> +{
>> + struct qmp_phy_dp_layout *layout = to_dp_layout(qmp);
>> + u32 status;
>> +
>> + writel(0x01, layout->dp_phy + QSERDES_DP_PHY_CFG);
>> + writel(0x05, layout->dp_phy + QSERDES_DP_PHY_CFG);
>> + writel(0x01, layout->dp_phy + QSERDES_DP_PHY_CFG);
>> + writel(0x09, layout->dp_phy + QSERDES_DP_PHY_CFG);
>> +
>> + writel(0x20, layout->dp_serdes + QSERDES_COM_RESETSM_CNTRL);
>> +
>> + // C_READY
>
> Use Linux coding style.
>
> Anyway, drop all useless comments. Say something useful or don't say
> anything.
>
Ok, will update in next seperated patches.
>> + if (readl_poll_timeout(layout->dp_serdes + QSERDES_COM_C_READY_STATUS,
>> + status,
>> + ((status & BIT(0)) > 0),
>> + 500,
>> + 10000)) {
>> + dev_err(qmp->dev, "C_READY not ready\n");
>> + return -ETIMEDOUT;
>> + }
>> +
>> + // FREQ_DONE
>> + if (readl_poll_timeout(layout->dp_serdes + QSERDES_COM_CMN_STATUS,
>> + status,
>> + ((status & BIT(0)) > 0),
>> + 500,
>> + 10000)){
>> + dev_err(qmp->dev, "FREQ_DONE not ready\n");
>> + return -ETIMEDOUT;
>> + }
>> +
>> + // PLL_LOCKED
>> + if (readl_poll_timeout(layout->dp_serdes + QSERDES_COM_CMN_STATUS,
>> + status,
>> + ((status & BIT(1)) > 0),
>> + 500,
>> + 10000)){
>> + dev_err(qmp->dev, "PLL_LOCKED not ready\n");
>> + return -ETIMEDOUT;
>> + }
>> +
>> + writel(0x19, layout->dp_phy + QSERDES_DP_PHY_CFG);
>> + udelay(10);
>> +
>> + // TSYNC_DONE
>> + if (readl_poll_timeout(layout->dp_phy + QSERDES_V3_DP_PHY_STATUS,
>> + status,
>> + ((status & BIT(0)) > 0),
>> + 500,
>> + 10000)){
>> + dev_err(qmp->dev, "TSYNC_DONE not ready\n");
>> + return -ETIMEDOUT;
>> + }
>> +
>> + // PHY_READY
>> + if (readl_poll_timeout(layout->dp_phy + QSERDES_V3_DP_PHY_STATUS,
>> + status,
>> + ((status & BIT(1)) > 0),
>> + 500,
>> + 10000)){
>> + dev_err(qmp->dev, "PHY_READY not ready\n");
>> + return -ETIMEDOUT;
>> + }
>> +
>> + writel(0x3f, layout->dp_tx + QSERDES_V3_TX_TRANSCEIVER_BIAS_EN);
>> + writel(0x10, layout->dp_tx + QSERDES_V3_TX_HIGHZ_DRVR_EN);
>> + writel(0x0a, layout->dp_tx + QSERDES_V3_TX_TX_POL_INV);
>> + writel(0x3f, layout->dp_tx2 + QSERDES_V3_TX_TRANSCEIVER_BIAS_EN);
>> + writel(0x10, layout->dp_tx2 + QSERDES_V3_TX_HIGHZ_DRVR_EN);
>> + writel(0x0a, layout->dp_tx2 + QSERDES_V3_TX_TX_POL_INV);
>> +
>> + writel(0x18, layout->dp_phy + QSERDES_DP_PHY_CFG);
>> + writel(0x19, layout->dp_phy + QSERDES_DP_PHY_CFG);
>> +
>> + if (readl_poll_timeout(layout->dp_phy + QSERDES_V3_DP_PHY_STATUS,
>> + status,
>> + ((status & BIT(1)) > 0),
>> + 500,
>> + 10000)){
>> + dev_err(qmp->dev, "PHY_READY not ready\n");
>> + return -ETIMEDOUT;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int qcs615_qmp_calibrate_dp_phy(struct qmp_usbc *qmp)
>> +{
>> + static const u8 cfg1_settings[] = {0x13, 0x23, 0x1d};
>> + struct qmp_phy_dp_layout *layout = to_dp_layout(qmp);
>> + u8 val;
>> +
>> + layout->dp_aux_cfg++;
>> + layout->dp_aux_cfg %= ARRAY_SIZE(cfg1_settings);
>> + val = cfg1_settings[layout->dp_aux_cfg];
>> +
>> + writel(val, layout->dp_phy + QSERDES_DP_PHY_AUX_CFG1);
>> +
>> + qmp_usbc_check_dp_phy(qmp, "pos_calibrate");
>> +
>> + return 0;
>> +}
>> +
>> +static int qmp_usbc_com_init(struct phy *phy)
>> {
>> struct qmp_usbc *qmp = phy_get_drvdata(phy);
>> - const struct qmp_phy_cfg *cfg = qmp->cfg;
>> - void __iomem *pcs = qmp->pcs;
>> + int num_vregs;
>> u32 val = 0;
>> int ret;
>> + unsigned int reg_pwr_dn;
>>
>> - ret = regulator_bulk_enable(cfg->num_vregs, qmp->vregs);
>> + if (qmp->type == QMP_PHY_USBC_USB) {
>
>
> Sorry, all this code is unreviewable. Organize your changes in logical,
> reviewable chunks.
>
Will create new patch list and seperate patchsets.
>> + struct qmp_phy_usb_cfg *cfg = to_usb_cfg(qmp);
>> +
>> + num_vregs = cfg->num_vregs;
>> + reg_pwr_dn = cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL];
>> + } else {
>
> ...
>
>> + .compatible = "qcom,qcs615-qmp-dp-phy",
>> + .data = &(struct dev_cfg) {
>> + .type = QMP_PHY_USBC_DP,
>> + .cfg = &qcs615_dpphy_cfg,
>> + },
>> }, {
>> .compatible = "qcom,sdm660-qmp-usb3-phy",
>> - .data = &sdm660_usb3phy_cfg,
>> + .data = &(struct dev_cfg) {
>> + .type = QMP_PHY_USBC_USB,
>> + .cfg = &sdm660_usb3phy_cfg,
>> + },
>> }, {
>> .compatible = "qcom,sm6115-qmp-usb3-phy",
>> - .data = &qcm2290_usb3phy_cfg,
>> + .data = &(struct dev_cfg) {
>> + .type = QMP_PHY_USBC_USB,
>> + .cfg = &qcm2290_usb3phy_cfg,
>> + },
>> },
>> { },
>> };
>> +
>
>
> You make some random changes all over this file. No, clean it up.
>
>> MODULE_DEVICE_TABLE(of, qmp_usbc_of_match_table);
>>
>> static struct platform_driver qmp_usbc_driver = {
>>
>
>
> Best regards,
> Krzysztof
