All of lore.kernel.org
 help / color / mirror / Atom feed
* Re: [PATCH 1/1] drivers/misc: Add Intel System ID driver
       [not found] <1487156981-4550-1-git-send-email-user@thloh-VirtualBox>
@ 2017-02-15 17:17 ` Greg KH
  2017-02-15 19:51   ` Arnd Bergmann
  2017-02-23  5:50   ` Loh, Tien Hock
  0 siblings, 2 replies; 11+ messages in thread
From: Greg KH @ 2017-02-15 17:17 UTC (permalink / raw)
  To: Loh, Tien Hock
  Cc: arnd, linux-kernel, thloh85, matthew.gerlach, dinh.nguyen, Loh,
	Tien Hock

On Wed, Feb 15, 2017 at 07:09:41PM +0800, thloh wrote:
> From: "Loh, Tien Hock" <tien.hock.loh@intel.com>
> 
> This patch is to add Altera System ID driver.
> User can obtain the system ID and timestamp of the system by
> reading the sysfs entry.
> 
> Usage:
> cat /sys/bus/platform/devices/[addr].sysid/sysid/id
> cat /sys/bus/platform/devices/[addr].sysid/sysid/timestamp

If you add new sysfs attributes, you need to also add a
Documentation/ABI/ description as well.

> 
> Signed-off-by: Loh, Tien Hock <tien.hock.loh@intel.com>
> ---
>  drivers/misc/Kconfig       |    5 ++
>  drivers/misc/Makefile      |    1 +
>  drivers/misc/intel_sysid.c |  142 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 148 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/misc/intel_sysid.c
> 
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index b4d6aac..e5552fa 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -139,6 +139,11 @@ config INTEL_MID_PTI
>  	  an Intel Atom (non-netbook) mobile device containing a MIPI
>  	  P1149.7 standard implementation.
>  
> +config INTEL_SYSID
> +        tristate "Intel System ID"
> +        help
> +        This enables Intel System ID soft core driver.
> +
>  config SGI_IOC4
>  	tristate "SGI IOC4 Base IO support"
>  	depends on PCI
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index 7a3ea89..24fe724 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -43,6 +43,7 @@ obj-y				+= ti-st/
>  obj-y				+= lis3lv02d/
>  obj-$(CONFIG_USB_SWITCH_FSA9480) += fsa9480.o
>  obj-$(CONFIG_ALTERA_STAPL)	+=altera-stapl/
> +obj-$(CONFIG_INTEL_SYSID)       += intel_sysid.o
>  obj-$(CONFIG_INTEL_MEI)		+= mei/
>  obj-$(CONFIG_VMWARE_VMCI)	+= vmw_vmci/
>  obj-$(CONFIG_LATTICE_ECP3_CONFIG)	+= lattice-ecp3-config.o
> diff --git a/drivers/misc/intel_sysid.c b/drivers/misc/intel_sysid.c
> new file mode 100644
> index 0000000..1ef72b8
> --- /dev/null
> +++ b/drivers/misc/intel_sysid.c
> @@ -0,0 +1,142 @@
> +/*
> + * Copyright Intel Corporation (C) 2017.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program.  If not, see <http://www.gnu.org/licenses/>.
> + *
> + * Credit:
> + * Walter Goossens
> + */
> +
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +#define DRV_NAME	"intel_sysid"
> +
> +struct intel_sysid {
> +	void __iomem		*regs;
> +};
> +
> +/* System ID Registers*/
> +#define SYSID_REG_ID		(0x0)
> +#define SYSID_REG_TIMESTAMP	(0x4)
> +
> +static ssize_t intel_sysid_show_id(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	struct intel_sysid *sysid = dev_get_drvdata(dev);
> +
> +	return sprintf(buf, "%u\n", readl(sysid->regs + SYSID_REG_ID));
> +}
> +
> +static ssize_t intel_sysid_show_timestamp(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	unsigned int reg;
> +	struct tm timestamp;
> +	struct intel_sysid *sysid = dev_get_drvdata(dev);
> +
> +	reg = readl(sysid->regs + SYSID_REG_TIMESTAMP);
> +
> +	time_to_tm(reg, 0, &timestamp);
> +
> +	return sprintf(buf, "%u (%u-%u-%u %u:%u:%u UTC)\n", reg,
> +		(unsigned int)(timestamp.tm_year + 1900),
> +		timestamp.tm_mon + 1, timestamp.tm_mday, timestamp.tm_hour,
> +		timestamp.tm_min, timestamp.tm_sec);
> +}
> +
> +static DEVICE_ATTR(id, S_IRUGO, intel_sysid_show_id, NULL);
> +static DEVICE_ATTR(timestamp, S_IRUGO, intel_sysid_show_timestamp, NULL);

DEVICE_ATTR_RO()?

> +static struct attribute *intel_sysid_attrs[] = {
> +	&dev_attr_id.attr,
> +	&dev_attr_timestamp.attr,
> +	NULL,
> +};
> +
> +struct attribute_group intel_sysid_attr_group = {
> +	.name = "sysid",
> +	.attrs = intel_sysid_attrs,
> +};

ATTRIBUTE_GROUP()?

thanks,

greg k-h

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

* Re: [PATCH 1/1] drivers/misc: Add Intel System ID driver
  2017-02-15 17:17 ` [PATCH 1/1] drivers/misc: Add Intel System ID driver Greg KH
@ 2017-02-15 19:51   ` Arnd Bergmann
  2017-02-23  5:58     ` Loh, Tien Hock
  2017-02-23  5:50   ` Loh, Tien Hock
  1 sibling, 1 reply; 11+ messages in thread
From: Arnd Bergmann @ 2017-02-15 19:51 UTC (permalink / raw)
  To: Greg KH
  Cc: Loh, Tien Hock, Linux Kernel Mailing List, thloh85,
	matthew.gerlach, dinh.nguyen

On Wed, Feb 15, 2017 at 6:17 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Wed, Feb 15, 2017 at 07:09:41PM +0800, thloh wrote:
>> From: "Loh, Tien Hock" <tien.hock.loh@intel.com>
>>
>> This patch is to add Altera System ID driver.
>> User can obtain the system ID and timestamp of the system by
>> reading the sysfs entry.
>>
>> Usage:
>> cat /sys/bus/platform/devices/[addr].sysid/sysid/id
>> cat /sys/bus/platform/devices/[addr].sysid/sysid/timestamp
>
> If you add new sysfs attributes, you need to also add a
> Documentation/ABI/ description as well.

Maybe we could pretend that this is for a SoC and use the standard soc_device
attributes as well as moving the driver into drivers/soc/?

     Arnd

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

* Re: [PATCH 1/1] drivers/misc: Add Intel System ID driver
  2017-02-15 17:17 ` [PATCH 1/1] drivers/misc: Add Intel System ID driver Greg KH
  2017-02-15 19:51   ` Arnd Bergmann
@ 2017-02-23  5:50   ` Loh, Tien Hock
  1 sibling, 0 replies; 11+ messages in thread
From: Loh, Tien Hock @ 2017-02-23  5:50 UTC (permalink / raw)
  To: gregkh; +Cc: linux-kernel, Nguyen, Dinh, thloh85, arnd, Gerlach, Matthew

Sorry for the late reply. I'll add the Documentation/ABI description,
use DEVICE_ATTR_RO and update ATTRIBUTE_GROUP. 

Thanks
Tien Hock

On Rab, 2017-02-15 at 09:17 -0800, Greg KH wrote:
> On Wed, Feb 15, 2017 at 07:09:41PM +0800, thloh wrote:
> > 
> > From: "Loh, Tien Hock" <tien.hock.loh@intel.com>
> > 
> > This patch is to add Altera System ID driver.
> > User can obtain the system ID and timestamp of the system by
> > reading the sysfs entry.
> > 
> > Usage:
> > cat /sys/bus/platform/devices/[addr].sysid/sysid/id
> > cat /sys/bus/platform/devices/[addr].sysid/sysid/timestamp
> If you add new sysfs attributes, you need to also add a
> Documentation/ABI/ description as well.
> 
> > 
> > 
> > Signed-off-by: Loh, Tien Hock <tien.hock.loh@intel.com>
> > ---
> >  drivers/misc/Kconfig       |    5 ++
> >  drivers/misc/Makefile      |    1 +
> >  drivers/misc/intel_sysid.c |  142
> > ++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 148 insertions(+), 0 deletions(-)
> >  create mode 100644 drivers/misc/intel_sysid.c
> > 
> > diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> > index b4d6aac..e5552fa 100644
> > --- a/drivers/misc/Kconfig
> > +++ b/drivers/misc/Kconfig
> > @@ -139,6 +139,11 @@ config INTEL_MID_PTI
> >  	  an Intel Atom (non-netbook) mobile device containing a
> > MIPI
> >  	  P1149.7 standard implementation.
> >  
> > +config INTEL_SYSID
> > +        tristate "Intel System ID"
> > +        help
> > +        This enables Intel System ID soft core driver.
> > +
> >  config SGI_IOC4
> >  	tristate "SGI IOC4 Base IO support"
> >  	depends on PCI
> > diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> > index 7a3ea89..24fe724 100644
> > --- a/drivers/misc/Makefile
> > +++ b/drivers/misc/Makefile
> > @@ -43,6 +43,7 @@ obj-y				+= ti-st/
> >  obj-y				+= lis3lv02d/
> >  obj-$(CONFIG_USB_SWITCH_FSA9480) += fsa9480.o
> >  obj-$(CONFIG_ALTERA_STAPL)	+=altera-stapl/
> > +obj-$(CONFIG_INTEL_SYSID)       += intel_sysid.o
> >  obj-$(CONFIG_INTEL_MEI)		+= mei/
> >  obj-$(CONFIG_VMWARE_VMCI)	+= vmw_vmci/
> >  obj-$(CONFIG_LATTICE_ECP3_CONFIG)	+= lattice-ecp3-config.o
> > diff --git a/drivers/misc/intel_sysid.c
> > b/drivers/misc/intel_sysid.c
> > new file mode 100644
> > index 0000000..1ef72b8
> > --- /dev/null
> > +++ b/drivers/misc/intel_sysid.c
> > @@ -0,0 +1,142 @@
> > +/*
> > + * Copyright Intel Corporation (C) 2017.
> > + *
> > + * This program is free software; you can redistribute it and/or
> > modify it
> > + * under the terms and conditions of the GNU General Public
> > License,
> > + * version 2, as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope it will be useful, but
> > WITHOUT
> > + * ANY WARRANTY; without even the implied warranty of
> > MERCHANTABILITY or
> > + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public
> > License for
> > + * more details.
> > + *
> > + * You should have received a copy of the GNU General Public
> > License along with
> > + * this program.  If not, see <http://www.gnu.org/licenses/>.
> > + *
> > + * Credit:
> > + * Walter Goossens
> > + */
> > +
> > +#include <linux/device.h>
> > +#include <linux/kernel.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/slab.h>
> > +
> > +#define DRV_NAME	"intel_sysid"
> > +
> > +struct intel_sysid {
> > +	void __iomem		*regs;
> > +};
> > +
> > +/* System ID Registers*/
> > +#define SYSID_REG_ID		(0x0)
> > +#define SYSID_REG_TIMESTAMP	(0x4)
> > +
> > +static ssize_t intel_sysid_show_id(struct device *dev,
> > +		struct device_attribute *attr, char *buf)
> > +{
> > +	struct intel_sysid *sysid = dev_get_drvdata(dev);
> > +
> > +	return sprintf(buf, "%u\n", readl(sysid->regs +
> > SYSID_REG_ID));
> > +}
> > +
> > +static ssize_t intel_sysid_show_timestamp(struct device *dev,
> > +		struct device_attribute *attr, char *buf)
> > +{
> > +	unsigned int reg;
> > +	struct tm timestamp;
> > +	struct intel_sysid *sysid = dev_get_drvdata(dev);
> > +
> > +	reg = readl(sysid->regs + SYSID_REG_TIMESTAMP);
> > +
> > +	time_to_tm(reg, 0, &timestamp);
> > +
> > +	return sprintf(buf, "%u (%u-%u-%u %u:%u:%u UTC)\n", reg,
> > +		(unsigned int)(timestamp.tm_year + 1900),
> > +		timestamp.tm_mon + 1, timestamp.tm_mday,
> > timestamp.tm_hour,
> > +		timestamp.tm_min, timestamp.tm_sec);
> > +}
> > +
> > +static DEVICE_ATTR(id, S_IRUGO, intel_sysid_show_id, NULL);
> > +static DEVICE_ATTR(timestamp, S_IRUGO, intel_sysid_show_timestamp,
> > NULL);
> DEVICE_ATTR_RO()?
> 
> > 
> > +static struct attribute *intel_sysid_attrs[] = {
> > +	&dev_attr_id.attr,
> > +	&dev_attr_timestamp.attr,
> > +	NULL,
> > +};
> > +
> > +struct attribute_group intel_sysid_attr_group = {
> > +	.name = "sysid",
> > +	.attrs = intel_sysid_attrs,
> > +};
> ATTRIBUTE_GROUP()?
> 
> thanks,
> 
> greg k-h

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

* Re: [PATCH 1/1] drivers/misc: Add Intel System ID driver
  2017-02-15 19:51   ` Arnd Bergmann
@ 2017-02-23  5:58     ` Loh, Tien Hock
  2017-02-23  8:05       ` Arnd Bergmann
  0 siblings, 1 reply; 11+ messages in thread
From: Loh, Tien Hock @ 2017-02-23  5:58 UTC (permalink / raw)
  To: gregkh, arnd; +Cc: linux-kernel, Nguyen, Dinh, thloh85, Gerlach, Matthew

Sorry for the late reply. 
This driver can currently be used by ARM and Nios II, so moving it into
drivers/soc might not be the best idea.

Thanks
Tien Hock

On Rab, 2017-02-15 at 20:51 +0100, Arnd Bergmann wrote:
> On Wed, Feb 15, 2017 at 6:17 PM, Greg KH <gregkh@linuxfoundation.org>
> wrote:
> > 
> > On Wed, Feb 15, 2017 at 07:09:41PM +0800, thloh wrote:
> > > 
> > > From: "Loh, Tien Hock" <tien.hock.loh@intel.com>
> > > 
> > > This patch is to add Altera System ID driver.
> > > User can obtain the system ID and timestamp of the system by
> > > reading the sysfs entry.
> > > 
> > > Usage:
> > > cat /sys/bus/platform/devices/[addr].sysid/sysid/id
> > > cat /sys/bus/platform/devices/[addr].sysid/sysid/timestamp
> > If you add new sysfs attributes, you need to also add a
> > Documentation/ABI/ description as well.
> Maybe we could pretend that this is for a SoC and use the standard
> soc_device
> attributes as well as moving the driver into drivers/soc/?
> 
>      Arnd

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

* Re: [PATCH 1/1] drivers/misc: Add Intel System ID driver
  2017-02-23  5:58     ` Loh, Tien Hock
@ 2017-02-23  8:05       ` Arnd Bergmann
  2017-02-23  8:15         ` Loh, Tien Hock
  0 siblings, 1 reply; 11+ messages in thread
From: Arnd Bergmann @ 2017-02-23  8:05 UTC (permalink / raw)
  To: Loh, Tien Hock
  Cc: gregkh, linux-kernel, Nguyen, Dinh, thloh85, Gerlach, Matthew

On Thu, Feb 23, 2017 at 6:58 AM, Loh, Tien Hock <tien.hock.loh@intel.com> wrote:
> On Rab, 2017-02-15 at 20:51 +0100, Arnd Bergmann wrote:
>> On Wed, Feb 15, 2017 at 6:17 PM, Greg KH <gregkh@linuxfoundation.org>
>> wrote:
>> >
>> > On Wed, Feb 15, 2017 at 07:09:41PM +0800, thloh wrote:
>> > >
>> > > From: "Loh, Tien Hock" <tien.hock.loh@intel.com>
>> > >
>> > > This patch is to add Altera System ID driver.
>> > > User can obtain the system ID and timestamp of the system by
>> > > reading the sysfs entry.
>> > >
>> > > Usage:
>> > > cat /sys/bus/platform/devices/[addr].sysid/sysid/id
>> > > cat /sys/bus/platform/devices/[addr].sysid/sysid/timestamp
>> > If you add new sysfs attributes, you need to also add a
>> > Documentation/ABI/ description as well.
>>
>> Maybe we could pretend that this is for a SoC and use the standard
>> soc_device
>> attributes as well as moving the driver into drivers/soc/?> Sorry for the late reply.
>
> This driver can currently be used by ARM and Nios II, so moving it into
> drivers/soc might not be the best idea.

Why not? drivers/soc/ was specifically introduced for stuff that is used on
some SoC but across more than one architecture (otherwise it would be
in arch/foo/). This seems to fit perfectly.

     Arnd

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

* Re: [PATCH 1/1] drivers/misc: Add Intel System ID driver
  2017-02-23  8:05       ` Arnd Bergmann
@ 2017-02-23  8:15         ` Loh, Tien Hock
  2017-03-01  7:23           ` Loh, Tien Hock
  0 siblings, 1 reply; 11+ messages in thread
From: Loh, Tien Hock @ 2017-02-23  8:15 UTC (permalink / raw)
  To: arnd; +Cc: linux-kernel, Nguyen, Dinh, thloh85, gregkh, Gerlach, Matthew

OK then I'll move it into driver/soc/ in that case.

On Kha, 2017-02-23 at 09:05 +0100, Arnd Bergmann wrote:
> On Thu, Feb 23, 2017 at 6:58 AM, Loh, Tien Hock <tien.hock.loh@intel.
> com> wrote:
> > 
> > On Rab, 2017-02-15 at 20:51 +0100, Arnd Bergmann wrote:
> > > 
> > > On Wed, Feb 15, 2017 at 6:17 PM, Greg KH <gregkh@linuxfoundation.
> > > org>
> > > wrote:
> > > > 
> > > > 
> > > > On Wed, Feb 15, 2017 at 07:09:41PM +0800, thloh wrote:
> > > > > 
> > > > > 
> > > > > From: "Loh, Tien Hock" <tien.hock.loh@intel.com>
> > > > > 
> > > > > This patch is to add Altera System ID driver.
> > > > > User can obtain the system ID and timestamp of the system by
> > > > > reading the sysfs entry.
> > > > > 
> > > > > Usage:
> > > > > cat /sys/bus/platform/devices/[addr].sysid/sysid/id
> > > > > cat /sys/bus/platform/devices/[addr].sysid/sysid/timestamp
> > > > If you add new sysfs attributes, you need to also add a
> > > > Documentation/ABI/ description as well.
> > > Maybe we could pretend that this is for a SoC and use the
> > > standard
> > > soc_device
> > > attributes as well as moving the driver into drivers/soc/?> Sorry
> > > for the late reply.
> > This driver can currently be used by ARM and Nios II, so moving it
> > into
> > drivers/soc might not be the best idea.
> Why not? drivers/soc/ was specifically introduced for stuff that is
> used on
> some SoC but across more than one architecture (otherwise it would be
> in arch/foo/). This seems to fit perfectly.
> 
>      Arnd

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

* Re: [PATCH 1/1] drivers/misc: Add Intel System ID driver
  2017-02-23  8:15         ` Loh, Tien Hock
@ 2017-03-01  7:23           ` Loh, Tien Hock
  2017-03-01  9:01             ` Arnd Bergmann
  0 siblings, 1 reply; 11+ messages in thread
From: Loh, Tien Hock @ 2017-03-01  7:23 UTC (permalink / raw)
  To: arnd; +Cc: linux-kernel, Nguyen, Dinh, thloh85, gregkh, Gerlach, Matthew

Arnd, Greg,

I checked the attributes returned by the soc attribute subsystem, but
it seems that it is lacking something equivalent to timestamp in the
Intel System ID controller. Do you think it is better to add a new
attribute (named timestamp) to soc or create a new sysfs entry like
what I did?

Thanks
Tien Hock 

On Kha, 2017-02-23 at 16:15 +0800, Loh, Tien Hock wrote:
> OK then I'll move it into driver/soc/ in that case.
> 
> On Kha, 2017-02-23 at 09:05 +0100, Arnd Bergmann wrote:
> > 
> > On Thu, Feb 23, 2017 at 6:58 AM, Loh, Tien Hock <tien.hock.loh@inte
> > l.
> > com> wrote:
> > > 
> > > 
> > > On Rab, 2017-02-15 at 20:51 +0100, Arnd Bergmann wrote:
> > > > 
> > > > 
> > > > On Wed, Feb 15, 2017 at 6:17 PM, Greg KH <gregkh@linuxfoundatio
> > > > n.
> > > > org>
> > > > wrote:
> > > > > 
> > > > > 
> > > > > 
> > > > > On Wed, Feb 15, 2017 at 07:09:41PM +0800, thloh wrote:
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > From: "Loh, Tien Hock" <tien.hock.loh@intel.com>
> > > > > > 
> > > > > > This patch is to add Altera System ID driver.
> > > > > > User can obtain the system ID and timestamp of the system
> > > > > > by
> > > > > > reading the sysfs entry.
> > > > > > 
> > > > > > Usage:
> > > > > > cat /sys/bus/platform/devices/[addr].sysid/sysid/id
> > > > > > cat /sys/bus/platform/devices/[addr].sysid/sysid/timestamp
> > > > > If you add new sysfs attributes, you need to also add a
> > > > > Documentation/ABI/ description as well.
> > > > Maybe we could pretend that this is for a SoC and use the
> > > > standard
> > > > soc_device
> > > > attributes as well as moving the driver into drivers/soc/?>
> > > > Sorry
> > > > for the late reply.
> > > This driver can currently be used by ARM and Nios II, so moving
> > > it
> > > into
> > > drivers/soc might not be the best idea.
> > Why not? drivers/soc/ was specifically introduced for stuff that is
> > used on
> > some SoC but across more than one architecture (otherwise it would
> > be
> > in arch/foo/). This seems to fit perfectly.
> > 
> >      Arnd

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

* Re: [PATCH 1/1] drivers/misc: Add Intel System ID driver
  2017-03-01  7:23           ` Loh, Tien Hock
@ 2017-03-01  9:01             ` Arnd Bergmann
  2017-03-01 10:42               ` Loh, Tien Hock
  0 siblings, 1 reply; 11+ messages in thread
From: Arnd Bergmann @ 2017-03-01  9:01 UTC (permalink / raw)
  To: Loh, Tien Hock
  Cc: linux-kernel, Nguyen, Dinh, thloh85, gregkh, Gerlach, Matthew

On Wed, Mar 1, 2017 at 8:23 AM, Loh, Tien Hock <tien.hock.loh@intel.com> wrote:
> Arnd, Greg,

Please don't top-post.

> I checked the attributes returned by the soc attribute subsystem, but
> it seems that it is lacking something equivalent to timestamp in the
> Intel System ID controller. Do you think it is better to add a new
> attribute (named timestamp) to soc or create a new sysfs entry like
> what I did?

It depends on how common and how important this attribute is.

- if it's not overly important, just drop it entirely.
- if it's important enough that other SoCs are likely to have the same
  kind of information, make it a standard attribute
- if this SoC is most likely the only one that will ever need it, but it has
  important uses, I'd make it a custom attribute

Another option would be to fold the timestamp into the revision attribute,
but whether that is a reasonable place for it would in turn depend on
what the timestamp signifies.

Can you explain what the timestamp is used for? Does it identify the
time that the hardware revision was made, the time that a software
was built which was loaded into it, or something else?
What kind of user space application would need this information?

     Arnd

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

* Re: [PATCH 1/1] drivers/misc: Add Intel System ID driver
  2017-03-01  9:01             ` Arnd Bergmann
@ 2017-03-01 10:42               ` Loh, Tien Hock
  2017-03-01 11:34                 ` Arnd Bergmann
  0 siblings, 1 reply; 11+ messages in thread
From: Loh, Tien Hock @ 2017-03-01 10:42 UTC (permalink / raw)
  To: arnd; +Cc: linux-kernel, Nguyen, Dinh, thloh85, gregkh, Gerlach, Matthew

On Rab, 2017-03-01 at 10:01 +0100, Arnd Bergmann wrote:
> On Wed, Mar 1, 2017 at 8:23 AM, Loh, Tien Hock <tien.hock.loh@intel.c
> om> wrote:
> > 
> > Arnd, Greg,
> Please don't top-post.
OK.

> 
> > 
> > I checked the attributes returned by the soc attribute subsystem,
> > but
> > it seems that it is lacking something equivalent to timestamp in
> > the
> > Intel System ID controller. Do you think it is better to add a new
> > attribute (named timestamp) to soc or create a new sysfs entry like
> > what I did?
> It depends on how common and how important this attribute is.
> 
> - if it's not overly important, just drop it entirely.
> - if it's important enough that other SoCs are likely to have the
> same
>   kind of information, make it a standard attribute
> - if this SoC is most likely the only one that will ever need it, but
> it has
>   important uses, I'd make it a custom attribute
> 
> Another option would be to fold the timestamp into the revision
> attribute,
> but whether that is a reasonable place for it would in turn depend on
> what the timestamp signifies.
> 
> Can you explain what the timestamp is used for? Does it identify the
> time that the hardware revision was made, the time that a software
> was built which was loaded into it, or something else?
> What kind of user space application would need this information?

I just checked, and it seems like we can't put this into soc subsystem.
In FPGA, we now can do partial reconfiguration, which "reconfigures"
the hardware to have an updated sysid and timestamp value, and the base
address of the Intel System ID may also be changed. This would require
the driver to be a module that will be removed, probed again. The soc
subsystem doesn't seem to be a suitable place to add this driver.

A note on the timestamp, in the old days this is used to check that the
BSP is using the correct FPGA hardware. I believe in Linux we should do
the same in the driver, and if it not, the driver should print a
warning. The timestamp's print is not exactly needed. I'll add the
feature into the driver in the next patch.

> 
>      Arnd

Thanks
Tien Hock

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

* Re: [PATCH 1/1] drivers/misc: Add Intel System ID driver
  2017-03-01 10:42               ` Loh, Tien Hock
@ 2017-03-01 11:34                 ` Arnd Bergmann
  2017-03-02  2:17                   ` Loh, Tien Hock
  0 siblings, 1 reply; 11+ messages in thread
From: Arnd Bergmann @ 2017-03-01 11:34 UTC (permalink / raw)
  To: Loh, Tien Hock
  Cc: linux-kernel, Nguyen, Dinh, thloh85, gregkh, Gerlach, Matthew

On Wed, Mar 1, 2017 at 11:42 AM, Loh, Tien Hock <tien.hock.loh@intel.com> wrote:
> On Rab, 2017-03-01 at 10:01 +0100, Arnd Bergmann wrote:
>> On Wed, Mar 1, 2017 at 8:23 AM, Loh, Tien Hock <tien.hock.loh@intel.c
>> Another option would be to fold the timestamp into the revision
>> attribute,
>> but whether that is a reasonable place for it would in turn depend on
>> what the timestamp signifies.
>>
>> Can you explain what the timestamp is used for? Does it identify the
>> time that the hardware revision was made, the time that a software
>> was built which was loaded into it, or something else?
>> What kind of user space application would need this information?
>
> I just checked, and it seems like we can't put this into soc subsystem.
> In FPGA, we now can do partial reconfiguration, which "reconfigures"
> the hardware to have an updated sysid and timestamp value, and the base
> address of the Intel System ID may also be changed. This would require
> the driver to be a module that will be removed, probed again. The soc
> subsystem doesn't seem to be a suitable place to add this driver.

Ah, I had not realized this is for fpga_manager.

Why not put the attributes into /sys/class/fpga_manager/*/ then
along with the other attributes that exist there? That way, we have
an interface that works for all users of drivers/fpga/

> A note on the timestamp, in the old days this is used to check that the
> BSP is using the correct FPGA hardware. I believe in Linux we should do
> the same in the driver, and if it not, the driver should print a
> warning. The timestamp's print is not exactly needed. I'll add the
> feature into the driver in the next patch.

Ok.

     Arnd

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

* Re: [PATCH 1/1] drivers/misc: Add Intel System ID driver
  2017-03-01 11:34                 ` Arnd Bergmann
@ 2017-03-02  2:17                   ` Loh, Tien Hock
  0 siblings, 0 replies; 11+ messages in thread
From: Loh, Tien Hock @ 2017-03-02  2:17 UTC (permalink / raw)
  To: arnd; +Cc: linux-kernel, Nguyen, Dinh, thloh85, gregkh, Gerlach, Matthew

On Rab, 2017-03-01 at 12:34 +0100, Arnd Bergmann wrote:
> On Wed, Mar 1, 2017 at 11:42 AM, Loh, Tien Hock <tien.hock.loh@intel.
> com> wrote:
> > 
> > On Rab, 2017-03-01 at 10:01 +0100, Arnd Bergmann wrote:
> > > 
> > > On Wed, Mar 1, 2017 at 8:23 AM, Loh, Tien Hock <tien.hock.loh@int
> > > el.c
> > > Another option would be to fold the timestamp into the revision
> > > attribute,
> > > but whether that is a reasonable place for it would in turn
> > > depend on
> > > what the timestamp signifies.
> > > 
> > > Can you explain what the timestamp is used for? Does it identify
> > > the
> > > time that the hardware revision was made, the time that a
> > > software
> > > was built which was loaded into it, or something else?
> > > What kind of user space application would need this information?
> > I just checked, and it seems like we can't put this into soc
> > subsystem.
> > In FPGA, we now can do partial reconfiguration, which
> > "reconfigures"
> > the hardware to have an updated sysid and timestamp value, and the
> > base
> > address of the Intel System ID may also be changed. This would
> > require
> > the driver to be a module that will be removed, probed again. The
> > soc
> > subsystem doesn't seem to be a suitable place to add this driver.
> Ah, I had not realized this is for fpga_manager.
> 
> Why not put the attributes into /sys/class/fpga_manager/*/ then
> along with the other attributes that exist there? That way, we have
> an interface that works for all users of drivers/fpga/
> 

Well, this is not only for fpga_manager, but often time used to ensure
that the correct hardware is programmed into the FPGA by checking the
sysid after fpga_manager reconfigures the hardware. Systems without
fpga_manager can still use sysid and timestamp to ensure the hardware
is as expected value.

What do you think of /sys/class/fpga_sysid/*/id and
/sys/class/fpga_sysid/*/timestamp?

> > 
> > A note on the timestamp, in the old days this is used to check that
> > the
> > BSP is using the correct FPGA hardware. I believe in Linux we
> > should do
> > the same in the driver, and if it not, the driver should print a
> > warning. The timestamp's print is not exactly needed. I'll add the
> > feature into the driver in the next patch.
> Ok.
> 
>      Arnd

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

end of thread, other threads:[~2017-03-02  2:17 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <1487156981-4550-1-git-send-email-user@thloh-VirtualBox>
2017-02-15 17:17 ` [PATCH 1/1] drivers/misc: Add Intel System ID driver Greg KH
2017-02-15 19:51   ` Arnd Bergmann
2017-02-23  5:58     ` Loh, Tien Hock
2017-02-23  8:05       ` Arnd Bergmann
2017-02-23  8:15         ` Loh, Tien Hock
2017-03-01  7:23           ` Loh, Tien Hock
2017-03-01  9:01             ` Arnd Bergmann
2017-03-01 10:42               ` Loh, Tien Hock
2017-03-01 11:34                 ` Arnd Bergmann
2017-03-02  2:17                   ` Loh, Tien Hock
2017-02-23  5:50   ` Loh, Tien Hock

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