Re: [PATCH 1/3] clk: exynos5420: Add 5800 specific clocks

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




Hi Arun, Alim,

On 02.05.2014 15:03, Arun Kumar K wrote:
From: Alim Akhtar <alim.akhtar@xxxxxxxxxxx>

Exynos5800 clock structure is mostly similar to 5420 with only
a small delta changes. So the 5420 clock file is re-used for
5800 also. The common clocks for both are seggreagated and few
clocks which are different for both are separately initialized.

Let's start with a general comment to this patch first. Have you consulted this with Shaik and Rahul (on CC) who are currently doing a quite heavy lifting of this driver to make sure that your work don't cause conflicts?

Also please see some more specific comments inline.


Signed-off-by: Alim Akhtar <alim.akhtar@xxxxxxxxxxx>
Signed-off-by: Arun Kumar K <arun.kk@xxxxxxxxxxx>
---
  .../devicetree/bindings/clock/exynos5420-clock.txt |    3 +-
  drivers/clk/samsung/clk-exynos5420.c               |  192 +++++++++++++++-----
  include/dt-bindings/clock/exynos5420.h             |    1 +
  3 files changed, 150 insertions(+), 46 deletions(-)

diff --git a/Documentation/devicetree/bindings/clock/exynos5420-clock.txt b/Documentation/devicetree/bindings/clock/exynos5420-clock.txt
index ca88c97..d54f42c 100644
--- a/Documentation/devicetree/bindings/clock/exynos5420-clock.txt
+++ b/Documentation/devicetree/bindings/clock/exynos5420-clock.txt
@@ -1,12 +1,13 @@
  * Samsung Exynos5420 Clock Controller

  The Exynos5420 clock controller generates and supplies clock to various
-controllers within the Exynos5420 SoC.
+controllers within the Exynos5420 SoC and for the Exynos5800 SoC.

  Required Properties:

  - compatible: should be one of the following.
    - "samsung,exynos5420-clock" - controller compatible with Exynos5420 SoC.
+  - "samsung,exynos5800-clock" - controller compatible with Exynos5800 SoC.

  - reg: physical base address of the controller and length of memory mapped
    region.
diff --git a/drivers/clk/samsung/clk-exynos5420.c b/drivers/clk/samsung/clk-exynos5420.c
index 60b2681..0543cb7 100644
--- a/drivers/clk/samsung/clk-exynos5420.c
+++ b/drivers/clk/samsung/clk-exynos5420.c
@@ -51,6 +51,7 @@
  #define SRC_TOP5		0x10214
  #define SRC_TOP6		0x10218
  #define SRC_TOP7		0x1021c
+#define SRC_TOP9		0x10224 /* 5800 specific */
  #define SRC_DISP10		0x1022c
  #define SRC_MAU			0x10240
  #define SRC_FSYS		0x10244
@@ -59,6 +60,7 @@
  #define SRC_TOP10		0x10280
  #define SRC_TOP11		0x10284
  #define SRC_TOP12		0x10288
+#define SRC_TOP13		0x1028c /* 5800 specific */
  #define	SRC_MASK_DISP10		0x1032c
  #define SRC_MASK_FSYS		0x10340
  #define SRC_MASK_PERIC0		0x10350
@@ -66,6 +68,7 @@
  #define DIV_TOP0		0x10500
  #define DIV_TOP1		0x10504
  #define DIV_TOP2		0x10508
+#define DIV_TOP9		0x10524 /* 5800 specific */
  #define DIV_DISP10		0x1052c
  #define DIV_MAU			0x10544
  #define DIV_FSYS0		0x10548
@@ -102,8 +105,14 @@
  #define SRC_KFC			0x28200
  #define DIV_KFC0		0x28500

+/* Exynos5x SoC type */
+enum exynos5x_soc {
+	EXYNOS5420,
+	EXYNOS5800,
+};
+
  /* list of PLLs */
-enum exynos5420_plls {
+enum exynos5x_plls {
  	apll, cpll, dpll, epll, rpll, ipll, spll, vpll, mpll,
  	bpll, kpll,
  	nr_plls			/* number of PLLs */
@@ -112,13 +121,13 @@ enum exynos5420_plls {
  static void __iomem *reg_base;

  #ifdef CONFIG_PM_SLEEP
-static struct samsung_clk_reg_dump *exynos5420_save;
+static struct samsung_clk_reg_dump *exynos5x_save;

  /*
   * list of controller registers to be saved and restored during a
   * suspend/resume cycle.
   */
-static unsigned long exynos5420_clk_regs[] __initdata = {
+static unsigned long exynos5x_clk_regs[] __initdata = {
  	SRC_CPU,
  	DIV_CPU0,
  	DIV_CPU1,
@@ -182,16 +191,16 @@ static unsigned long exynos5420_clk_regs[] __initdata = {

  static int exynos5420_clk_suspend(void)
  {
-	samsung_clk_save(reg_base, exynos5420_save,
-				ARRAY_SIZE(exynos5420_clk_regs));
+	samsung_clk_save(reg_base, exynos5x_save,
+				ARRAY_SIZE(exynos5x_clk_regs));

Don't you need to handle save and restore of 5800-specific registers as well? You can see Exynos4 clock driver for an example of handling such setup.


  	return 0;
  }

  static void exynos5420_clk_resume(void)
  {
-	samsung_clk_restore(reg_base, exynos5420_save,
-				ARRAY_SIZE(exynos5420_clk_regs));
+	samsung_clk_restore(reg_base, exynos5x_save,
+				ARRAY_SIZE(exynos5x_clk_regs));

Ditto.

  }

  static struct syscore_ops exynos5420_clk_syscore_ops = {
@@ -201,9 +210,9 @@ static struct syscore_ops exynos5420_clk_syscore_ops = {

  static void exynos5420_clk_sleep_init(void)
  {
-	exynos5420_save = samsung_clk_alloc_reg_dump(exynos5420_clk_regs,
-					ARRAY_SIZE(exynos5420_clk_regs));
-	if (!exynos5420_save) {
+	exynos5x_save = samsung_clk_alloc_reg_dump(exynos5x_clk_regs,
+					ARRAY_SIZE(exynos5x_clk_regs));
+	if (!exynos5x_save) {
  		pr_warn("%s: failed to allocate sleep save data, no sleep support!\n",
  			__func__);
  		return;
@@ -296,13 +305,29 @@ PNAME(hdmi_p)	= { "dout_hdmi_pixel", "sclk_hdmiphy" };
  PNAME(maudio0_p)	= { "fin_pll", "maudio_clk", "sclk_dpll", "sclk_mpll",
  			  "sclk_spll", "sclk_ipll", "sclk_epll", "sclk_rpll" };

+/* List of parents specific to exynos5800 */
+PNAME(mout_epll2_5800_p)	= { "mout_sclk_epll", "ffactor_dout_epll2" };
+PNAME(mout_group1_5800_p)	= { "mout_sclk_cpll", "mout_sclk_dpll",
+				"mout_sclk_mpll", "ffactor_dout_spll2" };
+PNAME(mout_group3_5800_p)	= { "mout_sclk_cpll", "mout_sclk_dpll",
+					"mout_sclk_mpll", "ffactor_dout_spll2",
+					"mout_epll2" };
+PNAME(mout_group5_5800_p)	= { "mout_sclk_cpll", "mout_sclk_dpll",
+					"mout_sclk_mpll", "mout_sclk_spll" };
+PNAME(mout_group6_5800_p)	= { "mout_sclk_ipll", "mout_sclk_dpll",
+				"mout_sclk_mpll", "ffactor_dout_spll2" };
+PNAME(mout_group8_5800_p)	= { "dout_aclk432_scaler", "dout_sclk_sw" };
+PNAME(mout_group9_5800_p)	= { "dout_osc_div", "mout_sw_aclk432_scaler" };
+
  /* fixed rate clocks generated outside the soc */
-static struct samsung_fixed_rate_clock exynos5420_fixed_rate_ext_clks[] __initdata = {
+static struct
+samsung_fixed_rate_clock exynos5x_fixed_rate_ext_clks[] __initdata = {

This is kind of strange way of wrapping long lines. I'd say that at least struct should be in the same line as samsung_fixed_rate_clock, so that could be at least kind of readable.

  	FRATE(CLK_FIN_PLL, "fin_pll", NULL, CLK_IS_ROOT, 0),
  };

  /* fixed rate clocks generated inside the soc */
-static struct samsung_fixed_rate_clock exynos5420_fixed_rate_clks[] __initdata = {
+static struct
+samsung_fixed_rate_clock exynos5x_fixed_rate_clks[] __initdata = {

Ditto.

  	FRATE(CLK_SCLK_HDMIPHY, "sclk_hdmiphy", NULL, CLK_IS_ROOT, 24000000),
  	FRATE(0, "sclk_pwi", NULL, CLK_IS_ROOT, 24000000),
  	FRATE(0, "sclk_usbh20", NULL, CLK_IS_ROOT, 48000000),
@@ -310,39 +335,88 @@ static struct samsung_fixed_rate_clock exynos5420_fixed_rate_clks[] __initdata =
  	FRATE(0, "sclk_usbh20_scan_clk", NULL, CLK_IS_ROOT, 480000000),
  };

-static struct samsung_fixed_factor_clock exynos5420_fixed_factor_clks[] __initdata = {
+static struct
+samsung_fixed_factor_clock exynos5x_fixed_factor_clks[] __initdata = {

Ditto (and the same for other numerous instances of this below).

  	FFACTOR(0, "sclk_hsic_12m", "fin_pll", 1, 2, 0),
  };

-static struct samsung_mux_clock exynos5420_mux_clks[] __initdata = {
-	MUX(0, "mout_mspll_kfc", mspll_cpu_p, SRC_TOP7, 8, 2),
-	MUX(0, "mout_mspll_cpu", mspll_cpu_p, SRC_TOP7, 12, 2),
-	MUX(0, "mout_apll", apll_p, SRC_CPU, 0, 1),
-	MUX(0, "mout_cpu", cpu_p, SRC_CPU, 16, 1),
-	MUX(0, "mout_kpll", kpll_p, SRC_KFC, 0, 1),
-	MUX(0, "mout_cpu_kfc", kfc_p, SRC_KFC, 16, 1),
+static struct
+samsung_fixed_factor_clock exynos5800_fixed_factor_clks[] __initdata = {
+	FFACTOR(0, "ffactor_dout_epll2", "mout_sclk_epll", 1, 2, 0),
+	FFACTOR(0, "ffactor_dout_spll2", "mout_sclk_spll", 1, 2, 0),

I don't think you need the "ffactor_" prefix in the name. I assume "dout_epll2" and "dout_spll2" are the names listed in the datasheet. Is that correct?

Best regards,
Tomasz
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux