On Wed, 2016-10-05 at 17:51 +0300, Imre Deak wrote:
> On ke, 2016-10-05 at 15:09 +0300, Ander Conselvan de Oliveira wrote:
> >
> > Use struct bxt_ddi_phy_info to hold information of where the Rcomp
> > resistor is located, instead of hard coding it in the init sequence.
> >
> > Note that this moves the enabling of the phy with the Rcomp resistor out
> > of the power well enable code. That should be safe since
> > bxt_ddi_phy_init() is called while the power domains lock is held, and
> > that is the only way that function gets called, so there is no
> > possibility of a concurrent phy enable caused by a power domain get
> > call.
> >
> > Signed-off-by: Ander Conselvan de Oliveira
> > ---
> > drivers/gpu/drm/i915/intel_dpio_phy.c | 76 +++++++++++++++++++++++++-----
> > ---
> > drivers/gpu/drm/i915/intel_runtime_pm.c | 15 -------
> > 2 files changed, 59 insertions(+), 32 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dpio_phy.c
> > b/drivers/gpu/drm/i915/intel_dpio_phy.c
> > index 66d750a..e8a75fd 100644
> > --- a/drivers/gpu/drm/i915/intel_dpio_phy.c
> > +++ b/drivers/gpu/drm/i915/intel_dpio_phy.c
> > @@ -124,6 +124,13 @@ struct bxt_ddi_phy_info {
> > bool dual_channel;
> >
> > /**
> > + * @rcomp_phy: If -1, indicates this phy has its own rcomp
> > resistor.
> > + * Otherwise the GRC value will be copied from the phy indicated by
> > + * this field.
> > + */
> > + enum dpio_phy rcomp_phy;
> > +
> > + /**
> > * @channel: struct containing per channel information.
> > */
> > struct {
> > @@ -137,6 +144,7 @@ struct bxt_ddi_phy_info {
> > static const struct bxt_ddi_phy_info bxt_ddi_phy_info[] = {
> > [DPIO_PHY0] = {
> > .dual_channel = true,
> > + .rcomp_phy = DPIO_PHY1,
> >
> > .channel = {
> > [DPIO_CH0] = { .port = PORT_B },
> > @@ -145,6 +153,7 @@ static const struct bxt_ddi_phy_info bxt_ddi_phy_info[]
> > = {
> > },
> > [DPIO_PHY1] = {
> > .dual_channel = false,
> > + .rcomp_phy = -1,
> >
> > .channel = {
> > [DPIO_CH0] = { .port = PORT_A },
> > @@ -152,6 +161,7 @@ static const struct bxt_ddi_phy_info bxt_ddi_phy_info[]
> > = {
> > },
> > };
> >
> > +static u32 bxt_phy_port_mask(const struct bxt_ddi_phy_info *phy_info)
> > {
> > return (phy_info->dual_channel * BIT(phy_info-
> > >channel[DPIO_CH1].port)) |
> > BIT(phy_info->channel[DPIO_CH0].port);
> > @@ -199,6 +209,7 @@ void bxt_ddi_phy_set_signal_level(struct
> > drm_i915_private *dev_priv,
> > bool bxt_ddi_phy_is_enabled(struct drm_i915_private *dev_priv,
> > enum dpio_phy phy)
> > {
> > + const struct bxt_ddi_phy_info *phy_info = &bxt_ddi_phy_info[phy];
> > enum port port;
> >
> > if (!(I915_READ(BXT_P_CR_GT_DISP_PWRON) &
> > GT_DISPLAY_POWER_ON(phy)))
> > @@ -212,9 +223,10 @@ bool bxt_ddi_phy_is_enabled(struct drm_i915_private
> > *dev_priv,
> > return false;
> > }
> >
> > - if (phy == DPIO_PHY1 &&
> > - !(I915_READ(BXT_PORT_REF_DW3(DPIO_PHY1)) & GRC_DONE)) {
> > - DRM_DEBUG_DRIVER("DDI PHY 1 powered, but GRC isn't
> > done\n");
> > + if (phy_info->rcomp_phy == -1 &&
> > + !(I915_READ(BXT_PORT_REF_DW3(phy)) & GRC_DONE)) {
> > + DRM_DEBUG_DRIVER("DDI PHY %d powered, but GRC isn't
> > done\n",
> > + phy);
> >
> > return false;
> > }
> > @@ -259,14 +271,15 @@ static void bxt_phy_wait_grc_done(struct
> > drm_i915_private *dev_priv,
> > DRM_ERROR("timeout waiting for PHY%d GRC\n", phy);
> > }
> >
> > -void bxt_ddi_phy_init(struct drm_i915_private *dev_priv, enum dpio_phy phy)
> > +static void _bxt_ddi_phy_init(struct drm_i915_private *dev_priv,
> > + enum dpio_phy phy)
> > {
> > const struct bxt_ddi_phy_info *phy_info = &bxt_ddi_phy_info[phy];
> > u32 val;
> >
> > if (bxt_ddi_phy_is_enabled(dev_priv, phy)) {
> > /* Still read out the GRC value for state verification */
> > - if (phy == DPIO_PHY0)
> > + if (phy_info->rcomp_phy != -1)
> > dev_priv->bxt_phy_grc = bxt_get_grc(dev_priv, phy);
> >
> > if (bxt_ddi_phy_verify_state(dev_priv, phy)) {
> > @@ -336,30 +349,32 @@ void bxt_ddi_phy_init(struct drm_i915_private
> > *dev_priv, enum dpio_phy phy)
> > val |= OCL2_LDOFUSE_PWR_DIS;
> > I915_WRITE(BXT_PORT_CL1CM_DW30(phy), val);
> >
> > - if (phy == DPIO_PHY0) {
> > + if (phy_info->rcomp_phy != -1) {
> > uint32_t grc_code;
> > /*
> > * PHY0 isn't connected to an RCOMP resistor so copy over
> > * the corresponding calibrated value from PHY1, and
> > disable
> > * the automatic calibration on PHY0.
> > */
> > - val = dev_priv->bxt_phy_grc = bxt_get_grc(dev_priv,
> > DPIO_PHY1);
> > + val = dev_priv->bxt_phy_grc = bxt_get_grc(dev_priv,
> > + phy_info-
> > >rcomp_phy);
> > grc_code = val << GRC_CODE_FAST_SHIFT |
> > val << GRC_CODE_SLOW_SHIFT |
> > val;
> > - I915_WRITE(BXT_PORT_REF_DW6(DPIO_PHY0), grc_code);
> > + I915_WRITE(BXT_PORT_REF_DW6(phy), grc_code);
> >
> > - val = I915_READ(BXT_PORT_REF_DW8(DPIO_PHY0));
> > + val = I915_READ(BXT_PORT_REF_DW8(phy));
> > val |= GRC_DIS | GRC_RDY_OVRD;
> > - I915_WRITE(BXT_PORT_REF_DW8(DPIO_PHY0), val);
> > + I915_WRITE(BXT_PORT_REF_DW8(phy), val);
> > }
> >
> > val = I915_READ(BXT_PHY_CTL_FAMILY(phy));
> > val |= COMMON_RESET_DIS;
> > I915_WRITE(BXT_PHY_CTL_FAMILY(phy), val);
> >
> > - if (phy == DPIO_PHY1)
> > - bxt_phy_wait_grc_done(dev_priv, DPIO_PHY1);
> > + if (phy_info->rcomp_phy == -1)
> > + bxt_phy_wait_grc_done(dev_priv, phy);
> > +
> > }
> >
> > void bxt_ddi_phy_uninit(struct drm_i915_private *dev_priv, enum dpio_phy
> > phy)
> > @@ -375,6 +390,33 @@ void bxt_ddi_phy_uninit(struct drm_i915_private
> > *dev_priv, enum dpio_phy phy)
> > I915_WRITE(BXT_P_CR_GT_DISP_PWRON, val);
> > }
> >
> > +/* This function assumes it is called from an intel_display_power_get()
> > call,
> > + * which would serialize any other attempts to enable the phy with the
> > Rcomp
> > + * resistor.
> > + */
> You could replace the above with a mutex locked assert.
I'll do that and resend.
> >
> > +void bxt_ddi_phy_init(struct drm_i915_private *dev_priv, enum dpio_phy phy)
> > +{
> > + const struct bxt_ddi_phy_info *phy_info = &bxt_ddi_phy_info[phy];
> > + enum dpio_phy rcomp_phy = phy_info->rcomp_phy;
> > + bool was_enabled;
> > +
> > + if (rcomp_phy != -1) {
> > + was_enabled = bxt_ddi_phy_is_enabled(dev_priv, rcomp_phy);
> > +
> > + /*
> > + * We need to copy the GRC calibration value from
> > rcomp_phy,
> > + * so make sure it's powered up.
> > + */
> > + if (!was_enabled)
> > + _bxt_ddi_phy_init(dev_priv, rcomp_phy);
> > + }
> > +
> > + _bxt_ddi_phy_init(dev_priv, phy);
> > +
> > + if (rcomp_phy != -1 && !was_enabled)
> > + bxt_ddi_phy_uninit(dev_priv, phy_info->rcomp_phy);
> > +}
> Nitpick: An alternative would be to add the power well dependency as a
> custom power well field for the cmn power wells. We'd avoid the
> unnecessary HW access in bxt_ddi_phy_is_enabled().
Since you gave me the option, I'll choose to ignore this one. What I like about
the current solution is that the dependency relationship between the phys is
captured in a single place, so I'd like to avoid duplicating that information in
the power well code.
I don't think the extra hardware access is performance critical, but if it were,
maybe we could track whether the phy is enabled in the phy code too?
Ander
>
> >
> > +
> > static bool __printf(6, 7)
> > __phy_reg_verify_state(struct drm_i915_private *dev_priv, enum dpio_phy
> > phy,
> > i915_reg_t reg, u32 mask, u32 expected,
> > @@ -441,7 +483,7 @@ bool bxt_ddi_phy_verify_state(struct drm_i915_private
> > *dev_priv,
> > * at least on stepping A this bit is read-only and fixed at 0.
> > */
> >
> > - if (phy == DPIO_PHY0) {
> > + if (phy_info->rcomp_phy != -1) {
> > u32 grc_code = dev_priv->bxt_phy_grc;
> >
> > grc_code = grc_code << GRC_CODE_FAST_SHIFT |
> > @@ -449,12 +491,12 @@ bool bxt_ddi_phy_verify_state(struct drm_i915_private
> > *dev_priv,
> > grc_code;
> > mask = GRC_CODE_FAST_MASK | GRC_CODE_SLOW_MASK |
> > GRC_CODE_NOM_MASK;
> > - ok &= _CHK(BXT_PORT_REF_DW6(DPIO_PHY0), mask, grc_code,
> > - "BXT_PORT_REF_DW6(%d)", DPIO_PHY0);
> > + ok &= _CHK(BXT_PORT_REF_DW6(phy), mask, grc_code,
> > + "BXT_PORT_REF_DW6(%d)", phy);
> >
> > mask = GRC_DIS | GRC_RDY_OVRD;
> > - ok &= _CHK(BXT_PORT_REF_DW8(DPIO_PHY0), mask, mask,
> > - "BXT_PORT_REF_DW8(%d)", DPIO_PHY0);
> > + ok &= _CHK(BXT_PORT_REF_DW8(phy), mask, mask,
> > + "BXT_PORT_REF_DW8(%d)", phy);
> > }
> >
> > return ok;
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index d41fd46..3b38998 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -849,22 +849,7 @@ static void skl_power_well_disable(struct
> > drm_i915_private *dev_priv,
> > static void bxt_dpio_cmn_power_well_enable(struct drm_i915_private
> > *dev_priv,
> > struct i915_power_well
> > *power_well)
> > {
> > - enum skl_disp_power_wells power_well_id = power_well->id;
> > - struct i915_power_well *cmn_a_well = NULL;
> > -
> > - if (power_well_id == BXT_DPIO_CMN_BC) {
> > - /*
> > - * We need to copy the GRC calibration value from the eDP
> > PHY,
> > - * so make sure it's powered up.
> > - */
> > - cmn_a_well = lookup_power_well(dev_priv, BXT_DPIO_CMN_A);
> > - intel_power_well_get(dev_priv, cmn_a_well);
> > - }
> > -
> > bxt_ddi_phy_init(dev_priv, power_well->data);
> > -
> > - if (cmn_a_well)
> > - intel_power_well_put(dev_priv, cmn_a_well);
> > }
> >
> > static void bxt_dpio_cmn_power_well_disable(struct drm_i915_private
> > *dev_priv,
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
