Re: [PATCH] EHCI driver - USB 2.0 support

[Aleš Nesrsta]

Hi,

To Szymon (& Vladimir too...):
...
> In such case EHCI should handover port to a companion 1.1 controller.
> (see section 4.2 of EHCI specification)

Yes, driver does it in such way - but I tested it first alone, without
any other USB driver, and in such case companion controllers of course
did not work. I wrote it maybe little bit not clearly in my e-mail...

I did two kind of tests:
1. Loaded only EHCI driver (paragraph beginning with "I shortly tested
main functions..."
2. Loaded EHCI driver and UHCI driver (for companion controllers;
paragraph beginning with "I tried also test EHCI and UHCI drivers
together..."

As You can see, in second kind of test is full/low speed working even if
it is connected to root port (EHCI/UHCI shared).

The only one thing, which is not working, is low speed HID device
connected via USB 2.0 hub (because in both cases the USB 2.0 hub is
handled  by EHCI). As I wrote, I suspect some bulk->interrupt
transaction conflict inside USB 2.0 hub when it is done via Split
Transaction from EHCI. I did not study USB 2.0 and EHCI specifications
too deeply to be sure...

There could be little bit another but related possible issue:
Driver is currently very simple and it does not test how many ports of
companion controllers are used and if some of them is free or not -> I
don't know what can happen in case when there is no free port of
companion controller - I cannot test it on my HW... Vladimir wrote it
has HW which has no companion controllers, so he can test it...


To Vladimir:

Ufff, You sent a lot of comments... :-)
Note, it is first working/tested version... My main problem is the time,
so I wrote it piece by piece through approx. half of year, often I
forgot what I did before... - so the code looks like it looks...

I will rewrite some part according to Your comments but don't expect it
too early...


Answers/questions to most of Your comments:

... macro->inline func. - I like macros... :-) But You are right.

... run indent - Why not. But what options of indent are preferred for
GRUB source ? I can set in the same way also my favorite editor (Geany)
to prevent necessity of such source code additional processing.

... __attribute__((packed)) - I remember, we probably discussed the same
thing on OHCI/UHCI in past. In these drivers are such attributes still
used for HW structures. I think it is more safe - we probably cannot
assume that compiler will align structure members to 32 bits.

... only 32 bits - The driver is very simple, similarly as UHCI driver.
EHCI can support memory higher than 4G but I currently don't care about
it, I fill all related registers with zero... I cannot test it (my
computer is little bit older and cannot use more than 4GB RAM...) and,
generally, I don't have experiences with 64 bits...
Similarly, to be simple, driver currently does not use DMA allocations
(as UHCI driver) - there is too much work with it... :-) - some thing
should be done little bit different because in GRUB is missing some
function which can convert DMA address to virtual address and vice
versa. I made some helping functions in OHCI driver but I am not sure if
these functions are safe because of possible not continuous DMA memory -
or does the function grub_memalign_dma32 ensure that whole allocated
memory is continuous in both DMA and virtual spaces in every case ? (It
is maybe stupid question but I don't want to study whole GRUB code
related to memory management, additionally, I don't know details about
memory management of non-x86 architectures...).

Under term "full 64-bit compatible" I understand that any register,
QH/TD structure and any buffer data can be above 4G - maybe it is wrong
from my side.
So, to be fully 64-bit compatible, if memory for framelist, QH and TD is
allocated above 4G, CTRLDSSEGMENT register should be properly set - and,
additionally, framelist, QH and TD should be in the same 4G memory area
and should not cross any 4G boundary.
Of course, CTRLDSSEGMENT register is set to zero in current driver
version and similarly also higher dwords of buffer address are set to
zero inside QH/TD structures - but it is also 32-bit simplification
because it assumes that memory of transfer buffer is below 4G in every
case.

... "No need to specifically exclude those. Just zero-pad address." -
Hm, I think it could be not so easy because (base_h !=0) means that EHCI
I/O memory registers are mapped above 4G... Maybe stupid question - does
it work access to mapped I/O in this case properly in GRUB ?

... "e->iobase should have volatile attribute" - I have related but
probably stupid question - why does work evaluation of QH/TD values in
runtime properly even if related variables are not declared as
"volatile" ? (It is the same in UHCI/OHCI drivers.)
AFAIK "volatile" should be used for variables which can be modified
"outside" of compiler code, i.e. by HW/DMA. So, I cannot understand why
for example e->iobase should be "volatile" (even if this variable cannot
be changed by HW/DMA - but, of course, values of memory area related to
this pointer can be changed by HW - maybe this is the reason (?)) and
structures QH/TD need not to be "volatile" even if their values can be
changed by HW/DMA... ???

... "Arithmetics with PCI address aren't guaranteed to be available or
to behave in a sane way." - Hm, yes, I change it to usual construction -
read whole DWORD, change only one bit and write it back.
I will also add the "hard way" of ownership as You propose.

Best regards
Ales