EHCI driver

EHCI driver

[yanxiang fang]

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

The ehci driver cannot work for vmware ehci host controller.
It can detect usb device, but failed to get device descriptor.
Anyone get the same thing?

Frank

[-- Attachment #2: Type: text/html, Size: 211 bytes --]

Re: EHCI driver

[Aleš Nesrsta]

Hi,

I reproduced this problem on VMware workstation (v. 7.1.5).

But I am totally confused - it looks like EHCI doesn't execute
Asynchronous List (AL) even it is active according to EHCI status.
I tried to do some experiments but QHs in AL remained "untouched" by
EHCI - EHCI ignored any settings/changes I made in QHs in AL.
EHCI status reported in status register said all is OK - no error, EHCI
is in Run state and AL is active...

I don't understand, I have no idea, sorry :-(

I will try to test EHCI in VirtualBox later if there will be the same
situation.

BR,
Ales

yanxiang fang wrote:
> The ehci driver cannot work for vmware ehci host controller.
> It can detect usb device, but failed to get device descriptor.
> Anyone get the same thing?
>  
> Frank
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

Re: EHCI driver

[Aleš Nesrsta]

Hi,

I tried GRUB EHCI driver inside VirtualBox (v. 4.1.18) - and it works.

So, why GRUB EHCI doesn't work in VMware ? I don't know.
I have only some probably stupid idea - maybe VMware virtualization has
built-in some optimization which prevents correct operation of EHCI in
poll mode (without interrupt)...

BR,
Ales


Aleš Nesrsta wrote:
> Hi,
> 
> I reproduced this problem on VMware workstation (v. 7.1.5).
> 
> But I am totally confused - it looks like EHCI doesn't execute
> Asynchronous List (AL) even it is active according to EHCI status.
> I tried to do some experiments but QHs in AL remained "untouched" by
> EHCI - EHCI ignored any settings/changes I made in QHs in AL.
> EHCI status reported in status register said all is OK - no error, EHCI
> is in Run state and AL is active...
> 
> I don't understand, I have no idea, sorry :-(
> 
> I will try to test EHCI in VirtualBox later if there will be the same
> situation.
> 
> BR,
> Ales
> 
> yanxiang fang wrote:
> > The ehci driver cannot work for vmware ehci host controller.
> > It can detect usb device, but failed to get device descriptor.
> > Anyone get the same thing?
> >  
> > Frank
> > _______________________________________________
> > Grub-devel mailing list
> > Grub-devel@gnu.org
> > https://lists.gnu.org/mailman/listinfo/grub-devel
> 
> 
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

Re: EHCI driver

[Aleš Nesrsta]

Vladimir 'φ-coder/phcoder' Serbinenko wrote:
> On 18.07.2012 17:32, Aleš Nesrsta wrote:
> 
> > Hi Frank,
> > 
> > You are probably right - it might be the problem with PCI bus master
> > settings! The same problem was solved in UHCI driver for coreboot by
> > Rock some time ago.
> > 
> > Try this patch, please:
> > 
> 
> Feel free to commit this patch (especially given the confirmation that
> it works). My response time nowadays is long, feel free to commit USB
> patches as you see fit.

Hi Vladimir,

I commited two simple USB patches as trunk revision 4559 (see below).

The first one is the same patch of EHCI which was tested by Frank (and
also by me - in little bit another version of VMware).
The second one is in fact the same correction but in OHCI driver - I
expect there the same problem even nobody reported it yet (probably OHCI
is not so wide used as UHCI/EHCI and nobody tried OHCI with coreboot
yet...).

BR,
Ales

Commited patches:

diff
-purB ./grub/grub-core/bus/usb/ehci.c ./grub_patched/grub-core/bus/usb/ehci.c
--- ./grub/grub-core/bus/usb/ehci.c	2012-07-19 23:05:24.993766000 +0200
+++ ./grub_patched/grub-core/bus/usb/ehci.c	2012-07-19
22:22:48.245011460 +0200
@@ -533,6 +533,11 @@ grub_ehci_pci_iter (grub_pci_device_t de
 			"EHCI grub_ehci_pci_iter: registers above 4G are not supported\n");
 	  return 0;
 	}
+
+      /* Set bus master - needed for coreboot, VMware, broken BIOSes
etc. */
+      addr = grub_pci_make_address (dev, GRUB_PCI_REG_COMMAND);
+      grub_pci_write_word(addr,
+          GRUB_PCI_COMMAND_BUS_MASTER | grub_pci_read_word(addr));
       
       grub_dprintf ("ehci", "EHCI grub_ehci_pci_iter: 32-bit EHCI OK
\n");
     }

diff
-purB ./grub/grub-core/bus/usb/ohci.c ./grub_patched/grub-core/bus/usb/ohci.c
--- ./grub/grub-core/bus/usb/ohci.c	2012-07-19 23:05:24.993766000 +0200
+++ ./grub_patched/grub-core/bus/usb/ohci.c	2012-07-19
22:22:48.269010600 +0200
@@ -270,6 +270,11 @@ grub_ohci_pci_iter (grub_pci_device_t de
 	return 0;
 #endif
 
+      /* Set bus master - needed for coreboot, VMware, broken BIOSes
etc. */
+      addr = grub_pci_make_address (dev, GRUB_PCI_REG_COMMAND);
+      grub_pci_write_word(addr,
+          GRUB_PCI_COMMAND_BUS_MASTER | grub_pci_read_word(addr));
+
       grub_dprintf ("ohci", "class=0x%02x 0x%02x interface 0x%02x\n",
 		    class, subclass, interf);
     }

Re: EHCI driver

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 18.07.2012 17:32, Aleš Nesrsta wrote:

> Hi Frank,
> 
> You are probably right - it might be the problem with PCI bus master
> settings! The same problem was solved in UHCI driver for coreboot by
> Rock some time ago.
> 
> Try this patch, please:
> 

Feel free to commit this patch (especially given the confirmation that
it works). My response time nowadays is long, feel free to commit USB
patches as you see fit.

> @@ -533,6 +533,11 @@ grub_ehci_pci_iter (grub_pci_device_t de
>  			"EHCI grub_ehci_pci_iter: registers above 4G are not supported\n");
>  	  return 0;
>  	}
> +
> +      /* Set bus master - needed for coreboot or broken BIOSes (and
> VMware?) */
> +      addr = grub_pci_make_address (dev, GRUB_PCI_REG_COMMAND);
> +      grub_pci_write_word(addr,
> +          GRUB_PCI_COMMAND_BUS_MASTER | grub_pci_read_word(addr));
>        
>        grub_dprintf ("ehci", "EHCI grub_ehci_pci_iter: 32-bit EHCI OK
> \n");
>      }
> 
> BR,
> Ales
> 
> 
> yanxiang fang wrote:
>> Hi, Ales.
>>  
>> I studied linux ehci driver which can work well in vmware. But found
>> nothing useful, mybe because it is too complex. So I turned to
>> other simpler OS to find some useful thing.
>>  
>> I don't know anything about PCI. Do you think this problem having
>> something to do with PCI configuration?
>>  
>> BR,
>>  
>> frank
>>  
>> _______________________________________________
>> Grub-devel mailing list
>> Grub-devel@gnu.org
>> https://lists.gnu.org/mailman/listinfo/grub-devel
> 
> 
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
> .
> 



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


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

Re: EHCI driver

[yanxiang fang]

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

hi, Ales

Great work!
It can work now.

BR,

Frank

[-- Attachment #2: Type: text/html, Size: 136 bytes --]

Re: EHCI driver

[Aleš Nesrsta]

Hi Frank,

You are probably right - it might be the problem with PCI bus master
settings! The same problem was solved in UHCI driver for coreboot by
Rock some time ago.

Try this patch, please:

@@ -533,6 +533,11 @@ grub_ehci_pci_iter (grub_pci_device_t de
 			"EHCI grub_ehci_pci_iter: registers above 4G are not supported\n");
 	  return 0;
 	}
+
+      /* Set bus master - needed for coreboot or broken BIOSes (and
VMware?) */
+      addr = grub_pci_make_address (dev, GRUB_PCI_REG_COMMAND);
+      grub_pci_write_word(addr,
+          GRUB_PCI_COMMAND_BUS_MASTER | grub_pci_read_word(addr));
       
       grub_dprintf ("ehci", "EHCI grub_ehci_pci_iter: 32-bit EHCI OK
\n");
     }

BR,
Ales


yanxiang fang wrote:
> Hi, Ales.
>  
> I studied linux ehci driver which can work well in vmware. But found
> nothing useful, mybe because it is too complex. So I turned to
> other simpler OS to find some useful thing.
>  
> I don't know anything about PCI. Do you think this problem having
> something to do with PCI configuration?
>  
> BR,
>  
> frank
>  
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

Re: EHCI driver

[yanxiang fang]

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

Hi, Ales.

I studied linux ehci driver which can work well in vmware. But found
nothing useful, mybe because it is too complex. So I turned to
other simpler OS to find some useful thing.

I don't know anything about PCI. Do you think this problem having something
to do with PCI configuration?

BR,

frank

[-- Attachment #2: Type: text/html, Size: 420 bytes --]

Re: EHCI driver

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

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


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

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

Re: EHCI driver

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 30.05.2012 19:28, Aleš Nesrsta wrote:

> It is probably not necessary - I think, really there will be "allocated"
> the same total number of QHs during communication by this new algorithm
> as with old algorithm.
> The reason is: Now will be used more "sync. QHs" but less "async.
> QHs" (because some "async. QHs" were "allocated" before to "emulate"
> interrupt transfer via bulk) - so the total sum should be the same now
> as before.

It's not about the used size but about code simplicity and avoiding
iterating through all QHs when not needed. Previously we had only one
type of QHs in one list, now we need 2 and clearly separating them would
make the code simpler.

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


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

Re: EHCI driver

[Aleš Nesrsta]

Hi Vladimir and Chris:

Chris:
Thank You for the research, correction of my mistakes (classical
copy-paste mistake... :-) ) and the patch.
As I see, Vladimir is currently discussing Your patch to be accepted, so
I am writing only some notes below mainly for Vladimir.
 
Vladimir:
1.
> interrupt transfers have nothing to do with CPU interrupts per se. You
> can check whether PCI interrupt occurred without having to enable CPU
> interrupts.

Yes, I know, of course. I mentioned CPU interrupt only because doing
such things via CPU interrupt is/could be easier... :-)
Without interrupt You need to have some program loop which will
guarantee periodic polling with frequency higher than USB interrupt
transfer frequency - in opposite case You can loose data from some
transfers. (I think it cannot happen in GRUB bulk "emulation" of
interrupt transfer working on UHCI/OHCI - because when bulk trasfer is
used, no new transfer is started before data are overtaken from old
transfer. But interrupt transfers are initiated by *HCI periodically
independently even if program overtake data or not.)
I was not sure if such fast-enough program loop can be fulfilled in GRUB
- probably yes. Butmaybe with some exceptions - ? - e.g. during some
large file-disk operations on slow media?.

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

It is probably not necessary - I think, really there will be "allocated"
the same total number of QHs during communication by this new algorithm
as with old algorithm.
The reason is: Now will be used more "sync. QHs" but less "async.
QHs" (because some "async. QHs" were "allocated" before to "emulate"
interrupt transfer via bulk) - so the total sum should be the same now
as before.

BR,
Ales

Re: EHCI driver

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- 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 --]

Re: EHCI driver

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 30.05.2012 00:46, Aleš Nesrsta wrote:

> From my point of view it is little bit more probable the second reason.
> In this case the solution could be real implementation of interrupt
> transfers in GRUB - at least for EHCI. It may be possible even though
> GRUB is not supporting CPU interrupts but currently I have no idea/time
> to think about.

interrupt transfers have nothing to do with CPU interrupts per se. You
can check whether PCI interrupt occurred without having to enable CPU
interrupts.

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


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

Re: EHCI driver

[Christer Weinigel]

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

Re: EHCI driver

[Christer Weinigel]

On 2012-05-30 00:46, Aleš Nesrsta wrote:

>> the EHCI driver in GRUB has some known problems with low-speed transfers
>> and split transactions [1].  On older machines this is not a big problem
>> since it's possible to connect low speed devices such a USB keyboard
>> directly to a port and use the UHCI driver instead.  But modern Intel
>> chipsets do not have a UHCI companion controller any more, they only
>> have a EHCI controller with a normal USB 2.0 rate matching hub
>> permanently connected to the EHCI port inside the chipset.


> In fact there could be probably only two reasons of the problem:

> - my mistake in split transfer configuration (I checked it but who
> knows...)
> - GRUB "simulation" of interrupt transfer via bulk transfers is not
> working when it is used via EHCI split transfers

>

> From my point of view it is little bit more probable the second reason.
> In this case the solution could be real implementation of interrupt
> transfers in GRUB - at least for EHCI. It may be possible even though
> GRUB is not supporting CPU interrupts but currently I have no idea/time
> to think about.


Right.  I put a USB bus analyzer on the bus and it turns out that when a
queue head  where the Endpoint Speed field is set to 01b (indicating a
_low speed_ split transaction) is put on the asynchronous schedule, what
actually comes out on the wire is a split packet saying that it is a
_full speed_ bulk transfer. So the hub sees a full speed transfer for a
low speed port (or doesn't get any intelligible response) and returns a
stall condition.

This is all according to the standards I guess since bulk transfers are
not allowed for low speed.  GRUB is wrong in asking for such a transfer
and the behavior when it does is undefined.  It's a bit surprising
though, Intel engineers must have made an extra effort to disallow low
speed bulk transfers in the EHCI controller instead of just doing what
the user asks the controller to do.

I've done a quick hack to test this, basically changed grub_ehci_find_qh
like this:

if (transfer->dev->speed == GRUB_USB_SPEED_LOW
    && transfer->type != GRUB_USB_TRANSACTION_TYPE_CONTROL)
{
    // special case to link the QH to e->qh_virt[0]
    // which is the periodic schedule
    // also set the s-mask and c-mask for interrupt transfers
}
else
{
    // the normal code to link the QH to e->qh_virt[1]
    // which is the asynchronous schedule
}

This of course crashes after a while since the queues get mixed up but
it does allow me to talk to a USB keyboard connected to a USB 2.0 hub,
and the split packets seen on the wire now correctly say that they are
interrupt transfers to a low speed device.  So it looks like GRUBs
misuse of bulk transfers instead of proper interrupt transfers is the
root cause.

> If the reason is only my mistake in EHCI code then the solution is very
> simple - find the bug and correct it... :-)

I did find one fairly obvious bug, the High-Bandwidth Pipe Multiplier
(Mult) field of endpoint characteristics have the wrong defines:

  GRUB_EHCI_MULT_ONE = (0 << 30),
  GRUB_EHCI_MULT_TWO = (0 << 30),
  GRUB_EHCI_MULT_THREE = (0 << 30),

They should be 1, 2 and 3 instead.  The EHCI specification says "A zero
in this field yields undefined results."  But it doesn't seem to make
any difference though.

> If You have time and related knowledges or You know somebody like that,
> You are welcome to help us - at least to find the reason of the problem
> or check the EHCI split transfer code or invent implemention of
> interrupt transfers in EHCI GRUB driver etc...


Done. :-)

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.

  /Christer

Re: EHCI driver

[Aleš Nesrsta]

Christer Weinigel píše v Út 29. 05. 2012 v 11:07 +0200:
> Hi list,
> 
> the EHCI driver in GRUB has some known problems with low-speed transfers
> and split transactions [1].  On older machines this is not a big problem
> since it's possible to connect low speed devices such a USB keyboard
> directly to a port and use the UHCI driver instead.  But modern Intel
> chipsets do not have a UHCI companion controller any more, they only
> have a EHCI controller with a normal USB 2.0 rate matching hub
> permanently connected to the EHCI port inside the chipset.
> 
> Has anybody had time to look any more at the EHCI driver?  Any ideas for
> how to fix this?

Uff, it is little bit difficult. The reason of the problem is still not
known, so it is not possible to think about solution.

I am afraid nobody has time to investigate this problem, including me,
unfortunately... - GRUB developers have lot of work on currently more
important things and I am still too busy (and there is no chance to get
more free time for me in near future).

In fact there could be probably only two reasons of the problem:
- my mistake in split transfer configuration (I checked it but who
knows...)
- GRUB "simulation" of interrupt transfer via bulk transfers is not
working when it is used via EHCI split transfers

From my point of view it is little bit more probable the second reason.
In this case the solution could be real implementation of interrupt
transfers in GRUB - at least for EHCI. It may be possible even though
GRUB is not supporting CPU interrupts but currently I have no idea/time
to think about.

If the reason is only my mistake in EHCI code then the solution is very
simple - find the bug and correct it... :-)

If You have time and related knowledges or You know somebody like that,
You are welcome to help us - at least to find the reason of the problem
or check the EHCI split transfer code or invent implemention of
interrupt transfers in EHCI GRUB driver etc...

BR,
Ales

> 
>   /Christer
> 
> [1] http://lists.gnu.org/archive/html/grub-devel/2011-10/msg00007.html
> 
> 

EHCI driver

[Christer Weinigel]

Hi list,

the EHCI driver in GRUB has some known problems with low-speed transfers
and split transactions [1].  On older machines this is not a big problem
since it's possible to connect low speed devices such a USB keyboard
directly to a port and use the UHCI driver instead.  But modern Intel
chipsets do not have a UHCI companion controller any more, they only
have a EHCI controller with a normal USB 2.0 rate matching hub
permanently connected to the EHCI port inside the chipset.

Has anybody had time to look any more at the EHCI driver?  Any ideas for
how to fix this?

  /Christer

[1] http://lists.gnu.org/archive/html/grub-devel/2011-10/msg00007.html


-- 
Have laptop, will travel.  I'm a consultant looking for interesting
jobs anywhere in the world.  I'm an experienced software engineer with
a solid understanding of hardware.  Specialties: Linux, device
drivers and embedded systems in general.  Find me at www.weinigel.se.