[PATCH v3 5/6] soc: samsung: exynos-chipid: Add Exynos Chipid driver support
Tomasz Figa
t.figa at samsung.com
Tue Jun 10 07:36:34 PDT 2014
Hi Pankaj,
On 10.05.2014 09:20, Pankaj Dubey wrote:
> Exynos SoCs have Chipid, for identification of product IDs
> and SoC revistions. Till now we are using static macros
> such as soc_is_exynosNNNN and #ifdefs for run time identification
> of SoCs and their revisions. This is leading to add new Kconfig,
> soc_is_exynosNNNN definitions each time new SoC support is getting
> added. So this driver intends to provide initialization code
> all these functionalites and thus helping in removing macros.
>
> Signed-off-by: Pankaj Dubey <pankaj.dubey at samsung.com>
> ---
> drivers/soc/Kconfig | 1 +
> drivers/soc/Makefile | 1 +
> drivers/soc/samsung/Kconfig | 10 +++
> drivers/soc/samsung/Makefile | 1 +
> drivers/soc/samsung/exynos-chipid.c | 166 +++++++++++++++++++++++++++++++++++
> include/linux/exynos-soc.h | 46 ++++++++++
> 6 files changed, 225 insertions(+)
> create mode 100644 drivers/soc/samsung/Kconfig
> create mode 100644 drivers/soc/samsung/Makefile
> create mode 100644 drivers/soc/samsung/exynos-chipid.c
> create mode 100644 include/linux/exynos-soc.h
>
> diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig
> index 07a11be..86e52b1 100644
> --- a/drivers/soc/Kconfig
> +++ b/drivers/soc/Kconfig
> @@ -1,5 +1,6 @@
> menu "SOC specific Drivers"
>
> source "drivers/soc/qcom/Kconfig"
> +source "drivers/soc/samsung/Kconfig"
>
> endmenu
> diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
> index 0f7c447..ee890aa 100644
> --- a/drivers/soc/Makefile
> +++ b/drivers/soc/Makefile
> @@ -3,3 +3,4 @@
> #
>
> obj-$(CONFIG_ARCH_QCOM) += qcom/
> +obj-$(CONFIG_ARCH_EXYNOS) += samsung/
EXYNOS or samsung/?
Also this is just a single file, is it necessary to create a directory
for it?
> diff --git a/drivers/soc/samsung/Kconfig b/drivers/soc/samsung/Kconfig
> new file mode 100644
> index 0000000..dacc95d
> --- /dev/null
> +++ b/drivers/soc/samsung/Kconfig
> @@ -0,0 +1,10 @@
> +
> +# SAMSUNG Soc drivers
> +#
> +config EXYNOS_CHIPID
> + tristate "Support Exynos CHIPID"
> + default y
> + select SOC_BUS
> + depends on ARCH_EXYNOS || ARM64
> + help
> + If you say Y here you get support for the Exynos CHIP id.
Do we need to have this user-selectable? Couldn't we just have it
selected by ARCH_EXYNOS and its 64-bit counterpart? AFAIK per-family
Kconfig symbols are already present (see ARCH_VEXPRESS and ARCH_XGENE in
arch/arm64/Kconfig), so it is not a problem.
> diff --git a/drivers/soc/samsung/Makefile b/drivers/soc/samsung/Makefile
> new file mode 100644
> index 0000000..855ca05
> --- /dev/null
> +++ b/drivers/soc/samsung/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_EXYNOS_CHIPID) += exynos-chipid.o
> diff --git a/drivers/soc/samsung/exynos-chipid.c b/drivers/soc/samsung/exynos-chipid.c
> new file mode 100644
> index 0000000..b5bccd9
> --- /dev/null
> +++ b/drivers/soc/samsung/exynos-chipid.c
> @@ -0,0 +1,166 @@
> +/*
> + * Copyright (c) 2014 Samsung Electronics Co., Ltd.
> + * http://www.samsung.com/
> + *
> + * EXYNOS - CHIP ID support
> + * Author: Pankaj Dubey <pankaj.dubey at samsung.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/slab.h>
> +#include <linux/sys_soc.h>
> +#include <linux/of_address.h>
> +#include <linux/exynos-soc.h>
> +
> +#define EXYNOS4_SOC_MASK 0xFFFE0
Even though I can see the mask defined in current code to the value
above, the documentation states that the whole bit field [31:12] is used
for product ID, so it should be safe to use the same mask as for Exynos5.
> +#define EXYNOS5_SOC_MASK 0xFFFFF
> +
> +#define PROD_ID_SHIFT (12)
> +
> +static void __iomem *exynos_chipid_base;
> +unsigned int exynos_soc_id = EXYNOS_SOC_UNKNOWN;
static?
> +enum exynos_soc_revision exynos_revision;
static?
> +
> +struct exynos_chipid_data {
> + unsigned int product_id_mask;
> + unsigned int product_id_shift;
> +};
> +
> +static struct exynos_chipid_data exynos4_chipid_data = {
> + .product_id_mask = EXYNOS4_SOC_MASK,
> + .product_id_shift = PROD_ID_SHIFT,
> +};
> +
> +static struct exynos_chipid_data exynos5_chipid_data = {
> + .product_id_mask = EXYNOS5_SOC_MASK,
> + .product_id_shift = PROD_ID_SHIFT,
> +};
> +
> +static struct of_device_id of_exynos_chipid_ids[] = {
> + {
> + .compatible = "samsung,exynos4-chipid",
> + .data = (void *)&exynos4_chipid_data,
> + },
> + {
> + .compatible = "samsung,exynos5-chipid",
> + .data = (void *)&exynos5_chipid_data,
We already have "samsung,exynos4210-chipid" compatible string defined
and used in existing device tree sources (and deployed DTBs).
> + },
> + {},
> +};
> +
> +bool is_soc_id_compatible(enum exynos_soc_id soc_id)
> +{
> + return soc_id == exynos_soc_id;
> +}
> +
> +bool is_soc_rev_compatible(enum exynos_soc_revision soc_rev)
> +{
> + return soc_rev == exynos_revision;
> +}
I don't think we need this API. (See below.)
> +
> +static const char *exynos_soc_id_to_name(enum exynos_soc_id id)
> +{
> + const char *soc_name;
> + switch (id) {
> + case EXYNOS4210:
> + soc_name = "EXYNOS4210";
> + break;
> + case EXYNOS4212:
> + soc_name = "EXYNOS4212";
> + break;
> + case EXYNOS4412:
> + soc_name = "EXYNOS4412";
> + break;
> + case EXYNOS5250:
> + soc_name = "EXYNOS5250";
> + break;
> + case EXYNOS5420:
> + soc_name = "EXYNOS5420";
> + break;
> + case EXYNOS5440:
> + soc_name = "EXYNOS5440";
> + break;
> + default:
> + soc_name = "";
> + }
> + return soc_name;
> +}
> +
> +struct device * __init exynos_soc_device_init(void)
> +{
> + struct soc_device_attribute *soc_dev_attr;
> + struct soc_device *exynos_soc_dev;
> + struct device_node *root;
> + int ret;
> +
> + if (!exynos_chipid_base)
> + early_exynos_chipid_init();
> +
> + soc_dev_attr = kzalloc(sizeof(*soc_dev_attr), GFP_KERNEL);
> + if (!soc_dev_attr)
> + return NULL;
> +
> + soc_dev_attr->family = "Samsung Exynos";
> +
> + root = of_find_node_by_path("/");
> + ret = of_property_read_string(root, "model", &soc_dev_attr->machine);
> + of_node_put(root);
> + if (ret)
> + goto free_soc;
> +
> + soc_dev_attr->revision = kasprintf(GFP_KERNEL, "%d", exynos_revision);
> + if (!soc_dev_attr->revision)
> + goto free_soc;
> +
> + soc_dev_attr->soc_id = exynos_soc_id_to_name(exynos_soc_id);
> +
> + exynos_soc_dev = soc_device_register(soc_dev_attr);
> + if (IS_ERR(exynos_soc_dev))
> + goto free_rev;
> +
> + return soc_device_to_device(exynos_soc_dev);
> +
> +free_rev:
> + kfree(soc_dev_attr->revision);
> +free_soc:
> + kfree(soc_dev_attr);
> + return NULL;
> +}
> +
> +/**
> + * early_exynos_chipid_init - Early chipid initialization
> + */
> +void __init early_exynos_chipid_init(void)
> +{
> + struct device_node *np = NULL;
> + const struct of_device_id *match;
> + struct exynos_chipid_data *chipid_data;
> + int pro_id;
> +
> + if (!exynos_chipid_base) {
> + np = of_find_matching_node_and_match(NULL,
> + of_exynos_chipid_ids, &match);
> + if (!np)
> + panic("%s, failed to find chipid node\n", __func__);
> +
> + chipid_data = (struct exynos_chipid_data *) match->data;
> + exynos_chipid_base = of_iomap(np, 0);
> +
> + if (!exynos_chipid_base)
> + panic("%s: failed to map registers\n", __func__);
> +
> + pro_id = __raw_readl(exynos_chipid_base);
> + exynos_soc_id = (pro_id >> chipid_data->product_id_shift)
> + & chipid_data->product_id_mask;
> + exynos_revision = pro_id & 0xFF;
> + pr_info("Exynos: CPU[%s] CPU_REV[0x%x] Detected\n",
> + exynos_soc_id_to_name(exynos_soc_id),
> + exynos_revision);
> + }
> +
> +}
Basically this API is not much better than soc_is_exynos*(). The goal is
to get rid of per-SoC checks, not provide another API to do this.
In general, a SoC driver may be useful, though. So my proposal is to
strip the whole questionable API from this driver, make it a platform
driver and let it simply register the SoC device.
> diff --git a/include/linux/exynos-soc.h b/include/linux/exynos-soc.h
> new file mode 100644
> index 0000000..ebd4366
> --- /dev/null
> +++ b/include/linux/exynos-soc.h
> @@ -0,0 +1,46 @@
> +/*
> + * Copyright (c) 2014 Samsung Electronics Co., Ltd.
> + * http://www.samsung.com
> + *
> + * Header for EXYNOS SoC Chipid support
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#ifndef __EXYNOS_SOC_H
> +#define __EXYNOS_SOC_H
> +
> +/* enum holding product id of Exynos SoC
> + * Any new Exynos SoC product id should be added here
> + */
> +enum exynos_soc_id {
> + EXYNOS4210 = 0xE4210,
This is not the correct ID of Exynos4210. The correct one is 0x43210.
> + EXYNOS4212 = 0xE4212,
Should be 0x43220.
> + EXYNOS4412 = 0xE4412,
> + EXYNOS5250 = 0x43520,
> + EXYNOS5420 = 0xE5420,
> + EXYNOS5440 = 0xE5440,
> + EXYNOS_SOC_UNKNOWN = -1,
> +};
> +
> +/* enum holding revision id of Exynos SoC
> + * Any new Exynos SoC revision id should be added here
> + */
> +enum exynos_soc_revision {
> + EXYNOS4210_REV_0 = 0x0,
> + EXYNOS4210_REV_1_0 = 0x10,
> + EXYNOS4210_REV_1_1 = 0x11,
> +};
AFAIK, considering my comments above, both SoC and revision IDs can be
defined as macros inside the .c file.
Best regards,
Tomasz
More information about the linux-arm-kernel
mailing list