Re: [LinuxBIOS] [PATCH] Add initial serial output for Via vt82c686

[Uwe Hermann]

Hi,

On Fri, Mar 09, 2007 at 10:44:21PM -0500, Corey Osgood wrote:
> This patch is just for initial serial output for the Via vt82c686(a/b) 
> southbridge, no more and no less. SMBus and the rest of the SuperI/O 
> functions are yet to be written. I've confirmed that this works on Tyan 
> S2507 (vt686b), and it should also work fine on vt686a. This is based 
> rather loosely on Uwe's code for the SMSC FDC37M60x.
>
> Signed-off-by: Corey Osgood

Nice, thanks.

Maybe I can test this in a few days, I currently don't have access to my
mainboards... A code quick review below:
> Index: src/southbridge/via/vt686/Config.lb
>
> --- src/southbridge/via/vt686/Config.lb	(revision 0)
> +++ src/southbridge/via/vt686/Config.lb	(revision 0)

Not sure. I think we usually use the full part names, so this should
probably be src/southbridge/via/vt82686b/?

How different are vt82686b and vt82686a? Should they become two
different directories? If not, how should the directory be called?
vt82686ab, or just vt82686a and we add a comment that it applies to
the vt82686b as well?

Also, shouldn't all of this go into src/superio/via? AFAIK the
southbridge has a built-in Super I/O, but do we still want to separate
the Super I/O parts and put them in src/superio/via?

Comments anyone?
> @@ -0,0 +1,23 @@
> +##
> +## This file is part of the LinuxBIOS project.
> +##
> +## Copyright (C) 2006 Uwe Hermann

Drop my line, add yourself. This is just a trivial two-line file...
> +##
> +## This program is free software; you can redistribute it and/or modify
> +## it under the terms of the GNU General Public License as published by
> +## the Free Software Foundation; either version 2 of the License, or
> +## (at your option) any later version.
> +##
> +## This program is distributed in the hope that it will be useful,
> +## but WITHOUT ANY WARRANTY; without even the implied warranty of
> +## MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +## GNU General Public License for more details.
> +##
> +## You should have received a copy of the GNU General Public License
> +## along with this program; if not, write to the Free Software
> +## Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
> +##
>
> +config chip.h
> +object superio.o
> Index: src/southbridge/via/vt686/vt686.h
>
> --- src/southbridge/via/vt686/vt686.h	(revision 0)
> +++ src/southbridge/via/vt686/vt686.h	(revision 0)
> @@ -0,0 +1,47 @@
> +/*
> This file is part of the LinuxBIOS project.
> Copyright (C) 2007 Corey Osgood
> This program is free software; you can redistribute it and/or modify
> it under the terms of the GNU General Public License as published by
> the Free Software Foundation; either version 2 of the License, or
> (at your option) any later version.
> This program is distributed in the hope that it will be useful,
> but WITHOUT ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> GNU General Public License for more details.
> You should have received a copy of the GNU General Public License
> along with this program; if not, write to the Free Software
> Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
> */
>
> +/* Datasheets:
> VT82C686B: http://www.datasheet4u.com/html/V/T/8/VT82C686B_VIA.pdf.html */

Please also add the exact name and order number (if available) of the datasheet.
See src/northbridge/intel/i440bx/i440bx.h for an example.
> +/* SuperI/0 Configuration Registers */
> +/* Any registers not listed here are for floppy control or are reserved */
> +#define VT686_CFG_PORT	0x3f0

All variables should probably be also called VT82C686A_FOO, etc...
> +#define	VT686_FS	0xE2 /* Function Select */
> +#define	VT686_PP_BA	0xE6 /* Parallel Port Base Addr. (def = 378-F) */
> +#define	VT686_COM1_BA	0xE7 /* Serial Port 1 Base Addr. (def =	3F8-F) */
> +#define	VT686_COM2_BA	0xE8 /* Serial Port 2 Base Addr. (def =	2F8-F) */
> +#define	VT686_COM_CFG	0xEE /* Serial Port Configuration */
> +#define	VT686_POWER	0xEF /* Power Down Control */
> +#define	VT686_PPCTRL	0xF0 /* Parallel Port Control */
> +#define	VT686_SPCTRL	0xF1 /* Serial Port Control */
> +#define	VT686_GP_IO	0xFC /* General Purpose I/O */

GP_IO -> GPIO
> +/* For reference, used PCI IDs and their names in pci_ids.h */
> +/*
> +PCI_VENDOR_ID_VIA		0x1106
> +PCI_DEVICE_ID_VIA_82C686	0x0686 //Function 0, PCI Config
> +PCI_DEVICE_ID_VIA_82C586_1	0x0571 //Function 1, IDE Controller
> +PCI_DEVICE_ID_VIA_82C586_2	0x3038 //Functions 2 & 3, USB Ports 0-1 & 2-3 Config
> +PCI_DEVICE_ID_VIA_82C586_3	0x3040 //Possible 2nd USB Controller? Check lspci, vt686 datasheet doesn't reference
> +PCI_DEVICE_ID_VIA_82C686_4	0x3057 //Function 4, Power Management
> +PCI_DEVICE_ID_VIA_82C686_5	0x3058 //Function 5 AC'97 Codec
> +PCI_DEVICE_ID_VIA_82C686_6	0x3068 //Function 6 MC'97 Codec */
> Index: src/southbridge/via/vt686/vt686_early_serial.c
> ===================================================================
> --- src/southbridge/via/vt686/vt686_early_serial.c	(revision 0)
> +++ src/southbridge/via/vt686/vt686_early_serial.c	(revision 0)
> @@ -0,0 +1,84 @@
> +/*
> This file is part of the LinuxBIOS project.
> Copyright (C) 2006 Uwe Hermann
> Copyright (C) 2007 Corey Osgood
> This program is free software; you can redistribute it and/or modify
> it under the terms of the GNU General Public License as published by
> the Free Software Foundation; either version 2 of the License, or
> (at your option) any later version.
> This program is distributed in the hope that it will be useful,
> but WITHOUT ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> GNU General Public License for more details.
> You should have received a copy of the GNU General Public License
> along with this program; if not, write to the Free Software
> Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
> */
>
> +/* This has been ported to the Via vt82c686(a/b) from SMSC FDC37M60x by Corey Osgood */
> +/* See vt686.h FMI */

For more information? Write the full text, no need to obfuscate.
> +#include <arch/romcc_io.h>
> +#include <device/pci_ids.h>
> +#include "vt686.h"
>
> +#define SIO_BASE			VT686_CFG_PORT
> +#define SIO_INDEX			SIO_BASE
> +#define SIO_DATA			SIO_BASE+1
>
> +static void vt686_sio_write( uint8_t index, uint8_t value )
> +{
> /* 2) Configure the chip */
> /*   a) Write index to port 3F0 */

0x3f0
> outb(index, SIO_BASE);
> /*   b) Read / write data from / to port 3F1 */

0x3f1

This only writes, drop the comment about reads.
> outb(value, SIO_DATA);
> /*   c) Repeat a and b for all desired registers */

You can drop this comment.
> +}
>
> +/* Enable the peripheral devices on the VT686 Super I/O chip. */
> +static void vt686_enable_serial( void )

Please use the Linux kernel coding style, see
http://linuxbios.org/Development_Guidelines#Coding_Style

You can use the 'indent' tool as a helper (but don't fully trust it)...
> +{
> /* (1) Enter the configuration state. ( Set PCI Function 0 Rx55[1] = 1 ) */
> device_t dev;
> /* First, find the southbridge. Trip the POST card if we don't (does die() do this?)*/

See src/console/console.c, it sets the POST value to 0xff.
> dev = pci_locate_device( PCI_ID( PCI_VENDOR_ID_VIA, PCI_DEVICE_ID_VIA_82C686 ), 0 );
> if ( dev == PCI_DEV_INVALID ) {
> outb( 0xf7, 0x80 );

Rather use post_code(0xf7) here...
> die( "Southbridge not found\r\n" ); //Somewhat pointless

Not pointless, die() halts the CPU and you probably don't want to
continue if you don't find the southbridge (?)
> }
> pci_write_config8( dev, 0x85, 0x1f ); //Enable the SuperI/O, SI/O Config, and disable everything else
> /* 2) Configure the chip */
> vt686_sio_write( VT686_POWER, 0x00 ); //Make sure all devs are powered on
> vt686_sio_write( VT686_COM_CFG, 0x00 ); //Set up COMs for normal (non-IR) operation
> vt686_sio_write( VT686_FS, 0x07 ); //Disable Floppy, Enable COM1, Disable COM2 and Parallel Port

This should be somehow configurable. Some people might want to use COM2
(can be configured via some Config.lb)...
> /* Index E2 (aka VT686_FS) ??? Super-I/O Function Select
> 7-5 Reserved (Reads 0)
> 4 Floppy Controller Enable
> 0 Disable (default)
> 1 Enable
> 3 Serial Port 2
> 1 Enable
> 0 Disable (default)
> 2 Serial Port 1
> 0 Disable (default)
> 1 Enable
> 1-0 Parallel Port Mode / Enable
> 00 Unidirectional mode
> 01 ECP
> 10 EPP
> 11 Disable (default) */
> vt686_sio_write( VT686_COM1_BA, 0xfe ); //Set COM1 Base Addr to doc default (3F8)

3F8 -> 0x3F8 (or better 0x3f8, I like lower-case hex numbers, personally).
> vt686_sio_write( VT686_COM_CFG, 0x40 ); //Enable High-Speed mode for COM1
> /* 3) Exit Configuration mode */
> pci_write_config8( dev, 0x85, 0x0d ); //Now set SI/O Config off, enable USB 2/3
>
> +}
> Index: src/southbridge/via/vt686/chip.h
> ===================================================================
> --- src/southbridge/via/vt686/chip.h	(revision 0)
> +++ src/southbridge/via/vt686/chip.h	(revision 0)
> @@ -0,0 +1,37 @@
> +/*
> This file is part of the LinuxBIOS project.
> Copyright (C) 2006 Uwe Hermann
> Copyright (C) 2007 Corey Osgood

Drop my line, this is just some trivial struct declarations...
> This program is free software; you can redistribute it and/or modify
> it under the terms of the GNU General Public License as published by
> the Free Software Foundation; either version 2 of the License, or
> (at your option) any later version.
> This program is distributed in the hope that it will be useful,
> but WITHOUT ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> GNU General Public License for more details.
> You should have received a copy of the GNU General Public License
> along with this program; if not, write to the Free Software
> Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
> */
>
> +#ifndef _SUPERIO_VIA_VT686
> +#define _SUPERIO_VIA_VT686
>
> +#include <device/device.h>
> +#include <pc80/keyboard.h>
> +#include <uart8250.h>
>
> +extern struct chip_operations superio_via_vt686_ops;
>
> +struct superio_via_vt686_config {
> struct uart8250 com1, com2;
> struct pc_keyboard keyboard;
>
> +};
>
> +#endif /* _SUPERIO_VIA_VT686 */

HTH, Uwe.