Possible LPCOpen Clock powerdown bug?

[otsl]

I've been getting familiar with LPCOpen for the LPC18xx series.  It seems Chip_Clock_SetBaseClock() might have a bug.  The function documentation says:

BaseClock : CHIP_CGU_BASE_CLK_T value indicating which base clock to set
Input : CHIP_CGU_CLKIN_T value indicating which clock source to use or CLOCKINPUT_PD to power down base clock

However, in the function itself, it seems it only ever *tries* to power down a base clock when BaseClock >= CLK_BASE_NONE (which obviously wouldn't power down anything).  I'm not sure if this is by-design, or if the bracketing on the if() statement is just awry.  It seems to me that the base clock powerdown should happen when the if(Input != CLKINPUT_PD) is false.

Might I also suggest updating the forum to allow pasting of formatted code snippets?

/* Sets a CGU Base Clock clock source */
void Chip_Clock_SetBaseClock(CHIP_CGU_BASE_CLK_T BaseClock, CHIP_CGU_CLKIN_T Input, bool autoblocken, bool powerdn)
{
   uint32_t reg = LPC_CGU->BASE_CLK[BaseClock];

   if (BaseClock < CLK_BASE_NONE) {
      if (Input != CLKINPUT_PD) {
         /* Mask off fields we plan to update */
         reg &= ~((0x1F << 24) | 1 | (1 << 11));

         if (autoblocken) {
            reg |= (1 << 11);
         }
         if (powerdn) {
            reg |= (1 << 0);
         }

         /* Set clock source */
         reg |= (Input << 24);

         LPC_CGU->BASE_CLK[BaseClock] = reg;
      }
   }
   else {
      LPC_CGU->BASE_CLK[BaseClock] = reg | 1; /* Power down this base clock */
   }
}

[otsl]

Ok, I hate to keep harping on this, but I do thing you're missing the point.  The function (or documentation of the function) has a bug.

According to the documentation, this particular function serves two purposes:

1. Attach a base clock to an input source.

2. Power down a base clock.

It should be possible to do #2 WITHOUT doing #1.  In your example, not only do I powerdown the SPIFI clock, I also attach it to the 32kHz oscillator.  Sure, I can do this, but then, depending on which clock I'm powering down, I have to select a proper input clock.  I *could* use the 32kHz clock for most (unless it's USB0), however, this is contrary to the documented use of the function.

ALSO, if the function was intended to be used how you describe, why would the fall through of the first 'if' statement be to shut down a non-existent BaseClock:

if (BaseClock < CLK_BASE_NONE) {

   ....
}
else {
   LPC_CGU->BASE_CLK[BaseClock] = reg | 1; /* Power down this base clock */
}

This would try to set the power-down bit of a non-existent (BaseClock >= CLK_BASE_NONE) clock by writing to a portion of register space that is not defined.

I think it's far more reasonable to assume that this function is buggy, and the 'else' parameter specified above should be related to the 'if(Input != CLKINPUT_PD)' block.

If you had a public bugtracker for LPCOpen, I would submit this, but since you don't (that I can find), the forums seem to be my only way.

[jeremyzhou]

Hi otsl,

Thanks for your clarification.

I've already reported the bug and thanks again.
Have a great day,
Ping

-----------------------------------------------------------------------------------------------------------------------
Note: If this post answers your question, please click the Correct Answer button. Thank you!
-----------------------------------------------------------------------------------------------------------------------

[otsl]

I don't think that's the whole story.

For example, if (as per the documentation) I call:

Chip_Clock_SetBaseClock(CLK_BASE_SPIFI, CLOCKINPUT_PD, 1, 1);

I would expect the BASE_SPIFI clock to be powered down.  However, based on the code:

if (BaseClock < CLK_BASE_NONE)    <----- would evaluate to true

Followed by:

if (Input != CLKINPUT_PD)   <------- evaluate as FALSE

And therefore would not execute anything at all in the function (just simply exiting).

[jeremyzhou]

Hi otsl,

You can use the Chip_Clock_SetBaseClock(CLK_BASE_SPIFI, CLKIN_32K, 1, 1); instead of the previous code.

In my opinion, the CLKINPUT_PD seems like a sign which is used to get your attention to select a correct CGU Clock input.

Have a great day,
Ping

-----------------------------------------------------------------------------------------------------------------------
Note: If this post answers your question, please click the Correct Answer button. Thank you!
-----------------------------------------------------------------------------------------------------------------------

[jeremyzhou]

Hi otsl ,

Thanks for your sharing.

Obviously, the power down setting has been executed in the following code snippet, and the following comment is awry.

I'd like to report the Chip_Clock_SetBaseClock() for checking, I'll also inform you if I get some replies.

         if (powerdn) {
            reg |= (1 << 0);
         }

LPC_CGU->BASE_CLK[BaseClock] = reg | 1; /* Power down this base clock */

Have a great day,
Ping

-----------------------------------------------------------------------------------------------------------------------
Note: If this post answers your question, please click the Correct Answer button. Thank you!
-----------------------------------------------------------------------------------------------------------------------