[PATCH 3/4 v2] libertas: implement function init/shutdown commands for SD8688

[PATCH 3/4 v2] libertas: implement function init/shutdown commands for SD8688

[Bing Zhao]

SD8688 is a WLAN/Bluetooth combo chip and both functions are supported
in a single firmware image. FUNC_INIT and FUNC_SHUTDOWN commands are
implemented to utilize the multiple function feature.

When SD8688 card is inserted, the firmware image should be downloaded
only once through either WLAN function (Libertas driver) or Bluetooth
function (Bluetooth driver).

This patch adds function init/shutdown for SD8688 WLAN function only.

Signed-off-by: Bing Zhao <bzhao@marvell.com>
---
 drivers/net/wireless/libertas/host.h    |    2 +
 drivers/net/wireless/libertas/if_sdio.c |   43 +++++++++++++++++++++++++++++-
 2 files changed, 43 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/libertas/host.h b/drivers/net/wireless/libertas/host.h
index 8ff8ac9..fe8f0cb 100644
--- a/drivers/net/wireless/libertas/host.h
+++ b/drivers/net/wireless/libertas/host.h
@@ -86,6 +86,8 @@
 #define CMD_MESH_CONFIG_OLD			0x00a3
 #define CMD_MESH_CONFIG				0x00ac
 #define	CMD_SET_BOOT2_VER			0x00a5
+#define	CMD_FUNC_INIT				0x00a9
+#define	CMD_FUNC_SHUTDOWN			0x00aa
 #define CMD_802_11_BEACON_CTRL			0x00b0
 
 /* For the IEEE Power Save */
diff --git a/drivers/net/wireless/libertas/if_sdio.c b/drivers/net/wireless/libertas/if_sdio.c
index e998c12..84fef6b 100644
--- a/drivers/net/wireless/libertas/if_sdio.c
+++ b/drivers/net/wireless/libertas/if_sdio.c
@@ -39,8 +39,12 @@
 #include "decl.h"
 #include "defs.h"
 #include "dev.h"
+#include "cmd.h"
 #include "if_sdio.h"
 
+/* set this flag in if_sdio_exit_module() if user removes this module */
+static u8 user_rmmod;
+
 static char *lbs_helper_name = NULL;
 module_param_named(helper_name, lbs_helper_name, charp, 0644);
 
@@ -539,7 +543,6 @@ static int if_sdio_prog_helper(struct if_sdio_card *card)
 	ret = 0;
 
 release:
-	sdio_set_block_size(card->func, IF_SDIO_BLOCK_SIZE);
 	sdio_release_host(card->func);
 	kfree(chunk_buffer);
 release_fw:
@@ -675,7 +678,6 @@ static int if_sdio_prog_real(struct if_sdio_card *card)
 	ret = 0;
 
 release:
-	sdio_set_block_size(card->func, IF_SDIO_BLOCK_SIZE);
 	sdio_release_host(card->func);
 	kfree(chunk_buffer);
 release_fw:
@@ -718,6 +720,9 @@ static int if_sdio_prog_firmware(struct if_sdio_card *card)
 		goto out;
 
 success:
+	sdio_claim_host(card->func);
+	sdio_set_block_size(card->func, IF_SDIO_BLOCK_SIZE);
+	sdio_release_host(card->func);
 	ret = 0;
 
 out:
@@ -985,6 +990,20 @@ static int if_sdio_probe(struct sdio_func *func,
 	if (ret)
 		goto reclaim;
 
+	/*
+	 * FUNC_INIT is required for SD8688 WLAN/BT multiple functions
+	 */
+	if (card->model == IF_SDIO_MODEL_8688) {
+		struct cmd_header cmd;
+
+		memset(&cmd, 0, sizeof(cmd));
+
+		lbs_deb_sdio("send function INIT command\n");
+		if (__lbs_cmd(priv, CMD_FUNC_INIT, &cmd, sizeof(cmd),
+				lbs_cmd_copyback, (unsigned long) &cmd))
+			lbs_pr_alert("CMD_FUNC_INIT cmd failed\n");
+	}
+
 	ret = lbs_start_card(priv);
 	if (ret)
 		goto err_activate_card;
@@ -1030,6 +1049,22 @@ static void if_sdio_remove(struct sdio_func *func)
 
 	card = sdio_get_drvdata(func);
 
+	if (user_rmmod && (card->model == IF_SDIO_MODEL_8688)) {
+		/*
+		 * FUNC_SHUTDOWN is required for SD8688 WLAN/BT
+		 * multiple functions
+		 */
+		struct cmd_header cmd;
+
+		memset(&cmd, 0, sizeof(cmd));
+
+		lbs_deb_sdio("send function SHUTDOWN command\n");
+		if (__lbs_cmd(card->priv, CMD_FUNC_SHUTDOWN,
+				&cmd, sizeof(cmd), lbs_cmd_copyback,
+				(unsigned long) &cmd))
+			lbs_pr_alert("CMD_FUNC_SHUTDOWN cmd failed\n");
+	}
+
 	card->priv->surpriseremoved = 1;
 
 	lbs_deb_sdio("call remove card\n");
@@ -1077,6 +1112,8 @@ static int __init if_sdio_init_module(void)
 
 	ret = sdio_register_driver(&if_sdio_driver);
 
+	user_rmmod = 0;
+
 	lbs_deb_leave_args(LBS_DEB_SDIO, "ret %d", ret);
 
 	return ret;
@@ -1086,6 +1123,8 @@ static void __exit if_sdio_exit_module(void)
 {
 	lbs_deb_enter(LBS_DEB_SDIO);
 
+	user_rmmod = 1;
+
 	sdio_unregister_driver(&if_sdio_driver);
 
 	lbs_deb_leave(LBS_DEB_SDIO);
-- 
1.5.3.6

Re: [PATCH 3/4 v2] libertas: implement function init/shutdown commands for SD8688

[Marcel Holtmann]

Hi Bing,

> SD8688 is a WLAN/Bluetooth combo chip and both functions are supported
> in a single firmware image. FUNC_INIT and FUNC_SHUTDOWN commands are
> implemented to utilize the multiple function feature.
> 
> When SD8688 card is inserted, the firmware image should be downloaded
> only once through either WLAN function (Libertas driver) or Bluetooth
> function (Bluetooth driver).
> 
> This patch adds function init/shutdown for SD8688 WLAN function only.
> 
> Signed-off-by: Bing Zhao <bzhao@marvell.com>
> ---
>  drivers/net/wireless/libertas/host.h    |    2 +
>  drivers/net/wireless/libertas/if_sdio.c |   43 +++++++++++++++++++++++++++++-
>  2 files changed, 43 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/libertas/host.h b/drivers/net/wireless/libertas/host.h
> index 8ff8ac9..fe8f0cb 100644
> --- a/drivers/net/wireless/libertas/host.h
> +++ b/drivers/net/wireless/libertas/host.h
> @@ -86,6 +86,8 @@
>  #define CMD_MESH_CONFIG_OLD			0x00a3
>  #define CMD_MESH_CONFIG				0x00ac
>  #define	CMD_SET_BOOT2_VER			0x00a5
> +#define	CMD_FUNC_INIT				0x00a9
> +#define	CMD_FUNC_SHUTDOWN			0x00aa
>  #define CMD_802_11_BEACON_CTRL			0x00b0
>  
>  /* For the IEEE Power Save */
> diff --git a/drivers/net/wireless/libertas/if_sdio.c b/drivers/net/wireless/libertas/if_sdio.c
> index e998c12..84fef6b 100644
> --- a/drivers/net/wireless/libertas/if_sdio.c
> +++ b/drivers/net/wireless/libertas/if_sdio.c
> @@ -39,8 +39,12 @@
>  #include "decl.h"
>  #include "defs.h"
>  #include "dev.h"
> +#include "cmd.h"
>  #include "if_sdio.h"
>  
> +/* set this flag in if_sdio_exit_module() if user removes this module */
> +static u8 user_rmmod;
> +

what is this suppose to be doing? There is no description in the commit
messages or the code. And it looks pretty hackish to me.

Regards

Marcel

RE: [PATCH 3/4 v2] libertas: implement function init/shutdown commands for SD8688

[Bing Zhao]

> -----Original Message-----
> From: Marcel Holtmann [mailto:marcel@holtmann.org]
> Sent: Thursday, May 28, 2009 12:21 AM
> To: Bing Zhao
> Cc: libertas-dev@lists.infradead.org; linux-wireless@vger.kernel.org
> Subject: Re: [PATCH 3/4 v2] libertas: implement function init/shutdown commands for SD8688
> 
> Hi Bing,
> 
> > SD8688 is a WLAN/Bluetooth combo chip and both functions are supported
> > in a single firmware image. FUNC_INIT and FUNC_SHUTDOWN commands are
> > implemented to utilize the multiple function feature.
> >
> > When SD8688 card is inserted, the firmware image should be downloaded
> > only once through either WLAN function (Libertas driver) or Bluetooth
> > function (Bluetooth driver).
> >
> > This patch adds function init/shutdown for SD8688 WLAN function only.
> >
> > Signed-off-by: Bing Zhao <bzhao@marvell.com>
> > ---
> >  drivers/net/wireless/libertas/host.h    |    2 +
> >  drivers/net/wireless/libertas/if_sdio.c |   43 +++++++++++++++++++++++++++++-
> >  2 files changed, 43 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/wireless/libertas/host.h b/drivers/net/wireless/libertas/host.h
> > index 8ff8ac9..fe8f0cb 100644
> > --- a/drivers/net/wireless/libertas/host.h
> > +++ b/drivers/net/wireless/libertas/host.h
> > @@ -86,6 +86,8 @@
> >  #define CMD_MESH_CONFIG_OLD			0x00a3
> >  #define CMD_MESH_CONFIG				0x00ac
> >  #define	CMD_SET_BOOT2_VER			0x00a5
> > +#define	CMD_FUNC_INIT				0x00a9
> > +#define	CMD_FUNC_SHUTDOWN			0x00aa
> >  #define CMD_802_11_BEACON_CTRL			0x00b0
> >
> >  /* For the IEEE Power Save */
> > diff --git a/drivers/net/wireless/libertas/if_sdio.c b/drivers/net/wireless/libertas/if_sdio.c
> > index e998c12..84fef6b 100644
> > --- a/drivers/net/wireless/libertas/if_sdio.c
> > +++ b/drivers/net/wireless/libertas/if_sdio.c
> > @@ -39,8 +39,12 @@
> >  #include "decl.h"
> >  #include "defs.h"
> >  #include "dev.h"
> > +#include "cmd.h"
> >  #include "if_sdio.h"
> >
> > +/* set this flag in if_sdio_exit_module() if user removes this module */
> > +static u8 user_rmmod;
> > +
> 
> what is this suppose to be doing? There is no description in the commit
> messages or the code. And it looks pretty hackish to me.
> 
> Regards
> 
> Marcel
> 


Hi Marcel,

I'm using this flag as the indication of module removal.

I have to distinguish these scenarios and handle them differently:
(1) user unplugs the card: As the card is unplugged, there is no way (no need) to send SHUTDOWN command.
(2) user removes the libertas_sdio module manually: Since the card is still in the slot, SHUTDOWN command will be sent to firmware.

When user rmmod the module, if_sdio_exit_module() is called and this flag "user_rmmod" is set to 1. When if_sdio_remove() is called this flag will be used to determine whether the SHUTDOWN command should be sent or not.

I hesitated to use this global variable in such a weird way. But I couldn't think of a better solution.

Thanks,

Bing

RE: [PATCH 3/4 v2] libertas: implement function init/shutdown commands for SD8688

[Marcel Holtmann]

Hi Bing,

> > > SD8688 is a WLAN/Bluetooth combo chip and both functions are supported
> > > in a single firmware image. FUNC_INIT and FUNC_SHUTDOWN commands are
> > > implemented to utilize the multiple function feature.
> > >
> > > When SD8688 card is inserted, the firmware image should be downloaded
> > > only once through either WLAN function (Libertas driver) or Bluetooth
> > > function (Bluetooth driver).
> > >
> > > This patch adds function init/shutdown for SD8688 WLAN function only.
> > >
> > > Signed-off-by: Bing Zhao <bzhao@marvell.com>
> > > ---
> > >  drivers/net/wireless/libertas/host.h    |    2 +
> > >  drivers/net/wireless/libertas/if_sdio.c |   43 +++++++++++++++++++++++++++++-
> > >  2 files changed, 43 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/net/wireless/libertas/host.h b/drivers/net/wireless/libertas/host.h
> > > index 8ff8ac9..fe8f0cb 100644
> > > --- a/drivers/net/wireless/libertas/host.h
> > > +++ b/drivers/net/wireless/libertas/host.h
> > > @@ -86,6 +86,8 @@
> > >  #define CMD_MESH_CONFIG_OLD			0x00a3
> > >  #define CMD_MESH_CONFIG				0x00ac
> > >  #define	CMD_SET_BOOT2_VER			0x00a5
> > > +#define	CMD_FUNC_INIT				0x00a9
> > > +#define	CMD_FUNC_SHUTDOWN			0x00aa
> > >  #define CMD_802_11_BEACON_CTRL			0x00b0
> > >
> > >  /* For the IEEE Power Save */
> > > diff --git a/drivers/net/wireless/libertas/if_sdio.c b/drivers/net/wireless/libertas/if_sdio.c
> > > index e998c12..84fef6b 100644
> > > --- a/drivers/net/wireless/libertas/if_sdio.c
> > > +++ b/drivers/net/wireless/libertas/if_sdio.c
> > > @@ -39,8 +39,12 @@
> > >  #include "decl.h"
> > >  #include "defs.h"
> > >  #include "dev.h"
> > > +#include "cmd.h"
> > >  #include "if_sdio.h"
> > >
> > > +/* set this flag in if_sdio_exit_module() if user removes this module */
> > > +static u8 user_rmmod;
> > > +
> > 
> > what is this suppose to be doing? There is no description in the commit
> > messages or the code. And it looks pretty hackish to me.
>
> I'm using this flag as the indication of module removal.
> 
> I have to distinguish these scenarios and handle them differently:
> (1) user unplugs the card: As the card is unplugged, there is no way (no need) to send SHUTDOWN command.
> (2) user removes the libertas_sdio module manually: Since the card is still in the slot, SHUTDOWN command will be sent to firmware.
> 
> When user rmmod the module, if_sdio_exit_module() is called and this flag "user_rmmod" is set to 1. When if_sdio_remove() is called this flag will be used to determine whether the SHUTDOWN command should be sent or not.
> 
> I hesitated to use this global variable in such a weird way. But I couldn't think of a better solution.

I still don't see why this is needed. The rmmod should trigger the
driver removal and you can do cleanup via the removal callback. I do
might be missing something essential here, but I have never seen any
driver needing such a weird hack.

Regards

Marcel

RE: [PATCH 3/4 v2] libertas: implement function init/shutdown commands for SD8688

[Bing Zhao]

> -----Original Message-----
> From: Marcel Holtmann [mailto:marcel@holtmann.org]
> Sent: Thursday, May 28, 2009 12:20 PM
> To: Bing Zhao
> Cc: libertas-dev@lists.infradead.org; linux-wireless@vger.kernel.org
> Subject: RE: [PATCH 3/4 v2] libertas: implement function init/shutdown commands for SD8688
> 
> Hi Bing,
> 
> > > > SD8688 is a WLAN/Bluetooth combo chip and both functions are supported
> > > > in a single firmware image. FUNC_INIT and FUNC_SHUTDOWN commands are
> > > > implemented to utilize the multiple function feature.
> > > >
> > > > When SD8688 card is inserted, the firmware image should be downloaded
> > > > only once through either WLAN function (Libertas driver) or Bluetooth
> > > > function (Bluetooth driver).
> > > >
> > > > This patch adds function init/shutdown for SD8688 WLAN function only.
> > > >
> > > > Signed-off-by: Bing Zhao <bzhao@marvell.com>
> > > > ---
> > > >  drivers/net/wireless/libertas/host.h    |    2 +
> > > >  drivers/net/wireless/libertas/if_sdio.c |   43 +++++++++++++++++++++++++++++-
> > > >  2 files changed, 43 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/net/wireless/libertas/host.h b/drivers/net/wireless/libertas/host.h
> > > > index 8ff8ac9..fe8f0cb 100644
> > > > --- a/drivers/net/wireless/libertas/host.h
> > > > +++ b/drivers/net/wireless/libertas/host.h
> > > > @@ -86,6 +86,8 @@
> > > >  #define CMD_MESH_CONFIG_OLD			0x00a3
> > > >  #define CMD_MESH_CONFIG				0x00ac
> > > >  #define	CMD_SET_BOOT2_VER			0x00a5
> > > > +#define	CMD_FUNC_INIT				0x00a9
> > > > +#define	CMD_FUNC_SHUTDOWN			0x00aa
> > > >  #define CMD_802_11_BEACON_CTRL			0x00b0
> > > >
> > > >  /* For the IEEE Power Save */
> > > > diff --git a/drivers/net/wireless/libertas/if_sdio.c b/drivers/net/wireless/libertas/if_sdio.c
> > > > index e998c12..84fef6b 100644
> > > > --- a/drivers/net/wireless/libertas/if_sdio.c
> > > > +++ b/drivers/net/wireless/libertas/if_sdio.c
> > > > @@ -39,8 +39,12 @@
> > > >  #include "decl.h"
> > > >  #include "defs.h"
> > > >  #include "dev.h"
> > > > +#include "cmd.h"
> > > >  #include "if_sdio.h"
> > > >
> > > > +/* set this flag in if_sdio_exit_module() if user removes this module */
> > > > +static u8 user_rmmod;
> > > > +
> > >
> > > what is this suppose to be doing? There is no description in the commit
> > > messages or the code. And it looks pretty hackish to me.
> >
> > I'm using this flag as the indication of module removal.
> >
> > I have to distinguish these scenarios and handle them differently:
> > (1) user unplugs the card: As the card is unplugged, there is no way (no need) to send SHUTDOWN
> command.
> > (2) user removes the libertas_sdio module manually: Since the card is still in the slot, SHUTDOWN
> command will be sent to firmware.
> >
> > When user rmmod the module, if_sdio_exit_module() is called and this flag "user_rmmod" is set to 1.
> When if_sdio_remove() is called this flag will be used to determine whether the SHUTDOWN command
> should be sent or not.
> >
> > I hesitated to use this global variable in such a weird way. But I couldn't think of a better
> solution.
> 
> I still don't see why this is needed. The rmmod should trigger the
> driver removal and you can do cleanup via the removal callback. I do
> might be missing something essential here, but I have never seen any
> driver needing such a weird hack.
> 
> Regards
> 
> Marcel
> 

Hi Marcel,

Unplugging the card also triggers if_sdio_remove() callback. I wonder if MMC/SDIO driver can provide such indication that upper layer driver can distinguish the two events: card removal or module removal.

Thanks,

Bing
 

Re: [PATCH 3/4 v2] libertas: implement function init/shutdown commands for SD8688

[John W. Linville]

On Wed, May 27, 2009 at 04:03:33PM -0700, Bing Zhao wrote:
> SD8688 is a WLAN/Bluetooth combo chip and both functions are supported
> in a single firmware image. FUNC_INIT and FUNC_SHUTDOWN commands are
> implemented to utilize the multiple function feature.
> 
> When SD8688 card is inserted, the firmware image should be downloaded
> only once through either WLAN function (Libertas driver) or Bluetooth
> function (Bluetooth driver).
> 
> This patch adds function init/shutdown for SD8688 WLAN function only.
> 
> Signed-off-by: Bing Zhao <bzhao@marvell.com>

I already merged the 5/19 version.  Could you send a patch rebased
on wireless-testing?

John
-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

RE: [PATCH 3/4 v2] libertas: implement function init/shutdown commands for SD8688

[Bing Zhao]

Hi John,

The new patch against "master-2009-06-01" has been submitted to the lists:

[PATCH] libertas: improve function init/shutdown handling for SD8688

Thanks,

Bing

> -----Original Message-----
> From: John W. Linville [mailto:linville@tuxdriver.com]
> Sent: Monday, June 01, 2009 11:35 AM
> To: Bing Zhao
> Cc: libertas-dev@lists.infradead.org; linux-wireless@vger.kernel.org
> Subject: Re: [PATCH 3/4 v2] libertas: implement function init/shutdown commands for SD8688
> 
> On Wed, May 27, 2009 at 04:03:33PM -0700, Bing Zhao wrote:
> > SD8688 is a WLAN/Bluetooth combo chip and both functions are supported
> > in a single firmware image. FUNC_INIT and FUNC_SHUTDOWN commands are
> > implemented to utilize the multiple function feature.
> >
> > When SD8688 card is inserted, the firmware image should be downloaded
> > only once through either WLAN function (Libertas driver) or Bluetooth
> > function (Bluetooth driver).
> >
> > This patch adds function init/shutdown for SD8688 WLAN function only.
> >
> > Signed-off-by: Bing Zhao <bzhao@marvell.com>
> 
> I already merged the 5/19 version.  Could you send a patch rebased
> on wireless-testing?
> 
> John
> --
> John W. Linville		Someday the world will need a hero, and you
> linville@tuxdriver.com			might be all we have.  Be ready.