[alsa-devel] [PATCH 0/3] ALSA: hda - restore BCLK from CDCLK for Haswell/Broadwell display audio
From: Mengdong Lin mengdong.lin@intel.com
The 3 patches are for Haswell/Broadwell display HD-A controller.
The 1st patch is for i915 display driver, providing a private interface for the audio driver to query CDCLK.
The other 2 patches are for HD-A driver, will restore BCLK by setting M/N values as per CDCLK.
Mengdong Lin (3): drm/i915: provide interface for audio driver to query cdclk ALSA: hda - define hda_get_display_clk to query CDCLK for Haswell/Broadwell ALSA: hda - restore BCLK M/N value as per CDCLK for HSW/BDW display HDA controller
drivers/gpu/drm/i915/intel_pm.c | 21 +++++++++++++++ include/drm/i915_powerwell.h | 1 + sound/pci/hda/hda_i915.c | 22 +++++++++++++++ sound/pci/hda/hda_i915.h | 1 + sound/pci/hda/hda_intel.c | 60 +++++++++++++++++++++++++---------------- 5 files changed, 82 insertions(+), 23 deletions(-)
From: Jani Nikula jani.nikula@intel.com
Signed-off-by: Jani Nikula jani.nikula@intel.com
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index a90fdbd..21170e5 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -6256,6 +6256,27 @@ int i915_release_power_well(void) } EXPORT_SYMBOL_GPL(i915_release_power_well);
+/* + * Private interface for the audio driver to get CDCLK in kHz. + * + * Caller must request power well using i915_request_power_well() prior to + * making the call. + */ +int i915_get_cdclk_freq(void) +{ + struct drm_i915_private *dev_priv; + + if (!hsw_pwr) + return -ENODEV; + + dev_priv = container_of(hsw_pwr, struct drm_i915_private, + power_domains); + + return intel_ddi_get_cdclk_freq(dev_priv); +} +EXPORT_SYMBOL_GPL(i915_get_cdclk_freq); + + #define POWER_DOMAIN_MASK (BIT(POWER_DOMAIN_NUM) - 1)
#define HSW_ALWAYS_ON_POWER_DOMAINS ( \ diff --git a/include/drm/i915_powerwell.h b/include/drm/i915_powerwell.h index 2baba99..baa6f11 100644 --- a/include/drm/i915_powerwell.h +++ b/include/drm/i915_powerwell.h @@ -32,5 +32,6 @@ /* For use by hda_i915 driver */ extern int i915_request_power_well(void); extern int i915_release_power_well(void); +extern int i915_get_cdclk_freq(void);
#endif /* _I915_POWERWELL_H_ */
At Wed, 2 Jul 2014 22:43:46 +0800, mengdong.lin@intel.com wrote:
From: Jani Nikula jani.nikula@intel.com
Signed-off-by: Jani Nikula jani.nikula@intel.com
If you submit a patch, you need to add your own sign-off. Just your signed-off-by line after Jani's line.
The code change itself looks OK.
thanks,
Takashi
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index a90fdbd..21170e5 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -6256,6 +6256,27 @@ int i915_release_power_well(void) } EXPORT_SYMBOL_GPL(i915_release_power_well);
+/*
- Private interface for the audio driver to get CDCLK in kHz.
- Caller must request power well using i915_request_power_well() prior to
- making the call.
- */
+int i915_get_cdclk_freq(void) +{
- struct drm_i915_private *dev_priv;
- if (!hsw_pwr)
return -ENODEV;- dev_priv = container_of(hsw_pwr, struct drm_i915_private,
power_domains);- return intel_ddi_get_cdclk_freq(dev_priv);
+} +EXPORT_SYMBOL_GPL(i915_get_cdclk_freq);
#define POWER_DOMAIN_MASK (BIT(POWER_DOMAIN_NUM) - 1)
#define HSW_ALWAYS_ON_POWER_DOMAINS ( \ diff --git a/include/drm/i915_powerwell.h b/include/drm/i915_powerwell.h index 2baba99..baa6f11 100644 --- a/include/drm/i915_powerwell.h +++ b/include/drm/i915_powerwell.h @@ -32,5 +32,6 @@ /* For use by hda_i915 driver */ extern int i915_request_power_well(void); extern int i915_release_power_well(void); +extern int i915_get_cdclk_freq(void);
#endif /* _I915_POWERWELL_H_ */
1.8.1.2
From: Mengdong Lin mengdong.lin@intel.com
This patch defines hda_get_display_clk() to query Core Display Clock (CDCLK) from the i915 display driver, via a private API i915_get_cdclk_freq().
The audio driver will set M/N values as per the CDCLK for restoring BCLK of the display HD-A controller.
Signed-off-by: Mengdong Lin mengdong.lin@intel.com
diff --git a/sound/pci/hda/hda_i915.c b/sound/pci/hda/hda_i915.c index e9e8a4a..76db293 100644 --- a/sound/pci/hda/hda_i915.c +++ b/sound/pci/hda/hda_i915.c @@ -24,6 +24,7 @@
static int (*get_power)(void); static int (*put_power)(void); +static int (*get_cdclk)(void);
int hda_display_power(bool enable) { @@ -38,6 +39,14 @@ int hda_display_power(bool enable) return put_power(); }
+int hda_get_display_clk(void) +{ + if (!get_cdclk) + return -ENODEV; + + return get_cdclk(); +} + int hda_i915_init(void) { int err = 0; @@ -55,6 +64,15 @@ int hda_i915_init(void) return -ENODEV; }
+ get_cdclk = symbol_request(i915_get_cdclk_freq); + if (!get_cdclk) { + symbol_put(i915_request_power_well); + get_power = NULL; + symbol_put(i915_release_power_well); + put_power = NULL; + return -ENODEV; + } + pr_debug("HDA driver get symbol successfully from i915 module\n");
return err; @@ -70,6 +88,10 @@ int hda_i915_exit(void) symbol_put(i915_release_power_well); put_power = NULL; } + if (get_cdclk) { + symbol_put(i915_get_cdclk_freq); + get_cdclk = NULL; + }
return 0; } diff --git a/sound/pci/hda/hda_i915.h b/sound/pci/hda/hda_i915.h index bfd835f..b4420e1 100644 --- a/sound/pci/hda/hda_i915.h +++ b/sound/pci/hda/hda_i915.h @@ -18,6 +18,7 @@
#ifdef CONFIG_SND_HDA_I915 int hda_display_power(bool enable); +int hda_get_display_clk(void); int hda_i915_init(void); int hda_i915_exit(void); #else
From: Mengdong Lin mengdong.lin@intel.com
For HSW/BDW display HD-A controller, this patch restores BCLK by setting the M/N values as per the core display clock (CDCLK) queried from i915 display driver.
And the audio driver will also restore BCLK in azx_first_init() since the display driver can turn off the shared power in boot phase if only eDP is connected and M/N values will be lost and must be reprogrammed.
Signed-off-by: Mengdong Lin mengdong.lin@intel.com
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index 25753db..c7dfd1d 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -299,10 +299,6 @@ static char *driver_short_names[] = {
struct hda_intel { struct azx chip; - - /* HSW/BDW display HDA controller to restore BCLK from CDCLK */ - unsigned int bclk_m; - unsigned int bclk_n; };
@@ -598,20 +594,41 @@ 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); + int cdclk_freq; + unsigned int bclk_m, bclk_n; + + cdclk_freq = hda_get_display_clk(); + switch (cdclk_freq) { + case 337500: + bclk_m = 16; + bclk_n = 225; + break; + + case 450000: /* default CDCLK 450MHz */ + bclk_m = 4; + bclk_n = 75; + break; + + case 540000: + bclk_m = 4; + bclk_n = 90; + break; + + case 675000: + bclk_m = 8; + bclk_n = 225; + break;
- azx_writew(chip, EM4, hda->bclk_m); - azx_writew(chip, EM5, hda->bclk_n); + default: + bclk_m = 4; + bclk_n = 75; + } + + azx_writew(chip, EM4, bclk_m); + azx_writew(chip, EM5, bclk_n); }
#if defined(CONFIG_PM_SLEEP) || defined(SUPPORT_VGA_SWITCHEROO) @@ -641,12 +658,6 @@ static int azx_suspend(struct device *dev) 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); @@ -713,10 +724,9 @@ 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) { - haswell_save_bclk(chip); + if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL) hda_display_power(false); - } + return 0; }
@@ -1426,6 +1436,10 @@ static int azx_first_init(struct azx *chip)
/* initialize chip */ azx_init_pci(chip); + + if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL) + haswell_restore_bclk(chip); + azx_init_chip(chip, (probe_only[dev] & 2) == 0);
/* codec detection */
At Wed, 2 Jul 2014 22:44:07 +0800, mengdong.lin@intel.com wrote:
From: Mengdong Lin mengdong.lin@intel.com
For HSW/BDW display HD-A controller, this patch restores BCLK by setting the M/N values as per the core display clock (CDCLK) queried from i915 display driver.
And the audio driver will also restore BCLK in azx_first_init() since the display driver can turn off the shared power in boot phase if only eDP is connected and M/N values will be lost and must be reprogrammed.
Signed-off-by: Mengdong Lin mengdong.lin@intel.com
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index 25753db..c7dfd1d 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -299,10 +299,6 @@ static char *driver_short_names[] = {
struct hda_intel { struct azx chip;
- /* HSW/BDW display HDA controller to restore BCLK from CDCLK */
- unsigned int bclk_m;
- unsigned int bclk_n;
};
@@ -598,20 +594,41 @@ 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);
- int cdclk_freq;
- unsigned int bclk_m, bclk_n;
- cdclk_freq = hda_get_display_clk();
- switch (cdclk_freq) {
- case 337500:
bclk_m = 16;bclk_n = 225;break;- case 450000: /* default CDCLK 450MHz */
bclk_m = 4;bclk_n = 75;break;- case 540000:
bclk_m = 4;bclk_n = 90;break;- case 675000:
bclk_m = 8;bclk_n = 225;break;
- azx_writew(chip, EM4, hda->bclk_m);
- azx_writew(chip, EM5, hda->bclk_n);
- default:
bclk_m = 4;bclk_n = 75;
You can put "default:" around "case 45000:", and move the comment about default CDCLK there.
- }
- azx_writew(chip, EM4, bclk_m);
- azx_writew(chip, EM5, bclk_n);
}
IMO, the whole function can be moved to hda_i915.c and provided as a function such as hda_set_display_clk(). Then you can drop hda_get_display_clk() but call get_cdclk() internally.
Also, all these patches may have Cc to stable, right?
thanks,
Takashi
Hi Takashi,
Thanks for your review! The revised patches are submitted: [PATCH v2 1/2] drm/i915: provide interface for audio driver to query cdclk [PATCH v2 2/2] ALSA: hda - restore BCLK M/N value as per CDCLK for HSW/BDW display HDA controller
And we think these patches may have Cc to stable, since Haswell need them.
Hi Jani,
Your first patch was also submitted to Intel gfx mailing list. Thanks for your help on this issue!
Regards Mengdong
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Wednesday, July 02, 2014 11:01 PM To: Lin, Mengdong Cc: alsa-devel@alsa-project.org; Nikula, Jani Subject: Re: [PATCH 3/3] ALSA: hda - restore BCLK M/N value as per CDCLK for HSW/BDW display HDA controller
At Wed, 2 Jul 2014 22:44:07 +0800, mengdong.lin@intel.com wrote:
From: Mengdong Lin mengdong.lin@intel.com
For HSW/BDW display HD-A controller, this patch restores BCLK by setting the M/N values as per the core display clock (CDCLK) queried from i915 display driver.
And the audio driver will also restore BCLK in azx_first_init() since the display driver can turn off the shared power in boot phase if only eDP is connected and M/N values will be lost and must be
reprogrammed.
Signed-off-by: Mengdong Lin mengdong.lin@intel.com
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index 25753db..c7dfd1d 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -299,10 +299,6 @@ static char *driver_short_names[] = {
struct hda_intel { struct azx chip;
- /* HSW/BDW display HDA controller to restore BCLK from CDCLK */
- unsigned int bclk_m;
- unsigned int bclk_n;
};
@@ -598,20 +594,41 @@ 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);
- int cdclk_freq;
- unsigned int bclk_m, bclk_n;
- cdclk_freq = hda_get_display_clk();
- switch (cdclk_freq) {
- case 337500:
bclk_m = 16;bclk_n = 225;break;- case 450000: /* default CDCLK 450MHz */
bclk_m = 4;bclk_n = 75;break;- case 540000:
bclk_m = 4;bclk_n = 90;break;- case 675000:
bclk_m = 8;bclk_n = 225;break;
- azx_writew(chip, EM4, hda->bclk_m);
- azx_writew(chip, EM5, hda->bclk_n);
- default:
bclk_m = 4;bclk_n = 75;You can put "default:" around "case 45000:", and move the comment about default CDCLK there.
- }
- azx_writew(chip, EM4, bclk_m);
- azx_writew(chip, EM5, bclk_n);
}
IMO, the whole function can be moved to hda_i915.c and provided as a function such as hda_set_display_clk(). Then you can drop hda_get_display_clk() but call get_cdclk() internally.
Also, all these patches may have Cc to stable, right?
thanks,
Takashi
participants (3)
-
Lin, Mengdong -
mengdong.lin＠intel.com
-
Takashi Iwai