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

[Ley Foon Tan <ley.foon.tan@intel.com>]

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