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