| Message ID | 20240613-dp-phy-sel-v2-1-99af348c9bae@linaro.org |
|---|---|
| State | Accepted |
| Commit | be3415c620d1ed4776068bc17dbda876fbda6953 |
| Headers | show |
| Series | [RFC,v2] drm/msm/dpu: Configure DP INTF/PHY selector | expand |
On 6/24/2024 6:39 PM, Abhinav Kumar wrote: > > > On 6/13/2024 4:17 AM, Dmitry Baryshkov wrote: >> From: Bjorn Andersson <andersson@kernel.org> >> >> 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. >> > > I have to cross-check internally about what makes it mandatory to > program this only for sc8180xp. We were not programming this so far for > any chipset and this register is present all the way from sm8150 till > xe10100 and all the chipsets do not have a correct default value which > makes me think whether this is required to be programmed. > > Will update this thread once I do. > Ok, I checked more. The reason this is mandatory for sc8180xp is the number of controllers is greater than number of PHYs needing this to be programmed. On all other chipsets its a 1:1 mapping. I am fine with the change once the genmap comment is addressed. >> The change implements the logic for optionally configuring which PHY >> each of the DP INTFs should be connected to 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 <andersson@kernel.org> >> Co-developed-by: Bjorn Andersson <andersson@kernel.org> >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >> --- >> Changes in v2: >> - Removed entry from the catalog. >> - Reworked the interface of dpu_hw_dp_phy_intf_sel(). Pass two entries >> for the PHYs instead of three entries. >> - It seems the register isn't present on sdm845, enabled the callback >> only for DPU >= 5.x >> - Added a comment regarding the data being platform-specific. >> - Link to v1: >> https://lore.kernel.org/r/20230612221047.1886709-1-quic_bjorande@quicinc.com >> --- >> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c | 39 >> +++++++++++++++++++++++++++--- >> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h | 18 ++++++++++++-- >> drivers/gpu/drm/msm/disp/dpu1/dpu_hwio.h | 7 ++++++ >> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 11 ++++++++- >> 4 files changed, 69 insertions(+), 6 deletions(-) >> >> 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 05e48cf4ec1d..a11fdbefc8d2 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c >> @@ -231,8 +231,38 @@ 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, >> + enum dpu_dp_phy_sel phys[2]) >> +{ >> + struct dpu_hw_blk_reg_map *c = &mdp->hw; >> + unsigned int intf; >> + u32 sel = 0; >> + >> + sel |= FIELD_PREP(MDP_DP_PHY_INTF_SEL_INTF0, phys[0]); >> + sel |= FIELD_PREP(MDP_DP_PHY_INTF_SEL_INTF1, phys[1]); >> + >> + for (intf = 0; intf < 2; intf++) { > > I wonder if ARRAY_SIZE(phys) is better here. > >> + switch (phys[intf]) { >> + case DPU_DP_PHY_0: >> + sel |= FIELD_PREP(MDP_DP_PHY_INTF_SEL_PHY0, intf + 1); >> + break; >> + case DPU_DP_PHY_1: >> + sel |= FIELD_PREP(MDP_DP_PHY_INTF_SEL_PHY1, intf + 1); >> + break; >> + case DPU_DP_PHY_2: >> + sel |= FIELD_PREP(MDP_DP_PHY_INTF_SEL_PHY2, intf + 1); >> + break; >> + default: >> + /* ignore */ >> + break; >> + } >> + } >> + >> + DPU_REG_WRITE(c, MDP_DP_PHY_INTF_SEL, sel); >> +} >> + >> static void _setup_mdp_ops(struct dpu_hw_mdp_ops *ops, >> - unsigned long cap) >> + unsigned long cap, const struct dpu_mdss_version *mdss_rev) >> { >> ops->setup_split_pipe = dpu_hw_setup_split_pipe; >> ops->setup_clk_force_ctrl = dpu_hw_setup_clk_force_ctrl; >> @@ -245,6 +275,9 @@ static void _setup_mdp_ops(struct dpu_hw_mdp_ops >> *ops, >> ops->get_safe_status = dpu_hw_get_safe_status; >> + if (mdss_rev->core_major_ver >= 5) >> + ops->dp_phy_intf_sel = dpu_hw_dp_phy_intf_sel; >> + >> if (cap & BIT(DPU_MDP_AUDIO_SELECT)) >> ops->intf_audio_select = dpu_hw_intf_audio_select; >> } >> @@ -252,7 +285,7 @@ static void _setup_mdp_ops(struct dpu_hw_mdp_ops >> *ops, >> struct dpu_hw_mdp *dpu_hw_mdptop_init(struct drm_device *dev, >> const struct dpu_mdp_cfg *cfg, >> void __iomem *addr, >> - const struct dpu_mdss_cfg *m) >> + const struct dpu_mdss_version *mdss_rev) >> { >> struct dpu_hw_mdp *mdp; >> @@ -270,7 +303,7 @@ struct dpu_hw_mdp *dpu_hw_mdptop_init(struct >> drm_device *dev, >> * Assign ops >> */ >> mdp->caps = cfg; >> - _setup_mdp_ops(&mdp->ops, mdp->caps->features); >> + _setup_mdp_ops(&mdp->ops, mdp->caps->features, mdss_rev); >> return mdp; >> } >> 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 6f3dc98087df..3a17e63b851c 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h >> @@ -67,6 +67,13 @@ struct dpu_vsync_source_cfg { >> u32 vsync_source; >> }; >> +enum dpu_dp_phy_sel { >> + DPU_DP_PHY_NONE, >> + DPU_DP_PHY_0, >> + DPU_DP_PHY_1, >> + DPU_DP_PHY_2, >> +}; >> + >> /** >> * struct dpu_hw_mdp_ops - interface to the MDP TOP Hw driver functions >> * Assumption is these functions will be called after clocks are >> enabled. >> @@ -125,6 +132,13 @@ 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 DP interfaces should be connected to. >> 0 disables the INTF. >> + */ >> + void (*dp_phy_intf_sel)(struct dpu_hw_mdp *mdp, enum >> dpu_dp_phy_sel phys[2]); >> + >> /** >> * intf_audio_select - select the external interface for audio >> * @mdp: mdp top context driver >> @@ -148,12 +162,12 @@ struct dpu_hw_mdp { >> * @dev: Corresponding device for devres management >> * @cfg: MDP TOP configuration from catalog >> * @addr: Mapped register io address of MDP >> - * @m: Pointer to mdss catalog data >> + * @mdss_rev: dpu core's major and minor versions >> */ >> struct dpu_hw_mdp *dpu_hw_mdptop_init(struct drm_device *dev, >> const struct dpu_mdp_cfg *cfg, >> void __iomem *addr, >> - const struct dpu_mdss_cfg *m); >> + const struct dpu_mdss_version *mdss_rev); >> void dpu_hw_mdp_destroy(struct dpu_hw_mdp *mdp); >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hwio.h >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hwio.h >> index 5acd5683d25a..f1acc04089af 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hwio.h >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hwio.h >> @@ -60,6 +60,13 @@ >> #define MDP_WD_TIMER_4_LOAD_VALUE 0x448 >> #define DCE_SEL 0x450 >> +#define MDP_DP_PHY_INTF_SEL 0x460 >> +#define MDP_DP_PHY_INTF_SEL_INTF0 GENMASK(3, 0) >> +#define MDP_DP_PHY_INTF_SEL_INTF1 GENMASK(6, 3) >> +#define MDP_DP_PHY_INTF_SEL_PHY0 GENMASK(9, 6) >> +#define MDP_DP_PHY_INTF_SEL_PHY1 GENMASK(12, 9) >> +#define MDP_DP_PHY_INTF_SEL_PHY2 GENMASK(15, 12) > > These masks do not match the docs, the below ones are what I see: > > #define MDP_DP_PHY_INTF_SEL_INTF0 GENMASK(2, 0) > #define MDP_DP_PHY_INTF_SEL_INTF1 GENMASK(5, 3) > #define MDP_DP_PHY_INTF_SEL_PHY0 GENMASK(8, 6) > #define MDP_DP_PHY_INTF_SEL_PHY1 GENMASK(11, 9) > #define MDP_DP_PHY_INTF_SEL_PHY2 GENMASK(14, 12) > >> + >> #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 1955848b1b78..9db5a784c92f 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c >> @@ -1102,7 +1102,7 @@ static int dpu_kms_hw_init(struct msm_kms *kms) >> dpu_kms->hw_mdp = dpu_hw_mdptop_init(dev, >> dpu_kms->catalog->mdp, >> dpu_kms->mmio, >> - dpu_kms->catalog); >> + dpu_kms->catalog->mdss_ver); >> if (IS_ERR(dpu_kms->hw_mdp)) { >> rc = PTR_ERR(dpu_kms->hw_mdp); >> DPU_ERROR("failed to get hw_mdp: %d\n", rc); >> @@ -1137,6 +1137,15 @@ static int dpu_kms_hw_init(struct msm_kms *kms) >> goto err_pm_put; >> } >> + /* >> + * We need to program DP <-> PHY relationship only for SC8180X. >> If any >> + * other platform requires the same kind of programming, or if >> the INTF >> + * <->DP relationship isn't static anymore, this needs to be >> configured >> + * through the DT. >> + */ >> + if (of_device_is_compatible(dpu_kms->pdev->dev.of_node, >> "qcom,sc8180x-dpu")) >> + dpu_kms->hw_mdp->ops.dp_phy_intf_sel(dpu_kms->hw_mdp, >> (unsigned int[]){ 1, 2, }); >> + >> dpu_kms->hw_intr = dpu_hw_intr_init(dev, dpu_kms->mmio, >> dpu_kms->catalog); >> if (IS_ERR(dpu_kms->hw_intr)) { >> rc = PTR_ERR(dpu_kms->hw_intr); >> >> --- >> base-commit: 03d44168cbd7fc57d5de56a3730427db758fc7f6 >> change-id: 20240613-dp-phy-sel-1b06dc48ed73 >> >> Best regards,
On Tue, 25 Jun 2024 at 22:28, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > > > On 6/25/2024 12:26 PM, Abhinav Kumar wrote: > > > > > > On 6/24/2024 6:39 PM, Abhinav Kumar wrote: > >> > >> > >> On 6/13/2024 4:17 AM, Dmitry Baryshkov wrote: > >>> From: Bjorn Andersson <andersson@kernel.org> > >>> > >>> 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. > >>> > >> > >> I have to cross-check internally about what makes it mandatory to > >> program this only for sc8180xp. We were not programming this so far > >> for any chipset and this register is present all the way from sm8150 > >> till xe10100 and all the chipsets do not have a correct default value > >> which makes me think whether this is required to be programmed. > >> > >> Will update this thread once I do. > >> > > > > Ok, I checked more. The reason this is mandatory for sc8180xp is the > > number of controllers is greater than number of PHYs needing this to be > > programmed. On all other chipsets its a 1:1 mapping. > > > > Correction, number of controllers is < number of PHYs. Thanks, I'll c&p your explanation to the commit message if you don't mind. > > > I am fine with the change once the genmap comment is addressed.
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 05e48cf4ec1d..a11fdbefc8d2 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c @@ -231,8 +231,38 @@ 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, + enum dpu_dp_phy_sel phys[2]) +{ + struct dpu_hw_blk_reg_map *c = &mdp->hw; + unsigned int intf; + u32 sel = 0; + + sel |= FIELD_PREP(MDP_DP_PHY_INTF_SEL_INTF0, phys[0]); + sel |= FIELD_PREP(MDP_DP_PHY_INTF_SEL_INTF1, phys[1]); + + for (intf = 0; intf < 2; intf++) { + switch (phys[intf]) { + case DPU_DP_PHY_0: + sel |= FIELD_PREP(MDP_DP_PHY_INTF_SEL_PHY0, intf + 1); + break; + case DPU_DP_PHY_1: + sel |= FIELD_PREP(MDP_DP_PHY_INTF_SEL_PHY1, intf + 1); + break; + case DPU_DP_PHY_2: + sel |= FIELD_PREP(MDP_DP_PHY_INTF_SEL_PHY2, intf + 1); + break; + default: + /* ignore */ + break; + } + } + + DPU_REG_WRITE(c, MDP_DP_PHY_INTF_SEL, sel); +} + static void _setup_mdp_ops(struct dpu_hw_mdp_ops *ops, - unsigned long cap) + unsigned long cap, const struct dpu_mdss_version *mdss_rev) { ops->setup_split_pipe = dpu_hw_setup_split_pipe; ops->setup_clk_force_ctrl = dpu_hw_setup_clk_force_ctrl; @@ -245,6 +275,9 @@ static void _setup_mdp_ops(struct dpu_hw_mdp_ops *ops, ops->get_safe_status = dpu_hw_get_safe_status; + if (mdss_rev->core_major_ver >= 5) + ops->dp_phy_intf_sel = dpu_hw_dp_phy_intf_sel; + if (cap & BIT(DPU_MDP_AUDIO_SELECT)) ops->intf_audio_select = dpu_hw_intf_audio_select; } @@ -252,7 +285,7 @@ static void _setup_mdp_ops(struct dpu_hw_mdp_ops *ops, struct dpu_hw_mdp *dpu_hw_mdptop_init(struct drm_device *dev, const struct dpu_mdp_cfg *cfg, void __iomem *addr, - const struct dpu_mdss_cfg *m) + const struct dpu_mdss_version *mdss_rev) { struct dpu_hw_mdp *mdp; @@ -270,7 +303,7 @@ struct dpu_hw_mdp *dpu_hw_mdptop_init(struct drm_device *dev, * Assign ops */ mdp->caps = cfg; - _setup_mdp_ops(&mdp->ops, mdp->caps->features); + _setup_mdp_ops(&mdp->ops, mdp->caps->features, mdss_rev); return mdp; } 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 6f3dc98087df..3a17e63b851c 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h @@ -67,6 +67,13 @@ struct dpu_vsync_source_cfg { u32 vsync_source; }; +enum dpu_dp_phy_sel { + DPU_DP_PHY_NONE, + DPU_DP_PHY_0, + DPU_DP_PHY_1, + DPU_DP_PHY_2, +}; + /** * struct dpu_hw_mdp_ops - interface to the MDP TOP Hw driver functions * Assumption is these functions will be called after clocks are enabled. @@ -125,6 +132,13 @@ 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 DP interfaces should be connected to. 0 disables the INTF. + */ + void (*dp_phy_intf_sel)(struct dpu_hw_mdp *mdp, enum dpu_dp_phy_sel phys[2]); + /** * intf_audio_select - select the external interface for audio * @mdp: mdp top context driver @@ -148,12 +162,12 @@ struct dpu_hw_mdp { * @dev: Corresponding device for devres management * @cfg: MDP TOP configuration from catalog * @addr: Mapped register io address of MDP - * @m: Pointer to mdss catalog data + * @mdss_rev: dpu core's major and minor versions */ struct dpu_hw_mdp *dpu_hw_mdptop_init(struct drm_device *dev, const struct dpu_mdp_cfg *cfg, void __iomem *addr, - const struct dpu_mdss_cfg *m); + const struct dpu_mdss_version *mdss_rev); void dpu_hw_mdp_destroy(struct dpu_hw_mdp *mdp); diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hwio.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hwio.h index 5acd5683d25a..f1acc04089af 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hwio.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hwio.h @@ -60,6 +60,13 @@ #define MDP_WD_TIMER_4_LOAD_VALUE 0x448 #define DCE_SEL 0x450 +#define MDP_DP_PHY_INTF_SEL 0x460 +#define MDP_DP_PHY_INTF_SEL_INTF0 GENMASK(3, 0) +#define MDP_DP_PHY_INTF_SEL_INTF1 GENMASK(6, 3) +#define MDP_DP_PHY_INTF_SEL_PHY0 GENMASK(9, 6) +#define MDP_DP_PHY_INTF_SEL_PHY1 GENMASK(12, 9) +#define MDP_DP_PHY_INTF_SEL_PHY2 GENMASK(15, 12) + #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 1955848b1b78..9db5a784c92f 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c @@ -1102,7 +1102,7 @@ static int dpu_kms_hw_init(struct msm_kms *kms) dpu_kms->hw_mdp = dpu_hw_mdptop_init(dev, dpu_kms->catalog->mdp, dpu_kms->mmio, - dpu_kms->catalog); + dpu_kms->catalog->mdss_ver); if (IS_ERR(dpu_kms->hw_mdp)) { rc = PTR_ERR(dpu_kms->hw_mdp); DPU_ERROR("failed to get hw_mdp: %d\n", rc); @@ -1137,6 +1137,15 @@ static int dpu_kms_hw_init(struct msm_kms *kms) goto err_pm_put; } + /* + * We need to program DP <-> PHY relationship only for SC8180X. If any + * other platform requires the same kind of programming, or if the INTF + * <->DP relationship isn't static anymore, this needs to be configured + * through the DT. + */ + if (of_device_is_compatible(dpu_kms->pdev->dev.of_node, "qcom,sc8180x-dpu")) + dpu_kms->hw_mdp->ops.dp_phy_intf_sel(dpu_kms->hw_mdp, (unsigned int[]){ 1, 2, }); + dpu_kms->hw_intr = dpu_hw_intr_init(dev, dpu_kms->mmio, dpu_kms->catalog); if (IS_ERR(dpu_kms->hw_intr)) { rc = PTR_ERR(dpu_kms->hw_intr);