[PATCH] usb/hcd: remove unnecessary local_irq_save
Remove the use of local_irq_save() and IRQF_DISABLED, no longer needed since
interrupt handlers are always run with interrupts disabled on the
current CPU.
Tested successfully with 3.12.0-rc4 on my PC. Didn't find
any issue because of this change.
Signed-off-by: Michael Opdenacker
---
drivers/usb/core/hcd.c | 15 ---
1 file changed, 15 deletions(-)
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index d6a8d23..1c309f1 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2324,15 +2324,8 @@ EXPORT_SYMBOL_GPL(usb_bus_start_enum);
irqreturn_t usb_hcd_irq (int irq, void *__hcd)
{
struct usb_hcd *hcd = __hcd;
- unsigned long flags;
irqreturn_t rc;
- /* IRQF_DISABLED doesn't work correctly with shared IRQs
-* when the first handler doesn't use it. So let's just
-* assume it's never used.
-*/
- local_irq_save(flags);
-
if (unlikely(HCD_DEAD(hcd) || !HCD_HW_ACCESSIBLE(hcd)))
rc = IRQ_NONE;
else if (hcd->driver->irq(hcd) == IRQ_NONE)
@@ -2340,7 +2333,6 @@ irqreturn_t usb_hcd_irq (int irq, void *__hcd)
else
rc = IRQ_HANDLED;
- local_irq_restore(flags);
return rc;
}
EXPORT_SYMBOL_GPL(usb_hcd_irq);
@@ -2547,13 +2539,6 @@ static int usb_hcd_request_irqs(struct usb_hcd *hcd,
if (hcd->driver->irq) {
- /* IRQF_DISABLED doesn't work as advertised when used together
-* with IRQF_SHARED. As usb_hcd_irq() will always disable
-* interrupts we can remove it here.
-*/
- if (irqflags & IRQF_SHARED)
- irqflags &= ~IRQF_DISABLED;
-
snprintf(hcd->irq_descr, sizeof(hcd->irq_descr), "%s:usb%d",
hcd->driver->description, hcd->self.busnum);
retval = request_irq(irqnum, &usb_hcd_irq, irqflags,
--
1.8.1.2
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: how to trigger function in usb_device_pm_ops
hi alan: 2013/10/7 Alan Stern : > On Mon, 7 Oct 2013, yoma sophian wrote: > >> >> 1. When will platform register dev_pm_ops be called? Is it called >> >> before or after usb_device_pm_ops ? >> > >> > The platform suspend routine is called after the USB suspend routine. >> > The platform resume routine is called before the USB resume routine. >> I found some driver register >> a. platform_driver->suspend/resume >> instead of >> b. platform_driver->driver->pm->suspend/resume >> >> is there any difference between above a) and b)? > > platform_driver->suspend/resume is deprecated. You should use > driver->pm->suspend/resume for new code. > >> > However, xHCI is fundamentally different from EHCI. A single xHCI >> > controller manages two physically independent buses: a SuperSpeed bus >> > and a full/low/high-speed bus. An EHCI controller, on the other hand, >> > manages only one bus. Therefore the drivers need to be somewhat >> > different. >> Is that the reason why we register 2 HCDs in xhci driver? > > Yes, it is. > >> if so, how driver decide to use which hcd when Super/Non-Super speed >> devices plug in? > > Simple: When a SuperSpeed device plugs in, the driver uses the > SuperSpeed hcd. When a non-SuperSpeed device plugs in, the driver uses > the non-SuperSpeed hcd. :-) in handle_port_status, we use major_revision to choose correct hcd, then find port_array. then I cannot see any benefit to make 2 hcds. if ((major_revision == 0x03) != (hcd->speed == HCD_USB3)) hcd = xhci->shared_hcd; bus_state = &xhci->bus_state[hcd_index(hcd)]; if (hcd->speed == HCD_USB3) port_array = xhci->usb3_ports; else port_array = xhci->usb2_ports; BTW, why in xhci we choose HCD with USB2 as major? > >> If we are doing hibernate for example, why we still need to put >> bus/controller suspend? >> it should be fine if we just reset it and re-initialize it when >> hibernate happen. >> After all, OS is power down in hibernate, right? > > Remember, it's possible that the hibernate transition might fail. If > it does fail, we want the controller to return to the same state it had > before, not to be reset. Yes, you are right. if we just reset controller each time for hibernate transition, that mean controller has no chance back to normal state when hibernate transition is fail. But why don't we just add error handling in xhci_suspend instead of calling bus spend for hibernate transition? Thanks for your kind help, -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: LTE vodafone K5150 (hilink) 12d1 1f16 ; 12d1 1575 cdc_ether mbim?
Am Mittwoch, 9. Oktober 2013, 23:29:49 schrieb Bjørn Mork: > >> will show you a number of interesting debug sites which can be enabled > >> without rebuilding anything. See Documentation/dynamic-debug-howto.txt > >> for details on how to use this. > I played a little bit with it, but there are no much debug messages: [ 1636.302763] cdc_mbim 1-2:2.0: cdc_mbim_manage_power() pmcount=0, on=1 [ 1636.547122] cdc_mbim 1-2:2.0: cdc_mbim_manage_power() pmcount=1, on=0 [ 1636.568796] cdc_mbim 1-2:2.0: cdc_mbim_manage_power() pmcount=0, on=1 [ 1666.741226] cdc_mbim 1-2:2.0: cdc_mbim_manage_power() pmcount=1, on=0 [ 1673.301643] cdc_mbim 1-2:2.0: cdc_mbim_manage_power() pmcount=0, on=1 [ 1673.545124] cdc_mbim 1-2:2.0: cdc_mbim_manage_power() pmcount=1, on=0 [ 1673.557197] cdc_mbim 1-2:2.0: cdc_mbim_manage_power() pmcount=0, on=1 [ 1996.872282] cdc_mbim 1-2:2.0: unknown notification 42 received: index 0 len 8 [ 1996.876271] cdc_mbim 1-2:2.0: unknown notification 2 received: index 0 len 0 [ 2002.287922] cdc_mbim 1-2:2.0: cdc_mbim_manage_power() pmcount=1, on=1 [ 2002.288004] cdc_mbim 1-2:2.0 wwan0: rxqlen 0 --> 2 [ 2193.313287] cdc_mbim 1-2:2.0: cdc_mbim_manage_power() pmcount=2, on=0 [ 2215.728510] cdc_mbim 1-2:2.0: unknown notification 42 received: index 0 len 8 [ 2215.732496] cdc_mbim 1-2:2.0: unknown notification 2 received: index 0 len 0 [ 2221.196734] cdc_mbim 1-2:2.0: cdc_mbim_manage_power() pmcount=1, on=1 [ 2221.196837] cdc_mbim 1-2:2.0 wwan0: rxqlen 0 --> 2 [ 2296.201417] usb 1-2: USB disconnect, device number 3 [ 2296.203103] cdc_mbim 1-2:2.0 wwan0: unregister 'cdc_mbim' usb-:00:1d.7-2, CDC MBIM [ 2296.203213] cdc_mbim 1-2:2.0: cdc_mbim_manage_power() pmcount=2, on=0 Thomas -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: LTE vodafone K5150 (hilink) 12d1 1f16 ; 12d1 1575 cdc_ether mbim?
Thomas Schäfer writes: > Am Samstag, 12. Oktober 2013, 13:46:19 schrieben Sie: >> So the questions still are: Do you see those solicitatins going from the >> driver to the IP layer? And do you see any Neighbor Advertisement >> replies from the IP layer? If the respective answers are yes/no, then >> you should look elsewhere to find the problem. Too restrictive >> ip6tables maybe? > > You asked a lot of questions. I attached two files, captured at wwan0 and at > "any". I see no answers to the NS there, so I assume that's the reason this does not work. But after looking at this again I am not as sure about my conclusions anymore. I do note that the NS is forwarded to the IP layer with an all zeroes MAC source address and the netdev unicast MAC destination address, as expected as those are the dummy values the driver put on any packet. These addresses are of course not correct. Neither of them in fact, as we would expect a multicast destination address here. So these packets could fail if there is anything validation those L2 addresses after the driver is finished. I've looked over and over again on ndisc_rcv() and ndisc_recv_ns() and cannot see any reason why it should fail, though. This is way out of my league... Too bad I don't have a working IPv6 enabled SIM at the moment. I fear this requires a bit of debugging to see where these NS end up. > Only one thing surprises me. There are more than one routeradvertisements. > ( in the past at the qmi devices I have seen it only once per connection) That could be because the firmware resends it until it sees a host address (which could depend on ND success), or just because this firmware is different and send it periodically. That seems more failsafe in any case, and cannot harm. Bjørn -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: how to trigger function in usb_device_pm_ops
On Sun, 13 Oct 2013, yoma sophian wrote: > >> if so, how driver decide to use which hcd when Super/Non-Super speed > >> devices plug in? > > > > Simple: When a SuperSpeed device plugs in, the driver uses the > > SuperSpeed hcd. When a non-SuperSpeed device plugs in, the driver uses > > the non-SuperSpeed hcd. :-) > in handle_port_status, we use major_revision to choose correct hcd, > then find port_array. > then I cannot see any benefit to make 2 hcds. We use two hcd structures because there are two physical buses. > BTW, why in xhci we choose HCD with USB2 as major? Because the low/full/high-speed bus is registered first and the SuperSpeed bus is registered second. I forget the reason why it gets done this way, but there is a good reason. Maybe you can find it in the mailing list archives. Or you can ask the xHCI maintainer. > > Remember, it's possible that the hibernate transition might fail. If > > it does fail, we want the controller to return to the same state it had > > before, not to be reset. > Yes, you are right. > if we just reset controller each time for hibernate transition, that > mean controller has no chance back to normal state when hibernate > transition is fail. > > But why don't we just add error handling in xhci_suspend instead of > calling bus spend for hibernate transition? I don't understand your question. Remember, even for hibernation we want to do an orderly shutdown of the USB buses. This means we should call the bus_suspend routine. Also, it is not guaranteed that the platform will remove all power when the system goes into hibernation. x86 platforms do this, but SoC platforms might not. For this reason, we want to put the controller into low-power mode at the start of hibernation. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb/hcd: remove unnecessary local_irq_save
On Sun, 13 Oct 2013, Michael Opdenacker wrote: > Remove the use of local_irq_save() and IRQF_DISABLED, no longer needed since > interrupt handlers are always run with interrupts disabled on the > current CPU. > > Tested successfully with 3.12.0-rc4 on my PC. Didn't find > any issue because of this change. > > Signed-off-by: Michael Opdenacker > --- > drivers/usb/core/hcd.c | 15 --- > 1 file changed, 15 deletions(-) How about removing all the other unnecessary usages of IRQF_DISABLED under drivers/usb while you're at it? Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: LTE vodafone K5150 (hilink) 12d1 1f16 ; 12d1 1575 cdc_ether mbim?
Bjørn Mork writes:
> I see no answers to the NS there, so I assume that's the reason this
> does not work.
Stupid me. My testing was bogus because I switched on/off the IFF_NOARP
without adding/deleting addresses. Reading the code is better...
We are not supposed to answer these solictiations because we haven't
joined the solicited-node multicast address they are sent to:
void addrconf_join_solict(struct net_device *dev, const struct in6_addr *addr)
{
struct in6_addr maddr;
if (dev->flags&(IFF_LOOPBACK|IFF_NOARP))
return;
addrconf_addr_solict_mult(addr, &maddr);
ipv6_dev_mc_inc(dev, &maddr);
}
So this all works as expected. But the underlying assumptions in the
driver could very well be wrong. Seeing the NS from the firmware was
really surprising to me. I thought it should work anyway, but it seems I
was wrong. IFF_NOARP effectively disables all ND operations. So if we
have to support them, then we probably must add some additional driver
hack.
I am not exactly sure what to do. MAC addresses have no meaning in
MBIM, so IFF_NOARP is really appropriate in my opinion. We could maybe
make the driver recognize solicited-node addresses and blindly replace
them with something else (all-nodes for example). But that's dead
ugly...
Ideas are welcome.
For a start, you could try turning on ndisc_notify on the interface and
see if that is good enough for the device firmware:
sysctl -w net.ipv6.conf.wwan0.ndisc_notify=1
This will make Linux send unsolictied NAs every time an address is added
to the interface. You'll have change the setting before any IPv6
addresses are added. I.e. before connecting and receiving the RA.
Bjørn
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: LTE vodafone K5150 (hilink) 12d1 1f16 ; 12d1 1575 cdc_ether mbim?
Bjørn Mork writes:
> void addrconf_join_solict(struct net_device *dev, const struct in6_addr *addr)
> {
> struct in6_addr maddr;
>
> if (dev->flags&(IFF_LOOPBACK|IFF_NOARP))
> return;
>
> addrconf_addr_solict_mult(addr, &maddr);
> ipv6_dev_mc_inc(dev, &maddr);
> }
>
>
> So this all works as expected. But the underlying assumptions in the
> driver could very well be wrong. Seeing the NS from the firmware was
> really surprising to me. I thought it should work anyway, but it seems I
> was wrong. IFF_NOARP effectively disables all ND operations. So if we
> have to support them, then we probably must add some additional driver
> hack.
>
> I am not exactly sure what to do. MAC addresses have no meaning in
> MBIM, so IFF_NOARP is really appropriate in my opinion. We could maybe
> make the driver recognize solicited-node addresses and blindly replace
> them with something else (all-nodes for example). But that's dead
> ugly...
Hmm, reading RFC4861 I am wondering if the above dependency on IFF_NOARP
is allowed:
7.2.1. Interface Initialization
When a multicast-capable interface becomes enabled, the node MUST
join the all-nodes multicast address on that interface, as well as
the solicited-node multicast address corresponding to each of the IP
addresses assigned to the interface.
I'm going to test a patch on the netdev people to see if I have
misunderstood this completely...
Bjørn
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH V3] USB: ohci-exynos: Remove non-DT support
The non-DT for EXYNOS SoCs is not supported from v3.11.
Thus, there is no need to support non-DT for Exynos OHCI driver.
The 'include/linux/platform_data/usb-ohci-exynos.h' file has been
used for non-DT support. Thus, the 'usb-ohci-exynos.h' file can
be removed.
Signed-off-by: Jingoo Han
---
- re-based against the latest 'usb-next' branch.
drivers/usb/host/ohci-exynos.c| 18 +++---
include/linux/platform_data/usb-ohci-exynos.h | 21 -
2 files changed, 3 insertions(+), 36 deletions(-)
delete mode 100644 include/linux/platform_data/usb-ohci-exynos.h
diff --git a/drivers/usb/host/ohci-exynos.c b/drivers/usb/host/ohci-exynos.c
index 122e52e..aa50e18 100644
--- a/drivers/usb/host/ohci-exynos.c
+++ b/drivers/usb/host/ohci-exynos.c
@@ -18,7 +18,6 @@
#include
#include
#include
-#include
#include
#include
#include
@@ -38,7 +37,6 @@ struct exynos_ohci_hcd {
struct clk *clk;
struct usb_phy *phy;
struct usb_otg *otg;
- struct exynos4_ohci_platdata *pdata;
};
static void exynos_ohci_phy_enable(struct platform_device *pdev)
@@ -48,8 +46,6 @@ static void exynos_ohci_phy_enable(struct platform_device
*pdev)
if (exynos_ohci->phy)
usb_phy_init(exynos_ohci->phy);
- else if (exynos_ohci->pdata && exynos_ohci->pdata->phy_init)
- exynos_ohci->pdata->phy_init(pdev, USB_PHY_TYPE_HOST);
}
static void exynos_ohci_phy_disable(struct platform_device *pdev)
@@ -59,13 +55,10 @@ static void exynos_ohci_phy_disable(struct platform_device
*pdev)
if (exynos_ohci->phy)
usb_phy_shutdown(exynos_ohci->phy);
- else if (exynos_ohci->pdata && exynos_ohci->pdata->phy_exit)
- exynos_ohci->pdata->phy_exit(pdev, USB_PHY_TYPE_HOST);
}
static int exynos_ohci_probe(struct platform_device *pdev)
{
- struct exynos4_ohci_platdata *pdata = dev_get_platdata(&pdev->dev);
struct exynos_ohci_hcd *exynos_ohci;
struct usb_hcd *hcd;
struct resource *res;
@@ -98,14 +91,9 @@ static int exynos_ohci_probe(struct platform_device *pdev)
phy = devm_usb_get_phy(&pdev->dev, USB_PHY_TYPE_USB2);
if (IS_ERR(phy)) {
- /* Fallback to pdata */
- if (!pdata) {
- usb_put_hcd(hcd);
- dev_warn(&pdev->dev, "no platform data or transceiver
defined\n");
- return -EPROBE_DEFER;
- } else {
- exynos_ohci->pdata = pdata;
- }
+ usb_put_hcd(hcd);
+ dev_warn(&pdev->dev, "no platform data or transceiver
defined\n");
+ return -EPROBE_DEFER;
} else {
exynos_ohci->phy = phy;
exynos_ohci->otg = phy->otg;
diff --git a/include/linux/platform_data/usb-ohci-exynos.h
b/include/linux/platform_data/usb-ohci-exynos.h
deleted file mode 100644
index c256c59..000
--- a/include/linux/platform_data/usb-ohci-exynos.h
+++ /dev/null
@@ -1,21 +0,0 @@
-/*
- * Copyright (C) 2011 Samsung Electronics Co.Ltd
- * http://www.samsung.com/
- *
- * This program is free software; you can redistribute it and/or modify it
- * under the terms of the GNU General Public License as published by the
- * Free Software Foundation; either version 2 of the License, or (at your
- * option) any later version.
- */
-
-#ifndef __MACH_EXYNOS_OHCI_H
-#define __MACH_EXYNOS_OHCI_H
-
-struct exynos4_ohci_platdata {
- int (*phy_init)(struct platform_device *pdev, int type);
- int (*phy_exit)(struct platform_device *pdev, int type);
-};
-
-extern void exynos4_ohci_set_platdata(struct exynos4_ohci_platdata *pd);
-
-#endif /* __MACH_EXYNOS_OHCI_H */
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 10/11] usb: chipidea: imx: add binding for supporting runtime pm
On Sat, Oct 12, 2013 at 10:40:37AM -0400, Alan Stern wrote:
> On Sat, 12 Oct 2013, Peter Chen wrote:
>
> > Add property for supporting runtime power management
> >
> > Signed-off-by: Peter Chen
> > ---
> > .../devicetree/bindings/usb/ci13xxx-imx.txt|2 ++
> > 1 files changed, 2 insertions(+), 0 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> > b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> > index b4b5b79..f666598 100644
> > --- a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> > +++ b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> > @@ -18,6 +18,7 @@ Optional properties:
> > - vbus-supply: regulator for vbus
> > - disable-over-current: disable over current detect
> > - external-vbus-divider: enables off-chip resistor divider for Vbus
> > +- supports_runtime_pm: enable runtime pm support
> >
> > Examples:
> > usb@02184000 { /* USB OTG */
> > @@ -28,4 +29,5 @@ usb@02184000 { /* USB OTG */
> > fsl,usbmisc = <&usbmisc 0>;
> > disable-over-current;
> > external-vbus-divider;
> > + supports_runtime_pm;
> > };
>
> This does not sound like a property of the hardware. What's the
> _hardware_ difference between parts that support runtime PM and parts
> that don't?
Thanks.
>From my point, all hardware using chipidea core should support runtime pm.
But some of platforms need special glue layer operations to support
it, it will break other platforms if enable chipidea core runtime pm.
Since device tree describes hardware property, maybe I should move
it to glue layer, or do you have any suggestions?
--
Best Regards,
Peter Chen
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 10/11] usb: chipidea: imx: add binding for supporting runtime pm
Dear Peter Chen,
> On Sat, Oct 12, 2013 at 10:40:37AM -0400, Alan Stern wrote:
> > On Sat, 12 Oct 2013, Peter Chen wrote:
> > > Add property for supporting runtime power management
> > >
> > > Signed-off-by: Peter Chen
> > > ---
> > >
> > > .../devicetree/bindings/usb/ci13xxx-imx.txt|2 ++
> > > 1 files changed, 2 insertions(+), 0 deletions(-)
> > >
> > > diff --git a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> > > b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt index
> > > b4b5b79..f666598 100644
> > > --- a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> > > +++ b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> > >
> > > @@ -18,6 +18,7 @@ Optional properties:
> > > - vbus-supply: regulator for vbus
> > > - disable-over-current: disable over current detect
> > > - external-vbus-divider: enables off-chip resistor divider for Vbus
> > >
> > > +- supports_runtime_pm: enable runtime pm support
> > >
> > > Examples:
> > > usb@02184000 { /* USB OTG */
> > >
> > > @@ -28,4 +29,5 @@ usb@02184000 { /* USB OTG */
> > >
> > > fsl,usbmisc = <&usbmisc 0>;
> > > disable-over-current;
> > > external-vbus-divider;
> > >
> > > + supports_runtime_pm;
> > >
> > > };
> >
> > This does not sound like a property of the hardware. What's the
> > _hardware_ difference between parts that support runtime PM and parts
> > that don't?
>
> Thanks.
>
> From my point, all hardware using chipidea core should support runtime pm.
> But some of platforms need special glue layer operations to support
> it, it will break other platforms if enable chipidea core runtime pm.
> Since device tree describes hardware property, maybe I should move
> it to glue layer, or do you have any suggestions?
You should certainly move it out of DT. This is linux-specific property, it has
nothing to do with HW. The best course of action would be to fix those
platforms
that are broken by runtime PM.
Best regards,
Marek Vasut
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 02/12] usb: phy-mxs: Enable IC fixes for mx6 SoC serial
On Sat, Oct 12, 2013 at 11:38:16AM +0200, Marek Vasut wrote: > Hi, > > > After adding IC fixes bits, some PHY bugs are fixed by > > IC logic. > > Can you please elaborate what those bits do exactly ? They seem like a magic > stuff to me thus far, which is not exactly helpful . I can't find them in the > datasheet either. > Yes, these bits are added at late TO verion for i.mx 6, and these TO versions will be for mass production, unfortunately, the related doc update may be forgotten. These two bits are related to two PHY bugs, two PHY bugs are still existed at mx28 and mx23, one bug is fixed at mx6dq and mx6dl, and both of two bugs are fixed at later mx6 (like mx6sololite and later SoCs), but the IC fixes are not enabled by default, it needs software opens it. -- Best Regards, Peter Chen -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 10/11] usb: chipidea: imx: add binding for supporting runtime pm
On Mon, Oct 14, 2013 at 03:39:36AM +0200, Marek Vasut wrote:
> Dear Peter Chen,
>
> > On Sat, Oct 12, 2013 at 10:40:37AM -0400, Alan Stern wrote:
> > > On Sat, 12 Oct 2013, Peter Chen wrote:
> > > > Add property for supporting runtime power management
> > > >
> > > > Signed-off-by: Peter Chen
> > > > ---
> > > >
> > > > .../devicetree/bindings/usb/ci13xxx-imx.txt|2 ++
> > > > 1 files changed, 2 insertions(+), 0 deletions(-)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> > > > b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt index
> > > > b4b5b79..f666598 100644
> > > > --- a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> > > > +++ b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> > > >
> > > > @@ -18,6 +18,7 @@ Optional properties:
> > > > - vbus-supply: regulator for vbus
> > > > - disable-over-current: disable over current detect
> > > > - external-vbus-divider: enables off-chip resistor divider for Vbus
> > > >
> > > > +- supports_runtime_pm: enable runtime pm support
> > > >
> > > > Examples:
> > > > usb@02184000 { /* USB OTG */
> > > >
> > > > @@ -28,4 +29,5 @@ usb@02184000 { /* USB OTG */
> > > >
> > > > fsl,usbmisc = <&usbmisc 0>;
> > > > disable-over-current;
> > > > external-vbus-divider;
> > > >
> > > > + supports_runtime_pm;
> > > >
> > > > };
> > >
> > > This does not sound like a property of the hardware. What's the
> > > _hardware_ difference between parts that support runtime PM and parts
> > > that don't?
> >
> > Thanks.
> >
> > From my point, all hardware using chipidea core should support runtime pm.
> > But some of platforms need special glue layer operations to support
> > it, it will break other platforms if enable chipidea core runtime pm.
> > Since device tree describes hardware property, maybe I should move
> > it to glue layer, or do you have any suggestions?
>
> You should certainly move it out of DT. This is linux-specific property, it
> has
> nothing to do with HW. The best course of action would be to fix those
> platforms
> that are broken by runtime PM.
>
Thanks, I move it out of DT. But I can only verify it at i.mx platform,
I will add a flag at glue layer.
--
Best Regards,
Peter Chen
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 05/12] usb: phy-mxs: Add implementation of nofity_suspend and notify_resume
On Sat, Oct 12, 2013 at 11:42:06AM +0200, Marek Vasut wrote:
> Dear Peter Chen,
>
> > Add notify_suspend and notify_resume according to different SoCs.
> >
> > Signed-off-by: Peter Chen
> > ---
> > drivers/usb/phy/phy-mxs-usb.c | 73
> > + 1 files changed, 73
> > insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/usb/phy/phy-mxs-usb.c b/drivers/usb/phy/phy-mxs-usb.c
> > index e0b0de0..8661dae 100644
> > --- a/drivers/usb/phy/phy-mxs-usb.c
> > +++ b/drivers/usb/phy/phy-mxs-usb.c
> > @@ -197,6 +197,78 @@ static int mxs_phy_on_disconnect(struct usb_phy *phy,
> > return 0;
> > }
> >
> > +static int mxs_phy_on_suspend_workaround(struct usb_phy *phy,
> > + enum usb_device_speed speed)
> > +{
> > + dev_dbg(phy->dev, "%s speed device has suspended\n",
> > + (speed == USB_SPEED_HIGH) ? "high" : "non-high");
>
> HS : FS/LS you mean?
Yes, it is what I mean.
OK, I will change to HS and FS/LS.
>
> > +/*
> > + * For mxs PHY, there are two PHY issues related to suspend/resume.
> > + * For mx23 and mx28, both of two issues are existed.
> > + * For mx6q and mx6dl, only one issue is existed.
> > + * For mx6 sololite, none issue is existed.
> > + */
> > +static void mxs_phy_workaround(struct mxs_phy *mxs_phy)
> > +{
> > + if (is_mx23_phy(mxs_phy)) {
>
> This is_mx23_phy() returns 1 for mx28 too? It's not too clear, not even from
> the
> comment above, a short comment here would help for sure.
mx23 and mx28 PHY are the same PHY, so the fixes are the same.
Anything I need to add?
--
Best Regards,
Peter Chen
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 07/12] usb: phy-mxs: Add implementation of set_wakeup
On Sat, Oct 12, 2013 at 11:44:59AM +0200, Marek Vasut wrote:
>
> > +static int mxs_phy_set_wakeup(struct usb_phy *x, bool enabled)
> > +{
> > + struct mxs_phy *mxs_phy = to_mxs_phy(x);
> > + u32 value = BM_USBPHY_CTRL_ENVBUSCHG_WKUP |
> > + BM_USBPHY_CTRL_ENDPDMCHG_WKUP |
> > + BM_USBPHY_CTRL_ENIDCHG_WKUP;
>
> Does this stuff pass checkpatch at all? I mean, this alignment seems a bit
> strange.
Yes, it passed.
u32 value = BM_USBPHY_CTRL_ENVBUSCHG_WKUP |
BM_USBPHY_CTRL_ENDPDMCHG_WKUP | /* two tabs for last
line */
BM_USBPHY_CTRL_ENIDCHG_WKUP; /* one tab for
last line */
Any better suggestions?
>
> > + if (enabled) {
> > + mxs_phy_disconnect_line(mxs_phy, true);
> > + writel_relaxed(value, x->io_priv + HW_USBPHY_CTRL_SET);
> > + } else {
> > + writel_relaxed(value, x->io_priv + HW_USBPHY_CTRL_CLR);
> > + mxs_phy_disconnect_line(mxs_phy, false);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static int mxs_phy_on_connect(struct usb_phy *phy,
> > enum usb_device_speed speed)
> > {
> > @@ -315,6 +390,10 @@ static int mxs_phy_probe(struct platform_device *pdev)
> > }
> > }
> >
> > + if (of_find_property(np, "disconnect_line_without_vbus", NULL) &&
> > + mxs_phy->regmap_anatop)
>
> You might want to introduce a variable here to make the condition shorter:
>
> var = of_find
> if (var && mxs_phy->...)
I will change.
--
Best Regards,
Peter Chen
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 11/12] usb: phy-mxs: update binding for adding disconnect line property
On Sat, Oct 12, 2013 at 11:47:06AM +0200, Marek Vasut wrote: > Dear Peter Chen, > > > This property is used to disconnect line between USB PHY and > > USB controller. > > > > Signed-off-by: Peter Chen > > --- > > Documentation/devicetree/bindings/usb/mxs-phy.txt |4 > > 1 files changed, 4 insertions(+), 0 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/usb/mxs-phy.txt > > b/Documentation/devicetree/bindings/usb/mxs-phy.txt index 1a9bd85..099b0bb > > 100644 > > --- a/Documentation/devicetree/bindings/usb/mxs-phy.txt > > +++ b/Documentation/devicetree/bindings/usb/mxs-phy.txt > > @@ -5,6 +5,9 @@ Required properties: > > - reg: Should contain registers location and length > > - interrupts: Should contain phy interrupt > > - fsl,anatop: phandle for anatop register > > +- disconnect_line_without_vbus: needs to disconnect > > +connection between USB PHY and controller, it can avoid > > +unexpected wakeup interrupt when the PHY is out of power > > Uh oh, this might needs some rewording. I didn't understand the reason for > this > prop before I checked the 12/12 patch. > So, the better sequence like below, correct? Binding doc patch source code change patch dts file patch -- Best Regards, Peter Chen -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 11/12] usb: phy-mxs: update binding for adding disconnect line property
On Sat, Oct 12, 2013 at 05:05:23PM +0200, Thomas Petazzoni wrote:
> Dear Peter Chen,
>
> On Sat, 12 Oct 2013 17:09:45 +0800, Peter Chen wrote:
> > This property is used to disconnect line between USB PHY and
> > USB controller.
> >
> > Signed-off-by: Peter Chen
> > ---
> > Documentation/devicetree/bindings/usb/mxs-phy.txt |4
> > 1 files changed, 4 insertions(+), 0 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/usb/mxs-phy.txt
> > b/Documentation/devicetree/bindings/usb/mxs-phy.txt
> > index 1a9bd85..099b0bb 100644
> > --- a/Documentation/devicetree/bindings/usb/mxs-phy.txt
> > +++ b/Documentation/devicetree/bindings/usb/mxs-phy.txt
> > @@ -5,6 +5,9 @@ Required properties:
> > - reg: Should contain registers location and length
> > - interrupts: Should contain phy interrupt
> > - fsl,anatop: phandle for anatop register
> > +- disconnect_line_without_vbus: needs to disconnect
> > +connection between USB PHY and controller, it can avoid
> > +unexpected wakeup interrupt when the PHY is out of power
> >
> > Example:
> > usbphy1: usbphy@020c9000 {
> > @@ -12,4 +15,5 @@ usbphy1: usbphy@020c9000 {
> > reg = <0x020c9000 0x1000>;
> > interrupts = <0 44 0x04>;
> > fsl,anatop = <&anatop>;
> > + disconnect_line_without_vbus;
> > };
>
> Device Tree properties use "-" as a separator, not "_". So, it should
> be:
>
> disconnect-line-without-vbus
>
> Also, all your patches touching Device Tree bindings should be Cc'ed to
> the devicetree@ mailing list.
>
Thanks, I will change, and cc DT ML and maintainer, thanks.
--
Best Regards,
Peter Chen
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 02/12] usb: phy-mxs: Enable IC fixes for mx6 SoC serial
Dear Peter Chen, > On Sat, Oct 12, 2013 at 11:38:16AM +0200, Marek Vasut wrote: > > Hi, > > > > > After adding IC fixes bits, some PHY bugs are fixed by > > > IC logic. > > > > Can you please elaborate what those bits do exactly ? They seem like a > > magic stuff to me thus far, which is not exactly helpful . I can't find > > them in the datasheet either. > > Yes, these bits are added at late TO verion for i.mx 6, and these TO > versions will be for mass production, unfortunately, the related doc > update may be forgotten. > > These two bits are related to two PHY bugs, two PHY bugs are still existed > at mx28 and mx23, one bug is fixed at mx6dq and mx6dl, and both of two > bugs are fixed at later mx6 (like mx6sololite and later SoCs), but the IC > fixes are not enabled by default, it needs software opens it. Sure, I get it. But what exactly does that bit do? Can you add a proper (and likely beefy) comment into the code to supplement the missing parts in the datasheet? Best regards, Marek Vasut -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 05/12] usb: phy-mxs: Add implementation of nofity_suspend and notify_resume
Dear Peter Chen,
> On Sat, Oct 12, 2013 at 11:42:06AM +0200, Marek Vasut wrote:
> > Dear Peter Chen,
> >
> > > Add notify_suspend and notify_resume according to different SoCs.
> > >
> > > Signed-off-by: Peter Chen
> > > ---
> > >
> > > drivers/usb/phy/phy-mxs-usb.c | 73
> > >
> > > + 1 files changed, 73
> > > insertions(+), 0 deletions(-)
> > >
> > > diff --git a/drivers/usb/phy/phy-mxs-usb.c
> > > b/drivers/usb/phy/phy-mxs-usb.c index e0b0de0..8661dae 100644
> > > --- a/drivers/usb/phy/phy-mxs-usb.c
> > > +++ b/drivers/usb/phy/phy-mxs-usb.c
> > > @@ -197,6 +197,78 @@ static int mxs_phy_on_disconnect(struct usb_phy
> > > *phy,
> > >
> > > return 0;
> > >
> > > }
> > >
> > > +static int mxs_phy_on_suspend_workaround(struct usb_phy *phy,
> > > + enum usb_device_speed speed)
> > > +{
> > > + dev_dbg(phy->dev, "%s speed device has suspended\n",
> > > + (speed == USB_SPEED_HIGH) ? "high" : "non-high");
> >
> > HS : FS/LS you mean?
>
> Yes, it is what I mean.
> OK, I will change to HS and FS/LS.
>
> > > +/*
> > > + * For mxs PHY, there are two PHY issues related to suspend/resume.
> > > + * For mx23 and mx28, both of two issues are existed.
> > > + * For mx6q and mx6dl, only one issue is existed.
> > > + * For mx6 sololite, none issue is existed.
> > > + */
> > > +static void mxs_phy_workaround(struct mxs_phy *mxs_phy)
> > > +{
> > > + if (is_mx23_phy(mxs_phy)) {
> >
> > This is_mx23_phy() returns 1 for mx28 too? It's not too clear, not even
> > from the comment above, a short comment here would help for sure.
>
> mx23 and mx28 PHY are the same PHY, so the fixes are the same.
> Anything I need to add?
I believe it's OK. But identifying MX28 PHY via is_mx23_phy() might be a little
confusing. I'm just trying to keep this driver flushed out of my brain here and
do an unbiased review ;-)
Best regards,
Marek Vasut
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 02/12] usb: phy-mxs: Enable IC fixes for mx6 SoC serial
On Mon, Oct 14, 2013 at 04:07:10AM +0200, Marek Vasut wrote: > Dear Peter Chen, > > > On Sat, Oct 12, 2013 at 11:38:16AM +0200, Marek Vasut wrote: > > > Hi, > > > > > > > After adding IC fixes bits, some PHY bugs are fixed by > > > > IC logic. > > > > > > Can you please elaborate what those bits do exactly ? They seem like a > > > magic stuff to me thus far, which is not exactly helpful . I can't find > > > them in the datasheet either. > > > > Yes, these bits are added at late TO verion for i.mx 6, and these TO > > versions will be for mass production, unfortunately, the related doc > > update may be forgotten. > > > > These two bits are related to two PHY bugs, two PHY bugs are still existed > > at mx28 and mx23, one bug is fixed at mx6dq and mx6dl, and both of two > > bugs are fixed at later mx6 (like mx6sololite and later SoCs), but the IC > > fixes are not enabled by default, it needs software opens it. > > Sure, I get it. But what exactly does that bit do? Can you add a proper (and > likely beefy) comment into the code to supplement the missing parts in the > datasheet? OK, I will add them at v2. -- Best Regards, Peter Chen -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb/hcd: remove unnecessary local_irq_save
Hi Alan,
On 10/13/2013 05:35 PM, Alan Stern wrote:
> On Sun, 13 Oct 2013, Michael Opdenacker wrote:
>
>> Remove the use of local_irq_save() and IRQF_DISABLED, no longer needed since
>> interrupt handlers are always run with interrupts disabled on the
>> current CPU.
>>
>> Tested successfully with 3.12.0-rc4 on my PC. Didn't find
>> any issue because of this change.
>>
>> Signed-off-by: Michael Opdenacker
>> ---
>> drivers/usb/core/hcd.c | 15 ---
>> 1 file changed, 15 deletions(-)
> How about removing all the other unnecessary usages of IRQF_DISABLED
> under drivers/usb while you're at it?
I believe I already did, through patches sent on Oct. 6:
drivers/usb/gadget/mv_u3d_core.c: IRQF_DISABLED |
IRQF_SHARED, driver_name, u3d)) {
=> Sent fix on Oct. 6, 2013
drivers/usb/host/uhci-platform.c: ret = usb_add_hcd(hcd,
pdev->resource[1].start, IRQF_DISABLED |
=> Sent fix on Oct. 6, 2013
Cheers,
Michael.
--
Michael Opdenacker, CEO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
+33 484 258 098
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
