| Message ID | 20190604170756.14338-1-olteanv@gmail.com |
|---|---|
| Headers | show |
| Series | PTP support for the SJA1105 DSA driver | expand |
From: Vladimir Oltean <olteanv@gmail.com> Date: Tue, 4 Jun 2019 20:07:39 +0300 > This patchset adds the following: > > - A timecounter/cyclecounter based PHC for the free-running > timestamping clock of this switch. > > - A state machine implemented in the DSA tagger for SJA1105, which > keeps track of metadata follow-up Ethernet frames (the switch's way > of transmitting RX timestamps). This series doesn't apply cleanly to net-next, please respin. Thank you.
On Wed, 5 Jun 2019 at 06:23, David Miller <davem@davemloft.net> wrote: > > From: Vladimir Oltean <olteanv@gmail.com> > Date: Tue, 4 Jun 2019 20:07:39 +0300 > > > This patchset adds the following: > > > > - A timecounter/cyclecounter based PHC for the free-running > > timestamping clock of this switch. > > > > - A state machine implemented in the DSA tagger for SJA1105, which > > keeps track of metadata follow-up Ethernet frames (the switch's way > > of transmitting RX timestamps). > > This series doesn't apply cleanly to net-next, please respin. > > Thank you. Hi Dave, It is conflicting because net-next at the moment lacks this patch that I submitted to net: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git/commit/?id=e8d67fa5696e2fcaf956dae36d11e6eff5246101 What would you like me to do: resubmit after you merge net into net-next, add the above patch to this series (which you'll have to skip upon the next merge), or you can just cherry-pick it and then the series will apply? Thanks! -Vladimir
On Wed, 5 Jun 2019 at 12:13, Vladimir Oltean <olteanv@gmail.com> wrote: > > On Wed, 5 Jun 2019 at 06:23, David Miller <davem@davemloft.net> wrote: > > > > From: Vladimir Oltean <olteanv@gmail.com> > > Date: Tue, 4 Jun 2019 20:07:39 +0300 > > > > > This patchset adds the following: > > > > > > - A timecounter/cyclecounter based PHC for the free-running > > > timestamping clock of this switch. > > > > > > - A state machine implemented in the DSA tagger for SJA1105, which > > > keeps track of metadata follow-up Ethernet frames (the switch's way > > > of transmitting RX timestamps). > > > > This series doesn't apply cleanly to net-next, please respin. > > > > Thank you. > > Hi Dave, > > It is conflicting because net-next at the moment lacks this patch that > I submitted to net: > https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git/commit/?id=e8d67fa5696e2fcaf956dae36d11e6eff5246101 > What would you like me to do: resubmit after you merge net into > net-next, add the above patch to this series (which you'll have to > skip upon the next merge), or you can just cherry-pick it and then the > series will apply? > > Thanks! > -Vladimir In the meantime: Richard, do you have any objections to this patchset? I was wondering whether the path delay difference between E2E and P2P rings any bell to you. Thank you, -Vladimir
On Wed, Jun 05, 2019 at 02:33:52PM +0300, Vladimir Oltean wrote: > In the meantime: Richard, do you have any objections to this patchset? I like the fact that you didn't have to change the dsa or ptp frameworks this time around. I haven't taken a closer look than that yet. > I was wondering whether the path delay difference between E2E and P2P > rings any bell to you. Can it be that the switch applies corrections in HW? Thanks, Richard
On Wed, 5 Jun 2019 at 20:45, Richard Cochran <richardcochran@gmail.com> wrote: > > On Wed, Jun 05, 2019 at 02:33:52PM +0300, Vladimir Oltean wrote: > > In the meantime: Richard, do you have any objections to this patchset? > > I like the fact that you didn't have to change the dsa or ptp > frameworks this time around. I haven't taken a closer look than that > yet. > > > I was wondering whether the path delay difference between E2E and P2P > > rings any bell to you. > > Can it be that the switch applies corrections in HW? > Yes it can be. It was one of the first things I thought of. Normally it updates the correction field with its own residence time in 1-step L2 event messages (but I use 2 step). It also has a bit called IGNORE2STF (ignore 2-step flag) by which it updates the correction field in all L2 event messages (including sync, thereby violating the spec for a switch, as far as I'm aware). But I'm not setting it. I also looked at egress frames with wireshark and the correction field is zero. > Thanks, > Richard Thanks, -Vladimir
On Wed, 5 Jun 2019 at 20:50, Vladimir Oltean <olteanv@gmail.com> wrote: > > On Wed, 5 Jun 2019 at 20:45, Richard Cochran <richardcochran@gmail.com> wrote: > > > > On Wed, Jun 05, 2019 at 02:33:52PM +0300, Vladimir Oltean wrote: > > > In the meantime: Richard, do you have any objections to this patchset? > > > > I like the fact that you didn't have to change the dsa or ptp > > frameworks this time around. I haven't taken a closer look than that > > yet. > > > > > I was wondering whether the path delay difference between E2E and P2P > > > rings any bell to you. > > > > Can it be that the switch applies corrections in HW? > > > > Yes it can be. It was one of the first things I thought of. > Normally it updates the correction field with its own residence time > in 1-step L2 event messages (but I use 2 step). > It also has a bit called IGNORE2STF (ignore 2-step flag) by which it > updates the correction field in all L2 event messages (including sync, > thereby violating the spec for a switch, as far as I'm aware). But I'm > not setting it. > I also looked at egress frames with wireshark and the correction field is zero. > I also changed around the values of ptp_dst_mac and p2p_dst_mac in linuxptp in the hope that I'd throw off whatever hardware parser it has to identify the event frames, but I still get negative path delay with E2E nonetheless. So it's probably not that. > > Thanks, > > Richard > > Thanks, > -Vladimir -Vladimir
On Wed, 5 Jun 2019 at 20:45, Richard Cochran <richardcochran@gmail.com> wrote: > > On Wed, Jun 05, 2019 at 02:33:52PM +0300, Vladimir Oltean wrote: > > In the meantime: Richard, do you have any objections to this patchset? > > I like the fact that you didn't have to change the dsa or ptp > frameworks this time around. I haven't taken a closer look than that > yet. > There's one more thing I wanted to ask you if you get the chance to take a closer look. Currently I'm using a cyclecounter, but I *will* need actual PHC manipulations for the time-based shaping and policing features that the switch has in hardware. On the other hand I get much tighter sync offset using the free-running counter than with hardware-corrected timestamps. So as far as I see it, I'll need to have two sets of operations. How should I design such a dual-PHC device driver? Just register two separate clocks, one for the timestamping counter, the other for the scheduling/policing PTP clock, and have phc2sys keep them in sync externally to the driver? Or implement the hardware corrections alongside the timecounter ones, and expose a single PHC (and for clock_gettime, just pick one of the time sources)? > > I was wondering whether the path delay difference between E2E and P2P > > rings any bell to you. > > Can it be that the switch applies corrections in HW? > > Thanks, > Richard Thanks, -Vladimir
From: Vladimir Oltean <olteanv@gmail.com> Date: Wed, 5 Jun 2019 12:13:59 +0300 > On Wed, 5 Jun 2019 at 06:23, David Miller <davem@davemloft.net> wrote: >> >> From: Vladimir Oltean <olteanv@gmail.com> >> Date: Tue, 4 Jun 2019 20:07:39 +0300 >> >> > This patchset adds the following: >> > >> > - A timecounter/cyclecounter based PHC for the free-running >> > timestamping clock of this switch. >> > >> > - A state machine implemented in the DSA tagger for SJA1105, which >> > keeps track of metadata follow-up Ethernet frames (the switch's way >> > of transmitting RX timestamps). >> >> This series doesn't apply cleanly to net-next, please respin. >> >> Thank you. > > Hi Dave, > > It is conflicting because net-next at the moment lacks this patch that > I submitted to net: > https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git/commit/?id=e8d67fa5696e2fcaf956dae36d11e6eff5246101 > What would you like me to do: resubmit after you merge net into > net-next, add the above patch to this series (which you'll have to > skip upon the next merge), or you can just cherry-pick it and then the > series will apply? So let me bring this series back to state "Under Review" and I'll apply it after I next merge net into net-next. Thanks for letting me know.
On Wed, Jun 05, 2019 at 09:08:54PM +0300, Vladimir Oltean wrote: > Currently I'm using a cyclecounter, but I *will* need actual PHC > manipulations for the time-based shaping and policing features that > the switch has in hardware. Okay. > On the other hand I get much tighter sync > offset using the free-running counter than with hardware-corrected > timestamps. Why? The time stamps come from the very same counter, don't they? > So as far as I see it, I'll need to have two sets of > operations. I doubt very much that this will work well. > How should I design such a dual-PHC device driver? Just register two > separate clocks, one for the timestamping counter, the other for the > scheduling/policing PTP clock, and have phc2sys keep them in sync > externally to the driver? But how would phc2sys do this? By comparing clock_gettime() values? That would surely introduce unnecessary time error. > Or implement the hardware corrections > alongside the timecounter ones, and expose a single PHC (and for > clock_gettime, just pick one of the time sources)? I would implement the hardware clock and drop the timecounter altogether. HTH, Richard
On Thu, 6 Jun 2019 at 06:11, Richard Cochran <richardcochran@gmail.com> wrote: > > On Wed, Jun 05, 2019 at 09:08:54PM +0300, Vladimir Oltean wrote: > > Currently I'm using a cyclecounter, but I *will* need actual PHC > > manipulations for the time-based shaping and policing features that > > the switch has in hardware. > > Okay. > > > On the other hand I get much tighter sync > > offset using the free-running counter than with hardware-corrected > > timestamps. > > Why? The time stamps come from the very same counter, don't they? > Plain and simply because it doesn't work very well. Even phc2sys from the system clock to the hardware (no timestamps involved) has trouble staying put (under 1000 ns offset). And using the hardware-corrected timestamps triggers a lot of clockchecks. > > So as far as I see it, I'll need to have two sets of > > operations. > > I doubt very much that this will work well. > > > How should I design such a dual-PHC device driver? Just register two > > separate clocks, one for the timestamping counter, the other for the > > scheduling/policing PTP clock, and have phc2sys keep them in sync > > externally to the driver? > > But how would phc2sys do this? By comparing clock_gettime() values? > That would surely introduce unnecessary time error. > > > Or implement the hardware corrections > > alongside the timecounter ones, and expose a single PHC (and for > > clock_gettime, just pick one of the time sources)? > > I would implement the hardware clock and drop the timecounter > altogether. > > HTH, > Richard
On Thu, Jun 06, 2019 at 04:40:19PM +0300, Vladimir Oltean wrote: > Plain and simply because it doesn't work very well. > Even phc2sys from the system clock to the hardware (no timestamps > involved) has trouble staying put (under 1000 ns offset). > And using the hardware-corrected timestamps triggers a lot of clockchecks. It sounds like a bug in reading or adjusting the HW clock. Is the HW clock stable when you don't adjust its frequency? Thanks, Richard
On Fri, 7 Jun 2019 at 06:32, Richard Cochran <richardcochran@gmail.com> wrote: > > On Thu, Jun 06, 2019 at 04:40:19PM +0300, Vladimir Oltean wrote: > > Plain and simply because it doesn't work very well. > > Even phc2sys from the system clock to the hardware (no timestamps > > involved) has trouble staying put (under 1000 ns offset). > > And using the hardware-corrected timestamps triggers a lot of clockchecks. > > It sounds like a bug in reading or adjusting the HW clock. Is the HW > clock stable when you don't adjust its frequency? How can I tell that for sure? > > Thanks, > Richard
From: David Miller <davem@davemloft.net> Date: Wed, 05 Jun 2019 11:44:29 -0700 (PDT) > From: Vladimir Oltean <olteanv@gmail.com> > Date: Wed, 5 Jun 2019 12:13:59 +0300 > >> It is conflicting because net-next at the moment lacks this patch that >> I submitted to net: >> https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git/commit/?id=e8d67fa5696e2fcaf956dae36d11e6eff5246101 >> What would you like me to do: resubmit after you merge net into >> net-next, add the above patch to this series (which you'll have to >> skip upon the next merge), or you can just cherry-pick it and then the >> series will apply? > > So let me bring this series back to state "Under Review" and I'll apply it > after I next merge net into net-next. So I applied the series but it doesn't even build: ERROR: "sja1105_unpack" [drivers/net/dsa/sja1105/sja1105_ptp.ko] undefined! ERROR: "sja1105_spi_send_packed_buf" [drivers/net/dsa/sja1105/sja1105_ptp.ko] undefined! ERROR: "sja1105_pack" [drivers/net/dsa/sja1105/sja1105_ptp.ko] undefined! ERROR: "sja1105_spi_send_int" [drivers/net/dsa/sja1105/sja1105_ptp.ko] undefined! ERROR: "sja1105_get_ts_info" [drivers/net/dsa/sja1105/sja1105.ko] undefined! ERROR: "sja1105pqrs_ptp_cmd" [drivers/net/dsa/sja1105/sja1105.ko] undefined! ERROR: "sja1105_ptp_clock_unregister" [drivers/net/dsa/sja1105/sja1105.ko] undefined! ERROR: "sja1105_ptpegr_ts_poll" [drivers/net/dsa/sja1105/sja1105.ko] undefined! ERROR: "sja1105et_ptp_cmd" [drivers/net/dsa/sja1105/sja1105.ko] undefined! ERROR: "sja1105_ptp_reset" [drivers/net/dsa/sja1105/sja1105.ko] undefined! ERROR: "sja1105_tstamp_reconstruct" [drivers/net/dsa/sja1105/sja1105.ko] undefined! ERROR: "sja1105_ptp_clock_register" [drivers/net/dsa/sja1105/sja1105.ko] undefined! You have to test better with the various modular/non-modular combinations. Thanks.
On Fri, 7 Jun 2019 at 22:15, David Miller <davem@davemloft.net> wrote: > > From: David Miller <davem@davemloft.net> > Date: Wed, 05 Jun 2019 11:44:29 -0700 (PDT) > > > From: Vladimir Oltean <olteanv@gmail.com> > > Date: Wed, 5 Jun 2019 12:13:59 +0300 > > > >> It is conflicting because net-next at the moment lacks this patch that > >> I submitted to net: > >> https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git/commit/?id=e8d67fa5696e2fcaf956dae36d11e6eff5246101 > >> What would you like me to do: resubmit after you merge net into > >> net-next, add the above patch to this series (which you'll have to > >> skip upon the next merge), or you can just cherry-pick it and then the > >> series will apply? > > > > So let me bring this series back to state "Under Review" and I'll apply it > > after I next merge net into net-next. > > So I applied the series but it doesn't even build: > > ERROR: "sja1105_unpack" [drivers/net/dsa/sja1105/sja1105_ptp.ko] undefined! > ERROR: "sja1105_spi_send_packed_buf" [drivers/net/dsa/sja1105/sja1105_ptp.ko] undefined! > ERROR: "sja1105_pack" [drivers/net/dsa/sja1105/sja1105_ptp.ko] undefined! > ERROR: "sja1105_spi_send_int" [drivers/net/dsa/sja1105/sja1105_ptp.ko] undefined! > ERROR: "sja1105_get_ts_info" [drivers/net/dsa/sja1105/sja1105.ko] undefined! > ERROR: "sja1105pqrs_ptp_cmd" [drivers/net/dsa/sja1105/sja1105.ko] undefined! > ERROR: "sja1105_ptp_clock_unregister" [drivers/net/dsa/sja1105/sja1105.ko] undefined! > ERROR: "sja1105_ptpegr_ts_poll" [drivers/net/dsa/sja1105/sja1105.ko] undefined! > ERROR: "sja1105et_ptp_cmd" [drivers/net/dsa/sja1105/sja1105.ko] undefined! > ERROR: "sja1105_ptp_reset" [drivers/net/dsa/sja1105/sja1105.ko] undefined! > ERROR: "sja1105_tstamp_reconstruct" [drivers/net/dsa/sja1105/sja1105.ko] undefined! > ERROR: "sja1105_ptp_clock_register" [drivers/net/dsa/sja1105/sja1105.ko] undefined! > > You have to test better with the various modular/non-modular combinations. > > Thanks. Ok, my bad, I'll resubmit it tomorrow. Thanks! -Vladimir
This patchset adds the following: - A timecounter/cyclecounter based PHC for the free-running timestamping clock of this switch. - A state machine implemented in the DSA tagger for SJA1105, which keeps track of metadata follow-up Ethernet frames (the switch's way of transmitting RX timestamps). Clock manipulations on the actual hardware PTP clock will have to be implemented anyway, for the TTEthernet block and the time-based ingress policer. This depends upon the "FDB updates for SJA1105 DSA driver" series at: https://patchwork.ozlabs.org/project/netdev/list/?series=111354&state=* v2 patchset can be found at: https://lkml.org/lkml/2019/6/2/146 Changes from v2: - Broke previous 09/10 patch (timestamping) into multiple smaller patches. - Every patch in the series compiles. v1 patchset can be found at: https://lkml.org/lkml/2019/5/28/1093 Changes from v1: - Removed the addition of the DSA .can_timestamp callback. - Waiting for meta frames is done completely inside the tagger, and all frames emitted on RX are already partially timestamped. - Added a global data structure for the tagger common to all ports. - Made PTP work with ports in standalone mode, by limiting use of the DMAC-mangling "incl_srcpt" mode only when ports are bridged, aka when the DSA master is already promiscuous and can receive anything. Also changed meta frames to be sent at the 01-80-C2-00-00-0E DMAC. - Made some progress w.r.t. observed negative path delay. Apparently it only appears when the delay mechanism is the delay request-response (end-to-end) one. If peer delay is used (-P), the path delay is positive and appears reasonable for an 1000Base-T link (485 ns in steady state). SJA1105 as PTP slave (OC) with E2E path delay: ptp4l[55.600]: master offset 8 s2 freq +83677 path delay -2390 ptp4l[56.600]: master offset 17 s2 freq +83688 path delay -2391 ptp4l[57.601]: master offset 6 s2 freq +83682 path delay -2391 ptp4l[58.601]: master offset -1 s2 freq +83677 path delay -2391 SJA1105 as PTP slave (OC) with P2P path delay: ptp4l[48.343]: master offset 5 s2 freq +83715 path delay 484 ptp4l[48.468]: master offset -3 s2 freq +83705 path delay 485 ptp4l[48.593]: master offset 0 s2 freq +83708 path delay 485 ptp4l[48.718]: master offset 1 s2 freq +83710 path delay 485 ptp4l[48.844]: master offset 1 s2 freq +83710 path delay 485 ptp4l[48.969]: master offset -5 s2 freq +83702 path delay 485 ptp4l[49.094]: master offset 3 s2 freq +83712 path delay 485 ptp4l[49.219]: master offset 4 s2 freq +83714 path delay 485 ptp4l[49.344]: master offset -5 s2 freq +83702 path delay 485 ptp4l[49.469]: master offset 3 s2 freq +83713 path delay 487 Vladimir Oltean (17): net: dsa: Keep a pointer to the skb clone for TX timestamping net: dsa: Add teardown callback for drivers net: dsa: tag_8021q: Create helper function for removing VLAN header net: dsa: sja1105: Move sja1105_change_tpid into sja1105_vlan_filtering net: dsa: sja1105: Reverse TPID and TPID2 net: dsa: sja1105: Limit use of incl_srcpt to bridge+vlan mode net: dsa: sja1105: Add support for the PTP clock net: dsa: sja1105: Move sja1105_is_link_local to include/linux net: dsa: sja1105: Add logic for TX timestamping net: dsa: sja1105: Build a minimal understanding of meta frames net: dsa: sja1105: Add support for the AVB Parameters Table net: dsa: sja1105: Make sja1105_is_link_local not match meta frames net: dsa: sja1105: Receive and decode meta frames net: dsa: sja1105: Add a global sja1105_tagger_data structure net: dsa: sja1105: Increase priority of CPU-trapped frames net: dsa: sja1105: Add a state machine for RX timestamping net: dsa: sja1105: Expose PTP timestamping ioctls to userspace drivers/net/dsa/sja1105/Kconfig | 7 + drivers/net/dsa/sja1105/Makefile | 1 + drivers/net/dsa/sja1105/sja1105.h | 29 ++ .../net/dsa/sja1105/sja1105_dynamic_config.c | 2 + drivers/net/dsa/sja1105/sja1105_main.c | 319 ++++++++++++-- drivers/net/dsa/sja1105/sja1105_ptp.c | 392 ++++++++++++++++++ drivers/net/dsa/sja1105/sja1105_ptp.h | 64 +++ drivers/net/dsa/sja1105/sja1105_spi.c | 33 ++ .../net/dsa/sja1105/sja1105_static_config.c | 59 +++ .../net/dsa/sja1105/sja1105_static_config.h | 10 + include/linux/dsa/8021q.h | 7 + include/linux/dsa/sja1105.h | 51 +++ include/net/dsa.h | 1 + net/dsa/dsa2.c | 3 + net/dsa/slave.c | 3 + net/dsa/tag_8021q.c | 15 + net/dsa/tag_sja1105.c | 203 ++++++++- 17 files changed, 1152 insertions(+), 47 deletions(-) create mode 100644 drivers/net/dsa/sja1105/sja1105_ptp.c create mode 100644 drivers/net/dsa/sja1105/sja1105_ptp.h