On 2017-02-06 12:25, Stanislaw Gruszka wrote:
> On Thu, Feb 02, 2017 at 12:52:08PM +0100, Felix Fietkau wrote:
>> This is a 2x2 PCIe 802.11ac chipset by MediaTek
>>
>> Signed-off-by: Felix Fietkau <[email protected]>
> Driver looks great to me, though I think it could be better commented.
> I have some minor issues, but if they need to be fixed, it could be done
> by incremental patches after apply this one to the tree.
>
> Reviewed-by: Stanislaw Gruszka <[email protected]>
Thanks for the review.
>> +enum dma_msg_port {
>> + WLAN_PORT,
>> + CPU_RX_PORT,
>> + CPU_TX_PORT,
>> + HOST_PORT,
>> + VIRTUAL_CPU_RX_PORT,
>> + VIRTUAL_CPU_TX_PORT,
>> + DISCARD,
>> +};
> Not used ?
Yeah, I guess it can be removed.
>> +void mt76x2_set_tx_ackto(struct mt76x2_dev *dev)
>> +{
>> + u8 ackto, sifs, slottime = dev->slottime;
>> +
>> + slottime += 3 * dev->coverage_class;
>> +
>> + sifs = mt76_get_field(dev, MT_XIFS_TIME_CFG,
>> + MT_XIFS_TIME_CFG_OFDM_SIFS);
>> +
>> + ackto = slottime + sifs;
>> + mt76_rmw_field(dev, MT_TX_TIMEOUT_CFG,
>> + MT_TX_TIMEOUT_CFG_ACKTO, ackto);
>> +}
> Interesting, if this correct way to configure the TX_TIMEOUT_CFG_ACKTO
> we will also need this in rt2x00. Vendor drivers use 32 for this setting
> and do not change it.
I don't think vendor drivers even have support for coverage class.
> Note we have also EXP_ACT_TIME and EXP_CTS_TIME registers, which stay
> with default settings, but perhaps should be changed depending on
> channel properties as well.
>
>> +static u32
>> +mt76x2_tx_power_mask(u8 v1, u8 v2, u8 v3, u8 v4)
>> +{
>> + u32 val = 0;
>> +
>> + val |= (v1 & (BIT(6) - 1)) << 0;
>> + val |= (v2 & (BIT(6) - 1)) << 8;
>> + val |= (v3 & (BIT(6) - 1)) << 16;
>> + val |= (v4 & (BIT(6) - 1)) << 24;
>> + return val;
>> +}
> TX_PWR_CFG registers consist of eight 4bit entries, masking
> two entries with 0x3f does not seems to be correct.
No, these registers consist of four 6bit entries. Both the vendor driver
and the datasheet describe them this way.
>> + mt76_wr(dev, MT_TX_PWR_CFG_3,
>> + mt76x2_tx_power_mask(t.ht[12], t.ht[14], t.ht[0], t.ht[2]));
>> + mt76_wr(dev, MT_TX_PWR_CFG_4,
>> + mt76x2_tx_power_mask(t.ht[4], t.ht[6], 0, 0));
>> + mt76_wr(dev, MT_TX_PWR_CFG_7,
>> + mt76x2_tx_power_mask(t.ofdm[6], t.vht[8], t.ht[6], t.vht[8]));
>> + mt76_wr(dev, MT_TX_PWR_CFG_8,
>> + mt76x2_tx_power_mask(t.ht[14], t.vht[8], t.vht[8], 0));
>> + mt76_wr(dev, MT_TX_PWR_CFG_9,
>> + mt76x2_tx_power_mask(t.ht[6], t.vht[8], t.vht[8], 0));
> Looks like some of arguments instead of t.vht[x] accidentally become t.ht[x],
> for example t.vht[6] is never used.
There are not enough register fields for all rates individually, so they
overlap. This looks a bit confusing and random, but I did check it
carefully against the vendor driver and the datasheet.
>> +static void
>> +mt76x2_configure_tx_delay(struct mt76x2_dev *dev, enum nl80211_band band,
>> u8 bw)
>> +{
>> + u32 cfg0, cfg1;
>> +
>> + if (mt76x2_ext_pa_enabled(dev, band)) {
>> + cfg0 = bw ? 0x000b0c01 : 0x00101101;
>> + cfg1 = 0x00011414;
>> + } else {
>> + cfg0 = bw ? 0x000b0b01 : 0x00101001;
>> + cfg1 = 0x00021414;
>> + }
>> + mt76_wr(dev, MT_TX_SW_CFG0, cfg0);
>> + mt76_wr(dev, MT_TX_SW_CFG1, cfg1);
>> +
>> + mt76_rmw_field(dev, MT_XIFS_TIME_CFG, MT_XIFS_TIME_CFG_CCK_SIFS,
>> + 13 + (bw ? 1 : 0));
>> +}
> SIFS for 2GHz should be 10us and for 5GHz 16us. Setting SIFS to 13
> or 14 looks wrong for 2GHz band. Can be correct for 5GHz assuming
> we have some internal delays configured on TX_SW_CFG registers.
This is a CCK-only SIFS value (there's a separate one for OFDM).
> But again this is interesting for rt2x00, where we stay with
> defaults, but looks we should configure XIFS_TIME_CFG based on
> channel.
I think many of these might be mt76x2 specific tweaks, so be careful
with applying any of that to rt2x00.
>> + if (chandef->width >= NL80211_CHAN_WIDTH_40)
>> + sifs++;
>> +
>> + mt76_rmw_field(dev, MT_XIFS_TIME_CFG, MT_XIFS_TIME_CFG_OFDM_SIFS, sifs);
> This probably should be set together with MT_XIFS_TIME_CFG_CCK_SIFS in
> mt76x2_configure_tx_delay().
Will look into that.
>> +static int mt76x2_insert_hdr_pad(struct sk_buff *skb)
>> +{
>> + int len = ieee80211_get_hdrlen_from_skb(skb);
>> + int ret;
>> +
>> + if (len % 4 == 0)
>> + return 0;
>> +
>> + if (skb_headroom(skb) < 2) {
>> + ret = pskb_expand_head(skb, 2, 0, GFP_ATOMIC);
>> + if (ret != 0)
>> + return ret;
> This should not be needed if hw->extra_tx_headroom is set properly.
Thanks, will send a follow-up cleanup patch if this one is accepted.
>> + if (info->flags & IEEE80211_TX_CTL_RATE_CTRL_PROBE)
>> + qsel = 0;
> For better understating: qsel = MT_QSEL_MGMT;
Right.
>> +void mt76x2_pre_tbtt_tasklet(unsigned long arg)
>> +{
>> + struct mt76x2_dev *dev = (struct mt76x2_dev *) arg;
>> + struct mt76_queue *q = &dev->mt76.q_tx[MT_TXQ_PSD];
>> + struct beacon_bc_data data = {};
>> + struct sk_buff *skb;
>> + int i, nframes;
>> +
>> + data.dev = dev;
>> + __skb_queue_head_init(&data.q);
>> +
>> + ieee80211_iterate_active_interfaces_atomic(mt76_hw(dev),
> This symbol, not like most of other mac80211 symbols, is exported via
> EXPORT_SYMBOL_GPL(), so I'm not sure if it can be used on dual licensed
> file, perhaps licence of this file should be changed to GPL only.
Technically only the resulting binary will become GPL. I still want to
keep the source code of the entire driver ISC licensed to make it easier
for the BSDs to copy code from it.
- Felix
