Re: EHCI driver

[Christer Weinigel]

On 2012-05-30 14:35, Vladimir 'φ-coder/phcoder' Serbinenko wrote:

>>    /* Now should be possible to power-up and enumerate ports etc. */
>> @@ -926,6 +931,12 @@ grub_ehci_setup_qh (grub_ehci_qh_t qh, 
>> grub_usb_transfer_t transfer)
>>      & GRUB_EHCI_DEVPORT_MASK;
>>    ep_cap |= (transfer->dev->hubaddr << GRUB_EHCI_HUBADDR_OFF)
>>      & GRUB_EHCI_HUBADDR_MASK;
>> +  if (transfer->dev->speed == GRUB_USB_SPEED_LOW &&
>> +      transfer->type != GRUB_USB_TRANSACTION_TYPE_CONTROL)
>> +  {
>> +    ep_cap |= (1<<0) << GRUB_EHCI_SMASK_OFF;
>> +    ep_cap |= (7<<2) << GRUB_EHCI_CMASK_OFF;
>> +  }
> 
> Could you use enums rather than hardcoding constants?


These aren't register settings, they are bitmasks of when to schedule
start-split and complete-split frames.  In a "proper" EHCI
implementation they would be calculated to spread the load over the
microframes of a frame.  But yes, I can do that.

>> +  /* low speed interrupt transfers are linked to the periodic
>> +   * scheudle, everything else to the asynchronous schedule */
>> +  if (transfer->dev->speed == GRUB_USB_SPEED_LOW &&
>> +      transfer->type != GRUB_USB_TRANSACTION_TYPE_CONTROL)
>> +      head = &qh[0];
>> +  else
>> +      head = &qh[1];
> 
> Wouldn't it be easier to have a range reserved for interrupt transfer?
> It's not like if we were short on QHs.


Probably easier, but having a proper linked list is good for other
reasons since it means that we can actually take frames off a linked
list.  And except for cancellation it doesn't make the code any more
complex. Having two separate lists would add more code.

>>    /* EHCI and AL are running. What to do?
>>     * Try to Halt QH via de-scheduling QH. */
>> -  /* Find index of current QH - we need previous QH, i.e. i-1 */
>> -  i = ((int) (e->qh_virt - cdata->qh_virt)) / sizeof (struct grub_ehci_qh);
>> +  /* Find index of previous QH */
>> +  qh_phys = grub_dma_virt2phys(cdata->qh_virt, e->qh_chunk);
>> +  for (i = 0; i < GRUB_EHCI_N_QH; i++)
>> +    {
>> +      if ((e->qh_virt[i].qh_hptr & GRUB_EHCI_QHTDPTR_MASK) == qh_phys)
>> +        break;
>> +    }
> 
> This makes it iterate through uncached memory which may be very slow.


cancellation isn't performance critical, right now it's only called when
a device (keyboard or hub) is unplugged.

>> +  /* If this is an interrupt transfer, we just wait for the periodic
>> +   * schedule to advance a few times and then assume that the EHCI
>> +   * controller has read the updated QH. */
>> +  if (cdata->qh_virt->ep_cap & GRUB_EHCI_SMASK_MASK)
>> +    {
>> +      grub_millisleep(20);
> 
> Isn't there a better way than arbitrary sleep? This sleep is pretty large.


No, we have to sleep and wait for the controller to advance to the next
frame.  The normal way to do this would be to unlink the packet from the
queue and then use a timer to reclaim the packet a few milliseconds
later.  And as I said, this code is only called on unplug, so it doesn't
really matter in practice.

But yes, the sleep can be a lot shorter.  I vaguely remembered the EHCI
specification mentioning that it could cache up to 8 frames (each frame
is one millisecond) and that's why I used a sleep for roughly twice as
long, but when I found the section in the specification again it was up
to eight microframes (1/8 frame).  So it should be enough with about 2
or 3 milliseconds: one millisecond to finish the current frame and start
reading the new list, one millisecond to finish that one and maybe one
more millisecond to be on the safe side.

> 
>> +      /* Wait answer with timeout */
>> +      maxtime = grub_get_time_ms () + 2;
> 
> 2 ms seems to be short as a maximum timeout. There is quite some number
> of buggy and slow hardware around.


I haven't modified this code, it's just an indentation change.  But the
basic unit of time in USB is a frame which is one millisecond, so a
timeout doesn't have to be much longer than that.

>> +      while (((grub_ehci_oper_read32 (e, GRUB_EHCI_STATUS)
>> +               & GRUB_EHCI_ST_AS_ADVANCE) == 0)
>> +             && (grub_get_time_ms () < maxtime));
>>  
>> -  /* Shut up the doorbell */
>> -  grub_ehci_oper_write32 (e, GRUB_EHCI_COMMAND,
>> -                      ~GRUB_EHCI_CMD_AS_ADV_D
>> -                      & grub_ehci_oper_read32 (e, GRUB_EHCI_COMMAND));
>> -  grub_ehci_oper_write32 (e, GRUB_EHCI_STATUS,
>> -                      GRUB_EHCI_ST_AS_ADVANCE
>> -                      | grub_ehci_oper_read32 (e, GRUB_EHCI_STATUS));
>> -  /* Ensure command is written */
>> -  grub_ehci_oper_read32 (e, GRUB_EHCI_STATUS);
>> +      /* We do not detect the timeout because if timeout occurs, it most
>> +       * probably means something wrong with EHCI - maybe stopped etc. */
>> +
> 
> Which should be reported as an error if it occurs.


Just an indentation change, so I can't say anything.

>> +  /* FIXME Putting the QH back on the list should work, but for some
>> +   * strange reason doing that will affect other QHs on the periodic
>> +   * list.  So free the QH instead of putting it back on the list
>> +   * which does seem to work, but I would like to know why. */
>> +
>> +#if 0
>>    /* Finaly we should return QH back to the AL... */
>> -  e->qh_virt[i - 1].qh_hptr =
>> +  e->qh_virt[i].qh_hptr =
>>      grub_cpu_to_le32 (grub_dma_virt2phys
>>                    (cdata->qh_virt, e->qh_chunk));
> 
> You may link it into wrong list AFAICS (interrupt vs bulk).


I don't think that's what's wrong.  A few lines earlier the packet was
unlinked from the same list, so all this code does is to put it back in
the old place.  I also added a bit of code to dump the data structures
before the QH is unlinked, while the QH is unlinked and after the QH has
been put back and it is put back in the same place.

Looking at the Linux EHCI driver it seems that there is a silicon bug
that sounds vaguely similar, the problem I'm seeing is that QHs become
inactive/halted when they shouldn't be:

/* if it weren't for a common silicon quirk (writing the dummy into the qh
 * overlay, so qh->hw_token wrongly becomes inactive/halted), only fault
 * recovery (including urb dequeue) would need software changes to a QH...
 */


So it may be that having the QH on the list while modifying fields in it
can cause problems, and by unlinking the QH from the list the problem is
avoided.  But I'm not sure at all.

  /Christer