On Tue, Jun 13, 2023 at 01:31:40AM +0300, Dmitry Baryshkov wrote: > On 13/06/2023 01:10, Bjorn Andersson wrote: > > From: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx> > > > > Some platforms provides a mechanism for configuring the mapping between > > (one or two) DisplayPort intfs and their PHYs. > > > > In particular SC8180X provides this functionality, without a default > > configuration, resulting in no connection between its two external > > DisplayPort controllers and any PHYs. > > > > The change implements the logic for optionally configuring which phy > > each of the intfs should be connected to, provides a new entry in the > > DPU catalog for specifying how many intfs to configure and marks the > > SC8180X DPU to program 2 entries. > > > > For now the request is simply to program the mapping 1:1, any support > > for alternative mappings is left until the use case arrise. > > > > Note that e.g. msm-4.14 unconditionally maps intf 0 to phy 0 on all > > rlatforms, so perhaps this is needed in order to get DisplayPort working > > on some other platforms as well. > > > > Signed-off-by: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx> > > Signed-off-by: Bjorn Andersson <quic_bjorande@xxxxxxxxxxx> > > --- > > .../msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h | 1 + > > .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 2 ++ > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c | 23 +++++++++++++++++++ > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h | 8 +++++++ > > drivers/gpu/drm/msm/disp/dpu1/dpu_hwio.h | 1 + > > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 10 ++++++++ > > 6 files changed, 45 insertions(+) > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h > > index 8ed2b263c5ea..9da952692a69 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h > > +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h > > @@ -19,6 +19,7 @@ static const struct dpu_caps sc8180x_dpu_caps = { > > .pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE, > > .max_hdeci_exp = MAX_HORZ_DECIMATION, > > .max_vdeci_exp = MAX_VERT_DECIMATION, > > + .num_dp_intf_sel = 2, > > }; > > static const struct dpu_ubwc_cfg sc8180x_ubwc_cfg = { > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h > > index ac4a9e73705c..4cb8d096d8ec 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h > > @@ -357,6 +357,7 @@ struct dpu_rotation_cfg { > > * @pixel_ram_size size of latency hiding and de-tiling buffer in bytes > > * @max_hdeci_exp max horizontal decimation supported (max is 2^value) > > * @max_vdeci_exp max vertical decimation supported (max is 2^value) > > + * @num_dp_intf_sel number of DP intfs to configure PHY selection for > > */ > > struct dpu_caps { > > u32 max_mixer_width; > > @@ -371,6 +372,7 @@ struct dpu_caps { > > u32 pixel_ram_size; > > u32 max_hdeci_exp; > > u32 max_vdeci_exp; > > + u32 num_dp_intf_sel; > > }; > > /** > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c > > index 963bdb5e0252..5afa99cb148c 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c > > @@ -250,6 +250,27 @@ static void dpu_hw_intf_audio_select(struct dpu_hw_mdp *mdp) > > DPU_REG_WRITE(c, HDMI_DP_CORE_SELECT, 0x1); > > } > > +static void dpu_hw_dp_phy_intf_sel(struct dpu_hw_mdp *mdp, unsigned int *phys, > > + unsigned int num_intfs) > > +{ > > + struct dpu_hw_blk_reg_map *c = &mdp->hw; > > + unsigned int intf; > > + u32 sel = 0; > > + > > + if (!num_intfs) > > + return; > > + > > + for (intf = 0; intf < num_intfs; intf++) { > > + /* Specify the PHY (1-indexed) for @intf */ > > + sel |= (phys[intf] + 1) << (intf * 3); > > + > > + /* Specify the @intf (1-indexed) of targeted PHY */ > > + sel |= (intf + 1) << (6 + phys[intf] * 3); > > From what I can see, phys[intf] is const. What about defining indexed masks > instead? > intf is the loop variable. What am I missing? > > + } > > + > > + DPU_REG_WRITE(c, DP_PHY_INTF_SEL, sel); > > +} > > + > > static void _setup_mdp_ops(struct dpu_hw_mdp_ops *ops, > > unsigned long cap) > > { > > @@ -264,6 +285,8 @@ static void _setup_mdp_ops(struct dpu_hw_mdp_ops *ops, > > ops->get_safe_status = dpu_hw_get_safe_status; > > + ops->dp_phy_intf_sel = dpu_hw_dp_phy_intf_sel; > > Should this be gated for DPU < 4.0? Or 5.0? > > > + > > if (cap & BIT(DPU_MDP_AUDIO_SELECT)) > > ops->intf_audio_select = dpu_hw_intf_audio_select; > > } > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h > > index a1a9e44bed36..8446d74d59b0 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h > > @@ -125,6 +125,14 @@ struct dpu_hw_mdp_ops { > > void (*get_safe_status)(struct dpu_hw_mdp *mdp, > > struct dpu_danger_safe_status *status); > > + /** > > + * dp_phy_intf_sel - configure intf to phy mapping > > + * @mdp: mdp top context driver > > + * @phys: list of phys the @num_intfs intfs should be connected to > > + * @num_intfs: number of intfs to configure > > + */ > > + void (*dp_phy_intf_sel)(struct dpu_hw_mdp *mdp, unsigned int *phys, > > + unsigned int num_intfs); > > /** > > * intf_audio_select - select the external interface for audio > > * @mdp: mdp top context driver > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hwio.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hwio.h > > index 5acd5683d25a..6d31bdc7269c 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hwio.h > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hwio.h > > @@ -59,6 +59,7 @@ > > #define MDP_WD_TIMER_4_CTL2 0x444 > > #define MDP_WD_TIMER_4_LOAD_VALUE 0x448 > > #define DCE_SEL 0x450 > > +#define DP_PHY_INTF_SEL 0x460 > > MDP_DP_PHY_INTF_SEL, if you don't mind. > I don't mind. > > #define MDP_PERIPH_TOP0 MDP_WD_TIMER_0_CTL > > #define MDP_PERIPH_TOP0_END CLK_CTRL3 > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > > index aa8499de1b9f..5dbe5d164c01 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > > @@ -1011,6 +1011,14 @@ unsigned long dpu_kms_get_clk_rate(struct dpu_kms *dpu_kms, char *clock_name) > > return clk_get_rate(clk); > > } > > +static void dpu_kms_dp_phy_intf_sel(struct dpu_kms *dpu_kms) > > +{ > > + const unsigned int num_intfs = dpu_kms->catalog->caps->num_dp_intf_sel; > > + static unsigned int phy_map[] = {0, 1, 2}; > > Please move this to dp_phy_intf_sel() and make it const. > There's a possible use case for passing a phy_map of {0, 2} or {2, 1} on SC8180X. While this is left to someone in the future to have that use case, as split dp_phy_intf_sel() would handle such variations. That said, per the layout of the DP_PHY_INTF_SEL, num_intfs can not be more than 2, so this list shouldn't have 3 elements. Regards, Bjorn > > + > > + dpu_kms->hw_mdp->ops.dp_phy_intf_sel(dpu_kms->hw_mdp, phy_map, num_intfs); > > +} > > + > > static int dpu_kms_hw_init(struct msm_kms *kms) > > { > > struct dpu_kms *dpu_kms; > > @@ -1122,6 +1130,8 @@ static int dpu_kms_hw_init(struct msm_kms *kms) > > goto perf_err; > > } > > + dpu_kms_dp_phy_intf_sel(dpu_kms); > > + > > dpu_kms->hw_intr = dpu_hw_intr_init(dpu_kms->mmio, dpu_kms->catalog); > > if (IS_ERR_OR_NULL(dpu_kms->hw_intr)) { > > rc = PTR_ERR(dpu_kms->hw_intr); > > -- > With best wishes > Dmitry >