Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] The common power-on flow unconditionally writes to the legacy TX_BAND offset, which corrupts registers on the Nord platform. - [High] The Nord LDO configuration ignores DP mode and applies incorrect eDP settings, contradicting the Hardware Programming Guide. --
commit ff420262cd547c67f8bafbb24b3a2db77628fd60 Author: Ritesh Kumar <[email protected]> phy: qcom: edp: Introduce Nord phy_ver_ops and phy_cfg descriptor Add qcom_edp_phy_ops_nord wiring all eight Nord-specific callbacks and nord_edp_phy_cfg collecting the Nord AUX, VCO divider, swing/pre-emphasis tables and ver_ops. > diff --git a/drivers/phy/qualcomm/phy-qcom-edp.c > b/drivers/phy/qualcomm/phy-qcom-edp.c > index 361f4e4673518..338f28cb404fb 100644 > --- a/drivers/phy/qualcomm/phy-qcom-edp.c > +++ b/drivers/phy/qualcomm/phy-qcom-edp.c > @@ -1847,6 +1847,29 @@ static int qcom_edp_phy_tx_res_cfg_nord(const struct > qcom_edp *edp) > } > > > +static const struct phy_ver_ops qcom_edp_phy_ops_nord = { > + .com_power_on = qcom_edp_phy_power_on_nord, > + .com_resetsm_cntrl = qcom_edp_phy_com_resetsm_cntrl_nord, > + .com_bias_en_clkbuflr = qcom_edp_com_bias_en_clkbuflr_nord, > + .com_configure_pll = qcom_edp_com_configure_pll_nord, > + .com_configure_ssc = qcom_edp_com_configure_ssc_nord, > + .com_ldo_config = qcom_edp_ldo_config_nord, [Severity: High] Does this hardcode the incorrect LDO configuration for DP mode? The comments for qcom_edp_ldo_config_nord() document that DP mode must use 0x00 and eDP mode must use 0xD1. However, the implementation seems to unconditionally hardcode 0xd0 and 0x03, ignoring edp->is_edp completely: qcom_edp_ldo_config_nord() ldo_config = 0xd0; phy_ldo_cfg = 0x03; writel(ldo_config, edp->tx0 + TXn_LDO_CONFIG); Could this cause instability or link failures when running in DP mode due to contradicting the hardware requirements? > + .phy_tx_lane_cfg = qcom_edp_phy_tx_lane_cfg_nord, [Severity: High] Can the common power-on flow corrupt registers on the Nord platform here? When qcom_edp_phy_power_on() runs, it calls this phy_tx_lane_cfg() callback which correctly programs the Nord-specific TX_BAND offset at 0x0024: qcom_edp_phy_power_on() if (edp->cfg->ver_ops->phy_tx_lane_cfg) { ret = edp->cfg->ver_ops->phy_tx_lane_cfg(edp); ... But immediately afterward, the common flow unconditionally writes to the legacy TX_BAND offset: qcom_edp_phy_power_on() /* TX-0 register configuration */ ... writel(0x04, edp->tx0 + TXn_TX_BAND); Since TXn_TX_BAND moved to 0x0024 on Nord, writing to the legacy offset (0x0028) might clobber whatever unknown register resides there now. Would it be better to conditionally bypass the legacy TX_BAND write when the Nord specific callback is used? > + .phy_tx_res_cfg = qcom_edp_phy_tx_res_cfg_nord, > +}; > + > +static const struct qcom_edp_phy_cfg nord_edp_phy_cfg = { > + .is_edp = false, > + .aux_cfg = edp_phy_aux_cfg_nord, > + .vco_div_cfg = edp_phy_vco_div_cfg_nord, > + .dp_swing_pre_emph_cfg = &nord_dp_swing_pre_emph_cfg, > + .edp_swing_pre_emph_cfg = &nord_edp_swing_pre_emph_cfg, > + .ver_ops = &qcom_edp_phy_ops_nord, > + .phy_status_reg = DP_PHY_STATUS_NORD, > + .bias1_en_2lane = 0x03, > +}; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=16
