ARM: dts: imx6sx: Fix sdma node

From: Fabio Estevam <[email protected]>

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 <[email protected]>

---
 arch/arm/boot/dts/imx6sx.dtsi | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

arch/arm/boot/dts/imx6sx.dtsi +0 -0

index 2929078..ef3cea2 100644

arch/arm/boot/dts/imx6sx.dtsi +5 -6

 					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";
 				};
 

Signature · 7 lines

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

Hi Fabio,

Quoted 4 lines · Fabio Estevam

From: Fabio Estevam <[email protected]>

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.

Quoted 8 lines

Also pass the fsl,fifo-depth property.

Signed-off-by Fabio Estevam <[email protected]>

---
 arch/arm/boot/dts/imx6sx.dtsi | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

arch/arm/boot/dts/imx6sx.dtsi line 304

Quoted 12 lines

arch/arm/boot/dts/imx6sx.dtsi +1 -3

index 2929078..ef3cea2 100644
@@ -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.

From: Fabio Estevam <[email protected]>

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 <[email protected]>

---
 arch/arm/mach-imx/clk-imx6sx.c | 3 ---
 1 file changed, 3 deletions(-)

arch/arm/mach-imx/clk-imx6sx.c +0 -3

index 2e96103..1b198ea 100644
@@ -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);

Signature · 1 line

1.8.3.2

From: Fabio Estevam <[email protected]>

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 <[email protected]>

---
 arch/arm/mach-imx/clk-gate2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/arm/mach-imx/clk-gate2.c +0 -0

index 4ba587d..463083c 100644

arch/arm/mach-imx/clk-gate2.c +0 -0

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

Signature · 1 line

1.8.3.2

Quoted 4 lines

arch/arm/mach-imx/clk-gate2.c +0 -0

@@ -67,7 +67,7 @@ static void clk_gate2_disable(struct clk_hw *hw)
 

arch/arm/mach-imx/clk-gate2.c static void clk_gate2_disable(struct clk_hw *hw) · line 70

Quoted 4 lines

arch/arm/mach-imx/clk-gate2.c +1 -1

      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?

Quoted 4 lines · Shawn Guo

Quoted 4 lines

arch/arm/mach-imx/clk-gate2.c +0 -0

@@ -67,7 +67,7 @@ static void clk_gate2_disable(struct clk_hw *hw)
 

Quoted 8 lines

arch/arm/mach-imx/clk-gate2.c static void clk_gate2_disable(struct clk_hw *hw) · line 70

Quoted 4 lines

arch/arm/mach-imx/clk-gate2.c +1 -1

      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?

From 0ed4b3edc661c63f86c914ea3c6deb3af3438151 Mon Sep 17 00:00:00 2001

What about the patch below?

Shawn

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

From 0ed4b3edc661c63f86c914ea3c6deb3af3438151 Mon Sep 17 00:00:00 2001
From: Shawn Guo <[email protected]>
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 <[email protected]>

Cc <[email protected]>

Fixes f9f28cdf2167 ("ARM: imx: add shared gate clock support")

Signed-off-by Shawn Guo <[email protected]>

---
 arch/arm/mach-imx/clk-gate2.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

arch/arm/mach-imx/clk-gate2.c +4 -0

index 4ba587da89d2..c5dca7cdbbb1 100644
@@ -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;
 

Quoted 20 lines

From 0ed4b3edc661c63f86c914ea3c6deb3af3438151 Mon Sep 17 00:00:00 2001
From: Shawn Guo <[email protected]>
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.

Quoted 3 lines

Reported-by Fabio Estevam <[email protected]>

Cc <[email protected]>

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.

Review static void clk_gate2_disable(struct clk_hw *hw) · line 75

Quoted 6 lines

(quoted diff) +2 -0

@@ -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)

From e057f4c129e77639372f2b4a3b9eb8a9de2095f8 Mon Sep 17 00:00:00 2001

Quoted 5 lines

Quoted 5 lines

arch/arm/mach-imx/clk-gate2.c +0 -0

@@ -67,7 +67,7 @@ static void clk_gate2_disable(struct clk_hw *hw)
 

Quoted 8 lines

arch/arm/mach-imx/clk-gate2.c static void clk_gate2_disable(struct clk_hw *hw) · line 70

Quoted 4 lines

arch/arm/mach-imx/clk-gate2.c +1 -1

      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?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 2 lines

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?

Hi Mike,

Quoted 5 lines · Mike Turquette

Quoted 2 lines

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

Hi Mike,

Quoted 5 lines · Mike Turquette

Quoted 2 lines

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

Quoted 4 lines

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;