[PATCH V2 0/3] ASOC: bcm2835: move bcm2835-i2s to use clock framework

[PATCH V2 0/3] ASOC: bcm2835: move bcm2835-i2s to use clock framework

[kernel at martin.sperl.org]

From: Martin Sperl <kernel@martin.sperl.org>

This patchset enables the bcm2835-i2s driver to use the clock
framework which was introduced with commit 94cb7f76caa0
("ARM: bcm2835: Switch to using the new clock driver support.").

This commit resulted in the fact that the bcm2835-i2s driver was
no longer working due to some register addresses used by 2 drivers
(clk-bcm2835 and bcm2835-i2s).

This patchset requires that the patchset for PCM-clock
support as well as fractional/mash support is applied to
the clk-bcm2835 driver and the corresponding device tree,
but as the current version of the driver is not working,
that should not be a problem.

Note that there is one change:
right now the current driver tries to calculate an optimal
bclk_ratio based on its knowledge that it is using the 19.2Mhz
oscillator. This computation would recommend the use of 40
or 80 bits instead of 32/64 bits that are required.

Some of the DACs can not handle this, so most downstream
drivers would set snd_soc_dai_set_bclk_ratio explicitly to
disable this "non-power-of-2" automatic selection.

So it seems wise to leave it out of the current patchset.

If it is deemed necessary, then I can provide a separate
patch that implements this again.

Changelog:
  V1->V2: * moving clock patches into a separate patchset,
            which fixes also other issues in the clock framework
          * remove unnecessary bclk_size assignments

Martin Sperl (3):
  ASoC: bcm2835: move to use the clock framework
  ARM: bcm2835: I2S: use new register-range and clock framework
  dt-bindings: bsm2835: fix bindings documentation to use new clock
    framework

 .../devicetree/bindings/sound/brcm,bcm2835-i2s.txt |    7 +-
 arch/arm/boot/dts/bcm2835.dtsi                     |    5 +-
 sound/soc/bcm/bcm2835-i2s.c                        |  284 +++++---------------
 3 files changed, 69 insertions(+), 227 deletions(-)

--
1.7.10.4

[PATCH V2 1/3] ASoC: bcm2835: move to use the clock framework

[kernel at martin.sperl.org]

From: Martin Sperl <kernel@martin.sperl.org>

Since the move to the new clock framework with commit 94cb7f76caa0
("ARM: bcm2835: Switch to using the new clock driver support.")
this driver was no longer functional as it was manipulating the
clock registers locally without going true the framework.

This patch moves to use the new clock framework and also
moves away from the hardcoded address offsets for DMA getting
the dma-address directly from the device tree.

Note that the optimal bclk_ratio selection to avoid jitter
due to the use of fractional dividers, which is in the
current version has been removed, because not all devices
support these non power of 2 sized transfers, which resulted
in lots of (downstream) modules that use:
  snd_soc_dai_set_bclk_ratio(cpu_dai, sample_bits * 2);

Signed-off-by: Martin Sperl <kernel@martin.sperl.org>
---
 sound/soc/bcm/bcm2835-i2s.c |  284 ++++++++++---------------------------------
 1 file changed, 64 insertions(+), 220 deletions(-)

diff --git a/sound/soc/bcm/bcm2835-i2s.c b/sound/soc/bcm/bcm2835-i2s.c
index 3303d5f..1c1f221 100644
--- a/sound/soc/bcm/bcm2835-i2s.c
+++ b/sound/soc/bcm/bcm2835-i2s.c
@@ -37,6 +37,7 @@
 #include <linux/init.h>
 #include <linux/io.h>
 #include <linux/module.h>
+#include <linux/of_address.h>
 #include <linux/slab.h>
 
 #include <sound/core.h>
@@ -46,55 +47,6 @@
 #include <sound/pcm_params.h>
 #include <sound/soc.h>
 
-/* Clock registers */
-#define BCM2835_CLK_PCMCTL_REG  0x00
-#define BCM2835_CLK_PCMDIV_REG  0x04
-
-/* Clock register settings */
-#define BCM2835_CLK_PASSWD		(0x5a000000)
-#define BCM2835_CLK_PASSWD_MASK	(0xff000000)
-#define BCM2835_CLK_MASH(v)		((v) << 9)
-#define BCM2835_CLK_FLIP		BIT(8)
-#define BCM2835_CLK_BUSY		BIT(7)
-#define BCM2835_CLK_KILL		BIT(5)
-#define BCM2835_CLK_ENAB		BIT(4)
-#define BCM2835_CLK_SRC(v)		(v)
-
-#define BCM2835_CLK_SHIFT		(12)
-#define BCM2835_CLK_DIVI(v)		((v) << BCM2835_CLK_SHIFT)
-#define BCM2835_CLK_DIVF(v)		(v)
-#define BCM2835_CLK_DIVF_MASK		(0xFFF)
-
-enum {
-	BCM2835_CLK_MASH_0 = 0,
-	BCM2835_CLK_MASH_1,
-	BCM2835_CLK_MASH_2,
-	BCM2835_CLK_MASH_3,
-};
-
-enum {
-	BCM2835_CLK_SRC_GND = 0,
-	BCM2835_CLK_SRC_OSC,
-	BCM2835_CLK_SRC_DBG0,
-	BCM2835_CLK_SRC_DBG1,
-	BCM2835_CLK_SRC_PLLA,
-	BCM2835_CLK_SRC_PLLC,
-	BCM2835_CLK_SRC_PLLD,
-	BCM2835_CLK_SRC_HDMI,
-};
-
-/* Most clocks are not useable (freq = 0) */
-static const unsigned int bcm2835_clk_freq[BCM2835_CLK_SRC_HDMI+1] = {
-	[BCM2835_CLK_SRC_GND]		= 0,
-	[BCM2835_CLK_SRC_OSC]		= 19200000,
-	[BCM2835_CLK_SRC_DBG0]		= 0,
-	[BCM2835_CLK_SRC_DBG1]		= 0,
-	[BCM2835_CLK_SRC_PLLA]		= 0,
-	[BCM2835_CLK_SRC_PLLC]		= 0,
-	[BCM2835_CLK_SRC_PLLD]		= 500000000,
-	[BCM2835_CLK_SRC_HDMI]		= 0,
-};
-
 /* I2S registers */
 #define BCM2835_I2S_CS_A_REG		0x00
 #define BCM2835_I2S_FIFO_A_REG		0x04
@@ -158,10 +110,6 @@ static const unsigned int bcm2835_clk_freq[BCM2835_CLK_SRC_HDMI+1] = {
 #define BCM2835_I2S_INT_RXR		BIT(1)
 #define BCM2835_I2S_INT_TXW		BIT(0)
 
-/* I2S DMA interface */
-/* FIXME: Needs IOMMU support */
-#define BCM2835_VCMMU_SHIFT		(0x7E000000 - 0x20000000)
-
 /* General device struct */
 struct bcm2835_i2s_dev {
 	struct device				*dev;
@@ -169,21 +117,23 @@ struct bcm2835_i2s_dev {
 	unsigned int				fmt;
 	unsigned int				bclk_ratio;
 
-	struct regmap *i2s_regmap;
-	struct regmap *clk_regmap;
+	struct regmap				*i2s_regmap;
+	struct clk				*clk;
+	bool					clk_prepared;
 };
 
 static void bcm2835_i2s_start_clock(struct bcm2835_i2s_dev *dev)
 {
-	/* Start the clock if in master mode */
 	unsigned int master = dev->fmt & SND_SOC_DAIFMT_MASTER_MASK;
 
+	if (dev->clk_prepared)
+		return;
+
 	switch (master) {
 	case SND_SOC_DAIFMT_CBS_CFS:
 	case SND_SOC_DAIFMT_CBS_CFM:
-		regmap_update_bits(dev->clk_regmap, BCM2835_CLK_PCMCTL_REG,
-			BCM2835_CLK_PASSWD_MASK | BCM2835_CLK_ENAB,
-			BCM2835_CLK_PASSWD | BCM2835_CLK_ENAB);
+		clk_prepare_enable(dev->clk);
+		dev->clk_prepared = true;
 		break;
 	default:
 		break;
@@ -192,28 +142,9 @@ static void bcm2835_i2s_start_clock(struct bcm2835_i2s_dev *dev)
 
 static void bcm2835_i2s_stop_clock(struct bcm2835_i2s_dev *dev)
 {
-	uint32_t clkreg;
-	int timeout = 1000;
-
-	/* Stop clock */
-	regmap_update_bits(dev->clk_regmap, BCM2835_CLK_PCMCTL_REG,
-			BCM2835_CLK_PASSWD_MASK | BCM2835_CLK_ENAB,
-			BCM2835_CLK_PASSWD);
-
-	/* Wait for the BUSY flag going down */
-	while (--timeout) {
-		regmap_read(dev->clk_regmap, BCM2835_CLK_PCMCTL_REG, &clkreg);
-		if (!(clkreg & BCM2835_CLK_BUSY))
-			break;
-	}
-
-	if (!timeout) {
-		/* KILL the clock */
-		dev_err(dev->dev, "I2S clock didn't stop. Kill the clock!\n");
-		regmap_update_bits(dev->clk_regmap, BCM2835_CLK_PCMCTL_REG,
-			BCM2835_CLK_KILL | BCM2835_CLK_PASSWD_MASK,
-			BCM2835_CLK_KILL | BCM2835_CLK_PASSWD);
-	}
+	if (dev->clk_prepared)
+		clk_disable_unprepare(dev->clk);
+	dev->clk_prepared = false;
 }
 
 static void bcm2835_i2s_clear_fifos(struct bcm2835_i2s_dev *dev,
@@ -223,8 +154,7 @@ static void bcm2835_i2s_clear_fifos(struct bcm2835_i2s_dev *dev,
 	uint32_t syncval;
 	uint32_t csreg;
 	uint32_t i2s_active_state;
-	uint32_t clkreg;
-	uint32_t clk_active_state;
+	bool clk_was_prepared;
 	uint32_t off;
 	uint32_t clr;
 
@@ -238,15 +168,10 @@ static void bcm2835_i2s_clear_fifos(struct bcm2835_i2s_dev *dev,
 	regmap_read(dev->i2s_regmap, BCM2835_I2S_CS_A_REG, &csreg);
 	i2s_active_state = csreg & (BCM2835_I2S_RXON | BCM2835_I2S_TXON);
 
-	regmap_read(dev->clk_regmap, BCM2835_CLK_PCMCTL_REG, &clkreg);
-	clk_active_state = clkreg & BCM2835_CLK_ENAB;
-
 	/* Start clock if not running */
-	if (!clk_active_state) {
-		regmap_update_bits(dev->clk_regmap, BCM2835_CLK_PCMCTL_REG,
-			BCM2835_CLK_PASSWD_MASK | BCM2835_CLK_ENAB,
-			BCM2835_CLK_PASSWD | BCM2835_CLK_ENAB);
-	}
+	clk_was_prepared = dev->clk_prepared;
+	if (!clk_was_prepared)
+		bcm2835_i2s_start_clock(dev);
 
 	/* Stop I2S module */
 	regmap_update_bits(dev->i2s_regmap, BCM2835_I2S_CS_A_REG, off, 0);
@@ -280,7 +205,7 @@ static void bcm2835_i2s_clear_fifos(struct bcm2835_i2s_dev *dev,
 		dev_err(dev->dev, "I2S SYNC error!\n");
 
 	/* Stop clock if it was not running before */
-	if (!clk_active_state)
+	if (!clk_was_prepared)
 		bcm2835_i2s_stop_clock(dev);
 
 	/* Restore I2S state */
@@ -309,19 +234,9 @@ static int bcm2835_i2s_hw_params(struct snd_pcm_substream *substream,
 				 struct snd_soc_dai *dai)
 {
 	struct bcm2835_i2s_dev *dev = snd_soc_dai_get_drvdata(dai);
-
 	unsigned int sampling_rate = params_rate(params);
 	unsigned int data_length, data_delay, bclk_ratio;
 	unsigned int ch1pos, ch2pos, mode, format;
-	unsigned int mash = BCM2835_CLK_MASH_1;
-	unsigned int divi, divf, target_frequency;
-	int clk_src = -1;
-	unsigned int master = dev->fmt & SND_SOC_DAIFMT_MASTER_MASK;
-	bool bit_master =	(master == SND_SOC_DAIFMT_CBS_CFS
-					|| master == SND_SOC_DAIFMT_CBS_CFM);
-
-	bool frame_master =	(master == SND_SOC_DAIFMT_CBS_CFS
-					|| master == SND_SOC_DAIFMT_CBM_CFS);
 	uint32_t csreg;
 
 	/*
@@ -343,11 +258,9 @@ static int bcm2835_i2s_hw_params(struct snd_pcm_substream *substream,
 	switch (params_format(params)) {
 	case SNDRV_PCM_FORMAT_S16_LE:
 		data_length = 16;
-		bclk_ratio = 40;
 		break;
 	case SNDRV_PCM_FORMAT_S32_LE:
 		data_length = 32;
-		bclk_ratio = 80;
 		break;
 	default:
 		return -EINVAL;
@@ -356,69 +269,12 @@ static int bcm2835_i2s_hw_params(struct snd_pcm_substream *substream,
 	/* If bclk_ratio already set, use that one. */
 	if (dev->bclk_ratio)
 		bclk_ratio = dev->bclk_ratio;
+	else
+		/* otherwise calculate a fitting block ratio */
+		bclk_ratio = 2 * data_length;
 
-	/*
-	 * Clock Settings
-	 *
-	 * The target frequency of the bit clock is
-	 *	sampling rate * frame length
-	 *
-	 * Integer mode:
-	 * Sampling rates that are multiples of 8000 kHz
-	 * can be driven by the oscillator of 19.2 MHz
-	 * with an integer divider as long as the frame length
-	 * is an integer divider of 19200000/8000=2400 as set up above.
-	 * This is no longer possible if the sampling rate
-	 * is too high (e.g. 192 kHz), because the oscillator is too slow.
-	 *
-	 * MASH mode:
-	 * For all other sampling rates, it is not possible to
-	 * have an integer divider. Approximate the clock
-	 * with the MASH module that induces a slight frequency
-	 * variance. To minimize that it is best to have the fastest
-	 * clock here. That is PLLD with 500 MHz.
-	 */
-	target_frequency = sampling_rate * bclk_ratio;
-	clk_src = BCM2835_CLK_SRC_OSC;
-	mash = BCM2835_CLK_MASH_0;
-
-	if (bcm2835_clk_freq[clk_src] % target_frequency == 0
-			&& bit_master && frame_master) {
-		divi = bcm2835_clk_freq[clk_src] / target_frequency;
-		divf = 0;
-	} else {
-		uint64_t dividend;
-
-		if (!dev->bclk_ratio) {
-			/*
-			 * Overwrite bclk_ratio, because the
-			 * above trick is not needed or can
-			 * not be used.
-			 */
-			bclk_ratio = 2 * data_length;
-		}
-
-		target_frequency = sampling_rate * bclk_ratio;
-
-		clk_src = BCM2835_CLK_SRC_PLLD;
-		mash = BCM2835_CLK_MASH_1;
-
-		dividend = bcm2835_clk_freq[clk_src];
-		dividend <<= BCM2835_CLK_SHIFT;
-		do_div(dividend, target_frequency);
-		divi = dividend >> BCM2835_CLK_SHIFT;
-		divf = dividend & BCM2835_CLK_DIVF_MASK;
-	}
-
-	/* Set clock divider */
-	regmap_write(dev->clk_regmap, BCM2835_CLK_PCMDIV_REG, BCM2835_CLK_PASSWD
-			| BCM2835_CLK_DIVI(divi)
-			| BCM2835_CLK_DIVF(divf));
-
-	/* Setup clock, but don't start it yet */
-	regmap_write(dev->clk_regmap, BCM2835_CLK_PCMCTL_REG, BCM2835_CLK_PASSWD
-			| BCM2835_CLK_MASH(mash)
-			| BCM2835_CLK_SRC(clk_src));
+	/* set target clock rate*/
+	clk_set_rate(dev->clk, sampling_rate * bclk_ratio);
 
 	/* Setup the frame format */
 	format = BCM2835_I2S_CHEN;
@@ -692,7 +548,7 @@ static const struct snd_soc_dai_ops bcm2835_i2s_dai_ops = {
 	.trigger	= bcm2835_i2s_trigger,
 	.hw_params	= bcm2835_i2s_hw_params,
 	.set_fmt	= bcm2835_i2s_set_dai_fmt,
-	.set_bclk_ratio	= bcm2835_i2s_set_dai_bclk_ratio
+	.set_bclk_ratio	= bcm2835_i2s_set_dai_bclk_ratio,
 };
 
 static int bcm2835_i2s_dai_probe(struct snd_soc_dai *dai)
@@ -750,34 +606,14 @@ static bool bcm2835_i2s_precious_reg(struct device *dev, unsigned int reg)
 	};
 }
 
-static bool bcm2835_clk_volatile_reg(struct device *dev, unsigned int reg)
-{
-	switch (reg) {
-	case BCM2835_CLK_PCMCTL_REG:
-		return true;
-	default:
-		return false;
-	};
-}
-
-static const struct regmap_config bcm2835_regmap_config[] = {
-	{
-		.reg_bits = 32,
-		.reg_stride = 4,
-		.val_bits = 32,
-		.max_register = BCM2835_I2S_GRAY_REG,
-		.precious_reg = bcm2835_i2s_precious_reg,
-		.volatile_reg = bcm2835_i2s_volatile_reg,
-		.cache_type = REGCACHE_RBTREE,
-	},
-	{
-		.reg_bits = 32,
-		.reg_stride = 4,
-		.val_bits = 32,
-		.max_register = BCM2835_CLK_PCMDIV_REG,
-		.volatile_reg = bcm2835_clk_volatile_reg,
-		.cache_type = REGCACHE_RBTREE,
-	},
+static const struct regmap_config bcm2835_regmap_config = {
+	.reg_bits = 32,
+	.reg_stride = 4,
+	.val_bits = 32,
+	.max_register = BCM2835_I2S_GRAY_REG,
+	.precious_reg = bcm2835_i2s_precious_reg,
+	.volatile_reg = bcm2835_i2s_volatile_reg,
+	.cache_type = REGCACHE_RBTREE,
 };
 
 static const struct snd_soc_component_driver bcm2835_i2s_component = {
@@ -787,42 +623,50 @@ static const struct snd_soc_component_driver bcm2835_i2s_component = {
 static int bcm2835_i2s_probe(struct platform_device *pdev)
 {
 	struct bcm2835_i2s_dev *dev;
-	int i;
 	int ret;
-	struct regmap *regmap[2];
-	struct resource *mem[2];
-
-	/* Request both ioareas */
-	for (i = 0; i <= 1; i++) {
-		void __iomem *base;
-
-		mem[i] = platform_get_resource(pdev, IORESOURCE_MEM, i);
-		base = devm_ioremap_resource(&pdev->dev, mem[i]);
-		if (IS_ERR(base))
-			return PTR_ERR(base);
-
-		regmap[i] = devm_regmap_init_mmio(&pdev->dev, base,
-					    &bcm2835_regmap_config[i]);
-		if (IS_ERR(regmap[i]))
-			return PTR_ERR(regmap[i]);
-	}
+	struct resource *mem;
+	void __iomem *base;
+	const __be32 *addr;
+	dma_addr_t dma_base;
 
 	dev = devm_kzalloc(&pdev->dev, sizeof(*dev),
 			   GFP_KERNEL);
 	if (!dev)
 		return -ENOMEM;
 
-	dev->i2s_regmap = regmap[0];
-	dev->clk_regmap = regmap[1];
+	/* get the clock */
+	dev->clk_prepared = false;
+	dev->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(dev->clk)) {
+		dev_err(&pdev->dev, "could not get clk: %ld\n",
+			PTR_ERR(dev->clk));
+		return PTR_ERR(dev->clk);
+	}
+
+	/* Request ioarea */
+	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	base = devm_ioremap_resource(&pdev->dev, mem);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	dev->i2s_regmap = devm_regmap_init_mmio(&pdev->dev, base,
+				&bcm2835_regmap_config);
+	if (IS_ERR(dev->i2s_regmap))
+		return PTR_ERR(dev->i2s_regmap);
+
+	/* Set the DMA address - we have to parse DT ourselves */
+	addr = of_get_address(pdev->dev.of_node, 0, NULL, NULL);
+	if (!addr) {
+		dev_err(&pdev->dev, "could not get DMA-register address\n");
+		return -EINVAL;
+	}
+	dma_base = be32_to_cpup(addr);
 
-	/* Set the DMA address */
 	dev->dma_data[SNDRV_PCM_STREAM_PLAYBACK].addr =
-		(dma_addr_t)mem[0]->start + BCM2835_I2S_FIFO_A_REG
-					  + BCM2835_VCMMU_SHIFT;
+		dma_base + BCM2835_I2S_FIFO_A_REG;
 
 	dev->dma_data[SNDRV_PCM_STREAM_CAPTURE].addr =
-		(dma_addr_t)mem[0]->start + BCM2835_I2S_FIFO_A_REG
-					  + BCM2835_VCMMU_SHIFT;
+		dma_base + BCM2835_I2S_FIFO_A_REG;
 
 	/* Set the bus width */
 	dev->dma_data[SNDRV_PCM_STREAM_PLAYBACK].addr_width =
-- 
1.7.10.4

[PATCH V2 1/3] ASoC: bcm2835: move to use the clock framework

[Eric Anholt]

kernel at martin.sperl.org writes:

> From: Martin Sperl <kernel@martin.sperl.org>
>
> Since the move to the new clock framework with commit 94cb7f76caa0
> ("ARM: bcm2835: Switch to using the new clock driver support.")
> this driver was no longer functional as it was manipulating the
> clock registers locally without going true the framework.
>
> This patch moves to use the new clock framework and also
> moves away from the hardcoded address offsets for DMA getting
> the dma-address directly from the device tree.
>
> Note that the optimal bclk_ratio selection to avoid jitter
> due to the use of fractional dividers, which is in the
> current version has been removed, because not all devices
> support these non power of 2 sized transfers, which resulted
> in lots of (downstream) modules that use:
>   snd_soc_dai_set_bclk_ratio(cpu_dai, sample_bits * 2);
>
> Signed-off-by: Martin Sperl <kernel@martin.sperl.org>
> ---
>  sound/soc/bcm/bcm2835-i2s.c |  284 ++++++++++---------------------------------
>  1 file changed, 64 insertions(+), 220 deletions(-)
>
> diff --git a/sound/soc/bcm/bcm2835-i2s.c b/sound/soc/bcm/bcm2835-i2s.c
> index 3303d5f..1c1f221 100644
> --- a/sound/soc/bcm/bcm2835-i2s.c
> +++ b/sound/soc/bcm/bcm2835-i2s.c

> -	dev->i2s_regmap = regmap[0];
> -	dev->clk_regmap = regmap[1];
> +	/* get the clock */
> +	dev->clk_prepared = false;
> +	dev->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(dev->clk)) {
> +		dev_err(&pdev->dev, "could not get clk: %ld\n",
> +			PTR_ERR(dev->clk));
> +		return PTR_ERR(dev->clk);
> +	}
> +
> +	/* Request ioarea */
> +	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	base = devm_ioremap_resource(&pdev->dev, mem);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	dev->i2s_regmap = devm_regmap_init_mmio(&pdev->dev, base,
> +				&bcm2835_regmap_config);
> +	if (IS_ERR(dev->i2s_regmap))
> +		return PTR_ERR(dev->i2s_regmap);
> +
> +	/* Set the DMA address - we have to parse DT ourselves */
> +	addr = of_get_address(pdev->dev.of_node, 0, NULL, NULL);
> +	if (!addr) {
> +		dev_err(&pdev->dev, "could not get DMA-register address\n");
> +		return -EINVAL;
> +	}
> +	dma_base = be32_to_cpup(addr);

Why aren't we just using mem->start like before?  That seems like an
independent change that should be justified on its own.  I'd be ready to
ack the patch if that change is removed.

I'm also trying to work my way through the clock changes.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160128/30a3848e/attachment.sig>

[PATCH V2 1/3] ASoC: bcm2835: move to use the clock framework

[Martin Sperl]

On 28.01.2016 23:08, Eric Anholt wrote:
> kernel at martin.sperl.org writes:
>
>> From: Martin Sperl <kernel@martin.sperl.org>
>>
>> Since the move to the new clock framework with commit 94cb7f76caa0
>> ("ARM: bcm2835: Switch to using the new clock driver support.")
>> this driver was no longer functional as it was manipulating the
>> clock registers locally without going true the framework.
>>
>> This patch moves to use the new clock framework and also
>> moves away from the hardcoded address offsets for DMA getting
>> the dma-address directly from the device tree.
>>
>> Note that the optimal bclk_ratio selection to avoid jitter
>> due to the use of fractional dividers, which is in the
>> current version has been removed, because not all devices
>> support these non power of 2 sized transfers, which resulted
>> in lots of (downstream) modules that use:
>>    snd_soc_dai_set_bclk_ratio(cpu_dai, sample_bits * 2);
>>
>> Signed-off-by: Martin Sperl <kernel@martin.sperl.org>
>> ---
>>   sound/soc/bcm/bcm2835-i2s.c |  284 ++++++++++---------------------------------
>>   1 file changed, 64 insertions(+), 220 deletions(-)
>>
>> diff --git a/sound/soc/bcm/bcm2835-i2s.c b/sound/soc/bcm/bcm2835-i2s.c
>> index 3303d5f..1c1f221 100644
>> --- a/sound/soc/bcm/bcm2835-i2s.c
>> +++ b/sound/soc/bcm/bcm2835-i2s.c
>
>> -	dev->i2s_regmap = regmap[0];
>> -	dev->clk_regmap = regmap[1];
>> +	/* get the clock */
>> +	dev->clk_prepared = false;
>> +	dev->clk = devm_clk_get(&pdev->dev, NULL);
>> +	if (IS_ERR(dev->clk)) {
>> +		dev_err(&pdev->dev, "could not get clk: %ld\n",
>> +			PTR_ERR(dev->clk));
>> +		return PTR_ERR(dev->clk);
>> +	}
>> +
>> +	/* Request ioarea */
>> +	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	base = devm_ioremap_resource(&pdev->dev, mem);
>> +	if (IS_ERR(base))
>> +		return PTR_ERR(base);
>> +
>> +	dev->i2s_regmap = devm_regmap_init_mmio(&pdev->dev, base,
>> +				&bcm2835_regmap_config);
>> +	if (IS_ERR(dev->i2s_regmap))
>> +		return PTR_ERR(dev->i2s_regmap);
>> +
>> +	/* Set the DMA address - we have to parse DT ourselves */
>> +	addr = of_get_address(pdev->dev.of_node, 0, NULL, NULL);
>> +	if (!addr) {
>> +		dev_err(&pdev->dev, "could not get DMA-register address\n");
>> +		return -EINVAL;
>> +	}
>> +	dma_base = be32_to_cpup(addr);
>
> Why aren't we just using mem->start like before?  That seems like an
> independent change that should be justified on its own.  I'd be ready to
> ack the patch if that change is removed.
>

Problem is that we need the VC4 bus-address (0x7e203000),
which is the actual <reg> value from the device tree without any
mapping.

Not the ARM MMU visible address mappings that mem->start provides
(typically 0x20203000 or 0x3f203000 for bcm2836)

Nor the mapped address (base) available in the kernel (typically
0xdc......).

This is the only way to get it correctly and has been done the same way
with spi-bcm2835.

[PATCH V2 1/3] ASoC: bcm2835: move to use the clock framework

[Eric Anholt]

Martin Sperl <kernel@martin.sperl.org> writes:

> On 28.01.2016 23:08, Eric Anholt wrote:
>> kernel at martin.sperl.org writes:
>>
>>> From: Martin Sperl <kernel@martin.sperl.org>
>>>
>>> Since the move to the new clock framework with commit 94cb7f76caa0
>>> ("ARM: bcm2835: Switch to using the new clock driver support.")
>>> this driver was no longer functional as it was manipulating the
>>> clock registers locally without going true the framework.
>>>
>>> This patch moves to use the new clock framework and also
>>> moves away from the hardcoded address offsets for DMA getting
>>> the dma-address directly from the device tree.
>>>
>>> Note that the optimal bclk_ratio selection to avoid jitter
>>> due to the use of fractional dividers, which is in the
>>> current version has been removed, because not all devices
>>> support these non power of 2 sized transfers, which resulted
>>> in lots of (downstream) modules that use:
>>>    snd_soc_dai_set_bclk_ratio(cpu_dai, sample_bits * 2);
>>>
>>> Signed-off-by: Martin Sperl <kernel@martin.sperl.org>
>>> ---
>>>   sound/soc/bcm/bcm2835-i2s.c |  284 ++++++++++---------------------------------
>>>   1 file changed, 64 insertions(+), 220 deletions(-)
>>>
>>> diff --git a/sound/soc/bcm/bcm2835-i2s.c b/sound/soc/bcm/bcm2835-i2s.c
>>> index 3303d5f..1c1f221 100644
>>> --- a/sound/soc/bcm/bcm2835-i2s.c
>>> +++ b/sound/soc/bcm/bcm2835-i2s.c
>>
>>> -	dev->i2s_regmap = regmap[0];
>>> -	dev->clk_regmap = regmap[1];
>>> +	/* get the clock */
>>> +	dev->clk_prepared = false;
>>> +	dev->clk = devm_clk_get(&pdev->dev, NULL);
>>> +	if (IS_ERR(dev->clk)) {
>>> +		dev_err(&pdev->dev, "could not get clk: %ld\n",
>>> +			PTR_ERR(dev->clk));
>>> +		return PTR_ERR(dev->clk);
>>> +	}
>>> +
>>> +	/* Request ioarea */
>>> +	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> +	base = devm_ioremap_resource(&pdev->dev, mem);
>>> +	if (IS_ERR(base))
>>> +		return PTR_ERR(base);
>>> +
>>> +	dev->i2s_regmap = devm_regmap_init_mmio(&pdev->dev, base,
>>> +				&bcm2835_regmap_config);
>>> +	if (IS_ERR(dev->i2s_regmap))
>>> +		return PTR_ERR(dev->i2s_regmap);
>>> +
>>> +	/* Set the DMA address - we have to parse DT ourselves */
>>> +	addr = of_get_address(pdev->dev.of_node, 0, NULL, NULL);
>>> +	if (!addr) {
>>> +		dev_err(&pdev->dev, "could not get DMA-register address\n");
>>> +		return -EINVAL;
>>> +	}
>>> +	dma_base = be32_to_cpup(addr);
>>
>> Why aren't we just using mem->start like before?  That seems like an
>> independent change that should be justified on its own.  I'd be ready to
>> ack the patch if that change is removed.
>>
>
> Problem is that we need the VC4 bus-address (0x7e203000),
> which is the actual <reg> value from the device tree without any
> mapping.
>
> Not the ARM MMU visible address mappings that mem->start provides
> (typically 0x20203000 or 0x3f203000 for bcm2836)
>
> Nor the mapped address (base) available in the kernel (typically
> 0xdc......).

Now that I've noticed the BCM2835_VCMMU_SHIFT removal, this makes sense,
but when you put unrelated changes like this together you end up slowing
down the review process on your patches.  Please separate it out into a
separate commit.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160212/b4877b08/attachment.sig>

[PATCH V2 1/3] ASoC: bcm2835: move to use the clock framework

[Martin Sperl]

> On 13.02.2016, at 01:47, Eric Anholt <eric@anholt.net> wrote:
> 
> Martin Sperl <kernel@martin.sperl.org> writes:
>> 
>> Problem is that we need the VC4 bus-address (0x7e203000),
>> which is the actual <reg> value from the device tree without any
>> mapping.
>> 
>> Not the ARM MMU visible address mappings that mem->start provides
>> (typically 0x20203000 or 0x3f203000 for bcm2836)
>> 
>> Nor the mapped address (base) available in the kernel (typically
>> 0xdc......).
> 
> Now that I've noticed the BCM2835_VCMMU_SHIFT removal, this makes sense,
> but when you put unrelated changes like this together you end up slowing
> down the review process on your patches.  Please separate it out into a
> separate commit.

OK - I agree: I really should have separated those, but as the
driver was broken for both reasons and the MMU_SHIFT was the earliest
fix I forgot about it when I got the final version for submission.

Anyway: Mark has already applied this patch on January 15th into for-next:
https://git.kernel.org/cgit/linux/kernel/git/broonie/sound.git/commit/sound/soc/bcm/bcm2835-i2s.c?h=for-next&id=517e7a1537ae4663268be5d0c0ec62c563b9fc99
so it should go into the next release...

So unless Mark wants to revert it and have two separate patches
instead, I guess we need to leave it like it is.

Martin

[PATCH V2 2/3] ARM: bcm2835: I2S: use new register-range and clock framework

[kernel at martin.sperl.org]

From: Martin Sperl <kernel@martin.sperl.org>

Since the move to the new clock framework with commit 94cb7f76caa0
("ARM: bcm2835: Switch to using the new clock driver support.")
the bcm2835-i2s driver was no longer working.

This patch fixes the address ranges:
* remove the PCM clock register range that is owned by the clockmanager
* fix the length, which did not include the last register of this device

It also adds the required pcm-clock.

Signed-off-by: Martin Sperl <kernel@martin.sperl.org>
---
 arch/arm/boot/dts/bcm2835.dtsi |    5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/arch/arm/boot/dts/bcm2835.dtsi b/arch/arm/boot/dts/bcm2835.dtsi
index aef64de..83d9787 100644
--- a/arch/arm/boot/dts/bcm2835.dtsi
+++ b/arch/arm/boot/dts/bcm2835.dtsi
@@ -120,9 +120,8 @@
 
 		i2s: i2s at 7e203000 {
 			compatible = "brcm,bcm2835-i2s";
-			reg = <0x7e203000 0x20>,
-			      <0x7e101098 0x02>;
-
+			reg = <0x7e203000 0x24>;
+			clocks = <&clocks BCM2835_CLOCK_PCM>;
 			dmas = <&dma 2>,
 			       <&dma 3>;
 			dma-names = "tx", "rx";
-- 
1.7.10.4

[PATCH V2 2/3] ARM: bcm2835: I2S: use new register-range and clock framework

[Stefan Wahren]

Hi Martin,

[add Mike and Remi]

Am 12.01.2016 um 13:35 schrieb kernel at martin.sperl.org:
> From: Martin Sperl <kernel@martin.sperl.org>
>
> Since the move to the new clock framework with commit 94cb7f76caa0
> ("ARM: bcm2835: Switch to using the new clock driver support.")
> the bcm2835-i2s driver was no longer working.
>
> This patch fixes the address ranges:
> * remove the PCM clock register range that is owned by the clockmanager
> * fix the length, which did not include the last register of this device
>
> It also adds the required pcm-clock.
>
> Signed-off-by: Martin Sperl <kernel@martin.sperl.org>
> ---
>   arch/arm/boot/dts/bcm2835.dtsi |    5 ++---

this won't apply, because the file has been renamed to bcm283x.dtsi.

>   1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm/boot/dts/bcm2835.dtsi b/arch/arm/boot/dts/bcm2835.dtsi
> index aef64de..83d9787 100644
> --- a/arch/arm/boot/dts/bcm2835.dtsi
> +++ b/arch/arm/boot/dts/bcm2835.dtsi
> @@ -120,9 +120,8 @@
>
>   		i2s: i2s at 7e203000 {
>   			compatible = "brcm,bcm2835-i2s";
> -			reg = <0x7e203000 0x20>,
> -			      <0x7e101098 0x02>;
> -
> +			reg = <0x7e203000 0x24>;
> +			clocks = <&clocks BCM2835_CLOCK_PCM>;

After applying clk series ([PATCH V4 0/7] clk: bcm2835: add clocks and 
add MASH support) and this series the pcm clock is an orphan.

Do we need to add "assigned-clocks" to the i2s node just like for pwm [1]?

Regards
Stefan

>   			dmas = <&dma 2>,
>   			       <&dma 3>;
>   			dma-names = "tx", "rx";
>

[1] - 
http://lists.infradead.org/pipermail/linux-rpi-kernel/2015-December/002789.html

[PATCH V2 2/3] ARM: bcm2835: I2S: use new register-range and clock framework

[Martin Sperl]

> On 16.01.2016, at 16:26, Stefan Wahren <info@lategoodbye.de> wrote:
> 
> Hi Martin,
> 
> [add Mike and Remi]
> 
> Am 12.01.2016 um 13:35 schrieb kernel at martin.sperl.org:
>> From: Martin Sperl <kernel@martin.sperl.org>
>> 
>> Since the move to the new clock framework with commit 94cb7f76caa0
>> ("ARM: bcm2835: Switch to using the new clock driver support.")
>> the bcm2835-i2s driver was no longer working.
>> 
>> This patch fixes the address ranges:
>> * remove the PCM clock register range that is owned by the clockmanager
>> * fix the length, which did not include the last register of this device
>> 
>> It also adds the required pcm-clock.
>> 
>> Signed-off-by: Martin Sperl <kernel@martin.sperl.org>
>> ---
>>  arch/arm/boot/dts/bcm2835.dtsi |    5 ++---
> 
> this won't apply, because the file has been renamed to bcm283x.dtsi.

Well - this "rename? was and still is not merged upstream, so it is an
unfortunate circumstance as I am going on vacation and can not create a
new patchset until I return. So please take it as a template
when applying it.

> 
>>  1 file changed, 2 insertions(+), 3 deletions(-)
>> 
>> diff --git a/arch/arm/boot/dts/bcm2835.dtsi b/arch/arm/boot/dts/bcm2835.dtsi
>> index aef64de..83d9787 100644
>> --- a/arch/arm/boot/dts/bcm2835.dtsi
>> +++ b/arch/arm/boot/dts/bcm2835.dtsi
>> @@ -120,9 +120,8 @@
>> 
>>  		i2s: i2s at 7e203000 {
>>  			compatible = "brcm,bcm2835-i2s";
>> -			reg = <0x7e203000 0x20>,
>> -			      <0x7e101098 0x02>;
>> -
>> +			reg = <0x7e203000 0x24>;
>> +			clocks = <&clocks BCM2835_CLOCK_PCM>;
> 
> After applying clk series ([PATCH V4 0/7] clk: bcm2835: add clocks and add MASH support) and this series the pcm clock is an orphan.
> 
> Do we need to add "assigned-clocks" to the i2s node just like for pwm [1]?

In my experience it is not needed for PCM, as the clock is set by the
bcm2835-i2s driver, so I left it out.

As I do not know how the orphan PWM clock would be used I can not comment
if this is really needed with PWM or not - it would just set the default
clock if it got referenced in the DT.

Thanks,
	Martin

[PATCH V2 2/3] ARM: bcm2835: I2S: use new register-range and clock framework

[Eric Anholt]

kernel at martin.sperl.org writes:

> From: Martin Sperl <kernel@martin.sperl.org>
>
> Since the move to the new clock framework with commit 94cb7f76caa0
> ("ARM: bcm2835: Switch to using the new clock driver support.")
> the bcm2835-i2s driver was no longer working.
>
> This patch fixes the address ranges:
> * remove the PCM clock register range that is owned by the clockmanager
> * fix the length, which did not include the last register of this device
>
> It also adds the required pcm-clock.
>
> Signed-off-by: Martin Sperl <kernel@martin.sperl.org>

Acked-by: Eric Anholt <eric@anholt.net>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160128/1aeb2fdd/attachment.sig>

[PATCH V2 3/3] dt-bindings: bsm2835: fix bindings documentation to use new clock framework

[kernel at martin.sperl.org]

From: Martin Sperl <kernel@martin.sperl.org>

The bcm2835-i2s driver has been updated to use the new clock framework
for the bcm2835 SOC.

This patch documents the required changes to the bindings.

Signed-off-by: Martin Sperl <kernel@martin.sperl.org>
---
 Documentation/devicetree/bindings/sound/brcm,bcm2835-i2s.txt |    7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/sound/brcm,bcm2835-i2s.txt b/Documentation/devicetree/bindings/sound/brcm,bcm2835-i2s.txt
index 65783de..b331f26 100644
--- a/Documentation/devicetree/bindings/sound/brcm,bcm2835-i2s.txt
+++ b/Documentation/devicetree/bindings/sound/brcm,bcm2835-i2s.txt
@@ -4,11 +4,10 @@ Required properties:
 - compatible: "brcm,bcm2835-i2s"
 - reg: A list of base address and size entries:
 	* The first entry should cover the PCM registers
-	* The second entry should cover the PCM clock registers
+- clocks: the (PCM) clock to use
 - dmas: List of DMA controller phandle and DMA request line ordered pairs.
 - dma-names: Identifier string for each DMA request line in the dmas property.
   These strings correspond 1:1 with the ordered pairs in dmas.
-
   One of the DMA channels will be responsible for transmission (should be
   named "tx") and one for reception (should be named "rx").
 
@@ -16,8 +15,8 @@ Example:
 
 bcm2835_i2s: i2s at 7e203000 {
 	compatible = "brcm,bcm2835-i2s";
-	reg = <0x7e203000 0x20>,
-	      <0x7e101098 0x02>;
+	reg = <0x7e203000 0x24>;
+	clocks = <&clocks BCM2835_CLOCK_PCM>;
 
 	dmas = <&dma 2>,
 	       <&dma 3>;
-- 
1.7.10.4

[PATCH V2 3/3] dt-bindings: bsm2835: fix bindings documentation to use new clock framework

[Rob Herring]

On Tue, Jan 12, 2016 at 12:35:48PM +0000, kernel at martin.sperl.org wrote:
> From: Martin Sperl <kernel@martin.sperl.org>

Typo in the subject (bsm).

> 
> The bcm2835-i2s driver has been updated to use the new clock framework
> for the bcm2835 SOC.
> 
> This patch documents the required changes to the bindings.
> 
> Signed-off-by: Martin Sperl <kernel@martin.sperl.org>

Please add acks when you post new versions.

[PATCH V2 3/3] dt-bindings: bsm2835: fix bindings documentation to use new clock framework

[Martin Sperl]

> On 12.01.2016, at 15:36, Rob Herring <robh@kernel.org> wrote:
> 
> On Tue, Jan 12, 2016 at 12:35:48PM +0000, kernel at martin.sperl.org wrote:
>> From: Martin Sperl <kernel@martin.sperl.org>
> 
> Typo in the subject (bsm).
> 
>> 
>> The bcm2835-i2s driver has been updated to use the new clock framework
>> for the bcm2835 SOC.
>> 
>> This patch documents the required changes to the bindings.
>> 
>> Signed-off-by: Martin Sperl <kernel@martin.sperl.org>
> 
> Please add acks when you post new versions.
> 

You want it resent with the typo fixed and signed-off?

[PATCH V2 3/3] dt-bindings: bsm2835: fix bindings documentation to use new clock framework

[Eric Anholt]

Martin Sperl <kernel@martin.sperl.org> writes:

>> On 12.01.2016, at 15:36, Rob Herring <robh@kernel.org> wrote:
>> 
>> On Tue, Jan 12, 2016 at 12:35:48PM +0000, kernel at martin.sperl.org wrote:
>>> From: Martin Sperl <kernel@martin.sperl.org>
>> 
>> Typo in the subject (bsm).
>> 
>>> 
>>> The bcm2835-i2s driver has been updated to use the new clock framework
>>> for the bcm2835 SOC.
>>> 
>>> This patch documents the required changes to the bindings.
>>> 
>>> Signed-off-by: Martin Sperl <kernel@martin.sperl.org>
>> 
>> Please add acks when you post new versions.
>> 
>
> You want it resent with the typo fixed and signed-off?

Generally, make sure you apply acks as you get them, so that they don't
get lost.  If you fix the subject and add Rob's ack, it's also:

Acked-by: Eric Anholt <eric@anholt.net>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160128/a64ac35d/attachment.sig>