Re: [PATCH 1/3] drm/bridge: analogix_dp: deassert DP_PHY_PD on power-on for exynos

[Alexandre Marquet <tb@a-marquet.fr>]

Hi,

Thank you for this feedback.

Le Mon, Nov 09, 2026 at 02:15:25PM +0900, Inki Dae a écrit :
> Hi,
> 
> 2026년 10월 27일 (화) 오후 9:17, Alexandre Marquet via B4 Relay
> <devnull+tb.a-marquet.fr@kernel.org>님이 작성:
> >
> > From: Alexandre Marquet <tb@a-marquet.fr>
> >
> > On exynos platforms at least, calling
> > analogix_dp_set_analog_power_down() to power on (with enable=false) CH0
> > to CH3 or the AUX bus currently produces no effect, unless the function
> > is first called to power on everything (by setting block to POWER_ALL or
> > ANALOG_TOTAL). The reason for this is that DP_PHY_PD being asserted
> > (which is its default state) renders every other register bit
> > ineffective.
> >
> > This is particularly an issue for AUX bus initialization, that tends to
> > happen before anything else (e.g., to request EDID), and thus, before
> > anything has any chance to request a power on of the whole analog
> > section of the chip.
> >
> > This commit allows analogix_dp_set_analog_power_down() to power on any
> > block, by deasserting DP_PHY_PD if needed.
> >
> > Signed-off-by: Alexandre Marquet <tb@a-marquet.fr>
> > ---
> >  drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c | 19 +++++++++++++------
> >  1 file changed, 13 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> > index c1344a3f013a..9f634d4e4793 100644
> > --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> > +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> > @@ -256,6 +256,19 @@ void analogix_dp_set_analog_power_down(struct analogix_dp_device *dp,
> >         if (dp->plat_data && analogix_dp_is_rockchip(dp->plat_data->dev_type))
> >                 phy_pd_addr = ANALOGIX_DP_PD;
> >
> > +       reg = readl(dp->reg_base + phy_pd_addr);
> > +
> > +       if (dp->plat_data && (dp->plat_data->dev_type == EXYNOS_DP)) {
> > +               /* On exynos, register AUX_PD and CH[0-3]_PD have no effect if
> > +                * DP_PHY_PD is asserted. So, when powering any of them up, we
> > +                * need to deassert DP_PHY_PD first.
> > +                */
> > +               if (!enable && (reg & DP_PHY_PD)) {
> > +                       reg &= ~DP_PHY_PD;
> > +                       reg |= AUX_PD | CH0_PD | CH1_PD | CH2_PD | CH3_PD;
> > +               }
> 
> When DP_PHY_PD is set, this path clears it but leaves the bandgap
> power-down bits unchanged. If the register retains 0xff from
> POWER_ALL(true), an AUX_BLOCK, false call produces 0xcf; both
> DP_INC_BG and DP_EXP_BG remain set. Could you check whether AUX can
> operate in that state?

It can. But what happens is that after calling POWER_ALL(true) (thus
setting the register to 0xff), when reading the register again, then I
get 0x7f.  Hence, after calling AUX_BLOCK(false), we get 0x4f and not
0xcf.  And although DP_INC_BG remains set as a result, AUX still works.
That being said, it sounds wrong not to deassert DP_INC_BG. This was
likely added to POWER_ALL for a reason... DP_EXP_BG, on the other hand,
does not seem to have any effect (and is not used anywhere, as far as I
can tell).

> I suggest limiting the new handling to AUX_BLOCK and CH[0-3]_BLOCK.
> When the bandgap bits are set, keep the other blocks powered down,
> release DP_INC_BG, wait as the existing POWER_ALL(false) path does,
> and then release the remaining required bits and the requested block.
> This also avoids changing ANALOG_TOTAL, false, which currently only
> clears DP_PHY_PD.

Right, in this case I'm leaning towards writing a small helper function
for AUX_BLOCK and CH[0-3]. Something like (not tested):

void analogix_dp_write_analog_power_down(struct analogix_dp_device *dp,
					 u32 phy_pd_addr,
					 u32 mask,
					 bool enable)
{
	u32 reg = readl(dp->reg_base + phy_pd_addr);

	if (enable)
		reg |= mask;
	else {
		if (reg & DP_INC_BG)) {
			reg |= AUX_PD | CH0_PD | CH1_PD | CH2_PD | CH3_PD;
			writel(reg, dp->reg_base + phy_pd_addr);
			usleep_range(10, 15);
			reg &= ~DP_INC_BG;
			writel(reg, dp->reg_base + phy_pd_addr);
			usleep_range(10, 15);
		}

		if (reg & DP_PHY_PD) {
			reg |= AUX_PD | CH0_PD | CH1_PD | CH2_PD | CH3_PD;
			reg &= ~DP_PHY_PD;
		}

		reg &= ~mask;
	}

	writel(reg, dp->reg_base + phy_pd_addr);
}

Also, since DP_INC_BG looks functionally equivalent to DP_PHY_PD on
rockchip, I'm tented to make the behaviour consistent accross exynos and
rockchip. But I have no rockchip board to test.

> 
> Thanks,
> Inki Dae
> 

Best regards,
Alex.

> > +       }
> > +
> >         switch (block) {
> >         case AUX_BLOCK:
> >                 if (dp->plat_data && analogix_dp_is_rockchip(dp->plat_data->dev_type))
> > @@ -263,7 +276,6 @@ void analogix_dp_set_analog_power_down(struct analogix_dp_device *dp,
> >                 else
> >                         mask = AUX_PD;
> >
> > -               reg = readl(dp->reg_base + phy_pd_addr);
> >                 if (enable)
> >                         reg |= mask;
> >                 else
> > @@ -272,7 +284,6 @@ void analogix_dp_set_analog_power_down(struct analogix_dp_device *dp,
> >                 break;
> >         case CH0_BLOCK:
> >                 mask = CH0_PD;
> > -               reg = readl(dp->reg_base + phy_pd_addr);
> >
> >                 if (enable)
> >                         reg |= mask;
> > @@ -282,7 +293,6 @@ void analogix_dp_set_analog_power_down(struct analogix_dp_device *dp,
> >                 break;
> >         case CH1_BLOCK:
> >                 mask = CH1_PD;
> > -               reg = readl(dp->reg_base + phy_pd_addr);
> >
> >                 if (enable)
> >                         reg |= mask;
> > @@ -292,7 +302,6 @@ void analogix_dp_set_analog_power_down(struct analogix_dp_device *dp,
> >                 break;
> >         case CH2_BLOCK:
> >                 mask = CH2_PD;
> > -               reg = readl(dp->reg_base + phy_pd_addr);
> >
> >                 if (enable)
> >                         reg |= mask;
> > @@ -302,7 +311,6 @@ void analogix_dp_set_analog_power_down(struct analogix_dp_device *dp,
> >                 break;
> >         case CH3_BLOCK:
> >                 mask = CH3_PD;
> > -               reg = readl(dp->reg_base + phy_pd_addr);
> >
> >                 if (enable)
> >                         reg |= mask;
> > @@ -321,7 +329,6 @@ void analogix_dp_set_analog_power_down(struct analogix_dp_device *dp,
> >                 else
> >                         mask = DP_PHY_PD;
> >
> > -               reg = readl(dp->reg_base + phy_pd_addr);
> >                 if (enable)
> >                         reg |= mask;
> >                 else
> >
> > --
> > 2.43.0
> >
> >
> >