On Wed, Feb 08, 2012 at 12:38:07PM -0800, Paul Zimmerman wrote: > > From: Felipe Balbi [mailto:balbi@xxxxxx] > > Sent: Tuesday, February 07, 2012 9:58 PM > > On Tue, Feb 07, 2012 at 02:38:55PM -0800, Paul Zimmerman wrote: > > > > From: linux-usb-owner@xxxxxxxxxxxxxxx [mailto:linux-usb-owner@xxxxxxxxxxxxxxx] On Behalf Of Felipe Balbi > > > > Sent: Monday, February 06, 2012 6:26 AM > > > > + /* > > > > + * REVISIT: the following assumes we will always enough space > > > > + * available on the FIFO RAM for all possible usecases. Make > > > > + * sure that's true somehow and change FIFO allocation > > > > + * accordingly. > > > > + * > > > > + * If we have Bulk or Isochronous endpoints, we want > > > > + * them to be able to be very, very fast. So we're giving > > > > + * those endpoints a fifo_size which is enough for 3 full > > > > + * packets > > > > + */ > > > > > > This breaks the HAPS platform, and any other platform which doesn't have > > > enough FIFO RAM to support this. I really think you must implement this > > > properly (checking for enough RAM) before you add it to the driver. > > > > Indeed, I can add such a check. But can you share any debugging messages > > you might have gotten through console ? That might help understanding > > what's breaking it. If you look carefully, I'm just using the fifo > > allocation equation per Databook. > > Sorry, I was wrong. I was thinking the HAPS platform had the minimal > amount of FIFO RAM needed to support its four IN EPs, but in fact it > does have enough RAM to support this. > > But the point still holds. There could be a platform that does have > the minimum amount of RAM needed, and this would break it. If you add > a way to disable this for those platforms, then I guess it's OK. It's already there right ? That device-tree attribute which I have added on previous patch ? :-) > > > But I really think trying to do things like this automatically in the > > > driver is misguided. This should be part of platform data or device tree > > > instead, IMHO. > > > > I tend to disagree, actually. Let's say someone has configured the core > > with 10 IN eps and has enough fifo for 1x 1024 byte for each endpoint. > > Let's just say he's taking the performance hit ;-) > > > > Now, when we're running with anything that uses less than 10 IN eps, we > > will have a bunch of unused FIFO space, right ? This will allow me to > > use that space. So, if we're running with mass storage I will have only > > two IN eps (EP1 and whatever bulk EP gets selected), meaning that I > > could allocate up to 9x 1024 bytes to the bulk endpoint. > > I'm afraid the gadget API won't allow that. The EPs are allocated one- > by-one by the gadget, so the UDC doesn't know how many or what type > until the last one has been allocated. In fact there is no explicit > notification that the last EP has been allocated. So the UDC doesn't > know how much RAM is available for each EP until it's too late. But I > guess that could be fixed by a modification to the gadget API. Take a closer look at the driver and when I'm resizing the FIFOs. I'm starting the FIFO resizing after my SetInterface() being received, but before starting STATUS phase. By the end of SetInterface() all my endpoints are already enabled, so I can just iterate over my endpoint array and check if it has the ENABLED flag set. By using that I can count how many endpoints I have enabled, what are their types, wMaxPacketSize, wMaxBurst, and so on. -- balbi
Attachment:
signature.asc
Description: Digital signature
