Re: [PATCH] soc: qcom: Add SoC info driver

From: Andy Gross
Date: Thu Oct 20 2016 - 11:20:26 EST

Next message: Patrice Chotard: "[GIT PULL] STi defconfig updates for v4.10"
Previous message: Mark Rutland: "Re: [PATCH v14 0/9] acpi, clocksource: add GTDT driver and GTDT support in arm_arch_timer"
In reply to: Mark Rutland: "Re: [PATCH] soc: qcom: Add SoC info driver"
Next in thread: Arnd Bergmann: "Re: [PATCH] soc: qcom: Add SoC info driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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?

> 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

Next message: Patrice Chotard: "[GIT PULL] STi defconfig updates for v4.10"
Previous message: Mark Rutland: "Re: [PATCH v14 0/9] acpi, clocksource: add GTDT driver and GTDT support in arm_arch_timer"
In reply to: Mark Rutland: "Re: [PATCH] soc: qcom: Add SoC info driver"
Next in thread: Arnd Bergmann: "Re: [PATCH] soc: qcom: Add SoC info driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]