[PATCH v2 3/4] phy: rockchip-samsung-dcphy: model TX and RX as separate PHYs
楊智成
jason98166 at gmail.com
Mon Aug 3 22:38:40 PDT 2026
Hi Michael,
Thanks for the review.
> 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 samsung_mipi_phy phys[2];
>
> which seems clean and simple to me.
That reads better - the per-PHY structure becomes samsung_mipi_phy.
Taking your other naming comment together with this one:
> 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?
Sure, "pd" becomes "samsung_phy".
> 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.
>
> [...]
>
> 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.
Both of these lead to the same change. You are right to be unsure
about the per-PHY flag - I checked, and its only readers are the two
"first" computations, which only decide about the shared block. So the
two flags become one count in the parent, taken and dropped inside
samsung_mipi_dphy_{tx,rx}_power_{on,off}, and the "first" computations
go away. I went for a count rather than a single flag because the
release side has to distinguish "the other direction is still using
it" from "nobody is left".
The shared-block helper will use lockdep_assert_held() rather than
take the mutex itself, since samsung_mipi_dcphy_power_{on,off} already
hold it, as you noted.
One thing I am unsure about is the enable/disable pairing. I could not
find an enable for this block - the four BIAS registers only select
bias currents, reference voltages, the HS output level and the BGR
chopper divider, while PLL_CON0 in the same common block does have a
PLL_EN. If that reading is right, there is nothing to turn off and the
release side would only drop the count - in which case maybe the
"enable" name is worth revisiting as well. Does that match your
understanding of this block?
Separately, I noticed "first" also guards the APB reset, and only in
the transmitter path - once 4/4 enables the receiver, a receiver-first
power-on does not get one. I plan to move it into the same shared-block
path so both directions behave the same, and check both power-on
orders on the board before sending v3.
I would like to fold all of this into v3 together with whatever comes
out of the PHY_TYPE_CSI/PHY_TYPE_DSI discussion in patch 1/4, since
that decides how the two PHYs are told apart.
Thanks,
Jason
Michael Riesch <michael.riesch at collabora.com> 於 2026年8月3日週一 下午8:05寫道：
>
> Hi Jason,
>
> Thanks for the patch!
>
> On 7/26/26 16:55, Jason Yang via B4 Relay wrote:
> > From: Jason Yang <jason98166 at gmail.com>
> >
> > 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 <jason98166 at gmail.com>
> > 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 <hero.huang at rock-chips.com>
> > */
> >
> > +#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)
> >
>
More information about the linux-phy
mailing list