[PATCH] x86/mrst: add SFI platform device parsing code

[PATCH] x86/mrst: add SFI platform device parsing code

[Alan Cox]

From: Feng Tang <feng.tang@intel.com>

SFI provides a series of tables (differing between 0.7 and 0.8 firmware).
These describe the platform devices present including SPI and I²C devices,
as well as various sensors, keypads and other glue as well as interfaces
provided via the SCU IPC mechanism (intel_scu_ipc.c)

This patch is a merge of the core elements and relevant fixes from the
Intel development code by Feng, Alek, myself into a single coherent patch
for upstream submission.

It provides the needed infrastructure to register I2C, SPI and platform devices
described by the tables, as well as handlers for some of the hardware already
supported in kernel. The 0.8 firmware also provides GPIO tables.

Devices are created at boot time or if they are SCU dependant at the point an
SCU is discovered. The existing Linux device mechanisms will then handle the
device binding. At an abstract level this is an SFI to Linux device translator.

Device/platform specific setup/glue is in this file. This is done so that the
drivers for the generic I²C and SPI bus devices remain cross platform as they
should.

(Updated from RFC version to correct the emc1403 name used by the firmware
 and a wrongly used #define)

[Ingo - let me know if you only see this via the kernel list]

Signed-off-by: Alek Du <alek.du@linux.intel.com>
Signed-off-by: Feng Tang <feng.tang@linux.intel.com>
Signed-off-by: Alan Cox <alan@linux.intel.com>
---

 arch/x86/Kconfig                     |    2 
 arch/x86/include/asm/mrst.h          |    6 
 arch/x86/kernel/mrst.c               |  700 ++++++++++++++++++++++++++++++++++
 drivers/platform/x86/intel_scu_ipc.c |    5 
 4 files changed, 713 insertions(+), 0 deletions(-)


diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 2f0d2b3..64fee5d 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -410,6 +410,8 @@ config X86_MRST
 	depends on X86_EXTENDED_PLATFORM
 	depends on X86_IO_APIC
 	select APB_TIMER
+	select I2C
+	select SPI
 	---help---
 	  Moorestown is Intel's Low Power Intel Architecture (LPIA) based Moblin
 	  Internet Device(MID) platform. Moorestown consists of two chips:
diff --git a/arch/x86/include/asm/mrst.h b/arch/x86/include/asm/mrst.h
index 1635074..a05c7aa 100644
--- a/arch/x86/include/asm/mrst.h
+++ b/arch/x86/include/asm/mrst.h
@@ -10,7 +10,11 @@
  */
 #ifndef _ASM_X86_MRST_H
 #define _ASM_X86_MRST_H
+
+#include <linux/sfi.h>
+
 extern int pci_mrst_init(void);
+
 int __init sfi_parse_mrtc(struct sfi_table_header *table);
 
 /*
@@ -42,4 +46,6 @@ extern enum mrst_timer_options mrst_timer_options;
 #define SFI_MTMR_MAX_NUM 8
 #define SFI_MRTC_MAX	8
 
+extern void intel_scu_devices_create(void);
+extern void intel_scu_devices_destroy(void);
 #endif /* _ASM_X86_MRST_H */
diff --git a/arch/x86/kernel/mrst.c b/arch/x86/kernel/mrst.c
index 79ae681..0bf74c5 100644
--- a/arch/x86/kernel/mrst.c
+++ b/arch/x86/kernel/mrst.c
@@ -12,6 +12,14 @@
 #include <linux/init.h>
 #include <linux/kernel.h>
 #include <linux/sfi.h>
+#include <linux/intel_pmic_gpio.h>
+#include <linux/spi/spi.h>
+#include <linux/i2c.h>
+#include <linux/sfi.h>
+#include <linux/i2c/pca953x.h>
+#include <linux/gpio_keys.h>
+#include <linux/input.h>
+#include <linux/platform_device.h>
 #include <linux/irq.h>
 #include <linux/module.h>
 
@@ -23,6 +31,7 @@
 #include <asm/mrst.h>
 #include <asm/io.h>
 #include <asm/i8259.h>
+#include <asm/intel_scu_ipc.h>
 #include <asm/apb_timer.h>
 
 /*
@@ -309,3 +318,694 @@ static inline int __init setup_x86_mrst_timer(char *arg)
 	return 0;
 }
 __setup("x86_mrst_timer=", setup_x86_mrst_timer);
+
+/*
+ * Parsing GPIO table first, since the DEVS table will need this table
+ * to map the pin name to the actual pin.
+ */
+static struct sfi_gpio_table_entry *gpio_table;
+static int gpio_num_entry;
+
+static int __init sfi_parse_gpio(struct sfi_table_header *table)
+{
+	struct sfi_table_simple *sb;
+	struct sfi_gpio_table_entry *pentry;
+	int num, i;
+
+	if (gpio_table)
+		return 0;
+	sb = (struct sfi_table_simple *)table;
+	num = SFI_GET_NUM_ENTRIES(sb, struct sfi_gpio_table_entry);
+	pentry = (struct sfi_gpio_table_entry *)sb->pentry;
+
+	gpio_table = (struct sfi_gpio_table_entry *)
+				kmalloc(num * sizeof(*pentry), GFP_KERNEL);
+	if (!gpio_table)
+		return -1;
+	memcpy(gpio_table, pentry, num * sizeof(*pentry));
+	gpio_num_entry = num;
+
+	pr_info("Moorestown GPIO pin info:\n");
+	for (i = 0; i < num; i++, pentry++)
+		pr_info("info[%2d]: controller = %16.16s, pin_name = %16.16s,"
+		" pin = %d\n", i,
+			pentry->controller_name,
+			pentry->pin_name,
+			pentry->pin_no);
+	return 0;
+}
+
+static int get_gpio_by_name(const char *name)
+{
+	struct sfi_gpio_table_entry *pentry = gpio_table;
+	int i;
+
+	if (!pentry)
+		return -1;
+	for (i = 0; i < gpio_num_entry; i++, pentry++) {
+		if (!strncmp(name, pentry->pin_name, 16))
+			return pentry->pin_no;
+	}
+	return -1;
+}
+
+/*
+ * Here defines the array of devices platform data that IAFW would export
+ * through SFI "DEVS" table, we use name and type to match the device and
+ * its platform data.
+ */
+struct devs_id {
+	char name[17];
+	u8 type;
+	u8 delay;
+	void *(*get_platform_data)(void *info);
+};
+
+/* the offset for the mapping of global gpio pin to irq */
+#define MRST_IRQ_OFFSET 0x100
+
+static void *pmic_gpio_platform_data(void *info)
+{
+	static struct intel_pmic_gpio_platform_data pmic_gpio_pdata;
+	int gpio_base = get_gpio_by_name("pmic_gpio_base");
+
+	if (gpio_base == -1)
+		gpio_base = 64;	/* Needed for 0.7 SFI firmware */
+	pmic_gpio_pdata.gpio_base = gpio_base;
+	pmic_gpio_pdata.irq_base = gpio_base + MRST_IRQ_OFFSET;
+	pmic_gpio_pdata.gpiointr = 0xffffeff8;
+
+	return &pmic_gpio_pdata;
+}
+
+static void *max3111_platform_data(void *info)
+{
+	static int dummy;
+	struct spi_board_info *spi_info = (struct spi_board_info *)info;
+	int intr = get_gpio_by_name("max3111_int");
+
+	if (intr == -1)
+		return NULL;
+	spi_info->irq = intr + MRST_IRQ_OFFSET;
+	/* we return a dummy pdata */
+	return &dummy;
+}
+
+/* we have multiple max7315 on the board ... */
+#define MAX7315_NUM 2
+static void *max7315_platform_data(void *info)
+{
+	static struct pca953x_platform_data max7315_pdata[MAX7315_NUM];
+	static int nr;
+	struct pca953x_platform_data *max7315 = &max7315_pdata[nr];
+	struct i2c_board_info *i2c_info = (struct i2c_board_info *)info;
+	int gpio_base;
+	int intr;
+	char base_pin_name[17];
+	char intr_pin_name[17];
+
+	if (nr == MAX7315_NUM) {
+		printk(KERN_ERR "too many max7315s, we only support %d\n",
+				MAX7315_NUM);
+		return NULL;
+	}
+	/* we have several max7315 on the board, we only need load several
+	 * instances of the same pca953x driver to cover them
+	 */
+	strcpy(i2c_info->type, "max7315");
+	if (nr++) {
+		sprintf(base_pin_name, "max7315_%d_base", nr);
+		sprintf(intr_pin_name, "max7315_%d_int", nr);
+	} else {
+		strcpy(base_pin_name, "max7315_base");
+		strcpy(intr_pin_name, "max7315_int");
+	}
+
+	gpio_base = get_gpio_by_name(base_pin_name);
+	intr = get_gpio_by_name(intr_pin_name);
+
+	if (gpio_base == -1)
+		return NULL;
+	max7315->gpio_base = gpio_base;
+	if (intr != -1) {
+		i2c_info->irq = intr + MRST_IRQ_OFFSET;
+		max7315->irq_base = gpio_base + MRST_IRQ_OFFSET;
+	} else {
+		i2c_info->irq = -1;
+		max7315->irq_base = -1;
+	}
+	return max7315;
+}
+
+static void *emc1403_platform_data(void *info)
+{
+	static short intr2nd_pdata;
+	struct i2c_board_info *i2c_info = (struct i2c_board_info *)info;
+	int intr = get_gpio_by_name("thermal_int");
+	int intr2nd = get_gpio_by_name("thermal_alert");
+
+	if (intr == -1 || intr2nd == -1)
+		return NULL;
+
+	i2c_info->irq = intr + MRST_IRQ_OFFSET;
+	intr2nd_pdata = intr2nd + MRST_IRQ_OFFSET;
+
+	return &intr2nd_pdata;
+}
+
+static void *lis331dl_platform_data(void *info)
+{
+	static short intr2nd_pdata;
+	struct i2c_board_info *i2c_info = (struct i2c_board_info *)info;
+	int intr = get_gpio_by_name("accel_int");
+	int intr2nd = get_gpio_by_name("accel_2");
+
+	if (intr == -1 || intr2nd == -1)
+		return NULL;
+
+	i2c_info->irq = intr + MRST_IRQ_OFFSET;
+	intr2nd_pdata = intr2nd + MRST_IRQ_OFFSET;
+
+	return &intr2nd_pdata;
+}
+
+static const struct devs_id device_ids[] = {
+	{"pmic_gpio", SFI_DEV_TYPE_SPI, 1, &pmic_gpio_platform_data},
+	{"spi_max3111", SFI_DEV_TYPE_SPI, 0, &max3111_platform_data},
+	{"i2c_max7315", SFI_DEV_TYPE_I2C, 1, &max7315_platform_data},
+	{"i2c_max7315_2", SFI_DEV_TYPE_I2C, 1, &max7315_platform_data},
+	{"emc1403", SFI_DEV_TYPE_I2C, 1, &emc1403_platform_data},
+	{"i2c_accel", SFI_DEV_TYPE_I2C, 0, &lis331dl_platform_data},
+	{},
+};
+
+#define MAX_IPCDEVS	24
+static struct platform_device *ipc_devs[MAX_IPCDEVS];
+static int ipc_next_dev;
+
+#define MAX_DELAYED_SPI	24
+static struct spi_board_info *spi_devs[MAX_DELAYED_SPI];
+static int spi_next_dev;
+
+#define MAX_DELAYED_I2C	24
+static struct i2c_board_info *i2c_devs[MAX_DELAYED_I2C];
+static int i2c_bus[MAX_DELAYED_I2C];
+static int i2c_next_dev;
+
+static void intel_scu_device_register(struct platform_device *pdev)
+{
+	BUG_ON(ipc_next_dev == MAX_IPCDEVS);
+	ipc_devs[ipc_next_dev++] = pdev;
+}
+
+static void intel_delayed_spi_device_register(struct spi_board_info *sdev)
+{
+	struct spi_board_info *new_dev;
+
+	BUG_ON(spi_next_dev == MAX_DELAYED_SPI);
+
+	new_dev = kzalloc(sizeof(*sdev), GFP_KERNEL);
+	if (!new_dev) {
+		pr_err("MRST: fail to alloc mem for delayed spi dev %s\n",
+			sdev->modalias);
+		return;
+	}
+	memcpy(new_dev, sdev, sizeof(*sdev));
+
+	spi_devs[spi_next_dev++] = new_dev;
+}
+
+static void intel_delayed_i2c_device_register(int bus,
+						struct i2c_board_info *idev)
+{
+	struct i2c_board_info *new_dev;
+
+	BUG_ON(i2c_next_dev == MAX_DELAYED_I2C);
+
+	new_dev = kzalloc(sizeof(*idev), GFP_KERNEL);
+	if (!new_dev) {
+		pr_err("MRST: fail to alloc mem for delayed i2c dev %s\n",
+			idev->type);
+		return;
+	}
+	memcpy(new_dev, idev, sizeof(*idev));
+
+	i2c_bus[i2c_next_dev] = bus;
+	i2c_devs[i2c_next_dev++] = new_dev;
+}
+
+/* Called by IPC driver */
+void intel_scu_devices_create(void)
+{
+	int i;
+
+	for (i = 0; i < ipc_next_dev; i++)
+		platform_device_add(ipc_devs[i]);
+
+	for (i = 0; i < spi_next_dev; i++) {
+		spi_register_board_info(spi_devs[i], 1);
+		kfree(spi_devs[i]);
+	}
+
+	for (i = 0; i < i2c_next_dev; i++) {
+		struct i2c_adapter *adapter;
+		struct i2c_client *client;
+
+		adapter = i2c_get_adapter(i2c_bus[i]);
+		if (adapter) {
+			client = i2c_new_device(adapter, i2c_devs[i]);
+			if (!client)
+				pr_err("mrst: can't create i2c device %s\n",
+					i2c_devs[i]->type);
+		} else
+			i2c_register_board_info(i2c_bus[i], i2c_devs[i], 1);
+		kfree(i2c_devs[i]);
+	}
+}
+EXPORT_SYMBOL_GPL(intel_scu_devices_create);
+
+/* Called by IPC driver */
+void intel_scu_devices_destroy(void)
+{
+	int i;
+
+	for (i = 0; i < ipc_next_dev; i++)
+		platform_device_del(ipc_devs[i]);
+}
+EXPORT_SYMBOL_GPL(intel_scu_devices_destroy);
+
+static void install_irq_resource(struct platform_device *pdev, int irq)
+{
+	/* Single threaded */
+	static struct resource res = {
+		.name = "IRQ",
+		.flags = IORESOURCE_IRQ,
+	};
+	res.start = irq;
+	platform_device_add_resources(pdev, &res, 1);
+}
+
+static void sfi_handle_ipc_dev(struct platform_device *pdev)
+{
+	const struct devs_id *dev = device_ids;
+	void *pdata = NULL;
+
+	while (dev->name[0]) {
+		if (dev->type == SFI_DEV_TYPE_IPC &&
+				!strncmp(dev->name, pdev->name, 16)) {
+			pdata = dev->get_platform_data(pdev);
+			break;
+		}
+		dev++;
+	}
+	pdev->dev.platform_data = pdata;
+	intel_scu_device_register(pdev);
+}
+
+static int sfi_force_ipc(const char *name)
+{
+	static const char *to_force[] = {
+		"pmic_gpio",
+		"pmic_battery",
+		"pmic_touch",
+		"pmic_audio",
+		"msic_audio",
+		NULL
+	};
+	const char **p = &to_force[0];
+	while (*p != NULL) {
+		if (strcmp(*p, name) == 0)
+			return 1;
+		p++;
+	}
+	return 0;
+}
+
+static void sfi_handle_spi_dev(struct spi_board_info *spi_info)
+{
+	const struct devs_id *dev = device_ids;
+	void *pdata = NULL;
+
+	/* Older firmware lists some IPC devices as SPI. We want to force
+	   these to be the correct type for Linux */
+	if (sfi_force_ipc(spi_info->modalias)) {
+		/* Allocate a platform device, and translate the device
+		   configuration, then use the ipc helper. */
+		struct platform_device *pdev;
+
+		pdev = platform_device_alloc(spi_info->modalias,
+					spi_info->irq);
+		if (pdev == NULL) {
+			pr_err("out of memory for SFI platform device '%s'.\n",
+						spi_info->modalias);
+			return;
+		}
+		install_irq_resource(pdev, spi_info->irq);
+		sfi_handle_ipc_dev(pdev);
+		return;
+	}
+
+
+	while (dev->name[0]) {
+		if (dev->type == SFI_DEV_TYPE_SPI &&
+				!strncmp(dev->name, spi_info->modalias, 16)) {
+			pdata = dev->get_platform_data(spi_info);
+			break;
+		}
+		dev++;
+	}
+	spi_info->platform_data = pdata;
+	if (dev->delay)
+		intel_delayed_spi_device_register(spi_info);
+	else
+		spi_register_board_info(spi_info, 1);
+}
+
+static void sfi_handle_i2c_dev(int bus, struct i2c_board_info *i2c_info)
+{
+	const struct devs_id *dev = device_ids;
+	void *pdata = NULL;
+
+	while (dev->name[0]) {
+		if (dev->type == SFI_DEV_TYPE_I2C &&
+				!strncmp(dev->name, i2c_info->type, 16)) {
+			pdata = dev->get_platform_data(i2c_info);
+			break;
+		}
+		dev++;
+	}
+	i2c_info->platform_data = pdata;
+
+	if (dev->delay)
+		intel_delayed_i2c_device_register(bus, i2c_info);
+	else
+		i2c_register_board_info(bus, i2c_info, 1);
+ }
+
+
+static int __init sfi_parse_devs(struct sfi_table_header *table)
+{
+	struct sfi_table_simple *sb;
+	struct sfi_device_table_entry *pentry;
+	struct spi_board_info spi_info;
+	struct i2c_board_info i2c_info;
+	struct platform_device *pdev;
+	int num, i, bus;
+	int ioapic;
+	struct io_apic_irq_attr irq_attr;
+
+	sb = (struct sfi_table_simple *)table;
+	num = SFI_GET_NUM_ENTRIES(sb, struct sfi_device_table_entry);
+	pentry = (struct sfi_device_table_entry *)sb->pentry;
+
+	for (i = 0; i < num; i++, pentry++) {
+		if (pentry->irq != (u8)0xff) { /* native RTE case */
+			/* these SPI2 devices are not exposed to system as PCI
+			 * devices, but they have separate RTE entry in IOAPIC
+			 * so we have to enable them one by one here
+			 */
+			ioapic = mp_find_ioapic(pentry->irq);
+			irq_attr.ioapic = ioapic;
+			irq_attr.ioapic_pin = pentry->irq;
+			irq_attr.trigger = 1;
+			irq_attr.polarity = 1;
+			io_apic_set_pci_routing(NULL, pentry->irq, &irq_attr);
+		}
+		switch (pentry->type) {
+		case SFI_DEV_TYPE_IPC:
+			/* ID as IRQ is a hack that will go away */
+			pdev = platform_device_alloc(pentry->name, pentry->irq);
+			if (pdev == NULL) {
+				pr_err("out of memory for SFI platform device '%s'.\n",
+							pentry->name);
+				continue;
+			}
+			install_irq_resource(pdev, pentry->irq);
+			pr_info("info[%2d]: IPC bus, name = %16.16s, "
+				"irq = 0x%2x\n", i, pentry->name, pentry->irq);
+			sfi_handle_ipc_dev(pdev);
+			break;
+		case SFI_DEV_TYPE_SPI:
+			memset(&spi_info, 0, sizeof(spi_info));
+			strncpy(spi_info.modalias, pentry->name, 16);
+			spi_info.irq = pentry->irq;
+			spi_info.bus_num = pentry->host_num;
+			spi_info.chip_select = pentry->addr;
+			spi_info.max_speed_hz = pentry->max_freq;
+			pr_info("info[%2d]: SPI bus = %d, name = %16.16s, "
+				"irq = 0x%2x, max_freq = %d, cs = %d\n", i,
+				spi_info.bus_num,
+				spi_info.modalias,
+				spi_info.irq,
+				spi_info.max_speed_hz,
+				spi_info.chip_select);
+			sfi_handle_spi_dev(&spi_info);
+			break;
+		case SFI_DEV_TYPE_I2C:
+			memset(&i2c_info, 0, sizeof(i2c_info));
+			bus = pentry->host_num;
+			strncpy(i2c_info.type, pentry->name, 16);
+			i2c_info.irq = pentry->irq;
+			i2c_info.addr = pentry->addr;
+			pr_info("info[%2d]: I2C bus = %d, name = %16.16s, "
+				"irq = 0x%2x, addr = 0x%x\n", i, bus,
+				i2c_info.type,
+				i2c_info.irq,
+				i2c_info.addr);
+			sfi_handle_i2c_dev(bus, &i2c_info);
+			break;
+		case SFI_DEV_TYPE_UART:
+		case SFI_DEV_TYPE_HSI:
+		default:
+			;
+		}
+	}
+	return 0;
+}
+
+#define MRST_SPI2_CS_START	4
+static struct intel_pmic_gpio_platform_data pmic_gpio_pdata;
+
+static int __init sfi_parse_spib(struct sfi_table_header *table)
+{
+	struct sfi_table_simple *sb;
+	struct sfi_spi_table_entry *pentry;
+	struct spi_board_info *info;
+	int num, i, j;
+	int ioapic;
+	struct io_apic_irq_attr irq_attr;
+
+	sb = (struct sfi_table_simple *)table;
+	num = SFI_GET_NUM_ENTRIES(sb, struct sfi_spi_table_entry);
+	pentry = (struct sfi_spi_table_entry *) sb->pentry;
+
+	info = kzalloc(num * sizeof(*info), GFP_KERNEL);
+	if (!info) {
+		pr_info("%s(): Error in kzalloc\n", __func__);
+		return -ENOMEM;
+	}
+
+	if (num)
+		pr_info("Moorestown SPI devices info:\n");
+
+	for (i = 0, j = 0; i < num; i++, pentry++) {
+		strncpy(info[j].modalias, pentry->name, 16);
+		info[j].irq = pentry->irq_info;
+		info[j].bus_num = pentry->host_num;
+		info[j].chip_select = pentry->cs;
+		info[j].max_speed_hz = 3125000;	/* hard coded */
+		if (info[i].chip_select >= MRST_SPI2_CS_START) {
+			/* these SPI2 devices are not exposed to system as PCI
+			 * devices, but they have separate RTE entry in IOAPIC
+			 * so we have to enable them one by one here
+			 */
+			ioapic = mp_find_ioapic(info[j].irq);
+			irq_attr.ioapic = ioapic;
+			irq_attr.ioapic_pin = info[j].irq;
+			irq_attr.trigger = 1;
+			irq_attr.polarity = 1;
+			io_apic_set_pci_routing(NULL, info[j].irq,
+							&irq_attr);
+		}
+		info[j].platform_data = pentry->dev_info;
+
+		if (!strcmp(pentry->name, "pmic_gpio")) {
+			memcpy(&pmic_gpio_pdata, pentry->dev_info, 8);
+			pmic_gpio_pdata.gpiointr = 0xffffeff8;
+			info[j].platform_data = &pmic_gpio_pdata;
+		}
+		pr_info("info[%d]: name = %16.16s, irq = 0x%04x, bus = %d, "
+			"cs = %d\n", j, info[j].modalias, info[j].irq,
+			info[j].bus_num, info[j].chip_select);
+		sfi_handle_spi_dev(&info[j]);
+		j++;
+	}
+	kfree(info);
+	return 0;
+}
+
+#define MRST_I2C_BUSNUM	3
+static struct pca953x_platform_data max7315_pdata;
+static struct pca953x_platform_data max7315_pdata_2;
+
+static int __init sfi_parse_i2cb(struct sfi_table_header *table)
+{
+	struct sfi_table_simple *sb;
+	struct sfi_i2c_table_entry *pentry;
+	struct i2c_board_info info;
+	int num, i, busnum;
+
+	sb = (struct sfi_table_simple *)table;
+	num = SFI_GET_NUM_ENTRIES(sb, struct sfi_i2c_table_entry);
+	pentry = (struct sfi_i2c_table_entry *) sb->pentry;
+
+	if (num <= 0)
+		return -ENODEV;
+
+	for (i = 0; i < num; i++, pentry++) {
+		busnum = pentry->host_num;
+		if (busnum >= MRST_I2C_BUSNUM || busnum < 0)
+			continue;
+
+		memset(&info, 0, sizeof(info));
+		strncpy(info.type, pentry->name, 16);
+		info.irq = pentry->irq_info;
+		info.addr = pentry->addr;
+		info.platform_data = pentry->dev_info;
+
+		if (!strcmp(pentry->name, "i2c_max7315")) {
+			strcpy(info.type, "max7315");
+			max7315_pdata.irq_base = *(int *)pentry->dev_info;
+			max7315_pdata.gpio_base =
+				*((u32 *)pentry->dev_info + 1);
+			info.platform_data = &max7315_pdata;
+		} else if (!strcmp(pentry->name, "i2c_max7315_2")) {
+			strcpy(info.type, "max7315");
+			max7315_pdata_2.irq_base = *(int *)pentry->dev_info;
+			max7315_pdata_2.gpio_base =
+				*((u32 *)pentry->dev_info + 1);
+			info.platform_data = &max7315_pdata_2;
+		}
+
+		pr_info("info[%d]: bus = %d, name = %16.16s, irq = 0x%04x, "
+			"addr = 0x%x\n", i, busnum, info.type,
+		       info.irq, info.addr);
+
+		i2c_register_board_info(busnum, &info, 1);
+	}
+
+	return 0;
+}
+
+
+static int __init mrst_platform_init(void)
+{
+	/* Keep for back compatibility for SFI 0.7 and before */
+	sfi_table_parse(SFI_SIG_SPIB, NULL, NULL, sfi_parse_spib);
+	sfi_table_parse(SFI_SIG_I2CB, NULL, NULL, sfi_parse_i2cb);
+
+	/* For SFi 0.8 version */
+	sfi_table_parse(SFI_SIG_GPIO, NULL, NULL, sfi_parse_gpio);
+	sfi_table_parse(SFI_SIG_DEVS, NULL, NULL, sfi_parse_devs);
+	return 0;
+}
+arch_initcall(mrst_platform_init);
+
+/*
+ * we will search these buttons in SFI GPIO table (by name)
+ * and register them dynamically. Please add all possible
+ * buttons here, we will shrink them if no GPIO found.
+ */
+static struct gpio_keys_button gpio_button[] = {
+	{KEY_POWER,		-1, 1, "power_btn",	EV_KEY, 0, 3000},
+	{KEY_PROG1,		-1, 1, "prog_btn1",	EV_KEY, 0, 20},
+	{KEY_PROG2,		-1, 1, "prog_btn2",	EV_KEY, 0, 20},
+	{SW_LID,		-1, 1, "lid_switch",	EV_SW,  0, 20},
+	{KEY_VOLUMEUP,		-1, 1, "vol_up",	EV_KEY, 0, 20},
+	{KEY_VOLUMEDOWN,	-1, 1, "vol_down",	EV_KEY, 0, 20},
+	{KEY_CAMERA,		-1, 1, "camera_full",	EV_KEY, 0, 20},
+	{KEY_CAMERA_FOCUS,	-1, 1, "camera_half",	EV_KEY, 0, 20},
+	{SW_KEYPAD_SLIDE,	-1, 1, "MagSw1",	EV_SW,  0, 20},
+	{SW_KEYPAD_SLIDE,	-1, 1, "MagSw2",	EV_SW,  0, 20},
+	{-1},/* must be ended with code = -1 */
+};
+
+static struct gpio_keys_platform_data mrst_gpio_keys = {
+	.buttons	= gpio_button,
+	.rep		= 1,
+	.nbuttons	= -1, /* will fill it after search */
+};
+
+static struct platform_device pb_device = {
+	.name		= "gpio-keys",
+	.id		= -1,
+	.dev		= {
+		.platform_data	= &mrst_gpio_keys,
+	},
+};
+
+static void __init pb_match(const char *name, int gpio)
+{
+	struct gpio_keys_button *gb = gpio_button;
+
+	while (gb->code != -1) {
+		if (!strcmp(name, gb->desc)) {
+			gb->gpio = gpio;
+			break;
+		}
+		gb++;
+	}
+}
+
+/* shrink non-existent buttons and return total available number */
+static int __init pb_shrink(void)
+{
+	struct gpio_keys_button *gb = gpio_button;
+	struct gpio_keys_button *next = NULL;
+	int num = 0;
+
+	while (gb->code != -1) {
+		if (gb->gpio == -1) {
+			if (!next)
+				next = gb + 1;
+			while (next->code != -1 && next->gpio == -1)
+				next++;
+			if (next->code == -1)/* end */
+				break;
+			*gb = *next;
+			next->gpio = -1;
+			next++;
+		}
+		num++;
+		gb++;
+	}
+	return num;
+}
+
+static int __init pb_keys_init(void)
+{
+	int num;
+	/* for SFI 0.7, we have to claim static gpio buttons */
+	if (!gpio_table) {
+		pb_match("power_btn", 65);
+		pb_match("prog_btn1", 66);
+		pb_match("prog_btn2", 69);
+		pb_match("lid_switch", 101);
+	} else {/* SFI 0.8 */
+		struct gpio_keys_button *gb = gpio_button;
+
+		while (gb->code != -1) {
+			gb->gpio = get_gpio_by_name(gb->desc);
+			gb++;
+		}
+	}
+	num = pb_shrink();
+	if (num) {
+		mrst_gpio_keys.nbuttons = num;
+		return platform_device_register(&pb_device);
+	}
+	return 0;
+}
+
+late_initcall(pb_keys_init);
+
diff --git a/drivers/platform/x86/intel_scu_ipc.c b/drivers/platform/x86/intel_scu_ipc.c
index 41a9e34..ca35b0c 100644
--- a/drivers/platform/x86/intel_scu_ipc.c
+++ b/drivers/platform/x86/intel_scu_ipc.c
@@ -26,6 +26,7 @@
 #include <linux/sfi.h>
 #include <asm/mrst.h>
 #include <asm/intel_scu_ipc.h>
+#include <asm/mrst.h>
 
 /* IPC defines the following message types */
 #define IPCMSG_WATCHDOG_TIMER 0xF8 /* Set Kernel Watchdog Threshold */
@@ -699,6 +700,9 @@ static int ipc_probe(struct pci_dev *dev, const struct pci_device_id *id)
 		iounmap(ipcdev.ipc_base);
 		return -ENOMEM;
 	}
+
+	intel_scu_devices_create();
+
 	return 0;
 }
 
@@ -720,6 +724,7 @@ static void ipc_remove(struct pci_dev *pdev)
 	iounmap(ipcdev.ipc_base);
 	iounmap(ipcdev.i2c_base);
 	ipcdev.pdev = NULL;
+	intel_scu_devices_destroy();
 }
 
 static const struct pci_device_id pci_ids[] = {

Re: [PATCH] x86/mrst: add SFI platform device parsing code

[Alan Cox]

> > +static void *lis331dl_platform_data(void *info)
> > +{
> > +	static short intr2nd_pdata;
> > +	struct i2c_board_info *i2c_info = (struct i2c_board_info
> > *)info;
> 
> It's rather concerning that the parser here needs to have all this
> knowledge about the specific chips that will be on the boards.  Is
> there a plan for how this will be managed once system integrators
> begin putting other chips onto Moorestown boards?

It depends on the device. The information for the platform device has
to come from somewhere and the kernel has no formal standard structure
for private data passed from platform devices - so in a lot of cases
its really just the needed glue so the platform device code in
drivers/i2c etc stays independent.

For a device that doesn't need any private structure passing it's just
a case of knowing the Linux name that matches the firmware description.
For a chip that already has a Linux driver that expects private data
(or a new driver that needs to) the info has to come from somewhere and
Linux packages it in per device fashion.

The parser has the knowledge precisely because we don't want to put the
knowledge in the driver.

Alan

Re: [PATCH] x86/mrst: add SFI platform device parsing code

[Mark Brown]

On Mon, Sep 20, 2010 at 03:01:06PM +0100, Alan Cox wrote:

> +static void *max3111_platform_data(void *info)
> +{
> +	static int dummy;
> +	struct spi_board_info *spi_info = (struct spi_board_info *)info;
> +	int intr = get_gpio_by_name("max3111_int");

> +/* we have multiple max7315 on the board ... */
> +#define MAX7315_NUM 2
> +static void *max7315_platform_data(void *info)

> +static void *emc1403_platform_data(void *info)
> +{

> +static void *lis331dl_platform_data(void *info)
> +{
> +	static short intr2nd_pdata;
> +	struct i2c_board_info *i2c_info = (struct i2c_board_info *)info;

It's rather concerning that the parser here needs to have all this
knowledge about the specific chips that will be on the boards.  Is there
a plan for how this will be managed once system integrators begin
putting other chips onto Moorestown boards?

Re: [PATCH] x86/mrst: add SFI platform device parsing code

[Mark Brown]

On Mon, Sep 20, 2010 at 03:27:26PM +0100, Alan Cox wrote:

> > It's rather concerning that the parser here needs to have all this
> > knowledge about the specific chips that will be on the boards.  Is
> > there a plan for how this will be managed once system integrators
> > begin putting other chips onto Moorestown boards?

> For a device that doesn't need any private structure passing it's just
> a case of knowing the Linux name that matches the firmware description.
> For a chip that already has a Linux driver that expects private data
> (or a new driver that needs to) the info has to come from somewhere and
> Linux packages it in per device fashion.

Right, we need to do something and given that folks decided that OF
wasn't for them... :/

> The parser has the knowledge precisely because we don't want to put the
> knowledge in the driver.

This is precisely the opposite approach to that which has been taken
with all the OF stuff where individual drivers take care of parsing
their own data out of the OF tree.  I guess it'd be good if we could
achieve some level of consistency on this one, though I have this
horrible feeling that we're going to end up with all sorts of board
specific workarounds in here, especially around things like audio where
you've got multiple chips working together.

The OF approach does have the advantage of avoiding collisions between
multiple devices, and gives us some hope that the driver maintainers may
have seen the definitions that are being created for the BIOSes.

Re: [PATCH] x86/mrst: add SFI platform device parsing code

[Grant Likely]

On Mon, Sep 20, 2010 at 12:27 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Mon, Sep 20, 2010 at 03:27:26PM +0100, Alan Cox wrote:
>
>> > It's rather concerning that the parser here needs to have all this
>> > knowledge about the specific chips that will be on the boards.  Is
>> > there a plan for how this will be managed once system integrators
>> > begin putting other chips onto Moorestown boards?
>
>> For a device that doesn't need any private structure passing it's just
>> a case of knowing the Linux name that matches the firmware description.
>> For a chip that already has a Linux driver that expects private data
>> (or a new driver that needs to) the info has to come from somewhere and
>> Linux packages it in per device fashion.
>
> Right, we need to do something and given that folks decided that OF
> wasn't for them... :/
>
>> The parser has the knowledge precisely because we don't want to put the
>> knowledge in the driver.
>
> This is precisely the opposite approach to that which has been taken
> with all the OF stuff where individual drivers take care of parsing
> their own data out of the OF tree.  I guess it'd be good if we could
> achieve some level of consistency on this one, though I have this
> horrible feeling that we're going to end up with all sorts of board
> specific workarounds in here, especially around things like audio where
> you've got multiple chips working together.
>
> The OF approach does have the advantage of avoiding collisions between
> multiple devices, and gives us some hope that the driver maintainers may
> have seen the definitions that are being created for the BIOSes.

Shudder.

Okay, before I start a long rant I'll start with this disclaimer; at
least this patch is isolated to the mrst platform so that it doesn't
impact the rest of the kernel.  Feel free to take the stance that this
is platform specific code and therefore will never have to handle the
general-case of multiple machines.  In that case each machine will
need to have a separate copy of this functionality, but at least the
damage is contained. </disclaim>

This will be an utter nightmare to maintain over the long term.  It
combines the worst parts of both static device tables and firmware
data parsers.  From the former it uses large statically allocated
tables that cannot be either extended or freed at runtime which is
unfriendly for multiplatform kernels.

>From the latter it takes the approach of abstracting all the details
of the firmware interface away from the device drivers which forces
the abstraction layer to encode every possible permutation and
combination of devices in a hunk of code that cannot be modularized
and will become larger and larger for each new device that is
supported.  The lesson was learned with OF that the device driver is
the only place where it makes sense to decode device specific data
(addresses and irqs can have common code decoders, but the binding
name and extra data is device driver specific).  Trying to do it
anywhere else is insanity.

That being said, there already is a defined method for passing in
external data about the machine layout.  I'm certainly not keen on
adding another.  It is already an uphill battle to convince device
driver maintainers that adding device tree parsing code (appropriately
contained) to .probe() hooks is the right thing to do.  Asking them to
also support SFI hooks (plus the next firmware-interface-of-the-month)
is likely to end in a revolt.

Both tglx and dwmw2 [cc'd] have stated that they they are in favor of
standardizing on OF device tree for x86 embedded.  dwmw2 suggested
translating data in other formats into the device tree during early
boot so that existing OF routines can be used to parse the data by
device drivers.  That would keep the support code in the device
drivers where it belongs without creating new SFI-specific device
infrastructure.  It also eliminates the statically allocated tables
and board specific hacks.

Since data in the tree is free-form key-value properties, any
device-specific SFI data can be imported into the tree verbatim to
eliminate the risk that critical data will get discarded in the
translation process.  Well understood properties, like address ranges,
IRQs and GPIOs would be straight forward to translate into the device
tree form to make it parsable by common code.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH] x86/mrst: add SFI platform device parsing code

[David Woodhouse]

On Wed, 2010-09-22 at 01:03 -0300, Grant Likely wrote:
> Shudder.
>
> Okay, before I start a long rant I'll start with this disclaimer; at
> least this patch is isolated to the mrst platform so that it doesn't
> impact the rest of the kernel.  Feel free to take the stance that this
> is platform specific code and therefore will never have to handle the
> general-case of multiple machines.  In that case each machine will
> need to have a separate copy of this functionality, but at least the
> damage is contained. </disclaim>
> 
> This will be an utter nightmare to maintain over the long term.  It
> combines the worst parts of both static device tables and firmware
> data parsers.  From the former it uses large statically allocated
> tables that cannot be either extended or freed at runtime which is
> unfriendly for multiplatform kernels.
> 
> From the latter it takes the approach of abstracting all the details
> of the firmware interface away from the device drivers which forces
> the abstraction layer to encode every possible permutation and
> combination of devices in a hunk of code that cannot be modularized
> and will become larger and larger for each new device that is
> supported.  The lesson was learned with OF that the device driver is
> the only place where it makes sense to decode device specific data
> (addresses and irqs can have common code decoders, but the binding
> name and extra data is device driver specific).  Trying to do it
> anywhere else is insanity.
> 
> That being said, there already is a defined method for passing in
> external data about the machine layout.  I'm certainly not keen on
> adding another.  It is already an uphill battle to convince device
> driver maintainers that adding device tree parsing code (appropriately
> contained) to .probe() hooks is the right thing to do.  Asking them to
> also support SFI hooks (plus the next firmware-interface-of-the-month)
> is likely to end in a revolt.
> 
> Both tglx and dwmw2 [cc'd] have stated that they they are in favor of
> standardizing on OF device tree for x86 embedded.  dwmw2 suggested
> translating data in other formats into the device tree during early
> boot so that existing OF routines can be used to parse the data by
> device drivers.  That would keep the support code in the device
> drivers where it belongs without creating new SFI-specific device
> infrastructure.  It also eliminates the statically allocated tables
> and board specific hacks.
> 
> Since data in the tree is free-form key-value properties, any
> device-specific SFI data can be imported into the tree verbatim to
> eliminate the risk that critical data will get discarded in the
> translation process.  Well understood properties, like address ranges,
> IRQs and GPIOs would be straight forward to translate into the device
> tree form to make it parsable by common code.

What Grant said.

This patch (and SFI in general) seems like entirely the wrong approach.
You're gratuitously reinventing the wheel, and your proposed replacement
isn't even round.

As Grant says, we already have a defined method for passing this kind of
information to the kernel. We have well-thought-out bindings for various
different types of devices -- which have been designed to ensure that
we're describing the *hardware* and not specifically tied to
implementation details in the current Linux driver setup.

It makes *no* sense to add probe support for your 'special' platform
devices to drivers which already have (or will need anyway) proper
device-tree support.

Seriously, just convert whatever crap you have into a device-tree at
early boot (or preferably in the bootloader, and *beat* the firmware
idiots until they provide it natively), and don't pollute the kernel any
more than you have to with this idiocy.

-- 
dwmw2

Re: [PATCH] x86/mrst: add SFI platform device parsing code

[Mark Brown]

On Wed, Sep 22, 2010 at 04:22:13PM +0100, David Woodhouse wrote:

> It makes *no* sense to add probe support for your 'special' platform
> devices to drivers which already have (or will need anyway) proper
> device-tree support.

To be fair they're not doing this, all this code is in the arch code
rather than the driver code (which is a separate concern).

> Seriously, just convert whatever crap you have into a device-tree at
> early boot (or preferably in the bootloader, and *beat* the firmware
> idiots until they provide it natively), and don't pollute the kernel any
> more than you have to with this idiocy.

This is roughly what it's doing except that it's going to native Linux
bus types and platform data rather than to device tree.

Re: [PATCH] x86/mrst: add SFI platform device parsing code

[David Woodhouse]

On Wed, 2010-09-22 at 16:33 +0100, Mark Brown wrote:
> On Wed, Sep 22, 2010 at 04:22:13PM +0100, David Woodhouse wrote:
> 
> > It makes *no* sense to add probe support for your 'special' platform
> > devices to drivers which already have (or will need anyway) proper
> > device-tree support.
> 
> To be fair they're not doing this, all this code is in the arch code
> rather than the driver code (which is a separate concern).
> 
> > Seriously, just convert whatever crap you have into a device-tree at
> > early boot (or preferably in the bootloader, and *beat* the firmware
> > idiots until they provide it natively), and don't pollute the kernel any
> > more than you have to with this idiocy.
> 
> This is roughly what it's doing except that it's going to native Linux
> bus types and platform data rather than to device tree.

It still means that drivers for chips which are seen both on this
bizarre abomination and also on other machines which use a device-tree
will need to support *both* probe types.

-- 
dwmw2

Re: [PATCH] x86/mrst: add SFI platform device parsing code

[Mark Brown]

On Wed, Sep 22, 2010 at 04:35:17PM +0100, David Woodhouse wrote:
> On Wed, 2010-09-22 at 16:33 +0100, Mark Brown wrote:

> > This is roughly what it's doing except that it's going to native Linux
> > bus types and platform data rather than to device tree.

> It still means that drivers for chips which are seen both on this
> bizarre abomination and also on other machines which use a device-tree
> will need to support *both* probe types.

No, the drivers don't see any of this but instead someone has to
write (and maintain) explicit code to do the translation in arch/x86
outside the drivers.  The drivers just see platform data and regular bus
probes which they'll be doing anyway until such time as device tree is
well enough established for people to stop doing platform data.

Re: [PATCH] x86/mrst: add SFI platform device parsing code

[Alan Cox]

> This patch (and SFI in general) seems like entirely the wrong
> approach. You're gratuitously reinventing the wheel, and your
> proposed replacement isn't even round.

SFI is what is implemented on these platforms. SFI is what you get. Now
it's possible the firmware folks may move away from SFI for future
hardware but that remains 

> As Grant says, we already have a defined method for passing this kind
> of information to the kernel. We have well-thought-out bindings for

No we don't. The existing drivers we need don't support it.

> It makes *no* sense to add probe support for your 'special' platform
> devices to drivers which already have (or will need anyway) proper
> device-tree support.

I've not seen any which have device tree support that are being used.

> Seriously, just convert whatever crap you have into a device-tree at
> early boot (or preferably in the bootloader, and *beat* the firmware
> idiots until they provide it natively), and don't pollute the kernel
> any more than you have to with this idiocy.

Thats why its in arch/x86/kernel/mrst.c - an arch private file specific
to the platform which then provides platform data in the format drivers
already use.

It's really quite simple

If drivers expect device tree the mrst.c code can provide device tree -
but in the real world none we care about do so yet. If stuff moves to
device tree then the translation will move that way because its job is
to convert SFI to what the kernel wants.

Right now the kernel wants a mix of private structs and as far as
possible platform_foo() stuff. When that is changes the rest will
follow.

Alan

Re: [PATCH] x86/mrst: add SFI platform device parsing code

[Alan Cox]

> Okay, before I start a long rant I'll start with this disclaimer; at
> least this patch is isolated to the mrst platform so that it doesn't
> impact the rest of the kernel.  Feel free to take the stance that this
> is platform specific code and therefore will never have to handle the
> general-case of multiple machines.  In that case each machine will
> need to have a separate copy of this functionality, but at least the
> damage is contained. </disclaim>

Simple answer to this. It's intended that SFI will be used on certain
Intel MID platforms. I am hopeful that something more sensible will
happen with future firmware for future platforms but for existing MID
platforms its not negotiable, we either support SFI or we don't run on
them.

> This will be an utter nightmare to maintain over the long term.  It

Intel will be maintaining it. While I can't speak for Intel on the
matter my personal way of seeing this is "our turd, our shovel" and its
localised so there will be no funny smells elsewhere.

> combines the worst parts of both static device tables and firmware
> data parsers.  From the former it uses large statically allocated
> tables that cannot be either extended or freed at runtime which is
> unfriendly for multiplatform kernels.

I know this. I've been managing it internally for a while. I have a
deeper understanding than I want but the SFI side is fixed and better
firmware design is a thing that you talk about now for products 'n'
years down the line because of the way all the
design/bringup/development for plaforms works.

In truth SFI is closer to being a device enumeration than a
configuration description.

> adding another.  It is already an uphill battle to convince device
> driver maintainers that adding device tree parsing code (appropriately
> contained) to .probe() hooks is the right thing to do.  Asking them to
> also support SFI hooks (plus the next firmware-interface-of-the-month)
> is likely to end in a revolt.

You need to look at the code again. There is *no* SFI specific hookery
in drivers. In fact I have been going around removing that from code as
we submit drivers upstream  - and rightfully if I didn't the i²c
maintainers and the like would reject the code.

If a driver wants device tree the SFI parser would ideally supply it
with device tree. If the entire kernel goes device tree I will whoop
with joy and make SFI use it.

> Since data in the tree is free-form key-value properties, any
> device-specific SFI data can be imported into the tree verbatim to
> eliminate the risk that critical data will get discarded in the
> translation process.  Well understood properties, like address ranges,
> IRQs and GPIOs would be straight forward to translate into the device
> tree form to make it parsable by common code.

In the idea world SFI would be replaced with a self describing
expandable data format that translated neatly into whatever we needed,
and the kernel drivers we are re-using would have corresponding
interfaces. Lots of people understand this, but it simply won't happen
for these existing SFI platforms.

In the real world they don't - yet, but if they do we'll be happy to
join the revolution.

Alan

Re: [PATCH] x86/mrst: add SFI platform device parsing code

[Grant Likely]

On Wed, Sep 22, 2010 at 11:15:14PM +0100, Alan Cox wrote:
> > Okay, before I start a long rant I'll start with this disclaimer; at
> > least this patch is isolated to the mrst platform so that it doesn't
> > impact the rest of the kernel.  Feel free to take the stance that this
> > is platform specific code and therefore will never have to handle the
> > general-case of multiple machines.  In that case each machine will
> > need to have a separate copy of this functionality, but at least the
> > damage is contained. </disclaim>
> 
> Simple answer to this. It's intended that SFI will be used on certain
> Intel MID platforms. I am hopeful that something more sensible will
> happen with future firmware for future platforms but for existing MID
> platforms its not negotiable, we either support SFI or we don't run on
> them.
> 
> > This will be an utter nightmare to maintain over the long term.  It
> 
> Intel will be maintaining it. While I can't speak for Intel on the
> matter my personal way of seeing this is "our turd, our shovel" and its
> localised so there will be no funny smells elsewhere.
> 
> > combines the worst parts of both static device tables and firmware
> > data parsers.  From the former it uses large statically allocated
> > tables that cannot be either extended or freed at runtime which is
> > unfriendly for multiplatform kernels.
> 
> I know this. I've been managing it internally for a while. I have a
> deeper understanding than I want but the SFI side is fixed and better
> firmware design is a thing that you talk about now for products 'n'
> years down the line because of the way all the
> design/bringup/development for plaforms works.
> 
> In truth SFI is closer to being a device enumeration than a
> configuration description.
> 
> > adding another.  It is already an uphill battle to convince device
> > driver maintainers that adding device tree parsing code (appropriately
> > contained) to .probe() hooks is the right thing to do.  Asking them to
> > also support SFI hooks (plus the next firmware-interface-of-the-month)
> > is likely to end in a revolt.
> 
> You need to look at the code again. There is *no* SFI specific hookery
> in drivers. In fact I have been going around removing that from code as
> we submit drivers upstream  - and rightfully if I didn't the i??c
> maintainers and the like would reject the code.

Heh; impedance mismatch.  Let me clarify...

I'm in full agreement that SFI platforms exist now and must be
supported (firmware replacement is not an option).  I also understand
that this patch is fully contained withing the moorestown platform
code.  It is obvious that pains were taken to ensure zero impact on
existing device drivers (hence my original disclaimer).

My point is that an isolated translation layer becomes
(exponentially?) more difficult to maintain for each additional
platform that depends on it.  Support must be explicitly added for
every device that gets attached to a moorestown system in a way that
cannot be modularized and has the potential to become very large.

How to interpret device specific data, including interpreting the
binding name, really does belong in device driver.  The SFI data
should be exported to the driver where intelligent decisions can be
made.

I think we both agree that calling a new API from drivers for each and
every firmware interface is a path of madness.  Instead, I was
suggesting dumping the SFI data into device tree properties and using
the existing API with an SFI-specific namespace, however...

I just spent some time looking closer at what SFI is providing and
reading the spec.  Wow.  Okay, I take it all back.  I don't see how
any of this code could ever be made generic.  The spec seems to only
cover the data structure, and relationships between devices seem to be
left to whatever convention the firmware writer feels like using, or
am I missing something?

>From what I've seen tonight, dumping SFI data verbatim into a device
tree isn't an option because SFI doesn't naturally group device data.
I withdraw the suggestion.  So, I guess I agree.  There doesn't seem
to be any way around machine-specific SFI translation code, regardless
of whether it translates into direct registrations, or into a device
tree.


> If a driver wants device tree the SFI parser would ideally supply it
> with device tree. If the entire kernel goes device tree I will whoop
> with joy and make SFI use it.

:-)

> > Since data in the tree is free-form key-value properties, any
> > device-specific SFI data can be imported into the tree verbatim to
> > eliminate the risk that critical data will get discarded in the
> > translation process.  Well understood properties, like address ranges,
> > IRQs and GPIOs would be straight forward to translate into the device
> > tree form to make it parsable by common code.
> 
> In the idea world SFI would be replaced with a self describing
> expandable data format that translated neatly into whatever we needed,
> and the kernel drivers we are re-using would have corresponding
> interfaces. Lots of people understand this, but it simply won't happen
> for these existing SFI platforms.
> 
> In the real world they don't - yet, but if they do we'll be happy to
> join the revolution.
> 
> Alan

Re: [PATCH] x86/mrst: add SFI platform device parsing code

[Mark Brown]

On Thu, Sep 23, 2010 at 03:07:03AM -0300, Grant Likely wrote:

> My point is that an isolated translation layer becomes
> (exponentially?) more difficult to maintain for each additional
> platform that depends on it.  Support must be explicitly added for
> every device that gets attached to a moorestown system in a way that
> cannot be modularized and has the potential to become very large.

It's not just the per-device aspect of it, it's also what happens when
OEMs start placing devices with no upstream defined mappings on their
boards.  Regardless of any specs it seems unlikely that OEMs can be
reliably persuaded to coordinate the creation of new SFI mappings for
these devices with anyone which means that we're likely to end up with
multiple incompatible definitions.  Looking at the code as it is I don't
see any structure in there for handling that - it's assuming that we
know what the platform data for a given device is going to look like.

> From what I've seen tonight, dumping SFI data verbatim into a device
> tree isn't an option because SFI doesn't naturally group device data.
> I withdraw the suggestion.  So, I guess I agree.  There doesn't seem
> to be any way around machine-specific SFI translation code, regardless
> of whether it translates into direct registrations, or into a device
> tree.

One thing that might help here is to put in place the mechanisms for
adding board-specific overrides now (I'm presuming there's board naming
data in the SFI somewhere, I've not looked).  That should hopefully
provide some hints to people and would at least mean we've got something
to hang machine specific workarounds on.

To be honest I'd actually be inclined to go more towards the way the
non-DT embedded platforms have gone and just ignore the data we get
from SFI as much as possible and have board specific initialisation in
code; it's boring and repetitive but it's also clear and *relatively*
robust.

> > If a driver wants device tree the SFI parser would ideally supply it
> > with device tree. If the entire kernel goes device tree I will whoop
> > with joy and make SFI use it.

> :-)

To be honest I suspect we'll have similar issue with or without device
tree.

Re: [PATCH] x86/mrst: add SFI platform device parsing code

[Alan Cox]

> So the expectation is that the platform data parser functions in the
> SFI device list will be querying the DMI data and selecting the actual
> parsing based on that?  Perhaps adding DMI keys to the match tables so
> the infrastructure is there for doing the device specific things would
> cover it; minor differences could be worked out in the default match
> function and we can drop back to board specific functions when it gets
> too messy.

That will just encourage people not to be careful.

> My main issue here is that the code is working on the assumption that
> we have one standard idea of what the SFI data means and provides no
> guidance to users about handling the inevitable variations.

There should be no variations and the nature of the platform means that
might even work out. I don't really want to add it to the table unless
we have lots needing DMI data. Right now we don't and there are
multiple platform implementations in existence.

Alan

Re: [PATCH] x86/mrst: add SFI platform device parsing code

[Alan Cox]

> To be honest I'd actually be inclined to go more towards the way the
> non-DT embedded platforms have gone and just ignore the data we get
> from SFI as much as possible and have board specific initialisation in
> code; it's boring and repetitive but it's also clear and *relatively*
> robust.

And even more unmaintainable. Won't happen.

Alan

Re: [PATCH] x86/mrst: add SFI platform device parsing code

[Mark Brown]

On Thu, Sep 23, 2010 at 11:27:03AM +0100, Alan Cox wrote:

> > To be honest I'd actually be inclined to go more towards the way the
> > non-DT embedded platforms have gone and just ignore the data we get
> > from SFI as much as possible and have board specific initialisation in
> > code; it's boring and repetitive but it's also clear and *relatively*
> > robust.

> And even more unmaintainable. Won't happen.

So what is the plan for coping with OEM systems?  Right now the code
makes no provision at all that I can see for system-specific handling
of the SFI data which seems very optimistic.

Re: [PATCH] x86/mrst: add SFI platform device parsing code

[Alan Cox]

> any of this code could ever be made generic.  The spec seems to only
> cover the data structure, and relationships between devices seem to be
> left to whatever convention the firmware writer feels like using, or
> am I missing something?

Not really no. It's basically little more than a component and IRQ list.
The GPIO table in 0.8 exposes the GPIO lines a bit more sanely.

In fact all that SFI provides generically pretty much fits into
platform_foo() already because there is so little - or into the I²C or
SPI bus data.

Alan

Re: [PATCH] x86/mrst: add SFI platform device parsing code

[Mark Brown]

On Thu, Sep 23, 2010 at 11:58:18AM +0100, Alan Cox wrote:
> Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:

> > So what is the plan for coping with OEM systems?  Right now the code
> > makes no provision at all that I can see for system-specific handling
> > of the SFI data which seems very optimistic.

> There are various system specific drivers which have their own sfi entry
> and name. Telling platforms apart is a problem with SFI but the latest
> firmware also supports DMI so we can get platform identification via the
> normal PC interfaces.

So the expectation is that the platform data parser functions in the SFI
device list will be querying the DMI data and selecting the actual
parsing based on that?  Perhaps adding DMI keys to the match tables so
the infrastructure is there for doing the device specific things would
cover it; minor differences could be worked out in the default match
function and we can drop back to board specific functions when it gets
too messy.

My main issue here is that the code is working on the assumption that we
have one standard idea of what the SFI data means and provides no
guidance to users about handling the inevitable variations.

Re: [PATCH] x86/mrst: add SFI platform device parsing code

[Mark Brown]

On Thu, Sep 23, 2010 at 11:48:45AM +0100, Alan Cox wrote:
> > any of this code could ever be made generic.  The spec seems to only
> > cover the data structure, and relationships between devices seem to be
> > left to whatever convention the firmware writer feels like using, or
> > am I missing something?

> Not really no. It's basically little more than a component and IRQ list.
> The GPIO table in 0.8 exposes the GPIO lines a bit more sanely.

> In fact all that SFI provides generically pretty much fits into
> platform_foo() already because there is so little - or into the I²C or
> SPI bus data.

I believe the issue here is the handling of things where the
functionality Linux needs is implemented over multiple chips so we need
a way for the chips to find each other.

Re: [PATCH] x86/mrst: add SFI platform device parsing code

[Alan Cox]

On Thu, 23 Sep 2010 11:27:08 +0100
Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:

> On Thu, Sep 23, 2010 at 11:27:03AM +0100, Alan Cox wrote:
> 
> > > To be honest I'd actually be inclined to go more towards the way the
> > > non-DT embedded platforms have gone and just ignore the data we get
> > > from SFI as much as possible and have board specific initialisation in
> > > code; it's boring and repetitive but it's also clear and *relatively*
> > > robust.
> 
> > And even more unmaintainable. Won't happen.
> 
> So what is the plan for coping with OEM systems?  Right now the code
> makes no provision at all that I can see for system-specific handling
> of the SFI data which seems very optimistic.

There are various system specific drivers which have their own sfi entry
and name. Telling platforms apart is a problem with SFI but the latest
firmware also supports DMI so we can get platform identification via the
normal PC interfaces.

Alan

Re: [PATCH] x86/mrst: add SFI platform device parsing code

[Alan Cox]

> > There should be no variations and the nature of the platform means
> > that might even work out. I don't really want to add it to the
> > table unless we have lots needing DMI data. Right now we don't and
> > there are multiple platform implementations in existence.
> 
> What is it about this platform that is going to restrict the problem?

The sort of people who will be using it and how,

> Code which makes this sort of assumption about knowing the platforms
> that the device will be deployed on well is relatively common but the
> usual result is that OEMs want to change the reference platforms and
> the assumptions that the code has been making about the systems and
> about how people will work with the code break down.

There are non reference platforms in existence without the problem you
envisage having occurred. So I think we'll worry about it if it happens
but knowing that with DMI we have the tools to deal with this.

Alan

Re: [PATCH] x86/mrst: add SFI platform device parsing code

[Mark Brown]

On Thu, Sep 23, 2010 at 11:13:47AM +0100, Alan Cox wrote:

> > So the expectation is that the platform data parser functions in the
> > SFI device list will be querying the DMI data and selecting the actual
> > parsing based on that?  Perhaps adding DMI keys to the match tables so
> > the infrastructure is there for doing the device specific things would

> That will just encourage people not to be careful.

I'm not see anything here which provides a way for people to be careful
- people just need to add the code directly into the parser if there's
no format defined in the kernel the're working on.  If (as will be the
case with most chips) there's nothing there then people are pretty much
left to their own devices unless they choose to work upstream.  From
experience what the overwheming majority of OEMs will actually do is
work on the latest kernel from their distro of choice, normally without
contributing their changes anywhere.

> > My main issue here is that the code is working on the assumption that
> > we have one standard idea of what the SFI data means and provides no
> > guidance to users about handling the inevitable variations.

> There should be no variations and the nature of the platform means that
> might even work out. I don't really want to add it to the table unless
> we have lots needing DMI data. Right now we don't and there are
> multiple platform implementations in existence.

What is it about this platform that is going to restrict the problem?

Code which makes this sort of assumption about knowing the platforms
that the device will be deployed on well is relatively common but the
usual result is that OEMs want to change the reference platforms and the
assumptions that the code has been making about the systems and about
how people will work with the code break down.  This problem is much
more pronounced in the embedded space where the hardware is much less
self-describing and regular than on PCs.

Re: [PATCH] x86/mrst: add SFI platform device parsing code

[Mark Brown]

On Thu, Sep 23, 2010 at 02:27:20PM +0100, Alan Cox wrote:

> > What is it about this platform that is going to restrict the problem?

> The sort of people who will be using it and how,

Can you be more specific?  This is the same sort of thing people
normally say but the experience is that it rarely survives a successful
product.

> > Code which makes this sort of assumption about knowing the platforms
> > that the device will be deployed on well is relatively common but the
> > usual result is that OEMs want to change the reference platforms and
> > the assumptions that the code has been making about the systems and
> > about how people will work with the code break down.

> There are non reference platforms in existence without the problem you
> envisage having occurred. So I think we'll worry about it if it happens
> but knowing that with DMI we have the tools to deal with this.

It doesn't break down immediately; a combination of volume and the next
generation product coming along and grabbing attention is usually what
causes things to break down.  However, if you're totally confident that
all the BIOS authors at the OEMs are going to work well together here
then I guess it's only Moorestown that's affected.

Re: [PATCH] x86/mrst: add SFI platform device parsing code

[Alan Cox]

> usually what causes things to break down.  However, if you're totally
> confident that all the BIOS authors at the OEMs are going to work
> well together here then I guess it's only Moorestown that's affected.

I'm not 100% confident which is why we lobbied hard to get DMI into the
firmware, but I think there is a good chance the SFI stuff will be
fine. My bigger worry to be honest is going to be people adding stuff
which isn't even in firmware enumerations.