That must be a too long interval of writing e-mail. I started it a month ago, then interrupted and now finishing it > there is little bit improved EHCI driver - ehci.c. > Changes in other files are still the same (more precisely, I hope - I > didn't check if there are some other unrelated changes from anybody else > in newest trunk revision...) - usb_ehci_110625_0. > Very impressive.
> Remaining issue: > - Any HID low speed device attached via USB 2.0 hub does not work. (It > is most probably because bulk split transfers are differently handled in > comparison with interrupt split transfers. Probably only one solution is > to add interrupt transfers into EHCI driver.) > Have you tried a device using bulk transfers? (e.g. a usbserial adapter) > I made short test, driver looks to be working. > > But there can be still mistakes in CPU/LE and VIRT/PHYS conversions - I > cannot test them on x86 machine (or at least I don't know how to do > it...). Right now I'm not home and have only this laptop. Its pecularity is of having EHCI without any apparent signs of companion controller. This Sunday I should be able to test your patch on fuloong. > Could You (or, of course, anybody else...) test EHCI patch on: > - some "big endian" machine ? Right now we don't have the PCI support for either sparc64 or powerpc. But since the firmware there provides PCI functions it would be fixable. But I don't remember if my machines have EHCI. They are some cheap old stuff. > - some machine with different virtual/physical addressing, i.e. like > Yeloong ? > When I get home. > What I didn't: > - ... packed isn't necessary here ... - GCC documentation says: > "packed > This attribute, attached to struct or union type definition, > specifies that each member of the structure or union is placed to > minimize the memory required." > > I.e., it is exactly what we need - members are stored in structure > without any additional space between them. Without this attribute > compiler can align structure members in any way (depend on its defaults > and global settings etc.) - so members can be aligned e.g. to 64 bits > inside structure and in this case we have structure which does not > correspond to EHCI HW data structure. > So, I left "packed" attribute in code. > No option will make GCC align on anything more than the size of element itself (otherwise there would be trouble with arrays). > static inline void * > grub_ehci_phys2virt (grub_uint32_t phys, struct grub_pci_dma_chunk *chunk) > { > if (!phys) > return NULL; Address 0, as well as (void *) 0 may be valid in the hw context. Do you really use 0 or NULL as incorectness marker somewhere? > return (void *) (phys - grub_dma_get_phys (chunk) > + (grub_uint32_t) grub_dma_get_virt (chunk)); > } Even the low addresses can be mapped high in 64-bit systems. Correct expression is: return (grub_uint8_t *) grub_dma_get_virt (chunk) + (phys - grub_dma_get_phys (chunk)); > static inline grub_uint32_t > grub_ehci_virt2phys (void *virt, struct grub_pci_dma_chunk *chunk) > { > if (!virt) > return 0; ditto > return ((grub_uint32_t) virt - (grub_uint32_t) grub_dma_get_virt (chunk) > + grub_dma_get_phys (chunk)); Should be: return ((grub_uint8_t *) virt - (grub_uint8_t *) grub_dma_get_virt (chunk)) + grub_dma_get_phys (chunk); Actually these 2 functions can be moved into a .h file > /* If this is not an EHCI controller, just return. */ > if (class != 0x0c || subclass != 0x03 || interf != 0x20) > return 0; I'll add geode here once I get home. > grub_dprintf ("ehci", "EHCI grub_ehci_pci_iter: class OK\n"); > > /* Check Serial Bus Release Number */ > addr = grub_pci_make_address (dev, GRUB_EHCI_PCI_SBRN_REG); > release = grub_pci_read_byte (addr); > if (release != 0x20) > { > grub_dprintf ("ehci", "EHCI grub_ehci_pci_iter: Wrong SBRN: %0x\n", > release); > return 0; > } > > grub_dprintf ("ehci", "EHCI grub_ehci_pci_iter: bus rev. num. OK\n"); > > /* Determine EHCI EHCC registers base address. */ > addr = grub_pci_make_address (dev, GRUB_PCI_REG_ADDRESS_REG0); > base = grub_pci_read (addr); > addr = grub_pci_make_address (dev, GRUB_PCI_REG_ADDRESS_REG1); > base_h = grub_pci_read (addr); > /* Stop if registers are mapped above 4G - GRUB does not currently > * work with registers mapped above 4G */ > if (((base & GRUB_PCI_ADDR_MEM_TYPE_MASK) != GRUB_PCI_ADDR_MEM_TYPE_32) > && (base_h != 0)) > { > grub_dprintf ("ehci", > "EHCI grub_ehci_pci_iter: registers above 4G are not > supported\n"); > return 1; > } > > grub_dprintf ("ehci", "EHCI grub_ehci_pci_iter: 32-bit EHCI OK\n"); > > > /* Allocate memory for the controller and fill basic values. */ > e = grub_zalloc (sizeof (*e)); > if (!e) > return 1; > e->framelist_chunk = NULL; > e->td_chunk = NULL; > e->qh_chunk = NULL; > e->iobase_ehcc = (grub_uint32_t *) (base & GRUB_EHCI_ADDR_MEM_MASK); > You need to use grub_pci_device_map_range. > /* Determine base address of EHCI operational registers */ > e->iobase = (grub_uint32_t *) ((grub_uint32_t) e->iobase_ehcc + > (grub_uint32_t) grub_ehci_ehcc_read8 (e, > > GRUB_EHCI_EHCC_CAPLEN)); > Should be: e->iobase = (volatile grub_uint32_t *) ((grub_uint8_t *) e->iobase_ehcc + grub_ehci_ehcc_read8 (e, GRUB_EHCI_EHCC_CAPLEN)); > grub_dprintf ("ehci", > "EHCI grub_ehci_pci_iter: iobase of oper. regs: %08x\n", > (grub_uint32_t) e->iobase); There is %p for this. > /* Note: QH 0 and QH 1 are reserved and must not be used anywhere. > * QH 0 is used as empty QH for framelist > * QH 1 is used as starting empty QH for asynchronous schedule > * QH 1 must exist at any time because at least one QH linked to > * itself must exist in asynchronous schedule > * QH 1 has the H flag set to one */ > > grub_dprintf ("ehci", "EHCI grub_ehci_pci_iter: QH/TD init. OK\n"); > > /* Determine and change ownership. */ > if (e->pcibase_eecp) /* Ownership can be changed via EECP > only */ > { > usblegsup = grub_pci_read (e->pcibase_eecp); > if (usblegsup & GRUB_EHCI_BIOS_OWNED) > { > grub_dprintf ("ehci", > "EHCI grub_ehci_pci_iter: EHCI owned by: BIOS\n"); > /* Ownership change - set OS_OWNED bit */ > grub_pci_write (e->pcibase_eecp, usblegsup | GRUB_EHCI_OS_OWNED); > /* Ensure PCI register is written */ > grub_pci_read (e->pcibase_eecp); > > /* Wait for finish of ownership change, EHCI specification > * doesn't say how long it can take... */ > maxtime = grub_get_time_ms () + 1000; > while ((grub_pci_read (e->pcibase_eecp) & GRUB_EHCI_BIOS_OWNED) > && (grub_get_time_ms () < maxtime)); > if (grub_pci_read (e->pcibase_eecp) & GRUB_EHCI_BIOS_OWNED) > { > grub_dprintf ("ehci", > "EHCI grub_ehci_pci_iter: EHCI change ownership > timeout"); > /* Change ownership in "hard way" - reset BIOS ownership */ > grub_pci_write (e->pcibase_eecp, GRUB_EHCI_OS_OWNED); > /* Ensure PCI register is written */ > grub_pci_read (e->pcibase_eecp); In this case you need to disable SMI interrupts (it's in register EECP+1 AFAIR) > } > } > else if (usblegsup & GRUB_EHCI_OS_OWNED) > /* XXX: What to do in this case - nothing ? Can it happen ? */ > grub_dprintf ("ehci", "EHCI grub_ehci_pci_iter: EHCI owned by: OS\n"); > else > { > grub_dprintf ("ehci", > "EHCI grub_ehci_pci_iter: EHCI owned by: NONE\n"); > /* XXX: What to do in this case ? Can it happen ? Yes. It means controller is unused. > * Is code below correct ? */ > /* Ownership change - set OS_OWNED bit */ > grub_pci_write (e->pcibase_eecp, GRUB_EHCI_OS_OWNED); > /* Ensure PCI register is written */ > grub_pci_read (e->pcibase_eecp); I'd also clean SMI, just to be sure. -- Regards Vladimir 'φ-coder/phcoder' Serbinenko
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel