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
