| Message ID | 1462324173-10976-1-git-send-email-festevam@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
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
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
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
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
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
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?
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.
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;