[alsa-devel] [PATCH] ALSA: hda - set intel audio clock to a properly value
Yang, Libin
libin.yang at intel.com
Thu Mar 23 13:41:51 CET 2017
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 <libin.yang at intel.com>; Takashi Iwai <tiwai at suse.de>
>Cc: Lin, Mengdong <mengdong.lin at intel.com>; alsa-devel at alsa-project.org;
>infernix at infernix.net
>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: alsa-devel-bounces at alsa-project.org
>>[mailto:alsa-devel-bounces at alsa- project.org] On Behalf Of Yang, Libin
>>Sent: Tuesday, March 21, 2017 9:11 AM
>>To: Takashi Iwai <tiwai at suse.de>
>>Cc: Lin, Mengdong <mengdong.lin at intel.com>;
>>alsa-devel at alsa-project.org; infernix at infernix.net
>>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: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
>>_______________________________________________
>>Alsa-devel mailing list
>>Alsa-devel at alsa-project.org
>>http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
More information about the Alsa-devel
mailing list