Skip to contentSkip to conversation

at86rf230: improvements and cleanups

PATCH In review Alexander Aring 2 participants 8 messages Mar 1, 2015 – Mar 3, 2015

Subsystems (2)
Network drivers IEEE 802.15.4
lore
Share & tools
lore

Alexander Aring

author maintainer
#0 lore raw

cover at86rf230: improvements and cleanups

Hi,

this series contains some improvements in transmit handling which insert a retry mode for going into STATE_TX_ON instead of turning into this state with forcing after the first try. This series contains also some improvements /cleanups which squash some multiple dereferencing and don't overwrite a memory space always with the same number.

- Alex

Alexander Aring (6):
at86rf230: add transmit retry support
at86rf230: cleanup and squash stack variable
at86rf230: refactor receive handling
at86rf230: remove multiple dereferencing for irq
at86rf230: remove multiple dereferencing for ctx
at86rf230: restore trx len when needed

 drivers/net/ieee802154/at86rf230.c | 117 +++++++++++++++++++++----------------
 1 file changed, 67 insertions(+), 50 deletions(-)
Signature · 1 line
2.3.0
author maintainer
#1 lore raw

PATCH 1/6 at86rf230: add transmit retry support

This patch introduce a transmit retry handling into at86rf230 transmit path. Current behaviour is to wait the normal receive time if we want to go into STATE_TX_ON when the transceiver is in STATE_BUSY_RX_AACK which indicates that a frame is currently receiving. A non force state change will not interrupt the the receiving state.

The current behaviour is that after the normal receive time we will start a force change into STATE_TX_ON. With this patch we do seven retries to go into STATE_TX_ON without forcing. After we hit the AT86RF2XX_MAX_TX_RETRIES we will start the force state change. This is a polling like method to go into STATE_TX_ON in times of maximum receiving time.

Signed-off-by Alexander Aring <[email protected]>
---
 drivers/net/ieee802154/at86rf230.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)
drivers/net/ieee802154/at86rf230.c +21 -3
index 1d438bc..503fabd 100644
@@ -52,7 +52,13 @@ struct at86rf2xx_chip_data {
 	int (*get_desense_steps)(struct at86rf230_local *, s32);
 };
 
-#define AT86RF2XX_MAX_BUF (127 + 3)
+#define AT86RF2XX_MAX_BUF		(127 + 3)
+/* tx retries to access the TX_ON state
+ * if it's above then force change will be started.
+ *
+ * We assume the max_frame_retries (7) value of 802.15.4 here.
+ */
+#define AT86RF2XX_MAX_TX_RETRIES	7
 
 struct at86rf230_state_change {
 	struct at86rf230_local *lp;

@@ -85,6 +91,7 @@ struct at86rf230_local {
 	bool is_tx;
 	/* spinlock for is_tx protection */
 	spinlock_t lock;
+	u8 tx_retry;
 	struct sk_buff *tx_skb;
 	struct at86rf230_state_change tx;
 };

@@ -512,10 +519,20 @@ at86rf230_async_state_assert(void *context)
 			 * in STATE_BUSY_RX_AACK, we run a force state change
 			 * to STATE_TX_ON. This is a timeout handling, if the
 			 * transceiver stucks in STATE_BUSY_RX_AACK.
+			 *
+			 * Additional we do several retries to try to get into
+			 * TX_ON state without forcing. If the retries are
+			 * higher or equal than AT86RF2XX_MAX_TX_RETRIES we
+			 * will do a force change.
 			 */
 			if (ctx->to_state == STATE_TX_ON) {
-				at86rf230_async_state_change(lp, ctx,
-							     STATE_FORCE_TX_ON,
+				u8 state = STATE_TX_ON;
+
+				if (lp->tx_retry >= AT86RF2XX_MAX_TX_RETRIES)
+					state = STATE_FORCE_TX_ON;
+				lp->tx_retry++;
+
+				at86rf230_async_state_change(lp, ctx, state,
 							     ctx->complete,
 							     ctx->irq_enable);
 				return;

@@ -963,6 +980,7 @@ at86rf230_xmit(struct ieee802154_hw *hw, struct sk_buff *skb)
 	if (lp->tx_aret)
 		tx_complete = at86rf230_xmit_tx_on;
 
+	lp->tx_retry = 0;
 	at86rf230_async_state_change(lp, ctx, STATE_TX_ON, tx_complete, false);
 
 	return 0;
Signature · 1 line
2.3.0
author maintainer
#2 lore raw

PATCH 2/6 at86rf230: cleanup and squash stack variable

In reply to Alexander Aring

I had this variable because I thought it would be protected by disable/enable irq but this is not true. It's protected by stop/wake netdev queue which is called by ieee802154_xmit_complete.

Signed-off-by Alexander Aring <[email protected]>
---
 drivers/net/ieee802154/at86rf230.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/net/ieee802154/at86rf230.c +1 -2
index 503fabd..8501220 100644
@@ -725,11 +725,10 @@ at86rf230_tx_complete(void *context)
 {
 	struct at86rf230_state_change *ctx = context;
 	struct at86rf230_local *lp = ctx->lp;
-	struct sk_buff *skb = lp->tx_skb;
 
 	enable_irq(lp->spi->irq);
 
-	ieee802154_xmit_complete(lp->hw, skb, !lp->tx_aret);
+	ieee802154_xmit_complete(lp->hw, lp->tx_skb, !lp->tx_aret);
 }
 
 static void
Signature · 1 line
2.3.0
author maintainer
#3 lore raw

PATCH 3/6 at86rf230: refactor receive handling

In reply to Alexander Aring

This patch refactor the receive handling into one function.

Signed-off-by Alexander Aring <[email protected]>
---
 drivers/net/ieee802154/at86rf230.c | 34 ++++++++++++++--------------------
 1 file changed, 14 insertions(+), 20 deletions(-)
drivers/net/ieee802154/at86rf230.c +14 -20
index 8501220..8e93ea4 100644
@@ -781,13 +781,23 @@ at86rf230_tx_trac_status(void *context)
 }
 
 static void
-at86rf230_rx(struct at86rf230_local *lp,
-	     const u8 *data, const u8 len, const u8 lqi)
+at86rf230_rx_read_frame_complete(void *context)
 {
-	struct sk_buff *skb;
+	struct at86rf230_state_change *ctx = context;
+	struct at86rf230_local *lp = ctx->lp;
 	u8 rx_local_buf[AT86RF2XX_MAX_BUF];
+	const u8 *buf = lp->irq.buf;
+	struct sk_buff *skb;
+	u8 len, lqi;
 
-	memcpy(rx_local_buf, data, len);
+	len = buf[1];
+	if (!ieee802154_is_valid_psdu_len(len)) {
+		dev_vdbg(&lp->spi->dev, "corrupted frame received\n");
+		len = IEEE802154_MTU;
+	}
+	lqi = buf[2 + len];
+
+	memcpy(rx_local_buf, buf + 2, len);
 	enable_irq(lp->spi->irq);
 
 	skb = dev_alloc_skb(IEEE802154_MTU);

@@ -801,22 +811,6 @@ at86rf230_rx(struct at86rf230_local *lp,
 }
 
 static void
-at86rf230_rx_read_frame_complete(void *context)
-{
-	struct at86rf230_state_change *ctx = context;
-	struct at86rf230_local *lp = ctx->lp;
-	const u8 *buf = lp->irq.buf;
-	u8 len = buf[1];
-
-	if (!ieee802154_is_valid_psdu_len(len)) {
-		dev_vdbg(&lp->spi->dev, "corrupted frame received\n");
-		len = IEEE802154_MTU;
-	}
-
-	at86rf230_rx(lp, buf + 2, len, buf[2 + len]);
-}
-
-static void
 at86rf230_rx_read_frame(struct at86rf230_local *lp)
 {
 	int rc;
Signature · 1 line
2.3.0
author maintainer
#4 lore raw

PATCH 4/6 at86rf230: remove multiple dereferencing for irq

In reply to Alexander Aring

By holding the irq variable inside at86rf230_state_change we can squash some multiple dereferencing for getting irq num.

Signed-off-by Alexander Aring <[email protected]>
---
 drivers/net/ieee802154/at86rf230.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)
drivers/net/ieee802154/at86rf230.c +14 -11
index 8e93ea4..7f27fa3 100644
@@ -62,6 +62,7 @@ struct at86rf2xx_chip_data {
 
 struct at86rf230_state_change {
 	struct at86rf230_local *lp;
+	int irq;
 
 	struct spi_message msg;
 	struct spi_transfer trx;

@@ -483,7 +484,7 @@ at86rf230_async_read_reg(struct at86rf230_local *lp, const u8 reg,
 	rc = spi_async(lp->spi, &ctx->msg);
 	if (rc) {
 		if (irq_enable)
-			enable_irq(lp->spi->irq);
+			enable_irq(ctx->irq);
 
 		at86rf230_async_error(lp, ctx, rc);
 	}

@@ -667,7 +668,7 @@ at86rf230_async_state_change_start(void *context)
 	rc = spi_async(lp->spi, &ctx->msg);
 	if (rc) {
 		if (ctx->irq_enable)
-			enable_irq(lp->spi->irq);
+			enable_irq(ctx->irq);
 
 		at86rf230_async_error(lp, ctx, rc);
 	}

@@ -726,7 +727,7 @@ at86rf230_tx_complete(void *context)
 	struct at86rf230_state_change *ctx = context;
 	struct at86rf230_local *lp = ctx->lp;
 
-	enable_irq(lp->spi->irq);
+	enable_irq(ctx->irq);
 
 	ieee802154_xmit_complete(lp->hw, lp->tx_skb, !lp->tx_aret);
 }

@@ -798,7 +799,7 @@ at86rf230_rx_read_frame_complete(void *context)
 	lqi = buf[2 + len];
 
 	memcpy(rx_local_buf, buf + 2, len);
-	enable_irq(lp->spi->irq);
+	enable_irq(ctx->irq);
 
 	skb = dev_alloc_skb(IEEE802154_MTU);
 	if (!skb) {

@@ -811,8 +812,10 @@ at86rf230_rx_read_frame_complete(void *context)
 }
 
 static void
-at86rf230_rx_read_frame(struct at86rf230_local *lp)
+at86rf230_rx_read_frame(void *context)
 {
+	struct at86rf230_state_change *ctx = context;
+	struct at86rf230_local *lp = ctx->lp;
 	int rc;
 
 	u8 *buf = lp->irq.buf;

@@ -822,7 +825,7 @@ at86rf230_rx_read_frame(struct at86rf230_local *lp)
 	lp->irq.msg.complete = at86rf230_rx_read_frame_complete;
 	rc = spi_async(lp->spi, &lp->irq.msg);
 	if (rc) {
-		enable_irq(lp->spi->irq);
+		enable_irq(ctx->irq);
 		at86rf230_async_error(lp, &lp->irq, rc);
 	}
 }

@@ -830,16 +833,13 @@ at86rf230_rx_read_frame(struct at86rf230_local *lp)
 static void
 at86rf230_rx_trac_check(void *context)
 {
-	struct at86rf230_state_change *ctx = context;
-	struct at86rf230_local *lp = ctx->lp;
-
 	/* Possible check on trac status here. This could be useful to make
 	 * some stats why receive is failed. Not used at the moment, but it's
 	 * maybe timing relevant. Datasheet doesn't say anything about this.
 	 * The programming guide say do it so.
 	 */
 
-	at86rf230_rx_read_frame(lp);
+	at86rf230_rx_read_frame(context);
 }
 
 static void

@@ -878,7 +878,7 @@ at86rf230_irq_status(void *context)
 	if (irq & IRQ_TRX_END) {
 		at86rf230_irq_trx_end(lp);
 	} else {
-		enable_irq(lp->spi->irq);
+		enable_irq(ctx->irq);
 		dev_err(&lp->spi->dev, "not supported irq %02x received\n",
 			irq);
 	}

@@ -1539,6 +1539,7 @@ static void
 at86rf230_setup_spi_messages(struct at86rf230_local *lp)
 {
 	lp->state.lp = lp;
+	lp->state.irq = lp->spi->irq;
 	spi_message_init(&lp->state.msg);
 	lp->state.msg.context = &lp->state;
 	lp->state.trx.tx_buf = lp->state.buf;

@@ -1546,6 +1547,7 @@ at86rf230_setup_spi_messages(struct at86rf230_local *lp)
 	spi_message_add_tail(&lp->state.trx, &lp->state.msg);
 
 	lp->irq.lp = lp;
+	lp->irq.irq = lp->spi->irq;
 	spi_message_init(&lp->irq.msg);
 	lp->irq.msg.context = &lp->irq;
 	lp->irq.trx.tx_buf = lp->irq.buf;

@@ -1553,6 +1555,7 @@ at86rf230_setup_spi_messages(struct at86rf230_local *lp)
 	spi_message_add_tail(&lp->irq.trx, &lp->irq.msg);
 
 	lp->tx.lp = lp;
+	lp->tx.irq = lp->spi->irq;
 	spi_message_init(&lp->tx.msg);
 	lp->tx.msg.context = &lp->tx;
 	lp->tx.trx.tx_buf = lp->tx.buf;
Signature · 1 line
2.3.0
author maintainer
#5 lore raw

PATCH 5/6 at86rf230: remove multiple dereferencing for ctx

In reply to Alexander Aring

This patch cleanups the referencing for the state change context variable. The state change context should only set once and this is by initial a state change. This patch will use the initial state change variable in the complete handler of the state change by using the ctx context which should be always the same like the initial state change context.

Signed-off-by Alexander Aring <[email protected]>
---
 drivers/net/ieee802154/at86rf230.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)
drivers/net/ieee802154/at86rf230.c +12 -13
index 7f27fa3..216c80c 100644
@@ -738,7 +738,7 @@ at86rf230_tx_on(void *context)
 	struct at86rf230_state_change *ctx = context;
 	struct at86rf230_local *lp = ctx->lp;
 
-	at86rf230_async_state_change(lp, &lp->irq, STATE_RX_AACK_ON,
+	at86rf230_async_state_change(lp, ctx, STATE_RX_AACK_ON,
 				     at86rf230_tx_complete, true);
 }
 

@@ -787,7 +787,7 @@ at86rf230_rx_read_frame_complete(void *context)
 	struct at86rf230_state_change *ctx = context;
 	struct at86rf230_local *lp = ctx->lp;
 	u8 rx_local_buf[AT86RF2XX_MAX_BUF];
-	const u8 *buf = lp->irq.buf;
+	const u8 *buf = ctx->buf;
 	struct sk_buff *skb;
 	u8 len, lqi;
 

@@ -816,17 +816,16 @@ at86rf230_rx_read_frame(void *context)
 {
 	struct at86rf230_state_change *ctx = context;
 	struct at86rf230_local *lp = ctx->lp;
+	u8 *buf = ctx->buf;
 	int rc;
 
-	u8 *buf = lp->irq.buf;
-
 	buf[0] = CMD_FB;
-	lp->irq.trx.len = AT86RF2XX_MAX_BUF;
-	lp->irq.msg.complete = at86rf230_rx_read_frame_complete;
-	rc = spi_async(lp->spi, &lp->irq.msg);
+	ctx->trx.len = AT86RF2XX_MAX_BUF;
+	ctx->msg.complete = at86rf230_rx_read_frame_complete;
+	rc = spi_async(lp->spi, &ctx->msg);
 	if (rc) {
 		enable_irq(ctx->irq);
-		at86rf230_async_error(lp, &lp->irq, rc);
+		at86rf230_async_error(lp, ctx, rc);
 	}
 }
 

@@ -872,7 +871,7 @@ at86rf230_irq_status(void *context)
 {
 	struct at86rf230_state_change *ctx = context;
 	struct at86rf230_local *lp = ctx->lp;
-	const u8 *buf = lp->irq.buf;
+	const u8 *buf = ctx->buf;
 	const u8 irq = buf[1];
 
 	if (irq & IRQ_TRX_END) {

@@ -929,7 +928,7 @@ at86rf230_write_frame(void *context)
 	struct at86rf230_state_change *ctx = context;
 	struct at86rf230_local *lp = ctx->lp;
 	struct sk_buff *skb = lp->tx_skb;
-	u8 *buf = lp->tx.buf;
+	u8 *buf = ctx->buf;
 	int rc;
 
 	spin_lock(&lp->lock);

@@ -939,9 +938,9 @@ at86rf230_write_frame(void *context)
 	buf[0] = CMD_FB | CMD_WRITE;
 	buf[1] = skb->len + 2;
 	memcpy(buf + 2, skb->data, skb->len);
-	lp->tx.trx.len = skb->len + 2;
-	lp->tx.msg.complete = at86rf230_write_frame_complete;
-	rc = spi_async(lp->spi, &lp->tx.msg);
+	ctx->trx.len = skb->len + 2;
+	ctx->msg.complete = at86rf230_write_frame_complete;
+	rc = spi_async(lp->spi, &ctx->msg);
 	if (rc)
 		at86rf230_async_error(lp, ctx, rc);
 }
Signature · 1 line
2.3.0
author maintainer
#6 lore raw

PATCH 6/6 at86rf230: restore trx len when needed

In reply to Alexander Aring

In the most cases the spi messages has a length of two. Currently we always set the the len field to two before transmit a spi message. In cases for read out/write in the frame buffer we need another len. This patch use trx len two as default. For the frame buffer cases we restore the trx len to two on success and failure. This will reduce the len setting of two when it's already two.

Signed-off-by Alexander Aring <[email protected]>
---
 drivers/net/ieee802154/at86rf230.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)
drivers/net/ieee802154/at86rf230.c +8 -4
index 216c80c..088fa68 100644
@@ -478,7 +478,6 @@ at86rf230_async_read_reg(struct at86rf230_local *lp, const u8 reg,
 	u8 *tx_buf = ctx->buf;
 
 	tx_buf[0] = (reg & CMD_REG_MASK) | CMD_REG;
-	ctx->trx.len = 2;
 	ctx->msg.complete = complete;
 	ctx->irq_enable = irq_enable;
 	rc = spi_async(lp->spi, &ctx->msg);

@@ -663,7 +662,6 @@ at86rf230_async_state_change_start(void *context)
 	 */
 	buf[0] = (RG_TRX_STATE & CMD_REG_MASK) | CMD_REG | CMD_WRITE;
 	buf[1] = ctx->to_state;
-	ctx->trx.len = 2;
 	ctx->msg.complete = at86rf230_async_state_delay;
 	rc = spi_async(lp->spi, &ctx->msg);
 	if (rc) {

@@ -799,6 +797,7 @@ at86rf230_rx_read_frame_complete(void *context)
 	lqi = buf[2 + len];
 
 	memcpy(rx_local_buf, buf + 2, len);
+	ctx->trx.len = 2;
 	enable_irq(ctx->irq);
 
 	skb = dev_alloc_skb(IEEE802154_MTU);

@@ -824,6 +823,7 @@ at86rf230_rx_read_frame(void *context)
 	ctx->msg.complete = at86rf230_rx_read_frame_complete;
 	rc = spi_async(lp->spi, &ctx->msg);
 	if (rc) {
+		ctx->trx.len = 2;
 		enable_irq(ctx->irq);
 		at86rf230_async_error(lp, ctx, rc);
 	}

@@ -893,7 +893,6 @@ static irqreturn_t at86rf230_isr(int irq, void *data)
 	disable_irq_nosync(irq);
 
 	buf[0] = (RG_IRQ_STATUS & CMD_REG_MASK) | CMD_REG;
-	ctx->trx.len = 2;
 	ctx->msg.complete = at86rf230_irq_status;
 	rc = spi_async(lp->spi, &ctx->msg);
 	if (rc) {

@@ -941,8 +940,10 @@ at86rf230_write_frame(void *context)
 	ctx->trx.len = skb->len + 2;
 	ctx->msg.complete = at86rf230_write_frame_complete;
 	rc = spi_async(lp->spi, &ctx->msg);
-	if (rc)
+	if (rc) {
+		ctx->trx.len = 2;
 		at86rf230_async_error(lp, ctx, rc);
+	}
 }
 
 static void

@@ -1541,6 +1542,7 @@ at86rf230_setup_spi_messages(struct at86rf230_local *lp)
 	lp->state.irq = lp->spi->irq;
 	spi_message_init(&lp->state.msg);
 	lp->state.msg.context = &lp->state;
+	lp->state.trx.len = 2;
 	lp->state.trx.tx_buf = lp->state.buf;
 	lp->state.trx.rx_buf = lp->state.buf;
 	spi_message_add_tail(&lp->state.trx, &lp->state.msg);

@@ -1549,6 +1551,7 @@ at86rf230_setup_spi_messages(struct at86rf230_local *lp)
 	lp->irq.irq = lp->spi->irq;
 	spi_message_init(&lp->irq.msg);
 	lp->irq.msg.context = &lp->irq;
+	lp->irq.trx.len = 2;
 	lp->irq.trx.tx_buf = lp->irq.buf;
 	lp->irq.trx.rx_buf = lp->irq.buf;
 	spi_message_add_tail(&lp->irq.trx, &lp->irq.msg);

@@ -1557,6 +1560,7 @@ at86rf230_setup_spi_messages(struct at86rf230_local *lp)
 	lp->tx.irq = lp->spi->irq;
 	spi_message_init(&lp->tx.msg);
 	lp->tx.msg.context = &lp->tx;
+	lp->tx.trx.len = 2;
 	lp->tx.trx.tx_buf = lp->tx.buf;
 	lp->tx.trx.rx_buf = lp->tx.buf;
 	spi_message_add_tail(&lp->tx.trx, &lp->tx.msg);
Signature · 1 line
2.3.0
#7 lore raw
In reply to Alexander Aring

Hi Alex,

Quoted 16 lines

this series contains some improvements in transmit handling which insert a retry mode for going into STATE_TX_ON instead of turning into this state with forcing after the first try. This series contains also some improvements /cleanups which squash some multiple dereferencing and don't overwrite a memory space always with the same number.

- Alex

Alexander Aring (6):
at86rf230: add transmit retry support
at86rf230: cleanup and squash stack variable
at86rf230: refactor receive handling
at86rf230: remove multiple dereferencing for irq
at86rf230: remove multiple dereferencing for ctx
at86rf230: restore trx len when needed

Quoted 2 lines

drivers/net/ieee802154/at86rf230.c | 117 +++++++++++++++++++++----------------

1 file changed, 67 insertions(+), 50 deletions(-)

all 6 patches have been applied to bluetooth-next tree.

Regards

Marcel