| Message ID | 1358511445-26656-1-git-send-email-rogerq@ti.com |
|---|---|
| State | New |
| Headers | show |
On 01/18/2013 02:17 PM, Roger Quadros wrote: > This driver does not request any gpios so don't free them. > Fixes L3 bus error on multiple modprobe/rmmod of ehci_hcd > with ehci-omap in use. > > Without this patch, EHCI will break on repeated insmod/rmmod > of ehci_hcd for all OMAP2+ platforms that use EHCI and > set 'phy_reset = true' in usbhs_omap_board_data. > i.e. > > board-3430sdp.c: .phy_reset = true, > board-3630sdp.c: .phy_reset = true, > board-am3517crane.c: .phy_reset = true, > board-am3517evm.c: .phy_reset = true, > board-cm-t3517.c: .phy_reset = true, > board-cm-t35.c: .phy_reset = true, > board-devkit8000.c: .phy_reset = true, > board-igep0020.c: .phy_reset = true, > board-igep0020.c: .phy_reset = true, > board-omap3beagle.c: .phy_reset = true, > board-omap3evm.c: .phy_reset = true, > board-omap3pandora.c: .phy_reset = true, > board-omap3stalker.c: .phy_reset = true, > board-omap3touchbook.c: .phy_reset = true, > board-omap4panda.c: .phy_reset = false, > board-overo.c: .phy_reset = true, > board-zoom.c: .phy_reset = true, > > CC: Alan Stern <stern@rowland.harvard.edu> > Cc: stable@kernel.org I messed up with the stable list id, so will resend just this one and update the git repo. > > Signed-off-by: Roger Quadros <rogerq@ti.com> > Reviewed-by: Felipe Balbi <balbi@ti.com> > Acked-by: Alan Stern <stern@rowland.harvard.edu> cheers, -roger
On Fri, Jan 18, 2013 at 02:17:08PM +0200, Roger Quadros wrote: > + tll->ch_clk = devm_kzalloc(dev, sizeof(struct clk * [tll->nch]), > + GFP_KERNEL); > + if (!tll->ch_clk) { > + ret = -ENOMEM; > + dev_err(dev, "Couldn't allocate memory for channel clocks\n"); > + goto err_clk_alloc; > + } > + > + for (i = 0; i < tll->nch; i++) { > + char clkname[] = "usb_tll_hs_usb_chx_clk"; > + struct clk *fck; > + > + snprintf(clkname, sizeof(clkname), > + "usb_tll_hs_usb_ch%d_clk", i); > + fck = clk_get(dev, clkname); > + > + if (IS_ERR(fck)) > + dev_dbg(dev, "can't get clock : %s\n", clkname); > + else > + tll->ch_clk[i] = fck; Hmm. I'd actually suggest that this becomes: tll->ch_clk[i] = fck; if (IS_ERR(fck) dev_dbg(dev, "can't get clock: %s\n", clkname); so that tll->ch_clk is always written to. > static int usbtll_omap_remove(struct platform_device *pdev) > { > struct usbtll_omap *tll = platform_get_drvdata(pdev); > + int i; > + > + for (i = 0; i < tll->nch; i++) > + clk_put(tll->ch_clk[i]); And change this to: for (i = 0; i < tll->nch; i++) if (!IS_ERR(tll->ch_clk[i])) clk_put(tll->ch_clk[i]); so that you're not passing a NULL pointer in. > + for (i = 0; i < tll->nch; i++) { > + if (is_ehci_tll_mode(pdata->port_mode[i])) { > + int r; > > - if (is_ehci_tll_mode(pdata->port_mode[1])) > - clk_enable(tll->usbtll_p2_fck); > + if (!tll->ch_clk[i]) if (IS_ERR(tll->ch_clk[i])) > + continue; > + > + r = clk_enable(tll->ch_clk[i]); > + if (r) { > + dev_err(dev, > + "Error enabling ch %d clock: %d\n", i, r); > + } > + } > + } > @@ -387,11 +409,12 @@ static int usbtll_runtime_suspend(struct device *dev) > > spin_lock_irqsave(&tll->lock, flags); > > - if (is_ehci_tll_mode(pdata->port_mode[0])) > - clk_disable(tll->usbtll_p1_fck); > - > - if (is_ehci_tll_mode(pdata->port_mode[1])) > - clk_disable(tll->usbtll_p2_fck); > + for (i = 0; i < tll->nch; i++) { > + if (is_ehci_tll_mode(pdata->port_mode[i])) { > + if (tll->ch_clk[i]) if (!IS_ERR(tll->ch_clk[i])) > + clk_disable(tll->ch_clk[i]); > + } > + } > > spin_unlock_irqrestore(&tll->lock, flags); > > -- > 1.7.4.1 >
On Fri, Jan 18, 2013 at 02:17:09PM +0200, Roger Quadros wrote: > +/* only PHY and UNUSED modes don't need TLL */ > +#define omap_usb_mode_needs_tll(x) ((x != OMAP_USBHS_PORT_MODE_UNUSED) &&\ > + (x != OMAP_EHCI_PORT_MODE_PHY)) Growl. These parens do not make anything safer at all - they just obfuscate. The safe version of the above macro would have been: #define omap_usb_mode_needs_tll(x) ((x) != OMAP_USBHS_PORT_MODE_UNUSED &&\ (x) != OMAP_EHCI_PORT_MODE_PHY) If you're going to use a macro argument in an expression, it needs parens. Simple expressions like a != b && c != d do _not_ need parens.
On 01/18/2013 04:59 PM, Russell King - ARM Linux wrote: > On Fri, Jan 18, 2013 at 02:17:08PM +0200, Roger Quadros wrote: >> + tll->ch_clk = devm_kzalloc(dev, sizeof(struct clk * [tll->nch]), >> + GFP_KERNEL); >> + if (!tll->ch_clk) { >> + ret = -ENOMEM; >> + dev_err(dev, "Couldn't allocate memory for channel clocks\n"); >> + goto err_clk_alloc; >> + } >> + >> + for (i = 0; i < tll->nch; i++) { >> + char clkname[] = "usb_tll_hs_usb_chx_clk"; >> + struct clk *fck; >> + >> + snprintf(clkname, sizeof(clkname), >> + "usb_tll_hs_usb_ch%d_clk", i); >> + fck = clk_get(dev, clkname); >> + >> + if (IS_ERR(fck)) >> + dev_dbg(dev, "can't get clock : %s\n", clkname); >> + else >> + tll->ch_clk[i] = fck; > > Hmm. I'd actually suggest that this becomes: > > tll->ch_clk[i] = fck; > if (IS_ERR(fck) > dev_dbg(dev, "can't get clock: %s\n", clkname); > > so that tll->ch_clk is always written to. Yes, I'll fix this. > >> static int usbtll_omap_remove(struct platform_device *pdev) >> { >> struct usbtll_omap *tll = platform_get_drvdata(pdev); >> + int i; >> + >> + for (i = 0; i < tll->nch; i++) >> + clk_put(tll->ch_clk[i]); > > And change this to: > > for (i = 0; i < tll->nch; i++) > if (!IS_ERR(tll->ch_clk[i])) > clk_put(tll->ch_clk[i]); > > so that you're not passing a NULL pointer in. > ok. >> + for (i = 0; i < tll->nch; i++) { >> + if (is_ehci_tll_mode(pdata->port_mode[i])) { >> + int r; >> >> - if (is_ehci_tll_mode(pdata->port_mode[1])) >> - clk_enable(tll->usbtll_p2_fck); >> + if (!tll->ch_clk[i]) > > if (IS_ERR(tll->ch_clk[i])) ok. > >> + continue; >> + >> + r = clk_enable(tll->ch_clk[i]); >> + if (r) { >> + dev_err(dev, >> + "Error enabling ch %d clock: %d\n", i, r); >> + } >> + } >> + } > >> @@ -387,11 +409,12 @@ static int usbtll_runtime_suspend(struct device *dev) >> >> spin_lock_irqsave(&tll->lock, flags); >> >> - if (is_ehci_tll_mode(pdata->port_mode[0])) >> - clk_disable(tll->usbtll_p1_fck); >> - >> - if (is_ehci_tll_mode(pdata->port_mode[1])) >> - clk_disable(tll->usbtll_p2_fck); >> + for (i = 0; i < tll->nch; i++) { >> + if (is_ehci_tll_mode(pdata->port_mode[i])) { >> + if (tll->ch_clk[i]) > > if (!IS_ERR(tll->ch_clk[i])) ok. > >> + clk_disable(tll->ch_clk[i]); >> + } >> + } >> >> spin_unlock_irqrestore(&tll->lock, flags); >> -- cheers, -roger
On 01/18/2013 05:02 PM, Russell King - ARM Linux wrote: > On Fri, Jan 18, 2013 at 02:17:09PM +0200, Roger Quadros wrote: >> +/* only PHY and UNUSED modes don't need TLL */ >> +#define omap_usb_mode_needs_tll(x) ((x != OMAP_USBHS_PORT_MODE_UNUSED) &&\ >> + (x != OMAP_EHCI_PORT_MODE_PHY)) > > Growl. > > These parens do not make anything safer at all - they just obfuscate. The > safe version of the above macro would have been: > > #define omap_usb_mode_needs_tll(x) ((x) != OMAP_USBHS_PORT_MODE_UNUSED &&\ > (x) != OMAP_EHCI_PORT_MODE_PHY) > > If you're going to use a macro argument in an expression, it needs parens. > Simple expressions like a != b && c != d do _not_ need parens. > Yikes, that was brain dead stuff from me ;). Will fix. cheers, -roger
Hi Roger, On Fri, 18 Jan 2013, Roger Quadros wrote: > We don't need multiple aliases for the OMAP USB host clocks and neither > the dummy clocks so remove them. > > CC: Paul Walmsley <paul@pwsan.com> > CC: Rajendra Nayak <rnayak@ti.com> > CC: Benoit Cousson <b-cousson@ti.com> > CC: Mike Turquette <mturquette@linaro.com> > > Signed-off-by: Roger Quadros <rogerq@ti.com> > Reviewed-by: Felipe Balbi <balbi@ti.com> > Acked-by: Paul Walmsley <paul@pwsan.com> Per Tony's earlier request, you can drop this patch and patch 20 from your series now. I've got them queued for 3.10 or late 3.9 merge window. - Paul
On 01/18/2013 10:27 PM, Paul Walmsley wrote: > Hi Roger, > > On Fri, 18 Jan 2013, Roger Quadros wrote: > >> We don't need multiple aliases for the OMAP USB host clocks and neither >> the dummy clocks so remove them. >> >> CC: Paul Walmsley <paul@pwsan.com> >> CC: Rajendra Nayak <rnayak@ti.com> >> CC: Benoit Cousson <b-cousson@ti.com> >> CC: Mike Turquette <mturquette@linaro.com> >> >> Signed-off-by: Roger Quadros <rogerq@ti.com> >> Reviewed-by: Felipe Balbi <balbi@ti.com> >> Acked-by: Paul Walmsley <paul@pwsan.com> > > Per Tony's earlier request, you can drop this patch and patch 20 from your > series now. I've got them queued for 3.10 or late 3.9 merge window. > Should have mentioned it earlier, but just this patch without the rest of the cleanup patches will break USB Host on OMAP3, as the old driver bails out if optional clock nodes are missing. Including patch 20 doesn't seem to cause a problem with OMAP4 though. regards, -roger
Hi On Mon, 21 Jan 2013, Roger Quadros wrote: > On 01/18/2013 10:27 PM, Paul Walmsley wrote: > > On Fri, 18 Jan 2013, Roger Quadros wrote: > > > >> We don't need multiple aliases for the OMAP USB host clocks and neither > >> the dummy clocks so remove them. > >> > >> CC: Paul Walmsley <paul@pwsan.com> > >> CC: Rajendra Nayak <rnayak@ti.com> > >> CC: Benoit Cousson <b-cousson@ti.com> > >> CC: Mike Turquette <mturquette@linaro.com> > >> > >> Signed-off-by: Roger Quadros <rogerq@ti.com> > >> Reviewed-by: Felipe Balbi <balbi@ti.com> > >> Acked-by: Paul Walmsley <paul@pwsan.com> > > > > Per Tony's earlier request, you can drop this patch and patch 20 from your > > series now. I've got them queued for 3.10 or late 3.9 merge window. > > > > Should have mentioned it earlier, but just this patch without the rest > of the cleanup patches will break USB Host on OMAP3, as the old driver > bails out if optional clock nodes are missing. > > Including patch 20 doesn't seem to cause a problem with OMAP4 though. I've got these two patches queued for merging after your other patches go upstream -- e.g., probably 3.10. Do you foresee any problems with that? - Paul
On Mon, Jan 21, 2013 at 01:04:46PM +0200, Roger Quadros wrote: > Every channel has a functional clock that is similarly named. > It makes sense to use a for loop to manage these clocks as OMAPs > can come with up to 3 channels. > > Dynamically allocate and get channel clocks depending on the > number of clocks avaiable on the platform. > > Signed-off-by: Roger Quadros <rogerq@ti.com> > Reviewed-by: Felipe Balbi <balbi@ti.com> Much better from the clk API usage, thanks. Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>
On 01/21/2013 05:03 PM, Paul Walmsley wrote: > Hi > > On Mon, 21 Jan 2013, Roger Quadros wrote: > >> On 01/18/2013 10:27 PM, Paul Walmsley wrote: >>> On Fri, 18 Jan 2013, Roger Quadros wrote: >>> >>>> We don't need multiple aliases for the OMAP USB host clocks and neither >>>> the dummy clocks so remove them. >>>> >>>> CC: Paul Walmsley <paul@pwsan.com> >>>> CC: Rajendra Nayak <rnayak@ti.com> >>>> CC: Benoit Cousson <b-cousson@ti.com> >>>> CC: Mike Turquette <mturquette@linaro.com> >>>> >>>> Signed-off-by: Roger Quadros <rogerq@ti.com> >>>> Reviewed-by: Felipe Balbi <balbi@ti.com> >>>> Acked-by: Paul Walmsley <paul@pwsan.com> >>> >>> Per Tony's earlier request, you can drop this patch and patch 20 from your >>> series now. I've got them queued for 3.10 or late 3.9 merge window. >>> >> >> Should have mentioned it earlier, but just this patch without the rest >> of the cleanup patches will break USB Host on OMAP3, as the old driver >> bails out if optional clock nodes are missing. >> >> Including patch 20 doesn't seem to cause a problem with OMAP4 though. > > I've got these two patches queued for merging after your other patches go > upstream -- e.g., probably 3.10. Do you foresee any problems with that? > That should be fine. Thanks :). -- cheers, -roger
Hi Paul, On 01/21/2013 05:03 PM, Paul Walmsley wrote: > Hi > > On Mon, 21 Jan 2013, Roger Quadros wrote: > >> On 01/18/2013 10:27 PM, Paul Walmsley wrote: >>> On Fri, 18 Jan 2013, Roger Quadros wrote: >>> >>>> We don't need multiple aliases for the OMAP USB host clocks and neither >>>> the dummy clocks so remove them. >>>> >>>> CC: Paul Walmsley <paul@pwsan.com> >>>> CC: Rajendra Nayak <rnayak@ti.com> >>>> CC: Benoit Cousson <b-cousson@ti.com> >>>> CC: Mike Turquette <mturquette@linaro.com> >>>> >>>> Signed-off-by: Roger Quadros <rogerq@ti.com> >>>> Reviewed-by: Felipe Balbi <balbi@ti.com> >>>> Acked-by: Paul Walmsley <paul@pwsan.com> >>> >>> Per Tony's earlier request, you can drop this patch and patch 20 from your >>> series now. I've got them queued for 3.10 or late 3.9 merge window. >>> >> >> Should have mentioned it earlier, but just this patch without the rest >> of the cleanup patches will break USB Host on OMAP3, as the old driver >> bails out if optional clock nodes are missing. >> >> Including patch 20 doesn't seem to cause a problem with OMAP4 though. > > I've got these two patches queued for merging after your other patches go > upstream -- e.g., probably 3.10. Do you foresee any problems with that? > FYI, the usbhost patches are already in linux-next. cheers, -roger
Hi Roger,
On Tue, 5 Feb 2013, Roger Quadros wrote:
> FYI, the usbhost patches are already in linux-next.
Thanks, I think I'll wait for 3.10 cleanup to send the clock cleanup
patches, they seem non-critical...
- Paul
Hi Roger, On Tue, 5 Feb 2013, Roger Quadros wrote: > On 01/21/2013 05:03 PM, Paul Walmsley wrote: > > Hi > > > > On Mon, 21 Jan 2013, Roger Quadros wrote: > > > >> On 01/18/2013 10:27 PM, Paul Walmsley wrote: > >>> On Fri, 18 Jan 2013, Roger Quadros wrote: > >>> > >>>> We don't need multiple aliases for the OMAP USB host clocks and neither > >>>> the dummy clocks so remove them. > >>>> > >>>> CC: Paul Walmsley <paul@pwsan.com> > >>>> CC: Rajendra Nayak <rnayak@ti.com> > >>>> CC: Benoit Cousson <b-cousson@ti.com> > >>>> CC: Mike Turquette <mturquette@linaro.com> > >>>> > >>>> Signed-off-by: Roger Quadros <rogerq@ti.com> > >>>> Reviewed-by: Felipe Balbi <balbi@ti.com> > >>>> Acked-by: Paul Walmsley <paul@pwsan.com> > >>> > >>> Per Tony's earlier request, you can drop this patch and patch 20 from your > >>> series now. I've got them queued for 3.10 or late 3.9 merge window. > >>> > >> > >> Should have mentioned it earlier, but just this patch without the rest > >> of the cleanup patches will break USB Host on OMAP3, as the old driver > >> bails out if optional clock nodes are missing. > >> > >> Including patch 20 doesn't seem to cause a problem with OMAP4 though. > > > > I've got these two patches queued for merging after your other patches go > > upstream -- e.g., probably 3.10. Do you foresee any problems with that? > > > > FYI, the usbhost patches are already in linux-next. Queued this patch for v3.11; looks like I missed it for v3.10. - Paul
Hi, This patchset addresses the following - Consolidate USB Host platform data. - Avoid addressing clocks one by one by name and use a for loop + bunch of cleanups. - Get number of channels/ports dynamically either from revision register or from platform data. Avoids getting clocks that are not present. - Add OMAP5 and HSIC mode (Not tested). v8: - Re-arranged patch "USB-ehci-omap-Don-t-free-gpios-that-we-didn-t-reques.patch" to be the first since it is a candidate for stable. - Fixed issues raised by Felipe, i.e. indentation and use of switch () v7: - Updated patch 4 to not hold a spinlock when using clk_get(). - Updated patches 3 and 11 to return -EADDRNOTAVAIL on failure of demv_request_and_ioremap(). v6: - Added USB Host platform data consolidation patch as the first patch. based on request from Tony. - Rebased on v3.8-rc3. v5: - Rebased on top of todays arm-soc/for-next. - Removed the clock merging patch from the list. - Updated patches 14, 19 and 20 to accomodate the above change. - Added patch 22 to fix a build warning. v4: - Added appropriate maintainers in to/cc. - minor print message fix in patch 23 to maintain consistency. v3: - Rebased on arm-soc/for-next commit f979306c4d38d213c6977aaf3b1115e8ded71e3a. - Rearranged patch that get rids of cpu_is_omap..() macros. - Coding style fixes. v2: - Clocks are allocated dynamically based on number of ports available on the platform. - Reduced console spam if non critical clocks are not found on the platform. - Get rid of cpu_is_.. macros from USB host driver. cheers, -roger The following changes since commit 9931faca02c604c22335f5a935a501bb2ace6e20: Linux 3.8-rc3 (2013-01-09 18:59:55 -0800) are available in the git repository at: git://github.com/rogerq/linux.git linux-usbhost13-part Roger Quadros (22): USB: ehci-omap: Don't free gpios that we didn't request mfd: omap-usb-host: Consolidate OMAP USB-HS platform data mfd: omap-usb-tll: Fix channel count detection mfd: omap-usb-tll: Use devm_kzalloc/ioremap and clean up error path mfd: omap-usb-tll: Clean up clock handling mfd: omap-usb-tll: introduce and use mode_needs_tll() mfd: omap-usb-tll: Check for missing platform data in probe mfd: omap-usb-tll: Fix error message mfd: omap-usb-tll: serialize access to TLL device mfd: omap-usb-tll: Add OMAP5 revision and HSIC support mfd: omap_usb_host: Avoid missing platform data checks in suspend/resume mfd: omap-usb-host: Use devm_kzalloc() and devm_request_and_ioremap() mfd: omap-usb-host: know about number of ports from revision register mfd: omap-usb-host: override number of ports from platform data mfd: omap-usb-host: cleanup clock management code mfd: omap-usb-host: Manage HSIC clocks for HSIC mode mfd: omap-usb-host: Get rid of unnecessary spinlock mfd: omap-usb-host: clean up omap_usbhs_init() ARM: OMAP3: clock data: get rid of unused USB host clock aliases and dummies ARM: OMAP4: clock data: get rid of unused USB host clock aliases mfd: omap-usb-host: Don't spam console on clk_set_parent failure mdf: omap-usb-host: get rid of build warning arch/arm/mach-omap2/board-3430sdp.c | 2 +- arch/arm/mach-omap2/board-3630sdp.c | 2 +- arch/arm/mach-omap2/board-am3517crane.c | 2 +- arch/arm/mach-omap2/board-am3517evm.c | 2 +- arch/arm/mach-omap2/board-cm-t35.c | 2 +- arch/arm/mach-omap2/board-cm-t3517.c | 2 +- arch/arm/mach-omap2/board-devkit8000.c | 2 +- arch/arm/mach-omap2/board-igep0020.c | 4 +- arch/arm/mach-omap2/board-omap3beagle.c | 2 +- arch/arm/mach-omap2/board-omap3evm.c | 2 +- arch/arm/mach-omap2/board-omap3pandora.c | 2 +- arch/arm/mach-omap2/board-omap3stalker.c | 2 +- arch/arm/mach-omap2/board-omap3touchbook.c | 2 +- arch/arm/mach-omap2/board-omap4panda.c | 2 +- arch/arm/mach-omap2/board-overo.c | 2 +- arch/arm/mach-omap2/board-zoom.c | 2 +- arch/arm/mach-omap2/cclock3xxx_data.c | 11 - arch/arm/mach-omap2/cclock44xx_data.c | 7 - arch/arm/mach-omap2/usb-host.c | 29 +-- arch/arm/mach-omap2/usb.h | 20 +- drivers/mfd/omap-usb-host.c | 546 +++++++++++++++++----------- drivers/mfd/omap-usb-tll.c | 245 +++++++------ drivers/usb/host/ehci-omap.c | 14 +- include/linux/platform_data/usb-omap.h | 24 +- 24 files changed, 491 insertions(+), 439 deletions(-)