[PATCH 0/4] Introducing Exynos ChipId driver

[PATCH 0/4] Introducing Exynos ChipId driver

[Pankaj Dubey]

This patch series attempts to get rid of soc_is_exynosXXXX macros
and eventually with the help of this series we can probably get
rid of CONFIG_SOC_EXYNOSXXXX in near future.
Each Exynos SoC has ChipID block which can give information about
SoC's product Id and revision number. Currently we have single
DT binding information for this as "samsung,exynos4210-chipid".
But Exynos4 and Exynos5 SoC series have one small difference in
chip Id, with resepect to product id bit-masks. So it means we
should have separate compatible string for these different series
of SoCs. So I have created new binding information for handling
this difference. Also currently I can think of putting this driver
code under "drivers/misc/" but suggestions are welcome.
Also current form of driver is missing platfrom driver and needs
init function to be called from machine file (either exynos.c or
platsmp.c). I hope lot of suggestions and comments to improve this
further.

This patch series is based on Kukjin Kim's for-next (3.14_rc1 tag)
and prepared on top of following patch series and it's dependent
patch series.

[1]: Map SYSRAM through generic SRAM bindings.
	http://www.spinics.net/lists/arm-kernel/msg327677.html
[2]: Exynos PMU cleanup and refactoring.
	https://lkml.org/lkml/2014/4/30/44

Pankaj Dubey (4):
  ARM: EXYNOS: remove soc_is_exynos4/5 from exynos.c
  ARM:  EXYNOS: remove unused header inclusion from hotplug.c
  misc: exynos-chipid: Add Exynos Chipid driver support
  ARM: EXYNOS: Refactoring to remove soc_is_exynosXXXX macros from
    exynos

 .../bindings/arm/samsung/exynos-chipid.txt         |   15 ++++
 arch/arm/Kconfig                                   |    1 +
 arch/arm/boot/dts/exynos4.dtsi                     |    2 +-
 arch/arm/boot/dts/exynos5.dtsi                     |    2 +-
 arch/arm/mach-exynos/exynos.c                      |   66 ++++++++--------
 arch/arm/mach-exynos/hotplug.c                     |    2 -
 arch/arm/mach-exynos/platsmp.c                     |   10 ++-
 arch/arm/mach-exynos/pm.c                          |   28 +++----
 arch/arm/plat-samsung/include/plat/cpu.h           |   60 --------------
 drivers/clk/samsung/clk-exynos4.c                  |    2 +-
 drivers/cpufreq/exynos-cpufreq.c                   |    9 +--
 drivers/cpufreq/exynos-cpufreq.h                   |    1 -
 drivers/cpufreq/exynos4x12-cpufreq.c               |    5 +-
 drivers/misc/Kconfig                               |    7 ++
 drivers/misc/Makefile                              |    1 +
 drivers/misc/exynos-chipid.c                       |   83 ++++++++++++++++++++
 include/linux/exynos-soc.h                         |   46 +++++++++++
 17 files changed, 215 insertions(+), 125 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arm/samsung/exynos-chipid.txt
 create mode 100644 drivers/misc/exynos-chipid.c
 create mode 100644 include/linux/exynos-soc.h

-- 
1.7.10.4

[PATCH 1/4] ARM: EXYNOS: remove soc_is_exynos4/5 from exynos.c

[Pankaj Dubey]

This patch adds support for checking soc compatibility based on
compatibility match. It will help us in removing soc_is_exynos4
and soc_is_exynos5 function usage and definition.

CC: Russell King <linux@arm.linux.org.uk>
CC: Heiko Stuebner <heiko@sntech.de>
CC: Thomas Abraham <thomas.abraham@linaro.org>
Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com>
---
 arch/arm/mach-exynos/exynos.c            |   30 +++++++++++++++++++++++++++---
 arch/arm/plat-samsung/include/plat/cpu.h |    3 ---
 2 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/arch/arm/mach-exynos/exynos.c b/arch/arm/mach-exynos/exynos.c
index 59eb1f1..93ae076 100644
--- a/arch/arm/mach-exynos/exynos.c
+++ b/arch/arm/mach-exynos/exynos.c
@@ -212,6 +212,30 @@ static const struct of_device_id exynos_dt_pmu_match[] = {
 	{},
 };
 
+static const struct of_device_id exynos5_device_ids[] = {
+	{ .compatible = "samsung,exynos5250", },
+	{ .compatible = "samsung,exynos5420", },
+	{},
+};
+
+static const struct of_device_id exynos4_device_ids[] = {
+	{ .compatible = "samsung,exynos4210", },
+	{ .compatible = "samsung,exynos4212", },
+	{ .compatible = "samsung,exynos4412", },
+	{},
+};
+
+static inline bool soc_is_compatible(const struct of_device_id *device_ids)
+{
+	unsigned long root = of_get_flat_dt_root();
+	const struct of_device_id *matches = device_ids;
+	for (; matches->compatible[0]; matches++) {
+		if (of_flat_dt_is_compatible(root, matches->compatible))
+			return true;
+	}
+	return false;
+}
+
 /*
  * exynos_map_io
  *
@@ -219,10 +243,10 @@ static const struct of_device_id exynos_dt_pmu_match[] = {
  */
 static void __init exynos_map_io(void)
 {
-	if (soc_is_exynos4())
+	if (soc_is_compatible(exynos4_device_ids))
 		iotable_init(exynos4_iodesc, ARRAY_SIZE(exynos4_iodesc));
 
-	if (soc_is_exynos5())
+	if (soc_is_compatible(exynos5_device_ids))
 		iotable_init(exynos5_iodesc, ARRAY_SIZE(exynos5_iodesc));
 }
 
@@ -306,7 +330,7 @@ static void __init exynos_dt_machine_init(void)
 	 * are available then re-configure the interrupts via the
 	 * system register.
 	 */
-	if (soc_is_exynos5()) {
+	if (soc_is_compatible(exynos5_device_ids)) {
 		for_each_compatible_node(i2c_np, NULL, i2c_compat) {
 			if (of_device_is_available(i2c_np)) {
 				id = of_alias_get_id(i2c_np, "i2c");
diff --git a/arch/arm/plat-samsung/include/plat/cpu.h b/arch/arm/plat-samsung/include/plat/cpu.h
index 5992b8d..18a9a00 100644
--- a/arch/arm/plat-samsung/include/plat/cpu.h
+++ b/arch/arm/plat-samsung/include/plat/cpu.h
@@ -166,9 +166,6 @@ IS_SAMSUNG_CPU(exynos5440, EXYNOS5440_SOC_ID, EXYNOS5_SOC_MASK)
 # define soc_is_exynos5440()	0
 #endif
 
-#define soc_is_exynos4() (soc_is_exynos4210() || soc_is_exynos4212() || \
-			  soc_is_exynos4412())
-#define soc_is_exynos5() (soc_is_exynos5250() || soc_is_exynos5420())
 
 #define IODESC_ENT(x) { (unsigned long)S3C24XX_VA_##x, __phys_to_pfn(S3C24XX_PA_##x), S3C24XX_SZ_##x, MT_DEVICE }
 
-- 
1.7.10.4

[PATCH 2/4] ARM: EXYNOS: remove unused header inclusion from hotplug.c

[Pankaj Dubey]

This patch removed "plat/cpu.h" inclusion from hotplug.c as it
is not required.

Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com>
---
 arch/arm/mach-exynos/hotplug.c |    2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/arm/mach-exynos/hotplug.c b/arch/arm/mach-exynos/hotplug.c
index 0243ef3..5e19601 100644
--- a/arch/arm/mach-exynos/hotplug.c
+++ b/arch/arm/mach-exynos/hotplug.c
@@ -19,8 +19,6 @@
 #include <asm/cp15.h>
 #include <asm/smp_plat.h>
 
-#include <plat/cpu.h>
-
 #include "common.h"
 #include "regs-pmu.h"
 
-- 
1.7.10.4

[PATCH 3/4] misc: exynos-chipid: Add Exynos Chipid driver support

[Pankaj Dubey]

Exynos SoCs have Chipid IP, for identification of product IDs
and SoC revistions. Till now we are using static macros
such as soc_is_exynosxxxx and #ifdefs for run time identification
of SoCs and their revisions. This is leading to add new Kconfig,
soc_is_exynosXXXX 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.

CC: Arnd Bergmann <arnd@arndb.de>
CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com>
---
 drivers/misc/Kconfig         |    7 ++++
 drivers/misc/Makefile        |    1 +
 drivers/misc/exynos-chipid.c |   83 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/exynos-soc.h   |   46 +++++++++++++++++++++++
 4 files changed, 137 insertions(+)
 create mode 100644 drivers/misc/exynos-chipid.c
 create mode 100644 include/linux/exynos-soc.h

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 1cb7408..f313bd3 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -515,6 +515,13 @@ config SRAM
 	  the genalloc API. It is supposed to be used for small on-chip SRAM
 	  areas found on many SoCs.
 
+config EXYNOS_CHIPID
+	tristate "Support Exynos CHIPID"
+	default y
+	depends on ARCH_EXYNOS || ARM64
+	help
+	  If you say Y here you get support for the Exynos CHIP id.
+
 source "drivers/misc/c2port/Kconfig"
 source "drivers/misc/eeprom/Kconfig"
 source "drivers/misc/cb710/Kconfig"
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index 7eb4b69..48c8fb5 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -55,3 +55,4 @@ obj-$(CONFIG_SRAM)		+= sram.o
 obj-y				+= mic/
 obj-$(CONFIG_GENWQE)		+= genwqe/
 obj-$(CONFIG_ECHO)		+= echo/
+obj-$(CONFIG_EXYNOS_CHIPID)	+= exynos-chipid.o
diff --git a/drivers/misc/exynos-chipid.c b/drivers/misc/exynos-chipid.c
new file mode 100644
index 0000000..eb23339
--- /dev/null
+++ b/drivers/misc/exynos-chipid.c
@@ -0,0 +1,83 @@
+/*
+ * Copyright (c) 2014-2015 Samsung Electronics Co., Ltd.
+ *	      http://www.samsung.com/
+ *
+ * EXYNOS - CHIP ID support
+ * Author: Pankaj Dubey <pankaj.dubey@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/of_address.h>
+#include <linux/exynos-soc.h>
+
+#define EXYNOS4_SOC_MASK	0xFFFE0
+#define EXYNOS5_SOC_MASK	0xFFFFF
+
+#define PROD_ID_SHIFT	(12)
+
+static void __iomem	*exynos_chipid_base;
+unsigned int exynos_soc_id = EXYNOS_SOC_UNKNOWN;
+unsigned int exynos_soc_rev;
+
+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,
+	},
+	{},
+};
+
+/**
+ * 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_soc_rev = pro_id & 0xFF;
+		pr_info("Exynos: CPUID[0x%x] CPU_REV[0x%x] Detected\n",
+				exynos_soc_id, exynos_soc_rev);
+	}
+}
diff --git a/include/linux/exynos-soc.h b/include/linux/exynos-soc.h
new file mode 100644
index 0000000..cb3ae06
--- /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,
+	EXYNOS4212 = 0xE4412,
+	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_rev {
+	EXYNOS4210_REV_0	= 0x0,
+	EXYNOS4210_REV_1_0	= 0x10,
+	EXYNOS4210_REV_1_1	= 0x11,
+};
+
+/* Since we need chipid to be initialized as early as possible
+ * during secondary core bootup adding early initialization function
+ * Note: This should be called only after device tree gets unflatten
+ */
+extern void early_exynos_chipid_init(void);
+
+extern unsigned int exynos_soc_id;
+extern unsigned int exynos_soc_rev;
+
+#endif /* __EXYNOS_SOC_H */
-- 
1.7.10.4

Re: [PATCH 0/4] Introducing Exynos ChipId driver

[Arnd Bergmann]

On Saturday 03 May 2014 15:11:36 Pankaj Dubey wrote:
> This patch series attempts to get rid of soc_is_exynosXXXX macros
> and eventually with the help of this series we can probably get
> rid of CONFIG_SOC_EXYNOSXXXX in near future.
> Each Exynos SoC has ChipID block which can give information about
> SoC's product Id and revision number. Currently we have single
> DT binding information for this as "samsung,exynos4210-chipid".
> But Exynos4 and Exynos5 SoC series have one small difference in
> chip Id, with resepect to product id bit-masks. So it means we
> should have separate compatible string for these different series
> of SoCs. So I have created new binding information for handling
> this difference. Also currently I can think of putting this driver
> code under "drivers/misc/" but suggestions are welcome.
> Also current form of driver is missing platfrom driver and needs
> init function to be called from machine file (either exynos.c or
> platsmp.c). I hope lot of suggestions and comments to improve this
> further.
> 
> This patch series is based on Kukjin Kim's for-next (3.14_rc1 tag)
> and prepared on top of following patch series and it's dependent
> patch series.

I think putting it into drivers/soc would be most appropriate.
We already have a few drivers lined up that we want in there,
although the directory currently doesn't exist.

However, I would ask that you use the infrastructure provided by
drivers/base/soc.c when you add this driver, to also make the
information available to user space using a standard API.

Ideally this should be done by slightly restructuring the DT
source to make all on-chip devices appear below the soc node.
We'd have to think a bit about how to best do this while
preserving compatibility with existing dts files.

Regarding patch 4, this is not what I meant when I asked for
removing the soc_is_exynos* macros. You basically do a 1:1 replacement
using a different interface, but you still have code that does
things differently based on a global identification.
The only user left in device drivers is now the cpufreq driver,
which is going to be replaced anyway, so that is ok. Having
a global variable that is accessible to random device drivers
is probably not a good idea though, it will just lead to
bad coding in drivers again.

To give an example of how I think it should really be restructured,
let's look at one function:

static const struct exynos_wkup_irq exynos4_wkup_irq[] = {
        { 76, BIT(1) }, /* RTC alarm */
        { 77, BIT(2) }, /* RTC tick */
        { /* sentinel */ },
};

static const struct exynos_wkup_irq exynos5250_wkup_irq[] = {
        { 75, BIT(1) }, /* RTC alarm */
        { 76, BIT(2) }, /* RTC tick */
        { /* sentinel */ },
};

static int exynos_irq_set_wake(struct irq_data *data, unsigned int state)
{
        const struct exynos_wkup_irq *wkup_irq;

        if (soc_is_exynos5250())
                wkup_irq = exynos5250_wkup_irq;
        else
                wkup_irq = exynos4_wkup_irq;

	...
}

There are multiple problems with this code:

- As mentioned, you depend on a specific SoC identification for
  something that could be done completely generic.
- The knowledge about what is a wakeup source or not doesn't
  really belong here. We don't have a DT binding for wakeups
  as far as I'm aware of, but this should probably be handled
  locally in the RTC device node, possibly in the node that
  contains the S5P_WAKEUP_MASK register.

	Arnd

Re: [PATCH 3/4] misc: exynos-chipid: Add Exynos Chipid driver support

[Krzysztof Kozlowski]

On sob, 2014-05-03 at 15:11 +0900, Pankaj Dubey wrote:
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index 7eb4b69..48c8fb5 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -55,3 +55,4 @@ obj-$(CONFIG_SRAM)		+= sram.o
>  obj-y				+= mic/
>  obj-$(CONFIG_GENWQE)		+= genwqe/
>  obj-$(CONFIG_ECHO)		+= echo/
> +obj-$(CONFIG_EXYNOS_CHIPID)	+= exynos-chipid.o
> diff --git a/drivers/misc/exynos-chipid.c b/drivers/misc/exynos-chipid.c
> new file mode 100644
> index 0000000..eb23339
> --- /dev/null
> +++ b/drivers/misc/exynos-chipid.c
> @@ -0,0 +1,83 @@
> +/*
> + * Copyright (c) 2014-2015 Samsung Electronics Co., Ltd.
> + *	      http://www.samsung.com/

I wonder why this is copyrighted in the future (2015)?

Best regards,
Krzysztof

Re: [PATCH 0/4] Introducing Exynos ChipId driver

[Pankaj Dubey]

Hi Arnd,

Thanks for review and suggestions.

On 05/04/2014 12:02 AM, Arnd Bergmann wrote:
> On Saturday 03 May 2014 15:11:36 Pankaj Dubey wrote:
>> This patch series attempts to get rid of soc_is_exynosXXXX macros
>> and eventually with the help of this series we can probably get
>> rid of CONFIG_SOC_EXYNOSXXXX in near future.
>> Each Exynos SoC has ChipID block which can give information about
>> SoC's product Id and revision number. Currently we have single
>> DT binding information for this as "samsung,exynos4210-chipid".
>> But Exynos4 and Exynos5 SoC series have one small difference in
>> chip Id, with resepect to product id bit-masks. So it means we
>> should have separate compatible string for these different series
>> of SoCs. So I have created new binding information for handling
>> this difference. Also currently I can think of putting this driver
>> code under "drivers/misc/" but suggestions are welcome.
>> Also current form of driver is missing platfrom driver and needs
>> init function to be called from machine file (either exynos.c or
>> platsmp.c). I hope lot of suggestions and comments to improve this
>> further.
>>
>> This patch series is based on Kukjin Kim's for-next (3.14_rc1 tag)
>> and prepared on top of following patch series and it's dependent
>> patch series.
> I think putting it into drivers/soc would be most appropriate.
> We already have a few drivers lined up that we want in there,
> although the directory currently doesn't exist.
OK. Will move to "drivers/soc". I can see already some patches
have been posted [1] by Andy Gross for creating directory
"drivers/soc" I will use those patches for adding samsung folder
under "drivers/soc".

[1]: https://lkml.org/lkml/2014/4/21/46

>
> However, I would ask that you use the infrastructure provided by
> drivers/base/soc.c when you add this driver, to also make the
> information available to user space using a standard API.

OK. Will update in next version.

>
> Ideally this should be done by slightly restructuring the DT
> source to make all on-chip devices appear below the soc node.

Currently I can't see soc nodes in exynos4 and exynos5 DT files.
So isn't it should be a separate patch first to modify all exynos4
exynos5 DT files to move all devices under soc node?
In that case existing chipid node will be also moved under soc node.

> We'd have to think a bit about how to best do this while
> preserving compatibility with existing dts files.

Is it necessary in this case?
As I have mentioned there is difference in bit-mask among exynos4
and exynos5's chipid. So is this reason not sufficient to keep separate
compatible for both?
Also even if we get some way to preserve existing compatibility, I afraid
in chipid driver that implementation will not look good, at least I am not
able to think of any good way. Any suggestions?

>
> Regarding patch 4, this is not what I meant when I asked for
> removing the soc_is_exynos* macros. You basically do a 1:1 replacement
> using a different interface, but you still have code that does
> things differently based on a global identification.
I agree with what you are trying to say. But if you see recently we had some
patches (cpu_idle.c: [2], pmu.c: [3])  to remove usage of such macros from
exynos machine files. So only leftover files using these macros are exynos.c
platsmp.c and pm.c.

For exynos.c I have tried to remove soc_is_exynos4/exynos5 by matching with
compatible string in patch 1 of this series. Please let me know if that is OK?

Also for platsmp.c and pm.c I can think of following approaches
1: Keep these macros till we get generic solution?
2: Allow chipid driver to expose APIs to check SoC id and SoC revisions 
till we get
generic solution. So that at least we can remove #ifdef  based macros
as soc_is_exynosXYZ.
3: Use of "of_flat_dt_is_compatible" or similar APIs in these machine files 
till we get
generic solution. For some cases where we want to know SoC revision let us
map chipid register and get revision there itself.

Please let me know what approach you think will be good?

[2]: http://thread.gmane.org/gmane.linux.kernel.samsung-soc/29085
[3]: https://lkml.org/lkml/2014/5/2/612
> The only user left in device drivers is now the cpufreq driver,
> which is going to be replaced anyway, so that is ok. Having
> a global variable that is accessible to random device drivers
> is probably not a good idea though, it will just lead to
> bad coding in drivers again.
>
> To give an example of how I think it should really be restructured,
> let's look at one function:
>
> static const struct exynos_wkup_irq exynos4_wkup_irq[] = {
>          { 76, BIT(1) }, /* RTC alarm */
>          { 77, BIT(2) }, /* RTC tick */
>          { /* sentinel */ },
> };
>
> static const struct exynos_wkup_irq exynos5250_wkup_irq[] = {
>          { 75, BIT(1) }, /* RTC alarm */
>          { 76, BIT(2) }, /* RTC tick */
>          { /* sentinel */ },
> };
>
> static int exynos_irq_set_wake(struct irq_data *data, unsigned int state)
> {
>          const struct exynos_wkup_irq *wkup_irq;
>
>          if (soc_is_exynos5250())
>                  wkup_irq = exynos5250_wkup_irq;
>          else
>                  wkup_irq = exynos4_wkup_irq;
>
> 	...
> }
>
> There are multiple problems with this code:
>
> - As mentioned, you depend on a specific SoC identification for
>    something that could be done completely generic.
> - The knowledge about what is a wakeup source or not doesn't
>    really belong here. We don't have a DT binding for wakeups
>    as far as I'm aware of, but this should probably be handled
>    locally in the RTC device node, possibly in the node that
>    contains the S5P_WAKEUP_MASK register.
>
> 	Arnd
>


-- 
Best Regards,
Pankaj Dubey

Re: [PATCH 3/4] misc: exynos-chipid: Add Exynos Chipid driver support

[Pankaj Dubey]

On 05/05/2014 04:57 PM, Krzysztof Kozlowski wrote:
> On sob, 2014-05-03 at 15:11 +0900, Pankaj Dubey wrote:
>> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
>> index 7eb4b69..48c8fb5 100644
>> --- a/drivers/misc/Makefile
>> +++ b/drivers/misc/Makefile
>> @@ -55,3 +55,4 @@ obj-$(CONFIG_SRAM)		+= sram.o
>>   obj-y				+= mic/
>>   obj-$(CONFIG_GENWQE)		+= genwqe/
>>   obj-$(CONFIG_ECHO)		+= echo/
>> +obj-$(CONFIG_EXYNOS_CHIPID)	+= exynos-chipid.o
>> diff --git a/drivers/misc/exynos-chipid.c b/drivers/misc/exynos-chipid.c
>> new file mode 100644
>> index 0000000..eb23339
>> --- /dev/null
>> +++ b/drivers/misc/exynos-chipid.c
>> @@ -0,0 +1,83 @@
>> +/*
>> + * Copyright (c) 2014-2015 Samsung Electronics Co., Ltd.
>> + *	      http://www.samsung.com/
> I wonder why this is copyrighted in the future (2015)?

Thanks for pointing out, will correct it in next version.

>
> Best regards,
> Krzysztof
>
>
>


-- 
Best Regards,
Pankaj Dubey

Re: [PATCH 0/4] Introducing Exynos ChipId driver

[Arnd Bergmann]

On Monday 05 May 2014 18:23:55 Pankaj Dubey wrote:
> On 05/04/2014 12:02 AM, Arnd Bergmann wrote:
> > Ideally this should be done by slightly restructuring the DT
> > source to make all on-chip devices appear below the soc node.
> 
> Currently I can't see soc nodes in exynos4 and exynos5 DT files.
> So isn't it should be a separate patch first to modify all exynos4
> exynos5 DT files to move all devices under soc node?
> In that case existing chipid node will be also moved under soc node.

Yes, that would be good. In fact the soc node could be identical
to the chipid node, effectively moving everything under chipid.

> > We'd have to think a bit about how to best do this while
> > preserving compatibility with existing dts files.
> 
> Is it necessary in this case?
> As I have mentioned there is difference in bit-mask among exynos4
> and exynos5's chipid. So is this reason not sufficient to keep separate
> compatible for both?

Having two "compatible" values for exynos4 and exynos5 is not a problem,
and it absolutely makes sense to have more specific values in there
as well:

compatible = "samsung,exynos4210-chipid", "samsung,exynos4-chipid";

> Also even if we get some way to preserve existing compatibility, I afraid
> in chipid driver that implementation will not look good, at least I am not
> able to think of any good way. Any suggestions?

The compatibility I mean is to ensure everything keeps working if
the node is not present.

> > Regarding patch 4, this is not what I meant when I asked for
> > removing the soc_is_exynos* macros. You basically do a 1:1 replacement
> > using a different interface, but you still have code that does
> > things differently based on a global identification.
> I agree with what you are trying to say. But if you see recently we had some
> patches (cpu_idle.c: [2], pmu.c: [3])  to remove usage of such macros from
> exynos machine files. So only leftover files using these macros are exynos.c
> platsmp.c and pm.c.
> 
> For exynos.c I have tried to remove soc_is_exynos4/exynos5 by matching with
> compatible string in patch 1 of this series. Please let me know if that is OK?

I've taken a closer look at that file now. My preferred solution
would be to go back to having two machine descriptors as it
was before Sachin Kamat's "ARM: EXYNOS: Consolidate exynos4 and
exynos5 machine files", but keep it all in one file and consolidated
as much as possible, e.g.

static void __init exynos_dt_machine_init(void)
{
       exynos_cpuidle_init();
       exynos_cpufreq_init();

       of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
}

static void __init exynos5_dt_machine_init(void)
{
       /*
        * Exynos5's legacy i2c controller and new high speed i2c
        * controller have muxed interrupt sources. By default the
        * interrupts for 4-channel HS-I2C controller are enabled.
        * If node for first four channels of legacy i2c controller
        * are available then re-configure the interrupts via the
        * system register.
        */
       struct device_node *i2c_np;
       const char *i2c_compat = "samsung,s3c2440-i2c";
       unsigned int tmp;
       int id;

       for_each_compatible_node(i2c_np, NULL, i2c_compat) {
              if (of_device_is_available(i2c_np)) {
                        id = of_alias_get_id(i2c_np, "i2c");
                        if (id < 4) {
                                  tmp = readl(EXYNOS5_SYS_I2C_CFG);
                                  writel(tmp & ~(0x1 << id),  EXYNOS5_SYS_I2C_CFG);
                        }
               }
       }

       exynos_dt_machine_init();
}

This way you can avoid having another check of the compatible node.
In the long run, all of the this code should go away: The cpuidle
and cpufreq drivers should become normal platform drivers that
get probed when the devices are present (just like it's required
for arm64 anyway), and the EXYNOS5_SYS_I2C_CFG register should
get set up by an appropriate driver, e.g. the i2c driver through
syscon, or a pinmux driver that changes the mux between the
sources based on DT information, whatever fits best.

Similarly for exynos_map_io(), with the sysram out of the picture,
it can be 

void __init exynos4_init_io(void)
{
        debug_ll_io_init();
	iotable_init(exynos4_iodesc, ARRAY_SIZE(exynos4_iodesc));
}

void __init exynos5_init_io(void)
{
        debug_ll_io_init();
	iotable_init(exynos5_iodesc, ARRAY_SIZE(exynos4_iodesc));
}

but in the long run, it would be nicer to completely eliminate
exynos4_iodesc and exynos5_iodesc as well, and remove the init_io
functions. Note that debug_ll_io_init() is already called when
you have a NULL .map_io callback.

> Also for platsmp.c and pm.c I can think of following approaches
> 1: Keep these macros till we get generic solution?
> 2: Allow chipid driver to expose APIs to check SoC id and SoC revisions 
> till we get
> generic solution. So that at least we can remove #ifdef  based macros
> as soc_is_exynosXYZ.
> 3: Use of "of_flat_dt_is_compatible" or similar APIs in these machine files 
> till we get
> generic solution. For some cases where we want to know SoC revision let us
> map chipid register and get revision there itself.
> 
> Please let me know what approach you think will be good?

I think 1 or 2 would be better than 3. Between those two, I'm undecided,
but I think either way the SoC specific values would be better kept in the
mach-samsung directory than in plat/cpu.h or linux/exynos-chipid.h.

	Arnd

Re: [PATCH 0/4] Introducing Exynos ChipId driver

[Arnd Bergmann]

On Monday 05 May 2014 16:58:14 Arnd Bergmann wrote:
> > Also for platsmp.c and pm.c I can think of following approaches
> > 1: Keep these macros till we get generic solution?
> > 2: Allow chipid driver to expose APIs to check SoC id and SoC revisions 
> > till we get
> > generic solution. So that at least we can remove #ifdef  based macros
> > as soc_is_exynosXYZ.
> > 3: Use of "of_flat_dt_is_compatible" or similar APIs in these machine files 
> > till we get
> > generic solution. For some cases where we want to know SoC revision let us
> > map chipid register and get revision there itself.
> > 
> > Please let me know what approach you think will be good?
> 
> I think 1 or 2 would be better than 3. Between those two, I'm undecided,
> but I think either way the SoC specific values would be better kept in the
> mach-samsung directory than in plat/cpu.h or linux/exynos-chipid.h.

Actually, a good compromise for now would be to add the chipid driver
to mach-exynos instead of drivers/bus. This way you can keep the uses
of the ID local to the exynos platform code until it's no longer needed.
Then it can get moved out to drivers/soc to be shared with arm64.

	Arnd

Re: [PATCH 0/4] Introducing Exynos ChipId driver

[Rob Herring]

On Sat, May 3, 2014 at 10:02 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Saturday 03 May 2014 15:11:36 Pankaj Dubey wrote:
>> This patch series attempts to get rid of soc_is_exynosXXXX macros
>> and eventually with the help of this series we can probably get
>> rid of CONFIG_SOC_EXYNOSXXXX in near future.
>> Each Exynos SoC has ChipID block which can give information about
>> SoC's product Id and revision number. Currently we have single
>> DT binding information for this as "samsung,exynos4210-chipid".
>> But Exynos4 and Exynos5 SoC series have one small difference in
>> chip Id, with resepect to product id bit-masks. So it means we
>> should have separate compatible string for these different series
>> of SoCs. So I have created new binding information for handling
>> this difference. Also currently I can think of putting this driver
>> code under "drivers/misc/" but suggestions are welcome.
>> Also current form of driver is missing platfrom driver and needs
>> init function to be called from machine file (either exynos.c or
>> platsmp.c). I hope lot of suggestions and comments to improve this
>> further.
>>
>> This patch series is based on Kukjin Kim's for-next (3.14_rc1 tag)
>> and prepared on top of following patch series and it's dependent
>> patch series.
>
> I think putting it into drivers/soc would be most appropriate.
> We already have a few drivers lined up that we want in there,
> although the directory currently doesn't exist.
>
> However, I would ask that you use the infrastructure provided by
> drivers/base/soc.c when you add this driver, to also make the
> information available to user space using a standard API.

Agreed.

> Ideally this should be done by slightly restructuring the DT
> source to make all on-chip devices appear below the soc node.
> We'd have to think a bit about how to best do this while
> preserving compatibility with existing dts files.

I don't agree. How is a block with chip ID info the parent of all the
other devices?

In doing some work to move default of_platform_populate out of
platforms, I noticed that most platforms using the soc device are
making it the parent of platform devices. I think this is either wrong
or all platforms should have a default soc device. It makes little
sense for some platforms to have a devices under a soc sysfs directory
while others do not. Or the location changes when a platform latter
adds the soc device.

Rob

Re: [PATCH 0/4] Introducing Exynos ChipId driver

[Pankaj Dubey]

On 05/05/2014 11:58 PM, Arnd Bergmann wrote:
> On Monday 05 May 2014 18:23:55 Pankaj Dubey wrote:
>> On 05/04/2014 12:02 AM, Arnd Bergmann wrote:
>>> Ideally this should be done by slightly restructuring the DT
>>> source to make all on-chip devices appear below the soc node.
>> Currently I can't see soc nodes in exynos4 and exynos5 DT files.
>> So isn't it should be a separate patch first to modify all exynos4
>> exynos5 DT files to move all devices under soc node?
>> In that case existing chipid node will be also moved under soc node.
> Yes, that would be good. In fact the soc node could be identical
> to the chipid node, effectively moving everything under chipid.

OK, in that case I would like to keep this as separate patch once
I do all other modifications.

>>> We'd have to think a bit about how to best do this while
>>> preserving compatibility with existing dts files.
>> Is it necessary in this case?
>> As I have mentioned there is difference in bit-mask among exynos4
>> and exynos5's chipid. So is this reason not sufficient to keep separate
>> compatible for both?
> Having two "compatible" values for exynos4 and exynos5 is not a problem,
> and it absolutely makes sense to have more specific values in there
> as well:
>
> compatible = "samsung,exynos4210-chipid", "samsung,exynos4-chipid";

OK, will keep compatible as you suggested.

>
>> Also even if we get some way to preserve existing compatibility, I afraid
>> in chipid driver that implementation will not look good, at least I am not
>> able to think of any good way. Any suggestions?
> The compatibility I mean is to ensure everything keeps working if
> the node is not present.
>
>>> Regarding patch 4, this is not what I meant when I asked for
>>> removing the soc_is_exynos* macros. You basically do a 1:1 replacement
>>> using a different interface, but you still have code that does
>>> things differently based on a global identification.
>> I agree with what you are trying to say. But if you see recently we had some
>> patches (cpu_idle.c: [2], pmu.c: [3])  to remove usage of such macros from
>> exynos machine files. So only leftover files using these macros are exynos.c
>> platsmp.c and pm.c.
>>
>> For exynos.c I have tried to remove soc_is_exynos4/exynos5 by matching with
>> compatible string in patch 1 of this series. Please let me know if that is OK?
> I've taken a closer look at that file now. My preferred solution
> would be to go back to having two machine descriptors as it
> was before Sachin Kamat's "ARM: EXYNOS: Consolidate exynos4 and
> exynos5 machine files", but keep it all in one file and consolidated
> as much as possible, e.g.

Yes, that case I do not need to add another function to compare compatible 
strings.
So if there is no issues in having two separate machine descriptor I will 
do this
modification in next version of patch.

>
> static void __init exynos_dt_machine_init(void)
> {
>         exynos_cpuidle_init();
>         exynos_cpufreq_init();
>
>         of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
> }
>
> static void __init exynos5_dt_machine_init(void)
> {
>         /*
>          * Exynos5's legacy i2c controller and new high speed i2c
>          * controller have muxed interrupt sources. By default the
>          * interrupts for 4-channel HS-I2C controller are enabled.
>          * If node for first four channels of legacy i2c controller
>          * are available then re-configure the interrupts via the
>          * system register.
>          */
>         struct device_node *i2c_np;
>         const char *i2c_compat = "samsung,s3c2440-i2c";
>         unsigned int tmp;
>         int id;
>
>         for_each_compatible_node(i2c_np, NULL, i2c_compat) {
>                if (of_device_is_available(i2c_np)) {
>                          id = of_alias_get_id(i2c_np, "i2c");
>                          if (id < 4) {
>                                    tmp = readl(EXYNOS5_SYS_I2C_CFG);
>                                    writel(tmp & ~(0x1 << id),  EXYNOS5_SYS_I2C_CFG);
>                          }
>                 }
>         }
>
>         exynos_dt_machine_init();
> }
>
> This way you can avoid having another check of the compatible node.
> In the long run, all of the this code should go away: The cpuidle
> and cpufreq drivers should become normal platform drivers that
> get probed when the devices are present (just like it's required
> for arm64 anyway), and the EXYNOS5_SYS_I2C_CFG register should
> get set up by an appropriate driver, e.g. the i2c driver through
> syscon, or a pinmux driver that changes the mux between the
> sources based on DT information, whatever fits best.

OK, will move this in i2c driver and will use sysreg as syscon phandle.

>
> Similarly for exynos_map_io(), with the sysram out of the picture,
> it can be
>
> void __init exynos4_init_io(void)
> {
>          debug_ll_io_init();
> 	iotable_init(exynos4_iodesc, ARRAY_SIZE(exynos4_iodesc));
> }
>
> void __init exynos5_init_io(void)
> {
>          debug_ll_io_init();
> 	iotable_init(exynos5_iodesc, ARRAY_SIZE(exynos4_iodesc));
> }
>
> but in the long run, it would be nicer to completely eliminate
> exynos4_iodesc and exynos5_iodesc as well, and remove the init_io
> functions. Note that debug_ll_io_init() is already called when
> you have a NULL .map_io callback.

Agreed.

>
>> Also for platsmp.c and pm.c I can think of following approaches
>> 1: Keep these macros till we get generic solution?
>> 2: Allow chipid driver to expose APIs to check SoC id and SoC revisions
>> till we get
>> generic solution. So that at least we can remove #ifdef  based macros
>> as soc_is_exynosXYZ.
>> 3: Use of "of_flat_dt_is_compatible" or similar APIs in these machine files
>> till we get
>> generic solution. For some cases where we want to know SoC revision let us
>> map chipid register and get revision there itself.
>>
>> Please let me know what approach you think will be good?
> I think 1 or 2 would be better than 3. Between those two, I'm undecided,
> but I think either way the SoC specific values would be better kept in the
> mach-samsung directory than in plat/cpu.h or linux/exynos-chipid.h.

OK, let me introduce this driver via "drivers/soc" in second revision,
there also if we think it's not proper to expose such APIs or variable
outside of the driver, I will be think to move it in under machine 
directory itself.

> 	Arnd
>


-- 
Best Regards,
Pankaj Dubey

Re: [PATCH 0/4] Introducing Exynos ChipId driver

[Arnd Bergmann]

On Monday 05 May 2014 10:34:02 Rob Herring wrote:
> 
> > Ideally this should be done by slightly restructuring the DT
> > source to make all on-chip devices appear below the soc node.
> > We'd have to think a bit about how to best do this while
> > preserving compatibility with existing dts files.
> 
> I don't agree. How is a block with chip ID info the parent of all the
> other devices?
> 
> In doing some work to move default of_platform_populate out of
> platforms, I noticed that most platforms using the soc device are
> making it the parent of platform devices. I think this is either wrong
> or all platforms should have a default soc device. It makes little
> sense for some platforms to have a devices under a soc sysfs directory
> while others do not. Or the location changes when a platform latter
> adds the soc device.

We had a long discussion about this when we introduced the drivers/soc
framework. The intention is that the /sys/devices/soc* node is meant to
describe the SoC in its entirety, the same way you have a pci0000:00 node
as the root of all PCI devices of the first pci host bridge on a PC
system. This also reflects the reality of a SoC, which normally has one
bus that the CPU is connected to and that has all the other devices as
children.

Having the chipid registers as part of the top-level bus should not
be interpreted as having the other devices as children of the chipid
device, but rather the chipid registers as a property of the soc itself
as opposed to a random device that happens to be part of the soc.

	Arnd

Re: [PATCH 0/4] Introducing Exynos ChipId driver

[Arnd Bergmann]

On Tuesday 06 May 2014 15:57:24 Pankaj Dubey wrote:
> On 05/05/2014 11:58 PM, Arnd Bergmann wrote:
> > On Monday 05 May 2014 18:23:55 Pankaj Dubey wrote:
> >> On 05/04/2014 12:02 AM, Arnd Bergmann wrote:
> >>> Ideally this should be done by slightly restructuring the DT
> >>> source to make all on-chip devices appear below the soc node.
> >> Currently I can't see soc nodes in exynos4 and exynos5 DT files.
> >> So isn't it should be a separate patch first to modify all exynos4
> >> exynos5 DT files to move all devices under soc node?
> >> In that case existing chipid node will be also moved under soc node.
> > Yes, that would be good. In fact the soc node could be identical
> > to the chipid node, effectively moving everything under chipid.
> 
> OK, in that case I would like to keep this as separate patch once
> I do all other modifications.

Yes, makes sense. Let's see if we can convince Rob first though, he
has some reservations.

> >> Also even if we get some way to preserve existing compatibility, I afraid
> >> in chipid driver that implementation will not look good, at least I am not
> >> able to think of any good way. Any suggestions?
> > The compatibility I mean is to ensure everything keeps working if
> > the node is not present.
> >
> >>> Regarding patch 4, this is not what I meant when I asked for
> >>> removing the soc_is_exynos* macros. You basically do a 1:1 replacement
> >>> using a different interface, but you still have code that does
> >>> things differently based on a global identification.
> >> I agree with what you are trying to say. But if you see recently we had some
> >> patches (cpu_idle.c: [2], pmu.c: [3])  to remove usage of such macros from
> >> exynos machine files. So only leftover files using these macros are exynos.c
> >> platsmp.c and pm.c.
> >>
> >> For exynos.c I have tried to remove soc_is_exynos4/exynos5 by matching with
> >> compatible string in patch 1 of this series. Please let me know if that is OK?
> > I've taken a closer look at that file now. My preferred solution
> > would be to go back to having two machine descriptors as it
> > was before Sachin Kamat's "ARM: EXYNOS: Consolidate exynos4 and
> > exynos5 machine files", but keep it all in one file and consolidated
> > as much as possible, e.g.
> 
> Yes, that case I do not need to add another function to compare compatible 
> strings.
> So if there is no issues in having two separate machine descriptor I will 
> do this
> modification in next version of patch.

ok.

> > static void __init exynos_dt_machine_init(void)
> > {
> >         exynos_cpuidle_init();
> >         exynos_cpufreq_init();
> >
> >         of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
> > }
> >
> > static void __init exynos5_dt_machine_init(void)
> > {
> >         /*
> >          * Exynos5's legacy i2c controller and new high speed i2c
> >          * controller have muxed interrupt sources. By default the
> >          * interrupts for 4-channel HS-I2C controller are enabled.
> >          * If node for first four channels of legacy i2c controller
> >          * are available then re-configure the interrupts via the
> >          * system register.
> >          */
> >         struct device_node *i2c_np;
> >         const char *i2c_compat = "samsung,s3c2440-i2c";
> >         unsigned int tmp;
> >         int id;
> >
> >         for_each_compatible_node(i2c_np, NULL, i2c_compat) {
> >                if (of_device_is_available(i2c_np)) {
> >                          id = of_alias_get_id(i2c_np, "i2c");
> >                          if (id < 4) {
> >                                    tmp = readl(EXYNOS5_SYS_I2C_CFG);
> >                                    writel(tmp & ~(0x1 << id),  EXYNOS5_SYS_I2C_CFG);
> >                          }
> >                 }
> >         }
> >
> >         exynos_dt_machine_init();
> > }
> >
> > This way you can avoid having another check of the compatible node.
> > In the long run, all of the this code should go away: The cpuidle
> > and cpufreq drivers should become normal platform drivers that
> > get probed when the devices are present (just like it's required
> > for arm64 anyway), and the EXYNOS5_SYS_I2C_CFG register should
> > get set up by an appropriate driver, e.g. the i2c driver through
> > syscon, or a pinmux driver that changes the mux between the
> > sources based on DT information, whatever fits best.
> 
> OK, will move this in i2c driver and will use sysreg as syscon phandle.

Ok, cool.

> >> Also for platsmp.c and pm.c I can think of following approaches
> >> 1: Keep these macros till we get generic solution?
> >> 2: Allow chipid driver to expose APIs to check SoC id and SoC revisions
> >> till we get
> >> generic solution. So that at least we can remove #ifdef  based macros
> >> as soc_is_exynosXYZ.
> >> 3: Use of "of_flat_dt_is_compatible" or similar APIs in these machine files
> >> till we get
> >> generic solution. For some cases where we want to know SoC revision let us
> >> map chipid register and get revision there itself.
> >>
> >> Please let me know what approach you think will be good?
> > I think 1 or 2 would be better than 3. Between those two, I'm undecided,
> > but I think either way the SoC specific values would be better kept in the
> > mach-samsung directory than in plat/cpu.h or linux/exynos-chipid.h.
> 
> OK, let me introduce this driver via "drivers/soc" in second revision,
> there also if we think it's not proper to expose such APIs or variable
> outside of the driver, I will be think to move it in under machine 
> directory itself.

Ok.

	Arnd

Re: [PATCH 0/4] Introducing Exynos ChipId driver

[Olof Johansson]

(Taking the discussion here since Panjak pointed me to this thread when
I commented on the latest patch set)

On Mon, May 05, 2014 at 04:58:14PM +0200, Arnd Bergmann wrote:
> On Monday 05 May 2014 18:23:55 Pankaj Dubey wrote:
> > On 05/04/2014 12:02 AM, Arnd Bergmann wrote:
> > > Ideally this should be done by slightly restructuring the DT
> > > source to make all on-chip devices appear below the soc node.
> > 
> > Currently I can't see soc nodes in exynos4 and exynos5 DT files.
> > So isn't it should be a separate patch first to modify all exynos4
> > exynos5 DT files to move all devices under soc node?
> > In that case existing chipid node will be also moved under soc node.
> 
> Yes, that would be good. In fact the soc node could be identical
> to the chipid node, effectively moving everything under chipid.
> 
> > > We'd have to think a bit about how to best do this while
> > > preserving compatibility with existing dts files.
> > 
> > Is it necessary in this case?
> > As I have mentioned there is difference in bit-mask among exynos4
> > and exynos5's chipid. So is this reason not sufficient to keep separate
> > compatible for both?
> 
> Having two "compatible" values for exynos4 and exynos5 is not a problem,
> and it absolutely makes sense to have more specific values in there
> as well:
> 
> compatible = "samsung,exynos4210-chipid", "samsung,exynos4-chipid";

Actually, "generic" compatibles for a device isn't the right thing to do
here. For machine-level compat it makes sense, but not for devices. There
it should instead use the first version of the IP block.

> > Also even if we get some way to preserve existing compatibility, I afraid
> > in chipid driver that implementation will not look good, at least I am not
> > able to think of any good way. Any suggestions?
> 
> The compatibility I mean is to ensure everything keeps working if
> the node is not present.
> 
> > > Regarding patch 4, this is not what I meant when I asked for
> > > removing the soc_is_exynos* macros. You basically do a 1:1 replacement
> > > using a different interface, but you still have code that does
> > > things differently based on a global identification.
> > I agree with what you are trying to say. But if you see recently we had some
> > patches (cpu_idle.c: [2], pmu.c: [3])  to remove usage of such macros from
> > exynos machine files. So only leftover files using these macros are exynos.c
> > platsmp.c and pm.c.
> > 
> > For exynos.c I have tried to remove soc_is_exynos4/exynos5 by matching with
> > compatible string in patch 1 of this series. Please let me know if that is OK?
> 
> I've taken a closer look at that file now. My preferred solution
> would be to go back to having two machine descriptors as it
> was before Sachin Kamat's "ARM: EXYNOS: Consolidate exynos4 and
> exynos5 machine files", but keep it all in one file and consolidated
> as much as possible, e.g.
> 
> static void __init exynos_dt_machine_init(void)
> {
>        exynos_cpuidle_init();
>        exynos_cpufreq_init();
> 
>        of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
> }
> 
> static void __init exynos5_dt_machine_init(void)
> {
>        /*
>         * Exynos5's legacy i2c controller and new high speed i2c
>         * controller have muxed interrupt sources. By default the
>         * interrupts for 4-channel HS-I2C controller are enabled.
>         * If node for first four channels of legacy i2c controller
>         * are available then re-configure the interrupts via the
>         * system register.
>         */
>        struct device_node *i2c_np;
>        const char *i2c_compat = "samsung,s3c2440-i2c";
>        unsigned int tmp;
>        int id;
> 
>        for_each_compatible_node(i2c_np, NULL, i2c_compat) {
>               if (of_device_is_available(i2c_np)) {
>                         id = of_alias_get_id(i2c_np, "i2c");
>                         if (id < 4) {
>                                   tmp = readl(EXYNOS5_SYS_I2C_CFG);
>                                   writel(tmp & ~(0x1 << id),  EXYNOS5_SYS_I2C_CFG);
>                         }
>                }
>        }
> 
>        exynos_dt_machine_init();
> }
> 
> This way you can avoid having another check of the compatible node.
> In the long run, all of the this code should go away: The cpuidle
> and cpufreq drivers should become normal platform drivers that
> get probed when the devices are present (just like it's required
> for arm64 anyway), and the EXYNOS5_SYS_I2C_CFG register should
> get set up by an appropriate driver, e.g. the i2c driver through
> syscon, or a pinmux driver that changes the mux between the
> sources based on DT information, whatever fits best.
> 
> Similarly for exynos_map_io(), with the sysram out of the picture,
> it can be 
> 
> void __init exynos4_init_io(void)
> {
>         debug_ll_io_init();
> 	iotable_init(exynos4_iodesc, ARRAY_SIZE(exynos4_iodesc));
> }
> 
> void __init exynos5_init_io(void)
> {
>         debug_ll_io_init();
> 	iotable_init(exynos5_iodesc, ARRAY_SIZE(exynos4_iodesc));
> }
> 
> but in the long run, it would be nicer to completely eliminate
> exynos4_iodesc and exynos5_iodesc as well, and remove the init_io
> functions. Note that debug_ll_io_init() is already called when
> you have a NULL .map_io callback.
> 
> > Also for platsmp.c and pm.c I can think of following approaches
> > 1: Keep these macros till we get generic solution?
> > 2: Allow chipid driver to expose APIs to check SoC id and SoC revisions 
> > till we get
> > generic solution. So that at least we can remove #ifdef  based macros
> > as soc_is_exynosXYZ.
> > 3: Use of "of_flat_dt_is_compatible" or similar APIs in these machine files 
> > till we get
> > generic solution. For some cases where we want to know SoC revision let us
> > map chipid register and get revision there itself.
> > 
> > Please let me know what approach you think will be good?
> 
> I think 1 or 2 would be better than 3. Between those two, I'm undecided,
> but I think either way the SoC specific values would be better kept in the
> mach-samsung directory than in plat/cpu.h or linux/exynos-chipid.h.

The generic solution is already there: of_machine_is_compatible() is perfectly
sensible to use for _some_ of these inits. Cpufreq is one of the few that comes
to mind, and maybe some of the platsmp and pm stuff.

Note that none of them should be used in runtime, i.e. you should only use them
at probe/setup time and maybe have a local state in the driver if needed.

I'd rather get people used to that format than everybody needing to implement
a chipid driver now too, especially on platforms that might not even have a
suitable chipid block to base a driver around -- not to mention having to
fill the namespace with is_soc_*() stuff.


-Olof

Re: [PATCH 0/4] Introducing Exynos ChipId driver

[Arnd Bergmann]

On Sunday 11 May 2014 18:47:28 Olof Johansson wrote:
> > > Also for platsmp.c and pm.c I can think of following approaches
> > > 1: Keep these macros till we get generic solution?
> > > 2: Allow chipid driver to expose APIs to check SoC id and SoC revisions 
> > > till we get
> > > generic solution. So that at least we can remove #ifdef  based macros
> > > as soc_is_exynosXYZ.
> > > 3: Use of "of_flat_dt_is_compatible" or similar APIs in these machine files 
> > > till we get
> > > generic solution. For some cases where we want to know SoC revision let us
> > > map chipid register and get revision there itself.
> > > 
> > > Please let me know what approach you think will be good?
> > 
> > I think 1 or 2 would be better than 3. Between those two, I'm undecided,
> > but I think either way the SoC specific values would be better kept in the
> > mach-samsung directory than in plat/cpu.h or linux/exynos-chipid.h.
> 
> The generic solution is already there: of_machine_is_compatible() is perfectly
> sensible to use for _some_ of these inits. Cpufreq is one of the few that comes
> to mind, and maybe some of the platsmp and pm stuff.
> 
> Note that none of them should be used in runtime, i.e. you should only use them
> at probe/setup time and maybe have a local state in the driver if needed.
> 
> I'd rather get people used to that format than everybody needing to implement
> a chipid driver now too, especially on platforms that might not even have a
> suitable chipid block to base a driver around -- not to mention having to
> fill the namespace with is_soc_*() stuff.

I was coming from the other angle: exynos is already using soc_is_*() in too
many places. I'd like to first see the ones cleaned up that really should be
doing something else because they have a device-local ID to look at.

If we end up with a couple of instances that don't have a good alternative,
we can use of_machine_is_compatible() for those, but I'd like to avoid doing
a blind conversion because that would likely lead to more instances in the
future, not fewer.

I agree that we should have to introduce new chip ID drivers on other
platforms for the purpose of finding out the SoC version, especially not
with private APIs.

	Arnd