[PATCH 0/2] x86/quark: Add eSRAM driver and test code

[PATCH 0/2] x86/quark: Add eSRAM driver and test code

[Bryan O'Donoghue]

Quark X1000 SoC contains a 512 KiB embedded SRAM (eSRAM) memory that can
be mapped onto an area of DRAM in block or on per-page overlay mode where a
4 KiB aligned region can be overlayed - allowing for broken up mappings
with a 4 KiB individual granularity.

eSRAM has access times similar to an L1 cache. The following patchset
adds a gen_pool driver and automatic test routine to exercise eSRAM. The
intent of the eSRAM driver is to allow other drivers to allocate SRAM
buffers. In contrast to the original BSP code no attempt will be made to
map kernel .data section code, this is a simple SRAM buffer allocation/free
mechanism and a sanity test to ensure it's ongoing correctness.

Bryan O'Donoghue (2):
  x86/quark: Add Quark embedded SRAM support
  x86/quark: Add Quark embedded SRAM self-test

 arch/x86/Kconfig.debug                         |  13 +
 arch/x86/include/asm/esram.h                   |  66 ++++
 arch/x86/platform/intel-quark/Makefile         |   2 +
 arch/x86/platform/intel-quark/esram.c          | 502 +++++++++++++++++++++++++
 arch/x86/platform/intel-quark/esram_selftest.c | 159 ++++++++
 drivers/platform/x86/Kconfig                   |  17 +-
 6 files changed, 758 insertions(+), 1 deletion(-)
 create mode 100644 arch/x86/include/asm/esram.h
 create mode 100644 arch/x86/platform/intel-quark/esram.c
 create mode 100644 arch/x86/platform/intel-quark/esram_selftest.c

-- 
1.9.1

[PATCH 1/2] x86/quark: Add Quark embedded SRAM support

[Bryan O'Donoghue]

Quark X1000 ships with 512 KiB of embedded SRAM (eSRAM) a low-latency
memory with access times similar to an L1 cache. eSRAM is used during the
initial bootstrap phases of EFI firmware, this driver provides a gen_pool
interface to eSRAM to allow drivers to make use of eSRAM for fast-access
buffers.

eSRAM can be configured in two flavours - block mode or in per-page overlay
mode. Per-page overlay mode is more interesting in that it allows overlay
of any valid RAM address by eSRAM with a granularity of 4 KiB. This driver
overlays a kzalloc() provided contiguous memory region in 4 KiB increments.

On a read-access to an overlayed region of DRAM data will be fetched from
eSRAM as opposed to DRAM - thus mitigating CAS/RAS latencies associated
with DRAM and allowing DRAM to continue in a lower-power state rather than
service the data access directly.

On a cache miss the cacheline fetch will be roughly 20% faster than
fetching from DRAM on average. Once the cacheline has been populated the
processor operates from the L1 cache so no further performance boost will
be observed.

A follow-on patch provides an eSRAM performance test that illustrates the
performance boost for varying sizes of read operation.

Signed-off-by: Bryan O'Donoghue <pure.logic@nexus-software.ie>
---
 arch/x86/include/asm/esram.h           |  66 +++++
 arch/x86/platform/intel-quark/Makefile |   1 +
 arch/x86/platform/intel-quark/esram.c  | 502 +++++++++++++++++++++++++++++++++
 drivers/platform/x86/Kconfig           |  17 +-
 4 files changed, 585 insertions(+), 1 deletion(-)
 create mode 100644 arch/x86/include/asm/esram.h
 create mode 100644 arch/x86/platform/intel-quark/esram.c

diff --git a/arch/x86/include/asm/esram.h b/arch/x86/include/asm/esram.h
new file mode 100644
index 0000000..9932862
--- /dev/null
+++ b/arch/x86/include/asm/esram.h
@@ -0,0 +1,66 @@
+/*
+ * esram.h: Embedded SRAM (eSRAM)
+ *
+ * Copyright(c) 2013 Intel Corporation.
+ * Copyright(c) 2015 Bryan O'Donoghue <pure.logic@nexus-software.ie>
+ *
+ * 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
+ * of the License.
+ *
+ * See 329676_QuarkDatasheet.pdf for register bitmap details.
+ */
+
+#ifndef __ESRAM_H__
+#define __ESRAM_H__
+
+#include <linux/genalloc.h>
+
+/* eSRAM registers */
+#define ESRAMCTRL_REG			0x81
+#define ESRAMPGBLOCK_REG		0x82
+#define ESRAMCERR_REG			0x83
+#define ESRAMUCERR_REG			0x84
+#define ESRAMSDROM_REG			0x88
+
+/* eSRAM Control - Offset 0x81 - Section 12.7.4.37 */
+#define ESRAMCTRL_SIZE(x)		(PAGE_SIZE * (((x >> 16) & 0x7F) + 1))
+#define ESRAMCTRL_ECCTHRESH(x)		((x >> 8) & 0xFF)
+#define ESRAMCTRL_THRESHMSG_EN		BIT(7)
+#define ESRAMCTRL_AVAILABLE		BIT(4)
+#define ESRAMCTRL_ENABLE_ALL		BIT(3)
+#define ESRAMCTRL_GLOBAL_CSR_LOCK	BIT(2)
+#define ESRAMCTRL_SECDEC_EN		BIT(0)
+
+/* eSRAM Page Block Control - Offset 0x82 - Section 12.7.4.38 */
+#define ESRAMPGBLOCK_FLUSH_EN		BIT(31)
+#define ESRAMPGBLOCK_DIS		BIT(29)
+#define ESRAMPGBLOCK_EN			BIT(28)
+#define ESRAMPGBLOCK_CSR_LOCK		BIT(27)
+#define ESRAMPGBLOCK_INIT		BIT(26)
+#define ESRAMPGBLOCK_BUSY		BIT(24)
+#define ESRAMPGBLOCK_BASE(x)		((x & 0xFF) << 24)
+
+/* eSRAM Correctable Error - Offset 0x83 - Section 12.7.4.39 */
+#define ESRAMCERR_ERR_CNT_RST		BIT(25)
+#define ESRAMCERR_ERR_CNT(x)		((x >> 17) & 0xFF)
+#define ESRAMCERR_ERR_PG_DW_OFFSET(x)	((x >> 9) & 0x7F)
+#define ESRAMCERR_ERR_PG_NUM(x)		(x & 0xFF)
+
+/* eSRAM Uncorrectable Error - Offset 0x84 - Section 12.7.4.40 */
+#define ESRAMUCERR_ERR_CNT(x)		((x >> 17) & 0xFF)
+#define ESRAMUCERR_ERR_PG_DW_OFFSET(x)	((x >> 9) & 0x7F)
+#define ESRAMUCERR_ERR_PG_NUM(x)	(x & 0xFF)
+
+/* eSRAM Page Control - Offsets 0-127 - Section 12.7.5.1 */
+#define ESRAMPGCTRL_FLUSH_PAGE_EN	BIT(31)
+#define ESRAMPGCTRL_EN			BIT(28)
+#define ESRAMPGCTRL_LOCK		BIT(27)
+#define ESRAMPGCTRL_INIT_IN_PROG	BIT(26)
+#define ESRAMPGCTRL_BUSY		BIT(24)
+
+struct gen_pool *esram_get_genpool(void);
+
+#endif /* __ESRAM_H__ */
+
diff --git a/arch/x86/platform/intel-quark/Makefile b/arch/x86/platform/intel-quark/Makefile
index 9cc57ed..94adb0b 100644
--- a/arch/x86/platform/intel-quark/Makefile
+++ b/arch/x86/platform/intel-quark/Makefile
@@ -1,2 +1,3 @@
 obj-$(CONFIG_INTEL_IMR) += imr.o
+obj-$(CONFIG_INTEL_ESRAM) += esram.o
 obj-$(CONFIG_DEBUG_IMR_SELFTEST) += imr_selftest.o
diff --git a/arch/x86/platform/intel-quark/esram.c b/arch/x86/platform/intel-quark/esram.c
new file mode 100644
index 0000000..51390c3
--- /dev/null
+++ b/arch/x86/platform/intel-quark/esram.c
@@ -0,0 +1,502 @@
+/*
+ * Copyright(c) 2013 Intel Corporation.
+ * Copyright(c) 2015 Bryan O'Donoghue <pure.logic@nexus-software.ie>
+ *
+ * Embedded SRAM (eSRAM) is an on-die low-latency SRAM that can operate in
+ * 512 KiB block mode or in 4 KiB page over-lay mode. eSRAM provides a
+ * low-latency memory with similar access times to an L1 cache.
+ *
+ * eSRAM supports one-time-programming of an overlayed 4 KiB aligned and 4
+ * KiB sized memory region.
+ *
+ * To populate eSRAM we must copy data to a temporary buffer, overlay and
+ * then copy data back to the eSRAM region.
+ *
+ * When entering S3 - we must save eSRAM state to DRAM, the RMU takes
+ * responsibility for this.
+ * When transitioning back to S0 Linux needs restore eSRAM overlay contents
+ * back to the original state - the RMU will not handle this.
+ *
+ * See quark-x1000-datasheet.pdf for register definitions.
+ * http://www.intel.com/content/dam/www/public/us/en/documents/datasheets/quark-x1000-datasheet.pdf
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <asm/cpu_device_id.h>
+#include <asm/desc.h>
+#include <asm/esram.h>
+#include <asm/io.h>
+#include <asm/iosf_mbi.h>
+#include <asm/pgtable.h>
+#include <asm/special_insns.h>
+#include <linux/debugfs.h>
+#include <linux/err.h>
+#include <linux/fs.h>
+#include <linux/genalloc.h>
+#include <linux/list.h>
+#include <linux/mm.h>
+#include <linux/module.h>
+#include <linux/printk.h>
+#include <linux/platform_device.h>
+#include <linux/pm.h>
+#include <linux/seq_file.h>
+#include <linux/slab.h>
+#include <linux/syscore_ops.h>
+#include <linux/timer.h>
+
+#define esram_to_phys(x)	((x) << PAGE_SHIFT)
+#define phys_to_esram(x)	((x) >> PAGE_SHIFT)
+
+/**
+ * struct esram_page
+ *
+ * Represents an eSRAM page.
+ */
+struct esram_page {
+	u32 id;
+	struct list_head list;
+	phys_addr_t addr;
+};
+
+/**
+ * struct esram_dev
+ *
+ * Structre to represent module state/data/etc.
+ */
+struct esram_dev {
+	struct dentry		*dbg;
+	void			*overlay;
+	struct esram_page	*pages;
+	struct gen_pool		*pool;
+	u8			cbuf[PAGE_SIZE];
+	bool			init;
+	struct mutex		lock;
+	u32			num_bytes;
+	struct list_head	page_list;
+	u32			total_pages;
+};
+
+static struct esram_dev esram_dev;
+
+/**
+ * esram_dbgfs_state_show - print state of eSRAM registers.
+ *
+ * @s:		pointer to seq_file for output.
+ * @unused:	unused parameter.
+ * @return:	0 on success or error code passed from mbi_iosf on failure.
+ */
+static int esram_dbgfs_state_show(struct seq_file *s, void *unused)
+{
+	struct esram_dev *edev = &esram_dev;
+	u32 data;
+	u32 reg = (u32)s->private;
+	int ret;
+
+	mutex_lock(&edev->lock);
+	ret = iosf_mbi_read(QRK_MBI_UNIT_MM, QRK_MBI_MM_READ, reg, &data);
+	if (ret == 0)
+		seq_printf(s, "0x%08x\n", data);
+	mutex_unlock(&edev->lock);
+	return ret;
+}
+
+/**
+ * esram_state_open - debugfs open callback.
+ *
+ * @inode:	pointer to struct inode.
+ * @file:	pointer to struct file.
+ * @return:	result of single open.
+ */
+static int esram_state_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, esram_dbgfs_state_show, inode->i_private);
+}
+
+static const struct file_operations esram_dbg_ops = {
+	.open		= esram_state_open,
+	.read		= seq_read,
+	.llseek		= seq_lseek,
+	.release	= single_release,
+};
+
+/**
+ * esram_debugfs_register - register debugfs hooks.
+ *
+ * @edev:	pointer to esram_device structure.
+ * @return:	0 on success - errno on failure.
+ */
+static int esram_debugfs_register(struct esram_dev *edev)
+{
+	struct dentry *dret;
+
+	edev->dbg = debugfs_create_dir("esram", NULL);
+	if (IS_ERR_OR_NULL(edev->dbg))
+		goto err;
+
+	dret = debugfs_create_file("ctrl", S_IRUGO, edev->dbg,
+				   (void *)ESRAMCTRL_REG, &esram_dbg_ops);
+	if (IS_ERR_OR_NULL(dret))
+		goto err;
+
+	dret = debugfs_create_file("pgblock", S_IRUGO, edev->dbg,
+				   (void *)ESRAMPGBLOCK_REG, &esram_dbg_ops);
+	if (IS_ERR_OR_NULL(dret))
+		goto err;
+
+	dret = debugfs_create_file("cerr", S_IRUGO, edev->dbg,
+				   (void *)ESRAMCERR_REG, &esram_dbg_ops);
+	if (IS_ERR_OR_NULL(dret))
+		goto err;
+
+	dret = debugfs_create_file("ucerr", S_IRUGO, edev->dbg,
+				   (void *)ESRAMUCERR_REG, &esram_dbg_ops);
+	if (IS_ERR_OR_NULL(dret))
+		goto err;
+
+	dret = debugfs_create_file("sdrome", S_IRUGO, edev->dbg,
+				   (void *)ESRAMSDROM_REG, &esram_dbg_ops);
+	if (IS_ERR_OR_NULL(dret))
+		goto err;
+
+	return 0;
+err:
+	if (!IS_ERR_OR_NULL(edev->dbg))
+		debugfs_remove_recursive(edev->dbg);
+	return -1;
+}
+
+/**
+ * esram_debugfs_unregister - unregister debugfs hooks.
+ *
+ * @edev:	pointer to esram_device structure.
+ * @return:
+ */
+static void esram_debugfs_unregister(struct esram_dev *edev)
+{
+	if (!IS_ERR_OR_NULL(edev->dbg))
+		debugfs_remove_recursive(edev->dbg);
+}
+
+/**
+ * esram_page_busy - Determine if an eSRAM page is busy.
+ *
+ * @param ep:	Pointer to the page descriptor.
+ * @return:		int indicating whether or not a page is enabled.
+ */
+static inline int esram_page_busy(struct esram_page *ep)
+{
+	u32 reg = 0;
+	int ret;
+
+	ret = iosf_mbi_read(QRK_MBI_UNIT_MM, QRK_MBI_MMESRAM_READ, ep->id, &reg);
+	if (ret)
+		return ret;
+	return (reg & ESRAMPGCTRL_BUSY);
+}
+
+/**
+ * esram_dump_fault - dump eSRAM registers and BUG().
+ *
+ * @return:
+ */
+static void esram_dump_fault(struct esram_page *ep)
+{
+	u32 pgc;
+	u32 pgd;
+	u32 pgb;
+
+	/* Show the page state. */
+	iosf_mbi_read(QRK_MBI_UNIT_MM, QRK_MBI_MMESRAM_READ, ep->id, &pgd);
+	pr_err("fault @ page %d state 0x%08x\n", ep->id, pgd);
+
+	/* Get state. */
+	iosf_mbi_read(QRK_MBI_UNIT_MM, QRK_MBI_MM_READ, ESRAMCTRL_REG, &pgc);
+	iosf_mbi_read(QRK_MBI_UNIT_MM, QRK_MBI_MM_READ, ESRAMPGBLOCK_REG, &pgb);
+	pr_err("page-control=0x%08x, page-block=0x%08x\n", pgc, pgb);
+
+	BUG();
+}
+
+/**
+ * esram_page_enable - Enable an eSRAM page spinning for page to become ready.
+ *
+ * @param ep: struct esram_page carries data to program to register.
+ * @return	zero on success < 0 on error.
+ */
+static int esram_page_enable(struct esram_page *ep)
+{
+	int ret = 0;
+
+	/* Enable a busy page => EINVAL, return IOSF error as necessary. */
+	ret = esram_page_busy(ep);
+	if (ret)
+		return ret < 0 ? ret : -EINVAL;
+
+	/* Enable page overlay - with automatic flush on S3 entry. */
+	ret = iosf_mbi_write(QRK_MBI_UNIT_MM, QRK_MBI_MMESRAM_WRITE, ep->id,
+			     ESRAMPGCTRL_FLUSH_PAGE_EN | ESRAMPGCTRL_EN |
+			     phys_to_esram(ep->addr));
+	if (ret)
+		return ret;
+
+	/* Busy bit true is good, ret < 0 means IOSF read error. */
+	ret = esram_page_busy(ep);
+	if (ret)
+		ret = 0;
+
+	return ret;
+}
+
+/**
+ * esram_page_overlay - Overlay a page with fast access eSRAM.
+ *
+ * This function takes a 4 KiB aligned physical address and programs an
+ * eSRAM page to overlay that 4 KiB region. We require and verify that the
+ * target memory is read-write - since we don't support overlay of read-only
+ * memory regions - such as kernel .text areas. Overlay of .text areas is
+ * not supported because eSRAM isn't self-populating and we cannot guarantee
+ * atomicity of the overlay operation. It is assumed and required that the
+ * caller of the overlay function is overlaying a data buffer not kernel
+ * code.
+ *
+ * @param ep:	Pointer to eSRAM page desciptor.
+ * @return:		0 on success < 0 on failure.
+ */
+static int esram_page_overlay(struct esram_dev *edev, struct esram_page *ep)
+{
+	int level = 0;
+	void *vaddr = __va(ep->addr);
+	pte_t *pte = lookup_address((unsigned long)vaddr, &level);
+	int ret;
+
+	/* We only support overlay for r/w memory. */
+	if (pte == NULL || !(pte_write(*pte))) {
+		pr_err("invalid address for overlay %pa\n", &ep->addr);
+		return -ENOMEM;
+	}
+
+	/* eSRAM does not autopopulate so save the contents. */
+	memcpy(&edev->cbuf, vaddr, PAGE_SIZE);
+	ret = esram_page_enable(ep);
+	if (ret) {
+		esram_dump_fault(ep);
+		goto err;
+	}
+
+	/* Overlay complete, repopulate the eSRAM page with original data. */
+	memcpy((void *)vaddr, &esram_dev.cbuf,  PAGE_SIZE);
+err:
+	return ret;
+}
+
+/**
+ * esram_map_page - Overlay a vritual address range aligned to 4 KiB.
+ *
+ * @param page:		Page to map.
+ * @return:		0 success < 0 failure.
+ */
+static int esram_map_page(struct esram_dev *edev, struct esram_page *ep)
+{
+	int ret = 0;
+
+	mutex_lock(&edev->lock);
+	ret = esram_page_overlay(edev, ep);
+	if (ret)
+		goto err;
+	list_add(&ep->list, &edev->page_list);
+err:
+	mutex_unlock(&edev->lock);
+	return ret;
+}
+
+/**
+ * esram_resume - restore eSRAM overlays on S3=>S0 transition.
+ *
+ * @return:
+ */
+static void esram_resume(void)
+{
+	struct esram_dev *edev = &esram_dev;
+	struct esram_page *ep = NULL;
+
+	mutex_lock(&edev->lock);
+	list_for_each_entry(ep, &edev->page_list, list)
+		if (esram_page_overlay(edev, ep))
+			pr_err("restore page %d phys %pa fail!\n",
+				ep->id, &ep->addr);
+	mutex_unlock(&edev->lock);
+}
+
+/* Shutdown is done by RMU. Kernel needs to-do the resume() though. */
+static struct syscore_ops esram_syscore_ops = {
+	.resume		= esram_resume,
+};
+
+/**
+ * esram_get_genpool - return pointer to esram genpool structure.
+ *
+ * @return:
+ */
+struct gen_pool *esram_get_genpool(void)
+{
+	struct esram_dev *edev = &esram_dev;
+
+	return edev->init ? edev->pool : NULL;
+}
+EXPORT_SYMBOL_GPL(esram_get_genpool);
+
+static const struct x86_cpu_id esram_ids[] __initconst = {
+	{ X86_VENDOR_INTEL, 5, 9 },	/* Intel Quark SoC X1000. */
+	{}
+};
+MODULE_DEVICE_TABLE(x86cpu, esram_ids);
+
+ /**
+ * esram_init - entry point for eSRAM driver.
+ *
+ * This driver manages eSRAM on a per-page basis. Therefore if we find block
+ * mode is enabled, or any global, block-level or page-level locks are in place
+ * at module initialisation time - we bail out.
+ *
+ * return: -ENODEV for no eSRAM support 0 if good to go.
+ */
+static int __init esram_init(void)
+{
+	u32 block;
+	u32 ctrl;
+	struct esram_page *ep = NULL;
+	struct esram_dev *edev = &esram_dev;
+	phys_addr_t addr;
+	int i;
+	int ret;
+
+	if (!x86_match_cpu(esram_ids) || !iosf_mbi_available())
+		return -ENODEV;
+
+	memset(edev, 0x00, sizeof(esram_dev));
+	INIT_LIST_HEAD(&edev->page_list);
+	mutex_init(&edev->lock);
+
+	/* Ensure block mode disabled. */
+	block = ESRAMPGBLOCK_DIS;
+	ret = iosf_mbi_write(QRK_MBI_UNIT_MM, QRK_MBI_MM_WRITE, ESRAMPGBLOCK_REG, block);
+	if (ret)
+		return ret;
+
+	/* Get global control and block status. */
+	ret = iosf_mbi_read(QRK_MBI_UNIT_MM, QRK_MBI_MM_READ, ESRAMCTRL_REG, &ctrl);
+	if (ret)
+		return ret;
+	ret = iosf_mbi_read(QRK_MBI_UNIT_MM, QRK_MBI_MM_READ, ESRAMPGBLOCK_REG, &block);
+	if (ret)
+		return ret;
+
+	/* Ensure no global lock exists. */
+	if (ctrl & ESRAMCTRL_GLOBAL_CSR_LOCK)
+		return -ENODEV;
+
+	if (block & (ESRAMPGBLOCK_CSR_LOCK | ESRAMPGBLOCK_EN))
+		return -ENODEV;
+
+	/* Calculate # of pages silicon supports. */
+	edev->num_bytes = ESRAMCTRL_SIZE(ctrl);
+	edev->total_pages = edev->num_bytes / PAGE_SIZE;
+	if (edev->total_pages == 0)
+		return -ENOMEM;
+
+	/* Get an array of esram pages. */
+	edev->pages = kzalloc(edev->total_pages *
+		sizeof(struct esram_page), GFP_KERNEL);
+	if (IS_ERR(edev->pages)) {
+		ret = PTR_ERR(edev->pages);
+		goto err;
+	}
+
+	/* Make an area for the gen_pool to operate from. */
+	edev->overlay = kmalloc(edev->num_bytes, GFP_KERNEL);
+	if (IS_ERR(edev->overlay)) {
+		ret = PTR_ERR(edev->overlay);
+		goto err;
+	}
+	edev->pool = gen_pool_create(ilog2(PAGE_SIZE), -1);
+	if (!edev->pool) {
+		ret = -ENOMEM;
+		goto err;
+	}
+	ret = gen_pool_add_virt(edev->pool, (unsigned long)edev->overlay,
+				__pa(edev->overlay), edev->num_bytes, -1);
+	if (ret)
+		goto err;
+
+	/* Overlay contiguous region with eSRAM pages. */
+	addr = __pa(edev->overlay);
+	for (i = 0; i < edev->total_pages; i++) {
+		ep = &edev->pages[i];
+		ep->id = i;
+		ep->addr = addr;
+
+		/* Validate page state is not busy. */
+		ret = esram_page_busy(ep);
+		if (ret) {
+			esram_dump_fault(ep);
+			ret = ret < 0 ? ret : -ENOMEM;
+			goto err;
+		}
+
+		/* Overlay. */
+		ret = esram_map_page(edev, ep);
+		if (ret)
+			goto err;
+		addr += PAGE_SIZE;
+	}
+
+	register_syscore_ops(&esram_syscore_ops);
+	ret = esram_debugfs_register(edev);
+	if (ret != 0)
+		pr_warn("debugfs register failed!\n");
+	edev->init = true;
+
+	pr_info("overlay mode with %d pages - OK\n", edev->total_pages);
+	return 0;
+err:
+	if (edev->pool != NULL)
+		gen_pool_destroy(edev->pool);
+
+	if (!IS_ERR(edev->pages))
+		kfree(edev->pages);
+
+	return ret;
+}
+
+/**
+ * esram_exit - exit point for eSRAM code.
+ *
+ * Deregisters debugfs, leave eSRAM state as-is.
+ *
+ * return:
+ */
+static void __exit esram_exit(void)
+{
+	struct esram_dev *edev = &esram_dev;
+
+	if (edev->pool != NULL) {
+		if (gen_pool_avail(edev->pool) < gen_pool_size(edev->pool))
+			pr_err("removing in-use eSRAM gen_pool!\n");
+		gen_pool_destroy(edev->pool);
+	}
+
+	if (!IS_ERR(edev->pages))
+		kfree(edev->pages);
+
+	esram_debugfs_unregister(&esram_dev);
+	unregister_syscore_ops(&esram_syscore_ops);
+}
+
+module_init(esram_init);
+module_exit(esram_exit);
+
+MODULE_AUTHOR("Bryan O'Donoghue <pure.logic@nexus-software.ie>");
+MODULE_DESCRIPTION("Intel Embedded SRAM overlay driver");
+MODULE_LICENSE("Dual BSD/GPL");
+
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index f9f205c..42b7b88 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -737,7 +737,7 @@ config INTEL_IPS
 	  supported platforms.
 
 config INTEL_IMR
-	bool "Intel Isolated Memory Region support"
+	tristate "Intel Isolated Memory Region support"
 	default n
 	depends on X86_INTEL_QUARK && IOSF_MBI
 	---help---
@@ -761,6 +761,21 @@ config INTEL_IMR
 
 	  If you are running on a Galileo/Quark say Y here.
 
+config INTEL_ESRAM
+	bool "Intel Embedded SRAM (eSRAM) support"
+	default n
+	depends on X86_INTEL_QUARK && IOSF_MBI
+	select GENERIC_ALLOCATOR
+	---help---
+	  This options provides an API to allocate memory from Embedded SRAM
+	  (eSRAM) present on Quark X1000 SoC processors.
+	  eSRAM is a 512 KiB block of low-latency SRAM organized as
+	  128 * 4 KiB pages or as one 512 KiB chunk of memory. This driver
+	  enables eSRAM in per-page overlay mode and provides a gen_pool
+	  allocator which allows allocation of memory from the eSRAM pool.
+
+	  If you are running on a Galileo/Quark say Y here.
+
 config IBM_RTL
 	tristate "Device driver to enable PRTL support"
 	depends on X86 && PCI
-- 
1.9.1

[PATCH 2/2] x86/quark: Add Quark embedded SRAM self-test

[Bryan O'Donoghue]

Quark X1000 contains an embedded SRAM (eSRAM) a fast access SRAM. This code
is a self-test routine to measure the performance of that SRAM for
different read-values. The tests are designed the performance gains
provided by eSRAM using different read-sizes and comparing the performance
of eSRAM overlayed DRAM to raw DRAM.

Signed-off-by: Bryan O'Donoghue <pure.logic@nexus-software.ie>
---
 arch/x86/Kconfig.debug                         |  13 ++
 arch/x86/platform/intel-quark/Makefile         |   1 +
 arch/x86/platform/intel-quark/esram_selftest.c | 159 +++++++++++++++++++++++++
 3 files changed, 173 insertions(+)
 create mode 100644 arch/x86/platform/intel-quark/esram_selftest.c

diff --git a/arch/x86/Kconfig.debug b/arch/x86/Kconfig.debug
index 72484a6..bb62174 100644
--- a/arch/x86/Kconfig.debug
+++ b/arch/x86/Kconfig.debug
@@ -322,6 +322,19 @@ config DEBUG_IMR_SELFTEST
 
 	  If unsure say N here.
 
+config DEBUG_ESRAM_SELFTEST
+	bool "Embedded SRAM self test"
+	default n
+	depends on INTEL_ESRAM
+	---help---
+	  This option enables automated sanity testing of the eSRAM driver
+	  on Quark X1000. A simple set of tests with performance metrics
+	  measured a DRAM baseline are run. These tests show the measured
+	  performance increase across a given memory size for a series of
+	  incrementing read sizes.
+
+	  If unsure say N here.
+
 config X86_DEBUG_STATIC_CPU_HAS
 	bool "Debug alternatives"
 	depends on DEBUG_KERNEL
diff --git a/arch/x86/platform/intel-quark/Makefile b/arch/x86/platform/intel-quark/Makefile
index 94adb0b..dc466be 100644
--- a/arch/x86/platform/intel-quark/Makefile
+++ b/arch/x86/platform/intel-quark/Makefile
@@ -1,3 +1,4 @@
 obj-$(CONFIG_INTEL_IMR) += imr.o
 obj-$(CONFIG_INTEL_ESRAM) += esram.o
 obj-$(CONFIG_DEBUG_IMR_SELFTEST) += imr_selftest.o
+obj-$(CONFIG_DEBUG_ESRAM_SELFTEST) += esram_selftest.o
diff --git a/arch/x86/platform/intel-quark/esram_selftest.c b/arch/x86/platform/intel-quark/esram_selftest.c
new file mode 100644
index 0000000..7849dac
--- /dev/null
+++ b/arch/x86/platform/intel-quark/esram_selftest.c
@@ -0,0 +1,159 @@
+/**
+ * esram_selftest.c
+ *
+ * Copyright(c) 2015 Bryan O'Donoghue <pure.logic@nexus-software.ie>
+ *
+ * IMR self test. The purpose of this module is to run a set of tests on the
+ * IMR API to validate it's sanity. We check for overlapping, reserved
+ * addresses and setup/teardown sanity.
+ *
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <asm/cpu_device_id.h>
+#include <asm/esram.h>
+#include <linux/init.h>
+#include <linux/mm.h>
+#include <linux/module.h>
+#include <linux/sizes.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+
+/**
+ * esram_self_test_time
+ *
+ * This function is carefully constructed to measure and verify the
+ * performance boost provided by eSRAM. We invalidate the cache with a
+ * wbinvd() and then perform a series of reads - each of which will cause a
+ * cacheline miss. We measure the aggregate time it takes to complete the
+ * series of reads and return the delta in cycles. The calling function will
+ * pass either a pointer to DRAM or a pointer to eSRAM.
+ *
+ * @param walker:	Pointer to RAM area to test.
+ * @return:		Number of cycles to complete test.
+ */
+static cycles_t esram_self_test_time(char *walker, ssize_t step, ssize_t size)
+{
+	volatile u32 dummy = 0;
+	int i;
+	int j;
+	cycle_t t1;
+	cycle_t t2;
+	u32 page_count = size / PAGE_SIZE;
+
+	local_irq_disable();
+	t1 = get_cycles();
+	for (i = 0; i < page_count; i++) {
+		for (j = 0; j < PAGE_SIZE/step; j++) {
+			dummy += *(u32 *)walker;
+			walker += step;
+		}
+	}
+	t2 = get_cycles();
+	local_irq_enable();
+
+	return t2 > t1 ? t2 - t1 : ULLONG_MAX - t2 + t1;
+}
+
+/**
+ * __get_percent - the the percentage that min is of max.
+ *
+ * @max:	The larger value.
+ * @min:	The smaller value.
+ * @return:	The % of the max that the min is.
+*/
+static inline u32 __get_percent(u32 max, u32 min)
+{
+	max = max/100UL;
+	return 100UL - (min/max);
+}
+
+/**
+ * esram_self_test
+ *
+ * Verify eSRAM self_test with some simple tests to verify overlap,
+ * zero sized allocations and 1 KiB sized areas.
+ *
+ * return:
+ */
+static void __init esram_self_test(void)
+{
+	bool pass;
+	size_t size = SZ_512K;
+	cycles_t t1;
+	cycles_t t2;
+	u32 percent;
+	char *dram_mem;
+	unsigned long esram_mem;
+	int i;
+	struct gen_pool *pool = esram_get_genpool();
+	size_t steps[] = { 8, 16, 32, 64, 128 };
+
+	if (pool == NULL) {
+		pr_err("unable to get esram_genpool pointer!\n");
+		return;
+	}
+
+	esram_mem = gen_pool_alloc(pool, size);
+	if (esram_mem == 0) {
+		pr_err("gen_pool_alloc of %d KiB fail!\n", size);
+		return;
+	}
+	dram_mem = kzalloc(GFP_KERNEL, size);
+	if (dram_mem == NULL)
+		goto err;
+
+	/* Cycle through a series of tests, measurng cycle times as we go. */
+	for (i = 0; i < sizeof(steps)/sizeof(size_t); i++) {
+		t1 = esram_self_test_time(dram_mem, steps[i], size);
+		t2 = esram_self_test_time((char *)esram_mem, steps[i], size);
+		pass = t2 < t1;
+		if (pass)
+			percent = __get_percent(t1, t2);
+		else
+			percent = __get_percent(t2, t1);
+		pr_info("%s, %d%% %s DRAM:%10llu cycles, eSRAM:%10llu cycles, step-size:%3d\n",
+			pass ? "pass" : "fail", percent, pass ? "better" : "worse",
+			t1, t2, steps[i]);
+	}
+err:
+	if (dram_mem != NULL)
+		kfree(dram_mem);
+	if (esram_mem != 0)
+		gen_pool_free(pool, esram_mem, size);
+}
+
+static const struct x86_cpu_id esram_ids[] __initconst = {
+	{ X86_VENDOR_INTEL, 5, 9 },	/* Intel Quark SoC X1000. */
+	{}
+};
+MODULE_DEVICE_TABLE(x86cpu, esram_ids);
+
+/**
+ * esram_self_test_init - entry point for IMR driver.
+ *
+ * return: -ENODEV for no IMR support 0 if good to go.
+ */
+static int __init esram_self_test_init(void)
+{
+	if (x86_match_cpu(esram_ids))
+		esram_self_test();
+	return 0;
+}
+
+/**
+ * esram_self_test_exit - exit point for IMR code.
+ *
+ * return:
+ */
+static void __exit esram_self_test_exit(void)
+{
+}
+
+module_init(esram_self_test_init);
+module_exit(esram_self_test_exit);
+
+MODULE_AUTHOR("Bryan O'Donoghue <pure.logic@nexus-software.ie>");
+MODULE_DESCRIPTION("Intel Quark eSRAM self-test driver");
+MODULE_LICENSE("Dual BSD/GPL");
-- 
1.9.1

Re: [PATCH 1/2] x86/quark: Add Quark embedded SRAM support

[Thomas Gleixner]

On Mon, 4 May 2015, Bryan O'Donoghue wrote:
> +++ b/arch/x86/include/asm/esram.h

This should be in platform/quark/....

> +++ b/arch/x86/platform/intel-quark/esram.c

> +#define esram_to_phys(x)	((x) << PAGE_SHIFT)

Unused macro.

> +#define phys_to_esram(x)	((x) >> PAGE_SHIFT)

There is a single usage size for this lousy documented magic.

> +/**
> + * struct esram_page
> + *
> + * Represents an eSRAM page.
> + */
> +struct esram_page {
> +	u32 id;
> +	struct list_head list;
> +	phys_addr_t addr;

Please tab align the struct member names as you did below.

> +};
> +
> +/**
> + * struct esram_dev
> + *
> + * Structre to represent module state/data/etc.
> + */
> +struct esram_dev {
> +	struct dentry		*dbg;

So dbgfs is a hard requirement for this to work?

> +	void			*overlay;
> +	struct esram_page	*pages;
> +	struct gen_pool		*pool;
> +	u8			cbuf[PAGE_SIZE];
> +	bool			init;
> +	struct mutex		lock;
> +	u32			num_bytes;
> +	struct list_head	page_list;
> +	u32			total_pages;

Lots of magic fields ....

> +};
> +
> +static struct esram_dev esram_dev;
> +
> +/**
> + * esram_dbgfs_state_show - print state of eSRAM registers.
> + *
> + * @s:		pointer to seq_file for output.
> + * @unused:	unused parameter.
> + * @return:	0 on success or error code passed from mbi_iosf on failure.

I don't think kerneldoc is happy about that one, but I might be wrong

> + */
> +static int esram_dbgfs_state_show(struct seq_file *s, void *unused)
> +{
> +	struct esram_dev *edev = &esram_dev;
> +	u32 data;
> +	u32 reg = (u32)s->private;

You really like to waste lines. What's wrong with:

    	u32 data, reg = .....

> +	int ret;
> +
> +	mutex_lock(&edev->lock);
> +	ret = iosf_mbi_read(QRK_MBI_UNIT_MM, QRK_MBI_MM_READ, reg, &data);
> +	if (ret == 0)

  	if (!ret)

> +		seq_printf(s, "0x%08x\n", data);



> +/**
> + * esram_dump_fault - dump eSRAM registers and BUG().
> + *
> + * @return:

Sigh. Please generate kernel docs from your file to catch all those
function comment failures.

> + */
> +static void esram_dump_fault(struct esram_page *ep)
> +{
> +	u32 pgc;
> +	u32 pgd;
> +	u32 pgb;

One line please for all of these.

> +
> +	/* Show the page state. */
> +	iosf_mbi_read(QRK_MBI_UNIT_MM, QRK_MBI_MMESRAM_READ, ep->id, &pgd);
> +	pr_err("fault @ page %d state 0x%08x\n", ep->id, pgd);
> +
> +	/* Get state. */
> +	iosf_mbi_read(QRK_MBI_UNIT_MM, QRK_MBI_MM_READ, ESRAMCTRL_REG, &pgc);
> +	iosf_mbi_read(QRK_MBI_UNIT_MM, QRK_MBI_MM_READ, ESRAMPGBLOCK_REG, &pgb);
> +	pr_err("page-control=0x%08x, page-block=0x%08x\n", pgc, pgb);
> +
> +	BUG();

So we force BUG() here. Why?

> +}
> +
> +/**
> + * esram_page_enable - Enable an eSRAM page spinning for page to become ready.
> + *
> + * @param ep: struct esram_page carries data to program to register.
> + * @return	zero on success < 0 on error.
> + */
> +static int esram_page_enable(struct esram_page *ep)
> +{
> +	int ret = 0;
> +
> +	/* Enable a busy page => EINVAL, return IOSF error as necessary. */

Why is EINVAL a good return code if the page is busy?

> +	ret = esram_page_busy(ep);
> +	if (ret)
> +		return ret < 0 ? ret : -EINVAL;
> +
> +	/* Enable page overlay - with automatic flush on S3 entry. */
> +	ret = iosf_mbi_write(QRK_MBI_UNIT_MM, QRK_MBI_MMESRAM_WRITE, ep->id,
> +			     ESRAMPGCTRL_FLUSH_PAGE_EN | ESRAMPGCTRL_EN |
> +			     phys_to_esram(ep->addr));
> +	if (ret)
> +		return ret;
> +
> +	/* Busy bit true is good, ret < 0 means IOSF read error. */
> +	ret = esram_page_busy(ep);
> +	if (ret)
> +		ret = 0;
> +
> +	return ret;

Why not just return 0 unconitionally?

> +/**
> + * esram_page_overlay - Overlay a page with fast access eSRAM.
> + *
> + * This function takes a 4 KiB aligned physical address and programs an
> + * eSRAM page to overlay that 4 KiB region. We require and verify that the
> + * target memory is read-write - since we don't support overlay of read-only
> + * memory regions - such as kernel .text areas. Overlay of .text areas is
> + * not supported because eSRAM isn't self-populating and we cannot guarantee
> + * atomicity of the overlay operation. It is assumed and required that the
> + * caller of the overlay function is overlaying a data buffer not kernel
> + * code.
> + *
> + * @param ep:	Pointer to eSRAM page desciptor.
> + * @return:		0 on success < 0 on failure.
> + */
> +static int esram_page_overlay(struct esram_dev *edev, struct esram_page *ep)
> +{
> +	int level = 0;
> +	void *vaddr = __va(ep->addr);
> +	pte_t *pte = lookup_address((unsigned long)vaddr, &level);
> +	int ret;
> +
> +	/* We only support overlay for r/w memory. */

Well, the check below is not really checking for a valid memory
mapping. You can have a r/w PTE for some peripheral device space as
well.

> +	if (pte == NULL || !(pte_write(*pte))) {
> +		pr_err("invalid address for overlay %pa\n", &ep->addr);
> +		return -ENOMEM;
> +	}

Also what makes sure that the mapping is not going away under you?

Nothing, but the whole thing is not required at all. Because you map a
kernel buffer from init(), so these half baken sanity checks are
really useless.

> +
> +	/* eSRAM does not autopopulate so save the contents. */
> +	memcpy(&edev->cbuf, vaddr, PAGE_SIZE);
> +	ret = esram_page_enable(ep);
> +	if (ret) {
> +		esram_dump_fault(ep);
> +		goto err;

  		return ret perhaps?

> +	}
> +
> +	/* Overlay complete, repopulate the eSRAM page with original data. */
> +	memcpy((void *)vaddr, &esram_dev.cbuf,  PAGE_SIZE);

So the caller must ensure that the DRAM content cannot change between
the two memcpys, right? Otherwise you end up with inconsistent data.

At init() time I can see how that works, on resume() rather not.

> +err:
> +	return ret;
> +}
> +
> +/**
> + * esram_map_page - Overlay a vritual address range aligned to 4 KiB.
> + *
> + * @param page:		Page to map.
> + * @return:		0 success < 0 failure.
> + */
> +static int esram_map_page(struct esram_dev *edev, struct esram_page *ep)
> +{
> +	int ret = 0;
> +
> +	mutex_lock(&edev->lock);
> +	ret = esram_page_overlay(edev, ep);
> +	if (ret)
> +		goto err;

Useless goto to further obfuscate the code.

> +	list_add(&ep->list, &edev->page_list);
> +err:
> +	mutex_unlock(&edev->lock);
> +	return ret;
> +}
> +
> +/**
> + * esram_resume - restore eSRAM overlays on S3=>S0 transition.
> + *
> + * @return:
> + */
> +static void esram_resume(void)
> +{
> +	struct esram_dev *edev = &esram_dev;
> +	struct esram_page *ep = NULL;
> +
> +	mutex_lock(&edev->lock);
> +	list_for_each_entry(ep, &edev->page_list, list)
> +		if (esram_page_overlay(edev, ep))
> +			pr_err("restore page %d phys %pa fail!\n",
> +				ep->id, &ep->addr);
> +	mutex_unlock(&edev->lock);
> +}
> +
> +/* Shutdown is done by RMU. Kernel needs to-do the resume() though. */
> +static struct syscore_ops esram_syscore_ops = {
> +	.resume		= esram_resume,
> +};
> +
> +/**
> + * esram_get_genpool - return pointer to esram genpool structure.
> + *
> + * @return:
> + */
> +struct gen_pool *esram_get_genpool(void)
> +{
> +	struct esram_dev *edev = &esram_dev;
> +
> +	return edev->init ? edev->pool : NULL;
> +}
> +EXPORT_SYMBOL_GPL(esram_get_genpool);
> +
> +static const struct x86_cpu_id esram_ids[] __initconst = {
> +	{ X86_VENDOR_INTEL, 5, 9 },	/* Intel Quark SoC X1000. */
> +	{}
> +};
> +MODULE_DEVICE_TABLE(x86cpu, esram_ids);
> +
> + /**
> + * esram_init - entry point for eSRAM driver.
> + *
> + * This driver manages eSRAM on a per-page basis. Therefore if we find block
> + * mode is enabled, or any global, block-level or page-level locks are in place
> + * at module initialisation time - we bail out.
> + *
> + * return: -ENODEV for no eSRAM support 0 if good to go.
> + */
> +static int __init esram_init(void)
> +{
> +	u32 block;
> +	u32 ctrl;
> +	struct esram_page *ep = NULL;
> +	struct esram_dev *edev = &esram_dev;
> +	phys_addr_t addr;
> +	int i;
> +	int ret;
> +

If there is a scheme in your variable declaration blocks, I rather
don't want to know about it.

> +	/* Calculate # of pages silicon supports. */
> +	edev->num_bytes = ESRAMCTRL_SIZE(ctrl);
> +	edev->total_pages = edev->num_bytes / PAGE_SIZE;
> +	if (edev->total_pages == 0)
> +		return -ENOMEM;
> +
> +	/* Get an array of esram pages. */
> +	edev->pages = kzalloc(edev->total_pages *
> +		sizeof(struct esram_page), GFP_KERNEL);
> +	if (IS_ERR(edev->pages)) {
> +		ret = PTR_ERR(edev->pages);
> +		goto err;
> +	}
> +
> +	/* Make an area for the gen_pool to operate from. */
> +	edev->overlay = kmalloc(edev->num_bytes, GFP_KERNEL);

This better be page aligned, right? How's that guaranteed?

> +	/* Overlay contiguous region with eSRAM pages. */
> +	addr = __pa(edev->overlay);
> +	for (i = 0; i < edev->total_pages; i++) {
> +		ep = &edev->pages[i];
> +		ep->id = i;
> +		ep->addr = addr;
> +
> +		/* Validate page state is not busy. */
> +		ret = esram_page_busy(ep);
> +		if (ret) {
> +			esram_dump_fault(ep);
> +			ret = ret < 0 ? ret : -ENOMEM;

This return value juggling is really horrible and hard to follow.

> +			goto err;
> +		}
> +
> +		/* Overlay. */
> +		ret = esram_map_page(edev, ep);
> +		if (ret)
> +			goto err;

What undoes already established mappings?

> +static void __exit esram_exit(void)
> +{
> +	struct esram_dev *edev = &esram_dev;

Again. What happens to the mappings?

Thanks,

	tglx

Re: [PATCH 2/2] x86/quark: Add Quark embedded SRAM self-test

[Paul Bolle]

On Mon, 2015-05-04 at 03:17 +0100, Bryan O'Donoghue wrote:
> --- a/arch/x86/Kconfig.debug
> +++ b/arch/x86/Kconfig.debug
 
> +config DEBUG_ESRAM_SELFTEST
> +	bool "Embedded SRAM self test"
> +	default n
> +	depends on INTEL_ESRAM
> +	---help---
> +	  This option enables automated sanity testing of the eSRAM driver
> +	  on Quark X1000. A simple set of tests with performance metrics
> +	  measured a DRAM baseline are run. These tests show the measured
> +	  performance increase across a given memory size for a series of
> +	  incrementing read sizes.
> +
> +	  If unsure say N here.

> --- a/arch/x86/platform/intel-quark/Makefile
> +++ b/arch/x86/platform/intel-quark/Makefile

> +obj-$(CONFIG_DEBUG_ESRAM_SELFTEST) += esram_selftest.o

DEBUG_ESRAM_SELFTEST is a bool Kconfig symbol. So esram_selftest.o will
never be part of a module, right?

> --- /dev/null
> +++ b/arch/x86/platform/intel-quark/esram_selftest.c

> + * IMR self test. The purpose of this module is to run a set of tests on the
> + * IMR API to validate it's sanity. We check for overlapping, reserved
> + * addresses and setup/teardown sanity.

> +MODULE_DEVICE_TABLE(x86cpu, esram_ids);

> +module_init(esram_self_test_init);
> +module_exit(esram_self_test_exit);
> +
> +MODULE_AUTHOR("Bryan O'Donoghue <pure.logic@nexus-software.ie>");
> +MODULE_DESCRIPTION("Intel Quark eSRAM self-test driver");
> +MODULE_LICENSE("Dual BSD/GPL");

There's some module specific boilerplate above. (Note that I'm not sure
whether KBUILD_MODNAME is module specific, sorry.) And the comment talks
about a module too.

Was your intention to make DEBUG_ESRAM_SELFTEST a tristate symbol?

Thanks,


Paul Bolle

Re: [PATCH 1/2] x86/quark: Add Quark embedded SRAM support

[Paul Bolle]

On Mon, 2015-05-04 at 03:17 +0100, Bryan O'Donoghue wrote:
> --- a/arch/x86/platform/intel-quark/Makefile
> +++ b/arch/x86/platform/intel-quark/Makefile

>  obj-$(CONFIG_INTEL_IMR) += imr.o

(Your change to drivers/platform/x86/Kconfig now makes it possible that
imr.o will be part of a module. More on that below.)

> +obj-$(CONFIG_INTEL_ESRAM) += esram.o

INTEL_ESRAM is a bool Kconfig symbol, so esram.o will never be part of a
module, right?

> --- /dev/null
> +++ b/arch/x86/platform/intel-quark/esram.c

> +MODULE_DEVICE_TABLE(x86cpu, esram_ids);

> +module_init(esram_init);
> +module_exit(esram_exit);
> +
> +MODULE_AUTHOR("Bryan O'Donoghue <pure.logic@nexus-software.ie>");
> +MODULE_DESCRIPTION("Intel Embedded SRAM overlay driver");
> +MODULE_LICENSE("Dual BSD/GPL");
> +

(Trailing empty line.)

> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig

>  config INTEL_IMR
> -	bool "Intel Isolated Memory Region support"
> +	tristate "Intel Isolated Memory Region support"

It's not obvious, at least to me, why this symbol is changed to
tristate. Does that follow from the other changes?

>  	default n
>  	depends on X86_INTEL_QUARK && IOSF_MBI
>  	---help---
 
> +config INTEL_ESRAM
> +	bool "Intel Embedded SRAM (eSRAM) support"

If I'm reading the corresponding Makefile change correctly, esram.o will
never be part of a module. But esram.c is added with some module
specific boilerplate. (Note, again, that I'm not sure whether
KBUILD_MODNAME is module specific.)

Was your intention perhaps to make this a tristate symbol?

> +	default n
> +	depends on X86_INTEL_QUARK && IOSF_MBI
> +	select GENERIC_ALLOCATOR
> +	---help---
> +	  This options provides an API to allocate memory from Embedded SRAM
> +	  (eSRAM) present on Quark X1000 SoC processors.
> +	  eSRAM is a 512 KiB block of low-latency SRAM organized as
> +	  128 * 4 KiB pages or as one 512 KiB chunk of memory. This driver
> +	  enables eSRAM in per-page overlay mode and provides a gen_pool
> +	  allocator which allows allocation of memory from the eSRAM pool.
> +
> +	  If you are running on a Galileo/Quark say Y here.

Thanks,


Paul Bolle

Re: [PATCH 1/2] x86/quark: Add Quark embedded SRAM support

[Bryan O'Donoghue]

On 05/05/15 01:44, Paul Bolle wrote:
> On Mon, 2015-05-04 at 03:17 +0100, Bryan O'Donoghue wrote:
>> --- a/arch/x86/platform/intel-quark/Makefile
>> +++ b/arch/x86/platform/intel-quark/Makefile
>
>>   obj-$(CONFIG_INTEL_IMR) += imr.o
>
> (Your change to drivers/platform/x86/Kconfig now makes it possible that
> imr.o will be part of a module. More on that below.)
>
>> +obj-$(CONFIG_INTEL_ESRAM) += esram.o
>
> INTEL_ESRAM is a bool Kconfig symbol, so esram.o will never be part of a
> module, right?


Nope - this is kak that is hanging around in my git repo for no good 
reason. No intention to make IMRs modules (and in fact everything would 
break if they were).

Thanks - no idea how that crept in :)
> Was your intention perhaps to make this a tristate symbol?

No I think I was making esram tristate for test purposes - edited the 
wrong line - and then didn't scrub it before the patchseries :(

Cheers !

Re: [PATCH 1/2] x86/quark: Add Quark embedded SRAM support

[Bryan O'Donoghue]

On 04/05/15 08:00, Thomas Gleixner wrote:
> On Mon, 4 May 2015, Bryan O'Donoghue wrote:
>> +++ b/arch/x86/include/asm/esram.h
>
> This should be in platform/quark/....
>
>> +++ b/arch/x86/platform/intel-quark/esram.c

No problem.

>> +#define phys_to_esram(x)	((x) >> PAGE_SHIFT)
>
> There is a single usage size for this lousy documented magic.

Hmm - OK I'll add a comment like "stuff the address field of the eSRAM 
page register" or similar.

>> +struct esram_page {
>> +	u32 id;
>> +	struct list_head list;
>> +	phys_addr_t addr;
>
> Please tab align the struct member names as you did below.

OK

>> +};
>> +
>> +/**
>> + * struct esram_dev
>> + *
>> + * Structre to represent module state/data/etc.
>> + */
>> +struct esram_dev {
>> +	struct dentry		*dbg;
>
> So dbgfs is a hard requirement for this to work?

No it's not. I had an awful hard time making a kernel without dbgfs but, 
I'll add an #ifdef for the field

>> + */
>> +static int esram_dbgfs_state_show(struct seq_file *s, void *unused)
>> +{
>> +	struct esram_dev *edev = &esram_dev;
>> +	u32 data;
>> +	u32 reg = (u32)s->private;
>
> You really like to waste lines. What's wrong with:
>
>      	u32 data, reg = .....

Hmm, I had feedback when doing the IMR code *not* to do that, so kept 
that pattern for eSRAM. More than happy to rationalize the line-count here.

>> +/**
>> + * esram_dump_fault - dump eSRAM registers and BUG().
>> + *
>> + * @return:
>
> Sigh. Please generate kernel docs from your file to catch all those
> function comment failures.

Hmm - OK - I've missed a trick here clearly - I'll check.

>> +
>> +	/* Show the page state. */
>> +	iosf_mbi_read(QRK_MBI_UNIT_MM, QRK_MBI_MMESRAM_READ, ep->id, &pgd);
>> +	pr_err("fault @ page %d state 0x%08x\n", ep->id, pgd);
>> +
>> +	/* Get state. */
>> +	iosf_mbi_read(QRK_MBI_UNIT_MM, QRK_MBI_MM_READ, ESRAMCTRL_REG, &pgc);
>> +	iosf_mbi_read(QRK_MBI_UNIT_MM, QRK_MBI_MM_READ, ESRAMPGBLOCK_REG, &pgb);
>> +	pr_err("page-control=0x%08x, page-block=0x%08x\n", pgc, pgb);
>> +
>> +	BUG();
>
> So we force BUG() here. Why?

A nice way to generate a backtrace which was useful to the BSP version 
of this code since BSP version supported deallocation of eSRAM pages and 
had a /sysfs interface to add/remove mappings.

With the version I'm proposing here, we could just as easily not BUG() 
at all.


>> +/**
>> + * esram_page_enable - Enable an eSRAM page spinning for page to become ready.
>> + *
>> + * @param ep: struct esram_page carries data to program to register.
>> + * @return	zero on success < 0 on error.
>> + */
>> +static int esram_page_enable(struct esram_page *ep)
>> +{
>> +	int ret = 0;
>> +
>> +	/* Enable a busy page => EINVAL, return IOSF error as necessary. */
>
> Why is EINVAL a good return code if the page is busy?

You're right ENOMEM is more logical.

>
>> +	ret = esram_page_busy(ep);
>> +	if (ret)
>> +		return ret < 0 ? ret : -EINVAL;
>> +
>> +	/* Enable page overlay - with automatic flush on S3 entry. */
>> +	ret = iosf_mbi_write(QRK_MBI_UNIT_MM, QRK_MBI_MMESRAM_WRITE, ep->id,
>> +			     ESRAMPGCTRL_FLUSH_PAGE_EN | ESRAMPGCTRL_EN |
>> +			     phys_to_esram(ep->addr));
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Busy bit true is good, ret < 0 means IOSF read error. */
>> +	ret = esram_page_busy(ep);
>> +	if (ret)
>> +		ret = 0;
>> +
>> +	return ret;
>
> Why not just return 0 unconitionally?

That should be if (ret < 0) we need to transmit iosf bus errors upwards.

>
>> +	if (pte == NULL || !(pte_write(*pte))) {
>> +		pr_err("invalid address for overlay %pa\n", &ep->addr);
>> +		return -ENOMEM;
>> +	}
>
> Also what makes sure that the mapping is not going away under you?
>
> Nothing, but the whole thing is not required at all. Because you map a
> kernel buffer from init(), so these half baken sanity checks are
> really useless.

The BSP code was checking for memory as ro or rw and flipping the bit in 
the page to make it r/w so the memcpy() could continue.

You're right though since the data comes from a kzalloc() there's no 
point in validating it further.

>
>> +
>> +	/* eSRAM does not autopopulate so save the contents. */
>> +	memcpy(&edev->cbuf, vaddr, PAGE_SIZE);
>> +	ret = esram_page_enable(ep);
>> +	if (ret) {
>> +		esram_dump_fault(ep);
>> +		goto err;
>
>    		return ret perhaps?
>
>> +	}
>> +
>> +	/* Overlay complete, repopulate the eSRAM page with original data. */
>> +	memcpy((void *)vaddr, &esram_dev.cbuf,  PAGE_SIZE);
>
> So the caller must ensure that the DRAM content cannot change between
> the two memcpys, right? Otherwise you end up with inconsistent data.
 > At init() time I can see how that works, on resume() rather not.

Yes absolutely true. eSRAM is not self populating - ideally you'd want 
memory transactions to be stopped until the eSRAM had populated itself.

During init this is safe. The resume callback is done via syscore_ops so 
the resume path should be called with interrupts off.

memcpy(&edev->cbuf, vaddr, PAGE_SIZE);
ret = esram_page_enable(ep);
if (ret) {
		esram_dump_fault(ep);
	goto err;
}
memcpy((void *)vaddr, &esram_dev.cbuf,  PAGE_SIZE);



>> +	/* Calculate # of pages silicon supports. */
>> +	edev->num_bytes = ESRAMCTRL_SIZE(ctrl);
>> +	edev->total_pages = edev->num_bytes / PAGE_SIZE;
>> +	if (edev->total_pages == 0)
>> +		return -ENOMEM;
>> +
>> +	/* Get an array of esram pages. */
>> +	edev->pages = kzalloc(edev->total_pages *
>> +		sizeof(struct esram_page), GFP_KERNEL);
>> +	if (IS_ERR(edev->pages)) {
>> +		ret = PTR_ERR(edev->pages);
>> +		goto err;
>> +	}
>> +
>> +	/* Make an area for the gen_pool to operate from. */
>> +	edev->overlay = kmalloc(edev->num_bytes, GFP_KERNEL);
>
> This better be page aligned, right? How's that guaranteed?

The silicon guarantees that by returning the size of eSRAM in 4k pages.

329676_QuarkDatasheet.pdf : 12.7.4.37 : ESRAMCTRL

24:16 07Fh
RO eSRAM Size (eSRAM_SIZE): eSRAM size in 4k pages ( 0 means 1)
>> +		if (ret) {
>> +			esram_dump_fault(ep);
>> +			ret = ret < 0 ? ret : -ENOMEM;
>
> This return value juggling is really horrible and hard to follow.

NP - I'll change it.

>> +			goto err;
>> +		}
>> +
>> +		/* Overlay. */
>> +		ret = esram_map_page(edev, ep);
>> +		if (ret)
>> +			goto err;
>
> What undoes already established mappings?

Nothing - unmap() is not supported by silicon. Disabling a mapping once 
it's been setup is not supported.


>> +static void __exit esram_exit(void)
>> +{
>> +	struct esram_dev *edev = &esram_dev;
>
> Again. What happens to the mappings?

Stay as-is. So in fact I shouldn't be doing any kfree()'s on already 
mapped pages.

I'll change that too.

Thanks for the review.
Bryan

Re: [PATCH 1/2] x86/quark: Add Quark embedded SRAM support

[Darren Hart]

On Tue, May 05, 2015 at 06:48:07AM -0700, Bryan O'Donoghue wrote:
> >>+ */
> >>+static int esram_dbgfs_state_show(struct seq_file *s, void *unused)
> >>+{
> >>+	struct esram_dev *edev = &esram_dev;
> >>+	u32 data;
> >>+	u32 reg = (u32)s->private;
> >
> >You really like to waste lines. What's wrong with:
> >
> >     	u32 data, reg = .....
> 
> Hmm, I had feedback when doing the IMR code *not* to do that, so kept that
> pattern for eSRAM. More than happy to rationalize the line-count here.
> 

This had come up in the IMR review, and several times in my other reviews, where
there was a strong preference for one variable per line.

I want to remain consistent with our lead maintainers, and I want to be able to
refer to something as the definitive source.

CodingStyle only has this to say in Chapter 8: Commenting:

  It's also important to comment data, whether they are basic types or derived
  types.  To this end, use just one data declaration per line (no commas for
  multiple data declarations).  This leaves you room for a small comment on each
  item, explaining its use.

Perhaps this ONLY applies if comments are used, but I didn't read it that way.

Personally, I prefer the one per line as it's the easiest to enforce and aligns
with the "decreasing line length" for declarations that Thomas has dinged me for
in the past.

Most importantly though, I just want to be consistent with how I review code so
I'm not guilty of telling Bryan one thing only for him to get dinged for
following it later (again).

Sorry about that Bryan :-)

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH 1/2] x86/quark: Add Quark embedded SRAM support

[H. Peter Anvin]

On 05/04/2015 08:00 AM, Thomas Gleixner wrote:
> 
>> + */
>> +static int esram_dbgfs_state_show(struct seq_file *s, void *unused)
>> +{
>> +	struct esram_dev *edev = &esram_dev;
>> +	u32 data;
>> +	u32 reg = (u32)s->private;
> 
> You really like to waste lines. What's wrong with:
> 
>     	u32 data, reg = .....
> 

I have to say I agree with Bryan here: it is rather ugly to mix
uninitialized and initialized variables on the same declaration line.

	-hpa

Re: [PATCH 0/2] x86/quark: Add eSRAM driver and test code

[Ingo Molnar]

* Bryan O'Donoghue <pure.logic@nexus-software.ie> wrote:

> Quark X1000 SoC contains a 512 KiB embedded SRAM (eSRAM) memory that can
> be mapped onto an area of DRAM in block or on per-page overlay mode where a
> 4 KiB aligned region can be overlayed - allowing for broken up mappings
> with a 4 KiB individual granularity.
> 
> eSRAM has access times similar to an L1 cache. The following patchset
> adds a gen_pool driver and automatic test routine to exercise eSRAM. The
> intent of the eSRAM driver is to allow other drivers to allocate SRAM
> buffers. In contrast to the original BSP code no attempt will be made to
> map kernel .data section code, this is a simple SRAM buffer allocation/free
> mechanism and a sanity test to ensure it's ongoing correctness.
> 
> Bryan O'Donoghue (2):
>   x86/quark: Add Quark embedded SRAM support
>   x86/quark: Add Quark embedded SRAM self-test

So I'm wondering what the primary usecase is for this. The eSRAM API 
is purely in-kernel, right? The only user seems to be the self-test. 
What other users will there be?

Thanks,

	Ingo

Re: [PATCH 1/2] x86/quark: Add Quark embedded SRAM support

[Ingo Molnar]

* Bryan O'Donoghue <pure.logic@nexus-software.ie> wrote:

> +/**
> + * esram_page_overlay - Overlay a page with fast access eSRAM.
> + *
> + * This function takes a 4 KiB aligned physical address and programs an
> + * eSRAM page to overlay that 4 KiB region. We require and verify that the
> + * target memory is read-write - since we don't support overlay of read-only
> + * memory regions - such as kernel .text areas. Overlay of .text areas is
> + * not supported because eSRAM isn't self-populating and we cannot guarantee
> + * atomicity of the overlay operation. It is assumed and required that the
> + * caller of the overlay function is overlaying a data buffer not kernel
> + * code.

So if this SRAM truly has L1 speeds then overlaying kernel text would 
sure be a lovely usecase!

Which part of the overlay operation is inatomic? Could we solve it by 
double buffering: first creating a temporary buffer, mapping the eSRAM 
pages there and copying kernel text to it?

Now that we've populated it, we only have to remap the eSRAM to the 
real kernel text pages.

Its inatomicity should not matter: whether the CPU fetches from DRAM 
or from eSRAM, it should be the same content.

Once the remapping is done, the double buffer can be freed.

Likewise, the text of kernel modules could be eSRAM cached as well, 
although core kernel text is probably even more beneficial.

Thanks,

	Ingo

Re: [PATCH 2/2] x86/quark: Add Quark embedded SRAM self-test

[Ingo Molnar]

* Bryan O'Donoghue <pure.logic@nexus-software.ie> wrote:

> +/**
> + * esram_self_test_time
> + *
> + * This function is carefully constructed to measure and verify the
> + * performance boost provided by eSRAM. We invalidate the cache with a
> + * wbinvd() and then perform a series of reads - each of which will cause a
> + * cacheline miss. We measure the aggregate time it takes to complete the
> + * series of reads and return the delta in cycles. The calling function will
> + * pass either a pointer to DRAM or a pointer to eSRAM.
> + *
> + * @param walker:	Pointer to RAM area to test.
> + * @return:		Number of cycles to complete test.
> + */
> +static cycles_t esram_self_test_time(char *walker, ssize_t step, ssize_t size)
> +{
> +	volatile u32 dummy = 0;
> +	int i;
> +	int j;
> +	cycle_t t1;
> +	cycle_t t2;
> +	u32 page_count = size / PAGE_SIZE;
> +
> +	local_irq_disable();
> +	t1 = get_cycles();
> +	for (i = 0; i < page_count; i++) {
> +		for (j = 0; j < PAGE_SIZE/step; j++) {
> +			dummy += *(u32 *)walker;
> +			walker += step;
> +		}
> +	}
> +	t2 = get_cycles();
> +	local_irq_enable();
> +
> +	return t2 > t1 ? t2 - t1 : ULLONG_MAX - t2 + t1;
> +}

So I don't see the wbinvd() that the description mentions.

> + * esram_self_test_init - entry point for IMR driver.
> + * esram_self_test_exit - exit point for IMR code.

Stale 'IMR' references left over from copy & paste?

Thanks,

	Ingo

Re: [PATCH 2/2] x86/quark: Add Quark embedded SRAM self-test

[Ingo Molnar]

* Bryan O'Donoghue <pure.logic@nexus-software.ie> wrote:

> +config INTEL_ESRAM
> +	bool "Intel Embedded SRAM (eSRAM) support"
> +	default n
> +	depends on X86_INTEL_QUARK && IOSF_MBI
> +	select GENERIC_ALLOCATOR
> +	---help---
> +	  This options provides an API to allocate memory from Embedded SRAM
> +	  (eSRAM) present on Quark X1000 SoC processors.
> +	  eSRAM is a 512 KiB block of low-latency SRAM organized as
> +	  128 * 4 KiB pages or as one 512 KiB chunk of memory. This driver
> +	  enables eSRAM in per-page overlay mode and provides a gen_pool
> +	  allocator which allows allocation of memory from the eSRAM pool.
> +
> +	  If you are running on a Galileo/Quark say Y here.

So presumably X86_INTEL_QUARK is only enabled if the user wants to 
build for Galileo/Quark. So you might as well make this 'default y'.

Thanks,

	Ingo

Re: [PATCH 2/2] x86/quark: Add Quark embedded SRAM self-test

[Bryan O'Donoghue]

On 06/05/15 03:02, Ingo Molnar wrote:

>> + * This function is carefully constructed to measure and verify the
>> + * performance boost provided by eSRAM. We invalidate the cache with a
>> + * wbinvd() and then perform a series of reads - each of which will cause a
>> + * cacheline miss. We measure the aggregate time it takes to complete the
>> + * series of reads and return the delta in cycles. The calling function will
>> + * pass either a pointer to DRAM or a pointer to eSRAM.
>> + *
>> + * @param walker:	Pointer to RAM area to test.
>> + * @return:		Number of cycles to complete test.
>> + */

<snip>

>
> So I don't see the wbinvd() that the description mentions.

wbinvd(); isn't required to make the test do it's job. Missed scrubbing 
from the description - damn :(

>
>> + * esram_self_test_init - entry point for IMR driver.
>> + * esram_self_test_exit - exit point for IMR code.
>
> Stale 'IMR' references left over from copy & paste?

Yep

Re: [PATCH 0/2] x86/quark: Add eSRAM driver and test code

[Bryan O'Donoghue]

On 06/05/15 02:52, Ingo Molnar wrote:
>
> * Bryan O'Donoghue <pure.logic@nexus-software.ie> wrote:
>
>> Quark X1000 SoC contains a 512 KiB embedded SRAM (eSRAM) memory that can
>> be mapped onto an area of DRAM in block or on per-page overlay mode where a
>> 4 KiB aligned region can be overlayed - allowing for broken up mappings
>> with a 4 KiB individual granularity.
>>
>> eSRAM has access times similar to an L1 cache. The following patchset
>> adds a gen_pool driver and automatic test routine to exercise eSRAM. The
>> intent of the eSRAM driver is to allow other drivers to allocate SRAM
>> buffers. In contrast to the original BSP code no attempt will be made to
>> map kernel .data section code, this is a simple SRAM buffer allocation/free
>> mechanism and a sanity test to ensure it's ongoing correctness.
>>
>> Bryan O'Donoghue (2):
>>    x86/quark: Add Quark embedded SRAM support
>>    x86/quark: Add Quark embedded SRAM self-test
>
> So I'm wondering what the primary usecase is for this. The eSRAM API
> is purely in-kernel, right? The only user seems to be the self-test.
> What other users will there be?


I see three to four users.

1. UART with DMA enabled.
2. Ethernet/STMMAC
3. SPI/I2C buffers
4. Potentially UIO

I'm working on a good use-case for UIO but rather than patch-bomb eSRAM 
+ other drivers in one go, I thought I'd get the core in and then do 
some modifications in other drivers to utilize the changes.

Re: [PATCH 1/2] x86/quark: Add Quark embedded SRAM support

[Bryan O'Donoghue]

On 06/05/15 02:58, Ingo Molnar wrote:
>
> * Bryan O'Donoghue <pure.logic@nexus-software.ie> wrote:
>
>> +/**
>> + * esram_page_overlay - Overlay a page with fast access eSRAM.
>> + *
>> + * This function takes a 4 KiB aligned physical address and programs an
>> + * eSRAM page to overlay that 4 KiB region. We require and verify that the
>> + * target memory is read-write - since we don't support overlay of read-only
>> + * memory regions - such as kernel .text areas. Overlay of .text areas is
>> + * not supported because eSRAM isn't self-populating and we cannot guarantee
>> + * atomicity of the overlay operation. It is assumed and required that the
>> + * caller of the overlay function is overlaying a data buffer not kernel
>> + * code.
>
> So if this SRAM truly has L1 speeds then overlaying kernel text would
> sure be a lovely usecase!

Yes it would. The original BSP code does support overlay of kernel .text 
areas - and there's no reason why careful and intelligent mapping of 
.text areas would be precluded.....

However we would need to be very careful about which pieces of code we 
overlay - because

1. Enable overlay @ address X
2. eSRAM enables the overlay - with a buffer containing junk
3. Kernel needs to repopulate the overlayed page


> Which part of the overlay operation is inatomic?

Since the overlay page doesn't populate itself what happens is

1. Issue command to overlay via IOSF-MBI - returning almost instantly
2. eSRAM overlay operation starts and a number of cycles later completes
3. Kernel needs to poll for completion
3. Overlayed page now contains the data in the SRAM buffer (junk)
4. Kernel needs to repopulate the overlayed page with the original data

So between the command to enable the page and the time the memcpy() 
finishes the target page for overlay cannot be relied on.

Any code necessary to do the overlay (memcpy, any address in the esram 
driver, any address in the MBI driver, any address in the PCI code) 
couldn't be overlayed. This may mean maintaining a blacklist of 
addresses which is a pain or just judicious use of the overlay by code 
that the knowledgeable of what can and can't be overlayed. I think the 
latter option is best and a pretty obvious way of doing it.

> Could we solve it by
> double buffering: first creating a temporary buffer, mapping the eSRAM
> pages there and copying kernel text to it?

I think that depends on which pieces of code we are talking about.

> Now that we've populated it, we only have to remap the eSRAM to the
> real kernel text pages.
>
> Its inatomicity should not matter: whether the CPU fetches from DRAM
> or from eSRAM, it should be the same content.
>
> Once the remapping is done, the double buffer can be freed.
>
> Likewise, the text of kernel modules could be eSRAM cached as well,
> although core kernel text is probably even more beneficial.

Hmm. You know that's a good suggestion.... I think it's worth a try.

Bryan