[LinuxBIOS] [PATCH] v3: 8254_pit.patch

Marc Jones marc.jones at amd.com
Wed Jul 4 02:19:48 CEST 2007


Updated the patch based on comments. Some comments inline below.

Uwe Hermann wrote:
> On Tue, Jul 03, 2007 at 10:53:23AM -0600, Marc Jones wrote:
>> Index: LinuxBIOSv3/arch/x86/geodelx/geodelx.c
>> ===================================================================
>> --- LinuxBIOSv3.orig/arch/x86/geodelx/geodelx.c	2007-07-02 10:23:52.000000000 -0600
>> +++ LinuxBIOSv3/arch/x86/geodelx/geodelx.c	2007-07-03 09:37:11.000000000 -0600
>> @@ -30,6 +30,7 @@
>>  #include <io.h>
>>  #include <amd_geodelx.h>
>>  #include <spd.h>
>> +#include <8254_pit.h>
>>  
>>  /* all these functions used to be in a lot of fiddly little files.  To
>>    * make it easier to find functions, we are merging them here. This
>> @@ -41,8 +42,6 @@
>>  
>>  /** 
>>    * Starts Timer 1 for port 61 use.
>> -  * 0x43 is PIT command/control.
>> -  * 0x41 is PIT counter 1.
>>    *
>>    * The command 0x56 means write counter 1 lower 8 bits in next IO,
>>    * set the counter mode to square wave generator (count down to 0
>> @@ -58,8 +57,8 @@
>>    */
>>  void start_timer1(void)
>>  {
>> -	outb(0x56, 0x43);
>> -	outb(0x12, 0x41);
>> +	outb(0x56, I82C54_CONTROL_WORD_REGISTER);
>> +	outb(0x12, I82C54_COUNTER1);
> 
> Yep, but the 0x56 and 0x12 should be #defines (or have comments), too.
> 
The 0x56 and 0x12 are well commented in the function header.

>>  }
>>  
>>  /** 
>> Index: LinuxBIOSv3/arch/x86/speaker.c
>> ===================================================================
>> --- LinuxBIOSv3.orig/arch/x86/speaker.c	2007-07-02 10:23:37.000000000 -0600
>> +++ LinuxBIOSv3/arch/x86/speaker.c	2007-07-03 09:34:08.000000000 -0600
>> @@ -2,6 +2,7 @@
>>   * This file is part of the LinuxBIOS project.
>>   *
>>   * Copyright (C) 2007 Uwe Hermann <uwe at hermann-uwe.de>
>> + * Copyright (C) 2007 Advanced Micro Devices, Inc.
> 
> Nope, please drop that this time. There's no new code, just shuffling of
> existing code in this patch.
> 
ok
> 
>>   *
>>   * 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
>> @@ -18,23 +19,9 @@
>>   * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
>>   */
>>  
>> -/*
>> - * Datasheet:
>> - *  - Name: 82C54 CHMOS Programmable Interval Timer
>> - *  - PDF: http://www.intel.com/design/archives/periphrl/docs/23124406.pdf
>> - *  - Order number: 231244-006
>> - */
> 
> Please let's keep this here _and_ in 8254_pit.h, it's useful in both
> files, IMO.
> 
ok

> 
>> -
>>  #include <io.h>
>>  #include <lib.h>
>> -
>> -#define I82C54_CONTROL_WORD_REGISTER	0x43	/* Write-only. */
>> -
>> -#define I82C54_COUNTER0			0x40
>> -#define I82C54_COUNTER1			0x41
>> -#define I82C54_COUNTER2			0x42
>> -
>> -#define PC_SPEAKER_PORT			0x61
> 
> The PC_SPEAKER_PORT belongs in this file, I think. It's not PIT related,
> but speaker related.
>
good point

> 
>> +#include <8254_pit.h>
>>  
>>  /**
>>   * Use the PC speaker to create a tone/sound of the specified frequency.
>> Index: LinuxBIOSv3/include/arch/x86/8254_pit.h
> 
> Hm, maybe we should make this include/arch/x86/legacy.h and stick other
> #defines in there, too. We should avoid too many small almost-empty
> files and there will be other "legacy" devices which need #defines.
> 
> 
I started on this path but since there wasn't a legacy.h i figured 
everything was getting it's own headers. There is a legacy.h now.

>> ===================================================================
>> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
>> +++ LinuxBIOSv3/include/arch/x86/8254_pit.h	2007-07-03 09:34:04.000000000 -0600
>> @@ -0,0 +1,36 @@
>> + * Copyright (C) 2007 Advanced Micro Devices, Inc.
> 
> See above, drop this please.
> 
ok
> 
> Otherwise patch is ok.
> 
> Uwe.
> 

Marc

-- 
Marc Jones
Senior Software Engineer
(970) 226-9684 Office
mailto:Marc.Jones at amd.com
http://www.amd.com/embeddedprocessors
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 8254_PIT.patch
URL: <http://www.coreboot.org/pipermail/coreboot/attachments/20070703/61bd8fc9/attachment.ksh>


More information about the coreboot mailing list