[alsa-devel] [PATCH] ALSA: hda - set intel audio clock to a properly value
Takashi Iwai
tiwai at suse.de
Mon Mar 20 10:50:31 CET 2017
On Tue, 07 Mar 2017 07:20:22 +0100,
libin.yang at intel.com wrote:
>
> From: Libin Yang <libin.yang at intel.com>
>
> On some Intel platforms, the audio clock may not be set correctly
> with initial setting. This will cause the audio playback/capture
> rates wrong.
>
> This patch checks the audio clock setting and will set it to a
> properly value if it is not set correct.
>
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=188411
>
> Signed-off-by: Libin Yang <libin.yang at intel.com>
> ---
> include/sound/hda_register.h | 12 +++--
> sound/hda/ext/hdac_ext_controller.c | 6 +--
> sound/pci/hda/hda_intel.c | 91 +++++++++++++++++++++++++++++++++++++
> 3 files changed, 103 insertions(+), 6 deletions(-)
>
> diff --git a/include/sound/hda_register.h b/include/sound/hda_register.h
> index 0013063..7ea16cb 100644
> --- a/include/sound/hda_register.h
> +++ b/include/sound/hda_register.h
> @@ -227,6 +227,8 @@ enum { SDI0, SDI1, SDI2, SDI3, SDO0, SDO1, SDO2, SDO3 };
> #define AZX_REG_PPLCLLPU 0xC
>
> /* registers for Multiple Links Capability Structure */
> +/* Multiple Links Capability */
> +#define AZX_REG_ML_CAP_BASE 0xc00
> #define AZX_ML_CAP_ID 0x2
> #define AZX_REG_ML_MLCH 0x00
> #define AZX_REG_ML_MLCD 0x04
> @@ -243,9 +245,13 @@ enum { SDI0, SDI1, SDI2, SDI3, SDO0, SDO1, SDO2, SDO3 };
> #define AZX_REG_ML_LOUTPAY 0x20
> #define AZX_REG_ML_LINPAY 0x30
>
> -#define AZX_MLCTL_SPA (1<<16)
> -#define AZX_MLCTL_CPA 23
> -
> +#define AZX_REG_ML_LCAPx(x) (AZX_REG_ML_CAP_BASE + (0x40 + 0x40 * x))
> +#define AZX_REG_ML_LCTLx(x) (AZX_REG_ML_CAP_BASE + (0x44 + 0x40 * x))
> +#define ML_LCTL_SCF_MASK 0xF
> +#define AZX_MLCTL_SPA (0x1 << 16)
> +#define AZX_MLCTL_CPA (0x1 << 23)
> +#define AZX_MLCTL_SPA_SHIFT 16
> +#define AZX_MLCTL_CPA_SHIFT 23
>
> /* registers for DMA Resume Capability Structure */
> #define AZX_DRSM_CAP_ID 0x5
> diff --git a/sound/hda/ext/hdac_ext_controller.c b/sound/hda/ext/hdac_ext_controller.c
> index 2614691..84f3b81 100644
> --- a/sound/hda/ext/hdac_ext_controller.c
> +++ b/sound/hda/ext/hdac_ext_controller.c
> @@ -171,7 +171,7 @@ static int check_hdac_link_power_active(struct hdac_ext_link *link, bool enable)
> {
> int timeout;
> u32 val;
> - int mask = (1 << AZX_MLCTL_CPA);
> + int mask = (1 << AZX_MLCTL_CPA_SHIFT);
>
> udelay(3);
> timeout = 150;
> @@ -179,10 +179,10 @@ static int check_hdac_link_power_active(struct hdac_ext_link *link, bool enable)
> do {
> val = readl(link->ml_addr + AZX_REG_ML_LCTL);
> if (enable) {
> - if (((val & mask) >> AZX_MLCTL_CPA))
> + if (((val & mask) >> AZX_MLCTL_CPA_SHIFT))
> return 0;
> } else {
> - if (!((val & mask) >> AZX_MLCTL_CPA))
> + if (!((val & mask) >> AZX_MLCTL_CPA_SHIFT))
> return 0;
> }
> udelay(3);
These changes are rather to fix the inconsistencies between CPA and
SPA register definitions. Better to split as a preliminary cleanup
patch (i.e. define AZX_MLCTL_SPA, *_CPA and *_SHIFT properly and adapt
to them).
> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> index c8256a8..017f64f 100644
> --- a/sound/pci/hda/hda_intel.c
> +++ b/sound/pci/hda/hda_intel.c
> @@ -539,6 +539,94 @@ static void bxt_reduce_dma_latency(struct azx *chip)
> azx_writel(chip, SKL_EM4L, val);
> }
>
> +/*
> + * ML_LCAP bits:
> + * bit 0: 6 MHz Supported
> + * bit 1: 12 MHz Supported
> + * bit 2: 24 MHz Supported
> + * bit 3: 48 MHz Supported
> + * bit 4: 96 MHz Supported
> + * bit 5: 192 MHz Supported
> + */
> +static int intel_get_lctl_scf(struct azx *chip)
> +{
> + u32 val;
> +
> + val = azx_readl(chip, ML_LCAPx(0));
> +
> + if (val & (1 << 2))
> + return 2;
> + else if (val & (1 << 3))
> + return 3;
> + else if (val & (1 << 1))
> + return 1;
> + else if (val & (1 << 4))
> + return 4;
> + else if (val & (1 << 5))
> + return 5;
I guess a loop is cleaner and error-prone, e.g.:
static int preferred_bits[] = { 2, 3, 1, 4, 5 };
int i;
for (i = 0; i < ARRAY_SIZE(preferred_bits); i++)
if (val & (1 << i))
return i;
return 0;
> +
> + dev_warn(chip->card->dev, "set audio clock frequency to 6MHz");
> + return 0;
> +}
> +
> +static void intel_init_lctl(struct azx *chip)
> +{
> + u32 val;
> + int timeout;
> +
> + /* 0. check lctl register value is correct or not */
> + /* the codecs are sharing the first link setting by default */
> + val = azx_readl(chip, ML_LCTLx(0));
> + /* if SCF is already set, let's use it */
> + if ((val & ML_LCTL_SCF_MASK) != 0)
> + return;
> +
> + /*
> + * Before operatiing on SPA, CPA must match SPA.
operating.
> + * Any deviation may result in undefined behavior.
> + */
> + if (((val & AZX_MLCTL_SPA) >> AZX_MLCTL_SPA_SHIFT) ^
> + ((val & AZX_MLCTL_CPA) >> AZX_MLCTL_CPA_SHIFT))
Should it be better with "==" instead of XOR here?
> + return;
> +
> + /* 1. turn link down: set SPA to 0 and wait CPA to 0 */
> + val = azx_readl(chip, ML_LCTLx(0));
> + val &= ~AZX_MLCTL_SPA;
> + azx_writel(chip, ML_LCTLx(0), val);
> + /* wait for CPA */
> + timeout = 50;
> + while (timeout) {
> + if ((azx_readl(chip, ML_LCTLx(0)) & AZX_MLCTL_CPA) == 0)
> + break;
> + timeout--;
> + udelay(10);
> + }
> + if (!timeout)
> + goto SET_SPA;
> + /* need add 100ns delay for hardware ready */
> + udelay(100);
> +
> + /* 2. update SCF to select a properly audio clock*/
> + val &= ~ML_LCTL_SCF_MASK;
> + val |= intel_get_lctl_scf(chip);
> + azx_writel(chip, ML_LCTLx(0), val);
> +
> +SET_SPA:
Use lower letters for a label.
> + /* 4. turn link up: set SPA to 1 and wait CPA to 1 */
> + val = azx_readl(chip, ML_LCTLx(0));
> + val |= AZX_MLCTL_SPA;
> + azx_writel(chip, ML_LCTLx(0), val);
> + timeout = 50;
> + while (timeout) {
> + if ((azx_readl(chip, ML_LCTLx(0)) & AZX_MLCTL_CPA) != 0)
> + break;
> + timeout--;
> + udelay(10);
> + }
> + /* need add 100ns delay for hardware ready */
> + udelay(100);
Well, both clearing and setting SPA are almost the same procedure, so
better to factor out a small function for that, IMO.
thanks,
Takashi
More information about the Alsa-devel
mailing list