[PATCH bluetooth-next 0/6] at86rf230: improvements and cleanups

[PATCH bluetooth-next 0/6] at86rf230: improvements and cleanups

[Alexander Aring]

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(-)

-- 
2.3.0

[PATCH bluetooth-next 1/6] at86rf230: add transmit retry support

[Alexander Aring]

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 <alex.aring@gmail.com>
---
 drivers/net/ieee802154/at86rf230.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ieee802154/at86rf230.c b/drivers/net/ieee802154/at86rf230.c
index 1d438bc..503fabd 100644
--- a/drivers/net/ieee802154/at86rf230.c
+++ b/drivers/net/ieee802154/at86rf230.c
@@ -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;
-- 
2.3.0

[PATCH bluetooth-next 2/6] at86rf230: cleanup and squash stack variable

[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 <alex.aring@gmail.com>
---
 drivers/net/ieee802154/at86rf230.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/ieee802154/at86rf230.c b/drivers/net/ieee802154/at86rf230.c
index 503fabd..8501220 100644
--- a/drivers/net/ieee802154/at86rf230.c
+++ b/drivers/net/ieee802154/at86rf230.c
@@ -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
-- 
2.3.0

[PATCH bluetooth-next 3/6] at86rf230: refactor receive handling

[Alexander Aring]

This patch refactor the receive handling into one function.

Signed-off-by: Alexander Aring <alex.aring@gmail.com>
---
 drivers/net/ieee802154/at86rf230.c | 34 ++++++++++++++--------------------
 1 file changed, 14 insertions(+), 20 deletions(-)

diff --git a/drivers/net/ieee802154/at86rf230.c b/drivers/net/ieee802154/at86rf230.c
index 8501220..8e93ea4 100644
--- a/drivers/net/ieee802154/at86rf230.c
+++ b/drivers/net/ieee802154/at86rf230.c
@@ -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;
-- 
2.3.0

[PATCH bluetooth-next 4/6] at86rf230: remove multiple dereferencing for irq

[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 <alex.aring@gmail.com>
---
 drivers/net/ieee802154/at86rf230.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ieee802154/at86rf230.c b/drivers/net/ieee802154/at86rf230.c
index 8e93ea4..7f27fa3 100644
--- a/drivers/net/ieee802154/at86rf230.c
+++ b/drivers/net/ieee802154/at86rf230.c
@@ -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;
-- 
2.3.0

[PATCH bluetooth-next 5/6] at86rf230: remove multiple dereferencing for ctx

[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 <alex.aring@gmail.com>
---
 drivers/net/ieee802154/at86rf230.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ieee802154/at86rf230.c b/drivers/net/ieee802154/at86rf230.c
index 7f27fa3..216c80c 100644
--- a/drivers/net/ieee802154/at86rf230.c
+++ b/drivers/net/ieee802154/at86rf230.c
@@ -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);
 }
-- 
2.3.0

[PATCH bluetooth-next 6/6] at86rf230: restore trx len when needed

[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 <alex.aring@gmail.com>
---
 drivers/net/ieee802154/at86rf230.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ieee802154/at86rf230.c b/drivers/net/ieee802154/at86rf230.c
index 216c80c..088fa68 100644
--- a/drivers/net/ieee802154/at86rf230.c
+++ b/drivers/net/ieee802154/at86rf230.c
@@ -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);
-- 
2.3.0

Re: [PATCH bluetooth-next 0/6] at86rf230: improvements and cleanups

[Marcel Holtmann]

Hi Alex,

> 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(-)

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

Regards

Marcel