Re: [PATCH 2/3] usb: Take attribute avoid_reset_quirk out of usb device's attribute group
Oliver Neukum writes: > On Monday 30 July 2012 22:35:37 Bjørn Mork wrote: >> Sarah Sharp writes: >> >> > Possible solutions >> > -- >> > >> > - We need a new interface driver flag to indicate a driver is fine with >> >the port being powered off. Something like "supports_power_off". >> >> May work for some devices. But what are the use cases? Which devices >> will work better with such driver support than they will if you simply >> unload the driver? > > uvc, some hid, some storage devices. I don't know anything about storage devices, but forcing them to power off sounds scary to me. If that can be done safely with driver support then I guess that's a good reason, yes. For uvc and hid: What's the advantage with driver support compared to just unbinding? When it comes to hid I would assume that most such devices need remote wakeup to be able to sleep at all? >> >2. If the device is internal and suspended, power off the port if all >> > the following are true: >> > >> > a) all interface drivers have supports_power_off set, or no >> > interface drivers are bound and usbfs has not claimed the >> > device. >> > b) remote wakeup is disabled >> > c) USB_QUIRK_RESET_MORPHS is not set >> >> Why can't that be a userspace decision? I.e. drop all this policy in >> the kernel stuff, and just implement: > > We cannot. User space doesn't know and cannot know when remote wakeup > is needed. So? Return an error. It's not like you are going to guarantee this operation will succeed anyway. Or if you worry about being able to power off the port as soon as possible: Replicate the autosuspend functionality, with userspace controlled enable and delay instead of making it an on/off switch. But please put the control of which ports to enable this feature for in the hands of the user. 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: [PATCH 2/3] usb: Take attribute avoid_reset_quirk out of usb device's attribute group
On Tuesday 31 July 2012 09:09:06 Bjørn Mork wrote: > Oliver Neukum writes: > > On Monday 30 July 2012 22:35:37 Bjørn Mork wrote: > >> Sarah Sharp writes: > > uvc, some hid, some storage devices. > > I don't know anything about storage devices, but forcing them to power > off sounds scary to me. If that can be done safely with driver support > then I guess that's a good reason, yes. Powering off storage devices is scary. However, we do exactly that when we go to S4. > For uvc and hid: What's the advantage with driver support compared to The system, including all devices, stays fully functional. > just unbinding? When it comes to hid I would assume that most such > devices need remote wakeup to be able to sleep at all? Depends on whether they are open. You can power down stuff like joysticks that are not in use. With the help of user space we could also power down internal touchpads and keyboards while the lid is closed. > >> >2. If the device is internal and suspended, power off the port if all > >> > the following are true: > >> > > >> > a) all interface drivers have supports_power_off set, or no > >> > interface drivers are bound and usbfs has not claimed the > >> > device. > >> > b) remote wakeup is disabled > >> > c) USB_QUIRK_RESET_MORPHS is not set > >> > >> Why can't that be a userspace decision? I.e. drop all this policy in > >> the kernel stuff, and just implement: > > > > We cannot. User space doesn't know and cannot know when remote wakeup > > is needed. > > So? Return an error. It's not like you are going to guarantee this > operation will succeed anyway. Or if you worry about being able to We don't guarantee any IO to succeed. That's no excuse to not try. > power off the port as soon as possible: Replicate the autosuspend > functionality, with userspace controlled enable and delay instead of > making it an on/off switch. User space cannot control auto resume. It is fundamentally impossible. You will deadlock sooner or later. Devices must be resumed by the kernel as soon as they are needed. Otherwise they must be totally closed. User space can power off devices. We can already do that by control messages and could extended the code for root hubs to use ACPI methods. That is beside the point for this discussion. > But please put the control of which ports to enable this feature for in > the hands of the user. Definitely, with very few exceptions. Regards Oliver -- 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 2/3] usb: Take attribute avoid_reset_quirk out of usb device's attribute group
Alan Stern writes:
> On Mon, 30 Jul 2012, Sarah Sharp wrote:
>
>> - I'm not sure if it's true that all devices that need firmware will
>>have USB_QUIRK_RESET_MORPHS set. Alan, Oliver?
>
> Probably not, and in any case the situation is more complex than it
> first appears. (In fact, I wonder whether we really need that quirk
> flag at all -- as far as I can tell it's not doing anybody any good.)
usb_modeswitch use it to prevent usb-storage from resetting the device
on errors. Some 3G modems expose a storage interface (SD card reader
and/or embedded driver CD) even after switching to modem mode. You do
not want them to switch back from modem mode on every usb-storage
problem, especially as you may assume the device firmware is buggy. Some
devices would probably end up in a continuous reset loop without this
quirk.
>From usb_modeswitch.tcl:
# In newer kernels there is a switch to avoid the use of a device
# reset (e.g. from usb-storage) which would possibly switch back
# a mode-switching device to initial mode
if [regexp {ok:} $report] {
Log "Checking for AVOID_RESET_QUIRK attribute"
if [file exists $devdir/avoid_reset_quirk] {
if [catch {exec echo "1" >$devdir/avoid_reset_quirk
2>/dev/null} err] {
Log " Error setting the attribute: $err"
} else {
Log " AVOID_RESET_QUIRK activated"
}
} else {
Log " AVOID_RESET_QUIRK not present"
}
}
IMHO that should have been the default for any composite device, but
whatever.. I don't think it's very nice of usb-storage to reset the
device if there are drivers bound to other functions.
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: [PATCH v3 1/3] USB: chipidea: add imx usbmisc support
On Tue, Jul 31, 2012 at 09:06:33AM +0800, Chen Peter-B29397 wrote: > > > > > > > <&usbmisc 0> would then mean port 0 of the usbmisc device. > > I didn't add the restriction that a usbmisc driver must have a usbmisc > > device. I'm not sure whether all SoC and future SoC can be look as > > a device. > > > > Peter, do you have any idea? > > > I have not followed this usbmisc design, I just list some facts at i.mx > USB controller: > > Sigmatel-derived SoCs (i.mx23, i.mx28) have no this register region, all phy > controls > are through PHY register. > Other freescale SoCs have this usbmisc register region to control phy and > tune some > signal quality. This register region is from another 0x800 or (last > controller base address + 0x200) Looks good. I'll take Sascha's suggestion. Thanks Richard > > > Thanks > > Richard > > > > > > If the usbmisc property exists, you can return -EPROBE_DEFER until it > > is > > > available. If it doesn't exist, you just continue without usbmisc > > > support (i.MX28) > > > > > > 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 2/2] xhci: EHCI/XHCI ports switching on Intense-PC.
Hi Sarah, Attached dmidecode output of Intense-PC. Denis Turischev On 07/31/2012 01:34 AM, Sarah Sharp wrote: > Hi Denis, > > Can you send me the output of `sudo dmidecode`? I'd like to see if I > can make a more general patch apply to the Intense-PC. > > Sarah Sharp > # dmidecode 2.11 SMBIOS 2.7 present. 62 structures occupying 2533 bytes. Table at 0x000E0840. Handle 0x0004, DMI type 4, 42 bytes Processor Information Socket Designation: CPU Socket - soldered on-board Type: Central Processor Family: Core i7 Manufacturer: Intel(R) Corporation ID: A5 06 03 00 FF FB EB BF Signature: Type 0, Family 6, Model 58, Stepping 5 Flags: FPU (Floating-point unit on-chip) VME (Virtual mode extension) DE (Debugging extension) PSE (Page size extension) TSC (Time stamp counter) MSR (Model specific registers) PAE (Physical address extension) MCE (Machine check exception) CX8 (CMPXCHG8 instruction supported) APIC (On-chip APIC hardware supported) SEP (Fast system call) MTRR (Memory type range registers) PGE (Page global enable) MCA (Machine check architecture) CMOV (Conditional move instruction supported) PAT (Page attribute table) PSE-36 (36-bit page size extension) CLFSH (CLFLUSH instruction supported) DS (Debug store) ACPI (ACPI supported) MMX (MMX technology supported) FXSR (FXSAVE and FXSTOR instructions supported) SSE (Streaming SIMD extensions) SSE2 (Streaming SIMD extensions 2) SS (Self-snoop) HTT (Multi-threading) TM (Thermal monitor supported) PBE (Pending break enabled) Version: Genuine Intel(R) CPU @ 1.50GHz Voltage: 0.8 V External Clock: 100 MHz Max Speed: 2000 MHz Current Speed: 2000 MHz Status: Populated, Enabled Upgrade: Socket rPGA988B L1 Cache Handle: 0x0006 L2 Cache Handle: 0x0007 L3 Cache Handle: 0x0008 Serial Number: To Be Filled By O.E.M. Asset Tag: To Be Filled By O.E.M. Part Number: To Be Filled By O.E.M. Core Count: 2 Core Enabled: 2 Thread Count: 4 Characteristics: 64-bit capable Handle 0x0005, DMI type 7, 19 bytes Cache Information Socket Designation: L1-Cache Configuration: Enabled, Not Socketed, Level 1 Operational Mode: Write Through Location: Internal Installed Size: 32 kB Maximum Size: 32 kB Supported SRAM Types: Unknown Installed SRAM Type: Unknown Speed: Unknown Error Correction Type: Parity System Type: Data Associativity: 8-way Set-associative Handle 0x0006, DMI type 7, 19 bytes Cache Information Socket Designation: L1-Cache Configuration: Enabled, Not Socketed, Level 1 Operational Mode: Write Through Location: Internal Installed Size: 32 kB Maximum Size: 32 kB Supported SRAM Types: Unknown Installed SRAM Type: Unknown Speed: Unknown Error Correction Type: Parity System Type: Instruction Associativity: 8-way Set-associative Handle 0x0007, DMI type 7, 19 bytes Cache Information Socket Designation: L2-Cache Configuration: Enabled, Not Socketed, Level 2 Operational Mode: Write Through Location: Internal Installed Size: 256 kB Maximum Size: 256 kB Supported SRAM Types: Unknown Installed SRAM Type: Unknown Speed: Unknown Error Correction Type: Multi-bit ECC System Type: Unified Associativity: 8-way Set-associative Handle 0x0008, DMI type 7, 19 bytes Cache Information Socket Designation: L3-Cache Configuration: Enabled, Not Socketed, Level 3 Operational Mode: Write Back Location: Internal Installed Size: 4096 kB Maximum Size: 4096 kB Supported SRAM Types: Unknown Installed SRAM Type: Unknown Speed: Unknown Error Correction Type: Multi-bit ECC System Type: Unified Associativity: 16-way Set-associative Handle 0x, DMI type 0, 24 bytes BIOS Information Vendor: Phoenix Technologies Ltd. Version: CR_2.2.0.262 X64 Release Date: 07/06/2012 Address: 0xE Runtime Size: 128 kB ROM Size: 3072 kB Characteristics: PCI is supported BIOS is upgradeable BIOS shadowing is allowed Boot from C
Re: [PATCH v3] usb: chipidea: fix and improve dependencies if usb host or gadget support is built as module
On 07/20/2012 09:33 AM, Marc Kleine-Budde wrote: > Since commit "5e0aa49 usb: chipidea: use generic map/unmap routines", > the udc part of the chipidea driver needs the generic usb gadget helper > functions. If the chipidea driver with udc support is built into the > kernel and usb gadget is built a module, the linking of the kernel > fails with: > > drivers/built-in.o: In function `_hardware_dequeue': > drivers/usb/chipidea/udc.c:527: > undefined reference to `usb_gadget_unmap_request' > drivers/usb/chipidea/udc.c:1269: > undefined reference to `usb_gadget_unmap_request' > drivers/usb/chipidea/udc.c:1821: > undefined reference to `usb_del_gadget_udc' > drivers/usb/chipidea/udc.c:443: > undefined reference to `usb_gadget_map_request' > drivers/usb/chipidea/udc.c:1774: > undefined reference to `usb_add_gadget_udc' > > This patch changes the dependencies, so that udc support can only be > activated if the linux gadget support (USB_GADGET) is builtin or both > chipidea driver and USB_GADGET are modular. Same dependencies for the > chipidea host support and the linux host side USB support (USB). > > While there, fix the indention of chipidea the help text. > > Cc: Alexander Shishkin > Reviewed-by: Felipe Balbi > Signed-off-by: Marc Kleine-Budde > --- > Hello Greg, > > rebased to your usb-next tree. ping Marc > > Marc > > Changes since v1 > - Do not depend on USB host support anymore (tnx Felipe for the input) > - Fix help text indention > > Changes since v2 > - rebased to Greg's usb-next > > drivers/usb/chipidea/Kconfig |9 + > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/drivers/usb/chipidea/Kconfig b/drivers/usb/chipidea/Kconfig > index 8337fb5..47e499c 100644 > --- a/drivers/usb/chipidea/Kconfig > +++ b/drivers/usb/chipidea/Kconfig > @@ -1,9 +1,9 @@ > config USB_CHIPIDEA > tristate "ChipIdea Highspeed Dual Role Controller" > - depends on USB > + depends on USB || USB_GADGET > help > - Say Y here if your system has a dual role high speed USB > - controller based on ChipIdea silicon IP. Currently, only the > + Say Y here if your system has a dual role high speed USB > + controller based on ChipIdea silicon IP. Currently, only the > peripheral mode is supported. > > When compiled dynamically, the module will be called ci-hdrc.ko. > @@ -12,7 +12,7 @@ if USB_CHIPIDEA > > config USB_CHIPIDEA_UDC > bool "ChipIdea device controller" > - depends on USB_GADGET > + depends on USB_GADGET=y || USB_GADGET=USB_CHIPIDEA > select USB_GADGET_DUALSPEED > help > Say Y here to enable device controller functionality of the > @@ -20,6 +20,7 @@ config USB_CHIPIDEA_UDC > > config USB_CHIPIDEA_HOST > bool "ChipIdea host controller" > + depends on USB=y || USB=USB_CHIPIDEA > select USB_EHCI_ROOT_HUB_TT > help > Say Y here to enable host controller functionality of the > -- Pengutronix e.K. | Marc Kleine-Budde | Industrial Linux Solutions| Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917- | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | signature.asc Description: OpenPGP digital signature
Re: [alsa-devel] Alsa: USB Class 2 Audio soundcard device disappears suddenly
On 31.07.2012 12:12, Joao Bonina wrote: > > > It doesn't show anything when that happens. Well, it shows: [ 349.789200] hub 1-0:1.0: port 4 disabled by hub (EMI?), re-enabling... I guess this about the time when the streaming stops? How does your USB wiring setup look like and what kind of system is that? Smells like a hardware issue, to be honest. You could do some stress-testing by transferring huge amounts of data from one USB storage media to another. Also try replacing the cables maybe. I'm Cc'ing the linux-usb list, maybe someone there has an idea. Daniel > > Looking at dmesg (I'm attaching it), it shows this: > [ 350.170478] usb 1-4: config 1 has an invalid interface number: 3 but max > is 2 > [ 350.170500] usb 1-4: config 1 has no interface number 2 > [ 350.170843] usb 1-4: config 1 has an invalid interface number: 3 but max > is 2 > [ 350.170864] usb 1-4: config 1 has no interface number 2 > [ 350.171344] usb 1-4: New USB device found, idVendor=21b4, idProduct=0230 > > [ 350.171366] usb 1-4: New USB device strings: Mfr=1, Product=2, > SerialNumber=3 > [ 350.171385] usb 1-4: Product: Ayre USB Interface > [ 350.171401] usb 1-4: Manufacturer: Ayre Acoustics > [ 350.171418] usb 1-4: SerialNumber: (C) 2010 XMOS/Wavelength Audio > > > Don't know if those 'invalid interface number' messages are relevant for the > issue. > > Joao > > > - Original Message - > From: Daniel Mack > To: Joao Bonina > Cc: "alsa-de...@alsa-project.org" > Sent: Tuesday, 31 July 2012, 6:24 > Subject: Re: [alsa-devel] Alsa: USB Class 2 Audio soundcard device disappears > suddenly > > On 31.07.2012 00:32, Joao Bonina wrote: >> Hi, >> >> I'm using an USB Class 2 Audio DAC (basically a soundcard) with my Linux box >> and MPD software. >> >> It works great when playing files stored in a remote share, but when >> playing files from an USB pen, it stops playing in a few minutes. >> >> Mpd.log shows this: >> >> Code: >> Jul 30 13:03 : output: "USB" [alsa] failed to play: No such device >> Jul 30 13:03 : output: closed plugin=alsa name="USB" >> ALSA lib pcm_hw.c:1401:(_snd_pcm_hw_open) Invalid value for card >> Jul 30 13:03 : output: Failed to open "USB" [alsa]: Failed to open ALSA >> device "hw:0,0": No such file or directoryIt seems to be somewhat related to >> the USB bus not being able to >> cope with both PCM streaming to the soundcard and reading from the flash pen >> simultaneously, I guess. >> >> So, anybody faced this issue previously and can provide some hints to solve >> this? >> >> MPD is 0.17, kernel is 3.2.17, alsa-utils is 1.0.25. > > Does the kernel log or 'dmesg' show anything weird what that happens? > > > Daniel > [0.00] Initializing cgroup subsys cpuset [0.00] Initializing cgroup subsys cpu [0.00] Linux version 3.2.17-rt28 (root@debian) (gcc version 4.4.5 (Debian 4.4.5-8) ) #1 PREEMPT RT Sat Jul 28 23:51:42 WEST 2012 [0.00] KERNEL supported cpus: [0.00] AMD AuthenticAMD [0.00] BIOS-provided physical RAM map: [0.00] BIOS-e820: - 0009e800 (usable) [0.00] BIOS-e820: 0009e800 - 000a (reserved) [0.00] BIOS-e820: 000f - 0010 (reserved) [0.00] BIOS-e820: 0010 - 0f7c (usable) [0.00] BIOS-e820: - 0001 (reserved) [0.00] Notice: NX (Execute Disable) protection missing in CPU! [0.00] DMI not present or invalid. [0.00] e820 update range: - 0001 (usable) ==> (reserved) [0.00] e820 remove range: 000a - 0010 (usable) [0.00] last_pfn = 0xf7c0 max_arch_pfn = 0x10 [0.00] initial memory mapped : 0 - 0180 [0.00] Base memory trampoline at [c009d000] 9d000 size 4096 [0.00] init_memory_mapping: -0f7c [0.00] 00 - 40 page 4k [0.00] 40 - 000f40 page 2M [0.00] 000f40 - 000f7c page 4k [0.00] kernel direct mapping tables up to f7c @ 17fb000-180 [0.00] RAMDISK: 0b7cd000 - 0ba1 [0.00] ACPI Error: A valid RSDP was not found (20110623/tbxfroot-219) [0.00] 0MB HIGHMEM available. [0.00] 247MB LOWMEM available. [0.00] mapped low ram: 0 - 0f7c [0.00] low ram: 0 - 0f7c [0.00] Zone PFN ranges: [0.00] DMA 0x0010 -> 0x1000 [0.00] Normal 0x1000 -> 0xf7c0 [0.00] HighMem empty [0.00] Movable zone start PFN for each node [0.00] early_node_map[2] active PFN ranges [0.00] 0: 0x0010 -> 0x009e [0.00] 0: 0x0100 -> 0xf7c0 [0.00] On node 0 totalpages: 63310 [0.00] free_area_init_node: node 0, pgdat c13303e0, node_mem_map cf5cf200 [0.00] DMA zone: 32 pages used for memma
Re: [alsa-devel] Alsa: USB Class 2 Audio soundcard device disappears suddenly
You're right about the EMI message, it's happening when the streaming stops. I've tried different cables before, but that didn't solve the problem. The motherboard is an Alix 1D, and I'm using the onboard USB 2.0 ports. If this really is an EMI issue, guess I'll have to source some kind of miniPCI-USB 2.0 card and see if that solves the problem. - Original Message - From: Daniel Mack To: Joao Bonina Cc: "alsa-de...@alsa-project.org" ; linux-usb Sent: Tuesday, 31 July 2012, 12:38 Subject: Re: [alsa-devel] Alsa: USB Class 2 Audio soundcard device disappears suddenly On 31.07.2012 12:12, Joao Bonina wrote: > > > It doesn't show anything when that happens. Well, it shows: [ 349.789200] hub 1-0:1.0: port 4 disabled by hub (EMI?), re-enabling... I guess this about the time when the streaming stops? How does your USB wiring setup look like and what kind of system is that? Smells like a hardware issue, to be honest. You could do some stress-testing by transferring huge amounts of data from one USB storage media to another. Also try replacing the cables maybe. I'm Cc'ing the linux-usb list, maybe someone there has an idea. Daniel > > Looking at dmesg (I'm attaching it), it shows this: > [ 350.170478] usb 1-4: config 1 has an invalid interface number: 3 but max > is 2 > [ 350.170500] usb 1-4: config 1 has no interface number 2 > [ 350.170843] usb 1-4: config 1 has an invalid interface number: 3 but max > is 2 > [ 350.170864] usb 1-4: config 1 has no interface number 2 > [ 350.171344] usb 1-4: New USB device found, idVendor=21b4, idProduct=0230 > > [ 350.171366] usb 1-4: New USB device strings: Mfr=1, Product=2, > SerialNumber=3 > [ 350.171385] usb 1-4: Product: Ayre USB Interface > [ 350.171401] usb 1-4: Manufacturer: Ayre Acoustics > [ 350.171418] usb 1-4: SerialNumber: (C) 2010 XMOS/Wavelength Audio > > > Don't know if those 'invalid interface number' messages are relevant for the > issue. > > Joao > > > - Original Message - > From: Daniel Mack > To: Joao Bonina > Cc: "alsa-de...@alsa-project.org" > Sent: Tuesday, 31 July 2012, 6:24 > Subject: Re: [alsa-devel] Alsa: USB Class 2 Audio soundcard device disappears > suddenly > > On 31.07.2012 00:32, Joao Bonina wrote: >> Hi, >> >> I'm using an USB Class 2 Audio DAC (basically a soundcard) with my Linux box >> and MPD software. >> >> It works great when playing files stored in a remote share, but when >> playing files from an USB pen, it stops playing in a few minutes. >> >> Mpd.log shows this: >> >> Code: >> Jul 30 13:03 : output: "USB" [alsa] failed to play: No such device >> Jul 30 13:03 : output: closed plugin=alsa name="USB" >> ALSA lib pcm_hw.c:1401:(_snd_pcm_hw_open) Invalid value for card >> Jul 30 13:03 : output: Failed to open "USB" [alsa]: Failed to open ALSA >> device "hw:0,0": No such file or directoryIt seems to be somewhat related to >> the USB bus not being able to >> cope with both PCM streaming to the soundcard and reading from the flash pen >> simultaneously, I guess. >> >> So, anybody faced this issue previously and can provide some hints to solve >> this? >> >> MPD is 0.17, kernel is 3.2.17, alsa-utils is 1.0.25. > > Does the kernel log or 'dmesg' show anything weird what that happens? > > > Daniel > ___ Alsa-devel mailing list alsa-de...@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel -- 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: echi-dbgp: increase the controller wait time to come out of halt.
On 07/30/2012 10:06 AM, Colin Ian King wrote: > The default 10 microsecond delay for the controller to come out of > halt in dbgp_ehci_startup is too short, so increase it to 1 millisecond. > > This is based on emperical testing on various USB debug ports on > modern machines such as a Lenovo X220i and an Ivybridge development > platform that needed to wait ~450-950 microseconds. This seems fine to me. I'll test it, merge it, and add a CC for -stable. Thanks, Jason. -- 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 v6 03/11] usb: musb: am335x: add support for dual instance
Hi,
On Fri, Jul 27, 2012 at 02:01:59PM +0530, Ravi B wrote:
> From: Ajay Kumar Gupta
>
> AM335x and TI81xx platform has dual musb controller so updating the
> musb_dspc.c to support the same.
>
> Changes:
> - Moved otg_workaround timer to glue structure
> - Moved static local variable last_timer to glue structure
> - PHY on/off related cleanups
>
> Signed-off-by: Ajay Kumar Gupta
> Signed-off-by: Ravi B
> ---
> drivers/usb/musb/musb_dsps.c | 118 +
> 1 files changed, 72 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c
> index 2174699..2fd5dc8 100644
> --- a/drivers/usb/musb/musb_dsps.c
> +++ b/drivers/usb/musb/musb_dsps.c
> @@ -105,6 +105,8 @@ struct dsps_musb_wrapper {
> /* miscellaneous stuff */
> u32 musb_core_offset;
> u8 poll_seconds;
> + /* number of musb instances */
> + u8 instances;
> };
>
> /**
> @@ -112,16 +114,18 @@ struct dsps_musb_wrapper {
> */
> struct dsps_glue {
> struct device *dev;
> - struct platform_device *musb; /* child musb pdev */
> + struct platform_device *musb[2];/* child musb pdev */
> const struct dsps_musb_wrapper *wrp; /* wrapper register offsets */
> - struct timer_list timer;/* otg_workaround timer */
> - u32 __iomem *usb_ctrl;
> + struct timer_list timer[2]; /* otg_workaround timer */
> + unsigned long last_timer[2];/* last timer data for each instance */
> + u32 __iomem *usb_ctrl[2];
> u8 usbss_rev;
> };
>
> /**
> * musb_dsps_phy_control - phy on/off
> * @glue: struct dsps_glue *
> + * @id: musb instance
> * @on: flag for phy to be switched on or off
> *
> * This is to enable the PHY using usb_ctrl register in system control
> @@ -130,11 +134,11 @@ struct dsps_glue {
> * XXX: This function will be removed once we have a seperate driver for
> * control module
> */
> -static void musb_dsps_phy_control(struct dsps_glue *glue, u8 on)
> +static void musb_dsps_phy_control(struct dsps_glue *glue, u8 id, u8 on)
> {
> u32 usbphycfg;
>
> - usbphycfg = __raw_readl(glue->usb_ctrl);
> + usbphycfg = __raw_readl(glue->usb_ctrl[id]);
>
> if (on) {
> if (glue->usbss_rev == MUSB_USBSS_REV_816X) {
> @@ -157,7 +161,7 @@ static void musb_dsps_phy_control(struct dsps_glue *glue,
> u8 on)
> glue->usbss_rev == MUSB_USBSS_REV_33XX)
> usbphycfg |= USBPHY_CM_PWRDN | USBPHY_OTG_PWRDN;
> }
> - __raw_writel(usbphycfg, glue->usb_ctrl);
> + __raw_writel(usbphycfg, glue->usb_ctrl[id]);
> }
> /**
> * dsps_musb_enable - enable interrupts
> @@ -207,8 +211,9 @@ static void otg_timer(unsigned long _musb)
> struct musb *musb = (void *)_musb;
> void __iomem *mregs = musb->mregs;
> struct device *dev = musb->controller;
> - struct platform_device *pdev = to_platform_device(dev->parent);
> - struct dsps_glue *glue = platform_get_drvdata(pdev);
> + struct platform_device *pdev = to_platform_device(dev);
> + struct platform_device *parent_pdev = to_platform_device(dev->parent);
> + struct dsps_glue *glue = platform_get_drvdata(parent_pdev);
> const struct dsps_musb_wrapper *wrp = glue->wrp;
> u8 devctl;
> unsigned long flags;
> @@ -247,7 +252,7 @@ static void otg_timer(unsigned long _musb)
>
> devctl = dsps_readb(mregs, MUSB_DEVCTL);
> if (devctl & MUSB_DEVCTL_BDEVICE)
> - mod_timer(&glue->timer,
> + mod_timer(&glue->timer[pdev->id],
> jiffies + wrp->poll_seconds * HZ);
> else
> musb->xceiv->state = OTG_STATE_A_IDLE;
> @@ -261,9 +266,9 @@ static void otg_timer(unsigned long _musb)
> static void dsps_musb_try_idle(struct musb *musb, unsigned long timeout)
> {
> struct device *dev = musb->controller;
> - struct platform_device *pdev = to_platform_device(dev->parent);
> - struct dsps_glue *glue = platform_get_drvdata(pdev);
> - static unsigned long last_timer;
> + struct platform_device *pdev = to_platform_device(dev);
> + struct platform_device *parent_pdev = to_platform_device(dev->parent);
> + struct dsps_glue *glue = platform_get_drvdata(parent_pdev);
just one thing that could be cleaned on a later patch:
if parent_pdev is only used to get to struct dsps_glue, you could just:
struct dsps_glue *glue = dev_get_drvdata(dev->parent);
with no need to do a container_of() to the platform_device ;-)
--
balbi
signature.asc
Description: Digital signature
RE: [PATCH v6 03/11] usb: musb: am335x: add support for dual instance
> Hi,
>
> On Fri, Jul 27, 2012 at 02:01:59PM +0530, Ravi B wrote:
> > From: Ajay Kumar Gupta
> >
> > AM335x and TI81xx platform has dual musb controller so updating the
> > musb_dspc.c to support the same.
> >
> > Changes:
> > - Moved otg_workaround timer to glue structure
> > - Moved static local variable last_timer to glue structure
> > - PHY on/off related cleanups
> >
> > Signed-off-by: Ajay Kumar Gupta
> > Signed-off-by: Ravi B
> > ---
> > drivers/usb/musb/musb_dsps.c | 118
> > +
> > 1 files changed, 72 insertions(+), 46 deletions(-)
> >
> > diff --git a/drivers/usb/musb/musb_dsps.c
> > b/drivers/usb/musb/musb_dsps.c index 2174699..2fd5dc8 100644
> > --- a/drivers/usb/musb/musb_dsps.c
> > +++ b/drivers/usb/musb/musb_dsps.c
> > @@ -105,6 +105,8 @@ struct dsps_musb_wrapper {
> > /* miscellaneous stuff */
> > u32 musb_core_offset;
> > u8 poll_seconds;
> > + /* number of musb instances */
> > + u8 instances;
> > };
> >
> > /**
> > @@ -112,16 +114,18 @@ struct dsps_musb_wrapper {
> > */
> > struct dsps_glue {
> > struct device *dev;
> > - struct platform_device *musb; /* child musb pdev */
> > + struct platform_device *musb[2];/* child musb pdev */
> > const struct dsps_musb_wrapper *wrp; /* wrapper
> register offsets */
> > - struct timer_list timer;/* otg_workaround timer */
> > - u32 __iomem *usb_ctrl;
> > + struct timer_list timer[2]; /* otg_workaround timer */
> > + unsigned long last_timer[2];/* last timer data for
> each instance */
> > + u32 __iomem *usb_ctrl[2];
> > u8 usbss_rev;
> > };
> >
> > /**
> > * musb_dsps_phy_control - phy on/off
> > * @glue: struct dsps_glue *
> > + * @id: musb instance
> > * @on: flag for phy to be switched on or off
> > *
> > * This is to enable the PHY using usb_ctrl register in system
> > control @@ -130,11 +134,11 @@ struct dsps_glue {
> > * XXX: This function will be removed once we have a
> seperate driver for
> > * control module
> > */
> > -static void musb_dsps_phy_control(struct dsps_glue *glue, u8 on)
> > +static void musb_dsps_phy_control(struct dsps_glue *glue,
> u8 id, u8
> > +on)
> > {
> > u32 usbphycfg;
> >
> > - usbphycfg = __raw_readl(glue->usb_ctrl);
> > + usbphycfg = __raw_readl(glue->usb_ctrl[id]);
> >
> > if (on) {
> > if (glue->usbss_rev == MUSB_USBSS_REV_816X) {
> @@ -157,7 +161,7 @@
> > static void musb_dsps_phy_control(struct dsps_glue *glue, u8 on)
> > glue->usbss_rev == MUSB_USBSS_REV_33XX)
> > usbphycfg |= USBPHY_CM_PWRDN | USBPHY_OTG_PWRDN;
> > }
> > - __raw_writel(usbphycfg, glue->usb_ctrl);
> > + __raw_writel(usbphycfg, glue->usb_ctrl[id]);
> > }
> > /**
> > * dsps_musb_enable - enable interrupts @@ -207,8 +211,9 @@ static
> > void otg_timer(unsigned long _musb)
> > struct musb *musb = (void *)_musb;
> > void __iomem *mregs = musb->mregs;
> > struct device *dev = musb->controller;
> > - struct platform_device *pdev = to_platform_device(dev->parent);
> > - struct dsps_glue *glue = platform_get_drvdata(pdev);
> > + struct platform_device *pdev = to_platform_device(dev);
> > + struct platform_device *parent_pdev =
> to_platform_device(dev->parent);
> > + struct dsps_glue *glue = platform_get_drvdata(parent_pdev);
> > const struct dsps_musb_wrapper *wrp = glue->wrp;
> > u8 devctl;
> > unsigned long flags;
> > @@ -247,7 +252,7 @@ static void otg_timer(unsigned long _musb)
> >
> > devctl = dsps_readb(mregs, MUSB_DEVCTL);
> > if (devctl & MUSB_DEVCTL_BDEVICE)
> > - mod_timer(&glue->timer,
> > + mod_timer(&glue->timer[pdev->id],
> > jiffies +
> wrp->poll_seconds * HZ);
> > else
> > musb->xceiv->state = OTG_STATE_A_IDLE;
> @@ -261,9 +266,9 @@ static
> > void otg_timer(unsigned long _musb) static void
> > dsps_musb_try_idle(struct musb *musb, unsigned long timeout) {
> > struct device *dev = musb->controller;
> > - struct platform_device *pdev = to_platform_device(dev->parent);
> > - struct dsps_glue *glue = platform_get_drvdata(pdev);
> > - static unsigned long last_timer;
> > + struct platform_device *pdev = to_platform_device(dev);
> > + struct platform_device *parent_pdev =
> to_platform_device(dev->parent);
> > + struct dsps_glue *glue = platform_get_drvdata(parent_pdev);
>
> just one thing that could be cleaned on a later patch:
>
> if parent_pdev is only used to get to struct dsps_glue, you
> could just:
>
> struct dsps_glue *glue = dev_get_drvdata(dev->parent);
>
> with no need to do a container_of() to the platform_device ;-)
Yes Felipe, parent_pdev is only to get dsps_glue. I will include this change in
later patch.
RaviBabu
>
> --
> balbi
> --
To u
Re: [PATCH 2/3] usb: Take attribute avoid_reset_quirk out of usb device's attribute group
On Monday 30 July 2012 17:03:27 Alan Stern wrote: > On Mon, 30 Jul 2012, Sarah Sharp wrote: > I'm not even sure that RESET_MORPHS is the right thing to look at here. > It's possible for a device to retain important settings across a reset > but lose them when power is removed. True. So an additional quirk? QUIRK_FIRMWARE_VOLATILE? [..] > >2. If the device is internal and suspended, power off the port if all > > the following are true: > > > > a) all interface drivers have supports_power_off set, or no > > interface drivers are bound and usbfs has not claimed the > > device. > > b) remote wakeup is disabled > > c) USB_QUIRK_RESET_MORPHS is not set > > > > - If userspace wants a port to be powered off, and one of the interface > >drivers doesn't set supports_power_off or the driver enables remote > >wakeup, then userspace will need to unbind or unload the driver. > > Like other people, I'm dubious about these conditions. Can you propose other conditions? > I tend to agree that having the kernel make these decisions is fraught > with difficulties, except in the most simple cases (unpluggable and no > device present). Yes. > Exposing the extra attributes to userspace can't hurt -- unless we want > to change or remove them in the future! These are two different things. The attributes are defined by ACPI. We have little say in how they would look like. But the API for controlling power off is an open question. Regards Oliver -- 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: [alsa-devel] Alsa: USB Class 2 Audio soundcard device disappears suddenly
A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing? A: Top-posting. Q: What is the most annoying thing in email? On 31.07.2012 14:04, Joao Bonina wrote: > You're right about the EMI message, it's happening when the streaming stops. > > I've tried different cables before, but that didn't solve the problem. > > The motherboard is an Alix 1D, and I'm using the onboard USB 2.0 ports. > > If this really is an EMI issue, guess I'll have to source some kind of > miniPCI-USB 2.0 card and see if that solves the problem. Yes, try that. It could still be something else than an EMI issue, but something makes the hub reset its ports. Let us know what you find! Daniel > - Original Message - > From: Daniel Mack > To: Joao Bonina > Cc: "alsa-de...@alsa-project.org" ; linux-usb > > Sent: Tuesday, 31 July 2012, 12:38 > Subject: Re: [alsa-devel] Alsa: USB Class 2 Audio soundcard device disappears > suddenly > > On 31.07.2012 12:12, Joao Bonina wrote: >> >> >> It doesn't show anything when that happens. > > Well, it shows: > > [ 349.789200] hub 1-0:1.0: port 4 disabled by hub (EMI?), re-enabling... > > I guess this about the time when the streaming stops? How does your USB > wiring setup look like and what kind of system is that? Smells like a > hardware issue, to be honest. You could do some stress-testing by > transferring huge amounts of data from one USB storage media to another. > Also try replacing the cables maybe. > > I'm Cc'ing the linux-usb list, maybe someone there has an idea. > > > Daniel > > >> >> Looking at dmesg (I'm attaching it), it shows this: >> [ 350.170478] usb 1-4: config 1 has an invalid interface number: 3 but max >> is 2 >> [ 350.170500] usb 1-4: config 1 has no interface number 2 >> [ 350.170843] usb 1-4: config 1 has an invalid interface number: 3 but max >> is 2 >> [ 350.170864] usb 1-4: config 1 has no interface number 2 >> [ 350.171344] usb 1-4: New USB device found, idVendor=21b4, idProduct=0230 >> >> [ 350.171366] usb 1-4: New USB device strings: Mfr=1, Product=2, >> SerialNumber=3 >> [ 350.171385] usb 1-4: Product: Ayre USB Interface >> [ 350.171401] usb 1-4: Manufacturer: Ayre Acoustics >> [ 350.171418] usb 1-4: SerialNumber: (C) 2010 XMOS/Wavelength Audio >> >> >> Don't know if those 'invalid interface number' messages are relevant for the >> issue. >> >> Joao >> >> >> - Original Message - >> From: Daniel Mack >> To: Joao Bonina >> Cc: "alsa-de...@alsa-project.org" >> Sent: Tuesday, 31 July 2012, 6:24 >> Subject: Re: [alsa-devel] Alsa: USB Class 2 Audio soundcard device >> disappears suddenly >> >> On 31.07.2012 00:32, Joao Bonina wrote: >>> Hi, >>> >>> I'm using an USB Class 2 Audio DAC (basically a soundcard) with my Linux >>> box and MPD software. >>> >>> It works great when playing files stored in a remote share, but when >>> playing files from an USB pen, it stops playing in a few minutes. >>> >>> Mpd.log shows this: >>> >>> Code: >>> Jul 30 13:03 : output: "USB" [alsa] failed to play: No such device >>> Jul 30 13:03 : output: closed plugin=alsa name="USB" >>> ALSA lib pcm_hw.c:1401:(_snd_pcm_hw_open) Invalid value for card >>> Jul 30 13:03 : output: Failed to open "USB" [alsa]: Failed to open ALSA >>> device "hw:0,0": No such file or directoryIt seems to be somewhat related >>> to the USB bus not being able to >>> cope with both PCM streaming to the soundcard and reading from the flash >>> pen simultaneously, I guess. >>> >>> So, anybody faced this issue previously and can provide some hints to solve >>> this? >>> >>> MPD is 0.17, kernel is 3.2.17, alsa-utils is 1.0.25. >> >> Does the kernel log or 'dmesg' show anything weird what that happens? >> >> >> Daniel >> > > > ___ > Alsa-devel mailing list > alsa-de...@alsa-project.org > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel > -- 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: [alsa-devel] Alsa: USB Class 2 Audio soundcard device disappears suddenly
On Tue, 31 Jul 2012, Joao Bonina wrote: > You're right about the EMI message, it's happening when the streaming stops. > > I've tried different cables before, but that didn't solve the problem. > > The motherboard is an Alix 1D, and I'm using the onboard USB 2.0 ports. > > If this really is an EMI issue, guess I'll have to source some kind of > miniPCI-USB 2.0 card and see if that solves the problem. Another possibility is to put a USB hub before the sound device. That _might_ clean up the signal. 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 3/3] usb: Change persist_enabled when attribute avoid_reset_quirk is modified
On Tue, 31 Jul 2012, Lan Tianyu wrote: > How about checking RESET_MORPHS before doing reset_resume, set reset_resume > to 0 and do resume when RESET_MORPHS is set. No, that won't work. When we do a reset-resume it is because we _know_ that a regular resume will fail. > At the same time, print "Convert > reset_resume to resume due to RESET_MORPHS". Then these two attributes are > separated but for reset-resume, there are two conditions. persist is true > RESET_MORPHS is unset. I think the best course is to leave things the way they are. Just add an explanation to persist.txt that the Persist mechanism is likely to fail if the avoid_reset_quirk attribute is set. 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 2/3] usb: Take attribute avoid_reset_quirk out of usb device's attribute group
On Tue, 31 Jul 2012, Bjørn Mork wrote: > Alan Stern writes: > > On Mon, 30 Jul 2012, Sarah Sharp wrote: > > > >> - I'm not sure if it's true that all devices that need firmware will > >>have USB_QUIRK_RESET_MORPHS set. Alan, Oliver? > > > > Probably not, and in any case the situation is more complex than it > > first appears. (In fact, I wonder whether we really need that quirk > > flag at all -- as far as I can tell it's not doing anybody any good.) > > usb_modeswitch use it to prevent usb-storage from resetting the device > on errors. Some 3G modems expose a storage interface (SD card reader > and/or embedded driver CD) even after switching to modem mode. You do > not want them to switch back from modem mode on every usb-storage > problem, especially as you may assume the device firmware is buggy. Some > devices would probably end up in a continuous reset loop without this > quirk. Okay. Guess I should have known that. > IMHO that should have been the default for any composite device, but > whatever.. I don't think it's very nice of usb-storage to reset the > device if there are drivers bound to other functions. That's why we have pre_reset and post_reset callbacks. The alternative is to lose access to all the mounted filesystems on the mass-storage device, which isn't very nice either. 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 2/3] usb: Take attribute avoid_reset_quirk out of usb device's attribute group
Alan Stern writes: > On Tue, 31 Jul 2012, Bjørn Mork wrote: > >> IMHO that should have been the default for any composite device, but >> whatever.. I don't think it's very nice of usb-storage to reset the >> device if there are drivers bound to other functions. > > That's why we have pre_reset and post_reset callbacks. The alternative > is to lose access to all the mounted filesystems on the mass-storage > device, which isn't very nice either. Well, you are then saying that usb-storage has priority over any other function and that is often wrong IMHO. There are many newer composite devices where the usb-storage function is thrown in as a bonus most users never will touch, where the main function can be modem or printer or whatever. Letting the usb-storage driver reset these devices is bad. Ref for example http://comments.gmane.org/gmane.linux.usb.general/60055 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: [PATCH 2/3] usb: Take attribute avoid_reset_quirk out of usb device's attribute group
On Tuesday 31 July 2012 16:54:13 Bjørn Mork wrote: > Alan Stern writes: > > On Tue, 31 Jul 2012, Bjørn Mork wrote: > > > >> IMHO that should have been the default for any composite device, but > >> whatever.. I don't think it's very nice of usb-storage to reset the > >> device if there are drivers bound to other functions. > > > > That's why we have pre_reset and post_reset callbacks. The alternative > > is to lose access to all the mounted filesystems on the mass-storage > > device, which isn't very nice either. > > Well, you are then saying that usb-storage has priority over any other > function and that is often wrong IMHO. There are many newer composite > devices where the usb-storage function is thrown in as a bonus most > users never will touch, where the main function can be modem or printer > or whatever. Letting the usb-storage driver reset these devices is bad. > > Ref for example http://comments.gmane.org/gmane.linux.usb.general/60055 User space can set the quirk. It wasn't intended for this, but it does the job. Furthermore on such devices, storage could be a bot more cooperative and try the class specific reset first. Regards Oliver -- 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 v3] usb: chipidea: fix and improve dependencies if usb host or gadget support is built as module
On Tue, Jul 31, 2012 at 01:16:01PM +0200, Marc Kleine-Budde wrote: > On 07/20/2012 09:33 AM, Marc Kleine-Budde wrote: > > Since commit "5e0aa49 usb: chipidea: use generic map/unmap routines", > > the udc part of the chipidea driver needs the generic usb gadget helper > > functions. If the chipidea driver with udc support is built into the > > kernel and usb gadget is built a module, the linking of the kernel > > fails with: > > > > drivers/built-in.o: In function `_hardware_dequeue': > > drivers/usb/chipidea/udc.c:527: > > undefined reference to `usb_gadget_unmap_request' > > drivers/usb/chipidea/udc.c:1269: > > undefined reference to `usb_gadget_unmap_request' > > drivers/usb/chipidea/udc.c:1821: > > undefined reference to `usb_del_gadget_udc' > > drivers/usb/chipidea/udc.c:443: > > undefined reference to `usb_gadget_map_request' > > drivers/usb/chipidea/udc.c:1774: > > undefined reference to `usb_add_gadget_udc' > > > > This patch changes the dependencies, so that udc support can only be > > activated if the linux gadget support (USB_GADGET) is builtin or both > > chipidea driver and USB_GADGET are modular. Same dependencies for the > > chipidea host support and the linux host side USB support (USB). > > > > While there, fix the indention of chipidea the help text. > > > > Cc: Alexander Shishkin > > Reviewed-by: Felipe Balbi > > Signed-off-by: Marc Kleine-Budde > > --- > > Hello Greg, > > > > rebased to your usb-next tree. > > ping You just pinged yourself? That's odd :) Anyway, I'll get to it after 3.6-rc1 is out. 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 v3] usb: chipidea: fix and improve dependencies if usb host or gadget support is built as module
On 07/31/2012 05:21 PM, Greg KH wrote: [...] >>> Hello Greg, >>> >>> rebased to your usb-next tree. >> >> ping > > You just pinged yourself? That's odd :) You're on Cc, and it actually worked :) > > Anyway, I'll get to it after 3.6-rc1 is out. tnx, Marc -- Pengutronix e.K. | Marc Kleine-Budde | Industrial Linux Solutions| Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917- | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | signature.asc Description: OpenPGP digital signature
Re: [PATCH 2/3] usb: Take attribute avoid_reset_quirk out of usb device's attribute group
On Tue, 31 Jul 2012, Bjørn Mork wrote: > Alan Stern writes: > > On Tue, 31 Jul 2012, Bjørn Mork wrote: > > > >> IMHO that should have been the default for any composite device, but > >> whatever.. I don't think it's very nice of usb-storage to reset the > >> device if there are drivers bound to other functions. > > > > That's why we have pre_reset and post_reset callbacks. The alternative > > is to lose access to all the mounted filesystems on the mass-storage > > device, which isn't very nice either. > > Well, you are then saying that usb-storage has priority over any other > function and that is often wrong IMHO. No, I'm not saying that at all. The situation is perfectly symmetric. Just as usb-storage is free to interrupt other drivers by doing a reset, so the other drivers are free to interrupt usb-storage by doing a reset. > There are many newer composite > devices where the usb-storage function is thrown in as a bonus most > users never will touch, where the main function can be modem or printer > or whatever. Letting the usb-storage driver reset these devices is bad. > > Ref for example http://comments.gmane.org/gmane.linux.usb.general/60055 If the mass-storage function isn't going to be used then usb-storage could be unbound from the device. Or (a better solution IMO), if the manufacturers would design their mass-storage functions without bugs then no resets would be needed. Another alternative is for the other drivers to delay the reset (by sleeping in their pre_reset callbacks) until they can handle the reset safely. The kernel doesn't know which functions the user is interested in. It has no basis for saying "The user doesn't care about the mass-storage function so don't let usb-storage issue any device resets". 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 2/3] usb: Take attribute avoid_reset_quirk out of usb device's attribute group
On Tue, 31 Jul 2012, Oliver Neukum wrote: > > Well, you are then saying that usb-storage has priority over any other > > function and that is often wrong IMHO. There are many newer composite > > devices where the usb-storage function is thrown in as a bonus most > > users never will touch, where the main function can be modem or printer > > or whatever. Letting the usb-storage driver reset these devices is bad. > > > > Ref for example http://comments.gmane.org/gmane.linux.usb.general/60055 > > User space can set the quirk. It wasn't intended for this, but it does the > job. Furthermore on such devices, storage could be a bot more cooperative > and try the class specific reset first. The class-specific reset almost never works. Windows doesn't use it, so manufacturers don't implement it correctly. 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: host: ehci-platform: add pm_runtime_xx()
On Mon, 30 Jul 2012, Kuninori Morimoto wrote:
> this patch enable to use pm_runtime_xxx() on ehci-platform
> if .config has CONFIG_PM_RUNTIME, otherwise, these are ignored
>
> Signed-off-by: Kuninori Morimoto
> ---
> drivers/usb/host/ehci-platform.c |6 ++
> 1 files changed, 6 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/usb/host/ehci-platform.c
> b/drivers/usb/host/ehci-platform.c
> index dfe881a..89029eb 100644
> --- a/drivers/usb/host/ehci-platform.c
> +++ b/drivers/usb/host/ehci-platform.c
> @@ -122,6 +122,9 @@ static int __devinit ehci_platform_probe(struct
> platform_device *dev)
> if (err)
> goto err_iounmap;
>
> + pm_runtime_enable(&dev->dev);
> + pm_runtime_get_sync(&dev->dev);
This line is peculiar. Why call pm_runtime_get_sync()? If the
controller was at low power before, the usb_add_hcd() call above would
have failed. If it was at full power before, this call isn't needed.
Also it leaves the runtime PM usage counter set to 1, which will
prevent the controller from runtime-suspending.
I think what you want to do is more like this:
pm_runtime_set_active(&dev->dev);
pm_runtime_enable(&dev->dev);
> +
> platform_set_drvdata(dev, hcd);
>
> return err;
> @@ -139,6 +142,9 @@ static int __devexit ehci_platform_remove(struct
> platform_device *dev)
> {
> struct usb_hcd *hcd = platform_get_drvdata(dev);
>
> + pm_runtime_put_sync(&dev->dev);
> + pm_runtime_disable(&dev->dev);
These look wrong also. The put_sync will set the controller to low
power, which will cause usb_remove_hcd() below to fail.
Probably you should have:
pm_runtime_get_sync(&dev->dev);
> usb_remove_hcd(hcd);
Then here do:
pm_runtime_disable(&dev->dev);
pm_runtime_put_nosuspend(&dev->dev);
pm_runtime_set_suspended(&dev->dev);
> iounmap(hcd->regs);
> release_mem_region(hcd->rsrc_start, hcd->rsrc_len);
Similar comments apply to your patch for ohci-platform.c.
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 2/3] usb: Take attribute avoid_reset_quirk out of usb device's attribute group
On Tue, 31 Jul 2012, Oliver Neukum wrote: > On Monday 30 July 2012 17:03:27 Alan Stern wrote: > > On Mon, 30 Jul 2012, Sarah Sharp wrote: > > > I'm not even sure that RESET_MORPHS is the right thing to look at here. > > It's possible for a device to retain important settings across a reset > > but lose them when power is removed. > > True. So an additional quirk? QUIRK_FIRMWARE_VOLATILE? To be consistent we should call it QUIRK_POWEROFF_MORPHS. But how is this quirk going to get set? Static entries in the quirks table? Dynamically by firmware loaders? > > >2. If the device is internal and suspended, power off the port if all > > > the following are true: > > > > > > a) all interface drivers have supports_power_off set, or no > > >interface drivers are bound and usbfs has not claimed the > > >device. > > > b) remote wakeup is disabled > > > c) USB_QUIRK_RESET_MORPHS is not set > > > > > > - If userspace wants a port to be powered off, and one of the interface > > >drivers doesn't set supports_power_off or the driver enables remote > > >wakeup, then userspace will need to unbind or unload the driver. > > > > Like other people, I'm dubious about these conditions. > > Can you propose other conditions? I don't know. I can't think of anything more, but there's a nagging feeling that if we use these then something will go wrong. How about if we start with only the first policy rule: Ports that can never have anything plugged in should be shut down automatically. Initially, everything else can be left up to userspace. Then we can experiment with adding other policies to the kernel. 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 2/2] xhci: EHCI/XHCI ports switching on Intense-PC.
On Tue, Jul 31, 2012 at 06:49:50AM +0200, Oliver Neukum wrote: > On Monday 30 July 2012 15:34:06 Sarah Sharp wrote: > > Hi Denis, > > > > Can you send me the output of `sudo dmidecode`? I'd like to see if I > > can make a more general patch apply to the Intense-PC. > > As this is for shutdown, why not all systems? Because it will cause a BIOS delay on the next boot. Approximately 100 ms. Some people trying to do extremely fast boot will care about that delay. Sarah Sharp -- 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: bisected regression, v3.5 -> next-20120724: PCI PM causes USB hotplug failure
On Tue, 31 Jul 2012, Huang Ying wrote: > > What happens when you run lspci and the device is in D3cold? Then even > > if the parent bridge is active, lspci will still fail. > > > > It seems that in this case you need to resume the device itself, not > > just its parent. > > How about the following patch? > > Subject: [BUGFIX] PCI/PM: Keep parent bridge active when read/write config reg > > This patch fixes the following bug: > > http://marc.info/?l=linux-pci&m=134338059022620&w=2 > > Where lspci does not work properly if a device and the corresponding > parent bridge (such as PCIe port) is suspended. This is because the > device configuration space registers will be not accessible if the > corresponding parent bridge is suspended or the device is put into > D3cold state. > > To solve the issue, the bridge/PCIe port connected to the device is > put into active state before read/write configuration space registers. > If the device is in D3cold state, it will be put into active state > too. > > To avoid resume/suspend PCIe port for each configuration register > read/write, a small delay is added before the PCIe port to go > suspended. The patch looks reasonable to me, but I haven't tested 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
Problems with a Zytronic touchscreen on kernel 3.5 (USB ID 0x14c8:0x0005) - regression from kernel 3.0
Hello, I'm trying to get a Zytronic single-touch touchscreen to work with Linux kernel 3.5; it previously worked on a 3.0 kernel. I'm happy to try git kernels and patches, even if they're just intended to gather more information. hid-multitouch is picking up the device because it has multitouch report descriptors as well as the single-touch descriptors. There's a quirk to make it use MT_CLS_SERIAL, but that seems to be insufficient for my needs. I've got as far as determining that it's mt_event in hid-multitouch.c that consumes the events, but as the screen never sends the events hid-multitouch.c expects to see, mt_event nevers sends an input event. evtest on 3.0 shows touch events as expected. evtest on 3.5 shows no touch events. I've attached /sys/kernel/debug/hid/0003:14C8:0005.0001/rdesc as zyntronic.rdesc and /sys/kernel/debug/hid/0003:14C8:0005.0001/events while I touch the screen as zytronic.events. I had evtest /dev/input/event5 running while I captured zytronic.events, and got the following output: Input driver version is 1.0.1 Input device ID: bus 0x3 vendor 0x14c8 product 0x5 version 0x101 Input device name: "Zytronic Displays Limited Zytronic Touchscreen Controller" Supported events: Event type 0 (EV_SYN) Event type 1 (EV_KEY) Event code 330 (BTN_TOUCH) Event type 3 (EV_ABS) Event code 0 (ABS_X) Value 0 Min0 Max 4096 Event code 1 (ABS_Y) Value 0 Min0 Max 4096 Event code 47 (ABS_MT_SLOT) Value 0 Min0 Max9 Event code 53 (ABS_MT_POSITION_X) Value 0 Min0 Max 4096 Event code 54 (ABS_MT_POSITION_Y) Value 0 Min0 Max 4096 Event code 57 (ABS_MT_TRACKING_ID) Value 0 Min0 Max65535 Testing ... (interrupt to exit) ^C Any ideas, or instructions for gathering more debug information will be gratefully received. -- Simon Farnsworth Software Engineer ONELAN Ltd http://www.onelan.com 05 0d 09 04 a1 01 85 01 09 22 a1 02 09 42 15 00 25 01 75 01 95 01 81 02 09 32 95 01 81 02 95 06 81 01 05 01 26 00 10 75 10 95 01 65 00 09 30 81 02 09 31 46 00 10 81 02 05 0d 09 51 26 ff 00 75 08 95 01 81 02 c0 85 02 09 55 15 00 25 08 75 08 95 01 b1 02 c0 05 0d 09 0e a1 01 85 03 a1 02 09 23 09 52 09 53 15 00 25 08 75 08 95 02 b1 02 c0 c0 05 01 09 02 a1 01 09 01 a1 00 85 04 05 09 19 01 29 02 15 00 25 01 95 02 75 01 81 02 95 01 75 06 81 01 05 01 09 30 09 31 15 00 26 00 10 35 00 46 00 10 65 00 75 10 95 02 81 62 c0 c0 06 00 ff 09 01 a1 01 85 05 09 00 15 00 26 ff 00 75 08 95 3f b1 02 c0 06 00 ff 09 01 a1 01 85 06 09 00 15 00 26 ff 00 75 08 95 3f 81 02 c0 INPUT(1)[INPUT] Field(0) Logical(Digitizers.Finger) Application(Digitizers.TouchScreen) Usage(1) Digitizers.TipSwitch Logical Minimum(0) Logical Maximum(1) Report Size(1) Report Count(1) Report Offset(0) Flags( Variable Absolute ) Field(1) Logical(Digitizers.Finger) Application(Digitizers.TouchScreen) Usage(1) Digitizers.InRange Logical Minimum(0) Logical Maximum(1) Report Size(1) Report Count(1) Report Offset(1) Flags( Variable Absolute ) Field(2) Logical(Digitizers.Finger) Application(Digitizers.TouchScreen) Usage(1) GenericDesktop.X Logical Minimum(0) Logical Maximum(4096) Report Size(16) Report Count(1) Report Offset(8) Flags( Variable Absolute ) Field(3) Logical(Digitizers.Finger) Application(Digitizers.TouchScreen) Usage(1) GenericDesktop.Y Logical Minimum(0) Logical Maximum(4096) Physical Minimum(0) Physical Maximum(4096) Report Size(16) Report Count(1) Report Offset(24) Flags( Variable Absolute ) Field(4) Logical(Digitizers.Finger) Application(Digitizers.TouchScreen) Usage(1) Digitizers.ContactID Logical Minimum(0) Logical Maximum(255) Physical Minimum(0) Physical Maximum(4096) Report Size(8) Report Count(1) Report Offset(40) Flags( Variable Absolute ) INPUT(4)[INPUT] Field(0) Physical(GenericDesktop.Pointer) Application(GenericDesktop.Mouse) Usage(2) Button.0001 Button.0002 Logical Minimum(0) Logical Maximum(1) Physical Minimum(0) Physical Maximum(4096) Report Size(1) Report Count(2) Report Offset(0) Flags( Variable Absolute ) Field(1) Physical(GenericDesktop.Pointer) Application(GenericDesktop.Mouse) Usage(2) GenericDesktop.X GenericDesktop.Y Logical Minimum(0) Logical Maximum(4096) Physical Minimum(0) Physical Maximum(4096) Report Size(16) Report Count(2) Report Offset(8) Flags( Variabl
Re: [PATCH 2/3] usb: Take attribute avoid_reset_quirk out of usb device's attribute group
On Mon, Jul 30, 2012 at 10:27:08PM +0200, Oliver Neukum wrote: > On Monday 30 July 2012 13:00:18 Sarah Sharp wrote: > > > So far, the discussion on the mailing list seems to boil down to: > > > > Issues > > -- > > > > - We need to issue a reset resume for all suspended devices that have > >been powered off. > > > > - We can't power off ports with connected devices that require firmware > >(e.g. bluetooth and 3G modems). The firmware would be lost when > >they're reset. > > > > - Some devices may morph into a different USB device on reset, so we > >need to avoid powering the port down when those devices are attached. > >Drivers for those devices will have the USB_QUIRK_RESET_MORPHS set. > > > > - I'm not sure if it's true that all devices that need firmware will > >have USB_QUIRK_RESET_MORPHS set. Alan, Oliver? > > Usually this is user space's responsibility. Given that we have a few firmware > uploaders in kernel space, we also need a port attribute for that. Ok, good to know. > > - Many drivers may turn on remote wakeup when a device is suspended, > >but userspace may not need the wakeup. An example would be if the > >user clicked "Disable bluetooth" from ConnMan. It obviously wouldn't > >care about remote wakeups from the bluetooth device. > > Then the driver should not request remote wakeup to be enabled. > If it does, it needs to be fixed. Ok, so if userspace closes the bluetooth interface, the driver shouldn't request remote wakeup, right? I'll have to look if this is true for bluetooth and modem devices. > > > Possible solutions > > -- > > > > - We need a new interface driver flag to indicate a driver is fine with > >the port being powered off. Something like "supports_power_off". > > > > - In addition to the port power sysfs values of "on" or "off", we also > >need an "auto" value that attempts to apply a smart in-kernel policy > >to when to power off the port. That policy might look like: > > > >1. If the device is internal and unpluggable, power off the port > > > >2. If the device is internal and suspended, power off the port if all > > the following are true: > > It should also apply to ports of hubs in compound devices. The port power off behavior is different for those hubs, so I want to address that later. For USB 2.0 hubs, doesn't it detect connect events if you power off a port? Also, few USB 2.0 hubs actually power gate the ports (or all ports are ganged together), so I don't think this would actually save power. > > a) all interface drivers have supports_power_off set, or no > > interface drivers are bound and usbfs has not claimed the > > device. > > b) remote wakeup is disabled > > c) USB_QUIRK_RESET_MORPHS is not set > > > > - If userspace wants a port to be powered off, and one of the interface > >drivers doesn't set supports_power_off or the driver enables remote > >wakeup, then userspace will need to unbind or unload the driver. > > > > > > So far, the discussion has been mainly focused on figuring out a smart > > policy for internal USB ports. However, I'd like to see the "auto" > > in-kernel policy extended to external USB ports. Perhaps we need to > > The policy is likely to be very similar. The question is about defaults rather > than mechanisms. > > > expose the ACPI internal/external port and connectable/unconnectable > > values through sysfs, and apply the policy to both internal and external > > devices? > > > > Then userspace could look at whether a port is internal or external, and > > decide when it makes sense to auto-power-off the port. It could decide > > to set an "auto" policy on all ports when the screen blanks. When the > > user starts interacting with the system and the screen turns back on, > > userspace could change the policy back to "on" for external ports and > > internal connectable ports. > > > > Then policy #1 would just change to "If the device is disconnected, > > power off the port" and policy #2 would apply to both internal and > > external suspended ports. > > > > Thoughts? > > This needs to be configurable. > > It is not obvious that external ports should be handled like internal ports. > It might be sensible to apply auto-powerdown to a connected and opened > device, but to leave external ports on, so that hotplugs are detected. Sure. That's why I was proposing that external ports have the port power sysfs file set to "on" by default. Then userspace can set it to "auto" when it feels that external ports are ok to be powered off, because the user isn't interacting with the system, and it's ok to delay hotplug events. Sarah Sharp -- 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 2/3] usb: Take attribute avoid_reset_quirk out of usb device's attribute group
On Mon, Jul 30, 2012 at 10:35:37PM +0200, Bjørn Mork wrote: > Sarah Sharp writes: > > - In addition to the port power sysfs values of "on" or "off", we also > >need an "auto" value that attempts to apply a smart in-kernel policy > >to when to power off the port. That policy might look like: > > > >1. If the device is internal and unpluggable, power off the port > > How do you detect the "unpluggable"? Did you consider rfkill switches > "plugging" and "unplugging" internal devices? ACPI provides methods to say whether a USB port is internal or external, and whether it's "connectable", meaning whether a USB device will ever connect to it. I would hope (although BIOS writers get the ACPI tables wrong all the time) that devices behind rfkill switches would be marked as connectable internal ports. > >2. If the device is internal and suspended, power off the port if all > > the following are true: > > > > a) all interface drivers have supports_power_off set, or no > > interface drivers are bound and usbfs has not claimed the > > device. > > b) remote wakeup is disabled > > c) USB_QUIRK_RESET_MORPHS is not set > > Why can't that be a userspace decision? I.e. drop all this policy in > the kernel stuff, and just implement: > > > - If userspace wants a port to be powered off, and one of the interface > >drivers doesn't set supports_power_off or the driver enables remote > >wakeup, then userspace will need to unbind or unload the driver. Yes, we could do that. However, I would really like the port power off to actually be *used*, so I'm not happy with leaving it completely off by default. I'd like unconnectable internal ports to have the port power setting on "auto" by default, and have the default auto policy for empty ports to be powered off. If that's too controversial, I'll settle for getting the distros to write the udev rule to use the exported ACPI information to turn off unconnectable internal ports. Or write a powertop rule. > > So far, the discussion has been mainly focused on figuring out a smart > > policy for internal USB ports. However, I'd like to see the "auto" > > in-kernel policy extended to external USB ports. Perhaps we need to > > expose the ACPI internal/external port and connectable/unconnectable > > values through sysfs, and apply the policy to both internal and external > > devices? > > > > Then userspace could look at whether a port is internal or external, and > > decide when it makes sense to auto-power-off the port. It could decide > > to set an "auto" policy on all ports when the screen blanks. When the > > user starts interacting with the system and the screen turns back on, > > userspace could change the policy back to "on" for external ports and > > internal connectable ports. > > Yes, this is the way to go IMHO. > > BTW, what if the interaction started with a USB keyboard being plugged > in? No problem for you - that was the userspace daemon bad coice of > policy :-) I was envisioning some sort of check box in the screensaver settings like "turn off USB ports on screen blank". Then people who turn on their desktops by plugging in a USB keyboard can untick it and be happy. Those people with laptops who can turn the screensaver off by hitting a key on the built-in keyboard can check it and get better power savings. > > Then policy #1 would just change to "If the device is disconnected, > > power off the port" and policy #2 would apply to both internal and > > external suspended ports. > > > > Thoughts? > > I really think the kernel should limit itself to providing the basic > functionality (driver support flag and port power switch), and leave any > policy decisions for userspace to screw up. > > It's not like the kernel decides whether to enable autosuspend in the > first place... Actually, it does enable auto-suspend by default for USB hubs. Sarah Sharp -- 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 2/3] usb: Take attribute avoid_reset_quirk out of usb device's attribute group
On Tuesday 31 July 2012 11:49:29 Alan Stern wrote: > On Tue, 31 Jul 2012, Bjørn Mork wrote: > > > Well, you are then saying that usb-storage has priority over any other > > function and that is often wrong IMHO. > > No, I'm not saying that at all. The situation is perfectly symmetric. Well, the capabilities are not. Most drivers are less well able to handle a reset than the storage driver is. > Another alternative is for the other drivers to delay the reset (by > sleeping in their pre_reset callbacks) until they can handle the reset > safely. No! You must not ever do that! The reset is a part of the error handling path of the block layer. Therefore it must not sleep indefinitely. In particular you must never wait for anything user space does or for any part of the kernel that allocates memory with GFP_KERNEL or GFP_NOFS, and you must allocate memory only with GFP_NOIO. The penalty is deadlock. > The kernel doesn't know which functions the user is interested in. It > has no basis for saying "The user doesn't care about the mass-storage > function so don't let usb-storage issue any device resets". Yes. Regards Oliver -- 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 2/3] usb: Take attribute avoid_reset_quirk out of usb device's attribute group
On Mon, Jul 30, 2012 at 05:03:27PM -0400, Alan Stern wrote: > On Mon, 30 Jul 2012, Sarah Sharp wrote: > > Possible solutions > > -- > > > > - We need a new interface driver flag to indicate a driver is fine with > >the port being powered off. Something like "supports_power_off". > > > > - In addition to the port power sysfs values of "on" or "off", we also > >need an "auto" value that attempts to apply a smart in-kernel policy > >to when to power off the port. That policy might look like: > > > >1. If the device is internal and unpluggable, power off the port > > That doesn't sound right. Don't you mean: If the port is internal and > not pluggable and there is no device attached, power off the port? If there's a device attached and the ACPI table reports the port is not "connectable", then there's something wrong with the ACPI table. > If the port is unpluggable and has no device, I don't see what > difference it makes whether the port is internal or external. External ports would never be marked as unpluggable, because the user can plug a device in. Internal ports can be unpluggable or pluggable. > >2. If the device is internal and suspended, power off the port if all > > the following are true: > > > > a) all interface drivers have supports_power_off set, or no > > interface drivers are bound and usbfs has not claimed the > > device. > > b) remote wakeup is disabled > > c) USB_QUIRK_RESET_MORPHS is not set > > > > - If userspace wants a port to be powered off, and one of the interface > >drivers doesn't set supports_power_off or the driver enables remote > >wakeup, then userspace will need to unbind or unload the driver. > > Like other people, I'm dubious about these conditions. > > > So far, the discussion has been mainly focused on figuring out a smart > > policy for internal USB ports. However, I'd like to see the "auto" > > in-kernel policy extended to external USB ports. > > Really, what's the difference? Are you assuming that internal ports > are always "unpluggable" (i.e., devices cannot be plugged into or > unplugged from the port)? No. Some internal ports can be pluggable, like your card reader, or those ones behind an rfkill switch. But only internal ports can be "unpluggable". I'm apparently not explaining the ACPI methods very well. Can you please take a look at the Microsoft page on them and let me know if it clears up your confusion: http://msdn.microsoft.com/en-us/library/windows/hardware/ff553550%28v=vs.85%29.aspx > I'm not sure even that much is true. For example, my Asus laptop has a > USB card reader that's built-in. The connection is definitely > internal; the user cannot get at it (although I don't know what the > ACPI tables have to say about it). But the card reader disconnects > itself from the USB bus whenever there's no card inserted. > > At any rate, it seems that you should be focusing on pluggable vs. > unpluggable rather than internal vs. external. Ok, fine. I do think the internal vs external information could be useful to userspace to decide whether it should turn off a port, but I agree that unpluggable vs pluggable is more useful. > > Perhaps we need to > > expose the ACPI internal/external port and connectable/unconnectable > > values through sysfs, and apply the policy to both internal and external > > devices? > > > > Then userspace could look at whether a port is internal or external, and > > decide when it makes sense to auto-power-off the port. It could decide > > to set an "auto" policy on all ports when the screen blanks. When the > > user starts interacting with the system and the screen turns back on, > > userspace could change the policy back to "on" for external ports and > > internal connectable ports. > > > > Then policy #1 would just change to "If the device is disconnected, > > power off the port" and policy #2 would apply to both internal and > > external suspended ports. > > > > Thoughts? > > I tend to agree that having the kernel make these decisions is fraught > with difficulties, except in the most simple cases (unpluggable and no > device present). > > Exposing the extra attributes to userspace can't hurt -- unless we want > to change or remove them in the future! Hmm, true. I suppose we could have a "no info" output if the ACPI port info isn't there, and that would allow us to deprecate the API in the future. Sarah Sharp -- 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 2/2] xhci: EHCI/XHCI ports switching on Intense-PC.
On Tue, Jul 31, 2012 at 10:06:34AM -0700, Sarah Sharp wrote: > On Tue, Jul 31, 2012 at 06:49:50AM +0200, Oliver Neukum wrote: > > On Monday 30 July 2012 15:34:06 Sarah Sharp wrote: > > > Hi Denis, > > > > > > Can you send me the output of `sudo dmidecode`? I'd like to see if I > > > can make a more general patch apply to the Intense-PC. > > > > As this is for shutdown, why not all systems? > > Because it will cause a BIOS delay on the next boot. Approximately > 100 ms. Some people trying to do extremely fast boot will care about > that delay. However, looking at the Intense-PC SMBIOS info, it looks like I can't make the patch more generic. I'll have to key off of PCI ID, and all PPT systems will have the ports switched back to EHCI. Sarah Sharp -- 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 2/3] usb: Take attribute avoid_reset_quirk out of usb device's attribute group
On Tue, 31 Jul 2012, Sarah Sharp wrote: > > It should also apply to ports of hubs in compound devices. > > The port power off behavior is different for those hubs, Are you sure about that? > so I want to > address that later. For USB 2.0 hubs, doesn't it detect connect events > if you power off a port? No. At least, not according the USB-2.0 spec, section 11.24.2.7.1.1. Some hubs may not obey the spec in this regard. > Also, few USB 2.0 hubs actually power gate the > ports (or all ports are ganged together), so I don't think this would > actually save power. Maybe not, but some hubs do actually turn off power to the ports. Besides, the kernel should do the right thing even if the devices don't. Particularly since the kernel can't tell how much power a hub is actually providing to its ports. > > It is not obvious that external ports should be handled like internal ports. Why not? The important criterion is whether the port is connectable, not whether it is internal. > > It might be sensible to apply auto-powerdown to a connected and opened > > device, but to leave external ports on, so that hotplugs are detected. > > Sure. That's why I was proposing that external ports have the port > power sysfs file set to "on" by default. Then userspace can set it to > "auto" when it feels that external ports are ok to be powered off, > because the user isn't interacting with the system, and it's ok to delay > hotplug events. The kernel should do this for all connectable ports, internal or external. 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 2/3] usb: Take attribute avoid_reset_quirk out of usb device's attribute group
On Tue, Jul 31, 2012 at 01:02:43PM -0400, Alan Stern wrote: > I don't know. I can't think of anything more, but there's a nagging > feeling that if we use these then something will go wrong. > > How about if we start with only the first policy rule: Ports that can > never have anything plugged in should be shut down automatically. > Initially, everything else can be left up to userspace. Then we can > experiment with adding other policies to the kernel. Ok, that sounds like a good start. I'm a little fuzzy on the details though. Does that mean you think we should add an "auto" option to the sysfs file? Then when the file is set to "auto", have the kernel turn off ports that don't have a USB device connected to it. Have the kernel set the default sysfs value to auto for unpluggable ports. Or do you think userspace should set the sysfs file to "off" after verifying the port is unpluggable through new sysfs files? Sarah Sharp -- 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 2/3] usb: Take attribute avoid_reset_quirk out of usb device's attribute group
On Tuesday 31 July 2012 14:46:38 Alan Stern wrote: > > > It is not obvious that external ports should be handled like internal > > > ports. > > Why not? The important criterion is whether the port is connectable, > not whether it is internal. That doesn't mean it should be handled differently always either. Just that it might be. The difference is that you know what would be connected. You just don't know whether it is connected. So if you've decided that you are not going to automount new media, there is no need to detect hotplugs. For an external port this conclusion cannot be drawn. Which opens up an opportunity for optimisation. Should we disable polling for new media on card readers that are internal and connectable and have only one LUN? Regards Oliver -- 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 2/3] usb: Take attribute avoid_reset_quirk out of usb device's attribute group
On Tue, 31 Jul 2012, Sarah Sharp wrote: > > >1. If the device is internal and unpluggable, power off the port > > > > That doesn't sound right. Don't you mean: If the port is internal and > > not pluggable and there is no device attached, power off the port? > > If there's a device attached and the ACPI table reports the port is not > "connectable", then there's something wrong with the ACPI table. Um, we've got a failure of terminology here. I don't know exactly what the ACPI entries are supposed to mean. However, it's possible to have: a port into which no device can ever be plugged, or a port with a fixed device plugged in, which can never be unplugged. I would call both of these not "pluggable" -- which may or may not be the same as "unpluggable"! Besides, it's hardly unheard of for something to be wrong with an ACPI table... :-) > > If the port is unpluggable and has no device, I don't see what > > difference it makes whether the port is internal or external. > > External ports would never be marked as unpluggable, because the user > can plug a device in. In theory such things are possible. For example, the embedded hub in a compound device might not have all of its ports wired up. However such a hub would have no way to report this fact to the kernel... > I'm apparently not explaining the ACPI methods very well. Can you > please take a look at the Microsoft page on them and let me know if it > clears up your confusion: > > http://msdn.microsoft.com/en-us/library/windows/hardware/ff553550%28v=vs.85%29.aspx Okay. For other people's reference this page says: "PortIsConnectable" means it's possible for a device to be connected (either temporarily or permanently). "UserVisible" means the user can freely plug and unplug devices. In addition to these ACPI definitions, the USB spec defines a "DeviceRemovable" flag. The spec doesn't appear to take into account the possibility of a port which can never have a device plugged in, so it defines this flag simply to mean that the port doesn't have a permanently-attached device. If we wanted to be thorough, our root-hub hub descriptors would set the DeviceRemovable flag for those ports where PortIsConnectable is set and UserVisible is clear. (But of course this only works on ACPI-based systems.) And your first rule now says that the kernel should turn off power to any port for which PortIsConnectable and UserVisible are both clear. In the context of these definitions, the terms "internal"/"external" and "pluggable"/"unpluggable" we've been tossing around are somewhat cloudy. For example, does "internal" mean that UserVisible is clear, or does it mean that the port has an ACPI table entry? > Ok, fine. I do think the internal vs external information could be > useful to userspace to decide whether it should turn off a port, but I > agree that unpluggable vs pluggable is more useful. Provided we agree on an exact meaning. > > Exposing the extra attributes to userspace can't hurt -- unless we want > > to change or remove them in the future! > > Hmm, true. I suppose we could have a "no info" output if the ACPI port > info isn't there, and that would allow us to deprecate the API in the > future. We should expose the USB DeviceRemovable flag too. 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 2/3] usb: Take attribute avoid_reset_quirk out of usb device's attribute group
On Tue, 31 Jul 2012, Sarah Sharp wrote: > On Tue, Jul 31, 2012 at 01:02:43PM -0400, Alan Stern wrote: > > I don't know. I can't think of anything more, but there's a nagging > > feeling that if we use these then something will go wrong. > > > > How about if we start with only the first policy rule: Ports that can > > never have anything plugged in should be shut down automatically. > > Initially, everything else can be left up to userspace. Then we can > > experiment with adding other policies to the kernel. > > Ok, that sounds like a good start. I'm a little fuzzy on the details > though. > > Does that mean you think we should add an "auto" option to the sysfs > file? Then when the file is set to "auto", have the kernel turn off ports > that don't have a USB device connected to it. Have the kernel set the > default sysfs value to auto for unpluggable ports. > > Or do you think userspace should set the sysfs file to "off" after > verifying the port is unpluggable through new sysfs files? Actually, what I had in mind was more like: We add an "auto" option to each port's power-policy sysfs attribute. When the policy is set to "auto" the kernel will turn off power to the port, but only if no USB device can ever be attached to it. Power policy for all ports will default to "on"; userspace can change the policy (perhaps after looking at other sysfs attributes). Alternatively, we could do something more like what you said: Set the default policy to "auto" if no device can ever be attached to the port; otherwise set it to "on". The "auto" policy setting means turn off power to the port, but only if no device is currently attached to it. Later on we can change what "auto" means. I'm not sure which scheme is best in terms of reliability and upward (future) compatilibity. 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 2/3] usb: Take attribute avoid_reset_quirk out of usb device's attribute group
On Tue, 31 Jul 2012, Oliver Neukum wrote: > > Another alternative is for the other drivers to delay the reset (by > > sleeping in their pre_reset callbacks) until they can handle the reset > > safely. > > No! You must not ever do that! > > The reset is a part of the error handling path of the block layer. Therefore > it must not sleep indefinitely. In particular you must never wait for anything > user space does or for any part of the kernel that allocates memory with > GFP_KERNEL or GFP_NOFS, and you must allocate memory only with GFP_NOIO. > The penalty is deadlock. True; it's easy to forget that. Although in practice it's not very likely to occur. It would require a writable memory-mapped file or swap partition on a USB drive and a system running low on available memory. 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 2/3] usb: Take attribute avoid_reset_quirk out of usb device's attribute group
On Tue, 31 Jul 2012, Oliver Neukum wrote: > Which opens up an opportunity for optimisation. Should we disable > polling for new media on card readers that are internal and connectable > and have only one LUN? Considering that we don't know which devices are card readers or whether they disconnect themselves when the medium is removed, I think this decision is better left to userspace. (Also, many of these card readers have multiple LUNs; they map different LUNs to different types of memory card.) 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 2/3] usb: Take attribute avoid_reset_quirk out of usb device's attribute group
On Tuesday 31 July 2012 15:42:51 Alan Stern wrote: > On Tue, 31 Jul 2012, Oliver Neukum wrote: > > > > Another alternative is for the other drivers to delay the reset (by > > > sleeping in their pre_reset callbacks) until they can handle the reset > > > safely. > > > > No! You must not ever do that! > > > > The reset is a part of the error handling path of the block layer. Therefore > > it must not sleep indefinitely. In particular you must never wait for > > anything > > user space does or for any part of the kernel that allocates memory with > > GFP_KERNEL or GFP_NOFS, and you must allocate memory only with GFP_NOIO. > > The penalty is deadlock. > > True; it's easy to forget that. Although in practice it's not very > likely to occur. It would require a writable memory-mapped file or > swap partition on a USB drive and a system running low on available > memory. If you refer to allocating memory, yes. Waiting for user space is worse. Any read access to the storage device would deadlock. Regards Oliver -- 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
Do we need asynchronous pm_runtime_get()? (was: Re: bisected regression ...)
On Sunday, July 29, 2012, Rafael J. Wysocki wrote: > On Sunday, July 29, 2012, Alan Stern wrote: > > On Sun, 29 Jul 2012, Rafael J. Wysocki wrote: > > > > > The difference is, if you use _put_sync(), you need to wait the extra 10 > > > ms > > > for local_pci_probe() to return (if the parent is actually suspended), > > > although you might not need to wait for it if you used _put(), right? > > > > Yes, that's the difference. But who waits for local_pci_probe() to > > return? :-) > > pci_register_driver() might, but that's not a big deal. Hot-plug might > as well, though. > > > > Which, to me, means that using _put_sync() may not be always better. > > > It probably doesn't matter a lot, but then the workqueue overhead > > > shouldn't > > > matter a lot either. > > > > It's that in the end, the extra overhead is pretty small. For me > > there's also an issue of style: If you do a synchronous get then it > > looks odd not to do a synchronous put. My feeling has always been that > > the async routines are for use in non-process contexts, where the sync > > routines can't be used. Using them just to return a little more > > quickly is a foreign idea. > > I see. :-) > > The reason for using sync get is quite obvious: we want to be able to access > the device later on. For sync put that's not so clear, though. There are > cases > when we definitely want to do it, like the failing .probe() in which we want > to > disable runtime PM next to the put, but usually it doesn't hurt (too much) to > defer it IMO. Now it occured to me that perhaps we don't need the current asynchronous pm_runtime_get() at all. The usefulness of it is quite questionable, because either we want to resume the device immediately, for which pm_runtime_get_sync() should be used, or we just want to bump up the usage counter, in which cases pm_runtime_get_noresume() should always be sufficient. I fail to see any particularly compelling use case for pm_runtime_get() doing an asynchronous resume at the moment, but perhaps that's just me. However, I receive reports of people using pm_runtime_get() where they really should use pm_runtime_get_sync(), so I wonder if we can simply rename pm_runtime_get_sync() as pm_runtime_get() and drop the asynchronous version altogether? Rafael -- 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 2/3] usb: Take attribute avoid_reset_quirk out of usb device's attribute group
Sarah Sharp writes:
> No. Some internal ports can be pluggable, like your card reader, or
> those ones behind an rfkill switch. But only internal ports can be
> "unpluggable".
>
> I'm apparently not explaining the ACPI methods very well. Can you
> please take a look at the Microsoft page on them and let me know if it
> clears up your confusion:
>
> http://msdn.microsoft.com/en-us/library/windows/hardware/ff553550%28v=vs.85%29.aspx
OK, that explains a lot of things for me, along with looking at the
actual DSDT on my laptop. I guess you are after things like PRT5 here
(note that it doesn't have any _PLD, which makes sense I believe):
Device (USB2)
{
Name (_ADR, 0x001D0002)
..
Device (URTH)
{
Name (_ADR, 0x00)
Device (PRT4)
{
Name (_ADR, 0x01)
Name (_UPC, Package (0x04)
{
0xFF,
0xFF,
0x00,
0x00
})
Name (_PLD, Buffer (0x10)
{
/* */0x81, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00,
/* 0008 */0x30, 0x1C, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00
})
}
Device (PRT5)
{
Name (_ADR, 0x02)
Name (_UPC, Package (0x04)
{
0x00,
0xFF,
0x00,
0x00
})
}
}
So PRT4 is internal and pluggable (and in fact connected to a modem in a
mini-PCIe slot), while PRT5 is unconnectable. Note that it doesn't have
any visibility indication. But that doesn't matter, does it?
No, I wouldn't mind if PRT5 was powered off. But I do wonder what the
actual power savings could possibly be. I assume the pins are
unconnected so that there couldn't be any power lost there, and the
controller must be active to support the other port anyway.
Some measurements sure would be interesting.
>> I'm not sure even that much is true. For example, my Asus laptop has a
>> USB card reader that's built-in. The connection is definitely
>> internal; the user cannot get at it (although I don't know what the
>> ACPI tables have to say about it). But the card reader disconnects
>> itself from the USB bus whenever there's no card inserted.
>>
>> At any rate, it seems that you should be focusing on pluggable vs.
>> unpluggable rather than internal vs. external.
>
> Ok, fine. I do think the internal vs external information could be
> useful to userspace to decide whether it should turn off a port, but I
> agree that unpluggable vs pluggable is more useful.
I don't think the DSDT provides enough information for me to know which
of the many internal pluggable ports I can power off. Some of them are
connected to internal devices I care about (bluetooth and modem), and
others are connected to devices I might want to power off (fingerprint
scanner and webcam). And two internal ports are pluggable but AFAIK not
connected to anything. I guess that must be the empty mini-PCIe slot
and the mini-PCIe slot with the wlan card.
Hmm, yes, thinking about this it might help to know which of the 5
unused ports are internal.
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: Do we need asynchronous pm_runtime_get()? (was: Re: bisected regression ...)
[CC: list trimmed] On Tue, 31 Jul 2012, Rafael J. Wysocki wrote: > Now it occured to me that perhaps we don't need the current asynchronous > pm_runtime_get() at all. The usefulness of it is quite questionable, because > either we want to resume the device immediately, for which > pm_runtime_get_sync() > should be used, or we just want to bump up the usage counter, in which cases > pm_runtime_get_noresume() should always be sufficient. I fail to see any > particularly compelling use case for pm_runtime_get() doing an asynchronous > resume at the moment, but perhaps that's just me. There are indeed valid uses for pm_runtime_get(). We are forced to use it in non-sleepable contexts when we want to resume the device as quickly as possible. Example: a driver receives an I/O request from an interrupt handler. > However, I receive reports of people using pm_runtime_get() where they really > should use pm_runtime_get_sync(), so I wonder if we can simply rename > pm_runtime_get_sync() as pm_runtime_get() and drop the asynchronous version > altogether? Well, IMO the naming should have been the other way around from the start. That is, we should have made pm_runtime_get be the synchronous routine and pm_runtime_get_async be the asynchronous one. But it's too late to change now. And no, we can't get rid of the async version. 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 2/3] usb: Take attribute avoid_reset_quirk out of usb device's attribute group
On Tue, 31 Jul 2012, Alan Stern wrote: > "PortIsConnectable" means it's possible for a device to be > connected (either temporarily or permanently). > > "UserVisible" means the user can freely plug and unplug > devices. > > In addition to these ACPI definitions, the USB spec defines > a "DeviceRemovable" flag. The spec doesn't appear to take into account > the possibility of a port which can never have a device plugged in, so > it defines this flag simply to mean that the port doesn't have a > permanently-attached device. > > If we wanted to be thorough, our root-hub hub descriptors would set the > DeviceRemovable flag for those ports where PortIsConnectable is set > and UserVisible is clear. (But of course this only works on ACPI-based > systems.) Typo: I meant to say our descriptors could _clear_ DeviceRemovable for those ports. I suppose we could also clear it when PortIsConnectable is clear. > And your first rule now says that the kernel should turn off power to > any port for which PortIsConnectable and UserVisible are both clear. In fact, it's enough for PortIsConnectable to be clear. The Microsoft document does not allow for the possibility of PortIsConnectable clear and UserVisible set -- that combination would be meaningless. 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
[PATCH] usb: host: xhci: Fix Compliance Mode on SN65LVPE502CP Hardware
This patch is intended to work around a known issue on the
SN65LVPE502CP USB3.0 re-driver that can delay the negotiation
between a device and the host past the usual handshake timeout,
and if that happens on the first insertion, the host controller
port will enter in Compliance Mode as per xHCI Spec. The patch
creates a timer which polls every 2 seconds the link state of each
host controller's port (this by reading the PORTSC register) and
recovers the port by issuing a Warm reset every time Compliance mode
is detected.
Signed-off-by: Alexis R. Cortes
---
drivers/usb/host/xhci-hub.c | 27 ++
drivers/usb/host/xhci.c | 81
+++
drivers/usb/host/xhci.h |6 +++
3 files changed, 114 insertions(+), 0 deletions(-)
diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 7b01094..4ea498d 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -493,7 +493,19 @@ static void xhci_hub_report_link_state(u32 *status,
u32 status_reg)
* when this bit is set.
*/
pls |= USB_PORT_STAT_CONNECTION;
+ } else {
+ /*
+* If CAS bit isn't set but the Port is already at
+* Compliance Mode, fake a connection so the USB core
+* notices the Compliance state and resets the port.
+* This resolves an issue generated by the SN65LVPE502CP
+* in which sometimes the port enters compliance mode
+* caused by a delay on the host-device negotiation.
+*/
+ if (pls == USB_SS_PORT_LS_COMP_MOD)
+ pls |= USB_PORT_STAT_CONNECTION;
}
+
/* update status field */
*status |= pls;
}
@@ -645,6 +657,21 @@ int xhci_hub_control(struct usb_hcd *hcd, u16
typeReq, u16 wValue,
/* Update Port Link State for super speed ports*/
if (hcd->speed == HCD_USB3) {
xhci_hub_report_link_state(&status, temp);
+ /*
+* Verify if all USB3 Ports Have entered U0. Delete
compliance
+* mode timer if so.
+*/
+ if (xhci->quirks & XHCI_COMP_MODE_QUIRK) {
+ if (xhci->port_status_u0 != ((1 <<
xhci->num_usb3_ports)-1)) {
+ if ((temp & PORT_PLS_MASK) == XDEV_U0)
+ xhci->port_status_u0 |= 1 <<
wIndex;
+ } else {
+ /* Deleting Compliance Mode Recovery
Timer */
+ if
(del_timer_sync(&xhci->comp_mode_recovery_timer))
+ xhci_dbg(xhci, "Compliance Mode
Recovery Timer Deleted. "
+ "All USB3 ports
have entered U0 at least once.\n");
+ }
+ }
}
if (bus_state->port_c_suspend & (1 << wIndex))
status |= 1 << USB_PORT_FEAT_C_SUSPEND;
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index a979cd0..d1f1cda 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -26,6 +26,7 @@
#include
#include
#include
+#include
#include "xhci.h"
@@ -397,6 +398,62 @@ static void xhci_msix_sync_irqs(struct xhci_hcd *xhci)
#endif
+static void compliance_mode_recovery(unsigned long arg)
+{
+ struct xhci_hcd *xhci;
+ struct usb_hcd *hcd;
+ u32 temp;
+ int i;
+
+ xhci = (struct xhci_hcd *)arg;
+
+ for (i = 0; i < xhci->num_usb3_ports; i++) {
+ temp = xhci_readl(xhci, xhci->usb3_ports[i]);
+ if ((temp & PORT_PLS_MASK) == USB_SS_PORT_LS_COMP_MOD) {
+ /* Compliance Mode Detected. Letting USB Core handle
the Warm Reset */
+ xhci_dbg(xhci, "Compliance Mode Detected on port %d!
Attempting
recovery routine.\n", i + 1);
+ hcd = xhci->shared_hcd;
+
+ if (hcd->state == HC_STATE_SUSPENDED)
+ usb_hcd_resume_root_hub(hcd);
+
+ usb_hcd_poll_rh_status(hcd);
+ }
+ }
+
+ if (xhci->port_status_u0 != ((1 << xhci->num_usb3_ports)-1))
+ mod_timer(&xhci->comp_mode_recovery_timer, jiffies +
msecs_to_jiffies(COMP_MODE_RCVRY_MSECS));
+}
+
+static void compliance_mode_recovery_timer_init(struct xhci_hcd *xhci)
+{
+ xhci->port_status_u0 = 0;
+ init_timer(&xhci->comp_mode_recovery_timer);
+ xhci->comp_mode_recovery_timer.data = (unsigned long) xhci;
+ xhci->comp_mode_recovery_timer.function = compliance_mode_recovery;
+ xhci->comp_mode_recovery_timer.expires = jiffies +
msecs_to_jiffies(COMP_MO
Re: Do we need asynchronous pm_runtime_get()? (was: Re: bisected regression ...)
On Tuesday, July 31, 2012, Alan Stern wrote: > [CC: list trimmed] > > On Tue, 31 Jul 2012, Rafael J. Wysocki wrote: > > > Now it occured to me that perhaps we don't need the current asynchronous > > pm_runtime_get() at all. The usefulness of it is quite questionable, > > because > > either we want to resume the device immediately, for which > > pm_runtime_get_sync() > > should be used, or we just want to bump up the usage counter, in which cases > > pm_runtime_get_noresume() should always be sufficient. I fail to see any > > particularly compelling use case for pm_runtime_get() doing an asynchronous > > resume at the moment, but perhaps that's just me. > > There are indeed valid uses for pm_runtime_get(). We are forced to use > it in non-sleepable contexts when we want to resume the device as > quickly as possible. Example: a driver receives an I/O request from an > interrupt handler. Is it actually suitable to be used in such contexts? It doesn't give any guarantee that the device will be active when it returns, so the caller can't really rely on it. The caller always has to check if the device has been resumed before accessing it anyway, so it may as well do a _get_noresume(), do the check and do pm_request_resume() if needed directly. Now, as far as interrupt handlers are concerned, there are two cases: either the device has to be active to generate an interrupt (like PCI), or the interrupt is generated by something else on behalf of it. We don't seem to handle the first case very well right now, because in that case the interrupt handler will always know that the device is active, so it should do a _get_noresume() and then change the device's status to "active" without calling any kind of "resume", but we don't provide a helper for that. In the second case calling pm_runtime_get() doesn't really help either. I think it's better to do _get_noresume(), check the status and if "suspended", set a flag for the resume callback to do something specific before returning successfully, then call pm_request_resume() and return. > > However, I receive reports of people using pm_runtime_get() where they > > really > > should use pm_runtime_get_sync(), so I wonder if we can simply rename > > pm_runtime_get_sync() as pm_runtime_get() and drop the asynchronous version > > altogether? > > Well, IMO the naming should have been the other way around from the > start. That is, we should have made pm_runtime_get be the synchronous > routine and pm_runtime_get_async be the asynchronous one. But it's too > late to change now. I'm not sure it is too late. If we first change all the instances of pm_runtime_get() to pm_runtime_get_async() and then all of the instances of pm_runtime_get_sync() to pm_runtime_get(), it should be technically possible. Of course, it would be confusing, but that's a different matter. :-) > And no, we can't get rid of the async version. I'm still not sure of that. Thanks, Rafael -- 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] usb, ehci: Avoid deadlock of ehci->lock by disabling interrupts
We ran into an interesting deadlock on RHEL-5 (2.6.18) that I believe
still appiles to the current kernel involving the ehci->lock.
CPU A:
submits a bulk transfer urb
ehci_urb_enqueue calls submit_async
submit_async blocks on ehci->lock with irq disabled (the result
of spin_lock_irqsave) for CPU B
CPU B:
takes an ehci interrupt
locks ehci->lock
pre-empted by an IPI handler which spins waiting for CPU C
CPU C:
takes an MTRR request
sends an IPI to all cpus to block
spins waiting for all cpus to block
CPU A nevers processes IPI because its interrupts are disabled,
this creates the 3-way deadlock.
This deadlock is hard to reproduce by our customer, but based on their vmcore
it seems clear the above is what happened. I attatched a suggested patch
from a colleague that would seem to resolve the problem. Because it is
hard to reproduce, I have not been able to test it to verify it resolves
the problem.
The patch just turns spin_locks in the spin_lock_irqsaves in the ehci_irq
function. This would essentially block the IPI handler and let the interrupt
handler finish before processing the IPI. Then CPU A would get a chance to
finish and process its IPI.
Looking at the code paths in 2.6.18 and 3.5, the locking still seems the same
which is why I believe the problem still exists. However, someone in the office
thought the MTRR code has been re-written, so the problem we are seeing might
be more difficult to see with the current kernel.
This patch does feel awkward, disabling interrupts in the irq handler. It seems
like it would make more sense to remove the locking from the irq handler. But
that is probably more work and my knowledge of USB is limited. I'll start with
this patch and see where the conversation goes.
Any feedback would be appreciated.
[dzickus - rewrote the description, ported to v3.5]
Signed-off-by: Casey Dahlin
Signed-off-by: Don Zickus
---
drivers/usb/host/ehci-hcd.c |7 ---
1 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 800be38..a31d9cc 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -849,8 +849,9 @@ static irqreturn_t ehci_irq (struct usb_hcd *hcd)
struct ehci_hcd *ehci = hcd_to_ehci (hcd);
u32 status, masked_status, pcd_status = 0, cmd;
int bh;
+ unsigned long flags;
- spin_lock (&ehci->lock);
+ spin_lock_irqsave(&ehci->lock, flags);
status = ehci_readl(ehci, &ehci->regs->status);
@@ -868,7 +869,7 @@ static irqreturn_t ehci_irq (struct usb_hcd *hcd)
/* Shared IRQ? */
if (!masked_status || unlikely(ehci->rh_state == EHCI_RH_HALTED)) {
- spin_unlock(&ehci->lock);
+ spin_unlock_irqrestore(&ehci->lock, flags);
return IRQ_NONE;
}
@@ -969,7 +970,7 @@ dead:
if (bh)
ehci_work (ehci);
- spin_unlock (&ehci->lock);
+ spin_unlock_irqrestore(&ehci->lock, flags);
if (pcd_status)
usb_hcd_poll_rh_status(hcd);
return IRQ_HANDLED;
--
1.7.7.6
--
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: host: xhci: Fix Compliance Mode on SN65LVPE502CP Hardware
On Tue, Jul 31, 2012 at 04:19:48PM -0500, Alexis Cortes wrote: That's some "From:" line in your patch, are you sure that is right? > This patch is intended to work around a known issue on the > SN65LVPE502CP USB3.0 re-driver that can delay the negotiation > between a device and the host past the usual handshake timeout, > and if that happens on the first insertion, the host controller > port will enter in Compliance Mode as per xHCI Spec. The patch > creates a timer which polls every 2 seconds the link state of each > host controller's port (this by reading the PORTSC register) and > recovers the port by issuing a Warm reset every time Compliance mode > is detected. > > Signed-off-by: Alexis R. Cortes Odds are you want this address there, right? If so, please put it in the first line of the patch comments, as described in the Documentation/SubmittingPatches file. > --- > drivers/usb/host/xhci-hub.c | 27 ++ > drivers/usb/host/xhci.c | 81 > +++ Your patch is linewrapped and can not be applied :( Care to try again? 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: Do we need asynchronous pm_runtime_get()? (was: Re: bisected regression ...)
On Tuesday, July 31, 2012, Rafael J. Wysocki wrote: > On Tuesday, July 31, 2012, Alan Stern wrote: > > [CC: list trimmed] > > > > On Tue, 31 Jul 2012, Rafael J. Wysocki wrote: > > > > > Now it occured to me that perhaps we don't need the current asynchronous > > > pm_runtime_get() at all. The usefulness of it is quite questionable, > > > because > > > either we want to resume the device immediately, for which > > > pm_runtime_get_sync() > > > should be used, or we just want to bump up the usage counter, in which > > > cases > > > pm_runtime_get_noresume() should always be sufficient. I fail to see any > > > particularly compelling use case for pm_runtime_get() doing an > > > asynchronous > > > resume at the moment, but perhaps that's just me. > > > > There are indeed valid uses for pm_runtime_get(). We are forced to use > > it in non-sleepable contexts when we want to resume the device as > > quickly as possible. Example: a driver receives an I/O request from an > > interrupt handler. > > Is it actually suitable to be used in such contexts? It doesn't give any > guarantee that the device will be active when it returns, so the caller can't > really rely on it. The caller always has to check if the device has been > resumed before accessing it anyway, so it may as well do a _get_noresume(), > do the check and do pm_request_resume() if needed directly. > > Now, as far as interrupt handlers are concerned, there are two cases: either > the device has to be active to generate an interrupt (like PCI), or the > interrupt is generated by something else on behalf of it. We don't seem to > handle the first case very well right now, because in that case the interrupt > handler will always know that the device is active, so it should do a > _get_noresume() and then change the device's status to "active" without > calling any kind of "resume", but we don't provide a helper for that. Unless, of course, this is a shared interrupt, in which case the handler may be invoked, because _another_ device has generated an interrupt, so the "active" check will have to be done anyway. > In the second case calling pm_runtime_get() doesn't really help either. > I think it's better to do _get_noresume(), check the status and if > "suspended", set a flag for the resume callback to do something specific > before returning successfully, then call pm_request_resume() and return. I realize that this may be somewhat racy, so perhaps we need something like pm_request_resume_and_call(dev, func) that will execute the given function once the device has been resumed (or just happens to be "active" when the resume work item is run for it). > > > However, I receive reports of people using pm_runtime_get() where they > > > really > > > should use pm_runtime_get_sync(), so I wonder if we can simply rename > > > pm_runtime_get_sync() as pm_runtime_get() and drop the asynchronous > > > version > > > altogether? > > > > Well, IMO the naming should have been the other way around from the > > start. That is, we should have made pm_runtime_get be the synchronous > > routine and pm_runtime_get_async be the asynchronous one. But it's too > > late to change now. > > I'm not sure it is too late. If we first change all the instances of > pm_runtime_get() to pm_runtime_get_async() and then all of the instances of > pm_runtime_get_sync() to pm_runtime_get(), it should be technically possible. > Of course, it would be confusing, but that's a different matter. :-) > > > And no, we can't get rid of the async version. > > I'm still not sure of that. Thanks, Rafael -- 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] usb: host: xhci: Fix Compliance Mode on SN65LVPE502CP Hardware
This patch is intended to work around a known issue on the
SN65LVPE502CP USB3.0 re-driver that can delay the negotiation
between a device and the host past the usual handshake timeout,
and if that happens on the first insertion, the host controller
port will enter in Compliance Mode as per xHCI Spec. The patch
creates a timer which polls every 2 seconds the link state of each
host controller's port (this by reading the PORTSC register) and
recovers the port by issuing a Warm reset every time Compliance mode
is detected.
Signed-off-by: Alexis R. Cortes
---
drivers/usb/host/xhci-hub.c | 27 ++
drivers/usb/host/xhci.c | 81 +++
drivers/usb/host/xhci.h |6 +++
3 files changed, 114 insertions(+), 0 deletions(-)
diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 7b01094..4ea498d 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -493,7 +493,19 @@ static void xhci_hub_report_link_state(u32 *status, u32
status_reg)
* when this bit is set.
*/
pls |= USB_PORT_STAT_CONNECTION;
+ } else {
+ /*
+* If CAS bit isn't set but the Port is already at
+* Compliance Mode, fake a connection so the USB core
+* notices the Compliance state and resets the port.
+* This resolves an issue generated by the SN65LVPE502CP
+* in which sometimes the port enters compliance mode
+* caused by a delay on the host-device negotiation.
+*/
+ if (pls == USB_SS_PORT_LS_COMP_MOD)
+ pls |= USB_PORT_STAT_CONNECTION;
}
+
/* update status field */
*status |= pls;
}
@@ -645,6 +657,21 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16
wValue,
/* Update Port Link State for super speed ports*/
if (hcd->speed == HCD_USB3) {
xhci_hub_report_link_state(&status, temp);
+ /*
+* Verify if all USB3 Ports Have entered U0. Delete
compliance
+* mode timer if so.
+*/
+ if (xhci->quirks & XHCI_COMP_MODE_QUIRK) {
+ if (xhci->port_status_u0 != ((1 <<
xhci->num_usb3_ports)-1)) {
+ if ((temp & PORT_PLS_MASK) == XDEV_U0)
+ xhci->port_status_u0 |= 1 <<
wIndex;
+ } else {
+ /* Deleting Compliance Mode Recovery
Timer */
+ if
(del_timer_sync(&xhci->comp_mode_recovery_timer))
+ xhci_dbg(xhci, "Compliance Mode
Recovery Timer Deleted. "
+ "All USB3 ports
have entered U0 at least once.\n");
+ }
+ }
}
if (bus_state->port_c_suspend & (1 << wIndex))
status |= 1 << USB_PORT_FEAT_C_SUSPEND;
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index a979cd0..d1f1cda 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -26,6 +26,7 @@
#include
#include
#include
+#include
#include "xhci.h"
@@ -397,6 +398,62 @@ static void xhci_msix_sync_irqs(struct xhci_hcd *xhci)
#endif
+static void compliance_mode_recovery(unsigned long arg)
+{
+ struct xhci_hcd *xhci;
+ struct usb_hcd *hcd;
+ u32 temp;
+ int i;
+
+ xhci = (struct xhci_hcd *)arg;
+
+ for (i = 0; i < xhci->num_usb3_ports; i++) {
+ temp = xhci_readl(xhci, xhci->usb3_ports[i]);
+ if ((temp & PORT_PLS_MASK) == USB_SS_PORT_LS_COMP_MOD) {
+ /* Compliance Mode Detected. Letting USB Core handle
the Warm Reset */
+ xhci_dbg(xhci, "Compliance Mode Detected on port %d!
Attempting recovery routine.\n", i + 1);
+ hcd = xhci->shared_hcd;
+
+ if (hcd->state == HC_STATE_SUSPENDED)
+ usb_hcd_resume_root_hub(hcd);
+
+ usb_hcd_poll_rh_status(hcd);
+ }
+ }
+
+ if (xhci->port_status_u0 != ((1 << xhci->num_usb3_ports)-1))
+ mod_timer(&xhci->comp_mode_recovery_timer, jiffies +
msecs_to_jiffies(COMP_MODE_RCVRY_MSECS));
+}
+
+static void compliance_mode_recovery_timer_init(struct xhci_hcd *xhci)
+{
+ xhci->port_status_u0 = 0;
+ init_timer(&xhci->comp_mode_recovery_timer);
+ xhci->comp_mode_recovery_timer.data = (unsigned long) xhci;
+ xhci->comp_mode_recovery_timer.function = compliance_mode_recovery;
+ xhci->comp_mode_recovery_timer.expires = jiffies +
msecs_to_jiffies
Re: [PATCH] usb: host: xhci: Fix Compliance Mode on SN65LVPE502CP Hardware
On Tue, Jul 31, 2012 at 05:54:04PM -0500, Alexis Cortes wrote: > Hi Greg, > > I have resent the patch. Hopefully it know arrives properly. BTW, I'm using > Thunderbird configured as described in Documentation/email-clients.txt for > sending the patch. The resend looks like it worked properly, thanks for resending it. 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] usb: host: xhci: Fix Compliance Mode on SN65LVPE502CP Hardware
On Tue, Jul 31, 2012 at 05:48:38PM -0500, Alexis R. Cortes wrote:
> This patch is intended to work around a known issue on the
> SN65LVPE502CP USB3.0 re-driver that can delay the negotiation
> between a device and the host past the usual handshake timeout,
> and if that happens on the first insertion, the host controller
> port will enter in Compliance Mode as per xHCI Spec. The patch
> creates a timer which polls every 2 seconds the link state of each
> host controller's port (this by reading the PORTSC register) and
> recovers the port by issuing a Warm reset every time Compliance mode
> is detected.
>
> Signed-off-by: Alexis R. Cortes
> ---
> drivers/usb/host/xhci-hub.c | 27 ++
> drivers/usb/host/xhci.c | 81
> +++
> drivers/usb/host/xhci.h |6 +++
> 3 files changed, 114 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
> index 7b01094..4ea498d 100644
> --- a/drivers/usb/host/xhci-hub.c
> +++ b/drivers/usb/host/xhci-hub.c
> @@ -493,7 +493,19 @@ static void xhci_hub_report_link_state(u32 *status, u32
> status_reg)
>* when this bit is set.
>*/
> pls |= USB_PORT_STAT_CONNECTION;
> + } else {
> + /*
> + * If CAS bit isn't set but the Port is already at
> + * Compliance Mode, fake a connection so the USB core
> + * notices the Compliance state and resets the port.
> + * This resolves an issue generated by the SN65LVPE502CP
> + * in which sometimes the port enters compliance mode
> + * caused by a delay on the host-device negotiation.
> + */
> + if (pls == USB_SS_PORT_LS_COMP_MOD)
> + pls |= USB_PORT_STAT_CONNECTION;
> }
> +
> /* update status field */
> *status |= pls;
> }
> @@ -645,6 +657,21 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq,
> u16 wValue,
> /* Update Port Link State for super speed ports*/
> if (hcd->speed == HCD_USB3) {
> xhci_hub_report_link_state(&status, temp);
> + /*
> + * Verify if all USB3 Ports Have entered U0. Delete
> compliance
> + * mode timer if so.
> + */
> + if (xhci->quirks & XHCI_COMP_MODE_QUIRK) {
> + if (xhci->port_status_u0 != ((1 <<
> xhci->num_usb3_ports)-1)) {
> + if ((temp & PORT_PLS_MASK) == XDEV_U0)
> + xhci->port_status_u0 |= 1 <<
> wIndex;
> + } else {
> + /* Deleting Compliance Mode Recovery
> Timer */
> + if
> (del_timer_sync(&xhci->comp_mode_recovery_timer))
> + xhci_dbg(xhci, "Compliance Mode
> Recovery Timer Deleted. "
> +"All USB3 ports
> have entered U0 at least once.\n");
You need to disable the quirk flag here now, right? Otherwise can't the
timer be deleted twice if the module is unloaded after this happens?
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: Problems with a Zytronic touchscreen on kernel 3.5 (USB ID 0x14c8:0x0005) - regression from kernel 3.0
On Tue, Jul 31, 2012 at 05:52:52PM +0100, Simon Farnsworth wrote: > Hello, > > I'm trying to get a Zytronic single-touch touchscreen to work with Linux > kernel 3.5; it previously worked on a 3.0 kernel. I'm happy to try git > kernels > and patches, even if they're just intended to gather more information. > > hid-multitouch is picking up the device because it has multitouch report > descriptors as well as the single-touch descriptors. There's a quirk to make > it use MT_CLS_SERIAL, but that seems to be insufficient for my needs. > > I've got as far as determining that it's mt_event in hid-multitouch.c that > consumes the events, but as the screen never sends the events > hid-multitouch.c > expects to see, mt_event nevers sends an input event. > > evtest on 3.0 shows touch events as expected. evtest on 3.5 shows no touch > events. you might want to ask this on the linux-input mailing list, the developers there should be able to help you out better than we can. 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
[PATCH 1/8] USB:support the new interfaces of Huawei Data Card devices in option driver
From: fangxiaozhi 1. This patch is based on the kernel of 3.5 2. In this patch, we add new micro for matching the series USB devices with vendor ID and interface information. 3. In this patch, we add new declarations into option.c to support the new interfaces of Huawei Data Card devices. And at the same time, remove the redundant declarations from option.c. Signed-off-by: fangxiaozhi --- --- test/linux-3.5/include/linux/usb.h 2012-07-22 04:58:29.0 +0800 +++ linux-3.5/include/linux/usb.h 2012-07-26 14:40:40.0 +0800 @@ -828,6 +828,27 @@ static inline int usb_make_path(struct u .bInterfaceClass = (cl), \ .bInterfaceSubClass = (sc), \ .bInterfaceProtocol = (pr) +/** + * * USB_VENDOR_AND_INTERFACE_INFO - describe a specific usb device with a class of usb interfaces, but independent of product ID + * @vend: the 16 bit USB Vendor ID + * @cl: bInterfaceClass value + * @sc: bInterfaceSubClass value + * @pr: bInterfaceProtocol value + * + * This macro is used to create a struct usb_device_id that matches a + * series of devices with a vendor ID and a specific class of interfaces. + * + * This is especially useful when explicitly matching the specific interface for series of different devices with + * the same vendor. + */ + +#define USB_VENDOR_AND_INTERFACE_INFO(vend, cl, sc, pr) \ + .match_flags = USB_DEVICE_ID_MATCH_INT_INFO \ + | USB_DEVICE_ID_MATCH_VENDOR, \ + .idVendor = (vend), \ + .bInterfaceClass = (cl), \ + .bInterfaceSubClass = (sc), \ + .bInterfaceProtocol = (pr) /* --- */ --- test/linux-3.5/drivers/usb/serial/option.c 2012-07-22 04:58:29.0 +0800 +++ linux-3.5/drivers/usb/serial/option.c 2012-07-26 15:51:30.0 +0800 @@ -80,85 +80,9 @@ static void option_instat_callback(struc #define OPTION_PRODUCT_GTM380_MODEM0x7201 #define HUAWEI_VENDOR_ID 0x12D1 -#define HUAWEI_PRODUCT_E6000x1001 -#define HUAWEI_PRODUCT_E2200x1003 -#define HUAWEI_PRODUCT_E220BIS 0x1004 -#define HUAWEI_PRODUCT_E1401 0x1401 -#define HUAWEI_PRODUCT_E1402 0x1402 -#define HUAWEI_PRODUCT_E1403 0x1403 -#define HUAWEI_PRODUCT_E1404 0x1404 -#define HUAWEI_PRODUCT_E1405 0x1405 -#define HUAWEI_PRODUCT_E1406 0x1406 -#define HUAWEI_PRODUCT_E1407 0x1407 -#define HUAWEI_PRODUCT_E1408 0x1408 -#define HUAWEI_PRODUCT_E1409 0x1409 -#define HUAWEI_PRODUCT_E140A 0x140A -#define HUAWEI_PRODUCT_E140B 0x140B -#define HUAWEI_PRODUCT_E140C 0x140C -#define HUAWEI_PRODUCT_E140D 0x140D -#define HUAWEI_PRODUCT_E140E 0x140E -#define HUAWEI_PRODUCT_E140F 0x140F -#define HUAWEI_PRODUCT_E1410 0x1410 -#define HUAWEI_PRODUCT_E1411 0x1411 -#define HUAWEI_PRODUCT_E1412 0x1412 -#define HUAWEI_PRODUCT_E1413 0x1413 -#define HUAWEI_PRODUCT_E1414 0x1414 -#define HUAWEI_PRODUCT_E1415 0x1415 -#define HUAWEI_PRODUCT_E1416 0x1416 -#define HUAWEI_PRODUCT_E1417 0x1417 -#define HUAWEI_PRODUCT_E1418 0x1418 -#define HUAWEI_PRODUCT_E1419 0x1419 -#define HUAWEI_PRODUCT_E141A 0x141A -#define HUAWEI_PRODUCT_E141B 0x141B -#define HUAWEI_PRODUCT_E141C 0x141C -#define HUAWEI_PRODUCT_E141D 0x141D -#define HUAWEI_PRODUCT_E141E 0x141E -#define HUAWEI_PRODUCT_E141F 0x141F -#define HUAWEI_PRODUCT_E1420 0x1420 -#define HUAWEI_PRODUCT_E1421 0x1421 -#define HUAWEI_PRODUCT_E1422 0x1422 -#define HUAWEI_PRODUCT_E1423 0x1423 -#define HUAWEI_PRODUCT_E1424 0x1424 -#define HUAWEI_PRODUCT_E1425 0x1425 -#define HUAWEI_PRODUCT_E1426 0x1426 -#define HUAWEI_PRODUCT_E1427 0x1427 -#define HUAWEI_PRODUCT_E1428 0x1428 -#define HUAWEI_PRODUCT_E1429 0x1429 -#define HUAWEI_PRODUCT_E142A 0x142A -#define HUAWEI_PRODUCT_E142B 0x142B -#define HUAWEI_PRODUCT_E142C 0x142C -#define HUAWEI_PRODUCT_E142D 0x142D -#define HUAWEI_PRODUCT_E142E 0x142E -#define HUAWEI_PRODUCT_E142F 0x142F -#define HUAWEI_PRODUCT_E1430 0x1430 -#define HUAWEI_PRODUCT_E1431 0x1431 -#define HUAWEI_PRODUCT_E1432 0x1432 -#define HUAWEI_PRODUCT_E1433
Re: [PATCH] usb: host: ehci-platform: add pm_runtime_xx()
Hi Alan
Cc Magnus, Rafael
> > this patch enable to use pm_runtime_xxx() on ehci-platform
> > if .config has CONFIG_PM_RUNTIME, otherwise, these are ignored
> >
> > Signed-off-by: Kuninori Morimoto
> > ---
> > drivers/usb/host/ehci-platform.c |6 ++
> > 1 files changed, 6 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/usb/host/ehci-platform.c
> > b/drivers/usb/host/ehci-platform.c
> > index dfe881a..89029eb 100644
> > --- a/drivers/usb/host/ehci-platform.c
> > +++ b/drivers/usb/host/ehci-platform.c
> > @@ -122,6 +122,9 @@ static int __devinit ehci_platform_probe(struct
> > platform_device *dev)
> > if (err)
> > goto err_iounmap;
> >
> > + pm_runtime_enable(&dev->dev);
> > + pm_runtime_get_sync(&dev->dev);
>
> This line is peculiar. Why call pm_runtime_get_sync()? If the
> controller was at low power before, the usb_add_hcd() call above would
> have failed. If it was at full power before, this call isn't needed.
> Also it leaves the runtime PM usage counter set to 1, which will
> prevent the controller from runtime-suspending.
>
> I think what you want to do is more like this:
>
> pm_runtime_set_active(&dev->dev);
> pm_runtime_enable(&dev->dev);
Thank you for explaining this.
Indeed pm_runtime_xxx should be called befor usb_add_hcd()
But on our system (= SuperH CPU), EHCI/OHCI device are
on-chip, and it needs power to use it.
So, we need to call pm_rumtime_get_xxx() to enable power.
Without this function, our EHCI/OHCI couldn't work.
In this case, where should I call it ?
Or should I create new ehci-xxx driver ?
> > +
> > platform_set_drvdata(dev, hcd);
> >
> > return err;
> > @@ -139,6 +142,9 @@ static int __devexit ehci_platform_remove(struct
> > platform_device *dev)
> > {
> > struct usb_hcd *hcd = platform_get_drvdata(dev);
> >
> > + pm_runtime_put_sync(&dev->dev);
> > + pm_runtime_disable(&dev->dev);
>
> These look wrong also. The put_sync will set the controller to low
> power, which will cause usb_remove_hcd() below to fail.
>
> Probably you should have:
>
> pm_runtime_get_sync(&dev->dev);
>
> > usb_remove_hcd(hcd);
>
> Then here do:
>
> pm_runtime_disable(&dev->dev);
> pm_runtime_put_nosuspend(&dev->dev);
> pm_runtime_set_suspended(&dev->dev);
>
> > iounmap(hcd->regs);
> > release_mem_region(hcd->rsrc_start, hcd->rsrc_len);
>
> Similar comments apply to your patch for ohci-platform.c.
>
> 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
Best regards
---
Kuninori Morimoto
--
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: host: ehci-platform: add pm_runtime_xx()
Hi Alan
Cc Magnus, Rafael
> > > this patch enable to use pm_runtime_xxx() on ehci-platform
> > > if .config has CONFIG_PM_RUNTIME, otherwise, these are ignored
> > >
> > > Signed-off-by: Kuninori Morimoto
> > > ---
> > > drivers/usb/host/ehci-platform.c |6 ++
> > > 1 files changed, 6 insertions(+), 0 deletions(-)
> > >
> > > diff --git a/drivers/usb/host/ehci-platform.c
> > > b/drivers/usb/host/ehci-platform.c
> > > index dfe881a..89029eb 100644
> > > --- a/drivers/usb/host/ehci-platform.c
> > > +++ b/drivers/usb/host/ehci-platform.c
> > > @@ -122,6 +122,9 @@ static int __devinit ehci_platform_probe(struct
> > > platform_device *dev)
> > > if (err)
> > > goto err_iounmap;
> > >
> > > + pm_runtime_enable(&dev->dev);
> > > + pm_runtime_get_sync(&dev->dev);
> >
> > This line is peculiar. Why call pm_runtime_get_sync()? If the
> > controller was at low power before, the usb_add_hcd() call above would
> > have failed. If it was at full power before, this call isn't needed.
> > Also it leaves the runtime PM usage counter set to 1, which will
> > prevent the controller from runtime-suspending.
> >
> > I think what you want to do is more like this:
> >
> > pm_runtime_set_active(&dev->dev);
> > pm_runtime_enable(&dev->dev);
>
> Thank you for explaining this.
>
> Indeed pm_runtime_xxx should be called befor usb_add_hcd()
>
> But on our system (= SuperH CPU), EHCI/OHCI device are
> on-chip, and it needs power to use it.
> So, we need to call pm_rumtime_get_xxx() to enable power.
> Without this function, our EHCI/OHCI couldn't work.
>
> In this case, where should I call it ?
> Or should I create new ehci-xxx driver ?
I'm sorry for my confusing mail.
I would like to say is "can I call pm_runtime_get_xxx() ?"
pm_runtime_set_active(&dev->dev);
pm_runtime_enable(&dev->dev);
pm_runtime_get_xxx(&dev->dev);
usb_add_hcd()...
>
> > > +
> > > platform_set_drvdata(dev, hcd);
> > >
> > > return err;
> > > @@ -139,6 +142,9 @@ static int __devexit ehci_platform_remove(struct
> > > platform_device *dev)
> > > {
> > > struct usb_hcd *hcd = platform_get_drvdata(dev);
> > >
> > > + pm_runtime_put_sync(&dev->dev);
> > > + pm_runtime_disable(&dev->dev);
> >
> > These look wrong also. The put_sync will set the controller to low
> > power, which will cause usb_remove_hcd() below to fail.
> >
> > Probably you should have:
> >
> > pm_runtime_get_sync(&dev->dev);
> >
> > > usb_remove_hcd(hcd);
> >
> > Then here do:
> >
> > pm_runtime_disable(&dev->dev);
> > pm_runtime_put_nosuspend(&dev->dev);
> > pm_runtime_set_suspended(&dev->dev);
> >
> > > iounmap(hcd->regs);
> > > release_mem_region(hcd->rsrc_start, hcd->rsrc_len);
> >
> > Similar comments apply to your patch for ohci-platform.c.
> >
> > 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
>
>
> Best regards
> ---
> Kuninori Morimoto
Best regards
---
Kuninori Morimoto
--
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 v4 0/3] imx: add usbmisc support
usbmisc driver handles the SoC specific non-core usb registers. Changes since last version: - rebase onto torvalds latest git tree - use phandle linke usb controllers and usbmisc device. It also make usbmisc driver possible to be kernel modules. Richard Zhao (3): USB: chipidea: add imx usbmisc support ARM: imx6q: clk_register_clkdev usbmisc clock ARM: dts: imx6q-sabrelite: add usbmisc device .../devicetree/bindings/usb/ci13xxx-imx.txt|5 + .../devicetree/bindings/usb/usbmisc-imx.txt| 14 ++ arch/arm/boot/dts/imx6q-sabrelite.dts |1 + arch/arm/boot/dts/imx6q.dtsi | 10 ++ arch/arm/mach-imx/clk-imx6q.c |1 + drivers/usb/chipidea/Makefile |2 +- drivers/usb/chipidea/ci13xxx_imx.c | 64 drivers/usb/chipidea/usbmisc_imx6q.c | 162 8 files changed, 258 insertions(+), 1 deletion(-) create mode 100644 Documentation/devicetree/bindings/usb/usbmisc-imx.txt create mode 100644 drivers/usb/chipidea/usbmisc_imx6q.c -- 1.7.9.5 -- 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 v4 1/3] USB: chipidea: add imx usbmisc support
i.MX usb controllers shares non-core registers, which may include
SoC specific controls. We take it as a usbmisc device and usbmisc
driver set operations needed by ci13xxx_imx driver.
For example, Sabrelite board has bad over-current design, we can
usbmisc to disable over-current detect.
Signed-off-by: Richard Zhao
---
.../devicetree/bindings/usb/ci13xxx-imx.txt|5 +
.../devicetree/bindings/usb/usbmisc-imx.txt| 14 ++
drivers/usb/chipidea/Makefile |2 +-
drivers/usb/chipidea/ci13xxx_imx.c | 64
drivers/usb/chipidea/usbmisc_imx6q.c | 162
5 files changed, 246 insertions(+), 1 deletion(-)
create mode 100644 Documentation/devicetree/bindings/usb/usbmisc-imx.txt
create mode 100644 drivers/usb/chipidea/usbmisc_imx6q.c
diff --git a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
index 2c29041..5778b9c 100644
--- a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
+++ b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
@@ -7,7 +7,10 @@ Required properties:
Optional properties:
- fsl,usbphy: phandler of usb phy that connects to the only one port
+- fsl,usbmisc: phandler of non-core register device, with one argument
+ that indicate usb controller index
- vbus-supply: regulator for vbus
+- disable-over-current: disable over current detect
Examples:
usb@02184000 { /* USB OTG */
@@ -15,4 +18,6 @@ usb@02184000 { /* USB OTG */
reg = <0x02184000 0x200>;
interrupts = <0 43 0x04>;
fsl,usbphy = <&usbphy1>;
+ fsl,usbmisc = <&usbmisc 0>;
+ disable-over-current;
};
diff --git a/Documentation/devicetree/bindings/usb/usbmisc-imx.txt
b/Documentation/devicetree/bindings/usb/usbmisc-imx.txt
new file mode 100644
index 000..97ce94e
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/usbmisc-imx.txt
@@ -0,0 +1,14 @@
+* Freescale i.MX non-core registers
+
+Required properties:
+- #index-cells: Cells used to descibe usb controller index. Should be <1>
+- compatible: Should be one of below:
+ "fsl,imx6q-usbmisc" for imx6q
+- reg: Should contain registers location and length
+
+Examples:
+usbmisc@02184800 {
+ #index-cells = <1>;
+ compatible = "fsl,imx6q-usbmisc";
+ reg = <0x02184800 0x200>;
+};
diff --git a/drivers/usb/chipidea/Makefile b/drivers/usb/chipidea/Makefile
index 5c66d9c..57e510f 100644
--- a/drivers/usb/chipidea/Makefile
+++ b/drivers/usb/chipidea/Makefile
@@ -15,5 +15,5 @@ ifneq ($(CONFIG_PCI),)
endif
ifneq ($(CONFIG_OF_DEVICE),)
- obj-$(CONFIG_USB_CHIPIDEA) += ci13xxx_imx.o
+ obj-$(CONFIG_USB_CHIPIDEA) += ci13xxx_imx.o usbmisc_imx6q.o
endif
diff --git a/drivers/usb/chipidea/ci13xxx_imx.c
b/drivers/usb/chipidea/ci13xxx_imx.c
index ef60d06..05cd0ae 100644
--- a/drivers/usb/chipidea/ci13xxx_imx.c
+++ b/drivers/usb/chipidea/ci13xxx_imx.c
@@ -22,6 +22,7 @@
#include
#include "ci.h"
+#include "ci13xxx_imx.h"
#define pdev_to_phy(pdev) \
((struct usb_phy *)platform_get_drvdata(pdev))
@@ -34,6 +35,55 @@ struct ci13xxx_imx_data {
struct regulator *reg_vbus;
};
+static const struct usbmisc_ops *usbmisc_ops;
+
+/* Common functions shared by usbmisc drivers */
+
+int usbmisc_set_ops(const struct usbmisc_ops *ops)
+{
+ if (usbmisc_ops)
+ return -EBUSY;
+
+ usbmisc_ops = ops;
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(usbmisc_set_ops);
+
+void usbmisc_unset_ops(const struct usbmisc_ops *ops)
+{
+ usbmisc_ops = NULL;
+}
+EXPORT_SYMBOL_GPL(usbmisc_unset_ops);
+
+int usbmisc_get_init_data(struct device *dev, struct usbmisc_usb_device
*usbdev)
+{
+ struct device_node *np = dev->of_node;
+ struct of_phandle_args args;
+ int ret;
+
+ usbdev->dev = dev;
+
+ ret = of_parse_phandle_with_args(np, "fsl,usbmisc", "#index-cells",
+ 0, &args);
+ if (ret) {
+ dev_err(dev, "Failed to parse property fsl,usbmisc, errno %d\n",
+ ret);
+ memset(usbdev, 0, sizeof(*usbdev));
+ return ret;
+ }
+ usbdev->index = args.args[0];
+ of_node_put(args.np);
+
+ if (of_find_property(np, "disable-over-current", NULL))
+ usbdev->disable_oc = 1;
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(usbmisc_get_init_data);
+
+/* End of common functions shared by usbmisc drivers*/
+
static struct ci13xxx_platform_data ci13xxx_imx_platdata __devinitdata = {
.name = "ci13xxx_imx",
.flags = CI13XXX_REQUIRE_TRANSCEIVER |
@@ -51,6 +101,10 @@ static int __devinit ci13xxx_imx_probe(struct
platform_device *pdev)
struct regulator *reg_vbus;
int ret;
+ if (of_find_property(pdev->dev.of_node, "fsl,usbmisc", NULL)
+ && !usbmisc_ops)
+ return -EPROBE_DEFER;
+
data =
[PATCH v4 2/3] ARM: imx6q: clk_register_clkdev usbmisc clock
Signed-off-by: Richard Zhao --- arch/arm/mach-imx/clk-imx6q.c |1 + 1 file changed, 1 insertion(+) diff --git a/arch/arm/mach-imx/clk-imx6q.c b/arch/arm/mach-imx/clk-imx6q.c index ea89520..660fcbb 100644 --- a/arch/arm/mach-imx/clk-imx6q.c +++ b/arch/arm/mach-imx/clk-imx6q.c @@ -405,6 +405,7 @@ int __init mx6q_clocks_init(void) clk_register_clkdev(clk[usboh3], NULL, "2184200.usb"); clk_register_clkdev(clk[usboh3], NULL, "2184400.usb"); clk_register_clkdev(clk[usboh3], NULL, "2184600.usb"); + clk_register_clkdev(clk[usboh3], NULL, "2184800.usbmisc"); clk_register_clkdev(clk[usbphy1], NULL, "20c9000.usbphy"); clk_register_clkdev(clk[usbphy2], NULL, "20ca000.usbphy"); clk_register_clkdev(clk[uart_serial], "per", "202.serial"); -- 1.7.9.5 -- 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 v4 3/3] ARM: dts: imx6q-sabrelite: add usbmisc device
- add usbmisc device
- set property fsl,usbmisc for usb controllers
Signed-off-by: Richard Zhao
---
arch/arm/boot/dts/imx6q-sabrelite.dts |1 +
arch/arm/boot/dts/imx6q.dtsi | 10 ++
2 files changed, 11 insertions(+)
diff --git a/arch/arm/boot/dts/imx6q-sabrelite.dts
b/arch/arm/boot/dts/imx6q-sabrelite.dts
index d42e851..b32ebfc 100644
--- a/arch/arm/boot/dts/imx6q-sabrelite.dts
+++ b/arch/arm/boot/dts/imx6q-sabrelite.dts
@@ -62,6 +62,7 @@
aips-bus@0210 { /* AIPS2 */
usb@02184000 { /* USB OTG */
vbus-supply = <®_usb_otg_vbus>;
+ disable-over-current;
status = "okay";
};
diff --git a/arch/arm/boot/dts/imx6q.dtsi b/arch/arm/boot/dts/imx6q.dtsi
index 3d3c64b..a6e1816 100644
--- a/arch/arm/boot/dts/imx6q.dtsi
+++ b/arch/arm/boot/dts/imx6q.dtsi
@@ -629,6 +629,7 @@
reg = <0x02184000 0x200>;
interrupts = <0 43 0x04>;
fsl,usbphy = <&usbphy1>;
+ fsl,usbmisc = <&usbmisc 0>;
status = "disabled";
};
@@ -637,6 +638,7 @@
reg = <0x02184200 0x200>;
interrupts = <0 40 0x04>;
fsl,usbphy = <&usbphy2>;
+ fsl,usbmisc = <&usbmisc 1>;
status = "disabled";
};
@@ -644,6 +646,7 @@
compatible = "fsl,imx6q-usb", "fsl,imx27-usb";
reg = <0x02184400 0x200>;
interrupts = <0 41 0x04>;
+ fsl,usbmisc = <&usbmisc 2>;
status = "disabled";
};
@@ -651,9 +654,16 @@
compatible = "fsl,imx6q-usb", "fsl,imx27-usb";
reg = <0x02184600 0x200>;
interrupts = <0 42 0x04>;
+ fsl,usbmisc = <&usbmisc 3>;
status = "disabled";
};
+ usbmisc: usbmisc@02184800 {
+ #index-cells = <1>;
+ compatible = "fsl,imx6q-usbmisc";
+ reg = <0x02184800 0x200>;
+ };
+
ethernet@02188000 {
compatible = "fsl,imx6q-fec";
reg = <0x02188000 0x4000>;
--
1.7.9.5
--
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: USB UPS slows boot
On 07/29/12 17:36, Alan Stern wrote: > On Sun, 29 Jul 2012, James wrote: > It noticeably slows down booting. >>> >>> It shouldn't interfere appreciably with booting, unless you're waiting >>> for some other USB device to be detected. All the USB communication >>> should happen in the background. >> >> It does wait until the UPS is stable, I don't know why. > > I don't know why either. > >>> If you want to get more information about what's happening, you can use >>> usbmon to record the communications when the UPS is plugged in (see >>> Documentation/usb/usbmon.txt for instructions). >> >> I read the document but I didn't see any way to turn it on while the OS is >> booting. > > No, there's no way to do that, unfortunately. > >> My UPS only has one disconnect/connect if I unplug/plug in the cable >> after the OS is done booting. > > In your previous email these was an example with more than one. > >> Maybe the UPS is trying to do something and the USB system is not >> fully initialized. > > I doubt that very much. More likely something is wrong with the UPS. > > One possibility is that the problem arises because on your system, > ohci-hcd is loaded before ehci-hcd. Normally this doesn't matter, but > perhaps your UPS is sensitive to the disruption that occurs when the > EHCI controller takes over the port and then hands it back to the OHCI > controller. > > In fact, there ought to be a message in the log warning about this, > somewhere before the start of the extracts you posted. > > 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 > It doesn't slow my boot anymore but there are 9 disconnect-connects. I changed the port and recompiled the kernel. -- 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
[no subject]
Hi Laurent, I have a query for you regarding the support and testing of MJPEG frame type in the UVC webcam gadget. I see that in the webcam.c gadget, the 720p and VGA MJPEG uvc formats are supported. I was trying the same out and got confused because the data arriving from a real video capture video supporting JPEG will have no fixed size. We will have the JPEG defined Start-of-Frame and End-of-Frame markers defining the boundary of the JPEG frame. But for almost all JPEG video capture devices even if we have kept a frame size of VGA initially, the final frame size will be a compressed version (with the compression depending on the nature of the scene, so a flat scene will have high compression and hence less frame size) of VGA and will not be equal to 640 * 480. So I couldn't exactly get why the dwMaxVideoFrameBufferSize is kept as 614400 in webcam.c (see [1]). Can you please let me know your opinions and how you tested the UVC gadget's MJPEG frame format. [1] http://lxr.linux.no/linux+v3.5/drivers/usb/gadget/webcam.c#L232 Thanks, Bhupesh -- 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
Query regarding the support and testing of MJPEG frame type in the UVC webcam gadget
Hi Laurent, I have a query for you regarding the support and testing of MJPEG frame type in the UVC webcam gadget. I see that in the webcam.c gadget, the 720p and VGA MJPEG uvc formats are supported. I was trying the same out and got confused because the data arriving from a real video capture video supporting JPEG will have no fixed size. We will have the JPEG defined Start-of-Frame and End-of-Frame markers defining the boundary of the JPEG frame. But for almost all JPEG video capture devices even if we have kept a frame size of VGA initially, the final frame size will be a compressed version (with the compression depending on the nature of the scene, so a flat scene will have high compression and hence less frame size) of VGA and will not be equal to 640 * 480. So I couldn't exactly get why the dwMaxVideoFrameBufferSize is kept as 614400 in webcam.c (see [1]). Can you please let me know your opinions and how you tested the UVC gadget's MJPEG frame format. [1] http://lxr.linux.no/linux+v3.5/drivers/usb/gadget/webcam.c#L232 Thanks, Bhupesh -- 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: [RFC/PATCH v3] usb: dwc3: Introduce OTG driver for dwc3
Hi Ido,
> -Original Message-
> From: linux-usb-ow...@vger.kernel.org [mailto:linux-usb-
> ow...@vger.kernel.org] On Behalf Of Ido Shayevitz
> Sent: Monday, July 30, 2012 10:16 PM
> To: Anton Tikhomirov
> Cc: 'Ido Shayevitz'; 'Felipe Balbi'; linux-usb@vger.kernel.org
> Subject: RE: [RFC/PATCH v3] usb: dwc3: Introduce OTG driver for dwc3
>
> Hi Anton,
>
> On Mon, July 30, 2012 5:25 am, Anton Tikhomirov wrote:
> > Hi Ido,
> >
> >> >
> >> > You activate sm only if gadget and host are ready. At the same time,
> >> > in dwc3_otg_interrupt() you schedule work if interrupt happens. In
> >> > situation
> >> > when host is not set yet, but cable is plugged, we will have unwanted
> >> sm
> >> > activation
> >> > (in interrupt handler) and, as a consequence, repeating error "unable
> >> to
> >> > start A-device\n"
> >> > in dwc3_otg_sm_work().
> >>
> >> Host and gadget should set themselves to the otg on drivers probe, in
> >> boot
> >> time. So cable connect happens later.
> >> If the scenario you describe does happen it means that the xHCI driver
> >> was
> >> not loaded into the kernel, but cable with micro-A was plugged into
> your
> >> device, so need add host support to the menuconfig.
> >> It should be enough to select in the menuconfig CONFIG_USB and
> >> CONFIG_USB_XHCI_HCD.
> >
> > Agree. But what if my controller supports DRD mode, but I _want_ to keep
> > this
> > option (XHCI support) off, for any reason. By the way, it seems we will
> > have
> > similar effect when micro-A cable is plugged after
otg_set_host(phy->otg,
> > NULL)
> > call. Of course such situations are rare.
> >
>
> OK, so I will avoid the re-schedule of the sm_work after this error will
> be printed (so will be printed once...)
> Thanks...
One more thing. Currently the work is first time scheduled in two cases:
when
interrupt happens or both, peripheral and host, were set. If host is not set
(and probably won't be) the work will _not_ be scheduled (and peripheral
will
not start) till we connect and then disconnect micro-A cable. In other
words:
we should be able to use peripheral even if host is not supported (and vice
versa?). What's your opinion?
>
> >> But if your controller DWC_MODE (see core.c) does not support DRD, or
> >> the
> >> cable plugged in is micro-B then this error should not be printed
> >> eventhough the host was not set.
> >>
> >> >> >> +} else {
> >> >> >> +if (otg->phy->state == OTG_STATE_A_HOST) {
> >> >> >> +dwc3_otg_start_host(otg, 0);
> >> >> >> +otg->host = NULL;
> >> >> >> +otg->phy->state = OTG_STATE_UNDEFINED;
> >> >> >> +schedule_work(&dotg->sm_work);
> >> >> >> +} else {
> >> >> >> +otg->host = NULL;
> >> >> >> +}
> >> >> >> +}
> >> >> >> +
> >> >> >> +return 0;
> >> >> >> +}
> >> >> >> +
> >> >> >> +/**
> >> >> >> + * dwc3_otg_start_peripheral - bind/unbind the peripheral
> >> >> controller.
> >> >> >> + *
> >> >> >> + * @otg: Pointer to the otg_transceiver structure.
> >> >> >> + * @gadget: pointer to the usb_gadget structure.
> >> >> >
> >> >> > This comment doesn't match the function definition (@on, not
> > @gadget).
> >> >> >
> >> >> >> + *
> >> >> >> + * Returns 0 on success otherwise negative errno.
> >> >> >> + */
> >> >> >> +static int dwc3_otg_start_peripheral(struct usb_otg *otg, int
on)
> >> >> >> +{
> >> >> >> +struct dwc3_otg *dotg = container_of(otg, struct dwc3_otg,
> >> otg);
> >> >> >> +
> >> >> >> +if (!otg->gadget)
> >> >> >> +return -EINVAL;
> >> >> >> +
> >> >> >> +if (on) {
> >> >> >> +dev_dbg(otg->phy->dev, "%s: turn on gadget %s\n",
> >> >> >> +__func__,
> > otg->gadget->name);
> >> >> >> +dwc3_otg_set_peripheral_regs(dotg);
> >> >> >> +usb_gadget_vbus_connect(otg->gadget);
> >> >> >> +} else {
> >> >> >> +dev_dbg(otg->phy->dev, "%s: turn off gadget %s\n",
> >> >> >> +__func__,
> > otg->gadget->name);
> >> >> >> +usb_gadget_vbus_disconnect(otg->gadget);
> >> >> >> +}
> >> >> >> +
> >> >> >> +return 0;
> >> >> >> +}
> >> >> >> +
> >> >> >> +/**
> >> >> >> + * dwc3_otg_set_peripheral - bind/unbind the peripheral
> >> controller
> >> >> >> driver.
> >> >> >> + *
> >> >> >> + * @otg: Pointer to the otg_transceiver structure.
> >> >> >> + * @gadget: pointer to the usb_gadget structure.
> >> >> >> + *
> >> >> >> + * Returns 0 on success otherwise negative errno.
> >> >> >> + */
> >> >> >> +static int dwc3_otg_set_peripheral(struct usb_otg *otg,
> >> >> >> +struct usb_gadget *gadget)
> >> >> >> +{
> >> >> >> +struct dwc3_otg *dotg = container_of(otg, struct dwc3_otg,
> >> otg);
> >> >> >> +
> >> >> >> +if (gadget) {
> >> >> >> +dev_dbg(otg->phy->dev, "%s: set gadget %s\n",
> >> >> >> +
Re: [PATCH v4 1/3] USB: chipidea: add imx usbmisc support
On Wed, Aug 01, 2012 at 10:51:07AM +0800, Richard Zhao wrote:
> i.MX usb controllers shares non-core registers, which may include
> SoC specific controls. We take it as a usbmisc device and usbmisc
> driver set operations needed by ci13xxx_imx driver.
>
> For example, Sabrelite board has bad over-current design, we can
> usbmisc to disable over-current detect.
>
> Signed-off-by: Richard Zhao
> ---
> .../devicetree/bindings/usb/ci13xxx-imx.txt|5 +
> .../devicetree/bindings/usb/usbmisc-imx.txt| 14 ++
> drivers/usb/chipidea/Makefile |2 +-
> drivers/usb/chipidea/ci13xxx_imx.c | 64
> drivers/usb/chipidea/usbmisc_imx6q.c | 162
>
> 5 files changed, 246 insertions(+), 1 deletion(-)
> create mode 100644 Documentation/devicetree/bindings/usb/usbmisc-imx.txt
> create mode 100644 drivers/usb/chipidea/usbmisc_imx6q.c
>
> diff --git a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> index 2c29041..5778b9c 100644
> --- a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> +++ b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> @@ -7,7 +7,10 @@ Required properties:
>
> Optional properties:
> - fsl,usbphy: phandler of usb phy that connects to the only one port
> +- fsl,usbmisc: phandler of non-core register device, with one argument
> + that indicate usb controller index
> - vbus-supply: regulator for vbus
> +- disable-over-current: disable over current detect
>
> Examples:
> usb@02184000 { /* USB OTG */
> @@ -15,4 +18,6 @@ usb@02184000 { /* USB OTG */
> reg = <0x02184000 0x200>;
> interrupts = <0 43 0x04>;
> fsl,usbphy = <&usbphy1>;
> + fsl,usbmisc = <&usbmisc 0>;
> + disable-over-current;
> };
> diff --git a/Documentation/devicetree/bindings/usb/usbmisc-imx.txt
> b/Documentation/devicetree/bindings/usb/usbmisc-imx.txt
> new file mode 100644
> index 000..97ce94e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/usbmisc-imx.txt
> @@ -0,0 +1,14 @@
> +* Freescale i.MX non-core registers
> +
> +Required properties:
> +- #index-cells: Cells used to descibe usb controller index. Should be <1>
> +- compatible: Should be one of below:
> + "fsl,imx6q-usbmisc" for imx6q
> +- reg: Should contain registers location and length
> +
> +Examples:
> +usbmisc@02184800 {
> + #index-cells = <1>;
> + compatible = "fsl,imx6q-usbmisc";
> + reg = <0x02184800 0x200>;
> +};
> diff --git a/drivers/usb/chipidea/Makefile b/drivers/usb/chipidea/Makefile
> index 5c66d9c..57e510f 100644
> --- a/drivers/usb/chipidea/Makefile
> +++ b/drivers/usb/chipidea/Makefile
> @@ -15,5 +15,5 @@ ifneq ($(CONFIG_PCI),)
> endif
>
> ifneq ($(CONFIG_OF_DEVICE),)
> - obj-$(CONFIG_USB_CHIPIDEA) += ci13xxx_imx.o
> + obj-$(CONFIG_USB_CHIPIDEA) += ci13xxx_imx.o usbmisc_imx6q.o
> endif
> diff --git a/drivers/usb/chipidea/ci13xxx_imx.c
> b/drivers/usb/chipidea/ci13xxx_imx.c
> index ef60d06..05cd0ae 100644
> --- a/drivers/usb/chipidea/ci13xxx_imx.c
> +++ b/drivers/usb/chipidea/ci13xxx_imx.c
> @@ -22,6 +22,7 @@
> #include
>
> #include "ci.h"
> +#include "ci13xxx_imx.h"
>
> #define pdev_to_phy(pdev) \
> ((struct usb_phy *)platform_get_drvdata(pdev))
> @@ -34,6 +35,55 @@ struct ci13xxx_imx_data {
> struct regulator *reg_vbus;
> };
>
> +static const struct usbmisc_ops *usbmisc_ops;
> +
> +/* Common functions shared by usbmisc drivers */
> +
> +int usbmisc_set_ops(const struct usbmisc_ops *ops)
> +{
> + if (usbmisc_ops)
> + return -EBUSY;
> +
> + usbmisc_ops = ops;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(usbmisc_set_ops);
> +
> +void usbmisc_unset_ops(const struct usbmisc_ops *ops)
> +{
> + usbmisc_ops = NULL;
> +}
> +EXPORT_SYMBOL_GPL(usbmisc_unset_ops);
> +
> +int usbmisc_get_init_data(struct device *dev, struct usbmisc_usb_device
> *usbdev)
> +{
> + struct device_node *np = dev->of_node;
> + struct of_phandle_args args;
> + int ret;
> +
> + usbdev->dev = dev;
> +
> + ret = of_parse_phandle_with_args(np, "fsl,usbmisc", "#index-cells",
> + 0, &args);
> + if (ret) {
> + dev_err(dev, "Failed to parse property fsl,usbmisc, errno %d\n",
> + ret);
> + memset(usbdev, 0, sizeof(*usbdev));
> + return ret;
> + }
> + usbdev->index = args.args[0];
> + of_node_put(args.np);
> +
> + if (of_find_property(np, "disable-over-current", NULL))
> + usbdev->disable_oc = 1;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(usbmisc_get_init_data);
> +
> +/* End of common functions shared by usbmisc drivers*/
> +
> static struct ci13xxx_platform_data ci13xxx_imx_platdata __devinitdata = {
> .name = "ci13xxx_imx",
> .flags = CI13XXX_REQUIRE_TRANSCEIVER |
> @@ -51,6 +101,10 @@ st
Re: (Query): [PATCH v4 2/4] usb gadget: Configure endpoint according to gadget speed.
On Mon, Jul 30, 2012 at 2:57 AM, Alan Stern wrote: > > On Sun, 29 Jul 2012, Rajaram R wrote: > > > > The ep list doesn't belong to the gadget driver; it belongs to the UDC > > > driver. The maxpacket has to be adjusted to match the value stored in > > > the descriptor so that the UDC will tell the hardware to use the right > > > maxpacket value. > > > > The ep list is owned by UDC and thus the function or class should not > > update it. > > This will create problem when we switch functions > > > > For example: > > > > Lets assume the UDC has an interrupt EP in the ep-list with maxpktsize > > say 32. When for the 1st time a class a requires interrupt EP, chooses > > this EP and adjusts the size to say 8. Then when we switch to a > > function that requies an interrupt EP of size say 10. This time though > > the UDC is capable of supporting this requirement, EP autoconf will > > not be successful. > > > > So IMO, classes should not update eplist. This patch has to be > > corrected and pls let me know if I am missing something? > > Ah, I see what you mean. But the problem is even worse than you think, > because several of the UDC drivers adjust ep->maxpacket in this same > way when the endpoint is enabled. OK. In this case, the patch is doing it in composite framework and will affect all UDC drivers irrespective of whether they update or not. Will send out a patch to correct the composite driver initially. > > The documentation in include/linux/usb/gadget.h should be updated to > indicate that the maxpacket value is the largest allowed by the > hardware, not the endpoint's current setting. The same is probably > true for maxburst and mult. > > 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 v4 1/3] USB: chipidea: add imx usbmisc support
> Could it be that something is missing in this patch? I don't see struct > usbmisc_usb_device defined anywhere in this patch. ah, Sorry I missed the header file. I'll send a new version. Thanks Richard > > 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 > -- 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
