lkml.org 
[lkml]   [2018]   [May]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC 2/2] x86, tsc: Enable clock for ealry printk timestamp
Hi Andy,

Thanks for the review.

On Wed, May 30, 2018 at 04:25:07PM +0300, Andy Shevchenko wrote:
> On Wed, May 30, 2018 at 12:20 PM, Feng Tang <feng.tang@intel.com> wrote:
> > To show time info in kernel log earlier and help optimizing kernel
> > boot time, printk adds a debug hook "boot_printk_clock_fn()" for
> > capable platform which has accurate clock in early boot phase.
> >
> > This patch will add early param setup option, so that user can
> > chose to provide a tsc based early printk clock simply by adding
> > in command line: "boot_tsc=xxxxM" (xxxxM is the stable TSC freq).
>
> > +
>
> This line is not needed.
Ok, will remove it.

>
> > + cur_tsc = rdtsc();
> > + cur_tsc -= boot_tsc_offset;
>
> In one expression?
Agree

>
> > + if (!p)
> > + return -EINVAL;
> > +
>
> I'm not sure it's needed at all.
>
> > + boot_tsc_mhz = div64_u64(tsc_hz, 1024 * 1024);
>
> Hmm... 1024*1024 != 1000 * 1000. So, hz -> mhz here (as by suffixes)
> looks weird.

The memparse is mostly for memory stuff which takes 1M as 1024*1024,
but for TSC frequency 1M means 1000*1000, and the code is trying to
do some awkward translation.

But you are right, this code will break if user doesn't use expression
with 'm".

>
> > + if (boot_tsc_mhz == 0)
> > + return -EINVAL;
>
> > + pr_info("TSC has run for %lld us\n",
> > + div64_u64(boot_tsc_offset, boot_tsc_mhz));
> > +
> > + /* Setup the early printk clock */
> > + boot_printk_clock_fn = boot_tsc_clock;
>
> > + pr_info("TSC: Setup early printk timestamp with %lldM TSC.",
> > + boot_tsc_mhz);
>
> Perhaps remove period (above doesn't have it) and move this to one line?

You mean merge the two pr_info line? yes, we can do that.


Thanks,
Feng

\
 
 \ /
  Last update: 2018-05-30 16:16    [W:0.086 / U:10.424 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site