Please don't gzip patches, it lowers the chance of someone reviewing them to zero ;-)
I figured if we're coding at this level, gzip won't be a hurdle. I announced it up front, didn't I? :p
There is 65x microcode in the model_67x directory, and it's not split up into single files as for the other CPUs.
The microcodes are static and are not supposed to change, our CPU drivers are grouped by family, and the microcodes are going to be included one after another in one place anyway even when made separate, so why have 5 files for 5 microcodes in the 67x family for example when one will do? Of course I retained the comments therein that marks the microcodes and the processors they're intended for.
To put things in perspective, the microcode data we get from Intel has hundreds of them all in one file.
- if (cpuid_res.ebx != 0x756e6547 || cpuid_res.edx != 0x49656e69 ||
cpuid_res.ecx != 0x6c65746e) {
- printk(BIOS_INFO, "Not 'GenuineIntel' Processor");
- return 0;
- }
This is not necessary. When model_6**_init() is executed, it's clear already that this is an intel cpu.
I'm not sure if the check for choosing the CPU driver to execute is on these CPUID signatures, or through some other fields. This check is carried from v1. I agree we can take a chance and take it out; we'll revisit when something breaks. :D
+/* if (signature & 0x1000) {
- printk(BIOS_DEBUG,"Overdrive chip no L2 cache configuration\n");
- return 0;
- }
- if (signature < 0x630 || signature >= 0x680) {
- printk(BIOS_DEBUG,"L2 cache on CPUID %x does not require
configuration\n", signature);
- return 0;
- }*/
I think this code should just be dropped
That may change when (IF) we manage to get coreboot on a socket 8 board on which a Pentium II OverDrive processor sits. Again this piece is from the original code from v1. I agree the second signature check is not needed.
Is it generally possible to move p6_configure_l2_cache to a generic place, maybe to cpu/x86/cache ?
+/*--- End L2 init code from corebbot v1 ---*/
I think this kind of comment is not needed.
That's a marker to help preserve my sanity. :-p
+static inline void strcpy(char *dst, char *src) +{
- while (*src) *dst++ = *src++;
+}
I know I am the idiot who introduced this in a model_xxx_init.c file, but maybe we should move it to lib/strcpy.c ?
Well, 63x and 67x don't even need this; they don't support the processor brand string that 6bx do. Same with the fill_processor_name() function. I'll just take them out entirely.
+static struct cpu_device_id cpu_table[] = {
- { X86_VENDOR_INTEL, 0x0650 },
- { X86_VENDOR_INTEL, 0x0651 },
- { X86_VENDOR_INTEL, 0x0652 },
- { X86_VENDOR_INTEL, 0x0653 },
- { X86_VENDOR_INTEL, 0x0671 },
- { X86_VENDOR_INTEL, 0x0672 },
- { X86_VENDOR_INTEL, 0x0673 },
- { 0, 0 },
+};
Instead of using a common driver, can two drivers use a common set of shared functions instead?
It's already partially done. src/cpu/intel/model_67x/l2_cache.c (new in this patch) is linked in by model_65x driver too.
Next prime suspect: fill_processor_name() in 6bx and a few other CPU models I'm no expert of.
But 65x and 67x really are so similar that one driver can do both. Now is our end goal here to spin off model_63x/65x/66x/67x/68x/6ax/6bx all into their own CPU drivers when most of what's being done is duplicating code from model_6xx which is what they collectively once were? What's the rationale behind this need for split? I think a split should only be done when different initialization sequence are needed, eg. the special L2 initialization sequence for these SECC P6s.
Same with motherboard models. When P2B-LS support matures I'd probably use the same directory for P2B-L and P2B-S support. They share the same circuit board anyway, and a -LS can emulate both by moving two jumpers.
+ifeq ($(CONFIG_USE_DCACHE_RAM),y) +cpu_incs += $(src)/cpu/intel/car/cache_as_ram.inc +endif
This will break all targets except the ones you worked on.
I read this and thought, "I gotta run abuild." So I did. The errors I saw all seems to be k8/k10 targets and/or ACPI related. Even the entire P2B family that is in the tree passed abuild with this patch applied. Oh they haven't been migrated to CAR yet.
Thanks Keith