[PATCH 1/5] ARM: dts: imx6sx: Fix sdma node

[PATCH 1/5] ARM: dts: imx6sx: Fix sdma node

[Fabio Estevam]

From: Fabio Estevam <fabio.estevam@freescale.com>

Use the correct compatible string for sdma and also provide the sdma firmware
path.

Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
 arch/arm/boot/dts/imx6sx.dtsi | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/imx6sx.dtsi b/arch/arm/boot/dts/imx6sx.dtsi
index a7d74e7..2929078 100644
--- a/arch/arm/boot/dts/imx6sx.dtsi
+++ b/arch/arm/boot/dts/imx6sx.dtsi
@@ -703,13 +703,15 @@
 			};
 
 			sdma: sdma at 020ec000 {
-				compatible = "fsl,imx6sx-sdma";
+				compatible = "fsl,imx6sx-sdma", "fsl,imx35-sdma";
 				reg = <0x020ec000 0x4000>;
 				interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>;
 				clocks = <&clks IMX6SX_CLK_SDMA>,
 					 <&clks IMX6SX_CLK_SDMA>;
 				clock-names = "ipg", "ahb";
 				#dma-cells = <3>;
+				/* imx6sx reuses imx6q sdma firmware */
+				fsl,sdma-ram-script-name = "imx/sdma/sdma-imx6q.bin";
 			};
 		};
 
-- 
1.8.3.2

[PATCH 2/5] ARM: dts: imx6sx: Fix SSI nodes

[Fabio Estevam]

From: Fabio Estevam <fabio.estevam@freescale.com>

The fsl_ssi driver only needs one clock to work, so only pass the required
SSI clock as done in the other mx6 dtsi files.

Also pass the  fsl,fifo-depth property.

Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
 arch/arm/boot/dts/imx6sx.dtsi | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/arch/arm/boot/dts/imx6sx.dtsi b/arch/arm/boot/dts/imx6sx.dtsi
index 2929078..ef3cea2 100644
--- a/arch/arm/boot/dts/imx6sx.dtsi
+++ b/arch/arm/boot/dts/imx6sx.dtsi
@@ -301,11 +301,10 @@
 					compatible = "fsl,imx6sx-ssi", "fsl,imx21-ssi";
 					reg = <0x02028000 0x4000>;
 					interrupts = <GIC_SPI 46 IRQ_TYPE_LEVEL_HIGH>;
-					clocks = <&clks IMX6SX_CLK_SSI1_IPG>,
-						 <&clks IMX6SX_CLK_SSI1>;
-					clock-names = "ipg", "baud";
+					clocks = <&clks IMX6SX_CLK_SSI1>;
 					dmas = <&sdma 37 1 0>, <&sdma 38 1 0>;
 					dma-names = "rx", "tx";
+					fsl,fifo-depth = <15>;
 					status = "disabled";
 				};
 
@@ -313,11 +312,10 @@
 					compatible = "fsl,imx6sx-ssi", "fsl,imx21-ssi";
 					reg = <0x0202c000 0x4000>;
 					interrupts = <GIC_SPI 47 IRQ_TYPE_LEVEL_HIGH>;
-					clocks = <&clks IMX6SX_CLK_SSI2_IPG>,
-						 <&clks IMX6SX_CLK_SSI2>;
-					clock-names = "ipg", "baud";
+					clocks = <&clks IMX6SX_CLK_SSI2>;
 					dmas = <&sdma 41 1 0>, <&sdma 42 1 0>;
 					dma-names = "rx", "tx";
+					fsl,fifo-depth = <15>;
 					status = "disabled";
 				};
 
@@ -325,11 +323,10 @@
 					compatible = "fsl,imx6sx-ssi", "fsl,imx21-ssi";
 					reg = <0x02030000 0x4000>;
 					interrupts = <GIC_SPI 48 IRQ_TYPE_LEVEL_HIGH>;
-					clocks = <&clks IMX6SX_CLK_SSI3_IPG>,
-						 <&clks IMX6SX_CLK_SSI3>;
-					clock-names = "ipg", "baud";
+					clocks = <&clks IMX6SX_CLK_SSI3>;
 					dmas = <&sdma 45 1 0>, <&sdma 46 1 0>;
 					dma-names = "rx", "tx";
+					fsl,fifo-depth = <15>;
 					status = "disabled";
 				};
 
-- 
1.8.3.2

[PATCH 3/5] ARM: imx: clk-imx6sx: Remove SSI IPG clocks

[Fabio Estevam]

From: Fabio Estevam <fabio.estevam@freescale.com>

Looking at the CCGR5 register we see that that each SSI has its corresponding
clock gate field.

There are no SSI IPG clock gate field, so remove these SSI IPG variants as
they point to the same SSI clock fields and have no use in the kernel.

Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
 arch/arm/mach-imx/clk-imx6sx.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/arch/arm/mach-imx/clk-imx6sx.c b/arch/arm/mach-imx/clk-imx6sx.c
index 2e96103..1b198ea 100644
--- a/arch/arm/mach-imx/clk-imx6sx.c
+++ b/arch/arm/mach-imx/clk-imx6sx.c
@@ -409,9 +409,6 @@ static void __init imx6sx_clocks_init(struct device_node *ccm_node)
 	clks[IMX6SX_CLK_SPBA]         = imx_clk_gate2("spba",          "ipg",               base + 0x7c, 12);
 	clks[IMX6SX_CLK_AUDIO]        = imx_clk_gate2_shared("audio",  "audio_podf",        base + 0x7c, 14, &share_count_audio);
 	clks[IMX6SX_CLK_SPDIF]        = imx_clk_gate2_shared("spdif",  "spdif_podf",        base + 0x7c, 14, &share_count_audio);
-	clks[IMX6SX_CLK_SSI1_IPG]     = imx_clk_gate2("ssi1_ipg",      "ipg",               base + 0x7c, 18);
-	clks[IMX6SX_CLK_SSI2_IPG]     = imx_clk_gate2("ssi2_ipg",      "ipg",               base + 0x7c, 20);
-	clks[IMX6SX_CLK_SSI3_IPG]     = imx_clk_gate2("ssi3_ipg",      "ipg",               base + 0x7c, 22);
 	clks[IMX6SX_CLK_SSI1]         = imx_clk_gate2("ssi1",          "ssi1_podf",         base + 0x7c, 18);
 	clks[IMX6SX_CLK_SSI2]         = imx_clk_gate2("ssi2",          "ssi2_podf",         base + 0x7c, 20);
 	clks[IMX6SX_CLK_SSI3]         = imx_clk_gate2("ssi3",          "ssi3_podf",         base + 0x7c, 22);
-- 
1.8.3.2

[PATCH 4/5] ARM: imx: clk-gate2: Use post decrement for share_count

[Fabio Estevam]

From: Fabio Estevam <fabio.estevam@freescale.com>

SSI clocks use the share_count mechanism since SSI and SPDIF share the same
clock gate bits.

When using the share_count for the SSI clock we notice that it gets disabled
due to the usage of pre-decrement operation in the clk_gate2_disable() function.

Use the post-decrement operation so that the correct share_count is used and the
SSI clock does not get disable when an audio file needs to be played. 

Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
 arch/arm/mach-imx/clk-gate2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/mach-imx/clk-gate2.c b/arch/arm/mach-imx/clk-gate2.c
index 4ba587d..463083c 100644
--- a/arch/arm/mach-imx/clk-gate2.c
+++ b/arch/arm/mach-imx/clk-gate2.c
@@ -67,7 +67,7 @@ static void clk_gate2_disable(struct clk_hw *hw)
 
 	spin_lock_irqsave(gate->lock, flags);
 
-	if (gate->share_count && --(*gate->share_count) > 0)
+	if (gate->share_count && (*gate->share_count)-- > 0)
 		goto out;
 
 	reg = readl(gate->reg);
-- 
1.8.3.2

[PATCH 5/5] ARM: dts: imx6sx-sdb: Add audio support

[Fabio Estevam]

From: Fabio Estevam <fabio.estevam@freescale.com>

Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
 arch/arm/boot/dts/imx6sx-sdb.dts | 72 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 72 insertions(+)

diff --git a/arch/arm/boot/dts/imx6sx-sdb.dts b/arch/arm/boot/dts/imx6sx-sdb.dts
index 2571261..a3980d9 100644
--- a/arch/arm/boot/dts/imx6sx-sdb.dts
+++ b/arch/arm/boot/dts/imx6sx-sdb.dts
@@ -82,9 +82,39 @@
 			gpio = <&gpio1 12 GPIO_ACTIVE_HIGH>;
 			enable-active-high;
 		};
+
+		reg_psu_5v: regulator at 3 {
+			compatible = "regulator-fixed";
+			reg = <3>;
+			regulator-name = "PSU-5V0";
+			regulator-min-microvolt = <5000000>;
+			regulator-max-microvolt = <5000000>;
+		};
+	};
+
+	sound {
+		compatible = "fsl,imx6sx-sdb-wm8962", "fsl,imx-audio-wm8962";
+		model = "wm8962-audio";
+		ssi-controller = <&ssi2>;
+		audio-codec = <&codec>;
+		audio-routing =
+			"Headphone Jack", "HPOUTL",
+			"Headphone Jack", "HPOUTR",
+			"Ext Spk", "SPKOUTL",
+			"Ext Spk", "SPKOUTR",
+			"AMIC", "MICBIAS",
+			"IN3R", "AMIC";
+		mux-int-port = <2>;
+		mux-ext-port = <6>;
 	};
 };
 
+&audmux {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_audmux>;
+	status = "okay";
+};
+
 &fec1 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_enet1>;
@@ -200,6 +230,31 @@
 	};
 };
 
+&i2c4 {
+        clock-frequency = <100000>;
+        pinctrl-names = "default";
+        pinctrl-0 = <&pinctrl_i2c4>;
+        status = "okay";
+
+	codec: wm8962 at 1a {
+		compatible = "wlf,wm8962";
+		reg = <0x1a>;
+		clocks = <&clks IMX6SX_CLK_AUDIO>;
+		DCVDD-supply = <&vgen4_reg>;
+		DBVDD-supply = <&vgen4_reg>;
+		AVDD-supply = <&vgen4_reg>;
+		CPVDD-supply = <&vgen4_reg>;
+		MICVDD-supply = <&vgen3_reg>;
+		PLLVDD-supply = <&vgen4_reg>;
+		SPKVDD1-supply = <&reg_psu_5v>;
+		SPKVDD2-supply = <&reg_psu_5v>;
+	};
+};
+
+&ssi2 {
+	status = "okay";
+};
+
 &uart1 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_uart1>;
@@ -260,6 +315,16 @@
 
 &iomuxc {
 	imx6x-sdb {
+		pinctrl_audmux: audmuxgrp {
+			fsl,pins = <
+				MX6SX_PAD_CSI_DATA00__AUDMUX_AUD6_TXC	0x130b0
+				MX6SX_PAD_CSI_DATA01__AUDMUX_AUD6_TXFS	0x130b0
+				MX6SX_PAD_CSI_HSYNC__AUDMUX_AUD6_TXD	0x120b0
+				MX6SX_PAD_CSI_VSYNC__AUDMUX_AUD6_RXD	0x130b0
+				MX6SX_PAD_CSI_PIXCLK__AUDMUX_MCLK	0x130b0
+			>;
+		};
+
 		pinctrl_enet1: enet1grp {
 			fsl,pins = <
 				MX6SX_PAD_ENET1_MDIO__ENET1_MDIO	0xa0b1
@@ -293,6 +358,13 @@
 			>;
 		};
 
+		pinctrl_i2c4: i2c4grp {
+			fsl,pins = <
+				MX6SX_PAD_CSI_DATA07__I2C4_SDA		0x4001b8b1
+				MX6SX_PAD_CSI_DATA06__I2C4_SCL		0x4001b8b1
+			>;
+		};
+
 		pinctrl_vcc_sd3: vccsd3grp {
 			fsl,pins = <
 				MX6SX_PAD_KEY_COL1__GPIO2_IO_11		0x17059
-- 
1.8.3.2

[PATCH 3/5] ARM: imx: clk-imx6sx: Remove SSI IPG clocks

[Nicolin Chen]

Hi Fabio,
   
On Tue, Jul 01, 2014 at 02:55:27AM -0300, Fabio Estevam wrote:
> From: Fabio Estevam <fabio.estevam@freescale.com>
> 
> Looking at the CCGR5 register we see that that each SSI has its corresponding
> clock gate field.
> 
> There are no SSI IPG clock gate field, so remove these SSI IPG variants as
> they point to the same SSI clock fields and have no use in the kernel.

I was just abort to say that actually each SSI has an IPG clock as CCM shows:

SSIn      ssi_clk      ssin_clk_root      CCGR5[CG11:CG9] (ssi[3:1]_clk_enable)
          ipg_clk      ipg_clk_root       CCGR5[CG11:CG9] (ssi[3:1]_clk_enable)
          ipg_clk_s    ipg_clk_root       CCGR5[CG11:CG9] (ssi[3:1]_clk_enable)

So we don't have to remove the IPG one. Instead we shall protect IPG and baud
clocks with imx_clk_gate2_shared().

Thank you,
Nicolin

> 
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> ---
>  arch/arm/mach-imx/clk-imx6sx.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/arch/arm/mach-imx/clk-imx6sx.c b/arch/arm/mach-imx/clk-imx6sx.c
> index 2e96103..1b198ea 100644
> --- a/arch/arm/mach-imx/clk-imx6sx.c
> +++ b/arch/arm/mach-imx/clk-imx6sx.c
> @@ -409,9 +409,6 @@ static void __init imx6sx_clocks_init(struct device_node *ccm_node)
>  	clks[IMX6SX_CLK_SPBA]         = imx_clk_gate2("spba",          "ipg",               base + 0x7c, 12);
>  	clks[IMX6SX_CLK_AUDIO]        = imx_clk_gate2_shared("audio",  "audio_podf",        base + 0x7c, 14, &share_count_audio);
>  	clks[IMX6SX_CLK_SPDIF]        = imx_clk_gate2_shared("spdif",  "spdif_podf",        base + 0x7c, 14, &share_count_audio);
> -	clks[IMX6SX_CLK_SSI1_IPG]     = imx_clk_gate2("ssi1_ipg",      "ipg",               base + 0x7c, 18);
> -	clks[IMX6SX_CLK_SSI2_IPG]     = imx_clk_gate2("ssi2_ipg",      "ipg",               base + 0x7c, 20);
> -	clks[IMX6SX_CLK_SSI3_IPG]     = imx_clk_gate2("ssi3_ipg",      "ipg",               base + 0x7c, 22);
>  	clks[IMX6SX_CLK_SSI1]         = imx_clk_gate2("ssi1",          "ssi1_podf",         base + 0x7c, 18);
>  	clks[IMX6SX_CLK_SSI2]         = imx_clk_gate2("ssi2",          "ssi2_podf",         base + 0x7c, 20);
>  	clks[IMX6SX_CLK_SSI3]         = imx_clk_gate2("ssi3",          "ssi3_podf",         base + 0x7c, 22);
> -- 
> 1.8.3.2
> 

[PATCH 2/5] ARM: dts: imx6sx: Fix SSI nodes

[Markus Pargmann]

Hi Fabio,

On Tue, Jul 01, 2014 at 02:55:26AM -0300, Fabio Estevam wrote:
> From: Fabio Estevam <fabio.estevam@freescale.com>
> 
> The fsl_ssi driver only needs one clock to work, so only pass the required
> SSI clock as done in the other mx6 dtsi files.

Actually I would prefer to have the clock defined in the .dtsi files.
The fsl-ssi driver only enables the baud clock if the SSI unit is the
clock master. So in all other cases the clock is not enabled and keeps
disabled.

Also the clock is SSI specific, you don't have a choice which clock to
use, there is no board dependency on this clock. So I think we should
fix the other imx6 dtsi files instead.

> 
> Also pass the  fsl,fifo-depth property.
> 
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> ---
>  arch/arm/boot/dts/imx6sx.dtsi | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/imx6sx.dtsi b/arch/arm/boot/dts/imx6sx.dtsi
> index 2929078..ef3cea2 100644
> --- a/arch/arm/boot/dts/imx6sx.dtsi
> +++ b/arch/arm/boot/dts/imx6sx.dtsi
> @@ -301,11 +301,10 @@
>  					compatible = "fsl,imx6sx-ssi", "fsl,imx21-ssi";
>  					reg = <0x02028000 0x4000>;
>  					interrupts = <GIC_SPI 46 IRQ_TYPE_LEVEL_HIGH>;
> -					clocks = <&clks IMX6SX_CLK_SSI1_IPG>,
> -						 <&clks IMX6SX_CLK_SSI1>;
> -					clock-names = "ipg", "baud";
> +					clocks = <&clks IMX6SX_CLK_SSI1>;

It seems you are removing the wrong clock here. 'ipg' is necessary,
'baud' is only used for master mode.

Regards,

Markus

>  					dmas = <&sdma 37 1 0>, <&sdma 38 1 0>;
>  					dma-names = "rx", "tx";
> +					fsl,fifo-depth = <15>;
>  					status = "disabled";
>  				};
>  
> @@ -313,11 +312,10 @@
>  					compatible = "fsl,imx6sx-ssi", "fsl,imx21-ssi";
>  					reg = <0x0202c000 0x4000>;
>  					interrupts = <GIC_SPI 47 IRQ_TYPE_LEVEL_HIGH>;
> -					clocks = <&clks IMX6SX_CLK_SSI2_IPG>,
> -						 <&clks IMX6SX_CLK_SSI2>;
> -					clock-names = "ipg", "baud";
> +					clocks = <&clks IMX6SX_CLK_SSI2>;
>  					dmas = <&sdma 41 1 0>, <&sdma 42 1 0>;
>  					dma-names = "rx", "tx";
> +					fsl,fifo-depth = <15>;
>  					status = "disabled";
>  				};
>  
> @@ -325,11 +323,10 @@
>  					compatible = "fsl,imx6sx-ssi", "fsl,imx21-ssi";
>  					reg = <0x02030000 0x4000>;
>  					interrupts = <GIC_SPI 48 IRQ_TYPE_LEVEL_HIGH>;
> -					clocks = <&clks IMX6SX_CLK_SSI3_IPG>,
> -						 <&clks IMX6SX_CLK_SSI3>;
> -					clock-names = "ipg", "baud";
> +					clocks = <&clks IMX6SX_CLK_SSI3>;
>  					dmas = <&sdma 45 1 0>, <&sdma 46 1 0>;
>  					dma-names = "rx", "tx";
> +					fsl,fifo-depth = <15>;
>  					status = "disabled";
>  				};
>  
> -- 
> 1.8.3.2
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

-- 
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-5555 |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140701/a8b5123f/attachment.sig>

[PATCH 2/5] ARM: dts: imx6sx: Fix SSI nodes

[Markus Pargmann]

On Tue, Jul 01, 2014 at 09:45:11AM +0200, Markus Pargmann wrote:
> Hi Fabio,
> 
> On Tue, Jul 01, 2014 at 02:55:26AM -0300, Fabio Estevam wrote:
> > From: Fabio Estevam <fabio.estevam@freescale.com>
> > 
> > The fsl_ssi driver only needs one clock to work, so only pass the required
> > SSI clock as done in the other mx6 dtsi files.
> 
> Actually I would prefer to have the clock defined in the .dtsi files.
> The fsl-ssi driver only enables the baud clock if the SSI unit is the
> clock master. So in all other cases the clock is not enabled and keeps
> disabled.
> 
> Also the clock is SSI specific, you don't have a choice which clock to
> use, there is no board dependency on this clock. So I think we should
> fix the other imx6 dtsi files instead.
> 
> > 
> > Also pass the  fsl,fifo-depth property.
> > 
> > Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> > ---
> >  arch/arm/boot/dts/imx6sx.dtsi | 15 ++++++---------
> >  1 file changed, 6 insertions(+), 9 deletions(-)
> > 
> > diff --git a/arch/arm/boot/dts/imx6sx.dtsi b/arch/arm/boot/dts/imx6sx.dtsi
> > index 2929078..ef3cea2 100644
> > --- a/arch/arm/boot/dts/imx6sx.dtsi
> > +++ b/arch/arm/boot/dts/imx6sx.dtsi
> > @@ -301,11 +301,10 @@
> >  					compatible = "fsl,imx6sx-ssi", "fsl,imx21-ssi";
> >  					reg = <0x02028000 0x4000>;
> >  					interrupts = <GIC_SPI 46 IRQ_TYPE_LEVEL_HIGH>;
> > -					clocks = <&clks IMX6SX_CLK_SSI1_IPG>,
> > -						 <&clks IMX6SX_CLK_SSI1>;
> > -					clock-names = "ipg", "baud";
> > +					clocks = <&clks IMX6SX_CLK_SSI1>;
> 
> It seems you are removing the wrong clock here. 'ipg' is necessary,
> 'baud' is only used for master mode.

Oh a later patch explains the removal of the 'wrong' clock. So just
ignore this part.

Regards,

Markus

-- 
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-5555 |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140701/7bdb8965/attachment-0001.sig>

[PATCH 4/5] ARM: imx: clk-gate2: Use post decrement for share_count

[Shawn Guo]

On Tue, Jul 01, 2014 at 02:55:28AM -0300, Fabio Estevam wrote:
> From: Fabio Estevam <fabio.estevam@freescale.com>
> 
> SSI clocks use the share_count mechanism since SSI and SPDIF share the same
> clock gate bits.
> 
> When using the share_count for the SSI clock we notice that it gets disabled
> due to the usage of pre-decrement operation in the clk_gate2_disable() function.
> 
> Use the post-decrement operation so that the correct share_count is used and the
> SSI clock does not get disable when an audio file needs to be played. 
> 
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> ---
>  arch/arm/mach-imx/clk-gate2.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm/mach-imx/clk-gate2.c b/arch/arm/mach-imx/clk-gate2.c
> index 4ba587d..463083c 100644
> --- a/arch/arm/mach-imx/clk-gate2.c
> +++ b/arch/arm/mach-imx/clk-gate2.c
> @@ -67,7 +67,7 @@ static void clk_gate2_disable(struct clk_hw *hw)
>  
>  	spin_lock_irqsave(gate->lock, flags);
>  
> -	if (gate->share_count && --(*gate->share_count) > 0)
> +	if (gate->share_count && (*gate->share_count)-- > 0)

The change makes no sense.  Let's say that clk_gate2_disable() gets
called with share_count being 1.  In this case, we should access
register to gate the clock, right?

Shawn

>  		goto out;
>  
>  	reg = readl(gate->reg);
> -- 
> 1.8.3.2
> 

[PATCH 4/5] ARM: imx: clk-gate2: Use post decrement for share_count

[Fabio Estevam]

On Tue, Jul 1, 2014 at 8:52 AM, Shawn Guo <shawn.guo@freescale.com> wrote:

>> --- a/arch/arm/mach-imx/clk-gate2.c
>> +++ b/arch/arm/mach-imx/clk-gate2.c
>> @@ -67,7 +67,7 @@ static void clk_gate2_disable(struct clk_hw *hw)
>>
>>       spin_lock_irqsave(gate->lock, flags);
>>
>> -     if (gate->share_count && --(*gate->share_count) > 0)
>> +     if (gate->share_count && (*gate->share_count)-- > 0)
>
> The change makes no sense.  Let's say that clk_gate2_disable() gets
> called with share_count being 1.  In this case, we should access
> register to gate the clock, right?

If share_count is 1 it means that someone else is using the clock and
we can't disable it.

Please try running the series without this patch. When the extern
audio clock is enabled, share_count is 1. Later the the spdif clock
(the one that is shared with extern audio clock) is disabled by the
CCF as it is not used, which makes the extern audio clock to be also
disabled, which is not what we want.

What would you suggest?

[PATCH 4/5] ARM: imx: clk-gate2: Use post decrement for share_count

[Shawn Guo]

On Tue, Jul 01, 2014 at 02:44:40PM -0300, Fabio Estevam wrote:
> On Tue, Jul 1, 2014 at 8:52 AM, Shawn Guo <shawn.guo@freescale.com> wrote:
> 
> >> --- a/arch/arm/mach-imx/clk-gate2.c
> >> +++ b/arch/arm/mach-imx/clk-gate2.c
> >> @@ -67,7 +67,7 @@ static void clk_gate2_disable(struct clk_hw *hw)
> >>
> >>       spin_lock_irqsave(gate->lock, flags);
> >>
> >> -     if (gate->share_count && --(*gate->share_count) > 0)
> >> +     if (gate->share_count && (*gate->share_count)-- > 0)
> >
> > The change makes no sense.  Let's say that clk_gate2_disable() gets
> > called with share_count being 1.  In this case, we should access
> > register to gate the clock, right?
> 
> If share_count is 1 it means that someone else is using the clock and
> we can't disable it.

You do not really know it's someone else or itself.

> 
> Please try running the series without this patch. When the extern
> audio clock is enabled, share_count is 1. Later the the spdif clock
> (the one that is shared with extern audio clock) is disabled by the
> CCF as it is not used, which makes the extern audio clock to be also
> disabled, which is not what we want.
> 
> What would you suggest?

What about the patch below?

Shawn

----8<-------------

>From 0ed4b3edc661c63f86c914ea3c6deb3af3438151 Mon Sep 17 00:00:00 2001
From: Shawn Guo <shawn.guo@freescale.com>
Date: Wed, 2 Jul 2014 11:32:06 +0800
Subject: [PATCH] ARM: imx: fix shared gate clock to have its own enable count

Let's say clock A and B are two gate clocks that share the same register
bit in hardware.  Therefore they should be registered as shared gate
clocks with imx_clk_gate2_shared().

In the current implementation, clk_enable(A) call will have share_count
become 1.  If clk_disable(B) is called right after that, the register
bit will be cleared to gate off the clocks.  This is unexpected.  The
cause for that is there is no enable count tracking for clock A and B
respectively.

The patch fixes the issue by adding enable_count into clk_gate2, and
tracks it prior to share_count in .enable and .disable.  Also,
.is_enabled is fixed to report enable state instead of hardware state
in case of shared gate clock.

Reported-by: Fabio Estevam <fabio.estevam@freescale.com>
Cc: <stable@vger.kernel.org>
Fixes: f9f28cdf2167 ("ARM: imx: add shared gate clock support")
Signed-off-by: Shawn Guo <shawn.guo@freescale.com>
---
 arch/arm/mach-imx/clk-gate2.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/arch/arm/mach-imx/clk-gate2.c b/arch/arm/mach-imx/clk-gate2.c
index 4ba587da89d2..c5dca7cdbbb1 100644
--- a/arch/arm/mach-imx/clk-gate2.c
+++ b/arch/arm/mach-imx/clk-gate2.c
@@ -33,6 +33,7 @@ struct clk_gate2 {
 	u8		bit_idx;
 	u8		flags;
 	spinlock_t	*lock;
+	unsigned int	enable_count;
 	unsigned int	*share_count;
 };
 
@@ -46,6 +47,9 @@ static int clk_gate2_enable(struct clk_hw *hw)
 
 	spin_lock_irqsave(gate->lock, flags);
 
+	if (gate->enable_count++ > 0)
+		goto out;
+
 	if (gate->share_count && (*gate->share_count)++ > 0)
 		goto out;
 
@@ -67,6 +71,9 @@ static void clk_gate2_disable(struct clk_hw *hw)
 
 	spin_lock_irqsave(gate->lock, flags);
 
+	if (--gate->enable_count > 0)
+		goto out;
+
 	if (gate->share_count && --(*gate->share_count) > 0)
 		goto out;
 
@@ -83,6 +90,13 @@ static int clk_gate2_is_enabled(struct clk_hw *hw)
 	u32 reg;
 	struct clk_gate2 *gate = to_clk_gate2(hw);
 
+	/*
+	 * In case this is a shared gate, we cannot just report the hardware
+	 * state but its own enable state.
+	 */
+	if (gate->share_count)
+		return !!gate->enable_count;
+
 	reg = readl(gate->reg);
 
 	if (((reg >> gate->bit_idx) & 1) == 1)
-- 
1.8.3.2

[PATCH 4/5] ARM: imx: clk-gate2: Use post decrement for share_count

[Fabio Estevam]

On Wed, Jul 2, 2014 at 1:35 AM, Shawn Guo <shawn.guo@freescale.com> wrote:
> What about the patch below?

It does allow audio to play, but it resulted in CCGR5 as 0xffffffff
(all clocks enabled), so we still need to adjust it.

> Shawn
>
> ----8<-------------
>
> From 0ed4b3edc661c63f86c914ea3c6deb3af3438151 Mon Sep 17 00:00:00 2001
> From: Shawn Guo <shawn.guo@freescale.com>
> Date: Wed, 2 Jul 2014 11:32:06 +0800
> Subject: [PATCH] ARM: imx: fix shared gate clock to have its own enable count
>
> Let's say clock A and B are two gate clocks that share the same register
> bit in hardware.  Therefore they should be registered as shared gate
> clocks with imx_clk_gate2_shared().
>
> In the current implementation, clk_enable(A) call will have share_count
> become 1.  If clk_disable(B) is called right after that, the register
> bit will be cleared to gate off the clocks.  This is unexpected.  The
> cause for that is there is no enable count tracking for clock A and B
> respectively.
>
> The patch fixes the issue by adding enable_count into clk_gate2, and
> tracks it prior to share_count in .enable and .disable.  Also,
> .is_enabled is fixed to report enable state instead of hardware state
> in case of shared gate clock.
>
> Reported-by: Fabio Estevam <fabio.estevam@freescale.com>
> Cc: <stable@vger.kernel.org>
> Fixes: f9f28cdf2167 ("ARM: imx: add shared gate clock support")

No need to Cc stable on this one as the this commit did not reach stable.

> Signed-off-by: Shawn Guo <shawn.guo@freescale.com>
> ---
>  arch/arm/mach-imx/clk-gate2.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/arch/arm/mach-imx/clk-gate2.c b/arch/arm/mach-imx/clk-gate2.c
> index 4ba587da89d2..c5dca7cdbbb1 100644
> --- a/arch/arm/mach-imx/clk-gate2.c
> +++ b/arch/arm/mach-imx/clk-gate2.c
> @@ -33,6 +33,7 @@ struct clk_gate2 {
>         u8              bit_idx;
>         u8              flags;
>         spinlock_t      *lock;
> +       unsigned int    enable_count;
>         unsigned int    *share_count;
>  };
>
> @@ -46,6 +47,9 @@ static int clk_gate2_enable(struct clk_hw *hw)
>
>         spin_lock_irqsave(gate->lock, flags);
>
> +       if (gate->enable_count++ > 0)
> +               goto out;
> +
>         if (gate->share_count && (*gate->share_count)++ > 0)
>                 goto out;
>
> @@ -67,6 +71,9 @@ static void clk_gate2_disable(struct clk_hw *hw)
>
>         spin_lock_irqsave(gate->lock, flags);
>
> +       if (--gate->enable_count > 0)
> +               goto out;

All these pre-decrement look buggy because enable_count and
share_count are 'unsigned int'.

If share_count is 0 and then you decrement it, it will still be
greater than zero.
--(*gate->share_count) > 0 and --gate->enable_count > 0 are always true.

I have tried to make share_count and enable_count as 'int'. Then it
resulted CGR5 as
0FFFCFFF, which still leaves ssi1 and ssi3 enabled (I have locally
made ssi a shared clock now)

I will post a v2 without touching arch/arm/mach-imx/clk-gate2.c to
make it easier for us to debug it.

Thanks,

Fabio Estevam

[PATCH 4/5] ARM: imx: clk-gate2: Use post decrement for share_count

[Mike Turquette]

Quoting Fabio Estevam (2014-07-01 10:44:40)
> On Tue, Jul 1, 2014 at 8:52 AM, Shawn Guo <shawn.guo@freescale.com> wrote:
> 
> >> --- a/arch/arm/mach-imx/clk-gate2.c
> >> +++ b/arch/arm/mach-imx/clk-gate2.c
> >> @@ -67,7 +67,7 @@ static void clk_gate2_disable(struct clk_hw *hw)
> >>
> >>       spin_lock_irqsave(gate->lock, flags);
> >>
> >> -     if (gate->share_count && --(*gate->share_count) > 0)
> >> +     if (gate->share_count && (*gate->share_count)-- > 0)
> >
> > The change makes no sense.  Let's say that clk_gate2_disable() gets
> > called with share_count being 1.  In this case, we should access
> > register to gate the clock, right?
> 
> If share_count is 1 it means that someone else is using the clock and
> we can't disable it.

Why do you keep track of share_count at all? Is the enable_count
bookkeeping within the clock framework insufficient for your needs?

Regards,
Mike

> 
> Please try running the series without this patch. When the extern
> audio clock is enabled, share_count is 1. Later the the spdif clock
> (the one that is shared with extern audio clock) is disabled by the
> CCF as it is not used, which makes the extern audio clock to be also
> disabled, which is not what we want.
> 
> What would you suggest?
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 4/5] ARM: imx: clk-gate2: Use post decrement for share_count

[Fabio Estevam]

Hi Mike,

On Wed, Jul 2, 2014 at 12:29 PM, Mike Turquette <mturquette@linaro.org> wrote:

>> If share_count is 1 it means that someone else is using the clock and
>> we can't disable it.
>
> Why do you keep track of share_count at all? Is the enable_count
> bookkeeping within the clock framework insufficient for your needs?

What we are trying to handle here is the case when two clocks share
the same clock gating bit.

Let's say clocks clk1 and clk2 share the same clock gating bit.

What we want to achieve are:

Scenario 1:

clk2 is disabled
clk1 is enabled --> clock gate bit is set to 1
clk1 is disabled --> clock gate bit is set to 0. As clk2 is disabled,
it is OK to gate off the clock here

Scenario 2:

clk1 is enabled -> clock gate bit is set to 1
clk2 is disabled -> clock gate bit cannot be set to 0 because clk1 is enabled

We currently have arch/arm/mach-imx/clk-gate2.c and we are trying to
fix it because the scenario 2 is not working.

I am wondering if we should just extend drivers/clk/clk-gate.c to
handle shared clock instead of doing this in arch/arm/mach-imx?

What do you think?

Thanks,

Fabio Estevam

[PATCH 4/5] ARM: imx: clk-gate2: Use post decrement for share_count

[Mike Turquette]

Quoting Fabio Estevam (2014-07-02 09:52:56)
> Hi Mike,
> 
> On Wed, Jul 2, 2014 at 12:29 PM, Mike Turquette <mturquette@linaro.org> wrote:
> 
> >> If share_count is 1 it means that someone else is using the clock and
> >> we can't disable it.
> >
> > Why do you keep track of share_count at all? Is the enable_count
> > bookkeeping within the clock framework insufficient for your needs?
> 
> What we are trying to handle here is the case when two clocks share
> the same clock gating bit.
> 
> Let's say clocks clk1 and clk2 share the same clock gating bit.
> 
> What we want to achieve are:
> 
> Scenario 1:
> 
> clk2 is disabled
> clk1 is enabled --> clock gate bit is set to 1
> clk1 is disabled --> clock gate bit is set to 0. As clk2 is disabled,
> it is OK to gate off the clock here
> 
> Scenario 2:
> 
> clk1 is enabled -> clock gate bit is set to 1
> clk2 is disabled -> clock gate bit cannot be set to 0 because clk1 is enabled
> 
> We currently have arch/arm/mach-imx/clk-gate2.c and we are trying to
> fix it because the scenario 2 is not working.
> 
> I am wondering if we should just extend drivers/clk/clk-gate.c to
> handle shared clock instead of doing this in arch/arm/mach-imx?
> 
> What do you think?

I am actually looking into this right now, so it is good timing for me
to come across this thread. While hacking on the coordinated clock rates
feature I started to think about coordinating clock enables for exactly
your case: a single clock control enables or disables multiple clock
outputs.

I think some core framework changes are needed to support this sensibly,
and only putting the changes into clk-gate.c might not be sufficient or
elegant. I've added this task to my stack and will keep you Cc'd when
something hits the list.

Regards,
Mike

> 
> Thanks,
> 
> Fabio Estevam

[PATCH 4/5] ARM: imx: clk-gate2: Use post decrement for share_count

[Shawn Guo]

On Wed, Jul 02, 2014 at 11:27:21AM -0300, Fabio Estevam wrote:
> > From 0ed4b3edc661c63f86c914ea3c6deb3af3438151 Mon Sep 17 00:00:00 2001
> > From: Shawn Guo <shawn.guo@freescale.com>
> > Date: Wed, 2 Jul 2014 11:32:06 +0800
> > Subject: [PATCH] ARM: imx: fix shared gate clock to have its own enable count
> >
> > Let's say clock A and B are two gate clocks that share the same register
> > bit in hardware.  Therefore they should be registered as shared gate
> > clocks with imx_clk_gate2_shared().
> >
> > In the current implementation, clk_enable(A) call will have share_count
> > become 1.  If clk_disable(B) is called right after that, the register
> > bit will be cleared to gate off the clocks.  This is unexpected.  The
> > cause for that is there is no enable count tracking for clock A and B
> > respectively.
> >
> > The patch fixes the issue by adding enable_count into clk_gate2, and
> > tracks it prior to share_count in .enable and .disable.  Also,
> > .is_enabled is fixed to report enable state instead of hardware state
> > in case of shared gate clock.
> >
> > Reported-by: Fabio Estevam <fabio.estevam@freescale.com>
> > Cc: <stable@vger.kernel.org>
> > Fixes: f9f28cdf2167 ("ARM: imx: add shared gate clock support")
> 
> No need to Cc stable on this one as the this commit did not reach stable.

Yes, you're right.

...

> > @@ -67,6 +71,9 @@ static void clk_gate2_disable(struct clk_hw *hw)
> >
> >         spin_lock_irqsave(gate->lock, flags);
> >
> > +       if (--gate->enable_count > 0)
> > +               goto out;
> 
> All these pre-decrement look buggy because enable_count and
> share_count are 'unsigned int'.
> 
> If share_count is 0 and then you decrement it, it will still be
> greater than zero.
> --(*gate->share_count) > 0 and --gate->enable_count > 0 are always true.

Hmm, clk_gate2_disable() should never be called with a zero share_count.
I will add a check for that.

> I have tried to make share_count and enable_count as 'int'. Then it
> resulted CGR5 as
> 0FFFCFFF, which still leaves ssi1 and ssi3 enabled (I have locally
> made ssi a shared clock now)

Right.  The patch did not fix the problem correctly.  I just started it
over again with the one below.  Can you please test it?  Thanks.

Shawn

---8<-------------------

>From e057f4c129e77639372f2b4a3b9eb8a9de2095f8 Mon Sep 17 00:00:00 2001
From: Shawn Guo <shawn.guo@freescale.com>
Date: Wed, 2 Jul 2014 11:32:06 +0800
Subject: [PATCH] ARM: imx: fix shared gate clock

to be added ...

Reported-by: Fabio Estevam <fabio.estevam@freescale.com>
Fixes: f9f28cdf2167 ("ARM: imx: add shared gate clock support")
Signed-off-by: Shawn Guo <shawn.guo@freescale.com>
---
 arch/arm/mach-imx/clk-gate2.c | 30 ++++++++++++++++++++++--------
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/arch/arm/mach-imx/clk-gate2.c b/arch/arm/mach-imx/clk-gate2.c
index 4ba587da89d2..3aa9c74d13be 100644
--- a/arch/arm/mach-imx/clk-gate2.c
+++ b/arch/arm/mach-imx/clk-gate2.c
@@ -67,8 +67,12 @@ static void clk_gate2_disable(struct clk_hw *hw)
 
 	spin_lock_irqsave(gate->lock, flags);
 
-	if (gate->share_count && --(*gate->share_count) > 0)
-		goto out;
+	if (gate->share_count) {
+		if (WARN_ON(*gate->share_count == 0))
+			goto out;
+		else if (--(*gate->share_count) > 0)
+			goto out;
+	}
 
 	reg = readl(gate->reg);
 	reg &= ~(3 << gate->bit_idx);
@@ -78,19 +82,26 @@ out:
 	spin_unlock_irqrestore(gate->lock, flags);
 }
 
-static int clk_gate2_is_enabled(struct clk_hw *hw)
+static int clk_gate2_reg_is_enabled(void __iomem *reg, u8 bit_idx)
 {
-	u32 reg;
-	struct clk_gate2 *gate = to_clk_gate2(hw);
+	u32 val = readl(reg);
 
-	reg = readl(gate->reg);
-
-	if (((reg >> gate->bit_idx) & 1) == 1)
+	if (((val >> bit_idx) & 1) == 1)
 		return 1;
 
 	return 0;
 }
 
+static int clk_gate2_is_enabled(struct clk_hw *hw)
+{
+	struct clk_gate2 *gate = to_clk_gate2(hw);
+
+	if (gate->share_count)
+		return !!(*gate->share_count);
+	else
+		return clk_gate2_reg_is_enabled(gate->reg, gate->bit_idx);
+}
+
 static struct clk_ops clk_gate2_ops = {
 	.enable = clk_gate2_enable,
 	.disable = clk_gate2_disable,
@@ -116,6 +127,9 @@ struct clk *clk_register_gate2(struct device *dev, const char *name,
 	gate->bit_idx = bit_idx;
 	gate->flags = clk_gate2_flags;
 	gate->lock = lock;
+
+	if (share_count)
+		*share_count = clk_gate2_reg_is_enabled(reg, bit_idx) ? 1 : 0;
 	gate->share_count = share_count;
 
 	init.name = name;
-- 
1.8.3.2

[PATCH 4/5] ARM: imx: clk-gate2: Use post decrement for share_count

[Shawn Guo]

On Wed, Jul 02, 2014 at 01:52:56PM -0300, Fabio Estevam wrote:
> Hi Mike,
> 
> On Wed, Jul 2, 2014 at 12:29 PM, Mike Turquette <mturquette@linaro.org> wrote:
> 
> >> If share_count is 1 it means that someone else is using the clock and
> >> we can't disable it.
> >
> > Why do you keep track of share_count at all? Is the enable_count
> > bookkeeping within the clock framework insufficient for your needs?
> 
> What we are trying to handle here is the case when two clocks share
> the same clock gating bit.
> 
> Let's say clocks clk1 and clk2 share the same clock gating bit.
> 
> What we want to achieve are:
> 
> Scenario 1:
> 
> clk2 is disabled
> clk1 is enabled --> clock gate bit is set to 1
> clk1 is disabled --> clock gate bit is set to 0. As clk2 is disabled,
> it is OK to gate off the clock here
> 
> Scenario 2:
> 
> clk1 is enabled -> clock gate bit is set to 1
> clk2 is disabled -> clock gate bit cannot be set to 0 because clk1 is enabled

This never happens.  If clk2 is disabled by clk_disable() without being
enabled by clk_enable() beforehand, it returns from __clk_disable()
immediately due to hitting of the WARN below.

	if (WARN_ON(clk->enable_count == 0))
		return;

Shawn

> 
> We currently have arch/arm/mach-imx/clk-gate2.c and we are trying to
> fix it because the scenario 2 is not working.
> 
> I am wondering if we should just extend drivers/clk/clk-gate.c to
> handle shared clock instead of doing this in arch/arm/mach-imx?
> 
> What do you think?
> 
> Thanks,
> 
> Fabio Estevam

[PATCH 4/5] ARM: imx: clk-gate2: Use post decrement for share_count

[Shawn Guo]

On Thu, Jul 03, 2014 at 03:46:00PM +0800, Shawn Guo wrote:
> > Scenario 2:
> > 
> > clk1 is enabled -> clock gate bit is set to 1
> > clk2 is disabled -> clock gate bit cannot be set to 0 because clk1 is enabled
> 
> This never happens.  If clk2 is disabled by clk_disable() without being
> enabled by clk_enable() beforehand, it returns from __clk_disable()
> immediately due to hitting of the WARN below.
> 
> 	if (WARN_ON(clk->enable_count == 0))
> 		return;

Well, it only happens in case that clock core calls clk->ops->disable()
directly from clk_disable_unused_subtree() in order to disable unused
clocks.

Shawn