[PATCH] base: add sysfs socs info

[PATCH] base: add sysfs socs info

[Jean-Christophe PLAGNIOL-VILLARD]

this provide an easy way to register soc information

arch, family, model, id, revision

as this for at91sam9g20

$ cat /sys/devices/system/soc/soc0/arch
current
$ cat /sys/devices/system/soc/soc0/family
at91
$ cat /sys/devices/system/soc/soc0/id
at91sam9g20
$ cat /sys/devices/system/soc/soc0/model
0x00000000019905a0
$ cat /sys/devices/system/soc/soc0/revision
1.1

Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
Cc: Patrice VILCHEZ <patrice.vilchez@atmel.com>
---
 drivers/base/Makefile |    3 +-
 drivers/base/base.h   |    1 +
 drivers/base/init.c   |    1 +
 drivers/base/soc.c    |  124 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/soc.h   |   27 +++++++++++
 5 files changed, 155 insertions(+), 1 deletions(-)
 create mode 100644 drivers/base/soc.c
 create mode 100644 include/linux/soc.h

diff --git a/drivers/base/Makefile b/drivers/base/Makefile
index 5f51c3b..cf3e59f 100644
--- a/drivers/base/Makefile
+++ b/drivers/base/Makefile
@@ -3,7 +3,8 @@
 obj-y			:= core.o sys.o bus.o dd.o \
 			   driver.o class.o platform.o \
 			   cpu.o firmware.o init.o map.o devres.o \
-			   attribute_container.o transport_class.o
+			   attribute_container.o transport_class.o \
+			   soc.o
 obj-$(CONFIG_DEVTMPFS)	+= devtmpfs.o
 obj-y			+= power/
 obj-$(CONFIG_HAS_DMA)	+= dma-mapping.o
diff --git a/drivers/base/base.h b/drivers/base/base.h
index 2ca7f5b..e2daaf6 100644
--- a/drivers/base/base.h
+++ b/drivers/base/base.h
@@ -107,6 +107,7 @@ static inline int hypervisor_init(void) { return 0; }
 extern int platform_bus_init(void);
 extern int system_bus_init(void);
 extern int cpu_dev_init(void);
+extern int soc_dev_init(void);
 
 extern int bus_add_device(struct device *dev);
 extern void bus_probe_device(struct device *dev);
diff --git a/drivers/base/init.c b/drivers/base/init.c
index c8a934e..f908faa 100644
--- a/drivers/base/init.c
+++ b/drivers/base/init.c
@@ -33,5 +33,6 @@ void __init driver_init(void)
 	platform_bus_init();
 	system_bus_init();
 	cpu_dev_init();
+	soc_dev_init();
 	memory_dev_init();
 }
diff --git a/drivers/base/soc.c b/drivers/base/soc.c
new file mode 100644
index 0000000..c24bb41
--- /dev/null
+++ b/drivers/base/soc.c
@@ -0,0 +1,124 @@
+/*
+ * drivers/base/soc.c - basic SOC class support
+ *
+ * Copyright (C) 2010 Jean-Chrisotphe PLAGNIOL-VILLARD * <plagnioj@jcrosoft.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/sysdev.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/soc.h>
+#include <linux/device.h>
+
+#include "base.h"
+
+static int nb_socs;
+
+struct sysdev_class soc_sysdev_class = {
+	.name = "soc",
+};
+EXPORT_SYMBOL_GPL(soc_sysdev_class);
+
+#define print_u64_attr(field)							\
+static ssize_t print_socs_##field(struct sys_device *dev,			\
+				  struct sysdev_attribute *attr, char *buf)	\
+{										\
+	struct soc *soc = container_of(dev, struct soc, sysdev);		\
+										\
+	return sprintf(buf, "0x%016Lx\n", soc->field);				\
+}										\
+static SYSDEV_ATTR(field, 0444, print_socs_##field, NULL);			\
+
+#define print_str_attr(field)							\
+static ssize_t print_socs_##field(struct sys_device *dev,			\
+				  struct sysdev_attribute *attr, char *buf)	\
+{										\
+	struct soc *soc = container_of(dev, struct soc, sysdev);		\
+										\
+	return sprintf(buf, "%s\n", soc->field);				\
+}										\
+static SYSDEV_ATTR(field, 0444, print_socs_##field, NULL);			\
+
+print_u64_attr(id)
+print_str_attr(arch)
+print_str_attr(family)
+print_str_attr(model)
+print_str_attr(revision)
+
+static char *arch_current = "current";
+/*
+ * register_soc - Setup a sysfs device for a SOC.
+ *
+ * Initialize and register the SOC device.
+ */
+int register_soc(struct soc *soc)
+{
+	int err;
+
+	if (!soc)
+		return -EINVAL;
+
+	soc->sysdev.id = nb_socs;
+	soc->sysdev.cls = &soc_sysdev_class;
+
+	if (!soc->arch)
+		soc->arch = arch_current;
+
+	err = sysdev_register(&soc->sysdev);
+
+	if (err)
+		return err;
+
+	err = sysdev_create_file(&soc->sysdev, &attr_arch);
+
+	if (err)
+		goto end;
+
+	err = sysdev_create_file(&soc->sysdev, &attr_family);
+
+	if (err)
+		goto end0;
+
+	err = sysdev_create_file(&soc->sysdev, &attr_model);
+
+	if (err)
+		goto end1;
+
+	err = sysdev_create_file(&soc->sysdev, &attr_id);
+
+	if (err)
+		goto end2;
+
+	err = sysdev_create_file(&soc->sysdev, &attr_revision);
+
+	if (err)
+		goto end3;
+
+	nb_socs++;
+
+	return 0;
+
+end3:
+	sysdev_remove_file(&soc->sysdev, &attr_id);
+end2:
+	sysdev_remove_file(&soc->sysdev, &attr_model);
+end1:
+	sysdev_remove_file(&soc->sysdev, &attr_family);
+end0:
+	sysdev_remove_file(&soc->sysdev, &attr_arch);
+end:
+	sysdev_unregister(&soc->sysdev);
+
+	return err;
+}
+
+int __init soc_dev_init(void)
+{
+	nb_socs = 0;
+
+	return sysdev_class_register(&soc_sysdev_class);
+}
diff --git a/include/linux/soc.h b/include/linux/soc.h
new file mode 100644
index 0000000..55e6ea2
--- /dev/null
+++ b/include/linux/soc.h
@@ -0,0 +1,27 @@
+/*
+ * include/linux/soc.h - generic soc definition
+ *
+ * Copyright (C) 2010 Jean-Chrisotphe PLAGNIOL-VILLARD * <plagnioj@jcrosoft.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+#ifndef _LINUX_SOC_H_
+#define _LINUX_OSC_H_
+
+#include <linux/sysdev.h>
+
+struct soc {
+	u64 id;
+	char *arch;
+	char *family;
+	char *model;
+	char *revision;
+	struct sys_device sysdev;
+};
+
+extern int register_soc(struct soc *soc);
+extern struct sysdev_class soc_sysdev_class;
+
+#endif /* _LINUX_SOC_H_ */
-- 
1.7.2.3

Re: [PATCH] base: add sysfs socs info

[Greg KH]

On Tue, Dec 14, 2010 at 01:40:17PM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> this provide an easy way to register soc information
> 
> arch, family, model, id, revision
> 
> as this for at91sam9g20
> 
> $ cat /sys/devices/system/soc/soc0/arch
> current
> $ cat /sys/devices/system/soc/soc0/family
> at91
> $ cat /sys/devices/system/soc/soc0/id
> at91sam9g20
> $ cat /sys/devices/system/soc/soc0/model
> 0x00000000019905a0
> $ cat /sys/devices/system/soc/soc0/revision
> 1.1

What is this for?  When you add sysfs files, you are required to add
entries to the Documentation/ABI/ files as well, please always provide
this so we are able to review the code easier.

> +/*
> + * register_soc - Setup a sysfs device for a SOC.
> + *
> + * Initialize and register the SOC device.
> + */
> +int register_soc(struct soc *soc)
> +{
> +	int err;
> +
> +	if (!soc)
> +		return -EINVAL;
> +
> +	soc->sysdev.id = nb_socs;
> +	soc->sysdev.cls = &soc_sysdev_class;
> +
> +	if (!soc->arch)
> +		soc->arch = arch_current;
> +
> +	err = sysdev_register(&soc->sysdev);
> +
> +	if (err)
> +		return err;
> +
> +	err = sysdev_create_file(&soc->sysdev, &attr_arch);
> +
> +	if (err)
> +		goto end;

<snip>

Please use an attribute group, it makes the code simpler and easier to
read.

> +struct soc {
> +	u64 id;
> +	char *arch;
> +	char *family;
> +	char *model;
> +	char *revision;
> +	struct sys_device sysdev;
> +};

What is a "SOC"?  A "Small Ordinary Creature"?

And does every system have one of these?  Just one?  Not multiple?  We
need a whole lot more information here as to why this code is needed,
and who will be using it.

Also, isn't this information already in /proc/cpu/ today?

thanks,

greg k-h

Re: [PATCH] base: add sysfs socs info

[Jean-Christophe PLAGNIOL-VILLARD]

On 08:11 Tue 14 Dec     , Greg KH wrote:
> On Tue, Dec 14, 2010 at 01:40:17PM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > this provide an easy way to register soc information
> > 
> > arch, family, model, id, revision
> > 
> > as this for at91sam9g20
> > 
> > $ cat /sys/devices/system/soc/soc0/arch
> > current
> > $ cat /sys/devices/system/soc/soc0/family
> > at91
> > $ cat /sys/devices/system/soc/soc0/id
> > at91sam9g20
> > $ cat /sys/devices/system/soc/soc0/model
> > 0x00000000019905a0
> > $ cat /sys/devices/system/soc/soc0/revision
> > 1.1
> 
> What is this for?  When you add sysfs files, you are required to add
> entries to the Documentation/ABI/ files as well, please always provide
> this so we are able to review the code easier.
I'll add a doc in Documentation/ABI
 
> > +struct soc {
> > +	u64 id;
> > +	char *arch;
> > +	char *family;
> > +	char *model;
> > +	char *revision;
> > +	struct sys_device sysdev;
> > +};
> 
> What is a "SOC"?  A "Small Ordinary Creature"?
:) Systen On Chip
> 
> And does every system have one of these?  Just one?  Not multiple?  We
> need a whole lot more information here as to why this code is needed,
> and who will be using it.
> 
> Also, isn't this information already in /proc/cpu/ today?
no all this information are not present in /proc/cpu and a cpu is not a soc

basicaly on a board you will have one soc but some boards have more as example
some ST boards you have two sh4 soc or on some amcc you have two powerpc soc

with these information you are allow to dynamise the userspace based on the
soc or the companion soc and also be able to display this information in a UI

Best Regards,
J.

Re: [PATCH] base: add sysfs socs info

[Greg KH]

On Wed, Dec 15, 2010 at 02:25:11AM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 08:11 Tue 14 Dec     , Greg KH wrote:
> > On Tue, Dec 14, 2010 at 01:40:17PM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > this provide an easy way to register soc information
> > > 
> > > arch, family, model, id, revision
> > > 
> > > as this for at91sam9g20
> > > 
> > > $ cat /sys/devices/system/soc/soc0/arch
> > > current
> > > $ cat /sys/devices/system/soc/soc0/family
> > > at91
> > > $ cat /sys/devices/system/soc/soc0/id
> > > at91sam9g20
> > > $ cat /sys/devices/system/soc/soc0/model
> > > 0x00000000019905a0
> > > $ cat /sys/devices/system/soc/soc0/revision
> > > 1.1
> > 
> > What is this for?  When you add sysfs files, you are required to add
> > entries to the Documentation/ABI/ files as well, please always provide
> > this so we are able to review the code easier.
> I'll add a doc in Documentation/ABI
>  
> > > +struct soc {
> > > +	u64 id;
> > > +	char *arch;
> > > +	char *family;
> > > +	char *model;
> > > +	char *revision;
> > > +	struct sys_device sysdev;
> > > +};
> > 
> > What is a "SOC"?  A "Small Ordinary Creature"?
> :) Systen On Chip
> > 
> > And does every system have one of these?  Just one?  Not multiple?  We
> > need a whole lot more information here as to why this code is needed,
> > and who will be using it.
> > 
> > Also, isn't this information already in /proc/cpu/ today?
> no all this information are not present in /proc/cpu and a cpu is not a soc
> 
> basicaly on a board you will have one soc but some boards have more as example
> some ST boards you have two sh4 soc or on some amcc you have two powerpc soc
> 
> with these information you are allow to dynamise the userspace based on the
> soc or the companion soc and also be able to display this information in a UI

Ok, but then why not make it "real" devices and not sysdev?  We are
trying to fix the sysdev code to be semi-real devices, so you might as
well make them real ones now.

thanks,

greg k-h

Re: [PATCH] base: add sysfs socs info

[Jean-Christophe PLAGNIOL-VILLARD]

> > > 
> > > Also, isn't this information already in /proc/cpu/ today?
> > no all this information are not present in /proc/cpu and a cpu is not a soc
> > 
> > basicaly on a board you will have one soc but some boards have more as example
> > some ST boards you have two sh4 soc or on some amcc you have two powerpc soc
> > 
> > with these information you are allow to dynamise the userspace based on the
> > soc or the companion soc and also be able to display this information in a UI
> 
> Ok, but then why not make it "real" devices and not sysdev?  We are
> trying to fix the sysdev code to be semi-real devices, so you might as
> well make them real ones now.
ok I base my work on drivers/base/cpu.c

where we also use sysdev

can we point me a code how you want I implement it

Best Regards,
J.

Re: [PATCH] base: add sysfs socs info

[Kay Sievers]

On Wed, Dec 15, 2010 at 08:39, Jean-Christophe PLAGNIOL-VILLARD
<plagnioj@jcrosoft.com> wrote:
>> > >
>> > > Also, isn't this information already in /proc/cpu/ today?
>> > no all this information are not present in /proc/cpu and a cpu is not a soc
>> >
>> > basicaly on a board you will have one soc but some boards have more as example
>> > some ST boards you have two sh4 soc or on some amcc you have two powerpc soc
>> >
>> > with these information you are allow to dynamise the userspace based on the
>> > soc or the companion soc and also be able to display this information in a UI
>>
>> Ok, but then why not make it "real" devices and not sysdev?  We are
>> trying to fix the sysdev code to be semi-real devices, so you might as
>> well make them real ones now.
> ok I base my work on drivers/base/cpu.c
>
> where we also use sysdev
>
> can we point me a code how you want I implement it

Sysdevs devices in general are really broken concept. They duplicate
all the usual class/bus/subsystem logic in some stupid custom way,
which does not fit into the rest of the driver model. They have a have
broken event handling, so that  udev/libudev can not even "see" them
properly.

Create your own bus "soc", and register a device "id" there which has
these attributes in a group, which is assigned to the device before it
is registered.

Or maybe (really no idea ow how that fits) use the "dmi" class's
device "id", if that fits, or create a "soc" device in the "dmi" class
that has all these attributes.

Kay

Re : [PATCH] base: add sysfs socs info

[Didier Lagard]

<plagnioj@jcrosoft.com> a écrit :
>
> this provide an easy way to register
> soc information
> 
> arch, family, model, id, revision
> 
> as this for at91sam9g20
> 
> $ cat /sys/devices/system/soc/soc0/arch
> current
> $ cat /sys/devices/system/soc/soc0/family
> at91
> $ cat /sys/devices/system/soc/soc0/id
> at91sam9g20
> $ cat /sys/devices/system/soc/soc0/model
> 0x00000000019905a0
> $ cat /sys/devices/system/soc/soc0/revision
> 1.1

Ouch, I've never seen so few explanations when it comes to add a new ABI...

Can you explain why this is needed at all ? How can it be usefull for an application for example ? Why make it a device ?

I mean there are plenty of SoC supports in the kernel (take a look to arch/arm) and none of them need that so far.

So at least we can expect some motivations to include this stuff in mailine.

Bye.


      

Re: [PATCH] base: add sysfs socs info

[Ryan Mallon]

On 12/15/2010 01:40 AM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> this provide an easy way to register soc information

This idea has been kicked around a few times in various forms, both as a
proc file and as sysfs files. Cc'ed the arm-linux and embedded-linux
lists since this patch mainly affects them.

> arch, family, model, id, revision

Some SoCs want to expose additional information also. This patch doesn't
appear to provide any standard way of extending the information
available in sysfs.

> as this for at91sam9g20
> 
> $ cat /sys/devices/system/soc/soc0/arch
> current

What does this mean? Shouldn't it be ARM? Also, we already have ways to
determine the architecture/cpu type.

> $ cat /sys/devices/system/soc/soc0/family
> at91
> $ cat /sys/devices/system/soc/soc0/id
> at91sam9g20
> $ cat /sys/devices/system/soc/soc0/model
> 0x00000000019905a0
> $ cat /sys/devices/system/soc/soc0/revision
> 1.1

What is the difference between model and revision? Do these fields make
sense for all SoCs?

What userspace tools actually need this information? Some of the
extended information for various SoCs may be useful, but I can't think
of many good reasons for a userspace application to care about the SoC
family or revision.

> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
> Cc: Patrice VILCHEZ <patrice.vilchez@atmel.com>
> ---
>  drivers/base/Makefile |    3 +-
>  drivers/base/base.h   |    1 +
>  drivers/base/init.c   |    1 +
>  drivers/base/soc.c    |  124 +++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/soc.h   |   27 +++++++++++
>  5 files changed, 155 insertions(+), 1 deletions(-)
>  create mode 100644 drivers/base/soc.c
>  create mode 100644 include/linux/soc.h
> 
> diff --git a/drivers/base/Makefile b/drivers/base/Makefile
> index 5f51c3b..cf3e59f 100644
> --- a/drivers/base/Makefile
> +++ b/drivers/base/Makefile
> @@ -3,7 +3,8 @@
>  obj-y			:= core.o sys.o bus.o dd.o \
>  			   driver.o class.o platform.o \
>  			   cpu.o firmware.o init.o map.o devres.o \
> -			   attribute_container.o transport_class.o
> +			   attribute_container.o transport_class.o \
> +			   soc.o
>  obj-$(CONFIG_DEVTMPFS)	+= devtmpfs.o
>  obj-y			+= power/
>  obj-$(CONFIG_HAS_DMA)	+= dma-mapping.o
> diff --git a/drivers/base/base.h b/drivers/base/base.h
> index 2ca7f5b..e2daaf6 100644
> --- a/drivers/base/base.h
> +++ b/drivers/base/base.h
> @@ -107,6 +107,7 @@ static inline int hypervisor_init(void) { return 0; }
>  extern int platform_bus_init(void);
>  extern int system_bus_init(void);
>  extern int cpu_dev_init(void);
> +extern int soc_dev_init(void);
>  
>  extern int bus_add_device(struct device *dev);
>  extern void bus_probe_device(struct device *dev);
> diff --git a/drivers/base/init.c b/drivers/base/init.c
> index c8a934e..f908faa 100644
> --- a/drivers/base/init.c
> +++ b/drivers/base/init.c
> @@ -33,5 +33,6 @@ void __init driver_init(void)
>  	platform_bus_init();
>  	system_bus_init();
>  	cpu_dev_init();
> +	soc_dev_init();
>  	memory_dev_init();
>  }
> diff --git a/drivers/base/soc.c b/drivers/base/soc.c
> new file mode 100644
> index 0000000..c24bb41
> --- /dev/null
> +++ b/drivers/base/soc.c
> @@ -0,0 +1,124 @@
> +/*
> + * drivers/base/soc.c - basic SOC class support
> + *
> + * Copyright (C) 2010 Jean-Chrisotphe PLAGNIOL-VILLARD * <plagnioj@jcrosoft.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/sysdev.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/soc.h>
> +#include <linux/device.h>
> +
> +#include "base.h"
> +
> +static int nb_socs;
> +
> +struct sysdev_class soc_sysdev_class = {
> +	.name = "soc",
> +};
> +EXPORT_SYMBOL_GPL(soc_sysdev_class);
> +
> +#define print_u64_attr(field)							\
> +static ssize_t print_socs_##field(struct sys_device *dev,			\
> +				  struct sysdev_attribute *attr, char *buf)	\
> +{										\
> +	struct soc *soc = container_of(dev, struct soc, sysdev);		\
> +										\
> +	return sprintf(buf, "0x%016Lx\n", soc->field);				\
> +}										\
> +static SYSDEV_ATTR(field, 0444, print_socs_##field, NULL);			\
> +
> +#define print_str_attr(field)							\
> +static ssize_t print_socs_##field(struct sys_device *dev,			\
> +				  struct sysdev_attribute *attr, char *buf)	\
> +{										\
> +	struct soc *soc = container_of(dev, struct soc, sysdev);		\
> +										\
> +	return sprintf(buf, "%s\n", soc->field);				\
> +}										\
> +static SYSDEV_ATTR(field, 0444, print_socs_##field, NULL);			\

At first glance this looks like two functions with the same name because
of the identical print_socs##field bit. I intuitively expect field to
just be the name of the sysfs file.

> +print_u64_attr(id)
> +print_str_attr(arch)
> +print_str_attr(family)
> +print_str_attr(model)
> +print_str_attr(revision)

These should have semicolons at the end (drop the final one from the
macro name). Also I think the names should be in caps and should be
renamed to better reflect what the do, i.e. SOC_SYSFS_U64_ATTR and
SOC_SYSFS_STRING_ATTR.

> +static char *arch_current = "current";

Should be const.

> +/*
> + * register_soc - Setup a sysfs device for a SOC.
> + *
> + * Initialize and register the SOC device.
> + */
> +int register_soc(struct soc *soc)
> +{

This name implies that it does much more than just adding some sysfs
files :-).

> +	int err;
> +
> +	if (!soc)
> +		return -EINVAL;

Wouldn't bother with this check. Just crash so that we can catch buggy code.

> +	soc->sysdev.id = nb_socs;
> +	soc->sysdev.cls = &soc_sysdev_class;
> +
> +	if (!soc->arch)
> +		soc->arch = arch_current;
> +
> +	err = sysdev_register(&soc->sysdev);
> +
> +	if (err)
> +		return err;

Why all the additional whitespace?
> +
> +	err = sysdev_create_file(&soc->sysdev, &attr_arch);
> +
> +	if (err)
> +		goto end;

You can use sysfs_create_group to register a bunch of files which will
greatly simply the code here.

> +	err = sysdev_create_file(&soc->sysdev, &attr_family);
> +
> +	if (err)
> +		goto end0;
> +
> +	err = sysdev_create_file(&soc->sysdev, &attr_model);
> +
> +	if (err)
> +		goto end1;
> +
> +	err = sysdev_create_file(&soc->sysdev, &attr_id);
> +
> +	if (err)
> +		goto end2;
> +
> +	err = sysdev_create_file(&soc->sysdev, &attr_revision);
> +
> +	if (err)
> +		goto end3;
> +
> +	nb_socs++;

If there is more than one SoC (SMP machine?) then how do you guarantee
the order of registration? Should the registration function take id as a
parameter?

> +	return 0;
> +
> +end3:
> +	sysdev_remove_file(&soc->sysdev, &attr_id);
> +end2:
> +	sysdev_remove_file(&soc->sysdev, &attr_model);
> +end1:
> +	sysdev_remove_file(&soc->sysdev, &attr_family);
> +end0:
> +	sysdev_remove_file(&soc->sysdev, &attr_arch);
> +end:
> +	sysdev_unregister(&soc->sysdev);
> +
> +	return err;
> +}

EXPORT_SYMBOL(register_soc)?

> +
> +int __init soc_dev_init(void)
> +{
> +	nb_socs = 0;
> +
> +	return sysdev_class_register(&soc_sysdev_class);
> +}

EXPORT_SYMBOL(soc_dev_init)?

> diff --git a/include/linux/soc.h b/include/linux/soc.h
> new file mode 100644
> index 0000000..55e6ea2
> --- /dev/null
> +++ b/include/linux/soc.h
> @@ -0,0 +1,27 @@
> +/*
> + * include/linux/soc.h - generic soc definition
> + *
> + * Copyright (C) 2010 Jean-Chrisotphe PLAGNIOL-VILLARD * <plagnioj@jcrosoft.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +#ifndef _LINUX_SOC_H_
> +#define _LINUX_OSC_H_
> +
> +#include <linux/sysdev.h>
> +
> +struct soc {
> +	u64 id;
> +	char *arch;
> +	char *family;
> +	char *model;
> +	char *revision;
> +	struct sys_device sysdev;
> +};
> +
> +extern int register_soc(struct soc *soc);
> +extern struct sysdev_class soc_sysdev_class;
> +
> +#endif /* _LINUX_SOC_H_ */

~Ryan

-- 
Bluewater Systems Ltd - ARM Technology Solution Centre

Ryan Mallon         		5 Amuri Park, 404 Barbadoes St
ryan@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

Re: [PATCH] base: add sysfs socs info

[Ryan Mallon]

On 12/16/2010 09:17 AM, Ryan Mallon wrote:
> On 12/15/2010 01:40 AM, Jean-Christophe PLAGNIOL-VILLARD wrote:
>> this provide an easy way to register soc information
> 
> This idea has been kicked around a few times in various forms, both as a
> proc file and as sysfs files. Cc'ed the arm-linux and embedded-linux
> lists since this patch mainly affects them.

Gah, my email client still has the old arm-linux address cached. Cc'ed
the new one.

~Ryan

>> arch, family, model, id, revision
> 
> Some SoCs want to expose additional information also. This patch doesn't
> appear to provide any standard way of extending the information
> available in sysfs.
> 
>> as this for at91sam9g20
>>
>> $ cat /sys/devices/system/soc/soc0/arch
>> current
> 
> What does this mean? Shouldn't it be ARM? Also, we already have ways to
> determine the architecture/cpu type.
> 
>> $ cat /sys/devices/system/soc/soc0/family
>> at91
>> $ cat /sys/devices/system/soc/soc0/id
>> at91sam9g20
>> $ cat /sys/devices/system/soc/soc0/model
>> 0x00000000019905a0
>> $ cat /sys/devices/system/soc/soc0/revision
>> 1.1
> 
> What is the difference between model and revision? Do these fields make
> sense for all SoCs?
> 
> What userspace tools actually need this information? Some of the
> extended information for various SoCs may be useful, but I can't think
> of many good reasons for a userspace application to care about the SoC
> family or revision.
> 
>> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
>> Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
>> Cc: Patrice VILCHEZ <patrice.vilchez@atmel.com>
>> ---
>>  drivers/base/Makefile |    3 +-
>>  drivers/base/base.h   |    1 +
>>  drivers/base/init.c   |    1 +
>>  drivers/base/soc.c    |  124 +++++++++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/soc.h   |   27 +++++++++++
>>  5 files changed, 155 insertions(+), 1 deletions(-)
>>  create mode 100644 drivers/base/soc.c
>>  create mode 100644 include/linux/soc.h
>>
>> diff --git a/drivers/base/Makefile b/drivers/base/Makefile
>> index 5f51c3b..cf3e59f 100644
>> --- a/drivers/base/Makefile
>> +++ b/drivers/base/Makefile
>> @@ -3,7 +3,8 @@
>>  obj-y			:= core.o sys.o bus.o dd.o \
>>  			   driver.o class.o platform.o \
>>  			   cpu.o firmware.o init.o map.o devres.o \
>> -			   attribute_container.o transport_class.o
>> +			   attribute_container.o transport_class.o \
>> +			   soc.o
>>  obj-$(CONFIG_DEVTMPFS)	+= devtmpfs.o
>>  obj-y			+= power/
>>  obj-$(CONFIG_HAS_DMA)	+= dma-mapping.o
>> diff --git a/drivers/base/base.h b/drivers/base/base.h
>> index 2ca7f5b..e2daaf6 100644
>> --- a/drivers/base/base.h
>> +++ b/drivers/base/base.h
>> @@ -107,6 +107,7 @@ static inline int hypervisor_init(void) { return 0; }
>>  extern int platform_bus_init(void);
>>  extern int system_bus_init(void);
>>  extern int cpu_dev_init(void);
>> +extern int soc_dev_init(void);
>>  
>>  extern int bus_add_device(struct device *dev);
>>  extern void bus_probe_device(struct device *dev);
>> diff --git a/drivers/base/init.c b/drivers/base/init.c
>> index c8a934e..f908faa 100644
>> --- a/drivers/base/init.c
>> +++ b/drivers/base/init.c
>> @@ -33,5 +33,6 @@ void __init driver_init(void)
>>  	platform_bus_init();
>>  	system_bus_init();
>>  	cpu_dev_init();
>> +	soc_dev_init();
>>  	memory_dev_init();
>>  }
>> diff --git a/drivers/base/soc.c b/drivers/base/soc.c
>> new file mode 100644
>> index 0000000..c24bb41
>> --- /dev/null
>> +++ b/drivers/base/soc.c
>> @@ -0,0 +1,124 @@
>> +/*
>> + * drivers/base/soc.c - basic SOC class support
>> + *
>> + * Copyright (C) 2010 Jean-Chrisotphe PLAGNIOL-VILLARD * <plagnioj@jcrosoft.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +
>> +#include <linux/sysdev.h>
>> +#include <linux/module.h>
>> +#include <linux/init.h>
>> +#include <linux/soc.h>
>> +#include <linux/device.h>
>> +
>> +#include "base.h"
>> +
>> +static int nb_socs;
>> +
>> +struct sysdev_class soc_sysdev_class = {
>> +	.name = "soc",
>> +};
>> +EXPORT_SYMBOL_GPL(soc_sysdev_class);
>> +
>> +#define print_u64_attr(field)							\
>> +static ssize_t print_socs_##field(struct sys_device *dev,			\
>> +				  struct sysdev_attribute *attr, char *buf)	\
>> +{										\
>> +	struct soc *soc = container_of(dev, struct soc, sysdev);		\
>> +										\
>> +	return sprintf(buf, "0x%016Lx\n", soc->field);				\
>> +}										\
>> +static SYSDEV_ATTR(field, 0444, print_socs_##field, NULL);			\
>> +
>> +#define print_str_attr(field)							\
>> +static ssize_t print_socs_##field(struct sys_device *dev,			\
>> +				  struct sysdev_attribute *attr, char *buf)	\
>> +{										\
>> +	struct soc *soc = container_of(dev, struct soc, sysdev);		\
>> +										\
>> +	return sprintf(buf, "%s\n", soc->field);				\
>> +}										\
>> +static SYSDEV_ATTR(field, 0444, print_socs_##field, NULL);			\
> 
> At first glance this looks like two functions with the same name because
> of the identical print_socs##field bit. I intuitively expect field to
> just be the name of the sysfs file.
> 
>> +print_u64_attr(id)
>> +print_str_attr(arch)
>> +print_str_attr(family)
>> +print_str_attr(model)
>> +print_str_attr(revision)
> 
> These should have semicolons at the end (drop the final one from the
> macro name). Also I think the names should be in caps and should be
> renamed to better reflect what the do, i.e. SOC_SYSFS_U64_ATTR and
> SOC_SYSFS_STRING_ATTR.
> 
>> +static char *arch_current = "current";
> 
> Should be const.
> 
>> +/*
>> + * register_soc - Setup a sysfs device for a SOC.
>> + *
>> + * Initialize and register the SOC device.
>> + */
>> +int register_soc(struct soc *soc)
>> +{
> 
> This name implies that it does much more than just adding some sysfs
> files :-).
> 
>> +	int err;
>> +
>> +	if (!soc)
>> +		return -EINVAL;
> 
> Wouldn't bother with this check. Just crash so that we can catch buggy code.
> 
>> +	soc->sysdev.id = nb_socs;
>> +	soc->sysdev.cls = &soc_sysdev_class;
>> +
>> +	if (!soc->arch)
>> +		soc->arch = arch_current;
>> +
>> +	err = sysdev_register(&soc->sysdev);
>> +
>> +	if (err)
>> +		return err;
> 
> Why all the additional whitespace?
>> +
>> +	err = sysdev_create_file(&soc->sysdev, &attr_arch);
>> +
>> +	if (err)
>> +		goto end;
> 
> You can use sysfs_create_group to register a bunch of files which will
> greatly simply the code here.
> 
>> +	err = sysdev_create_file(&soc->sysdev, &attr_family);
>> +
>> +	if (err)
>> +		goto end0;
>> +
>> +	err = sysdev_create_file(&soc->sysdev, &attr_model);
>> +
>> +	if (err)
>> +		goto end1;
>> +
>> +	err = sysdev_create_file(&soc->sysdev, &attr_id);
>> +
>> +	if (err)
>> +		goto end2;
>> +
>> +	err = sysdev_create_file(&soc->sysdev, &attr_revision);
>> +
>> +	if (err)
>> +		goto end3;
>> +
>> +	nb_socs++;
> 
> If there is more than one SoC (SMP machine?) then how do you guarantee
> the order of registration? Should the registration function take id as a
> parameter?
> 
>> +	return 0;
>> +
>> +end3:
>> +	sysdev_remove_file(&soc->sysdev, &attr_id);
>> +end2:
>> +	sysdev_remove_file(&soc->sysdev, &attr_model);
>> +end1:
>> +	sysdev_remove_file(&soc->sysdev, &attr_family);
>> +end0:
>> +	sysdev_remove_file(&soc->sysdev, &attr_arch);
>> +end:
>> +	sysdev_unregister(&soc->sysdev);
>> +
>> +	return err;
>> +}
> 
> EXPORT_SYMBOL(register_soc)?
> 
>> +
>> +int __init soc_dev_init(void)
>> +{
>> +	nb_socs = 0;
>> +
>> +	return sysdev_class_register(&soc_sysdev_class);
>> +}
> 
> EXPORT_SYMBOL(soc_dev_init)?
> 
>> diff --git a/include/linux/soc.h b/include/linux/soc.h
>> new file mode 100644
>> index 0000000..55e6ea2
>> --- /dev/null
>> +++ b/include/linux/soc.h
>> @@ -0,0 +1,27 @@
>> +/*
>> + * include/linux/soc.h - generic soc definition
>> + *
>> + * Copyright (C) 2010 Jean-Chrisotphe PLAGNIOL-VILLARD * <plagnioj@jcrosoft.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +#ifndef _LINUX_SOC_H_
>> +#define _LINUX_OSC_H_
>> +
>> +#include <linux/sysdev.h>
>> +
>> +struct soc {
>> +	u64 id;
>> +	char *arch;
>> +	char *family;
>> +	char *model;
>> +	char *revision;
>> +	struct sys_device sysdev;
>> +};
>> +
>> +extern int register_soc(struct soc *soc);
>> +extern struct sysdev_class soc_sysdev_class;
>> +
>> +#endif /* _LINUX_SOC_H_ */
> 
> ~Ryan
> 


-- 
Bluewater Systems Ltd - ARM Technology Solution Centre

Ryan Mallon         		5 Amuri Park, 404 Barbadoes St
ryan@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

[PATCH] base: add sysfs socs info

[Ryan Mallon]

On 12/16/2010 09:17 AM, Ryan Mallon wrote:
> On 12/15/2010 01:40 AM, Jean-Christophe PLAGNIOL-VILLARD wrote:
>> this provide an easy way to register soc information
> 
> This idea has been kicked around a few times in various forms, both as a
> proc file and as sysfs files. Cc'ed the arm-linux and embedded-linux
> lists since this patch mainly affects them.

Gah, my email client still has the old arm-linux address cached. Cc'ed
the new one.

~Ryan

>> arch, family, model, id, revision
> 
> Some SoCs want to expose additional information also. This patch doesn't
> appear to provide any standard way of extending the information
> available in sysfs.
> 
>> as this for at91sam9g20
>>
>> $ cat /sys/devices/system/soc/soc0/arch
>> current
> 
> What does this mean? Shouldn't it be ARM? Also, we already have ways to
> determine the architecture/cpu type.
> 
>> $ cat /sys/devices/system/soc/soc0/family
>> at91
>> $ cat /sys/devices/system/soc/soc0/id
>> at91sam9g20
>> $ cat /sys/devices/system/soc/soc0/model
>> 0x00000000019905a0
>> $ cat /sys/devices/system/soc/soc0/revision
>> 1.1
> 
> What is the difference between model and revision? Do these fields make
> sense for all SoCs?
> 
> What userspace tools actually need this information? Some of the
> extended information for various SoCs may be useful, but I can't think
> of many good reasons for a userspace application to care about the SoC
> family or revision.
> 
>> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
>> Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
>> Cc: Patrice VILCHEZ <patrice.vilchez@atmel.com>
>> ---
>>  drivers/base/Makefile |    3 +-
>>  drivers/base/base.h   |    1 +
>>  drivers/base/init.c   |    1 +
>>  drivers/base/soc.c    |  124 +++++++++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/soc.h   |   27 +++++++++++
>>  5 files changed, 155 insertions(+), 1 deletions(-)
>>  create mode 100644 drivers/base/soc.c
>>  create mode 100644 include/linux/soc.h
>>
>> diff --git a/drivers/base/Makefile b/drivers/base/Makefile
>> index 5f51c3b..cf3e59f 100644
>> --- a/drivers/base/Makefile
>> +++ b/drivers/base/Makefile
>> @@ -3,7 +3,8 @@
>>  obj-y			:= core.o sys.o bus.o dd.o \
>>  			   driver.o class.o platform.o \
>>  			   cpu.o firmware.o init.o map.o devres.o \
>> -			   attribute_container.o transport_class.o
>> +			   attribute_container.o transport_class.o \
>> +			   soc.o
>>  obj-$(CONFIG_DEVTMPFS)	+= devtmpfs.o
>>  obj-y			+= power/
>>  obj-$(CONFIG_HAS_DMA)	+= dma-mapping.o
>> diff --git a/drivers/base/base.h b/drivers/base/base.h
>> index 2ca7f5b..e2daaf6 100644
>> --- a/drivers/base/base.h
>> +++ b/drivers/base/base.h
>> @@ -107,6 +107,7 @@ static inline int hypervisor_init(void) { return 0; }
>>  extern int platform_bus_init(void);
>>  extern int system_bus_init(void);
>>  extern int cpu_dev_init(void);
>> +extern int soc_dev_init(void);
>>  
>>  extern int bus_add_device(struct device *dev);
>>  extern void bus_probe_device(struct device *dev);
>> diff --git a/drivers/base/init.c b/drivers/base/init.c
>> index c8a934e..f908faa 100644
>> --- a/drivers/base/init.c
>> +++ b/drivers/base/init.c
>> @@ -33,5 +33,6 @@ void __init driver_init(void)
>>  	platform_bus_init();
>>  	system_bus_init();
>>  	cpu_dev_init();
>> +	soc_dev_init();
>>  	memory_dev_init();
>>  }
>> diff --git a/drivers/base/soc.c b/drivers/base/soc.c
>> new file mode 100644
>> index 0000000..c24bb41
>> --- /dev/null
>> +++ b/drivers/base/soc.c
>> @@ -0,0 +1,124 @@
>> +/*
>> + * drivers/base/soc.c - basic SOC class support
>> + *
>> + * Copyright (C) 2010 Jean-Chrisotphe PLAGNIOL-VILLARD * <plagnioj@jcrosoft.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +
>> +#include <linux/sysdev.h>
>> +#include <linux/module.h>
>> +#include <linux/init.h>
>> +#include <linux/soc.h>
>> +#include <linux/device.h>
>> +
>> +#include "base.h"
>> +
>> +static int nb_socs;
>> +
>> +struct sysdev_class soc_sysdev_class = {
>> +	.name = "soc",
>> +};
>> +EXPORT_SYMBOL_GPL(soc_sysdev_class);
>> +
>> +#define print_u64_attr(field)							\
>> +static ssize_t print_socs_##field(struct sys_device *dev,			\
>> +				  struct sysdev_attribute *attr, char *buf)	\
>> +{										\
>> +	struct soc *soc = container_of(dev, struct soc, sysdev);		\
>> +										\
>> +	return sprintf(buf, "0x%016Lx\n", soc->field);				\
>> +}										\
>> +static SYSDEV_ATTR(field, 0444, print_socs_##field, NULL);			\
>> +
>> +#define print_str_attr(field)							\
>> +static ssize_t print_socs_##field(struct sys_device *dev,			\
>> +				  struct sysdev_attribute *attr, char *buf)	\
>> +{										\
>> +	struct soc *soc = container_of(dev, struct soc, sysdev);		\
>> +										\
>> +	return sprintf(buf, "%s\n", soc->field);				\
>> +}										\
>> +static SYSDEV_ATTR(field, 0444, print_socs_##field, NULL);			\
> 
> At first glance this looks like two functions with the same name because
> of the identical print_socs##field bit. I intuitively expect field to
> just be the name of the sysfs file.
> 
>> +print_u64_attr(id)
>> +print_str_attr(arch)
>> +print_str_attr(family)
>> +print_str_attr(model)
>> +print_str_attr(revision)
> 
> These should have semicolons at the end (drop the final one from the
> macro name). Also I think the names should be in caps and should be
> renamed to better reflect what the do, i.e. SOC_SYSFS_U64_ATTR and
> SOC_SYSFS_STRING_ATTR.
> 
>> +static char *arch_current = "current";
> 
> Should be const.
> 
>> +/*
>> + * register_soc - Setup a sysfs device for a SOC.
>> + *
>> + * Initialize and register the SOC device.
>> + */
>> +int register_soc(struct soc *soc)
>> +{
> 
> This name implies that it does much more than just adding some sysfs
> files :-).
> 
>> +	int err;
>> +
>> +	if (!soc)
>> +		return -EINVAL;
> 
> Wouldn't bother with this check. Just crash so that we can catch buggy code.
> 
>> +	soc->sysdev.id = nb_socs;
>> +	soc->sysdev.cls = &soc_sysdev_class;
>> +
>> +	if (!soc->arch)
>> +		soc->arch = arch_current;
>> +
>> +	err = sysdev_register(&soc->sysdev);
>> +
>> +	if (err)
>> +		return err;
> 
> Why all the additional whitespace?
>> +
>> +	err = sysdev_create_file(&soc->sysdev, &attr_arch);
>> +
>> +	if (err)
>> +		goto end;
> 
> You can use sysfs_create_group to register a bunch of files which will
> greatly simply the code here.
> 
>> +	err = sysdev_create_file(&soc->sysdev, &attr_family);
>> +
>> +	if (err)
>> +		goto end0;
>> +
>> +	err = sysdev_create_file(&soc->sysdev, &attr_model);
>> +
>> +	if (err)
>> +		goto end1;
>> +
>> +	err = sysdev_create_file(&soc->sysdev, &attr_id);
>> +
>> +	if (err)
>> +		goto end2;
>> +
>> +	err = sysdev_create_file(&soc->sysdev, &attr_revision);
>> +
>> +	if (err)
>> +		goto end3;
>> +
>> +	nb_socs++;
> 
> If there is more than one SoC (SMP machine?) then how do you guarantee
> the order of registration? Should the registration function take id as a
> parameter?
> 
>> +	return 0;
>> +
>> +end3:
>> +	sysdev_remove_file(&soc->sysdev, &attr_id);
>> +end2:
>> +	sysdev_remove_file(&soc->sysdev, &attr_model);
>> +end1:
>> +	sysdev_remove_file(&soc->sysdev, &attr_family);
>> +end0:
>> +	sysdev_remove_file(&soc->sysdev, &attr_arch);
>> +end:
>> +	sysdev_unregister(&soc->sysdev);
>> +
>> +	return err;
>> +}
> 
> EXPORT_SYMBOL(register_soc)?
> 
>> +
>> +int __init soc_dev_init(void)
>> +{
>> +	nb_socs = 0;
>> +
>> +	return sysdev_class_register(&soc_sysdev_class);
>> +}
> 
> EXPORT_SYMBOL(soc_dev_init)?
> 
>> diff --git a/include/linux/soc.h b/include/linux/soc.h
>> new file mode 100644
>> index 0000000..55e6ea2
>> --- /dev/null
>> +++ b/include/linux/soc.h
>> @@ -0,0 +1,27 @@
>> +/*
>> + * include/linux/soc.h - generic soc definition
>> + *
>> + * Copyright (C) 2010 Jean-Chrisotphe PLAGNIOL-VILLARD * <plagnioj@jcrosoft.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +#ifndef _LINUX_SOC_H_
>> +#define _LINUX_OSC_H_
>> +
>> +#include <linux/sysdev.h>
>> +
>> +struct soc {
>> +	u64 id;
>> +	char *arch;
>> +	char *family;
>> +	char *model;
>> +	char *revision;
>> +	struct sys_device sysdev;
>> +};
>> +
>> +extern int register_soc(struct soc *soc);
>> +extern struct sysdev_class soc_sysdev_class;
>> +
>> +#endif /* _LINUX_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