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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 27/01/2021 09:28, Jan Beulich wrote:
> 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.

I can see about trying to make my description more clear, but your
attempt to summarise it is incorrect.  You've got the behaviour
backwards WRT when SETVAL is logically set.

It is the second write to CMP after SETVAL which is special, not the
first write.

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

"stop" and "restart" are a perfectly valid pair to use together, in this
and other contexts.

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

The legacy silicon block won't have changed in this regard.

Back when there were PCI lanes, they had to be real wires.  PCIe will
have had to mux INTx onto those real wires.  These days even with an
IO-APIC, some GPIO pins are still real wires.

Everything upstream of the PIC/IOAPIC is message based (both devices
actually send MSIs since the P4 era).  Nothing below it will have changed.

~Andrew