On Wed, 12 Jan 2022 at 19:09, Rajeev Nandan <rajee...@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
--
With best wishes
Dmitry
