This makes the code surrounding the last few things using
IWL_SET_BITS16 actually readable and removes the macros
so nobody will ever consider using them again.
Signed-off-by: Johannes Berg <[email protected]>
---
Tested on 5000 hw.
drivers/net/wireless/iwlwifi/iwl-4965-hw.h | 82 ++++++++---------------
drivers/net/wireless/iwlwifi/iwl-4965.c | 15 ++--
drivers/net/wireless/iwlwifi/iwl-5000-hw.h | 55 +++++++--------
drivers/net/wireless/iwlwifi/iwl-5000.c | 41 ++++++-----
drivers/net/wireless/iwlwifi/iwl-helpers.h | 102 -----------------------------
drivers/net/wireless/iwlwifi/iwl-tx.c | 6 -
6 files changed, 90 insertions(+), 211 deletions(-)
--- wireless-testing.orig/drivers/net/wireless/iwlwifi/iwl-4965-hw.h 2008-10-06 18:01:57.207234025 +0200
+++ wireless-testing/drivers/net/wireless/iwlwifi/iwl-4965-hw.h 2008-10-06 18:27:22.318362726 +0200
@@ -857,18 +857,24 @@ struct iwl_tfd_addr_desc {
* A maximum of 255 (not 256!) TFDs may be on a queue waiting for Tx.
*/
struct iwl_tfd_frame {
- __le32 val0;
- /* __le32 rsvd1:24; */
- /* __le32 num_tbs:5; */
-#define IWL_num_tbs_POS 24
-#define IWL_num_tbs_LEN 5
-#define IWL_num_tbs_SYM val0
- /* __le32 rsvd2:1; */
- /* __le32 padding:2; */
+ u8 reserved1[3];
+ u8 num_tbs; /* 5 bits, rest reserved/padding */
struct iwl_tfd_addr_desc addrs[20];
__le32 reserved;
} __attribute__ ((packed));
+static inline u8 iwl_get_num_tbs(struct iwl_tfd_frame *frame)
+{
+ return frame->num_tbs & 0x1f;
+}
+
+static inline void iwl_set_num_tbs(struct iwl_tfd_frame *frame, u8 num_tbs)
+{
+ BUG_ON(num_tbs >= 20);
+
+ frame->num_tbs &= ~0x1f;
+ frame->num_tbs |= num_tbs;
+}
static inline dma_addr_t iwl_get_addr(struct iwl_tfd_frame *frame, u8 idx)
{
@@ -914,25 +920,6 @@ static inline void iwl_set_len(struct iw
/**
- * struct iwl4965_queue_byte_cnt_entry
- *
- * Byte Count Table Entry
- *
- * Bit fields:
- * 15-12: reserved
- * 11- 0: total to-be-transmitted byte count of frame (does not include command)
- */
-struct iwl4965_queue_byte_cnt_entry {
- __le16 val;
- /* __le16 byte_cnt:12; */
-#define IWL_byte_cnt_POS 0
-#define IWL_byte_cnt_LEN 12
-#define IWL_byte_cnt_SYM val
- /* __le16 rsvd:4; */
-} __attribute__ ((packed));
-
-
-/**
* struct iwl4965_sched_queue_byte_cnt_tbl
*
* Byte Count table
@@ -951,13 +938,23 @@ struct iwl4965_queue_byte_cnt_entry {
* 4965 assumes tables are separated by 1024 bytes.
*/
struct iwl4965_sched_queue_byte_cnt_tbl {
- struct iwl4965_queue_byte_cnt_entry tfd_offset[IWL49_QUEUE_SIZE +
- IWL49_MAX_WIN_SIZE];
+ /* highest 4 bits of each entry are reserved */
+ __le16 _tfd_offset[IWL49_QUEUE_SIZE + IWL49_MAX_WIN_SIZE];
u8 dont_care[1024 -
(IWL49_QUEUE_SIZE + IWL49_MAX_WIN_SIZE) *
sizeof(__le16)];
} __attribute__ ((packed));
+static inline void
+iwl4965_queue_byte_cnt_set(struct iwl4965_sched_queue_byte_cnt_tbl *tbl,
+ u32 idx, u16 cnt)
+{
+ BUG_ON(idx >= IWL49_QUEUE_SIZE + IWL49_MAX_WIN_SIZE);
+ BUG_ON(cnt > 0xFFF);
+ tbl->_tfd_offset[idx] &= cpu_to_le16(~0xFFF);
+ tbl->_tfd_offset[idx] |= cpu_to_le16(cnt);
+}
+
/**
* struct iwl4965_shared - handshake area for Tx and Rx
@@ -985,30 +982,11 @@ struct iwl4965_sched_queue_byte_cnt_tbl
struct iwl4965_shared {
struct iwl4965_sched_queue_byte_cnt_tbl
queues_byte_cnt_tbls[IWL49_NUM_QUEUES];
- __le32 rb_closed;
+ __le16 rb_closed_stts_rb_num_rsvd; /* high 4 bits reserved */
+ __le16 rb_closed_stts_rx_frame_num_rsvd; /* high 4 bits reserved */
- /* __le32 rb_closed_stts_rb_num:12; */
-#define IWL_rb_closed_stts_rb_num_POS 0
-#define IWL_rb_closed_stts_rb_num_LEN 12
-#define IWL_rb_closed_stts_rb_num_SYM rb_closed
- /* __le32 rsrv1:4; */
- /* __le32 rb_closed_stts_rx_frame_num:12; */
-#define IWL_rb_closed_stts_rx_frame_num_POS 16
-#define IWL_rb_closed_stts_rx_frame_num_LEN 12
-#define IWL_rb_closed_stts_rx_frame_num_SYM rb_closed
- /* __le32 rsrv2:4; */
-
- __le32 frm_finished;
- /* __le32 frame_finished_stts_rb_num:12; */
-#define IWL_frame_finished_stts_rb_num_POS 0
-#define IWL_frame_finished_stts_rb_num_LEN 12
-#define IWL_frame_finished_stts_rb_num_SYM frm_finished
- /* __le32 rsrv3:4; */
- /* __le32 frame_finished_stts_rx_frame_num:12; */
-#define IWL_frame_finished_stts_rx_frame_num_POS 16
-#define IWL_frame_finished_stts_rx_frame_num_LEN 12
-#define IWL_frame_finished_stts_rx_frame_num_SYM frm_finished
- /* __le32 rsrv4:4; */
+ __le16 frm_finished_stts_rb_num_rsvd; /* high 4 bits reserved */
+ __le16 frm_finished_stts_rx_frame_num_rsvd; /* high 4 bits rsvd */
__le32 padding1; /* so that allocation will be aligned to 16B */
__le32 padding2;
--- wireless-testing.orig/drivers/net/wireless/iwlwifi/iwl-tx.c 2008-10-06 18:01:30.487483581 +0200
+++ wireless-testing/drivers/net/wireless/iwlwifi/iwl-tx.c 2008-10-06 18:05:49.150109045 +0200
@@ -76,7 +76,7 @@ static void iwl_hw_txq_free_tfd(struct i
return;
/* Sanity check on number of chunks */
- counter = IWL_GET_BITS(*bd, num_tbs);
+ counter = iwl_get_num_tbs(bd);
if (counter >= MAX_NUM_OF_TBS) {
IWL_ERROR("Too many chunks: %i\n", counter);
/* @todo issue fatal error, it is quite serious situation */
@@ -97,7 +97,7 @@ static int iwl_hw_txq_attach_buf_to_tfd(
dma_addr_t addr, u16 len)
{
struct iwl_tfd_frame *tfd = ptr;
- u32 num_tbs = IWL_GET_BITS(*tfd, num_tbs);
+ u32 num_tbs = iwl_get_num_tbs(tfd);
/* Each TFD can point to a maximum 20 Tx buffers */
if (num_tbs >= MAX_NUM_OF_TBS) {
@@ -109,7 +109,7 @@ static int iwl_hw_txq_attach_buf_to_tfd(
iwl_set_addr(tfd, num_tbs, addr);
iwl_set_len(tfd, num_tbs, len);
- IWL_SET_BITS(*tfd, num_tbs, num_tbs + 1);
+ iwl_set_num_tbs(tfd, num_tbs + 1);
return 0;
}
--- wireless-testing.orig/drivers/net/wireless/iwlwifi/iwl-4965.c 2008-10-06 18:15:26.712108096 +0200
+++ wireless-testing/drivers/net/wireless/iwlwifi/iwl-4965.c 2008-10-06 18:28:03.086201127 +0200
@@ -1653,7 +1653,7 @@ static int iwl4965_hw_channel_switch(str
static int iwl4965_shared_mem_rx_idx(struct iwl_priv *priv)
{
struct iwl4965_shared *s = priv->shared_virt;
- return le32_to_cpu(s->rb_closed) & 0xFFF;
+ return le16_to_cpu(s->rb_closed_stts_rb_num_rsvd) & 0xFFF;
}
static int iwl4965_alloc_shared_mem(struct iwl_priv *priv)
@@ -1666,7 +1666,8 @@ static int iwl4965_alloc_shared_mem(stru
memset(priv->shared_virt, 0, sizeof(struct iwl4965_shared));
- priv->rb_closed_offset = offsetof(struct iwl4965_shared, rb_closed);
+ priv->rb_closed_offset = offsetof(struct iwl4965_shared,
+ rb_closed_stts_rb_num_rsvd);
return 0;
}
@@ -1694,14 +1695,14 @@ static void iwl4965_txq_update_byte_cnt_
len = byte_cnt + IWL_TX_CRC_SIZE + IWL_TX_DELIMITER_SIZE;
/* Set up byte count within first 256 entries */
- IWL_SET_BITS16(shared_data->queues_byte_cnt_tbls[txq_id].
- tfd_offset[txq->q.write_ptr], byte_cnt, len);
+ iwl4965_queue_byte_cnt_set(&shared_data->queues_byte_cnt_tbls[txq_id],
+ txq->q.write_ptr, len);
/* If within first 64 entries, duplicate at end */
if (txq->q.write_ptr < IWL49_MAX_WIN_SIZE)
- IWL_SET_BITS16(shared_data->queues_byte_cnt_tbls[txq_id].
- tfd_offset[IWL49_QUEUE_SIZE + txq->q.write_ptr],
- byte_cnt, len);
+ iwl4965_queue_byte_cnt_set(
+ &shared_data->queues_byte_cnt_tbls[txq_id],
+ IWL49_QUEUE_SIZE + txq->q.write_ptr, len);
}
/**
--- wireless-testing.orig/drivers/net/wireless/iwlwifi/iwl-5000-hw.h 2008-10-06 18:09:21.375233932 +0200
+++ wireless-testing/drivers/net/wireless/iwlwifi/iwl-5000-hw.h 2008-10-06 18:32:50.180236365 +0200
@@ -84,46 +84,43 @@
#define IWL50_NUM_AMPDU_QUEUES 10
#define IWL50_FIRST_AMPDU_QUEUE 10
-#define IWL_sta_id_POS 12
-#define IWL_sta_id_LEN 4
-#define IWL_sta_id_SYM val
-
/* Fixed (non-configurable) rx data from phy */
/* Base physical address of iwl5000_shared is provided to SCD_DRAM_BASE_ADDR
* and &iwl5000_shared.val0 is provided to FH_RSCSR_CHNL0_STTS_WPTR_REG */
struct iwl5000_sched_queue_byte_cnt_tbl {
- struct iwl4965_queue_byte_cnt_entry tfd_offset[IWL50_QUEUE_SIZE +
- IWL50_MAX_WIN_SIZE];
+ /* highest 4 bits of each entry are sta ID */
+ __le16 _tfd_offset[IWL50_QUEUE_SIZE + IWL50_MAX_WIN_SIZE];
} __attribute__ ((packed));
+static inline void
+iwl5000_queue_byte_cnt_set(struct iwl5000_sched_queue_byte_cnt_tbl *tbl,
+ u32 idx, u16 cnt)
+{
+ BUG_ON(idx >= IWL50_QUEUE_SIZE + IWL50_MAX_WIN_SIZE);
+ BUG_ON(cnt > 0xFFF);
+ tbl->_tfd_offset[idx] &= cpu_to_le16(~0xFFF);
+ tbl->_tfd_offset[idx] |= cpu_to_le16(cnt);
+}
+
+static inline void
+iwl5000_queue_sta_id_set(struct iwl5000_sched_queue_byte_cnt_tbl *tbl,
+ u32 idx, u16 sta_id)
+{
+ BUG_ON(idx >= IWL50_QUEUE_SIZE + IWL50_MAX_WIN_SIZE);
+ BUG_ON(sta_id > 0xF);
+ tbl->_tfd_offset[idx] &= cpu_to_le16(0xFFF);
+ tbl->_tfd_offset[idx] |= cpu_to_le16(sta_id << 12);
+}
+
struct iwl5000_shared {
struct iwl5000_sched_queue_byte_cnt_tbl
queues_byte_cnt_tbls[IWL50_NUM_QUEUES];
- __le32 rb_closed;
+ __le16 rb_closed_stts_rb_num_rsvd; /* high 4 bits reserved */
+ __le16 rb_closed_stts_rx_frame_num_rsvd; /* high 4 bits reserved */
- /* __le32 rb_closed_stts_rb_num:12; */
-#define IWL_rb_closed_stts_rb_num_POS 0
-#define IWL_rb_closed_stts_rb_num_LEN 12
-#define IWL_rb_closed_stts_rb_num_SYM rb_closed
- /* __le32 rsrv1:4; */
- /* __le32 rb_closed_stts_rx_frame_num:12; */
-#define IWL_rb_closed_stts_rx_frame_num_POS 16
-#define IWL_rb_closed_stts_rx_frame_num_LEN 12
-#define IWL_rb_closed_stts_rx_frame_num_SYM rb_closed
- /* __le32 rsrv2:4; */
-
- __le32 frm_finished;
- /* __le32 frame_finished_stts_rb_num:12; */
-#define IWL_frame_finished_stts_rb_num_POS 0
-#define IWL_frame_finished_stts_rb_num_LEN 12
-#define IWL_frame_finished_stts_rb_num_SYM frm_finished
- /* __le32 rsrv3:4; */
- /* __le32 frame_finished_stts_rx_frame_num:12; */
-#define IWL_frame_finished_stts_rx_frame_num_POS 16
-#define IWL_frame_finished_stts_rx_frame_num_LEN 12
-#define IWL_frame_finished_stts_rx_frame_num_SYM frm_finished
- /* __le32 rsrv4:4; */
+ __le16 frm_finished_stts_rb_num_rsvd; /* high 4 bits reserved */
+ __le16 frm_finished_stts_rx_frame_num_rsvd; /* high 4 bits rsvd */
__le32 padding1; /* so that allocation will be aligned to 16B */
__le32 padding2;
--- wireless-testing.orig/drivers/net/wireless/iwlwifi/iwl-5000.c 2008-10-06 18:09:08.752483265 +0200
+++ wireless-testing/drivers/net/wireless/iwlwifi/iwl-5000.c 2008-10-06 18:32:08.379109332 +0200
@@ -856,7 +856,8 @@ static int iwl5000_alloc_shared_mem(stru
memset(priv->shared_virt, 0, sizeof(struct iwl5000_shared));
- priv->rb_closed_offset = offsetof(struct iwl5000_shared, rb_closed);
+ priv->rb_closed_offset = offsetof(struct iwl5000_shared,
+ rb_closed_stts_rb_num_rsvd);
return 0;
}
@@ -873,7 +874,7 @@ static void iwl5000_free_shared_mem(stru
static int iwl5000_shared_mem_rx_idx(struct iwl_priv *priv)
{
struct iwl5000_shared *s = priv->shared_virt;
- return le32_to_cpu(s->rb_closed) & 0xFFF;
+ return le16_to_cpu(s->rb_closed_stts_rb_num_rsvd) & 0xFFF;
}
/**
@@ -908,19 +909,18 @@ static void iwl5000_txq_update_byte_cnt_
}
}
- IWL_SET_BITS16(shared_data->queues_byte_cnt_tbls[txq_id].
- tfd_offset[txq->q.write_ptr], byte_cnt, len);
-
- IWL_SET_BITS16(shared_data->queues_byte_cnt_tbls[txq_id].
- tfd_offset[txq->q.write_ptr], sta_id, sta);
+ iwl5000_queue_byte_cnt_set(&shared_data->queues_byte_cnt_tbls[txq_id],
+ txq->q.write_ptr, len);
+ iwl5000_queue_sta_id_set(&shared_data->queues_byte_cnt_tbls[txq_id],
+ txq->q.write_ptr, sta);
if (txq->q.write_ptr < IWL50_MAX_WIN_SIZE) {
- IWL_SET_BITS16(shared_data->queues_byte_cnt_tbls[txq_id].
- tfd_offset[IWL50_QUEUE_SIZE + txq->q.write_ptr],
- byte_cnt, len);
- IWL_SET_BITS16(shared_data->queues_byte_cnt_tbls[txq_id].
- tfd_offset[IWL50_QUEUE_SIZE + txq->q.write_ptr],
- sta_id, sta);
+ iwl5000_queue_byte_cnt_set(
+ &shared_data->queues_byte_cnt_tbls[txq_id],
+ IWL50_QUEUE_SIZE + txq->q.write_ptr, len);
+ iwl5000_queue_sta_id_set(
+ &shared_data->queues_byte_cnt_tbls[txq_id],
+ IWL50_QUEUE_SIZE + txq->q.write_ptr, sta);
}
}
@@ -934,13 +934,18 @@ static void iwl5000_txq_inval_byte_cnt_t
if (txq_id != IWL_CMD_QUEUE_NUM)
sta = txq->cmd[txq->q.read_ptr]->cmd.tx.sta_id;
- shared_data->queues_byte_cnt_tbls[txq_id].tfd_offset[txq->q.read_ptr].
- val = cpu_to_le16(1 | (sta << 12));
+ iwl5000_queue_byte_cnt_set(&shared_data->queues_byte_cnt_tbls[txq_id],
+ txq->q.read_ptr, 1);
+ iwl5000_queue_sta_id_set(&shared_data->queues_byte_cnt_tbls[txq_id],
+ txq->q.read_ptr, sta);
if (txq->q.write_ptr < IWL50_MAX_WIN_SIZE) {
- shared_data->queues_byte_cnt_tbls[txq_id].
- tfd_offset[IWL50_QUEUE_SIZE + txq->q.read_ptr].
- val = cpu_to_le16(1 | (sta << 12));
+ iwl5000_queue_byte_cnt_set(
+ &shared_data->queues_byte_cnt_tbls[txq_id],
+ IWL50_QUEUE_SIZE + txq->q.read_ptr, 1);
+ iwl5000_queue_sta_id_set(
+ &shared_data->queues_byte_cnt_tbls[txq_id],
+ IWL50_QUEUE_SIZE + txq->q.read_ptr, sta);
}
}
--- wireless-testing.orig/drivers/net/wireless/iwlwifi/iwl-helpers.h 2008-10-06 18:01:02.250108736 +0200
+++ wireless-testing/drivers/net/wireless/iwlwifi/iwl-helpers.h 2008-10-06 18:06:18.639110868 +0200
@@ -32,108 +32,6 @@
#include <linux/ctype.h>
-/*
- * The structures defined by the hardware/uCode interface
- * have bit-wise operations. For each bit-field there is
- * a data symbol in the structure, the start bit position
- * and the length of the bit-field.
- *
- * iwl_get_bits and iwl_set_bits will return or set the
- * appropriate bits on a 32-bit value.
- *
- * IWL_GET_BITS and IWL_SET_BITS use symbol expansion to
- * expand out to the appropriate call to iwl_get_bits
- * and iwl_set_bits without having to reference all of the
- * numerical constants and defines provided in the hardware
- * definition
- */
-
-/**
- * iwl_get_bits - Extract a hardware bit-field value
- * @src: source hardware value (__le32)
- * @pos: bit-position (0-based) of first bit of value
- * @len: length of bit-field
- *
- * iwl_get_bits will return the bit-field in cpu endian ordering.
- *
- * NOTE: If used from IWL_GET_BITS then pos and len are compile-constants and
- * will collapse to minimal code by the compiler.
- */
-static inline u32 iwl_get_bits(__le32 src, u8 pos, u8 len)
-{
- u32 tmp = le32_to_cpu(src);
-
- tmp >>= pos;
- tmp &= (1UL << len) - 1;
- return tmp;
-}
-
-/**
- * iwl_set_bits - Set a hardware bit-field value
- * @dst: Address of __le32 hardware value
- * @pos: bit-position (0-based) of first bit of value
- * @len: length of bit-field
- * @val: cpu endian value to encode into the bit-field
- *
- * iwl_set_bits will encode val into dst, masked to be len bits long at bit
- * position pos.
- *
- * NOTE: If used IWL_SET_BITS pos and len will be compile-constants and
- * will collapse to minimal code by the compiler.
- */
-static inline void iwl_set_bits(__le32 *dst, u8 pos, u8 len, int val)
-{
- u32 tmp = le32_to_cpu(*dst);
-
- tmp &= ~(((1UL << len) - 1) << pos);
- tmp |= (val & ((1UL << len) - 1)) << pos;
- *dst = cpu_to_le32(tmp);
-}
-
-static inline void iwl_set_bits16(__le16 *dst, u8 pos, u8 len, int val)
-{
- u16 tmp = le16_to_cpu(*dst);
-
- tmp &= ~((1UL << (pos + len)) - (1UL << pos));
- tmp |= (val & ((1UL << len) - 1)) << pos;
- *dst = cpu_to_le16(tmp);
-}
-
-/*
- * The bit-field definitions in iwl-xxxx-hw.h are in the form of:
- *
- * struct example {
- * __le32 val1;
- * #define IWL_name_POS 8
- * #define IWL_name_LEN 4
- * #define IWL_name_SYM val1
- * };
- *
- * The IWL_SET_BITS and IWL_GET_BITS macros are provided to allow the driver
- * to call:
- *
- * struct example bar;
- * u32 val = IWL_GET_BITS(bar, name);
- * val = val * 2;
- * IWL_SET_BITS(bar, name, val);
- *
- * All cpu / host ordering, masking, and shifts are performed by the macros
- * and iwl_{get,set}_bits.
- *
- */
-#define IWL_SET_BITS(s, sym, v) \
- iwl_set_bits(&(s).IWL_ ## sym ## _SYM, IWL_ ## sym ## _POS, \
- IWL_ ## sym ## _LEN, (v))
-
-#define IWL_SET_BITS16(s, sym, v) \
- iwl_set_bits16(&(s).IWL_ ## sym ## _SYM, IWL_ ## sym ## _POS, \
- IWL_ ## sym ## _LEN, (v))
-
-#define IWL_GET_BITS(s, sym) \
- iwl_get_bits((s).IWL_ ## sym ## _SYM, IWL_ ## sym ## _POS, \
- IWL_ ## sym ## _LEN)
-
-
#define KELVIN_TO_CELSIUS(x) ((x)-273)
#define CELSIUS_TO_KELVIN(x) ((x)+273)
#define IWL_MASK(lo, hi) ((1 << (hi)) | ((1 << (hi)) - (1 << (lo))))
On Tue, Oct 7, 2008 at 6:58 PM, Johannes Berg <[email protected]> wrote:
> On Tue, 2008-10-07 at 16:03 +0200, Tomas Winkler wrote:
>
>> > Now, can we discuss the patch rather than Johannes' intemperate
>> > word choice?
>>
>> With Johannes permissions I will split the patch into chunks first so
>> I can take smaller steps second
>> it will better fit into patches I have in pipe.
>
> Fine with me, can you adopt the first one too that cleans up the DMA
> code?
Yes I handling that too.
And please keep the 10 pairs -> 20 addresses, it's a lot easier to
> understand :)
I don't like on this solution two things one is that it 32 bit
variables sits on unaligned address and second
it doesn't mach HW description which defines this as descriptor couple
Tomas
On Tue, 2008-10-07 at 22:01 +0200, Tomas Winkler wrote:
> On Tue, Oct 7, 2008 at 9:34 PM, Johannes Berg <[email protected]> wrote:
> > On Tue, 2008-10-07 at 21:28 +0200, Tomas Winkler wrote:
> >
> >> > And please keep the 10 pairs -> 20 addresses, it's a lot easier to
> >> > understand :)
> >
> >> I don't like on this solution two things one is that it 32 bit
> >> variables sits on unaligned address
> >
> > That doesn't matter at all since it's packed, and the bitwise accesses
> > you did previously are way less efficient.
>
> This is not packet this is shared memory.
??
The struct is _packed_, nothing to do with packets.
johannes
On Wed, 2008-10-08 at 14:38 +0200, Tomas Winkler wrote:
> >> > Also, keep in mind that address 2 is _never_ used at all anyway
> >
> >> This is not correct we always use first 2 pointers. First for tx command
> >> second for the actual packet.
> >
> > Ok so you use two.
>
> My only point is just to make sure that you understand that __never__
> is not correct
Right, sorry, I really should have checked.
> > Writing three words in total. I strongly suggest that
> > you have WAY more trouble in iwlwifi than an imagined performance issue
> > coming from a corrected and understandable struct layout.
>
> You are rally trying breaking into open doors Currently I'm more
> concern with correctness then performance so I wanted to rise
> hopefully all issues.
> I'm testing your layout it's work so far in my home setup. I'm on
> holidays till EOW so I will be able to give it some more stress in Lab
> only next week.
Sure, that's fine, I can't really see what can go wrong but testing is
always good. It just seemed to me that for some reason you preferred
keeping the pair-layout and complicating the code?
johannes
On Mon, Oct 06, 2008 at 10:24:07PM +0200, drago01 wrote:
> > did I miss an internal joke here? I fail to see the cursing ;)
>
> he is talking about the "crap" in the subject ;)
In that case I'd recommend he gets a little thicker skin. We're not a
bunch of cinderallas.
On Tue, 2008-10-07 at 22:46 +0200, Tomas Winkler wrote:
> >> > bytes (with nibbles): | . | . | . | . | . | . || . | . | . | . | . | . |
> >> > | addr 1 |len1 || addr 2 |len2 |
> >> > your forced layout: | | | |
> >> > my layout: | | || | |
> >> >
> >> > Note how this actually matches the border between the border between the
> >> > two descriptors, the || border.
> >>
> >> Your layout put addr2 (which is 32 bit) sits on 16 bit boundary
> >
> > So? Yours makes it need two words, which is even less efficient to
> > access. And since the struct is packed, on those architectures that
> > don't do unaligned accesses efficiently won't need any put_unaligned
> > either.
> >
> > Also, keep in mind that address 2 is _never_ used at all anyway
> This is not correct we always use first 2 pointers. First for tx command
> second for the actual packet.
Ok so you use two. Writing three words in total. I strongly suggest that
you have WAY more trouble in iwlwifi than an imagined performance issue
coming from a corrected and understandable struct layout.
> > because
> > we don't and cannot do fragmented frames due to the lack of IP
> > checksumming in hardware, as I and davem have tried to explain to you
> > multiple times already.
>
> This is bad side of current Linux implementation
Look, we've tried to explain it to you so many times that it simply is
not possible to enable fragment collection without IP checksumming in
hardware that I don't know what to tell you. This may sound offensive,
but are you really too stupid to understand it?
johannes
On Wed, Oct 8, 2008 at 10:02 AM, Johannes Berg
<[email protected]> wrote:
> On Tue, 2008-10-07 at 22:46 +0200, Tomas Winkler wrote:
>> >> > bytes (with nibbles): | . | . | . | . | . | . || . | . | . | . | . | . |
>> >> > | addr 1 |len1 || addr 2 |len2 |
>> >> > your forced layout: | | | |
>> >> > my layout: | | || | |
>> >> >
>> >> > Note how this actually matches the border between the border between the
>> >> > two descriptors, the || border.
>> >>
>> >> Your layout put addr2 (which is 32 bit) sits on 16 bit boundary
>> >
>> > So? Yours makes it need two words, which is even less efficient to
>> > access. And since the struct is packed, on those architectures that
>> > don't do unaligned accesses efficiently won't need any put_unaligned
>> > either.
>> >
>> > Also, keep in mind that address 2 is _never_ used at all anyway
>
>> This is not correct we always use first 2 pointers. First for tx command
>> second for the actual packet.
>
> Ok so you use two.
My only point is just to make sure that you understand that __never__
is not correct
Writing three words in total. I strongly suggest that
> you have WAY more trouble in iwlwifi than an imagined performance issue
> coming from a corrected and understandable struct layout.
You are rally trying breaking into open doors Currently I'm more
concern with correctness then performance so I wanted to rise
hopefully all issues.
I'm testing your layout it's work so far in my home setup. I'm on
holidays till EOW so I will be able to give it some more stress in Lab
only next week.
>
>> > because
>> > we don't and cannot do fragmented frames due to the lack of IP
>> > checksumming in hardware, as I and davem have tried to explain to you
>> > multiple times already.
>>
>> This is bad side of current Linux implementation
>
> Look, we've tried to explain it to you so many times that it simply is
> not possible to enable fragment collection without IP checksumming in
> hardware that I don't know what to tell you. This may sound offensive,
> but are you really too stupid to understand it?
Okay not fair from my side I just couldn't resist ... I just really
like the subject :)
Tomas
> --- wireless-testing.orig/drivers/net/wireless/iwlwifi/iwl-5000-hw.h 2008-10-06 18:09:21.375233932 +0200
> +++ wireless-testing/drivers/net/wireless/iwlwifi/iwl-5000-hw.h 2008-10-06 18:32:50.180236365 +0200
> @@ -84,46 +84,43 @@
> #define IWL50_NUM_AMPDU_QUEUES 10
> #define IWL50_FIRST_AMPDU_QUEUE 10
>
> -#define IWL_sta_id_POS 12
> -#define IWL_sta_id_LEN 4
> -#define IWL_sta_id_SYM val
> -
> /* Fixed (non-configurable) rx data from phy */
>
> /* Base physical address of iwl5000_shared is provided to SCD_DRAM_BASE_ADDR
> * and &iwl5000_shared.val0 is provided to FH_RSCSR_CHNL0_STTS_WPTR_REG */
> struct iwl5000_sched_queue_byte_cnt_tbl {
> - struct iwl4965_queue_byte_cnt_entry tfd_offset[IWL50_QUEUE_SIZE +
> - IWL50_MAX_WIN_SIZE];
> + /* highest 4 bits of each entry are sta ID */
> + __le16 _tfd_offset[IWL50_QUEUE_SIZE + IWL50_MAX_WIN_SIZE];
> } __attribute__ ((packed));
>
> +static inline void
> +iwl5000_queue_byte_cnt_set(struct iwl5000_sched_queue_byte_cnt_tbl *tbl,
> + u32 idx, u16 cnt)
> +{
> + BUG_ON(idx >= IWL50_QUEUE_SIZE + IWL50_MAX_WIN_SIZE);
I just hit that BUG_ON, which means that something is passing a bogus
value in. If I'm right and this isn't a bug in my patch (I cannot see
how it should be) then it means that will corrupt memory silently
without my patch. FWIW, the index is 511, which is totally wrong, being
called from iwl5000_txq_inval_byte_cnt_tbl.
I'd much appreciate if somebody would review this patch. I think it's
right and the added sanity check blows up because of other bugs, but
obviously I cannot guarantee that I didn't make a stupid mistake.
johannes
On Tue, Oct 07, 2008 at 03:15:29AM -0400, Christoph Hellwig wrote:
> On Tue, Oct 07, 2008 at 08:21:56AM +0200, Holger Schurig wrote:
> > > I don't want these macros eithers but I NACK any patches with
> > > cursing in the commit logs.
> >
> > I fully agree. There's no need to speak about "crap" when
> > changing other people's code. In some cultures thats quite
> > insulting.
>
> Well, it is crap. Looks at it objectively. It's a perfectly fine
> description and most of us use it perfectly happily for our own code,
> too.
I agree that this is at worst a little bit rude. The patch should be
rejected or merged on its own basis, although I do reserve the right
to edit a changelog from time to time... :-)
Now, can we discuss the patch rather than Johannes' intemperate
word choice?
John
--
John W. Linville Linux should be at the core
[email protected] of your literate lifestyle.
On Tue, 2008-10-07 at 21:28 +0200, Tomas Winkler wrote:
> > And please keep the 10 pairs -> 20 addresses, it's a lot easier to
> > understand :)
> I don't like on this solution two things one is that it 32 bit
> variables sits on unaligned address
That doesn't matter at all since it's packed, and the bitwise accesses
you did previously are way less efficient.
> and second
> it doesn't mach HW description which defines this as descriptor couple
Then tell the HW engineers to fix the HW docs. It makes no sense at all
to say this is 10 pairs rather than 20 single ones if there's nothing
that uses a pair, the whole thing entirely uses single entries in the
numbering etc.
johannes
Hi Tomas,
> > This makes the code surrounding the last few things using
> > IWL_SET_BITS16 actually readable and removes the macros
> > so nobody will ever consider using them again.
> >
> > Signed-off-by: Johannes Berg <[email protected]>
>
> I don't want these macros eithers but I NACK any patches with cursing
> in the commit logs.
did I miss an internal joke here? I fail to see the cursing ;)
Regards
Marcel
> I don't want these macros eithers but I NACK any patches with
> cursing in the commit logs.
I fully agree. There's no need to speak about "crap" when
changing other people's code. In some cultures thats quite
insulting.
This makes the code surrounding the last few things using
IWL_SET_BITS16 actually readable and removes the macros
so nobody will ever consider using them again.
Signed-off-by: Johannes Berg <[email protected]>
---
Tested on 5000 hw.
v2: don't BUG_ON an invalid index because it actually happens!
drivers/net/wireless/iwlwifi/iwl-4965-hw.h | 85 +++++++++---------------
drivers/net/wireless/iwlwifi/iwl-4965.c | 15 ++--
drivers/net/wireless/iwlwifi/iwl-5000-hw.h | 61 +++++++++--------
drivers/net/wireless/iwlwifi/iwl-5000.c | 41 ++++++-----
drivers/net/wireless/iwlwifi/iwl-helpers.h | 102 -----------------------------
drivers/net/wireless/iwlwifi/iwl-tx.c | 6 -
6 files changed, 99 insertions(+), 211 deletions(-)
--- wireless-testing.orig/drivers/net/wireless/iwlwifi/iwl-4965-hw.h 2008-10-06 18:01:57.207234025 +0200
+++ wireless-testing/drivers/net/wireless/iwlwifi/iwl-4965-hw.h 2008-10-06 19:35:09.488701911 +0200
@@ -857,18 +857,24 @@ struct iwl_tfd_addr_desc {
* A maximum of 255 (not 256!) TFDs may be on a queue waiting for Tx.
*/
struct iwl_tfd_frame {
- __le32 val0;
- /* __le32 rsvd1:24; */
- /* __le32 num_tbs:5; */
-#define IWL_num_tbs_POS 24
-#define IWL_num_tbs_LEN 5
-#define IWL_num_tbs_SYM val0
- /* __le32 rsvd2:1; */
- /* __le32 padding:2; */
+ u8 reserved1[3];
+ u8 num_tbs; /* 5 bits, rest reserved/padding */
struct iwl_tfd_addr_desc addrs[20];
__le32 reserved;
} __attribute__ ((packed));
+static inline u8 iwl_get_num_tbs(struct iwl_tfd_frame *frame)
+{
+ return frame->num_tbs & 0x1f;
+}
+
+static inline void iwl_set_num_tbs(struct iwl_tfd_frame *frame, u8 num_tbs)
+{
+ BUG_ON(num_tbs >= 20);
+
+ frame->num_tbs &= ~0x1f;
+ frame->num_tbs |= num_tbs;
+}
static inline dma_addr_t iwl_get_addr(struct iwl_tfd_frame *frame, u8 idx)
{
@@ -914,25 +920,6 @@ static inline void iwl_set_len(struct iw
/**
- * struct iwl4965_queue_byte_cnt_entry
- *
- * Byte Count Table Entry
- *
- * Bit fields:
- * 15-12: reserved
- * 11- 0: total to-be-transmitted byte count of frame (does not include command)
- */
-struct iwl4965_queue_byte_cnt_entry {
- __le16 val;
- /* __le16 byte_cnt:12; */
-#define IWL_byte_cnt_POS 0
-#define IWL_byte_cnt_LEN 12
-#define IWL_byte_cnt_SYM val
- /* __le16 rsvd:4; */
-} __attribute__ ((packed));
-
-
-/**
* struct iwl4965_sched_queue_byte_cnt_tbl
*
* Byte Count table
@@ -951,13 +938,26 @@ struct iwl4965_queue_byte_cnt_entry {
* 4965 assumes tables are separated by 1024 bytes.
*/
struct iwl4965_sched_queue_byte_cnt_tbl {
- struct iwl4965_queue_byte_cnt_entry tfd_offset[IWL49_QUEUE_SIZE +
- IWL49_MAX_WIN_SIZE];
+ /* highest 4 bits of each entry are reserved */
+ __le16 _tfd_offset[IWL49_QUEUE_SIZE + IWL49_MAX_WIN_SIZE];
u8 dont_care[1024 -
(IWL49_QUEUE_SIZE + IWL49_MAX_WIN_SIZE) *
sizeof(__le16)];
} __attribute__ ((packed));
+static inline void
+iwl4965_queue_byte_cnt_set(struct iwl4965_sched_queue_byte_cnt_tbl *tbl,
+ u32 idx, u16 cnt)
+{
+ if (WARN_ON(idx >= IWL49_QUEUE_SIZE + IWL49_MAX_WIN_SIZE)) {
+ printk(KERN_DEBUG "idx is %d\n", idx);
+ return;
+ }
+ BUG_ON(cnt > 0xFFF);
+ tbl->_tfd_offset[idx] &= cpu_to_le16(~0xFFF);
+ tbl->_tfd_offset[idx] |= cpu_to_le16(cnt);
+}
+
/**
* struct iwl4965_shared - handshake area for Tx and Rx
@@ -985,30 +985,11 @@ struct iwl4965_sched_queue_byte_cnt_tbl
struct iwl4965_shared {
struct iwl4965_sched_queue_byte_cnt_tbl
queues_byte_cnt_tbls[IWL49_NUM_QUEUES];
- __le32 rb_closed;
+ __le16 rb_closed_stts_rb_num_rsvd; /* high 4 bits reserved */
+ __le16 rb_closed_stts_rx_frame_num_rsvd; /* high 4 bits reserved */
- /* __le32 rb_closed_stts_rb_num:12; */
-#define IWL_rb_closed_stts_rb_num_POS 0
-#define IWL_rb_closed_stts_rb_num_LEN 12
-#define IWL_rb_closed_stts_rb_num_SYM rb_closed
- /* __le32 rsrv1:4; */
- /* __le32 rb_closed_stts_rx_frame_num:12; */
-#define IWL_rb_closed_stts_rx_frame_num_POS 16
-#define IWL_rb_closed_stts_rx_frame_num_LEN 12
-#define IWL_rb_closed_stts_rx_frame_num_SYM rb_closed
- /* __le32 rsrv2:4; */
-
- __le32 frm_finished;
- /* __le32 frame_finished_stts_rb_num:12; */
-#define IWL_frame_finished_stts_rb_num_POS 0
-#define IWL_frame_finished_stts_rb_num_LEN 12
-#define IWL_frame_finished_stts_rb_num_SYM frm_finished
- /* __le32 rsrv3:4; */
- /* __le32 frame_finished_stts_rx_frame_num:12; */
-#define IWL_frame_finished_stts_rx_frame_num_POS 16
-#define IWL_frame_finished_stts_rx_frame_num_LEN 12
-#define IWL_frame_finished_stts_rx_frame_num_SYM frm_finished
- /* __le32 rsrv4:4; */
+ __le16 frm_finished_stts_rb_num_rsvd; /* high 4 bits reserved */
+ __le16 frm_finished_stts_rx_frame_num_rsvd; /* high 4 bits rsvd */
__le32 padding1; /* so that allocation will be aligned to 16B */
__le32 padding2;
--- wireless-testing.orig/drivers/net/wireless/iwlwifi/iwl-tx.c 2008-10-06 18:01:30.487483581 +0200
+++ wireless-testing/drivers/net/wireless/iwlwifi/iwl-tx.c 2008-10-06 18:05:49.150109045 +0200
@@ -76,7 +76,7 @@ static void iwl_hw_txq_free_tfd(struct i
return;
/* Sanity check on number of chunks */
- counter = IWL_GET_BITS(*bd, num_tbs);
+ counter = iwl_get_num_tbs(bd);
if (counter >= MAX_NUM_OF_TBS) {
IWL_ERROR("Too many chunks: %i\n", counter);
/* @todo issue fatal error, it is quite serious situation */
@@ -97,7 +97,7 @@ static int iwl_hw_txq_attach_buf_to_tfd(
dma_addr_t addr, u16 len)
{
struct iwl_tfd_frame *tfd = ptr;
- u32 num_tbs = IWL_GET_BITS(*tfd, num_tbs);
+ u32 num_tbs = iwl_get_num_tbs(tfd);
/* Each TFD can point to a maximum 20 Tx buffers */
if (num_tbs >= MAX_NUM_OF_TBS) {
@@ -109,7 +109,7 @@ static int iwl_hw_txq_attach_buf_to_tfd(
iwl_set_addr(tfd, num_tbs, addr);
iwl_set_len(tfd, num_tbs, len);
- IWL_SET_BITS(*tfd, num_tbs, num_tbs + 1);
+ iwl_set_num_tbs(tfd, num_tbs + 1);
return 0;
}
--- wireless-testing.orig/drivers/net/wireless/iwlwifi/iwl-4965.c 2008-10-06 18:15:26.712108096 +0200
+++ wireless-testing/drivers/net/wireless/iwlwifi/iwl-4965.c 2008-10-06 18:28:03.086201127 +0200
@@ -1653,7 +1653,7 @@ static int iwl4965_hw_channel_switch(str
static int iwl4965_shared_mem_rx_idx(struct iwl_priv *priv)
{
struct iwl4965_shared *s = priv->shared_virt;
- return le32_to_cpu(s->rb_closed) & 0xFFF;
+ return le16_to_cpu(s->rb_closed_stts_rb_num_rsvd) & 0xFFF;
}
static int iwl4965_alloc_shared_mem(struct iwl_priv *priv)
@@ -1666,7 +1666,8 @@ static int iwl4965_alloc_shared_mem(stru
memset(priv->shared_virt, 0, sizeof(struct iwl4965_shared));
- priv->rb_closed_offset = offsetof(struct iwl4965_shared, rb_closed);
+ priv->rb_closed_offset = offsetof(struct iwl4965_shared,
+ rb_closed_stts_rb_num_rsvd);
return 0;
}
@@ -1694,14 +1695,14 @@ static void iwl4965_txq_update_byte_cnt_
len = byte_cnt + IWL_TX_CRC_SIZE + IWL_TX_DELIMITER_SIZE;
/* Set up byte count within first 256 entries */
- IWL_SET_BITS16(shared_data->queues_byte_cnt_tbls[txq_id].
- tfd_offset[txq->q.write_ptr], byte_cnt, len);
+ iwl4965_queue_byte_cnt_set(&shared_data->queues_byte_cnt_tbls[txq_id],
+ txq->q.write_ptr, len);
/* If within first 64 entries, duplicate at end */
if (txq->q.write_ptr < IWL49_MAX_WIN_SIZE)
- IWL_SET_BITS16(shared_data->queues_byte_cnt_tbls[txq_id].
- tfd_offset[IWL49_QUEUE_SIZE + txq->q.write_ptr],
- byte_cnt, len);
+ iwl4965_queue_byte_cnt_set(
+ &shared_data->queues_byte_cnt_tbls[txq_id],
+ IWL49_QUEUE_SIZE + txq->q.write_ptr, len);
}
/**
--- wireless-testing.orig/drivers/net/wireless/iwlwifi/iwl-5000-hw.h 2008-10-06 18:09:21.375233932 +0200
+++ wireless-testing/drivers/net/wireless/iwlwifi/iwl-5000-hw.h 2008-10-06 19:28:17.739667564 +0200
@@ -84,46 +84,49 @@
#define IWL50_NUM_AMPDU_QUEUES 10
#define IWL50_FIRST_AMPDU_QUEUE 10
-#define IWL_sta_id_POS 12
-#define IWL_sta_id_LEN 4
-#define IWL_sta_id_SYM val
-
/* Fixed (non-configurable) rx data from phy */
/* Base physical address of iwl5000_shared is provided to SCD_DRAM_BASE_ADDR
* and &iwl5000_shared.val0 is provided to FH_RSCSR_CHNL0_STTS_WPTR_REG */
struct iwl5000_sched_queue_byte_cnt_tbl {
- struct iwl4965_queue_byte_cnt_entry tfd_offset[IWL50_QUEUE_SIZE +
- IWL50_MAX_WIN_SIZE];
+ /* highest 4 bits of each entry are sta ID */
+ __le16 _tfd_offset[IWL50_QUEUE_SIZE + IWL50_MAX_WIN_SIZE];
} __attribute__ ((packed));
+static inline void
+iwl5000_queue_byte_cnt_set(struct iwl5000_sched_queue_byte_cnt_tbl *tbl,
+ u32 idx, u16 cnt)
+{
+ if (WARN_ON(idx >= IWL50_QUEUE_SIZE + IWL50_MAX_WIN_SIZE)) {
+ printk(KERN_DEBUG "idx is %d\n", idx);
+ return;
+ }
+ BUG_ON(cnt > 0xFFF);
+ tbl->_tfd_offset[idx] &= cpu_to_le16(~0xFFF);
+ tbl->_tfd_offset[idx] |= cpu_to_le16(cnt);
+}
+
+static inline void
+iwl5000_queue_sta_id_set(struct iwl5000_sched_queue_byte_cnt_tbl *tbl,
+ u32 idx, u16 sta_id)
+{
+ if (WARN_ON(idx >= IWL50_QUEUE_SIZE + IWL50_MAX_WIN_SIZE)) {
+ printk(KERN_DEBUG "idx is %d\n", idx);
+ return;
+ }
+ BUG_ON(sta_id > 0xF);
+ tbl->_tfd_offset[idx] &= cpu_to_le16(0xFFF);
+ tbl->_tfd_offset[idx] |= cpu_to_le16(sta_id << 12);
+}
+
struct iwl5000_shared {
struct iwl5000_sched_queue_byte_cnt_tbl
queues_byte_cnt_tbls[IWL50_NUM_QUEUES];
- __le32 rb_closed;
+ __le16 rb_closed_stts_rb_num_rsvd; /* high 4 bits reserved */
+ __le16 rb_closed_stts_rx_frame_num_rsvd; /* high 4 bits reserved */
- /* __le32 rb_closed_stts_rb_num:12; */
-#define IWL_rb_closed_stts_rb_num_POS 0
-#define IWL_rb_closed_stts_rb_num_LEN 12
-#define IWL_rb_closed_stts_rb_num_SYM rb_closed
- /* __le32 rsrv1:4; */
- /* __le32 rb_closed_stts_rx_frame_num:12; */
-#define IWL_rb_closed_stts_rx_frame_num_POS 16
-#define IWL_rb_closed_stts_rx_frame_num_LEN 12
-#define IWL_rb_closed_stts_rx_frame_num_SYM rb_closed
- /* __le32 rsrv2:4; */
-
- __le32 frm_finished;
- /* __le32 frame_finished_stts_rb_num:12; */
-#define IWL_frame_finished_stts_rb_num_POS 0
-#define IWL_frame_finished_stts_rb_num_LEN 12
-#define IWL_frame_finished_stts_rb_num_SYM frm_finished
- /* __le32 rsrv3:4; */
- /* __le32 frame_finished_stts_rx_frame_num:12; */
-#define IWL_frame_finished_stts_rx_frame_num_POS 16
-#define IWL_frame_finished_stts_rx_frame_num_LEN 12
-#define IWL_frame_finished_stts_rx_frame_num_SYM frm_finished
- /* __le32 rsrv4:4; */
+ __le16 frm_finished_stts_rb_num_rsvd; /* high 4 bits reserved */
+ __le16 frm_finished_stts_rx_frame_num_rsvd; /* high 4 bits rsvd */
__le32 padding1; /* so that allocation will be aligned to 16B */
__le32 padding2;
--- wireless-testing.orig/drivers/net/wireless/iwlwifi/iwl-5000.c 2008-10-06 18:09:08.752483265 +0200
+++ wireless-testing/drivers/net/wireless/iwlwifi/iwl-5000.c 2008-10-06 18:32:08.379109332 +0200
@@ -856,7 +856,8 @@ static int iwl5000_alloc_shared_mem(stru
memset(priv->shared_virt, 0, sizeof(struct iwl5000_shared));
- priv->rb_closed_offset = offsetof(struct iwl5000_shared, rb_closed);
+ priv->rb_closed_offset = offsetof(struct iwl5000_shared,
+ rb_closed_stts_rb_num_rsvd);
return 0;
}
@@ -873,7 +874,7 @@ static void iwl5000_free_shared_mem(stru
static int iwl5000_shared_mem_rx_idx(struct iwl_priv *priv)
{
struct iwl5000_shared *s = priv->shared_virt;
- return le32_to_cpu(s->rb_closed) & 0xFFF;
+ return le16_to_cpu(s->rb_closed_stts_rb_num_rsvd) & 0xFFF;
}
/**
@@ -908,19 +909,18 @@ static void iwl5000_txq_update_byte_cnt_
}
}
- IWL_SET_BITS16(shared_data->queues_byte_cnt_tbls[txq_id].
- tfd_offset[txq->q.write_ptr], byte_cnt, len);
-
- IWL_SET_BITS16(shared_data->queues_byte_cnt_tbls[txq_id].
- tfd_offset[txq->q.write_ptr], sta_id, sta);
+ iwl5000_queue_byte_cnt_set(&shared_data->queues_byte_cnt_tbls[txq_id],
+ txq->q.write_ptr, len);
+ iwl5000_queue_sta_id_set(&shared_data->queues_byte_cnt_tbls[txq_id],
+ txq->q.write_ptr, sta);
if (txq->q.write_ptr < IWL50_MAX_WIN_SIZE) {
- IWL_SET_BITS16(shared_data->queues_byte_cnt_tbls[txq_id].
- tfd_offset[IWL50_QUEUE_SIZE + txq->q.write_ptr],
- byte_cnt, len);
- IWL_SET_BITS16(shared_data->queues_byte_cnt_tbls[txq_id].
- tfd_offset[IWL50_QUEUE_SIZE + txq->q.write_ptr],
- sta_id, sta);
+ iwl5000_queue_byte_cnt_set(
+ &shared_data->queues_byte_cnt_tbls[txq_id],
+ IWL50_QUEUE_SIZE + txq->q.write_ptr, len);
+ iwl5000_queue_sta_id_set(
+ &shared_data->queues_byte_cnt_tbls[txq_id],
+ IWL50_QUEUE_SIZE + txq->q.write_ptr, sta);
}
}
@@ -934,13 +934,18 @@ static void iwl5000_txq_inval_byte_cnt_t
if (txq_id != IWL_CMD_QUEUE_NUM)
sta = txq->cmd[txq->q.read_ptr]->cmd.tx.sta_id;
- shared_data->queues_byte_cnt_tbls[txq_id].tfd_offset[txq->q.read_ptr].
- val = cpu_to_le16(1 | (sta << 12));
+ iwl5000_queue_byte_cnt_set(&shared_data->queues_byte_cnt_tbls[txq_id],
+ txq->q.read_ptr, 1);
+ iwl5000_queue_sta_id_set(&shared_data->queues_byte_cnt_tbls[txq_id],
+ txq->q.read_ptr, sta);
if (txq->q.write_ptr < IWL50_MAX_WIN_SIZE) {
- shared_data->queues_byte_cnt_tbls[txq_id].
- tfd_offset[IWL50_QUEUE_SIZE + txq->q.read_ptr].
- val = cpu_to_le16(1 | (sta << 12));
+ iwl5000_queue_byte_cnt_set(
+ &shared_data->queues_byte_cnt_tbls[txq_id],
+ IWL50_QUEUE_SIZE + txq->q.read_ptr, 1);
+ iwl5000_queue_sta_id_set(
+ &shared_data->queues_byte_cnt_tbls[txq_id],
+ IWL50_QUEUE_SIZE + txq->q.read_ptr, sta);
}
}
--- wireless-testing.orig/drivers/net/wireless/iwlwifi/iwl-helpers.h 2008-10-06 18:01:02.250108736 +0200
+++ wireless-testing/drivers/net/wireless/iwlwifi/iwl-helpers.h 2008-10-06 18:06:18.639110868 +0200
@@ -32,108 +32,6 @@
#include <linux/ctype.h>
-/*
- * The structures defined by the hardware/uCode interface
- * have bit-wise operations. For each bit-field there is
- * a data symbol in the structure, the start bit position
- * and the length of the bit-field.
- *
- * iwl_get_bits and iwl_set_bits will return or set the
- * appropriate bits on a 32-bit value.
- *
- * IWL_GET_BITS and IWL_SET_BITS use symbol expansion to
- * expand out to the appropriate call to iwl_get_bits
- * and iwl_set_bits without having to reference all of the
- * numerical constants and defines provided in the hardware
- * definition
- */
-
-/**
- * iwl_get_bits - Extract a hardware bit-field value
- * @src: source hardware value (__le32)
- * @pos: bit-position (0-based) of first bit of value
- * @len: length of bit-field
- *
- * iwl_get_bits will return the bit-field in cpu endian ordering.
- *
- * NOTE: If used from IWL_GET_BITS then pos and len are compile-constants and
- * will collapse to minimal code by the compiler.
- */
-static inline u32 iwl_get_bits(__le32 src, u8 pos, u8 len)
-{
- u32 tmp = le32_to_cpu(src);
-
- tmp >>= pos;
- tmp &= (1UL << len) - 1;
- return tmp;
-}
-
-/**
- * iwl_set_bits - Set a hardware bit-field value
- * @dst: Address of __le32 hardware value
- * @pos: bit-position (0-based) of first bit of value
- * @len: length of bit-field
- * @val: cpu endian value to encode into the bit-field
- *
- * iwl_set_bits will encode val into dst, masked to be len bits long at bit
- * position pos.
- *
- * NOTE: If used IWL_SET_BITS pos and len will be compile-constants and
- * will collapse to minimal code by the compiler.
- */
-static inline void iwl_set_bits(__le32 *dst, u8 pos, u8 len, int val)
-{
- u32 tmp = le32_to_cpu(*dst);
-
- tmp &= ~(((1UL << len) - 1) << pos);
- tmp |= (val & ((1UL << len) - 1)) << pos;
- *dst = cpu_to_le32(tmp);
-}
-
-static inline void iwl_set_bits16(__le16 *dst, u8 pos, u8 len, int val)
-{
- u16 tmp = le16_to_cpu(*dst);
-
- tmp &= ~((1UL << (pos + len)) - (1UL << pos));
- tmp |= (val & ((1UL << len) - 1)) << pos;
- *dst = cpu_to_le16(tmp);
-}
-
-/*
- * The bit-field definitions in iwl-xxxx-hw.h are in the form of:
- *
- * struct example {
- * __le32 val1;
- * #define IWL_name_POS 8
- * #define IWL_name_LEN 4
- * #define IWL_name_SYM val1
- * };
- *
- * The IWL_SET_BITS and IWL_GET_BITS macros are provided to allow the driver
- * to call:
- *
- * struct example bar;
- * u32 val = IWL_GET_BITS(bar, name);
- * val = val * 2;
- * IWL_SET_BITS(bar, name, val);
- *
- * All cpu / host ordering, masking, and shifts are performed by the macros
- * and iwl_{get,set}_bits.
- *
- */
-#define IWL_SET_BITS(s, sym, v) \
- iwl_set_bits(&(s).IWL_ ## sym ## _SYM, IWL_ ## sym ## _POS, \
- IWL_ ## sym ## _LEN, (v))
-
-#define IWL_SET_BITS16(s, sym, v) \
- iwl_set_bits16(&(s).IWL_ ## sym ## _SYM, IWL_ ## sym ## _POS, \
- IWL_ ## sym ## _LEN, (v))
-
-#define IWL_GET_BITS(s, sym) \
- iwl_get_bits((s).IWL_ ## sym ## _SYM, IWL_ ## sym ## _POS, \
- IWL_ ## sym ## _LEN)
-
-
#define KELVIN_TO_CELSIUS(x) ((x)-273)
#define CELSIUS_TO_KELVIN(x) ((x)+273)
#define IWL_MASK(lo, hi) ((1 << (hi)) | ((1 << (hi)) - (1 << (lo))))
On Tue, 2008-10-07 at 21:34 +0200, Johannes Berg wrote:
> Then tell the HW engineers to fix the HW docs. It makes no sense at all
> to say this is 10 pairs rather than 20 single ones if there's nothing
> that uses a pair, the whole thing entirely uses single entries in the
> numbering etc.
If you need to visualise this, let me draw it:
bytes (with nibbles): | . | . | . | . | . | . || . | . | . | . | . | . |
| addr 1 |len1 || addr 2 |len2 |
your forced layout: | | | |
my layout: | | || | |
Note how this actually matches the border between the border between the
two descriptors, the || border.
_Please_, just fix it, this is _way_ easier than the forced layout
you're doing.
johannes
On Tue, Oct 7, 2008 at 10:06 PM, Johannes Berg
<[email protected]> wrote:
> On Tue, 2008-10-07 at 22:01 +0200, Tomas Winkler wrote:
>> On Tue, Oct 7, 2008 at 9:34 PM, Johannes Berg <[email protected]> wrote:
>> > On Tue, 2008-10-07 at 21:28 +0200, Tomas Winkler wrote:
>> >
>> >> > And please keep the 10 pairs -> 20 addresses, it's a lot easier to
>> >> > understand :)
>> >
>> >> I don't like on this solution two things one is that it 32 bit
>> >> variables sits on unaligned address
>> >
>> > That doesn't matter at all since it's packed, and the bitwise accesses
>> > you did previously are way less efficient.
>>
>> This is not packet this is shared memory.
>
> ??
> The struct is _packed_, nothing to do with packets.
I'm tired .. :)
On Tue, Oct 7, 2008 at 9:39 PM, Johannes Berg <[email protected]> wrote:
> On Tue, 2008-10-07 at 21:34 +0200, Johannes Berg wrote:
>
>> Then tell the HW engineers to fix the HW docs. It makes no sense at all
>> to say this is 10 pairs rather than 20 single ones if there's nothing
>> that uses a pair, the whole thing entirely uses single entries in the
>> numbering etc.
>
> If you need to visualise this, let me draw it:
>
> bytes (with nibbles): | . | . | . | . | . | . || . | . | . | . | . | . |
> | addr 1 |len1 || addr 2 |len2 |
> your forced layout: | | | |
> my layout: | | || | |
>
> Note how this actually matches the border between the border between the
> two descriptors, the || border.
Your layout put addr2 (which is 32 bit) sits on 16 bit boundary
Tomas
On Thu, Oct 9, 2008 at 11:52 AM, Johannes Berg
<[email protected]> wrote:
> On Wed, 2008-10-08 at 14:38 +0200, Tomas Winkler wrote:
>
>> >> > Also, keep in mind that address 2 is _never_ used at all anyway
>> >
>> >> This is not correct we always use first 2 pointers. First for tx command
>> >> second for the actual packet.
>> >
>> > Ok so you use two.
>>
>> My only point is just to make sure that you understand that __never__
>> is not correct
>
> Right, sorry, I really should have checked.
>
>> > Writing three words in total. I strongly suggest that
>> > you have WAY more trouble in iwlwifi than an imagined performance issue
>> > coming from a corrected and understandable struct layout.
>>
>> You are rally trying breaking into open doors Currently I'm more
>> concern with correctness then performance so I wanted to rise
>> hopefully all issues.
>> I'm testing your layout it's work so far in my home setup. I'm on
>> holidays till EOW so I will be able to give it some more stress in Lab
>> only next week.
>
> Sure, that's fine, I can't really see what can go wrong but testing is
> always good.
Not everything can be seen I hoped you've learned that already.
People tend to only test their code whether it fixes the particular
problem they want to fix not seeing the collateral bugs they are
creating on the way.
But there hundred of books about the subject.
It just seemed to me that for some reason you preferred
> keeping the pair-layout and complicating the code?
Don't take assumptions, people communication is most flawed protocol.
Tomas
Tomas
On Tue, 2008-10-07 at 16:03 +0200, Tomas Winkler wrote:
> > Now, can we discuss the patch rather than Johannes' intemperate
> > word choice?
>
> With Johannes permissions I will split the patch into chunks first so
> I can take smaller steps second
> it will better fit into patches I have in pipe.
Fine with me, can you adopt the first one too that cleans up the DMA
code? And please keep the 10 pairs -> 20 addresses, it's a lot easier to
understand :)
johannes
On Mon, 2008-10-06 at 20:18 +0200, Marcel Holtmann wrote:
> > +static inline u8 iwl_get_num_tbs(struct iwl_tfd_frame *frame)
> > +{
> > + return frame->num_tbs & 0x1f;
> > +}
> > +
> > +static inline void iwl_set_num_tbs(struct iwl_tfd_frame *frame, u8 num_tbs)
> > +{
> > + BUG_ON(num_tbs >= 20);
> > +
> > + frame->num_tbs &= ~0x1f;
> > + frame->num_tbs |= num_tbs;
> > +}
>
> what is this magic doing and what is wrong with
>
> frame->num_tbs = num_tbs & 0x1f
>
> Or do you expect to do something with the upper 3 bits?
I don't know. I was just keeping the original semantics. Maybe future hw
will use those bits? Maybe they are already used and need to be
preserved? This hw programming stuff in the driver is all black magic to
me.
> Personally I think it is important that we always zero any data
> structure and especially the reserved bits of it. Otherwise stuff just
> magically seems to work, but it is pure luck if the right bits are set.
Well, that doesn't just affect that but all other reserved fields too,
so we really need to kzalloc everything anyway.
> The overall patch looks good to me and I think it is the way to go. Less
> home grown bit magic the better.
:)
johannes
On Mon, Oct 6, 2008 at 9:12 PM, Marcel Holtmann
<[email protected]> wrote:
> Hi Tomas,
>
>> > This makes the code surrounding the last few things using
>> > IWL_SET_BITS16 actually readable and removes the macros
>> > so nobody will ever consider using them again.
>> >
>> > Signed-off-by: Johannes Berg <[email protected]>
>>
>> I don't want these macros eithers but I NACK any patches with cursing
>> in the commit logs.
>
> did I miss an internal joke here? I fail to see the cursing ;)
he is talking about the "crap" in the subject ;)
On Tue, 2008-10-07 at 22:06 +0200, Tomas Winkler wrote:
> > bytes (with nibbles): | . | . | . | . | . | . || . | . | . | . | . | . |
> > | addr 1 |len1 || addr 2 |len2 |
> > your forced layout: | | | |
> > my layout: | | || | |
> >
> > Note how this actually matches the border between the border between the
> > two descriptors, the || border.
>
> Your layout put addr2 (which is 32 bit) sits on 16 bit boundary
So? Yours makes it need two words, which is even less efficient to
access. And since the struct is packed, on those architectures that
don't do unaligned accesses efficiently won't need any put_unaligned
either.
Also, keep in mind that address 2 is _never_ used at all anyway because
we don't and cannot do fragmented frames due to the lack of IP
checksumming in hardware, as I and davem have tried to explain to you
multiple times already.
johannes
On Mon, Oct 6, 2008 at 6:50 PM, Johannes Berg <[email protected]> wrote:
> This makes the code surrounding the last few things using
> IWL_SET_BITS16 actually readable and removes the macros
> so nobody will ever consider using them again.
>
> Signed-off-by: Johannes Berg <[email protected]>
I don't want these macros eithers but I NACK any patches with cursing
in the commit logs.
Thanks
Tomas
> ---
> Tested on 5000 hw.
>
> drivers/net/wireless/iwlwifi/iwl-4965-hw.h | 82 ++++++++---------------
> drivers/net/wireless/iwlwifi/iwl-4965.c | 15 ++--
> drivers/net/wireless/iwlwifi/iwl-5000-hw.h | 55 +++++++--------
> drivers/net/wireless/iwlwifi/iwl-5000.c | 41 ++++++-----
> drivers/net/wireless/iwlwifi/iwl-helpers.h | 102 -----------------------------
> drivers/net/wireless/iwlwifi/iwl-tx.c | 6 -
> 6 files changed, 90 insertions(+), 211 deletions(-)
>
> --- wireless-testing.orig/drivers/net/wireless/iwlwifi/iwl-4965-hw.h 2008-10-06 18:01:57.207234025 +0200
> +++ wireless-testing/drivers/net/wireless/iwlwifi/iwl-4965-hw.h 2008-10-06 18:27:22.318362726 +0200
> @@ -857,18 +857,24 @@ struct iwl_tfd_addr_desc {
> * A maximum of 255 (not 256!) TFDs may be on a queue waiting for Tx.
> */
> struct iwl_tfd_frame {
> - __le32 val0;
> - /* __le32 rsvd1:24; */
> - /* __le32 num_tbs:5; */
> -#define IWL_num_tbs_POS 24
> -#define IWL_num_tbs_LEN 5
> -#define IWL_num_tbs_SYM val0
> - /* __le32 rsvd2:1; */
> - /* __le32 padding:2; */
> + u8 reserved1[3];
> + u8 num_tbs; /* 5 bits, rest reserved/padding */
> struct iwl_tfd_addr_desc addrs[20];
> __le32 reserved;
> } __attribute__ ((packed));
>
> +static inline u8 iwl_get_num_tbs(struct iwl_tfd_frame *frame)
> +{
> + return frame->num_tbs & 0x1f;
> +}
> +
> +static inline void iwl_set_num_tbs(struct iwl_tfd_frame *frame, u8 num_tbs)
> +{
> + BUG_ON(num_tbs >= 20);
> +
> + frame->num_tbs &= ~0x1f;
> + frame->num_tbs |= num_tbs;
> +}
>
> static inline dma_addr_t iwl_get_addr(struct iwl_tfd_frame *frame, u8 idx)
> {
> @@ -914,25 +920,6 @@ static inline void iwl_set_len(struct iw
>
>
> /**
> - * struct iwl4965_queue_byte_cnt_entry
> - *
> - * Byte Count Table Entry
> - *
> - * Bit fields:
> - * 15-12: reserved
> - * 11- 0: total to-be-transmitted byte count of frame (does not include command)
> - */
> -struct iwl4965_queue_byte_cnt_entry {
> - __le16 val;
> - /* __le16 byte_cnt:12; */
> -#define IWL_byte_cnt_POS 0
> -#define IWL_byte_cnt_LEN 12
> -#define IWL_byte_cnt_SYM val
> - /* __le16 rsvd:4; */
> -} __attribute__ ((packed));
> -
> -
> -/**
> * struct iwl4965_sched_queue_byte_cnt_tbl
> *
> * Byte Count table
> @@ -951,13 +938,23 @@ struct iwl4965_queue_byte_cnt_entry {
> * 4965 assumes tables are separated by 1024 bytes.
> */
> struct iwl4965_sched_queue_byte_cnt_tbl {
> - struct iwl4965_queue_byte_cnt_entry tfd_offset[IWL49_QUEUE_SIZE +
> - IWL49_MAX_WIN_SIZE];
> + /* highest 4 bits of each entry are reserved */
> + __le16 _tfd_offset[IWL49_QUEUE_SIZE + IWL49_MAX_WIN_SIZE];
> u8 dont_care[1024 -
> (IWL49_QUEUE_SIZE + IWL49_MAX_WIN_SIZE) *
> sizeof(__le16)];
> } __attribute__ ((packed));
>
> +static inline void
> +iwl4965_queue_byte_cnt_set(struct iwl4965_sched_queue_byte_cnt_tbl *tbl,
> + u32 idx, u16 cnt)
> +{
> + BUG_ON(idx >= IWL49_QUEUE_SIZE + IWL49_MAX_WIN_SIZE);
> + BUG_ON(cnt > 0xFFF);
> + tbl->_tfd_offset[idx] &= cpu_to_le16(~0xFFF);
> + tbl->_tfd_offset[idx] |= cpu_to_le16(cnt);
> +}
> +
>
> /**
> * struct iwl4965_shared - handshake area for Tx and Rx
> @@ -985,30 +982,11 @@ struct iwl4965_sched_queue_byte_cnt_tbl
> struct iwl4965_shared {
> struct iwl4965_sched_queue_byte_cnt_tbl
> queues_byte_cnt_tbls[IWL49_NUM_QUEUES];
> - __le32 rb_closed;
> + __le16 rb_closed_stts_rb_num_rsvd; /* high 4 bits reserved */
> + __le16 rb_closed_stts_rx_frame_num_rsvd; /* high 4 bits reserved */
>
> - /* __le32 rb_closed_stts_rb_num:12; */
> -#define IWL_rb_closed_stts_rb_num_POS 0
> -#define IWL_rb_closed_stts_rb_num_LEN 12
> -#define IWL_rb_closed_stts_rb_num_SYM rb_closed
> - /* __le32 rsrv1:4; */
> - /* __le32 rb_closed_stts_rx_frame_num:12; */
> -#define IWL_rb_closed_stts_rx_frame_num_POS 16
> -#define IWL_rb_closed_stts_rx_frame_num_LEN 12
> -#define IWL_rb_closed_stts_rx_frame_num_SYM rb_closed
> - /* __le32 rsrv2:4; */
> -
> - __le32 frm_finished;
> - /* __le32 frame_finished_stts_rb_num:12; */
> -#define IWL_frame_finished_stts_rb_num_POS 0
> -#define IWL_frame_finished_stts_rb_num_LEN 12
> -#define IWL_frame_finished_stts_rb_num_SYM frm_finished
> - /* __le32 rsrv3:4; */
> - /* __le32 frame_finished_stts_rx_frame_num:12; */
> -#define IWL_frame_finished_stts_rx_frame_num_POS 16
> -#define IWL_frame_finished_stts_rx_frame_num_LEN 12
> -#define IWL_frame_finished_stts_rx_frame_num_SYM frm_finished
> - /* __le32 rsrv4:4; */
> + __le16 frm_finished_stts_rb_num_rsvd; /* high 4 bits reserved */
> + __le16 frm_finished_stts_rx_frame_num_rsvd; /* high 4 bits rsvd */
>
> __le32 padding1; /* so that allocation will be aligned to 16B */
> __le32 padding2;
> --- wireless-testing.orig/drivers/net/wireless/iwlwifi/iwl-tx.c 2008-10-06 18:01:30.487483581 +0200
> +++ wireless-testing/drivers/net/wireless/iwlwifi/iwl-tx.c 2008-10-06 18:05:49.150109045 +0200
> @@ -76,7 +76,7 @@ static void iwl_hw_txq_free_tfd(struct i
> return;
>
> /* Sanity check on number of chunks */
> - counter = IWL_GET_BITS(*bd, num_tbs);
> + counter = iwl_get_num_tbs(bd);
> if (counter >= MAX_NUM_OF_TBS) {
> IWL_ERROR("Too many chunks: %i\n", counter);
> /* @todo issue fatal error, it is quite serious situation */
> @@ -97,7 +97,7 @@ static int iwl_hw_txq_attach_buf_to_tfd(
> dma_addr_t addr, u16 len)
> {
> struct iwl_tfd_frame *tfd = ptr;
> - u32 num_tbs = IWL_GET_BITS(*tfd, num_tbs);
> + u32 num_tbs = iwl_get_num_tbs(tfd);
>
> /* Each TFD can point to a maximum 20 Tx buffers */
> if (num_tbs >= MAX_NUM_OF_TBS) {
> @@ -109,7 +109,7 @@ static int iwl_hw_txq_attach_buf_to_tfd(
> iwl_set_addr(tfd, num_tbs, addr);
> iwl_set_len(tfd, num_tbs, len);
>
> - IWL_SET_BITS(*tfd, num_tbs, num_tbs + 1);
> + iwl_set_num_tbs(tfd, num_tbs + 1);
>
> return 0;
> }
> --- wireless-testing.orig/drivers/net/wireless/iwlwifi/iwl-4965.c 2008-10-06 18:15:26.712108096 +0200
> +++ wireless-testing/drivers/net/wireless/iwlwifi/iwl-4965.c 2008-10-06 18:28:03.086201127 +0200
> @@ -1653,7 +1653,7 @@ static int iwl4965_hw_channel_switch(str
> static int iwl4965_shared_mem_rx_idx(struct iwl_priv *priv)
> {
> struct iwl4965_shared *s = priv->shared_virt;
> - return le32_to_cpu(s->rb_closed) & 0xFFF;
> + return le16_to_cpu(s->rb_closed_stts_rb_num_rsvd) & 0xFFF;
> }
>
> static int iwl4965_alloc_shared_mem(struct iwl_priv *priv)
> @@ -1666,7 +1666,8 @@ static int iwl4965_alloc_shared_mem(stru
>
> memset(priv->shared_virt, 0, sizeof(struct iwl4965_shared));
>
> - priv->rb_closed_offset = offsetof(struct iwl4965_shared, rb_closed);
> + priv->rb_closed_offset = offsetof(struct iwl4965_shared,
> + rb_closed_stts_rb_num_rsvd);
>
> return 0;
> }
> @@ -1694,14 +1695,14 @@ static void iwl4965_txq_update_byte_cnt_
> len = byte_cnt + IWL_TX_CRC_SIZE + IWL_TX_DELIMITER_SIZE;
>
> /* Set up byte count within first 256 entries */
> - IWL_SET_BITS16(shared_data->queues_byte_cnt_tbls[txq_id].
> - tfd_offset[txq->q.write_ptr], byte_cnt, len);
> + iwl4965_queue_byte_cnt_set(&shared_data->queues_byte_cnt_tbls[txq_id],
> + txq->q.write_ptr, len);
>
> /* If within first 64 entries, duplicate at end */
> if (txq->q.write_ptr < IWL49_MAX_WIN_SIZE)
> - IWL_SET_BITS16(shared_data->queues_byte_cnt_tbls[txq_id].
> - tfd_offset[IWL49_QUEUE_SIZE + txq->q.write_ptr],
> - byte_cnt, len);
> + iwl4965_queue_byte_cnt_set(
> + &shared_data->queues_byte_cnt_tbls[txq_id],
> + IWL49_QUEUE_SIZE + txq->q.write_ptr, len);
> }
>
> /**
> --- wireless-testing.orig/drivers/net/wireless/iwlwifi/iwl-5000-hw.h 2008-10-06 18:09:21.375233932 +0200
> +++ wireless-testing/drivers/net/wireless/iwlwifi/iwl-5000-hw.h 2008-10-06 18:32:50.180236365 +0200
> @@ -84,46 +84,43 @@
> #define IWL50_NUM_AMPDU_QUEUES 10
> #define IWL50_FIRST_AMPDU_QUEUE 10
>
> -#define IWL_sta_id_POS 12
> -#define IWL_sta_id_LEN 4
> -#define IWL_sta_id_SYM val
> -
> /* Fixed (non-configurable) rx data from phy */
>
> /* Base physical address of iwl5000_shared is provided to SCD_DRAM_BASE_ADDR
> * and &iwl5000_shared.val0 is provided to FH_RSCSR_CHNL0_STTS_WPTR_REG */
> struct iwl5000_sched_queue_byte_cnt_tbl {
> - struct iwl4965_queue_byte_cnt_entry tfd_offset[IWL50_QUEUE_SIZE +
> - IWL50_MAX_WIN_SIZE];
> + /* highest 4 bits of each entry are sta ID */
> + __le16 _tfd_offset[IWL50_QUEUE_SIZE + IWL50_MAX_WIN_SIZE];
> } __attribute__ ((packed));
>
> +static inline void
> +iwl5000_queue_byte_cnt_set(struct iwl5000_sched_queue_byte_cnt_tbl *tbl,
> + u32 idx, u16 cnt)
> +{
> + BUG_ON(idx >= IWL50_QUEUE_SIZE + IWL50_MAX_WIN_SIZE);
> + BUG_ON(cnt > 0xFFF);
> + tbl->_tfd_offset[idx] &= cpu_to_le16(~0xFFF);
> + tbl->_tfd_offset[idx] |= cpu_to_le16(cnt);
> +}
> +
> +static inline void
> +iwl5000_queue_sta_id_set(struct iwl5000_sched_queue_byte_cnt_tbl *tbl,
> + u32 idx, u16 sta_id)
> +{
> + BUG_ON(idx >= IWL50_QUEUE_SIZE + IWL50_MAX_WIN_SIZE);
> + BUG_ON(sta_id > 0xF);
> + tbl->_tfd_offset[idx] &= cpu_to_le16(0xFFF);
> + tbl->_tfd_offset[idx] |= cpu_to_le16(sta_id << 12);
> +}
> +
> struct iwl5000_shared {
> struct iwl5000_sched_queue_byte_cnt_tbl
> queues_byte_cnt_tbls[IWL50_NUM_QUEUES];
> - __le32 rb_closed;
> + __le16 rb_closed_stts_rb_num_rsvd; /* high 4 bits reserved */
> + __le16 rb_closed_stts_rx_frame_num_rsvd; /* high 4 bits reserved */
>
> - /* __le32 rb_closed_stts_rb_num:12; */
> -#define IWL_rb_closed_stts_rb_num_POS 0
> -#define IWL_rb_closed_stts_rb_num_LEN 12
> -#define IWL_rb_closed_stts_rb_num_SYM rb_closed
> - /* __le32 rsrv1:4; */
> - /* __le32 rb_closed_stts_rx_frame_num:12; */
> -#define IWL_rb_closed_stts_rx_frame_num_POS 16
> -#define IWL_rb_closed_stts_rx_frame_num_LEN 12
> -#define IWL_rb_closed_stts_rx_frame_num_SYM rb_closed
> - /* __le32 rsrv2:4; */
> -
> - __le32 frm_finished;
> - /* __le32 frame_finished_stts_rb_num:12; */
> -#define IWL_frame_finished_stts_rb_num_POS 0
> -#define IWL_frame_finished_stts_rb_num_LEN 12
> -#define IWL_frame_finished_stts_rb_num_SYM frm_finished
> - /* __le32 rsrv3:4; */
> - /* __le32 frame_finished_stts_rx_frame_num:12; */
> -#define IWL_frame_finished_stts_rx_frame_num_POS 16
> -#define IWL_frame_finished_stts_rx_frame_num_LEN 12
> -#define IWL_frame_finished_stts_rx_frame_num_SYM frm_finished
> - /* __le32 rsrv4:4; */
> + __le16 frm_finished_stts_rb_num_rsvd; /* high 4 bits reserved */
> + __le16 frm_finished_stts_rx_frame_num_rsvd; /* high 4 bits rsvd */
>
> __le32 padding1; /* so that allocation will be aligned to 16B */
> __le32 padding2;
> --- wireless-testing.orig/drivers/net/wireless/iwlwifi/iwl-5000.c 2008-10-06 18:09:08.752483265 +0200
> +++ wireless-testing/drivers/net/wireless/iwlwifi/iwl-5000.c 2008-10-06 18:32:08.379109332 +0200
> @@ -856,7 +856,8 @@ static int iwl5000_alloc_shared_mem(stru
>
> memset(priv->shared_virt, 0, sizeof(struct iwl5000_shared));
>
> - priv->rb_closed_offset = offsetof(struct iwl5000_shared, rb_closed);
> + priv->rb_closed_offset = offsetof(struct iwl5000_shared,
> + rb_closed_stts_rb_num_rsvd);
>
> return 0;
> }
> @@ -873,7 +874,7 @@ static void iwl5000_free_shared_mem(stru
> static int iwl5000_shared_mem_rx_idx(struct iwl_priv *priv)
> {
> struct iwl5000_shared *s = priv->shared_virt;
> - return le32_to_cpu(s->rb_closed) & 0xFFF;
> + return le16_to_cpu(s->rb_closed_stts_rb_num_rsvd) & 0xFFF;
> }
>
> /**
> @@ -908,19 +909,18 @@ static void iwl5000_txq_update_byte_cnt_
> }
> }
>
> - IWL_SET_BITS16(shared_data->queues_byte_cnt_tbls[txq_id].
> - tfd_offset[txq->q.write_ptr], byte_cnt, len);
> -
> - IWL_SET_BITS16(shared_data->queues_byte_cnt_tbls[txq_id].
> - tfd_offset[txq->q.write_ptr], sta_id, sta);
> + iwl5000_queue_byte_cnt_set(&shared_data->queues_byte_cnt_tbls[txq_id],
> + txq->q.write_ptr, len);
> + iwl5000_queue_sta_id_set(&shared_data->queues_byte_cnt_tbls[txq_id],
> + txq->q.write_ptr, sta);
>
> if (txq->q.write_ptr < IWL50_MAX_WIN_SIZE) {
> - IWL_SET_BITS16(shared_data->queues_byte_cnt_tbls[txq_id].
> - tfd_offset[IWL50_QUEUE_SIZE + txq->q.write_ptr],
> - byte_cnt, len);
> - IWL_SET_BITS16(shared_data->queues_byte_cnt_tbls[txq_id].
> - tfd_offset[IWL50_QUEUE_SIZE + txq->q.write_ptr],
> - sta_id, sta);
> + iwl5000_queue_byte_cnt_set(
> + &shared_data->queues_byte_cnt_tbls[txq_id],
> + IWL50_QUEUE_SIZE + txq->q.write_ptr, len);
> + iwl5000_queue_sta_id_set(
> + &shared_data->queues_byte_cnt_tbls[txq_id],
> + IWL50_QUEUE_SIZE + txq->q.write_ptr, sta);
> }
> }
>
> @@ -934,13 +934,18 @@ static void iwl5000_txq_inval_byte_cnt_t
> if (txq_id != IWL_CMD_QUEUE_NUM)
> sta = txq->cmd[txq->q.read_ptr]->cmd.tx.sta_id;
>
> - shared_data->queues_byte_cnt_tbls[txq_id].tfd_offset[txq->q.read_ptr].
> - val = cpu_to_le16(1 | (sta << 12));
> + iwl5000_queue_byte_cnt_set(&shared_data->queues_byte_cnt_tbls[txq_id],
> + txq->q.read_ptr, 1);
> + iwl5000_queue_sta_id_set(&shared_data->queues_byte_cnt_tbls[txq_id],
> + txq->q.read_ptr, sta);
>
> if (txq->q.write_ptr < IWL50_MAX_WIN_SIZE) {
> - shared_data->queues_byte_cnt_tbls[txq_id].
> - tfd_offset[IWL50_QUEUE_SIZE + txq->q.read_ptr].
> - val = cpu_to_le16(1 | (sta << 12));
> + iwl5000_queue_byte_cnt_set(
> + &shared_data->queues_byte_cnt_tbls[txq_id],
> + IWL50_QUEUE_SIZE + txq->q.read_ptr, 1);
> + iwl5000_queue_sta_id_set(
> + &shared_data->queues_byte_cnt_tbls[txq_id],
> + IWL50_QUEUE_SIZE + txq->q.read_ptr, sta);
> }
> }
>
> --- wireless-testing.orig/drivers/net/wireless/iwlwifi/iwl-helpers.h 2008-10-06 18:01:02.250108736 +0200
> +++ wireless-testing/drivers/net/wireless/iwlwifi/iwl-helpers.h 2008-10-06 18:06:18.639110868 +0200
> @@ -32,108 +32,6 @@
>
> #include <linux/ctype.h>
>
> -/*
> - * The structures defined by the hardware/uCode interface
> - * have bit-wise operations. For each bit-field there is
> - * a data symbol in the structure, the start bit position
> - * and the length of the bit-field.
> - *
> - * iwl_get_bits and iwl_set_bits will return or set the
> - * appropriate bits on a 32-bit value.
> - *
> - * IWL_GET_BITS and IWL_SET_BITS use symbol expansion to
> - * expand out to the appropriate call to iwl_get_bits
> - * and iwl_set_bits without having to reference all of the
> - * numerical constants and defines provided in the hardware
> - * definition
> - */
> -
> -/**
> - * iwl_get_bits - Extract a hardware bit-field value
> - * @src: source hardware value (__le32)
> - * @pos: bit-position (0-based) of first bit of value
> - * @len: length of bit-field
> - *
> - * iwl_get_bits will return the bit-field in cpu endian ordering.
> - *
> - * NOTE: If used from IWL_GET_BITS then pos and len are compile-constants and
> - * will collapse to minimal code by the compiler.
> - */
> -static inline u32 iwl_get_bits(__le32 src, u8 pos, u8 len)
> -{
> - u32 tmp = le32_to_cpu(src);
> -
> - tmp >>= pos;
> - tmp &= (1UL << len) - 1;
> - return tmp;
> -}
> -
> -/**
> - * iwl_set_bits - Set a hardware bit-field value
> - * @dst: Address of __le32 hardware value
> - * @pos: bit-position (0-based) of first bit of value
> - * @len: length of bit-field
> - * @val: cpu endian value to encode into the bit-field
> - *
> - * iwl_set_bits will encode val into dst, masked to be len bits long at bit
> - * position pos.
> - *
> - * NOTE: If used IWL_SET_BITS pos and len will be compile-constants and
> - * will collapse to minimal code by the compiler.
> - */
> -static inline void iwl_set_bits(__le32 *dst, u8 pos, u8 len, int val)
> -{
> - u32 tmp = le32_to_cpu(*dst);
> -
> - tmp &= ~(((1UL << len) - 1) << pos);
> - tmp |= (val & ((1UL << len) - 1)) << pos;
> - *dst = cpu_to_le32(tmp);
> -}
> -
> -static inline void iwl_set_bits16(__le16 *dst, u8 pos, u8 len, int val)
> -{
> - u16 tmp = le16_to_cpu(*dst);
> -
> - tmp &= ~((1UL << (pos + len)) - (1UL << pos));
> - tmp |= (val & ((1UL << len) - 1)) << pos;
> - *dst = cpu_to_le16(tmp);
> -}
> -
> -/*
> - * The bit-field definitions in iwl-xxxx-hw.h are in the form of:
> - *
> - * struct example {
> - * __le32 val1;
> - * #define IWL_name_POS 8
> - * #define IWL_name_LEN 4
> - * #define IWL_name_SYM val1
> - * };
> - *
> - * The IWL_SET_BITS and IWL_GET_BITS macros are provided to allow the driver
> - * to call:
> - *
> - * struct example bar;
> - * u32 val = IWL_GET_BITS(bar, name);
> - * val = val * 2;
> - * IWL_SET_BITS(bar, name, val);
> - *
> - * All cpu / host ordering, masking, and shifts are performed by the macros
> - * and iwl_{get,set}_bits.
> - *
> - */
> -#define IWL_SET_BITS(s, sym, v) \
> - iwl_set_bits(&(s).IWL_ ## sym ## _SYM, IWL_ ## sym ## _POS, \
> - IWL_ ## sym ## _LEN, (v))
> -
> -#define IWL_SET_BITS16(s, sym, v) \
> - iwl_set_bits16(&(s).IWL_ ## sym ## _SYM, IWL_ ## sym ## _POS, \
> - IWL_ ## sym ## _LEN, (v))
> -
> -#define IWL_GET_BITS(s, sym) \
> - iwl_get_bits((s).IWL_ ## sym ## _SYM, IWL_ ## sym ## _POS, \
> - IWL_ ## sym ## _LEN)
> -
> -
> #define KELVIN_TO_CELSIUS(x) ((x)-273)
> #define CELSIUS_TO_KELVIN(x) ((x)+273)
> #define IWL_MASK(lo, hi) ((1 << (hi)) | ((1 << (hi)) - (1 << (lo))))
>
>
>
On Tue, Oct 7, 2008 at 3:16 PM, John W. Linville <[email protected]> wrote:
> On Tue, Oct 07, 2008 at 03:15:29AM -0400, Christoph Hellwig wrote:
>> On Tue, Oct 07, 2008 at 08:21:56AM +0200, Holger Schurig wrote:
>> > > I don't want these macros eithers but I NACK any patches with
>> > > cursing in the commit logs.
>> >
>> > I fully agree. There's no need to speak about "crap" when
>> > changing other people's code. In some cultures thats quite
>> > insulting.
>>
>> Well, it is crap. Looks at it objectively. It's a perfectly fine
>> description and most of us use it perfectly happily for our own code,
>> too.
>
> I agree that this is at worst a little bit rude. The patch should be
> rejected or merged on its own basis, although I do reserve the right
> to edit a changelog from time to time... :-)
Appreciated.
> Now, can we discuss the patch rather than Johannes' intemperate
> word choice?
With Johannes permissions I will split the patch into chunks first so
I can take smaller steps second
it will better fit into patches I have in pipe.
Thanks
Tomas
On Tue, Oct 7, 2008 at 10:10 PM, Johannes Berg
<[email protected]> wrote:
> On Tue, 2008-10-07 at 22:06 +0200, Tomas Winkler wrote:
>
>> > bytes (with nibbles): | . | . | . | . | . | . || . | . | . | . | . | . |
>> > | addr 1 |len1 || addr 2 |len2 |
>> > your forced layout: | | | |
>> > my layout: | | || | |
>> >
>> > Note how this actually matches the border between the border between the
>> > two descriptors, the || border.
>>
>> Your layout put addr2 (which is 32 bit) sits on 16 bit boundary
>
> So? Yours makes it need two words, which is even less efficient to
> access. And since the struct is packed, on those architectures that
> don't do unaligned accesses efficiently won't need any put_unaligned
> either.
>
> Also, keep in mind that address 2 is _never_ used at all anyway
This is not correct we always use first 2 pointers. First for tx command
second for the actual packet.
because
> we don't and cannot do fragmented frames due to the lack of IP
> checksumming in hardware, as I and davem have tried to explain to you
> multiple times already.
This is bad side of current Linux implementation
Tomas
On Tue, Oct 7, 2008 at 9:34 PM, Johannes Berg <[email protected]> wrote:
> On Tue, 2008-10-07 at 21:28 +0200, Tomas Winkler wrote:
>
>> > And please keep the 10 pairs -> 20 addresses, it's a lot easier to
>> > understand :)
>
>> I don't like on this solution two things one is that it 32 bit
>> variables sits on unaligned address
>
> That doesn't matter at all since it's packed, and the bitwise accesses
> you did previously are way less efficient.
This is not packet this is shared memory.
Tomas
Hi Johannes,
> This makes the code surrounding the last few things using
> IWL_SET_BITS16 actually readable and removes the macros
> so nobody will ever consider using them again.
>
> Signed-off-by: Johannes Berg <[email protected]>
> ---
> Tested on 5000 hw.
>
> v2: don't BUG_ON an invalid index because it actually happens!
>
> drivers/net/wireless/iwlwifi/iwl-4965-hw.h | 85 +++++++++---------------
> drivers/net/wireless/iwlwifi/iwl-4965.c | 15 ++--
> drivers/net/wireless/iwlwifi/iwl-5000-hw.h | 61 +++++++++--------
> drivers/net/wireless/iwlwifi/iwl-5000.c | 41 ++++++-----
> drivers/net/wireless/iwlwifi/iwl-helpers.h | 102 -----------------------------
> drivers/net/wireless/iwlwifi/iwl-tx.c | 6 -
> 6 files changed, 99 insertions(+), 211 deletions(-)
>
> --- wireless-testing.orig/drivers/net/wireless/iwlwifi/iwl-4965-hw.h 2008-10-06 18:01:57.207234025 +0200
> +++ wireless-testing/drivers/net/wireless/iwlwifi/iwl-4965-hw.h 2008-10-06 19:35:09.488701911 +0200
> @@ -857,18 +857,24 @@ struct iwl_tfd_addr_desc {
> * A maximum of 255 (not 256!) TFDs may be on a queue waiting for Tx.
> */
> struct iwl_tfd_frame {
> - __le32 val0;
> - /* __le32 rsvd1:24; */
> - /* __le32 num_tbs:5; */
> -#define IWL_num_tbs_POS 24
> -#define IWL_num_tbs_LEN 5
> -#define IWL_num_tbs_SYM val0
> - /* __le32 rsvd2:1; */
> - /* __le32 padding:2; */
> + u8 reserved1[3];
> + u8 num_tbs; /* 5 bits, rest reserved/padding */
> struct iwl_tfd_addr_desc addrs[20];
> __le32 reserved;
> } __attribute__ ((packed));
>
> +static inline u8 iwl_get_num_tbs(struct iwl_tfd_frame *frame)
> +{
> + return frame->num_tbs & 0x1f;
> +}
> +
> +static inline void iwl_set_num_tbs(struct iwl_tfd_frame *frame, u8 num_tbs)
> +{
> + BUG_ON(num_tbs >= 20);
> +
> + frame->num_tbs &= ~0x1f;
> + frame->num_tbs |= num_tbs;
> +}
what is this magic doing and what is wrong with
frame->num_tbs = num_tbs & 0x1f
Or do you expect to do something with the upper 3 bits?
Personally I think it is important that we always zero any data
structure and especially the reserved bits of it. Otherwise stuff just
magically seems to work, but it is pure luck if the right bits are set.
The overall patch looks good to me and I think it is the way to go. Less
home grown bit magic the better.
Regards
Marcel
On Tue, Oct 07, 2008 at 08:21:56AM +0200, Holger Schurig wrote:
> > I don't want these macros eithers but I NACK any patches with
> > cursing in the commit logs.
>
> I fully agree. There's no need to speak about "crap" when
> changing other people's code. In some cultures thats quite
> insulting.
Well, it is crap. Looks at it objectively. It's a perfectly fine
description and most of us use it perfectly happily for our own code,
too.