Re: [PATCH v2 06/18] wl1271: AP mode - init sequence

[Luciano Coelho <luciano.coelho@nokia.com>]

On Wed, 2010-12-22 at 16:27 +0200, ext Arik Nemtsov wrote:
> Split HW init sequence into AP/STA specific parts
> 
> The AP specific init sequence includes configuration of templates, rate
> classes, power mode, etc. Also unmask AP specific events in the event mbox.
> 
> Separate the differences between AP and STA init into mode
> specific functions called from wl1271_hw_init. The first is called after
> radio configuration and the second after memory configuration.
> 
> Signed-off-by: Arik Nemtsov <arik@wizery.com>
> ---

[...]

> diff --git a/drivers/net/wireless/wl12xx/cmd.h b/drivers/net/wireless/wl12xx/cmd.h
> index 02f843c..d51324d 100644
> --- a/drivers/net/wireless/wl12xx/cmd.h
> +++ b/drivers/net/wireless/wl12xx/cmd.h
> @@ -140,6 +140,13 @@ enum cmd_templ {
>  				  * For CTS-to-self (FastCTS) mechanism
>  				  * for BT/WLAN coexistence (SoftGemini). */
>  	CMD_TEMPL_ARP_RSP,
> +
> +	/* AP-mode specific */
> +	CMD_TEMPL_AP_BEACON = 13,
> +	CMD_TEMPL_AP_PROBE_RESPONSE,
> +	CMD_TEMPL_AP_ARP_RSP,
> +	CMD_TEMPL_DEAUTH_AP,
> +
>  	CMD_TEMPL_MAX = 0xff
>  };

Cool, I see that you fixed the issue we had before.  Just one question,
is the CMD_TEMPL_AP_ARP_RSP the same as the original CMD_TEMPL_ARP_RSP?
I mean is it only the order in the enum that is different between the AP
and the STA firmwares? If that's the case, we should *really* try to get
this fixed in the AP firmware.

It's still new enough and not so many people are using it yet, right? So
it's the time to fix it if it makes sense.

 
> diff --git a/drivers/net/wireless/wl12xx/init.c b/drivers/net/wireless/wl12xx/init.c
> index 799c369..df6666b 100644
> --- a/drivers/net/wireless/wl12xx/init.c
> +++ b/drivers/net/wireless/wl12xx/init.c

[...]

> @@ -145,10 +253,6 @@ int wl1271_init_phy_config(struct wl1271 *wl)
>  	if (ret < 0)
>  		return ret;
>  
> -	ret = wl1271_acx_group_address_tbl(wl, true, NULL, 0);
> -	if (ret < 0)
> -		return ret;
> -

Here we're changing the order of these initialization ACXs, but it
shouldn't be a problem.  If it is a problem, it's a firmware bug that
(hopefully) can be fixed.


> @@ -213,11 +317,150 @@ static int wl1271_init_beacon_broadcast(struct wl1271 *wl)
>  	return 0;
>  }
>  
> +static int wl1271_sta_hw_init(struct wl1271 *wl)
> +{
> +	int ret;
> +
> +	ret = wl1271_cmd_ext_radio_parms(wl);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = wl1271_sta_init_templates_config(wl);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = wl1271_acx_group_address_tbl(wl, true, NULL, 0);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Initialize connection monitoring thresholds */
> +	ret = wl1271_acx_conn_monit_params(wl, false);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Beacon filtering */
> +	ret = wl1271_init_beacon_filter(wl);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Bluetooth WLAN coexistence */
> +	ret = wl1271_init_pta(wl);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Beacons and boradcast settings */

You copied the typo, maybe you could take the opportunity to fix it?

> +	ret = wl1271_init_beacon_broadcast(wl);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Configure for ELP power saving */
> +	ret = wl1271_acx_sleep_auth(wl, WL1271_PSM_ELP);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Configure rssi/snr averaging weights */
> +	ret = wl1271_acx_rssi_snr_avg_weights(wl);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = wl1271_acx_sta_rate_policies(wl);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}

Also here, this changes the initialization order a bit.  I think this
was Juuso's concern earlier.  At least while we were having debug camps,
the TI people supporting us were always asking us to be extremely
cautious with the order of the initialization.  They were saying that it
was really important to do the initialization in the same way as the
tiwlan driver does.  Hopefully this is not an issue now...


> @@ -279,21 +512,11 @@ int wl1271_hw_init(struct wl1271 *wl)
>  	if (ret < 0)
>  		goto out_free_memmap;
>  
> -	/* Bluetooth WLAN coexistence */
> -	ret = wl1271_init_pta(wl);
> -	if (ret < 0)
> -		goto out_free_memmap;
> -

Just out of curiosity... Is AP coexistence with Bluetooth possible? This
could be important in some scenarios.


> diff --git a/drivers/net/wireless/wl12xx/init.h b/drivers/net/wireless/wl12xx/init.h
> index 7762421..4d37210 100644
> --- a/drivers/net/wireless/wl12xx/init.h
> +++ b/drivers/net/wireless/wl12xx/init.h
> @@ -27,10 +27,13 @@
>  #include "wl12xx.h"
>  
>  int wl1271_hw_init_power_auth(struct wl1271 *wl);
> -int wl1271_init_templates_config(struct wl1271 *wl);
> +int wl1271_sta_init_templates_config(struct wl1271 *wl);
>  int wl1271_init_phy_config(struct wl1271 *wl);
>  int wl1271_init_pta(struct wl1271 *wl);
>  int wl1271_init_energy_detection(struct wl1271 *wl);
>  int wl1271_hw_init(struct wl1271 *wl);
>  
> +/* Functions from wl1271_main.c */
> +u32 wl1271_min_rate_get(struct wl1271 *wl);
> +

This is bad and shouldn't be here.  Need to find a better place for it.


> diff --git a/drivers/net/wireless/wl12xx/wl12xx_80211.h b/drivers/net/wireless/wl12xx/wl12xx_80211.h
> index be21032..b230e72 100644
> --- a/drivers/net/wireless/wl12xx/wl12xx_80211.h
> +++ b/drivers/net/wireless/wl12xx/wl12xx_80211.h
> @@ -160,4 +160,9 @@ struct wl12xx_probe_resp_template {
>  	struct wl12xx_ie_country country;
>  } __packed;
>  
> +struct wl12xx_disconn_template {
> +	struct ieee80211_header header;
> +	__le16 disconn_reason;
> +} __packed;
> +

Isn't padding necessary here to keep the struct 32-bit aligned?

-- 
Cheers,
Luca.