Re: [PATCH v4 4/4] phy: qualcomm: qmp-combo: Add support for Hawi SoC

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 <[email protected]>

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.

> 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

[ ... ]

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

> +	.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,

[ ... ]

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

> +		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/[email protected]?part=4