[v2] ALSA: hda - restore BCLK M/N values when resuming HSW/BDW display controller

Commit Message

Lin, Mengdong June 26, 2014, 10:45 a.m. UTC

From: Mengdong Lin <[e-mail]>

For Intel Haswell/Broadwell display HD-A controller, the 24MHz HD-A link BCLK
is converted from Core Display Clock (CDCLK): BCLK = CDCLK * M / N
And there are two registers EM4 and EM5 to program M, N value respectively.
The EM4/EM5 values will be lost and when the display power well is disabled.

BIOS programs CDCLK selected by OEM and EM4/EM5, but BIOS has no idea about
display power well on/off at runtime. So the M/N can be wrong if non-default
CDCLK is used when the audio controller resumes, which results in an invalid
BCLK and abnormal audio playback rate. So this patch saves and restores valid
M/N values on controller suspend/resume.

And 'struct hda_intel' is defined to contain standard HD-A 'struct azx' and
Intel specific fields, as Takashi suggested.

Signed-off-by: Mengdong Lin <[e-mail]>

Comments

Takashi Iwai June 26, 2014, 10:55 a.m. UTC | #1

At Thu, 26 Jun 2014 18:45:16 +0800,
[e-mail] wrote:
> 
> From: Mengdong Lin <[e-mail]>
> 
> For Intel Haswell/Broadwell display HD-A controller, the 24MHz HD-A link BCLK
> is converted from Core Display Clock (CDCLK): BCLK = CDCLK * M / N
> And there are two registers EM4 and EM5 to program M, N value respectively.
> The EM4/EM5 values will be lost and when the display power well is disabled.
> 
> BIOS programs CDCLK selected by OEM and EM4/EM5, but BIOS has no idea about
> display power well on/off at runtime. So the M/N can be wrong if non-default
> CDCLK is used when the audio controller resumes, which results in an invalid
> BCLK and abnormal audio playback rate. So this patch saves and restores valid
> M/N values on controller suspend/resume.
> 
> And 'struct hda_intel' is defined to contain standard HD-A 'struct azx' and
> Intel specific fields, as Takashi suggested.
> 
> Signed-off-by: Mengdong Lin <[e-mail]>
> 
> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> index bb65a124..ff9cacd 100644
> --- a/sound/pci/hda/hda_intel.c
> +++ b/sound/pci/hda/hda_intel.c
> @@ -288,6 +288,24 @@ static char *driver_short_names[] = {
>  	[AZX_DRIVER_GENERIC] = "HD-Audio Generic",
>  };
>  
> +
> +/* Intel HSW/BDW display HDA controller Extended Mode registers.
> + * EM4 (M value) and EM5 (N Value) are used to convert CDClk (Core Display
> + * Clock) to 24MHz BCLK: BCLK = CDCLK * M / N
> + * The values will be lost when the display power well is disabled.
> + */
> +#define ICH6_REG_EM4			0x100c
> +#define ICH6_REG_EM5			0x1010

Avoid ICH6 prefix.  It's just confusing as these are only for HSW/BDW.

Other than that, looks good to me.


thanks,

Takashi

Lin, Mengdong June 26, 2014, 11:16 a.m. UTC | #2

> -----Original Message-----
> From: Takashi Iwai [mailto:[e-mail]]
> Sent: Thursday, June 26, 2014 6:55 PM
> To: Lin, Mengdong
> Cc: [e-mail]
> Subject: Re: [PATCH v2] ALSA: hda - restore BCLK M/N values when
> resuming HSW/BDW display controller
> 
> At Thu, 26 Jun 2014 18:45:16 +0800,
> [e-mail] wrote:
> >
> > From: Mengdong Lin <[e-mail]>
> >
> > For Intel Haswell/Broadwell display HD-A controller, the 24MHz HD-A
> > link BCLK is converted from Core Display Clock (CDCLK): BCLK = CDCLK *
> > M / N And there are two registers EM4 and EM5 to program M, N value
> respectively.
> > The EM4/EM5 values will be lost and when the display power well is
> disabled.
> >
> > BIOS programs CDCLK selected by OEM and EM4/EM5, but BIOS has no
> idea
> > about display power well on/off at runtime. So the M/N can be wrong if
> > non-default CDCLK is used when the audio controller resumes, which
> > results in an invalid BCLK and abnormal audio playback rate. So this
> > patch saves and restores valid M/N values on controller
> suspend/resume.
> >
> > And 'struct hda_intel' is defined to contain standard HD-A 'struct
> > azx' and Intel specific fields, as Takashi suggested.
> >
> > Signed-off-by: Mengdong Lin <[e-mail]>
> >
> > diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> > index bb65a124..ff9cacd 100644
> > --- a/sound/pci/hda/hda_intel.c
> > +++ b/sound/pci/hda/hda_intel.c
> > @@ -288,6 +288,24 @@ static char *driver_short_names[] = {
> >  	[AZX_DRIVER_GENERIC] = "HD-Audio Generic",  };
> >
> > +
> > +/* Intel HSW/BDW display HDA controller Extended Mode registers.
> > + * EM4 (M value) and EM5 (N Value) are used to convert CDClk (Core
> > +Display
> > + * Clock) to 24MHz BCLK: BCLK = CDCLK * M / N
> > + * The values will be lost when the display power well is disabled.
> > + */
> > +#define ICH6_REG_EM4			0x100c
> > +#define ICH6_REG_EM5			0x1010
> 
> Avoid ICH6 prefix.  It's just confusing as these are only for HSW/BDW.
> 
> Other than that, looks good to me.
>

So how about adding these?

+#define HDMI_REG_EM4			0x100c
+#define HDMI_REG_EM5			0x1010

#define azx_hdmi_readw(chip, reg) \
	((chip)->ops->reg_readw((chip)->remap_addr + HDMI_REG_##reg))

#define azx_hdmi_writew(chip, reg, value) \
	((chip)->ops->reg_writew(value, (chip)->remap_addr + HDMI_REG_##reg))

static void haswell_save_bclk(struct azx *chip)
{
	struct hda_intel *hda = container_of(chip, struct hda_intel, chip);

	hda->bclk_m = azx_hdmi_readw(chip, EM4);
	hda->bclk_n = azx_hdmi_readw(chip, EM5);
}

Or use "intel" instead of "hdmi", indicating it's Intel specific.

Thanks
Mengdong

Takashi Iwai June 26, 2014, 12:01 p.m. UTC | #3

At Thu, 26 Jun 2014 11:16:46 +0000,
Lin, Mengdong wrote:
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:[e-mail]]
> > Sent: Thursday, June 26, 2014 6:55 PM
> > To: Lin, Mengdong
> > Cc: [e-mail]
> > Subject: Re: [PATCH v2] ALSA: hda - restore BCLK M/N values when
> > resuming HSW/BDW display controller
> > 
> > At Thu, 26 Jun 2014 18:45:16 +0800,
> > [e-mail] wrote:
> > >
> > > From: Mengdong Lin <[e-mail]>
> > >
> > > For Intel Haswell/Broadwell display HD-A controller, the 24MHz HD-A
> > > link BCLK is converted from Core Display Clock (CDCLK): BCLK = CDCLK *
> > > M / N And there are two registers EM4 and EM5 to program M, N value
> > respectively.
> > > The EM4/EM5 values will be lost and when the display power well is
> > disabled.
> > >
> > > BIOS programs CDCLK selected by OEM and EM4/EM5, but BIOS has no
> > idea
> > > about display power well on/off at runtime. So the M/N can be wrong if
> > > non-default CDCLK is used when the audio controller resumes, which
> > > results in an invalid BCLK and abnormal audio playback rate. So this
> > > patch saves and restores valid M/N values on controller
> > suspend/resume.
> > >
> > > And 'struct hda_intel' is defined to contain standard HD-A 'struct
> > > azx' and Intel specific fields, as Takashi suggested.
> > >
> > > Signed-off-by: Mengdong Lin <[e-mail]>
> > >
> > > diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> > > index bb65a124..ff9cacd 100644
> > > --- a/sound/pci/hda/hda_intel.c
> > > +++ b/sound/pci/hda/hda_intel.c
> > > @@ -288,6 +288,24 @@ static char *driver_short_names[] = {
> > >  	[AZX_DRIVER_GENERIC] = "HD-Audio Generic",  };
> > >
> > > +
> > > +/* Intel HSW/BDW display HDA controller Extended Mode registers.
> > > + * EM4 (M value) and EM5 (N Value) are used to convert CDClk (Core
> > > +Display
> > > + * Clock) to 24MHz BCLK: BCLK = CDCLK * M / N
> > > + * The values will be lost when the display power well is disabled.
> > > + */
> > > +#define ICH6_REG_EM4			0x100c
> > > +#define ICH6_REG_EM5			0x1010
> > 
> > Avoid ICH6 prefix.  It's just confusing as these are only for HSW/BDW.
> > 
> > Other than that, looks good to me.
> >
> 
> So how about adding these?
> 
> +#define HDMI_REG_EM4			0x100c
> +#define HDMI_REG_EM5			0x1010
> 
> #define azx_hdmi_readw(chip, reg) \
> 	((chip)->ops->reg_readw((chip)->remap_addr + HDMI_REG_##reg))
> 
> #define azx_hdmi_writew(chip, reg, value) \
> 	((chip)->ops->reg_writew(value, (chip)->remap_addr + HDMI_REG_##reg))
> 
> static void haswell_save_bclk(struct azx *chip)
> {
> 	struct hda_intel *hda = container_of(chip, struct hda_intel, chip);
> 
> 	hda->bclk_m = azx_hdmi_readw(chip, EM4);
> 	hda->bclk_n = azx_hdmi_readw(chip, EM5);
> }
> 
> Or use "intel" instead of "hdmi", indicating it's Intel specific.

I'd just open-code a few read and write lines.


Takashi

Takashi Iwai June 26, 2014, 1:47 p.m. UTC | #4

At Thu, 26 Jun 2014 14:01:10 +0200,
Takashi Iwai wrote:
> 
> At Thu, 26 Jun 2014 11:16:46 +0000,
> Lin, Mengdong wrote:
> > 
> > > -----Original Message-----
> > > From: Takashi Iwai [mailto:[e-mail]]
> > > Sent: Thursday, June 26, 2014 6:55 PM
> > > To: Lin, Mengdong
> > > Cc: [e-mail]
> > > Subject: Re: [PATCH v2] ALSA: hda - restore BCLK M/N values when
> > > resuming HSW/BDW display controller
> > > 
> > > At Thu, 26 Jun 2014 18:45:16 +0800,
> > > [e-mail] wrote:
> > > >
> > > > From: Mengdong Lin <[e-mail]>
> > > >
> > > > For Intel Haswell/Broadwell display HD-A controller, the 24MHz HD-A
> > > > link BCLK is converted from Core Display Clock (CDCLK): BCLK = CDCLK *
> > > > M / N And there are two registers EM4 and EM5 to program M, N value
> > > respectively.
> > > > The EM4/EM5 values will be lost and when the display power well is
> > > disabled.
> > > >
> > > > BIOS programs CDCLK selected by OEM and EM4/EM5, but BIOS has no
> > > idea
> > > > about display power well on/off at runtime. So the M/N can be wrong if
> > > > non-default CDCLK is used when the audio controller resumes, which
> > > > results in an invalid BCLK and abnormal audio playback rate. So this
> > > > patch saves and restores valid M/N values on controller
> > > suspend/resume.
> > > >
> > > > And 'struct hda_intel' is defined to contain standard HD-A 'struct
> > > > azx' and Intel specific fields, as Takashi suggested.
> > > >
> > > > Signed-off-by: Mengdong Lin <[e-mail]>
> > > >
> > > > diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> > > > index bb65a124..ff9cacd 100644
> > > > --- a/sound/pci/hda/hda_intel.c
> > > > +++ b/sound/pci/hda/hda_intel.c
> > > > @@ -288,6 +288,24 @@ static char *driver_short_names[] = {
> > > >  	[AZX_DRIVER_GENERIC] = "HD-Audio Generic",  };
> > > >
> > > > +
> > > > +/* Intel HSW/BDW display HDA controller Extended Mode registers.
> > > > + * EM4 (M value) and EM5 (N Value) are used to convert CDClk (Core
> > > > +Display
> > > > + * Clock) to 24MHz BCLK: BCLK = CDCLK * M / N
> > > > + * The values will be lost when the display power well is disabled.
> > > > + */
> > > > +#define ICH6_REG_EM4			0x100c
> > > > +#define ICH6_REG_EM5			0x1010
> > > 
> > > Avoid ICH6 prefix.  It's just confusing as these are only for HSW/BDW.
> > > 
> > > Other than that, looks good to me.
> > >
> > 
> > So how about adding these?
> > 
> > +#define HDMI_REG_EM4			0x100c
> > +#define HDMI_REG_EM5			0x1010
> > 
> > #define azx_hdmi_readw(chip, reg) \
> > 	((chip)->ops->reg_readw((chip)->remap_addr + HDMI_REG_##reg))
> > 
> > #define azx_hdmi_writew(chip, reg, value) \
> > 	((chip)->ops->reg_writew(value, (chip)->remap_addr + HDMI_REG_##reg))
> > 
> > static void haswell_save_bclk(struct azx *chip)
> > {
> > 	struct hda_intel *hda = container_of(chip, struct hda_intel, chip);
> > 
> > 	hda->bclk_m = azx_hdmi_readw(chip, EM4);
> > 	hda->bclk_n = azx_hdmi_readw(chip, EM5);
> > }
> > 
> > Or use "intel" instead of "hdmi", indicating it's Intel specific.
> 
> I'd just open-code a few read and write lines.

Thinking of this again, maybe it's better to keep the patch smaller at
this stage, just concentrating on the fix.

We can get rid of ICH6 prefix later with a series of cleanup patches.

I'm going to take your rev2 patch then.


thanks,

Takashi

Lin, Mengdong June 27, 2014, 6:38 a.m. UTC | #5

Hi Takashi,

Sorry. This patch cannot handle the following case:
If only embedded DP is connected on boot, the gfx driver will turn off the display power well on boot case before the audio driver probes and request this power.
So the M/V values programmed by BIOS in EM4/5 registers will be lost at the very beginning. And so the audio driver will save invalid M/N values on suspend and restore these invalid values on resume. 
One phenomenon is: if HDMI or DP monitor is connected after boot, audio playback rate is ~20% faster than normal.
And I guess it's the patch cannot fix the Haswell issue https://bugzilla.kernel.org/show_bug.cgi?id=74861

There are two possible solutions:
(1) Follows Windows model: Windows, gfx driver notifies BIOS on LPSP (when only eDP is connected) exit and BIOS reprograms those EM4/5 registers.
   There is no such implementation in Linux gfx driver now. We need time to check the effort.

(2) Gfx driver notify audio driver on exiting LPSP and CDCLK value (by reading GPU registers), so audio driver can reprograms EM4/5 registers.
  The open source interface between audio/gfx driver is still not ready. Would an private API to get CDCLK from gfx driver will be a possible workaround atm?

Could you share your opinion on this issue?

Thanks
Mengdong

> -----Original Message-----
> From: Takashi Iwai [mailto:[e-mail]]
> Sent: Thursday, June 26, 2014 9:47 PM

> > I'd just open-code a few read and write lines.
> 
> Thinking of this again, maybe it's better to keep the patch smaller at this
> stage, just concentrating on the fix.
> 
> We can get rid of ICH6 prefix later with a series of cleanup patches.
> 
> I'm going to take your rev2 patch then.
> 
> 
> thanks,
> 
> Takashi

Takashi Iwai June 27, 2014, 10:27 a.m. UTC | #6

At Fri, 27 Jun 2014 06:38:47 +0000,
Lin, Mengdong wrote:
> 
> Hi Takashi,
> 
> Sorry. This patch cannot handle the following case:
> If only embedded DP is connected on boot, the gfx driver will turn off the display power well on boot case before the audio driver probes and request this power.
> So the M/V values programmed by BIOS in EM4/5 registers will be lost at the very beginning. And so the audio driver will save invalid M/N values on suspend and restore these invalid values on resume. 
> One phenomenon is: if HDMI or DP monitor is connected after boot, audio playback rate is ~20% faster than normal.
> And I guess it's the patch cannot fix the Haswell issue https://bugzilla.kernel.org/show_bug.cgi?id=74861
> 
> There are two possible solutions:
> (1) Follows Windows model: Windows, gfx driver notifies BIOS on LPSP (when only eDP is connected) exit and BIOS reprograms those EM4/5 registers.
>    There is no such implementation in Linux gfx driver now. We need time to check the effort.
> 
> (2) Gfx driver notify audio driver on exiting LPSP and CDCLK value (by reading GPU registers), so audio driver can reprograms EM4/5 registers.
>   The open source interface between audio/gfx driver is still not ready. Would an private API to get CDCLK from gfx driver will be a possible workaround atm?
> 
> Could you share your opinion on this issue?

To me, the former sounds like an easier solution.  In the latter case,
it can be still an issue of module loading order.

In anyway, I keep the former patch as is, since it fixes actually some
cases although it's not perfect.  Please work on top of it for further
fixes.


thanks,

Takashi

Lin, Mengdong July 2, 2014, 10:57 a.m. UTC | #7

Add Jani from Gfx driver team and audio team.

Hi Takashi,

> -----Original Message-----
> From: Takashi Iwai [mailto:[e-mail]]
> Sent: Friday, June 27, 2014 6:28 PM
> 
> At Fri, 27 Jun 2014 06:38:47 +0000,
> Lin, Mengdong wrote:
> >
> > Hi Takashi,
> >
> > Sorry. This patch cannot handle the following case:
> > If only embedded DP is connected on boot, the gfx driver will turn off
> the display power well on boot case before the audio driver probes and
> request this power.
> > So the M/V values programmed by BIOS in EM4/5 registers will be lost at
> the very beginning. And so the audio driver will save invalid M/N values
> on suspend and restore these invalid values on resume.
> > One phenomenon is: if HDMI or DP monitor is connected after boot,
> audio playback rate is ~20% faster than normal.
> > And I guess it's the patch cannot fix the Haswell issue
> > https://bugzilla.kernel.org/show_bug.cgi?id=74861
> >
> > There are two possible solutions:
> > (1) Follows Windows model: Windows, gfx driver notifies BIOS on LPSP
> (when only eDP is connected) exit and BIOS reprograms those EM4/5
> registers.
> >    There is no such implementation in Linux gfx driver now. We need
> time to check the effort.
> >
> > (2) Gfx driver notify audio driver on exiting LPSP and CDCLK value (by
> reading GPU registers), so audio driver can reprograms EM4/5 registers.
> >   The open source interface between audio/gfx driver is still not ready.
> Would an private API to get CDCLK from gfx driver will be a possible
> workaround atm?
> >
> > Could you share your opinion on this issue?
> 
> To me, the former sounds like an easier solution.  In the latter case, it can
> be still an issue of module loading order.

Can we introduce a private i915 interface for the audio driver to get the CDCLK as a quick fix?  Then the audio driver can reprogram the EM4/5 M/N values as per the CDCLK.

The interface can be like 'int i915_get_cdclk_freq(void)' and the audio driver will use symbol_request() to get this private API and avoid the issue of module loading order. 
We can put the relevant code in sound/pci/hda/ hda_i915.c

For Broadwell Jani has implemented a notification to BIOS, so BIOS can reprogram the EM4/5 value after restoring the power. 
http://patchwork.freedesktop.org/patch/28866/
http://patchwork.freedesktop.org/patch/28868/

But for Haswell, BIOS does not support the above notification and provide another heavy-weight notification which will enable/disable the HD-A controller at runtime and so need OS to re-enumerate the device stack. That causes system hang. And test on Haswell shows that reprogramming the EM4/5 as per the CDCLK is also enough to give good BCLK and audio rate. So we hope to avoid further dependency on the black-box BIOS.

Libin is working on the generic interface between audio and gfx now and we can clean up code in the future.
But before the interface is ready, a private API to query CDCLK seems to be a quick and reliable fix for Haswell and Broadwell.

Thanks
Mengdong

Lin, Mengdong July 2, 2014, 2:50 p.m. UTC | #8

Hi Takashi,

I submitted patches, using the private API to query CDCLK from i915 and restore BCLK. Could you have a review?
http://mailman.alsa-project.org/pipermail/alsa-devel/2014-July/078489.html

The 1st patch by Jani is actually for i915 to provide the interface for audio driver to query CDCLK. 

Thanks
Mengdong

> -----Original Message-----
> From: [e-mail]
> [mailto:[e-mail]] On Behalf Of Lin, Mengdong
> Sent: Wednesday, July 02, 2014 6:58 PM
> To: Takashi Iwai
> Cc: Nikula, Jani; Yang, Libin; [e-mail]; Li, Jocelyn
> Subject: Re: [alsa-devel] [PATCH v2] ALSA: hda - restore BCLK M/N values
> when resuming HSW/BDW display controller
> 
> Add Jani from Gfx driver team and audio team.
> 
> Hi Takashi,
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:[e-mail]]
> > Sent: Friday, June 27, 2014 6:28 PM
> >
> > At Fri, 27 Jun 2014 06:38:47 +0000,
> > Lin, Mengdong wrote:
> > >
> > > Hi Takashi,
> > >
> > > Sorry. This patch cannot handle the following case:
> > > If only embedded DP is connected on boot, the gfx driver will turn
> > > off
> > the display power well on boot case before the audio driver probes and
> > request this power.
> > > So the M/V values programmed by BIOS in EM4/5 registers will be lost
> > > at
> > the very beginning. And so the audio driver will save invalid M/N
> > values on suspend and restore these invalid values on resume.
> > > One phenomenon is: if HDMI or DP monitor is connected after boot,
> > audio playback rate is ~20% faster than normal.
> > > And I guess it's the patch cannot fix the Haswell issue
> > > https://bugzilla.kernel.org/show_bug.cgi?id=74861
> > >
> > > There are two possible solutions:
> > > (1) Follows Windows model: Windows, gfx driver notifies BIOS on LPSP
> > (when only eDP is connected) exit and BIOS reprograms those EM4/5
> > registers.
> > >    There is no such implementation in Linux gfx driver now. We need
> > time to check the effort.
> > >
> > > (2) Gfx driver notify audio driver on exiting LPSP and CDCLK value
> > > (by
> > reading GPU registers), so audio driver can reprograms EM4/5 registers.
> > >   The open source interface between audio/gfx driver is still not
> ready.
> > Would an private API to get CDCLK from gfx driver will be a possible
> > workaround atm?
> > >
> > > Could you share your opinion on this issue?
> >
> > To me, the former sounds like an easier solution.  In the latter case,
> > it can be still an issue of module loading order.
> 
> Can we introduce a private i915 interface for the audio driver to get the
> CDCLK as a quick fix?  Then the audio driver can reprogram the EM4/5
> M/N values as per the CDCLK.
> 
> The interface can be like 'int i915_get_cdclk_freq(void)' and the audio
> driver will use symbol_request() to get this private API and avoid the issue
> of module loading order.
> We can put the relevant code in sound/pci/hda/ hda_i915.c
> 
> For Broadwell Jani has implemented a notification to BIOS, so BIOS can
> reprogram the EM4/5 value after restoring the power.
> http://patchwork.freedesktop.org/patch/28866/
> http://patchwork.freedesktop.org/patch/28868/
> 
> But for Haswell, BIOS does not support the above notification and provide
> another heavy-weight notification which will enable/disable the HD-A
> controller at runtime and so need OS to re-enumerate the device stack.
> That causes system hang. And test on Haswell shows that reprogramming
> the EM4/5 as per the CDCLK is also enough to give good BCLK and audio
> rate. So we hope to avoid further dependency on the black-box BIOS.
> 
> Libin is working on the generic interface between audio and gfx now and
> we can clean up code in the future.
> But before the interface is ready, a private API to query CDCLK seems to be
> a quick and reliable fix for Haswell and Broadwell.
> 
> Thanks
> Mengdong
> 
> 
> 
> _______________________________________________
> Alsa-devel mailing list
> [e-mail]
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Patch

diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index bb65a124..ff9cacd 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -288,6 +288,24 @@  static char *driver_short_names[] = {
 	[AZX_DRIVER_GENERIC] = "HD-Audio Generic",
 };
 
+
+/* Intel HSW/BDW display HDA controller Extended Mode registers.
+ * EM4 (M value) and EM5 (N Value) are used to convert CDClk (Core Display
+ * Clock) to 24MHz BCLK: BCLK = CDCLK * M / N
+ * The values will be lost when the display power well is disabled.
+ */
+#define ICH6_REG_EM4			0x100c
+#define ICH6_REG_EM5			0x1010
+
+struct hda_intel {
+	struct azx chip;
+
+	/* HSW/BDW display HDA controller to restore BCLK from CDCLK */
+	unsigned int bclk_m;
+	unsigned int bclk_n;
+};
+
+
 #ifdef CONFIG_X86
 static void __mark_pages_wc(struct azx *chip, struct snd_dma_buffer *dmab, bool on)
 {
@@ -580,6 +598,22 @@  static int param_set_xint(const char *val, const struct kernel_param *kp)
 #define azx_del_card_list(chip) /* NOP */
 #endif /* CONFIG_PM */
 
+static void haswell_save_bclk(struct azx *chip)
+{
+	struct hda_intel *hda = container_of(chip, struct hda_intel, chip);
+
+	hda->bclk_m = azx_readw(chip, EM4);
+	hda->bclk_n = azx_readw(chip, EM5);
+}
+
+static void haswell_restore_bclk(struct azx *chip)
+{
+	struct hda_intel *hda = container_of(chip, struct hda_intel, chip);
+
+	azx_writew(chip, EM4, hda->bclk_m);
+	azx_writew(chip, EM5, hda->bclk_n);
+}
+
 #if defined(CONFIG_PM_SLEEP) || defined(SUPPORT_VGA_SWITCHEROO)
 /*
  * power management
@@ -606,6 +640,13 @@  static int azx_suspend(struct device *dev)
 		free_irq(chip->irq, chip);
 		chip->irq = -1;
 	}
+
+	/* Save BCLK M/N values before they become invalid in D3.
+	 * Will test if display power well can be released now.
+	 */
+	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL)
+		haswell_save_bclk(chip);
+
 	if (chip->msi)
 		pci_disable_msi(chip->pci);
 	pci_disable_device(pci);
@@ -625,8 +666,10 @@  static int azx_resume(struct device *dev)
 	if (chip->disabled)
 		return 0;
 
-	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL)
+	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL) {
 		hda_display_power(true);
+		haswell_restore_bclk(chip);
+	}
 	pci_set_power_state(pci, PCI_D0);
 	pci_restore_state(pci);
 	if (pci_enable_device(pci) < 0) {
@@ -670,8 +713,10 @@  static int azx_runtime_suspend(struct device *dev)
 	azx_stop_chip(chip);
 	azx_enter_link_reset(chip);
 	azx_clear_irq_pending(chip);
-	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL)
+	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL) {
+		haswell_save_bclk(chip);
 		hda_display_power(false);
+	}
 	return 0;
 }
 
@@ -689,8 +734,10 @@  static int azx_runtime_resume(struct device *dev)
 	if (!(chip->driver_caps & AZX_DCAPS_PM_RUNTIME))
 		return 0;
 
-	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL)
+	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL) {
 		hda_display_power(true);
+		haswell_restore_bclk(chip);
+	}
 
 	/* Read STATESTS before controller reset */
 	status = azx_readw(chip, STATESTS);
@@ -883,6 +930,8 @@  static int register_vga_switcheroo(struct azx *chip)
 static int azx_free(struct azx *chip)
 {
 	struct pci_dev *pci = chip->pci;
+	struct hda_intel *hda = container_of(chip, struct hda_intel, chip);
+
 	int i;
 
 	if ((chip->driver_caps & AZX_DCAPS_PM_RUNTIME)
@@ -930,7 +979,7 @@  static int azx_free(struct azx *chip)
 		hda_display_power(false);
 		hda_i915_exit();
 	}
-	kfree(chip);
+	kfree(hda);
 
 	return 0;
 }
@@ -1174,6 +1223,7 @@  static int azx_create(struct snd_card *card, struct pci_dev *pci,
 	static struct snd_device_ops ops = {
 		.dev_free = azx_dev_free,
 	};
+	struct hda_intel *hda;
 	struct azx *chip;
 	int err;
 
@@ -1183,13 +1233,14 @@  static int azx_create(struct snd_card *card, struct pci_dev *pci,
 	if (err < 0)
 		return err;
 
-	chip = kzalloc(sizeof(*chip), GFP_KERNEL);
-	if (!chip) {
-		dev_err(card->dev, "Cannot allocate chip\n");
+	hda = kzalloc(sizeof(*hda), GFP_KERNEL);
+	if (!hda) {
+		dev_err(card->dev, "Cannot allocate hda\n");
 		pci_disable_device(pci);
 		return -ENOMEM;
 	}
 
+	chip = &hda->chip;
 	spin_lock_init(&chip->reg_lock);
 	mutex_init(&chip->open_mutex);
 	chip->card = card;