On Sun, Nov 15, 2009 at 11:14:53PM +0100, Felix Fietkau wrote:
> This patch changes ath9k to pass proper MCS indexes and flags
> between the RC and the rest of the driver code.
> sc->cur_rate_table remains, as it's used by the RC code internally,
> but the rest of the driver code no longer uses it, so a potential
> new RC for ath9k would not have to update it.
Awesome! Some comments below.
> +static const struct ath_rate_table *hw_rate_table[ATH9K_MODE_MAX] = {
> + [ATH9K_MODE_11A] = &ar5416_11a_ratetable,
> + [ATH9K_MODE_11G] = &ar5416_11g_ratetable,
> + [ATH9K_MODE_11NA_HT20] = &ar5416_11na_ratetable,
> + [ATH9K_MODE_11NG_HT20] = &ar5416_11ng_ratetable,
> + [ATH9K_MODE_11NA_HT40PLUS] = &ar5416_11na_ratetable,
> + [ATH9K_MODE_11NA_HT40MINUS] = &ar5416_11na_ratetable,
> + [ATH9K_MODE_11NG_HT40PLUS] = &ar5416_11ng_ratetable,
> + [ATH9K_MODE_11NG_HT40MINUS] = &ar5416_11ng_ratetable,
> +};
It seems this is the only reason to keep around the
sc->cur_rate_table and cur_rate_mode. We could just remove
this array and sc->cur_rate_table and cur_rate_mode if we just
relied on the current wiphy->hw conf as we do in the hw code.
To make checks easier we could add helpers to mac80211.h for things
like:
ATH9K_MODE_11A --> conf_is_no_ht_5g()
ATH9K_MODE_11G --> conf_is_no_ht_2g()
ATH9K_MODE_11NA_HT20 --> conf_is_ht20_2g()
ATH9K_MODE_11NG_HT20] --> conf_is_ht20_5g()
ATH9K_MODE_11NA_HT40PLUS --> conf_is_ht40_plus_5g()
ATH9K_MODE_11NA_HT40MINUS --> conf_is_ht40_minus_5g()
ATH9K_MODE_11NG_HT40PLUS --> conf_is_ht40_plus_2g()
ATH9K_MODE_11NG_HT40MINUS --> conf_is_ht40_minus_2g()
Then we can just pass the iee80211_hw to rc.c and remove the
cur_rate_mode and cur_rate_table.
Then -- ath9k.h no longer needs to forward declare or even know
about struct ath_rate_table and we can keep that private to rc.c.
This could be done on a separate patch though.
> +static struct ieee80211_rate ath9k_legacy_rates[] = {
> + { .bitrate = 10,
> + .hw_value = 0x1b,
> + .flags = 0 },
> + { .bitrate = 20,
> + .hw_value = 0x1a,
> + .hw_value_short = 0x1a | 0x04,
> + .flags = IEEE80211_RATE_SHORT_PREAMBLE },
> + { .bitrate = 55,
> + .hw_value = 0x19,
> + .hw_value_short = 0x19 | 0x04,
> + .flags = IEEE80211_RATE_SHORT_PREAMBLE },
> + { .bitrate = 110,
> + .hw_value = 0x18,
> + .hw_value_short = 0x18 | 0x4,
> + .flags = IEEE80211_RATE_SHORT_PREAMBLE },
> + { .bitrate = 60,
> + .hw_value = 0x0b,
> + .flags = 0 },
> + { .bitrate = 90,
> + .hw_value = 0x0f,
> + .flags = 0 },
> + { .bitrate = 120,
> + .hw_value = 0x0a,
> + .flags = 0 },
> + { .bitrate = 180,
> + .hw_value = 0x0e,
> + .flags = 0 },
> + { .bitrate = 240,
> + .hw_value = 0x09,
> + .flags = 0 },
> + { .bitrate = 360,
> + .hw_value = 0x0d,
> + .flags = 0 },
> + { .bitrate = 480,
> + .hw_value = 0x08,
> + .flags = 0 },
> + { .bitrate = 540,
> + .hw_value = 0x0c,
> + .flags = 0 },
> +};
> +
Cool! Hey so I had done something similar for ath9k_htc, feel free to
use:
/* Atheros hardware rate code addition for short premble */
#define SHPCHECK(__hw_rate, __flags) \
((__flags & IEEE80211_RATE_SHORT_PREAMBLE) ? (__hw_rate | 0x04 ) : 0)
#define RATE(_bitrate, _hw_rate, _flags) { \
.bitrate = (_bitrate), \
.flags = (_flags), \
.hw_value = (_hw_rate), \
.hw_value_short = (SHPCHECK(_hw_rate, _flags)) \
}
static struct ieee80211_rate ath9k_htc_ratetable[] = {
RATE(10, 0x1b, 0),
RATE(20, 0x1a, IEEE80211_RATE_SHORT_PREAMBLE), /* shortp : 0x1e */
RATE(55, 0x19, IEEE80211_RATE_SHORT_PREAMBLE), /* shortp: 0x1d */
RATE(110, 0x18, IEEE80211_RATE_SHORT_PREAMBLE), /* short: 0x1c */
RATE(60, 0x0b, 0),
RATE(90, 0x0f, 0),
RATE(120, 0x0a, 0),
RATE(180, 0x0e, 0),
RATE(240, 0x09, 0),
RATE(360, 0x0d, 0),
RATE(480, 0x08, 0),
RATE(540, 0x0c, 0),
};
> static void ath_cache_conf_rate(struct ath_softc *sc,
> struct ieee80211_conf *conf)
> {
> switch (conf->channel->band) {
> case IEEE80211_BAND_2GHZ:
> if (conf_is_ht20(conf))
> - sc->cur_rate_table =
> - sc->hw_rate_table[ATH9K_MODE_11NG_HT20];
> + sc->cur_rate_mode = ATH9K_MODE_11NG_HT20;
> else if (conf_is_ht40_minus(conf))
> - sc->cur_rate_table =
> - sc->hw_rate_table[ATH9K_MODE_11NG_HT40MINUS];
> + sc->cur_rate_mode = ATH9K_MODE_11NG_HT40MINUS;
> else if (conf_is_ht40_plus(conf))
> - sc->cur_rate_table =
> - sc->hw_rate_table[ATH9K_MODE_11NG_HT40PLUS];
> + sc->cur_rate_mode = ATH9K_MODE_11NG_HT40PLUS;
> else
> - sc->cur_rate_table =
> - sc->hw_rate_table[ATH9K_MODE_11G];
> + sc->cur_rate_mode = ATH9K_MODE_11G;
> break;
> case IEEE80211_BAND_5GHZ:
> if (conf_is_ht20(conf))
> - sc->cur_rate_table =
> - sc->hw_rate_table[ATH9K_MODE_11NA_HT20];
> + sc->cur_rate_mode = ATH9K_MODE_11NA_HT20;
> else if (conf_is_ht40_minus(conf))
> - sc->cur_rate_table =
> - sc->hw_rate_table[ATH9K_MODE_11NA_HT40MINUS];
> + sc->cur_rate_mode = ATH9K_MODE_11NA_HT40MINUS;
> else if (conf_is_ht40_plus(conf))
> - sc->cur_rate_table =
> - sc->hw_rate_table[ATH9K_MODE_11NA_HT40PLUS];
> + sc->cur_rate_mode = ATH9K_MODE_11NA_HT40PLUS;
> else
> - sc->cur_rate_table =
> - sc->hw_rate_table[ATH9K_MODE_11A];
> + sc->cur_rate_mode = ATH9K_MODE_11A;
> break;
> default:
> BUG_ON(1);
If we do the conf check on the hw struct we could end up removing this stuff.
> @@ -1833,19 +1816,22 @@ static int ath_init_softc(u16 devid, str
> /* setup channels and rates */
>
> sc->sbands[IEEE80211_BAND_2GHZ].channels = ath9k_2ghz_chantable;
> - sc->sbands[IEEE80211_BAND_2GHZ].bitrates =
> - sc->rates[IEEE80211_BAND_2GHZ];
> sc->sbands[IEEE80211_BAND_2GHZ].band = IEEE80211_BAND_2GHZ;
> sc->sbands[IEEE80211_BAND_2GHZ].n_channels =
> ARRAY_SIZE(ath9k_2ghz_chantable);
> + sc->sbands[IEEE80211_BAND_2GHZ].bitrates = ath9k_legacy_rates;
> + sc->sbands[IEEE80211_BAND_2GHZ].n_bitrates =
> + ARRAY_SIZE(ath9k_legacy_rates);
>
> if (test_bit(ATH9K_MODE_11A, sc->sc_ah->caps.wireless_modes)) {
> sc->sbands[IEEE80211_BAND_5GHZ].channels = ath9k_5ghz_chantable;
> - sc->sbands[IEEE80211_BAND_5GHZ].bitrates =
> - sc->rates[IEEE80211_BAND_5GHZ];
> sc->sbands[IEEE80211_BAND_5GHZ].band = IEEE80211_BAND_5GHZ;
> sc->sbands[IEEE80211_BAND_5GHZ].n_channels =
> ARRAY_SIZE(ath9k_5ghz_chantable);
> + sc->sbands[IEEE80211_BAND_5GHZ].bitrates =
> + ath9k_legacy_rates + 4;
> + sc->sbands[IEEE80211_BAND_5GHZ].n_bitrates =
> + ARRAY_SIZE(ath9k_legacy_rates) - 4;
> }
We could also remove sc->sbands and just assign the sband for the hw
if appropriate, somethign like:
hw->wiphy->bands[IEEE80211_BAND_2GHZ] = &ath9k_2ghz_sband;
But again, best on another patch.
> struct ath_rate_table {
> int rate_cnt;
> + int mcs_start;
> struct {
> int valid;
> int valid_single_stream;
> @@ -111,14 +112,12 @@ struct ath_rate_table {
> u32 ratekbps;
> u32 user_ratekbps;
> u8 ratecode;
> - u8 short_preamble;
> u8 dot11rate;
> u8 ctrl_rate;
> u8 base_index;
> u8 cw40index;
> u8 sgi_index;
> u8 ht_index;
> - u32 max_4ms_framelen;
> } info[RATE_TABLE_SIZE];
> u32 probe_interval;
> u8 initial_ratemax;
Do you think its possible to do this change in one patch and the rest in
another for easier review?
> --- a/drivers/net/wireless/ath/ath9k/beacon.c
> +++ b/drivers/net/wireless/ath/ath9k/beacon.c
> @@ -65,9 +65,9 @@ static void ath_beacon_setup(struct ath_
> struct ath_common *common = ath9k_hw_common(ah);
> struct ath_desc *ds;
> struct ath9k_11n_rate_series series[4];
> - const struct ath_rate_table *rt;
> int flags, antenna, ctsrate = 0, ctsduration = 0;
> - u8 rate;
> + struct ieee80211_supported_band *sband;
> + u8 rate = 0;
>
> ds = bf->bf_desc;
> flags = ATH9K_TXDESC_NOACK;
> @@ -91,10 +91,10 @@ static void ath_beacon_setup(struct ath_
>
> ds->ds_data = bf->bf_buf_addr;
>
> - rt = sc->cur_rate_table;
> - rate = rt->info[0].ratecode;
> + sband = &sc->sbands[sc->hw->conf.channel->band];
The sc->hw *will not* be the current hw config but the first wiphy so this
could end up using the wrong hw config for a virtual wiphy. This should rely
on the aphy->hw instead of sc->hw, or the common->hw as that is updated when
the virtual ath9k wiphy changes. Using the aphy->hw seems best though, I suppose
we can pass the aphy on the routine as the caller has it.
Rest looks good so far, thanks!
Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
