[U-Boot] [PATCH 3/8] Tegra124: Add SPL/AVP (arm720t) cpu files
Thierry Reding
treding at nvidia.com
Tue Oct 8 10:13:20 CEST 2013
On Tue, Oct 08, 2013 at 12:42:53AM +0200, Tom Warren wrote:
> This provides SPL support for T124 boards - AVP
> early init, plus CPU (A15) init/jump to main U-Boot.
>
> Change-Id: I721f83f1d5fa549e0698e0cc76ab3e5ea11ba895
> Signed-off-by: Tom Warren <twarren at nvidia.com>
> ---
> arch/arm/cpu/arm720t/tegra-common/cpu.c | 63 +++++--
> arch/arm/cpu/arm720t/tegra-common/cpu.h | 6 +-
> arch/arm/cpu/arm720t/tegra124/Makefile | 31 ++++
> arch/arm/cpu/arm720t/tegra124/config.mk | 7 +
> arch/arm/cpu/arm720t/tegra124/cpu.c | 301 ++++++++++++++++++++++++++++++++
> 5 files changed, 387 insertions(+), 21 deletions(-)
> create mode 100644 arch/arm/cpu/arm720t/tegra124/Makefile
> create mode 100644 arch/arm/cpu/arm720t/tegra124/config.mk
> create mode 100644 arch/arm/cpu/arm720t/tegra124/cpu.c
>
> diff --git a/arch/arm/cpu/arm720t/tegra-common/cpu.c b/arch/arm/cpu/arm720t/tegra-common/cpu.c
> index 72c69b9..fbe553a 100644
> --- a/arch/arm/cpu/arm720t/tegra-common/cpu.c
> +++ b/arch/arm/cpu/arm720t/tegra-common/cpu.c
> @@ -1,17 +1,8 @@
> /*
> - * Copyright (c) 2010-2012, NVIDIA CORPORATION. All rights reserved.
> + * (C) Copyright 2013
> + * NVIDIA Corporation <www.nvidia.com>
What about years 2010-2012? Shouldn't they be kept in the copyright
line? Also, we should probably try to be more consistent with our
copyright notices. There's a variety of ways that we use:
(C) Copyright 2013
NVIDIA Corporation <www.nvidia.com>
Copyright (c) 2010-2013, NVIDIA CORPORATION. All rights reserved.
(C) Copyright 2010-2013, NVIDIA Corporation <www.nvidia.com>
Copyright (c) 2010-2011 NVIDIA Corporation
There may be more. I personally have a preference for the last of these,
but as long as we can keep some consistency I don't mind which one we
settle on.
> void adjust_pllp_out_freqs(void)
> @@ -146,6 +153,18 @@ int pllx_set_rate(struct clk_pll_simple *pll , u32 divn, u32 divm,
>
> debug(" pllx_set_rate entry\n");
>
> +#if defined(CONFIG_TEGRA124)
> + struct clk_rst_ctlr *clkrst = (struct clk_rst_ctlr *)NV_PA_CLK_RST_BASE;
> +
> + /* Disable IDDQ */
> + reg = readl(&clkrst->crc_pllx_misc3);
> + reg &= ~PLLX_IDDQ_MASK;
> + writel(reg, &clkrst->crc_pllx_misc3);
> + udelay(2);
> + debug("%s: IDDQ: PLLX IDDQ = 0x%08X\n", __func__,
> + readl(&clkrst->crc_pllx_misc3));
> +#endif /* T124 */
Perhaps this should be moved to a separate function that can be provided
as a dummy for non-Tegra124?
> @@ -162,18 +181,23 @@ int pllx_set_rate(struct clk_pll_simple *pll , u32 divn, u32 divm,
> reg |= (1 << PLL_DCCON_SHIFT);
> writel(reg, &pll->pll_misc);
>
> - /* Enable PLLX */
> - reg = readl(&pll->pll_base);
> - reg |= PLL_ENABLE_MASK;
> -
> /* Disable BYPASS */
> + reg = readl(&pll->pll_base);
> reg &= ~PLL_BYPASS_MASK;
> writel(reg, &pll->pll_base);
> + debug(" pllx_set_rate: base = 0x%08X\n", reg);
Why's there a leading space in that debug message? I see that other
debug messages have it as well, but I don't see any reason for it.
Copied and pasted?
> /* Set lock_enable to PLLX_MISC */
> reg = readl(&pll->pll_misc);
> reg |= PLL_LOCK_ENABLE_MASK;
> writel(reg, &pll->pll_misc);
> + debug(" pllx_set_rate: misc = 0x%08X\n", reg);
> +
> + /* Enable PLLX last, as per JZ */
I guess JZ is Jimmy Zhang, but a proper explanation for why this is done
on necessary would be more valuable.
> - /* adjust PLLP_out1-4 on T3x/T114 */
> + /* adjust PLLP_out1-4 on T3x/T1x4 */
I don't know about this. What ever happens if engineering comes up with
a chip, say, T194 that's not compatible? I think there's some sense in
being explicit here and making this T3x/T114/T124. I don't know why T3x
is used here either for that matter.
> soc_type = tegra_get_chip();
> - if (soc_type == CHIPID_TEGRA30 || soc_type == CHIPID_TEGRA114)
> + if (soc_type == CHIPID_TEGRA30 || soc_type == CHIPID_TEGRA114 ||
> + soc_type == CHIPID_TEGRA124)
Perhaps:
if (soc_type >= CHIPID_TEGRA30)
?
> @@ -12,7 +12,8 @@
>
> #if defined(CONFIG_TEGRA20)
> #define NVBL_PLLP_KHZ (216000)
> -#elif defined(CONFIG_TEGRA30) || defined(CONFIG_TEGRA114)
> +#elif defined(CONFIG_TEGRA30) || defined(CONFIG_TEGRA114) || \
> + defined(CONFIG_TEGRA124)
> #define NVBL_PLLP_KHZ (408000)
> #else
> #error "Unknown Tegra chip!"
> @@ -68,3 +69,4 @@ int tegra_get_chip(void);
> int tegra_get_sku_info(void);
> int tegra_get_chip_sku(void);
> void adjust_pllp_out_freqs(void);
> +void pmic_enable_cpu_vdd(void);
This doesn't seem to exist until patch 8. Perhaps this should really be
an weak function so that it always exists but can still be overwritten
by individual boards?
> diff --git a/arch/arm/cpu/arm720t/tegra124/cpu.c b/arch/arm/cpu/arm720t/tegra124/cpu.c
> +static void enable_cpu_power_rail(void)
> +{
> + struct pmc_ctlr *pmc = (struct pmc_ctlr *)NV_PA_PMC_BASE;
> +
> + debug("enable_cpu_power_rail entry\n");
> +
> + /* un-tristate PWR_I2C SCL/SDA, rest of the defaults are correct */
> + pinmux_tristate_disable(PINGRP_PWR_I2C_SCL);
> + pinmux_tristate_disable(PINGRP_PWR_I2C_SDA);
> +
> + pmic_enable_cpu_vdd();
> +
> + /*
> + * Set CPUPWRGOOD_TIMER - APB clock is 1/2 of SCLK (102MHz),
> + * set it for 5ms as per SysEng (102MHz/5mS = 510000 (7C830h).
Nit: "102MHz/5ms". And it should probably be 102MHz*5ms to yield the
correct result.
> +static void enable_cpu_clocks(void)
> +{
> + struct clk_rst_ctlr *clkrst = (struct clk_rst_ctlr *)NV_PA_CLK_RST_BASE;
> + u32 reg;
> +
> + debug("enable_cpu_clocks entry\n");
> +
> + /* Wait for PLL-X to lock */
> + do {
> + reg = readl(&clkrst->crc_pll_simple[SIMPLE_PLLX].pll_base);
> + debug("%s: PLLX base = 0x%08X\n", __func__, reg);
> + } while ((reg & (1 << 27)) == 0);
1 << 27 -> PLLX_LOCK?
> + debug("%s: Enabling clock to all CPUs\n", __func__);
> + /* Enable the clock to all CPUs */
> + reg = readl(&clkrst->crc_clk_cpu_cmplx_clr);
> + reg |= (CLR_CPU3_CLK_STP + CLR_CPU2_CLK_STP);
> + reg |= CLR_CPU1_CLK_STP;
> + writel((reg | CLR_CPU0_CLK_STP), &clkrst->crc_clk_cpu_cmplx_clr);
Perhaps:
reg = readl(&clkrst->crc_clk_cpu_cmplx_clr);
reg |= CLR_CPU3_CLK_STP | CLR_CPU2_CLK_STP | CLR_CPU1_CLK_STP |
CLR_CPU0_CLK_STP;
writel(reg, &clkrst->crc_clk_cpu_cmplx_clr);
? Also this is a write-1-to-clear register, right? So there should be no
need to read it first and OR in the new bits.
> +static void remove_cpu_resets(void)
> +{
> + struct clk_rst_ctlr *clkrst = (struct clk_rst_ctlr *)NV_PA_CLK_RST_BASE;
> + u32 reg;
> +
> + debug("remove_cpu_resets entry\n");
> +
> + /* Take the slow and fast partitions out of reset */
> + reg = CLR_NONCPURESET;
> + writel(reg, &clkrst->crc_rst_cpulp_cmplx_clr);
> + writel(reg, &clkrst->crc_rst_cpug_cmplx_clr);
> +
> + /* Clear the SW-controlled reset of the slow cluster */
> + reg = (CLR_CPURESET0 + CLR_DBGRESET0 + CLR_CORERESET0 + CLR_CXRESET0);
> + reg |= (CLR_L2RESET + CLR_PRESETDBG);
s/+/|/? And you can safely drop the parentheses as they aren't required.
> + writel(reg, &clkrst->crc_rst_cpulp_cmplx_clr);
> +
> + /* Clear the SW-controlled reset of the fast cluster */
> + reg = (CLR_CPURESET0 + CLR_DBGRESET0 + CLR_CORERESET0 + CLR_CXRESET0);
> + reg |= (CLR_CPURESET1 + CLR_DBGRESET1 + CLR_CORERESET1 + CLR_CXRESET1);
> + reg |= (CLR_CPURESET2 + CLR_DBGRESET2 + CLR_CORERESET2 + CLR_CXRESET2);
> + reg |= (CLR_CPURESET3 + CLR_DBGRESET3 + CLR_CORERESET3 + CLR_CXRESET3);
> + reg |= (CLR_L2RESET + CLR_PRESETDBG);
Same here.
> +static int is_partition_powered(u32 mask)
Perhaps this should return bool?
> + struct pmc_ctlr *pmc = (struct pmc_ctlr *)NV_PA_PMC_BASE;
> + u32 reg;
> +
> + /* Get power gate status */
> + reg = readl(&pmc->pmc_pwrgate_status);
> + return (reg & mask) == mask;
> +}
> +
> +static void power_partition(u32 status, u32 partid)
> +{
> + struct pmc_ctlr *pmc = (struct pmc_ctlr *)NV_PA_PMC_BASE;
> +
> + debug("%s: status = %08X, part ID = %08X\n", __func__, status, partid);
> + /* Is the partition already on? */
> + if (!is_partition_powered(status)) {
Shouldn't this pass along the partid as well? Otherwise, how does the
is_partition_powered() function know what to check for. Perhaps I'm
confused by the API here. Why do we even pass around status in the first
place? The partition ID should be enough, shouldn't it?
> + /* No, toggle the partition power state (OFF -> ON) */
> + debug("power_partition, toggling state\n");
> + clrbits_le32(&pmc->pmc_pwrgate_toggle, 0x1F);
> + setbits_le32(&pmc->pmc_pwrgate_toggle, partid);
> + setbits_le32(&pmc->pmc_pwrgate_toggle, START_CP);
The register has only two fields, so no need for read/modify/write. This
can be abbreviated to:
writel(START_CP | partid, &pmc->pmc_pwrgate_toggle);
> +void powerup_cpus(void)
> +{
> + debug("powerup_cpus entry\n");
> +
> + /* We boot to the fast cluster */
> + debug("powerup_cpus entry: G cluster\n");
> +
> + /* Power up the fast cluster rail partition */
> + debug("powerup_cpus: CRAIL\n");
> + power_partition(CRAIL, CRAILID);
Ah, I see now. I'd find something like the following more intuitive:
power_partition_set(CRAIL, true);
But looking deeper I see that we already have the same API for earlier
SoC generations, so I suppose we might as well stick with it.
The current API always has the risk of someone doing something like:
power_partition(CONC, CRAILID);
> +void start_cpu(u32 reset_vector)
> +{
[...]
> + /* Enable VDD_CPU */
> + enable_cpu_power_rail();
> +
> + /* Get the CPU(s) running */
> + enable_cpu_clocks();
> +
> + /* Enable CoreSight */
> + clock_enable_coresight(1);
> +
> + /* Take CPU(s) out of reset */
> + remove_cpu_resets();
> +
> + /* Set the entry point for CPU execution from reset */
> + writel(reset_vector, EXCEP_VECTOR_CPU_RESET_VECTOR);
> +
> + /* If the CPU(s) don't already have power, power 'em up */
> + powerup_cpus();
I don't think any of the comments above add value, so they can just as
well be dropped. The code is self-explanatory.
> +/*
> + * On poweron, AVP clock source (also called system clock) is set to PLLP_out0
Nit: "power on"
Thierry
-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information. Any unauthorized review, use, disclosure or distribution
is prohibited. If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20131008/c0a960a8/attachment.pgp>
More information about the U-Boot
mailing list