All of lore.kernel.org
 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
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 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.