[RFC PATCHv1 0/2] Export SoC info through sysfs

[RFC PATCHv1 0/2] Export SoC info through sysfs

[Maxime Coquelin]

Here is the first version of the proposal to export SoC related information to user-space through sysFS interface.

This serie is to continue what has been discussed on the "socinfo" thread created by Eduardo Valentin:
https://lkml.org/lkml/2010/5/11/364

The first patch introduces the common part, which provides an interface to the platform to register its name, and exports platform-defined IDs to user-space.
The IDs strings can be provided in two ways: either with a pointer to the string, or by a callback returning the string.

The second patch is given as example for the ux500 architecture. It registers with the machine name "DB8500", and exports 3 informations (SoC unique ID, sili
con revision and silicon process).

Here is the output for DB8500:
root at ME:/sys/devices/system/soc ls -l
-r--r--r--    1 root     root         4096 Jan 11 02:43 mach_name
-r--r--r--    1 root     root         4096 Jan 11 02:43 process
-r--r--r--    1 root     root         4096 Jan 11 02:43 revision
-r--r--r--    1 root     root         4096 Jan 11 02:43 soc_id
root at ME:/sys/devices/system/soc cat mach_name
DB8500
root at ME:/sys/devices/system/soc cat process
Standard
root at ME:/sys/devices/system/soc cat revision
2.0
root at ME:/sys/devices/system/soc cat soc_id
2ba07ce9e5835d6185321e9577462ef2ea2129cf

Any comments are welcome.

Note that I will be off from 13th to 21th of March.

Regards,
Maxime

-------------------------------------------------------------------------------

Maxime Coquelin (2):
  Export SoC info through sysfs
  ux500: Export U8500 SoC info through sysfs

 arch/arm/mach-ux500/id.c |   96 ++++++++++++++++++++++++++++++++++++++++++++++
 drivers/base/Kconfig     |    4 ++
 drivers/base/Makefile    |    1 +
 drivers/base/soc.c       |   88 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/sys_soc.h  |   33 ++++++++++++++++
 5 files changed, 222 insertions(+), 0 deletions(-)
 create mode 100644 drivers/base/soc.c
 create mode 100644 include/linux/sys_soc.h

[RFC PATCHv1 1/2] Export SoC info through sysfs

[Maxime Coquelin]

Common base to export System-on-Chip related informations through sysfs.

Creation of a "soc" directory under /sys/devices/system/.
Creation of a common "mach_name" entry to export machine name.
Creation of platform-defined SoC information entries.

Signed-off-by: Maxime COQUELIN <maxime.coquelin-nonst@stericsson.com>
---
 drivers/base/Kconfig    |    4 ++
 drivers/base/Makefile   |    1 +
 drivers/base/soc.c      |   88 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/sys_soc.h |   33 +++++++++++++++++
 4 files changed, 126 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..4f2b56d 100644
--- a/drivers/base/Kconfig
+++ b/drivers/base/Kconfig
@@ -168,4 +168,8 @@ config SYS_HYPERVISOR
 	bool
 	default n
 
+config SYS_SOC
+	bool "Export SoC specific informations"
+	depends on EMBEDDED
+
 endmenu
diff --git a/drivers/base/Makefile b/drivers/base/Makefile
index 5f51c3b..f3bcfb3 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..6fa538b
--- /dev/null
+++ b/drivers/base/soc.c
@@ -0,0 +1,88 @@
+/*
+ * Copyright (C) ST-Ericsson SA 2011
+ * Author: Maxime Coquelin <maxime.coquelin-nonst@stericsson.com> for ST-Ericsson.
+ * License terms:  GNU General Public License (GPL), version 2
+ */
+
+#include <linux/sysdev.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/stat.h>
+#include <linux/slab.h>
+#include <linux/sys_soc.h>
+
+static struct sysdev_class_attribute *soc_default_attrs[];
+
+struct sys_soc {
+	char *mach_name;
+	struct sysdev_class class;
+};
+
+struct sys_soc soc = {
+	.class = {
+		.name = "soc",
+		.attrs = soc_default_attrs,
+	},
+};
+
+static ssize_t show_mach_name(struct sysdev_class *class,
+				struct sysdev_class_attribute *attr, char *buf)
+{
+	return sprintf(buf, "%s\n", soc.mach_name);
+}
+static SYSDEV_CLASS_ATTR(mach_name, S_IRUGO, show_mach_name, NULL);
+
+static ssize_t show_info(struct sysdev_class *class,
+				struct sysdev_class_attribute *attr, char *buf)
+{
+	struct sys_soc_info *si = container_of(attr,
+			struct sys_soc_info, attr);
+
+	if (si->info)
+		return sprintf(buf, "%s\n", si->info);
+	else if (si->get_info)
+		return sprintf(buf, "%s\n", si->get_info(si));
+	else
+		return sprintf(buf, "No data\n");
+}
+
+void __init register_sys_soc_info(struct sys_soc_info *info, int nb_info)
+{
+	int i;
+
+	for (i = 0; i < nb_info; i++) {
+		info[i].attr.attr.name = info[i].name;
+		info[i].attr.attr.mode = S_IRUGO;
+		info[i].attr.show = show_info;
+
+		sysdev_class_create_file(&soc.class, &info[i].attr);
+	}
+}
+
+void __init register_sys_soc(char *name, struct sys_soc_info *info, int num)
+{
+	int len;
+
+	len = strlen(name);
+	soc.mach_name = kzalloc(len + 1, GFP_KERNEL);
+	if (!soc.mach_name)
+		return;
+
+	sprintf(soc.mach_name, "%s", name);
+
+	if (sysdev_class_register(&soc.class)) {
+		kfree(soc.mach_name);
+		return;
+	}
+
+	register_sys_soc_info(info, num);
+}
+
+/*
+ * Common attributes for all platforms.
+ * Only machine name for now
+ */
+static struct sysdev_class_attribute *soc_default_attrs[] = {
+	&attr_mach_name,
+	NULL
+};
diff --git a/include/linux/sys_soc.h b/include/linux/sys_soc.h
new file mode 100644
index 0000000..b91a924
--- /dev/null
+++ b/include/linux/sys_soc.h
@@ -0,0 +1,33 @@
+/*
+ * Copyright (C) ST-Ericsson SA 2011
+ * Author: Maxime Coquelin <maxime.coquelin-nonst@stericsson.com> for ST-Ericsson.
+ * License terms:  GNU General Public License (GPL), version 2
+ */
+#ifndef __SYS_SOC_H
+#define __SYS_SOC_H
+
+#include <linux/sysdev.h>
+
+/**
+ * struct sys_soc_info - SoC exports related informations
+ * @name: name of the export
+ * @info: pointer on the key to export
+ * @get_info: callback to retrieve key if info field is NULL
+ * @attr: export's sysdev class attribute
+ */
+struct sys_soc_info {
+	char *name;
+	char *info;
+	char *(*get_info)(struct sys_soc_info *);
+	struct sysdev_class_attribute attr;
+};
+
+/**
+ * void register_sys_soc(char *name, struct sys_soc_info *, int num)
+ * @name: name of the machine
+ * @info: pointer on the info table to export
+ * @num: number of info to export
+ */
+void register_sys_soc(char *name, struct sys_soc_info *info, int num);
+
+#endif /* __SYS_SOC_H */
-- 
1.7.1

[RFC PATCHv1 1/2] Export SoC info through sysfs

[Jamie Iles]

Hi Maxime,

Thanks for doing this, it looks very nice!  A couple of nitpicks, but 
I've given it a quick spin on my platform and it provides us with all of 
the hooks to export all of the information we need.

Jamie

On Wed, Mar 09, 2011 at 05:59:20PM +0100, Maxime Coquelin wrote:
> Common base to export System-on-Chip related informations through sysfs.
> 
> Creation of a "soc" directory under /sys/devices/system/.
> Creation of a common "mach_name" entry to export machine name.
> Creation of platform-defined SoC information entries.
> 
> Signed-off-by: Maxime COQUELIN <maxime.coquelin-nonst@stericsson.com>
> ---
>  drivers/base/Kconfig    |    4 ++
>  drivers/base/Makefile   |    1 +
>  drivers/base/soc.c      |   88 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/sys_soc.h |   33 +++++++++++++++++
>  4 files changed, 126 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..4f2b56d 100644
> --- a/drivers/base/Kconfig
> +++ b/drivers/base/Kconfig
> @@ -168,4 +168,8 @@ config SYS_HYPERVISOR
>  	bool
>  	default n
>  
> +config SYS_SOC
> +	bool "Export SoC specific informations"
> +	depends on EMBEDDED
> +
>  endmenu
> diff --git a/drivers/base/Makefile b/drivers/base/Makefile
> index 5f51c3b..f3bcfb3 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..6fa538b
> --- /dev/null
> +++ b/drivers/base/soc.c
> @@ -0,0 +1,88 @@
> +/*
> + * Copyright (C) ST-Ericsson SA 2011
> + * Author: Maxime Coquelin <maxime.coquelin-nonst@stericsson.com> for ST-Ericsson.
> + * License terms:  GNU General Public License (GPL), version 2
> + */
> +
> +#include <linux/sysdev.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/stat.h>
> +#include <linux/slab.h>
> +#include <linux/sys_soc.h>
> +
> +static struct sysdev_class_attribute *soc_default_attrs[];
> +
> +struct sys_soc {
> +	char *mach_name;

Can this be made const?

> +	struct sysdev_class class;
> +};
> +
> +struct sys_soc soc = {
> +	.class = {
> +		.name = "soc",
> +		.attrs = soc_default_attrs,
> +	},
> +};
> +
> +static ssize_t show_mach_name(struct sysdev_class *class,
> +				struct sysdev_class_attribute *attr, char *buf)
> +{
> +	return sprintf(buf, "%s\n", soc.mach_name);
> +}
> +static SYSDEV_CLASS_ATTR(mach_name, S_IRUGO, show_mach_name, NULL);
> +
> +static ssize_t show_info(struct sysdev_class *class,
> +				struct sysdev_class_attribute *attr, char *buf)
> +{
> +	struct sys_soc_info *si = container_of(attr,
> +			struct sys_soc_info, attr);
> +
> +	if (si->info)
> +		return sprintf(buf, "%s\n", si->info);
> +	else if (si->get_info)
> +		return sprintf(buf, "%s\n", si->get_info(si));
> +	else
> +		return sprintf(buf, "No data\n");

Isn't this a platform error if we don't have a way to get the required 
info?  If in register_sys_soc_info() we check that one of si->info or 
si->get_info is non-NULL then we don't need this check.  If we have 
something like:

static bool sys_soc_info_is_valid(struct sys_soc_info *info)
{
	if ((!info->info && !info->get_info) ||
	    info->info && info->get_info)
	    return false;

	return true;
}

then we can do this at registration.  Is there a valid use case where 
someone could set the static info and the dynamic get_info callback?

> +}
> +
> +void __init register_sys_soc_info(struct sys_soc_info *info, int nb_info)
> +{
> +	int i;
> +
> +	for (i = 0; i < nb_info; i++) {
> +		info[i].attr.attr.name = info[i].name;
> +		info[i].attr.attr.mode = S_IRUGO;
> +		info[i].attr.show = show_info;
> +
> +		sysdev_class_create_file(&soc.class, &info[i].attr);

		if (sys_soc_info_is_valid(&info[i]))
			sysdev_class_create_file(...); ?

> +	}
> +}
> +
> +void __init register_sys_soc(char *name, struct sys_soc_info *info, int num)

Make name const?  Also, should num be a size_t?

> +{
> +	int len;
> +
> +	len = strlen(name);
> +	soc.mach_name = kzalloc(len + 1, GFP_KERNEL);
> +	if (!soc.mach_name)
> +		return;
> +
> +	sprintf(soc.mach_name, "%s", name);

soc.mach_name = kstrdup(name, GFP_KERNEL) instead of the allocate and 
sprintf?

> +
> +	if (sysdev_class_register(&soc.class)) {
> +		kfree(soc.mach_name);
> +		return;
> +	}
> +
> +	register_sys_soc_info(info, num);
> +}
> +
> +/*
> + * Common attributes for all platforms.
> + * Only machine name for now
> + */
> +static struct sysdev_class_attribute *soc_default_attrs[] = {
> +	&attr_mach_name,
> +	NULL
> +};
> diff --git a/include/linux/sys_soc.h b/include/linux/sys_soc.h
> new file mode 100644
> index 0000000..b91a924
> --- /dev/null
> +++ b/include/linux/sys_soc.h
> @@ -0,0 +1,33 @@
> +/*
> + * Copyright (C) ST-Ericsson SA 2011
> + * Author: Maxime Coquelin <maxime.coquelin-nonst@stericsson.com> for ST-Ericsson.
> + * License terms:  GNU General Public License (GPL), version 2
> + */
> +#ifndef __SYS_SOC_H
> +#define __SYS_SOC_H
> +
> +#include <linux/sysdev.h>
> +
> +/**
> + * struct sys_soc_info - SoC exports related informations
> + * @name: name of the export
> + * @info: pointer on the key to export
> + * @get_info: callback to retrieve key if info field is NULL
> + * @attr: export's sysdev class attribute
> + */
> +struct sys_soc_info {
> +	char *name;
> +	char *info;
> +	char *(*get_info)(struct sys_soc_info *);

Could this return a const char* ?

> +	struct sysdev_class_attribute attr;
> +};
> +
> +/**
> + * void register_sys_soc(char *name, struct sys_soc_info *, int num)

I think this should be "register_sys_soc - register the soc information" 
for valid kerneldoc notation..

> + * @name: name of the machine
> + * @info: pointer on the info table to export
> + * @num: number of info to export
> + */
> +void register_sys_soc(char *name, struct sys_soc_info *info, int num);
> +
> +#endif /* __SYS_SOC_H */
> -- 
> 1.7.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

[RFC PATCHv1 1/2] Export SoC info through sysfs

[Maxime Coquelin]

On 03/09/2011 06:39 PM, Jamie Iles wrote:
> Hi Maxime,
>
> Thanks for doing this, it looks very nice!  A couple of nitpicks, but
> I've given it a quick spin on my platform and it provides us with all of
> the hooks to export all of the information we need.
>
> Jamie

Hi Jamie,
Thanks for your review. Fine it works for you.

> +
> +static struct sysdev_class_attribute *soc_default_attrs[];
> +
> +struct sys_soc {
> +	char *mach_name;
> Can this be made const?

Yes, you're right.

>> +
>> +	if (si->info)
>> +		return sprintf(buf, "%s\n", si->info);
>> +	else if (si->get_info)
>> +		return sprintf(buf, "%s\n", si->get_info(si));
>> +	else
>> +		return sprintf(buf, "No data\n");
> Isn't this a platform error if we don't have a way to get the required
> info?  If in register_sys_soc_info() we check that one of si->info or
> si->get_info is non-NULL then we don't need this check.  If we have
> something like:
>
> static bool sys_soc_info_is_valid(struct sys_soc_info *info)
> {
> 	if ((!info->info&&  !info->get_info) ||
> 	    info->info&&  info->get_info)
> 	    return false;
>
> 	return true;
> }

Yes it is a platform error, it should be tested at registration time.

> then we can do this at registration.  Is there a valid use case where
> someone could set the static info and the dynamic get_info callback?
>

At registration, no. But in the get_info callback, we can set the info 
field in order to call the callback once if the value will never change.
However, I will remove this possibility as it is not clear.

>
> Make name const?  Also, should num be a size_t?

Fixed

> soc.mach_name = kstrdup(name, GFP_KERNEL) instead of the allocate and
> sprintf?
>

Fixed.

>> +struct sys_soc_info {
>> +	char *name;
>> +	char *info;
>> +	char *(*get_info)(struct sys_soc_info *);
> Could this return a const char* ?
>

Fixed.

>> +	struct sysdev_class_attribute attr;
>> +};
>> +
>> +/**
>> + * void register_sys_soc(char *name, struct sys_soc_info *, int num)
> I think this should be "register_sys_soc - register the soc information"
> for valid kerneldoc notation..

Fixed.

Regards,
Maxime

[RFC PATCHv1 1/2] Export SoC info through sysfs

[Mark Brown]

On Wed, Mar 09, 2011 at 05:59:20PM +0100, Maxime Coquelin wrote:

> +config SYS_SOC
> +	bool "Export SoC specific informations"
> +	depends on EMBEDDED
> +
>  endmenu

Would it not be better for this to depend on a symbol that systems can
select when they add useful output?  If there's nothing that generates
information for it on a given platform there's no point in enabling it.

> +static ssize_t show_info(struct sysdev_class *class,
> +				struct sysdev_class_attribute *attr, char *buf)
> +{
> +	struct sys_soc_info *si = container_of(attr,
> +			struct sys_soc_info, attr);
> +
> +	if (si->info)
> +		return sprintf(buf, "%s\n", si->info);
> +	else if (si->get_info)
> +		return sprintf(buf, "%s\n", si->get_info(si));

It seems like it'd be easier to pass the output buffer directly to
get_info(), otherwise the get_info() implementation will have to figure
out a buffer to return data from.

[RFC PATCHv1 1/2] Export SoC info through sysfs

[Maxime Coquelin]

On 03/09/2011 06:47 PM, Mark Brown wrote:
> On Wed, Mar 09, 2011 at 05:59:20PM +0100, Maxime Coquelin wrote:
>
>> +config SYS_SOC
>> +	bool "Export SoC specific informations"
>> +	depends on EMBEDDED
>> +
>>   endmenu
> Would it not be better for this to depend on a symbol that systems can
> select when they add useful output?  If there's nothing that generates
> information for it on a given platform there's no point in enabling it.
>

It is deactivated by default when "default" parameter is not mentioned.

>> +static ssize_t show_info(struct sysdev_class *class,
>> +				struct sysdev_class_attribute *attr, char *buf)
>> +{
>> +	struct sys_soc_info *si = container_of(attr,
>> +			struct sys_soc_info, attr);
>> +
>> +	if (si->info)
>> +		return sprintf(buf, "%s\n", si->info);
>> +	else if (si->get_info)
>> +		return sprintf(buf, "%s\n", si->get_info(si));
> It seems like it'd be easier to pass the output buffer directly to
> get_info(), otherwise the get_info() implementation will have to figure
> out a buffer to return data from.

You're right, I will propose a path with passing the output buffer directly.

Regards,
Maxime

[RFC PATCHv1 1/2] Export SoC info through sysfs

[Mark Brown]

On Thu, Mar 10, 2011 at 10:58:36AM +0100, Maxime Coquelin wrote:
> On 03/09/2011 06:47 PM, Mark Brown wrote:

>> Would it not be better for this to depend on a symbol that systems can
>> select when they add useful output?  If there's nothing that generates
>> information for it on a given platform there's no point in enabling it.

> It is deactivated by default when "default" parameter is not mentioned.

That's not the point - if the user sees the option and turns it on then
there won't be anything they can actually do with it unless some other
bit of software creates some data.  No sense in offering them an option
they can't actually use.

[RFC PATCHv1 1/2] Export SoC info through sysfs

[Maxime Coquelin]

On 03/10/2011 02:18 PM, Mark Brown wrote:
> On Thu, Mar 10, 2011 at 10:58:36AM +0100, Maxime Coquelin wrote:
>> On 03/09/2011 06:47 PM, Mark Brown wrote:
>>> Would it not be better for this to depend on a symbol that systems can
>>> select when they add useful output?  If there's nothing that generates
>>> information for it on a given platform there's no point in enabling it.
>> It is deactivated by default when "default" parameter is not mentioned.
> That's not the point - if the user sees the option and turns it on then
> there won't be anything they can actually do with it unless some other
> bit of software creates some data.  No sense in offering them an option
> they can't actually use.

I agree, I will remove the Kconfig option and let platform selects it.

[RFC PATCHv1 1/2] Export SoC info through sysfs

[Arnd Bergmann]

On Wednesday 09 March 2011 17:59:20 Maxime Coquelin wrote:
> Common base to export System-on-Chip related informations through sysfs.
> 
> Creation of a "soc" directory under /sys/devices/system/.

Why under system?

As far as I can tell, the SOC already exists as a platform device
under /sys/devices/platform, so just put the data in there.

There is no point in having the same physical device represented
as multiple separate instances in sysfs.

> Creation of a common "mach_name" entry to export machine name.
> Creation of platform-defined SoC information entries.
> 
> Signed-off-by: Maxime COQUELIN <maxime.coquelin-nonst@stericsson.com>
> ---
>  drivers/base/Kconfig    |    4 ++
>  drivers/base/Makefile   |    1 +
>  drivers/base/soc.c      |   88 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/sys_soc.h |   33 +++++++++++++++++

This seems to be missing the documentation file. Every sysfs
attribute you create must be documented.

> --- a/drivers/base/Kconfig
> +++ b/drivers/base/Kconfig
> @@ -168,4 +168,8 @@ config SYS_HYPERVISOR
>  	bool
>  	default n
>  
> +config SYS_SOC
> +	bool "Export SoC specific informations"
> +	depends on EMBEDDED
> +
>  endmenu

CONFIG_EMBEDDED is gone, and did not mean what you intended.

Just make the information unconditionally available in
the code that manages you SoC. 

> +void __init register_sys_soc(char *name, struct sys_soc_info *info, int num)
> +{
> +	int len;
> +
> +	len = strlen(name);
> +	soc.mach_name = kzalloc(len + 1, GFP_KERNEL);
> +	if (!soc.mach_name)
> +		return;
> +
> +	sprintf(soc.mach_name, "%s", name);
> +
> +	if (sysdev_class_register(&soc.class)) {
> +		kfree(soc.mach_name);
> +		return;
> +	}
> +
> +	register_sys_soc_info(info, num);
> +}

You do way too much here when all you need is a platform
device attribute.

	Arnd

[RFC PATCHv1 1/2] Export SoC info through sysfs

[Maxime Coquelin]

On 03/09/2011 08:58 PM, Arnd Bergmann wrote:
> On Wednesday 09 March 2011 17:59:20 Maxime Coquelin wrote:
>> Common base to export System-on-Chip related informations through sysfs.
>>
>> Creation of a "soc" directory under /sys/devices/system/.
> Why under system?
>
> As far as I can tell, the SOC already exists as a platform device
> under /sys/devices/platform, so just put the data in there.

The aim of this patch is to provide a common interface for SoC vendors 
to export some pieces of information.
For now, only the machine name is common in this patch set, but some 
other attributes might be common (family name, silicon process...).
Moving to /sys/devices/platform means it will be vendor-specific, so we 
loose the gain of having a common ABI. Right?

> There is no point in having the same physical device represented
> as multiple separate instances in sysfs.
>
>> Creation of a common "mach_name" entry to export machine name.
>> Creation of platform-defined SoC information entries.
>>
>> Signed-off-by: Maxime COQUELIN<maxime.coquelin-nonst@stericsson.com>
>> ---
>>   drivers/base/Kconfig    |    4 ++
>>   drivers/base/Makefile   |    1 +
>>   drivers/base/soc.c      |   88 +++++++++++++++++++++++++++++++++++++++++++++++
>>   include/linux/sys_soc.h |   33 +++++++++++++++++
> This seems to be missing the documentation file. Every sysfs
> attribute you create must be documented.
>

Ok, I will fix it.

>> --- a/drivers/base/Kconfig
>> +++ b/drivers/base/Kconfig
>> @@ -168,4 +168,8 @@ config SYS_HYPERVISOR
>>   	bool
>>   	default n
>>
>> +config SYS_SOC
>> +	bool "Export SoC specific informations"
>> +	depends on EMBEDDED
>> +
>>   endmenu
> CONFIG_EMBEDDED is gone, and did not mean what you intended.
>
> Just make the information unconditionally available in
> the code that manages you SoC.
>

Ok.

>> +void __init register_sys_soc(char *name, struct sys_soc_info *info, int num)
>> +{
>> +	int len;
>> +
>> +	len = strlen(name);
>> +	soc.mach_name = kzalloc(len + 1, GFP_KERNEL);
>> +	if (!soc.mach_name)
>> +		return;
>> +
>> +	sprintf(soc.mach_name, "%s", name);
>> +
>> +	if (sysdev_class_register(&soc.class)) {
>> +		kfree(soc.mach_name);
>> +		return;
>> +	}
>> +
>> +	register_sys_soc_info(info, num);
>> +}
> You do way too much here when all you need is a platform
> device attribute.
>
> 	Arnd

Thanks for your review,
Maxime

[RFC PATCHv1 1/2] Export SoC info through sysfs

[Linus Walleij]

2011/3/9 Arnd Bergmann <arnd@arndb.de>:
> On Wednesday 09 March 2011 17:59:20 Maxime Coquelin wrote:
>> Common base to export System-on-Chip related informations through sysfs.
>>
>> Creation of a "soc" directory under /sys/devices/system/.
>
> Why under system?
>
> As far as I can tell, the SOC already exists as a platform device
> under /sys/devices/platform,

This is from U8500:

/sys/devices/platform ls

ab8500-i2c.0   gpio.0         gpio.5         nmk-i2c.0      uevent
arm-pmu.0      gpio.1         gpio.6         nmk-i2c.1
cpufreq-u8500  gpio.2         gpio.7         nmk-i2c.2
dma40.0        gpio.3         gpio.8         nmk-i2c.3
gpio-keys.0    gpio.4         musb-ux500.0   reg-dummy

In our case the above are platform_device:s and also MFD cells,
children of I2C devices (anything prefixed with ab8500-*) which
have absolutely *nothing* to do with what is inside the SoC.

Further these SoC entries don't even have a device of *any* kind tied
to them. It's just hooks reading binary right out of some extremely
system-specific memory locations and producing matching strings.

"platform" means "platform bus" in this context, does it not?
So your reasoning is that since on SoC:s there is one dominant
bus called the platform bus that should also hold a reference to the
SoC-specific stuff.

On the ARM Versatiles this isn't even true: there the dominant
bus is the AMBA (PrimeCell) bus, and the U8500 uses a mixture
of both. AMBA devices incidentally turn up as entries directly
in /sys/devices:

/sys/devices ls
platform   sdi0       sdi4       system     uart1      virtual
rtc-pl031  sdi2       ssp0       uart0      uart2

The sdi, uart & rtc you see here are AMBA devices.

So platform is just the platform bus, not the, you know,
*platform*. There are competing on-chip busses not just one.

So standardizing the location here,
/sys/devices/system contain these more neutral things:

/sys/devices/system ls
clocksource  cpu          timekeeping  timer

IMHO socinfo fits here, just as it is in the patch.

This might be a bit of bikeshed-color discussion but I
happen to like this black color sooooo much...

Just my ?0.01
Linus Walleij

[RFC PATCHv1 1/2] Export SoC info through sysfs

[Mark Brown]

On Thu, Mar 10, 2011 at 02:25:19PM +0100, Linus Walleij wrote:

> "platform" means "platform bus" in this context, does it not?
> So your reasoning is that since on SoC:s there is one dominant
> bus called the platform bus that should also hold a reference to the
> SoC-specific stuff.

Lots of stuff (like the MFD children of I2C devices) gets shoved on the
platform bus on Linux - every time someone considers creating a new bus
type for trivial devices the discussion ends up saying that the code
will end up being pretty much identical to the platform bus as it's so
straightforward so there's no point in cut'n'pasting the code.

> So standardizing the location here,
> /sys/devices/system contain these more neutral things:

> /sys/devices/system ls
> clocksource  cpu          timekeeping  timer

> IMHO socinfo fits here, just as it is in the patch.

There's a general idea to kill of the system devices as the fact that
they do special magic and don't have things like a struct device creates
integration problems.

[RFC PATCHv1 1/2] Export SoC info through sysfs

[Arnd Bergmann]

On Thursday 10 March 2011, Mark Brown wrote:
> On Thu, Mar 10, 2011 at 02:25:19PM +0100, Linus Walleij wrote:
> 
> > "platform" means "platform bus" in this context, does it not?
> > So your reasoning is that since on SoC:s there is one dominant
> > bus called the platform bus that should also hold a reference to the
> > SoC-specific stuff.
> 
> Lots of stuff (like the MFD children of I2C devices) gets shoved on the
> platform bus on Linux - every time someone considers creating a new bus
> type for trivial devices the discussion ends up saying that the code
> will end up being pretty much identical to the platform bus as it's so
> straightforward so there's no point in cut'n'pasting the code.

Note that there is a difference between using top-level platform
devices (the ones that show up in /sys/devices/platform/) and platform
devices that are children of others (all the others that are in
/sys/bus/platform).

Creating a new bus_type for trivial things is rather pointless,
so using a platform device is fine for MFD, since the child devices
just show up in the hierarchy.

What some broken drivers do is to create platform devices ad hoc
and just put them into /sys/platform/ directly although the
devices clearly have a parent somewhere. These are bugs that should
be fixed.

	Arnd

[RFC PATCHv1 1/2] Export SoC info through sysfs

[Mark Brown]

On Thu, Mar 10, 2011 at 03:29:25PM +0100, Arnd Bergmann wrote:
> On Thursday 10 March 2011, Mark Brown wrote:

> > Lots of stuff (like the MFD children of I2C devices) gets shoved on the
> > platform bus on Linux - every time someone considers creating a new bus
> > type for trivial devices the discussion ends up saying that the code
> > will end up being pretty much identical to the platform bus as it's so
> > straightforward so there's no point in cut'n'pasting the code.

> Note that there is a difference between using top-level platform
> devices (the ones that show up in /sys/devices/platform/) and platform
> devices that are children of others (all the others that are in
> /sys/bus/platform).

Sure.

> What some broken drivers do is to create platform devices ad hoc
> and just put them into /sys/platform/ directly although the
> devices clearly have a parent somewhere. These are bugs that should
> be fixed.

Most of the devices doing that on ARMs tend to be devices where there's
no bus that it's meaningful to expose to software - they'll typically be
hanging off an AHB or APB bus which has either no real software control
or control which is very heavily tied into the overall system control
and so is better treated over the whole SoC rather than the bus.

[RFC PATCHv1 1/2] Export SoC info through sysfs

[Arnd Bergmann]

On Thursday 10 March 2011, Mark Brown wrote:
> Most of the devices doing that on ARMs tend to be devices where there's
> no bus that it's meaningful to expose to software - they'll typically be
> hanging off an AHB or APB bus which has either no real software control
> or control which is very heavily tied into the overall system control
> and so is better treated over the whole SoC rather than the bus.

I can accept that it's reasonable to put all devices under
/sys/devices/platform/ that are on the same top-level AHB or APB bus,
although I also don't see a reason against making that bus a single
device.

The one thing that really doesn't belong in /sys/devices/platform/
however are devices that are clearly children of some other
device, e.g. a UART behind MFD behind I2C behind AHB.

	Arnd

[RFC PATCHv1 1/2] Export SoC info through sysfs

[Russell King - ARM Linux]

On Thu, Mar 10, 2011 at 04:02:18PM +0100, Arnd Bergmann wrote:
> The one thing that really doesn't belong in /sys/devices/platform/
> however are devices that are clearly children of some other
> device, e.g. a UART behind MFD behind I2C behind AHB.

Who ever thought about putting a UART behind I2C ?  That's insane!

[RFC PATCHv1 1/2] Export SoC info through sysfs

[Linus Walleij]

On Thu, Mar 10, 2011 at 4:10 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Thu, Mar 10, 2011 at 04:02:18PM +0100, Arnd Bergmann wrote:
>> The one thing that really doesn't belong in /sys/devices/platform/
>> however are devices that are clearly children of some other
>> device, e.g. a UART behind MFD behind I2C behind AHB.
>
> Who ever thought about putting a UART behind I2C ? ?That's insane!

For immediate entertainment I can tell you we have such a thing in
the AB3100 on the U300, at the other side of an I2C link.

That UART connects to a so-called "smart battery", since the
AB3100 is our mixed-signal device it had the physical connections
to the battery and thus that UART that can talk to the CPU inside
the battery.

There aren't many bauds on this bus, I assure you... yet,
batteries aren't very talkative anyway.

Yours,
Linus Walleij

[RFC PATCHv1 1/2] Export SoC info through sysfs

[Helmut Raiger]

On 03/10/2011 04:10 PM, Russell King - ARM Linux wrote:
> Who ever thought about putting a UART behind I2C ?  That's insane!

Maxim MAX 3107
http://www.maxim-ic.com/datasheet/index.mvp/id/6463

The advantage is that you can have 8 of them on I2C and hundreds on SPI 
(which is insane ;-) )

Helmut



--
Scanned by MailScanner.

[RFC PATCHv1 1/2] Export SoC info through sysfs

[Mark Brown]

On Thu, Mar 10, 2011 at 04:02:18PM +0100, Arnd Bergmann wrote:

> I can accept that it's reasonable to put all devices under
> /sys/devices/platform/ that are on the same top-level AHB or APB bus,
> although I also don't see a reason against making that bus a single
> device.

You could, though the bus will just be a noop.  Typically it's more than
one bus but software basically can't tell.

> The one thing that really doesn't belong in /sys/devices/platform/
> however are devices that are clearly children of some other
> device, e.g. a UART behind MFD behind I2C behind AHB.

Right, and we seem to be fairly good at squashing such bugs when people
notice them.

[RFC PATCHv1 1/2] Export SoC info through sysfs

[Arnd Bergmann]

On Thursday 10 March 2011, Mark Brown wrote:
> On Thu, Mar 10, 2011 at 04:02:18PM +0100, Arnd Bergmann wrote:
> 
> > I can accept that it's reasonable to put all devices under
> > /sys/devices/platform/ that are on the same top-level AHB or APB bus,
> > although I also don't see a reason against making that bus a single
> > device.
> 
> You could, though the bus will just be a noop.  Typically it's more than
> one bus but software basically can't tell.

Yes. The main reason for representing such a bus in sysfs would be
to match the SOC's block diagram with the structure in the kernel.

The cost of an extra device is roughly zero, but structure generally
helps. Another advantage is that you can associate attributes and names
with the device, and simplify naming of the children without risking
name space conflicts.

	Arnd

[RFC PATCHv1 1/2] Export SoC info through sysfs

[Mark Brown]

On Thu, Mar 10, 2011 at 05:11:59PM +0100, Arnd Bergmann wrote:
> On Thursday 10 March 2011, Mark Brown wrote:

> > You could, though the bus will just be a noop.  Typically it's more than
> > one bus but software basically can't tell.

> Yes. The main reason for representing such a bus in sysfs would be
> to match the SOC's block diagram with the structure in the kernel.

If you're doing that things like power domains tend to be a lot more
interesting since you can do something meaningful with them in software.
The non-visible buses aren't reliably documented anyway and the first
procesor datasheet I just pulled up had a whole bunch of devices that
span multiple buses anyway :)

[RFC PATCHv1 1/2] Export SoC info through sysfs

[Arnd Bergmann]

On Thursday 10 March 2011, Mark Brown wrote:
> On Thu, Mar 10, 2011 at 05:11:59PM +0100, Arnd Bergmann wrote:
> > On Thursday 10 March 2011, Mark Brown wrote:
> 
> > > You could, though the bus will just be a noop.  Typically it's more than
> > > one bus but software basically can't tell.
> 
> > Yes. The main reason for representing such a bus in sysfs would be
> > to match the SOC's block diagram with the structure in the kernel.
> 
> If you're doing that things like power domains tend to be a lot more
> interesting since you can do something meaningful with them in software.
> The non-visible buses aren't reliably documented anyway and the first
> procesor datasheet I just pulled up had a whole bunch of devices that
> span multiple buses anyway :)

Yes, I'm aware that devices are not alwasy in a clear hierarchy and
that you have bus addressing, device driver, clock, power and interrupt
(and possibly more) trees that often don't match up.

My point is simply that we should still try to find a helpful tree
representation in these cases, even if it's not perfect. Almost anything
is better than having lots of unrelated devices in a flat directory.

	Arnd

[RFC PATCHv1 1/2] Export SoC info through sysfs

[Arnd Bergmann]

On Thursday 10 March 2011, Linus Walleij wrote:
> >
> > As far as I can tell, the SOC already exists as a platform device
> > under /sys/devices/platform,
> 
> This is from U8500:
> 
> /sys/devices/platform ls
> 
> ab8500-i2c.0   gpio.0         gpio.5         nmk-i2c.0      uevent
> arm-pmu.0      gpio.1         gpio.6         nmk-i2c.1
> cpufreq-u8500  gpio.2         gpio.7         nmk-i2c.2
> dma40.0        gpio.3         gpio.8         nmk-i2c.3
> gpio-keys.0    gpio.4         musb-ux500.0   reg-dummy
> 
> In our case the above are platform_device:s and also MFD cells,
> children of I2C devices (anything prefixed with ab8500-*) which
> have absolutely nothing to do with what is inside the SoC.

Ok, I must correct myself:

There *should* *be* an SOC device that is the parent of all
devices that are part of the SOC, instead of a bunch of
random stuff getting placed directly under /sys/devices/platform.

Most of the stuff you have there really isn't a top-level
platform device at all.

> Further these SoC entries don't even have a device of any kind tied
> to them. It's just hooks reading binary right out of some extremely
> system-specific memory locations and producing matching strings.

On the SOC devices that I've dealt with, they are organized in
some hierarchy of buses, and each bus has its own registers
that identify it, and should therefore be represented in
sysfs as a device with other child devices.

Making up a pseudo-device that does not refer to any hardware
in particular is against the 'devices are only "devices"'
rule in Documentation/sysfs-rules.txt.

If you really want that, it should be in /sys/kernel/,
/sys/firmware/ or a new top-level directory in sysfs.

I think putting it in /sys/devices is good, but it has
to be an attribute of an actual device, not an empty
one that does not even have any child devices. If the
device represents the soc, then every other device
that is found in the soc should be a child of this one.

> "platform" means "platform bus" in this context, does it not?
> So your reasoning is that since on SoC:s there is one dominant
> bus called the platform bus that should also hold a reference to the
> SoC-specific stuff.

No. Be careful not to confuse bus and bus_type here. Platform
bus refers to the bus_type and it can hold any number of
devices that are directly addressable but cannot be probed.

> On the ARM Versatiles this isn't even true: there the dominant
> bus is the AMBA (PrimeCell) bus, and the U8500 uses a mixture
> of both. AMBA devices incidentally turn up as entries directly
> in /sys/devices:
> 
> /sys/devices ls
> platform   sdi0       sdi4       system     uart1      virtual
> rtc-pl031  sdi2       ssp0       uart0      uart2
> 
> The sdi, uart & rtc you see here are AMBA devices.

I would argue that these belong into a top-level AMBA bus device
that holds all devices connected to one AMBA bus. It's a
really bad idea to put random devices into a the global
/sys/devices/ namespace, because of potential conflicts.

> So platform is just the platform bus, not the, you know,
> *platform*. There are competing on-chip busses not just one.

Good point. Note that I did not mean to put the attributes
directly under /sys/platform/, but under /sys/platform/foo/,
where foo is the main bus that is used between the CPU core
and all the SOC components.

This may of course not be easy. If you have an AMBA or PCI
bus, you might want to have it represented as
/sys/devices/pci0 and /sys/devices/amba0 instead of
/sys/devices/platform/foo/amba0/.

	Arnd

[RFC PATCHv1 1/2] Export SoC info through sysfs

[Linus Walleij]

On Thu, Mar 10, 2011 at 3:23 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Thursday 10 March 2011, Linus Walleij wrote:
>> So platform is just the platform bus, not the, you know,
>> *platform*. There are competing on-chip busses not just one.
>
> Good point. Note that I did not mean to put the attributes
> directly under /sys/platform/, but under /sys/platform/foo/,
> where foo is the main bus that is used between the CPU core
> and all the SOC components.
>
> This may of course not be easy. If you have an AMBA or PCI
> bus, you might want to have it represented as
> /sys/devices/pci0 and /sys/devices/amba0 instead of
> /sys/devices/platform/foo/amba0/.

Userspace app doing something needs the name and some
unique number to be looked up. The first iteration of this
patch (for the OMAPs) put it in /proc/socinfo with all the
expected fuzz that this should go into sysfs as result.

So: where do we suggest Maxime actually put this then?

> Making up a pseudo-device that does not refer to any hardware
> in particular is against the 'devices are only "devices"'
> rule in Documentation/sysfs-rules.txt.

Fine then I think we agree this can not realistically be
under /sys/devices/* unless we first refactor all platforms
that want to use this mechanism to have top-level-devices.

> If you really want that, it should be in /sys/kernel/,
> /sys/firmware/ or a new top-level directory in sysfs.
>
> I think putting it in /sys/devices is good, but it has
> to be an attribute of an actual device, not an empty
> one that does not even have any child devices. If the
> device represents the soc, then every other device
> that is found in the soc should be a child of this one.

IMO this is not good for socinfo, what is good for apps that
want socinfo is to just attempt to open+read these file paths.
Not to invoke libsysfs and start parsing the file tree to see if
they can find some socinfo somewhere.

What about we just put it in
/sys/socinfo/* then, and we have a simple,
easy-to-understand way for apps that want socinfo to
read it out?

Everyone happy with this?

Yours,
Linus Walleij

[RFC PATCHv1 1/2] Export SoC info through sysfs

[Arnd Bergmann]

On Thursday 10 March 2011, Linus Walleij wrote:
> On Thu, Mar 10, 2011 at 3:23 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Thursday 10 March 2011, Linus Walleij wrote:
> >> So platform is just the platform bus, not the, you know,
> >> *platform*. There are competing on-chip busses not just one.
> >
> > Good point. Note that I did not mean to put the attributes
> > directly under /sys/platform/, but under /sys/platform/foo/,
> > where foo is the main bus that is used between the CPU core
> > and all the SOC components.
> >
> > This may of course not be easy. If you have an AMBA or PCI
> > bus, you might want to have it represented as
> > /sys/devices/pci0 and /sys/devices/amba0 instead of
> > /sys/devices/platform/foo/amba0/.
> 
> Userspace app doing something needs the name and some
> unique number to be looked up. The first iteration of this
> patch (for the OMAPs) put it in /proc/socinfo with all the
> expected fuzz that this should go into sysfs as result.
> 
> So: where do we suggest Maxime actually put this then?

I'm not sure we even need to define something really new.
Ideally, it could just be properties of the devices in
the device tree, but obviously we're not there yet and
may not even want to go there for all platforms.

> > Making up a pseudo-device that does not refer to any hardware
> > in particular is against the 'devices are only "devices"'
> > rule in Documentation/sysfs-rules.txt.
> 
> Fine then I think we agree this can not realistically be
> under /sys/devices/* unless we first refactor all platforms
> that want to use this mechanism to have top-level-devices.

Not much refactoring required there, just register one
(platform or top-level, I don't care) device named "soc",
and make all platform devices of the soc have a .parent
pointer to that device.

Then you can add attributes to the device.

> > If you really want that, it should be in /sys/kernel/,
> > /sys/firmware/ or a new top-level directory in sysfs.
> >
> > I think putting it in /sys/devices is good, but it has
> > to be an attribute of an actual device, not an empty
> > one that does not even have any child devices. If the
> > device represents the soc, then every other device
> > that is found in the soc should be a child of this one.
> 
> IMO this is not good for socinfo, what is good for apps that
> want socinfo is to just attempt to open+read these file paths.
> Not to invoke libsysfs and start parsing the file tree to see if
> they can find some socinfo somewhere.
> 
> What about we just put it in
> /sys/socinfo/* then, and we have a simple,
> easy-to-understand way for apps that want socinfo to
> read it out?

Possible, but I'd be less happy with this. It's a bit
of duplication of the information that is already present
in both /sys/devices and /sys/firmware.

	Arnd

[RFC PATCHv1 1/2] Export SoC info through sysfs

[Linus Walleij]

On Thu, Mar 10, 2011 at 5:32 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Thursday 10 March 2011, Linus Walleij wrote:
>> On Thu, Mar 10, 2011 at 3:23 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> > On Thursday 10 March 2011, Linus Walleij wrote:
> Not much refactoring required there, just register one
> (platform or top-level, I don't care) device named "soc",
> and make all platform devices of the soc have a .parent
> pointer to that device.
>
> Then you can add attributes to the device.

But since the name of the top-level device name is not
subject to standardization, I suspect that an app
not knowing which SoC it is running on will indeed
need to parse through the entire sysfs/devices/*
hierarchy to see if it can find some soc name.

No easy feat, indeed libsysfs can help you.
Equal to:

x=`find /sys/devices | grep mach-name`
cat x

Compare this to getting the *name* of the hardware
you want to obtain the SoC-info for. Easy:

grep Hardware /proc/cpuinfo

What I'm trying to get at is that the user of this
info isn't really interested in any device tree structure,
it just becomes an obstacle s/he has to overcome
to get this info out.

But there may be a compromise: if we create the
socinfo in one place in the device tree (possibly the
top node, or a dedicated device) then class it?

/sys/class/soc/soc0 -> ../../devices/platform/top-node

Then you find your entries easily by opening
/sys/class/soc/soc0/* ?

If the socinfo interface is singleton (and why should
it not be) then:

/sys/class/soc -> ../devices/platfom/top-node

Or is this thing too trivial to have it's own class?

Yours,
Linus Walleij

[RFC PATCHv1 1/2] Export SoC info through sysfs

[Arnd Bergmann]

On Thursday 10 March 2011, Linus Walleij wrote:
> What I'm trying to get at is that the user of this
> info isn't really interested in any device tree structure,
> it just becomes an obstacle s/he has to overcome
> to get this info out.
> 
> But there may be a compromise: if we create the
> socinfo in one place in the device tree (possibly the
> top node, or a dedicated device) then class it?
> 
> /sys/class/soc/soc0 -> ../../devices/platform/top-node
> 
> Then you find your entries easily by opening
> /sys/class/soc/soc0/* ?

That would be one way of finding all soc nodes, in
case we want to have multiple ones. Similarly,
it could be done based on

1. the name under /sys/devices/platform:

/sys/devices/platform/soc0
/sys/devices/platform/soc1
/sys/devices/platform/soc2

2. a new bus_type for soc devices, with bus_attributes:

/sys/bus/soc/devices/foo0 -> ../../../devices/platform/foo0
/sys/bus/soc/devices/bar0 -> ../../../devices/platform/bar0
/sys/bus/soc/devices/bar1 -> ../../../devices/platform/bar1

3. A new top-level device besides /sys/devices/platform (like PCI):

/sys/devices/soc0
/sys/devices/soc1
/sys/devices/soc2

> If the socinfo interface is singleton (and why should
> it not be) then:
> 
> /sys/class/soc -> ../devices/platfom/top-node
> 
> Or is this thing too trivial to have it's own class?

In case of a singleton, I'd just use a fixed device as
the parent:

/sys/devices/platform/soc/

or

/sys/devices/soc/

And pass that as the parent for all devices under it.

	Arnd

[RFC PATCHv1 1/2] Export SoC info through sysfs

[Ryan Mallon]

On 03/10/2011 05:59 AM, Maxime Coquelin wrote:
> Common base to export System-on-Chip related informations through sysfs.
> 
> Creation of a "soc" directory under /sys/devices/system/.
> Creation of a common "mach_name" entry to export machine name.
> Creation of platform-defined SoC information entries.

A few comments below. Echoing some of Jamie's and Mark's comments.

~Ryan

> 
> Signed-off-by: Maxime COQUELIN <maxime.coquelin-nonst@stericsson.com>
> ---
>  drivers/base/Kconfig    |    4 ++
>  drivers/base/Makefile   |    1 +
>  drivers/base/soc.c      |   88 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/sys_soc.h |   33 +++++++++++++++++
>  4 files changed, 126 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..4f2b56d 100644
> --- a/drivers/base/Kconfig
> +++ b/drivers/base/Kconfig
> @@ -168,4 +168,8 @@ config SYS_HYPERVISOR
>  	bool
>  	default n
>  
> +config SYS_SOC
> +	bool "Export SoC specific informations"
> +	depends on EMBEDDED

This should ideally be automatically selected by platforms which provide
SoC information.

Also, the word "informations" sounds odd to me. It can be replaced above
with "information" and in some other places with "pieces of information"
or similar.

> +
>  endmenu
> diff --git a/drivers/base/Makefile b/drivers/base/Makefile
> index 5f51c3b..f3bcfb3 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..6fa538b
> --- /dev/null
> +++ b/drivers/base/soc.c
> @@ -0,0 +1,88 @@
> +/*
> + * Copyright (C) ST-Ericsson SA 2011
> + * Author: Maxime Coquelin <maxime.coquelin-nonst@stericsson.com> for ST-Ericsson.
> + * License terms:  GNU General Public License (GPL), version 2
> + */
> +
> +#include <linux/sysdev.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/stat.h>
> +#include <linux/slab.h>
> +#include <linux/sys_soc.h>
> +
> +static struct sysdev_class_attribute *soc_default_attrs[];
> +
> +struct sys_soc {
> +	char *mach_name;
> +	struct sysdev_class class;
> +};
> +
> +struct sys_soc soc = {
> +	.class = {
> +		.name = "soc",
> +		.attrs = soc_default_attrs,
> +	},
> +};
> +
> +static ssize_t show_mach_name(struct sysdev_class *class,
> +				struct sysdev_class_attribute *attr, char *buf)
> +{
> +	return sprintf(buf, "%s\n", soc.mach_name);
> +}
> +static SYSDEV_CLASS_ATTR(mach_name, S_IRUGO, show_mach_name, NULL);

If you make mach_name work like the other SoC info attributes then it
can be exported using the generic show_info function below. It also
means that struct sys_soc is no longer necessary; you can just have the
global sysdev_class.

> +
> +static ssize_t show_info(struct sysdev_class *class,
> +				struct sysdev_class_attribute *attr, char *buf)
> +{
> +	struct sys_soc_info *si = container_of(attr,
> +			struct sys_soc_info, attr);
> +
> +	if (si->info)
> +		return sprintf(buf, "%s\n", si->info);
> +	else if (si->get_info)
> +		return sprintf(buf, "%s\n", si->get_info(si));
> +	else
> +		return sprintf(buf, "No data\n");

Agree with others that it should be a bug to have an item with no data.
i.e. the registration should fail, probably with a WARN_ON. I also agree
that buf should be passed directly into the get_info callback. How do we
deal with overflow from dodgy callbacks? From memory we can safely write
at least one page to a sysfs buffer?

> +}
> +
> +void __init register_sys_soc_info(struct sys_soc_info *info, int nb_info)
> +{

register_sys_soc_info can only be called once. That may be the
intention, but it needs to be documented as such. Possibly a WARN_ON to
catch it being called multiple times? I could see potential bugs in
kernels which have multiple SoC/platforms built in.

This function should also be static, since calling it directly will
result in the mach_name entry never being set.

> +	int i;
> +
> +	for (i = 0; i < nb_info; i++) {
> +		info[i].attr.attr.name = info[i].name;
> +		info[i].attr.attr.mode = S_IRUGO;
> +		info[i].attr.show = show_info;

This is double handling. Can't we do something like this:

struct sys_soc_info {
	char *info;
	char *(*get_info)(struct sys_soc_info *);
	struct sysdev_class_attribute attr;
};

#define SYS_SOC_ATTR_VALUE(name, value) {	\
	.attr.attr.name = name,			\
	.mode           = S_IRUGO,		\
	.info           = value,		\
}

#define SYS_SOC_ATTR_CALLBACK(name, callback) {	\
	.attr.attr.name = name,			\
	.mode           = S_IRUGO,		\
	.get_info       = callback,		\
}

Then the above code isn't necessary and the ux500 platform registration
looks like this:

static struct sys_soc_info soc_info[] = {
	SYS_SOC_ATTR_CALLBACK("soc_id",   ux500_get_soc_id),
	SYS_SOC_ATTR_CALLBACK("revision", ux500_get_revision),
	SYS_SOC_ATTR_CALLBACK("process",  ux500_get_process),
};

Which is much more concise.

> +
> +		sysdev_class_create_file(&soc.class, &info[i].attr);

sysdev_class_create_file can fail, so we should propagate the error up.

> +	}
> +}
> +
> +void __init register_sys_soc(char *name, struct sys_soc_info *info, int num)
> +{
> +	int len;
> +
> +	len = strlen(name);
> +	soc.mach_name = kzalloc(len + 1, GFP_KERNEL);
> +	if (!soc.mach_name)
> +		return;
> +
> +	sprintf(soc.mach_name, "%s", name);

Change name to mach_name (or family?) to make it clearer what it is for
and make it const. Use kstrdup. This function should really return
proper error codes rather than silently failing.

I also think mach_name should be added the same way as the other items
since it should simplify the code a little.

> +
> +	if (sysdev_class_register(&soc.class)) {
> +		kfree(soc.mach_name);
> +		return;
> +	}

Standard kernel coding practice is:

	err = sysdev_class_register(&soc.class);	
	if (err) {
		kfree(soc.mach_name);
		return err;
	}

> +
> +	register_sys_soc_info(info, num);
> +}
> +
> +/*
> + * Common attributes for all platforms.
> + * Only machine name for now
> + */
> +static struct sysdev_class_attribute *soc_default_attrs[] = {
> +	&attr_mach_name,
> +	NULL
> +};

This becomes part of the ABI, so needs to be documented.

> diff --git a/include/linux/sys_soc.h b/include/linux/sys_soc.h
> new file mode 100644
> index 0000000..b91a924
> --- /dev/null
> +++ b/include/linux/sys_soc.h
> @@ -0,0 +1,33 @@
> +/*
> + * Copyright (C) ST-Ericsson SA 2011
> + * Author: Maxime Coquelin <maxime.coquelin-nonst@stericsson.com> for ST-Ericsson.
> + * License terms:  GNU General Public License (GPL), version 2
> + */
> +#ifndef __SYS_SOC_H
> +#define __SYS_SOC_H
> +
> +#include <linux/sysdev.h>
> +
> +/**
> + * struct sys_soc_info - SoC exports related informations
> + * @name: name of the export
> + * @info: pointer on the key to export
> + * @get_info: callback to retrieve key if info field is NULL

It should be noted here that one and only one of info and get_info must
be set.

> + * @attr: export's sysdev class attribute
> + */
> +struct sys_soc_info {
> +	char *name;
> +	char *info;
> +	char *(*get_info)(struct sys_soc_info *);
> +	struct sysdev_class_attribute attr;
> +};
> +
> +/**
> + * void register_sys_soc(char *name, struct sys_soc_info *, int num)
> + * @name: name of the machine

This is not really descriptive enough. We need to be reasonably clear
about what we mean by 'name'. Is it the mach name (i.e. the name of the
mach-xxxx) directory, is it the family (i.e. omap3 and omap4 are both
under omap), or is it something else?

> + * @info: pointer on the info table to export
> + * @num: number of info to export
> + */

Need to document here that register_sys_soc must only be called once.

> +void register_sys_soc(char *name, struct sys_soc_info *info, int num);
> +
> +#endif /* __SYS_SOC_H */

~Ryan

-- 
Bluewater Systems Ltd - ARM Technology Solution Centre

Ryan Mallon         		5 Amuri Park, 404 Barbadoes St
ryan at bluewatersys.com         	PO Box 13 889, Christchurch 8013
http://www.bluewatersys.com	New Zealand
Phone: +64 3 3779127		Freecall: Australia 1800 148 751
Fax:   +64 3 3779135			  USA 1800 261 2934

[RFC PATCHv1 2/2] ux500: Export U8500 SoC info through sysfs

[Maxime Coquelin]

ST-Ericsson's U8500 implementation.
Register sysfs SoC interface, and export SoC ID, process and silicon revision
number.

Signed-off-by: Maxime COQUELIN <maxime.coquelin-nonst@stericsson.com>
---
 arch/arm/mach-ux500/id.c |   96 ++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 96 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-ux500/id.c b/arch/arm/mach-ux500/id.c
index d35122e..0d41c77 100644
--- a/arch/arm/mach-ux500/id.c
+++ b/arch/arm/mach-ux500/id.c
@@ -8,6 +8,8 @@
 #include <linux/kernel.h>
 #include <linux/init.h>
 #include <linux/io.h>
+#include <linux/sys_soc.h>
+#include <linux/slab.h>
 
 #include <asm/cputype.h>
 #include <asm/tlbflush.h>
@@ -105,3 +107,97 @@ void __init ux500_map_io(void)
 
 	ux500_print_soc_info(asicid);
 }
+
+
+#ifdef CONFIG_SYS_SOC
+#define U8500_BB_UID_BASE (U8500_BACKUPRAM1_BASE + 0xFC0)
+#define U8500_BB_UID_LENGTH 5
+
+#define UX500_SOC_INFO_SZ 64
+
+static char soc_id[UX500_SOC_INFO_SZ];
+static char revision[UX500_SOC_INFO_SZ];
+static char process[UX500_SOC_INFO_SZ];
+
+static char *ux500_get_soc_id(struct sys_soc_info *si)
+{
+	void __iomem *uid_base = __io_address(U8500_BB_UID_BASE);
+	int i, sz = 0;
+
+	if (dbx500_partnumber() == 0x8500) {
+		for (i = 0; i < U8500_BB_UID_LENGTH; i++)
+			sz += snprintf(soc_id + sz, UX500_SOC_INFO_SZ - sz, "%08x",
+					readl(uid_base + i * sizeof(u32)));
+	}
+	else {
+		/* Don't know where it is located for U5500 */
+		sprintf(soc_id, "N/A");
+	}
+
+	/*
+	 * As this key will never change, no need to read it anymore.
+	 */
+	si->info = soc_id;
+	return soc_id;
+}
+
+static char *ux500_get_revision(struct sys_soc_info *si)
+{
+	unsigned int rev = dbx500_revision();
+
+	if (rev == 0x01)
+		sprintf(revision, "%s", "ED");
+	else if (rev >= 0xA0)
+		sprintf(revision, "%d.%d" , (rev >> 4) - 0xA + 1, rev & 0xf);
+	else
+		sprintf(revision, "%s", "Unknown");
+
+	/*
+	 * As this key will never change, no need to read it anymore.
+	 */
+	si->info = revision;
+	return revision;
+}
+
+static char *ux500_get_process(struct sys_soc_info *si)
+{
+	if (dbx500_id.process == 0x00)
+		sprintf(process, "Standard");
+	else
+		sprintf(process, "%02xnm", dbx500_id.process);
+
+	/*
+	 * As this key will never change, no need to read it anymore.
+	 */
+	si->info = process;
+	return process;
+}
+
+static struct sys_soc_info soc_info[] = {
+	[0] = {
+		.name = "soc_id",
+		.get_info = ux500_get_soc_id,
+	},
+	[1] = {
+		.name = "revision",
+		.get_info = ux500_get_revision,
+	},
+	[2] = {
+		.name = "process",
+		.get_info = ux500_get_process,
+	},
+};
+
+static int __init ux500_sys_soc_init(void)
+{
+	char part_number[8];
+
+	sprintf(part_number, "DB%4x", dbx500_partnumber());
+	register_sys_soc(part_number, soc_info, ARRAY_SIZE(soc_info));
+
+	return 0;
+}
+
+module_init(ux500_sys_soc_init);
+#endif
+
-- 
1.7.1

[RFC PATCHv1 2/2] ux500: Export U8500 SoC info through sysfs

[Arnd Bergmann]

On Wednesday 09 March 2011 17:59:21 Maxime Coquelin wrote:
> ST-Ericsson's U8500 implementation.
> Register sysfs SoC interface, and export SoC ID, process and silicon revision
> number.

> +static char *ux500_get_soc_id(struct sys_soc_info *si)
> +{
> +	void __iomem *uid_base = __io_address(U8500_BB_UID_BASE);

> +static char *ux500_get_revision(struct sys_soc_info *si)
> +{
> +	unsigned int rev = dbx500_revision();
>
> +
> +static char *ux500_get_process(struct sys_soc_info *si)
> +{

These should all just be attributes of the platform device that
represents your SoC.

	Arnd

[RFC PATCHv1 0/2] Export SoC info through sysfs

[Eduardo Valentin]

Hi,

Back after long silent :-)

On Wed, Mar 09, 2011 at 05:59:19PM +0100, ext Maxime Coquelin wrote:
> Here is the first version of the proposal to export SoC related information to user-space through sysFS interface.
> 
> This serie is to continue what has been discussed on the "socinfo" thread created by Eduardo Valentin:
> https://lkml.org/lkml/2010/5/11/364

Thanks for taking this further. Looks much more promising now :-).

> 
> The first patch introduces the common part, which provides an interface to the platform to register its name, and exports platform-defined IDs to user-space.
> The IDs strings can be provided in two ways: either with a pointer to the string, or by a callback returning the string.

Do you mind refreshing my memory why we need two ways of providing data to these attributes?
I mean, I think if we provide the attribute value during registration time should be enough
and simpler.

Unless you guys are talking about attributes with changes over the time, I don't really see
the need for the callback there. At least from the original scope, I don't see any attributes
which would be exported under soc info which would change over the time.

Or am I missing something?

> 
> The second patch is given as example for the ux500 architecture. It registers with the machine name "DB8500", and exports 3 informations (SoC unique ID, sili
> con revision and silicon process).
> 
> Here is the output for DB8500:
> root at ME:/sys/devices/system/soc ls -l
> -r--r--r--    1 root     root         4096 Jan 11 02:43 mach_name
> -r--r--r--    1 root     root         4096 Jan 11 02:43 process
> -r--r--r--    1 root     root         4096 Jan 11 02:43 revision
> -r--r--r--    1 root     root         4096 Jan 11 02:43 soc_id
> root at ME:/sys/devices/system/soc cat mach_name
> DB8500
> root at ME:/sys/devices/system/soc cat process
> Standard
> root at ME:/sys/devices/system/soc cat revision
> 2.0
> root at ME:/sys/devices/system/soc cat soc_id
> 2ba07ce9e5835d6185321e9577462ef2ea2129cf
> 
> Any comments are welcome.
> 
> Note that I will be off from 13th to 21th of March.
> 
> Regards,
> Maxime
> 
> -------------------------------------------------------------------------------
> 
> Maxime Coquelin (2):
>   Export SoC info through sysfs
>   ux500: Export U8500 SoC info through sysfs
> 
>  arch/arm/mach-ux500/id.c |   96 ++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/base/Kconfig     |    4 ++
>  drivers/base/Makefile    |    1 +
>  drivers/base/soc.c       |   88 ++++++++++++++++++++++++++++++++++++++++++
>  include/linux/sys_soc.h  |   33 ++++++++++++++++
>  5 files changed, 222 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/base/soc.c
>  create mode 100644 include/linux/sys_soc.h

-- 
Eduardo Valentin

[RFC PATCHv1 0/2] Export SoC info through sysfs

[Maxime Coquelin]

Hi Eduardo,

On 03/10/2011 02:05 PM, Eduardo Valentin wrote:
>
>> The first patch introduces the common part, which provides an interface to the platform to register its name, and exports platform-defined IDs to user-space.
>> The IDs strings can be provided in two ways: either with a pointer to the string, or by a callback returning the string.
> Do you mind refreshing my memory why we need two ways of providing data to these attributes?
> I mean, I think if we provide the attribute value during registration time should be enough
> and simpler.
>
> Unless you guys are talking about attributes with changes over the time, I don't really see
> the need for the callback there. At least from the original scope, I don't see any attributes
> which would be exported under soc info which would change over the time.
>
> Or am I missing something?


It would be simpler indeed. But I don't know what are other SoC vendors 
needs, does someone need to export a piece of information which may 
change at runtime? If we all agree that providing a string at 
registration is enough, I'll remove it.