[PATCH] clk: imx25: set correct parents for ssi ipg clocks

[PATCH] clk: imx25: set correct parents for ssi ipg clocks

[Martin Kaiser]

The ssi1_ipg clock depends on ssi1_ipg_per. Set ssi1_ipg_per as parent
clock of ssi1_ipg. Without this link, the fsl_ssi driver does not
activate the ssi1_ipg clock correctly and ssi1 is unable to transfer
audio data.

Fix the parent clock of ssi2 as well, it shows the same behaviour.

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
Dear all,

could you have a look at this patch? This makes SSI1 and 2 work for me.
With the default "ipg" parent clocks, I get no ipg clocks on the SSIs.
However, I was wondering why we don't need anything similar e.g. for the
UARTs...

Best regards,
Martin

 drivers/clk/imx/clk-imx25.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/imx/clk-imx25.c b/drivers/clk/imx/clk-imx25.c
index 23686f7..ee2852f 100644
--- a/drivers/clk/imx/clk-imx25.c
+++ b/drivers/clk/imx/clk-imx25.c
@@ -217,8 +217,8 @@ static int __init __mx25_clocks_init(void __iomem *ccm_base)
 	clk[sim2_ipg] = imx_clk_gate("sim2_ipg", "ipg", ccm(CCM_CGCR2),  8);
 	clk[slcdc_ipg] = imx_clk_gate("slcdc_ipg", "ipg", ccm(CCM_CGCR2),  9);
 	clk[spba_ipg] = imx_clk_gate("spba_ipg", "ipg", ccm(CCM_CGCR2),  10);
-	clk[ssi1_ipg] = imx_clk_gate("ssi1_ipg", "ipg", ccm(CCM_CGCR2), 11);
-	clk[ssi2_ipg] = imx_clk_gate("ssi2_ipg", "ipg", ccm(CCM_CGCR2), 12);
+	clk[ssi1_ipg] = imx_clk_gate("ssi1_ipg", "ssi1_ipg_per", ccm(CCM_CGCR2), 11);
+	clk[ssi2_ipg] = imx_clk_gate("ssi2_ipg", "ssi2_ipg_per", ccm(CCM_CGCR2), 12);
 	clk[tsc_ipg] = imx_clk_gate("tsc_ipg", "ipg", ccm(CCM_CGCR2), 13);
 	clk[uart1_ipg] = imx_clk_gate("uart1_ipg", "ipg", ccm(CCM_CGCR2), 14);
 	clk[uart2_ipg] = imx_clk_gate("uart2_ipg", "ipg", ccm(CCM_CGCR2), 15);
-- 
2.1.4

[PATCH] clk: imx25: set correct parents for ssi ipg clocks

[Fabio Estevam]

Hi Martin,

On Tue, Mar 6, 2018 at 7:02 PM, Martin Kaiser <martin@kaiser.cx> wrote:
> The ssi1_ipg clock depends on ssi1_ipg_per. Set ssi1_ipg_per as parent
> clock of ssi1_ipg. Without this link, the fsl_ssi driver does not
> activate the ssi1_ipg clock correctly and ssi1 is unable to transfer
> audio data.
>
> Fix the parent clock of ssi2 as well, it shows the same behaviour.
>
> Signed-off-by: Martin Kaiser <martin@kaiser.cx>
> ---
> Dear all,
>
> could you have a look at this patch? This makes SSI1 and 2 work for me.

I get audio working from SSI1, but I guess this is due to the fact
that the bootloader enables the SSI clock:

I have the following in U-Boot:

/* Enable the clocks */
DATA 4 0x53f8000c 0x1fffffff
DATA 4 0x53f80010 0xffffffff
DATA 4 0x53f80014 0xfdfff

I will try tomorrow to disable the SSI1 clock from U-Boot and apply your patch.

Will let you know how my testing goes.

> With the default "ipg" parent clocks, I get no ipg clocks on the SSIs.
> However, I was wondering why we don't need anything similar e.g. for the
> UARTs...

Maybe your bootloader is already turning on the UART clocks?

[PATCH] clk: imx25: set correct parents for ssi ipg clocks

[Martin Kaiser]

Hi Fabio,

Thus wrote Fabio Estevam (festevam at gmail.com):

> I get audio working from SSI1, but I guess this is due to the fact
> that the bootloader enables the SSI clock:

> I have the following in U-Boot:

> /* Enable the clocks */
> DATA 4 0x53f8000c 0x1fffffff
> DATA 4 0x53f80010 0xffffffff
> DATA 4 0x53f80014 0xfdfff

I'm using the same initial settings.

Nevertheless, ssi1_ipg_per is disbled after loading the kernel.

Digging into this a bit more, it turned out that without my patch,
clk_disable_unused() recognizes ssi1_ipg_per as unused and disables it.

If my patch is applied and ssi1_ipg_per is declared as parent of
ssi1_ipg, clk_disable_unused() will not disable it and fsl_ssi_startup()
will enable both ssi1_ipg_per and ssi1_ipg before playing sound.

Best regards,
Martin

[PATCH] clk: imx25: set correct parents for ssi ipg clocks

[Fabio Estevam]

Hi Martin,

On Thu, Mar 8, 2018 at 11:08 AM, Martin Kaiser <martin@kaiser.cx> wrote:
> Hi Fabio,
>
> Thus wrote Fabio Estevam (festevam at gmail.com):
>
>> I get audio working from SSI1, but I guess this is due to the fact
>> that the bootloader enables the SSI clock:
>
>> I have the following in U-Boot:
>
>> /* Enable the clocks */
>> DATA 4 0x53f8000c 0x1fffffff
>> DATA 4 0x53f80010 0xffffffff
>> DATA 4 0x53f80014 0xfdfff
>
> I'm using the same initial settings.
>
> Nevertheless, ssi1_ipg_per is disbled after loading the kernel.
>
> Digging into this a bit more, it turned out that without my patch,
> clk_disable_unused() recognizes ssi1_ipg_per as unused and disables it.
>
> If my patch is applied and ssi1_ipg_per is declared as parent of
> ssi1_ipg, clk_disable_unused() will not disable it and fsl_ssi_startup()
> will enable both ssi1_ipg_per and ssi1_ipg before playing sound.

I can get audio to work fine without your patch on a mx25pdk.

In the other i.MX clock drivers we have this same pattern:

clks[IMX6SL_CLK_SSI1_IPG]     = imx_clk_gate2_shared("ssi1_ipg",     "ipg",

It is not clear to me what is the real issue this patch is trying to fix.

Thanks

[PATCH] clk: imx25: set correct parents for ssi ipg clocks

[Martin Kaiser]

Hi Fabio,

thanks for the quick response.

Thus wrote Fabio Estevam (festevam at gmail.com):

> Hi Martin,

> On Thu, Mar 8, 2018 at 11:08 AM, Martin Kaiser <martin@kaiser.cx> wrote:
> > Hi Fabio,

> > Thus wrote Fabio Estevam (festevam at gmail.com):

> >> I get audio working from SSI1, but I guess this is due to the fact
> >> that the bootloader enables the SSI clock:

> >> I have the following in U-Boot:

> >> /* Enable the clocks */
> >> DATA 4 0x53f8000c 0x1fffffff
> >> DATA 4 0x53f80010 0xffffffff
> >> DATA 4 0x53f80014 0xfdfff

> > I'm using the same initial settings.

> > Nevertheless, ssi1_ipg_per is disbled after loading the kernel.

> > Digging into this a bit more, it turned out that without my patch,
> > clk_disable_unused() recognizes ssi1_ipg_per as unused and disables it.

> > If my patch is applied and ssi1_ipg_per is declared as parent of
> > ssi1_ipg, clk_disable_unused() will not disable it and fsl_ssi_startup()
> > will enable both ssi1_ipg_per and ssi1_ipg before playing sound.

> I can get audio to work fine without your patch on a mx25pdk.

this is surprising. How come the ssi1_ipg_per clock is not turned off by
clk_disable_unused()? Where is it used? Do you have

<&clks 55>

anywhere in your DT?

> In the other i.MX clock drivers we have this same pattern:

> clks[IMX6SL_CLK_SSI1_IPG]     = imx_clk_gate2_shared("ssi1_ipg",     "ipg",

It seems to the that imx25 uses a different clock hierarchy for ssi than other
imx chips.

Documentation/devicetree/bindings/clock/imx25-clock.txt: ssi1_ipg_per      55
Documentation/devicetree/bindings/clock/imx25-clock.txt: ssi2_ipg_per      56
Documentation/devicetree/bindings/clock/imx25-clock.txt: ssi1_ipg    117
Documentation/devicetree/bindings/clock/imx25-clock.txt: ssi2_ipg    118
Documentation/devicetree/bindings/clock/imx31-clock.txt: ssi1_gate            32
Documentation/devicetree/bindings/clock/imx31-clock.txt: ssi2_gate            52
Documentation/devicetree/bindings/clock/imx35-clock.txt: ssi_sel        22
Documentation/devicetree/bindings/clock/imx35-clock.txt: ssi1_div_pre      23
Documentation/devicetree/bindings/clock/imx35-clock.txt: ssi1_div_post     24
Documentation/devicetree/bindings/clock/imx35-clock.txt: ssi2_div_pre      25
Documentation/devicetree/bindings/clock/imx35-clock.txt: ssi2_div_post     26
Documentation/devicetree/bindings/clock/imx35-clock.txt: ssi1_gate      68
Documentation/devicetree/bindings/clock/imx35-clock.txt: ssi2_gate      69

Others don't have ssiX_ipg_per.

> It is not clear to me what is the real issue this patch is trying to fix.

True. This needs clarification.

I found that, in oder to get a tx clock out of the SSI, both ssi1_ipg_per and
ssi1_ipg clocks must be active.

The fsl_ssi driver only activates ssi1_ipg.

If I activate ssi1_ipg_per in the bootloader, clk_disable_unused() deactivates it.

(My codec chip does not use a dedicated clock line. It takes the bit clock that
is the output of SSI. Are you maybe using ssi1_ipg_per for your codec and
enable it there?)

In my first mail, I was wondering about imx25 uart1, where we also have
uart1_ipg and uart_ipg_per and the clock seeting is

   clk[uart1_ipg] = imx_clk_gate("uart1_ipg", "ipg", ccm(CCM_CGCR2), 14);

In this case, both uart1 and uart_ipg_per are listed in the device tree

         uart1: serial at 43f90000 {                                                                                                      
...
            clocks = <&clks 120>, <&clks 57>;                                                                                          
            clock-names = "ipg", "per";                                                                                                
         };                                                                                                                            

Documentation/devicetree/bindings/clock/imx25-clock.txt
   uart_ipg_per      57
   uart1_ipg      120

and the driver enables both clocks explicitly. So they are not unused.


Doing something like this is not an option for ssi, this will not work with
imx31, 35 etc.

Therefore, I suggest setting ssi1_ipg_per as parent of ssi1_ipg.


Best regards,

   Martin

[PATCH] clk: imx25: set correct parents for ssi ipg clocks

[Lothar Waßmann]

Hi,

On Thu, 8 Mar 2018 17:46:54 +0100 Martin Kaiser wrote:
> Hi Fabio,
> 
> thanks for the quick response.
> 
> Thus wrote Fabio Estevam (festevam at gmail.com):
> 
> > Hi Martin,
> 
> > On Thu, Mar 8, 2018 at 11:08 AM, Martin Kaiser <martin@kaiser.cx> wrote:
> > > Hi Fabio,
> 
> > > Thus wrote Fabio Estevam (festevam at gmail.com):
> 
> > >> I get audio working from SSI1, but I guess this is due to the fact
> > >> that the bootloader enables the SSI clock:
> 
> > >> I have the following in U-Boot:
> 
> > >> /* Enable the clocks */
> > >> DATA 4 0x53f8000c 0x1fffffff
> > >> DATA 4 0x53f80010 0xffffffff
> > >> DATA 4 0x53f80014 0xfdfff
> 
> > > I'm using the same initial settings.
> 
> > > Nevertheless, ssi1_ipg_per is disbled after loading the kernel.
> 
> > > Digging into this a bit more, it turned out that without my patch,
> > > clk_disable_unused() recognizes ssi1_ipg_per as unused and disables it.
> 
> > > If my patch is applied and ssi1_ipg_per is declared as parent of
> > > ssi1_ipg, clk_disable_unused() will not disable it and fsl_ssi_startup()
> > > will enable both ssi1_ipg_per and ssi1_ipg before playing sound.
> 
> > I can get audio to work fine without your patch on a mx25pdk.
> 
> this is surprising. How come the ssi1_ipg_per clock is not turned off by
> clk_disable_unused()? Where is it used? Do you have
> 
> <&clks 55>
> 
> anywhere in your DT?
> 
> > In the other i.MX clock drivers we have this same pattern:
> 
> > clks[IMX6SL_CLK_SSI1_IPG]     = imx_clk_gate2_shared("ssi1_ipg",     "ipg",
> 
> It seems to the that imx25 uses a different clock hierarchy for ssi than other
> imx chips.
> 
> Documentation/devicetree/bindings/clock/imx25-clock.txt: ssi1_ipg_per      55
> Documentation/devicetree/bindings/clock/imx25-clock.txt: ssi2_ipg_per      56
> Documentation/devicetree/bindings/clock/imx25-clock.txt: ssi1_ipg    117
> Documentation/devicetree/bindings/clock/imx25-clock.txt: ssi2_ipg    118
> Documentation/devicetree/bindings/clock/imx31-clock.txt: ssi1_gate            32
> Documentation/devicetree/bindings/clock/imx31-clock.txt: ssi2_gate            52
> Documentation/devicetree/bindings/clock/imx35-clock.txt: ssi_sel        22
> Documentation/devicetree/bindings/clock/imx35-clock.txt: ssi1_div_pre      23
> Documentation/devicetree/bindings/clock/imx35-clock.txt: ssi1_div_post     24
> Documentation/devicetree/bindings/clock/imx35-clock.txt: ssi2_div_pre      25
> Documentation/devicetree/bindings/clock/imx35-clock.txt: ssi2_div_post     26
> Documentation/devicetree/bindings/clock/imx35-clock.txt: ssi1_gate      68
> Documentation/devicetree/bindings/clock/imx35-clock.txt: ssi2_gate      69
> 
> Others don't have ssiX_ipg_per.
> 
> > It is not clear to me what is the real issue this patch is trying to fix.
> 
> True. This needs clarification.
> 
> I found that, in oder to get a tx clock out of the SSI, both ssi1_ipg_per and
> ssi1_ipg clocks must be active.
> 
> The fsl_ssi driver only activates ssi1_ipg.
> 
> If I activate ssi1_ipg_per in the bootloader, clk_disable_unused() deactivates it.
> 
> (My codec chip does not use a dedicated clock line. It takes the bit clock that
> is the output of SSI. Are you maybe using ssi1_ipg_per for your codec and
> enable it there?)
> 
> In my first mail, I was wondering about imx25 uart1, where we also have
> uart1_ipg and uart_ipg_per and the clock seeting is
> 
>    clk[uart1_ipg] = imx_clk_gate("uart1_ipg", "ipg", ccm(CCM_CGCR2), 14);
> 
> In this case, both uart1 and uart_ipg_per are listed in the device tree
> 
>          uart1: serial at 43f90000 {                                                                                                      
> ...
>             clocks = <&clks 120>, <&clks 57>;                                                                                          
>             clock-names = "ipg", "per";                                                                                                
>          };                                                                                                                            
> 
> Documentation/devicetree/bindings/clock/imx25-clock.txt
>    uart_ipg_per      57
>    uart1_ipg      120
> 
> and the driver enables both clocks explicitly. So they are not unused.
> 
> 
> Doing something like this is not an option for ssi, this will not work with
> imx31, 35 etc.
> 
> Therefore, I suggest setting ssi1_ipg_per as parent of ssi1_ipg.
>
The right wayto fix this is to add the missing ipg_per clock to the DTB
rather than introducing a bogus clock relationship that doesn't exist
in hardware.
The sound/soc/fsl/fsl_ssi.c driver does already handle a second clock
as bitclock. It only needs to be specified in the DTB:
&ssi1 {
	clocks = <&clks 117>, <&clk 55>;
	clock-names = "ipg", "baud";
};


Lothar Wa?mann

[PATCH] clk: imx25: set correct parents for ssi ipg clocks

[Fabio Estevam]

Hi Martin,

On Thu, Mar 8, 2018 at 1:46 PM, Martin Kaiser <martin@kaiser.cx> wrote:

>> I can get audio to work fine without your patch on a mx25pdk.
>
> this is surprising. How come the ssi1_ipg_per clock is not turned off by
> clk_disable_unused()? Where is it used? Do you have
>
> <&clks 55>
>
> anywhere in your DT?

No, I don't. imx25-pdk board operates SSI in slave mode.

> (My codec chip does not use a dedicated clock line. It takes the bit clock that
> is the output of SSI. Are you maybe using ssi1_ipg_per for your codec and
> enable it there?)

The difference between our boards is that you use SSI in master mode
and mx25pdk in slave mode.

> In my first mail, I was wondering about imx25 uart1, where we also have
> uart1_ipg and uart_ipg_per and the clock seeting is
>
>    clk[uart1_ipg] = imx_clk_gate("uart1_ipg", "ipg", ccm(CCM_CGCR2), 14);
>
> In this case, both uart1 and uart_ipg_per are listed in the device tree
>
>          uart1: serial at 43f90000 {
> ...
>             clocks = <&clks 120>, <&clks 57>;
>             clock-names = "ipg", "per";
>          };
>
> Documentation/devicetree/bindings/clock/imx25-clock.txt
>    uart_ipg_per      57
>    uart1_ipg      120
>
> and the driver enables both clocks explicitly. So they are not unused.
>
>
> Doing something like this is not an option for ssi, this will not work with
> imx31, 35 etc.

The solution to this is passing the "baud" clock as Lothar pointed out.

[PATCH] clk: imx25: set correct parents for ssi ipg clocks

[Martin Kaiser]

Hi Fabio & Lothar,

Thus wrote Fabio Estevam (festevam at gmail.com):

> On Thu, Mar 8, 2018 at 1:46 PM, Martin Kaiser <martin@kaiser.cx> wrote:

> >> I can get audio to work fine without your patch on a mx25pdk.

> > this is surprising. How come the ssi1_ipg_per clock is not turned off by
> > clk_disable_unused()? Where is it used? Do you have

> > <&clks 55>

> > anywhere in your DT?

> No, I don't. imx25-pdk board operates SSI in slave mode.

> > (My codec chip does not use a dedicated clock line. It takes the bit clock that
> > is the output of SSI. Are you maybe using ssi1_ipg_per for your codec and
> > enable it there?)

> The difference between our boards is that you use SSI in master mode
> and mx25pdk in slave mode.

> > In my first mail, I was wondering about imx25 uart1, where we also have
> > uart1_ipg and uart_ipg_per and the clock seeting is

> >    clk[uart1_ipg] = imx_clk_gate("uart1_ipg", "ipg", ccm(CCM_CGCR2), 14);

> > In this case, both uart1 and uart_ipg_per are listed in the device tree

> >          uart1: serial at 43f90000 {
> > ...
> >             clocks = <&clks 120>, <&clks 57>;
> >             clock-names = "ipg", "per";
> >          };

> > Documentation/devicetree/bindings/clock/imx25-clock.txt
> >    uart_ipg_per      57
> >    uart1_ipg      120

> > and the driver enables both clocks explicitly. So they are not unused.


> > Doing something like this is not an option for ssi, this will not work with
> > imx31, 35 etc.

> The solution to this is passing the "baud" clock as Lothar pointed out.

ok, I got your point now.

The ssi1_ipg clock does not depend on ssi1_ipg_per. This is in line with
the clock generation scheme in the reference manual.

And there's configurations where ssi1_ipg should be switched on, but not
ssi1_ipg_per.

I'll look into using the baud clock on my board.

Thanks to both of you for taking the time to explain this.

Best regards,

   Martin