[PATCH] arch/tile: add arch/tile/drivers/ directory with SROM driver

[PATCH] arch/tile: add arch/tile/drivers/ directory with SROM driver

[Chris Metcalf]

This commit does two things: it adds the infrastructure for a new
arch/tile/drivers/ directory, and it populates it with the first
instance of a driver for that directory, a SPI Flash ROM (srom) driver.

The directory is motivated as follows.  While some classes of
driver implementations should be grouped together so they are easily
modified as a class (network, ATA, RTC, PCI, I2C, etc etc) there are
"miscellaneous" drivers that don't benefit from any sharing with other
driver implementations.  If those drivers are for hardware that can
plausibly be used by multiple architectures, it makes sense to put
them somewhere like drivers/char.  But if they are only usable on
a single architecture (in this case drivers written for the Tilera
para-virtualization model using our hypervisor) it makes sense to group
such drivers with their architecture, to avoid cluttering the "drivers"
hierarchy for other architectures that can't use that driver.

The actual SROM driver is fairly uncontroversial, and is just a simple
driver that allows user space to read and write the SROM at a raw level.
(A separate MTD driver exists for "tile", but this is not that driver.)
The driver is particularly useful since the Tile chip can boot directly
from the SROM, so providing this driver interface allows for updating
the boot image prior to a reboot.

Signed-off-by: Chris Metcalf <cmetcalf@tilera.com>
---
 arch/tile/Kconfig                    |    2 +
 arch/tile/Makefile                   |    2 +
 arch/tile/configs/tile_defconfig     |    1 +
 arch/tile/configs/tilegx_defconfig   |    1 +
 arch/tile/drivers/Kconfig            |    8 +
 arch/tile/drivers/Makefile           |   13 +
 arch/tile/drivers/srom.c             |  499 ++++++++++++++++++++++++++++++++++
 arch/tile/include/hv/drv_srom_intf.h |   41 +++
 8 files changed, 567 insertions(+), 0 deletions(-)
 create mode 100644 arch/tile/drivers/Kconfig
 create mode 100644 arch/tile/drivers/Makefile
 create mode 100644 arch/tile/drivers/srom.c
 create mode 100644 arch/tile/include/hv/drv_srom_intf.h

diff --git a/arch/tile/Kconfig b/arch/tile/Kconfig
index 635e1bf..be47e07 100644
--- a/arch/tile/Kconfig
+++ b/arch/tile/Kconfig
@@ -315,6 +315,8 @@ config KERNEL_PL
 	  kernel will be built to run at.  Generally you should use
 	  the default value here.
 
+source "arch/tile/drivers/Kconfig"
+
 endmenu  # Tilera-specific configuration
 
 menu "Bus options"
diff --git a/arch/tile/Makefile b/arch/tile/Makefile
index 17acce7..d7f778c 100644
--- a/arch/tile/Makefile
+++ b/arch/tile/Makefile
@@ -50,6 +50,8 @@ head-y		:= arch/tile/kernel/head_$(BITS).o
 libs-y		+= arch/tile/lib/
 libs-y		+= $(LIBGCC_PATH)
 
+drivers-y	+= arch/tile/drivers/
+
 # See arch/tile/Kbuild for content of core part of the kernel
 core-y		+= arch/tile/
 
diff --git a/arch/tile/configs/tile_defconfig b/arch/tile/configs/tile_defconfig
index cb47612..041c456 100644
--- a/arch/tile/configs/tile_defconfig
+++ b/arch/tile/configs/tile_defconfig
@@ -224,6 +224,7 @@ CONFIG_DEFAULT_MMAP_MIN_ADDR=4096
 CONFIG_VMALLOC_RESERVE=0x1000000
 CONFIG_HARDWALL=y
 CONFIG_KERNEL_PL=1
+CONFIG_TILE_SROM=y
 
 #
 # Bus options
diff --git a/arch/tile/configs/tilegx_defconfig b/arch/tile/configs/tilegx_defconfig
index 776ec0c..6343962 100644
--- a/arch/tile/configs/tilegx_defconfig
+++ b/arch/tile/configs/tilegx_defconfig
@@ -251,6 +251,7 @@ CONFIG_DEFAULT_MMAP_MIN_ADDR=4096
 CONFIG_VMALLOC_RESERVE=0x1000000
 CONFIG_HARDWALL=y
 CONFIG_KERNEL_PL=1
+CONFIG_TILE_SROM=y
 
 #
 # Bus options
diff --git a/arch/tile/drivers/Kconfig b/arch/tile/drivers/Kconfig
new file mode 100644
index 0000000..e1f9551
--- /dev/null
+++ b/arch/tile/drivers/Kconfig
@@ -0,0 +1,8 @@
+config TILE_SROM
+	bool "Character-device access to the Tilera on-board SROM"
+	default y
+	---help---
+	  This device provides character-level read-write access
+	  to the SROM, typically via the "0", "1", "2", and "info"
+	  devices in /dev/srom/.
+
diff --git a/arch/tile/drivers/Makefile b/arch/tile/drivers/Makefile
new file mode 100644
index 0000000..311e741
--- /dev/null
+++ b/arch/tile/drivers/Makefile
@@ -0,0 +1,13 @@
+#
+# Makefile for TILE drivers that don't belong elsewhere in the tree.
+# Typically these are drivers that just present a simple char
+# file_operations interface.
+#
+# For other Tilera-specific drivers, see:
+#
+#  drivers/net/tile/			network
+#  drivers/tty/hvc/hvc_tile.c		hypervisor console
+#  drivers/edac/tile_edac.c		EDAC driver
+#
+
+obj-$(CONFIG_TILE_SROM)		+= srom.o
diff --git a/arch/tile/drivers/srom.c b/arch/tile/drivers/srom.c
new file mode 100644
index 0000000..1a5b070
--- /dev/null
+++ b/arch/tile/drivers/srom.c
@@ -0,0 +1,499 @@
+/*
+ * Copyright 2011 Tilera Corporation. All Rights Reserved.
+ *
+ *   This program is free software; you can redistribute it and/or
+ *   modify it under the terms of the GNU General Public License
+ *   as published by the Free Software Foundation, version 2.
+ *
+ *   This program is distributed in the hope that it will be useful, but
+ *   WITHOUT ANY WARRANTY; without even the implied warranty of
+ *   MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or
+ *   NON INFRINGEMENT.  See the GNU General Public License for
+ *   more details.
+ *
+ * SPI Flash ROM driver
+ *
+ * This source code is derived from code provided in "Linux Device
+ * Drivers" by Alessandro Rubini and Jonathan Corbet, published by
+ * O'Reilly & Associates.
+ */
+
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/init.h>
+#include <linux/kernel.h>	/* printk() */
+#include <linux/slab.h>		/* kmalloc() */
+#include <linux/fs.h>		/* everything... */
+#include <linux/errno.h>	/* error codes */
+#include <linux/types.h>	/* size_t */
+#include <linux/proc_fs.h>
+#include <linux/fcntl.h>	/* O_ACCMODE */
+#include <linux/aio.h>
+#include <linux/pagemap.h>
+#include <linux/hugetlb.h>
+#include <linux/uaccess.h>
+#include <hv/hypervisor.h>
+#include <linux/ioctl.h>
+#include <linux/cdev.h>
+#include <linux/delay.h>
+#include <hv/drv_srom_intf.h>
+
+/*
+ * Size of our hypervisor I/O requests.  We break up large transfers
+ * so that we don't spend large uninterrupted spans of time in the
+ * hypervisor.  Erasing an SROM sector takes a significant fraction of
+ * a second, so if we allowed the user to, say, do one I/O to write the
+ * entire ROM, we'd get soft lockup timeouts, or worse.
+ */
+#define SROM_CHUNK_SIZE 4096
+
+/*
+ * When hypervisor is busy (e.g. erasing), poll the status periodically.
+ */
+
+/*
+ * Interval to poll the state in msec
+ */
+#define SROM_WAIT_TRY_INTERVAL 20
+
+/*
+ * Maximum times to poll the state
+ */
+#define SROM_MAX_WAIT_TRY_TIMES 1000
+
+struct srom_dev {
+	struct cdev cdev;		/* Character device structure */
+	int hv_devhdl;			/* Handle for hypervisor device */
+	u32 size;		        /* Size of this device */
+	char *info_string;		/* String returned by the info device
+					   if this is it; NULL otherwise */
+	struct mutex srom_lock;		/* Allow only one accessor at a time */
+};
+
+static int srom_major;		/* Dynamic major by default */
+static int srom_devs =    4;	/* One per SROM partition plus one info file */
+
+/* Minor number of the info file */
+static inline int srom_info_minor(void)
+{
+	return srom_devs - 1;
+}
+
+module_param(srom_major, int, 0);
+module_param(srom_devs, int, 0);
+MODULE_AUTHOR("Tilera Corporation");
+MODULE_LICENSE("Dual BSD/GPL");
+
+static struct srom_dev *srom_devices; /* allocated in srom_init */
+
+/**
+ * Macro to complete a read/write transaction. Returns last hv
+ * syscall return value.
+ */
+#define SROM_RD_OR_WR(func, ...) \
+	({								\
+	int _hv_retval = func(__VA_ARGS__);				\
+	int retries;							\
+									\
+	while (_hv_retval < 0) {					\
+		if (_hv_retval == HV_EBUSY) {				\
+			retries = SROM_MAX_WAIT_TRY_TIMES;		\
+									\
+			while (retries > 0) {				\
+				_hv_retval = func(__VA_ARGS__);		\
+				if (_hv_retval == HV_EBUSY) {		\
+					msleep(SROM_WAIT_TRY_INTERVAL);	\
+					retries--;			\
+				} else {				\
+					break;				\
+				}					\
+			}						\
+									\
+			if (_hv_retval == HV_EBUSY) {			\
+				pr_err("srom: "#func" failed ("		\
+				       "timeout)\n");			\
+				retval = -EIO;				\
+				goto op_done;				\
+			}						\
+		} else if (_hv_retval == HV_EAGAIN) {			\
+			_hv_retval = func(__VA_ARGS__);			\
+		} else {						\
+			pr_err("srom: "#func" failed, "			\
+				"error %d\n", _hv_retval);		\
+			retval = -EIO;					\
+			goto op_done;					\
+		}							\
+	}								\
+	_hv_retval;							\
+	})
+
+/**
+ * srom_open() - Device open routine.
+ * @inode: Inode for this device.
+ * @filp: File for this specific open of the device.
+ *
+ * Returns zero, or an error code.
+ */
+static int srom_open(struct inode *inode, struct file *filp)
+{
+	struct srom_dev *dev;
+
+	/* Find the device */
+	dev = container_of(inode->i_cdev, struct srom_dev, cdev);
+
+	/* Now open the hypervisor device if we haven't already. */
+	if (dev->hv_devhdl == 0) {
+		char buf[20];
+		int instance = iminor(inode);
+		if (instance != srom_info_minor()) {
+			sprintf(buf, "srom/0/%d", instance);
+			dev->hv_devhdl = hv_dev_open((HV_VirtAddr)buf, 0);
+			if (dev->hv_devhdl > 0) {
+				dev->size = 0;
+				if (mutex_lock_interruptible(&dev->srom_lock))
+					return -ERESTARTSYS;
+				hv_dev_pread(dev->hv_devhdl, 0,
+					     (HV_VirtAddr)&dev->size,
+					     sizeof(dev->size),
+					     SROM_TOTAL_SIZE_OFF);
+				mutex_unlock(&dev->srom_lock);
+			}
+		} else {
+			u32 sector_size = 0;
+			u32 page_size = 0;
+
+			sprintf(buf, "srom/0/0");
+			dev->hv_devhdl = hv_dev_open((HV_VirtAddr)buf, 0);
+			if (dev->hv_devhdl > 0) {
+				static const int info_string_size = 80;
+
+				if (mutex_lock_interruptible(&dev->srom_lock))
+					return -ERESTARTSYS;
+				hv_dev_pread(dev->hv_devhdl, 0,
+					     (HV_VirtAddr)&sector_size,
+					     sizeof(sector_size),
+					     SROM_SECTOR_SIZE_OFF);
+				hv_dev_pread(dev->hv_devhdl, 0,
+					     (HV_VirtAddr)&page_size,
+					     sizeof(page_size),
+					     SROM_PAGE_SIZE_OFF);
+				mutex_unlock(&dev->srom_lock);
+
+				dev->info_string = kmalloc(info_string_size,
+							GFP_KERNEL);
+				snprintf(dev->info_string, info_string_size,
+					"sector_size: %d\npage_size: %d\n",
+					sector_size, page_size);
+			}
+		}
+	}
+
+	/* If we tried and failed to open it, fail. */
+	if (dev->hv_devhdl < 0) {
+		switch (dev->hv_devhdl)	{
+		case HV_ENODEV:
+			return -ENODEV;
+		default:
+			return (ssize_t)dev->hv_devhdl;
+		}
+	}
+
+	filp->private_data = dev;
+
+	return 0;          /* success */
+}
+
+
+/**
+ * srom_release() - Device release routine.
+ * @inode: Inode for this device.
+ * @filp: File for this specific open of the device.
+ *
+ * Returns zero, or an error code.
+ */
+static int srom_release(struct inode *inode, struct file *filp)
+{
+	struct srom_dev *dev = filp->private_data;
+	int retval = 0;
+	char dummy;
+
+	/*
+	 * First we need to make sure we've flushed anything written to
+	 * the ROM.
+	 */
+	if (mutex_lock_interruptible(&dev->srom_lock)) {
+		retval = -ERESTARTSYS;
+		filp->private_data = NULL;
+		return retval;
+	}
+
+	if (dev->hv_devhdl > 0) {
+		SROM_RD_OR_WR(hv_dev_pwrite, dev->hv_devhdl,
+			      0, (HV_VirtAddr)&dummy, 1, SROM_FLUSH_OFF);
+	}
+
+op_done:
+	mutex_unlock(&dev->srom_lock);
+	filp->private_data = NULL;
+
+	return retval;
+}
+
+
+/**
+ * srom_read() - Read (control) data from the device.
+ * @filp: File for this specific open of the device.
+ * @buf: User's data buffer.
+ * @count: Number of bytes requested.
+ * @f_pos: File position.
+ *
+ * Returns number of bytes read, or an error code.
+ */
+static ssize_t srom_read(struct file *filp, char __user *buf,
+			 size_t count, loff_t *f_pos)
+{
+	int retval = 0;
+	void *kernbuf;
+	struct srom_dev *dev = filp->private_data;
+
+	if (dev->hv_devhdl < 0)
+		return -EINVAL;
+
+	if (dev->info_string) {
+		int info_len = strlen(dev->info_string);
+		int bytes_avail = info_len - *f_pos;
+		int xfer_len = (bytes_avail < count) ? bytes_avail : count;
+
+		if (xfer_len <= 0)
+			return 0;
+
+		if (copy_to_user(buf, dev->info_string + *f_pos, xfer_len))
+			return -EFAULT;
+		*f_pos += xfer_len;
+		return xfer_len;
+	}
+
+	kernbuf = kmalloc(SROM_CHUNK_SIZE, GFP_KERNEL);
+	if (!kernbuf)
+		return -ENOMEM;
+
+	if (mutex_lock_interruptible(&dev->srom_lock)) {
+		retval = -ERESTARTSYS;
+		kfree(kernbuf);
+		return retval;
+	}
+
+	while (count) {
+		int hv_retval;
+		int bytes_this_pass = count;
+
+		if (bytes_this_pass > SROM_CHUNK_SIZE)
+			bytes_this_pass = SROM_CHUNK_SIZE;
+
+		hv_retval = SROM_RD_OR_WR(hv_dev_pread, dev->hv_devhdl,
+			      0, (HV_VirtAddr) kernbuf, bytes_this_pass,
+			      *f_pos);
+
+		if (hv_retval > 0) {
+			if (copy_to_user(buf, kernbuf, hv_retval) != 0) {
+				retval = -EFAULT;
+				break;
+			}
+		} else if (hv_retval == 0) {
+			break;
+		}
+
+		retval += hv_retval;
+		*f_pos += hv_retval;
+		buf += hv_retval;
+		count -= hv_retval;
+	}
+
+op_done:
+	mutex_unlock(&dev->srom_lock);
+	kfree(kernbuf);
+
+	return retval;
+}
+
+/**
+ * srom_write() - Write (control) data to the device.
+ * @filp: File for this specific open of the device.
+ * @buf: User's data buffer.
+ * @count: Number of bytes requested.
+ * @f_pos: File position.
+ *
+ * Returns number of bytes written, or an error code.
+ */
+static ssize_t srom_write(struct file *filp, const char __user *buf,
+			  size_t count, loff_t *f_pos)
+{
+	int retval = 0;
+	void *kernbuf;
+	struct srom_dev *dev = filp->private_data;
+
+	if (dev->hv_devhdl < 0 || dev->info_string)
+		return -EINVAL;
+
+	kernbuf = kmalloc(SROM_CHUNK_SIZE, GFP_KERNEL);
+	if (!kernbuf)
+		return -ENOMEM;
+
+	if (mutex_lock_interruptible(&dev->srom_lock)) {
+		retval = -ERESTARTSYS;
+		kfree(kernbuf);
+		return retval;
+	}
+
+	while (count) {
+		int hv_retval;
+		int bytes_this_pass = count;
+
+		if (bytes_this_pass > SROM_CHUNK_SIZE)
+			bytes_this_pass = SROM_CHUNK_SIZE;
+
+		if (copy_from_user(kernbuf, buf, bytes_this_pass) != 0) {
+			retval = -EFAULT;
+			break;
+		}
+
+		hv_retval = SROM_RD_OR_WR(hv_dev_pwrite, dev->hv_devhdl,
+			      0, (HV_VirtAddr) kernbuf, bytes_this_pass,
+			      *f_pos);
+
+		if (hv_retval == 0)
+			break;
+
+		retval += hv_retval;
+		*f_pos += hv_retval;
+		buf += hv_retval;
+		count -= hv_retval;
+	}
+
+op_done:
+	mutex_unlock(&dev->srom_lock);
+	kfree(kernbuf);
+
+	return retval;
+}
+
+/**
+ * srom_llseek() - Change the current device offset.
+ * @filp: File for this specific open of the device.
+ * @off: New offset value.
+ * @whence: Base for new offset value.
+ *
+ * Returns new offset, or an error code.
+ */
+static loff_t srom_llseek(struct file *filp, loff_t off, int whence)
+{
+	struct srom_dev *dev = filp->private_data;
+	long newpos;
+
+	switch (whence) {
+	case 0: /* SEEK_SET */
+		newpos = off;
+		break;
+
+	case 1: /* SEEK_CUR */
+		newpos = filp->f_pos + off;
+		break;
+
+	case 2: /* SEEK_END */
+		newpos = dev->size + off;
+		break;
+
+	default: /* can't happen */
+		return -EINVAL;
+	}
+
+	if (newpos < 0 || newpos > dev->size)
+		return -EINVAL;
+
+	filp->f_pos = newpos;
+	return newpos;
+}
+
+
+/*
+ * The fops
+ */
+static const struct file_operations srom_fops = {
+	.owner =     THIS_MODULE,
+	.llseek =    srom_llseek,
+	.read =	     srom_read,
+	.write =     srom_write,
+	.open =	     srom_open,
+	.release =   srom_release,
+};
+
+/**
+ * srom_setup_cdev() - Set up a device instance in the cdev table.
+ * @dev: Per-device SROM state.
+ * @index: Device to set up.
+ */
+static void srom_setup_cdev(struct srom_dev *dev, int index)
+{
+	int err, devno = MKDEV(srom_major, index);
+
+	cdev_init(&dev->cdev, &srom_fops);
+	dev->cdev.owner = THIS_MODULE;
+	dev->cdev.ops = &srom_fops;
+	err = cdev_add(&dev->cdev, devno, 1);
+	/* Fail gracefully if need be */
+	if (err)
+		pr_notice("Error %d adding srom%d", err, index);
+}
+
+/** srom_init() - Initialize the driver's module. */
+static int srom_init(void)
+{
+	int result, i;
+	dev_t dev = MKDEV(srom_major, 0);
+
+	/*
+	 * Register our major, and accept a dynamic number.
+	 */
+	if (srom_major)
+		result = register_chrdev_region(dev, srom_devs, "srom");
+	else {
+		result = alloc_chrdev_region(&dev, 0, srom_devs, "srom");
+		srom_major = MAJOR(dev);
+	}
+	if (result < 0)
+		return result;
+
+
+	/*
+	 * Allocate the devices -- we can't have them static, as the number
+	 * can be specified at load time.
+	 */
+	srom_devices = kzalloc(srom_devs * sizeof(struct srom_dev),
+			       GFP_KERNEL);
+	if (!srom_devices) {
+		unregister_chrdev_region(dev, srom_devs);
+		return -ENOMEM;
+	}
+	for (i = 0; i < srom_devs; i++) {
+		srom_setup_cdev(srom_devices + i, i);
+		mutex_init(&srom_devices[i].srom_lock);
+	}
+
+	return 0; /* succeed */
+}
+
+/** srom_cleanup() - Clean up the driver's module. */
+static void srom_cleanup(void)
+{
+	int i;
+
+	for (i = 0; i < srom_devs; i++) {
+		cdev_del(&srom_devices[i].cdev);
+		kfree(srom_devices[i].info_string);
+	}
+	kfree(srom_devices);
+	unregister_chrdev_region(MKDEV(srom_major, 0), srom_devs);
+}
+
+module_init(srom_init);
+module_exit(srom_cleanup);
diff --git a/arch/tile/include/hv/drv_srom_intf.h b/arch/tile/include/hv/drv_srom_intf.h
new file mode 100644
index 0000000..6395faa
--- /dev/null
+++ b/arch/tile/include/hv/drv_srom_intf.h
@@ -0,0 +1,41 @@
+/*
+ * Copyright 2011 Tilera Corporation. All Rights Reserved.
+ *
+ *   This program is free software; you can redistribute it and/or
+ *   modify it under the terms of the GNU General Public License
+ *   as published by the Free Software Foundation, version 2.
+ *
+ *   This program is distributed in the hope that it will be useful, but
+ *   WITHOUT ANY WARRANTY; without even the implied warranty of
+ *   MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or
+ *   NON INFRINGEMENT.  See the GNU General Public License for
+ *   more details.
+ */
+
+/**
+ * @file drv_srom_intf.h
+ * Interface definitions for the SPI Flash ROM driver.
+ */
+
+#ifndef _SYS_HV_INCLUDE_DRV_SROM_INTF_H
+#define _SYS_HV_INCLUDE_DRV_SROM_INTF_H
+
+/** Read this offset to get the total device size. */
+#define SROM_TOTAL_SIZE_OFF   0xF0000000
+
+/** Read this offset to get the device sector size. */
+#define SROM_SECTOR_SIZE_OFF  0xF0000004
+
+/** Read this offset to get the device page size. */
+#define SROM_PAGE_SIZE_OFF    0xF0000008
+
+/** Write this offset to flush any pending writes. */
+#define SROM_FLUSH_OFF        0xF1000000
+
+/** Write this offset, plus the byte offset of the start of a sector, to
+ *  erase a sector.  Any write data is ignored, but there must be at least
+ *  one byte of write data.  Only applies when the driver is in MTD mode.
+ */
+#define SROM_ERASE_OFF        0xF2000000
+
+#endif /* _SYS_HV_INCLUDE_DRV_SROM_INTF_H */
-- 
1.6.5.2

Re: [PATCH] arch/tile: add arch/tile/drivers/ directory with SROM driver

[Arnd Bergmann]

On Wednesday 04 May 2011, Chris Metcalf wrote:
> This commit does two things: it adds the infrastructure for a new
> arch/tile/drivers/ directory, and it populates it with the first
> instance of a driver for that directory, a SPI Flash ROM (srom) driver.
> 
> The directory is motivated as follows.  While some classes of
> driver implementations should be grouped together so they are easily
> modified as a class (network, ATA, RTC, PCI, I2C, etc etc) there are
> "miscellaneous" drivers that don't benefit from any sharing with other
> driver implementations.  If those drivers are for hardware that can
> plausibly be used by multiple architectures, it makes sense to put
> them somewhere like drivers/char.  But if they are only usable on
> a single architecture (in this case drivers written for the Tilera
> para-virtualization model using our hypervisor) it makes sense to group
> such drivers with their architecture, to avoid cluttering the "drivers"
> hierarchy for other architectures that can't use that driver.

I generally advise against putting any device drivers into arch/*/,
on the ground that it is much easier to find similar drivers when
they are in a common place. We probably have a few similar drivers
in the tree already, e.g drivers/misc/eeprom/* and drivers/char/ps3flash.c
and drivers/platform/x86/intel_scu_ipcutil.c are examples of preexisting
drivers.

I'd probably put this one in driver/misc/eeprom and make the interface
look like the other ones (sysfs bin attribute instead of character
device), which is a trivial change.

Alternatively, you could create drivers/platform/tile to hold tile
specific device drivers, instead of keeping them under arch/tile.

> The actual SROM driver is fairly uncontroversial, and is just a simple
> driver that allows user space to read and write the SROM at a raw level.
> (A separate MTD driver exists for "tile", but this is not that driver.)
> The driver is particularly useful since the Tile chip can boot directly
> from the SROM, so providing this driver interface allows for updating
> the boot image prior to a reboot.

I think the sysfs bin attribute works well here because you don't need
any ioctl, and the contents are basically a representation of a flat
file. The implementation would be almost the same.

> +/**
> + * srom_llseek() - Change the current device offset.
> + * @filp: File for this specific open of the device.
> + * @off: New offset value.
> + * @whence: Base for new offset value.
> + *
> + * Returns new offset, or an error code.
> + */
> +static loff_t srom_llseek(struct file *filp, loff_t off, int whence)
> +{
> +	struct srom_dev *dev = filp->private_data;
> +	long newpos;
> +
> +	switch (whence) {
> +	case 0: /* SEEK_SET */
> +		newpos = off;
> +		break;
> +
> +	case 1: /* SEEK_CUR */
> +		newpos = filp->f_pos + off;
> +		break;
> +
> +	case 2: /* SEEK_END */
> +		newpos = dev->size + off;
> +		break;
> +
> +	default: /* can't happen */
> +		return -EINVAL;
> +	}
> +
> +	if (newpos < 0 || newpos > dev->size)
> +		return -EINVAL;
> +
> +	filp->f_pos = newpos;
> +	return newpos;
> +}

This looks unnecessary, just use generic_file_llseek.
	
	Arnd

Re: [PATCH] arch/tile: add arch/tile/drivers/ directory with SROM driver

[Chris Metcalf]

On 5/5/2011 2:41 AM, Arnd Bergmann wrote:
> On Wednesday 04 May 2011, Chris Metcalf wrote:
>> This commit does two things: it adds the infrastructure for a new
>> arch/tile/drivers/ directory, and it populates it with the first
>> instance of a driver for that directory, a SPI Flash ROM (srom) driver.
>>
>> The directory is motivated as follows.  While some classes of
>> driver implementations should be grouped together so they are easily
>> modified as a class (network, ATA, RTC, PCI, I2C, etc etc) there are
>> "miscellaneous" drivers that don't benefit from any sharing with other
>> driver implementations.  If those drivers are for hardware that can
>> plausibly be used by multiple architectures, it makes sense to put
>> them somewhere like drivers/char.  But if they are only usable on
>> a single architecture (in this case drivers written for the Tilera
>> para-virtualization model using our hypervisor) it makes sense to group
>> such drivers with their architecture, to avoid cluttering the "drivers"
>> hierarchy for other architectures that can't use that driver.
> I generally advise against putting any device drivers into arch/*/,
> on the ground that it is much easier to find similar drivers when
> they are in a common place. We probably have a few similar drivers
> in the tree already, e.g drivers/misc/eeprom/* and drivers/char/ps3flash.c
> and drivers/platform/x86/intel_scu_ipcutil.c are examples of preexisting
> drivers.
>
> I'd probably put this one in driver/misc/eeprom and make the interface
> look like the other ones (sysfs bin attribute instead of character
> device), which is a trivial change.
>
> Alternatively, you could create drivers/platform/tile to hold tile
> specific device drivers, instead of keeping them under arch/tile.

For the implementation, as you say, it's worth conforming to the other
eeprom drivers regardless of where we put the actual implementation, so
I'll look into the sysfs infrastructure (I haven't used it before, but I
need to learn about it anyway).

As far as the location in the tree, I think it's 50/50 on something like
our eeprom driver.  On the one hand the actual implementation is very
Tilera-specific and very similar to all the other simple Tilera-specific
para-virtualized drivers, which are mostly just read/write wrappers around
hypervisor syscalls (we currently have about a dozen of these for various
minor bits of I/O).  On the other hand there is a little bit of eeprom
commonality too, but on balance it feels like it makes more sense to keep
it with the other "misc" Tilera drivers.  I'd say neither answer is
obviously wrong :-)

As to where the "misc" Tilera drivers should go, I see that so far there is
only drivers/platform/x86 (as created by Len Brown in 2008, along with a
note that "other architectures will create drivers/platform/<arch>").  Is
it fair to say that "drivers/s390", "drivers/parisc", and "drivers/sh" (for
example) are all in their current locations just for historical reasons,
and should in principle also be under "drivers/platform"?  If so, yes,
drivers/platform/tile seems like a reasonable location.

>> +/**
>> + * srom_llseek() - Change the current device offset.
>> [...]
> This looks unnecessary, just use generic_file_llseek.

It looks like I also can just discard the llseek code altogether if we
switch over to the sysfs model - even better :-)

-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com

Re: [PATCH] arch/tile: add arch/tile/drivers/ directory with SROM driver

[Chris Metcalf]

On 5/5/2011 2:41 AM, Arnd Bergmann wrote:
> On Wednesday 04 May 2011, Chris Metcalf wrote:
>> This commit does two things: it adds the infrastructure for a new
>> arch/tile/drivers/ directory, and it populates it with the first
>> instance of a driver for that directory, a SPI Flash ROM (srom) driver.
>>
>> [...]
> I generally advise against putting any device drivers into arch/*/,
> on the ground that it is much easier to find similar drivers when
> they are in a common place. We probably have a few similar drivers
> in the tree already, e.g drivers/misc/eeprom/* and drivers/char/ps3flash.c
> and drivers/platform/x86/intel_scu_ipcutil.c are examples of preexisting
> drivers.
>
> I'd probably put this one in driver/misc/eeprom and make the interface
> look like the other ones (sysfs bin attribute instead of character
> device), which is a trivial change.
>
> [...]
>> The actual SROM driver is fairly uncontroversial, and is just a simple
>> driver that allows user space to read and write the SROM at a raw level.
>> (A separate MTD driver exists for "tile", but this is not that driver.)
>> The driver is particularly useful since the Tile chip can boot directly
>> from the SROM, so providing this driver interface allows for updating
>> the boot image prior to a reboot.
> I think the sysfs bin attribute works well here because you don't need
> any ioctl, and the contents are basically a representation of a flat
> file. The implementation would be almost the same.

This works well, except for the fact that we take advantage of the fact
that the hypervisor driver internally buffers up writes to the current
EEPROM sector, and flushes it to hardware only when explicitly told to do
so, or when we start writing to another sector.  This avoids excessive wear
on the EEPROM and also handles detection of whether we need to do a sector
erase before the re-write.  However, it also means that we need to be able
to issue the final explicit flush, or else the last write just sits in the
hypervisor buffer indefinitely.  To make that happen I think I need to
extend the bin_attribute structure, and the bin.c release() function, slightly:

--- fs/sysfs/bin.c~   2011-05-20 14:02:43.000000000 -0400
+++ fs/sysfs/bin.c  2011-05-20 13:31:23.240866000 -0400
@@ -434,18 +434,26 @@
 }

 static int release(struct inode * inode, struct file * file)
 {
        struct bin_buffer *bb = file->private_data;
+       struct sysfs_dirent *attr_sd = file->f_path.dentry->d_fsdata;
+       struct bin_attribute *attr = attr_sd->s_bin_attr.bin_attr;
+       int rc = 0;
+
+       if (attr->release && sysfs_get_active(attr_sd)) {
+               rc = attr->release(file, attr_sd->s_parent->s_dir.kobj, attr);
+               sysfs_put_active(attr_sd);
+       }

        mutex_lock(&sysfs_bin_lock);
        hlist_del(&bb->list);
        mutex_unlock(&sysfs_bin_lock);

        kfree(bb->buffer);
        kfree(bb);
-       return 0;
+       return rc;
 }

 const struct file_operations bin_fops = {
        .read           = read,
        .write          = write,
--- include/linux/sysfs.h~   2011-05-20 14:02:43.000000000 -0400
+++ include/linux/sysfs.h   2011-05-20 13:23:57.915080000 -0400
@@ -93,10 +93,11 @@
                        char *, loff_t, size_t);
        ssize_t (*write)(struct file *,struct kobject *, struct bin_attribute *,
                         char *, loff_t, size_t);
        int (*mmap)(struct file *, struct kobject *, struct bin_attribute *attr,
                    struct vm_area_struct *vma);
+       int (*release)(struct file *, struct kobject *, struct bin_attribute *);
 };

 /**
  *     sysfs_bin_attr_init - initialize a dynamically allocated bin_attribute
  *     @attr: struct bin_attribute to initialize



This change shouldn't require any changes to any other bin_attribute users
elsewhere in the kernel.

If something like this seems plausible, I already have the patch ready, and
I can send it to LKML.  I've cc'ed Greg K-H and a few other recent
submitters to bin.c in case they'd like to weigh in at this point too; thanks.

-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com

Re: [PATCH] arch/tile: add arch/tile/drivers/ directory with SROM driver

[Arnd Bergmann]

On Friday 20 May 2011 20:05:05 Chris Metcalf wrote:
> This works well, except for the fact that we take advantage of the fact
> that the hypervisor driver internally buffers up writes to the current
> EEPROM sector, and flushes it to hardware only when explicitly told to do
> so, or when we start writing to another sector.  This avoids excessive wear
> on the EEPROM and also handles detection of whether we need to do a sector
> erase before the re-write.  However, it also means that we need to be able
> to issue the final explicit flush, or else the last write just sits in the
> hypervisor buffer indefinitely.  To make that happen I think I need to
> extend the bin_attribute structure, and the bin.c release() function, slightly:

Yes, that would work. Another option would be to add a flush() callback
to the bin_attribute. Since filp_close calls both fops->flush and
fops->release, that would also make it possible to flush the buffer
without closing it, if necessary.

	Arnd

Re: [PATCH] arch/tile: add arch/tile/drivers/ directory with SROM driver

[Eric Biederman]

On Fri, May 20, 2011 at 11:46 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday 20 May 2011 20:05:05 Chris Metcalf wrote:
>> This works well, except for the fact that we take advantage of the fact
>> that the hypervisor driver internally buffers up writes to the current
>> EEPROM sector, and flushes it to hardware only when explicitly told to do
>> so, or when we start writing to another sector.  This avoids excessive wear
>> on the EEPROM and also handles detection of whether we need to do a sector
>> erase before the re-write.  However, it also means that we need to be able
>> to issue the final explicit flush, or else the last write just sits in the
>> hypervisor buffer indefinitely.  To make that happen I think I need to
>> extend the bin_attribute structure, and the bin.c release() function, slightly:
>
> Yes, that would work. Another option would be to add a flush() callback
> to the bin_attribute. Since filp_close calls both fops->flush and
> fops->release, that would also make it possible to flush the buffer
> without closing it, if necessary.

Ouch!

Please do not make sysfs the direct access method to your device.

I may be wrong but I don't think any other driver relies exclusively on sysfs.
Certainly with the introduction of a flush you are introducing an completely
different usage paradigm from current users and will need an entirely different
set of tools.

You are vastly exceeding the one value per file rule of sysfs.

Please look at the i2c core and if at all possible build a proper interface to
your eeprom, that existing programs have a chance of utilizing rather than
extending sysfs in ways that means cli tools can not work with it.

Eric

Re: [PATCH] arch/tile: add arch/tile/drivers/ directory with SROM driver

[Chris Metcalf]

On 5/20/2011 6:40 PM, Eric Biederman wrote:
> Please do not make sysfs the direct access method to your device.
> I may be wrong but I don't think any other driver relies exclusively on sysfs.

I'm basing my implementation on drivers/misc/eeprom/.  All the drivers
there use the same sysfs model.

> Certainly with the introduction of a flush you are introducing an completely
> different usage paradigm from current users and will need an entirely different
> set of tools.

I don't think using my proposed implementation will be detectably different
for most user tools.  The addition of the flush() method just allows my
implementation to defer the final sector's write until the device is closed
(sectors are in fact still written to hardware as one does seek() or
write() from one sector to another; only the "current" sector is cached by
the hypervisor).  I suppose some third-party tool that kept the eeprom file
descriptor open indefinitely and did multiple writes to the same sector
might not work as expected with this implementation.  But it seems hard to
imagine a use case for such a tool.

The direct motivation for this case is to "impedance match" to the
hypervisor driver for this device, which handles sector management
internally, so the Linux device doesn't have to.  Having a 'flush' method
avoids excessive re-writes of the same sector for certain access patterns. 
The only alternatives that I see are to rewrite the tile userspace tools,
but they are the way they are because the current model gives good
consistency guarantees for writing the boot rom in the presence of
arbitrary failure modes; or, to add something like a delayed timer event
that allows the Linux driver to notify the hypervisor driver that writes
are likely complete and it can write out the last sector.  Neither of these
are particularly attractive.

> You are vastly exceeding the one value per file rule of sysfs.

True, but bin_attribute has always been an exception to that rule anyway.

> Please look at the i2c core and if at all possible build a proper interface to
> your eeprom, that existing programs have a chance of utilizing rather than
> extending sysfs in ways that means cli tools can not work with it.

This driver is a paravirtualized hypervisor driver, so not really an I2C
driver at all (in fact it handles both SPI and I2C eeproms almost
identically within the Linux driver).  And I think the driver's "eeprom"
file should be compatible with userspace cli tools, assuming they close
their file descriptor when they're done writing.

I apologize for not including more of the back story in this email when
adding the cc's, by the way.  Originally I proposed a straightforward
character device for this role.  Arnd Bergmann encouraged me to look at
kernel precedents like drivers/char/eeprom/, which is why I converted this
driver to sysfs.  The first post in this thread is here:
https://lkml.org/lkml/2011/5/4/415 .  Since then I've come around to
believing that it's more valuable to group this driver with the other
eeprom drivers, rather than with the other paravirtualized tile drivers.

-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com

Re: [PATCH] arch/tile: add arch/tile/drivers/ directory with SROM driver

[Greg KH]

On Fri, May 20, 2011 at 07:39:10PM -0400, Chris Metcalf wrote:
> On 5/20/2011 6:40 PM, Eric Biederman wrote:
> > Please do not make sysfs the direct access method to your device.
> > I may be wrong but I don't think any other driver relies exclusively on sysfs.
> 
> I'm basing my implementation on drivers/misc/eeprom/.  All the drivers
> there use the same sysfs model.
> 
> > Certainly with the introduction of a flush you are introducing an completely
> > different usage paradigm from current users and will need an entirely different
> > set of tools.
> 
> I don't think using my proposed implementation will be detectably different
> for most user tools.  The addition of the flush() method just allows my
> implementation to defer the final sector's write until the device is closed
> (sectors are in fact still written to hardware as one does seek() or
> write() from one sector to another; only the "current" sector is cached by
> the hypervisor).  I suppose some third-party tool that kept the eeprom file
> descriptor open indefinitely and did multiple writes to the same sector
> might not work as expected with this implementation.  But it seems hard to
> imagine a use case for such a tool.
> 
> The direct motivation for this case is to "impedance match" to the
> hypervisor driver for this device, which handles sector management
> internally, so the Linux device doesn't have to.  Having a 'flush' method
> avoids excessive re-writes of the same sector for certain access patterns. 
> The only alternatives that I see are to rewrite the tile userspace tools,
> but they are the way they are because the current model gives good
> consistency guarantees for writing the boot rom in the presence of
> arbitrary failure modes; or, to add something like a delayed timer event
> that allows the Linux driver to notify the hypervisor driver that writes
> are likely complete and it can write out the last sector.  Neither of these
> are particularly attractive.
> 
> > You are vastly exceeding the one value per file rule of sysfs.
> 
> True, but bin_attribute has always been an exception to that rule anyway.

No it hasn't.  A bin attribute is for something that is "one" value, it
is a binary file that the kernel doesn't know anything about, nor does
it intrepret it.

It sounds like you want this binary attribute file to be a bit different
than the "normal" one, and because of that you are wanting to modify the
sysfs file, which proves that you are doing things differently here.

So, I agree with Eric, please do something else as you are not really
wanting to use a binary attribute, you want something else, like a
character driver.

Why can't you do that?

> This driver is a paravirtualized hypervisor driver, so not really an I2C
> driver at all (in fact it handles both SPI and I2C eeproms almost
> identically within the Linux driver).  And I think the driver's "eeprom"
> file should be compatible with userspace cli tools, assuming they close
> their file descriptor when they're done writing.

Big assumption.

> I apologize for not including more of the back story in this email when
> adding the cc's, by the way.

Yeah, that made things difficult :)

> Originally I proposed a straightforward character device for this
> role.

Ah, ok, I think you should do that.

> Arnd Bergmann encouraged me to look at kernel precedents like
> drivers/char/eeprom/, which is why I converted this driver to sysfs.
> The first post in this thread is here:
> https://lkml.org/lkml/2011/5/4/415 .  Since then I've come around to
> believing that it's more valuable to group this driver with the other
> eeprom drivers, rather than with the other paravirtualized tile
> drivers.

See above why I don't think that is so.

thanks,

greg k-h

Re: [PATCH] arch/tile: add arch/tile/drivers/ directory with SROM driver

[Eric Biederman]

On Fri, May 20, 2011 at 4:39 PM, Chris Metcalf <cmetcalf@tilera.com> wrote:
> On 5/20/2011 6:40 PM, Eric Biederman wrote:
>> Please do not make sysfs the direct access method to your device.
>> I may be wrong but I don't think any other driver relies exclusively on sysfs.
>
> I'm basing my implementation on drivers/misc/eeprom/.  All the drivers
> there use the same sysfs model.
>
>> Certainly with the introduction of a flush you are introducing an completely
>> different usage paradigm from current users and will need an entirely different
>> set of tools.
>
> I don't think using my proposed implementation will be detectably different
> for most user tools.  The addition of the flush() method just allows my
> implementation to defer the final sector's write until the device is closed
> (sectors are in fact still written to hardware as one does seek() or
> write() from one sector to another; only the "current" sector is cached by
> the hypervisor).  I suppose some third-party tool that kept the eeprom file
> descriptor open indefinitely and did multiple writes to the same sector
> might not work as expected with this implementation.  But it seems hard to
> imagine a use case for such a tool.
>
> The direct motivation for this case is to "impedance match" to the
> hypervisor driver for this device, which handles sector management
> internally, so the Linux device doesn't have to.  Having a 'flush' method
> avoids excessive re-writes of the same sector for certain access patterns.
> The only alternatives that I see are to rewrite the tile userspace tools,
> but they are the way they are because the current model gives good
> consistency guarantees for writing the boot rom in the presence of
> arbitrary failure modes; or, to add something like a delayed timer event
> that allows the Linux driver to notify the hypervisor driver that writes
> are likely complete and it can write out the last sector.  Neither of these
> are particularly attractive.

What is wrong with an mtd driver?

Looking a bit back into the conversation it appears clear that you are
talking about something that resembles NOR flash with multiple sectors,
etc.

eeproms have random byte access and are typically 256 bytes.  You devices
doesn't sound anything like an eeprom.

> I apologize for not including more of the back story in this email when
> adding the cc's, by the way.  Originally I proposed a straightforward
> character device for this role.  Arnd Bergmann encouraged me to look at
> kernel precedents like drivers/char/eeprom/, which is why I converted this
> driver to sysfs.  The first post in this thread is here:
> https://lkml.org/lkml/2011/5/4/415 .  Since then I've come around to
> believing that it's more valuable to group this driver with the other
> eeprom drivers, rather than with the other paravirtualized tile drivers.

Again this sounds most like an mtd driver to me, and arguably should
be grouped there.

If you have to worry about keeping sectors open or closed your hardware
is very much not an eeprom.

Eric

Re: [PATCH] arch/tile: add arch/tile/drivers/ directory with SROM driver

[Arnd Bergmann]

On Saturday 21 May 2011 09:46:55 Eric Biederman wrote:
> On Fri, May 20, 2011 at 4:39 PM, Chris Metcalf <cmetcalf@tilera.com> wrote:
> > On 5/20/2011 6:40 PM, Eric Biederman wrote:
> > The direct motivation for this case is to "impedance match" to the
> > hypervisor driver for this device, which handles sector management
> > internally, so the Linux device doesn't have to.  Having a 'flush' method
> > avoids excessive re-writes of the same sector for certain access patterns.
> > The only alternatives that I see are to rewrite the tile userspace tools,
> > but they are the way they are because the current model gives good
> > consistency guarantees for writing the boot rom in the presence of
> > arbitrary failure modes; or, to add something like a delayed timer event
> > that allows the Linux driver to notify the hypervisor driver that writes
> > are likely complete and it can write out the last sector.  Neither of these
> > are particularly attractive.
> 
> What is wrong with an mtd driver?
> 
> Looking a bit back into the conversation it appears clear that you are
> talking about something that resembles NOR flash with multiple sectors,
> etc.
> 
> eeproms have random byte access and are typically 256 bytes.  You devices
> doesn't sound anything like an eeprom.

MTD implies that you have low-level access to the NOR flash registers,
which this one doesn't. It's certainly not the right Linux interface
for a high-level flash device, and there is no precedent for this at
all in Linux.

We do have precedent for multiple interfaces that have the same
purpose as this one:

drivers/misc/eeprom/*
drivers/char/ps3flash.c
drivers/char/nwflash.c
drivers/char/bfin-otp.c
arch/powerpc/kernel/rtas_flash.c
drivers/sbus/char/jsflash.c

And then some more that I missed, plus the ones that use MTD.

	Arnd

Re: [PATCH] arch/tile: add arch/tile/drivers/ directory with SROM driver

[Arnd Bergmann]

On Saturday 21 May 2011 05:21:02 Greg KH wrote:
> On Fri, May 20, 2011 at 07:39:10PM -0400, Chris Metcalf wrote:
> > On 5/20/2011 6:40 PM, Eric Biederman wrote:
> > 
> > > You are vastly exceeding the one value per file rule of sysfs.
> > 
> > True, but bin_attribute has always been an exception to that rule anyway.
> 
> No it hasn't.  A bin attribute is for something that is "one" value, it
> is a binary file that the kernel doesn't know anything about, nor does
> it intrepret it.
> 
> It sounds like you want this binary attribute file to be a bit different
> than the "normal" one, and because of that you are wanting to modify the
> sysfs file, which proves that you are doing things differently here.

I initially argued that the file is the same as all the other ones
in drivers/misc/eeprom/*.c. None of them care what the data is and
they pass it through to the user. It would also be possible to
always flush after each write, which makes the interface act more
like you would expect it to, at the cost of lower performance in
case the user writes it in small blocks.

> > Originally I proposed a straightforward character device for this
> > role.
> 
> Ah, ok, I think you should do that.
> 
> > Arnd Bergmann encouraged me to look at kernel precedents like
> > drivers/char/eeprom/, which is why I converted this driver to sysfs.
> > The first post in this thread is here:
> > https://lkml.org/lkml/2011/5/4/415 .  Since then I've come around to
> > believing that it's more valuable to group this driver with the other
> > eeprom drivers, rather than with the other paravirtualized tile
> > drivers.
> 
> See above why I don't think that is so.

What I only now noticed is that the other eeprom drivers only support
reading the eeprom, not writing it, so there is a significant difference.

Using the bin_attribute doesn't sound completely wrong to me, especially
if you put it in your /sys/hypervisor/* direcory together with the
regular attributes we talked about. The character device would also
be an option (better than /proc/ppc/update_flash that is used on pSeries),
but if we do that, I would group it together with the other similar
files (ps3flash, nwflash, ...) in a new subdirectory.

	Arnd

Re: [PATCH] arch/tile: add arch/tile/drivers/ directory with SROM driver

[Chris Metcalf]

(Resending since Thunderbird "helpfully" made the previous attempt include
an HTML portion, for no obvious reason.)

On 5/20/2011 11:21 PM, Greg KH wrote:
> On Fri, May 20, 2011 at 07:39:10PM -0400, Chris Metcalf wrote:
>> This driver is a paravirtualized hypervisor driver, so not really an I2C
>> driver at all (in fact it handles both SPI and I2C eeproms almost
>> identically within the Linux driver).  And I think the driver's "eeprom"
>> file should be compatible with userspace cli tools, assuming they close
>> their file descriptor when they're done writing.
> Big assumption.

Interesting!  I did in fact assume that most tools that read or write
eeproms would tend to be "batch" tools, i.e. they would read or write
whatever it was they wanted, then exit; the tool we have that modifies this
EEPROM is written like that, for example.  It sounds like you're saying
there are (or may be) tools that open the file descriptor and do writes to
it, then don't exit or close the file descriptor, but expect the last write
to have hit the device immediately.  It's fair to say that since my
suggested API does defer the last sector's worth of writes until the app
exits, it potentially violates that assumption, so it's a bad idea.

On 5/21/2011 3:46 AM, Eric Biederman wrote:
> What is wrong with an mtd driver?

More backstory: we actually have an MTD driver for this device (in our
local tree as drivers/mtd/devices/tile_srom.c) but we haven't yet tried to
push it back to the community.  But it doesn't work for the most important
purpose of this device, namely to serve as one of the possible boot streams
at power-on.  For this, we have to control exactly what data is on what
sector, which means a character (or sysfs) device.

> Looking a bit back into the conversation it appears clear that you are
> talking about something that resembles NOR flash with multiple sectors,
> etc.
>
> eeproms have random byte access and are typically 256 bytes.  You devices
> doesn't sound anything like an eeprom.

Yes and no.  As Arnd points out in his followup, the hypervisor driver for
this SPI ROM makes it look a lot like a typical eeprom.  But, perhaps, not
quite close enough to count as a "real" eeprom.

On 5/21/2011 5:33 AM, Arnd Bergmann wrote:
> What I only now noticed is that the other eeprom drivers only support
> reading the eeprom, not writing it, so there is a significant difference.

However, this point isn't quite correct.  Both the at24 (I2C) and at25
(SPI) sysfs drivers support the "write" callback for at least some of their
supported hardware.  It's true that all the other drivers are read-only.

> Using the bin_attribute doesn't sound completely wrong to me, especially
> if you put it in your /sys/hypervisor/* direcory together with the
> regular attributes we talked about. The character device would also
> be an option (better than /proc/ppc/update_flash that is used on pSeries),
> but if we do that, I would group it together with the other similar
> files (ps3flash, nwflash, ...) in a new subdirectory.

Sounds like the consensus is that a character driver is in fact the best
option here.

> We do have precedent for multiple interfaces that have the same
> purpose as this one:
>
> drivers/misc/eeprom/*
> drivers/char/ps3flash.c
> drivers/char/nwflash.c
> drivers/char/bfin-otp.c
> arch/powerpc/kernel/rtas_flash.c
> drivers/sbus/char/jsflash.c

I'm certainly happy to push the original SPI ROM character device as
drivers/platform/tile/srom.c.  I'm not sure there's enough value in trying
to group that device together with other devices that are sort of similar
but with pretty variant ancestry and not a lot of need for commonality --
other than all have a file_operations structure :-)  In this case I think
grouping it with other paravirtualized tile drivers may be the closer
connection.

Having gone through the process of creating a sysfs driver, I will also go
ahead and remove the SPI support from it (since that's the part that
requires "flush") and post the remaining pure-EEPROM I2C paravirtualized
driver to LKML.  Since it has no need for flushes, it ends up exactly
parallel to at24.c.

So here's my thought.  How does this sound?

- Add drivers/platform/tile
- Put the original character device there as srom.c
- Drop the bin_attribute "flush" changes
- Add drivers/misc/eeprom/tile.c for our "pure" I2C EEPROM driver

I appreciate everyone's feedback and help on this!

-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com

Re: [PATCH] arch/tile: add arch/tile/drivers/ directory with SROM driver

[Arnd Bergmann]

On Saturday 21 May 2011 15:52:39 Chris Metcalf wrote:

> On 5/21/2011 5:33 AM, Arnd Bergmann wrote:
> > What I only now noticed is that the other eeprom drivers only support
> > reading the eeprom, not writing it, so there is a significant difference.
> 
> However, this point isn't quite correct.  Both the at24 (I2C) and at25
> (SPI) sysfs drivers support the "write" callback for at least some of their
> supported hardware.  It's true that all the other drivers are read-only.

Ok, I stand corrected again.

> > Using the bin_attribute doesn't sound completely wrong to me, especially
> > if you put it in your /sys/hypervisor/* direcory together with the
> > regular attributes we talked about. The character device would also
> > be an option (better than /proc/ppc/update_flash that is used on pSeries),
> > but if we do that, I would group it together with the other similar
> > files (ps3flash, nwflash, ...) in a new subdirectory.
> 
> Sounds like the consensus is that a character driver is in fact the best
> option here.

At least the one that meets the least resistance ;-)

> > We do have precedent for multiple interfaces that have the same
> > purpose as this one:
> >
> > drivers/misc/eeprom/*
> > drivers/char/ps3flash.c
> > drivers/char/nwflash.c
> > drivers/char/bfin-otp.c
> > arch/powerpc/kernel/rtas_flash.c
> > drivers/sbus/char/jsflash.c
> 
> I'm certainly happy to push the original SPI ROM character device as
> drivers/platform/tile/srom.c.  I'm not sure there's enough value in trying
> to group that device together with other devices that are sort of similar
> but with pretty variant ancestry and not a lot of need for commonality --
> other than all have a file_operations structure :-)  In this case I think
> grouping it with other paravirtualized tile drivers may be the closer
> connection.

The general tendency is to always group drivers by their purpose these
days, instead of by the bus or other interface that they are attached
to. I would definitely prefer grouping it with drivers of the same
purpose.

The main reason is to make sure that a driver writer can easily find
existing drivers with the same purpose take them as example code, so
that new code uses the same API as existing code.

> Having gone through the process of creating a sysfs driver, I will also go
> ahead and remove the SPI support from it (since that's the part that
> requires "flush") and post the remaining pure-EEPROM I2C paravirtualized
> driver to LKML.  Since it has no need for flushes, it ends up exactly
> parallel to at24.c.
> 
> So here's my thought.  How does this sound?
> 
> - Add drivers/platform/tile
> - Put the original character device there as srom.c
> - Drop the bin_attribute "flush" changes
> - Add drivers/misc/eeprom/tile.c for our "pure" I2C EEPROM driver
> 
> I appreciate everyone's feedback and help on this!

Can you explain why there are both i2c and spi drivers? If they
have the same purpose (e.g. providing a place to store the kernel
binary) and same data layout, I would recommend using the
same user interface for both.

If you want to keep the srom.c driver with a chardev interface for
the SPI chip only, I would recommend sticking it into drivers/char/
for now, or possibly a new drivers/char/flash/.

I have agreed to take over future maintainership of drivers/char and
driver/misc during UDS in Budapest, basically to keep random
stuff from getting added there, so I will try to find a better place
for flash drivers like this.

	Arnd

Re: [PATCH] arch/tile: add arch/tile/drivers/ directory with SROM driver

[Chris Metcalf]

On 5/21/2011 11:02 AM, Arnd Bergmann wrote:
> On Saturday 21 May 2011 15:52:39 Chris Metcalf wrote:
>> Sounds like the consensus is that a character driver is in fact the best
>> option here.
> At least the one that meets the least resistance ;-)

Well, perhaps that's what I meant :-)  I do take Greg K-H's point that we
don't really want to mess with the cleanliness of the bin_attribute API,
though.

> The general tendency is to always group drivers by their purpose these
> days, instead of by the bus or other interface that they are attached
> to. I would definitely prefer grouping it with drivers of the same
> purpose.
>
> The main reason is to make sure that a driver writer can easily find
> existing drivers with the same purpose take them as example code, so
> that new code uses the same API as existing code.
>
> If you want to keep the srom.c driver with a chardev interface for
> the SPI chip only, I would recommend sticking it into drivers/char/
> for now, or possibly a new drivers/char/flash/.

I will plan to push it to drivers/char/tile-srom.c for now, then.

> I have agreed to take over future maintainership of drivers/char and
> driver/misc during UDS in Budapest, basically to keep random
> stuff from getting added there, so I will try to find a better place
> for flash drivers like this.

I'll add my thanks for helping keep the kernel a better place.  It's great
that there are folks like yourself who are willing to devote time to code
and structural cleanliness rather than just shoving in whatever they can
get away with :-)

> Can you explain why there are both i2c and spi drivers? If they
> have the same purpose (e.g. providing a place to store the kernel
> binary) and same data layout, I would recommend using the
> same user interface for both.

The I2C eeprom driver can't be used to store the boot stream, just the SPI
driver; the I2C eeprom driver is mostly for customer use.  In fact the
drivers are similar enough that I rewrote the SPI driver for sysfs first,
since it was harder, but then found that grafting in support for the I2C
driver was pretty easy, since all I had to do was add some
conditionalization (different "chunk" size for writes, no need for write
retries during sector erase, no support for page_size and sector_size
attributes).  The absence of "size" attributes for the I2C driver is the
only visible user interface difference, but it's fundamental to the devices.

It's tempting to combine the drivers into one, but it feels like we might
as well publish the I2C driver with the other eeprom sysfs drivers, and the
SPI driver as a character device, as discussed.

-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com

Re: [PATCH] arch/tile: add arch/tile/drivers/ directory with SROM driver

[Eric Biederman]

On Sat, May 21, 2011 at 6:52 AM, Chris Metcalf <cmetcalf@tilera.com> wrote:
> Interesting!  I did in fact assume that most tools that read or write
> eeproms would tend to be "batch" tools, i.e. they would read or write
> whatever it was they wanted, then exit; the tool we have that modifies this
> EEPROM is written like that, for example.  It sounds like you're saying
> there are (or may be) tools that open the file descriptor and do writes to
> it, then don't exit or close the file descriptor, but expect the last write
> to have hit the device immediately.  It's fair to say that since my
> suggested API does defer the last sector's worth of writes until the app
> exits, it potentially violates that assumption, so it's a bad idea.

Plus the other gigantic difference that I have seen very few eeproms
that can hold a sectors worth of data.

My reading of this conversation is that you are confusing a configuration
eeprom with a nor flash boot device.  That is a bit like confusing a yellow
sticky not with the Encyclopedia Britannica.   They aren't at all the
same thing.

> On 5/21/2011 3:46 AM, Eric Biederman wrote:
>> What is wrong with an mtd driver?
>
> More backstory: we actually have an MTD driver for this device (in our
> local tree as drivers/mtd/devices/tile_srom.c) but we haven't yet tried to
> push it back to the community.  But it doesn't work for the most important
> purpose of this device, namely to serve as one of the possible boot streams
> at power-on.  For this, we have to control exactly what data is on what
> sector, which means a character (or sysfs) device.

How can an MTD driver not work to control exactly what data is on what
sector????  That is what MTD drivers do.

It sounds like you have a bug in either your userspace tools or your
implementation of your mtd driver.  That is definitely not a reason to
dump your device into a char device (mtdchar already exists).

> Sounds like the consensus is that a character driver is in fact the best
> option here.

I think the consensus is that your design is buggy.

Eric

Re: [PATCH] arch/tile: add arch/tile/drivers/ directory with SROM driver

[Mike Frysinger]

On Sat, May 21, 2011 at 01:32, Arnd Bergmann wrote:
> On Saturday 21 May 2011 09:46:55 Eric Biederman wrote:
>> What is wrong with an mtd driver?
>>
>> Looking a bit back into the conversation it appears clear that you are
>> talking about something that resembles NOR flash with multiple sectors,
>> etc.
>>
>> eeproms have random byte access and are typically 256 bytes.  You devices
>> doesn't sound anything like an eeprom.
>
> MTD implies that you have low-level access to the NOR flash registers,
> which this one doesn't. It's certainly not the right Linux interface
> for a high-level flash device, and there is no precedent for this at
> all in Linux.
>
> We do have precedent for multiple interfaces that have the same
> purpose as this one:
>
> drivers/misc/eeprom/*
> drivers/char/ps3flash.c
> drivers/char/nwflash.c
> drivers/char/bfin-otp.c

there is a new otp framework in the works that this driver would move
to.  but that wouldnt matter to the driver being proposed for the tile
arch i dont think.
-mike

Re: [PATCH] arch/tile: add arch/tile/drivers/ directory with SROM driver

[Chris Metcalf]

On 5/21/2011 11:50 AM, Eric Biederman wrote:
> On Sat, May 21, 2011 at 6:52 AM, Chris Metcalf <cmetcalf@tilera.com> wrote:
>> Interesting!  I did in fact assume that most tools that read or write
>> eeproms would tend to be "batch" tools, i.e. they would read or write
>> whatever it was they wanted, then exit; the tool we have that modifies this
>> EEPROM is written like that, for example.  It sounds like you're saying
>> there are (or may be) tools that open the file descriptor and do writes to
>> it, then don't exit or close the file descriptor, but expect the last write
>> to have hit the device immediately.  It's fair to say that since my
>> suggested API does defer the last sector's worth of writes until the app
>> exits, it potentially violates that assumption, so it's a bad idea.
> Plus the other gigantic difference that I have seen very few eeproms
> that can hold a sectors worth of data.
>
> My reading of this conversation is that you are confusing a configuration
> eeprom with a nor flash boot device.  That is a bit like confusing a yellow
> sticky not with the Encyclopedia Britannica.   They aren't at all the
> same thing.

Well, I don't think I'm confusing the two types of hardware, per se.  The
issue is that the hypervisor provides an API to Linux (or other operating
systems) that attempts to make them look reasonably similar.  In
particular, for the SPI ROM, the hypervisor implicitly manages background
reading of the sector data, doing a "diff" of new data to determine whether
a sector erase is required, and deferring the actual sector write until the
client attempts to write to a different sector.  (This last feature is why
an explicit "flush" is required.)

This approach (unlike mtdchar) provides a "transparent" mode for accessing
the SPI ROM.  Users of our character driver don't need to know they are
writing a flash device, don't need to worry about erasing/rewriting
sectors, etc.  We could potentially rework our boot ROM management tool to
support the mtdchar API, but it would be a modest pain, particularly since
we still have to support the normal file mode without "erase" ioctls, etc.,
for use in the "cross-compiling" mode that we offer with this tool.  And,
another Tilera tool that stores data in the SPI ROM for bootup
configuration would also have to be similarly reworked.  And, if any of our
customers have taken advantage of the part of the SPI ROM that is reserved
for applications, they'd have to make that change as well.  This seems like
a problem.  Plus, for what it's worth, it would make it impossible for us
to use our tools on top of another OS, since they'd be dependent on a
Linux-only interface.

None of this is impossible to change, but my sense is that a basically
trivial character device that makes use of the existing Tilera hypervisor's
virtualization of the flash ROM should not be particularly problematic.  It
is true that it may not be worth grouping with other flash ROM character
devices, since it has almost no "flash ROM" characteristics due to the
hypervisor virtualization, but I will leave that up to Arnd.  I'm happy to
add some comments to the driver explaining what makes it different.

I hope this adequately addresses your concerns.

-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com

[PATCH v2] arch/tile: add hypervisor-based character driver for SPI flash ROM

[Chris Metcalf]

The first version of this patch proposed an arch/tile/drivers/ directory,
but the consensus was that this was probably a poor choice for a place to
group Tilera-specific drivers, and that in any case grouping by platform
was discouraged, and grouping by function was preferred.

This driver has no particular "peer" drivers it can be grouped with.
It is sort of like an MTD driver for SPI ROM, but it doesn't group well
with the other MTD devices since it relies on hypervisor virtualization
to handle many of the irritating aspects of flash ROM management: sector
awareness, background read for sub-sector writes, bit examination to
determine whether a sector erase needs to be issued, etc.  It is in fact
more like an EEPROM driver, but the hypervisor virtualization does require
a "flush" command if you wish to commit a sector write prior to writing
to a different sector, and this is sufficiently different from generic
I2C/SPI EEPROMs that as a result it doesn't group well with them either.

The simple character device is already in use by a range of Tilera
SPI ROM management tools, as well as by customers.  In addition, using
the simple character device actually simplifies the userspace tools,
since they don't need to manage sector erase, background read, etc.
This both simplifies the code (since we can uniformly manage plain files
and the SPI ROM) as well as makes the user code portable to non-Linux
platforms that don't offer the same MTD ioctls.

Signed-off-by: Chris Metcalf <cmetcalf@tilera.com>
---
 arch/tile/include/hv/drv_srom_intf.h |   41 +++
 drivers/char/Kconfig                 |   11 +
 drivers/char/Makefile                |    2 +
 drivers/char/tile-srom.c             |  453 ++++++++++++++++++++++++++++++++++
 4 files changed, 507 insertions(+), 0 deletions(-)
 create mode 100644 arch/tile/include/hv/drv_srom_intf.h
 create mode 100644 drivers/char/tile-srom.c

diff --git a/arch/tile/include/hv/drv_srom_intf.h b/arch/tile/include/hv/drv_srom_intf.h
new file mode 100644
index 0000000..6395faa
--- /dev/null
+++ b/arch/tile/include/hv/drv_srom_intf.h
@@ -0,0 +1,41 @@
+/*
+ * Copyright 2011 Tilera Corporation. All Rights Reserved.
+ *
+ *   This program is free software; you can redistribute it and/or
+ *   modify it under the terms of the GNU General Public License
+ *   as published by the Free Software Foundation, version 2.
+ *
+ *   This program is distributed in the hope that it will be useful, but
+ *   WITHOUT ANY WARRANTY; without even the implied warranty of
+ *   MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or
+ *   NON INFRINGEMENT.  See the GNU General Public License for
+ *   more details.
+ */
+
+/**
+ * @file drv_srom_intf.h
+ * Interface definitions for the SPI Flash ROM driver.
+ */
+
+#ifndef _SYS_HV_INCLUDE_DRV_SROM_INTF_H
+#define _SYS_HV_INCLUDE_DRV_SROM_INTF_H
+
+/** Read this offset to get the total device size. */
+#define SROM_TOTAL_SIZE_OFF   0xF0000000
+
+/** Read this offset to get the device sector size. */
+#define SROM_SECTOR_SIZE_OFF  0xF0000004
+
+/** Read this offset to get the device page size. */
+#define SROM_PAGE_SIZE_OFF    0xF0000008
+
+/** Write this offset to flush any pending writes. */
+#define SROM_FLUSH_OFF        0xF1000000
+
+/** Write this offset, plus the byte offset of the start of a sector, to
+ *  erase a sector.  Any write data is ignored, but there must be at least
+ *  one byte of write data.  Only applies when the driver is in MTD mode.
+ */
+#define SROM_ERASE_OFF        0xF2000000
+
+#endif /* _SYS_HV_INCLUDE_DRV_SROM_INTF_H */
diff --git a/drivers/char/Kconfig b/drivers/char/Kconfig
index ad59b4e..d3c84c5 100644
--- a/drivers/char/Kconfig
+++ b/drivers/char/Kconfig
@@ -616,5 +616,16 @@ config MSM_SMD_PKT
 	  Enables userspace clients to read and write to some packet SMD
 	  ports via device interface for MSM chipset.
 
+config TILE_SROM
+	bool "Character-device access via hypervisor to the Tilera SPI ROM"
+	depends on TILE
+	default y
+	---help---
+	  This device provides character-level read-write access
+	  to the SROM, typically via the "0", "1", "2", and "info"
+	  devices in /dev/srom/.  The Tilera hypervisor makes the
+	  flash device appear much like a simple EEPROM, and knows
+	  how to partition a single ROM for multiple purposes.
+
 endmenu
 
diff --git a/drivers/char/Makefile b/drivers/char/Makefile
index 7a00672..32762ba 100644
--- a/drivers/char/Makefile
+++ b/drivers/char/Makefile
@@ -63,3 +63,5 @@ obj-$(CONFIG_RAMOOPS)		+= ramoops.o
 
 obj-$(CONFIG_JS_RTC)		+= js-rtc.o
 js-rtc-y = rtc.o
+
+obj-$(CONFIG_TILE_SROM)		+= tile-srom.o
diff --git a/drivers/char/tile-srom.c b/drivers/char/tile-srom.c
new file mode 100644
index 0000000..9e03826
--- /dev/null
+++ b/drivers/char/tile-srom.c
@@ -0,0 +1,453 @@
+/*
+ * Copyright 2011 Tilera Corporation. All Rights Reserved.
+ *
+ *   This program is free software; you can redistribute it and/or
+ *   modify it under the terms of the GNU General Public License
+ *   as published by the Free Software Foundation, version 2.
+ *
+ *   This program is distributed in the hope that it will be useful, but
+ *   WITHOUT ANY WARRANTY; without even the implied warranty of
+ *   MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or
+ *   NON INFRINGEMENT.  See the GNU General Public License for
+ *   more details.
+ *
+ * SPI Flash ROM driver
+ *
+ * This source code is derived from code provided in "Linux Device
+ * Drivers" by Alessandro Rubini and Jonathan Corbet, published by
+ * O'Reilly & Associates.
+ */
+
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/init.h>
+#include <linux/kernel.h>	/* printk() */
+#include <linux/slab.h>		/* kmalloc() */
+#include <linux/fs.h>		/* everything... */
+#include <linux/errno.h>	/* error codes */
+#include <linux/types.h>	/* size_t */
+#include <linux/proc_fs.h>
+#include <linux/fcntl.h>	/* O_ACCMODE */
+#include <linux/aio.h>
+#include <linux/pagemap.h>
+#include <linux/hugetlb.h>
+#include <linux/uaccess.h>
+#include <hv/hypervisor.h>
+#include <linux/ioctl.h>
+#include <linux/cdev.h>
+#include <linux/delay.h>
+#include <hv/drv_srom_intf.h>
+
+/*
+ * Size of our hypervisor I/O requests.  We break up large transfers
+ * so that we don't spend large uninterrupted spans of time in the
+ * hypervisor.  Erasing an SROM sector takes a significant fraction of
+ * a second, so if we allowed the user to, say, do one I/O to write the
+ * entire ROM, we'd get soft lockup timeouts, or worse.
+ */
+#define SROM_CHUNK_SIZE ((size_t)4096)
+
+/*
+ * When hypervisor is busy (e.g. erasing), poll the status periodically.
+ */
+
+/*
+ * Interval to poll the state in msec
+ */
+#define SROM_WAIT_TRY_INTERVAL 20
+
+/*
+ * Maximum times to poll the state
+ */
+#define SROM_MAX_WAIT_TRY_TIMES 1000
+
+struct srom_dev {
+	struct cdev cdev;		/* Character device structure */
+	int hv_devhdl;			/* Handle for hypervisor device */
+	u32 size;		        /* Size of this device */
+	char *info_string;		/* String returned by the info device
+					   if this is it; NULL otherwise */
+	struct mutex srom_lock;		/* Allow only one accessor at a time */
+};
+
+static int srom_major;		/* Dynamic major by default */
+static int srom_devs =    4;	/* One per SROM partition plus one info file */
+
+/* Minor number of the info file */
+static inline int srom_info_minor(void)
+{
+	return srom_devs - 1;
+}
+
+module_param(srom_major, int, 0);
+module_param(srom_devs, int, 0);
+MODULE_AUTHOR("Tilera Corporation");
+MODULE_LICENSE("Dual BSD/GPL");
+
+static struct srom_dev *srom_devices; /* allocated in srom_init */
+
+/*
+ * Handle calling the hypervisor and managing EAGAIN/EBUSY.
+ */
+
+static ssize_t _srom_read(int hv_devhdl, void *buf,
+			  loff_t off, size_t count)
+{
+	int retval, retries = SROM_MAX_WAIT_TRY_TIMES;
+	for (;;) {
+		retval = hv_dev_pread(hv_devhdl, 0, (HV_VirtAddr)buf,
+				      count, off);
+		if (retval >= 0)
+			return retval;
+		if (retval == HV_EAGAIN)
+			continue;
+		if (retval == HV_EBUSY && --retries > 0) {
+			msleep(SROM_WAIT_TRY_INTERVAL);
+			continue;
+		}
+		pr_err("_srom_read: error %d\n", retval);
+		return -EIO;
+	}
+}
+
+static ssize_t _srom_write(int hv_devhdl, const void *buf,
+			   loff_t off, size_t count)
+{
+	int retval, retries = SROM_MAX_WAIT_TRY_TIMES;
+	for (;;) {
+		retval = hv_dev_pwrite(hv_devhdl, 0, (HV_VirtAddr)buf,
+				       count, off);
+		if (retval >= 0)
+			return retval;
+		if (retval == HV_EAGAIN)
+			continue;
+		if (retval == HV_EBUSY && --retries > 0) {
+			msleep(SROM_WAIT_TRY_INTERVAL);
+			continue;
+		}
+		pr_err("_srom_write: error %d\n", retval);
+		return -EIO;
+	}
+}
+
+/**
+ * srom_open() - Device open routine.
+ * @inode: Inode for this device.
+ * @filp: File for this specific open of the device.
+ *
+ * Returns zero, or an error code.
+ */
+static int srom_open(struct inode *inode, struct file *filp)
+{
+	struct srom_dev *dev;
+
+	/* Find the device */
+	dev = container_of(inode->i_cdev, struct srom_dev, cdev);
+
+	/* Now open the hypervisor device if we haven't already. */
+	if (dev->hv_devhdl == 0) {
+		char buf[20];
+		int instance = iminor(inode);
+		if (instance != srom_info_minor()) {
+			sprintf(buf, "srom/0/%d", instance);
+			dev->hv_devhdl = hv_dev_open((HV_VirtAddr)buf, 0);
+			if (dev->hv_devhdl > 0) {
+				dev->size = 0;
+				if (mutex_lock_interruptible(&dev->srom_lock))
+					return -ERESTARTSYS;
+				_srom_read(dev->hv_devhdl, &dev->size,
+					   SROM_TOTAL_SIZE_OFF,
+					   sizeof(dev->size));
+				mutex_unlock(&dev->srom_lock);
+				i_size_write(inode, dev->size);
+			}
+		} else {
+			u32 sector_size = 0;
+			u32 page_size = 0;
+
+			sprintf(buf, "srom/0/0");
+			dev->hv_devhdl = hv_dev_open((HV_VirtAddr)buf, 0);
+			if (dev->hv_devhdl > 0) {
+				static const int info_string_size = 80;
+
+				if (mutex_lock_interruptible(&dev->srom_lock))
+					return -ERESTARTSYS;
+				_srom_read(dev->hv_devhdl, &sector_size,
+					   SROM_SECTOR_SIZE_OFF,
+					   sizeof(sector_size));
+				_srom_read(dev->hv_devhdl, &page_size,
+					   SROM_PAGE_SIZE_OFF,
+					   sizeof(page_size));
+				mutex_unlock(&dev->srom_lock);
+
+				dev->info_string = kmalloc(info_string_size,
+							GFP_KERNEL);
+				snprintf(dev->info_string, info_string_size,
+					"sector_size: %d\npage_size: %d\n",
+					sector_size, page_size);
+			}
+		}
+	}
+
+	/* If we tried and failed to open it, fail. */
+	if (dev->hv_devhdl < 0) {
+		switch (dev->hv_devhdl)	{
+		case HV_ENODEV:
+			return -ENODEV;
+		default:
+			return (ssize_t)dev->hv_devhdl;
+		}
+	}
+
+	filp->private_data = dev;
+
+	return 0;          /* success */
+}
+
+
+/**
+ * srom_release() - Device release routine.
+ * @inode: Inode for this device.
+ * @filp: File for this specific open of the device.
+ *
+ * Returns zero, or an error code.
+ */
+static int srom_release(struct inode *inode, struct file *filp)
+{
+	struct srom_dev *dev = filp->private_data;
+	int retval = 0;
+	char dummy;
+
+	/*
+	 * First we need to make sure we've flushed anything written to
+	 * the ROM.
+	 */
+	if (mutex_lock_interruptible(&dev->srom_lock)) {
+		retval = -ERESTARTSYS;
+		filp->private_data = NULL;
+		return retval;
+	}
+
+	if (dev->hv_devhdl > 0)
+		_srom_write(dev->hv_devhdl, &dummy, SROM_FLUSH_OFF, 1);
+
+	mutex_unlock(&dev->srom_lock);
+	filp->private_data = NULL;
+
+	return retval;
+}
+
+
+/**
+ * srom_read() - Read (control) data from the device.
+ * @filp: File for this specific open of the device.
+ * @buf: User's data buffer.
+ * @count: Number of bytes requested.
+ * @f_pos: File position.
+ *
+ * Returns number of bytes read, or an error code.
+ */
+static ssize_t srom_read(struct file *filp, char __user *buf,
+			 size_t count, loff_t *f_pos)
+{
+	int retval = 0;
+	void *kernbuf;
+	struct srom_dev *dev = filp->private_data;
+
+	if (dev->hv_devhdl < 0)
+		return -EINVAL;
+
+	if (dev->info_string) {
+		int info_len = strlen(dev->info_string);
+		int bytes_avail = info_len - *f_pos;
+		int xfer_len = (bytes_avail < count) ? bytes_avail : count;
+
+		if (xfer_len <= 0)
+			return 0;
+
+		if (copy_to_user(buf, dev->info_string + *f_pos, xfer_len))
+			return -EFAULT;
+		*f_pos += xfer_len;
+		return xfer_len;
+	}
+
+	kernbuf = kmalloc(SROM_CHUNK_SIZE, GFP_KERNEL);
+	if (!kernbuf)
+		return -ENOMEM;
+
+	if (mutex_lock_interruptible(&dev->srom_lock)) {
+		retval = -ERESTARTSYS;
+		kfree(kernbuf);
+		return retval;
+	}
+
+	while (count) {
+		int hv_retval;
+		int bytes_this_pass = min(count, SROM_CHUNK_SIZE);
+
+		hv_retval = _srom_read(dev->hv_devhdl, kernbuf,
+				       *f_pos, bytes_this_pass);
+		if (hv_retval > 0) {
+			if (copy_to_user(buf, kernbuf, hv_retval) != 0) {
+				retval = -EFAULT;
+				break;
+			}
+		} else if (hv_retval <= 0) {
+			if (retval == 0)
+				retval = hv_retval;
+			break;
+		}
+
+		retval += hv_retval;
+		*f_pos += hv_retval;
+		buf += hv_retval;
+		count -= hv_retval;
+	}
+
+	mutex_unlock(&dev->srom_lock);
+	kfree(kernbuf);
+
+	return retval;
+}
+
+/**
+ * srom_write() - Write (control) data to the device.
+ * @filp: File for this specific open of the device.
+ * @buf: User's data buffer.
+ * @count: Number of bytes requested.
+ * @f_pos: File position.
+ *
+ * Returns number of bytes written, or an error code.
+ */
+static ssize_t srom_write(struct file *filp, const char __user *buf,
+			  size_t count, loff_t *f_pos)
+{
+	int retval = 0;
+	void *kernbuf;
+	struct srom_dev *dev = filp->private_data;
+
+	if (dev->hv_devhdl < 0 || dev->info_string)
+		return -EINVAL;
+
+	kernbuf = kmalloc(SROM_CHUNK_SIZE, GFP_KERNEL);
+	if (!kernbuf)
+		return -ENOMEM;
+
+	if (mutex_lock_interruptible(&dev->srom_lock)) {
+		retval = -ERESTARTSYS;
+		kfree(kernbuf);
+		return retval;
+	}
+
+	while (count) {
+		int hv_retval;
+		int bytes_this_pass = min(count, SROM_CHUNK_SIZE);
+
+		if (copy_from_user(kernbuf, buf, bytes_this_pass) != 0) {
+			retval = -EFAULT;
+			break;
+		}
+
+		hv_retval = _srom_write(dev->hv_devhdl, kernbuf,
+					*f_pos, bytes_this_pass);
+		if (hv_retval <= 0) {
+			if (retval == 0)
+				retval = hv_retval;
+			break;
+		}
+
+		retval += hv_retval;
+		*f_pos += hv_retval;
+		buf += hv_retval;
+		count -= hv_retval;
+	}
+
+	mutex_unlock(&dev->srom_lock);
+	kfree(kernbuf);
+
+	return retval;
+}
+
+
+/*
+ * The fops
+ */
+static const struct file_operations srom_fops = {
+	.owner =     THIS_MODULE,
+	.llseek =    generic_file_llseek,
+	.read =	     srom_read,
+	.write =     srom_write,
+	.open =	     srom_open,
+	.release =   srom_release,
+};
+
+/**
+ * srom_setup_cdev() - Set up a device instance in the cdev table.
+ * @dev: Per-device SROM state.
+ * @index: Device to set up.
+ */
+static void srom_setup_cdev(struct srom_dev *dev, int index)
+{
+	int err, devno = MKDEV(srom_major, index);
+
+	cdev_init(&dev->cdev, &srom_fops);
+	dev->cdev.owner = THIS_MODULE;
+	dev->cdev.ops = &srom_fops;
+	err = cdev_add(&dev->cdev, devno, 1);
+	/* Fail gracefully if need be */
+	if (err)
+		pr_notice("Error %d adding srom%d", err, index);
+}
+
+/** srom_init() - Initialize the driver's module. */
+static int srom_init(void)
+{
+	int result, i;
+	dev_t dev = MKDEV(srom_major, 0);
+
+	/*
+	 * Register our major, and accept a dynamic number.
+	 */
+	if (srom_major)
+		result = register_chrdev_region(dev, srom_devs, "srom");
+	else {
+		result = alloc_chrdev_region(&dev, 0, srom_devs, "srom");
+		srom_major = MAJOR(dev);
+	}
+	if (result < 0)
+		return result;
+
+
+	/*
+	 * Allocate the devices -- we can't have them static, as the number
+	 * can be specified at load time.
+	 */
+	srom_devices = kzalloc(srom_devs * sizeof(struct srom_dev),
+			       GFP_KERNEL);
+	if (!srom_devices) {
+		unregister_chrdev_region(dev, srom_devs);
+		return -ENOMEM;
+	}
+	for (i = 0; i < srom_devs; i++) {
+		srom_setup_cdev(srom_devices + i, i);
+		mutex_init(&srom_devices[i].srom_lock);
+	}
+
+	return 0; /* succeed */
+}
+
+/** srom_cleanup() - Clean up the driver's module. */
+static void srom_cleanup(void)
+{
+	int i;
+
+	for (i = 0; i < srom_devs; i++) {
+		cdev_del(&srom_devices[i].cdev);
+		kfree(srom_devices[i].info_string);
+	}
+	kfree(srom_devices);
+	unregister_chrdev_region(MKDEV(srom_major, 0), srom_devs);
+}
+
+module_init(srom_init);
+module_exit(srom_cleanup);
-- 
1.6.5.2

Re: [PATCH v2] arch/tile: add hypervisor-based character driver for SPI flash ROM

[Greg KH]

On Sat, May 28, 2011 at 11:13:54AM -0400, Chris Metcalf wrote:
> --- /dev/null
> +++ b/drivers/char/tile-srom.c
> @@ -0,0 +1,453 @@
> +/*
> + * Copyright 2011 Tilera Corporation. All Rights Reserved.
> + *
> + *   This program is free software; you can redistribute it and/or
> + *   modify it under the terms of the GNU General Public License
> + *   as published by the Free Software Foundation, version 2.
> + *
> + *   This program is distributed in the hope that it will be useful, but
> + *   WITHOUT ANY WARRANTY; without even the implied warranty of
> + *   MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or
> + *   NON INFRINGEMENT.  See the GNU General Public License for
> + *   more details.
> + *
> + * SPI Flash ROM driver
> + *
> + * This source code is derived from code provided in "Linux Device
> + * Drivers" by Alessandro Rubini and Jonathan Corbet, published by
> + * O'Reilly & Associates.
> + */

LDD version 2?  Wow, this driver is old.

> +MODULE_LICENSE("Dual BSD/GPL");

Wait, where did the BSD come from?  You just said it was GPL only above
in the header of the file?

> +/**
> + * srom_setup_cdev() - Set up a device instance in the cdev table.
> + * @dev: Per-device SROM state.
> + * @index: Device to set up.
> + */
> +static void srom_setup_cdev(struct srom_dev *dev, int index)
> +{
> +	int err, devno = MKDEV(srom_major, index);
> +
> +	cdev_init(&dev->cdev, &srom_fops);
> +	dev->cdev.owner = THIS_MODULE;
> +	dev->cdev.ops = &srom_fops;
> +	err = cdev_add(&dev->cdev, devno, 1);
> +	/* Fail gracefully if need be */
> +	if (err)
> +		pr_notice("Error %d adding srom%d", err, index);
> +}

As you are only using 1 minor device, why not just use a misc device
instead?  It's simpler, and you get the sysfs code for free, which you
forgot to do, so your device node will never show up in userspace :(

thanks,

greg k-h

Re: [PATCH v2] arch/tile: add hypervisor-based character driver for SPI flash ROM

[Chris Metcalf]

On 5/28/2011 5:23 PM, Greg KH wrote:
> On Sat, May 28, 2011 at 11:13:54AM -0400, Chris Metcalf wrote:
>> + * This source code is derived from code provided in "Linux Device
>> + * Drivers" by Alessandro Rubini and Jonathan Corbet, published by
>> + * O'Reilly & Associates.
>> + */
> LDD version 2?  Wow, this driver is old.

The driver was originally written in 2007. LDD3 came out in 2005, right?  I
guess we were a little behind the curve :-)

>> +MODULE_LICENSE("Dual BSD/GPL");
> Wait, where did the BSD come from?  You just said it was GPL only above
> in the header of the file?

Good point.  We have a boiler-plate auto-generated GPL license comment that
we use for kernel sources.  In general, we've tended toward licenses that
are as available to the community as possible -- for example, the string
code that we've written is under GPL in the kernel (and glibc) but under a
BSD license in newlib, because we wrote it all from scratch and can do
that.  And, since we're hardware vendors, our general goal is to enable
whatever kinds of software will sell chips.  :-)

My guess is that we should be using a consistent GPL-only license for these
kinds of kernel drivers, because frankly, they aren't useful outside the
context of Linux.  I'll double-check with the rest of the software team,
but I think we should probably just switch this over to a straight "GPL"
license.

> As you are only using 1 minor device, why not just use a misc device
> instead?  It's simpler, and you get the sysfs code for free, which you
> forgot to do, so your device node will never show up in userspace :(

Interesting; this appears to be a bug.  We use 4 minors (see "srom_devs =
4" higher up).  I'll fix this.  We may have some other devices that would
benefit from being recast as misc devices, so I'll look at our set of
internal devices.

Is there a good example of a character device that has multiple minors and
also is registered with sysfs?

-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com

Re: [PATCH v2] arch/tile: add hypervisor-based character driver for SPI flash ROM

[Greg KH]

On Sat, May 28, 2011 at 08:32:07PM -0400, Chris Metcalf wrote:
> > As you are only using 1 minor device, why not just use a misc device
> > instead?  It's simpler, and you get the sysfs code for free, which you
> > forgot to do, so your device node will never show up in userspace :(
> 
> Interesting; this appears to be a bug.  We use 4 minors (see "srom_devs =
> 4" higher up).  I'll fix this.  We may have some other devices that would
> benefit from being recast as misc devices, so I'll look at our set of
> internal devices.

This kind of implies that you didn't test this code, right?  You might
want to do that next time :)

> Is there a good example of a character device that has multiple minors and
> also is registered with sysfs?

Lots of them, look all over the kernel, and in LDD3 there's even an
outdated example of one I think.

thanks,

greg k-h

Re: [PATCH v2] arch/tile: add hypervisor-based character driver for SPI flash ROM

[Chris Metcalf]

On 5/29/2011 7:45 AM, Greg KH wrote:
> On Sat, May 28, 2011 at 08:32:07PM -0400, Chris Metcalf wrote:
>>> As you are only using 1 minor device, why not just use a misc device
>>> instead?  It's simpler, and you get the sysfs code for free, which you
>>> forgot to do, so your device node will never show up in userspace :(
>> Interesting; this appears to be a bug.  We use 4 minors (see "srom_devs =
>> 4" higher up).  I'll fix this.  We may have some other devices that would
>> benefit from being recast as misc devices, so I'll look at our set of
>> internal devices.
> This kind of implies that you didn't test this code, right?  You might
> want to do that next time :)

No, this bug has been in the code since day one (I just double-checked our
SCM), and it has always worked fine.  I'm looking into how this is possible
now, but trust me, we've tested this aspect of the driver the whole time. :-)

>> Is there a good example of a character device that has multiple minors and
>> also is registered with sysfs?
> Lots of them, look all over the kernel, and in LDD3 there's even an
> outdated example of one I think.

 I didn't actually find many drivers that have both cdev_add with count>1,
and mention sysfs or kobj, but char/raw.c is one that looks pretty clean. 
(I found only four others total: uio/uio.c, s390/char/vmlogrdr.c,
staging/comedi/comedi_fops.c, and staging/vme/devices/vme_user.c.)

-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com

Re: [PATCH v2] arch/tile: add hypervisor-based character driver for SPI flash ROM

[Greg KH]

On Sun, May 29, 2011 at 08:18:36AM -0400, Chris Metcalf wrote:
> >> Is there a good example of a character device that has multiple minors and
> >> also is registered with sysfs?
> > Lots of them, look all over the kernel, and in LDD3 there's even an
> > outdated example of one I think.
> 
>  I didn't actually find many drivers that have both cdev_add with count>1,
> and mention sysfs or kobj, but char/raw.c is one that looks pretty clean. 
> (I found only four others total: uio/uio.c, s390/char/vmlogrdr.c,
> staging/comedi/comedi_fops.c, and staging/vme/devices/vme_user.c.)

No, you don't need "raw" kobjects or sysfs at all.  Just use the
device_create() call.

Hope this helps,

greg k-h

Re: [PATCH v2] arch/tile: add hypervisor-based character driver for SPI flash ROM

[Arnd Bergmann]

On Sunday 29 May 2011 14:18:36 Chris Metcalf wrote:
> On 5/29/2011 7:45 AM, Greg KH wrote:
> > On Sat, May 28, 2011 at 08:32:07PM -0400, Chris Metcalf wrote:
> >>> As you are only using 1 minor device, why not just use a misc device
> >>> instead?  It's simpler, and you get the sysfs code for free, which you
> >>> forgot to do, so your device node will never show up in userspace :(
> >> Interesting; this appears to be a bug.  We use 4 minors (see "srom_devs =
> >> 4" higher up).  I'll fix this.  We may have some other devices that would
> >> benefit from being recast as misc devices, so I'll look at our set of
> >> internal devices.
> > This kind of implies that you didn't test this code, right?  You might
> > want to do that next time :)
> 
> No, this bug has been in the code since day one (I just double-checked our
> SCM), and it has always worked fine.  I'm looking into how this is possible
> now, but trust me, we've tested this aspect of the driver the whole time. :-)
> 

AFAICT, the driver just calls cdev_add in a loop, adding one device
add a time. This is actually a correct way to register multiple
character devices, but simply passing the number of devices you
want to add as the third argument would be simpler.

On a related note, the number of devices you add is a module parameter,
which seems a bit backwards, since the hypervisor should actually
know how many devices there are. Can't you just ask the hypervisor?

	Arnd

Re: [PATCH v2] arch/tile: add hypervisor-based character driver for SPI flash ROM

[Chris Metcalf]

On 5/29/2011 11:45 AM, Arnd Bergmann wrote:
> On Sunday 29 May 2011 14:18:36 Chris Metcalf wrote:
>> On 5/29/2011 7:45 AM, Greg KH wrote:
>>> On Sat, May 28, 2011 at 08:32:07PM -0400, Chris Metcalf wrote:
>>>>> As you are only using 1 minor device, why not just use a misc device
>>>>> instead?  It's simpler, and you get the sysfs code for free, which you
>>>>> forgot to do, so your device node will never show up in userspace :(
>>>> Interesting; this appears to be a bug.  We use 4 minors (see "srom_devs =
>>>> 4" higher up).  I'll fix this.  We may have some other devices that would
>>>> benefit from being recast as misc devices, so I'll look at our set of
>>>> internal devices.
>>> This kind of implies that you didn't test this code, right?  You might
>>> want to do that next time :)
>> No, this bug has been in the code since day one (I just double-checked our
>> SCM), and it has always worked fine.  I'm looking into how this is possible
>> now, but trust me, we've tested this aspect of the driver the whole time. :-)
>>
> AFAICT, the driver just calls cdev_add in a loop, adding one device
> add a time. This is actually a correct way to register multiple
> character devices, but simply passing the number of devices you
> want to add as the third argument would be simpler.

Good catch!  Amusingly, I audited our other drivers that we haven't tried
to push back yet, and did find one that actually did have this bug: it was
doing cdev_add() without the surrounding loop, but without passing the
right minor count from alloc_chrdev_region() (although in that case the
default value was "1" in any case).

I restructured the tile-srom init code to call cdev_add() once, using a
single global "struct cdev".  The "open" routine now uses "iminor()"
instead of "container_of()" to get the proper srom_dev structure with the
driver's per-partition info.

> On a related note, the number of devices you add is a module parameter,
> which seems a bit backwards, since the hypervisor should actually
> know how many devices there are. Can't you just ask the hypervisor?

Fair point.  I restructured the init so we loop calling hv_dev_open() until
the hypervisor says we've hit a bad partition number, and that's the number
of partitions the Linux driver then supports.

-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com

[PATCH v3] arch/tile: add hypervisor-based character driver for SPI flash ROM

[Chris Metcalf]

The first version of this patch proposed an arch/tile/drivers/ directory,
but the consensus was that this was probably a poor choice for a place to
group Tilera-specific drivers, and that in any case grouping by platform
was discouraged, and grouping by function was preferred.

This version of the patch addresses various issues raised in the
community, primarily the absence of sysfs integration.  The sysfs
integration now handles passing information on sector size, page size,
and total partition size to userspace as well.  In addition, we now
use a single "struct cdev" to manage all the partition minor devices,
and dynamically discover the correct number of partitions from the
hypervisor rather than using a module_param with a default value.

This driver has no particular "peer" drivers it can be grouped with.
It is sort of like an MTD driver for SPI ROM, but it doesn't group well
with the other MTD devices since it relies on hypervisor virtualization
to handle many of the irritating aspects of flash ROM management: sector
awareness, background read for sub-sector writes, bit examination to
determine whether a sector erase needs to be issued, etc.  It is in fact
more like an EEPROM driver, but the hypervisor virtualization does require
a "flush" command if you wish to commit a sector write prior to writing
to a different sector, and this is sufficiently different from generic
I2C/SPI EEPROMs that as a result it doesn't group well with them either.

The simple character device is already in use by a range of Tilera
SPI ROM management tools, as well as by customers.  In addition, using
the simple character device actually simplifies the userspace tools,
since they don't need to manage sector erase, background read, etc.
This both simplifies the code (since we can uniformly manage plain files
and the SPI ROM) as well as makes the user code portable to non-Linux
platforms that don't offer the same MTD ioctls.

Signed-off-by: Chris Metcalf <cmetcalf@tilera.com>
---
 arch/tile/include/hv/drv_srom_intf.h |   41 +++
 drivers/char/Kconfig                 |   11 +
 drivers/char/Makefile                |    2 +
 drivers/char/tile-srom.c             |  481 ++++++++++++++++++++++++++++++++++
 4 files changed, 535 insertions(+), 0 deletions(-)
 create mode 100644 arch/tile/include/hv/drv_srom_intf.h
 create mode 100644 drivers/char/tile-srom.c

diff --git a/arch/tile/include/hv/drv_srom_intf.h b/arch/tile/include/hv/drv_srom_intf.h
new file mode 100644
index 0000000..6395faa
--- /dev/null
+++ b/arch/tile/include/hv/drv_srom_intf.h
@@ -0,0 +1,41 @@
+/*
+ * Copyright 2011 Tilera Corporation. All Rights Reserved.
+ *
+ *   This program is free software; you can redistribute it and/or
+ *   modify it under the terms of the GNU General Public License
+ *   as published by the Free Software Foundation, version 2.
+ *
+ *   This program is distributed in the hope that it will be useful, but
+ *   WITHOUT ANY WARRANTY; without even the implied warranty of
+ *   MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or
+ *   NON INFRINGEMENT.  See the GNU General Public License for
+ *   more details.
+ */
+
+/**
+ * @file drv_srom_intf.h
+ * Interface definitions for the SPI Flash ROM driver.
+ */
+
+#ifndef _SYS_HV_INCLUDE_DRV_SROM_INTF_H
+#define _SYS_HV_INCLUDE_DRV_SROM_INTF_H
+
+/** Read this offset to get the total device size. */
+#define SROM_TOTAL_SIZE_OFF   0xF0000000
+
+/** Read this offset to get the device sector size. */
+#define SROM_SECTOR_SIZE_OFF  0xF0000004
+
+/** Read this offset to get the device page size. */
+#define SROM_PAGE_SIZE_OFF    0xF0000008
+
+/** Write this offset to flush any pending writes. */
+#define SROM_FLUSH_OFF        0xF1000000
+
+/** Write this offset, plus the byte offset of the start of a sector, to
+ *  erase a sector.  Any write data is ignored, but there must be at least
+ *  one byte of write data.  Only applies when the driver is in MTD mode.
+ */
+#define SROM_ERASE_OFF        0xF2000000
+
+#endif /* _SYS_HV_INCLUDE_DRV_SROM_INTF_H */
diff --git a/drivers/char/Kconfig b/drivers/char/Kconfig
index 49502bc..5afd610 100644
--- a/drivers/char/Kconfig
+++ b/drivers/char/Kconfig
@@ -616,5 +616,16 @@ config MSM_SMD_PKT
 	  Enables userspace clients to read and write to some packet SMD
 	  ports via device interface for MSM chipset.
 
+config TILE_SROM
+	bool "Character-device access via hypervisor to the Tilera SPI ROM"
+	depends on TILE
+	default y
+	---help---
+	  This device provides character-level read-write access
+	  to the SROM, typically via the "0", "1", "2", and "info"
+	  devices in /dev/srom/.  The Tilera hypervisor makes the
+	  flash device appear much like a simple EEPROM, and knows
+	  how to partition a single ROM for multiple purposes.
+
 endmenu
 
diff --git a/drivers/char/Makefile b/drivers/char/Makefile
index 7a00672..32762ba 100644
--- a/drivers/char/Makefile
+++ b/drivers/char/Makefile
@@ -63,3 +63,5 @@ obj-$(CONFIG_RAMOOPS)		+= ramoops.o
 
 obj-$(CONFIG_JS_RTC)		+= js-rtc.o
 js-rtc-y = rtc.o
+
+obj-$(CONFIG_TILE_SROM)		+= tile-srom.o
diff --git a/drivers/char/tile-srom.c b/drivers/char/tile-srom.c
new file mode 100644
index 0000000..cf3ee00
--- /dev/null
+++ b/drivers/char/tile-srom.c
@@ -0,0 +1,481 @@
+/*
+ * Copyright 2011 Tilera Corporation. All Rights Reserved.
+ *
+ *   This program is free software; you can redistribute it and/or
+ *   modify it under the terms of the GNU General Public License
+ *   as published by the Free Software Foundation, version 2.
+ *
+ *   This program is distributed in the hope that it will be useful, but
+ *   WITHOUT ANY WARRANTY; without even the implied warranty of
+ *   MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or
+ *   NON INFRINGEMENT.  See the GNU General Public License for
+ *   more details.
+ *
+ * SPI Flash ROM driver
+ *
+ * This source code is derived from code provided in "Linux Device
+ * Drivers, Third Edition", by Jonathan Corbet, Alessandro Rubini, and
+ * Greg Kroah-Hartman, published by O'Reilly Media, Inc.
+ */
+
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/init.h>
+#include <linux/kernel.h>	/* printk() */
+#include <linux/slab.h>		/* kmalloc() */
+#include <linux/fs.h>		/* everything... */
+#include <linux/errno.h>	/* error codes */
+#include <linux/types.h>	/* size_t */
+#include <linux/proc_fs.h>
+#include <linux/fcntl.h>	/* O_ACCMODE */
+#include <linux/aio.h>
+#include <linux/pagemap.h>
+#include <linux/hugetlb.h>
+#include <linux/uaccess.h>
+#include <linux/platform_device.h>
+#include <hv/hypervisor.h>
+#include <linux/ioctl.h>
+#include <linux/cdev.h>
+#include <linux/delay.h>
+#include <hv/drv_srom_intf.h>
+
+/*
+ * Size of our hypervisor I/O requests.  We break up large transfers
+ * so that we don't spend large uninterrupted spans of time in the
+ * hypervisor.  Erasing an SROM sector takes a significant fraction of
+ * a second, so if we allowed the user to, say, do one I/O to write the
+ * entire ROM, we'd get soft lockup timeouts, or worse.
+ */
+#define SROM_CHUNK_SIZE ((size_t)4096)
+
+/*
+ * When hypervisor is busy (e.g. erasing), poll the status periodically.
+ */
+
+/*
+ * Interval to poll the state in msec
+ */
+#define SROM_WAIT_TRY_INTERVAL 20
+
+/*
+ * Maximum times to poll the state
+ */
+#define SROM_MAX_WAIT_TRY_TIMES 1000
+
+struct srom_dev {
+	int hv_devhdl;			/* Handle for hypervisor device */
+	u32 total_size;			/* Size of this device */
+	u32 sector_size;		/* Size of a sector */
+	u32 page_size;			/* Size of a page */
+	struct mutex lock;		/* Allow only one accessor at a time */
+};
+
+static int srom_major;			/* Dynamic major by default */
+module_param(srom_major, int, 0);
+MODULE_AUTHOR("Tilera Corporation");
+MODULE_LICENSE("GPL");
+
+static int srom_devs;			/* Number of SROM partitions */
+static struct cdev srom_cdev;
+static struct class *srom_class;
+static struct srom_dev *srom_devices;
+
+/*
+ * Handle calling the hypervisor and managing EAGAIN/EBUSY.
+ */
+
+static ssize_t _srom_read(int hv_devhdl, void *buf,
+			  loff_t off, size_t count)
+{
+	int retval, retries = SROM_MAX_WAIT_TRY_TIMES;
+	for (;;) {
+		retval = hv_dev_pread(hv_devhdl, 0, (HV_VirtAddr)buf,
+				      count, off);
+		if (retval >= 0)
+			return retval;
+		if (retval == HV_EAGAIN)
+			continue;
+		if (retval == HV_EBUSY && --retries > 0) {
+			msleep(SROM_WAIT_TRY_INTERVAL);
+			continue;
+		}
+		pr_err("_srom_read: error %d\n", retval);
+		return -EIO;
+	}
+}
+
+static ssize_t _srom_write(int hv_devhdl, const void *buf,
+			   loff_t off, size_t count)
+{
+	int retval, retries = SROM_MAX_WAIT_TRY_TIMES;
+	for (;;) {
+		retval = hv_dev_pwrite(hv_devhdl, 0, (HV_VirtAddr)buf,
+				       count, off);
+		if (retval >= 0)
+			return retval;
+		if (retval == HV_EAGAIN)
+			continue;
+		if (retval == HV_EBUSY && --retries > 0) {
+			msleep(SROM_WAIT_TRY_INTERVAL);
+			continue;
+		}
+		pr_err("_srom_write: error %d\n", retval);
+		return -EIO;
+	}
+}
+
+/**
+ * srom_open() - Device open routine.
+ * @inode: Inode for this device.
+ * @filp: File for this specific open of the device.
+ *
+ * Returns zero, or an error code.
+ */
+static int srom_open(struct inode *inode, struct file *filp)
+{
+	filp->private_data = &srom_devices[iminor(inode)];
+	return 0;
+}
+
+
+/**
+ * srom_release() - Device release routine.
+ * @inode: Inode for this device.
+ * @filp: File for this specific open of the device.
+ *
+ * Returns zero, or an error code.
+ */
+static int srom_release(struct inode *inode, struct file *filp)
+{
+	struct srom_dev *srom = filp->private_data;
+	char dummy;
+
+	/* Make sure we've flushed anything written to the ROM. */
+	mutex_lock(&srom->lock);
+	if (srom->hv_devhdl >= 0)
+		_srom_write(srom->hv_devhdl, &dummy, SROM_FLUSH_OFF, 1);
+	mutex_unlock(&srom->lock);
+
+	filp->private_data = NULL;
+
+	return 0;
+}
+
+
+/**
+ * srom_read() - Read data from the device.
+ * @filp: File for this specific open of the device.
+ * @buf: User's data buffer.
+ * @count: Number of bytes requested.
+ * @f_pos: File position.
+ *
+ * Returns number of bytes read, or an error code.
+ */
+static ssize_t srom_read(struct file *filp, char __user *buf,
+			 size_t count, loff_t *f_pos)
+{
+	int retval = 0;
+	void *kernbuf;
+	struct srom_dev *srom = filp->private_data;
+
+	kernbuf = kmalloc(SROM_CHUNK_SIZE, GFP_KERNEL);
+	if (!kernbuf)
+		return -ENOMEM;
+
+	if (mutex_lock_interruptible(&srom->lock)) {
+		retval = -ERESTARTSYS;
+		kfree(kernbuf);
+		return retval;
+	}
+
+	while (count) {
+		int hv_retval;
+		int bytes_this_pass = min(count, SROM_CHUNK_SIZE);
+
+		hv_retval = _srom_read(srom->hv_devhdl, kernbuf,
+				       *f_pos, bytes_this_pass);
+		if (hv_retval > 0) {
+			if (copy_to_user(buf, kernbuf, hv_retval) != 0) {
+				retval = -EFAULT;
+				break;
+			}
+		} else if (hv_retval <= 0) {
+			if (retval == 0)
+				retval = hv_retval;
+			break;
+		}
+
+		retval += hv_retval;
+		*f_pos += hv_retval;
+		buf += hv_retval;
+		count -= hv_retval;
+	}
+
+	mutex_unlock(&srom->lock);
+	kfree(kernbuf);
+
+	return retval;
+}
+
+/**
+ * srom_write() - Write data to the device.
+ * @filp: File for this specific open of the device.
+ * @buf: User's data buffer.
+ * @count: Number of bytes requested.
+ * @f_pos: File position.
+ *
+ * Returns number of bytes written, or an error code.
+ */
+static ssize_t srom_write(struct file *filp, const char __user *buf,
+			  size_t count, loff_t *f_pos)
+{
+	int retval = 0;
+	void *kernbuf;
+	struct srom_dev *srom = filp->private_data;
+
+	kernbuf = kmalloc(SROM_CHUNK_SIZE, GFP_KERNEL);
+	if (!kernbuf)
+		return -ENOMEM;
+
+	if (mutex_lock_interruptible(&srom->lock)) {
+		retval = -ERESTARTSYS;
+		kfree(kernbuf);
+		return retval;
+	}
+
+	while (count) {
+		int hv_retval;
+		int bytes_this_pass = min(count, SROM_CHUNK_SIZE);
+
+		if (copy_from_user(kernbuf, buf, bytes_this_pass) != 0) {
+			retval = -EFAULT;
+			break;
+		}
+
+		hv_retval = _srom_write(srom->hv_devhdl, kernbuf,
+					*f_pos, bytes_this_pass);
+		if (hv_retval <= 0) {
+			if (retval == 0)
+				retval = hv_retval;
+			break;
+		}
+
+		retval += hv_retval;
+		*f_pos += hv_retval;
+		buf += hv_retval;
+		count -= hv_retval;
+	}
+
+	mutex_unlock(&srom->lock);
+	kfree(kernbuf);
+
+	return retval;
+}
+
+/* Provide our own implementation so we can use srom->total_size. */
+loff_t srom_llseek(struct file *filp, loff_t offset, int origin)
+{
+	struct srom_dev *srom = filp->private_data;
+
+	if (mutex_lock_interruptible(&srom->lock))
+		return -ERESTARTSYS;
+
+	switch (origin) {
+	case SEEK_END:
+		offset += srom->total_size;
+		break;
+	case SEEK_CUR:
+		offset += filp->f_pos;
+		break;
+	}
+
+	if (offset < 0 || offset > srom->total_size) {
+		offset = -EINVAL;
+	} else {
+		filp->f_pos = offset;
+		filp->f_version = 0;
+	}
+
+	mutex_unlock(&srom->lock);
+
+	return offset;
+}
+
+static ssize_t total_show(struct device *dev,
+			  struct device_attribute *attr, char *buf)
+{
+	struct srom_dev *srom = dev_get_drvdata(dev);
+	return sprintf(buf, "%u\n", srom->total_size);
+}
+
+static ssize_t sector_show(struct device *dev,
+			   struct device_attribute *attr, char *buf)
+{
+	struct srom_dev *srom = dev_get_drvdata(dev);
+	return sprintf(buf, "%u\n", srom->sector_size);
+}
+
+static ssize_t page_show(struct device *dev,
+			 struct device_attribute *attr, char *buf)
+{
+	struct srom_dev *srom = dev_get_drvdata(dev);
+	return sprintf(buf, "%u\n", srom->page_size);
+}
+
+static struct device_attribute srom_dev_attrs[] = {
+	__ATTR(total_size, S_IRUGO, total_show, NULL),
+	__ATTR(sector_size, S_IRUGO, sector_show, NULL),
+	__ATTR(page_size, S_IRUGO, page_show, NULL),
+	__ATTR_NULL
+};
+
+static char *srom_devnode(struct device *dev, mode_t *mode)
+{
+	*mode = S_IRUGO | S_IWUSR;
+	return kasprintf(GFP_KERNEL, "srom/%s", dev_name(dev));
+}
+
+/*
+ * The fops
+ */
+static const struct file_operations srom_fops = {
+	.owner =     THIS_MODULE,
+	.llseek =    srom_llseek,
+	.read =	     srom_read,
+	.write =     srom_write,
+	.open =	     srom_open,
+	.release =   srom_release,
+};
+
+/**
+ * srom_setup_minor() - Initialize per-minor information.
+ * @srom: Per-device SROM state.
+ * @index: Device to set up.
+ */
+static int srom_setup_minor(struct srom_dev *srom, int index)
+{
+	struct device *dev;
+	int devhdl = srom->hv_devhdl;
+
+	mutex_init(&srom->lock);
+
+	if (_srom_read(devhdl, &srom->total_size,
+		       SROM_TOTAL_SIZE_OFF, sizeof(srom->total_size)) < 0)
+		return -EIO;
+	if (_srom_read(devhdl, &srom->sector_size,
+		       SROM_SECTOR_SIZE_OFF, sizeof(srom->sector_size)) < 0)
+		return -EIO;
+	if (_srom_read(devhdl, &srom->page_size,
+		       SROM_PAGE_SIZE_OFF, sizeof(srom->page_size)) < 0)
+		return -EIO;
+
+	dev = device_create(srom_class, &platform_bus,
+			    MKDEV(srom_major, index), srom, "%d", index);
+	return IS_ERR(dev) ? PTR_ERR(dev) : 0;
+}
+
+/** srom_init() - Initialize the driver's module. */
+static int srom_init(void)
+{
+	int result, i;
+	dev_t dev = MKDEV(srom_major, 0);
+
+	/*
+	 * Start with a plausible number of partitions; the krealloc() call
+	 * below will yield about log(srom_devs) additional allocations.
+	 */
+	srom_devices = kzalloc(4 * sizeof(struct srom_dev), GFP_KERNEL);
+
+	/* Discover the number of srom partitions. */
+	for (i = 0; ; i++) {
+		int devhdl;
+		char buf[20];
+		struct srom_dev *new_srom_devices =
+			krealloc(srom_devices, (i+1) * sizeof(struct srom_dev),
+				 GFP_KERNEL | __GFP_ZERO);
+		if (!new_srom_devices) {
+			result = -ENOMEM;
+			goto fail_mem;
+		}
+		srom_devices = new_srom_devices;
+		sprintf(buf, "srom/0/%d", i);
+		devhdl = hv_dev_open((HV_VirtAddr)buf, 0);
+		if (devhdl < 0) {
+			if (devhdl != HV_ENODEV)
+				pr_notice("srom/%d: hv_dev_open failed: %d.\n",
+					  i, devhdl);
+			break;
+		}
+		srom_devices[i].hv_devhdl = devhdl;
+	}
+	srom_devs = i;
+
+	/* Bail out early if we have no partitions at all. */
+	if (srom_devs == 0) {
+		result = -ENODEV;
+		goto fail_mem;
+	}
+
+	/* Register our major, and accept a dynamic number. */
+	if (srom_major)
+		result = register_chrdev_region(dev, srom_devs, "srom");
+	else {
+		result = alloc_chrdev_region(&dev, 0, srom_devs, "srom");
+		srom_major = MAJOR(dev);
+	}
+	if (result < 0)
+		goto fail_mem;
+
+	/* Register a character device. */
+	cdev_init(&srom_cdev, &srom_fops);
+	srom_cdev.owner = THIS_MODULE;
+	srom_cdev.ops = &srom_fops;
+	result = cdev_add(&srom_cdev, dev, srom_devs);
+	if (result < 0)
+		goto fail_chrdev;
+
+	/* Create a sysfs class. */
+	srom_class = class_create(THIS_MODULE, "srom");
+	if (IS_ERR(srom_class)) {
+		result = PTR_ERR(srom_class);
+		goto fail_cdev;
+	}
+	srom_class->dev_attrs = srom_dev_attrs;
+	srom_class->devnode = srom_devnode;
+
+	/* Do per-partition initialization */
+	for (i = 0; i < srom_devs; i++) {
+		result = srom_setup_minor(srom_devices + i, i);
+		if (result < 0)
+			goto fail_class;
+	}
+
+	return 0;
+
+fail_class:
+	for (i = 0; i < srom_devs; i++)
+		device_destroy(srom_class, MKDEV(srom_major, i));
+	class_destroy(srom_class);
+fail_cdev:
+	cdev_del(&srom_cdev);
+fail_chrdev:
+	unregister_chrdev_region(dev, srom_devs);
+fail_mem:
+	kfree(srom_devices);
+	return result;
+}
+
+/** srom_cleanup() - Clean up the driver's module. */
+static void srom_cleanup(void)
+{
+	int i;
+	for (i = 0; i < srom_devs; i++)
+		device_destroy(srom_class, MKDEV(srom_major, i));
+	class_destroy(srom_class);
+	cdev_del(&srom_cdev);
+	unregister_chrdev_region(MKDEV(srom_major, 0), srom_devs);
+	kfree(srom_devices);
+}
+
+module_init(srom_init);
+module_exit(srom_cleanup);
-- 
1.6.5.2

Re: [PATCH v3] arch/tile: add hypervisor-based character driver for SPI flash ROM

[Arnd Bergmann]

On Thursday 02 June 2011, Chris Metcalf wrote:
> The first version of this patch proposed an arch/tile/drivers/ directory,
> but the consensus was that this was probably a poor choice for a place to
> group Tilera-specific drivers, and that in any case grouping by platform
> was discouraged, and grouping by function was preferred.
> 
> This version of the patch addresses various issues raised in the
> community, primarily the absence of sysfs integration.  The sysfs
> integration now handles passing information on sector size, page size,
> and total partition size to userspace as well.  In addition, we now
> use a single "struct cdev" to manage all the partition minor devices,
> and dynamically discover the correct number of partitions from the
> hypervisor rather than using a module_param with a default value.
> 
> This driver has no particular "peer" drivers it can be grouped with.
> It is sort of like an MTD driver for SPI ROM, but it doesn't group well
> with the other MTD devices since it relies on hypervisor virtualization
> to handle many of the irritating aspects of flash ROM management: sector
> awareness, background read for sub-sector writes, bit examination to
> determine whether a sector erase needs to be issued, etc.  It is in fact
> more like an EEPROM driver, but the hypervisor virtualization does require
> a "flush" command if you wish to commit a sector write prior to writing
> to a different sector, and this is sufficiently different from generic
> I2C/SPI EEPROMs that as a result it doesn't group well with them either.
> 
> The simple character device is already in use by a range of Tilera
> SPI ROM management tools, as well as by customers.  In addition, using
> the simple character device actually simplifies the userspace tools,
> since they don't need to manage sector erase, background read, etc.
> This both simplifies the code (since we can uniformly manage plain files
> and the SPI ROM) as well as makes the user code portable to non-Linux
> platforms that don't offer the same MTD ioctls.
> 
> Signed-off-by: Chris Metcalf <cmetcalf@tilera.com>

Reviewed-by: Arnd Bergmann <arnd@arndb.de>