grub-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] EHCI driver - USB 2.0 support - addendum...


From: Aleš Nesrsta
Subject: Re: [PATCH] EHCI driver - USB 2.0 support - addendum...
Date: Sat, 01 Oct 2011 21:01:16 +0200

Hi,
I am sorry, I forgot to say something to Your question about testing in
previous e-mail:

I tested (but not so much deeply) in fact all combinations which I can
realize with EHCI/UHCI controller, USB 2.0 hub, approx. 5 different USB
disks and one USB keyboard. Serial adapter I did not test, unfortunately
I cannot use it currently.

I don't have another low or full speed device except of mentioned
keyboard and one USB disk (GARMIN GPS unit in mass-storage mode).
And only one thing which is not working is combination EHCI - USB 2.0
hub - USB keyboard.
Split transfer returns some error to driver when used for interrupt
transfer of low-speed device (I forgot which error code exactly, it is
long time ago...).
I checked if I set something wrong in split-transfer QD/TD for the low
speed device but I see nothing - maybe somebody other will find
mistake...?
So, it looks like problem with interrupt transfer - probably it cannot
be "replaced" by bulk transfer when used in split-transfers.

Unfortunately, using of USB keyboard via USB 2.0 hub could be often
case, as some keyboards have hub built inside and the keyboard itself is
connected via this internal hub. So maybe we will need to find some
workaround.

But first try to test EHCI yourself on Your machine(s) and then we will
see...

Best regards
Ales


Aleš Nesrsta píše v So 01. 10. 2011 v 10:15 +0200:
> 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
> 
> 
> 
> _______________________________________________
> Grub-devel mailing list
> address@hidden
> https://lists.gnu.org/mailman/listinfo/grub-devel





reply via email to

[Prev in Thread] Current Thread [Next in Thread]