Issue in RT1060 SDK: CLOCK_GetSysPfdFreq() and CLOCK_GetUsb1PfdFreq() produce incorrect results

[udoeb]

Hi,

These functions divide the PLL frequency by the fraction configured in the PFD and then multiply the result by 18. This should be done the other way around to avoid truncation. For example:

uint32_t CLOCK_GetSysPfdFreq(clock_pfd_t pfd)
{
    uint32_t freq = CLOCK_GetPllFreq(kCLOCK_PllSys);

    switch (pfd)
    {
        case kCLOCK_Pfd0:
            freq /= ((CCM_ANALOG->PFD_528 & CCM_ANALOG_PFD_528_PFD0_FRAC_MASK) >> CCM_ANALOG_PFD_528_PFD0_FRAC_SHIFT);
            break;

        case kCLOCK_Pfd1:
            freq /= ((CCM_ANALOG->PFD_528 & CCM_ANALOG_PFD_528_PFD1_FRAC_MASK) >> CCM_ANALOG_PFD_528_PFD1_FRAC_SHIFT);
            break;

        case kCLOCK_Pfd2:
            freq /= ((CCM_ANALOG->PFD_528 & CCM_ANALOG_PFD_528_PFD2_FRAC_MASK) >> CCM_ANALOG_PFD_528_PFD2_FRAC_SHIFT);
            break;

        case kCLOCK_Pfd3:
            freq /= ((CCM_ANALOG->PFD_528 & CCM_ANALOG_PFD_528_PFD3_FRAC_MASK) >> CCM_ANALOG_PFD_528_PFD3_FRAC_SHIFT);
            break;

        default:
            freq = 0U;
            break;
    }
    freq *= 18U;

    return freq;
}

should be

uint32_t CLOCK_GetSysPfdFreq(clock_pfd_t pfd)
{
    uint64_t freq = CLOCK_GetPllFreq(kCLOCK_PllSys);
    freq *= 18U;

    switch (pfd)
    {
        case kCLOCK_Pfd0:
            freq /= ((CCM_ANALOG->PFD_528 & CCM_ANALOG_PFD_528_PFD0_FRAC_MASK) >> CCM_ANALOG_PFD_528_PFD0_FRAC_SHIFT);
            break;

        case kCLOCK_Pfd1:
            freq /= ((CCM_ANALOG->PFD_528 & CCM_ANALOG_PFD_528_PFD1_FRAC_MASK) >> CCM_ANALOG_PFD_528_PFD1_FRAC_SHIFT);
            break;

        case kCLOCK_Pfd2:
            freq /= ((CCM_ANALOG->PFD_528 & CCM_ANALOG_PFD_528_PFD2_FRAC_MASK) >> CCM_ANALOG_PFD_528_PFD2_FRAC_SHIFT);
            break;

        case kCLOCK_Pfd3:
            freq /= ((CCM_ANALOG->PFD_528 & CCM_ANALOG_PFD_528_PFD3_FRAC_MASK) >> CCM_ANALOG_PFD_528_PFD3_FRAC_SHIFT);
            break;

        default:
            freq = 0U;
            break;
    }

    return (uint32_t)freq;
}

I found this issue in the RT1060 SDK. Probably other SDKs are affected as well.

Best regards,
Udo

[jeremyzhou]

Hi，
Thank you for your interest in NXP Semiconductor products and for the opportunity to serve you.
Actually, I'm not very clear with the question, whether you share an instance for producing incorrect results when using the original function.
Have a great day,
TIC

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

- We are following threads for 7 weeks after the last post, later replies are ignored
Please open a new thread and refer to the closed one, if you have a related question at a later point in time.
-------------------------------------------------------------------------------

[udoeb]

Hi,

I thought it was clear because this is a classical (and well-known) mistake in integer math. Here is an example:

Assume PLL2 is set to 528 MHz and PFD1 is set to 1 (FRAC=18).

CLOCK_GetPllFreq(kCLOCK_PllSys) returns 528000000, so freq = 528000000

freq gets divided by PFD1_FRAC which is 18, so:

freq = freq / 18 = 528000000 / 18 = 29333333.33 which will be rounded down to freq = 29333333

Next, freq gets multiplied by 18:

freq = freq * 18 = 29333333 * 18 = 527999994

CLOCK_GetSysPfdFreq() returns 527999994 which is not correct. 

If integer arithmetic is used, multiplication must be done before the division. If the function is rewritten as indicated above, it returns 528000000 as expected.

Udo

[jeremyzhou]

Hi,
Thanks for your reply.
The output frequency is equal to Fvco*18/N, N can range from 12-35. It seems like your assumption is right, however, you miss a precondition that the PFD output clock should be divisible by the N.
If not, it should define a float variable to store the PFD output clock, meanwhile, the above two functions both fail to return a precise float value.
Lastly, it can't select an appropriate N to generate a 528 MHz, when we use the 24 OSC as the Fvco.
Have a great day,
TIC

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

- We are following threads for 7 weeks after the last post, later replies are ignored
Please open a new thread and refer to the closed one, if you have a related question at a later point in time.
-------------------------------------------------------------------------------

[udoeb]

Hi,

I don't understand your comment. Whether or not the output frequency is divisible by N is not relevant here. The function divides the input frequency by N.

I showed you that in the trivial case where N=18 the function returns an incorrect result. The expectation is that it returns Fvco in this case.

If the integer math would be done right (as I showed you) the result would be correct. No float math is needed here.

Udo

[stefanct]

Udo was 100% correct. This was silently fixed in the SDK version 2.16 two and a half years later(!). For some(?) new devices, e.g., MIMXRT1181, they now use double float calculations but cast the result in the end instead of rounding it correctly - but it's somewhat progress. m(