From patchwork Fri Feb 17 22:51:05 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Stephen Boyd X-Patchwork-Id: 9580851 X-Patchwork-Delegate: agross@codeaurora.org Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 094B66043A for ; Fri, 17 Feb 2017 22:51:25 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id F37D528775 for ; Fri, 17 Feb 2017 22:51:24 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id E715128795; Fri, 17 Feb 2017 22:51:24 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.8 required=2.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_HI,T_DKIM_INVALID autolearn=unavailable version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id E0D162878E for ; Fri, 17 Feb 2017 22:51:23 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S964787AbdBQWvK (ORCPT ); Fri, 17 Feb 2017 17:51:10 -0500 Received: from smtp.codeaurora.org ([198.145.29.96]:52328 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S964828AbdBQWvH (ORCPT ); Fri, 17 Feb 2017 17:51:07 -0500 Received: by smtp.codeaurora.org (Postfix, from userid 1000) id 15470607EE; Fri, 17 Feb 2017 22:51:07 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=codeaurora.org; s=default; t=1487371867; bh=zC//Df0nguE/jIzDL/5FlTrBf9ZXSU6Vu8lluipm+eg=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=Fn27+3p00A8HnYqbVr5UmNFhNtJv8eRcoakTIFumqsHFuHdeHEY+eXbIXWgC1HZBY 0bINx2GD5jBEDacIGlWdV7QUGy9lEoL5BLhTVqWiiypuSwlxUjtTVBfjlKSRtzMZ2r SCBSkTUOMPvFnmIyXN/1pnY/y+d95K9Xb6VwfIAM= Received: from localhost (i-global254.qualcomm.com [199.106.103.254]) (using TLSv1.2 with cipher DHE-RSA-AES128-SHA (128/128 bits)) (No client certificate requested) (Authenticated sender: sboyd@smtp.codeaurora.org) by smtp.codeaurora.org (Postfix) with ESMTPSA id 26A9A6070C; Fri, 17 Feb 2017 22:51:06 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=codeaurora.org; s=default; t=1487371866; bh=zC//Df0nguE/jIzDL/5FlTrBf9ZXSU6Vu8lluipm+eg=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=mGdgSvQrcrN9jnMdMhOxR2HWY0RqHxal4+DFqfEAKyK1uIhYr5mLZoUCJ2xVk+3xu KENXDNmgtRnFyGxvIB6QeCZEWk+6JchTDs46m9J9dXM7C3qqCQuWnSosY+IPmak21e WcgAr/4eob67IddNyXdxikMlJtKfCKBJvRhr5FgU= DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 26A9A6070C Authentication-Results: pdx-caf-mail.web.codeaurora.org; dmarc=none (p=none dis=none) header.from=codeaurora.org Authentication-Results: pdx-caf-mail.web.codeaurora.org; spf=none smtp.mailfrom=sboyd@codeaurora.org Date: Fri, 17 Feb 2017 14:51:05 -0800 From: Stephen Boyd To: Imran Khan Cc: bjorn.andersson@linaro.org, agross@codeaurora.org, linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org, linux-soc@vger.kernel.org Subject: Re: [PATCH v9] soc: qcom: Add SoC info driver Message-ID: <20170217225105.GC25384@codeaurora.org> References: <1487241927-18201-1-git-send-email-kimran@codeaurora.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <1487241927-18201-1-git-send-email-kimran@codeaurora.org> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-arm-msm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-arm-msm@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP 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 > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +/* > + * 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. */