> -----Original Message-----
> From: Deak, Imre <imre.deak@xxxxxxxxx>
> Sent: Tuesday, April 4, 2023 6:28 AM
> To: Kahola, Mika <mika.kahola@xxxxxxxxx>
> Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Sripada, Radhakrishna
> <radhakrishna.sripada@xxxxxxxxx>; Shankar, Uma <uma.shankar@xxxxxxxxx>;
> Sousa, Gustavo <gustavo.sousa@xxxxxxxxx>
> 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 on ly
Changes for DP and HDMI so does it make sense to include in the pll structure?
-RK
>
> > > > > > +
> > > > > > + 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.
>
> >
> > >
> > > > > > +}
> > > > > > +
