usb issue on imx27: 3 clocks are needed

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.

Hi Philippe,

Quoted 6 lines · 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.

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<[email protected]>
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<[email protected]>
     Signed-off-by: Shawn Guo<[email protected]>

Do you have this one applied?

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.

Quoted 7 lines · 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.

Quoted 5 lines

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?

Quoted 10 lines · Philippe Reynes

Quoted 9 lines

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.

Quoted 6 lines

Quoted 3 lines

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.

Quoted 13 lines · Peter Chen

Quoted 10 lines · Philippe Reynes

Quoted 9 lines

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.

Quoted 9 lines

Quoted 6 lines

Quoted 3 lines

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

Quoted 13 lines · Peter Chen

Quoted 10 lines · Philippe Reynes

Quoted 9 lines

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.

Quoted 9 lines

Quoted 6 lines

Quoted 3 lines

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

Quoted 31 lines · On Mon, 18 Aug 2014 10:35:53 +0000 Peter Chen <[email protected]> wrote:

Quoted 13 lines · Peter Chen

Quoted 10 lines · Philippe Reynes

Quoted 9 lines

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.

Quoted 9 lines

Quoted 6 lines

Quoted 3 lines

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

Quoted 7 lines

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

Quoted 56 lines · gwenhael.goavec

Quoted 31 lines · On Mon, 18 Aug 2014 10:35:53 +0000 Peter Chen <[email protected]> wrote:

Quoted 13 lines · Peter Chen

Quoted 10 lines · Philippe Reynes

Quoted 9 lines

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.

Quoted 9 lines

Quoted 6 lines

Quoted 3 lines

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

Quoted 7 lines

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

Quoted 2 lines

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.

Quoted 6 lines

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

Signature · 2 lines

Best Regards,
Peter Chen