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 <[email protected]> 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
