ASoC: fsl_sai: Allow the SAI driver to turn on SAI_MCLK
diff mbox

Message ID 1462324173-10976-1-git-send-email-festevam@gmail.com
State New
Headers show

Commit Message

Fabio Estevam May 4, 2016, 1:09 a.m. UTC
From: Fabio Estevam <fabio.estevam@nxp.com>

On mx6ul the General Purpose Register 1 (GPR1) contains the following
bits for enabling the output of the SAI MCLKs:
SAI1_MCLK_DIR, SAI2_MCLK_DIR, SAI3_MCLK_DIR

Introduce "gpr" and "fsl,sai-enable-mclk" optional properties to allow
the enablement of the SAI_MCLK outputs.

Tested on a imx6ul-evk board.

Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
---
 .../devicetree/bindings/sound/fsl-sai.txt          |  8 +++++
 include/linux/mfd/syscon/imx6q-iomuxc-gpr.h        |  3 ++
 sound/soc/fsl/fsl_sai.c                            | 37 ++++++++++++++++++++++
 3 files changed, 48 insertions(+)

Comments

Nicolin Chen May 4, 2016, 1:44 a.m. UTC | #1
On Tue, May 03, 2016 at 10:09:33PM -0300, Fabio Estevam wrote:
> From: Fabio Estevam <fabio.estevam@nxp.com>
> 
> On mx6ul the General Purpose Register 1 (GPR1) contains the following
> bits for enabling the output of the SAI MCLKs:
> SAI1_MCLK_DIR, SAI2_MCLK_DIR, SAI3_MCLK_DIR
> 
> Introduce "gpr" and "fsl,sai-enable-mclk" optional properties to allow
> the enablement of the SAI_MCLK outputs.

The field names are literally saying "direction" instead of gating
or enabling. So it would make more sense to me that they may also
support clock inputs from the PADs. And I remember Zidan mentioned
something related to MCLK direction in one of patches. (CCed Zidan)

If it has the capability of clock input, we may need to set them in
the set_dai_sysclk() instead of simply putting in the probe(); If
not, may ignore my comments here.

> 
> Tested on a imx6ul-evk board.
> 
> Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
> ---
>  .../devicetree/bindings/sound/fsl-sai.txt          |  8 +++++
>  include/linux/mfd/syscon/imx6q-iomuxc-gpr.h        |  3 ++
>  sound/soc/fsl/fsl_sai.c                            | 37 ++++++++++++++++++++++
>  3 files changed, 48 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/sound/fsl-sai.txt b/Documentation/devicetree/bindings/sound/fsl-sai.txt
> index 044e5d7..86755bb 100644
> --- a/Documentation/devicetree/bindings/sound/fsl-sai.txt
> +++ b/Documentation/devicetree/bindings/sound/fsl-sai.txt
> @@ -48,6 +48,14 @@ Required properties:
>  			  receive data by following their own bit clocks and
>  			  frame sync clocks separately.
>  
> +Optional properties (for mx6ul):
> +
> +  - gpr			: The phandle to the General Purpose Register (GPR)
> +			  node.
> +
> +  - fsl,sai-enable-mclk	: This is a boolean property. If present, indicates
> +			  that SAI will output the SAI MCLK clock.
> +
>  Note:
>  - If both fsl,sai-asynchronous and fsl,sai-synchronous-rx are absent, the
>    default synchronous mode (sync Rx with Tx) will be used, which means both
> diff --git a/include/linux/mfd/syscon/imx6q-iomuxc-gpr.h b/include/linux/mfd/syscon/imx6q-iomuxc-gpr.h
> index 238c8db..401f97e 100644
> --- a/include/linux/mfd/syscon/imx6q-iomuxc-gpr.h
> +++ b/include/linux/mfd/syscon/imx6q-iomuxc-gpr.h
> @@ -447,5 +447,8 @@
>  #define IMX6UL_GPR1_ENET2_CLK_OUTPUT		(0x1 << 18)
>  #define IMX6UL_GPR1_ENET_CLK_DIR		(0x3 << 17)
>  #define IMX6UL_GPR1_ENET_CLK_OUTPUT		(0x3 << 17)
> +#define IMX6UL_GPR1_SAI1_MCLK_DIR		(0x1 << 19)
> +#define IMX6UL_GPR1_SAI2_MCLK_DIR		(0x1 << 20)
> +#define IMX6UL_GPR1_SAI3_MCLK_DIR		(0x1 << 21)
>  
>  #endif /* __LINUX_IMX6Q_IOMUXC_GPR_H */
> diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c
> index 0754df7..311325e 100644
> --- a/sound/soc/fsl/fsl_sai.c
> +++ b/sound/soc/fsl/fsl_sai.c
> @@ -21,6 +21,8 @@
>  #include <sound/core.h>
>  #include <sound/dmaengine_pcm.h>
>  #include <sound/pcm_params.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/mfd/syscon/imx6q-iomuxc-gpr.h>
>  
>  #include "fsl_sai.h"
>  #include "imx-pcm.h"
> @@ -785,11 +787,14 @@ static const struct regmap_config fsl_sai_regmap_config = {
>  static int fsl_sai_probe(struct platform_device *pdev)
>  {
>  	struct device_node *np = pdev->dev.of_node;
> +	struct device_node *gpr_np = of_parse_phandle(np, "gpr", 0);
>  	struct fsl_sai *sai;
> +	struct regmap *gpr;
>  	struct resource *res;
>  	void __iomem *base;
>  	char tmp[8];
>  	int irq, ret, i;
> +	u32 index;
>  
>  	sai = devm_kzalloc(&pdev->dev, sizeof(*sai), GFP_KERNEL);
>  	if (!sai)
> @@ -877,6 +882,38 @@ static int fsl_sai_probe(struct platform_device *pdev)
>  		fsl_sai_dai.symmetric_samplebits = 0;
>  	}
>  
> +	if (of_find_property(np, "fsl,sai-enable-mclk", NULL)) {

It would safer to have a compatible check here beside mentioning in
the DT binding doc.

> +		ret = of_property_read_u32(np, "sai-index", &index);
> +		if (ret) {
> +			dev_err(&pdev->dev, "could not read sai-index\n");
> +			return ret;
> +		}
> +
> +		gpr = syscon_node_to_regmap(gpr_np);
> +		if (IS_ERR(gpr)) {
> +			dev_err(&pdev->dev, "could not find gpr node\n");
> +			return PTR_ERR(gpr);
> +		}
> +
> +		switch (index) {
> +		case 1:
> +			regmap_update_bits(gpr, IOMUXC_GPR1,
> +					   IMX6UL_GPR1_SAI1_MCLK_DIR,
> +					   IMX6UL_GPR1_SAI1_MCLK_DIR);
> +			break;
> +		case 2:
> +			regmap_update_bits(gpr, IOMUXC_GPR1,
> +					   IMX6UL_GPR1_SAI2_MCLK_DIR,
> +					   IMX6UL_GPR1_SAI2_MCLK_DIR);
> +			break;
> +		case 3:
> +			regmap_update_bits(gpr, IOMUXC_GPR1,
> +					   IMX6UL_GPR1_SAI3_MCLK_DIR,
> +					   IMX6UL_GPR1_SAI3_MCLK_DIR);
> +			break;
> +		}

How about:
		regmap_update_bits(gpr, IOMUXC_GPR1,
				   IMX6UL_GPR1_SAIx_MCLK_DIR(index),
				   IMX6UL_GPR1_SAIx_MCLK_DIR(index));

Thanks
Nicolin

> +	}
> +
>  	sai->dma_params_rx.addr = res->start + FSL_SAI_RDR;
>  	sai->dma_params_tx.addr = res->start + FSL_SAI_TDR;
>  	sai->dma_params_rx.maxburst = FSL_SAI_MAXBURST_RX;
> -- 
> 1.9.1
>
Fabio Estevam May 4, 2016, 2:01 a.m. UTC | #2
On Tue, May 3, 2016 at 10:44 PM, Nicolin Chen <nicoleotsuka@gmail.com> wrote:

> The field names are literally saying "direction" instead of gating
> or enabling. So it would make more sense to me that they may also

Looking in the MX6UL Referece Manual we have the following description
for these bits:

0 - "...output driver is disabled..."

1 - "...output driver is enabled..."

That's why I preferred using 'enabled' in the property name.

I removed Zidan's email from Cc as his email address is no longer valid.

>> +     if (of_find_property(np, "fsl,sai-enable-mclk", NULL)) {
>
> It would safer to have a compatible check here beside mentioning in
> the DT binding doc.

In this case I need to add a { .compatible = "fsl,imx6ul-sai", }, entry.

I can do that.

> How about:
>                 regmap_update_bits(gpr, IOMUXC_GPR1,
>                                    IMX6UL_GPR1_SAIx_MCLK_DIR(index),
>                                    IMX6UL_GPR1_SAIx_MCLK_DIR(index));

That's a good idea.

Will send a v2 tomorrow.

Thanks
Nicolin Chen May 4, 2016, 7:07 a.m. UTC | #3
On Tue, May 03, 2016 at 11:01:15PM -0300, Fabio Estevam wrote:
> On Tue, May 3, 2016 at 10:44 PM, Nicolin Chen <nicoleotsuka@gmail.com> wrote:
> 
> > The field names are literally saying "direction" instead of gating
> > or enabling. So it would make more sense to me that they may also
> 
> Looking in the MX6UL Referece Manual we have the following description
> for these bits:
> 
> 0 - "...output driver is disabled..."
> 
> 1 - "...output driver is enabled..."
> 
> That's why I preferred using 'enabled' in the property name.

I found out the mail from Zidan regarding the same GPR bit:
http://mailman.alsa-project.org/pipermail/alsa-devel/2015-August/096401.html

According to his reply, this bit also controls the clock source of MCLK2
for the SAI. Each SAI has 3 MCLKs, but first disregarding MCLK3:

When this bit gets set, the MCLK1 and MCLK2 of the corresponding SAI are
both getting clock from CCM, and in the meantime outputting the clock via
the PAD to external Codec chips. In this case, MCLK1 and MCLK2 have same
clock rate, nothing special for bit clock dividing.

When this bit gets clear, MCLK1 of the corresponding is still getting its
clock from CCM while MCLK2 switches its source from CCM to the PAD. In this
case, MCLK1 and MCLK2 can have different clock rates so as to support two
sample rate groups: 44.1KHz and 48Khz.

So, beside gating the clock output, it's more likely a clock MUX for MCLK2
of each SAI to switch between CCM and external input.

Because DT property will be hard to change once we define it. I think it
would be better to confirm this first before patching it (with Zidan or
i.MX IC team). But the driver part, whether putting it to probe() or to
set_dai_sysclk(), doesn't matter to me since we can change/improve it
later as long as it follows the correct binding.

Thanks
Nicolin

> 
> I removed Zidan's email from Cc as his email address is no longer valid.
> 
> >> +     if (of_find_property(np, "fsl,sai-enable-mclk", NULL)) {
> >
> > It would safer to have a compatible check here beside mentioning in
> > the DT binding doc.
> 
> In this case I need to add a { .compatible = "fsl,imx6ul-sai", }, entry.
> 
> I can do that.
> 
> > How about:
> >                 regmap_update_bits(gpr, IOMUXC_GPR1,
> >                                    IMX6UL_GPR1_SAIx_MCLK_DIR(index),
> >                                    IMX6UL_GPR1_SAIx_MCLK_DIR(index));
> 
> That's a good idea.
> 
> Will send a v2 tomorrow.
> 
> Thanks
Fabio Estevam May 4, 2016, 12:17 p.m. UTC | #4
Hi Nicolin,

On Wed, May 4, 2016 at 4:07 AM, Nicolin Chen <nicoleotsuka@gmail.com> wrote:

> I found out the mail from Zidan regarding the same GPR bit:
> http://mailman.alsa-project.org/pipermail/alsa-devel/2015-August/096401.html
>
> According to his reply, this bit also controls the clock source of MCLK2
> for the SAI. Each SAI has 3 MCLKs, but first disregarding MCLK3:
>
> When this bit gets set, the MCLK1 and MCLK2 of the corresponding SAI are
> both getting clock from CCM, and in the meantime outputting the clock via
> the PAD to external Codec chips. In this case, MCLK1 and MCLK2 have same
> clock rate, nothing special for bit clock dividing.
>
> When this bit gets clear, MCLK1 of the corresponding is still getting its
> clock from CCM while MCLK2 switches its source from CCM to the PAD. In this
> case, MCLK1 and MCLK2 can have different clock rates so as to support two
> sample rate groups: 44.1KHz and 48Khz.
>
> So, beside gating the clock output, it's more likely a clock MUX for MCLK2
> of each SAI to switch between CCM and external input.

At imx6ul.dtsi the mclk2 and mclk3 are just dummy clocks.

> Because DT property will be hard to change once we define it. I think it
> would be better to confirm this first before patching it (with Zidan or
> i.MX IC team). But the driver part, whether putting it to probe() or to
> set_dai_sysclk(), doesn't matter to me since we can change/improve it
> later as long as it follows the correct binding.

Also tried Zidan's NXP email and it also bounced, so not able to contact him.

Also don't have any contact with the audio folks in the i.MX IC team.

This binding is all about setting IMX6UL_GPR1_SAIx_MCLK_DIR or not.

So this is a very simple case:

IMX6UL_GPR1_SAIx_MCLK_DIR = 0 (this is the current behaviour)

IMX6UL_GPR1_SAIx_MCLK_DIR = 1 (this is what this patch allows)

I cannot see why this proposed binding would possibly break things or
would need a change in the future.

It's only purpose is to simply set IMX6UL_GPR1_SAIx_MCLK_DIR.

Regards,

Fabio Estevam
Nicolin Chen May 4, 2016, 5:29 p.m. UTC | #5
Hi Fabio,

On Wed, May 04, 2016 at 09:17:11AM -0300, Fabio Estevam wrote:
> Hi Nicolin,
> 
> On Wed, May 4, 2016 at 4:07 AM, Nicolin Chen <nicoleotsuka@gmail.com> wrote:
> 
> > I found out the mail from Zidan regarding the same GPR bit:
> > http://mailman.alsa-project.org/pipermail/alsa-devel/2015-August/096401.html
> >
> > According to his reply, this bit also controls the clock source of MCLK2
> > for the SAI. Each SAI has 3 MCLKs, but first disregarding MCLK3:
> >
> > When this bit gets set, the MCLK1 and MCLK2 of the corresponding SAI are
> > both getting clock from CCM, and in the meantime outputting the clock via
> > the PAD to external Codec chips. In this case, MCLK1 and MCLK2 have same
> > clock rate, nothing special for bit clock dividing.
> >
> > When this bit gets clear, MCLK1 of the corresponding is still getting its
> > clock from CCM while MCLK2 switches its source from CCM to the PAD. In this
> > case, MCLK1 and MCLK2 can have different clock rates so as to support two
> > sample rate groups: 44.1KHz and 48Khz.
> >
> > So, beside gating the clock output, it's more likely a clock MUX for MCLK2
> > of each SAI to switch between CCM and external input.
> 
> At imx6ul.dtsi the mclk2 and mclk3 are just dummy clocks.

We both know sometimes the source code might not reflect the true
IC design because a part of the design is not documented in detail,
some hidden registers in SSI for example.

> > Because DT property will be hard to change once we define it. I think it
> > would be better to confirm this first before patching it (with Zidan or
> > i.MX IC team). But the driver part, whether putting it to probe() or to
> > set_dai_sysclk(), doesn't matter to me since we can change/improve it
> > later as long as it follows the correct binding.
> 
> Also tried Zidan's NXP email and it also bounced, so not able to contact him.
> 
> Also don't have any contact with the audio folks in the i.MX IC team.
> 
> This binding is all about setting IMX6UL_GPR1_SAIx_MCLK_DIR or not.
>
> So this is a very simple case:
> 
> IMX6UL_GPR1_SAIx_MCLK_DIR = 0 (this is the current behaviour)
> 
> IMX6UL_GPR1_SAIx_MCLK_DIR = 1 (this is what this patch allows)

I have already described the difference between set and clear is
not this simple. Honestly, without reading Zidan's mail above,
I would suggest to set all three bits for SAIs in mach-imx6ul.c
anyway since they are just simply enablers. Since each SAI clock
has a gate in CCM, why bother to set these three bits permanently.

> I cannot see why this proposed binding would possibly break things or
> would need a change in the future.

I have never said anything about breaking things but only accuracy
of the property name.

> It's only purpose is to simply set IMX6UL_GPR1_SAIx_MCLK_DIR.

I understand you want to get this in as soon as possible, that's why
I said I can ignore the place of implementation as long as the name
of the DT property won't create confusion for input clock feature.
So why not just use the name as itself -- direction?

Thanks
Nicolin

> 
> Regards,
> 
> Fabio Estevam
Fabio Estevam May 4, 2016, 5:41 p.m. UTC | #6
Hi Nicolin,

On Wed, May 4, 2016 at 2:29 PM, Nicolin Chen <nicoleotsuka@gmail.com> wrote:

> I have already described the difference between set and clear is
> not this simple. Honestly, without reading Zidan's mail above,
> I would suggest to set all three bits for SAIs in mach-imx6ul.c

I think Shawn was against this idea.

> I understand you want to get this in as soon as possible, that's why
> I said I can ignore the place of implementation as long as the name
> of the DT property won't create confusion for input clock feature.
> So why not just use the name as itself -- direction?

Would you be happy if I do a v2 that uses a property like:

fsl,sai-mclk-direction = <1>;

Thanks
Fabio Estevam May 4, 2016, 5:51 p.m. UTC | #7
On Wed, May 4, 2016 at 2:41 PM, Fabio Estevam <festevam@gmail.com> wrote:

> Would you be happy if I do a v2 that uses a property like:
>
> fsl,sai-mclk-direction = <1>;

As it is boolean, this could be simply "fsl,sai-mclk-direction-output".

What do you think?
Nicolin Chen May 4, 2016, 5:55 p.m. UTC | #8
On Wed, May 04, 2016 at 02:51:14PM -0300, Fabio Estevam wrote:
> On Wed, May 4, 2016 at 2:41 PM, Fabio Estevam <festevam@gmail.com> wrote:
> 
> > Would you be happy if I do a v2 that uses a property like:
> >
> > fsl,sai-mclk-direction = <1>;
> 
> As it is boolean, this could be simply "fsl,sai-mclk-direction-output".
> 
> What do you think?

Yea, that looks perfect to me. At this point, we only need to support
MCLK1 output feature. Whenever we want more than this, we can simply
update the description of the property.

Patch
diff mbox

diff --git a/Documentation/devicetree/bindings/sound/fsl-sai.txt b/Documentation/devicetree/bindings/sound/fsl-sai.txt
index 044e5d7..86755bb 100644
--- a/Documentation/devicetree/bindings/sound/fsl-sai.txt
+++ b/Documentation/devicetree/bindings/sound/fsl-sai.txt
@@ -48,6 +48,14 @@  Required properties:
 			  receive data by following their own bit clocks and
 			  frame sync clocks separately.
 
+Optional properties (for mx6ul):
+
+  - gpr			: The phandle to the General Purpose Register (GPR)
+			  node.
+
+  - fsl,sai-enable-mclk	: This is a boolean property. If present, indicates
+			  that SAI will output the SAI MCLK clock.
+
 Note:
 - If both fsl,sai-asynchronous and fsl,sai-synchronous-rx are absent, the
   default synchronous mode (sync Rx with Tx) will be used, which means both
diff --git a/include/linux/mfd/syscon/imx6q-iomuxc-gpr.h b/include/linux/mfd/syscon/imx6q-iomuxc-gpr.h
index 238c8db..401f97e 100644
--- a/include/linux/mfd/syscon/imx6q-iomuxc-gpr.h
+++ b/include/linux/mfd/syscon/imx6q-iomuxc-gpr.h
@@ -447,5 +447,8 @@ 
 #define IMX6UL_GPR1_ENET2_CLK_OUTPUT		(0x1 << 18)
 #define IMX6UL_GPR1_ENET_CLK_DIR		(0x3 << 17)
 #define IMX6UL_GPR1_ENET_CLK_OUTPUT		(0x3 << 17)
+#define IMX6UL_GPR1_SAI1_MCLK_DIR		(0x1 << 19)
+#define IMX6UL_GPR1_SAI2_MCLK_DIR		(0x1 << 20)
+#define IMX6UL_GPR1_SAI3_MCLK_DIR		(0x1 << 21)
 
 #endif /* __LINUX_IMX6Q_IOMUXC_GPR_H */
diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c
index 0754df7..311325e 100644
--- a/sound/soc/fsl/fsl_sai.c
+++ b/sound/soc/fsl/fsl_sai.c
@@ -21,6 +21,8 @@ 
 #include <sound/core.h>
 #include <sound/dmaengine_pcm.h>
 #include <sound/pcm_params.h>
+#include <linux/mfd/syscon.h>
+#include <linux/mfd/syscon/imx6q-iomuxc-gpr.h>
 
 #include "fsl_sai.h"
 #include "imx-pcm.h"
@@ -785,11 +787,14 @@  static const struct regmap_config fsl_sai_regmap_config = {
 static int fsl_sai_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
+	struct device_node *gpr_np = of_parse_phandle(np, "gpr", 0);
 	struct fsl_sai *sai;
+	struct regmap *gpr;
 	struct resource *res;
 	void __iomem *base;
 	char tmp[8];
 	int irq, ret, i;
+	u32 index;
 
 	sai = devm_kzalloc(&pdev->dev, sizeof(*sai), GFP_KERNEL);
 	if (!sai)
@@ -877,6 +882,38 @@  static int fsl_sai_probe(struct platform_device *pdev)
 		fsl_sai_dai.symmetric_samplebits = 0;
 	}
 
+	if (of_find_property(np, "fsl,sai-enable-mclk", NULL)) {
+		ret = of_property_read_u32(np, "sai-index", &index);
+		if (ret) {
+			dev_err(&pdev->dev, "could not read sai-index\n");
+			return ret;
+		}
+
+		gpr = syscon_node_to_regmap(gpr_np);
+		if (IS_ERR(gpr)) {
+			dev_err(&pdev->dev, "could not find gpr node\n");
+			return PTR_ERR(gpr);
+		}
+
+		switch (index) {
+		case 1:
+			regmap_update_bits(gpr, IOMUXC_GPR1,
+					   IMX6UL_GPR1_SAI1_MCLK_DIR,
+					   IMX6UL_GPR1_SAI1_MCLK_DIR);
+			break;
+		case 2:
+			regmap_update_bits(gpr, IOMUXC_GPR1,
+					   IMX6UL_GPR1_SAI2_MCLK_DIR,
+					   IMX6UL_GPR1_SAI2_MCLK_DIR);
+			break;
+		case 3:
+			regmap_update_bits(gpr, IOMUXC_GPR1,
+					   IMX6UL_GPR1_SAI3_MCLK_DIR,
+					   IMX6UL_GPR1_SAI3_MCLK_DIR);
+			break;
+		}
+	}
+
 	sai->dma_params_rx.addr = res->start + FSL_SAI_RDR;
 	sai->dma_params_tx.addr = res->start + FSL_SAI_TDR;
 	sai->dma_params_rx.maxburst = FSL_SAI_MAXBURST_RX;