Re: [PATCH] x86: CPU detection for RDC System-on-Chip

[Florian Fainelli <florian@openwrt.org>]

Hi,

Le lundi 17 mai 2010 00:07:48, H. Peter Anvin a écrit :
> On 05/16/2010 06:21 AM, Florian Fainelli wrote:
> > diff --git a/arch/x86/kernel/cpu/rdc.c b/arch/x86/kernel/cpu/rdc.c
> > new file mode 100644
> > index 0000000..909c2b5
> > --- /dev/null
> > +++ b/arch/x86/kernel/cpu/rdc.c
> > @@ -0,0 +1,71 @@
> > +/*
> > + * See Documentation/x86/rdc.txt
> > + *
> > + * mark@bifferos.com
> > + */
> > +
> > +#include <linux/pci.h>
> > +#include <asm/pci-direct.h>
> > +#include "cpu.h"
> > +
> > +
> > +static void __cpuinit rdc_identify(struct cpuinfo_x86 *c)
> > +{
> > +	u16 vendor, device;
> > +	u32 customer_id;
> > +
> > +	if (!early_pci_allowed())
> > +		return;
> > +
> > +	/* RDC CPU is SoC (system-on-chip), Northbridge is always present */
> > +	vendor = read_pci_config_16(0, 0, 0, PCI_VENDOR_ID);
> > +	device = read_pci_config_16(0, 0, 0, PCI_DEVICE_ID);
> > +
> > +	if (vendor != PCI_VENDOR_ID_RDC || device != PCI_DEVICE_ID_RDC_R6020)
> > +		return;  /* not RDC */
> > +	/*
> > +	 * NB: We could go on and check other devices, e.g. r6040 NIC, but
> > +	 * that's probably overkill
> > +	 */
> > +
> > +	customer_id = read_pci_config(0, 0, 0, 0x90);
> > +
> > +	switch (customer_id) {
> > +		/* id names are from RDC */
> > +	case 0x00321000:
> > +		strcpy(c->x86_model_id, "R3210/R3211");
> > +		break;
> > +	case 0x00321001:
> > +		strcpy(c->x86_model_id, "AMITRISC20000/20010");
> > +		break;
> > +	case 0x00321002:
> > +		strcpy(c->x86_model_id, "R3210X/Edimax");
> > +		break;
> > +	case 0x00321003:
> > +		strcpy(c->x86_model_id, "R3210/Kcodes");
> > +		break;
> > +	case 0x00321004:  /* tested */
> > +		strcpy(c->x86_model_id, "S3282/CodeTek");
> > +		break;
> > +	case 0x00321007:
> > +		strcpy(c->x86_model_id, "R8610");
> > +		break;
> > +	default:
> > +		pr_info("RDC CPU: Unrecognised Customer ID (0x%x) please "
> > +			"report to: linux-kernel@vger.kernel.org\n",
> > +			customer_id);
> > +		break;
> 
> This pr_info() is completely useless... reporting things to LKML is very
> likely to get lost in the din.  If someone (like yourself) wants to be
> the maintainer for it that's one thing, otherwise it's probably better
> to tell them to file a bugzilla... or just report it as "unknown" like
> we do for all other CPU vendors.

Allright, filling a bugzilla entry sounds okay with me.

> 
> > +	}
> > +
> > +	strcpy(c->x86_vendor_id, "RDC");
> > +	c->x86_vendor = X86_VENDOR_RDC;
> > +}
> > +
> > +static const struct cpu_dev __cpuinitconst rdc_cpu_dev = {
> > +	.c_vendor	= "RDC",
> > +	.c_ident	= { "RDC" },
> > +	.c_identify	= rdc_identify,
> > +	.c_x86_vendor	= X86_VENDOR_RDC,
> > +};
> > +
> > +cpu_dev_register(rdc_cpu_dev);
> 
> .c_ident here is bogus: c_ident is supposed to represent the CPUID
> string, but this device apparently doesn't have CPUID.
> 
> This adds at least one PCI reference to every boot on every device.  As
> such, at least please read vendor and device as a single 32-bit
> reference.  Since this identification isn't actually used for any real
> purpose (like workarounds) we could also set it up as a PCI quirk... but
> it's probably better to just keep the same code flow.

More precisely, every device that has X86_RDC321X set in its kernel. I want to
get this merged first before actually using it, but the real purpose is to have 
different board flavors which register platform devices for the various models 
of RDC-based devices. Having this detection logic based on the RDC model is 
imho much cleaner than poking at the first bytes of the NOR flash and check the 
vendor firmware header.

As where to put those board files, this is another topic.
--
Florian