| Message ID | 1475669354-22622-9-git-send-email-ander.conselvan.de.oliveira@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. > +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(). > + > 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,
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,
On to, 2016-10-06 at 14:50 +0300, Ander Conselvan De Oliveira wrote: > 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_in > > > fo- > > > > 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? No, I meant that we already track that in the power well code so checking for it here again would be redundant. But I'm also fine with your solution so let's go with that. --Imre > > 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,
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. + */ +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); +} + 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,
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 <ander.conselvan.de.oliveira@intel.com> --- 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(-)