Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing DisplayPort initialization callbacks in `hawi_usb3dpphy_cfg` will cause a NULL pointer dereference when the DP PHY is initialized.
- [High] The AON toggle registers will not be cleared because `cfg->regs[QPHY_AON_TOGGLE_ENABLE]` evaluates to false for the Hawi V10 PHY.
--
--- Patch [4]: [PATCH v4 4/4] phy: qualcomm: qmp-combo: Add support for Hawi SoC ---
commit 74a7f87645b4d6da0a341014f5299e1ad20ae0d5
Author: Ronak Raheja [off-list ref]
phy: qualcomm: qmp-combo: Add support for Hawi SoC
Add support for the USB3-DP combo PHY found on Hawi platform.
The QMP PHY for Hawi uses QSERDES V10 register layouts.
quoted hunk ↗ jump to hunk
diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
index cdcfad2e86b1d..ae80eee628c5e 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -2705,6 +2895,27 @@ static const struct qmp_phy_cfg x1e80100_usb3dpphy_cfg = {
.regs = qmp_v6_n4_usb3phy_regs_layout,
};
+static const struct qmp_phy_cfg hawi_usb3dpphy_cfg = {
+ .offsets = &qmp_combo_offsets_v10,
+
+ .serdes_tbl = hawi_usb3_serdes_tbl,
+ .serdes_tbl_num = ARRAY_SIZE(hawi_usb3_serdes_tbl),
+ .tx_tbl = hawi_usb3_tx_tbl,
+ .tx_tbl_num = ARRAY_SIZE(hawi_usb3_tx_tbl),
+ .rx_tbl = hawi_usb3_rx_tbl,
+ .rx_tbl_num = ARRAY_SIZE(hawi_usb3_rx_tbl),
+ .pcs_tbl = hawi_usb3_pcs_tbl,
+ .pcs_tbl_num = ARRAY_SIZE(hawi_usb3_pcs_tbl),
+ .pcs_usb_tbl = hawi_usb3_pcs_usb_tbl,
+ .pcs_usb_tbl_num = ARRAY_SIZE(hawi_usb3_pcs_usb_tbl),
+
+ .regs = qmp_v10_usb3phy_regs_layout,
[Severity: High]
Does this configuration miss the mandatory DisplayPort function pointers
like dp_aux_init, configure_dp_tx, and configure_dp_phy?
If a DisplayPort client calls phy_init() or phy_power_on(), the driver
executes qmp_combo_dp_init() and qmp_combo_dp_power_on() which
unconditionally call these function pointers. Because they are omitted here,
they will be NULL and cause a NULL pointer dereference regression.
quoted hunk ↗ jump to hunk
+ .reset_list = msm8996_usb3phy_reset_l,
+ .num_resets = ARRAY_SIZE(msm8996_usb3phy_reset_l),
+ .vreg_list = qmp_phy_vreg_l,
+ .num_vregs = ARRAY_SIZE(qmp_phy_vreg_l),
+};
+
static const struct qmp_phy_cfg sm6350_usb3dpphy_cfg = {
.offsets = &qmp_combo_offsets_v3,
[ ... ]
quoted hunk ↗ jump to hunk
@@ -3706,9 +3922,9 @@ static int qmp_combo_com_init(struct qmp_combo *qmp, bool force)
SW_USB3PHY_RESET_MUX | SW_USB3PHY_RESET);
/* override hardware control for reset of qmp phy */
- if (pcs_aon && cfg->regs[QPHY_AON_TOGGLE_ENABLE]) {
- qphy_clrbits(pcs_aon, cfg->regs[QPHY_AON_TOGGLE_ENABLE], 0x1);
- qphy_clrbits(pcs_aon, cfg->regs[QPHY_DP_AON_TOGGLE_ENABLE], 0x1);
+ if (aon_toggle && cfg->regs[QPHY_AON_TOGGLE_ENABLE]) {
[Severity: High]
Will this condition fail to clear the AON toggle registers for the Hawi V10
PHY?
Since QPHY_V10_COM_AON_USB3_AON_TOGGLE_ENABLE is defined as 0x00,
cfg->regs[QPHY_AON_TOGGLE_ENABLE] evaluates to 0. This causes the condition
to fail and skips the qphy_clrbits() execution, leaving the hardware reset
control improperly configured and potentially causing a regression.
quoted hunk ↗ jump to hunk
+ qphy_clrbits(aon_toggle, cfg->regs[QPHY_AON_TOGGLE_ENABLE], 0x1);
+ qphy_clrbits(aon_toggle, cfg->regs[QPHY_DP_AON_TOGGLE_ENABLE], 0x1);
}
/* Use software based port select and switch on typec orientation */
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260803091441.3040830-1-mukesh.ojha@oss.qualcomm.com?part=4
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy