Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 23 Apr 2019 12:30:34 +0200
From:   Ingo Molnar <mingo@kernel.org>
To:     Daniel Drake <drake@endlessm.com>
Cc:     Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        hpa@zytor.com, x86@kernel.org,
        Linux Kernel <linux-kernel@vger.kernel.org>,
        Len Brown <len.brown@intel.com>,
        "Wysocki, Rafael J" <rafael.j.wysocki@intel.com>,
        Linux Upstreaming Team <linux@endlessm.com>
Subject: Re: [PATCH 2/2] x86/tsc: set LAPIC timer frequency to crystal clock
 frequency
Message-ID: <20190423103034.GA8216@gmail.com>
References: <20190422101526.28415-1-drake@endlessm.com>
 <20190422101526.28415-2-drake@endlessm.com>
 <20190422120451.GA9499@gmail.com>
 <CAD8Lp44ZaifPf+809BzbLg=9ijsRsE=7JMVpXkqRenY9radpZA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAD8Lp44ZaifPf+809BzbLg=9ijsRsE=7JMVpXkqRenY9radpZA@mail.gmail.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


* Daniel Drake <drake@endlessm.com> wrote:

> On Mon, Apr 22, 2019 at 8:04 PM Ingo Molnar <mingo@kernel.org> wrote:
> > Minor style nit: the parentheses are unnecessary, integer expressions
> > like this are evaluated left to right and multiplication and division has
> > the same precedence.
> 
> Fair point, although the same could be said for cpu_khz_from_msr().

Yes, this standardization on mhz, if it makes sense, might have to be 
propagated a bit to function names and any other variables.

Another naming quirk: what unit is 'lapic_timer_frequency' in? AFAICS 
it's a "period" unit (number of clock cycles per jiffy), not a frequency 
(which is number of cycles per second) - so the better name might be 
lapic_timer_period?

> > But it might also make sense to actually store crystal_mhz instead of
> > crystal_khz, because both CPUID 15H and 16H provides MHz values.
> >
> > That way the above expression would simplify to:
> >
> >         lapic_timer_frequency = crystal_mhz / HZ;
> 
> Wouldn't it be
>         lapic_timer_frequency = crystal_mhz * 1000000 / HZ;
> ?

Sorry, what I wanted to suggest is crystal_hz, not crystal_mhz.

I.e. store the raw unit that comes out of the CPUID, which appears to be 
HZ, right?

That would change the calculation to:

         lapic_timer_period = crystal_hz / HZ;

Note how clearly it's visible in that calculation that what we calculate 
there is a 'period', not a frequency.

Thanks,

	Ingo