Re: [PATCH v4 2/3] phy: cix: Add Sky1 DP PHY driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] phy: cix: incomplete hardware reset and state desync in cix_dp_reset()
--

commit ad82029ac97121969cc71e598d4b0b9c41076e02
Author: Luo Liu <Luo.Liu@cixtech.com>

phy: cix: Add Sky1 DP PHY driver

This commit adds a generic PHY driver for the CIX Sky1 dedicated DisplayPort
PMA, supporting link rate, lane count, SSC, and voltage swing configuration.

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

[ ... ]

> +static int cix_dp_hw_reset(struct cix_dp *dp)
> +{
> +	int ret;
> +
> +	ret = cix_dp_toggle_reset(dp);
> +	if (ret)
> +		return ret;
> +
> +	cix_dp_phy_write(dp, CIX_DP_PHY_DATA_ENABLE, 0);
> +	cix_dp_phy_write(dp, CIX_DP_PHY_LANE_RSTN_BITS, 0);
> +	cix_dp_phy_write(dp, CIX_DP_PHY_LANE_EN_BITS, 0);
> +	cix_dp_phy_write(dp, CIX_DP_PHY_CONFIG_RSTN, 0);
> +	dp->powered = false;
> +
> +	return 0;
> +}

[ ... ]

> +static int cix_dp_init(struct phy *phy)
> +{
> +	struct cix_dp *dp = phy_get_drvdata(phy);
> +	int ret;
> +
> +	ret = clk_prepare_enable(dp->apb_clk);
> +	if (ret)
> +		return ret;
> +
> +	ret = cix_dp_toggle_reset(dp);
> +	if (ret)
> +		goto disable_clk;
> +
> +	ret = cix_dp_hw_prepare(dp);
> +	if (ret)
> +		goto disable_clk;
> +
> +	ret = cix_dp_hw_load_regs(dp);
> +	if (ret)
> +		goto disable_clk;
> +
> +	return 0;
> +
> +disable_clk:
> +	clk_disable_unprepare(dp->apb_clk);
> +	return ret;
> +}

[ ... ]

> +static int cix_dp_reset(struct phy *phy)
> +{
> +	struct cix_dp *dp = phy_get_drvdata(phy);
> +
> +	return cix_dp_hw_reset(dp);
> +}

[Severity: High]
When a consumer calls phy_reset(), cix_dp_reset() invokes cix_dp_hw_reset(),
which pulses dp->reset via cix_dp_toggle_reset(), clears the control
registers, and sets dp->powered = false without calling cix_dp_hw_prepare()
or cix_dp_hw_load_regs().

In cix_dp_init(), pulsing dp->reset is followed by cix_dp_hw_prepare() (to
deassert CIX_DP_CDB_RSTN and enable CIX_DP_AUXHPD_ENABLE_BITS) and
cix_dp_hw_load_regs() (to program cix_dp_reg_conf[] and
CIX_DP_PHY_PMA_PLL_RAW_CTRL over the CDB bus).

Since pulsing the hardware reset resets CIX_DP_CDB_RSTN,
CIX_DP_AUXHPD_ENABLE_BITS, and the PMA registers back to hardware defaults,
and phy_reset() does not decrement phy->init_count in the PHY core, won't
subsequent calls to phy_configure() or phy_power_on() fail with -ETIMEDOUT in
cix_dp_phy_cdb_ready() because CIX_DP_CDB_RSTN remains asserted?

Additionally, if phy_reset() is called while the PHY is powered on
(phy->power_count > 0), setting dp->powered = false desynchronizes the driver
from phy->power_count so a subsequent phy_power_on() is skipped by the core
while phy_power_off() attempts CDB accesses on a reset PHY. Conversely, if
phy_reset() is called before phy_init() or after phy_exit(),
cix_dp_hw_reset() writes to dp->base while dp->apb_clk is disabled.

Should cix_dp_reset() re-run the hardware preparation and register loading
sequence (or should .reset be omitted if cix_dp_hw_reset() is only intended
for teardown in cix_dp_exit())?

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

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy