> -----Original Message-----
> From: Nikula, Jani <jani.nikula@xxxxxxxxx>
> Sent: Thursday, October 28, 2021 8:06 PM
> To: Kulkarni, Vandita <vandita.kulkarni@xxxxxxxxx>; intel-
> gfx@xxxxxxxxxxxxxxxxxxxxx
> Cc: Deak, Imre <imre.deak@xxxxxxxxx>; Roper, Matthew D
> <matthew.d.roper@xxxxxxxxx>; ville.syrjala@xxxxxxxxxxxxxxx
> Subject: RE: [V2 4/4] drm/i915/dsi: Ungate clock before enabling the phy
>
> On Thu, 28 Oct 2021, "Kulkarni, Vandita" <vandita.kulkarni@xxxxxxxxx>
> wrote:
> >> -----Original Message-----
> >> From: Nikula, Jani <jani.nikula@xxxxxxxxx>
> >> Sent: Thursday, October 28, 2021 5:13 PM
> >> To: Kulkarni, Vandita <vandita.kulkarni@xxxxxxxxx>; intel-
> >> gfx@xxxxxxxxxxxxxxxxxxxxx
> >> Cc: Deak, Imre <imre.deak@xxxxxxxxx>; Roper, Matthew D
> >> <matthew.d.roper@xxxxxxxxx>; ville.syrjala@xxxxxxxxxxxxxxx; Kulkarni,
> >> Vandita <vandita.kulkarni@xxxxxxxxx>
> >> Subject: Re: [V2 4/4] drm/i915/dsi: Ungate clock before enabling the
> >> phy
> >>
> >> On Tue, 19 Oct 2021, Vandita Kulkarni <vandita.kulkarni@xxxxxxxxx>
> wrote:
> >> > For the PHY enable/disable signalling to propagate between Dispaly
> >> > and PHY, DDI clocks need to be running when enabling the PHY.
> >> >
> >> > Bspec: 49188 says gate the clocks after enabling the
> >> > DDI Buffer.
> >> > We also have a commit 991d9557b0c4 ("drm/i915/tgl/dsi: Gate the
> ddi
> >> > clocks after pll mapping") which gates the clocks much before,
> >> > as per the older spec. This commit nullifies its effect and makes
> >> > sure that the clocks are not gated while we enable the DDI
> >> > buffer.
> >> > v2: Bspec ref, add a comment wrt earlier clock gating sequence
> >> > (Jani)
> >> >
> >> > Signed-off-by: Vandita Kulkarni <vandita.kulkarni@xxxxxxxxx>
> >> > ---
> >> > drivers/gpu/drm/i915/display/icl_dsi.c | 8 +++-----
> >> > 1 file changed, 3 insertions(+), 5 deletions(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/i915/display/icl_dsi.c
> >> > b/drivers/gpu/drm/i915/display/icl_dsi.c
> >> > index 63dd75c6448a..e5ef5c4a32d7 100644
> >> > --- a/drivers/gpu/drm/i915/display/icl_dsi.c
> >> > +++ b/drivers/gpu/drm/i915/display/icl_dsi.c
> >> > @@ -1135,8 +1135,6 @@ static void
> >> > gen11_dsi_enable_port_and_phy(struct intel_encoder *encoder,
> >> > const struct intel_crtc_state *crtc_state) {
> >> > - struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> >> > -
> >> > /* step 4a: power up all lanes of the DDI used by DSI */
> >> > gen11_dsi_power_up_lanes(encoder);
> >> >
> >> > @@ -1146,6 +1144,8 @@ gen11_dsi_enable_port_and_phy(struct
> >> intel_encoder *encoder,
> >> > /* step 4c: configure voltage swing and skew */
> >> > gen11_dsi_voltage_swing_program_seq(encoder);
> >> >
> >> > + gen11_dsi_ungate_clocks(encoder);
> >> > +
> >>
> >> What about the changes to gen11_dsi_map_pll() in commit 991d9557b0c4
> >> ("drm/i915/tgl/dsi: Gate the ddi clocks after pll mapping")? It
> >> starts off with clocks gated for gen12+, ungated otherwise.
> >
> > Now the same spec is updated with the gate step after ddi buffer enable.
> > And the one before is marked with remove tag.
> > That makes all gen12+ align with gen 11.
> > You suggested to update the same in the commit message on v1.
> > Should I still consider just reverting that commit?
>
> I'm just royally confused about the sequence myself, so I'm asking you. ;)
>
> It doesn't help that the code has step references to gen 11 mode set that are
> completely different from the steps in gen 12 sequence.
Right, they have lot of different steps coming in.
As per gen11 sequence, we were gating pll after enabling ddi buffer.
Initially when there was only gen12 in the bspec, it stated to gate the pll after mapping.
Hence we had that commit 991d9557b0c4.
Then Gen12's mipi dsi sequence was carried fwd for all later platforms as well.
with the modification saying that
Do not gate the pll until we enable the ddi buffer.
And this applies to gen 12 as well because they have marked the earlier mentioned step of gating pll
after pll mapping as removed on all gen12 and later platforms.
This patch now is keeping the older step as is, but making sure that clocks are ungated while enabling ddi buffer.
Thanks
Vandita
>
> BR,
> Jani.
>
>
>
> >
> > Thanks,
> > Vandita
> >
> >>
> >> BR,
> >> Jani.
> >>
> >>
> >> > /* enable DDI buffer */
> >> > gen11_dsi_enable_ddi_buffer(encoder);
> >> >
> >> > @@ -1161,9 +1161,7 @@ gen11_dsi_enable_port_and_phy(struct
> >> intel_encoder *encoder,
> >> > /* Step (4h, 4i, 4j, 4k): Configure transcoder */
> >> > gen11_dsi_configure_transcoder(encoder, crtc_state);
> >> >
> >> > - /* Step 4l: Gate DDI clocks */
> >> > - if (DISPLAY_VER(dev_priv) == 11)
> >> > - gen11_dsi_gate_clocks(encoder);
> >> > + gen11_dsi_gate_clocks(encoder);
> >> > }
> >> >
> >> > static void gen11_dsi_powerup_panel(struct intel_encoder *encoder)
> >>
> >> --
> >> Jani Nikula, Intel Open Source Graphics Center
>
> --
> Jani Nikula, Intel Open Source Graphics Center
