| Message ID | 1373988136-24622-1-git-send-email-b.brezillon@overkiz.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 16 Jul 2013, Boris BREZILLON wrote: > The AT91 PMC (Power Management Controller) provides an USB clock used by > USB Full Speed host (ohci) and USB Full Speed device (udc). > The usb drivers (ohci and udc) must configure this clock to 48Mhz. > This configuration was formely done in mach-at91/clock.c, but this > implementation will be removed when moving to common clk framework. > > This patch add support for usb clock retrieval and configuration, and is > backward compatible with the current at91 clk implementation (if usb clk > is not found, it does not configure/enable the usb clk). But it does print a warning in the system log, right? > @@ -144,6 +150,11 @@ static int usb_hcd_at91_probe(const struct hc_driver *driver, > goto err2; > } > > + uclk = clk_get(&pdev->dev, "usb_clk"); > + if (IS_ERR(uclk)) { > + uclk = NULL; > + dev_warn(&pdev->dev, "failed to get usb_clk\n"); > + } Is this really what you want for backward compatibility? Alan Stern
On Tue, Jul 16, 2013 at 05:22:15PM +0200, Boris BREZILLON wrote: > @@ -41,6 +41,10 @@ extern int usb_disabled(void); > > static void at91_start_clock(void) > { > + if (uclk) { if (!IS_ERR(uclk)) { > + clk_set_rate(uclk, 48000000); > + clk_prepare_enable(uclk); > + } > clk_prepare_enable(hclk); > clk_prepare_enable(iclk); > clk_prepare_enable(fclk); > @@ -52,6 +56,8 @@ static void at91_stop_clock(void) > clk_disable_unprepare(fclk); > clk_disable_unprepare(iclk); > clk_disable_unprepare(hclk); > + if (uclk) if (!IS_ERR(uclk)) > + clk_disable_unprepare(uclk); > clocked = 0; > } > > @@ -144,6 +150,11 @@ static int usb_hcd_at91_probe(const struct hc_driver *driver, > goto err2; > } > > + uclk = clk_get(&pdev->dev, "usb_clk"); > + if (IS_ERR(uclk)) { > + uclk = NULL; Remove this line. > + dev_warn(&pdev->dev, "failed to get usb_clk\n"); > + } > iclk = clk_get(&pdev->dev, "ohci_clk"); > if (IS_ERR(iclk)) { > dev_err(&pdev->dev, "failed to get ohci_clk\n"); > @@ -212,10 +223,11 @@ static void usb_hcd_at91_remove(struct usb_hcd *hcd, > release_mem_region(hcd->rsrc_start, hcd->rsrc_len); > usb_put_hcd(hcd); > What if uclk is NULL here? if (!IS_ERR(uclk)) > + clk_put(uclk); > clk_put(hclk); > clk_put(fclk); > clk_put(iclk); > - fclk = iclk = hclk = NULL; > + fclk = iclk = hclk = uclk = NULL; Don't. Range of invalid struct clk pointers: those which IS_ERR(clk) returns true. Therefore, the range of valid struct clk pointers is: _____________________ (answer on a post card, stamped and addressed to...)
Hello Alan, On 16/07/2013 18:48, Alan Stern wrote: > On Tue, 16 Jul 2013, Boris BREZILLON wrote: > >> The AT91 PMC (Power Management Controller) provides an USB clock used by >> USB Full Speed host (ohci) and USB Full Speed device (udc). >> The usb drivers (ohci and udc) must configure this clock to 48Mhz. >> This configuration was formely done in mach-at91/clock.c, but this >> implementation will be removed when moving to common clk framework. >> >> This patch add support for usb clock retrieval and configuration, and is >> backward compatible with the current at91 clk implementation (if usb clk >> is not found, it does not configure/enable the usb clk). > But it does print a warning in the system log, right? Yes it does. > >> @@ -144,6 +150,11 @@ static int usb_hcd_at91_probe(const struct hc_driver *driver, >> goto err2; >> } >> >> + uclk = clk_get(&pdev->dev, "usb_clk"); >> + if (IS_ERR(uclk)) { >> + uclk = NULL; >> + dev_warn(&pdev->dev, "failed to get usb_clk\n"); >> + } > Is this really what you want for backward compatibility? Here are some proposition to remove the warning message: 1) replace it with a dev_info and change the message: dev_info(&pdev->dev, "failed to get usb_clk (most likely using old at91 clk implementation)\n"); 2) drop the log and silently ignore the missing clk (I'm not a big fan of this solution as it may lead to some errors if we're using new clk implem and the clock is really missing) 3) rework the current clk_set_rate function to accept clk_set_rate on usb clk and add clk_lookup entries for the usb clk (I'm not a big fan of this solution neither as this modifications will only be used for a short time until the transition to common clk framework is completed). > Alan Stern > Thanks for your review. Best Regards, Boris
On Tue, 16 Jul 2013, boris brezillon wrote: > >> + uclk = clk_get(&pdev->dev, "usb_clk"); > >> + if (IS_ERR(uclk)) { > >> + uclk = NULL; > >> + dev_warn(&pdev->dev, "failed to get usb_clk\n"); > >> + } > > Is this really what you want for backward compatibility? > Here are some proposition to remove the warning message: > > 1) replace it with a dev_info and change the message: > dev_info(&pdev->dev, "failed to get usb_clk (most likely using old > at91 clk implementation)\n"); > 2) drop the log and silently ignore the missing clk (I'm not a big fan > of this solution > as it may lead to some errors if we're using new clk implem and the > clock is really missing) > 3) rework the current clk_set_rate function to accept clk_set_rate on > usb clk and add clk_lookup entries > for the usb clk (I'm not a big fan of this solution neither as this > modifications will only be used for a short time > until the transition to common clk framework is completed). Another possibility is to combine this change with the clock implementation update, and do them in a single patch. Then backward compatibility would not be an issue. Alan Stern
On 16/07/2013 20:47, Alan Stern wrote: > On Tue, 16 Jul 2013, boris brezillon wrote: > >>>> + uclk = clk_get(&pdev->dev, "usb_clk"); >>>> + if (IS_ERR(uclk)) { >>>> + uclk = NULL; >>>> + dev_warn(&pdev->dev, "failed to get usb_clk\n"); >>>> + } >>> Is this really what you want for backward compatibility? >> Here are some proposition to remove the warning message: >> >> 1) replace it with a dev_info and change the message: >> dev_info(&pdev->dev, "failed to get usb_clk (most likely using old >> at91 clk implementation)\n"); >> 2) drop the log and silently ignore the missing clk (I'm not a big fan >> of this solution >> as it may lead to some errors if we're using new clk implem and the >> clock is really missing) >> 3) rework the current clk_set_rate function to accept clk_set_rate on >> usb clk and add clk_lookup entries >> for the usb clk (I'm not a big fan of this solution neither as this >> modifications will only be used for a short time >> until the transition to common clk framework is completed). > Another possibility is to combine this change with the clock > implementation update, and do them in a single patch. Then backward > compatibility would not be an issue. Yes, that was one of the question I asked in the cover-letter. I think I'll move these patches in the "move to common clk" series. Thanks > Alan Stern >
On Tue, 16 Jul 2013, Boris BREZILLON wrote: > The AT91 PMC (Power Management Controller) provides an USB clock used by > USB Full Speed host (ohci) and USB Full Speed device (udc). > The usb drivers (ohci and udc) must configure this clock to 48Mhz. > This configuration was formely done in mach-at91/clock.c, but this > implementation will be removed when moving to common clk framework. > > This patch add support for usb clock retrieval and configuration, and is > backward compatible with the current at91 clk implementation (if usb clk > is not found, it does not configure/enable the usb clk). This does not take into account any of the changes you discussed with Russell King and me -- it is exactly the same as the previous version. > @@ -144,6 +150,11 @@ static int usb_hcd_at91_probe(const struct hc_driver *driver, > goto err2; > } > > + uclk = clk_get(&pdev->dev, "usb_clk"); > + if (IS_ERR(uclk)) { > + uclk = NULL; > + dev_warn(&pdev->dev, "failed to get usb_clk\n"); > + } > iclk = clk_get(&pdev->dev, "ohci_clk"); > if (IS_ERR(iclk)) { > dev_err(&pdev->dev, "failed to get ohci_clk\n"); > @@ -212,10 +223,11 @@ static void usb_hcd_at91_remove(struct usb_hcd *hcd, > release_mem_region(hcd->rsrc_start, hcd->rsrc_len); > usb_put_hcd(hcd); > > + clk_put(uclk); What will clk_put() do when uclk is NULL? Alan Stern
On 17/07/2013 17:33, Alan Stern wrote: > On Tue, 16 Jul 2013, Boris BREZILLON wrote: > >> The AT91 PMC (Power Management Controller) provides an USB clock used by >> USB Full Speed host (ohci) and USB Full Speed device (udc). >> The usb drivers (ohci and udc) must configure this clock to 48Mhz. >> This configuration was formely done in mach-at91/clock.c, but this >> implementation will be removed when moving to common clk framework. >> >> This patch add support for usb clock retrieval and configuration, and is >> backward compatible with the current at91 clk implementation (if usb clk >> is not found, it does not configure/enable the usb clk). > This does not take into account any of the changes you discussed with > Russell King and me -- it is exactly the same as the previous version. Sorry, I don't understand. I didn't send any new version since yesterday. I'm sending it right now and it's part of the "ARM: at91: move to common clk framework" series (as discussed with you). >> @@ -144,6 +150,11 @@ static int usb_hcd_at91_probe(const struct hc_driver *driver, >> goto err2; >> } >> >> + uclk = clk_get(&pdev->dev, "usb_clk"); >> + if (IS_ERR(uclk)) { >> + uclk = NULL; >> + dev_warn(&pdev->dev, "failed to get usb_clk\n"); >> + } >> iclk = clk_get(&pdev->dev, "ohci_clk"); >> if (IS_ERR(iclk)) { >> dev_err(&pdev->dev, "failed to get ohci_clk\n"); >> @@ -212,10 +223,11 @@ static void usb_hcd_at91_remove(struct usb_hcd *hcd, >> release_mem_region(hcd->rsrc_start, hcd->rsrc_len); >> usb_put_hcd(hcd); >> >> + clk_put(uclk); > What will clk_put() do when uclk is NULL? > > Alan Stern >
On Wed, 17 Jul 2013, boris brezillon wrote: > On 17/07/2013 17:33, Alan Stern wrote: > > On Tue, 16 Jul 2013, Boris BREZILLON wrote: > > > >> The AT91 PMC (Power Management Controller) provides an USB clock used by > >> USB Full Speed host (ohci) and USB Full Speed device (udc). > >> The usb drivers (ohci and udc) must configure this clock to 48Mhz. > >> This configuration was formely done in mach-at91/clock.c, but this > >> implementation will be removed when moving to common clk framework. > >> > >> This patch add support for usb clock retrieval and configuration, and is > >> backward compatible with the current at91 clk implementation (if usb clk > >> is not found, it does not configure/enable the usb clk). > > This does not take into account any of the changes you discussed with > > Russell King and me -- it is exactly the same as the previous version. > Sorry, I don't understand. I didn't send any new version since yesterday. Oh. Never mind. That message was the _same_ one that I replied to yesterday. I got two copies of it, because you sent it both to me directly and to linux-usb. For some reason one of the copies was delayed for many hours, so I thought it was a new message. Alan Stern
diff --git a/drivers/usb/host/ohci-at91.c b/drivers/usb/host/ohci-at91.c index 9677f68..426c7d2 100644 --- a/drivers/usb/host/ohci-at91.c +++ b/drivers/usb/host/ohci-at91.c @@ -31,8 +31,8 @@ #define at91_for_each_port(index) \ for ((index) = 0; (index) < AT91_MAX_USBH_PORTS; (index)++) -/* interface and function clocks; sometimes also an AHB clock */ -static struct clk *iclk, *fclk, *hclk; +/* interface, function and usb clocks; sometimes also an AHB clock */ +static struct clk *iclk, *fclk, *uclk, *hclk; static int clocked; extern int usb_disabled(void); @@ -41,6 +41,10 @@ extern int usb_disabled(void); static void at91_start_clock(void) { + if (uclk) { + clk_set_rate(uclk, 48000000); + clk_prepare_enable(uclk); + } clk_prepare_enable(hclk); clk_prepare_enable(iclk); clk_prepare_enable(fclk); @@ -52,6 +56,8 @@ static void at91_stop_clock(void) clk_disable_unprepare(fclk); clk_disable_unprepare(iclk); clk_disable_unprepare(hclk); + if (uclk) + clk_disable_unprepare(uclk); clocked = 0; } @@ -144,6 +150,11 @@ static int usb_hcd_at91_probe(const struct hc_driver *driver, goto err2; } + uclk = clk_get(&pdev->dev, "usb_clk"); + if (IS_ERR(uclk)) { + uclk = NULL; + dev_warn(&pdev->dev, "failed to get usb_clk\n"); + } iclk = clk_get(&pdev->dev, "ohci_clk"); if (IS_ERR(iclk)) { dev_err(&pdev->dev, "failed to get ohci_clk\n"); @@ -212,10 +223,11 @@ static void usb_hcd_at91_remove(struct usb_hcd *hcd, release_mem_region(hcd->rsrc_start, hcd->rsrc_len); usb_put_hcd(hcd); + clk_put(uclk); clk_put(hclk); clk_put(fclk); clk_put(iclk); - fclk = iclk = hclk = NULL; + fclk = iclk = hclk = uclk = NULL; dev_set_drvdata(&pdev->dev, NULL); }
The AT91 PMC (Power Management Controller) provides an USB clock used by USB Full Speed host (ohci) and USB Full Speed device (udc). The usb drivers (ohci and udc) must configure this clock to 48Mhz. This configuration was formely done in mach-at91/clock.c, but this implementation will be removed when moving to common clk framework. This patch add support for usb clock retrieval and configuration, and is backward compatible with the current at91 clk implementation (if usb clk is not found, it does not configure/enable the usb clk). Signed-off-by: Boris BREZILLON <b.brezillon@overkiz.com> --- drivers/usb/host/ohci-at91.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-)