From mboxrd@z Thu Jan  1 00:00:00 1970
From: Stephen Warren <swarren@wwwdotorg.org>
Date: Wed, 16 Jan 2013 15:42:02 -0700
Subject: [U-Boot] [PATCH 2/7] Tegra114: Add AVP (arm720t) files
In-Reply-To: <1358370848-29469-3-git-send-email-twarren@nvidia.com>
References: <1358370848-29469-1-git-send-email-twarren@nvidia.com>
	<1358370848-29469-3-git-send-email-twarren@nvidia.com>
Message-ID: <50F72CBA.9050800@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 01/16/2013 02:14 PM, Tom Warren wrote:
> This provides SPL support for T114 boards - AVP early init, plus
> CPU (A15) init/jump to main U-Boot.

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

> -	if (chip_id == 0x30)
> +	if (chip_id >= 0x30)
>  		return TEGRA_FAMILY_T3x;
>  	else
>  		return TEGRA_FAMILY_T2x;

Shouldn't that comparison use CHIPID_TEGRA30?

Shouldn't there be a TEGRA_FAMILY_T11x for Tegra114; anything that cares
about >=Tegra30 can compare the family with >= not ==.

Hmm. It seems the only use of the FAMILY value is get_num_cpus() right
below; why not have that just switch on the same chip_id variable that
everything else uses?

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

> +#ifndef TRUE
> +#define TRUE	1
> +#endif
> +#ifndef FALSE
> +#define FALSE	0
> +#endif

Surely those are in a standard header somewhere; we shouldn't create yet
another duplicate of them.

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

> +static int is_partition_powered(u32 mask)

> +	reg = readl(&pmc->pmc_pwrgate_status);
> +	if ((reg & mask) == mask)
> +		return TRUE;
> +
> +	return FALSE;

The last 4 lines are just "return reg & mask;" or "return (reg & mask)
== mask;". Same in the next function.

> +void powerup_cpus(void)
> +{
> +	debug("powerup_cpus entry\n");
> +
> +	/* Are we booting to the fast cluster? */
> +	if (get_cluster_id() == 0) {

Why would we ever boot on anything other than the fast cluster? I would
assume that the kernel assume it will always get booted on the fast
cluster, which would then imply that U-Boot had to boot on or switch to
the fast cluster.