Re: [PATCH 3/8] phy: qcom: qmp-usbc: Add DP phy mode support on QCS615

[Xiangxu Yin <quic_xiangxuy@quicinc.com>]

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_xiangxuy@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