Re: [PATCH] EHCI driver - USB 2.0 support

[Aleš Nesrsta]

Vladimir 'φ-coder/phcoder' Serbinenko píše v Pá 30. 09. 2011 v 17:37
+0200:
> That must be a too long interval of writing e-mail. I started it a month
> ago, then interrupted and now finishing it
No problem, I have often very, very long "response time" too...
(sometimes infinite...) :-)

According to all comments/changes below - in fact I agree with all, but
I will wait with changes in code until You try EHCI on fuloong or
another machine(s), as there could be some additional mistakes/changes
in code after test - or feel totally free to make Your own corrected
starting version of this driver code for any experimental branch or
trunk.

Currently I have only note to discussed "packed" attribute "problem":
I have no problem to delete it - especially if it is usual programmer
praxis in such cases (or You can do it instead of me without my
agreement - no problem, because You are the maintainer of whole code and
You are making code rules... :-) ).

Only to clarify my point of view:

1. I think there is difference between structures and arrays, compiler
can (must) use different alignment for each case:
For array members - there it must be according to type of array, resp.
length of array member
For structure members (and possibly whole structure alignment) - there
can be used any alignment, I see no reason why should be structure
member alignment generally restricted in some way even if the whole
structure is then used as array member

2. AFAIK (but maybe it is not true, maybe it was never true...?)
compilers are using default alignment of variables usually according to
native length of CPU "word". So, as we have 64-bit machines now, it
could be only question of time when compilers will use 64 bits alignment
as default.

3. Additionally, such attribute can be something like "warning" for
programmers which will want to modify this code in future - "...this is
some special structure related to HW, be careful!..." :-)

So, thats are reasons why I think it is more safe using of "packed"
attribute even if it is not necessary now - it is prepared for the
future...
But if some of assumptions mentioned above are not true then whole idea
is wrong  - I am not C compiler (nor driver) specialist as You can see
from my code... :-)
So, don't spend unusual time to discuss about some attribute, simply
delete it if You want - the code is open for change... :-)

Best regards
Ales

> > 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.
> 
> _______________________________________________
> Grub-devel mailing list
> address@hidden
> https://lists.gnu.org/mailman/listinfo/grub-devel