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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.01.2021 18:31, Andrew Cooper wrote:
> 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.

I agree. I nevertheless wanted to mention it because it has been
there all the time, for many years.

>> 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.

IOW you don't want to change how you describe it. I found it
somewhat confusing (or at least not clear enough) to read this
way, so had to compare and match the comment with the spec to
follow what exactly you mean.

>>> +        /* 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.

I don't follow you here. If it was strictly a restart, where
is the place where we first started it? Higher up you have

        /* Stop the main counter. */
        hpet_write32(hpet_cfg & ~HPET_CFG_ENABLE, HPET_CFG);

which of course stops the counter only if HPET_CFG_ENABLE
was set at that point. In find it less relevant to point out
this fact in the comment here, than I do in the "restart"
case; in fact I wouldn't have commented at all if the
comment simply was saying "Start ...".

Furthermore, wrt legacy mode I'm even less convinced there's
necessarily any _re_-starting.

>> 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.

I didn't think chipsets, where all of the various functionality
is bundled in a single chip, would necessarily use electrical
signals over an actual "line", but instead components would act
on a set of shared registers, using effectively software along
the lines of e.g. our vPIC emulation state machine. But I may
be entirely wrong with this ...

Assuming my implications above are correct that you don't really
want to change any of the commentary, I guess I don't want to
further hold this up, so preferably with the one pair of missing
parentheses added
Acked-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan