Message-ID: <20170217225105.GC25384@codeaurora.org>
Date:   Fri, 17 Feb 2017 14:51:05 -0800
From:   Stephen Boyd <sboyd@...eaurora.org>
To:     Imran Khan <kimran@...eaurora.org>
Cc:     bjorn.andersson@...aro.org, agross@...eaurora.org,
        linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-soc@...r.kernel.org
Subject: Re: [PATCH v9] soc: qcom: Add SoC info driver

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

Powered by blists - more mailing lists