RE: [v2 2/3] drm/msm/dsi: Add dsi phy tuning configuration support

[Rajeev Nandan]

Hi Dmitry,

> 
> On Wed, 12 Jan 2022 at 19:09, Rajeev Nandan
> <RAJEEVNY@xxxxxxxxxxxxxxxx> 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