On Tue, Feb 02, 2021 at 08:31:48AM +0200, Ville Syrjälä wrote:
> On Tue, Feb 02, 2021 at 05:52:28AM +0000, Surendrakumar Upadhyay, TejaskumarX wrote:
> >
> >
> > > -----Original Message-----
> > > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > Sent: 28 January 2021 04:46
> > > To: Surendrakumar Upadhyay, TejaskumarX
> > > <tejaskumarx.surendrakumar.upadhyay@xxxxxxxxx>
> > > Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Pandey, Hariom
> > > <hariom.pandey@xxxxxxxxx>; Roper, Matthew D
> > > <matthew.d.roper@xxxxxxxxx>
> > > Subject: Re: [PATCH] drm/i915/gen9bc: Handle TGP PCH during
> > > suspend/resume
> > >
> > > On Wed, Jan 27, 2021 at 03:38:30PM +0530, Tejas Upadhyay wrote:
> > > > For Legacy S3 suspend/resume GEN9 BC needs to enable and setup TGP
> > > > PCH.
> > > >
> > > > Cc: Matt Roper <matthew.d.roper@xxxxxxxxx>
> > > > Signed-off-by: Tejas Upadhyay
> > > > <tejaskumarx.surendrakumar.upadhyay@xxxxxxxxx>
> > > > ---
> > > > drivers/gpu/drm/i915/i915_irq.c | 36
> > > > ++++++++++++++++++++++++---------
> > > > 1 file changed, 27 insertions(+), 9 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/i915_irq.c
> > > > b/drivers/gpu/drm/i915/i915_irq.c index a31980f69120..6dcefc3e24ac
> > > > 100644
> > > > --- a/drivers/gpu/drm/i915/i915_irq.c
> > > > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > > > @@ -3026,8 +3026,20 @@ static void gen8_irq_reset(struct
> > > drm_i915_private *dev_priv)
> > > > GEN3_IRQ_RESET(uncore, GEN8_DE_MISC_);
> > > > GEN3_IRQ_RESET(uncore, GEN8_PCU_);
> > > >
> > > > - if (HAS_PCH_SPLIT(dev_priv))
> > > > + if (INTEL_PCH_TYPE(dev_priv) >= PCH_ICP)
> > > > + GEN3_IRQ_RESET(uncore, SDE);
> > > > + else if (HAS_PCH_SPLIT(dev_priv))
> > > > ibx_irq_reset(dev_priv);
> > > > +
> > > > + /* Wa_14010685332:cnp/cmp,tgp,adp */
> > > > + if (INTEL_PCH_TYPE(dev_priv) == PCH_CNP ||
> > > > + (INTEL_PCH_TYPE(dev_priv) >= PCH_TGP &&
> > > > + INTEL_PCH_TYPE(dev_priv) < PCH_DG1)) {
> > > > + intel_uncore_rmw(uncore, SOUTH_CHICKEN1,
> > > > + SBCLK_RUN_REFCLK_DIS,
> > > SBCLK_RUN_REFCLK_DIS);
> > > > + intel_uncore_rmw(uncore, SOUTH_CHICKEN1,
> > > > + SBCLK_RUN_REFCLK_DIS, 0);
> > > > + }
> > >
> > > Time to refactor instead of copypasta.
> > Do you expect below? :
> >
> > If ((INTEL_PCH_TYPE(dev_priv) == PCH_TGP) {
> > intel_uncore_rmw(uncore, SOUTH_CHICKEN1,
> > SBCLK_RUN_REFCLK_DIS,
> > SBCLK_RUN_REFCLK_DIS);
> > intel_uncore_rmw(uncore, SOUTH_CHICKEN1,
> > SBCLK_RUN_REFCLK_DIS, 0);
> > }
>
> I expect a new function instead of copy pasting this whole thing
> into multiple places.
>
> That said even the current code doesn't make any sense to me.
> Take for instance this part:
> if (INTEL_PCH_TYPE(dev_priv) >= PCH_ICP)
> GEN3_IRQ_RESET(uncore, SDE);
> What is that PCH type check doing there? What weird PCH
> type are we supposed to have that doesn't need this?
>
> Also the Wa_14010685332 part looks a bit odd. Is it
> correct that icp doesn't need that, but cnp and tgp
> both do somehow? Can we even have cnp on icl+?
Hmm. Looking at it a bit more, that w/a seems to have something to do
with suspend/resume, so seems rather misplaced in irq_reset(). Should
probably just move the whole thing into a more appropriate place.
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
