| Message ID | 1408372115-4570-8-git-send-email-kishon@ti.com |
|---|---|
| State | New |
| Headers | show |
Hi, On Monday 18 August 2014 08:08 PM, Felipe Balbi wrote: > On Mon, Aug 18, 2014 at 07:58:29PM +0530, Kishon Vijay Abraham I wrote: >> Explicity set the max packet size in the device descriptor to 0x40 >> as specified in the device class specification for device firmware upgrade. >> Also changed debug to printf to explicitly notify the user if the >> device has been enumerated. >> >> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> >> --- >> drivers/usb/gadget/composite.c | 3 +-- >> drivers/usb/gadget/g_dnl.c | 1 + >> 2 files changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c >> index 7bd2562..60ca9a5 100644 >> --- a/drivers/usb/gadget/composite.c >> +++ b/drivers/usb/gadget/composite.c >> @@ -331,7 +331,7 @@ static int set_config(struct usb_composite_dev *cdev, >> } else >> result = 0; >> >> - debug("%s: %s speed config #%d: %s\n", __func__, >> + printf("%s: %s speed config #%d: %s\n", __func__, > > this looks like it's only needed for debugging. No strong feelings > though yeah.. it helps to know if the device has been enumerated. > >> @@ -987,7 +987,6 @@ static int composite_bind(struct usb_gadget *gadget) >> >> memcpy(&cdev->desc, composite->dev, >> sizeof(struct usb_device_descriptor)); >> - cdev->desc.bMaxPacketSize0 = gadget->ep0->maxpacket; > > why are you dropping this line ? Actually faced an issue with this. DWC3 gadget sets ep0 maxpacket size to 512 and if the device is operating at hight speed, the bMaxPacketSize0 field should be 64? If the maxpacket size is set to 512, the host reports Invalid EP0 Maxpacket. Thanks Kishon
On Monday 18 August 2014 08:26 PM, Lukasz Majewski wrote: > Hi Kishon, > >>> Explicity set the max packet size in the device descriptor to 0x40 >>> as specified in the device class specification for device firmware >>> upgrade. Also changed debug to printf to explicitly notify the user >>> if the device has been enumerated. > > It seems like this patch slipped without my attention ... > > Hence, I'd like to ask why you must hardcode the packet size here? http://www.usb.org/developers/devclass_docs/usbdfu10.pdf (in section 4.2.1 DFU Mode Device Descriptor), specifies the bMaxPacketSize0 can be 8,16,32,64. And the value we get from gadget is 512. So hard coded it. Thanks Kishon
Hi, On Tue, Aug 19, 2014 at 09:04:06PM +0530, Kishon Vijay Abraham I wrote: > On Monday 18 August 2014 08:08 PM, Felipe Balbi wrote: > > On Mon, Aug 18, 2014 at 07:58:29PM +0530, Kishon Vijay Abraham I wrote: > >> Explicity set the max packet size in the device descriptor to 0x40 > >> as specified in the device class specification for device firmware upgrade. > >> Also changed debug to printf to explicitly notify the user if the > >> device has been enumerated. > >> > >> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> > >> --- > >> drivers/usb/gadget/composite.c | 3 +-- > >> drivers/usb/gadget/g_dnl.c | 1 + > >> 2 files changed, 2 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c > >> index 7bd2562..60ca9a5 100644 > >> --- a/drivers/usb/gadget/composite.c > >> +++ b/drivers/usb/gadget/composite.c > >> @@ -331,7 +331,7 @@ static int set_config(struct usb_composite_dev *cdev, > >> } else > >> result = 0; > >> > >> - debug("%s: %s speed config #%d: %s\n", __func__, > >> + printf("%s: %s speed config #%d: %s\n", __func__, > > > > this looks like it's only needed for debugging. No strong feelings > > though > > yeah.. it helps to know if the device has been enumerated. > > > >> @@ -987,7 +987,6 @@ static int composite_bind(struct usb_gadget *gadget) > >> > >> memcpy(&cdev->desc, composite->dev, > >> sizeof(struct usb_device_descriptor)); > >> - cdev->desc.bMaxPacketSize0 = gadget->ep0->maxpacket; > > > > why are you dropping this line ? > > Actually faced an issue with this. DWC3 gadget sets ep0 maxpacket size > to 512 and if the device is operating at hight speed, the > bMaxPacketSize0 field should be 64? right, but that's changed on connection done IRQ. > If the maxpacket size is set to 512, the host reports Invalid EP0 > Maxpacket. huh ? who broke dwc3 ? connection done happens before we receive the first setup packet, how can host complain about anything ?
On Tuesday 19 August 2014 09:09 PM, Felipe Balbi wrote: > Hi, > > On Tue, Aug 19, 2014 at 09:04:06PM +0530, Kishon Vijay Abraham I wrote: >> On Monday 18 August 2014 08:08 PM, Felipe Balbi wrote: >>> On Mon, Aug 18, 2014 at 07:58:29PM +0530, Kishon Vijay Abraham I wrote: >>>> Explicity set the max packet size in the device descriptor to 0x40 >>>> as specified in the device class specification for device firmware upgrade. >>>> Also changed debug to printf to explicitly notify the user if the >>>> device has been enumerated. >>>> >>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> >>>> --- >>>> drivers/usb/gadget/composite.c | 3 +-- >>>> drivers/usb/gadget/g_dnl.c | 1 + >>>> 2 files changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c >>>> index 7bd2562..60ca9a5 100644 >>>> --- a/drivers/usb/gadget/composite.c >>>> +++ b/drivers/usb/gadget/composite.c >>>> @@ -331,7 +331,7 @@ static int set_config(struct usb_composite_dev *cdev, >>>> } else >>>> result = 0; >>>> >>>> - debug("%s: %s speed config #%d: %s\n", __func__, >>>> + printf("%s: %s speed config #%d: %s\n", __func__, >>> >>> this looks like it's only needed for debugging. No strong feelings >>> though >> >> yeah.. it helps to know if the device has been enumerated. >>> >>>> @@ -987,7 +987,6 @@ static int composite_bind(struct usb_gadget *gadget) >>>> >>>> memcpy(&cdev->desc, composite->dev, >>>> sizeof(struct usb_device_descriptor)); >>>> - cdev->desc.bMaxPacketSize0 = gadget->ep0->maxpacket; >>> >>> why are you dropping this line ? >> >> Actually faced an issue with this. DWC3 gadget sets ep0 maxpacket size >> to 512 and if the device is operating at hight speed, the >> bMaxPacketSize0 field should be 64? > > right, but that's changed on connection done IRQ. > >> If the maxpacket size is set to 512, the host reports Invalid EP0 >> Maxpacket. > > huh ? who broke dwc3 ? connection done happens before we receive the > first setup packet, how can host complain about anything ? > composite_bind() (where maxpacket is being set) happens much before connection done interrupt. So it's not dwc3's fault. Thanks Kishon
On Tue, Aug 19, 2014 at 09:08:00PM +0530, Kishon Vijay Abraham I wrote: > > > On Monday 18 August 2014 08:26 PM, Lukasz Majewski wrote: > > Hi Kishon, > > > >>> Explicity set the max packet size in the device descriptor to 0x40 > >>> as specified in the device class specification for device firmware > >>> upgrade. Also changed debug to printf to explicitly notify the user > >>> if the device has been enumerated. > > > > It seems like this patch slipped without my attention ... > > > > Hence, I'd like to ask why you must hardcode the packet size here? > > http://www.usb.org/developers/devclass_docs/usbdfu10.pdf (in section 4.2.1 DFU > Mode Device Descriptor), specifies the bMaxPacketSize0 can be 8,16,32,64. And > the value we get from gadget is 512. So hard coded it. as I mentioned, this should be changed on connection done interrupt. DFU has one of the poorest documentations for a USB Class and has some moronic definitions. What happens if you want ot run DFU with a superspeed controller (such as dwc3) when connected to a superspeed port. We cannot use ep0 packet size as 64, right ? That's why hardcoding is bad ;-)
Hi, On Tue, Aug 19, 2014 at 09:12:29PM +0530, Kishon Vijay Abraham I wrote: > >>>> @@ -987,7 +987,6 @@ static int composite_bind(struct usb_gadget *gadget) > >>>> > >>>> memcpy(&cdev->desc, composite->dev, > >>>> sizeof(struct usb_device_descriptor)); > >>>> - cdev->desc.bMaxPacketSize0 = gadget->ep0->maxpacket; > >>> > >>> why are you dropping this line ? > >> > >> Actually faced an issue with this. DWC3 gadget sets ep0 maxpacket size > >> to 512 and if the device is operating at hight speed, the > >> bMaxPacketSize0 field should be 64? > > > > right, but that's changed on connection done IRQ. > > > >> If the maxpacket size is set to 512, the host reports Invalid EP0 > >> Maxpacket. > > > > huh ? who broke dwc3 ? connection done happens before we receive the > > first setup packet, how can host complain about anything ? > > > composite_bind() (where maxpacket is being set) happens much before connection > done interrupt. So it's not dwc3's fault. seems like you're just missing a few fixes to composite.c, such as commit 765f5b830e547229bb752e7b232ee83e2b3d49d5
On Tuesday 19 August 2014 09:18 PM, Felipe Balbi wrote: > Hi, > > On Tue, Aug 19, 2014 at 09:12:29PM +0530, Kishon Vijay Abraham I wrote: >>>>>> @@ -987,7 +987,6 @@ static int composite_bind(struct usb_gadget *gadget) >>>>>> >>>>>> memcpy(&cdev->desc, composite->dev, >>>>>> sizeof(struct usb_device_descriptor)); >>>>>> - cdev->desc.bMaxPacketSize0 = gadget->ep0->maxpacket; >>>>> >>>>> why are you dropping this line ? >>>> >>>> Actually faced an issue with this. DWC3 gadget sets ep0 maxpacket size >>>> to 512 and if the device is operating at hight speed, the >>>> bMaxPacketSize0 field should be 64? >>> >>> right, but that's changed on connection done IRQ. >>> >>>> If the maxpacket size is set to 512, the host reports Invalid EP0 >>>> Maxpacket. >>> >>> huh ? who broke dwc3 ? connection done happens before we receive the >>> first setup packet, how can host complain about anything ? >>> >> composite_bind() (where maxpacket is being set) happens much before connection >> done interrupt. So it's not dwc3's fault. > > seems like you're just missing a few fixes to composite.c, such as > commit 765f5b830e547229bb752e7b232ee83e2b3d49d5 hmm.. yeah. -Kishon >
Hi Kishon, > > > On Monday 18 August 2014 08:26 PM, Lukasz Majewski wrote: > > Hi Kishon, > > > >>> Explicity set the max packet size in the device descriptor to 0x40 > >>> as specified in the device class specification for device firmware > >>> upgrade. Also changed debug to printf to explicitly notify the > >>> user if the device has been enumerated. > > > > It seems like this patch slipped without my attention ... > > > > Hence, I'd like to ask why you must hardcode the packet size here? > > http://www.usb.org/developers/devclass_docs/usbdfu10.pdf (in section > 4.2.1 DFU Mode Device Descriptor), specifies the bMaxPacketSize0 can > be 8,16,32,64. And the value we get from gadget is 512. So hard > coded it. As fair as I know ep0 MaxPacket for USB 2.0 is up to 64 bytes. This value here is set in a following way: ./drivers/usb/gadget/composite.c:990: cdev->desc.bMaxPacketSize0 = gadget->ep0->maxpacket; And in my UDC driver .maxpacket = EP0_FIFO_SIZE, for ep0 the max packet is set to 64B: s3c_udc_otg.c:782 .maxpacket = EP0_FIFO_SIZE, So you don't need to hardcode it. Another issue is how DFU works. It uses ONLY ep0 for transmission (as it was the requirement for very tiny microcontrollers). According to USB 2.0 spec the ep0 MaxPacket size is up to 64B. Considering above, DFU shall work with USB 3.0 using compliance mode with USB 2.0. For other gadgets - like USB Mass Storage - which use other EPs you should notice benefit from using USB 3.0. > > Thanks > Kishon
Hi Felipe, > On Tue, Aug 19, 2014 at 09:08:00PM +0530, Kishon Vijay Abraham I > wrote: > > > > > > On Monday 18 August 2014 08:26 PM, Lukasz Majewski wrote: > > > Hi Kishon, > > > > > >>> Explicity set the max packet size in the device descriptor to > > >>> 0x40 as specified in the device class specification for device > > >>> firmware upgrade. Also changed debug to printf to explicitly > > >>> notify the user if the device has been enumerated. > > > > > > It seems like this patch slipped without my attention ... > > > > > > Hence, I'd like to ask why you must hardcode the packet size here? > > > > http://www.usb.org/developers/devclass_docs/usbdfu10.pdf (in > > section 4.2.1 DFU Mode Device Descriptor), specifies the > > bMaxPacketSize0 can be 8,16,32,64. And the value we get from > > gadget is 512. So hard coded it. > > as I mentioned, this should be changed on connection done interrupt. In the u-boot it is set up during binding the composite. > DFU has one of the poorest documentations for a USB Class and has some > moronic definitions. Frankly speaking I prefer to have the worst standard than some "brilliant" in house developed protocols. > What happens if you want ot run DFU with a > superspeed controller (such as dwc3) when connected to a superspeed > port. We cannot use ep0 packet size as 64, right ? Correct me if I'm wrong, but MaxPacket for SS ep0 is 2^9 = 512B. I'm curious if dfu-util will manage to handle ep0 MaxPacket size up to 512B. It not, then we shall be able to fallback to 64B. > That's why > hardcoding is bad ;-) >
On Wed, Aug 20, 2014 at 09:34:13AM +0200, Lukasz Majewski wrote: > Hi Felipe, > > > On Tue, Aug 19, 2014 at 09:08:00PM +0530, Kishon Vijay Abraham I > > wrote: > > > > > > > > > On Monday 18 August 2014 08:26 PM, Lukasz Majewski wrote: > > > > Hi Kishon, > > > > > > > >>> Explicity set the max packet size in the device descriptor to > > > >>> 0x40 as specified in the device class specification for device > > > >>> firmware upgrade. Also changed debug to printf to explicitly > > > >>> notify the user if the device has been enumerated. > > > > > > > > It seems like this patch slipped without my attention ... > > > > > > > > Hence, I'd like to ask why you must hardcode the packet size here? > > > > > > http://www.usb.org/developers/devclass_docs/usbdfu10.pdf (in > > > section 4.2.1 DFU Mode Device Descriptor), specifies the > > > bMaxPacketSize0 can be 8,16,32,64. And the value we get from > > > gadget is 512. So hard coded it. > > > > as I mentioned, this should be changed on connection done interrupt. > > In the u-boot it is set up during binding the composite. look at dwc3. dwc3 has a connection done IRQ which also tells me the negotiated link speed. > > DFU has one of the poorest documentations for a USB Class and has some > > moronic definitions. > > Frankly speaking I prefer to have the worst standard than some > "brilliant" in house developed protocols. nobody is talking about changing the protocol. DFU was written back when Full speed was the fastest USB on earth. > > What happens if you want ot run DFU with a > > superspeed controller (such as dwc3) when connected to a superspeed > > port. We cannot use ep0 packet size as 64, right ? > > Correct me if I'm wrong, but MaxPacket for SS ep0 is 2^9 = 512B. > I'm curious if dfu-util will manage to handle ep0 MaxPacket size up to > 512B. > It not, then we shall be able to fallback to 64B. should be a test away.
Hi Felipe, > On Wed, Aug 20, 2014 at 09:34:13AM +0200, Lukasz Majewski wrote: > > Hi Felipe, > > > > > On Tue, Aug 19, 2014 at 09:08:00PM +0530, Kishon Vijay Abraham I > > > wrote: > > > > > > > > > > > > On Monday 18 August 2014 08:26 PM, Lukasz Majewski wrote: > > > > > Hi Kishon, > > > > > > > > > >>> Explicity set the max packet size in the device descriptor > > > > >>> to 0x40 as specified in the device class specification for > > > > >>> device firmware upgrade. Also changed debug to printf to > > > > >>> explicitly notify the user if the device has been > > > > >>> enumerated. > > > > > > > > > > It seems like this patch slipped without my attention ... > > > > > > > > > > Hence, I'd like to ask why you must hardcode the packet size > > > > > here? > > > > > > > > http://www.usb.org/developers/devclass_docs/usbdfu10.pdf (in > > > > section 4.2.1 DFU Mode Device Descriptor), specifies the > > > > bMaxPacketSize0 can be 8,16,32,64. And the value we get from > > > > gadget is 512. So hard coded it. > > > > > > as I mentioned, this should be changed on connection done > > > interrupt. > > > > In the u-boot it is set up during binding the composite. > > look at dwc3. dwc3 has a connection done IRQ which also tells me the > negotiated link speed. My point is that you will need to pool in u-boot to emulate this IRQ. > > > > DFU has one of the poorest documentations for a USB Class and has > > > some moronic definitions. > > > > Frankly speaking I prefer to have the worst standard than some > > "brilliant" in house developed protocols. > > nobody is talking about changing the protocol. DFU was written back > when Full speed was the fastest USB on earth. As you can see the standard is a bit old now... and should be revised. > > > > What happens if you want ot run DFU with a > > > superspeed controller (such as dwc3) when connected to a > > > superspeed port. We cannot use ep0 packet size as 64, right ? > > > > Correct me if I'm wrong, but MaxPacket for SS ep0 is 2^9 = 512B. > > I'm curious if dfu-util will manage to handle ep0 MaxPacket size up > > to 512B. > > It not, then we shall be able to fallback to 64B. > > should be a test away. > Ok.
On Thu, Aug 21, 2014 at 10:00:46AM +0200, Lukasz Majewski wrote: > Hi Felipe, > > > On Wed, Aug 20, 2014 at 09:34:13AM +0200, Lukasz Majewski wrote: > > > Hi Felipe, > > > > > > > On Tue, Aug 19, 2014 at 09:08:00PM +0530, Kishon Vijay Abraham I > > > > wrote: > > > > > > > > > > > > > > > On Monday 18 August 2014 08:26 PM, Lukasz Majewski wrote: > > > > > > Hi Kishon, > > > > > > > > > > > >>> Explicity set the max packet size in the device descriptor > > > > > >>> to 0x40 as specified in the device class specification for > > > > > >>> device firmware upgrade. Also changed debug to printf to > > > > > >>> explicitly notify the user if the device has been > > > > > >>> enumerated. > > > > > > > > > > > > It seems like this patch slipped without my attention ... > > > > > > > > > > > > Hence, I'd like to ask why you must hardcode the packet size > > > > > > here? > > > > > > > > > > http://www.usb.org/developers/devclass_docs/usbdfu10.pdf (in > > > > > section 4.2.1 DFU Mode Device Descriptor), specifies the > > > > > bMaxPacketSize0 can be 8,16,32,64. And the value we get from > > > > > gadget is 512. So hard coded it. > > > > > > > > as I mentioned, this should be changed on connection done > > > > interrupt. > > > > > > In the u-boot it is set up during binding the composite. > > > > look at dwc3. dwc3 has a connection done IRQ which also tells me the > > negotiated link speed. > > My point is that you will need to pool in u-boot to emulate this IRQ. you will need to poll anyways for a bunch of other things. There are some details in DWC3 which are mandated that you wait for event X before doing Y. For example control transfers, it's mandatory that you wait for a XferNotReady event before trying to start a Status Phase. > > > > DFU has one of the poorest documentations for a USB Class and has > > > > some moronic definitions. > > > > > > Frankly speaking I prefer to have the worst standard than some > > > "brilliant" in house developed protocols. > > > > nobody is talking about changing the protocol. DFU was written back > > when Full speed was the fastest USB on earth. > > As you can see the standard is a bit old now... and should be revised. right, once USB-IF does that, I hope they switch to bulk endpoints :-p
diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c index 7bd2562..60ca9a5 100644 --- a/drivers/usb/gadget/composite.c +++ b/drivers/usb/gadget/composite.c @@ -331,7 +331,7 @@ static int set_config(struct usb_composite_dev *cdev, } else result = 0; - debug("%s: %s speed config #%d: %s\n", __func__, + printf("%s: %s speed config #%d: %s\n", __func__, ({ char *speed; switch (gadget->speed) { case USB_SPEED_LOW: @@ -987,7 +987,6 @@ static int composite_bind(struct usb_gadget *gadget) memcpy(&cdev->desc, composite->dev, sizeof(struct usb_device_descriptor)); - cdev->desc.bMaxPacketSize0 = gadget->ep0->maxpacket; debug("%s: ready\n", composite->name); return 0; diff --git a/drivers/usb/gadget/g_dnl.c b/drivers/usb/gadget/g_dnl.c index 25611ac..8128b86 100644 --- a/drivers/usb/gadget/g_dnl.c +++ b/drivers/usb/gadget/g_dnl.c @@ -59,6 +59,7 @@ static struct usb_device_descriptor device_desc = { .bcdUSB = __constant_cpu_to_le16(0x0200), .bDeviceClass = USB_CLASS_COMM, .bDeviceSubClass = 0x02, /*0x02:CDC-modem , 0x00:CDC-serial*/ + .bMaxPacketSize0 = 0x40, .idVendor = __constant_cpu_to_le16(CONFIG_G_DNL_VENDOR_NUM), .idProduct = __constant_cpu_to_le16(CONFIG_G_DNL_PRODUCT_NUM),
Explicity set the max packet size in the device descriptor to 0x40 as specified in the device class specification for device firmware upgrade. Also changed debug to printf to explicitly notify the user if the device has been enumerated. Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> --- drivers/usb/gadget/composite.c | 3 +-- drivers/usb/gadget/g_dnl.c | 1 + 2 files changed, 2 insertions(+), 2 deletions(-)