Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1580960 > unrolled thread

Re: [PATCH v8 1/2] soc: qcom: Add SoC info driver

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v8 1/2] soc: qcom: Add SoC info driver Bjorn Andersson <bjorn.andersson@linaro.org> - 2017-02-15 01:30 +0100
    Re: [PATCH v8 1/2] soc: qcom: Add SoC info driver Imran Khan <kimran@codeaurora.org> - 2017-02-15 06:40 +0100
      Re: [PATCH v8 1/2] soc: qcom: Add SoC info driver Bjorn Andersson <bjorn.andersson@linaro.org> - 2017-02-16 05:40 +0100

#1580960 — Re: [PATCH v8 1/2] soc: qcom: Add SoC info driver

On Tue 10 Jan 07:48 PST 2017, Imran Khan wrote:

> diff --git a/drivers/soc/qcom/socinfo.c b/drivers/soc/qcom/socinfo.c
> new file mode 100644
> index 0000000..40c180d
> --- /dev/null
> +++ b/drivers/soc/qcom/socinfo.c
> @@ -0,0 +1,516 @@
> +/*
> + * Copyright (c) 2009-2016, The Linux Foundation. All rights reserved.

Sorry for the slow progress, please bump the year now.

[..]
> +
> +static const char *const pmic_model[] = {
> +	[0]  = "Unknown PMIC model",
> +	[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",
> +};

I thought the conclusion was to drop the "hw_platform",
"qrd_hw_platform_subtype" and hw_platform_subtype" lists, but to keep
the cpu_of_id (although named soc_of_id).

The hw_platform ids are re-used by ODMs and might have different
meaning, but the SoC name is quite useful. So please put the soc_of_id
back in here.

[..]
> +
> +static ssize_t
> +qcom_odm_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	return scnprintf(buf, PAGE_SIZE, "%s\n", odm_name);
> +}
> +DEVICE_ATTR_RO(qcom_odm);

In the case that ODMs will start using this implementation for
communicating information to user-space I believe a name will not be
enough - most likely there would be some product name and some
revision/build information.

My suggestion is that you just skip the "odm" attribute in your patch,
this gives a good opportunity for the ODMs to make a simple contribution
of what they actually want here.

[..]
> +
> +void qcom_socinfo_init(struct device *device)
> +{
> +	struct soc_device_attribute *attr;
> +	const struct fdt_property *prop;
> +	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");
> +		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)) {

Indent wrapped lines by the start parenthesis and skip the extra
parenthesis please.

I.e.

if (... ||
    ... ||
    ...) {

> +		dev_err(device, "Wrong socinfo format\n");
> +		return;
> +	}
> +

[..]

> +
> +	odm_name = of_get_property(device->of_node, "qcom,odm", NULL);
> +	if (odm_name)
> +		device_create_file(dev, &dev_attr_qcom_odm);

Skip this for now.

> +
> +	/* Feed the soc specific unique data into entropy pool */
> +	add_device_randomness(socinfo, item_size);
> +}
> +EXPORT_SYMBOL(qcom_socinfo_init);

Please add me as To: on the next spin of your patch so I don't miss it
on the mailing list. I do expect to be able to recommend Andy to merge
that version.

Regards,
Bjorn

[toc] | [next] | [standalone]

#1581070

On 2/15/2017 5:54 AM, Bjorn Andersson wrote:
> On Tue 10 Jan 07:48 PST 2017, Imran Khan wrote:
> 
>> diff --git a/drivers/soc/qcom/socinfo.c b/drivers/soc/qcom/socinfo.c
>> new file mode 100644
>> index 0000000..40c180d
>> --- /dev/null
>> +++ b/drivers/soc/qcom/socinfo.c
>> @@ -0,0 +1,516 @@
>> +/*
>> + * Copyright (c) 2009-2016, The Linux Foundation. All rights reserved.
> 
> Sorry for the slow progress, please bump the year now.
>

Sure, Will update the year.
 
> [..]
>> +
>> +static const char *const pmic_model[] = {
>> +	[0]  = "Unknown PMIC model",
>> +	[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",
>> +};
> 
> I thought the conclusion was to drop the "hw_platform",
> "qrd_hw_platform_subtype" and hw_platform_subtype" lists, but to keep
> the cpu_of_id (although named soc_of_id).
> 
> The hw_platform ids are re-used by ODMs and might have different
> meaning, but the SoC name is quite useful. So please put the soc_of_id
> back in here.
> 

cpu_of_id was mapping soc-id read from SMEM into SoC name, which in turn
was being used as machine name in soc_device_attribute. But we can also
read machine name from DT. Reading the machine name from DT would make the 
solution more flexible as we don't have to make an entry in soc_of_id every
time we get a new SoC.
Also as soc_of_id uses soc-id as index, there is theoretically no limit on how
many elements it may end up having.
Because of the above reasons, I wanted to get rid off soc_of_id (or cpu_of_id)
array and use DT to get machine name as shown in the following snippet:

+	attr->family = "Snapdragon";
+	prop = fdt_get_property(initial_boot_params, 0, "model", NULL);
+	if (prop)
+		attr->machine = kasprintf(GFP_KERNEL, "%s", prop->data);

Could you please let me know if this approach looks fine to you?

> [..]
>> +
>> +static ssize_t
>> +qcom_odm_show(struct device *dev, struct device_attribute *attr, char *buf)
>> +{
>> +	return scnprintf(buf, PAGE_SIZE, "%s\n", odm_name);
>> +}
>> +DEVICE_ATTR_RO(qcom_odm);
> 
> In the case that ODMs will start using this implementation for
> communicating information to user-space I believe a name will not be
> enough - most likely there would be some product name and some
> revision/build information.
> 
> My suggestion is that you just skip the "odm" attribute in your patch,
> this gives a good opportunity for the ODMs to make a simple contribution
> of what they actually want here.
> 
> [..]

Okay, will skip the odm attribute.
>> +
>> +void qcom_socinfo_init(struct device *device)
>> +{
>> +	struct soc_device_attribute *attr;
>> +	const struct fdt_property *prop;
>> +	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");
>> +		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)) {
> 
> Indent wrapped lines by the start parenthesis and skip the extra
> parenthesis please.
> 
> I.e.
> 
> if (... ||
>     ... ||
>     ...) {
> 
>> +		dev_err(device, "Wrong socinfo format\n");
>> +		return;
>> +	}
>> +
> 
> [..]
> 
>> +
>> +	odm_name = of_get_property(device->of_node, "qcom,odm", NULL);
>> +	if (odm_name)
>> +		device_create_file(dev, &dev_attr_qcom_odm);
> 
> Skip this for now.
> 

Okay.
>> +
>> +	/* Feed the soc specific unique data into entropy pool */
>> +	add_device_randomness(socinfo, item_size);
>> +}
>> +EXPORT_SYMBOL(qcom_socinfo_init);
> 
> Please add me as To: on the next spin of your patch so I don't miss it
> on the mailing list. I do expect to be able to recommend Andy to merge
> that version.
> 
> Regards,
> Bjorn

Sure. Will send the next patch set once I have confirmation of the above
query. 

Thanks and Regards,
Imran

> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


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

[toc] | [prev] | [next] | [standalone]

#1582308

On Tue 14 Feb 21:31 PST 2017, Imran Khan wrote:

> On 2/15/2017 5:54 AM, Bjorn Andersson wrote:
[..]
> > 
> > I thought the conclusion was to drop the "hw_platform",
> > "qrd_hw_platform_subtype" and hw_platform_subtype" lists, but to keep
> > the cpu_of_id (although named soc_of_id).
> > 
> > The hw_platform ids are re-used by ODMs and might have different
> > meaning, but the SoC name is quite useful. So please put the soc_of_id
> > back in here.
> > 
> 
> cpu_of_id was mapping soc-id read from SMEM into SoC name, which in turn
> was being used as machine name in soc_device_attribute. But we can also
> read machine name from DT. Reading the machine name from DT would make the 
> solution more flexible as we don't have to make an entry in soc_of_id every
> time we get a new SoC.

I don't think we need a more flexible solution, as the purpose of this
driver is to expose the socinfo information to user space; i.e. there's
nothing flexible about that.

If we just expose this information as a proprietary magic number, then
it will be up to each consumer to keep some mapping table. So you will
not get rid of the work of keeping things updated, you will just push
the problem to somewhere else.

> Also as soc_of_id uses soc-id as index, there is theoretically no limit on how
> many elements it may end up having.

There's a practical - and reasonable - limit on how much space this data
will take, one could however argue that it's unwise to keep the sparse
data in a simple array. A better solution would be to maintain an array
of id,soc pairs and loop through that on lookup.

> Because of the above reasons, I wanted to get rid off soc_of_id (or cpu_of_id)
> array and use DT to get machine name as shown in the following snippet:
> 
> +	attr->family = "Snapdragon";
> +	prop = fdt_get_property(initial_boot_params, 0, "model", NULL);
> +	if (prop)
> +		attr->machine = kasprintf(GFP_KERNEL, "%s", prop->data);

The data is already in socinfo, we should not manually duplicate it in
DT.

> 
> Could you please let me know if this approach looks fine to you?
> 

I would prefer that the socinfo doesn't expose magic numbers without a
publicly available lookup table. And I don't see a need for the added
flexibility.

Regards,
Bjorn

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web