From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with archive (Exim 4.43) id 1L0gZV-0003LJ-ME for mharc-grub-devel@gnu.org; Thu, 13 Nov 2008 13:05:37 -0500 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1L0gZT-0003K8-Ea for grub-devel@gnu.org; Thu, 13 Nov 2008 13:05:35 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1L0gZS-0003J8-Hm for grub-devel@gnu.org; Thu, 13 Nov 2008 13:05:34 -0500 Received: from [199.232.76.173] (port=45444 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1L0gZR-0003It-Vi for grub-devel@gnu.org; Thu, 13 Nov 2008 13:05:34 -0500 Received: from mta-out.inet.fi ([195.156.147.13]:55989 helo=jenni1.inet.fi) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1L0gZR-00088W-5b for grub-devel@gnu.org; Thu, 13 Nov 2008 13:05:33 -0500 Received: from [127.0.0.1] (84.248.105.254) by jenni1.inet.fi (8.5.014) id 48FC59C7013B57A3 for grub-devel@gnu.org; Thu, 13 Nov 2008 20:05:32 +0200 Message-ID: <491C6C71.1090903@nic.fi> Date: Thu, 13 Nov 2008 20:05:37 +0200 From: =?ISO-8859-1?Q?Vesa_J=E4=E4skel=E4inen?= User-Agent: Thunderbird 2.0.0.17 (Windows/20080914) MIME-Version: 1.0 To: The development of GRUB 2 References: <20081106160641.GA26883@thorin> <20081107160725.GD17223@xolotl.n0ano.com> <49147245.6080105@nic.fi> <20081108112536.GA11583@thorin> <491584C4.5020407@nic.fi> <20081108124544.GA25958@thorin> <20081109015819.GA4022@xolotl.n0ano.com> <20081109215730.GA26330@thorin> <20081112185803.GI17223@xolotl.n0ano.com> In-Reply-To: <20081112185803.GI17223@xolotl.n0ano.com> X-Enigmail-Version: 0.95.7 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: Quoted-Printable X-detected-operating-system: by monty-python.gnu.org: GNU/Linux 2.6 (newer, 3) Subject: Re: [PATCH] PCI serial card support X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: The development of GRUB 2 List-Id: The development of GRUB 2 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 13 Nov 2008 18:05:35 -0000 Hi Don, Thanks for the patch! n0ano@n0ano.com wrote: > On Sun, Nov 09, 2008 at 10:57:30PM +0100, Robert Millan wrote: >> On Sat, Nov 08, 2008 at 06:58:19PM -0700, n0ano@n0ano.com wrote: >>> I think this is pretty much what I'm in the middle of doing. I want = to >>> put the infrastructure in place so that we can handle an arbitrary PC= I >>> device but I will only put the actual code in to handle the PCI card = that >>> I have (the only one I can test). >>> >>> What I'm doing is creating a table that matches the tuple (vendor id, >>> device id, subsystem vendor id, subsystem device id, device type, dev= ice >>> type mask) to a base baud and a configuration function. The default >>> configuration function does nothing so the table only provides the ba= se >>> baud. We can add config functions for different cards as time goes b= y. >=20 > Finally, here is the patch that adds an infrastructure to support > multiple serial devices (legacy & PCI, we can think about USB for > the future but that will be harder). >=20 > This patch creates a table with an entry for each serial device > in the system. It attempts to fill in appropriate defaults for > each entry. Right now it should determine the base baud for most > PCI devices but it can only find the I/O port for the Titan PCI > serial card that I have to test. >=20 > The command `serial' will print out the table, flagging the > currently selected serial entry, e.g.: >=20 > grub> serial > Available serial units: > * 0: legacy COM1 0x0000 9600/115200 8N1 > 1: legacy COM2 0x0000 9600/115200 8N1 > 2: legacy COM3 0x0000 9600/115200 8N1 > 3: legacy COM4 0x0000 9600/115200 8N1 > 4: pci 1:00.0 0xe880 9600/921600 8N1 >=20 > Note that unit 0 (the legacy COM1 port) is currently the selected > unit. Since my machine doesn't have any legacy serial devices > (note the 0x0000 for the I/O port) this device won't work. The > command `serial -u 4' will select the PCI device and, since all > the defaults are correct, that device will work. You can override > all defaults with the serial command so, to duplicate the defaults > for the PCI device, you would use the command: >=20 > serial -u 4 -p 0xe880 -s 9600 -b 921600 -w 8 -r n -t 1 Why not hide the legacy comports if they are not there and we can auto detect that? That actually brings us to other issue. Should be have some kind of HW dependant device path support that would be universal in grub?. Now if user unplugs in example USB serial converter then the order will change and can cause some problems. > --- include/grub/pci_serial_ids.h (revision 0) > +++ include/grub/pci_serial_ids.h (revision 0) > @@ -0,0 +1,642 @@ > +/* > + * GRUB -- GRand Unified Bootloader > + * Copyright (C) 2000,2001,2002,2003,2004,2005,2007 Free Software Fo= undation, Inc. This seems to be new file. Please change copyright years to start from 20= 08. Where did you get that device table?. Generated by yourself from pci database? Anyway. I think those vendor and device ID's should be in global file as they are "universal" to any PCI device. > --- term/i386/pc/serial.c (revision 1911) > +++ term/i386/pc/serial.c (working copy) > +#include > =20 > +void grub_serial_add(int type, unsigned int id, unsigned int base, uns= igned int port); > +#include > + Please keep includes at same place. Why is this prototype even here?. And should it be static? > +char *serial_types[] =3D { > + "legacy", > + " pci", > + " usb" > +}; static const And please leave output formatting to actual printing. > + grub_printf("%c%2d: %s ", (p =3D=3D serial_dev) ? '*' : ' ', > + i, serial_types[p->type]); What is p->type is not within range of serial_types? > +char parity[] =3D { static const > +static int > +serial_pci_scan (int bus, int dev, int func, grub_pci_id_t pciid) > + vid =3D pciid & 0xffff; > + did =3D pciid >> 16; > + addr =3D grub_pci_make_address (bus, dev, func, 2); > + w =3D grub_pci_read (addr); > + class =3D (w >> 16) | ((w >> 8) & 0xff); > + addr =3D grub_pci_make_address (bus, dev, func, 11); > + w =3D grub_pci_read (addr); > + ss_vid =3D w & 0xffff; > + ss_did =3D w >> 16; This smells. Perhaps helper function or macro. As a generic note. If this is generic information that is available on every PCI device. Why not change PCI to iterate with structure. Well... I didn't look too deep there this time. :) Thanks, Vesa J=E4=E4skel=E4inen