Review request: add pseudo-hardware timestamps to mrf24j40 driver
diff mbox

Message ID 20170222192032.22360-1-web+oss@zopieux.com
State New
Headers show

Commit Message

web+oss@zopieux.com Feb. 22, 2017, 7:20 p.m. UTC
Hello,

This is not a formal patch. I am trying to add timestamping support to
the drivers/net/ieee802154/mrf24j40.c driver. I need more precise
timestamps that the software ones attached to the packets when
entering/leaving the kernel.

As far as I am aware, the MRF24J40 has no registers[1] to retrieve
hardware timestamps. Thus the idea is to use ktime_get() timestamps at
the proper places in the driver -- hence the *pseudo*-hardware.

- for incoming packets, call ktime_get() in the interrupt handler and
store it for later user. Then push it to skb_hwtstamps just before
sending the skb to ieee802154_rx_irqsafe().
- for outgoing packets, call ktime_get() in the "TX complete"
interrupt handler. This interrupt will only be triggered if the packet
is indeed going out the physical radio, which may no always be the
case, eg. if we send raw garbage on the wpan interface. But I guess
this is fine.

I implemented these changes according to [2] and by looking at existing
timestamping code in other (mostly ethernet) drivers.

Does this method make any sense? Could you do a review of my changes?
Would you be interested in up-streaming these changes once reviewed and
cleaned up?

Thanks for your time.

Alexandre

[1] http://ww1.microchip.com/downloads/en/DeviceDoc/39776C.pdf
[2] https://www.kernel.org/doc/Documentation/networking/timestamping.txt

---
 drivers/net/ieee802154/mrf24j40.c | 29 ++++++++++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)

Comments

Alexander Aring Feb. 26, 2017, 5:51 a.m. UTC | #1
Hi,

On 02/22/2017 08:20 PM, Alexandre Macabies wrote:
> Hello,
> 
> This is not a formal patch. I am trying to add timestamping support to
> the drivers/net/ieee802154/mrf24j40.c driver. I need more precise
> timestamps that the software ones attached to the packets when
> entering/leaving the kernel.
> 
> As far as I am aware, the MRF24J40 has no registers[1] to retrieve
> hardware timestamps. Thus the idea is to use ktime_get() timestamps at
> the proper places in the driver -- hence the *pseudo*-hardware.
> 

Sometimes e.g. at86rf230 has interrupts for that.

> - for incoming packets, call ktime_get() in the interrupt handler and
> store it for later user. Then push it to skb_hwtstamps just before
> sending the skb to ieee802154_rx_irqsafe().
> - for outgoing packets, call ktime_get() in the "TX complete"
> interrupt handler. This interrupt will only be triggered if the packet
> is indeed going out the physical radio, which may no always be the
> case, eg. if we send raw garbage on the wpan interface. But I guess
> this is fine.
> 
> I implemented these changes according to [2] and by looking at existing
> timestamping code in other (mostly ethernet) drivers.
> 
> Does this method make any sense? Could you do a review of my changes?
> Would you be interested in up-streaming these changes once reviewed and
> cleaned up?
> 

question: does it work for you use case? :-)

I think it's looking small enough to fit make a minimal implementation
for such tx timestamping for all other users which looking for such
feature.

My question would also be:

Do you can move such handling into ieee802154 subsystem?

Add some functions to mac802154/util.c (should be fine at first, maybe
we need another file if we get hardmac support).

Then offer some API that others driver know when calling timestamp
function (in a very abstract way).

For me:

TX start is when you set some bit to transmit finally the frame.

TX end if when you know that the interrupt was a "tx done" irq.

That's what I see you currently have done in the driver and makes sense
to me.

(Adding alan, the maintainer of the driver)

- Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-wpan" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alan Ott Feb. 27, 2017, 9:37 p.m. UTC | #2
On 02/26/2017 12:51 AM, Alexander Aring wrote:
> On 02/22/2017 08:20 PM, Alexandre Macabies wrote:
>> Hello,
>>
>> This is not a formal patch. I am trying to add timestamping support to
>> the drivers/net/ieee802154/mrf24j40.c driver. I need more precise
>> timestamps that the software ones attached to the packets when
>> entering/leaving the kernel.
>>
>> As far as I am aware, the MRF24J40 has no registers[1] to retrieve
>> hardware timestamps. Thus the idea is to use ktime_get() timestamps at
>> the proper places in the driver -- hence the *pseudo*-hardware.
>>
> Sometimes e.g. at86rf230 has interrupts for that.
>
>> - for incoming packets, call ktime_get() in the interrupt handler and
>> store it for later user. Then push it to skb_hwtstamps just before
>> sending the skb to ieee802154_rx_irqsafe().
>> - for outgoing packets, call ktime_get() in the "TX complete"
>> interrupt handler. This interrupt will only be triggered if the packet
>> is indeed going out the physical radio, which may no always be the
>> case, eg. if we send raw garbage on the wpan interface. But I guess
>> this is fine.
>>
>> I implemented these changes according to [2] and by looking at existing
>> timestamping code in other (mostly ethernet) drivers.
>>
>> Does this method make any sense? Could you do a review of my changes?
>> Would you be interested in up-streaming these changes once reviewed and
>> cleaned up?
>>
> question: does it work for you use case? :-)
>
> I think it's looking small enough to fit make a minimal implementation
> for such tx timestamping for all other users which looking for such
> feature.

It seems fairly device-specific. I'm not sure what you mean here or how 
it would work.

> My question would also be:
>
> Do you can move such handling into ieee802154 subsystem?
>
> Add some functions to mac802154/util.c (should be fine at first, maybe
> we need another file if we get hardmac support).
>
> Then offer some API that others driver know when calling timestamp
> function (in a very abstract way).
>
> For me:
>
> TX start is when you set some bit to transmit finally the frame.
>
> TX end if when you know that the interrupt was a "tx done" irq.
>
> That's what I see you currently have done in the driver and makes sense
> to me.
>
> (Adding alan, the maintainer of the driver)

Thanks for the heads-up Alex.

I think as far as making it generic, timestamping is generic at the 
skb-level, which is where it makes sense (since this is not something 
specific to 802154, but done on other types of network interfaces as well.

Depending on what kind of precision you need, you might need to get the 
timestamping into the actual ISR (which is right now given to (spi->isr)).

Alan.

--
To unsubscribe from this list: send the line "unsubscribe linux-wpan" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alexander Aring Feb. 28, 2017, 4:57 p.m. UTC | #3
On 02/27/2017 10:37 PM, Alan Ott wrote:
> On 02/26/2017 12:51 AM, Alexander Aring wrote:
>> On 02/22/2017 08:20 PM, Alexandre Macabies wrote:
>>> Hello,
>>>
>>> This is not a formal patch. I am trying to add timestamping support to
>>> the drivers/net/ieee802154/mrf24j40.c driver. I need more precise
>>> timestamps that the software ones attached to the packets when
>>> entering/leaving the kernel.
>>>
>>> As far as I am aware, the MRF24J40 has no registers[1] to retrieve
>>> hardware timestamps. Thus the idea is to use ktime_get() timestamps at
>>> the proper places in the driver -- hence the *pseudo*-hardware.
>>>
>> Sometimes e.g. at86rf230 has interrupts for that.
>>
>>> - for incoming packets, call ktime_get() in the interrupt handler and
>>> store it for later user. Then push it to skb_hwtstamps just before
>>> sending the skb to ieee802154_rx_irqsafe().
>>> - for outgoing packets, call ktime_get() in the "TX complete"
>>> interrupt handler. This interrupt will only be triggered if the packet
>>> is indeed going out the physical radio, which may no always be the
>>> case, eg. if we send raw garbage on the wpan interface. But I guess
>>> this is fine.
>>>
>>> I implemented these changes according to [2] and by looking at existing
>>> timestamping code in other (mostly ethernet) drivers.
>>>
>>> Does this method make any sense? Could you do a review of my changes?
>>> Would you be interested in up-streaming these changes once reviewed and
>>> cleaned up?
>>>
>> question: does it work for you use case? :-)
>>
>> I think it's looking small enough to fit make a minimal implementation
>> for such tx timestamping for all other users which looking for such
>> feature.
> 
> It seems fairly device-specific. I'm not sure what you mean here or how it would work.
>

me either, I don't know how the timestamp information are useful or how
you can get them from userspace... I never used that feature.

Device specific -> yes. I think we need to clear on what "times" we
measure the timestamp -> e.g. rx start or rx done. mostly rx done should
be easy.

>> My question would also be:
>>
>> Do you can move such handling into ieee802154 subsystem?
>>
>> Add some functions to mac802154/util.c (should be fine at first, maybe
>> we need another file if we get hardmac support).
>>
>> Then offer some API that others driver know when calling timestamp
>> function (in a very abstract way).
>>
>> For me:
>>
>> TX start is when you set some bit to transmit finally the frame.
>>
>> TX end if when you know that the interrupt was a "tx done" irq.
>>
>> That's what I see you currently have done in the driver and makes sense
>> to me.
>>
>> (Adding alan, the maintainer of the driver)
> 
> Thanks for the heads-up Alex.
> 
> I think as far as making it generic, timestamping is generic at the skb-level, which is where it makes sense (since this is not something specific to 802154, but done on other types of network interfaces as well.
> 

Yes, just to provide some API that I also can use it for at86rf230. I
don't care about that currently, when I need it... I will do that as API
and then both drivers can use it somehow (and I think there will follow
some parts in such API functions and not only the two calls which are
needed here).

So first it's fine for me to make it as first inside this driver.

> Depending on what kind of precision you need, you might need to get the timestamping into the actual ISR (which is right now given to (spi->isr)).
> 

Agree -> ISR, make timestamp then if "tx done" after readout stats then
set the timestamp, so we had somehow the hardirq time kontext and not
the time after irqstat readout.

For receive the same.

I am now busy with my exam. I will not read mails again until it's
finished.

Bye.

- Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-wpan" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/drivers/net/ieee802154/mrf24j40.c b/drivers/net/ieee802154/mrf24j40.c
index f446db828561..165c86992672 100644
--- a/drivers/net/ieee802154/mrf24j40.c
+++ b/drivers/net/ieee802154/mrf24j40.c
@@ -21,6 +21,7 @@ 
 #include <linux/regmap.h>
 #include <linux/ieee802154.h>
 #include <linux/irq.h>
+#include <linux/ktime.h>
 #include <net/cfg802154.h>
 #include <net/mac802154.h>
 
@@ -236,6 +237,7 @@  struct mrf24j40 {
 	struct spi_transfer rx_lqi_trx;
 	u8 rx_fifo_buf[RX_FIFO_SIZE];
 	struct spi_transfer rx_fifo_buf_trx;
+	ktime_t rx_tstamp;
 
 	/* isr handling for reading intstat */
 	struct spi_message irq_msg;
@@ -558,6 +560,8 @@  static void write_tx_buf_complete(void *context)
 	if (ieee802154_is_ackreq(fc))
 		val |= BIT_TXNACKREQ;
 
+	skb_tx_timestamp(devrec->tx_skb);
+
 	devrec->tx_post_msg.complete = NULL;
 	devrec->tx_post_buf[0] = MRF24J40_WRITESHORT(REG_TXNCON);
 	devrec->tx_post_buf[1] = val;
@@ -604,6 +608,9 @@  static int mrf24j40_tx(struct ieee802154_hw *hw, struct sk_buff *skb)
 	dev_dbg(printdev(devrec), "tx packet of %d bytes\n", skb->len);
 	devrec->tx_skb = skb;
 
+	if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP))
+		skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
+
 	return write_tx_buf(devrec, 0x000, skb->data, skb->len);
 }
 
@@ -774,6 +781,9 @@  static void mrf24j40_handle_rx_read_buf_complete(void *context)
 	}
 
 	memcpy(skb_put(skb, len), rx_local_buf, len);
+
+	skb_hwtstamps(skb)->hwtstamp = devrec->rx_tstamp;
+
 	ieee802154_rx_irqsafe(devrec->hw, skb, 0);
 
 #ifdef DEBUG
@@ -1038,12 +1048,25 @@  static void mrf24j40_intstat_complete(void *context)
 				   BIT_SECIGNORE);
 
 	/* Check for TX complete */
-	if (intstat & BIT_TXNIF)
-		ieee802154_xmit_complete(devrec->hw, devrec->tx_skb, false);
+	if (intstat & BIT_TXNIF) {
+		struct sk_buff *skb = devrec->tx_skb;
+		/* Set hardware timestamp */
+		if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS)) {
+			struct skb_shared_hwtstamps shhwtstamps;
+
+			memset(&shhwtstamps, 0, sizeof(shhwtstamps));
+			shhwtstamps.hwtstamp = ktime_get_real();
+			skb_tstamp_tx(skb, &shhwtstamps);
+		}
+		ieee802154_xmit_complete(devrec->hw, skb, false);
+	}
 
 	/* Check for Rx */
-	if (intstat & BIT_RXIF)
+	if (intstat & BIT_RXIF) {
+		/* Save system clock timestamp for later */
+		devrec->rx_tstamp = ktime_get_real();
 		mrf24j40_handle_rx(devrec);
+	}
 }
 
 static irqreturn_t mrf24j40_isr(int irq, void *data)