| Message ID | 1641819337-17037-3-git-send-email-quic_rajeevny@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/msm/dsi: Add 10nm dsi phy tuning configuration support | expand |
Hi Dmitry, > > > > + if (phy->cfg->ops.tuning_cfg_init) > > + phy->cfg->ops.tuning_cfg_init(phy); > > Please rename to parse_dt_properties() or something like that. Sure. I didn't understand your comment in v1 to use config_dt() hook. I think, now I understood. This function can be used to parse PHY version (nm) specific DT properties, currently we will be using it for PHY tuning parameters, and later some other properties can be added. I will use parse_dt_properties() in next post. Please correct me if I still didn't get it. > > > + > > ret = dsi_phy_regulator_init(phy); > > if (ret) > > goto fail; > > diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h > > b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h > > index b91303a..b559a2b 100644 > > --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h > > +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h > > @@ -25,6 +25,7 @@ struct msm_dsi_phy_ops { > > void (*save_pll_state)(struct msm_dsi_phy *phy); > > int (*restore_pll_state)(struct msm_dsi_phy *phy); > > bool (*set_continuous_clock)(struct msm_dsi_phy *phy, bool > > enable); > > + void (*tuning_cfg_init)(struct msm_dsi_phy *phy); > > }; > > > > struct msm_dsi_phy_cfg { > > @@ -81,6 +82,20 @@ struct msm_dsi_dphy_timing { > > #define DSI_PIXEL_PLL_CLK 1 > > #define NUM_PROVIDED_CLKS 2 > > > > +#define DSI_LANE_MAX 5 > > + > > +/** > > + * struct msm_dsi_phy_tuning_cfg - Holds PHY tuning config parameters. > > + * @rescode_offset_top: Offset for pull-up legs rescode. > > + * @rescode_offset_bot: Offset for pull-down legs rescode. > > + * @vreg_ctrl: vreg ctrl to drive LDO level */ struct > > +msm_dsi_phy_tuning_cfg { > > + u8 rescode_offset_top[DSI_LANE_MAX]; > > + u8 rescode_offset_bot[DSI_LANE_MAX]; > > + u8 vreg_ctrl; > > +}; > > How generic is this? In other words, you are adding a struct with the generic > name to the generic structure. I'd expect that it would be common to several > PHY generations. The 10nm is PHY v3.x, and the PHY tuning register configuration is same across DSI PHY v3.x devices. Similarly the PHY v4.x devices have same register configuration to adjust PHY tuning parameters. The v4.x has few changes as compared to v3.x : - rescode_offset_top: In v4.x, the value is not per lane, register is moved from PHY_LN_ block to PHY_CMN_ block. One value needed to configure all the lanes. Whereas in v3.x, configuration for each lane can be different. In case of v4.x, we can use rescode_offset_top[0] and ignore rest values. - rescode_offset_bot: same as rescode_offset_top comments given above. - vreg_ctrl: v4.x has two registers to adjust drive level, REG_DSI_7nm_PHY_CMN_VREG_CTRL_0 and REG_DSI_7nm_PHY_CMN_VREG_CTRL_1 The first one is the same for v3 and v4, only name is changed (_0 suffix) The second one REG_DSI_7nm_PHY_CMN_VREG_CTRL_1 is added to adjust mid-level amplitudes (C-PHY mode only) We can add a new member vreg_ctrl_1 in the "struct msm_dsi_phy_tuning_cfg" while adding PHY tuning support for V4.x Apart from the existing members, the v4.x has a few more register config options for drive strength adjustment and De-emphasis. We can add new members to address them for v4.x PHY tuning. The PHY v4.x is the latest PHY version, and most of the new devices are coming with v4.x. So, I can say that the structure member is going to remain the same for some time. (Things may/may not change in v5, but I never heard of it coming) Thanks, Rajeev > > > + > > struct msm_dsi_phy { > > struct platform_device *pdev; > > void __iomem *base; > > @@ -98,6 +113,7 @@ struct msm_dsi_phy { > > > > struct msm_dsi_dphy_timing timing; > > const struct msm_dsi_phy_cfg *cfg; > > + struct msm_dsi_phy_tuning_cfg tuning_cfg; > > > > enum msm_dsi_phy_usecase usecase; > > bool regulator_ldo_mode; > > -- > > 2.7.4 > > > > > -- > With best wishes > Dmitry
On Wed, 12 Jan 2022 at 19:09, Rajeev Nandan <RAJEEVNY@qti.qualcomm.com> wrote: > > Hi Dmitry, > > > > > > > + if (phy->cfg->ops.tuning_cfg_init) > > > + phy->cfg->ops.tuning_cfg_init(phy); > > > > Please rename to parse_dt_properties() or something like that. > Sure. I didn't understand your comment in v1 to use config_dt() hook. I think, now I understood. > This function can be used to parse PHY version (nm) specific DT properties, currently we will be using it for PHY tuning parameters, and later some other properties can be added. > I will use parse_dt_properties() in next post. Please correct me if I still didn't get it. You understanding follows my proposal, thank you. > > > > > > + > > > ret = dsi_phy_regulator_init(phy); > > > if (ret) > > > goto fail; > > > diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h > > > b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h > > > index b91303a..b559a2b 100644 > > > --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h > > > +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h > > > @@ -25,6 +25,7 @@ struct msm_dsi_phy_ops { > > > void (*save_pll_state)(struct msm_dsi_phy *phy); > > > int (*restore_pll_state)(struct msm_dsi_phy *phy); > > > bool (*set_continuous_clock)(struct msm_dsi_phy *phy, bool > > > enable); > > > + void (*tuning_cfg_init)(struct msm_dsi_phy *phy); > > > }; > > > > > > struct msm_dsi_phy_cfg { > > > @@ -81,6 +82,20 @@ struct msm_dsi_dphy_timing { > > > #define DSI_PIXEL_PLL_CLK 1 > > > #define NUM_PROVIDED_CLKS 2 > > > > > > +#define DSI_LANE_MAX 5 > > > + > > > +/** > > > + * struct msm_dsi_phy_tuning_cfg - Holds PHY tuning config parameters. > > > + * @rescode_offset_top: Offset for pull-up legs rescode. > > > + * @rescode_offset_bot: Offset for pull-down legs rescode. > > > + * @vreg_ctrl: vreg ctrl to drive LDO level */ struct > > > +msm_dsi_phy_tuning_cfg { > > > + u8 rescode_offset_top[DSI_LANE_MAX]; > > > + u8 rescode_offset_bot[DSI_LANE_MAX]; > > > + u8 vreg_ctrl; > > > +}; > > > > How generic is this? In other words, you are adding a struct with the generic > > name to the generic structure. I'd expect that it would be common to several > > PHY generations. > > The 10nm is PHY v3.x, and the PHY tuning register configuration is same across DSI PHY v3.x devices. > Similarly the PHY v4.x devices have same register configuration to adjust PHY tuning parameters. What about 14nm (v2.x, sdm660)? Or even 0.0-lpm (apq8016)? > > The v4.x has few changes as compared to v3.x : > - rescode_offset_top: > In v4.x, the value is not per lane, register is moved from PHY_LN_ block to PHY_CMN_ block. One value needed to configure all the lanes. > Whereas in v3.x, configuration for each lane can be different. > In case of v4.x, we can use rescode_offset_top[0] and ignore rest values. Ugh. > > - rescode_offset_bot: > same as rescode_offset_top comments given above. > > - vreg_ctrl: > v4.x has two registers to adjust drive level, REG_DSI_7nm_PHY_CMN_VREG_CTRL_0 and REG_DSI_7nm_PHY_CMN_VREG_CTRL_1 > The first one is the same for v3 and v4, only name is changed (_0 suffix) > The second one REG_DSI_7nm_PHY_CMN_VREG_CTRL_1 is added to adjust mid-level amplitudes (C-PHY mode only) > We can add a new member vreg_ctrl_1 in the "struct msm_dsi_phy_tuning_cfg" while adding PHY tuning support for V4.x > > Apart from the existing members, the v4.x has a few more register config options for drive strength adjustment and De-emphasis. > We can add new members to address them for v4.x PHY tuning. I do not like the idea of pushing each and every possible option into generic structure. I see two possible solutions: - Add opaque void pointer to struct msm_dsi_phy. Allow each driver to specify it on it's own. Or: - add a union of per-nm structures. From these two options I'm biassed towards the first one. It encapsulates the data structure with the using code. > > The PHY v4.x is the latest PHY version, and most of the new devices are coming with v4.x. So, I can say that the structure member is going to remain the same for some time. > (Things may/may not change in v5, but I never heard of it coming) > > Thanks, > Rajeev > > > > > + > > > struct msm_dsi_phy { > > > struct platform_device *pdev; > > > void __iomem *base; > > > @@ -98,6 +113,7 @@ struct msm_dsi_phy { > > > > > > struct msm_dsi_dphy_timing timing; > > > const struct msm_dsi_phy_cfg *cfg; > > > + struct msm_dsi_phy_tuning_cfg tuning_cfg; > > > > > > enum msm_dsi_phy_usecase usecase; > > > bool regulator_ldo_mode; > > > -- > > > 2.7.4 > > > > > > > > > -- > > With best wishes > > Dmitry
Hi Dmitry, > > On Wed, 12 Jan 2022 at 19:09, Rajeev Nandan > <RAJEEVNY@qti.qualcomm.com> wrote: > > > > Hi Dmitry, > > > > > > > > > > + if (phy->cfg->ops.tuning_cfg_init) > > > > + phy->cfg->ops.tuning_cfg_init(phy); > > > > > > Please rename to parse_dt_properties() or something like that. > > Sure. I didn't understand your comment in v1 to use config_dt() hook. I > think, now I understood. > > This function can be used to parse PHY version (nm) specific DT properties, > currently we will be using it for PHY tuning parameters, and later some other > properties can be added. > > I will use parse_dt_properties() in next post. Please correct me if I still > didn't get it. > > You understanding follows my proposal, thank you. > > > > > > > > > > + > > > > ret = dsi_phy_regulator_init(phy); > > > > if (ret) > > > > goto fail; > > > > diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h > > > > b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h > > > > index b91303a..b559a2b 100644 > > > > --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h > > > > +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h > > > > @@ -25,6 +25,7 @@ struct msm_dsi_phy_ops { > > > > void (*save_pll_state)(struct msm_dsi_phy *phy); > > > > int (*restore_pll_state)(struct msm_dsi_phy *phy); > > > > bool (*set_continuous_clock)(struct msm_dsi_phy *phy, bool > > > > enable); > > > > + void (*tuning_cfg_init)(struct msm_dsi_phy *phy); > > > > }; > > > > > > > > struct msm_dsi_phy_cfg { > > > > @@ -81,6 +82,20 @@ struct msm_dsi_dphy_timing { > > > > #define DSI_PIXEL_PLL_CLK 1 > > > > #define NUM_PROVIDED_CLKS 2 > > > > > > > > +#define DSI_LANE_MAX 5 > > > > + > > > > +/** > > > > + * struct msm_dsi_phy_tuning_cfg - Holds PHY tuning config > parameters. > > > > + * @rescode_offset_top: Offset for pull-up legs rescode. > > > > + * @rescode_offset_bot: Offset for pull-down legs rescode. > > > > + * @vreg_ctrl: vreg ctrl to drive LDO level */ struct > > > > +msm_dsi_phy_tuning_cfg { > > > > + u8 rescode_offset_top[DSI_LANE_MAX]; > > > > + u8 rescode_offset_bot[DSI_LANE_MAX]; > > > > + u8 vreg_ctrl; > > > > +}; > > > > > > How generic is this? In other words, you are adding a struct with > > > the generic name to the generic structure. I'd expect that it would > > > be common to several PHY generations. > > > > The 10nm is PHY v3.x, and the PHY tuning register configuration is same > across DSI PHY v3.x devices. > > Similarly the PHY v4.x devices have same register configuration to adjust > PHY tuning parameters. > > What about 14nm (v2.x, sdm660)? Or even 0.0-lpm (apq8016)? The 14nm (v2.x) has different registers and parameters for the DSI PHY tuning. Drive strength: DSIPHY_HSTX_STR_HSTOP & _HSBOT for each lane (Top/bottom branch drive strength adjustment) Drive level: NA Pre-emphasis: DSIPHY_PEMPH_STRBOT & _STRTOP for each lane (values are based on HSTX loading on the lane) The apq8016 is a very old chip and I couldn't find the tuning docs for it. I think going with your suggestion to allow each driver to specify its structure, we don't need to worry about the details of the tuning configs needed for each PHY versions. > > > > > The v4.x has few changes as compared to v3.x : > > - rescode_offset_top: > > In v4.x, the value is not per lane, register is moved from PHY_LN_ block to > PHY_CMN_ block. One value needed to configure all the lanes. > > Whereas in v3.x, configuration for each lane can be different. > > In case of v4.x, we can use rescode_offset_top[0] and ignore rest values. > > Ugh. > > > > > - rescode_offset_bot: > > same as rescode_offset_top comments given above. > > > > - vreg_ctrl: > > v4.x has two registers to adjust drive level, > REG_DSI_7nm_PHY_CMN_VREG_CTRL_0 and > REG_DSI_7nm_PHY_CMN_VREG_CTRL_1 > > The first one is the same for v3 and v4, only name is changed (_0 suffix) > > The second one REG_DSI_7nm_PHY_CMN_VREG_CTRL_1 is added to > adjust mid-level amplitudes (C-PHY mode only) > > We can add a new member vreg_ctrl_1 in the "struct > > msm_dsi_phy_tuning_cfg" while adding PHY tuning support for V4.x > > > > Apart from the existing members, the v4.x has a few more register config > options for drive strength adjustment and De-emphasis. > > We can add new members to address them for v4.x PHY tuning. > > I do not like the idea of pushing each and every possible option into generic > structure. > I see two possible solutions: > - Add opaque void pointer to struct msm_dsi_phy. Allow each driver to > specify it on it's own. > > Or: > > - add a union of per-nm structures. > > From these two options I'm biassed towards the first one. It encapsulates > the data structure with the using code. I agree with you, I will implement the first option. Thanks, Rajeev > > > > > > The PHY v4.x is the latest PHY version, and most of the new devices are > coming with v4.x. So, I can say that the structure member is going to remain > the same for some time. > > (Things may/may not change in v5, but I never heard of it coming) > > > > Thanks, > > Rajeev > > > > > > > + > > > > struct msm_dsi_phy { > > > > struct platform_device *pdev; > > > > void __iomem *base; > > > > @@ -98,6 +113,7 @@ struct msm_dsi_phy { > > > > > > > > struct msm_dsi_dphy_timing timing; > > > > const struct msm_dsi_phy_cfg *cfg; > > > > + struct msm_dsi_phy_tuning_cfg tuning_cfg; > > > > > > > > enum msm_dsi_phy_usecase usecase; > > > > bool regulator_ldo_mode; > > > > -- > > > > 2.7.4 > > > > > > > > > > > > > -- > > > With best wishes > > > Dmitry > > > > -- > With best wishes > Dmitry
diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c index 8c65ef6..ee3739d 100644 --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c @@ -739,6 +739,9 @@ static int dsi_phy_driver_probe(struct platform_device *pdev) } } + if (phy->cfg->ops.tuning_cfg_init) + phy->cfg->ops.tuning_cfg_init(phy); + ret = dsi_phy_regulator_init(phy); if (ret) goto fail; diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h index b91303a..b559a2b 100644 --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h @@ -25,6 +25,7 @@ struct msm_dsi_phy_ops { void (*save_pll_state)(struct msm_dsi_phy *phy); int (*restore_pll_state)(struct msm_dsi_phy *phy); bool (*set_continuous_clock)(struct msm_dsi_phy *phy, bool enable); + void (*tuning_cfg_init)(struct msm_dsi_phy *phy); }; struct msm_dsi_phy_cfg { @@ -81,6 +82,20 @@ struct msm_dsi_dphy_timing { #define DSI_PIXEL_PLL_CLK 1 #define NUM_PROVIDED_CLKS 2 +#define DSI_LANE_MAX 5 + +/** + * struct msm_dsi_phy_tuning_cfg - Holds PHY tuning config parameters. + * @rescode_offset_top: Offset for pull-up legs rescode. + * @rescode_offset_bot: Offset for pull-down legs rescode. + * @vreg_ctrl: vreg ctrl to drive LDO level + */ +struct msm_dsi_phy_tuning_cfg { + u8 rescode_offset_top[DSI_LANE_MAX]; + u8 rescode_offset_bot[DSI_LANE_MAX]; + u8 vreg_ctrl; +}; + struct msm_dsi_phy { struct platform_device *pdev; void __iomem *base; @@ -98,6 +113,7 @@ struct msm_dsi_phy { struct msm_dsi_dphy_timing timing; const struct msm_dsi_phy_cfg *cfg; + struct msm_dsi_phy_tuning_cfg tuning_cfg; enum msm_dsi_phy_usecase usecase; bool regulator_ldo_mode;
Add support for MSM DSI PHY tuning configuration. Current design is to support drive strength and drive level/amplitude tuning for 10nm PHY version, but this can be extended to other PHY versions. Signed-off-by: Rajeev Nandan <quic_rajeevny@quicinc.com> --- Changes in v2: - New. - Split into generic code and 10nm-specific part (Dmitry Baryshkov) drivers/gpu/drm/msm/dsi/phy/dsi_phy.c | 3 +++ drivers/gpu/drm/msm/dsi/phy/dsi_phy.h | 16 ++++++++++++++++ 2 files changed, 19 insertions(+)