From mboxrd@z Thu Jan  1 00:00:00 1970
From: Stephen Warren <swarren@wwwdotorg.org>
Date: Tue, 08 Oct 2013 15:18:25 -0600
Subject: [U-Boot] [PATCH] Tegra114: spl: Set system clock to clk_m
In-Reply-To: <1381180679-29524-1-git-send-email-twarren@nvidia.com>
References: <1381180679-29524-1-git-send-email-twarren@nvidia.com>
Message-ID: <525476A1.4020004@wwwdotorg.org>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

On 10/07/2013 03:17 PM, Tom Warren wrote:
> From: Jimmy Zhang <jimmzhang@nvidia.com>
> 
> Based on Tegra114 TRM, system clock can run up to 275MHz. On power on,

Which exact clock is "system clock"?

> the default sytem clock source is set to pllp_out0. In function
> clock_early_init(), pllp_out0 will be set to 480MHz which is beyond system
> clock's upper limit.
> 
> The fix is to set the system clock to CLK_M before initializing PLLP.
> Tested on A02 dalmore board.

> diff --git a/arch/arm/cpu/arm720t/tegra-common/spl.c b/arch/arm/cpu/arm720t/tegra-common/spl.c

> @@ -24,6 +28,9 @@ void spl_board_init(void)
>  	/* enable JTAG */
>  	writel(0xC0, &pmt->pmt_cfg_ctl);
>  
> +	/* use clk_m as avp clock source */
> +	set_avp_clock_to_clkm();

Doesn't clk_m run at the crystal frequency, so this is going to make the
AVP run pretty slow, at least for a while? Is that a good thing?

> diff --git a/arch/arm/cpu/arm720t/tegra114/cpu.c b/arch/arm/cpu/arm720t/tegra114/cpu.c

> @@ -127,8 +127,8 @@ void t114_init_clocks(void)
...
>  	/*
> -	 * Switch system clock to PLLP_OUT4 (108 MHz), AVP will now run
> -	 * at 108 MHz. This is glitch free as only the source is changed, no
> +	 * Switch system clock to PLLP_OUT4 (204 MHz), AVP will now run
> +	 * at 204 MHz. This is glitch free as only the source is changed, no
>  	 * special precaution needed.
>  	 */

... OK, so hopefully the AVP only runs very slowly for a very short
time. How much code runs between spl_board_init() and
t114_init_clocks()? Should spl_board_init() do all of the AVP clock
source setup to avoid too long a time running at crystal frequency?

> @@ -152,18 +152,11 @@ void t114_init_clocks(void)
>  	clock_set_enable(PERIPH_ID_CACHE2, 1);
>  	clock_set_enable(PERIPH_ID_GPIO, 1);
>  	clock_set_enable(PERIPH_ID_TMR, 1);
> -	clock_set_enable(PERIPH_ID_RTC, 1);

This seems entirely unrelated to the intended purpose of this patch. Why
remove all these clock_set_enable() calls? At the very least, this
should be explained in the commit description. Since it seems like a
different logical change, I would expect it to be a separate patch.

> @@ -220,7 +212,7 @@ static int is_clamp_enabled(u32 mask)
>  	u32 reg;
>  
>  	/* Get clamp status. TODO: Add pmc_clamp_status alias to pmc.h */
> -	reg = readl(&pmc->pmc_pwrgate_timer_on);
> +	reg = readl(&pmc->pmc_clamp_status);

Again, unrelated?

> +/*
> + * On poweron, AVP clock source (also called system clock) is set to PLLP_out0
> + * with frequency set at 1MHz. Before initializing PLLP, we need to move the
> + * system clock's source to CLK_M temporarily. And then switch it to PLLP_out4
> + * (204MHz) at a later time.
> + */
> +void set_avp_clock_to_clkm(void)

That comment would be better located in spl_board_init() in order to
explain why it calls this function. Any comment for this function would
be better explaining the function itself more than why other code might
call it.