Re: [RFT] Re: [Patch] [bug #26237] multiple problems with usb devices - faster OHCI

[Aleš Nesrsta]

Hi Vladimir,

in attachment you find patch which speed-up OHCI to (approx.) the same
speed as UHCI.
It is huge change of ohci.c file, so I attach also whole ohci.c file.
Changes in usbms and usbtrans are not important and can be omitted, they
are related only to debugging, as far as I remember...

Patch is done according current usb branch rev. 2426.
I did not commit anything into bazaar repository yet, at first, I have
some trouble with SSH, I cannot access repository via SFTP, and at
second, I am still not so familiar with bazaar...


Some maybe important things:
1.
It may not work on 64-bits computers.
Also it should be tested on Yeeloong and possibly also on computers with
Big Endian memory organization.

2.
I am missing functions which make physical address from virtual and vice
versa, there are only functions which are doing chunk->phys and
chunk->virt. I made some workaround macros in ohci.c to solve my needs
but it is probably not too nice and systematic.

3.
I met some peculiar problems in this faster version and I found that the
problem is related probably to PCI posted writes, as mentioned by Seth
Goldberg (Sun, 23 May 2010 13:41:01 -0700 (22:41 CEST)).
So I added some workarounds for that in transfer function (read back of
written register) - it solved these problems. (It was not necessary in
"slow" version, because there is waiting for SOF after each transfer.)

4.
I added some additional things into grub_ohci_fini_hw. But I am not sure
with de-allocating of HCCA and EDs/TDs. I am little bit confused by
grub_ohci_fini_hw / grub_ohci_restore_hw - I don't know when it can be
called and what to do with memory allocation.
Probably we should de-allocate memory in grub_ohci_fini_hw for the case
if "rmmod ohci" is used - but if we de-allocate memory there, we should
probably allocate the memory again in grub_ohci_restore_hw. (?)

5.
"Allocation" and "de-allocation" of TDs is similar to uhci.c but not the
same as they are used in little bit another way than in UHCI.
There is missing "de-allocation" of EDs - I think it is currently not
necessary (there is no hot plugging, no (working) hub support), normally
we need only few EDs:
- one control with address 0
- one control with address 1
- two bulk
And bulk endpoints have often numbers 1 and 2, so even if hot plugging
will be implemented, once "allocated" EDs can be reused. But it can be
further enhancement in future, I focused only on speed-up of OHCI.

6.
Some answers:

Vladimir 'φ-coder/phcoder' Serbinenko wrote:
> Hello, Aleš. Today I merged usb branch into mainline. Now I've started
> some work towards hotplugging and module autoloading. Currently it just
> checks portsstatus to see newly connected devices. Touble is that if you
> disconnect a device and plug a new one at its place this routine won't
> notice anything because ports are polled only when enumerating
> usbdevices. We need a more reliable way to detect new devices. Perhaps
> we should try to assign addresses to any device without address and see
> if it responds to configuration request. If noone responds assume that
> no devices were newly connected.
> On Yeeloong 'usb' command lists hotplugged devices correctly but bulk
> transfer fails with donehead=0.

OHCI (but not only) related idea:
Did you are using the RootHubStatusChange bit in HcInterruptStatus
register ? It can be helpful - maybe.
But probably more useful will be ConnectStatusChange bit in
HcRhPortstatus[1:NDP] register (UHCI has also similar bit in PORTSC). If
this bit is set it is sure that device was disconnected and connected
again and needs to be re-addressed. And after re-addressing this bit
will be cleared. It could work.

> > Some additional informations:
> >
> > 1.
> > I found multi-LUN relatedproblem: I have one multi-LUN device - card
> > reader - and I tried plug into it more than one card. In this case "ls"
> > command reports bad number of filesystems (and also bad type of listed
> > partition filesystem) on another than first LUN.
> >
> > It looks like that (not true copy of console):
> > (usb0a) (usb0,msdos1) (usb0b,msdos1)
> > It is bad because LUN usb0b has four partitions with ext2 filesystem.
> >
> > When I try for example "ls (usb0b,4)/", grub returns for the first try
> > "error: unknown filesystem.", for second try grub does normal file list.
> > (the same for another partitions, i.e. (usb0b,1), (usb0b,2), (usb0b,3))
> >
> >   
> Perhaps there are strill problems with toggle bit. Problem is that if it
> gets out of sync it can only hard get in-sync again. Hence strange
> problems. But it would appear on single-LUN devices too.

I think the multi-LUN problem is not related to toggling. I looked into
debug messages and there is no problem on USB level - all transactions
are working well in my case.

> >
> > Oh, I find probably the mistake right now - there is missing my older
> > small patch in usbms.c - there is fast "hand made" "patch", I have no
> > more time, sorry:
> > usbms.c, line 240:
> > -   cbw.lun = scsi->lun << GRUB_SCSI_LUN_SHIFT;
> > +   cbw.lun = scsi->lun; /* In USB MS CBW are LUN bits on another place
> > than in SCSI CDB, both should be set correctly. */
> >
> >   
> Perhaps (scsi->lun << GRUB_SCSI_LUN_SHIFT) | scsi->lun ?

As I answered before, it should be "scsi->lun" only (according to
specification - reality can be different...).
But this problem remains even when I tested corrected usbms.c version.
As I wrote above, on USB level it looks fine - as far as I can see, it
looks like ohci.c/uhci.c, usbtrans.c, usbms.c, scsi.c are working well.
Could you check if something can be wrong in higher level for multi-LUN
devices ?

> > 2.
> > I found some other devices to test, now I have 13 USB MS bulk-only
> > devices, each is from another manufacturer and there are approx. five
> > different kinds of devices - flash disk, hard disk, card reader, camera,
> > mobile phone.
> > What is known but maybe interesting, three devices are not working at
> > all on UHCI - but are working on OHCI (and under Linux / Windows they
> > are also working) - as we discussed previously, it can be some problem
> > with power or EHCI influence because it looks like device is not
> > properly powered.
> > UHCI-problematic devices  are: Transcend SDHC card reader 
> I have a cardreader and it may be Transcend.

Yes, as we discussed before, it can be some power related problem on my
computer or something else... Maybe I try investigate it again.

> > 4.
> > One really stupid question related to your older e-mail:
> >
> > What does mean "For this you can add a new field in "o"." ?
> >
> > It is something related to Bazaar branch ? Or to something in source
> > code configuration etc. ?
> > (I am totally unexperienced with bazaar and similar tools, the same with
> > autogen and similar..., sorry.)
> >
> >   
> I meant this:
>   struct grub_ohci *o = (struct grub_ohci *) dev->data;
> 

Thanks :-), sometimes I don't see evident things...

One question at the end but not so important - I am thinking about some
"resarch" of EHCI - at least in possible relation between
UHCI-not-working devices and some EHCI power settings. So, if I will
have some time, I maybe try to do something with EHCI. 
But I am not familiar with atomake, autogen and similar tools - how to
add correctly new module "ehci" - i.e. file bus/usb/ehci.c into
makefile ?

Best regards
Ales

usb_fast_patch_100612
Description: Text Data

ohci.c
Description: Text Data