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 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;
        };
};

--Imre