Re: [RFC PATCH net-next 03/11] spi: Add a PTP system timestamp to the transfer structure
On Tue, Aug 20, 2019 at 04:48:39PM +0300, Vladimir Oltean wrote: > On Tue, 20 Aug 2019 at 15:55, Mark Brown wrote: > > On Fri, Aug 16, 2019 at 05:05:53PM +0300, Vladimir Oltean wrote: > > > Maybe the SPI master driver should just report what sort of > > > snapshotting capability it can offer, ranging from none (default > > > unless otherwise specified), to transfer-level (DMA style) or > > > byte-level. > > That does then have the consequence that the majority of controllers > > aren't going to be usable with the API which isn't great. > Can we continue this discussion on this thread: > https://www.spinics.net/lists/netdev/msg593772.html > The whole point there is that if there's nothing that the driver can > do, the SPI core will take the timestamps and record their (bad) > precision. I'm not on that thread. > > I'm not 100% clear what the problem you're trying to solve is, or if > > it's a sensible problem to try to solve for that matter. > The problem can simply be summarized as: you're trying to read a clock > over SPI, but there's so much timing jitter in you doing that, that > you have a high degree of uncertainty in the actual precision of the > readout you took. That doesn't seem like a great idea... signature.asc Description: PGP signature
Re: [RFC PATCH net-next 03/11] spi: Add a PTP system timestamp to the transfer structure
Hi Mark, On Tue, 20 Aug 2019 at 15:55, Mark Brown wrote: > > On Fri, Aug 16, 2019 at 05:05:53PM +0300, Vladimir Oltean wrote: > > > I'm not sure how to respond to this, because I don't know anything > > about the timing of DMA transfers. > > Maybe snapshotting DMA transfers the same way is not possible (if at > > all). Maybe they are not exactly adequate for this sort of application > > anyway. Maybe it depends. > > DMA transfers generally proceed without any involvement from the CPU, > this is broadly the point of DMA. You *may* be able to split into > multiple transactions but it's not reliable that you'd be able to do so > on byte boundaries and there will be latency getting notified of > completions. > > > In other words, from a purely performance perspective, I am against > > limiting the API to just snapshotting the first and last byte. At this > > level of "zoom", if I change the offset of the byte to anything other > > than 3, the synchronization offset refuses to converge towards zero, > > because the snapshot is incurring a constant offset that the servo > > loop from userspace (phc2sys) can't compensate for. > > > Maybe the SPI master driver should just report what sort of > > snapshotting capability it can offer, ranging from none (default > > unless otherwise specified), to transfer-level (DMA style) or > > byte-level. > > That does then have the consequence that the majority of controllers > aren't going to be usable with the API which isn't great. > Can we continue this discussion on this thread: https://www.spinics.net/lists/netdev/msg593772.html The whole point there is that if there's nothing that the driver can do, the SPI core will take the timestamps and record their (bad) precision. > > I'm afraid more actual experimentation is needed with DMA-based > > controllers to understand what can be expected from them, and as a > > result, how the API should map around them. > > MDIO bus controllers are in a similar situation (with Hubert's patch) > > but at least there the frame size is fixed and I haven't heard of an > > MDIO controller to use DMA. > > I'm not 100% clear what the problem you're trying to solve is, or if > it's a sensible problem to try to solve for that matter. The problem can simply be summarized as: you're trying to read a clock over SPI, but there's so much timing jitter in you doing that, that you have a high degree of uncertainty in the actual precision of the readout you took. The solution has two parts: - Make the SPI access itself more predictable in terms of latency. This is always going to have to be dealt with on a driver-by-driver, hardware-by-hardware basis. - Provide a way of taking a software timestamp in the time interval when the latency is predictable, and as close as possible to the moment when the SPI slave will receive the request. Disabling interrupts and preemption always helps to snapshot that critical section. Again, the SPI core can't do that. And finding the correct "pre" and "post" hooks that surround the hardware transfer in a deterministic fashion is crucial. If you read the cover letter, I used a GPIO pin to make sure the timestamps are where they should be, and that they don't vary in width (post - pre) - there are also some screenshots on Gdrive. Maybe something similar is not impossible for a DMA transfer, although the problem formulation so far is too vague to emit a more clear statement. If you know when the SPI slave's clock was actually read, you have a better idea of what time it was. Regards, -Vladimir
Re: [RFC PATCH net-next 03/11] spi: Add a PTP system timestamp to the transfer structure
On Fri, Aug 16, 2019 at 05:05:53PM +0300, Vladimir Oltean wrote: > I'm not sure how to respond to this, because I don't know anything > about the timing of DMA transfers. > Maybe snapshotting DMA transfers the same way is not possible (if at > all). Maybe they are not exactly adequate for this sort of application > anyway. Maybe it depends. DMA transfers generally proceed without any involvement from the CPU, this is broadly the point of DMA. You *may* be able to split into multiple transactions but it's not reliable that you'd be able to do so on byte boundaries and there will be latency getting notified of completions. > In other words, from a purely performance perspective, I am against > limiting the API to just snapshotting the first and last byte. At this > level of "zoom", if I change the offset of the byte to anything other > than 3, the synchronization offset refuses to converge towards zero, > because the snapshot is incurring a constant offset that the servo > loop from userspace (phc2sys) can't compensate for. > Maybe the SPI master driver should just report what sort of > snapshotting capability it can offer, ranging from none (default > unless otherwise specified), to transfer-level (DMA style) or > byte-level. That does then have the consequence that the majority of controllers aren't going to be usable with the API which isn't great. > I'm afraid more actual experimentation is needed with DMA-based > controllers to understand what can be expected from them, and as a > result, how the API should map around them. > MDIO bus controllers are in a similar situation (with Hubert's patch) > but at least there the frame size is fixed and I haven't heard of an > MDIO controller to use DMA. I'm not 100% clear what the problem you're trying to solve is, or if it's a sensible problem to try to solve for that matter. signature.asc Description: PGP signature
Re: [RFC PATCH net-next 03/11] spi: Add a PTP system timestamp to the transfer structure
> MDIO bus controllers are in a similar situation (with Hubert's patch) > but at least there the frame size is fixed and I haven't heard of an > MDIO controller to use DMA. Linux does not have any DMA driver MDIO busses, as far as i know. It does not make sense, since you are only transferring 16bits of data. The vast majority are polled completion, but there is one which generates an interrupt on completion. Andrew
Re: [RFC PATCH net-next 03/11] spi: Add a PTP system timestamp to the transfer structure
On Fri, 16 Aug 2019 at 15:58, Mark Brown wrote: > > On Fri, Aug 16, 2019 at 03:35:30PM +0300, Vladimir Oltean wrote: > > On Fri, 16 Aug 2019 at 15:18, Mark Brown wrote: > > > On Fri, Aug 16, 2019 at 03:44:41AM +0300, Vladimir Oltean wrote: > > > > > @@ -842,6 +843,9 @@ struct spi_transfer { > > > > > > > > u32 effective_speed_hz; > > > > > > > > + struct ptp_system_timestamp *ptp_sts; > > > > + unsigned intptp_sts_word_offset; > > > > + > > > > You've not documented these fields at all so it's not clear what the > > > intended usage is. > > > Thanks for looking into this. > > Indeed I didn't document them as the patch is part of a RFC and I > > thought the purpose was more clear from the context (cover letter > > etc). > > If I do ever send a patchset for submission I will document the newly > > introduced fields properly. > > The issue I'm having is that I have zero idea about the PTP API so I've > got nothing to go on when thinking about if this approach makes any > sense unless I go do some research. > > > So let me clarify: > > The SPI slave device driver is populating these fields to indicate to > > the controller driver that it wants word number @ptp_sts_word_offset > > from the tx buffer snapshotted. The controller driver is supposed to > > put the snapshot into the @ptp_sts field, which is a pointer to a > > memory location under the control of the SPI slave device driver. > > Snapshot here basically meaning recording a timestamp? This interface > does seem like it basically precludes DMA based controllers from using > it unless someone happened to implement some very specific stuff in > hardware which seems implausible. I'd be inclined to just require that > users can only snapshot the first (and possibly also the last, though > DMA completions make that fun) word of a transfer, we could then pull > this out into the core a bit by providing a wrapper function drivers > should call at the appropriate moment. > I'm not sure how to respond to this, because I don't know anything about the timing of DMA transfers. Maybe snapshotting DMA transfers the same way is not possible (if at all). Maybe they are not exactly adequate for this sort of application anyway. Maybe it depends. But the switch I'm working on is issuing an internal read transaction of the PTP timer exactly at the 4th-to-last bit of the 3rd byte. This is so that it has time (4 SPI clock cycles, to be precise) for the result of the read transaction to become available again to the SPI block, for output. It is impossible to know exactly when the switch will snapshot the time internally (because there are several clock domain crossings from the SPI interface towards its core) but for certain it takes place during the latter part of the 3rd SPI byte. I believe other devices are similar in this regard. In other words, from a purely performance perspective, I am against limiting the API to just snapshotting the first and last byte. At this level of "zoom", if I change the offset of the byte to anything other than 3, the synchronization offset refuses to converge towards zero, because the snapshot is incurring a constant offset that the servo loop from userspace (phc2sys) can't compensate for. Maybe the SPI master driver should just report what sort of snapshotting capability it can offer, ranging from none (default unless otherwise specified), to transfer-level (DMA style) or byte-level. I'm afraid more actual experimentation is needed with DMA-based controllers to understand what can be expected from them, and as a result, how the API should map around them. MDIO bus controllers are in a similar situation (with Hubert's patch) but at least there the frame size is fixed and I haven't heard of an MDIO controller to use DMA. I'm not really sure what the next step would be. In the other thread, Richard Cochran mentioned something about a two-part write API, although I didn't quite understand the idea behind it. > > It is ok if the ptp_sts pointer is NULL (no need to check), because > > the API for taking snapshots already checks for that. > > At the moment there is yet no proposed mechanism for the SPI slave > > driver to ensure that the controller will really act upon this > > request. That would be really nice to have, since some SPI slave > > devices are time-sensitive and warning early is a good way to prevent > > unnecessary troubleshooting. > > Yes, that's one of the things I was thinking about looking at the series > - we should at least be able to warn if we can't timestamp so nobody > gets confused, possibly error out if the calling code particularly > depends on it. Regards, -Vladimir
Re: [RFC PATCH net-next 03/11] spi: Add a PTP system timestamp to the transfer structure
On Fri, Aug 16, 2019 at 03:35:30PM +0300, Vladimir Oltean wrote: > On Fri, 16 Aug 2019 at 15:18, Mark Brown wrote: > > On Fri, Aug 16, 2019 at 03:44:41AM +0300, Vladimir Oltean wrote: > > > @@ -842,6 +843,9 @@ struct spi_transfer { > > > > > > u32 effective_speed_hz; > > > > > > + struct ptp_system_timestamp *ptp_sts; > > > + unsigned intptp_sts_word_offset; > > > + > > You've not documented these fields at all so it's not clear what the > > intended usage is. > Thanks for looking into this. > Indeed I didn't document them as the patch is part of a RFC and I > thought the purpose was more clear from the context (cover letter > etc). > If I do ever send a patchset for submission I will document the newly > introduced fields properly. The issue I'm having is that I have zero idea about the PTP API so I've got nothing to go on when thinking about if this approach makes any sense unless I go do some research. > So let me clarify: > The SPI slave device driver is populating these fields to indicate to > the controller driver that it wants word number @ptp_sts_word_offset > from the tx buffer snapshotted. The controller driver is supposed to > put the snapshot into the @ptp_sts field, which is a pointer to a > memory location under the control of the SPI slave device driver. Snapshot here basically meaning recording a timestamp? This interface does seem like it basically precludes DMA based controllers from using it unless someone happened to implement some very specific stuff in hardware which seems implausible. I'd be inclined to just require that users can only snapshot the first (and possibly also the last, though DMA completions make that fun) word of a transfer, we could then pull this out into the core a bit by providing a wrapper function drivers should call at the appropriate moment. > It is ok if the ptp_sts pointer is NULL (no need to check), because > the API for taking snapshots already checks for that. > At the moment there is yet no proposed mechanism for the SPI slave > driver to ensure that the controller will really act upon this > request. That would be really nice to have, since some SPI slave > devices are time-sensitive and warning early is a good way to prevent > unnecessary troubleshooting. Yes, that's one of the things I was thinking about looking at the series - we should at least be able to warn if we can't timestamp so nobody gets confused, possibly error out if the calling code particularly depends on it. signature.asc Description: PGP signature
Re: [RFC PATCH net-next 03/11] spi: Add a PTP system timestamp to the transfer structure
Hi Mark, On Fri, 16 Aug 2019 at 15:18, Mark Brown wrote: > > On Fri, Aug 16, 2019 at 03:44:41AM +0300, Vladimir Oltean wrote: > > > @@ -842,6 +843,9 @@ struct spi_transfer { > > > > u32 effective_speed_hz; > > > > + struct ptp_system_timestamp *ptp_sts; > > + unsigned intptp_sts_word_offset; > > + > > You've not documented these fields at all so it's not clear what the > intended usage is. Thanks for looking into this. Indeed I didn't document them as the patch is part of a RFC and I thought the purpose was more clear from the context (cover letter etc). If I do ever send a patchset for submission I will document the newly introduced fields properly. So let me clarify: The SPI slave device driver is populating these fields to indicate to the controller driver that it wants word number @ptp_sts_word_offset from the tx buffer snapshotted. The controller driver is supposed to put the snapshot into the @ptp_sts field, which is a pointer to a memory location under the control of the SPI slave device driver. It is ok if the ptp_sts pointer is NULL (no need to check), because the API for taking snapshots already checks for that. At the moment there is yet no proposed mechanism for the SPI slave driver to ensure that the controller will really act upon this request. That would be really nice to have, since some SPI slave devices are time-sensitive and warning early is a good way to prevent unnecessary troubleshooting. Regards, -Vladimir
Re: [RFC PATCH net-next 03/11] spi: Add a PTP system timestamp to the transfer structure
On Fri, Aug 16, 2019 at 03:44:41AM +0300, Vladimir Oltean wrote: > @@ -842,6 +843,9 @@ struct spi_transfer { > > u32 effective_speed_hz; > > + struct ptp_system_timestamp *ptp_sts; > + unsigned intptp_sts_word_offset; > + You've not documented these fields at all so it's not clear what the intended usage is. signature.asc Description: PGP signature