http://defect.opensolaris.org/bz/show_bug.cgi?id=6499
Bill Holler <bill.holler at sun.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|CAUSEKNOWN |FIXINPROGRESS
--- Comment #5 from Bill Holler <bill.holler at sun.com> 2009-02-13 18:31:49
---
(In reply to comment #0)
> hpet.h:
>
> Copyright date
> SCCS IDs
done.
> 43 * Using the top bit of a 64-bit hrtime_t reduces the max cyclic timeout
> 44 * we can track to about 260 thousand years after boot.
> What does "using the top bit" mean? You mean it's used for something
> else, or are you talking about signed int, or?...
Commented update. The original comment was leftover from bringup when
this was using a signed 64 bit int instead of the current implementation.
> HPET_NOT_SUPPORT presumably should be "no support"
> HPET_INTRRUPT_SUPPORT needs an E
done
> All the "/* support" comments there should presumably be "supports"
done
> *configure is used ... at some CPR callback time? It'd be nice
> if its name indicated that; configure is pretty generic.
changed to *callback
> The reason I was asking about gethpethrtime is the XXX comment by it.
> That should be removed, or the function should, one of the two.
Both dead-code gethpethrtime and XXX comment removed.
> Why are the lapic_ functions called "schedule"? They actually do the
> transition, right? and for that matter, why are they called hpet_lapic_?
> Which one's being scheduled?...or used?...
changes to hpet_use_[hpet|lapic]
> why are some of the entry points called non-hpet names, some have hpet
> embedded, and some start with hpet_?
Changed to consistent names. No longer use hpet_ in name.
> hpet_acpi.h:
>
> 39 #include <sys/acpi/acresrc.h> /* XXX: needed? */
>
> should probably figure that out pre-putback
done.
> "femto" is spelled without a 'p', despite what some idiot who wrote the
> HPET spec, and half the Internet, think
done
> cyclic.h: probably shouldn't be included in the push
This and another false positive file will be removed from changeset.
> hpet_acpi.c:
>
> 37 * This code is in pcplusmp.
> seems odd.
Removed. It is obvious from the Makefile and binary where this is.
> I'd have put the large comment explaining HPET in an include file, with
> a reference to it here if you wish, but along with the hpet definition
Done. Big block comment moved into hpet_acpi.c.
All of the defines and function declarations have also been moved.
> 68 * replacement" for the PIC and RTC hardware timers. These first two
> 69 * timers are reserved for that purpose.
> does that mean this file avoids using the first two timers, or the spec
Comment clarified. hpet_acpi.c avoids using the first two timers because
the spec instructs us to reserve them for possible legacy replacement.
Please see the HPET Spec. The first two timers *may* be wired differently
than the other HPET timer(s).
> 69 * timers are reserved for that purpose.
> does that mean this file avoids using the first two timers, or the spec
> requires it? I think you are not using them for PIC and RTC, but
> one could interpret this statement as implying that.
Comment updated to mention hpet_acpi.c is not using the first two timers.
File avoids these timers, and we are not using them for legacy replacement.
The two timers may be difficult to use for Deep C-state support because
they are slightly different than the other HPET timer(s).
> 74 * Mutual exclusion of HPET access is provided at the caller level.
> doesn't really tell me something I can act on; are you saying "make your
> own mutex before calling routines in this file"?
Done. This comment was me talking to myself during design. It is no
longer useful.
> 78 * HPET Specification 1.0a defines the HPET to be memory mapped to 1024
> bytes
> "be memory mapped to" should be "occupy"
done
> I'd much prefer the #defines to be in hpet_acpi.h, at least, if not hpet.h, or
> if you really want them private, hpet_impl.h, but not in the .c file.
> static prototypes: eh. There are a whole lot of them, and they clutter
> the file, but I'm OK I guess. 50/50 on moving them to hpet_acpi.h.
done. All moved into hpet_acpi.h
> 236 * Setup pointers to access symbols in pcplusmp.
>
> "Set up". (Setup is a noun.)
done
> 274 hpet.supported = 0;
> is superfluous, although I'm not very offended by it
changed to use the define HPET_NO_SUPPORT.
> 276 if (idle_cpu_no_deep_c)
> 277 return (DDI_FAILURE);
> 278
> 279 if (!cpuid_deep_cstates_supported())
> 280 return (DDI_FAILURE);
>
> Why here? Why not set up the info, and then let the configure() callback
> enable conditionally?
Solaris's sole use of the HPET is to support Deep C-states.
If the hardware does not support Deep C-states, then we do not enable
any of the HPET features (such mapping it nor starting its Main Counter).
We do not want to enable any of the HPET if we do not have to ,do to the
small risk that the HPET or ACPI may have bugs.
This executes early in boot (during pcplusmp initialization). This is
the first good place to check for Deep C-state support.
> hpet_memory_map() doesn't seem worth the extra function, and I wonder
> about hpet_read_gen_cap and gen_config; are they there for dtrace/kmdb, or?..
It is a design decision to access each hardware register with a function.
Each HPET register read is thousands of CPU cycles compared to a few cycles
per function call. If anyone really objects we can change it, otherwise
it will stay this way.
--
Configure bugmail: http://defect.opensolaris.org/bz/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.
