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@xxxxxxxxxxxxxx>
>> ---
>> .../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
>
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a\nmember of the Code Aurora Forum, hosted by The Linux Foundation
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
