Re: [PATCH] x86/timer: Fix boot on Intel systems using ITSSPRC static PIT clock gating

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 07/01/2021 13:53, Jan Beulich wrote:
>> +         !((hpet_cfg = hpet_read32(HPET_CFG)) & HPET_CFG_LEGACY) )
>> +    {
>> +        unsigned int c0_cfg, ticks, count;
>> +
>> +        /* Stop the main counter. */
>> +        hpet_write32(hpet_cfg & ~HPET_CFG_ENABLE, HPET_CFG);
>> +
>> +        /* Reconfigure channel 0 to be 32bit periodic. */
>> +        c0_cfg = hpet_read32(HPET_Tn_CFG(0));
>> +        c0_cfg |= (HPET_TN_ENABLE | HPET_TN_PERIODIC | HPET_TN_SETVAL |
>> +                   HPET_TN_32BIT);
>> +        hpet_write32(c0_cfg, HPET_Tn_CFG(0));
>> +
>> +        /*
>> +         * The exact period doesn't have to match a legacy PIT.  All we need
>> +         * is an interrupt queued up via the IO-APIC to check routing.
>> +         *
>> +         * Use HZ as the frequency.
>> +         */
>> +        ticks = (SECONDS(1) / HZ) * div_sc(hpet_rate, SECONDS(1), 32)) >> 32;
>> +
>> +        count = hpet_read32(HPET_COUNTER);
>> +
>> +        /*
>> +         * HPET_TN_SETVAL above is atrociously documented in the spec.
>> +         *
>> +         * Periodic HPET channels have a main comparator register, and cache
>> +         * the "last write to cmp", as a hidden register.
>> +         *
>> +         * The semantics on generating a periodic interrupt is:
>> +         *   cmp += "last value written to the cmp register"
>> +         * which will reload a new period.
>> +         *
>> +         * Normally, writes to cmp update the main comparator as well as being
>> +         * cached for the reload value.  However, under these semantics, the
>> +         * HPET main counter needs resetting to 0 to be able to configure the
>> +         * period correctly.
>> +         *
>> +         * Instead, HPET_TN_SETVAL is a self-clearing control bit which we can
>> +         * use for periodic timers to mean that the second write to the
>> +         * comparator updates only the "last written" cache, and not the
>> +         * absolute comparator value.
>> +         *
>> +         * This lets us set a period when the main counter isn't at 0.
>> +         */
>> +        hpet_write32(count + ticks, HPET_Tn_CMP(0));
>> +        hpet_write32(ticks,         HPET_Tn_CMP(0));
> As you say, documentation is poor here. In fact the wording in the
> HPET spec talks about updating of the "accumulator" instead;
> perhaps just an unhelpful use of a different term. (Respective
> Linux code has a comment indicating this is needed on a specific
> AMD chipset only.)

I'm fairly certain that Linux's comment is wrong.  This behaviour is
described in the HPET spec, which is an Intel spec.

It smells very much like a bug discovered during software bringup on
early alpha platforms with an HPET, and fixed at v0.9 of the spec (or
thereabouts).  I can entirely believe that it is the kind of thing which
would have been fixed in beta silicon before the spec was formally updated.

> It would seem more natural to me if things were explained a little
> differently: Writes to the comparator with SETVAL clear always
> update the "last written" value, i.e. the increment to be used
> once the main counter equals the comparator. Writes with SETVAL set
> update the comparator itself. (Assuming that's how it really is, of
> course, but that's what I infer from the doc available.)

By default, all writes update both the accumulator and comparator registers.

Except for the 2nd write of CMP following a write of SETVAL, where only
the accumulator is updated, and the comparator retains its old value.


I can only guess at the horrors for the internals of the HPET to make
this work.

SETVAL is RAZ so can't be observed in the CFG register.  To get the
observed semantics, every write to CMG will have to copy the SEVAL bit
from CFG to a local flipflop, and on the falling edge of bit, forgo the
comparator update.

This also smells of being a "least invasive fix" at a late point in
development.

> Linux additionally puts udelay(1) between the two writes. Do you
> think we're fine without such, on all platforms?

HPETs substantially predate 64bit capable systems.

There is no spec requirement for a pause, and there is a good chance
that implementation bugs like that were shaken out back in the 32bit days.

>> +        /* Restart the main counter, and legacy mode. */
>> +        hpet_write32(hpet_cfg | HPET_CFG_ENABLE | HPET_CFG_LEGACY, HPET_CFG);
> This isn't necessarily "restart" - you may start the counter for
> the first time. Hence in the comment maybe "(Re)start ..."?

Well - it is strictly a restart because of how the logic is laid out,
and even if that weren't the case, "restart" is fine to use in this context.

> As to the spurious IRQs, does it perhaps matter at which point
> CFG_LEGACY gets set? We could try setting it when clearing
> CFG_ENABLE further up, or we could also try two separate writes
> each setting just one of the bits. (At least I can't deduce
> from the spec that we ought to be prepared to observe spurious
> IRQs here.)

It will an (extra) electrical load stabilising on the line into the
PIC.  Repositioning control writes of the HPET won't make a difference.

~Andrew