[PATCH 1/3] rt2x00: rt2800: update initial SIFS values

[PATCH 1/3] rt2x00: rt2800: update initial SIFS values

[Helmut Schaa]

Currently the CCK and OFDM SIFS value is set to 32us. This value is neither
used by the Ralink driver nor specified in 802.11.

Instead of using 10us for CCK SIFS (as defined in 802.11) use 16us like in the
Ralink drivers. And indeed using a SIFS value of 10us breaks connectivity with
11g + CTS protected connections. Add a comment to the code why we don't use 10us
for CCK SIFS value.

The OFDM SIFS value is set to 16us (as defined in 802.11 and also used by the
Ralink drivers).

Signed-off-by: Helmut Schaa <helmut.schaa@googlemail.com>
---
 drivers/net/wireless/rt2x00/rt2800lib.c |   11 +++++++++--
 1 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/rt2x00/rt2800lib.c b/drivers/net/wireless/rt2x00/rt2800lib.c
index e37bbea..f786068 100644
--- a/drivers/net/wireless/rt2x00/rt2800lib.c
+++ b/drivers/net/wireless/rt2x00/rt2800lib.c
@@ -1415,9 +1415,16 @@ int rt2800_init_registers(struct rt2x00_dev *rt2x00dev)
 
 	rt2800_register_write(rt2x00dev, EXP_ACK_TIME, 0x002400ca);
 
+	/*
+	 * Usually the CCK SIFS time should be set to 10 and the OFDM SIFS
+	 * time should be set to 16. However, the original Ralink driver uses
+	 * 16 for both and indeed using a value of 10 for CCK SIFS results in
+	 * connection problems with 11g + CTS protection. Hence, use the same
+	 * defaults as the Ralink driver: 16 for both, CCK and OFDM SIFS.
+	 */
 	rt2800_register_read(rt2x00dev, XIFS_TIME_CFG, &reg);
-	rt2x00_set_field32(&reg, XIFS_TIME_CFG_CCKM_SIFS_TIME, 32);
-	rt2x00_set_field32(&reg, XIFS_TIME_CFG_OFDM_SIFS_TIME, 32);
+	rt2x00_set_field32(&reg, XIFS_TIME_CFG_CCKM_SIFS_TIME, 16);
+	rt2x00_set_field32(&reg, XIFS_TIME_CFG_OFDM_SIFS_TIME, 16);
 	rt2x00_set_field32(&reg, XIFS_TIME_CFG_OFDM_XIFS_TIME, 4);
 	rt2x00_set_field32(&reg, XIFS_TIME_CFG_EIFS, 314);
 	rt2x00_set_field32(&reg, XIFS_TIME_CFG_BB_RXEND_ENABLE, 1);
-- 
1.6.4.2

Re: [PATCH 1/3] rt2x00: rt2800: update initial SIFS values

[Gertjan van Wingerde]

On 05/06/10 12:29, Helmut Schaa wrote:
> Currently the CCK and OFDM SIFS value is set to 32us. This value is neither
> used by the Ralink driver nor specified in 802.11.
> 
> Instead of using 10us for CCK SIFS (as defined in 802.11) use 16us like in the
> Ralink drivers. And indeed using a SIFS value of 10us breaks connectivity with
> 11g + CTS protected connections. Add a comment to the code why we don't use 10us
> for CCK SIFS value.
> 
> The OFDM SIFS value is set to 16us (as defined in 802.11 and also used by the
> Ralink drivers).
> 
> Signed-off-by: Helmut Schaa <helmut.schaa@googlemail.com>

Good catch.

Acked-by: Gertjan van Wingerde <gwingerde@gmail.com>

> ---
>  drivers/net/wireless/rt2x00/rt2800lib.c |   11 +++++++++--
>  1 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/rt2x00/rt2800lib.c b/drivers/net/wireless/rt2x00/rt2800lib.c
> index e37bbea..f786068 100644
> --- a/drivers/net/wireless/rt2x00/rt2800lib.c
> +++ b/drivers/net/wireless/rt2x00/rt2800lib.c
> @@ -1415,9 +1415,16 @@ int rt2800_init_registers(struct rt2x00_dev *rt2x00dev)
>  
>  	rt2800_register_write(rt2x00dev, EXP_ACK_TIME, 0x002400ca);
>  
> +	/*
> +	 * Usually the CCK SIFS time should be set to 10 and the OFDM SIFS
> +	 * time should be set to 16. However, the original Ralink driver uses
> +	 * 16 for both and indeed using a value of 10 for CCK SIFS results in
> +	 * connection problems with 11g + CTS protection. Hence, use the same
> +	 * defaults as the Ralink driver: 16 for both, CCK and OFDM SIFS.
> +	 */
>  	rt2800_register_read(rt2x00dev, XIFS_TIME_CFG, &reg);
> -	rt2x00_set_field32(&reg, XIFS_TIME_CFG_CCKM_SIFS_TIME, 32);
> -	rt2x00_set_field32(&reg, XIFS_TIME_CFG_OFDM_SIFS_TIME, 32);
> +	rt2x00_set_field32(&reg, XIFS_TIME_CFG_CCKM_SIFS_TIME, 16);
> +	rt2x00_set_field32(&reg, XIFS_TIME_CFG_OFDM_SIFS_TIME, 16);
>  	rt2x00_set_field32(&reg, XIFS_TIME_CFG_OFDM_XIFS_TIME, 4);
>  	rt2x00_set_field32(&reg, XIFS_TIME_CFG_EIFS, 314);
>  	rt2x00_set_field32(&reg, XIFS_TIME_CFG_BB_RXEND_ENABLE, 1);

Re: [PATCH 1/3] rt2x00: rt2800: update initial SIFS values

[Ivo Van Doorn]

On Thu, May 6, 2010 at 12:29 PM, Helmut Schaa
<helmut.schaa@googlemail.com> wrote:
> Currently the CCK and OFDM SIFS value is set to 32us. This value is neither
> used by the Ralink driver nor specified in 802.11.
>
> Instead of using 10us for CCK SIFS (as defined in 802.11) use 16us like in the
> Ralink drivers. And indeed using a SIFS value of 10us breaks connectivity with
> 11g + CTS protected connections. Add a comment to the code why we don't use 10us
> for CCK SIFS value.
>
> The OFDM SIFS value is set to 16us (as defined in 802.11 and also used by the
> Ralink drivers).

Just wondering, but we hardcode the SIFS value in the rt2x00.h file.
Perhaps we should remove it in there, and no longer pass it from
rt2x00lib. That way there can't be any confusion about which drivers
uses the sifs field and whcih do not.

> Signed-off-by: Helmut Schaa <helmut.schaa@googlemail.com>

But that is really a different issue then this fix. So:

Acked-by: Ivo van Doorn <IvDoorn@gmail.com>

> ---
>  drivers/net/wireless/rt2x00/rt2800lib.c |   11 +++++++++--
>  1 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/rt2x00/rt2800lib.c b/drivers/net/wireless/rt2x00/rt2800lib.c
> index e37bbea..f786068 100644
> --- a/drivers/net/wireless/rt2x00/rt2800lib.c
> +++ b/drivers/net/wireless/rt2x00/rt2800lib.c
> @@ -1415,9 +1415,16 @@ int rt2800_init_registers(struct rt2x00_dev *rt2x00dev)
>
>        rt2800_register_write(rt2x00dev, EXP_ACK_TIME, 0x002400ca);
>
> +       /*
> +        * Usually the CCK SIFS time should be set to 10 and the OFDM SIFS
> +        * time should be set to 16. However, the original Ralink driver uses
> +        * 16 for both and indeed using a value of 10 for CCK SIFS results in
> +        * connection problems with 11g + CTS protection. Hence, use the same
> +        * defaults as the Ralink driver: 16 for both, CCK and OFDM SIFS.
> +        */
>        rt2800_register_read(rt2x00dev, XIFS_TIME_CFG, &reg);
> -       rt2x00_set_field32(&reg, XIFS_TIME_CFG_CCKM_SIFS_TIME, 32);
> -       rt2x00_set_field32(&reg, XIFS_TIME_CFG_OFDM_SIFS_TIME, 32);
> +       rt2x00_set_field32(&reg, XIFS_TIME_CFG_CCKM_SIFS_TIME, 16);
> +       rt2x00_set_field32(&reg, XIFS_TIME_CFG_OFDM_SIFS_TIME, 16);
>        rt2x00_set_field32(&reg, XIFS_TIME_CFG_OFDM_XIFS_TIME, 4);
>        rt2x00_set_field32(&reg, XIFS_TIME_CFG_EIFS, 314);
>        rt2x00_set_field32(&reg, XIFS_TIME_CFG_BB_RXEND_ENABLE, 1);
> --
> 1.6.4.2
>
>

Re: [PATCH 1/3] rt2x00: rt2800: update initial SIFS values

[Helmut Schaa]

Am Donnerstag 06 Mai 2010 schrieb Ivo Van Doorn:
> On Thu, May 6, 2010 at 12:29 PM, Helmut Schaa
> <helmut.schaa@googlemail.com> wrote:
> > Currently the CCK and OFDM SIFS value is set to 32us. This value is neither
> > used by the Ralink driver nor specified in 802.11.
> >
> > Instead of using 10us for CCK SIFS (as defined in 802.11) use 16us like in the
> > Ralink drivers. And indeed using a SIFS value of 10us breaks connectivity with
> > 11g + CTS protected connections. Add a comment to the code why we don't use 10us
> > for CCK SIFS value.
> >
> > The OFDM SIFS value is set to 16us (as defined in 802.11 and also used by the
> > Ralink drivers).
> 
> Just wondering, but we hardcode the SIFS value in the rt2x00.h file.
> Perhaps we should remove it in there, and no longer pass it from
> rt2x00lib. That way there can't be any confusion about which drivers
> uses the sifs field and whcih do not.

Yes, wouldn't be too bad I guess. But all non 2800 drivers use the same sifs
value, so we could just leave the define in rt2x00.h and remove the sifs value
from the config_erp calback and use the define in all other places?

Nevertheless, I'm not sure why the rt2800 devices don't like the 10us CCK
SIFS value (which is defined in 802.11) but I wasn't able to get CTS to self
working correctly with it set to 10 (the actual data frame was sent out
way too late after the CTS frame, somtimes with delays >100us). Using the 16us
also for CCK (as the ralink drivers do) results in perfect CTS & data frame
timing. Maybe it's a hardware issue and the (ralink) driver just works around
it?

Helmut

Re: [PATCH 1/3] rt2x00: rt2800: update initial SIFS values

[Ivo Van Doorn]

On Thu, May 6, 2010 at 8:38 PM, Helmut Schaa
<helmut.schaa@googlemail.com> wrote:
> Am Donnerstag 06 Mai 2010 schrieb Ivo Van Doorn:
>> On Thu, May 6, 2010 at 12:29 PM, Helmut Schaa
>> <helmut.schaa@googlemail.com> wrote:
>> > Currently the CCK and OFDM SIFS value is set to 32us. This value is neither
>> > used by the Ralink driver nor specified in 802.11.
>> >
>> > Instead of using 10us for CCK SIFS (as defined in 802.11) use 16us like in the
>> > Ralink drivers. And indeed using a SIFS value of 10us breaks connectivity with
>> > 11g + CTS protected connections. Add a comment to the code why we don't use 10us
>> > for CCK SIFS value.
>> >
>> > The OFDM SIFS value is set to 16us (as defined in 802.11 and also used by the
>> > Ralink drivers).
>>
>> Just wondering, but we hardcode the SIFS value in the rt2x00.h file.
>> Perhaps we should remove it in there, and no longer pass it from
>> rt2x00lib. That way there can't be any confusion about which drivers
>> uses the sifs field and whcih do not.
>
> Yes, wouldn't be too bad I guess. But all non 2800 drivers use the same sifs
> value, so we could just leave the define in rt2x00.h and remove the sifs value
> from the config_erp calback and use the define in all other places?

Well I would still remove the define in that case. I think some of the
legacy drivers
work with different SIFS values but always accepted the value which we used in
the define. So I would still remove the define, and then each driver
is free to set
the value as used in the legacy drivers.

> Nevertheless, I'm not sure why the rt2800 devices don't like the 10us CCK
> SIFS value (which is defined in 802.11) but I wasn't able to get CTS to self
> working correctly with it set to 10 (the actual data frame was sent out
> way too late after the CTS frame, somtimes with delays >100us). Using the 16us
> also for CCK (as the ralink drivers do) results in perfect CTS & data frame
> timing. Maybe it's a hardware issue and the (ralink) driver just works around
> it?

Yes, I expect it to be case. This kind of odd values are used in the
legacy drivers
for other settings as well.

Ivo

Re: [PATCH 1/3] rt2x00: rt2800: update initial SIFS values

[Helmut Schaa]

Am Donnerstag 06 Mai 2010 schrieb Ivo Van Doorn:
> On Thu, May 6, 2010 at 8:38 PM, Helmut Schaa
> <helmut.schaa@googlemail.com> wrote:
> > Am Donnerstag 06 Mai 2010 schrieb Ivo Van Doorn:
> >> On Thu, May 6, 2010 at 12:29 PM, Helmut Schaa
> >> <helmut.schaa@googlemail.com> wrote:
> >> > Currently the CCK and OFDM SIFS value is set to 32us. This value is neither
> >> > used by the Ralink driver nor specified in 802.11.
> >> >
> >> > Instead of using 10us for CCK SIFS (as defined in 802.11) use 16us like in the
> >> > Ralink drivers. And indeed using a SIFS value of 10us breaks connectivity with
> >> > 11g + CTS protected connections. Add a comment to the code why we don't use 10us
> >> > for CCK SIFS value.
> >> >
> >> > The OFDM SIFS value is set to 16us (as defined in 802.11 and also used by the
> >> > Ralink drivers).
> >>
> >> Just wondering, but we hardcode the SIFS value in the rt2x00.h file.
> >> Perhaps we should remove it in there, and no longer pass it from
> >> rt2x00lib. That way there can't be any confusion about which drivers
> >> uses the sifs field and whcih do not.
> >
> > Yes, wouldn't be too bad I guess. But all non 2800 drivers use the same sifs
> > value, so we could just leave the define in rt2x00.h and remove the sifs value
> > from the config_erp calback and use the define in all other places?
> 
> Well I would still remove the define in that case. I think some of the
> legacy drivers
> work with different SIFS values but always accepted the value which we used in
> the define. So I would still remove the define, and then each driver
> is free to set
> the value as used in the legacy drivers.

Sure. No objections from my side.

Helmut

Re: [PATCH 1/3] rt2x00: rt2800: update initial SIFS values

[Ivo Van Doorn]

On Thu, May 6, 2010 at 8:59 PM, Helmut Schaa
<helmut.schaa@googlemail.com> wrote:
> Am Donnerstag 06 Mai 2010 schrieb Ivo Van Doorn:
>> On Thu, May 6, 2010 at 8:38 PM, Helmut Schaa
>> <helmut.schaa@googlemail.com> wrote:
>> > Am Donnerstag 06 Mai 2010 schrieb Ivo Van Doorn:
>> >> On Thu, May 6, 2010 at 12:29 PM, Helmut Schaa
>> >> <helmut.schaa@googlemail.com> wrote:
>> >> > Currently the CCK and OFDM SIFS value is set to 32us. This value is neither
>> >> > used by the Ralink driver nor specified in 802.11.
>> >> >
>> >> > Instead of using 10us for CCK SIFS (as defined in 802.11) use 16us like in the
>> >> > Ralink drivers. And indeed using a SIFS value of 10us breaks connectivity with
>> >> > 11g + CTS protected connections. Add a comment to the code why we don't use 10us
>> >> > for CCK SIFS value.
>> >> >
>> >> > The OFDM SIFS value is set to 16us (as defined in 802.11 and also used by the
>> >> > Ralink drivers).
>> >>
>> >> Just wondering, but we hardcode the SIFS value in the rt2x00.h file.
>> >> Perhaps we should remove it in there, and no longer pass it from
>> >> rt2x00lib. That way there can't be any confusion about which drivers
>> >> uses the sifs field and whcih do not.
>> >
>> > Yes, wouldn't be too bad I guess. But all non 2800 drivers use the same sifs
>> > value, so we could just leave the define in rt2x00.h and remove the sifs value
>> > from the config_erp calback and use the define in all other places?
>>
>> Well I would still remove the define in that case. I think some of the
>> legacy drivers
>> work with different SIFS values but always accepted the value which we used in
>> the define. So I would still remove the define, and then each driver
>> is free to set
>> the value as used in the legacy drivers.
>
> Sure. No objections from my side.

I'll write a patch for this, this weekend along with some other updates. :)

Ivo