Re: [PATCH v6 2/2] phy: qcom-qmp-ufs: Add UFS PHY support on Hawi

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Missing configuration overlay for UFS Gear 4 in Hawi UFS PHY
--

--- Patch [2]: [PATCH v6 2/2] phy: qcom-qmp-ufs: Add UFS PHY support on Hawi ---
commit 69d2ab29b4fbd3a27d861c7619c3a546898758d6
Author: Palash Kambar <[email protected]>

phy: qcom-qmp-ufs: Add UFS PHY support on Hawi

This patch adds the init sequence tables and configuration for the UFS QMP
PHY found in the Hawi SoC.

> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
> index 0f4ad24aa4058..d4aca22c181e4 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c

[ ... ]

> @@ -1845,6 +1868,119 @@ static const struct qmp_phy_cfg sm8750_ufsphy_cfg = {
>  
>  };
>  
> +static const struct qmp_phy_init_tbl hawi_ufsphy_serdes[] = {

[ ... ]

> +static const struct qmp_phy_init_tbl hawi_ufsphy_g5_pcs[] = {
> +	QMP_PHY_INIT_CFG(QPHY_V7_PCS_UFS_PLL_CNTL, 0x3b),
> +	QMP_PHY_INIT_CFG(QPHY_V7_PCS_UFS_TX_HSGEAR_CAPABILITY, 0x05),
> +	QMP_PHY_INIT_CFG(QPHY_V7_PCS_UFS_RX_HSGEAR_CAPABILITY, 0x05),
> +};
> +
> +static const struct qmp_phy_cfg hawi_ufsphy_cfg = {
> +	.lanes			= 2,
> +
> +	.offsets		= &qmp_ufs_offsets_v7,
> +	.max_supported_gear	= UFS_HS_G5,
> +
> +	.tbls = {
> +		.serdes		= hawi_ufsphy_serdes,
> +		.serdes_num	= ARRAY_SIZE(hawi_ufsphy_serdes),
> +		.tx		= hawi_ufsphy_tx,
> +		.tx_num		= ARRAY_SIZE(hawi_ufsphy_tx),
> +		.rx		= hawi_ufsphy_rx,
> +		.rx_num		= ARRAY_SIZE(hawi_ufsphy_rx),
> +		.pcs		= hawi_ufsphy_pcs,
> +		.pcs_num	= ARRAY_SIZE(hawi_ufsphy_pcs),
> +	},
> +
> +	.tbls_hs_overlay[0] = {
> +		.pcs		= hawi_ufsphy_g5_pcs,
> +		.pcs_num	= ARRAY_SIZE(hawi_ufsphy_g5_pcs),
> +		.max_gear	= UFS_HS_G5,
> +	},

[Severity: High]
Does this configuration need a fallback overlay for Gear 4?

Looking at qmp_ufs_get_gear_overlay(), if the UFS controller negotiates Gear 4
(or lower) with the connected UFS device, it will return -EINVAL because the
only overlay provided here has max_gear = UFS_HS_G5:

	for (idx = NUM_OVERLAY - 1; idx >= 0; idx--) {
		max_gear = cfg->tbls_hs_overlay[idx].max_gear;

		/* Skip if the table is not available */
		if (max_gear == 0)
			continue;

		/* Direct matching, bail */
		if (qmp->submode == max_gear)
			return idx;

		/* If no direct matching, the lowest gear is the best matching */
		if (max_gear < floor_max_gear) {
			ret = idx;
			floor_max_gear = max_gear;
		}
	}

	return ret;

Without an overlay where max_gear < 5, it fails to find a fallback. If the
overlay fails to apply, critical hardware configurations like
QPHY_V7_PCS_UFS_PLL_CNTL and the HSGEAR capabilities are omitted since they
are only present in the G5 overlay and not in the base hawi_ufsphy_pcs table.

Will this cause UFS link failure and render the storage unusable for any gear
below G5?

> +
> +	.vreg_list		= hawi_ufsphy_vreg_l,
> +	.num_vregs		= ARRAY_SIZE(hawi_ufsphy_vreg_l),
> +	.regs			= ufsphy_v7_regs_layout,
> +};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2