回复: [PATCH v2 2/3] phy: cix: Add Sky1 DP PHY driver

[Luo Liu (刘罗)]

HI Sashiko，

	Thank you very much for your reply and suggestions.

-----邮件原件-----
发件人: sashiko-bot at kernel.org <sashiko-bot at kernel.org> 
发送时间: 2026年10月12日 20:48
收件人: Luo Liu (刘罗) <Luo.Liu at cixtech.com>
抄送: olteanv at gmail.com; devicetree at vger.kernel.org; vkoul at kernel.org; linux-phy at lists.infradead.org; neil.armstrong at linaro.org; conor+dt at kernel.org; robh at kernel.org
主题: Re: [PATCH v2 2/3] phy: cix: Add Sky1 DP PHY driver

[你通常不会收到来自 sashiko-bot at kernel.org 的电子邮件。请访问 https://aka.ms/LearnAboutSenderIdentification，以了解这一点为什么很重要]

EXTERNAL EMAIL

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] phy: cix: missing state restore on cix_dp_phy_set_lane_voltage() error path
- [Medium] phy: cix: Race condition in CDB transaction completion due to ignored busy pulse timeout
--

commit 813f2a99f8250a74e6b3856914829e75ea21a786
Author: Luo Liu <Luo.Liu at cixtech.com>

phy: cix: Add Sky1 DP PHY driver

This commit adds a PHY driver for the CIX Sky1 dedicated DisplayPort PMA.
It supports configuring the link rate, lane count, SSC, and voltage swing using the generic PHY DP operations.

> diff --git a/drivers/phy/cix/phy-cix-dp.c 
> b/drivers/phy/cix/phy-cix-dp.c new file mode 100644
> --- /dev/null
> +++ b/drivers/phy/cix/phy-cix-dp.c

[ ... ]

> +static int cix_dp_phy_cdb_complete(struct cix_dp *dp) {
> +     u32 value;
> +
> +     readl_poll_timeout_atomic(dp->base + CIX_DP_CDB_READY, value,
> +                               !(value & BIT(0)), 0, 1);
> +
> +     return cix_dp_phy_cdb_ready(dp); }

[Severity: Medium]
Does this explicitly suppress the return value from the busy-pulse poll?

If the hardware takes longer than 1us to drop the CIX_DP_CDB_READY bit to 0 after CIX_DP_CDB_ENABLE is written, the first polling loop will time out and the error is ignored. The second polling loop in cix_dp_phy_cdb_ready() could then immediately read the stale READY == 1 state and return success before the hardware actually processes the command.

Can this race condition lead to PHY initialization failures by allowing overlapping commands to be issued to the hardware?



Yes, the busy-poll return is intentionally ignored.

ASIC confirmed in RTL that READY drops within a few clocks after ENABLE, which is far below the 1 us atomic poll window, so we do observe busy before waiting for completion. Treating a missed busy pulse 

as a hard error previously caused false timeouts and broke display bring-up.



[ ... ]

> +static int cix_dp_phy_set_lane_voltage(struct cix_dp *dp, u32 lane,
> +                                    const struct cix_dp_voltage_conf 
> +*cfg) {
> +     int value;
> +     int ret;
> +
> +     value = cix_dp_phy_cdb_read(dp, CIX_DP_TX_DIAG_ACYA_LANE(lane));
> +     if (value < 0)
> +             return value;
> +
> +     ret = cix_dp_phy_cdb_write(dp, CIX_DP_TX_DIAG_ACYA_LANE(lane),
> +                                value | BIT(0));
> +     if (ret)
> +             return ret;
> +
> +     ret = cix_dp_phy_cdb_write(dp, CIX_DP_TX_TXCC_CTRL_LANE(lane),
> +                                cfg->tx_txcc_ctrl);
> +     if (ret)
> +             return ret;

[Severity: Medium]
If this write or any subsequent configuration write fails, the function returns immediately. Does this skip the cleanup step at the end of the function that restores the CIX_DP_TX_DIAG_ACYA_LANE register?

It appears that returning early leaves the hardware stuck in a diagnostic override state (with BIT(0) still set), which might silently corrupt future operations or link training attempts. Should these error paths ensure the original state is restored?

> +
> +     ret = cix_dp_phy_cdb_write(dp, CIX_DP_DRV_DIAG_TX_DRV_LANE(lane),
> +                                cfg->drv_diag_tx_drv);
> +     if (ret)
> +             return ret;
> +
> +     ret = cix_dp_phy_cdb_write(dp,
> +                                CIX_DP_TX_TXCC_MGNFS_MULT_000_LANE(lane),
> +                                cfg->tx_txcc_mgnfs_mult_000);
> +     if (ret)
> +             return ret;
> +
> +     ret = cix_dp_phy_cdb_write(dp,
> +                                CIX_DP_TX_TXCC_CPOST_MULT_00_LANE(lane),
> +                                cfg->tx_txcc_cpost_mult_00);
> +     if (ret)
> +             return ret;
> +
> +     return cix_dp_phy_cdb_write(dp, CIX_DP_TX_DIAG_ACYA_LANE(lane),
> +                                 value & ~BIT(0)); }



Good catch, thanks. Early returns after setting BIT(0) can leave TX_DIAG_ACYA stuck in the diagnostic override state. I'll fix the error paths in v3 so the original ACYA state is always restored.

Thanks,
Luo.

--
Sashiko AI review ・ https://sashiko.dev/#/patchset/20261012123618.648930-1-Luo.Liu@cixtech.com?part=2