exynos: add basic support for exynos5260 SoC

From: Pankaj Dubey <[email protected]>

This patch add basic arch side support for exynos5260 SoC.

Signed-off-by Pankaj Dubey <[email protected]>

Signed-off-by Arun Kumar K <[email protected]>

arch/arm/mach-exynos/common.c +0 -0

        if (soc_is_exynos5250())
                iotable_init(exynos5250_iodesc, ARRAY_SIZE(exynos5250_iodesc));
 }

#define S5P_VA_SYSRAM S3C_ADDR(0x02400000)
#define S5P_VA_SYSRAM_NS S3C_ADDR(0x02410000)
+

Hi Rahul,

Quoted 6 lines · Rahul Sharma

From: Pankaj Dubey <[email protected]>

This patch add basic arch side support for exynos5260 SoC.

Signed-off-by Pankaj Dubey <[email protected]>

Signed-off-by Arun Kumar K <[email protected]>

Since you are posting the patch, your signed off is also required.

[snip]

arch/arm/mach-exynos/common.c static void __init exynos4_map_io(void) · line 391

Quoted 10 lines

void exynos4_restart(enum reboot_mode mode, const char *cmd)

 {
        __raw_writel(0x1, S5P_SWRESET);

arch/arm/mach-exynos/common.c +2 -1

@@ -371,7 +387,8 @@ static void __init exynos4_map_io(void)
 static void __init exynos5_map_io(void)
 {
        iotable_init(exynos5_iodesc, ARRAY_SIZE(exynos5_iodesc));
-
+       if (soc_is_exynos5260())
+               iotable_init(exynos5260_iodesc, ARRAY_SIZE(exynos5260_iodesc));

Add this after 5250 to maintain numerical order.

arch/arm/mach-exynos/common.c static void __init exynos4_map_io(void) · line 394

Quoted 3 lines

arch/arm/mach-exynos/common.c +0 -0

        if (soc_is_exynos5250())
                iotable_init(exynos5250_iodesc, ARRAY_SIZE(exynos5250_iodesc));
 }

[snip]

Quoted 4 lines

#define S5P_VA_SYSRAM S3C_ADDR(0x02400000)
#define S5P_VA_SYSRAM_NS S3C_ADDR(0x02410000)
+

Unnecessary blank line addition.

--
With warm regards,
Sachin

Hi Rahul,

Quoted 6 lines · Rahul Sharma

From: Pankaj Dubey <[email protected]>

This patch add basic arch side support for exynos5260 SoC.

Signed-off-by Pankaj Dubey <[email protected]>

Signed-off-by Arun Kumar K <[email protected]>

Since you are posting the patch, your signed off is also required.

[snip]

arch/arm/mach-exynos/common.c static void __init exynos4_map_io(void) · line 391

Quoted 10 lines

void exynos4_restart(enum reboot_mode mode, const char *cmd)

 {
        __raw_writel(0x1, S5P_SWRESET);

arch/arm/mach-exynos/common.c +2 -1

@@ -371,7 +387,8 @@ static void __init exynos4_map_io(void)
 static void __init exynos5_map_io(void)
 {
        iotable_init(exynos5_iodesc, ARRAY_SIZE(exynos5_iodesc));
-
+       if (soc_is_exynos5260())
+               iotable_init(exynos5260_iodesc, ARRAY_SIZE(exynos5260_iodesc));

Add this after 5250 to maintain numerical order.

Quoted 4 lines

arch/arm/mach-exynos/common.c +0 -0

        if (soc_is_exynos5250())
                iotable_init(exynos5250_iodesc, ARRAY_SIZE(exynos5250_iodesc));
 }

[snip]

Quoted 4 lines

#define S5P_VA_SYSRAM S3C_ADDR(0x02400000)
#define S5P_VA_SYSRAM_NS S3C_ADDR(0x02410000)
+

Unnecessary blank line addition.

--
With warm regards,
Sachin

From: Pankaj Dubey <[email protected]>

This patch add basic arch side support for exynos5260 SoC.

Signed-off-by Pankaj Dubey <[email protected]>

Signed-off-by Arun Kumar K <[email protected]>

---
 arch/arm/mach-exynos/Kconfig                 |    9 +++++++++
 arch/arm/mach-exynos/common.c                |   19 ++++++++++++++++++-
 arch/arm/mach-exynos/cpuidle.c               |    2 +-
 arch/arm/mach-exynos/include/mach/map.h      |    1 +
 arch/arm/mach-exynos/include/mach/regs-pmu.h |    4 ++++
 arch/arm/mach-exynos/mach-exynos5-dt.c       |    1 +
 arch/arm/plat-samsung/include/plat/cpu.h     |    8 ++++++++
 arch/arm/plat-samsung/include/plat/map-s5p.h |    1 +
 8 files changed, 43 insertions(+), 2 deletions(-)

arch/arm/mach-exynos/cpuidle.c +0 -0

 		scu_enable(S5P_VA_SCU);
 #endif
 	cpu_pm_exit();

arch/arm/mach-exynos/include/mach/map.h +1 -0

index 7b046b5..bd6fa02 100644
@@ -29,6 +29,7 @@
 #define EXYNOS4210_PA_SYSRAM_NS		0x0203F000
 #define EXYNOS4x12_PA_SYSRAM_NS		0x0204F000
 #define EXYNOS5250_PA_SYSRAM_NS		0x0204F000
+#define EXYNOS5260_PA_SYSRAM_NS		0x02073000
 
 #define EXYNOS_PA_CHIPID		0x10000000
 

arch/arm/mach-exynos/include/mach/regs-pmu.h +0 -0

index 2cdb63e..09ae29a 100644

Hi Pankaj, Rahul, Arun,

In addition to issues already pointed by Sachin, please also see my comments inline.

Quoted 16 lines · On Friday 06 of December 2013 21:26:25 Rahul Sharma wrote:

From: Pankaj Dubey <[email protected]>

This patch add basic arch side support for exynos5260 SoC.

Signed-off-by Pankaj Dubey <[email protected]>

Signed-off-by Arun Kumar K <[email protected]>

---
 arch/arm/mach-exynos/Kconfig                 |    9 +++++++++
 arch/arm/mach-exynos/common.c                |   19 ++++++++++++++++++-
 arch/arm/mach-exynos/cpuidle.c               |    2 +-
 arch/arm/mach-exynos/include/mach/map.h      |    1 +
 arch/arm/mach-exynos/include/mach/regs-pmu.h |    4 ++++
 arch/arm/mach-exynos/mach-exynos5-dt.c       |    1 +
 arch/arm/plat-samsung/include/plat/cpu.h     |    8 ++++++++
 arch/arm/plat-samsung/include/plat/map-s5p.h |    1 +
 8 files changed, 43 insertions(+), 2 deletions(-)

[snip]

arch/arm/mach-exynos/cpuidle.c static int exynos4_enter_core0_aftr(struct cpuidle_device *dev, · line 123

Quoted 10 lines

arch/arm/mach-exynos/cpuidle.c +1 -1

index ddbfe87..405c11a 100644
@@ -120,7 +120,7 @@ static int exynos4_enter_core0_aftr(struct cpuidle_device *dev,
      cpu_suspend(0, idle_finisher);
 
 #ifdef CONFIG_SMP
-     if (!soc_is_exynos5250())
+     if (!soc_is_exynos5250() || soc_is_exynos5260())

The added OR condition doesn't affect the if condition in any way, because when running on Exynos5260, soc_is_exynos5250() will return false and make the whole condition evaluate to true.

Shouldn't it be

        if (!soc_is_exynos5250() && !soc_is_exynos5260())

if Exynos5260 doesn't need scu_enable(), or left as is if it needs?

Quoted 21 lines

arch/arm/mach-exynos/cpuidle.c +0 -0

              scu_enable(S5P_VA_SCU);
 #endif
      cpu_pm_exit();

arch/arm/mach-exynos/include/mach/map.h +1 -0

index 7b046b5..bd6fa02 100644
@@ -29,6 +29,7 @@
 #define EXYNOS4210_PA_SYSRAM_NS              0x0203F000
 #define EXYNOS4x12_PA_SYSRAM_NS              0x0204F000
 #define EXYNOS5250_PA_SYSRAM_NS              0x0204F000
+#define EXYNOS5260_PA_SYSRAM_NS              0x02073000
 
 #define EXYNOS_PA_CHIPID             0x10000000
 

arch/arm/mach-exynos/include/mach/regs-pmu.h +0 -0

index 2cdb63e..09ae29a 100644
@@ -234,6 +234,10 @@
 

arch/arm/mach-exynos/include/mach/regs-pmu.h line 240

Quoted 6 lines

arch/arm/mach-exynos/include/mach/regs-pmu.h +4 -0

 #define EXYNOS5_SYS_WDTRESET                                 (1 << 20)
 
+#define EXYNOS5260_A7_WDTRST                                 (1 << 24)
+#define EXYNOS5260_A15_WDTRST                                        (1 << 23)
+#define EXYNOS5260_SYS_WDTRESET      (EXYNOS5260_A7_WDTRST || EXYNOS5260_A15_WDTRST)
+

Are these definitions needed? I don't see any user in this patch.

Best regards,
Tomasz

From: Young-Gun Jang <[email protected]>

Add Samsung Exynos5260 SoC specific data to enable pinctrl support for all platforms based on EXYNOS5260.

Signed-off-by Pankaj Dubey <[email protected]>

Signed-off-by Rahul Sharma <[email protected]>

Signed-off-by Arun Kumar K <[email protected]>

Hi Rahul, Young-Gun,

Quoted 2 lines · Rahul Sharma

From: Young-Gun Jang <[email protected]>

Quoted 6 lines

Add Samsung Exynos5260 SoC specific data to enable pinctrl support for all platforms based on EXYNOS5260.

Signed-off-by Pankaj Dubey <[email protected]>

Signed-off-by Rahul Sharma <[email protected]>

Signed-off-by Arun Kumar K <[email protected]>

Author's (Young-Gun Jang) signed-off by is missing here.

[snip]

drivers/pinctrl/pinctrl-exynos.c struct samsung_pin_ctrl exynos5250_pin_ctrl[] = { · line 1086

Quoted 9 lines

drivers/pinctrl/pinctrl-exynos.c +9 -0

+
+/* pin banks of exynos5260 pin-controller 2 */
+static struct samsung_pin_bank exynos5260_pin_banks2[] = {
+       EXYNOS_PIN_BANK_EINTG(7, 0x000, "gpz0", 0x00),
+       EXYNOS_PIN_BANK_EINTG(4, 0x020, "gpz1", 0x04),
+};
+
+/*
+ * Samsung pinctrl driver data for Exynos5260 SoC. Exynos5420 SoC includes

s/5420/5260 ?

--
With warm regards,
Sachin

Hi Rahul, Young-Gun,

Quoted 2 lines · Rahul Sharma

From: Young-Gun Jang <[email protected]>

Quoted 6 lines

Add Samsung Exynos5260 SoC specific data to enable pinctrl support for all platforms based on EXYNOS5260.

Signed-off-by Pankaj Dubey <[email protected]>

Signed-off-by Rahul Sharma <[email protected]>

Signed-off-by Arun Kumar K <[email protected]>

Author's (Young-Gun Jang) signed-off by is missing here.

[snip]

drivers/pinctrl/pinctrl-exynos.c struct samsung_pin_ctrl exynos5250_pin_ctrl[] = { · line 1086

Quoted 9 lines

drivers/pinctrl/pinctrl-exynos.c +9 -0

+
+/* pin banks of exynos5260 pin-controller 2 */
+static struct samsung_pin_bank exynos5260_pin_banks2[] = {
+       EXYNOS_PIN_BANK_EINTG(7, 0x000, "gpz0", 0x00),
+       EXYNOS_PIN_BANK_EINTG(4, 0x020, "gpz1", 0x04),
+};
+
+/*
+ * Samsung pinctrl driver data for Exynos5260 SoC. Exynos5420 SoC includes

s/5420/5260 ?

--
With warm regards,
Sachin

From: Young-Gun Jang <[email protected]>

Add Samsung Exynos5260 SoC specific data to enable pinctrl support for all platforms based on EXYNOS5260.

drivers/pinctrl/pinctrl-exynos.c +13 -0

index 155b1b3..9a93df6 100644
@@ -1042,6 +1042,88 @@ struct samsung_pin_ctrl exynos5250_pin_ctrl[] = {
 	},
 };
 
+/* pin banks of exynos5260 pin-controller 0 */
+static struct samsung_pin_bank exynos5260_pin_banks0[] = {
+	EXYNOS_PIN_BANK_EINTG(4, 0x000, "gpa0", 0x00),
+	EXYNOS_PIN_BANK_EINTG(7, 0x020, "gpa1", 0x04),
+	EXYNOS_PIN_BANK_EINTG(8, 0x040, "gpa2", 0x08),
+	EXYNOS_PIN_BANK_EINTG(5, 0x060, "gpb0", 0x0c),
+	EXYNOS_PIN_BANK_EINTG(4, 0x080, "gpb1", 0x10),
+	EXYNOS_PIN_BANK_EINTG(5, 0x0A0, "gpb2", 0x14),
+	EXYNOS_PIN_BANK_EINTG(8, 0x0C0, "gpb3", 0x18),
+	EXYNOS_PIN_BANK_EINTG(8, 0x0E0, "gpb4", 0x1c),
+	EXYNOS_PIN_BANK_EINTG(8, 0x100, "gpb5", 0x20),
+	EXYNOS_PIN_BANK_EINTG(8, 0x120, "gpd0", 0x24),
+	EXYNOS_PIN_BANK_EINTG(7, 0x140, "gpd1", 0x28),

drivers/pinctrl/pinctrl-exynos.c +10 -0

+};
+
+/* pin banks of exynos5260 pin-controller 1 */
+static struct samsung_pin_bank exynos5260_pin_banks1[] = {
+	EXYNOS_PIN_BANK_EINTG(7, 0x000, "gpc0", 0x00),
+	EXYNOS_PIN_BANK_EINTG(6, 0x020, "gpc1", 0x04),
+	EXYNOS_PIN_BANK_EINTG(7, 0x040, "gpc2", 0x08),
+	EXYNOS_PIN_BANK_EINTG(4, 0x060, "gpc3", 0x0c),
+	EXYNOS_PIN_BANK_EINTG(4, 0x080, "gpc4", 0x10),
+};

Hi Young-Gun, Pankaj, Rahul, Arun,

Please see my comments inline.

Quoted 4 lines · On Friday 06 of December 2013 21:26:26 Rahul Sharma wrote:

From: Young-Gun Jang <[email protected]>

Add Samsung Exynos5260 SoC specific data to enable pinctrl support for all platforms based on EXYNOS5260.

[snip]

Quoted 26 lines

drivers/pinctrl/pinctrl-exynos.c +18 -0

index 155b1b3..9a93df6 100644
@@ -1042,6 +1042,88 @@ struct samsung_pin_ctrl exynos5250_pin_ctrl[] = {
      },
 };
 
+/* pin banks of exynos5260 pin-controller 0 */
+static struct samsung_pin_bank exynos5260_pin_banks0[] = {
+     EXYNOS_PIN_BANK_EINTG(4, 0x000, "gpa0", 0x00),
+     EXYNOS_PIN_BANK_EINTG(7, 0x020, "gpa1", 0x04),
+     EXYNOS_PIN_BANK_EINTG(8, 0x040, "gpa2", 0x08),
+     EXYNOS_PIN_BANK_EINTG(5, 0x060, "gpb0", 0x0c),
+     EXYNOS_PIN_BANK_EINTG(4, 0x080, "gpb1", 0x10),
+     EXYNOS_PIN_BANK_EINTG(5, 0x0A0, "gpb2", 0x14),
+     EXYNOS_PIN_BANK_EINTG(8, 0x0C0, "gpb3", 0x18),
+     EXYNOS_PIN_BANK_EINTG(8, 0x0E0, "gpb4", 0x1c),
+     EXYNOS_PIN_BANK_EINTG(8, 0x100, "gpb5", 0x20),
+     EXYNOS_PIN_BANK_EINTG(8, 0x120, "gpd0", 0x24),
+     EXYNOS_PIN_BANK_EINTG(7, 0x140, "gpd1", 0x28),
+     EXYNOS_PIN_BANK_EINTG(5, 0x160, "gpd2", 0x2c),
+     EXYNOS_PIN_BANK_EINTG(8, 0x180, "gpe0", 0x30),
+     EXYNOS_PIN_BANK_EINTG(5, 0x1A0, "gpe1", 0x34),
+     EXYNOS_PIN_BANK_EINTG(4, 0x1C0, "gpf0", 0x38),
+     EXYNOS_PIN_BANK_EINTG(8, 0x1E0, "gpf1", 0x3c),

drivers/pinctrl/pinctrl-exynos.c struct samsung_pin_ctrl exynos5250_pin_ctrl[] = { · line 1067

Quoted 5 lines

drivers/pinctrl/pinctrl-exynos.c +5 -0

+     EXYNOS_PIN_BANK_EINTG(2, 0x200, "gpk0", 0x40),
+     EXYNOS_PIN_BANK_EINTW(8, 0xC00, "gpx0", 0x00),
+     EXYNOS_PIN_BANK_EINTW(8, 0xC20, "gpx1", 0x04),
+     EXYNOS_PIN_BANK_EINTW(8, 0xC40, "gpx2", 0x08),
+     EXYNOS_PIN_BANK_EINTW(8, 0xC60, "gpx3", 0x0c),

nit: Please use lowercase hexadecimal characters. Despite of already existing code, lowercase is the preferred way.

Quoted 14 lines

drivers/pinctrl/pinctrl-exynos.c +14 -0

+};
+
+/* pin banks of exynos5260 pin-controller 1 */
+static struct samsung_pin_bank exynos5260_pin_banks1[] = {
+     EXYNOS_PIN_BANK_EINTG(7, 0x000, "gpc0", 0x00),
+     EXYNOS_PIN_BANK_EINTG(6, 0x020, "gpc1", 0x04),
+     EXYNOS_PIN_BANK_EINTG(7, 0x040, "gpc2", 0x08),
+     EXYNOS_PIN_BANK_EINTG(4, 0x060, "gpc3", 0x0c),
+     EXYNOS_PIN_BANK_EINTG(4, 0x080, "gpc4", 0x10),
+};
+
+/* pin banks of exynos5260 pin-controller 2 */
+static struct samsung_pin_bank exynos5260_pin_banks2[] = {
+     EXYNOS_PIN_BANK_EINTG(7, 0x000, "gpz0", 0x00),

drivers/pinctrl/pinctrl-exynos.c struct samsung_pin_ctrl exynos5250_pin_ctrl[] = { · line 1087

Quoted 6 lines

drivers/pinctrl/pinctrl-exynos.c +6 -0

+     EXYNOS_PIN_BANK_EINTG(4, 0x020, "gpz1", 0x04),
+};
+
+/*
+ * Samsung pinctrl driver data for Exynos5260 SoC. Exynos5420 SoC includes
+ * four gpio/pin-mux/pinconfig controllers.

Hmm, I can see only three of them below. Is there one left undefined?

Otherwise, the patch looks fine.

Best regards,
Tomasz

From: Arun Kumar K <[email protected]>

The patch adds the dts files for exynos5260 and for xyref evt0 board.

(quoted diff) +1 -0

+		};

arch/arm/boot/dts/exynos5260-pinctrl.dtsi new file +3 -0

+			samsung,pin-pud = <0>;
+			samsung,pin-drv = <0>;
+		};

arch/arm/boot/dts/exynos5260-pinctrl.dtsi new file +4 -0

+			samsung,pin-pud = <1>;
+			samsung,pin-drv = <0>;
+		};
+	};

arch/arm/boot/dts/exynos5260-pinctrl.dtsi new file +10 -0

+		sd0_bus1: sd0-bus-width1 {
+			samsung,pins = "gpc0-3";
+			samsung,pin-function = <2>;
+			samsung,pin-pud = <3>;
+			samsung,pin-drv = <3>;
+		};
+
+		sd0_bus4: sd0-bus-width4 {
+			samsung,pins = "gpc0-3", "gpc0-4", "gpc0-5", "gpc0-6";
+			samsung,pin-function = <2>;

arch/arm/boot/dts/exynos5260-xyref5260-evt0.dts new file +8 -0

new file mode 100644
index 0000000..aa1fcda
@@ -0,0 +1,85 @@
+/*
+ * SAMSUNG XYREF5260 EVT0 board device tree source
+ *
+ * Copyright (c) 2013 Samsung Electronics Co., Ltd.
+ *		http://www.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

arch/arm/boot/dts/exynos5260-xyref5260-evt0.dts new file +4 -0

+
+	serial at 12C00000 {
+		status = "okay";
+	};

arch/arm/boot/dts/exynos5260-xyref5260-evt0.dts new file +8 -0

+		card-detect-delay = <200>;
+		samsung,dw-mshc-ciu-div = <3>;
+		samsung,dw-mshc-sdr-timing = <0 4>;
+		samsung,dw-mshc-ddr-timing = <0 2>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&sd0_clk &sd0_cmd &sd0_bus4 &sd0_bus8>;
+
+		slot at 0 {

arch/arm/boot/dts/exynos5260-xyref5260-evt0.dts new file +14 -0

+		card-detect-delay = <200>;
+		samsung,dw-mshc-ciu-div = <3>;
+		samsung,dw-mshc-sdr-timing = <2 3>;
+		samsung,dw-mshc-ddr-timing = <1 2>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&sd2_clk &sd2_cmd &sd2_cd &sd2_bus4>;
+
+		slot at 0 {
+			reg = <0>;
+			bus-width = <4>;
+			disable-wp;
+		};
+	};
+};

arch/arm/boot/dts/exynos5260.dtsi new file +5 -0

new file mode 100644
index 0000000..fcb8d4f
@@ -0,0 +1,315 @@
+/*
+ * SAMSUNG EXYNOS5260 SoC device tree source
+ *
+ * Copyright (c) 2013 Samsung Electronics Co., Ltd.
+ *		http://www.samsung.com

arch/arm/boot/dts/exynos5260.dtsi new file +23 -0

+
+/ {
+	compatible = "samsung,exynos5260";
+	interrupt-parent = <&gic>;
+
+	aliases {
+		pinctrl0 = &pinctrl_0;
+		pinctrl1 = &pinctrl_1;
+		pinctrl2 = &pinctrl_2;
+	};
+
+	chipid at 10000000 {
+		compatible = "samsung,exynos4210-chipid";
+		reg = <0x10000000 0x100>;
+	};
+
+	cpus {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		cpu at 0 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a15";

arch/arm/boot/dts/exynos5260.dtsi new file +5 -0

+		compatible = "arm,cortex-a15-gic", "arm,cortex-a9-gic";
+		#interrupt-cells = <3>;
+		#address-cells = <0>;
+		#size-cells = <0>;
+		interrupt-controller;

arch/arm/boot/dts/exynos5260.dtsi new file +15 -0

+			#address-cells = <0>;
+			#size-cells = <0>;
+			interrupt-map = <0x0 0 &gic 0 104 0>,
+					<0x1 0 &gic 0 105 0>,
+					<0x2 0 &gic 0 106 0>,
+					<0x3 0 &gic 0 107 0>,
+					<0x4 0 &gic 0 122 0>,
+					<0x5 0 &gic 0 123 0>,
+					<0x6 0 &gic 0 124 0>,
+					<0x7 0 &gic 0 125 0>,
+					<0x8 0 &gic 0 126 0>,
+					<0x9 0 &gic 0 127 0>,
+					<0xa 0 &gic 0 128 0>,
+					<0xb 0 &gic 0 129 0>;
+		};

arch/arm/boot/dts/exynos5260.dtsi new file +6 -0

+			compatible = "arm,cci-400-ctrl-if";
+			interface-type = "ace";
+			reg = <0x4000 0x1000>;			/* Please check again */
+		};
+
+		cci_control1: slave-if at 5000 {			/* Please check again */

arch/arm/boot/dts/exynos5260.dtsi new file +7 -0

+
+		wakeup-interrupt-controller {
+			compatible = "samsung,exynos4210-wakeup-eint";
+			interrupt-parent = <&gic>;
+			interrupts = <0 32 0>;
+		};
+	};

Hi Pankaj, Rahul, Arun,

Please split generic SoC dtsi files and board dts files into separate patches. Also please see my comments inline.

Quoted 4 lines

From: Arun Kumar K <[email protected]>

The patch adds the dts files for exynos5260 and for xyref evt0 board.

[snip]

arch/arm/boot/dts/exynos5260-pinctrl.dtsi line 158

Quoted 6 lines

arch/arm/boot/dts/exynos5260-pinctrl.dtsi new file +6 -0

+             gpx0: gpx0 {
+                     gpio-controller;
+                     #gpio-cells = <2>;
+
+                     interrupt-controller;
+                     #interrupt-cells = <2>;

Just to make sure, all your GPX banks are muxed type, with wake-up interrupts muxed to a single GIC interrupt line, right?

Quoted 2 lines

+ };

[snip]

arch/arm/boot/dts/exynos5260-pinctrl.dtsi line 359

Quoted 5 lines

arch/arm/boot/dts/exynos5260-pinctrl.dtsi new file +5 -0

+             cam_gpio_a: cam-gpio-a {
+                     samsung,pins = "gpe0-0", "gpe0-1", "gpe0-2", "gpe0-3",
+                             "gpe0-4", "gpe0-5", "gpe0-6", "gpe0-7",
+                             "gpe1-0", "gpe1-1";
+                             samsung,pin-function = <2>;

Incorrect indentation.

Quoted 3 lines

arch/arm/boot/dts/exynos5260-pinctrl.dtsi new file +3 -0

+                     samsung,pin-pud = <0>;
+                     samsung,pin-drv = <0>;
+             };

[snip]

arch/arm/boot/dts/exynos5260-pinctrl.dtsi line 402

Quoted 3 lines

arch/arm/boot/dts/exynos5260-pinctrl.dtsi new file +3 -0

+             hdmi_hpd_irq: hdmi-hpd-irq {
+                     samsung,pins = "gpx3-7";
+                     samsung,pin-function = <0>;

Function 0 is input, not a special function. It shouldn't be handled this way. If a board needs to set up pull-up/down and driver strength for GPIO pins then it should add its own board-specific pinconf nodes with just pin-pud and/or pin-drv properties and without pin-function.

Quoted 5 lines

arch/arm/boot/dts/exynos5260-pinctrl.dtsi new file +4 -0

+                     samsung,pin-pud = <1>;
+                     samsung,pin-drv = <0>;
+             };
+     };

[snip]

Quoted 10 lines

arch/arm/boot/dts/exynos5260-pinctrl.dtsi new file +10 -0

+             sd0_bus1: sd0-bus-width1 {
+                     samsung,pins = "gpc0-3";
+                     samsung,pin-function = <2>;
+                     samsung,pin-pud = <3>;
+                     samsung,pin-drv = <3>;
+             };
+
+             sd0_bus4: sd0-bus-width4 {
+                     samsung,pins = "gpc0-3", "gpc0-4", "gpc0-5", "gpc0-6";
+                     samsung,pin-function = <2>;

arch/arm/boot/dts/exynos5260-pinctrl.dtsi line 489

Quoted 10 lines

arch/arm/boot/dts/exynos5260-pinctrl.dtsi new file +10 -0

+                     samsung,pin-pud = <3>;
+                     samsung,pin-drv = <3>;
+             };
+
+             sd0_bus8: sd0-bus-width8 {
+                     samsung,pins = "gpc3-0", "gpc3-1", "gpc3-2", "gpc3-3";
+                     samsung,pin-function = <2>;
+                     samsung,pin-pud = <3>;
+                     samsung,pin-drv = <3>;
+             };

This is inconsistent. To specify 1- and 4-bit SD busses you need to include reference to just one pinconf node (sd0_bus1 or sd0_bus4), but for 8-bit bus you need to specify both sd0_bus4 and sd0_bus8.

Please make the nodes exclusive, so you always need to specify all possible configurations with given wiring (e.g. with 4 wires, you can run in 1-bit and 4-bit modes, not just 4-bit).

Same for remaining instances of SD bus.

[snip]

Quoted 18 lines

arch/arm/boot/dts/exynos5260-xyref5260-evt0.dts new file +12 -0

new file mode 100644
index 0000000..aa1fcda
@@ -0,0 +1,85 @@
+/*
+ * SAMSUNG XYREF5260 EVT0 board device tree source
+ *
+ * Copyright (c) 2013 Samsung Electronics Co., Ltd.
+ *           http://www.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.
+*/
+
+/dts-v1/;

arch/arm/boot/dts/exynos5260-xyref5260-evt0.dts line 18

Quoted 6 lines

arch/arm/boot/dts/exynos5260-xyref5260-evt0.dts new file +6 -0

+#include "exynos5260.dtsi"
+
+/ {
+     model = "SAMSUNG XYREF5260 EVT0 board based on EXYNOS5260";
+     compatible = "samsung,xyref5260", "samsung,exynos5260";
+

Shouldn't you have a memory node here?

arch/arm/boot/dts/exynos5260-xyref5260-evt0.dts line 28

Quoted 11 lines

arch/arm/boot/dts/exynos5260-xyref5260-evt0.dts new file +10 -0

+     chosen {
+             bootargs = "console=ttySAC2,115200";
+     };
+
+     fixed-rate-clocks {
+             oscclk {
+                     compatible = "samsung,exynos5260-oscclk";
+                     clock-frequency = <24000000>;
+             };
+     };

Please use generic fixed clock bindings. You can take [1] as an example how to use them.

[1] arch/arm/boot/dts/s3c6410-smdk6410.dtsi

Quoted 10 lines

arch/arm/boot/dts/exynos5260-xyref5260-evt0.dts new file +9 -0

+
+     serial at 12C00000 {
+             status = "okay";
+     };
+
+     serial at 12C10000 {
+             status = "okay";
+     };
+

arch/arm/boot/dts/exynos5260-xyref5260-evt0.dts line 42

Quoted 5 lines

arch/arm/boot/dts/exynos5260-xyref5260-evt0.dts new file +5 -0

+     serial at 12C20000 {
+             status = "okay";
+     };
+
+     serial at 12860000 {

Is it the correct UART address? It seems a bit off compared to addresses of other ports.

arch/arm/boot/dts/exynos5260-xyref5260-evt0.dts line 50

Quoted 8 lines

arch/arm/boot/dts/exynos5260-xyref5260-evt0.dts new file +8 -0

+             status = "okay";
+     };
+
+     dwmmc0 at 12140000 {
+             status = "okay";
+             num-slots = <1>;
+             broken-cd;
+             bypass-smu;

This is not a valid property, according to binding documentation.

arch/arm/boot/dts/exynos5260-xyref5260-evt0.dts line 52

Quoted 2 lines

arch/arm/boot/dts/exynos5260-xyref5260-evt0.dts new file +2 -0

+             supports-highspeed;
+             supports-hs200-mode; /* 200 Mhz */

Neither is this one.

arch/arm/boot/dts/exynos5260-xyref5260-evt0.dts line 53

Quoted 1 line

arch/arm/boot/dts/exynos5260-xyref5260-evt0.dts new file +1 -0

+             fifo-depth = <0x40>;

This is a property of the SoC, not the board.

Quoted 12 lines

arch/arm/boot/dts/exynos5260-xyref5260-evt0.dts new file +12 -0

+             card-detect-delay = <200>;
+             samsung,dw-mshc-ciu-div = <3>;
+             samsung,dw-mshc-sdr-timing = <0 4>;
+             samsung,dw-mshc-ddr-timing = <0 2>;
+             pinctrl-names = "default";
+             pinctrl-0 = <&sd0_clk &sd0_cmd &sd0_bus4 &sd0_bus8>;
+
+             slot at 0 {
+                     reg = <0>;
+                     bus-width = <8>;
+             };
+     };

arch/arm/boot/dts/exynos5260-xyref5260-evt0.dts line 71

Quoted 6 lines

arch/arm/boot/dts/exynos5260-xyref5260-evt0.dts new file +6 -0

+
+     dwmmc2 at 12160000 {
+             status = "okay";
+             num-slots = <1>;
+             supports-highspeed;
+             fifo-depth = <0x40>;

See above.

Quoted 31 lines

arch/arm/boot/dts/exynos5260-xyref5260-evt0.dts new file +14 -0

+             card-detect-delay = <200>;
+             samsung,dw-mshc-ciu-div = <3>;
+             samsung,dw-mshc-sdr-timing = <2 3>;
+             samsung,dw-mshc-ddr-timing = <1 2>;
+             pinctrl-names = "default";
+             pinctrl-0 = <&sd2_clk &sd2_cmd &sd2_cd &sd2_bus4>;
+
+             slot at 0 {
+                     reg = <0>;
+                     bus-width = <4>;
+                     disable-wp;
+             };
+     };
+};

arch/arm/boot/dts/exynos5260.dtsi new file +10 -0

new file mode 100644
index 0000000..fcb8d4f
@@ -0,0 +1,315 @@
+/*
+ * SAMSUNG EXYNOS5260 SoC device tree source
+ *
+ * Copyright (c) 2013 Samsung Electronics Co., Ltd.
+ *           http://www.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.
+*/

arch/arm/boot/dts/exynos5260.dtsi line 15

Quoted 5 lines

arch/arm/boot/dts/exynos5260.dtsi new file +5 -0

+
+#include "skeleton.dtsi"
+#include "exynos5260-pinctrl.dtsi"
+
+#include <dt-bindings/clk/exynos5260-clk.h>

This won't compile, because this file hasn't been added yet by previous patches.

Isn't it possible to reuse some of the definitions from exynos5.dtsi? How much different is this SoC from other SoCs from the series?

Quoted 28 lines

arch/arm/boot/dts/exynos5260.dtsi new file +27 -0

+
+/ {
+     compatible = "samsung,exynos5260";
+     interrupt-parent = <&gic>;
+
+     aliases {
+             pinctrl0 = &pinctrl_0;
+             pinctrl1 = &pinctrl_1;
+             pinctrl2 = &pinctrl_2;
+     };
+
+     chipid at 10000000 {
+             compatible = "samsung,exynos4210-chipid";
+             reg = <0x10000000 0x100>;
+     };
+
+     cpus {
+             #address-cells = <1>;
+             #size-cells = <0>;
+
+             cpu at 0 {
+                     device_type = "cpu";
+                     compatible = "arm,cortex-a15";
+                     reg = <0>;
+                     cci-control-port = <&cci_control1>;
+             };
+             cpu at 1 {

arch/arm/boot/dts/exynos5260.dtsi line 48

Quoted 6 lines

arch/arm/boot/dts/exynos5260.dtsi new file +6 -0

+                     device_type = "cpu";
+                     compatible = "arm,cortex-a15";
+                     reg = <1>;
+                     cci-control-port = <&cci_control1>;
+             };
+             cpu at 2 {

@unit-address suffix must match the first entry of reg property.

arch/arm/boot/dts/exynos5260.dtsi line 54

Quoted 7 lines

arch/arm/boot/dts/exynos5260.dtsi new file +6 -0

+                     device_type = "cpu";
+                     compatible = "arm,cortex-a7";
+                     reg = <0x100>;
+                     cci-control-port = <&cci_control0>;
+             };
+             cpu at 3 {

Ditto.

arch/arm/boot/dts/exynos5260.dtsi line 60

Quoted 6 lines

arch/arm/boot/dts/exynos5260.dtsi new file +6 -0

+                     device_type = "cpu";
+                     compatible = "arm,cortex-a7";
+                     reg = <0x101>;
+                     cci-control-port = <&cci_control0>;
+             };
+             cpu at 4 {

Ditto.

arch/arm/boot/dts/exynos5260.dtsi line 66

Quoted 6 lines

arch/arm/boot/dts/exynos5260.dtsi new file +6 -0

+                     device_type = "cpu";
+                     compatible = "arm,cortex-a7";
+                     reg = <0x102>;
+                     cci-control-port = <&cci_control0>;
+             };
+             cpu at 5 {

Ditto.

arch/arm/boot/dts/exynos5260.dtsi line 74

Quoted 8 lines

arch/arm/boot/dts/exynos5260.dtsi new file +8 -0

+                     device_type = "cpu";
+                     compatible = "arm,cortex-a7";
+                     reg = <0x103>;
+                     cci-control-port = <&cci_control0>;
+             };
+     };
+
+     cmus {

You need compatible = "simple-bus" here if you need the nodes below to be instantiated.

However I'm not sure if there is a point in placing them inside a simple-bus. This needs more thought, so please give me a bit more time to think over this and patches 4 and 7.

arch/arm/boot/dts/exynos5260.dtsi line 79

Quoted 6 lines

arch/arm/boot/dts/exynos5260.dtsi new file +5 -0

+             #address-cells = <1>;
+             #size-cells = <1>;
+             ranges;
+
+             cmu_top: clock-controller at 0x10010000 {

coding style: There should be no 0x prefix in @unit-address suffix.

+ all the CMU instances below.

[snip]

arch/arm/boot/dts/exynos5260.dtsi line 159

Quoted 2 lines

arch/arm/boot/dts/exynos5260.dtsi new file +2 -0

+
+     gic:interrupt-controller at 10481000 {

coding style: There should be a space after the colon ending the label.

Quoted 5 lines

arch/arm/boot/dts/exynos5260.dtsi new file +5 -0

+             compatible = "arm,cortex-a15-gic", "arm,cortex-a9-gic";
+             #interrupt-cells = <3>;
+             #address-cells = <0>;
+             #size-cells = <0>;
+             interrupt-controller;

arch/arm/boot/dts/exynos5260.dtsi line 174

Quoted 10 lines

arch/arm/boot/dts/exynos5260.dtsi new file +10 -0

+             reg = <0x10481000 0x1000>,
+                     <0x10482000 0x1000>,
+                     <0x10484000 0x2000>,
+                     <0x10486000 0x2000>;
+             interrupts = <1 9 0xf04>;
+     };
+
+     mct at 100B0000 {
+             compatible = "samsung,exynos4210-mct";
+             reg = <0x100B0000 0xb00>;

nit: Inconsistent hexadecimal character case, on Exynos in dts* files upper case should be used.

Also the reg size looks a bit suspicious, as it's not even page aligned. Is it the whole area used by the MCT block, not just the used registers?

arch/arm/boot/dts/exynos5260.dtsi line 97

Quoted 1 line

arch/arm/boot/dts/exynos5260.dtsi new file +1 -0

+             interrupt-controller;

MCT is not an interrupt controller.

arch/arm/boot/dts/exynos5260.dtsi line 176

Quoted 1 line

arch/arm/boot/dts/exynos5260.dtsi new file +1 -0

+             #interrups-cells = <2>;

Ditto. This is a property specific to interrupt controllers.

arch/arm/boot/dts/exynos5260.dtsi line 185

Quoted 9 lines

arch/arm/boot/dts/exynos5260.dtsi new file +9 -0

+             interrupt-parent = <&mct_map>;
+             interrupts = <0 0>, <1 0>, <2 0>, <3 0>,
+                             <4 0>, <5 0>, <6 0>, <7 0>,
+                             <8 0>, <9 0>, <10 0>, <11 0>;
+             clocks = <&cmu_top FIN_PLL>, <&cmu_peri PERI_PCLK_MCT>;
+             clock-names = "fin_pll", "mct";
+
+             mct_map: mct-map {
+                     #interrupt-cells = <2>;

Why two cells are needed? Using just one woudl simplify interrupt specifiers above and interrupt-map specifiers below.

Quoted 15 lines

arch/arm/boot/dts/exynos5260.dtsi new file +15 -0

+                     #address-cells = <0>;
+                     #size-cells = <0>;
+                     interrupt-map = <0x0 0 &gic 0 104 0>,
+                                     <0x1 0 &gic 0 105 0>,
+                                     <0x2 0 &gic 0 106 0>,
+                                     <0x3 0 &gic 0 107 0>,
+                                     <0x4 0 &gic 0 122 0>,
+                                     <0x5 0 &gic 0 123 0>,
+                                     <0x6 0 &gic 0 124 0>,
+                                     <0x7 0 &gic 0 125 0>,
+                                     <0x8 0 &gic 0 126 0>,
+                                     <0x9 0 &gic 0 127 0>,
+                                     <0xa 0 &gic 0 128 0>,
+                                     <0xb 0 &gic 0 129 0>;
+             };

arch/arm/boot/dts/exynos5260.dtsi line 210

Quoted 10 lines

arch/arm/boot/dts/exynos5260.dtsi new file +10 -0

+     };
+
+     cci at 10F00000 {
+             compatible = "arm,cci-400";
+             #address-cells = <1>;
+             #size-cells = <1>;
+             reg = <0x10F00000 0x1000>;
+             ranges = <0x0 0x10F00000 0x6000>;
+
+             cci_control0: slave-if at 4000 {                   /* Please check again */

Huh? Please check again and send correct data.

Quoted 12 lines

arch/arm/boot/dts/exynos5260.dtsi new file +12 -0

+                     compatible = "arm,cci-400-ctrl-if";
+                     interface-type = "ace";
+                     reg = <0x4000 0x1000>;                  /* Please check again */
+             };
+
+             cci_control1: slave-if at 5000 {                   /* Please check again */
+                     compatible = "arm,cci-400-ctrl-if";
+                     interface-type = "ace";
+                     reg = <0x5000 0x1000>;                  /* Please check again */
+             };
+     };
+

arch/arm/boot/dts/exynos5260.dtsi line 226

Quoted 4 lines

arch/arm/boot/dts/exynos5260.dtsi new file +4 -0

+     pinctrl_0: pinctrl at 11600000 {
+             compatible = "samsung,exynos5260-pinctrl";
+             reg = <0x11600000 0x1000>;
+             interrupts = <0 79 0>;                          /* GPIO_RT */

Instead of using such comment, maybe it would be better to rename labels of pinctrl nodes to be more meaningful, such as pinctrl_rt, pinctrl_fsys and pinctrl_aud?

arch/arm/boot/dts/exynos5260.dtsi line 233

Quoted 8 lines

arch/arm/boot/dts/exynos5260.dtsi new file +7 -0

+
+             wakeup-interrupt-controller {
+                     compatible = "samsung,exynos4210-wakeup-eint";
+                     interrupt-parent = <&gic>;
+                     interrupts = <0 32 0>;
+             };
+     };

[snip]

arch/arm/boot/dts/exynos5260.dtsi line 283

Quoted 2 lines

arch/arm/boot/dts/exynos5260.dtsi new file +2 -0

+
+     dwmmc_0: dwmmc0 at 12140000 {

Please use generic "mmc@" names for MMC nodes and move fifo-depth property here to SoC-level dtsi.

Best regards,
Tomasz

Samsung CCF helper functions do not provide support to register multiple Clock Providers for a given SoC. Due to this limitation SoC platforms are not able to use these helpers for registering mulitple clock providers and are forced to bypass this layer.

This layer is modified acordingly to enable the support.

Clockfile for exynos4, exynos5250, exynos5420, exynos5440 and S3c64xx are also modified as per changed helper functions.

drivers/clk/samsung/clk.c +0 -7

-static struct clk **clk_table;
-static void __iomem *reg_base;
-#ifdef CONFIG_OF
-static struct clk_onecell_data clk_data;
-#endif
 
 void samsung_clk_save(void __iomem *base,
 				    struct samsung_clk_reg_dump *rd,

@@ -55,40 +50,53 @@ struct samsung_clk_reg_dump *samsung_clk_alloc_reg_dump(
 }
 
 /* setup the essentials required to support clock lookup using ccf */
-void __init samsung_clk_init(struct device_node *np, void __iomem *base,
-			     unsigned long nr_clks)

drivers/clk/samsung/clk.c +13 -7

+
+	ctx = kzalloc(sizeof(struct samsung_clk_provider), GFP_KERNEL);
+	if (!ctx)
+		panic("could not allocate clock provider context.\n");
 
 	clk_table = kzalloc(sizeof(struct clk *) * nr_clks, GFP_KERNEL);
 	if (!clk_table)
 		panic("could not allocate clock lookup table\n");
 
-	if (!np)
-		return;
+	ctx->reg_base = base;
+	ctx->clk_data.clks = clk_table;
+	ctx->clk_data.clk_num = nr_clks;
 
-#ifdef CONFIG_OF
-	clk_data.clks = clk_table;
-	clk_data.clk_num = nr_clks;
-	of_clk_add_provider(np, of_clk_src_onecell_get, &clk_data);
-#endif
+	ret = of_clk_add_provider(np, of_clk_src_onecell_get,
+			&ctx->clk_data);
+	if (ret)
+		panic("could not register clock provide\n");
+
+	return ctx;
 }

drivers/clk/samsung/clk.h +24 -10

+struct samsung_clk_provider {
+	void __iomem *reg_base;
+	struct clk_onecell_data clk_data;
+};
+
 /**
  * struct samsung_clock_alias: information about mux clock
  * @id: platform specific id of the clock.

@@ -312,29 +323,40 @@ struct samsung_pll_clock {
 	__PLL(_typ, _id, NULL, _name, _pname, CLK_GET_RATE_NOCACHE,	\
 		_lock, _con, _rtable, _alias)
 
-extern void __init samsung_clk_init(struct device_node *np, void __iomem *base,
-				    unsigned long nr_clks);
+extern struct samsung_clk_provider *__init samsung_clk_init(
+			struct device_node *np, void __iomem *base,
+			unsigned long nr_clks);
 extern void __init samsung_clk_of_register_fixed_ext(
+		struct samsung_clk_provider *ctx,
 		struct samsung_fixed_rate_clock *fixed_rate_clk,
 		unsigned int nr_fixed_rate_clk,
 		struct of_device_id *clk_matches);
 
-extern void samsung_clk_add_lookup(struct clk *clk, unsigned int id);
+extern void samsung_clk_add_lookup(struct samsung_clk_provider *ctx,
+			struct clk *clk, unsigned int id);
 
-extern void samsung_clk_register_alias(struct samsung_clock_alias *list,
-		unsigned int nr_clk);
+extern void samsung_clk_register_alias(struct samsung_clk_provider *ctx,
+			struct samsung_clock_alias *list,
+			unsigned int nr_clk);
 extern void __init samsung_clk_register_fixed_rate(
-		struct samsung_fixed_rate_clock *clk_list, unsigned int nr_clk);
+			struct samsung_clk_provider *ctx,
+			struct samsung_fixed_rate_clock *clk_list,
+			unsigned int nr_clk);
 extern void __init samsung_clk_register_fixed_factor(
-		struct samsung_fixed_factor_clock *list, unsigned int nr_clk);
-extern void __init samsung_clk_register_mux(struct samsung_mux_clock *clk_list,
+			struct samsung_clk_provider *ctx,
+			struct samsung_fixed_factor_clock *list,
+			unsigned int nr_clk);
+extern void __init samsung_clk_register_mux(struct samsung_clk_provider *ctx,
+		struct samsung_mux_clock *clk_list,
 		unsigned int nr_clk);
-extern void __init samsung_clk_register_div(struct samsung_div_clock *clk_list,
+extern void __init samsung_clk_register_div(struct samsung_clk_provider *ctx,
+		struct samsung_div_clock *clk_list,
 		unsigned int nr_clk);
-extern void __init samsung_clk_register_gate(

Hi Rahul,

Please see my comments inline.

Quoted 10 lines · On Friday 06 of December 2013 21:26:28 Rahul Sharma wrote:

Samsung CCF helper functions do not provide support to register multiple Clock Providers for a given SoC. Due to this limitation SoC platforms are not able to use these helpers for registering mulitple clock providers and are forced to bypass this layer.

This layer is modified acordingly to enable the support.

Clockfile for exynos4, exynos5250, exynos5420, exynos5440 and S3c64xx are also modified as per changed helper functions.

[snip]

Quoted 7 lines

drivers/clk/samsung/clk.c +0 -0

index 91bec3e..20de446 100644
@@ -15,11 +15,6 @@
 #include "clk.h"
 

drivers/clk/samsung/clk.c line 17

Quoted 1 line

drivers/clk/samsung/clk.c +0 -0

 static DEFINE_SPINLOCK(lock);

IMHO you can also move the spinlock into samsung_clk_provider struct, to have more fine grained locking, as you shouldn't need to lock between particular providers, just multiple requests for one.

Quoted 20 lines

drivers/clk/samsung/clk.c +4 -8

-static struct clk **clk_table;
-static void __iomem *reg_base;
-#ifdef CONFIG_OF
-static struct clk_onecell_data clk_data;
-#endif
 
 void samsung_clk_save(void __iomem *base,
                                  struct samsung_clk_reg_dump *rd,

@@ -55,40 +50,53 @@ struct samsung_clk_reg_dump *samsung_clk_alloc_reg_dump(
 }
 
 /* setup the essentials required to support clock lookup using ccf */
-void __init samsung_clk_init(struct device_node *np, void __iomem *base,
-                          unsigned long nr_clks)
+struct samsung_clk_provider *__init samsung_clk_init(struct device_node *np,
+                     void __iomem *base, unsigned long nr_clks)
 {
-     reg_base = base;
+     struct samsung_clk_provider *ctx;
+     struct clk **clk_table;

drivers/clk/samsung/clk.c struct samsung_clk_reg_dump *samsung_clk_alloc_reg_dump( · line 61

Quoted 4 lines

drivers/clk/samsung/clk.c +4 -0

+     int ret;
+
+     if (!np)
+             return NULL;

This check is incorrect. It's completely correct to call this function with NULL np, when booted without DT and this was handled correctly before this patch. Please keep the same behavior.

Quoted 27 lines

drivers/clk/samsung/clk.c +13 -7

+
+     ctx = kzalloc(sizeof(struct samsung_clk_provider), GFP_KERNEL);
+     if (!ctx)
+             panic("could not allocate clock provider context.\n");
 
      clk_table = kzalloc(sizeof(struct clk *) * nr_clks, GFP_KERNEL);
      if (!clk_table)
              panic("could not allocate clock lookup table\n");
 
-     if (!np)
-             return;
+     ctx->reg_base = base;
+     ctx->clk_data.clks = clk_table;
+     ctx->clk_data.clk_num = nr_clks;
 
-#ifdef CONFIG_OF
-     clk_data.clks = clk_table;
-     clk_data.clk_num = nr_clks;
-     of_clk_add_provider(np, of_clk_src_onecell_get, &clk_data);
-#endif
+     ret = of_clk_add_provider(np, of_clk_src_onecell_get,
+                     &ctx->clk_data);
+     if (ret)
+             panic("could not register clock provide\n");
+
+     return ctx;
 }

[snip]

Quoted 8 lines

drivers/clk/samsung/clk.h +0 -0

index c7141ba..433bab3 100644
@@ -21,6 +21,17 @@
 #include <linux/of_address.h>
 #include "clk-pll.h"
 

drivers/clk/samsung/clk.h line 26

Quoted 3 lines

drivers/clk/samsung/clk.h +3 -0

+/* Context node which holds information about the clock provider. */
+/**
+ * struct samsung_clk_provider: information about clock plovider

typo: s/plovider/provider/

drivers/clk/samsung/clk.h line 29

Quoted 4 lines

drivers/clk/samsung/clk.h +3 -0

+ * @reg_base: virtual address for the register base.
+ * @clk_data: holds clock related data like clk* and number of clocks.
+ */

Why two comments? The kerneldoc one should be enough.

Quoted 59 lines

drivers/clk/samsung/clk.h +28 -12

+struct samsung_clk_provider {
+     void __iomem *reg_base;
+     struct clk_onecell_data clk_data;
+};
+
 /**
  * struct samsung_clock_alias: information about mux clock
  * @id: platform specific id of the clock.

@@ -312,29 +323,40 @@ struct samsung_pll_clock {
      __PLL(_typ, _id, NULL, _name, _pname, CLK_GET_RATE_NOCACHE,     \
              _lock, _con, _rtable, _alias)
 
-extern void __init samsung_clk_init(struct device_node *np, void __iomem *base,
-                                 unsigned long nr_clks);
+extern struct samsung_clk_provider *__init samsung_clk_init(
+                     struct device_node *np, void __iomem *base,
+                     unsigned long nr_clks);
 extern void __init samsung_clk_of_register_fixed_ext(
+             struct samsung_clk_provider *ctx,
              struct samsung_fixed_rate_clock *fixed_rate_clk,
              unsigned int nr_fixed_rate_clk,
              struct of_device_id *clk_matches);
 
-extern void samsung_clk_add_lookup(struct clk *clk, unsigned int id);
+extern void samsung_clk_add_lookup(struct samsung_clk_provider *ctx,
+                     struct clk *clk, unsigned int id);
 
-extern void samsung_clk_register_alias(struct samsung_clock_alias *list,
-             unsigned int nr_clk);
+extern void samsung_clk_register_alias(struct samsung_clk_provider *ctx,
+                     struct samsung_clock_alias *list,
+                     unsigned int nr_clk);
 extern void __init samsung_clk_register_fixed_rate(
-             struct samsung_fixed_rate_clock *clk_list, unsigned int nr_clk);
+                     struct samsung_clk_provider *ctx,
+                     struct samsung_fixed_rate_clock *clk_list,
+                     unsigned int nr_clk);
 extern void __init samsung_clk_register_fixed_factor(
-             struct samsung_fixed_factor_clock *list, unsigned int nr_clk);
-extern void __init samsung_clk_register_mux(struct samsung_mux_clock *clk_list,
+                     struct samsung_clk_provider *ctx,
+                     struct samsung_fixed_factor_clock *list,
+                     unsigned int nr_clk);
+extern void __init samsung_clk_register_mux(struct samsung_clk_provider *ctx,
+             struct samsung_mux_clock *clk_list,
              unsigned int nr_clk);
-extern void __init samsung_clk_register_div(struct samsung_div_clock *clk_list,
+extern void __init samsung_clk_register_div(struct samsung_clk_provider *ctx,
+             struct samsung_div_clock *clk_list,
              unsigned int nr_clk);
-extern void __init samsung_clk_register_gate(
+extern void __init samsung_clk_register_gate(struct samsung_clk_provider *ctx,
              struct samsung_gate_clock *clk_list, unsigned int nr_clk);
-extern void __init samsung_clk_register_pll(struct samsung_pll_clock *pll_list,
-             unsigned int nr_clk, void __iomem *base);
+extern void __init samsung_clk_register_pll(struct samsung_clk_provider *ctx,
+                     struct samsung_pll_clock *pll_list,
+                     unsigned int nr_clk, void __iomem *base);
 

drivers/clk/samsung/clk.h struct samsung_pll_clock { · line 362

Quoted 2 lines

drivers/clk/samsung/clk.h +0 -0

 extern unsigned long _get_rate(const char *clk_name);
 

nit: Please keep the indentation consistent in all the function prototypes above.

Best regards,
Tomasz

From: Pankaj Dubey <[email protected]>

exynos5260 use pll2520xx and it has different bitfields for P,M,S values as compared to pll2550xx. Support for pll2520xx is added here.

From: Pankaj Dubey <[email protected]>

exynos5260 use pll2520xx and it has different bitfields for P,M,S values as compared to pll2550xx. Support for pll2520xx is added here.

Signed-off-by Pankaj Dubey <[email protected]>

Signed-off-by Rahul Sharma <[email protected]>

Signed-off-by Arun Kumar K <[email protected]>

---
 drivers/clk/samsung/clk-pll.c |  107 +++++++++++++++++++++++++++++++++++++++++
 drivers/clk/samsung/clk-pll.h |    1 +
 2 files changed, 108 insertions(+)

drivers/clk/samsung/clk-pll.c +33 -0

index e8e8953..237a889 100644
@@ -710,6 +710,107 @@ struct clk * __init samsung_clk_register_pll2550x(const char *name,
 	return clk;
 }
 
+/*
+ * PLL2550xx Clock Type
+ */
+
+/* Maximum lock time can be 270 * PDIV cycles */
+#define PLL2550XX_LOCK_FACTOR (270)
+
+#define PLL2550XX_MDIV_MASK		(0x3FF)
+#define PLL2550XX_PDIV_MASK		(0x3F)
+#define PLL2550XX_SDIV_MASK		(0x7)
+#define PLL2550XX_LOCK_STAT_MASK	(0x1)
+#define PLL2550XX_MDIV_SHIFT		(9)
+#define PLL2550XX_PDIV_SHIFT		(3)
+#define PLL2550XX_SDIV_SHIFT		(0)
+#define PLL2550XX_LOCK_STAT_SHIFT	(21)
+
+static unsigned long samsung_pll2550xx_recalc_rate(struct clk_hw *hw,
+				unsigned long parent_rate)
+{
+	struct samsung_clk_pll *pll = to_clk_pll(hw);
+	u32 mdiv, pdiv, sdiv, pll_con;
+	u64 fvco = parent_rate;
+
+	pll_con = __raw_readl(pll->con_reg);
+	mdiv = (pll_con >> PLL2550XX_MDIV_SHIFT) & PLL2550XX_MDIV_MASK;
+	pdiv = (pll_con >> PLL2550XX_PDIV_SHIFT) & PLL2550XX_PDIV_MASK;
+	sdiv = (pll_con >> PLL2550XX_SDIV_SHIFT) & PLL2550XX_SDIV_MASK;
+
+	fvco *= mdiv;
+	do_div(fvco, (pdiv << sdiv));
+
+	return (unsigned long)fvco;
+}

drivers/clk/samsung/clk-pll.c +15 -0

+}
+
+static int samsung_pll2550xx_set_rate(struct clk_hw *hw, unsigned long drate,
+					unsigned long prate)
+{
+	struct samsung_clk_pll *pll = to_clk_pll(hw);
+	const struct samsung_pll_rate_table *rate;
+	u32 tmp;
+
+	/* Get required rate settings from table */
+	rate = samsung_get_pll_settings(pll, drate);
+	if (!rate) {
+		pr_err("%s: Invalid rate : %lu for pll clk %s\n", __func__,
+			drate, __clk_get_name(hw->clk));
+		return -EINVAL;

Hi Pankaj, Rahul, Arun,

Quoted 61 lines · On Friday 06 of December 2013 21:26:29 Rahul Sharma wrote:

From: Pankaj Dubey <[email protected]>

exynos5260 use pll2520xx and it has different bitfields for P,M,S values as compared to pll2550xx. Support for pll2520xx is added here.

Signed-off-by Pankaj Dubey <[email protected]>

Signed-off-by Rahul Sharma <[email protected]>

Signed-off-by Arun Kumar K <[email protected]>

---
 drivers/clk/samsung/clk-pll.c |  107 +++++++++++++++++++++++++++++++++++++++++
 drivers/clk/samsung/clk-pll.h |    1 +
 2 files changed, 108 insertions(+)

drivers/clk/samsung/clk-pll.c +39 -0

index e8e8953..237a889 100644
@@ -710,6 +710,107 @@ struct clk * __init samsung_clk_register_pll2550x(const char *name,
      return clk;
 }
 
+/*
+ * PLL2550xx Clock Type
+ */
+
+/* Maximum lock time can be 270 * PDIV cycles */
+#define PLL2550XX_LOCK_FACTOR (270)
+
+#define PLL2550XX_MDIV_MASK          (0x3FF)
+#define PLL2550XX_PDIV_MASK          (0x3F)
+#define PLL2550XX_SDIV_MASK          (0x7)
+#define PLL2550XX_LOCK_STAT_MASK     (0x1)
+#define PLL2550XX_MDIV_SHIFT         (9)
+#define PLL2550XX_PDIV_SHIFT         (3)
+#define PLL2550XX_SDIV_SHIFT         (0)
+#define PLL2550XX_LOCK_STAT_SHIFT    (21)
+
+static unsigned long samsung_pll2550xx_recalc_rate(struct clk_hw *hw,
+                             unsigned long parent_rate)
+{
+     struct samsung_clk_pll *pll = to_clk_pll(hw);
+     u32 mdiv, pdiv, sdiv, pll_con;
+     u64 fvco = parent_rate;
+
+     pll_con = __raw_readl(pll->con_reg);
+     mdiv = (pll_con >> PLL2550XX_MDIV_SHIFT) & PLL2550XX_MDIV_MASK;
+     pdiv = (pll_con >> PLL2550XX_PDIV_SHIFT) & PLL2550XX_PDIV_MASK;
+     sdiv = (pll_con >> PLL2550XX_SDIV_SHIFT) & PLL2550XX_SDIV_MASK;
+
+     fvco *= mdiv;
+     do_div(fvco, (pdiv << sdiv));
+
+     return (unsigned long)fvco;
+}
+
+static inline bool samsung_pll2550xx_mp_change(u32 mdiv, u32 pdiv, u32 pll_con)
+{
+     if ((mdiv != ((pll_con >> PLL2550XX_MDIV_SHIFT) &
+                             PLL2550XX_MDIV_MASK)) ||
+             (pdiv != ((pll_con >> PLL2550XX_PDIV_SHIFT) &

drivers/clk/samsung/clk-pll.c struct clk * __init samsung_clk_register_pll2550x(const char *name, · line 755

Quoted 4 lines

drivers/clk/samsung/clk-pll.c +4 -0

+                             PLL2550XX_PDIV_MASK)))
+             return 1;
+     else
+             return 0;

This doesn't look too good. Can you make this consistent with implementations of this helper for other PLLs, such as samsung_pll35xx_mp_change()?

Quoted 16 lines

drivers/clk/samsung/clk-pll.c +15 -0

+}
+
+static int samsung_pll2550xx_set_rate(struct clk_hw *hw, unsigned long drate,
+                                     unsigned long prate)
+{
+     struct samsung_clk_pll *pll = to_clk_pll(hw);
+     const struct samsung_pll_rate_table *rate;
+     u32 tmp;
+
+     /* Get required rate settings from table */
+     rate = samsung_get_pll_settings(pll, drate);
+     if (!rate) {
+             pr_err("%s: Invalid rate : %lu for pll clk %s\n", __func__,
+                     drate, __clk_get_name(hw->clk));
+             return -EINVAL;

drivers/clk/samsung/clk-pll.c struct clk * __init samsung_clk_register_pll2550x(const char *name, · line 780

Quoted 10 lines

drivers/clk/samsung/clk-pll.c +10 -0

+     }
+
+     tmp = __raw_readl(pll->con_reg);
+
+     if (!(samsung_pll2550xx_mp_change(rate->mdiv, rate->pdiv, tmp))) {
+             /* If only s change, change just s value only*/
+             tmp &= ~(PLL2550XX_SDIV_MASK << PLL2550XX_SDIV_SHIFT);
+             tmp |= rate->sdiv << PLL2550XX_SDIV_SHIFT;
+             __raw_writel(tmp, pll->con_reg);
+     } else {

Please make coding style of this function consistent with implementations of this operation for other PLLs, such as samsung_pll35xx_set_rate().

Otherwise the patch looks fine.

Best regards,
Tomasz

Add support for pll2650xx in samsung pll file. This pll variant is close to pll36xx but uses CON2 registers instead of CON1.

Aud_pll in Exynos5260 is pll2650xx and uses this code.

Signed-off-by Rahul Sharma <[email protected]>

---
 drivers/clk/samsung/clk-pll.c |  101 +++++++++++++++++++++++++++++++++++++++++
 drivers/clk/samsung/clk-pll.h |    2 +-
 2 files changed, 102 insertions(+), 1 deletion(-)

drivers/clk/samsung/clk-pll.c +8 -0

index 237a889..60c5679 100644
@@ -811,6 +811,101 @@ static const struct clk_ops samsung_pll2550xx_clk_min_ops = {
        .recalc_rate = samsung_pll2550xx_recalc_rate,
 };
 
+/*
+ * PLL2650XX Clock Type
+ */
+
+/* Maximum lock time can be 3000 * PDIV cycles */
+#define PLL2650XX_LOCK_FACTOR (3000)
+
+#define PLL2650XX_MDIV_SHIFT           (9)

Add support for pll2650xx in samsung pll file. This pll variant is close to pll36xx but uses CON2 registers instead of CON1.

Hi Rahul,

Quoted 2 lines · On Friday 06 of December 2013 21:26:30 Rahul Sharma wrote:

Add support for pll2650xx in samsung pll file. This pll variant is close to pll36xx but uses CON2 registers instead of CON1.

If the ops are otherwise idential, why not reuse the ops for pll36xx and use CON1 or CON2 register conditionally based on pll->type field? (Just as it is already done for pll4600, 4650 and 4650c.)

Best regards,
Tomasz

Hi Tomasz,

Quoted 10 lines · Tomasz Figa

Hi Rahul,

Quoted 2 lines · On Friday 06 of December 2013 21:26:30 Rahul Sharma wrote:

Add support for pll2650xx in samsung pll file. This pll variant is close to pll36xx but uses CON2 registers instead of CON1.

If the ops are otherwise idential, why not reuse the ops for pll36xx and use CON1 or CON2 register conditionally based on pll->type field? (Just as it is already done for pll4600, 4650 and 4650c.)

I verified the difference and found that pll2650xx is fairly different in terms of
Bit Fields, Con2 register, and additional PLL config bits than pll36xx. Due to this, I have to add lot of if-else code based on pll type which doesn't looks clean.