Re: [PATCH] vdso: Fix powerpc build U64_MAX undeclared error
On Mon, Apr 8, 2024 at 11:27 PM Adrian Hunter wrote:
>
> U64_MAX is not in include/vdso/limits.h, although that isn't noticed on x86
> because x86 includes include/linux/limits.h indirectly. However powerpc
> is more selective, resulting in the following build error:
>
> In file included from :
> lib/vdso/gettimeofday.c: In function 'vdso_calc_ns':
> lib/vdso/gettimeofday.c:11:33: error: 'U64_MAX' undeclared
> 11 | # define VDSO_DELTA_MASK(vd)U64_MAX
> | ^~~
>
> Use ULLONG_MAX instead which will work just as well and is in
> include/vdso/limits.h.
>
> Reported-by: Stephen Rothwell
> Closes: https://lore.kernel.org/all/20240409124905.6816d...@canb.auug.org.au/
> Fixes: c8e3a8b6f2e6 ("vdso: Consolidate vdso_calc_delta()")
> Signed-off-by: Adrian Hunter
Acked-by: John Stultz
Re: [PATCH 01/19] vdso: Consolidate vdso_calc_delta()
On Fri, Mar 8, 2024 at 5:15 AM Adrian Hunter wrote:
>
> Consolidate vdso_calc_delta(), in preparation for further simplification.
>
> Suggested-by: Thomas Gleixner
> Signed-off-by: Adrian Hunter
> ---
> arch/powerpc/include/asm/vdso/gettimeofday.h | 17 ++---
> arch/s390/include/asm/vdso/gettimeofday.h| 7 ++-
> lib/vdso/gettimeofday.c | 4
> 3 files changed, 8 insertions(+), 20 deletions(-)
>
> --- a/lib/vdso/gettimeofday.c
> +++ b/lib/vdso/gettimeofday.c
> @@ -13,7 +13,11 @@
> static __always_inline
> u64 vdso_calc_delta(u64 cycles, u64 last, u64 mask, u32 mult)
> {
> +#ifdef VDSO_DELTA_NOMASK
> + return (cycles - last) * mult;
> +#else
> return ((cycles - last) & mask) * mult;
> +#endif
> }
Nit: Just for readability, usually we avoid #ifdefs inside of functions.
Instead maybe:
#ifdef VDSO_DELTA_NOMASK
static __always_inline
u64 vdso_calc_delta(u64 cycles, u64 last, u64 mask, u32 mult)
{
return (cycles - last) * mult;
}
#else
static __always_inline
u64 vdso_calc_delta(u64 cycles, u64 last, u64 mask, u32 mult)
{
return ((cycles - last) & mask) * mult;
}
#endif
Re: [alsa-devel] [PATCH V6 3/4] ASoC: pcm_dmaengine: Extract snd_dmaengine_pcm_refine_runtime_hwparams
On Thu, Jan 16, 2020 at 11:11 PM Shengjiu Wang wrote:
>
> Hi
>
> On Thu, Jan 16, 2020 at 1:56 PM John Stultz wrote:
> >
> > On Wed, Jan 8, 2020 at 8:58 PM John Stultz wrote:
> > > On Thu, Sep 26, 2019 at 6:50 PM Shengjiu Wang
> > > wrote:
> > > >
> > > > When set the runtime hardware parameters, we may need to query
> > > > the capability of DMA to complete the parameters.
> > > >
> > > > This patch is to Extract this operation from
> > > > dmaengine_pcm_set_runtime_hwparams function to a separate function
> > > > snd_dmaengine_pcm_refine_runtime_hwparams, that other components
> > > > which need this feature can call this function.
> > > >
> > > > Signed-off-by: Shengjiu Wang
> > > > Reviewed-by: Nicolin Chen
> > >
> > > As a heads up, this patch seems to be causing a regression on the HiKey
> > > board.
> > >
> > > On boot up I'm seeing:
> > > [ 17.721424] hi6210_i2s f7118000.i2s: ASoC: can't open component
> > > f7118000.i2s: -6
> > >
> > > And HDMI audio isn't working. With this patch reverted, audio works again.
> > >
> > >
> > > > diff --git a/sound/core/pcm_dmaengine.c b/sound/core/pcm_dmaengine.c
> > > > index 89a05926ac73..5749a8a49784 100644
> > > > --- a/sound/core/pcm_dmaengine.c
> > > > +++ b/sound/core/pcm_dmaengine.c
> > > > @@ -369,4 +369,87 @@ int snd_dmaengine_pcm_close_release_chan(struct
> > > > snd_pcm_substream *substream)
> > > ...
> > > > + ret = dma_get_slave_caps(chan, _caps);
> > > > + if (ret == 0) {
> > > > + if (dma_caps.cmd_pause && dma_caps.cmd_resume)
> > > > + hw->info |= SNDRV_PCM_INFO_PAUSE |
> > > > SNDRV_PCM_INFO_RESUME;
> > > > + if (dma_caps.residue_granularity <=
> > > > DMA_RESIDUE_GRANULARITY_SEGMENT)
> > > > + hw->info |= SNDRV_PCM_INFO_BATCH;
> > > > +
> > > > + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> > > > + addr_widths = dma_caps.dst_addr_widths;
> > > > + else
> > > > + addr_widths = dma_caps.src_addr_widths;
> > > > + }
> > >
> > > It seems a failing ret from dma_get_slave_caps() here is being returned...
> > >
> > > > +
> > > > + /*
> > > > +* If SND_DMAENGINE_PCM_DAI_FLAG_PACK is set keep
> > > > +* hw.formats set to 0, meaning no restrictions are in place.
> > > > +* In this case it's the responsibility of the DAI driver to
> > > > +* provide the supported format information.
> > > > +*/
> > > > + if (!(dma_data->flags & SND_DMAENGINE_PCM_DAI_FLAG_PACK))
> > > > + /*
> > > > +* Prepare formats mask for valid/allowed sample types.
> > > > If the
> > > > +* dma does not have support for the given physical
> > > > word size,
> > > > +* it needs to be masked out so user space can not use
> > > > the
> > > > +* format which produces corrupted audio.
> > > > +* In case the dma driver does not implement the
> > > > slave_caps the
> > > > +* default assumption is that it supports 1, 2 and 4
> > > > bytes
> > > > +* widths.
> > > > +*/
> > > > + for (i = SNDRV_PCM_FORMAT_FIRST; i <=
> > > > SNDRV_PCM_FORMAT_LAST; i++) {
> > > > + int bits = snd_pcm_format_physical_width(i);
> > > > +
> > > > + /*
> > > > +* Enable only samples with DMA supported
> > > > physical
> > > > +* widths
> > > > +*/
> > > > + switch (bits) {
> > > > + case 8:
> > > > + case 16:
> > > > + case 24:
> > > > + case 32:
> > > > + case 64:
> > > > + if (addr_widths & (1 << (bit
Re: [PATCH V6 3/4] ASoC: pcm_dmaengine: Extract snd_dmaengine_pcm_refine_runtime_hwparams
On Wed, Jan 8, 2020 at 8:58 PM John Stultz wrote:
> On Thu, Sep 26, 2019 at 6:50 PM Shengjiu Wang wrote:
> >
> > When set the runtime hardware parameters, we may need to query
> > the capability of DMA to complete the parameters.
> >
> > This patch is to Extract this operation from
> > dmaengine_pcm_set_runtime_hwparams function to a separate function
> > snd_dmaengine_pcm_refine_runtime_hwparams, that other components
> > which need this feature can call this function.
> >
> > Signed-off-by: Shengjiu Wang
> > Reviewed-by: Nicolin Chen
>
> As a heads up, this patch seems to be causing a regression on the HiKey board.
>
> On boot up I'm seeing:
> [ 17.721424] hi6210_i2s f7118000.i2s: ASoC: can't open component
> f7118000.i2s: -6
>
> And HDMI audio isn't working. With this patch reverted, audio works again.
>
>
> > diff --git a/sound/core/pcm_dmaengine.c b/sound/core/pcm_dmaengine.c
> > index 89a05926ac73..5749a8a49784 100644
> > --- a/sound/core/pcm_dmaengine.c
> > +++ b/sound/core/pcm_dmaengine.c
> > @@ -369,4 +369,87 @@ int snd_dmaengine_pcm_close_release_chan(struct
> > snd_pcm_substream *substream)
> ...
> > + ret = dma_get_slave_caps(chan, _caps);
> > + if (ret == 0) {
> > + if (dma_caps.cmd_pause && dma_caps.cmd_resume)
> > + hw->info |= SNDRV_PCM_INFO_PAUSE |
> > SNDRV_PCM_INFO_RESUME;
> > + if (dma_caps.residue_granularity <=
> > DMA_RESIDUE_GRANULARITY_SEGMENT)
> > + hw->info |= SNDRV_PCM_INFO_BATCH;
> > +
> > + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> > + addr_widths = dma_caps.dst_addr_widths;
> > + else
> > + addr_widths = dma_caps.src_addr_widths;
> > + }
>
> It seems a failing ret from dma_get_slave_caps() here is being returned...
>
> > +
> > + /*
> > +* If SND_DMAENGINE_PCM_DAI_FLAG_PACK is set keep
> > +* hw.formats set to 0, meaning no restrictions are in place.
> > +* In this case it's the responsibility of the DAI driver to
> > +* provide the supported format information.
> > +*/
> > + if (!(dma_data->flags & SND_DMAENGINE_PCM_DAI_FLAG_PACK))
> > + /*
> > +* Prepare formats mask for valid/allowed sample types. If
> > the
> > +* dma does not have support for the given physical word
> > size,
> > +* it needs to be masked out so user space can not use the
> > +* format which produces corrupted audio.
> > +* In case the dma driver does not implement the slave_caps
> > the
> > +* default assumption is that it supports 1, 2 and 4 bytes
> > +* widths.
> > +*/
> > + for (i = SNDRV_PCM_FORMAT_FIRST; i <=
> > SNDRV_PCM_FORMAT_LAST; i++) {
> > + int bits = snd_pcm_format_physical_width(i);
> > +
> > + /*
> > +* Enable only samples with DMA supported physical
> > +* widths
> > +*/
> > + switch (bits) {
> > + case 8:
> > + case 16:
> > + case 24:
> > + case 32:
> > + case 64:
> > + if (addr_widths & (1 << (bits / 8)))
> > + hw->formats |=
> > pcm_format_to_bits(i);
> > + break;
> > + default:
> > + /* Unsupported types */
> > + break;
> > + }
> > + }
> > +
> > + return ret;
>
> ... down here.
>
> Where as in the old code...
>
> > diff --git a/sound/soc/soc-generic-dmaengine-pcm.c
> > b/sound/soc/soc-generic-dmaengine-pcm.c
> > index 748f5f641002..b9f147eaf7c4 100644
> > --- a/sound/soc/soc-generic-dmaengine-pcm.c
> > +++ b/sound/soc/soc-generic-dmaengine-pcm.c
>
> > @@ -145,56 +140,12 @@ static int dmaengine_pcm_set_runtime_hwparams(struct
> > snd_pcm_substream *substrea
> > if (pcm->flags & SND_DMAENGINE_PCM_FLAG_NO_RESIDUE)
> > hw.info |= SNDRV_PCM_INFO_BATCH;
> >
> > - ret = dma_get_sl
Re: [PATCH V6 3/4] ASoC: pcm_dmaengine: Extract snd_dmaengine_pcm_refine_runtime_hwparams
On Thu, Sep 26, 2019 at 6:50 PM Shengjiu Wang wrote:
>
> When set the runtime hardware parameters, we may need to query
> the capability of DMA to complete the parameters.
>
> This patch is to Extract this operation from
> dmaengine_pcm_set_runtime_hwparams function to a separate function
> snd_dmaengine_pcm_refine_runtime_hwparams, that other components
> which need this feature can call this function.
>
> Signed-off-by: Shengjiu Wang
> Reviewed-by: Nicolin Chen
As a heads up, this patch seems to be causing a regression on the HiKey board.
On boot up I'm seeing:
[ 17.721424] hi6210_i2s f7118000.i2s: ASoC: can't open component
f7118000.i2s: -6
And HDMI audio isn't working. With this patch reverted, audio works again.
> diff --git a/sound/core/pcm_dmaengine.c b/sound/core/pcm_dmaengine.c
> index 89a05926ac73..5749a8a49784 100644
> --- a/sound/core/pcm_dmaengine.c
> +++ b/sound/core/pcm_dmaengine.c
> @@ -369,4 +369,87 @@ int snd_dmaengine_pcm_close_release_chan(struct
> snd_pcm_substream *substream)
...
> + ret = dma_get_slave_caps(chan, _caps);
> + if (ret == 0) {
> + if (dma_caps.cmd_pause && dma_caps.cmd_resume)
> + hw->info |= SNDRV_PCM_INFO_PAUSE |
> SNDRV_PCM_INFO_RESUME;
> + if (dma_caps.residue_granularity <=
> DMA_RESIDUE_GRANULARITY_SEGMENT)
> + hw->info |= SNDRV_PCM_INFO_BATCH;
> +
> + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> + addr_widths = dma_caps.dst_addr_widths;
> + else
> + addr_widths = dma_caps.src_addr_widths;
> + }
It seems a failing ret from dma_get_slave_caps() here is being returned...
> +
> + /*
> +* If SND_DMAENGINE_PCM_DAI_FLAG_PACK is set keep
> +* hw.formats set to 0, meaning no restrictions are in place.
> +* In this case it's the responsibility of the DAI driver to
> +* provide the supported format information.
> +*/
> + if (!(dma_data->flags & SND_DMAENGINE_PCM_DAI_FLAG_PACK))
> + /*
> +* Prepare formats mask for valid/allowed sample types. If the
> +* dma does not have support for the given physical word size,
> +* it needs to be masked out so user space can not use the
> +* format which produces corrupted audio.
> +* In case the dma driver does not implement the slave_caps
> the
> +* default assumption is that it supports 1, 2 and 4 bytes
> +* widths.
> +*/
> + for (i = SNDRV_PCM_FORMAT_FIRST; i <= SNDRV_PCM_FORMAT_LAST;
> i++) {
> + int bits = snd_pcm_format_physical_width(i);
> +
> + /*
> +* Enable only samples with DMA supported physical
> +* widths
> +*/
> + switch (bits) {
> + case 8:
> + case 16:
> + case 24:
> + case 32:
> + case 64:
> + if (addr_widths & (1 << (bits / 8)))
> + hw->formats |= pcm_format_to_bits(i);
> + break;
> + default:
> + /* Unsupported types */
> + break;
> + }
> + }
> +
> + return ret;
... down here.
Where as in the old code...
> diff --git a/sound/soc/soc-generic-dmaengine-pcm.c
> b/sound/soc/soc-generic-dmaengine-pcm.c
> index 748f5f641002..b9f147eaf7c4 100644
> --- a/sound/soc/soc-generic-dmaengine-pcm.c
> +++ b/sound/soc/soc-generic-dmaengine-pcm.c
> @@ -145,56 +140,12 @@ static int dmaengine_pcm_set_runtime_hwparams(struct
> snd_pcm_substream *substrea
> if (pcm->flags & SND_DMAENGINE_PCM_FLAG_NO_RESIDUE)
> hw.info |= SNDRV_PCM_INFO_BATCH;
>
> - ret = dma_get_slave_caps(chan, _caps);
> - if (ret == 0) {
> - if (dma_caps.cmd_pause && dma_caps.cmd_resume)
> - hw.info |= SNDRV_PCM_INFO_PAUSE |
> SNDRV_PCM_INFO_RESUME;
> - if (dma_caps.residue_granularity <=
> DMA_RESIDUE_GRANULARITY_SEGMENT)
> - hw.info |= SNDRV_PCM_INFO_BATCH;
> -
> - if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> - addr_widths = dma_caps.dst_addr_widths;
> - else
> - addr_widths = dma_caps.src_addr_widths;
> - }
...the ret from dma_get_slave_caps() checked above, but is not
actually returned.
Suggestions on how to sort this out?
thanks
-john
Re: [PATCH] powerpc: Convert VDSO update function to use new update_vsyscall interface
On Sat, May 27, 2017 at 1:04 AM, Paul Mackerras <pau...@ozlabs.org> wrote:
> This converts the powerpc VDSO time update function to use the new
> interface introduced in commit 576094b7f0aa ("time: Introduce new
> GENERIC_TIME_VSYSCALL", 2012-09-11). Where the old interface gave
> us the time as of the last update in seconds and whole nanoseconds,
> with the new interface we get the nanoseconds part effectively in
> a binary fixed-point format with tk->tkr_mono.shift bits to the
> right of the binary point.
>
> With the old interface, the fractional nanoseconds got truncated,
> meaning that the value returned by the VDSO clock_gettime function
> would have about 1ns of jitter in it compared to the value computed
> by the generic timekeeping code in the kernel.
>
> The powerpc VDSO time functions (clock_gettime and gettimeofday)
> already work in units of 2^-32 seconds, or 0.23283 ns, because that
> makes it simple to split the result into seconds and fractional
> seconds, and represent the fractional seconds in either microseconds
> or nanoseconds. This is good enough accuracy for now, so this patch
> avoids changing how the VDSO works or the interface in the VDSO data
> page.
>
> This patch converts the powerpc update_vsyscall_old to be called
> update_vsyscall and use the new interface. We convert the fractional
> second to units of 2^-32 seconds without truncating to whole nanoseconds.
> (There is still a conversion to whole nanoseconds for any legacy users
> of the vdso_data/systemcfg stamp_xtime field.)
>
> In addition, this improves the accuracy of the computation of tb_to_xs
> for those systems with high-frequency timebase clocks (>= 268.5 MHz)
> by doing the right shift in two parts, one before the multiplication and
> one after, rather than doing the right shift before the multiplication.
> (We can't do all of the right shift after the multiplication unless we
> use 128-bit arithmetic.)
>
> Signed-off-by: Paul Mackerras <pau...@ozlabs.org>
Apologies again for missing this earlier.
So no objections from me. I can't say I really worked the whole thing
out, but you're handling the xtime_nsec field properly and the rest
looks reasonable and is well documented.
So for what its worth:
Acked-by: John Stultz <john.stu...@linaro.org>
Thanks again for making this update!
-john
Re: [PATCH] tick-broadcast: Fix the printing of broadcast masks
On Sun, May 3, 2015 at 10:51 PM, Preeti U Murthy pre...@linux.vnet.ibm.com wrote: Ping. Any comments on this patch ? Got this queued for testing, and hopefully 4.2 thanks -john ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [tip:timers/core] timekeeping: Fixup typo in update_vsyscall_old definition
On 08/10/2014 09:19 PM, Benjamin Herrenschmidt wrote: On Wed, 2014-07-30 at 00:31 -0700, tip-bot for John Stultz wrote: Commit-ID: 953dec21aed4038464fec02f96a2f1b8701a5bce Gitweb: http://git.kernel.org/tip/953dec21aed4038464fec02f96a2f1b8701a5bce Author: John Stultz john.stu...@linaro.org AuthorDate: Fri, 25 Jul 2014 21:37:19 -0700 Committer: Thomas Gleixner t...@linutronix.de CommitDate: Wed, 30 Jul 2014 09:26:25 +0200 timekeeping: Fixup typo in update_vsyscall_old definition In commit 4a0e637738f0 (clocksource: Get rid of cycle_last), currently in the -tip tree, there was a small typo where cycles_t was used intstead of cycle_t. This broke ppc64 builds. There's another bug in there... You fix timespec vs. timespec64 for the first argument of update_vsyscall_old but not the second one ... (wall_to_monotonic). Also, in e2dff1ec0 you claim this is minor, you seem to forget that arch/powerpc also deals with 32-bit kernels which use the same time keeping code, so we have a pretty serious regressions here... Yikes. My apologies. I had missed that issue and had forgotten ppc32 has the vsyscall support as well. Thanks for pointing it out. I'll send a fix here shortly (though I only have the ppc64le toolchain handy, so forgive me if its not quite right). BTW. Is there some documentation you can point me to to figure out what replace that _OLD stuff so we can update to whatever is new ? So there's not exactly documentation, but the idea is rather then doing: nsecs + (mult*(now - cycle_last) shift); We're preserving the sub-nanosecond precision, and doing: (shifted_nsecs + mult*(now-cycle_last)) shift This avoids the rounding up 1ns every tick, which we did to avoid errors from truncating the precision. I think the hard part for ppc, is if I recall, ppc's vsyscall exports the xsec unit, which less granular then nanoseconds, so it had its own version of the same precision truncation issues. I recall Paul working on that, and I thought his solution was to reduce the multiplier by one so the inter-tick time was slightly slower, then the tick update would catch up causing a slight stair-step, which isn't ideal but is better then the inconsistencies that came out of not-handling the precision properly. That said, skimming the ppc code, I don't see that logic right off, and have a fuzzy memory that maybe that solution was RHEL5 specific or something like that. thanks -john ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v2] NTP: Add a CONFIG_RTC_SYSTOHC configuration
On 12/14/2012 03:19 PM, Jason Gunthorpe wrote: The purpose of this option is to allow ARM/etc systems that rely on the class RTC subsystem to have the same kind of automatic NTP based synchronization that we have on PC platforms. Today ARM does not implement update_persistent_clock and makes extensive use of the class RTC system. When enabled CONFIG_RTC_SYSTOHC will provide a generic rtc_update_persistent_clock that stores the current time in the RTC and is intended complement the existing CONFIG_RTC_HCTOSYS option that loads the RTC at boot. Like with RTC_HCTOSYS the platform's update_persistent_clock is used first, if it works. Platforms with mixed class RTC and non-RTC drivers need to return ENODEV when class RTC should be used. Such an update for PPC is included in this patch. Long term, implementations of update_persistent_clock should migrate to proper class RTC drivers and use CONFIG_RTC_SYSTOHC instead. Ok. This all sounds good. Still one minor question below. diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig index 19c03ab..7b3702b 100644 --- a/drivers/rtc/Kconfig +++ b/drivers/rtc/Kconfig @@ -28,6 +28,7 @@ config RTC_HCTOSYS config RTC_HCTOSYS_DEVICE string RTC used to set the system time depends on RTC_HCTOSYS = y + depends on RTC_SYSTOHC = y Is this right? This should probably be a depends on RTC_HCTOSYS OR RTC_SYSTOCH.. Otherwise you have to select both in order to change the default device. diff --git a/drivers/rtc/systohc.c b/drivers/rtc/systohc.c new file mode 100644 index 000..a625740 --- /dev/null +++ b/drivers/rtc/systohc.c @@ -0,0 +1,44 @@ +/* + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 as published by + * the Free Software Foundation. + * + */ +#include linux/rtc.h +#include linux/time.h + +/** + * rtc_update_persistent_clock - Save NTP synchronized time to the RTC + * @now: Current time of day + * + * Replacement for the NTP platform function update_persistent_clock + * that stores time for later retrieval by rtc_hctosys + * + * Returns 0 on successful RTC update, -ENODEV if a RTC update is not + * possible at all, and various other -errno for specific temporary failure + * cases. + * + * If temporary failure is indicated the caller should try again 'soon' + */ +int rtc_update_persistent_clock(struct timespec now) If we're going to move away from update_persistent_clock across the board (and as the update path doesn't have the same constraints of the read_persistent_clock interface), might it be better just to name this: rtc_update_clock() (or something similar)? That way if/when we do finally remove the other users of update_persistent_clock() and move them to an RTC driver, we will avoid any confusion between read/update. This is in the similar vein of your suggestion of changing update_persistent_clock to platform_save_ntp_time_to_rtc().. Sorry for the last minute nit! Other then these two issues, I'm happy to queue this (if Alessandro doesn't object). Although with the merge window already open it may have to wait to 3.9, but I'll see what Thomas says. thanks -john ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 4/8] time: Condense timekeeper.xtime into xtime_sec
On 08/21/2012 12:14 AM, Andreas Schwab wrote:
John Stultz john.stu...@linaro.org writes:
@@ -115,6 +115,7 @@ static void tk_xtime_add(struct timekeeper *tk, const
struct timespec *ts)
{
tk-xtime_sec += ts-tv_sec;
tk-xtime_nsec += (u64)ts-tv_nsec tk-shift;
+ tk_normalize_xtime(tk);
}
Yes, that does it. Failure to normalize is always bad.
Great. Thanks for the testing and the bug report! I'll be sending out
the patch to -tip later today.
thanks
-john
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 4/8] time: Condense timekeeper.xtime into xtime_sec
On 08/19/2012 02:02 PM, Andreas Schwab wrote:
John Stultz john.stu...@linaro.org writes:
The timekeeper struct has a xtime_nsec, which keeps the
sub-nanosecond remainder. This ends up being somewhat
duplicative of the timekeeper.xtime.tv_nsec value, and we
have to do extra work to keep them apart, copying the full
nsec portion out and back in over and over.
This patch simplifies some of the logic by taking the timekeeper
xtime value and splitting it into timekeeper.xtime_sec and
reuses the timekeeper.xtime_nsec for the sub-second portion
(stored in higher res shifted nanoseconds).
This simplifies some of the accumulation logic. And will
allow for more accurate timekeeping once the vsyscall code
is updated to use the shifted nanosecond remainder.
This (together with b44d50d time: Fix casting issue in tk_set_xtime and
tk_xtime_add) is causing resume to hang on the iBook (PowerBook6,7).
The fact that the add-on commit is needed to uncover the bug might give
a hint, but I'm unable to decipher it.
Thanks for the bug report and narrowing this down.
I'm not very familiar w/ the iBook hardware, but does it use a
clocksource, or does it use arch_gettimeoffset()?
I suspect that the casting has avoided clipping some strange values from
the persistent clock.
Could you try with the following patch against Linus' HEAD? I suspect it
will let the box resume (although it will seem as though no time was
spent in resume) and then let me know what the JDB lines print out?
thanks
-john
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index e16af19..03f5a82 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -753,10 +753,14 @@ static void timekeeping_resume(void)
clocksource_resume();
write_seqlock_irqsave(tk-lock, flags);
+ printk(JDB: suspend_time: %ld:%ld resume_time: %ld:%ld\n,
+ timekeeping_suspend_time.tv_sec,
timekeeping_suspend_time.tv_nsec,
+ ts.tv_sec, ts.tv_nsec);
if (timespec_compare(ts, timekeeping_suspend_time) 0) {
ts = timespec_sub(ts, timekeeping_suspend_time);
- __timekeeping_inject_sleeptime(tk, ts);
+ printk(JDB: Trying to add: %ld:%ld\n, ts.tv_sec, ts.tv_nsec);
+ //__timekeeping_inject_sleeptime(tk, ts);
}
/* re-base the last cycle value */
tk-clock-cycle_last = tk-clock-read(tk-clock);
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 4/8] time: Condense timekeeper.xtime into xtime_sec
On 08/20/2012 12:45 PM, Andreas Schwab wrote: John Stultz john.stu...@linaro.org writes: I'm not very familiar w/ the iBook hardware, but does it use a clocksource, or does it use arch_gettimeoffset()? clocksource: timebase mult[3640e38e] shift[24] registered I suspect that the casting has avoided clipping some strange values from the persistent clock. That's my guess as well. Could you try with the following patch against Linus' HEAD? I suspect it will let the box resume (although it will seem as though no time was spent in resume) and then let me know what the JDB lines print out? JDB: suspend_time: 1345491706:0 resume_time: 1345491737:0 JDB: Trying to add: 31:0 (Looks reasonable.) Huh. Yea, that looks fine. And without the __timekeeping_inject_sleeptime() call, the system resumed ok? Thanks for the testing! I'll keep looking here. -john ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 4/8] time: Condense timekeeper.xtime into xtime_sec
On 08/20/2012 01:04 PM, Andreas Schwab wrote:
John Stultz john.stu...@linaro.org writes:
Huh. Yea, that looks fine. And without the
__timekeeping_inject_sleeptime() call, the system resumed ok?
Yes, it does.
So I'm mostly still stumped on this. But I did find one possible related
bugfix that maybe you can try?
Let me know if the patch below dodges the problem, and if not, please
send me the JDB printk output.
(if the resume hangs without any output, add a return; before the
tk_xtime_add() call between the JDB printks).
thanks
-john
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index e16af19..1a9b9c5 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -115,6 +115,7 @@ static void tk_xtime_add(struct timekeeper *tk, const
struct timespec *ts)
{
tk-xtime_sec += ts-tv_sec;
tk-xtime_nsec += (u64)ts-tv_nsec tk-shift;
+ tk_normalize_xtime(tk);
}
static void tk_set_wall_to_mono(struct timekeeper *tk, struct timespec wtm)
@@ -695,9 +696,22 @@ static void __timekeeping_inject_sleeptime(struct
timekeeper *tk,
sleep delta value!\n);
return;
}
+
+ printk(JDB: pre xt %ld:%ld wtm: %ld:%ld st: %ld:%ld\n,
+ tk_xtime(tk).tv_sec, tk_xtime(tk).tv_nsec,
+ tk-wall_to_monotonic.tv_sec,tk-wall_to_monotonic.tv_nsec,
+ tk-total_sleep_time.tv_sec, tk-total_sleep_time.tv_nsec);
+ printk(JDB: Adding %ld:%ld\n, delta-tv_sec, delta-tv_nsec);
+
tk_xtime_add(tk, delta);
tk_set_wall_to_mono(tk, timespec_sub(tk-wall_to_monotonic, *delta));
tk_set_sleep_time(tk, timespec_add(tk-total_sleep_time, *delta));
+
+ printk(JDB: post xt %ld:%ld wtm: %ld:%ld st: %ld:%ld\n,
+ tk_xtime(tk).tv_sec, tk_xtime(tk).tv_nsec,
+ tk-wall_to_monotonic.tv_sec,tk-wall_to_monotonic.tv_nsec,
+ tk-total_sleep_time.tv_sec, tk-total_sleep_time.tv_nsec);
+
}
/**
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 3/6] powerpc/time: Use clocksource_register_hz
On Thu, 2011-11-24 at 17:07 +1100, Anton Blanchard wrote: plain text document attachment (clock3) Use clocksource_register_hz which calculates the shift/mult factors for us. Also remove the shift = 22 assumption in vsyscall_update - thanks to Paul Mackerras and John Stultz for catching that. Signed-off-by: Anton Blanchard an...@samba.org --- Index: linux-build/arch/powerpc/kernel/time.c === --- linux-build.orig/arch/powerpc/kernel/time.c 2011-11-17 10:11:51.175038860 +1100 +++ linux-build/arch/powerpc/kernel/time.c2011-11-17 10:11:55.547114957 +1100 @@ -86,8 +86,6 @@ static struct clocksource clocksource_rt .rating = 400, .flags= CLOCK_SOURCE_IS_CONTINUOUS, .mask = CLOCKSOURCE_MASK(64), - .shift= 22, - .mult = 0, /* To be filled in */ .read = rtc_read, }; @@ -97,8 +95,6 @@ static struct clocksource clocksource_ti .rating = 400, .flags= CLOCK_SOURCE_IS_CONTINUOUS, .mask = CLOCKSOURCE_MASK(64), - .shift= 22, - .mult = 0, /* To be filled in */ .read = timebase_read, }; @@ -822,9 +818,8 @@ void update_vsyscall(struct timespec *wa ++vdso_data-tb_update_count; smp_mb(); - /* XXX this assumes clock-shift == 22 */ - /* 4611686018 ~= 2^(20+64-22) / 1e9 */ - new_tb_to_xs = (u64) mult * 4611686018ULL; + /* 19342813113834067 ~= 2^(20+64) / 1e9 */ + new_tb_to_xs = (u64) mult * (19342813113834067ULL clock-shift); I never verified the math on this, but assuming Paul had it right, this patch looks good! Acked-by: John Stultz johns...@us.ibm.com Thanks again! -john ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/4] powerpc/time: Use clocksource_register_hz
On Sat, 2011-11-05 at 11:55 +1100, Paul Mackerras wrote: On Thu, Nov 03, 2011 at 09:14:44AM -0400, John Stultz wrote: On Thu, 2011-11-03 at 11:59 +1100, Anton Blanchard wrote: plain text document attachment (clock3) Use clocksource_register_hz which calculates the shift/mult factors for us. Signed-off-by: Anton Blanchard an...@samba.org --- Index: linux-build/arch/powerpc/kernel/time.c === --- linux-build.orig/arch/powerpc/kernel/time.c 2011-11-03 10:19:59.493679032 +1100 +++ linux-build/arch/powerpc/kernel/time.c2011-11-03 10:20:00.965704053 +1100 @@ -86,8 +86,6 @@ static struct clocksource clocksource_rt .rating = 400, .flags= CLOCK_SOURCE_IS_CONTINUOUS, .mask = CLOCKSOURCE_MASK(64), - .shift= 22, - .mult = 0, /* To be filled in */ .read = rtc_read, }; @@ -97,8 +95,6 @@ static struct clocksource clocksource_ti .rating = 400, .flags= CLOCK_SOURCE_IS_CONTINUOUS, .mask = CLOCKSOURCE_MASK(64), - .shift= 22, - .mult = 0, /* To be filled in */ .read = timebase_read, }; So I've held off on ppc conversion to clocksource_register_hz due to the fact that the ppc vdso gettimeofday at least used to make assumptions that shift was 22. Is that no longer the case? It is still the case; specifically, update_vsyscall() in arch/powerpc/kernel/time.c converts a multiplier value to a 'tb_to_xs' multiplier (timebase to xsec conversion factor, where 1 xsec = 2^-20 seconds) using a factor which assumes a shift of 22. The factor needs to be 2^(20 + 64 - shift) / 1e9, so we could accommodate other shift values by changing the line that computes new_tb_to_xs to do new_tb_to_xs = (u64) mult * (19342813113834067ULL shift); assuming the shift value is easily available to update_vsyscall (I assume it would be clock-shift). Ok. That sounds reasonable. clock-shift should be correct there. thanks -john ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/4] powerpc/time: Use clocksource_register_hz
On Thu, 2011-11-03 at 11:59 +1100, Anton Blanchard wrote: plain text document attachment (clock3) Use clocksource_register_hz which calculates the shift/mult factors for us. Signed-off-by: Anton Blanchard an...@samba.org --- Index: linux-build/arch/powerpc/kernel/time.c === --- linux-build.orig/arch/powerpc/kernel/time.c 2011-11-03 10:19:59.493679032 +1100 +++ linux-build/arch/powerpc/kernel/time.c2011-11-03 10:20:00.965704053 +1100 @@ -86,8 +86,6 @@ static struct clocksource clocksource_rt .rating = 400, .flags= CLOCK_SOURCE_IS_CONTINUOUS, .mask = CLOCKSOURCE_MASK(64), - .shift= 22, - .mult = 0, /* To be filled in */ .read = rtc_read, }; @@ -97,8 +95,6 @@ static struct clocksource clocksource_ti .rating = 400, .flags= CLOCK_SOURCE_IS_CONTINUOUS, .mask = CLOCKSOURCE_MASK(64), - .shift= 22, - .mult = 0, /* To be filled in */ .read = timebase_read, }; So I've held off on ppc conversion to clocksource_register_hz due to the fact that the ppc vdso gettimeofday at least used to make assumptions that shift was 22. Is that no longer the case? thanks -john ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH V13 1/4] ptp: Added a brand new class driver for ptp clocks.
On Sun, 2011-03-27 at 08:38 +0200, Richard Cochran wrote: This patch adds an infrastructure for hardware clocks that implement IEEE 1588, the Precision Time Protocol (PTP). A class driver offers a registration method to particular hardware clock drivers. Each clock is presented as a standard POSIX clock. The ancillary clock features are exposed in two different ways, via the sysfs and by a character device. Signed-off-by: Richard Cochran richard.coch...@omicron.at Acked-by: John Stultz john.stu...@linaro.org thanks -john ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH V13 2/4] ptp: Added a clock that uses the eTSEC found on the MPC85xx.
On Sun, 2011-03-27 at 08:38 +0200, Richard Cochran wrote: The eTSEC includes a PTP clock with quite a few features. This patch adds support for the basic clock adjustment functions, plus two external time stamps, one alarm, and the PPS callback. Signed-off-by: Richard Cochran richard.coch...@omicron.at Acked-by: John Stultz john.stu...@linaro.org ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH V13 3/4] ptp: Added a clock driver for the IXP46x.
On Sun, 2011-03-27 at 08:39 +0200, Richard Cochran wrote: This patch adds a driver for the hardware time stamping unit found on the IXP465. The basic clock operations and an external trigger are implemented. Signed-off-by: Richard Cochran richard.coch...@omicron.at Acked-by: John Stultz john.stu...@linaro.org ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH V13 4/4] ptp: Added a clock driver for the National Semiconductor PHYTER.
On Sun, 2011-03-27 at 08:39 +0200, Richard Cochran wrote: This patch adds support for the PTP clock found on the DP83640. The basic clock operations and one external time stamp have been implemented. Signed-off-by: Richard Cochran richard.coch...@omicron.at Acked-by: John Stultz john.stu...@linaro.org ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH V12 1/4] ptp: Added a brand new class driver for ptp clocks.
On Mon, 2011-02-28 at 08:57 +0100, Richard Cochran wrote:
This patch adds an infrastructure for hardware clocks that implement
IEEE 1588, the Precision Time Protocol (PTP). A class driver offers a
registration method to particular hardware clock drivers. Each clock is
presented as a standard POSIX clock.
The ancillary clock features are exposed in two different ways, via
the sysfs and by a character device.
Signed-off-by: Richard Cochran richard.coch...@omicron.at
I think this looks mostly ok. I've got one concern on the locking and
there are a few minor nits that might need extra commenting below.
thanks
-john
--- /dev/null
+++ b/Documentation/ptp/testptp.c
[snip]
+ if (0x7fff != adjfreq) {
+ memset(tx, 0, sizeof(tx));
+ tx.modes = ADJ_FREQUENCY;
+ tx.freq = (long) (adjfreq * 65.536);
So, 65.536... I understand you're converting ppb to ppm 16 (which is
what adjtime takes), but I'm not sure if anyone else will. I'd either
wrap that in a commented conversion function/macro or just do the
conversion explicitly (freq = (adjfreq 16)/1000; although this has
potential overflow risks with large ppb values) with a comment about
what is going on.
--- /dev/null
+++ b/drivers/ptp/ptp_clock.c
@@ -0,0 +1,320 @@
[snip]
+static void enqueue_external_timestamp(struct timestamp_event_queue *queue,
+struct ptp_clock_event *src)
+{
+ struct ptp_extts_event *dst;
+ unsigned long flags;
+ u32 remainder;
+
+ dst = queue-buf[queue-tail];
Doesn't the lock need to happen before you access the
queue-buf[queue-tail] ?
For example: What happens if two cpus enter the function at the same
time, both get the same tail index, one overwrite the other's data, then
both take turns bumping up the tail pointer?
+ dst-index = src-index;
+ dst-t.sec = div_u64_rem(src-timestamp, 10, remainder);
+ dst-t.nsec = remainder;
+
+ spin_lock_irqsave(queue-lock, flags);
+
+ if (!queue_free(queue))
+ queue-head = (queue-head + 1) % PTP_MAX_TIMESTAMPS;
+
+ queue-tail = (queue-tail + 1) % PTP_MAX_TIMESTAMPS;
+
+ spin_unlock_irqrestore(queue-lock, flags);
+}
[snip]
+static int ptp_clock_adjtime(struct posix_clock *pc, struct timex *tx)
+{
+ struct ptp_clock *ptp = container_of(pc, struct ptp_clock, clock);
+ struct ptp_clock_info *ops;
+ int err = -EOPNOTSUPP;
+
+ ops = ptp-info;
+
+ if (tx-modes ADJ_SETOFFSET) {
+ struct timespec ts;
+ ktime_t kt;
+ s64 delta;
+
+ ts.tv_sec = tx-time.tv_sec;
+ ts.tv_nsec = tx-time.tv_usec;
+
+ if (!(tx-modes ADJ_NANO))
+ ts.tv_nsec *= 1000;
+
+ if ((unsigned long) ts.tv_nsec = NSEC_PER_SEC)
+ return -EINVAL;
+
+ kt = timespec_to_ktime(ts);
+ delta = ktime_to_ns(kt);
+ err = ops-adjtime(ops, delta);
+
+ } else if (tx-modes ADJ_FREQUENCY) {
+ s64 ppb = 1 + tx-freq;
+ ppb *= 125;
+ ppb = 13;
((1 + freq) *125)13 == magic :)
Needs a clear comment.
+ err = ops-adjfreq(ops, (s32)ppb);
+ }
+
+ return err;
+}
[snip]
diff --git a/drivers/ptp/ptp_private.h b/drivers/ptp/ptp_private.h
new file mode 100644
index 000..2f76266
--- /dev/null
+++ b/drivers/ptp/ptp_private.h
@@ -0,0 +1,86 @@
+/*
+ * PTP 1588 clock support - private declarations for the core module.
+ *
+ * Copyright (C) 2010 OMICRON electronics GmbH
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+#ifndef _PTP_PRIVATE_H_
+#define _PTP_PRIVATE_H_
+
+#include linux/cdev.h
+#include linux/device.h
+#include linux/mutex.h
+#include linux/posix-clock.h
+#include linux/ptp_clock.h
+#include linux/ptp_clock_kernel.h
+#include linux/time.h
+
+#define PTP_MAX_TIMESTAMPS 128
+#define PTP_BUF_TIMESTAMPS 30
+
+struct timestamp_event_queue {
+ struct ptp_extts_event buf[PTP_MAX_TIMESTAMPS];
+ int head;
+ int tail;
+ spinlock_t lock;
+};
+
+struct ptp_clock {
+ struct posix_clock clock;
+ struct device *dev;
+ struct ptp_clock_info *info;
+ dev_t devid;
+
Re: [PATCH V12 3/4] ptp: Added a clock driver for the IXP46x.
On Mon, 2011-02-28 at 08:58 +0100, Richard Cochran wrote: This patch adds a driver for the hardware time stamping unit found on the IXP465. The basic clock operations and an external trigger are implemented. Signed-off-by: Richard Cochran richard.coch...@omicron.at Acked-by: John Stultz johns...@us.ibm.com My ack is still fine, but found a few more nits to consider on another pass... +static int match(struct sk_buff *skb, u16 uid_hi, u32 uid_lo, u16 seq) [snip] +static void do_rx_timestamp(struct port *port, struct sk_buff *skb) [snip] +static u64 sys_time_read(struct ixp46x_ts_regs *regs) [snip] +static void sys_time_write(struct ixp46x_ts_regs *regs, u64 ns) I know these are static, but these function names are sort of generic names, and make grep/cscoping for similar sounding functions a little noisy. Maybe add a ipx_ prefix just to be more obvious? thanks -john ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH V12 4/4] ptp: Added a clock driver for the National Semiconductor PHYTER.
On Mon, 2011-02-28 at 08:58 +0100, Richard Cochran wrote:
This patch adds support for the PTP clock found on the DP83640.
The basic clock operations and one external time stamp have
been implemented.
Just locking rule comment nits here.
+static int tdr_write(int bc, struct phy_device *dev,
+ const struct timespec *ts, u16 cmd)
+{
+ ext_write(bc, dev, PAGE4, PTP_TDR, ts-tv_nsec 0x);/* ns[15:0] */
+ ext_write(bc, dev, PAGE4, PTP_TDR, ts-tv_nsec 16); /* ns[31:16] */
+ ext_write(bc, dev, PAGE4, PTP_TDR, ts-tv_sec 0x); /* sec[15:0] */
+ ext_write(bc, dev, PAGE4, PTP_TDR, ts-tv_sec 16);/* sec[31:16]*/
+
+ ext_write(bc, dev, PAGE4, PTP_CTL, cmd);
+
+ return 0;
+}
The above needs to hold the extreg_lock, and should be commented as
such.
And again, the function names are sort of generic, and could use a
dp83640_ prefix or something.
thanks
-john
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 1/8] posix clocks: introduce a syscall for clock tuning.
On Fri, 2010-09-24 at 09:29 +0200, Richard Cochran wrote:
On Thu, Sep 23, 2010 at 12:48:51PM -0700, john stultz wrote:
So I'd still split this patch up a little bit more.
1) Patch that implements the ADJ_SETOFFSET (*and its implementation*)
in do_adjtimex.
2) Patch that adds the new syscall and clock_id multiplexing.
3) Patches that wire it up to the rest of the architectures (there's
still a bunch missing here).
I was not sure what the policy is about adding syscalls. Is it the
syscall author's responsibility to add it into every arch?
The last time (see a2e2725541fad7) the commit only added half of some
archs, and ignored others. In my patch, the syscall *really* works on
the archs that are present in the patch.
(Actually, I did not test blackfin, since I don't have one, but I
included it since I know they have a PTP hardware clock.)
I'm not sure about policy, but I think for completeness sake you should
make sure every arch supports a new syscall. You're not expected to be
able to test every one, but getting the basic support patch sent to
maintainers should be done.
+static inline int common_clock_adj(const clockid_t which_clock, struct
timex *t)
+{
+ if (CLOCK_REALTIME == which_clock)
+ return do_adjtimex(t);
+ else
+ return -EOPNOTSUPP;
+}
Would it make sense to point to the do_adjtimex() in the k_clock
definition for CLOCK_REALTIME rather then conditionalizing it here?
But what about CLOCK_MONOTONIC_RAW, for example?
-EOPNOTSUPP
Does it make sense to allow it to be adjusted?
No. I think only CLOCK_REALTIME would make sense of the existing clocks.
I'm just suggesting you conditionalize it from the function pointer,
rather then in the common function.
thanks
-john
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v6 0/8] ptp: IEEE 1588 hardware clock support
On Thu, 2010-09-23 at 12:53 -0500, Christoph Lameter wrote: On Thu, 23 Sep 2010, Richard Cochran wrote: In contrast to the standard Linux system clock, a PHC is adjustable in hardware, for example using frequency compensation registers or a VCO. The ability to directly tune the PHC is essential to reap the benefit of hardware timestamping. There is a reason for not being able to shift posix clocks: The system has one time base. The various clocks are contributing to maintaining that sytem wide time. I do not understand why you want to maintain different clocks running at different speeds. Certainly interesting for some uses I guess that I do not have the energy to imagine right now. But can we get the PTP killer feature of synchronized accurate system time first? This was my initial gut reaction as well, but in the end, I agree with Richard that in the case of one or multiple PTP hardware clocks, we really can't abstract over the different time domains. 3.3 Synchronizing the Linux System Time One could offer a PHC as a combined clock source and clock event device. The advantage of this approach would be that it obviates the need for synchronization when the PHC is selected as the system timer. However, some PHCs, namely the PHY based clocks, cannot be used in this way. Why not? Do PHY based clock not at least provide a counter that increments in synchronized intervals throughout the network? I really don't think the PTP clock can be used as a clocksource sanely. First, the hardware access is much to slow for system timekeeping. Second, there is the problem that the system time is a software clock, and adjustments made (like freq) are made in the layer that interprets the underlying hardware cycle counter. Adjustments made in PTP (in order to sync the network timestamps) are made at the hardware level. This would cause a disconnect between the hardware freq understood by the system time management code and the actual hardware freq. Richard, I'd actually strike this paragraph from the rational, as I feel it has the tendency to confuse as it suggests having the PHC as a clocksource is feasible when really it isn't. Or alternatively, maybe express more clearly why its not feasible, so it doesn't just seem like a minor design choice. thanks -john ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v6 0/8] ptp: IEEE 1588 hardware clock support
On Thu, 2010-09-23 at 19:30 +0200, Richard Cochran wrote: Here is the sixth version of my patch set adding PTP hardware clock support to the Linux kernel. The main difference to v5 is that the character device interface has been replaced with one based on the posix clock system calls. The first three patches add necessary background support in the posix clock code. The last five add the new PTP hardware clock features. Previously, I had tried to present the posix clock changes all by themselves, but commentators asked to see the whole context. Richard, Its great to see this work continue and the patch set is shaping up nicely! There's still a few details to work out, but I think the remaining issues are relatively small. 3.2.3 Dynamic POSIX Clock IDs -- The reaction on the list to having a static id like CLOCK_PTP was mostly negative. However, the idea of generating a clock id dynamically seems to have gained acceptance. The general idea is to advertise the available clock ids to user space via sysfs. This patch set implements two different ways: /sys/class/timesource/name/id /sys/class/ptp/ptp_clock_X/id Note: I am not too sure that this is exactly what people imagined, but it is my best understanding so far. I gleaned two different ideas about where to offer the clock id. In order to keep just one way, I will be happy to remove the less popular one. So yea, I'm not a fan of the timesource sysfs interface. One, I think the name is poor (posix_clocks or something a little more specific would be an improvement), and second, I don't like the dictionary interface, where one looks up the clock by name. Instead, I think having the id hanging off the class driver is much better, as it allows mapping the actual hardware to the id more clearly. So I'd drop the timesource listing. And maybe change id to clock_id so its a little more clear what the id is for. 3.3 Synchronizing the Linux System Time One could offer a PHC as a combined clock source and clock event device. The advantage of this approach would be that it obviates the need for synchronization when the PHC is selected as the system timer. However, some PHCs, namely the PHY based clocks, cannot be used in this way. Again, I'd scratch this. What I think you might want to mention is that an application like NTP could use the PTP clockid much like NTP currently can be configured to use the RTC to steer the system time. Possibly the PTPd could just do this, reducing the number of deamons and avoiding mixing NTP up in what is really a different sync algorithm. Instead, the patch set provides a way to offer a Pulse Per Second (PPS) event from the PHC to the Linux PPS subsystem. A user space application can read the PPS events and tune the system clock, just like when using other external time sources like radio clocks or GPS. Forgive me for a bit of a tangent here: So while I think this PPS method is a neat idea, I'm a little curious how much of a difference the PPS method for syncing the clock would be over just a simple reading of the two clocks and correcting the offset. It seems much of it depends on the read latency of the PTP hardware vs the interrupt latency. Also the PTP clock granularity would effect the read accuracy (like on the RTC, you don't really know how close to the second boundary you are). Have you done any such measurements between the two methods? I just wonder if it would actually be something noticeable, and if its not, how much lighter this patch-set would be without the PPS connection. Again, this isn't super critical, just trying to make sure we don't end up adding a bunch of code that doesn't end up being used. Also PPS interrupts are awfully frequent, so systems concerned with power-saving and deep idles probably would like something that could be done at a more coarse interval. 3.5 User timers Using the POSIX clock API gived user space the possibility to create and use timers with timer_create and timer_settime. In the current patch set the kernel functionality is not implemented, since there are some issues to consider first. I see two ways to do about this. 1. Implement the functionality anew. This approach might end up duplicating similar code that already exists. Also, looking at the hrtimer code, getting user timers right seems to have a number of gotchas and thorny issues. 2. Reuse the hrtimer code. Since the hrtimer code uses a clock event device under the hood, it might be possible (in theory) to offer capable PHCs as clock event devices. However, the current hrtimers are hard-coded to the event device via a per-cpu global. Perhaps one could associate an event device with a
Re: [PATCH 1/8] posix clocks: introduce a syscall for clock tuning.
On Thu, 2010-09-23 at 19:31 +0200, Richard Cochran wrote:
A new syscall is introduced that allows tuning of a POSIX clock. The
syscall is implemented for four architectures: arm, blackfin, powerpc,
and x86.
The new syscall, clock_adjtime, takes two parameters, the clock ID,
and a pointer to a struct timex. The semantics of the timex struct
have been expanded by one additional mode flag, which allows an
absolute offset correction. When specificied, the clock offset is
immediately corrected by adding the given time value to the current
time value.
So I'd still split this patch up a little bit more.
1) Patch that implements the ADJ_SETOFFSET (*and its implementation*)
in do_adjtimex.
2) Patch that adds the new syscall and clock_id multiplexing.
3) Patches that wire it up to the rest of the architectures (there's
still a bunch missing here).
And one little nit in the code:
diff --git a/kernel/posix-timers.c b/kernel/posix-timers.c
index 9ca4973..446b566 100644
--- a/kernel/posix-timers.c
+++ b/kernel/posix-timers.c
@@ -197,6 +197,14 @@ static int common_timer_create(struct k_itimer
*new_timer)
return 0;
}
+static inline int common_clock_adj(const clockid_t which_clock, struct timex
*t)
+{
+ if (CLOCK_REALTIME == which_clock)
+ return do_adjtimex(t);
+ else
+ return -EOPNOTSUPP;
+}
Would it make sense to point to the do_adjtimex() in the k_clock
definition for CLOCK_REALTIME rather then conditionalizing it here?
thanks
-john
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v6 0/8] ptp: IEEE 1588 hardware clock support
On Thu, 2010-09-23 at 14:15 -0500, Christoph Lameter wrote: On Thu, 23 Sep 2010, john stultz wrote: This was my initial gut reaction as well, but in the end, I agree with Richard that in the case of one or multiple PTP hardware clocks, we really can't abstract over the different time domains. My (arguably still superficial) review of the source does not show anything that would make me reach that conclusion. I really don't think the PTP clock can be used as a clocksource sanely. First, the hardware access is much to slow for system timekeeping. The HPET or pit timesource are also quite slow these days. You only need access periodically to essentially tune the TSC ratio. If we're using the TSC, then we're not using the PTP clock as you suggest. Further the HPET and PIT aren't used to steer the system time when we are using the TSC as a clocksource. Its only used to calibrate the initial constant freq used by the timekeeping code (and if its non-constant, we throw it out). Second, there is the problem that the system time is a software clock, and adjustments made (like freq) are made in the layer that interprets the underlying hardware cycle counter. Adjustments made in PTP (in order to sync the network timestamps) are made at the hardware level. From what I can see the PTP clocks are periodic hardware cycle counters like any other clock that we currently support. If its configurable enough then setup a hardware cycle counter that mimics nanoseconds since the epoch as closely as possible and use that to sync the TSC rate to. Makes it very easy. I guess I'm confused by what you're suggesting. If we're using the TSC, then that's the clocksource timekeeping uses. The original issue seemed to be around the suggestion of using the PTP clock as a clocksource, which I don't think is really feasible. Again, that's because 1) The PTP access latency is slow (so is the PIT, true enough, but no one should be using the PIT as a clocksource unless they really have no better hardware - its really only useful for 486s and old freq scaling laptops that have no other stable clocksource). 2) The way PTP clocks are steered to sync with network time causes their hardware freq to actually change. Since these adjustments are done on the hardware clock level, and not on the system time level, the adjustments to sync the system time/freq would then be made incorrect by PTP hardware adjustments. 3) Further, the PTP hardware counter can be simply set to a new offset to put it in line with the network time. This could cause trouble with timekeeping much like unsynced TSCs do. Now, what you seem to be suggesting is to use the TSC (or whatever clocksource the system time is using) but to steer the system time using the PTP clock. This is actually what is being proposed, however, the steering is done in userland. This is due to the fact that there are two components to the steering, 1) adjusting the PTP clock hardware to network time and 2) adjusting the system time to the PTP hardware. By exposing the PTP clock to userland via the posix clocks interface, we allow this to easily be done. This would cause a disconnect between the hardware freq understood by the system time management code and the actual hardware freq. We can switch underlying clocks for system time already. We can adapt to a different hw frequency. Actually no. The timekeeping code requires a fixed freq counter. Dealing with hardware freq changes is difficult, because error is introduced by the latency between when the freq changes and when the timekeeping code is notified of it. So the system treats the hardware counters as fixed freq. Now, hardware does vary freq ever so slightly as thermal conditions change, but this is addressed in userland and corrected via adjtimex. But then I do not know why adjust the freq? I thought the point was that the periodic clock was network synchronized and can be used as the master clock for multiple machines? Not parsing that. What do you mean by periodic clock? Richard, I'd actually strike this paragraph from the rational, as I feel it has the tendency to confuse as it suggests having the PHC as a clocksource is feasible when really it isn't. Or alternatively, maybe express more clearly why its not feasible, so it doesn't just seem like a minor design choice. Sorry but I still feel that this is pretty much a misguided approach that creates unnecessary layers in the kernel. Unnecessary layers? Where? This approach has less in-kernel layers, as it exposes the PTP clock to userland, instead of trying to layer things on top of it and stretching the system time abstraction to cover it. The trivial easy approach was not done (copy a driver from drivers/clocksource, modify so that it programs access to a centralized periodic ptp signal and uses it for system sync). I disagree. I've argued through the approach trying to keep it all internal to the kernel
Re: [PATCH v6 0/8] ptp: IEEE 1588 hardware clock support
On Thu, 2010-09-23 at 21:36 +0100, Alan Cox wrote: So as far as the POSIX standard is concerned, offering a clock id to represent the PHC would be acceptable. But completely useless as you may have more than one entirely different time managed by PTP and in which you are not master but must work with the timebases provided. I don't see how this is a problem, as it exposes the multiple hardware clocks via different posix clock ids. So in the boundary clock case, you can configure which side is the client and which side is the master in a config file and the PTPd will appropriately steer them individually. /sys/class/timesource/name/id /sys/class/ptp/ptp_clock_X/id Note: I am not too sure that this is exactly what people imagined, but it is my best understanding so far. I gleaned two different ideas about where to offer the clock id. In order to keep just one way, I will be happy to remove the less popular one. I see no fix proposed for the race condition I pointed out. This doesn't work. So, if I recall this was: How do you keep the module from unloading while its being used? There may need to be proper locking for unregistering the posix clock_id on module unload, but I don't think we need a use-count to prevent the module from being unloaded. My question would be: How do we handle a USB network device ($14.99 now with PTP!) being unplugged? We can't say Sorry! That's in use!. So we note the hardware is gone, and return the proper error code. Or am I missing something else? If the Linux system time is synchronized to the PHC via the PPS To which PHC we can have several + Intel IXP465 - Auxiliary Slave/Master Mode Snapshot (optional interrupt) - Target Time (optional interrupt) And about 40 already supported by char driver interface clocks and rtcs in the kernel... And those char driver interfaces are all subtly different. I actually recently submitted an RFC to expose the RTC devices via the posix clock/timer interface, because working with the RTC hardware device directly is terrible for managing alarm interrupts. For instance, you easily run into the case where your TV recording application programs an alarm to record your favorite show at 8pm. Then your backup script programs an alarm to wake up at 2am to do your nightly backups. Your box suspends and the next morning, you're missing your favorite show! I'd say the inability to have multiple clocks and the race condition because of the clockid stuff leaves the proposal dead in the water. It also ignores the existing APIs we have floating around attached to devices. You need to make one small important change. You need to take the POSIX crap about enumerating things out and shoot it, bury it at a crossroads and sprinkle holy water on it. We agree the list-by-name stuff isn't the way to go. :) Drop the clockid_t and swap it for a file handle like a proper Unix or Linux interface. The rest is much the same fd = open /sys/class/timesource/[whatever] various queries you may want to do to check the name etc fclock_adjtime(fd, ...) The posix interface is fundamentally flawed. It only works for staticly enumerable objects. Unix avoided that forty years ago by making the identifier a handle which immediately cures all your object lifetime problems in one swoop. So, I don't really see how that's so different from what is being proposed. The clock_id is dynamically assigned per registered clock, and exposed via the sysfs interface from ptp hardware entry. The only difference is the open/close reference counting, which I don't think is necessary here (since we can't always keep the hardware from going away). thanks -john ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v6 0/8] ptp: IEEE 1588 hardware clock support
On Thu, 2010-09-23 at 15:49 -0500, Christoph Lameter wrote: On Thu, 23 Sep 2010, john stultz wrote: The HPET or pit timesource are also quite slow these days. You only need access periodically to essentially tune the TSC ratio. If we're using the TSC, then we're not using the PTP clock as you suggest. Further the HPET and PIT aren't used to steer the system time when we are using the TSC as a clocksource. Its only used to calibrate the initial constant freq used by the timekeeping code (and if its non-constant, we throw it out). There is no other scalable time source available for fast timer access than the time stamp counter in the cpu. Other time source require memory accesses which is inherently slower. Right, but no one likes the HPET or ACPI PM for a clocksource, its just the TSC isn't usable in some cases, so they have to be used. We don't want to force folks to decide between closely sycned time and fast time reads. So that is part of the reason why PTP as a clocksource isn't a good idea. An accurate other time source is used to adjust this clock. NTP does that via the clock interfaces from user space which has its problems with accuracy. PTP can provide the network synced time access that would a more accurate calibration of the time. Calibration isn't whats needed here (it is an issue, but a separate one - and I've got some patches if you're interested!) as its a one-time source of error and can be corrected by ntp today without trouble. Adjustments to the system time is something that has to be done continuously to handle for variable thermal drift over time. 2) The way PTP clocks are steered to sync with network time causes their hardware freq to actually change. Since these adjustments are done on the hardware clock level, and not on the system time level, the adjustments to sync the system time/freq would then be made incorrect by PTP hardware adjustments. Right. So use these as a way to fine tune the TSC clock (and thereby the system time). So you're then not suggesting to use the PTP as a clocksource. Using the PTP hardware to adjust the system time freq is exactly whats being proposed. 3) Further, the PTP hardware counter can be simply set to a new offset to put it in line with the network time. This could cause trouble with timekeeping much like unsynced TSCs do. You can do the same for system time. Settimeofday does allow CLOCK_REALTIME to jump, but the CLOCK_MONOTONIC time cannot jump around. Having a clocksource that is non-monotonic would break this. Now, what you seem to be suggesting is to use the TSC (or whatever clocksource the system time is using) but to steer the system time using the PTP clock. This is actually what is being proposed, however, the steering is done in userland. This is due to the fact that there are two components to the steering, 1) adjusting the PTP clock hardware to network time and 2) adjusting the system time to the PTP hardware. By exposing the PTP clock to userland via the posix clocks interface, we allow this to easily be done. Userland code would introduce latencies that would make sub microsecond time sync very difficult. The design actually avoids most userland induced latency. 1) On the PTP hardware syncing point, the reference packet gets timestamped with the PTP hardware time on arrival. This allows the offset calculation to be done in userland without introducing latency. 2) On the system syncing side, the proposal for the PPS interrupt allows the PTP hardware to trigger an interrupt on the second boundary that would take a timestamp of the system time. Then the pps interface allows for the timestamp to be read from userland allowing the offset to be calculated without introducing additional latency. We can switch underlying clocks for system time already. We can adapt to a different hw frequency. Actually no. The timekeeping code requires a fixed freq counter. Dealing with hardware freq changes is difficult, because error is introduced by the latency between when the freq changes and when the timekeeping code is notified of it. So the system treats the hardware counters as fixed freq. Now, hardware does vary freq ever so slightly as thermal conditions change, but this is addressed in userland and corrected via adjtimex. Acadmic hair splitting? I have repeatedly switched between different clocks on various systems. So its difficult but we do it? Sure, we handle the fairly-rare case of switching clocksources. And that introduces a bit of error each time. But one doesn't expect to be switching clock-sources every second and still keep synced time. Unnecessary layers? Where? This approach has less in-kernel layers, as it exposes the PTP clock to userland, instead of trying to layer things on top of it and stretching the system time abstraction to cover it. You dont need the user APIs if you directly use the PTP time source
Re: [PATCH v6 0/8] ptp: IEEE 1588 hardware clock support
On Thu, 2010-09-23 at 22:30 +0100, Alan Cox wrote: O I don't see how this is a problem, as it exposes the multiple hardware clocks via different posix clock ids. So in the boundary clock case, you can configure which side is the client and which side is the master in a config file and the PTPd will appropriately steer them individually. They may all be slaves - that means you can't treat them as part of system time. Sure, and that's something one would configure. So I'm not sure I see how exposing the different hardware bits via a clock_id is problematic. They're just clocks that are being exposed. The steering of system time to PTP or PTP to system time (or just PTP to other PTP clocks). on module unload, but I don't think we need a use-count to prevent the module from being unloaded. My question would be: How do we handle a USB network device ($14.99 now with PTP!) being unplugged? We can't say Sorry! That's in use!. So we note the hardware is gone, and return the proper error code. Or am I missing something else? Open list Oh number 31 appears to be the device I want Close list USB unplugged Random other device plugged clock_op(31, ) Oh bugger I've just reprogrammed the wrong time source. Ok. So its just the issue of clock_id reuse. I was confusing it with some sort of module use counting issue. And yea, I can see how it might be easier to re-use the file descriptor then re-implementing the reuse logic in the posix-clock registration. We don't have stop the device being removed, instead of a disaster you get clock_op(fd, blah) -ENODEV which btw is how just about everything else USB works when you pull the hardware. Right, which was what I was thinking as well, but assuming we didn't re-use clockids quickly. So, I don't really see how that's so different from what is being proposed. The clock_id is dynamically assigned per registered clock, and exposed via the sysfs interface from ptp hardware entry. The only difference is the open/close reference counting, which I don't think is necessary here (since we can't always keep the hardware from going away). It is absolutely neccessary in order that you can be sure that two calls actually relate to the *same* device. It's as fundamental as the difference betweeh chmod and fchmod although with the added ugliness of some random numeric identifier stuck in the middle. It also btw makes it much easier to fix up the existing random collection of /dev/rtc devices - because you can open them and issue fclock_adjtime if we are careful how we do it and it makes sense. Wait, you're suggesting we add new fclock_* calls that duplicate the posix interface? That doesn't sound great to me. What did you think of Kyle Moffett's suggestion of utilizing the fd to map to the clock_id which could then be used by the posix clocks interface? Although I'm still not sure if it wouldn't be so hard to just simply increment the id on each registration and index to a clock through a reasonably small hash table. I suspect that would solve the enumeration/reuse issue without much trouble (but again, I'm open to being corrected if I'm missing something larger). But yes, in summary, this is an issue to be addressed one way or another. thanks -john ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 1/5] ptp: Added a brand new class driver for ptp clocks.
On Fri, 2010-08-27 at 13:08 +0200, Richard Cochran wrote: On Mon, Aug 23, 2010 at 01:21:39PM -0700, john stultz wrote: On Thu, 2010-08-19 at 17:38 +0200, Richard Cochran wrote: On Thu, Aug 19, 2010 at 02:28:04PM +0200, Arnd Bergmann wrote: My point was that a syscall is better than an ioctl based interface here, which I definitely still think. Given that John knows much more about clocks than I do, we still need to get agreement on the question that he raised, which is whether we actually need to expose this clock to the user or not. If we can find a way to sync system time accurate enough with PTP and PPS, user applications may not need to see two separate clocks at all. At the very least, one user application (the PTPd) needs to see the PTP clock. SYSCALL_DEFINE3(clock_adjtime, const clockid_t, clkid, int, ppb, struct timespec __user *, ts) ppb - desired frequency adjustment in parts per billion ts - desired time step (or jump) in sec,nsec to correct a measured offset Arguably, this syscall might be useful for other clocks, too. This is a mix of adjtime and adjtimex with the addition of the clkid parameter, right? Sort of, but not really. ADJTIME(3) takes an offset and slowly corrects the clock using a servo in the kernel, over hours. For this function, the offset passed in the 'ts' parameter will be immediately corrected, by jumping to the new time. This reflects the way that PTP works. After the first few samples, the PTPd has an estimate of the offset to the master and the rate difference. The PTPd can proceed in one of two ways. 1. If resetting the clock is not desired, then the clock is set to the maximum adjustment (in the right direction) until the clock time is close to the master's time. 2. The estimated offset is added to the current time, resulting in a jump in time. We need clock_adjtime(id, 0, ts) for the second case. Have you considered passing a struct timex instead of ppb and ts? Yes, but the timex is not suitable, IMHO. Could you expand on this? We need to able to specify that the call is for a PTP clock. We could add that to the modes flag, like this: /*timex.h*/ #define ADJ_PTP_0 0x1 #define ADJ_PTP_1 0x2 #define ADJ_PTP_2 0x3 #define ADJ_PTP_3 0x4 I can live with this, if everyone else can, too. I wasn't suggesting adding the clock multiplexing to the timex, just using the timex to specify the adjustments in the clock_adjtime call. So I was asking why a timex was not suitable instead of using just the ppb and timespec. Could we not add a adjustment mode ADJ_SETOFFSET or something that would provide the instantaneous offset correction? Yes, but we would also need to add a struct timespec to the struct timex, in order to get nanosecond resolution. I think it would be possible to do in the padding at the end? The existing struct timeval in the timex can be also used as a timespec. NTPv4 uses it that way specifying the ADJ_NANO flag. You're right that the timex is a little crufty. But its legacy that we will support indefinitely. So following the established interface helps maintainability. We can use it for PTP, with the modifications suggested above. Or we can just introduce the clock_adjtime method, instead. Again, I think you misunderstood my suggestion. I was suggesting something like clock_adjtime(clock_id, struct timex*). So if the clock_adjtime interface is needed, it would seem best for it to be generic enough to support not only PTP, but also the NTP kernel PLL. For the proposed clock_adjime, what else is needed to support clock adjustment in general? I don't mind making the interface generic enough to support any (realistic) conceivable clock adjustment scheme, but beyond the present PTP hardware clocks, I don't know what else might be needed. I think using the timex struct covers most of the existing knowledge of what is needed. thanks -john ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 1/5] ptp: Added a brand new class driver for ptp clocks.
On Fri, 2010-08-27 at 14:03 +0200, Arnd Bergmann wrote: On Friday 27 August 2010, Richard Cochran wrote: On Mon, Aug 23, 2010 at 01:21:39PM -0700, john stultz wrote: On Thu, 2010-08-19 at 17:38 +0200, Richard Cochran wrote: On Thu, Aug 19, 2010 at 02:28:04PM +0200, Arnd Bergmann wrote: Have you considered passing a struct timex instead of ppb and ts? Yes, but the timex is not suitable, IMHO. Could you expand on this? We need to able to specify that the call is for a PTP clock. We could add that to the modes flag, like this: /*timex.h*/ #define ADJ_PTP_0 0x1 #define ADJ_PTP_1 0x2 #define ADJ_PTP_2 0x3 #define ADJ_PTP_3 0x4 I can live with this, if everyone else can, too. My suggestion was actually to have a new syscall with the existing structure, and pass a clockid_t value to it, similar to your sys_clock_adjtime(), not change the actual sys_adjtime syscall. Could we not add a adjustment mode ADJ_SETOFFSET or something that would provide the instantaneous offset correction? Yes, but we would also need to add a struct timespec to the struct timex, in order to get nanosecond resolution. I think it would be possible to do in the padding at the end? Yes, that's exactly what the padding is for. Instead of timespec, you can probably have a extra values for replacing the existing ppm values with ppb values. Right, although the ppm/ppb issue shouldn't be a problem as the timex allows for much finer then ppb resolution changes. The only adjustment to the adjtimex/timex interface that may be needed is the ability to set the time by an offset (ie: ADJ_SETOFFSET), rather then slewing the offset in (ADJ_OFFSET, or ADJ_OFFSET_SINGLESHOT). This avoids the calc offset, gettime(now), settime(now+offset) method where any latency between the gettime and settime adds to the error. thanks -john thanks -john ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 1/5] ptp: Added a brand new class driver for ptp clocks.
On Fri, 2010-08-27 at 14:38 +0200, Richard Cochran wrote: On Mon, Aug 23, 2010 at 01:08:45PM -0700, john stultz wrote: On Thu, 2010-08-19 at 07:55 +0200, Richard Cochran wrote: The clockid_t CLOCK_PTP will be arch-neutral. Sure, but are they conceptually neutral? There are other clock synchronization algorithms out there. Will they need their own similar-but-different clock_ids? Look at the other clock ids and what the represent: IMHO, the presently offered clock ids are a mixed bag... CLOCK_REALTIME : Wall time (possibly freq/offset corrected) CLOCK_MONOTONIC: Monotonic time (possibly freq corrected). CLOCK_PROCESS_CPUTIME_ID: Process cpu time. CLOCK_THREAD_CPUTIME_ID: Thread cpu time. The amount of time a thread has been granted by the kernel is really not connected to the real passage of time, at least not in a direct way. CLOCK_MONOTONIC_RAW: Non freq corrected monotonic time. This one comes from commit 2d42244ae71d6c7b0884b5664cf2eda30fb2ae68 and is surely a special case, unrelated to the other clock ids. The commit message mentions that this was added to help the btime.sf.net project. That project does not seem to have had any activity since 2007. If we can justify adding a clock id in this case, surely we can add one for PTP as well! I'm not opposed to adding a clock id, I just want it to be generic enough to make sense. So while MONOTONIC_RAW it was spurred into being from btime, it isn't a CLOCK_BTIME interface. I've also been pushing the RADClock folks to use it since it avoids the ugly raw hardware access they want to get access to a constant freq counter. In addition, I've used it to monitor freq adjustments done by NTP. CLOCK_REALTIME_COARSE: Tick granular wall time (filesystem timestamp) CLOCK_MONOTONIC_COARSE: Tick granular monotonic time. These were added in commit da15cfdae03351c689736f8d142618592e3cebc3 in order to fulfill needs of special applications. Right, but what they represent and how its different from CLOCK_REALTIME is easy to describe in a generic way. CLOCK_PTP that you're proposing doesn't seem to be at the same level of abstraction. I'm not saying that this isn't the right place for it, but can we take a step back from PTP and consider what your exposing in more generic terms. In other words, could someone use the same packet-timestamping hardware to implement a different non-PTP time synchronization algorithm? Yes, of course. There is nothing at all in the patch set about the PTP protocol itself. It just lets you access the hardware. In short: 1. SO_TIMESTAMPING delivers timestamped packets 2. the PTP API lets you tune the clock. That's all, folks. Right. So CLOCK_SO_TIMESTAMP is maybe a better description? And isn't it just an adjustment interface, not a PTP API to tune the clock? Further, if we're using PTP to synchoronize the system time, then there shouldn't be any measurable difference between CLOCK_PTP and CLOCK_REALTIME, no? When using software timestamping, then the clocks are one in the same. So this is the duplication I don't like. If the API represents CLOCK_PTP or whatever as something different from CLOCK_REALTIME, there shouldn't be a mode where they're the same. That would just cause confusion. Instead CLOCK_PTP calls just return -EINVAL and the PTPd application should use CLOCK_REALTIME. When using PTP, with the PPS hook to synchoronize the Linux system time, the clocks will be a close as the servo algorithm provides. I have not measured this yet, but it cannot be much different than using any other PPS source. SYSCALL_DEFINE3(clock_adjtime, const clockid_t, clkid, int, ppb, struct timespec __user *, ts) ppb - desired frequency adjustment in parts per billion ts - desired time step (or jump) in sec,nsec to correct a measured offset Arguably, this syscall might be useful for other clocks, too. So yea, obviously the syscall should not be CLOCK_PTP specific, so we would want it to be usable against CLOCK_REALTIME. That said, the clock_adjtime your proposing does not seem to be sufficient for usage by NTPd. So this suggests that it is not generic enough. I don't think we need to support ntpd. It already has adjtimex, and it won't get any better by using another interface. I strongly disagree. If the new interface isn't generic enough that NTPd couldn't use it to adjust CLOCK_REALTIME, then its too specific to only your needs. There will be other clock sync algorithms down the road, so if we have to expose this sort of timestamping hardware to userland, we need to allow those other sync methods to use the API you're providing, so if you're API isn't a superset of the existing adjustment needs, then its already missing something. I'm not saying we have to implement the kernel PLL adjustment and every other adjustment mode for every CLOCK_ID right now, but we should be able to extend things
Re: [PATCH 1/5] ptp: Added a brand new class driver for ptp clocks.
On Wed, 2010-08-25 at 11:40 +0200, Christian Riesch wrote: What you describe here is only one of the use cases. If the hardware has a single network port and operates as a PTP slave, it timestamps the PTP packets that are sent and received and subsequently uses these timestamps and the information it received from the master in the packets to steer its clock to align it with the master clock. In such a case the timestamping hardware and the clock hardware work together closely and it seems to be okay to use the same interface to control both the timestamping and the PTP clock. But we have to consider other use cases, e.g., 1) Boundary clocks: We have more than one network port. One port operates as a slave clock, our system gets time information via this port and steers its PTP clock to align with the master clock. The other network ports of our system operate as master clocks and redistribute the time information we got from the master to other clocks on these networks. In such a case we do timestamping on each of the network ports, but we only have a single PTP clock. Each network port's timestamping hardware uses the same hardware clock to generate time stamps. 2) Master clock: We have one or more network ports. Our system has a really good clock (ovenized quartz crystal, an atomic clock, a GPS timing receiver...) and it distributes this time on the network. In such a case we do not steer our clock based on the (packet) timestamps we get from our timestamping unit. Instead, we directly drive our clock hardware with a very stable frequency that we get from the OCXO or the atomic clock... Ok. Following you here... or we use one of the ancillary features of the PTP clock that Richard mentioned to timestamp not network packets but a 1pps signal and use these timestamps to steer the clock. Wait.. I thought we weren't using PTP to steer the clock? But now we're using the pps signal from it to do so? Do I misunderstand you? Or did you just not mean this? Packet time stamping is used to distribute the time to the slaves, but it is not part of the control loop in this case. I assume here you mean PTPd is steering the PTP clock according to the system time (which is NTP/GPS/whatever sourced)? And then the PTP clock distributes that time through the network? So in the first case we have one PTP clock but several network packet timestamping units, whereas in the second case the packet timestamping is done but it is not part of the control loop that steers the clock. Of course in most hardware implementations both the PTP clock and the timestamping unit sit on the same chip and often use the same means of communication to the cpu, e.g., the MDIO bus, but I think we need some logical separation here. So first of all, thanks for the extra explanation and context here! I really appreciate it, as I'm not familiar with all the hardware details and possible use cases, but I'm trying to learn. So in the two cases you mention, the time flow is something like: #1) [Master Clock on Network1] = [PTP Clock] = [PTPd] = [PTP Clock] = [PTP Clients on Network2] #2) [GPS] = [NTPd] = [System Time] = [PTPd] = [PTP clock] = [PTP clients on Network] And the original case: #3) [Master Clock on Network] = [PTP clock] = [PTPd] = [PTP clock] With a secondary control flow: [PPS signal from PTP clock] = [NTPd] = [System Time] Right? So, just brainstorming here, I guess the question I'm trying to figure out here, is can the System Time and PTP clock be merged/globbed into a single Time interface from the userspace point of view? In other words, if internal to the kernel, the PTP clock was always synced to the system time, couldn't the flow look something like: #3') [Master clock on network] = [PTP clock] = [PTPd] = [System Time] = [in-kernel sync thread] = [PTP clock] So PTPd sees the offset adjustment from the PTP clock, and then feeds that offset correction right into (a possibly enhanced) adjtimex. The kernel would then immediately steer the PTP clock by the same amount to keep it in sync with system time (along with a periodic offset/freq correction step to deal with crystal drift). Similarly: #2') [GPS] = [NTPd] = [System Time] = [in-kernel sync thread] = [PTP clock] = [PTP clients on Network] and #1') [Master Clock on Network1] = [PTP Clock] = [PTPd] = [System Time] = [in-kernel sync thread] = [PTP Clock] = [PTP Clients on Network2] Now, I realize PTP purists probably won't like this, because it effectively makes the in-kernel sync thread similar to a PTP boundary clock (or worse, since the control loop isn't exactly direct). But considering that the kernel (internally) allows for *very* fine-grained adjustments (we keep our long-term offset error in (nanoseconds 32) ie: ~quarter-*billion*ths of a nanosecond - I think that's sub-attosecond, if I recall the unit). And even the existing external
Re: [PATCH 1/5] ptp: Added a brand new class driver for ptp clocks.
Sorry for the slow response here, I got busy with other work at the end of last week. On Thu, 2010-08-19 at 07:55 +0200, Richard Cochran wrote: On Wed, Aug 18, 2010 at 05:12:56PM -0700, john stultz wrote: On Wed, 2010-08-18 at 09:19 +0200, Richard Cochran wrote: The timer/alarm stuff is ancillary and is not at all necessary. It is just a nice to have. I will happily remove it, if it is too troubling for people. If there's a compelling argument for it, I'm interested to hear. But again, it seems like just yet-another-way-to-get-alarm/timer-functionality, so before we add an extra API (or widen an existing API) I'd like to understand the need. We don't really need it, IMHO. But if we offer clockid_t CLOCK_PTP, then we get timer_settime() without any extra effort. Sure. There are some clear parallels and the API seems to match, but what I'm asking is: does it make sense from an overall API view that application developers have to understand? I was emulating the posix interface. Instead I should use it directly. I'm definitely interested to see what you come up with here. I'm still hesitant with adding a PTP clock_id, but extending the posix-clocks interface in this way isn't unprecedented (see: CLOCK_SGI_CYCLE) I just would like to make sure we don't end up with a clock_id namespace littered with oddball clocks that were not well abstracted (see: CLOCK_SGI_CYCLE :). For instance: imagine if instead of keeping the clocksource abstraction internal to the timekeeping core, we exposed each clocksource to userland via a clock_id. Every arch would have different ids, and each arch might have multiple ids. Programming against that would be a huge pain. The clockid_t CLOCK_PTP will be arch-neutral. Sure, but are they conceptually neutral? There are other clock synchronization algorithms out there. Will they need their own similar-but-different clock_ids? Look at the other clock ids and what the represent: CLOCK_REALTIME : Wall time (possibly freq/offset corrected) CLOCK_MONOTONIC: Monotonic time (possibly freq corrected). CLOCK_PROCESS_CPUTIME_ID: Process cpu time. CLOCK_THREAD_CPUTIME_ID: Thread cpu time. CLOCK_MONOTONIC_RAW: Non freq corrected monotonic time. CLOCK_REALTIME_COARSE: Tick granular wall time (filesystem timestamp) CLOCK_MONOTONIC_COARSE: Tick granular monotonic time. CLOCK_PTP that you're proposing doesn't seem to be at the same level of abstraction. I'm not saying that this isn't the right place for it, but can we take a step back from PTP and consider what your exposing in more generic terms. In other words, could someone use the same packet-timestamping hardware to implement a different non-PTP time synchronization algorithm? Further, if we're using PTP to synchoronize the system time, then there shouldn't be any measurable difference between CLOCK_PTP and CLOCK_REALTIME, no? So in thinking about this, try to focus on what the new clock_id provides that the other existing clockids do not? Are they at comparable levels of abstraction? 15 years from now, are folks likely to still be using it? Will it be maintainable? etc... Arnd convinced me that clockid_t=CLOCK_PTP is a good fit. My plan would be to introduce just one additional syscall: SYSCALL_DEFINE3(clock_adjtime, const clockid_t, clkid, int, ppb, struct timespec __user *, ts) ppb - desired frequency adjustment in parts per billion ts - desired time step (or jump) in sec,nsec to correct a measured offset Arguably, this syscall might be useful for other clocks, too. So yea, obviously the syscall should not be CLOCK_PTP specific, so we would want it to be usable against CLOCK_REALTIME. That said, the clock_adjtime your proposing does not seem to be sufficient for usage by NTPd. So this suggests that it is not generic enough. I think the ancillary features from PTP hardware clocks should be made available throught the sysfs. A syscall for these would end up very ugly, looking like an ioctl. Also, it is hard to see how these features relate to the more general idea of the clockid. This may be a good approach, but be aware that adding stuff to sysfs requires similar scrutiny as adding a syscall. In contrast, sysfs attributes will fit the need nicely: 1. enable or disable pps 2. enable or disable external timestamps 3. read out external timestamp 4. configure period for periodic output Things to consider here: Do having these options really make sense? Why would we want pps disabled? And if that does make sense, would it be better to do so via the existing pps interface instead of adding a new ptp specific one? Same for the timestamps and periodic output (ie: and how do they differ from reading or setting a timer on CLOCK_PTP?) 1. Use Case: SW timestamping The way I tend to see it: PTP is just one of the many ways to sync system time. 2. Use Case: HW timestamping for industrial control
Re: [PATCH 1/5] ptp: Added a brand new class driver for ptp clocks.
On Thu, 2010-08-19 at 17:38 +0200, Richard Cochran wrote: On Thu, Aug 19, 2010 at 02:28:04PM +0200, Arnd Bergmann wrote: My point was that a syscall is better than an ioctl based interface here, which I definitely still think. Given that John knows much more about clocks than I do, we still need to get agreement on the question that he raised, which is whether we actually need to expose this clock to the user or not. If we can find a way to sync system time accurate enough with PTP and PPS, user applications may not need to see two separate clocks at all. At the very least, one user application (the PTPd) needs to see the PTP clock. SYSCALL_DEFINE3(clock_adjtime, const clockid_t, clkid, int, ppb, struct timespec __user *, ts) ppb - desired frequency adjustment in parts per billion ts - desired time step (or jump) in sec,nsec to correct a measured offset Arguably, this syscall might be useful for other clocks, too. This is a mix of adjtime and adjtimex with the addition of the clkid parameter, right? Sort of, but not really. ADJTIME(3) takes an offset and slowly corrects the clock using a servo in the kernel, over hours. For this function, the offset passed in the 'ts' parameter will be immediately corrected, by jumping to the new time. This reflects the way that PTP works. After the first few samples, the PTPd has an estimate of the offset to the master and the rate difference. The PTPd can proceed in one of two ways. 1. If resetting the clock is not desired, then the clock is set to the maximum adjustment (in the right direction) until the clock time is close to the master's time. 2. The estimated offset is added to the current time, resulting in a jump in time. We need clock_adjtime(id, 0, ts) for the second case. Have you considered passing a struct timex instead of ppb and ts? Yes, but the timex is not suitable, IMHO. Could you expand on this? Could we not add a adjustment mode ADJ_SETOFFSET or something that would provide the instantaneous offset correction? Is using ppb instead of the timex ppm required to get the accuracy you want? That is one very good reason. Another is this: can you explain what the 20+ fields mean? Consider the field, freq. The comment says frequency offset (scaled ppm). To what is it scaled? The only way I know of to find out is to read the NTP code (which is fairly complex) and see what the unit really is meant to be. Ditto for the other fields. The timex structure reveals, AFAICT, the inner workings of the kernel clock servo. For PTP, we don't need or want the kernel servo. The PTPd has its own clock servo in user space. You're right that the timex is a little crufty. But its legacy that we will support indefinitely. So following the established interface helps maintainability. So if the clock_adjtime interface is needed, it would seem best for it to be generic enough to support not only PTP, but also the NTP kernel PLL. In effect, this would make clock_adjtime(or clock_adjtimex) identical to adjtimex, but only applicable to different CLOCK_ids. Additionally, it would simplify the code for the CLOCK_REALTIME case as you could just call directly into do_adjtimex(). Of course, extending adjtimex for ADJ_SETOFFSET would be needed first, but that seems like a reasonable expansion of the interface that could be used by more then just PTP. thanks -john ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 1/5] ptp: Added a brand new class driver for ptp clocks.
On Wed, 2010-08-18 at 09:19 +0200, Richard Cochran wrote: On Tue, Aug 17, 2010 at 05:22:43PM -0700, john stultz wrote: So while to me, it think it would be more ideal (or maybe just less different) to have a read-only interface (like the RTC), leaving PTPd to manage offset calculations and use that to steer the system time. I can acknowledge the need to have some way to correct the freq so the packet timestamps are corrected. The PTPd need not change the system time at all for PTP clock to be useful. (see below) Right, obviously an ok-solution is often more useful then no-solution. But that doesn't mean we shouldn't shoot for a good or even great-solution. :) I still feel a little concerned over the timer/alarm related interfaces. Could you explain why the alarm interface is necessary? The timer/alarm stuff is ancillary and is not at all necessary. It is just a nice to have. I will happily remove it, if it is too troubling for people. If there's a compelling argument for it, I'm interested to hear. But again, it seems like just yet-another-way-to-get-alarm/timer-functionality, so before we add an extra API (or widen an existing API) I'd like to understand the need. But maybe it might simplify the discussion to pull it for now, but keeping it in mind to possibly include later as an extension? So really I think my initial negative gut reaction to this was mostly out of the fact that you introduced a char dev that provides almost 100% coverage of the posix-time interface. That is duplication we definitely don't want. The reason why I modelled the char device on the posix interface was to make the API more familiar to application programmers. After the recent discussion (and having reviewed the posix clock implementation in Linux), I now think it would be even better to simply offer a new posic clock ID for PTP. I was emulating the posix interface. Instead I should use it directly. I'm definitely interested to see what you come up with here. I'm still hesitant with adding a PTP clock_id, but extending the posix-clocks interface in this way isn't unprecedented (see: CLOCK_SGI_CYCLE) I just would like to make sure we don't end up with a clock_id namespace littered with oddball clocks that were not well abstracted (see: CLOCK_SGI_CYCLE :). For instance: imagine if instead of keeping the clocksource abstraction internal to the timekeeping core, we exposed each clocksource to userland via a clock_id. Every arch would have different ids, and each arch might have multiple ids. Programming against that would be a huge pain. So in thinking about this, try to focus on what the new clock_id provides that the other existing clockids do not? Are they at comparable levels of abstraction? 15 years from now, are folks likely to still be using it? Will it be maintainable? etc... Also I think the documentation I've read about PTP (likely just due to the engineering focus) has an odd inverted sense of priority, focusing on keeping obscure hardware clocks on NIC cards in sync, rather then the the more tangible feature of keeping the system time in sync. This could be comically interpreted as trying to create a shadow-time on the system that is the real time and yea, maybe we'll let the system know what time it is, but user-apps who want to know the score can send a magic ioctl to /dev/something and get the real deal. ;) I'm sure that's not the case, but I'd like to keep any confusion in userland about which time is the best time to a minimum (ie: use the system time). You are right. As John Eidson's excellent book points out, modern computers and operating systems provide surprisingly little support for programming based on absolute time. It is not PTP's fault. PTP is actually a step in the right direction, but it doesn't yet really fit in to the present computing world. You'll have to forgive me, as I haven't had the time to check out that book. What exactly do you mean by operating systems provide little support for programming based on absolute time? Okay, here is the Big Picture. 1. Use Case: SW timestamping PTP with software timestamping (ie without special hardware) can acheive synchronization within a few dozen microseconds, after about twenty minutes. This is sufficient for very many people. The new API (whether char device or syscall) fully and simply supports this use case. When the PTPd adjusts the PTP clock, it is actually adjusting the system time, just like NTPd. Again this illustrates the inversion of focus: system time is merely one of many possible PTP clocks in the larger PTP framework. The way I tend to see it: PTP is just one of the many ways to sync system time. 2. Use Case: HW timestamping for industrial control PTP with hardware timestamping can acheive synchronization within 100 nanoseconds after one minute. If you want to do something with your wonderfully
Re: [PATCH 1/5] ptp: Added a brand new class driver for ptp clocks.
On Tue, 2010-08-17 at 10:53 +0200, Richard Cochran wrote: On Mon, Aug 16, 2010 at 12:24:48PM -0700, john stultz wrote: 3) I'm not sure I see the benefit of being able to have multiple frequency corrected time domains. In other words, what benefit would you get from adjusting a PTP clock's frequency instead of just adjusting the system's time freq? Having the PTP time as a reference to correct the system time seems reasonable, but I'm not sure I see why userland would want to adjust the PTP clock's freq. For PTP enabled hardware, the timestamp on the network packet comes from from the PTP clock, not from the system time. Of course, you can always just leave the PTP clock alone, figure the needed correction, and apply it whenever needed. But this has some disadvantages. First of all, the (one and only) open source PTPd does not do it that way. Also, only one program (the PTPd or equivalent) will know the correct time. Other programs will not be able to ask the operating system for time services. Instead, they would need to use IPC to the PTPd. Why would system time not be adjusted to the PTP time? This is my main concern, that we're presenting a fractured API to userland. Suddenly there isn't just system time, but ptp time as well, and possibly multiple different ptp times. Forgive me, as I suspect I'm missing something key here. The PTP protocol (and some PTP hardware) offers a one step method, where the timestamps are inserted by the hardware on the fly. Here you really do need the PTP clock to be correctly adjusted. All of the PTP hardware that I am familiar with provides a clock adjustment method, so it simpler and cleaner just to use this facility to tune the PTP clock. Hmm. So trying to recap here to see if I'm understanding properly: The PTP clock is a bit of hardware (usually on the NIC) that can put timestamps on packets (both incoming or outgoing?). The need to offset/freq correct the PTP clock is important so that the timestamps (incoming and outgoing) are correct, which makes future offset calculations simpler. Hmm. So I'm actually starting to come around to the chardev interface. In a way, it seems it has some similarities to how the RTC device is used in NTPd. Granted, NTPd doesn't correct the RTC (the kernel does when we're synced, but its not a perfect parallel), but it can be used as an input the steer the clock. So while to me, it think it would be more ideal (or maybe just less different) to have a read-only interface (like the RTC), leaving PTPd to manage offset calculations and use that to steer the system time. I can acknowledge the need to have some way to correct the freq so the packet timestamps are corrected. I still feel a little concerned over the timer/alarm related interfaces. Could you explain why the alarm interface is necessary? So really I think my initial negative gut reaction to this was mostly out of the fact that you introduced a char dev that provides almost 100% coverage of the posix-time interface. That is duplication we definitely don't want. Also I think the documentation I've read about PTP (likely just due to the engineering focus) has an odd inverted sense of priority, focusing on keeping obscure hardware clocks on NIC cards in sync, rather then the the more tangible feature of keeping the system time in sync. This could be comically interpreted as trying to create a shadow-time on the system that is the real time and yea, maybe we'll let the system know what time it is, but user-apps who want to know the score can send a magic ioctl to /dev/something and get the real deal. ;) I'm sure that's not the case, but I'd like to keep any confusion in userland about which time is the best time to a minimum (ie: use the system time). thanks -john ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 1/5] ptp: Added a brand new class driver for ptp clocks.
On Tue, 2010-08-17 at 13:36 +0200, Arnd Bergmann wrote: On Tuesday 17 August 2010, Richard Cochran wrote: On Tue, Aug 17, 2010 at 09:25:55AM +, Arnd Bergmann wrote: Another difference is that we generally use ioctl for devices that can be enumerated, while syscalls are for system services that are not tied to a specific device. This argument works both ways for PTP IMHO: On the one hand you want to have a reliable clock that you can use without knowing where it comes from, on the other you might have multiple PTP sources that you need to differentiate. Yes, I agree. In normal use, there will be only one PTP clock in a system. However, for research purposes, it would be nice to have more than one. I've been looking at offering the PTP clock as a posix clock, and it is not as hard as I first thought. The PTP clock or clocks just have to be registered as one of the posix_clocks[MAX_CLOCKS] in posix-timers.c. Ok sounds good. Oh no. I'm starting to waffle here. :) So while I'm not a fan of the duplication of the posix clocks interface that the proposed chardev interface introduced, I'm not sure if absorbing the PTP clocks as a clockid is much better. Mainly due to the fact that userland apps now have to chose between two clockids that supposedly represent the same thing (the current wall time). My suggestion would be to reserve three clock ids in time.h, CLOCK_PTP0, CLOCK_PTP1, and CLOCK_PTP2. The first one would be the same as CLOCK_REALTIME, for SW timestamping, and the other two would allow two different PTP clocks at the same time, for the research use case. Why would you have CLOCK_PTP0 == CLOCK_REALTIME? Whats the point of that? I don't think there is a point in making exactly two independent sources available. The clockid_t space is not really limited, so we could define an arbitrary range of ids for ptp sources that could be used simultaneously, as long as we have space more more ids with a fixed number. Would it be reasonable to assume that on a machine with a large number of NICs, you'd want a separate ptp source for each of them for timestamping? Or would you preferably define just one source in such a setup? I think both could be done with the use of class device attributes in sysfs for configuration. Maybe you can have one CLOCK_PTP value for one global PTP source and use sysfs to configure which device that is. If you also need simultaneous access to the specific clocks, you could have run-time configured clockid numbers in a sysfs attribute for each ptp class device. Using the clock id will bring another advantage, since it will then be possible for user space to specify the desired timestamp source for SO_TIMESTAMPING. So how exactly would one map CLOCK_PTPx to an eth device? So this is a little bit further out there, but assuming PTPd can sync the PTP clock correctly, why could the kernel itself not sync the PTP clock to system time? So instead of the sync path looking like: 1) packet from master arrives on NIC, is timestamped by PTP clock 2) PTPd calculates offset between PTP clock and master time 3) PTPd corrects PTP clock freq/offset 4) PTPd corrects system time freq/offset It would look like: 1) packet from master arrives on NIC, is timestamped by PTP clock 2) PTPd calculates offset between PTP clock and master time 3) PTPd corrects system time freq/offset 4) kernel corrects PTP clock freq/offset I'm guessing the indirect PTP clock sync would have greater error, but it avoids the fractured sense of time. thanks -john ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 1/5] ptp: Added a brand new class driver for ptp clocks.
On Mon, Aug 16, 2010 at 4:17 AM, Richard Cochran richardcoch...@gmail.com wrote: This patch adds an infrastructure for hardware clocks that implement IEEE 1588, the Precision Time Protocol (PTP). A class driver offers a registration method to particular hardware clock drivers. Each clock is exposed to user space as a character device with ioctls that allow tuning of the PTP clock. Signed-off-by: Richard Cochran richard.coch...@omicron.at Hey Richard! Its very cool to see this work on lkml! I'm excited to see more work done on ptp. We had a short private thread discussion earlier (I got busy and never replied to your last message, my apologies!), but I wanted to bring up the concerns I have here as well. A few comments below +** PTP user space API + + The class driver creates a character device for each registered PTP + clock. User space programs may control the clock using standardized + ioctls. A program may query, enable, configure, and disable the + ancillary clock features. User space can receive time stamped + events via blocking read() and poll(). One shot and periodic + signals may be configured via an ioctl API with semantics similar + to the POSIX timer_settime() system call. As I mentioned earlier, I'm not a huge fan of the char device interface for abstracted PTP clocks. If it was just the direct hardware access, similar to RTC, which user apps then use as a timesource, I'd not have much of a problem. But as I mentioned in an earlier private mail, the abstraction level concerns me. 1) The driver-like model exposes a char dev for each clock, which allows for poorly-written userland applications to hit portability issues (ie: /dev/hpet vs /dev/rtc). Granted this isn't a huge flaw, but good APIs should be hard to get wrong. 2) As Arnd already mentioned, the chardev interface seems to duplicate the clock_gettime/settime() and adjtimex() interfaces. 3) I'm not sure I see the benefit of being able to have multiple frequency corrected time domains. In other words, what benefit would you get from adjusting a PTP clock's frequency instead of just adjusting the system's time freq? Having the PTP time as a reference to correct the system time seems reasonable, but I'm not sure I see why userland would want to adjust the PTP clock's freq. thanks -john ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 1/5] ptp: Added a brand new class driver for ptp clocks.
On Mon, Aug 16, 2010 at 12:24 PM, john stultz johns...@us.ibm.com wrote: On Mon, Aug 16, 2010 at 4:17 AM, Richard Cochran A few comments below +** PTP user space API + + The class driver creates a character device for each registered PTP + clock. User space programs may control the clock using standardized + ioctls. A program may query, enable, configure, and disable the + ancillary clock features. User space can receive time stamped + events via blocking read() and poll(). One shot and periodic + signals may be configured via an ioctl API with semantics similar + to the POSIX timer_settime() system call. As I mentioned earlier, I'm not a huge fan of the char device interface for abstracted PTP clocks. If it was just the direct hardware access, similar to RTC, which user apps then use as a timesource, I'd not have much of a problem. But as I mentioned in an earlier private mail, the abstraction level concerns me. [snip] 2) As Arnd already mentioned, the chardev interface seems to duplicate the clock_gettime/settime() and adjtimex() interfaces. And maybe just to clarify, as I saw your response to Arnd, I'm not suggesting using PTP clocks as clocksources for the internal timekeeping core. Instead I'm trying to understand why PTP clocks need the equivalent of the existing posix clocks/timer interface. Why would only having a read-time interface not suffice? thanks -john ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 04/11] powerpc: Simplify update_vsyscall
On Wed, 2010-07-28 at 09:41 +1000, Paul Mackerras wrote: On Tue, Jul 13, 2010 at 05:56:21PM -0700, John Stultz wrote: Currently powerpc's update_vsyscall calls an inline update_gtod. However, both are straightforward, and there are no other users, so this patch merges update_gtod into update_vsyscall. Compiles, but otherwise untested. This and the following two patches will cause interesting conflicts with two commits in Ben Herrenschmidt's powerpc.git next branch, specifically 8fd63a9e (powerpc: Rework VDSO gettimeofday to prevent time going backwards) and c1aa687d (powerpc: Clean up obsolete code relating to decrementer and timebase) from me. In fact the first of those two commits includes changes equivalent to those in your 5/11 patch (powerpc: Cleanup xtime usage), as far as I can see. Ahh.. Right.. I guess I should have remembered you were working on those changes (even though I don't think I saw the final results sent to lkml or anything). Sorry about that. BTW, BenH's tree is at: git://git.kernel.org/pub/scm/linux/kernel/git/benh/powerpc.git So I've cherry picked the two changes from the ppc tree, applied them onto linus' git tree and then rebased my changes ontop of them. The net of the change to the patch set: Added to the head of the patch queue: powerpc: Rework VDSO gettimeofday to prevent time going backwards powerpc: Clean up obsolete code relating to decrementer and timebase Modified to resolve collision: powerpc: Simplify update_vsyscall Dropped (as earlier patches already made equivalent changes): powerpc: Cleanup xtime usage The full set is in the attached tarball. Thomas, would you consider re-adding these? Hopefully that will avoid any -next collisions. thanks -john patches.tar.bz2 Description: application/bzip-compressed-tar ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/5] Implement generic time of day clocksource for powerpc machines.
On Thu, 2007-09-20 at 10:52 +1000, Paul Mackerras wrote: Daniel Walker writes: If you switch to the rtc do the shift and mult need to change? You can't switch; any given CPU chip will have either the RTC or the timebase but not both. I think what Daniel is pointing out is that the clocksource read function isn't the place for the __USE_RTC() conditional. It would likely be better instead of the timebase clocksource managing multiple type of hardware (timebase and RTC), to have a separate simple RTC clocksource, and then conditionally register one or the other at init time. thanks -john ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
