Fix superio enable calls

Submitter	Rudolf Marek
Date	2011-01-25 20:59:01
Message ID	<4D3F3995.40206@assembler.cz>
Download	mbox \| patch
Permalink	/patch/2559/
State	Accepted
Commit	r6373
Headers	show Return-path: <coreboot-bounces+patchwork=coreboot.org@coreboot.org> Envelope-to: patchwork@coreboot.org sender: r.marek@assembler.cz) by ms.trustica.cz (Postfix) with ESMTPSA id 4903E33A61A for <coreboot@coreboot.org>; Tue, 25 Jan 2011 21:59:01 +0100 (CET) Message-ID: <4D3F3995.40206@assembler.cz> Date: Tue, 25 Jan 2011 21:59:01 +0100 From: Rudolf Marek <r.marek@assembler.cz> User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.13) Gecko/20101208 Thunderbird/3.1.7 MIME-Version: 1.0 To: Coreboot <coreboot@coreboot.org> Content-Type: multipart/mixed; boundary="------------020400060301090501020107" Subject: [coreboot] [PATCH] Fix superio enable calls Precedence: list Sender: coreboot-bounces+patchwork=coreboot.org@coreboot.org Errors-To: coreboot-bounces+patchwork=coreboot.org@coreboot.org

Comments

Rudolf Marek - 2011-01-25 20:59:01

Hello,

While hunting yet another bug, I noticed that part of my serial log is missing. 
Especially I missed the Allocating & Setting resources.

It turns out that the code which enables specific LDN is somewhat buggy:



  static void w83627dhg_pnp_enable(device_t dev)
  {
-	if (!dev->enabled)
-		return;
-
  	pnp_enter_ext_func_mode(dev);
  	pnp_set_logical_device(dev);
-	pnp_set_enable(dev, 0);
  	pnp_exit_ext_func_mode(dev);
  }


So instead of enable the device the device gets disabled. However after some 
time the serial line gets back, most likely some "enable resources" might fix it.

I'm attaching patch which somewhat fixes the problem and changes the function to 
look same in all superio code. Some boards even did not convert the dev->enabled 
to 0,1 values.

Also makes me wonder some sio call pnp_enable directly, which has yet another 
semantics, it wont enable already enabled devices just disable those which 
declared not enabled. And no PnP enter conf magic is called (maybe this is the 
reason).

Signed-off-by: Rudolf Marek <r.marek@assembler.cz>

Not abuild tested, works on dhg. I need to know if this fix is OK this way.

Thanks,
Rudolf

Rudolf Marek - 2011-02-12 17:31:29

Hi all,

Can please someone take a look into this?
Thanks,

Rudolf

Peter Stuge - 2011-02-12 17:35:07

Rudolf Marek wrote:
> Hello,
>
> While hunting yet another bug, I noticed that part of my serial log is 
> missing. Especially I missed the Allocating & Setting resources.
>
> It turns out that the code which enables specific LDN is somewhat buggy:
>
>
>
>  static void w83627dhg_pnp_enable(device_t dev)
>  {
> -	if (!dev->enabled)
> -		return;
> -
>  	pnp_enter_ext_func_mode(dev);
>  	pnp_set_logical_device(dev);
> -	pnp_set_enable(dev, 0);
>  	pnp_exit_ext_func_mode(dev);
>  }
>
>
> So instead of enable the device the device gets disabled. However after 
> some time the serial line gets back, most likely some "enable resources" 
> might fix it.
>
> I'm attaching patch which somewhat fixes the problem and changes the 
> function to look same in all superio code. Some boards even did not convert 
> the dev->enabled to 0,1 values.
>
> Also makes me wonder some sio call pnp_enable directly, which has yet 
> another semantics, it wont enable already enabled devices just disable 
> those which declared not enabled. And no PnP enter conf magic is called 
> (maybe this is the reason).
>
> Signed-off-by: Rudolf Marek <r.marek@assembler.cz>

Acked-by: Peter Stuge <peter@stuge.se>

Peter Stuge - 2011-02-12 17:35:34

Rudolf Marek wrote:
> Can please someone take a look into this?

Thanks for the ping. Good fix. Go for it!


//Peter

Patch

Index: src/superio/via/vt1211/vt1211.c
===================================================================
--- src/superio/via/vt1211/vt1211.c	(revision 6298)
+++ src/superio/via/vt1211/vt1211.c	(working copy)
@@ -186,12 +186,9 @@ 
 
 static void vt1211_pnp_enable(device_t dev)
 {
-	if (dev->enabled)
-		return;
-
 	pnp_enter_ext_func_mode(dev);
 	pnp_set_logical_device(dev);
-	pnp_set_enable(dev, 0);
+	pnp_set_enable(dev, !!dev->enabled);
 	pnp_exit_ext_func_mode(dev);
 }
 
Index: src/superio/fintek/f71805f/superio.c
===================================================================
--- src/superio/fintek/f71805f/superio.c	(revision 6298)
+++ src/superio/fintek/f71805f/superio.c	(working copy)
@@ -77,7 +77,7 @@ 
 {
 	pnp_enter_conf_state(dev);
 	pnp_set_logical_device(dev);
-	(dev->enabled) ? pnp_set_enable(dev, 1) : pnp_set_enable(dev, 0);
+	pnp_set_enable(dev, !!dev->enabled);
 	pnp_exit_conf_state(dev);
 }
 
Index: src/superio/fintek/f71859/superio.c
===================================================================
--- src/superio/fintek/f71859/superio.c	(revision 6298)
+++ src/superio/fintek/f71859/superio.c	(working copy)
@@ -74,7 +74,7 @@ 
 {
 	pnp_enter_conf_state(dev);
 	pnp_set_logical_device(dev);
-	(dev->enabled) ? pnp_set_enable(dev, 1) : pnp_set_enable(dev, 0);
+	pnp_set_enable(dev, !!dev->enabled);
 	pnp_exit_conf_state(dev);
 }
 
Index: src/superio/fintek/f71889/superio.c
===================================================================
--- src/superio/fintek/f71889/superio.c	(revision 6298)
+++ src/superio/fintek/f71889/superio.c	(working copy)
@@ -80,7 +80,7 @@ 
 {
 	pnp_enter_conf_state(dev);
 	pnp_set_logical_device(dev);
-	(dev->enabled) ? pnp_set_enable(dev, 1) : pnp_set_enable(dev, 0);
+	pnp_set_enable(dev, !!dev->enabled);
 	pnp_exit_conf_state(dev);
 }
 
Index: src/superio/fintek/f71863fg/superio.c
===================================================================
--- src/superio/fintek/f71863fg/superio.c	(revision 6298)
+++ src/superio/fintek/f71863fg/superio.c	(working copy)
@@ -81,7 +81,7 @@ 
 {
 	pnp_enter_conf_state(dev);
 	pnp_set_logical_device(dev);
-	(dev->enabled) ? pnp_set_enable(dev, 1) : pnp_set_enable(dev, 0);
+	pnp_set_enable(dev, !!dev->enabled);
 	pnp_exit_conf_state(dev);
 }
 
Index: src/superio/fintek/f71872/superio.c
===================================================================
--- src/superio/fintek/f71872/superio.c	(revision 6298)
+++ src/superio/fintek/f71872/superio.c	(working copy)
@@ -79,7 +79,7 @@ 
 {
 	pnp_enter_conf_state(dev);
 	pnp_set_logical_device(dev);
-	(dev->enabled) ? pnp_set_enable(dev, 1) : pnp_set_enable(dev, 0);
+	pnp_set_enable(dev, !!dev->enabled);
 	pnp_exit_conf_state(dev);
 }
 
Index: src/superio/winbond/w83627dhg/superio.c
===================================================================
--- src/superio/winbond/w83627dhg/superio.c	(revision 6298)
+++ src/superio/winbond/w83627dhg/superio.c	(working copy)
@@ -76,12 +76,9 @@ 
 
 static void w83627dhg_pnp_enable(device_t dev)
 {
-	if (!dev->enabled)
-		return;
-
 	pnp_enter_ext_func_mode(dev);
 	pnp_set_logical_device(dev);
-	pnp_set_enable(dev, 0);
+	pnp_set_enable(dev, !!dev->enabled);
 	pnp_exit_ext_func_mode(dev);
 }
 
Index: src/superio/winbond/w83627ehg/superio.c
===================================================================
--- src/superio/winbond/w83627ehg/superio.c	(revision 6298)
+++ src/superio/winbond/w83627ehg/superio.c	(working copy)
@@ -160,12 +160,9 @@ 
 
 static void w83627ehg_pnp_enable(device_t dev)
 {
-	if (dev->enabled)
-		return;
-
 	pnp_enter_ext_func_mode(dev);
 	pnp_set_logical_device(dev);
-	pnp_set_enable(dev, 0);
+	pnp_set_enable(dev, !!dev->enabled);
 	pnp_exit_ext_func_mode(dev);
 }
 
Index: src/superio/winbond/w83697hf/superio.c
===================================================================
--- src/superio/winbond/w83697hf/superio.c	(revision 6298)
+++ src/superio/winbond/w83697hf/superio.c	(working copy)
@@ -70,12 +70,9 @@ 
 
 static void w83697hf_pnp_enable(device_t dev)
 {
-	if (dev->enabled)
-		return;
-
 	pnp_enter_ext_func_mode(dev);
 	pnp_set_logical_device(dev);
-	pnp_set_enable(dev, 0);
+	pnp_set_enable(dev, !!dev->enabled);
 	pnp_exit_ext_func_mode(dev);
 }
 
Index: src/superio/winbond/w83627hf/superio.c
===================================================================
--- src/superio/winbond/w83627hf/superio.c	(revision 6298)
+++ src/superio/winbond/w83627hf/superio.c	(working copy)
@@ -179,12 +179,9 @@ 
 
 static void w83627hf_pnp_enable(device_t dev)
 {
-	if (dev->enabled)
-		return;
-
 	pnp_enter_ext_func_mode(dev);
 	pnp_set_logical_device(dev);
-	pnp_set_enable(dev, 0);
+	pnp_set_enable(dev, !!dev->enabled);
 	pnp_exit_ext_func_mode(dev);
 }
 
Index: src/superio/ite/it8712f/superio.c
===================================================================
--- src/superio/ite/it8712f/superio.c	(revision 6298)
+++ src/superio/ite/it8712f/superio.c	(working copy)
@@ -99,7 +99,7 @@ 
 {
 	pnp_enter_ext_func_mode(dev);
 	pnp_set_logical_device(dev);
-	pnp_set_enable(dev, dev->enabled);
+	pnp_set_enable(dev, !!dev->enabled);
 	pnp_exit_ext_func_mode(dev);
 }
 
Index: src/superio/ite/it8716f/superio.c
===================================================================
--- src/superio/ite/it8716f/superio.c	(revision 6298)
+++ src/superio/ite/it8716f/superio.c	(working copy)
@@ -122,7 +122,7 @@ 
 {
 	pnp_enter_ext_func_mode(dev);
 	pnp_set_logical_device(dev);
-	pnp_set_enable(dev, dev->enabled);
+	pnp_set_enable(dev, !!dev->enabled);
 	pnp_exit_ext_func_mode(dev);
 }
 
Index: src/superio/smsc/lpc47m10x/superio.c
===================================================================
--- src/superio/smsc/lpc47m10x/superio.c	(revision 6298)
+++ src/superio/smsc/lpc47m10x/superio.c	(working copy)
@@ -103,7 +103,7 @@ 
 {
 	pnp_enter_conf_state(dev);
 	pnp_set_logical_device(dev);
-	pnp_set_enable(dev, (dev->enabled) ? 1 : 0);
+	pnp_set_enable(dev, !!dev->enabled);
 	pnp_exit_conf_state(dev);
 }
 
Index: src/superio/smsc/lpc47n217/superio.c
===================================================================
--- src/superio/smsc/lpc47n217/superio.c	(revision 6298)
+++ src/superio/smsc/lpc47n217/superio.c	(working copy)
@@ -119,7 +119,7 @@ 
 static void lpc47n217_pnp_enable(device_t dev)
 {
 	pnp_enter_conf_state(dev);
-	lpc47n217_pnp_set_enable(dev, (dev->enabled) ? 1 : 0);
+	lpc47n217_pnp_set_enable(dev, !!dev->enabled);
 	pnp_exit_conf_state(dev);
 }
 
Index: src/superio/smsc/lpc47n227/superio.c
===================================================================
--- src/superio/smsc/lpc47n227/superio.c	(revision 6298)
+++ src/superio/smsc/lpc47n227/superio.c	(working copy)
@@ -116,7 +116,7 @@ 
 void lpc47n227_pnp_enable(device_t dev)
 {
 	pnp_enter_conf_state(dev);
-	lpc47n227_pnp_set_enable(dev, (dev->enabled) ? 1 : 0);
+	lpc47n227_pnp_set_enable(dev, !!dev->enabled);
 	pnp_exit_conf_state(dev);
 }
 
Index: src/superio/smsc/lpc47m15x/superio.c
===================================================================
--- src/superio/smsc/lpc47m15x/superio.c	(revision 6298)
+++ src/superio/smsc/lpc47m15x/superio.c	(working copy)
@@ -87,7 +87,7 @@ 
 {
 	pnp_enter_conf_state(dev);
 	pnp_set_logical_device(dev);
-	pnp_set_enable(dev, (dev->enabled) ? 1 : 0);
+	pnp_set_enable(dev, !!dev->enabled);
 	pnp_exit_conf_state(dev);
 }
 
Index: src/superio/smsc/lpc47b272/superio.c
===================================================================
--- src/superio/smsc/lpc47b272/superio.c	(revision 6298)
+++ src/superio/smsc/lpc47b272/superio.c	(working copy)
@@ -105,7 +105,7 @@ 
 {
 	pnp_enter_conf_state(dev);
 	pnp_set_logical_device(dev);
-	pnp_set_enable(dev, (dev->enabled) ? 1 : 0);
+	pnp_set_enable(dev, !!dev->enabled);
 	pnp_exit_conf_state(dev);
 }
 
Index: src/superio/smsc/smscsuperio/superio.c
===================================================================
--- src/superio/smsc/smscsuperio/superio.c	(revision 6298)
+++ src/superio/smsc/smscsuperio/superio.c	(working copy)
@@ -193,7 +193,7 @@ 
 {
 	smsc_pnp_enter_conf_state(dev);
 	pnp_set_logical_device(dev);
-	(dev->enabled) ? pnp_set_enable(dev, 1) : pnp_set_enable(dev, 0);
+	pnp_set_enable(dev, !!dev->enabled);
 	smsc_pnp_exit_conf_state(dev);
 }
 
Index: src/superio/smsc/lpc47b397/superio.c
===================================================================
--- src/superio/smsc/lpc47b397/superio.c	(revision 6298)
+++ src/superio/smsc/lpc47b397/superio.c	(working copy)
@@ -116,7 +116,7 @@ 
 {
 	pnp_enter_conf_state(dev);
 	pnp_set_logical_device(dev);
-	pnp_set_enable(dev, (dev->enabled) ? 1 : 0);
+	pnp_set_enable(dev, !!dev->enabled);
 	pnp_exit_conf_state(dev);
 }
 
Index: src/superio/intel/i3100/superio.c
===================================================================
--- src/superio/intel/i3100/superio.c	(revision 6298)
+++ src/superio/intel/i3100/superio.c	(working copy)
@@ -78,7 +78,7 @@ 
 {
 	pnp_enter_ext_func_mode(dev);
 	pnp_set_logical_device(dev);
-	pnp_set_enable(dev, dev->enabled);
+	pnp_set_enable(dev, !!dev->enabled);
 	pnp_exit_ext_func_mode(dev);
 }

Patchwork Fix superio enable calls

Comments

Patch