ALSA: hda - set intel audio clock to a properly value

Commit Message

Yang, Libin March 7, 2017, 6:20 a.m. UTC

From: Libin Yang <[e-mail]>

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 <[e-mail]>
---
 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(-)

Comments

Takashi Iwai March 20, 2017, 9:50 a.m. UTC | #1

On Tue, 07 Mar 2017 07:20:22 +0100,
[e-mail] wrote:
> 
> From: Libin Yang <[e-mail]>
> 
> 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 <[e-mail]>
> ---
>  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

Yang, Libin March 21, 2017, 4:11 p.m. UTC | #2

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:[e-mail]]
>Sent: Monday, March 20, 2017 5:51 PM
>To: Yang, Libin <[e-mail]>
>Cc: [e-mail]; Lin, Mengdong <[e-mail]>;
>[e-mail]
>Subject: Re: [alsa-devel] [PATCH] ALSA: hda - set intel audio clock to a properly
>value
>
>On Tue, 07 Mar 2017 07:20:22 +0100,
>[e-mail] wrote:
>>
>> From: Libin Yang <[e-mail]>
>>
>> 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 <[e-mail]>
>> ---
>>  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

Ughreja, Rakesh A March 21, 2017, 4:34 p.m. UTC | #3

Hi Libin,

One generic comment.

This patch uses the enhanced capability structure and so instead 
of using IS_SKL_PLUS() to check the capability it is better to check 
the ppcap. All the SKL platforms may not be having the enhanced
capability structure.

Regards,
Rakesh 


>-----Original Message-----
>From: [e-mail] [mailto:alsa-devel-bounces@alsa-
>project.org] On Behalf Of Yang, Libin
>Sent: Tuesday, March 21, 2017 9:11 AM
>To: Takashi Iwai <[e-mail]>
>Cc: Lin, Mengdong <[e-mail]>; [e-mail];
>[e-mail]
>Subject: Re: [alsa-devel] [PATCH] ALSA: hda - set intel audio clock to a
>properly value
>
>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:[e-mail]]
>>Sent: Monday, March 20, 2017 5:51 PM
>>To: Yang, Libin <[e-mail]>
>>Cc: [e-mail]; Lin, Mengdong <[e-mail]>;
>>[e-mail]
>>Subject: Re: [alsa-devel] [PATCH] ALSA: hda - set intel audio clock to a
>properly
>>value
>>
>>On Tue, 07 Mar 2017 07:20:22 +0100,
>>[e-mail] wrote:
>>>
>>> From: Libin Yang <[e-mail]>
>>>
>>> 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 <[e-mail]>
>>> ---
>>>  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
>_______________________________________________
>Alsa-devel mailing list
>[e-mail]
>http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Takashi Iwai March 21, 2017, 4:51 p.m. UTC | #4

On Tue, 21 Mar 2017 17:11:27 +0100,
Yang, Libin wrote:
> 
> >> +	 * 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.

I meant that (A != B) is more understandable than (A ^ B).


Takashi

Yang, Libin March 22, 2017, 8:12 a.m. UTC | #5

>-----Original Message-----
>From: Takashi Iwai [mailto:[e-mail]]
>Sent: Wednesday, March 22, 2017 12:51 AM
>To: Yang, Libin <[e-mail]>
>Cc: [e-mail]; Lin, Mengdong <[e-mail]>;
>[e-mail]
>Subject: Re: [alsa-devel] [PATCH] ALSA: hda - set intel audio clock to a properly
>value
>
>On Tue, 21 Mar 2017 17:11:27 +0100,
>Yang, Libin wrote:
>>
>> >> +	 * 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.
>
>I meant that (A != B) is more understandable than (A ^ B).

Get it. I will use (A != B)

Regards,
Libin

>
>
>Takashi

Yang, Libin March 23, 2017, 12:41 p.m. UTC | #6

HI Rakesh,

Get it. Thanks. I will check it.

Regards,
Libin


>-----Original Message-----
>From: Ughreja, Rakesh A
>Sent: Wednesday, March 22, 2017 12:35 AM
>To: Yang, Libin <[e-mail]>; Takashi Iwai <[e-mail]>
>Cc: Lin, Mengdong <[e-mail]>; [e-mail];
>[e-mail]
>Subject: RE: [alsa-devel] [PATCH] ALSA: hda - set intel audio clock to a properly
>value
>
>Hi Libin,
>
>One generic comment.
>
>This patch uses the enhanced capability structure and so instead of using
>IS_SKL_PLUS() to check the capability it is better to check the ppcap. All the SKL
>platforms may not be having the enhanced capability structure.
>
>Regards,
>Rakesh
>
>
>>-----Original Message-----
>>From: [e-mail]
>>[mailto:alsa-devel-bounces@alsa- project.org] On Behalf Of Yang, Libin
>>Sent: Tuesday, March 21, 2017 9:11 AM
>>To: Takashi Iwai <[e-mail]>
>>Cc: Lin, Mengdong <[e-mail]>;
>>[e-mail]; [e-mail]
>>Subject: Re: [alsa-devel] [PATCH] ALSA: hda - set intel audio clock to
>>a properly value
>>
>>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:[e-mail]]
>>>Sent: Monday, March 20, 2017 5:51 PM
>>>To: Yang, Libin <[e-mail]>
>>>Cc: [e-mail]; Lin, Mengdong
>>><[e-mail]>; [e-mail]
>>>Subject: Re: [alsa-devel] [PATCH] ALSA: hda - set intel audio clock to
>>>a
>>properly
>>>value
>>>
>>>On Tue, 07 Mar 2017 07:20:22 +0100,
>>>[e-mail] wrote:
>>>>
>>>> From: Libin Yang <[e-mail]>
>>>>
>>>> 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 <[e-mail]>
>>>> ---
>>>>  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
>>_______________________________________________
>>Alsa-devel mailing list
>>[e-mail]
>>http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Jeeja KP March 23, 2017, 1:11 p.m. UTC | #7

> -----Original Message-----
> From: [e-mail] [mailto:alsa-devel-
> [e-mail]] On Behalf Of Yang, Libin
> Sent: Tuesday, March 7, 2017 11:50 AM
> To: [e-mail]; [e-mail]
> Cc: Yang, Libin <[e-mail]>; Lin, Mengdong
> <[e-mail]>; [e-mail]
> Subject: [alsa-devel] [PATCH] ALSA: hda - set intel audio clock to a properly
> value
> 
> From: Libin Yang <[e-mail]>
> 
> 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 <[e-mail]>
> ---
>  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
Base is already available as part of bus, use bus->mlcap


> +#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))
This is specific to the had legacy driver as only link0 is used, I think this can be moved to driver
 As define. If it required to use all the link, then need to use ext functions to initialize the address
per link.

> +#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
> 
> +
>  static void hda_intel_init_chip(struct azx *chip, bool full_reset)  {
>  	struct hdac_bus *bus = azx_bus(chip);
> @@ -564,6 +652,9 @@ static void hda_intel_init_chip(struct azx *chip, bool
> full_reset)
> +
> +	if (IS_SKL_PLUS(pci))
> +		intel_init_lctl(chip);
Use bus->mlcap to check for multilink capability  is present.

Yang, Libin March 30, 2017, 7:58 a.m. UTC | #8

Hi Jeeja,

>-----Original Message-----
>From: Kp, Jeeja
>Sent: Thursday, March 23, 2017 9:11 PM
>To: Yang, Libin <[e-mail]>; [e-mail];
>[e-mail]
>Cc: Lin, Mengdong <[e-mail]>; [e-mail]
>Subject: RE: [alsa-devel] [PATCH] ALSA: hda - set intel audio clock to a properly
>value
>
>> -----Original Message-----
>> From: [e-mail] [mailto:alsa-devel-
>> [e-mail]] On Behalf Of Yang, Libin
>> Sent: Tuesday, March 7, 2017 11:50 AM
>> To: [e-mail]; [e-mail]
>> Cc: Yang, Libin <[e-mail]>; Lin, Mengdong
>> <[e-mail]>; [e-mail]
>> Subject: [alsa-devel] [PATCH] ALSA: hda - set intel audio clock to a
>> properly value
>>
>> From: Libin Yang <[e-mail]>
>>
>> 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 <[e-mail]>
>> ---
>>  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
>Base is already available as part of bus, use bus->mlcap
>
>
>> +#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))
>This is specific to the had legacy driver as only link0 is used, I think this can be
>moved to driver  As define. If it required to use all the link, then need to use
>ext functions to initialize the address per link.

I'm not very sure what's your meaning. Do you mean using
+#define AZX_REG_ML_LCAP0	(AZX_REG_ML_CAP_BASE + 0x40)

>
>> +#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
>>
>> +
>>  static void hda_intel_init_chip(struct azx *chip, bool full_reset)  {
>>  	struct hdac_bus *bus = azx_bus(chip); @@ -564,6 +652,9 @@ static
>> void hda_intel_init_chip(struct azx *chip, bool
>> full_reset)
>> +
>> +	if (IS_SKL_PLUS(pci))
>> +		intel_init_lctl(chip);
>Use bus->mlcap to check for multilink capability  is present.

Get it.

Thanks,
Libin

Jeeja KP March 30, 2017, 8:06 a.m. UTC | #9

> >-----Original Message-----
> >From: Kp, Jeeja
> >Sent: Thursday, March 23, 2017 9:11 PM
> >To: Yang, Libin <[e-mail]>; [e-mail];
> >[e-mail]
> >Cc: Lin, Mengdong <[e-mail]>; [e-mail]
> >Subject: RE: [alsa-devel] [PATCH] ALSA: hda - set intel audio clock to
> >a properly value
> >
> >> -----Original Message-----
> >> From: [e-mail] [mailto:alsa-devel-
> >> [e-mail]] On Behalf Of Yang, Libin
> >> Sent: Tuesday, March 7, 2017 11:50 AM
> >> To: [e-mail]; [e-mail]
> >> Cc: Yang, Libin <[e-mail]>; Lin, Mengdong
> >> <[e-mail]>; [e-mail]
> >> Subject: [alsa-devel] [PATCH] ALSA: hda - set intel audio clock to a
> >> properly value
> >>
> >> From: Libin Yang <[e-mail]>
> >>
> >> 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 <[e-mail]>
> >> ---
> >>  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
> >Base is already available as part of bus, use bus->mlcap
> >
> >
> >> +#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))
> >This is specific to the had legacy driver as only link0 is used, I
> >think this can be moved to driver  As define. If it required to use all
> >the link, then need to use ext functions to initialize the address per link.
> 
> I'm not very sure what's your meaning. Do you mean using

Use bus->mlcap + 0x44  as you are always using link0 (add offset, no need to define explicitly - AZX_REG_ML_LCTLx(x)).

Yang, Libin March 30, 2017, 8:24 a.m. UTC | #10

Hi Jeeja,

>-----Original Message-----
>From: Kp, Jeeja
>Sent: Thursday, March 30, 2017 4:07 PM
>To: Yang, Libin <[e-mail]>; [e-mail];
>[e-mail]
>Cc: Lin, Mengdong <[e-mail]>; [e-mail]
>Subject: RE: [alsa-devel] [PATCH] ALSA: hda - set intel audio clock to a properly
>value
>
>> >-----Original Message-----
>> >From: Kp, Jeeja
>> >Sent: Thursday, March 23, 2017 9:11 PM
>> >To: Yang, Libin <[e-mail]>; [e-mail];
>> >[e-mail]
>> >Cc: Lin, Mengdong <[e-mail]>; [e-mail]
>> >Subject: RE: [alsa-devel] [PATCH] ALSA: hda - set intel audio clock
>> >to a properly value
>> >
>> >> -----Original Message-----
>> >> From: [e-mail] [mailto:alsa-devel-
>> >> [e-mail]] On Behalf Of Yang, Libin
>> >> Sent: Tuesday, March 7, 2017 11:50 AM
>> >> To: [e-mail]; [e-mail]
>> >> Cc: Yang, Libin <[e-mail]>; Lin, Mengdong
>> >> <[e-mail]>; [e-mail]
>> >> Subject: [alsa-devel] [PATCH] ALSA: hda - set intel audio clock to
>> >> a properly value
>> >>
>> >> From: Libin Yang <[e-mail]>
>> >>
>> >> 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 <[e-mail]>
>> >> ---
>> >>  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
>> >Base is already available as part of bus, use bus->mlcap
>> >
>> >
>> >> +#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))
>> >This is specific to the had legacy driver as only link0 is used, I
>> >think this can be moved to driver  As define. If it required to use
>> >all the link, then need to use ext functions to initialize the address per link.
>>
>> I'm not very sure what's your meaning. Do you mean using
>
>Use bus->mlcap + 0x44  as you are always using link0 (add offset, no need to
>define explicitly - AZX_REG_ML_LCTLx(x)).

OK. Thanks for suggestion. It can avoid new definition. Thanks for suggestion.

Regards,
Libin

Patch

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);
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;
+
+	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.
+	 * Any deviation may result in undefined behavior.
+	 */
+	if (((val & AZX_MLCTL_SPA) >> AZX_MLCTL_SPA_SHIFT) ^
+		((val & AZX_MLCTL_CPA) >> AZX_MLCTL_CPA_SHIFT))
+		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:
+	/* 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);
+}
+
 static void hda_intel_init_chip(struct azx *chip, bool full_reset)
 {
 	struct hdac_bus *bus = azx_bus(chip);
@@ -564,6 +652,9 @@  static void hda_intel_init_chip(struct azx *chip, bool full_reset)
 	/* reduce dma latency to avoid noise */
 	if (IS_BXT(pci))
 		bxt_reduce_dma_latency(chip);
+
+	if (IS_SKL_PLUS(pci))
+		intel_init_lctl(chip);
 }
 
 /* calculate runtime delay from LPIB */