Error? XMC1100 library uses "SCU_CLK->CGATCLR0 |="

Tip / Sign in to post questions, reply, level up, and achieve exciting badges. Know more

cross mob
User7804
Level 4
Level 4
Hi all,

a snippet from xmc1_scu.c


void XMC_SCU_CLOCK_UngatePeripheralClock(const XMC_SCU_PERIPHERAL_CLOCK_t peripheral)
{
XMC_SCU_UnlockProtectedBits();
SCU_CLK->CGATCLR0 |= (uint32_t)peripheral;


IMO it should be "SCU_CLK->CGATCLR0 = (uint32_t)peripheral;", because CGATCLR0 is a clear register.

Oliver
0 Likes
8 Replies
Travis
Employee
Employee
First solution authored Welcome! 500 replies posted
Hi Oliver,

using "SCU_CLK->CGATCLR0 |= (uint32_t)peripheral;" instead is a safer way to prevent unnecessary setting of the register bit which will disable other clocks.
0 Likes
User7804
Level 4
Level 4
Hello Travis,

did you even read the documentation of CGATCLR0? It states:

Write one to selected bit to disable gating of corresponding clock, writing zeros has no effect.


As I wrote, CGATCLR0 is a write only clear register not intended to be used with "read-modify-write" instructions.

The general "Bit Function Terminology" documentation says: "The bit or bit field can only be written (write-only). A read to this register will always give a default value back."

Using "read-modify-write" is dangerous and plain wrong.

Oliver
0 Likes
Not applicable
Hi,
I certainly got concerned reading your note, because I allways use the "SCU_CLK->CGATCLR0 |= (uint32_t)peripheral;" version.
So I checked, and found that reading CGATCLR0 returns 0 both in the registers window in DAVE and in a statement like uint32_t test=SCU_CLK->CGATCLR0;

If this holds than the "SCU_CLK->CGATCLR0 |= (uint32_t)peripheral;" version should not run into a problem.
0 Likes
User7804
Level 4
Level 4
@smktec: Correct, I also observed that the register reads all zeros, and the reference manual states "Reset Value: 0000 0000 H".

Nevertheless it is wrong to use a RMF operation on registers made to avoid the need for RMF operations.

@Travis: Will you take care about this issue?

Oliver
0 Likes
jferreira
Employee
Employee
10 sign-ins 5 sign-ins First like received
Hi,

It will be fixed in the next version of the library planned for end of April.
Thanks for reporting.

Best regards,
Jesus
0 Likes
Travis
Employee
Employee
First solution authored Welcome! 500 replies posted
Hi obetz,

You are 100% right. This is a write only register and using |= is not recommended. The DAVE developer has taken note of this topic and fixes is on the way for this.

Therefore a temporary fix will be to use the below coding.

SCU_CLK->CGATCLR0 = (uint32_t)peripheral;


And to reconfirm your configuration, please check CGATSTAT0 (Peripheral 0 Clock Gating Status) using the debugger.
0 Likes
Travis
Employee
Employee
First solution authored Welcome! 500 replies posted
Hi all,

Just to ensure everything is ok.

SCU_CLK->CGATCLR0 |= (uint32_t)peripheral;

With regards to the use of this above mentioned instruction (which is inappropriate for WRITE only register) shall not cause any functional issue. This is because there are 2 independent registers SET and CLEAR registers to Enable or Disable the Clock Gating.


For example,

SCU_CLK->CGATCLR0 |= 0x0001;

Only bit field 0 which is VADC and SHS Gating will be disabled. The rest of the bit field or Gating will not be affected.
0 Likes
User7804
Level 4
Level 4
Hello Travis,

although I agree with your statement that RMF doesn't harm in this case, I disagree with the reasoning "because there are 2 independent registers SET and CLEAR registers".

The reason why RMF doesn't harm is that the register reads all zeros, and the reference manual states "Reset Value: 0000 0000 H" as stated 2016-02-11

But the read result of CGATCLR0 isn't explicitly stated in the manual, the information is from trying out and from guessing what "Reset Value: 0000 0000 H" could mean for a write only register.

Oliver
0 Likes