On Tue, Apr 04, 2023 at 10:22:53PM +0300, Sripada, Radhakrishna wrote:
>
>
> > -----Original Message-----
> > From: Deak, Imre <imre.deak@xxxxxxxxx>
> > Sent: Tuesday, April 4, 2023 11:03 AM
> > To: Sripada, Radhakrishna <radhakrishna.sripada@xxxxxxxxx>
> > Cc: Kahola, Mika <mika.kahola@xxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx;
> > 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 07:50:00PM +0300, Sripada, Radhakrishna wrote:
> > >
> > >
> > > > -----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 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
