[PATCH 1/5] Framework for exporting System-on-Chip information via sysfs

[PATCH 1/5] Framework for exporting System-on-Chip information via sysfs

[Lee Jones]

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 device, 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      |  103 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/sys_soc.h |   27 ++++++++++++
 4 files changed, 134 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 21cf46f..95f10c5 100644
--- a/drivers/base/Kconfig
+++ b/drivers/base/Kconfig
@@ -172,6 +172,9 @@ config SYS_HYPERVISOR
 	bool
 	default n
 
+config SYS_SOC
+	bool
+
 source "drivers/base/regmap/Kconfig"
 
 endmenu
diff --git a/drivers/base/Makefile b/drivers/base/Makefile
index 99a375a..a67a1e7 100644
--- a/drivers/base/Makefile
+++ b/drivers/base/Makefile
@@ -18,6 +18,7 @@ obj-$(CONFIG_MODULES)	+= module.o
 endif
 obj-$(CONFIG_SYS_HYPERVISOR) += hypervisor.o
 obj-$(CONFIG_REGMAP)	+= regmap/
+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..e9d908b
--- /dev/null
+++ b/drivers/base/soc.c
@@ -0,0 +1,103 @@
+/*
+ * 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 = container_of(dev, struct soc_device, dev);
+
+	if (!strcmp(attr->attr.name, "machine"))
+		return sprintf(buf, "%s\n", soc_dev->machine);
+	if (!strcmp(attr->attr.name, "family"))
+		return sprintf(buf, "%s\n", soc_dev->family);
+	if (!strcmp(attr->attr.name, "revision"))
+		return sprintf(buf, "%s\n", soc_dev->revision);
+	if (!strcmp(attr->attr.name, "soc_id")) {
+		if (soc_dev->pfn_soc_id)
+			return sprintf(buf, "%s\n", soc_dev->pfn_soc_id());
+		else return sprintf(buf, "N/A \n");
+	}
+
+	return -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 soc_device_remove_files(struct device *soc, int i)
+{
+	while (i > 0)
+		device_remove_file(soc, &soc_attrs[--i]);
+}
+
+static int __init soc_device_create_files(struct device *dev)
+{
+	int ret = 0;
+	int i   = 0;
+
+	while (soc_attrs[i].attr.name != NULL) {
+		ret = device_create_file(dev, &soc_attrs[i++]);
+		if (ret)
+			goto out;
+	}
+	return ret;
+
+out:
+	soc_device_remove_files(dev, --i);
+	return ret;
+}
+
+int __init soc_device_register(struct device *dev)
+{
+	struct soc_device *soc_dev = container_of(dev, struct soc_device, 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++;
+	dev->parent = &soc_grandparent;
+	dev_set_name(dev, "%i_%s", soc_count, soc_dev->machine);
+
+	spin_unlock_irq(&register_lock);
+
+	ret = device_register(dev);
+	if (ret)
+		return ret;
+
+	soc_device_create_files(dev);
+
+	return ret;
+}
diff --git a/include/linux/sys_soc.h b/include/linux/sys_soc.h
new file mode 100644
index 0000000..811d7fe
--- /dev/null
+++ b/include/linux/sys_soc.h
@@ -0,0 +1,27 @@
+/*
+ * 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>
+
+struct soc_device {
+	struct device dev;
+	const char *machine;
+	const char *family;
+	const char *revision;
+	const char *(*pfn_soc_id)(void);
+};
+
+/**
+ * soc_device_register - register SoC as a device
+ * @dev: Parent node '/sys/devices/soc/X'
+ */
+int soc_device_register(struct device *dev);
+
+#endif /* __SYS_SOC_H */
-- 
1.7.4.1

[PATCH 2/5] Add documentation for new sysfs devices/soc functionality

[Lee Jones]

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 Documentation/ABI/testing/sysfs-devices-soc |   20 ++++++++++++++++++++
 1 files changed, 20 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..943a20f
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-devices-soc
@@ -0,0 +1,20 @@
+What:		/sys/devices/soc
+Date:		September 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
+		functionality is only available if implemented by the platform.
+		This directory contains two kinds of attributes:
+			- Common attributes:
+				* machine: SoC machine name (e.g. Ux500).
+				* family: SoC family name (e.g. DB8500).
+			- SoC-specific attributes:
+				* SoC vendor declared
+                                * ST-Ericsson delcare the following
+                                  * soc_id: SoC serial number
+                                  * revision: SoC revision number
+				   * process: the way in which the chip was manufactured
+Users:
+		Any platform dependent user-space applications or profiling tools.
-- 
1.7.4.1

[PATCH 3/5] mach-ux500: export System-on-Chip information ux500 via sysfs

[Lee Jones]

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.
'process' depicts the way in which the silicon was manufactured.

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

diff --git a/arch/arm/mach-ux500/Kconfig b/arch/arm/mach-ux500/Kconfig
index 86188b2..fa70f65 100644
--- a/arch/arm/mach-ux500/Kconfig
+++ b/arch/arm/mach-ux500/Kconfig
@@ -27,6 +27,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..8fd53c7 100644
--- a/arch/arm/mach-ux500/id.c
+++ b/arch/arm/mach-ux500/id.c
@@ -2,12 +2,16 @@
  * 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/platform_device.h>
 
 #include <asm/cputype.h>
 #include <asm/tlbflush.h>
@@ -105,3 +109,76 @@ void __init ux500_map_io(void)
 
 	ux500_print_soc_info(asicid);
 }
+
+struct soc_device *soc_dev;
+
+static const char *ux500_get_machine(void)
+{
+	return kasprintf(GFP_KERNEL, "DB%4x", dbx500_partnumber());
+}
+
+static const char *ux500_get_family(void)
+{
+	return kasprintf(GFP_KERNEL, "Ux500");
+}
+
+static const char *ux500_get_revision(void)
+{
+	unsigned int rev = dbx500_revision();
+
+	if (rev == 0x01) {
+		return kasprintf(GFP_KERNEL, "%s", "ED");
+	}
+	else if (rev >= 0xA0) {
+		return kasprintf(GFP_KERNEL, "%d.%d" , (rev >> 4) - 0xA + 1, rev & 0xf);
+	}
+
+	return kasprintf(GFP_KERNEL, "%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_populate(struct soc_device *soc_dev)
+{
+	soc_dev->machine  = ux500_get_machine();
+	soc_dev->family   = ux500_get_family();
+	soc_dev->revision = ux500_get_revision();
+}
+
+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;
+
+	soc_dev = kzalloc(sizeof(*soc_dev), GFP_KERNEL);
+	if (soc_dev == NULL)
+		return -ENOMEM;
+
+	soc_info_populate(soc_dev);
+
+	ret = soc_device_register(&soc_dev->dev);
+
+	if (ret >= 0) {
+		while (ux500_soc_attrs[i].attr.name != NULL) {
+			ret = device_create_file(&soc_dev->dev, &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..7d4c35f 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 soc_device *soc_dev;
 
 #define __IO_DEV_DESC(x, sz)	{		\
 	.virtual	= IO_ADDRESS(x),	\
-- 
1.7.4.1

[PATCH 3/5] mach-ux500: export System-on-Chip information ux500 via sysfs

[Arnd Bergmann]

On Thursday 01 September 2011, 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.
> 'process' depicts the way in which the silicon was manufactured.
> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>

The code looks much better than last time, but the description still
talks about 'using the drivers/base/soc driver', which is not how
you should think of or describe what you are doing.

What you do here is make the db8500 support an soc driver.

> diff --git a/arch/arm/mach-ux500/id.c b/arch/arm/mach-ux500/id.c
> index d35122e..8fd53c7 100644
> --- a/arch/arm/mach-ux500/id.c
> +++ b/arch/arm/mach-ux500/id.c

You should probably rename this file or fold it into one of the db8500
files.

>  }
> +
> +struct soc_device *soc_dev;
> +
> +static const char *ux500_get_machine(void)
> +{
> +	return kasprintf(GFP_KERNEL, "DB%4x", dbx500_partnumber());
> +}
> +
> +static const char *ux500_get_family(void)
> +{
> +	return kasprintf(GFP_KERNEL, "Ux500");
> +}
> +
> +static const char *ux500_get_revision(void)
> +{
> +	unsigned int rev = dbx500_revision();
> +
> +	if (rev == 0x01) {
> +		return kasprintf(GFP_KERNEL, "%s", "ED");
> +	}
> +	else if (rev >= 0xA0) {
> +		return kasprintf(GFP_KERNEL, "%d.%d" , (rev >> 4) - 0xA + 1, rev & 0xf);
> +	}
> +
> +	return kasprintf(GFP_KERNEL, "%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_populate(struct soc_device *soc_dev)
> +{
> +	soc_dev->machine  = ux500_get_machine();
> +	soc_dev->family   = ux500_get_family();
> +	soc_dev->revision = ux500_get_revision();
> +}

It's often better to have multiple smaller functions than to do
something too complex in just one function, but I think you are
overdoing it here. Not a huge problem though, just my opinion
on coding style.

> +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;
> +
> +	soc_dev = kzalloc(sizeof(*soc_dev), GFP_KERNEL);
> +	if (soc_dev == NULL)
> +		return -ENOMEM;
> +
> +	soc_info_populate(soc_dev);
> +
> +	ret = soc_device_register(&soc_dev->dev);
> +
> +	if (ret >= 0) {
> +		while (ux500_soc_attrs[i].attr.name != NULL) {
> +			ret = device_create_file(&soc_dev->dev, &ux500_soc_attrs[i++]);
> +			if (ret)
> +				goto out;
> +		}
> +	}

Better not make it too easy to add more of these, you don't want to
have people add random stuff here, so I would recommend just
using one device_create_file calls without the loop.

> diff --git a/arch/arm/mach-ux500/include/mach/setup.h b/arch/arm/mach-ux500/include/mach/setup.h
> index a7d363f..7d4c35f 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 soc_device *soc_dev;

Do you really need to make this global?

The soc_dev is the central data structure describing your soc system, so
you should have it available in all the places where it might be needed
anyway, e.g. to get the base address, as described in the other mail.

	Arnd

[PATCH 4/5] mach-ux500: move top level platform devices in sysfs to /sys/devices/soc/X

[Lee Jones]

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>_<SocMachine>/. 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   |   16 ++++++++++++++++
 arch/arm/mach-ux500/board-u5500.c    |    7 +++++++
 arch/arm/mach-ux500/cpu-db5500.c     |    7 +++++++
 arch/arm/mach-ux500/cpu-db8500.c     |   13 ++++++++++++-
 arch/arm/mach-ux500/devices-common.c |   19 ++++++++++++++++---
 arch/arm/mach-ux500/usb.c            |    6 ++++++
 6 files changed, 64 insertions(+), 4 deletions(-)

diff --git a/arch/arm/mach-ux500/board-mop500.c b/arch/arm/mach-ux500/board-mop500.c
index 4d75b0d..d3b1447 100644
--- a/arch/arm/mach-ux500/board-mop500.c
+++ b/arch/arm/mach-ux500/board-mop500.c
@@ -29,6 +29,7 @@
 #include <linux/smsc911x.h>
 #include <linux/gpio_keys.h>
 #include <linux/delay.h>
+#include <linux/sys_soc.h>
 
 #include <linux/leds.h>
 #include <asm/mach-types.h>
@@ -602,6 +603,7 @@ static struct platform_device *snowball_platform_devs[] __initdata = {
 static void __init mop500_init_machine(void)
 {
 	int i2c0_devs;
+	int i;
 
 	mop500_gpio_keys[0].gpio = GPIO_PROX_SENSOR;
 
@@ -609,6 +611,10 @@ static void __init mop500_init_machine(void)
 
 	mop500_pins_init();
 
+	if (soc_dev)
+		for (i=0; i<ARRAY_SIZE(mop500_platform_devs); i++)
+			mop500_platform_devs[i]->dev.parent = &soc_dev->dev;
+
 	platform_add_devices(mop500_platform_devs,
 			ARRAY_SIZE(mop500_platform_devs));
 
@@ -630,11 +636,16 @@ static void __init mop500_init_machine(void)
 static void __init snowball_init_machine(void)
 {
 	int i2c0_devs;
+	int i;
 
 	u8500_init_devices();
 
 	snowball_pins_init();
 
+	if (soc_dev)
+		for (i=0; i<ARRAY_SIZE(snowball_platform_devs); i++)
+			snowball_platform_devs[i]->dev.parent = &soc_dev->dev;
+
 	platform_add_devices(snowball_platform_devs,
 			ARRAY_SIZE(snowball_platform_devs));
 
@@ -655,6 +666,7 @@ static void __init snowball_init_machine(void)
 static void __init hrefv60_init_machine(void)
 {
 	int i2c0_devs;
+	int i;
 
 	/*
 	 * The HREFv60 board removed a GPIO expander and routed
@@ -667,6 +679,10 @@ static void __init hrefv60_init_machine(void)
 
 	hrefv60_pins_init();
 
+	if (soc_dev)
+		for (i=0; i<ARRAY_SIZE(mop500_platform_devs); i++)
+			mop500_platform_devs[i]->dev.parent = &soc_dev->dev;
+
 	platform_add_devices(mop500_platform_devs,
 			ARRAY_SIZE(mop500_platform_devs));
 
diff --git a/arch/arm/mach-ux500/board-u5500.c b/arch/arm/mach-ux500/board-u5500.c
index 5c9e665..5a74a41 100644
--- a/arch/arm/mach-ux500/board-u5500.c
+++ b/arch/arm/mach-ux500/board-u5500.c
@@ -11,6 +11,7 @@
 #include <linux/gpio.h>
 #include <linux/irq.h>
 #include <linux/i2c.h>
+#include <linux/sys_soc.h>
 #include <linux/mfd/ab5500/ab5500.h>
 
 #include <asm/mach/arch.h>
@@ -134,12 +135,18 @@ static void __init u5500_uart_init(void)
 
 static void __init u5500_init_machine(void)
 {
+	int i;
+
 	u5500_init_devices();
 	nmk_config_pins(u5500_pins, ARRAY_SIZE(u5500_pins));
 	u5500_i2c_init();
 	u5500_sdi_init();
 	u5500_uart_init();
 
+	if (soc_dev)
+		for (i=0; i<ARRAY_SIZE(u5500_platform_devices); i++)
+			u5500_platform_devices[i]->dev.parent = &soc_dev->dev;
+
 	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 e532c32..1638ccc 100644
--- a/arch/arm/mach-ux500/cpu-db5500.c
+++ b/arch/arm/mach-ux500/cpu-db5500.c
@@ -9,6 +9,7 @@
 #include <linux/amba/bus.h>
 #include <linux/io.h>
 #include <linux/irq.h>
+#include <linux/sys_soc.h>
 
 #include <asm/mach/map.h>
 #include <asm/pmu.h>
@@ -222,11 +223,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_dev)
+		for (i=0; i<ARRAY_SIZE(db5500_platform_devs); i++)
+			db5500_platform_devs[i]->dev.parent = &soc_dev->dev;
+
 	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 4598b06..655639e 100644
--- a/arch/arm/mach-ux500/cpu-db8500.c
+++ b/arch/arm/mach-ux500/cpu-db8500.c
@@ -17,6 +17,7 @@
 #include <linux/gpio.h>
 #include <linux/platform_device.h>
 #include <linux/io.h>
+#include <linux/sys_soc.h>
 
 #include <asm/mach/map.h>
 #include <asm/pmu.h>
@@ -193,6 +194,8 @@ static int usb_db8500_tx_dma_cfg[] = {
  */
 void __init u8500_init_devices(void)
 {
+	int i;
+
 	if (cpu_is_u8500ed())
 		dma40_u8500ed_fixup();
 
@@ -200,7 +203,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_dev) ? &soc_dev->dev : NULL,
+		"cpufreq-u8500", -1, NULL, 0, NULL, 0);
+
+	if (soc_dev) {
+		for (i=0; i<ARRAY_SIZE(platform_devs); i++)
+			platform_devs[i]->dev.parent = &soc_dev->dev;
+	}
+
 	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 13a4ce0..74bfd1f 100644
--- a/arch/arm/mach-ux500/devices-common.c
+++ b/arch/arm/mach-ux500/devices-common.c
@@ -12,10 +12,12 @@
 #include <linux/slab.h>
 #include <linux/platform_device.h>
 #include <linux/amba/bus.h>
+#include <linux/sys_soc.h>
 
 #include <plat/gpio.h>
 
 #include <mach/hardware.h>
+#include <mach/setup.h>
 
 #include "devices-common.h"
 
@@ -46,6 +48,9 @@ dbx500_add_amba_device(const char *name, resource_size_t base,
 
 	dev->dev.platform_data = pdata;
 
+	if (soc_dev)
+		dev->dev.parent = &soc_dev->dev;
+
 	ret = amba_device_register(dev, &iomem_resource);
 	if (ret) {
 		kfree(dev);
@@ -75,6 +80,9 @@ dbx500_add_platform_device(const char *name, int id, void *pdata,
 
 	dev->dev.platform_data = pdata;
 
+	if (soc_dev)
+		dev->dev.parent = &soc_dev->dev;
+
 	ret = platform_device_add(dev);
 	if (ret)
 		goto out_free;
@@ -125,9 +133,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_dev) ? &soc_dev->dev : 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..aa2df4a 100644
--- a/arch/arm/mach-ux500/usb.c
+++ b/arch/arm/mach-ux500/usb.c
@@ -7,8 +7,11 @@
 #include <linux/platform_device.h>
 #include <linux/usb/musb.h>
 #include <linux/dma-mapping.h>
+#include <linux/sys_soc.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 +160,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_dev)
+		ux500_musb_device.dev.parent = &soc_dev->dev;
+
 	platform_device_register(&ux500_musb_device);
 }
-- 
1.7.4.1

[PATCH 5/5] mach-ux500: add a SoC ID (serial) callback for the u8500

[Lee Jones]

With this patch applied the Export SoC Information framework
will be able to callback into u8500 Platform code in order to
probe for the SoC's unique Identification (serial) number.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/mach-ux500/cpu-db8500.c               |   16 ++++++++++++++++
 arch/arm/mach-ux500/include/mach/db8500-regs.h |    4 ++++
 2 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-ux500/cpu-db8500.c b/arch/arm/mach-ux500/cpu-db8500.c
index 655639e..9824cd3 100644
--- a/arch/arm/mach-ux500/cpu-db8500.c
+++ b/arch/arm/mach-ux500/cpu-db8500.c
@@ -167,6 +167,20 @@ static void __init db8500_add_gpios(void)
 			 IRQ_DB8500_GPIO0, &pdata);
 }
 
+static const char *db8500_get_soc_id(void)
+{
+	void __iomem *uid_base;
+	char buf[1024];
+	ssize_t sz = 0;
+	int i;
+
+	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 kasprintf(GFP_KERNEL, "%s", buf);
+}
+
 static int usb_db8500_rx_dma_cfg[] = {
 	DB8500_DMA_DEV38_USB_OTG_IEP_1_9,
 	DB8500_DMA_DEV37_USB_OTG_IEP_2_10,
@@ -208,6 +222,8 @@ void __init u8500_init_devices(void)
 		"cpufreq-u8500", -1, NULL, 0, NULL, 0);
 
 	if (soc_dev) {
+		soc_dev->pfn_soc_id = db8500_get_soc_id;
+
 		for (i=0; i<ARRAY_SIZE(platform_devs); i++)
 			platform_devs[i]->dev.parent = &soc_dev->dev;
 	}
diff --git a/arch/arm/mach-ux500/include/mach/db8500-regs.h b/arch/arm/mach-ux500/include/mach/db8500-regs.h
index 0499971..f569d5c 100644
--- a/arch/arm/mach-ux500/include/mach/db8500-regs.h
+++ b/arch/arm/mach-ux500/include/mach/db8500-regs.h
@@ -166,4 +166,8 @@
 #define U8500_MODEM_BASE	0xe000000
 #define U8500_APE_BASE		0x6000000
 
+/* SoC identification number information */
+#define U8500_BB_UID_BASE (U8500_BACKUPRAM1_BASE + 0xFC0)
+#define U8500_BB_UID_LENGTH 5
+
 #endif
-- 
1.7.4.1

[PATCH 5/5] mach-ux500: add a SoC ID (serial) callback for the u8500

[Arnd Bergmann]

On Thursday 01 September 2011, Lee Jones wrote:
> +static const char *db8500_get_soc_id(void)
> +{
> +       void __iomem *uid_base;
> +       char buf[1024];
> +       ssize_t sz = 0;
> +       int i;
> +
> +       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 kasprintf(GFP_KERNEL, "%s", buf);
> +}

You will get a warning from the stack checker here, about putting a 1024 byte string
on the stack. Also, I still think it's bad to just access the U8500_BB_UID_BASE
from a compile-time constant. Since this gets called from a function that knows
the base address of the DB8500 register area, better pass the device in there
so that you end up with something like

static void __devinit db8500_read_soc_id(struct db8500_dev *dev)
{
	u32 __iomem *uid = dev->base + U8500_BB_UID_OFFSET;
	snprintf(dev->soc_id, sizeof (dev->soc_id), "%08x%08x%08x%08x%08x",
		readl(uid[0]), readl(uid[1]), readl(uid[2]), readl(uid[3]), readl(uid[4]));
}

The style you use here is preexisting in the db8500 code, but you should
not keep adding more of that crap. All the code like

#define db8500_add_i2c0(pdata) \
        dbx500_add_i2c(0, U8500_I2C0_BASE, IRQ_DB8500_I2C0, pdata)
#define db8500_add_i2c1(pdata) \
        dbx500_add_i2c(1, U8500_I2C1_BASE, IRQ_DB8500_I2C1, pdata)

should never really have been there. What you want to do for this is
to call this from the code that initializes the db8500 controller, and
pass the board specific pdata into the db8500 init function, along
with the i2c client data.

	Arnd

[PATCH 5/5] mach-ux500: add a SoC ID (serial) callback for the u8500

[Lee Jones]

On 02/09/11 15:22, Arnd Bergmann wrote:
> On Thursday 01 September 2011, Lee Jones wrote:
>> +static const char *db8500_get_soc_id(void)
>> +{
>> +       void __iomem *uid_base;
>> +       char buf[1024];
>> +       ssize_t sz = 0;
>> +       int i;
>> +
>> +       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 kasprintf(GFP_KERNEL, "%s", buf);
>> +}
> 
> You will get a warning from the stack checker here, about putting a 1024 byte string
> on the stack. Also, I still think it's bad to just access the U8500_BB_UID_BASE
> from a compile-time constant. Since this gets called from a function that knows
> the base address of the DB8500 register area, better pass the device in there
> so that you end up with something like
> 
> static void __devinit db8500_read_soc_id(struct db8500_dev *dev)
> {
> 	u32 __iomem *uid = dev->base + U8500_BB_UID_OFFSET;
> 	snprintf(dev->soc_id, sizeof (dev->soc_id), "%08x%08x%08x%08x%08x",
> 		readl(uid[0]), readl(uid[1]), readl(uid[2]), readl(uid[3]), readl(uid[4]));
> }

No problem.

Where did you get dev->base from though?

> The style you use here is preexisting in the db8500 code, but you should
> not keep adding more of that crap. 

Not meaning to pass the buck at all, but this isn't my code.

It's remnant from when I took this over (starting to wish I hadn't ;))

> All the code like 
> #define db8500_add_i2c0(pdata) \
>         dbx500_add_i2c(0, U8500_I2C0_BASE, IRQ_DB8500_I2C0, pdata)
> #define db8500_add_i2c1(pdata) \
>         dbx500_add_i2c(1, U8500_I2C1_BASE, IRQ_DB8500_I2C1, pdata)
> 
> should never really have been there. What you want to do for this is
> to call this from the code that initializes the db8500 controller, and
> pass the board specific pdata into the db8500 init function, along
> with the i2c client data.

Not my domain. Speak to Linus W. :)

-- 
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

[PATCH 5/5] mach-ux500: add a SoC ID (serial) callback for the u8500

[Arnd Bergmann]

On Friday 02 September 2011, Lee Jones wrote:
> > static void __devinit db8500_read_soc_id(struct db8500_dev *dev)
> > {
> >       u32 __iomem *uid = dev->base + U8500_BB_UID_OFFSET;
> >       snprintf(dev->soc_id, sizeof (dev->soc_id), "%08x%08x%08x%08x%08x",
> >               readl(uid[0]), readl(uid[1]), readl(uid[2]), readl(uid[3]), readl(uid[4]));
> > }
> 
> No problem.
> 
> Where did you get dev->base from though?

In this example, db8500_dev would be derived from soc_device. and
get initialized at the time you register it to the soc subsystem.
You clearly have to pass in the base address at some point, so if
there is no device tree, that init function should statically set it,
but you would still avoid spreading the use of these constants all
over the code.

> > The style you use here is preexisting in the db8500 code, but you should
> > not keep adding more of that crap. 
> 
> Not meaning to pass the buck at all, but this isn't my code.
> 
> It's remnant from when I took this over (starting to wish I hadn't ;))

I feel your pain. Sorry that you got dragged in this so deeply when
you only wanted a place to add a tiny bit of information.

> > All the code like 
> > #define db8500_add_i2c0(pdata) \
> >         dbx500_add_i2c(0, U8500_I2C0_BASE, IRQ_DB8500_I2C0, pdata)
> > #define db8500_add_i2c1(pdata) \
> >         dbx500_add_i2c(1, U8500_I2C1_BASE, IRQ_DB8500_I2C1, pdata)
> > 
> > should never really have been there. What you want to do for this is
> > to call this from the code that initializes the db8500 controller, and
> > pass the board specific pdata into the db8500 init function, along
> > with the i2c client data.
> 
> Not my domain. Speak to Linus W. :)

Yes, I understand. I was mostly using this as explanation about where
the problem lies. I'm not expecting you to fix all of the problems
of the platform, but in order to add new code the right way, we have
to agree on how it should have been there to start with.

	Arnd

[PATCH 1/5] Framework for exporting System-on-Chip information via sysfs

[Greg KH]

On Thu, Sep 01, 2011 at 01:27:19PM +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 device, 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      |  103 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/sys_soc.h |   27 ++++++++++++
>  4 files changed, 134 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 21cf46f..95f10c5 100644
> --- a/drivers/base/Kconfig
> +++ b/drivers/base/Kconfig
> @@ -172,6 +172,9 @@ config SYS_HYPERVISOR
>  	bool
>  	default n
>  
> +config SYS_SOC
> +	bool
> +
>  source "drivers/base/regmap/Kconfig"
>  
>  endmenu
> diff --git a/drivers/base/Makefile b/drivers/base/Makefile
> index 99a375a..a67a1e7 100644
> --- a/drivers/base/Makefile
> +++ b/drivers/base/Makefile
> @@ -18,6 +18,7 @@ obj-$(CONFIG_MODULES)	+= module.o
>  endif
>  obj-$(CONFIG_SYS_HYPERVISOR) += hypervisor.o
>  obj-$(CONFIG_REGMAP)	+= regmap/
> +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..e9d908b
> --- /dev/null
> +++ b/drivers/base/soc.c
> @@ -0,0 +1,103 @@
> +/*
> + * 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);

If this is really needed, please make it a mutex as you end up calling
code paths that can sleep.

> +static int soc_count = 0;

This should not be needed.

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

NEVER create a static struct device, this is totally not needed and
completly wrong.

> +static ssize_t soc_info_get(struct device *dev,
> +			struct device_attribute *attr,
> +			char *buf)
> +{
> +	struct soc_device *soc_dev = container_of(dev, struct soc_device, dev);
> +
> +	if (!strcmp(attr->attr.name, "machine"))
> +		return sprintf(buf, "%s\n", soc_dev->machine);
> +	if (!strcmp(attr->attr.name, "family"))
> +		return sprintf(buf, "%s\n", soc_dev->family);
> +	if (!strcmp(attr->attr.name, "revision"))
> +		return sprintf(buf, "%s\n", soc_dev->revision);
> +	if (!strcmp(attr->attr.name, "soc_id")) {
> +		if (soc_dev->pfn_soc_id)
> +			return sprintf(buf, "%s\n", soc_dev->pfn_soc_id());
> +		else return sprintf(buf, "N/A \n");

Move this line

> +	}
> +
> +	return -EINVAL;

To here?


> +}
> +
> +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 soc_device_remove_files(struct device *soc, int i)
> +{
> +	while (i > 0)
> +		device_remove_file(soc, &soc_attrs[--i]);
> +}

You do know how to add multiple files, right?  Oh wait, this is all not
the way to do this in the first place.  You just raced userspace :(

> +
> +static int __init soc_device_create_files(struct device *dev)
> +{
> +	int ret = 0;
> +	int i   = 0;
> +
> +	while (soc_attrs[i].attr.name != NULL) {
> +		ret = device_create_file(dev, &soc_attrs[i++]);
> +		if (ret)
> +			goto out;
> +	}
> +	return ret;
> +
> +out:
> +	soc_device_remove_files(dev, --i);
> +	return ret;
> +}

Yup, you raced userspace.  Please use the proper api for defining a
group of attributes for a device by default so that the driver core
handles creating and destroying them correctly and in a way that will
not be racy (unlike this way.)

> +
> +int __init soc_device_register(struct device *dev)

Don't pass a struct device in here, why are you making the caller create
the device?  This is the function that should create it for you?

> +{
> +	struct soc_device *soc_dev = container_of(dev, struct soc_device, dev);
> +	int ret;
> +
> +	spin_lock_irq(&register_lock);

Ok.

> +
> +	if (!soc_count) {
> +		/* Register top-level SoC device '/sys/devices/soc' */
> +		ret = device_register(&soc_grandparent);

device_register can't be called with a spinlock.  You must have gotten
lucky in your testing, if you tested this.

Anyway, this is not needed at all, please don't create "dummy" devices
in the sysfs tree, properly hook up your new device to where it should
be in the device tree.

> +		if (ret) {
> +			spin_unlock_irq(&register_lock);
> +			return ret;
> +		}
> +	}
> +
> +	soc_count++;

Nope, don't use this, again, use the correct kernel interface for
providing you with a unique number that increments as needed.  Don't
roll your own that will end up being wrong in the end (like this is,
what happens if you remove a device in the middle?)

> +	dev->parent = &soc_grandparent;

Nope, don't do that.

> +	dev_set_name(dev, "%i_%s", soc_count, soc_dev->machine);
> +
> +	spin_unlock_irq(&register_lock);
> +
> +	ret = device_register(dev);
> +	if (ret)
> +		return ret;
> +
> +	soc_device_create_files(dev);

Nor that.

See above for why.

> +
> +	return ret;
> +}
> diff --git a/include/linux/sys_soc.h b/include/linux/sys_soc.h
> new file mode 100644
> index 0000000..811d7fe
> --- /dev/null
> +++ b/include/linux/sys_soc.h
> @@ -0,0 +1,27 @@
> +/*
> + * 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>
> +
> +struct soc_device {
> +	struct device dev;
> +	const char *machine;
> +	const char *family;
> +	const char *revision;
> +	const char *(*pfn_soc_id)(void);
> +};

Nice structure, but why do you export it, you aren't using it anywhere,
nor should you be...

> +
> +/**
> + * soc_device_register - register SoC as a device
> + * @dev: Parent node '/sys/devices/soc/X'
> + */
> +int soc_device_register(struct device *dev);

This whole api needs to be rethunk, please, it's really wrong.

What you want is a way to create a soc device, by calling a function,
not be responsible for creating it, then call the soc core, and then
somehow, removing it.

Oh wait, you forgot to have a function to remove anything created with
the above call, that seems really broken and wrong.

greg k-h

[PATCH 1/5] Framework for exporting System-on-Chip information via sysfs

[Lee Jones]

Comments and questions in-line.

On 02/09/11 00:34, Greg KH wrote:
> On Thu, Sep 01, 2011 at 01:27:19PM +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 device, 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      |  103 +++++++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/sys_soc.h |   27 ++++++++++++
>>  4 files changed, 134 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 21cf46f..95f10c5 100644
>> --- a/drivers/base/Kconfig
>> +++ b/drivers/base/Kconfig
>> @@ -172,6 +172,9 @@ config SYS_HYPERVISOR
>>  	bool
>>  	default n
>>  
>> +config SYS_SOC
>> +	bool
>> +
>>  source "drivers/base/regmap/Kconfig"
>>  
>>  endmenu
>> diff --git a/drivers/base/Makefile b/drivers/base/Makefile
>> index 99a375a..a67a1e7 100644
>> --- a/drivers/base/Makefile
>> +++ b/drivers/base/Makefile
>> @@ -18,6 +18,7 @@ obj-$(CONFIG_MODULES)	+= module.o
>>  endif
>>  obj-$(CONFIG_SYS_HYPERVISOR) += hypervisor.o
>>  obj-$(CONFIG_REGMAP)	+= regmap/
>> +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..e9d908b
>> --- /dev/null
>> +++ b/drivers/base/soc.c
>> @@ -0,0 +1,103 @@
>> +/*
>> + * 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);
> 
> If this is really needed, please make it a mutex as you end up calling
> code paths that can sleep.

If I use:

"the correct kernel interface for providing you with a unique number
that increments as needed"

I'm I correct in assuming that I don't need to use locking?

>> +static int soc_count = 0;
> 
> This should not be needed.
> 
>> +
>> +static struct device soc_grandparent = {
>> +	.init_name = "soc",
>> +};
> 
> NEVER create a static struct device, this is totally not needed and
> completly wrong.

Noted on the static point, but I believe that it is needed.

That's how /sys/devices/platform does it, which this essentially replaces.

>> +static ssize_t soc_info_get(struct device *dev,
>> +			struct device_attribute *attr,
>> +			char *buf)
>> +{
>> +	struct soc_device *soc_dev = container_of(dev, struct soc_device, dev);
>> +
>> +	if (!strcmp(attr->attr.name, "machine"))
>> +		return sprintf(buf, "%s\n", soc_dev->machine);
>> +	if (!strcmp(attr->attr.name, "family"))
>> +		return sprintf(buf, "%s\n", soc_dev->family);
>> +	if (!strcmp(attr->attr.name, "revision"))
>> +		return sprintf(buf, "%s\n", soc_dev->revision);
>> +	if (!strcmp(attr->attr.name, "soc_id")) {
>> +		if (soc_dev->pfn_soc_id)
>> +			return sprintf(buf, "%s\n", soc_dev->pfn_soc_id());
>> +		else return sprintf(buf, "N/A \n");
> 
> Move this line
> 
>> +	}
>> +
>> +	return -EINVAL;
> 
> To here?
> 

I initially had invalid requests return a useful message, but I was told
to remove it and return -EINVAL instead:

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

Jamie, what say you?

>> +}
>> +
>> +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 soc_device_remove_files(struct device *soc, int i)
>> +{
>> +	while (i > 0)
>> +		device_remove_file(soc, &soc_attrs[--i]);
>> +}
> 
> You do know how to add multiple files, right?  Oh wait, this is all not
> the way to do this in the first place.  You just raced userspace :(

I didn't, but I will soon.

>> +
>> +static int __init soc_device_create_files(struct device *dev)
>> +{
>> +	int ret = 0;
>> +	int i   = 0;
>> +
>> +	while (soc_attrs[i].attr.name != NULL) {
>> +		ret = device_create_file(dev, &soc_attrs[i++]);
>> +		if (ret)
>> +			goto out;
>> +	}
>> +	return ret;
>> +
>> +out:
>> +	soc_device_remove_files(dev, --i);
>> +	return ret;
>> +}
> 
> Yup, you raced userspace.  Please use the proper api for defining a
> group of attributes for a device by default so that the driver core
> handles creating and destroying them correctly and in a way that will
> not be racy (unlike this way.)

No problem. I'll look it up and make the changes.

>> +
>> +int __init soc_device_register(struct device *dev)
> 
> Don't pass a struct device in here, why are you making the caller create
> the device?  This is the function that should create it for you?

I guess I can just return it instead, so it will become:

struct device * __init soc_device_register(void)

Is that more in keeping with what you would expect to see?

>> +{
>> +	struct soc_device *soc_dev = container_of(dev, struct soc_device, dev);
>> +	int ret;
>> +
>> +	spin_lock_irq(&register_lock);
> 
> Ok.
> 
>> +
>> +	if (!soc_count) {
>> +		/* Register top-level SoC device '/sys/devices/soc' */
>> +		ret = device_register(&soc_grandparent);
> 
> device_register can't be called with a spinlock.  You must have gotten
> lucky in your testing, if you tested this.
> 
> Anyway, this is not needed at all, please don't create "dummy" devices
> in the sysfs tree, properly hook up your new device to where it should
> be in the device tree.

We need a /sys/devices/soc, how else can this be done?

Would it make you happier if I called it a bus?

>> +		if (ret) {
>> +			spin_unlock_irq(&register_lock);
>> +			return ret;
>> +		}
>> +	}
>> +
>> +	soc_count++;
> 
> Nope, don't use this, again, use the correct kernel interface for
> providing you with a unique number that increments as needed.  Don't
> roll your own that will end up being wrong in the end (like this is,
> what happens if you remove a device in the middle?)

Noted.

>> +	dev->parent = &soc_grandparent;
> 
> Nope, don't do that.

As above.

>> +	dev_set_name(dev, "%i_%s", soc_count, soc_dev->machine);
>> +
>> +	spin_unlock_irq(&register_lock);
>> +
>> +	ret = device_register(dev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	soc_device_create_files(dev);
> 
> Nor that.
> 
> See above for why.
> 
>> +
>> +	return ret;
>> +}
>> diff --git a/include/linux/sys_soc.h b/include/linux/sys_soc.h
>> new file mode 100644
>> index 0000000..811d7fe
>> --- /dev/null
>> +++ b/include/linux/sys_soc.h
>> @@ -0,0 +1,27 @@
>> +/*
>> + * 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>
>> +
>> +struct soc_device {
>> +	struct device dev;
>> +	const char *machine;
>> +	const char *family;
>> +	const char *revision;
>> +	const char *(*pfn_soc_id)(void);
>> +};
> 
> Nice structure, but why do you export it, you aren't using it anywhere,
> nor should you be...

Okay, I'll use the returning struct device * from soc_device_register()
register instead.

>> +
>> +/**
>> + * soc_device_register - register SoC as a device
>> + * @dev: Parent node '/sys/devices/soc/X'
>> + */
>> +int soc_device_register(struct device *dev);
> 
> This whole api needs to be rethunk, please, it's really wrong.
> 
> What you want is a way to create a soc device, by calling a function,
> not be responsible for creating it, then call the soc core, and then
> somehow, removing it.
> 
> Oh wait, you forgot to have a function to remove anything created with
> the above call, that seems really broken and wrong.

Again, I did have an unregister function, but I was told to remove it.

"The exit function does not make any sense if you cannot build the soc
support itself as a module, which in turn makes no sense at all."

What if I put it back and removed the __exit syntax and used it as a
standard unregister/free function? Would that be more to everyone's taste?

Thanks for your time.

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

[PATCH 1/5] Framework for exporting System-on-Chip information via sysfs

[Jamie Iles]

On Fri, Sep 02, 2011 at 09:44:52AM +0100, Lee Jones wrote:
> On 02/09/11 00:34, Greg KH wrote:
> > On Thu, Sep 01, 2011 at 01:27:19PM +0100, Lee Jones wrote:
> >> +static ssize_t soc_info_get(struct device *dev,
> >> +			struct device_attribute *attr,
> >> +			char *buf)
> >> +{
> >> +	struct soc_device *soc_dev = container_of(dev, struct soc_device, dev);
> >> +
> >> +	if (!strcmp(attr->attr.name, "machine"))
> >> +		return sprintf(buf, "%s\n", soc_dev->machine);
> >> +	if (!strcmp(attr->attr.name, "family"))
> >> +		return sprintf(buf, "%s\n", soc_dev->family);
> >> +	if (!strcmp(attr->attr.name, "revision"))
> >> +		return sprintf(buf, "%s\n", soc_dev->revision);
> >> +	if (!strcmp(attr->attr.name, "soc_id")) {
> >> +		if (soc_dev->pfn_soc_id)
> >> +			return sprintf(buf, "%s\n", soc_dev->pfn_soc_id());
> >> +		else return sprintf(buf, "N/A \n");
> > 
> > Move this line
> > 
> >> +	}
> >> +
> >> +	return -EINVAL;
> > 
> > To here?
> > 
> 
> I initially had invalid requests return a useful message, but I was told
> to remove it and return -EINVAL instead:
> 
> "Just return -EINVAL or similar here to propogate the error to the user."
> 
> Jamie, what say you?

Well if there isn't an ID for the SoC, then I think it's better for a 
read() to return an error rather than have to parse for a special string 
like "N/A ".  So rather than returning that string, perhaps something 
like:

	if (!strcmp(attr->attr.name, "soc_id")) {
		if (!soc_dev->pfn_soc_id)
			return -ENOENT;
		return sprintf(buf, "%s\n", soc_dev->pfn_soc_id());
	}

or if you don't think this is an error, default to an id of "0" so that 
it can be parsed by tools?

Jamie

[PATCH 1/5] Framework for exporting System-on-Chip information via sysfs

[Lee Jones]

On 02/09/11 10:29, Jamie Iles wrote:
> On Fri, Sep 02, 2011 at 09:44:52AM +0100, Lee Jones wrote:
>> On 02/09/11 00:34, Greg KH wrote:
>>> On Thu, Sep 01, 2011 at 01:27:19PM +0100, Lee Jones wrote:
>>>> +static ssize_t soc_info_get(struct device *dev,
>>>> +			struct device_attribute *attr,
>>>> +			char *buf)
>>>> +{
>>>> +	struct soc_device *soc_dev = container_of(dev, struct soc_device, dev);
>>>> +
>>>> +	if (!strcmp(attr->attr.name, "machine"))
>>>> +		return sprintf(buf, "%s\n", soc_dev->machine);
>>>> +	if (!strcmp(attr->attr.name, "family"))
>>>> +		return sprintf(buf, "%s\n", soc_dev->family);
>>>> +	if (!strcmp(attr->attr.name, "revision"))
>>>> +		return sprintf(buf, "%s\n", soc_dev->revision);
>>>> +	if (!strcmp(attr->attr.name, "soc_id")) {
>>>> +		if (soc_dev->pfn_soc_id)
>>>> +			return sprintf(buf, "%s\n", soc_dev->pfn_soc_id());
>>>> +		else return sprintf(buf, "N/A \n");
>>>
>>> Move this line
>>>
>>>> +	}
>>>> +
>>>> +	return -EINVAL;
>>>
>>> To here?
>>>
>>
>> I initially had invalid requests return a useful message, but I was told
>> to remove it and return -EINVAL instead:
>>
>> "Just return -EINVAL or similar here to propogate the error to the user."
>>
>> Jamie, what say you?
> 
> Well if there isn't an ID for the SoC, then I think it's better for a 
> read() to return an error rather than have to parse for a special string 
> like "N/A ".  So rather than returning that string, perhaps something 
> like:
> 
> 	if (!strcmp(attr->attr.name, "soc_id")) {
> 		if (!soc_dev->pfn_soc_id)
> 			return -ENOENT;
> 		return sprintf(buf, "%s\n", soc_dev->pfn_soc_id());
> 	}
> 
> or if you don't think this is an error, default to an id of "0" so that 
> it can be parsed by tools?

What about at the very end of the function?

What should we return if an inappropriate attr->attr.name is used?

-- 
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

[PATCH 1/5] Framework for exporting System-on-Chip information via sysfs

[Jamie Iles]

On Fri, Sep 02, 2011 at 10:37:47AM +0100, Lee Jones wrote:
> On 02/09/11 10:29, Jamie Iles wrote:
> > On Fri, Sep 02, 2011 at 09:44:52AM +0100, Lee Jones wrote:
> >> On 02/09/11 00:34, Greg KH wrote:
> >>> On Thu, Sep 01, 2011 at 01:27:19PM +0100, Lee Jones wrote:
> >>>> +static ssize_t soc_info_get(struct device *dev,
> >>>> +			struct device_attribute *attr,
> >>>> +			char *buf)
> >>>> +{
> >>>> +	struct soc_device *soc_dev = container_of(dev, struct soc_device, dev);
> >>>> +
> >>>> +	if (!strcmp(attr->attr.name, "machine"))
> >>>> +		return sprintf(buf, "%s\n", soc_dev->machine);
> >>>> +	if (!strcmp(attr->attr.name, "family"))
> >>>> +		return sprintf(buf, "%s\n", soc_dev->family);
> >>>> +	if (!strcmp(attr->attr.name, "revision"))
> >>>> +		return sprintf(buf, "%s\n", soc_dev->revision);
> >>>> +	if (!strcmp(attr->attr.name, "soc_id")) {
> >>>> +		if (soc_dev->pfn_soc_id)
> >>>> +			return sprintf(buf, "%s\n", soc_dev->pfn_soc_id());
> >>>> +		else return sprintf(buf, "N/A \n");
> >>>
> >>> Move this line
> >>>
> >>>> +	}
> >>>> +
> >>>> +	return -EINVAL;
> >>>
> >>> To here?
> >>>
> >>
> >> I initially had invalid requests return a useful message, but I was told
> >> to remove it and return -EINVAL instead:
> >>
> >> "Just return -EINVAL or similar here to propogate the error to the user."
> >>
> >> Jamie, what say you?
> > 
> > Well if there isn't an ID for the SoC, then I think it's better for a 
> > read() to return an error rather than have to parse for a special string 
> > like "N/A ".  So rather than returning that string, perhaps something 
> > like:
> > 
> > 	if (!strcmp(attr->attr.name, "soc_id")) {
> > 		if (!soc_dev->pfn_soc_id)
> > 			return -ENOENT;
> > 		return sprintf(buf, "%s\n", soc_dev->pfn_soc_id());
> > 	}
> > 
> > or if you don't think this is an error, default to an id of "0" so that 
> > it can be parsed by tools?
> 
> What about at the very end of the function?
> 
> What should we return if an inappropriate attr->attr.name is used?

That shouldn't happen as it is only ever registered with valid 
attributes.  That said, you do need to return something so -EINVAL seems 
like a reasonable choice.

Jamie

[PATCH 1/5] Framework for exporting System-on-Chip information via sysfs

[Greg KH]

On Fri, Sep 02, 2011 at 09:44:52AM +0100, Lee Jones wrote:
> Comments and questions in-line.

As they should be :)

> On 02/09/11 00:34, Greg KH wrote:
> > On Thu, Sep 01, 2011 at 01:27:19PM +0100, Lee Jones wrote:
> >> +static DEFINE_SPINLOCK(register_lock);
> > 
> > If this is really needed, please make it a mutex as you end up calling
> > code paths that can sleep.
> 
> If I use:
> 
> "the correct kernel interface for providing you with a unique number
> that increments as needed"
> 
> I'm I correct in assuming that I don't need to use locking?

Yes, don't you agree?

You do know what this interface is, right?  :)

> >> +static int soc_count = 0;
> > 
> > This should not be needed.
> > 
> >> +
> >> +static struct device soc_grandparent = {
> >> +	.init_name = "soc",
> >> +};
> > 
> > NEVER create a static struct device, this is totally not needed and
> > completly wrong.
> 
> Noted on the static point, but I believe that it is needed.
> 
> That's how /sys/devices/platform does it, which this essentially replaces.

NO NO NO NO NO NO

Again, no.

This does NOT replace /sys/devices/platform in any shape, manner, or
form.

Platform devices are "special" in that they are the "grab-bag of devices
that no one seems to know how to hook up to a sane bus so we throw them
all in the same heap and try to ignore the ugliness of them."

Never repeat the platform code anywhere else, that's not acceptable at
all.

If you want to make this a "bus" that's probably best, as then you would
get to enumerate over all /sys/bus/soc/ devices just fine, no matter
where in the device tree they live.

> >> +static ssize_t soc_info_get(struct device *dev,
> >> +			struct device_attribute *attr,
> >> +			char *buf)
> >> +{
> >> +	struct soc_device *soc_dev = container_of(dev, struct soc_device, dev);
> >> +
> >> +	if (!strcmp(attr->attr.name, "machine"))
> >> +		return sprintf(buf, "%s\n", soc_dev->machine);
> >> +	if (!strcmp(attr->attr.name, "family"))
> >> +		return sprintf(buf, "%s\n", soc_dev->family);
> >> +	if (!strcmp(attr->attr.name, "revision"))
> >> +		return sprintf(buf, "%s\n", soc_dev->revision);
> >> +	if (!strcmp(attr->attr.name, "soc_id")) {
> >> +		if (soc_dev->pfn_soc_id)
> >> +			return sprintf(buf, "%s\n", soc_dev->pfn_soc_id());
> >> +		else return sprintf(buf, "N/A \n");
> > 
> > Move this line
> > 
> >> +	}
> >> +
> >> +	return -EINVAL;
> > 
> > To here?
> > 
> 
> I initially had invalid requests return a useful message, but I was told
> to remove it and return -EINVAL instead:
> 
> "Just return -EINVAL or similar here to propogate the error to the user."

No, if a SOC doesn't have one of these attributes, then just don't
create the sysfs file.  Don't return -EINVAL for something that you know
better than to have created in the first place, are you trying to make
it hard for userspace? :)

> >> +int __init soc_device_register(struct device *dev)
> > 
> > Don't pass a struct device in here, why are you making the caller create
> > the device?  This is the function that should create it for you?
> 
> I guess I can just return it instead, so it will become:
> 
> struct device * __init soc_device_register(void)
> 
> Is that more in keeping with what you would expect to see?

no.

How did you pass the attribute data into this core so that it knows how
to get to it?  You didn't, which is a mess.

How about something like:
	struct soc_device *soc_device_create(struct device *parent, struct soc_device_attributes *attr);

That would have the proper way to handle lifetime rules, userspace
notificatation of userpace, placement in sysfs, and all the other good
stuff.

I'm feeling like it would be simpler for me to write this code than to
keep providing review comments like this telling you to rewrite the
whole thing over again each time :)

Anyone want to send me a device that this code is needed for so I can do
it?

> >> +{
> >> +	struct soc_device *soc_dev = container_of(dev, struct soc_device, dev);
> >> +	int ret;
> >> +
> >> +	spin_lock_irq(&register_lock);
> > 
> > Ok.
> > 
> >> +
> >> +	if (!soc_count) {
> >> +		/* Register top-level SoC device '/sys/devices/soc' */
> >> +		ret = device_register(&soc_grandparent);
> > 
> > device_register can't be called with a spinlock.  You must have gotten
> > lucky in your testing, if you tested this.
> > 
> > Anyway, this is not needed at all, please don't create "dummy" devices
> > in the sysfs tree, properly hook up your new device to where it should
> > be in the device tree.
> 
> We need a /sys/devices/soc, how else can this be done?

No you do not not, why would you think so?

> Would it make you happier if I called it a bus?

Yes, see the other response about creating a bus for these, which would
give you /sys/bus/soc/ where you can properly enumerate your devices,
which is what I think you want to be able to do, right?

thanks,

greg k-h

[PATCH 1/5] Framework for exporting System-on-Chip information via sysfs

[Arnd Bergmann]

On Friday 02 September 2011, Greg KH wrote:
> no.
> 
> How did you pass the attribute data into this core so that it knows how
> to get to it?  You didn't, which is a mess.
> 
> How about something like:
>         struct soc_device *soc_device_create(struct device *parent, struct soc_device_attributes *attr);
> 
> That would have the proper way to handle lifetime rules, userspace
> notificatation of userpace, placement in sysfs, and all the other good
> stuff.

I early on objected to the individual soc driver providing a set of
attributes, because we really want the attributes to follow a
common interface and I want to make it *hard* to add arbitrary
other attributes that are not documented.

> > >> +
> > >> +  if (!soc_count) {
> > >> +          /* Register top-level SoC device '/sys/devices/soc' */
> > >> +          ret = device_register(&soc_grandparent);
> > > 
> > > device_register can't be called with a spinlock.  You must have gotten
> > > lucky in your testing, if you tested this.
> > > 
> > > Anyway, this is not needed at all, please don't create "dummy" devices
> > > in the sysfs tree, properly hook up your new device to where it should
> > > be in the device tree.
> > 
> > We need a /sys/devices/soc, how else can this be done?
> 
> No you do not not, why would you think so?

Well, where is all comes from is the desire to have a way from user space
to find these devices, based on some path in the device tree. We had
discussed putting it into

/sys/class/soc
/sys/bus/soc
/sys/devices/platform/soc
/sys/devices/platform/soc%d

and a few others and eventually settled on /sys/devices/soc.

> > Would it make you happier if I called it a bus?
> 
> Yes, see the other response about creating a bus for these, which would
> give you /sys/bus/soc/ where you can properly enumerate your devices,
> which is what I think you want to be able to do, right?

I think that would be a reasonable interface, but how does this work
when the device was created by of_platform_bus_probe? Should we
assume that any top-level device in the flattened device tree is
a soc? How about just adding a child device in each soc node that
is registered to the soc_bus_type?

That would end up looking like

/sys/devices/db8500
		   /soc0   # this is the soc device
		   /i2c-0  # and here are the other platform devices below db8500 
		   /spi-0
    /bus/soc/devices/soc0 -> ../../../devices/db8500/soc0
    /bus/platform/devices/i2c-0 -> ../../../devices/db8500/i2c-0

	Arnd

[PATCH 1/5] Framework for exporting System-on-Chip information via sysfs

[Greg KH]

On Fri, Sep 02, 2011 at 07:22:04PM +0200, Arnd Bergmann wrote:
> On Friday 02 September 2011, Greg KH wrote:
> > no.
> > 
> > How did you pass the attribute data into this core so that it knows how
> > to get to it?  You didn't, which is a mess.
> > 
> > How about something like:
> >         struct soc_device *soc_device_create(struct device *parent, struct soc_device_attributes *attr);
> > 
> > That would have the proper way to handle lifetime rules, userspace
> > notificatation of userpace, placement in sysfs, and all the other good
> > stuff.
> 
> I early on objected to the individual soc driver providing a set of
> attributes, because we really want the attributes to follow a
> common interface and I want to make it *hard* to add arbitrary
> other attributes that are not documented.

That's fine, which is why the struct soc_device_attributes are there,
those are common and well defined.

We can just do a simple return of 'int' to keep the caller from ever
having access to the device at all, which, if these can never be
removed, is a great way to keep anyone from messing with them.

So the interface gets even simpler:
	int soc_device_create(struct device *parent, struct soc_device_attributes *attr);

Look good?

> > > >> +
> > > >> +  if (!soc_count) {
> > > >> +          /* Register top-level SoC device '/sys/devices/soc' */
> > > >> +          ret = device_register(&soc_grandparent);
> > > > 
> > > > device_register can't be called with a spinlock.  You must have gotten
> > > > lucky in your testing, if you tested this.
> > > > 
> > > > Anyway, this is not needed at all, please don't create "dummy" devices
> > > > in the sysfs tree, properly hook up your new device to where it should
> > > > be in the device tree.
> > > 
> > > We need a /sys/devices/soc, how else can this be done?
> > 
> > No you do not not, why would you think so?
> 
> Well, where is all comes from is the desire to have a way from user space
> to find these devices, based on some path in the device tree. We had
> discussed putting it into
> 
> /sys/class/soc
> /sys/bus/soc
> /sys/devices/platform/soc
> /sys/devices/platform/soc%d
> 
> and a few others and eventually settled on /sys/devices/soc.

No, please use /sys/bus/soc as these are all of the same "type".

> > > Would it make you happier if I called it a bus?
> > 
> > Yes, see the other response about creating a bus for these, which would
> > give you /sys/bus/soc/ where you can properly enumerate your devices,
> > which is what I think you want to be able to do, right?
> 
> I think that would be a reasonable interface, but how does this work
> when the device was created by of_platform_bus_probe? Should we
> assume that any top-level device in the flattened device tree is
> a soc? How about just adding a child device in each soc node that
> is registered to the soc_bus_type?

Yes, that will work with the above soc_device_create() call.

> That would end up looking like
> 
> /sys/devices/db8500
> 		   /soc0   # this is the soc device
> 		   /i2c-0  # and here are the other platform devices below db8500 
> 		   /spi-0
>     /bus/soc/devices/soc0 -> ../../../devices/db8500/soc0
>     /bus/platform/devices/i2c-0 -> ../../../devices/db8500/i2c-0

Looks good to me.  Using a bus, and the above interface, will result in
this.

thanks,

greg k-h

[PATCH 1/5] Framework for exporting System-on-Chip information via sysfs

[Lee Jones]

I don't have a great deal of time at the moment, as I'm currently on the
train on my way to Plumbers, but I would like to say just one thing.

On 02/09/11 17:31, Greg KH wrote:
> I'm feeling like it would be simpler for me to write this code than to
> keep providing review comments like this telling you to rewrite the
> whole thing over again each time :)

I'd really rather you didn't.

I have put a great deal of my own time into this and I really want to
see it through. Up until now I have been finding it quite difficult to
please everyone. I have been trying to follow everyone's suggestions and
as soon as I think we're getting close, the world flips upside down.
When I return, I will sit down and go through all of your (and Arnd's)
suggestions and re-write it.

Please bear with me.

> Anyone want to send me a device that this code is needed for so I can do
> it?

No. ;)

Kind regards,
Lee

[PATCH 1/5] Framework for exporting System-on-Chip information via sysfs

[Arnd Bergmann]

On Tuesday 06 September 2011 10:41:11 Lee Jones wrote:
> I don't have a great deal of time at the moment, as I'm currently on the
> train on my way to Plumbers, but I would like to say just one thing.
> 
> On 02/09/11 17:31, Greg KH wrote:
> > I'm feeling like it would be simpler for me to write this code than to
> > keep providing review comments like this telling you to rewrite the
> > whole thing over again each time 
> 
> I'd really rather you didn't.
> 
> I have put a great deal of my own time into this and I really want to
> see it through. Up until now I have been finding it quite difficult to
> please everyone. I have been trying to follow everyone's suggestions and
> as soon as I think we're getting close, the world flips upside down.
> When I return, I will sit down and go through all of your (and Arnd's)
> suggestions and re-write it.

Ok. BTW, I feel a bit guilty for making you go through all this work
on code that was written by other people and you are not to blame for,
but I think we on the right track now. If you have any doubts about
how to go on with the next version, please contact on IRC or phone
so we can discuss the details. I think that will be more productive
than the past email exchanges.

	Arnd

[PATCH 1/5] Framework for exporting System-on-Chip information via sysfs

[Greg KH]

On Tue, Sep 06, 2011 at 10:41:11AM +0100, Lee Jones wrote:
> I don't have a great deal of time at the moment, as I'm currently on the
> train on my way to Plumbers, but I would like to say just one thing.
> 
> On 02/09/11 17:31, Greg KH wrote:
> > I'm feeling like it would be simpler for me to write this code than to
> > keep providing review comments like this telling you to rewrite the
> > whole thing over again each time :)
> 
> I'd really rather you didn't.
> 
> I have put a great deal of my own time into this and I really want to
> see it through. Up until now I have been finding it quite difficult to
> please everyone. I have been trying to follow everyone's suggestions and
> as soon as I think we're getting close, the world flips upside down.
> When I return, I will sit down and go through all of your (and Arnd's)
> suggestions and re-write it.

Ok, but expect a very tough code review if you do this. :)

greg k-h

[PATCH 1/5] Framework for exporting System-on-Chip information via sysfs

[Lee Jones]

On 06/09/11 20:45, Greg KH wrote:
> On Tue, Sep 06, 2011 at 10:41:11AM +0100, Lee Jones wrote:
>> I don't have a great deal of time at the moment, as I'm currently on the
>> train on my way to Plumbers, but I would like to say just one thing.
>>
>> On 02/09/11 17:31, Greg KH wrote:
>>> I'm feeling like it would be simpler for me to write this code than to
>>> keep providing review comments like this telling you to rewrite the
>>> whole thing over again each time :)
>>
>> I'd really rather you didn't.
>>
>> I have put a great deal of my own time into this and I really want to
>> see it through. Up until now I have been finding it quite difficult to
>> please everyone. I have been trying to follow everyone's suggestions and
>> as soon as I think we're getting close, the world flips upside down.
>> When I return, I will sit down and go through all of your (and Arnd's)
>> suggestions and re-write it.
> 
> Ok, but expect a very tough code review if you do this. :)

What's new? ;)

[PATCH 1/5] Framework for exporting System-on-Chip information via sysfs

[Arnd Bergmann]

On Friday 02 September 2011, Greg KH wrote:
> On Thu, Sep 01, 2011 at 01:27:19PM +0100, Lee Jones wrote:
> > +
> > +int __init soc_device_register(struct device *dev)
> 
> Don't pass a struct device in here, why are you making the caller create
> the device?  This is the function that should create it for you?

Well, when we have device tree based probing, the soc device might already
have been created by of_bus_probe(), so we just want to pass it to the
soc core code in order to create the common interfaces. 

It's probably not right that soc_device_register actually calls
device_register, because when you have a device that you pass in,
it will already be registered.

> > diff --git a/include/linux/sys_soc.h b/include/linux/sys_soc.h
> > new file mode 100644
> > index 0000000..811d7fe
> > --- /dev/null
> > +++ b/include/linux/sys_soc.h
> > @@ -0,0 +1,27 @@
> > +/*
> > + * 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>
> > +
> > +struct soc_device {
> > +	struct device dev;
> > +	const char *machine;
> > +	const char *family;
> > +	const char *revision;
> > +	const char *(*pfn_soc_id)(void);
> > +};
> 
> Nice structure, but why do you export it, you aren't using it anywhere,
> nor should you be...

This gets filled by the soc-specific code with information that is exported
through the common callbacks, so it needs to be visible to the platforms.

> > +
> > +/**
> > + * soc_device_register - register SoC as a device
> > + * @dev: Parent node '/sys/devices/soc/X'
> > + */
> > +int soc_device_register(struct device *dev);
> 
> This whole api needs to be rethunk, please, it's really wrong.
> 
> What you want is a way to create a soc device, by calling a function,
> not be responsible for creating it, then call the soc core, and then
> somehow, removing it.

Yes, it's definitely still not where it ought to be, and we've been
discussing this for ages. It all started out as a way to find out
information about the system, and I suggested to better make that
information part of the interface for the soc node that is required
anyway, rather than making up a purely fictional device.

Now, we need this to cover two scenarios:

1. The classic setup without device tree where the soc node is
   created by platform code. This would work great by just calling
   a function that creates the device and lets the soc code add
   the children according to compile-time tables of child devices.

2. Probing from OF device-tree, where the linux device tree is populated
   by walking the devices found in the flat device tree description.
   This doesn't really fit your suggestion, nor does it fit Lee's
   patch really, because it would require reparenting the soc node
   into /sys/devices/soc and registering it again after it's already
   registered.

I think the ideal solution would be to make the soc layer a
class that can be set when initially adding the device (one way
or another) and that adds the standard attributes. Since you don't
really like classes any more, another option would be to just
have a function call that adds an attribute group to the soc node,
but then it's not easy to find which devices are actually soc
root nodes.

> Oh wait, you forgot to have a function to remove anything created with
> the above call, that seems really broken and wrong.

There is no way to remove a soc from the system, just like there
is no complementary function to pcibios_scan_root that removes
the PCI root bridge.

	Arnd

[PATCH 1/5] Framework for exporting System-on-Chip information via sysfs

[Greg KH]

On Fri, Sep 02, 2011 at 04:02:14PM +0200, Arnd Bergmann wrote:
> On Friday 02 September 2011, Greg KH wrote:
> > On Thu, Sep 01, 2011 at 01:27:19PM +0100, Lee Jones wrote:
> > > +
> > > +int __init soc_device_register(struct device *dev)
> > 
> > Don't pass a struct device in here, why are you making the caller create
> > the device?  This is the function that should create it for you?
> 
> Well, when we have device tree based probing, the soc device might already
> have been created by of_bus_probe(), so we just want to pass it to the
> soc core code in order to create the common interfaces. 

No, you want to pass the PARENT pointer in here that this new SOC device
will hang off of.  You can't expect to pass in any old random struct
device and expect it to make any kind of sense at all.

> It's probably not right that soc_device_register actually calls
> device_register, because when you have a device that you pass in,
> it will already be registered.

And where would it be registered?  No, please don't do this, again, have
this code create the struct device because it is responsible for it.
That's the way the rest of the kernel works, please don't try to create
new models to confuse everyone even more than is needed :)

> > > diff --git a/include/linux/sys_soc.h b/include/linux/sys_soc.h
> > > new file mode 100644
> > > index 0000000..811d7fe
> > > --- /dev/null
> > > +++ b/include/linux/sys_soc.h
> > > @@ -0,0 +1,27 @@
> > > +/*
> > > + * 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>
> > > +
> > > +struct soc_device {
> > > +	struct device dev;
> > > +	const char *machine;
> > > +	const char *family;
> > > +	const char *revision;
> > > +	const char *(*pfn_soc_id)(void);
> > > +};
> > 
> > Nice structure, but why do you export it, you aren't using it anywhere,
> > nor should you be...
> 
> This gets filled by the soc-specific code with information that is exported
> through the common callbacks, so it needs to be visible to the platforms.

But this api doesn't allow that to happen, so use it to create the data
in the first place somehow, this is really confusing as-is.

> > > +
> > > +/**
> > > + * soc_device_register - register SoC as a device
> > > + * @dev: Parent node '/sys/devices/soc/X'
> > > + */
> > > +int soc_device_register(struct device *dev);
> > 
> > This whole api needs to be rethunk, please, it's really wrong.
> > 
> > What you want is a way to create a soc device, by calling a function,
> > not be responsible for creating it, then call the soc core, and then
> > somehow, removing it.
> 
> Yes, it's definitely still not where it ought to be, and we've been
> discussing this for ages. It all started out as a way to find out
> information about the system, and I suggested to better make that
> information part of the interface for the soc node that is required
> anyway, rather than making up a purely fictional device.
> 
> Now, we need this to cover two scenarios:
> 
> 1. The classic setup without device tree where the soc node is
>    created by platform code. This would work great by just calling
>    a function that creates the device and lets the soc code add
>    the children according to compile-time tables of child devices.
> 
> 2. Probing from OF device-tree, where the linux device tree is populated
>    by walking the devices found in the flat device tree description.
>    This doesn't really fit your suggestion, nor does it fit Lee's
>    patch really, because it would require reparenting the soc node
>    into /sys/devices/soc and registering it again after it's already
>    registered.

No it would not, both of these work just fine like we handle with all
other subsystems.  You pass in the parent, and if it is not present, the
device will show up under /sys/devices/virtual/ just fine.  Please don't
create new type of things here.

> I think the ideal solution would be to make the soc layer a
> class that can be set when initially adding the device (one way
> or another) and that adds the standard attributes. Since you don't
> really like classes any more, another option would be to just
> have a function call that adds an attribute group to the soc node,
> but then it's not easy to find which devices are actually soc
> root nodes.

Then make it a bus, like it should be, which would solve all of this,
right?

> > Oh wait, you forgot to have a function to remove anything created with
> > the above call, that seems really broken and wrong.
> 
> There is no way to remove a soc from the system, just like there
> is no complementary function to pcibios_scan_root that removes
> the PCI root bridge.

And no one is ever going to remove the module that created the SOC
device in the first place?  It just really bothers me to see creation
code but no removal code, as it feels wrong somehow :)

thanks,

greg k-h

[PATCH 1/5] Framework for exporting System-on-Chip information via sysfs

[Arnd Bergmann]

On Friday 02 September 2011, Greg KH wrote:
> On Fri, Sep 02, 2011 at 04:02:14PM +0200, Arnd Bergmann wrote:
> > On Friday 02 September 2011, Greg KH wrote:
> > > > diff --git a/include/linux/sys_soc.h b/include/linux/sys_soc.h
> > > > new file mode 100644
> > > > index 0000000..811d7fe
> > > > --- /dev/null
> > > > +++ b/include/linux/sys_soc.h
> > > > @@ -0,0 +1,27 @@
> > > > +/*
> > > > + * 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>
> > > > +
> > > > +struct soc_device {
> > > > +	struct device dev;
> > > > +	const char *machine;
> > > > +	const char *family;
> > > > +	const char *revision;
> > > > +	const char *(*pfn_soc_id)(void);
> > > > +};
> > > 
> > > Nice structure, but why do you export it, you aren't using it anywhere,
> > > nor should you be...
> > 
> > This gets filled by the soc-specific code with information that is exported
> > through the common callbacks, so it needs to be visible to the platforms.
> 
> But this api doesn't allow that to happen, so use it to create the data
> in the first place somehow, this is really confusing as-is.

Yes, that is true. I hadn't noticed the 'struct device' in the above
structure. What really needs to happen is the platform code to fill
out a structure with all the attribute data and to pass that into
the soc common code as constant data which then gets added to the
device data.

> > I think the ideal solution would be to make the soc layer a
> > class that can be set when initially adding the device (one way
> > or another) and that adds the standard attributes. Since you don't
> > really like classes any more, another option would be to just
> > have a function call that adds an attribute group to the soc node,
> > but then it's not easy to find which devices are actually soc
> > root nodes.
> 
> Then make it a bus, like it should be, which would solve all of this,
> right?

I'm not sure if we're talking about the same thing. Do you mean
adding an artifical device (like a class_device) under the soc
device node and have that be of soc_bus type? Or do you mean
putting the entire soc with all its platform_device children
on the soc_bus?

In the former case, it's a somewhat ugly representation. It
would be really nice for the soc to be just one device, not
one device to hang all the children off and another one to
provide a user-space interface.

In the latter case, we would have to rewrite the way we probe
SoCs from the device tree so that the root node of the soc
becomes a soc_bus device instead of a platform_bus device.

> > > Oh wait, you forgot to have a function to remove anything created with
> > > the above call, that seems really broken and wrong.
> > 
> > There is no way to remove a soc from the system, just like there
> > is no complementary function to pcibios_scan_root that removes
> > the PCI root bridge.
> 
> And no one is ever going to remove the module that created the SOC
> device in the first place?  It just really bothers me to see creation
> code but no removal code, as it feels wrong somehow :)

SoC code fundamentally cannot be a module, since the soc contains
all the basic stuff like irq controller, clocksource etc. If it didn't
have those, it would not be a system-on-a-chip but rather some random
multifunction device you hang off some bus.

	Arnd