Re: [PATCH 3/4] net: macb: Add hardware PTP support
On Tue, May 02, 2017 at 01:57:15PM +, Rafal Ozieblo wrote:
> > What is the point of this wrapper function anyhow? Please remove it.
> gem_ptp_gettime() is assigned in ptp_clock_info and it has to have
> ptp_clock_info pointer as first parameter. gem_tsu_get_time() is used in
> the source code but with macb pointer.
> Do you want me to do something like:
> gem_ptp_gettime(macb->ptp, ts);
> and first would be getting macb pointer from ptp ?
> struct macb *bp = container_of(ptp, struct macb, ptp_clock_info);
Yes. Unless your sub-function is used in more than one place, then it
is wasteful and confusing to wrap the functionality for no apparent
reason.
> > > + switch (rq->type) {
> > > + case PTP_CLK_REQ_EXTTS: /* Toggle TSU match interrupt */
> > > + if (on)
> > > + macb_writel(bp, IER, MACB_BIT(TCI));
> >
> > No locking to protect IER and IDE?
> There is no need.
But what happens when the PTP_CLK_REQ_EXTTS and PTP_CLK_REQ_PPS ioctls
are called at the same time?
You need to ensure that IDR is consistent. If the bits are write
only, then you should comment this fact.
> > > + else
> > > + macb_writel(bp, IDR, MACB_BIT(TCI));
> > > + break;
> > > + case PTP_CLK_REQ_PEROUT: /* Toggle Periodic output */
> > > + return -EOPNOTSUPP;
> > > + /* break; */
> > > + case PTP_CLK_REQ_PPS: /* Toggle TSU periodic (second)
> > interrupt */
> > > + if (on)
> > > + macb_writel(bp, IER, MACB_BIT(SRI));
> > > + else
> > > + macb_writel(bp, IDR, MACB_BIT(SRI));
> > > + break;
> > > + default:
> > > + break;
> > > + }
> > > + return 0;
> > > +}
Thanks,
Richard
RE: [PATCH 3/4] net: macb: Add hardware PTP support
> From: Richard Cochran [mailto:richardcoch...@gmail.com]
> Sent: 14 kwietnia 2017 20:29
> To: Rafal Ozieblo
> Cc: David Miller ; nicolas.fe...@atmel.com;
> netdev@vger.kernel.org; linux-ker...@vger.kernel.org;
> devicet...@vger.kernel.org; linux-arm-ker...@lists.infradead.org;
> harinikatakamli...@gmail.com; harini.kata...@xilinx.com;
> andrei.pistir...@microchip.com
> Subject: Re: [PATCH 3/4] net: macb: Add hardware PTP support
>
> On Thu, Apr 13, 2017 at 02:39:23PM +0100, Rafal Ozieblo wrote:
(...)
> > +static int gem_tsu_get_time(struct macb *bp, struct timespec64 *ts)
> > +{
> > + long first, second;
> > + u32 secl, sech;
> > + unsigned long flags;
> > +
> > + if (!bp || !ts)
> > + return -EINVAL;
>
> Useless test.
Sorry for me being too carefulness, I'll remove all tests.
>
(...)
> > +static int gem_ptp_gettime(struct ptp_clock_info *ptp, struct timespec64
> *ts)
> > +{
> > + struct macb *bp = container_of(ptp, struct macb, ptp_clock_info);
> > +
> > + if (!ptp || !ts)
> > + return -EINVAL;
>
> Useles test.
>
> What is the point of this wrapper function anyhow? Please remove it.
gem_ptp_gettime() is assigned in ptp_clock_info and it has to have
ptp_clock_info pointer as first parameter. gem_tsu_get_time() is used in
the source code but with macb pointer.
Do you want me to do something like:
gem_ptp_gettime(macb->ptp, ts);
and first would be getting macb pointer from ptp ?
struct macb *bp = container_of(ptp, struct macb, ptp_clock_info);
>
> > +
> > + gem_tsu_get_time(bp, ts);
> > + return 0;
> > +}
> > +
> > +static int gem_ptp_settime(struct ptp_clock_info *ptp,
> > + const struct timespec64 *ts)
> > +{
> > + struct macb *bp = container_of(ptp, struct macb, ptp_clock_info);
> > +
> > + if (!ptp || !ts)
> > + return -EINVAL;
>
> Another useless function.
ditto
>
> > + gem_tsu_set_time(bp, ts);
> > + return 0;
> > +}
> > +
> > +static int gem_ptp_enable(struct ptp_clock_info *ptp,
> > + struct ptp_clock_request *rq, int on)
> > +{
> > + struct macb *bp = container_of(ptp, struct macb, ptp_clock_info);
> > +
> > + if (!ptp || !rq)
> > + return -EINVAL;
>
> Sigh.
>
> > +
> > + switch (rq->type) {
> > + case PTP_CLK_REQ_EXTTS: /* Toggle TSU match interrupt */
> > + if (on)
> > + macb_writel(bp, IER, MACB_BIT(TCI));
>
> No locking to protect IER and IDE?
There is no need.
>
> > + else
> > + macb_writel(bp, IDR, MACB_BIT(TCI));
> > + break;
> > + case PTP_CLK_REQ_PEROUT: /* Toggle Periodic output */
> > + return -EOPNOTSUPP;
> > + /* break; */
> > + case PTP_CLK_REQ_PPS: /* Toggle TSU periodic (second)
> interrupt */
> > + if (on)
> > + macb_writel(bp, IER, MACB_BIT(SRI));
> > + else
> > + macb_writel(bp, IDR, MACB_BIT(SRI));
> > + break;
> > + default:
> > + break;
> > + }
> > + return 0;
> > +}
> > +
(...)
> > --
> > 2.4.5
> >
>
> Thanks,
> Richard
Re: [PATCH 3/4] net: macb: Add hardware PTP support
On Thu, Apr 13, 2017 at 02:39:23PM +0100, Rafal Ozieblo wrote:
> This patch is based on original Harini's patch and Andrei's patch,
> implemented in a separate file to ease the review/maintanance
> and integration with other platforms.
Please see if you can break this patch into 2 parts:
1. SO_TIMESTAMPING
2. PHC support
> This driver does support GEM-GXL:
"This driver supports GEM-GXL:"
> - HW time stamp on the PTP Ethernet packets are received using the
> SO_TIMESTAMPING API. Where timers are obtained from the dma buffer
> descriptors
This text is poor. No "timers" are obtained but rather time stamps.
Also, second sentence is not a sentence. (An english sentence has a
noun and a verb.)
> diff --git a/drivers/net/ethernet/cadence/macb.c
> b/drivers/net/ethernet/cadence/macb.c
> index 59d459b..603bac1 100644
> --- a/drivers/net/ethernet/cadence/macb.c
> +++ b/drivers/net/ethernet/cadence/macb.c
> @@ -826,6 +826,15 @@ static void macb_tx_interrupt(struct macb_queue *queue)
>
> /* First, update TX stats if needed */
> if (skb) {
> +#ifdef CONFIG_MACB_USE_HWSTAMP
No need for ifdef here. Instead, let gem_ptp_do_txstamp() return -1.
> + if (gem_ptp_do_txstamp(queue, skb, desc) == 0) {
> + /* skb now belongs to timestamp buffer
> + * and will be removed later
> + */
> + tx_skb->skb = NULL;
> + schedule_work(&queue->tx_ts_task);
> + }
> +#endif
> netdev_vdbg(bp->dev, "skb %u (data %p) TX
> complete\n",
> macb_tx_ring_wrap(bp, tail),
> skb->data);
> @@ -992,6 +1001,10 @@ static int gem_rx(struct macb *bp, int budget)
> bp->dev->stats.rx_packets++;
> bp->dev->stats.rx_bytes += skb->len;
>
> +#ifdef CONFIG_MACB_USE_HWSTAMP
No ifdef needed.
> + gem_ptp_do_rxstamp(bp, skb, desc);
> +#endif
> +
> #if defined(DEBUG) && defined(VERBOSE_DEBUG)
> netdev_vdbg(bp->dev, "received skb of length %u, csum: %08x\n",
> skb->len, skb->csum);
> @@ -1314,6 +1327,11 @@ static irqreturn_t macb_interrupt(int irq, void
> *dev_id)
> queue_writel(queue, ISR, MACB_BIT(HRESP));
> }
>
> +#ifdef CONFIG_MACB_USE_HWSTAMP
> + if (status & MACB_PTP_INT_MASK)
Can't you use IS_ENABLED(CONFIG_MACB_USE_HWSTAMP) here?
> + macb_ptp_int(queue, status);
> +#endif
> +
> status = queue_readl(queue, ISR);
> }
>
> @@ -1643,8 +1661,10 @@ static int macb_start_xmit(struct sk_buff *skb, struct
> net_device *dev)
>
> /* Make newly initialized descriptor visible to hardware */
> wmb();
> -
> - skb_tx_timestamp(skb);
> +#ifdef CONFIG_MACB_USE_HWSTAMP
> + if (!bp->ptp_hw_support)
> +#endif
> + skb_tx_timestamp(skb);
This is wrong. You should call skb_tx_timestamp() unconditionally,
but be sure to set SKBTX_IN_PROGRESS when appropriate.
> diff --git a/drivers/net/ethernet/cadence/macb.h
> b/drivers/net/ethernet/cadence/macb.h
> index 2606970..5295045 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -11,6 +11,9 @@
> #define _MACB_H
>
> #include
> +#include
You don't need to include ptp_clock.h.
> +#include
> +#include
>
> #if defined(CONFIG_ARCH_DMA_ADDR_T_64BIT) || defined(CONFIG_MACB_USE_HWSTAMP)
> #define MACB_EXT_DESC
...
> @@ -527,6 +595,8 @@
> #define queue_readl(queue, reg)
> (queue)->bp->macb_reg_readl((queue)->bp, (queue)->reg)
> #define queue_writel(queue, reg, value)
> (queue)->bp->macb_reg_writel((queue)->bp, (queue)->reg, (value))
>
> +#define PTP_TS_BUFFER_SIZE 128 /* must be power of 2 */
> +
> /* Conditional GEM/MACB macros. These perform the operation to the correct
> * register dependent on whether the device is a GEM or a MACB. For
> registers
> * and bitfields that are common across both devices, use macb_{read,write}l
> @@ -889,6 +959,18 @@ struct macb_config {
> int jumbo_max_len;
> };
>
> +#ifdef CONFIG_MACB_USE_HWSTAMP
No need for ifdef here.
> +struct tsu_incr {
> + u32 sub_ns;
> + u32 ns;
> +};
> +
> +struct gem_tx_ts {
> + struct sk_buff *skb;
> + struct macb_dma_desc_ptp desc_ptp;
> +};
> +#endif
> +
> struct macb_queue {
> struct macb *bp;
> int irq;
...
> diff --git a/drivers/net/ethernet/cadence/macb_ptp.c
> b/drivers/net/ethernet/cadence/macb_ptp.c
> new file mode 100755
> index 000..72a79c4
> --- /dev/null
> +++ b/drivers/net/ethernet/cadence/macb_ptp.c
> @@ -0,0 +1,724 @@
> +/**
> + * 1588 PTP support for Cadence G
Re: [PATCH 3/4] net: macb: Add hardware PTP support
Hi Rafal,
[auto build test ERROR on net-next/master]
[also build test ERROR on v4.11-rc6 next-20170413]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system]
url:
https://github.com/0day-ci/linux/commits/Rafal-Ozieblo/net-macb-Add-support-for-PTP-timestamps-in-DMA-descriptors/20170414-001330
config: arm64-defconfig (attached as .config)
compiler: aarch64-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
wget
https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm64
Note: the
linux-review/Rafal-Ozieblo/net-macb-Add-support-for-PTP-timestamps-in-DMA-descriptors/20170414-001330
HEAD 3b878618e04f866388fd62f6c44752e50b15658a builds fine.
It only hurts bisectibility.
All errors (new ones prefixed by >>):
drivers/built-in.o: In function `gem_ptp_do_rxstamp':
>> drivers/net/ethernet/cadence/macb.h:1108: undefined reference to
>> `gem_ptp_rxstamp'
drivers/net/ethernet/cadence/macb.h:1108:(.text+0x264604): relocation
truncated to fit: R_AARCH64_CALL26 against undefined symbol `gem_ptp_rxstamp'
drivers/built-in.o: In function `gem_ptp_do_txstamp':
>> drivers/net/ethernet/cadence/macb.h:1100: undefined reference to
>> `gem_ptp_txstamp'
drivers/net/ethernet/cadence/macb.h:1100:(.text+0x266564): relocation
truncated to fit: R_AARCH64_CALL26 against undefined symbol `gem_ptp_txstamp'
drivers/built-in.o: In function `macb_interrupt':
>> drivers/net/ethernet/cadence/macb.c:1332: undefined reference to
>> `macb_ptp_int'
drivers/net/ethernet/cadence/macb.c:1332:(.text+0x2666ac): relocation
truncated to fit: R_AARCH64_CALL26 against undefined symbol `macb_ptp_int'
drivers/built-in.o:(.data+0x93fc8): undefined reference to `gem_ptp_init'
drivers/built-in.o:(.data+0x93fd0): undefined reference to `gem_ptp_remove'
drivers/built-in.o:(.data+0x93ff0): undefined reference to `gem_get_hwtst'
drivers/built-in.o:(.data+0x93ff8): undefined reference to `gem_set_hwtst'
vim +1108 drivers/net/ethernet/cadence/macb.h
1094 void macb_ptp_int(struct macb_queue *queue, u32 status);
1095 static inline int gem_ptp_do_txstamp(struct macb_queue *queue, struct
sk_buff *skb, struct macb_dma_desc *desc)
1096 {
1097 if (queue->bp->tstamp_config.tx_type == TSTAMP_DISABLED)
1098 return -ENOTSUPP;
1099
> 1100 return gem_ptp_txstamp(queue, skb, desc);
1101 }
1102
1103 static inline void gem_ptp_do_rxstamp(struct macb *bp, struct sk_buff
*skb, struct macb_dma_desc *desc)
1104 {
1105 if (bp->tstamp_config.rx_filter == TSTAMP_DISABLED)
1106 return;
1107
> 1108 gem_ptp_rxstamp(bp, skb, desc);
1109 }
1110 int gem_get_hwtst(struct net_device *dev, struct ifreq *rq);
int gem_set_hwtst(struct net_device *dev, struct ifreq *ifr, int cmd);
---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
.config.gz
Description: application/gzip
Re: [PATCH 3/4] net: macb: Add hardware PTP support
Hi Rafal, [auto build test ERROR on net-next/master] [also build test ERROR on v4.11-rc6 next-20170413] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Rafal-Ozieblo/net-macb-Add-support-for-PTP-timestamps-in-DMA-descriptors/20170414-001330 config: arm-at91_dt_defconfig (attached as .config) compiler: arm-linux-gnueabi-gcc (Debian 6.1.1-9) 6.1.1 20160705 reproduce: wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree make.cross ARCH=arm Note: the linux-review/Rafal-Ozieblo/net-macb-Add-support-for-PTP-timestamps-in-DMA-descriptors/20170414-001330 HEAD 3b878618e04f866388fd62f6c44752e50b15658a builds fine. It only hurts bisectibility. All errors (new ones prefixed by >>): drivers/built-in.o: In function `macb_interrupt': >> kfifo_buf.c:(.text+0xeabd8): undefined reference to `macb_ptp_int' >> kfifo_buf.c:(.text+0xeac6c): undefined reference to `gem_ptp_txstamp' drivers/built-in.o: In function `gem_rx': >> kfifo_buf.c:(.text+0xebbd8): undefined reference to `gem_ptp_rxstamp' >> drivers/built-in.o:(.data+0xa5e0): undefined reference to `gem_ptp_init' >> drivers/built-in.o:(.data+0xa5e4): undefined reference to `gem_ptp_remove' >> drivers/built-in.o:(.data+0xa5f4): undefined reference to `gem_get_hwtst' >> drivers/built-in.o:(.data+0xa5f8): undefined reference to `gem_set_hwtst' --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: application/gzip
[PATCH 3/4] net: macb: Add hardware PTP support
This patch is based on original Harini's patch and Andrei's patch,
implemented in a separate file to ease the review/maintanance
and integration with other platforms.
This driver does support GEM-GXL:
- Register ptp clock framework
- Initialize PTP related registers
- HW time stamp on the PTP Ethernet packets are received using the
SO_TIMESTAMPING API. Where timers are obtained from the dma buffer
descriptors
Signed-off-by: Rafal Ozieblo
---
drivers/net/ethernet/cadence/macb.c | 99 -
drivers/net/ethernet/cadence/macb.h | 140 ++
drivers/net/ethernet/cadence/macb_ptp.c | 724
3 files changed, 958 insertions(+), 5 deletions(-)
create mode 100755 drivers/net/ethernet/cadence/macb_ptp.c
diff --git a/drivers/net/ethernet/cadence/macb.c
b/drivers/net/ethernet/cadence/macb.c
index 59d459b..603bac1 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -826,6 +826,15 @@ static void macb_tx_interrupt(struct macb_queue *queue)
/* First, update TX stats if needed */
if (skb) {
+#ifdef CONFIG_MACB_USE_HWSTAMP
+ if (gem_ptp_do_txstamp(queue, skb, desc) == 0) {
+ /* skb now belongs to timestamp buffer
+* and will be removed later
+*/
+ tx_skb->skb = NULL;
+ schedule_work(&queue->tx_ts_task);
+ }
+#endif
netdev_vdbg(bp->dev, "skb %u (data %p) TX
complete\n",
macb_tx_ring_wrap(bp, tail),
skb->data);
@@ -992,6 +1001,10 @@ static int gem_rx(struct macb *bp, int budget)
bp->dev->stats.rx_packets++;
bp->dev->stats.rx_bytes += skb->len;
+#ifdef CONFIG_MACB_USE_HWSTAMP
+ gem_ptp_do_rxstamp(bp, skb, desc);
+#endif
+
#if defined(DEBUG) && defined(VERBOSE_DEBUG)
netdev_vdbg(bp->dev, "received skb of length %u, csum: %08x\n",
skb->len, skb->csum);
@@ -1314,6 +1327,11 @@ static irqreturn_t macb_interrupt(int irq, void *dev_id)
queue_writel(queue, ISR, MACB_BIT(HRESP));
}
+#ifdef CONFIG_MACB_USE_HWSTAMP
+ if (status & MACB_PTP_INT_MASK)
+ macb_ptp_int(queue, status);
+#endif
+
status = queue_readl(queue, ISR);
}
@@ -1643,8 +1661,10 @@ static int macb_start_xmit(struct sk_buff *skb, struct
net_device *dev)
/* Make newly initialized descriptor visible to hardware */
wmb();
-
- skb_tx_timestamp(skb);
+#ifdef CONFIG_MACB_USE_HWSTAMP
+ if (!bp->ptp_hw_support)
+#endif
+ skb_tx_timestamp(skb);
macb_writel(bp, NCR, macb_readl(bp, NCR) | MACB_BIT(TSTART));
@@ -2502,6 +2522,71 @@ static int macb_set_ringparam(struct net_device *netdev,
return 0;
}
+#ifdef CONFIG_MACB_USE_HWSTAMP
+static unsigned int gem_get_tsu_rate(struct macb *bp)
+{
+ struct clk *tsu_clk;
+ unsigned int tsu_rate;
+
+ tsu_clk = devm_clk_get(&bp->pdev->dev, "tsu_clk");
+ if (!IS_ERR(tsu_clk))
+ tsu_rate = clk_get_rate(tsu_clk);
+ /* try pclk instead */
+ else if (!IS_ERR(bp->pclk)) {
+ tsu_clk = bp->pclk;
+ tsu_rate = clk_get_rate(tsu_clk);
+ } else
+ return -ENOTSUPP;
+ return tsu_rate;
+}
+
+static s32 gem_get_ptp_max_adj(void)
+{
+ return 64E6;
+}
+
+static int gem_get_ts_info(struct net_device *dev,
+ struct ethtool_ts_info *info)
+{
+ struct macb *bp = netdev_priv(dev);
+
+ ethtool_op_get_ts_info(dev, info);
+ if (!bp->ptp_hw_support)
+ return 0;
+
+ info->so_timestamping =
+ SOF_TIMESTAMPING_TX_SOFTWARE |
+ SOF_TIMESTAMPING_RX_SOFTWARE |
+ SOF_TIMESTAMPING_SOFTWARE |
+ SOF_TIMESTAMPING_TX_HARDWARE |
+ SOF_TIMESTAMPING_RX_HARDWARE |
+ SOF_TIMESTAMPING_RAW_HARDWARE;
+ info->tx_types =
+ (1 << HWTSTAMP_TX_ONESTEP_SYNC) |
+ (1 << HWTSTAMP_TX_OFF) |
+ (1 << HWTSTAMP_TX_ON);
+ info->rx_filters =
+ (1 << HWTSTAMP_FILTER_NONE) |
+ (1 << HWTSTAMP_FILTER_ALL);
+ info->phc_index = -1;
+
+ if (bp->ptp_clock)
+ info->phc_index = ptp_clock_index(bp->ptp_clock);
+
+ return 0;
+}
+
+static struct macb_ptp_info gem_ptp_info = {
+ .ptp_init= gem_ptp_init,
+ .ptp_remove = gem_ptp_remove,
+ .get_ptp_max_adj = gem_get_ptp_max_adj,
+ .get_tsu_rate= gem_get_tsu_rate,
+ .get_ts_info = gem_get_ts
