On Tue, Apr 04, 2023 at 10:22:53PM +0300, Sripada, Radhakrishna wrote: > > > > -----Original Message----- > > From: Deak, Imre <[email protected]> > > Sent: Tuesday, April 4, 2023 11:03 AM > > To: Sripada, Radhakrishna <[email protected]> > > Cc: Kahola, Mika <[email protected]>; [email protected]; > > Shankar, Uma <[email protected]>; Sousa, Gustavo > > <[email protected]> > > 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 <[email protected]> > > > > Sent: Tuesday, April 4, 2023 6:28 AM > > > > To: Kahola, Mika <[email protected]> > > > > Cc: [email protected]; Sripada, Radhakrishna > > > > <[email protected]>; Shankar, Uma > > <[email protected]>; > > > > Sousa, Gustavo <[email protected]> > > > > 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
