[RFC] usb issue on imx27: 3 clocks are needed

[RFC] usb issue on imx27: 3 clocks are needed

[Philippe Reynes]

Hi all,

i.MX27's usb needs three clocks (usb_ipg_gate, usb_ahb_gate and usb_div)
but the current chipidea driver implementation, and devicetree, provides
only ipg and ahb. Consequently, if the bootloader don't enable the last
one, the kernel will crash.

Our approach/idea is to add a second, optionnal, clock in ci_hdrc_imx.c
with 'per' name in devicetree  and to add  clock name 'main_clk' for mandatory clock.
                   
This approach it correct? Or an other approach seems better?
                            
Thank you very much for your point of view.

Regards,
Philippe and Gwenhael

[RFC] usb issue on imx27: 3 clocks are needed

[Fabio Estevam]

Hi Philippe,

On Sat, Aug 16, 2014 at 12:38 PM, Philippe Reynes <tremyfr@gmail.com> wrote:
> Hi all,
>
> i.MX27's usb needs three clocks (usb_ipg_gate, usb_ahb_gate and usb_div)
> but the current chipidea driver implementation, and devicetree, provides
> only ipg and ahb. Consequently, if the bootloader don't enable the last
> one, the kernel will crash.

Which kernel version and what is the crash log you are getting?

I used to get a USB crash on mx27, which was fixed with the following commit:

commit b67b19447eb4f60d4f004f48298154630d4bed39
Author: Fabio Estevam <fabio.estevam@freescale.com>
Date:   Wed Apr 16 14:53:18 2014 -0300

    ARM: dts: imx27: Use the correct usb clock gate

    USB Host1, Host2 and OTG are gated via 'usb_ipg_gate' clock, so
fix it in order
    to avoid the following kernel oops:

    usbcore: registered new interface driver usb-storage
    10024000.usb supply vbus not found, using dummy regulator
    Unhandled fault: external abort on non-linefetch (0x808) at 0xf4424184
    Internal error: : 808 [#1] PREEMPT ARM
    Modules linked in:
    CPU: 0 PID: 1 Comm: swapper Not tainted 3.15.0-rc1-26325-g971f9fd-dirty #64
    task: c7829aa0 ti: c7836000 task.ti: c7836000
    PC is at ci_hdrc_probe+0x3a4/0x634
    LR is at ci_hdrc_probe+0x100/0x634
    pc : [<c036cc78>]    lr : [<c036c9d4>]    psr: 60000013
    sp : c7837d48  ip : 00000001  fp : 00000000
    r10: 00000000  r9 : 00000000  r8 : c791b6c0
    r7 : c7945000  r6 : f4424000  r5 : c7945010  r4 : c794e010
    r3 : f4424184  r2 : 00000000  r1 : 8c000004  r0 : 0c000004
    Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment kernel
    Control: 0005317f  Table: a0004000  DAC: 00000017
    Process swapper (pid: 1, stack limit = 0xc78361c0)
    Stack: (0xc7837d48 to 0xc7838000)

    Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
    Signed-off-by: Shawn Guo <shawn.guo@freescale.com>

Do you have this one applied?

[RFC] usb issue on imx27: 3 clocks are needed

[Philippe Reynes]

Hi Fabio,


On 16/08/14 18:01, Fabio Estevam wrote:
> Hi Philippe,
>
> On Sat, Aug 16, 2014 at 12:38 PM, Philippe Reynes<tremyfr@gmail.com>  wrote:
>> Hi all,
>>
>> i.MX27's usb needs three clocks (usb_ipg_gate, usb_ahb_gate and usb_div)
>> but the current chipidea driver implementation, and devicetree, provides
>> only ipg and ahb. Consequently, if the bootloader don't enable the last
>> one, the kernel will crash.
>
> Which kernel version and what is the crash log you are getting?

I use linux git kernel (from linus) and 3.16.
Both has the same result, a crash in the function hw_phymode_configure.
  
> I used to get a USB crash on mx27, which was fixed with the following commit:
>
> commit b67b19447eb4f60d4f004f48298154630d4bed39
> Author: Fabio Estevam<fabio.estevam@freescale.com>
> Date:   Wed Apr 16 14:53:18 2014 -0300
>
>      ARM: dts: imx27: Use the correct usb clock gate
>
>      USB Host1, Host2 and OTG are gated via 'usb_ipg_gate' clock, so
> fix it in order
>      to avoid the following kernel oops:
>
>      usbcore: registered new interface driver usb-storage
>      10024000.usb supply vbus not found, using dummy regulator
>      Unhandled fault: external abort on non-linefetch (0x808) at 0xf4424184
>      Internal error: : 808 [#1] PREEMPT ARM
>      Modules linked in:
>      CPU: 0 PID: 1 Comm: swapper Not tainted 3.15.0-rc1-26325-g971f9fd-dirty #64
>      task: c7829aa0 ti: c7836000 task.ti: c7836000
>      PC is at ci_hdrc_probe+0x3a4/0x634
>      LR is at ci_hdrc_probe+0x100/0x634
>      pc : [<c036cc78>]    lr : [<c036c9d4>]    psr: 60000013
>      sp : c7837d48  ip : 00000001  fp : 00000000
>      r10: 00000000  r9 : 00000000  r8 : c791b6c0
>      r7 : c7945000  r6 : f4424000  r5 : c7945010  r4 : c794e010
>      r3 : f4424184  r2 : 00000000  r1 : 8c000004  r0 : 0c000004
>      Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment kernel
>      Control: 0005317f  Table: a0004000  DAC: 00000017
>      Process swapper (pid: 1, stack limit = 0xc78361c0)
>      Stack: (0xc7837d48 to 0xc7838000)
>
>      Signed-off-by: Fabio Estevam<fabio.estevam@freescale.com>
>      Signed-off-by: Shawn Guo<shawn.guo@freescale.com>
>
> Do you have this one applied?

Yes, I've got this commit on my kernel.

This patch enable clock ipg, but the clock usb_div is disable,
so the "crash" still happen. The only way we found to get the
usb working is to enable the three usb clock : ipg, ahb and per.

Best regards,
Philippe

[RFC] usb issue on imx27: 3 clocks are needed

[Peter Chen]

On Sat, Aug 16, 2014 at 05:38:30PM +0200, Philippe Reynes wrote:
> Hi all,
> 
> i.MX27's usb needs three clocks (usb_ipg_gate, usb_ahb_gate and usb_div)
> but the current chipidea driver implementation, and devicetree, provides
> only ipg and ahb. Consequently, if the bootloader don't enable the last
> one, the kernel will crash.
> 
> Our approach/idea is to add a second, optionnal, clock in ci_hdrc_imx.c
> with 'per' name in devicetree  and to add  clock name 'main_clk' for mandatory clock.
> This approach it correct? Or an other approach seems better?
> Thank you very much for your point of view.
> 

It is ok for me to have ipg, ahb and per clocks at driver, but how can you maintain
DT consistent? Can you accept open ipg and per always on at clock.c?

-- 
Best Regards,
Peter Chen

[RFC] usb issue on imx27: 3 clocks are needed

[Shawn Guo]

On Mon, Aug 18, 2014 at 05:00:59PM +0800, Peter Chen wrote:
> On Sat, Aug 16, 2014 at 05:38:30PM +0200, Philippe Reynes wrote:
> > Hi all,
> > 
> > i.MX27's usb needs three clocks (usb_ipg_gate, usb_ahb_gate and usb_div)
> > but the current chipidea driver implementation, and devicetree, provides
> > only ipg and ahb. Consequently, if the bootloader don't enable the last
> > one, the kernel will crash.
> > 
> > Our approach/idea is to add a second, optionnal, clock in ci_hdrc_imx.c
> > with 'per' name in devicetree  and to add  clock name 'main_clk' for mandatory clock.
> > This approach it correct? Or an other approach seems better?
> > Thank you very much for your point of view.
> > 
> 
> It is ok for me to have ipg, ahb and per clocks at driver, but how can you maintain
> DT consistent?

Adding new clock as optional one will just maintain the DT
compatibility.

> Can you accept open ipg and per always on at clock.c?

No, usb driver should manage its clocks.

Shawn

[RFC] usb issue on imx27: 3 clocks are needed

[Peter Chen]

 
> On Mon, Aug 18, 2014 at 05:00:59PM +0800, Peter Chen wrote:
> > On Sat, Aug 16, 2014 at 05:38:30PM +0200, Philippe Reynes wrote:
> > > Hi all,
> > >
> > > i.MX27's usb needs three clocks (usb_ipg_gate, usb_ahb_gate and
> > > usb_div) but the current chipidea driver implementation, and
> > > devicetree, provides only ipg and ahb. Consequently, if the
> > > bootloader don't enable the last one, the kernel will crash.
> > >
> > > Our approach/idea is to add a second, optionnal, clock in
> > > ci_hdrc_imx.c with 'per' name in devicetree  and to add  clock name
> 'main_clk' for mandatory clock.
> > > This approach it correct? Or an other approach seems better?
> > > Thank you very much for your point of view.
> > >
> >
> > It is ok for me to have ipg, ahb and per clocks at driver, but how can
> > you maintain DT consistent?
> 
> Adding new clock as optional one will just maintain the DT compatibility.
> 

How to handle node_usb_soc1's clock which is without clk name to consistent with three
clocks for node_usb_soc2 at driver? Except for adding some platform judge code at
driver, do you have other solutions?

node_usb_soc1: {
	clocks = <&clks IMX6_CLK_USBOH3>;
};

node_usb_soc2: {
	clocks = <&clks IMX27_CLK_USBIPG>, <&clks IMX27_CLK_USBOH3>, <&clks IMX27_CLK_USBPER>;
	clock-names = "ipg", "ahb", "per";
};

Peter

[RFC] usb issue on imx27: 3 clocks are needed

[Shawn Guo]

On Mon, Aug 18, 2014 at 06:35:53PM +0800, Chen Peter-B29397 wrote:
>  
> > On Mon, Aug 18, 2014 at 05:00:59PM +0800, Peter Chen wrote:
> > > On Sat, Aug 16, 2014 at 05:38:30PM +0200, Philippe Reynes wrote:
> > > > Hi all,
> > > >
> > > > i.MX27's usb needs three clocks (usb_ipg_gate, usb_ahb_gate and
> > > > usb_div) but the current chipidea driver implementation, and
> > > > devicetree, provides only ipg and ahb. Consequently, if the
> > > > bootloader don't enable the last one, the kernel will crash.
> > > >
> > > > Our approach/idea is to add a second, optionnal, clock in
> > > > ci_hdrc_imx.c with 'per' name in devicetree  and to add  clock name
> > 'main_clk' for mandatory clock.
> > > > This approach it correct? Or an other approach seems better?
> > > > Thank you very much for your point of view.
> > > >
> > >
> > > It is ok for me to have ipg, ahb and per clocks at driver, but how can
> > > you maintain DT consistent?
> > 
> > Adding new clock as optional one will just maintain the DT compatibility.
> > 
> 
> How to handle node_usb_soc1's clock which is without clk name to consistent with three
> clocks for node_usb_soc2 at driver? Except for adding some platform judge code at
> driver, do you have other solutions?
> 
> node_usb_soc1: {
> 	clocks = <&clks IMX6_CLK_USBOH3>;
> };
> 
> node_usb_soc2: {
> 	clocks = <&clks IMX27_CLK_USBIPG>, <&clks IMX27_CLK_USBOH3>, <&clks IMX27_CLK_USBPER>;
> 	clock-names = "ipg", "ahb", "per";
> };

Oh, what I was talking about is we need to ensure the new kernel
(handling additional clocks) doesn't break old/existing DTB.  That's
what we call DT compatibility.

There is no DT consistency to maintain.  If there is only one clock for
USB on i.MX6 while 3 clocks on i.MX27, that's something USB driver needs
to handle, as clearly these two SoCs have different programming model on
USB device.

Shawn

[RFC] usb issue on imx27: 3 clocks are needed

[gwenhael.goavec]

On Mon, 18 Aug 2014 10:35:53 +0000
Peter Chen <Peter.Chen@freescale.com> wrote:

>  
> > On Mon, Aug 18, 2014 at 05:00:59PM +0800, Peter Chen wrote:
> > > On Sat, Aug 16, 2014 at 05:38:30PM +0200, Philippe Reynes wrote:
> > > > Hi all,
> > > >
> > > > i.MX27's usb needs three clocks (usb_ipg_gate, usb_ahb_gate and
> > > > usb_div) but the current chipidea driver implementation, and
> > > > devicetree, provides only ipg and ahb. Consequently, if the
> > > > bootloader don't enable the last one, the kernel will crash.
> > > >
> > > > Our approach/idea is to add a second, optionnal, clock in
> > > > ci_hdrc_imx.c with 'per' name in devicetree  and to add  clock name
> > 'main_clk' for mandatory clock.
> > > > This approach it correct? Or an other approach seems better?
> > > > Thank you very much for your point of view.
> > > >
> > >
> > > It is ok for me to have ipg, ahb and per clocks at driver, but how can
> > > you maintain DT consistent?
> > 
> > Adding new clock as optional one will just maintain the DT compatibility.
> > 
> 
> How to handle node_usb_soc1's clock which is without clk name to consistent with three
> clocks for node_usb_soc2 at driver? Except for adding some platform judge code at
> driver, do you have other solutions?
> 
> node_usb_soc1: {
> 	clocks = <&clks IMX6_CLK_USBOH3>;
> };
> 
> node_usb_soc2: {
> 	clocks = <&clks IMX27_CLK_USBIPG>, <&clks IMX27_CLK_USBOH3>, <&clks IMX27_CLK_USBPER>;
> 	clock-names = "ipg", "ahb", "per";
> };
> 
> Peter
> --

Our idea is something like this :
node_usb_soc: {
	clocks = <&clks IMX27_CLK_USBxxx>;
	clock-names = "main_clk";
};
for all CPUs

and 
node_usb_imx27: {
	clocks = <&clks IMX27_CLK_USB_IPG_GATE>,
		<&clks IMX27_CLK_USB_DIV>;
	clock-names = "main_clk", "per";
};

For imx27 and CPUs with the need of more than one clock.

The last clock (ie ahb) is handled by usbmisc.c and if "per" clock is not
present in the dtsi chipidea will not fails, just setting data->per_clk = NULL
for prepare/unprepare.

Gwenhael

[RFC] usb issue on imx27: 3 clocks are needed

[Peter Chen]

On Mon, Aug 18, 2014 at 01:49:03PM +0200, gwenhael.goavec wrote:
> On Mon, 18 Aug 2014 10:35:53 +0000
> Peter Chen <Peter.Chen@freescale.com> wrote:
> 
> >  
> > > On Mon, Aug 18, 2014 at 05:00:59PM +0800, Peter Chen wrote:
> > > > On Sat, Aug 16, 2014 at 05:38:30PM +0200, Philippe Reynes wrote:
> > > > > Hi all,
> > > > >
> > > > > i.MX27's usb needs three clocks (usb_ipg_gate, usb_ahb_gate and
> > > > > usb_div) but the current chipidea driver implementation, and
> > > > > devicetree, provides only ipg and ahb. Consequently, if the
> > > > > bootloader don't enable the last one, the kernel will crash.
> > > > >
> > > > > Our approach/idea is to add a second, optionnal, clock in
> > > > > ci_hdrc_imx.c with 'per' name in devicetree  and to add  clock name
> > > 'main_clk' for mandatory clock.
> > > > > This approach it correct? Or an other approach seems better?
> > > > > Thank you very much for your point of view.
> > > > >
> > > >
> > > > It is ok for me to have ipg, ahb and per clocks at driver, but how can
> > > > you maintain DT consistent?
> > > 
> > > Adding new clock as optional one will just maintain the DT compatibility.
> > > 
> > 
> > How to handle node_usb_soc1's clock which is without clk name to consistent with three
> > clocks for node_usb_soc2 at driver? Except for adding some platform judge code at
> > driver, do you have other solutions?
> > 
> > node_usb_soc1: {
> > 	clocks = <&clks IMX6_CLK_USBOH3>;
> > };
> > 
> > node_usb_soc2: {
> > 	clocks = <&clks IMX27_CLK_USBIPG>, <&clks IMX27_CLK_USBOH3>, <&clks IMX27_CLK_USBPER>;
> > 	clock-names = "ipg", "ahb", "per";
> > };
> > 
> > Peter
> > --
> 
> Our idea is something like this :
> node_usb_soc: {
> 	clocks = <&clks IMX27_CLK_USBxxx>;
> 	clock-names = "main_clk";
> };
> for all CPUs
> 
> and 
> node_usb_imx27: {
> 	clocks = <&clks IMX27_CLK_USB_IPG_GATE>,
> 		<&clks IMX27_CLK_USB_DIV>;
> 	clock-names = "main_clk", "per";
> };
> 
> For imx27 and CPUs with the need of more than one clock.
> 

Just like Shawn said, it breaks DT compatibility, how the old dtb works
with newer version kernel after you change.

> The last clock (ie ahb) is handled by usbmisc.c and if "per" clock is not
> present in the dtsi chipidea will not fails, just setting data->per_clk = NULL
> for prepare/unprepare.
> 
> Gwenhael

-- 
Best Regards,
Peter Chen

[RFC] usb issue on imx27: 3 clocks are needed

[gwenhael.goavec]

On Tue, 19 Aug 2014 08:18:17 +0800
Peter Chen <peter.chen@freescale.com> wrote:

> On Mon, Aug 18, 2014 at 01:49:03PM +0200, gwenhael.goavec wrote:
> > On Mon, 18 Aug 2014 10:35:53 +0000
> > Peter Chen <Peter.Chen@freescale.com> wrote:
> > 
> > >  
> > > > On Mon, Aug 18, 2014 at 05:00:59PM +0800, Peter Chen wrote:
> > > > > On Sat, Aug 16, 2014 at 05:38:30PM +0200, Philippe Reynes wrote:
> > > > > > Hi all,
> > > > > >
> > > > > > i.MX27's usb needs three clocks (usb_ipg_gate, usb_ahb_gate and
> > > > > > usb_div) but the current chipidea driver implementation, and
> > > > > > devicetree, provides only ipg and ahb. Consequently, if the
> > > > > > bootloader don't enable the last one, the kernel will crash.
> > > > > >
> > > > > > Our approach/idea is to add a second, optionnal, clock in
> > > > > > ci_hdrc_imx.c with 'per' name in devicetree  and to add  clock name
> > > > 'main_clk' for mandatory clock.
> > > > > > This approach it correct? Or an other approach seems better?
> > > > > > Thank you very much for your point of view.
> > > > > >
> > > > >
> > > > > It is ok for me to have ipg, ahb and per clocks at driver, but how can
> > > > > you maintain DT consistent?
> > > > 
> > > > Adding new clock as optional one will just maintain the DT compatibility.
> > > > 
> > > 
> > > How to handle node_usb_soc1's clock which is without clk name to consistent with three
> > > clocks for node_usb_soc2 at driver? Except for adding some platform judge code at
> > > driver, do you have other solutions?
> > > 
> > > node_usb_soc1: {
> > > 	clocks = <&clks IMX6_CLK_USBOH3>;
> > > };
> > > 
> > > node_usb_soc2: {
> > > 	clocks = <&clks IMX27_CLK_USBIPG>, <&clks IMX27_CLK_USBOH3>, <&clks IMX27_CLK_USBPER>;
> > > 	clock-names = "ipg", "ahb", "per";
> > > };
> > > 
> > > Peter
> > > --
> > 
> > Our idea is something like this :
> > node_usb_soc: {
> > 	clocks = <&clks IMX27_CLK_USBxxx>;
> > 	clock-names = "main_clk";
> > };
> > for all CPUs
> > 
> > and 
> > node_usb_imx27: {
> > 	clocks = <&clks IMX27_CLK_USB_IPG_GATE>,
> > 		<&clks IMX27_CLK_USB_DIV>;
> > 	clock-names = "main_clk", "per";
> > };
> > 
> > For imx27 and CPUs with the need of more than one clock.
> > 
> 
> Just like Shawn said, it breaks DT compatibility, how the old dtb works
> with newer version kernel after you change.
>
New dtsi must be updated according to the clock name and to avoid breaking old
dtb, i think the best way is to to do something like :

data->clk = devm_clk_get(&pdev->dev, "main_clk");
if (IS_ERR(data->clk)) {
	/* DT compatibility */
	data->clk = devm_clk_get(&pdev->dev, NULL);
	if (IS_ERR(data->clk)) {
		dev_err(&pdev->dev,
			"Failed to get main clock, err=%ld\n",
			PTR_ERR(data->clk));
		return PTR_ERR(data->clk);
	}   
}
Gwenhael

>
> > The last clock (ie ahb) is handled by usbmisc.c and if "per" clock is not
> > present in the dtsi chipidea will not fails, just setting data->per_clk = NULL
> > for prepare/unprepare.
> > 
> > Gwenhael
> 
> -- 
> Best Regards,
> Peter Chen