[PATCH 1/4] libertas: define macros for SDIO model numbers

[PATCH 1/4] libertas: define macros for SDIO model numbers

[Bing Zhao]

replace direct usages of SDIO model numbers with defined macros.

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

diff --git a/drivers/net/wireless/libertas/if_sdio.c b/drivers/net/wireless/libertas/if_sdio.c
index 55864c1..69d7fd3 100644
--- a/drivers/net/wireless/libertas/if_sdio.c
+++ b/drivers/net/wireless/libertas/if_sdio.c
@@ -66,19 +66,19 @@ struct if_sdio_model {
 static struct if_sdio_model if_sdio_models[] = {
 	{
 		/* 8385 */
-		.model = 0x04,
+		.model = IF_SDIO_MODEL_8385,
 		.helper = "sd8385_helper.bin",
 		.firmware = "sd8385.bin",
 	},
 	{
 		/* 8686 */
-		.model = 0x0B,
+		.model = IF_SDIO_MODEL_8686,
 		.helper = "sd8686_helper.bin",
 		.firmware = "sd8686.bin",
 	},
 	{
 		/* 8688 */
-		.model = 0x10,
+		.model = IF_SDIO_MODEL_8688,
 		.helper = "sd8688_helper.bin",
 		.firmware = "sd8688.bin",
 	},
@@ -118,7 +118,7 @@ static u16 if_sdio_read_scratch(struct if_sdio_card *card, int *err)
 	int ret, reg;
 	u16 scratch;
 
-	if (card->model == 0x04)
+	if (card->model == IF_SDIO_MODEL_8385)
 		reg = IF_SDIO_SCRATCH_OLD;
 	else
 		reg = IF_SDIO_SCRATCH;
@@ -216,7 +216,7 @@ static int if_sdio_handle_event(struct if_sdio_card *card,
 
 	lbs_deb_enter(LBS_DEB_SDIO);
 
-	if (card->model == 0x04) {
+	if (card->model == IF_SDIO_MODEL_8385) {
 		event = sdio_readb(card->func, IF_SDIO_EVENT, &ret);
 		if (ret)
 			goto out;
@@ -829,10 +829,10 @@ static int if_sdio_probe(struct sdio_func *func,
 		if (sscanf(func->card->info[i],
 				"ID: %x", &model) == 1)
 			break;
-               if (!strcmp(func->card->info[i], "IBIS Wireless SDIO Card")) {
-                       model = 4;
-                       break;
-               }
+		if (!strcmp(func->card->info[i], "IBIS Wireless SDIO Card")) {
+			model = IF_SDIO_MODEL_8385;
+			break;
+		}
 	}
 
 	if (i == func->card->num_info) {
diff --git a/drivers/net/wireless/libertas/if_sdio.h b/drivers/net/wireless/libertas/if_sdio.h
index 37ada2c..cea343f 100644
--- a/drivers/net/wireless/libertas/if_sdio.h
+++ b/drivers/net/wireless/libertas/if_sdio.h
@@ -12,6 +12,10 @@
 #ifndef _LBS_IF_SDIO_H
 #define _LBS_IF_SDIO_H
 
+#define IF_SDIO_MODEL_8385	0x04
+#define IF_SDIO_MODEL_8686	0x0b
+#define IF_SDIO_MODEL_8688	0x10
+
 #define IF_SDIO_IOPORT		0x00
 
 #define IF_SDIO_H_INT_MASK	0x04
-- 
1.5.3.6

[PATCH 2/4] libertas: get SD8688 rx length with one CMD52

[Bing Zhao]

Usually, the 16-bit rx length is read from scratch pad registers
with two CMD52 transactions:
SD8385: 	IF_SDIO_SCRATCH_OLD (0x80fe/0x80ff)
SD8686/SD8688:	IF_SDIO_SCRATCH     (0x34/0x35)

Alternatively, SD8688 firmware offers an enhanced method for driver
to read an 8-bit rx length (in units) with a single CMD52:
IF_SDIO_RX_UNIT 0x43 is read one time after firmware is ready.
IF_SDIO_RX_LEN  0x42 is read every time when rx interrupt is received.

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

diff --git a/drivers/net/wireless/libertas/if_sdio.c b/drivers/net/wireless/libertas/if_sdio.c
index 69d7fd3..e998c12 100644
--- a/drivers/net/wireless/libertas/if_sdio.c
+++ b/drivers/net/wireless/libertas/if_sdio.c
@@ -107,6 +107,8 @@ struct if_sdio_card {
 
 	struct workqueue_struct	*workqueue;
 	struct work_struct	packet_worker;
+
+	u8			rx_unit;
 };
 
 /********************************************************************/
@@ -136,6 +138,46 @@ static u16 if_sdio_read_scratch(struct if_sdio_card *card, int *err)
 	return scratch;
 }
 
+static u8 if_sdio_read_rx_unit(struct if_sdio_card *card)
+{
+	int ret;
+	u8 rx_unit;
+
+	rx_unit = sdio_readb(card->func, IF_SDIO_RX_UNIT, &ret);
+
+	if (ret)
+		rx_unit = 0;
+
+	return rx_unit;
+}
+
+static u16 if_sdio_read_rx_len(struct if_sdio_card *card, int *err)
+{
+	int ret;
+	u16 rx_len;
+
+	switch (card->model) {
+	case IF_SDIO_MODEL_8385:
+	case IF_SDIO_MODEL_8686:
+		rx_len = if_sdio_read_scratch(card, &ret);
+		break;
+	case IF_SDIO_MODEL_8688:
+	default: /* for newer chipsets */
+		rx_len = sdio_readb(card->func, IF_SDIO_RX_LEN, &ret);
+		if (!ret)
+			rx_len <<= card->rx_unit;
+		else
+			rx_len = 0xffff;	/* invalid length */
+
+		break;
+	}
+
+	if (err)
+		*err = ret;
+
+	return rx_len;
+}
+
 static int if_sdio_handle_cmd(struct if_sdio_card *card,
 		u8 *buffer, unsigned size)
 {
@@ -254,7 +296,7 @@ static int if_sdio_card_to_host(struct if_sdio_card *card)
 
 	lbs_deb_enter(LBS_DEB_SDIO);
 
-	size = if_sdio_read_scratch(card, &ret);
+	size = if_sdio_read_rx_len(card, &ret);
 	if (ret)
 		goto out;
 
@@ -923,10 +965,21 @@ static int if_sdio_probe(struct sdio_func *func,
 
 	priv->fw_ready = 1;
 
+	sdio_claim_host(func);
+
+	/*
+	 * Get rx_unit if the chip is SD8688 or newer.
+	 * SD8385 & SD8686 do not have rx_unit.
+	 */
+	if ((card->model != IF_SDIO_MODEL_8385)
+			&& (card->model != IF_SDIO_MODEL_8686))
+		card->rx_unit = if_sdio_read_rx_unit(card);
+	else
+		card->rx_unit = 0;
+
 	/*
 	 * Enable interrupts now that everything is set up
 	 */
-	sdio_claim_host(func);
 	sdio_writeb(func, 0x0f, IF_SDIO_H_INT_MASK, &ret);
 	sdio_release_host(func);
 	if (ret)
diff --git a/drivers/net/wireless/libertas/if_sdio.h b/drivers/net/wireless/libertas/if_sdio.h
index cea343f..d3a4fbe 100644
--- a/drivers/net/wireless/libertas/if_sdio.h
+++ b/drivers/net/wireless/libertas/if_sdio.h
@@ -44,6 +44,9 @@
 #define IF_SDIO_SCRATCH_OLD	0x80fe
 #define   IF_SDIO_FIRMWARE_OK	0xfedc
 
+#define IF_SDIO_RX_LEN		0x42
+#define IF_SDIO_RX_UNIT		0x43
+
 #define IF_SDIO_EVENT           0x80fc
 
 #define IF_SDIO_BLOCK_SIZE	256
-- 
1.5.3.6

Re: [PATCH 2/4] libertas: get SD8688 rx length with one CMD52

[Dan Williams]

On Tue, 2009-05-19 at 19:48 -0700, Bing Zhao wrote:
> Usually, the 16-bit rx length is read from scratch pad registers
> with two CMD52 transactions:
> SD8385: 	IF_SDIO_SCRATCH_OLD (0x80fe/0x80ff)
> SD8686/SD8688:	IF_SDIO_SCRATCH     (0x34/0x35)
> 
> Alternatively, SD8688 firmware offers an enhanced method for driver
> to read an 8-bit rx length (in units) with a single CMD52:
> IF_SDIO_RX_UNIT 0x43 is read one time after firmware is ready.
> IF_SDIO_RX_LEN  0x42 is read every time when rx interrupt is received.
> 
> Signed-off-by: Bing Zhao <bzhao@marvell.com>

Acked-by: Dan Williams <dcbw@redhat.com>

> ---
>  drivers/net/wireless/libertas/if_sdio.c |   57 +++++++++++++++++++++++++++++-
>  drivers/net/wireless/libertas/if_sdio.h |    3 ++
>  2 files changed, 58 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/libertas/if_sdio.c b/drivers/net/wireless/libertas/if_sdio.c
> index 69d7fd3..e998c12 100644
> --- a/drivers/net/wireless/libertas/if_sdio.c
> +++ b/drivers/net/wireless/libertas/if_sdio.c
> @@ -107,6 +107,8 @@ struct if_sdio_card {
>  
>  	struct workqueue_struct	*workqueue;
>  	struct work_struct	packet_worker;
> +
> +	u8			rx_unit;
>  };
>  
>  /********************************************************************/
> @@ -136,6 +138,46 @@ static u16 if_sdio_read_scratch(struct if_sdio_card *card, int *err)
>  	return scratch;
>  }
>  
> +static u8 if_sdio_read_rx_unit(struct if_sdio_card *card)
> +{
> +	int ret;
> +	u8 rx_unit;
> +
> +	rx_unit = sdio_readb(card->func, IF_SDIO_RX_UNIT, &ret);
> +
> +	if (ret)
> +		rx_unit = 0;
> +
> +	return rx_unit;
> +}
> +
> +static u16 if_sdio_read_rx_len(struct if_sdio_card *card, int *err)
> +{
> +	int ret;
> +	u16 rx_len;
> +
> +	switch (card->model) {
> +	case IF_SDIO_MODEL_8385:
> +	case IF_SDIO_MODEL_8686:
> +		rx_len = if_sdio_read_scratch(card, &ret);
> +		break;
> +	case IF_SDIO_MODEL_8688:
> +	default: /* for newer chipsets */
> +		rx_len = sdio_readb(card->func, IF_SDIO_RX_LEN, &ret);
> +		if (!ret)
> +			rx_len <<= card->rx_unit;
> +		else
> +			rx_len = 0xffff;	/* invalid length */
> +
> +		break;
> +	}
> +
> +	if (err)
> +		*err = ret;
> +
> +	return rx_len;
> +}
> +
>  static int if_sdio_handle_cmd(struct if_sdio_card *card,
>  		u8 *buffer, unsigned size)
>  {
> @@ -254,7 +296,7 @@ static int if_sdio_card_to_host(struct if_sdio_card *card)
>  
>  	lbs_deb_enter(LBS_DEB_SDIO);
>  
> -	size = if_sdio_read_scratch(card, &ret);
> +	size = if_sdio_read_rx_len(card, &ret);
>  	if (ret)
>  		goto out;
>  
> @@ -923,10 +965,21 @@ static int if_sdio_probe(struct sdio_func *func,
>  
>  	priv->fw_ready = 1;
>  
> +	sdio_claim_host(func);
> +
> +	/*
> +	 * Get rx_unit if the chip is SD8688 or newer.
> +	 * SD8385 & SD8686 do not have rx_unit.
> +	 */
> +	if ((card->model != IF_SDIO_MODEL_8385)
> +			&& (card->model != IF_SDIO_MODEL_8686))
> +		card->rx_unit = if_sdio_read_rx_unit(card);
> +	else
> +		card->rx_unit = 0;
> +
>  	/*
>  	 * Enable interrupts now that everything is set up
>  	 */
> -	sdio_claim_host(func);
>  	sdio_writeb(func, 0x0f, IF_SDIO_H_INT_MASK, &ret);
>  	sdio_release_host(func);
>  	if (ret)
> diff --git a/drivers/net/wireless/libertas/if_sdio.h b/drivers/net/wireless/libertas/if_sdio.h
> index cea343f..d3a4fbe 100644
> --- a/drivers/net/wireless/libertas/if_sdio.h
> +++ b/drivers/net/wireless/libertas/if_sdio.h
> @@ -44,6 +44,9 @@
>  #define IF_SDIO_SCRATCH_OLD	0x80fe
>  #define   IF_SDIO_FIRMWARE_OK	0xfedc
>  
> +#define IF_SDIO_RX_LEN		0x42
> +#define IF_SDIO_RX_UNIT		0x43
> +
>  #define IF_SDIO_EVENT           0x80fc
>  
>  #define IF_SDIO_BLOCK_SIZE	256

[PATCH 3/4] 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/dev.h     |    2 +
 drivers/net/wireless/libertas/host.h    |    2 +
 drivers/net/wireless/libertas/if_sdio.c |   41 ++++++++++++++++++++++++++++--
 drivers/net/wireless/libertas/main.c    |   20 +++++++++++++++
 4 files changed, 62 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/libertas/dev.h b/drivers/net/wireless/libertas/dev.h
index cbaafa6..a4455ec 100644
--- a/drivers/net/wireless/libertas/dev.h
+++ b/drivers/net/wireless/libertas/dev.h
@@ -321,6 +321,8 @@ struct lbs_private {
 
 	u32 monitormode;
 	u8 fw_ready;
+	u8 fn_init_required;
+	u8 fn_shutdown_required;
 };
 
 extern struct cmd_confirm_sleep confirm_sleep;
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..478d766 100644
--- a/drivers/net/wireless/libertas/if_sdio.c
+++ b/drivers/net/wireless/libertas/if_sdio.c
@@ -61,6 +61,7 @@ struct if_sdio_model {
 	int model;
 	const char *helper;
 	const char *firmware;
+	struct if_sdio_card *card;
 };
 
 static struct if_sdio_model if_sdio_models[] = {
@@ -69,18 +70,21 @@ static struct if_sdio_model if_sdio_models[] = {
 		.model = IF_SDIO_MODEL_8385,
 		.helper = "sd8385_helper.bin",
 		.firmware = "sd8385.bin",
+		.card = NULL,
 	},
 	{
 		/* 8686 */
 		.model = IF_SDIO_MODEL_8686,
 		.helper = "sd8686_helper.bin",
 		.firmware = "sd8686.bin",
+		.card = NULL,
 	},
 	{
 		/* 8688 */
 		.model = IF_SDIO_MODEL_8688,
 		.helper = "sd8688_helper.bin",
 		.firmware = "sd8688.bin",
+		.card = NULL,
 	},
 };
 
@@ -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:
@@ -903,6 +908,8 @@ static int if_sdio_probe(struct sdio_func *func,
 		goto free;
 	}
 
+	if_sdio_models[i].card = card;
+
 	card->helper = if_sdio_models[i].helper;
 	card->firmware = if_sdio_models[i].firmware;
 
@@ -985,6 +992,12 @@ static int if_sdio_probe(struct sdio_func *func,
 	if (ret)
 		goto reclaim;
 
+	/*
+	 * FUNC_INIT is required for SD8688 WLAN/BT multiple functions
+	 */
+	priv->fn_init_required =
+		(card->model == IF_SDIO_MODEL_8688) ? 1 : 0;
+
 	ret = lbs_start_card(priv);
 	if (ret)
 		goto err_activate_card;
@@ -1025,23 +1038,30 @@ static void if_sdio_remove(struct sdio_func *func)
 {
 	struct if_sdio_card *card;
 	struct if_sdio_packet *packet;
+	int ret;
 
 	lbs_deb_enter(LBS_DEB_SDIO);
 
 	card = sdio_get_drvdata(func);
 
+	lbs_stop_card(card->priv);
+
 	card->priv->surpriseremoved = 1;
 
 	lbs_deb_sdio("call remove card\n");
-	lbs_stop_card(card->priv);
 	lbs_remove_card(card->priv);
 
 	flush_workqueue(card->workqueue);
 	destroy_workqueue(card->workqueue);
 
 	sdio_claim_host(func);
+
+	/* Disable interrupts */
+	sdio_writeb(func, 0x00, IF_SDIO_H_INT_MASK, &ret);
+
 	sdio_release_irq(func);
 	sdio_disable_func(func);
+
 	sdio_release_host(func);
 
 	while (card->packets) {
@@ -1084,8 +1104,23 @@ static int __init if_sdio_init_module(void)
 
 static void __exit if_sdio_exit_module(void)
 {
+	int i;
+	struct if_sdio_card *card;
+
 	lbs_deb_enter(LBS_DEB_SDIO);
 
+	for (i = 0; i < ARRAY_SIZE(if_sdio_models); i++) {
+		card = if_sdio_models[i].card;
+
+		/*
+		 * FUNC_SHUTDOWN is required for SD8688 WLAN/BT
+		 * multiple functions
+		 */
+		if (card && card->priv)
+			card->priv->fn_shutdown_required =
+				(card->model == IF_SDIO_MODEL_8688) ? 1 : 0;
+	}
+
 	sdio_unregister_driver(&if_sdio_driver);
 
 	lbs_deb_leave(LBS_DEB_SDIO);
diff --git a/drivers/net/wireless/libertas/main.c b/drivers/net/wireless/libertas/main.c
index 89575e4..a58a123 100644
--- a/drivers/net/wireless/libertas/main.c
+++ b/drivers/net/wireless/libertas/main.c
@@ -1002,9 +1002,17 @@ static int lbs_setup_firmware(struct lbs_private *priv)
 {
 	int ret = -1;
 	s16 curlevel = 0, minlevel = 0, maxlevel = 0;
+	struct cmd_header cmd;
 
 	lbs_deb_enter(LBS_DEB_FW);
 
+	if (priv->fn_init_required) {
+		memset(&cmd, 0, sizeof(cmd));
+		if (__lbs_cmd(priv, CMD_FUNC_INIT, &cmd, sizeof(cmd),
+				lbs_cmd_copyback, (unsigned long) &cmd))
+			lbs_pr_alert("CMD_FUNC_INIT command failed\n");
+	}
+
 	/* Read MAC address from firmware */
 	memset(priv->current_addr, 0xff, ETH_ALEN);
 	ret = lbs_update_hw_spec(priv);
@@ -1192,6 +1200,9 @@ struct lbs_private *lbs_add_card(void *card, struct device *dmdev)
 	priv->mesh_open = 0;
 	priv->infra_open = 0;
 
+	priv->fn_init_required = 0;
+	priv->fn_shutdown_required = 0;
+
 	/* Setup the OS Interface to our functions */
  	dev->netdev_ops = &lbs_netdev_ops;
 	dev->watchdog_timeo = 5 * HZ;
@@ -1373,11 +1384,20 @@ void lbs_stop_card(struct lbs_private *priv)
 	struct net_device *dev;
 	struct cmd_ctrl_node *cmdnode;
 	unsigned long flags;
+	struct cmd_header cmd;
 
 	lbs_deb_enter(LBS_DEB_MAIN);
 
 	if (!priv)
 		goto out;
+
+	if (priv->fn_shutdown_required) {
+		memset(&cmd, 0, sizeof(cmd));
+		if (__lbs_cmd(priv, CMD_FUNC_SHUTDOWN, &cmd, sizeof(cmd),
+				lbs_cmd_copyback, (unsigned long) &cmd))
+			lbs_pr_alert("CMD_FUNC_SHUTDOWN command failed\n");
+	}
+
 	dev = priv->dev;
 
 	netif_stop_queue(dev);
-- 
1.5.3.6

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

[Dan Williams]

On Tue, 2009-05-19 at 19:48 -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>
> ---
>  drivers/net/wireless/libertas/dev.h     |    2 +
>  drivers/net/wireless/libertas/host.h    |    2 +
>  drivers/net/wireless/libertas/if_sdio.c |   41 ++++++++++++++++++++++++++++--
>  drivers/net/wireless/libertas/main.c    |   20 +++++++++++++++
>  4 files changed, 62 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/wireless/libertas/dev.h b/drivers/net/wireless/libertas/dev.h
> index cbaafa6..a4455ec 100644
> --- a/drivers/net/wireless/libertas/dev.h
> +++ b/drivers/net/wireless/libertas/dev.h
> @@ -321,6 +321,8 @@ struct lbs_private {
>  
>  	u32 monitormode;
>  	u8 fw_ready;
> +	u8 fn_init_required;
> +	u8 fn_shutdown_required;
>  };
>  
>  extern struct cmd_confirm_sleep confirm_sleep;
> 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..478d766 100644
> --- a/drivers/net/wireless/libertas/if_sdio.c
> +++ b/drivers/net/wireless/libertas/if_sdio.c
> @@ -61,6 +61,7 @@ struct if_sdio_model {
>  	int model;
>  	const char *helper;
>  	const char *firmware;
> +	struct if_sdio_card *card;
>  };
>  
>  static struct if_sdio_model if_sdio_models[] = {
> @@ -69,18 +70,21 @@ static struct if_sdio_model if_sdio_models[] = {
>  		.model = IF_SDIO_MODEL_8385,
>  		.helper = "sd8385_helper.bin",
>  		.firmware = "sd8385.bin",
> +		.card = NULL,
>  	},
>  	{
>  		/* 8686 */
>  		.model = IF_SDIO_MODEL_8686,
>  		.helper = "sd8686_helper.bin",
>  		.firmware = "sd8686.bin",
> +		.card = NULL,
>  	},
>  	{
>  		/* 8688 */
>  		.model = IF_SDIO_MODEL_8688,
>  		.helper = "sd8688_helper.bin",
>  		.firmware = "sd8688.bin",
> +		.card = NULL,
>  	},
>  };
>  
> @@ -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);

What's the reason for setting the block size later?  Does it only need
to be set after the firmware download is complete?  Just not sure about
SDIO block sizes, not my area of expertise...

> 	ret = 0;
>  
>  out:
> @@ -903,6 +908,8 @@ static int if_sdio_probe(struct sdio_func *func,
>  		goto free;
>  	}
>  
> +	if_sdio_models[i].card = card;
> +

So I think this is problematic; the problem is that if we have two
libertas SDIO cards in the system (which could happen although not
likely) then this would overwrite the first card info.  Also seems
somewhat suspect to be writing into the structure which is otherwise
essentially 'const'.

> 	card->helper = if_sdio_models[i].helper;
>  	card->firmware = if_sdio_models[i].firmware;
>  
> @@ -985,6 +992,12 @@ static int if_sdio_probe(struct sdio_func *func,
>  	if (ret)
>  		goto reclaim;
>  
> +	/*
> +	 * FUNC_INIT is required for SD8688 WLAN/BT multiple functions
> +	 */
> +	priv->fn_init_required =
> +		(card->model == IF_SDIO_MODEL_8688) ? 1 : 0;
> +

I'd rather, I think, have generalized startup/shutdown hooks at the
interface level (like reset_card and hw_host_to_card) that the interface
can set to a function or to NULL, and call them that way.

> 
>  	ret = lbs_start_card(priv);
>  	if (ret)
>  		goto err_activate_card;
> @@ -1025,23 +1038,30 @@ static void if_sdio_remove(struct sdio_func *func)
>  {
>  	struct if_sdio_card *card;
>  	struct if_sdio_packet *packet;
> +	int ret;
>  
>  	lbs_deb_enter(LBS_DEB_SDIO);
>  
>  	card = sdio_get_drvdata(func);
>  
> +	lbs_stop_card(card->priv);

Seems odd, was this causing some problem when it was below
surpriseremoved?  I mention this becuase there's a lot of stuff during
the command processing paths that uses surpriseremoved to break out
early, and moving stop_card() above that may break stuff there.

> 	card->priv->surpriseremoved = 1;
>  
>  	lbs_deb_sdio("call remove card\n");
> -	lbs_stop_card(card->priv);
>  	lbs_remove_card(card->priv);
>  
>  	flush_workqueue(card->workqueue);
>  	destroy_workqueue(card->workqueue);
>  
>  	sdio_claim_host(func);
> +
> +	/* Disable interrupts */
> +	sdio_writeb(func, 0x00, IF_SDIO_H_INT_MASK, &ret);

What was the reason for this?  Was it a mistake/omission in the original
code or something?

> 	sdio_release_irq(func);
>  	sdio_disable_func(func);
> +
>  	sdio_release_host(func);
>  
>  	while (card->packets) {
> @@ -1084,8 +1104,23 @@ static int __init if_sdio_init_module(void)
>  
>  static void __exit if_sdio_exit_module(void)
>  {
> +	int i;
> +	struct if_sdio_card *card;
> +
>  	lbs_deb_enter(LBS_DEB_SDIO);
>  
> +	for (i = 0; i < ARRAY_SIZE(if_sdio_models); i++) {
> +		card = if_sdio_models[i].card;
> +
> +		/*
> +		 * FUNC_SHUTDOWN is required for SD8688 WLAN/BT
> +		 * multiple functions
> +		 */
> +		if (card && card->priv)
> +			card->priv->fn_shutdown_required =
> +				(card->model == IF_SDIO_MODEL_8688) ? 1 : 0;
> +	}
> +

So this seems pretty weird; I don't think we should be storing the card
value there.  Why does this have to be done from exit_module() instead
of stop_card?  If fn_init_required is TRUE, why wouldn't
fn_shutdown_required also be TRUE?

>  	sdio_unregister_driver(&if_sdio_driver);
>  
>  	lbs_deb_leave(LBS_DEB_SDIO);
> diff --git a/drivers/net/wireless/libertas/main.c b/drivers/net/wireless/libertas/main.c
> index 89575e4..a58a123 100644
> --- a/drivers/net/wireless/libertas/main.c
> +++ b/drivers/net/wireless/libertas/main.c
> @@ -1002,9 +1002,17 @@ static int lbs_setup_firmware(struct lbs_private *priv)
>  {
>  	int ret = -1;
>  	s16 curlevel = 0, minlevel = 0, maxlevel = 0;
> +	struct cmd_header cmd;
>  
>  	lbs_deb_enter(LBS_DEB_FW);
>  
> +	if (priv->fn_init_required) {
> +		memset(&cmd, 0, sizeof(cmd));
> +		if (__lbs_cmd(priv, CMD_FUNC_INIT, &cmd, sizeof(cmd),
> +				lbs_cmd_copyback, (unsigned long) &cmd))
> +			lbs_pr_alert("CMD_FUNC_INIT command failed\n");
> +	}
> +

I'd rather have this in the generic code in main.c as a callback into
the interface code from lbs_start_card(), or else since for the moment
it's SDIO-specific (will it also be used with GSPI or USB?) we could
just put this into if_sdio_probe().

>  	/* Read MAC address from firmware */
>  	memset(priv->current_addr, 0xff, ETH_ALEN);
>  	ret = lbs_update_hw_spec(priv);
> @@ -1192,6 +1200,9 @@ struct lbs_private *lbs_add_card(void *card, struct device *dmdev)
>  	priv->mesh_open = 0;
>  	priv->infra_open = 0;
>  
> +	priv->fn_init_required = 0;
> +	priv->fn_shutdown_required = 0;
> +
>  	/* Setup the OS Interface to our functions */
>   	dev->netdev_ops = &lbs_netdev_ops;
>  	dev->watchdog_timeo = 5 * HZ;
> @@ -1373,11 +1384,20 @@ void lbs_stop_card(struct lbs_private *priv)
>  	struct net_device *dev;
>  	struct cmd_ctrl_node *cmdnode;
>  	unsigned long flags;
> +	struct cmd_header cmd;
>  
>  	lbs_deb_enter(LBS_DEB_MAIN);
>  
>  	if (!priv)
>  		goto out;
> +
> +	if (priv->fn_shutdown_required) {
> +		memset(&cmd, 0, sizeof(cmd));
> +		if (__lbs_cmd(priv, CMD_FUNC_SHUTDOWN, &cmd, sizeof(cmd),
> +				lbs_cmd_copyback, (unsigned long) &cmd))
> +			lbs_pr_alert("CMD_FUNC_SHUTDOWN command failed\n");
> +	}

Again, if this is SDIO specific then lets put this into
if_sdio_remove(), if it's not SDIO specific (ie, will apply to USB or
GSPI variants) then lets do the interface-hook thing I mentioned
earlier.

Dan

>  	dev = priv->dev;
>  
>  	netif_stop_queue(dev);

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

[Bing Zhao]

Hi Dan,

Thanks for the review.

> -----Original Message-----
> From: Dan Williams [mailto:dcbw@redhat.com]
> Sent: Tuesday, May 26, 2009 1:28 PM
> To: Bing Zhao
> Cc: libertas-dev@lists.infradead.org; linux-wireless@vger.kernel.org
> Subject: Re: [PATCH 3/4] libertas: implement function init/shutdown commands for SD8688
> 
> On Tue, 2009-05-19 at 19:48 -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>
> > ---
> >  drivers/net/wireless/libertas/dev.h     |    2 +
> >  drivers/net/wireless/libertas/host.h    |    2 +
> >  drivers/net/wireless/libertas/if_sdio.c |   41 ++++++++++++++++++++++++++++--
> >  drivers/net/wireless/libertas/main.c    |   20 +++++++++++++++
> >  4 files changed, 62 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/wireless/libertas/dev.h b/drivers/net/wireless/libertas/dev.h
> > index cbaafa6..a4455ec 100644
> > --- a/drivers/net/wireless/libertas/dev.h
> > +++ b/drivers/net/wireless/libertas/dev.h
> > @@ -321,6 +321,8 @@ struct lbs_private {
> >
> >  	u32 monitormode;
> >  	u8 fw_ready;
> > +	u8 fn_init_required;
> > +	u8 fn_shutdown_required;
> >  };
> >
> >  extern struct cmd_confirm_sleep confirm_sleep;
> > 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..478d766 100644
> > --- a/drivers/net/wireless/libertas/if_sdio.c
> > +++ b/drivers/net/wireless/libertas/if_sdio.c
> > @@ -61,6 +61,7 @@ struct if_sdio_model {
> >  	int model;
> >  	const char *helper;
> >  	const char *firmware;
> > +	struct if_sdio_card *card;
> >  };
> >
> >  static struct if_sdio_model if_sdio_models[] = {
> > @@ -69,18 +70,21 @@ static struct if_sdio_model if_sdio_models[] = {
> >  		.model = IF_SDIO_MODEL_8385,
> >  		.helper = "sd8385_helper.bin",
> >  		.firmware = "sd8385.bin",
> > +		.card = NULL,
> >  	},
> >  	{
> >  		/* 8686 */
> >  		.model = IF_SDIO_MODEL_8686,
> >  		.helper = "sd8686_helper.bin",
> >  		.firmware = "sd8686.bin",
> > +		.card = NULL,
> >  	},
> >  	{
> >  		/* 8688 */
> >  		.model = IF_SDIO_MODEL_8688,
> >  		.helper = "sd8688_helper.bin",
> >  		.firmware = "sd8688.bin",
> > +		.card = NULL,
> >  	},
> >  };
> >
> > @@ -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);
> 
> What's the reason for setting the block size later?  Does it only need
> to be set after the firmware download is complete?  Just not sure about
> SDIO block sizes, not my area of expertise...
> 

In case that the firmware is already downloaded, we jump to "success" without re-downloading helper and firmware images:

        if (scratch == IF_SDIO_FIRMWARE_OK) {
                lbs_deb_sdio("firmware already loaded\n");
                goto success;
        }

In this case we never call sdio_set_block_size API to configure the block size to 256 (IF_SDIO_BLOCK_SIZE). So the default block size defined in CIS table will be used by MMC/SDIO driver. This is OK for SD8385/SD8686 because the default block size read from CIS happens to be 256. But for SD8688, the CIS block size is 512. If we take this default block size 512 we will have to transfer 4 blocks data (4x512=2048 bytes) for a regular data packet (15xx bytes + headers) from host to device. Due to a buffer limitation in firmware, this will corrupt the firmware.

When block size is set to 256, we transfer 7 blocks (or 7x256=1792 bytes) data to firmware for the same packet.

> > 	ret = 0;
> >
> >  out:
> > @@ -903,6 +908,8 @@ static int if_sdio_probe(struct sdio_func *func,
> >  		goto free;
> >  	}
> >
> > +	if_sdio_models[i].card = card;
> > +
> 
> So I think this is problematic; the problem is that if we have two
> libertas SDIO cards in the system (which could happen although not
> likely) then this would overwrite the first card info.  Also seems
> somewhat suspect to be writing into the structure which is otherwise
> essentially 'const'.
> 

I added this "card" variable to the if_sdio_model structure in order to save the card pointer because I need the "card" or "priv" pointer to issue function SHUTDOWN command. The SHUTDOWN command is only needed if the libertas_sdio module is removed by user (rmmod libertas_sdio). As the exit_module() does not take any parameter, I have to save the card pointer somewhere. I chose to add this "card" variable in if_sdio_model structure to avoid adding new global variable. Sure, the issues raised by multiple cards must be taken into account.

> > 	card->helper = if_sdio_models[i].helper;
> >  	card->firmware = if_sdio_models[i].firmware;
> >
> > @@ -985,6 +992,12 @@ static int if_sdio_probe(struct sdio_func *func,
> >  	if (ret)
> >  		goto reclaim;
> >
> > +	/*
> > +	 * FUNC_INIT is required for SD8688 WLAN/BT multiple functions
> > +	 */
> > +	priv->fn_init_required =
> > +		(card->model == IF_SDIO_MODEL_8688) ? 1 : 0;
> > +
> 
> I'd rather, I think, have generalized startup/shutdown hooks at the
> interface level (like reset_card and hw_host_to_card) that the interface
> can set to a function or to NULL, and call them that way.
> 

Will do FUNC_INIT command directly in if_sdio_probe() as it's SDIO specific.

> >
> >  	ret = lbs_start_card(priv);
> >  	if (ret)
> >  		goto err_activate_card;
> > @@ -1025,23 +1038,30 @@ static void if_sdio_remove(struct sdio_func *func)
> >  {
> >  	struct if_sdio_card *card;
> >  	struct if_sdio_packet *packet;
> > +	int ret;
> >
> >  	lbs_deb_enter(LBS_DEB_SDIO);
> >
> >  	card = sdio_get_drvdata(func);
> >
> > +	lbs_stop_card(card->priv);
> 
> Seems odd, was this causing some problem when it was below
> surpriseremoved?  I mention this becuase there's a lot of stuff during
> the command processing paths that uses surpriseremoved to break out
> early, and moving stop_card() above that may break stuff there.
> 

I wanted to send FUNC_SHUTDOWN command in lbs_stop_card() but "surpriseremoved" blocks sending any command. Have to find another way to bypass this check for SHUTDOWN command.

> > 	card->priv->surpriseremoved = 1;
> >
> >  	lbs_deb_sdio("call remove card\n");
> > -	lbs_stop_card(card->priv);
> >  	lbs_remove_card(card->priv);
> >
> >  	flush_workqueue(card->workqueue);
> >  	destroy_workqueue(card->workqueue);
> >
> >  	sdio_claim_host(func);
> > +
> > +	/* Disable interrupts */
> > +	sdio_writeb(func, 0x00, IF_SDIO_H_INT_MASK, &ret);
> 
> What was the reason for this?  Was it a mistake/omission in the original
> code or something?
> 

I wanted to play it safe when user unloads the libertas_sdio module (using rmmod command). It seems not necessary however. It will be reverted.

> > 	sdio_release_irq(func);
> >  	sdio_disable_func(func);
> > +
> >  	sdio_release_host(func);
> >
> >  	while (card->packets) {
> > @@ -1084,8 +1104,23 @@ static int __init if_sdio_init_module(void)
> >
> >  static void __exit if_sdio_exit_module(void)
> >  {
> > +	int i;
> > +	struct if_sdio_card *card;
> > +
> >  	lbs_deb_enter(LBS_DEB_SDIO);
> >
> > +	for (i = 0; i < ARRAY_SIZE(if_sdio_models); i++) {
> > +		card = if_sdio_models[i].card;
> > +
> > +		/*
> > +		 * FUNC_SHUTDOWN is required for SD8688 WLAN/BT
> > +		 * multiple functions
> > +		 */
> > +		if (card && card->priv)
> > +			card->priv->fn_shutdown_required =
> > +				(card->model == IF_SDIO_MODEL_8688) ? 1 : 0;
> > +	}
> > +
> 
> So this seems pretty weird; I don't think we should be storing the card
> value there.  Why does this have to be done from exit_module() instead
> of stop_card?  If fn_init_required is TRUE, why wouldn't
> fn_shutdown_required also be TRUE?
> 

The SHUTDOWN command is sent only if the user removes the module (rmmod libertas_sdio). It should not be sent if the user simply ejects the card from the slot. The fn_init_required flag is always TRUE. The fn_shutdown_required flag is TRUE when libertas_sdio module is removed (card still in slot); or FALSE if the card is unplugged. Anyway, both flags will be removed in the updated patch.

Previously I thought about using a global variable to solve this problem. A flag (user_rmmod) is set to 1 in exit_module(), and then the flag is checked in if_sdio_remove() to determine if SHUTDOWN command should be issued or not. But, is a global variable allowed?

> >  	sdio_unregister_driver(&if_sdio_driver);
> >
> >  	lbs_deb_leave(LBS_DEB_SDIO);
> > diff --git a/drivers/net/wireless/libertas/main.c b/drivers/net/wireless/libertas/main.c
> > index 89575e4..a58a123 100644
> > --- a/drivers/net/wireless/libertas/main.c
> > +++ b/drivers/net/wireless/libertas/main.c
> > @@ -1002,9 +1002,17 @@ static int lbs_setup_firmware(struct lbs_private *priv)
> >  {
> >  	int ret = -1;
> >  	s16 curlevel = 0, minlevel = 0, maxlevel = 0;
> > +	struct cmd_header cmd;
> >
> >  	lbs_deb_enter(LBS_DEB_FW);
> >
> > +	if (priv->fn_init_required) {
> > +		memset(&cmd, 0, sizeof(cmd));
> > +		if (__lbs_cmd(priv, CMD_FUNC_INIT, &cmd, sizeof(cmd),
> > +				lbs_cmd_copyback, (unsigned long) &cmd))
> > +			lbs_pr_alert("CMD_FUNC_INIT command failed\n");
> > +	}
> > +
> 
> I'd rather have this in the generic code in main.c as a callback into
> the interface code from lbs_start_card(), or else since for the moment
> it's SDIO-specific (will it also be used with GSPI or USB?) we could
> just put this into if_sdio_probe().
> 

It will be moved to if_sdio_probe() as it's for SDIO only.

> >  	/* Read MAC address from firmware */
> >  	memset(priv->current_addr, 0xff, ETH_ALEN);
> >  	ret = lbs_update_hw_spec(priv);
> > @@ -1192,6 +1200,9 @@ struct lbs_private *lbs_add_card(void *card, struct device *dmdev)
> >  	priv->mesh_open = 0;
> >  	priv->infra_open = 0;
> >
> > +	priv->fn_init_required = 0;
> > +	priv->fn_shutdown_required = 0;
> > +
> >  	/* Setup the OS Interface to our functions */
> >   	dev->netdev_ops = &lbs_netdev_ops;
> >  	dev->watchdog_timeo = 5 * HZ;
> > @@ -1373,11 +1384,20 @@ void lbs_stop_card(struct lbs_private *priv)
> >  	struct net_device *dev;
> >  	struct cmd_ctrl_node *cmdnode;
> >  	unsigned long flags;
> > +	struct cmd_header cmd;
> >
> >  	lbs_deb_enter(LBS_DEB_MAIN);
> >
> >  	if (!priv)
> >  		goto out;
> > +
> > +	if (priv->fn_shutdown_required) {
> > +		memset(&cmd, 0, sizeof(cmd));
> > +		if (__lbs_cmd(priv, CMD_FUNC_SHUTDOWN, &cmd, sizeof(cmd),
> > +				lbs_cmd_copyback, (unsigned long) &cmd))
> > +			lbs_pr_alert("CMD_FUNC_SHUTDOWN command failed\n");
> > +	}
> 
> Again, if this is SDIO specific then lets put this into
> if_sdio_remove(), if it's not SDIO specific (ie, will apply to USB or
> GSPI variants) then lets do the interface-hook thing I mentioned
> earlier.
> 

It will be moved to if_sdio interface driver because it is SDIO specific.
If a global variable (the flag for user_rmmod) can be added, the command can be issued in if_sdio_remove() and no need to add "card" variable in if_sdio_model structure.

> Dan
> 
> >  	dev = priv->dev;
> >
> >  	netif_stop_queue(dev);


Thanks,

Bing

[PATCH 4/4] libertas: read SD8688 firmware status from new register

[Bing Zhao]

The scratch pad register is used to store firmware status after
firmware is downloaded and initialized. After firmware status is
verified OK, the same register is used to store RX packet length.
Hence the firmware status code is no longer valid afterwards.

SD8688 firmware introduces a new register for firmware status
which will never be overwritten.

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

diff --git a/drivers/net/wireless/libertas/if_sdio.c b/drivers/net/wireless/libertas/if_sdio.c
index 478d766..18132d4 100644
--- a/drivers/net/wireless/libertas/if_sdio.c
+++ b/drivers/net/wireless/libertas/if_sdio.c
@@ -119,15 +119,29 @@ struct if_sdio_card {
 /* I/O                                                              */
 /********************************************************************/
 
+/*
+ *  For SD8385/SD8686, this function reads firmware status after
+ *  the image is downloaded, or reads RX packet length when
+ *  interrupt (with IF_SDIO_H_INT_UPLD bit set) is received.
+ *  For SD8688, this function reads firmware status only.
+ */
 static u16 if_sdio_read_scratch(struct if_sdio_card *card, int *err)
 {
 	int ret, reg;
 	u16 scratch;
 
-	if (card->model == IF_SDIO_MODEL_8385)
+	switch (card->model) {
+	case IF_SDIO_MODEL_8385:
 		reg = IF_SDIO_SCRATCH_OLD;
-	else
+		break;
+	case IF_SDIO_MODEL_8686:
 		reg = IF_SDIO_SCRATCH;
+		break;
+	case IF_SDIO_MODEL_8688:
+	default: /* for newer chipsets */
+		reg = IF_SDIO_FW_STATUS;
+		break;
+	}
 
 	scratch = sdio_readb(card->func, reg, &ret);
 	if (!ret)
@@ -706,6 +720,8 @@ static int if_sdio_prog_firmware(struct if_sdio_card *card)
 	if (ret)
 		goto out;
 
+	lbs_deb_sdio("firmware status = %#x\n", scratch);
+
 	if (scratch == IF_SDIO_FIRMWARE_OK) {
 		lbs_deb_sdio("firmware already loaded\n");
 		goto success;
diff --git a/drivers/net/wireless/libertas/if_sdio.h b/drivers/net/wireless/libertas/if_sdio.h
index d3a4fbe..60c9b2f 100644
--- a/drivers/net/wireless/libertas/if_sdio.h
+++ b/drivers/net/wireless/libertas/if_sdio.h
@@ -42,6 +42,7 @@
 
 #define IF_SDIO_SCRATCH		0x34
 #define IF_SDIO_SCRATCH_OLD	0x80fe
+#define IF_SDIO_FW_STATUS	0x40
 #define   IF_SDIO_FIRMWARE_OK	0xfedc
 
 #define IF_SDIO_RX_LEN		0x42
-- 
1.5.3.6

Re: [PATCH 4/4] libertas: read SD8688 firmware status from new register

[Dan Williams]

On Tue, 2009-05-19 at 19:48 -0700, Bing Zhao wrote:
> The scratch pad register is used to store firmware status after
> firmware is downloaded and initialized. After firmware status is
> verified OK, the same register is used to store RX packet length.
> Hence the firmware status code is no longer valid afterwards.
> 
> SD8688 firmware introduces a new register for firmware status
> which will never be overwritten.

This could become confusing :)  read_rx_len() already has a switch
statement, but since read_rx_len() can also call into read_scratch() for
the 8385 and 8686, that *also* has a switch selector.  I can't think of
a great way to break it up though without copying a chunk of code around
or making useless wrapper functions.

Acked-by: Dan Williams <dcbw@redhat.com>

Dan

> Signed-off-by: Bing Zhao <bzhao@marvell.com>
> ---
>  drivers/net/wireless/libertas/if_sdio.c |   20 ++++++++++++++++++--
>  drivers/net/wireless/libertas/if_sdio.h |    1 +
>  2 files changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/libertas/if_sdio.c b/drivers/net/wireless/libertas/if_sdio.c
> index 478d766..18132d4 100644
> --- a/drivers/net/wireless/libertas/if_sdio.c
> +++ b/drivers/net/wireless/libertas/if_sdio.c
> @@ -119,15 +119,29 @@ struct if_sdio_card {
>  /* I/O                                                              */
>  /********************************************************************/
>  
> +/*
> + *  For SD8385/SD8686, this function reads firmware status after
> + *  the image is downloaded, or reads RX packet length when
> + *  interrupt (with IF_SDIO_H_INT_UPLD bit set) is received.
> + *  For SD8688, this function reads firmware status only.
> + */
>  static u16 if_sdio_read_scratch(struct if_sdio_card *card, int *err)
>  {
>  	int ret, reg;
>  	u16 scratch;
>  
> -	if (card->model == IF_SDIO_MODEL_8385)
> +	switch (card->model) {
> +	case IF_SDIO_MODEL_8385:
>  		reg = IF_SDIO_SCRATCH_OLD;
> -	else
> +		break;
> +	case IF_SDIO_MODEL_8686:
>  		reg = IF_SDIO_SCRATCH;
> +		break;
> +	case IF_SDIO_MODEL_8688:
> +	default: /* for newer chipsets */
> +		reg = IF_SDIO_FW_STATUS;
> +		break;
> +	}
>  
>  	scratch = sdio_readb(card->func, reg, &ret);
>  	if (!ret)
> @@ -706,6 +720,8 @@ static int if_sdio_prog_firmware(struct if_sdio_card *card)
>  	if (ret)
>  		goto out;
>  
> +	lbs_deb_sdio("firmware status = %#x\n", scratch);
> +
>  	if (scratch == IF_SDIO_FIRMWARE_OK) {
>  		lbs_deb_sdio("firmware already loaded\n");
>  		goto success;
> diff --git a/drivers/net/wireless/libertas/if_sdio.h b/drivers/net/wireless/libertas/if_sdio.h
> index d3a4fbe..60c9b2f 100644
> --- a/drivers/net/wireless/libertas/if_sdio.h
> +++ b/drivers/net/wireless/libertas/if_sdio.h
> @@ -42,6 +42,7 @@
>  
>  #define IF_SDIO_SCRATCH		0x34
>  #define IF_SDIO_SCRATCH_OLD	0x80fe
> +#define IF_SDIO_FW_STATUS	0x40
>  #define   IF_SDIO_FIRMWARE_OK	0xfedc
>  
>  #define IF_SDIO_RX_LEN		0x42

RE: [PATCH 4/4] libertas: read SD8688 firmware status from new register

[Bing Zhao]

Hi Dan,

> -----Original Message-----
> From: Dan Williams [mailto:dcbw@redhat.com]
> Sent: Wednesday, May 20, 2009 3:46 PM
> To: Bing Zhao
> Cc: libertas-dev@lists.infradead.org; linux-wireless@vger.kernel.org
> Subject: Re: [PATCH 4/4] libertas: read SD8688 firmware status from new register
> 
> On Tue, 2009-05-19 at 19:48 -0700, Bing Zhao wrote:
> > The scratch pad register is used to store firmware status after
> > firmware is downloaded and initialized. After firmware status is
> > verified OK, the same register is used to store RX packet length.
> > Hence the firmware status code is no longer valid afterwards.
> >
> > SD8688 firmware introduces a new register for firmware status
> > which will never be overwritten.
> 
> This could become confusing :)  read_rx_len() already has a switch
> statement, but since read_rx_len() can also call into read_scratch() for
> the 8385 and 8686, that *also* has a switch selector.  I can't think of
> a great way to break it up though without copying a chunk of code around
> or making useless wrapper functions.
> 

Do you think this change makes things better?

diff --git a/drivers/net/wireless/libertas/if_sdio.c b/drivers/net/wireless/libertas/if_sdio.c
index 18132d4..a7e3fc1 100644
--- a/drivers/net/wireless/libertas/if_sdio.c
+++ b/drivers/net/wireless/libertas/if_sdio.c
@@ -100,6 +100,7 @@ struct if_sdio_card {
 
 	int			model;
 	unsigned long		ioport;
+	unsigned int		scratch_reg;
 
 	const char		*helper;
 	const char		*firmware;
@@ -127,25 +128,13 @@ struct if_sdio_card {
  */
 static u16 if_sdio_read_scratch(struct if_sdio_card *card, int *err)
 {
-	int ret, reg;
+	int ret;
 	u16 scratch;
 
-	switch (card->model) {
-	case IF_SDIO_MODEL_8385:
-		reg = IF_SDIO_SCRATCH_OLD;
-		break;
-	case IF_SDIO_MODEL_8686:
-		reg = IF_SDIO_SCRATCH;
-		break;
-	case IF_SDIO_MODEL_8688:
-	default: /* for newer chipsets */
-		reg = IF_SDIO_FW_STATUS;
-		break;
-	}
-
-	scratch = sdio_readb(card->func, reg, &ret);
+	scratch = sdio_readb(card->func, card->scratch_reg, &ret);
 	if (!ret)
-		scratch |= sdio_readb(card->func, reg + 1, &ret) << 8;
+		scratch |= sdio_readb(card->func, card->scratch_reg + 1,
+					&ret) << 8;
 
 	if (err)
 		*err = ret;
@@ -909,6 +898,20 @@ static int if_sdio_probe(struct sdio_func *func,
 
 	card->func = func;
 	card->model = model;
+
+	switch (card->model) {
+	case IF_SDIO_MODEL_8385:
+		card->scratch_reg = IF_SDIO_SCRATCH_OLD;
+		break;
+	case IF_SDIO_MODEL_8686:
+		card->scratch_reg = IF_SDIO_SCRATCH;
+		break;
+	case IF_SDIO_MODEL_8688:
+	default: /* for newer chipsets */
+		card->scratch_reg = IF_SDIO_FW_STATUS;
+		break;
+	}
+
 	spin_lock_init(&card->lock);
 	card->workqueue = create_workqueue("libertas_sdio");
 	INIT_WORK(&card->packet_worker, if_sdio_host_to_card_worker);



Thanks,

Bing


> Acked-by: Dan Williams <dcbw@redhat.com>
> 
> Dan
> 
> > Signed-off-by: Bing Zhao <bzhao@marvell.com>
> > ---
> >  drivers/net/wireless/libertas/if_sdio.c |   20 ++++++++++++++++++--
> >  drivers/net/wireless/libertas/if_sdio.h |    1 +
> >  2 files changed, 19 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/wireless/libertas/if_sdio.c b/drivers/net/wireless/libertas/if_sdio.c
> > index 478d766..18132d4 100644
> > --- a/drivers/net/wireless/libertas/if_sdio.c
> > +++ b/drivers/net/wireless/libertas/if_sdio.c
> > @@ -119,15 +119,29 @@ struct if_sdio_card {
> >  /* I/O                                                              */
> >  /********************************************************************/
> >
> > +/*
> > + *  For SD8385/SD8686, this function reads firmware status after
> > + *  the image is downloaded, or reads RX packet length when
> > + *  interrupt (with IF_SDIO_H_INT_UPLD bit set) is received.
> > + *  For SD8688, this function reads firmware status only.
> > + */
> >  static u16 if_sdio_read_scratch(struct if_sdio_card *card, int *err)
> >  {
> >  	int ret, reg;
> >  	u16 scratch;
> >
> > -	if (card->model == IF_SDIO_MODEL_8385)
> > +	switch (card->model) {
> > +	case IF_SDIO_MODEL_8385:
> >  		reg = IF_SDIO_SCRATCH_OLD;
> > -	else
> > +		break;
> > +	case IF_SDIO_MODEL_8686:
> >  		reg = IF_SDIO_SCRATCH;
> > +		break;
> > +	case IF_SDIO_MODEL_8688:
> > +	default: /* for newer chipsets */
> > +		reg = IF_SDIO_FW_STATUS;
> > +		break;
> > +	}
> >
> >  	scratch = sdio_readb(card->func, reg, &ret);
> >  	if (!ret)
> > @@ -706,6 +720,8 @@ static int if_sdio_prog_firmware(struct if_sdio_card *card)
> >  	if (ret)
> >  		goto out;
> >
> > +	lbs_deb_sdio("firmware status = %#x\n", scratch);
> > +
> >  	if (scratch == IF_SDIO_FIRMWARE_OK) {
> >  		lbs_deb_sdio("firmware already loaded\n");
> >  		goto success;
> > diff --git a/drivers/net/wireless/libertas/if_sdio.h b/drivers/net/wireless/libertas/if_sdio.h
> > index d3a4fbe..60c9b2f 100644
> > --- a/drivers/net/wireless/libertas/if_sdio.h
> > +++ b/drivers/net/wireless/libertas/if_sdio.h
> > @@ -42,6 +42,7 @@
> >
> >  #define IF_SDIO_SCRATCH		0x34
> >  #define IF_SDIO_SCRATCH_OLD	0x80fe
> > +#define IF_SDIO_FW_STATUS	0x40
> >  #define   IF_SDIO_FIRMWARE_OK	0xfedc
> >
> >  #define IF_SDIO_RX_LEN		0x42

RE: [PATCH 4/4] libertas: read SD8688 firmware status from new register

[Dan Williams]

On Wed, 2009-05-20 at 16:50 -0700, Bing Zhao wrote:
> Hi Dan,
> 
> > -----Original Message-----
> > From: Dan Williams [mailto:dcbw@redhat.com]
> > Sent: Wednesday, May 20, 2009 3:46 PM
> > To: Bing Zhao
> > Cc: libertas-dev@lists.infradead.org; linux-wireless@vger.kernel.org
> > Subject: Re: [PATCH 4/4] libertas: read SD8688 firmware status from new register
> > 
> > On Tue, 2009-05-19 at 19:48 -0700, Bing Zhao wrote:
> > > The scratch pad register is used to store firmware status after
> > > firmware is downloaded and initialized. After firmware status is
> > > verified OK, the same register is used to store RX packet length.
> > > Hence the firmware status code is no longer valid afterwards.
> > >
> > > SD8688 firmware introduces a new register for firmware status
> > > which will never be overwritten.
> > 
> > This could become confusing :)  read_rx_len() already has a switch
> > statement, but since read_rx_len() can also call into read_scratch() for
> > the 8385 and 8686, that *also* has a switch selector.  I can't think of
> > a great way to break it up though without copying a chunk of code around
> > or making useless wrapper functions.
> > 
> 
> Do you think this change makes things better?

Yeah, that's better.  Also saves some processing time because the
scratch reg doesn't have to be set each time a frame comes in.

Want to resubmit with the Signed-off-by and description just for fun? :)

Dan

> diff --git a/drivers/net/wireless/libertas/if_sdio.c b/drivers/net/wireless/libertas/if_sdio.c
> index 18132d4..a7e3fc1 100644
> --- a/drivers/net/wireless/libertas/if_sdio.c
> +++ b/drivers/net/wireless/libertas/if_sdio.c
> @@ -100,6 +100,7 @@ struct if_sdio_card {
>  
>  	int			model;
>  	unsigned long		ioport;
> +	unsigned int		scratch_reg;
>  
>  	const char		*helper;
>  	const char		*firmware;
> @@ -127,25 +128,13 @@ struct if_sdio_card {
>   */
>  static u16 if_sdio_read_scratch(struct if_sdio_card *card, int *err)
>  {
> -	int ret, reg;
> +	int ret;
>  	u16 scratch;
>  
> -	switch (card->model) {
> -	case IF_SDIO_MODEL_8385:
> -		reg = IF_SDIO_SCRATCH_OLD;
> -		break;
> -	case IF_SDIO_MODEL_8686:
> -		reg = IF_SDIO_SCRATCH;
> -		break;
> -	case IF_SDIO_MODEL_8688:
> -	default: /* for newer chipsets */
> -		reg = IF_SDIO_FW_STATUS;
> -		break;
> -	}
> -
> -	scratch = sdio_readb(card->func, reg, &ret);
> +	scratch = sdio_readb(card->func, card->scratch_reg, &ret);
>  	if (!ret)
> -		scratch |= sdio_readb(card->func, reg + 1, &ret) << 8;
> +		scratch |= sdio_readb(card->func, card->scratch_reg + 1,
> +					&ret) << 8;
>  
>  	if (err)
>  		*err = ret;
> @@ -909,6 +898,20 @@ static int if_sdio_probe(struct sdio_func *func,
>  
>  	card->func = func;
>  	card->model = model;
> +
> +	switch (card->model) {
> +	case IF_SDIO_MODEL_8385:
> +		card->scratch_reg = IF_SDIO_SCRATCH_OLD;
> +		break;
> +	case IF_SDIO_MODEL_8686:
> +		card->scratch_reg = IF_SDIO_SCRATCH;
> +		break;
> +	case IF_SDIO_MODEL_8688:
> +	default: /* for newer chipsets */
> +		card->scratch_reg = IF_SDIO_FW_STATUS;
> +		break;
> +	}
> +
>  	spin_lock_init(&card->lock);
>  	card->workqueue = create_workqueue("libertas_sdio");
>  	INIT_WORK(&card->packet_worker, if_sdio_host_to_card_worker);
> 
> 
> 
> Thanks,
> 
> Bing
> 
> 
> > Acked-by: Dan Williams <dcbw@redhat.com>
> > 
> > Dan
> > 
> > > Signed-off-by: Bing Zhao <bzhao@marvell.com>
> > > ---
> > >  drivers/net/wireless/libertas/if_sdio.c |   20 ++++++++++++++++++--
> > >  drivers/net/wireless/libertas/if_sdio.h |    1 +
> > >  2 files changed, 19 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/net/wireless/libertas/if_sdio.c b/drivers/net/wireless/libertas/if_sdio.c
> > > index 478d766..18132d4 100644
> > > --- a/drivers/net/wireless/libertas/if_sdio.c
> > > +++ b/drivers/net/wireless/libertas/if_sdio.c
> > > @@ -119,15 +119,29 @@ struct if_sdio_card {
> > >  /* I/O                                                              */
> > >  /********************************************************************/
> > >
> > > +/*
> > > + *  For SD8385/SD8686, this function reads firmware status after
> > > + *  the image is downloaded, or reads RX packet length when
> > > + *  interrupt (with IF_SDIO_H_INT_UPLD bit set) is received.
> > > + *  For SD8688, this function reads firmware status only.
> > > + */
> > >  static u16 if_sdio_read_scratch(struct if_sdio_card *card, int *err)
> > >  {
> > >  	int ret, reg;
> > >  	u16 scratch;
> > >
> > > -	if (card->model == IF_SDIO_MODEL_8385)
> > > +	switch (card->model) {
> > > +	case IF_SDIO_MODEL_8385:
> > >  		reg = IF_SDIO_SCRATCH_OLD;
> > > -	else
> > > +		break;
> > > +	case IF_SDIO_MODEL_8686:
> > >  		reg = IF_SDIO_SCRATCH;
> > > +		break;
> > > +	case IF_SDIO_MODEL_8688:
> > > +	default: /* for newer chipsets */
> > > +		reg = IF_SDIO_FW_STATUS;
> > > +		break;
> > > +	}
> > >
> > >  	scratch = sdio_readb(card->func, reg, &ret);
> > >  	if (!ret)
> > > @@ -706,6 +720,8 @@ static int if_sdio_prog_firmware(struct if_sdio_card *card)
> > >  	if (ret)
> > >  		goto out;
> > >
> > > +	lbs_deb_sdio("firmware status = %#x\n", scratch);
> > > +
> > >  	if (scratch == IF_SDIO_FIRMWARE_OK) {
> > >  		lbs_deb_sdio("firmware already loaded\n");
> > >  		goto success;
> > > diff --git a/drivers/net/wireless/libertas/if_sdio.h b/drivers/net/wireless/libertas/if_sdio.h
> > > index d3a4fbe..60c9b2f 100644
> > > --- a/drivers/net/wireless/libertas/if_sdio.h
> > > +++ b/drivers/net/wireless/libertas/if_sdio.h
> > > @@ -42,6 +42,7 @@
> > >
> > >  #define IF_SDIO_SCRATCH		0x34
> > >  #define IF_SDIO_SCRATCH_OLD	0x80fe
> > > +#define IF_SDIO_FW_STATUS	0x40
> > >  #define   IF_SDIO_FIRMWARE_OK	0xfedc
> > >
> > >  #define IF_SDIO_RX_LEN		0x42
> 
> 
> _______________________________________________
> libertas-dev mailing list
> libertas-dev@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/libertas-dev

Re: [PATCH 1/4] libertas: define macros for SDIO model numbers

[Dan Williams]

On Tue, 2009-05-19 at 19:48 -0700, Bing Zhao wrote:
> replace direct usages of SDIO model numbers with defined macros.
> 
> Signed-off-by: Bing Zhao <bzhao@marvell.com>

Acked-by: Dan Williams <dcbw@redhat.com>

> ---
>  drivers/net/wireless/libertas/if_sdio.c |   18 +++++++++---------
>  drivers/net/wireless/libertas/if_sdio.h |    4 ++++
>  2 files changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/wireless/libertas/if_sdio.c b/drivers/net/wireless/libertas/if_sdio.c
> index 55864c1..69d7fd3 100644
> --- a/drivers/net/wireless/libertas/if_sdio.c
> +++ b/drivers/net/wireless/libertas/if_sdio.c
> @@ -66,19 +66,19 @@ struct if_sdio_model {
>  static struct if_sdio_model if_sdio_models[] = {
>  	{
>  		/* 8385 */
> -		.model = 0x04,
> +		.model = IF_SDIO_MODEL_8385,
>  		.helper = "sd8385_helper.bin",
>  		.firmware = "sd8385.bin",
>  	},
>  	{
>  		/* 8686 */
> -		.model = 0x0B,
> +		.model = IF_SDIO_MODEL_8686,
>  		.helper = "sd8686_helper.bin",
>  		.firmware = "sd8686.bin",
>  	},
>  	{
>  		/* 8688 */
> -		.model = 0x10,
> +		.model = IF_SDIO_MODEL_8688,
>  		.helper = "sd8688_helper.bin",
>  		.firmware = "sd8688.bin",
>  	},
> @@ -118,7 +118,7 @@ static u16 if_sdio_read_scratch(struct if_sdio_card *card, int *err)
>  	int ret, reg;
>  	u16 scratch;
>  
> -	if (card->model == 0x04)
> +	if (card->model == IF_SDIO_MODEL_8385)
>  		reg = IF_SDIO_SCRATCH_OLD;
>  	else
>  		reg = IF_SDIO_SCRATCH;
> @@ -216,7 +216,7 @@ static int if_sdio_handle_event(struct if_sdio_card *card,
>  
>  	lbs_deb_enter(LBS_DEB_SDIO);
>  
> -	if (card->model == 0x04) {
> +	if (card->model == IF_SDIO_MODEL_8385) {
>  		event = sdio_readb(card->func, IF_SDIO_EVENT, &ret);
>  		if (ret)
>  			goto out;
> @@ -829,10 +829,10 @@ static int if_sdio_probe(struct sdio_func *func,
>  		if (sscanf(func->card->info[i],
>  				"ID: %x", &model) == 1)
>  			break;
> -               if (!strcmp(func->card->info[i], "IBIS Wireless SDIO Card")) {
> -                       model = 4;
> -                       break;
> -               }
> +		if (!strcmp(func->card->info[i], "IBIS Wireless SDIO Card")) {
> +			model = IF_SDIO_MODEL_8385;
> +			break;
> +		}
>  	}
>  
>  	if (i == func->card->num_info) {
> diff --git a/drivers/net/wireless/libertas/if_sdio.h b/drivers/net/wireless/libertas/if_sdio.h
> index 37ada2c..cea343f 100644
> --- a/drivers/net/wireless/libertas/if_sdio.h
> +++ b/drivers/net/wireless/libertas/if_sdio.h
> @@ -12,6 +12,10 @@
>  #ifndef _LBS_IF_SDIO_H
>  #define _LBS_IF_SDIO_H
>  
> +#define IF_SDIO_MODEL_8385	0x04
> +#define IF_SDIO_MODEL_8686	0x0b
> +#define IF_SDIO_MODEL_8688	0x10
> +
>  #define IF_SDIO_IOPORT		0x00
>  
>  #define IF_SDIO_H_INT_MASK	0x04