Re: [Intel-gfx] [PATCH v2 09/21] drm/i915/mtl: C20 HW readout

[Imre Deak <imre.deak@intel.com>]

On Fri, Mar 24, 2023 at 05:51:11PM -0300, Gustavo Sousa wrote:
> On Thu, Feb 23, 2023 at 06:47:27AM -0300, Kahola, Mika wrote:
> > > > [...]
> > > > +void intel_c20pll_readout_hw_state(struct intel_encoder *encoder,
> > > > +				   struct intel_c20pll_state *pll_state) {
> > > > +	struct drm_i915_private *i915 = to_i915(encoder->base.dev);
> > > > +	bool cntx, use_mplla;
> > > > +	u32 val;
> > > > +	int i;
> > > > +
> > > > +        /* 1. Read current context selection */
> > > > +        cntx = intel_cx0_read(i915, encoder->port, 0, PHY_C20_VDR_CUSTOM_SERDES_RATE) & PHY_C20_CONTEXT_TOGGLE;
> > > > +
> > > > +	/* Read Tx configuration */
> > > > +	for (i = 0; i < ARRAY_SIZE(pll_state->tx); i++) {
> > > > +		if (cntx)
> > > > +			pll_state->tx[i] = intel_c20_read(i915, encoder->port, 0, PHY_C20_B_TX_CNTX_CFG(i));
> > > > +		else
> > > > +			pll_state->tx[i] = intel_c20_read(i915, encoder->port, 0, PHY_C20_A_TX_CNTX_CFG(i));
> > > > +	}
> > > > +
> > > > +	/* Read common configuration */
> > > > +	for (i = 0; i < ARRAY_SIZE(pll_state->cmn); i++) {
> > > > +		if (cntx)
> > > > +			pll_state->cmn[i] = intel_c20_read(i915, encoder->port, 0, PHY_C20_B_CMN_CNTX_CFG(i));
> > > > +		else
> > > > +			pll_state->cmn[i] = intel_c20_read(i915, encoder->port, 0, PHY_C20_A_CMN_CNTX_CFG(i));
> > > > +	}
> > > > +
> > > > +	val = intel_c20_read(i915, encoder->port, 0, PHY_C20_A_MPLLA_CNTX_CFG(6));
> > > > +	use_mplla = val & C20_MPLLB_FRACEN;
> > > 
> > >
> > > Some questions about this:
> > > 
> > >   1. This is hardcoded to read from context A. Shouldn't we read from the
> > >      current context?
> > > 
> > >   2. s/C20_MPLLB_FRACEN/C20_MPLLA_FRACEN/ ?
> >
> > The both uses the same bit 13 for frac_en. Maybe just renaming
> > differently C10_MPLLx_FRACEN?
> > > 
> > >   3. When we are programming PLL for MPLLB, we are not clearing
> > >      PHY_C20_A_MPLLA_CNTX_CFG(6). Wouldn't this be problematic if MPLLB is the
> > >      current one in use MPLLB but MPLLA was already used in a previous
> > >      configuration?
> >
> > Do you mean this when we are switching the context? In this case, as
> > far as I have understood the spec, we wouldn't need to clear
> > previous configuration.
> 
> Hi, Mika. Sorry to taking so long to reply!
> 
> What I mean is as follows. Consider the sequence of events below for an example:
> 
>   1. For a certain PLL programming, context A was used and MPLLA was selected.
>   2. For a new PLL programming, context B is used and we are not clearing
>      PHY_C20_A_MPLLA_CNTX_CFG(6).
>   3. Context A is used again for the next PLL programming, but this time MPLLB
>      is selected.
> 
> My concern is the following: *If* PHY_C20_A_MPLLA_CNTX_CFG(6) is not
> automatically cleared by the hardware in event (2), then after (3) this function
> would still think the current configuration is using MPLLA while it is in fact
> using MPLLB.
> 
> Now, if we have guarantee that PHY_C20_A_MPLLA_CNTX_CFG(6) is reset
> automatically when we switch to context B, then we wouldn't have to
> worry here.

Why the PLL's FRACEN flag used here? It's a detail in how the PLL is
programmed and may be either 0 or 1 (even if it's now happens to be 1).
I think PHY_C20_<cnxt>_TX_CNTX_CFG_0[7] should be used which is txX_mpllb_sel
(see Bspec 68862, C20 TX Context programming).

--Imre