Re: [U-Boot] [PATCH] arm: Tegra2: add support for A9 CPU init
Peter, On Thu, Mar 17, 2011 at 7:32 AM, Peter Tyser pty...@xes-inc.com wrote: Hi Tom, + /* Is PLL-X already running? */ + reg = readl(clkrst-crc_pllx_base); + if (reg PLL_ENABLE) + return; + + /* Do PLLX init if it isn't running, but BootROM sets it, so TBD */ +} The above function looks incorrect. What looks incorrect? It checks to see if the PLL is already running/enabled, and returns if it is. Tegra2 mask ROM (BootROM) currently always sets PLLX, so this will always return, but the comment is there for future chips that may not set up PLLX. It looks like a function that does a read of a value it doesn't care about, does an unnecessary comparison, and then returns either way, without doing anything:) If/when you want to do future stuff with the PLL-X, code should be added then - as is it doesn't do anything and is confusing. The general policy of U-Boot is not to add dead code. OK, so not really incorrect, just unnecessary. Agreed - again a vestigial leftover from our in-house code. I'll remove it. Unnecessary/misleading/dead code is inherently not correct:) snip +#include asm/types.h + +#ifndef FALSE +#define FALSE 0 +#endif +#ifndef TRUE +#define TRUE 1 +#endif These shouldn't be added here. They should be somewhere common, or shouldn't be used (my preference). I would think they'd be in the ARM tree somewhere, but I couldn't find them so I added 'em here. My preference is to use TRUE/FALSE - it carries more context than 1/0 to me. If you prefer to use TRUE/FALSE, they should be moved into a common location so everywhere uses the same, once-defined definition. Their definitions are already littered over a few files, which would ideally be cleaned up. Perhaps moving all definitions into include/common.h, or somewhere similar would work. Others may have opinions about TRUE/FALSE vs 1/0 - it seems like TRUE/FALSE aren't generally used. I don't want to pollute all builds by adding to include/common.h. I'll try to find a more central header in my own tree. My point is that there are already 32 definitions of TRUE/FALSE - adding a 33rd doesn't seem like a good thing to do. I view a 33rd identical definition as polluting the code more than 1 common definition that most people won't generally use. Its not my decision, but I assume the powers that be would recommend one of: - Not using TRUE/FALSE since using non-zero values and 0 are widely accepted as TRUE/FALSE. I think using TRUE/FALSE makes the code harder to understand and more open to bugs. Eg for other code that interacts with your code, or someone reviewing your code, they either have to either assume you defined TRUE as 1, FALSE as 0, or import your definitions. Anyway, I view their use as another layer of unnecessary abstraction with very little benefit. I've removed both the defines and the use of TRUE/FALSE in ap20.* - there were only a few instances. - Consolidating the definition of TRUE/FALSE. Wolfgang, do you have a preference about how TRUE/FALSE are generally used/defined? Best, Peter Thanks, Tom ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] arm: Tegra2: add support for A9 CPU init
Allesandro, On Thu, Mar 17, 2011 at 8:30 AM, Alessandro Rubini rubini-l...@gnudd.com wrote: It looks like most of your uses are standalone functions that would function just fine on their own. Is there a reason you prefer to have them in a C-file instead of in an assembly file? Just laziness ;) I'll move these to a new .S file in the next patchset. Actually, writing assembly-only C functions is difficul and error-pronet. I've seen you use r0 and other registers esplicitly, but this is not allowed in general. I once wasted some hours in tracking why a non-submitted port of u-boot was not working with a newer compiler. The problem was just that: the new compiler was inlining a void(void) function; the asm used r0 and r1 explicitly, which worked over a function call but was corrupting data when inlined by the newer and more optimizing compiler. I've moved most of the in-line assembly to lowlevel_init.S. However, the cold_boot() code proved difficult - I got fixup errors, undefined constants, etc. I've cleaned it up a bit, but left it in ap20.c, using the %vars - again, replacing them with the actual values caused errors that I couldn't resolve. Maybe someone else on the list can help to port this code to the assembly file - I'm not expert enough in gcc/as and ARM to do it. New patch to follow RSN. While your functions are currently not inlined (or, like cold_boot, they may be inlined in a place where no register needs to be preserved), another user may move them to a context where the semantics are different, for another board or another boot loader. If they are in a .S files, they will only be called by bl and you know the rules for register allocation appy. Besides, a _real_ lazy programmer avoids the extra quotes and \n in the code :) /alessandro Thanks for your insight, Tom ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] arm: Tegra2: add support for A9 CPU init
Hi Tom, + /* Is PLL-X already running? */ + reg = readl(clkrst-crc_pllx_base); + if (reg PLL_ENABLE) + return; + + /* Do PLLX init if it isn't running, but BootROM sets it, so TBD */ +} The above function looks incorrect. What looks incorrect? It checks to see if the PLL is already running/enabled, and returns if it is. Tegra2 mask ROM (BootROM) currently always sets PLLX, so this will always return, but the comment is there for future chips that may not set up PLLX. It looks like a function that does a read of a value it doesn't care about, does an unnecessary comparison, and then returns either way, without doing anything:) If/when you want to do future stuff with the PLL-X, code should be added then - as is it doesn't do anything and is confusing. The general policy of U-Boot is not to add dead code. OK, so not really incorrect, just unnecessary. Agreed - again a vestigial leftover from our in-house code. I'll remove it. Unnecessary/misleading/dead code is inherently not correct:) snip +#include asm/types.h + +#ifndef FALSE +#define FALSE0 +#endif +#ifndef TRUE +#define TRUE 1 +#endif These shouldn't be added here. They should be somewhere common, or shouldn't be used (my preference). I would think they'd be in the ARM tree somewhere, but I couldn't find them so I added 'em here. My preference is to use TRUE/FALSE - it carries more context than 1/0 to me. If you prefer to use TRUE/FALSE, they should be moved into a common location so everywhere uses the same, once-defined definition. Their definitions are already littered over a few files, which would ideally be cleaned up. Perhaps moving all definitions into include/common.h, or somewhere similar would work. Others may have opinions about TRUE/FALSE vs 1/0 - it seems like TRUE/FALSE aren't generally used. I don't want to pollute all builds by adding to include/common.h. I'll try to find a more central header in my own tree. My point is that there are already 32 definitions of TRUE/FALSE - adding a 33rd doesn't seem like a good thing to do. I view a 33rd identical definition as polluting the code more than 1 common definition that most people won't generally use. Its not my decision, but I assume the powers that be would recommend one of: - Not using TRUE/FALSE since using non-zero values and 0 are widely accepted as TRUE/FALSE. I think using TRUE/FALSE makes the code harder to understand and more open to bugs. Eg for other code that interacts with your code, or someone reviewing your code, they either have to either assume you defined TRUE as 1, FALSE as 0, or import your definitions. Anyway, I view their use as another layer of unnecessary abstraction with very little benefit. - Consolidating the definition of TRUE/FALSE. Wolfgang, do you have a preference about how TRUE/FALSE are generally used/defined? Best, Peter ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] arm: Tegra2: add support for A9 CPU init
It looks like most of your uses are standalone functions that would function just fine on their own. Is there a reason you prefer to have them in a C-file instead of in an assembly file? Just laziness ;) I'll move these to a new .S file in the next patchset. Actually, writing assembly-only C functions is difficul and error-pronet. I've seen you use r0 and other registers esplicitly, but this is not allowed in general. I once wasted some hours in tracking why a non-submitted port of u-boot was not working with a newer compiler. The problem was just that: the new compiler was inlining a void(void) function; the asm used r0 and r1 explicitly, which worked over a function call but was corrupting data when inlined by the newer and more optimizing compiler. While your functions are currently not inlined (or, like cold_boot, they may be inlined in a place where no register needs to be preserved), another user may move them to a context where the semantics are different, for another board or another boot loader. If they are in a .S files, they will only be called by bl and you know the rules for register allocation appy. Besides, a _real_ lazy programmer avoids the extra quotes and \n in the code :) /alessandro ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] arm: Tegra2: add support for A9 CPU init
Hi Tom,
I'm not too familiar with the architecture, so many comments are
aesthetic. It would be a good idea to run the patch through
checkpatch.pl too.
diff --git a/arch/arm/cpu/armv7/start.S b/arch/arm/cpu/armv7/start.S
index 684f2d2..50a1725 100644
--- a/arch/arm/cpu/armv7/start.S
+++ b/arch/arm/cpu/armv7/start.S
@@ -70,6 +70,12 @@ _end_vect:
_TEXT_BASE:
.word CONFIG_SYS_TEXT_BASE
+#ifdef CONFIG_TEGRA2
+.globl _armboot_start
+_armboot_start:
+.word _start
+#endif
+
It'd be good to add a comment about what's going on above, and why its
tegra2-specific. Eg why is it needed?
snip
+static void init_pll_x(void)
+{
+ struct clk_rst_ctlr *clkrst = (struct clk_rst_ctlr *)NV_PA_CLK_RST_BASE;
+ u32 reg;
The spaces in front of reg can be removed. Ditto for all
functions/variables.
+ /* Is PLL-X already running? */
+ reg = readl(clkrst-crc_pllx_base);
+ if (reg PLL_ENABLE)
+ return;
+
+ /* Do PLLX init if it isn't running, but BootROM sets it, so TBD */
+}
The above function looks incorrect.
+static void set_cpu_reset_vector(u32 vector)
+{
+ writel(vector, EXCEP_VECTOR_CPU_RESET_VECTOR);
+}
Is it worth breaking this out into its own function? The define names
make it pretty clear what is being done, and its only called from 1
location.
+static void enable_cpu_clock(int enable)
+{
+ struct clk_rst_ctlr *clkrst = (struct clk_rst_ctlr *)NV_PA_CLK_RST_BASE;
+ u32 reg, clk;
+
+ /*
+ * NOTE:
+ * Regardless of whether the request is to enable or disable the CPU
+ * clock, every processor in the CPU complex except the master (CPU 0)
+ * will have it's clock stopped because the AVP only talks to the
+ * master. The AVP does not know (nor does it need to know) that there
+ * are multiple processors in the CPU complex.
+ */
+
+ /* Need to initialize PLLX? */
+ if (enable) {
+ /* Initialize PLL */
+ init_pll_x();
+
+ /* Wait until stable */
+ udelay(NVBOOT_CLOCKS_PLL_STABILIZATION_DELAY);
+
+ reg = CCLK_BURST_POLICY;
+ writel(reg, clkrst-crc_cclk_brst_pol);
It'd look cleaner to not set reg for each access, just use the define
directly. I'd personally use less temporary variables in general and
only use them when it made the code cleaner and easier to understand.
+ reg = SUPER_CCLK_DIVIDER;
+ writel(reg, clkrst-crc_super_cclk_div);
+ }
+
+ /* Fetch the register containing the main CPU complex clock enable */
+ reg = readl(clkrst-crc_clk_out_enb_l);
Is this read necessary? You overwrite reg unconditionally below.
+ /*
+ * Read the register containing the individual CPU clock enables and
+ * always stop the clock to CPU 1.
+ */
+ clk = readl(clkrst-crc_clk_cpu_cmplx);
+ clk |= CPU1_CLK_STP;
+
+ if (enable) {
+ /* Enable the CPU clock */
+ reg = readl(clkrst-crc_clk_out_enb_l);
+ reg |= CLK_ENB_CPU;
+ clk = readl(clkrst-crc_clk_cpu_cmplx);
+ clk = ~CPU0_CLK_STP;
+ } else {
+ /* Disable the CPU clock */
+ reg = readl(clkrst-crc_clk_out_enb_l);
+ reg |= CLK_ENB_CPU;
+ clk = readl(clkrst-crc_clk_cpu_cmplx);
+ clk |= CPU0_CLK_STP;
+ }
For if/elses that share common code, the common code should be broken
out. eg above only the one-line |=/= operations should be conditional.
+ writel(clk, clkrst-crc_clk_cpu_cmplx);
+ writel(reg, clkrst-crc_clk_out_enb_l);
+}
+
+static int is_cpu_powered(void)
+{
+ struct pmc_ctlr *pmc = (struct pmc_ctlr *)NV_PA_PMC_BASE;
+ u32 reg;
+
+ reg = readl(pmc-pmc_pwrgate_status);
+
+ if (reg CPU_PWRED)
+ return TRUE;
I'd remove the reg variable. eg something like:
return readl(pmc-pmc_pwrgate_status CPU_PWRED ? 1 : 0);
+ return FALSE;
+}
+
+static void remove_cpu_io_clamps(void)
+{
+ struct pmc_ctlr *pmc = (struct pmc_ctlr *)NV_PA_PMC_BASE;
+ u32 reg;
+
+ /* Remove the clamps on the CPU I/O signals */
+ reg = readl(pmc-pmc_remove_clamping);
+ reg |= CPU_CLMP;
+ writel(reg, pmc-pmc_remove_clamping);
+
+ /* Give I/O signals time to stabilize */
+ udelay(1000);
For magic delays it'd be good to reference why you're waiting a specific
amount of time. Does a manual state 1 ms? Did testing show 1ms was
required?
+}
+
+static void powerup_cpu(void)
+{
+ struct pmc_ctlr *pmc = (struct pmc_ctlr *)NV_PA_PMC_BASE;
+ u32 reg;
+
+ if (!is_cpu_powered()) {
+ /* Toggle the CPU power state (OFF - ON) */
+ reg = readl(pmc-pmc_pwrgate_toggle);
+ reg = (PARTID_CP);
+ reg |= START_CP;
Why ()'s on one operation, but not the other?
+ writel(reg,
Re: [U-Boot] [PATCH] arm: Tegra2: add support for A9 CPU init
Albert,
On Sun, Mar 13, 2011 at 10:46 AM, Albert ARIBAUD albert.arib...@free.fr wrote:
Le 16/02/2011 21:26, Tom Warren a écrit :
Signed-off-by: Tom Warrentwar...@nvidia.com
---
arch/arm/cpu/armv7/start.S | 6 +
arch/arm/cpu/armv7/tegra2/Makefile | 2 +-
arch/arm/cpu/armv7/tegra2/ap20.c | 490
This one has an extra empty line at end of file.
Thanks - I'll remove it.
+void cold_boot(void)
+{
+ asm volatile(
+
+ msr cpsr_c, #0xd3 \n
+ /*
+ * Check current processor: CPU or AVP?
+ * If AVP, go to AVP boot code, else continue on.
+ */
+ mov r0, %0 \n
+ ldrb r2, [r0, %1] \n
+ /* are we the CPU? */
+ cmp r2, %2 \n
+ mov sp, %3 \n
+ /* leave in some symbols for release debugging */
+ mov r3, %6 \n
+ str r3, [sp, #-4]! \n
+ str r3, [sp, #-4]! \n
+ /* yep, we are the CPU */
+ bxeq %4 \n
+ /* AVP Initialization follows this path */
+ mov sp, %5 \n
+ /* leave in some symbols for release debugging */
+ mov r3, %6 \n
+ str r3, [sp, #-4]! \n
+ str r3, [sp, #-4]! \n
+
+ /* Init and Start CPU */
+ b startup_cpu \n
+ :
+ : i(NV_PA_PG_UP_BASE),
If I'm not mistaken, NV_PA_PG_UP_BASE could be used just as well directly in
the asm statement instead of via %0 and i(), as anyway the asm will be
preprocessed and the macro will turn to a number. That would simplify the
asm instruction as a whole and make the asm statement more understandable
(also applies to other macros used similarly).
Yeah, this code came this way from our bringup group for out in-house
bootloader, and it's always been confusing.
I'll try to simplify it when I incorporate Peter's critiques. Thanks.
Apart from that, I must admit I don't know the Tegra2/A9 well enough to
comment further.
amicalement,
Amicalement,
--
Albert.
Thanks for the input,
Tom
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] arm: Tegra2: add support for A9 CPU init
Peter,
On Mon, Mar 14, 2011 at 8:33 AM, Peter Tyser pty...@xes-inc.com wrote:
Hi Tom,
I'm not too familiar with the architecture, so many comments are
aesthetic. It would be a good idea to run the patch through
checkpatch.pl too.
I run checkpatch on every submission. Only 1 warning was generated
(about an extern), and no errors.
diff --git a/arch/arm/cpu/armv7/start.S b/arch/arm/cpu/armv7/start.S
index 684f2d2..50a1725 100644
--- a/arch/arm/cpu/armv7/start.S
+++ b/arch/arm/cpu/armv7/start.S
@@ -70,6 +70,12 @@ _end_vect:
_TEXT_BASE:
.word CONFIG_SYS_TEXT_BASE
+#ifdef CONFIG_TEGRA2
+.globl _armboot_start
+_armboot_start:
+ .word _start
+#endif
+
It'd be good to add a comment about what's going on above, and why its
tegra2-specific. Eg why is it needed?
Tegra uses 2 separate CPUs - the AVP (ARM7TDI) and the dual-core A9. A
bootloader first runs on the AVP and sets things up for the CPU
complex to take over.
The CPU complex jumps here during it's init phase/boot (from it's
reset vector). It needs a dereferenced jump to the _start code in
start.S (which in turn chains to the reset code).
I'll add a comment about this in the next patch.
snip
+static void init_pll_x(void)
+{
+ struct clk_rst_ctlr *clkrst = (struct clk_rst_ctlr
*)NV_PA_CLK_RST_BASE;
+ u32 reg;
The spaces in front of reg can be removed. Ditto for all
functions/variables.
Not sure what you mean, here, Peter. There are no spaces here in my
ap20.c file, just tabs.
Please clarify.
+ /* Is PLL-X already running? */
+ reg = readl(clkrst-crc_pllx_base);
+ if (reg PLL_ENABLE)
+ return;
+
+ /* Do PLLX init if it isn't running, but BootROM sets it, so TBD */
+}
The above function looks incorrect.
What looks incorrect? It checks to see if the PLL is already
running/enabled, and returns if it is.
Tegra2 mask ROM (BootROM) currently always sets PLLX, so this will
always return, but the comment is there for future chips that may not
set up PLLX.
+static void set_cpu_reset_vector(u32 vector)
+{
+ writel(vector, EXCEP_VECTOR_CPU_RESET_VECTOR);
+}
Is it worth breaking this out into its own function? The define names
make it pretty clear what is being done, and its only called from 1
location.
I can move the writel() to the called location and remove this func.
+static void enable_cpu_clock(int enable)
+{
+ struct clk_rst_ctlr *clkrst = (struct clk_rst_ctlr
*)NV_PA_CLK_RST_BASE;
+ u32 reg, clk;
+
+ /*
+ * NOTE:
+ * Regardless of whether the request is to enable or disable the CPU
+ * clock, every processor in the CPU complex except the master (CPU 0)
+ * will have it's clock stopped because the AVP only talks to the
+ * master. The AVP does not know (nor does it need to know) that there
+ * are multiple processors in the CPU complex.
+ */
+
+ /* Need to initialize PLLX? */
+ if (enable) {
+ /* Initialize PLL */
+ init_pll_x();
+
+ /* Wait until stable */
+ udelay(NVBOOT_CLOCKS_PLL_STABILIZATION_DELAY);
+
+ reg = CCLK_BURST_POLICY;
+ writel(reg, clkrst-crc_cclk_brst_pol);
It'd look cleaner to not set reg for each access, just use the define
directly. I'd personally use less temporary variables in general and
only use them when it made the code cleaner and easier to understand.
Will do. This code was ported from our orignal, in-house bootloader,
so it's going to be a bit hairy.
+ reg = SUPER_CCLK_DIVIDER;
+ writel(reg, clkrst-crc_super_cclk_div);
+ }
+
+ /* Fetch the register containing the main CPU complex clock enable */
+ reg = readl(clkrst-crc_clk_out_enb_l);
Is this read necessary? You overwrite reg unconditionally below.
Yeah, I saw that and did a rewrite a week or two ago, but was waiting
to incorporate into the next patch to get everything in one place.
+ /*
+ * Read the register containing the individual CPU clock enables and
+ * always stop the clock to CPU 1.
+ */
+ clk = readl(clkrst-crc_clk_cpu_cmplx);
+ clk |= CPU1_CLK_STP;
+
+ if (enable) {
+ /* Enable the CPU clock */
+ reg = readl(clkrst-crc_clk_out_enb_l);
+ reg |= CLK_ENB_CPU;
+ clk = readl(clkrst-crc_clk_cpu_cmplx);
+ clk = ~CPU0_CLK_STP;
+ } else {
+ /* Disable the CPU clock */
+ reg = readl(clkrst-crc_clk_out_enb_l);
+ reg |= CLK_ENB_CPU;
+ clk = readl(clkrst-crc_clk_cpu_cmplx);
+ clk |= CPU0_CLK_STP;
+ }
For if/elses that share common code, the common code should be broken
out. eg above only the one-line |=/= operations should be conditional.
Yep, already done as part of the previous cleanup. Just need to put it
into the full patchset #2.
+ writel(clk, clkrst-crc_clk_cpu_cmplx);
+
Re: [U-Boot] [PATCH] arm: Tegra2: add support for A9 CPU init
Hi Tom,
snip
+static void init_pll_x(void)
+{
+ struct clk_rst_ctlr *clkrst = (struct clk_rst_ctlr
*)NV_PA_CLK_RST_BASE;
+ u32 reg;
The spaces in front of reg can be removed. Ditto for all
functions/variables.
Not sure what you mean, here, Peter. There are no spaces here in my
ap20.c file, just tabs.
Please clarify.
My email client converted to spaces I was referring to the u32reg;
above. I think it should be u32spacereg instead of u32tabreg.
Other places in this patch spaces are used (eg in enable_cpu_clock, and
in the struct declaration above), so at a minimum the tabs/spaces should
be changed to be consistent, and typespacename seems cleanest to
me.
+ /* Is PLL-X already running? */
+ reg = readl(clkrst-crc_pllx_base);
+ if (reg PLL_ENABLE)
+ return;
+
+ /* Do PLLX init if it isn't running, but BootROM sets it, so TBD */
+}
The above function looks incorrect.
What looks incorrect? It checks to see if the PLL is already
running/enabled, and returns if it is.
Tegra2 mask ROM (BootROM) currently always sets PLLX, so this will
always return, but the comment is there for future chips that may not
set up PLLX.
It looks like a function that does a read of a value it doesn't care
about, does an unnecessary comparison, and then returns either way,
without doing anything:) If/when you want to do future stuff with the
PLL-X, code should be added then - as is it doesn't do anything and is
confusing. The general policy of U-Boot is not to add dead code.
snip
+static void enable_cpu_power_rail(void)
+{
+ struct pmc_ctlr *pmc = (struct pmc_ctlr *)NV_PA_PMC_BASE;
+ u32 reg;
+
+ reg = readl(pmc-pmc_cntrl);
+ reg |= CPUPWRREQ_OE;
+ writel(reg, pmc-pmc_cntrl);
+
+ /* Delay to allow for stable power (CPU_PWR_REQ - VDD_CPU from PMU)
*/
+ udelay(3750);
Ditto for description.
Ditto on why the delay? In this case, I did find a reference to the
time needed between the request from the chipset to the PMU, hence the
comment.
It'd be nice mention that reference in the comment. For those designing
boards around your chips, during debug they will look through the
initialization code and wonder I wonder if we need to delay longer, or
I'm not using the same power supply sequencer, I wonder if this might
be a problem. If you more explicitly state where the delay came from,
or what the delay specifically accomplishes, it saves others from having
to guess and investigate.
+}
+
+static void reset_A9_cpu(int reset)
+{
+ struct clk_rst_ctlr *clkrst = (struct clk_rst_ctlr
*)NV_PA_CLK_RST_BASE;
+ u32 reg, cpu;
+
+ /*
+ * NOTE: Regardless of whether the request is to hold the CPU in
reset
+ *or take it out of reset, every processor in the CPU complex
+ *except the master (CPU 0) will be held in reset because the
+ *AVP only talks to the master. The AVP does not know that
there
+ *are multiple processors in the CPU complex.
+ */
+
+ /* Hold CPU 1 in reset */
+ cpu = SET_DBGRESET1 | SET_DERESET1 | SET_CPURESET1;
+ writel(cpu, clkrst-crc_cpu_cmplx_set);
The cpu variable can be removed.
It could be, sure. But it makes the line longer, 80 chars, and hence
it would have to be broken into two lines.
Actually, now that I look at the code again, it appears that 'cpu'
later should be OR'd with the SET_/CLR_DBGRESET0, etc. bits, depending
on the state of 'reset'.
I'll give it a rewrite for the next patch.
Its a matter of preference, but is acceptable either way. I think:
+ writel(SET_DBGRESET1 | SET_DERESET1 | SET_CPURESET1,
+ clkrst-crc_cpu_cmplx_set);
makes it clearer what is going on. Setting 'cpu', then writing would
imply to me that 'cpu' has some additional purpose, or is being used
later. If you restructure the code, this comment will likely be moot.
snip
+ if (enable) {
+ /*
+ * Put CoreSight on PLLP_OUT0 (216 MHz) and divide it down by
+ * 1.5, giving an effective frequency of 144MHz.
+ * Set PLLP_OUT0 [bits31:30 = 00], and use a 7.1 divisor
+ * (bits 7:0), so 0001b == 1.5 (n+1 + .5)
+ */
+ src = CLK_DIVIDER(NVBL_PLLP_KHZ, 144000);
+ writel(src, clkrst-crc_clk_src_csite);
+
+ /* Unlock the CPU CoreSight interfaces */
+ rst = 0xC5ACCE55;
What's this number? Is there a define that could be used instead?
Sure, I can add a #define. Some magic ARM access sequence to unlock
the CoreSight I/F, as the comment says
Its not clear what the number is from the comment. Is it a bitmask
where each bit has some significance? Or is it just a magic number of
some sort? If its a magic number with no other significance, a more
verbose comment is fine stating so without
Re: [U-Boot] [PATCH] arm: Tegra2: add support for A9 CPU init
Peter,
On Mon, Mar 14, 2011 at 3:20 PM, Peter Tyser pty...@xes-inc.com wrote:
Hi Tom,
snip
+static void init_pll_x(void)
+{
+ struct clk_rst_ctlr *clkrst = (struct clk_rst_ctlr *)NV_PA_CLK_RST
_BASE;
+ u32 reg;
The spaces in front of reg can be removed. Ditto for all
functions/variables.
Not sure what you mean, here, Peter. There are no spaces here in my
ap20.c file, just tabs.
Please clarify.
My email client converted to spaces I was referring to the u32 reg;
above. I think it should be u32spacereg instead of u32tabreg.
Other places in this patch spaces are used (eg in enable_cpu_clock, and
in the struct declaration above), so at a minimum the tabs/spaces should
be changed to be consistent, and typespacename seems cleanest to
me.
I see. I can change to typespacename globally, no problem.
+ /* Is PLL-X already running? */
+ reg = readl(clkrst-crc_pllx_base);
+ if (reg PLL_ENABLE)
+ return;
+
+ /* Do PLLX init if it isn't running, but BootROM sets it, so TBD */
+}
The above function looks incorrect.
What looks incorrect? It checks to see if the PLL is already
running/enabled, and returns if it is.
Tegra2 mask ROM (BootROM) currently always sets PLLX, so this will
always return, but the comment is there for future chips that may not
set up PLLX.
It looks like a function that does a read of a value it doesn't care
about, does an unnecessary comparison, and then returns either way,
without doing anything:) If/when you want to do future stuff with the
PLL-X, code should be added then - as is it doesn't do anything and is
confusing. The general policy of U-Boot is not to add dead code.
OK, so not really incorrect, just unnecessary. Agreed - again a
vestigial leftover from our in-house code. I'll remove it.
snip
+static void enable_cpu_power_rail(void)
+{
+ struct pmc_ctlr *pmc = (struct pmc_ctlr *)NV_PA_PMC_BASE;
+ u32 reg;
+
+ reg = readl(pmc-pmc_cntrl);
+ reg |= CPUPWRREQ_OE;
+ writel(reg, pmc-pmc_cntrl);
+
+ /* Delay to allow for stable power (CPU_PWR_REQ - VDD_CPU from
PMU) */
+ udelay(3750);
Ditto for description.
Ditto on why the delay? In this case, I did find a reference to the
time needed between the request from the chipset to the PMU, hence the
comment.
It'd be nice mention that reference in the comment. For those designing
boards around your chips, during debug they will look through the
initialization code and wonder I wonder if we need to delay longer, or
I'm not using the same power supply sequencer, I wonder if this might
be a problem. If you more explicitly state where the delay came from,
or what the delay specifically accomplishes, it saves others from having
to guess and investigate.
OK, I'll expand the comment.
+}
+
+static void reset_A9_cpu(int reset)
+{
+ struct clk_rst_ctlr *clkrst = (struct clk_rst_ctlr
*)NV_PA_CLK_RST_BASE;
+ u32 reg, cpu;
+
+ /*
+ * NOTE: Regardless of whether the request is to hold the CPU in
reset
+ * or take it out of reset, every processor in the CPU complex
+ * except the master (CPU 0) will be held in reset because the
+ * AVP only talks to the master. The AVP does not know that
there
+ * are multiple processors in the CPU complex.
+ */
+
+ /* Hold CPU 1 in reset */
+ cpu = SET_DBGRESET1 | SET_DERESET1 | SET_CPURESET1;
+ writel(cpu, clkrst-crc_cpu_cmplx_set);
The cpu variable can be removed.
It could be, sure. But it makes the line longer, 80 chars, and hence
it would have to be broken into two lines.
Actually, now that I look at the code again, it appears that 'cpu'
later should be OR'd with the SET_/CLR_DBGRESET0, etc. bits, depending
on the state of 'reset'.
I'll give it a rewrite for the next patch.
Its a matter of preference, but is acceptable either way. I think:
+ writel(SET_DBGRESET1 | SET_DERESET1 | SET_CPURESET1,
+ clkrst-crc_cpu_cmplx_set);
makes it clearer what is going on. Setting 'cpu', then writing would
imply to me that 'cpu' has some additional purpose, or is being used
later. If you restructure the code, this comment will likely be moot.
snip
+ if (enable) {
+ /*
+ * Put CoreSight on PLLP_OUT0 (216 MHz) and divide it down
by
+ * 1.5, giving an effective frequency of 144MHz.
+ * Set PLLP_OUT0 [bits31:30 = 00], and use a 7.1 divisor
+ * (bits 7:0), so 0001b == 1.5 (n+1 + .5)
+ */
+ src = CLK_DIVIDER(NVBL_PLLP_KHZ, 144000);
+ writel(src, clkrst-crc_clk_src_csite);
+
+ /* Unlock the CPU CoreSight interfaces */
+ rst = 0xC5ACCE55;
What's this number? Is there a define that could be used instead?
Sure, I can add a #define. Some magic
Re: [U-Boot] [PATCH] arm: Tegra2: add support for A9 CPU init
Albert, On Tue, Feb 22, 2011 at 4:57 PM, Albert ARIBAUD albert.arib...@free.fr wrote: Hi Tom, Le 23/02/2011 00:41, Tom Warren a écrit : Anyone willing to review this? I'd like to get it pulled in to Albert's arm repo ASAP. I should be able to review it during the week-end--not before, sorry. Any chance you can give this some bandwidth? I'd like to get it into the arm tree. As far as I know, it should still apply, but let me know if I need to rebase it against next (or master). Thanks, Tom Note however that since this is not a bugfix and it came after the merge window closed, it would go to u-boot-arm/next, not /master; it will go to u-boot-arm/master after the the upcoming release (and then to u-boot/master when the next merge window closes). Thanks, Tom Amicalement, -- Albert. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] arm: Tegra2: add support for A9 CPU init
Albert, On Tue, Feb 22, 2011 at 4:57 PM, Albert ARIBAUD albert.arib...@free.fr wrote: Hi Tom, Le 23/02/2011 00:41, Tom Warren a écrit : Anyone willing to review this? I'd like to get it pulled in to Albert's arm repo ASAP. I should be able to review it during the week-end--not before, sorry. Thank you. That would be fine - no rush. Note however that since this is not a bugfix and it came after the merge window closed, it would go to u-boot-arm/next, not /master; it will go to u-boot-arm/master after the the upcoming release (and then to u-boot/master when the next merge window closes). Understood. Just wanted an ACK, etc. so I can sign off on the CPU work and move on to the drivers. Thanks, Tom Amicalement, -- Albert. Thanks for your help, Tom ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] arm: Tegra2: add support for A9 CPU init
Anyone willing to review this? I'd like to get it pulled in to
Albert's arm repo ASAP.
Thanks,
Tom
On Wed, Feb 16, 2011 at 1:26 PM, Tom Warren twarren.nvi...@gmail.com wrote:
Signed-off-by: Tom Warren twar...@nvidia.com
---
arch/arm/cpu/armv7/start.S | 6 +
arch/arm/cpu/armv7/tegra2/Makefile | 2 +-
arch/arm/cpu/armv7/tegra2/ap20.c | 490
arch/arm/cpu/armv7/tegra2/ap20.h | 105 ++
arch/arm/include/asm/arch-tegra2/clk_rst.h | 27 ++
arch/arm/include/asm/arch-tegra2/pmc.h | 8 +
arch/arm/include/asm/arch-tegra2/scu.h | 43 +++
arch/arm/include/asm/arch-tegra2/tegra2.h | 5 +
board/nvidia/common/board.c | 10 +
board/nvidia/common/board.h | 29 ++
include/configs/harmony.h | 1 +
include/configs/seaboard.h | 1 +
include/configs/tegra2-common.h | 2 +
13 files changed, 728 insertions(+), 1 deletions(-)
create mode 100644 arch/arm/cpu/armv7/tegra2/ap20.c
create mode 100644 arch/arm/cpu/armv7/tegra2/ap20.h
create mode 100644 arch/arm/include/asm/arch-tegra2/scu.h
create mode 100644 board/nvidia/common/board.h
diff --git a/arch/arm/cpu/armv7/start.S b/arch/arm/cpu/armv7/start.S
index 684f2d2..50a1725 100644
--- a/arch/arm/cpu/armv7/start.S
+++ b/arch/arm/cpu/armv7/start.S
@@ -70,6 +70,12 @@ _end_vect:
_TEXT_BASE:
.word CONFIG_SYS_TEXT_BASE
+#ifdef CONFIG_TEGRA2
+.globl _armboot_start
+_armboot_start:
+ .word _start
+#endif
+
/*
* These are defined in the board-specific linker script.
*/
diff --git a/arch/arm/cpu/armv7/tegra2/Makefile
b/arch/arm/cpu/armv7/tegra2/Makefile
index 687c887..f1ea915 100644
--- a/arch/arm/cpu/armv7/tegra2/Makefile
+++ b/arch/arm/cpu/armv7/tegra2/Makefile
@@ -28,7 +28,7 @@ include $(TOPDIR)/config.mk
LIB = $(obj)lib$(SOC).o
SOBJS := lowlevel_init.o
-COBJS := board.o sys_info.o timer.o
+COBJS := ap20.o board.o sys_info.o timer.o
SRCS := $(SOBJS:.o=.S) $(COBJS:.o=.c)
OBJS := $(addprefix $(obj),$(COBJS) $(SOBJS))
diff --git a/arch/arm/cpu/armv7/tegra2/ap20.c
b/arch/arm/cpu/armv7/tegra2/ap20.c
new file mode 100644
index 000..89d0d5e
--- /dev/null
+++ b/arch/arm/cpu/armv7/tegra2/ap20.c
@@ -0,0 +1,490 @@
+/*
+* (C) Copyright 2010-2011
+* NVIDIA Corporation www.nvidia.com
+*
+* See file CREDITS for list of people who contributed to this
+* project.
+*
+* This program is free software; you can redistribute it and/or
+* modify it under the terms of the GNU General Public License as
+* published by the Free Software Foundation; either version 2 of
+* the License, or (at your option) any later version.
+*
+* This program is distributed in the hope that it will be useful,
+* but WITHOUT ANY WARRANTY; without even the implied warranty of
+* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+* GNU General Public License for more details.
+*
+* You should have received a copy of the GNU General Public License
+* along with this program; if not, write to the Free Software
+* Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+* MA 02111-1307 USA
+*/
+
+#include ap20.h
+#include asm/io.h
+#include asm/arch/tegra2.h
+#include asm/arch/clk_rst.h
+#include asm/arch/pmc.h
+#include asm/arch/pinmux.h
+#include asm/arch/scu.h
+#include common.h
+
+static void init_pll_x(void)
+{
+ struct clk_rst_ctlr *clkrst = (struct clk_rst_ctlr
*)NV_PA_CLK_RST_BASE;
+ u32 reg;
+
+ /* Is PLL-X already running? */
+ reg = readl(clkrst-crc_pllx_base);
+ if (reg PLL_ENABLE)
+ return;
+
+ /* Do PLLX init if it isn't running, but BootROM sets it, so TBD */
+}
+
+static void set_cpu_reset_vector(u32 vector)
+{
+ writel(vector, EXCEP_VECTOR_CPU_RESET_VECTOR);
+}
+
+static void enable_cpu_clock(int enable)
+{
+ struct clk_rst_ctlr *clkrst = (struct clk_rst_ctlr
*)NV_PA_CLK_RST_BASE;
+ u32 reg, clk;
+
+ /*
+ * NOTE:
+ * Regardless of whether the request is to enable or disable the CPU
+ * clock, every processor in the CPU complex except the master (CPU 0)
+ * will have it's clock stopped because the AVP only talks to the
+ * master. The AVP does not know (nor does it need to know) that there
+ * are multiple processors in the CPU complex.
+ */
+
+ /* Need to initialize PLLX? */
+ if (enable) {
+ /* Initialize PLL */
+ init_pll_x();
+
+ /* Wait until stable */
+ udelay(NVBOOT_CLOCKS_PLL_STABILIZATION_DELAY);
+
+ reg = CCLK_BURST_POLICY;
+ writel(reg, clkrst-crc_cclk_brst_pol);
+
+ reg = SUPER_CCLK_DIVIDER;
+ writel(reg, clkrst-crc_super_cclk_div);
+ }
+
+ /* Fetch the register
Re: [U-Boot] [PATCH] arm: Tegra2: add support for A9 CPU init
Hi Tom, Le 23/02/2011 00:41, Tom Warren a écrit : Anyone willing to review this? I'd like to get it pulled in to Albert's arm repo ASAP. I should be able to review it during the week-end--not before, sorry. Note however that since this is not a bugfix and it came after the merge window closed, it would go to u-boot-arm/next, not /master; it will go to u-boot-arm/master after the the upcoming release (and then to u-boot/master when the next merge window closes). Thanks, Tom Amicalement, -- Albert. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
