Re: [PATCH] EHCI driver - USB 2.0 support

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

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

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



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