Re: EHCI driver

["Vladimir 'φ-coder/phcoder' Serbinenko" <phcoder@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4901 bytes --]


> 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;
> +  }

Could you use enums rather than hardcoding constants?

> +  /* 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.

>  
>    /* 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.

> +  /* 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.

> +      /* 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.

> +      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.

>  
> +  /* 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).
-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]