[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] EHCI driver - USB 2.0 support
From: |
Vladimir 'φ-coder/phcoder' Serbinenko |
Subject: |
Re: [PATCH] EHCI driver - USB 2.0 support |
Date: |
Fri, 30 Sep 2011 17:37:16 +0200 |
User-agent: |
Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.21) Gecko/20110831 Iceowl/1.0b2 Icedove/3.1.13 |
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
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- Re: [PATCH] EHCI driver - USB 2.0 support,
Vladimir 'φ-coder/phcoder' Serbinenko <=