[Intel-gfx] [PATCH 4/5] drm/i915/gen11: Start distinguishing 'phy' from 'port'

[Matt Roper]

On Fri, Jun 21, 2019 at 05:24:10PM -0700, Souza, Jose wrote:
> On Fri, 2019-06-21 at 07:08 -0700, Matt Roper wrote:
> > Our past DDI-based Intel platforms have had a fixed DDI<->PHY
> > mapping.
> > Because of this, both the bspec documentation and our i915 code has
> > used
> > the term "port" when talking about either DDI's or PHY's; it was
> > always
> > easy to tell what terms like "Port A" were referring to from the
> > context.
> > 
> > Unfortunately this is starting to break down now that EHL allows PHY-
> > A
> > to be driven by either DDI-A or DDI-D.  Is a setup with DDI-D driving
> > PHY-A considered "Port A" or "Port D?"  The answer depends on which
> > register we're working with, and even the bspec doesn't do a great
> > job
> > of clarifying this.
> > 
> > Let's try to be more explicit about whether we're talking about the
> > DDI
> > or the PHY on gen11+ by using 'port' to refer to the DDI and creating
> > a
> > new 'enum phy' namespace to refer to the PHY in use.
> > 
> > A few general notes:
> > 
> >  - ICL_PORT_COMP_* and ICL_PORT_CL_* belong to the actual combo PHY
> > so
> >    they should always be programmed according to the PHY in use,
> >    regardless of which DDI is driving it.
> > 
> >  - The pipe part of the hardware expects "port" to refer to the
> >    DDI, so registers like TRANS_CLK_SEL and TRANS_DDI_FUNC_CTL should
> >    set bits according to the desired DDI (e.g., DDI-D) rather than
> > the
> >    PHY (PHY-A).
> > 
> >  - Non-pipe registers refer to the PHY.  Notably, DPCLKA_CFGCR0_ICL
> >    needs to set bits according to the PHY.
> > 
> > Most of the changes here are on the combo PHY side.  I didn't touch
> > most
> > of the TC port code yet, so it still refers to everything as ports.
> > That's okay for now since there's no TC on EHL, but we'll probably
> > want
> > to separate out the DDI vs PHY terminology for TC in the future as
> > well
> > to avoid confusion.
> > 
> > v2:
> >  - Convert a few more 'port' uses to 'phy.' (Sparse)
> > 
> > Suggested-by: Ville Syrjala <ville.syrjala at linux.intel.com>
> > Cc: José Roberto de Souza <jose.souza at intel.com>
> > Cc: Lucas De Marchi <lucas.demarchi at intel.com>
> > Signed-off-by: Matt Roper <matthew.d.roper at intel.com>
> > ---
...
> > @@ -9922,9 +9930,10 @@ static void cannonlake_get_ddi_pll(struct
> > drm_i915_private *dev_priv,
> >  {
> >  	enum intel_dpll_id id;
> >  	u32 temp;
> > +	enum phy phy = intel_port_to_phy(dev_priv, port);
> >  
> > -	temp = I915_READ(DPCLKA_CFGCR0) &
> > DPCLKA_CFGCR0_DDI_CLK_SEL_MASK(port);
> > -	id = temp >> DPCLKA_CFGCR0_DDI_CLK_SEL_SHIFT(port);
> > +	temp = I915_READ(DPCLKA_CFGCR0) &
> > DPCLKA_CFGCR0_DDI_CLK_SEL_MASK(phy);
> > +	id = temp >> DPCLKA_CFGCR0_DDI_CLK_SEL_SHIFT(phy);
> 
> 
> Should be port, same for the icelake_get_ddi_pll()

I agree with your other corrections, but I think the
icelake_get_ddi_pll() one needs to stay with PHY.  Bspec page 33148
indicates

        "DDID clock tied to DDIA clock, so DPCLKA_CFGCR0 DDIA Clock
        Select chooses the PLL for both DDIA and DDID and drives port A
        in all cases."

DPCLKA_CFGCR0_ICL doesn't have any clock select bits that correspond to
a "port D" (according to bspec page 15726) so I believe passing the PHY
to DPCLKA_CFGCR0_DDI_CLK_SEL is the right thing to do.  I should
probably add a comment clarifying this in the code.

For the cannonlake function I just used phy for consistency with the ICL
code (port=phy in all cases for CNL), but that's probably just adding
confusion so I'll switch it back to port in that one.


Matt

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795