From: Andrei Pistirica <hidden> Date: 2016-12-14 10:06:12
Cadence GEM provides a 102 bit time counter with 48 bits for seconds,
30 bits for nsecs and 24 bits for sub-nsecs to control 1588 timestamping.
This patch does the following:
- Registers to ptp clock framework
- Timer initialization is done by writing time of day to the timer counter.
- ns increment register is programmed as NSEC_PER_SEC/tsu-clock-rate.
For a 16 bit subns precision, the subns increment equals
remainder of (NS_PER_SEC/TSU_CLK) * (2^16).
- Timestamps are obtained from the TX/RX PTP event/PEER registers.
The timestamp obtained thus is updated in skb for upper layers to access.
- The drivers register functions with ptp to perform time and frequency
adjustment.
- Time adjustment is done by writing to the 1558_ADJUST register.
The controller will read the delta in this register and update the timer
counter register. Alternatively, for large time offset adjustments,
the driver reads the secs and nsecs counter values, adds/subtracts the
delta and updates the timer counter.
- Frequency is adjusted by adjusting addend (8bit nanosecond increment) and
addendsub (16bit increment nanosecond fractions).
The 102bit counter is incremented at nominal frequency with addend and
addendsub values. Each period addend and addendsub values are adjusted
based on ppm drift.
Signed-off-by: Andrei Pistirica <redacted>
Signed-off-by: Harini Katakam <redacted>
---
Patch history:
Version 1:
This patch is based on original Harini's patch, implemented in a
separate file to ease the review/maintanance and integration with
other platforms (e.g. Zynq Ultrascale+ MPSoC).
Feature was tested on SAMA5D2 platform using ptp4l v1.6 from linuxptp
project and also with ptpd2 version 2.3.1. PTP was tested over
IPv4,IPv6 and 802.3 protocols.
In case that macb is compiled as a module, it has been renamed to
cadence-macb.ko to avoid naming confusion in Makefile.
Version 2 modifications:
- bitfields for TSU are named according to SAMA5D2 data sheet
- identify GEM-PTP support based on platform capability
- add spinlock for TSU access
- change macb_ptp_adjfreq and use fewer 64bit divisions
Version 3 modifications:
- new adjfine api with one 64 division for frequency adjustment
(based on Richard's input)
- add maximum adjustment frequency (ppb) based on nominal frequency
- per platform PTP configuration
- cosmetic changes
Note 1: Kbuild uses "select" instead of "imply", and the macb maintainer agreed
to make the change when it will be available in net-next.
Version 4 modifications:
- update adjfine for a better approximation
- add maximum adjustment frequency callback to PTP platform configuraion
Note 1: This driver does not support GEM-GXL!
Note 2: Patch on net-next, on December 14th.
drivers/net/ethernet/cadence/Kconfig | 10 +-
drivers/net/ethernet/cadence/Makefile | 8 +-
drivers/net/ethernet/cadence/macb.h | 118 ++++++++++
drivers/net/ethernet/cadence/macb_ptp.c | 366 ++++++++++++++++++++++++++++++++
4 files changed, 500 insertions(+), 2 deletions(-)
create mode 100644 drivers/net/ethernet/cadence/macb_ptp.c
From: Andrei Pistirica <hidden> Date: 2016-12-14 10:05:40
This patch does the following:
- Enable HW time stamp for the following platforms: SAMA5D2, SAMA5D3 and
SAMA5D4.
- HW time stamp capabilities are advertised via ethtool and macb ioctl is
updated accordingly.
- HW time stamp on the PTP Ethernet packets are received using the
SO_TIMESTAMPING API. Where timers are obtained from the PTP event/peer
registers.
Note: Patch on net-next, on December 7th.
Signed-off-by: Andrei Pistirica <redacted>
---
Patch history:
Version 1:
Integration with SAMA5D2 only. This feature wasn't tested on any
other platform that might use cadence/gem.
Patch is not completely ported to the very latest version of net-next,
and it will be after review.
Version 2 modifications:
- add PTP caps for SAMA5D2/3/4 platforms
- and cosmetic changes
Version 3 modifications:
- add support for sama5D2/3/4 platforms using GEM-PTP interface.
Version 4 modifications:
- time stamp only PTP_V2 events
- maximum adjustment value is set based on Richard's input
Note: Patch on net-next, on December 14th.
drivers/net/ethernet/cadence/macb.c | 168 ++++++++++++++++++++++++++++++++++--
1 file changed, 163 insertions(+), 5 deletions(-)
From: Andrei Pistirica [mailto:andrei.pistirica@microchip.com]
Sent: 14 grudnia 2016 13:56
quoted hunk
This patch does the following:
- Enable HW time stamp for the following platforms: SAMA5D2, SAMA5D3 and
SAMA5D4.
- HW time stamp capabilities are advertised via ethtool and macb ioctl is
updated accordingly.
- HW time stamp on the PTP Ethernet packets are received using the
SO_TIMESTAMPING API. Where timers are obtained from the PTP event/peer
registers.
Note: Patch on net-next, on December 7th.
Signed-off-by: Andrei Pistirica <redacted>
---
Patch history:
Version 1:
Integration with SAMA5D2 only. This feature wasn't tested on any other platform that might use cadence/gem.
Patch is not completely ported to the very latest version of net-next, and it will be after review.
Version 2 modifications:
- add PTP caps for SAMA5D2/3/4 platforms
- and cosmetic changes
Version 3 modifications:
- add support for sama5D2/3/4 platforms using GEM-PTP interface.
Version 4 modifications:
- time stamp only PTP_V2 events
- maximum adjustment value is set based on Richard's input
Note: Patch on net-next, on December 14th.
drivers/net/ethernet/cadence/macb.c | 168 ++++++++++++++++++++++++++++++++++--
1 file changed, 163 insertions(+), 5 deletions(-)
I think, you can not do it in that way.
It will hold two locks. If you enable appropriate option in kernel (as far as I
remember CONFIG_DEBUG_SPINLOCK) you will get a warning here.
Please look at following call-stack:
1. macb_interrupt() // spin_lock(&bp->lock) is taken
2. macb_tx_interrupt()
3. macb_handle_txtstamp()
4. skb_tstamp_tx()
5. __skb_tstamp_tx()
6. skb_may_tx_timestamp()
7. read_lock_bh() // second lock is taken
I know that those are different locks and different types. But this could lead
to deadlocks. This is the reason of warning I could see.
And this is the reason why I get timestamp in interrupt routine but pass it to
skb outside interrupt (using circular buffer).
Please, refer to this:
https://lkml.org/lkml/2016/11/18/168
1. macb_tx_interrupt()
2. macb_tx_timestamp_add() and schedule_work(&queue->tx_timestamp_task)
Then, outside interrupt (without holding a lock) :
1. macb_tx_timestamp_flush()
2. macb_tstamp_tx()
3. skb_tstamp_tx()
@@ -2080,6 +2084,9 @@ static int macb_open(struct net_device *dev) netif_tx_start_all_queues(dev); + if (bp->ptp_info) + bp->ptp_info->ptp_init(dev); + return 0; }
@@ -2101,6 +2108,9 @@ static int macb_close(struct net_device *dev) macb_free_consistent(bp); + if (bp->ptp_info) + bp->ptp_info->ptp_remove(dev); + return 0; }
@@ -2374,6 +2384,133 @@ 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) { + /* Note: TSU rate is hardwired to PCLK. */ + return clk_get_rate(bp->pclk); +}
Not exactly. There could be separate TSU clock.
In my solution I check tsu_clk in DT before I decide to take pclk.
But it could be change in macb_ptp_info.
@@ -2411,7 +2549,20 @@ static int macb_ioctl(struct net_device *dev, struct ifreq *rq, int cmd) if (!phydev) return -ENODEV;- return phy_mii_ioctl(phydev, rq, cmd);+ switch (cmd) {+ case SIOCSHWTSTAMP:+ if (bp->ptp_info)+ return bp->ptp_info->set_hwtst(dev, rq, cmd);++ return -EOPNOTSUPP;+ case SIOCGHWTSTAMP:+ if (bp->ptp_info)+ return bp->ptp_info->get_hwtst(dev, rq);++ return -EOPNOTSUPP;+ default:+ return phy_mii_ioctl(phydev, rq, cmd);+ } } static int macb_set_features(struct net_device *netdev, @@ -2485,6 +2636,12 @@ static void macb_configure_caps(struct macb *bp, dcfg = gem_readl(bp, DCFG2); if ((dcfg & (GEM_BIT(RX_PKT_BUFF) | GEM_BIT(TX_PKT_BUFF))) == 0) bp->caps |= MACB_CAPS_FIFO_MODE;++ /* iff HWSTAMP is configure and gem has the capability */ #ifdef+CONFIG_MACB_USE_HWSTAMP+ if (gem_has_ptp(bp))+ bp->ptp_info = &gem_ptp_info;+#endif } dev_dbg(&bp->pdev->dev, "Cadence caps 0x%08x\n", bp->caps); @@ -3041,7 +3198,7 @@ static const struct macb_config pc302gem_config = { }; static const struct macb_config sama5d2_config = {- .caps = MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII,+ .caps = MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII | MACB_CAPS_GEM_HAS_PTP,
There are many IP cores with many configuration. If it is possible, capabilities should be read from IP directly.
And it is possible in that case:
Design Configuration Register 5 (0x290)
bit 8: tsu
There is now PTP hardware support without that bit.
In macb_start_xmit() there is also invoked skb_tx_timestamp() for software timestamping.
I think, it should be disabled if you do hardware timestamping.
Best regards,
Rafal Ozieblo | Firmware System Engineer,
www.cadence.com
From: Andrei Pistirica [mailto:andrei.pistirica@microchip.com]
Sent: 14 grudnia 2016 13:56
Subject: [RFC PATCH net-next v4 1/2] macb: Add 1588 support in Cadence GEM.
Cadence GEM provides a 102 bit time counter with 48 bits for seconds,
30 bits for nsecs and 24 bits for sub-nsecs to control 1588 timestamping.
This patch does the following:
- Registers to ptp clock framework
- Timer initialization is done by writing time of day to the timer counter.
- ns increment register is programmed as NSEC_PER_SEC/tsu-clock-rate.
For a 16 bit subns precision, the subns increment equals
remainder of (NS_PER_SEC/TSU_CLK) * (2^16).
- Timestamps are obtained from the TX/RX PTP event/PEER registers.
The timestamp obtained thus is updated in skb for upper layers to access.
- The drivers register functions with ptp to perform time and frequency
adjustment.
- Time adjustment is done by writing to the 1558_ADJUST register.
The controller will read the delta in this register and update the timer
counter register. Alternatively, for large time offset adjustments,
the driver reads the secs and nsecs counter values, adds/subtracts the
delta and updates the timer counter.
- Frequency is adjusted by adjusting addend (8bit nanosecond increment) and
addendsub (16bit increment nanosecond fractions).
The 102bit counter is incremented at nominal frequency with addend and
addendsub values. Each period addend and addendsub values are adjusted
based on ppm drift.
Signed-off-by: Andrei Pistirica <redacted>
Signed-off-by: Harini Katakam <redacted>
---
Patch history:
Version 1:
This patch is based on original Harini's patch, implemented in a separate file to ease the review/maintanance and integration with other platforms (e.g. Zynq Ultrascale+ MPSoC).
Feature was tested on SAMA5D2 platform using ptp4l v1.6 from linuxptp project and also with ptpd2 version 2.3.1. PTP was tested over
IPv4,IPv6 and 802.3 protocols.
In case that macb is compiled as a module, it has been renamed to cadence-macb.ko to avoid naming confusion in Makefile.
Version 2 modifications:
- bitfields for TSU are named according to SAMA5D2 data sheet
- identify GEM-PTP support based on platform capability
- add spinlock for TSU access
- change macb_ptp_adjfreq and use fewer 64bit divisions
Version 3 modifications:
- new adjfine api with one 64 division for frequency adjustment
(based on Richard's input)
- add maximum adjustment frequency (ppb) based on nominal frequency
- per platform PTP configuration
- cosmetic changes
Note 1: Kbuild uses "select" instead of "imply", and the macb maintainer agreed
to make the change when it will be available in net-next.
Version 4 modifications:
- update adjfine for a better approximation
- add maximum adjustment frequency callback to PTP platform configuraion
Note 1: This driver does not support GEM-GXL!
Note 2: Patch on net-next, on December 14th.
drivers/net/ethernet/cadence/Kconfig | 10 +-
drivers/net/ethernet/cadence/Makefile | 8 +-
drivers/net/ethernet/cadence/macb.h | 118 ++++++++++
drivers/net/ethernet/cadence/macb_ptp.c | 366 ++++++++++++++++++++++++++++++++
4 files changed, 500 insertions(+), 2 deletions(-) create mode 100644 drivers/net/ethernet/cadence/macb_ptp.c
@@ -0,0 +1,366 @@+/*+*1588PTPsupportforGEMdevice.+*+*Copyright(C)2016MicrochipTechnology+*+*ThisfileislicensedunderthetermsoftheGNUGeneralPublic+*Licenseversion2.Thisprogramislicensed"as is"withoutany+*warrantyofanykind,whetherexpressorimplied.+*/++#include<linux/clk.h>+#include<linux/device.h>+#include<linux/etherdevice.h>+#include<linux/platform_device.h>+#include<linux/time64.h>+#include<linux/ptp_classify.h>+#include<linux/if_ether.h>+#include<linux/if_vlan.h>+#include<linux/net_tstamp.h>++#include"macb.h"++#define GEM_PTP_TIMER_NAME "gem-ptp-timer"++staticinlinevoidgem_tsu_get_time(structmacb*bp,+structtimespec64*ts)+{+u64sec,sech,secl;++spin_lock(&bp->tsu_clk_lock);++/* GEM's internal time */+sech=gem_readl(bp,TSH);+secl=gem_readl(bp,TSL);+ts->tv_nsec=gem_readl(bp,TN);+ts->tv_sec=(sech<<32)|secl;++/* minimize error */+sech=gem_readl(bp,TSH);+secl=gem_readl(bp,TSL);+sec=(sech<<32)|secl;+if(ts->tv_sec!=sec){+ts->tv_sec=sec;+ts->tv_nsec=gem_readl(bp,TN);+}++spin_unlock(&bp->tsu_clk_lock);+}++staticinlinevoidgem_tsu_set_time(structmacb*bp,+conststructtimespec64*ts)+{+u32ns,sech,secl;+s64word_mask=0xffffffff;++sech=(u32)ts->tv_sec;+secl=(u32)ts->tv_sec;+ns=ts->tv_nsec;+if(ts->tv_sec>word_mask)+sech=(ts->tv_sec>>32);++spin_lock(&bp->tsu_clk_lock);++/* TSH doesn't latch the time and no atomicity! */+gem_writel(bp,TN,0);/* clear to avoid overflow */+gem_writel(bp,TSH,sech);+gem_writel(bp,TSL,secl);+gem_writel(bp,TN,ns);++spin_unlock(&bp->tsu_clk_lock);+}++staticintgem_ptp_adjfine(structptp_clock_info*ptp,longscaled_ppm)+{+structmacb*bp=container_of(ptp,structmacb,ptp_caps);+u32word,diff;+u64adj,rate;+intneg_adj=0;++if(scaled_ppm<0){+neg_adj=1;+scaled_ppm=-scaled_ppm;+}+rate=scaled_ppm;++/* word: unused(8bit) | ns(8bit) | fractions(16bit) */+word=(bp->ns_incr<<16)+bp->subns_incr;++adj=word;+adj*=rate;+adj+=500000UL<<16;+adj>>=16;/* remove fractions */+diff=div_u64(adj,1000000UL);+word=neg_adj?word-diff:word+diff;++spin_lock(&bp->tsu_clk_lock);++gem_writel(bp,TISUBN,GEM_BF(SUBNSINCR,(word&0xffff)));+gem_writel(bp,TI,GEM_BF(NSINCR,(word>>16)));++spin_unlock(&bp->tsu_clk_lock);+return0;+}++staticintgem_ptp_adjtime(structptp_clock_info*ptp,s64delta){+structmacb*bp=container_of(ptp,structmacb,ptp_caps);+structtimespec64now,then=ns_to_timespec64(delta);+u32adj,sign=0;++if(delta<0){+delta=-delta;+sign=1;+}++if(delta>0x3FFFFFFF){+gem_tsu_get_time(bp,&now);++if(sign)+now=timespec64_sub(now,then);+else+now=timespec64_add(now,then);++gem_tsu_set_time(bp,(conststructtimespec64*)&now);+}else{+adj=delta;+if(sign)+adj|=GEM_BIT(ADDSUB);++gem_writel(bp,TA,adj);+}++return0;+}++staticintgem_ptp_gettime(structptp_clock_info*ptp,struct+timespec64*ts){+structmacb*bp=container_of(ptp,structmacb,ptp_caps);++gem_tsu_get_time(bp,ts);++return0;+}++staticintgem_ptp_settime(structptp_clock_info*ptp,+conststructtimespec64*ts)+{+structmacb*bp=container_of(ptp,structmacb,ptp_caps);++gem_tsu_set_time(bp,ts);++return0;+}++staticintgem_ptp_enable(structptp_clock_info*ptp,+structptp_clock_request*rq,inton){+return-EOPNOTSUPP;+}
I think, we can support here:
1. PTP_CLK_REQ_EXTTS (interrupt mask register 0x030, bit 29: tsu_timer_comparison_mask)
2. PTP_CLK_REQ_PPS (interrupt mask register 0x030, bit 26: tsu_seconds_register_increment_mask)
Why do you change TSU clock here? Is it necessary? You overwrite all values
even when someone doesn't need it. ptp4l calls ioctl SIOCSHWTSTAMP on start.
IMHO, there should be set up TSU and Increments.
What is the reason for zeroing Timer Adjust Register?
+}
+
+static void gem_ptp_clear_timer(struct macb *bp) {
+ bp->ns_incr = 0;
+ bp->subns_incr = 0;
+
+ gem_writel(bp, TISUBN, GEM_BF(SUBNSINCR, 0));
+ gem_writel(bp, TI, GEM_BF(NSINCR, 0));
+ gem_writel(bp, TA, 0);
+}
+
+/* While GEM can timestamp PTP packets, it does not mark the RX
+descriptor
+ * to identify them. UDP packets must be parsed to identify PTP packets.
+ *
+ * Note: Inspired from drivers/net/ethernet/ti/cpts.c */ static int
+gem_get_ptp_peer(struct sk_buff *skb, int ptp_class) {
+ unsigned int offset = 0;
+ u8 *msgtype, *data = skb->data;
+
+ /* PTP frames are rare! */
+ if (likely(ptp_class == PTP_CLASS_NONE))
+ return -1;
+
+ if (ptp_class & PTP_CLASS_VLAN)
+ offset += VLAN_HLEN;
+
+ switch (ptp_class & PTP_CLASS_PMASK) {
+ case PTP_CLASS_IPV4:
+ offset += ETH_HLEN + IPV4_HLEN(data + offset) + UDP_HLEN;
+ break;
+ case PTP_CLASS_IPV6:
+ offset += ETH_HLEN + IP6_HLEN + UDP_HLEN;
+ break;
+ case PTP_CLASS_L2:
+ offset += ETH_HLEN;
+ break;
+
+ /* something went wrong! */
+ default:
+ return -1;
+ }
+
+ if (skb->len + ETH_HLEN < offset + OFF_PTP_SEQUENCE_ID)
+ return -1;
+
+ if (unlikely(ptp_class & PTP_CLASS_V1))
+ msgtype = data + offset + OFF_PTP_CONTROL;
+ else
+ msgtype = data + offset;
+
+ return (*msgtype) & 0x2;
+}
+
+static void gem_ptp_tx_hwtstamp(struct macb *bp, struct sk_buff *skb,
+ int peer_ev)
+{
+ struct skb_shared_hwtstamps *shhwtstamps = skb_hwtstamps(skb);
+ struct timespec64 ts;
+ u64 ns;
+
+ /* PTP Peer Event Frame packets */
+ if (peer_ev) {
+ ts.tv_sec = gem_readl(bp, PEFTSL);
+ ts.tv_nsec = gem_readl(bp, PEFTN);
+
+ /* PTP Event Frame packets */
+ } else {
+ ts.tv_sec = gem_readl(bp, EFTSL);
+ ts.tv_nsec = gem_readl(bp, EFTN);
+ }
I'm wondering what is a difference between timestamp in transmit buffer descriptor (Word 2 and 3)
and PTP Event Frame Transmitted Seconds/Nanoseconds Register (0x1E0, 0x1E4).
Why don't you support HWTSTAMP_TX_ONESTEP_SYNC?
(Network control register 0x000, bit 24: one_step_sync_mode)
Best regards,
Rafal Ozieblo | Firmware System Engineer,
www.cadence.com
I'm wondering what is a difference between timestamp in transmit buffer descriptor (Word 2 and 3) and PTP Event Frame Transmitted Seconds/Nanoseconds Register (0x1E0, 0x1E4).
According Cadence Hardware team:
"It is just that some customers prefer to have the time in the descriptors as that is provided per frame.
The registers are simply overwritten when a new event frame is transmitted/received and so software could miss it."
The question is are you sure that you read timestamp for current frame? (not for the next frame).
From: Richard Cochran <richardcochran@gmail.com> Date: 2017-01-02 11:32:04
On Mon, Jan 02, 2017 at 09:36:10AM +0000, Rafal Ozieblo wrote:
According Cadence Hardware team:
"It is just that some customers prefer to have the time in the descriptors as that is provided per frame.
The registers are simply overwritten when a new event frame is transmitted/received and so software could miss it."
The question is are you sure that you read timestamp for current frame? (not for the next frame).
AFAICT, having the time stamp in the descriptor is not universally
supported. Looking at the Xilinx Zynq 7000 TRM, I can't find any
mention of this.
This Cadence IP core is a complete disaster.
Unless someone can tell us how this IP works in all of its
incarnations, this series is going nowhere.
Thanks,
Richard
Hi Richard,
On Mon, Jan 2, 2017 at 5:01 PM, Richard Cochran
[off-list ref] wrote:
On Mon, Jan 02, 2017 at 09:36:10AM +0000, Rafal Ozieblo wrote:
quoted
According Cadence Hardware team:
"It is just that some customers prefer to have the time in the descriptors as that is provided per frame.
The registers are simply overwritten when a new event frame is transmitted/received and so software could miss it."
The question is are you sure that you read timestamp for current frame? (not for the next frame).
AFAICT, having the time stamp in the descriptor is not universally
supported. Looking at the Xilinx Zynq 7000 TRM, I can't find any
mention of this.
This Cadence IP core is a complete disaster.
Unless someone can tell us how this IP works in all of its
incarnations, this series is going nowhere.
From the revision history of Cadence spec, all versions starting
r1p02 have ability to include timestamp in descriptors.
For previous versions the event register is the only option.
But yes, there have been multiple enhancements and
bug fixes in this IP w.r.t PTP making each implementation
different.
Regards,
Harini
From: Nicolas Ferre <hidden> Date: 2017-01-02 14:46:55
Le 02/01/2017 à 12:31, Richard Cochran a écrit :
On Mon, Jan 02, 2017 at 09:36:10AM +0000, Rafal Ozieblo wrote:
quoted
According Cadence Hardware team:
"It is just that some customers prefer to have the time in the descriptors as that is provided per frame.
The registers are simply overwritten when a new event frame is transmitted/received and so software could miss it."
The question is are you sure that you read timestamp for current frame? (not for the next frame).
AFAICT, having the time stamp in the descriptor is not universally
supported. Looking at the Xilinx Zynq 7000 TRM, I can't find any
mention of this.
This is why I proposed to address options incrementally: without
timestamp support in descriptor (this patch series), then adding this
feature in another patch series.
Rafal, this is why Andrei noted that the case covered by this series is
not adapted to GEM-GXL and doesn't address the "timestamp in descriptor"
case.
This Cadence IP core is a complete disaster.
Well, it evolved and propose several options to different SoC
integrators. This is not something unusual...
I suspect as well that some other network adapters have the same
weakness concerning PTP timestamp in single register as the early
revisions of this IP.
Unless someone can tell us how this IP works in all of its
incarnations, this series is going nowhere.
We're already as v4 (thanks to your fruitful contributions BTW) for this
series and will try to add features for other IP options & revisions
incrementally.
I suspect that Rafal tend to jump too quickly to the latest IP revisions
and add more options to this series: let's not try to pour too much
things into this code right now.
FYI, Andrei will be back online next week.
Regards,
--
Nicolas Ferre
From: Richard Cochran <richardcochran@gmail.com> Date: 2017-01-02 16:14:07
On Mon, Jan 02, 2017 at 03:47:07PM +0100, Nicolas Ferre wrote:
Le 02/01/2017 à 12:31, Richard Cochran a écrit :
quoted
This Cadence IP core is a complete disaster.
Well, it evolved and propose several options to different SoC
integrators. This is not something unusual...
I suspect as well that some other network adapters have the same
weakness concerning PTP timestamp in single register as the early
revisions of this IP.
It appears that this core can neither latch the time on read or write,
or even latch time stamps. I have worked with many different PTP HW
implementations, even early ones like on the ixp4xx, and it is no
exaggeration to say that this one is uniquely broken.
I suspect that Rafal tend to jump too quickly to the latest IP revisions
and add more options to this series: let's not try to pour too much
things into this code right now.
Why can't you check the IP version in the driver?
And is it really true that the registers don't latch the time stamps,
as Rafal said? If so, then we cannot accept the non-descriptor driver
version, since it cannot possibly work correctly.
Thanks,
Richard
Hi Richard,
On Mon, Jan 2, 2017 at 9:43 PM, Richard Cochran
[off-list ref] wrote:
On Mon, Jan 02, 2017 at 03:47:07PM +0100, Nicolas Ferre wrote:
quoted
Le 02/01/2017 à 12:31, Richard Cochran a écrit :
quoted
This Cadence IP core is a complete disaster.
Well, it evolved and propose several options to different SoC
integrators. This is not something unusual...
I suspect as well that some other network adapters have the same
weakness concerning PTP timestamp in single register as the early
revisions of this IP.
It appears that this core can neither latch the time on read or write,
or even latch time stamps. I have worked with many different PTP HW
implementations, even early ones like on the ixp4xx, and it is no
exaggeration to say that this one is uniquely broken.
quoted
I suspect that Rafal tend to jump too quickly to the latest IP revisions
and add more options to this series: let's not try to pour too much
things into this code right now.
Why can't you check the IP version in the driver?
There is an IP revision register but it would be probably be better
to rely on "caps" from the compatibility strings - to cover SoC
specific implementations. Also, when this extended BD is
added (with timestamp), additional words will need to be added
statically which will be consistent with Andrei's CONFIG_
checks.
And is it really true that the registers don't latch the time stamps,
as Rafal said? If so, then we cannot accept the non-descriptor driver
version, since it cannot possibly work correctly.
AFAIK, the two sets of registers only hold the timestamp till the next
event (or peer event) packet comes in.
I understand that it is not accurate - it is an initial version.
Regards,
Harini
From: Richard Cochran <richardcochran@gmail.com> Date: 2017-01-03 10:20:59
On Tue, Jan 03, 2017 at 10:36:11AM +0530, Harini Katakam wrote:
I understand that it is not accurate - it is an initial version.
No, it is not inaccurate at all, it is WRONG.
This means that time stamps will be randomly associated with PTP
network packets. To the application, the protocol will appear to
work, but the time stamp information (and thus the synchronization)
will be wrong.
To me, this is unacceptable, and I will push back on this driver
getting merged.
[ In contrast, the descriptor based approach would be ok, afaict. ]
Thanks,
Richard
From: Richard Cochran <richardcochran@gmail.com> Date: 2017-01-03 10:30:14
On Tue, Jan 03, 2017 at 10:36:11AM +0530, Harini Katakam wrote:
I understand that it is not accurate - it is an initial version.
Why do you say, "it is an initial version?"
The Atmel device has this IP core burned in. The core is hopelessly
broken, and it cannot be fixed in SW either, so what is your point?
Thanks,
Richard
From: Harini Katakam [mailto:harinikatakamlinux@gmail.com]
Sent: 3 stycznia 2017 06:06
Subject: Re: [RFC PATCH net-next v4 1/2] macb: Add 1588 support in Cadence GEM.
Hi Richard,
On Mon, Jan 2, 2017 at 9:43 PM, Richard Cochran [off-list ref] wrote:
quoted
On Mon, Jan 02, 2017 at 03:47:07PM +0100, Nicolas Ferre wrote:
quoted
Le 02/01/2017 à 12:31, Richard Cochran a écrit :
quoted
This Cadence IP core is a complete disaster.
Well, it evolved and propose several options to different SoC
integrators. This is not something unusual...
I suspect as well that some other network adapters have the same
weakness concerning PTP timestamp in single register as the early
revisions of this IP.
It appears that this core can neither latch the time on read or write,
or even latch time stamps. I have worked with many different PTP HW
implementations, even early ones like on the ixp4xx, and it is no
exaggeration to say that this one is uniquely broken.
quoted
I suspect that Rafal tend to jump too quickly to the latest IP
revisions and add more options to this series: let's not try to pour
too much things into this code right now.
Why can't you check the IP version in the driver?
There is an IP revision register but it would be probably be better to rely on "caps" from the compatibility strings - to cover SoC specific implementations. Also, when this extended BD is added (with timestamp), additional words will need to be added statically which will be consistent with Andrei's CONFIG_ checks.
We can distinguish IP cores with and without PTP support by reading Design Configuration Register. But to distinguish IP cores with timestamps in buffer descriptors and which support only event registers, we can only check IP version by reading the revision ID register and base on that.
I agree with Harini, compatibility strings could be better. But we might end up with many different configuration in the future.
We could use only descriptor approach but there are many Atmel's cores on the market which support only event registers.
Hi Richard,
On Tue, Jan 3, 2017 at 3:59 PM, Richard Cochran
[off-list ref] wrote:
On Tue, Jan 03, 2017 at 10:36:11AM +0530, Harini Katakam wrote:
quoted
I understand that it is not accurate - it is an initial version.
Why do you say, "it is an initial version?"
The Atmel device has this IP core burned in. The core is hopelessly
broken, and it cannot be fixed in SW either, so what is your point?
I'm sorry - I just meant that this was before many necessary
enhancements and fixes.
Newer SoCs including ZynqMP (for which the original series was sent)
have the descriptor based approach which is reliable.
Regards,
Harini
From: Nicolas Ferre <hidden> Date: 2017-01-03 14:22:22
Le 03/01/2017 à 11:47, Rafal Ozieblo a écrit :
quoted
From: Harini Katakam [mailto:harinikatakamlinux@gmail.com]
Sent: 3 stycznia 2017 06:06
Subject: Re: [RFC PATCH net-next v4 1/2] macb: Add 1588 support in Cadence GEM.
Hi Richard,
On Mon, Jan 2, 2017 at 9:43 PM, Richard Cochran [off-list ref] wrote:
quoted
On Mon, Jan 02, 2017 at 03:47:07PM +0100, Nicolas Ferre wrote:
quoted
Le 02/01/2017 à 12:31, Richard Cochran a écrit :
quoted
This Cadence IP core is a complete disaster.
Well, it evolved and propose several options to different SoC
integrators. This is not something unusual...
I suspect as well that some other network adapters have the same
weakness concerning PTP timestamp in single register as the early
revisions of this IP.
It appears that this core can neither latch the time on read or write,
or even latch time stamps. I have worked with many different PTP HW
implementations, even early ones like on the ixp4xx, and it is no
exaggeration to say that this one is uniquely broken.
quoted
I suspect that Rafal tend to jump too quickly to the latest IP
revisions and add more options to this series: let's not try to pour
too much things into this code right now.
Why can't you check the IP version in the driver?
There is an IP revision register but it would be probably be
better to rely on "caps" from the compatibility strings - to cover SoC specific
implementations. Also, when this extended BD is added (with timestamp),
additional words will need to be added statically which will be
consistent with Andrei's CONFIG_ checks.
We can distinguish IP cores with and without PTP support by reading
Design Configuration Register. But to distinguish IP cores with
timestamps in buffer descriptors and which support only event
registers, we can only check IP version by reading the revision ID
register and base on that.
I agree with Harini, compatibility strings could be better. But we
might end up with many different configuration in the future.
Compatibility strings and associated configurations are cheap. It's not
a problem to have many different configurations and clearer for this
particular "composite" feature.
We could use only descriptor approach but there are many Atmel's
cores on the market which support only event registers.
Yes and once in silicon, it's hard to modify ;-)
Regards,
--
Nicolas Ferre
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Tue, Jan 03, 2017 at 10:47:56AM +0000, Rafal Ozieblo wrote:
quoted
We could use only descriptor approach but there are many Atmel's cores
on the market which support only event registers.
As I said in my other reply in this thread, the Atmel cores cannot possibly be
made to work correctly.
Sad, but true.
In conscience, I will make a patch containing only the common code (just to benefit from this effort) and then Rafal can add the driver for the GXL version on top of it.
Everybody agrees with this?
Best regards,
Andrei