Thanks, I've committed the patch with just style changes.
On 30.05.2012 14:12, Christer Weinigel wrote:

> On 2012-05-30 11:28, Christer Weinigel wrote:
> 
>> [talk about EHCI problems with low speed split transactions]
> 
>> Now I just have to fix up the queue management so that it properly keeps
>> track of QHs for two queues instead of one. And figure out if there are
>> any other problems.
> 
> 
> So, how about something like this?
> 
> Fix low-speed USB split transactions on EHCI
> 
> Low-speed split transactions did not work on EHCI.  The reason is that
> only control and interrupt transfers are allowed with low-speed, there
> is no such thing as bulk transfers for low-speed.  GRUB doesn't know
> about interrupt transfers, it always uses bulk transfers, and the EHCI
> driver did not support interrupt transfers either.
> 
> It seems that when doing split transactions from a QH (queue header)
> on the asynchronous schedule in the EHCI controller, the SPLIT packet
> on the bus will be for a full-speed transfer, even if the QH says
> that it is a low-speed transfer.  To actually get a low-speed transfer
> the QH has to be on the periodic schedule.
> 
> This patch adds a hack that checks if a bulk transfer is for a
> low-speed device and in that case puts the QH on the periodic schedule
> instead of on the asynchronous schedule.  Checking for bulk transfers
> to a low speed device is a rather ugly hack, it would be better to
> implement proper support for interrupt transfers in GRUB.  But this
> made my keyboard work with minimal changes.
> 
> The same queue of transfers is used for all 1024 frames in the periodic
> schedule, so the interrupt transfer will be retried every frame, there
> is no support for longer poll times.  I don't know if that is a problem
> in practice, but I could imagine that some slow device might choke
> if the host polls it too often.
> 
> To be able to use the same pool of QHs for both the periodic and the
> asynchronous schedules the way QHs are allocated had to change a bit.
> Instead of qh_virt[i-1] always being followed by qh_virt[i], they
> are now proper linked lists.  When cancelling a transfer we have
> to look through the list for the previous QH instead of just using
> qh_virt[i-1].  On the other hand, cancelling happens very seldom
> so this doesn't really affect performance.
> 
> The declarations of GRUB_EHCI_MULT_ONE, TWO and THREE were all zero
> which is explicitly disallowed by the EHCI specification:
> "A zero in this field yields undefined results."
> I changed the defines to have the correct values.
> 
> BTW, there is one really strange issue when I have multiple keyboards 
> For example if I have three keyboards, with devices addresses 2, 3, and 
> 4.  If I unplug and replug keyboard 3, this for some reason sets the
> halted flags or clears the active flags for keyboards 2 and 4.  The 
> alternate code selected by #if in grub_ehci_cancel_transfer makes things
> work, but I have no idea why which is a bit frightening.
> 
> Note, this change is against some old copy of ehci.c from a few months
> ago, so it probably won't apply to the latest code in bazaar.  But if
> you think this looks good, I can figure out how to check out the
> bazaar repo again and make a new patch.
> 
> I'm trying to use thunderbird to send this patch, so it might be a bit 
> mangled, but hopefully I'm managed to get it right.
> 
>   /Christer
> 
> diff --git a/grub-core/bus/usb/ehci.c b/grub-core/bus/usb/ehci.c
> index 0f41361..d8ecf26 100644
> --- a/grub-core/bus/usb/ehci.c
> +++ b/grub-core/bus/usb/ehci.c
> @@ -209,18 +209,22 @@ enum
>  {
>    GRUB_EHCI_MULT_MASK = (3 << 30),
>    GRUB_EHCI_MULT_RESERVED = (0 << 30),
> -  GRUB_EHCI_MULT_ONE = (0 << 30),
> -  GRUB_EHCI_MULT_TWO = (0 << 30),
> -  GRUB_EHCI_MULT_THREE = (0 << 30),
> +  GRUB_EHCI_MULT_ONE = (1 << 30),
> +  GRUB_EHCI_MULT_TWO = (2 << 30),
> +  GRUB_EHCI_MULT_THREE = (3 << 30),
>    GRUB_EHCI_DEVPORT_MASK = (0x7f << 23),
> -  GRUB_EHCI_HUBADDR_MASK = (0x7f << 16)
> +  GRUB_EHCI_HUBADDR_MASK = (0x7f << 16),
> +  GRUB_EHCI_CMASK_MASK = (0xff << 8),
> +  GRUB_EHCI_SMASK_MASK = (0xff << 0),
>  };
>  
>  enum
>  {
>    GRUB_EHCI_MULT_OFF = 30,
>    GRUB_EHCI_DEVPORT_OFF = 23,
> -  GRUB_EHCI_HUBADDR_OFF = 16
> +  GRUB_EHCI_HUBADDR_OFF = 16,
> +  GRUB_EHCI_CMASK_OFF = 8,
> +  GRUB_EHCI_SMASK_OFF = 0,
>  };
>  
>  #define GRUB_EHCI_TERMINATE      (1<<0)
> @@ -782,6 +786,7 @@ grub_ehci_pci_iter (grub_pci_device_t dev,
>    /* Enable asynchronous list */
>    grub_ehci_oper_write32 (e, GRUB_EHCI_COMMAND,
>  			  GRUB_EHCI_CMD_AS_ENABL
> +			  | GRUB_EHCI_CMD_PS_ENABL
>  			  | grub_ehci_oper_read32 (e, GRUB_EHCI_COMMAND));
>  
>    /* 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;
> +  }
>    qh->ep_cap = grub_cpu_to_le32 (ep_cap);
>  
>    grub_dprintf ("ehci", "setup_qh: qh=%08x, not changed: qh_hptr=%08x\n",
> @@ -949,6 +960,7 @@ grub_ehci_find_qh (struct grub_ehci *e, grub_usb_transfer_t transfer)
>    grub_uint32_t target, mask;
>    int i;
>    grub_ehci_qh_t qh = e->qh_virt;
> +  grub_ehci_qh_t head;
>  
>    /* Prepare part of EP Characteristic to find existing QH */
>    target = ((transfer->endpoint << GRUB_EHCI_EP_NUM_OFF) |
> @@ -983,14 +995,21 @@ grub_ehci_find_qh (struct grub_ehci *e, grub_usb_transfer_t transfer)
>     * de-allocate QHs of unplugged devices. */
>    /* We should preset new QH and link it into AL */
>    grub_ehci_setup_qh (&qh[i], transfer);
> -  /* Linking - this new (last) QH will point to first QH */
> -  qh[i].qh_hptr = grub_cpu_to_le32 (GRUB_EHCI_HPTR_TYPE_QH
> -				    | grub_dma_virt2phys (&qh[1],
> -							  e->qh_chunk));
> -  /* Linking - previous last QH will point to this new QH */
> -  qh[i - 1].qh_hptr = grub_cpu_to_le32 (GRUB_EHCI_HPTR_TYPE_QH
> -					| grub_dma_virt2phys (&qh[i],
> -							      e->qh_chunk));
> +
> +  /* 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];
> +
> +  /* Linking - this new (last) QH will copy the QH from the head QH */
> +  qh[i].qh_hptr = head->qh_hptr;
> +  /* Linking - the head QH will point to this new QH */
> +  head->qh_hptr = grub_cpu_to_le32 (GRUB_EHCI_HPTR_TYPE_QH
> +                                    | grub_dma_virt2phys (&qh[i],
> +                                                          e->qh_chunk));
>  
>    return &qh[i];
>  }
> @@ -1221,7 +1240,7 @@ grub_ehci_setup_transfer (grub_usb_controller_t dev,
>      /* XXX: Fix it: Currently we don't do anything to restart EHCI */
>      return GRUB_USB_ERR_INTERNAL;
>    if ((grub_ehci_oper_read32 (e, GRUB_EHCI_STATUS)
> -       & GRUB_EHCI_ST_AS_STATUS) == 0)
> +       & (GRUB_EHCI_ST_AS_STATUS | GRUB_EHCI_ST_PS_STATUS)) == 0)
>      /* XXX: Fix it: Currently we don't do anything to restart EHCI */
>      return GRUB_USB_ERR_INTERNAL;
>  
> @@ -1372,6 +1391,7 @@ grub_ehci_parse_notrun (grub_usb_controller_t dev,
>    /* Try enable EHCI and AL */
>    grub_ehci_oper_write32 (e, GRUB_EHCI_COMMAND,
>  			  GRUB_EHCI_CMD_RUNSTOP | GRUB_EHCI_CMD_AS_ENABL
> +			  | GRUB_EHCI_CMD_PS_ENABL
>  			  | grub_ehci_oper_read32 (e, GRUB_EHCI_COMMAND));
>    /* Ensure command is written */
>    grub_ehci_oper_read32 (e, GRUB_EHCI_COMMAND);
> @@ -1470,7 +1490,7 @@ grub_ehci_check_transfer (grub_usb_controller_t dev,
>         & GRUB_EHCI_ST_HC_HALTED) != 0)
>      return grub_ehci_parse_notrun (dev, transfer, actual);
>    if ((grub_ehci_oper_read32 (e, GRUB_EHCI_STATUS)
> -       & GRUB_EHCI_ST_AS_STATUS) == 0)
> +       & (GRUB_EHCI_ST_AS_STATUS | GRUB_EHCI_ST_PS_STATUS)) == 0)
>      return grub_ehci_parse_notrun (dev, transfer, actual);
>  
>    token = grub_le_to_cpu32 (cdata->qh_virt->td_overlay.token);
> @@ -1508,6 +1528,7 @@ grub_ehci_cancel_transfer (grub_usb_controller_t dev,
>    grub_size_t actual;
>    int i;
>    grub_uint64_t maxtime;
> +  grub_uint32_t qh_phys;
>  
>    /* QH can be active and should be de-activated and halted */
>  
> @@ -1518,7 +1539,7 @@ grub_ehci_cancel_transfer (grub_usb_controller_t dev,
>    if (((grub_ehci_oper_read32 (e, GRUB_EHCI_STATUS)
>  	& GRUB_EHCI_ST_HC_HALTED) != 0) ||
>        ((grub_ehci_oper_read32 (e, GRUB_EHCI_STATUS)
> -	& GRUB_EHCI_ST_AS_STATUS) == 0))
> +	& (GRUB_EHCI_ST_AS_STATUS | GRUB_EHCI_ST_PS_STATUS)) == 0))
>      {
>        grub_ehci_pre_finish_transfer (transfer);
>        grub_ehci_free_tds (e, cdata->td_first_virt, transfer, &actual);
> @@ -1530,44 +1551,83 @@ grub_ehci_cancel_transfer (grub_usb_controller_t dev,
>  
>    /* 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;
> +    }
> +  if (i == GRUB_EHCI_N_QH)
> +    {
> +      grub_printf ("%s: prev not found, queues are corrupt\n", __func__);
> +      return GRUB_USB_ERR_UNRECOVERABLE;
> +    }
>    /* Unlink QH from AL */
> -  e->qh_virt[i - 1].qh_hptr = cdata->qh_virt->qh_hptr;
> -  /* Ring the doorbell */
> -  grub_ehci_oper_write32 (e, GRUB_EHCI_COMMAND,
> -			  GRUB_EHCI_CMD_AS_ADV_D
> -			  | grub_ehci_oper_read32 (e, GRUB_EHCI_COMMAND));
> -  /* Ensure command is written */
> -  grub_ehci_oper_read32 (e, GRUB_EHCI_COMMAND);
> -  /* Wait answer with timeout */
> -  maxtime = grub_get_time_ms () + 2;
> -  while (((grub_ehci_oper_read32 (e, GRUB_EHCI_STATUS)
> -	   & GRUB_EHCI_ST_AS_ADVANCE) == 0)
> -	 && (grub_get_time_ms () < maxtime));
> +  e->qh_virt[i].qh_hptr = cdata->qh_virt->qh_hptr;
> +
> +  /* 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);
> +    }
> +  else
> +    {
> +      /* For the asynchronous schedule we use the advance doorbell to find
> +       * out when the EHCI controller has read the updated QH. */
>  
> -  /* We do not detect the timeout because if timeout occurs, it most
> -   * probably means something wrong with EHCI - maybe stopped etc. */
> +      /* Ring the doorbell */
> +      grub_ehci_oper_write32 (e, GRUB_EHCI_COMMAND,
> +                              GRUB_EHCI_CMD_AS_ADV_D
> +                              | grub_ehci_oper_read32 (e, GRUB_EHCI_COMMAND));
> +      /* Ensure command is written */
> +      grub_ehci_oper_read32 (e, GRUB_EHCI_COMMAND);
> +      /* Wait answer with timeout */
> +      maxtime = grub_get_time_ms () + 2;
> +      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. */
> +
> +      /* 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);
> +    }
>  
>    /* Now is QH out of AL and we can do anything with it... */
>    grub_ehci_pre_finish_transfer (transfer);
>    grub_ehci_free_tds (e, cdata->td_first_virt, transfer, &actual);
>    grub_ehci_free_td (e, cdata->td_alt_virt);
>  
> +  /* 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));
> +#else
> +  /* Free the QH */
> +  cdata->qh_virt->ep_char = 0;
> +  cdata->qh_virt->qh_hptr =
> +    grub_cpu_to_le32 ((grub_dma_virt2phys (cdata->qh_virt,
> +                                           e->qh_chunk) &
> +                       GRUB_EHCI_POINTER_MASK) | GRUB_EHCI_HPTR_TYPE_QH);
> +#endif
> +
>    grub_free (cdata);
>  
>    grub_dprintf ("ehci", "cancel_transfer: end\n");
> @@ -1777,6 +1837,7 @@ grub_ehci_restore_hw (void)
>        /* Enable asynchronous list */
>        grub_ehci_oper_write32 (e, GRUB_EHCI_COMMAND,
>  			      GRUB_EHCI_CMD_AS_ENABL
> +			      | GRUB_EHCI_CMD_PS_ENABL
>  			      | grub_ehci_oper_read32 (e, GRUB_EHCI_COMMAND));
>  
>        /* Now should be possible to power-up and enumerate ports etc. */
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
> 



-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko