Re: [PATCH v4 17/39] drm/msm/dp: Add catalog support for 3rd/4th stream MST

[Dmitry Baryshkov]

On Mon, May 25, 2026 at 08:01:09PM +0800, Yongxing Mou wrote:
> 
> 
> On 5/25/2026 4:25 PM, Dmitry Baryshkov wrote:
> > On Mon, May 25, 2026 at 04:06:17PM +0800, Yongxing Mou wrote:
> > > 
> > > 
> > > On 4/12/2026 2:24 AM, Dmitry Baryshkov wrote:
> > > > Nit, there is no catalog anymore, please fix the subject line.
> > > > 
> > > > On Fri, Apr 10, 2026 at 05:33:52PM +0800, Yongxing Mou wrote:
> > > > > To support 4-stream MST, the link clocks for stream 3 and stream 4 are
> > > > > controlled by MST_2_LCLK and MST_3_LCLK which share the same register
> > > > > definitions but use different base addresses.
> > > > > 
> > > > > Signed-off-by: Yongxing Mou <yongxing.mou@xxxxxxxxxxxxxxxx>
> > > > > ---
> > > > >    drivers/gpu/drm/msm/dp/dp_ctrl.c    | 225 ++++++++++++++++++++++--------------
> > > > >    drivers/gpu/drm/msm/dp/dp_ctrl.h    |   4 +-
> > > > >    drivers/gpu/drm/msm/dp/dp_display.c |  24 +++-
> > > > >    drivers/gpu/drm/msm/dp/dp_panel.c   | 135 +++++++++++++++++-----
> > > > >    drivers/gpu/drm/msm/dp/dp_panel.h   |   2 +
> > > > >    drivers/gpu/drm/msm/dp/dp_reg.h     |  16 ++-
> > > > >    6 files changed, 283 insertions(+), 123 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> > > > > index a52bcd9ea2a3..1109b2df21be 100644
> > > > > --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> > > > > +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> > > > > @@ -118,6 +118,8 @@ struct msm_dp_ctrl_private {
> > > > >    	struct msm_dp_link *link;
> > > > >    	void __iomem *ahb_base;
> > > > >    	void __iomem *link_base;
> > > > > +	void __iomem *mst2link_base;
> > > > > +	void __iomem *mst3link_base;
> > > > >    	struct phy *phy;
> > > > > @@ -158,19 +160,45 @@ static inline void msm_dp_write_ahb(struct msm_dp_ctrl_private *ctrl,
> > > > >    	writel(data, ctrl->ahb_base + offset);
> > > > >    }
> > > > > -static inline u32 msm_dp_read_link(struct msm_dp_ctrl_private *ctrl, u32 offset)
> > > > > +static inline u32 msm_dp_read_link(struct msm_dp_ctrl_private *ctrl,
> > > > > +				   enum msm_dp_stream_id stream_id, u32 offset)
> > > > >    {
> > > > > -	return readl_relaxed(ctrl->link_base + offset);
> > > > > +	switch (stream_id) {
> > > > > +	case DP_STREAM_0:
> > > > > +	case DP_STREAM_1:
> > > > 
> > > > Continuing the comment from the previous patch:
> > > > 
> > > > Or, can we remap all offsets here? It would be much, much easier to
> > > > always pass REG_DP_foo and then here, in the link reading code, remap
> > > > the offsets for DP_STREAM_1.
> > > > 
> > > The stream 1 offsets relative to stream 0 are not uniform:
> > >      REG_DP_CONFIGURATION_CTRL  0x008 -> 0x400  (diff 0x3F8)
> > >      REG_DP_MISC1_MISC0         0x02C -> 0x42C  (diff 0x400)
> > >   So remapping inside msm_dp_read/write_link would require a full lookup
> > >   table, which seems not help for current status.
> > > 
> > > Can we introduce a small helper msm_dp_stream_reg() that selects the correct
> > > register address at the call site:
> > > //dp_panel.h
> > > static inline u32 msm_dp_stream_reg(enum msm_dp_stream_id id,
> > > 				     u32 reg_s0, u32 reg_s1, u32 reg_mst)
> > > {
> > > 	if (id > DP_STREAM_1)
> > > 		return reg_mst;
> > > 	return id == DP_STREAM_1 ? reg_s1 : reg_s0;
> > > }
> > 
> > No, that's what I want to get rid of. You are spreading these lookups
> > all over the code. Push them to a single function (or a single set of
> > functions, one per the stream, but having a single frontend to be called
> > by the rest of the driver).
> > 
> Okay. I should got how to do..
> void msm_dp_write_link ()
> {
> 	offset = msm_dp_stream_reg(stream_id, offset);
> 	switch (stream_id) {
> 	....
> }
> 
> msm_dp_stream_reg() will handle all the offset. then callers then simply do:
>   msm_dp_write_link(ctrl, panel->stream_id, REG_DP_MISC1_MISC0, val);
> Does this match what you had in mind?

Yes, exactly!

> > > 
> > > //dp_panel.c and dp_ctrl.c
> > >        u32 reg = msm_dp_stream_reg(stream_id,
> > >                                    REG_DP_MISC1_MISC0,
> > >                                    REG_DP1_MISC1_MISC0,
> > >                                    REG_DP_MSTLINK_MISC1_MISC0);
> > 
> > And then, if we need to support more streams or if the map for the
> > registers change, we'd need to go through all caller sites. No, that's a
> > bad idea.
> > 
> > >        msm_dp_read_link(ctrl, stream_id, reg);
> > > > > +		return readl_relaxed(ctrl->link_base + offset);
> > > > > +	case DP_STREAM_2:
> > > > > +		return readl_relaxed(ctrl->mst2link_base + offset);
> > > > > +	case DP_STREAM_3:
> > > > > +		return readl_relaxed(ctrl->mst3link_base + offset);
> > > > > +	default:
> > > > > +		DRM_ERROR("error stream_id\n");
> > > > > +		return 0;
> > > > > +	}
> > > > >    }

-- 
With best wishes
Dmitry