[PATCH v2 0/2] ALSA - hda: Add support for link audio time reporting

[PATCH v2 0/2] ALSA - hda: Add support for link audio time reporting

[Vinod Koul]

Skylake onwards we support GTS capability and thus can report audio link
time. This series adds support for parsing the capability and then reports
link time.

Guneshwor Singh (2):
  ALSA - hda: Add support for parsing new HDA capabilities
  ALSA - hda: Add support for link audio time reporting

 include/sound/hda_register.h   |  23 ++++
 sound/pci/hda/hda_controller.c | 244 ++++++++++++++++++++++++++++++++++++++++-
 sound/pci/hda/hda_controller.h |  27 +++++
 sound/pci/hda/hda_intel.c      |  12 ++
 4 files changed, 305 insertions(+), 1 deletion(-)

-- 
1.9.1

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

[Vinod Koul]

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;
+
+	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);
+}
+EXPORT_SYMBOL_GPL(azx_parse_capabilities);
+
 static int azx_pcm_open(struct snd_pcm_substream *substream)
 {
 	struct azx_pcm *apcm = snd_pcm_substream_chip(substream);
@@ -412,6 +456,11 @@ static int azx_pcm_open(struct snd_pcm_substream *substream)
 		goto unlock;
 	}
 	runtime->private_data = azx_dev;
+
+	if (chip->gts_present)
+		azx_pcm_hw.info = azx_pcm_hw.info |
+			SNDRV_PCM_INFO_HAS_LINK_SYNCHRONIZED_ATIME;
+
 	runtime->hw = azx_pcm_hw;
 	runtime->hw.channels_min = hinfo->channels_min;
 	runtime->hw.channels_max = hinfo->channels_max;
diff --git a/sound/pci/hda/hda_controller.h b/sound/pci/hda/hda_controller.h
index ec63bbf1ec6d..fc57eef9fd88 100644
--- a/sound/pci/hda/hda_controller.h
+++ b/sound/pci/hda/hda_controller.h
@@ -159,9 +159,14 @@ struct azx {
 	unsigned int region_requested:1;
 	unsigned int disabled:1; /* disabled by vga_switcheroo */
 
+	/* GTS present */
+	unsigned int gts_present:1;
+
 #ifdef CONFIG_SND_HDA_DSP_LOADER
 	struct azx_dev saved_azx_dev;
 #endif
+	unsigned int link_count;
+	unsigned int mlcap_offset;
 };
 
 #define azx_bus(chip)	(&(chip)->bus.core)
@@ -173,6 +178,27 @@ struct azx {
 #define azx_snoop(chip)		true
 #endif
 
+#define AZX_REG_LLCH 0x14
+
+#define AZX_REG_GTS_BASE 0x520
+
+#define AZX_REG_GTSCC	(AZX_REG_GTS_BASE + 0x00)
+#define AZX_REG_WALFCC	(AZX_REG_GTS_BASE + 0x04)
+#define AZX_REG_TSCCL	(AZX_REG_GTS_BASE + 0x08)
+#define AZX_REG_TSCCU	(AZX_REG_GTS_BASE + 0x0C)
+#define AZX_REG_LLPFOC	(AZX_REG_GTS_BASE + 0x14)
+#define AZX_REG_LLPCL	(AZX_REG_GTS_BASE + 0x18)
+#define AZX_REG_LLPCU	(AZX_REG_GTS_BASE + 0x1C)
+
+#define CAP_HDR_VER_OFF                 28
+#define CAP_HDR_VER_MASK                (0xF << CAP_HDR_VER_OFF)
+
+#define CAP_HDR_ID_OFF                  16
+#define CAP_HDR_ID_MASK                 (0xFFF << CAP_HDR_ID_OFF)
+
+#define GTS_CAP_ID                 0x1
+#define CAP_HDR_NXT_PTR_MASK    0xFFFF
+
 /*
  * macros for easy use
  */
@@ -201,6 +227,7 @@ static inline struct azx_dev *get_azx_dev(struct snd_pcm_substream *substream)
 unsigned int azx_get_position(struct azx *chip, struct azx_dev *azx_dev);
 unsigned int azx_get_pos_lpib(struct azx *chip, struct azx_dev *azx_dev);
 unsigned int azx_get_pos_posbuf(struct azx *chip, struct azx_dev *azx_dev);
+void azx_parse_capabilities(struct azx *chip);
 
 /* Stream control. */
 void azx_stop_all_streams(struct azx *chip);
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index 6f8ea13323c1..bd392183fbc1 100644
--- 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;
+
 	if (chip->msi) {
 		if (chip->driver_caps & AZX_DCAPS_NO_MSI64) {
 			dev_dbg(card->dev, "Disabling 64bit MSI\n");
-- 
1.9.1

[PATCH v2 2/2] ALSA - hda: Add support for link audio time reporting

[Vinod Koul]

From: Guneshwor Singh <guneshwor.o.singh@intel.com>

The HDA controller from SKL onwards support additional timestamp
reporting of the link time. The link time is read from HW
registers and converted to audio values.

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>
---
 sound/pci/hda/hda_controller.c | 195 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 194 insertions(+), 1 deletion(-)

diff --git a/sound/pci/hda/hda_controller.c b/sound/pci/hda/hda_controller.c
index 7e5e4c261e51..9ae69c000a13 100644
--- a/sound/pci/hda/hda_controller.c
+++ b/sound/pci/hda/hda_controller.c
@@ -27,6 +27,12 @@
 #include <linux/module.h>
 #include <linux/pm_runtime.h>
 #include <linux/slab.h>
+
+#ifdef CONFIG_X86
+/* for art-tsc conversion */
+#include <asm/tsc.h>
+#endif
+
 #include <sound/core.h>
 #include <sound/initval.h>
 #include "hda_controller.h"
@@ -337,12 +343,169 @@ static snd_pcm_uframes_t azx_pcm_pointer(struct snd_pcm_substream *substream)
 			       azx_get_position(chip, azx_dev));
 }
 
+/*
+ * azx_scale64: Scale base by mult/div while not overflowing sanely
+ *
+ * Derived from scale64_check_overflow in kernel/time/timekeeping.c
+ *
+ * The tmestamps for a 48Khz stream can overflow after (2^64/10^9)/48K which
+ * is about 384307 ie ~4.5 days.
+ *
+ * This scales the calculation so that overflow will happen but after 2^64 /
+ * 48000 secs, which is pretty large!
+ *
+ * In caln below:
+ *	base may overflow, but since there isn’t any additional division
+ *	performed on base it’s OK
+ *	rem can’t overflow because both are 32-bit values
+ */
+
+#ifdef CONFIG_X86
+static u64 azx_scale64(u64 base, u32 num, u32 den)
+{
+	u64 rem;
+
+	rem = do_div(base, den);
+
+	base *= num;
+	rem *= num;
+
+	do_div(rem, den);
+
+	return base + rem;
+}
+
+static int azx_get_sync_time(ktime_t *device,
+		struct system_counterval_t *system, void *ctx)
+{
+	struct snd_pcm_substream *substream = (struct snd_pcm_substream *)ctx;
+	struct azx_dev *azx_dev = get_azx_dev(substream);
+	struct azx_pcm *apcm = snd_pcm_substream_chip(substream);
+	struct azx *chip = apcm->chip;
+	struct snd_pcm_runtime *runtime;
+	u64 ll_counter, ll_counter_l, ll_counter_h;
+	u64 tsc_counter, tsc_counter_l, tsc_counter_h;
+	u32 wallclk_ctr, wallclk_cycles;
+	bool direction;
+	u32 dma_select;
+	u32 timeout = 200;
+	u32 retry_count = 0;
+
+	runtime = substream->runtime;
+
+	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+		direction = 1;
+	else
+		direction = 0;
+
+	/* 0th stream tag is not used, so DMA ch 0 is for 1st stream tag */
+	do {
+		timeout = 100;
+		dma_select = (direction << GTSCC_CDMAS_DMA_DIR_SHIFT) |
+					(azx_dev->core.stream_tag - 1);
+		_snd_hdac_chip_write(l, azx_bus(chip), AZX_REG_GTSCC,
+						dma_select);
+		/* Enable the capture */
+		_snd_hdac_chip_write(l, azx_bus(chip), AZX_REG_GTSCC,
+				     _snd_hdac_chip_read(l, azx_bus(chip),
+					AZX_REG_GTSCC) | GTSCC_TSCCI_MASK);
+
+		while (timeout) {
+			if (_snd_hdac_chip_read(l, azx_bus(chip),
+				AZX_REG_GTSCC) & GTSCC_TSCCD_MASK)
+				break;
+			timeout--;
+		}
+
+		if (!timeout) {
+			dev_err(chip->card->dev, "GTSCC capture Timedout!\n");
+			return -EIO;
+		}
+
+		/* Read wall clock counter */
+		wallclk_ctr = _snd_hdac_chip_read(l, azx_bus(chip),
+						  AZX_REG_WALFCC);
+
+		/* Read TSC counter */
+		tsc_counter_l = _snd_hdac_chip_read(l, azx_bus(chip),
+						    AZX_REG_TSCCL);
+		tsc_counter_h = _snd_hdac_chip_read(l, azx_bus(chip),
+						    AZX_REG_TSCCU);
+
+		/* Read Link counter */
+		ll_counter_l = _snd_hdac_chip_read(l, azx_bus(chip),
+						   AZX_REG_LLPCL);
+		ll_counter_h = _snd_hdac_chip_read(l, azx_bus(chip),
+						   AZX_REG_LLPCU);
+
+		/* Ack: registers read done */
+		_snd_hdac_chip_write(l, azx_bus(chip),
+				     AZX_REG_GTSCC,
+				     (0x1 << GTSCC_TSCCD_SHIFT));
+
+		tsc_counter = (tsc_counter_h << TSCCU_CCU_SHIFT) |
+						tsc_counter_l;
+
+		ll_counter = (ll_counter_h << LLPC_CCU_SHIFT) |	ll_counter_l;
+		wallclk_cycles = wallclk_ctr & WALFCC_CIF_MASK;
+
+		/*
+		 * 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 i.e.,
+		 * HDA_MAX_CYCLE_OFFSET
+		 */
+		if (wallclk_cycles < HDA_MAX_CYCLE_VALUE - HDA_MAX_CYCLE_OFFSET
+		    && wallclk_cycles > HDA_MAX_CYCLE_OFFSET)
+			break;
+
+		/*
+		 * Sleep before we read again, else we may again get
+		 * value near to MAX_CYCLE. Try to sleep for different
+		 * amount of time so we dont hit the same number again
+		 */
+		udelay(retry_count++);
+
+	} while (retry_count != HDA_MAX_CYCLE_READ_RETRY);
+
+	if (retry_count == HDA_MAX_CYCLE_READ_RETRY) {
+		dev_err(chip->card->dev, "Error in WALFCC cycle count\n");
+		return -EIO;
+	}
+
+	*device = ns_to_ktime(azx_scale64(ll_counter,
+				NSEC_PER_SEC, runtime->rate));
+	*device = ktime_add_ns(*device, (wallclk_cycles * NSEC_PER_SEC) /
+			       ((HDA_MAX_CYCLE_VALUE + 1) * runtime->rate));
+
+	*system = convert_art_to_tsc(tsc_counter);
+
+	return 0;
+}
+
+#else
+static int azx_get_sync_time(ktime_t *device,
+		struct system_counterval_t *system, void *ctx)
+{
+	return -ENXIO;
+}
+#endif
+
+static int azx_get_crosststamp(struct snd_pcm_substream *substream,
+			      struct system_device_crosststamp *xtstamp)
+{
+	return get_device_system_crosststamp(azx_get_sync_time,
+					substream, NULL, xtstamp);
+}
+
 static int azx_get_time_info(struct snd_pcm_substream *substream,
 			struct timespec *system_ts, struct timespec *audio_ts,
 			struct snd_pcm_audio_tstamp_config *audio_tstamp_config,
 			struct snd_pcm_audio_tstamp_report *audio_tstamp_report)
 {
 	struct azx_dev *azx_dev = get_azx_dev(substream);
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	struct system_device_crosststamp xtstamp;
 	u64 nsec;
 
 	if ((substream->runtime->hw.info & SNDRV_PCM_INFO_HAS_LINK_ATIME) &&
@@ -361,8 +524,38 @@ static int azx_get_time_info(struct snd_pcm_substream *substream,
 		audio_tstamp_report->accuracy_report = 1; /* rest of structure is valid */
 		audio_tstamp_report->accuracy = 42; /* 24 MHz WallClock == 42ns resolution */
 
-	} else
+	} else if ((runtime->hw.info &
+			SNDRV_PCM_INFO_HAS_LINK_SYNCHRONIZED_ATIME) &&
+			(audio_tstamp_config->type_requested ==
+			SNDRV_PCM_AUDIO_TSTAMP_TYPE_LINK_SYNCHRONIZED)) {
+
+		azx_get_crosststamp(substream, &xtstamp);
+
+		switch (runtime->tstamp_type) {
+		case SNDRV_PCM_TSTAMP_TYPE_MONOTONIC:
+			return -EINVAL;
+
+		case SNDRV_PCM_TSTAMP_TYPE_MONOTONIC_RAW:
+			*system_ts = ktime_to_timespec(xtstamp.sys_monoraw);
+			break;
+
+		default:
+			*system_ts = ktime_to_timespec(xtstamp.sys_realtime);
+			break;
+
+		}
+
+		*audio_ts = ktime_to_timespec(xtstamp.device);
+
+		audio_tstamp_report->actual_type =
+			SNDRV_PCM_AUDIO_TSTAMP_TYPE_LINK_SYNCHRONIZED;
+		audio_tstamp_report->accuracy_report = 1;
+		/* 24 MHz WallClock == 42ns resolution */
+		audio_tstamp_report->accuracy = 42;
+
+	} else {
 		audio_tstamp_report->actual_type = SNDRV_PCM_AUDIO_TSTAMP_TYPE_DEFAULT;
+	}
 
 	return 0;
 }
-- 
1.9.1

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[Takashi Iwai]

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

Re: [PATCH v2 2/2] ALSA - hda: Add support for link audio time reporting

[Takashi Iwai]

On Tue, 19 Jul 2016 12:46:42 +0200,
Vinod Koul wrote:
> 
> +#ifdef CONFIG_X86
....
> +#else
> +static int azx_get_sync_time(ktime_t *device,
> +		struct system_counterval_t *system, void *ctx)
> +{
> +	return -ENXIO;

This doesn't help much because the caller of azx_get_crosstime()
doesn't check the return code.


Takashi

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

[Vinod Koul]

On Wed, Jul 20, 2016 at 07:31:17AM +0200, Takashi Iwai wrote:

> > +	unsigned int offset; unsigned int counter = 0;
> 
> Need a line break.

arghh, will fix..

> > +	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.

Then in that case first read will give error. But yes I see the benifits.

Btw this is same as snd_hdac_ext_bus_parse_capabilities()

Should we move this to hdac and use for both. Ofcourse many new capablities
do not make sense to legacy driver, so we would have to ignore them.

> > +	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.)

This is intel.c :) I did compile it for ARM before sending.

-- 
~Vinod

Re: [PATCH v2 2/2] ALSA - hda: Add support for link audio time reporting

[Vinod Koul]

On Wed, Jul 20, 2016 at 07:36:54AM +0200, Takashi Iwai wrote:
> On Tue, 19 Jul 2016 12:46:42 +0200,
> Vinod Koul wrote:
> > 
> > +#ifdef CONFIG_X86
> ....
> > +#else
> > +static int azx_get_sync_time(ktime_t *device,
> > +		struct system_counterval_t *system, void *ctx)
> > +{
> > +	return -ENXIO;
> 
> This doesn't help much because the caller of azx_get_crosstime()
> doesn't check the return code.

Yes this is placeholder for getting arm build compiled. Anyway on ARM i dont
expect azx_get_crosstime to be called :)

-- 
~Vinod

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

[Takashi Iwai]

On Thu, 21 Jul 2016 06:24:26 +0200,
Vinod Koul wrote:
> 
> On Wed, Jul 20, 2016 at 07:31:17AM +0200, Takashi Iwai wrote:
> 
> > > +	unsigned int offset; unsigned int counter = 0;
> > 
> > Need a line break.
> 
> arghh, will fix..
> 
> > > +	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.
> 
> Then in that case first read will give error. But yes I see the benifits.
> 
> Btw this is same as snd_hdac_ext_bus_parse_capabilities()
> 
> Should we move this to hdac and use for both. Ofcourse many new capablities
> do not make sense to legacy driver, so we would have to ignore them.

It sounds not bad, indeed.

> > > +	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.)
> 
> This is intel.c :)

Intel created non-x86 chips, too :)

> I did compile it for ARM before sending.

With 32bit ARM?  arm64 and s390 have this header, casually.

The driver is for all architectures with PCI.  Try with powerpc or
sparc as well.  Or better to look through the tree to confirm who has
asm/cpufeature.h.


Takashi