> -----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.
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
