AM6442: MCU-PLUS-SDK-AM64X: GPTIMER driver possible bug

Petar Donchev

Part Number: AM6442

Hello,
While reviewing gptimer.c I found an issue in this function:

static void GPTIMER_clrIRQStatus(uint32_t baseAddr, uint32_t mask)
{
    volatile uint32_t *addr = (volatile uint32_t *)(baseAddr + TIMER_IRQSTATUS);
    *addr |= mask;
}

According to the documentation:

The operation uses a read-modify-write OR. This register is write‑one‑to‑clear (W1C), so the correct action is a direct write of the mask, not an OR. While it may not cause functional issues right now, it adds a substantial number of additional CPU cycles.

Regards,
Peter

9 months ago

+1 Vaibhav Kumar 9 months ago

TI__Guru 58955 points

Hello Peter,

Petar Donchev said:
The operation uses a read-modify-write OR. This register is write‑one‑to‑clear (W1C), so the correct action is a direct write of the mask, not an OR. While it may not cause functional issues right now, it adds a substantial number of additional CPU cycles.

I have raised a bug for this. This is more of an improvement rather a bug, but it still makes sense to save few CPU Clock Cycles and perform a direct write.

Thank you for identifying this.

Marking the thread closed.

Kind Regards,

Vaibhav

0 Petar Donchev 9 months ago in reply to Vaibhav Kumar

Prodigy 60 points

Thank you, Vaibhav!
I consider this a bug because it clears all flags, not only those specified by the mask. The saved CPU clock cycles are approximately 250 (at 800MHz).
Regards,
Peter

0 Vaibhav Kumar 9 months ago in reply to Petar Donchev

TI__Guru 58955 points

Hi Petar,

Thanks for your comments.

I will check for other IRQSTATUS API going forward which requires a similar implementation on other peripheral fronts, like SPI, UART and so on.

Kind Regards,

Vaibhav

0 Petar Donchev 9 months ago in reply to Vaibhav Kumar

Prodigy 60 points

Hi Vaibhav,
Thank you!
There are also other functions in gptimer.c affected by the same issue.

static void GPTIMER_setIRQStatusEnable(uint32_t baseAddr, uint32_t mask)
{
    volatile uint32_t *addr =
                    (volatile uint32_t *)(baseAddr + TIMER_IRQSTATUS_SET);
    *addr |= mask;
}

Regards,
Peter

0 Vaibhav Kumar 9 months ago in reply to Petar Donchev

TI__Guru 58955 points

Yes, this is what I am also talking about. Thanks for pointing these out as well.

I appreciate you providing suggestions.

Processors

Processors forum

AM6442: MCU-PLUS-SDK-AM64X: GPTIMER driver possible bug