MIME-Version: 1.0
In-Reply-To: <1477560155-17044-1-git-send-email-kimran@codeaurora.org>
References: <1477560155-17044-1-git-send-email-kimran@codeaurora.org>
From: Linus Walleij <linus.walleij@linaro.org>
Date: Thu, 27 Oct 2016 22:05:44 +0200
Message-ID: <CACRpkdZTz-oYksCeAw1_4DG40zy7fBcL-084kSG61q-a9pmbcA@mail.gmail.com>
Subject: Re: [PATCH v2] soc: qcom: Add SoC info driver
To: Imran Khan <kimran@codeaurora.org>, Lee Jones <lee.jones@linaro.org>
Cc: Andy Gross <andy.gross@linaro.org>,
        David Brown <david.brown@linaro.org>,
        open list <linux-kernel@vger.kernel.org>,
        "open list:ARM/QUALCOMM SUPPORT" <linux-arm-msm@vger.kernel.org>,
        "open list:ARM/QUALCOMM SUPPORT" <linux-soc@vger.kernel.org>
Content-Type: text/plain; charset=UTF-8
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3374
Lines: 128

On Thu, Oct 27, 2016 at 11:22 AM, Imran Khan <kimran@codeaurora.org> 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@codeaurora.org>

I like this patch. It is good to know stuff about the hardware.

(...)
> +++ 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.

> +/*
> + * 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.

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

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?

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

Yours,
Linus Walleij