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
