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

[Sripada, Radhakrishna]

> -----Original Message-----
> From: Deak, Imre <imre.deak at intel.com>
> Sent: Tuesday, April 4, 2023 11:03 AM
> To: Sripada, Radhakrishna <radhakrishna.sripada at intel.com>
> Cc: Kahola, Mika <mika.kahola at intel.com>; intel-gfx at lists.freedesktop.org;
> Shankar, Uma <uma.shankar at intel.com>; Sousa, Gustavo
> <gustavo.sousa at 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 at intel.com>
> > > Sent: Tuesday, April 4, 2023 6:28 AM
> > > To: Kahola, Mika <mika.kahola at intel.com>
> > > Cc: intel-gfx at lists.freedesktop.org; Sripada, Radhakrishna
> > > <radhakrishna.sripada at intel.com>; Shankar, Uma
> <uma.shankar at intel.com>;
> > > Sousa, Gustavo <gustavo.sousa at 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.

Thoughts?

-RK
> --Imre