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

From: Stephen Boyd
Date: Fri Feb 17 2017 - 17:51:14 EST

Next message: Dmitry Torokhov: "[PATCH] Input: tsc2007 - switch to using input_set_capability()"
Previous message: Joe Perches: "Re: [PATCH] Staging: ks7010: ks_*: Braces should be used on all arms of these statements"
In reply to: Bjorn Andersson: "Re: [PATCH v9] soc: qcom: Add SoC info driver"
Next in thread: Imran Khan: "Re: [PATCH v9] soc: qcom: Add SoC info driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 02/16, Imran Khan wrote:
> diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
> index 18ec52f..4f0ccf8 100644
> --- a/drivers/soc/qcom/smem.c
> +++ b/drivers/soc/qcom/smem.c
> @@ -85,6 +85,9 @@
> /* Max number of processors/hosts in a system */
> #define SMEM_HOST_COUNT 9
>
> +
> +extern void qcom_socinfo_init(struct device *device);
> +

Sparse still complains... o well.

> diff --git a/drivers/soc/qcom/socinfo.c b/drivers/soc/qcom/socinfo.c
> new file mode 100644
> index 0000000..495f937
> --- /dev/null
> +++ b/drivers/soc/qcom/socinfo.c
> @@ -0,0 +1,557 @@
> +/*
> + * Copyright (c) 2009-2017, 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.
> + *
> + */
> +
> +#include <linux/export.h>
> +#include <linux/err.h>
> +#include <linux/of.h>
> +#include <linux/of_fdt.h>
> +#include <linux/libfdt.h>
> +#include <linux/platform_device.h>
> +#include <linux/sys_soc.h>
> +#include <linux/slab.h>
> +#include <linux/string.h>
> +#include <linux/types.h>
> +#include <linux/random.h>
> +#include <linux/soc/qcom/smem.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 SOC_VER_MAJ(ver) (((ver) & 0xffff0000) >> 16)
> +#define SOC_VER_MIN(ver) ((ver) & 0x0000ffff)
> +#define SOCINFO_VERSION_MAJOR SOC_VER_MAJ
> +#define SOCINFO_VERSION_MINOR SOC_VER_MIN

Do we really need two defines for the same thing?

> +
> +#define SMEM_SOCINFO_BUILD_ID_LENGTH 32
> +#define SMEM_IMAGE_VERSION_BLOCKS_COUNT 32
> +#define SMEM_IMAGE_VERSION_SIZE 4096
> +#define SMEM_IMAGE_VERSION_NAME_SIZE 75
> +#define SMEM_IMAGE_VERSION_VARIANT_SIZE 20
> +#define SMEM_IMAGE_VERSION_OEM_SIZE 32
> +
> +/*
> + * SMEM item ids, used to acquire handles to respective
> + * SMEM region.
> + */
> +#define SMEM_IMAGE_VERSION_TABLE 469
> +#define SMEM_HW_SW_BUILD_ID 137
> +
> +/*
> + * SMEM Image table indices
> + */
> +#define SMEM_IMAGE_TABLE_BOOT_INDEX 0
> +#define SMEM_IMAGE_TABLE_TZ_INDEX 1
> +#define SMEM_IMAGE_TABLE_RPM_INDEX 3
> +#define SMEM_IMAGE_TABLE_APPS_INDEX 10
> +#define SMEM_IMAGE_TABLE_MPSS_INDEX 11
> +#define SMEM_IMAGE_TABLE_ADSP_INDEX 12
> +#define SMEM_IMAGE_TABLE_CNSS_INDEX 13
> +#define SMEM_IMAGE_TABLE_VIDEO_INDEX 14
> +
> +struct qcom_socinfo_attr {
> + struct device_attribute attr;
> + int min_ver;
> +};
> +
> +#define QCOM_SOCINFO_ATTR(_name, _show, _min_ver) \
> + { __ATTR(_name, 0444, _show, NULL), _min_ver }
> +
> +
> +struct smem_image_attribute {
> + struct device_attribute version;
> + struct device_attribute variant;
> + struct device_attribute crm;
> + int index;
> +};
> +
> +#define QCOM_SMEM_IMG_ITEM(_name, _mode, _index) \
> + static struct smem_image_attribute _name##_image_attrs = { \
> + __ATTR(_name##_image_version, _mode, \
> + qcom_show_image_version, qcom_store_image_version), \
> + __ATTR(_name##_image_variant, _mode, \
> + qcom_show_image_variant, qcom_store_image_variant), \
> + __ATTR(_name##_image_crm, _mode, \
> + qcom_show_image_crm, qcom_store_image_crm), \
> + _index \
> + }; \
> + static struct attribute_group _name##_image_attr_group = { \

can be const.

> + .attrs = (struct attribute*[]) { \
> + &_name##_image_attrs.version.attr, \
> + &_name##_image_attrs.variant.attr, \
> + &_name##_image_attrs.crm.attr, \
> + NULL \
> + } \
> + }
> +
> +static const char *const pmic_model[] = {
> + [0] = "Unknown PMIC model",

Missing pm8916, but it doesn't seem to matter for me. My db410c
says 0 here.

> + [13] = "PM8058",
> + [14] = "PM8028",
> + [15] = "PM8901",
> + [16] = "PM8027",
> + [17] = "ISL9519",
> + [18] = "PM8921",
> + [19] = "PM8018",
> + [20] = "PM8015",
> + [21] = "PM8014",
> + [22] = "PM8821",
> + [23] = "PM8038",
> + [24] = "PM8922",
> + [25] = "PM8917",
> +};
> +
> +struct smem_image_version {
> + char name[SMEM_IMAGE_VERSION_NAME_SIZE];
> + char variant[SMEM_IMAGE_VERSION_VARIANT_SIZE];
> + char pad;
> + char oem[SMEM_IMAGE_VERSION_OEM_SIZE];
> +};
> +
> +struct qcom_soc_info {
> + int id;
> + const char *name;
> +};
> +
> +/* Used to parse shared memory. Must match the modem. */
> +struct socinfo_v0_1 {
> + __le32 fmt;
> + __le32 id;
> + __le32 ver;
> + char build_id[SMEM_SOCINFO_BUILD_ID_LENGTH];

Is this a C style string? Or just a bunch of bytes?

> +};
> +
> +struct socinfo_v0_2 {
> + struct socinfo_v0_1 v0_1;
> + __le32 raw_ver;
> +};
> +
> +struct socinfo_v0_3 {
> + struct socinfo_v0_2 v0_2;
> + __le32 hw_plat;
> +};
> +
> +struct socinfo_v0_4 {
> + struct socinfo_v0_3 v0_3;
> + __le32 plat_ver;
> +};
> +
> +struct socinfo_v0_5 {
> + struct socinfo_v0_4 v0_4;
> + __le32 accsry_chip;

accesory_chip?

> +};
> +
> +struct socinfo_v0_6 {
> + struct socinfo_v0_5 v0_5;
> + __le32 hw_plat_subtype;
> +};
> +
> +struct socinfo_v0_7 {
> + struct socinfo_v0_6 v0_6;
> + __le32 pmic_model;
> + __le32 pmic_die_rev;
> +};
> +
> +struct socinfo_v0_8 {
> + struct socinfo_v0_7 v0_7;
> + __le32 pmic_model_1;
> + __le32 pmic_die_rev_1;
> + __le32 pmic_model_2;
> + __le32 pmic_die_rev_2;
> +};

So we have multiple ways to print out pmic information. Hopefully
we can do the right one at the right time based on version we
support?

> +
> +struct socinfo_v0_9 {
> + struct socinfo_v0_8 v0_8;
> + __le32 fndry_id;

Can we write foundry_id?

> +};
> +
> +struct socinfo_v0_10 {
> + struct socinfo_v0_9 v0_9;
> + __le32 srl_num;

And serial_number or serial_num? Not sure why we lost so many vowels.

> +};
> +
> +struct socinfo_v0_11 {
> + struct socinfo_v0_10 v0_10;
> + __le32 num_pmics;
> + __le32 pmic_array_offset;
> +};
> +
> +struct socinfo_v0_12 {
> + struct socinfo_v0_11 v0_11;
> + __le32 chip_family;
> + __le32 raw_dev_fam;

raw_device_family?

> + __le32 raw_dev_num;
> +};
> +
> +static union {
> + struct socinfo_v0_1 v0_1;
> + struct socinfo_v0_2 v0_2;
> + struct socinfo_v0_3 v0_3;
> + struct socinfo_v0_4 v0_4;
> + struct socinfo_v0_5 v0_5;
> + struct socinfo_v0_6 v0_6;
> + struct socinfo_v0_7 v0_7;
> + struct socinfo_v0_8 v0_8;
> + struct socinfo_v0_9 v0_9;
> + struct socinfo_v0_10 v0_10;
> + struct socinfo_v0_11 v0_11;
> + struct socinfo_v0_12 v0_12;

I find this design odd. Do we really care that all these versions
append to each other? Why not just have one structure with
comments delimiting where a new version fields start and then
dropping the whole v0_1, v0_2 thing?

> +} *socinfo;
> +
> +static struct qcom_soc_info soc_of_id[] = {

const?

> + {87, "MSM8960"},

Also please throw some spaces around brackets here.

> + {109, "APQ8064"},
> + {130, "MPQ8064"},
> + {122, "MSM8660A"},
> + {123, "MSM8260A"},
> + {124, "APQ8060A"},
> + {126, "MSM8974"},
> + {184, "APQ8074"},
> + {185, "MSM8274"},
> + {186, "MSM8674"},
> + {208, "APQ8074-AA"},
> + {211, "MSM8274-AA"},
> + {214, "MSM8674-AA"},
> + {217, "MSM8974-AA"},
> + {209, "APQ8074-AB"},
> + {212, "MSM8274-AB"},
> + {215, "MSM8674-AB"},
> + {218, "MSM8974-AB"},
> + {194, "MSM8974PRO"},
> + {210, "APQ8074PRO"},
> + {213, "MSM8274PRO"},
> + {216, "MSM8674PRO"},
> + {138, "MSM8960AB"},
> + {139, "APQ8060AB"},
> + {140, "MSM8260AB"},
> + {141, "MSM8660AB"},
> + {178, "APQ8084"},
> + {206, "MSM8916"},
> + {247, "APQ8016"},
> + {248, "MSM8216"},
> + {249, "MSM8116"},
> + {250, "MSM8616"},
> + {246, "MSM8996"},
> + {310, "MSM8996AU"},
> + {311, "APQ8096AU"},
> + {291, "APQ8096"},
> + {305, "MSM8996SG"},
> + {312, "APQ8096SG"},
> +};
> +
> +static struct smem_image_version *smem_image_version;
> +
> +/* max socinfo format version supported */
> +#define MAX_SOCINFO_FORMAT 12
> +
> +/* socinfo: sysfs functions */
> +
> +static ssize_t
> +qcom_show_vendor(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> + return sprintf(buf, "%s", "Qualcomm\n");

We can't just sprintf(buf, "Qualcomm\n") ?

> +}
> +
> +static ssize_t
> +qcom_show_raw_version(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + return sprintf(buf, "%u\n", le32_to_cpu(socinfo->v0_2.raw_ver));
> +}
> +
> +static ssize_t
> +qcom_show_build_id(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> + return scnprintf(buf, PAGE_SIZE, "%s\n", socinfo->v0_1.build_id);

Why does this one get PAGE_SIZE but others don't?

> +}
> +
> +static ssize_t
> +qcom_show_hw_platform(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + return sprintf(buf, "%u\n", le32_to_cpu(socinfo->v0_3.hw_plat));
> +}
> +
> +static ssize_t
> +qcom_show_platform_version(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + return sprintf(buf, "%u\n", le32_to_cpu(socinfo->v0_4.plat_ver));
> +}
> +
> +static ssize_t
> +qcom_show_accessory_chip(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + return sprintf(buf, "%u\n", le32_to_cpu(socinfo->v0_5.accsry_chip));
> +}
> +
> +static ssize_t
> +qcom_show_platform_subtype(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + u32 subtype = le32_to_cpu(socinfo->v0_6.hw_plat_subtype);
> +
> + if (subtype < 0)

How can an unsigned type be less than zero?

> + return -EINVAL;
> +
> + return sprintf(buf, "%u\n", subtype);
> +}
> +
[...]
> +
> +static const char *socinfo_get_id_string(int id)
> +{
> + int idx;
> +
> + for (idx = 0; idx < ARRAY_SIZE(soc_of_id); idx++) {
> + if (soc_of_id[idx].id == id)
> + return soc_of_id[idx].name;
> + }
> +
> + return NULL;
> +}
> +
> +void qcom_socinfo_init(struct device *device)
> +{
> + struct soc_device_attribute *attr;
> + struct soc_device *soc_dev;
> + struct device *dev;
> + size_t item_size;
> + size_t size;
> + int i;
> +
> + socinfo = qcom_smem_get(QCOM_SMEM_HOST_ANY, SMEM_HW_SW_BUILD_ID,
> + &item_size);
> + if (IS_ERR(socinfo)) {
> + dev_err(device, "Coudn't find socinfo\n");

Couldn't

> + return;
> + }
> +
> + if (SOCINFO_VERSION_MAJOR(le32_to_cpu(socinfo->v0_1.fmt) != 0) ||
> + SOCINFO_VERSION_MINOR(le32_to_cpu(socinfo->v0_1.fmt) < 0) ||
> + le32_to_cpu(socinfo->v0_1.fmt) > MAX_SOCINFO_FORMAT) {
> + dev_err(device, "Wrong socinfo format\n");
> + return;
> + }
> +
> + if (!le32_to_cpu(socinfo->v0_1.id))
> + dev_err(device, "socinfo: Unknown SoC ID!\n");

Drop socinfo: please.

---8<----
diff --git a/drivers/soc/qcom/socinfo.c b/drivers/soc/qcom/socinfo.c
index 495f937ce77b..6a453be283ca 100644
--- a/drivers/soc/qcom/socinfo.c
+++ b/drivers/soc/qcom/socinfo.c
@@ -88,7 +88,7 @@ struct smem_image_attribute {
qcom_show_image_crm, qcom_store_image_crm), \
_index \
}; \
- static struct attribute_group _name##_image_attr_group = { \
+ static const struct attribute_group _name##_image_attr_group = { \
.attrs = (struct attribute*[]) { \
&_name##_image_attrs.version.attr, \
&_name##_image_attrs.variant.attr, \
@@ -99,6 +99,7 @@ struct smem_image_attribute {

static const char *const pmic_model[] = {
[0] = "Unknown PMIC model",
+ [11] = "PM8916",
[13] = "PM8058",
[14] = "PM8028",
[15] = "PM8901",
@@ -453,8 +454,8 @@ QCOM_SMEM_IMG_ITEM(adsp, 0444, SMEM_IMAGE_TABLE_ADSP_INDEX);
QCOM_SMEM_IMG_ITEM(cnss, 0444, SMEM_IMAGE_TABLE_CNSS_INDEX);
QCOM_SMEM_IMG_ITEM(video, 0444, SMEM_IMAGE_TABLE_VIDEO_INDEX);

-static const struct attribute_group
- *smem_img_tbl[SMEM_IMAGE_VERSION_BLOCKS_COUNT] = {
+static const
+struct attribute_group *smem_img_tbl[SMEM_IMAGE_VERSION_BLOCKS_COUNT] = {
[SMEM_IMAGE_TABLE_BOOT_INDEX] = &boot_image_attr_group,
[SMEM_IMAGE_TABLE_TZ_INDEX] = &tz_image_attr_group,
[SMEM_IMAGE_TABLE_RPM_INDEX] = &rpm_image_attr_group,
@@ -489,7 +490,7 @@ void qcom_socinfo_init(struct device *device)
socinfo = qcom_smem_get(QCOM_SMEM_HOST_ANY, SMEM_HW_SW_BUILD_ID,
&item_size);
if (IS_ERR(socinfo)) {
- dev_err(device, "Coudn't find socinfo\n");
+ dev_err(device, "Couldn't find socinfo\n");
return;
}

@@ -501,7 +502,7 @@ void qcom_socinfo_init(struct device *device)
}

if (!le32_to_cpu(socinfo->v0_1.id))
- dev_err(device, "socinfo: Unknown SoC ID!\n");
+ dev_err(device, "Unknown SoC ID!\n");

smem_image_version = qcom_smem_get(QCOM_SMEM_HOST_ANY,
SMEM_IMAGE_VERSION_TABLE,
@@ -534,7 +535,7 @@ void qcom_socinfo_init(struct device *device)
/*
* Expose SMEM_IMAGE_TABLE to sysfs only when we have IMAGE_TABLE
* available in SMEM. As IMAGE_TABLE and SOCINFO are two separate
- * items within SMEM, we expose the remaining soc information(i.e
+ * items within SMEM, we expose the remaining soc information (i.e
* only the SOCINFO item available in SMEM) to sysfs even in the
* absence of an IMAGE_TABLE.
*/
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Next message: Dmitry Torokhov: "[PATCH] Input: tsc2007 - switch to using input_set_capability()"
Previous message: Joe Perches: "Re: [PATCH] Staging: ks7010: ks_*: Braces should be used on all arms of these statements"
In reply to: Bjorn Andersson: "Re: [PATCH v9] soc: qcom: Add SoC info driver"
Next in thread: Imran Khan: "Re: [PATCH v9] soc: qcom: Add SoC info driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]