[alsa-devel] [PATCH] ALSA: hda - set intel audio clock to a properly value
Yang, Libin
libin.yang at intel.com
Tue Mar 21 17:11:27 CET 2017
Hi Takashi,
Thanks for review and please see my comments inline.
I'm OOO currently and I will send the updated one next week.
>-----Original Message-----
>From: Takashi Iwai [mailto:tiwai at suse.de]
>Sent: Monday, March 20, 2017 5:51 PM
>To: Yang, Libin <libin.yang at intel.com>
>Cc: alsa-devel at alsa-project.org; Lin, Mengdong <mengdong.lin at intel.com>;
>infernix at infernix.net
>Subject: Re: [alsa-devel] [PATCH] ALSA: hda - set intel audio clock to a properly
>value
>
>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).
Yes. I will do it.
>
>
>> 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;
OK. I will do it.
>
>> +
>> + 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.
Yes, my typo.
>
>> + * 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?
AZX_MLCTL_SPA bit value should be the same with AZX_MLCTL_CPA bit
value before operation. If they are different, we should not touch this
register. So if XOR is true, we should return directly.
>
>> + 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.
OK.
>
>
>> + /* 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.
OK, I will do it. Thanks for suggestion
Best Regards,
Libin
>
>
>thanks,
>
>Takashi
More information about the Alsa-devel
mailing list