Re: [PATCH v2 1/2] ALSA - hda: Add support for parsing new HDA capabilities

[Takashi Iwai <tiwai@suse.de>]

On Tue, 19 Jul 2016 12:46:41 +0200,
Vinod Koul wrote:
> 
> From: Guneshwor Singh <guneshwor.o.singh@intel.com>
> 
> Skylake onwards HDA controller supports new capabilities like
> Global Time Stamping (GTS) capability. So add support to parse
> these new capabilities.
> 
> Signed-off-by: Guneshwor Singh <guneshwor.o.singh@intel.com>
> Signed-off-by: Hardik T Shah <hardik.t.shah@intel.com>
> Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> ---
>  include/sound/hda_register.h   | 23 ++++++++++++++++++++
>  sound/pci/hda/hda_controller.c | 49 ++++++++++++++++++++++++++++++++++++++++++
>  sound/pci/hda/hda_controller.h | 27 +++++++++++++++++++++++
>  sound/pci/hda/hda_intel.c      | 12 +++++++++++
>  4 files changed, 111 insertions(+)
> 
> diff --git a/include/sound/hda_register.h b/include/sound/hda_register.h
> index ff1aecf325e8..e4178328e8c8 100644
> --- a/include/sound/hda_register.h
> +++ b/include/sound/hda_register.h
> @@ -242,6 +242,29 @@ enum { SDI0, SDI1, SDI2, SDI3, SDO0, SDO1, SDO2, SDO3 };
>  /* Interval used to calculate the iterating register offset */
>  #define AZX_DRSM_INTERVAL		0x08
>  
> +/* Global time synchronization registers */
> +#define GTSCC_TSCCD_MASK		0x80000000
> +#define GTSCC_TSCCD_SHIFT		31
> +#define GTSCC_TSCCI_MASK		0x20
> +#define GTSCC_CDMAS_DMA_DIR_SHIFT	4
> +
> +#define WALFCC_CIF_MASK			0x1FF
> +#define WALFCC_FN_SHIFT			9
> +#define HDA_CLK_CYCLES_PER_FRAME	512
> +
> +/*
> + * An error occurs near frame "rollover". The clocks in frame value indicates
> + * whether this error may have occurred. Here we use the value of 10. Please
> + * see the errata for the right number [<10]
> + */
> +#define HDA_MAX_CYCLE_VALUE		499
> +#define HDA_MAX_CYCLE_OFFSET		10
> +#define HDA_MAX_CYCLE_READ_RETRY	10
> +
> +#define TSCCU_CCU_SHIFT			32
> +#define LLPC_CCU_SHIFT			32
> +
> +
>  /*
>   * helpers to read the stream position
>   */
> diff --git a/sound/pci/hda/hda_controller.c b/sound/pci/hda/hda_controller.c
> index 27de8015717d..7e5e4c261e51 100644
> --- a/sound/pci/hda/hda_controller.c
> +++ b/sound/pci/hda/hda_controller.c
> @@ -393,6 +393,50 @@ static struct snd_pcm_hardware azx_pcm_hw = {
>  	.fifo_size =		0,
>  };
>  
> +#define AZX_MAX_CAPS 10
> +
> +/**
> + * azx_parse_capabilities - parse the additional HDA capabilities for HDA
> + * controller. HDA controller defines capabilities as link list which can be
> + * parsed to find the controller support.
> + *
> + * @chip: azx controller
> + */
> +void azx_parse_capabilities(struct azx *chip)
> +{
> +	unsigned int cur_cap;
> +	unsigned int offset; unsigned int counter = 0;

Need a line break.


> +
> +	offset = azx_readl(chip, LLCH);
> +
> +	/* Lets walk the linked capabilities list */
> +	do {
> +		cur_cap = _snd_hdac_chip_read(l, azx_bus(chip), offset);
> +
> +		switch ((cur_cap & CAP_HDR_ID_MASK) >> CAP_HDR_ID_OFF) {
> +		case GTS_CAP_ID:
> +			dev_dbg(chip->card->dev, "Found GTS capability");
> +			chip->gts_present = 1;
> +			break;
> +
> +		default:
> +			break;
> +		}
> +
> +		counter++;
> +
> +		if (counter > AZX_MAX_CAPS) {
> +			dev_err(chip->card->dev, "We exceeded azx capabilities!!!\n");
> +			break;
> +		}
> +
> +		/* read the offset of next capabiity */
> +		offset = cur_cap & CAP_HDR_NXT_PTR_MASK;
> +
> +	} while (offset);

Wouldn't it be safer to use a normal while () {} loop?
The first LLCH read might be zero, in theory.


> --- a/sound/pci/hda/hda_intel.c
> +++ b/sound/pci/hda/hda_intel.c
> @@ -1655,6 +1655,18 @@ static int azx_first_init(struct azx *chip)
>  		return -ENXIO;
>  	}
>  
> +	if (IS_SKL_PLUS(pci))
> +		azx_parse_capabilities(chip);
> +
> +	/*
> +	 * Some Intel CPUs has always running timer (ART) feature and
> +	 * controller may have Global time sync reporting capability, so
> +	 * check both of these before declaring synchronized time reporting
> +	 * capability SNDRV_PCM_INFO_HAS_LINK_SYNCHRONIZED_ATIME
> +	 */
> +	if (!(chip->gts_present && boot_cpu_has(X86_FEATURE_ART)))
> +		chip->gts_present = false;

Need #ifdef CONFIG_X86 guard here, too.
Also, the inclusion of <asm/cpufeature.h> isn't needed?  (Again
X86-only.)


thanks,

Takashi