* EHCI driver @ 2012-05-29 9:07 Christer Weinigel 2012-05-29 22:46 ` Aleš Nesrsta 0 siblings, 1 reply; 18+ messages in thread From: Christer Weinigel @ 2012-05-29 9:07 UTC (permalink / raw) To: grub-devel 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: EHCI driver 2012-05-29 9:07 EHCI driver Christer Weinigel @ 2012-05-29 22:46 ` Aleš Nesrsta 2012-05-30 9:28 ` Christer Weinigel 2012-05-30 12:24 ` Vladimir 'φ-coder/phcoder' Serbinenko 0 siblings, 2 replies; 18+ messages in thread From: Aleš Nesrsta @ 2012-05-29 22:46 UTC (permalink / raw) To: The development of GNU GRUB 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 > > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: EHCI driver 2012-05-29 22:46 ` Aleš Nesrsta @ 2012-05-30 9:28 ` Christer Weinigel 2012-05-30 12:12 ` Christer Weinigel 2012-05-30 12:24 ` Vladimir 'φ-coder/phcoder' Serbinenko 1 sibling, 1 reply; 18+ messages in thread From: Christer Weinigel @ 2012-05-30 9:28 UTC (permalink / raw) To: The development of GNU GRUB; +Cc: Aleš Nesrsta 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: EHCI driver 2012-05-30 9:28 ` Christer Weinigel @ 2012-05-30 12:12 ` Christer Weinigel 2012-05-30 12:35 ` Vladimir 'φ-coder/phcoder' Serbinenko 2012-05-31 12:08 ` Vladimir 'φ-coder/phcoder' Serbinenko 0 siblings, 2 replies; 18+ messages in thread From: Christer Weinigel @ 2012-05-30 12:12 UTC (permalink / raw) To: The development of GNU GRUB; +Cc: Aleš Nesrsta 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. */ ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: EHCI driver 2012-05-30 12:12 ` Christer Weinigel @ 2012-05-30 12:35 ` Vladimir 'φ-coder/phcoder' Serbinenko 2012-05-31 8:40 ` Christer Weinigel 2012-05-31 12:08 ` Vladimir 'φ-coder/phcoder' Serbinenko 1 sibling, 1 reply; 18+ messages in thread From: Vladimir 'φ-coder/phcoder' Serbinenko @ 2012-05-30 12:35 UTC (permalink / raw) To: The development of GNU GRUB [-- 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 --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: EHCI driver 2012-05-30 12:35 ` Vladimir 'φ-coder/phcoder' Serbinenko @ 2012-05-31 8:40 ` Christer Weinigel 0 siblings, 0 replies; 18+ messages in thread From: Christer Weinigel @ 2012-05-31 8:40 UTC (permalink / raw) To: The development of GNU GRUB Cc: Vladimir 'φ-coder/phcoder' Serbinenko 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: EHCI driver 2012-05-30 12:12 ` Christer Weinigel 2012-05-30 12:35 ` Vladimir 'φ-coder/phcoder' Serbinenko @ 2012-05-31 12:08 ` Vladimir 'φ-coder/phcoder' Serbinenko 1 sibling, 0 replies; 18+ messages in thread From: Vladimir 'φ-coder/phcoder' Serbinenko @ 2012-05-31 12:08 UTC (permalink / raw) To: grub-devel [-- 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 --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: EHCI driver 2012-05-29 22:46 ` Aleš Nesrsta 2012-05-30 9:28 ` Christer Weinigel @ 2012-05-30 12:24 ` Vladimir 'φ-coder/phcoder' Serbinenko 2012-05-30 17:28 ` Aleš Nesrsta 1 sibling, 1 reply; 18+ messages in thread From: Vladimir 'φ-coder/phcoder' Serbinenko @ 2012-05-30 12:24 UTC (permalink / raw) To: The development of GNU GRUB [-- 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 --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: EHCI driver 2012-05-30 12:24 ` Vladimir 'φ-coder/phcoder' Serbinenko @ 2012-05-30 17:28 ` Aleš Nesrsta 2012-05-30 17:40 ` Vladimir 'φ-coder/phcoder' Serbinenko 0 siblings, 1 reply; 18+ messages in thread From: Aleš Nesrsta @ 2012-05-30 17:28 UTC (permalink / raw) To: The development of GNU GRUB 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: EHCI driver 2012-05-30 17:28 ` Aleš Nesrsta @ 2012-05-30 17:40 ` Vladimir 'φ-coder/phcoder' Serbinenko 0 siblings, 0 replies; 18+ messages in thread From: Vladimir 'φ-coder/phcoder' Serbinenko @ 2012-05-30 17:40 UTC (permalink / raw) To: grub-devel [-- 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 --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* EHCI driver @ 2012-07-09 8:48 yanxiang fang 2012-07-16 22:16 ` Aleš Nesrsta 0 siblings, 1 reply; 18+ messages in thread From: yanxiang fang @ 2012-07-09 8:48 UTC (permalink / raw) To: grub-devel [-- 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 --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: EHCI driver 2012-07-09 8:48 yanxiang fang @ 2012-07-16 22:16 ` Aleš Nesrsta 2012-07-17 18:20 ` Aleš Nesrsta 0 siblings, 1 reply; 18+ messages in thread From: Aleš Nesrsta @ 2012-07-16 22:16 UTC (permalink / raw) To: The development of GNU GRUB 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: EHCI driver 2012-07-16 22:16 ` Aleš Nesrsta @ 2012-07-17 18:20 ` Aleš Nesrsta 0 siblings, 0 replies; 18+ messages in thread From: Aleš Nesrsta @ 2012-07-17 18:20 UTC (permalink / raw) To: The development of GNU GRUB 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: EHCI driver @ 2012-07-17 23:18 yanxiang fang 2012-07-18 15:32 ` Aleš Nesrsta 0 siblings, 1 reply; 18+ messages in thread From: yanxiang fang @ 2012-07-17 23:18 UTC (permalink / raw) To: Grub-devel [-- 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 --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: EHCI driver 2012-07-17 23:18 yanxiang fang @ 2012-07-18 15:32 ` Aleš Nesrsta 2012-07-22 16:58 ` Vladimir 'φ-coder/phcoder' Serbinenko 0 siblings, 1 reply; 18+ messages in thread From: Aleš Nesrsta @ 2012-07-18 15:32 UTC (permalink / raw) To: The development of GNU GRUB 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: EHCI driver 2012-07-18 15:32 ` Aleš Nesrsta @ 2012-07-22 16:58 ` Vladimir 'φ-coder/phcoder' Serbinenko 2012-07-22 19:24 ` Aleš Nesrsta 0 siblings, 1 reply; 18+ messages in thread From: Vladimir 'φ-coder/phcoder' Serbinenko @ 2012-07-22 16:58 UTC (permalink / raw) To: grub-devel [-- 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 --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: EHCI driver 2012-07-22 16:58 ` Vladimir 'φ-coder/phcoder' Serbinenko @ 2012-07-22 19:24 ` Aleš Nesrsta 0 siblings, 0 replies; 18+ messages in thread From: Aleš Nesrsta @ 2012-07-22 19:24 UTC (permalink / raw) To: The development of GNU GRUB 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); } ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: EHCI driver @ 2012-07-19 0:31 yanxiang fang 0 siblings, 0 replies; 18+ messages in thread From: yanxiang fang @ 2012-07-19 0:31 UTC (permalink / raw) To: grub-devel [-- 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 --] ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2012-07-22 19:24 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-05-29 9:07 EHCI driver Christer Weinigel 2012-05-29 22:46 ` Aleš Nesrsta 2012-05-30 9:28 ` Christer Weinigel 2012-05-30 12:12 ` Christer Weinigel 2012-05-30 12:35 ` Vladimir 'φ-coder/phcoder' Serbinenko 2012-05-31 8:40 ` Christer Weinigel 2012-05-31 12:08 ` Vladimir 'φ-coder/phcoder' Serbinenko 2012-05-30 12:24 ` Vladimir 'φ-coder/phcoder' Serbinenko 2012-05-30 17:28 ` Aleš Nesrsta 2012-05-30 17:40 ` Vladimir 'φ-coder/phcoder' Serbinenko 2012-07-09 8:48 yanxiang fang 2012-07-16 22:16 ` Aleš Nesrsta 2012-07-17 18:20 ` Aleš Nesrsta 2012-07-17 23:18 yanxiang fang 2012-07-18 15:32 ` Aleš Nesrsta 2012-07-22 16:58 ` Vladimir 'φ-coder/phcoder' Serbinenko 2012-07-22 19:24 ` Aleš Nesrsta 2012-07-19 0:31 yanxiang fang
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.