[PATCH] ASoC: add xtensa xtfpga I2S interface and platform

[PATCH] ASoC: add xtensa xtfpga I2S interface and platform

[Max Filippov]

XTFPGA boards providean audio subsystem that consists of TI CDCE706
clock synthesizer, I2S transmitter and TLV320AIC23 audio codec.

I2S transmitter has MMIO-based interface that resembles that of the
OpenCores I2S transmitter. I2S transmitter is always a master on I2S
bus. There's no specialized audio DMA, sample data are transferred to
I2S transmitter FIFO by CPU through memory-mapped queue interface.

Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
---
 .../devicetree/bindings/sound/cdns,xtfpga-i2s.txt  |  18 +
 MAINTAINERS                                        |   1 +
 sound/soc/Kconfig                                  |   1 +
 sound/soc/Makefile                                 |   1 +
 sound/soc/xtensa/Kconfig                           |  14 +
 sound/soc/xtensa/Makefile                          |   1 +
 sound/soc/xtensa/xtfpga-i2s.c                      | 647 +++++++++++++++++++++
 7 files changed, 683 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/cdns,xtfpga-i2s.txt
 create mode 100644 sound/soc/xtensa/Kconfig
 create mode 100644 sound/soc/xtensa/Makefile
 create mode 100644 sound/soc/xtensa/xtfpga-i2s.c

diff --git a/Documentation/devicetree/bindings/sound/cdns,xtfpga-i2s.txt b/Documentation/devicetree/bindings/sound/cdns,xtfpga-i2s.txt
new file mode 100644
index 0000000..befd125
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/cdns,xtfpga-i2s.txt
@@ -0,0 +1,18 @@
+Bindings for I2S controller built into xtfpga Xtensa bitstreams.
+
+Required properties:
+- compatible: shall be "cdns,xtfpga-i2s".
+- reg: memory region (address and length) with device registers.
+- interrupts: interrupt for the device.
+- clocks: phandle to the clk used as master clock. I2S bus clock
+  is derived from it.
+
+Examples:
+
+	i2s0: xtfpga-i2s@0d080000 {
+		#sound-dai-cells = <0>;
+		compatible = "cdns,xtfpga-i2s";
+		reg = <0x0d080000 0x40>;
+		interrupts = <2 1>;
+		clocks = <&cdce706 4>;
+	};
diff --git a/MAINTAINERS b/MAINTAINERS
index acb4523..d7bfeec 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10204,6 +10204,7 @@ M:	Max Filippov <jcmvbkbc@gmail.com>
 L:	linux-xtensa@linux-xtensa.org
 S:	Maintained
 F:	drivers/spi/spi-xtensa-xtfpga.c
+F:	sound/soc/xtensa/xtfpga-i2s.c
 
 YAM DRIVER FOR AX.25
 M:	Jean-Paul Roubelat <jpr@f6fbb.org>
diff --git a/sound/soc/Kconfig b/sound/soc/Kconfig
index 0e96233..9ca12e7 100644
--- a/sound/soc/Kconfig
+++ b/sound/soc/Kconfig
@@ -56,6 +56,7 @@ source "sound/soc/spear/Kconfig"
 source "sound/soc/tegra/Kconfig"
 source "sound/soc/txx9/Kconfig"
 source "sound/soc/ux500/Kconfig"
+source "sound/soc/xtensa/Kconfig"
 
 # Supported codecs
 source "sound/soc/codecs/Kconfig"
diff --git a/sound/soc/Makefile b/sound/soc/Makefile
index 534714a..99d7222 100644
--- a/sound/soc/Makefile
+++ b/sound/soc/Makefile
@@ -33,3 +33,4 @@ obj-$(CONFIG_SND_SOC)	+= spear/
 obj-$(CONFIG_SND_SOC)	+= tegra/
 obj-$(CONFIG_SND_SOC)	+= txx9/
 obj-$(CONFIG_SND_SOC)	+= ux500/
+obj-$(CONFIG_SND_SOC)	+= xtensa/
diff --git a/sound/soc/xtensa/Kconfig b/sound/soc/xtensa/Kconfig
new file mode 100644
index 0000000..98d6e0c
--- /dev/null
+++ b/sound/soc/xtensa/Kconfig
@@ -0,0 +1,14 @@
+config SND_SOC_XTFPGA_I2S
+	tristate
+	select REGMAP_MMIO
+
+config SND_SOC_XTENSA_XTFPGA
+	tristate "SoC Audio for xtensa xtfpga"
+	depends on XTENSA_PLATFORM_XTFPGA
+	select SND_SOC_XTFPGA_I2S
+	select SND_SOC_TLV320AIC23_SPI
+	select SND_SIMPLE_CARD
+	help
+	  Say Y or M if you want to add support for codecs attached to
+	  xtfpga daughter board. You will also need to select the platform
+	  to support below.
diff --git a/sound/soc/xtensa/Makefile b/sound/soc/xtensa/Makefile
new file mode 100644
index 0000000..97b8eb5
--- /dev/null
+++ b/sound/soc/xtensa/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_SND_SOC_XTFPGA_I2S) += xtfpga-i2s.o
diff --git a/sound/soc/xtensa/xtfpga-i2s.c b/sound/soc/xtensa/xtfpga-i2s.c
new file mode 100644
index 0000000..393d5c3
--- /dev/null
+++ b/sound/soc/xtensa/xtfpga-i2s.c
@@ -0,0 +1,647 @@
+/*
+ * Xtfpga I2S controller driver
+ *
+ * Copyright (c) 2014 Cadence Design Systems Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <sound/pcm_params.h>
+#include <sound/soc.h>
+
+#define DRV_NAME	"xtfpga-i2s"
+
+#define XTFPGA_I2S_VERSION	0x00
+#define XTFPGA_I2S_CONFIG	0x04
+#define XTFPGA_I2S_INT_MASK	0x08
+#define XTFPGA_I2S_INT_STATUS	0x0c
+#define XTFPGA_I2S_CHAN0_DATA	0x10
+#define XTFPGA_I2S_CHAN1_DATA	0x14
+#define XTFPGA_I2S_CHAN2_DATA	0x18
+#define XTFPGA_I2S_CHAN3_DATA	0x1c
+
+#define XTFPGA_I2S_CONFIG_TX_ENABLE	0x1
+#define XTFPGA_I2S_CONFIG_INT_ENABLE	0x2
+#define XTFPGA_I2S_CONFIG_LEFT		0x4
+#define XTFPGA_I2S_CONFIG_RATIO_BASE	8
+#define XTFPGA_I2S_CONFIG_RATIO_MASK	0x0000ff00
+#define XTFPGA_I2S_CONFIG_RES_BASE	16
+#define XTFPGA_I2S_CONFIG_RES_MASK	0x003f0000
+#define XTFPGA_I2S_CONFIG_LEVEL_BASE	24
+#define XTFPGA_I2S_CONFIG_LEVEL_MASK	0x0f000000
+#define XTFPGA_I2S_CONFIG_CHANNEL_BASE	28
+
+#define XTFPGA_I2S_INT_UNDERRUN		0x1
+#define XTFPGA_I2S_INT_LEVEL		0x2
+#define XTFPGA_I2S_INT_VALID		0x3
+
+#define XTFPGA_I2S_FIFO_SIZE		8192
+#define XTFPGA_I2S_CONFIG_DEFAULT_LEVEL	3
+
+/*
+ * I2S controller operation:
+ *
+ * Enabling TX: output 1 period of zeros (starting with left channel)
+ * and then queued data.
+ *
+ * Level status and interrupt: whenever FIFO level is below FIFO trigger,
+ * level status is 1 and an IRQ is asserted (if enabled).
+ *
+ * Underrun status and interrupt: whenever FIFO is empty, underrun status
+ * is 1 and an IRQ is asserted (if enabled).
+ */
+struct xtfpga_i2s {
+	struct device *dev;
+	struct clk *clk;
+	bool clk_enabled;
+	struct regmap *regmap;
+	void __iomem *regs;
+
+	struct snd_pcm_substream *tx_substream;
+	unsigned tx_ptr; /* next frame index in the sample buffer */
+	unsigned tx_fifo_sz; /* current fifo level estimate */
+	unsigned tx_fifo_low; /* FIFO level when level interrupt occurs */
+	unsigned tx_fifo_high; /* maximal FIFO level */
+	struct tasklet_struct refill_fifo; /* FIFO refilling tasklet */
+};
+
+static bool xtfpga_i2s_wr_reg(struct device *dev, unsigned int reg)
+{
+	return reg >= XTFPGA_I2S_CONFIG;
+}
+
+static bool xtfpga_i2s_rd_reg(struct device *dev, unsigned int reg)
+{
+	return reg < XTFPGA_I2S_CHAN0_DATA;
+}
+
+static bool xtfpga_i2s_volatile_reg(struct device *dev, unsigned int reg)
+{
+	return reg == XTFPGA_I2S_INT_STATUS;
+}
+
+static const struct regmap_config xtfpga_i2s_regmap_config = {
+	.reg_bits = 32,
+	.reg_stride = 4,
+	.val_bits = 32,
+	.max_register = XTFPGA_I2S_CHAN3_DATA,
+	.writeable_reg = xtfpga_i2s_wr_reg,
+	.readable_reg = xtfpga_i2s_rd_reg,
+	.volatile_reg = xtfpga_i2s_volatile_reg,
+	.cache_type = REGCACHE_FLAT,
+};
+
+static void xtfpga_i2s_push_tx(struct xtfpga_i2s *i2s)
+{
+	struct snd_pcm_substream *tx_substream;
+	struct snd_pcm_runtime *runtime;
+
+	rcu_read_lock();
+	tx_substream = rcu_dereference(i2s->tx_substream);
+	runtime = tx_substream->runtime;
+#define xtfpga_i2s_tx_loop(i2s, runtime, channels, sample_bits) \
+	do { \
+		const u##sample_bits (*p)[channels] = \
+			(void *)runtime->dma_area; \
+		for (; i2s->tx_fifo_sz < i2s->tx_fifo_high; \
+		     i2s->tx_fifo_sz += 2) { \
+			iowrite32(p[i2s->tx_ptr][0], \
+				  i2s->regs + XTFPGA_I2S_CHAN0_DATA); \
+			iowrite32(p[i2s->tx_ptr][channels - 1], \
+				  i2s->regs + XTFPGA_I2S_CHAN0_DATA); \
+			if (++i2s->tx_ptr >= runtime->buffer_size) \
+				i2s->tx_ptr = 0; \
+		} \
+	} while (0)
+
+	switch (runtime->sample_bits | (runtime->channels << 8)) {
+	case 0x110:
+		xtfpga_i2s_tx_loop(i2s, runtime, 1, 16);
+		break;
+	case 0x210:
+		xtfpga_i2s_tx_loop(i2s, runtime, 2, 16);
+		break;
+	case 0x120:
+		xtfpga_i2s_tx_loop(i2s, runtime, 1, 32);
+		break;
+	case 0x220:
+		xtfpga_i2s_tx_loop(i2s, runtime, 2, 32);
+		break;
+	}
+	rcu_read_unlock();
+}
+
+static void xtfpga_i2s_irq_update_config(struct xtfpga_i2s *i2s,
+					 unsigned int_status)
+{
+	if (!(int_status & XTFPGA_I2S_INT_LEVEL))
+		regmap_write(i2s->regmap, XTFPGA_I2S_INT_MASK,
+			     XTFPGA_I2S_INT_VALID);
+	else if (!(int_status & XTFPGA_I2S_INT_UNDERRUN))
+		regmap_write(i2s->regmap, XTFPGA_I2S_INT_MASK,
+			     XTFPGA_I2S_INT_UNDERRUN);
+
+	if (!(int_status & XTFPGA_I2S_INT_UNDERRUN))
+		regmap_update_bits(i2s->regmap, XTFPGA_I2S_CONFIG,
+				   XTFPGA_I2S_CONFIG_INT_ENABLE |
+				   XTFPGA_I2S_CONFIG_TX_ENABLE,
+				   XTFPGA_I2S_CONFIG_INT_ENABLE |
+				   XTFPGA_I2S_CONFIG_TX_ENABLE);
+}
+
+static void xtfpga_i2s_refill_fifo(struct xtfpga_i2s *i2s)
+{
+	struct snd_pcm_substream *tx_substream;
+	unsigned int_status;
+	unsigned i;
+
+	rcu_read_lock();
+	tx_substream = rcu_dereference(i2s->tx_substream);
+	if (tx_substream && snd_pcm_running(tx_substream)) {
+		for (i = 0; i < 2; ++i) {
+			xtfpga_i2s_push_tx(i2s);
+			regmap_write(i2s->regmap, XTFPGA_I2S_INT_STATUS,
+				     XTFPGA_I2S_INT_VALID);
+			regmap_read(i2s->regmap, XTFPGA_I2S_INT_STATUS,
+				    &int_status);
+
+			if (!(int_status & XTFPGA_I2S_INT_LEVEL))
+				break;
+			i2s->tx_fifo_sz = i2s->tx_fifo_low;
+		}
+	} else {
+		regmap_read(i2s->regmap, XTFPGA_I2S_INT_STATUS,
+			    &int_status);
+	}
+	rcu_read_unlock();
+	xtfpga_i2s_irq_update_config(i2s, int_status);
+}
+
+static void xtfpga_i2s_refill_fifo_tasklet(unsigned long p)
+{
+	xtfpga_i2s_refill_fifo((struct xtfpga_i2s *)p);
+}
+
+static irqreturn_t xtfpga_i2s_interrupt(int irq, void *dev_id)
+{
+	struct xtfpga_i2s *i2s = dev_id;
+	struct snd_pcm_substream *tx_substream;
+	int tx_active;
+	unsigned int_status;
+
+	regmap_read(i2s->regmap, XTFPGA_I2S_INT_STATUS, &int_status);
+	if (!(int_status & XTFPGA_I2S_INT_VALID))
+		return IRQ_NONE;
+
+	if (int_status & XTFPGA_I2S_INT_UNDERRUN) {
+		i2s->tx_fifo_sz = 0;
+		regmap_update_bits(i2s->regmap, XTFPGA_I2S_CONFIG,
+				   XTFPGA_I2S_CONFIG_INT_ENABLE |
+				   XTFPGA_I2S_CONFIG_TX_ENABLE, 0);
+	} else {
+		i2s->tx_fifo_sz = i2s->tx_fifo_low;
+		regmap_update_bits(i2s->regmap, XTFPGA_I2S_CONFIG,
+				   XTFPGA_I2S_CONFIG_INT_ENABLE, 0);
+	}
+
+	rcu_read_lock();
+	tx_substream = rcu_dereference(i2s->tx_substream);
+	tx_active = tx_substream && snd_pcm_running(tx_substream);
+	if (tx_active) {
+		snd_pcm_period_elapsed(tx_substream);
+		if (int_status & XTFPGA_I2S_INT_UNDERRUN)
+			dev_dbg_ratelimited(i2s->dev, "%s: underrun\n",
+					    __func__);
+		tx_substream = rcu_dereference(i2s->tx_substream);
+		tx_active = tx_substream && snd_pcm_running(tx_substream);
+	}
+	rcu_read_unlock();
+
+	if (tx_active) {
+		if (i2s->tx_fifo_high < 256)
+			xtfpga_i2s_refill_fifo(i2s);
+		else
+			tasklet_hi_schedule(&i2s->refill_fifo);
+	} else {
+		xtfpga_i2s_irq_update_config(i2s, int_status);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static int xtfpga_i2s_startup(struct snd_pcm_substream *substream,
+				 struct snd_soc_dai *dai)
+{
+	struct xtfpga_i2s *i2s = snd_soc_dai_get_drvdata(dai);
+
+	snd_soc_dai_set_dma_data(dai, substream, i2s);
+	return 0;
+}
+
+static int xtfpga_i2s_start(struct xtfpga_i2s *i2s,
+			    struct snd_pcm_substream *substream)
+{
+	i2s->tx_ptr = 0;
+	rcu_assign_pointer(i2s->tx_substream, substream);
+
+	xtfpga_i2s_push_tx(i2s);
+	regmap_write(i2s->regmap, XTFPGA_I2S_INT_STATUS, XTFPGA_I2S_INT_VALID);
+	regmap_write(i2s->regmap, XTFPGA_I2S_INT_MASK, XTFPGA_I2S_INT_VALID);
+	regmap_update_bits(i2s->regmap, XTFPGA_I2S_CONFIG,
+			   XTFPGA_I2S_CONFIG_INT_ENABLE |
+			   XTFPGA_I2S_CONFIG_TX_ENABLE,
+			   XTFPGA_I2S_CONFIG_INT_ENABLE |
+			   XTFPGA_I2S_CONFIG_TX_ENABLE);
+	return 0;
+}
+
+static int xtfpga_i2s_stop(struct xtfpga_i2s *i2s,
+			   struct snd_pcm_substream *substream)
+{
+	rcu_assign_pointer(i2s->tx_substream, NULL);
+	return 0;
+}
+
+static int xtfpga_i2s_trigger(struct snd_pcm_substream *substream, int cmd,
+				 struct snd_soc_dai *dai)
+{
+	int ret;
+	struct xtfpga_i2s *i2s = snd_soc_dai_get_drvdata(dai);
+
+	switch (cmd) {
+	case SNDRV_PCM_TRIGGER_START:
+	case SNDRV_PCM_TRIGGER_RESUME:
+	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
+		ret = xtfpga_i2s_start(i2s, substream);
+		break;
+
+	case SNDRV_PCM_TRIGGER_STOP:
+	case SNDRV_PCM_TRIGGER_SUSPEND:
+	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
+		ret = xtfpga_i2s_stop(i2s, substream);
+		break;
+
+	default:
+		ret = -EINVAL;
+		break;
+	}
+	return ret;
+}
+
+static int xtfpga_i2s_hw_params(struct snd_pcm_substream *substream,
+				   struct snd_pcm_hw_params *params,
+				   struct snd_soc_dai *dai)
+{
+	struct xtfpga_i2s *i2s = snd_soc_dai_get_drvdata(dai);
+	unsigned srate = params_rate(params);
+	unsigned channels = params_channels(params);
+	unsigned period_size = params_period_size(params);
+	unsigned sample_size = snd_pcm_format_width(params_format(params));
+	unsigned freq, ratio, level;
+	int err;
+
+	switch (params_format(params)) {
+	case SNDRV_PCM_FORMAT_S16_LE:
+	case SNDRV_PCM_FORMAT_S32_LE:
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	regmap_update_bits(i2s->regmap, XTFPGA_I2S_CONFIG,
+			   XTFPGA_I2S_CONFIG_RES_MASK,
+			   sample_size << XTFPGA_I2S_CONFIG_RES_BASE);
+
+	if (i2s->clk_enabled) {
+		clk_disable_unprepare(i2s->clk);
+		i2s->clk_enabled = false;
+	}
+	freq = 256 * srate;
+	err = clk_set_rate(i2s->clk, freq);
+	if (err < 0)
+		return err;
+
+	err = clk_prepare_enable(i2s->clk);
+	if (err < 0)
+		return err;
+	i2s->clk_enabled = true;
+
+	ratio = (freq - srate * sample_size * 8) /
+		(srate * sample_size * 4);
+
+	regmap_update_bits(i2s->regmap, XTFPGA_I2S_CONFIG,
+			   XTFPGA_I2S_CONFIG_RATIO_MASK,
+			   ratio << XTFPGA_I2S_CONFIG_RATIO_BASE);
+
+	i2s->tx_fifo_low = XTFPGA_I2S_FIFO_SIZE / 2;
+	for (level = 1;
+	     /* period_size * 2: FIFO always gets 2 samples per frame */
+	     i2s->tx_fifo_low / 2 >= period_size * 2;
+	     ++level)
+		i2s->tx_fifo_low /= 2;
+
+	i2s->tx_fifo_high = 2 * i2s->tx_fifo_low;
+	i2s->tx_fifo_sz = 0;
+	i2s->tx_ptr = 0;
+
+	regmap_update_bits(i2s->regmap, XTFPGA_I2S_CONFIG,
+			   XTFPGA_I2S_CONFIG_LEVEL_MASK,
+			   level << XTFPGA_I2S_CONFIG_LEVEL_BASE);
+
+	dev_dbg(i2s->dev,
+		"%s srate: %u, channels: %u, sample_size: %u, period_size: %u\n",
+		__func__, srate, channels, sample_size, period_size);
+	dev_dbg(i2s->dev, "%s freq: %u, ratio: %u, level: %u\n",
+		__func__, freq, ratio, level);
+
+	return 0;
+}
+
+static int xtfpga_i2s_hw_free(struct snd_pcm_substream *substream,
+			      struct snd_soc_dai *dai)
+{
+	struct xtfpga_i2s *i2s = snd_soc_dai_get_drvdata(dai);
+
+	if (i2s->clk_enabled) {
+		clk_disable_unprepare(i2s->clk);
+		i2s->clk_enabled = false;
+	}
+	return 0;
+}
+
+static int xtfpga_i2s_set_fmt(struct snd_soc_dai *cpu_dai,
+				 unsigned int fmt)
+{
+	if ((fmt & SND_SOC_DAIFMT_INV_MASK) != SND_SOC_DAIFMT_NB_NF)
+		return -EINVAL;
+	if ((fmt & SND_SOC_DAIFMT_MASTER_MASK) != SND_SOC_DAIFMT_CBS_CFS)
+		return -EINVAL;
+	if ((fmt & SND_SOC_DAIFMT_FORMAT_MASK) != SND_SOC_DAIFMT_I2S)
+		return -EINVAL;
+
+	return 0;
+}
+
+/* PCM */
+
+static const struct snd_pcm_hardware xtfpga_pcm_hardware = {
+	.info = SNDRV_PCM_INFO_INTERLEAVED |
+		SNDRV_PCM_INFO_MMAP_VALID |
+		SNDRV_PCM_INFO_BLOCK_TRANSFER,
+	.formats		= SNDRV_PCM_FMTBIT_S16_LE |
+				  SNDRV_PCM_FMTBIT_S32_LE,
+	.channels_min		= 1,
+	.channels_max		= 2,
+	.period_bytes_min	= 2,
+	.period_bytes_max	= XTFPGA_I2S_FIFO_SIZE / 2 * 8,
+	.periods_min		= 2,
+	.periods_max		= XTFPGA_I2S_FIFO_SIZE * 8 / 2,
+	.buffer_bytes_max	= XTFPGA_I2S_FIFO_SIZE * 8,
+	.fifo_size		= 16,
+};
+
+static int xtfpga_pcm_open(struct snd_pcm_substream *substream)
+{
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	struct snd_soc_pcm_runtime *rtd = substream->private_data;
+	void *p;
+
+	snd_soc_set_runtime_hwparams(substream, &xtfpga_pcm_hardware);
+	p = snd_soc_dai_get_dma_data(rtd->cpu_dai, substream);
+	runtime->private_data = p;
+
+	return 0;
+}
+
+static int xtfpga_pcm_close(struct snd_pcm_substream *substream)
+{
+	synchronize_rcu();
+	return 0;
+}
+
+static int xtfpga_pcm_hw_params(struct snd_pcm_substream *substream,
+				struct snd_pcm_hw_params *hw_params)
+{
+	int ret;
+
+	ret = snd_pcm_lib_malloc_pages(substream,
+				       params_buffer_bytes(hw_params));
+	return ret;
+}
+
+static int xtfpga_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
+{
+	int ret;
+
+	switch (cmd) {
+	case SNDRV_PCM_TRIGGER_START:
+	case SNDRV_PCM_TRIGGER_RESUME:
+	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
+	case SNDRV_PCM_TRIGGER_STOP:
+	case SNDRV_PCM_TRIGGER_SUSPEND:
+	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
+		ret = 0;
+		break;
+
+	default:
+		ret = -EINVAL;
+		break;
+	}
+	return ret;
+}
+
+static snd_pcm_uframes_t xtfpga_pcm_pointer(struct snd_pcm_substream *substream)
+{
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	struct xtfpga_i2s *i2s = runtime->private_data;
+	snd_pcm_uframes_t pos = i2s->tx_ptr;
+
+	return pos < runtime->buffer_size ? pos : 0;
+}
+
+static int xtfpga_pcm_new(struct snd_soc_pcm_runtime *rtd)
+{
+	struct snd_card *card = rtd->card->snd_card;
+	size_t size = xtfpga_pcm_hardware.buffer_bytes_max;
+
+	return snd_pcm_lib_preallocate_pages_for_all(rtd->pcm,
+						     SNDRV_DMA_TYPE_DEV,
+						     card->dev, size, size);
+}
+
+static void xtfpga_pcm_free(struct snd_pcm *pcm)
+{
+	snd_pcm_lib_preallocate_free_for_all(pcm);
+}
+
+static struct snd_pcm_ops xtfpga_pcm_ops = {
+	.open		= xtfpga_pcm_open,
+	.close		= xtfpga_pcm_close,
+	.ioctl		= snd_pcm_lib_ioctl,
+	.hw_params	= xtfpga_pcm_hw_params,
+	.trigger	= xtfpga_pcm_trigger,
+	.pointer	= xtfpga_pcm_pointer,
+};
+
+struct snd_soc_platform_driver xtfpga_soc_platform = {
+	.pcm_new	= xtfpga_pcm_new,
+	.pcm_free	= xtfpga_pcm_free,
+	.ops		= &xtfpga_pcm_ops,
+};
+
+static const struct snd_soc_component_driver xtfpga_i2s_component = {
+	.name		= DRV_NAME,
+};
+
+static const struct snd_soc_dai_ops xtfpga_i2s_dai_ops = {
+	.startup	= xtfpga_i2s_startup,
+	.trigger	= xtfpga_i2s_trigger,
+	.hw_params      = xtfpga_i2s_hw_params,
+	.hw_free	= xtfpga_i2s_hw_free,
+	.set_fmt        = xtfpga_i2s_set_fmt,
+};
+
+static struct snd_soc_dai_driver xtfpga_i2s_dai[] = {
+	{
+		.name = "xtfpga-i2s",
+		.id = 0,
+		.playback = {
+			.channels_min = 1,
+			.channels_max = 2,
+			.rates = SNDRV_PCM_RATE_8000_96000,
+			.formats = SNDRV_PCM_FMTBIT_S16_LE |
+				   SNDRV_PCM_FMTBIT_S32_LE,
+		},
+		.ops = &xtfpga_i2s_dai_ops,
+	},
+};
+
+static int xtfpga_i2s_probe(struct platform_device *pdev)
+{
+	struct xtfpga_i2s *i2s;
+	struct resource *mem, *irq;
+	int err;
+
+	i2s = devm_kzalloc(&pdev->dev, sizeof(*i2s), GFP_KERNEL);
+	if (!i2s) {
+		err = -ENOMEM;
+		goto err;
+	}
+	dev_set_drvdata(&pdev->dev, i2s);
+	i2s->dev = &pdev->dev;
+	dev_dbg(&pdev->dev, "dev: %p, i2s: %p\n", &pdev->dev, i2s);
+
+	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!mem) {
+		dev_err(&pdev->dev, "No memory resource\n");
+		err = -ENODEV;
+		goto err;
+	}
+	i2s->regs = devm_ioremap_resource(&pdev->dev, mem);
+	if (IS_ERR(i2s->regs)) {
+		err = PTR_ERR(i2s->regs);
+		goto err;
+	}
+
+	i2s->regmap = devm_regmap_init_mmio(&pdev->dev, i2s->regs,
+					    &xtfpga_i2s_regmap_config);
+	if (IS_ERR(i2s->regmap)) {
+		dev_err(&pdev->dev, "regmap init failed\n");
+		err = PTR_ERR(i2s->regmap);
+		goto err;
+	}
+
+	i2s->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(i2s->clk)) {
+		dev_err(&pdev->dev, "couldn't get clock\n");
+		err = PTR_ERR(i2s->clk);
+		goto err;
+	}
+
+	irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+	if (!irq) {
+		dev_err(&pdev->dev, "No IRQ resource\n");
+		err = -ENODEV;
+		goto err;
+	}
+	err = devm_request_irq(&pdev->dev, irq->start, xtfpga_i2s_interrupt,
+			       IRQF_SHARED, pdev->name, i2s);
+	if (err < 0) {
+		dev_err(&pdev->dev, "request_irq failed\n");
+		goto err;
+	}
+	tasklet_init(&i2s->refill_fifo, xtfpga_i2s_refill_fifo_tasklet,
+		     (unsigned long)i2s);
+
+	regmap_write(i2s->regmap, XTFPGA_I2S_CONFIG,
+		     (0x1 << XTFPGA_I2S_CONFIG_CHANNEL_BASE));
+	regmap_write(i2s->regmap, XTFPGA_I2S_INT_STATUS, XTFPGA_I2S_INT_VALID);
+	regmap_write(i2s->regmap, XTFPGA_I2S_INT_MASK, XTFPGA_I2S_INT_UNDERRUN);
+
+	err = snd_soc_register_platform(&pdev->dev, &xtfpga_soc_platform);
+	if (err < 0) {
+		dev_err(&pdev->dev, "couldn't register platform\n");
+		goto err;
+	}
+	err = devm_snd_soc_register_component(&pdev->dev,
+					      &xtfpga_i2s_component,
+					      xtfpga_i2s_dai,
+					      ARRAY_SIZE(xtfpga_i2s_dai));
+	if (err < 0)
+		dev_err(&pdev->dev, "couldn't register component\n");
+err:
+	if (err)
+		dev_err(&pdev->dev, "%s: err = %d\n", __func__, err);
+	return err;
+}
+
+static int xtfpga_i2s_remove(struct platform_device *pdev)
+{
+	struct xtfpga_i2s *i2s = dev_get_drvdata(&pdev->dev);
+
+	if (i2s) {
+		tasklet_kill(&i2s->refill_fifo);
+		if (i2s->regmap && !IS_ERR(i2s->regmap)) {
+			regmap_write(i2s->regmap, XTFPGA_I2S_CONFIG, 0);
+			regmap_write(i2s->regmap, XTFPGA_I2S_INT_MASK, 0);
+			regmap_write(i2s->regmap, XTFPGA_I2S_INT_STATUS,
+				     XTFPGA_I2S_INT_VALID);
+		}
+		if (i2s->clk_enabled)
+			clk_disable_unprepare(i2s->clk);
+	}
+
+	return 0;
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id xtfpga_i2s_of_match[] = {
+	{ .compatible = "cdns,xtfpga-i2s", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, xtfpga_i2s_of_match);
+#endif
+
+static struct platform_driver xtfpga_i2s_driver = {
+	.probe   = xtfpga_i2s_probe,
+	.remove  = xtfpga_i2s_remove,
+	.driver  = {
+		.name = "xtfpga-i2s",
+		.owner = THIS_MODULE,
+		.of_match_table = of_match_ptr(xtfpga_i2s_of_match),
+	},
+};
+
+module_platform_driver(xtfpga_i2s_driver);
+
+MODULE_AUTHOR("Max Filippov <jcmvbkbc@gmail.com>");
+MODULE_DESCRIPTION("xtfpga I2S controller driver");
+MODULE_LICENSE("GPL v2");
-- 
1.8.1.4

Re: [PATCH] ASoC: add xtensa xtfpga I2S interface and platform

[Mark Brown]

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

On Mon, Oct 27, 2014 at 10:07:05PM +0300, Max Filippov wrote:

> +config SND_SOC_XTENSA_XTFPGA
> +	tristate "SoC Audio for xtensa xtfpga"
> +	depends on XTENSA_PLATFORM_XTFPGA
> +	select SND_SOC_XTFPGA_I2S
> +	select SND_SOC_TLV320AIC23_SPI
> +	select SND_SIMPLE_CARD

I've only got this far in the file and have to leave now so I'll look
properly at the actual code later but the above shouldn't have the CODEC
or card driver selected, if the I2S driver is well written it should be
usable independently of either.

> +++ b/sound/soc/xtensa/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_SND_SOC_XTFPGA_I2S) += xtfpga-i2s.o

Please follow the style for all other ASoC drivers and name the module
snd-soc-foo.

> +static void xtfpga_i2s_push_tx(struct xtfpga_i2s *i2s)
> +{
> +	struct snd_pcm_substream *tx_substream;
> +	struct snd_pcm_runtime *runtime;
> +
> +	rcu_read_lock();
> +	tx_substream = rcu_dereference(i2s->tx_substream);
> +	runtime = tx_substream->runtime;
> +#define xtfpga_i2s_tx_loop(i2s, runtime, channels, sample_bits) \
> +	do { \
> +		const u##sample_bits (*p)[channels] = \
> +			(void *)runtime->dma_area; \
> +		for (; i2s->tx_fifo_sz < i2s->tx_fifo_high; \
> +		     i2s->tx_fifo_sz += 2) { \
> +			iowrite32(p[i2s->tx_ptr][0], \
> +				  i2s->regs + XTFPGA_I2S_CHAN0_DATA); \
> +			iowrite32(p[i2s->tx_ptr][channels - 1], \
> +				  i2s->regs + XTFPGA_I2S_CHAN0_DATA); \
> +			if (++i2s->tx_ptr >= runtime->buffer_size) \
> +				i2s->tx_ptr = 0; \
> +		} \
> +	} while (0)
> +
> +	switch (runtime->sample_bits | (runtime->channels << 8)) {
> +	case 0x110:
> +		xtfpga_i2s_tx_loop(i2s, runtime, 1, 16);
> +		break;

This really needs some rework to be legible.  I'd suggest making your
macro an inline function and either replacing these magic numbers in the
switch statement with defines or just writing the code clearly and
letting the compiler figure it out.

Some documentation explaining what your RCU usage is all about would
also be good...  I'm not clear why it's being used, none of the FIQ
drivers do this and I can't entirely figure out what we're defending
against other than the tasklet which we should be stopping anyway and
why what we're doing is safe.

I would also expect to see the data transfer and I2S bits more split
out, presumably this IP can actually be used in designs with a DMA
controller and one would expect that for practical systems this would be
the common case?

> +	if (int_status & XTFPGA_I2S_INT_UNDERRUN) {
> +		i2s->tx_fifo_sz = 0;
> +		regmap_update_bits(i2s->regmap, XTFPGA_I2S_CONFIG,
> +				   XTFPGA_I2S_CONFIG_INT_ENABLE |
> +				   XTFPGA_I2S_CONFIG_TX_ENABLE, 0);
> +	} else {
> +		i2s->tx_fifo_sz = i2s->tx_fifo_low;
> +		regmap_update_bits(i2s->regmap, XTFPGA_I2S_CONFIG,
> +				   XTFPGA_I2S_CONFIG_INT_ENABLE, 0);
> +	}

We're trying to implement a NAPI style thing here?

> +	if (tx_active) {
> +		if (i2s->tx_fifo_high < 256)
> +			xtfpga_i2s_refill_fifo(i2s);
> +		else
> +			tasklet_hi_schedule(&i2s->refill_fifo);

How does the interrupt handler avoid getting into a fight with the
tasklet if the interrupt line is shared?

> +static int xtfpga_i2s_hw_params(struct snd_pcm_substream *substream,
> +				   struct snd_pcm_hw_params *params,
> +				   struct snd_soc_dai *dai)
> +{
> +	struct xtfpga_i2s *i2s = snd_soc_dai_get_drvdata(dai);
> +	unsigned srate = params_rate(params);
> +	unsigned channels = params_channels(params);
> +	unsigned period_size = params_period_size(params);
> +	unsigned sample_size = snd_pcm_format_width(params_format(params));
> +	unsigned freq, ratio, level;
> +	int err;
> +
> +	switch (params_format(params)) {
> +	case SNDRV_PCM_FORMAT_S16_LE:
> +	case SNDRV_PCM_FORMAT_S32_LE:
> +		break;

This looks buggy, we're accepting two different formats but not storing
anything or telling the hardware - especially concerning given that this
is a master only driver.

> +	regmap_update_bits(i2s->regmap, XTFPGA_I2S_CONFIG,
> +			   XTFPGA_I2S_CONFIG_RES_MASK,
> +			   sample_size << XTFPGA_I2S_CONFIG_RES_BASE);
> +
> +	if (i2s->clk_enabled) {
> +		clk_disable_unprepare(i2s->clk);
> +		i2s->clk_enabled = false;
> +	}
> +	freq = 256 * srate;
> +	err = clk_set_rate(i2s->clk, freq);
> +	if (err < 0)
> +		return err;
> +
> +	err = clk_prepare_enable(i2s->clk);
> +	if (err < 0)
> +		return err;
> +	i2s->clk_enabled = true;

This stuff with the clock seems complicated, why not just leave it
disabled when not in use and take advantage of the reference counting
the core already has?

> +	ratio = (freq - srate * sample_size * 8) /
> +		(srate * sample_size * 4);

What ratio exactly?  This needs more brackets in the first line and a
comment explaining what it's calculating.

> +	i2s->tx_fifo_low = XTFPGA_I2S_FIFO_SIZE / 2;
> +	for (level = 1;
> +	     /* period_size * 2: FIFO always gets 2 samples per frame */
> +	     i2s->tx_fifo_low / 2 >= period_size * 2;
> +	     ++level)
> +		i2s->tx_fifo_low /= 2;

This can't come out with a bad value?

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

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

Re: [PATCH] ASoC: add xtensa xtfpga I2S interface and platform

[Max Filippov]

On Mon, Oct 27, 2014 at 10:32 PM, Mark Brown <broonie@kernel.org> wrote:
> On Mon, Oct 27, 2014 at 10:07:05PM +0300, Max Filippov wrote:
>
>> +config SND_SOC_XTENSA_XTFPGA
>> +     tristate "SoC Audio for xtensa xtfpga"
>> +     depends on XTENSA_PLATFORM_XTFPGA
>> +     select SND_SOC_XTFPGA_I2S
>> +     select SND_SOC_TLV320AIC23_SPI
>> +     select SND_SIMPLE_CARD
>
> I've only got this far in the file and have to leave now so I'll look
> properly at the actual code later but the above shouldn't have the CODEC
> or card driver selected, if the I2S driver is well written it should be
> usable independently of either.

The above entry is for the whole sound subsystem of that SoC.
An entry for I2S driver is above it.

>> +++ b/sound/soc/xtensa/Makefile
>> @@ -0,0 +1 @@
>> +obj-$(CONFIG_SND_SOC_XTFPGA_I2S) += xtfpga-i2s.o
>
> Please follow the style for all other ASoC drivers and name the module
> snd-soc-foo.

Ok.

>> +static void xtfpga_i2s_push_tx(struct xtfpga_i2s *i2s)
>> +{
>> +     struct snd_pcm_substream *tx_substream;
>> +     struct snd_pcm_runtime *runtime;
>> +
>> +     rcu_read_lock();
>> +     tx_substream = rcu_dereference(i2s->tx_substream);
>> +     runtime = tx_substream->runtime;
>> +#define xtfpga_i2s_tx_loop(i2s, runtime, channels, sample_bits) \
>> +     do { \
>> +             const u##sample_bits (*p)[channels] = \
>> +                     (void *)runtime->dma_area; \
>> +             for (; i2s->tx_fifo_sz < i2s->tx_fifo_high; \
>> +                  i2s->tx_fifo_sz += 2) { \
>> +                     iowrite32(p[i2s->tx_ptr][0], \
>> +                               i2s->regs + XTFPGA_I2S_CHAN0_DATA); \
>> +                     iowrite32(p[i2s->tx_ptr][channels - 1], \
>> +                               i2s->regs + XTFPGA_I2S_CHAN0_DATA); \
>> +                     if (++i2s->tx_ptr >= runtime->buffer_size) \
>> +                             i2s->tx_ptr = 0; \
>> +             } \
>> +     } while (0)
>> +
>> +     switch (runtime->sample_bits | (runtime->channels << 8)) {
>> +     case 0x110:
>> +             xtfpga_i2s_tx_loop(i2s, runtime, 1, 16);
>> +             break;
>
> This really needs some rework to be legible.  I'd suggest making your
> macro an inline function and either replacing these magic numbers in the
> switch statement with defines or just writing the code clearly and
> letting the compiler figure it out.

Ok, I'll rewrite it.

> Some documentation explaining what your RCU usage is all about would
> also be good...  I'm not clear why it's being used, none of the FIQ
> drivers do this and I can't entirely figure out what we're defending
> against other than the tasklet which we should be stopping anyway and
> why what we're doing is safe.

Ok, I'll document it. I'm synchronizing trigger callback with both interrupt
handler and the tasklet. All that they need to know is whether we're playing
a stream or not, so I protect the stream pointer with RCU. A spinlock for
protection of a single pointer seems too big for me.

> I would also expect to see the data transfer and I2S bits more split
> out, presumably this IP can actually be used in designs with a DMA
> controller and one would expect that for practical systems this would be
> the common case?

I don't know of other designs that use this IP block. Can we do it when
we get such users?

>> +     if (int_status & XTFPGA_I2S_INT_UNDERRUN) {
>> +             i2s->tx_fifo_sz = 0;
>> +             regmap_update_bits(i2s->regmap, XTFPGA_I2S_CONFIG,
>> +                                XTFPGA_I2S_CONFIG_INT_ENABLE |
>> +                                XTFPGA_I2S_CONFIG_TX_ENABLE, 0);
>> +     } else {
>> +             i2s->tx_fifo_sz = i2s->tx_fifo_low;
>> +             regmap_update_bits(i2s->regmap, XTFPGA_I2S_CONFIG,
>> +                                XTFPGA_I2S_CONFIG_INT_ENABLE, 0);
>> +     }
>
> We're trying to implement a NAPI style thing here?

No, there's no way to retrieve the exact FIFO level from the hardware,
there's no such register. But there are two interrupt reasons: one
when the FIFO is below preset level and another when FIFO is empty.
So this code tracks the FIFO level according to the interrupt reason.

>> +     if (tx_active) {
>> +             if (i2s->tx_fifo_high < 256)
>> +                     xtfpga_i2s_refill_fifo(i2s);
>> +             else
>> +                     tasklet_hi_schedule(&i2s->refill_fifo);
>
> How does the interrupt handler avoid getting into a fight with the
> tasklet if the interrupt line is shared?

IRQ is masked in the I2S controller once we're in the interrupt
handler and is not re-enabled until we're done with FIFO, which
happens at the very end of a tasklet. But if the line could ever be
shared interrupt handler logic that checks if IRQ came from our
device needs an additional check. Thanks for pointing that out.

>> +static int xtfpga_i2s_hw_params(struct snd_pcm_substream *substream,
>> +                                struct snd_pcm_hw_params *params,
>> +                                struct snd_soc_dai *dai)
>> +{
>> +     struct xtfpga_i2s *i2s = snd_soc_dai_get_drvdata(dai);
>> +     unsigned srate = params_rate(params);
>> +     unsigned channels = params_channels(params);
>> +     unsigned period_size = params_period_size(params);
>> +     unsigned sample_size = snd_pcm_format_width(params_format(params));
>> +     unsigned freq, ratio, level;
>> +     int err;
>> +
>> +     switch (params_format(params)) {
>> +     case SNDRV_PCM_FORMAT_S16_LE:
>> +     case SNDRV_PCM_FORMAT_S32_LE:
>> +             break;
>
> This looks buggy, we're accepting two different formats but not storing
> anything or telling the hardware - especially concerning given that this
> is a master only driver.

xtfpga_i2s_push_tx does the right thing based on runtime->sample_bits
value.

>> +     regmap_update_bits(i2s->regmap, XTFPGA_I2S_CONFIG,
>> +                        XTFPGA_I2S_CONFIG_RES_MASK,
>> +                        sample_size << XTFPGA_I2S_CONFIG_RES_BASE);
>> +
>> +     if (i2s->clk_enabled) {
>> +             clk_disable_unprepare(i2s->clk);
>> +             i2s->clk_enabled = false;
>> +     }
>> +     freq = 256 * srate;
>> +     err = clk_set_rate(i2s->clk, freq);
>> +     if (err < 0)
>> +             return err;
>> +
>> +     err = clk_prepare_enable(i2s->clk);
>> +     if (err < 0)
>> +             return err;
>> +     i2s->clk_enabled = true;
>
> This stuff with the clock seems complicated, why not just leave it
> disabled when not in use and take advantage of the reference counting
> the core already has?

Because this callback is said to be potentially called multiple times per
initialization. Is it not?

>> +     ratio = (freq - srate * sample_size * 8) /
>> +             (srate * sample_size * 4);
>
> What ratio exactly?  This needs more brackets in the first line and a
> comment explaining what it's calculating.

It controls the divider in the I2S controller that derives I2S bus frequency
from the master clock. The manual basically says 'use the following
equation to determine the ratio value'.

>> +     i2s->tx_fifo_low = XTFPGA_I2S_FIFO_SIZE / 2;
>> +     for (level = 1;
>> +          /* period_size * 2: FIFO always gets 2 samples per frame */
>> +          i2s->tx_fifo_low / 2 >= period_size * 2;
>> +          ++level)
>> +             i2s->tx_fifo_low /= 2;
>
> This can't come out with a bad value?

I've tested it in the range of frequencies that we support -- works well.

-- 
Thanks.
-- Max

Re: [PATCH] ASoC: add xtensa xtfpga I2S interface and platform

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 5120 bytes --]

On Mon, Oct 27, 2014 at 11:38:53PM +0300, Max Filippov wrote:
> On Mon, Oct 27, 2014 at 10:32 PM, Mark Brown <broonie@kernel.org> wrote:
> > On Mon, Oct 27, 2014 at 10:07:05PM +0300, Max Filippov wrote:

> >> +config SND_SOC_XTENSA_XTFPGA
> >> +     tristate "SoC Audio for xtensa xtfpga"
> >> +     depends on XTENSA_PLATFORM_XTFPGA
> >> +     select SND_SOC_XTFPGA_I2S
> >> +     select SND_SOC_TLV320AIC23_SPI
> >> +     select SND_SIMPLE_CARD

> > I've only got this far in the file and have to leave now so I'll look
> > properly at the actual code later but the above shouldn't have the CODEC
> > or card driver selected, if the I2S driver is well written it should be
> > usable independently of either.

> The above entry is for the whole sound subsystem of that SoC.
> An entry for I2S driver is above it.

Then this just shouldn't exist at all, adding Kconfig entries for all
the simple-card devices would defeat the point of having simple-card
which is why we don't do it for other systems.

> > Some documentation explaining what your RCU usage is all about would
> > also be good...  I'm not clear why it's being used, none of the FIQ
> > drivers do this and I can't entirely figure out what we're defending
> > against other than the tasklet which we should be stopping anyway and
> > why what we're doing is safe.

> Ok, I'll document it. I'm synchronizing trigger callback with both interrupt
> handler and the tasklet. All that they need to know is whether we're playing
> a stream or not, so I protect the stream pointer with RCU. A spinlock for
> protection of a single pointer seems too big for me.

So atomics don't work?  Simple flags are one of the few cases where they
might cover it...  again, the fact that this isn't similar to other code
doing the same thing is worrying.

> > I would also expect to see the data transfer and I2S bits more split
> > out, presumably this IP can actually be used in designs with a DMA
> > controller and one would expect that for practical systems this would be
> > the common case?

> I don't know of other designs that use this IP block. Can we do it when
> we get such users?

It's also about ensuring that the code is cleanly split up so that
someone can actually go in and add the required support later (and TBH
it'd be better to just add a DMA controller on the FPGA, everyone will
be much happier).

> >> +     if (int_status & XTFPGA_I2S_INT_UNDERRUN) {
> >> +             i2s->tx_fifo_sz = 0;
> >> +             regmap_update_bits(i2s->regmap, XTFPGA_I2S_CONFIG,
> >> +                                XTFPGA_I2S_CONFIG_INT_ENABLE |
> >> +                                XTFPGA_I2S_CONFIG_TX_ENABLE, 0);
> >> +     } else {
> >> +             i2s->tx_fifo_sz = i2s->tx_fifo_low;
> >> +             regmap_update_bits(i2s->regmap, XTFPGA_I2S_CONFIG,
> >> +                                XTFPGA_I2S_CONFIG_INT_ENABLE, 0);
> >> +     }

> > We're trying to implement a NAPI style thing here?

> No, there's no way to retrieve the exact FIFO level from the hardware,
> there's no such register. But there are two interrupt reasons: one
> when the FIFO is below preset level and another when FIFO is empty.
> So this code tracks the FIFO level according to the interrupt reason.

...and then it appears to try to implement something like NAPI?

> >> +     switch (params_format(params)) {
> >> +     case SNDRV_PCM_FORMAT_S16_LE:
> >> +     case SNDRV_PCM_FORMAT_S32_LE:
> >> +             break;

> > This looks buggy, we're accepting two different formats but not storing
> > anything or telling the hardware - especially concerning given that this
> > is a master only driver.

> xtfpga_i2s_push_tx does the right thing based on runtime->sample_bits
> value.

So add a comment then.  The thing I was saying about the data push code
being hard to understand apply generally.

> >> +     err = clk_prepare_enable(i2s->clk);
> >> +     if (err < 0)
> >> +             return err;
> >> +     i2s->clk_enabled = true;

> > This stuff with the clock seems complicated, why not just leave it
> > disabled when not in use and take advantage of the reference counting
> > the core already has?

> Because this callback is said to be potentially called multiple times per
> initialization. Is it not?

It can but but I'm not seeing any connection between that and the idea
of not keeping the clock enabled when the device isn't in use?

> >> +     i2s->tx_fifo_low = XTFPGA_I2S_FIFO_SIZE / 2;
> >> +     for (level = 1;
> >> +          /* period_size * 2: FIFO always gets 2 samples per frame */
> >> +          i2s->tx_fifo_low / 2 >= period_size * 2;
> >> +          ++level)
> >> +             i2s->tx_fifo_low /= 2;

> > This can't come out with a bad value?

> I've tested it in the range of frequencies that we support -- works well.

I'm not sure I find that terribly convincing, I'd like to be able to
look at the code and tell that it's not going to blow up.  This again
comes back to the general comment about clarity - the code *looks*
suspicious (strange indentation of the for loop with a comment in the
middle of the statement itself for example).

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

Re: [PATCH] ASoC: add xtensa xtfpga I2S interface and platform

[Max Filippov]

On Tue, Oct 28, 2014 at 6:42 PM, Mark Brown <broonie@kernel.org> wrote:
> On Mon, Oct 27, 2014 at 11:38:53PM +0300, Max Filippov wrote:
>> On Mon, Oct 27, 2014 at 10:32 PM, Mark Brown <broonie@kernel.org> wrote:
>> > On Mon, Oct 27, 2014 at 10:07:05PM +0300, Max Filippov wrote:
>
>> >> +config SND_SOC_XTENSA_XTFPGA
>> >> +     tristate "SoC Audio for xtensa xtfpga"
>> >> +     depends on XTENSA_PLATFORM_XTFPGA
>> >> +     select SND_SOC_XTFPGA_I2S
>> >> +     select SND_SOC_TLV320AIC23_SPI
>> >> +     select SND_SIMPLE_CARD
>
>> > I've only got this far in the file and have to leave now so I'll look
>> > properly at the actual code later but the above shouldn't have the CODEC
>> > or card driver selected, if the I2S driver is well written it should be
>> > usable independently of either.
>
>> The above entry is for the whole sound subsystem of that SoC.
>> An entry for I2S driver is above it.
>
> Then this just shouldn't exist at all, adding Kconfig entries for all
> the simple-card devices would defeat the point of having simple-card
> which is why we don't do it for other systems.

sound/soc/sh/Kconfig does that as well.
Not having single config item to enable at once all relevant drivers feels
a bit strange... But ok, I'll drop that.

>> > Some documentation explaining what your RCU usage is all about would
>> > also be good...  I'm not clear why it's being used, none of the FIQ
>> > drivers do this and I can't entirely figure out what we're defending
>> > against other than the tasklet which we should be stopping anyway and
>> > why what we're doing is safe.
>
>> Ok, I'll document it. I'm synchronizing trigger callback with both interrupt
>> handler and the tasklet. All that they need to know is whether we're playing
>> a stream or not, so I protect the stream pointer with RCU. A spinlock for
>> protection of a single pointer seems too big for me.
>
> So atomics don't work?  Simple flags are one of the few cases where they
> might cover it...  again, the fact that this isn't similar to other code
> doing the same thing is worrying.

tx_substream use pattern is the same as of a typical RCU variable.
RCU wrappers combine ACCESS_ONCE and barriers that I'd otherwise
have to write myself.

>> > I would also expect to see the data transfer and I2S bits more split
>> > out, presumably this IP can actually be used in designs with a DMA
>> > controller and one would expect that for practical systems this would be
>> > the common case?
>
>> I don't know of other designs that use this IP block. Can we do it when
>> we get such users?
>
> It's also about ensuring that the code is cleanly split up so that
> someone can actually go in and add the required support later (and TBH

Can you point me to an example of such split, so that I don't write it in
an unusual way?

> it'd be better to just add a DMA controller on the FPGA, everyone will
> be much happier).

Hardware people think different and it's been like that for more than 5
years.

>> >> +     if (int_status & XTFPGA_I2S_INT_UNDERRUN) {
>> >> +             i2s->tx_fifo_sz = 0;
>> >> +             regmap_update_bits(i2s->regmap, XTFPGA_I2S_CONFIG,
>> >> +                                XTFPGA_I2S_CONFIG_INT_ENABLE |
>> >> +                                XTFPGA_I2S_CONFIG_TX_ENABLE, 0);
>> >> +     } else {
>> >> +             i2s->tx_fifo_sz = i2s->tx_fifo_low;
>> >> +             regmap_update_bits(i2s->regmap, XTFPGA_I2S_CONFIG,
>> >> +                                XTFPGA_I2S_CONFIG_INT_ENABLE, 0);
>> >> +     }
>
>> > We're trying to implement a NAPI style thing here?
>
>> No, there's no way to retrieve the exact FIFO level from the hardware,
>> there's no such register. But there are two interrupt reasons: one
>> when the FIFO is below preset level and another when FIFO is empty.
>> So this code tracks the FIFO level according to the interrupt reason.
>
> ...and then it appears to try to implement something like NAPI?

If the programmed FIFO level is low enough I refill FIFO right in the
interrupt handler. If I use tasklet for such short FIFOs the sound
becomes choppy.

>> >> +     switch (params_format(params)) {
>> >> +     case SNDRV_PCM_FORMAT_S16_LE:
>> >> +     case SNDRV_PCM_FORMAT_S32_LE:
>> >> +             break;
>
>> > This looks buggy, we're accepting two different formats but not storing
>> > anything or telling the hardware - especially concerning given that this
>> > is a master only driver.
>
>> xtfpga_i2s_push_tx does the right thing based on runtime->sample_bits
>> value.
>
> So add a comment then.  The thing I was saying about the data push code
> being hard to understand apply generally.

Ok.

>> >> +     err = clk_prepare_enable(i2s->clk);
>> >> +     if (err < 0)
>> >> +             return err;
>> >> +     i2s->clk_enabled = true;
>
>> > This stuff with the clock seems complicated, why not just leave it
>> > disabled when not in use and take advantage of the reference counting
>> > the core already has?
>
>> Because this callback is said to be potentially called multiple times per
>> initialization. Is it not?
>
> It can but but I'm not seeing any connection between that and the idea
> of not keeping the clock enabled when the device isn't in use?

hw_params callback can change MCLK rate, so it has to disable and
enable the clock anyway, and since enable can fail it does not guarantee
that the clock will be left in the same state. Or should I adjust MCLK rate
w/o disabling the clock?

>> >> +     i2s->tx_fifo_low = XTFPGA_I2S_FIFO_SIZE / 2;
>> >> +     for (level = 1;
>> >> +          /* period_size * 2: FIFO always gets 2 samples per frame */
>> >> +          i2s->tx_fifo_low / 2 >= period_size * 2;
>> >> +          ++level)
>> >> +             i2s->tx_fifo_low /= 2;
>
>> > This can't come out with a bad value?
>
>> I've tested it in the range of frequencies that we support -- works well.
>
> I'm not sure I find that terribly convincing, I'd like to be able to
> look at the code and tell that it's not going to blow up.  This again
> comes back to the general comment about clarity - the code *looks*
> suspicious (strange indentation of the for loop with a comment in the
> middle of the statement itself for example).

The level field in the control register is 4 bit wide, so the allowed range of
level is 0..15. FIFO size is 8192 entries, level = 1 corresponds to
FIFO size / 2, level = 14 -- to FIFO size of 0. I guess this function
won't get period_size of 0?

-- 
Thanks.
-- Max

Re: [PATCH] ASoC: add xtensa xtfpga I2S interface and platform

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 4195 bytes --]

On Tue, Oct 28, 2014 at 08:00:45PM +0300, Max Filippov wrote:
> On Tue, Oct 28, 2014 at 6:42 PM, Mark Brown <broonie@kernel.org> wrote:

> > Then this just shouldn't exist at all, adding Kconfig entries for all
> > the simple-card devices would defeat the point of having simple-card
> > which is why we don't do it for other systems.

> sound/soc/sh/Kconfig does that as well.
> Not having single config item to enable at once all relevant drivers feels
> a bit strange... But ok, I'll drop that.

This is new code, not a replacement of old code.

> > So atomics don't work?  Simple flags are one of the few cases where they
> > might cover it...  again, the fact that this isn't similar to other code
> > doing the same thing is worrying.

> tx_substream use pattern is the same as of a typical RCU variable.
> RCU wrappers combine ACCESS_ONCE and barriers that I'd otherwise
> have to write myself.

You *really* need to explain how it's supposed to work - right now it's
not at all obvious, like I say the fact that this is a rarely used idiom
is not helping.  For example when we tear down the stream we just assign
the pointer in _stop() but don't bother with a sync until the stream is
closed - why?  We also appear to be doing several different dereferences
of the pointer inside a single rcu_read_lock() for some reason which is
worrying.

> >> > I would also expect to see the data transfer and I2S bits more split
> >> > out, presumably this IP can actually be used in designs with a DMA
> >> > controller and one would expect that for practical systems this would be
> >> > the common case?

> >> I don't know of other designs that use this IP block. Can we do it when
> >> we get such users?

> > It's also about ensuring that the code is cleanly split up so that
> > someone can actually go in and add the required support later (and TBH

> Can you point me to an example of such split, so that I don't write it in
> an unusual way?

Essentially all drivers are split this way...

> > it'd be better to just add a DMA controller on the FPGA, everyone will
> > be much happier).

> Hardware people think different and it's been like that for more than 5
> years.

They appear to be the only hardware people who think this way, while we
do have some FIQ based PIO stuff in mainline all the examples I can
think of are there because people didn't work out how to drive the DMA
controller yet (and even there we're using FIQs not bashing things in
from a regular interrupt).

> >> Because this callback is said to be potentially called multiple times per
> >> initialization. Is it not?

> > It can but but I'm not seeing any connection between that and the idea
> > of not keeping the clock enabled when the device isn't in use?

> hw_params callback can change MCLK rate, so it has to disable and
> enable the clock anyway, and since enable can fail it does not guarantee
> that the clock will be left in the same state. Or should I adjust MCLK rate
> w/o disabling the clock?

So yet again: why not just enable the clock only when the device is in
use?  If it's being configured it stands to reason that the device isn't
actively in use...

> > I'm not sure I find that terribly convincing, I'd like to be able to
> > look at the code and tell that it's not going to blow up.  This again
> > comes back to the general comment about clarity - the code *looks*
> > suspicious (strange indentation of the for loop with a comment in the
> > middle of the statement itself for example).

> The level field in the control register is 4 bit wide, so the allowed range of
> level is 0..15. FIFO size is 8192 entries, level = 1 corresponds to
> FIFO size / 2, level = 14 -- to FIFO size of 0. I guess this function
> won't get period_size of 0?

So if the IP gets changed and the code gets blown up this could well
explode then...  which doesn't seem entirely unlikely considering this
is a FPGA platform so presumably this is easy to update.  To repeat this
is about clarity and the code looking like it's probably hiding bugs as
much as if the code actually works if you really sit down and study it.

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

Re: [PATCH] ASoC: add xtensa xtfpga I2S interface and platform

[Max Filippov]

On Tue, Oct 28, 2014 at 8:38 PM, Mark Brown <broonie@kernel.org> wrote:
> On Tue, Oct 28, 2014 at 08:00:45PM +0300, Max Filippov wrote:
>> On Tue, Oct 28, 2014 at 6:42 PM, Mark Brown <broonie@kernel.org> wrote:
>> > So atomics don't work?  Simple flags are one of the few cases where they
>> > might cover it...  again, the fact that this isn't similar to other code
>> > doing the same thing is worrying.
>
>> tx_substream use pattern is the same as of a typical RCU variable.
>> RCU wrappers combine ACCESS_ONCE and barriers that I'd otherwise
>> have to write myself.
>
> You *really* need to explain how it's supposed to work - right now it's
> not at all obvious, like I say the fact that this is a rarely used idiom
> is not helping.  For example when we tear down the stream we just assign
> the pointer in _stop() but don't bother with a sync until the stream is
> closed - why?

Because we can't wait in stop and syncing is not time critical, we can
do it any time before the stream becomes invalid.

> We also appear to be doing several different dereferences
> of the pointer inside a single rcu_read_lock() for some reason which is
> worrying.

I've cleaned that up.

>> > it'd be better to just add a DMA controller on the FPGA, everyone will
>> > be much happier).
>
>> Hardware people think different and it's been like that for more than 5
>> years.
>
> They appear to be the only hardware people who think this way, while we

The whole audio subsystem on xtfpga boards is there for, I think, a single
purpose: for the demonstration of CPU audio processing capabilities.
That's why it's that simple.

> do have some FIQ based PIO stuff in mainline all the examples I can
> think of are there because people didn't work out how to drive the DMA
> controller yet (and even there we're using FIQs not bashing things in
> from a regular interrupt).

Currently we don't support fast interrupt handlers written in C on xtensa.

>> >> Because this callback is said to be potentially called multiple times per
>> >> initialization. Is it not?
>
>> > It can but but I'm not seeing any connection between that and the idea
>> > of not keeping the clock enabled when the device isn't in use?
>
>> hw_params callback can change MCLK rate, so it has to disable and
>> enable the clock anyway, and since enable can fail it does not guarantee
>> that the clock will be left in the same state. Or should I adjust MCLK rate
>> w/o disabling the clock?
>
> So yet again: why not just enable the clock only when the device is in
> use?  If it's being configured it stands to reason that the device isn't
> actively in use...

Mark, I don't get it, sorry ): My clock synthesizer is I2C controlled, so
I can't prepare/unprepare it in the trigger callback. When should I do it?

>> > I'm not sure I find that terribly convincing, I'd like to be able to
>> > look at the code and tell that it's not going to blow up.  This again
>> > comes back to the general comment about clarity - the code *looks*
>> > suspicious (strange indentation of the for loop with a comment in the
>> > middle of the statement itself for example).
>
>> The level field in the control register is 4 bit wide, so the allowed range of
>> level is 0..15. FIFO size is 8192 entries, level = 1 corresponds to
>> FIFO size / 2, level = 14 -- to FIFO size of 0. I guess this function
>> won't get period_size of 0?
>
> So if the IP gets changed and the code gets blown up this could well
> explode then...  which doesn't seem entirely unlikely considering this
> is a FPGA platform so presumably this is easy to update.  To repeat this
> is about clarity and the code looking like it's probably hiding bugs as
> much as if the code actually works if you really sit down and study it.

The calculation does not depend on the actual hardware, but on the
constant definitions in the same file. They need to be updated if the
hardware changes. I'll try to rewrite it in a cleaner way.

-- 
Thanks.
-- Max

Re: [PATCH] ASoC: add xtensa xtfpga I2S interface and platform

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2939 bytes --]

On Tue, Oct 28, 2014 at 09:11:34PM +0300, Max Filippov wrote:
> On Tue, Oct 28, 2014 at 8:38 PM, Mark Brown <broonie@kernel.org> wrote:

> > You *really* need to explain how it's supposed to work - right now it's
> > not at all obvious, like I say the fact that this is a rarely used idiom
> > is not helping.  For example when we tear down the stream we just assign
> > the pointer in _stop() but don't bother with a sync until the stream is
> > closed - why?

> Because we can't wait in stop and syncing is not time critical, we can
> do it any time before the stream becomes invalid.

To be clear: the important part is that someone reading the code can
understand what's going on.

> >> > it'd be better to just add a DMA controller on the FPGA, everyone will
> >> > be much happier).

> >> Hardware people think different and it's been like that for more than 5
> >> years.

> > They appear to be the only hardware people who think this way, while we

> The whole audio subsystem on xtfpga boards is there for, I think, a single
> purpose: for the demonstration of CPU audio processing capabilities.
> That's why it's that simple.

This means that the demos include cycles spent taking interrupts and
stuffing data into the FIFO (with associated cache impacts) instead of
signal processing which isn't usually helpful for benchmarks.

> >> hw_params callback can change MCLK rate, so it has to disable and
> >> enable the clock anyway, and since enable can fail it does not guarantee
> >> that the clock will be left in the same state. Or should I adjust MCLK rate
> >> w/o disabling the clock?

> > So yet again: why not just enable the clock only when the device is in
> > use?  If it's being configured it stands to reason that the device isn't
> > actively in use...

> Mark, I don't get it, sorry ): My clock synthesizer is I2C controlled, so
> I can't prepare/unprepare it in the trigger callback. When should I do it?

Runtime PM is the normal way of doing it.

> >> The level field in the control register is 4 bit wide, so the allowed range of
> >> level is 0..15. FIFO size is 8192 entries, level = 1 corresponds to
> >> FIFO size / 2, level = 14 -- to FIFO size of 0. I guess this function
> >> won't get period_size of 0?

> > So if the IP gets changed and the code gets blown up this could well
> > explode then...  which doesn't seem entirely unlikely considering this
> > is a FPGA platform so presumably this is easy to update.  To repeat this
> > is about clarity and the code looking like it's probably hiding bugs as
> > much as if the code actually works if you really sit down and study it.

> The calculation does not depend on the actual hardware, but on the
> constant definitions in the same file. They need to be updated if the
> hardware changes. I'll try to rewrite it in a cleaner way.

Right, my point is that if someone changes the hardware they'll just
update the constants and then things will break.

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

Re: [PATCH] ASoC: add xtensa xtfpga I2S interface and platform

[Max Filippov]

On Wed, Oct 29, 2014 at 12:34 AM, Mark Brown <broonie@kernel.org> wrote:
> On Tue, Oct 28, 2014 at 09:11:34PM +0300, Max Filippov wrote:
>> On Tue, Oct 28, 2014 at 8:38 PM, Mark Brown <broonie@kernel.org> wrote:
>
>> > You *really* need to explain how it's supposed to work - right now it's
>> > not at all obvious, like I say the fact that this is a rarely used idiom
>> > is not helping.  For example when we tear down the stream we just assign
>> > the pointer in _stop() but don't bother with a sync until the stream is
>> > closed - why?
>
>> Because we can't wait in stop and syncing is not time critical, we can
>> do it any time before the stream becomes invalid.
>
> To be clear: the important part is that someone reading the code can
> understand what's going on.

Ok, I'll change it.

>> >> hw_params callback can change MCLK rate, so it has to disable and
>> >> enable the clock anyway, and since enable can fail it does not guarantee
>> >> that the clock will be left in the same state. Or should I adjust MCLK rate
>> >> w/o disabling the clock?
>
>> > So yet again: why not just enable the clock only when the device is in
>> > use?  If it's being configured it stands to reason that the device isn't
>> > actively in use...
>
>> Mark, I don't get it, sorry ): My clock synthesizer is I2C controlled, so
>> I can't prepare/unprepare it in the trigger callback. When should I do it?
>
> Runtime PM is the normal way of doing it.

Ok, thanks.

>> >> The level field in the control register is 4 bit wide, so the allowed range of
>> >> level is 0..15. FIFO size is 8192 entries, level = 1 corresponds to
>> >> FIFO size / 2, level = 14 -- to FIFO size of 0. I guess this function
>> >> won't get period_size of 0?
>
>> > So if the IP gets changed and the code gets blown up this could well
>> > explode then...  which doesn't seem entirely unlikely considering this
>> > is a FPGA platform so presumably this is easy to update.  To repeat this
>> > is about clarity and the code looking like it's probably hiding bugs as
>> > much as if the code actually works if you really sit down and study it.
>
>> The calculation does not depend on the actual hardware, but on the
>> constant definitions in the same file. They need to be updated if the
>> hardware changes. I'll try to rewrite it in a cleaner way.
>
> Right, my point is that if someone changes the hardware they'll just
> update the constants and then things will break.

Ok, I've rewritten it in a safer manner.

-- 
Thanks.
-- Max

Re: [PATCH] ASoC: add xtensa xtfpga I2S interface and platform

[Max Filippov]

On Tue, Oct 28, 2014 at 8:38 PM, Mark Brown <broonie@kernel.org> wrote:
> On Tue, Oct 28, 2014 at 08:00:45PM +0300, Max Filippov wrote:
>> On Tue, Oct 28, 2014 at 6:42 PM, Mark Brown <broonie@kernel.org> wrote:
>> >> > I would also expect to see the data transfer and I2S bits more split
>> >> > out, presumably this IP can actually be used in designs with a DMA
>> >> > controller and one would expect that for practical systems this would be
>> >> > the common case?
>
>> >> I don't know of other designs that use this IP block. Can we do it when
>> >> we get such users?
>
>> > It's also about ensuring that the code is cleanly split up so that
>> > someone can actually go in and add the required support later (and TBH
>
>> Can you point me to an example of such split, so that I don't write it in
>> an unusual way?
>
> Essentially all drivers are split this way...

But all of them have DMA registers and I2S registers completely separated,
right? How do I share registers between pcm and i2s parts?

-- 
Thanks.
-- Max

Re: [PATCH] ASoC: add xtensa xtfpga I2S interface and platform

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 836 bytes --]

On Wed, Oct 29, 2014 at 05:23:15PM +0300, Max Filippov wrote:
> On Tue, Oct 28, 2014 at 8:38 PM, Mark Brown <broonie@kernel.org> wrote:
> > On Tue, Oct 28, 2014 at 08:00:45PM +0300, Max Filippov wrote:

> >> > It's also about ensuring that the code is cleanly split up so that
> >> > someone can actually go in and add the required support later (and TBH

> >> Can you point me to an example of such split, so that I don't write it in
> >> an unusual way?

> > Essentially all drivers are split this way...

> But all of them have DMA registers and I2S registers completely separated,
> right? How do I share registers between pcm and i2s parts?

Just keep the code physically separate so the DMA bits are hanging off
the DMA operations and the interface setup bits are hanging off the DAI
operations rather than all mixed in together.

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

Re: [PATCH] ASoC: add xtensa xtfpga I2S interface and platform

[Max Filippov]

On Thu, Oct 30, 2014 at 12:02 AM, Mark Brown <broonie@kernel.org> wrote:
> On Wed, Oct 29, 2014 at 05:23:15PM +0300, Max Filippov wrote:
>> On Tue, Oct 28, 2014 at 8:38 PM, Mark Brown <broonie@kernel.org> wrote:
>> > On Tue, Oct 28, 2014 at 08:00:45PM +0300, Max Filippov wrote:
>
>> >> > It's also about ensuring that the code is cleanly split up so that
>> >> > someone can actually go in and add the required support later (and TBH
>
>> >> Can you point me to an example of such split, so that I don't write it in
>> >> an unusual way?
>
>> > Essentially all drivers are split this way...
>
>> But all of them have DMA registers and I2S registers completely separated,
>> right? How do I share registers between pcm and i2s parts?
>
> Just keep the code physically separate so the DMA bits are hanging off
> the DMA operations and the interface setup bits are hanging off the DAI
> operations rather than all mixed in together.

Ok. Should I also move them to a separate file, or one file is good for now?

-- 
Thanks.
-- Max

Re: [PATCH] ASoC: add xtensa xtfpga I2S interface and platform

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 472 bytes --]

On Thu, Oct 30, 2014 at 12:10:56AM +0300, Max Filippov wrote:
> On Thu, Oct 30, 2014 at 12:02 AM, Mark Brown <broonie@kernel.org> wrote:

> > Just keep the code physically separate so the DMA bits are hanging off
> > the DMA operations and the interface setup bits are hanging off the DAI
> > operations rather than all mixed in together.

> Ok. Should I also move them to a separate file, or one file is good for now?

It's probably not essential to split at this point.

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

Re: [alsa-devel] [PATCH] ASoC: add xtensa xtfpga I2S interface and platform

[Lars-Peter Clausen]

On 10/27/2014 08:07 PM, Max Filippov wrote:

A few minor things.

[...]
> +static irqreturn_t xtfpga_i2s_interrupt(int irq, void *dev_id)
> +{
> +	struct xtfpga_i2s *i2s = dev_id;
> +	struct snd_pcm_substream *tx_substream;
> +	int tx_active;
> +	unsigned int_status;
> +
> +	regmap_read(i2s->regmap, XTFPGA_I2S_INT_STATUS, &int_status);
> +	if (!(int_status & XTFPGA_I2S_INT_VALID))
> +		return IRQ_NONE;
> +
> +	if (int_status & XTFPGA_I2S_INT_UNDERRUN) {
> +		i2s->tx_fifo_sz = 0;
> +		regmap_update_bits(i2s->regmap, XTFPGA_I2S_CONFIG,
> +				   XTFPGA_I2S_CONFIG_INT_ENABLE |
> +				   XTFPGA_I2S_CONFIG_TX_ENABLE, 0);
> +	} else {
> +		i2s->tx_fifo_sz = i2s->tx_fifo_low;
> +		regmap_update_bits(i2s->regmap, XTFPGA_I2S_CONFIG,
> +				   XTFPGA_I2S_CONFIG_INT_ENABLE, 0);
> +	}
> +
> +	rcu_read_lock();
> +	tx_substream = rcu_dereference(i2s->tx_substream);
> +	tx_active = tx_substream && snd_pcm_running(tx_substream);
> +	if (tx_active) {
> +		snd_pcm_period_elapsed(tx_substream);
> +		if (int_status & XTFPGA_I2S_INT_UNDERRUN)
> +			dev_dbg_ratelimited(i2s->dev, "%s: underrun\n",
> +					    __func__);
> +		tx_substream = rcu_dereference(i2s->tx_substream);
> +		tx_active = tx_substream && snd_pcm_running(tx_substream);
> +	}
> +	rcu_read_unlock();
> +
> +	if (tx_active) {
> +		if (i2s->tx_fifo_high < 256)
> +			xtfpga_i2s_refill_fifo(i2s);
> +		else
> +			tasklet_hi_schedule(&i2s->refill_fifo);

Maybe use threaded IRQs instead of IRQ + tasklet.

> +	} else {
> +		xtfpga_i2s_irq_update_config(i2s, int_status);
> +	}
> +
> +	return IRQ_HANDLED;
> +}
[...]
> +
> +static int xtfpga_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
> +{
> +	int ret;
> +
> +	switch (cmd) {
> +	case SNDRV_PCM_TRIGGER_START:
> +	case SNDRV_PCM_TRIGGER_RESUME:
> +	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> +	case SNDRV_PCM_TRIGGER_STOP:
> +	case SNDRV_PCM_TRIGGER_SUSPEND:
> +	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
> +		ret = 0;
> +		break;
> +
> +	default:
> +		ret = -EINVAL;
> +		break;
> +	}
> +	return ret;
> +}

If you don't do anything you don't need the handler. The core will handle 
things if it is NULL.

> +static struct snd_pcm_ops xtfpga_pcm_ops = {

const

> +	.open		= xtfpga_pcm_open,
> +	.close		= xtfpga_pcm_close,
> +	.ioctl		= snd_pcm_lib_ioctl,
> +	.hw_params	= xtfpga_pcm_hw_params,
> +	.trigger	= xtfpga_pcm_trigger,
> +	.pointer	= xtfpga_pcm_pointer,
> +};
[...]
> +static int xtfpga_i2s_probe(struct platform_device *pdev)
> +{
> +	struct xtfpga_i2s *i2s;
> +	struct resource *mem, *irq;
> +	int err;
> +
> +	i2s = devm_kzalloc(&pdev->dev, sizeof(*i2s), GFP_KERNEL);
> +	if (!i2s) {
> +		err = -ENOMEM;
> +		goto err;
> +	}
> +	dev_set_drvdata(&pdev->dev, i2s);

platform_set_drvdata(...)

> +	i2s->dev = &pdev->dev;
> +	dev_dbg(&pdev->dev, "dev: %p, i2s: %p\n", &pdev->dev, i2s);
> +
> +	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!mem) {
> +		dev_err(&pdev->dev, "No memory resource\n");
> +		err = -ENODEV;
> +		goto err;
> +	}

devm_ioremap_resource will check if mem is NULL and print a error message, 
so the check above can be removed.

> +	i2s->regs = devm_ioremap_resource(&pdev->dev, mem);
> +	if (IS_ERR(i2s->regs)) {
> +		err = PTR_ERR(i2s->regs);
> +		goto err;
> +	}
> +
> +	i2s->regmap = devm_regmap_init_mmio(&pdev->dev, i2s->regs,
> +					    &xtfpga_i2s_regmap_config);
> +	if (IS_ERR(i2s->regmap)) {
> +		dev_err(&pdev->dev, "regmap init failed\n");
> +		err = PTR_ERR(i2s->regmap);
> +		goto err;
> +	}
> +
> +	i2s->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(i2s->clk)) {
> +		dev_err(&pdev->dev, "couldn't get clock\n");
> +		err = PTR_ERR(i2s->clk);
> +		goto err;
> +	}
> +
> +	irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);

platform_get_irq(...)

> +	if (!irq) {
> +		dev_err(&pdev->dev, "No IRQ resource\n");
> +		err = -ENODEV;
> +		goto err;
> +	}
> +	err = devm_request_irq(&pdev->dev, irq->start, xtfpga_i2s_interrupt,
> +			       IRQF_SHARED, pdev->name, i2s);
> +	if (err < 0) {
> +		dev_err(&pdev->dev, "request_irq failed\n");
> +		goto err;
> +	}
> +	tasklet_init(&i2s->refill_fifo, xtfpga_i2s_refill_fifo_tasklet,
> +		     (unsigned long)i2s);
> +

Since the tasklet is is used in the interrupt handler it should initialized 
before the IRQ is requested.


> +	regmap_write(i2s->regmap, XTFPGA_I2S_CONFIG,
> +		     (0x1 << XTFPGA_I2S_CONFIG_CHANNEL_BASE));
> +	regmap_write(i2s->regmap, XTFPGA_I2S_INT_STATUS, XTFPGA_I2S_INT_VALID);
> +	regmap_write(i2s->regmap, XTFPGA_I2S_INT_MASK, XTFPGA_I2S_INT_UNDERRUN);
> +
> +	err = snd_soc_register_platform(&pdev->dev, &xtfpga_soc_platform);
> +	if (err < 0) {
> +		dev_err(&pdev->dev, "couldn't register platform\n");
> +		goto err;
> +	}
> +	err = devm_snd_soc_register_component(&pdev->dev,
> +					      &xtfpga_i2s_component,
> +					      xtfpga_i2s_dai,
> +					      ARRAY_SIZE(xtfpga_i2s_dai));
> +	if (err < 0)
> +		dev_err(&pdev->dev, "couldn't register component\n");
> +err:
> +	if (err)
> +		dev_err(&pdev->dev, "%s: err = %d\n", __func__, err);
> +	return err;
> +}
> +
> +static int xtfpga_i2s_remove(struct platform_device *pdev)
> +{
> +	struct xtfpga_i2s *i2s = dev_get_drvdata(&pdev->dev);
> +
> +	if (i2s) {

This will always be non NULL.

> +		tasklet_kill(&i2s->refill_fifo);
> +		if (i2s->regmap && !IS_ERR(i2s->regmap)) {
> +			regmap_write(i2s->regmap, XTFPGA_I2S_CONFIG, 0);
> +			regmap_write(i2s->regmap, XTFPGA_I2S_INT_MASK, 0);
> +			regmap_write(i2s->regmap, XTFPGA_I2S_INT_STATUS,
> +				     XTFPGA_I2S_INT_VALID);
> +		}
> +		if (i2s->clk_enabled)
> +			clk_disable_unprepare(i2s->clk);
> +	}
> +
> +	return 0;
> +}
[...]

Re: [alsa-devel] [PATCH] ASoC: add xtensa xtfpga I2S interface and platform

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 489 bytes --]

On Tue, Oct 28, 2014 at 04:58:24PM +0100, Lars-Peter Clausen wrote:
> On 10/27/2014 08:07 PM, Max Filippov wrote:

> >+	if (tx_active) {
> >+		if (i2s->tx_fifo_high < 256)
> >+			xtfpga_i2s_refill_fifo(i2s);
> >+		else
> >+			tasklet_hi_schedule(&i2s->refill_fifo);

> Maybe use threaded IRQs instead of IRQ + tasklet.

Is that going to play nicely with the fact that the interrupt can be
shared and the desire to (AFAICT) do NAPI style stuff with the interrupt
disabled for long periods?

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

Re: [alsa-devel] [PATCH] ASoC: add xtensa xtfpga I2S interface and platform

[Lars-Peter Clausen]

On 10/28/2014 05:04 PM, Mark Brown wrote:
> On Tue, Oct 28, 2014 at 04:58:24PM +0100, Lars-Peter Clausen wrote:
>> On 10/27/2014 08:07 PM, Max Filippov wrote:
>
>>> +	if (tx_active) {
>>> +		if (i2s->tx_fifo_high < 256)
>>> +			xtfpga_i2s_refill_fifo(i2s);
>>> +		else
>>> +			tasklet_hi_schedule(&i2s->refill_fifo);
>
>> Maybe use threaded IRQs instead of IRQ + tasklet.
>
> Is that going to play nicely with the fact that the interrupt can be
> shared and the desire to (AFAICT) do NAPI style stuff with the interrupt
> disabled for long periods?
>

Threaded interrupts got support for interrupt sharing a while ago, so I 
guess yes. I think it will even work better than the tasklet approach. You 
can configure the IRQ to disable itself as long as the thread is running.

- Lars

Re: [alsa-devel] [PATCH] ASoC: add xtensa xtfpga I2S interface and platform

[Takashi Iwai]

At Tue, 28 Oct 2014 17:39:13 +0100,
Lars-Peter Clausen wrote:
> 
> On 10/28/2014 05:04 PM, Mark Brown wrote:
> > On Tue, Oct 28, 2014 at 04:58:24PM +0100, Lars-Peter Clausen wrote:
> >> On 10/27/2014 08:07 PM, Max Filippov wrote:
> >
> >>> +	if (tx_active) {
> >>> +		if (i2s->tx_fifo_high < 256)
> >>> +			xtfpga_i2s_refill_fifo(i2s);
> >>> +		else
> >>> +			tasklet_hi_schedule(&i2s->refill_fifo);
> >
> >> Maybe use threaded IRQs instead of IRQ + tasklet.
> >
> > Is that going to play nicely with the fact that the interrupt can be
> > shared and the desire to (AFAICT) do NAPI style stuff with the interrupt
> > disabled for long periods?
> >
> 
> Threaded interrupts got support for interrupt sharing a while ago, so I 
> guess yes. I think it will even work better than the tasklet approach. You 
> can configure the IRQ to disable itself as long as the thread is running.

Yes, I tested the threaded irq with PCI drivers using shared irqs and
worked well. 


Takashi

Re: [alsa-devel] [PATCH] ASoC: add xtensa xtfpga I2S interface and platform

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 841 bytes --]

On Tue, Oct 28, 2014 at 05:39:13PM +0100, Lars-Peter Clausen wrote:
> On 10/28/2014 05:04 PM, Mark Brown wrote:

> >Is that going to play nicely with the fact that the interrupt can be
> >shared and the desire to (AFAICT) do NAPI style stuff with the interrupt
> >disabled for long periods?

> Threaded interrupts got support for interrupt sharing a while ago, so I
> guess yes. I think it will even work better than the tasklet approach. You
> can configure the IRQ to disable itself as long as the thread is running.

I know you *can* share threaded interrupts, I'm just not sure that
hogging the threaded handler plays nicely with other users.  Though now
I look at this again tx_fifo_high is a "constant" so it's not actually
trying to do a NAPI type mitigation thing and this is really just an
open coded threaded interrupt as you say.

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

Re: [alsa-devel] [PATCH] ASoC: add xtensa xtfpga I2S interface and platform

[Max Filippov]

Hi Lars-Peter,

On Tue, Oct 28, 2014 at 6:58 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
> On 10/27/2014 08:07 PM, Max Filippov wrote:
>
> A few minor things.
>
> [...]
>
>> +static irqreturn_t xtfpga_i2s_interrupt(int irq, void *dev_id)
>> +       if (tx_active) {
>> +               if (i2s->tx_fifo_high < 256)
>> +                       xtfpga_i2s_refill_fifo(i2s);
>> +               else
>> +                       tasklet_hi_schedule(&i2s->refill_fifo);
>
> Maybe use threaded IRQs instead of IRQ + tasklet.

Ok, I'll look at it.

>> +static int xtfpga_pcm_trigger(struct snd_pcm_substream *substream, int

> If you don't do anything you don't need the handler. The core will handle
> things if it is NULL.

Ok.

>> +static struct snd_pcm_ops xtfpga_pcm_ops = {
>
> const

Ok.

>> +static int xtfpga_i2s_probe(struct platform_device *pdev)
>> +       dev_set_drvdata(&pdev->dev, i2s);
>
> platform_set_drvdata(...)

Ok.

>> +       i2s->dev = &pdev->dev;
>> +       dev_dbg(&pdev->dev, "dev: %p, i2s: %p\n", &pdev->dev, i2s);
>> +
>> +       mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +       if (!mem) {
>> +               dev_err(&pdev->dev, "No memory resource\n");
>> +               err = -ENODEV;
>> +               goto err;
>> +       }
>
> devm_ioremap_resource will check if mem is NULL and print a error message,
> so the check above can be removed.

Ok.

>> +       irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>
> platform_get_irq(...)

Ok.

>> +       err = devm_request_irq(&pdev->dev, irq->start,
>> xtfpga_i2s_interrupt,
>> +                              IRQF_SHARED, pdev->name, i2s);
>> +       if (err < 0) {
>> +               dev_err(&pdev->dev, "request_irq failed\n");
>> +               goto err;
>> +       }
>> +       tasklet_init(&i2s->refill_fifo, xtfpga_i2s_refill_fifo_tasklet,
>> +                    (unsigned long)i2s);
>> +
>
>
> Since the tasklet is is used in the interrupt handler it should initialized
> before the IRQ is requested.

Ok.

>> +static int xtfpga_i2s_remove(struct platform_device *pdev)
>> +{
>> +       struct xtfpga_i2s *i2s = dev_get_drvdata(&pdev->dev);
>> +
>> +       if (i2s) {
>
>
> This will always be non NULL.

Ok.

-- 
Thanks.
-- Max