On Wed, Sep 19, 2018 at 05:31:37PM +0000, Kulkarni, Vandita wrote:
>
>
> > -----Original Message-----
> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > Sent: Friday, September 14, 2018 9:37 PM
> > To: Kulkarni, Vandita <vandita.kulkarni@xxxxxxxxx>
> > Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Nikula, Jani <jani.nikula@xxxxxxxxx>;
> > Zanoni, Paulo R <paulo.r.zanoni@xxxxxxxxx>
> > Subject: Re: [RFC 1/3] drm/i915/icl: Restructure ICL DPLL enable
> > functionality
> >
> > On Fri, Sep 14, 2018 at 12:24:12PM +0530, Vandita Kulkarni wrote:
> > > From: Madhav Chauhan <madhav.chauhan@xxxxxxxxx>
> > >
> > > In Gen11, DPLL 0 and 1 are shared between DDI and DSI.
> > > Most of the steps for enabling DPLL are common across DDI and DSI.
> > > This patch makes icl_dpll_enable() generic which will be used by all
> > > the encoders.
> > >
> > > Signed-off-by: Madhav Chauhan <madhav.chauhan@xxxxxxxxx>
> > > Signed-off-by: Vandita Kulkarni <vandita.kulkarni@xxxxxxxxx>
> > > ---
> > > drivers/gpu/drm/i915/intel_ddi.c | 12 ++++++++++++
> > > drivers/gpu/drm/i915/intel_dpll_mgr.c | 19 ++-----------------
> > > drivers/gpu/drm/i915/intel_dpll_mgr.h | 2 +-
> > > 3 files changed, 15 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> > > b/drivers/gpu/drm/i915/intel_ddi.c
> > > index cd01a09..2942a24 100644
> > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > @@ -2810,6 +2810,18 @@ static void intel_ddi_clk_select(struct
> > intel_encoder *encoder,
> > > mutex_lock(&dev_priv->dpll_lock);
> > >
> > > if (IS_ICELAKE(dev_priv)) {
> > > + enum intel_dpll_id id = pll->info->id;
> > > + i915_reg_t enable_reg = icl_pll_id_to_enable_reg(id);
> > > +
> > > + val = I915_READ(enable_reg);
> > > + val |= PLL_ENABLE;
> > > + I915_WRITE(enable_reg, val);
> > > +
> > > + /* TODO: wait times missing from the spec. */
> > > + if (intel_wait_for_register(dev_priv, enable_reg, PLL_LOCK,
> > > + PLL_LOCK, 5))
> > > + DRM_ERROR("PLL %d not locked\n", id);
> > > +
> >
> > I don't really see why this can't stay in the dpll mgr.
> This was part of icl_pll_enable, which gets called for all hdmi, dp and mipi dsi.
> But for mipi dsi we have couple of more things to do before we enable pll.
>
> In order to keep it unchanged for other encoders , pll enable was moved here.
> Another way could be have an encoder check in icl_pll_enable function and do those extra steps for mipi dsi and then enable the pll.
> Please let me know what you think would be a better way to do.
Hard to judge what is best without knowing what the differences are.
>
> Thanks,
> Vandita
> >
> > > if (port >= PORT_C)
> > > I915_WRITE(DDI_CLK_SEL(port),
> > > icl_pll_to_ddi_pll_sel(encoder, pll)); diff --git
> > > a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > index e6cac92..36ed155 100644
> > > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > @@ -2930,7 +2930,7 @@ static bool icl_calc_mg_pll_state(struct
> > intel_crtc_state *crtc_state,
> > > return pll;
> > > }
> > >
> > > -static i915_reg_t icl_pll_id_to_enable_reg(enum intel_dpll_id id)
> > > +i915_reg_t icl_pll_id_to_enable_reg(enum intel_dpll_id id)
> > > {
> > > switch (id) {
> > > default:
> > > @@ -3119,22 +3119,7 @@ static void icl_pll_enable(struct drm_i915_private
> > *dev_priv,
> > > default:
> > > MISSING_CASE(id);
> > > }
> > > -
> > > - /*
> > > - * DVFS pre sequence would be here, but in our driver the cdclk code
> > > - * paths should already be setting the appropriate voltage, hence we do
> > > - * nothign here.
> > > - */
> > > -
> > > - val = I915_READ(enable_reg);
> > > - val |= PLL_ENABLE;
> > > - I915_WRITE(enable_reg, val);
> > > -
> > > - if (intel_wait_for_register(dev_priv, enable_reg, PLL_LOCK, PLL_LOCK,
> > > - 1)) /* 600us actually. */
> > > - DRM_ERROR("PLL %d not locked\n", id);
> > > -
> > > - /* DVFS post sequence would be here. See the comment above. */
> > > + /* Encoder specific PLL enable steps are added in encoder file */
> > > }
> > >
> > > static void icl_pll_disable(struct drm_i915_private *dev_priv, diff
> > > --git a/drivers/gpu/drm/i915/intel_dpll_mgr.h
> > > b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> > > index bf0de8a..9e89265 100644
> > > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.h
> > > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> > > @@ -345,5 +345,5 @@ void intel_dpll_dump_hw_state(struct
> > > drm_i915_private *dev_priv, int icl_calc_dp_combo_pll_link(struct
> > drm_i915_private *dev_priv,
> > > uint32_t pll_id);
> > > int cnl_hdmi_pll_ref_clock(struct drm_i915_private *dev_priv);
> > > -
> > > +i915_reg_t icl_pll_id_to_enable_reg(enum intel_dpll_id id);
> > > #endif /* _INTEL_DPLL_MGR_H_ */
> > > --
> > > 1.9.1
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> > --
> > Ville Syrjälä
> > Intel
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
