soc: qcom: Add SoC info driver

The SoC info driver provides information such as Chip ID,
Chip family, serial number and other such details about
Qualcomm SoCs.

Signed-off-by: Imran Khan <kimran@codeaurora.org>
---
 .../devicetree/bindings/soc/qcom/qcom,socinfo.txt  |   18 +
 drivers/soc/qcom/socinfo.c                         | 1173 ++++++++++++++++++++
 include/linux/soc/qcom/socinfo.h                   |  198 ++++
 3 files changed, 1389 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,socinfo.txt
 create mode 100644 drivers/soc/qcom/socinfo.c
 create mode 100644 include/linux/soc/qcom/socinfo.h

On Thu, Oct 20, 2016 at 07:36:22PM +0530, Imran Khan wrote:
> The SoC info driver provides information such as Chip ID,
> Chip family, serial number and other such details about
> Qualcomm SoCs.
> 
> Signed-off-by: Imran Khan <kimran@codeaurora.org>
> ---
>  .../devicetree/bindings/soc/qcom/qcom,socinfo.txt  |   18 +
>  drivers/soc/qcom/socinfo.c                         | 1173 ++++++++++++++++++++
>  include/linux/soc/qcom/socinfo.h                   |  198 ++++
>  3 files changed, 1389 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,socinfo.txt
>  create mode 100644 drivers/soc/qcom/socinfo.c
>  create mode 100644 include/linux/soc/qcom/socinfo.h
> 
> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,socinfo.txt b/Documentation/devicetree/bindings/soc/qcom/qcom,socinfo.txt
> new file mode 100644
> index 0000000..1f26299
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,socinfo.txt
> @@ -0,0 +1,18 @@
> +Qualcomm SoC Information (socinfo) Driver binding
> +
> +This binding describes the Qualcomm SoC Information Driver, which provides
> +information such as chip id, chip family, serial number and other such
> +details about Qualcomm SoCs.
> +
> +- compatible:
> +	Usage: required
> +	Value type: <stringlist>
> +	Definition: must be "qcom,socinfo"
> +
> += EXAMPLE
> +
> +The following example represents a socinfo node.
> +
> +	socinfo {
> +		compatible = "qcom,socinfo";
> +	};

Binding should describe *hardware*, not *drivers*.

This clearly does not describe a paerticular piece of hardware, and as
such does not make sense.

If this is trying to describe some paritcular hardware, please describe
that in the binding explicitly.

Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, Oct 20, 2016 at 07:36:22PM +0530, Imran Khan wrote:
> The SoC info driver provides information such as Chip ID,
> Chip family, serial number and other such details about
> Qualcomm SoCs.
> 
> Signed-off-by: Imran Khan <kimran@codeaurora.org>
> ---
>  .../devicetree/bindings/soc/qcom/qcom,socinfo.txt  |   18 +
>  drivers/soc/qcom/socinfo.c                         | 1173 ++++++++++++++++++++

1173 lines!!!!!!  To latch smem entry and read/process those contents?

>  include/linux/soc/qcom/socinfo.h                   |  198 ++++
>  3 files changed, 1389 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,socinfo.txt
>  create mode 100644 drivers/soc/qcom/socinfo.c
>  create mode 100644 include/linux/soc/qcom/socinfo.h
> 
> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,socinfo.txt b/Documentation/devicetree/bindings/soc/qcom/qcom,socinfo.txt
> new file mode 100644
> index 0000000..1f26299
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,socinfo.txt
> @@ -0,0 +1,18 @@
> +Qualcomm SoC Information (socinfo) Driver binding
> +
> +This binding describes the Qualcomm SoC Information Driver, which provides
> +information such as chip id, chip family, serial number and other such
> +details about Qualcomm SoCs.
> +
> +- compatible:
> +	Usage: required
> +	Value type: <stringlist>
> +	Definition: must be "qcom,socinfo"
> +
> += EXAMPLE
> +
> +The following example represents a socinfo node.
> +
> +	socinfo {
> +		compatible = "qcom,socinfo";
> +	};

Let's drop adding a new binding.  Let's roll this into the smem driver itself.

> diff --git a/drivers/soc/qcom/socinfo.c b/drivers/soc/qcom/socinfo.c
> new file mode 100644
> index 0000000..dc26028
> --- /dev/null
> +++ b/drivers/soc/qcom/socinfo.c
> @@ -0,0 +1,1173 @@
> +/*
> + * Copyright (c) 2009-2016, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that 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.
> + *
> + */
> +
> +/*
> + * SOC Info Routines
> + *
> + */
> +
> +#define pr_fmt(fmt) "%s: " fmt, __func__
> +
> +#include <linux/export.h>
> +#include <linux/module.h>
> +#include <linux/err.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/sys_soc.h>
> +#include <linux/slab.h>
> +#include <linux/stat.h>
> +#include <linux/string.h>
> +#include <linux/types.h>
> +#include <linux/soc/qcom/socinfo.h>
> +#include <linux/soc/qcom/smem.h>
> +
> +#include <asm/system_misc.h>
> +
> +
> +#define BUILD_ID_LENGTH 32

This needs to be more specific.  SMEM_SOCINFO_XXX

> +#define SMEM_IMAGE_VERSION_BLOCKS_COUNT 32
> +#define SMEM_IMAGE_VERSION_SINGLE_BLOCK_SIZE 128
> +#define SMEM_IMAGE_VERSION_SIZE 4096
> +#define SMEM_IMAGE_VERSION_NAME_SIZE 75
> +#define SMEM_IMAGE_VERSION_VARIANT_SIZE 20
> +#define SMEM_IMAGE_VERSION_VARIANT_OFFSET 75
> +#define SMEM_IMAGE_VERSION_OEM_SIZE 32
> +#define SMEM_IMAGE_VERSION_OEM_OFFSET 96
> +#define SMEM_IMAGE_VERSION_PARTITION_APPS 10
> +#define SMEM_ITEM_SIZE_ALIGN 8
> +/*
> + * Shared memory identifiers, used to acquire handles to respective memory
> + * region.
> + */
> +#define SMEM_IMAGE_VERSION_TABLE	469
> +#define SMEM_HW_SW_BUILD_ID		137
> +

<snip>

> +static enum qcom_cpu cur_cpu;
> +static int current_image;
> +static uint32_t socinfo_format;
> +
> +static struct socinfo_v0_1 dummy_socinfo = {
> +	.format = SOCINFO_VERSION(0, 1),
> +	.version = 1,
> +};

Instead of having a dummy, can we just fail if we don't find a match?  And by
fail, I mean having an 'unknown' cpu?

> +
> +static char *socinfo_get_id_string(void)
> +{
> +	return (socinfo) ? cpu_of_id[socinfo->v0_1.id].soc_id_string : NULL;
> +}
> +

<snip>

> +uint32_t socinfo_get_id(void)
> +{
> +	return (socinfo) ? socinfo->v0_1.id : 0;
> +}
> +EXPORT_SYMBOL_GPL(socinfo_get_id);

Why do we have EXPORTS at all?  Drivers should be using compatibles to figure
out what they are.

<snip>

> +/* socinfo: sysfs functions */
> +
> +static ssize_t
> +qcom_get_vendor(struct device *dev,
> +		struct device_attribute *attr,
> +		char *buf)
> +{
> +	return snprintf(buf, PAGE_SIZE, "Qualcomm\n");
> +}
> +
> +static ssize_t
> +qcom_get_raw_id(struct device *dev,
> +		struct device_attribute *attr,
> +		char *buf)
> +{
> +	return snprintf(buf, PAGE_SIZE, "%u\n",
> +		socinfo_get_raw_id());
> +}
> +

<snip>

> +static void * __init setup_dummy_socinfo(void)
> +{
> +	if (early_machine_is_apq8064()) {
> +		dummy_socinfo.id = APQ_8064_ID;
> +		strlcpy(dummy_socinfo.build_id, "apq8064",
> +			sizeof(dummy_socinfo.build_id));
> +	} else if (early_machine_is_apq8084()) {
> +		dummy_socinfo.id = APQ_8084_ID;
> +		strlcpy(dummy_socinfo.build_id, "apq8084",
> +			sizeof(dummy_socinfo.build_id));
> +	} else if (early_machine_is_msm8916()) {
> +		dummy_socinfo.id = MSM_8916_ID;
> +		strlcpy(dummy_socinfo.build_id, "msm8916",
> +			sizeof(dummy_socinfo.build_id));
> +	} else if (early_machine_is_msm8660()) {
> +		dummy_socinfo.id = MSM_8660A_ID;
> +		strlcpy(dummy_socinfo.build_id, "msm8660",
> +			sizeof(dummy_socinfo.build_id));
> +	} else if (early_machine_is_msm8960()) {
> +		dummy_socinfo.id = MSM_8960_ID;
> +		strlcpy(dummy_socinfo.build_id, "msm8960",
> +			sizeof(dummy_socinfo.build_id));
> +	} else if (early_machine_is_msm8974()) {
> +		dummy_socinfo.id = MSM_8974_ID;
> +		strlcpy(dummy_socinfo.build_id, "msm8974",
> +			sizeof(dummy_socinfo.build_id));
> +	} else if (early_machine_is_msm8996()) {
> +		dummy_socinfo.id = MSM_8996_ID;
> +		strlcpy(dummy_socinfo.build_id, "msm8996",
> +			sizeof(dummy_socinfo.build_id));
> +	}
> +
> +	strlcat(dummy_socinfo.build_id, "Dummy socinfo",
> +		sizeof(dummy_socinfo.build_id));
> +	return (void *) &dummy_socinfo;
> +}

Can we just remove this and just return whatever unknown values for the sysfs
entries?

> +
> +static void socinfo_populate_sysfs_files(struct device *qcom_soc_device)
> +{
> +	device_create_file(qcom_soc_device, &qcom_soc_attr_vendor);
> +	device_create_file(qcom_soc_device, &image_version);
> +	device_create_file(qcom_soc_device, &image_variant);
> +	device_create_file(qcom_soc_device, &image_crm_version);
> +	device_create_file(qcom_soc_device, &select_image);
> +	device_create_file(qcom_soc_device, &images);
> +
> +	switch (socinfo_format) {
> +	case SOCINFO_VERSION(0, 12):
> +		device_create_file(qcom_soc_device,
> +					&qcom_soc_attr_chip_family);
> +		device_create_file(qcom_soc_device,
> +					&qcom_soc_attr_raw_device_family);
> +		device_create_file(qcom_soc_device,
> +					&qcom_soc_attr_raw_device_number);
> +	case SOCINFO_VERSION(0, 11):
> +	case SOCINFO_VERSION(0, 10):
> +		 device_create_file(qcom_soc_device,
> +					&qcom_soc_attr_serial_number);
> +	case SOCINFO_VERSION(0, 9):
> +		 device_create_file(qcom_soc_device,
> +					&qcom_soc_attr_foundry_id);
> +	case SOCINFO_VERSION(0, 8):
> +	case SOCINFO_VERSION(0, 7):
> +		device_create_file(qcom_soc_device,
> +					&qcom_soc_attr_pmic_model);
> +		device_create_file(qcom_soc_device,
> +					&qcom_soc_attr_pmic_die_revision);
> +	case SOCINFO_VERSION(0, 6):
> +		device_create_file(qcom_soc_device,
> +					&qcom_soc_attr_platform_subtype);
> +		device_create_file(qcom_soc_device,
> +					&qcom_soc_attr_platform_subtype_id);
> +	case SOCINFO_VERSION(0, 5):
> +		device_create_file(qcom_soc_device,
> +					&qcom_soc_attr_accessory_chip);
> +	case SOCINFO_VERSION(0, 4):
> +		device_create_file(qcom_soc_device,
> +					&qcom_soc_attr_platform_version);
> +	case SOCINFO_VERSION(0, 3):
> +		device_create_file(qcom_soc_device,
> +					&qcom_soc_attr_hw_platform);
> +	case SOCINFO_VERSION(0, 2):
> +		device_create_file(qcom_soc_device,
> +					&qcom_soc_attr_raw_id);
> +		device_create_file(qcom_soc_device,
> +					&qcom_soc_attr_raw_version);
> +	case SOCINFO_VERSION(0, 1):
> +		device_create_file(qcom_soc_device,
> +					&qcom_soc_attr_build_id);
> +		break;
> +	default:
> +		pr_err("Unknown socinfo format: v%u.%u\n",
> +				SOCINFO_VERSION_MAJOR(socinfo_format),
> +				SOCINFO_VERSION_MINOR(socinfo_format));
> +		break;
> +	}
> +}
> +

<snip>

> +static int qcom_socinfo_probe(struct platform_device *pdev)
> +{
> +	size_t size;
> +
> +	socinfo = qcom_smem_get(QCOM_SMEM_HOST_ANY, SMEM_HW_SW_BUILD_ID,
> +			&size);
> +	if (IS_ERR_OR_NULL(socinfo)) {
> +		if (PTR_ERR(socinfo) == -EPROBE_DEFER)
> +			return PTR_ERR(socinfo);
> +		else {
> +			dev_warn(&pdev->dev,
> +				"Can't find SMEM_HW_SW_BUILD_ID; falling back on dummy values.\n");
> +			socinfo = setup_dummy_socinfo();
> +		}
> +	}
> +
> +	socinfo_select_format();
> +
> +	WARN(!socinfo_get_id(), "Unknown SOC ID!\n");
> +
> +	if (socinfo_get_id() >= ARRAY_SIZE(cpu_of_id))
> +		BUG_ON("New IDs added! ID => CPU mapping needs an update.\n");
> +	else
> +		cur_cpu = cpu_of_id[socinfo->v0_1.id].generic_soc_type;
> +
> +	socinfo_print();
> +
> +	socinfo_init_sysfs();
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id qcom_socinfo_of_match[] = {
> +	{ .compatible = "qcom,socinfo" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, qcom_socinfo_of_match);
> +
> +static struct platform_driver qcom_socinfo_driver = {
> +	.probe = qcom_socinfo_probe,
> +	.driver  = {
> +		.name  = "qcom-socinfo",
> +		.of_match_table = qcom_socinfo_of_match,
> +	},
> +};

As mentioned above:
Perhaps we can forgo the separate platform driver and just roll this into the
smem driver that already exists.  You can create a device there and get the
right attributes for the system and setup your sysfs entries.

Maybe splitting off the sysfs into a separate file would also be good.  That
keeps it nice and clean and away from the main smem code.


> +module_platform_driver(qcom_socinfo_driver);
> +
> diff --git a/include/linux/soc/qcom/socinfo.h b/include/linux/soc/qcom/socinfo.h
> new file mode 100644
> index 0000000..17ca50a
> --- /dev/null
> +++ b/include/linux/soc/qcom/socinfo.h
> @@ -0,0 +1,198 @@
> +/*
> + * Copyright (c) 2009-2016, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that 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.
> + *
> + */
> +
> +#ifndef _ARCH_ARM_MACH_MSM_SOCINFO_H_
> +#define _ARCH_ARM_MACH_MSM_SOCINFO_H_
> +
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/errno.h>
> +#include <linux/of_fdt.h>
> +#include <linux/of.h>
> +
> +#include <asm/cputype.h>
> +/*
> + * SOC version type with major number in the upper 16 bits and minor
> + * number in the lower 16 bits.  For example:
> + *   1.0 -> 0x00010000
> + *   2.3 -> 0x00020003
> + */
> +#define SOCINFO_VERSION_MAJOR(ver) (((ver) & 0xffff0000) >> 16)
> +#define SOCINFO_VERSION_MINOR(ver) ((ver) & 0x0000ffff)
> +#define SOCINFO_VERSION(maj, min)  ((((maj) & 0xffff) << 16)|((min) & 0xffff))
> +
> +#ifdef CONFIG_OF
> +#define early_machine_is_apq8064()	\
> +	of_flat_dt_is_compatible(of_get_flat_dt_root(), "qcom,apq8064")
> +#define early_machine_is_apq8084()	\
> +	of_flat_dt_is_compatible(of_get_flat_dt_root(), "qcom,apq8084")
> +#define early_machine_is_msm8916()	\
> +	of_flat_dt_is_compatible(of_get_flat_dt_root(), "qcom,msm8916")
> +#define early_machine_is_msm8660()	\
> +	of_flat_dt_is_compatible(of_get_flat_dt_root(), "qcom,msm8660")
> +#define early_machine_is_msm8960()	\
> +	of_flat_dt_is_compatible(of_get_flat_dt_root(), "qcom,msm8960")
> +#define early_machine_is_msm8974()	\
> +	of_flat_dt_is_compatible(of_get_flat_dt_root(), "qcom,msm8974")
> +#define early_machine_is_msm8996()	\
> +	of_flat_dt_is_compatible(of_get_flat_dt_root(), "qcom,msm8996")
> +#else
> +#define early_machine_is_apq8064()	0
> +#define early_machine_is_apq8084()	0
> +#define early_machine_is_msm8916()	0
> +#define early_machine_is_msm8660()	0
> +#define early_machine_is_msm8960()	0
> +#define early_machine_is_msm8974()	0
> +#define early_machine_is_msm8996()	0
> +#endif

You don't need this mess.  You could just build a compat match and match against
the root compatible.

> +
> +#define PLATFORM_SUBTYPE_MDM	1
> +#define PLATFORM_SUBTYPE_INTERPOSERV3 2
> +#define PLATFORM_SUBTYPE_SGLTE	6
> +

<snip>

> +enum msm_cpu socinfo_get_msm_cpu(void);
> +uint32_t socinfo_get_id(void);
> +uint32_t socinfo_get_version(void);
> +uint32_t socinfo_get_raw_id(void);
> +char *socinfo_get_build_id(void);
> +uint32_t socinfo_get_platform_type(void);
> +uint32_t socinfo_get_platform_subtype(void);
> +uint32_t socinfo_get_platform_version(void);
> +uint32_t socinfo_get_serial_number(void);
> +enum qcom_pmic_model socinfo_get_pmic_model(void);
> +uint32_t socinfo_get_pmic_die_revision(void);
> +int __init socinfo_init(void) __must_check;

We shouldn't have APIs for this.  This was supposed to be userspace access only,
correct?


Andy
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thursday, October 20, 2016 7:36:22 PM CEST Imran Khan wrote:
> +#include <linux/soc/qcom/socinfo.h>
> +#include <linux/soc/qcom/smem.h>
> +
> +#include <asm/system_misc.h>

I don't see anything here that needs asm/system_misc.h

> +const char *hw_platform[] = {
> +	[HW_PLATFORM_UNKNOWN] = "Unknown",
> +	[HW_PLATFORM_SURF] = "Surf",
> +	[HW_PLATFORM_FFA] = "FFA",
> +	[HW_PLATFORM_FLUID] = "Fluid",
> +};
> +
> +const char *qrd_hw_platform_subtype[] = {
> +	[PLATFORM_SUBTYPE_QRD] = "QRD",
> +	[PLATFORM_SUBTYPE_SKUAA] = "SKUAA",
> +};
> +
> +const char *hw_platform_subtype[] = {
> +	[PLATFORM_SUBTYPE_UNKNOWN] = "Unknown",
> +};


Make these all static

> +
> +/* Used to parse shared memory. Must match the modem. */
> +struct socinfo_v0_1 {
> +	uint32_t format;
> +	uint32_t id;
> +	uint32_t version;

s/uint32_t/u32/

> +
> +uint32_t socinfo_get_id(void)
> +{
> +	return (socinfo) ? socinfo->v0_1.id : 0;
> +}
> +EXPORT_SYMBOL_GPL(socinfo_get_id);
> +
> +uint32_t socinfo_get_version(void)
> +{
> +	return (socinfo) ? socinfo->v0_1.version : 0;
> +}
> +EXPORT_SYMBOL_GPL(socinfo_get_version);
> +
> +char *socinfo_get_build_id(void)
> +{
> +	return (socinfo) ? socinfo->v0_1.build_id : NULL;
> +}
> +EXPORT_SYMBOL_GPL(socinfo_get_build_id);

Please remove all the exports and mark the functions static,
we don't want any drivers poking vendor-specific interfaces
for this.

> +/* socinfo: sysfs functions */

This seems overly verbose, having both raw and human-readable
IDs is generally not necessary, pick one of the two. If you
need any fields that we don't already support in soc_device,
let's talk about adding them to the generic structure.


> +static ssize_t
> +qcom_get_image_version(struct device *dev,
> +			struct device_attribute *attr,
> +			char *buf)
> +{
> +	char *string_address;
> +
> +	string_address = socinfo_get_image_version_base_address(dev);
> +	if (IS_ERR_OR_NULL(string_address)) {

IS_ERR_OR_NULL() indicates that your interface is not well-designed.
Please return either NULL on all failures, or return an error pointer
on all failures, not both.

> +/* Platform Subtype String is being deprecated. Use Platform
> + * Subtype ID instead.
> + */

If it's deprecated, don't add it to the kernel!


> diff --git a/include/linux/soc/qcom/socinfo.h b/include/linux/soc/qcom/socinfo.h
> new file mode 100644
> index 0000000..17ca50a
> --- /dev/null
> +++ b/include/linux/soc/qcom/socinfo.h

Merge the header file into the driver.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 10/20/2016 8:50 PM, Andy Gross wrote:
> On Thu, Oct 20, 2016 at 07:36:22PM +0530, Imran Khan wrote:
>> The SoC info driver provides information such as Chip ID,
>> Chip family, serial number and other such details about
>> Qualcomm SoCs.
>>
>> Signed-off-by: Imran Khan <kimran@codeaurora.org>
>> ---
>>  .../devicetree/bindings/soc/qcom/qcom,socinfo.txt  |   18 +
>>  drivers/soc/qcom/socinfo.c                         | 1173 ++++++++++++++++++++
> 
> 1173 lines!!!!!!  To latch smem entry and read/process those contents?
> 
The number of code lines will be reduced once we remove the function to setup dummy socinfo.
Also I will try to remove redundant sysfs attributes.

>>  include/linux/soc/qcom/socinfo.h                   |  198 ++++
>>  3 files changed, 1389 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,socinfo.txt
>>  create mode 100644 drivers/soc/qcom/socinfo.c
>>  create mode 100644 include/linux/soc/qcom/socinfo.h
>>
>> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,socinfo.txt b/Documentation/devicetree/bindings/soc/qcom/qcom,socinfo.txt
>> new file mode 100644
>> index 0000000..1f26299
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,socinfo.txt
>> @@ -0,0 +1,18 @@
>> +Qualcomm SoC Information (socinfo) Driver binding
>> +
>> +This binding describes the Qualcomm SoC Information Driver, which provides
>> +information such as chip id, chip family, serial number and other such
>> +details about Qualcomm SoCs.
>> +
>> +- compatible:
>> +	Usage: required
>> +	Value type: <stringlist>
>> +	Definition: must be "qcom,socinfo"
>> +
>> += EXAMPLE
>> +
>> +The following example represents a socinfo node.
>> +
>> +	socinfo {
>> +		compatible = "qcom,socinfo";
>> +	};
> 
> Let's drop adding a new binding.  Let's roll this into the smem driver itself.
> 
Okay. I will move the socinfo setup part to smem and will keep only socinfo sysfs related
stuff in this file.
>> diff --git a/drivers/soc/qcom/socinfo.c b/drivers/soc/qcom/socinfo.c
>> new file mode 100644
>> index 0000000..dc26028
>> --- /dev/null
>> +++ b/drivers/soc/qcom/socinfo.c
>> @@ -0,0 +1,1173 @@
>> +/*
>> + * Copyright (c) 2009-2016, The Linux Foundation. All rights reserved.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 and
>> + * only version 2 as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that 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.
>> + *
>> + */
>> +
>> +/*
>> + * SOC Info Routines
>> + *
>> + */
>> +
>> +#define pr_fmt(fmt) "%s: " fmt, __func__
>> +
>> +#include <linux/export.h>
>> +#include <linux/module.h>
>> +#include <linux/err.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/sys_soc.h>
>> +#include <linux/slab.h>
>> +#include <linux/stat.h>
>> +#include <linux/string.h>
>> +#include <linux/types.h>
>> +#include <linux/soc/qcom/socinfo.h>
>> +#include <linux/soc/qcom/smem.h>
>> +
>> +#include <asm/system_misc.h>
>> +
>> +
>> +#define BUILD_ID_LENGTH 32
> 
> This needs to be more specific.  SMEM_SOCINFO_XXX
> 
>> +#define SMEM_IMAGE_VERSION_BLOCKS_COUNT 32
>> +#define SMEM_IMAGE_VERSION_SINGLE_BLOCK_SIZE 128
>> +#define SMEM_IMAGE_VERSION_SIZE 4096
>> +#define SMEM_IMAGE_VERSION_NAME_SIZE 75
>> +#define SMEM_IMAGE_VERSION_VARIANT_SIZE 20
>> +#define SMEM_IMAGE_VERSION_VARIANT_OFFSET 75
>> +#define SMEM_IMAGE_VERSION_OEM_SIZE 32
>> +#define SMEM_IMAGE_VERSION_OEM_OFFSET 96
>> +#define SMEM_IMAGE_VERSION_PARTITION_APPS 10
>> +#define SMEM_ITEM_SIZE_ALIGN 8
>> +/*
>> + * Shared memory identifiers, used to acquire handles to respective memory
>> + * region.
>> + */
>> +#define SMEM_IMAGE_VERSION_TABLE	469
>> +#define SMEM_HW_SW_BUILD_ID		137
>> +
> 
> <snip>
> 
>> +static enum qcom_cpu cur_cpu;
>> +static int current_image;
>> +static uint32_t socinfo_format;
>> +
>> +static struct socinfo_v0_1 dummy_socinfo = {
>> +	.format = SOCINFO_VERSION(0, 1),
>> +	.version = 1,
>> +};
> 
> Instead of having a dummy, can we just fail if we don't find a match?  And by
> fail, I mean having an 'unknown' cpu?
> 
>> +
>> +static char *socinfo_get_id_string(void)
>> +{
>> +	return (socinfo) ? cpu_of_id[socinfo->v0_1.id].soc_id_string : NULL;
>> +}
>> +
> 
> <snip>
> 
>> +uint32_t socinfo_get_id(void)
>> +{
>> +	return (socinfo) ? socinfo->v0_1.id : 0;
>> +}
>> +EXPORT_SYMBOL_GPL(socinfo_get_id);
> 
> Why do we have EXPORTS at all?  Drivers should be using compatibles to figure
> out what they are.
> 
> <snip>
> 
Okay. 
>> +/* socinfo: sysfs functions */
>> +
>> +static ssize_t
>> +qcom_get_vendor(struct device *dev,
>> +		struct device_attribute *attr,
>> +		char *buf)
>> +{
>> +	return snprintf(buf, PAGE_SIZE, "Qualcomm\n");
>> +}
>> +
>> +static ssize_t
>> +qcom_get_raw_id(struct device *dev,
>> +		struct device_attribute *attr,
>> +		char *buf)
>> +{
>> +	return snprintf(buf, PAGE_SIZE, "%u\n",
>> +		socinfo_get_raw_id());
>> +}
>> +
> 
> <snip>
> 
>> +static void * __init setup_dummy_socinfo(void)
>> +{
>> +	if (early_machine_is_apq8064()) {
>> +		dummy_socinfo.id = APQ_8064_ID;
>> +		strlcpy(dummy_socinfo.build_id, "apq8064",
>> +			sizeof(dummy_socinfo.build_id));
>> +	} else if (early_machine_is_apq8084()) {
>> +		dummy_socinfo.id = APQ_8084_ID;
>> +		strlcpy(dummy_socinfo.build_id, "apq8084",
>> +			sizeof(dummy_socinfo.build_id));
>> +	} else if (early_machine_is_msm8916()) {
>> +		dummy_socinfo.id = MSM_8916_ID;
>> +		strlcpy(dummy_socinfo.build_id, "msm8916",
>> +			sizeof(dummy_socinfo.build_id));
>> +	} else if (early_machine_is_msm8660()) {
>> +		dummy_socinfo.id = MSM_8660A_ID;
>> +		strlcpy(dummy_socinfo.build_id, "msm8660",
>> +			sizeof(dummy_socinfo.build_id));
>> +	} else if (early_machine_is_msm8960()) {
>> +		dummy_socinfo.id = MSM_8960_ID;
>> +		strlcpy(dummy_socinfo.build_id, "msm8960",
>> +			sizeof(dummy_socinfo.build_id));
>> +	} else if (early_machine_is_msm8974()) {
>> +		dummy_socinfo.id = MSM_8974_ID;
>> +		strlcpy(dummy_socinfo.build_id, "msm8974",
>> +			sizeof(dummy_socinfo.build_id));
>> +	} else if (early_machine_is_msm8996()) {
>> +		dummy_socinfo.id = MSM_8996_ID;
>> +		strlcpy(dummy_socinfo.build_id, "msm8996",
>> +			sizeof(dummy_socinfo.build_id));
>> +	}
>> +
>> +	strlcat(dummy_socinfo.build_id, "Dummy socinfo",
>> +		sizeof(dummy_socinfo.build_id));
>> +	return (void *) &dummy_socinfo;
>> +}
> 
> Can we just remove this and just return whatever unknown values for the sysfs
> entries?
> 
Will shorten this function. In case of error I will use 0 as socid and "unknown CPU"
as soc-id.
>> +
>> +static void socinfo_populate_sysfs_files(struct device *qcom_soc_device)
>> +{
>> +	device_create_file(qcom_soc_device, &qcom_soc_attr_vendor);
>> +	device_create_file(qcom_soc_device, &image_version);
>> +	device_create_file(qcom_soc_device, &image_variant);
>> +	device_create_file(qcom_soc_device, &image_crm_version);
>> +	device_create_file(qcom_soc_device, &select_image);
>> +	device_create_file(qcom_soc_device, &images);
>> +
>> +	switch (socinfo_format) {
>> +	case SOCINFO_VERSION(0, 12):
>> +		device_create_file(qcom_soc_device,
>> +					&qcom_soc_attr_chip_family);
>> +		device_create_file(qcom_soc_device,
>> +					&qcom_soc_attr_raw_device_family);
>> +		device_create_file(qcom_soc_device,
>> +					&qcom_soc_attr_raw_device_number);
>> +	case SOCINFO_VERSION(0, 11):
>> +	case SOCINFO_VERSION(0, 10):
>> +		 device_create_file(qcom_soc_device,
>> +					&qcom_soc_attr_serial_number);
>> +	case SOCINFO_VERSION(0, 9):
>> +		 device_create_file(qcom_soc_device,
>> +					&qcom_soc_attr_foundry_id);
>> +	case SOCINFO_VERSION(0, 8):
>> +	case SOCINFO_VERSION(0, 7):
>> +		device_create_file(qcom_soc_device,
>> +					&qcom_soc_attr_pmic_model);
>> +		device_create_file(qcom_soc_device,
>> +					&qcom_soc_attr_pmic_die_revision);
>> +	case SOCINFO_VERSION(0, 6):
>> +		device_create_file(qcom_soc_device,
>> +					&qcom_soc_attr_platform_subtype);
>> +		device_create_file(qcom_soc_device,
>> +					&qcom_soc_attr_platform_subtype_id);
>> +	case SOCINFO_VERSION(0, 5):
>> +		device_create_file(qcom_soc_device,
>> +					&qcom_soc_attr_accessory_chip);
>> +	case SOCINFO_VERSION(0, 4):
>> +		device_create_file(qcom_soc_device,
>> +					&qcom_soc_attr_platform_version);
>> +	case SOCINFO_VERSION(0, 3):
>> +		device_create_file(qcom_soc_device,
>> +					&qcom_soc_attr_hw_platform);
>> +	case SOCINFO_VERSION(0, 2):
>> +		device_create_file(qcom_soc_device,
>> +					&qcom_soc_attr_raw_id);
>> +		device_create_file(qcom_soc_device,
>> +					&qcom_soc_attr_raw_version);
>> +	case SOCINFO_VERSION(0, 1):
>> +		device_create_file(qcom_soc_device,
>> +					&qcom_soc_attr_build_id);
>> +		break;
>> +	default:
>> +		pr_err("Unknown socinfo format: v%u.%u\n",
>> +				SOCINFO_VERSION_MAJOR(socinfo_format),
>> +				SOCINFO_VERSION_MINOR(socinfo_format));
>> +		break;
>> +	}
>> +}
>> +
> 
> <snip>
> 
>> +static int qcom_socinfo_probe(struct platform_device *pdev)
>> +{
>> +	size_t size;
>> +
>> +	socinfo = qcom_smem_get(QCOM_SMEM_HOST_ANY, SMEM_HW_SW_BUILD_ID,
>> +			&size);
>> +	if (IS_ERR_OR_NULL(socinfo)) {
>> +		if (PTR_ERR(socinfo) == -EPROBE_DEFER)
>> +			return PTR_ERR(socinfo);
>> +		else {
>> +			dev_warn(&pdev->dev,
>> +				"Can't find SMEM_HW_SW_BUILD_ID; falling back on dummy values.\n");
>> +			socinfo = setup_dummy_socinfo();
>> +		}
>> +	}
>> +
>> +	socinfo_select_format();
>> +
>> +	WARN(!socinfo_get_id(), "Unknown SOC ID!\n");
>> +
>> +	if (socinfo_get_id() >= ARRAY_SIZE(cpu_of_id))
>> +		BUG_ON("New IDs added! ID => CPU mapping needs an update.\n");
>> +	else
>> +		cur_cpu = cpu_of_id[socinfo->v0_1.id].generic_soc_type;
>> +
>> +	socinfo_print();
>> +
>> +	socinfo_init_sysfs();
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id qcom_socinfo_of_match[] = {
>> +	{ .compatible = "qcom,socinfo" },
>> +	{}
>> +};
>> +MODULE_DEVICE_TABLE(of, qcom_socinfo_of_match);
>> +
>> +static struct platform_driver qcom_socinfo_driver = {
>> +	.probe = qcom_socinfo_probe,
>> +	.driver  = {
>> +		.name  = "qcom-socinfo",
>> +		.of_match_table = qcom_socinfo_of_match,
>> +	},
>> +};
> 
> As mentioned above:
> Perhaps we can forgo the separate platform driver and just roll this into the
> smem driver that already exists.  You can create a device there and get the
> right attributes for the system and setup your sysfs entries.
> 
> Maybe splitting off the sysfs into a separate file would also be good.  That
> keeps it nice and clean and away from the main smem code.
> 
> 
Okay. I will move the socinfo setup part to smem and will keep only socinfo sysfs related
stuff in this file.
>> +module_platform_driver(qcom_socinfo_driver);
>> +
>> diff --git a/include/linux/soc/qcom/socinfo.h b/include/linux/soc/qcom/socinfo.h
>> new file mode 100644
>> index 0000000..17ca50a
>> --- /dev/null
>> +++ b/include/linux/soc/qcom/socinfo.h
>> @@ -0,0 +1,198 @@
>> +/*
>> + * Copyright (c) 2009-2016, The Linux Foundation. All rights reserved.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 and
>> + * only version 2 as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that 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.
>> + *
>> + */
>> +
>> +#ifndef _ARCH_ARM_MACH_MSM_SOCINFO_H_
>> +#define _ARCH_ARM_MACH_MSM_SOCINFO_H_
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/init.h>
>> +#include <linux/errno.h>
>> +#include <linux/of_fdt.h>
>> +#include <linux/of.h>
>> +
>> +#include <asm/cputype.h>
>> +/*
>> + * SOC version type with major number in the upper 16 bits and minor
>> + * number in the lower 16 bits.  For example:
>> + *   1.0 -> 0x00010000
>> + *   2.3 -> 0x00020003
>> + */
>> +#define SOCINFO_VERSION_MAJOR(ver) (((ver) & 0xffff0000) >> 16)
>> +#define SOCINFO_VERSION_MINOR(ver) ((ver) & 0x0000ffff)
>> +#define SOCINFO_VERSION(maj, min)  ((((maj) & 0xffff) << 16)|((min) & 0xffff))
>> +
>> +#ifdef CONFIG_OF
>> +#define early_machine_is_apq8064()	\
>> +	of_flat_dt_is_compatible(of_get_flat_dt_root(), "qcom,apq8064")
>> +#define early_machine_is_apq8084()	\
>> +	of_flat_dt_is_compatible(of_get_flat_dt_root(), "qcom,apq8084")
>> +#define early_machine_is_msm8916()	\
>> +	of_flat_dt_is_compatible(of_get_flat_dt_root(), "qcom,msm8916")
>> +#define early_machine_is_msm8660()	\
>> +	of_flat_dt_is_compatible(of_get_flat_dt_root(), "qcom,msm8660")
>> +#define early_machine_is_msm8960()	\
>> +	of_flat_dt_is_compatible(of_get_flat_dt_root(), "qcom,msm8960")
>> +#define early_machine_is_msm8974()	\
>> +	of_flat_dt_is_compatible(of_get_flat_dt_root(), "qcom,msm8974")
>> +#define early_machine_is_msm8996()	\
>> +	of_flat_dt_is_compatible(of_get_flat_dt_root(), "qcom,msm8996")
>> +#else
>> +#define early_machine_is_apq8064()	0
>> +#define early_machine_is_apq8084()	0
>> +#define early_machine_is_msm8916()	0
>> +#define early_machine_is_msm8660()	0
>> +#define early_machine_is_msm8960()	0
>> +#define early_machine_is_msm8974()	0
>> +#define early_machine_is_msm8996()	0
>> +#endif
> 
> You don't need this mess.  You could just build a compat match and match against
> the root compatible.
> 
Okay. As we don't intend to use dummy soc information, this part will become 
redundant and hence it will be removed.
>> +
>> +#define PLATFORM_SUBTYPE_MDM	1
>> +#define PLATFORM_SUBTYPE_INTERPOSERV3 2
>> +#define PLATFORM_SUBTYPE_SGLTE	6
>> +
> 
> <snip>
> 
>> +enum msm_cpu socinfo_get_msm_cpu(void);
>> +uint32_t socinfo_get_id(void);
>> +uint32_t socinfo_get_version(void);
>> +uint32_t socinfo_get_raw_id(void);
>> +char *socinfo_get_build_id(void);
>> +uint32_t socinfo_get_platform_type(void);
>> +uint32_t socinfo_get_platform_subtype(void);
>> +uint32_t socinfo_get_platform_version(void);
>> +uint32_t socinfo_get_serial_number(void);
>> +enum qcom_pmic_model socinfo_get_pmic_model(void);
>> +uint32_t socinfo_get_pmic_die_revision(void);
>> +int __init socinfo_init(void) __must_check;
> 
> We shouldn't have APIs for this.  This was supposed to be userspace access only,
> correct?
> 
Yes.I will remove these. Although if some driver needs some data pertaining to soc 
information, I will have to provide an alternate way for that but that can be done 
in a generic way rather than using vendor specific interface
> 
> Andy
>

On 10/21/2016 4:03 PM, Arnd Bergmann wrote:
> On Thursday, October 20, 2016 7:36:22 PM CEST Imran Khan wrote:
>> +#include <linux/soc/qcom/socinfo.h>
>> +#include <linux/soc/qcom/smem.h>
>> +
>> +#include <asm/system_misc.h>
> 
> I don't see anything here that needs asm/system_misc.h
> 

Okay. I will not use this header file here.

>> +const char *hw_platform[] = {
>> +	[HW_PLATFORM_UNKNOWN] = "Unknown",
>> +	[HW_PLATFORM_SURF] = "Surf",
>> +	[HW_PLATFORM_FFA] = "FFA",
>> +	[HW_PLATFORM_FLUID] = "Fluid",
>> +};
>> +
>> +const char *qrd_hw_platform_subtype[] = {
>> +	[PLATFORM_SUBTYPE_QRD] = "QRD",
>> +	[PLATFORM_SUBTYPE_SKUAA] = "SKUAA",
>> +};
>> +
>> +const char *hw_platform_subtype[] = {
>> +	[PLATFORM_SUBTYPE_UNKNOWN] = "Unknown",
>> +};
> 
> 
> Make these all static
> 

Okay. Will do this.

>> +
>> +/* Used to parse shared memory. Must match the modem. */
>> +struct socinfo_v0_1 {
>> +	uint32_t format;
>> +	uint32_t id;
>> +	uint32_t version;
> 
> s/uint32_t/u32/
>

Okay. Will do this.
 
>> +
>> +uint32_t socinfo_get_id(void)
>> +{
>> +	return (socinfo) ? socinfo->v0_1.id : 0;
>> +}
>> +EXPORT_SYMBOL_GPL(socinfo_get_id);
>> +
>> +uint32_t socinfo_get_version(void)
>> +{
>> +	return (socinfo) ? socinfo->v0_1.version : 0;
>> +}
>> +EXPORT_SYMBOL_GPL(socinfo_get_version);
>> +
>> +char *socinfo_get_build_id(void)
>> +{
>> +	return (socinfo) ? socinfo->v0_1.build_id : NULL;
>> +}
>> +EXPORT_SYMBOL_GPL(socinfo_get_build_id);
> 
> Please remove all the exports and mark the functions static,
> we don't want any drivers poking vendor-specific interfaces
> for this.
> 

Yes. I will avoid exporting vendor specific interface.

>> +/* socinfo: sysfs functions */
> 
> This seems overly verbose, having both raw and human-readable
> IDs is generally not necessary, pick one of the two. If you
> need any fields that we don't already support in soc_device,
> let's talk about adding them to the generic structure.
> 
> 

Okay. I will go for human readable IDs. Can we add 2 more fields
in the generic structure.
These 2 fields would be:

vendor: A string for vendor name
serial_number: A string containing serial number for the platform

>> +static ssize_t
>> +qcom_get_image_version(struct device *dev,
>> +			struct device_attribute *attr,
>> +			char *buf)
>> +{
>> +	char *string_address;
>> +
>> +	string_address = socinfo_get_image_version_base_address(dev);
>> +	if (IS_ERR_OR_NULL(string_address)) {
> 
> IS_ERR_OR_NULL() indicates that your interface is not well-designed.
> Please return either NULL on all failures, or return an error pointer
> on all failures, not both.
> 
>> +/* Platform Subtype String is being deprecated. Use Platform
>> + * Subtype ID instead.
>> + */
> 
> If it's deprecated, don't add it to the kernel!
> 
>

Okay.
 
>> diff --git a/include/linux/soc/qcom/socinfo.h b/include/linux/soc/qcom/socinfo.h
>> new file mode 100644
>> index 0000000..17ca50a
>> --- /dev/null
>> +++ b/include/linux/soc/qcom/socinfo.h
> 
> Merge the header file into the driver.
> 

okay.

> 	Arnd
>

On Tuesday, October 25, 2016 3:23:34 PM CEST Imran Khan wrote:
> On 10/21/2016 4:03 PM, Arnd Bergmann wrote:
> >> +/* socinfo: sysfs functions */
> > 
> > This seems overly verbose, having both raw and human-readable
> > IDs is generally not necessary, pick one of the two. If you
> > need any fields that we don't already support in soc_device,
> > let's talk about adding them to the generic structure.
> > 
> > 
> 
> Okay. I will go for human readable IDs. Can we add 2 more fields
> in the generic structure.
> These 2 fields would be:
> 
> vendor: A string for vendor name
> serial_number: A string containing serial number for the platform


serial_number seems straightforward, adding this seems like a good
idea. I don't understand yet what would go into the vendor field
though. For this particular driver, is it always "Qualcomm", or
would it be a third-party that makes a device based on that chip?

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 10/26/2016 2:19 AM, Arnd Bergmann wrote:
> On Tuesday, October 25, 2016 3:23:34 PM CEST Imran Khan wrote:
>> On 10/21/2016 4:03 PM, Arnd Bergmann wrote:
>>>> +/* socinfo: sysfs functions */
>>>
>>> This seems overly verbose, having both raw and human-readable
>>> IDs is generally not necessary, pick one of the two. If you
>>> need any fields that we don't already support in soc_device,
>>> let's talk about adding them to the generic structure.
>>>
>>>
>>
>> Okay. I will go for human readable IDs. Can we add 2 more fields
>> in the generic structure.
>> These 2 fields would be:
>>
>> vendor: A string for vendor name
>> serial_number: A string containing serial number for the platform
> 
> 
> serial_number seems straightforward, adding this seems like a good
> idea. I don't understand yet what would go into the vendor field
> though. For this particular driver, is it always "Qualcomm", or
> would it be a third-party that makes a device based on that chip?
> 

As we are talking about generic soc_device_attribute fields, I was hoping that
having a vendor field would be helpful as along with family it would provide
a more thorough information. Also as more than one foundries may be used for 
a soc, can we have a field say foundry_id to provide this information.
  
> 	Arnd
>

On Wednesday, October 26, 2016 7:20:42 PM CEST Imran Khan wrote:
> On 10/26/2016 2:19 AM, Arnd Bergmann wrote:
> > On Tuesday, October 25, 2016 3:23:34 PM CEST Imran Khan wrote:
> >> On 10/21/2016 4:03 PM, Arnd Bergmann wrote:
> >>>> +/* socinfo: sysfs functions */
> >>>
> >>> This seems overly verbose, having both raw and human-readable
> >>> IDs is generally not necessary, pick one of the two. If you
> >>> need any fields that we don't already support in soc_device,
> >>> let's talk about adding them to the generic structure.
> >>>
> >>>
> >>
> >> Okay. I will go for human readable IDs. Can we add 2 more fields
> >> in the generic structure.
> >> These 2 fields would be:
> >>
> >> vendor: A string for vendor name
> >> serial_number: A string containing serial number for the platform
> > 
> > 
> > serial_number seems straightforward, adding this seems like a good
> > idea. I don't understand yet what would go into the vendor field
> > though. For this particular driver, is it always "Qualcomm", or
> > would it be a third-party that makes a device based on that chip?
> > 
> 
> As we are talking about generic soc_device_attribute fields, I was hoping that
> having a vendor field would be helpful as along with family it would provide
> a more thorough information. Also as more than one foundries may be used for 
> a soc, can we have a field say foundry_id to provide this information.

My first feeling is that this 'vendor' information can should be
derived from the family. It's also not clear what would happen
to this when a company gets bought. E.g. the Oxnas product family
was subsequently owned by Oxford, PLX, Avago and Broadcom, and the
mxs family was Sigmatel, Freescale, now NXP and might soon be
Qualcomm. What would you put in there in this case?

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 10/26/2016 7:35 PM, Arnd Bergmann wrote:
>> > As we are talking about generic soc_device_attribute fields, I was hoping that
>> > having a vendor field would be helpful as along with family it would provide
>> > a more thorough information. Also as more than one foundries may be used for 
>> > a soc, can we have a field say foundry_id to provide this information.
> My first feeling is that this 'vendor' information can should be
> derived from the family. It's also not clear what would happen
> to this when a company gets bought. E.g. the Oxnas product family
> was subsequently owned by Oxford, PLX, Avago and Broadcom, and the
> mxs family was Sigmatel, Freescale, now NXP and might soon be
> Qualcomm. What would you put in there in this case?

Okay, not having vendor field is fine for me. Could you also suggest
something about the foundry_id field.

On Wednesday, October 26, 2016 7:42:08 PM CEST Imran Khan wrote:
> On 10/26/2016 7:35 PM, Arnd Bergmann wrote:
> >> > As we are talking about generic soc_device_attribute fields, I was hoping that
> >> > having a vendor field would be helpful as along with family it would provide
> >> > a more thorough information. Also as more than one foundries may be used for 
> >> > a soc, can we have a field say foundry_id to provide this information.
> > My first feeling is that this 'vendor' information can should be
> > derived from the family. It's also not clear what would happen
> > to this when a company gets bought. E.g. the Oxnas product family
> > was subsequently owned by Oxford, PLX, Avago and Broadcom, and the
> > mxs family was Sigmatel, Freescale, now NXP and might soon be
> > Qualcomm. What would you put in there in this case?
> 
> Okay, not having vendor field is fine for me. Could you also suggest
> something about the foundry_id field.

This one seems more well-defined, so it's probably ok to add. What
would be the use case of reading this? Would you want to read it
just from user space or also from the kernel?

Maybe this can be combined with a manufacturing process, which probably
falls into a similar category, so we could have something like
"TSMC 28ULP" as a string in there.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 10/26/2016 8:16 PM, Arnd Bergmann wrote:
> On Wednesday, October 26, 2016 7:42:08 PM CEST Imran Khan wrote:
>> On 10/26/2016 7:35 PM, Arnd Bergmann wrote:
>>>>> As we are talking about generic soc_device_attribute fields, I was hoping that
>>>>> having a vendor field would be helpful as along with family it would provide
>>>>> a more thorough information. Also as more than one foundries may be used for 
>>>>> a soc, can we have a field say foundry_id to provide this information.
>>> My first feeling is that this 'vendor' information can should be
>>> derived from the family. It's also not clear what would happen
>>> to this when a company gets bought. E.g. the Oxnas product family
>>> was subsequently owned by Oxford, PLX, Avago and Broadcom, and the
>>> mxs family was Sigmatel, Freescale, now NXP and might soon be
>>> Qualcomm. What would you put in there in this case?
>>
>> Okay, not having vendor field is fine for me. Could you also suggest
>> something about the foundry_id field.
> 
> This one seems more well-defined, so it's probably ok to add. What
> would be the use case of reading this? Would you want to read it
> just from user space or also from the kernel?
> 

As of now the use case I can think of, only involve reading this from user
space. For example for the same soc, coming from different foundries with
different manufacturing process, we may have a situation where some inconsistent
h/w behavior is being observed only on parts received from a certain foundry
and in those cases this information may help in segregation of problematic socs
and may also be used in testing these socs under a different set of settings like
voltage, frequency etc.

> Maybe this can be combined with a manufacturing process, which probably
> falls into a similar category, so we could have something like
> "TSMC 28ULP" as a string in there.
>

Yes. Having a manufacturing process as part of foundry-id can provide a more
thorough information.
 
> 	Arnd
>

On Thursday, October 27, 2016 6:40:27 PM CEST Imran Khan wrote:
> On 10/26/2016 8:16 PM, Arnd Bergmann wrote:
> > On Wednesday, October 26, 2016 7:42:08 PM CEST Imran Khan wrote:
> >> On 10/26/2016 7:35 PM, Arnd Bergmann wrote:
> >>>>> As we are talking about generic soc_device_attribute fields, I was hoping that
> >>>>> having a vendor field would be helpful as along with family it would provide
> >>>>> a more thorough information. Also as more than one foundries may be used for 
> >>>>> a soc, can we have a field say foundry_id to provide this information.
> >>> My first feeling is that this 'vendor' information can should be
> >>> derived from the family. It's also not clear what would happen
> >>> to this when a company gets bought. E.g. the Oxnas product family
> >>> was subsequently owned by Oxford, PLX, Avago and Broadcom, and the
> >>> mxs family was Sigmatel, Freescale, now NXP and might soon be
> >>> Qualcomm. What would you put in there in this case?
> >>
> >> Okay, not having vendor field is fine for me. Could you also suggest
> >> something about the foundry_id field.
> > 
> > This one seems more well-defined, so it's probably ok to add. What
> > would be the use case of reading this? Would you want to read it
> > just from user space or also from the kernel?
> > 
> 
> As of now the use case I can think of, only involve reading this from user
> space. For example for the same soc, coming from different foundries with
> different manufacturing process, we may have a situation where some inconsistent
> h/w behavior is being observed only on parts received from a certain foundry
> and in those cases this information may help in segregation of problematic socs
> and may also be used in testing these socs under a different set of settings like
> voltage, frequency etc.
>
> > Maybe this can be combined with a manufacturing process, which probably
> > falls into a similar category, so we could have something like
> > "TSMC 28ULP" as a string in there.
> >
> 
> Yes. Having a manufacturing process as part of foundry-id can provide a more
> thorough information.

Ok, sounds good. Let's do it like this. We can always add support for
in-kernel matching of this string if needed later.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 10/27/2016 7:11 PM, Arnd Bergmann wrote:
> On Thursday, October 27, 2016 6:40:27 PM CEST Imran Khan wrote:
>> On 10/26/2016 8:16 PM, Arnd Bergmann wrote:
>>> On Wednesday, October 26, 2016 7:42:08 PM CEST Imran Khan wrote:
>>>> On 10/26/2016 7:35 PM, Arnd Bergmann wrote:
>>>>>>> As we are talking about generic soc_device_attribute fields, I was hoping that
>>>>>>> having a vendor field would be helpful as along with family it would provide
>>>>>>> a more thorough information. Also as more than one foundries may be used for 
>>>>>>> a soc, can we have a field say foundry_id to provide this information.
>>>>> My first feeling is that this 'vendor' information can should be
>>>>> derived from the family. It's also not clear what would happen
>>>>> to this when a company gets bought. E.g. the Oxnas product family
>>>>> was subsequently owned by Oxford, PLX, Avago and Broadcom, and the
>>>>> mxs family was Sigmatel, Freescale, now NXP and might soon be
>>>>> Qualcomm. What would you put in there in this case?
>>>>
>>>> Okay, not having vendor field is fine for me. Could you also suggest
>>>> something about the foundry_id field.
>>>
>>> This one seems more well-defined, so it's probably ok to add. What
>>> would be the use case of reading this? Would you want to read it
>>> just from user space or also from the kernel?
>>>
>>
>> As of now the use case I can think of, only involve reading this from user
>> space. For example for the same soc, coming from different foundries with
>> different manufacturing process, we may have a situation where some inconsistent
>> h/w behavior is being observed only on parts received from a certain foundry
>> and in those cases this information may help in segregation of problematic socs
>> and may also be used in testing these socs under a different set of settings like
>> voltage, frequency etc.
>>
>>> Maybe this can be combined with a manufacturing process, which probably
>>> falls into a similar category, so we could have something like
>>> "TSMC 28ULP" as a string in there.
>>>
>>
>> Yes. Having a manufacturing process as part of foundry-id can provide a more
>> thorough information.
> 
> Ok, sounds good. Let's do it like this. We can always add support for
> in-kernel matching of this string if needed later.
>

Thanks for the feedback. So how should I proceed now, should I 

i. send one patch first that adds the serial_number and foundry_id fields
in generic soc_dev_attribute structure and then send my modified socinfo 
driver as per new soc_dev_attribute structure

or 

ii. send both the changes as 2 separate patches of the same patch set.

or 

iii. Continue with the current soc_dev_attribute structure and modify
the socinfo driver once soc_dev_attribute structure has serial_number
and foundry_id fields.

 
> 	Arnd
>

On Wednesday 02 November 2016, Imran Khan wrote:
> On 10/27/2016 7:11 PM, Arnd Bergmann wrote:
> > On Thursday, October 27, 2016 6:40:27 PM CEST Imran Khan wrote:
> >> On 10/26/2016 8:16 PM, Arnd Bergmann wrote:
> >>> On Wednesday, October 26, 2016 7:42:08 PM CEST Imran Khan wrote:
> >>
> >> Yes. Having a manufacturing process as part of foundry-id can provide a more
> >> thorough information.
> > 
> > Ok, sounds good. Let's do it like this. We can always add support for
> > in-kernel matching of this string if needed later.
> >
> 
> Thanks for the feedback. So how should I proceed now, should I 
> 
> i. send one patch first that adds the serial_number and foundry_id fields
> in generic soc_dev_attribute structure and then send my modified socinfo 
> driver as per new soc_dev_attribute structure
> 
> or 
> 
> ii. send both the changes as 2 separate patches of the same patch set.
> 
> or 
> 
> iii. Continue with the current soc_dev_attribute structure and modify
> the socinfo driver once soc_dev_attribute structure has serial_number
> and foundry_id fields.

It's not overly important, but I'd prefer the third approach, the advantage
being that we can review the driver and merge it independently of
any possible further discussion on the added attributes. Sending
this as a series of three patches (1. add the driver, 2. add the
infrastructure, 3. use the infrastructure in your driver) is probably
best.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Wed 26 Oct 07:05 PDT 2016, Arnd Bergmann wrote:

> On Wednesday, October 26, 2016 7:20:42 PM CEST Imran Khan wrote:
> > On 10/26/2016 2:19 AM, Arnd Bergmann wrote:
> > > On Tuesday, October 25, 2016 3:23:34 PM CEST Imran Khan wrote:
> > >> On 10/21/2016 4:03 PM, Arnd Bergmann wrote:
> > >>>> +/* socinfo: sysfs functions */
> > >>>
> > >>> This seems overly verbose, having both raw and human-readable
> > >>> IDs is generally not necessary, pick one of the two. If you
> > >>> need any fields that we don't already support in soc_device,
> > >>> let's talk about adding them to the generic structure.
> > >>>
> > >>>
> > >>
> > >> Okay. I will go for human readable IDs. Can we add 2 more fields
> > >> in the generic structure.
> > >> These 2 fields would be:
> > >>
> > >> vendor: A string for vendor name
> > >> serial_number: A string containing serial number for the platform
> > > 
> > > 
> > > serial_number seems straightforward, adding this seems like a good
> > > idea. I don't understand yet what would go into the vendor field
> > > though. For this particular driver, is it always "Qualcomm", or
> > > would it be a third-party that makes a device based on that chip?
> > > 
> > 
> > As we are talking about generic soc_device_attribute fields, I was hoping that
> > having a vendor field would be helpful as along with family it would provide
> > a more thorough information. Also as more than one foundries may be used for 
> > a soc, can we have a field say foundry_id to provide this information.
> 
> My first feeling is that this 'vendor' information can should be
> derived from the family.

In [1] Geert just put the vendor directly into "family", while Imran
uses "Snapdragon" (which I find reasonable in the Qualcomm case). But it
seems like Geert would like a "vendor" as well, rather than a "family".


And if "family" really is supposed to contain the "SoC family name" and
we're trying to provide user space with some useful information (for
some reason), should we just rely on the unlikeliness of two vendors
using the same family name?

[1] http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1261742.html

Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Bjorn,

On Wed, Nov 2, 2016 at 5:28 PM, Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
> On Wed 26 Oct 07:05 PDT 2016, Arnd Bergmann wrote:
>> On Wednesday, October 26, 2016 7:20:42 PM CEST Imran Khan wrote:
>> > On 10/26/2016 2:19 AM, Arnd Bergmann wrote:
>> > > On Tuesday, October 25, 2016 3:23:34 PM CEST Imran Khan wrote:
>> > >> On 10/21/2016 4:03 PM, Arnd Bergmann wrote:
>> > >> Okay. I will go for human readable IDs. Can we add 2 more fields
>> > >> in the generic structure.
>> > >> These 2 fields would be:
>> > >>
>> > >> vendor: A string for vendor name
>> > >> serial_number: A string containing serial number for the platform
>> > >
>> > >
>> > > serial_number seems straightforward, adding this seems like a good
>> > > idea. I don't understand yet what would go into the vendor field
>> > > though. For this particular driver, is it always "Qualcomm", or
>> > > would it be a third-party that makes a device based on that chip?
>> >
>> > As we are talking about generic soc_device_attribute fields, I was hoping that
>> > having a vendor field would be helpful as along with family it would provide
>> > a more thorough information. Also as more than one foundries may be used for
>> > a soc, can we have a field say foundry_id to provide this information.
>>
>> My first feeling is that this 'vendor' information can should be
>> derived from the family.
>
> In [1] Geert just put the vendor directly into "family", while Imran
> uses "Snapdragon" (which I find reasonable in the Qualcomm case). But it
> seems like Geert would like a "vendor" as well, rather than a "family".
>
> And if "family" really is supposed to contain the "SoC family name" and
> we're trying to provide user space with some useful information (for
> some reason), should we just rely on the unlikeliness of two vendors
> using the same family name?
>
> [1] http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1261742.html

I think vendor is slightly less volatile than family. Family may
change overnight
if marketing had a good dream. Well, vendor may change, too...

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html