Re: [Intel-gfx] [PATCH 4/7] drm/i915/mtl: Add Support for C10 PHY message bus and pll programming

[Imre Deak <imre.deak@intel.com>]

On Tue, Apr 04, 2023 at 10:22:53PM +0300, Sripada, Radhakrishna wrote:
> 
> 
> > -----Original Message-----
> > From: Deak, Imre <imre.deak@intel.com>
> > Sent: Tuesday, April 4, 2023 11:03 AM
> > To: Sripada, Radhakrishna <radhakrishna.sripada@intel.com>
> > Cc: Kahola, Mika <mika.kahola@intel.com>; intel-gfx@lists.freedesktop.org;
> > Shankar, Uma <uma.shankar@intel.com>; Sousa, Gustavo
> > <gustavo.sousa@intel.com>
> > Subject: Re: [PATCH 4/7] drm/i915/mtl: Add Support for C10 PHY message bus
> > and pll programming
> >
> > On Tue, Apr 04, 2023 at 07:50:00PM +0300, Sripada, Radhakrishna wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Deak, Imre <imre.deak@intel.com>
> > > > Sent: Tuesday, April 4, 2023 6:28 AM
> > > > To: Kahola, Mika <mika.kahola@intel.com>
> > > > Cc: intel-gfx@lists.freedesktop.org; Sripada, Radhakrishna
> > > > <radhakrishna.sripada@intel.com>; Shankar, Uma
> > <uma.shankar@intel.com>;
> > > > Sousa, Gustavo <gustavo.sousa@intel.com>
> > > > Subject: Re: [PATCH 4/7] drm/i915/mtl: Add Support for C10 PHY message
> > bus
> > > > and pll programming
> > > >
> > > > On Tue, Apr 04, 2023 at 04:01:55PM +0300, Kahola, Mika wrote:
> > > > > [...]
> > > > > > >
> > > > > > > > > +void intel_c10mpllb_readout_hw_state(struct intel_encoder
> > *encoder,
> > > > > > > > > +                                struct intel_c10mpllb_state pll_state) {
> > > > > > > > > +   struct drm_i915_private *i915 = to_i915(encoder->base.dev);
> > > > > > > > > +   struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
> > > > > > > > > +   bool lane_reversal = dig_port->saved_port_bits &
> > DDI_BUF_PORT_REVERSAL;
> > > > > > > > > +   u8 lane = lane_reversal ? INTEL_CX0_LANE1 :
> > > > > > > > > +                             INTEL_CX0_LANE0;
> > > > > > > > > +   enum phy phy = intel_port_to_phy(i915, encoder->port);
> > > > > > > > > +   int i;
> > > > > > > > > +   u8 cmn, tx0;
> > > > > > > > > +
> > > > > > > > > +   /*
> > > > > > > > > +    * According to C10 VDR Register programming Sequence we
> > need
> > > > > > > > > +    * to do this to read PHY internal registers from MsgBus.
> > > > > > > > > +    */
> > > > > > > > > +   intel_cx0_rmw(i915, encoder->port, lane,
> > PHY_C10_VDR_CONTROL(1), 0,
> > > > > > > > > +                 C10_VDR_CTRL_MSGBUS_ACCESS,
> > MB_WRITE_COMMITTED);
> > > > > > > > > +
> > > > > > > > > +   for (i = 0; i < ARRAY_SIZE(pll_state->pll); i++)
> > > > > > > > > +           pll_state->pll[i] = intel_cx0_read(i915, encoder->port, lane,
> > PHY_C10_VDR_PLL(i));
> > > > > > > > > +
> > > > > > > > > +   cmn = intel_cx0_read(i915, encoder->port, lane,
> > PHY_C10_VDR_CMN(0));
> > > > > > > > > +   tx0 = intel_cx0_read(i915, encoder->port, lane,
> > PHY_C10_VDR_TX(0));
> > > > > > > >
> > > > > > > > The driver programs these registers, so why aren't they also stored
> > > > > > > > in the intell_c20pll_state struct?
> > > > > > >
> > > > > > > Maybe I'm not really following here but intel_c20pll_state has its own
> > > > > > > tx, cmn and mplla/mpllb stored.
> > > > > >
> > > > > > Yes, just typoed that, I meant struct intel_c10mpllb_state which
> > > > > > doesn't include tx and cmn.
> > > > >
> > > > > Yes, for C10 tx and cmn is missing. Maybe we could add those here as
> > > > > well. It seems that currently these are not necessary required but for
> > > > > the future use, these could be defined.
> > > >
> > > > These are needed already now to make the state computation / HW readout
> > /
> > > > state checking work for these two params the same way they do for the
> > > > rest of PLL state.
> > >
> > > I believe C10 tx and cmn values are not changing across frequencies. Cmn only
> > > Changes for DP and HDMI so does it make sense to include in the pll structure?
> >
> > They should be part of the atomic state. To save the bytes in the
> > precomputed tables they could be added to intel_cx0pll_state, something
> > like:
> >
> > struct intel_cx0pll_state {
> >         union {
> >                 struct {
> >                         struct intel_c10mpllb_state pllb;
> >                         u8 cmn;
> >                         u8 tx;
> >                 } c10;
> >                 struct intel_c20pll_state c20pll_state;
> >         };
> > };
> >
> I am bit concerned about the mismatch in the names for c10 and c20 states,
> adding further complexity in the structure may look more ugly. Let us afford the
> extra space in the tables if they need to be part of the atomic state and maintain
> homogeneity across c10 and c20 structures.

Both ways are better than the current way and fine by me.

> 
> Thoughts?
> 
> -RK
> > --Imre