Message ID | 20180918181015.11001-1-jose.souza@intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [v3,1/6] drm/i915/runtime_pm: Share code to enable/disable PCH reset handshake | expand |
On Tue, Sep 18, 2018 at 11:10:10AM -0700, José Roberto de Souza wrote: > Instead of have the same code spread into 4 platforms lets share it. > BXT do not have a PCH so here also handling this case by unseting > RESET_PCH_HANDSHAKE_ENABLE. > > v2(Rodrigo): > - renamed to intel_pch_reset_handshake() > - added comment about why BXT need the bit to be unset > > v3(Rodrigo and Ville): > - added bool have_pch to intel_pch_reset_handshake() > - added back BXT comment > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > --- > drivers/gpu/drm/i915/intel_runtime_pm.c | 30 ++++++++++++++----------- > 1 file changed, 17 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c > index 9bebec389de1..4e050dc3ebde 100644 > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c > @@ -3239,18 +3239,29 @@ static void icl_mbus_init(struct drm_i915_private *dev_priv) > I915_WRITE(MBUS_ABOX_CTL, val); > } > > +static void intel_pch_reset_handshake(struct drm_i915_private *dev_priv, > + bool have_pch) You seem very intent on moving the HAS_PCH_NOP() into this function. I still don't like that approach. I think it's clearner to let the caller 100% decide whether to enable or disable the reset handshake. > +{ > + u32 val = I915_READ(HSW_NDE_RSTWRN_OPT); > + > + if (have_pch) > + val |= RESET_PCH_HANDSHAKE_ENABLE; > + else > + val &= ~RESET_PCH_HANDSHAKE_ENABLE; > + > + I915_WRITE(HSW_NDE_RSTWRN_OPT, val); > +} > + > static void skl_display_core_init(struct drm_i915_private *dev_priv, > bool resume) > { > struct i915_power_domains *power_domains = &dev_priv->power_domains; > struct i915_power_well *well; > - uint32_t val; > > gen9_set_dc_state(dev_priv, DC_STATE_DISABLE); > > /* enable PCH reset handshake */ > - val = I915_READ(HSW_NDE_RSTWRN_OPT); > - I915_WRITE(HSW_NDE_RSTWRN_OPT, val | RESET_PCH_HANDSHAKE_ENABLE); > + intel_pch_reset_handshake(dev_priv, true); > > /* enable PG1 and Misc I/O */ > mutex_lock(&power_domains->lock); > @@ -3306,7 +3317,6 @@ void bxt_display_core_init(struct drm_i915_private *dev_priv, > { > struct i915_power_domains *power_domains = &dev_priv->power_domains; > struct i915_power_well *well; > - uint32_t val; > > gen9_set_dc_state(dev_priv, DC_STATE_DISABLE); > > @@ -3316,9 +3326,7 @@ void bxt_display_core_init(struct drm_i915_private *dev_priv, > * Move the handshake programming to initialization sequence. > * Previously was left up to BIOS. > */ > - val = I915_READ(HSW_NDE_RSTWRN_OPT); > - val &= ~RESET_PCH_HANDSHAKE_ENABLE; > - I915_WRITE(HSW_NDE_RSTWRN_OPT, val); > + intel_pch_reset_handshake(dev_priv, false); > > /* Enable PG1 */ > mutex_lock(&power_domains->lock); > @@ -3439,9 +3447,7 @@ static void cnl_display_core_init(struct drm_i915_private *dev_priv, bool resume > gen9_set_dc_state(dev_priv, DC_STATE_DISABLE); > > /* 1. Enable PCH Reset Handshake */ > - val = I915_READ(HSW_NDE_RSTWRN_OPT); > - val |= RESET_PCH_HANDSHAKE_ENABLE; > - I915_WRITE(HSW_NDE_RSTWRN_OPT, val); > + intel_pch_reset_handshake(dev_priv, true); > > /* 2. Enable Comp */ > val = I915_READ(CHICKEN_MISC_2); > @@ -3524,9 +3530,7 @@ static void icl_display_core_init(struct drm_i915_private *dev_priv, > gen9_set_dc_state(dev_priv, DC_STATE_DISABLE); > > /* 1. Enable PCH reset handshake. */ > - val = I915_READ(HSW_NDE_RSTWRN_OPT); > - val |= RESET_PCH_HANDSHAKE_ENABLE; > - I915_WRITE(HSW_NDE_RSTWRN_OPT, val); > + intel_pch_reset_handshake(dev_priv, true); > > for (port = PORT_A; port <= PORT_B; port++) { > /* 2. Enable DDI combo PHY comp. */ > -- > 2.19.0
On Tue, 2018-09-18 at 22:04 +0300, Ville Syrjälä wrote: > On Tue, Sep 18, 2018 at 11:10:10AM -0700, José Roberto de Souza > wrote: > > Instead of have the same code spread into 4 platforms lets share > > it. > > BXT do not have a PCH so here also handling this case by unseting > > RESET_PCH_HANDSHAKE_ENABLE. > > > > v2(Rodrigo): > > - renamed to intel_pch_reset_handshake() > > - added comment about why BXT need the bit to be unset > > > > v3(Rodrigo and Ville): > > - added bool have_pch to intel_pch_reset_handshake() > > - added back BXT comment > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > --- > > drivers/gpu/drm/i915/intel_runtime_pm.c | 30 ++++++++++++++------- > > ---- > > 1 file changed, 17 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c > > b/drivers/gpu/drm/i915/intel_runtime_pm.c > > index 9bebec389de1..4e050dc3ebde 100644 > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c > > @@ -3239,18 +3239,29 @@ static void icl_mbus_init(struct > > drm_i915_private *dev_priv) > > I915_WRITE(MBUS_ABOX_CTL, val); > > } > > > > +static void intel_pch_reset_handshake(struct drm_i915_private > > *dev_priv, > > + bool have_pch) > > You seem very intent on moving the HAS_PCH_NOP() into this function. > I still don't like that approach. I think it's clearner to let the > caller 100% decide whether to enable or disable the reset handshake. Because it would just move HAS_PCH_NOP() to all the callers expect for BXT, also we are doing the same thing right now but in a wrong place. But if you and Rodrigo really wants it, I can change that. > > > +{ > > + u32 val = I915_READ(HSW_NDE_RSTWRN_OPT); > > + > > + if (have_pch) > > + val |= RESET_PCH_HANDSHAKE_ENABLE; > > + else > > + val &= ~RESET_PCH_HANDSHAKE_ENABLE; > > + > > + I915_WRITE(HSW_NDE_RSTWRN_OPT, val); > > +} > > + > > static void skl_display_core_init(struct drm_i915_private > > *dev_priv, > > bool resume) > > { > > struct i915_power_domains *power_domains = &dev_priv- > > >power_domains; > > struct i915_power_well *well; > > - uint32_t val; > > > > gen9_set_dc_state(dev_priv, DC_STATE_DISABLE); > > > > /* enable PCH reset handshake */ > > - val = I915_READ(HSW_NDE_RSTWRN_OPT); > > - I915_WRITE(HSW_NDE_RSTWRN_OPT, val | > > RESET_PCH_HANDSHAKE_ENABLE); > > + intel_pch_reset_handshake(dev_priv, true); > > > > /* enable PG1 and Misc I/O */ > > mutex_lock(&power_domains->lock); > > @@ -3306,7 +3317,6 @@ void bxt_display_core_init(struct > > drm_i915_private *dev_priv, > > { > > struct i915_power_domains *power_domains = &dev_priv- > > >power_domains; > > struct i915_power_well *well; > > - uint32_t val; > > > > gen9_set_dc_state(dev_priv, DC_STATE_DISABLE); > > > > @@ -3316,9 +3326,7 @@ void bxt_display_core_init(struct > > drm_i915_private *dev_priv, > > * Move the handshake programming to initialization sequence. > > * Previously was left up to BIOS. > > */ > > - val = I915_READ(HSW_NDE_RSTWRN_OPT); > > - val &= ~RESET_PCH_HANDSHAKE_ENABLE; > > - I915_WRITE(HSW_NDE_RSTWRN_OPT, val); > > + intel_pch_reset_handshake(dev_priv, false); > > > > /* Enable PG1 */ > > mutex_lock(&power_domains->lock); > > @@ -3439,9 +3447,7 @@ static void cnl_display_core_init(struct > > drm_i915_private *dev_priv, bool resume > > gen9_set_dc_state(dev_priv, DC_STATE_DISABLE); > > > > /* 1. Enable PCH Reset Handshake */ > > - val = I915_READ(HSW_NDE_RSTWRN_OPT); > > - val |= RESET_PCH_HANDSHAKE_ENABLE; > > - I915_WRITE(HSW_NDE_RSTWRN_OPT, val); > > + intel_pch_reset_handshake(dev_priv, true); > > > > /* 2. Enable Comp */ > > val = I915_READ(CHICKEN_MISC_2); > > @@ -3524,9 +3530,7 @@ static void icl_display_core_init(struct > > drm_i915_private *dev_priv, > > gen9_set_dc_state(dev_priv, DC_STATE_DISABLE); > > > > /* 1. Enable PCH reset handshake. */ > > - val = I915_READ(HSW_NDE_RSTWRN_OPT); > > - val |= RESET_PCH_HANDSHAKE_ENABLE; > > - I915_WRITE(HSW_NDE_RSTWRN_OPT, val); > > + intel_pch_reset_handshake(dev_priv, true); > > > > for (port = PORT_A; port <= PORT_B; port++) { > > /* 2. Enable DDI combo PHY comp. */ > > -- > > 2.19.0 > >
On Tue, Sep 18, 2018 at 12:40:21PM -0700, Souza, Jose wrote: > On Tue, 2018-09-18 at 22:04 +0300, Ville Syrjälä wrote: > > On Tue, Sep 18, 2018 at 11:10:10AM -0700, José Roberto de Souza > > wrote: > > > Instead of have the same code spread into 4 platforms lets share > > > it. > > > BXT do not have a PCH so here also handling this case by unseting > > > RESET_PCH_HANDSHAKE_ENABLE. > > > > > > v2(Rodrigo): > > > - renamed to intel_pch_reset_handshake() > > > - added comment about why BXT need the bit to be unset > > > > > > v3(Rodrigo and Ville): > > > - added bool have_pch to intel_pch_reset_handshake() > > > - added back BXT comment > > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_runtime_pm.c | 30 ++++++++++++++------- > > > ---- > > > 1 file changed, 17 insertions(+), 13 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c > > > b/drivers/gpu/drm/i915/intel_runtime_pm.c > > > index 9bebec389de1..4e050dc3ebde 100644 > > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c > > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c > > > @@ -3239,18 +3239,29 @@ static void icl_mbus_init(struct > > > drm_i915_private *dev_priv) > > > I915_WRITE(MBUS_ABOX_CTL, val); > > > } > > > > > > +static void intel_pch_reset_handshake(struct drm_i915_private > > > *dev_priv, > > > + bool have_pch) > > > > You seem very intent on moving the HAS_PCH_NOP() into this function. > > I still don't like that approach. I think it's clearner to let the > > caller 100% decide whether to enable or disable the reset handshake. > > Because it would just move HAS_PCH_NOP() to all the callers expect for > BXT, also we are doing the same thing right now but in a wrong place. > But if you and Rodrigo really wants it, I can change that. :) Thanks for changing to the bool approach already. Checking the other patch I agree with ville that leaving the enable/disable decision outside is better. > > > > > > > +{ > > > + u32 val = I915_READ(HSW_NDE_RSTWRN_OPT); > > > + > > > + if (have_pch) > > > + val |= RESET_PCH_HANDSHAKE_ENABLE; > > > + else > > > + val &= ~RESET_PCH_HANDSHAKE_ENABLE; > > > + > > > + I915_WRITE(HSW_NDE_RSTWRN_OPT, val); > > > +} > > > + > > > static void skl_display_core_init(struct drm_i915_private > > > *dev_priv, > > > bool resume) > > > { > > > struct i915_power_domains *power_domains = &dev_priv- > > > >power_domains; > > > struct i915_power_well *well; > > > - uint32_t val; > > > > > > gen9_set_dc_state(dev_priv, DC_STATE_DISABLE); > > > > > > /* enable PCH reset handshake */ > > > - val = I915_READ(HSW_NDE_RSTWRN_OPT); > > > - I915_WRITE(HSW_NDE_RSTWRN_OPT, val | > > > RESET_PCH_HANDSHAKE_ENABLE); > > > + intel_pch_reset_handshake(dev_priv, true); > > > > > > /* enable PG1 and Misc I/O */ > > > mutex_lock(&power_domains->lock); > > > @@ -3306,7 +3317,6 @@ void bxt_display_core_init(struct > > > drm_i915_private *dev_priv, > > > { > > > struct i915_power_domains *power_domains = &dev_priv- > > > >power_domains; > > > struct i915_power_well *well; > > > - uint32_t val; > > > > > > gen9_set_dc_state(dev_priv, DC_STATE_DISABLE); > > > > > > @@ -3316,9 +3326,7 @@ void bxt_display_core_init(struct > > > drm_i915_private *dev_priv, > > > * Move the handshake programming to initialization sequence. > > > * Previously was left up to BIOS. > > > */ > > > - val = I915_READ(HSW_NDE_RSTWRN_OPT); > > > - val &= ~RESET_PCH_HANDSHAKE_ENABLE; > > > - I915_WRITE(HSW_NDE_RSTWRN_OPT, val); > > > + intel_pch_reset_handshake(dev_priv, false); > > > > > > /* Enable PG1 */ > > > mutex_lock(&power_domains->lock); > > > @@ -3439,9 +3447,7 @@ static void cnl_display_core_init(struct > > > drm_i915_private *dev_priv, bool resume > > > gen9_set_dc_state(dev_priv, DC_STATE_DISABLE); > > > > > > /* 1. Enable PCH Reset Handshake */ > > > - val = I915_READ(HSW_NDE_RSTWRN_OPT); > > > - val |= RESET_PCH_HANDSHAKE_ENABLE; > > > - I915_WRITE(HSW_NDE_RSTWRN_OPT, val); > > > + intel_pch_reset_handshake(dev_priv, true); > > > > > > /* 2. Enable Comp */ > > > val = I915_READ(CHICKEN_MISC_2); > > > @@ -3524,9 +3530,7 @@ static void icl_display_core_init(struct > > > drm_i915_private *dev_priv, > > > gen9_set_dc_state(dev_priv, DC_STATE_DISABLE); > > > > > > /* 1. Enable PCH reset handshake. */ > > > - val = I915_READ(HSW_NDE_RSTWRN_OPT); > > > - val |= RESET_PCH_HANDSHAKE_ENABLE; > > > - I915_WRITE(HSW_NDE_RSTWRN_OPT, val); > > > + intel_pch_reset_handshake(dev_priv, true); > > > > > > for (port = PORT_A; port <= PORT_B; port++) { > > > /* 2. Enable DDI combo PHY comp. */ > > > -- > > > 2.19.0 > > > >
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c index 9bebec389de1..4e050dc3ebde 100644 --- a/drivers/gpu/drm/i915/intel_runtime_pm.c +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c @@ -3239,18 +3239,29 @@ static void icl_mbus_init(struct drm_i915_private *dev_priv) I915_WRITE(MBUS_ABOX_CTL, val); } +static void intel_pch_reset_handshake(struct drm_i915_private *dev_priv, + bool have_pch) +{ + u32 val = I915_READ(HSW_NDE_RSTWRN_OPT); + + if (have_pch) + val |= RESET_PCH_HANDSHAKE_ENABLE; + else + val &= ~RESET_PCH_HANDSHAKE_ENABLE; + + I915_WRITE(HSW_NDE_RSTWRN_OPT, val); +} + static void skl_display_core_init(struct drm_i915_private *dev_priv, bool resume) { struct i915_power_domains *power_domains = &dev_priv->power_domains; struct i915_power_well *well; - uint32_t val; gen9_set_dc_state(dev_priv, DC_STATE_DISABLE); /* enable PCH reset handshake */ - val = I915_READ(HSW_NDE_RSTWRN_OPT); - I915_WRITE(HSW_NDE_RSTWRN_OPT, val | RESET_PCH_HANDSHAKE_ENABLE); + intel_pch_reset_handshake(dev_priv, true); /* enable PG1 and Misc I/O */ mutex_lock(&power_domains->lock); @@ -3306,7 +3317,6 @@ void bxt_display_core_init(struct drm_i915_private *dev_priv, { struct i915_power_domains *power_domains = &dev_priv->power_domains; struct i915_power_well *well; - uint32_t val; gen9_set_dc_state(dev_priv, DC_STATE_DISABLE); @@ -3316,9 +3326,7 @@ void bxt_display_core_init(struct drm_i915_private *dev_priv, * Move the handshake programming to initialization sequence. * Previously was left up to BIOS. */ - val = I915_READ(HSW_NDE_RSTWRN_OPT); - val &= ~RESET_PCH_HANDSHAKE_ENABLE; - I915_WRITE(HSW_NDE_RSTWRN_OPT, val); + intel_pch_reset_handshake(dev_priv, false); /* Enable PG1 */ mutex_lock(&power_domains->lock); @@ -3439,9 +3447,7 @@ static void cnl_display_core_init(struct drm_i915_private *dev_priv, bool resume gen9_set_dc_state(dev_priv, DC_STATE_DISABLE); /* 1. Enable PCH Reset Handshake */ - val = I915_READ(HSW_NDE_RSTWRN_OPT); - val |= RESET_PCH_HANDSHAKE_ENABLE; - I915_WRITE(HSW_NDE_RSTWRN_OPT, val); + intel_pch_reset_handshake(dev_priv, true); /* 2. Enable Comp */ val = I915_READ(CHICKEN_MISC_2); @@ -3524,9 +3530,7 @@ static void icl_display_core_init(struct drm_i915_private *dev_priv, gen9_set_dc_state(dev_priv, DC_STATE_DISABLE); /* 1. Enable PCH reset handshake. */ - val = I915_READ(HSW_NDE_RSTWRN_OPT); - val |= RESET_PCH_HANDSHAKE_ENABLE; - I915_WRITE(HSW_NDE_RSTWRN_OPT, val); + intel_pch_reset_handshake(dev_priv, true); for (port = PORT_A; port <= PORT_B; port++) { /* 2. Enable DDI combo PHY comp. */
Instead of have the same code spread into 4 platforms lets share it. BXT do not have a PCH so here also handling this case by unseting RESET_PCH_HANDSHAKE_ENABLE. v2(Rodrigo): - renamed to intel_pch_reset_handshake() - added comment about why BXT need the bit to be unset v3(Rodrigo and Ville): - added bool have_pch to intel_pch_reset_handshake() - added back BXT comment Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Signed-off-by: José Roberto de Souza <jose.souza@intel.com> --- drivers/gpu/drm/i915/intel_runtime_pm.c | 30 ++++++++++++++----------- 1 file changed, 17 insertions(+), 13 deletions(-)