Patchwork warning days - m57sli/mcp55

login
register
about
Submitter Ward Vandewege
Date 2010-04-14 21:09:37
Message ID <20100414210937.GA8019@countzero.vandewege.net>
Download mbox | patch
Permalink /patch/1237/
State Accepted, archived
Headers show

Comments

Ward Vandewege - 2010-04-14 21:09:37
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;i<0x8000;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.
Myles Watson - 2010-04-14 21:28:36
> > 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;i<0x8000;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 <mylesgw@gmail.com>

Thanks,
Myles

Patch


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 <ward@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;i<0x8000;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>
 #include <stdlib.h>
+#include "fanctl.h"
 
 static void write_index(uint16_t port_base, uint8_t reg, uint8_t value)
 {
Index: src/mainboard/gigabyte/m57sli/fanctl.h
===================================================================
--- src/mainboard/gigabyte/m57sli/fanctl.h	(revision 0)
+++ src/mainboard/gigabyte/m57sli/fanctl.h	(revision 0)
@@ -0,0 +1,3 @@ 
+
+void init_ec(uint16_t base);
+
Index: src/mainboard/gigabyte/m57sli/romstage.c
===================================================================
--- src/mainboard/gigabyte/m57sli/romstage.c	(revision 5437)
+++ 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 5437)
+++ src/northbridge/amd/amdk8/exit_from_self.c	(working copy)
@@ -17,7 +17,7 @@ 
  * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
  */
 
-void exit_from_self(int controllers, const struct mem_controller *ctrl,
+static void exit_from_self(int controllers, const struct mem_controller *ctrl,
 		    struct sys_info *sysinfo)
 {
 	int i;