[PATCH v5 0/4] ASoC: intel - add skylake PCM driver

[PATCH v5 0/4] ASoC: intel - add skylake PCM driver

[Vinod Koul]

This patch series adds the SKL PCM driver in first two patches and then adds
decoupling of the controller for splitting the links in last patch

This version removes the bus and match function as that is merged in HDA
core now and we have extended HDA bus merged as well

Please note this series is dependent on sound next having hda core patches
and extended bus patches

Jeeja KP (4):
  ASoC: Intel: add Skylake HDA platform driver
  ASoC: Intel - add Skylake HDA audio driver
  ASoC: Intel - add makefile support for SKL driver
  ASoC: intel - adds support for decoupled mode in skl driver

 sound/soc/intel/Kconfig           |  17 +
 sound/soc/intel/Makefile          |   1 +
 sound/soc/intel/skylake/Makefile  |   3 +
 sound/soc/intel/skylake/skl-pcm.c | 936 ++++++++++++++++++++++++++++++++++++++
 sound/soc/intel/skylake/skl.c     | 646 ++++++++++++++++++++++++++
 sound/soc/intel/skylake/skl.h     |  74 +++
 6 files changed, 1677 insertions(+)
 create mode 100644 sound/soc/intel/skylake/Makefile
 create mode 100644 sound/soc/intel/skylake/skl-pcm.c
 create mode 100644 sound/soc/intel/skylake/skl.c
 create mode 100644 sound/soc/intel/skylake/skl.h

-- 
1.9.1

[PATCH v5 1/4] ASoC: Intel: add Skylake HDA platform driver

[Vinod Koul]

From: Jeeja KP <jeeja.kp@intel.com>

This patch starts to add the Skylake HDA platform driver by defining
SoC CPU dais, DMA driver ops and implements ALSA operations

Signed-off-by: Jeeja KP <jeeja.kp@intel.com>
Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
Signed-off-by: Vinod Koul <vinod.koul@intel.com>
---
 sound/soc/intel/skylake/skl-pcm.c | 585 ++++++++++++++++++++++++++++++++++++++
 sound/soc/intel/skylake/skl.h     |  74 +++++
 2 files changed, 659 insertions(+)
 create mode 100644 sound/soc/intel/skylake/skl-pcm.c
 create mode 100644 sound/soc/intel/skylake/skl.h

diff --git a/sound/soc/intel/skylake/skl-pcm.c b/sound/soc/intel/skylake/skl-pcm.c
new file mode 100644
index 000000000000..42cafab47915
--- /dev/null
+++ b/sound/soc/intel/skylake/skl-pcm.c
@@ -0,0 +1,585 @@
+/*
+ *  skl-pcm.c -ASoC HDA Platform driver file implementing PCM functionality
+ *
+ *  Copyright (C) 2014-2015 Intel Corp
+ *  Author:  Jeeja KP <jeeja.kp@intel.com>
+ *
+ *  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; version 2 of the License.
+ *
+ *  This program is distributed in the hope that it will be useful, but
+ *  WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ *  General Public License for more details.
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ */
+#include <linux/pci.h>
+#include <linux/pm_runtime.h>
+#include <sound/core.h>
+#include <sound/pcm.h>
+#include <sound/pcm_params.h>
+#include <sound/soc.h>
+#include "skl.h"
+
+#define HDA_MONO 1
+#define HDA_STEREO 2
+
+static struct snd_pcm_hardware azx_pcm_hw = {
+	.info =			(SNDRV_PCM_INFO_MMAP |
+				 SNDRV_PCM_INFO_INTERLEAVED |
+				 SNDRV_PCM_INFO_BLOCK_TRANSFER |
+				 SNDRV_PCM_INFO_MMAP_VALID |
+				 SNDRV_PCM_INFO_PAUSE |
+				 SNDRV_PCM_INFO_SYNC_START |
+				 SNDRV_PCM_INFO_HAS_WALL_CLOCK | /* legacy */
+				 SNDRV_PCM_INFO_HAS_LINK_ATIME |
+				 SNDRV_PCM_INFO_NO_PERIOD_WAKEUP),
+	.formats =		SNDRV_PCM_FMTBIT_S16_LE,
+	.rates =		SNDRV_PCM_RATE_48000,
+	.rate_min =		48000,
+	.rate_max =		48000,
+	.channels_min =		2,
+	.channels_max =		2,
+	.buffer_bytes_max =	AZX_MAX_BUF_SIZE,
+	.period_bytes_min =	128,
+	.period_bytes_max =	AZX_MAX_BUF_SIZE / 2,
+	.periods_min =		2,
+	.periods_max =		AZX_MAX_FRAG,
+	.fifo_size =		0,
+};
+
+static inline
+struct hdac_ext_stream *get_hdac_stream(struct snd_pcm_substream *substream)
+{
+	return substream->runtime->private_data;
+}
+
+static struct hdac_ext_bus *get_bus_ctx(struct snd_pcm_substream *substream)
+{
+	struct hdac_ext_stream *stream = get_hdac_stream(substream);
+	struct hdac_stream *hstream = hdac_stream(stream);
+	struct hdac_bus *bus = hstream->bus;
+
+	return hbus_to_ebus(bus);
+}
+
+static int skl_substream_alloc_pages(struct hdac_ext_bus *ebus,
+				 struct snd_pcm_substream *substream,
+				 size_t size)
+{
+	struct hdac_ext_stream *stream = get_hdac_stream(substream);
+	int ret;
+
+	hdac_stream(stream)->bufsize = 0;
+	hdac_stream(stream)->period_bytes = 0;
+	hdac_stream(stream)->format_val = 0;
+
+	ret = snd_pcm_lib_malloc_pages(substream, size);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static int skl_substream_free_pages(struct hdac_bus *bus,
+				struct snd_pcm_substream *substream)
+{
+	return snd_pcm_lib_free_pages(substream);
+}
+
+static void skl_set_pcm_constrains(struct hdac_ext_bus *ebus,
+				 struct snd_pcm_runtime *runtime)
+{
+	snd_pcm_hw_constraint_integer(runtime, SNDRV_PCM_HW_PARAM_PERIODS);
+
+	/* avoid wrap-around with wall-clock */
+	snd_pcm_hw_constraint_minmax(runtime, SNDRV_PCM_HW_PARAM_BUFFER_TIME,
+				     20, 178000000);
+}
+
+static int skl_pcm_open(struct snd_pcm_substream *substream,
+		struct snd_soc_dai *dai)
+{
+	struct hdac_ext_bus *ebus = dev_get_drvdata(dai->dev);
+	struct hdac_ext_stream *stream;
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	struct skl_dma_params *dma_params;
+
+	dev_dbg(dai->dev, "%s: %s\n", __func__, dai->name);
+	pm_runtime_get_sync(dai->dev);
+
+	stream = snd_hdac_ext_stream_assign(ebus, substream,
+					HDAC_EXT_STREAM_TYPE_COUPLED);
+	if (stream == NULL)
+		return -EBUSY;
+
+	skl_set_pcm_constrains(ebus, runtime);
+
+	/*
+	 * disable WALLCLOCK timestamps for capture streams
+	 * until we figure out how to handle digital inputs
+	 */
+	if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) {
+		runtime->hw.info &= ~SNDRV_PCM_INFO_HAS_WALL_CLOCK; /* legacy */
+		runtime->hw.info &= ~SNDRV_PCM_INFO_HAS_LINK_ATIME;
+	}
+
+	runtime->private_data = stream;
+
+	dma_params = kzalloc(sizeof(*dma_params), GFP_KERNEL);
+	if (!dma_params)
+		return -ENOMEM;
+
+	dma_params->stream_tag = hdac_stream(stream)->stream_tag;
+	snd_soc_dai_set_dma_data(dai, substream, (void *)dma_params);
+
+	dev_dbg(dai->dev, "stream tag set in dma params=%d\n",
+				 dma_params->stream_tag);
+	snd_pcm_set_sync(substream);
+
+	return 0;
+}
+
+static int skl_get_format(struct snd_pcm_substream *substream,
+		struct snd_soc_dai *dai)
+{
+	struct snd_soc_pcm_runtime *rtd = snd_pcm_substream_chip(substream);
+	struct skl_dma_params *dma_params;
+	int format_val = 0;
+	struct snd_soc_dai *codec_dai = rtd->codec_dai;
+
+	dma_params  = (struct skl_dma_params *)
+			snd_soc_dai_get_dma_data(codec_dai, substream);
+
+	if (!dma_params)
+		format_val = dma_params->format;
+
+	return format_val;
+}
+
+static int skl_pcm_prepare(struct snd_pcm_substream *substream,
+		struct snd_soc_dai *dai)
+{
+	struct hdac_ext_stream *stream = get_hdac_stream(substream);
+	unsigned int format_val;
+	int err;
+
+	dev_dbg(dai->dev, "%s: %s\n", __func__, dai->name);
+	if (hdac_stream(stream)->prepared) {
+		dev_dbg(dai->dev, "already stream is prepared - returning\n");
+		return 0;
+	}
+
+	format_val = skl_get_format(substream, dai);
+	dev_dbg(dai->dev, "stream_tag=%d formatvalue=%d\n",
+				hdac_stream(stream)->stream_tag, format_val);
+	snd_hdac_stream_reset(hdac_stream(stream));
+
+	err = snd_hdac_stream_set_params(hdac_stream(stream), format_val);
+
+	if (err < 0)
+		return err;
+
+	snd_hdac_stream_setup(hdac_stream(stream));
+	hdac_stream(stream)->prepared = 1;
+
+	return err;
+}
+
+static int skl_pcm_hw_params(struct snd_pcm_substream *substream,
+				struct snd_pcm_hw_params *params,
+				struct snd_soc_dai *dai)
+{
+	struct hdac_ext_bus *ebus = dev_get_drvdata(dai->dev);
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	int ret;
+
+	dev_dbg(dai->dev, "%s: %s\n", __func__, dai->name);
+	ret = skl_substream_alloc_pages(ebus, substream,
+					  params_buffer_bytes(params));
+
+	if (ret < 0)
+		return ret;
+
+	if (substream->runtime->dma_area)
+		memset(substream->runtime->dma_area, 0,
+				params_buffer_bytes(params));
+
+	dev_dbg(dai->dev, "format_val, rate=%d, ch=%d, format=%d\n",
+			runtime->rate, runtime->channels, runtime->format);
+
+	return ret;
+}
+
+static void skl_pcm_close(struct snd_pcm_substream *substream,
+		struct snd_soc_dai *dai)
+{
+	struct hdac_ext_stream *stream = get_hdac_stream(substream);
+	struct skl_dma_params *dma_params = NULL;
+
+	dev_dbg(dai->dev, "%s: %s\n", __func__, dai->name);
+	snd_hdac_ext_stream_release(stream, HDAC_EXT_STREAM_TYPE_COUPLED);
+
+	dma_params  = (struct skl_dma_params *)
+			snd_soc_dai_get_dma_data(dai, substream);
+
+	pm_runtime_mark_last_busy(dai->dev);
+	pm_runtime_put_autosuspend(dai->dev);
+	kfree(dma_params);
+}
+
+static int skl_pcm_hw_free(struct snd_pcm_substream *substream,
+		struct snd_soc_dai *dai)
+{
+	struct hdac_ext_bus *ebus = dev_get_drvdata(dai->dev);
+	struct hdac_ext_stream *stream = get_hdac_stream(substream);
+
+	dev_dbg(dai->dev, "%s: %s\n", __func__, dai->name);
+
+	snd_hdac_stream_cleanup(hdac_stream(stream));
+	hdac_stream(stream)->prepared = 0;
+
+	return skl_substream_free_pages(ebus_to_hbus(ebus), substream);
+}
+
+static struct snd_soc_dai_ops skl_pcm_dai_ops = {
+	.startup = skl_pcm_open,
+	.shutdown = skl_pcm_close,
+	.prepare = skl_pcm_prepare,
+	.hw_params = skl_pcm_hw_params,
+	.hw_free = skl_pcm_hw_free,
+};
+
+static struct snd_soc_dai_driver skl_platform_dai[] = {
+{
+	.name = "System Pin",
+	.ops = &skl_pcm_dai_ops,
+	.playback = {
+		.stream_name = "System Playback",
+		.channels_min = HDA_MONO,
+		.channels_max = HDA_STEREO,
+		.rates = SNDRV_PCM_RATE_48000 | SNDRV_PCM_RATE_16000 | SNDRV_PCM_RATE_8000,
+		.formats = SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S24_LE,
+	},
+	.capture = {
+		.stream_name = "System Capture",
+		.channels_min = HDA_MONO,
+		.channels_max = HDA_STEREO,
+		.rates = SNDRV_PCM_RATE_48000 | SNDRV_PCM_RATE_16000,
+		.formats = SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S24_LE,
+	},
+},
+{
+	.name = "Deepbuffer Pin",
+	.ops = &skl_pcm_dai_ops,
+	.playback = {
+		.stream_name = "Deepbuffer Playback",
+		.channels_min = HDA_STEREO,
+		.channels_max = HDA_STEREO,
+		.rates = SNDRV_PCM_RATE_48000,
+		.formats = SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S24_LE,
+	},
+},
+{
+	.name = "LowLatency Pin",
+	.ops = &skl_pcm_dai_ops,
+	.playback = {
+		.stream_name = "Low Latency Playback",
+		.channels_min = HDA_STEREO,
+		.channels_max = HDA_STEREO,
+		.rates = SNDRV_PCM_RATE_48000,
+		.formats = SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S24_LE,
+	},
+},
+};
+
+static int skl_platform_open(struct snd_pcm_substream *substream)
+{
+	struct snd_pcm_runtime *runtime;
+	struct snd_soc_pcm_runtime *rtd = substream->private_data;
+	struct snd_soc_dai_link *dai_link = rtd->dai_link;
+
+	dev_dbg(rtd->cpu_dai->dev, "In %s:%s\n", __func__,
+					dai_link->cpu_dai_name);
+
+	runtime = substream->runtime;
+	snd_soc_set_runtime_hwparams(substream, &azx_pcm_hw);
+	return 0;
+}
+
+static int skl_platform_pcm_trigger(struct snd_pcm_substream *substream,
+					int cmd)
+{
+	struct hdac_ext_bus *ebus = get_bus_ctx(substream);
+	struct hdac_bus *bus = ebus_to_hbus(ebus);
+	struct hdac_ext_stream *stream;
+	struct snd_pcm_substream *s;
+	bool start;
+	int sbits = 0;
+	unsigned long cookie;
+	struct hdac_stream *hstr;
+
+	stream = get_hdac_stream(substream);
+	hstr = hdac_stream(stream);
+
+	dev_dbg(bus->dev, "In %s cmd=%d\n", __func__, cmd);
+
+	if (!hstr->prepared)
+		return -EPIPE;
+
+	switch (cmd) {
+	case SNDRV_PCM_TRIGGER_START:
+	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
+	case SNDRV_PCM_TRIGGER_RESUME:
+		start = true;
+		break;
+
+	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
+	case SNDRV_PCM_TRIGGER_SUSPEND:
+	case SNDRV_PCM_TRIGGER_STOP:
+		start = false;
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	snd_pcm_group_for_each_entry(s, substream) {
+		if (s->pcm->card != substream->pcm->card)
+			continue;
+		stream = get_hdac_stream(s);
+		sbits |= 1 << hdac_stream(stream)->index;
+		snd_pcm_trigger_done(s, substream);
+	}
+
+	spin_lock_irqsave(&bus->reg_lock, cookie);
+
+	/* first, set SYNC bits of corresponding streams */
+	snd_hdac_stream_sync_trigger(hstr, true, sbits, AZX_REG_SSYNC);
+
+	snd_pcm_group_for_each_entry(s, substream) {
+		if (s->pcm->card != substream->pcm->card)
+			continue;
+		stream = get_hdac_stream(s);
+		if (start)
+			snd_hdac_stream_start(hdac_stream(stream), true);
+		else
+			snd_hdac_stream_stop(hdac_stream(stream));
+	}
+	spin_unlock_irqrestore(&bus->reg_lock, cookie);
+
+	snd_hdac_stream_sync(hstr, start, sbits);
+
+	spin_lock_irqsave(&bus->reg_lock, cookie);
+
+	/* reset SYNC bits */
+	snd_hdac_stream_sync_trigger(hstr, false, sbits, AZX_REG_SSYNC);
+	if (start)
+		snd_hdac_stream_timecounter_init(hstr, sbits);
+	spin_unlock_irqrestore(&bus->reg_lock, cookie);
+	return 0;
+}
+
+/* calculate runtime delay from LPIB */
+static int skl_get_delay_from_lpib(struct hdac_ext_bus *ebus,
+				struct hdac_ext_stream *sstream,
+				unsigned int pos)
+{
+	struct hdac_bus *bus = ebus_to_hbus(ebus);
+	struct hdac_stream *hstream = hdac_stream(sstream);
+	struct snd_pcm_substream *substream = hstream->substream;
+	int stream = substream->stream;
+	unsigned int lpib_pos = snd_hdac_stream_get_pos_lpib(hstream);
+	int delay;
+
+	if (stream == SNDRV_PCM_STREAM_PLAYBACK)
+		delay = pos - lpib_pos;
+	else
+		delay = lpib_pos - pos;
+	if (delay < 0) {
+		if (delay >= hstream->delay_negative_threshold)
+			delay = 0;
+		else
+			delay += hstream->bufsize;
+	}
+
+	if (delay >= hstream->period_bytes) {
+		dev_info(bus->dev,
+			 "Unstable LPIB (%d >= %d); disabling LPIB delay counting\n",
+			 delay, hstream->period_bytes);
+		delay = 0;
+	}
+
+	return bytes_to_frames(substream->runtime, delay);
+}
+
+static unsigned int skl_get_position(struct hdac_ext_stream *hstream,
+					int codec_delay)
+{
+	struct hdac_stream *hstr = hdac_stream(hstream);
+	struct snd_pcm_substream *substream = hstr->substream;
+	struct hdac_ext_bus *ebus = get_bus_ctx(substream);
+	unsigned int pos;
+	int delay = 0;
+
+	/* use the position buffer as default */
+	pos = snd_hdac_stream_get_pos_posbuf(hdac_stream(hstream));
+
+	if (pos >= hdac_stream(hstream)->bufsize)
+		pos = 0;
+
+	if (substream->runtime) {
+		delay = skl_get_delay_from_lpib(ebus, hstream, pos)
+						 + codec_delay;
+		substream->runtime->delay += delay;
+	}
+
+	return pos;
+}
+
+static snd_pcm_uframes_t skl_platform_pcm_pointer
+			(struct snd_pcm_substream *substream)
+{
+	struct hdac_ext_stream *hstream = get_hdac_stream(substream);
+
+	return bytes_to_frames(substream->runtime,
+			       skl_get_position(hstream, 0));
+}
+
+static u64 skl_adjust_codec_delay(struct snd_pcm_substream *substream,
+				u64 nsec)
+{
+	struct snd_soc_pcm_runtime *rtd = snd_pcm_substream_chip(substream);
+	struct snd_soc_dai *codec_dai = rtd->codec_dai;
+	u64 codec_frames, codec_nsecs;
+
+	if (!codec_dai->driver->ops->delay)
+		return nsec;
+
+	codec_frames = codec_dai->driver->ops->delay(substream, codec_dai);
+	codec_nsecs = div_u64(codec_frames * 1000000000LL,
+			      substream->runtime->rate);
+
+	if (substream->stream == SNDRV_PCM_STREAM_CAPTURE)
+		return nsec + codec_nsecs;
+
+	return (nsec > codec_nsecs) ? nsec - codec_nsecs : 0;
+}
+
+static int skl_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 hdac_ext_stream *sstream = get_hdac_stream(substream);
+	struct hdac_stream *hstr = hdac_stream(sstream);
+	u64 nsec;
+
+	if ((substream->runtime->hw.info & SNDRV_PCM_INFO_HAS_LINK_ATIME) &&
+		(audio_tstamp_config->type_requested == SNDRV_PCM_AUDIO_TSTAMP_TYPE_LINK)) {
+
+		snd_pcm_gettime(substream->runtime, system_ts);
+
+		nsec = timecounter_read(&hstr->tc);
+		nsec = div_u64(nsec, 3); /* can be optimized */
+		if (audio_tstamp_config->report_delay)
+			nsec = skl_adjust_codec_delay(substream, nsec);
+
+		*audio_ts = ns_to_timespec(nsec);
+
+		audio_tstamp_report->actual_type = SNDRV_PCM_AUDIO_TSTAMP_TYPE_LINK;
+		audio_tstamp_report->accuracy_report = 1; /* rest of struct is valid */
+		audio_tstamp_report->accuracy = 42; /* 24MHzWallClk == 42ns resolution */
+
+	} else
+		audio_tstamp_report->actual_type = SNDRV_PCM_AUDIO_TSTAMP_TYPE_DEFAULT;
+
+	return 0;
+}
+
+static struct snd_pcm_ops skl_platform_ops = {
+	.open = skl_platform_open,
+	.ioctl = snd_pcm_lib_ioctl,
+	.trigger = skl_platform_pcm_trigger,
+	.pointer = skl_platform_pcm_pointer,
+	.get_time_info =  skl_get_time_info,
+	.mmap = snd_pcm_lib_default_mmap,
+	.page = snd_pcm_sgbuf_ops_page,
+};
+
+static void skl_pcm_free(struct snd_pcm *pcm)
+{
+	snd_pcm_lib_preallocate_free_for_all(pcm);
+}
+
+#define MAX_PREALLOC_SIZE	(32 * 1024 * 1024)
+
+static int skl_pcm_new(struct snd_soc_pcm_runtime *rtd)
+{
+	struct snd_soc_dai *dai = rtd->cpu_dai;
+	struct hdac_ext_bus *ebus = dev_get_drvdata(dai->dev);
+	struct snd_pcm *pcm = rtd->pcm;
+	unsigned int size;
+	int retval = 0;
+	struct skl *skl = ebus_to_skl(ebus);
+
+	if (dai->driver->playback.channels_min ||
+		dai->driver->capture.channels_min) {
+		/* buffer pre-allocation */
+		size = CONFIG_SND_SKL_PREALLOC_SIZE * 1024;
+		if (size > MAX_PREALLOC_SIZE)
+			size = MAX_PREALLOC_SIZE;
+		retval = snd_pcm_lib_preallocate_pages_for_all(pcm,
+						SNDRV_DMA_TYPE_DEV_SG,
+						snd_dma_pci_data(skl->pci),
+						size, MAX_PREALLOC_SIZE);
+		if (retval) {
+			dev_err(dai->dev, "dma buffer allocationf fail\n");
+			return retval;
+		}
+	}
+	return retval;
+}
+
+static struct snd_soc_platform_driver skl_platform_drv  = {
+	.ops		= &skl_platform_ops,
+	.pcm_new	= skl_pcm_new,
+	.pcm_free	= skl_pcm_free,
+};
+
+static const struct snd_soc_component_driver skl_component = {
+	.name           = "pcm",
+};
+
+int skl_platform_register(struct device *dev)
+{
+	int ret;
+
+	ret = snd_soc_register_platform(dev, &skl_platform_drv);
+	if (ret) {
+		dev_err(dev, "soc platform registration failed %d\n", ret);
+		return ret;
+	}
+	ret = snd_soc_register_component(dev, &skl_component,
+				skl_platform_dai,
+				ARRAY_SIZE(skl_platform_dai));
+	if (ret) {
+		dev_err(dev, "soc component registration failed %d\n", ret);
+		snd_soc_unregister_platform(dev);
+	}
+
+	return ret;
+
+}
+
+int skl_platform_unregister(struct device *dev)
+{
+	snd_soc_unregister_component(dev);
+	snd_soc_unregister_platform(dev);
+	return 0;
+}
diff --git a/sound/soc/intel/skylake/skl.h b/sound/soc/intel/skylake/skl.h
new file mode 100644
index 000000000000..0fe5e848a2ff
--- /dev/null
+++ b/sound/soc/intel/skylake/skl.h
@@ -0,0 +1,74 @@
+/*
+ *  skl.h - HD Audio skylake defintions.
+ *
+ *  Copyright (C) 2015 Intel Corp
+ *  Author: Jeeja KP <jeeja.kp@intel.com>
+ *  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; version 2 of the License.
+ *
+ *  This program is distributed in the hope that it will be useful, but
+ *  WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ *  General Public License for more details.
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ */
+
+#ifndef __SOUND_SOC_SKL_H
+#define __SOUND_SOC_SKL_H
+
+#include <sound/core.h>
+#include <sound/initval.h>
+#include <sound/hda_register.h>
+#include <sound/hdaudio.h>
+#include <sound/hdaudio_ext.h>
+
+#define SKL_SUSPEND_DELAY 2000
+
+/* Vendor Specific Registers */
+#define AZX_REG_VS_EM1			0x1000
+#define AZX_REG_VS_INRC			0x1004
+#define AZX_REG_VS_OUTRC		0x1008
+#define AZX_REG_VS_FIFOTRK		0x100C
+#define AZX_REG_VS_FIFOTRK2		0x1010
+#define AZX_REG_VS_EM2			0x1030
+#define AZX_REG_VS_EM3L			0x1038
+#define AZX_REG_VS_EM3U			0x103C
+#define AZX_REG_VS_EM4L			0x1040
+#define AZX_REG_VS_EM4U			0x1044
+#define AZX_REG_VS_LTRC			0x1048
+#define AZX_REG_VS_D0I3C		0x104A
+#define AZX_REG_VS_PCE			0x104B
+#define AZX_REG_VS_L2MAGC		0x1050
+#define AZX_REG_VS_L2LAHPT		0x1054
+#define AZX_REG_VS_SDXDPIB_XBASE	0x1084
+#define AZX_REG_VS_SDXDPIB_XINTERVAL	0x20
+#define AZX_REG_VS_SDXEFIFOS_XBASE	0x1094
+#define AZX_REG_VS_SDXEFIFOS_XINTERVAL	0x20
+
+struct skl {
+	struct hdac_ext_bus ebus;
+	struct pci_dev *pci;
+
+	unsigned int init_failed:1; /* delayed init failed */
+	struct platform_device *dmic_dev;
+};
+
+#define skl_to_ebus(s)	(&(s)->ebus)
+#define ebus_to_skl(sbus) \
+	container_of(sbus, struct skl, sbus)
+
+/* to pass dai dma data */
+struct skl_dma_params {
+	u32 format;
+	u8 stream_tag;
+};
+
+int skl_platform_unregister(struct device *dev);
+int skl_platform_register(struct device *dev);
+
+#endif /* __SOUND_SOC_SKL_H */
-- 
1.9.1

[PATCH v5 2/4] ASoC: Intel - add Skylake HDA audio driver

[Vinod Koul]

From: Jeeja KP <jeeja.kp@intel.com>

This patch follows up by adding the HDA controller operations. This
code is mostly derived from Intel HDA PCI driver without legacy bits

Signed-off-by: Jeeja KP <jeeja.kp@intel.com>
Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
Signed-off-by: Vinod Koul <vinod.koul@intel.com>
---
 sound/soc/intel/skylake/skl.c | 635 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 635 insertions(+)
 create mode 100644 sound/soc/intel/skylake/skl.c

diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
new file mode 100644
index 000000000000..c6f4902af9d0
--- /dev/null
+++ b/sound/soc/intel/skylake/skl.c
@@ -0,0 +1,635 @@
+/*
+ *  skl.c - Implementation of ASoC Intel SKL HD Audio driver
+ *
+ *  Copyright (C) 2014-2015 Intel Corp
+ *  Author: Jeeja KP <jeeja.kp@intel.com>
+ *
+ *  Derived mostly from Intel HDA driver with following copyrights:
+ *  Copyright (c) 2004 Takashi Iwai <tiwai@suse.de>
+ *                     PeiSen Hou <pshou@realtek.com.tw>
+ *  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; version 2 of the License.
+ *
+ *  This program is distributed in the hope that it will be useful, but
+ *  WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ *  General Public License for more details.
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ */
+
+#include <linux/delay.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/pci.h>
+#include <linux/mutex.h>
+#include <linux/io.h>
+#include <linux/pm_runtime.h>
+
+#include <linux/platform_device.h>
+#include <sound/core.h>
+#include <sound/pcm.h>
+#include <sound/hda_register.h>
+#include <sound/hdaudio.h>
+#include "skl.h"
+
+/*
+ * initialize the PCI registers
+ */
+static void skl_update_pci_byte(struct pci_dev *pci, unsigned int reg,
+			    unsigned char mask, unsigned char val)
+{
+	unsigned char data;
+
+	pci_read_config_byte(pci, reg, &data);
+	data &= ~mask;
+	data |= (val & mask);
+	pci_write_config_byte(pci, reg, data);
+}
+
+static void skl_init_pci(struct skl *skl)
+{
+	struct hdac_ext_bus *ebus = &skl->ebus;
+
+	/*
+	 * Clear bits 0-2 of PCI register TCSEL (at offset 0x44)
+	 * TCSEL == Traffic Class Select Register, which sets PCI express QOS
+	 * Ensuring these bits are 0 clears playback static on some HD Audio
+	 * codecs.
+	 * The PCI register TCSEL is defined in the Intel manuals.
+	 */
+	dev_dbg(ebus_to_hbus(ebus)->dev, "Clearing TCSEL\n");
+	skl_update_pci_byte(skl->pci, AZX_PCIREG_TCSEL, 0x07, 0);
+}
+
+/* called from IRQ */
+static void skl_stream_update(struct hdac_bus *bus, struct hdac_stream *hstr)
+{
+	snd_pcm_period_elapsed(hstr->substream);
+}
+
+static irqreturn_t skl_interrupt(int irq, void *dev_id)
+{
+	struct hdac_ext_bus *ebus = dev_id;
+	struct hdac_bus *bus = ebus_to_hbus(ebus);
+	u32 status;
+
+#ifdef CONFIG_PM
+	if (!pm_runtime_active(bus->dev))
+		return IRQ_NONE;
+#endif
+
+	spin_lock(&bus->reg_lock);
+
+	status = snd_hdac_chip_readl(bus, INTSTS);
+	if (status == 0 || status == 0xffffffff) {
+		spin_unlock(&bus->reg_lock);
+		return IRQ_NONE;
+	}
+
+	/* clear rirb int */
+	status = snd_hdac_chip_readb(bus, RIRBSTS);
+	if (status & RIRB_INT_MASK) {
+		if (status & RIRB_INT_RESPONSE)
+			snd_hdac_bus_update_rirb(bus);
+		snd_hdac_chip_writeb(bus, RIRBSTS, RIRB_INT_MASK);
+	}
+
+	spin_unlock(&bus->reg_lock);
+
+	return snd_hdac_chip_readl(bus, INTSTS) ? IRQ_WAKE_THREAD : IRQ_HANDLED;
+}
+
+static irqreturn_t skl_threaded_handler(int irq, void *dev_id)
+{
+	struct hdac_ext_bus *ebus = dev_id;
+	struct hdac_bus *bus = ebus_to_hbus(ebus);
+	u32 status;
+
+	status = snd_hdac_chip_readl(bus, INTSTS);
+
+	snd_hdac_bus_handle_stream_irq(bus, status, skl_stream_update);
+
+	return IRQ_HANDLED;
+}
+
+/* initialize SD streams, use seprate streeam tag for PB and CP */
+static int skl_init_stream(struct hdac_ext_bus *ebus, int start_idx,
+				int num_stream, int dir)
+{
+	int stream_tag = 0;
+	int i, tag, idx = start_idx;
+
+	for (i = 0; i < num_stream; i++) {
+		struct hdac_ext_stream *stream =
+				kzalloc(sizeof(*stream), GFP_KERNEL);
+		if (!stream)
+			return -ENOMEM;
+		tag = ++stream_tag;
+		snd_hdac_ext_stream_init(ebus, stream, idx, dir, tag);
+		idx++;
+	}
+
+	return 0;
+}
+
+static void skl_free_streams(struct hdac_ext_bus *ebus)
+{
+	struct hdac_stream *s;
+	struct hdac_ext_stream *stream;
+	struct hdac_bus *bus = ebus_to_hbus(ebus);
+
+	while (!list_empty(&bus->stream_list)) {
+		s = list_first_entry(&bus->stream_list, struct hdac_stream, list);
+		stream = stream_to_hdac_ext_stream(s);
+		list_del(&s->list);
+		kfree(stream);
+	}
+}
+
+static void skl_free_hda_links(struct hdac_ext_bus *ebus)
+{
+	struct hdac_ext_link *l;
+
+	while (!list_empty(&ebus->hlink_list)) {
+		l = list_first_entry(&ebus->hlink_list, struct hdac_ext_link, list);
+		list_del(&l->list);
+		kfree(l);
+	}
+}
+
+static int skl_acquire_irq(struct hdac_ext_bus *ebus, int do_disconnect)
+{
+	struct skl *skl = ebus_to_skl(ebus);
+	struct hdac_bus *bus = ebus_to_hbus(ebus);
+	int ret = 0;
+
+	ret = request_threaded_irq(skl->pci->irq, skl_interrupt,
+			skl_threaded_handler,
+			IRQF_SHARED,
+			KBUILD_MODNAME, ebus);
+	if (ret) {
+		dev_err(bus->dev,
+			"unable to grab IRQ %d, disabling device\n",
+			skl->pci->irq);
+		return ret;
+	}
+
+	bus->irq = skl->pci->irq;
+	pci_intx(skl->pci, 1);
+	return ret;
+}
+
+#ifdef CONFIG_PM_SLEEP
+/*
+ * power management
+ */
+static int skl_suspend(struct device *dev)
+{
+	struct pci_dev *pci = to_pci_dev(dev);
+	struct hdac_ext_bus *ebus = pci_get_drvdata(pci);
+	struct hdac_bus *bus = ebus_to_hbus(ebus);
+
+	snd_hdac_bus_stop_chip(bus);
+	snd_hdac_bus_enter_link_reset(bus);
+	if (bus->irq >= 0) {
+		free_irq(bus->irq, bus);
+		bus->irq = -1;
+	}
+
+	return 0;
+}
+
+static int skl_resume(struct device *dev)
+{
+	struct pci_dev *pci = to_pci_dev(dev);
+	struct hdac_ext_bus *ebus = pci_get_drvdata(pci);
+	struct hdac_bus *bus = ebus_to_hbus(ebus);
+	struct skl *hda = ebus_to_skl(ebus);
+
+	if (skl_acquire_irq(ebus, 1) < 0)
+		return -EIO;
+	skl_init_pci(hda);
+
+	snd_hdac_bus_init_chip(bus, 1);
+	return 0;
+}
+#endif /* CONFIG_PM_SLEEP */
+
+#ifdef CONFIG_PM
+static int skl_runtime_suspend(struct device *dev)
+{
+	struct pci_dev *pci = to_pci_dev(dev);
+	struct hdac_ext_bus *ebus = pci_get_drvdata(pci);
+	struct hdac_bus *bus = ebus_to_hbus(ebus);
+
+	dev_dbg(bus->dev, "in %s\n", __func__);
+
+	/* enable controller wake up event */
+	snd_hdac_chip_updatew(bus, WAKEEN, 0, STATESTS_INT_MASK);
+
+	snd_hdac_bus_stop_chip(bus);
+	snd_hdac_bus_enter_link_reset(bus);
+	return 0;
+}
+
+static int skl_runtime_resume(struct device *dev)
+{
+	struct pci_dev *pci = to_pci_dev(dev);
+	struct hdac_ext_bus *ebus = pci_get_drvdata(pci);
+	struct hdac_bus *bus = ebus_to_hbus(ebus);
+	struct skl *hda = ebus_to_skl(ebus);
+	int status;
+
+	dev_dbg(bus->dev, "in %s\n", __func__);
+
+	/* Read STATESTS before controller reset */
+	status = snd_hdac_chip_readw(bus, STATESTS);
+
+	skl_init_pci(hda);
+	snd_hdac_bus_init_chip(bus, true);
+	/* disable controller Wake Up event */
+	snd_hdac_chip_updatew(bus, WAKEEN, STATESTS_INT_MASK, 0);
+	return 0;
+}
+#endif /* CONFIG_PM */
+
+static const struct dev_pm_ops skl_pm = {
+	SET_SYSTEM_SLEEP_PM_OPS(skl_suspend, skl_resume)
+	SET_RUNTIME_PM_OPS(skl_runtime_suspend, skl_runtime_resume, NULL)
+};
+
+/*
+ * destructor
+ */
+static int skl_free(struct hdac_ext_bus *ebus)
+{
+	struct skl *skl  = ebus_to_skl(ebus);
+	struct hdac_bus *bus = ebus_to_hbus(ebus);
+
+	skl->init_failed = 1; /* to be sure */
+
+	snd_hdac_ext_stop_streams(ebus);
+
+	if (bus->irq >= 0)
+		free_irq(bus->irq, (void *)bus);
+	if (bus->remap_addr)
+		iounmap(bus->remap_addr);
+
+	snd_hdac_bus_free_stream_pages(bus);
+	skl_free_streams(ebus);
+	skl_free_hda_links(ebus);
+	pci_release_regions(skl->pci);
+	pci_disable_device(skl->pci);
+
+	snd_hdac_ext_bus_exit(ebus);
+	return 0;
+}
+
+static int skl_dmic_device_register(struct skl *skl)
+{
+	struct hdac_bus *bus = ebus_to_hbus(&skl->ebus);
+	struct platform_device *pdev;
+	int ret;
+
+	pdev = platform_device_alloc("dmic-codec", -1);
+	if (!pdev) {
+		dev_err(bus->dev, "failed to allocate dmic device\n");
+		return -1;
+	}
+
+	ret = platform_device_add(pdev);
+	if (ret) {
+		dev_err(bus->dev, "failed to add hda codec device\n");
+		platform_device_put(pdev);
+		return -1;
+	}
+	skl->dmic_dev = pdev;
+	return 0;
+}
+
+static void skl_dmic_device_unregister(struct skl *skl)
+{
+
+	if (skl->dmic_dev)
+		platform_device_unregister(skl->dmic_dev);
+}
+
+/*
+ * Probe the given codec address
+ */
+static int probe_codec(struct hdac_ext_bus *ebus, int addr)
+{
+	struct hdac_bus *bus = ebus_to_hbus(ebus);
+	unsigned int cmd = (addr << 28) | (AC_NODE_ROOT << 20) |
+		(AC_VERB_PARAMETERS << 8) | AC_PAR_VENDOR_ID;
+	unsigned int res;
+
+	mutex_lock(&bus->cmd_mutex);
+	snd_hdac_bus_send_cmd(bus, cmd);
+	snd_hdac_bus_get_response(bus, addr, &res);
+	mutex_unlock(&bus->cmd_mutex);
+	if (res == -1)
+		return -EIO;
+	dev_dbg(bus->dev, "codec #%d probed OK\n", addr);
+
+	return snd_hdac_ext_bus_device_init(ebus, addr);
+}
+
+/* Codec initialization */
+static int skl_codec_create(struct hdac_ext_bus *ebus)
+{
+	struct hdac_bus *bus = ebus_to_hbus(ebus);
+	int c, max_slots;
+
+	max_slots = HDA_MAX_CODECS;
+
+	/* First try to probe all given codec slots */
+	for (c = 0; c < max_slots; c++) {
+		if ((bus->codec_mask & (1 << c))) {
+			if (probe_codec(ebus, c) < 0) {
+				/*
+				 * Some BIOSen give you wrong codec addresses
+				 * that don't exist
+				 */
+				dev_warn(bus->dev,
+					 "Codec #%d probe error; disabling it...\n", c);
+				bus->codec_mask &= ~(1 << c);
+				/*
+				 * More badly, accessing to a non-existing
+				 * codec often screws up the controller bus,
+				 * and disturbs the further communications.
+				 * Thus if an error occurs during probing,
+				 * better to reset the controller bus to get
+				 * back to the sanity state.
+				 */
+				snd_hdac_bus_stop_chip(bus);
+				snd_hdac_bus_init_chip(bus, true);
+			}
+		}
+	}
+	return 0;
+}
+
+static const struct hdac_bus_ops bus_core_ops = {
+	.command = snd_hdac_bus_send_cmd,
+	.get_response = snd_hdac_bus_get_response,
+};
+
+/*
+ * constructor
+ */
+static int skl_create(struct pci_dev *pci,
+		      const struct hdac_io_ops *io_ops,
+		      struct skl **rskl)
+{
+	struct skl *skl;
+	struct hdac_ext_bus *ebus;
+
+	int err;
+
+	*rskl = NULL;
+
+	err = pci_enable_device(pci);
+	if (err < 0)
+		return err;
+
+	skl = devm_kzalloc(&pci->dev, sizeof(*skl), GFP_KERNEL);
+	if (!skl) {
+		pci_disable_device(pci);
+		return -ENOMEM;
+	}
+	ebus = &skl->ebus;
+	snd_hdac_ext_bus_init(ebus, &pci->dev, &bus_core_ops, io_ops);
+	ebus->bus.use_posbuf = 1;
+	skl->pci = pci;
+
+	ebus->bus.bdl_pos_adj = 0;
+
+	*rskl = skl;
+
+	return 0;
+}
+
+static int skl_first_init(struct hdac_ext_bus *ebus)
+{
+	struct skl *skl = ebus_to_skl(ebus);
+	struct hdac_bus *bus = ebus_to_hbus(ebus);
+	struct pci_dev *pci = skl->pci;
+	int err;
+	unsigned short gcap;
+	int cp_streams, pb_streams, start_idx;
+
+	err = pci_request_regions(pci, "Skylake HD audio");
+	if (err < 0)
+		return err;
+
+	bus->addr = pci_resource_start(pci, 0);
+	bus->remap_addr = pci_ioremap_bar(pci, 0);
+	if (bus->remap_addr == NULL) {
+		dev_err(bus->dev, "ioremap error\n");
+		return -ENXIO;
+	}
+
+	if (skl_acquire_irq(ebus, 0) < 0)
+		return -EBUSY;
+
+	pci_set_master(pci);
+	synchronize_irq(bus->irq);
+
+	gcap = snd_hdac_chip_readw(bus, GCAP);
+	dev_dbg(bus->dev, "chipset global capabilities = 0x%x\n", gcap);
+
+	/* allow 64bit DMA address if supported by H/W */
+	if (!dma_set_mask(bus->dev, DMA_BIT_MASK(64)))
+		dma_set_coherent_mask(bus->dev, DMA_BIT_MASK(64));
+	else {
+		dma_set_mask(bus->dev, DMA_BIT_MASK(32));
+		dma_set_coherent_mask(bus->dev, DMA_BIT_MASK(32));
+	}
+
+	/* read number of streams from GCAP register */
+	cp_streams = (gcap >> 8) & 0x0f;
+	pb_streams = (gcap >> 12) & 0x0f;
+
+	if (!pb_streams && !cp_streams)
+		return -EIO;
+
+	ebus->num_streams = cp_streams + pb_streams;
+
+	/* initialize streams */
+	skl_init_stream(ebus, 0, cp_streams, SNDRV_PCM_STREAM_CAPTURE);
+	start_idx = cp_streams;
+	skl_init_stream(ebus, start_idx, pb_streams, SNDRV_PCM_STREAM_PLAYBACK);
+
+	err = snd_hdac_bus_alloc_stream_pages(bus);
+	if (err < 0)
+		return err;
+
+	/* initialize chip */
+	skl_init_pci(skl);
+
+	snd_hdac_bus_init_chip(bus, true);
+
+	/* codec detection */
+	if (!bus->codec_mask) {
+		dev_err(bus->dev, "no codecs found!\n");
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
+/* PCI register access. */
+static void skl_pci_writel(u32 value, u32 __iomem *addr)
+{
+	writel(value, addr);
+}
+
+static u32 skl_pci_readl(u32 __iomem *addr)
+{
+	return readl(addr);
+}
+
+static void skl_pci_writew(u16 value, u16 __iomem *addr)
+{
+	writew(value, addr);
+}
+
+static u16 skl_pci_readw(u16 __iomem *addr)
+{
+	return readw(addr);
+}
+
+static void skl_pci_writeb(u8 value, u8 __iomem *addr)
+{
+	writeb(value, addr);
+}
+
+static u8 skl_pci_readb(u8 __iomem *addr)
+{
+	return readb(addr);
+}
+
+/* DMA page allocation helpers.  */
+static int skl_dma_alloc_pages(struct hdac_bus *bus, int type,
+			   size_t size, struct snd_dma_buffer *buf)
+{
+	return snd_dma_alloc_pages(type, bus->dev, size, buf);
+}
+
+static void skl_dma_free_pages(struct hdac_bus *bus, struct snd_dma_buffer *buf)
+{
+	snd_dma_free_pages(buf);
+}
+
+static const struct hdac_io_ops skl_io_ops = {
+	.reg_writel = skl_pci_writel,
+	.reg_readl = skl_pci_readl,
+	.reg_writew = skl_pci_writew,
+	.reg_readw = skl_pci_readw,
+	.reg_writeb = skl_pci_writeb,
+	.reg_readb = skl_pci_readb,
+	.dma_alloc_pages = skl_dma_alloc_pages,
+	.dma_free_pages = skl_dma_free_pages,
+};
+
+static int skl_probe(struct pci_dev *pci,
+		     const struct pci_device_id *pci_id)
+{
+	struct skl *skl;
+	struct hdac_ext_bus *ebus = NULL;
+	struct hdac_bus *bus = NULL;
+	int err;
+
+	err = skl_create(pci, &skl_io_ops, &skl);
+	if (err < 0)
+		return err;
+
+	ebus = &skl->ebus;
+	bus = ebus_to_hbus(ebus);
+
+	err = skl_first_init(ebus);
+	if (err < 0)
+		goto out_free;
+
+	pci_set_drvdata(skl->pci, ebus);
+
+	/* create device for soc dmic */
+	err = skl_dmic_device_register(skl);
+	if (err < 0)
+		goto out_free;
+
+	/* register platform dai and controls */
+	err = skl_platform_register(bus->dev);
+	if (err < 0)
+		goto out_dmic_free;
+
+	/* create codec instances */
+	err = skl_codec_create(ebus);
+	if (err < 0)
+		goto out_unregister;
+
+	pci_set_drvdata(skl->pci, ebus);
+
+	/*configure PM */
+	pm_runtime_set_autosuspend_delay(bus->dev, SKL_SUSPEND_DELAY);
+	pm_runtime_use_autosuspend(bus->dev);
+	pm_runtime_put_noidle(bus->dev);
+	pm_runtime_allow(bus->dev);
+
+	pci_set_drvdata(skl->pci, ebus);
+	return 0;
+
+out_unregister:
+	skl_platform_unregister(bus->dev);
+out_dmic_free:
+	skl_dmic_device_unregister(skl);
+out_free:
+	skl->init_failed = 1;
+	skl_free(ebus);
+	pci_set_drvdata(skl->pci, NULL);
+	return err;
+}
+
+static void skl_remove(struct pci_dev *pci)
+{
+	struct hdac_ext_bus *ebus = pci_get_drvdata(pci);
+	struct skl *skl = ebus_to_skl(ebus);
+
+	if (pci_dev_run_wake(pci))
+		pm_runtime_get_noresume(&pci->dev);
+	pci_dev_put(pci);
+	skl_platform_unregister(&pci->dev);
+	skl_dmic_device_unregister(skl);
+	skl_free(ebus);
+	dev_set_drvdata(&pci->dev, NULL);
+}
+
+/* PCI IDs */
+static const struct pci_device_id skl_ids[] = {
+	/* Sunrise Point-LP */
+	{ PCI_DEVICE(0x8086, 0x9d70), 0},
+	{ 0, }
+};
+MODULE_DEVICE_TABLE(pci, skl_ids);
+
+/* pci_driver definition */
+static struct pci_driver skl_driver = {
+	.name = KBUILD_MODNAME,
+	.id_table = skl_ids,
+	.probe = skl_probe,
+	.remove = skl_remove,
+	.driver = {
+		.pm = &skl_pm,
+	},
+};
+module_pci_driver(skl_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Intel Skylake ASoC HDA driver");
-- 
1.9.1

[PATCH v5 3/4] ASoC: Intel - add makefile support for SKL driver

[Vinod Koul]

From: Jeeja KP <jeeja.kp@intel.com>

This adds makefile and Kconfig to enable Skylake HD audio PCM driver

Signed-off-by: Jeeja KP <jeeja.kp@intel.com>
Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
Signed-off-by: Vinod Koul <vinod.koul@intel.com>
---
 sound/soc/intel/Kconfig          | 17 +++++++++++++++++
 sound/soc/intel/Makefile         |  1 +
 sound/soc/intel/skylake/Makefile |  3 +++
 3 files changed, 21 insertions(+)
 create mode 100644 sound/soc/intel/skylake/Makefile

diff --git a/sound/soc/intel/Kconfig b/sound/soc/intel/Kconfig
index 791953ffbc41..c426d64723b1 100644
--- a/sound/soc/intel/Kconfig
+++ b/sound/soc/intel/Kconfig
@@ -132,3 +132,20 @@ config SND_SOC_INTEL_CHT_BSW_MAX98090_TI_MACH
       This adds support for ASoC machine driver for Intel(R) Cherrytrail & Braswell
       platforms with MAX98090 audio codec it also can support TI jack chip as aux device.
       If unsure select "N".
+
+config SND_SKL_PREALLOC_SIZE
+	int "Pre-allocated buffer size for skylake HD-audio driver"
+	range 0 32768
+	default 64
+	help
+	  Specifies the default pre-allocated buffer-size in kB for the
+	  HD-audio driver.  A larger buffer (e.g. 2048) is preferred
+	  for systems using PulseAudio.  The default 64 is chosen just
+	  for compatibility reasons.
+
+config SND_SOC_INTEL_SKYLAKE
+	tristate
+	select SND_HDA_EXT_CORE
+	help
+	  Say Y here to include support for ASoC Intel "High Definition
+	  Audio" (Skylake) and its compatible devices.
diff --git a/sound/soc/intel/Makefile b/sound/soc/intel/Makefile
index 3853ec2ddbc7..2972699e1435 100644
--- a/sound/soc/intel/Makefile
+++ b/sound/soc/intel/Makefile
@@ -5,6 +5,7 @@ obj-$(CONFIG_SND_SOC_INTEL_SST) += common/
 obj-$(CONFIG_SND_SOC_INTEL_HASWELL) += haswell/
 obj-$(CONFIG_SND_SOC_INTEL_BAYTRAIL) += baytrail/
 obj-$(CONFIG_SND_SST_MFLD_PLATFORM) += atom/
+obj-$(CONFIG_SND_SOC_INTEL_SKYLAKE) += skylake/
 
 # Machine support
 obj-$(CONFIG_SND_SOC_INTEL_SST) += boards/
diff --git a/sound/soc/intel/skylake/Makefile b/sound/soc/intel/skylake/Makefile
new file mode 100644
index 000000000000..734d17cafde7
--- /dev/null
+++ b/sound/soc/intel/skylake/Makefile
@@ -0,0 +1,3 @@
+snd-soc-skl-objs := skl.o skl-pcm.o
+
+obj-$(CONFIG_SND_SOC_INTEL_SKYLAKE) += snd-soc-skl.o
-- 
1.9.1

[PATCH v5 4/4] ASoC: intel - adds support for decoupled mode in skl driver

[Vinod Koul]

From: Jeeja KP <jeeja.kp@intel.com>

Decoupled mode is where audio link is broken to frontend HDA and backend
(hda/i2s/dmic/hdmi) links. This patch adds support for decoupled mode and
then adds dais, dai ops for be/fe cpu dais and interrupt handler change to
support decoupled mode

Signed-off-by: Jeeja KP <jeeja.kp@intel.com>
Signed-off-by: Vinod Koul <vinod.koul@intel.com>
---
 sound/soc/intel/skylake/skl-pcm.c | 367 +++++++++++++++++++++++++++++++++++++-
 sound/soc/intel/skylake/skl.c     |  11 ++
 2 files changed, 370 insertions(+), 8 deletions(-)

diff --git a/sound/soc/intel/skylake/skl-pcm.c b/sound/soc/intel/skylake/skl-pcm.c
index 42cafab47915..97efcd7181ad 100644
--- a/sound/soc/intel/skylake/skl-pcm.c
+++ b/sound/soc/intel/skylake/skl-pcm.c
@@ -102,6 +102,14 @@ static void skl_set_pcm_constrains(struct hdac_ext_bus *ebus,
 				     20, 178000000);
 }
 
+static enum hdac_ext_stream_type skl_get_host_stream_type(struct hdac_ext_bus *ebus)
+{
+	if (ebus->ppcap)
+		return HDAC_EXT_STREAM_TYPE_HOST;
+	else
+		return HDAC_EXT_STREAM_TYPE_COUPLED;
+}
+
 static int skl_pcm_open(struct snd_pcm_substream *substream,
 		struct snd_soc_dai *dai)
 {
@@ -114,7 +122,7 @@ static int skl_pcm_open(struct snd_pcm_substream *substream,
 	pm_runtime_get_sync(dai->dev);
 
 	stream = snd_hdac_ext_stream_assign(ebus, substream,
-					HDAC_EXT_STREAM_TYPE_COUPLED);
+					skl_get_host_stream_type(ebus));
 	if (stream == NULL)
 		return -EBUSY;
 
@@ -150,14 +158,26 @@ static int skl_get_format(struct snd_pcm_substream *substream,
 {
 	struct snd_soc_pcm_runtime *rtd = snd_pcm_substream_chip(substream);
 	struct skl_dma_params *dma_params;
+	struct hdac_ext_bus *ebus = dev_get_drvdata(dai->dev);
 	int format_val = 0;
-	struct snd_soc_dai *codec_dai = rtd->codec_dai;
 
-	dma_params  = (struct skl_dma_params *)
+	if (ebus->ppcap) {
+		struct snd_pcm_runtime *runtime = substream->runtime;
+
+		format_val = snd_hdac_calc_stream_format(runtime->rate,
+						runtime->channels,
+						runtime->format,
+						32, 0);
+
+	} else {
+		struct snd_soc_dai *codec_dai = rtd->codec_dai;
+
+		dma_params  = (struct skl_dma_params *)
 			snd_soc_dai_get_dma_data(codec_dai, substream);
 
-	if (!dma_params)
-		format_val = dma_params->format;
+		if (!dma_params)
+			format_val = dma_params->format;
+	}
 
 	return format_val;
 }
@@ -196,8 +216,9 @@ static int skl_pcm_hw_params(struct snd_pcm_substream *substream,
 				struct snd_soc_dai *dai)
 {
 	struct hdac_ext_bus *ebus = dev_get_drvdata(dai->dev);
+	struct hdac_ext_stream *stream = get_hdac_stream(substream);
 	struct snd_pcm_runtime *runtime = substream->runtime;
-	int ret;
+	int ret, dma_id;
 
 	dev_dbg(dai->dev, "%s: %s\n", __func__, dai->name);
 	ret = skl_substream_alloc_pages(ebus, substream,
@@ -213,17 +234,22 @@ static int skl_pcm_hw_params(struct snd_pcm_substream *substream,
 	dev_dbg(dai->dev, "format_val, rate=%d, ch=%d, format=%d\n",
 			runtime->rate, runtime->channels, runtime->format);
 
+	dma_id = hdac_stream(stream)->stream_tag - 1;
+	dev_dbg(dai->dev, "dma_id=%d\n", dma_id);
+
 	return ret;
 }
 
 static void skl_pcm_close(struct snd_pcm_substream *substream,
 		struct snd_soc_dai *dai)
 {
+	struct hdac_ext_bus *ebus = dev_get_drvdata(dai->dev);
 	struct hdac_ext_stream *stream = get_hdac_stream(substream);
 	struct skl_dma_params *dma_params = NULL;
 
 	dev_dbg(dai->dev, "%s: %s\n", __func__, dai->name);
-	snd_hdac_ext_stream_release(stream, HDAC_EXT_STREAM_TYPE_COUPLED);
+
+	snd_hdac_ext_stream_release(stream, skl_get_host_stream_type(ebus));
 
 	dma_params  = (struct skl_dma_params *)
 			snd_soc_dai_get_dma_data(dai, substream);
@@ -247,6 +273,169 @@ static int skl_pcm_hw_free(struct snd_pcm_substream *substream,
 	return skl_substream_free_pages(ebus_to_hbus(ebus), substream);
 }
 
+static int skl_dmic_prepare(struct snd_pcm_substream *substream,
+				struct snd_soc_dai *dai)
+{
+	struct snd_pcm_hw_params params = {0};
+	struct snd_interval *channels, *rate;
+
+	dev_dbg(dai->dev, "%s: %s\n", __func__, dai->name);
+
+	channels = hw_param_interval(&params, SNDRV_PCM_HW_PARAM_CHANNELS);
+	channels->min = channels->max = substream->runtime->channels;
+	rate = hw_param_interval(&params, SNDRV_PCM_HW_PARAM_RATE);
+	rate->min = rate->max = substream->runtime->rate;
+	snd_mask_set(&params.masks[SNDRV_PCM_HW_PARAM_FORMAT -
+					SNDRV_PCM_HW_PARAM_FIRST_MASK],
+					substream->runtime->format);
+
+	return 0;
+}
+
+static int skl_link_hw_params(struct snd_pcm_substream *substream,
+				struct snd_pcm_hw_params *params,
+				struct snd_soc_dai *dai)
+{
+	struct hdac_ext_bus *ebus = dev_get_drvdata(dai->dev);
+	struct hdac_ext_stream *link_dev;
+	struct snd_soc_pcm_runtime *rtd = snd_pcm_substream_chip(substream);
+	struct skl_dma_params *dma_params;
+	struct snd_soc_dai *codec_dai = rtd->codec_dai;
+	int dma_id;
+
+	pr_debug("%s\n", __func__);
+	link_dev = snd_hdac_ext_stream_assign(ebus, substream,
+					HDAC_EXT_STREAM_TYPE_LINK);
+	if (!link_dev)
+		return -EBUSY;
+
+	snd_soc_dai_set_dma_data(dai, substream, (void *)link_dev);
+
+	/* set the stream tag in the codec dai dma params  */
+	dma_params = (struct skl_dma_params *)
+			snd_soc_dai_get_dma_data(codec_dai, substream);
+	if (dma_params)
+		dma_params->stream_tag =  hdac_stream(link_dev)->stream_tag;
+	snd_soc_dai_set_dma_data(codec_dai, substream, (void *)dma_params);
+	dma_id = hdac_stream(link_dev)->stream_tag - 1;
+
+	return 0;
+}
+
+static int skl_link_pcm_prepare(struct snd_pcm_substream *substream,
+		struct snd_soc_dai *dai)
+{
+	struct snd_soc_pcm_runtime *rtd = snd_pcm_substream_chip(substream);
+	struct hdac_ext_bus *ebus = dev_get_drvdata(dai->dev);
+	struct hdac_ext_stream *link_dev =
+			snd_soc_dai_get_dma_data(dai, substream);
+	unsigned int format_val = 0;
+	struct skl_dma_params *dma_params;
+	struct snd_soc_dai *codec_dai = rtd->codec_dai;
+	struct snd_pcm_hw_params *params;
+	struct snd_interval *channels, *rate;
+	struct hdac_ext_link *link;
+
+	dev_dbg(dai->dev, "%s: %s\n", __func__, dai->name);
+	if (link_dev->link_prepared) {
+		dev_dbg(dai->dev, "already stream is prepared - returning\n");
+		return 0;
+	}
+	params  = devm_kzalloc(dai->dev, sizeof(*params), GFP_KERNEL);
+	if (params == NULL)
+		return -ENOMEM;
+
+	channels = hw_param_interval(params, SNDRV_PCM_HW_PARAM_CHANNELS);
+	channels->min = channels->max = substream->runtime->channels;
+	rate = hw_param_interval(params, SNDRV_PCM_HW_PARAM_RATE);
+	rate->min = rate->max = substream->runtime->rate;
+	snd_mask_set(&params->masks[SNDRV_PCM_HW_PARAM_FORMAT -
+					SNDRV_PCM_HW_PARAM_FIRST_MASK],
+					substream->runtime->format);
+
+
+	dma_params  = (struct skl_dma_params *)
+			snd_soc_dai_get_dma_data(codec_dai, substream);
+	if (dma_params)
+		format_val = dma_params->format;
+	dev_dbg(dai->dev, "stream_tag=%d formatvalue=%d codec_dai_name=%s\n",
+			hdac_stream(link_dev)->stream_tag, format_val, codec_dai->name);
+
+	snd_hdac_ext_link_stream_reset(link_dev);
+
+	snd_hdac_ext_link_stream_setup(link_dev, format_val);
+
+	link = snd_hdac_ext_bus_get_link(ebus, rtd->codec->component.name);
+	if (!link)
+		return -EINVAL;
+
+	snd_hdac_ext_link_set_stream_id(link, hdac_stream(link_dev)->stream_tag);
+	link_dev->link_prepared = 1;
+
+	return 0;
+}
+
+static int skl_link_pcm_trigger(struct snd_pcm_substream *substream,
+	int cmd, struct snd_soc_dai *dai)
+{
+	struct hdac_ext_stream *link_dev =
+				snd_soc_dai_get_dma_data(dai, substream);
+
+	dev_dbg(dai->dev, "In %s cmd=%d\n", __func__, cmd);
+	switch (cmd) {
+	case SNDRV_PCM_TRIGGER_START:
+	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
+	case SNDRV_PCM_TRIGGER_RESUME:
+		snd_hdac_ext_link_stream_start(link_dev);
+		break;
+
+	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
+	case SNDRV_PCM_TRIGGER_SUSPEND:
+	case SNDRV_PCM_TRIGGER_STOP:
+		snd_hdac_ext_link_stream_clear(link_dev);
+		break;
+
+	default:
+		return -EINVAL;
+	}
+	return 0;
+}
+
+static int skl_link_hw_free(struct snd_pcm_substream *substream,
+		struct snd_soc_dai *dai)
+{
+	struct hdac_ext_bus *ebus = dev_get_drvdata(dai->dev);
+	struct snd_soc_pcm_runtime *rtd = snd_pcm_substream_chip(substream);
+	struct hdac_ext_stream *link_dev =
+				snd_soc_dai_get_dma_data(dai, substream);
+	struct hdac_ext_link *link;
+
+	dev_dbg(dai->dev, "%s: %s\n", __func__, dai->name);
+
+	link_dev->link_prepared = 0;
+
+	link = snd_hdac_ext_bus_get_link(ebus, rtd->codec->component.name);
+	if (!link)
+		return -EINVAL;
+
+	snd_hdac_ext_link_clear_stream_id(link, hdac_stream(link_dev)->stream_tag);
+	snd_hdac_ext_stream_release(link_dev, HDAC_EXT_STREAM_TYPE_LINK);
+	return 0;
+}
+
+static int skl_hda_be_startup(struct snd_pcm_substream *substream,
+		struct snd_soc_dai *dai)
+{
+	return pm_runtime_get_sync(dai->dev);
+}
+
+static void skl_hda_be_shutdown(struct snd_pcm_substream *substream,
+		struct snd_soc_dai *dai)
+{
+	pm_runtime_mark_last_busy(dai->dev);
+	pm_runtime_put_autosuspend(dai->dev);
+}
+
 static struct snd_soc_dai_ops skl_pcm_dai_ops = {
 	.startup = skl_pcm_open,
 	.shutdown = skl_pcm_close,
@@ -255,6 +444,21 @@ static struct snd_soc_dai_ops skl_pcm_dai_ops = {
 	.hw_free = skl_pcm_hw_free,
 };
 
+static struct snd_soc_dai_ops skl_dmic_dai_ops = {
+	.startup = skl_hda_be_startup,
+	.prepare = skl_dmic_prepare,
+	.shutdown = skl_hda_be_shutdown,
+};
+
+static struct snd_soc_dai_ops skl_link_dai_ops = {
+	.startup = skl_hda_be_startup,
+	.prepare = skl_link_pcm_prepare,
+	.hw_params = skl_link_hw_params,
+	.hw_free = skl_link_hw_free,
+	.trigger = skl_link_pcm_trigger,
+	.shutdown = skl_hda_be_shutdown,
+};
+
 static struct snd_soc_dai_driver skl_platform_dai[] = {
 {
 	.name = "System Pin",
@@ -275,6 +479,17 @@ static struct snd_soc_dai_driver skl_platform_dai[] = {
 	},
 },
 {
+	.name = "Reference Pin",
+	.ops = &skl_pcm_dai_ops,
+	.capture = {
+		.stream_name = "Reference Capture",
+		.channels_min = HDA_MONO,
+		.channels_max = HDA_STEREO,
+		.rates = SNDRV_PCM_RATE_48000 | SNDRV_PCM_RATE_16000,
+		.formats = SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S24_LE,
+	},
+},
+{
 	.name = "Deepbuffer Pin",
 	.ops = &skl_pcm_dai_ops,
 	.playback = {
@@ -296,6 +511,80 @@ static struct snd_soc_dai_driver skl_platform_dai[] = {
 		.formats = SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S24_LE,
 	},
 },
+/* BE CPU  Dais */
+{
+	.name = "iDisp Pin",
+	.ops = &skl_link_dai_ops,
+	.playback = {
+		.stream_name = "iDisp Tx",
+		.channels_min = HDA_STEREO,
+		.channels_max = HDA_STEREO,
+		.rates = SNDRV_PCM_RATE_8000|SNDRV_PCM_RATE_16000|SNDRV_PCM_RATE_48000,
+		.formats = SNDRV_PCM_FMTBIT_S16_LE,
+	},
+},
+{
+	.name = "DMIC01 Pin",
+	.ops = &skl_dmic_dai_ops,
+	.capture = {
+		.stream_name = "DMIC01 Rx",
+		.channels_min = HDA_STEREO,
+		.channels_max = HDA_STEREO,
+		.rates = SNDRV_PCM_RATE_48000 | SNDRV_PCM_RATE_16000,
+		.formats = SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S24_LE,
+	},
+},
+{
+	.name = "DMIC23 Pin",
+	.ops = &skl_dmic_dai_ops,
+	.capture = {
+		.stream_name = "DMIC23 Rx",
+		.channels_min = HDA_STEREO,
+		.channels_max = HDA_STEREO,
+		.rates = SNDRV_PCM_RATE_48000 | SNDRV_PCM_RATE_16000,
+		.formats = SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S24_LE,
+	},
+},
+{
+	.name = "HD-Codec Pin",
+	.ops = &skl_link_dai_ops,
+	.playback = {
+		.stream_name = "HD-Codec Tx",
+		.channels_min = HDA_STEREO,
+		.channels_max = HDA_STEREO,
+		.rates = SNDRV_PCM_RATE_48000,
+		.formats = SNDRV_PCM_FMTBIT_S16_LE,
+	},
+	.capture = {
+		.stream_name = "HD-Codec Rx",
+		.channels_min = HDA_STEREO,
+		.channels_max = HDA_STEREO,
+		.rates = SNDRV_PCM_RATE_48000,
+		.formats = SNDRV_PCM_FMTBIT_S16_LE,
+	},
+},
+{
+	.name = "HD-Codec-SPK Pin",
+	.ops = &skl_link_dai_ops,
+	.playback = {
+		.stream_name = "HD-Codec-SPK Tx",
+		.channels_min = HDA_STEREO,
+		.channels_max = HDA_STEREO,
+		.rates = SNDRV_PCM_RATE_48000,
+		.formats = SNDRV_PCM_FMTBIT_S16_LE,
+	},
+},
+{
+	.name = "HD-Codec-AMIC Pin",
+	.ops = &skl_link_dai_ops,
+	.capture = {
+		.stream_name = "HD-Codec-AMIC Rx",
+		.channels_min = HDA_STEREO,
+		.channels_max = HDA_STEREO,
+		.rates = SNDRV_PCM_RATE_48000,
+		.formats = SNDRV_PCM_FMTBIT_S16_LE,
+	},
+},
 };
 
 static int skl_platform_open(struct snd_pcm_substream *substream)
@@ -312,7 +601,7 @@ static int skl_platform_open(struct snd_pcm_substream *substream)
 	return 0;
 }
 
-static int skl_platform_pcm_trigger(struct snd_pcm_substream *substream,
+static int skl_pcm_trigger(struct snd_pcm_substream *substream,
 					int cmd)
 {
 	struct hdac_ext_bus *ebus = get_bus_ctx(substream);
@@ -385,6 +674,68 @@ static int skl_platform_pcm_trigger(struct snd_pcm_substream *substream,
 	return 0;
 }
 
+static int skl_dsp_trigger(struct snd_pcm_substream *substream,
+		int cmd)
+{
+	struct hdac_ext_bus *ebus = get_bus_ctx(substream);
+	struct hdac_bus *bus = ebus_to_hbus(ebus);
+	struct snd_soc_pcm_runtime *rtd = snd_pcm_substream_chip(substream);
+	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
+	struct hdac_ext_stream *stream;
+	int start;
+	unsigned long cookie;
+	struct hdac_stream *hstr;
+
+	dev_dbg(bus->dev, "In %s cmd=%d streamname=%s\n", __func__, cmd, cpu_dai->name);
+
+	stream = get_hdac_stream(substream);
+	hstr = hdac_stream(stream);
+
+	if (!hstr->prepared)
+		return -EPIPE;
+
+	switch (cmd) {
+	case SNDRV_PCM_TRIGGER_START:
+	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
+	case SNDRV_PCM_TRIGGER_RESUME:
+		start = 1;
+		break;
+
+	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
+	case SNDRV_PCM_TRIGGER_SUSPEND:
+	case SNDRV_PCM_TRIGGER_STOP:
+		start = 0;
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	spin_lock_irqsave(&bus->reg_lock, cookie);
+
+	if (start)
+		snd_hdac_stream_start(hdac_stream(stream), true);
+	else
+		snd_hdac_stream_stop(hdac_stream(stream));
+
+	if (start)
+		snd_hdac_stream_timecounter_init(hstr, 0);
+
+	spin_unlock_irqrestore(&bus->reg_lock, cookie);
+
+	return 0;
+}
+static int skl_platform_pcm_trigger(struct snd_pcm_substream *substream,
+					int cmd)
+{
+	struct hdac_ext_bus *ebus = get_bus_ctx(substream);
+
+	if (ebus->ppcap)
+		return skl_dsp_trigger(substream, cmd);
+	else
+		return skl_pcm_trigger(substream, cmd);
+}
+
 /* calculate runtime delay from LPIB */
 static int skl_get_delay_from_lpib(struct hdac_ext_bus *ebus,
 				struct hdac_ext_stream *sstream,
diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
index c6f4902af9d0..7d7ae81d5a08 100644
--- a/sound/soc/intel/skylake/skl.c
+++ b/sound/soc/intel/skylake/skl.c
@@ -436,6 +436,8 @@ static int skl_first_init(struct hdac_ext_bus *ebus)
 		return -ENXIO;
 	}
 
+	snd_hdac_ext_bus_parse_capabilities(ebus);
+
 	if (skl_acquire_irq(ebus, 0) < 0)
 		return -EBUSY;
 
@@ -560,6 +562,15 @@ static int skl_probe(struct pci_dev *pci,
 
 	pci_set_drvdata(skl->pci, ebus);
 
+	/* check if dsp is there */
+	if (ebus->ppcap) {
+		/* TODO register with dsp IPC */
+		dev_dbg(bus->dev, "Register dsp\n");
+	}
+
+	if (ebus->mlcap)
+		snd_hdac_ext_bus_get_ml_capabilities(ebus);
+
 	/* create device for soc dmic */
 	err = skl_dmic_device_register(skl);
 	if (err < 0)
-- 
1.9.1

Re: [PATCH v5 2/4] ASoC: Intel - add Skylake HDA audio driver

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 2805 bytes --]

On Thu, Jun 11, 2015 at 10:03:56PM +0530, Vinod Koul wrote:

> +/* initialize SD streams, use seprate streeam tag for PB and CP */
> +static int skl_init_stream(struct hdac_ext_bus *ebus, int start_idx,
> +				int num_stream, int dir)
> +{
> +	int stream_tag = 0;
> +	int i, tag, idx = start_idx;
> +
> +	for (i = 0; i < num_stream; i++) {
> +		struct hdac_ext_stream *stream =
> +				kzalloc(sizeof(*stream), GFP_KERNEL);
> +		if (!stream)
> +			return -ENOMEM;
> +		tag = ++stream_tag;
> +		snd_hdac_ext_stream_init(ebus, stream, idx, dir, tag);
> +		idx++;
> +	}
> +
> +	return 0;
> +}
> +
> +static void skl_free_streams(struct hdac_ext_bus *ebus)
> +{
> +	struct hdac_stream *s;
> +	struct hdac_ext_stream *stream;
> +	struct hdac_bus *bus = ebus_to_hbus(ebus);
> +
> +	while (!list_empty(&bus->stream_list)) {
> +		s = list_first_entry(&bus->stream_list, struct hdac_stream, list);
> +		stream = stream_to_hdac_ext_stream(s);
> +		list_del(&s->list);
> +		kfree(stream);
> +	}
> +}

Still not sure why these are Sky Lake specific?

> +static void skl_free_hda_links(struct hdac_ext_bus *ebus)
> +{
> +	struct hdac_ext_link *l;
> +
> +	while (!list_empty(&ebus->hlink_list)) {
> +		l = list_first_entry(&ebus->hlink_list, struct hdac_ext_link, list);
> +		list_del(&l->list);
> +		kfree(l);
> +	}
> +}

Or this?

> +static int skl_suspend(struct device *dev)
> +{
> +	struct pci_dev *pci = to_pci_dev(dev);
> +	struct hdac_ext_bus *ebus = pci_get_drvdata(pci);
> +	struct hdac_bus *bus = ebus_to_hbus(ebus);
> +
> +	snd_hdac_bus_stop_chip(bus);
> +	snd_hdac_bus_enter_link_reset(bus);
> +	if (bus->irq >= 0) {
> +		free_irq(bus->irq, bus);


Why are we freeing the interrupt over suspend?  That is very unusual
behaviour.

> +static int skl_dmic_device_register(struct skl *skl)
> +{
> +	struct hdac_bus *bus = ebus_to_hbus(&skl->ebus);
> +	struct platform_device *pdev;
> +	int ret;
> +
> +	pdev = platform_device_alloc("dmic-codec", -1);
> +	if (!pdev) {
> +		dev_err(bus->dev, "failed to allocate dmic device\n");
> +		return -1;
> +	}
> +
> +	ret = platform_device_add(pdev);
> +	if (ret) {
> +		dev_err(bus->dev, "failed to add hda codec device\n");
> +		platform_device_put(pdev);
> +		return -1;
> +	}

Don't squash the error codes you get, return them - and if you must
return a fixed error code I'm pretty sure you don't mean -EPERM.

> +	skl->dmic_dev = pdev;

There can only ever be one DMIC?

> +
> +static const struct hdac_io_ops skl_io_ops = {
> +	.reg_writel = skl_pci_writel,
> +	.reg_readl = skl_pci_readl,
> +	.reg_writew = skl_pci_writew,
> +	.reg_readw = skl_pci_readw,
> +	.reg_writeb = skl_pci_writeb,
> +	.reg_readb = skl_pci_readb,
> +	.dma_alloc_pages = skl_dma_alloc_pages,
> +	.dma_free_pages = skl_dma_free_pages,
> +};

Still not thrilled at open coding these wrappers.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v5 3/4] ASoC: Intel - add makefile support for SKL driver

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 587 bytes --]

On Thu, Jun 11, 2015 at 10:03:57PM +0530, Vinod Koul wrote:

> +config SND_SKL_PREALLOC_SIZE
> +	int "Pre-allocated buffer size for skylake HD-audio driver"
> +	range 0 32768
> +	default 64

Size in units of...?

> +	help
> +	  Specifies the default pre-allocated buffer-size in kB for the
> +	  HD-audio driver.  A larger buffer (e.g. 2048) is preferred
> +	  for systems using PulseAudio.  The default 64 is chosen just
> +	  for compatibility reasons.

What are those compatibility reasons - why should users have to worry
about this?  Should this be a module parameter or something?

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v5 2/4] ASoC: Intel - add Skylake HDA audio driver

[Takashi Iwai]

Just a few pin-points:

At Mon, 15 Jun 2015 16:56:33 +0100,
Mark Brown wrote:
> 
> On Thu, Jun 11, 2015 at 10:03:56PM +0530, Vinod Koul wrote:
> 
> > +/* initialize SD streams, use seprate streeam tag for PB and CP */
> > +static int skl_init_stream(struct hdac_ext_bus *ebus, int start_idx,
> > +				int num_stream, int dir)
> > +{
> > +	int stream_tag = 0;
> > +	int i, tag, idx = start_idx;
> > +
> > +	for (i = 0; i < num_stream; i++) {
> > +		struct hdac_ext_stream *stream =
> > +				kzalloc(sizeof(*stream), GFP_KERNEL);
> > +		if (!stream)
> > +			return -ENOMEM;
> > +		tag = ++stream_tag;
> > +		snd_hdac_ext_stream_init(ebus, stream, idx, dir, tag);
> > +		idx++;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static void skl_free_streams(struct hdac_ext_bus *ebus)
> > +{
> > +	struct hdac_stream *s;
> > +	struct hdac_ext_stream *stream;
> > +	struct hdac_bus *bus = ebus_to_hbus(ebus);
> > +
> > +	while (!list_empty(&bus->stream_list)) {
> > +		s = list_first_entry(&bus->stream_list, struct hdac_stream, list);
> > +		stream = stream_to_hdac_ext_stream(s);
> > +		list_del(&s->list);
> > +		kfree(stream);
> > +	}
> > +}
> 
> Still not sure why these are Sky Lake specific?

Currently the allocation and the free of each HDA(-ext) stream are
left to each controller driver.  (See the stream object is embedded.)

And, yes, the allocation (especially the assignment of the stream tag)
*is* SKL specific.  SKL has some twists in the interpretation of
HD-audio spec.


> > +static int skl_suspend(struct device *dev)
> > +{
> > +	struct pci_dev *pci = to_pci_dev(dev);
> > +	struct hdac_ext_bus *ebus = pci_get_drvdata(pci);
> > +	struct hdac_bus *bus = ebus_to_hbus(ebus);
> > +
> > +	snd_hdac_bus_stop_chip(bus);
> > +	snd_hdac_bus_enter_link_reset(bus);
> > +	if (bus->irq >= 0) {
> > +		free_irq(bus->irq, bus);
> 
> 
> Why are we freeing the interrupt over suspend?  That is very unusual
> behaviour.

Believe or not, a PCI controller may reassign to a different IRQ
number after hibernation.  This really happened in the past for some
old stuff, hence we have this in the legacy driver.

But SKL won't need this, I suppose.


> > +
> > +static const struct hdac_io_ops skl_io_ops = {
> > +	.reg_writel = skl_pci_writel,
> > +	.reg_readl = skl_pci_readl,
> > +	.reg_writew = skl_pci_writew,
> > +	.reg_readw = skl_pci_readw,
> > +	.reg_writeb = skl_pci_writeb,
> > +	.reg_readb = skl_pci_readb,
> > +	.dma_alloc_pages = skl_dma_alloc_pages,
> > +	.dma_free_pages = skl_dma_free_pages,
> > +};
> 
> Still not thrilled at open coding these wrappers.

This is not so sexy, right, but be patient until the next HDA core
code rewrite.


Takashi

Re: [PATCH v5 2/4] ASoC: Intel - add Skylake HDA audio driver

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 2108 bytes --]

On Mon, Jun 15, 2015 at 06:35:16PM +0200, Takashi Iwai wrote:
> Mark Brown wrote:
> > On Thu, Jun 11, 2015 at 10:03:56PM +0530, Vinod Koul wrote:

> > > +	for (i = 0; i < num_stream; i++) {
> > > +		struct hdac_ext_stream *stream =
> > > +				kzalloc(sizeof(*stream), GFP_KERNEL);

> > Still not sure why these are Sky Lake specific?

> Currently the allocation and the free of each HDA(-ext) stream are
> left to each controller driver.  (See the stream object is embedded.)

It looks awfully like it's dynamically allocated here...

> And, yes, the allocation (especially the assignment of the stream tag)
> *is* SKL specific.  SKL has some twists in the interpretation of
> HD-audio spec.

I can see the thing calling these functions being driver specific but as
far as I can see all these are doing is allocating structs, initialising
them with passed in parameters, putting them on a list and calling a
core function on them.  It's not the bit taking the decisions, it's just
doing mechanical things.

> > > +	snd_hdac_bus_stop_chip(bus);
> > > +	snd_hdac_bus_enter_link_reset(bus);
> > > +	if (bus->irq >= 0) {
> > > +		free_irq(bus->irq, bus);

> > Why are we freeing the interrupt over suspend?  That is very unusual
> > behaviour.

> Believe or not, a PCI controller may reassign to a different IRQ
> number after hibernation.  This really happened in the past for some
> old stuff, hence we have this in the legacy driver.

> But SKL won't need this, I suppose.

Do you mean PCI controller or BIOS here?

> > > +static const struct hdac_io_ops skl_io_ops = {
> > > +	.reg_writel = skl_pci_writel,
> > > +	.reg_readl = skl_pci_readl,
> > > +	.reg_writew = skl_pci_writew,
> > > +	.reg_readw = skl_pci_readw,
> > > +	.reg_writeb = skl_pci_writeb,
> > > +	.reg_readb = skl_pci_readb,
> > > +	.dma_alloc_pages = skl_dma_alloc_pages,
> > > +	.dma_free_pages = skl_dma_free_pages,
> > > +};

> > Still not thrilled at open coding these wrappers.

> This is not so sexy, right, but be patient until the next HDA core
> code rewrite.

Or we could add the ops to the core now and convert the drivers later?

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v5 3/4] ASoC: Intel - add makefile support for SKL driver

[Takashi Iwai]

At Mon, 15 Jun 2015 16:57:52 +0100,
Mark Brown wrote:
> 
> On Thu, Jun 11, 2015 at 10:03:57PM +0530, Vinod Koul wrote:
> 
> > +config SND_SKL_PREALLOC_SIZE
> > +	int "Pre-allocated buffer size for skylake HD-audio driver"
> > +	range 0 32768
> > +	default 64
> 
> Size in units of...?
> 
> > +	help
> > +	  Specifies the default pre-allocated buffer-size in kB for the
> > +	  HD-audio driver.  A larger buffer (e.g. 2048) is preferred
> > +	  for systems using PulseAudio.  The default 64 is chosen just
> > +	  for compatibility reasons.
> 
> What are those compatibility reasons - why should users have to worry
> about this?

... if a user wants to reduce the memory foot print.
2MB buffers for each stream are significant size, supposing that it
can have 16 streams.

> Should this be a module parameter or something?

The buffers can be reallocated dynamically via procfs in general, but
it's not practical and intuitive.  That's why the similar option was
provided for HDA legacy driver.  It was the time when PA started
accepted slowly.  I don't know whether it makes sense for SKL ASoC
driver, though -- it's up to you guys.


Takashi

Re: [PATCH v5 2/4] ASoC: Intel - add Skylake HDA audio driver

[Takashi Iwai]

At Mon, 15 Jun 2015 17:42:02 +0100,
Mark Brown wrote:
> 
> On Mon, Jun 15, 2015 at 06:35:16PM +0200, Takashi Iwai wrote:
> > Mark Brown wrote:
> > > On Thu, Jun 11, 2015 at 10:03:56PM +0530, Vinod Koul wrote:
> 
> > > > +	for (i = 0; i < num_stream; i++) {
> > > > +		struct hdac_ext_stream *stream =
> > > > +				kzalloc(sizeof(*stream), GFP_KERNEL);
> 
> > > Still not sure why these are Sky Lake specific?
> 
> > Currently the allocation and the free of each HDA(-ext) stream are
> > left to each controller driver.  (See the stream object is embedded.)
> 
> It looks awfully like it's dynamically allocated here...
> 
> > And, yes, the allocation (especially the assignment of the stream tag)
> > *is* SKL specific.  SKL has some twists in the interpretation of
> > HD-audio spec.
> 
> I can see the thing calling these functions being driver specific but as
> far as I can see all these are doing is allocating structs, initialising
> them with passed in parameters, putting them on a list and calling a
> core function on them.  It's not the bit taking the decisions, it's just
> doing mechanical things.

Actually, this can be done a bit more cleanly -- if there will be no
more additions to the stream object.  It wasn't clear, so the
allocation and free are left to the driver, so far.

> > > > +	snd_hdac_bus_stop_chip(bus);
> > > > +	snd_hdac_bus_enter_link_reset(bus);
> > > > +	if (bus->irq >= 0) {
> > > > +		free_irq(bus->irq, bus);
> 
> > > Why are we freeing the interrupt over suspend?  That is very unusual
> > > behaviour.
> 
> > Believe or not, a PCI controller may reassign to a different IRQ
> > number after hibernation.  This really happened in the past for some
> > old stuff, hence we have this in the legacy driver.
> 
> > But SKL won't need this, I suppose.
> 
> Do you mean PCI controller or BIOS here?

Weird combination of both.

> > > > +static const struct hdac_io_ops skl_io_ops = {
> > > > +	.reg_writel = skl_pci_writel,
> > > > +	.reg_readl = skl_pci_readl,
> > > > +	.reg_writew = skl_pci_writew,
> > > > +	.reg_readw = skl_pci_readw,
> > > > +	.reg_writeb = skl_pci_writeb,
> > > > +	.reg_readb = skl_pci_readb,
> > > > +	.dma_alloc_pages = skl_dma_alloc_pages,
> > > > +	.dma_free_pages = skl_dma_free_pages,
> > > > +};
> 
> > > Still not thrilled at open coding these wrappers.
> 
> > This is not so sexy, right, but be patient until the next HDA core
> > code rewrite.
> 
> Or we could add the ops to the core now and convert the drivers later?

This should be feasible.


Takashi

Re: [PATCH v5 3/4] ASoC: Intel - add makefile support for SKL driver

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1417 bytes --]

On Mon, Jun 15, 2015 at 06:42:39PM +0200, Takashi Iwai wrote:
> Mark Brown wrote:
> > On Thu, Jun 11, 2015 at 10:03:57PM +0530, Vinod Koul wrote:

> > > +	  HD-audio driver.  A larger buffer (e.g. 2048) is preferred
> > > +	  for systems using PulseAudio.  The default 64 is chosen just
> > > +	  for compatibility reasons.

> > What are those compatibility reasons - why should users have to worry
> > about this?

> ... if a user wants to reduce the memory foot print.
> 2MB buffers for each stream are significant size, supposing that it
> can have 16 streams.

That doesn't sound like a compatibility thing though?  Compatibility
makes it sound like things will break rather than we'll just use too
much RAM.  Is there anything else going on?

> > Should this be a module parameter or something?

> The buffers can be reallocated dynamically via procfs in general, but
> it's not practical and intuitive.  That's why the similar option was
> provided for HDA legacy driver.  It was the time when PA started
> accepted slowly.  I don't know whether it makes sense for SKL ASoC
> driver, though -- it's up to you guys.

I'm not against having configuration, I just want it to be configuration
that people can understand and it seems better to make the default be
that for PulseAudio given how much of the common case it is now.
Something like "make this smaller to reduce the default memory
footprint" for example.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v5 2/4] ASoC: Intel - add Skylake HDA audio driver

[Vinod Koul]

On Mon, Jun 15, 2015 at 04:56:33PM +0100, Mark Brown wrote:
> > +static int skl_suspend(struct device *dev)
> > +{
> > +	struct pci_dev *pci = to_pci_dev(dev);
> > +	struct hdac_ext_bus *ebus = pci_get_drvdata(pci);
> > +	struct hdac_bus *bus = ebus_to_hbus(ebus);
> > +
> > +	snd_hdac_bus_stop_chip(bus);
> > +	snd_hdac_bus_enter_link_reset(bus);
> > +	if (bus->irq >= 0) {
> > +		free_irq(bus->irq, bus);
> 
> 
> Why are we freeing the interrupt over suspend?  That is very unusual
> behaviour.
As Takashi said, these maybe old HDA related, so I can check if we cna
remove these. I we find an odd machine/BIOS we can always add this later

> 
> > +static int skl_dmic_device_register(struct skl *skl)
> > +{
> > +	struct hdac_bus *bus = ebus_to_hbus(&skl->ebus);
> > +	struct platform_device *pdev;
> > +	int ret;
> > +
> > +	pdev = platform_device_alloc("dmic-codec", -1);
> > +	if (!pdev) {
> > +		dev_err(bus->dev, "failed to allocate dmic device\n");
> > +		return -1;
> > +	}
> > +
> > +	ret = platform_device_add(pdev);
> > +	if (ret) {
> > +		dev_err(bus->dev, "failed to add hda codec device\n");
> > +		platform_device_put(pdev);
> > +		return -1;
> > +	}
> 
> Don't squash the error codes you get, return them - and if you must
> return a fixed error code I'm pretty sure you don't mean -EPERM.
Ah yes, will fix this

> 
> > +	skl->dmic_dev = pdev;
> 
> There can only ever be one DMIC?
One DMIC port, so we need only one DMIC codec device for creating the
DAI-link.

> 
> > +
> > +static const struct hdac_io_ops skl_io_ops = {
> > +	.reg_writel = skl_pci_writel,
> > +	.reg_readl = skl_pci_readl,
> > +	.reg_writew = skl_pci_writew,
> > +	.reg_readw = skl_pci_readw,
> > +	.reg_writeb = skl_pci_writeb,
> > +	.reg_readb = skl_pci_readb,
> > +	.dma_alloc_pages = skl_dma_alloc_pages,
> > +	.dma_free_pages = skl_dma_free_pages,
> > +};
> 
> Still not thrilled at open coding these wrappers.
If we have this is core, I will remove the part :)


-- 
~Vinod

Re: [PATCH v5 3/4] ASoC: Intel - add makefile support for SKL driver

[Vinod Koul]

On Mon, Jun 15, 2015 at 07:54:05PM +0100, Mark Brown wrote:
> On Mon, Jun 15, 2015 at 06:42:39PM +0200, Takashi Iwai wrote:
> > Mark Brown wrote:
> > > On Thu, Jun 11, 2015 at 10:03:57PM +0530, Vinod Koul wrote:
> 
> > > > +	  HD-audio driver.  A larger buffer (e.g. 2048) is preferred
> > > > +	  for systems using PulseAudio.  The default 64 is chosen just
> > > > +	  for compatibility reasons.
> 
> > > What are those compatibility reasons - why should users have to worry
> > > about this?
> 
> > ... if a user wants to reduce the memory foot print.
> > 2MB buffers for each stream are significant size, supposing that it
> > can have 16 streams.
> 
> That doesn't sound like a compatibility thing though?  Compatibility
> makes it sound like things will break rather than we'll just use too
> much RAM.  Is there anything else going on?
> 
> > > Should this be a module parameter or something?
> 
> > The buffers can be reallocated dynamically via procfs in general, but
> > it's not practical and intuitive.  That's why the similar option was
> > provided for HDA legacy driver.  It was the time when PA started
> > accepted slowly.  I don't know whether it makes sense for SKL ASoC
> > driver, though -- it's up to you guys.
> 
> I'm not against having configuration, I just want it to be configuration
> that people can understand and it seems better to make the default be
> that for PulseAudio given how much of the common case it is now.
> Something like "make this smaller to reduce the default memory
> footprint" for example.
We have been using defaults for our testing, and PA and other usermode
entities seem happy :)

Btw Mark, the text is not our invention. This exists in legacy driver
as well.

Said that, i see two thing to be done here, one is to remove this in SKL
driver for now and if required add it later with better explanation as for
now usermode seems happy atm, so may not be required right away

Second one is to actually make this in core, move the flag in legacy driver to
hdac parts and be used in legacy driver as well as anywhere else

Let me know which one looks good to you guys

Thanks
-- 
~Vinod

Re: [PATCH v5 3/4] ASoC: Intel - add makefile support for SKL driver

[Takashi Iwai]

At Mon, 15 Jun 2015 19:54:05 +0100,
Mark Brown wrote:
> 
> On Mon, Jun 15, 2015 at 06:42:39PM +0200, Takashi Iwai wrote:
> > Mark Brown wrote:
> > > On Thu, Jun 11, 2015 at 10:03:57PM +0530, Vinod Koul wrote:
> 
> > > > +	  HD-audio driver.  A larger buffer (e.g. 2048) is preferred
> > > > +	  for systems using PulseAudio.  The default 64 is chosen just
> > > > +	  for compatibility reasons.
> 
> > > What are those compatibility reasons - why should users have to worry
> > > about this?
> 
> > ... if a user wants to reduce the memory foot print.
> > 2MB buffers for each stream are significant size, supposing that it
> > can have 16 streams.
> 
> That doesn't sound like a compatibility thing though?  Compatibility
> makes it sound like things will break rather than we'll just use too
> much RAM.  Is there anything else going on?

Before the parameter was introduced, the allocation was fixed to
64kB.  Thus if you really want to make the driver behaving compatible
as before, it should be 64kB -- not only reducing the memory
footprint, it also keeps the same behavior.

> > > Should this be a module parameter or something?
> 
> > The buffers can be reallocated dynamically via procfs in general, but
> > it's not practical and intuitive.  That's why the similar option was
> > provided for HDA legacy driver.  It was the time when PA started
> > accepted slowly.  I don't know whether it makes sense for SKL ASoC
> > driver, though -- it's up to you guys.
> 
> I'm not against having configuration, I just want it to be configuration
> that people can understand and it seems better to make the default be
> that for PulseAudio given how much of the common case it is now.
> Something like "make this smaller to reduce the default memory
> footprint" for example.

We have no consensus how much size should be given here; e.g. I myself
am not so convinced by a merit of so big buffer allocation like 2MB.


Takashi

Re: [PATCH v5 2/4] ASoC: Intel - add Skylake HDA audio driver

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 675 bytes --]

On Tue, Jun 16, 2015 at 09:22:47AM +0530, Vinod Koul wrote:
> On Mon, Jun 15, 2015 at 04:56:33PM +0100, Mark Brown wrote:

> > > +static const struct hdac_io_ops skl_io_ops = {
> > > +	.reg_writel = skl_pci_writel,
> > > +	.reg_readl = skl_pci_readl,
> > > +	.reg_writew = skl_pci_writew,
> > > +	.reg_readw = skl_pci_readw,
> > > +	.reg_writeb = skl_pci_writeb,
> > > +	.reg_readb = skl_pci_readb,
> > > +	.dma_alloc_pages = skl_dma_alloc_pages,
> > > +	.dma_free_pages = skl_dma_free_pages,
> > > +};

> > Still not thrilled at open coding these wrappers.

> If we have this is core, I will remove the part :)

You could always send changes to add default ops to the core!

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v5 3/4] ASoC: Intel - add makefile support for SKL driver

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1090 bytes --]

On Tue, Jun 16, 2015 at 10:14:58AM +0530, Vinod Koul wrote:

> We have been using defaults for our testing, and PA and other usermode
> entities seem happy :)

> Btw Mark, the text is not our invention. This exists in legacy driver
> as well.

So you're cargo culting it without really understanding it which isn't
so great...

> Said that, i see two thing to be done here, one is to remove this in SKL
> driver for now and if required add it later with better explanation as for
> now usermode seems happy atm, so may not be required right away

Based on what Takashi said it seems this is a memory consumption tuning
knob so it sounds like the low memory work that some of your colleagues
are doing is going to want to add it back sooner rather than later?  It
seems like a reasonable thing to tune, it's just that the text needs to
be updated to reflect reality.

> Second one is to actually make this in core, move the flag in legacy driver to
> hdac parts and be used in legacy driver as well as anywhere else

That's good, of course - just have one config option for all HDA
drivers.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v5 3/4] ASoC: Intel - add makefile support for SKL driver

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1195 bytes --]

On Tue, Jun 16, 2015 at 07:01:58AM +0200, Takashi Iwai wrote:
> Mark Brown wrote:

> > That doesn't sound like a compatibility thing though?  Compatibility
> > makes it sound like things will break rather than we'll just use too
> > much RAM.  Is there anything else going on?

> Before the parameter was introduced, the allocation was fixed to
> 64kB.  Thus if you really want to make the driver behaving compatible
> as before, it should be 64kB -- not only reducing the memory
> footprint, it also keeps the same behavior.

Sure, but given that this is per driver applications really ought to be
able to cope with random values.

> > I'm not against having configuration, I just want it to be configuration
> > that people can understand and it seems better to make the default be
> > that for PulseAudio given how much of the common case it is now.
> > Something like "make this smaller to reduce the default memory
> > footprint" for example.

> We have no consensus how much size should be given here; e.g. I myself
> am not so convinced by a merit of so big buffer allocation like 2MB.

That's fair enough, though we do have to pick a default and like I say
the text should reflect this.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v5 3/4] ASoC: Intel - add makefile support for SKL driver

[Vinod Koul]

On Tue, Jun 16, 2015 at 11:43:46AM +0100, Mark Brown wrote:
 
> > Second one is to actually make this in core, move the flag in legacy driver to
> > hdac parts and be used in legacy driver as well as anywhere else
> 
> That's good, of course - just have one config option for all HDA
> drivers.
Okay I will send that now and remove form here :)

Thanks
-- 
~Vinod

Re: [PATCH v5 2/4] ASoC: Intel - add Skylake HDA audio driver

[Vinod Koul]

On Tue, Jun 16, 2015 at 11:28:26AM +0100, Mark Brown wrote:
> On Tue, Jun 16, 2015 at 09:22:47AM +0530, Vinod Koul wrote:
> > On Mon, Jun 15, 2015 at 04:56:33PM +0100, Mark Brown wrote:
> 
> > > > +static const struct hdac_io_ops skl_io_ops = {
> > > > +	.reg_writel = skl_pci_writel,
> > > > +	.reg_readl = skl_pci_readl,
> > > > +	.reg_writew = skl_pci_writew,
> > > > +	.reg_readw = skl_pci_readw,
> > > > +	.reg_writeb = skl_pci_writeb,
> > > > +	.reg_readb = skl_pci_readb,
> > > > +	.dma_alloc_pages = skl_dma_alloc_pages,
> > > > +	.dma_free_pages = skl_dma_free_pages,
> > > > +};
> 
> > > Still not thrilled at open coding these wrappers.
> 
> > If we have this is core, I will remove the part :)
> 
> You could always send changes to add default ops to the core!
Takashi, you okay with that approach?

Thanks
-- 
~Vinod

Re: [PATCH v5 2/4] ASoC: Intel - add Skylake HDA audio driver

[Vinod Koul]

On Mon, Jun 15, 2015 at 06:46:33PM +0200, Takashi Iwai wrote:
> At Mon, 15 Jun 2015 17:42:02 +0100,
> Mark Brown wrote:
> > 
> > On Mon, Jun 15, 2015 at 06:35:16PM +0200, Takashi Iwai wrote:
> > > Mark Brown wrote:
> > > > On Thu, Jun 11, 2015 at 10:03:56PM +0530, Vinod Koul wrote:
> > 
> > > > > +	for (i = 0; i < num_stream; i++) {
> > > > > +		struct hdac_ext_stream *stream =
> > > > > +				kzalloc(sizeof(*stream), GFP_KERNEL);
> > 
> > > > Still not sure why these are Sky Lake specific?
> > 
> > > Currently the allocation and the free of each HDA(-ext) stream are
> > > left to each controller driver.  (See the stream object is embedded.)
> > 
> > It looks awfully like it's dynamically allocated here...
> > 
> > > And, yes, the allocation (especially the assignment of the stream tag)
> > > *is* SKL specific.  SKL has some twists in the interpretation of
> > > HD-audio spec.
> > 
> > I can see the thing calling these functions being driver specific but as
> > far as I can see all these are doing is allocating structs, initialising
> > them with passed in parameters, putting them on a list and calling a
> > core function on them.  It's not the bit taking the decisions, it's just
> > doing mechanical things.
> 
> Actually, this can be done a bit more cleanly -- if there will be no
> more additions to the stream object.  It wasn't clear, so the
> allocation and free are left to the driver, so far.
Stream object shouldnt be modfied, so in that case should I move this to
core and ext/ ?

-- 
~Vinod

Re: [PATCH v5 2/4] ASoC: Intel - add Skylake HDA audio driver

[Takashi Iwai]

At Tue, 16 Jun 2015 20:55:27 +0530,
Vinod Koul wrote:
> 
> On Mon, Jun 15, 2015 at 06:46:33PM +0200, Takashi Iwai wrote:
> > At Mon, 15 Jun 2015 17:42:02 +0100,
> > Mark Brown wrote:
> > > 
> > > On Mon, Jun 15, 2015 at 06:35:16PM +0200, Takashi Iwai wrote:
> > > > Mark Brown wrote:
> > > > > On Thu, Jun 11, 2015 at 10:03:56PM +0530, Vinod Koul wrote:
> > > 
> > > > > > +	for (i = 0; i < num_stream; i++) {
> > > > > > +		struct hdac_ext_stream *stream =
> > > > > > +				kzalloc(sizeof(*stream), GFP_KERNEL);
> > > 
> > > > > Still not sure why these are Sky Lake specific?
> > > 
> > > > Currently the allocation and the free of each HDA(-ext) stream are
> > > > left to each controller driver.  (See the stream object is embedded.)
> > > 
> > > It looks awfully like it's dynamically allocated here...
> > > 
> > > > And, yes, the allocation (especially the assignment of the stream tag)
> > > > *is* SKL specific.  SKL has some twists in the interpretation of
> > > > HD-audio spec.
> > > 
> > > I can see the thing calling these functions being driver specific but as
> > > far as I can see all these are doing is allocating structs, initialising
> > > them with passed in parameters, putting them on a list and calling a
> > > core function on them.  It's not the bit taking the decisions, it's just
> > > doing mechanical things.
> > 
> > Actually, this can be done a bit more cleanly -- if there will be no
> > more additions to the stream object.  It wasn't clear, so the
> > allocation and free are left to the driver, so far.
> Stream object shouldnt be modfied, so in that case should I move this to
> core and ext/ ?

If there won't be any changes in your side, it's fine to merge into
hda/ext.


Takashi

Re: [PATCH v5 2/4] ASoC: Intel - add Skylake HDA audio driver

[Takashi Iwai]

At Tue, 16 Jun 2015 20:54:05 +0530,
Vinod Koul wrote:
> 
> On Tue, Jun 16, 2015 at 11:28:26AM +0100, Mark Brown wrote:
> > On Tue, Jun 16, 2015 at 09:22:47AM +0530, Vinod Koul wrote:
> > > On Mon, Jun 15, 2015 at 04:56:33PM +0100, Mark Brown wrote:
> > 
> > > > > +static const struct hdac_io_ops skl_io_ops = {
> > > > > +	.reg_writel = skl_pci_writel,
> > > > > +	.reg_readl = skl_pci_readl,
> > > > > +	.reg_writew = skl_pci_writew,
> > > > > +	.reg_readw = skl_pci_readw,
> > > > > +	.reg_writeb = skl_pci_writeb,
> > > > > +	.reg_readb = skl_pci_readb,
> > > > > +	.dma_alloc_pages = skl_dma_alloc_pages,
> > > > > +	.dma_free_pages = skl_dma_free_pages,
> > > > > +};
> > 
> > > > Still not thrilled at open coding these wrappers.
> > 
> > > If we have this is core, I will remove the part :)
> > 
> > You could always send changes to add default ops to the core!
> Takashi, you okay with that approach?

Yes.


Takashi