Re: [RFC PATCH v2 09/14] m68k: hp300: Remove hp300_gettimeoffset()
On Sun, 25 Nov 2018, Michael Schmitz wrote: > > We can benchmark gettimeofday syscalls on elgar but is that hardware > > representative of other relevant models? > > I suppose the CIA is on the main board, so running with the slower clock > speed that you'd see with a vanilla Amiga without 060 accelerator board. > Ought to be representative enough? > Not really. An accelerated CPU clock exaggerates the slowdown factor, given the fixed 0.7 MHz CIA clock. The "(ticks > jiffy_ticks / 2)" conditional won't make anything worse even if interrupt latency is already too high, so I'm inclined towards the more prudent option. This patch series is already complicated enough. In anycase, I'd prefer not to speculate about interrupt priorities or latencies when the list probably has people who know the answers. --
Re: [RFC PATCH v2 09/14] m68k: hp300: Remove hp300_gettimeoffset()
Hi Finn, Am 25.11.2018 um 14:15 schrieb Finn Thain: Maybe the timer interrupt has a sufficiently high priority and latency is low? Maybe cia_set_irq() is really expensive? I don't know the platform well enough so I'm inclined to revert. We can benchmark gettimeofday syscalls on elgar but is that hardware representative of other relevant models? I suppose the CIA is on the main board, so running with the slower clock speed that you'd see with a vanilla Amiga without 060 accelerator board. Ought to be representative enough? Adrian has a few other Amigas with different hardware base, so we might be able to get test coverage on more than one model. Cheers, Michael [1] https://github.com/mamedev/mame/commit/e2ed0490520f538c346c8bdaa9084bcbc43427cb [2] http://vice-emu.sourceforge.net/vice_9.html [3] https://www.commodore.ca/manuals/funet/cbm/documents/chipdata/cia6526.zip
Re: [RFC PATCH v2 09/14] m68k: hp300: Remove hp300_gettimeoffset()
On Wed, 21 Nov 2018, I wrote: > On Wed, 21 Nov 2018, Geert Uytterhoeven wrote: > > > > This suggests that either 0 or N (the latched value) would result > > > from a read from the counter immediately following an interrupt. Who > > > can say which? Just have to try it. The answer should allow us to > > > avoid the risk of a clocksource that jumps forwards and backwards. > > > > The code in amiga_gettimeoffset() does: > > > > ticks = hi << 8 | lo; > > > > if (ticks > jiffy_ticks / 2) > > /* check for pending interrupt */ > > if (cia_set_irq(&ciab_base, 0) & CIA_ICR_TA) > > offset = 1; > > > > That _suggests_ that there's no interrupt when ticks == 0. > > But look what happens next: > > > ticks = jiffy_ticks - ticks; > > > > ticks = (1 * ticks) / jiffy_ticks; > > > > return (ticks + offset) * 1000; > > If (hi << 8 | lo) == 0, and you set offset = 1, then the return > value would be maximal. > > Let's immediately call this function again. This time (hi << 8 | lo) == > N. Let's add the offset again. I'm afraid the clock just jumped > backwards. > > So the logic you quoted has a rationale which is unrelated to the > question. > I've learned that emulators like MAME [1] and VICE [2] have used a reverse-engineered description of the CIA called "A Software Model of the CIA6526" by Wolfgang Lorenz and an accompanying test suite [3]. That document says, "When the counter has reached zero, it is reloaded from the latch as soon as there is another clock waiting in the pipeline. In phase 2 mode, this is always the case. This explains why you are reading zeros in cascaded mode only (2-2-2-1-1-1-0-0-2) but not in phase 2 mode (2-1-2)." I think this is a good argument that a zero count will never be observed by reading the counter register. Hence, it seems that this conditional in the v3 patch, if (msb > 0) is redundant and should be removed. It could be reverted to, if (ticks > jiffy_ticks / 2) which is intended to reduce the number of calls to cia_set_irq() but assumes low interrupt latency (below 5 ms). Maybe the timer interrupt has a sufficiently high priority and latency is low? Maybe cia_set_irq() is really expensive? I don't know the platform well enough so I'm inclined to revert. We can benchmark gettimeofday syscalls on elgar but is that hardware representative of other relevant models? [1] https://github.com/mamedev/mame/commit/e2ed0490520f538c346c8bdaa9084bcbc43427cb [2] http://vice-emu.sourceforge.net/vice_9.html [3] https://www.commodore.ca/manuals/funet/cbm/documents/chipdata/cia6526.zip --
Re: [RFC PATCH v2 09/14] m68k: hp300: Remove hp300_gettimeoffset()
On Wed, 21 Nov 2018, Geert Uytterhoeven wrote: > > This suggests that either 0 or N (the latched value) would result from > > a read from the counter immediately following an interrupt. Who can > > say which? Just have to try it. The answer should allow us to avoid > > the risk of a clocksource that jumps forwards and backwards. > > The code in amiga_gettimeoffset() does: > > ticks = hi << 8 | lo; > > if (ticks > jiffy_ticks / 2) > /* check for pending interrupt */ > if (cia_set_irq(&ciab_base, 0) & CIA_ICR_TA) > offset = 1; > That _suggests_ that there's no interrupt when ticks == 0. But look what happens next: > ticks = jiffy_ticks - ticks; > > ticks = (1 * ticks) / jiffy_ticks; > > return (ticks + offset) * 1000; If (hi << 8 | lo) == 0, and you set offset = 1, then the return value would be maximal. Let's immediately call this function again. This time (hi << 8 | lo) == N. Let's add the offset again. I'm afraid the clock just jumped backwards. So the logic you quoted has a rationale which is unrelated to the question. --
Re: [RFC PATCH v2 09/14] m68k: hp300: Remove hp300_gettimeoffset()
Hi Finn,
On Wed, Nov 21, 2018 at 10:47 AM Finn Thain wrote:
> On Wed, 21 Nov 2018, Geert Uytterhoeven wrote:
> > The 8520 CIA is almost identical to the 6526 CIA, as used in the C64...
>
> The 6526 CIA datasheet says, "In continuous mode, the timer will count
> from the latched value to zero, generate and interrupt, reload the latched
> value and repeat the procedure continuously."
I stand corrected...
> This suggests that either 0 or N (the latched value) would result from a
> read from the counter immediately following an interrupt. Who can say
> which? Just have to try it. The answer should allow us to avoid the risk
> of a clocksource that jumps forwards and backwards.
The code in amiga_gettimeoffset() does:
ticks = hi << 8 | lo;
if (ticks > jiffy_ticks / 2)
/* check for pending interrupt */
if (cia_set_irq(&ciab_base, 0) & CIA_ICR_TA)
offset = 1;
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [RFC PATCH v2 09/14] m68k: hp300: Remove hp300_gettimeoffset()
On Wed, 21 Nov 2018, Geert Uytterhoeven wrote: > The 8520 CIA is almost identical to the 6526 CIA, as used in the C64... > The 6526 CIA datasheet says, "In continuous mode, the timer will count from the latched value to zero, generate and interrupt, reload the latched value and repeat the procedure continuously." This suggests that either 0 or N (the latched value) would result from a read from the counter immediately following an interrupt. Who can say which? Just have to try it. The answer should allow us to avoid the risk of a clocksource that jumps forwards and backwards. --
Re: [RFC PATCH v2 09/14] m68k: hp300: Remove hp300_gettimeoffset()
Hi Finn,
On Wed, Nov 21, 2018 at 9:41 AM Finn Thain wrote:
> On Wed, 21 Nov 2018, Geert Uytterhoeven wrote:
> > On Wed, Nov 21, 2018 at 12:13 AM Finn Thain
> > wrote:
> > > On atari, the 68901 counts down to 0x01 and raises an interrupt. On
> > > mac, the 6522 counts down to 0x then raises an interrupt. No idea
> > > about amiga (Geert?) -- this has to be handled correctly to get a
> > > monotonic clocksource. I'll fix this in v3 (where the information is
> > > available).
> >
> > The docs state that the CIA generates on interrupt on underflow, so I
> > guess that's the same behavior as the 6522 VIA.
>
> Difficult to say. The sequence varies from one implementation to another.
> Let's ignore the MSB and LSB and pretend it's one register:
>
> MC68901: N, N-1, N-2, ..., 2, 1, N, N-1, N-2, ...
> MC6840: N, N-1, N-2, ..., 2, 1, 0, N, N-1, N-2, ...
> SY6522: N, N-1, N-2, ..., 2, 1, 0, 0x, N, N-1, N-2, ...
>
> Now the question is, when the timer asserts its interrupt, and the count
> register is fetched immediately, what value does it have?
>
> For the MC68901, you get 1. For the SY6522, you get 0x. For MC6840, as
> far as I can tell, you'd get 0.
I assume 0x. The 8520 CIA is almost identical to the 6526 CIA, as used in
the C64, which is a direct descendant of the 6522 VIA.
> > Unfortunately the 24-bit ("TOD") counters in the two CIAs run from HSYNC
> > resp. VSYNC, which depends on the video mode, and thus can't be used as
> > a monotonic clock source.
>
> Is that because of video mode changes? Could the clocksource be
> unregistered before the mode change and then re-registered at a different
> frequency afterwards?
On older Amigas (most plain 68000, unless expanded), video modes are fixed.
Some have a jumper to select the power supply tick instead of the VSYNC
signal, which is more accurate, but still runs at a low 50 or 60 Hz.
On anything more modern (A3000 and up), video modes are programmable.
In addition, blanking the screen according to VESA will disable sync signals,
thus stopping the clock, I assume.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [RFC PATCH v2 09/14] m68k: hp300: Remove hp300_gettimeoffset()
Op wo 21 nov. 2018 om 00:13 schreef Finn Thain : > > On Tue, 20 Nov 2018, Kars de Jong wrote: > > > Op ma 19 nov. 2018 om 02:10 schreef Finn Thain : > > > > > > hp300_gettimeoffset() never checks the timer interrupt flag and will > > > fail to notice when the timer counter gets reloaded. That means the > > > clock could jump backwards. > > > > > > Remove this code and leave this platform on the 'jiffies' clocksource. > > > Note that this amounts to a regression in clock precision. However, > > > adopting the 'jiffies' clocksource does resolve the monotonicity issue. > > > > > > Signed-off-by: Finn Thain > > > --- > > > hp300_gettimeoffset() cannot be used in a clocksource conversion > > > unless it can be made monotonic. I can't fix this without knowing the > > > details of the timer implementation, such as the relationship between > > > the timer count and the interrupt flag. > > > > I don't really like this regression... > > > > Me neither... > > I'll see if I can write a conversion patch based on the information you've > provided. Can you test it? I can try... It's been a while since I booted the machine to Linux though (NFS support only). MAME is also starting to support it, but not quite there yet :-) > > According to NetBSD sources, there are 3 timers in the chip (originally > > an MC6840 PTM). > > Thanks for the tip. I will examine the datasheet for the 6840. > > I'll also take another look at the NetBSD code. > > > Timer 1 is used as the system timer, timer 3 runs at the same rate and > > is unused on Linux (on NetBSD it is used as the statistics/profiling > > timer), and timer 3 is connected to timer 2 so you can make a 32-bit > > timer out of the two timers together (also unused on Linux). > > > > Timers 1 counts down at 25 MHz. > > You mean, 250 kHz, right? The code in mainline programs the timer for 2500 > cycles, hoping to get 10 ms. That is, 250 cycles per ms. Eh, yes, that makes a lot more sense. > > The interrupt flag is set when the counter reaches 0 after which it is > > automatically reloaded and starts counting down again. > > > > Thanks. > > On atari, the 68901 counts down to 0x01 and raises an interrupt. On mac, > the 6522 counts down to 0x then raises an interrupt. No idea about > amiga (Geert?) -- this has to be handled correctly to get a monotonic > clocksource. I'll fix this in v3 (where the information is available). Cool! Kars.
Re: [RFC PATCH v2 09/14] m68k: hp300: Remove hp300_gettimeoffset()
On Wed, 21 Nov 2018, Geert Uytterhoeven wrote:
> Hi Finn,
>
> On Wed, Nov 21, 2018 at 12:13 AM Finn Thain
> wrote:
> > On atari, the 68901 counts down to 0x01 and raises an interrupt. On
> > mac, the 6522 counts down to 0x then raises an interrupt. No idea
> > about amiga (Geert?) -- this has to be handled correctly to get a
> > monotonic clocksource. I'll fix this in v3 (where the information is
> > available).
>
> The docs state that the CIA generates on interrupt on underflow, so I
> guess that's the same behavior as the 6522 VIA.
>
Difficult to say. The sequence varies from one implementation to another.
Let's ignore the MSB and LSB and pretend it's one register:
MC68901: N, N-1, N-2, ..., 2, 1, N, N-1, N-2, ...
MC6840: N, N-1, N-2, ..., 2, 1, 0, N, N-1, N-2, ...
SY6522: N, N-1, N-2, ..., 2, 1, 0, 0x, N, N-1, N-2, ...
Now the question is, when the timer asserts its interrupt, and the count
register is fetched immediately, what value does it have?
For the MC68901, you get 1. For the SY6522, you get 0x. For MC6840, as
far as I can tell, you'd get 0.
I'll add some code to my github repo to find out what happens with CIA.
> Unfortunately the 24-bit ("TOD") counters in the two CIAs run from HSYNC
> resp. VSYNC, which depends on the video mode, and thus can't be used as
> a monotonic clock source.
>
Is that because of video mode changes? Could the clocksource be
unregistered before the mode change and then re-registered at a different
frequency afterwards?
--
> Gr{oetje,eeting}s,
>
> Geert
>
>
Re: [RFC PATCH v2 09/14] m68k: hp300: Remove hp300_gettimeoffset()
Hi Finn,
On Wed, Nov 21, 2018 at 12:13 AM Finn Thain wrote:
> On atari, the 68901 counts down to 0x01 and raises an interrupt. On mac,
> the 6522 counts down to 0x then raises an interrupt. No idea about
> amiga (Geert?) -- this has to be handled correctly to get a monotonic
> clocksource. I'll fix this in v3 (where the information is available).
The docs state that the CIA generates on interrupt on underflow, so I
guess that's the same behavior as the 6522 VIA.
Unfortunately the 24-bit ("TOD") counters in the two CIAs run from HSYNC
resp. VSYNC, which depends on the video mode, and thus can't be used
as a monotonic clock source.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [RFC PATCH v2 09/14] m68k: hp300: Remove hp300_gettimeoffset()
On Tue, 20 Nov 2018, Kars de Jong wrote: > Op ma 19 nov. 2018 om 02:10 schreef Finn Thain : > > > > hp300_gettimeoffset() never checks the timer interrupt flag and will > > fail to notice when the timer counter gets reloaded. That means the > > clock could jump backwards. > > > > Remove this code and leave this platform on the 'jiffies' clocksource. > > Note that this amounts to a regression in clock precision. However, > > adopting the 'jiffies' clocksource does resolve the monotonicity issue. > > > > Signed-off-by: Finn Thain > > --- > > hp300_gettimeoffset() cannot be used in a clocksource conversion > > unless it can be made monotonic. I can't fix this without knowing the > > details of the timer implementation, such as the relationship between > > the timer count and the interrupt flag. > > I don't really like this regression... > Me neither... I'll see if I can write a conversion patch based on the information you've provided. Can you test it? > According to NetBSD sources, there are 3 timers in the chip (originally > an MC6840 PTM). Thanks for the tip. I will examine the datasheet for the 6840. I'll also take another look at the NetBSD code. > Timer 1 is used as the system timer, timer 3 runs at the same rate and > is unused on Linux (on NetBSD it is used as the statistics/profiling > timer), and timer 3 is connected to timer 2 so you can make a 32-bit > timer out of the two timers together (also unused on Linux). > > Timers 1 counts down at 25 MHz. You mean, 250 kHz, right? The code in mainline programs the timer for 2500 cycles, hoping to get 10 ms. That is, 250 cycles per ms. > The interrupt flag is set when the counter reaches 0 after which it is > automatically reloaded and starts counting down again. > Thanks. On atari, the 68901 counts down to 0x01 and raises an interrupt. On mac, the 6522 counts down to 0x then raises an interrupt. No idea about amiga (Geert?) -- this has to be handled correctly to get a monotonic clocksource. I'll fix this in v3 (where the information is available). -- > > --- > > arch/m68k/hp300/time.c | 19 --- > > 1 file changed, 19 deletions(-) >
Re: [RFC PATCH v2 09/14] m68k: hp300: Remove hp300_gettimeoffset()
Op ma 19 nov. 2018 om 02:10 schreef Finn Thain : > > hp300_gettimeoffset() never checks the timer interrupt flag and will > fail to notice when the timer counter gets reloaded. That means the > clock could jump backwards. > > Remove this code and leave this platform on the 'jiffies' clocksource. > Note that this amounts to a regression in clock precision. However, > adopting the 'jiffies' clocksource does resolve the monotonicity issue. > > Signed-off-by: Finn Thain > --- > hp300_gettimeoffset() cannot be used in a clocksource conversion > unless it can be made monotonic. I can't fix this without knowing the > details of the timer implementation, such as the relationship between > the timer count and the interrupt flag. I don't really like this regression... According to NetBSD sources, there are 3 timers in the chip (originally an MC6840 PTM). Timer 1 is used as the system timer, timer 3 runs at the same rate and is unused on Linux (on NetBSD it is used as the statistics/profiling timer), and timer 3 is connected to timer 2 so you can make a 32-bit timer out of the two timers together (also unused on Linux). Timers 1 counts down at 25 MHz. The interrupt flag is set when the counter reaches 0 after which it is automatically reloaded and starts counting down again. > --- > arch/m68k/hp300/time.c | 19 --- > 1 file changed, 19 deletions(-)
