From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1R9f9V-0006lp-Nv for mharc-grub-devel@gnu.org; Fri, 30 Sep 2011 11:37:29 -0400 Received: from eggs.gnu.org ([140.186.70.92]:39038) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1R9f9R-0006jt-5V for grub-devel@gnu.org; Fri, 30 Sep 2011 11:37:27 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1R9f9P-00084v-IU for grub-devel@gnu.org; Fri, 30 Sep 2011 11:37:25 -0400 Received: from mail-fx0-f41.google.com ([209.85.161.41]:55447) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1R9f9P-00084j-8u for grub-devel@gnu.org; Fri, 30 Sep 2011 11:37:23 -0400 Received: by fxh17 with SMTP id 17so3695437fxh.0 for ; Fri, 30 Sep 2011 08:37:22 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=message-id:date:from:user-agent:mime-version:to:subject:references :in-reply-to:x-enigmail-version:content-type; bh=kldb70a2lhSG5s/I98urrMXE7Ks5CTigeBZ571E/gAQ=; b=p46gjzzgpiLiPc9ptmJHSVQlAEy/qhojGCWQEsks8dv8hV8GK4UzHSqvrKUrNbtl5G gAbKy3kHWuFka3suPe9++ccSspk5EQlXKz30q+0aZYhpTJxFLnj/ku7FJdAcLhiiUbg4 UHu5q1KSUwWyLOivo4eFIgIHyO6KkAlQGYq78= Received: by 10.223.53.146 with SMTP id m18mr18168018fag.67.1317397041854; Fri, 30 Sep 2011 08:37:21 -0700 (PDT) Received: from debian.x201.phnet (hg-public-dock-72-dhcp.ethz.ch. [82.130.80.72]) by mx.google.com with ESMTPS id c1sm7154289fab.15.2011.09.30.08.37.17 (version=TLSv1/SSLv3 cipher=OTHER); Fri, 30 Sep 2011 08:37:18 -0700 (PDT) Message-ID: <4E85E22C.2010804@gmail.com> Date: Fri, 30 Sep 2011 17:37:16 +0200 From: =?UTF-8?B?VmxhZGltaXIgJ8+GLWNvZGVyL3BoY29kZXInIFNlcmJpbmVua28=?= User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.21) Gecko/20110831 Iceowl/1.0b2 Icedove/3.1.13 MIME-Version: 1.0 To: The development of GNU GRUB Subject: Re: [PATCH] EHCI driver - USB 2.0 support References: <1309029199.2532.35.camel@pracovna> <201106252151.12483.szymon@janc.net.pl> <1309120668.3268.153.camel@pracovna> <4E28A149.4010008@gmail.com> <1314463338.2555.56.camel@pracovna> In-Reply-To: <1314463338.2555.56.camel@pracovna> X-Enigmail-Version: 1.1.2 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="------------enig674112D1B32D10B8E75F25F2" X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.6 (newer, 2) X-Received-From: 209.85.161.41 X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.14 Precedence: list Reply-To: The development of GNU GRUB List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 30 Sep 2011 15:37:27 -0000 This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enig674112D1B32D10B8E75F25F2 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable That must be a too long interval of writing e-mail. I started it a month ago, then interrupted and now finishing it > 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 els= e > 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 i= n > comparison with interrupt split transfers. Probably only one solution i= s > 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 *chu= nk) > { > 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 (chu= nk) > + grub_dma_get_phys (chunk)); Should be: return ((grub_uint8_t *) virt - (grub_uint8_t *) grub_dma_get_virt (chu= nk)) + 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 !=3D 0x0c || subclass !=3D 0x03 || interf !=3D 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 =3D grub_pci_make_address (dev, GRUB_EHCI_PCI_SBRN_REG); > release =3D grub_pci_read_byte (addr); > if (release !=3D 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 =3D grub_pci_make_address (dev, GRUB_PCI_REG_ADDRESS_REG0); > base =3D grub_pci_read (addr); > addr =3D grub_pci_make_address (dev, GRUB_PCI_REG_ADDRESS_REG1); > base_h =3D 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) !=3D GRUB_PCI_ADDR_MEM_TYPE= _32) > && (base_h !=3D 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 =3D grub_zalloc (sizeof (*e)); > if (!e) > return 1; > e->framelist_chunk =3D NULL; > e->td_chunk =3D NULL; > e->qh_chunk =3D NULL; > e->iobase_ehcc =3D (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 =3D (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 =3D (volatile grub_uint32_t *) ((grub_uint8_t *) e->iobase_eh= cc + 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 =3D 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 =3D 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. --=20 Regards Vladimir '=CF=86-coder/phcoder' Serbinenko --------------enig674112D1B32D10B8E75F25F2 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iF4EAREKAAYFAk6F4iwACgkQNak7dOguQgkC7QD/aJIVngtKxjR2ngdYWpNHTk/3 VJ1EHf9jH+jkPItTA0YA/2Tz01xNH481ao+/QmUa6naF1f+UWwHBJhde7FzQH7yi =9eJ+ -----END PGP SIGNATURE----- --------------enig674112D1B32D10B8E75F25F2--