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

[Imran Khan]

Hi Stephen,
Thanks for your review comments.

On 2/18/2017 4:21 AM, Stephen Boyd wrote:
> 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.
>

It was agreed earlier that we can keep this extern declaration.
Please let me know if it needs to be removed. 
 
>> 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?
> 
Though both of these defines are doing the same thing, but one is used 
for version of SoC/PMIC (e.g. 3.1, 3.3 etc.) while other is used for version
of socinfo. We have different versions of socinfo and the information 
conveyed by one version of socinfo differs from that conveyed by other
version.
The main purpose of keeping 2 macros was to avoid confusion in readability of 
code, so that a user of the code can understand where we are dealing version
of SoC/PMIC and where we are dealing with version of socinfo.
Where we need to deal with version of socinfo we use SOCINFO_VERSION_MAJOR/
MINOR otherwise we use SOC_VER_MAJ/MIN.
Please let me know if this looks okay to you.

>> +
>> +#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
>> +

<snip>

>> +#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.
> 
Done.
>> +		.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.
>

Okay. PM8916 and PM8994 have been added now. Also I found that I was not
interpreting pmic_model field and pmic_die_revision fields properly, 
I have corrected it in the next patch set. Now we should see correct values
for db410c too.

 
>> +	[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",
>> +};
>> +
<snip>
>> +};
>> +
>> +/* 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?
>

build_id is a C style string.
 
>> +};
>> +
>> +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?
> 

Done.

>> +};
>> +
>> +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?
> 

True. But right now we are just providing the information about one PMIC,
so we are using the format as specified in socinfo version 7. This allows
us to show PMIC information for all socinfo versions from 7 on wards(PMIC
information was not available before version 7)
Later on, I intend to enhance this part to show information of multiple PMICs,
according to socifo versions 8 and 11.
Please let me know if it sounds okay to you.
 
>> +
>> +struct socinfo_v0_9 {
>> +	struct socinfo_v0_8 v0_8;
>> +	__le32 fndry_id;
> 
> Can we write foundry_id?
> 

Done.

>> +};
>> +
>> +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.
>

Done. Had trimmed some of the member names to avoid extra lines because
of indentation.
 
>> +};
>> +
>> +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?
> 

Done.

>> +	__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?
>

Done. The main reason behind appending subsequent versions was to 
keep the fields added by newer versions in order, so that they can
be accessed properly. However the same can be done using a single
structure as you have suggested. I have dropped the version specific
objects.
 
>> +} *socinfo;
>> +
>> +static struct qcom_soc_info soc_of_id[] = {
> 
> const?
> 

Done.

>> +	{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"},

<snip>
>> +/* 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") ?
> 

True. But we don't have any information regarding Vendor in the socinfo. So
we were using a hard coded value here. But as this information is not coming from
socinfo neither it is required in the generic soc_device_attribute, we may altogether
drop this attribute as well. 
Please let me know your view in this regard.

>> +}
>> +
>> +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?
>

The places, where sprintf is being used, are not getting PAGE_SIZE. Now to keep the
implementation consistent I have used scnprintf throughout. As anyways sysfs gives
a buffer of PAGE_SIZE, so I have used this size throughout with scnprintf in show
functions.
 
>> +}
>> +

<snip>

>> +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?
>
 
It can't. Have corrected type for subtype.

>> +		return -EINVAL;
>> +
>> +	return sprintf(buf, "%u\n", subtype);
>> +}

<snip>

>> +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
>

Done.
 
>> +		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.
>
Done.
 
> ---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.
>  	 */
> 
Thanks for the patch.

Thanks and Regards,
Imran

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a\nmember of the Code Aurora Forum, hosted by The Linux Foundation