Re: How should we handle isochronous underruns?
On Sun, Jul 21, 2013 at 2:00 AM, Alan Stern st...@rowland.harvard.edu wrote: On Sat, 20 Jul 2013, Ming Lei wrote: No, we don't have to change every driver using isochronous URBs. Many of them set URB_ISO_ASAP on every URB -- in fact, AFAIK snd-usb-audio is the only one that has been fixed not to do this. OK, if you are sure just snd-usb-audio and very less drivers need the change, using the API only is correct way. I haven't checked in detail, but this seems very likely. For ehci example, URB_ISO_ASAP is not checked when isoc queue is empty, so will cause a new schedule when underrun happens. But never mind. For a new API, how about void usb_new_iso_stream(struct usb_device *, struct usb_host_endpoint *); Does that look okay? Looks you missed one parameter of 'int on'. I don't understand. Why is another parameter needed? So do you only want to set streaming on and not streaming off? If so, that looks a bit weird, could you describe its usage? The new call doesn't turn streaming on or off. The driver does that by submitting URBs or not submitting them. The new call merely separates an old stream from a new one. It tells the HCD that the old stream (if any) is finished, so that any URBs submitted in the future will belong to a new stream. In particular, it tells the HCD that the next URB for this endpoint should be assigned to a time slot that will be visible to the hardware, instead of being assigned to the next time slot after the last packet of the previous URB (which has probably expired already). OK, I got it, looks usb_new_iso_stream(udev, ep) need to be called before starting a new stream. From view of HCD, the hcd only knows a stream has been started, and doesn't know if it has been stopped. And the most possible usage for hcd is that: if (test_and_clear_bit(ISOC_NEW_STREAM, ep-stream_flag) ; /* new stream */ So looks hcd has to write the flag. If we define the API as usb_iso_stream(udev, ep, start), hcd only need to read the flag, so it should be simper to use and implement. Also with both streaming on and off information, the HCD might improve bandwidth management in the future. In short, it would simply tell the HCD to act as though the next URB has URB_ISO_ASAP set. For HCDs that act as though URB_ISO_ASAP was always set, the new call wouldn't do anything -- they would not need to implement it. (with extra overhead for every URB!) to fix a problem that ought to be In a normal system, the URB count won't be very much( 1000), and if you mean the overhead is from memory, I guess it won't cause real memory increase per URB for slab's sake. If you mean performance loss with set/get the single lockless/common variable, maybe we should focus on the big HCD lock, which might be the biggest bottle of USB protocol, :-) If you're talking about the hcd_urb_list_lock, note that it could easily be made hcd-specific. I haven't done that because the regions it protects are all very short, so there probably isn't very much contention. I mean the HCD private lock, and there are heavy contention on the lock. Also I thought of one improvement on the idea of 'urb-completing': - define one percpu 'struct urb *' variable inside 'struct hcd' to record the completing urb, so HCD can see easily if the urb is being resubmitted inside its completion handler. Anyway this approach is only helpful if much lots of drivers need changes for underrun case. Thanks, -- Ming Lei -- 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][usbutils] lsusb: port to hwdb
On Sun, Jul 21, 2013 at 3:34 AM, Greg KH gre...@linuxfoundation.org wrote: Can this mean I can drop the usb.ids file from the usbutils package? I can't remember where hwdb is generated from, does it rely on the usb.ids file for the initial creation? hwdb does not use the usb.ids from the usbutils package. However, hwdb only contains vendor, product, class, subclass and protocol. So if you drop usb.ids the rest of the information will be lost. Maybe split the rest out into a separate file and ship only that? Or is there a way to get this info into hwdb? Kay? Also, I just realise there is also lsusb,py, which I did not port. What is the usecase for this? Is it also worth porting over? Cheers, Tom -- 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 01/15] drivers: phy: add generic PHY framework
On Sat, Jul 20, 2013 at 07:59:10PM -0700, Greg KH wrote: On Sat, Jul 20, 2013 at 10:32:26PM -0400, Alan Stern wrote: On Sat, 20 Jul 2013, Greg KH wrote: That should be passed using platform data. Ick, don't pass strings around, pass pointers. If you have platform data you can get to, then put the pointer there, don't use a name. I don't think I understood you here :-s We wont have phy pointer when we create the device for the controller no?(it'll be done in board file). Probably I'm missing something. Why will you not have that pointer? You can't rely on the name as the device id will not match up, so you should be able to rely on the pointer being in the structure that the board sets up, right? Don't use names, especially as ids can, and will, change, that is going to cause big problems. Use pointers, this is C, we are supposed to be doing that :) Kishon, I think what Greg means is this: The name you are using must be stored somewhere in a data structure constructed by the board file, right? Or at least, associated with some data structure somehow. Otherwise the platform code wouldn't know which PHY hardware corresponded to a particular name. Greg's suggestion is that you store the address of that data structure in the platform data instead of storing the name string. Have the consumer pass the data structure's address when it calls phy_create, instead of passing the name. Then you don't have to worry about two PHYs accidentally ending up with the same name or any other similar problems. Close, but the issue is that whatever returns from phy_create() should then be used, no need to call any find functions, as you can just use the pointer that phy_create() returns. Much like all other class api functions in the kernel work. I think the problem here is to connect two from the bus structure completely independent devices. Several frameworks (ASoC, soc-camera) had this problem and this wasn't solved until the advent of devicetrees and their phandles. phy_create might be called from the probe function of some i2c device (the phy device) and the resulting pointer is then needed in some other platform devices (the user of the phy) probe function. The best solution we have right now is implemented in the clk framework which uses a string matching of the device names in clk_get() (at least in the non-dt case). Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | -- 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 01/15] drivers: phy: add generic PHY framework
Hi,
On Saturday 20 of July 2013 19:59:10 Greg KH wrote:
On Sat, Jul 20, 2013 at 10:32:26PM -0400, Alan Stern wrote:
On Sat, 20 Jul 2013, Greg KH wrote:
That should be passed using platform data.
Ick, don't pass strings around, pass pointers. If you have
platform
data you can get to, then put the pointer there, don't use a
name.
I don't think I understood you here :-s We wont have phy pointer
when we create the device for the controller no?(it'll be done in
board file). Probably I'm missing something.
Why will you not have that pointer? You can't rely on the name as
the device id will not match up, so you should be able to rely on
the pointer being in the structure that the board sets up, right?
Don't use names, especially as ids can, and will, change, that is
going
to cause big problems. Use pointers, this is C, we are supposed to
be
doing that :)
Kishon, I think what Greg means is this: The name you are using must
be stored somewhere in a data structure constructed by the board file,
right? Or at least, associated with some data structure somehow.
Otherwise the platform code wouldn't know which PHY hardware
corresponded to a particular name.
Greg's suggestion is that you store the address of that data structure
in the platform data instead of storing the name string. Have the
consumer pass the data structure's address when it calls phy_create,
instead of passing the name. Then you don't have to worry about two
PHYs accidentally ending up with the same name or any other similar
problems.
Close, but the issue is that whatever returns from phy_create() should
then be used, no need to call any find functions, as you can just use
the pointer that phy_create() returns. Much like all other class api
functions in the kernel work.
I think there is a confusion here about who registers the PHYs.
All platform code does is registering a platform/i2c/whatever device,
which causes a driver (located in drivers/phy/) to be instantiated. Such
drivers call phy_create(), usually in their probe() callbacks, so
platform_code has no way (and should have no way, for the sake of
layering) to get what phy_create() returns.
IMHO we need a lookup method for PHYs, just like for clocks, regulators,
PWMs or even i2c busses because there are complex cases when passing just
a name using platform data will not work. I would second what Stephen said
[1] and define a structure doing things in a DT-like way.
Example;
[platform code]
static const struct phy_lookup my_phy_lookup[] = {
PHY_LOOKUP(s3c-hsotg.0, otg, samsung-usbphy.1, phy.2),
/* etc... */
};
static void my_machine_init(void)
{
/* ... */
phy_register_lookup(my_phy_lookup, ARRAY_SIZE(my_phy_lookup));
/* ... */
}
/* Notice nothing stuffed in platform data. */
[provider code - samsung-usbphy driver]
static int samsung_usbphy_probe(...)
{
/* ... */
for (i = 0; i PHY_COUNT; ++i) {
usbphy-phy[i].name = phy;
usbphy-phy[i].id = i;
/* ... */
ret = phy_create(usbphy-phy);
/* err handling */
}
/* ... */
}
[consumer code - s3c-hsotg driver]
static int s3c_hsotg_probe(...)
{
/* ... */
phy = phy_get(pdev-dev, otg);
/* ... */
}
[1] http://thread.gmane.org/gmane.linux.ports.arm.kernel/252813
Best regards,
Tomasz
--
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][usbutils] lsusb: port to hwdb
On Sun, Jul 21, 2013 at 12:01 PM, Tom Gundersen t...@jklm.no wrote: On Sun, Jul 21, 2013 at 3:34 AM, Greg KH gre...@linuxfoundation.org wrote: Can this mean I can drop the usb.ids file from the usbutils package? I can't remember where hwdb is generated from, does it rely on the usb.ids file for the initial creation? hwdb does not use the usb.ids from the usbutils package. We download the usb.ids file from: http://www.linux-usb.org/usb.ids and convert them into *.hwdb files, which end up in the binary hwdb udev is using. Shipping a copy of it in the usbutils package makes probably no sense even today before using the hwdb data from udev. Almost all distros ship the downloaded file, which is regularly updated and ignore the one from usbutils. However, hwdb only contains vendor, product, class, subclass and protocol. So if you drop usb.ids the rest of the information will be lost. Maybe split the rest out into a separate file and ship only that? Or is there a way to get this info into hwdb? Kay? It should work to add some of that data to the existing modalias, right? For some things we probably need to invent new synthetic modaliases to query these strings. We should give it a try, I think. Having lsusb shipping a private file only for that seems ugly. Also, I just realise there is also lsusb,py, which I did not port. What is the usecase for this? Is it also worth porting over? I think this redundancy is just confusing and should be sorted out. If the output mode is more useful than the one from lsusb, it probably should be added to the C program. The .py version has no man page, and new system commands ending in language suffixes just don't look right. I think that should be sorted out and only one of them should exist in the end. Kay -- 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 01/15] drivers: phy: add generic PHY framework
Hi,
On Sunday 21 July 2013 04:01 PM, Tomasz Figa wrote:
Hi,
On Saturday 20 of July 2013 19:59:10 Greg KH wrote:
On Sat, Jul 20, 2013 at 10:32:26PM -0400, Alan Stern wrote:
On Sat, 20 Jul 2013, Greg KH wrote:
That should be passed using platform data.
Ick, don't pass strings around, pass pointers. If you have
platform
data you can get to, then put the pointer there, don't use a
name.
I don't think I understood you here :-s We wont have phy pointer
when we create the device for the controller no?(it'll be done in
board file). Probably I'm missing something.
Why will you not have that pointer? You can't rely on the name as
the device id will not match up, so you should be able to rely on
the pointer being in the structure that the board sets up, right?
Don't use names, especially as ids can, and will, change, that is
going
to cause big problems. Use pointers, this is C, we are supposed to
be
doing that :)
Kishon, I think what Greg means is this: The name you are using must
be stored somewhere in a data structure constructed by the board file,
right? Or at least, associated with some data structure somehow.
Otherwise the platform code wouldn't know which PHY hardware
corresponded to a particular name.
Greg's suggestion is that you store the address of that data structure
in the platform data instead of storing the name string. Have the
consumer pass the data structure's address when it calls phy_create,
instead of passing the name. Then you don't have to worry about two
PHYs accidentally ending up with the same name or any other similar
problems.
Close, but the issue is that whatever returns from phy_create() should
then be used, no need to call any find functions, as you can just use
the pointer that phy_create() returns. Much like all other class api
functions in the kernel work.
I think there is a confusion here about who registers the PHYs.
All platform code does is registering a platform/i2c/whatever device,
which causes a driver (located in drivers/phy/) to be instantiated. Such
drivers call phy_create(), usually in their probe() callbacks, so
platform_code has no way (and should have no way, for the sake of
layering) to get what phy_create() returns.
right.
IMHO we need a lookup method for PHYs, just like for clocks, regulators,
PWMs or even i2c busses because there are complex cases when passing just
a name using platform data will not work. I would second what Stephen said
[1] and define a structure doing things in a DT-like way.
Example;
[platform code]
static const struct phy_lookup my_phy_lookup[] = {
PHY_LOOKUP(s3c-hsotg.0, otg, samsung-usbphy.1, phy.2),
The only problem here is that if *PLATFORM_DEVID_AUTO* is used while
creating the device, the ids in the device name would change and
PHY_LOOKUP wont be useful.
Thanks
Kishon
--
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 01/15] drivers: phy: add generic PHY framework
On Sunday 21 of July 2013 16:37:33 Kishon Vijay Abraham I wrote:
Hi,
On Sunday 21 July 2013 04:01 PM, Tomasz Figa wrote:
Hi,
On Saturday 20 of July 2013 19:59:10 Greg KH wrote:
On Sat, Jul 20, 2013 at 10:32:26PM -0400, Alan Stern wrote:
On Sat, 20 Jul 2013, Greg KH wrote:
That should be passed using platform data.
Ick, don't pass strings around, pass pointers. If you have
platform
data you can get to, then put the pointer there, don't use a
name.
I don't think I understood you here :-s We wont have phy pointer
when we create the device for the controller no?(it'll be done in
board file). Probably I'm missing something.
Why will you not have that pointer? You can't rely on the name
as
the device id will not match up, so you should be able to rely on
the pointer being in the structure that the board sets up, right?
Don't use names, especially as ids can, and will, change, that is
going
to cause big problems. Use pointers, this is C, we are supposed to
be
doing that :)
Kishon, I think what Greg means is this: The name you are using
must
be stored somewhere in a data structure constructed by the board
file,
right? Or at least, associated with some data structure somehow.
Otherwise the platform code wouldn't know which PHY hardware
corresponded to a particular name.
Greg's suggestion is that you store the address of that data
structure
in the platform data instead of storing the name string. Have the
consumer pass the data structure's address when it calls phy_create,
instead of passing the name. Then you don't have to worry about two
PHYs accidentally ending up with the same name or any other similar
problems.
Close, but the issue is that whatever returns from phy_create()
should
then be used, no need to call any find functions, as you can just
use
the pointer that phy_create() returns. Much like all other class api
functions in the kernel work.
I think there is a confusion here about who registers the PHYs.
All platform code does is registering a platform/i2c/whatever device,
which causes a driver (located in drivers/phy/) to be instantiated.
Such drivers call phy_create(), usually in their probe() callbacks,
so platform_code has no way (and should have no way, for the sake of
layering) to get what phy_create() returns.
right.
IMHO we need a lookup method for PHYs, just like for clocks,
regulators, PWMs or even i2c busses because there are complex cases
when passing just a name using platform data will not work. I would
second what Stephen said [1] and define a structure doing things in a
DT-like way.
Example;
[platform code]
static const struct phy_lookup my_phy_lookup[] = {
PHY_LOOKUP(s3c-hsotg.0, otg, samsung-usbphy.1, phy.2),
The only problem here is that if *PLATFORM_DEVID_AUTO* is used while
creating the device, the ids in the device name would change and
PHY_LOOKUP wont be useful.
I don't think this is a problem. All the existing lookup methods already
use ID to identify devices (see regulators, clkdev, PWMs, i2c, ...). You
can simply add a requirement that the ID must be assigned manually,
without using PLATFORM_DEVID_AUTO to use PHY lookup.
Best regards,
Tomasz
--
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] xhci: fix null pointer dereference on ring_doorbell_for_active_rings
in some cases where device is attched to xhci port and do not responding,
for example ath9k_htc with stalled firmware, kernel will
crash on ring_doorbell_for_active_rings.
This patch check if pointer exist before it is used.
Signed-off-by: Oleksij Rempel li...@rempel-privat.de
---
drivers/usb/host/xhci-ring.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 1e57eaf..5b08cd8 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -434,7 +434,7 @@ static void ring_doorbell_for_active_rings(struct xhci_hcd
*xhci,
/* A ring has pending URBs if its TD list is not empty */
if (!(ep-ep_state EP_HAS_STREAMS)) {
- if (!(list_empty(ep-ring-td_list)))
+ if (ep-ring !(list_empty(ep-ring-td_list)))
xhci_ring_ep_doorbell(xhci, slot_id, ep_index, 0);
return;
}
--
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 should we handle isochronous underruns?
On Sun, 21 Jul 2013, Ming Lei wrote: On Sun, Jul 21, 2013 at 2:00 AM, Alan Stern st...@rowland.harvard.edu wrote: On Sat, 20 Jul 2013, Ming Lei wrote: No, we don't have to change every driver using isochronous URBs. Many of them set URB_ISO_ASAP on every URB -- in fact, AFAIK snd-usb-audio is the only one that has been fixed not to do this. OK, if you are sure just snd-usb-audio and very less drivers need the change, using the API only is correct way. I haven't checked in detail, but this seems very likely. For ehci example, URB_ISO_ASAP is not checked when isoc queue is empty, so will cause a new schedule when underrun happens. ehci-hcd is indeed one of the drivers that will need to be updated. So will ohci-hcd and uhci-hcd, when they start using tasklets. The new call doesn't turn streaming on or off. The driver does that by submitting URBs or not submitting them. The new call merely separates an old stream from a new one. It tells the HCD that the old stream (if any) is finished, so that any URBs submitted in the future will belong to a new stream. In particular, it tells the HCD that the next URB for this endpoint should be assigned to a time slot that will be visible to the hardware, instead of being assigned to the next time slot after the last packet of the previous URB (which has probably expired already). OK, I got it, looks usb_new_iso_stream(udev, ep) need to be called before starting a new stream. Or after ending an old stream. Either way would work. From view of HCD, the hcd only knows a stream has been started, and doesn't know if it has been stopped. And the most possible usage for hcd is that: if (test_and_clear_bit(ISOC_NEW_STREAM, ep-stream_flag) ; /* new stream */ So looks hcd has to write the flag. Not exactly. The HCD has to keep track of the next available time slot for the endpoint. Some special value, like -1, will indicate that no stream is running yet. So the new function will merely have to set the slot value to -1. If we define the API as usb_iso_stream(udev, ep, start), hcd only need to read the flag, so it should be simper to use and implement. It wouldn't be simpler for drivers. And what happens if the HCD gets an URB submitted for an endpoint where the stream_flag is off? Also with both streaming on and off information, the HCD might improve bandwidth management in the future. I doubt it very much. The USB spec states that bandwidth for periodic endpoints gets allocated whenever a new altsetting is installed, and that is the approach the HCDs will try to take. In a normal system, the URB count won't be very much( 1000), and if you mean the overhead is from memory, I guess it won't cause real memory increase per URB for slab's sake. If you mean performance loss with set/get the single lockless/common variable, maybe we should focus on the big HCD lock, which might be the biggest bottle of USB protocol, :-) If you're talking about the hcd_urb_list_lock, note that it could easily be made hcd-specific. I haven't done that because the regions it protects are all very short, so there probably isn't very much contention. I mean the HCD private lock, and there are heavy contention on the lock. I don't have any ideas how to reduce that contention. Do you? Also I thought of one improvement on the idea of 'urb-completing': - define one percpu 'struct urb *' variable inside 'struct hcd' to record the completing urb, so HCD can see easily if the urb is being resubmitted inside its completion handler. Even on a UP system, what happens if multiple URBs get completed before the tasklet runs and one of them is resubmitted? Anyway this approach is only helpful if much lots of drivers need changes for underrun case. 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][usbutils] lsusb: port to hwdb
On Sun, Jul 21, 2013 at 12:01:11PM +0200, Tom Gundersen wrote: On Sun, Jul 21, 2013 at 3:34 AM, Greg KH gre...@linuxfoundation.org wrote: Can this mean I can drop the usb.ids file from the usbutils package? I can't remember where hwdb is generated from, does it rely on the usb.ids file for the initial creation? hwdb does not use the usb.ids from the usbutils package. However, hwdb only contains vendor, product, class, subclass and protocol. So if you drop usb.ids the rest of the information will be lost. Maybe split the rest out into a separate file and ship only that? Or is there a way to get this info into hwdb? Kay? The rest of the info in there should be split out of usb.ids and probably just imported into the .c code directly, as it's not anything that changes very often, if at all. I can move that directly into usbutils without an issue. Also, I just realise there is also lsusb,py, which I did not port. What is the usecase for this? Is it also worth porting over? I agree with Kay, the options it provides should just be moved into the .c code itself. I added it years ago when someone sent it to me as it provided some options that the existing .c code didn't handle. Other than color support, I don't think that is true anymore, but I'll look closer at it to make sure. thanks, greg k-h -- 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][usbutils] lsusb: port to hwdb
On Sun, Jul 21, 2013 at 01:05:31PM +0200, Kay Sievers wrote: However, hwdb only contains vendor, product, class, subclass and protocol. So if you drop usb.ids the rest of the information will be lost. Maybe split the rest out into a separate file and ship only that? Or is there a way to get this info into hwdb? Kay? It should work to add some of that data to the existing modalias, right? For some things we probably need to invent new synthetic modaliases to query these strings. We should give it a try, I think. Having lsusb shipping a private file only for that seems ugly. These are things that are just described in the USB-IF specifications, (HID table commands, class codes, etc.) and really should not be part of any hardware database as they don't change, and come directly from the spec. I can just move those into the usbutils package, and get them removed from the usb.ids file, sound good? thanks, greg k-h -- 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 01/15] drivers: phy: add generic PHY framework
On Sun, Jul 21, 2013 at 01:12:07PM +0200, Tomasz Figa wrote:
On Sunday 21 of July 2013 16:37:33 Kishon Vijay Abraham I wrote:
Hi,
On Sunday 21 July 2013 04:01 PM, Tomasz Figa wrote:
Hi,
On Saturday 20 of July 2013 19:59:10 Greg KH wrote:
On Sat, Jul 20, 2013 at 10:32:26PM -0400, Alan Stern wrote:
On Sat, 20 Jul 2013, Greg KH wrote:
That should be passed using platform data.
Ick, don't pass strings around, pass pointers. If you have
platform
data you can get to, then put the pointer there, don't use a
name.
I don't think I understood you here :-s We wont have phy pointer
when we create the device for the controller no?(it'll be done in
board file). Probably I'm missing something.
Why will you not have that pointer? You can't rely on the name
as
the device id will not match up, so you should be able to rely on
the pointer being in the structure that the board sets up, right?
Don't use names, especially as ids can, and will, change, that is
going
to cause big problems. Use pointers, this is C, we are supposed to
be
doing that :)
Kishon, I think what Greg means is this: The name you are using
must
be stored somewhere in a data structure constructed by the board
file,
right? Or at least, associated with some data structure somehow.
Otherwise the platform code wouldn't know which PHY hardware
corresponded to a particular name.
Greg's suggestion is that you store the address of that data
structure
in the platform data instead of storing the name string. Have the
consumer pass the data structure's address when it calls phy_create,
instead of passing the name. Then you don't have to worry about two
PHYs accidentally ending up with the same name or any other similar
problems.
Close, but the issue is that whatever returns from phy_create()
should
then be used, no need to call any find functions, as you can just
use
the pointer that phy_create() returns. Much like all other class api
functions in the kernel work.
I think there is a confusion here about who registers the PHYs.
All platform code does is registering a platform/i2c/whatever device,
which causes a driver (located in drivers/phy/) to be instantiated.
Such drivers call phy_create(), usually in their probe() callbacks,
so platform_code has no way (and should have no way, for the sake of
layering) to get what phy_create() returns.
Why not put pointers in the platform data structure that can hold these
pointers? I thought that is why we created those structures in the
first place. If not, what are they there for?
IMHO we need a lookup method for PHYs, just like for clocks,
regulators, PWMs or even i2c busses because there are complex cases
when passing just a name using platform data will not work. I would
second what Stephen said [1] and define a structure doing things in a
DT-like way.
Example;
[platform code]
static const struct phy_lookup my_phy_lookup[] = {
PHY_LOOKUP(s3c-hsotg.0, otg, samsung-usbphy.1, phy.2),
The only problem here is that if *PLATFORM_DEVID_AUTO* is used while
creating the device, the ids in the device name would change and
PHY_LOOKUP wont be useful.
I don't think this is a problem. All the existing lookup methods already
use ID to identify devices (see regulators, clkdev, PWMs, i2c, ...). You
can simply add a requirement that the ID must be assigned manually,
without using PLATFORM_DEVID_AUTO to use PHY lookup.
And I'm saying that this idea, of using a specific name and id, is
frought with fragility and will break in the future in various ways when
devices get added to systems, making these strings constantly have to be
kept up to date with different board configurations.
People, NEVER, hardcode something like an id. The fact that this
happens today with the clock code, doesn't make it right, it makes the
clock code wrong. Others have already said that this is wrong there as
well, as systems change and dynamic ids get used more and more.
Let's not repeat the same mistakes of the past just because we refuse to
learn from them...
So again, the find a phy by a string functions should be removed, the
device id should be automatically created by the phy core just to make
things unique in sysfs, and no driver code should _ever_ be reliant on
the number that is being created, and the pointer to the phy structure
should be used everywhere instead.
With those types of changes, I will consider merging this subsystem, but
without them, sorry, I will not.
thanks,
greg k-h
--
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 01/15] drivers: phy: add generic PHY framework
On Sun, Jul 21, 2013 at 12:22:48PM +0200, Sascha Hauer wrote: On Sat, Jul 20, 2013 at 07:59:10PM -0700, Greg KH wrote: On Sat, Jul 20, 2013 at 10:32:26PM -0400, Alan Stern wrote: On Sat, 20 Jul 2013, Greg KH wrote: That should be passed using platform data. Ick, don't pass strings around, pass pointers. If you have platform data you can get to, then put the pointer there, don't use a name. I don't think I understood you here :-s We wont have phy pointer when we create the device for the controller no?(it'll be done in board file). Probably I'm missing something. Why will you not have that pointer? You can't rely on the name as the device id will not match up, so you should be able to rely on the pointer being in the structure that the board sets up, right? Don't use names, especially as ids can, and will, change, that is going to cause big problems. Use pointers, this is C, we are supposed to be doing that :) Kishon, I think what Greg means is this: The name you are using must be stored somewhere in a data structure constructed by the board file, right? Or at least, associated with some data structure somehow. Otherwise the platform code wouldn't know which PHY hardware corresponded to a particular name. Greg's suggestion is that you store the address of that data structure in the platform data instead of storing the name string. Have the consumer pass the data structure's address when it calls phy_create, instead of passing the name. Then you don't have to worry about two PHYs accidentally ending up with the same name or any other similar problems. Close, but the issue is that whatever returns from phy_create() should then be used, no need to call any find functions, as you can just use the pointer that phy_create() returns. Much like all other class api functions in the kernel work. I think the problem here is to connect two from the bus structure completely independent devices. Several frameworks (ASoC, soc-camera) had this problem and this wasn't solved until the advent of devicetrees and their phandles. phy_create might be called from the probe function of some i2c device (the phy device) and the resulting pointer is then needed in some other platform devices (the user of the phy) probe function. The best solution we have right now is implemented in the clk framework which uses a string matching of the device names in clk_get() (at least in the non-dt case). I would argue that clocks are wrong here as well, as others have already pointed out. What's wrong with the platform_data structure, why can't that be used for this? Or, if not, we can always add pointers to the platform device structure, or even the main 'struct device' as well, that's what it is there for. thanks, greg k-h -- 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][usbutils] lsusb: port to hwdb
On Sun, Jul 21, 2013 at 5:36 PM, Greg KH gre...@linuxfoundation.org wrote: On Sun, Jul 21, 2013 at 01:05:31PM +0200, Kay Sievers wrote: However, hwdb only contains vendor, product, class, subclass and protocol. So if you drop usb.ids the rest of the information will be lost. Maybe split the rest out into a separate file and ship only that? Or is there a way to get this info into hwdb? Kay? It should work to add some of that data to the existing modalias, right? For some things we probably need to invent new synthetic modaliases to query these strings. We should give it a try, I think. Having lsusb shipping a private file only for that seems ugly. These are things that are just described in the USB-IF specifications, (HID table commands, class codes, etc.) and really should not be part of any hardware database as they don't change, and come directly from the spec. I can just move those into the usbutils package, and get them removed from the usb.ids file, sound good? Sounds great to me, yeah. You are right, I think, the reason we *can* stuff it into the hwdb, is probably not a good enough reason to do it. lsusb would very likely be the only user ever. Kay -- 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][usbutils] lsusb: port to hwdb
On Sun, Jul 21, 2013 at 05:49:05PM +0200, Kay Sievers wrote: On Sun, Jul 21, 2013 at 5:36 PM, Greg KH gre...@linuxfoundation.org wrote: On Sun, Jul 21, 2013 at 01:05:31PM +0200, Kay Sievers wrote: However, hwdb only contains vendor, product, class, subclass and protocol. So if you drop usb.ids the rest of the information will be lost. Maybe split the rest out into a separate file and ship only that? Or is there a way to get this info into hwdb? Kay? It should work to add some of that data to the existing modalias, right? For some things we probably need to invent new synthetic modaliases to query these strings. We should give it a try, I think. Having lsusb shipping a private file only for that seems ugly. These are things that are just described in the USB-IF specifications, (HID table commands, class codes, etc.) and really should not be part of any hardware database as they don't change, and come directly from the spec. I can just move those into the usbutils package, and get them removed from the usb.ids file, sound good? Sounds great to me, yeah. You are right, I think, the reason we *can* stuff it into the hwdb, is probably not a good enough reason to do it. lsusb would very likely be the only user ever. Ok, I'll do that this week, and work with the owner of usb.ids to see if we can just remove them entirely from the file. Thanks again Tom for this patch, much appreciated. greg k-h -- 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 01/15] drivers: phy: add generic PHY framework
On 07/21/2013 05:48 PM, Greg KH wrote: On Sun, Jul 21, 2013 at 12:22:48PM +0200, Sascha Hauer wrote: On Sat, Jul 20, 2013 at 07:59:10PM -0700, Greg KH wrote: On Sat, Jul 20, 2013 at 10:32:26PM -0400, Alan Stern wrote: On Sat, 20 Jul 2013, Greg KH wrote: That should be passed using platform data. Ick, don't pass strings around, pass pointers. If you have platform data you can get to, then put the pointer there, don't use a name. I don't think I understood you here :-s We wont have phy pointer when we create the device for the controller no?(it'll be done in board file). Probably I'm missing something. Why will you not have that pointer? You can't rely on the name as the device id will not match up, so you should be able to rely on the pointer being in the structure that the board sets up, right? Don't use names, especially as ids can, and will, change, that is going to cause big problems. Use pointers, this is C, we are supposed to be doing that :) Kishon, I think what Greg means is this: The name you are using must be stored somewhere in a data structure constructed by the board file, right? Or at least, associated with some data structure somehow. Otherwise the platform code wouldn't know which PHY hardware corresponded to a particular name. Greg's suggestion is that you store the address of that data structure in the platform data instead of storing the name string. Have the consumer pass the data structure's address when it calls phy_create, instead of passing the name. Then you don't have to worry about two PHYs accidentally ending up with the same name or any other similar problems. Close, but the issue is that whatever returns from phy_create() should then be used, no need to call any find functions, as you can just use the pointer that phy_create() returns. Much like all other class api functions in the kernel work. I think the problem here is to connect two from the bus structure completely independent devices. Several frameworks (ASoC, soc-camera) had this problem and this wasn't solved until the advent of devicetrees and their phandles. phy_create might be called from the probe function of some i2c device (the phy device) and the resulting pointer is then needed in some other platform devices (the user of the phy) probe function. The best solution we have right now is implemented in the clk framework which uses a string matching of the device names in clk_get() (at least in the non-dt case). I would argue that clocks are wrong here as well, as others have already pointed out. What's wrong with the platform_data structure, why can't that be used for this? At the point the platform data of some driver is initialized, e.g. in board setup code the PHY pointer is not known, since the PHY supplier driver has not initialized yet. Even though we wanted to pass pointer to a PHY through some notifier call, it would have been not clear which PHY user driver should match on such notifier. A single PHY supplier driver can create M PHY objects and this needs to be mapped to N PHY user drivers. This mapping needs to be defined somewhere by the system integrator. It works well with device tree, but except that there seems to be no other reliable infrastructure in the kernel to define links/dependencies between devices, since device identifiers are not known in advance in all cases. What Tomasz proposed seems currently most reasonable to me for non-dt. Or, if not, we can always add pointers to the platform device structure, or even the main 'struct device' as well, that's what it is there for. Still we would need to solve a problem which platform device structure gets which PHY pointer. -- Regards, Sylwester -- 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 01/15] drivers: phy: add generic PHY framework
On Sun, 21 Jul 2013, Sylwester Nawrocki wrote: What's wrong with the platform_data structure, why can't that be used for this? At the point the platform data of some driver is initialized, e.g. in board setup code the PHY pointer is not known, since the PHY supplier driver has not initialized yet. Even though we wanted to pass pointer to a PHY through some notifier call, it would have been not clear which PHY user driver should match on such notifier. A single PHY supplier driver can create M PHY objects and this needs to be mapped to N PHY user drivers. This mapping needs to be defined somewhere by the system integrator. It works well with device tree, but except that there seems to be no other reliable infrastructure in the kernel to define links/dependencies between devices, since device identifiers are not known in advance in all cases. What Tomasz proposed seems currently most reasonable to me for non-dt. Or, if not, we can always add pointers to the platform device structure, or even the main 'struct device' as well, that's what it is there for. Still we would need to solve a problem which platform device structure gets which PHY pointer. Can you explain the issues in more detail? I don't fully understand the situation. Here's what I think I know: The PHY and the controller it is attached to are both physical devices. The connection between them is hardwired by the system manufacturer and cannot be changed by software. PHYs are generally described by fixed system-specific board files or by Device Tree information. Are they ever discovered dynamically? Is the same true for the controllers attached to the PHYs? If not -- if both a PHY and a controller are discovered dynamically -- how does the kernel know whether they are connected to each other? The kernel needs to know which controller is attached to which PHY. Currently this information is represented by name or ID strings embedded in platform data. The PHY's driver (the supplier) uses the platform data to construct a platform_device structure that represents the PHY. Until this is done, the controller's driver (the client) cannot use the PHY. Since there is no parent-child relation between the PHY and the controller, there is no guarantee that the PHY's driver will be ready when the controller's driver wants to use it. A deferred probe may be needed. The issue (or one of the issues) in this discussion is that Greg does not like the idea of using names or IDs to associate PHYs with controllers, because they are too prone to duplications or other errors. Pointers are more reliable. But pointers to what? Since the only data known to be available to both the PHY driver and controller driver is the platform data, the obvious answer is a pointer to platform data (either for the PHY or for the controller, or maybe both). Probably some of the details above are wrong; please indicate where I have gone astray. Also, I'm not clear about the role played by various APIs. For example, where does phy_create() fit into this picture? 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: [PROBLEM/BUG] mouse polling at only half rate on ohci_hcd
I took Alan's excellent advice and read a good bit of that book last night. Definitely some good authors there! After pondering Alan's diagnosis for a bit, I went to inspect the usbhid driver code, and wound up creating a patch which works! I've tested three different gaming mice, and they now all poll properly using the ohci_hcd driver when the usbhid driver uses two URBs to receive input data: (output from the evhz utility) event3: latest hz = 1000 (average hz = 1000) event3: latest hz = 1000 (average hz = 1000) event3: latest hz = 1000 (average hz = 1000) event3: latest hz = 1000 (average hz = 1000) event3: latest hz = 1000 (average hz = 1000) I probably need a lot more time to research things and make sure that I did this the proper way. I'm a bit scared to submit a patch for the first time, so I'd like it to be right. Jiri, can I send you some private e-mail to ask for your advice about all that? -- 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: [PROBLEM/BUG] mouse polling at only half rate on ohci_hcd
On Sun, Jul 21, 2013 at 02:59:41PM -0500, sgtcapslock wrote: I took Alan's excellent advice and read a good bit of that book last night. Definitely some good authors there! After pondering Alan's diagnosis for a bit, I went to inspect the usbhid driver code, and wound up creating a patch which works! I've tested three different gaming mice, and they now all poll properly using the ohci_hcd driver when the usbhid driver uses two URBs to receive input data: (output from the evhz utility) event3: latest hz = 1000 (average hz = 1000) event3: latest hz = 1000 (average hz = 1000) event3: latest hz = 1000 (average hz = 1000) event3: latest hz = 1000 (average hz = 1000) event3: latest hz = 1000 (average hz = 1000) I probably need a lot more time to research things and make sure that I did this the proper way. I'm a bit scared to submit a patch for the first time, so I'd like it to be right. Jiri, can I send you some private e-mail to ask for your advice about all that? Private email does not scale for kernel maintainers: http://www.arm.linux.org.uk/news/?newsitem=11 http://www.eyrie.org/~eagle/faqs/questions.html Please just submit the patch here and we will be glad to review it and see what, if anything, needs to be done to get it merged. You might want to read the file, Documentation/SubmittingPatches and run the tool, scripts/checkpatch.pl on your patch before sending it. Other than that, don't worry about public patch review, it's how the kernel is developed, not in private :) thanks, greg k-h -- 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
kernel panic in sg_complete
Hi Few days ago I fried (?) my SD card (with custom Arduino-based hw :) and now every time I insert it into my Linux laptop, after ~10 seconds, kernel panics: [ 258.288466] BUG: unable to handle kernel NULL pointer dereference at 0040 [ 258.289742] IP: [814d0849] sg_complete+0xb9/0x1d0 [ 258.290997] PGD 0 [ 258.29] Oops: [#1] PREEMPT SMP [ 258.293443] Modules linked in: arc4 iwldvm mac80211 dm_crypt iwlwifi snd_hda_codec_hdmi cfg80211 snd_hda_codec_idt snd_hda_intel snd_hda_codec uvcvideo rts5139(C) snd_hwdep snd_pcm videobuf2_core videodev snd_seq_midi snd_rawmidi bnep videobuf2_vmalloc snd_seq_midi_event videobuf2_memops snd_seq coretemp psmouse snd_timer snd_seq_device snd rfcomm serio_raw dell_wmi btusb lpc_ich microcode soundcore snd_page_alloc dell_laptop dcdbas sparse_keymap bluetooth mac_hid parport_pc ppdev lp parport binfmt_misc hid_generic i915 ghash_clmulni_intel cryptd usbhid drm_kms_helper hid drm i2c_algo_bit video ahci libahci wmi [ 258.299260] CPU: 3 PID: 0 Comm: swapper/3 Tainted: G C 3.10.1 #22 [ 258.300711] Hardware name: Dell Inc. Inspiron 7720/04M3YM, BIOS A07 08/16/2012 [ 258.302190] task: 88013a344560 ti: 88013a34e000 task.ti: 88013a34e000 [ 258.303681] RIP: 0010:[814d0849] [814d0849] sg_complete+0xb9/0x1d0 [ 258.305204] RSP: 0018:88013f2c3cc8 EFLAGS: 00010002 [ 258.306723] RAX: ff92 RBX: 88012ee30a70 RCX: 19ff [ 258.308260] RDX: 880130f17400 RSI: RDI: 0001 [ 258.309807] RBP: 88013f2c3cf8 R08: 81a3b1a8 R09: ff98 [ 258.311350] R10: 0001 R11: 0036 R12: ff98 [ 258.312887] R13: 88012f86ac00 R14: 88012ee30a80 R15: 8801302a7118 [ 258.314433] FS: () GS:88013f2c() knlGS: [ 258.315998] CS: 0010 DS: ES: CR0: 80050033 [ 258.317555] CR2: 0040 CR3: 01c0b000 CR4: 001407e0 [ 258.319138] DR0: DR1: DR2: [ 258.320724] DR3: DR6: 0ff0 DR7: 0400 [ 258.322298] Stack: [ 258.323866] 88012f86ac00 ff98 88012f86ac00 ff98 [ 258.325485] 880130e18a74 880130e18800 88013f2c3d18 814cd584 [ 258.327105] 88012f86ac00 ff8d 88013f2c3d48 814e1742 [ 258.328724] Call Trace: [ 258.330317] IRQ [ 258.330330] [814cd584] usb_hcd_giveback_urb+0x44/0xa0 [ 258.333499] [814e1742] ehci_urb_done+0x72/0xb0 [ 258.335092] [814e1d1e] qh_completions+0x33e/0x430 [ 258.336687] [814e1f58] end_unlink_async+0x148/0x250 [ 258.338280] [814e47d8] ehci_irq+0xa8/0x2e0 [ 258.339872] [814cc90e] usb_hcd_irq+0x2e/0x50 [ 258.341460] [811164a5] handle_irq_event_percpu+0x75/0x250 [ 258.343057] [811166c8] handle_irq_event+0x48/0x70 [ 258.344642] [811197ea] handle_fasteoi_irq+0x5a/0x100 [ 258.346214] [81047222] handle_irq+0x22/0x40 [ 258.347784] [816999fa] do_IRQ+0x5a/0xd0 [ 258.349355] [81690caa] common_interrupt+0x6a/0x6a [ 258.350909] EOI [ 258.350922] [810aed0a] ? __hrtimer_start_range_ns+0x16a/0x460 [ 258.354032] [81547bbb] ? cpuidle_enter_state+0x5b/0xe0 [ 258.355595] [81547bb7] ? cpuidle_enter_state+0x57/0xe0 [ 258.357150] [81547cfb] cpuidle_idle_call+0xbb/0x260 [ 258.358698] [8104e0fe] arch_cpu_idle+0xe/0x30 [ 258.360241] [810d70b0] cpu_startup_entry+0xd0/0x2d0 [ 258.361758] [810df126] ? clockevents_config_and_register+0x26/0x30 [ 258.363263] [8167efb6] start_secondary+0x1e3/0x1ea [ 258.364753] Code: 00 00 31 d2 45 85 c9 74 b7 41 f6 45 65 02 48 c7 c2 b7 b1 a3 81 45 89 e1 48 8b 73 18 49 c7 c0 a8 b1 a3 81 4c 0f 44 c2 49 8b 55 50 48 8b 7e 40 0f b6 4a 02 89 04 24 48 8d 56 04 48 8b 3f 31 c0 48 [ 258.368220] RIP [814d0849] sg_complete+0xb9/0x1d0 [ 258.369869] RSP 88013f2c3cc8 [ 258.371496] CR2: 0040 [ 258.373097] ---[ end trace e81441c38e5ace26 ]--- [ 258.374690] Kernel panic - not syncing: Fatal exception in interrupt [ 258.376291] drm_kms_helper: panic occurred, switching back to text console It happens both with 3.10.1 and 3.9.11 kernels. I traced it to io-dev being NULL in sg_complete. With the attached bandage Linux survives: [ 138.375564] sd 6:0:0:0: [sdb] 3862528 512-byte logical blocks: (1.97 GB/1.84 GiB) [ 138.375717] sd 6:0:0:0: [sdb] Cache data unavailable [ 138.375721] sd 6:0:0:0: [sdb] Assuming drive cache: write through [ 138.378277] sd 6:0:0:0: [sdb] Cache data unavailable [ 138.378279] sd 6:0:0:0: [sdb] Assuming drive cache: write through [ 138.394977] sdb: sdb1 [ 148.587252] (NULL device *): dev ??? ep2in scatterlist error -104/-110 [ 148.589748] sd 6:0:0:0: [sdb] Unhandled sense code [
Re: kernel panic in sg_complete
On Mon, Jul 22, 2013 at 01:00:41AM +0200, Marcin Ślusarz wrote: If you want me to test some patches, just shout ;) Interesting patch, it makes sense to have this applied. Can you resend it with a Signed-off-by: line so that I can add it to the tree? thanks, greg k-h -- 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 v12 00/13] Add tested id switch and vbus connect detect support for Chipidea
Hi Peter, On Fri, Jul 12, 2013 at 03:18:31PM +0200, Marek Vasut wrote: Hi Peter, On Fri, Jul 12, 2013 at 06:04:43AM +0200, Marek Vasut wrote: Hi Peter, On Thu, Jul 11, 2013 at 07:57:19PM +0200, Marek Vasut wrote: Hi Peter, This patchset adds tested otg id switch function and vbus connect and disconnect detection for chipidea driver. And fix kinds of bugs found at chipidea drivers after enabling id and vbus detection. This patch is fully tested at imx6 sabresd platform. My chipidea repo: https://github.com/hzpeterchen/linux-usb.git Changes for v12: - Rebased greg's usb-next tree (3.10.0-rc7+) - Split more small patches for single function and fix. I tested the patchset. Here are the results: - VBUS switching I'm no longer getting any ID interrupts at all when I apply the patch below. The board stays in HOST mode all the time. If I configure it as peripheral, it works as peripheral. Note with [1], I was able to switch from Peripheral-Host , not the other way around. Thanks for your testing. But first, can you have me check if your ID wakeup is enabled? ID wakeup? How do I check? See otgsc at controller register, the ID wakeup enable is bit 24. Yes, ID interrupt (IDIE) is set. I noticed this backtrace in the kernel bootlog, but this only happens if the dr_mode=otg , it comes from the host-mode irq handler : [2.757563] irq 238: nobody cared (try booting with the irqpoll option) [2.764398] CPU: 0 PID: 1 Comm: swapper Not tainted 3.10.0- next-20130711-00013-g011c4b3-dirty #703 [2.773445] [80013878] (unwind_backtrace+0x0/0xe8) from [80011644] (show_stack+0x10/0x14) [2.782027] [80011644] (show_stack+0x10/0x14) from [800659f4] (__report_bad_irq.isra.6+0x20/0xe0) [2.791286] [800659f4] (__report_bad_irq.isra.6+0x20/0xe0) from [80065c98] (note_interrupt+0x16c/0x230) [2.801063] [80065c98] (note_interrupt+0x16c/0x230) from [80064000] (handle_irq_event_percpu+0x10c/0x1a4) [2.811010] [80064000] (handle_irq_event_percpu+0x10c/0x1a4) from [800640e8] (handle_irq_event+0x50/0x78) [2.820958] [800640e8] (handle_irq_event+0x50/0x78) from [8006652c] (handle_level_irq+0x88/0x10c) [2.830210] [8006652c] (handle_level_irq+0x88/0x10c) from [800638d0] (generic_handle_irq+0x28/0x3c) [2.839637] [800638d0] (generic_handle_irq+0x28/0x3c) from [8000f84c] (handle_IRQ+0x30/0x84) [2.848461] [8000f84c] (handle_IRQ+0x30/0x84) from [80012160] (__irq_svc+0x40/0x6c) [2.856510] [80012160] (__irq_svc+0x40/0x6c) from [80022a44] (__do_softirq+0x90/0x1d8) [2.864812] [80022a44] (__do_softirq+0x90/0x1d8) from [80022edc] (irq_exit+0x98/0xd4) [2.873025] [80022edc] (irq_exit+0x98/0xd4) from [8000f850] (handle_IRQ+0x34/0x84) [2.880980] [8000f850] (handle_IRQ+0x34/0x84) from [80012160] (__irq_svc+0x40/0x6c) [2.889020] [80012160] (__irq_svc+0x40/0x6c) from [8001d724] (vprintk_emit+0x1bc/0x524) [2.897411] [8001d724] (vprintk_emit+0x1bc/0x524) from [804da5a4] (printk+0x30/0x40) [2.905551] [804da5a4] (printk+0x30/0x40) from [80630138] (mousedev_init+0x4c/0x60) [2.913617] [80630138] (mousedev_init+0x4c/0x60) from [806178fc] (do_one_initcall+0x94/0x14c) [2.922537] [806178fc] (do_one_initcall+0x94/0x14c) from [80617b20] (kernel_init_freeable+0x16c/0x22c) [2.932230] [80617b20] (kernel_init_freeable+0x16c/0x22c) from [804d8cbc] (kernel_init+0x8/0x150) [2.941486] [804d8cbc] (kernel_init+0x8/0x150) from [8000ea70] (ret_from_fork+0x14/0x24) [2.949932] handlers: [2.952227] [8033fc58] ci_irq [2.955388] Disabling IRQ #238 Marek, I have a test at imx28evk (Freescale imx28evk) for this patchset, it works well for dual-role switch after adding below dts change. There is a kernel dump when works at udc mode(due to enable CONFIG_LOCKUP_DETECTOR), at the first connect, but it does not affect function, it should be a common chipidea problem, I will check later. Sorry for the long delay. I finally got further with this, at least now I can switch Periph-Host again, but if I do so the other way around (unplug host adapter and plug computer cable) and dump the controller registers, I still see the controller in host (according to USBMODE register) mode. The controller stays in HOST mode even if I force it to gadget mode by writing gadget into debugfs role entry. I will keep poking. In the meantime, I guess if it works on the EVK, it might be a hardware error so I better stop stalling you. I'll drop you an email if I find what this problem is. 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 v12 00/13] Add tested id switch and vbus connect detect support for Chipidea
On Mon, Jul 22, 2013 at 03:15:28AM +0200, Marek Vasut wrote: Hi Peter, On Fri, Jul 12, 2013 at 03:18:31PM +0200, Marek Vasut wrote: Hi Peter, On Fri, Jul 12, 2013 at 06:04:43AM +0200, Marek Vasut wrote: Hi Peter, On Thu, Jul 11, 2013 at 07:57:19PM +0200, Marek Vasut wrote: Hi Peter, This patchset adds tested otg id switch function and vbus connect and disconnect detection for chipidea driver. And fix kinds of bugs found at chipidea drivers after enabling id and vbus detection. This patch is fully tested at imx6 sabresd platform. My chipidea repo: https://github.com/hzpeterchen/linux-usb.git Changes for v12: - Rebased greg's usb-next tree (3.10.0-rc7+) - Split more small patches for single function and fix. I tested the patchset. Here are the results: - VBUS switching I'm no longer getting any ID interrupts at all when I apply the patch below. The board stays in HOST mode all the time. If I configure it as peripheral, it works as peripheral. Note with [1], I was able to switch from Peripheral-Host , not the other way around. Thanks for your testing. But first, can you have me check if your ID wakeup is enabled? ID wakeup? How do I check? See otgsc at controller register, the ID wakeup enable is bit 24. Yes, ID interrupt (IDIE) is set. I noticed this backtrace in the kernel bootlog, but this only happens if the dr_mode=otg , it comes from the host-mode irq handler : [2.757563] irq 238: nobody cared (try booting with the irqpoll option) [2.764398] CPU: 0 PID: 1 Comm: swapper Not tainted 3.10.0- next-20130711-00013-g011c4b3-dirty #703 [2.773445] [80013878] (unwind_backtrace+0x0/0xe8) from [80011644] (show_stack+0x10/0x14) [2.782027] [80011644] (show_stack+0x10/0x14) from [800659f4] (__report_bad_irq.isra.6+0x20/0xe0) [2.791286] [800659f4] (__report_bad_irq.isra.6+0x20/0xe0) from [80065c98] (note_interrupt+0x16c/0x230) [2.801063] [80065c98] (note_interrupt+0x16c/0x230) from [80064000] (handle_irq_event_percpu+0x10c/0x1a4) [2.811010] [80064000] (handle_irq_event_percpu+0x10c/0x1a4) from [800640e8] (handle_irq_event+0x50/0x78) [2.820958] [800640e8] (handle_irq_event+0x50/0x78) from [8006652c] (handle_level_irq+0x88/0x10c) [2.830210] [8006652c] (handle_level_irq+0x88/0x10c) from [800638d0] (generic_handle_irq+0x28/0x3c) [2.839637] [800638d0] (generic_handle_irq+0x28/0x3c) from [8000f84c] (handle_IRQ+0x30/0x84) [2.848461] [8000f84c] (handle_IRQ+0x30/0x84) from [80012160] (__irq_svc+0x40/0x6c) [2.856510] [80012160] (__irq_svc+0x40/0x6c) from [80022a44] (__do_softirq+0x90/0x1d8) [2.864812] [80022a44] (__do_softirq+0x90/0x1d8) from [80022edc] (irq_exit+0x98/0xd4) [2.873025] [80022edc] (irq_exit+0x98/0xd4) from [8000f850] (handle_IRQ+0x34/0x84) [2.880980] [8000f850] (handle_IRQ+0x34/0x84) from [80012160] (__irq_svc+0x40/0x6c) [2.889020] [80012160] (__irq_svc+0x40/0x6c) from [8001d724] (vprintk_emit+0x1bc/0x524) [2.897411] [8001d724] (vprintk_emit+0x1bc/0x524) from [804da5a4] (printk+0x30/0x40) [2.905551] [804da5a4] (printk+0x30/0x40) from [80630138] (mousedev_init+0x4c/0x60) [2.913617] [80630138] (mousedev_init+0x4c/0x60) from [806178fc] (do_one_initcall+0x94/0x14c) [2.922537] [806178fc] (do_one_initcall+0x94/0x14c) from [80617b20] (kernel_init_freeable+0x16c/0x22c) [2.932230] [80617b20] (kernel_init_freeable+0x16c/0x22c) from [804d8cbc] (kernel_init+0x8/0x150) [2.941486] [804d8cbc] (kernel_init+0x8/0x150) from [8000ea70] (ret_from_fork+0x14/0x24) [2.949932] handlers: [2.952227] [8033fc58] ci_irq [2.955388] Disabling IRQ #238 Marek, I have a test at imx28evk (Freescale imx28evk) for this patchset, it works well for dual-role switch after adding below dts change. There is a kernel dump when works at udc mode(due to enable CONFIG_LOCKUP_DETECTOR), at the first connect, but it does not affect function, it should be a common chipidea problem, I will check later. Sorry for the long delay. I finally got further with this, at least now I can switch Periph-Host again, but if I do so the other way around (unplug host adapter and plug computer cable) and dump the controller registers, I still see the controller in host (according to USBMODE register) mode. The controller stays in HOST mode even if I force it to gadget mode by writing gadget into debugfs role entry. I will keep poking. In the meantime, I guess if it works on the EVK, it might be a hardware error so I better stop stalling you. I'll drop you an email if I find what this problem is. No, the problem is you did not config the
Re: [PATCH v12 00/13] Add tested id switch and vbus connect detect support for Chipidea
Dear Peter Chen, On Mon, Jul 22, 2013 at 03:15:28AM +0200, Marek Vasut wrote: Hi Peter, On Fri, Jul 12, 2013 at 03:18:31PM +0200, Marek Vasut wrote: Hi Peter, On Fri, Jul 12, 2013 at 06:04:43AM +0200, Marek Vasut wrote: Hi Peter, On Thu, Jul 11, 2013 at 07:57:19PM +0200, Marek Vasut wrote: Hi Peter, This patchset adds tested otg id switch function and vbus connect and disconnect detection for chipidea driver. And fix kinds of bugs found at chipidea drivers after enabling id and vbus detection. This patch is fully tested at imx6 sabresd platform. My chipidea repo: https://github.com/hzpeterchen/linux-usb.git Changes for v12: - Rebased greg's usb-next tree (3.10.0-rc7+) - Split more small patches for single function and fix. I tested the patchset. Here are the results: - VBUS switching I'm no longer getting any ID interrupts at all when I apply the patch below. The board stays in HOST mode all the time. If I configure it as peripheral, it works as peripheral. Note with [1], I was able to switch from Peripheral-Host , not the other way around. Thanks for your testing. But first, can you have me check if your ID wakeup is enabled? ID wakeup? How do I check? See otgsc at controller register, the ID wakeup enable is bit 24. Yes, ID interrupt (IDIE) is set. I noticed this backtrace in the kernel bootlog, but this only happens if the dr_mode=otg , it comes from the host-mode irq handler : [2.757563] irq 238: nobody cared (try booting with the irqpoll option) [2.764398] CPU: 0 PID: 1 Comm: swapper Not tainted 3.10.0- next-20130711-00013-g011c4b3-dirty #703 [2.773445] [80013878] (unwind_backtrace+0x0/0xe8) from [80011644] (show_stack+0x10/0x14) [2.782027] [80011644] (show_stack+0x10/0x14) from [800659f4] (__report_bad_irq.isra.6+0x20/0xe0) [2.791286] [800659f4] (__report_bad_irq.isra.6+0x20/0xe0) from [80065c98] (note_interrupt+0x16c/0x230) [2.801063] [80065c98] (note_interrupt+0x16c/0x230) from [80064000] (handle_irq_event_percpu+0x10c/0x1a4) [2.811010] [80064000] (handle_irq_event_percpu+0x10c/0x1a4) from [800640e8] (handle_irq_event+0x50/0x78) [2.820958] [800640e8] (handle_irq_event+0x50/0x78) from [8006652c] (handle_level_irq+0x88/0x10c) [2.830210] [8006652c] (handle_level_irq+0x88/0x10c) from [800638d0] (generic_handle_irq+0x28/0x3c) [2.839637] [800638d0] (generic_handle_irq+0x28/0x3c) from [8000f84c] (handle_IRQ+0x30/0x84) [2.848461] [8000f84c] (handle_IRQ+0x30/0x84) from [80012160] (__irq_svc+0x40/0x6c) [2.856510] [80012160] (__irq_svc+0x40/0x6c) from [80022a44] (__do_softirq+0x90/0x1d8) [2.864812] [80022a44] (__do_softirq+0x90/0x1d8) from [80022edc] (irq_exit+0x98/0xd4) [2.873025] [80022edc] (irq_exit+0x98/0xd4) from [8000f850] (handle_IRQ+0x34/0x84) [2.880980] [8000f850] (handle_IRQ+0x34/0x84) from [80012160] (__irq_svc+0x40/0x6c) [2.889020] [80012160] (__irq_svc+0x40/0x6c) from [8001d724] (vprintk_emit+0x1bc/0x524) [2.897411] [8001d724] (vprintk_emit+0x1bc/0x524) from [804da5a4] (printk+0x30/0x40) [2.905551] [804da5a4] (printk+0x30/0x40) from [80630138] (mousedev_init+0x4c/0x60) [2.913617] [80630138] (mousedev_init+0x4c/0x60) from [806178fc] (do_one_initcall+0x94/0x14c) [2.922537] [806178fc] (do_one_initcall+0x94/0x14c) from [80617b20] (kernel_init_freeable+0x16c/0x22c) [2.932230] [80617b20] (kernel_init_freeable+0x16c/0x22c) from [804d8cbc] (kernel_init+0x8/0x150) [2.941486] [804d8cbc] (kernel_init+0x8/0x150) from [8000ea70] (ret_from_fork+0x14/0x24) [2.949932] handlers: [2.952227] [8033fc58] ci_irq [2.955388] Disabling IRQ #238 Marek, I have a test at imx28evk (Freescale imx28evk) for this patchset, it works well for dual-role switch after adding below dts change. There is a kernel dump when works at udc mode(due to enable CONFIG_LOCKUP_DETECTOR), at the first connect, but it does not affect function, it should be a common chipidea problem, I will check later. Sorry for the long delay. I finally got further with this, at least now I can switch Periph-Host again, but if I do so the other way around (unplug host adapter and plug computer cable) and dump the controller registers, I still see the controller in host (according to USBMODE register) mode. The controller stays in HOST mode even if I force it to gadget mode by writing gadget into debugfs role entry. I will keep poking. In the meantime, I guess if it works on the EVK, it might be a hardware error so I
Re: [PATCH v12 00/13] Add tested id switch and vbus connect detect support for Chipidea
On Mon, Jul 22, 2013 at 03:40:32AM +0200, Marek Vasut wrote: Dear Peter Chen, On Mon, Jul 22, 2013 at 03:15:28AM +0200, Marek Vasut wrote: Hi Peter, On Fri, Jul 12, 2013 at 03:18:31PM +0200, Marek Vasut wrote: Hi Peter, On Fri, Jul 12, 2013 at 06:04:43AM +0200, Marek Vasut wrote: Hi Peter, On Thu, Jul 11, 2013 at 07:57:19PM +0200, Marek Vasut wrote: Hi Peter, This patchset adds tested otg id switch function and vbus connect and disconnect detection for chipidea driver. And fix kinds of bugs found at chipidea drivers after enabling id and vbus detection. This patch is fully tested at imx6 sabresd platform. My chipidea repo: https://github.com/hzpeterchen/linux-usb.git Changes for v12: - Rebased greg's usb-next tree (3.10.0-rc7+) - Split more small patches for single function and fix. I tested the patchset. Here are the results: - VBUS switching I'm no longer getting any ID interrupts at all when I apply the patch below. The board stays in HOST mode all the time. If I configure it as peripheral, it works as peripheral. Note with [1], I was able to switch from Peripheral-Host , not the other way around. Thanks for your testing. But first, can you have me check if your ID wakeup is enabled? ID wakeup? How do I check? See otgsc at controller register, the ID wakeup enable is bit 24. Yes, ID interrupt (IDIE) is set. I noticed this backtrace in the kernel bootlog, but this only happens if the dr_mode=otg , it comes from the host-mode irq handler : [2.757563] irq 238: nobody cared (try booting with the irqpoll option) [2.764398] CPU: 0 PID: 1 Comm: swapper Not tainted 3.10.0- next-20130711-00013-g011c4b3-dirty #703 [2.773445] [80013878] (unwind_backtrace+0x0/0xe8) from [80011644] (show_stack+0x10/0x14) [2.782027] [80011644] (show_stack+0x10/0x14) from [800659f4] (__report_bad_irq.isra.6+0x20/0xe0) [2.791286] [800659f4] (__report_bad_irq.isra.6+0x20/0xe0) from [80065c98] (note_interrupt+0x16c/0x230) [2.801063] [80065c98] (note_interrupt+0x16c/0x230) from [80064000] (handle_irq_event_percpu+0x10c/0x1a4) [2.811010] [80064000] (handle_irq_event_percpu+0x10c/0x1a4) from [800640e8] (handle_irq_event+0x50/0x78) [2.820958] [800640e8] (handle_irq_event+0x50/0x78) from [8006652c] (handle_level_irq+0x88/0x10c) [2.830210] [8006652c] (handle_level_irq+0x88/0x10c) from [800638d0] (generic_handle_irq+0x28/0x3c) [2.839637] [800638d0] (generic_handle_irq+0x28/0x3c) from [8000f84c] (handle_IRQ+0x30/0x84) [2.848461] [8000f84c] (handle_IRQ+0x30/0x84) from [80012160] (__irq_svc+0x40/0x6c) [2.856510] [80012160] (__irq_svc+0x40/0x6c) from [80022a44] (__do_softirq+0x90/0x1d8) [2.864812] [80022a44] (__do_softirq+0x90/0x1d8) from [80022edc] (irq_exit+0x98/0xd4) [2.873025] [80022edc] (irq_exit+0x98/0xd4) from [8000f850] (handle_IRQ+0x34/0x84) [2.880980] [8000f850] (handle_IRQ+0x34/0x84) from [80012160] (__irq_svc+0x40/0x6c) [2.889020] [80012160] (__irq_svc+0x40/0x6c) from [8001d724] (vprintk_emit+0x1bc/0x524) [2.897411] [8001d724] (vprintk_emit+0x1bc/0x524) from [804da5a4] (printk+0x30/0x40) [2.905551] [804da5a4] (printk+0x30/0x40) from [80630138] (mousedev_init+0x4c/0x60) [2.913617] [80630138] (mousedev_init+0x4c/0x60) from [806178fc] (do_one_initcall+0x94/0x14c) [2.922537] [806178fc] (do_one_initcall+0x94/0x14c) from [80617b20] (kernel_init_freeable+0x16c/0x22c) [2.932230] [80617b20] (kernel_init_freeable+0x16c/0x22c) from [804d8cbc] (kernel_init+0x8/0x150) [2.941486] [804d8cbc] (kernel_init+0x8/0x150) from [8000ea70] (ret_from_fork+0x14/0x24) [2.949932] handlers: [2.952227] [8033fc58] ci_irq [2.955388] Disabling IRQ #238 Marek, I have a test at imx28evk (Freescale imx28evk) for this patchset, it works well for dual-role switch after adding below dts change. There is a kernel dump when works at udc mode(due to enable CONFIG_LOCKUP_DETECTOR), at the first connect, but it does not affect function, it should be a common chipidea problem, I will check later. Sorry for the long delay. I finally got further with this, at least now I can switch Periph-Host again, but if I do so the other way around (unplug host adapter and plug computer cable) and dump the controller registers, I still see the controller in host (according to USBMODE register) mode. The controller stays in HOST mode even if I force it to
Re: kernel panic in sg_complete
On Sun, 21 Jul 2013, Greg Kroah-Hartman wrote: On Mon, Jul 22, 2013 at 01:00:41AM +0200, Marcin Ślusarz wrote: If you want me to test some patches, just shout ;) Interesting patch, it makes sense to have this applied. Can you resend it with a Signed-off-by: line so that I can add it to the tree? Wait a minute. The patch catches cases where io-dev is NULL in sg_complete(). But how does that happen? The only place io-dev gets set to NULL is in sg_clean(), and sg_clean() gets called in only two places: if an URB could not be allocated in usb_sg_init(), or after all the URBs have completed in usb_sg_wait(). In either case, no more URBs will complete after sg_clean() is called, so sg_complete() won't run. A patch like this merely covers up the symptom without fixing the underlying cause. How did this manage to go wrong in the first place? There must be a bug in the logic of sg_complete() or usb_sg_wait(). _That_ logic bug is what needs to be fixed. 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 v2 3/4] arm: dts: Add USB phy nodes for AM33XX
On 7/20/2013 9:11 AM, George Cherian wrote:
On 7/20/2013 12:12 AM, Sebastian Andrzej Siewior wrote:
On 07/19/2013 08:33 PM, Sergei Shtylyov wrote:
Hello.
Hello,
usb: usb@4740 {
compatible = ti,am33xx-usb;
usb0_phy: phy@47401300 {
compatible = ti,am335x-usb-phy;
}
usb0: usb@47401000 {
musb0: usb@47401400 {
compatible = mg,musbmhdrc;
}
}
usb1_phy: phy@47402300 {
compatible = ti,am335x-usb-phy;
}
usb1: usb@47402000 {
musb1: usb@47402400 {
compatible = mg,musbmhdrc;
}
}
}
How about something like this ?
Here am making
wrapper - musb instance 0 - phy0
wrapper - musb instance 1 - phy1
musb: usb@4740 {
compatible = ti,musb-am33xx;
reg = 0x4740 0x1000;
ranges;
#address-cells = 1;
#size-cells = 1;
interrupts = 17;
ti,hwmods = usb_otg_hs;
usb0@47401000 {
reg = 0x47401000 0x800;
interrupts = 18;
interrupt-names = mc;
multipoint = 1;
num-eps = 16;
ram-bits = 12;
port-mode = 3;
power = 250;
phys = phy1;
phy-names = am335x-usb0;
phy1: am335x-usb0 {
compatible = ti,am335x-usb2;
#phy-cells = 0;
id= 0;
};
};
usb1@47401800 {
reg = 0x47401800 0x800;
interrupts = 19;
interrupt-names = mc;
multipoint = 1;
num-eps = 16;
ram-bits = 12;
port-mode = 3;
power = 250;
phys = phy2;
phy-names = am335x-usb1;
phy2: am335x-usb1 {
compatible = ti,am335x-usb2;
#phy-cells = 0;
id= 1;
};
};
};
--
-George
--
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
