Re: [U-Boot] [PATCH] arm: Tegra2: add support for A9 CPU init
Allesandro, On Thu, Mar 17, 2011 at 8:30 AM, Alessandro Rubini 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
Peter, On Thu, Mar 17, 2011 at 7:32 AM, Peter Tyser 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:) > > > >> >> >> +#include >> >> >> + >> >> >> +#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
>> 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, > >> > > >> >> + /* 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:) > >> >> +#include > >> >> + > >> >> +#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
Peter,
On Mon, Mar 14, 2011 at 3:20 PM, Peter Tyser wrote:
> Hi Tom,
>
>
>
>> >> +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 "u32reg" instead of "u32reg".
> 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 seems cleanest to
> me.
I see. I can change to 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.
>
>
>
>> >> +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.
>
>
>
>> >> + 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(NVB
Re: [U-Boot] [PATCH] arm: Tegra2: add support for A9 CPU init
Hi Tom,
> >> +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 "u32reg" instead of "u32reg".
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 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.
> >> +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.
> >> + 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 comm
Re: [U-Boot] [PATCH] arm: Tegra2: add support for A9 CPU init
Peter,
On Mon, Mar 14, 2011 at 8:33 AM, Peter Tyser 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.
>
>
>> +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
Re: [U-Boot] [PATCH] arm: Tegra2: add support for A9 CPU init
Albert,
On Sun, Mar 13, 2011 at 10:46 AM, Albert ARIBAUD wrote:
> Le 16/02/2011 21:26, Tom Warren a écrit :
>>
>> Signed-off-by: Tom Warren
>> ---
>> 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
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?
> +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_toggl
Re: [U-Boot] [PATCH] arm: Tegra2: add support for A9 CPU init
Le 16/02/2011 21:26, Tom Warren a écrit :
> Signed-off-by: Tom Warren
> ---
> 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.
> +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"
> + "ldrbr2, [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).
Apart from that, I must admit I don't know the Tegra2/A9 well enough to
comment further.
amicalement,
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 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 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
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
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 wrote:
> Signed-off-by: Tom Warren
> ---
> 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
> +*
> +* 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
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +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 conta
[U-Boot] [PATCH] arm: Tegra2: add support for A9 CPU init
Signed-off-by: Tom Warren
---
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
+*
+* 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
+#include
+#include
+#include
+#include
+#include
+#include
+
+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 containing the main CPU complex clock enable */
+ reg = readl(&clkrst->crc_clk_out_enb_l);
+
+ /*
+* 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_e
