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

[Imran Khan]

On 10/28/2016 1:35 AM, Linus Walleij wrote:
> On Thu, Oct 27, 2016 at 11:22 AM, Imran Khan <kimran@xxxxxxxxxxxxxx> wrote:
> 
>> The SoC info driver provides information such as Chip ID,
>> Chip family, serial number and other such details about
>> Qualcomm SoCs.
>>
>> Signed-off-by: Imran Khan <kimran@xxxxxxxxxxxxxx>
> 
> I like this patch. It is good to know stuff about the hardware.
> 

Thanks Linus for the review comments.

> (...)
>> +++ b/drivers/soc/qcom/socinfo.c
> (...)
>> +#include <linux/export.h>
>> +#include <linux/module.h>
>> +#include <linux/err.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/sys_soc.h>
> 
> Uses <linux/sys_soc.h> so you should definately get Lee Jones
> to review this.
> 
Okay. Will add Lee Jones while sending future patch sets.

>> +/*
>> + * Qcom SoC IDs
>> + */
>> +enum qcom_cpu_id {
>> +       MSM_UNKNOWN_ID,
>> +       MSM_8960_ID = 87,
>> +       APQ_8064_ID = 109,
>> +       MSM_8660A_ID = 122,
>> +       MSM_8260A_ID,
>> +       APQ_8060A_ID,
>> +       MSM_8974_ID = 126,
>> +       MPQ_8064_ID = 130,
>> +       MSM_8960AB_ID = 138,
>> +       APQ_8060AB_ID,
>> +       MSM_8260AB_ID,
>> +       MSM_8660AB_ID,
>> +       APQ_8084_ID = 178,
>> +       APQ_8074_ID = 184,
>> +       MSM_8274_ID,
>> +       MSM_8674_ID,
>> +       MSM_8974PRO_ID = 194,
>> +       MSM_8916_ID = 206,
>> +       APQ_8074_AA_ID = 208,
>> +       APQ_8074_AB_ID,
>> +       APQ_8074PRO_ID,
>> +       MSM_8274_AA_ID,
>> +       MSM_8274_AB_ID,
>> +       MSM_8274PRO_ID,
>> +       MSM_8674_AA_ID,
>> +       MSM_8674_AB_ID,
>> +       MSM_8674PRO_ID,
>> +       MSM_8974_AA_ID,
>> +       MSM_8974_AB_ID,
>> +       MSM_8996_ID = 246,
>> +       APQ_8016_ID,
>> +       MSM_8216_ID,
>> +       MSM_8116_ID,
>> +       MSM_8616_ID,
>> +       APQ8096_ID = 291,
>> +       MSM_8996SG_ID = 305,
>> +       MSM_8996AU_ID = 310,
>> +       APQ_8096AU_ID,
>> +       APQ_8096SG_ID
>> +};
> 
> Seems to support everything and its dog. Good job!
> 
> Now I guess I first have to support smem on my pet peeves
> (MSM8660 and APQ8060) for this to work.
> 

Yes. Actually socinfo is one of the smem items and can be accessed
only via smem interface.

>> +enum hw_platform_type {
>> +       HW_PLATFORM_UNKNOWN,
>> +       HW_PLATFORM_SURF,
>> +       HW_PLATFORM_FFA,
>> +       HW_PLATFORM_FLUID,
>> +       HW_PLATFORM_SVLTE_FFA,
>> +       HW_PLATFORM_SVLTE_SURF,
>> +       HW_PLATFORM_MTP_MDM = 7,
>> +       HW_PLATFORM_MTP,
>> +       HW_PLATFORM_LIQUID,
>> +       /* Dragonboard platform id is assigned as 10 in CDT */
>> +       HW_PLATFORM_DRAGON,
> 
> I guess that is not "my" dragonboard generation 1 with the APQ8060
> but rather the APQ8074 Dragonboard?
>

Let me check this and get back to you.
 
> Is mine just a variant of SURF then... I dunno. Will be interesting
> to see what number it has in its hardware.
> 
>> +#ifdef CONFIG_SOC_BUS
> 
> Why not just select that in Kconfig and get rid of this #ifdef?
> 

As far as socinfo driver is concerned, it's main purpose is to read 
some information already latched in smem. So instead of having a 
separate platform driver, we have just rolled this into smem driver 
itself. On the other hand it may not be necessary to expose the socinfo
to user space via sysfs so we may have a kernel config with QCOM_SMEM
enabled while SOC_BUS not enabled at the same time and in such cases
this #ifdef becomes useful. That's why instead of making entire
socinfo.c dependent on CONFIG_SOC_BUS, we are just making a part of this 
file dependent on this flag. 
Please let me know if you have some suggestion to improve on this.

>> +int qcom_socinfo_init(void *info)
>> +{
>> +       socinfo = info;
>> +
>> +       socinfo_select_format();
>> +
>> +       WARN(!socinfo_get_id(), "Unknown SOC ID!\n");
>> +
>> +       WARN(socinfo_get_id() >= ARRAY_SIZE(cpu_of_id),
>> +               "New IDs added! ID => CPU mapping needs an update.\n");
>> +
>> +       socinfo_print();
>> +
>> +       socinfo_init_sysfs();
>> +
>> +       return 0;
>> +}
>> +EXPORT_SYMBOL(qcom_socinfo_init);
> 
> So something I've been adding to drivers that handle any kind of
> device-unique numbers is:
> 
> #include <linux/random.h>
> 
> /* Toss the unique data into the entropy pool */
> add_device_randomness(buf, sizeof(buf));
> 
> Can you do this here in init() with the register that are *device unique*
> because I guess not all of them are?
> 

The socinfo item that we obtain from smem here will be unique to each soc
and I have added support to feed this data into entropy pool. This change
will be available in next patch set.

 --Imran

> Yours,
> Linus Walleij
> 


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