Re: [U-Boot] [PATCH 07/20] arm: socfpga: add clock driver for Arria 10

On Sab, 2017-02-25 at 22:35 +0100, Marek Vasut wrote:

On 02/22/2017 10:47 AM, Ley Foon Tan wrote:

...

Add clock driver support for Arria 10 and update Gen5 clock driver.

Signed-off-by: Tien Fong Chee tien.fong.chee@intel.com Signed-off-by: Ley Foon Tan ley.foon.tan@intel.com

---

arch/arm/mach-socfpga/Makefile                     |    3 +-  arch/arm/mach-socfpga/clock_manager.c              |   36 +-  arch/arm/mach-socfpga/clock_manager_arria10.c      | 1104 ++++++++++++++++++++  arch/arm/mach-socfpga/clock_manager_gen5.c         |  132 ++-  arch/arm/mach-socfpga/include/mach/clock_manager.h |    8 +-  .../include/mach/clock_manager_arria10.h           |  231 ++++  6 files changed, 1455 insertions(+), 59 deletions(-)  create mode 100644 arch/arm/mach-socfpga/clock_manager_arria10.c  mode change 100755 => 100644 arch/arm/mach- socfpga/clock_manager_gen5.c  create mode 100644 arch/arm/mach- socfpga/include/mach/clock_manager_arria10.h

diff --git a/arch/arm/mach-socfpga/Makefile b/arch/arm/mach- socfpga/Makefile index d81f003..c494930 100644 --- a/arch/arm/mach-socfpga/Makefile +++ b/arch/arm/mach-socfpga/Makefile @@ -10,7 +10,8 @@  obj-y += misc.o timer.o reset_manager.o clock_manager.o \      fpga_manager.o board.o   -obj-$(CONFIG_TARGET_SOCFPGA_ARRIA10) += reset_manager_arria10.o +obj-$(CONFIG_TARGET_SOCFPGA_ARRIA10) += clock_manager_arria10.o \

• 			reset_manager_arria10.o
  

obj-$(CONFIG_SPL_BUILD) += spl.o freeze_controller.o   diff --git a/arch/arm/mach-socfpga/clock_manager.c b/arch/arm/mach- socfpga/clock_manager.c index d675973..7f48193 100644 --- a/arch/arm/mach-socfpga/clock_manager.c +++ b/arch/arm/mach-socfpga/clock_manager.c @@ -7,6 +7,7 @@  #include <common.h>  #include <asm/io.h>  #include <asm/arch/clock_manager.h> +#include <fdtdec.h>    DECLARE_GLOBAL_DATA_PTR;   @@ -18,7 +19,12 @@ void cm_wait_for_lock(u32 mask)   register u32 inter_val;   u32 retry = 0;   do { +#if defined(CONFIG_TARGET_SOCFPGA_ARRIA10)

A10 should be below Gen5 to keep this sorted chronologically IMO.

Okay.

...

• inter_val = readl(&clock_manager_base->stat) &
  

mask; +#elif defined(CONFIG_TARGET_SOCFPGA_GEN5)   inter_val = readl(&clock_manager_base->inter) & mask; +#endif

• /* Wait for stable lock */
  

if (inter_val == mask)   retry++;   else @@ -31,22 +37,39 @@ void cm_wait_for_lock(u32 mask)  /* function to poll in the fsm busy bit */  void cm_wait_for_fsm(void)  {

• while (readl(&clock_manager_base->stat) &

CLKMGR_STAT_BUSY)

• while (readl(&clock_manager_base->stat) &
• CLKMGR_CLKMGR_STAT_BUSY_SET_MSK)

;  }    static void cm_print_clock_quick_summary(void)  {   printf("MPU       %10ld kHz\n", cm_get_mpu_clk_hz() / 1000);

• printf("MMC         %8d kHz\n",

cm_get_mmc_controller_clk_hz() / 1000);

• printf("QSPI        %8d kHz\n",
• cm_get_qspi_controller_clk_hz() / 1000);
• printf("SPI         %8d kHz\n",

cm_get_spi_controller_clk_hz() / 1000); +#if defined(CONFIG_TARGET_SOCFPGA_ARRIA10)

• printf("EOSC1       %8d kHz\n", eosc1_hz / 1000);
• printf("cb_intosc   %8d kHz\n", cb_intosc_hz / 1000);
• printf("f2s_free    %8d kHz\n", f2s_free_hz / 1000);
• printf("Main VCO    %8d kHz\n",

cm_get_main_vco_clk_hz()/1000);

• printf("NOC         %8d kHz\n", cm_get_noc_clk_hz()/1000);
• printf("L4 Main     %8d kHz\n",
• cm_get_l4_noc_hz(CLKMGR_MAINPLL_NOCDIV_L4MAINCLK_LS

B)/1000);

• printf("L4 MP       %8d kHz\n",
• cm_get_l4_noc_hz(CLKMGR_MAINPLL_NOCDIV_L4MPCLK_LSB)

/1000);

• printf("L4 SP       %8d kHz\n",
• cm_get_l4_sp_clk_hz()/1000);
• printf("L4 sys free %8d kHz\n",

cm_l4_sys_free_clk_hz/1000); +#elif defined(CONFIG_TARGET_SOCFPGA_GEN5)   printf("DDR       %10ld kHz\n", cm_get_sdram_clk_hz() / 1000);   printf("EOSC1       %8d kHz\n", cm_get_osc_clk_hz(1) / 1000);   printf("EOSC2       %8d kHz\n", cm_get_osc_clk_hz(2) / 1000);   printf("F2S_SDR_REF %8d kHz\n", cm_get_f2s_sdr_ref_clk_hz() / 1000);   printf("F2S_PER_REF %8d kHz\n", cm_get_f2s_per_ref_clk_hz() / 1000);

• printf("MMC         %8d kHz\n",

cm_get_mmc_controller_clk_hz() / 1000);

• printf("QSPI        %8d kHz\n",

cm_get_qspi_controller_clk_hz() / 1000);   printf("UART        %8d kHz\n", cm_get_l4_sp_clk_hz() / 1000);

• printf("SPI         %8d kHz\n",

cm_get_spi_controller_clk_hz() / 1000); +#endif  }    int set_cpu_clk_info(void) @@ -57,7 +80,12 @@ int set_cpu_clk_info(void)     gd->bd->bi_arm_freq = cm_get_mpu_clk_hz() / 1000000;   gd->bd->bi_dsp_freq = 0;

+#if defined(CONFIG_TARGET_SOCFPGA_ARRIA10)

• gd->bd->bi_ddr_freq = 0;

+#elif defined(CONFIG_TARGET_SOCFPGA_GEN5)   gd->bd->bi_ddr_freq = cm_get_sdram_clk_hz() / 1000000; +#endif     return 0;  } diff --git a/arch/arm/mach-socfpga/clock_manager_arria10.c b/arch/arm/mach-socfpga/clock_manager_arria10.c new file mode 100644 index 0000000..4fa841f --- /dev/null +++ b/arch/arm/mach-socfpga/clock_manager_arria10.c @@ -0,0 +1,1104 @@ +/*

• • Copyright (C) 2016-2017 Intel Corporation
• • SPDX-License-Identifier:    GPL-2.0
• */

+#include <common.h> +#include <asm/io.h> +#include <asm/arch/clock_manager.h> +#include <fdtdec.h>

+DECLARE_GLOBAL_DATA_PTR;

+u32 eosc1_hz; +u32 cb_intosc_hz; +u32 f2s_free_hz; +u32 cm_l4_main_clk_hz; +u32 cm_l4_sp_clk_hz; +u32 cm_l4_mp_clk_hz; +u32 cm_l4_sys_free_clk_hz;

Shouldn't these values be static ? (if needed at all) ?

They are used in clock_manager.c clock info printing. But, I can restructure that part and keep all these static.

...

+struct strtopu32 {

• const char *str;
• u32 *p;

+};

What is this ^ and is this needed ? Probably not ...

Will remove.

[...]

...

+struct alteragrp_cfg {

• u32 nocclk;
• u32 mpuclk;

+};

+static const struct socfpga_clock_manager *clock_manager_base =

• (struct socfpga_clock_manager *)SOCFPGA_CLKMGR_ADDRESS;

+static int of_to_struct(const void *blob, int node, int cfg_len, void *cfg) +{

• if (fdtdec_get_int_array(blob, node, "altr,of_reg_value",

• 		 (u32 *)cfg, cfg_len)) {
  

• /* could not find required property */
  

• return 100;
  

100 ? Why ?

Will change to -EINVAL.

...

• }
• return 0;

+}

+static int of_get_input_clks(const void *blob, int node, void *cfg) +{

• if (fdtdec_get_int_array(blob, node, "clock-frequency",

• 		 (u32 *)cfg, 1)) {
  

• /* could not find required property */
  

• return 100;
  

DTTO

Same here.

...

• }
• return 0;

+}

+static int of_get_clk_cfg(const void *blob, struct mainpll_cfg *main_cfg,

• 	  struct perpll_cfg *per_cfg,
  

• 	  struct alteragrp_cfg *altrgrp_cfg)
  

+{

• int node, child, len;
• const char *node_name;
• node = fdtdec_next_compatible(blob, 0,

COMPAT_ALTERA_SOCFPGA_CLK);

• if (node < 0)

• return 1;
  

• child = fdt_first_subnode(blob, node);

Drop this extra space.

Noted.

...

• if (child < 0)

• return 2;
  

Use errno.h retvals ?

Will change to -EINVAL. Same for error below.

...

• child = fdt_first_subnode(blob, child);
• if (child < 0)

• return 3;
  

• node_name = fdt_get_name(blob, child, &len);
• while (node_name) {

• if (!strcmp(node_name, "osc1")) {
  

• 	if (of_get_input_clks(blob, child,
  

&eosc1_hz))

• 		return 101;
  

• } else if (!strcmp(node_name, "cb_intosc_ls_clk"))
  

{

• 	if (of_get_input_clks(blob, child,
  

&cb_intosc_hz))

• 		return 102;
  

• } else if (!strcmp(node_name, "f2s_free_clk")) {
  

• 	if (of_get_input_clks(blob, child,
  

&f2s_free_hz))

• 		return 103;
  

• } else if (!strcmp(node_name, "main_pll")) {
  

• 	if (of_to_struct(blob, child,
  

• 			 sizeof(*main_cfg)/sizeof(
  

u32),

• 			 main_cfg))
  

• 		return 104;
  

• } else if (!strcmp(node_name, "periph_pll")) {
  

• 	if (of_to_struct(blob, child,
  

• 			 sizeof(*per_cfg)/sizeof(u
  

32),

• 			 per_cfg))
  

• 		return 105;
  

• } else if (!strcmp(node_name, "altera")) {
  

• 	if (of_to_struct(blob, child,
  

• 			 sizeof(*altrgrp_cfg)/size
  

of(u32),

• 			 altrgrp_cfg))
  

• 		return 106;
  

• 	main_cfg->mpuclk = altrgrp_cfg->mpuclk;
  

• 	main_cfg->nocclk = altrgrp_cfg->nocclk;
  

• }
  

• child = fdt_next_subnode(blob, child);
  

• if (child < 0)
  

• 	break;
  

• node_name = fdt_get_name(blob, child, &len);
  

• }
• return 0;

+}

[...]

...

+/* ramping the main PLL to final value */ +static void cm_pll_ramp_main(struct mainpll_cfg *main_cfg,

• 	    struct perpll_cfg *per_cfg,
  

• 	    unsigned int pll_ramp_main_hz)
  

+{

• unsigned int clk_hz = 0, clk_incr_hz = 0, clk_final_hz =

0;

• /* find out the increment value */
• if (main_cfg->mpuclk_src ==

CLKMGR_MAINPLL_MPUCLK_SRC_MAIN) {

• clk_incr_hz = CLKMGR_PLL_RAMP_MPUCLK_INCREMENT_HZ;
  

• clk_final_hz =
  

cm_calc_handoff_mpu_clk_hz(main_cfg, per_cfg);

• } else if (main_cfg->nocclk_src ==

CLKMGR_MAINPLL_NOCCLK_SRC_MAIN) {

• clk_incr_hz = CLKMGR_PLL_RAMP_NOCCLK_INCREMENT_HZ;
  

• clk_final_hz =
  

cm_calc_handoff_noc_clk_hz(main_cfg, per_cfg);

• }
• /* execute the ramping here */
• for (clk_hz = pll_ramp_main_hz + clk_incr_hz;
• clk_hz < clk_final_hz; clk_hz += clk_incr_hz) {

• writel((main_cfg->vco1_denom <<
  

• 	CLKMGR_MAINPLL_VCO1_DENOM_LSB) |
  

• 	cm_calc_safe_pll_numer(0, main_cfg,
  

per_cfg, clk_hz),

• 	&clock_manager_base->main_pll.vco1);
  

• udelay(1000);
  

mdelay(1);

Okay.

...

• cm_wait_for_lock(LOCKED_MASK);
  

• }
• writel((main_cfg->vco1_denom <<

CLKMGR_MAINPLL_VCO1_DENOM_LSB) |

• main_cfg->vco1_numer,
  

• 	&clock_manager_base->main_pll.vco1);
  

• udelay(1000);
• cm_wait_for_lock(LOCKED_MASK);

+}

[...]

...

diff --git a/arch/arm/mach-socfpga/clock_manager_gen5.c b/arch/arm/mach-socfpga/clock_manager_gen5.c old mode 100755 new mode 100644 index 1df2ed4..5632843 --- a/arch/arm/mach-socfpga/clock_manager_gen5.c +++ b/arch/arm/mach-socfpga/clock_manager_gen5.c @@ -1,7 +1,7 @@  /*

• *  Copyright (C) 2013-2017 Altera Corporation <www.altera.com>

• • Copyright (C) 2016-2017 Intel Corporation

*

• • SPDX-License-Identifier: GPL-2.0+

• • SPDX-License-Identifier:    GPL-2.0

Keep the license, GPL-2.0+ ...

Okay.

...

*/    #include <common.h> @@ -65,7 +65,6 @@ static void cm_write_with_phase(u32 value,   * set source main and peripheral clocks   * Ungate clocks   */

void cm_basic_init(const struct cm_config * const cfg)  {   unsigned long end; @@ -304,58 +303,75 @@ void cm_basic_init(const struct cm_config * const cfg)          &clock_manager_base->inter);  }   -static unsigned int cm_get_main_vco_clk_hz(void) +unsigned int cm_get_main_vco_clk_hz(void)  {

• u32 reg, clock;

• u32 src_hz, numer, denom, vco;

/* get the main VCO clock */

• reg = readl(&clock_manager_base->main_pll.vco);
• clock = cm_get_osc_clk_hz(1);
• clock /= ((reg & CLKMGR_MAINPLLGRP_VCO_DENOM_MASK) >>

•   CLKMGR_MAINPLLGRP_VCO_DENOM_OFFSET) + 1;
  

• clock *= ((reg & CLKMGR_MAINPLLGRP_VCO_NUMER_MASK) >>

•   CLKMGR_MAINPLLGRP_VCO_NUMER_OFFSET) + 1;
  

• vco = readl(&clock_manager_base->main_pll.vco);

How many of the changes here are relevant ?

The changes to this file are total chaos and this really does need some splitting, it's unreviewable.

This file mainly to rename of variables and minor fixes on some clock calculation. I will separate these changes from this patch and move to after "[PATCH 01/20] arm: socfpga: restructure clock manager driver".

...

• return clock;

• numer = ((vco & CLKMGR_MAINPLLGRP_VCO_NUMER_MASK) >>

• CLKMGR_MAINPLLGRP_VCO_NUMER_OFFSET);
  

• denom = ((vco & CLKMGR_MAINPLLGRP_VCO_DENOM_MASK) >>

•  CLKMGR_MAINPLLGRP_VCO_DENOM_OFFSET);
  

• src_hz = cm_get_osc_clk_hz(1);
• vco = src_hz;
• vco /= (1 + denom);
• vco *= (1 + numer);
• return vco;

}

[...]

Thanks

Regards Ley Foon