| Message ID | 1352909950-32555-3-git-send-email-m.grzeschik@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Nov 14, 2012 at 05:19:03PM +0100, Michael Grzeschik wrote: > This patch adds support for a second and third clock to the chipidea driver. On > modern freescale ARM cores like the imx51, imx53 and imx6q three clocks ("ahb", > "ipg" and "per") must be enabled in order to access the USB core. > > In the original driver, the clock was requested without specifying the > connection id, further all mainline ARM archs with support for the chipidea > core (imx23, imx28) register their USB clock without a connection id. > > This patch first renames the existing clk variable to clk_ahb. The connection > id "ahb" is added to the devm_clk_get() call. Then the clocks "ipg" and "per" > are requested. As all archs don't specify a connection id, all clk_get return > the same clock. This ensures compatibility to existing USB support and adds > support for imx5x at the same time. > > This patch has been tested on imx28 and on imx53 with seperate "ahb", "ipg" > and "per" clocks. mx23, mx28, and mx6q has the same usb clock sources and different with mxc (mx5x, mx3x). I am not sure which method is better: - Add dummy clock at clock.c - Add platform information(id_table or something similar) at driver. Add dummy clock may confuse some users, for example, mx6q has no "per" and "ipg" clock at all. > > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> > Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de> > --- > drivers/usb/chipidea/ci13xxx_imx.c | 54 ++++++++++++++++++++++++++++++------ > 1 file changed, 45 insertions(+), 9 deletions(-) > > diff --git a/drivers/usb/chipidea/ci13xxx_imx.c b/drivers/usb/chipidea/ci13xxx_imx.c > index 935de97..570aedf 100644 > --- a/drivers/usb/chipidea/ci13xxx_imx.c > +++ b/drivers/usb/chipidea/ci13xxx_imx.c > @@ -32,7 +32,9 @@ struct ci13xxx_imx_data { > struct device_node *phy_np; > struct usb_phy *phy; > struct platform_device *ci_pdev; > - struct clk *clk; > + struct clk *clk_ahb; > + struct clk *clk_ipg; > + struct clk *clk_per; > struct regulator *reg_vbus; > }; > > @@ -144,20 +146,48 @@ static int __devinit ci13xxx_imx_probe(struct platform_device *pdev) > dev_warn(&pdev->dev, "pinctrl get/select failed, err=%ld\n", > PTR_ERR(pinctrl)); > > - data->clk = devm_clk_get(&pdev->dev, NULL); > - if (IS_ERR(data->clk)) { > + data->clk_ahb = devm_clk_get(&pdev->dev, "ahb"); > + if (IS_ERR(data->clk_ahb)) { > dev_err(&pdev->dev, > - "Failed to get clock, err=%ld\n", PTR_ERR(data->clk)); > - return PTR_ERR(data->clk); > + "Failed to get ahb clock, err=%ld\n", PTR_ERR(data->clk_ahb)); > + return PTR_ERR(data->clk_ahb); > } > > - ret = clk_prepare_enable(data->clk); > + data->clk_ipg = devm_clk_get(&pdev->dev, "ipg"); > + if (IS_ERR(data->clk_ipg)) { > + dev_err(&pdev->dev, > + "Failed to get ipg clock, err=%ld\n", PTR_ERR(data->clk_ipg)); > + return PTR_ERR(data->clk_ipg); > + } > + > + data->clk_per = devm_clk_get(&pdev->dev, "per"); > + if (IS_ERR(data->clk_per)) { > + dev_err(&pdev->dev, > + "Failed to get per clock, err=%ld\n", PTR_ERR(data->clk_per)); > + return PTR_ERR(data->clk_per); > + } > + > + ret = clk_prepare_enable(data->clk_ahb); > if (ret) { > dev_err(&pdev->dev, > - "Failed to prepare or enable clock, err=%d\n", ret); > + "Failed to prepare or enable ahb clock, err=%d\n", ret); > return ret; > } > > + ret = clk_prepare_enable(data->clk_ipg); > + if (ret) { > + dev_err(&pdev->dev, > + "Failed to prepare or enable ipg clock, err=%d\n", ret); > + goto err_ipg_failed; > + } > + > + ret = clk_prepare_enable(data->clk_per); > + if (ret) { > + dev_err(&pdev->dev, > + "Failed to prepare or enable per clock, err=%d\n", ret); > + goto err_per_failed; > + } > + > phy_np = of_parse_phandle(pdev->dev.of_node, "fsl,usbphy", 0); > if (phy_np) { > data->phy_np = phy_np; > @@ -246,7 +276,11 @@ static int __devinit ci13xxx_imx_probe(struct platform_device *pdev) > put_np: > if (phy_np) > of_node_put(phy_np); > - clk_disable_unprepare(data->clk); > + clk_disable_unprepare(data->clk_per); > +err_per_failed: > + clk_disable_unprepare(data->clk_ipg); > +err_ipg_failed: > + clk_disable_unprepare(data->clk_ahb); > > return ret; > } > @@ -268,7 +302,9 @@ static int __devexit ci13xxx_imx_remove(struct platform_device *pdev) > > of_node_put(data->phy_np); > > - clk_disable_unprepare(data->clk); > + clk_disable_unprepare(data->clk_per); > + clk_disable_unprepare(data->clk_ipg); > + clk_disable_unprepare(data->clk_ahb); > > platform_set_drvdata(pdev, NULL); > > -- > 1.7.10.4 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-usb" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
On Mon, Nov 26, 2012 at 05:29:31PM +0800, Peter Chen wrote: > On Wed, Nov 14, 2012 at 05:19:03PM +0100, Michael Grzeschik wrote: > > This patch adds support for a second and third clock to the chipidea driver. On > > modern freescale ARM cores like the imx51, imx53 and imx6q three clocks ("ahb", > > "ipg" and "per") must be enabled in order to access the USB core. > > > > In the original driver, the clock was requested without specifying the > > connection id, further all mainline ARM archs with support for the chipidea > > core (imx23, imx28) register their USB clock without a connection id. > > > > This patch first renames the existing clk variable to clk_ahb. The connection > > id "ahb" is added to the devm_clk_get() call. Then the clocks "ipg" and "per" > > are requested. As all archs don't specify a connection id, all clk_get return > > the same clock. This ensures compatibility to existing USB support and adds > > support for imx5x at the same time. > > > > This patch has been tested on imx28 and on imx53 with seperate "ahb", "ipg" > > and "per" clocks. > mx23, mx28, and mx6q has the same usb clock sources and different with > mxc (mx5x, mx3x). > > I am not sure which method is better: > - Add dummy clock at clock.c > - Add platform information(id_table or something similar) at driver. > > Add dummy clock may confuse some users, for example, mx6q has no > "per" and "ipg" clock at all. The general idea is: The USB core has different input clocks. The driver has to be provided with these input clocks. When i.MX6 does not have these clocks, it only means that this SoC has no software controllable gates for these clocks, thus they are not documented. Still this SoC has these clocks. This way we can describe the differences between the SoC purely on SoC level without bothering the driver. You might want to ask your hardware guys to get more information what input clocks the USB core actually has, there actually is a lot of guesswork in it due to missing/inconsistent/confusing documentation. Sascha
On Mon, Nov 26, 2012 at 11:22:32AM +0100, Sascha Hauer wrote: > On Mon, Nov 26, 2012 at 05:29:31PM +0800, Peter Chen wrote: > > On Wed, Nov 14, 2012 at 05:19:03PM +0100, Michael Grzeschik wrote: > > > This patch adds support for a second and third clock to the chipidea driver. On > > > modern freescale ARM cores like the imx51, imx53 and imx6q three clocks ("ahb", > > > "ipg" and "per") must be enabled in order to access the USB core. > > > > > > In the original driver, the clock was requested without specifying the > > > connection id, further all mainline ARM archs with support for the chipidea > > > core (imx23, imx28) register their USB clock without a connection id. > > > > > > This patch first renames the existing clk variable to clk_ahb. The connection > > > id "ahb" is added to the devm_clk_get() call. Then the clocks "ipg" and "per" > > > are requested. As all archs don't specify a connection id, all clk_get return > > > the same clock. This ensures compatibility to existing USB support and adds > > > support for imx5x at the same time. > > > > > > This patch has been tested on imx28 and on imx53 with seperate "ahb", "ipg" > > > and "per" clocks. > > mx23, mx28, and mx6q has the same usb clock sources and different with > > mxc (mx5x, mx3x). > > > > I am not sure which method is better: > > - Add dummy clock at clock.c > > - Add platform information(id_table or something similar) at driver. > > > > Add dummy clock may confuse some users, for example, mx6q has no > > "per" and "ipg" clock at all. > > The general idea is: The USB core has different input clocks. The driver > has to be provided with these input clocks. When i.MX6 does not have > these clocks, it only means that this SoC has no software controllable > gates for these clocks, thus they are not documented. Still this SoC > has these clocks. > This way we can describe the differences between the SoC purely on SoC > level without bothering the driver. > You might want to ask your hardware guys to get more information what > input clocks the USB core actually has, there actually is a lot of > guesswork in it due to missing/inconsistent/confusing documentation. Discussed with USB IC module owner, some feedback like below: - You are correct, the input clock for controller is always same, there are three sources: - ahb - xcvr_clk - xcvr_ser_clk - ahb: the source is the system ipg and ahb source, but for usb subsystem, there is NO separate control for ipg and ahb, there is only one bit to control ipg and ahb clock together, just like we say usboh3_ipg_ahb bit at CCM for mx51. The USB controller internal will have two clocks, one for ipg (visit register), and the other is ahb (access DDR). From the system point, we only need to control usboh3_ipg_ahb, so, this clock is better as "usb_ahb". "ipg" clock for usb may only the system ipg clock. -xcvr_clk: it is the phy output clk, the software can't control it, (for mx28/mx6q, it can be controlled from phy controller), the software can only choose the PHY clk source. -xcvr_ser_clk, it is used at serial phy mode, we need it as the default phy mode is serial. I find we take it as "usb_per" at mx5 platform, but this xcvr_ser_clk is fixed (60M), and can't be adjusted (it is not different with 54M TLL clock). So in order to consolidate i.mx usb clock, do you think below definitions are ok: usb_ahb: ahb clock used to visit register and access DDR usb_ipg: system ipg clock (mx28 usb no ipg clock) usb_per: serial phy clock, it is useless if serial phy is not supported, eg mx28, mx6q. usb_phy: phy clock source, either from 24M or PLL. So, in order to align with kinds of platforms, I suggest: If the clock exists and share the same bit at CCM with others (like usb_ahb and usb_ipg at some platforms), give the same clock num but different connection id. If the clock does not exists, just give a dummy clock but with connection id (like usb_per at imx6q).
On Tue, Nov 27, 2012 at 02:50:26PM +0800, Peter Chen wrote: > On Mon, Nov 26, 2012 at 11:22:32AM +0100, Sascha Hauer wrote: > > On Mon, Nov 26, 2012 at 05:29:31PM +0800, Peter Chen wrote: > > > On Wed, Nov 14, 2012 at 05:19:03PM +0100, Michael Grzeschik wrote: > > > > This patch adds support for a second and third clock to the chipidea driver. On > > > > modern freescale ARM cores like the imx51, imx53 and imx6q three clocks ("ahb", > > > > "ipg" and "per") must be enabled in order to access the USB core. > > > > > > > > In the original driver, the clock was requested without specifying the > > > > connection id, further all mainline ARM archs with support for the chipidea > > > > core (imx23, imx28) register their USB clock without a connection id. > > > > > > > > This patch first renames the existing clk variable to clk_ahb. The connection > > > > id "ahb" is added to the devm_clk_get() call. Then the clocks "ipg" and "per" > > > > are requested. As all archs don't specify a connection id, all clk_get return > > > > the same clock. This ensures compatibility to existing USB support and adds > > > > support for imx5x at the same time. > > > > > > > > This patch has been tested on imx28 and on imx53 with seperate "ahb", "ipg" > > > > and "per" clocks. > > > mx23, mx28, and mx6q has the same usb clock sources and different with > > > mxc (mx5x, mx3x). > > > > > > I am not sure which method is better: > > > - Add dummy clock at clock.c > > > - Add platform information(id_table or something similar) at driver. > > > > > > Add dummy clock may confuse some users, for example, mx6q has no > > > "per" and "ipg" clock at all. > > > > The general idea is: The USB core has different input clocks. The driver > > has to be provided with these input clocks. When i.MX6 does not have > > these clocks, it only means that this SoC has no software controllable > > gates for these clocks, thus they are not documented. Still this SoC > > has these clocks. > > This way we can describe the differences between the SoC purely on SoC > > level without bothering the driver. > > You might want to ask your hardware guys to get more information what > > input clocks the USB core actually has, there actually is a lot of > > guesswork in it due to missing/inconsistent/confusing documentation. > Discussed with USB IC module owner, some feedback like below: > - You are correct, the input clock for controller is always same, there > are three sources: > - ahb > - xcvr_clk > - xcvr_ser_clk > - ahb: the source is the system ipg and ahb source, but for usb subsystem, > there is NO separate control for ipg and ahb, there is only one bit > to control ipg and ahb clock together, just like we say usboh3_ipg_ahb bit > at CCM for mx51. The USB controller internal will have two clocks, one for > ipg (visit register), and the other is ahb (access DDR). > From the system point, we only need to control usboh3_ipg_ahb, > so, this clock is better as "usb_ahb". "ipg" clock for usb may only the > system ipg clock. The i.MX27 has separate gates for ipg (PCCR1[25]) and ahb PCCR1[11]) > > -xcvr_clk: it is the phy output clk, the software can't control it, > (for mx28/mx6q, it can be controlled from phy controller), the software > can only choose the PHY clk source. > > -xcvr_ser_clk, it is used at serial phy mode, we need it as the default > phy mode is serial. > I find we take it as "usb_per" at mx5 platform, but this xcvr_ser_clk is > fixed (60M), and can't be adjusted (it is not different with 54M TLL clock). > > So in order to consolidate i.mx usb clock, do you think below definitions > are ok: > usb_ahb: ahb clock used to visit register and access DDR > usb_ipg: system ipg clock (mx28 usb no ipg clock) > usb_per: serial phy clock, it is useless if serial phy is not supported, eg mx28, mx6q. > usb_phy: phy clock source, either from 24M or PLL. > > So, in order to align with kinds of platforms, I suggest: > If the clock exists and share the same bit at CCM with others > (like usb_ahb and usb_ipg at some platforms), give the same clock num > but different connection id. Yes. > If the clock does not exists, just give a dummy clock but with > connection id (like usb_per at imx6q). Yes. Thanks for the informations, it's good to be able to get feedback from the hardware people. Sascha
diff --git a/drivers/usb/chipidea/ci13xxx_imx.c b/drivers/usb/chipidea/ci13xxx_imx.c index 935de97..570aedf 100644 --- a/drivers/usb/chipidea/ci13xxx_imx.c +++ b/drivers/usb/chipidea/ci13xxx_imx.c @@ -32,7 +32,9 @@ struct ci13xxx_imx_data { struct device_node *phy_np; struct usb_phy *phy; struct platform_device *ci_pdev; - struct clk *clk; + struct clk *clk_ahb; + struct clk *clk_ipg; + struct clk *clk_per; struct regulator *reg_vbus; }; @@ -144,20 +146,48 @@ static int __devinit ci13xxx_imx_probe(struct platform_device *pdev) dev_warn(&pdev->dev, "pinctrl get/select failed, err=%ld\n", PTR_ERR(pinctrl)); - data->clk = devm_clk_get(&pdev->dev, NULL); - if (IS_ERR(data->clk)) { + data->clk_ahb = devm_clk_get(&pdev->dev, "ahb"); + if (IS_ERR(data->clk_ahb)) { dev_err(&pdev->dev, - "Failed to get clock, err=%ld\n", PTR_ERR(data->clk)); - return PTR_ERR(data->clk); + "Failed to get ahb clock, err=%ld\n", PTR_ERR(data->clk_ahb)); + return PTR_ERR(data->clk_ahb); } - ret = clk_prepare_enable(data->clk); + data->clk_ipg = devm_clk_get(&pdev->dev, "ipg"); + if (IS_ERR(data->clk_ipg)) { + dev_err(&pdev->dev, + "Failed to get ipg clock, err=%ld\n", PTR_ERR(data->clk_ipg)); + return PTR_ERR(data->clk_ipg); + } + + data->clk_per = devm_clk_get(&pdev->dev, "per"); + if (IS_ERR(data->clk_per)) { + dev_err(&pdev->dev, + "Failed to get per clock, err=%ld\n", PTR_ERR(data->clk_per)); + return PTR_ERR(data->clk_per); + } + + ret = clk_prepare_enable(data->clk_ahb); if (ret) { dev_err(&pdev->dev, - "Failed to prepare or enable clock, err=%d\n", ret); + "Failed to prepare or enable ahb clock, err=%d\n", ret); return ret; } + ret = clk_prepare_enable(data->clk_ipg); + if (ret) { + dev_err(&pdev->dev, + "Failed to prepare or enable ipg clock, err=%d\n", ret); + goto err_ipg_failed; + } + + ret = clk_prepare_enable(data->clk_per); + if (ret) { + dev_err(&pdev->dev, + "Failed to prepare or enable per clock, err=%d\n", ret); + goto err_per_failed; + } + phy_np = of_parse_phandle(pdev->dev.of_node, "fsl,usbphy", 0); if (phy_np) { data->phy_np = phy_np; @@ -246,7 +276,11 @@ static int __devinit ci13xxx_imx_probe(struct platform_device *pdev) put_np: if (phy_np) of_node_put(phy_np); - clk_disable_unprepare(data->clk); + clk_disable_unprepare(data->clk_per); +err_per_failed: + clk_disable_unprepare(data->clk_ipg); +err_ipg_failed: + clk_disable_unprepare(data->clk_ahb); return ret; } @@ -268,7 +302,9 @@ static int __devexit ci13xxx_imx_remove(struct platform_device *pdev) of_node_put(data->phy_np); - clk_disable_unprepare(data->clk); + clk_disable_unprepare(data->clk_per); + clk_disable_unprepare(data->clk_ipg); + clk_disable_unprepare(data->clk_ahb); platform_set_drvdata(pdev, NULL);
This patch adds support for a second and third clock to the chipidea driver. On modern freescale ARM cores like the imx51, imx53 and imx6q three clocks ("ahb", "ipg" and "per") must be enabled in order to access the USB core. In the original driver, the clock was requested without specifying the connection id, further all mainline ARM archs with support for the chipidea core (imx23, imx28) register their USB clock without a connection id. This patch first renames the existing clk variable to clk_ahb. The connection id "ahb" is added to the devm_clk_get() call. Then the clocks "ipg" and "per" are requested. As all archs don't specify a connection id, all clk_get return the same clock. This ensures compatibility to existing USB support and adds support for imx5x at the same time. This patch has been tested on imx28 and on imx53 with seperate "ahb", "ipg" and "per" clocks. Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de> --- drivers/usb/chipidea/ci13xxx_imx.c | 54 ++++++++++++++++++++++++++++++------ 1 file changed, 45 insertions(+), 9 deletions(-)