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 "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 > _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot