grub-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] PCI serial card support


From: Vesa Jääskeläinen
Subject: Re: [PATCH] PCI serial card support
Date: Thu, 13 Nov 2008 20:05:37 +0200
User-agent: Thunderbird 2.0.0.17 (Windows/20080914)

Hi Don,

Thanks for the patch!

address@hidden wrote:
> On Sun, Nov 09, 2008 at 10:57:30PM +0100, Robert Millan wrote:
>> On Sat, Nov 08, 2008 at 06:58:19PM -0700, address@hidden 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 PCI
>>> 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, device
>>> type mask) to a base baud and a configuration function.  The default
>>> configuration function does nothing so the table only provides the base
>>> baud.  We can add config functions for different cards as time goes by.
> 
> 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).
> 
> 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.
> 
> The command `serial' will print out the table, flagging the
> currently selected serial entry, e.g.:
> 
> 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
> 
> 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:
> 
> 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 
> Foundation, Inc.

This seems to be new file. Please change copyright years to start from 2008.

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 <grub/pci.h>
>  
> +void grub_serial_add(int type, unsigned int id, unsigned int base, unsigned 
> int port);
> +#include <grub/pci_serial_ids.h>
> +

Please keep includes at same place.

Why is this prototype even here?. And should it be static?

> +char *serial_types[] = {
> +     "legacy",
> +     "   pci",
> +     "   usb"
> +};

static const

And please leave output formatting to actual printing.

> +  grub_printf("%c%2d: %s ", (p == serial_dev) ? '*' : ' ',
> +                        i, serial_types[p->type]);

What is p->type is not within range of serial_types?

> +char parity[] = {

static const

> +static int
> +serial_pci_scan (int bus, int dev, int func, grub_pci_id_t pciid)

> +  vid = pciid & 0xffff;
> +  did = pciid >> 16;
> +  addr = grub_pci_make_address (bus, dev, func, 2);
> +  w = grub_pci_read (addr);
> +  class = (w >> 16) | ((w >> 8) & 0xff);
> +  addr = grub_pci_make_address (bus, dev, func, 11);
> +  w = grub_pci_read (addr);
> +  ss_vid = w & 0xffff;
> +  ss_did = 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ääskeläinen




reply via email to

[Prev in Thread] Current Thread [Next in Thread]