Re: [PATCH v2 3/4] phy: rockchip-samsung-dcphy: model TX and RX as separate PHYs

Hi Jason,

Thanks for the patch!

On 7/26/26 16:55, Jason Yang via B4 Relay wrote:
> From: Jason Yang <[email protected]>
> 
> The DC-PHY drives a MIPI DSI transmitter and a MIPI CSI receiver, and on
> RK3588 both can be wired to the same PHY as independent consumers. The
> PHY core reference-counts power_on() per struct phy, so a single struct
> phy cannot bring the two directions up independently.
> 
> Register one struct phy per direction and move the per-direction state
> (direction, PHY type, lane count and powered flag) into its own driver
> data. of_xlate() accepts the legacy single cell as the transmitter and
> an optional second cell that selects the direction, so existing
> single-cell DSI device trees keep resolving to the transmitter.
> configure() rejects the not yet supported receiver before touching any
> state, and each direction keeps its own lane count.
> 
> The PHY's common block contains a single BIAS block shared by both
> directions (RK3588 TRM section 22.2), so it must be programmed only
> once. Whichever direction powers on first enables it; each direction
> records its powered state under the per-provider mutex so that powering
> one direction on does not disturb an already running one. The APB reset
> is issued on the transmitter bring-up path.
> The receiver bring-up itself is added in a later change.
> 
> Signed-off-by: Jason Yang <[email protected]>
> Assisted-by: Claude:claude-opus-4-8
> ---
>  drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c | 143 +++++++++++++++-------
>  1 file changed, 102 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c b/drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c
> index d0d77421bd4b..95eb1200cab4 100644
> --- a/drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c
> +++ b/drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c
> @@ -5,6 +5,7 @@
>   *      Guochun Huang <[email protected]>
>   */
>  
> +#include <dt-bindings/phy/rockchip,rk3588-mipi-dcphy.h>
>  #include <dt-bindings/phy/phy.h>
>  #include <linux/bitfield.h>
>  #include <linux/clk.h>
> @@ -13,6 +14,7 @@
>  #include <linux/kernel.h>
>  #include <linux/mfd/syscon.h>
>  #include <linux/module.h>
> +#include <linux/mutex.h>
>  #include <linux/of.h>
>  #include <linux/phy/phy.h>
>  #include <linux/platform_device.h>
> @@ -280,6 +282,19 @@ struct samsung_mipi_dcphy_plat_data {
>  	u32 dphy_tx_max_lane_kbps;
>  };
>  
> +struct samsung_mipi_dcphy;
> +
> +/* One PHY per direction: transmitter (DSI) and receiver (CSI). */
> +struct samsung_mipi_dcphy_dir {

This may be bike-shedding, but I am not a big fan of the notion of
"direction". This is a combo PHY that in essence contains different
PHYs, so why not call this "samsung_mipi_phy" or something?

> +	struct phy *phy;
> +	struct samsung_mipi_dcphy *parent;
> +	u8 dir;
> +	u8 type;
> +	unsigned int lanes;
> +	/* Written under the parent's lock. */
> +	bool powered;
> +};
> +
>  struct samsung_mipi_dcphy {
>  	struct device *dev;
>  	struct clk *ref_clk;
> @@ -290,9 +305,9 @@ struct samsung_mipi_dcphy {
>  	struct reset_control *s_phy_rst;
>  	struct reset_control *apb_rst;
>  	struct reset_control *grf_apb_rst;
> -	unsigned int lanes;
> -	struct phy *phy;
> -	u8 type;
> +	struct samsung_mipi_dcphy_dir phys[2];

This would look like this then:

	struct samsung_mipi_phy phys[2];

which seems clean and simple to me.

> +	/* Serialises the two directions' access to the shared PHY state. */
> +	struct mutex lock;
>  
>  	const struct samsung_mipi_dcphy_plat_data *pdata;
>  	struct {
> @@ -995,7 +1010,7 @@ static void samsung_mipi_dphy_lane_enable(struct samsung_mipi_dcphy *samsung)
>  	regmap_update_bits(samsung->regmap, DPHY_MC_GNR_CON0,
>  			   PHY_ENABLE, PHY_ENABLE);
>  
> -	switch (samsung->lanes) {
> +	switch (samsung->phys[RK_DCPHY_DIR_TX].lanes) {
>  	case 4:
>  		regmap_write(samsung->regmap, DPHY_MD3_GNR_CON1,
>  			     T_PHY_READY(0x2000));
> @@ -1026,7 +1041,7 @@ static void samsung_mipi_dphy_lane_enable(struct samsung_mipi_dcphy *samsung)
>  
>  static void samsung_mipi_dphy_lane_disable(struct samsung_mipi_dcphy *samsung)
>  {
> -	switch (samsung->lanes) {
> +	switch (samsung->phys[RK_DCPHY_DIR_TX].lanes) {
>  	case 4:
>  		regmap_update_bits(samsung->regmap, DPHY_MD3_GNR_CON0,
>  				   PHY_ENABLE, 0);
> @@ -1334,11 +1349,23 @@ samsung_mipi_dphy_data_lane_timing_init(struct samsung_mipi_dcphy *samsung)
>  
>  static int samsung_mipi_dphy_tx_power_on(struct samsung_mipi_dcphy *samsung)
>  {
> +	bool first = !samsung->phys[RK_DCPHY_DIR_RX].powered;

Note to myself: the lock is acquired in the calling method, all is well.

>  	int ret;
>  
> +	/*
> +	 * The shared BIAS block is brought up by whichever direction powers
> +	 * on first, leaving an already active peer undisturbed.
> +	 */
> +	if (first) {
> +		reset_control_assert(samsung->apb_rst);
> +		udelay(1);
> +		reset_control_deassert(samsung->apb_rst);
> +	}
> +
>  	reset_control_assert(samsung->m_phy_rst);
>  
> -	samsung_mipi_dcphy_bias_block_enable(samsung);
> +	if (first)
> +		samsung_mipi_dcphy_bias_block_enable(samsung);

This approach with the "first" variable seems unintuitive. If you need
reference counting on the BIAS block, then use a state variable
"bias_powered", use a mutex that protects it, and call
samsung_mipi_dcphy_bias_block_{en,dis}able (from
samsung_mipi_dcphy_power{on, off}) unconditionally. Those methods shall
acquire the mutex, check the state variable, {en,dis}able the BIAS
block, set the state variable accordingly, release the mutex.


>  	samsung_mipi_dcphy_pll_configure(samsung);
>  	samsung_mipi_dphy_clk_lane_timing_init(samsung);
>  	samsung_mipi_dphy_data_lane_timing_init(samsung);
> @@ -1368,34 +1395,44 @@ static int samsung_mipi_dphy_tx_power_off(struct samsung_mipi_dcphy *samsung)
>  
>  static int samsung_mipi_dcphy_power_on(struct phy *phy)
>  {
> -	struct samsung_mipi_dcphy *samsung = phy_get_drvdata(phy);
> -
> -	reset_control_assert(samsung->apb_rst);
> -	udelay(1);
> -	reset_control_deassert(samsung->apb_rst);
> +	struct samsung_mipi_dcphy_dir *pd = phy_get_drvdata(phy);

Again, this may be bike-shedding, but "pd" seems misleading to me.
Please reconsider the naming. Maybe "samsung_phy" for the individual PHY
and "samsung" for the complete combo PHY? Or "phy_data" and
"dcphy_data"? ...?

> +	struct samsung_mipi_dcphy *samsung = pd->parent;
> +	int ret;
>  
> -	switch (samsung->type) {
> -	case PHY_TYPE_DPHY:
> -		return samsung_mipi_dphy_tx_power_on(samsung);
> -	default:
> -		/* CPHY part to be implemented later */
> +	if (pd->type != PHY_TYPE_DPHY)
>  		return -EOPNOTSUPP;
> -	}
>  
> -	return 0;
> +	mutex_lock(&samsung->lock);
> +	if (pd->dir == RK_DCPHY_DIR_RX)
> +		ret = -EOPNOTSUPP;
> +	else
> +		ret = samsung_mipi_dphy_tx_power_on(samsung);
> +	if (!ret)
> +		pd->powered = true;

Not sure whether you actually need to track the power status of the
individual PHYs (maybe you actually need to track the BIAS block status
alone) but anyway I would appreciate if
"samsung_mipi_dphy_tx_power_{on,off}" would set the "pd->powered" variable.

Best regards,
Michael


> +	mutex_unlock(&samsung->lock);
> +
> +	return ret;
>  }
>  
>  static int samsung_mipi_dcphy_power_off(struct phy *phy)
>  {
> -	struct samsung_mipi_dcphy *samsung = phy_get_drvdata(phy);
> +	struct samsung_mipi_dcphy_dir *pd = phy_get_drvdata(phy);
> +	struct samsung_mipi_dcphy *samsung = pd->parent;
> +	int ret;
>  
> -	switch (samsung->type) {
> -	case PHY_TYPE_DPHY:
> -		return samsung_mipi_dphy_tx_power_off(samsung);
> -	default:
> -		/* CPHY part to be implemented later */
> +	if (pd->type != PHY_TYPE_DPHY)
>  		return -EOPNOTSUPP;
> -	}
> +
> +	if (pd->dir == RK_DCPHY_DIR_RX)
> +		return -EOPNOTSUPP;
> +
> +	mutex_lock(&samsung->lock);
> +	ret = samsung_mipi_dphy_tx_power_off(samsung);
> +	if (!ret)
> +		pd->powered = false;
> +	mutex_unlock(&samsung->lock);
> +
> +	return ret;
>  }
>  
>  static int
> @@ -1488,10 +1525,14 @@ samsung_mipi_dcphy_pll_calc_rate(struct samsung_mipi_dcphy *samsung,
>  static int samsung_mipi_dcphy_configure(struct phy *phy,
>  					union phy_configure_opts *opts)
>  {
> -	struct samsung_mipi_dcphy *samsung = phy_get_drvdata(phy);
> +	struct samsung_mipi_dcphy_dir *pd = phy_get_drvdata(phy);
> +	struct samsung_mipi_dcphy *samsung = pd->parent;
>  	unsigned long long target_rate = opts->mipi_dphy.hs_clk_rate;
>  
> -	samsung->lanes = opts->mipi_dphy.lanes > 4 ? 4 : opts->mipi_dphy.lanes;
> +	if (pd->dir == RK_DCPHY_DIR_RX)
> +		return -EOPNOTSUPP;
> +
> +	pd->lanes = opts->mipi_dphy.lanes > 4 ? 4 : opts->mipi_dphy.lanes;
>  
>  	samsung_mipi_dcphy_pll_calc_rate(samsung, target_rate);
>  	opts->mipi_dphy.hs_clk_rate = samsung->pll.rate;
> @@ -1501,16 +1542,16 @@ static int samsung_mipi_dcphy_configure(struct phy *phy,
>  
>  static int samsung_mipi_dcphy_init(struct phy *phy)
>  {
> -	struct samsung_mipi_dcphy *samsung = phy_get_drvdata(phy);
> +	struct samsung_mipi_dcphy_dir *pd = phy_get_drvdata(phy);
>  
> -	return pm_runtime_resume_and_get(samsung->dev);
> +	return pm_runtime_resume_and_get(pd->parent->dev);
>  }
>  
>  static int samsung_mipi_dcphy_exit(struct phy *phy)
>  {
> -	struct samsung_mipi_dcphy *samsung = phy_get_drvdata(phy);
> +	struct samsung_mipi_dcphy_dir *pd = phy_get_drvdata(phy);
>  
> -	pm_runtime_put(samsung->dev);
> +	pm_runtime_put(pd->parent->dev);
>  
>  	return 0;
>  }
> @@ -1536,19 +1577,29 @@ static struct phy *samsung_mipi_dcphy_xlate(struct device *dev,
>  					    const struct of_phandle_args *args)
>  {
>  	struct samsung_mipi_dcphy *samsung = dev_get_drvdata(dev);
> +	struct samsung_mipi_dcphy_dir *pd;
> +	u8 dir = RK_DCPHY_DIR_TX;
>  
> -	if (args->args_count != 1) {
> +	if (args->args_count < 1 || args->args_count > 2) {
>  		dev_err(dev, "invalid number of arguments\n");
>  		return ERR_PTR(-EINVAL);
>  	}
>  
> -	if (samsung->type != PHY_NONE && samsung->type != args->args[0])
> -		dev_warn(dev, "phy type select %d overwriting type %d\n",
> -			 args->args[0], samsung->type);
> +	if (args->args_count == 2) {
> +		if (args->args[1] > RK_DCPHY_DIR_RX) {
> +			dev_err(dev, "invalid direction %u\n", args->args[1]);
> +			return ERR_PTR(-EINVAL);
> +		}
> +		dir = args->args[1];
> +	}
>  
> -	samsung->type = args->args[0];
> +	pd = &samsung->phys[dir];
> +	if (pd->type != PHY_NONE && pd->type != args->args[0])
> +		dev_warn(dev, "phy type select %u overwriting type %u\n",
> +			 args->args[0], pd->type);
> +	pd->type = args->args[0];
>  
> -	return samsung->phy;
> +	return pd->phy;
>  }
>  
>  static int samsung_mipi_dcphy_probe(struct platform_device *pdev)
> @@ -1559,6 +1610,7 @@ static int samsung_mipi_dcphy_probe(struct platform_device *pdev)
>  	struct phy_provider *phy_provider;
>  	struct resource *res;
>  	void __iomem *regs;
> +	unsigned int i;
>  	int ret;
>  
>  	samsung = devm_kzalloc(dev, sizeof(*samsung), GFP_KERNEL);
> @@ -1568,6 +1620,7 @@ static int samsung_mipi_dcphy_probe(struct platform_device *pdev)
>  	samsung->dev = dev;
>  	samsung->pdata = device_get_match_data(dev);
>  	platform_set_drvdata(pdev, samsung);
> +	mutex_init(&samsung->lock);
>  
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	regs = devm_ioremap_resource(dev, res);
> @@ -1613,11 +1666,19 @@ static int samsung_mipi_dcphy_probe(struct platform_device *pdev)
>  		return dev_err_probe(dev, PTR_ERR(samsung->grf_apb_rst),
>  				     "Failed to get system grf_apb_rst control\n");
>  
> -	samsung->phy = devm_phy_create(dev, NULL, &samsung_mipi_dcphy_ops);
> -	if (IS_ERR(samsung->phy))
> -		return dev_err_probe(dev, PTR_ERR(samsung->phy), "Failed to create MIPI DC-PHY\n");
> +	for (i = 0; i < ARRAY_SIZE(samsung->phys); i++) {
> +		struct phy *phy = devm_phy_create(dev, NULL,
> +						  &samsung_mipi_dcphy_ops);
>  
> -	phy_set_drvdata(samsung->phy, samsung);
> +		if (IS_ERR(phy))
> +			return dev_err_probe(dev, PTR_ERR(phy),
> +					     "Failed to create MIPI DC-PHY\n");
> +
> +		samsung->phys[i].phy = phy;
> +		samsung->phys[i].parent = samsung;
> +		samsung->phys[i].dir = i;
> +		phy_set_drvdata(phy, &samsung->phys[i]);
> +	}
>  
>  	ret = devm_pm_runtime_enable(dev);
>  	if (ret)
> 


-- 
linux-phy mailing list
[email protected]
https://lists.infradead.org/mailman/listinfo/linux-phy