* [PATCH] usb: gadget: get rid of USB_GADGET_DUALSPEED and USB_GADGET_SUPERSPEED
[not found] <4E4B9D9C.2010607@linutronix.de>
@ 2011-08-17 13:03 ` Michal Nazarewicz
2011-08-17 14:20 ` Alan Stern
2011-08-17 14:36 ` Sergei Shtylyov
2011-08-17 15:33 ` [PATCHv2] " Michal Nazarewicz
1 sibling, 2 replies; 70+ messages in thread
From: Michal Nazarewicz @ 2011-08-17 13:03 UTC (permalink / raw)
To: Sebastian Andrzej Siewior, Yang Rui Rui, Felipe Balbi
Cc: Greg Kroah-Hartman, linux-usb, linux-kernel
From: Michal Nazarewicz <mina86@mina86.com>
This commit removes the use of USB_GADGET_DUALSPEED and
USB_GADGET_SUPERSPEED Kconfig options. Those were selected
by UDC drivers which supported respective speeds.
However, since kernel now allows multiple UDC drivers to be
compiled, the options in question may no longer reflect the
state of all gadgets.
For instance, if one driver that supports dual speed is selected
and another that does not, the USB_GADGE_DUALSPEED will be set
"for both".
This commit replaces all the #ifdefs by a run-time checks made
by calling gadget_is_dualspeed().
Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
---
drivers/usb/gadget/Kconfig | 30 -------------------
drivers/usb/gadget/composite.c | 9 +-----
drivers/usb/gadget/file_storage.c | 4 --
drivers/usb/gadget/inode.c | 18 ++---------
drivers/usb/gadget/printer.c | 59 ++++++++++--------------------------
drivers/usb/gadget/u_ether.c | 7 ----
6 files changed, 21 insertions(+), 106 deletions(-)
Please note that because I'm currently lacking hardware to test it
(and PandaBoard seem out of stock everywhere), the code is
compile-tested only.
It is applied on top of next-20110817.
diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
index 5a084b9..c60c167 100644
--- a/drivers/usb/gadget/Kconfig
+++ b/drivers/usb/gadget/Kconfig
@@ -133,7 +133,6 @@ config USB_AT91
config USB_ATMEL_USBA
tristate "Atmel USBA"
- select USB_GADGET_DUALSPEED
depends on AVR32 || ARCH_AT91CAP9 || ARCH_AT91SAM9RL || ARCH_AT91SAM9G45
help
USBA is the integrated high-speed USB Device controller on
@@ -142,7 +141,6 @@ config USB_ATMEL_USBA
config USB_FSL_USB2
tristate "Freescale Highspeed USB DR Peripheral Controller"
depends on FSL_SOC || ARCH_MXC
- select USB_GADGET_DUALSPEED
select USB_FSL_MPH_DR_OF if OF
help
Some of Freescale PowerPC processors have a High Speed
@@ -158,7 +156,6 @@ config USB_FSL_USB2
config USB_FUSB300
tristate "Faraday FUSB300 USB Peripheral Controller"
depends on !PHYS_ADDR_T_64BIT
- select USB_GADGET_DUALSPEED
help
Faraday usb device controller FUSB300 driver
@@ -206,7 +203,6 @@ config USB_PXA25X_SMALL
config USB_R8A66597
tristate "Renesas R8A66597 USB Peripheral Controller"
- select USB_GADGET_DUALSPEED
help
R8A66597 is a discrete USB host and peripheral controller chip that
supports both full and high speed USB 2.0 data transfers.
@@ -220,7 +216,6 @@ config USB_RENESAS_USBHS_UDC
tristate 'Renesas USBHS controller'
depends on SUPERH || ARCH_SHMOBILE
depends on USB_RENESAS_USBHS
- select USB_GADGET_DUALSPEED
help
Renesas USBHS is a discrete USB host and peripheral controller chip
that supports both full and high speed USB 2.0 data transfers.
@@ -249,7 +244,6 @@ config USB_S3C_HSOTG
tristate "S3C HS/OtG USB Device controller"
depends on S3C_DEV_USB_HSOTG
select USB_GADGET_S3C_HSOTG_PIO
- select USB_GADGET_DUALSPEED
help
The Samsung S3C64XX USB2.0 high-speed gadget controller
integrated into the S3C64XX series SoC.
@@ -287,7 +281,6 @@ config USB_S3C2410_DEBUG
config USB_S3C_HSUDC
tristate "S3C2416, S3C2443 and S3C2450 USB Device Controller"
depends on ARCH_S3C2410
- select USB_GADGET_DUALSPEED
help
Samsung's S3C2416, S3C2443 and S3C2450 is an ARM9 based SoC
integrated with dual speed USB 2.0 device controller. It has
@@ -298,7 +291,6 @@ config USB_S3C_HSUDC
config USB_PXA_U2O
tristate "PXA9xx Processor USB2.0 controller"
depends on ARCH_MMP
- select USB_GADGET_DUALSPEED
help
PXA9xx Processor series include a high speed USB2.0 device
controller, which support high speed and full speed USB peripheral.
@@ -311,14 +303,12 @@ config USB_PXA_U2O
config USB_GADGET_MUSB_HDRC
tristate "Inventra HDRC USB Peripheral (TI, ADI, ...)"
depends on USB_MUSB_HDRC
- select USB_GADGET_DUALSPEED
help
This OTG-capable silicon IP is used in dual designs including
the TI DaVinci, OMAP 243x, OMAP 343x, TUSB 6010, and ADI Blackfin
config USB_M66592
tristate "Renesas M66592 USB Peripheral Controller"
- select USB_GADGET_DUALSPEED
help
M66592 is a discrete USB peripheral controller chip that
supports both full and high speed USB 2.0 data transfers.
@@ -335,7 +325,6 @@ config USB_M66592
config USB_AMD5536UDC
tristate "AMD5536 UDC"
depends on PCI
- select USB_GADGET_DUALSPEED
help
The AMD5536 UDC is part of the AMD Geode CS5536, an x86 southbridge.
It is a USB Highspeed DMA capable USB device controller. Beside ep0
@@ -363,7 +352,6 @@ config USB_FSL_QE
config USB_CI13XXX_PCI
tristate "MIPS USB CI13xxx PCI UDC"
depends on PCI
- select USB_GADGET_DUALSPEED
help
MIPS USB IP core family device controller
Currently it only supports IP part number CI13412
@@ -374,7 +362,6 @@ config USB_CI13XXX_PCI
config USB_NET2272
tristate "PLX NET2272"
- select USB_GADGET_DUALSPEED
help
PLX NET2272 is a USB peripheral controller which supports
both full and high speed USB 2.0 data transfers.
@@ -398,7 +385,6 @@ config USB_NET2272_DMA
config USB_NET2280
tristate "NetChip 228x"
depends on PCI
- select USB_GADGET_DUALSPEED
help
NetChip 2280 / 2282 is a PCI based USB peripheral controller which
supports both full and high speed USB 2.0 data transfers.
@@ -429,7 +415,6 @@ config USB_LANGWELL
tristate "Intel Langwell USB Device Controller"
depends on PCI
depends on !PHYS_ADDR_T_64BIT
- select USB_GADGET_DUALSPEED
help
Intel Langwell USB Device Controller is a High-Speed USB
On-The-Go device controller.
@@ -444,7 +429,6 @@ config USB_LANGWELL
config USB_EG20T
tristate "Intel EG20T PCH/OKI SEMICONDUCTOR ML7213 IOH UDC"
depends on PCI
- select USB_GADGET_DUALSPEED
help
This is a USB device driver for EG20T PCH.
EG20T PCH is the platform controller hub that is used in Intel's
@@ -466,7 +450,6 @@ config USB_EG20T
config USB_CI13XXX_MSM
tristate "MIPS USB CI13xxx for MSM"
depends on ARCH_MSM
- select USB_GADGET_DUALSPEED
select USB_MSM_OTG
help
MSM SoC has chipidea USB controller. This driver uses
@@ -487,8 +470,6 @@ config USB_CI13XXX_MSM
config USB_DUMMY_HCD
tristate "Dummy HCD (DEVELOPMENT)"
depends on USB=y || (USB=m && USB_GADGET=m)
- select USB_GADGET_DUALSPEED
- select USB_GADGET_SUPERSPEED
help
This host controller driver emulates USB, looping all data transfer
requests back to a USB "gadget driver" in the same host. The host
@@ -513,17 +494,6 @@ config USB_DUMMY_HCD
endchoice
-# Selected by UDC drivers that support high-speed operation.
-config USB_GADGET_DUALSPEED
- bool
- depends on USB_GADGET
-
-# Selected by UDC drivers that support super-speed opperation
-config USB_GADGET_SUPERSPEED
- bool
- depends on USB_GADGET
- depends on USB_GADGET_DUALSPEED
-
#
# USB Gadget Drivers
#
diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index 5a3461e..a0355a3 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -1559,12 +1559,6 @@ composite_resume(struct usb_gadget *gadget)
/*-------------------------------------------------------------------------*/
static struct usb_gadget_driver composite_driver = {
-#ifdef CONFIG_USB_GADGET_SUPERSPEED
- .speed = USB_SPEED_SUPER,
-#else
- .speed = USB_SPEED_HIGH,
-#endif
-
.unbind = composite_unbind,
.setup = composite_setup,
@@ -1609,8 +1603,7 @@ int usb_composite_probe(struct usb_composite_driver *driver,
driver->iProduct = driver->name;
composite_driver.function = (char *) driver->name;
composite_driver.driver.name = driver->name;
- composite_driver.speed = min((u8)composite_driver.speed,
- (u8)driver->max_speed);
+ composite_driver.speed = min(USB_SPEED_SUPER, driver->max_speed);
composite = driver;
composite_gadget_bind = bind;
diff --git a/drivers/usb/gadget/file_storage.c b/drivers/usb/gadget/file_storage.c
index cf875be..61ed4ec 100644
--- a/drivers/usb/gadget/file_storage.c
+++ b/drivers/usb/gadget/file_storage.c
@@ -3562,11 +3562,7 @@ static void fsg_resume(struct usb_gadget *gadget)
/*-------------------------------------------------------------------------*/
static struct usb_gadget_driver fsg_driver = {
-#ifdef CONFIG_USB_GADGET_DUALSPEED
.speed = USB_SPEED_HIGH,
-#else
- .speed = USB_SPEED_FULL,
-#endif
.function = (char *) fsg_string_product,
.unbind = fsg_unbind,
.disconnect = fsg_disconnect,
diff --git a/drivers/usb/gadget/inode.c b/drivers/usb/gadget/inode.c
index 1b24099..1869478 100644
--- a/drivers/usb/gadget/inode.c
+++ b/drivers/usb/gadget/inode.c
@@ -837,7 +837,6 @@ ep_config (struct file *fd, const char __user *buf, size_t len, loff_t *ptr)
if (value == 0)
data->state = STATE_EP_ENABLED;
break;
-#ifdef CONFIG_USB_GADGET_DUALSPEED
case USB_SPEED_HIGH:
/* fails if caller didn't provide that descriptor... */
ep->desc = &data->hs_desc;
@@ -845,7 +844,6 @@ ep_config (struct file *fd, const char __user *buf, size_t len, loff_t *ptr)
if (value == 0)
data->state = STATE_EP_ENABLED;
break;
-#endif
default:
DBG(data->dev, "unconnected, %s init abandoned\n",
data->name);
@@ -1015,9 +1013,8 @@ ep0_read (struct file *fd, char __user *buf, size_t len, loff_t *ptr)
if (dev->current_config) {
unsigned power;
- if (gadget_is_dualspeed(dev->gadget)
- && (dev->gadget->speed
- == USB_SPEED_HIGH))
+ if (gadget_is_dualspeed(dev->gadget) &&
+ (dev->gadget->speed == USB_SPEED_HIGH))
power = dev->hs_config->bMaxPower;
else
power = dev->config->bMaxPower;
@@ -1331,7 +1328,6 @@ static const struct file_operations ep0_io_operations = {
* Unrecognized ep0 requests may be handled in user space.
*/
-#ifdef CONFIG_USB_GADGET_DUALSPEED
static void make_qualifier (struct dev_data *dev)
{
struct usb_qualifier_descriptor qual;
@@ -1354,10 +1350,8 @@ static void make_qualifier (struct dev_data *dev)
memcpy (dev->rbuf, &qual, sizeof qual);
}
-#endif
-static int
-config_buf (struct dev_data *dev, u8 type, unsigned index)
+static int config_buf (struct dev_data *dev, u8 type, unsigned index)
{
int len;
int hs = 0;
@@ -1434,7 +1428,6 @@ gadgetfs_setup (struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
dev->dev->bMaxPacketSize0 = dev->gadget->ep0->maxpacket;
req->buf = dev->dev;
break;
-#ifdef CONFIG_USB_GADGET_DUALSPEED
case USB_DT_DEVICE_QUALIFIER:
if (!dev->hs_config)
break;
@@ -1444,7 +1437,6 @@ gadgetfs_setup (struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
break;
case USB_DT_OTHER_SPEED_CONFIG:
// FALLTHROUGH
-#endif
case USB_DT_CONFIG:
value = config_buf (dev,
w_value >> 8,
@@ -1773,11 +1765,7 @@ gadgetfs_suspend (struct usb_gadget *gadget)
}
static struct usb_gadget_driver gadgetfs_driver = {
-#ifdef CONFIG_USB_GADGET_DUALSPEED
.speed = USB_SPEED_HIGH,
-#else
- .speed = USB_SPEED_FULL,
-#endif
.function = (char *) driver_desc,
.unbind = gadgetfs_unbind,
.setup = gadgetfs_setup,
diff --git a/drivers/usb/gadget/printer.c b/drivers/usb/gadget/printer.c
index a341dde..87f8d03 100644
--- a/drivers/usb/gadget/printer.c
+++ b/drivers/usb/gadget/printer.c
@@ -164,12 +164,6 @@ module_param(qlen, uint, S_IRUGO|S_IWUSR);
#define QLEN qlen
-#ifdef CONFIG_USB_GADGET_DUALSPEED
-#define DEVSPEED USB_SPEED_HIGH
-#else /* full speed (low speed doesn't do bulk) */
-#define DEVSPEED USB_SPEED_FULL
-#endif
-
/*-------------------------------------------------------------------------*/
#define xprintk(d, level, fmt, args...) \
@@ -288,8 +282,6 @@ static const struct usb_descriptor_header *fs_printer_function [11] = {
NULL
};
-#ifdef CONFIG_USB_GADGET_DUALSPEED
-
/*
* usb 2.0 devices need to expose both high speed and full speed
* descriptors, unless they only run at full speed.
@@ -326,14 +318,13 @@ static const struct usb_descriptor_header *hs_printer_function [11] = {
};
/* maxpacket and other transfer characteristics vary by speed. */
-#define ep_desc(g, hs, fs) (((g)->speed == USB_SPEED_HIGH)?(hs):(fs))
-
-#else
-
-/* if there's no high speed support, maxpacket doesn't change. */
-#define ep_desc(g, hs, fs) (((void)(g)), (fs))
-
-#endif /* !CONFIG_USB_GADGET_DUALSPEED */
+static inline struct usb_endpoint_descriptor *
+ep_desc(struct printer_dev *dev,
+ struct usb_endpoint_descriptor *hs,
+ struct usb_endpoint_descriptor *fs)
+{
+ return gadget_is_dualspeed(dev->gadget) ? hs : fs;
+}
/*-------------------------------------------------------------------------*/
@@ -897,11 +888,10 @@ set_printer_interface(struct printer_dev *dev)
{
int result = 0;
- dev->in_ep->desc = ep_desc(dev->gadget, &hs_ep_in_desc, &fs_ep_in_desc);
+ dev->in_ep->desc = ep_desc(dev, &hs_ep_in_desc, &fs_ep_in_desc);
dev->in_ep->driver_data = dev;
- dev->out_ep->desc = ep_desc(dev->gadget, &hs_ep_out_desc,
- &fs_ep_out_desc);
+ dev->out_ep->desc = ep_desc(dev, &hs_ep_out_desc, &fs_ep_out_desc);
dev->out_ep->driver_data = dev;
result = usb_ep_enable(dev->in_ep);
@@ -979,9 +969,7 @@ printer_set_config(struct printer_dev *dev, unsigned number)
switch (gadget->speed) {
case USB_SPEED_FULL: speed = "full"; break;
-#ifdef CONFIG_USB_GADGET_DUALSPEED
case USB_SPEED_HIGH: speed = "high"; break;
-#endif
default: speed = "?"; break;
}
@@ -998,23 +986,15 @@ config_buf(enum usb_device_speed speed, u8 *buf, u8 type, unsigned index,
{
int len;
const struct usb_descriptor_header **function;
-#ifdef CONFIG_USB_GADGET_DUALSPEED
int hs = (speed == USB_SPEED_HIGH);
+ if (index >= device_desc.bNumConfigurations)
+ return -EINVAL;
+
if (type == USB_DT_OTHER_SPEED_CONFIG)
hs = !hs;
- if (hs) {
- function = hs_printer_function;
- } else {
- function = fs_printer_function;
- }
-#else
- function = fs_printer_function;
-#endif
-
- if (index >= device_desc.bNumConfigurations)
- return -EINVAL;
+ function = hs ? hs_printer_function : fs_printer_function;
/* for now, don't advertise srp-only devices */
if (!is_otg)
@@ -1156,9 +1136,8 @@ printer_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
value = min(wLength, (u16) sizeof device_desc);
memcpy(req->buf, &device_desc, value);
break;
-#ifdef CONFIG_USB_GADGET_DUALSPEED
case USB_DT_DEVICE_QUALIFIER:
- if (!gadget->is_dualspeed)
+ if (!gadget_is_dualspeed(gadget))
break;
/*
* assumes ep0 uses the same value for both
@@ -1172,10 +1151,9 @@ printer_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
break;
case USB_DT_OTHER_SPEED_CONFIG:
- if (!gadget->is_dualspeed)
+ if (!gadget_is_dualspeed(gadget))
break;
/* FALLTHROUGH */
-#endif /* CONFIG_USB_GADGET_DUALSPEED */
case USB_DT_CONFIG:
value = config_buf(gadget->speed, req->buf,
wValue >> 8,
@@ -1460,11 +1438,9 @@ autoconf_fail:
goto autoconf_fail;
out_ep->driver_data = out_ep; /* claim */
-#ifdef CONFIG_USB_GADGET_DUALSPEED
/* assumes that all endpoints are dual-speed */
hs_ep_in_desc.bEndpointAddress = fs_ep_in_desc.bEndpointAddress;
hs_ep_out_desc.bEndpointAddress = fs_ep_out_desc.bEndpointAddress;
-#endif /* DUALSPEED */
usb_gadget_set_selfpowered(gadget);
@@ -1552,7 +1528,7 @@ fail:
/*-------------------------------------------------------------------------*/
static struct usb_gadget_driver printer_driver = {
- .speed = DEVSPEED,
+ .speed = USB_SPEED_HIGH,
.function = (char *) driver_desc,
.unbind = printer_unbind,
@@ -1570,8 +1546,7 @@ MODULE_DESCRIPTION(DRIVER_DESC);
MODULE_AUTHOR("Craig Nadler");
MODULE_LICENSE("GPL");
-static int __init
-init(void)
+static int __init init(void)
{
int status;
diff --git a/drivers/usb/gadget/u_ether.c b/drivers/usb/gadget/u_ether.c
index dfed4c1..8642abb 100644
--- a/drivers/usb/gadget/u_ether.c
+++ b/drivers/usb/gadget/u_ether.c
@@ -92,17 +92,10 @@ struct eth_dev {
#define DEFAULT_QLEN 2 /* double buffering by default */
-
-#ifdef CONFIG_USB_GADGET_DUALSPEED
-
static unsigned qmult = 5;
module_param(qmult, uint, S_IRUGO|S_IWUSR);
MODULE_PARM_DESC(qmult, "queue length multiplier at high/super speed");
-#else /* full speed (low speed doesn't do bulk) */
-#define qmult 1
-#endif
-
/* for dual-speed hardware, use deeper queues at high/super speed */
static inline int qlen(struct usb_gadget *gadget)
{
--
1.7.3.1
^ permalink raw reply related [flat|nested] 70+ messages in thread
* Re: [PATCH] usb: gadget: get rid of USB_GADGET_DUALSPEED and USB_GADGET_SUPERSPEED
2011-08-17 13:03 ` [PATCH] usb: gadget: get rid of USB_GADGET_DUALSPEED and USB_GADGET_SUPERSPEED Michal Nazarewicz
@ 2011-08-17 14:20 ` Alan Stern
2011-08-17 14:27 ` Michal Nazarewicz
2011-08-17 14:36 ` Sergei Shtylyov
1 sibling, 1 reply; 70+ messages in thread
From: Alan Stern @ 2011-08-17 14:20 UTC (permalink / raw)
To: Michal Nazarewicz
Cc: Sebastian Andrzej Siewior, Yang Rui Rui, Felipe Balbi,
Greg Kroah-Hartman, linux-usb, linux-kernel
On Wed, 17 Aug 2011, Michal Nazarewicz wrote:
> From: Michal Nazarewicz <mina86@mina86.com>
>
> This commit removes the use of USB_GADGET_DUALSPEED and
> USB_GADGET_SUPERSPEED Kconfig options. Those were selected
> by UDC drivers which supported respective speeds.
>
> However, since kernel now allows multiple UDC drivers to be
> compiled, the options in question may no longer reflect the
> state of all gadgets.
>
> For instance, if one driver that supports dual speed is selected
> and another that does not, the USB_GADGE_DUALSPEED will be set
> "for both".
>
> This commit replaces all the #ifdefs by a run-time checks made
> by calling gadget_is_dualspeed().
> @@ -1609,8 +1603,7 @@ int usb_composite_probe(struct usb_composite_driver *driver,
> driver->iProduct = driver->name;
> composite_driver.function = (char *) driver->name;
> composite_driver.driver.name = driver->name;
> - composite_driver.speed = min((u8)composite_driver.speed,
> - (u8)driver->max_speed);
> + composite_driver.speed = min(USB_SPEED_SUPER, driver->max_speed);
I wonder if the min is needed here at all. It doesn't seem to be doing
any good.
> --- a/drivers/usb/gadget/file_storage.c
> +++ b/drivers/usb/gadget/file_storage.c
> @@ -3562,11 +3562,7 @@ static void fsg_resume(struct usb_gadget *gadget)
> /*-------------------------------------------------------------------------*/
>
> static struct usb_gadget_driver fsg_driver = {
> -#ifdef CONFIG_USB_GADGET_DUALSPEED
> .speed = USB_SPEED_HIGH,
> -#else
> - .speed = USB_SPEED_FULL,
> -#endif
> .function = (char *) fsg_string_product,
> .unbind = fsg_unbind,
> .disconnect = fsg_disconnect,
You mustn't remove the .speed field entirely. Set it to USB_SPEED_HIGH.
The same goes for the other drivers below.
> diff --git a/drivers/usb/gadget/inode.c b/drivers/usb/gadget/inode.c
> index 1b24099..1869478 100644
> --- a/drivers/usb/gadget/inode.c
> +++ b/drivers/usb/gadget/inode.c
> @@ -1015,9 +1013,8 @@ ep0_read (struct file *fd, char __user *buf, size_t len, loff_t *ptr)
> if (dev->current_config) {
> unsigned power;
>
> - if (gadget_is_dualspeed(dev->gadget)
> - && (dev->gadget->speed
> - == USB_SPEED_HIGH))
> + if (gadget_is_dualspeed(dev->gadget) &&
> + (dev->gadget->speed == USB_SPEED_HIGH))
> power = dev->hs_config->bMaxPower;
> else
> power = dev->config->bMaxPower;
Please don't mix functional changes with cosmetic whitespace changes.
Alan Stern
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH] usb: gadget: get rid of USB_GADGET_DUALSPEED and USB_GADGET_SUPERSPEED
2011-08-17 14:20 ` Alan Stern
@ 2011-08-17 14:27 ` Michal Nazarewicz
2011-08-17 14:47 ` Alan Stern
0 siblings, 1 reply; 70+ messages in thread
From: Michal Nazarewicz @ 2011-08-17 14:27 UTC (permalink / raw)
To: Alan Stern
Cc: Sebastian Andrzej Siewior, Yang Rui Rui, Felipe Balbi,
Greg Kroah-Hartman, linux-usb, linux-kernel
> On Wed, 17 Aug 2011, Michal Nazarewicz wrote:
>> @@ -1609,8 +1603,7 @@ int usb_composite_probe(struct
>> usb_composite_driver *driver,
>> driver->iProduct = driver->name;
>> composite_driver.function = (char *) driver->name;
>> composite_driver.driver.name = driver->name;
>> - composite_driver.speed = min((u8)composite_driver.speed,
>> - (u8)driver->max_speed);
>> + composite_driver.speed = min(USB_SPEED_SUPER, driver->max_speed);
On Wed, 17 Aug 2011 16:20:15 +0200, Alan Stern <stern@rowland.harvard.edu>
wrote:
> I wonder if the min is needed here at all. It doesn't seem to be doing
> any good.
Do you mean just doing “composite_driver.speed = driver->max_speed”?
>> --- a/drivers/usb/gadget/file_storage.c
>> +++ b/drivers/usb/gadget/file_storage.c
>> @@ -3562,11 +3562,7 @@ static void fsg_resume(struct usb_gadget *gadget)
>> /*-------------------------------------------------------------------------*/
>>
>> static struct usb_gadget_driver fsg_driver = {
>> -#ifdef CONFIG_USB_GADGET_DUALSPEED
>> .speed = USB_SPEED_HIGH,
>> -#else
>> - .speed = USB_SPEED_FULL,
>> -#endif
>> .function = (char *) fsg_string_product,
>> .unbind = fsg_unbind,
>> .disconnect = fsg_disconnect,
> You mustn't remove the .speed field entirely. Set it to USB_SPEED_HIGH.
> The same goes for the other drivers below.
Please note that the field is not in fact removed.
I've removed it in composite_driver only but that's because it is later
set in usb_composite_probe() (ie. see hunk above).
>> diff --git a/drivers/usb/gadget/inode.c b/drivers/usb/gadget/inode.c
>> index 1b24099..1869478 100644
>> --- a/drivers/usb/gadget/inode.c
>> +++ b/drivers/usb/gadget/inode.c
>
>> @@ -1015,9 +1013,8 @@ ep0_read (struct file *fd, char __user *buf,
>> size_t len, loff_t *ptr)
>> if (dev->current_config) {
>> unsigned power;
>>
>> - if (gadget_is_dualspeed(dev->gadget)
>> - && (dev->gadget->speed
>> - == USB_SPEED_HIGH))
>> + if (gadget_is_dualspeed(dev->gadget) &&
>> + (dev->gadget->speed == USB_SPEED_HIGH))
>> power = dev->hs_config->bMaxPower;
>> else
>> power = dev->config->bMaxPower;
>
> Please don't mix functional changes with cosmetic whitespace changes.
Will fix.
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michal "mina86" Nazarewicz (o o)
ooo +-----<email/xmpp: mnazarewicz@google.com>-----ooO--(_)--Ooo--
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH] usb: gadget: get rid of USB_GADGET_DUALSPEED and USB_GADGET_SUPERSPEED
2011-08-17 13:03 ` [PATCH] usb: gadget: get rid of USB_GADGET_DUALSPEED and USB_GADGET_SUPERSPEED Michal Nazarewicz
2011-08-17 14:20 ` Alan Stern
@ 2011-08-17 14:36 ` Sergei Shtylyov
2011-08-17 14:45 ` Michal Nazarewicz
1 sibling, 1 reply; 70+ messages in thread
From: Sergei Shtylyov @ 2011-08-17 14:36 UTC (permalink / raw)
To: Michal Nazarewicz
Cc: Sebastian Andrzej Siewior, Yang Rui Rui, Felipe Balbi,
Greg Kroah-Hartman, linux-usb, linux-kernel
Hello.
Michal Nazarewicz wrote:
> From: Michal Nazarewicz <mina86@mina86.com>
> This commit removes the use of USB_GADGET_DUALSPEED and
> USB_GADGET_SUPERSPEED Kconfig options. Those were selected
> by UDC drivers which supported respective speeds.
> However, since kernel now allows multiple UDC drivers to be
> compiled, the options in question may no longer reflect the
> state of all gadgets.
> For instance, if one driver that supports dual speed is selected
> and another that does not, the USB_GADGE_DUALSPEED will be set
> "for both".
> This commit replaces all the #ifdefs by a run-time checks made
> by calling gadget_is_dualspeed().
> Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
[...]
> diff --git a/drivers/usb/gadget/inode.c b/drivers/usb/gadget/inode.c
> index 1b24099..1869478 100644
> --- a/drivers/usb/gadget/inode.c
> +++ b/drivers/usb/gadget/inode.c
[...]
> @@ -1015,9 +1013,8 @@ ep0_read (struct file *fd, char __user *buf, size_t len, loff_t *ptr)
> if (dev->current_config) {
> unsigned power;
>
> - if (gadget_is_dualspeed(dev->gadget)
> - && (dev->gadget->speed
> - == USB_SPEED_HIGH))
> + if (gadget_is_dualspeed(dev->gadget) &&
> + (dev->gadget->speed == USB_SPEED_HIGH))
As Alan noted, this is quite unrelated white space change.
> power = dev->hs_config->bMaxPower;
> else
> power = dev->config->bMaxPower;
> @@ -1354,10 +1350,8 @@ static void make_qualifier (struct dev_data *dev)
>
> memcpy (dev->rbuf, &qual, sizeof qual);
> }
> -#endif
>
> -static int
> -config_buf (struct dev_data *dev, u8 type, unsigned index)
> +static int config_buf (struct dev_data *dev, u8 type, unsigned index)
> {
> int len;
> int hs = 0;
WARNING: space prohibited between function name and open parenthesis '('
#291: FILE: drivers/usb/gadget/inode.c:1354:
+static int config_buf (struct dev_data *dev, u8 type, unsigned index)
total: 0 errors, 1 warnings, 400 lines checked
patches/usb-gadget-get-rid-of-USB_GADGET_DUALSPEED-and-USB_GADGET_SUPERSPEED.patch
has style problems, please review.
If any of these errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
WBR, Sergei
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH] usb: gadget: get rid of USB_GADGET_DUALSPEED and USB_GADGET_SUPERSPEED
2011-08-17 14:36 ` Sergei Shtylyov
@ 2011-08-17 14:45 ` Michal Nazarewicz
0 siblings, 0 replies; 70+ messages in thread
From: Michal Nazarewicz @ 2011-08-17 14:45 UTC (permalink / raw)
To: Sergei Shtylyov
Cc: Sebastian Andrzej Siewior, Yang Rui Rui, Felipe Balbi,
Greg Kroah-Hartman, linux-usb, linux-kernel
> Michal Nazarewicz wrote:
>> @@ -1354,10 +1350,8 @@ static void make_qualifier (struct dev_data *dev)
>> memcpy (dev->rbuf, &qual, sizeof qual);
>> }
>> -#endif
>> -static int
>> -config_buf (struct dev_data *dev, u8 type, unsigned index)
>> +static int config_buf (struct dev_data *dev, u8 type, unsigned index)
>> {
>> int len;
>> int hs = 0;
On Wed, 17 Aug 2011 16:36:41 +0200, Sergei Shtylyov <sshtylyov@mvista.com>
wrote:
> WARNING: space prohibited between function name and open parenthesis '('
> #291: FILE: drivers/usb/gadget/inode.c:1354:
> +static int config_buf (struct dev_data *dev, u8 type, unsigned index)
This is actually also an unrelated white-space change so I'll just remove
this
change.
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michal "mina86" Nazarewicz (o o)
ooo +-----<email/xmpp: mnazarewicz@google.com>-----ooO--(_)--Ooo--
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH] usb: gadget: get rid of USB_GADGET_DUALSPEED and USB_GADGET_SUPERSPEED
2011-08-17 14:27 ` Michal Nazarewicz
@ 2011-08-17 14:47 ` Alan Stern
2011-08-17 15:07 ` Michal Nazarewicz
0 siblings, 1 reply; 70+ messages in thread
From: Alan Stern @ 2011-08-17 14:47 UTC (permalink / raw)
To: Michal Nazarewicz
Cc: Sebastian Andrzej Siewior, Yang Rui Rui, Felipe Balbi,
Greg Kroah-Hartman, linux-usb, linux-kernel
On Wed, 17 Aug 2011, Michal Nazarewicz wrote:
> > On Wed, 17 Aug 2011, Michal Nazarewicz wrote:
> >> @@ -1609,8 +1603,7 @@ int usb_composite_probe(struct
> >> usb_composite_driver *driver,
> >> driver->iProduct = driver->name;
> >> composite_driver.function = (char *) driver->name;
> >> composite_driver.driver.name = driver->name;
> >> - composite_driver.speed = min((u8)composite_driver.speed,
> >> - (u8)driver->max_speed);
> >> + composite_driver.speed = min(USB_SPEED_SUPER, driver->max_speed);
>
> On Wed, 17 Aug 2011 16:20:15 +0200, Alan Stern <stern@rowland.harvard.edu>
> wrote:
> > I wonder if the min is needed here at all. It doesn't seem to be doing
> > any good.
>
> Do you mean just doing “composite_driver.speed = driver->max_speed”?
Yes. After all, there aren't any usb_device_speed values larger than
USB_SPEED_SUPER.
> >> --- a/drivers/usb/gadget/file_storage.c
> >> +++ b/drivers/usb/gadget/file_storage.c
> >> @@ -3562,11 +3562,7 @@ static void fsg_resume(struct usb_gadget *gadget)
> >> /*-------------------------------------------------------------------------*/
> >>
> >> static struct usb_gadget_driver fsg_driver = {
> >> -#ifdef CONFIG_USB_GADGET_DUALSPEED
> >> .speed = USB_SPEED_HIGH,
> >> -#else
> >> - .speed = USB_SPEED_FULL,
> >> -#endif
> >> .function = (char *) fsg_string_product,
> >> .unbind = fsg_unbind,
> >> .disconnect = fsg_disconnect,
>
> > You mustn't remove the .speed field entirely. Set it to USB_SPEED_HIGH.
> > The same goes for the other drivers below.
>
> Please note that the field is not in fact removed.
No, but you removed its initializer and as a result, it will be set to
0 whereas it should be set to USB_SPEED_HIGH.
Alan Stern
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH] usb: gadget: get rid of USB_GADGET_DUALSPEED and USB_GADGET_SUPERSPEED
2011-08-17 14:47 ` Alan Stern
@ 2011-08-17 15:07 ` Michal Nazarewicz
2011-08-17 16:25 ` Alan Stern
0 siblings, 1 reply; 70+ messages in thread
From: Michal Nazarewicz @ 2011-08-17 15:07 UTC (permalink / raw)
To: Alan Stern
Cc: Sebastian Andrzej Siewior, Yang Rui Rui, Felipe Balbi,
Greg Kroah-Hartman, linux-usb, linux-kernel
>>> On Wed, 17 Aug 2011, Michal Nazarewicz wrote:
>>>> @@ -1609,8 +1603,7 @@ int usb_composite_probe(struct
>>>> usb_composite_driver *driver,
>>>> driver->iProduct = driver->name;
>>>> composite_driver.function = (char *) driver->name;
>>>> composite_driver.driver.name = driver->name;
>>>> - composite_driver.speed = min((u8)composite_driver.speed,
>>>> - (u8)driver->max_speed);
>>>> + composite_driver.speed = min(USB_SPEED_SUPER, driver->max_speed);
>> On Wed, 17 Aug 2011 16:20:15 +0200, Alan Stern wrote:
>>> I wonder if the min is needed here at all. It doesn't seem to be
>>> doing any good.
> On Wed, 17 Aug 2011, Michal Nazarewicz wrote:
>> Do you mean just doing “composite_driver.speed = driver->max_speed”?
On Wed, 17 Aug 2011 16:47:37 +0200, Alan Stern <stern@rowland.harvard.edu>
wrote:
> Yes. After all, there aren't any usb_device_speed values larger than
> USB_SPEED_SUPER.
Will fix.
>>>> --- a/drivers/usb/gadget/file_storage.c
>>>> +++ b/drivers/usb/gadget/file_storage.c
>>>> @@ -3562,11 +3562,7 @@ static void fsg_resume(struct usb_gadget
>>>> *gadget)
>>>> static struct usb_gadget_driver fsg_driver = {
>>>> -#ifdef CONFIG_USB_GADGET_DUALSPEED
>>>> .speed = USB_SPEED_HIGH,
>>>> -#else
>>>> - .speed = USB_SPEED_FULL,
>>>> -#endif
>>>> .function = (char *) fsg_string_product,
>>>> .unbind = fsg_unbind,
>>>> .disconnect = fsg_disconnect,
>>> You mustn't remove the .speed field entirely. Set it to
>>> USB_SPEED_HIGH.
>> Please note that the field is not in fact removed.
> No, but you removed its initializer and as a result, it will be set to
> 0 whereas it should be set to USB_SPEED_HIGH.
The initializer is still there. ;) Only the “#” lines and the initializer
in the “#else” part are removed.
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michal "mina86" Nazarewicz (o o)
ooo +-----<email/xmpp: mnazarewicz@google.com>-----ooO--(_)--Ooo--
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCHv2] usb: gadget: get rid of USB_GADGET_DUALSPEED and USB_GADGET_SUPERSPEED
[not found] <4E4B9D9C.2010607@linutronix.de>
2011-08-17 13:03 ` [PATCH] usb: gadget: get rid of USB_GADGET_DUALSPEED and USB_GADGET_SUPERSPEED Michal Nazarewicz
@ 2011-08-17 15:33 ` Michal Nazarewicz
2011-08-17 21:09 ` Alan Stern
` (2 more replies)
1 sibling, 3 replies; 70+ messages in thread
From: Michal Nazarewicz @ 2011-08-17 15:33 UTC (permalink / raw)
To: Alan Stern, Sergei Shtylyov, Felipe Balbi
Cc: Sebastian Andrzej Siewior, Yang Rui Rui, Greg Kroah-Hartman,
linux-usb, linux-kernel
From: Michal Nazarewicz <mina86@mina86.com>
This commit removes the use of USB_GADGET_DUALSPEED and
USB_GADGET_SUPERSPEED Kconfig options. Those were selected
by UDC drivers which supported respective speeds.
However, since kernel now allows multiple UDC drivers to be
compiled, the options in question may no longer reflect the
state of all gadgets.
For instance, if one driver that supports dual speed is selected
and another that does not, the USB_GADGE_DUALSPEED will be set
"for both".
This commit replaces all the #ifdefs by a run-time checks made
by calling gadget_is_dualspeed().
---
drivers/usb/gadget/Kconfig | 30 -------------------------
drivers/usb/gadget/composite.c | 9 +-------
drivers/usb/gadget/file_storage.c | 4 ---
drivers/usb/gadget/inode.c | 10 --------
drivers/usb/gadget/printer.c | 43 ++++++-------------------------------
drivers/usb/gadget/u_ether.c | 7 ------
6 files changed, 8 insertions(+), 95 deletions(-)
Like before, this has been only compile-tested (make allmodconfig
bzImage modules on x86_64).
Compared to v1:
* Removed unrelated whitespace changes
* Removed the use of min() in usb_composite_probe()
diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
index 5a084b9..c60c167 100644
--- a/drivers/usb/gadget/Kconfig
+++ b/drivers/usb/gadget/Kconfig
@@ -133,7 +133,6 @@ config USB_AT91
config USB_ATMEL_USBA
tristate "Atmel USBA"
- select USB_GADGET_DUALSPEED
depends on AVR32 || ARCH_AT91CAP9 || ARCH_AT91SAM9RL || ARCH_AT91SAM9G45
help
USBA is the integrated high-speed USB Device controller on
@@ -142,7 +141,6 @@ config USB_ATMEL_USBA
config USB_FSL_USB2
tristate "Freescale Highspeed USB DR Peripheral Controller"
depends on FSL_SOC || ARCH_MXC
- select USB_GADGET_DUALSPEED
select USB_FSL_MPH_DR_OF if OF
help
Some of Freescale PowerPC processors have a High Speed
@@ -158,7 +156,6 @@ config USB_FSL_USB2
config USB_FUSB300
tristate "Faraday FUSB300 USB Peripheral Controller"
depends on !PHYS_ADDR_T_64BIT
- select USB_GADGET_DUALSPEED
help
Faraday usb device controller FUSB300 driver
@@ -206,7 +203,6 @@ config USB_PXA25X_SMALL
config USB_R8A66597
tristate "Renesas R8A66597 USB Peripheral Controller"
- select USB_GADGET_DUALSPEED
help
R8A66597 is a discrete USB host and peripheral controller chip that
supports both full and high speed USB 2.0 data transfers.
@@ -220,7 +216,6 @@ config USB_RENESAS_USBHS_UDC
tristate 'Renesas USBHS controller'
depends on SUPERH || ARCH_SHMOBILE
depends on USB_RENESAS_USBHS
- select USB_GADGET_DUALSPEED
help
Renesas USBHS is a discrete USB host and peripheral controller chip
that supports both full and high speed USB 2.0 data transfers.
@@ -249,7 +244,6 @@ config USB_S3C_HSOTG
tristate "S3C HS/OtG USB Device controller"
depends on S3C_DEV_USB_HSOTG
select USB_GADGET_S3C_HSOTG_PIO
- select USB_GADGET_DUALSPEED
help
The Samsung S3C64XX USB2.0 high-speed gadget controller
integrated into the S3C64XX series SoC.
@@ -287,7 +281,6 @@ config USB_S3C2410_DEBUG
config USB_S3C_HSUDC
tristate "S3C2416, S3C2443 and S3C2450 USB Device Controller"
depends on ARCH_S3C2410
- select USB_GADGET_DUALSPEED
help
Samsung's S3C2416, S3C2443 and S3C2450 is an ARM9 based SoC
integrated with dual speed USB 2.0 device controller. It has
@@ -298,7 +291,6 @@ config USB_S3C_HSUDC
config USB_PXA_U2O
tristate "PXA9xx Processor USB2.0 controller"
depends on ARCH_MMP
- select USB_GADGET_DUALSPEED
help
PXA9xx Processor series include a high speed USB2.0 device
controller, which support high speed and full speed USB peripheral.
@@ -311,14 +303,12 @@ config USB_PXA_U2O
config USB_GADGET_MUSB_HDRC
tristate "Inventra HDRC USB Peripheral (TI, ADI, ...)"
depends on USB_MUSB_HDRC
- select USB_GADGET_DUALSPEED
help
This OTG-capable silicon IP is used in dual designs including
the TI DaVinci, OMAP 243x, OMAP 343x, TUSB 6010, and ADI Blackfin
config USB_M66592
tristate "Renesas M66592 USB Peripheral Controller"
- select USB_GADGET_DUALSPEED
help
M66592 is a discrete USB peripheral controller chip that
supports both full and high speed USB 2.0 data transfers.
@@ -335,7 +325,6 @@ config USB_M66592
config USB_AMD5536UDC
tristate "AMD5536 UDC"
depends on PCI
- select USB_GADGET_DUALSPEED
help
The AMD5536 UDC is part of the AMD Geode CS5536, an x86 southbridge.
It is a USB Highspeed DMA capable USB device controller. Beside ep0
@@ -363,7 +352,6 @@ config USB_FSL_QE
config USB_CI13XXX_PCI
tristate "MIPS USB CI13xxx PCI UDC"
depends on PCI
- select USB_GADGET_DUALSPEED
help
MIPS USB IP core family device controller
Currently it only supports IP part number CI13412
@@ -374,7 +362,6 @@ config USB_CI13XXX_PCI
config USB_NET2272
tristate "PLX NET2272"
- select USB_GADGET_DUALSPEED
help
PLX NET2272 is a USB peripheral controller which supports
both full and high speed USB 2.0 data transfers.
@@ -398,7 +385,6 @@ config USB_NET2272_DMA
config USB_NET2280
tristate "NetChip 228x"
depends on PCI
- select USB_GADGET_DUALSPEED
help
NetChip 2280 / 2282 is a PCI based USB peripheral controller which
supports both full and high speed USB 2.0 data transfers.
@@ -429,7 +415,6 @@ config USB_LANGWELL
tristate "Intel Langwell USB Device Controller"
depends on PCI
depends on !PHYS_ADDR_T_64BIT
- select USB_GADGET_DUALSPEED
help
Intel Langwell USB Device Controller is a High-Speed USB
On-The-Go device controller.
@@ -444,7 +429,6 @@ config USB_LANGWELL
config USB_EG20T
tristate "Intel EG20T PCH/OKI SEMICONDUCTOR ML7213 IOH UDC"
depends on PCI
- select USB_GADGET_DUALSPEED
help
This is a USB device driver for EG20T PCH.
EG20T PCH is the platform controller hub that is used in Intel's
@@ -466,7 +450,6 @@ config USB_EG20T
config USB_CI13XXX_MSM
tristate "MIPS USB CI13xxx for MSM"
depends on ARCH_MSM
- select USB_GADGET_DUALSPEED
select USB_MSM_OTG
help
MSM SoC has chipidea USB controller. This driver uses
@@ -487,8 +470,6 @@ config USB_CI13XXX_MSM
config USB_DUMMY_HCD
tristate "Dummy HCD (DEVELOPMENT)"
depends on USB=y || (USB=m && USB_GADGET=m)
- select USB_GADGET_DUALSPEED
- select USB_GADGET_SUPERSPEED
help
This host controller driver emulates USB, looping all data transfer
requests back to a USB "gadget driver" in the same host. The host
@@ -513,17 +494,6 @@ config USB_DUMMY_HCD
endchoice
-# Selected by UDC drivers that support high-speed operation.
-config USB_GADGET_DUALSPEED
- bool
- depends on USB_GADGET
-
-# Selected by UDC drivers that support super-speed opperation
-config USB_GADGET_SUPERSPEED
- bool
- depends on USB_GADGET
- depends on USB_GADGET_DUALSPEED
-
#
# USB Gadget Drivers
#
diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index 5a3461e..c5249b3 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -1559,12 +1559,6 @@ composite_resume(struct usb_gadget *gadget)
/*-------------------------------------------------------------------------*/
static struct usb_gadget_driver composite_driver = {
-#ifdef CONFIG_USB_GADGET_SUPERSPEED
- .speed = USB_SPEED_SUPER,
-#else
- .speed = USB_SPEED_HIGH,
-#endif
-
.unbind = composite_unbind,
.setup = composite_setup,
@@ -1609,8 +1603,7 @@ int usb_composite_probe(struct usb_composite_driver *driver,
driver->iProduct = driver->name;
composite_driver.function = (char *) driver->name;
composite_driver.driver.name = driver->name;
- composite_driver.speed = min((u8)composite_driver.speed,
- (u8)driver->max_speed);
+ composite_driver.speed = driver->max_speed;
composite = driver;
composite_gadget_bind = bind;
diff --git a/drivers/usb/gadget/file_storage.c b/drivers/usb/gadget/file_storage.c
index cf875be..61ed4ec 100644
--- a/drivers/usb/gadget/file_storage.c
+++ b/drivers/usb/gadget/file_storage.c
@@ -3562,11 +3562,7 @@ static void fsg_resume(struct usb_gadget *gadget)
/*-------------------------------------------------------------------------*/
static struct usb_gadget_driver fsg_driver = {
-#ifdef CONFIG_USB_GADGET_DUALSPEED
.speed = USB_SPEED_HIGH,
-#else
- .speed = USB_SPEED_FULL,
-#endif
.function = (char *) fsg_string_product,
.unbind = fsg_unbind,
.disconnect = fsg_disconnect,
diff --git a/drivers/usb/gadget/inode.c b/drivers/usb/gadget/inode.c
index 1b24099..dddb3e4 100644
--- a/drivers/usb/gadget/inode.c
+++ b/drivers/usb/gadget/inode.c
@@ -837,7 +837,6 @@ ep_config (struct file *fd, const char __user *buf, size_t len, loff_t *ptr)
if (value == 0)
data->state = STATE_EP_ENABLED;
break;
-#ifdef CONFIG_USB_GADGET_DUALSPEED
case USB_SPEED_HIGH:
/* fails if caller didn't provide that descriptor... */
ep->desc = &data->hs_desc;
@@ -845,7 +844,6 @@ ep_config (struct file *fd, const char __user *buf, size_t len, loff_t *ptr)
if (value == 0)
data->state = STATE_EP_ENABLED;
break;
-#endif
default:
DBG(data->dev, "unconnected, %s init abandoned\n",
data->name);
@@ -1331,7 +1329,6 @@ static const struct file_operations ep0_io_operations = {
* Unrecognized ep0 requests may be handled in user space.
*/
-#ifdef CONFIG_USB_GADGET_DUALSPEED
static void make_qualifier (struct dev_data *dev)
{
struct usb_qualifier_descriptor qual;
@@ -1354,7 +1351,6 @@ static void make_qualifier (struct dev_data *dev)
memcpy (dev->rbuf, &qual, sizeof qual);
}
-#endif
static int
config_buf (struct dev_data *dev, u8 type, unsigned index)
@@ -1434,7 +1430,6 @@ gadgetfs_setup (struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
dev->dev->bMaxPacketSize0 = dev->gadget->ep0->maxpacket;
req->buf = dev->dev;
break;
-#ifdef CONFIG_USB_GADGET_DUALSPEED
case USB_DT_DEVICE_QUALIFIER:
if (!dev->hs_config)
break;
@@ -1444,7 +1439,6 @@ gadgetfs_setup (struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
break;
case USB_DT_OTHER_SPEED_CONFIG:
// FALLTHROUGH
-#endif
case USB_DT_CONFIG:
value = config_buf (dev,
w_value >> 8,
@@ -1773,11 +1767,7 @@ gadgetfs_suspend (struct usb_gadget *gadget)
}
static struct usb_gadget_driver gadgetfs_driver = {
-#ifdef CONFIG_USB_GADGET_DUALSPEED
.speed = USB_SPEED_HIGH,
-#else
- .speed = USB_SPEED_FULL,
-#endif
.function = (char *) driver_desc,
.unbind = gadgetfs_unbind,
.setup = gadgetfs_setup,
diff --git a/drivers/usb/gadget/printer.c b/drivers/usb/gadget/printer.c
index a341dde..901b7ba 100644
--- a/drivers/usb/gadget/printer.c
+++ b/drivers/usb/gadget/printer.c
@@ -164,12 +164,6 @@ module_param(qlen, uint, S_IRUGO|S_IWUSR);
#define QLEN qlen
-#ifdef CONFIG_USB_GADGET_DUALSPEED
-#define DEVSPEED USB_SPEED_HIGH
-#else /* full speed (low speed doesn't do bulk) */
-#define DEVSPEED USB_SPEED_FULL
-#endif
-
/*-------------------------------------------------------------------------*/
#define xprintk(d, level, fmt, args...) \
@@ -288,8 +282,6 @@ static const struct usb_descriptor_header *fs_printer_function [11] = {
NULL
};
-#ifdef CONFIG_USB_GADGET_DUALSPEED
-
/*
* usb 2.0 devices need to expose both high speed and full speed
* descriptors, unless they only run at full speed.
@@ -328,13 +320,6 @@ static const struct usb_descriptor_header *hs_printer_function [11] = {
/* maxpacket and other transfer characteristics vary by speed. */
#define ep_desc(g, hs, fs) (((g)->speed == USB_SPEED_HIGH)?(hs):(fs))
-#else
-
-/* if there's no high speed support, maxpacket doesn't change. */
-#define ep_desc(g, hs, fs) (((void)(g)), (fs))
-
-#endif /* !CONFIG_USB_GADGET_DUALSPEED */
-
/*-------------------------------------------------------------------------*/
/* descriptors that are built on-demand */
@@ -979,9 +964,7 @@ printer_set_config(struct printer_dev *dev, unsigned number)
switch (gadget->speed) {
case USB_SPEED_FULL: speed = "full"; break;
-#ifdef CONFIG_USB_GADGET_DUALSPEED
case USB_SPEED_HIGH: speed = "high"; break;
-#endif
default: speed = "?"; break;
}
@@ -998,23 +981,15 @@ config_buf(enum usb_device_speed speed, u8 *buf, u8 type, unsigned index,
{
int len;
const struct usb_descriptor_header **function;
-#ifdef CONFIG_USB_GADGET_DUALSPEED
int hs = (speed == USB_SPEED_HIGH);
+ if (index >= device_desc.bNumConfigurations)
+ return -EINVAL;
+
if (type == USB_DT_OTHER_SPEED_CONFIG)
hs = !hs;
- if (hs) {
- function = hs_printer_function;
- } else {
- function = fs_printer_function;
- }
-#else
- function = fs_printer_function;
-#endif
-
- if (index >= device_desc.bNumConfigurations)
- return -EINVAL;
+ function = hs ? hs_printer_function : fs_printer_function;
/* for now, don't advertise srp-only devices */
if (!is_otg)
@@ -1156,9 +1131,8 @@ printer_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
value = min(wLength, (u16) sizeof device_desc);
memcpy(req->buf, &device_desc, value);
break;
-#ifdef CONFIG_USB_GADGET_DUALSPEED
case USB_DT_DEVICE_QUALIFIER:
- if (!gadget->is_dualspeed)
+ if (!gadget_is_dualspeed(gadget))
break;
/*
* assumes ep0 uses the same value for both
@@ -1172,10 +1146,9 @@ printer_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
break;
case USB_DT_OTHER_SPEED_CONFIG:
- if (!gadget->is_dualspeed)
+ if (!gadget_is_dualspeed(gadget))
break;
/* FALLTHROUGH */
-#endif /* CONFIG_USB_GADGET_DUALSPEED */
case USB_DT_CONFIG:
value = config_buf(gadget->speed, req->buf,
wValue >> 8,
@@ -1460,11 +1433,9 @@ autoconf_fail:
goto autoconf_fail;
out_ep->driver_data = out_ep; /* claim */
-#ifdef CONFIG_USB_GADGET_DUALSPEED
/* assumes that all endpoints are dual-speed */
hs_ep_in_desc.bEndpointAddress = fs_ep_in_desc.bEndpointAddress;
hs_ep_out_desc.bEndpointAddress = fs_ep_out_desc.bEndpointAddress;
-#endif /* DUALSPEED */
usb_gadget_set_selfpowered(gadget);
@@ -1552,7 +1523,7 @@ fail:
/*-------------------------------------------------------------------------*/
static struct usb_gadget_driver printer_driver = {
- .speed = DEVSPEED,
+ .speed = USB_SPEED_HIGH,
.function = (char *) driver_desc,
.unbind = printer_unbind,
diff --git a/drivers/usb/gadget/u_ether.c b/drivers/usb/gadget/u_ether.c
index dfed4c1..8642abb 100644
--- a/drivers/usb/gadget/u_ether.c
+++ b/drivers/usb/gadget/u_ether.c
@@ -92,17 +92,10 @@ struct eth_dev {
#define DEFAULT_QLEN 2 /* double buffering by default */
-
-#ifdef CONFIG_USB_GADGET_DUALSPEED
-
static unsigned qmult = 5;
module_param(qmult, uint, S_IRUGO|S_IWUSR);
MODULE_PARM_DESC(qmult, "queue length multiplier at high/super speed");
-#else /* full speed (low speed doesn't do bulk) */
-#define qmult 1
-#endif
-
/* for dual-speed hardware, use deeper queues at high/super speed */
static inline int qlen(struct usb_gadget *gadget)
{
--
1.7.3.1
^ permalink raw reply related [flat|nested] 70+ messages in thread
* Re: [PATCH] usb: gadget: get rid of USB_GADGET_DUALSPEED and USB_GADGET_SUPERSPEED
2011-08-17 15:07 ` Michal Nazarewicz
@ 2011-08-17 16:25 ` Alan Stern
0 siblings, 0 replies; 70+ messages in thread
From: Alan Stern @ 2011-08-17 16:25 UTC (permalink / raw)
To: Michal Nazarewicz
Cc: Sebastian Andrzej Siewior, Yang Rui Rui, Felipe Balbi,
Greg Kroah-Hartman, linux-usb, linux-kernel
On Wed, 17 Aug 2011, Michal Nazarewicz wrote:
> >>>> --- a/drivers/usb/gadget/file_storage.c
> >>>> +++ b/drivers/usb/gadget/file_storage.c
> >>>> @@ -3562,11 +3562,7 @@ static void fsg_resume(struct usb_gadget
> >>>> *gadget)
> >>>> static struct usb_gadget_driver fsg_driver = {
> >>>> -#ifdef CONFIG_USB_GADGET_DUALSPEED
> >>>> .speed = USB_SPEED_HIGH,
> >>>> -#else
> >>>> - .speed = USB_SPEED_FULL,
> >>>> -#endif
> >>>> .function = (char *) fsg_string_product,
> >>>> .unbind = fsg_unbind,
> >>>> .disconnect = fsg_disconnect,
>
> >>> You mustn't remove the .speed field entirely. Set it to
> >>> USB_SPEED_HIGH.
>
> >> Please note that the field is not in fact removed.
>
> > No, but you removed its initializer and as a result, it will be set to
> > 0 whereas it should be set to USB_SPEED_HIGH.
>
> The initializer is still there. ;) Only the “#” lines and the initializer
> in the “#else” part are removed.
I must be going selectively blind. Never mind...
Alan Stern
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv2] usb: gadget: get rid of USB_GADGET_DUALSPEED and USB_GADGET_SUPERSPEED
2011-08-17 15:33 ` [PATCHv2] " Michal Nazarewicz
@ 2011-08-17 21:09 ` Alan Stern
2011-08-18 13:19 ` Michal Nazarewicz
2011-08-18 3:01 ` Yang Rui Rui
2011-08-19 22:32 ` [PATCHv3 0/4] Figuring out speed refactorisation Michal Nazarewicz
2 siblings, 1 reply; 70+ messages in thread
From: Alan Stern @ 2011-08-17 21:09 UTC (permalink / raw)
To: Michal Nazarewicz
Cc: Sergei Shtylyov, Felipe Balbi, Sebastian Andrzej Siewior,
Yang Rui Rui, Greg Kroah-Hartman, linux-usb, linux-kernel
On Wed, 17 Aug 2011, Michal Nazarewicz wrote:
> From: Michal Nazarewicz <mina86@mina86.com>
>
> This commit removes the use of USB_GADGET_DUALSPEED and
> USB_GADGET_SUPERSPEED Kconfig options. Those were selected
> by UDC drivers which supported respective speeds.
>
> However, since kernel now allows multiple UDC drivers to be
> compiled, the options in question may no longer reflect the
> state of all gadgets.
>
> For instance, if one driver that supports dual speed is selected
> and another that does not, the USB_GADGE_DUALSPEED will be set
> "for both".
>
> This commit replaces all the #ifdefs by a run-time checks made
> by calling gadget_is_dualspeed().
> --- a/drivers/usb/gadget/composite.c
> +++ b/drivers/usb/gadget/composite.c
> @@ -1559,12 +1559,6 @@ composite_resume(struct usb_gadget *gadget)
> /*-------------------------------------------------------------------------*/
>
> static struct usb_gadget_driver composite_driver = {
> -#ifdef CONFIG_USB_GADGET_SUPERSPEED
> - .speed = USB_SPEED_SUPER,
> -#else
> - .speed = USB_SPEED_HIGH,
> -#endif
> -
> .unbind = composite_unbind,
>
> .setup = composite_setup,
> @@ -1609,8 +1603,7 @@ int usb_composite_probe(struct usb_composite_driver *driver,
> driver->iProduct = driver->name;
> composite_driver.function = (char *) driver->name;
> composite_driver.driver.name = driver->name;
> - composite_driver.speed = min((u8)composite_driver.speed,
> - (u8)driver->max_speed);
> + composite_driver.speed = driver->max_speed;
> composite = driver;
> composite_gadget_bind = bind;
I see what's going on here. Your original patch was wrong and then my
correction was wrong as well.
This line has to remain the way it was (although those (u8) typecasts
don't seem to be necessary). Above, you have to initialize
composite_driver.speed to an appropriate value, probably
USB_SPEED_SUPER.
What you didn't realize in your original patch is that
usb_composite_probe() gets called more than once. Each time it is
called, it has to adjust composite_driver.speed.
Alan Stern
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv2] usb: gadget: get rid of USB_GADGET_DUALSPEED and USB_GADGET_SUPERSPEED
2011-08-17 15:33 ` [PATCHv2] " Michal Nazarewicz
2011-08-17 21:09 ` Alan Stern
@ 2011-08-18 3:01 ` Yang Rui Rui
2011-08-18 11:57 ` Michal Nazarewicz
2011-08-19 22:32 ` [PATCHv3 0/4] Figuring out speed refactorisation Michal Nazarewicz
2 siblings, 1 reply; 70+ messages in thread
From: Yang Rui Rui @ 2011-08-18 3:01 UTC (permalink / raw)
To: Michal Nazarewicz
Cc: Alan Stern, Sergei Shtylyov, Felipe Balbi,
Sebastian Andrzej Siewior, Yang Ruirui R, Greg Kroah-Hartman,
linux-usb, linux-kernel
On 08/17/2011 11:33 PM, Michal Nazarewicz wrote:
> From: Michal Nazarewicz<mina86@mina86.com>
>
> This commit removes the use of USB_GADGET_DUALSPEED and
> USB_GADGET_SUPERSPEED Kconfig options. Those were selected
> by UDC drivers which supported respective speeds.
>
> However, since kernel now allows multiple UDC drivers to be
> compiled, the options in question may no longer reflect the
> state of all gadgets.
>
> For instance, if one driver that supports dual speed is selected
> and another that does not, the USB_GADGE_DUALSPEED will be set
> "for both".
>
> This commit replaces all the #ifdefs by a run-time checks made
> by calling gadget_is_dualspeed().
I did not see the include/linux/usb/gadget.h changes, did you have another patch?
There's CONFIG_USB_GADGET_DUALSPEED ifdefs in gadget_is_dualspeed function of the gadget.h
If so, I will get oops as before.
> ---
> drivers/usb/gadget/Kconfig | 30 -------------------------
> drivers/usb/gadget/composite.c | 9 +-------
> drivers/usb/gadget/file_storage.c | 4 ---
> drivers/usb/gadget/inode.c | 10 --------
> drivers/usb/gadget/printer.c | 43 ++++++-------------------------------
> drivers/usb/gadget/u_ether.c | 7 ------
> 6 files changed, 8 insertions(+), 95 deletions(-)
>
> Like before, this has been only compile-tested (make allmodconfig
> bzImage modules on x86_64).
>
> Compared to v1:
> * Removed unrelated whitespace changes
> * Removed the use of min() in usb_composite_probe()
>
> diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
> index 5a084b9..c60c167 100644
> --- a/drivers/usb/gadget/Kconfig
> +++ b/drivers/usb/gadget/Kconfig
> @@ -133,7 +133,6 @@ config USB_AT91
>
> config USB_ATMEL_USBA
> tristate "Atmel USBA"
> - select USB_GADGET_DUALSPEED
> depends on AVR32 || ARCH_AT91CAP9 || ARCH_AT91SAM9RL || ARCH_AT91SAM9G45
> help
> USBA is the integrated high-speed USB Device controller on
> @@ -142,7 +141,6 @@ config USB_ATMEL_USBA
> config USB_FSL_USB2
> tristate "Freescale Highspeed USB DR Peripheral Controller"
> depends on FSL_SOC || ARCH_MXC
> - select USB_GADGET_DUALSPEED
> select USB_FSL_MPH_DR_OF if OF
> help
> Some of Freescale PowerPC processors have a High Speed
> @@ -158,7 +156,6 @@ config USB_FSL_USB2
> config USB_FUSB300
> tristate "Faraday FUSB300 USB Peripheral Controller"
> depends on !PHYS_ADDR_T_64BIT
> - select USB_GADGET_DUALSPEED
> help
> Faraday usb device controller FUSB300 driver
>
> @@ -206,7 +203,6 @@ config USB_PXA25X_SMALL
>
> config USB_R8A66597
> tristate "Renesas R8A66597 USB Peripheral Controller"
> - select USB_GADGET_DUALSPEED
> help
> R8A66597 is a discrete USB host and peripheral controller chip that
> supports both full and high speed USB 2.0 data transfers.
> @@ -220,7 +216,6 @@ config USB_RENESAS_USBHS_UDC
> tristate 'Renesas USBHS controller'
> depends on SUPERH || ARCH_SHMOBILE
> depends on USB_RENESAS_USBHS
> - select USB_GADGET_DUALSPEED
> help
> Renesas USBHS is a discrete USB host and peripheral controller chip
> that supports both full and high speed USB 2.0 data transfers.
> @@ -249,7 +244,6 @@ config USB_S3C_HSOTG
> tristate "S3C HS/OtG USB Device controller"
> depends on S3C_DEV_USB_HSOTG
> select USB_GADGET_S3C_HSOTG_PIO
> - select USB_GADGET_DUALSPEED
> help
> The Samsung S3C64XX USB2.0 high-speed gadget controller
> integrated into the S3C64XX series SoC.
> @@ -287,7 +281,6 @@ config USB_S3C2410_DEBUG
> config USB_S3C_HSUDC
> tristate "S3C2416, S3C2443 and S3C2450 USB Device Controller"
> depends on ARCH_S3C2410
> - select USB_GADGET_DUALSPEED
> help
> Samsung's S3C2416, S3C2443 and S3C2450 is an ARM9 based SoC
> integrated with dual speed USB 2.0 device controller. It has
> @@ -298,7 +291,6 @@ config USB_S3C_HSUDC
> config USB_PXA_U2O
> tristate "PXA9xx Processor USB2.0 controller"
> depends on ARCH_MMP
> - select USB_GADGET_DUALSPEED
> help
> PXA9xx Processor series include a high speed USB2.0 device
> controller, which support high speed and full speed USB peripheral.
> @@ -311,14 +303,12 @@ config USB_PXA_U2O
> config USB_GADGET_MUSB_HDRC
> tristate "Inventra HDRC USB Peripheral (TI, ADI, ...)"
> depends on USB_MUSB_HDRC
> - select USB_GADGET_DUALSPEED
> help
> This OTG-capable silicon IP is used in dual designs including
> the TI DaVinci, OMAP 243x, OMAP 343x, TUSB 6010, and ADI Blackfin
>
> config USB_M66592
> tristate "Renesas M66592 USB Peripheral Controller"
> - select USB_GADGET_DUALSPEED
> help
> M66592 is a discrete USB peripheral controller chip that
> supports both full and high speed USB 2.0 data transfers.
> @@ -335,7 +325,6 @@ config USB_M66592
> config USB_AMD5536UDC
> tristate "AMD5536 UDC"
> depends on PCI
> - select USB_GADGET_DUALSPEED
> help
> The AMD5536 UDC is part of the AMD Geode CS5536, an x86 southbridge.
> It is a USB Highspeed DMA capable USB device controller. Beside ep0
> @@ -363,7 +352,6 @@ config USB_FSL_QE
> config USB_CI13XXX_PCI
> tristate "MIPS USB CI13xxx PCI UDC"
> depends on PCI
> - select USB_GADGET_DUALSPEED
> help
> MIPS USB IP core family device controller
> Currently it only supports IP part number CI13412
> @@ -374,7 +362,6 @@ config USB_CI13XXX_PCI
>
> config USB_NET2272
> tristate "PLX NET2272"
> - select USB_GADGET_DUALSPEED
> help
> PLX NET2272 is a USB peripheral controller which supports
> both full and high speed USB 2.0 data transfers.
> @@ -398,7 +385,6 @@ config USB_NET2272_DMA
> config USB_NET2280
> tristate "NetChip 228x"
> depends on PCI
> - select USB_GADGET_DUALSPEED
> help
> NetChip 2280 / 2282 is a PCI based USB peripheral controller which
> supports both full and high speed USB 2.0 data transfers.
> @@ -429,7 +415,6 @@ config USB_LANGWELL
> tristate "Intel Langwell USB Device Controller"
> depends on PCI
> depends on !PHYS_ADDR_T_64BIT
> - select USB_GADGET_DUALSPEED
> help
> Intel Langwell USB Device Controller is a High-Speed USB
> On-The-Go device controller.
> @@ -444,7 +429,6 @@ config USB_LANGWELL
> config USB_EG20T
> tristate "Intel EG20T PCH/OKI SEMICONDUCTOR ML7213 IOH UDC"
> depends on PCI
> - select USB_GADGET_DUALSPEED
> help
> This is a USB device driver for EG20T PCH.
> EG20T PCH is the platform controller hub that is used in Intel's
> @@ -466,7 +450,6 @@ config USB_EG20T
> config USB_CI13XXX_MSM
> tristate "MIPS USB CI13xxx for MSM"
> depends on ARCH_MSM
> - select USB_GADGET_DUALSPEED
> select USB_MSM_OTG
> help
> MSM SoC has chipidea USB controller. This driver uses
> @@ -487,8 +470,6 @@ config USB_CI13XXX_MSM
> config USB_DUMMY_HCD
> tristate "Dummy HCD (DEVELOPMENT)"
> depends on USB=y || (USB=m&& USB_GADGET=m)
> - select USB_GADGET_DUALSPEED
> - select USB_GADGET_SUPERSPEED
> help
> This host controller driver emulates USB, looping all data transfer
> requests back to a USB "gadget driver" in the same host. The host
> @@ -513,17 +494,6 @@ config USB_DUMMY_HCD
>
> endchoice
>
> -# Selected by UDC drivers that support high-speed operation.
> -config USB_GADGET_DUALSPEED
> - bool
> - depends on USB_GADGET
> -
> -# Selected by UDC drivers that support super-speed opperation
> -config USB_GADGET_SUPERSPEED
> - bool
> - depends on USB_GADGET
> - depends on USB_GADGET_DUALSPEED
> -
> #
> # USB Gadget Drivers
> #
> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
> index 5a3461e..c5249b3 100644
> --- a/drivers/usb/gadget/composite.c
> +++ b/drivers/usb/gadget/composite.c
> @@ -1559,12 +1559,6 @@ composite_resume(struct usb_gadget *gadget)
> /*-------------------------------------------------------------------------*/
>
> static struct usb_gadget_driver composite_driver = {
> -#ifdef CONFIG_USB_GADGET_SUPERSPEED
> - .speed = USB_SPEED_SUPER,
> -#else
> - .speed = USB_SPEED_HIGH,
> -#endif
> -
> .unbind = composite_unbind,
>
> .setup = composite_setup,
> @@ -1609,8 +1603,7 @@ int usb_composite_probe(struct usb_composite_driver *driver,
> driver->iProduct = driver->name;
> composite_driver.function = (char *) driver->name;
> composite_driver.driver.name = driver->name;
> - composite_driver.speed = min((u8)composite_driver.speed,
> - (u8)driver->max_speed);
> + composite_driver.speed = driver->max_speed;
This does not works, many drivers will check the driver->speed, ie in musg_gadget.c
static int musb_gadget_start(struct usb_gadget *g,
struct usb_gadget_driver *driver)
{
struct musb *musb = gadget_to_musb(g);
unsigned long flags;
int retval = -EINVAL;
if (driver->speed != USB_SPEED_HIGH)
goto err0;
[snip]
> composite = driver;
> composite_gadget_bind = bind;
>
> diff --git a/drivers/usb/gadget/file_storage.c b/drivers/usb/gadget/file_storage.c
> index cf875be..61ed4ec 100644
> --- a/drivers/usb/gadget/file_storage.c
> +++ b/drivers/usb/gadget/file_storage.c
> @@ -3562,11 +3562,7 @@ static void fsg_resume(struct usb_gadget *gadget)
> /*-------------------------------------------------------------------------*/
>
> static struct usb_gadget_driver fsg_driver = {
> -#ifdef CONFIG_USB_GADGET_DUALSPEED
> .speed = USB_SPEED_HIGH,
> -#else
> - .speed = USB_SPEED_FULL,
> -#endif
> .function = (char *) fsg_string_product,
> .unbind = fsg_unbind,
> .disconnect = fsg_disconnect,
> diff --git a/drivers/usb/gadget/inode.c b/drivers/usb/gadget/inode.c
> index 1b24099..dddb3e4 100644
> --- a/drivers/usb/gadget/inode.c
> +++ b/drivers/usb/gadget/inode.c
> @@ -837,7 +837,6 @@ ep_config (struct file *fd, const char __user *buf, size_t len, loff_t *ptr)
> if (value == 0)
> data->state = STATE_EP_ENABLED;
> break;
> -#ifdef CONFIG_USB_GADGET_DUALSPEED
> case USB_SPEED_HIGH:
> /* fails if caller didn't provide that descriptor... */
> ep->desc =&data->hs_desc;
> @@ -845,7 +844,6 @@ ep_config (struct file *fd, const char __user *buf, size_t len, loff_t *ptr)
> if (value == 0)
> data->state = STATE_EP_ENABLED;
> break;
> -#endif
> default:
> DBG(data->dev, "unconnected, %s init abandoned\n",
> data->name);
> @@ -1331,7 +1329,6 @@ static const struct file_operations ep0_io_operations = {
> * Unrecognized ep0 requests may be handled in user space.
> */
>
> -#ifdef CONFIG_USB_GADGET_DUALSPEED
> static void make_qualifier (struct dev_data *dev)
> {
> struct usb_qualifier_descriptor qual;
> @@ -1354,7 +1351,6 @@ static void make_qualifier (struct dev_data *dev)
>
> memcpy (dev->rbuf,&qual, sizeof qual);
> }
> -#endif
>
> static int
> config_buf (struct dev_data *dev, u8 type, unsigned index)
> @@ -1434,7 +1430,6 @@ gadgetfs_setup (struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
> dev->dev->bMaxPacketSize0 = dev->gadget->ep0->maxpacket;
> req->buf = dev->dev;
> break;
> -#ifdef CONFIG_USB_GADGET_DUALSPEED
> case USB_DT_DEVICE_QUALIFIER:
> if (!dev->hs_config)
> break;
> @@ -1444,7 +1439,6 @@ gadgetfs_setup (struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
> break;
> case USB_DT_OTHER_SPEED_CONFIG:
> // FALLTHROUGH
> -#endif
> case USB_DT_CONFIG:
> value = config_buf (dev,
> w_value>> 8,
> @@ -1773,11 +1767,7 @@ gadgetfs_suspend (struct usb_gadget *gadget)
> }
>
> static struct usb_gadget_driver gadgetfs_driver = {
> -#ifdef CONFIG_USB_GADGET_DUALSPEED
> .speed = USB_SPEED_HIGH,
> -#else
> - .speed = USB_SPEED_FULL,
> -#endif
> .function = (char *) driver_desc,
> .unbind = gadgetfs_unbind,
> .setup = gadgetfs_setup,
> diff --git a/drivers/usb/gadget/printer.c b/drivers/usb/gadget/printer.c
> index a341dde..901b7ba 100644
> --- a/drivers/usb/gadget/printer.c
> +++ b/drivers/usb/gadget/printer.c
> @@ -164,12 +164,6 @@ module_param(qlen, uint, S_IRUGO|S_IWUSR);
>
> #define QLEN qlen
>
> -#ifdef CONFIG_USB_GADGET_DUALSPEED
> -#define DEVSPEED USB_SPEED_HIGH
> -#else /* full speed (low speed doesn't do bulk) */
> -#define DEVSPEED USB_SPEED_FULL
> -#endif
> -
> /*-------------------------------------------------------------------------*/
>
> #define xprintk(d, level, fmt, args...) \
> @@ -288,8 +282,6 @@ static const struct usb_descriptor_header *fs_printer_function [11] = {
> NULL
> };
>
> -#ifdef CONFIG_USB_GADGET_DUALSPEED
> -
> /*
> * usb 2.0 devices need to expose both high speed and full speed
> * descriptors, unless they only run at full speed.
> @@ -328,13 +320,6 @@ static const struct usb_descriptor_header *hs_printer_function [11] = {
> /* maxpacket and other transfer characteristics vary by speed. */
> #define ep_desc(g, hs, fs) (((g)->speed == USB_SPEED_HIGH)?(hs):(fs))
>
> -#else
> -
> -/* if there's no high speed support, maxpacket doesn't change. */
> -#define ep_desc(g, hs, fs) (((void)(g)), (fs))
> -
> -#endif /* !CONFIG_USB_GADGET_DUALSPEED */
> -
> /*-------------------------------------------------------------------------*/
>
> /* descriptors that are built on-demand */
> @@ -979,9 +964,7 @@ printer_set_config(struct printer_dev *dev, unsigned number)
>
> switch (gadget->speed) {
> case USB_SPEED_FULL: speed = "full"; break;
> -#ifdef CONFIG_USB_GADGET_DUALSPEED
> case USB_SPEED_HIGH: speed = "high"; break;
> -#endif
> default: speed = "?"; break;
> }
>
> @@ -998,23 +981,15 @@ config_buf(enum usb_device_speed speed, u8 *buf, u8 type, unsigned index,
> {
> int len;
> const struct usb_descriptor_header **function;
> -#ifdef CONFIG_USB_GADGET_DUALSPEED
> int hs = (speed == USB_SPEED_HIGH);
>
> + if (index>= device_desc.bNumConfigurations)
> + return -EINVAL;
> +
> if (type == USB_DT_OTHER_SPEED_CONFIG)
> hs = !hs;
>
> - if (hs) {
> - function = hs_printer_function;
> - } else {
> - function = fs_printer_function;
> - }
> -#else
> - function = fs_printer_function;
> -#endif
> -
> - if (index>= device_desc.bNumConfigurations)
> - return -EINVAL;
> + function = hs ? hs_printer_function : fs_printer_function;
>
> /* for now, don't advertise srp-only devices */
> if (!is_otg)
> @@ -1156,9 +1131,8 @@ printer_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
> value = min(wLength, (u16) sizeof device_desc);
> memcpy(req->buf,&device_desc, value);
> break;
> -#ifdef CONFIG_USB_GADGET_DUALSPEED
> case USB_DT_DEVICE_QUALIFIER:
> - if (!gadget->is_dualspeed)
> + if (!gadget_is_dualspeed(gadget))
> break;
> /*
> * assumes ep0 uses the same value for both
> @@ -1172,10 +1146,9 @@ printer_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
> break;
>
> case USB_DT_OTHER_SPEED_CONFIG:
> - if (!gadget->is_dualspeed)
> + if (!gadget_is_dualspeed(gadget))
> break;
> /* FALLTHROUGH */
> -#endif /* CONFIG_USB_GADGET_DUALSPEED */
> case USB_DT_CONFIG:
> value = config_buf(gadget->speed, req->buf,
> wValue>> 8,
> @@ -1460,11 +1433,9 @@ autoconf_fail:
> goto autoconf_fail;
> out_ep->driver_data = out_ep; /* claim */
>
> -#ifdef CONFIG_USB_GADGET_DUALSPEED
> /* assumes that all endpoints are dual-speed */
> hs_ep_in_desc.bEndpointAddress = fs_ep_in_desc.bEndpointAddress;
> hs_ep_out_desc.bEndpointAddress = fs_ep_out_desc.bEndpointAddress;
> -#endif /* DUALSPEED */
>
> usb_gadget_set_selfpowered(gadget);
>
> @@ -1552,7 +1523,7 @@ fail:
> /*-------------------------------------------------------------------------*/
>
> static struct usb_gadget_driver printer_driver = {
> - .speed = DEVSPEED,
> + .speed = USB_SPEED_HIGH,
>
> .function = (char *) driver_desc,
> .unbind = printer_unbind,
> diff --git a/drivers/usb/gadget/u_ether.c b/drivers/usb/gadget/u_ether.c
> index dfed4c1..8642abb 100644
> --- a/drivers/usb/gadget/u_ether.c
> +++ b/drivers/usb/gadget/u_ether.c
> @@ -92,17 +92,10 @@ struct eth_dev {
>
> #define DEFAULT_QLEN 2 /* double buffering by default */
>
> -
> -#ifdef CONFIG_USB_GADGET_DUALSPEED
> -
> static unsigned qmult = 5;
> module_param(qmult, uint, S_IRUGO|S_IWUSR);
> MODULE_PARM_DESC(qmult, "queue length multiplier at high/super speed");
>
> -#else /* full speed (low speed doesn't do bulk) */
> -#define qmult 1
> -#endif
> -
> /* for dual-speed hardware, use deeper queues at high/super speed */
> static inline int qlen(struct usb_gadget *gadget)
> {
> --
> 1.7.3.1
>
--
Thanks
Yang Ruirui
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv2] usb: gadget: get rid of USB_GADGET_DUALSPEED and USB_GADGET_SUPERSPEED
2011-08-18 3:01 ` Yang Rui Rui
@ 2011-08-18 11:57 ` Michal Nazarewicz
2011-08-18 13:24 ` Dave Young
0 siblings, 1 reply; 70+ messages in thread
From: Michal Nazarewicz @ 2011-08-18 11:57 UTC (permalink / raw)
To: Yang Rui Rui
Cc: Alan Stern, Sergei Shtylyov, Felipe Balbi,
Sebastian Andrzej Siewior, Yang Ruirui R, Greg Kroah-Hartman,
linux-usb, linux-kernel
On Thu, 18 Aug 2011 05:01:11 +0200, Yang Rui Rui <ruirui.r.yang@tieto.com>
wrote:
> I did not see the include/linux/usb/gadget.h changes, did you have
> another patch?
> There's CONFIG_USB_GADGET_DUALSPEED ifdefs in gadget_is_dualspeed
> function of the gadget.h
> If so, I will get oops as before.
Thanks, fixed. I did “git add -u” while I was in drivers/usb/gadget so
gadget.h did not get added and I've overlooked it. Sorry about that.
> On 08/17/2011 11:33 PM, Michal Nazarewicz wrote:
>> --- a/drivers/usb/gadget/composite.c
>> +++ b/drivers/usb/gadget/composite.c
>> @@ -1559,12 +1559,6 @@ composite_resume(struct usb_gadget *gadget)
>> /*-------------------------------------------------------------------------*/
>>
>> static struct usb_gadget_driver composite_driver = {
>> -#ifdef CONFIG_USB_GADGET_SUPERSPEED
>> - .speed = USB_SPEED_SUPER,
>> -#else
>> - .speed = USB_SPEED_HIGH,
>> -#endif
>> -
>> .unbind = composite_unbind,
>>
>> .setup = composite_setup,
>> @@ -1609,8 +1603,7 @@ int usb_composite_probe(struct
>> usb_composite_driver *driver,
>> driver->iProduct = driver->name;
>> composite_driver.function = (char *) driver->name;
>> composite_driver.driver.name = driver->name;
>> - composite_driver.speed = min((u8)composite_driver.speed,
>> - (u8)driver->max_speed);
>> + composite_driver.speed = driver->max_speed;
> This does not works, many drivers will check the driver->speed, ie in
> musg_gadget.c
Uh? Not sure what you mean here. Note that driver above is not the same
structure as driver below. composite_driver is what musb_gadget_start()
will get.
> static int musb_gadget_start(struct usb_gadget *g,
> struct usb_gadget_driver *driver)
> {
> struct musb *musb = gadget_to_musb(g);
> unsigned long flags;
> int retval = -EINVAL;
>
> if (driver->speed != USB_SPEED_HIGH)
> goto err0;
> [snip]
>
>> composite = driver;
>> composite_gadget_bind = bind;
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michal "mina86" Nazarewicz (o o)
ooo +-----<email/xmpp: mnazarewicz@google.com>-----ooO--(_)--Ooo--
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv2] usb: gadget: get rid of USB_GADGET_DUALSPEED and USB_GADGET_SUPERSPEED
2011-08-17 21:09 ` Alan Stern
@ 2011-08-18 13:19 ` Michal Nazarewicz
2011-08-18 14:59 ` Alan Stern
0 siblings, 1 reply; 70+ messages in thread
From: Michal Nazarewicz @ 2011-08-18 13:19 UTC (permalink / raw)
To: Alan Stern
Cc: Sergei Shtylyov, Felipe Balbi, Sebastian Andrzej Siewior,
Yang Rui Rui, Greg Kroah-Hartman, linux-usb, linux-kernel
> On Wed, 17 Aug 2011, Michal Nazarewicz wrote:
>> --- a/drivers/usb/gadget/composite.c
>> +++ b/drivers/usb/gadget/composite.c
>> @@ -1559,12 +1559,6 @@ composite_resume(struct usb_gadget *gadget)
>> /*-------------------------------------------------------------------------*/
>>
>> static struct usb_gadget_driver composite_driver = {
>> -#ifdef CONFIG_USB_GADGET_SUPERSPEED
>> - .speed = USB_SPEED_SUPER,
>> -#else
>> - .speed = USB_SPEED_HIGH,
>> -#endif
>> -
>> .unbind = composite_unbind,
>>
>> .setup = composite_setup,
>> @@ -1609,8 +1603,7 @@ int usb_composite_probe(struct
>> usb_composite_driver *driver,
>> driver->iProduct = driver->name;
>> composite_driver.function = (char *) driver->name;
>> composite_driver.driver.name = driver->name;
>> - composite_driver.speed = min((u8)composite_driver.speed,
>> - (u8)driver->max_speed);
>> + composite_driver.speed = driver->max_speed;
>> composite = driver;
>> composite_gadget_bind = bind;
On Wed, 17 Aug 2011 23:09:37 +0200, Alan Stern <stern@rowland.harvard.edu>
wrote:
> I see what's going on here. Your original patch was wrong and then my
> correction was wrong as well.
>
> This line has to remain the way it was (although those (u8) typecasts
> don't seem to be necessary). Above, you have to initialize
> composite_driver.speed to an appropriate value, probably
> USB_SPEED_SUPER.
>
> What you didn't realize in your original patch is that
> usb_composite_probe() gets called more than once. Each time it is
> called, it has to adjust composite_driver.speed.
That's sneaky of composite.c... But is it desired behaviour? I cannot
came up with a situation where that's what we want. I would imagine that
usb_composite_probe() should assume the same speed for given
usb_composite_driver regardless if some other slower usb_composite_driver
was loaded.
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michal "mina86" Nazarewicz (o o)
ooo +-----<email/xmpp: mnazarewicz@google.com>-----ooO--(_)--Ooo--
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv2] usb: gadget: get rid of USB_GADGET_DUALSPEED and USB_GADGET_SUPERSPEED
2011-08-18 11:57 ` Michal Nazarewicz
@ 2011-08-18 13:24 ` Dave Young
2011-08-18 13:41 ` Michal Nazarewicz
0 siblings, 1 reply; 70+ messages in thread
From: Dave Young @ 2011-08-18 13:24 UTC (permalink / raw)
To: Michal Nazarewicz
Cc: Yang Rui Rui, Alan Stern, Sergei Shtylyov, Felipe Balbi,
Sebastian Andrzej Siewior, Yang Ruirui R, Greg Kroah-Hartman,
linux-usb, linux-kernel
On Thu, Aug 18, 2011 at 7:57 PM, Michal Nazarewicz <mina86@mina86.com> wrote:
> On Thu, 18 Aug 2011 05:01:11 +0200, Yang Rui Rui <ruirui.r.yang@tieto.com>
> wrote:
>>
>> I did not see the include/linux/usb/gadget.h changes, did you have another
>> patch?
>> There's CONFIG_USB_GADGET_DUALSPEED ifdefs in gadget_is_dualspeed
>> function of the gadget.h
>> If so, I will get oops as before.
>
> Thanks, fixed. I did “git add -u” while I was in drivers/usb/gadget so
> gadget.h did not get added and I've overlooked it. Sorry about that.
>
>> On 08/17/2011 11:33 PM, Michal Nazarewicz wrote:
>>>
>>> --- a/drivers/usb/gadget/composite.c
>>> +++ b/drivers/usb/gadget/composite.c
>>> @@ -1559,12 +1559,6 @@ composite_resume(struct usb_gadget *gadget)
>>>
>>> /*-------------------------------------------------------------------------*/
>>>
>>> static struct usb_gadget_driver composite_driver = {
>>> -#ifdef CONFIG_USB_GADGET_SUPERSPEED
>>> - .speed = USB_SPEED_SUPER,
>>> -#else
>>> - .speed = USB_SPEED_HIGH,
>>> -#endif
>>> -
>>> .unbind = composite_unbind,
>>>
>>> .setup = composite_setup,
>>> @@ -1609,8 +1603,7 @@ int usb_composite_probe(struct usb_composite_driver
>>> *driver,
>>> driver->iProduct = driver->name;
>>> composite_driver.function = (char *) driver->name;
>>> composite_driver.driver.name = driver->name;
>>> - composite_driver.speed = min((u8)composite_driver.speed,
>>> - (u8)driver->max_speed);
>>> + composite_driver.speed = driver->max_speed;
>
>> This does not works, many drivers will check the driver->speed, ie in
>> musg_gadget.c
>
> Uh? Not sure what you mean here. Note that driver above is not the same
> structure as driver below. composite_driver is what musb_gadget_start()
> will get.
For example I got -EINVAL in musb_gadget_start in case
composite_driver.speed = USB_SPEED_SUPER
the driver param is the composite_driver in composite.c, isn't it?
>
>> static int musb_gadget_start(struct usb_gadget *g,
>> struct usb_gadget_driver *driver)
>> {
>> struct musb *musb = gadget_to_musb(g);
>> unsigned long flags;
>> int retval = -EINVAL;
>>
>> if (driver->speed != USB_SPEED_HIGH)
>> goto err0;
>> [snip]
>>
>>> composite = driver;
>>> composite_gadget_bind = bind;
>
> --
> Best regards, _ _
> .o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
> ..o | Computer Science, Michal "mina86" Nazarewicz (o o)
> ooo +-----<email/xmpp: mnazarewicz@google.com>-----ooO--(_)--Ooo--
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
--
Regards
Yang RuiRui
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv2] usb: gadget: get rid of USB_GADGET_DUALSPEED and USB_GADGET_SUPERSPEED
2011-08-18 13:24 ` Dave Young
@ 2011-08-18 13:41 ` Michal Nazarewicz
0 siblings, 0 replies; 70+ messages in thread
From: Michal Nazarewicz @ 2011-08-18 13:41 UTC (permalink / raw)
To: Dave Young
Cc: Yang Rui Rui, Alan Stern, Sergei Shtylyov, Felipe Balbi,
Sebastian Andrzej Siewior, Yang Ruirui R, Greg Kroah-Hartman,
linux-usb, linux-kernel
>>> On 08/17/2011 11:33 PM, Michal Nazarewicz wrote:
>>>> --- a/drivers/usb/gadget/composite.c
>>>> +++ b/drivers/usb/gadget/composite.c
>>>> @@ -1559,12 +1559,6 @@ composite_resume(struct usb_gadget *gadget)
>>>>
>>>> /*-------------------------------------------------------------------------*/
>>>>
>>>> static struct usb_gadget_driver composite_driver = {
>>>> -#ifdef CONFIG_USB_GADGET_SUPERSPEED
>>>> - .speed = USB_SPEED_SUPER,
>>>> -#else
>>>> - .speed = USB_SPEED_HIGH,
>>>> -#endif
>>>> -
>>>> .unbind = composite_unbind,
>>>>
>>>> .setup = composite_setup,
>>>> @@ -1609,8 +1603,7 @@ int usb_composite_probe(struct
>>>> usb_composite_driver
>>>> *driver,
>>>> driver->iProduct = driver->name;
>>>> composite_driver.function = (char *) driver->name;
>>>> composite_driver.driver.name = driver->name;
>>>> - composite_driver.speed = min((u8)composite_driver.speed,
>>>> - (u8)driver->max_speed);
>>>> + composite_driver.speed = driver->max_speed;
>> On Thu, 18 Aug 2011 05:01:11 +0200, Yang Rui Rui wrote:
>>> This does not works, many drivers will check the driver->speed, ie in
>>> musg_gadget.c
> On Thu, Aug 18, 2011 at 7:57 PM, Michal Nazarewicz wrote:
>> Uh? Not sure what you mean here. Note that driver above is not the
>> same
>> structure as driver below. composite_driver is what musb_gadget_start()
>> will get.
On Thu, 18 Aug 2011 15:24:07 +0200, Dave Young wrote:
> For example I got -EINVAL in musb_gadget_start in case
> composite_driver.speed = USB_SPEED_SUPER
For anything other then g_ether and g_zero, the speed should be set to
USB_SPEED_HIGH. Unless this is somehow related to what Alan said about
this code.
> the driver param is the composite_driver in composite.c, isn't it?
Yes.
>>> static int musb_gadget_start(struct usb_gadget *g,
>>> struct usb_gadget_driver *driver)
>>> {
>>> struct musb *musb = gadget_to_musb(g);
>>> unsigned long flags;
>>> int retval = -EINVAL;
>>>
>>> if (driver->speed != USB_SPEED_HIGH)
>>> goto err0;
>>> [snip]
>>>
>>>> composite = driver;
>>>> composite_gadget_bind = bind;
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michal "mina86" Nazarewicz (o o)
ooo +-----<email/xmpp: mnazarewicz@google.com>-----ooO--(_)--Ooo--
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv2] usb: gadget: get rid of USB_GADGET_DUALSPEED and USB_GADGET_SUPERSPEED
2011-08-18 13:19 ` Michal Nazarewicz
@ 2011-08-18 14:59 ` Alan Stern
2011-08-18 17:05 ` Michal Nazarewicz
0 siblings, 1 reply; 70+ messages in thread
From: Alan Stern @ 2011-08-18 14:59 UTC (permalink / raw)
To: Michal Nazarewicz
Cc: Sergei Shtylyov, Felipe Balbi, Sebastian Andrzej Siewior,
Yang Rui Rui, Greg Kroah-Hartman, linux-usb, linux-kernel
On Thu, 18 Aug 2011, Michal Nazarewicz wrote:
> On Wed, 17 Aug 2011 23:09:37 +0200, Alan Stern <stern@rowland.harvard.edu>
> wrote:
> > I see what's going on here. Your original patch was wrong and then my
> > correction was wrong as well.
> >
> > This line has to remain the way it was (although those (u8) typecasts
> > don't seem to be necessary). Above, you have to initialize
> > composite_driver.speed to an appropriate value, probably
> > USB_SPEED_SUPER.
> >
> > What you didn't realize in your original patch is that
> > usb_composite_probe() gets called more than once. Each time it is
> > called, it has to adjust composite_driver.speed.
>
> That's sneaky of composite.c... But is it desired behaviour?
Yes, it is.
> I cannot
> came up with a situation where that's what we want. I would imagine that
> usb_composite_probe() should assume the same speed for given
> usb_composite_driver regardless if some other slower usb_composite_driver
> was loaded.
The speed being calculated isn't the speed of the usb_composite_driver;
it's the speed of the usb_gadget_driver. The gadget itself cannot be
allowed to run faster than its internal drivers can handle.
For example, if you have a composite gadget where one of the function
drivers can handle SuperSpeed and the other can't go beyond high speed,
the overall gadget must never run faster than high speed.
Alan Stern
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv2] usb: gadget: get rid of USB_GADGET_DUALSPEED and USB_GADGET_SUPERSPEED
2011-08-18 14:59 ` Alan Stern
@ 2011-08-18 17:05 ` Michal Nazarewicz
2011-08-18 17:27 ` Alan Stern
2011-08-19 2:02 ` Yang Rui Rui
0 siblings, 2 replies; 70+ messages in thread
From: Michal Nazarewicz @ 2011-08-18 17:05 UTC (permalink / raw)
To: Alan Stern
Cc: Sergei Shtylyov, Felipe Balbi, Sebastian Andrzej Siewior,
Yang Rui Rui, Greg Kroah-Hartman, linux-usb, linux-kernel
On Thu, 18 Aug 2011 16:59:28 +0200, Alan Stern <stern@rowland.harvard.edu>
wrote:
> On Thu, 18 Aug 2011, Michal Nazarewicz wrote:
>
>> On Wed, 17 Aug 2011 23:09:37 +0200, Alan Stern
>> <stern@rowland.harvard.edu>
>> wrote:
>> > I see what's going on here. Your original patch was wrong and then my
>> > correction was wrong as well.
>> >
>> > This line has to remain the way it was (although those (u8) typecasts
>> > don't seem to be necessary). Above, you have to initialize
>> > composite_driver.speed to an appropriate value, probably
>> > USB_SPEED_SUPER.
>> >
>> > What you didn't realize in your original patch is that
>> > usb_composite_probe() gets called more than once. Each time it is
>> > called, it has to adjust composite_driver.speed.
>>
>> That's sneaky of composite.c... But is it desired behaviour?
>
> Yes, it is.
>
>> I cannot
>> came up with a situation where that's what we want. I would imagine
>> that
>> usb_composite_probe() should assume the same speed for given
>> usb_composite_driver regardless if some other slower
>> usb_composite_driver
>> was loaded.
>
> The speed being calculated isn't the speed of the usb_composite_driver;
> it's the speed of the usb_gadget_driver. The gadget itself cannot be
> allowed to run faster than its internal drivers can handle.
Yeah, that's why you set usb_gadget_driver's speed to the speed declared
in usb_composite_driver.
For the most part, usb_composite_probe() is called only once in module's
init function. As far as I know, only g_ffs calls it several times. So
in all cases expect for g_ffs, composite_driver.speed =
min(composite_driver.speed,
driver->max_speed) should have the same effect as composite_driver.speed
= driver->max_speed.
> For example, if you have a composite gadget where one of the function
> drivers can handle SuperSpeed and the other can't go beyond high speed,
> the overall gadget must never run faster than high speed.
Shouldn't that be dealt in usb_add_function()? I cannot see any code that
would do that here atm though.
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michal "mina86" Nazarewicz (o o)
ooo +-----<email/xmpp: mnazarewicz@google.com>-----ooO--(_)--Ooo--
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv2] usb: gadget: get rid of USB_GADGET_DUALSPEED and USB_GADGET_SUPERSPEED
2011-08-18 17:05 ` Michal Nazarewicz
@ 2011-08-18 17:27 ` Alan Stern
2011-08-18 20:13 ` Michal Nazarewicz
2011-08-19 2:02 ` Yang Rui Rui
1 sibling, 1 reply; 70+ messages in thread
From: Alan Stern @ 2011-08-18 17:27 UTC (permalink / raw)
To: Michal Nazarewicz
Cc: Sergei Shtylyov, Felipe Balbi, Sebastian Andrzej Siewior,
Yang Rui Rui, Greg Kroah-Hartman, linux-usb, linux-kernel
On Thu, 18 Aug 2011, Michal Nazarewicz wrote:
> For the most part, usb_composite_probe() is called only once in module's
> init function. As far as I know, only g_ffs calls it several times. So
> in all cases expect for g_ffs, composite_driver.speed =
> min(composite_driver.speed,
> driver->max_speed) should have the same effect as composite_driver.speed
> = driver->max_speed.
>
> > For example, if you have a composite gadget where one of the function
> > drivers can handle SuperSpeed and the other can't go beyond high speed,
> > the overall gadget must never run faster than high speed.
>
> Shouldn't that be dealt in usb_add_function()? I cannot see any code that
> would do that here atm though.
Maybe you're right. But isn't that too late? The UDC driver has to
know the gadget driver's limitations before it can connect to the host.
Alan Stern
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv2] usb: gadget: get rid of USB_GADGET_DUALSPEED and USB_GADGET_SUPERSPEED
2011-08-18 17:27 ` Alan Stern
@ 2011-08-18 20:13 ` Michal Nazarewicz
2011-08-18 20:30 ` Alan Stern
0 siblings, 1 reply; 70+ messages in thread
From: Michal Nazarewicz @ 2011-08-18 20:13 UTC (permalink / raw)
To: Alan Stern
Cc: Sergei Shtylyov, Felipe Balbi, Sebastian Andrzej Siewior,
Yang Rui Rui, Greg Kroah-Hartman, linux-usb, linux-kernel
On Thu, 18 Aug 2011 19:27:21 +0200, Alan Stern <stern@rowland.harvard.edu>
wrote:
> On Thu, 18 Aug 2011, Michal Nazarewicz wrote:
>
>> For the most part, usb_composite_probe() is called only once in module's
>> init function. As far as I know, only g_ffs calls it several times. So
>> in all cases expect for g_ffs, composite_driver.speed =
>> min(composite_driver.speed,
>> driver->max_speed) should have the same effect as composite_driver.speed
>> = driver->max_speed.
>>
>> > For example, if you have a composite gadget where one of the function
>> > drivers can handle SuperSpeed and the other can't go beyond high
>> speed,
>> > the overall gadget must never run faster than high speed.
>>
>> Shouldn't that be dealt in usb_add_function()? I cannot see any code
>> that would do that here atm though.
> Maybe you're right. But isn't that too late? The UDC driver has to
> know the gadget driver's limitations before it can connect to the host.
usb_add_function() is called before usb_composite_probe() which only then
calls usb_gadget_probe_driver().
What's more, if I understand it correctly, comment in usb_add_function()
says that we support all the speeds usb_composite_driver structure claims
we do but then return different sets of functions depending on speed. Ie.
if a function is only full speed and host requests high speed,
configuration
will lack that function. The comment in question is:
/* We allow configurations that don't work at both speeds.
* If we run into a lowspeed Linux system, treat it the same
* as full speed ... it's the function drivers that will need
* to avoid bulk and ISO transfers. */
usb_add_function() then proceeds to set config->fullspeed,
config->highspeed
and config->superspeed depending on what descriptors function provides.
Those are later used in config_desc() where it iterates over all
configurations checking which have functions supporting given speed:
list_for_each_entry(c, &cdev->configs, list) {
switch (speed) {
case USB_SPEED_SUPER: if (!c->superspeed) continue; break;
case USB_SPEED_HIGH: if (!c->highspeed) continue; break;
default: if (!c->fullspeed) continue;
}
if (w_value == 0)
return config_buf(c, speed, cdev->req->buf, type);
w_value--;
}
config_buf() at the same time, iterates over all functions and skips the
ones
that do not provide descriptors for given speed:
list_for_each_entry(f, &config->functions, list) {
switch (speed) {
case USB_SPEED_SUPER: descriptors = f->ss_descriptors; break;
case USB_SPEED_HIGH: descriptors = f->hs_descriptors; break;
default: descriptors = f->descriptors;
}
if (!descriptors)
continue;
[...]
}
So I think that your suggestion to use composite_driver.speed =
driver->max_speed was by all means correct.
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michal "mina86" Nazarewicz (o o)
ooo +-----<email/xmpp: mnazarewicz@google.com>-----ooO--(_)--Ooo--
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv2] usb: gadget: get rid of USB_GADGET_DUALSPEED and USB_GADGET_SUPERSPEED
2011-08-18 20:13 ` Michal Nazarewicz
@ 2011-08-18 20:30 ` Alan Stern
2011-08-18 20:44 ` Michal Nazarewicz
2011-08-19 10:53 ` Michal Nazarewicz
0 siblings, 2 replies; 70+ messages in thread
From: Alan Stern @ 2011-08-18 20:30 UTC (permalink / raw)
To: Michal Nazarewicz
Cc: Sergei Shtylyov, Felipe Balbi, Sebastian Andrzej Siewior,
Yang Rui Rui, Greg Kroah-Hartman, linux-usb, linux-kernel
On Thu, 18 Aug 2011, Michal Nazarewicz wrote:
> usb_add_function() is called before usb_composite_probe() which only then
> calls usb_gadget_probe_driver().
>
> What's more, if I understand it correctly, comment in usb_add_function()
> says that we support all the speeds usb_composite_driver structure claims
> we do but then return different sets of functions depending on speed. Ie.
> if a function is only full speed and host requests high speed,
> configuration
> will lack that function. The comment in question is:
>
> /* We allow configurations that don't work at both speeds.
> * If we run into a lowspeed Linux system, treat it the same
> * as full speed ... it's the function drivers that will need
> * to avoid bulk and ISO transfers. */
>
> usb_add_function() then proceeds to set config->fullspeed,
> config->highspeed
> and config->superspeed depending on what descriptors function provides.
Have you misinterpreted that comment? To me, it seems to be saying
that the gadget should allow connections that are slower than the
maximum supported speed. But that's not what I'm talking about -- I'm
suggesting that the gadget needs to avoid running faster than the
maximum supported speed.
> Those are later used in config_desc() where it iterates over all
> configurations checking which have functions supporting given speed:
>
> list_for_each_entry(c, &cdev->configs, list) {
> switch (speed) {
> case USB_SPEED_SUPER: if (!c->superspeed) continue; break;
> case USB_SPEED_HIGH: if (!c->highspeed) continue; break;
> default: if (!c->fullspeed) continue;
> }
> if (w_value == 0)
> return config_buf(c, speed, cdev->req->buf, type);
> w_value--;
> }
>
> config_buf() at the same time, iterates over all functions and skips the
> ones
> that do not provide descriptors for given speed:
>
> list_for_each_entry(f, &config->functions, list) {
> switch (speed) {
> case USB_SPEED_SUPER: descriptors = f->ss_descriptors; break;
> case USB_SPEED_HIGH: descriptors = f->hs_descriptors; break;
> default: descriptors = f->descriptors;
> }
> if (!descriptors)
> continue;
> [...]
> }
>
> So I think that your suggestion to use composite_driver.speed =
> driver->max_speed was by all means correct.
Okay, that line was probably all right, but it seems that something
else needs to be changed. In a composite gadget, if none of the
function drivers support SuperSpeed then we don't want
composite_driver.speed to be set to USB_SPEED_SUPER. It would be
foolish to allow the UDC to connect at a speed which would make the
gadget useless.
Alan Stern
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv2] usb: gadget: get rid of USB_GADGET_DUALSPEED and USB_GADGET_SUPERSPEED
2011-08-18 20:30 ` Alan Stern
@ 2011-08-18 20:44 ` Michal Nazarewicz
2011-08-19 10:53 ` Michal Nazarewicz
1 sibling, 0 replies; 70+ messages in thread
From: Michal Nazarewicz @ 2011-08-18 20:44 UTC (permalink / raw)
To: Alan Stern
Cc: Sergei Shtylyov, Felipe Balbi, Sebastian Andrzej Siewior,
Yang Rui Rui, Greg Kroah-Hartman, linux-usb, linux-kernel
> On Thu, 18 Aug 2011, Michal Nazarewicz wrote:
>> usb_add_function() is called before usb_composite_probe() which only
>> then calls usb_gadget_probe_driver().
>>
>> What's more, if I understand it correctly, comment in usb_add_function()
>> says that we support all the speeds usb_composite_driver structure
>> claims we do but then return different sets of functions depending
>> on speed. Ie. if a function is only full speed and host requests
>> high speed, configuration will lack that function. The comment in
>> question is:
>>
>> /* We allow configurations that don't work at both speeds.
>> * If we run into a lowspeed Linux system, treat it the same
>> * as full speed ... it's the function drivers that will need
>> * to avoid bulk and ISO transfers. */
>>
>> usb_add_function() then proceeds to set config->fullspeed,
>> config->highspeed and config->superspeed depending on what
>> descriptors function provides.
On Thu, 18 Aug 2011 22:30:14 +0200, Alan Stern wrote:
> Have you misinterpreted that comment?
Might be. ;)
[...]
>> So I think that your suggestion to use composite_driver.speed =
>> driver->max_speed was by all means correct.
> Okay, that line was probably all right, but it seems that something
> else needs to be changed. In a composite gadget, if none of the
> function drivers support SuperSpeed then we don't want
> composite_driver.speed to be set to USB_SPEED_SUPER. It would be
> foolish to allow the UDC to connect at a speed which would make the
> gadget useless.
My understanding is that that's the purpose of driver->max_speed.
Composite gadget's author is supposed to set it to the maximum speed
he intends his gadget to run with.
This have some consequences of course: driver->max_speed may be set
to super speed where no function actually supports super speed so
if host chooses this speed we will return no configuration (which
I don't know if it's even legal).
At the same time, author may consciously decide to set
driver->max_speed to some value that not all functions support
aware that depending on speed host chooses, different functions
will be available.
The best composite layer can do, is try to check whether there
is at least one configuration supporting the speed
driver->max_speed declares and if not, decrease it or return
with error.
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michal "mina86" Nazarewicz (o o)
ooo +-----<email/xmpp: mnazarewicz@google.com>-----ooO--(_)--Ooo--
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv2] usb: gadget: get rid of USB_GADGET_DUALSPEED and USB_GADGET_SUPERSPEED
2011-08-18 17:05 ` Michal Nazarewicz
2011-08-18 17:27 ` Alan Stern
@ 2011-08-19 2:02 ` Yang Rui Rui
2011-08-19 12:17 ` Michal Nazarewicz
1 sibling, 1 reply; 70+ messages in thread
From: Yang Rui Rui @ 2011-08-19 2:02 UTC (permalink / raw)
To: Michal Nazarewicz
Cc: Alan Stern, Sergei Shtylyov, Felipe Balbi,
Sebastian Andrzej Siewior, Yang Ruirui R, Greg Kroah-Hartman,
linux-usb, linux-kernel
On 08/19/2011 01:05 AM, Michal Nazarewicz wrote:
> On Thu, 18 Aug 2011 16:59:28 +0200, Alan Stern<stern@rowland.harvard.edu>
> wrote:
>
>> On Thu, 18 Aug 2011, Michal Nazarewicz wrote:
>>
>>> On Wed, 17 Aug 2011 23:09:37 +0200, Alan Stern
>>> <stern@rowland.harvard.edu>
>>> wrote:
>>>> I see what's going on here. Your original patch was wrong and then my
>>>> correction was wrong as well.
>>>>
>>>> This line has to remain the way it was (although those (u8) typecasts
>>>> don't seem to be necessary). Above, you have to initialize
>>>> composite_driver.speed to an appropriate value, probably
>>>> USB_SPEED_SUPER.
>>>>
>>>> What you didn't realize in your original patch is that
>>>> usb_composite_probe() gets called more than once. Each time it is
>>>> called, it has to adjust composite_driver.speed.
>>>
>>> That's sneaky of composite.c... But is it desired behaviour?
>>
>> Yes, it is.
>>
>>> I cannot
>>> came up with a situation where that's what we want. I would imagine
>>> that
>>> usb_composite_probe() should assume the same speed for given
>>> usb_composite_driver regardless if some other slower
>>> usb_composite_driver
>>> was loaded.
>>
>> The speed being calculated isn't the speed of the usb_composite_driver;
>> it's the speed of the usb_gadget_driver. The gadget itself cannot be
>> allowed to run faster than its internal drivers can handle.
>
> Yeah, that's why you set usb_gadget_driver's speed to the speed declared
> in usb_composite_driver.
This is what I means in before reply, musb_gadget_start will check the speed set here.
if it != USB_SPEED_HIGH it will return -EINVAL.
>
> For the most part, usb_composite_probe() is called only once in module's
> init function. As far as I know, only g_ffs calls it several times. So
> in all cases expect for g_ffs, composite_driver.speed =
> min(composite_driver.speed,
> driver->max_speed) should have the same effect as composite_driver.speed
> = driver->max_speed.
For original code, ifndef CONFIG_USB_GADGET_SUPERSPEED and max_speed = USB_SPEED_SUPER
then the result will be:
composite_driver.speed = min(USB_SPEED_HIGH, USB_SPEED_SUPER)
For patched code, composite_driver.speed = USB_SPEED_SUPER
>
>> For example, if you have a composite gadget where one of the function
>> drivers can handle SuperSpeed and the other can't go beyond high speed,
>> the overall gadget must never run faster than high speed.
>
> Shouldn't that be dealt in usb_add_function()? I cannot see any code that
> would do that here atm though.
>
--
Thanks
Yang Ruirui
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv2] usb: gadget: get rid of USB_GADGET_DUALSPEED and USB_GADGET_SUPERSPEED
2011-08-18 20:30 ` Alan Stern
2011-08-18 20:44 ` Michal Nazarewicz
@ 2011-08-19 10:53 ` Michal Nazarewicz
2011-08-19 11:13 ` Sebastian Andrzej Siewior
1 sibling, 1 reply; 70+ messages in thread
From: Michal Nazarewicz @ 2011-08-19 10:53 UTC (permalink / raw)
To: Alan Stern
Cc: Sergei Shtylyov, Felipe Balbi, Sebastian Andrzej Siewior,
Yang Rui Rui, Greg Kroah-Hartman, linux-usb, linux-kernel
On Thu, 18 Aug 2011 22:30:14 +0200, Alan Stern <stern@rowland.harvard.edu>
wrote:
> Okay, that line was probably all right, but it seems that something
> else needs to be changed. In a composite gadget, if none of the
> function drivers support SuperSpeed then we don't want
> composite_driver.speed to be set to USB_SPEED_SUPER. It would be
> foolish to allow the UDC to connect at a speed which would make the
> gadget useless.
OK, so it seems I was misunderstanding your comment all along, but
that was nothing a good night sleep couldn't fix. :P
So, what you are saying is that when passed to the UDC driver (ie.
by calling usb_gadget_probe_driver()) the usb_gadget_driver structure
must have speed no higher then what the UDC supports, right? And in
that case, it's not composite specific but affects all the gadgets.
In that case, I think that usb_gadget_probe_driver() itself needs to be
changed since prior to this function we don't know which gadget the
gadget driver will be bound to. Ie. something as follows could be added
somewhere there:
driver->speed = min(driver->speed, udc->gadget->speed)
Does that sound like it makes sense?
At this point though, gadget drivers need to be aware that
usb_gadget_probe_driver() may change the speed field. It should not
be a problem since, as far as I can tell, all gadgets expect for g_ffs
call usb_gadget_probe_driver() only once and in case of g_ffs this can
be solved at composite level.
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michal "mina86" Nazarewicz (o o)
ooo +-----<email/xmpp: mnazarewicz@google.com>-----ooO--(_)--Ooo--
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv2] usb: gadget: get rid of USB_GADGET_DUALSPEED and USB_GADGET_SUPERSPEED
2011-08-19 10:53 ` Michal Nazarewicz
@ 2011-08-19 11:13 ` Sebastian Andrzej Siewior
2011-08-19 12:14 ` Michal Nazarewicz
0 siblings, 1 reply; 70+ messages in thread
From: Sebastian Andrzej Siewior @ 2011-08-19 11:13 UTC (permalink / raw)
To: Michal Nazarewicz
Cc: Alan Stern, Sergei Shtylyov, Felipe Balbi, Yang Rui Rui,
Greg Kroah-Hartman, linux-usb, linux-kernel
Michal Nazarewicz wrote:
> On Thu, 18 Aug 2011 22:30:14 +0200, Alan Stern
> <stern@rowland.harvard.edu> wrote:
>> Okay, that line was probably all right, but it seems that something
>> else needs to be changed. In a composite gadget, if none of the
>> function drivers support SuperSpeed then we don't want
>> composite_driver.speed to be set to USB_SPEED_SUPER. It would be
>> foolish to allow the UDC to connect at a speed which would make the
>> gadget useless.
>
> OK, so it seems I was misunderstanding your comment all along, but
> that was nothing a good night sleep couldn't fix. :P
>
> So, what you are saying is that when passed to the UDC driver (ie.
> by calling usb_gadget_probe_driver()) the usb_gadget_driver structure
> must have speed no higher then what the UDC supports, right? And in
> that case, it's not composite specific but affects all the gadgets.
>
> In that case, I think that usb_gadget_probe_driver() itself needs to be
> changed since prior to this function we don't know which gadget the
> gadget driver will be bound to. Ie. something as follows could be added
> somewhere there:
>
> driver->speed = min(driver->speed, udc->gadget->speed)
>
> Does that sound like it makes sense?
>
> At this point though, gadget drivers need to be aware that
> usb_gadget_probe_driver() may change the speed field. It should not
> be a problem since, as far as I can tell, all gadgets expect for g_ffs
> call usb_gadget_probe_driver() only once and in case of g_ffs this can
> be solved at composite level.
There are three speeds:
- the max speed the UDC supports i.e. HS (no field for that?)
- the max speed the gadget / function driver support i.e. SS
(driver->max_speed ?)
- the speed the UDC is connected at the moment i.e. FS. (gadget->speed)
So in this case you have to go down and use FS descriptors. The FS speed
is represented by gadget->speed right? So for now we do FS. On the next
re-plug the user places OHCI controller with XHCI so you can use HS
descriptors now. So changing dirver->speed wouldn't allow you go beyond FS
once you to FS right?
Sebastian
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv2] usb: gadget: get rid of USB_GADGET_DUALSPEED and USB_GADGET_SUPERSPEED
2011-08-19 11:13 ` Sebastian Andrzej Siewior
@ 2011-08-19 12:14 ` Michal Nazarewicz
2011-08-19 14:29 ` Alan Stern
0 siblings, 1 reply; 70+ messages in thread
From: Michal Nazarewicz @ 2011-08-19 12:14 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: Alan Stern, Sergei Shtylyov, Felipe Balbi, Yang Rui Rui,
Greg Kroah-Hartman, linux-usb, linux-kernel
On Fri, 19 Aug 2011 13:13:41 +0200, Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
> There are three speeds:
> - the max speed the UDC supports i.e. HS (no field for that?)
Yep, the field seems to be missing for that one. We have is_dualspeed and
comment in gadget_is_superspeed() says about is_superspeed but that does
not exist.
> - the max speed the gadget / function driver support i.e. SS
> (driver->max_speed ?)
Actually “max_speed” is only in usb_composite_driver structure.
usb_gadget_driver has only “speed” and it's description says that it is
the “highest speed the driver handles.” Because of that description I've
assumed that it is UDC driver's responsibility to figure out maximum speed
it can use.
> - the speed the UDC is connected at the moment i.e. FS. (gadget->speed)
OK, I see my mistake. I misinterpreted gadget->speed as the max speed
gadget supports.
So in the end, it would seem to me that we need to do the following is
usb_gadget_probe_direvr() before passing the usb_gadget_driver to the
usb_gadegt's bind:
if (driver->speed == USB_SPEED_SUPER && !gadget_is_superspeed(udc->gadget))
driver->speed = USB_SPEED_HIGH;
if (driver->speed == USB_SPEED_HIGH && !gadget_is_dualspeed(udc->gadget))
driver->speed = USB_SPEED_FULL;
(Although I'm not sure about USB_SPEED_FULL vs. USB_SPEED_LOW.)
Either than, or make sure that all UDC drivers handle correctly gadgets
that declare speed as highest of what the UDC can handle.
At the moment however, gadget_is_superspeed() is implemented using #ifdefs
(which I missed before) so we need to add a is_superspeed to usb_gadget
structure. Or maybe better yet get rid of is_dualspeed and add max_speed?
Does what I'm writing make sense at all?
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michal "mina86" Nazarewicz (o o)
ooo +-----<email/xmpp: mnazarewicz@google.com>-----ooO--(_)--Ooo--
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv2] usb: gadget: get rid of USB_GADGET_DUALSPEED and USB_GADGET_SUPERSPEED
2011-08-19 2:02 ` Yang Rui Rui
@ 2011-08-19 12:17 ` Michal Nazarewicz
0 siblings, 0 replies; 70+ messages in thread
From: Michal Nazarewicz @ 2011-08-19 12:17 UTC (permalink / raw)
To: Yang Rui Rui
Cc: Alan Stern, Sergei Shtylyov, Felipe Balbi,
Sebastian Andrzej Siewior, Yang Ruirui R, Greg Kroah-Hartman,
linux-usb, linux-kernel
> On 08/19/2011 01:05 AM, Michal Nazarewicz wrote:
>> For the most part, usb_composite_probe() is called only once in module's
>> init function. As far as I know, only g_ffs calls it several times. So
>> in all cases expect for g_ffs, composite_driver.speed =
>> min(composite_driver.speed,
>> driver->max_speed) should have the same effect as composite_driver.speed
>> = driver->max_speed.
On Fri, 19 Aug 2011 04:02:00 +0200, Yang Rui Rui <ruirui.r.yang@tieto.com>
wrote:
> For original code, ifndef CONFIG_USB_GADGET_SUPERSPEED and max_speed =
> USB_SPEED_SUPER then the result will be:
>
> composite_driver.speed = min(USB_SPEED_HIGH, USB_SPEED_SUPER)
>
> For patched code, composite_driver.speed = USB_SPEED_SUPER
OK, I see your point. This is the issue that we are discussing in another
“branch” of the thread. It seems either usb_gadget_driver_probe() should
make sure driver->speed is not too high or each UDC need to handle gadgets
which support speed higher then the UDC can handle.
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michal "mina86" Nazarewicz (o o)
ooo +-----<email/xmpp: mnazarewicz@google.com>-----ooO--(_)--Ooo--
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv2] usb: gadget: get rid of USB_GADGET_DUALSPEED and USB_GADGET_SUPERSPEED
2011-08-19 12:14 ` Michal Nazarewicz
@ 2011-08-19 14:29 ` Alan Stern
2011-08-19 14:38 ` Michal Nazarewicz
0 siblings, 1 reply; 70+ messages in thread
From: Alan Stern @ 2011-08-19 14:29 UTC (permalink / raw)
To: Michal Nazarewicz
Cc: Sebastian Andrzej Siewior, Sergei Shtylyov, Felipe Balbi,
Yang Rui Rui, Greg Kroah-Hartman, USB list,
Kernel development list
On Fri, 19 Aug 2011, Michal Nazarewicz wrote:
> On Fri, 19 Aug 2011 13:13:41 +0200, Sebastian Andrzej Siewior
> <bigeasy@linutronix.de> wrote:
> > There are three speeds:
> > - the max speed the UDC supports i.e. HS (no field for that?)
>
> Yep, the field seems to be missing for that one. We have is_dualspeed and
> comment in gadget_is_superspeed() says about is_superspeed but that does
> not exist.
Evidently we need is_superspeed.
> > - the max speed the gadget / function driver support i.e. SS
> > (driver->max_speed ?)
>
> Actually “max_speed” is only in usb_composite_driver structure.
> usb_gadget_driver has only “speed” and it's description says that it is
> the “highest speed the driver handles.” Because of that description I've
> assumed that it is UDC driver's responsibility to figure out maximum speed
> it can use.
Correct.
> > - the speed the UDC is connected at the moment i.e. FS. (gadget->speed)
>
> OK, I see my mistake. I misinterpreted gadget->speed as the max speed
> gadget supports.
>
> So in the end, it would seem to me that we need to do the following is
> usb_gadget_probe_direvr() before passing the usb_gadget_driver to the
> usb_gadegt's bind:
>
> if (driver->speed == USB_SPEED_SUPER && !gadget_is_superspeed(udc->gadget))
> driver->speed = USB_SPEED_HIGH;
> if (driver->speed == USB_SPEED_HIGH && !gadget_is_dualspeed(udc->gadget))
> driver->speed = USB_SPEED_FULL;
No, the usb_gadget_driver structure is read-only to the UDC driver
(except for the .driver field). .speed is the maximum speed supported
by the gadget driver -- it doesn't change at runtime. If the driver
was written to support speeds up to high speed, then those are the
speeds it supports, regardless of the UDC's capabilities.
Maybe if would help if the .speed field in struct usb_gadget_driver was
renamed to max_speed.
> (Although I'm not sure about USB_SPEED_FULL vs. USB_SPEED_LOW.)
>
> Either than, or make sure that all UDC drivers handle correctly gadgets
> that declare speed as highest of what the UDC can handle.
I don't quite understand that sentence. However, it is definitely true
that all UDC drivers _must_ avoid connecting at speeds that are faster
than the usb_gadget_driver's .speed field.
> At the moment however, gadget_is_superspeed() is implemented using #ifdefs
> (which I missed before) so we need to add a is_superspeed to usb_gadget
> structure. Or maybe better yet get rid of is_dualspeed and add max_speed?
Either one would be okay. max_speed is simpler, although it would
require some work to change the existing usages of is_dualspeed.
Alan Stern
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv2] usb: gadget: get rid of USB_GADGET_DUALSPEED and USB_GADGET_SUPERSPEED
2011-08-19 14:29 ` Alan Stern
@ 2011-08-19 14:38 ` Michal Nazarewicz
2011-08-19 14:57 ` Alan Stern
0 siblings, 1 reply; 70+ messages in thread
From: Michal Nazarewicz @ 2011-08-19 14:38 UTC (permalink / raw)
To: Alan Stern
Cc: Sebastian Andrzej Siewior, Sergei Shtylyov, Felipe Balbi,
Yang Rui Rui, Greg Kroah-Hartman, USB list,
Kernel development list
> On Fri, 19 Aug 2011, Michal Nazarewicz wrote:
>> OK, I see my mistake. I misinterpreted gadget->speed as the max speed
>> gadget supports.
>>
>> So in the end, it would seem to me that we need to do the following is
>> usb_gadget_probe_direvr() before passing the usb_gadget_driver to the
>> usb_gadegt's bind:
>>
>> if (driver->speed == USB_SPEED_SUPER &&
>> !gadget_is_superspeed(udc->gadget))
>> driver->speed = USB_SPEED_HIGH;
>> if (driver->speed == USB_SPEED_HIGH &&
>> !gadget_is_dualspeed(udc->gadget))
>> driver->speed = USB_SPEED_FULL;
On Fri, 19 Aug 2011 16:29:15 +0200, Alan Stern <stern@rowland.harvard.edu>
wrote:
> No, the usb_gadget_driver structure is read-only to the UDC driver
> (except for the .driver field). .speed is the maximum speed supported
> by the gadget driver -- it doesn't change at runtime. If the driver
> was written to support speeds up to high speed, then those are the
> speeds it supports, regardless of the UDC's capabilities.
OK.
> Maybe if would help if the .speed field in struct usb_gadget_driver was
> renamed to max_speed.
I'll take a look at how much work that would require.
>> (Although I'm not sure about USB_SPEED_FULL vs. USB_SPEED_LOW.)
>>
>> Either than, or make sure that all UDC drivers handle correctly gadgets
>> that declare speed as highest of what the UDC can handle.
> I don't quite understand that sentence. However, it is definitely true
> that all UDC drivers _must_ avoid connecting at speeds that are faster
> than the usb_gadget_driver's .speed field.
Yang has brought up an issue that musb_gadget_start() checks if
driver->speed
equals USB_SPEED_HIGH. This means that if a gadget driver supports super
speed
it'll set driver->speed to USB_SPEED_SUPER and musb_gadget_start() will
complain.
My understanding is that this is a bug in musb driver as it should allow
the
gadget driver to start but never set super speed.
>> At the moment however, gadget_is_superspeed() is implemented using
>> #ifdefs (which I missed before) so we need to add a is_superspeed
>> to usb_gadget structure. Or maybe better yet get rid of
>> is_dualspeed and add max_speed?
> Either one would be okay. max_speed is simpler, although it would
> require some work to change the existing usages of is_dualspeed.
I'm on it already. ;)
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michal "mina86" Nazarewicz (o o)
ooo +-----<email/xmpp: mnazarewicz@google.com>-----ooO--(_)--Ooo--
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv2] usb: gadget: get rid of USB_GADGET_DUALSPEED and USB_GADGET_SUPERSPEED
2011-08-19 14:38 ` Michal Nazarewicz
@ 2011-08-19 14:57 ` Alan Stern
0 siblings, 0 replies; 70+ messages in thread
From: Alan Stern @ 2011-08-19 14:57 UTC (permalink / raw)
To: Michal Nazarewicz
Cc: Sebastian Andrzej Siewior, Sergei Shtylyov, Felipe Balbi,
Yang Rui Rui, Greg Kroah-Hartman, USB list,
Kernel development list
On Fri, 19 Aug 2011, Michal Nazarewicz wrote:
> > I don't quite understand that sentence. However, it is definitely true
> > that all UDC drivers _must_ avoid connecting at speeds that are faster
> > than the usb_gadget_driver's .speed field.
>
> Yang has brought up an issue that musb_gadget_start() checks if
> driver->speed
> equals USB_SPEED_HIGH. This means that if a gadget driver supports super
> speed
> it'll set driver->speed to USB_SPEED_SUPER and musb_gadget_start() will
> complain.
>
> My understanding is that this is a bug in musb driver as it should allow
> the
> gadget driver to start but never set super speed.
That's right. It should also allow driver->speed to be USB_SPEED_FULL,
in which case it should never connect at high speed (see gmidi.c).
Alan Stern
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCHv3 0/4] Figuring out speed refactorisation
2011-08-17 15:33 ` [PATCHv2] " Michal Nazarewicz
2011-08-17 21:09 ` Alan Stern
2011-08-18 3:01 ` Yang Rui Rui
@ 2011-08-19 22:32 ` Michal Nazarewicz
2011-08-19 22:32 ` [PATCHv3 1/4] usb: Provide usb_device_speed_name() function Michal Nazarewicz
` (3 more replies)
2 siblings, 4 replies; 70+ messages in thread
From: Michal Nazarewicz @ 2011-08-19 22:32 UTC (permalink / raw)
To: Alan Stern, Sebastian Andrzej Siewior, Yang Rui Rui, Dave Young
Cc: Felipe Balbi, linux-usb, linux-kernel
From: Michal Nazarewicz <mina86@mina86.com>
Michal Nazarewicz (4):
usb: Provide usb_device_speed_name() function
Just a little helper.
usb: gadget: replace "is_dualspeed" with "max_speed"
This replaces "is_dualspeed" with "max_speed" so that one
can encode in the field whether given UDC is super speed
as well.
Without this change, we would have to add a "is_superspeed"
field since after removal of USB_GADGET_SUPERSPEED Kconfig
option current implementation of gadget_is_superspeed()
won't work.
usb: gadget: rename usb_gadget_driver::speed to max_speed
I don't have strong feelings about this patch, but the current
name of the usb_gadget_driver::speed field may lead to some
confusion.
Changing it to "max_speed" should be apparent that it describes
the maximum speed the gadget driver supports.
This comes in pair with usb_gadget::max_speed and
usb_composite_driver::max_speed.
usb: gadget: get rid of USB_GADGET_{DUAL,SUPER}SPEED
Removes the USB_GADGET_DUALSPEED and USB_GADGET_SUPERSPEED Kconfig
options. In most cases, they were checked just for optimisation
(ie. if UDC was not dual speed, gadget drivers could skip some of
the logic) so in the end this change was just about deleting
#ifdefs.
Making those changes has exposed problems with some UDC drivers,
namely:
$ g grep -A3 'max_speed.*[=!]=.*USB_SPEED_HIGH'
drivers/usb/gadget/amd5536udc.c: || driver->max_speed != USB_SPEED_HIGH)
drivers/usb/gadget/amd5536udc.c- return -EINVAL;
drivers/usb/gadget/amd5536udc.c- if (!dev)
drivers/usb/gadget/amd5536udc.c- return -ENODEV;
--
drivers/usb/gadget/m66592-udc.c: || driver->max_speed != USB_SPEED_HIGH
drivers/usb/gadget/m66592-udc.c- || !bind
drivers/usb/gadget/m66592-udc.c- || !driver->setup)
drivers/usb/gadget/m66592-udc.c- return -EINVAL;
--
drivers/usb/gadget/net2272.c: driver->max_speed != USB_SPEED_HIGH)
drivers/usb/gadget/net2272.c- return -EINVAL;
drivers/usb/gadget/net2272.c- if (!dev)
drivers/usb/gadget/net2272.c- return -ENODEV;
--
drivers/usb/gadget/net2280.c: || driver->max_speed != USB_SPEED_HIGH
drivers/usb/gadget/net2280.c- || !bind || !driver->setup)
drivers/usb/gadget/net2280.c- return -EINVAL;
drivers/usb/gadget/net2280.c- if (!dev)
--
drivers/usb/gadget/r8a66597-udc.c: || driver->max_speed != USB_SPEED_HIGH
drivers/usb/gadget/r8a66597-udc.c- || !bind
drivers/usb/gadget/r8a66597-udc.c- || !driver->setup)
drivers/usb/gadget/r8a66597-udc.c- return -EINVAL;
All those drivers require usb_gadget_driver::max_speed to be set to
USB_SPEED_HIGH. This basically means that gmidi (which sets it to
USB_SPEED_FULL), g_ether and g_zero (which set it to USB_SPEED_SUPER)
won't work with those UDCs:
$ g grep -e '\.max_speed.*=.*USB_SPEED_SUPER' -e '\.max_speed.*=.*USB_SPEED_FULL'
dummy_hcd.c: dum->gadget.max_speed = USB_SPEED_SUPER;;
ether.c: .max_speed = USB_SPEED_SUPER,
gmidi.c: .max_speed = USB_SPEED_FULL,
goku_udc.c: dev->gadget.max_speed = USB_SPEED_FULL;
omap_udc.c: udc->gadget.max_speed = USB_SPEED_FULL;
zero.c: .max_speed = USB_SPEED_SUPER,
So as far as I can tell, UDC drivers for those need to be fixed (but
that's probably scope for another patchset). Or am I missing
something?
Like before, this has been compile-tested only.
drivers/usb/Makefile | 3 ++
drivers/usb/common.c | 24 +++++++++++++++++
drivers/usb/gadget/Kconfig | 30 ----------------------
drivers/usb/gadget/amd5536udc.c | 4 +-
drivers/usb/gadget/atmel_usba_udc.c | 2 +-
drivers/usb/gadget/ci13xxx_udc.c | 9 ++++--
drivers/usb/gadget/composite.c | 9 +------
drivers/usb/gadget/dbgp.c | 2 +-
drivers/usb/gadget/dummy_hcd.c | 15 +++++------
drivers/usb/gadget/epautoconf.c | 6 ++--
drivers/usb/gadget/file_storage.c | 6 +---
drivers/usb/gadget/fsl_udc_core.c | 2 +-
drivers/usb/gadget/fusb300_udc.c | 2 +-
drivers/usb/gadget/gmidi.c | 2 +-
drivers/usb/gadget/goku_udc.c | 3 +-
drivers/usb/gadget/inode.c | 14 +---------
drivers/usb/gadget/langwell_udc.c | 2 +-
drivers/usb/gadget/m66592-udc.c | 4 +-
drivers/usb/gadget/mv_udc_core.c | 2 +-
drivers/usb/gadget/net2272.c | 4 +-
drivers/usb/gadget/net2280.c | 4 +-
drivers/usb/gadget/omap_udc.c | 1 +
drivers/usb/gadget/pch_udc.c | 4 +-
drivers/usb/gadget/printer.c | 43 +++++--------------------------
drivers/usb/gadget/r8a66597-udc.c | 4 +-
drivers/usb/gadget/s3c-hsotg.c | 2 +-
drivers/usb/gadget/s3c-hsudc.c | 2 +-
drivers/usb/gadget/u_ether.c | 7 -----
drivers/usb/gadget/udc-core.c | 41 +++++++++++++++---------------
drivers/usb/musb/musb_gadget.c | 2 +-
drivers/usb/renesas_usbhs/mod_gadget.c | 2 +-
include/linux/usb/ch9.h | 8 ++++++
include/linux/usb/gadget.h | 29 +++++----------------
33 files changed, 115 insertions(+), 179 deletions(-)
create mode 100644 drivers/usb/common.c
--
1.7.3.1
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCHv3 1/4] usb: Provide usb_device_speed_name() function
2011-08-19 22:32 ` [PATCHv3 0/4] Figuring out speed refactorisation Michal Nazarewicz
@ 2011-08-19 22:32 ` Michal Nazarewicz
2011-08-19 23:15 ` Felipe Balbi
2011-08-19 22:33 ` [PATCHv3 2/4] usb: gadget: replace "is_dualspeed" with "max_speed" Michal Nazarewicz
` (2 subsequent siblings)
3 siblings, 1 reply; 70+ messages in thread
From: Michal Nazarewicz @ 2011-08-19 22:32 UTC (permalink / raw)
To: Alan Stern, Sebastian Andrzej Siewior, Yang Rui Rui, Dave Young
Cc: Felipe Balbi, linux-usb, linux-kernel
From: Michal Nazarewicz <mina86@mina86.com>
In a few places kernel wants to print a human-readable USB
device speed name (eg. "high") instead of a raw number
(eg. 3). Usually a switch is introduced in those places
leading to code repetition. To mitigate this issue, this
commit introduces usb_device_speed_name() function, which
returns a human-readable name of provided speed.
Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
---
drivers/usb/Makefile | 3 +++
drivers/usb/common.c | 24 ++++++++++++++++++++++++
drivers/usb/gadget/udc-core.c | 19 ++-----------------
include/linux/usb/ch9.h | 8 ++++++++
4 files changed, 37 insertions(+), 17 deletions(-)
create mode 100644 drivers/usb/common.c
diff --git a/drivers/usb/Makefile b/drivers/usb/Makefile
index 30ddf8d..a3167cc 100644
--- a/drivers/usb/Makefile
+++ b/drivers/usb/Makefile
@@ -51,3 +51,6 @@ obj-$(CONFIG_USB_MUSB_HDRC) += musb/
obj-$(CONFIG_USB_RENESAS_USBHS) += renesas_usbhs/
obj-$(CONFIG_USB_OTG_UTILS) += otg/
obj-$(CONFIG_USB_GADGET) += gadget/
+
+obj-$(CONFIG_USB) += common.o
+obj-$(CONFIG_USB_GADGET) += common.o
diff --git a/drivers/usb/common.c b/drivers/usb/common.c
new file mode 100644
index 0000000..e9d7141
--- /dev/null
+++ b/drivers/usb/common.c
@@ -0,0 +1,24 @@
+/*
+ * Provides code common for host and device side USB.
+ */
+
+#include <linux/kernel.h> /* for ARRAY_SIZE() */
+#include <linux/module.h> /* for EXPORT_SYMBOL_GPL() */
+#include <linux/usb/ch9.h>
+
+const char *usb_device_speed_name(enum usb_device_speed speed)
+{
+ static const char *const names[] = {
+ "UNKNOWN",
+ "low-speed",
+ "full-speed",
+ "high-speed",
+ "wireless",
+ "super-speed",
+ };
+
+ if (speed < 0 || speed >= ARRAY_SIZE(names))
+ speed = USB_SPEED_UNKNOWN;
+ return names[speed];
+}
+EXPORT_SYMBOL_GPL(usb_device_speed_name);
diff --git a/drivers/usb/gadget/udc-core.c b/drivers/usb/gadget/udc-core.c
index 05ba472..e1ecdbc 100644
--- a/drivers/usb/gadget/udc-core.c
+++ b/drivers/usb/gadget/udc-core.c
@@ -375,23 +375,8 @@ static ssize_t usb_udc_speed_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
struct usb_udc *udc = container_of(dev, struct usb_udc, dev);
- struct usb_gadget *gadget = udc->gadget;
-
- switch (gadget->speed) {
- case USB_SPEED_LOW:
- return snprintf(buf, PAGE_SIZE, "low-speed\n");
- case USB_SPEED_FULL:
- return snprintf(buf, PAGE_SIZE, "full-speed\n");
- case USB_SPEED_HIGH:
- return snprintf(buf, PAGE_SIZE, "high-speed\n");
- case USB_SPEED_WIRELESS:
- return snprintf(buf, PAGE_SIZE, "wireless\n");
- case USB_SPEED_SUPER:
- return snprintf(buf, PAGE_SIZE, "super-speed\n");
- case USB_SPEED_UNKNOWN: /* FALLTHROUGH */
- default:
- return snprintf(buf, PAGE_SIZE, "UNKNOWN\n");
- }
+ return snprintf(buf, PAGE_SIZE, "%s\n",
+ usb_device_speed_name(udc->gadget->speed));
}
static DEVICE_ATTR(speed, S_IRUSR, usb_udc_speed_show, NULL);
diff --git a/include/linux/usb/ch9.h b/include/linux/usb/ch9.h
index 0fd3fbd..61c4c80 100644
--- a/include/linux/usb/ch9.h
+++ b/include/linux/usb/ch9.h
@@ -851,6 +851,14 @@ enum usb_device_speed {
USB_SPEED_SUPER, /* usb 3.0 */
};
+/**
+ * usb_device_speed_name() - Returns human readable-name of the speed.
+ * @speed: The speed to return human-readable name for. If it's not
+ * any of the speeds defined in usb_device_speed enum, string for
+ * USB_SPEED_UNKNOWN will be returned.
+ */
+extern const char *usb_device_speed_name(enum usb_device_speed speed);
+
enum usb_device_state {
/* NOTATTACHED isn't in the USB spec, and this state acts
* the same as ATTACHED ... but it's clearer this way.
--
1.7.3.1
^ permalink raw reply related [flat|nested] 70+ messages in thread
* [PATCHv3 2/4] usb: gadget: replace "is_dualspeed" with "max_speed"
2011-08-19 22:32 ` [PATCHv3 0/4] Figuring out speed refactorisation Michal Nazarewicz
2011-08-19 22:32 ` [PATCHv3 1/4] usb: Provide usb_device_speed_name() function Michal Nazarewicz
@ 2011-08-19 22:33 ` Michal Nazarewicz
2011-08-19 23:28 ` Felipe Balbi
2011-08-19 22:33 ` [PATCHv3 3/4] usb: gadget: rename usb_gadget_driver::speed to max_speed Michal Nazarewicz
2011-08-19 22:33 ` [PATCHv3 4/4] usb: gadget: get rid of USB_GADGET_{DUAL,SUPER}SPEED Michal Nazarewicz
3 siblings, 1 reply; 70+ messages in thread
From: Michal Nazarewicz @ 2011-08-19 22:33 UTC (permalink / raw)
To: Alan Stern, Sebastian Andrzej Siewior, Yang Rui Rui, Dave Young
Cc: Felipe Balbi, linux-usb, linux-kernel
From: Michal Nazarewicz <mina86@mina86.com>
This commit replaces usb_gadget's is_dualspeed field with
a max_speed field.
This change is made so that one will be able to check at
run-time if given gadget supports super speed.
Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
---
drivers/usb/gadget/amd5536udc.c | 2 +-
drivers/usb/gadget/atmel_usba_udc.c | 2 +-
drivers/usb/gadget/ci13xxx_udc.c | 7 +++++--
drivers/usb/gadget/dummy_hcd.c | 2 +-
drivers/usb/gadget/epautoconf.c | 6 +++---
drivers/usb/gadget/fsl_udc_core.c | 2 +-
drivers/usb/gadget/fusb300_udc.c | 2 +-
drivers/usb/gadget/goku_udc.c | 1 +
drivers/usb/gadget/langwell_udc.c | 2 +-
drivers/usb/gadget/m66592-udc.c | 2 +-
drivers/usb/gadget/mv_udc_core.c | 2 +-
drivers/usb/gadget/net2272.c | 2 +-
drivers/usb/gadget/net2280.c | 2 +-
drivers/usb/gadget/omap_udc.c | 1 +
drivers/usb/gadget/pch_udc.c | 2 +-
drivers/usb/gadget/printer.c | 4 ++--
drivers/usb/gadget/r8a66597-udc.c | 2 +-
drivers/usb/gadget/s3c-hsotg.c | 2 +-
drivers/usb/gadget/s3c-hsudc.c | 2 +-
drivers/usb/gadget/udc-core.c | 26 ++++++++++++++++++++------
drivers/usb/musb/musb_gadget.c | 2 +-
drivers/usb/renesas_usbhs/mod_gadget.c | 2 +-
include/linux/usb/gadget.h | 10 +++++-----
23 files changed, 53 insertions(+), 34 deletions(-)
diff --git a/drivers/usb/gadget/amd5536udc.c b/drivers/usb/gadget/amd5536udc.c
index 70f2b37..80ae808 100644
--- a/drivers/usb/gadget/amd5536udc.c
+++ b/drivers/usb/gadget/amd5536udc.c
@@ -3363,7 +3363,7 @@ static int udc_probe(struct udc *dev)
dev_set_name(&dev->gadget.dev, "gadget");
dev->gadget.dev.release = gadget_release;
dev->gadget.name = name;
- dev->gadget.is_dualspeed = 1;
+ dev->gadget.max_speed = USB_SPEED_HIGH;
/* init registers, interrupts, ... */
startup_registers(dev);
diff --git a/drivers/usb/gadget/atmel_usba_udc.c b/drivers/usb/gadget/atmel_usba_udc.c
index 5b1665e..8837d92c 100644
--- a/drivers/usb/gadget/atmel_usba_udc.c
+++ b/drivers/usb/gadget/atmel_usba_udc.c
@@ -1038,7 +1038,7 @@ static struct usba_udc the_udc = {
.gadget = {
.ops = &usba_udc_ops,
.ep_list = LIST_HEAD_INIT(the_udc.gadget.ep_list),
- .is_dualspeed = 1,
+ .max_speed = USB_SPEED_HIGH,
.name = "atmel_usba_udc",
.dev = {
.init_name = "gadget",
diff --git a/drivers/usb/gadget/ci13xxx_udc.c b/drivers/usb/gadget/ci13xxx_udc.c
index 1265a85..b877464 100644
--- a/drivers/usb/gadget/ci13xxx_udc.c
+++ b/drivers/usb/gadget/ci13xxx_udc.c
@@ -751,8 +751,11 @@ static ssize_t show_device(struct device *dev, struct device_attribute *attr,
n += scnprintf(buf + n, PAGE_SIZE - n, "speed = %d\n",
gadget->speed);
+ n += scnprintf(buf + n, PAGE_SIZE - n, "max_speed = %d\n",
+ gadget->max_speed);
+ /* Provide "is_dualspeed" for backward compatibility. */
n += scnprintf(buf + n, PAGE_SIZE - n, "is_dualspeed = %d\n",
- gadget->is_dualspeed);
+ gadget_is_dualspeed(gadget));
n += scnprintf(buf + n, PAGE_SIZE - n, "is_otg = %d\n",
gadget->is_otg);
n += scnprintf(buf + n, PAGE_SIZE - n, "is_a_peripheral = %d\n",
@@ -2866,7 +2869,7 @@ static int udc_probe(struct ci13xxx_udc_driver *driver, struct device *dev,
udc->gadget.ops = &usb_gadget_ops;
udc->gadget.speed = USB_SPEED_UNKNOWN;
- udc->gadget.is_dualspeed = 1;
+ udc->gadget.max_speed = USB_SPEED_HIGH;
udc->gadget.is_otg = 0;
udc->gadget.name = driver->name;
diff --git a/drivers/usb/gadget/dummy_hcd.c b/drivers/usb/gadget/dummy_hcd.c
index e755a9d..fa60e9b 100644
--- a/drivers/usb/gadget/dummy_hcd.c
+++ b/drivers/usb/gadget/dummy_hcd.c
@@ -986,7 +986,7 @@ static int dummy_udc_probe (struct platform_device *pdev)
dum->gadget.name = gadget_name;
dum->gadget.ops = &dummy_ops;
- dum->gadget.is_dualspeed = 1;
+ dum->gadget.max_speed = USB_SPEED_SUPER;;
dev_set_name(&dum->gadget.dev, "gadget");
dum->gadget.dev.parent = &pdev->dev;
diff --git a/drivers/usb/gadget/epautoconf.c b/drivers/usb/gadget/epautoconf.c
index 7a7e6b7..5b40f4b 100644
--- a/drivers/usb/gadget/epautoconf.c
+++ b/drivers/usb/gadget/epautoconf.c
@@ -162,7 +162,7 @@ ep_matches (
switch (type) {
case USB_ENDPOINT_XFER_INT:
/* INT: limit 64 bytes full speed, 1024 high/super speed */
- if (!gadget->is_dualspeed && max > 64)
+ if (!gadget_is_dualspeed(gadget) && max > 64)
return 0;
/* FALLTHROUGH */
@@ -170,12 +170,12 @@ ep_matches (
/* ISO: limit 1023 bytes full speed, 1024 high/super speed */
if (ep->maxpacket < max)
return 0;
- if (!gadget->is_dualspeed && max > 1023)
+ if (!gadget_is_dualspeed(gadget) && max > 1023)
return 0;
/* BOTH: "high bandwidth" works only at high speed */
if ((desc->wMaxPacketSize & cpu_to_le16(3<<11))) {
- if (!gadget->is_dualspeed)
+ if (!gadget_is_dualspeed(gadget))
return 0;
/* configure your hardware with enough buffering!! */
}
diff --git a/drivers/usb/gadget/fsl_udc_core.c b/drivers/usb/gadget/fsl_udc_core.c
index de24a42..b16034c 100644
--- a/drivers/usb/gadget/fsl_udc_core.c
+++ b/drivers/usb/gadget/fsl_udc_core.c
@@ -2546,7 +2546,7 @@ static int __init fsl_udc_probe(struct platform_device *pdev)
/* Setup gadget structure */
udc_controller->gadget.ops = &fsl_gadget_ops;
- udc_controller->gadget.is_dualspeed = 1;
+ udc_controller->gadget.max_speed = USB_SPEED_HIGH;
udc_controller->gadget.ep0 = &udc_controller->eps[0].ep;
INIT_LIST_HEAD(&udc_controller->gadget.ep_list);
udc_controller->gadget.speed = USB_SPEED_UNKNOWN;
diff --git a/drivers/usb/gadget/fusb300_udc.c b/drivers/usb/gadget/fusb300_udc.c
index 0e4489f..731675f 100644
--- a/drivers/usb/gadget/fusb300_udc.c
+++ b/drivers/usb/gadget/fusb300_udc.c
@@ -1473,7 +1473,7 @@ static int __init fusb300_probe(struct platform_device *pdev)
dev_set_name(&fusb300->gadget.dev, "gadget");
- fusb300->gadget.is_dualspeed = 1;
+ fusb300->gadget.max_speed = USB_SPEED_HIGH;
fusb300->gadget.dev.parent = &pdev->dev;
fusb300->gadget.dev.dma_mask = pdev->dev.dma_mask;
fusb300->gadget.dev.release = pdev->dev.release;
diff --git a/drivers/usb/gadget/goku_udc.c b/drivers/usb/gadget/goku_udc.c
index 7f87805..ab9c924 100644
--- a/drivers/usb/gadget/goku_udc.c
+++ b/drivers/usb/gadget/goku_udc.c
@@ -1796,6 +1796,7 @@ static int goku_probe(struct pci_dev *pdev, const struct pci_device_id *id)
spin_lock_init(&dev->lock);
dev->pdev = pdev;
dev->gadget.ops = &goku_ops;
+ dev->gadget.max_speed = USB_SPEED_FULL;
/* the "gadget" abstracts/virtualizes the controller */
dev_set_name(&dev->gadget.dev, "gadget");
diff --git a/drivers/usb/gadget/langwell_udc.c b/drivers/usb/gadget/langwell_udc.c
index a06e2c2..121a64b 100644
--- a/drivers/usb/gadget/langwell_udc.c
+++ b/drivers/usb/gadget/langwell_udc.c
@@ -3320,7 +3320,7 @@ static int langwell_udc_probe(struct pci_dev *pdev,
dev->gadget.ep0 = &dev->ep[0].ep; /* gadget ep0 */
INIT_LIST_HEAD(&dev->gadget.ep_list); /* ep_list */
dev->gadget.speed = USB_SPEED_UNKNOWN; /* speed */
- dev->gadget.is_dualspeed = 1; /* support dual speed */
+ dev->gadget.max_speed = USB_SPEED_HIGH; /* support dual speed */
#ifdef OTG_TRANSCEIVER
dev->gadget.is_otg = 1; /* support otg mode */
#endif
diff --git a/drivers/usb/gadget/m66592-udc.c b/drivers/usb/gadget/m66592-udc.c
index 491f825..fbdfca8 100644
--- a/drivers/usb/gadget/m66592-udc.c
+++ b/drivers/usb/gadget/m66592-udc.c
@@ -1663,7 +1663,7 @@ static int __init m66592_probe(struct platform_device *pdev)
m66592->gadget.ops = &m66592_gadget_ops;
device_initialize(&m66592->gadget.dev);
dev_set_name(&m66592->gadget.dev, "gadget");
- m66592->gadget.is_dualspeed = 1;
+ m66592->gadget.max_speed = USB_SPEED_HIGH;
m66592->gadget.dev.parent = &pdev->dev;
m66592->gadget.dev.dma_mask = pdev->dev.dma_mask;
m66592->gadget.dev.release = pdev->dev.release;
diff --git a/drivers/usb/gadget/mv_udc_core.c b/drivers/usb/gadget/mv_udc_core.c
index ce1ac2b..17e9c5d 100644
--- a/drivers/usb/gadget/mv_udc_core.c
+++ b/drivers/usb/gadget/mv_udc_core.c
@@ -2061,7 +2061,7 @@ int mv_udc_probe(struct platform_device *dev)
udc->gadget.ep0 = &udc->eps[0].ep; /* gadget ep0 */
INIT_LIST_HEAD(&udc->gadget.ep_list); /* ep_list */
udc->gadget.speed = USB_SPEED_UNKNOWN; /* speed */
- udc->gadget.is_dualspeed = 1; /* support dual speed */
+ udc->gadget.max_speed = USB_SPEED_HIGH; /* support dual speed */
/* the "gadget" abstracts/virtualizes the controller */
dev_set_name(&udc->gadget.dev, "gadget");
diff --git a/drivers/usb/gadget/net2272.c b/drivers/usb/gadget/net2272.c
index ab98ea9..e21ec48 100644
--- a/drivers/usb/gadget/net2272.c
+++ b/drivers/usb/gadget/net2272.c
@@ -2265,7 +2265,7 @@ net2272_probe_init(struct device *dev, unsigned int irq)
ret->irq = irq;
ret->dev = dev;
ret->gadget.ops = &net2272_ops;
- ret->gadget.is_dualspeed = 1;
+ ret->gadget.max_speed = USB_SPEED_HIGH;
/* the "gadget" abstracts/virtualizes the controller */
dev_set_name(&ret->gadget.dev, "gadget");
diff --git a/drivers/usb/gadget/net2280.c b/drivers/usb/gadget/net2280.c
index 3dd40b4..c3220a1 100644
--- a/drivers/usb/gadget/net2280.c
+++ b/drivers/usb/gadget/net2280.c
@@ -2743,7 +2743,7 @@ static int net2280_probe (struct pci_dev *pdev, const struct pci_device_id *id)
spin_lock_init (&dev->lock);
dev->pdev = pdev;
dev->gadget.ops = &net2280_ops;
- dev->gadget.is_dualspeed = 1;
+ dev->gadget.max_speed = USB_SPEED_HIGH;
/* the "gadget" abstracts/virtualizes the controller */
dev_set_name(&dev->gadget.dev, "gadget");
diff --git a/drivers/usb/gadget/omap_udc.c b/drivers/usb/gadget/omap_udc.c
index 740c7da..fff7e1c 100644
--- a/drivers/usb/gadget/omap_udc.c
+++ b/drivers/usb/gadget/omap_udc.c
@@ -2686,6 +2686,7 @@ omap_udc_setup(struct platform_device *odev, struct otg_transceiver *xceiv)
INIT_LIST_HEAD(&udc->gadget.ep_list);
INIT_LIST_HEAD(&udc->iso);
udc->gadget.speed = USB_SPEED_UNKNOWN;
+ udc->gadget.max_speed = USB_SPEED_FULL;
udc->gadget.name = driver_name;
device_initialize(&udc->gadget.dev);
diff --git a/drivers/usb/gadget/pch_udc.c b/drivers/usb/gadget/pch_udc.c
index f96615a..d781540 100644
--- a/drivers/usb/gadget/pch_udc.c
+++ b/drivers/usb/gadget/pch_udc.c
@@ -2949,7 +2949,7 @@ static int pch_udc_probe(struct pci_dev *pdev,
dev->gadget.dev.dma_mask = pdev->dev.dma_mask;
dev->gadget.dev.release = gadget_release;
dev->gadget.name = KBUILD_MODNAME;
- dev->gadget.is_dualspeed = 1;
+ dev->gadget.max_speed = USB_SPEED_HIGH;
retval = device_register(&dev->gadget.dev);
if (retval)
diff --git a/drivers/usb/gadget/printer.c b/drivers/usb/gadget/printer.c
index a341dde..2db4913 100644
--- a/drivers/usb/gadget/printer.c
+++ b/drivers/usb/gadget/printer.c
@@ -1158,7 +1158,7 @@ printer_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
break;
#ifdef CONFIG_USB_GADGET_DUALSPEED
case USB_DT_DEVICE_QUALIFIER:
- if (!gadget->is_dualspeed)
+ if (!gadget_is_dualspeed(gadget))
break;
/*
* assumes ep0 uses the same value for both
@@ -1172,7 +1172,7 @@ printer_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
break;
case USB_DT_OTHER_SPEED_CONFIG:
- if (!gadget->is_dualspeed)
+ if (!gadget_is_dualspeed(gadget))
break;
/* FALLTHROUGH */
#endif /* CONFIG_USB_GADGET_DUALSPEED */
diff --git a/drivers/usb/gadget/r8a66597-udc.c b/drivers/usb/gadget/r8a66597-udc.c
index 50991e5..a8f7a05 100644
--- a/drivers/usb/gadget/r8a66597-udc.c
+++ b/drivers/usb/gadget/r8a66597-udc.c
@@ -1616,7 +1616,7 @@ static int __init r8a66597_probe(struct platform_device *pdev)
r8a66597->gadget.ops = &r8a66597_gadget_ops;
device_initialize(&r8a66597->gadget.dev);
dev_set_name(&r8a66597->gadget.dev, "gadget");
- r8a66597->gadget.is_dualspeed = 1;
+ r8a66597->gadget.max_speed = USB_SPEED_HIGH;
r8a66597->gadget.dev.parent = &pdev->dev;
r8a66597->gadget.dev.dma_mask = pdev->dev.dma_mask;
r8a66597->gadget.dev.release = pdev->dev.release;
diff --git a/drivers/usb/gadget/s3c-hsotg.c b/drivers/usb/gadget/s3c-hsotg.c
index 8bdee67..88dd131 100644
--- a/drivers/usb/gadget/s3c-hsotg.c
+++ b/drivers/usb/gadget/s3c-hsotg.c
@@ -3368,7 +3368,7 @@ static int __devinit s3c_hsotg_probe(struct platform_device *pdev)
dev_set_name(&hsotg->gadget.dev, "gadget");
- hsotg->gadget.is_dualspeed = 1;
+ hsotg->gadget.max_speed = USB_SPEED_HIGH;
hsotg->gadget.ops = &s3c_hsotg_gadget_ops;
hsotg->gadget.name = dev_name(dev);
diff --git a/drivers/usb/gadget/s3c-hsudc.c b/drivers/usb/gadget/s3c-hsudc.c
index 3fa717c..3ac4241 100644
--- a/drivers/usb/gadget/s3c-hsudc.c
+++ b/drivers/usb/gadget/s3c-hsudc.c
@@ -1287,7 +1287,7 @@ static int s3c_hsudc_probe(struct platform_device *pdev)
device_initialize(&hsudc->gadget.dev);
dev_set_name(&hsudc->gadget.dev, "gadget");
- hsudc->gadget.is_dualspeed = 1;
+ hsudc->gadget.max_speed = USB_SPEED_HIGH;
hsudc->gadget.ops = &s3c_hsudc_gadget_ops;
hsudc->gadget.name = dev_name(dev);
hsudc->gadget.dev.parent = dev;
diff --git a/drivers/usb/gadget/udc-core.c b/drivers/usb/gadget/udc-core.c
index e1ecdbc..25058b4 100644
--- a/drivers/usb/gadget/udc-core.c
+++ b/drivers/usb/gadget/udc-core.c
@@ -371,14 +371,28 @@ static ssize_t usb_udc_softconn_store(struct device *dev,
}
static DEVICE_ATTR(soft_connect, S_IWUSR, NULL, usb_udc_softconn_store);
-static ssize_t usb_udc_speed_show(struct device *dev,
+#define USB_UDC_SPEED_ATTR(name) \
+ssize_t usb_udc_##name##_show(struct device *dev, \
+ struct device_attribute *attr, char *buf) \
+{ \
+ struct usb_udc *udc = container_of(dev, struct usb_udc, dev); \
+ return snprintf(buf, PAGE_SIZE, "%s\n", \
+ usb_device_speed_name(udc->gadget->name)); \
+} \
+static DEVICE_ATTR(name, S_IRUSR, usb_udc_##name##_show, NULL)
+
+static USB_UDC_SPEED_ATTR(speed);
+static USB_UDC_SPEED_ATTR(max_speed);
+
+/* Provide "is_dualspeed" for backward compatibility. */
+static ssize_t usb_udc_is_dualspeed_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
- struct usb_udc *udc = container_of(dev, struct usb_udc, dev);
- return snprintf(buf, PAGE_SIZE, "%s\n",
- usb_device_speed_name(udc->gadget->speed));
+ struct usb_udc *udc = container_of(dev, struct usb_udc, dev);
+ return snprintf(buf, PAGE_SIZE, "%d\n",
+ gadget_is_dualspeed(udc->gadget));
}
-static DEVICE_ATTR(speed, S_IRUSR, usb_udc_speed_show, NULL);
+static DEVICE_ATTR(is_dualspeed, S_IRUSR, usb_udc_is_dualspeed_show, NULL);
#define USB_UDC_ATTR(name) \
ssize_t usb_udc_##name##_show(struct device *dev, \
@@ -391,7 +405,6 @@ ssize_t usb_udc_##name##_show(struct device *dev, \
} \
static DEVICE_ATTR(name, S_IRUSR, usb_udc_##name##_show, NULL)
-static USB_UDC_ATTR(is_dualspeed);
static USB_UDC_ATTR(is_otg);
static USB_UDC_ATTR(is_a_peripheral);
static USB_UDC_ATTR(b_hnp_enable);
@@ -402,6 +415,7 @@ static struct attribute *usb_udc_attrs[] = {
&dev_attr_srp.attr,
&dev_attr_soft_connect.attr,
&dev_attr_speed.attr,
+ &dev_attr_max_speed.attr,
&dev_attr_is_dualspeed.attr,
&dev_attr_is_otg.attr,
diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
index 8c41a2e..0b3fb20 100644
--- a/drivers/usb/musb/musb_gadget.c
+++ b/drivers/usb/musb/musb_gadget.c
@@ -1827,7 +1827,7 @@ int __init musb_gadget_setup(struct musb *musb)
*/
musb->g.ops = &musb_gadget_operations;
- musb->g.is_dualspeed = 1;
+ musb->g.max_speed = USB_SPEED_HIGH;
musb->g.speed = USB_SPEED_UNKNOWN;
/* this "gadget" abstracts/virtualizes the controller */
diff --git a/drivers/usb/renesas_usbhs/mod_gadget.c b/drivers/usb/renesas_usbhs/mod_gadget.c
index cb2d451..a8ae54f 100644
--- a/drivers/usb/renesas_usbhs/mod_gadget.c
+++ b/drivers/usb/renesas_usbhs/mod_gadget.c
@@ -917,7 +917,7 @@ int __devinit usbhs_mod_gadget_probe(struct usbhs_priv *priv)
gpriv->gadget.dev.parent = dev;
gpriv->gadget.name = "renesas_usbhs_udc";
gpriv->gadget.ops = &usbhsg_gadget_ops;
- gpriv->gadget.is_dualspeed = 1;
+ gpriv->gadget.max_speed = USB_SPEED_HIGH;
INIT_LIST_HEAD(&gpriv->gadget.ep_list);
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index 087f4b9..7eb2ec3 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -477,8 +477,8 @@ struct usb_gadget_ops {
* driver setup() requests
* @ep_list: List of other endpoints supported by the device.
* @speed: Speed of current connection to USB host.
- * @is_dualspeed: True if the controller supports both high and full speed
- * operation. If it does, the gadget driver must also support both.
+ * @max_speed: Maximal speed the UDC can handle. UDC must support this
+ * and all slower speeds.
* @is_otg: True if the USB device port uses a Mini-AB jack, so that the
* gadget driver must provide a USB OTG descriptor.
* @is_a_peripheral: False unless is_otg, the "A" end of a USB cable
@@ -518,7 +518,7 @@ struct usb_gadget {
struct usb_ep *ep0;
struct list_head ep_list; /* of usb_ep */
enum usb_device_speed speed;
- unsigned is_dualspeed:1;
+ enum usb_device_speed max_speed;
unsigned is_otg:1;
unsigned is_a_peripheral:1;
unsigned b_hnp_enable:1;
@@ -549,7 +549,7 @@ static inline struct usb_gadget *dev_to_usb_gadget(struct device *dev)
static inline int gadget_is_dualspeed(struct usb_gadget *g)
{
#ifdef CONFIG_USB_GADGET_DUALSPEED
- /* runtime test would check "g->is_dualspeed" ... that might be
+ /* runtime test would check "g->max_speed" ... that might be
* useful to work around hardware bugs, but is mostly pointless
*/
return 1;
@@ -567,7 +567,7 @@ static inline int gadget_is_superspeed(struct usb_gadget *g)
{
#ifdef CONFIG_USB_GADGET_SUPERSPEED
/*
- * runtime test would check "g->is_superspeed" ... that might be
+ * runtime test would check "g->max_speed" ... that might be
* useful to work around hardware bugs, but is mostly pointless
*/
return 1;
--
1.7.3.1
^ permalink raw reply related [flat|nested] 70+ messages in thread
* [PATCHv3 3/4] usb: gadget: rename usb_gadget_driver::speed to max_speed
2011-08-19 22:32 ` [PATCHv3 0/4] Figuring out speed refactorisation Michal Nazarewicz
2011-08-19 22:32 ` [PATCHv3 1/4] usb: Provide usb_device_speed_name() function Michal Nazarewicz
2011-08-19 22:33 ` [PATCHv3 2/4] usb: gadget: replace "is_dualspeed" with "max_speed" Michal Nazarewicz
@ 2011-08-19 22:33 ` Michal Nazarewicz
2011-08-19 23:31 ` Felipe Balbi
2011-08-19 22:33 ` [PATCHv3 4/4] usb: gadget: get rid of USB_GADGET_{DUAL,SUPER}SPEED Michal Nazarewicz
3 siblings, 1 reply; 70+ messages in thread
From: Michal Nazarewicz @ 2011-08-19 22:33 UTC (permalink / raw)
To: Alan Stern, Sebastian Andrzej Siewior, Yang Rui Rui, Dave Young
Cc: Felipe Balbi, linux-usb, linux-kernel
From: Michal Nazarewicz <mina86@mina86.com>
This commit renames the “speed” field of the usb_gadget_driver
structure to “max_speed”. This is so that to make it more
apparent that the field represents the maximum speed gadget
driver can support.
This also make the field look more like fields with the same
name in usb_gadget and usb_composite_driver structures. All
of those represent the *maximal* speed given entity supports.
After this commit, there are the following fields in various
structures:
* usb_gadget::speed - the current connection speed,
* usb_gadget::max_speed - maximal speed UDC supports,
* usb_gadget_driver::max_speed - maximal speed gadget driver
supports, and
* usb_composite_driver::max_speed - maximal speed composite
gadget supports.
Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
---
drivers/usb/gadget/amd5536udc.c | 2 +-
drivers/usb/gadget/ci13xxx_udc.c | 2 +-
drivers/usb/gadget/composite.c | 8 ++++----
drivers/usb/gadget/dbgp.c | 2 +-
drivers/usb/gadget/dummy_hcd.c | 13 ++++++-------
drivers/usb/gadget/file_storage.c | 4 ++--
drivers/usb/gadget/gmidi.c | 2 +-
drivers/usb/gadget/goku_udc.c | 2 +-
drivers/usb/gadget/inode.c | 6 +++---
drivers/usb/gadget/m66592-udc.c | 2 +-
drivers/usb/gadget/net2272.c | 2 +-
drivers/usb/gadget/net2280.c | 2 +-
drivers/usb/gadget/pch_udc.c | 2 +-
drivers/usb/gadget/printer.c | 2 +-
drivers/usb/gadget/r8a66597-udc.c | 2 +-
include/linux/usb/gadget.h | 4 ++--
16 files changed, 28 insertions(+), 29 deletions(-)
diff --git a/drivers/usb/gadget/amd5536udc.c b/drivers/usb/gadget/amd5536udc.c
index 80ae808..7b48ed2 100644
--- a/drivers/usb/gadget/amd5536udc.c
+++ b/drivers/usb/gadget/amd5536udc.c
@@ -1968,7 +1968,7 @@ static int amd5536_start(struct usb_gadget_driver *driver,
u32 tmp;
if (!driver || !bind || !driver->setup
- || driver->speed != USB_SPEED_HIGH)
+ || driver->max_speed != USB_SPEED_HIGH)
return -EINVAL;
if (!dev)
return -ENODEV;
diff --git a/drivers/usb/gadget/ci13xxx_udc.c b/drivers/usb/gadget/ci13xxx_udc.c
index b877464..6c13174 100644
--- a/drivers/usb/gadget/ci13xxx_udc.c
+++ b/drivers/usb/gadget/ci13xxx_udc.c
@@ -798,7 +798,7 @@ static ssize_t show_driver(struct device *dev, struct device_attribute *attr,
n += scnprintf(buf + n, PAGE_SIZE - n, "function = %s\n",
(driver->function ? driver->function : ""));
n += scnprintf(buf + n, PAGE_SIZE - n, "max speed = %d\n",
- driver->speed);
+ driver->max_speed);
return n;
}
diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index 5a3461e..851b5da 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -1560,9 +1560,9 @@ composite_resume(struct usb_gadget *gadget)
static struct usb_gadget_driver composite_driver = {
#ifdef CONFIG_USB_GADGET_SUPERSPEED
- .speed = USB_SPEED_SUPER,
+ .max_speed = USB_SPEED_SUPER,
#else
- .speed = USB_SPEED_HIGH,
+ .max_speed = USB_SPEED_HIGH,
#endif
.unbind = composite_unbind,
@@ -1609,8 +1609,8 @@ int usb_composite_probe(struct usb_composite_driver *driver,
driver->iProduct = driver->name;
composite_driver.function = (char *) driver->name;
composite_driver.driver.name = driver->name;
- composite_driver.speed = min((u8)composite_driver.speed,
- (u8)driver->max_speed);
+ composite_driver.max_speed = min((u8)composite_driver.max_speed,
+ (u8)driver->max_speed);
composite = driver;
composite_gadget_bind = bind;
diff --git a/drivers/usb/gadget/dbgp.c b/drivers/usb/gadget/dbgp.c
index e8737b5..4e6f8a2 100644
--- a/drivers/usb/gadget/dbgp.c
+++ b/drivers/usb/gadget/dbgp.c
@@ -405,7 +405,7 @@ fail:
static struct usb_gadget_driver dbgp_driver = {
.function = "dbgp",
- .speed = USB_SPEED_HIGH,
+ .max_speed = USB_SPEED_HIGH,
.unbind = dbgp_unbind,
.setup = dbgp_setup,
.disconnect = dbgp_disconnect,
diff --git a/drivers/usb/gadget/dummy_hcd.c b/drivers/usb/gadget/dummy_hcd.c
index fa60e9b..ccbe5ef 100644
--- a/drivers/usb/gadget/dummy_hcd.c
+++ b/drivers/usb/gadget/dummy_hcd.c
@@ -832,19 +832,18 @@ static int dummy_pullup (struct usb_gadget *_gadget, int value)
if (value && dum->driver) {
if (mod_data.is_super_speed)
- dum->gadget.speed = dum->driver->speed;
+ dum->gadget.speed = dum->driver->max_speed;
else if (mod_data.is_high_speed)
dum->gadget.speed = min_t(u8, USB_SPEED_HIGH,
- dum->driver->speed);
+ dum->driver->max_speed);
else
dum->gadget.speed = USB_SPEED_FULL;
dummy_udc_udpate_ep0(dum);
- if (dum->gadget.speed < dum->driver->speed)
+ if (dum->gadget.speed < dum->driver->max_speed)
dev_dbg(udc_dev(dum), "This device can perform faster"
- " if you connect it to a %s port...\n",
- (dum->driver->speed == USB_SPEED_SUPER ?
- "SuperSpeed" : "HighSpeed"));
+ " if you connect it to a %s port...\n",
+ usb_device_speed_name(dum->driver->max_speed));
}
dum_hcd = gadget_to_dummy_hcd(_gadget);
@@ -907,7 +906,7 @@ static int dummy_udc_start(struct usb_gadget *g,
struct dummy_hcd *dum_hcd = gadget_to_dummy_hcd(g);
struct dummy *dum = dum_hcd->dum;
- if (driver->speed == USB_SPEED_UNKNOWN)
+ if (driver->max_speed == USB_SPEED_UNKNOWN)
return -EINVAL;
/*
diff --git a/drivers/usb/gadget/file_storage.c b/drivers/usb/gadget/file_storage.c
index cf875be..3cb083b 100644
--- a/drivers/usb/gadget/file_storage.c
+++ b/drivers/usb/gadget/file_storage.c
@@ -3563,9 +3563,9 @@ static void fsg_resume(struct usb_gadget *gadget)
static struct usb_gadget_driver fsg_driver = {
#ifdef CONFIG_USB_GADGET_DUALSPEED
- .speed = USB_SPEED_HIGH,
+ .max_speed = USB_SPEED_HIGH,
#else
- .speed = USB_SPEED_FULL,
+ .max_speed = USB_SPEED_FULL,
#endif
.function = (char *) fsg_string_product,
.unbind = fsg_unbind,
diff --git a/drivers/usb/gadget/gmidi.c b/drivers/usb/gadget/gmidi.c
index 305a8a8..23cab05 100644
--- a/drivers/usb/gadget/gmidi.c
+++ b/drivers/usb/gadget/gmidi.c
@@ -1292,7 +1292,7 @@ static void gmidi_resume(struct usb_gadget *gadget)
static struct usb_gadget_driver gmidi_driver = {
- .speed = USB_SPEED_FULL,
+ .max_speed = USB_SPEED_FULL,
.function = (char *)longname,
.unbind = gmidi_unbind,
diff --git a/drivers/usb/gadget/goku_udc.c b/drivers/usb/gadget/goku_udc.c
index ab9c924..5af70fcc 100644
--- a/drivers/usb/gadget/goku_udc.c
+++ b/drivers/usb/gadget/goku_udc.c
@@ -1357,7 +1357,7 @@ static int goku_start(struct usb_gadget_driver *driver,
int retval;
if (!driver
- || driver->speed < USB_SPEED_FULL
+ || driver->max_speed < USB_SPEED_FULL
|| !bind
|| !driver->disconnect
|| !driver->setup)
diff --git a/drivers/usb/gadget/inode.c b/drivers/usb/gadget/inode.c
index 1b24099..4bd48ea 100644
--- a/drivers/usb/gadget/inode.c
+++ b/drivers/usb/gadget/inode.c
@@ -1774,9 +1774,9 @@ gadgetfs_suspend (struct usb_gadget *gadget)
static struct usb_gadget_driver gadgetfs_driver = {
#ifdef CONFIG_USB_GADGET_DUALSPEED
- .speed = USB_SPEED_HIGH,
+ .max_speed = USB_SPEED_HIGH,
#else
- .speed = USB_SPEED_FULL,
+ .max_speed = USB_SPEED_FULL,
#endif
.function = (char *) driver_desc,
.unbind = gadgetfs_unbind,
@@ -1800,7 +1800,7 @@ static int gadgetfs_probe (struct usb_gadget *gadget)
}
static struct usb_gadget_driver probe_driver = {
- .speed = USB_SPEED_HIGH,
+ .max_speed = USB_SPEED_HIGH,
.unbind = gadgetfs_nop,
.setup = (void *)gadgetfs_nop,
.disconnect = gadgetfs_nop,
diff --git a/drivers/usb/gadget/m66592-udc.c b/drivers/usb/gadget/m66592-udc.c
index fbdfca8..72dc3f1 100644
--- a/drivers/usb/gadget/m66592-udc.c
+++ b/drivers/usb/gadget/m66592-udc.c
@@ -1482,7 +1482,7 @@ static int m66592_start(struct usb_gadget_driver *driver,
int retval;
if (!driver
- || driver->speed != USB_SPEED_HIGH
+ || driver->max_speed != USB_SPEED_HIGH
|| !bind
|| !driver->setup)
return -EINVAL;
diff --git a/drivers/usb/gadget/net2272.c b/drivers/usb/gadget/net2272.c
index e21ec48..c6c4934 100644
--- a/drivers/usb/gadget/net2272.c
+++ b/drivers/usb/gadget/net2272.c
@@ -1461,7 +1461,7 @@ static int net2272_start(struct usb_gadget_driver *driver,
unsigned i;
if (!driver || !bind || !driver->unbind || !driver->setup ||
- driver->speed != USB_SPEED_HIGH)
+ driver->max_speed != USB_SPEED_HIGH)
return -EINVAL;
if (!dev)
return -ENODEV;
diff --git a/drivers/usb/gadget/net2280.c b/drivers/usb/gadget/net2280.c
index c3220a1..1eb4774 100644
--- a/drivers/usb/gadget/net2280.c
+++ b/drivers/usb/gadget/net2280.c
@@ -1892,7 +1892,7 @@ static int net2280_start(struct usb_gadget_driver *driver,
* "must not be used in normal operation"
*/
if (!driver
- || driver->speed != USB_SPEED_HIGH
+ || driver->max_speed != USB_SPEED_HIGH
|| !bind || !driver->setup)
return -EINVAL;
if (!dev)
diff --git a/drivers/usb/gadget/pch_udc.c b/drivers/usb/gadget/pch_udc.c
index d781540..1db6d0e 100644
--- a/drivers/usb/gadget/pch_udc.c
+++ b/drivers/usb/gadget/pch_udc.c
@@ -2701,7 +2701,7 @@ static int pch_udc_start(struct usb_gadget_driver *driver,
struct pch_udc_dev *dev = pch_udc;
int retval;
- if (!driver || (driver->speed == USB_SPEED_UNKNOWN) || !bind ||
+ if (!driver || (driver->max_speed == USB_SPEED_UNKNOWN) || !bind ||
!driver->setup || !driver->unbind || !driver->disconnect) {
dev_err(&dev->pdev->dev,
"%s: invalid driver parameter\n", __func__);
diff --git a/drivers/usb/gadget/printer.c b/drivers/usb/gadget/printer.c
index 2db4913..104a18a 100644
--- a/drivers/usb/gadget/printer.c
+++ b/drivers/usb/gadget/printer.c
@@ -1552,7 +1552,7 @@ fail:
/*-------------------------------------------------------------------------*/
static struct usb_gadget_driver printer_driver = {
- .speed = DEVSPEED,
+ .max_speed = DEVSPEED,
.function = (char *) driver_desc,
.unbind = printer_unbind,
diff --git a/drivers/usb/gadget/r8a66597-udc.c b/drivers/usb/gadget/r8a66597-udc.c
index a8f7a05..df99690 100644
--- a/drivers/usb/gadget/r8a66597-udc.c
+++ b/drivers/usb/gadget/r8a66597-udc.c
@@ -1442,7 +1442,7 @@ static int r8a66597_start(struct usb_gadget_driver *driver,
int retval;
if (!driver
- || driver->speed != USB_SPEED_HIGH
+ || driver->max_speed != USB_SPEED_HIGH
|| !bind
|| !driver->setup)
return -EINVAL;
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index 7eb2ec3..c8ea048 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -760,7 +760,7 @@ static inline int usb_gadget_disconnect(struct usb_gadget *gadget)
/**
* struct usb_gadget_driver - driver for usb 'slave' devices
* @function: String describing the gadget's function
- * @speed: Highest speed the driver handles.
+ * @max_speed: Highest speed the driver handles.
* @setup: Invoked for ep0 control requests that aren't handled by
* the hardware level driver. Most calls must be handled by
* the gadget driver, including descriptor and configuration
@@ -824,7 +824,7 @@ static inline int usb_gadget_disconnect(struct usb_gadget *gadget)
*/
struct usb_gadget_driver {
char *function;
- enum usb_device_speed speed;
+ enum usb_device_speed max_speed;
void (*unbind)(struct usb_gadget *);
int (*setup)(struct usb_gadget *,
const struct usb_ctrlrequest *);
--
1.7.3.1
^ permalink raw reply related [flat|nested] 70+ messages in thread
* [PATCHv3 4/4] usb: gadget: get rid of USB_GADGET_{DUAL,SUPER}SPEED
2011-08-19 22:32 ` [PATCHv3 0/4] Figuring out speed refactorisation Michal Nazarewicz
` (2 preceding siblings ...)
2011-08-19 22:33 ` [PATCHv3 3/4] usb: gadget: rename usb_gadget_driver::speed to max_speed Michal Nazarewicz
@ 2011-08-19 22:33 ` Michal Nazarewicz
2011-08-20 13:41 ` Alan Stern
3 siblings, 1 reply; 70+ messages in thread
From: Michal Nazarewicz @ 2011-08-19 22:33 UTC (permalink / raw)
To: Alan Stern, Sebastian Andrzej Siewior, Yang Rui Rui, Dave Young
Cc: Felipe Balbi, linux-usb, linux-kernel
From: Michal Nazarewicz <mina86@mina86.com>
This commit removes USB_GADGET_DUALSPEED and USB_GADGET_SUPERSPEED
Kconfig options. Since now kernel allows many UDC drivers to be
compiled, those options may turn to no longer be valid. For
instance, if someone decides to build UDC that supports super
speed and UDC that supports high speed only, the latter will be
"assumed" to support super speed since USB_GADGET_SUPERSPEED will
be selected by the former.
The test of whether CONFIG_USB_GADGET_*SPEED was defined was just
an optimisation which removed otherwise dead code (ie. if UDC is
not dual speed, there is no need to handle cases that can happen
if speed is high). This commit removes those checks.
Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
---
drivers/usb/gadget/Kconfig | 30 ----------------------------
drivers/usb/gadget/composite.c | 9 +-------
drivers/usb/gadget/dummy_hcd.c | 2 +-
drivers/usb/gadget/file_storage.c | 4 ---
drivers/usb/gadget/inode.c | 10 ---------
drivers/usb/gadget/printer.c | 39 ++++--------------------------------
drivers/usb/gadget/u_ether.c | 7 ------
include/linux/usb/gadget.h | 19 +----------------
8 files changed, 9 insertions(+), 111 deletions(-)
diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
index 5a084b9..c60c167 100644
--- a/drivers/usb/gadget/Kconfig
+++ b/drivers/usb/gadget/Kconfig
@@ -133,7 +133,6 @@ config USB_AT91
config USB_ATMEL_USBA
tristate "Atmel USBA"
- select USB_GADGET_DUALSPEED
depends on AVR32 || ARCH_AT91CAP9 || ARCH_AT91SAM9RL || ARCH_AT91SAM9G45
help
USBA is the integrated high-speed USB Device controller on
@@ -142,7 +141,6 @@ config USB_ATMEL_USBA
config USB_FSL_USB2
tristate "Freescale Highspeed USB DR Peripheral Controller"
depends on FSL_SOC || ARCH_MXC
- select USB_GADGET_DUALSPEED
select USB_FSL_MPH_DR_OF if OF
help
Some of Freescale PowerPC processors have a High Speed
@@ -158,7 +156,6 @@ config USB_FSL_USB2
config USB_FUSB300
tristate "Faraday FUSB300 USB Peripheral Controller"
depends on !PHYS_ADDR_T_64BIT
- select USB_GADGET_DUALSPEED
help
Faraday usb device controller FUSB300 driver
@@ -206,7 +203,6 @@ config USB_PXA25X_SMALL
config USB_R8A66597
tristate "Renesas R8A66597 USB Peripheral Controller"
- select USB_GADGET_DUALSPEED
help
R8A66597 is a discrete USB host and peripheral controller chip that
supports both full and high speed USB 2.0 data transfers.
@@ -220,7 +216,6 @@ config USB_RENESAS_USBHS_UDC
tristate 'Renesas USBHS controller'
depends on SUPERH || ARCH_SHMOBILE
depends on USB_RENESAS_USBHS
- select USB_GADGET_DUALSPEED
help
Renesas USBHS is a discrete USB host and peripheral controller chip
that supports both full and high speed USB 2.0 data transfers.
@@ -249,7 +244,6 @@ config USB_S3C_HSOTG
tristate "S3C HS/OtG USB Device controller"
depends on S3C_DEV_USB_HSOTG
select USB_GADGET_S3C_HSOTG_PIO
- select USB_GADGET_DUALSPEED
help
The Samsung S3C64XX USB2.0 high-speed gadget controller
integrated into the S3C64XX series SoC.
@@ -287,7 +281,6 @@ config USB_S3C2410_DEBUG
config USB_S3C_HSUDC
tristate "S3C2416, S3C2443 and S3C2450 USB Device Controller"
depends on ARCH_S3C2410
- select USB_GADGET_DUALSPEED
help
Samsung's S3C2416, S3C2443 and S3C2450 is an ARM9 based SoC
integrated with dual speed USB 2.0 device controller. It has
@@ -298,7 +291,6 @@ config USB_S3C_HSUDC
config USB_PXA_U2O
tristate "PXA9xx Processor USB2.0 controller"
depends on ARCH_MMP
- select USB_GADGET_DUALSPEED
help
PXA9xx Processor series include a high speed USB2.0 device
controller, which support high speed and full speed USB peripheral.
@@ -311,14 +303,12 @@ config USB_PXA_U2O
config USB_GADGET_MUSB_HDRC
tristate "Inventra HDRC USB Peripheral (TI, ADI, ...)"
depends on USB_MUSB_HDRC
- select USB_GADGET_DUALSPEED
help
This OTG-capable silicon IP is used in dual designs including
the TI DaVinci, OMAP 243x, OMAP 343x, TUSB 6010, and ADI Blackfin
config USB_M66592
tristate "Renesas M66592 USB Peripheral Controller"
- select USB_GADGET_DUALSPEED
help
M66592 is a discrete USB peripheral controller chip that
supports both full and high speed USB 2.0 data transfers.
@@ -335,7 +325,6 @@ config USB_M66592
config USB_AMD5536UDC
tristate "AMD5536 UDC"
depends on PCI
- select USB_GADGET_DUALSPEED
help
The AMD5536 UDC is part of the AMD Geode CS5536, an x86 southbridge.
It is a USB Highspeed DMA capable USB device controller. Beside ep0
@@ -363,7 +352,6 @@ config USB_FSL_QE
config USB_CI13XXX_PCI
tristate "MIPS USB CI13xxx PCI UDC"
depends on PCI
- select USB_GADGET_DUALSPEED
help
MIPS USB IP core family device controller
Currently it only supports IP part number CI13412
@@ -374,7 +362,6 @@ config USB_CI13XXX_PCI
config USB_NET2272
tristate "PLX NET2272"
- select USB_GADGET_DUALSPEED
help
PLX NET2272 is a USB peripheral controller which supports
both full and high speed USB 2.0 data transfers.
@@ -398,7 +385,6 @@ config USB_NET2272_DMA
config USB_NET2280
tristate "NetChip 228x"
depends on PCI
- select USB_GADGET_DUALSPEED
help
NetChip 2280 / 2282 is a PCI based USB peripheral controller which
supports both full and high speed USB 2.0 data transfers.
@@ -429,7 +415,6 @@ config USB_LANGWELL
tristate "Intel Langwell USB Device Controller"
depends on PCI
depends on !PHYS_ADDR_T_64BIT
- select USB_GADGET_DUALSPEED
help
Intel Langwell USB Device Controller is a High-Speed USB
On-The-Go device controller.
@@ -444,7 +429,6 @@ config USB_LANGWELL
config USB_EG20T
tristate "Intel EG20T PCH/OKI SEMICONDUCTOR ML7213 IOH UDC"
depends on PCI
- select USB_GADGET_DUALSPEED
help
This is a USB device driver for EG20T PCH.
EG20T PCH is the platform controller hub that is used in Intel's
@@ -466,7 +450,6 @@ config USB_EG20T
config USB_CI13XXX_MSM
tristate "MIPS USB CI13xxx for MSM"
depends on ARCH_MSM
- select USB_GADGET_DUALSPEED
select USB_MSM_OTG
help
MSM SoC has chipidea USB controller. This driver uses
@@ -487,8 +470,6 @@ config USB_CI13XXX_MSM
config USB_DUMMY_HCD
tristate "Dummy HCD (DEVELOPMENT)"
depends on USB=y || (USB=m && USB_GADGET=m)
- select USB_GADGET_DUALSPEED
- select USB_GADGET_SUPERSPEED
help
This host controller driver emulates USB, looping all data transfer
requests back to a USB "gadget driver" in the same host. The host
@@ -513,17 +494,6 @@ config USB_DUMMY_HCD
endchoice
-# Selected by UDC drivers that support high-speed operation.
-config USB_GADGET_DUALSPEED
- bool
- depends on USB_GADGET
-
-# Selected by UDC drivers that support super-speed opperation
-config USB_GADGET_SUPERSPEED
- bool
- depends on USB_GADGET
- depends on USB_GADGET_DUALSPEED
-
#
# USB Gadget Drivers
#
diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index 851b5da..c08edd9 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -1559,12 +1559,6 @@ composite_resume(struct usb_gadget *gadget)
/*-------------------------------------------------------------------------*/
static struct usb_gadget_driver composite_driver = {
-#ifdef CONFIG_USB_GADGET_SUPERSPEED
- .max_speed = USB_SPEED_SUPER,
-#else
- .max_speed = USB_SPEED_HIGH,
-#endif
-
.unbind = composite_unbind,
.setup = composite_setup,
@@ -1609,8 +1603,7 @@ int usb_composite_probe(struct usb_composite_driver *driver,
driver->iProduct = driver->name;
composite_driver.function = (char *) driver->name;
composite_driver.driver.name = driver->name;
- composite_driver.max_speed = min((u8)composite_driver.max_speed,
- (u8)driver->max_speed);
+ composite_driver.max_speed = driver->max_speed;
composite = driver;
composite_gadget_bind = bind;
diff --git a/drivers/usb/gadget/dummy_hcd.c b/drivers/usb/gadget/dummy_hcd.c
index ccbe5ef..d3ce66c 100644
--- a/drivers/usb/gadget/dummy_hcd.c
+++ b/drivers/usb/gadget/dummy_hcd.c
@@ -985,7 +985,7 @@ static int dummy_udc_probe (struct platform_device *pdev)
dum->gadget.name = gadget_name;
dum->gadget.ops = &dummy_ops;
- dum->gadget.max_speed = USB_SPEED_SUPER;;
+ dum->gadget.max_speed = USB_SPEED_SUPER;
dev_set_name(&dum->gadget.dev, "gadget");
dum->gadget.dev.parent = &pdev->dev;
diff --git a/drivers/usb/gadget/file_storage.c b/drivers/usb/gadget/file_storage.c
index 3cb083b..0b94981 100644
--- a/drivers/usb/gadget/file_storage.c
+++ b/drivers/usb/gadget/file_storage.c
@@ -3562,11 +3562,7 @@ static void fsg_resume(struct usb_gadget *gadget)
/*-------------------------------------------------------------------------*/
static struct usb_gadget_driver fsg_driver = {
-#ifdef CONFIG_USB_GADGET_DUALSPEED
.max_speed = USB_SPEED_HIGH,
-#else
- .max_speed = USB_SPEED_FULL,
-#endif
.function = (char *) fsg_string_product,
.unbind = fsg_unbind,
.disconnect = fsg_disconnect,
diff --git a/drivers/usb/gadget/inode.c b/drivers/usb/gadget/inode.c
index 4bd48ea..23c75d8 100644
--- a/drivers/usb/gadget/inode.c
+++ b/drivers/usb/gadget/inode.c
@@ -837,7 +837,6 @@ ep_config (struct file *fd, const char __user *buf, size_t len, loff_t *ptr)
if (value == 0)
data->state = STATE_EP_ENABLED;
break;
-#ifdef CONFIG_USB_GADGET_DUALSPEED
case USB_SPEED_HIGH:
/* fails if caller didn't provide that descriptor... */
ep->desc = &data->hs_desc;
@@ -845,7 +844,6 @@ ep_config (struct file *fd, const char __user *buf, size_t len, loff_t *ptr)
if (value == 0)
data->state = STATE_EP_ENABLED;
break;
-#endif
default:
DBG(data->dev, "unconnected, %s init abandoned\n",
data->name);
@@ -1331,7 +1329,6 @@ static const struct file_operations ep0_io_operations = {
* Unrecognized ep0 requests may be handled in user space.
*/
-#ifdef CONFIG_USB_GADGET_DUALSPEED
static void make_qualifier (struct dev_data *dev)
{
struct usb_qualifier_descriptor qual;
@@ -1354,7 +1351,6 @@ static void make_qualifier (struct dev_data *dev)
memcpy (dev->rbuf, &qual, sizeof qual);
}
-#endif
static int
config_buf (struct dev_data *dev, u8 type, unsigned index)
@@ -1434,7 +1430,6 @@ gadgetfs_setup (struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
dev->dev->bMaxPacketSize0 = dev->gadget->ep0->maxpacket;
req->buf = dev->dev;
break;
-#ifdef CONFIG_USB_GADGET_DUALSPEED
case USB_DT_DEVICE_QUALIFIER:
if (!dev->hs_config)
break;
@@ -1444,7 +1439,6 @@ gadgetfs_setup (struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
break;
case USB_DT_OTHER_SPEED_CONFIG:
// FALLTHROUGH
-#endif
case USB_DT_CONFIG:
value = config_buf (dev,
w_value >> 8,
@@ -1773,11 +1767,7 @@ gadgetfs_suspend (struct usb_gadget *gadget)
}
static struct usb_gadget_driver gadgetfs_driver = {
-#ifdef CONFIG_USB_GADGET_DUALSPEED
.max_speed = USB_SPEED_HIGH,
-#else
- .max_speed = USB_SPEED_FULL,
-#endif
.function = (char *) driver_desc,
.unbind = gadgetfs_unbind,
.setup = gadgetfs_setup,
diff --git a/drivers/usb/gadget/printer.c b/drivers/usb/gadget/printer.c
index 104a18a..93f0b94 100644
--- a/drivers/usb/gadget/printer.c
+++ b/drivers/usb/gadget/printer.c
@@ -164,12 +164,6 @@ module_param(qlen, uint, S_IRUGO|S_IWUSR);
#define QLEN qlen
-#ifdef CONFIG_USB_GADGET_DUALSPEED
-#define DEVSPEED USB_SPEED_HIGH
-#else /* full speed (low speed doesn't do bulk) */
-#define DEVSPEED USB_SPEED_FULL
-#endif
-
/*-------------------------------------------------------------------------*/
#define xprintk(d, level, fmt, args...) \
@@ -288,8 +282,6 @@ static const struct usb_descriptor_header *fs_printer_function [11] = {
NULL
};
-#ifdef CONFIG_USB_GADGET_DUALSPEED
-
/*
* usb 2.0 devices need to expose both high speed and full speed
* descriptors, unless they only run at full speed.
@@ -328,13 +320,6 @@ static const struct usb_descriptor_header *hs_printer_function [11] = {
/* maxpacket and other transfer characteristics vary by speed. */
#define ep_desc(g, hs, fs) (((g)->speed == USB_SPEED_HIGH)?(hs):(fs))
-#else
-
-/* if there's no high speed support, maxpacket doesn't change. */
-#define ep_desc(g, hs, fs) (((void)(g)), (fs))
-
-#endif /* !CONFIG_USB_GADGET_DUALSPEED */
-
/*-------------------------------------------------------------------------*/
/* descriptors that are built on-demand */
@@ -979,9 +964,7 @@ printer_set_config(struct printer_dev *dev, unsigned number)
switch (gadget->speed) {
case USB_SPEED_FULL: speed = "full"; break;
-#ifdef CONFIG_USB_GADGET_DUALSPEED
case USB_SPEED_HIGH: speed = "high"; break;
-#endif
default: speed = "?"; break;
}
@@ -998,23 +981,15 @@ config_buf(enum usb_device_speed speed, u8 *buf, u8 type, unsigned index,
{
int len;
const struct usb_descriptor_header **function;
-#ifdef CONFIG_USB_GADGET_DUALSPEED
int hs = (speed == USB_SPEED_HIGH);
+ if (index >= device_desc.bNumConfigurations)
+ return -EINVAL;
+
if (type == USB_DT_OTHER_SPEED_CONFIG)
hs = !hs;
- if (hs) {
- function = hs_printer_function;
- } else {
- function = fs_printer_function;
- }
-#else
- function = fs_printer_function;
-#endif
-
- if (index >= device_desc.bNumConfigurations)
- return -EINVAL;
+ function = hs ? hs_printer_function : fs_printer_function;
/* for now, don't advertise srp-only devices */
if (!is_otg)
@@ -1156,7 +1131,6 @@ printer_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
value = min(wLength, (u16) sizeof device_desc);
memcpy(req->buf, &device_desc, value);
break;
-#ifdef CONFIG_USB_GADGET_DUALSPEED
case USB_DT_DEVICE_QUALIFIER:
if (!gadget_is_dualspeed(gadget))
break;
@@ -1175,7 +1149,6 @@ printer_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
if (!gadget_is_dualspeed(gadget))
break;
/* FALLTHROUGH */
-#endif /* CONFIG_USB_GADGET_DUALSPEED */
case USB_DT_CONFIG:
value = config_buf(gadget->speed, req->buf,
wValue >> 8,
@@ -1460,11 +1433,9 @@ autoconf_fail:
goto autoconf_fail;
out_ep->driver_data = out_ep; /* claim */
-#ifdef CONFIG_USB_GADGET_DUALSPEED
/* assumes that all endpoints are dual-speed */
hs_ep_in_desc.bEndpointAddress = fs_ep_in_desc.bEndpointAddress;
hs_ep_out_desc.bEndpointAddress = fs_ep_out_desc.bEndpointAddress;
-#endif /* DUALSPEED */
usb_gadget_set_selfpowered(gadget);
@@ -1552,7 +1523,7 @@ fail:
/*-------------------------------------------------------------------------*/
static struct usb_gadget_driver printer_driver = {
- .max_speed = DEVSPEED,
+ .max_speed = USB_SPEED_HIGH,
.function = (char *) driver_desc,
.unbind = printer_unbind,
diff --git a/drivers/usb/gadget/u_ether.c b/drivers/usb/gadget/u_ether.c
index dfed4c1..8642abb 100644
--- a/drivers/usb/gadget/u_ether.c
+++ b/drivers/usb/gadget/u_ether.c
@@ -92,17 +92,10 @@ struct eth_dev {
#define DEFAULT_QLEN 2 /* double buffering by default */
-
-#ifdef CONFIG_USB_GADGET_DUALSPEED
-
static unsigned qmult = 5;
module_param(qmult, uint, S_IRUGO|S_IWUSR);
MODULE_PARM_DESC(qmult, "queue length multiplier at high/super speed");
-#else /* full speed (low speed doesn't do bulk) */
-#define qmult 1
-#endif
-
/* for dual-speed hardware, use deeper queues at high/super speed */
static inline int qlen(struct usb_gadget *gadget)
{
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index c8ea048..22404cf 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -548,14 +548,7 @@ static inline struct usb_gadget *dev_to_usb_gadget(struct device *dev)
*/
static inline int gadget_is_dualspeed(struct usb_gadget *g)
{
-#ifdef CONFIG_USB_GADGET_DUALSPEED
- /* runtime test would check "g->max_speed" ... that might be
- * useful to work around hardware bugs, but is mostly pointless
- */
- return 1;
-#else
- return 0;
-#endif
+ return g->max_speed >= USB_SPEED_HIGH;
}
/**
@@ -565,15 +558,7 @@ static inline int gadget_is_dualspeed(struct usb_gadget *g)
*/
static inline int gadget_is_superspeed(struct usb_gadget *g)
{
-#ifdef CONFIG_USB_GADGET_SUPERSPEED
- /*
- * runtime test would check "g->max_speed" ... that might be
- * useful to work around hardware bugs, but is mostly pointless
- */
- return 1;
-#else
- return 0;
-#endif
+ return g->max_speed >= USB_SPEED_SUPER;
}
/**
--
1.7.3.1
^ permalink raw reply related [flat|nested] 70+ messages in thread
* Re: [PATCHv3 1/4] usb: Provide usb_device_speed_name() function
2011-08-19 22:32 ` [PATCHv3 1/4] usb: Provide usb_device_speed_name() function Michal Nazarewicz
@ 2011-08-19 23:15 ` Felipe Balbi
2011-08-22 14:53 ` Michal Nazarewicz
0 siblings, 1 reply; 70+ messages in thread
From: Felipe Balbi @ 2011-08-19 23:15 UTC (permalink / raw)
To: Michal Nazarewicz
Cc: Alan Stern, Sebastian Andrzej Siewior, Yang Rui Rui, Dave Young,
Felipe Balbi, linux-usb, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 3646 bytes --]
Hi,
On Sat, Aug 20, 2011 at 12:32:59AM +0200, Michal Nazarewicz wrote:
> From: Michal Nazarewicz <mina86@mina86.com>
>
> In a few places kernel wants to print a human-readable USB
> device speed name (eg. "high") instead of a raw number
> (eg. 3). Usually a switch is introduced in those places
> leading to code repetition. To mitigate this issue, this
> commit introduces usb_device_speed_name() function, which
> returns a human-readable name of provided speed.
>
> Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
> ---
> drivers/usb/Makefile | 3 +++
> drivers/usb/common.c | 24 ++++++++++++++++++++++++
> drivers/usb/gadget/udc-core.c | 19 ++-----------------
> include/linux/usb/ch9.h | 8 ++++++++
> 4 files changed, 37 insertions(+), 17 deletions(-)
> create mode 100644 drivers/usb/common.c
>
> diff --git a/drivers/usb/Makefile b/drivers/usb/Makefile
> index 30ddf8d..a3167cc 100644
> --- a/drivers/usb/Makefile
> +++ b/drivers/usb/Makefile
> @@ -51,3 +51,6 @@ obj-$(CONFIG_USB_MUSB_HDRC) += musb/
> obj-$(CONFIG_USB_RENESAS_USBHS) += renesas_usbhs/
> obj-$(CONFIG_USB_OTG_UTILS) += otg/
> obj-$(CONFIG_USB_GADGET) += gadget/
> +
> +obj-$(CONFIG_USB) += common.o
> +obj-$(CONFIG_USB_GADGET) += common.o
> diff --git a/drivers/usb/common.c b/drivers/usb/common.c
> new file mode 100644
> index 0000000..e9d7141
> --- /dev/null
> +++ b/drivers/usb/common.c
> @@ -0,0 +1,24 @@
> +/*
> + * Provides code common for host and device side USB.
> + */
> +
> +#include <linux/kernel.h> /* for ARRAY_SIZE() */
> +#include <linux/module.h> /* for EXPORT_SYMBOL_GPL() */
> +#include <linux/usb/ch9.h>
> +
> +const char *usb_device_speed_name(enum usb_device_speed speed)
somehow usb_speed_string() sounds better to me.
> +{
> + static const char *const names[] = {
> + "UNKNOWN",
> + "low-speed",
> + "full-speed",
> + "high-speed",
> + "wireless",
> + "super-speed",
> + };
> +
> + if (speed < 0 || speed >= ARRAY_SIZE(names))
> + speed = USB_SPEED_UNKNOWN;
I generally dislike depending on a particular ordering of enumerations.
While's it's highly unlikely that anyone will change that, if someone
does change, then it'll take a long time to actually find the bug on the
print here :-)
I woud rather use a switch in this case.
> diff --git a/drivers/usb/gadget/udc-core.c b/drivers/usb/gadget/udc-core.c
> index 05ba472..e1ecdbc 100644
> --- a/drivers/usb/gadget/udc-core.c
> +++ b/drivers/usb/gadget/udc-core.c
> @@ -375,23 +375,8 @@ static ssize_t usb_udc_speed_show(struct device *dev,
> struct device_attribute *attr, char *buf)
> {
> struct usb_udc *udc = container_of(dev, struct usb_udc, dev);
> - struct usb_gadget *gadget = udc->gadget;
> -
> - switch (gadget->speed) {
> - case USB_SPEED_LOW:
> - return snprintf(buf, PAGE_SIZE, "low-speed\n");
> - case USB_SPEED_FULL:
> - return snprintf(buf, PAGE_SIZE, "full-speed\n");
> - case USB_SPEED_HIGH:
> - return snprintf(buf, PAGE_SIZE, "high-speed\n");
> - case USB_SPEED_WIRELESS:
> - return snprintf(buf, PAGE_SIZE, "wireless\n");
> - case USB_SPEED_SUPER:
> - return snprintf(buf, PAGE_SIZE, "super-speed\n");
> - case USB_SPEED_UNKNOWN: /* FALLTHROUGH */
> - default:
> - return snprintf(buf, PAGE_SIZE, "UNKNOWN\n");
> - }
> + return snprintf(buf, PAGE_SIZE, "%s\n",
> + usb_device_speed_name(udc->gadget->speed));
> }
> static DEVICE_ATTR(speed, S_IRUSR, usb_udc_speed_show, NULL);
you said 'several locations', but there's only one ? No other driver is
using something similar ?
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv3 2/4] usb: gadget: replace "is_dualspeed" with "max_speed"
2011-08-19 22:33 ` [PATCHv3 2/4] usb: gadget: replace "is_dualspeed" with "max_speed" Michal Nazarewicz
@ 2011-08-19 23:28 ` Felipe Balbi
2011-08-23 13:48 ` Michal Nazarewicz
0 siblings, 1 reply; 70+ messages in thread
From: Felipe Balbi @ 2011-08-19 23:28 UTC (permalink / raw)
To: Michal Nazarewicz
Cc: Alan Stern, Sebastian Andrzej Siewior, Yang Rui Rui, Dave Young,
Felipe Balbi, linux-usb, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 2882 bytes --]
Hi,
On Sat, Aug 20, 2011 at 12:33:00AM +0200, Michal Nazarewicz wrote:
> From: Michal Nazarewicz <mina86@mina86.com>
>
> This commit replaces usb_gadget's is_dualspeed field with
> a max_speed field.
>
> This change is made so that one will be able to check at
> run-time if given gadget supports super speed.
IMHO the logic is inverted here. It should start from the function
drivers. They should say which USB speeds they support, that would go up
to composite layer and composite would call e.g.
usb_gadget_set_speed(gadget, maximum_speed);
that function will set gadget->maximum_speed and call into the UDC
driver which would write maximum_speed into its internal registers, thus
preventing the core from working at speeds unsupported by the gadget
driver.
All of this should happen before we even connect to the host, so first
all UDCs should have their conversion to udc-core finished and start
using udc_start()/udc_stop(). The driver I just sent [1] illustrates
what udc_start()/udc_stop() should do.
[1] http://marc.info/?l=linux-usb&m=131376669022473&w=2
<snip>
> diff --git a/drivers/usb/gadget/udc-core.c b/drivers/usb/gadget/udc-core.c
> index e1ecdbc..25058b4 100644
> --- a/drivers/usb/gadget/udc-core.c
> +++ b/drivers/usb/gadget/udc-core.c
> @@ -371,14 +371,28 @@ static ssize_t usb_udc_softconn_store(struct device *dev,
> }
> static DEVICE_ATTR(soft_connect, S_IWUSR, NULL, usb_udc_softconn_store);
>
> -static ssize_t usb_udc_speed_show(struct device *dev,
> +#define USB_UDC_SPEED_ATTR(name) \
> +ssize_t usb_udc_##name##_show(struct device *dev, \
> + struct device_attribute *attr, char *buf) \
> +{ \
> + struct usb_udc *udc = container_of(dev, struct usb_udc, dev); \
> + return snprintf(buf, PAGE_SIZE, "%s\n", \
> + usb_device_speed_name(udc->gadget->name)); \
> +} \
> +static DEVICE_ATTR(name, S_IRUSR, usb_udc_##name##_show, NULL)
> +
> +static USB_UDC_SPEED_ATTR(speed);
how about "current_speed" ?
> +static USB_UDC_SPEED_ATTR(max_speed);
in this case, humans will be reading sysfs, so maximum_speed will look
nicer, IMHO.
> +/* Provide "is_dualspeed" for backward compatibility. */
> +static ssize_t usb_udc_is_dualspeed_show(struct device *dev,
> struct device_attribute *attr, char *buf)
> {
> - struct usb_udc *udc = container_of(dev, struct usb_udc, dev);
> - return snprintf(buf, PAGE_SIZE, "%s\n",
> - usb_device_speed_name(udc->gadget->speed));
> + struct usb_udc *udc = container_of(dev, struct usb_udc, dev);
> + return snprintf(buf, PAGE_SIZE, "%d\n",
> + gadget_is_dualspeed(udc->gadget));
> }
> -static DEVICE_ATTR(speed, S_IRUSR, usb_udc_speed_show, NULL);
> +static DEVICE_ATTR(is_dualspeed, S_IRUSR, usb_udc_is_dualspeed_show, NULL);
maybe deprecate this one on feature-removal-schedule ??
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv3 3/4] usb: gadget: rename usb_gadget_driver::speed to max_speed
2011-08-19 22:33 ` [PATCHv3 3/4] usb: gadget: rename usb_gadget_driver::speed to max_speed Michal Nazarewicz
@ 2011-08-19 23:31 ` Felipe Balbi
2011-08-20 2:34 ` Alan Stern
0 siblings, 1 reply; 70+ messages in thread
From: Felipe Balbi @ 2011-08-19 23:31 UTC (permalink / raw)
To: Michal Nazarewicz
Cc: Alan Stern, Sebastian Andrzej Siewior, Yang Rui Rui, Dave Young,
Felipe Balbi, linux-usb, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 3588 bytes --]
On Sat, Aug 20, 2011 at 12:33:01AM +0200, Michal Nazarewicz wrote:
> From: Michal Nazarewicz <mina86@mina86.com>
>
> This commit renames the “speed” field of the usb_gadget_driver
> structure to “max_speed”. This is so that to make it more
> apparent that the field represents the maximum speed gadget
> driver can support.
>
> This also make the field look more like fields with the same
> name in usb_gadget and usb_composite_driver structures. All
> of those represent the *maximal* speed given entity supports.
>
> After this commit, there are the following fields in various
> structures:
> * usb_gadget::speed - the current connection speed,
> * usb_gadget::max_speed - maximal speed UDC supports,
this will be handled inside the UDC itself, so why do you need to expose
it ?
> * usb_gadget_driver::max_speed - maximal speed gadget driver
> supports, and
> * usb_composite_driver::max_speed - maximal speed composite
> gadget supports.
>
> Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
> ---
> drivers/usb/gadget/amd5536udc.c | 2 +-
> drivers/usb/gadget/ci13xxx_udc.c | 2 +-
> drivers/usb/gadget/composite.c | 8 ++++----
> drivers/usb/gadget/dbgp.c | 2 +-
> drivers/usb/gadget/dummy_hcd.c | 13 ++++++-------
> drivers/usb/gadget/file_storage.c | 4 ++--
> drivers/usb/gadget/gmidi.c | 2 +-
> drivers/usb/gadget/goku_udc.c | 2 +-
> drivers/usb/gadget/inode.c | 6 +++---
> drivers/usb/gadget/m66592-udc.c | 2 +-
> drivers/usb/gadget/net2272.c | 2 +-
> drivers/usb/gadget/net2280.c | 2 +-
> drivers/usb/gadget/pch_udc.c | 2 +-
> drivers/usb/gadget/printer.c | 2 +-
> drivers/usb/gadget/r8a66597-udc.c | 2 +-
> include/linux/usb/gadget.h | 4 ++--
> 16 files changed, 28 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/usb/gadget/amd5536udc.c b/drivers/usb/gadget/amd5536udc.c
> index 80ae808..7b48ed2 100644
> --- a/drivers/usb/gadget/amd5536udc.c
> +++ b/drivers/usb/gadget/amd5536udc.c
> @@ -1968,7 +1968,7 @@ static int amd5536_start(struct usb_gadget_driver *driver,
> u32 tmp;
>
> if (!driver || !bind || !driver->setup
> - || driver->speed != USB_SPEED_HIGH)
> + || driver->max_speed != USB_SPEED_HIGH)
> return -EINVAL;
> if (!dev)
> return -ENODEV;
> diff --git a/drivers/usb/gadget/ci13xxx_udc.c b/drivers/usb/gadget/ci13xxx_udc.c
> index b877464..6c13174 100644
> --- a/drivers/usb/gadget/ci13xxx_udc.c
> +++ b/drivers/usb/gadget/ci13xxx_udc.c
> @@ -798,7 +798,7 @@ static ssize_t show_driver(struct device *dev, struct device_attribute *attr,
> n += scnprintf(buf + n, PAGE_SIZE - n, "function = %s\n",
> (driver->function ? driver->function : ""));
> n += scnprintf(buf + n, PAGE_SIZE - n, "max speed = %d\n",
> - driver->speed);
> + driver->max_speed);
>
> return n;
> }
> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
> index 5a3461e..851b5da 100644
> --- a/drivers/usb/gadget/composite.c
> +++ b/drivers/usb/gadget/composite.c
> @@ -1560,9 +1560,9 @@ composite_resume(struct usb_gadget *gadget)
>
> static struct usb_gadget_driver composite_driver = {
> #ifdef CONFIG_USB_GADGET_SUPERSPEED
> - .speed = USB_SPEED_SUPER,
> + .max_speed = USB_SPEED_SUPER,
> #else
> - .speed = USB_SPEED_HIGH,
> + .max_speed = USB_SPEED_HIGH,
by doing this based on what the function drivers tell you, you could
drop this ifdeferry.
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv3 3/4] usb: gadget: rename usb_gadget_driver::speed to max_speed
2011-08-19 23:31 ` Felipe Balbi
@ 2011-08-20 2:34 ` Alan Stern
2011-08-22 10:42 ` Felipe Balbi
0 siblings, 1 reply; 70+ messages in thread
From: Alan Stern @ 2011-08-20 2:34 UTC (permalink / raw)
To: Felipe Balbi
Cc: Michal Nazarewicz, Sebastian Andrzej Siewior, Yang Rui Rui,
Dave Young, linux-usb, linux-kernel
On Sat, 20 Aug 2011, Felipe Balbi wrote:
> On Sat, Aug 20, 2011 at 12:33:01AM +0200, Michal Nazarewicz wrote:
> > From: Michal Nazarewicz <mina86@mina86.com>
> >
> > This commit renames the “speed” field of the usb_gadget_driver
> > structure to “max_speed”. This is so that to make it more
> > apparent that the field represents the maximum speed gadget
> > driver can support.
> >
> > This also make the field look more like fields with the same
> > name in usb_gadget and usb_composite_driver structures. All
> > of those represent the *maximal* speed given entity supports.
> >
> > After this commit, there are the following fields in various
> > structures:
> > * usb_gadget::speed - the current connection speed,
> > * usb_gadget::max_speed - maximal speed UDC supports,
>
> this will be handled inside the UDC itself, so why do you need to expose
> it ?
I wondered about this too. It turns out that sometimes the gadget
driver really does need to know about the maximum speed supported by
the UDC hardware and driver.
For example, suppose the gadget driver and the UDC both support high
speed, but the gadget is currently plugged into a USB-1.1 controller
and therefore running only at full speed. Then the gadget driver has
to respond to the Get-Device-Qualifier and Get-Other-Speed-Config
requests by sending the appropriate descriptors. But if the UDC
doesn't support high speed operation then the gadget driver has to
respond to those requests with a STALL.
Alan Stern
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv3 4/4] usb: gadget: get rid of USB_GADGET_{DUAL,SUPER}SPEED
2011-08-19 22:33 ` [PATCHv3 4/4] usb: gadget: get rid of USB_GADGET_{DUAL,SUPER}SPEED Michal Nazarewicz
@ 2011-08-20 13:41 ` Alan Stern
2011-08-22 14:51 ` Michal Nazarewicz
0 siblings, 1 reply; 70+ messages in thread
From: Alan Stern @ 2011-08-20 13:41 UTC (permalink / raw)
To: Michal Nazarewicz
Cc: Sebastian Andrzej Siewior, Yang Rui Rui, Dave Young,
Felipe Balbi, linux-usb, linux-kernel
On Sat, 20 Aug 2011, Michal Nazarewicz wrote:
> From: Michal Nazarewicz <mina86@mina86.com>
>
> This commit removes USB_GADGET_DUALSPEED and USB_GADGET_SUPERSPEED
> Kconfig options. Since now kernel allows many UDC drivers to be
> compiled, those options may turn to no longer be valid. For
> instance, if someone decides to build UDC that supports super
> speed and UDC that supports high speed only, the latter will be
> "assumed" to support super speed since USB_GADGET_SUPERSPEED will
> be selected by the former.
>
> The test of whether CONFIG_USB_GADGET_*SPEED was defined was just
> an optimisation which removed otherwise dead code (ie. if UDC is
> not dual speed, there is no need to handle cases that can happen
> if speed is high). This commit removes those checks.
>
> Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
...
> --- a/drivers/usb/gadget/dummy_hcd.c
> +++ b/drivers/usb/gadget/dummy_hcd.c
> @@ -985,7 +985,7 @@ static int dummy_udc_probe (struct platform_device *pdev)
>
> dum->gadget.name = gadget_name;
> dum->gadget.ops = &dummy_ops;
> - dum->gadget.max_speed = USB_SPEED_SUPER;;
> + dum->gadget.max_speed = USB_SPEED_SUPER;
You might as well get this right in patch 2/4. Then this hunk
wouldn't be needed.
> --- a/drivers/usb/gadget/inode.c
> +++ b/drivers/usb/gadget/inode.c
...
> @@ -1773,11 +1767,7 @@ gadgetfs_suspend (struct usb_gadget *gadget)
> }
>
> static struct usb_gadget_driver gadgetfs_driver = {
> -#ifdef CONFIG_USB_GADGET_DUALSPEED
> .max_speed = USB_SPEED_HIGH,
> -#else
> - .max_speed = USB_SPEED_FULL,
> -#endif
I'm not sure that a static definition is correct here. The actual
max_speed depends on what descriptors the userspace program sends.
Take a look at the ep_config() routine.
Alan Stern
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv3 3/4] usb: gadget: rename usb_gadget_driver::speed to max_speed
2011-08-20 2:34 ` Alan Stern
@ 2011-08-22 10:42 ` Felipe Balbi
0 siblings, 0 replies; 70+ messages in thread
From: Felipe Balbi @ 2011-08-22 10:42 UTC (permalink / raw)
To: Alan Stern
Cc: Felipe Balbi, Michal Nazarewicz, Sebastian Andrzej Siewior,
Yang Rui Rui, Dave Young, linux-usb, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1692 bytes --]
Hi,
On Fri, Aug 19, 2011 at 10:34:29PM -0400, Alan Stern wrote:
> On Sat, 20 Aug 2011, Felipe Balbi wrote:
>
> > On Sat, Aug 20, 2011 at 12:33:01AM +0200, Michal Nazarewicz wrote:
> > > From: Michal Nazarewicz <mina86@mina86.com>
> > >
> > > This commit renames the “speed” field of the usb_gadget_driver
> > > structure to “max_speed”. This is so that to make it more
> > > apparent that the field represents the maximum speed gadget
> > > driver can support.
> > >
> > > This also make the field look more like fields with the same
> > > name in usb_gadget and usb_composite_driver structures. All
> > > of those represent the *maximal* speed given entity supports.
> > >
> > > After this commit, there are the following fields in various
> > > structures:
> > > * usb_gadget::speed - the current connection speed,
> > > * usb_gadget::max_speed - maximal speed UDC supports,
> >
> > this will be handled inside the UDC itself, so why do you need to expose
> > it ?
>
> I wondered about this too. It turns out that sometimes the gadget
> driver really does need to know about the maximum speed supported by
> the UDC hardware and driver.
>
> For example, suppose the gadget driver and the UDC both support high
> speed, but the gadget is currently plugged into a USB-1.1 controller
> and therefore running only at full speed. Then the gadget driver has
> to respond to the Get-Device-Qualifier and Get-Other-Speed-Config
> requests by sending the appropriate descriptors. But if the UDC
> doesn't support high speed operation then the gadget driver has to
> respond to those requests with a STALL.
aa, good point :-)
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv3 4/4] usb: gadget: get rid of USB_GADGET_{DUAL,SUPER}SPEED
2011-08-20 13:41 ` Alan Stern
@ 2011-08-22 14:51 ` Michal Nazarewicz
2011-08-22 15:03 ` Alan Stern
0 siblings, 1 reply; 70+ messages in thread
From: Michal Nazarewicz @ 2011-08-22 14:51 UTC (permalink / raw)
To: Alan Stern
Cc: Sebastian Andrzej Siewior, Yang Rui Rui, Dave Young,
Felipe Balbi, linux-usb, linux-kernel
> On Sat, 20 Aug 2011, Michal Nazarewicz wrote:
>> --- a/drivers/usb/gadget/dummy_hcd.c
>> +++ b/drivers/usb/gadget/dummy_hcd.c
>> @@ -985,7 +985,7 @@ static int dummy_udc_probe (struct platform_device
>> *pdev)
>>
>> dum->gadget.name = gadget_name;
>> dum->gadget.ops = &dummy_ops;
>> - dum->gadget.max_speed = USB_SPEED_SUPER;;
>> + dum->gadget.max_speed = USB_SPEED_SUPER;
On Sat, 20 Aug 2011 15:41:17 +0200, Alan Stern <stern@rowland.harvard.edu>
wrote:
> You might as well get this right in patch 2/4. Then this hunk
> wouldn't be needed.
Sorry about that, I've fixuped the wrong commit
>> --- a/drivers/usb/gadget/inode.c
>> +++ b/drivers/usb/gadget/inode.c
> ...
>> @@ -1773,11 +1767,7 @@ gadgetfs_suspend (struct usb_gadget *gadget)
>> }
>>
>> static struct usb_gadget_driver gadgetfs_driver = {
>> -#ifdef CONFIG_USB_GADGET_DUALSPEED
>> .max_speed = USB_SPEED_HIGH,
>> -#else
>> - .max_speed = USB_SPEED_FULL,
>> -#endif
> I'm not sure that a static definition is correct here. The actual
> max_speed depends on what descriptors the userspace program sends.
> Take a look at the ep_config() routine.
Good catch. I've changed it to:
if (dev->hs_config)
gadgetfs_driver.max_speed = USB_SPEED_HIGH;
else
gadgetfs_driver.max_speed = USB_SPEED_FULL;
just before usb_gadget_probe_driver(), which should do the trick.
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michal "mina86" Nazarewicz (o o)
ooo +-----<email/xmpp: mnazarewicz@google.com>-----ooO--(_)--Ooo--
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv3 1/4] usb: Provide usb_device_speed_name() function
2011-08-19 23:15 ` Felipe Balbi
@ 2011-08-22 14:53 ` Michal Nazarewicz
0 siblings, 0 replies; 70+ messages in thread
From: Michal Nazarewicz @ 2011-08-22 14:53 UTC (permalink / raw)
To: Felipe Balbi
Cc: Alan Stern, Sebastian Andrzej Siewior, Yang Rui Rui, Dave Young,
linux-usb, linux-kernel
On Sat, 20 Aug 2011 01:15:00 +0200, Felipe Balbi <balbi@ti.com> wrote:
> On Sat, Aug 20, 2011 at 12:32:59AM +0200, Michal Nazarewicz wrote:
>> From: Michal Nazarewicz <mina86@mina86.com>
>>
>> In a few places kernel wants to print a human-readable USB
>> device speed name (eg. "high") instead of a raw number
>> (eg. 3). Usually a switch is introduced in those places
>> leading to code repetition. To mitigate this issue, this
>> commit introduces usb_device_speed_name() function, which
>> returns a human-readable name of provided speed.
>>
>> Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
>> ---
>> drivers/usb/Makefile | 3 +++
>> drivers/usb/common.c | 24 ++++++++++++++++++++++++
>> drivers/usb/gadget/udc-core.c | 19 ++-----------------
>> include/linux/usb/ch9.h | 8 ++++++++
>> 4 files changed, 37 insertions(+), 17 deletions(-)
>> create mode 100644 drivers/usb/common.c
>>
>> diff --git a/drivers/usb/Makefile b/drivers/usb/Makefile
>> index 30ddf8d..a3167cc 100644
>> --- a/drivers/usb/Makefile
>> +++ b/drivers/usb/Makefile
>> @@ -51,3 +51,6 @@ obj-$(CONFIG_USB_MUSB_HDRC) += musb/
>> obj-$(CONFIG_USB_RENESAS_USBHS) += renesas_usbhs/
>> obj-$(CONFIG_USB_OTG_UTILS) += otg/
>> obj-$(CONFIG_USB_GADGET) += gadget/
>> +
>> +obj-$(CONFIG_USB) += common.o
>> +obj-$(CONFIG_USB_GADGET) += common.o
>> diff --git a/drivers/usb/common.c b/drivers/usb/common.c
>> new file mode 100644
>> index 0000000..e9d7141
>> --- /dev/null
>> +++ b/drivers/usb/common.c
>> @@ -0,0 +1,24 @@
>> +/*
>> + * Provides code common for host and device side USB.
>> + */
>> +
>> +#include <linux/kernel.h> /* for ARRAY_SIZE() */
>> +#include <linux/module.h> /* for EXPORT_SYMBOL_GPL() */
>> +#include <linux/usb/ch9.h>
>> +
>> +const char *usb_device_speed_name(enum usb_device_speed speed)
>
> somehow usb_speed_string() sounds better to me.
Done.
>> +{
>> + static const char *const names[] = {
>> + "UNKNOWN",
>> + "low-speed",
>> + "full-speed",
>> + "high-speed",
>> + "wireless",
>> + "super-speed",
>> + };
>> +
>> + if (speed < 0 || speed >= ARRAY_SIZE(names))
>> + speed = USB_SPEED_UNKNOWN;
>
> I generally dislike depending on a particular ordering of enumerations.
> While's it's highly unlikely that anyone will change that, if someone
> does change, then it'll take a long time to actually find the bug on the
> print here :-)
>
> I woud rather use a switch in this case.
Would a C99 initialisation be fine by you?
I, myself, have this aversion to switch as being slower (which is
disputable) and too verbose.
>> diff --git a/drivers/usb/gadget/udc-core.c
>> b/drivers/usb/gadget/udc-core.c
>> index 05ba472..e1ecdbc 100644
>> --- a/drivers/usb/gadget/udc-core.c
>> +++ b/drivers/usb/gadget/udc-core.c
>> @@ -375,23 +375,8 @@ static ssize_t usb_udc_speed_show(struct device
>> *dev,
>> struct device_attribute *attr, char *buf)
>> {
>> struct usb_udc *udc = container_of(dev, struct usb_udc, dev);
>> - struct usb_gadget *gadget = udc->gadget;
>> -
>> - switch (gadget->speed) {
>> - case USB_SPEED_LOW:
>> - return snprintf(buf, PAGE_SIZE, "low-speed\n");
>> - case USB_SPEED_FULL:
>> - return snprintf(buf, PAGE_SIZE, "full-speed\n");
>> - case USB_SPEED_HIGH:
>> - return snprintf(buf, PAGE_SIZE, "high-speed\n");
>> - case USB_SPEED_WIRELESS:
>> - return snprintf(buf, PAGE_SIZE, "wireless\n");
>> - case USB_SPEED_SUPER:
>> - return snprintf(buf, PAGE_SIZE, "super-speed\n");
>> - case USB_SPEED_UNKNOWN: /* FALLTHROUGH */
>> - default:
>> - return snprintf(buf, PAGE_SIZE, "UNKNOWN\n");
>> - }
>> + return snprintf(buf, PAGE_SIZE, "%s\n",
>> + usb_device_speed_name(udc->gadget->speed));
>> }
>> static DEVICE_ATTR(speed, S_IRUSR, usb_udc_speed_show, NULL);
>
> you said 'several locations', but there's only one ? No other driver is
> using something similar ?
OK, you caught me on being lazy... ;) Hunted down a few more instances.
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michal "mina86" Nazarewicz (o o)
ooo +-----<email/xmpp: mnazarewicz@google.com>-----ooO--(_)--Ooo--
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv3 4/4] usb: gadget: get rid of USB_GADGET_{DUAL,SUPER}SPEED
2011-08-22 14:51 ` Michal Nazarewicz
@ 2011-08-22 15:03 ` Alan Stern
0 siblings, 0 replies; 70+ messages in thread
From: Alan Stern @ 2011-08-22 15:03 UTC (permalink / raw)
To: Michal Nazarewicz
Cc: Sebastian Andrzej Siewior, Yang Rui Rui, Dave Young,
Felipe Balbi, linux-usb, linux-kernel
On Mon, 22 Aug 2011, Michal Nazarewicz wrote:
> >> --- a/drivers/usb/gadget/inode.c
> >> +++ b/drivers/usb/gadget/inode.c
> > ...
> >> @@ -1773,11 +1767,7 @@ gadgetfs_suspend (struct usb_gadget *gadget)
> >> }
> >>
> >> static struct usb_gadget_driver gadgetfs_driver = {
> >> -#ifdef CONFIG_USB_GADGET_DUALSPEED
> >> .max_speed = USB_SPEED_HIGH,
> >> -#else
> >> - .max_speed = USB_SPEED_FULL,
> >> -#endif
>
> > I'm not sure that a static definition is correct here. The actual
> > max_speed depends on what descriptors the userspace program sends.
> > Take a look at the ep_config() routine.
>
> Good catch. I've changed it to:
>
> if (dev->hs_config)
> gadgetfs_driver.max_speed = USB_SPEED_HIGH;
> else
> gadgetfs_driver.max_speed = USB_SPEED_FULL;
>
> just before usb_gadget_probe_driver(), which should do the trick.
Yes, that sounds like the right thing to do.
Alan Stern
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv3 2/4] usb: gadget: replace "is_dualspeed" with "max_speed"
2011-08-19 23:28 ` Felipe Balbi
@ 2011-08-23 13:48 ` Michal Nazarewicz
2011-08-23 13:58 ` Felipe Balbi
0 siblings, 1 reply; 70+ messages in thread
From: Michal Nazarewicz @ 2011-08-23 13:48 UTC (permalink / raw)
To: Felipe Balbi
Cc: Alan Stern, Sebastian Andrzej Siewior, Yang Rui Rui, Dave Young,
linux-usb, linux-kernel
Hi,
Sorry, I somehow missed this mail before.
> On Sat, Aug 20, 2011 at 12:33:00AM +0200, Michal Nazarewicz wrote:
>> This commit replaces usb_gadget's is_dualspeed field with
>> a max_speed field.
>>
>> This change is made so that one will be able to check at
>> run-time if given gadget supports super speed.
On Sat, 20 Aug 2011 01:28:06 +0200, Felipe Balbi <balbi@ti.com> wrote:
> IMHO the logic is inverted here. It should start from the function
> drivers. They should say which USB speeds they support, that would go up
> to composite layer and composite would call e.g.
> usb_gadget_set_speed(gadget, maximum_speed);
This is actually not how composite works at the moment. Currently,
a composite gadget can declare a maximum speed of say “high” even if
all the functions do not support that speed. Of course when host asks
about descriptors for given speed, only functions that support that
speed will be returned (and hence only configurations that have at
least one function supporting that speed).
Whether the behaviour should be changed is, in my opinion, issue separate
from the patchset that I'm sending.
>> diff --git a/drivers/usb/gadget/udc-core.c
>> b/drivers/usb/gadget/udc-core.c
>> index e1ecdbc..25058b4 100644
>> --- a/drivers/usb/gadget/udc-core.c
>> +++ b/drivers/usb/gadget/udc-core.c
>> @@ -371,14 +371,28 @@ static ssize_t usb_udc_softconn_store(struct
>> device *dev,
>> }
>> static DEVICE_ATTR(soft_connect, S_IWUSR, NULL,
>> usb_udc_softconn_store);
>>
>> -static ssize_t usb_udc_speed_show(struct device *dev,
>> +#define USB_UDC_SPEED_ATTR(name) \
>> +ssize_t usb_udc_##name##_show(struct device *dev, \
>> + struct device_attribute *attr, char *buf) \
>> +{ \
>> + struct usb_udc *udc = container_of(dev, struct usb_udc, dev); \
>> + return snprintf(buf, PAGE_SIZE, "%s\n", \
>> + usb_device_speed_name(udc->gadget->name)); \
>> +} \
>> +static DEVICE_ATTR(name, S_IRUSR, usb_udc_##name##_show, NULL)
>> +
>> +static USB_UDC_SPEED_ATTR(speed);
>
> how about "current_speed" ?
Is there a big advantage? That would change external interface and I don't
see reason to do so. Of course, udc class is quite recent so if you feel
we can
ignore this issue I can go forward with that change.
>> +static USB_UDC_SPEED_ATTR(max_speed);
>
> in this case, humans will be reading sysfs, so maximum_speed will look
> nicer, IMHO.
I don't care much. Will change.
>> +/* Provide "is_dualspeed" for backward compatibility. */
>> +static ssize_t usb_udc_is_dualspeed_show(struct device *dev,
>> struct device_attribute *attr, char *buf)
>> {
>> - struct usb_udc *udc = container_of(dev, struct usb_udc, dev);
>> - return snprintf(buf, PAGE_SIZE, "%s\n",
>> - usb_device_speed_name(udc->gadget->speed));
>> + struct usb_udc *udc = container_of(dev, struct usb_udc, dev);
>> + return snprintf(buf, PAGE_SIZE, "%d\n",
>> + gadget_is_dualspeed(udc->gadget));
>> }
>> -static DEVICE_ATTR(speed, S_IRUSR, usb_udc_speed_show, NULL);
>> +static DEVICE_ATTR(is_dualspeed, S_IRUSR, usb_udc_is_dualspeed_show,
>> NULL);
>
> maybe deprecate this one on feature-removal-schedule ??
Sure.
Also, if we decide to change “speed” to “current_speed”, we could just drop
this without the announcement.
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michal "mina86" Nazarewicz (o o)
ooo +-----<email/xmpp: mnazarewicz@google.com>-----ooO--(_)--Ooo--
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv3 2/4] usb: gadget: replace "is_dualspeed" with "max_speed"
2011-08-23 13:48 ` Michal Nazarewicz
@ 2011-08-23 13:58 ` Felipe Balbi
2011-08-23 14:15 ` Michal Nazarewicz
0 siblings, 1 reply; 70+ messages in thread
From: Felipe Balbi @ 2011-08-23 13:58 UTC (permalink / raw)
To: Michal Nazarewicz
Cc: Felipe Balbi, Alan Stern, Sebastian Andrzej Siewior,
Yang Rui Rui, Dave Young, linux-usb, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 4763 bytes --]
Hi,
On Tue, Aug 23, 2011 at 03:48:30PM +0200, Michal Nazarewicz wrote:
> Sorry, I somehow missed this mail before.
np
> >On Sat, Aug 20, 2011 at 12:33:00AM +0200, Michal Nazarewicz wrote:
> >>This commit replaces usb_gadget's is_dualspeed field with
> >>a max_speed field.
> >>
> >>This change is made so that one will be able to check at
> >>run-time if given gadget supports super speed.
>
> On Sat, 20 Aug 2011 01:28:06 +0200, Felipe Balbi <balbi@ti.com> wrote:
> >IMHO the logic is inverted here. It should start from the function
> >drivers. They should say which USB speeds they support, that would go up
> >to composite layer and composite would call e.g.
> >usb_gadget_set_speed(gadget, maximum_speed);
>
> This is actually not how composite works at the moment. Currently,
my suggestion was exactly to change that :-) Speed is something
functions support. composite.c shouldn't dictate which speed functions
should use, rather composite.c should use the maximum speed which all
functions support.
> a composite gadget can declare a maximum speed of say “high” even if
> all the functions do not support that speed. Of course when host asks
> about descriptors for given speed, only functions that support that
> speed will be returned (and hence only configurations that have at
> least one function supporting that speed).
>
> Whether the behaviour should be changed is, in my opinion, issue
> separate from the patchset that I'm sending.
I wouldn't say that, actually. Just replacing is_dualspeed with
max_speed isn't changing much and if you want to make that part of the
code better, why not doing The Right Thing(TM) ?
All of the speed negotiation between composite.c and f_*.c should happen
before even connecting to host (before attaching data pullups, enabling
IRQs, etc), that's exactly why me and Sebastian have decided (at that
time off list) to add udc_start()/udc_stop() methods. One of the reason
was that it would be a quite intrusive change to all UDC drivers, second
we wanted to give maintainers/authors of those UDC drivers some grace
period for the change, third when all UDCs are converted, it allow us
to do the speed negotiation before connecting to host.
> >>diff --git a/drivers/usb/gadget/udc-core.c
> >>b/drivers/usb/gadget/udc-core.c
> >>index e1ecdbc..25058b4 100644
> >>--- a/drivers/usb/gadget/udc-core.c
> >>+++ b/drivers/usb/gadget/udc-core.c
> >>@@ -371,14 +371,28 @@ static ssize_t
> >>usb_udc_softconn_store(struct device *dev,
> >> }
> >> static DEVICE_ATTR(soft_connect, S_IWUSR, NULL,
> >>usb_udc_softconn_store);
> >>
> >>-static ssize_t usb_udc_speed_show(struct device *dev,
> >>+#define USB_UDC_SPEED_ATTR(name) \
> >>+ssize_t usb_udc_##name##_show(struct device *dev, \
> >>+ struct device_attribute *attr, char *buf) \
> >>+{ \
> >>+ struct usb_udc *udc = container_of(dev, struct usb_udc, dev); \
> >>+ return snprintf(buf, PAGE_SIZE, "%s\n", \
> >>+ usb_device_speed_name(udc->gadget->name)); \
> >>+} \
> >>+static DEVICE_ATTR(name, S_IRUSR, usb_udc_##name##_show, NULL)
> >>+
> >>+static USB_UDC_SPEED_ATTR(speed);
> >
> >how about "current_speed" ?
>
> Is there a big advantage? That would change external interface and I don't
> see reason to do so. Of course, udc class is quite recent so if you
> feel we can
> ignore this issue I can go forward with that change.
you already maximum_speed (below) and speed alone looses some extra
hint of what kind of information will be there. I think it's better to
change this to current_speed and make a symbolic link called 'speed'
which we can keep for the next 5 years and remove it in e.g. Linux v5.0
> >>+/* Provide "is_dualspeed" for backward compatibility. */
> >>+static ssize_t usb_udc_is_dualspeed_show(struct device *dev,
> >> struct device_attribute *attr, char *buf)
> >> {
> >>- struct usb_udc *udc = container_of(dev, struct usb_udc, dev);
> >>- return snprintf(buf, PAGE_SIZE, "%s\n",
> >>- usb_device_speed_name(udc->gadget->speed));
> >>+ struct usb_udc *udc = container_of(dev, struct usb_udc, dev);
> >>+ return snprintf(buf, PAGE_SIZE, "%d\n",
> >>+ gadget_is_dualspeed(udc->gadget));
> >> }
> >>-static DEVICE_ATTR(speed, S_IRUSR, usb_udc_speed_show, NULL);
> >>+static DEVICE_ATTR(is_dualspeed, S_IRUSR,
> >>usb_udc_is_dualspeed_show, NULL);
> >
> >maybe deprecate this one on feature-removal-schedule ??
>
> Sure.
>
> Also, if we decide to change “speed” to “current_speed”, we could just drop
> this without the announcement.
I rather have the announcement. See above that you can make a symbolic
link with the name 'speed' for the time being.
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv3 2/4] usb: gadget: replace "is_dualspeed" with "max_speed"
2011-08-23 13:58 ` Felipe Balbi
@ 2011-08-23 14:15 ` Michal Nazarewicz
2011-08-23 14:37 ` Alan Stern
2011-08-23 15:05 ` Felipe Balbi
0 siblings, 2 replies; 70+ messages in thread
From: Michal Nazarewicz @ 2011-08-23 14:15 UTC (permalink / raw)
To: Felipe Balbi
Cc: Alan Stern, Sebastian Andrzej Siewior, Yang Rui Rui, Dave Young,
linux-usb, linux-kernel
On Tue, 23 Aug 2011 15:58:17 +0200, Felipe Balbi <balbi@ti.com> wrote:
>> On Sat, 20 Aug 2011 01:28:06 +0200, Felipe Balbi <balbi@ti.com> wrote:
>>> IMHO the logic is inverted here. It should start from the function
>>> drivers. They should say which USB speeds they support, that would go
>>> up to composite layer and composite would call e.g.
>>> usb_gadget_set_speed(gadget, maximum_speed);
>>
>> This is actually not how composite works at the moment. Currently,
>
> my suggestion was exactly to change that :-) Speed is something
> functions support. composite.c shouldn't dictate which speed functions
> should use, rather composite.c should use the maximum speed which all
> functions support.
Strictly speaking, composite.c does not dictate anything. It just copies
what the composite gadget driver declared as maximum speed.
My understanding was that one could consciously create a composite gadget
such that not all of the functions support all of the speeds.
I would suggest leaving everything as is, expect if usb_composite_driver's
max_speed is set to USB_SPEED_UNKNOWN in which case composite would iterate
over all the functions and figure out the maximum speed that all of the
functions support.
>> a composite gadget can declare a maximum speed of say “high” even if
>> all the functions do not support that speed. Of course when host asks
>> about descriptors for given speed, only functions that support that
>> speed will be returned (and hence only configurations that have at
>> least one function supporting that speed).
>>
>> Whether the behaviour should be changed is, in my opinion, issue
>> separate from the patchset that I'm sending.
>
> I wouldn't say that, actually. Just replacing is_dualspeed with
> max_speed isn't changing much and if you want to make that part of the
> code better, why not doing The Right Thing(TM) ?
I'm just saying that the main reason for this patchset is to get rid of
the USB_GADGET_DUALSPEED and USB_GADGET_SUPERSPEED Kconfig options. So
the purpose of changing is_dualspeed with max_speed is to be able to
check if gadget is super speed at run-time so that gadget_is_superspeed()
can be implemented.
So I would like to just get this done and then figure out what to do with
composite.c. How does that sound?
> All of the speed negotiation between composite.c and f_*.c should happen
> before even connecting to host
Yep, obviously. The usb_gadget_probe_driver() is called at the very and
once all the functions and everything is added so composite.c can do all
the analysis it wants and figure out the maximum speed.
> (before attaching data pullups, enabling IRQs, etc), that's exactly why
> me and Sebastian have decided (at that time off list) to add
> udc_start()/udc_stop() methods.
I don't really follow why those would be needed...
> One of the reason was that it would be a quite intrusive change to
> all UDC drivers, second we wanted to give maintainers/authors of
> those UDC drivers some grace period for the change, third when
> all UDCs are converted, it allow us to do the speed negotiation
> before connecting to host.
Again, I don't follow. We can figure out the max_speed before calling
usb_gadget_probe_driver() just fine. We don't even have to have UDC
to figure that out (ie. gadget driver's max_speed does not change
depending on UDC, right?).
>>> how about "current_speed" ?
>>
>> Is there a big advantage? That would change external interface and I
>> don't see reason to do so. Of course, udc class is quite recent so if
>> you feel we can ignore this issue I can go forward with that change.
> you already maximum_speed (below) and speed alone looses some extra
> hint of what kind of information will be there. I think it's better to
> change this to current_speed and make a symbolic link called 'speed'
> which we can keep for the next 5 years and remove it in e.g. Linux v5.0
OK, I'll do that (as soon as I figure out/recall how to make symlinks that
is ;) ).
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michal "mina86" Nazarewicz (o o)
ooo +-----<email/xmpp: mnazarewicz@google.com>-----ooO--(_)--Ooo--
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv3 2/4] usb: gadget: replace "is_dualspeed" with "max_speed"
2011-08-23 14:15 ` Michal Nazarewicz
@ 2011-08-23 14:37 ` Alan Stern
2011-08-23 14:58 ` Felipe Balbi
2011-08-23 15:05 ` Felipe Balbi
1 sibling, 1 reply; 70+ messages in thread
From: Alan Stern @ 2011-08-23 14:37 UTC (permalink / raw)
To: Michal Nazarewicz
Cc: Felipe Balbi, Sebastian Andrzej Siewior, Yang Rui Rui,
Dave Young, linux-usb, linux-kernel
On Tue, 23 Aug 2011, Michal Nazarewicz wrote:
> My understanding was that one could consciously create a composite gadget
> such that not all of the functions support all of the speeds.
That's an important point. If somebody wants to build a composite
gadget that includes both midi and storage support, the storage driver
would be able to go all the way up to SuperSpeed (or rather, it will
once Felipe's patch is finished and merged) but the midi driver would
only support full speed. So what speed should the UDC connect at?
In cases like this there's really no way to know. Ultimately the best
approach would be to update the midi driver, but that's a long-term
sort of thing.
Usually when the kernel can't decide something, it asks the user for
help (by allowing the user to specify the speed via a module parameter,
for instance). In this case even that wouldn't work, because the
gadget's userspace isn't aware of which functions the host's user will
want to utilize.
At the moment I don't know the best answer. This is something we
should all think about.
Alan Stern
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv3 2/4] usb: gadget: replace "is_dualspeed" with "max_speed"
2011-08-23 14:37 ` Alan Stern
@ 2011-08-23 14:58 ` Felipe Balbi
2011-08-23 15:07 ` Michal Nazarewicz
2011-08-23 15:43 ` Alan Stern
0 siblings, 2 replies; 70+ messages in thread
From: Felipe Balbi @ 2011-08-23 14:58 UTC (permalink / raw)
To: Alan Stern
Cc: Michal Nazarewicz, Felipe Balbi, Sebastian Andrzej Siewior,
Yang Rui Rui, Dave Young, linux-usb, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1643 bytes --]
Hi,
On Tue, Aug 23, 2011 at 10:37:23AM -0400, Alan Stern wrote:
> On Tue, 23 Aug 2011, Michal Nazarewicz wrote:
>
> > My understanding was that one could consciously create a composite gadget
> > such that not all of the functions support all of the speeds.
>
> That's an important point. If somebody wants to build a composite
> gadget that includes both midi and storage support, the storage driver
> would be able to go all the way up to SuperSpeed (or rather, it will
> once Felipe's patch is finished and merged) but the midi driver would
> only support full speed. So what speed should the UDC connect at?
if they are both part of the of the same configuration, full speed.
Otherwise Audio won't work. If they are part of different
configurations, then maximum_speed will be defined by the chosen config,
no ?
> In cases like this there's really no way to know. Ultimately the best
> approach would be to update the midi driver, but that's a long-term
> sort of thing.
yeah, I agree here.
> Usually when the kernel can't decide something, it asks the user for
> help (by allowing the user to specify the speed via a module parameter,
> for instance). In this case even that wouldn't work, because the
> gadget's userspace isn't aware of which functions the host's user will
> want to utilize.
>
> At the moment I don't know the best answer. This is something we
> should all think about.
I stand with the fact that if they are part of the same configuration,
there's no way around it, storage will have to come down to full speed.
Unless I'm missing something.
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv3 2/4] usb: gadget: replace "is_dualspeed" with "max_speed"
2011-08-23 14:15 ` Michal Nazarewicz
2011-08-23 14:37 ` Alan Stern
@ 2011-08-23 15:05 ` Felipe Balbi
2011-08-23 15:30 ` Michal Nazarewicz
1 sibling, 1 reply; 70+ messages in thread
From: Felipe Balbi @ 2011-08-23 15:05 UTC (permalink / raw)
To: Michal Nazarewicz
Cc: Felipe Balbi, Alan Stern, Sebastian Andrzej Siewior,
Yang Rui Rui, Dave Young, linux-usb, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 5235 bytes --]
Hi,
On Tue, Aug 23, 2011 at 04:15:08PM +0200, Michal Nazarewicz wrote:
> On Tue, 23 Aug 2011 15:58:17 +0200, Felipe Balbi <balbi@ti.com> wrote:
>
> >>On Sat, 20 Aug 2011 01:28:06 +0200, Felipe Balbi <balbi@ti.com> wrote:
> >>>IMHO the logic is inverted here. It should start from the function
> >>>drivers. They should say which USB speeds they support, that
> >>>would go up to composite layer and composite would call e.g.
> >>>usb_gadget_set_speed(gadget, maximum_speed);
> >>
> >>This is actually not how composite works at the moment. Currently,
> >
> >my suggestion was exactly to change that :-) Speed is something
> >functions support. composite.c shouldn't dictate which speed functions
> >should use, rather composite.c should use the maximum speed which all
> >functions support.
>
> Strictly speaking, composite.c does not dictate anything. It just copies
> what the composite gadget driver declared as maximum speed.
true.
> My understanding was that one could consciously create a composite gadget
> such that not all of the functions support all of the speeds.
of course, but if he puts FS function and SS function on the same
configuration, SS function will have to come down to FS, no ?
> >>a composite gadget can declare a maximum speed of say “high” even if
> >>all the functions do not support that speed. Of course when host asks
> >>about descriptors for given speed, only functions that support that
> >>speed will be returned (and hence only configurations that have at
> >>least one function supporting that speed).
> >>
> >>Whether the behaviour should be changed is, in my opinion, issue
> >>separate from the patchset that I'm sending.
> >
> >I wouldn't say that, actually. Just replacing is_dualspeed with
> >max_speed isn't changing much and if you want to make that part of the
> >code better, why not doing The Right Thing(TM) ?
>
> I'm just saying that the main reason for this patchset is to get rid of
> the USB_GADGET_DUALSPEED and USB_GADGET_SUPERSPEED Kconfig options. So
yes, but for removing USB_GADGET_DUALSPEED there's no requirement, per
se, to remove is_dualspeed, am I right ? I could be missing something.
> the purpose of changing is_dualspeed with max_speed is to be able to
> check if gadget is super speed at run-time so that gadget_is_superspeed()
> can be implemented.
I understand. It does sound better than adding is_superspeed flag.
> So I would like to just get this done and then figure out what to do with
> composite.c. How does that sound?
When you put it that way, I guess I must agree ;-)
> >All of the speed negotiation between composite.c and f_*.c should happen
> >before even connecting to host
>
> Yep, obviously. The usb_gadget_probe_driver() is called at the very and
> once all the functions and everything is added so composite.c can do all
> the analysis it wants and figure out the maximum speed.
>
> >(before attaching data pullups, enabling IRQs, etc), that's exactly why
> >me and Sebastian have decided (at that time off list) to add
> >udc_start()/udc_stop() methods.
>
> I don't really follow why those would be needed...
Ok, I guess I need to give the full picture here, my bad.
Let's say you have a SuperSpeed controller, but you know that this
particular gadget driver can only support fullspeed, so why do you need
to go through RX detection, HS chirp sequence and whatnot if you can
decide the maximum_speed before kickstarting the UDC's state machine ?
most controllers (or at least the ones I have seen) have ways to set the
maximum_speed we are going to support. As of today, we always, blindly,
set the maximum supported by the controller, but that can change
overtime.
This might (and I never tested) mean we would be saving a few uA in the
sense that unused HW blocks wouldn't even be powered up (at the least
the more recent cores have quite a fine grain control over what powers
up and what doesn't).
> >One of the reason was that it would be a quite intrusive change to
> >all UDC drivers, second we wanted to give maintainers/authors of
> >those UDC drivers some grace period for the change, third when
> >all UDCs are converted, it allow us to do the speed negotiation
> >before connecting to host.
>
> Again, I don't follow. We can figure out the max_speed before calling
> usb_gadget_probe_driver() just fine. We don't even have to have UDC
> to figure that out (ie. gadget driver's max_speed does not change
> depending on UDC, right?).
it doesn't change depending on UDC, but UDC can take certain decisions
depending on the maximum_speed current gadget driver supports ;-)
> >you already maximum_speed (below) and speed alone looses some extra
> >hint of what kind of information will be there. I think it's better to
> >change this to current_speed and make a symbolic link called 'speed'
> >which we can keep for the next 5 years and remove it in e.g. Linux v5.0
>
> OK, I'll do that (as soon as I figure out/recall how to make symlinks that
> is ;) ).
yeah, I would have to go through the same re-education ;-)
(please add a note on feature-removal-schedule too)
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv3 2/4] usb: gadget: replace "is_dualspeed" with "max_speed"
2011-08-23 14:58 ` Felipe Balbi
@ 2011-08-23 15:07 ` Michal Nazarewicz
2011-08-23 15:11 ` Felipe Balbi
2011-08-23 15:43 ` Alan Stern
1 sibling, 1 reply; 70+ messages in thread
From: Michal Nazarewicz @ 2011-08-23 15:07 UTC (permalink / raw)
To: Alan Stern, Felipe Balbi
Cc: Sebastian Andrzej Siewior, Yang Rui Rui, Dave Young, linux-usb,
linux-kernel
On Tue, 23 Aug 2011 16:58:01 +0200, Felipe Balbi <balbi@ti.com> wrote:
> I stand with the fact that if they are part of the same configuration,
> there's no way around it, storage will have to come down to full speed.
> Unless I'm missing something.
If you look at config_buf() in composite.c, you'll notice that composite
just skips functions that do not support current speed. This means, that
if a connection uses high speed but given function can handle only full
speed, it won't be included in the descriptors.
config_buf() is called from config_desc() which similarly, iterates over
configurations skipping the ones that do not support given speed.
Configuration is defined to support given speed if at least one function
that is added to that configuration supports that speed (see
usb_add_function()).
This also allows creating functions that support high speed but do not
support full speed (which I'm unsure whether it makes sense).
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michal "mina86" Nazarewicz (o o)
ooo +-----<email/xmpp: mnazarewicz@google.com>-----ooO--(_)--Ooo--
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv3 2/4] usb: gadget: replace "is_dualspeed" with "max_speed"
2011-08-23 15:07 ` Michal Nazarewicz
@ 2011-08-23 15:11 ` Felipe Balbi
2011-08-23 15:26 ` Michal Nazarewicz
0 siblings, 1 reply; 70+ messages in thread
From: Felipe Balbi @ 2011-08-23 15:11 UTC (permalink / raw)
To: Michal Nazarewicz
Cc: Alan Stern, Felipe Balbi, Sebastian Andrzej Siewior,
Yang Rui Rui, Dave Young, linux-usb, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1265 bytes --]
Hi,
On Tue, Aug 23, 2011 at 05:07:11PM +0200, Michal Nazarewicz wrote:
> On Tue, 23 Aug 2011 16:58:01 +0200, Felipe Balbi <balbi@ti.com> wrote:
> >I stand with the fact that if they are part of the same configuration,
> >there's no way around it, storage will have to come down to full speed.
> >Unless I'm missing something.
>
> If you look at config_buf() in composite.c, you'll notice that composite
> just skips functions that do not support current speed. This means, that
> if a connection uses high speed but given function can handle only full
> speed, it won't be included in the descriptors.
>
> config_buf() is called from config_desc() which similarly, iterates over
> configurations skipping the ones that do not support given speed.
>
> Configuration is defined to support given speed if at least one function
> that is added to that configuration supports that speed (see
> usb_add_function()).
I'm trying to think if this is really what we want. This might mean
$current_config will be different depending if we connect it to SS, HS
or FS hubs. Do we need USB-IF's help here ? I guess I could drop them a
mail and ask for some clarification. Don't know how long it would take
to get an answer though :-s
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv3 2/4] usb: gadget: replace "is_dualspeed" with "max_speed"
2011-08-23 15:11 ` Felipe Balbi
@ 2011-08-23 15:26 ` Michal Nazarewicz
2011-08-23 17:19 ` Felipe Balbi
0 siblings, 1 reply; 70+ messages in thread
From: Michal Nazarewicz @ 2011-08-23 15:26 UTC (permalink / raw)
To: Felipe Balbi
Cc: Alan Stern, Sebastian Andrzej Siewior, Yang Rui Rui, Dave Young,
linux-usb, linux-kernel
> On Tue, Aug 23, 2011 at 05:07:11PM +0200, Michal Nazarewicz wrote:
>> If you look at config_buf() in composite.c, you'll notice that composite
>> just skips functions that do not support current speed. This means,
>> that if a connection uses high speed but given function can handle only
>> full speed, it won't be included in the descriptors.
>>
>> config_buf() is called from config_desc() which similarly, iterates over
>> configurations skipping the ones that do not support given speed.
>>
>> Configuration is defined to support given speed if at least one function
>> that is added to that configuration supports that speed (see
>> usb_add_function()).
On Tue, 23 Aug 2011 17:11:06 +0200, Felipe Balbi <balbi@ti.com> wrote:
> I'm trying to think if this is really what we want. This might mean
> $current_config will be different depending if we connect it to SS, HS
> or FS hubs.
What do you mean by $current_config? The bConfigurationValue will be
the same for "the same" configuration regardless of speed but different
configurations may be available for different speeds and "the same"
configuration in various speeds might have different functions.
So just for the sake of having something concrete, let's say we have:
Configuration #1 with
function A supporting FS and HS and
function B supporting FS only.
Configuration #2 with
function B supporting FS only.
Configuration #3 with
function A supporting FS and HS and
function C supporting HS only (dunno if that makes sense).
When connected to FS hub, we will return the following:
Configuration #1: A, B
Configuration #2: B
Configuration #3: A
When connect to HS hub, we will return the following:
Configuration #1: A
Configuration #3: A, C
And similarly to bConfigurationValue, the interface IDs may have holes in
them.
Whether that's what we want, I don't know... But if I understand
composite.c correctly, that's how it is implemented now.
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michal "mina86" Nazarewicz (o o)
ooo +-----<email/xmpp: mnazarewicz@google.com>-----ooO--(_)--Ooo--
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv3 2/4] usb: gadget: replace "is_dualspeed" with "max_speed"
2011-08-23 15:05 ` Felipe Balbi
@ 2011-08-23 15:30 ` Michal Nazarewicz
0 siblings, 0 replies; 70+ messages in thread
From: Michal Nazarewicz @ 2011-08-23 15:30 UTC (permalink / raw)
To: Felipe Balbi
Cc: Alan Stern, Sebastian Andrzej Siewior, Yang Rui Rui, Dave Young,
linux-usb, linux-kernel
>> On Tue, 23 Aug 2011 15:58:17 +0200, Felipe Balbi <balbi@ti.com> wrote:
>>> All of the speed negotiation between composite.c and f_*.c should
>>> happen before even connecting to host
> On Tue, Aug 23, 2011 at 04:15:08PM +0200, Michal Nazarewicz wrote:
>> Yep, obviously. The usb_gadget_probe_driver() is called at the very and
>> once all the functions and everything is added so composite.c can do all
>> the analysis it wants and figure out the maximum speed.
>>
>> >(before attaching data pullups, enabling IRQs, etc), that's exactly why
>> >me and Sebastian have decided (at that time off list) to add
>> >udc_start()/udc_stop() methods.
>>
>> I don't really follow why those would be needed...
On Tue, 23 Aug 2011 17:05:48 +0200, Felipe Balbi <balbi@ti.com> wrote:
> Ok, I guess I need to give the full picture here, my bad.
>
> Let's say you have a SuperSpeed controller, but you know that this
> particular gadget driver can only support fullspeed, so why do you need
> to go through RX detection, HS chirp sequence and whatnot if you can
> decide the maximum_speed before kickstarting the UDC's state machine ?
But isn't that what's happening right now? The gadget_driver structure
has a speed field which is set to the maximum speed the gadget driver
can handle. Only after this is set, usb_gadget_probe_driver() is
called so at this point a SS UDC can figure out whether it needs to
turn pieces needed for SS support or not.
>>> you already maximum_speed (below) and speed alone looses some extra
>>> hint of what kind of information will be there. I think it's better to
>>> change this to current_speed and make a symbolic link called 'speed'
>>> which we can keep for the next 5 years and remove it in e.g. Linux v5.0
>>
>> OK, I'll do that (as soon as I figure out/recall how to make symlinks
>> that is ;) ).
> yeah, I would have to go through the same re-education ;-)
Adding another attribute with the same show function seems easy, but
that's probably not elegant. ;)
> (please add a note on feature-removal-schedule too)
Yep!
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michal "mina86" Nazarewicz (o o)
ooo +-----<email/xmpp: mnazarewicz@google.com>-----ooO--(_)--Ooo--
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv3 2/4] usb: gadget: replace "is_dualspeed" with "max_speed"
2011-08-23 14:58 ` Felipe Balbi
2011-08-23 15:07 ` Michal Nazarewicz
@ 2011-08-23 15:43 ` Alan Stern
2011-08-23 17:21 ` Felipe Balbi
1 sibling, 1 reply; 70+ messages in thread
From: Alan Stern @ 2011-08-23 15:43 UTC (permalink / raw)
To: Felipe Balbi
Cc: Michal Nazarewicz, Sebastian Andrzej Siewior, Yang Rui Rui,
Dave Young, linux-usb, linux-kernel
On Tue, 23 Aug 2011, Felipe Balbi wrote:
> Hi,
>
> On Tue, Aug 23, 2011 at 10:37:23AM -0400, Alan Stern wrote:
> > On Tue, 23 Aug 2011, Michal Nazarewicz wrote:
> >
> > > My understanding was that one could consciously create a composite gadget
> > > such that not all of the functions support all of the speeds.
> >
> > That's an important point. If somebody wants to build a composite
> > gadget that includes both midi and storage support, the storage driver
> > would be able to go all the way up to SuperSpeed (or rather, it will
> > once Felipe's patch is finished and merged) but the midi driver would
> > only support full speed. So what speed should the UDC connect at?
>
> if they are both part of the of the same configuration, full speed.
> Otherwise Audio won't work. If they are part of different
> configurations, then maximum_speed will be defined by the chosen config,
> no ?
No. The speed has to be chosen first, when the gadget connects to the
host. The config isn't chosen until later, after the host retrieves
the various descriptors and decides among them.
> > In cases like this there's really no way to know. Ultimately the best
> > approach would be to update the midi driver, but that's a long-term
> > sort of thing.
>
> yeah, I agree here.
>
> > Usually when the kernel can't decide something, it asks the user for
> > help (by allowing the user to specify the speed via a module parameter,
> > for instance). In this case even that wouldn't work, because the
> > gadget's userspace isn't aware of which functions the host's user will
> > want to utilize.
> >
> > At the moment I don't know the best answer. This is something we
> > should all think about.
>
> I stand with the fact that if they are part of the same configuration,
> there's no way around it, storage will have to come down to full speed.
> Unless I'm missing something.
What if they are in different configurations?
Alan Stern
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv3 2/4] usb: gadget: replace "is_dualspeed" with "max_speed"
2011-08-23 15:26 ` Michal Nazarewicz
@ 2011-08-23 17:19 ` Felipe Balbi
2011-08-23 18:44 ` Michal Nazarewicz
0 siblings, 1 reply; 70+ messages in thread
From: Felipe Balbi @ 2011-08-23 17:19 UTC (permalink / raw)
To: Michal Nazarewicz
Cc: Felipe Balbi, Alan Stern, Sebastian Andrzej Siewior,
Yang Rui Rui, Dave Young, linux-usb, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 2213 bytes --]
Hi,
On Tue, Aug 23, 2011 at 05:26:24PM +0200, Michal Nazarewicz wrote:
> >On Tue, Aug 23, 2011 at 05:07:11PM +0200, Michal Nazarewicz wrote:
> >>If you look at config_buf() in composite.c, you'll notice that composite
> >>just skips functions that do not support current speed. This
> >>means, that if a connection uses high speed but given function
> >>can handle only
> >>full speed, it won't be included in the descriptors.
> >>
> >>config_buf() is called from config_desc() which similarly, iterates over
> >>configurations skipping the ones that do not support given speed.
> >>
> >>Configuration is defined to support given speed if at least one function
> >>that is added to that configuration supports that speed (see
> >>usb_add_function()).
>
> On Tue, 23 Aug 2011 17:11:06 +0200, Felipe Balbi <balbi@ti.com> wrote:
> >I'm trying to think if this is really what we want. This might mean
> >$current_config will be different depending if we connect it to SS, HS
> >or FS hubs.
>
> What do you mean by $current_config? The bConfigurationValue will be
the currently chosen configuration.
> the same for "the same" configuration regardless of speed but different
> configurations may be available for different speeds and "the same"
> configuration in various speeds might have different functions.
>
> So just for the sake of having something concrete, let's say we have:
>
> Configuration #1 with
> function A supporting FS and HS and
my question is what happens with the HS support on function A
(descriptors, endpoints, etc) when you connect it to a HS hub or to a FS
hub.
> function B supporting FS only.
> Configuration #2 with
> function B supporting FS only.
> Configuration #3 with
> function A supporting FS and HS and
> function C supporting HS only (dunno if that makes sense).
>
> When connected to FS hub, we will return the following:
>
> Configuration #1: A, B
> Configuration #2: B
> Configuration #3: A
>
> When connect to HS hub, we will return the following:
>
> Configuration #1: A
See, I'm not sure this is allowed by USB spec. But I could be wrong. (I
mean having a function disappear)
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv3 2/4] usb: gadget: replace "is_dualspeed" with "max_speed"
2011-08-23 15:43 ` Alan Stern
@ 2011-08-23 17:21 ` Felipe Balbi
2011-08-23 18:00 ` Alan Stern
0 siblings, 1 reply; 70+ messages in thread
From: Felipe Balbi @ 2011-08-23 17:21 UTC (permalink / raw)
To: Alan Stern
Cc: Felipe Balbi, Michal Nazarewicz, Sebastian Andrzej Siewior,
Yang Rui Rui, Dave Young, linux-usb, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 823 bytes --]
Hi,
On Tue, Aug 23, 2011 at 11:43:44AM -0400, Alan Stern wrote:
> > if they are both part of the of the same configuration, full speed.
> > Otherwise Audio won't work. If they are part of different
> > configurations, then maximum_speed will be defined by the chosen config,
> > no ?
>
> No. The speed has to be chosen first, when the gadget connects to the
> host. The config isn't chosen until later, after the host retrieves
> the various descriptors and decides among them.
true
> > I stand with the fact that if they are part of the same configuration,
> > there's no way around it, storage will have to come down to full speed.
> > Unless I'm missing something.
>
> What if they are in different configurations?
then you use the fastest speed between both configurations?
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv3 2/4] usb: gadget: replace "is_dualspeed" with "max_speed"
2011-08-23 17:21 ` Felipe Balbi
@ 2011-08-23 18:00 ` Alan Stern
2011-08-23 19:05 ` Michal Nazarewicz
0 siblings, 1 reply; 70+ messages in thread
From: Alan Stern @ 2011-08-23 18:00 UTC (permalink / raw)
To: Felipe Balbi
Cc: Michal Nazarewicz, Sebastian Andrzej Siewior, Yang Rui Rui,
Dave Young, linux-usb, linux-kernel
On Tue, 23 Aug 2011, Felipe Balbi wrote:
> Hi,
>
> On Tue, Aug 23, 2011 at 11:43:44AM -0400, Alan Stern wrote:
> > > if they are both part of the of the same configuration, full speed.
> > > Otherwise Audio won't work. If they are part of different
> > > configurations, then maximum_speed will be defined by the chosen config,
> > > no ?
> >
> > No. The speed has to be chosen first, when the gadget connects to the
> > host. The config isn't chosen until later, after the host retrieves
> > the various descriptors and decides among them.
>
> true
>
> > > I stand with the fact that if they are part of the same configuration,
> > > there's no way around it, storage will have to come down to full speed.
> > > Unless I'm missing something.
> >
> > What if they are in different configurations?
>
> then you use the fastest speed between both configurations?
I simply don't know. It's a difficult problem.
For the time being, perhaps the best answer is to use the _lowest_
max_speed value among all the function drivers. It's the simplest
answer and most likely to be correct most of the time.
If anybody ever does want to put together a composite gadget that runs
faster than full speed but still provides a midi function ... well,
they'll just have to update the midi gadget driver.
(Actually that's a bogus example. It turns out that gmidi.c doesn't
support the composite framework. Oh well.)
Alan Stern
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv3 2/4] usb: gadget: replace "is_dualspeed" with "max_speed"
2011-08-23 17:19 ` Felipe Balbi
@ 2011-08-23 18:44 ` Michal Nazarewicz
0 siblings, 0 replies; 70+ messages in thread
From: Michal Nazarewicz @ 2011-08-23 18:44 UTC (permalink / raw)
To: Felipe Balbi
Cc: Alan Stern, Sebastian Andrzej Siewior, Yang Rui Rui, Dave Young,
linux-usb, linux-kernel
>> On Tue, 23 Aug 2011 17:11:06 +0200, Felipe Balbi <balbi@ti.com> wrote:
>>> I'm trying to think if this is really what we want. This might mean
>>> $current_config will be different depending if we connect it to SS, HS
>>> or FS hubs.
> On Tue, Aug 23, 2011 at 05:26:24PM +0200, Michal Nazarewicz wrote:
>> What do you mean by $current_config? The bConfigurationValue will be
On Tue, 23 Aug 2011 19:19:43 +0200, Felipe Balbi <balbi@ti.com> wrote:
> the currently chosen configuration.
So yes, it potentially ma be different depending on the speed.
>> the same for "the same" configuration regardless of speed but different
>> configurations may be available for different speeds and "the same"
>> configuration in various speeds might have different functions.
>>
>> So just for the sake of having something concrete, let's say we have:
>>
>> Configuration #1 with
>> function A supporting FS and HS and
> my question is what happens with the HS support on function A
> (descriptors, endpoints, etc) when you connect it to a HS hub or to a FS
> hub.
When you connect it to HS hub you get it's HS descriptors (and FS
descriptors
if you request “other speed descriptors”). When you connect it to FS hub
you
get it's FS descriptors.
>> function B supporting FS only.
>> Configuration #2 with
>> function B supporting FS only.
>> Configuration #3 with
>> function A supporting FS and HS and
>> function C supporting HS only (dunno if that makes sense).
>>
>> When connected to FS hub, we will return the following:
>>
>> Configuration #1: A, B
>> Configuration #2: B
>> Configuration #3: A
>>
>> When connect to HS hub, we will return the following:
>>
>> Configuration #1: A
>
> See, I'm not sure this is allowed by USB spec. But I could be wrong. (I
> mean having a function disappear)
I'm hardly an expert so I cannot address that concern.
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michal "mina86" Nazarewicz (o o)
ooo +-----<email/xmpp: mnazarewicz@google.com>-----ooO--(_)--Ooo--
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv3 2/4] usb: gadget: replace "is_dualspeed" with "max_speed"
2011-08-23 18:00 ` Alan Stern
@ 2011-08-23 19:05 ` Michal Nazarewicz
2011-08-23 20:49 ` Alan Stern
0 siblings, 1 reply; 70+ messages in thread
From: Michal Nazarewicz @ 2011-08-23 19:05 UTC (permalink / raw)
To: Felipe Balbi, Alan Stern
Cc: Sebastian Andrzej Siewior, Yang Rui Rui, Dave Young, linux-usb,
linux-kernel
On Tue, 23 Aug 2011 20:00:25 +0200, Alan Stern <stern@rowland.harvard.edu>
wrote:
> For the time being, perhaps the best answer is to use the _lowest_
> max_speed value among all the function drivers. It's the simplest
> answer and most likely to be correct most of the time.
In theory, one could create a composite function with only SS/HS
descriptors. So it would work for SS/HS but not for FS. I'm not
saying that it would make sense (or that it's not a bug) but that
adds to the discussion.
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michal "mina86" Nazarewicz (o o)
ooo +-----<email/xmpp: mnazarewicz@google.com>-----ooO--(_)--Ooo--
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv3 2/4] usb: gadget: replace "is_dualspeed" with "max_speed"
2011-08-23 19:05 ` Michal Nazarewicz
@ 2011-08-23 20:49 ` Alan Stern
2011-08-24 8:56 ` Felipe Balbi
2011-08-24 13:10 ` Michal Nazarewicz
0 siblings, 2 replies; 70+ messages in thread
From: Alan Stern @ 2011-08-23 20:49 UTC (permalink / raw)
To: Michal Nazarewicz
Cc: Felipe Balbi, Sebastian Andrzej Siewior, Yang Rui Rui,
Dave Young, linux-usb, linux-kernel
On Tue, 23 Aug 2011, Michal Nazarewicz wrote:
> On Tue, 23 Aug 2011 20:00:25 +0200, Alan Stern <stern@rowland.harvard.edu>
> wrote:
> > For the time being, perhaps the best answer is to use the _lowest_
> > max_speed value among all the function drivers. It's the simplest
> > answer and most likely to be correct most of the time.
>
> In theory, one could create a composite function with only SS/HS
> descriptors.
The only reason for doing that would be if the function was incapable
of carrying out its duties when running at full speed (insufficient
bandwidth, for example). Something like this is discussed briefly in
section 9.2.6.6 of the USB-2 spec.
> So it would work for SS/HS but not for FS. I'm not
> saying that it would make sense (or that it's not a bug) but that
> adds to the discussion.
If somebody writes a composite gadget containing two functions, one of
which supports only full speed and the other only high speed, they'll
get what they deserve. :-)
Still, maybe I was wrong. Maybe it would be better to use the fastest
speed supported by at least one of the function drivers. The user can
always force a SuperSpeed-capable device to run at high speed by using
a USB-2 cable to plug it in. I'm not sure whether the user could force
such a device to run at full speed, however.
Alan Stern
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv3 2/4] usb: gadget: replace "is_dualspeed" with "max_speed"
2011-08-23 20:49 ` Alan Stern
@ 2011-08-24 8:56 ` Felipe Balbi
2011-08-24 13:10 ` Michal Nazarewicz
1 sibling, 0 replies; 70+ messages in thread
From: Felipe Balbi @ 2011-08-24 8:56 UTC (permalink / raw)
To: Alan Stern
Cc: Michal Nazarewicz, Felipe Balbi, Sebastian Andrzej Siewior,
Yang Rui Rui, Dave Young, linux-usb, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1572 bytes --]
Hi,
On Tue, Aug 23, 2011 at 04:49:37PM -0400, Alan Stern wrote:
> On Tue, 23 Aug 2011, Michal Nazarewicz wrote:
>
> > On Tue, 23 Aug 2011 20:00:25 +0200, Alan Stern <stern@rowland.harvard.edu>
> > wrote:
> > > For the time being, perhaps the best answer is to use the _lowest_
> > > max_speed value among all the function drivers. It's the simplest
> > > answer and most likely to be correct most of the time.
> >
> > In theory, one could create a composite function with only SS/HS
> > descriptors.
>
> The only reason for doing that would be if the function was incapable
> of carrying out its duties when running at full speed (insufficient
> bandwidth, for example). Something like this is discussed briefly in
> section 9.2.6.6 of the USB-2 spec.
>
> > So it would work for SS/HS but not for FS. I'm not
> > saying that it would make sense (or that it's not a bug) but that
> > adds to the discussion.
>
> If somebody writes a composite gadget containing two functions, one of
> which supports only full speed and the other only high speed, they'll
> get what they deserve. :-)
>
> Still, maybe I was wrong. Maybe it would be better to use the fastest
> speed supported by at least one of the function drivers. The user can
> always force a SuperSpeed-capable device to run at high speed by using
> a USB-2 cable to plug it in. I'm not sure whether the user could force
> such a device to run at full speed, however.
plug it behind a full speed hub, like those that usually come on USB
keyboards.
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv3 2/4] usb: gadget: replace "is_dualspeed" with "max_speed"
2011-08-23 20:49 ` Alan Stern
2011-08-24 8:56 ` Felipe Balbi
@ 2011-08-24 13:10 ` Michal Nazarewicz
2011-08-24 14:31 ` Alan Stern
1 sibling, 1 reply; 70+ messages in thread
From: Michal Nazarewicz @ 2011-08-24 13:10 UTC (permalink / raw)
To: Alan Stern
Cc: Felipe Balbi, Sebastian Andrzej Siewior, Yang Rui Rui,
Dave Young, linux-usb, linux-kernel
On Tue, 23 Aug 2011 22:49:37 +0200, Alan Stern <stern@rowland.harvard.edu>
wrote:
> Still, maybe I was wrong. Maybe it would be better to use the fastest
> speed supported by at least one of the function drivers. The user can
> always force a SuperSpeed-capable device to run at high speed by using
> a USB-2 cable to plug it in. I'm not sure whether the user could force
> such a device to run at full speed, however.
I'm actually wondering whether we really need to worry about it.
composite_driver has the “max_speed” field and I would just leave it as
composite driver's author responsibility to put correct value there.
One thing that could be worth doing is iterate over all the configurations
and figure out if at least one supports the speed declared by “max_speed”
and if not, lower it.
Like I've said before, as additional functionality, composite.c could
check if it's USB_SPEED_UNKWONW in which case it would look for the lowest
speed that all the functions support.
Such configuration would allow composite driver authors set the speed to
USB_SPEED_SUPER when they mean “choose maximum speed at least one function
supports” and to “USB_SPEED_UNKWONW” when they mean “choose maximum speed
all the functions support”.
Does that make sense?
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michal "mina86" Nazarewicz (o o)
ooo +-----<email/xmpp: mnazarewicz@google.com>-----ooO--(_)--Ooo--
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv3 2/4] usb: gadget: replace "is_dualspeed" with "max_speed"
2011-08-24 13:10 ` Michal Nazarewicz
@ 2011-08-24 14:31 ` Alan Stern
2011-08-24 14:53 ` Michal Nazarewicz
2011-08-24 22:57 ` Felipe Balbi
0 siblings, 2 replies; 70+ messages in thread
From: Alan Stern @ 2011-08-24 14:31 UTC (permalink / raw)
To: Michal Nazarewicz
Cc: Felipe Balbi, Sebastian Andrzej Siewior, Yang Rui Rui,
Dave Young, linux-usb, linux-kernel
On Wed, 24 Aug 2011, Michal Nazarewicz wrote:
> On Tue, 23 Aug 2011 22:49:37 +0200, Alan Stern <stern@rowland.harvard.edu>
> wrote:
> > Still, maybe I was wrong. Maybe it would be better to use the fastest
> > speed supported by at least one of the function drivers. The user can
> > always force a SuperSpeed-capable device to run at high speed by using
> > a USB-2 cable to plug it in. I'm not sure whether the user could force
> > such a device to run at full speed, however.
>
> I'm actually wondering whether we really need to worry about it.
> composite_driver has the “max_speed” field and I would just leave it as
> composite driver's author responsibility to put correct value there.
>
> One thing that could be worth doing is iterate over all the configurations
> and figure out if at least one supports the speed declared by “max_speed”
> and if not, lower it.
That's the same as what I said: Use the highest speed supported by any
function driver. There really isn't any point in allowing connections
faster than that. And as Felipe pointed out, the user always has the
capability to force the connection to be slower, by using the right
sorts of cables and hubs.
> Like I've said before, as additional functionality, composite.c could
> check if it's USB_SPEED_UNKWONW in which case it would look for the lowest
> speed that all the functions support.
>
> Such configuration would allow composite driver authors set the speed to
> USB_SPEED_SUPER when they mean “choose maximum speed at least one function
> supports” and to “USB_SPEED_UNKWONW” when they mean “choose maximum speed
> all the functions support”.
>
> Does that make sense?
It's reasonable. Just be sure to document it properly; otherwise
nobody will understand or remember it!
Alan Stern
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv3 2/4] usb: gadget: replace "is_dualspeed" with "max_speed"
2011-08-24 14:31 ` Alan Stern
@ 2011-08-24 14:53 ` Michal Nazarewicz
2011-08-24 15:15 ` Alan Stern
2011-08-24 22:57 ` Felipe Balbi
1 sibling, 1 reply; 70+ messages in thread
From: Michal Nazarewicz @ 2011-08-24 14:53 UTC (permalink / raw)
To: Alan Stern
Cc: Felipe Balbi, Sebastian Andrzej Siewior, Yang Rui Rui,
Dave Young, linux-usb, linux-kernel
> On Wed, 24 Aug 2011, Michal Nazarewicz wrote:
>> I'm actually wondering whether we really need to worry about it.
>> composite_driver has the “max_speed” field and I would just leave it as
>> composite driver's author responsibility to put correct value there.
>>
>> One thing that could be worth doing is iterate over all the
>> configurations and figure out if at least one supports the speed
>> declared by “max_speed” and if not, lower it.
On Wed, 24 Aug 2011 16:31:27 +0200, Alan Stern <stern@rowland.harvard.edu>
wrote:
> That's the same as what I said: Use the highest speed supported by any
> function driver.
Right.
>> Like I've said before, as additional functionality, composite.c could
>> check if it's USB_SPEED_UNKWONW in which case it would look for the
>> lowest speed that all the functions support.
> It's reasonable. Just be sure to document it properly; otherwise
> nobody will understand or remember it!
I've found where my reasoning was faulty. The usb_gadget_driver's
max_speed is set before all the functions get added so composite.c has
no way to figure those things in advance. That's why we need to relay
on usb_composite_driver's max_speed be set to a proper value.
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michal "mina86" Nazarewicz (o o)
ooo +-----<email/xmpp: mnazarewicz@google.com>-----ooO--(_)--Ooo--
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv3 2/4] usb: gadget: replace "is_dualspeed" with "max_speed"
2011-08-24 14:53 ` Michal Nazarewicz
@ 2011-08-24 15:15 ` Alan Stern
2011-08-24 15:25 ` Michal Nazarewicz
0 siblings, 1 reply; 70+ messages in thread
From: Alan Stern @ 2011-08-24 15:15 UTC (permalink / raw)
To: Michal Nazarewicz
Cc: Felipe Balbi, Sebastian Andrzej Siewior, Yang Rui Rui,
Dave Young, linux-usb, linux-kernel
On Wed, 24 Aug 2011, Michal Nazarewicz wrote:
> I've found where my reasoning was faulty. The usb_gadget_driver's
> max_speed is set before all the functions get added so composite.c has
> no way to figure those things in advance. That's why we need to relay
> on usb_composite_driver's max_speed be set to a proper value.
This is what Felipe was complaining about earlier. We shouldn't set
max_speed or allow the UDC to connect to the host until all the
functions have been added.
Alan Stern
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv3 2/4] usb: gadget: replace "is_dualspeed" with "max_speed"
2011-08-24 15:15 ` Alan Stern
@ 2011-08-24 15:25 ` Michal Nazarewicz
2011-08-24 23:04 ` Felipe Balbi
0 siblings, 1 reply; 70+ messages in thread
From: Michal Nazarewicz @ 2011-08-24 15:25 UTC (permalink / raw)
To: Alan Stern
Cc: Felipe Balbi, Sebastian Andrzej Siewior, Yang Rui Rui,
Dave Young, linux-usb, linux-kernel
On Wed, 24 Aug 2011 17:15:29 +0200, Alan Stern <stern@rowland.harvard.edu>
wrote:
> On Wed, 24 Aug 2011, Michal Nazarewicz wrote:
>
>> I've found where my reasoning was faulty. The usb_gadget_driver's
>> max_speed is set before all the functions get added so composite.c has
>> no way to figure those things in advance. That's why we need to relay
>> on usb_composite_driver's max_speed be set to a proper value.
>
> This is what Felipe was complaining about earlier. We shouldn't set
> max_speed or allow the UDC to connect to the host until all the
> functions have been added.
We're not doing the latter. The functions are added in bind callback so
(hopefully) UDC will first run the bind callback and then try connecting
to host.
And the former would be fixed if we allowed gadget driver to set max_speed
in bind callback rather then prior to calling usb_gadget_probe_driver().
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michal "mina86" Nazarewicz (o o)
ooo +-----<email/xmpp: mnazarewicz@google.com>-----ooO--(_)--Ooo--
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv3 2/4] usb: gadget: replace "is_dualspeed" with "max_speed"
2011-08-24 14:31 ` Alan Stern
2011-08-24 14:53 ` Michal Nazarewicz
@ 2011-08-24 22:57 ` Felipe Balbi
1 sibling, 0 replies; 70+ messages in thread
From: Felipe Balbi @ 2011-08-24 22:57 UTC (permalink / raw)
To: Alan Stern
Cc: Michal Nazarewicz, Felipe Balbi, Sebastian Andrzej Siewior,
Yang Rui Rui, Dave Young, linux-usb, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 2025 bytes --]
On Wed, Aug 24, 2011 at 10:31:27AM -0400, Alan Stern wrote:
> On Wed, 24 Aug 2011, Michal Nazarewicz wrote:
>
> > On Tue, 23 Aug 2011 22:49:37 +0200, Alan Stern <stern@rowland.harvard.edu>
> > wrote:
> > > Still, maybe I was wrong. Maybe it would be better to use the fastest
> > > speed supported by at least one of the function drivers. The user can
> > > always force a SuperSpeed-capable device to run at high speed by using
> > > a USB-2 cable to plug it in. I'm not sure whether the user could force
> > > such a device to run at full speed, however.
> >
> > I'm actually wondering whether we really need to worry about it.
> > composite_driver has the “max_speed” field and I would just leave it as
> > composite driver's author responsibility to put correct value there.
> >
> > One thing that could be worth doing is iterate over all the configurations
> > and figure out if at least one supports the speed declared by “max_speed”
> > and if not, lower it.
>
> That's the same as what I said: Use the highest speed supported by any
> function driver. There really isn't any point in allowing connections
> faster than that. And as Felipe pointed out, the user always has the
> capability to force the connection to be slower, by using the right
> sorts of cables and hubs.
>
> > Like I've said before, as additional functionality, composite.c could
> > check if it's USB_SPEED_UNKWONW in which case it would look for the lowest
> > speed that all the functions support.
> >
> > Such configuration would allow composite driver authors set the speed to
> > USB_SPEED_SUPER when they mean “choose maximum speed at least one function
> > supports” and to “USB_SPEED_UNKWONW” when they mean “choose maximum speed
> > all the functions support”.
> >
> > Does that make sense?
>
> It's reasonable. Just be sure to document it properly; otherwise
> nobody will understand or remember it!
good point. I agree with this approach.
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv3 2/4] usb: gadget: replace "is_dualspeed" with "max_speed"
2011-08-24 15:25 ` Michal Nazarewicz
@ 2011-08-24 23:04 ` Felipe Balbi
2011-08-25 12:46 ` Michal Nazarewicz
0 siblings, 1 reply; 70+ messages in thread
From: Felipe Balbi @ 2011-08-24 23:04 UTC (permalink / raw)
To: Michal Nazarewicz
Cc: Alan Stern, Felipe Balbi, Sebastian Andrzej Siewior,
Yang Rui Rui, Dave Young, linux-usb, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 2426 bytes --]
Hi,
On Wed, Aug 24, 2011 at 05:25:11PM +0200, Michal Nazarewicz wrote:
> On Wed, 24 Aug 2011 17:15:29 +0200, Alan Stern
> <stern@rowland.harvard.edu> wrote:
>
> >On Wed, 24 Aug 2011, Michal Nazarewicz wrote:
> >
> >>I've found where my reasoning was faulty. The usb_gadget_driver's
> >>max_speed is set before all the functions get added so composite.c has
> >>no way to figure those things in advance. That's why we need to relay
> >>on usb_composite_driver's max_speed be set to a proper value.
> >
> >This is what Felipe was complaining about earlier. We shouldn't set
> >max_speed or allow the UDC to connect to the host until all the
> >functions have been added.
>
> We're not doing the latter. The functions are added in bind callback so
> (hopefully) UDC will first run the bind callback and then try connecting
> to host.
>
> And the former would be fixed if we allowed gadget driver to set max_speed
> in bind callback rather then prior to calling usb_gadget_probe_driver().
there's one catch. As of today, we always start UDCs with data pullups
connected, which means that we could connect to a host even before a
gadget driver is loaded. My point in moving to udc_start/udc_stop is
that the above would be take care of. See udc-core.c:
int usb_gadget_probe_driver(struct usb_gadget_driver *driver,
int (*bind)(struct usb_gadget *))
{
....
if (udc_is_newstyle(udc)) {
ret = bind(udc->gadget);
if (ret)
goto err1;
ret = usb_gadget_udc_start(udc->gadget, driver);
if (ret) {
driver->unbind(udc->gadget);
goto err1;
}
usb_gadget_connect(udc->gadget);
} else {
ret = usb_gadget_start(udc->gadget, driver, bind);
if (ret)
goto err1;
}
...
}
If all UDCs are converted to udc_start()/udc_stop() we get the guarantee
that they will only conect to host after gadget driver is fully loaded
for free. We can also, finally, properly use the
usb_function_deactivate/usb_function_activate properly. So for each
registered function, composite.c calls usb_function_deactivate() and
function is _required_ to call usb_function_activate when it's ready
(for f_obex.c that would be when userland OBEX stack has opened
/dev/ttyGS*, for g_mass_storage that would be on function bind(), and so
on).
Then, when on gadget driver's bind() we can take this kind of spee
decision and pass that on to UDC driver.
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv3 2/4] usb: gadget: replace "is_dualspeed" with "max_speed"
2011-08-24 23:04 ` Felipe Balbi
@ 2011-08-25 12:46 ` Michal Nazarewicz
2011-08-25 12:53 ` Felipe Balbi
0 siblings, 1 reply; 70+ messages in thread
From: Michal Nazarewicz @ 2011-08-25 12:46 UTC (permalink / raw)
To: Felipe Balbi
Cc: Alan Stern, Sebastian Andrzej Siewior, Yang Rui Rui, Dave Young,
linux-usb, linux-kernel
On Thu, 25 Aug 2011 01:04:19 +0200, Felipe Balbi <balbi@ti.com> wrote:
> there's one catch. As of today, we always start UDCs with data pullups
> connected, which means that we could connect to a host even before a
> gadget driver is loaded. My point in moving to udc_start/udc_stop is
> that the above would be take care of. See udc-core.c: [...]
Honestly I'm not quite sure why udc_start/udc_stop is needed here. Even
without those the UDC driver can start with all hw disabled and turn it
on only after the gadget driver's bind callback finishes.
> If all UDCs are converted to udc_start()/udc_stop() we get the guarantee
> that they will only conect to host after gadget driver is fully loaded
> for free.
> We can also, finally, properly use the usb_function_deactivate/
> usb_function_activate properly. So for each registered function,
> composite.c calls usb_function_deactivate() and function is _required_
> to call usb_function_activate when it's ready.
I'm not really sure why that would be beneficial. Also, it would still
require disconnect-connect cycle if some function decides to (de)activate
itself while gadget is connected.
> Then, when on gadget driver's bind() we can take this kind of speed
> decision and pass that on to UDC driver.
So can we leave things as they are for now and wait for UDCs to be
converted
and once this is done, do all kinds of magic we want in copomiset's bind
callback?
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michal "mina86" Nazarewicz (o o)
ooo +-----<email/xmpp: mnazarewicz@google.com>-----ooO--(_)--Ooo--
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCHv3 2/4] usb: gadget: replace "is_dualspeed" with "max_speed"
2011-08-25 12:46 ` Michal Nazarewicz
@ 2011-08-25 12:53 ` Felipe Balbi
0 siblings, 0 replies; 70+ messages in thread
From: Felipe Balbi @ 2011-08-25 12:53 UTC (permalink / raw)
To: Michal Nazarewicz
Cc: Felipe Balbi, Alan Stern, Sebastian Andrzej Siewior,
Yang Rui Rui, Dave Young, linux-usb, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1600 bytes --]
Hi,
On Thu, Aug 25, 2011 at 02:46:55PM +0200, Michal Nazarewicz wrote:
> On Thu, 25 Aug 2011 01:04:19 +0200, Felipe Balbi <balbi@ti.com> wrote:
> >there's one catch. As of today, we always start UDCs with data pullups
> >connected, which means that we could connect to a host even before a
> >gadget driver is loaded. My point in moving to udc_start/udc_stop is
> >that the above would be take care of. See udc-core.c: [...]
>
> Honestly I'm not quite sure why udc_start/udc_stop is needed here. Even
> without those the UDC driver can start with all hw disabled and turn it
> on only after the gadget driver's bind callback finishes.
>
> >If all UDCs are converted to udc_start()/udc_stop() we get the guarantee
> >that they will only conect to host after gadget driver is fully loaded
> >for free.
>
> >We can also, finally, properly use the usb_function_deactivate/
> >usb_function_activate properly. So for each registered function,
> >composite.c calls usb_function_deactivate() and function is _required_
> >to call usb_function_activate when it's ready.
>
> I'm not really sure why that would be beneficial. Also, it would still
> require disconnect-connect cycle if some function decides to (de)activate
> itself while gadget is connected.
>
> >Then, when on gadget driver's bind() we can take this kind of speed
> >decision and pass that on to UDC driver.
>
> So can we leave things as they are for now and wait for UDCs to be
> converted
> and once this is done, do all kinds of magic we want in copomiset's bind
> callback?
sure.
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply [flat|nested] 70+ messages in thread
end of thread, other threads:[~2011-08-25 12:53 UTC | newest]
Thread overview: 70+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <4E4B9D9C.2010607@linutronix.de>
2011-08-17 13:03 ` [PATCH] usb: gadget: get rid of USB_GADGET_DUALSPEED and USB_GADGET_SUPERSPEED Michal Nazarewicz
2011-08-17 14:20 ` Alan Stern
2011-08-17 14:27 ` Michal Nazarewicz
2011-08-17 14:47 ` Alan Stern
2011-08-17 15:07 ` Michal Nazarewicz
2011-08-17 16:25 ` Alan Stern
2011-08-17 14:36 ` Sergei Shtylyov
2011-08-17 14:45 ` Michal Nazarewicz
2011-08-17 15:33 ` [PATCHv2] " Michal Nazarewicz
2011-08-17 21:09 ` Alan Stern
2011-08-18 13:19 ` Michal Nazarewicz
2011-08-18 14:59 ` Alan Stern
2011-08-18 17:05 ` Michal Nazarewicz
2011-08-18 17:27 ` Alan Stern
2011-08-18 20:13 ` Michal Nazarewicz
2011-08-18 20:30 ` Alan Stern
2011-08-18 20:44 ` Michal Nazarewicz
2011-08-19 10:53 ` Michal Nazarewicz
2011-08-19 11:13 ` Sebastian Andrzej Siewior
2011-08-19 12:14 ` Michal Nazarewicz
2011-08-19 14:29 ` Alan Stern
2011-08-19 14:38 ` Michal Nazarewicz
2011-08-19 14:57 ` Alan Stern
2011-08-19 2:02 ` Yang Rui Rui
2011-08-19 12:17 ` Michal Nazarewicz
2011-08-18 3:01 ` Yang Rui Rui
2011-08-18 11:57 ` Michal Nazarewicz
2011-08-18 13:24 ` Dave Young
2011-08-18 13:41 ` Michal Nazarewicz
2011-08-19 22:32 ` [PATCHv3 0/4] Figuring out speed refactorisation Michal Nazarewicz
2011-08-19 22:32 ` [PATCHv3 1/4] usb: Provide usb_device_speed_name() function Michal Nazarewicz
2011-08-19 23:15 ` Felipe Balbi
2011-08-22 14:53 ` Michal Nazarewicz
2011-08-19 22:33 ` [PATCHv3 2/4] usb: gadget: replace "is_dualspeed" with "max_speed" Michal Nazarewicz
2011-08-19 23:28 ` Felipe Balbi
2011-08-23 13:48 ` Michal Nazarewicz
2011-08-23 13:58 ` Felipe Balbi
2011-08-23 14:15 ` Michal Nazarewicz
2011-08-23 14:37 ` Alan Stern
2011-08-23 14:58 ` Felipe Balbi
2011-08-23 15:07 ` Michal Nazarewicz
2011-08-23 15:11 ` Felipe Balbi
2011-08-23 15:26 ` Michal Nazarewicz
2011-08-23 17:19 ` Felipe Balbi
2011-08-23 18:44 ` Michal Nazarewicz
2011-08-23 15:43 ` Alan Stern
2011-08-23 17:21 ` Felipe Balbi
2011-08-23 18:00 ` Alan Stern
2011-08-23 19:05 ` Michal Nazarewicz
2011-08-23 20:49 ` Alan Stern
2011-08-24 8:56 ` Felipe Balbi
2011-08-24 13:10 ` Michal Nazarewicz
2011-08-24 14:31 ` Alan Stern
2011-08-24 14:53 ` Michal Nazarewicz
2011-08-24 15:15 ` Alan Stern
2011-08-24 15:25 ` Michal Nazarewicz
2011-08-24 23:04 ` Felipe Balbi
2011-08-25 12:46 ` Michal Nazarewicz
2011-08-25 12:53 ` Felipe Balbi
2011-08-24 22:57 ` Felipe Balbi
2011-08-23 15:05 ` Felipe Balbi
2011-08-23 15:30 ` Michal Nazarewicz
2011-08-19 22:33 ` [PATCHv3 3/4] usb: gadget: rename usb_gadget_driver::speed to max_speed Michal Nazarewicz
2011-08-19 23:31 ` Felipe Balbi
2011-08-20 2:34 ` Alan Stern
2011-08-22 10:42 ` Felipe Balbi
2011-08-19 22:33 ` [PATCHv3 4/4] usb: gadget: get rid of USB_GADGET_{DUAL,SUPER}SPEED Michal Nazarewicz
2011-08-20 13:41 ` Alan Stern
2011-08-22 14:51 ` Michal Nazarewicz
2011-08-22 15:03 ` Alan Stern
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.