linux-wireless.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Ivo van Doorn <ivdoorn@gmail.com>
To: Gertjan van Wingerde <gwingerde@gmail.com>
Cc: "John W. Linville" <linville@tuxdriver.com>,
	linux-wireless@vger.kernel.org, users@rt2x00.serialmonkey.com
Subject: Re: [PATCH 5/9] rt2x00: Align rt2800 register initialization with vendor driver.
Date: Fri, 9 Apr 2010 00:33:29 +0200	[thread overview]
Message-ID: <201004090033.29518.IvDoorn@gmail.com> (raw)
In-Reply-To: <1270763437-29526-6-git-send-email-gwingerde@gmail.com>

On Thursday 08 April 2010, Gertjan van Wingerde wrote:
> Align the rt2800 register initializations with the latest versions of the
> Ralink vendor driver.
> This patch is also preparation for the addition of support for RT3070 /
> RT3071 / RT3090 / RT3390 based devices.
> 
> Signed-off-by: Gertjan van Wingerde <gwingerde@gmail.com>
> ---
>  drivers/net/wireless/rt2x00/rt2800lib.c |  155 +++++++++++++++++++++++--------
>  drivers/net/wireless/rt2x00/rt2800pci.c |   13 ---
>  2 files changed, 114 insertions(+), 54 deletions(-)

> @@ -531,12 +526,8 @@ void rt2800_config_filter(struct rt2x00_dev *rt2x00dev,
>  			   !(filter_flags & FIF_PLCPFAIL));
>  	rt2x00_set_field32(&reg, RX_FILTER_CFG_DROP_NOT_TO_ME,
>  			   !(filter_flags & FIF_PROMISC_IN_BSS));
> -	rt2x00_set_field32(&reg, RX_FILTER_CFG_DROP_NOT_MY_BSSD, 0);
> -	rt2x00_set_field32(&reg, RX_FILTER_CFG_DROP_VER_ERROR, 1);
>  	rt2x00_set_field32(&reg, RX_FILTER_CFG_DROP_MULTICAST,
>  			   !(filter_flags & FIF_ALLMULTI));
> -	rt2x00_set_field32(&reg, RX_FILTER_CFG_DROP_BROADCAST, 0);
> -	rt2x00_set_field32(&reg, RX_FILTER_CFG_DROP_DUPLICATE, 1);
>  	rt2x00_set_field32(&reg, RX_FILTER_CFG_DROP_CF_END_ACK,
>  			   !(filter_flags & FIF_CONTROL));
>  	rt2x00_set_field32(&reg, RX_FILTER_CFG_DROP_CF_END,
> @@ -549,8 +540,6 @@ void rt2800_config_filter(struct rt2x00_dev *rt2x00dev,
>  			   !(filter_flags & FIF_CONTROL));
>  	rt2x00_set_field32(&reg, RX_FILTER_CFG_DROP_PSPOLL,
>  			   !(filter_flags & FIF_PSPOLL));
> -	rt2x00_set_field32(&reg, RX_FILTER_CFG_DROP_BA, 1);
> -	rt2x00_set_field32(&reg, RX_FILTER_CFG_DROP_BAR, 0);
>  	rt2x00_set_field32(&reg, RX_FILTER_CFG_DROP_CNTL,
>  			   !(filter_flags & FIF_CONTROL));
>  	rt2800_register_write(rt2x00dev, RX_FILTER_CFG, reg);

I'm not a big fan of these changes. Unless the filters don't work as expected,
I rather see the entire list of filters set to their correct values here.

> @@ -1176,6 +1179,31 @@ int rt2800_init_registers(struct rt2x00_dev *rt2x00dev)
>  	rt2x00_set_field32(&reg, BCN_TIME_CFG_TX_TIME_COMPENSATE, 0);
>  	rt2800_register_write(rt2x00dev, BCN_TIME_CFG, reg);
>  
> +	rt2800_register_read(rt2x00dev, RX_FILTER_CFG, &reg);
> +	rt2x00_set_field32(&reg, RX_FILTER_CFG_DROP_CRC_ERROR, 1);
> +	rt2x00_set_field32(&reg, RX_FILTER_CFG_DROP_PHY_ERROR, 1);
> +	rt2x00_set_field32(&reg, RX_FILTER_CFG_DROP_NOT_TO_ME, 1);
> +	rt2x00_set_field32(&reg, RX_FILTER_CFG_DROP_NOT_MY_BSSD, 0);
> +	rt2x00_set_field32(&reg, RX_FILTER_CFG_DROP_VER_ERROR, 1);
> +	rt2x00_set_field32(&reg, RX_FILTER_CFG_DROP_MULTICAST, 0);
> +	rt2x00_set_field32(&reg, RX_FILTER_CFG_DROP_BROADCAST, 0);
> +	rt2x00_set_field32(&reg, RX_FILTER_CFG_DROP_DUPLICATE, 1);
> +	rt2x00_set_field32(&reg, RX_FILTER_CFG_DROP_CF_END_ACK, 1);
> +	rt2x00_set_field32(&reg, RX_FILTER_CFG_DROP_CF_END, 1);
> +	rt2x00_set_field32(&reg, RX_FILTER_CFG_DROP_ACK, 1);
> +	rt2x00_set_field32(&reg, RX_FILTER_CFG_DROP_CTS, 1);
> +	rt2x00_set_field32(&reg, RX_FILTER_CFG_DROP_RTS, 1);
> +	rt2x00_set_field32(&reg, RX_FILTER_CFG_DROP_PSPOLL, 1);
> +	rt2x00_set_field32(&reg, RX_FILTER_CFG_DROP_BA, 1);
> +	rt2x00_set_field32(&reg, RX_FILTER_CFG_DROP_BAR, 0);
> +	rt2x00_set_field32(&reg, RX_FILTER_CFG_DROP_CNTL, 1);
> +	rt2800_register_write(rt2x00dev, RX_FILTER_CFG, reg);

Can't we just fix this by calling the config_filter() function with the proper settings?
This function is getting huge by now, so we could try to reduce it a bit.

> @@ -1214,38 +1243,61 @@ int rt2800_init_registers(struct rt2x00_dev *rt2x00dev)
>  	rt2x00_set_field32(&reg, MAX_LEN_CFG_MIN_MPDU, 0);
>  	rt2800_register_write(rt2x00dev, MAX_LEN_CFG, reg);
>  
> +	rt2800_register_read(rt2x00dev, LED_CFG, &reg);
> +	rt2x00_set_field32(&reg, LED_CFG_ON_PERIOD, 70);
> +	rt2x00_set_field32(&reg, LED_CFG_OFF_PERIOD, 30);
> +	rt2x00_set_field32(&reg, LED_CFG_SLOW_BLINK_PERIOD, 3);
> +	rt2x00_set_field32(&reg, LED_CFG_R_LED_MODE, 3);
> +	rt2x00_set_field32(&reg, LED_CFG_G_LED_MODE, 3);
> +	rt2x00_set_field32(&reg, LED_CFG_Y_LED_MODE, 3);
> +	rt2x00_set_field32(&reg, LED_CFG_LED_POLAR, 1);
> +	rt2800_register_write(rt2x00dev, LED_CFG, reg);

Same here.

>  	rt2800_register_write(rt2x00dev, PBF_MAX_PCNT, 0x1f3fbf9f);
>  
> +	rt2800_register_read(rt2x00dev, TX_RTY_CFG, &reg);
> +	rt2x00_set_field32(&reg, TX_RTY_CFG_SHORT_RTY_LIMIT, 15);
> +	rt2x00_set_field32(&reg, TX_RTY_CFG_LONG_RTY_LIMIT, 31);
> +	rt2x00_set_field32(&reg, TX_RTY_CFG_LONG_RTY_THRE, 2000);
> +	rt2x00_set_field32(&reg, TX_RTY_CFG_NON_AGG_RTY_MODE, 0);
> +	rt2x00_set_field32(&reg, TX_RTY_CFG_AGG_RTY_MODE, 0);
> +	rt2x00_set_field32(&reg, TX_RTY_CFG_TX_AUTO_FB_ENABLE, 1);
> +	rt2800_register_write(rt2x00dev, TX_RTY_CFG, reg);

And here.

> +	if (rt2x00_is_pci(rt2x00dev) || rt2x00_is_soc(rt2x00dev))
> +		rt2x00_set_field32(&reg, MM40_PROT_CFG_PROTECT_CTRL, 1);
> +	else if (rt2x00_is_usb(rt2x00dev))
> +		rt2x00_set_field32(&reg, MM40_PROT_CFG_PROTECT_CTRL, 0);

perhaps:
	rt2x00_set_field32(&reg, MM40_PROT_CFG_PROTECT_CTRL, !rt2x00_is_usb(rt2x00dev));

> @@ -1323,6 +1382,15 @@ int rt2800_init_registers(struct rt2x00_dev *rt2x00dev)
>  	rt2800_register_write(rt2x00dev, TX_RTS_CFG, reg);
>  
>  	rt2800_register_write(rt2x00dev, EXP_ACK_TIME, 0x002400ca);
> +
> +	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_OFDM_XIFS_TIME, 4);
> +	rt2x00_set_field32(&reg, XIFS_TIME_CFG_EIFS, 314);
> +	rt2x00_set_field32(&reg, XIFS_TIME_CFG_BB_RXEND_ENABLE, 1);
> +	rt2800_register_write(rt2x00dev, XIFS_TIME_CFG, reg);

Not really fond of the magical values written to the device, while mac80211
sends us the proper values later. This is probably a register initialization ordering issue?

Ivo

  reply	other threads:[~2010-04-08 22:33 UTC|newest]

Thread overview: 48+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2010-04-08 21:50 [PATCH 0/9] rt2x00: Align with vendor driver and add support for rt3070/rt3071/rt3090/rt3390 Gertjan van Wingerde
2010-04-08 21:50 ` [PATCH 1/9] rt2x00: Let RF chipset decide the RF channel switch method to use in rt2800 Gertjan van Wingerde
2010-04-08 22:17   ` Ivo van Doorn
2010-04-08 21:50 ` [PATCH 2/9] rt2x00: Update rt2800 register definitions towards latest definitions Gertjan van Wingerde
2010-04-08 22:17   ` Ivo van Doorn
2010-04-08 21:50 ` [PATCH 3/9] rt2x00: Align RT chipset definitions with vendor driver Gertjan van Wingerde
2010-04-08 22:28   ` Felix Fietkau
2010-04-09  5:10     ` Gertjan van Wingerde
2010-04-09  5:38       ` Gertjan van Wingerde
2010-04-09  7:42         ` Helmut Schaa
2010-04-09  5:43       ` Felix Fietkau
2010-04-09 11:32         ` Helmut Schaa
2010-04-09 11:52           ` Felix Fietkau
2010-04-09 12:23             ` Helmut Schaa
2010-04-09 12:26               ` Felix Fietkau
2010-04-09 12:30                 ` Luis Correia
2010-04-09 14:01                   ` Gabor Juhos
2010-04-09 12:32                 ` Helmut Schaa
2010-04-09 17:23                   ` Gertjan van Wingerde
2010-04-08 22:33   ` Ivo van Doorn
2010-04-08 21:50 ` [PATCH 4/9] rt2x00: Remove rt2800 version constants Gertjan van Wingerde
2010-04-08 22:32   ` Ivo van Doorn
2010-04-08 23:53     ` Julian Calaby
2010-04-09  6:54       ` Ivo Van Doorn
2010-04-09  7:00         ` Julian Calaby
2010-04-10  8:57           ` Gertjan van Wingerde
2010-04-10  9:16             ` Ivo van Doorn
2010-04-10 11:20             ` Julian Calaby
2010-04-09 21:53     ` [rt2x00-users] " Benoit PAPILLAULT
2010-04-10  8:57       ` Gertjan van Wingerde
2010-04-08 21:50 ` [PATCH 5/9] rt2x00: Align rt2800 register initialization with vendor driver Gertjan van Wingerde
2010-04-08 22:33   ` Ivo van Doorn [this message]
2010-04-10 19:14     ` Gertjan van Wingerde
2010-04-10 20:58       ` Ivo van Doorn
2010-04-08 21:50 ` [PATCH 6/9] rt2x00: Finish rt3070 support in rt2800 register initialization Gertjan van Wingerde
2010-04-08 22:33   ` Ivo van Doorn
2010-04-10 20:52     ` Gertjan van Wingerde
2010-04-08 21:50 ` [PATCH 7/9] rt2x00: Add rt3071 " Gertjan van Wingerde
2010-04-08 22:38   ` Ivo van Doorn
2010-04-10 21:30     ` Gertjan van Wingerde
2010-04-08 21:50 ` [PATCH 8/9] rt2x00: Add rt3090 " Gertjan van Wingerde
2010-04-08 22:39   ` Ivo van Doorn
2010-04-08 21:50 ` [PATCH 9/9] rt2x00: Add rt3390 " Gertjan van Wingerde
2010-04-08 22:39   ` Ivo van Doorn
2010-04-08 22:16 ` [PATCH 0/9] rt2x00: Align with vendor driver and add support for rt3070/rt3071/rt3090/rt3390 Ivo van Doorn
2010-04-10  9:01   ` Gertjan van Wingerde
  -- strict thread matches above, loose matches on Subject: below --
2010-04-11 12:31 [PATCH v2 " Gertjan van Wingerde
2010-04-11 12:31 ` [PATCH 5/9] rt2x00: Align rt2800 register initialization with vendor driver Gertjan van Wingerde
2010-04-11 16:56   ` Ivo van Doorn

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=201004090033.29518.IvDoorn@gmail.com \
    --to=ivdoorn@gmail.com \
    --cc=gwingerde@gmail.com \
    --cc=linux-wireless@vger.kernel.org \
    --cc=linville@tuxdriver.com \
    --cc=users@rt2x00.serialmonkey.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).