[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