* [PATCH 3/3] rt2x00: rt2800: use correct txop value in tx descriptor
@ 2010-05-06 10:29 Helmut Schaa
2010-05-06 18:32 ` Gertjan van Wingerde
0 siblings, 1 reply; 5+ messages in thread
From: Helmut Schaa @ 2010-05-06 10:29 UTC (permalink / raw)
To: John Linville; +Cc: linux-wireless, Ivo van Doorn, Gertjan van Wingerde
rt2800 devices use a different enumeration to specify what IFS values should
be used on frame transmission compared to the other rt2x00 devices. Hence,
create a new enum called txop that contains the valid values.
Furthermore use TXOP_HTTXOP as default on HT devices which means that the
hardware should select the correct IFS value on its own.
I'm not sure if we have the need to specify one of the other txop values
in the future but as far as I could see TXOP_HTTXOP is used by the Ralink
drivers for all normal data frames and should be a reasonable default.
This patch doesn't really fix anything as until now the IFS_BACKOFF(=0)
value was used as txop which luckily mapped to TXOP_HTTXOP(=0).
Signed-off-by: Helmut Schaa <helmut.schaa@googlemail.com>
---
drivers/net/wireless/rt2x00/rt2800pci.c | 2 +-
drivers/net/wireless/rt2x00/rt2800usb.c | 2 +-
drivers/net/wireless/rt2x00/rt2x00ht.c | 2 ++
drivers/net/wireless/rt2x00/rt2x00queue.h | 2 ++
drivers/net/wireless/rt2x00/rt2x00reg.h | 10 ++++++++++
5 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/rt2x00/rt2800pci.c b/drivers/net/wireless/rt2x00/rt2800pci.c
index f08b6a3..df2c3fb 100644
--- a/drivers/net/wireless/rt2x00/rt2800pci.c
+++ b/drivers/net/wireless/rt2x00/rt2800pci.c
@@ -643,7 +643,7 @@ static int rt2800pci_write_tx_data(struct queue_entry* entry,
rt2x00_set_field32(&word, TXWI_W0_AMPDU,
test_bit(ENTRY_TXD_HT_AMPDU, &txdesc->flags));
rt2x00_set_field32(&word, TXWI_W0_MPDU_DENSITY, txdesc->mpdu_density);
- rt2x00_set_field32(&word, TXWI_W0_TX_OP, txdesc->ifs);
+ rt2x00_set_field32(&word, TXWI_W0_TX_OP, txdesc->txop);
rt2x00_set_field32(&word, TXWI_W0_MCS, txdesc->mcs);
rt2x00_set_field32(&word, TXWI_W0_BW,
test_bit(ENTRY_TXD_HT_BW_40, &txdesc->flags));
diff --git a/drivers/net/wireless/rt2x00/rt2800usb.c b/drivers/net/wireless/rt2x00/rt2800usb.c
index e3f3a97..c9e1320 100644
--- a/drivers/net/wireless/rt2x00/rt2800usb.c
+++ b/drivers/net/wireless/rt2x00/rt2800usb.c
@@ -417,7 +417,7 @@ static void rt2800usb_write_tx_desc(struct rt2x00_dev *rt2x00dev,
rt2x00_set_field32(&word, TXWI_W0_AMPDU,
test_bit(ENTRY_TXD_HT_AMPDU, &txdesc->flags));
rt2x00_set_field32(&word, TXWI_W0_MPDU_DENSITY, txdesc->mpdu_density);
- rt2x00_set_field32(&word, TXWI_W0_TX_OP, txdesc->ifs);
+ rt2x00_set_field32(&word, TXWI_W0_TX_OP, txdesc->txop);
rt2x00_set_field32(&word, TXWI_W0_MCS, txdesc->mcs);
rt2x00_set_field32(&word, TXWI_W0_BW,
test_bit(ENTRY_TXD_HT_BW_40, &txdesc->flags));
diff --git a/drivers/net/wireless/rt2x00/rt2x00ht.c b/drivers/net/wireless/rt2x00/rt2x00ht.c
index 1056c92..5483fec 100644
--- a/drivers/net/wireless/rt2x00/rt2x00ht.c
+++ b/drivers/net/wireless/rt2x00/rt2x00ht.c
@@ -66,4 +66,6 @@ void rt2x00ht_create_tx_descriptor(struct queue_entry *entry,
__set_bit(ENTRY_TXD_HT_BW_40, &txdesc->flags);
if (txrate->flags & IEEE80211_TX_RC_SHORT_GI)
__set_bit(ENTRY_TXD_HT_SHORT_GI, &txdesc->flags);
+
+ txdesc->txop = TXOP_HTTXOP;
}
diff --git a/drivers/net/wireless/rt2x00/rt2x00queue.h b/drivers/net/wireless/rt2x00/rt2x00queue.h
index 94a48c1..36a957a 100644
--- a/drivers/net/wireless/rt2x00/rt2x00queue.h
+++ b/drivers/net/wireless/rt2x00/rt2x00queue.h
@@ -299,6 +299,7 @@ enum txentry_desc_flags {
* @retry_limit: Max number of retries.
* @aifs: AIFS value.
* @ifs: IFS value.
+ * @txop: IFS value for 11n capable chips.
* @cw_min: cwmin value.
* @cw_max: cwmax value.
* @cipher: Cipher type used for encryption.
@@ -328,6 +329,7 @@ struct txentry_desc {
short retry_limit;
short aifs;
short ifs;
+ short txop;
short cw_min;
short cw_max;
diff --git a/drivers/net/wireless/rt2x00/rt2x00reg.h b/drivers/net/wireless/rt2x00/rt2x00reg.h
index 603bfc0..894ff78 100644
--- a/drivers/net/wireless/rt2x00/rt2x00reg.h
+++ b/drivers/net/wireless/rt2x00/rt2x00reg.h
@@ -101,6 +101,16 @@ enum ifs {
};
/*
+ * IFS values for HT devices
+ */
+enum txop {
+ TXOP_HTTXOP = 0,
+ TXOP_PIFS = 1,
+ TXOP_SIFS = 2,
+ TXOP_BACKOFF = 3,
+};
+
+/*
* Cipher types for hardware encryption
*/
enum cipher {
--
1.6.4.2
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH 3/3] rt2x00: rt2800: use correct txop value in tx descriptor
2010-05-06 10:29 [PATCH 3/3] rt2x00: rt2800: use correct txop value in tx descriptor Helmut Schaa
@ 2010-05-06 18:32 ` Gertjan van Wingerde
2010-05-06 18:57 ` Helmut Schaa
0 siblings, 1 reply; 5+ messages in thread
From: Gertjan van Wingerde @ 2010-05-06 18:32 UTC (permalink / raw)
To: Helmut Schaa; +Cc: John Linville, linux-wireless, Ivo van Doorn
On 05/06/10 12:29, Helmut Schaa wrote:
> rt2800 devices use a different enumeration to specify what IFS values should
> be used on frame transmission compared to the other rt2x00 devices. Hence,
> create a new enum called txop that contains the valid values.
>
> Furthermore use TXOP_HTTXOP as default on HT devices which means that the
> hardware should select the correct IFS value on its own.
>
> I'm not sure if we have the need to specify one of the other txop values
> in the future but as far as I could see TXOP_HTTXOP is used by the Ralink
> drivers for all normal data frames and should be a reasonable default.
>
> This patch doesn't really fix anything as until now the IFS_BACKOFF(=0)
> value was used as txop which luckily mapped to TXOP_HTTXOP(=0).
>
> Signed-off-by: Helmut Schaa <helmut.schaa@googlemail.com>
> ---
> drivers/net/wireless/rt2x00/rt2800pci.c | 2 +-
> drivers/net/wireless/rt2x00/rt2800usb.c | 2 +-
> drivers/net/wireless/rt2x00/rt2x00ht.c | 2 ++
> drivers/net/wireless/rt2x00/rt2x00queue.h | 2 ++
> drivers/net/wireless/rt2x00/rt2x00reg.h | 10 ++++++++++
> 5 files changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/rt2x00/rt2800pci.c b/drivers/net/wireless/rt2x00/rt2800pci.c
> index f08b6a3..df2c3fb 100644
> --- a/drivers/net/wireless/rt2x00/rt2800pci.c
> +++ b/drivers/net/wireless/rt2x00/rt2800pci.c
> @@ -643,7 +643,7 @@ static int rt2800pci_write_tx_data(struct queue_entry* entry,
> rt2x00_set_field32(&word, TXWI_W0_AMPDU,
> test_bit(ENTRY_TXD_HT_AMPDU, &txdesc->flags));
> rt2x00_set_field32(&word, TXWI_W0_MPDU_DENSITY, txdesc->mpdu_density);
> - rt2x00_set_field32(&word, TXWI_W0_TX_OP, txdesc->ifs);
> + rt2x00_set_field32(&word, TXWI_W0_TX_OP, txdesc->txop);
> rt2x00_set_field32(&word, TXWI_W0_MCS, txdesc->mcs);
> rt2x00_set_field32(&word, TXWI_W0_BW,
> test_bit(ENTRY_TXD_HT_BW_40, &txdesc->flags));
> diff --git a/drivers/net/wireless/rt2x00/rt2800usb.c b/drivers/net/wireless/rt2x00/rt2800usb.c
> index e3f3a97..c9e1320 100644
> --- a/drivers/net/wireless/rt2x00/rt2800usb.c
> +++ b/drivers/net/wireless/rt2x00/rt2800usb.c
> @@ -417,7 +417,7 @@ static void rt2800usb_write_tx_desc(struct rt2x00_dev *rt2x00dev,
> rt2x00_set_field32(&word, TXWI_W0_AMPDU,
> test_bit(ENTRY_TXD_HT_AMPDU, &txdesc->flags));
> rt2x00_set_field32(&word, TXWI_W0_MPDU_DENSITY, txdesc->mpdu_density);
> - rt2x00_set_field32(&word, TXWI_W0_TX_OP, txdesc->ifs);
> + rt2x00_set_field32(&word, TXWI_W0_TX_OP, txdesc->txop);
> rt2x00_set_field32(&word, TXWI_W0_MCS, txdesc->mcs);
> rt2x00_set_field32(&word, TXWI_W0_BW,
> test_bit(ENTRY_TXD_HT_BW_40, &txdesc->flags));
> diff --git a/drivers/net/wireless/rt2x00/rt2x00ht.c b/drivers/net/wireless/rt2x00/rt2x00ht.c
> index 1056c92..5483fec 100644
> --- a/drivers/net/wireless/rt2x00/rt2x00ht.c
> +++ b/drivers/net/wireless/rt2x00/rt2x00ht.c
> @@ -66,4 +66,6 @@ void rt2x00ht_create_tx_descriptor(struct queue_entry *entry,
> __set_bit(ENTRY_TXD_HT_BW_40, &txdesc->flags);
> if (txrate->flags & IEEE80211_TX_RC_SHORT_GI)
> __set_bit(ENTRY_TXD_HT_SHORT_GI, &txdesc->flags);
> +
> + txdesc->txop = TXOP_HTTXOP;
> }
I am not too sure about this part. If I look at the Ralink vendor driver, they are most of the time
using IFS_BACKOFF (value 3). Why did you put this on TXOP_HTTXOP?
> diff --git a/drivers/net/wireless/rt2x00/rt2x00queue.h b/drivers/net/wireless/rt2x00/rt2x00queue.h
> index 94a48c1..36a957a 100644
> --- a/drivers/net/wireless/rt2x00/rt2x00queue.h
> +++ b/drivers/net/wireless/rt2x00/rt2x00queue.h
> @@ -299,6 +299,7 @@ enum txentry_desc_flags {
> * @retry_limit: Max number of retries.
> * @aifs: AIFS value.
> * @ifs: IFS value.
> + * @txop: IFS value for 11n capable chips.
> * @cw_min: cwmin value.
> * @cw_max: cwmax value.
> * @cipher: Cipher type used for encryption.
> @@ -328,6 +329,7 @@ struct txentry_desc {
> short retry_limit;
> short aifs;
> short ifs;
> + short txop;
> short cw_min;
> short cw_max;
>
> diff --git a/drivers/net/wireless/rt2x00/rt2x00reg.h b/drivers/net/wireless/rt2x00/rt2x00reg.h
> index 603bfc0..894ff78 100644
> --- a/drivers/net/wireless/rt2x00/rt2x00reg.h
> +++ b/drivers/net/wireless/rt2x00/rt2x00reg.h
> @@ -101,6 +101,16 @@ enum ifs {
> };
>
> /*
> + * IFS values for HT devices
> + */
> +enum txop {
> + TXOP_HTTXOP = 0,
> + TXOP_PIFS = 1,
> + TXOP_SIFS = 2,
> + TXOP_BACKOFF = 3,
> +};
> +
> +/*
> * Cipher types for hardware encryption
> */
> enum cipher {
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH 3/3] rt2x00: rt2800: use correct txop value in tx descriptor
2010-05-06 18:32 ` Gertjan van Wingerde
@ 2010-05-06 18:57 ` Helmut Schaa
2010-05-07 8:11 ` Helmut Schaa
0 siblings, 1 reply; 5+ messages in thread
From: Helmut Schaa @ 2010-05-06 18:57 UTC (permalink / raw)
To: Gertjan van Wingerde; +Cc: John Linville, linux-wireless, Ivo van Doorn
Am Donnerstag 06 Mai 2010 schrieb Gertjan van Wingerde:
> On 05/06/10 12:29, Helmut Schaa wrote:
[...]
> > diff --git a/drivers/net/wireless/rt2x00/rt2x00ht.c b/drivers/net/wireless/rt2x00/rt2x00ht.c
> > index 1056c92..5483fec 100644
> > --- a/drivers/net/wireless/rt2x00/rt2x00ht.c
> > +++ b/drivers/net/wireless/rt2x00/rt2x00ht.c
> > @@ -66,4 +66,6 @@ void rt2x00ht_create_tx_descriptor(struct queue_entry *entry,
> > __set_bit(ENTRY_TXD_HT_BW_40, &txdesc->flags);
> > if (txrate->flags & IEEE80211_TX_RC_SHORT_GI)
> > __set_bit(ENTRY_TXD_HT_SHORT_GI, &txdesc->flags);
> > +
> > + txdesc->txop = TXOP_HTTXOP;
> > }
>
> I am not too sure about this part. If I look at the Ralink vendor driver, they are most of the time
> using IFS_BACKOFF (value 3). Why did you put this on TXOP_HTTXOP?
>From what I saw in the ralink driver IFS_BACKOFF is only used for management frames, IFS_SIFS only
for subsequent frames in a fragment burst and IFS_HTTXOPS for "normal" data frames. But that's
just the result of a _quick_ review. So I might be wrong here as well :)
To be honest I don't really know what the device does in case IFS_HTTXOPS is set but that was
the value we've passed to the driver before ;) (==IFS_BACKOFF on all other ralink chips) and it
works quite well. I also tried IFS_BACKOFF and I wasn't able to see a difference when using
legacy (11b & 11g) rates (neither on the device itself nor with a second machine monitoring
the traffic).
But I agree that we have to dig further on when to use which value but nevertheless the patch
shouldn't cause any harm but is meant as a base for further improvements.
If you want to we can also wait with this one until we completely figured out what to do?
Helmut
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 3/3] rt2x00: rt2800: use correct txop value in tx descriptor
2010-05-06 18:57 ` Helmut Schaa
@ 2010-05-07 8:11 ` Helmut Schaa
2010-05-07 8:21 ` Gertjan van Wingerde
0 siblings, 1 reply; 5+ messages in thread
From: Helmut Schaa @ 2010-05-07 8:11 UTC (permalink / raw)
To: Gertjan van Wingerde; +Cc: John Linville, linux-wireless, Ivo van Doorn
Am Donnerstag 06 Mai 2010 schrieb Helmut Schaa:
> Am Donnerstag 06 Mai 2010 schrieb Gertjan van Wingerde:
> > On 05/06/10 12:29, Helmut Schaa wrote:
>
> [...]
>
> > > diff --git a/drivers/net/wireless/rt2x00/rt2x00ht.c b/drivers/net/wireless/rt2x00/rt2x00ht.c
> > > index 1056c92..5483fec 100644
> > > --- a/drivers/net/wireless/rt2x00/rt2x00ht.c
> > > +++ b/drivers/net/wireless/rt2x00/rt2x00ht.c
> > > @@ -66,4 +66,6 @@ void rt2x00ht_create_tx_descriptor(struct queue_entry *entry,
> > > __set_bit(ENTRY_TXD_HT_BW_40, &txdesc->flags);
> > > if (txrate->flags & IEEE80211_TX_RC_SHORT_GI)
> > > __set_bit(ENTRY_TXD_HT_SHORT_GI, &txdesc->flags);
> > > +
> > > + txdesc->txop = TXOP_HTTXOP;
> > > }
> >
> > I am not too sure about this part. If I look at the Ralink vendor driver, they are most of the time
> > using IFS_BACKOFF (value 3). Why did you put this on TXOP_HTTXOP?
>
> From what I saw in the ralink driver IFS_BACKOFF is only used for management frames, IFS_SIFS only
> for subsequent frames in a fragment burst and IFS_HTTXOPS for "normal" data frames. But that's
> just the result of a _quick_ review. So I might be wrong here as well :)
>
> To be honest I don't really know what the device does in case IFS_HTTXOPS is set but that was
> the value we've passed to the driver before ;) (==IFS_BACKOFF on all other ralink chips) and it
> works quite well. I also tried IFS_BACKOFF and I wasn't able to see a difference when using
> legacy (11b & 11g) rates (neither on the device itself nor with a second machine monitoring
> the traffic).
Ok, after further examination it turns out to be:
- Management frames are sent with IFS_BACKOFF
- Special case for PsPoll frames also with IFS_BACKOFF
- Data frames are sent with IFS_HTTXOP
- Data frame subsequent fragments are send with IFS_SIFS
- CTS frames (in AP mode) use IFS_SIFS
So, I guess I resend this patch after some testing with IFS_BACKOFF for
management frames and IFS_HTTXOP for data frames and SIFS for subsequent
fragments.
John, please don't merge this patch yet. Thanks.
Helmut
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 3/3] rt2x00: rt2800: use correct txop value in tx descriptor
2010-05-07 8:11 ` Helmut Schaa
@ 2010-05-07 8:21 ` Gertjan van Wingerde
0 siblings, 0 replies; 5+ messages in thread
From: Gertjan van Wingerde @ 2010-05-07 8:21 UTC (permalink / raw)
To: Helmut Schaa; +Cc: John Linville, linux-wireless, Ivo van Doorn
On 05/07/10 10:11, Helmut Schaa wrote:
> Am Donnerstag 06 Mai 2010 schrieb Helmut Schaa:
>> Am Donnerstag 06 Mai 2010 schrieb Gertjan van Wingerde:
>>> On 05/06/10 12:29, Helmut Schaa wrote:
>>
>> [...]
>>
>>>> diff --git a/drivers/net/wireless/rt2x00/rt2x00ht.c b/drivers/net/wireless/rt2x00/rt2x00ht.c
>>>> index 1056c92..5483fec 100644
>>>> --- a/drivers/net/wireless/rt2x00/rt2x00ht.c
>>>> +++ b/drivers/net/wireless/rt2x00/rt2x00ht.c
>>>> @@ -66,4 +66,6 @@ void rt2x00ht_create_tx_descriptor(struct queue_entry *entry,
>>>> __set_bit(ENTRY_TXD_HT_BW_40, &txdesc->flags);
>>>> if (txrate->flags & IEEE80211_TX_RC_SHORT_GI)
>>>> __set_bit(ENTRY_TXD_HT_SHORT_GI, &txdesc->flags);
>>>> +
>>>> + txdesc->txop = TXOP_HTTXOP;
>>>> }
>>>
>>> I am not too sure about this part. If I look at the Ralink vendor driver, they are most of the time
>>> using IFS_BACKOFF (value 3). Why did you put this on TXOP_HTTXOP?
>>
>> From what I saw in the ralink driver IFS_BACKOFF is only used for management frames, IFS_SIFS only
>> for subsequent frames in a fragment burst and IFS_HTTXOPS for "normal" data frames. But that's
>> just the result of a _quick_ review. So I might be wrong here as well :)
>>
>> To be honest I don't really know what the device does in case IFS_HTTXOPS is set but that was
>> the value we've passed to the driver before ;) (==IFS_BACKOFF on all other ralink chips) and it
>> works quite well. I also tried IFS_BACKOFF and I wasn't able to see a difference when using
>> legacy (11b & 11g) rates (neither on the device itself nor with a second machine monitoring
>> the traffic).
>
> Ok, after further examination it turns out to be:
>
> - Management frames are sent with IFS_BACKOFF
> - Special case for PsPoll frames also with IFS_BACKOFF
> - Data frames are sent with IFS_HTTXOP
> - Data frame subsequent fragments are send with IFS_SIFS
> - CTS frames (in AP mode) use IFS_SIFS
>
> So, I guess I resend this patch after some testing with IFS_BACKOFF for
> management frames and IFS_HTTXOP for data frames and SIFS for subsequent
> fragments.
>
> John, please don't merge this patch yet. Thanks.
Thanks for investigating this further. I think the approach you describe above is the correct
approach for this.
---
Gertjan.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2010-05-07 8:21 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-05-06 10:29 [PATCH 3/3] rt2x00: rt2800: use correct txop value in tx descriptor Helmut Schaa
2010-05-06 18:32 ` Gertjan van Wingerde
2010-05-06 18:57 ` Helmut Schaa
2010-05-07 8:11 ` Helmut Schaa
2010-05-07 8:21 ` Gertjan van Wingerde
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.