Re: [coreboot] [PATCH] warning days - m57sli/mcp55

2010-04-14 Thread Ward Vandewege 
On Tue, Apr 13, 2010 at 12:15:42AM +0200, Stefan Reinauer wrote:
  Index: src/southbridge/nvidia/mcp55/mcp55_early_setup_car.c
  ===
  --- src/southbridge/nvidia/mcp55/mcp55_early_setup_car.c(revision 5411)
  +++ src/southbridge/nvidia/mcp55/mcp55_early_setup_car.c(working copy)
  @@ -177,7 +177,7 @@
  pci_write_config32(dev, 0xe4, dword);
   
   // need to wait 100ms
  -   delayx(1000);
  +   delayx(232);
   }

 it sounds a lot to do 0x8000 outb to wait 100us, but who knows...
 I think it would be better to change the input type to something else
 than uint8_t, supposedly unsigned as most other udelay functions.

That works, boot-tested.

 Alternatively you could try if this works:
 
 Index: src/southbridge/nvidia/mcp55/mcp55_early_setup_car.c
 ===
 --- src/southbridge/nvidia/mcp55/mcp55_early_setup_car.c (revision 5411)
 +++ src/southbridge/nvidia/mcp55/mcp55_early_setup_car.c (working copy)
 @@ -131,15 +131,9 @@
 
 
 }
 -static void delayx(uint8_t value) {
 -#if 1
 - int i;
 - for(i=0;i0x8000;i++) {
 - outb(value, 0x80);
 - }
 -#endif
 -}
 
 +#include pc80/udelay_io.c
 +
 static void mcp55_early_pcie_setup(unsigned busnx, unsigned devnx,
 unsigned anactrl_io_base, unsigned pci_e_x)
 {
 uint32_t tgio_ctrl;
 @@ -170,14 +164,14 @@
 outl(tgio_ctrl, anactrl_io_base + 0xcc);
 
 // wait 100us
 - delayx(1);
 + udelay(100);
 
 dword = pci_read_config32(dev, 0xe4);
 dword = ~(0x3f0); // enable
 pci_write_config32(dev, 0xe4, dword);
 
 // need to wait 100ms
 - delayx(1000);
 + udelay(100 * 1000);
 }
 
 static void mcp55_early_setup(unsigned mcp55_num, unsigned *busn,
 unsigned *devn, unsigned *io_base, unsigned *pci_e_x)

Hmm, that generates a conflict: 

In file included from
src/southbridge/nvidia/mcp55/mcp55_early_setup_car.c:143,
 from src/mainboard/gigabyte/m57sli/romstage.c:133:
src/pc80/udelay_io.c:4: error: redefinition of 'udelay'
src/cpu/amd/model_fxx/apic_timer.c:19: note: previous definition of 'udelay'
was here

We do indeed have 2 different functions called udelay.

Ideas?

  Index: src/mainboard/gigabyte/m57sli/fanctl.c
  ===
  --- src/mainboard/gigabyte/m57sli/fanctl.c  (revision 5411)
  +++ src/mainboard/gigabyte/m57sli/fanctl.c  (working copy)
  @@ -71,6 +71,7 @@
   /*
* Called from superio.c
*/
  +extern void init_ec(uint16_t base);
   void init_ec(uint16_t base)
   {
  int i;

 
 init_ec() is the API between the superio drivers and the mainboard
 drivers...
 
 If this is a single hack, it's fine as it is.. If we're going to have an
 API here, we should create a src/include/superio.h or some such

It's only used on this particular board. Myles expressed a preference for a
header file, so I moved the definition to fanctl.h, and I dropped the
'extern' as you suggested.

  Index: src/northbridge/amd/amdk8/exit_from_self.c
  ===
  --- src/northbridge/amd/amdk8/exit_from_self.c  (revision 5411)
  +++ src/northbridge/amd/amdk8/exit_from_self.c  (working copy)
  @@ -17,6 +17,8 @@
* Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 
  USA
*/
   
  +extern void exit_from_self(int controllers, const struct mem_controller 
  *ctrl,
  +struct sys_info *sysinfo);
   void exit_from_self(int controllers, const struct mem_controller *ctrl,
  struct sys_info *sysinfo)
   {

 since this is a C file that is included in exactly one file,
 raminit_f.c you could as well just mark the function static.

Done.

 btw, for function prototypes the extern in not really needed. I keep
 removing them from the tree, but if people think we should have them,
 I'll try to be consistent and stop deleting them :-)

I'll not add any new ones then!

Updated patch attached. I see the ACPI warnings were already fixed in another
commit.

Thanks,
Ward.

-- 
Ward Vandewege w...@fsf.org
Free Software Foundation - Senior Systems Administrator

This fixes a number of warnings when building m57sli (and other boards with mcp55).

This patch is boot tested on m57sli.

Signed-off-by: Ward Vandewege w...@gnu.org

Index: src/southbridge/nvidia/mcp55/mcp55_early_setup_car.c
===
--- src/southbridge/nvidia/mcp55/mcp55_early_setup_car.c	(revision 5437)
+++ src/southbridge/nvidia/mcp55/mcp55_early_setup_car.c	(working copy)
@@ -131,7 +131,7 @@
 
 
 }
-static void delayx(uint8_t value) {
+static void delayx(unsigned value) {
 #if 1
 	int i;
 	for(i=0;i0x8000;i++) {
Index: src/mainboard/gigabyte/m57sli/fanctl.c
===
--- src/mainboard/gigabyte/m57sli/fanctl.c	(revision 5437)
+++ src/mainboard/gigabyte/m57sli/fanctl.c	(working copy)
@@ -1,5 +1,6 @@
 #include arch/io.h

Re: [coreboot] [PATCH] warning days - m57sli/mcp55

2010-04-14 Thread Myles Watson 
  Alternatively you could try if this works:
 
  Index: src/southbridge/nvidia/mcp55/mcp55_early_setup_car.c
  ===
  --- src/southbridge/nvidia/mcp55/mcp55_early_setup_car.c (revision 5411)
  +++ src/southbridge/nvidia/mcp55/mcp55_early_setup_car.c (working copy)
  @@ -131,15 +131,9 @@
 
 
  }
  -static void delayx(uint8_t value) {
  -#if 1
  - int i;
  - for(i=0;i0x8000;i++) {
  - outb(value, 0x80);
  - }
  -#endif
  -}
 
  +#include pc80/udelay_io.c
  +
  static void mcp55_early_pcie_setup(unsigned busnx, unsigned devnx,
  unsigned anactrl_io_base, unsigned pci_e_x)
  {
  uint32_t tgio_ctrl;
  @@ -170,14 +164,14 @@
  outl(tgio_ctrl, anactrl_io_base + 0xcc);
 
  // wait 100us
  - delayx(1);
  + udelay(100);
 
  dword = pci_read_config32(dev, 0xe4);
  dword = ~(0x3f0); // enable
  pci_write_config32(dev, 0xe4, dword);
 
  // need to wait 100ms
  - delayx(1000);
  + udelay(100 * 1000);
  }
 
  static void mcp55_early_setup(unsigned mcp55_num, unsigned *busn,
  unsigned *devn, unsigned *io_base, unsigned *pci_e_x)

 Hmm, that generates a conflict:

 In file included from
 src/southbridge/nvidia/mcp55/mcp55_early_setup_car.c:143,
 from src/mainboard/gigabyte/m57sli/romstage.c:133:
 src/pc80/udelay_io.c:4: error: redefinition of 'udelay'
 src/cpu/amd/model_fxx/apic_timer.c:19: note: previous definition of
 'udelay'
 was here

 We do indeed have 2 different functions called udelay.

 Ideas?

You could just call udelay without including pc80/udelay.c.




   Index: src/mainboard/gigabyte/m57sli/fanctl.c
   ===
   --- src/mainboard/gigabyte/m57sli/fanctl.c  (revision 5411)
   +++ src/mainboard/gigabyte/m57sli/fanctl.c  (working copy)
   @@ -71,6 +71,7 @@
/*
 * Called from superio.c
 */
   +extern void init_ec(uint16_t base);
void init_ec(uint16_t base)
{
   int i;
  
 
  init_ec() is the API between the superio drivers and the mainboard
  drivers...
 
  If this is a single hack, it's fine as it is.. If we're going to have an
  API here, we should create a src/include/superio.h or some such

 It's only used on this particular board.


Isn't there another init_ec function in the tree?  Could they use the same
header file?

Acked-by: Myles Watson myle...@gmail.com

Thanks,
Myles
-- 
coreboot mailing list: coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot

[coreboot] [PATCH] warning days - m57sli/mcp55

2010-04-12 Thread Ward Vandewege 
If there are better ways to kill the warnings, please let me know!

Thanks,
Ward.

-- 
Ward Vandewege w...@fsf.org
Free Software Foundation - Senior Systems Administrator

This fixes a number of warnings when building m57sli (and other boards with mcp55).

This patch is boot tested on m57sli.

What appears to be a shortening of the delay in

  src/southbridge/nvidia/mcp55/mcp55_early_setup_car.c

is functionally exactly the same as before; the delayx function takes a uint8_t
argument, so the old value (1000 - 0x3E8) was truncated to 232 (0xE8). 

Signed-off-by: Ward Vandewege w...@gnu.org

Index: src/southbridge/nvidia/mcp55/mcp55_fadt.c
===
--- src/southbridge/nvidia/mcp55/mcp55_fadt.c	(revision 5411)
+++ src/southbridge/nvidia/mcp55/mcp55_fadt.c	(working copy)
@@ -51,8 +51,8 @@
 
 	printk(BIOS_INFO, ACPI: pm_base: %u...\n, pm_base);
 
-	fadt-firmware_ctrl = facs;
-	fadt-dsdt = dsdt;
+	fadt-firmware_ctrl = (u32) facs;
+	fadt-dsdt = (u32) dsdt;
 	fadt-preferred_pm_profile = 1; //check
 	fadt-sci_int = 9;
 	/* disable system management mode by setting to 0 */
@@ -108,9 +108,9 @@
 	fadt-reset_reg.addrh = 0x0;
 
 	fadt-reset_value = 0;
-	fadt-x_firmware_ctl_l = facs;
+	fadt-x_firmware_ctl_l = (u32) facs;
 	fadt-x_firmware_ctl_h = 0;
-	fadt-x_dsdt_l = dsdt;
+	fadt-x_dsdt_l = (u32) dsdt;
 	fadt-x_dsdt_h = 0;
 
 	fadt-x_pm1a_evt_blk.space_id = 1;
Index: src/southbridge/nvidia/mcp55/mcp55_early_setup_car.c
===
--- src/southbridge/nvidia/mcp55/mcp55_early_setup_car.c	(revision 5411)
+++ src/southbridge/nvidia/mcp55/mcp55_early_setup_car.c	(working copy)
@@ -177,7 +177,7 @@
 	pci_write_config32(dev, 0xe4, dword);
 
 //	need to wait 100ms
-	delayx(1000);
+	delayx(232);
 }
 
 static void mcp55_early_setup(unsigned mcp55_num, unsigned *busn, unsigned *devn, unsigned *io_base, unsigned *pci_e_x)
@@ -388,7 +388,7 @@
 	int mcp55_num = 0;
 	unsigned busnx;
 	unsigned devnx;
-	int ht_c_index,j;
+	int ht_c_index;
 
 	/* FIXME: multi pci segment handling */
 
Index: src/mainboard/gigabyte/m57sli/fanctl.c
===
--- src/mainboard/gigabyte/m57sli/fanctl.c	(revision 5411)
+++ src/mainboard/gigabyte/m57sli/fanctl.c	(working copy)
@@ -71,6 +71,7 @@
 /*
  * Called from superio.c
  */
+extern void init_ec(uint16_t base);
 void init_ec(uint16_t base)
 {
 	int i;
Index: src/mainboard/gigabyte/m57sli/romstage.c
===
--- src/mainboard/gigabyte/m57sli/romstage.c	(revision 5411)
+++ src/mainboard/gigabyte/m57sli/romstage.c	(working copy)
@@ -146,7 +146,6 @@
 static void sio_setup(void)
 {
 
-unsigned value;
 uint32_t dword;
 uint8_t byte;
 
Index: src/northbridge/amd/amdk8/exit_from_self.c
===
--- src/northbridge/amd/amdk8/exit_from_self.c	(revision 5411)
+++ src/northbridge/amd/amdk8/exit_from_self.c	(working copy)
@@ -17,6 +17,8 @@
  * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
  */
 
+extern void exit_from_self(int controllers, const struct mem_controller *ctrl,
+struct sys_info *sysinfo);
 void exit_from_self(int controllers, const struct mem_controller *ctrl,
 		struct sys_info *sysinfo)
 {
-- 
coreboot mailing list: coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot

Re: [coreboot] [PATCH] warning days - m57sli/mcp55

2010-04-12 Thread Stefan Reinauer 


 This fixes a number of warnings when building m57sli (and other boards with 
 mcp55).

 This patch is boot tested on m57sli.

 What appears to be a shortening of the delay in

   src/southbridge/nvidia/mcp55/mcp55_early_setup_car.c

 is functionally exactly the same as before; the delayx function takes a 
 uint8_t
 argument, so the old value (1000 - 0x3E8) was truncated to 232 (0xE8). 

 Signed-off-by: Ward Vandewege w...@gnu.org

   

  
 Index: src/southbridge/nvidia/mcp55/mcp55_early_setup_car.c
 ===
 --- src/southbridge/nvidia/mcp55/mcp55_early_setup_car.c  (revision 5411)
 +++ src/southbridge/nvidia/mcp55/mcp55_early_setup_car.c  (working copy)
 @@ -177,7 +177,7 @@
   pci_write_config32(dev, 0xe4, dword);
  
  //   need to wait 100ms
 - delayx(1000);
 + delayx(232);
  }
   
it sounds a lot to do 0x8000 outb to wait 100us, but who knows...
I think it would be better to change the input type to something else
than uint8_t, supposedly unsigned as most other udelay functions.

Alternatively you could try if this works:

Index: src/southbridge/nvidia/mcp55/mcp55_early_setup_car.c
===
--- src/southbridge/nvidia/mcp55/mcp55_early_setup_car.c (revision 5411)
+++ src/southbridge/nvidia/mcp55/mcp55_early_setup_car.c (working copy)
@@ -131,15 +131,9 @@


}
-static void delayx(uint8_t value) {
-#if 1
- int i;
- for(i=0;i0x8000;i++) {
- outb(value, 0x80);
- }
-#endif
-}

+#include pc80/udelay_io.c
+
static void mcp55_early_pcie_setup(unsigned busnx, unsigned devnx,
unsigned anactrl_io_base, unsigned pci_e_x)
{
uint32_t tgio_ctrl;
@@ -170,14 +164,14 @@
outl(tgio_ctrl, anactrl_io_base + 0xcc);

// wait 100us
- delayx(1);
+ udelay(100);

dword = pci_read_config32(dev, 0xe4);
dword = ~(0x3f0); // enable
pci_write_config32(dev, 0xe4, dword);

// need to wait 100ms
- delayx(1000);
+ udelay(100 * 1000);
}

static void mcp55_early_setup(unsigned mcp55_num, unsigned *busn,
unsigned *devn, unsigned *io_base, unsigned *pci_e_x)





  
 Index: src/mainboard/gigabyte/m57sli/fanctl.c
 ===
 --- src/mainboard/gigabyte/m57sli/fanctl.c(revision 5411)
 +++ src/mainboard/gigabyte/m57sli/fanctl.c(working copy)
 @@ -71,6 +71,7 @@
  /*
   * Called from superio.c
   */
 +extern void init_ec(uint16_t base);
  void init_ec(uint16_t base)
  {
   int i;
   

init_ec() is the API between the superio drivers and the mainboard
drivers...

If this is a single hack, it's fine as it is.. If we're going to have an
API here, we should create a src/include/superio.h or some such

  
 Index: src/northbridge/amd/amdk8/exit_from_self.c
 ===
 --- src/northbridge/amd/amdk8/exit_from_self.c(revision 5411)
 +++ src/northbridge/amd/amdk8/exit_from_self.c(working copy)
 @@ -17,6 +17,8 @@
   * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
   */
  
 +extern void exit_from_self(int controllers, const struct mem_controller 
 *ctrl,
 +struct sys_info *sysinfo);
  void exit_from_self(int controllers, const struct mem_controller *ctrl,
   struct sys_info *sysinfo)
  {
   
since this is a C file that is included in exactly one file,
raminit_f.c you could as well just mark the function static.

btw, for function prototypes the extern in not really needed. I keep
removing them from the tree, but if people think we should have them,
I'll try to be consistent and stop deleting them :-)

Stefan

Acked-by: Stefan Reinauer ste...@coresystems.de

-- 
coresystems GmbH • Brahmsstr. 16 • D-79104 Freiburg i. Br.
  Tel.: +49 761 7668825 • Fax: +49 761 7664613
Email: i...@coresystems.de  • http://www.coresystems.de/
Registergericht: Amtsgericht Freiburg • HRB 7656
Geschäftsführer: Stefan Reinauer • Ust-IdNr.: DE245674866


-- 
coreboot mailing list: coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot

Re: [coreboot] [PATCH] warning days - m57sli/mcp55

2010-04-12 Thread Myles Watson 

 If there are better ways to kill the warnings, please let me know!

 Index: src/mainboard/gigabyte/m57sli/fanctl.c
 ===
 --- src/mainboard/gigabyte/m57sli/fanctl.c(revision 5411)
 +++ src/mainboard/gigabyte/m57sli/fanctl.c(working copy)
 @@ -71,6 +71,7 @@
  /*
   * Called from superio.c
   */
 +extern void init_ec(uint16_t base);
  void init_ec(uint16_t base)
  {
   int i;

I would ack the patch except for the added extern statements.  I thought
extern was for functions and variables that are declared in another file.

I've been fixing the no previous prototype warnings in two ways:
1. declare the function static if it is only used in that file
2. Add a prototype in a header that is included in the file where the
function is defined and the file where it's used.

Thanks,
Myles


-- 
coreboot mailing list: coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot