Re: [PATCH v8 1/2] ASoC: fsl: Add S/PDIF CPU DAI driver
On 08/19/2013 08:28 PM, Nicolin Chen wrote: > On Mon, Aug 19, 2013 at 03:35:58PM -0600, Stephen Warren wrote: >>> + "core" The core clock of spdif controller >>> + "rxtx<0-7>" Clock source list for tx and rx clock. >>> + This clock list should be identical to >>> + the source list connecting to the spdif >>> + clock mux in "SPDIF Transceiver Clock >>> + Diagram" of SoC reference manual. It >>> + can also be referred to TxClk_Source >>> + bit of register SPDIF_STC. >> >> So, the HW block has 1 clock input, yet there's a mux somewhere else in >> the SoC which has 8 inputs? >> >> If so, I'm not completely sure it's correct to reference anything other >> than the "core" clock in this binding. I think the other clocks would be >> more suitably represented in the system-level "sound card" binding that >> I guess patch 2/2 (which I haven't read yet) adds, since I assume those >> clock are more to do with system-level clock tree setup decisions, and >> might not even exist in some other SoC that included this IP block. >> >> What do others think, assuming I'm correct about my HW design assumptions? > > The core clock is being only needed when accessing registers of this IP. > Thus, in the driver, I let regmap handle it. > > While the other 8 clocks are actual reference clocks for Tx. Tx clock needs > to select one of them that can easily derive a child clock matching the tx > sample rate. This is essential for the IP, so I don't think it's nicer to > put into machine driver. So just to be clear, the S/PDIF IP block truly has 8 rxtx clock input signals, and the mux between them is internal to the S/PDIF block? If so, then this aspect of the binding is fine. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v8 1/2] ASoC: fsl: Add S/PDIF CPU DAI driver
On Mon, Aug 19, 2013 at 03:35:58PM -0600, Stephen Warren wrote: > > + "core" The core clock of spdif controller > > + "rxtx<0-7>" Clock source list for tx and rx clock. > > + This clock list should be identical to > > + the source list connecting to the spdif > > + clock mux in "SPDIF Transceiver Clock > > + Diagram" of SoC reference manual. It > > + can also be referred to TxClk_Source > > + bit of register SPDIF_STC. > > So, the HW block has 1 clock input, yet there's a mux somewhere else in > the SoC which has 8 inputs? > > If so, I'm not completely sure it's correct to reference anything other > than the "core" clock in this binding. I think the other clocks would be > more suitably represented in the system-level "sound card" binding that > I guess patch 2/2 (which I haven't read yet) adds, since I assume those > clock are more to do with system-level clock tree setup decisions, and > might not even exist in some other SoC that included this IP block. > > What do others think, assuming I'm correct about my HW design assumptions? The core clock is being only needed when accessing registers of this IP. Thus, in the driver, I let regmap handle it. While the other 8 clocks are actual reference clocks for Tx. Tx clock needs to select one of them that can easily derive a child clock matching the tx sample rate. This is essential for the IP, so I don't think it's nicer to put into machine driver. Thank you Nicolin ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v8 1/2] ASoC: fsl: Add S/PDIF CPU DAI driver
Thank you Sascha, I'll revise them all in v9
On Mon, Aug 19, 2013 at 02:34:49PM +0200, Sascha Hauer wrote:
> Hi Nicolas,
>
> Some misc other comments inline.
>
> On Mon, Aug 19, 2013 at 08:08:48PM +0800, Nicolin Chen wrote:
> > This patch implements a device-tree-only CPU DAI driver for Freescale
> > +
> > +struct fsl_spdif_priv {
> > + struct spdif_mixer_control fsl_spdif_control;
> > + struct snd_soc_dai_driver cpu_dai_drv;
> > + struct platform_device *pdev;
> > + struct regmap *regmap;
> > + atomic_t dpll_locked;
>
> You don't need an atomic_t to track a bool variable. Use a plain bool or
> int instead.
>
> > + u8 txclk_div[SPDIF_TXRATE_MAX];
> > + u8 txclk_src[SPDIF_TXRATE_MAX];
> > + u8 rxclk_src;
> > + struct clk *txclk[SPDIF_TXRATE_MAX];
> > + struct clk *rxclk;
> > + struct snd_dmaengine_dai_dma_data dma_params_tx;
> > + struct snd_dmaengine_dai_dma_data dma_params_rx;
> > +
> > + /* The name space will be allocated dynamically */
> > + char name[0];
> > +};
> > +
> > +
> > +#ifdef DEBUG
> > +static void dumpregs(struct fsl_spdif_priv *spdif_priv)
> > +{
> > + struct regmap *regmap = spdif_priv->regmap;
> > + struct platform_device *pdev = spdif_priv->pdev;
> > + u32 val, i;
> > + int ret;
> > +
> > + /* Valid address set of SPDIF is {[0x0-0x38], 0x44, 0x50} */
> > + for (i = 0 ; i <= REG_SPDIF_STC; i += 4) {
> > + ret = regmap_read(regmap, REG_SPDIF_SCR + i, &val);
> > + if (!ret)
> > + dev_dbg(&pdev->dev, "REG 0x%02x = 0x%06x\n", i, val);
> > + }
> > +}
> > +#else
> > +static void dumpregs(struct fsl_spdif_priv *spdif_priv) {}
> > +#endif
>
> Is this needed? regmap provides a register dump in debugfs.
>
> > +
> > +static int spdif_clk_set_rate(struct clk *clk, unsigned long rate)
> > +{
> > + unsigned long rate_actual;
> > +
> > + rate_actual = clk_round_rate(clk, rate);
> > + return clk_set_rate(clk, rate_actual);
> > +}
>
> clk_round_rate returns the rate which clk_set_rate would set if called
> with the same rate. The clk_round_rate() is unnecessary.
>
> > +
> > + dev_dbg(&pdev->dev, "expected clock rate = %d\n",
> > + (int)(64 * sample_rate * div));
> > + dev_dbg(&pdev->dev, "acutal clock rate = %d\n",
> > + (int)clk_get_rate(spdif_priv->txclk[rate]));
>
> s/acutal/actual/
>
> Also please use %ld instead of casting the unsigned long to int.
>
> > +
> > + dev_dbg(&pdev->dev, "FreqMeas: %d\n", (int)freqmeas);
> > + dev_dbg(&pdev->dev, "BusclkFreq: %d\n", (int)busclk_freq);
> > + dev_dbg(&pdev->dev, "RxRate: %d\n", (int)tmpval64);
>
> Get rid of the casts
>
> > +
> > + spdif_priv = devm_kzalloc(&pdev->dev,
> > + sizeof(struct fsl_spdif_priv) + strlen(np->name) + 1,
> > GFP_KERNEL);
> > + if (!spdif_priv) {
> > + dev_err(&pdev->dev, "could not allocate DAI object\n");
>
> Please drop this message. You'll never see it.
>
> > +
> > + for (i = 0; i < SPDIF_TXRATE_MAX; i++) {
> > + ret = fsl_spdif_probe_txclk(spdif_priv, i);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + /* Prepare rx/tx clock */
> > + clk_prepare(spdif_priv->rxclk);
> > + for (i = 0; i < SPDIF_TXRATE_MAX; i++)
> > + clk_prepare(spdif_priv->txclk[i]);
>
> Why do you prepare all clocks instead of the one you actually use? Also,
> no need to do this here. You can use clk_prepare_enable instead where
> you have clk_enable now.
>
> > +
> > +/* SPDIF rx clock source */
> > +enum spdif_rxclk_src {
> > + SRPC_CLKSRC_0 = 0,
> > + SRPC_CLKSRC_1,
> > + SRPC_CLKSRC_2,
> > + SRPC_CLKSRC_3,
> > + SRPC_CLKSRC_4,
> > + SRPC_CLKSRC_5,
> > + SRPC_CLKSRC_6,
> > + SRPC_CLKSRC_7,
> > + SRPC_CLKSRC_8,
> > + SRPC_CLKSRC_9,
> > + SRPC_CLKSRC_10,
> > + SRPC_CLKSRC_11,
> > + SRPC_CLKSRC_12,
> > + SRPC_CLKSRC_13,
> > + SRPC_CLKSRC_14,
> > + SRPC_CLKSRC_15,
> > +};
>
> These are unused and look unnecessary.
>
> > +
> > +/* SPDIF tx clksrc */
> > +enum spdif_txclk_src {
> > + STC_TXCLK_SRC_0 = 0,
> > + STC_TXCLK_SRC_1,
> > + STC_TXCLK_SRC_2,
> > + STC_TXCLK_SRC_3,
> > + STC_TXCLK_SRC_4,
> > + STC_TXCLK_SRC_5,
> > + STC_TXCLK_SRC_6,
> > + STC_TXCLK_SRC_7,
> > +};
>
> Also unused.
>
> Sascha
>
> --
> Pengutronix e.K. | |
> Industrial Linux Solutions | http://www.pengutronix.de/ |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
>
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v8 1/2] ASoC: fsl: Add S/PDIF CPU DAI driver
On 08/19/2013 06:08 AM, Nicolin Chen wrote: > This patch implements a device-tree-only CPU DAI driver for Freescale > S/PDIF controller that supports stereo playback and record feature. > diff --git a/Documentation/devicetree/bindings/sound/fsl,spdif.txt > b/Documentation/devicetree/bindings/sound/fsl,spdif.txt > +Freescale Sony/Philips Digital Interface Format (S/PDIF) Controller > + > +The Freescale S/PDIF audio block is a stereo transceiver that allows the > +processor to receive and transmit digital audio via an coaxial cable or > +a fibre cable. Well, the cable type is more a property of the components that are hooked to the SoC signals that this module drives, so are somewhat out of scope of this binding document. However, this is nit-picky comment, and I'm not too worried about it. > + - clock-names : Includes the following entries: > + "core" The core clock of spdif controller > + "rxtx<0-7>" Clock source list for tx and rx clock. > + This clock list should be identical to > + the source list connecting to the spdif > + clock mux in "SPDIF Transceiver Clock > + Diagram" of SoC reference manual. It > + can also be referred to TxClk_Source > + bit of register SPDIF_STC. So, the HW block has 1 clock input, yet there's a mux somewhere else in the SoC which has 8 inputs? If so, I'm not completely sure it's correct to reference anything other than the "core" clock in this binding. I think the other clocks would be more suitably represented in the system-level "sound card" binding that I guess patch 2/2 (which I haven't read yet) adds, since I assume those clock are more to do with system-level clock tree setup decisions, and might not even exist in some other SoC that included this IP block. What do others think, assuming I'm correct about my HW design assumptions? ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v8 1/2] ASoC: fsl: Add S/PDIF CPU DAI driver
Hi Nicolas,
Some misc other comments inline.
On Mon, Aug 19, 2013 at 08:08:48PM +0800, Nicolin Chen wrote:
> This patch implements a device-tree-only CPU DAI driver for Freescale
> +
> +struct fsl_spdif_priv {
> + struct spdif_mixer_control fsl_spdif_control;
> + struct snd_soc_dai_driver cpu_dai_drv;
> + struct platform_device *pdev;
> + struct regmap *regmap;
> + atomic_t dpll_locked;
You don't need an atomic_t to track a bool variable. Use a plain bool or
int instead.
> + u8 txclk_div[SPDIF_TXRATE_MAX];
> + u8 txclk_src[SPDIF_TXRATE_MAX];
> + u8 rxclk_src;
> + struct clk *txclk[SPDIF_TXRATE_MAX];
> + struct clk *rxclk;
> + struct snd_dmaengine_dai_dma_data dma_params_tx;
> + struct snd_dmaengine_dai_dma_data dma_params_rx;
> +
> + /* The name space will be allocated dynamically */
> + char name[0];
> +};
> +
> +
> +#ifdef DEBUG
> +static void dumpregs(struct fsl_spdif_priv *spdif_priv)
> +{
> + struct regmap *regmap = spdif_priv->regmap;
> + struct platform_device *pdev = spdif_priv->pdev;
> + u32 val, i;
> + int ret;
> +
> + /* Valid address set of SPDIF is {[0x0-0x38], 0x44, 0x50} */
> + for (i = 0 ; i <= REG_SPDIF_STC; i += 4) {
> + ret = regmap_read(regmap, REG_SPDIF_SCR + i, &val);
> + if (!ret)
> + dev_dbg(&pdev->dev, "REG 0x%02x = 0x%06x\n", i, val);
> + }
> +}
> +#else
> +static void dumpregs(struct fsl_spdif_priv *spdif_priv) {}
> +#endif
Is this needed? regmap provides a register dump in debugfs.
> +
> +static int spdif_clk_set_rate(struct clk *clk, unsigned long rate)
> +{
> + unsigned long rate_actual;
> +
> + rate_actual = clk_round_rate(clk, rate);
> + return clk_set_rate(clk, rate_actual);
> +}
clk_round_rate returns the rate which clk_set_rate would set if called
with the same rate. The clk_round_rate() is unnecessary.
> +
> + dev_dbg(&pdev->dev, "expected clock rate = %d\n",
> + (int)(64 * sample_rate * div));
> + dev_dbg(&pdev->dev, "acutal clock rate = %d\n",
> + (int)clk_get_rate(spdif_priv->txclk[rate]));
s/acutal/actual/
Also please use %ld instead of casting the unsigned long to int.
> +
> + dev_dbg(&pdev->dev, "FreqMeas: %d\n", (int)freqmeas);
> + dev_dbg(&pdev->dev, "BusclkFreq: %d\n", (int)busclk_freq);
> + dev_dbg(&pdev->dev, "RxRate: %d\n", (int)tmpval64);
Get rid of the casts
> +
> + spdif_priv = devm_kzalloc(&pdev->dev,
> + sizeof(struct fsl_spdif_priv) + strlen(np->name) + 1,
> GFP_KERNEL);
> + if (!spdif_priv) {
> + dev_err(&pdev->dev, "could not allocate DAI object\n");
Please drop this message. You'll never see it.
> +
> + for (i = 0; i < SPDIF_TXRATE_MAX; i++) {
> + ret = fsl_spdif_probe_txclk(spdif_priv, i);
> + if (ret)
> + return ret;
> + }
> +
> + /* Prepare rx/tx clock */
> + clk_prepare(spdif_priv->rxclk);
> + for (i = 0; i < SPDIF_TXRATE_MAX; i++)
> + clk_prepare(spdif_priv->txclk[i]);
Why do you prepare all clocks instead of the one you actually use? Also,
no need to do this here. You can use clk_prepare_enable instead where
you have clk_enable now.
> +
> +/* SPDIF rx clock source */
> +enum spdif_rxclk_src {
> + SRPC_CLKSRC_0 = 0,
> + SRPC_CLKSRC_1,
> + SRPC_CLKSRC_2,
> + SRPC_CLKSRC_3,
> + SRPC_CLKSRC_4,
> + SRPC_CLKSRC_5,
> + SRPC_CLKSRC_6,
> + SRPC_CLKSRC_7,
> + SRPC_CLKSRC_8,
> + SRPC_CLKSRC_9,
> + SRPC_CLKSRC_10,
> + SRPC_CLKSRC_11,
> + SRPC_CLKSRC_12,
> + SRPC_CLKSRC_13,
> + SRPC_CLKSRC_14,
> + SRPC_CLKSRC_15,
> +};
These are unused and look unnecessary.
> +
> +/* SPDIF tx clksrc */
> +enum spdif_txclk_src {
> + STC_TXCLK_SRC_0 = 0,
> + STC_TXCLK_SRC_1,
> + STC_TXCLK_SRC_2,
> + STC_TXCLK_SRC_3,
> + STC_TXCLK_SRC_4,
> + STC_TXCLK_SRC_5,
> + STC_TXCLK_SRC_6,
> + STC_TXCLK_SRC_7,
> +};
Also unused.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH v8 1/2] ASoC: fsl: Add S/PDIF CPU DAI driver
This patch implements a device-tree-only CPU DAI driver for Freescale
S/PDIF controller that supports stereo playback and record feature.
Signed-off-by: Nicolin Chen
---
.../devicetree/bindings/sound/fsl,spdif.txt| 54 +
sound/soc/fsl/Kconfig |3 +
sound/soc/fsl/Makefile |2 +
sound/soc/fsl/fsl_spdif.c | 1281
sound/soc/fsl/fsl_spdif.h | 224
5 files changed, 1564 insertions(+), 0 deletions(-)
create mode 100644 Documentation/devicetree/bindings/sound/fsl,spdif.txt
create mode 100644 sound/soc/fsl/fsl_spdif.c
create mode 100644 sound/soc/fsl/fsl_spdif.h
diff --git a/Documentation/devicetree/bindings/sound/fsl,spdif.txt
b/Documentation/devicetree/bindings/sound/fsl,spdif.txt
new file mode 100644
index 000..f2ae335
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/fsl,spdif.txt
@@ -0,0 +1,54 @@
+Freescale Sony/Philips Digital Interface Format (S/PDIF) Controller
+
+The Freescale S/PDIF audio block is a stereo transceiver that allows the
+processor to receive and transmit digital audio via an coaxial cable or
+a fibre cable.
+
+Required properties:
+
+ - compatible : Compatible list, must contain "fsl,imx35-spdif".
+
+ - reg : Offset and length of the register set for the device.
+
+ - interrupts : Contains the spdif interrupt.
+
+ - dmas : Generic dma devicetree binding as described in
+ Documentation/devicetree/bindings/dma/dma.txt.
+
+ - dma-names : Two dmas have to be defined, "tx" and "rx".
+
+ - clocks : Contains an entry for each entry in clock-names.
+
+ - clock-names : Includes the following entries:
+ "core" The core clock of spdif controller
+ "rxtx<0-7>" Clock source list for tx and rx clock.
+ This clock list should be identical to
+ the source list connecting to the spdif
+ clock mux in "SPDIF Transceiver Clock
+ Diagram" of SoC reference manual. It
+ can also be referred to TxClk_Source
+ bit of register SPDIF_STC.
+
+Example:
+
+spdif: spdif@02004000 {
+ compatible = "fsl,imx35-spdif";
+ reg = <0x02004000 0x4000>;
+ interrupts = <0 52 0x04>;
+ dmas = <&sdma 14 18 0>,
+ <&sdma 15 18 0>;
+ dma-names = "rx", "tx";
+
+ clocks = <&clks 197>, <&clks 3>,
+ <&clks 197>, <&clks 107>,
+ <&clks 0>, <&clks 118>,
+ <&clks 62>, <&clks 139>,
+ <&clks 0>;
+ clock-names = "core", "rxtx0",
+ "rxtx1", "rxtx2",
+ "rxtx3", "rxtx4",
+ "rxtx5", "rxtx6",
+ "rxtx7";
+
+ status = "okay";
+};
diff --git a/sound/soc/fsl/Kconfig b/sound/soc/fsl/Kconfig
index 3a4808d..cd088cc 100644
--- a/sound/soc/fsl/Kconfig
+++ b/sound/soc/fsl/Kconfig
@@ -1,6 +1,9 @@
config SND_SOC_FSL_SSI
tristate
+config SND_SOC_FSL_SPDIF
+ tristate
+
config SND_SOC_FSL_UTILS
tristate
diff --git a/sound/soc/fsl/Makefile b/sound/soc/fsl/Makefile
index d4b4aa8..4b5970e 100644
--- a/sound/soc/fsl/Makefile
+++ b/sound/soc/fsl/Makefile
@@ -12,9 +12,11 @@ obj-$(CONFIG_SND_SOC_P1022_RDK) += snd-soc-p1022-rdk.o
# Freescale PowerPC SSI/DMA Platform Support
snd-soc-fsl-ssi-objs := fsl_ssi.o
+snd-soc-fsl-spdif-objs := fsl_spdif.o
snd-soc-fsl-utils-objs := fsl_utils.o
snd-soc-fsl-dma-objs := fsl_dma.o
obj-$(CONFIG_SND_SOC_FSL_SSI) += snd-soc-fsl-ssi.o
+obj-$(CONFIG_SND_SOC_FSL_SPDIF) += snd-soc-fsl-spdif.o
obj-$(CONFIG_SND_SOC_FSL_UTILS) += snd-soc-fsl-utils.o
obj-$(CONFIG_SND_SOC_POWERPC_DMA) += snd-soc-fsl-dma.o
diff --git a/sound/soc/fsl/fsl_spdif.c b/sound/soc/fsl/fsl_spdif.c
new file mode 100644
index 000..b7f97a3
--- /dev/null
+++ b/sound/soc/fsl/fsl_spdif.c
@@ -0,0 +1,1281 @@
+/*
+ * Freescale S/PDIF ALSA SoC Digital Audio Interface (DAI) driver
+ *
+ * Copyright (C) 2013 Freescale Semiconductor, Inc.
+ *
+ * Based on stmp3xxx_spdif_dai.c
+ * Vladimir Barinov
+ * Copyright 2008 SigmaTel, Inc
+ * Copyright 2008 Embedded Alley Solutions, Inc
+ *
+ * This file is licensed under the terms of the GNU General Public License
+ * version 2. This program is licensed "as is" without any warranty of any
+ * kind, whether express or implied.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+#include
+
+#include "fsl_spdif.h"
+#include "imx-pcm.h"
+
+#define FSL_SPDIF_TXFIFO_WML 0x8
+#define FSL_SPDIF_RXFIFO_WML 0x8
+
+#define INTR_FOR_PLAYBACK (INT_TXFIFO_RESYNC)
+#define INTR_FOR_CAPTURE (INT_SYM_ERR | INT_BIT_ERR | INT_URX_FUL |
INT_URX_OV|\
+ INT_QRX_FUL | INT_QRX_OV | INT_UQ_SYNC | INT_UQ_ERR |\
+ INT_RXFIFO_RESYNC | INT_LOSS_LOCK | INT_DPLL_LOCKED)
+
+/* Index list for the values that has if (DPLL Locked) condition */
