[PATCH 0/4] Armada 38x and 370 audio

[PATCH 0/4] Armada 38x and 370 audio

[Marcin Wojtas]

Hi,

I send a series of patches adding I2S - S/PDIF support for Marvell Armada 38x
SoC. It is same controller as in Armada 370, however two quirks had to be taken
into consideration:

* new PLL frequency setting

* exceptive pinout of I2S and S/PDIF formats

Kirkwood-i2s driver was enhanced with obtaining mem resources by name. Sound
interface was enabled for Armada 385 DB board, using simple-card DT binding for
both I2S and S/PDIF. In addition to that minor fix was applied for Armada 370
audio DT description (stable-CC'ed).

Any comments and remarks are welcome.

Best regards,
Marcin

Marcin Wojtas (4):
  ASoC: kirkwood: enable Kirkwood driver for Armada 38x platforms
  ARM: mvebu: add audio I2S controller to Armada 38x Device Tree
  ARM: mvebu: add audio support to Armada 385 DB
  ARM: mvebu: fix routing for analog audio input of Armada 370 DB
    platform

 .../devicetree/bindings/sound/mvebu-audio.txt      |  14 ++-
 arch/arm/boot/dts/armada-370-db.dts                |   2 +-
 arch/arm/boot/dts/armada-388-db.dts                |  69 +++++++++++
 arch/arm/boot/dts/armada-38x.dtsi                  |  17 +++
 sound/soc/kirkwood/kirkwood-i2s.c                  | 132 ++++++++++++++++++++-
 sound/soc/kirkwood/kirkwood.h                      |   2 +
 6 files changed, 232 insertions(+), 4 deletions(-)

-- 
1.8.3.1

[PATCH 1/4] ASoC: kirkwood: enable Kirkwood driver for Armada 38x platforms

[Marcin Wojtas]

The audio unit of Marvell Armada38x SoC is similar to the ones comprised by
other Marvell SoCs (Kirkwood, Dove and Armada 370). Therefore Kirkwood audio
driver can be used to support it and this commit adds new compatible string
to identify Armada 38x variant.

Two new memory regions are added: first one for PLL configuration and
the second one for choosing one of audio I/O modes (I2S or S/PDIF).
For the latter purpose a new optional DT property is added ('spdif-mode').

kirkwood-i2s driver is extended by adding a new init function for Armada
38x flavor and also a routine that enables PLL output (i.e. MCLK)
configuration.

Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
 .../devicetree/bindings/sound/mvebu-audio.txt      |  14 ++-
 sound/soc/kirkwood/kirkwood-i2s.c                  | 132 ++++++++++++++++++++-
 sound/soc/kirkwood/kirkwood.h                      |   2 +
 3 files changed, 145 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/sound/mvebu-audio.txt b/Documentation/devicetree/bindings/sound/mvebu-audio.txt
index cb8c07c..4f5dec5 100644
--- a/Documentation/devicetree/bindings/sound/mvebu-audio.txt
+++ b/Documentation/devicetree/bindings/sound/mvebu-audio.txt
@@ -6,9 +6,14 @@ Required properties:
   "marvell,kirkwood-audio" for Kirkwood platforms
   "marvell,dove-audio" for Dove platforms
   "marvell,armada370-audio" for Armada 370 platforms
+  "marvell,armada-380-audio" for Armada 38x platforms
 
 - reg: physical base address of the controller and length of memory mapped
-  region.
+  region (named "i2s_regs").
+  With "marvell,armada-380-audio" two other regions are required:
+  first of those is dedicated for Audio PLL Configuration registers
+  (named "pll_regs") and the second one ("soc_ctrl") - for register
+  where one of exceptive I/O types (I2S or S/PDIF) is set.
 
 - interrupts:
   with "marvell,kirkwood-audio", the audio interrupt
@@ -23,6 +28,13 @@ Required properties:
 	"internal" for the internal clock
 	"extclk" for the external clock
 
+Optional properties:
+
+- spdif-mode:
+  Enable S/PDIF mode on Armada 38x SoC. Using this property
+  disables standard I2S I/O. Valid only with "marvell,armada-380-audio"
+  compatible string.
+
 Example:
 
 i2s1: audio-controller@b4000 {
diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c
index def7d82..8818861 100644
--- a/sound/soc/kirkwood/kirkwood-i2s.c
+++ b/sound/soc/kirkwood/kirkwood-i2s.c
@@ -37,6 +37,124 @@
 	(SNDRV_PCM_FMTBIT_S16_LE | \
 	 SNDRV_PCM_FMTBIT_S24_LE)
 
+/* These registers are relative to the second register region -
+ * audio pll configuration.
+ */
+#define A38X_PLL_CONF_REG0			0x0
+#define     A38X_PLL_FB_CLK_DIV_OFFSET		10
+#define     A38X_PLL_FB_CLK_DIV_MASK		0x7fc00
+#define A38X_PLL_CONF_REG1			0x4
+#define     A38X_PLL_FREQ_OFFSET_MASK		0xffff
+#define     A38X_PLL_FREQ_OFFSET_VALID		BIT(16)
+#define     A38X_PLL_SW_RESET			BIT(31)
+#define A38X_PLL_CONF_REG2			0x8
+#define     A38X_PLL_AUDIO_POSTDIV_MASK		0x7f
+
+/* Bit below belongs to SoC control register corresponding to the third
+ * register region.
+ */
+#define A38X_SPDIF_MODE_ENABLE			BIT(27)
+
+static int armada_38x_i2s_init_quirk(struct platform_device *pdev,
+				     struct kirkwood_dma_data *priv,
+				     struct snd_soc_dai_driver *dai_drv)
+{
+	struct resource *mem;
+	u32 reg_val;
+	int i;
+
+	mem = platform_get_resource_byname(pdev, IORESOURCE_MEM, "pll_regs");
+	priv->pll_config = devm_ioremap_resource(&pdev->dev, mem);
+	if (IS_ERR(priv->pll_config))
+		return -ENOMEM;
+
+	mem = platform_get_resource_byname(pdev, IORESOURCE_MEM, "soc_ctrl");
+	priv->soc_control = devm_ioremap_resource(&pdev->dev, mem);
+	if (IS_ERR(priv->soc_control))
+		return -ENOMEM;
+
+	/* Select one of exceptive modes: I2S or S/PDIF */
+	reg_val = readl(priv->soc_control);
+	if (of_property_read_bool(np, "spdif-mode")) {
+		reg_val |= A38X_SPDIF_MODE_ENABLE;
+		dev_info(&pdev->dev, "using S/PDIF mode\n");
+	} else {
+		reg_val &= ~A38X_SPDIF_MODE_ENABLE;
+		dev_info(&pdev->dev, "using I2S mode\n");
+	}
+	writel(reg_val, priv->soc_control);
+
+	/* Update available rates of mclk's fs */
+	for (i = 0; i < 2; i++) {
+		dai_drv[i].playback.rates |= SNDRV_PCM_RATE_192000;
+		dai_drv[i].capture.rates |= SNDRV_PCM_RATE_192000;
+	}
+
+	return 0;
+}
+
+static inline void armada_38x_set_pll(void __iomem *base, unsigned long rate)
+{
+	u32 reg_val;
+	u16 freq_offset = 0x22b0;
+	u8 audio_postdiv, fb_clk_div = 0x1d;
+
+	/* Set frequency offset value to not valid and enable PLL reset */
+	reg_val = readl(base + A38X_PLL_CONF_REG1);
+	reg_val &= ~A38X_PLL_FREQ_OFFSET_VALID;
+	reg_val &= ~A38X_PLL_SW_RESET;
+	writel(reg_val, base + A38X_PLL_CONF_REG1);
+
+	udelay(1);
+
+	/* Update PLL parameters */
+	switch (rate) {
+	default:
+	case 44100:
+		freq_offset = 0x735;
+		fb_clk_div = 0x1b;
+		audio_postdiv = 0xc;
+		break;
+	case 48000:
+		audio_postdiv = 0xc;
+		break;
+	case 96000:
+		audio_postdiv = 0x6;
+		break;
+	case 192000:
+		audio_postdiv = 0x3;
+		break;
+	}
+
+	reg_val = readl(base + A38X_PLL_CONF_REG0);
+	reg_val &= ~A38X_PLL_FB_CLK_DIV_MASK;
+	reg_val |= (fb_clk_div << A38X_PLL_FB_CLK_DIV_OFFSET);
+	writel(reg_val, base + A38X_PLL_CONF_REG0);
+
+	reg_val = readl(base + A38X_PLL_CONF_REG2);
+	reg_val &= ~A38X_PLL_AUDIO_POSTDIV_MASK;
+	reg_val |= audio_postdiv;
+	writel(reg_val, base + A38X_PLL_CONF_REG2);
+
+	reg_val = readl(base + A38X_PLL_CONF_REG1);
+	reg_val &= ~A38X_PLL_FREQ_OFFSET_MASK;
+	reg_val |= freq_offset;
+	writel(reg_val, base + A38X_PLL_CONF_REG1);
+
+	udelay(1);
+
+	/* Disable reset */
+	reg_val |= A38X_PLL_SW_RESET;
+	writel(reg_val, base + A38X_PLL_CONF_REG1);
+
+	/* Wait 50us for PLL to lock */
+	udelay(50);
+
+	/* Restore frequency offset value validity */
+	reg_val |= A38X_PLL_FREQ_OFFSET_VALID;
+	writel(reg_val, base + A38X_PLL_CONF_REG1);
+}
+
 static int kirkwood_i2s_set_fmt(struct snd_soc_dai *cpu_dai,
 		unsigned int fmt)
 {
@@ -112,7 +230,10 @@ static void kirkwood_set_rate(struct snd_soc_dai *dai,
 		 * defined in kirkwood_i2s_dai */
 		dev_dbg(dai->dev, "%s: dco set rate = %lu\n",
 			__func__, rate);
-		kirkwood_set_dco(priv->io, rate);
+		if (priv->pll_config)
+			armada_38x_set_pll(priv->pll_config, rate);
+		else
+			kirkwood_set_dco(priv->io, rate);
 
 		clks_ctrl = KIRKWOOD_MCLK_SOURCE_DCO;
 	} else {
@@ -544,7 +665,7 @@ static int kirkwood_i2s_dev_probe(struct platform_device *pdev)
 	}
 	dev_set_drvdata(&pdev->dev, priv);
 
-	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	mem = platform_get_resource_byname(pdev, IORESOURCE_MEM, "i2s_regs");
 	priv->io = devm_ioremap_resource(&pdev->dev, mem);
 	if (IS_ERR(priv->io))
 		return PTR_ERR(priv->io);
@@ -555,6 +676,12 @@ static int kirkwood_i2s_dev_probe(struct platform_device *pdev)
 		return -ENXIO;
 	}
 
+	if (of_device_is_compatible(np, "marvell,armada-380-audio")) {
+		err = armada_38x_i2s_init_quirk(pdev, priv, soc_dai);
+		if (err < 0)
+			return err;
+	}
+
 	if (np) {
 		priv->burst = 128;		/* might be 32 or 128 */
 	} else if (data) {
@@ -647,6 +774,7 @@ static struct of_device_id mvebu_audio_of_match[] = {
 	{ .compatible = "marvell,kirkwood-audio" },
 	{ .compatible = "marvell,dove-audio" },
 	{ .compatible = "marvell,armada370-audio" },
+	{ .compatible = "marvell,armada-380-audio" },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, mvebu_audio_of_match);
diff --git a/sound/soc/kirkwood/kirkwood.h b/sound/soc/kirkwood/kirkwood.h
index 90e32a7..0bb6dc6 100644
--- a/sound/soc/kirkwood/kirkwood.h
+++ b/sound/soc/kirkwood/kirkwood.h
@@ -133,6 +133,8 @@
 
 struct kirkwood_dma_data {
 	void __iomem *io;
+	void __iomem *pll_config;
+	void __iomem *soc_control;
 	struct clk *clk;
 	struct clk *extclk;
 	uint32_t ctl_play;
-- 
1.8.3.1

[PATCH 2/4] ARM: mvebu: add audio I2S controller to Armada 38x Device Tree

[Marcin Wojtas]

This commit adds the description of the I2S controller to the Marvell
Armada 38x SoC's Device Tree, as well as its pin configuration.

Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
 arch/arm/boot/dts/armada-38x.dtsi | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/arch/arm/boot/dts/armada-38x.dtsi b/arch/arm/boot/dts/armada-38x.dtsi
index 1dff30a..fa21cb5 100644
--- a/arch/arm/boot/dts/armada-38x.dtsi
+++ b/arch/arm/boot/dts/armada-38x.dtsi
@@ -314,6 +314,12 @@
 					marvell,pins = "mpp44";
 					marvell,function = "sata3";
 				};
+
+				i2s_pins: i2s_pins {
+					marvell,pins = "mpp48", "mpp49", "mpp50",
+						       "mpp51", "mpp52", "mpp53";
+					marvell,function = "audio";
+				};
 			};
 
 			gpio0: gpio@18100 {
@@ -555,6 +561,17 @@
 				status = "disabled";
 			};
 
+			audio_controller: audio-controller@e8000 {
+				#sound-dai-cells = <1>;
+				compatible = "marvell,armada-380-audio";
+				reg = <0xe8000 0x4000>, <0x18410 0xc>, <0x18204 0x4>;
+				reg-names = "i2s_regs", "pll_regs", "soc_ctrl";
+				interrupts = <GIC_SPI 75 IRQ_TYPE_LEVEL_HIGH>;
+				clocks = <&gateclk 0>;
+				clock-names = "internal";
+				status = "disabled";
+			};
+
 			usb3@f0000 {
 				compatible = "marvell,armada-380-xhci";
 				reg = <0xf0000 0x4000>,<0xf4000 0x4000>;
-- 
1.8.3.1

[PATCH 3/4] ARM: mvebu: add audio support to Armada 385 DB

[Marcin Wojtas]

This commit adds the necessary Device Tree information to enable
audio support on the Armada 385 DB platform. In details it:

 * Instantiates the CS42L51 audio codec on the I2C0 bus

 * Adds simple-card DT binding for audio on Armada 385 DB

 * Adds description for both analog I2S and S/PDIF I/O

Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
 arch/arm/boot/dts/armada-388-db.dts | 69 +++++++++++++++++++++++++++++++++++++
 1 file changed, 69 insertions(+)

diff --git a/arch/arm/boot/dts/armada-388-db.dts b/arch/arm/boot/dts/armada-388-db.dts
index 16512ef..bf5d9a7 100644
--- a/arch/arm/boot/dts/armada-388-db.dts
+++ b/arch/arm/boot/dts/armada-388-db.dts
@@ -82,6 +82,11 @@
 			i2c@11000 {
 				status = "okay";
 				clock-frequency = <100000>;
+				audio_codec: audio-codec@4a {
+					#sound-dai-cells = <0>;
+					compatible = "cirrus,cs42l51";
+					reg = <0x4a>;
+				};
 			};
 
 			i2c@11100 {
@@ -158,6 +163,12 @@
 				no-1-8-v;
 			};
 
+			audio-controller@e8000 {
+				pinctrl-0 = <&i2s_pins>;
+				pinctrl-names = "default";
+				status = "okay";
+			};
+
 			usb3@f0000 {
 				status = "okay";
 			};
@@ -183,4 +194,62 @@
 			};
 		};
 	};
+
+	sound {
+		compatible = "simple-audio-card";
+		simple-audio-card,name = "Armada 385 DB Audio";
+		simple-audio-card,mclk-fs = <256>;
+		simple-audio-card,widgets =
+			"Headphone", "Out Jack",
+			"Line", "In Jack";
+		simple-audio-card,routing =
+			"Out Jack", "HPL",
+			"Out Jack", "HPR",
+			"AIN1L", "In Jack",
+			"AIN1R", "In Jack";
+		status = "okay";
+
+		simple-audio-card,dai-link@0 {
+			format = "i2s";
+			cpu {
+				sound-dai = <&audio_controller 0>;
+			};
+
+			codec {
+				sound-dai = <&audio_codec>;
+			};
+		};
+
+		simple-audio-card,dai-link@1 {
+			format = "i2s";
+			cpu {
+				sound-dai = <&audio_controller 1>;
+			};
+
+			codec {
+				sound-dai = <&spdif_out>;
+			};
+		};
+
+		simple-audio-card,dai-link@2 {
+			format = "i2s";
+			cpu {
+				sound-dai = <&audio_controller 1>;
+			};
+
+			codec {
+				sound-dai = <&spdif_in>;
+			};
+		};
+	};
+
+	spdif_out: spdif-out {
+		#sound-dai-cells = <0>;
+		compatible = "linux,spdif-dit";
+	};
+
+	spdif_in: spdif-in {
+		#sound-dai-cells = <0>;
+		compatible = "linux,spdif-dir";
+	};
 };
-- 
1.8.3.1

[PATCH 4/4] ARM: mvebu: fix routing for analog audio input of Armada 370 DB platform

[Marcin Wojtas]

Armada 370 DB sound complex comprise routing for audio card with a
description for analog input line with two left channels instead of a pair
left-right. This commit introduces routing for the right channel.

Fixes: a6b334514be2 ("use simple-card DT binding for audio on Armada 370 DB")

Signed-off-by: Marcin Wojtas <mw@semihalf.com>
Cc: <stable@vger.kernel.org> # v3.19+
Reviewed-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 arch/arm/boot/dts/armada-370-db.dts | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/armada-370-db.dts b/arch/arm/boot/dts/armada-370-db.dts
index e993c46..d6e72a4 100644
--- a/arch/arm/boot/dts/armada-370-db.dts
+++ b/arch/arm/boot/dts/armada-370-db.dts
@@ -190,7 +190,7 @@
 			"Out Jack", "HPL",
 			"Out Jack", "HPR",
 			"AIN1L", "In Jack",
-			"AIN1L", "In Jack";
+			"AIN1R", "In Jack";
 		status = "okay";
 
 		simple-audio-card,dai-link@0 {
-- 
1.8.3.1

Re: [PATCH 2/4] ARM: mvebu: add audio I2S controller to Armada 38x Device Tree

[Sebastian Hesselbarth]

On 25.02.2015 22:58, Marcin Wojtas wrote:
> This commit adds the description of the I2S controller to the Marvell
> Armada 38x SoC's Device Tree, as well as its pin configuration.
>
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> ---
>   arch/arm/boot/dts/armada-38x.dtsi | 17 +++++++++++++++++
>   1 file changed, 17 insertions(+)
>
> diff --git a/arch/arm/boot/dts/armada-38x.dtsi b/arch/arm/boot/dts/armada-38x.dtsi
> index 1dff30a..fa21cb5 100644
> --- a/arch/arm/boot/dts/armada-38x.dtsi
> +++ b/arch/arm/boot/dts/armada-38x.dtsi
[...]
> @@ -555,6 +561,17 @@
>   				status = "disabled";
>   			};
>
> +			audio_controller: audio-controller@e8000 {
> +				#sound-dai-cells = <1>;
> +				compatible = "marvell,armada-380-audio";
> +				reg = <0xe8000 0x4000>, <0x18410 0xc>, <0x18204 0x4>;
> +				reg-names = "i2s_regs", "pll_regs", "soc_ctrl";

Marcin,

sorry but NACK. The PLL and SoC ctrl are not even close to the audio
controller registers.

> +				interrupts = <GIC_SPI 75 IRQ_TYPE_LEVEL_HIGH>;
> +				clocks = <&gateclk 0>;
> +				clock-names = "internal";

How about providing access to audio PLL by a clock driver and amend the
binding to allow for a second more precise PLL clock, e.g.

	clocks = <&gateclk CLK_AUDIO>, <&pll PLL_AUDIO>;
	clock-names = "internal", "pll";

we already check for an "extclk" on Dove for the same reason but the
name might be misleading here.

Also, i2c/spdif muxing option could be handled by 38x's pinctrl driver,
we have the same for Dove's internal i2c mux.

If you want to use i2s you just add the option to the default pinctrl
hog:

	pinctrl-0 = <&i2s_pins &audio_mux_i2s>;
	pinctrl-names = "default";

Sebastian

> +				status = "disabled";
> +			};
> +
>   			usb3@f0000 {
>   				compatible = "marvell,armada-380-xhci";
>   				reg = <0xf0000 0x4000>,<0xf4000 0x4000>;
>

Re: [PATCH 2/4] ARM: mvebu: add audio I2S controller to Armada 38x Device Tree

[Marcin Wojtas]

Hi Sebastian,

Thanks for your input.

2015-02-25 23:21 GMT+01:00 Sebastian Hesselbarth
<sebastian.hesselbarth@gmail.com>:
> On 25.02.2015 22:58, Marcin Wojtas wrote:
>>
>> This commit adds the description of the I2S controller to the Marvell
>> Armada 38x SoC's Device Tree, as well as its pin configuration.
>>
>> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
>> ---
>>   arch/arm/boot/dts/armada-38x.dtsi | 17 +++++++++++++++++
>>   1 file changed, 17 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/armada-38x.dtsi
>> b/arch/arm/boot/dts/armada-38x.dtsi
>> index 1dff30a..fa21cb5 100644
>> --- a/arch/arm/boot/dts/armada-38x.dtsi
>> +++ b/arch/arm/boot/dts/armada-38x.dtsi
>
> [...]
>>
>> @@ -555,6 +561,17 @@
>>                                 status = "disabled";
>>                         };
>>
>> +                       audio_controller: audio-controller@e8000 {
>> +                               #sound-dai-cells = <1>;
>> +                               compatible = "marvell,armada-380-audio";
>> +                               reg = <0xe8000 0x4000>, <0x18410 0xc>,
>> <0x18204 0x4>;
>> +                               reg-names = "i2s_regs", "pll_regs",
>> "soc_ctrl";
>
>
> Marcin,
>
> sorry but NACK. The PLL and SoC ctrl are not even close to the audio
> controller registers.
>
>> +                               interrupts = <GIC_SPI 75
>> IRQ_TYPE_LEVEL_HIGH>;
>> +                               clocks = <&gateclk 0>;
>> +                               clock-names = "internal";
>
>
> How about providing access to audio PLL by a clock driver and amend the
> binding to allow for a second more precise PLL clock, e.g.
>
>         clocks = <&gateclk CLK_AUDIO>, <&pll PLL_AUDIO>;
>         clock-names = "internal", "pll";
>

Good idea. Would you suggest some name for the new driver? How about
clk-audio.c under drivers/clk/mvebu?

> we already check for an "extclk" on Dove for the same reason but the
> name might be misleading here.
>
> Also, i2c/spdif muxing option could be handled by 38x's pinctrl driver,
> we have the same for Dove's internal i2c mux.
>
> If you want to use i2s you just add the option to the default pinctrl
> hog:
>
>         pinctrl-0 = <&i2s_pins &audio_mux_i2s>;
>         pinctrl-names = "default";
>

Ok, I'll try to extend pinctrl-armada-38x then.

Marcin

Re: [PATCH 2/4] ARM: mvebu: add audio I2S controller to Armada 38x Device Tree

[Sebastian Hesselbarth]

On 26.02.2015 01:11, Marcin Wojtas wrote:
> 2015-02-25 23:21 GMT+01:00 Sebastian Hesselbarth
> <sebastian.hesselbarth@gmail.com>:
>> On 25.02.2015 22:58, Marcin Wojtas wrote:
[...]
>> sorry but NACK. The PLL and SoC ctrl are not even close to the audio
>> controller registers.
>>
>>> +                               interrupts = <GIC_SPI 75
>>> IRQ_TYPE_LEVEL_HIGH>;
>>> +                               clocks = <&gateclk 0>;
>>> +                               clock-names = "internal";
>>
>>
>> How about providing access to audio PLL by a clock driver and amend the
>> binding to allow for a second more precise PLL clock, e.g.
>>
>>          clocks = <&gateclk CLK_AUDIO>, <&pll PLL_AUDIO>;
>>          clock-names = "internal", "pll";
>>
>
> Good idea. Would you suggest some name for the new driver? How about
> clk-audio.c under drivers/clk/mvebu?

Depends on how much you know about the surrounding registers.

If there is more clock related stuff than the audio pll, I'd
suggest to have a single driver for everything that isn't core clk
or clk gates which are already dealt with in the existing driver.

Given that this PLL layout is most likely 38x specific, I'd just put it
into drivers/clk/mvebu/armada-38x.c.

>> we already check for an "extclk" on Dove for the same reason but the
>> name might be misleading here.
>>
>> Also, i2c/spdif muxing option could be handled by 38x's pinctrl driver,
>> we have the same for Dove's internal i2c mux.
>>
>> If you want to use i2s you just add the option to the default pinctrl
>> hog:
>>
>>          pinctrl-0 = <&i2s_pins &audio_mux_i2s>;
>>          pinctrl-names = "default";
>>
>
> Ok, I'll try to extend pinctrl-armada-38x then.

IMHO, adding a "syscon" compatible to the system-controller node should
do the trick. You can reference the regmap from 38x's pinctrl driver and
have a specific callback for the i2s/spdif muxing. The reg property
range of the system-controller should be extended but that definitely
depends on what the you or the Free Electron guys know about the
register set.

Sebastian

Re: [PATCH 2/4] ARM: mvebu: add audio I2S controller to Armada 38x Device Tree

[Marcin Wojtas]

2015-02-26 1:30 GMT+01:00 Sebastian Hesselbarth
<sebastian.hesselbarth@gmail.com>:
> On 26.02.2015 01:11, Marcin Wojtas wrote:
>>
>> 2015-02-25 23:21 GMT+01:00 Sebastian Hesselbarth
>> <sebastian.hesselbarth@gmail.com>:
>>>
>>> On 25.02.2015 22:58, Marcin Wojtas wrote:
>
> [...]
>>>
>>> sorry but NACK. The PLL and SoC ctrl are not even close to the audio
>>> controller registers.
>>>
>>>> +                               interrupts = <GIC_SPI 75
>>>> IRQ_TYPE_LEVEL_HIGH>;
>>>> +                               clocks = <&gateclk 0>;
>>>> +                               clock-names = "internal";
>>>
>>>
>>>
>>> How about providing access to audio PLL by a clock driver and amend the
>>> binding to allow for a second more precise PLL clock, e.g.
>>>
>>>          clocks = <&gateclk CLK_AUDIO>, <&pll PLL_AUDIO>;
>>>          clock-names = "internal", "pll";
>>>
>>
>> Good idea. Would you suggest some name for the new driver? How about
>> clk-audio.c under drivers/clk/mvebu?
>
>
> Depends on how much you know about the surrounding registers.
>
> If there is more clock related stuff than the audio pll, I'd
> suggest to have a single driver for everything that isn't core clk
> or clk gates which are already dealt with in the existing driver.
>
> Given that this PLL layout is most likely 38x specific, I'd just put it
> into drivers/clk/mvebu/armada-38x.c.
>

Audio PLL setting consist of 3 independent registers. Adjacent to them
there are same regs for setting TDM PLL, however I doubt  telephony is
going to be supported in foreseeable future (if ever). I also have
strong suspicion that it looks the same in a39x - Thomas, can you take
a look into spec and confirm?

>>> we already check for an "extclk" on Dove for the same reason but the
>>> name might be misleading here.
>>>
>>> Also, i2c/spdif muxing option could be handled by 38x's pinctrl driver,
>>> we have the same for Dove's internal i2c mux.
>>>
>>> If you want to use i2s you just add the option to the default pinctrl
>>> hog:
>>>
>>>          pinctrl-0 = <&i2s_pins &audio_mux_i2s>;
>>>          pinctrl-names = "default";
>>>
>>
>> Ok, I'll try to extend pinctrl-armada-38x then.
>
>
> IMHO, adding a "syscon" compatible to the system-controller node should
> do the trick. You can reference the regmap from 38x's pinctrl driver and
> have a specific callback for the i2s/spdif muxing. The reg property
> range of the system-controller should be extended but that definitely
> depends on what the you or the Free Electron guys know about the
> register set.
>

For muxing i2s/spdif (exactly the same pins, they have to be either
way set to "audio" by pinctrl driver) changing single bit in "system
control 1" register is needed. However currently unused, it is already
a part of system-controller@18200 regmap. Anyway the will of using
either i2s or spdif has to be explicitly represented in DT.

Marcin

Re: [PATCH 2/4] ARM: mvebu: add audio I2S controller to Armada 38x Device Tree

[Thomas Petazzoni]

Dear Marcin Wojtas,

On Thu, 26 Feb 2015 10:05:08 +0100, Marcin Wojtas wrote:

> > Given that this PLL layout is most likely 38x specific, I'd just put it
> > into drivers/clk/mvebu/armada-38x.c.
> 
> Audio PLL setting consist of 3 independent registers. Adjacent to them
> there are same regs for setting TDM PLL, however I doubt  telephony is
> going to be supported in foreseeable future (if ever).

We might be supporting TDM in the future. However, I see that the TDM
PLL registers are nicely grouped together, and so are the audio PLL
registers. So maybe we can simply have a driver to handle the audio PLL
registers for now, and if needed in the future, add another driver for
the TDM PLL.

> I also have strong suspicion that it looks the same in a39x - Thomas,
> can you take a look into spec and confirm?

As far as I can see there is no audio support and no TDM support in
Armada 39x, and there those TDM PLL and audio PLL registers do not
exist.

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH 2/4] ARM: mvebu: add audio I2S controller to Armada 38x Device Tree

[Marcin Wojtas]

Dear Thomas,

2015-02-27 15:03 GMT+01:00 Thomas Petazzoni
<thomas.petazzoni@free-electrons.com>:
> Dear Marcin Wojtas,
>
> On Thu, 26 Feb 2015 10:05:08 +0100, Marcin Wojtas wrote:
>
>> > Given that this PLL layout is most likely 38x specific, I'd just put it
>> > into drivers/clk/mvebu/armada-38x.c.
>>
>> Audio PLL setting consist of 3 independent registers. Adjacent to them
>> there are same regs for setting TDM PLL, however I doubt  telephony is
>> going to be supported in foreseeable future (if ever).
>
> We might be supporting TDM in the future. However, I see that the TDM
> PLL registers are nicely grouped together, and so are the audio PLL
> registers. So maybe we can simply have a driver to handle the audio PLL
> registers for now, and if needed in the future, add another driver for
> the TDM PLL.

I've got very fresh experience of adding TDM support for A38x, it
definitely would require longer discussion.

>
>> I also have strong suspicion that it looks the same in a39x - Thomas,
>> can you take a look into spec and confirm?
>
> As far as I can see there is no audio support and no TDM support in
> Armada 39x, and there those TDM PLL and audio PLL registers do not
> exist.
>

Given all the facts, do you think that drivers/clk/mvebu/armada-38x.c
is a right place for adding this PLL setting support?

Best regards,
Marcin

Re: [PATCH 2/4] ARM: mvebu: add audio I2S controller to Armada 38x Device Tree

[Thomas Petazzoni]

Dear Marcin Wojtas,

On Fri, 27 Feb 2015 21:22:30 +0100, Marcin Wojtas wrote:

> Given all the facts, do you think that drivers/clk/mvebu/armada-38x.c
> is a right place for adding this PLL setting support?

Yes, sounds right to me. I was thinking that maybe it should be in a
separate file armada-38x-pll.c, but that's probably a bit too much for
something that is causing to be a simple driver.

Have you sorted out how to handle the pin-muxing part of the problem?

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH 2/4] ARM: mvebu: add audio I2S controller to Armada 38x Device Tree

[Marcin Wojtas]

Hello Thomas,

2015-02-28 10:58 GMT+01:00 Thomas Petazzoni
<thomas.petazzoni@free-electrons.com>:
> Dear Marcin Wojtas,
>
> On Fri, 27 Feb 2015 21:22:30 +0100, Marcin Wojtas wrote:
>
>> Given all the facts, do you think that drivers/clk/mvebu/armada-38x.c
>> is a right place for adding this PLL setting support?
>
> Yes, sounds right to me. I was thinking that maybe it should be in a
> separate file armada-38x-pll.c, but that's probably a bit too much for
> something that is causing to be a simple driver.
>

I've got an update from Marvell - TDM and Audio are available in A39x
and it has the same set of PLL registers like A38x. Shouldn't we place
PLL configuration code in a separate file like clk-pll.c, or whatever
name you suggest?

> Have you sorted out how to handle the pin-muxing part of the problem?
>

I'll come back with this issue after implementing poc.

Best regards,
Marcin

Re: [PATCH 2/4] ARM: mvebu: add audio I2S controller to Armada 38x Device Tree

[Thomas Petazzoni]

Dear Marcin Wojtas,

On Wed, 4 Mar 2015 13:12:10 +0100, Marcin Wojtas wrote:

> I've got an update from Marvell - TDM and Audio are available in A39x
> and it has the same set of PLL registers like A38x. Shouldn't we place
> PLL configuration code in a separate file like clk-pll.c, or whatever
> name you suggest?

Yes, seems OK to me.

> > Have you sorted out how to handle the pin-muxing part of the problem?
> 
> I'll come back with this issue after implementing poc.

Ok, thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com