[U-Boot] [PATCH] arm: Tegra2: add support for A9 CPU init
Tom Warren
twarren.nvidia at gmail.com
Tue Mar 15 00:08:26 CET 2011
Peter,
On Mon, Mar 14, 2011 at 3:20 PM, Peter Tyser <ptyser at 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 "u32<space>reg" instead of "u32<tab>reg".
> 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 <type><space><name> seems cleanest to
> me.
I see. I can change to <type><space><name> 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 00000001b == 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 adding a define.
OK, I'll expand the comment.
>
>> >
>> >> + writel(rst, CSITE_CPU_DBG0_LAR);
>> >> + writel(rst, CSITE_CPU_DBG1_LAR);
>> >> + }
>> >> +}
>> >> +
>> >> +void start_cpu(u32 reset_vector)
>> >> +{
>> >> + /* Enable VDD_CPU */
>> >> + enable_cpu_power_rail();
>> >> +
>> >> + /* Hold the CPUs in reset */
>> >> + reset_A9_cpu(TRUE);
>> >> +
>> >> + /* Disable the CPU clock */
>> >> + enable_cpu_clock(FALSE);
>> >> +
>> >> + /* Enable CoreSight */
>> >> + clock_enable_coresight(TRUE);
>> >> +
>> >> + /*
>> >> + * Set the entry point for CPU execution from reset,
>> >> + * if it's a non-zero value.
>> >> + */
>> >
>> > Spaces should be added above.
>> Spaces where? Please be more specific. Again, checkpatch had no
>> problem with this section.
>
> The multiline comment is not aligned:
> /*
> *
> *
> */
> should be
> /*
> *
> */
>
OK, now I see it. Will fix, thanks.
> <snip>
>
>> >> +void halt_avp(void)
>> >> +{
>> >> + u32 reg;
>> >> +
>> >> + for (;;) {
>> >> + reg = HALT_COP_EVENT_JTAG | HALT_COP_EVENT_IRQ_1 \
>> >> + | HALT_COP_EVENT_FIQ_1 | (FLOW_MODE_STOP<<29);
>> >> + writel(reg, FLOW_CTLR_HALT_COP_EVENTS);
>> >> + }
>> >> + /* Should never get here */
>> >> + for (;;)
>> >> + ;
>> >
>> > I'd remove the use of reg, and a secondary infinite loop seems
>> > necessary.
>> OK, I'll rewrite it.
>
> Sorry, should have said "unnecessary" instead of "necessary" in the
> original comment.
>
>> >> +void startup_cpu(void)
>> >> +{
>> >> + /* Initialize the AVP, clocks, and memory controller */
>> >> + /* SDRAM is guaranteed to be on at this point */
>> >> +
>> >> + asm volatile(
>> >> +
>> >> + /* Start the CPU */
>> >> + /* R0 = reset vector for CPU */
>> >> + "ldr r0, =cold_boot \n"
>> >> + "bl start_cpu \n"
>> >> +
>> >> + /* Transfer control to the AVP code */
>> >> + "bl halt_avp \n"
>> >> +
>> >> + /* Should never get here */
>> >> + "b . \n"
>> >> + );
>> >
>> > The assembly code should be indented. Actually, is there a reason not
>> > to move all these assembly functions into a seperate assembly file?
>> I can indent the asm code, no problem.
>> I don't see the need to move it to a .S file, though. Lots of examples
>> of embedded assembly code in the U-Boot tree.
>> Unless I see some strong objections, I'm just going to indent it (and
>> other asm statements).
>
> Agreed are lots of embedded assembly in C files, but they are generally
> small snippets that interact with surrounding C-code, or simple
> one-liners.
>
> Eg look in arch/powerpc:
> find grep -r asm arch/powerpc | grep vola | grep -v ";"
>
> There are only a handful of multi-line inline assembly references, lots
> are in headers, and the remaining generally interact with surrounding
> code.
>
> 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.
>
> <snip>
>
>> >
>> >> +}
>> >> +
>> >> +void cache_configure(void)
>> >> +{
>> >> + asm volatile(
>> >> +
>> >> + "stmdb r13!,{r14} \n"
>> >> + /* invalidate instruction cache */
>> >> + "mov r1, #0 \n"
>> >> + "mcr p15, 0, r1, c7, c5, 0 \n"
>> >> +
>> >> + /* invalidate the i&d tlb entries */
>> >> + "mcr p15, 0, r1, c8, c5, 0 \n"
>> >> + "mcr p15, 0, r1, c8, c6, 0 \n"
>> >> +
>> >> + /* enable instruction cache */
>> >> + "mrc p15, 0, r1, c1, c0, 0 \n"
>> >> + "orr r1, r1, #(1<<12) \n"
>> >> + "mcr p15, 0, r1, c1, c0, 0 \n"
>> >> +
>> >> + "bl enable_scu \n"
>> >> +
>> >> + /* enable SMP mode and FW for CPU0, by writing to Auxiliary Ctl reg */
>> >> + "mrc p15, 0, r0, c1, c0, 1 \n"
>> >> + "orr r0, r0, #0x41 \n"
>> >> + "mcr p15, 0, r0, c1, c0, 1 \n"
>> >> +
>> >> + /* Now flush the Dcache */
>> >> + "mov r0, #0 \n"
>> >> + /* 256 cache lines */
>> >> + "mov r1, #256 \n"
>> >> +
>> >> + "invalidate_loop: \n"
>> >> +
>> >> + "add r1, r1, #-1 \n"
>> >> + "mov r0, r1, lsl #5 \n"
>> >> + /* invalidate d-cache using line (way0) */
>> >> + "mcr p15, 0, r0, c7, c6, 2 \n"
>> >> +
>> >> + "orr r2, r0, #(1<<30) \n"
>> >> + /* invalidate d-cache using line (way1) */
>> >> + "mcr p15, 0, r2, c7, c6, 2 \n"
>> >> +
>> >> + "orr r2, r0, #(2<<30) \n"
>> >> + /* invalidate d-cache using line (way2) */
>> >> + "mcr p15, 0, r2, c7, c6, 2 \n"
>> >> +
>> >> + "orr r2, r0, #(3<<30) \n"
>> >> + /* invalidate d-cache using line (way3) */
>> >> + "mcr p15, 0, r2, c7, c6, 2 \n"
>> >> + "cmp r1, #0 \n"
>> >> + "bne invalidate_loop \n"
>> >> +
>> >> + /* FIXME: should have ap20's L2 disabled too */
>> >> + "invalidate_done: \n"
>> >> + "ldmia r13!,{pc} \n"
>> >> + ".ltorg \n"
>> >> + );
>> >
>> > Ditto on indentation/move.
>> I'll indent it.
>
> That's a pretty big and ugly inline assembly function... I'd push to
> have it in an assembly file along with the few other assembly-only
> functions, but its not my say ultimately.
No problem - I'll move to .S.
>
>> >
>> >> +}
>> >> +
>> >> +void init_pmc_scratch(void)
>> >> +{
>> >> + struct pmc_ctlr *const pmc = (struct pmc_ctlr *)NV_PA_PMC_BASE;
>> >> +
>> >> + /* SCRATCH0 is initialized by the boot ROM and shouldn't be cleared */
>> >> + memset(&pmc->pmc_scratch1, 0, 23*4);
>> >
>> > Is it safe to memset on IO regions here? A for loop of writel's would
>> > be safer, and more consistent.
>> The generated assembly looks OK. These are mem-mapped scratch
>> registers, so there're no side-effects here.
>> I can convert to a loop if you insist.
>
> The policy is if you are accessing registers, you should use proper IO
> access functions. The generated assembly and/or CPU may behave
> differently down the road, as others have ran into.
All right. I'll put in a loop of writel()'s.
>
> <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.
>
> < snip>
>
>> >> +/* ARM Snoop Control Unit (SCU) registers */
>> >> +struct scu_ctlr {
>> >> + uint scu_ctrl; /* SCU Control Register, offset 00 */
>> >> + uint scu_cfg; /* SCU Config Register, offset 04 */
>> >> + uint scu_cpu_pwr_stat; /* SCU CPU Power Status Register, offset 08 */
>> >> + uint scu_inv_all; /* SCU Invalidate All Register, offset 0C */
>> >> + uint scu_reserved0[12]; /* reserved, offset 10-3C */
>> >> + uint scu_filt_start; /* SCU Filtering Start Address Reg, offset 40 */
>> >> + uint scu_filt_end; /* SCU Filtering End Address Reg, offset 44 */
>> >> + uint scu_reserved1[2]; /* reserved, offset 48-4C */
>> >> + uint scu_acc_ctl; /* SCU Access Control Register, offset 50 */
>> >> + uint scu_ns_acc_ctl; /* SCU Non-secure Access Cntrl Reg, offset 54 */
>> >> +};
>> >> +
>> >> +#define SCU_ENABLE 1
>> >
>> > Should this be here? Seems like its not the proper location to enable
>> > things...
>> It's a bit setting, for the scu_ctrl register. I could change it to (1
>> << 0), if you'd prefer.
>> There are other bits in the SCU, but since they aren't used in this
>> code, I didn't declare 'em.
>
> Ahh, I get it - wasn't thinking of a bit definition. I think (1 << 0)
> would be clearer, or a comment, or the inclusion of the other SCU
> registers. If its for a specific register, naming it similarly would
> help too, eg "SCU_CTRL_ENABLE".
Good idea. I'll make it SCU_CTRL_ENABLE. Thanks.
>
> Best,
> Peter
Thanks,
Tom
>
More information about the U-Boot
mailing list