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,
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
