* [RFC] usb issue on imx27: 3 clocks are needed
@ 2014-08-18 10:35 ` Peter Chen
0 siblings, 0 replies; 19+ messages in thread
From: Peter Chen @ 2014-08-18 10:35 UTC (permalink / raw)
To: linux-arm-kernel
> 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
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [RFC] usb issue on imx27: 3 clocks are needed
2014-08-18 10:35 ` Peter Chen
@ 2014-08-18 11:48 ` Shawn Guo
-1 siblings, 0 replies; 19+ messages in thread
From: Shawn Guo @ 2014-08-18 11:48 UTC (permalink / raw)
To: Chen Peter-B29397
Cc: Philippe Reynes, Linux-kernel@vger.kernel.org,
linux-usb@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
gwenhael.goavec-merou@trabucayre.com
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
^ permalink raw reply [flat|nested] 19+ messages in thread* [RFC] usb issue on imx27: 3 clocks are needed
@ 2014-08-18 11:48 ` Shawn Guo
0 siblings, 0 replies; 19+ messages in thread
From: Shawn Guo @ 2014-08-18 11:48 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC] usb issue on imx27: 3 clocks are needed
2014-08-18 10:35 ` Peter Chen
@ 2014-08-18 11:49 ` gwenhael.goavec
-1 siblings, 0 replies; 19+ messages in thread
From: gwenhael.goavec @ 2014-08-18 11:49 UTC (permalink / raw)
To: Peter Chen
Cc: Shawn Guo, Philippe Reynes, Linux-kernel@vger.kernel.org,
linux-usb@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
gwenhael.goavec-merou@trabucayre.com
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
^ permalink raw reply [flat|nested] 19+ messages in thread* [RFC] usb issue on imx27: 3 clocks are needed
@ 2014-08-18 11:49 ` gwenhael.goavec
0 siblings, 0 replies; 19+ messages in thread
From: gwenhael.goavec @ 2014-08-18 11:49 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 19+ messages in thread* [RFC] usb issue on imx27: 3 clocks are needed
2014-08-18 11:49 ` gwenhael.goavec
(?)
@ 2014-08-19 0:18 ` Peter Chen
2014-08-19 8:47 ` gwenhael.goavec
-1 siblings, 1 reply; 19+ messages in thread
From: Peter Chen @ 2014-08-19 0:18 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [RFC] usb issue on imx27: 3 clocks are needed
2014-08-19 0:18 ` Peter Chen
@ 2014-08-19 8:47 ` gwenhael.goavec
0 siblings, 0 replies; 19+ messages in thread
From: gwenhael.goavec @ 2014-08-19 8:47 UTC (permalink / raw)
To: Peter Chen
Cc: Shawn Guo, Philippe Reynes, Linux-kernel@vger.kernel.org,
linux-usb@vger.kernel.org, linux-arm-kernel@lists.infradead.org
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
^ permalink raw reply [flat|nested] 19+ messages in thread* [RFC] usb issue on imx27: 3 clocks are needed
@ 2014-08-19 8:47 ` gwenhael.goavec
0 siblings, 0 replies; 19+ messages in thread
From: gwenhael.goavec @ 2014-08-19 8:47 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 19+ messages in thread