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 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.

> > > > > +
> > > > > +   if (tx0 != C10_TX0_VAL || cmn != C10_CMN0_DP_VAL)
> > > > > +           drm_warn(&i915->drm, "Unexpected tx: %x or cmn: %x for phy: %c.\n",
> > > > > +                    tx0, cmn, phy_name(phy));
> > > >
> > > > Shouldn't PHY_C10_VDR_CONTROL(1)/C10_VDR_CTRL_MSGBUS_ACCESS be
> > > > cleared here?
> > >
> > > Usually this means that we are not accessing these values from the
> > > register. Was this in the spec that we would need to clear it?
> >
> > It does get cleared at the end of intel_c10_pll_program(), at least
> > from one of the PHY lanes, so was wondering why things are done
> > differently here. Yes, the spec doesn't require clearing it, but
> > then it should not be cleared at other places either (has related
> > comments on this in follow-up reviews).
> 
> To be consistent maybe we can clear this here as well.

If there is no need for it, let's follow the spec and not clear it at
any other spot either.

> 
> >
> > > > > +}
> > > > > +