Discussion:
[PATCH] EHCI driver - USB 2.0 support
Aleš Nesrsta
2011-06-25 19:13:19 UTC
Permalink
Hi,

because I still see no EHCI driver in GRUB for long time, I slowly
prepared myself something what looks to be working...
EHCI driver code is based on UHCI (OHCI) GRUB driver, no other source
code was used (copied).

There are included two files:
ehci.c - The driver itself. It should be located on the same place as
uhci.c, ohci.c etc., i.e. in grub-core/bus/usb.
usb_ehci_110625_0 - Patch which includes changes in some other existing
files in GRUB. Patch was made against today's GRUB trunk revision 3350.

Limitations - see ehci.c.

Known issue:
USB keyboard (and probably any low speed HID device) is not working when
connected via USB 2.0 hub. I don't know why - control transfers are OK
but bulk transfer halts with STALL. Maybe USB 2.0 hub strictly
distinguishes between bulk / interrupt transfers when Split Transaction
is used. (?)

I shortly tested main functions:
- high speed device connected directly to EHCI port - working, OK
- low/full speed device connected directly to EHCI port - not working
but it is OK (it cannot work according to specification)
- high speed device connected via USB 2.0 hub - working, OK
- full speed device connected via USB 2.0 hub - working, OK
- low speed device connected via USB 2.0 hub - I have only keyboard and
it is not working - see known issue above.

I tried also test EHCI and UHCI drivers together - it also looks to be
working:
- full speed device connected directly to EHCI/UHCI port is working (via
UHCI driver), OK
- low speed device (keyboard) connected directly to EHCI/UHCI port is
working (via UHCI driver), OK

Best regards
Ales
Szymon Janc
2011-06-25 19:51:12 UTC
Permalink
Post by Aleš Nesrsta
Hi,
Hi Aleš,
Post by Aleš Nesrsta
because I still see no EHCI driver in GRUB for long time, I slowly
prepared myself something what looks to be working...
EHCI driver code is based on UHCI (OHCI) GRUB driver, no other source
code was used (copied).
This is a really cool news :)
Post by Aleš Nesrsta
- high speed device connected directly to EHCI port - working, OK
- low/full speed device connected directly to EHCI port - not working
but it is OK (it cannot work according to specification)
In such case EHCI should handover port to a companion 1.1 controller.
(see section 4.2 of EHCI specification)
--
Szymon K. Janc
***@janc.net.pl // GG: 1383435
Aleš Nesrsta
2011-06-26 20:37:48 UTC
Permalink
Hi,

To Szymon (& Vladimir too...):
...
Post by Szymon Janc
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
Vladimir 'φ-coder/phcoder' Serbinenko
2011-07-21 21:59:37 UTC
Permalink
Post by Aleš Nesrsta
Ufff, You sent a lot of comments... :-)
Don't take it as not appreciation of your work. I appreciate it and if
you have no time to do the requested fixes I can help you. It's very
important patch.
Post by Aleš Nesrsta
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 understand.
Post by Aleš Nesrsta
I will rewrite some part according to Your comments but don't expect it
too early...
... macro->inline func. - I like macros... :-) But You are right.
Macros are a good way to hit quirks and unless really necessary are to
be avoided.
Post by Aleš Nesrsta
... 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.
No options, just defaults.
Post by Aleš Nesrsta
... __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.
You already ensure that the structures are aligned at 16 bytes which is
more than 4 bytes.
Post by Aleš Nesrsta
... 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...
Even on 64-bit machines it's fine and recommended to keep all structures
in first 4G. We currently do so for AHCI
Post by Aleš Nesrsta
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.
P2V wouldn't be single-valued. We have grub_vtop but the DMA address to
write into registers is different from the physical address on Yeeloong
so DMA needs separate function to retrieve this value.
Post by Aleš Nesrsta
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 does.
Post by Aleš Nesrsta
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.
There is no need. We can just stick to use first 4G of memory to stay on
the safe side. For GRUB 3G of RAM is much more than we need.
Post by Aleš Nesrsta
... "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 ?
It doesn't. on i386-pc GRUB is limited to first 4G
Post by Aleš Nesrsta
... "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... ???
But its a pointer to an area which has this property, hence it needs
volatile attribute.
Post by Aleš Nesrsta
... "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.
Read bytes is both correct and fine. Trouble is incrementing an address
like e.g. addr = ...; addr++;
Post by Aleš Nesrsta
I will also add the "hard way" of ownership as You propose.
I have a laptop we can test it on.
Post by Aleš Nesrsta
Best regards
Ales
_______________________________________________
Grub-devel mailing list
https://lists.gnu.org/mailman/listinfo/grub-devel
--
Regards
Vladimir 'φ-coder/phcoder' Serbinenko
Aleš Nesrsta
2011-08-27 16:42:18 UTC
Permalink
Hi Vladimir,

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.

Changes were done according to Your notes:
- ... run indent ... - done
- ... macro into inline function ... - done
- ... volatile attribute ... - done
- ... pointer arithmetic via char* ... - done
- ... access to PCI register in correct way in ownership change ... -
done
- ... "hard way" of ownership change ... - done
- ... use of grub_memalign_dma32 ... - done
- (corrected year in header of file...)

Result:
- driver should run on 64-bits machines with exception if EHCI I/O
registers are mapped above 4G
- driver should run on machines with different virtual/physical
addressing
- driver should run on "big endian" machines (but also original "zero
version" EHCI patch supports big endian)

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.)

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...).
Could You (or, of course, anybody else...) test EHCI patch on:
- some "big endian" machine ?
- some machine with different virtual/physical addressing, i.e. like
Yeloong ?


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.

Best regards
Ales
Vladimir 'φ-coder/phcoder' Serbinenko
2011-09-30 15:37:16 UTC
Permalink
That must be a too long interval of writing e-mail. I started it a month
ago, then interrupted and now finishing it
Post by Aleš Nesrsta
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.
Post by Aleš Nesrsta
- 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)
Post by Aleš Nesrsta
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.
Post by Aleš Nesrsta
- 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.
Post by Aleš Nesrsta
- some machine with different virtual/physical addressing, i.e. like
Yeloong ?
When I get home.
Post by Aleš Nesrsta
"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).
Post by Aleš Nesrsta
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?
Post by Aleš Nesrsta
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));
Post by Aleš Nesrsta
static inline grub_uint32_t
grub_ehci_virt2phys (void *virt, struct grub_pci_dma_chunk *chunk)
{
if (!virt)
return 0;
ditto
Post by Aleš Nesrsta
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
Post by Aleš Nesrsta
/* 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.
Post by Aleš Nesrsta
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.
Post by Aleš Nesrsta
/* 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));
Post by Aleš Nesrsta
grub_dprintf ("ehci",
"EHCI grub_ehci_pci_iter: iobase of oper. regs: %08x\n",
(grub_uint32_t) e->iobase);
There is %p for this.
Post by Aleš Nesrsta
/* 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)
Post by Aleš Nesrsta
}
}
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.
Post by Aleš Nesrsta
* 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
Aleš Nesrsta
2011-10-01 08:15:41 UTC
Permalink
Vladimir 'φ-coder/phcoder' Serbinenko píše v Pá 30. 09. 2011 v 17:37
Post by Vladimir 'φ-coder/phcoder' Serbinenko
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
Post by Vladimir 'φ-coder/phcoder' Serbinenko
Post by Aleš Nesrsta
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.
Post by Aleš Nesrsta
- 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)
Post by Aleš Nesrsta
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.
Post by Aleš Nesrsta
- 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.
Post by Aleš Nesrsta
- some machine with different virtual/physical addressing, i.e. like
Yeloong ?
When I get home.
Post by Aleš Nesrsta
"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).
Post by Aleš Nesrsta
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?
Post by Aleš Nesrsta
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
return (grub_uint8_t *) grub_dma_get_virt (chunk) + (phys -
grub_dma_get_phys (chunk));
Post by Aleš Nesrsta
static inline grub_uint32_t
grub_ehci_virt2phys (void *virt, struct grub_pci_dma_chunk *chunk)
{
if (!virt)
return 0;
ditto
Post by Aleš Nesrsta
return ((grub_uint32_t) virt - (grub_uint32_t) grub_dma_get_virt (chunk)
+ grub_dma_get_phys (chunk));
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
Post by Aleš Nesrsta
/* 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.
Post by Aleš Nesrsta
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.
Post by Aleš Nesrsta
/* 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));
e->iobase = (volatile grub_uint32_t *) ((grub_uint8_t *) e->iobase_ehcc +
grub_ehci_ehcc_read8 (e,
GRUB_EHCI_EHCC_CAPLEN));
Post by Aleš Nesrsta
grub_dprintf ("ehci",
"EHCI grub_ehci_pci_iter: iobase of oper. regs: %08x\n",
(grub_uint32_t) e->iobase);
There is %p for this.
Post by Aleš Nesrsta
/* 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)
Post by Aleš Nesrsta
}
}
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.
Post by Aleš Nesrsta
* 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
https://lists.gnu.org/mailman/listinfo/grub-devel
Vladimir 'φ-coder/phcoder' Serbinenko
2011-10-01 18:05:07 UTC
Permalink
Vladimir 'φ-coder/phcoder' Serbinenko píše v Pá 30. 09. 2011 v 17:37
Post by Vladimir 'φ-coder/phcoder' Serbinenko
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.
Ok, I'll do it since EHCI is important. I also feel like there is no
need to maroon it in experimental
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... :-) ).
Don't consider me some kind of dictator, I just want some coherency in
design, rather than having incoherent problems.
1. I think there is difference between structures and arrays, compiler
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.
Reading and writing into a byte aligned on word doesn't bring any
advantage and wastes memory and decreases portability so no compiler
will ever do it.
The whole reason to align is because of performance advantages and
instruction availability.
--
Regards
Vladimir 'φ-coder/phcoder' Serbinenko
Vladimir 'φ-coder/phcoder' Serbinenko
2011-10-01 20:04:35 UTC
Permalink
Post by Vladimir 'φ-coder/phcoder' Serbinenko
Vladimir 'φ-coder/phcoder' Serbinenko píše v Pá 30. 09. 2011 v 17:37
Post by Vladimir 'φ-coder/phcoder' Serbinenko
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.
Ok, I'll do it since EHCI is important. I also feel like there is no
need to maroon it in experimental
Pushed corrected version to branches/ehci. Not tested yet.
--
Regards
Vladimir 'φ-coder/phcoder' Serbinenko
Aleš Nesrsta
2011-10-01 22:03:58 UTC
Permalink
Hi Vladimir,
see below...
Post by Vladimir 'φ-coder/phcoder' Serbinenko
Post by Aleš Nesrsta
...
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.
Reading and writing into a byte aligned on word doesn't bring any
advantage and wastes memory and decreases portability so no compiler
will ever do it.
The whole reason to align is because of performance advantages and
instruction availability.
Aah, it looks like we are both speaking about different things !
I am talking about alignment of structure MEMBERS.
You are probably talking about alignment of WHOLE STRUCTURE.

First to alignment of whole structure:
Structure(s), we are talking about, are special structures related to HW
which must be in any case aligned to 32 BYTES.
This is solved by grub_memalign allocation in driver code. I.e., structure
will never 1 BYTE or 1 WORD aligned, so there should not be any performance
problem.

Now to alignment of members in structure:
In these special HW related structures are DWORD variables which must be
placed in memory immediately one after another without any space between
them.
I.e., offsets of structure members in structure should look like that:
0x00000000 variable1
0x00000004 variable2
0x00000008 variable3
0x0000000c variable4
I am afraid new compilers in future can use default alignment 64 bits, so
offsets of structure members in structure can, without any attribute, look
like that:
0x00000000 variable1
0x00000008 variable2
0x00000010 variable3
0x00000018 variable4
To prevent this wrong situation, I am using "packed" attribute for
structure - it will prevent any change of structure members alignment in
future. Independent of default compiler alignment, the variables will be
placed in memory immediately one after another. As all variables are of
DWORD type and whole structure (begin of structure) is always aligned to 32
bytes (i.e. to multiple of DWORD), there also should not be any problem with
performance.

Even if it looks maybe little bit confusing and surprising, using of
"packed" attribute for whole structure does mean it is applied for any
member of structure (and not for whole structure) alignment - according e.g.
this
( http://gcc.gnu.org/onlinedocs/gcc/Type-Attributes.html ):
"...
packed
This attribute, attached to struct or union type definition, specifies that
each member (other than zero-width bitfields) of the structure or union is
placed to minimize the memory required. When attached to an enum definition,
it indicates that the smallest integral type should be used.
Specifying this attribute for struct and union types is equivalent to
specifying the packed attribute on each of the structure or union members.
Specifying the -fshort-enums flag on the line is equivalent to specifying
the packed attribute on all enum definitions.

..."

Best regards
Ales
Post by Vladimir 'φ-coder/phcoder' Serbinenko
--
Regards
Vladimir 'φ-coder/phcoder' Serbinenko
Best regards
Ales
Aleš Nesrsta
2011-10-01 19:01:16 UTC
Permalink
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
Post by Aleš Nesrsta
Vladimir 'φ-coder/phcoder' Serbinenko píše v Pá 30. 09. 2011 v 17:37
Post by Vladimir 'φ-coder/phcoder' Serbinenko
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.
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... :-) ).
1. I think there is difference between structures and arrays, compiler
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
Post by Vladimir 'φ-coder/phcoder' Serbinenko
Post by Aleš Nesrsta
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.
Post by Aleš Nesrsta
- 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)
Post by Aleš Nesrsta
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.
Post by Aleš Nesrsta
- 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.
Post by Aleš Nesrsta
- some machine with different virtual/physical addressing, i.e. like
Yeloong ?
When I get home.
Post by Aleš Nesrsta
"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).
Post by Aleš Nesrsta
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?
Post by Aleš Nesrsta
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
return (grub_uint8_t *) grub_dma_get_virt (chunk) + (phys -
grub_dma_get_phys (chunk));
Post by Aleš Nesrsta
static inline grub_uint32_t
grub_ehci_virt2phys (void *virt, struct grub_pci_dma_chunk *chunk)
{
if (!virt)
return 0;
ditto
Post by Aleš Nesrsta
return ((grub_uint32_t) virt - (grub_uint32_t) grub_dma_get_virt (chunk)
+ grub_dma_get_phys (chunk));
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
Post by Aleš Nesrsta
/* 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.
Post by Aleš Nesrsta
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.
Post by Aleš Nesrsta
/* 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));
e->iobase = (volatile grub_uint32_t *) ((grub_uint8_t *) e->iobase_ehcc +
grub_ehci_ehcc_read8 (e,
GRUB_EHCI_EHCC_CAPLEN));
Post by Aleš Nesrsta
grub_dprintf ("ehci",
"EHCI grub_ehci_pci_iter: iobase of oper. regs: %08x\n",
(grub_uint32_t) e->iobase);
There is %p for this.
Post by Aleš Nesrsta
/* 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)
Post by Aleš Nesrsta
}
}
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.
Post by Aleš Nesrsta
* 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
https://lists.gnu.org/mailman/listinfo/grub-devel
_______________________________________________
Grub-devel mailing list
https://lists.gnu.org/mailman/listinfo/grub-devel
Philipp Hahn
2011-11-03 09:25:23 UTC
Permalink
Hello,

I had a look at your EHCI driver, while investigating why grub-1.99
needs 2 minutes to load my 58 MB InitRd.
Post by Aleš Nesrsta
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.
Compiling on x86_86 failed for me in grub-core/bus/usb/usbhub.c:104
Post by Aleš Nesrsta
grub_dprintf ("usb", "Added new usb device: %08x, addr=%d\n",
(grub_uint32_t)dev, i);
because dev is a 64 bit pointer which GCC didn't like to cast to an
uint32 with the compiler options used by Debian. I fixed this locally by
changing those two lines to use '%p' instead of '%08x' and without the
cast:

grub_dprintf ("usb", "Added new usb device: %p, addr=%d\n",
dev, i);

Even without that I wasn't able to get EHCI working: If I insmod 'usbms'
and 'ehci', no disk is detected. Only when I insmod 'ohci' is the stick
detected. With 'set debug=ehci pager=1' I get some lines, before the
screen is spammed with 'STATUS'-lines and the computer reboots.
Did I miss something obvious or can I help debug this?

Sincerely
Philipp
--
Philipp Hahn Open Source Software Engineer ***@univention.de
Univention GmbH Linux for Your Business fon: +49 421 22 232- 0
Mary-Somerville-Str.1 D-28359 Bremen fax: +49 421 22 232-99
http://www.univention.de/
Aleš Nesrsta
2011-11-04 20:56:22 UTC
Permalink
Hi Phillip,

driver is "near zero" version and the problem with pointer in debug
message is one of problematic or potentially problematic things which
are known and which are waiting for repair - see into e-mail from
Vladimir, sent Fri, 30 Sep 2011 17:37:16. Maybe some of these known
things is the reason why EHCI dosn't work for You...
Unfortunately, I cannot fix and more deeply test it now as I am very
busy in last time and it will be not better till new year...

Maybe I can help You if You send debug output to me.
But there is only one possibility to get usable debug output - capture
it via serial line.
Any of current USB drivers produces lot of debug messages - it is
according to "hot-plug" system algorithm, which generates lot of
repeating messages and, additionally, there are some loops in USB device
detection which caused very very long delay when debug is on. Also there
should be printed out lot of values to have the possibility to find
consequences etc.

To get usable debug output I recommend for example this procedure:
- connect another computer via serial line - this computer will capture
debug messages into some file
- disconnect USB drive
- start grub into command line without any USB driver loaded
- set serial parameters as You need
- set "terminal_output console serial"
- test serial communication with second computer and start capturing
into file
- set "debug=all" and wait for command line
- "insmod ehci" and connect USB device few seconds later (usbms will be
loaded automatically if necessary)
- be patient - there will be lot of messages displayed, wait at least
approx. two minutes (or till computer reboots, if it happens...)

Then send debug output to me and I will try to find what happened (but
don't expect fast answer in any case, sorry...).

Best regards
Ales
Post by Philipp Hahn
Hello,
I had a look at your EHCI driver, while investigating why grub-1.99
needs 2 minutes to load my 58 MB InitRd.
Post by Aleš Nesrsta
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.
Compiling on x86_86 failed for me in grub-core/bus/usb/usbhub.c:104
Post by Aleš Nesrsta
grub_dprintf ("usb", "Added new usb device: %08x, addr=%d\n",
(grub_uint32_t)dev, i);
because dev is a 64 bit pointer which GCC didn't like to cast to an
uint32 with the compiler options used by Debian. I fixed this locally by
changing those two lines to use '%p' instead of '%08x' and without the
grub_dprintf ("usb", "Added new usb device: %p, addr=%d\n",
dev, i);
Even without that I wasn't able to get EHCI working: If I insmod 'usbms'
and 'ehci', no disk is detected. Only when I insmod 'ohci' is the stick
detected. With 'set debug=ehci pager=1' I get some lines, before the
screen is spammed with 'STATUS'-lines and the computer reboots.
Did I miss something obvious or can I help debug this?
Sincerely
Philipp
Cui Lei
2011-08-19 02:58:00 UTC
Permalink
Thank you for your help, very much! ^_^
This problem have been resolved and I can usb the usb_keyborard under
grub-shell and I can boot ubuntu11.04 from usb disk.
My mainboard is via 8595a, the usb controller is uhci.
I resolved it by add these code in the grub-core/bus/usb/uhci.c:

(1)
182 /*Set bus master*/
183 addr = grub_pci_make_address (dev, GRUB_PCI_REG_COMMAND);
184 grub_uint16_t val = grub_pci_read_word(addr);
185 val = (val & ~0) | GRUB_PCI_COMMAND_BUS_MASTER;
186 grub_pci_write_word(addr, val);

(2)
203 // Reset PIRQ and SMI
204 addr = grub_pci_make_address (dev, 0xC0);
//USBLEGSUP 0xc0
205 grub_pci_write_word(addr, 0x8f00); //USBLEGSUP_RWC
0x8f00 /* the R/WC bits */
206 // Reset the HC
207 grub_uhci_writereg16(u, GRUB_UHCI_REG_USBCMD, 0x0002);
//USBCMD_HCRESET 0x0002
208 grub_millisleep(5);
209 // Disable interrupts and commands (just to be safe).
210 grub_outw (0, u->iobase + 4); //USBINTR 4 /*Interrupt
enable register*/
211 grub_uhci_writereg16 (u, GRUB_UHCI_REG_USBCMD, 0);

I don't know whether it is useful to the other one, but may be a reference.

BRs,

Rock.
Hi,
I shortly looked into ML to Your posts. As I saw short part of debug
output in one of Your e-mail, GRUB freezes when it wants to get device
descriptor - more precisely, when it requests first 8 bytes of device
descriptor. It is the first thing which is done after address is
assigned to the device.
So, it looks like device does not set address properly (even if control
message Set Address returns success) or happened something else what
prevent device to respond (but I don't know what...).
...
/* Wait "recovery interval", spec. says 2ms */
grub_millisleep (2);<<<<---- HERE (try 4ms or more)
grub_usb_device_attach (dev);
...
...
/* Enable the port. */
err = hub->controller->dev->portstatus (hub->controller, portno, 1);
if (err)
return;
hub->controller->dev->pending_reset = grub_get_time_ms () + 5000;
grub_millisleep (10);<<<<---- maybe here also
/* Enable the port and create a device. */
dev = grub_usb_hub_add_dev (hub->controller, speed, portno, 0);
hub->controller->dev->pending_reset = 0;
if (! dev)
return;
...
If this will not help You, I currently have no other idea what could be
the reason of timeout.
I think You don't need EHCI because it looks like Set Address control
message works (at least it does not return error), i.e. You probably
have OHCI or UHCI USB (companion) controller on computer and Your device
is working at full or low speed with Your USB controller.
By the way, for the first look into ML I did not find which USB
controller You have - OHCI/UHCI ? (which driver/module are You using -
ohci/uhci?) - and which machine/architecture is the computer You are
trying to boot with GRUB2 - ?
I sometimes had some unidentified problems on my UHCI/EHCI controller,
mostly with port powering - UHCI does not have power management but EHCI
does and if EHCI is not properly initialized by BIOS (it could be Your
case with coreboot, maybe ?) then USB ports are not properly powered.
Another BIOS (coreboot?) issue could be improper handling of USB
controller ownership.
Do You have USB device connected directly into root port or via some USB
hub ? Try to do it in opposite way (i.e. if You are not using the USB
hub, try use it and connect USB device via hub - maybe it helps...)
Hmmm, I remember now one issue which could be related to Your problem.
On my very old machine with OHCI USB controller some devices are not
working "for the first time". I am still not able to debug why it
happened (it does not happened when full debug is active - so it looks
like it is related to some timing). But I am afraid it will be not Your
case because device stops working after it is recognized, configured,
usbms module loaded and GRUB USB device usb0 created.
But - try load ohci/uhci module when USB disk is connected and then
disconnect and connect it again after few seconds. In my case device
becomes working as new usb device (i.e. usb1).
Additionally, lot of manufacturers does not follow USB or USBMS
specifications, as You can read in Linux source code of USB controllers
and USB mass storage devices and related documentation.
Did You tried more different USB mass storage devices ?
What is manufacturer& type of Your USB mass storage device ?
Of course, You can also try EHCI driver, it maybe can solve Your problem
because of little bit different ports/devices handling. But EHCI driver
is currently highly experimental, it still exists only as uncorrected
and not accepted "patch". I have to do some improvement but I don't have
sufficient time still, unfortunately...
If You want try to use it, You can get my patch from ML (sent at
25.6.2011) and use it with related source code trunk branch revision
(maybe also any later or current revision, because USB parts of GRUB are
not frequently changed). Please also read about know issue and another
limitations of the "zero version" of EHCI driver - e.g. it may not work
if Your PC is not x86 machine or USB registers are mapped above 4GB etc.
Sorry if You will wait longer time for my response in future - I don't
check the post so often and additionally currently I am (and probably
will be) longer time too busy - I am not regular GRUB2 contributor, I do
something for GRUB2 USB part only time to time...
BRs,
Ales
Post by Szymon Janc
Hi Aleš,
I am trying to boot OS from USB disk, I use coreboot-v4 with grub2 as
payload, but my usb disk can not been
detect. I try to use usb-keyboard, it is not working. I know you are
working on the EHCI driver from Vladimir ,
could you give me some advices? Vladimir said it may need EHCI driver,
but I think the usb device should run
with low-speed or full-speed if no EHCI driver.C
Looking forward to your reply.
BRs,
Rock Cui.
Vladimir 'φ-coder/phcoder' Serbinenko
2011-06-25 20:27:03 UTC
Permalink
Post by Aleš Nesrsta
Hi,
because I still see no EHCI driver in GRUB for long time, I slowly
prepared myself something what looks to be working...
EHCI driver code is based on UHCI (OHCI) GRUB driver, no other source
code was used (copied).
Very good.
Post by Aleš Nesrsta
- high speed device connected directly to EHCI port - working, OK
- low/full speed device connected directly to EHCI port - not working
but it is OK (it cannot work according to specification)
Ir must be rerouted to companion. Some controllers (like in my thinkpad)
have no companion. I haven't played with internals to see how it's done.
Post by Aleš Nesrsta
/* ehci.c - EHCI Support. */
/*
* GRUB -- GRand Unified Bootloader
* Copyright (C) 2008 Free Software Foundation, Inc.
Add 2011 here.
Post by Aleš Nesrsta
GRUB_MOD_LICENSE ("GPLv3+");
* - assumes 32 bits architecture, no IRQ
How exactly do you use this?
Post by Aleš Nesrsta
#define GRUB_EHCI_SPARAMS_PPC (1<<4) /* Power port control */
Please run indent on the file.
Post by Aleš Nesrsta
#define GRUB_EHCI_PORT_READ(e, port) \
grub_ehci_oper_read32 ((e), GRUB_EHCI_PORT_STAT_CMD + (port)*4)
Why not make it an inline static function?
Post by Aleš Nesrsta
#define GRUB_EHCI_PORT_RESBITS(e, port, bits) \
{ grub_ehci_oper_write32 ((e), GRUB_EHCI_PORT_STAT_CMD + (port)*4, \
GRUB_EHCI_PORT_READ((e), (port)) & GRUB_EHCI_PORT_WMASK & ~(bits)); \
GRUB_EHCI_PORT_READ((e), (port)); }
ditto
Post by Aleš Nesrsta
#define GRUB_EHCI_PORT_SETBITS(e, port, bits) \
{ grub_ehci_oper_write32 ((e), GRUB_EHCI_PORT_STAT_CMD + (port)*4, \
(GRUB_EHCI_PORT_READ((e), (port)) & GRUB_EHCI_PORT_WMASK) | (bits)); \
GRUB_EHCI_PORT_READ((e), (port)); }
ditto
Post by Aleš Nesrsta
/* EHCI Queue Element Transfer Descriptor (qTD) */
/* Align to 32-byte boundaries */
struct grub_ehci_td
{
/* EHCI HW part */
grub_uint32_t next_td; /* Pointer to next qTD */
grub_uint32_t alt_next_td; /* Pointer to alternate next qTD */
grub_uint32_t token; /* Toggle, Len, Interrupt, Page, Error, PID, Status */
grub_uint32_t buffer_page[GRUB_EHCI_TD_BUF_PAGES]; /* Buffer pointer (+ cur. offset in page 0 */
/* 64-bits part */
grub_uint32_t buffer_page_high[GRUB_EHCI_TD_BUF_PAGES];
/* EHCI driver part */
grub_ehci_td_t link_td; /* pointer to next free/chained TD */
grub_uint32_t size;
grub_uint32_t pad[1]; /* padding to some multiple of 32 bytes */
} __attribute__((packed));
packed isn't necessary here
Post by Aleš Nesrsta
/* EHCI Queue Head */
/* Align to 32-byte boundaries */
/* QH allocation is made in the similar/same way as in OHCI driver,
* because unlninking QH from the Asynchronous list is not so
* trivial as on UHCI (at least it is time consuming) */
struct grub_ehci_qh
{
/* EHCI HW part */
grub_uint32_t qh_hptr; /* Horiz. pointer & Terminate */
grub_uint32_t ep_char; /* EP characteristics */
grub_uint32_t ep_cap; /* EP capabilities */
grub_uint32_t td_current; /* current TD link pointer */
struct grub_ehci_td td_overlay; /* TD overlay area = 64 bytes */
/* EHCI driver part */
grub_uint32_t pad[4]; /* padding to some multiple of 32 bytes */
} __attribute__((packed));
Same here
Post by Aleš Nesrsta
/* EHCC registers access functions */
static inline grub_uint32_t
grub_ehci_ehcc_read32 (struct grub_ehci *e, grub_uint32_t addr)
{
return grub_le_to_cpu32 (
*((grub_uint32_t *)((grub_uint32_t)e->iobase_ehcc + addr)));
}
Convert to char * in order to do arithmetics, not grub_uint32_t. Also
you need to use volatile attribute in last conversion. Same for
following functions
Post by Aleš Nesrsta
/* Halt if EHCI HC not halted */
static grub_err_t
grub_ehci_halt (struct grub_ehci *e)
{
grub_uint64_t maxtime;
if ((grub_ehci_oper_read32 (e, GRUB_EHCI_STATUS)
& GRUB_EHCI_ST_HC_HALTED) == 0 ) /* EHCI is not halted */
{
/* Halt EHCI */
grub_ehci_oper_write32 (e, GRUB_EHCI_COMMAND,
~GRUB_EHCI_CMD_RUNSTOP
& grub_ehci_oper_read32 (e, GRUB_EHCI_COMMAND));
/* Ensure command is written */
grub_ehci_oper_read32 (e, GRUB_EHCI_COMMAND);
maxtime = grub_get_time_ms () + 1000; /* Fix: Should be 2ms ! */
while (((grub_ehci_oper_read32 (e, GRUB_EHCI_STATUS)
& GRUB_EHCI_ST_HC_HALTED) == 0)
&& (grub_get_time_ms () < maxtime));
if ((grub_ehci_oper_read32 (e, GRUB_EHCI_STATUS)
& GRUB_EHCI_ST_HC_HALTED) == 0 )
return GRUB_ERR_TIMEOUT;
}
return GRUB_ERR_NONE;
}
/* EHCI HC reset */
static grub_err_t
grub_ehci_reset (struct grub_ehci *e)
{
grub_uint64_t maxtime;
grub_ehci_oper_write32 (e, GRUB_EHCI_COMMAND,
GRUB_EHCI_CMD_HC_RESET
| grub_ehci_oper_read32 (e, GRUB_EHCI_COMMAND));
/* Ensure command is written */
grub_ehci_oper_read32 (e, GRUB_EHCI_COMMAND);
/* XXX: How long time could take reset of HC ? */
maxtime = grub_get_time_ms () + 1000;
while (((grub_ehci_oper_read32 (e, GRUB_EHCI_COMMAND)
& GRUB_EHCI_CMD_HC_RESET) != 0)
&& (grub_get_time_ms () < maxtime));
if ((grub_ehci_oper_read32 (e, GRUB_EHCI_COMMAND)
& GRUB_EHCI_CMD_HC_RESET) != 0 )
return GRUB_ERR_TIMEOUT;
return GRUB_ERR_NONE;
}
/* PCI iteration function... */
static int NESTED_FUNC_ATTR
grub_ehci_pci_iter (grub_pci_device_t dev,
grub_pci_id_t pciid __attribute__((unused)))
{
grub_pci_address_t addr;
grub_uint8_t release;
grub_uint32_t class_code;
grub_uint32_t interf;
grub_uint32_t subclass;
grub_uint32_t class;
grub_uint32_t base, base_h;
struct grub_ehci *e;
grub_uint32_t eecp_offset;
grub_uint32_t fp;
int i;
grub_uint32_t usblegsup = 0;
grub_uint64_t maxtime;
grub_uint32_t n_ports;
grub_dprintf ("ehci", "EHCI grub_ehci_pci_iter: begin\n");
addr = grub_pci_make_address (dev, GRUB_PCI_REG_CLASS);
class_code = grub_pci_read (addr) >> 8;
interf = class_code & 0xFF;
subclass = (class_code >> 8) & 0xFF;
class = class_code >> 16;
/* If this is not an EHCI controller, just return. */
if (class != 0x0c || subclass != 0x03 || interf != 0x20)
return 0;
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 not 32bit address type - this driver does not currently
* work with 64bit - maybe later... */
No need to specifically exclude those. Just zero-pad address.
Post by Aleš Nesrsta
/* 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));
e->iobase should have volatile attribute
Post by Aleš Nesrsta
/* Reserve a page for the frame list - it is accurate for max.
* possible size of framelist. But currently it is not used. */
e->framelist = grub_memalign (4096, 4096);
if (! e->framelist)
goto fail;
/* XXX: The currently allowed EHCI pointers are only 32 bits,
* make sure this code works on on 64 bits architectures. */
That's why you have to use dmaalign32
Post by Aleš Nesrsta
/* Determine and change ownership. */
/* XXX: Really should we handle it ?
* Is BIOS code active when GRUB is loaded and can BIOS properly
* "assist" in change of EHCI 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 */
/* XXX: Is PCI address for grub_pci_write_byte() correct ? */
grub_pci_write_byte (e->pcibase_eecp + GRUB_EHCI_OS_OWNED_OFF, 1);
Arithmetics with PCI address aren't guaranteed to be available or to
behave in a sane way.
Post by Aleš Nesrsta
/* 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_error (GRUB_ERR_TIMEOUT, "EHCI grub_ehci_pci_iter:EHCI change ownership timeout");
In this case you have to take the ownership the hard way. You clean
GRUB_EHCI_BIOS_OWNED yourself and disable all SMM interrupts (next EECP
field)
--
Regards
Vladimir 'φ-coder/phcoder' Serbinenko
Continue reading on narkive:
Loading...