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