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.
> >
> >> + /* 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 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.
> >
> >> + 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
/*
*
*/
<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?
<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.
> >
> >> +}
> >> +
> >> +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.
<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.
< 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".
Best,
Peter
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
