All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/4] Framework for exporting System-on-Chip information via sysfs
@ 2011-08-10 13:03 Lee Jones
  2011-08-10 13:03 ` [PATCH 2/4] Add documenation for new sysfs devices/soc functionallity Lee Jones
                   ` (4 more replies)
  0 siblings, 5 replies; 24+ messages in thread
From: Lee Jones @ 2011-08-10 13:03 UTC (permalink / raw)
  To: linux-arm-kernel

This patch introduces a new driver to drivers/base. The
driver provides a means to export vital SoC data out to
userspace via sysfs. Standard information applicable to all
SoCs are exported to:

/sys/devices/soc/[1|2|3|...]/[family|machine|revision|soc_id].

It is possible to create SoC specific items via the
soc_parent, which is returned post-registration, although
this should be discouraged.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/base/Kconfig    |    3 +
 drivers/base/Makefile   |    1 +
 drivers/base/soc.c      |  104 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/sys_soc.h |   41 ++++++++++++++++++
 4 files changed, 149 insertions(+), 0 deletions(-)
 create mode 100644 drivers/base/soc.c
 create mode 100644 include/linux/sys_soc.h

diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
index d57e8d0..372ef3a 100644
--- a/drivers/base/Kconfig
+++ b/drivers/base/Kconfig
@@ -168,4 +168,7 @@ config SYS_HYPERVISOR
 	bool
 	default n
 
+config SYS_SOC
+	bool
+
 endmenu
diff --git a/drivers/base/Makefile b/drivers/base/Makefile
index 4c5701c..a0d246d 100644
--- a/drivers/base/Makefile
+++ b/drivers/base/Makefile
@@ -18,6 +18,7 @@ ifeq ($(CONFIG_SYSFS),y)
 obj-$(CONFIG_MODULES)	+= module.o
 endif
 obj-$(CONFIG_SYS_HYPERVISOR) += hypervisor.o
+obj-$(CONFIG_SYS_SOC) += soc.o
 
 ccflags-$(CONFIG_DEBUG_DRIVER) := -DDEBUG
 
diff --git a/drivers/base/soc.c b/drivers/base/soc.c
new file mode 100644
index 0000000..05944ef
--- /dev/null
+++ b/drivers/base/soc.c
@@ -0,0 +1,104 @@
+/*
+ * Copyright (C) ST-Ericsson SA 2011
+ *
+ * Author: Lee Jones <lee.jones@linaro.org> for ST-Ericsson.
+ * License terms:  GNU General Public License (GPL), version 2
+ */
+
+#include <linux/sysfs.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/stat.h>
+#include <linux/slab.h>
+#include <linux/sys_soc.h>
+
+static DEFINE_SPINLOCK(register_lock);
+static int soc_count = 0;
+
+static struct device soc_grandparent = {
+	.init_name = "soc",
+};
+
+static ssize_t soc_info_get(struct device *dev,
+			struct device_attribute *attr,
+			char *buf)
+{
+	struct soc_device *soc_dev = dev->platform_data;
+
+	if (!strncmp(attr->attr.name, "machine", 7))
+		return sprintf(buf, "%s\n", soc_dev->machine);
+	if (!strncmp(attr->attr.name, "family", 6))
+		return sprintf(buf, "%s\n", soc_dev->family);
+	if (!strncmp(attr->attr.name, "soc_id", 6))
+		return sprintf(buf, "%s\n", soc_dev->soc_id);
+	if (!strncmp(attr->attr.name, "revision", 8))
+		return sprintf(buf, "%s\n", soc_dev->revision);
+
+	return sprintf(buf, "Unknown attribute name - %s\n", attr->attr.name);
+}
+
+struct device_attribute soc_attrs[] = {
+	__ATTR(machine,  S_IRUGO, soc_info_get,  NULL),
+	__ATTR(family,   S_IRUGO, soc_info_get,  NULL),
+	__ATTR(soc_id,   S_IRUGO, soc_info_get,  NULL),
+	__ATTR(revision, S_IRUGO, soc_info_get,  NULL),
+	__ATTR_NULL,
+};
+
+static void __exit soc_device_remove_files(struct device *soc)
+{
+	int i = 0;
+
+	while (soc_attrs[i++].attr.name != NULL)
+		device_remove_file(soc, &soc_attrs[i]);
+}
+
+static int __init soc_device_create_files(struct device *soc)
+{
+	int ret = 0;
+	int i   = 0;
+
+	while (soc_attrs[i].attr.name != NULL) {
+		ret = device_create_file(soc, &soc_attrs[i++]);
+		if (ret)
+			goto out;
+	}
+	return ret;
+
+out:
+	soc_device_remove_files(soc);
+	return ret;
+}
+
+int __init soc_device_register(struct device *soc_parent,
+			struct soc_device *soc_dev)
+{
+	int ret;
+
+	spin_lock_irq(&register_lock);
+
+	if (!soc_count) {
+		/* Register top-level SoC device '/sys/devices/soc' */
+		ret = device_register(&soc_grandparent);
+		if (ret)
+		{
+			spin_unlock_irq(&register_lock);
+			return ret;
+		}
+	}
+
+	soc_count++;
+	soc_parent->parent = &soc_grandparent;
+	dev_set_name(soc_parent, "%i", soc_count);
+	soc_parent->platform_data = soc_dev;
+
+	spin_unlock_irq(&register_lock);
+
+	ret = device_register(soc_parent);
+	if (ret)
+		return ret;
+
+	soc_device_create_files(soc_parent);
+
+	return ret;
+}
diff --git a/include/linux/sys_soc.h b/include/linux/sys_soc.h
new file mode 100644
index 0000000..3de6405
--- /dev/null
+++ b/include/linux/sys_soc.h
@@ -0,0 +1,41 @@
+/*
+ * Copyright (C) ST-Ericsson SA 2011
+ * Author: Lee Jones <lee.jones@linaro.org> for ST-Ericsson.
+ * License terms:  GNU General Public License (GPL), version 2
+ */
+#ifndef __SYS_SOC_H
+#define __SYS_SOC_H
+
+#include <linux/kobject.h>
+#include <linux/device.h>
+#include <linux/platform_device.h>
+
+#define MAX_SOCS 8
+#define MACHINE  0
+#define FAMILY   1
+#define SOCID    2
+#define REVISION 3
+
+struct soc_device {
+	struct device dev;
+	const char *machine;
+	const char *family;
+	const char *soc_id;
+	const char *revision;
+};
+
+/**
+ * soc_device_register - register SoC as a device
+ * @soc_parent: Parent node '/sys/devices/soc/X'
+ * @soc_dev: SoC device specific information
+ */
+int soc_device_register(struct device *soc_parent,
+			struct soc_device *soc_dev);
+
+/**
+ *  soc_device_unregister - unregister SoC as a device
+ * @soc_parent: Parent node '/sys/devices/soc/X'
+ */
+void soc_device_unregister(struct device *soc_parent);
+
+#endif /* __SYS_SOC_H */
-- 
1.7.4.1

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH 2/4] Add documenation for new sysfs devices/soc functionallity
  2011-08-10 13:03 [PATCH 1/4] Framework for exporting System-on-Chip information via sysfs Lee Jones
@ 2011-08-10 13:03 ` Lee Jones
  2011-08-10 15:02   ` Greg KH
  2011-08-10 13:03 ` [PATCH 3/4] mach-ux500: export System-on-Chip information ux500 via sysfs Lee Jones
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 24+ messages in thread
From: Lee Jones @ 2011-08-10 13:03 UTC (permalink / raw)
  To: linux-arm-kernel

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 Documentation/ABI/testing/sysfs-devices-soc |   16 ++++++++++++++++
 1 files changed, 16 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-devices-soc

diff --git a/Documentation/ABI/testing/sysfs-devices-soc b/Documentation/ABI/testing/sysfs-devices-soc
new file mode 100644
index 0000000..9156ba0
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-devices-soc
@@ -0,0 +1,16 @@
+What:		/sys/devices/soc
+Date:		April 2011
+contact:	Lee Jones <lee.jones@linaro.org>
+Description:
+		The /sys/devices/soc directory contains a sub-directory for each
+		System-on-Chip (SoC) device on a running platform. Information
+		regarding each SoC can be obtained by reading sysfs files. This
+		functionallity is only available if implemented by the platform.
+		This directory contains two kinds of attributes:
+			- Common attributes:
+				* machine: the name of the machine (e.g. Ux500).
+				* family: the family name of the SoC (e.g. DB8500).
+			- SoC-specific attributes:
+				* SoC vendor declared, such as serial and rev numbers.
+Users:
+		Any platform dependent user-space applications or profiling tools.
-- 
1.7.4.1

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH 3/4] mach-ux500: export System-on-Chip information ux500 via sysfs
  2011-08-10 13:03 [PATCH 1/4] Framework for exporting System-on-Chip information via sysfs Lee Jones
  2011-08-10 13:03 ` [PATCH 2/4] Add documenation for new sysfs devices/soc functionallity Lee Jones
@ 2011-08-10 13:03 ` Lee Jones
  2011-08-10 13:34   ` Jamie Iles
                     ` (2 more replies)
  2011-08-10 13:03 ` [PATCH 4/4] Move top level platform devices in sysfs to /sys/devices/soc/X Lee Jones
                   ` (2 subsequent siblings)
  4 siblings, 3 replies; 24+ messages in thread
From: Lee Jones @ 2011-08-10 13:03 UTC (permalink / raw)
  To: linux-arm-kernel

Here we make use of the new drivers/base/soc driver to export
vital SoC information out to userspace via sysfs. This patch
provides a data structure of strings to populate the base
nodes found in:

/sys/devices/soc/[1|2|3|...]/[family|machine|revision|soc_id].

It also adds one more node as requested by ST-Ericsson.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/mach-ux500/Kconfig              |    1 +
 arch/arm/mach-ux500/id.c                 |  115 ++++++++++++++++++++++++++++++
 arch/arm/mach-ux500/include/mach/setup.h |    1 +
 3 files changed, 117 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-ux500/Kconfig b/arch/arm/mach-ux500/Kconfig
index 4210cb4..4d2f2c2 100644
--- a/arch/arm/mach-ux500/Kconfig
+++ b/arch/arm/mach-ux500/Kconfig
@@ -26,6 +26,7 @@ config MACH_U8500
 	bool "U8500 Development platform"
 	depends on UX500_SOC_DB8500
 	select TPS6105X
+	select SYS_SOC
 	help
 	  Include support for the mop500 development platform.
 
diff --git a/arch/arm/mach-ux500/id.c b/arch/arm/mach-ux500/id.c
index d35122e..917d222 100644
--- a/arch/arm/mach-ux500/id.c
+++ b/arch/arm/mach-ux500/id.c
@@ -2,12 +2,17 @@
  * Copyright (C) ST-Ericsson SA 2010
  *
  * Author: Rabin Vincent <rabin.vincent@stericsson.com> for ST-Ericsson
+ * Author: Lee Jones <lee.jones@linaro.org> for ST-Ericsson
  * License terms: GNU General Public License (GPL) version 2
  */
 
 #include <linux/kernel.h>
 #include <linux/init.h>
 #include <linux/io.h>
+#include <linux/sys_soc.h>
+#include <linux/slab.h>
+#include <linux/stat.h>
+#include <linux/platform_device.h>
 
 #include <asm/cputype.h>
 #include <asm/tlbflush.h>
@@ -105,3 +110,113 @@ void __init ux500_map_io(void)
 
 	ux500_print_soc_info(asicid);
 }
+
+#define U8500_BB_UID_BASE (U8500_BACKUPRAM1_BASE + 0xFC0)
+#define U8500_BB_UID_LENGTH 5
+
+struct device *soc_parent;
+
+static void ux500_get_machine(char *buf)
+{
+	sprintf(buf, "DB%4x", dbx500_partnumber());
+}
+
+static void ux500_get_family(char *buf)
+{
+	sprintf(buf, "Ux500");
+}
+
+static void ux500_get_soc_id(char *buf)
+{
+	void __iomem *uid_base;
+	int i;
+	ssize_t sz = 0;
+
+	if (dbx500_partnumber() == 0x8500) {
+		uid_base = __io_address(U8500_BB_UID_BASE);
+		for (i = 0; i < U8500_BB_UID_LENGTH; i++) {
+			sz += sprintf(buf + sz, "%08x", readl(uid_base + i * sizeof(u32)));
+		}
+		return;
+	} else {
+		/* Don't know where it is located for U5500 */
+		sprintf(buf, "N/A");
+		return;
+	}
+}
+
+static void ux500_get_revision(char *buf)
+{
+	unsigned int rev = dbx500_revision();
+
+	if (rev == 0x01) {
+		sprintf(buf, "%s", "ED");
+		return;
+	}
+	else if (rev >= 0xA0) {
+		sprintf(buf, "%d.%d" , (rev >> 4) - 0xA + 1, rev & 0xf);
+		return;
+	}
+
+	sprintf(buf, "%s", "Unknown");
+}
+
+static ssize_t ux500_get_process(struct device *dev,
+				struct device_attribute *attr,
+				char *buf)
+{
+	if (dbx500_id.process == 0x00)
+		return sprintf(buf, "Standard\n");
+
+	return sprintf(buf, "%02xnm\n", dbx500_id.process);
+}
+
+static void soc_info_put(struct soc_device *soc_dev)
+{
+	char buf[1024];
+
+	ux500_get_machine(buf);
+	soc_dev->machine = kzalloc(strlen(buf), GFP_KERNEL);
+	strcpy((char *)soc_dev->machine, buf);
+
+	ux500_get_family(buf);
+	soc_dev->family = kzalloc(strlen(buf), GFP_KERNEL);
+	strcpy((char *)soc_dev->family, buf);
+
+	ux500_get_soc_id(buf);
+	soc_dev->soc_id = kzalloc(strlen(buf), GFP_KERNEL);
+	strcpy((char *)soc_dev->soc_id, buf);
+
+	ux500_get_revision(buf);
+	soc_dev->revision = kzalloc(strlen(buf), GFP_KERNEL);
+	strcpy((char *)soc_dev->revision, buf);
+}
+
+struct device_attribute ux500_soc_attrs[] = {
+	__ATTR(process,  S_IRUGO, ux500_get_process,  NULL),
+	__ATTR_NULL,
+};
+
+static int __init ux500_soc_sysfs_init(void)
+{
+	int ret;
+	int i = 0;
+	struct soc_device *soc_dev = kzalloc(sizeof(struct soc_device), GFP_KERNEL);
+
+	soc_parent = kzalloc(sizeof(struct device), GFP_KERNEL);
+
+	soc_info_put(soc_dev);
+
+	ret = soc_device_register(soc_parent, soc_dev);
+
+	if (ret >= 0) {
+		while (ux500_soc_attrs[i].attr.name != NULL) {
+			ret = device_create_file(soc_parent, &ux500_soc_attrs[i++]);
+			if (ret)
+				goto out;
+		}
+	}
+out:
+	return ret;
+}
+postcore_initcall(ux500_soc_sysfs_init);
diff --git a/arch/arm/mach-ux500/include/mach/setup.h b/arch/arm/mach-ux500/include/mach/setup.h
index a7d363f..a24093f 100644
--- a/arch/arm/mach-ux500/include/mach/setup.h
+++ b/arch/arm/mach-ux500/include/mach/setup.h
@@ -35,6 +35,7 @@ extern void __init amba_add_devices(struct amba_device *devs[], int num);
 
 struct sys_timer;
 extern struct sys_timer ux500_timer;
+extern struct device *soc_parent;
 
 #define __IO_DEV_DESC(x, sz)	{		\
 	.virtual	= IO_ADDRESS(x),	\
-- 
1.7.4.1

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH 4/4] Move top level platform devices in sysfs to /sys/devices/soc/X
  2011-08-10 13:03 [PATCH 1/4] Framework for exporting System-on-Chip information via sysfs Lee Jones
  2011-08-10 13:03 ` [PATCH 2/4] Add documenation for new sysfs devices/soc functionallity Lee Jones
  2011-08-10 13:03 ` [PATCH 3/4] mach-ux500: export System-on-Chip information ux500 via sysfs Lee Jones
@ 2011-08-10 13:03 ` Lee Jones
  2011-08-10 15:02   ` Greg KH
  2011-08-24 15:25   ` Arnd Bergmann
  2011-08-10 13:29 ` [PATCH 1/4] Framework for exporting System-on-Chip information via sysfs Jamie Iles
  2011-08-10 15:02 ` Greg KH
  4 siblings, 2 replies; 24+ messages in thread
From: Lee Jones @ 2011-08-10 13:03 UTC (permalink / raw)
  To: linux-arm-kernel

At the request of Arnd Bergmann this patch moves all SoC
platform devices found in sysfs from /sys/devices/platform to
/sys/devices/soc/<SoCNum>/. It is believed as the devices are
SoC specific and a /sys/devices/soc node has recently become
available, that this would be a more appropriate place to
display the data.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/mach-ux500/board-mop500.c   |   19 +++++++++++++++----
 arch/arm/mach-ux500/board-u5500.c    |    6 ++++++
 arch/arm/mach-ux500/cpu-db5500.c     |    6 ++++++
 arch/arm/mach-ux500/cpu-db8500.c     |   12 +++++++++++-
 arch/arm/mach-ux500/devices-common.c |   19 +++++++++++++++----
 arch/arm/mach-ux500/usb.c            |    4 ++++
 6 files changed, 57 insertions(+), 9 deletions(-)

diff --git a/arch/arm/mach-ux500/board-mop500.c b/arch/arm/mach-ux500/board-mop500.c
index 5b43fed..4c3295d 100644
--- a/arch/arm/mach-ux500/board-mop500.c
+++ b/arch/arm/mach-ux500/board-mop500.c
@@ -603,6 +603,7 @@ static struct platform_device *snowball_platform_devs[] __initdata = {
 static void __init mop500_init_machine(void)
 {
 	int i2c0_devs;
+	int i;
 
 	/*
 	 * The HREFv60 board removed a GPIO expander and routed
@@ -620,12 +621,22 @@ static void __init mop500_init_machine(void)
 
 	mop500_pins_init();
 
-	if (machine_is_snowball())
+	if (machine_is_snowball()) {
+		if (soc_parent->parent != NULL)
+			for (i=0; i<ARRAY_SIZE(snowball_platform_devs); i++)
+				snowball_platform_devs[i]->dev.parent = soc_parent;
+
 		platform_add_devices(snowball_platform_devs,
-					ARRAY_SIZE(snowball_platform_devs));
-	else
+				ARRAY_SIZE(snowball_platform_devs));
+	}
+	else {
+		if (soc_parent->parent != NULL)
+			for (i=0; i<ARRAY_SIZE(mop500_platform_devs); i++)
+				mop500_platform_devs[i]->dev.parent = soc_parent;
+
 		platform_add_devices(mop500_platform_devs,
-					ARRAY_SIZE(mop500_platform_devs));
+				ARRAY_SIZE(mop500_platform_devs));
+	}
 
 	mop500_i2c_init();
 	mop500_sdi_init();
diff --git a/arch/arm/mach-ux500/board-u5500.c b/arch/arm/mach-ux500/board-u5500.c
index 81e0475..4866436 100644
--- a/arch/arm/mach-ux500/board-u5500.c
+++ b/arch/arm/mach-ux500/board-u5500.c
@@ -53,11 +53,17 @@ static void __init u5500_uart_init(void)
 
 static void __init u5500_init_machine(void)
 {
+	int i;
+
 	u5500_init_devices();
 
 	u5500_sdi_init();
 	u5500_uart_init();
 
+	if (soc_parent->parent != NULL)
+		for (i=0; i<ARRAY_SIZE(u5500_platform_devices); i++)
+			u5500_platform_devices[i]->dev.parent = soc_parent;
+
 	platform_add_devices(u5500_platform_devices,
 		ARRAY_SIZE(u5500_platform_devices));
 }
diff --git a/arch/arm/mach-ux500/cpu-db5500.c b/arch/arm/mach-ux500/cpu-db5500.c
index 7d940a8..52cbbd2 100644
--- a/arch/arm/mach-ux500/cpu-db5500.c
+++ b/arch/arm/mach-ux500/cpu-db5500.c
@@ -216,11 +216,17 @@ static int usb_db5500_tx_dma_cfg[] = {
 
 void __init u5500_init_devices(void)
 {
+	int i;
+
 	db5500_add_gpios();
 	db5500_dma_init();
 	db5500_add_rtc();
 	db5500_add_usb(usb_db5500_rx_dma_cfg, usb_db5500_tx_dma_cfg);
 
+	if (soc_parent->parent != NULL)
+		for (i=0; i<ARRAY_SIZE(db5500_platform_devs); i++)
+			db5500_platform_devs[i]->dev.parent = soc_parent;
+
 	platform_add_devices(db5500_platform_devs,
 			     ARRAY_SIZE(db5500_platform_devs));
 }
diff --git a/arch/arm/mach-ux500/cpu-db8500.c b/arch/arm/mach-ux500/cpu-db8500.c
index 07562e8..cd63363 100644
--- a/arch/arm/mach-ux500/cpu-db8500.c
+++ b/arch/arm/mach-ux500/cpu-db8500.c
@@ -196,6 +196,8 @@ static int usb_db8500_tx_dma_cfg[] = {
  */
 void __init u8500_init_devices(void)
 {
+	int i;
+
 	if (cpu_is_u8500ed())
 		dma40_u8500ed_fixup();
 
@@ -203,7 +205,15 @@ void __init u8500_init_devices(void)
 	db8500_add_gpios();
 	db8500_add_usb(usb_db8500_rx_dma_cfg, usb_db8500_tx_dma_cfg);
 
-	platform_device_register_simple("cpufreq-u8500", -1, NULL, 0);
+
+	platform_device_register_resndata(
+		(soc_parent->parent != NULL) ? soc_parent : NULL,
+		"cpufreq-u8500", -1, NULL, 0, NULL, 0);
+
+	if (soc_parent->parent != NULL)
+		for (i=0; i<ARRAY_SIZE(platform_devs); i++)
+			platform_devs[i]->dev.parent = soc_parent;
+
 	platform_add_devices(platform_devs, ARRAY_SIZE(platform_devs));
 
 	return ;
diff --git a/arch/arm/mach-ux500/devices-common.c b/arch/arm/mach-ux500/devices-common.c
index 7418552..c8d1399 100644
--- a/arch/arm/mach-ux500/devices-common.c
+++ b/arch/arm/mach-ux500/devices-common.c
@@ -15,7 +15,7 @@
 #include <linux/gpio/nomadik.h>
 
 #include <mach/hardware.h>
-
+#include <mach/setup.h>
 #include "devices-common.h"
 
 struct amba_device *
@@ -45,6 +45,9 @@ dbx500_add_amba_device(const char *name, resource_size_t base,
 
 	dev->dev.platform_data = pdata;
 
+	if (soc_parent->parent != NULL)
+		dev->dev.parent = soc_parent;
+
 	ret = amba_device_register(dev, &iomem_resource);
 	if (ret) {
 		kfree(dev);
@@ -74,6 +77,9 @@ dbx500_add_platform_device(const char *name, int id, void *pdata,
 
 	dev->dev.platform_data = pdata;
 
+	if (soc_parent->parent != NULL)
+		dev->dev.parent = soc_parent;
+
 	ret = platform_device_add(dev);
 	if (ret)
 		goto out_free;
@@ -124,9 +130,14 @@ dbx500_add_gpio(int id, resource_size_t addr, int irq,
 		}
 	};
 
-	return platform_device_register_resndata(NULL, "gpio", id,
-				resources, ARRAY_SIZE(resources),
-				pdata, sizeof(*pdata));
+	return platform_device_register_resndata(
+		(soc_parent->parent != NULL) ? soc_parent : NULL,
+		"gpio",
+		id,
+		resources,
+		ARRAY_SIZE(resources),
+		pdata,
+		sizeof(*pdata));
 }
 
 void dbx500_add_gpios(resource_size_t *base, int num, int irq,
diff --git a/arch/arm/mach-ux500/usb.c b/arch/arm/mach-ux500/usb.c
index 0a01cbd..29cdd3c 100644
--- a/arch/arm/mach-ux500/usb.c
+++ b/arch/arm/mach-ux500/usb.c
@@ -9,6 +9,7 @@
 #include <linux/dma-mapping.h>
 #include <plat/ste_dma40.h>
 #include <mach/hardware.h>
+#include <mach/setup.h>
 #include <mach/usb.h>
 
 #define MUSB_DMA40_RX_CH { \
@@ -157,5 +158,8 @@ void ux500_add_usb(resource_size_t base, int irq, int *dma_rx_cfg,
 	ux500_usb_dma_update_rx_ch_config(dma_rx_cfg);
 	ux500_usb_dma_update_tx_ch_config(dma_tx_cfg);
 
+	if (soc_parent->parent != NULL)
+		ux500_musb_device.dev.parent = soc_parent;
+
 	platform_device_register(&ux500_musb_device);
 }
-- 
1.7.4.1

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH 1/4] Framework for exporting System-on-Chip information via sysfs
  2011-08-10 13:03 [PATCH 1/4] Framework for exporting System-on-Chip information via sysfs Lee Jones
                   ` (2 preceding siblings ...)
  2011-08-10 13:03 ` [PATCH 4/4] Move top level platform devices in sysfs to /sys/devices/soc/X Lee Jones
@ 2011-08-10 13:29 ` Jamie Iles
  2011-08-24 14:08   ` Lee Jones
  2011-08-10 15:02 ` Greg KH
  4 siblings, 1 reply; 24+ messages in thread
From: Jamie Iles @ 2011-08-10 13:29 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Lee,

A few minor comments inline.

Jamie

On Wed, Aug 10, 2011 at 02:03:39PM +0100, Lee Jones wrote:
> This patch introduces a new driver to drivers/base. The
> driver provides a means to export vital SoC data out to
> userspace via sysfs. Standard information applicable to all
> SoCs are exported to:
> 
> /sys/devices/soc/[1|2|3|...]/[family|machine|revision|soc_id].
> 
> It is possible to create SoC specific items via the
> soc_parent, which is returned post-registration, although
> this should be discouraged.
> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/base/Kconfig    |    3 +
>  drivers/base/Makefile   |    1 +
>  drivers/base/soc.c      |  104 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/sys_soc.h |   41 ++++++++++++++++++
>  4 files changed, 149 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/base/soc.c
>  create mode 100644 include/linux/sys_soc.h
> 
> diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
> index d57e8d0..372ef3a 100644
> --- a/drivers/base/Kconfig
> +++ b/drivers/base/Kconfig
> @@ -168,4 +168,7 @@ config SYS_HYPERVISOR
>  	bool
>  	default n
>  
> +config SYS_SOC
> +	bool
> +
>  endmenu
> diff --git a/drivers/base/Makefile b/drivers/base/Makefile
> index 4c5701c..a0d246d 100644
> --- a/drivers/base/Makefile
> +++ b/drivers/base/Makefile
> @@ -18,6 +18,7 @@ ifeq ($(CONFIG_SYSFS),y)
>  obj-$(CONFIG_MODULES)	+= module.o
>  endif
>  obj-$(CONFIG_SYS_HYPERVISOR) += hypervisor.o
> +obj-$(CONFIG_SYS_SOC) += soc.o
>  
>  ccflags-$(CONFIG_DEBUG_DRIVER) := -DDEBUG
>  
> diff --git a/drivers/base/soc.c b/drivers/base/soc.c
> new file mode 100644
> index 0000000..05944ef
> --- /dev/null
> +++ b/drivers/base/soc.c
> @@ -0,0 +1,104 @@
> +/*
> + * Copyright (C) ST-Ericsson SA 2011
> + *
> + * Author: Lee Jones <lee.jones@linaro.org> for ST-Ericsson.
> + * License terms:  GNU General Public License (GPL), version 2
> + */
> +
> +#include <linux/sysfs.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/stat.h>
> +#include <linux/slab.h>
> +#include <linux/sys_soc.h>
> +
> +static DEFINE_SPINLOCK(register_lock);
> +static int soc_count = 0;
> +
> +static struct device soc_grandparent = {
> +	.init_name = "soc",
> +};
> +
> +static ssize_t soc_info_get(struct device *dev,
> +			struct device_attribute *attr,
> +			char *buf)
> +{
> +	struct soc_device *soc_dev = dev->platform_data;
> +
> +	if (!strncmp(attr->attr.name, "machine", 7))
> +		return sprintf(buf, "%s\n", soc_dev->machine);
> +	if (!strncmp(attr->attr.name, "family", 6))
> +		return sprintf(buf, "%s\n", soc_dev->family);
> +	if (!strncmp(attr->attr.name, "soc_id", 6))
> +		return sprintf(buf, "%s\n", soc_dev->soc_id);
> +	if (!strncmp(attr->attr.name, "revision", 8))
> +		return sprintf(buf, "%s\n", soc_dev->revision);

I think strcmp() would be better here rather than strncmp() otherwise 
there could be problems if someone adds an attribute that has a previous 
one as a starting sub-string.

> +
> +	return sprintf(buf, "Unknown attribute name - %s\n", attr->attr.name);

Just return -EINVAL or similar here to propogate the error to the user.

> +}
> +
> +struct device_attribute soc_attrs[] = {
> +	__ATTR(machine,  S_IRUGO, soc_info_get,  NULL),
> +	__ATTR(family,   S_IRUGO, soc_info_get,  NULL),
> +	__ATTR(soc_id,   S_IRUGO, soc_info_get,  NULL),
> +	__ATTR(revision, S_IRUGO, soc_info_get,  NULL),
> +	__ATTR_NULL,
> +};
> +
> +static void __exit soc_device_remove_files(struct device *soc)
> +{
> +	int i = 0;
> +
> +	while (soc_attrs[i++].attr.name != NULL)
> +		device_remove_file(soc, &soc_attrs[i]);
> +}
> +
> +static int __init soc_device_create_files(struct device *soc)
> +{
> +	int ret = 0;
> +	int i   = 0;
> +
> +	while (soc_attrs[i].attr.name != NULL) {
> +		ret = device_create_file(soc, &soc_attrs[i++]);
> +		if (ret)
> +			goto out;
> +	}
> +	return ret;
> +
> +out:
> +	soc_device_remove_files(soc);

This should count back from i and unregister the attributes as 
soc_device_remove_files may try and remove files that didn't get 
registered.

> +	return ret;
> +}
> +
> +int __init soc_device_register(struct device *soc_parent,
> +			struct soc_device *soc_dev)
> +{
> +	int ret;
> +
> +	spin_lock_irq(&register_lock);
> +
> +	if (!soc_count) {
> +		/* Register top-level SoC device '/sys/devices/soc' */
> +		ret = device_register(&soc_grandparent);
> +		if (ret)
> +		{
> +			spin_unlock_irq(&register_lock);
> +			return ret;
> +		}
> +	}
> +
> +	soc_count++;
> +	soc_parent->parent = &soc_grandparent;
> +	dev_set_name(soc_parent, "%i", soc_count);
> +	soc_parent->platform_data = soc_dev;

I don't think platform_data is the right place for this.  It's not clear 
what soc_parent and soc_dev do here as soc_dev never gets registered.

Should this be:

	soc_dev->parent = &soc_grandparent;
	dev_set_name(soc_dev, "%i", soc_count);
	device_register(soc_dev);

and drop soc_parent and add the files to soc_device?  In soc_info_get(), 
you could then do:

	struct soc_device *soc = container_of(dev, struct soc_device, dev);

> +	spin_unlock_irq(&register_lock);
> +
> +	ret = device_register(soc_parent);
> +	if (ret)
> +		return ret;
> +
> +	soc_device_create_files(soc_parent);
> +
> +	return ret;
> +}
> diff --git a/include/linux/sys_soc.h b/include/linux/sys_soc.h
> new file mode 100644
> index 0000000..3de6405
> --- /dev/null
> +++ b/include/linux/sys_soc.h
> @@ -0,0 +1,41 @@
> +/*
> + * Copyright (C) ST-Ericsson SA 2011
> + * Author: Lee Jones <lee.jones@linaro.org> for ST-Ericsson.
> + * License terms:  GNU General Public License (GPL), version 2
> + */
> +#ifndef __SYS_SOC_H
> +#define __SYS_SOC_H
> +
> +#include <linux/kobject.h>
> +#include <linux/device.h>
> +#include <linux/platform_device.h>
> +
> +#define MAX_SOCS 8
> +#define MACHINE  0
> +#define FAMILY   1
> +#define SOCID    2
> +#define REVISION 3

These defines don't appear to be used anywhere afaict.

> +
> +struct soc_device {
> +	struct device dev;
> +	const char *machine;
> +	const char *family;
> +	const char *soc_id;
> +	const char *revision;
> +};
> +
> +/**
> + * soc_device_register - register SoC as a device
> + * @soc_parent: Parent node '/sys/devices/soc/X'
> + * @soc_dev: SoC device specific information
> + */
> +int soc_device_register(struct device *soc_parent,
> +			struct soc_device *soc_dev);
> +
> +/**
> + *  soc_device_unregister - unregister SoC as a device
> + * @soc_parent: Parent node '/sys/devices/soc/X'
> + */
> +void soc_device_unregister(struct device *soc_parent);

This doesn't appear to exist, and probably doesn't need to.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH 3/4] mach-ux500: export System-on-Chip information ux500 via sysfs
  2011-08-10 13:03 ` [PATCH 3/4] mach-ux500: export System-on-Chip information ux500 via sysfs Lee Jones
@ 2011-08-10 13:34   ` Jamie Iles
  2011-08-10 15:03   ` Greg KH
  2011-08-24 16:10   ` Arnd Bergmann
  2 siblings, 0 replies; 24+ messages in thread
From: Jamie Iles @ 2011-08-10 13:34 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Lee,

A few nits inline.

Jamie

On Wed, Aug 10, 2011 at 02:03:41PM +0100, Lee Jones wrote:
> Here we make use of the new drivers/base/soc driver to export
> vital SoC information out to userspace via sysfs. This patch
> provides a data structure of strings to populate the base
> nodes found in:
> 
> /sys/devices/soc/[1|2|3|...]/[family|machine|revision|soc_id].
> 
> It also adds one more node as requested by ST-Ericsson.
> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  arch/arm/mach-ux500/Kconfig              |    1 +
>  arch/arm/mach-ux500/id.c                 |  115 ++++++++++++++++++++++++++++++
>  arch/arm/mach-ux500/include/mach/setup.h |    1 +
>  3 files changed, 117 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/mach-ux500/Kconfig b/arch/arm/mach-ux500/Kconfig
> index 4210cb4..4d2f2c2 100644
> --- a/arch/arm/mach-ux500/Kconfig
> +++ b/arch/arm/mach-ux500/Kconfig
> @@ -26,6 +26,7 @@ config MACH_U8500
>  	bool "U8500 Development platform"
>  	depends on UX500_SOC_DB8500
>  	select TPS6105X
> +	select SYS_SOC
>  	help
>  	  Include support for the mop500 development platform.
>  
> diff --git a/arch/arm/mach-ux500/id.c b/arch/arm/mach-ux500/id.c
> index d35122e..917d222 100644
> --- a/arch/arm/mach-ux500/id.c
> +++ b/arch/arm/mach-ux500/id.c
> @@ -2,12 +2,17 @@
>   * Copyright (C) ST-Ericsson SA 2010
>   *
>   * Author: Rabin Vincent <rabin.vincent@stericsson.com> for ST-Ericsson
> + * Author: Lee Jones <lee.jones@linaro.org> for ST-Ericsson
>   * License terms: GNU General Public License (GPL) version 2
>   */
>  
>  #include <linux/kernel.h>
>  #include <linux/init.h>
>  #include <linux/io.h>
> +#include <linux/sys_soc.h>
> +#include <linux/slab.h>
> +#include <linux/stat.h>
> +#include <linux/platform_device.h>
>  
>  #include <asm/cputype.h>
>  #include <asm/tlbflush.h>
> @@ -105,3 +110,113 @@ void __init ux500_map_io(void)
>  
>  	ux500_print_soc_info(asicid);
>  }
> +
> +#define U8500_BB_UID_BASE (U8500_BACKUPRAM1_BASE + 0xFC0)
> +#define U8500_BB_UID_LENGTH 5
> +
> +struct device *soc_parent;
> +
> +static void ux500_get_machine(char *buf)
> +{
> +	sprintf(buf, "DB%4x", dbx500_partnumber());
> +}
> +
> +static void ux500_get_family(char *buf)
> +{
> +	sprintf(buf, "Ux500");
> +}
> +
> +static void ux500_get_soc_id(char *buf)
> +{
> +	void __iomem *uid_base;
> +	int i;
> +	ssize_t sz = 0;
> +
> +	if (dbx500_partnumber() == 0x8500) {
> +		uid_base = __io_address(U8500_BB_UID_BASE);
> +		for (i = 0; i < U8500_BB_UID_LENGTH; i++) {
> +			sz += sprintf(buf + sz, "%08x", readl(uid_base + i * sizeof(u32)));
> +		}
> +		return;
> +	} else {
> +		/* Don't know where it is located for U5500 */
> +		sprintf(buf, "N/A");
> +		return;
> +	}
> +}
> +
> +static void ux500_get_revision(char *buf)
> +{
> +	unsigned int rev = dbx500_revision();
> +
> +	if (rev == 0x01) {
> +		sprintf(buf, "%s", "ED");
> +		return;
> +	}
> +	else if (rev >= 0xA0) {
> +		sprintf(buf, "%d.%d" , (rev >> 4) - 0xA + 1, rev & 0xf);
> +		return;
> +	}
> +
> +	sprintf(buf, "%s", "Unknown");
> +}
> +
> +static ssize_t ux500_get_process(struct device *dev,
> +				struct device_attribute *attr,
> +				char *buf)
> +{
> +	if (dbx500_id.process == 0x00)
> +		return sprintf(buf, "Standard\n");
> +
> +	return sprintf(buf, "%02xnm\n", dbx500_id.process);
> +}
> +
> +static void soc_info_put(struct soc_device *soc_dev)
> +{
> +	char buf[1024];
> +
> +	ux500_get_machine(buf);
> +	soc_dev->machine = kzalloc(strlen(buf), GFP_KERNEL);
> +	strcpy((char *)soc_dev->machine, buf);
> +
> +	ux500_get_family(buf);
> +	soc_dev->family = kzalloc(strlen(buf), GFP_KERNEL);
> +	strcpy((char *)soc_dev->family, buf);
> +
> +	ux500_get_soc_id(buf);
> +	soc_dev->soc_id = kzalloc(strlen(buf), GFP_KERNEL);
> +	strcpy((char *)soc_dev->soc_id, buf);
> +
> +	ux500_get_revision(buf);
> +	soc_dev->revision = kzalloc(strlen(buf), GFP_KERNEL);
> +	strcpy((char *)soc_dev->revision, buf);

The returns of kzalloc() aren't checked here, but this can be simplified 
if you have the helpers do the allocation e.g.

	static const char *ux500_get_revision(void)
	{
		return kasprintf(GFP_KERNEL, "%s", ...);
	}

> +}
> +
> +struct device_attribute ux500_soc_attrs[] = {
> +	__ATTR(process,  S_IRUGO, ux500_get_process,  NULL),
> +	__ATTR_NULL,
> +};
> +
> +static int __init ux500_soc_sysfs_init(void)
> +{
> +	int ret;
> +	int i = 0;
> +	struct soc_device *soc_dev = kzalloc(sizeof(struct soc_device), GFP_KERNEL);
> +
> +	soc_parent = kzalloc(sizeof(struct device), GFP_KERNEL);

Should check the return values of kzalloc() here.  
kzalloc(sizeof(*soc_parent), GFP_KERNEL) might be a bit safer too.

> +	soc_info_put(soc_dev);

soc_info_put() sounds like a reference counting thing to me, so 
soc_info_populate() would be nicer :-)

> +
> +	ret = soc_device_register(soc_parent, soc_dev);
> +
> +	if (ret >= 0) {
> +		while (ux500_soc_attrs[i].attr.name != NULL) {
> +			ret = device_create_file(soc_parent, &ux500_soc_attrs[i++]);
> +			if (ret)
> +				goto out;
> +		}
> +	}
> +out:
> +	return ret;
> +}
> +postcore_initcall(ux500_soc_sysfs_init);
> diff --git a/arch/arm/mach-ux500/include/mach/setup.h b/arch/arm/mach-ux500/include/mach/setup.h
> index a7d363f..a24093f 100644
> --- a/arch/arm/mach-ux500/include/mach/setup.h
> +++ b/arch/arm/mach-ux500/include/mach/setup.h
> @@ -35,6 +35,7 @@ extern void __init amba_add_devices(struct amba_device *devs[], int num);
>  
>  struct sys_timer;
>  extern struct sys_timer ux500_timer;
> +extern struct device *soc_parent;
>  
>  #define __IO_DEV_DESC(x, sz)	{		\
>  	.virtual	= IO_ADDRESS(x),	\
> -- 
> 1.7.4.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH 1/4] Framework for exporting System-on-Chip information via sysfs
  2011-08-10 13:03 [PATCH 1/4] Framework for exporting System-on-Chip information via sysfs Lee Jones
                   ` (3 preceding siblings ...)
  2011-08-10 13:29 ` [PATCH 1/4] Framework for exporting System-on-Chip information via sysfs Jamie Iles
@ 2011-08-10 15:02 ` Greg KH
  4 siblings, 0 replies; 24+ messages in thread
From: Greg KH @ 2011-08-10 15:02 UTC (permalink / raw)
  To: linux-arm-kernel

On Wed, Aug 10, 2011 at 02:03:39PM +0100, Lee Jones wrote:
> This patch introduces a new driver to drivers/base. The
> driver provides a means to export vital SoC data out to
> userspace via sysfs. Standard information applicable to all
> SoCs are exported to:
> 
> /sys/devices/soc/[1|2|3|...]/[family|machine|revision|soc_id].
> 
> It is possible to create SoC specific items via the
> soc_parent, which is returned post-registration, although
> this should be discouraged.

Then don't return the pointer, if you don't want it to be used.

> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/base/Kconfig    |    3 +
>  drivers/base/Makefile   |    1 +
>  drivers/base/soc.c      |  104 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/sys_soc.h |   41 ++++++++++++++++++
>  4 files changed, 149 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/base/soc.c
>  create mode 100644 include/linux/sys_soc.h
> 
> diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
> index d57e8d0..372ef3a 100644
> --- a/drivers/base/Kconfig
> +++ b/drivers/base/Kconfig
> @@ -168,4 +168,7 @@ config SYS_HYPERVISOR
>  	bool
>  	default n
>  
> +config SYS_SOC
> +	bool
> +
>  endmenu
> diff --git a/drivers/base/Makefile b/drivers/base/Makefile
> index 4c5701c..a0d246d 100644
> --- a/drivers/base/Makefile
> +++ b/drivers/base/Makefile
> @@ -18,6 +18,7 @@ ifeq ($(CONFIG_SYSFS),y)
>  obj-$(CONFIG_MODULES)	+= module.o
>  endif
>  obj-$(CONFIG_SYS_HYPERVISOR) += hypervisor.o
> +obj-$(CONFIG_SYS_SOC) += soc.o
>  
>  ccflags-$(CONFIG_DEBUG_DRIVER) := -DDEBUG
>  
> diff --git a/drivers/base/soc.c b/drivers/base/soc.c
> new file mode 100644
> index 0000000..05944ef
> --- /dev/null
> +++ b/drivers/base/soc.c
> @@ -0,0 +1,104 @@
> +/*
> + * Copyright (C) ST-Ericsson SA 2011
> + *
> + * Author: Lee Jones <lee.jones@linaro.org> for ST-Ericsson.
> + * License terms:  GNU General Public License (GPL), version 2
> + */
> +
> +#include <linux/sysfs.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/stat.h>
> +#include <linux/slab.h>
> +#include <linux/sys_soc.h>
> +
> +static DEFINE_SPINLOCK(register_lock);
> +static int soc_count = 0;

Please just use the in-kernel api for stuff like this, and don't roll
your own.

> +
> +static struct device soc_grandparent = {
> +	.init_name = "soc",
> +};

No, please NEVER create a static struct device.  Again, use the properly
kernel functions that are provided to you to create such a thing.

> +
> +static ssize_t soc_info_get(struct device *dev,
> +			struct device_attribute *attr,
> +			char *buf)
> +{
> +	struct soc_device *soc_dev = dev->platform_data;
> +
> +	if (!strncmp(attr->attr.name, "machine", 7))
> +		return sprintf(buf, "%s\n", soc_dev->machine);
> +	if (!strncmp(attr->attr.name, "family", 6))
> +		return sprintf(buf, "%s\n", soc_dev->family);
> +	if (!strncmp(attr->attr.name, "soc_id", 6))
> +		return sprintf(buf, "%s\n", soc_dev->soc_id);
> +	if (!strncmp(attr->attr.name, "revision", 8))
> +		return sprintf(buf, "%s\n", soc_dev->revision);
> +
> +	return sprintf(buf, "Unknown attribute name - %s\n", attr->attr.name);

No need to return the unknown attribute name, that's what userspace just
asked for, so it knows it.  Return an error instead (-EINVAL);

> +}
> +
> +struct device_attribute soc_attrs[] = {
> +	__ATTR(machine,  S_IRUGO, soc_info_get,  NULL),
> +	__ATTR(family,   S_IRUGO, soc_info_get,  NULL),
> +	__ATTR(soc_id,   S_IRUGO, soc_info_get,  NULL),
> +	__ATTR(revision, S_IRUGO, soc_info_get,  NULL),
> +	__ATTR_NULL,
> +};
> +
> +static void __exit soc_device_remove_files(struct device *soc)
> +{
> +	int i = 0;
> +
> +	while (soc_attrs[i++].attr.name != NULL)
> +		device_remove_file(soc, &soc_attrs[i]);
> +}

We do have this ability to add and remove whole attribute groups at
once.

Your ability to reinvent core code is admirable though :)

You really want to make this whole thing a class, and use it that way
instead of what you are trying to do here (roll your own class by
reinventing all of the same code again.)

Please rewrite it that way, it will be much simpler, and smaller, and
work properly (hint, you have races here with userspace tools...)

> +#include <linux/platform_device.h>

Why is platform device needed?

> +
> +#define MAX_SOCS 8

Why have a max?

> +#define MACHINE  0
> +#define FAMILY   1
> +#define SOCID    2
> +#define REVISION 3

Why does the whole kernel need these, very generic, defines?
> +
> +struct soc_device {
> +	struct device dev;
> +	const char *machine;
> +	const char *family;
> +	const char *soc_id;
> +	const char *revision;
> +};
> +
> +/**
> + * soc_device_register - register SoC as a device
> + * @soc_parent: Parent node '/sys/devices/soc/X'
> + * @soc_dev: SoC device specific information
> + */
> +int soc_device_register(struct device *soc_parent,
> +			struct soc_device *soc_dev);

No, you should just pass in the information for the soc device, and have
the core create everything for you (like the struct device and the
rest.)  So, soc_device should really just have everything above, minus
the struct device, as that would be better to pass into the function
from callers.

greg k-h

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH 4/4] Move top level platform devices in sysfs to /sys/devices/soc/X
  2011-08-10 13:03 ` [PATCH 4/4] Move top level platform devices in sysfs to /sys/devices/soc/X Lee Jones
@ 2011-08-10 15:02   ` Greg KH
  2011-08-11 11:57     ` Linus Walleij
  2011-08-24 15:25   ` Arnd Bergmann
  1 sibling, 1 reply; 24+ messages in thread
From: Greg KH @ 2011-08-10 15:02 UTC (permalink / raw)
  To: linux-arm-kernel

On Wed, Aug 10, 2011 at 02:03:42PM +0100, Lee Jones wrote:
> At the request of Arnd Bergmann this patch moves all SoC
> platform devices found in sysfs from /sys/devices/platform to
> /sys/devices/soc/<SoCNum>/. It is believed as the devices are
> SoC specific and a /sys/devices/soc node has recently become
> available, that this would be a more appropriate place to
> display the data.

And what userspace tools did you just break by doing this?

greg k-h

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH 2/4] Add documenation for new sysfs devices/soc functionallity
  2011-08-10 13:03 ` [PATCH 2/4] Add documenation for new sysfs devices/soc functionallity Lee Jones
@ 2011-08-10 15:02   ` Greg KH
  0 siblings, 0 replies; 24+ messages in thread
From: Greg KH @ 2011-08-10 15:02 UTC (permalink / raw)
  To: linux-arm-kernel

On Wed, Aug 10, 2011 at 02:03:40PM +0100, Lee Jones wrote:
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  Documentation/ABI/testing/sysfs-devices-soc |   16 ++++++++++++++++
>  1 files changed, 16 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/ABI/testing/sysfs-devices-soc
> 
> diff --git a/Documentation/ABI/testing/sysfs-devices-soc b/Documentation/ABI/testing/sysfs-devices-soc
> new file mode 100644
> index 0000000..9156ba0
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-devices-soc
> @@ -0,0 +1,16 @@
> +What:		/sys/devices/soc
> +Date:		April 2011
> +contact:	Lee Jones <lee.jones@linaro.org>
> +Description:
> +		The /sys/devices/soc directory contains a sub-directory for each
> +		System-on-Chip (SoC) device on a running platform. Information
> +		regarding each SoC can be obtained by reading sysfs files. This
> +		functionallity is only available if implemented by the platform.
> +		This directory contains two kinds of attributes:
> +			- Common attributes:
> +				* machine: the name of the machine (e.g. Ux500).
> +				* family: the family name of the SoC (e.g. DB8500).
> +			- SoC-specific attributes:
> +				* SoC vendor declared, such as serial and rev numbers.

Spell out these individual files with individual entries please.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH 3/4] mach-ux500: export System-on-Chip information ux500 via sysfs
  2011-08-10 13:03 ` [PATCH 3/4] mach-ux500: export System-on-Chip information ux500 via sysfs Lee Jones
  2011-08-10 13:34   ` Jamie Iles
@ 2011-08-10 15:03   ` Greg KH
  2011-09-01  6:58     ` Lee Jones
  2011-08-24 16:10   ` Arnd Bergmann
  2 siblings, 1 reply; 24+ messages in thread
From: Greg KH @ 2011-08-10 15:03 UTC (permalink / raw)
  To: linux-arm-kernel

On Wed, Aug 10, 2011 at 02:03:41PM +0100, Lee Jones wrote:
> Here we make use of the new drivers/base/soc driver to export
> vital SoC information out to userspace via sysfs. This patch
> provides a data structure of strings to populate the base
> nodes found in:
> 
> /sys/devices/soc/[1|2|3|...]/[family|machine|revision|soc_id].
> 
> It also adds one more node as requested by ST-Ericsson.

What is that node, and why was it requested and where is it documented?

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH 4/4] Move top level platform devices in sysfs to /sys/devices/soc/X
  2011-08-10 15:02   ` Greg KH
@ 2011-08-11 11:57     ` Linus Walleij
  2011-08-11 15:22       ` Greg KH
  0 siblings, 1 reply; 24+ messages in thread
From: Linus Walleij @ 2011-08-11 11:57 UTC (permalink / raw)
  To: linux-arm-kernel

On 08/10/2011 05:02 PM, Greg KH wrote:
> On Wed, Aug 10, 2011 at 02:03:42PM +0100, Lee Jones wrote:
>    
>> At the request of Arnd Bergmann this patch moves all SoC
>> platform devices found in sysfs from /sys/devices/platform to
>> /sys/devices/soc/<SoCNum>/. It is believed as the devices are
>> SoC specific and a /sys/devices/soc node has recently become
>> available, that this would be a more appropriate place to
>> display the data.
>>      
> And what userspace tools did you just break by doing this?
>    

I think most of our userspace for this platform poking
around in sysfs use /sys/class/* so should be pretty OK.

And none of which were properly documented in
Documentation/ABI/* anyway, so do we care?
grep -r 'devices/platform' Documentation/ABI/
doesn't show anything relating to the ux500.

The major breakage would be out-of-tree stuff, if
any.

Which is out-of-tree due to the absence of a SoC ID
framework.

Which was nixed by Arnd et al precisely because
the above change was not part of it.

It's a bit hard for Lee to please everyone with this
one I suspect :-D

Thanks,
Linus Walleij

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH 4/4] Move top level platform devices in sysfs to /sys/devices/soc/X
  2011-08-11 11:57     ` Linus Walleij
@ 2011-08-11 15:22       ` Greg KH
  2011-08-11 18:24         ` Linus Walleij
  0 siblings, 1 reply; 24+ messages in thread
From: Greg KH @ 2011-08-11 15:22 UTC (permalink / raw)
  To: linux-arm-kernel

On Thu, Aug 11, 2011 at 01:57:13PM +0200, Linus Walleij wrote:
> On 08/10/2011 05:02 PM, Greg KH wrote:
> >On Wed, Aug 10, 2011 at 02:03:42PM +0100, Lee Jones wrote:
> >>At the request of Arnd Bergmann this patch moves all SoC
> >>platform devices found in sysfs from /sys/devices/platform to
> >>/sys/devices/soc/<SoCNum>/. It is believed as the devices are
> >>SoC specific and a /sys/devices/soc node has recently become
> >>available, that this would be a more appropriate place to
> >>display the data.
> >And what userspace tools did you just break by doing this?
> 
> I think most of our userspace for this platform poking
> around in sysfs use /sys/class/* so should be pretty OK.
> 
> And none of which were properly documented in
> Documentation/ABI/* anyway, so do we care?
> grep -r 'devices/platform' Documentation/ABI/
> doesn't show anything relating to the ux500.
> 
> The major breakage would be out-of-tree stuff, if
> any.
> 
> Which is out-of-tree due to the absence of a SoC ID
> framework.
> 
> Which was nixed by Arnd et al precisely because
> the above change was not part of it.
> 
> It's a bit hard for Lee to please everyone with this
> one I suspect :-D

Ok, if you get guarantees from anyone using the in-kernel platform soc
code that this move is acceptable, and you guarantee that no userspace
tools will break, that's fine.  But odds are, you are going to break
something...

greg k-h

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH 4/4] Move top level platform devices in sysfs to /sys/devices/soc/X
  2011-08-11 15:22       ` Greg KH
@ 2011-08-11 18:24         ` Linus Walleij
  2011-08-24 15:21           ` Arnd Bergmann
  0 siblings, 1 reply; 24+ messages in thread
From: Linus Walleij @ 2011-08-11 18:24 UTC (permalink / raw)
  To: linux-arm-kernel

2011/8/11 Greg KH <gregkh@suse.de>:

> Ok, if you get guarantees from anyone using the in-kernel platform soc
> code that this move is acceptable, and you guarantee that no userspace
> tools will break, that's fine. ?But odds are, you are going to break
> something...

Yeah I know, but I strongly suspect that these userspace tools are
created by the same organization that should've consulted me and
the rest of the community about it in the first place, so I'll cope with
it and write patches as I get flamed.

Thanks,
Linus Walleij

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH 1/4] Framework for exporting System-on-Chip information via sysfs
  2011-08-10 13:29 ` [PATCH 1/4] Framework for exporting System-on-Chip information via sysfs Jamie Iles
@ 2011-08-24 14:08   ` Lee Jones
  2011-08-24 14:19     ` Jamie Iles
  0 siblings, 1 reply; 24+ messages in thread
From: Lee Jones @ 2011-08-24 14:08 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Jamie,

Thanks for reviewing this for me.

Most of your comments I've adhered to, but I do have a question inline.

>> +	return ret;
>> +}
>> +
>> +int __init soc_device_register(struct device *soc_parent,
>> +			struct soc_device *soc_dev)
>> +{
>> +	int ret;
>> +
>> +	spin_lock_irq(&register_lock);
>> +
>> +	if (!soc_count) {
>> +		/* Register top-level SoC device '/sys/devices/soc' */
>> +		ret = device_register(&soc_grandparent);
>> +		if (ret)
>> +		{
>> +			spin_unlock_irq(&register_lock);
>> +			return ret;
>> +		}
>> +	}
>> +
>> +	soc_count++;
>> +	soc_parent->parent = &soc_grandparent;
>> +	dev_set_name(soc_parent, "%i", soc_count);
>> +	soc_parent->platform_data = soc_dev;
> 
> I don't think platform_data is the right place for this.  

I agree with you, but I was unsure how else to get the data back.

> It's not clear
> what soc_parent and soc_dev do here as soc_dev never gets registered.
>
> Should this be:
> 
> 	soc_dev->parent = &soc_grandparent;
> 	dev_set_name(soc_dev, "%i", soc_count);
> 	device_register(soc_dev);

AFAIK soc_dev can't be registered. It's just a container which holds
each SoW's information to be exported in the way of const char *'s.

struct soc_device {
	struct device dev;
	const char *machine;
	const char *family;
	const char *soc_id;
	const char *revision;
};

I don't believe that the "struct device dev;" is required either, but
this is something I was advised to put in.

Can you think of a better way to store these values other than in
platform_data then?

> and drop soc_parent and add the files to soc_device?  In soc_info_get(), 
> you could then do:
> 
> 	struct soc_device *soc = container_of(dev, struct soc_device, dev);
> 
>> +	spin_unlock_irq(&register_lock);
>> +
>> +	ret = device_register(soc_parent);
>> +	if (ret)
>> +		return ret;
>> +
>> +	soc_device_create_files(soc_parent);
>> +
>> +	return ret;
>> +}

Kind regards,
Lee

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH 1/4] Framework for exporting System-on-Chip information via sysfs
  2011-08-24 14:08   ` Lee Jones
@ 2011-08-24 14:19     ` Jamie Iles
  2011-08-24 14:22       ` Jamie Iles
  0 siblings, 1 reply; 24+ messages in thread
From: Jamie Iles @ 2011-08-24 14:19 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Lee,

On Wed, Aug 24, 2011 at 03:08:42PM +0100, Lee Jones wrote:
[...]
> >> +	return ret;
> >> +}
> >> +
> >> +int __init soc_device_register(struct device *soc_parent,
> >> +			struct soc_device *soc_dev)
> >> +{
> >> +	int ret;
> >> +
> >> +	spin_lock_irq(&register_lock);
> >> +
> >> +	if (!soc_count) {
> >> +		/* Register top-level SoC device '/sys/devices/soc' */
> >> +		ret = device_register(&soc_grandparent);
> >> +		if (ret)
> >> +		{
> >> +			spin_unlock_irq(&register_lock);
> >> +			return ret;
> >> +		}
> >> +	}
> >> +
> >> +	soc_count++;
> >> +	soc_parent->parent = &soc_grandparent;
> >> +	dev_set_name(soc_parent, "%i", soc_count);
> >> +	soc_parent->platform_data = soc_dev;
> > 
> > I don't think platform_data is the right place for this.  
> 
> I agree with you, but I was unsure how else to get the data back.
> 
> > It's not clear
> > what soc_parent and soc_dev do here as soc_dev never gets registered.
> >
> > Should this be:
> > 
> > 	soc_dev->parent = &soc_grandparent;
> > 	dev_set_name(soc_dev, "%i", soc_count);
> > 	device_register(soc_dev);
> 
> AFAIK soc_dev can't be registered. It's just a container which holds
> each SoW's information to be exported in the way of const char *'s.

Sorry, that should have been:

	dev_set_name(&soc_dev->dev, "%i", soc_count);
	device_register(&soc_dev->dev);

You should probably also set soc_dev->dev->release to something that'll 
kfree() the device on removal (even though they are unlikely to be 
removed).

> struct soc_device {
> 	struct device dev;
> 	const char *machine;
> 	const char *family;
> 	const char *soc_id;
> 	const char *revision;
> };
> 
> I don't believe that the "struct device dev;" is required either, but
> this is something I was advised to put in.
> 
> Can you think of a better way to store these values other than in
> platform_data then?

You can get the struct soc_device from the struct device with:

	#define to_soc_device(__dev) \
		container_of((__dev), struct soc_device, dev)

Then in the code:

	static ssize_t soc_info_get(struct device *dev,                                                                                                                                                                                                                                  
				    struct device_attribute *attr,                                                                                                                                                                                                                            
				    char *buf)                                                                                                                                                                                                                                                
	{                                                                                                                                                                                                                                                                                
		struct soc_device *soc_dev = to_soc_device(dev);
		...

struct platform_device is probably a good reference for this.

Jamie

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH 1/4] Framework for exporting System-on-Chip information via sysfs
  2011-08-24 14:19     ` Jamie Iles
@ 2011-08-24 14:22       ` Jamie Iles
  0 siblings, 0 replies; 24+ messages in thread
From: Jamie Iles @ 2011-08-24 14:22 UTC (permalink / raw)
  To: linux-arm-kernel

On Wed, Aug 24, 2011 at 03:19:35PM +0100, Jamie Iles wrote:
> Hi Lee,
> 
> On Wed, Aug 24, 2011 at 03:08:42PM +0100, Lee Jones wrote:
> [...]
> > >> +	return ret;
> > >> +}
> > >> +
> > >> +int __init soc_device_register(struct device *soc_parent,
> > >> +			struct soc_device *soc_dev)
> > >> +{
> > >> +	int ret;
> > >> +
> > >> +	spin_lock_irq(&register_lock);
> > >> +
> > >> +	if (!soc_count) {
> > >> +		/* Register top-level SoC device '/sys/devices/soc' */
> > >> +		ret = device_register(&soc_grandparent);
> > >> +		if (ret)
> > >> +		{
> > >> +			spin_unlock_irq(&register_lock);
> > >> +			return ret;
> > >> +		}
> > >> +	}
> > >> +
> > >> +	soc_count++;
> > >> +	soc_parent->parent = &soc_grandparent;
> > >> +	dev_set_name(soc_parent, "%i", soc_count);
> > >> +	soc_parent->platform_data = soc_dev;
> > > 
> > > I don't think platform_data is the right place for this.  
> > 
> > I agree with you, but I was unsure how else to get the data back.
> > 
> > > It's not clear
> > > what soc_parent and soc_dev do here as soc_dev never gets registered.
> > >
> > > Should this be:
> > > 
> > > 	soc_dev->parent = &soc_grandparent;
> > > 	dev_set_name(soc_dev, "%i", soc_count);
> > > 	device_register(soc_dev);
> > 
> > AFAIK soc_dev can't be registered. It's just a container which holds
> > each SoW's information to be exported in the way of const char *'s.
> 
> Sorry, that should have been:
> 
> 	dev_set_name(&soc_dev->dev, "%i", soc_count);
> 	device_register(&soc_dev->dev);
> 
> You should probably also set soc_dev->dev->release to something that'll 
> kfree() the device on removal (even though they are unlikely to be 
> removed).

Also you could take advantage struct device::type and its groups member 
for adding the common attributes.  This means that the files get added 
automatically at device_register() so there is no race between the 
device appearing and the attributes being added.

Jamie

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH 4/4] Move top level platform devices in sysfs to /sys/devices/soc/X
  2011-08-11 18:24         ` Linus Walleij
@ 2011-08-24 15:21           ` Arnd Bergmann
  0 siblings, 0 replies; 24+ messages in thread
From: Arnd Bergmann @ 2011-08-24 15:21 UTC (permalink / raw)
  To: linux-arm-kernel

On Thursday 11 August 2011, Linus Walleij wrote:
> 2011/8/11 Greg KH <gregkh@suse.de>:
> 
> > Ok, if you get guarantees from anyone using the in-kernel platform soc
> > code that this move is acceptable, and you guarantee that no userspace
> > tools will break, that's fine.  But odds are, you are going to break
> > something...
> 
> Yeah I know, but I strongly suspect that these userspace tools are
> created by the same organization that should've consulted me and
> the rest of the community about it in the first place, so I'll cope with
> it and write patches as I get flamed.

Right. Greg, note that the u8500 platform is still only partially supported
upstream, the majority of the necessary device drivers for having a 
running system is not there and those drivers keep getting changed
in order to be merged upstream.

Further, with the move of the platform to device tree based probing,
the position of all devices will change as well, to resemble the physical
HW layout.

	Arnd

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH 4/4] Move top level platform devices in sysfs to /sys/devices/soc/X
  2011-08-10 13:03 ` [PATCH 4/4] Move top level platform devices in sysfs to /sys/devices/soc/X Lee Jones
  2011-08-10 15:02   ` Greg KH
@ 2011-08-24 15:25   ` Arnd Bergmann
  1 sibling, 0 replies; 24+ messages in thread
From: Arnd Bergmann @ 2011-08-24 15:25 UTC (permalink / raw)
  To: linux-arm-kernel

On Wednesday 10 August 2011, Lee Jones wrote:
> At the request of Arnd Bergmann this patch moves all SoC
> platform devices found in sysfs from /sys/devices/platform to
> /sys/devices/soc/<SoCNum>/. It is believed as the devices are
> SoC specific and a /sys/devices/soc node has recently become
> available, that this would be a more appropriate place to
> display the data.
> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  arch/arm/mach-ux500/board-mop500.c   |   19 +++++++++++++++----
>  arch/arm/mach-ux500/board-u5500.c    |    6 ++++++
>  arch/arm/mach-ux500/cpu-db5500.c     |    6 ++++++
>  arch/arm/mach-ux500/cpu-db8500.c     |   12 +++++++++++-
>  arch/arm/mach-ux500/devices-common.c |   19 +++++++++++++++----
>  arch/arm/mach-ux500/usb.c            |    4 ++++
>  6 files changed, 57 insertions(+), 9 deletions(-)
> 

Ok. What does the device tree in /sys/devices look like after this?

Does it match the flattened device tree description that you will have?

	Arnd

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH 3/4] mach-ux500: export System-on-Chip information ux500 via sysfs
  2011-08-10 13:03 ` [PATCH 3/4] mach-ux500: export System-on-Chip information ux500 via sysfs Lee Jones
  2011-08-10 13:34   ` Jamie Iles
  2011-08-10 15:03   ` Greg KH
@ 2011-08-24 16:10   ` Arnd Bergmann
  2011-08-25  9:20     ` Lee Jones
  2 siblings, 1 reply; 24+ messages in thread
From: Arnd Bergmann @ 2011-08-24 16:10 UTC (permalink / raw)
  To: linux-arm-kernel

On Wednesday 10 August 2011, Lee Jones wrote:
> +
> +static void ux500_get_soc_id(char *buf)
> +{
> +       void __iomem *uid_base;
> +       int i;
> +       ssize_t sz = 0;
> +
> +       if (dbx500_partnumber() == 0x8500) {
> +               uid_base = __io_address(U8500_BB_UID_BASE);
> +               for (i = 0; i < U8500_BB_UID_LENGTH; i++) {
> +                       sz += sprintf(buf + sz, "%08x", readl(uid_base + i * sizeof(u32)));
> +               }
> +               return;
> +       } else {
> +               /* Don't know where it is located for U5500 */
> +               sprintf(buf, "N/A");
> +               return;
> +       }
> +}
> +

This still feels like it's hanging upside-down. You add an SOC node before you know
what it is, and then attach other device to it.

Similarly, having a function named 'ux500_soc_sysfs_init' is plain wrong.

You don't initialize sysfs here, but you should be probing a device with a
driver that happens to have a sysfs interface.

All probing of devices in general should start at the root and then trickle
down as you discover the child devices:
Each board has its own init_machine() callback that knows the main devices,
most importantly the SoC and registers them. When the driver for the SoC
gets loaded, that driver knows what devices are present in the device and
registers those recursively.

When you get this right, you can also eliminate the ugly machine_is_* checks
in the board file.

	Arnd

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH 3/4] mach-ux500: export System-on-Chip information ux500 via sysfs
  2011-08-24 16:10   ` Arnd Bergmann
@ 2011-08-25  9:20     ` Lee Jones
  2011-08-25 14:56       ` Arnd Bergmann
  0 siblings, 1 reply; 24+ messages in thread
From: Lee Jones @ 2011-08-25  9:20 UTC (permalink / raw)
  To: linux-arm-kernel

On 24/08/11 17:10, Arnd Bergmann wrote:
> On Wednesday 10 August 2011, Lee Jones wrote:
>> +
>> +static void ux500_get_soc_id(char *buf)
>> +{
>> +       void __iomem *uid_base;
>> +       int i;
>> +       ssize_t sz = 0;
>> +
>> +       if (dbx500_partnumber() == 0x8500) {
>> +               uid_base = __io_address(U8500_BB_UID_BASE);
>> +               for (i = 0; i < U8500_BB_UID_LENGTH; i++) {
>> +                       sz += sprintf(buf + sz, "%08x", readl(uid_base + i * sizeof(u32)));
>> +               }
>> +               return;
>> +       } else {
>> +               /* Don't know where it is located for U5500 */
>> +               sprintf(buf, "N/A");
>> +               return;
>> +       }
>> +}
>> +
> 
> This still feels like it's hanging upside-down. You add an SOC node before you know
> what it is, and then attach other device to it.

Does this comment have anything to do with the code above, or was it
quoted just to illustrate that we do find out what the SoC is eventually?

If I am understanding you correctly, you mean that we're registering a
'/sys/devices/soc/NODE', before we know what kind of SoC we're dealing
with and which devices it contains?

If that is the case, I don't believe that this is an issue. If this code
has been reached, we know that we're dealing with an SoC, of any type
and we decided on a naming convention of 1, 2, 3, ..., thus making prior
knowledge of the type of SoC irrelevant.

IMO, as soon as we know we're dealing with an SoC, then
'/sys/devices/soc' along with the first node '1' should be registered.
Then as we have devices appear, they should be allowed to register
themselves as children of that node.

Again, please correct me if I misunderstand you.

> Similarly, having a function named 'ux500_soc_sysfs_init' is plain wrong.
> 
> You don't initialize sysfs here, but you should be probing a device with a
> driver that happens to have a sysfs interface.

Understood. Would something like 'ux500_export_soc_info_init' be more
suitable, or maybe even drop the init? It seems a little long (although
fully describes what we're trying to achieve) to me. Perhaps you can
provide a more succinct suggestion.

> All probing of devices in general should start at the root and then trickle
> down as you discover the child devices:
> Each board has its own init_machine() callback that knows the main devices,
> most importantly the SoC and registers them. When the driver for the SoC
> gets loaded, that driver knows what devices are present in the device and
> registers those recursively.

I completely agree with you. So how does that differ to what's happening
here?

> When you get this right, you can also eliminate the ugly machine_is_* checks
> in the board file.

We can?

Kind regards,
Lee


-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH 3/4] mach-ux500: export System-on-Chip information ux500 via sysfs
  2011-08-25  9:20     ` Lee Jones
@ 2011-08-25 14:56       ` Arnd Bergmann
  2011-08-25 15:16         ` Lee Jones
  0 siblings, 1 reply; 24+ messages in thread
From: Arnd Bergmann @ 2011-08-25 14:56 UTC (permalink / raw)
  To: linux-arm-kernel

On Thursday 25 August 2011, Lee Jones wrote:
> On 24/08/11 17:10, Arnd Bergmann wrote:
> > On Wednesday 10 August 2011, Lee Jones wrote:
> >> +
> >> +static void ux500_get_soc_id(char *buf)
> >> +{
> >> +       void __iomem *uid_base;
> >> +       int i;
> >> +       ssize_t sz = 0;
> >> +
> >> +       if (dbx500_partnumber() == 0x8500) {
> >> +               uid_base = __io_address(U8500_BB_UID_BASE);
> >> +               for (i = 0; i < U8500_BB_UID_LENGTH; i++) {
> >> +                       sz += sprintf(buf + sz, "%08x", readl(uid_base + i * sizeof(u32)));
> >> +               }
> >> +               return;
> >> +       } else {
> >> +               /* Don't know where it is located for U5500 */
> >> +               sprintf(buf, "N/A");
> >> +               return;
> >> +       }
> >> +}
> >> +
> > 
> > This still feels like it's hanging upside-down. You add an SOC node before you know
> > what it is, and then attach other device to it.
> 
> Does this comment have anything to do with the code above, or was it
> quoted just to illustrate that we do find out what the SoC is eventually?

I should have been more elaborate here. The problem is that you try to provide
a generic *soc*id* function for the entire ux500 platform, which already
supports multiple SoCs and will gain support for further ones. Obviously,
the ID is a central part of the SoC that will be different for every one,
making it totally pointless to share the function across multiple SoCs.

Then you go further and inside that function check which soc you actually
have and *directly* access the registers from some magic address constant.
We spend a lot of work right now trying to get rid of those constants,
but a lot of the time we still need them to set up platform_devices on
platforms that don't yet use device tree probing. However, under no
circumstances should a random function just take a hardcoded base
address and do I/O on that.

> If I am understanding you correctly, you mean that we're registering a
> '/sys/devices/soc/NODE', before we know what kind of SoC we're dealing
> with and which devices it contains?
> 
> If that is the case, I don't believe that this is an issue. If this code
> has been reached, we know that we're dealing with an SoC, of any type
> and we decided on a naming convention of 1, 2, 3, ..., thus making prior
> knowledge of the type of SoC irrelevant.
> 
> IMO, as soon as we know we're dealing with an SoC, then
> '/sys/devices/soc' along with the first node '1' should be registered.
> Then as we have devices appear, they should be allowed to register
> themselves as children of that node.
> 
> Again, please correct me if I misunderstand you.

The name is indeed irrelevant, although a name such as 'db8500' would
arguably be more useful than a name of '1'.

Why can't you just put all db8500 specific code into the cpu-db8500.c
file along with the other code that knows about what a db8500 looks like?

> > Similarly, having a function named 'ux500_soc_sysfs_init' is plain wrong.
> > 
> > You don't initialize sysfs here, but you should be probing a device with a
> > driver that happens to have a sysfs interface.
> 
> Understood. Would something like 'ux500_export_soc_info_init' be more
> suitable, or maybe even drop the init? It seems a little long (although
> fully describes what we're trying to achieve) to me. Perhaps you can
> provide a more succinct suggestion.

The problem is the idea that you separate the "info" part from the actual
"soc" part. What I want to see is a *driver* for the soc that handles all
aspects of the soc that are unique to the soc but not to specific
devices inside of the soc.

> > All probing of devices in general should start at the root and then trickle
> > down as you discover the child devices:
> > Each board has its own init_machine() callback that knows the main devices,
> > most importantly the SoC and registers them. When the driver for the SoC
> > gets loaded, that driver knows what devices are present in the device and
> > registers those recursively.
> 
> I completely agree with you. So how does that differ to what's happening
> here?

You currently have a single mop500_init_machine() function that tries to handle
multiple very different boards, instead of having one init_machine function
for each one that is actually different.

> > When you get this right, you can also eliminate the ugly machine_is_* checks
> > in the board file.
> 
> We can?

What you should have here is instead of 

>static void __init mop500_init_machine(void)
>{
>        int i2c0_devs;
>
>        /*
>         * The HREFv60 board removed a GPIO expander and routed
>         * all these GPIO pins to the internal GPIO controller
>         * instead.
>         */
>        if (!machine_is_snowball()) {
>                if (machine_is_hrefv60())
>                        mop500_gpio_keys[0].gpio = HREFV60_PROX_SENSE_GPIO;
>                else
>                        mop500_gpio_keys[0].gpio = GPIO_PROX_SENSOR;
>        }
>
>        u8500_init_devices();
>
>        mop500_pins_init();
>        if (machine_is_snowball())
>                platform_add_devices(snowball_platform_devs,
>                                        ARRAY_SIZE(snowball_platform_devs));
>        else
>                platform_add_devices(mop500_platform_devs,
>                                        ARRAY_SIZE(mop500_platform_devs));

just do

static void snowball_init_machine(void)
{
	u8500_init_devices();
	snowball_pins_init();
	platform_add_devices(snowball_platform_devs,
                             ARRAY_SIZE(snowball_platform_devs));
	...
}

static void hrefv60_init_machine(void)
{
	u8500_init_devices();
	hrefv60_pins_init();
	platform_add_devices(mop500_platform_devs,
                             ARRAY_SIZE(mop500_platform_devs));
	...
}

Everything related to the soc node should then go into the u8500_init_devices()
function that already knows how to take care of that soc. The remaining parts
of the init_machine just deal with the board-specific components, which can
of course be very similar on related boards, e.g. each of the boards would
add an ab8500 device to the external bus, if I interpret the source correctly.

	Arnd

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH 3/4] mach-ux500: export System-on-Chip information ux500 via sysfs
  2011-08-25 14:56       ` Arnd Bergmann
@ 2011-08-25 15:16         ` Lee Jones
  0 siblings, 0 replies; 24+ messages in thread
From: Lee Jones @ 2011-08-25 15:16 UTC (permalink / raw)
  To: linux-arm-kernel

Whoa, what a brilliant response. Thanks Arnd.

I think I'm going to have to take this offline with Linus.

On 25/08/11 15:56, Arnd Bergmann wrote:
> On Thursday 25 August 2011, Lee Jones wrote:
>> On 24/08/11 17:10, Arnd Bergmann wrote:
>>> On Wednesday 10 August 2011, Lee Jones wrote:
>>>> +
>>>> +static void ux500_get_soc_id(char *buf)
>>>> +{
>>>> +       void __iomem *uid_base;
>>>> +       int i;
>>>> +       ssize_t sz = 0;
>>>> +
>>>> +       if (dbx500_partnumber() == 0x8500) {
>>>> +               uid_base = __io_address(U8500_BB_UID_BASE);
>>>> +               for (i = 0; i < U8500_BB_UID_LENGTH; i++) {
>>>> +                       sz += sprintf(buf + sz, "%08x", readl(uid_base + i * sizeof(u32)));
>>>> +               }
>>>> +               return;
>>>> +       } else {
>>>> +               /* Don't know where it is located for U5500 */
>>>> +               sprintf(buf, "N/A");
>>>> +               return;
>>>> +       }
>>>> +}
>>>> +
>>>
>>> This still feels like it's hanging upside-down. You add an SOC node before you know
>>> what it is, and then attach other device to it.
>>
>> Does this comment have anything to do with the code above, or was it
>> quoted just to illustrate that we do find out what the SoC is eventually?
> 
> I should have been more elaborate here. The problem is that you try to provide
> a generic *soc*id* function for the entire ux500 platform, which already
> supports multiple SoCs and will gain support for further ones. Obviously,
> the ID is a central part of the SoC that will be different for every one,
> making it totally pointless to share the function across multiple SoCs.
> 
> Then you go further and inside that function check which soc you actually
> have and *directly* access the registers from some magic address constant.
> We spend a lot of work right now trying to get rid of those constants,
> but a lot of the time we still need them to set up platform_devices on
> platforms that don't yet use device tree probing. However, under no
> circumstances should a random function just take a hardcoded base
> address and do I/O on that.
> 
>> If I am understanding you correctly, you mean that we're registering a
>> '/sys/devices/soc/NODE', before we know what kind of SoC we're dealing
>> with and which devices it contains?
>>
>> If that is the case, I don't believe that this is an issue. If this code
>> has been reached, we know that we're dealing with an SoC, of any type
>> and we decided on a naming convention of 1, 2, 3, ..., thus making prior
>> knowledge of the type of SoC irrelevant.
>>
>> IMO, as soon as we know we're dealing with an SoC, then
>> '/sys/devices/soc' along with the first node '1' should be registered.
>> Then as we have devices appear, they should be allowed to register
>> themselves as children of that node.
>>
>> Again, please correct me if I misunderstand you.
> 
> The name is indeed irrelevant, although a name such as 'db8500' would
> arguably be more useful than a name of '1'.
> 
> Why can't you just put all db8500 specific code into the cpu-db8500.c
> file along with the other code that knows about what a db8500 looks like?
> 
>>> Similarly, having a function named 'ux500_soc_sysfs_init' is plain wrong.
>>>
>>> You don't initialize sysfs here, but you should be probing a device with a
>>> driver that happens to have a sysfs interface.
>>
>> Understood. Would something like 'ux500_export_soc_info_init' be more
>> suitable, or maybe even drop the init? It seems a little long (although
>> fully describes what we're trying to achieve) to me. Perhaps you can
>> provide a more succinct suggestion.
> 
> The problem is the idea that you separate the "info" part from the actual
> "soc" part. What I want to see is a *driver* for the soc that handles all
> aspects of the soc that are unique to the soc but not to specific
> devices inside of the soc.
> 
>>> All probing of devices in general should start at the root and then trickle
>>> down as you discover the child devices:
>>> Each board has its own init_machine() callback that knows the main devices,
>>> most importantly the SoC and registers them. When the driver for the SoC
>>> gets loaded, that driver knows what devices are present in the device and
>>> registers those recursively.
>>
>> I completely agree with you. So how does that differ to what's happening
>> here?
> 
> You currently have a single mop500_init_machine() function that tries to handle
> multiple very different boards, instead of having one init_machine function
> for each one that is actually different.
> 
>>> When you get this right, you can also eliminate the ugly machine_is_* checks
>>> in the board file.
>>
>> We can?
> 
> What you should have here is instead of 
> 
>> static void __init mop500_init_machine(void)
>> {
>>        int i2c0_devs;
>>
>>        /*
>>         * The HREFv60 board removed a GPIO expander and routed
>>         * all these GPIO pins to the internal GPIO controller
>>         * instead.
>>         */
>>        if (!machine_is_snowball()) {
>>                if (machine_is_hrefv60())
>>                        mop500_gpio_keys[0].gpio = HREFV60_PROX_SENSE_GPIO;
>>                else
>>                        mop500_gpio_keys[0].gpio = GPIO_PROX_SENSOR;
>>        }
>>
>>        u8500_init_devices();
>>
>>        mop500_pins_init();
>>        if (machine_is_snowball())
>>                platform_add_devices(snowball_platform_devs,
>>                                        ARRAY_SIZE(snowball_platform_devs));
>>        else
>>                platform_add_devices(mop500_platform_devs,
>>                                        ARRAY_SIZE(mop500_platform_devs));
> 
> just do
> 
> static void snowball_init_machine(void)
> {
> 	u8500_init_devices();
> 	snowball_pins_init();
> 	platform_add_devices(snowball_platform_devs,
>                              ARRAY_SIZE(snowball_platform_devs));
> 	...
> }
> 
> static void hrefv60_init_machine(void)
> {
> 	u8500_init_devices();
> 	hrefv60_pins_init();
> 	platform_add_devices(mop500_platform_devs,
>                              ARRAY_SIZE(mop500_platform_devs));
> 	...
> }
> 
> Everything related to the soc node should then go into the u8500_init_devices()
> function that already knows how to take care of that soc. The remaining parts
> of the init_machine just deal with the board-specific components, which can
> of course be very similar on related boards, e.g. each of the boards would
> add an ab8500 device to the external bus, if I interpret the source correctly.
> 
> 	Arnd


-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH 3/4] mach-ux500: export System-on-Chip information ux500 via sysfs
  2011-08-10 15:03   ` Greg KH
@ 2011-09-01  6:58     ` Lee Jones
  2011-09-01 14:24       ` Greg KH
  0 siblings, 1 reply; 24+ messages in thread
From: Lee Jones @ 2011-09-01  6:58 UTC (permalink / raw)
  To: linux-arm-kernel

On 10/08/11 16:03, Greg KH wrote:
> On Wed, Aug 10, 2011 at 02:03:41PM +0100, Lee Jones wrote:
>> Here we make use of the new drivers/base/soc driver to export
>> vital SoC information out to userspace via sysfs. This patch
>> provides a data structure of strings to populate the base
>> nodes found in:
>>
>> /sys/devices/soc/[1|2|3|...]/[family|machine|revision|soc_id].
>>
>> It also adds one more node as requested by ST-Ericsson.
> 
> What is that node, and why was it requested and where is it documented?

I can answer the first two questions, but where would you like it
documented? Is the commit message suitable, or would you like another
Document text file created/appended to?

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH 3/4] mach-ux500: export System-on-Chip information ux500 via sysfs
  2011-09-01  6:58     ` Lee Jones
@ 2011-09-01 14:24       ` Greg KH
  0 siblings, 0 replies; 24+ messages in thread
From: Greg KH @ 2011-09-01 14:24 UTC (permalink / raw)
  To: linux-arm-kernel

On Thu, Sep 01, 2011 at 07:58:15AM +0100, Lee Jones wrote:
> On 10/08/11 16:03, Greg KH wrote:
> > On Wed, Aug 10, 2011 at 02:03:41PM +0100, Lee Jones wrote:
> >> Here we make use of the new drivers/base/soc driver to export
> >> vital SoC information out to userspace via sysfs. This patch
> >> provides a data structure of strings to populate the base
> >> nodes found in:
> >>
> >> /sys/devices/soc/[1|2|3|...]/[family|machine|revision|soc_id].
> >>
> >> It also adds one more node as requested by ST-Ericsson.
> > 
> > What is that node, and why was it requested and where is it documented?
> 
> I can answer the first two questions, but where would you like it
> documented? Is the commit message suitable, or would you like another
> Document text file created/appended to?

All user apis are documented in Documentation/ABI/, so files need to be
added there if you are adding/changing/deleting sysfs files.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 24+ messages in thread

end of thread, other threads:[~2011-09-01 14:24 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-08-10 13:03 [PATCH 1/4] Framework for exporting System-on-Chip information via sysfs Lee Jones
2011-08-10 13:03 ` [PATCH 2/4] Add documenation for new sysfs devices/soc functionallity Lee Jones
2011-08-10 15:02   ` Greg KH
2011-08-10 13:03 ` [PATCH 3/4] mach-ux500: export System-on-Chip information ux500 via sysfs Lee Jones
2011-08-10 13:34   ` Jamie Iles
2011-08-10 15:03   ` Greg KH
2011-09-01  6:58     ` Lee Jones
2011-09-01 14:24       ` Greg KH
2011-08-24 16:10   ` Arnd Bergmann
2011-08-25  9:20     ` Lee Jones
2011-08-25 14:56       ` Arnd Bergmann
2011-08-25 15:16         ` Lee Jones
2011-08-10 13:03 ` [PATCH 4/4] Move top level platform devices in sysfs to /sys/devices/soc/X Lee Jones
2011-08-10 15:02   ` Greg KH
2011-08-11 11:57     ` Linus Walleij
2011-08-11 15:22       ` Greg KH
2011-08-11 18:24         ` Linus Walleij
2011-08-24 15:21           ` Arnd Bergmann
2011-08-24 15:25   ` Arnd Bergmann
2011-08-10 13:29 ` [PATCH 1/4] Framework for exporting System-on-Chip information via sysfs Jamie Iles
2011-08-24 14:08   ` Lee Jones
2011-08-24 14:19     ` Jamie Iles
2011-08-24 14:22       ` Jamie Iles
2011-08-10 15:02 ` Greg KH

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.