| Message ID | 20170222192032.22360-1-web+oss@zopieux.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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)