Re: [PATCH v2 4/4] phy: rockchip-samsung-dcphy: add MIPI D-PHY receiver support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The RX power-on sequence fails to toggle the shared APB reset (`apb_rst`) when it is the first PHY direction to power on, potentially leaving shared PHY state uninitialized.
- [High] The RX lane enable function polls for `PHY_READY` while the PHY is still held in reset, guaranteeing a timeout failure.
--

--- Patch [4]: [PATCH v2 4/4] phy: rockchip-samsung-dcphy: add MIPI D-PHY receiver support ---
commit 8b1f5adbd06df56ad03e2b8d081a6587be5fe71d
Author: Jason Yang <jason98166@gmail.com>

phy: rockchip-samsung-dcphy: add MIPI D-PHY receiver support

This patch implements the receiver (CSI) direction of the D-PHY for the
Rockchip Samsung driver. It adds the RX register layout, HS-frequency settle
table, and hooks the receiver bring-up and teardown into the per-direction
power paths. The PLL sequence is also skipped for the receive path as the
sensor supplies the link clock.

> diff --git a/drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c b/drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c
> index 95eb1200cab48..f6d112935c016 100644
> --- a/drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c
> +++ b/drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c
[ ... ]
> @@ -988,6 +1046,228 @@ struct samsung_mipi_dphy_timing samsung_mipi_dphy_timing_table[] = {
[ ... ]
> +static int samsung_mipi_dphy_rx_lane_enable(struct samsung_mipi_dcphy *samsung)
> +{
> +	struct samsung_mipi_dcphy_dir *pd = &samsung->phys[RK_DCPHY_DIR_RX];
> +	u32 sts;
> +	int ret, i;
> +
> +	regmap_update_bits(samsung->regmap, DPHY_SC_GNR_CON0, PHY_ENABLE, PHY_ENABLE);
> +	for (i = 0; i < pd->lanes; i++)
> +		regmap_update_bits(samsung->regmap, rx_data_lane_gnr_con0[i],
> +				   PHY_ENABLE, PHY_ENABLE);
> +
> +	ret = regmap_read_poll_timeout(samsung->regmap, DPHY_SC_GNR_CON0,
> +				       sts, sts & PHY_READY, 200, 4000);

[Severity: High]
Does this code poll for PHY_READY while the PHY is still held in reset?

Looking at samsung_mipi_dphy_rx_power_on(), samsung->s_phy_rst is asserted,
then samsung_mipi_dphy_rx_lane_enable() is called, and only after that is
s_phy_rst deasserted. 

Will this cause the PHY_READY poll to always timeout?

> +	if (ret) {
> +		dev_err(samsung->dev, "RX clock lane not ready\n");
> +		goto err_lane_disable;
> +	}
[ ... ]
> @@ -1004,6 +1284,44 @@ static void samsung_mipi_dcphy_bias_block_enable(struct samsung_mipi_dcphy *sams
[ ... ]
> +static int samsung_mipi_dphy_rx_power_on(struct samsung_mipi_dcphy *samsung)
> +{
> +	bool first = !samsung->phys[RK_DCPHY_DIR_TX].powered;
> +	int ret;
> +
> +	if (!samsung->phys[RK_DCPHY_DIR_RX].hs_clk_rate)
> +		return -EINVAL;
> +
> +	reset_control_assert(samsung->s_phy_rst);
> +
> +	if (first)
> +		samsung_mipi_dcphy_bias_block_enable(samsung);

[Severity: High]
If the RX direction is powered on first, does it need to toggle the shared APB
reset (apb_rst) before enabling the bias block?

In samsung_mipi_dphy_tx_power_on(), when first is true, it does:

	if (first) {
		reset_control_assert(samsung->apb_rst);
		udelay(1);
		reset_control_deassert(samsung->apb_rst);
	}

Without this in the RX path, could the shared APB bus interface and BIAS
block remain in an uninitialized state from the bootloader?

> +	samsung_mipi_dphy_rx_config_common(samsung);
> +
> +	ret = samsung_mipi_dphy_rx_config_settle(samsung);
> +	if (ret)
> +		goto out_deassert;
> +
> +	ret = samsung_mipi_dphy_rx_lane_enable(samsung);
> +
> +out_deassert:
> +	reset_control_deassert(samsung->s_phy_rst);
> +
> +	return ret;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260726-dcphy-rx-v1-v2-0-cf9cc34a412a@gmail.com?part=4

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