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@xxxxxxxxxxxxxxx>
> > Cc: José Roberto de Souza <jose.souza@xxxxxxxxx>
> > Cc: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
> > Signed-off-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
> > ---
...
> > @@ -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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
