Re: [PATCH U-Boot v1] aspeed/ast-scu.c: add ast_get_m_pll_clk() support

[Andrew Jeffery]

Hi David,

Apologies for the long turn-around time.

On Sat, 16 Feb 2019, at 06:56, David Thompson wrote:
> The source code for ast_get_m_pll_clk() is adapted from
> the file "arch/arm/mach-aspeed/ast-bmc-scu.c" contained
> in the bootloader portion of the ASPEED AST2500 SDK.
> 
> The values for "A2" silicon revision IDs are from the
> document "AST2500/AST2520 Integrated Remote Management
> Processor A2 Datasheet", version 1.6, pages 373-374.

This tells us the what but doesn't tell us the why. A lot of what's
in the commit message would be more useful as comments in the
code in the appropriate places. The only reference I've found in
the source is in arch/arm/mach-aspeed/cpuinfo.c which is only
required if CONFIG_DISPLAY_CPUINFO is enabled.  Is the
motivation for the patch that you want to enable
CONFIG_DISPLAY_CPUINFO, or something else?

> 
> Signed-off-by: David Thompson <dthompson at mellanox.com>
> Reviewed-by: Shravan Ramani <sramani at mellanox.com>
> ---
>  arch/arm/include/asm/arch-aspeed/ast_scu.h |  1 +
>  arch/arm/mach-aspeed/ast-scu.c             | 30 ++++++++++++++++++++++++++++++
>  2 files changed, 31 insertions(+)
> 
> diff --git a/arch/arm/include/asm/arch-aspeed/ast_scu.h 
> b/arch/arm/include/asm/arch-aspeed/ast_scu.h
> index d248416..dcbc673 100644
> --- a/arch/arm/include/asm/arch-aspeed/ast_scu.h
> +++ b/arch/arm/include/asm/arch-aspeed/ast_scu.h
> @@ -37,6 +37,7 @@ extern void ast_scu_get_who_init_dram(void);
>  
>  extern u32 ast_get_clk_source(void);
>  extern u32 ast_get_h_pll_clk(void);
> +extern u32 ast_get_m_pll_clk(void);
>  extern u32 ast_get_ahbclk(void);
>  
>  extern u32 ast_scu_get_vga_memsize(void);
> diff --git a/arch/arm/mach-aspeed/ast-scu.c b/arch/arm/mach-aspeed/ast-scu.c
> index 0cc0d67..6184ff4 100644
> --- a/arch/arm/mach-aspeed/ast-scu.c
> +++ b/arch/arm/mach-aspeed/ast-scu.c
> @@ -100,6 +100,10 @@ static struct soc_id soc_map_table[] = {
>  	SOC_ID("AST2510-A1", 0x04010103),
>  	SOC_ID("AST2520-A1", 0x04010203),
>  	SOC_ID("AST2530-A1", 0x04010403),
> +	SOC_ID("AST2500-A2", 0x04030303),
> +	SOC_ID("AST2510-A2", 0x04030103),
> +	SOC_ID("AST2520-A2", 0x04030203),
> +	SOC_ID("AST2530-A2", 0x04030403),
>  };
>  
>  void ast_scu_init_eth(u8 num)
> @@ -235,6 +239,32 @@ u32 ast_get_h_pll_clk(void)
>  	return clk;
>  }
>  
> +u32 ast_get_m_pll_clk(void)
> +{
> +	u32 clk = 0;
> +	u32 m_pll_set = ast_scu_read(AST_SCU_M_PLL);
> +
> +	if (m_pll_set & SCU_M_PLL_OFF)
> +		return 0;
> +
> +	/* Programming */
> +	clk = ast_get_clk_source();
> +	if (m_pll_set & SCU_M_PLL_BYPASS) {
> +		return clk;
> +	} else {
> +		/* P = SCU20[13:18]
> +		 * M = SCU20[5:12]
> +		 * N = SCU20[0:4]
> +		 * mpll = 24MHz * [(M+1) /(N+1)] / (P+1)
> +		 */
> +		clk = ((clk * (SCU_M_PLL_GET_MNUM(m_pll_set) + 1)) /
> +			   (SCU_M_PLL_GET_NNUM(m_pll_set) + 1)) /
> +			(SCU_M_PLL_GET_PDNUM(m_pll_set) + 1);
> +	}
> +	debug("m_pll = %d\n", clk);
> +	return clk;
> +}
> +

The implementation itself looks correct to me.

Andrew

>  u32 ast_get_ahbclk(void)
>  {
>  	unsigned int axi_div, ahb_div, hpll;
> -- 
> 2.1.2
> 
>