Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35148 )
Change subject: soc/intel/common/timer: Make TSC frequency calculation dynamically ......................................................................
Patch Set 4:
(4 comments)
https://review.coreboot.org/c/coreboot/+/35148/4/src/soc/intel/common/block/... File src/soc/intel/common/block/timer/timer.c:
https://review.coreboot.org/c/coreboot/+/35148/4/src/soc/intel/common/block/... PS4, Line 24: #define CPU_MODEL_INTEL_ATOM_DENVERTON 0x5F Why remove the header that provided this?
https://review.coreboot.org/c/coreboot/+/35148/4/src/soc/intel/common/block/... PS4, Line 67: assert(cpuidr_15h.ebx != 0 || cpuidr_15h.eax != 0); I know this is what I wrote, but I have second thoughts about the assert() now. It's probably too early for console/printk() to work and this could hit infinite recursion via udelay(). Better leave assert() away.
https://review.coreboot.org/c/coreboot/+/35148/4/src/soc/intel/common/block/... PS4, Line 78: core_crystal_nominal_freq_khz = 25000; Indentation is confusing.
You don't use cpuidr_15 for atom/denvertgon at all so you can move this up. For common code it is recommended that you check feature bits(?) that certain CPUID is implemented before accessing those leafs. This file should be made compatible with older intel SoCs as well, but can be followup work.
https://review.coreboot.org/c/coreboot/+/35148/4/src/soc/intel/common/block/... PS4, Line 94: return (core_crystal_nominal_freq_khz * cpuidr_15h.ebx / Returning 0 here is what I want to avoid. That assert() might be fall-thru and not appear in the logs at all. I have a topic thread 'monotonic-timer', I will come up with some plan such that the detected timebase always gets printed on the console.